diff --git a/packages/cli/src/commands/review/submit.test.ts b/packages/cli/src/commands/review/submit.test.ts index 49c6cd90e2..e9e540bb8c 100644 --- a/packages/cli/src/commands/review/submit.test.ts +++ b/packages/cli/src/commands/review/submit.test.ts @@ -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 = {}) => + 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); + }); +}); diff --git a/packages/cli/src/commands/review/submit.ts b/packages/cli/src/commands/review/submit.ts index 2da1b6f1e5..866a58a18d 100644 --- a/packages/cli/src/commands/review/submit.ts +++ b/packages/cli/src/commands/review/submit.ts @@ -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, diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index 87d32c3f3b..3694460e23 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -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: `, 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://///pull/` — 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 `` is the same suffix as above (`pr-6740`, `local`, a filename) an - `, not posted ( Critical, Suggestion)` — **high or medium** effort without `--comment`/publish authorization (medium never posts — `--comment` forces high); `` is Approve / Request changes / Comment (a medium verdict never exceeds Comment — see Step 5). - `quick pass, not posted ( unverified findings)` — **low** effort only. +For any `posted` disposition, the line immediately **above** this one is `Posted: ` — 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.