Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions packages/cli/src/commands/review/cleanup.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)', () => {
Expand Down
13 changes: 8 additions & 5 deletions packages/cli/src/commands/review/cleanup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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));
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/skills/bundled/review/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -958,7 +958,7 @@ Run the bundled cleanup subcommand:
"${QWEN_CODE_CLI:-qwen}" review cleanup <target>
```

`<target>` is the same suffix used throughout (`pr-<n>`, `local`, or filename). The command removes the worktree at `.qwen/tmp/review-pr-<n>` (PR targets only), deletes the local branch ref `qwen-review/pr-<n>`, and clears any `.qwen/tmp/qwen-review-<target>-*` 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-<session>/` (the path from the `<skill-args>` 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.)
`<target>` is the same suffix used throughout (`pr-<n>`, `local`, or filename). The command removes the worktree at `.qwen/tmp/review-pr-<n>` (PR targets only), deletes the local branch ref `qwen-review/pr-<n>`, and clears any `.qwen/tmp/qwen-review-<target>-*` 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-<session>/` (the path from the `<skill-args>` 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.

Expand Down
Loading