mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-08-11 09:46:05 +00:00
feat(cli): surface the posted review link from /review submit (#8770)
The Create Review response's html_url was parsed for the receipt id and then dropped, so the terminal summary had no deterministic link to the review just posted — in the Web Shell there is no scrollback to recover it from. submit now relays html_url on the Posted stderr line and as `url` in the stdout JSON (best-effort, like the receipt), and SKILL.md requires the final summary to carry a `Posted: <url>` line before the fixed `Review complete:` line.
This commit is contained in:
parent
306bfa582a
commit
4a79517815
3 changed files with 74 additions and 3 deletions
|
|
@ -40,9 +40,10 @@ vi.mock('./lib/gh.js', async (importOriginal) => {
|
|||
});
|
||||
|
||||
const writeStdoutSpy = vi.hoisted(() => vi.fn((_line: string) => {}));
|
||||
const writeStderrSpy = vi.hoisted(() => vi.fn((_line: string) => {}));
|
||||
vi.mock('../../utils/stdioHelpers.js', () => ({
|
||||
writeStdoutLine: writeStdoutSpy,
|
||||
writeStderrLine: vi.fn(),
|
||||
writeStderrLine: writeStderrSpy,
|
||||
}));
|
||||
vi.mock('../../utils/version.js', () => ({
|
||||
getCliVersion: vi.fn().mockResolvedValue('0.21.2'),
|
||||
|
|
@ -96,6 +97,7 @@ beforeEach(() => {
|
|||
ghMock.mockClear();
|
||||
ghViewMock.mockClear();
|
||||
writeStdoutSpy.mockClear();
|
||||
writeStderrSpy.mockClear();
|
||||
process.exitCode = undefined;
|
||||
savedSessionId = process.env['QWEN_CODE_SESSION_ID'];
|
||||
delete process.env['QWEN_CODE_SESSION_ID'];
|
||||
|
|
@ -1090,3 +1092,52 @@ describe('submit receipt (producer half of the audit contract)', () => {
|
|||
expect(leftovers).toEqual([]);
|
||||
});
|
||||
});
|
||||
|
||||
// The link back to what was just written. GitHub's Create Review response
|
||||
// carries `html_url` — the deep link to the review — and submit relays it in
|
||||
// both channels, because a summary without it leaves the user to reassemble
|
||||
// the PR address by hand. Best-effort like the receipt: a response without it
|
||||
// (or an unparseable one) must never fail a review that DID post, and never
|
||||
// invents a link either.
|
||||
describe('the posted-review link', () => {
|
||||
const authorizedPost = (over: Record<string, unknown> = {}) =>
|
||||
args({ userAuthorized: true, ...over });
|
||||
const stdoutJson = () =>
|
||||
JSON.parse(writeStdoutSpy.mock.calls.at(-1)![0] as string);
|
||||
|
||||
let savedCwd: string;
|
||||
beforeEach(() => {
|
||||
savedCwd = process.cwd();
|
||||
process.chdir(dir);
|
||||
});
|
||||
afterEach(() => process.chdir(savedCwd));
|
||||
|
||||
it('relays html_url in the stdout JSON and the Posted line', () => {
|
||||
const url =
|
||||
'https://github.com/QwenLM/qwen-code/pull/6771#pullrequestreview-42';
|
||||
ghMock.mockImplementationOnce(() =>
|
||||
JSON.stringify({ id: 42, html_url: url }),
|
||||
);
|
||||
runSubmit(authorizedPost());
|
||||
expect(stdoutJson()).toMatchObject({ posted: true, url });
|
||||
const postedLine = writeStderrSpy.mock.calls
|
||||
.map((c) => c[0] as string)
|
||||
.find((l) => l.startsWith('Posted '));
|
||||
expect(postedLine).toContain(url);
|
||||
});
|
||||
|
||||
it('omits url when the response carries none — a link is relayed, never built', () => {
|
||||
ghMock.mockImplementationOnce(() => JSON.stringify({ id: 42 }));
|
||||
runSubmit(authorizedPost());
|
||||
expect(stdoutJson().posted).toBe(true);
|
||||
expect('url' in stdoutJson()).toBe(false);
|
||||
});
|
||||
|
||||
it('still reports posted:true when the response is unparseable', () => {
|
||||
// ghMock's default return is '' — JSON.parse throws, and both the receipt
|
||||
// and the link ride the same best-effort read of a post that succeeded.
|
||||
runSubmit(authorizedPost());
|
||||
expect(stdoutJson().posted).toBe(true);
|
||||
expect('url' in stdoutJson()).toBe(false);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -511,6 +511,21 @@ export function runSubmit(args: SubmitArgs, cliVersion = 'unknown'): void {
|
|||
'--input',
|
||||
'-',
|
||||
);
|
||||
// GitHub's answer, read best-effort: `id` feeds the bypass-audit receipt
|
||||
// below; `html_url` is the deep link to the review just created, surfaced in
|
||||
// both output channels so the summary the user reads can carry it — without
|
||||
// it, "view what was posted" means hand-assembling a PR URL.
|
||||
let reviewId: number | undefined;
|
||||
let reviewUrl: string | undefined;
|
||||
try {
|
||||
const parsed = JSON.parse(response) as { id?: number; html_url?: string };
|
||||
if (typeof parsed.id === 'number') reviewId = parsed.id;
|
||||
if (typeof parsed.html_url === 'string' && parsed.html_url.trim() !== '') {
|
||||
reviewUrl = parsed.html_url;
|
||||
}
|
||||
} catch {
|
||||
/* response metadata only — the post itself succeeded */
|
||||
}
|
||||
// Receipt for cleanup's bypass audit: EVERY review this session was
|
||||
// authorised to create, by id. The audit lists reviews by the reviewing
|
||||
// account inside the window and flags any the receipt does not vouch for —
|
||||
|
|
@ -525,7 +540,6 @@ export function runSubmit(args: SubmitArgs, cliVersion = 'unknown'): void {
|
|||
// add this one, dedupe, write back. Best-effort: a receipt failure must
|
||||
// never fail a review that DID post.
|
||||
try {
|
||||
const reviewId = (JSON.parse(response) as { id?: number }).id;
|
||||
if (typeof reviewId === 'number') {
|
||||
const receiptPath = tmpFile(`pr-${args.pr}`, 'submit-receipt.json');
|
||||
const priorIds = readReceiptIds(receiptPath);
|
||||
|
|
@ -542,7 +556,8 @@ export function runSubmit(args: SubmitArgs, cliVersion = 'unknown'): void {
|
|||
writeStderrLine(
|
||||
`Posted ${event} to ${args.repo}#${args.pr} — ${auth.why}` +
|
||||
(cappedBy.length ? ` (capped by ${cappedBy.join(', ')})` : '') +
|
||||
'.',
|
||||
'.' +
|
||||
(reviewUrl ? ` ${reviewUrl}` : ''),
|
||||
);
|
||||
writeStdoutLine(
|
||||
JSON.stringify(
|
||||
|
|
@ -551,6 +566,7 @@ export function runSubmit(args: SubmitArgs, cliVersion = 'unknown'): void {
|
|||
event,
|
||||
cappedBy,
|
||||
inlineComments: post.comments.length,
|
||||
...(reviewUrl ? { url: reviewUrl } : {}),
|
||||
},
|
||||
null,
|
||||
2,
|
||||
|
|
|
|||
|
|
@ -900,6 +900,8 @@ If the user responds with "post comments" (or similar intent like "yes post them
|
|||
|
||||
It also refuses a payload that contradicts itself — a body promising inline comments next to an empty `comments` array, a literal `\n` from building the JSON with `-f body=`, a `start_line` without its `side` fields — because GitHub accepts every one of those and the author is the one who finds out.
|
||||
|
||||
**On success, relay the link.** `submit`'s stdout JSON carries `url` — the `html_url` deep link GitHub returned for the review just created. Put it in your final summary on its own line, `Posted: <url>`, immediately **before** the machine-readable `Review complete:` line (which never carries it — Step 9 forbids putting anything on or after that line). This is the only way the user reaches what was just posted in one click: in the Web Shell there is no terminal scrollback to fish the stderr line out of, and a summary without the link reports a public write while hiding where it landed. If the stdout JSON has no `url` (GitHub answered without one), fall back to the PR page the run already knows — `https://<host>/<owner>/<repo>/pull/<n>` — rather than omitting the line; a resubmission after the 422 recovery relays the `url` of the review that actually posted, the last one.
|
||||
|
||||
**Why this is code and not a rule you remember.** The gate below is what this step used to be: a paragraph asking you to check, first, before anything else. It has now failed twice under dogfooding. Both runs reasoned their way to a verdict they wanted to file — one a public COMMENT on this skill's own PR, with no authorisation at all (measured; DESIGN.md — The self-filed COMMENT review (PR #6771)). That is the same failure the event and body had, for the same reason, and it has the same fix: the decision is a computed fact, so a subcommand computes it. Read the gate below to understand _what_ authorises a post; do not treat it as the thing that enforces one.
|
||||
|
||||
**The gate, for your understanding — `submit` is what enforces it.** Posting is a public, irreversible write to someone else's PR, so it happens ONLY on an explicit instruction, never as a courtesy or because a verdict "wants" to be filed. A run is authorised **only if** one of these is true:
|
||||
|
|
@ -1265,6 +1267,8 @@ where `<target>` is the same suffix as above (`pr-6740`, `local`, a filename) an
|
|||
- `<verdict>, not posted (<C> Critical, <S> Suggestion)` — **high or medium** effort without `--comment`/publish authorization (medium never posts — `--comment` forces high); `<verdict>` is Approve / Request changes / Comment (a medium verdict never exceeds Comment — see Step 5).
|
||||
- `quick pass, not posted (<N> unverified findings)` — **low** effort only.
|
||||
|
||||
For any `posted` disposition, the line immediately **above** this one is `Posted: <url>` — the review link `submit` returned (Step 7). The link rides its own line because the completion line's shape is fixed and scrapers must not have to strip a URL out of it.
|
||||
|
||||
**The word `posted` is a fact about this run, not a description of the verdict, and it is not yours to reason about.** Write it **only** if `qwen review submit` returned `{"posted": true}` in this run. That command is the one thing here that writes to the pull request, so its answer _is_ the fact — not the `gh api` call you did not make (Step 7 forbids it, and keying the contract on a call that can no longer happen would report every successful submission as `not posted`), and not the verdict you would have liked to file. If `submit` never ran, or refused (exit 3, `{"posted": false}`), or Step 7 was skipped entirely — the target is not a PR, the effort was low or medium — the disposition takes the `not posted` form, carrying the verdict you computed. **The posting gate and this line are the same fact stated twice; they cannot disagree.** A run has emitted `APPROVE posted` where nothing whatsoever was sent to GitHub (measured; DESIGN.md — The phantom APPROVE posted line). Nothing downstream can detect that: this line _is_ the completion contract that batch drivers and log scrapers read, so a review that files no approval and announces one has handed its wrapper a public approval that does not exist.
|
||||
|
||||
Everything before this line is for the human; this line is for machines — batch drivers, CI wrappers, and log scrapers detect run completion by `^Review complete: `, and dogfooding measured three different ad-hoc completion phrasings across one batch, each needing its own regex. Do not reword it, translate it, wrap it in markdown emphasis, or put text after it.
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue