diff --git a/packages/cli/src/commands/review/cleanup.test.ts b/packages/cli/src/commands/review/cleanup.test.ts index 690d0794c0e..25a29f4e57a 100644 --- a/packages/cli/src/commands/review/cleanup.test.ts +++ b/packages/cli/src/commands/review/cleanup.test.ts @@ -547,6 +547,20 @@ describe('runCleanup — bypass-write audit', () => { expect(warnings.join('\n')).toContain('review 501 (APPROVED)'); expect(warnings.join('\n')).toContain('no submit receipt vouches for it'); expect(warnings.join('\n')).not.toContain('review 500'); + // The footer leads with the benign explanation — a same-account write is + // usually external (you, a bot, or a concurrent workflow under the same + // login) — names the account, and qualifies the bypass claim instead of + // asserting a gate bypass outright. A concurrent same-account write on an + // observe-only run must not read as "you bypassed the submit gate". + expect(warnings.join('\n')).toContain('likely cause is benign'); + // Pin the interpolation SHAPE `(${me})`, not the bare word — the header + // also says "reviewing account", so `toContain('reviewer')` would stay + // green even if the account name were dropped from the footer. + expect(warnings.join('\n')).toContain('(reviewer)'); + expect(warnings.join('\n')).toMatch(/real bypass of that gate only if/); + // The relay instruction is the sentence that actually moves the warning to + // a human — the rest of the audit is inert without it, so pin it here. + expect(warnings.join('\n')).toContain('Relay this warning verbatim'); }); it('spares every review in a multi-id receipt (two sanctioned submits in one window)', () => { diff --git a/packages/cli/src/commands/review/cleanup.ts b/packages/cli/src/commands/review/cleanup.ts index efdccc5119d..230b17d0fd0 100644 --- a/packages/cli/src/commands/review/cleanup.ts +++ b/packages/cli/src/commands/review/cleanup.ts @@ -68,8 +68,8 @@ function isAutomationComment(body: string | null | undefined): boolean { * is used as the audit boundary. `fetchedAt` is a LOCAL timestamp compared * against GitHub's SERVER timestamps: a fast local clock would otherwise * hide bypass writes made in the first moments of the review. Two minutes - * errs toward over-flagging (fail-safe — the warning copy already covers - * "the user did this themselves"). + * errs toward over-flagging (fail-safe — the warning copy frames a flagged + * write as most likely an external same-account one, not a bypass). */ const CLOCK_SKEW_MS = 2 * 60 * 1000; @@ -356,9 +356,12 @@ function auditPrWrites(target: string, prNumber: string): void { ); } writeStdoutLine( - `warning: if the user did this themselves — or another workflow posts under the same ` + - `account — ignore this; otherwise a write bypassed the submit gate. ` + - `Relay this warning verbatim in the terminal summary.`, + `warning: The likely cause is benign — the user (from another terminal), ` + + `another workflow, or a bot posting under the same account (${me}) produces ` + + `exactly this shape. ` + + `\`/review\` writes to the PR only through \`qwen review submit\`; a write ` + + `here is a real bypass of that gate only if its content is this review's own ` + + `output. Relay this warning verbatim in the terminal summary so a human can judge.`, ); } catch (err) { skipNote(briefErrorLine(err)); diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index e96997a5e31..3f6f6015f5d 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -958,7 +958,7 @@ Run the bundled cleanup subcommand: "${QWEN_CODE_CLI:-qwen}" review cleanup ``` -`` is the same suffix used throughout (`pr-`, `local`, or filename). The command removes the worktree at `.qwen/tmp/review-pr-` (PR targets only), deletes the local branch ref `qwen-review/pr-`, and clears any `.qwen/tmp/qwen-review--*` side files (review JSON, PR context, presubmit / findings reports). It is idempotent — missing files are silent OK. For PR targets it first **audits the review window**: any issue comment the reviewing account posted — or edited — since `fetch-pr` opened the window (the boundary reaches back across drift restarts and a clock-skew allowance), and any **review** the account submitted that `submit`'s receipt does not vouch for, is flagged with `warning:` lines, because submit's one sanctioned write is receipt-recorded and never touches issue comments (Step 7's write ban) — so such a comment is a write that bypassed the gate, something the user did by hand, or **another workflow posting under the same account** (in CI the review shares the bot identity with precheck/triage; their marker-stamped comments are filtered out automatically, but the third reading stays real for anything unmarked). **Relay those `warning:` lines verbatim in your terminal summary** — the user can dismiss their own comment; a bypass they were never told about, they cannot. The audit is best-effort: when it cannot run (offline, unauthenticated, no report) it says so once on stderr — `note: bypass audit skipped (…)` — so a skipped audit is never mistaken for a clean one. Also remove `.qwen/tmp/qwen-review-parse-args.json` and the session args directory `.qwen/tmp/s-/` (the path from the `` note) — both are written before the target suffix is known, so the pattern above misses them. (Leave the args file in place if you had to fall back to writing it yourself and the run failed: it is the only record of what the review was actually asked to do.) +`` is the same suffix used throughout (`pr-`, `local`, or filename). The command removes the worktree at `.qwen/tmp/review-pr-` (PR targets only), deletes the local branch ref `qwen-review/pr-`, and clears any `.qwen/tmp/qwen-review--*` side files (review JSON, PR context, presubmit / findings reports). It is idempotent — missing files are silent OK. For PR targets it first **audits the review window**: any issue comment the reviewing account posted — or edited — since `fetch-pr` opened the window (the boundary reaches back across drift restarts and a clock-skew allowance), and any **review** the account submitted that `submit`'s receipt does not vouch for, is flagged with `warning:` lines, because submit's one sanctioned write is receipt-recorded and never touches issue comments (Step 7's write ban) — so such a comment is most likely an external same-account write — something the user did by hand from another terminal, or **another workflow posting under the same account** (in CI the review shares the bot identity with precheck/triage; their marker-stamped comments are filtered out automatically, but this reading stays real for anything unmarked) — and is a write that bypassed the gate only if its content is this review's own output. **Relay those `warning:` lines verbatim in your terminal summary** — the user can dismiss their own comment; a bypass they were never told about, they cannot. The audit is best-effort: when it cannot run (offline, unauthenticated, no report) it says so once on stderr — `note: bypass audit skipped (…)` — so a skipped audit is never mistaken for a clean one. Also remove `.qwen/tmp/qwen-review-parse-args.json` and the session args directory `.qwen/tmp/s-/` (the path from the `` note) — both are written before the target suffix is known, so the pattern above misses them. (Leave the args file in place if you had to fall back to writing it yourself and the run failed: it is the only record of what the review was actually asked to do.) This step runs **after** Step 7 and Step 8 to ensure all review outputs are saved before cleanup.