fix(review): guard prose routing against local-write intent, qualify scoping

The deep-verification round on ef86b03a99 measured one more hole in the
option-only class and one overclaim:

- F1: "review my local changes --fix" is prose with no classifiable
  target, so it still matched the routing trigger and resolution would
  disarm --fix. The clause now parses the verbatim input first and never
  resolves a PR into a verdict with fix.effective true — a mechanical
  guard, not a wording guess; prose asking for a local review is
  excluded even without --fix. The hash form's copy ambiguity is closed
  too: a # prefix is reference decoration, not part of the target. Four
  new parser cells pin the guard input, the PR-target contrast, the
  hash form, and the ./ escape-hatch boundary.
- F2: delta scoping on a plain re-review is high-only; the docs (both
  surfaces), the resume bullet, the routing clause, the --resume help,
  and the head-moved stderr now all carry the high-effort qualification.
- The ./ escape-hatch wording gains the measured precision: it holds
  when no PR-shaped token is present, not unconditionally.

DESIGN.md records the verification round; the PR body carries the same
boundaries in both languages.
This commit is contained in:
Shaojin Wen committed 2026-10-31 18:31:32 +08:00
1 parent ef86b03a99
commit 6a5372d6e4
8 files changed
+90 -17

No files matched your search

+2 -2
View File
@@ -248,7 +248,7 @@ It applies to **PR targets only** (a local review's diff comes from a live worki
Two things to know. With only the built-in target default, a continuation keeps the interrupted run's recorded **effort**. An explicit `--effort`, a project-remembered level, the operator `review.effort` setting, or an effective `--comment` supplies a required level; if it differs from the interrupted run, resume is refused and a fresh run starts at that level, because different effort is different work. And if the PR head moved while the review was down, the resume refuses (`head-moved`) and the fresh run reviews the new commits — which is what you want, and it counts as this review's one restart.
`--resume` is **not** the flag for a re-review after the PR gained commits: just run `/review 123` again. A plain re-review reads the previous round's anchor and scopes itself to the new commits (plus the files that import them), and it rules on every finding the previous round posted — `--resume` would only be refused for the moved head and spend the run's one restart on the way.
`--resume` is **not** the flag for a re-review after the PR gained commits: just run `/review 123` again. At high effort a plain re-review reads the previous round's anchor and scopes itself to the new commits (plus the files that import them); lower efforts review the full diff. Either way it rules on every finding the previous round posted — `--resume` would only be refused for the moved head and spend the run's one restart on the way.
## Findings as Data
@@ -422,7 +422,7 @@ The exit code is the contract a gate should read:
`3` (not `2`) lets a gate distinguish "the review is blocking" from "the tool broke" — yargs already uses `1` for usage errors — without parsing any output. `--timeout-minutes` (default 120, floored at 1) terminates a hung review and exits `1`, and cancelling the command (Ctrl+C / SIGTERM) terminates the review's process group rather than orphaning it.
`--resume` continues an interrupted review of the same PR instead of starting over — when a long local run dies part-way (a dropped connection, a timeout, a killed terminal), the retry would otherwise re-fetch, re-chunk and re-launch agents whose work is already on disk. It is safe to pass unconditionally on a retry: `fetch-pr` rules on the on-disk state itself (worktree still at the fetched SHA and clean, diff bytes unchanged, PR head unmoved, resume cap unspent) and silently falls back to a fresh review whenever anything no longer matches, so the flag never fails a run that could start over. When the current invocation has only the built-in target default, a continuation stays pinned to the interrupted run's recorded effort. An explicit `--effort`, a project-remembered level, the operator `review.effort` setting, or an effective `--comment` supplies a required level; a mismatch refuses the resume and runs fresh at that level. PR targets only (a local review's diff is captured from a live working tree, which has no stable interrupted state to continue). Resume is a **local convenience**: the repository's own CI review workflow does **not** resume — each retry re-runs fresh, because a CI attempt runs no-sandbox and its worktree is deleted on exit, leaving no interrupted state to continue. It is also not the flag for a re-review after the PR gained commits — a plain `/review <n>` scopes itself to the new commits and rules on the previous round's findings, while `--resume` would only be refused for the moved head (`head-moved`), spending the run's one restart.
`--resume` continues an interrupted review of the same PR instead of starting over — when a long local run dies part-way (a dropped connection, a timeout, a killed terminal), the retry would otherwise re-fetch, re-chunk and re-launch agents whose work is already on disk. It is safe to pass unconditionally on a retry: `fetch-pr` rules on the on-disk state itself (worktree still at the fetched SHA and clean, diff bytes unchanged, PR head unmoved, resume cap unspent) and silently falls back to a fresh review whenever anything no longer matches, so the flag never fails a run that could start over. When the current invocation has only the built-in target default, a continuation stays pinned to the interrupted run's recorded effort. An explicit `--effort`, a project-remembered level, the operator `review.effort` setting, or an effective `--comment` supplies a required level; a mismatch refuses the resume and runs fresh at that level. PR targets only (a local review's diff is captured from a live working tree, which has no stable interrupted state to continue). Resume is a **local convenience**: the repository's own CI review workflow does **not** resume — each retry re-runs fresh, because a CI attempt runs no-sandbox and its worktree is deleted on exit, leaving no interrupted state to continue. It is also not the flag for a re-review after the PR gained commits — a plain `/review <n>` at high effort scopes itself to the new commits (lower efforts review the full diff) and rules on the previous round's findings, while `--resume` would only be refused for the moved head (`head-moved`), spending the run's one restart.
A time-budgeted run can also export a **soft** deadline so the review stops its open-ended reverse-audit loop while there is still time to verify, compose and post: `QWEN_REVIEW_DEADLINE_EPOCH` is the Unix-seconds moment the run will be killed, and `QWEN_REVIEW_DEADLINE_RESERVE_SECONDS` (default 4800; `0` keeps only the round estimate) is the tail that must remain for the last round's verification, `compose-review` and submission. When the remaining budget no longer fits another round plus that tail, the round builder refuses to build it, and the composed verdict discloses the truncated audit (an otherwise-Approve verdict is capped at Comment). Without that variable the review uses the wall its plan recorded at capture: `--deadline <minutes>` on `qwen review fetch-pr` / `capture-local` / `plan-diff`, or by default **8 hours** on a small (Step 3A) diff, **12** on a large (3B) one and **16** on a huge one — sized by the diff's own lines, so a critical-posture fix-audit round keeps the wall its delta's size earns even though it fans out per territory and takes the chunked round cap. That default is a liveness bound — sized above what a healthy run at the round cap spends — so it ends a loop that has stopped converging and leaves a run that keeps working untouched; it is charged in elapsed time, idle included, so a session left overnight spends it, and because the next round is priced from the previous round's admission, a multi-hour pause between rounds can stop a run with time still on the wall (a `--resume` from a new session restarts both the wall and the pricing; **Review Deadline** below gives the ceilings). It stops the loop at the next round admission or verify build; it does not interrupt a build already running. A plan-recorded wall keeps a third of itself (capped at 4800 seconds, floored at the compose floor) as the tail reserve unless the reserve variable is set; a `--deadline` that cannot hold a convergence (two rounds plus that reserve — at or under ninety minutes, or more when the shell's reserve or compose-floor overrides raise the bar; that second leg is skipped while the environment exports a well-formed epoch, since the flag is inert then, and the wall it records is priced again in whatever shell a later env-less `--resume` runs in) is refused up front, and a wall just above it can still stop before round 1 once the fan-out and verification have spent it. When the wall can no longer admit a rebuild the coverage check would otherwise prescribe (a reverse-audit round, or a verifier shard under the compose floor), `compose-review` withholds that `FIX:` line and prints a `NOTE:` line saying so, with the gate's arithmetic; the gap is still disclosed in the posted body. The plan wall bounds the round builder and the verifier; two readers stay on the environment alone — the generated review workflow's launch allowance, and `qwen review run`'s own `--timeout-minutes` kill, which is not exported as a deadline. The plan stores the wall as a duration from the attempt's start, so a `--resume` from a new session renews it (a same-session resume continues the attempt and its wall; the flag itself is ignored on a resumed run — only its grammar and the default rule are checked — and the run says what wall the plan holds and how much of it is left, or that it has run out, and which builds the gates will now refuse; a wall can only run out on a same-session resume, or when the ledger failed to record the attempt, so the cheapest way past it is to resume from a new session, which renews the wall and keeps the worktree; a fresh review without `--resume` records a new wall too, and `--deadline none` on that fresh capture records none). `--deadline none` records no wall and leaves the loop bounded by the round cap alone — unless the environment exports an epoch, which clocks the run regardless. The `/review` slash command does not take the flag: it warns and drops it together with a value that could be one (whole minutes, or `none` — so a PR number typed right after it is dropped too), while any other value after it is disposed of like an invalid `--effort` value (a PR URL becomes the target; a typo is reported, and survives only as a sole file-shaped token), and a skill-driven run always uses the default. The huge tier's round reduction (3 rounds instead of 5) keys on an explicit deadline only — the environment's or the flag's — never on the default. A malformed environment deadline falls through to the plan's wall; a malformed plan field leaves the review ungated — the outer timeout still bounds the run.
+2 -1
View File
@@ -1143,7 +1143,8 @@ async function runFetchPr(args: FetchPrArgs): Promise<void> {
`Cannot resume PR #${prNumber} (${outcome.reason}); starting a fresh review.` +
(outcome.reason === 'head-moved'
? ' The PR gained commits since the interrupted run, so this fresh' +
' run IS the re-review — its incremental check scopes the delta.' +
' run IS the re-review — at high effort its incremental check' +
' scopes the delta (lower efforts review the full diff).' +
' After future updates use a plain `/review <n>` without' +
' --resume; this refusal spends the one head-movement restart.'
: ''),
@@ -509,6 +509,48 @@ const CASES: Case[] = [
warningCount: 1,
},
},
{
// Verification F1 on #15998: a prose LOCAL-write request keeps fix
// effective on its file target — the routing clause's mechanical guard
// reads this verdict and must never resolve a PR into it.
name: 'prose local-write request keeps --fix effective (routing guard input)',
raw: 'review my local changes --fix',
expect: {
targetType: 'file',
target: { type: 'file', path: 'review' },
fix: { requested: true, effective: true },
extraTokens: ['my', 'local', 'changes'],
warningCount: 1,
},
},
{
name: 'the same --fix on a PR target is ineffective (what resolution would do)',
raw: '15998 --fix',
expect: {
targetType: 'pr-number',
fix: { requested: true, effective: false },
extraTokens: [],
warningCount: 1,
},
},
{
name: 'a hash-prefixed number is not PR-shaped; the routing clause owns the rescue',
raw: '有更新了,重新评审 #15998',
expect: {
targetType: 'file',
extraTokens: ['#15998'],
warningCount: 1,
},
},
{
name: 'the ./ escape hatch does not survive a second PR-shaped token',
raw: './6711 6712',
expect: {
targetType: 'pr-number',
extraTokens: ['./6711'],
warningCount: 1,
},
},
{
name: 'numeric-prefix junk after /pull/ is not a PR URL (bug: /pull/42oops read as PR 42)',
raw: 'https://github.com/QwenLM/qwen-code/pull/42oops',
@@ -888,8 +888,9 @@ export function parseReviewArgs(
// order: prose typed before a PR number ("有更新了,重新评审 6711")
// classifies as a file path, and first-token-wins let the sentence steal
// the target while the number was demoted to extraTokens. An explicit
// path spelling (`./6711`) is not PR-shaped and still wins as a file
// target.
// path spelling (`./6711`) is not PR-shaped and stays a file target when
// no PR-shaped token is present; beside one, the PR wins and the path is
// disclosed as an extra.
const prIdx = targetTokens.findIndex(isPrShapedToken);
if (prIdx > 0) {
const [prToken] = targetTokens.splice(prIdx, 1);
+1 -1
View File
@@ -885,7 +885,7 @@ export const runCommand: CommandModule = {
type: 'boolean',
default: false,
describe:
'Continue an interrupted review of this PR when its on-disk state still matches, instead of starting over (PR targets only) — same meaning as `/review <pr> --resume`. Falls back to a fresh review when nothing can be resumed. Not for a re-review after new commits — that is a plain run whose delta the incremental check scopes.',
'Continue an interrupted review of this PR when its on-disk state still matches, instead of starting over (PR targets only) — same meaning as `/review <pr> --resume`. Falls back to a fresh review when nothing can be resumed. Not for a re-review after new commits — that is a plain run whose delta the incremental check scopes at high effort.',
})
.option('json', {
type: 'boolean',
@@ -1308,8 +1308,10 @@ Rounds 17–23 of #9659 produced 31 new Criticals with near-zero false positives
On 2026-10-31 a maintainer asked what happens when, after a completed round, the PR gains commits and the user types "有更新了,重新评审". Reading the entry path surfaced two gaps, neither visible from the pipeline side.
First, the parser does not refuse prose. `classifyToken`'s fallback classifies every non-flag, non-integer, non-URL token as a FILE target, and target selection took the first classifiable token — so the sentence becomes a file path and the round dies capturing it, and, worse, prose typed _beside_ a PR number steals the target while the number is demoted to `extraTokens` with only an "Ignoring extra argument(s)" warning. Reproduced as a red unit test before the fix (`有更新了,重新评审 6711` classified as `file`). The fix is deliberately not a prose heuristic — Unicode paths are legitimate file targets, and guessing which token is a "sentence" is the same guessing the parser exists to eliminate. Instead: PR-shaped tokens now outrank file-shaped ones regardless of arrival order (an explicit `./6711` spelling is not PR-shaped and still wins as a file), and a Step 1 intent-routing clause handles the no-target case — resolve the PR from the conversation, copy the explicitly named number or URL into a parser input file of the run's own, keep the CLI's args file as the record of what was asked, and ask when no unique PR is recoverable. The clause is the one sanctioned departure from the copy-verbatim rule, scoped to inputs carrying no classifiable target at all, so the transcribed-argument defense above stays intact.
First, the parser does not refuse prose. `classifyToken`'s fallback classifies every non-flag, non-integer, non-URL token as a FILE target, and target selection took the first classifiable token — so the sentence becomes a file path and the round dies capturing it, and, worse, prose typed _beside_ a PR number steals the target while the number is demoted to `extraTokens` with only an "Ignoring extra argument(s)" warning. Reproduced as a red unit test before the fix (`有更新了,重新评审 6711` classified as `file`). The fix is deliberately not a prose heuristic — Unicode paths are legitimate file targets, and guessing which token is a "sentence" is the same guessing the parser exists to eliminate. Instead: PR-shaped tokens now outrank file-shaped ones regardless of arrival order (an explicit `./6711` spelling is not PR-shaped and stays a file target when no PR-shaped token is present; beside one, the PR wins and the path is disclosed), and a Step 1 intent-routing clause handles the no-target case — resolve the PR from the conversation, copy the explicitly named number or URL into a parser input file of the run's own, keep the CLI's args file as the record of what was asked, and ask when no unique PR is recoverable. The clause is the one sanctioned departure from the copy-verbatim rule, scoped to inputs carrying no classifiable target at all, so the transcribed-argument defense above stays intact.
Second, `--resume` read as "re-review". Its description said "continue an interrupted review … instead of starting over" with nothing excluding the updated-PR case, and a resume refused with `head-moved` spends the run's one head-movement restart without a word about either fact. The description and the resume verdict bullet now name the exclusion (a re-review after new commits is a plain run whose delta the incremental check scopes), and the refusal stderr teaches the same at the moment of misuse. The restart-bound semantics are deliberately unchanged: the fresh fall-through run IS the post-head-movement restart, and charging it is the existing design, not a defect this incident established.
The first review round (#15998) then caught two defects in the routing clause itself, both reproduced against the exact commit before fixing. **Option-only hijack:** the clause's condition — non-empty, no classifiable target — literally matches `/review --fix` and `/review --effort low`, supported local invocations; resolving the conversation's last PR into them would retarget a working-tree run and disarm `--fix` (effective only on local/file targets). The clause now names both non-intent shapes explicitly — bare and option-only — and leaves them to the parser verbatim. **Authorization break:** the clause promised that a `--comment` carried through the resolution "keeps its posting authority", but the write gate re-parses the CLI's verbatim record on purpose — a model-written resolution is exactly the untrusted source that design refuses — so a prose record ends the run at `submit` with `unbound` after the whole review ran. The promise was false and is gone: posting is now the resolution's hard boundary. When the resolved verdict has `comment.effective` true, the skill presents the resolved invocation and asks the user to re-invoke it explicitly — that invocation's CLI record authorises normally — instead of running a review whose submission is already refused. Making the resolution itself an authorization source would mean trusting a model-written file at the write gate, which is the one thing the gate exists to refuse; the boundary, not a new trust channel, is the fix. Both shapes are pinned: the parser contract for option-only invocations, the gate's `unbound` refusal of a prose record beside the explicit re-invocation's acceptance, and the clause's boundary sentences. The round's non-blocking nit also proved real once pinned as a test: the PR-shape reorder moved a malformed URL behind the assigned target, where it lost its specific "not a PR/CR URL" diagnostic to the generic extra-argument line — a small regression the reorder introduced. The invalid-url refusal is now hoisted ahead of the target-assigned branch, so the diagnostic is position-independent, and a test pins both its presence and the absence of the generic line beside it.
The deep-verification round (head `ef86b03a99`) then measured one more hole in the same class and one overclaim. **F1 — prose local-write hijack:** the option-only exclusion named the flags-alone spelling, but "review my local changes --fix" is prose with no classifiable target, so it still matched the trigger and resolution would disarm `--fix` exactly as before — one prose word past the fixed boundary. The fix is keyed on the parser verdict, not on wording: the clause now parses the verbatim input first and never resolves a PR into a verdict with `fix.effective: true` (a local-write invocation whatever its prose says); prose asking for a LOCAL review is excluded even without `--fix`. The same round's `#15998` cell exposed a copy ambiguity — the hash form is not PR-shaped, and "copy it, do not reconstruct it" left the `#` undecided — so the clause now states that a `#` prefix is reference decoration, not part of the target. **F2 — scoping overclaim:** "a plain re-review scopes the delta" is true only at high effort (the incremental cache is high-only); the docs, the resume bullet, the `--resume` help, and the head-moved stderr now all carry the high-effort qualification. The `./` escape-hatch wording gained the same precision the sibling cells measured: it holds when no PR-shaped token is present, not unconditionally.
@@ -37,7 +37,7 @@ You are an expert code reviewer. Your job is to review code changes and provide
Your goal here is to understand the scope of changes so you can dispatch agents effectively in Step 3.
**Route conversational intent before the parser.** A request like "有更新了,重新评审" / "re-review after the update" is intent, not arguments: it carries no target the parser could classify, and the parser will not refuse it — every non-flag, non-number, non-URL token classifies as a FILE target, so the prose becomes a file path and the round dies capturing the user's sentence; typed beside a PR number, the prose arrives first and steals the target while the number is demoted to `extraTokens` (measured; DESIGN.md — The prose re-review request). When a non-empty request holds no classifiable target — no PR number, no PR URL, no file path — resolve the target from the conversation (the PR this session last reviewed), announce the resolution in the output language, and run the parser on the resolved invocation — the target plus any flags the input already carries — instead of the prose: write that invocation to a file of your own under `.qwen/tmp/`, keep the CLI's args file untouched (it is the record of what was asked), pipe your file to the parser, remove it once the parser has read it (no Step 9 sweep knows that name), and say you did. The resolved target is a number or URL the conversation explicitly names — copy it, do not reconstruct it. This is the one sanctioned departure from the copy-verbatim rule below, and it is narrow: verbatim governs the arguments the user gave FOR a review, and any input already carrying a number, URL, or path goes to the parser untouched. Two shapes are NOT intent and never reach this resolution: a bare `/review` with no arguments is a local review, not intent (the empty-stdin rule below owns it), and an option-only invocation — flags and their values alone, like `/review --fix` or `/review --effort low` — is a supported local review the parser owns verbatim; resolving a PR into it would silently retarget the user's working-tree run and disarm `--fix`. When the conversation names no unique PR, ask which one — never guess, and never pass the prose through. **Posting is the resolution's hard boundary.** Step 7's write gate re-parses the CLI's verbatim record on purpose — a model-written resolution is not an authorization source — so a prose record cannot authorise posting even when the resolved invocation carries `--comment`. Read the verdict after parsing: when `comment.effective` is true (the request carries `--comment`, or the `review.comment` setting is on), do not run a review whose submission is already refused — present the resolved invocation (e.g. `/review <n> --comment`) and ask the user to re-invoke it explicitly; that invocation's record authorises normally. A non-posting re-review runs end-to-end on the resolved target. A re-review after new commits is a plain run: the incremental check below scopes the delta, and `--resume` is never the flag for it — that continues an interrupted run at an UNMOVED head.
**Route conversational intent, guarded by the parser.** A request like "有更新了,重新评审" / "re-review after the update" is intent, not arguments: it carries no target the parser could classify, and the parser will not refuse it — every non-flag, non-number, non-URL token classifies as a FILE target, so the prose becomes a file path and the round dies capturing the user's sentence; typed beside a PR number, the prose arrives first and steals the target while the number is demoted to `extraTokens` (measured; DESIGN.md — The prose re-review request). Resolution is for requests to review a PULL REQUEST again, and it is guarded, not free: run the parser on the verbatim input first, and resolve only when the input is such a request and holds no classifiable target — no PR number, no PR URL, no file path. Three shapes never reach resolution: a bare `/review` with no arguments is a local review (the empty-stdin rule below owns it); an option-only invocation — flags and their values alone, like `/review --fix` or `/review --effort low` — is a supported local review the parser owns verbatim; and the mechanical guard — when the original verdict has `fix.effective: true`, the input is a local-write invocation whatever its prose says ("review my local changes --fix" is the measured shape), so run it as parsed or ask, and never resolve a PR into it: `--fix` is effective only on a non-PR target, and resolution would disarm it. Prose that asks for a LOCAL review is not re-review intent even without `--fix` — run the local review or ask. When resolution does apply, resolve the target from the conversation (the PR this session last reviewed), announce the resolution in the output language, and run the parser on the resolved invocation — the target plus any flags the input already carries: write that invocation to a file of your own under `.qwen/tmp/`, keep the CLI's args file untouched (it is the record of what was asked), pipe your file to the parser, remove it once the parser has read it (no Step 9 sweep knows that name), and say you did. The resolved target is a number or URL the conversation explicitly names — copy it, do not reconstruct it; a `#` prefix is reference decoration, not part of the target (`#15998` names PR 15998). This is the one sanctioned departure from the copy-verbatim rule below, and it is narrow: verbatim governs the arguments the user gave FOR a review, and any input already carrying a number, URL, or path goes to the parser untouched. When the conversation names no unique PR, ask which one — never guess, and never review the prose as a file path. **Posting is the resolution's hard boundary.** Step 7's write gate re-parses the CLI's verbatim record on purpose — a model-written resolution is not an authorization source — so a prose record cannot authorise posting even when the resolved invocation carries `--comment`. Read the verdict after parsing: when `comment.effective` is true (the request carries `--comment`, or the `review.comment` setting is on), do not run a review whose submission is already refused — present the resolved invocation (e.g. `/review <n> --comment`) and ask the user to re-invoke it explicitly; that invocation's record authorises normally. A non-posting re-review runs end-to-end on the resolved target. A re-review after new commits is a plain run: at high effort the incremental check below scopes the delta (lower efforts review the full diff), and `--resume` is never the flag for it — that continues an interrupted run at an UNMOVED head.
**Do not parse the arguments yourself — run the parser. And do not retype them — they are already in a file.** The flag grammar (`--comment`, `--effort <level>`, `--effort=<level>`) and the target disambiguation are deterministic, and three separate parsing bugs shipped while they lived here as prose. The tested implementation is a subcommand, and it reads the argument string **on stdin from a file — never as a positional shell argument, and never inline in shell syntax**: a raw string that begins with a flag (`/review --effort low`) is eaten by the CLI's own argument parsing before the subcommand runs (`Unknown argument: effort low`); one containing a quote or `$(...)` is mangled by the shell; and a heredoc is not safe either — the delimiter is recognized inside the content, so a raw string carrying that exact line would terminate the heredoc early and hand the rest to the shell as commands. A file crosses the boundary with zero shell parsing of the content.
@@ -75,7 +75,7 @@ It prints a JSON verdict; use it **verbatim**:
- `fix.requested` / `fix.effective` — `--fix` is `--comment` reflected, and gated on the opposite target. `--comment` writes to a **pull request**, so it needs one; `--fix` writes to a **working tree**, so it needs one that outlives the review. A PR review's tree is the ephemeral worktree `fetch-pr` creates and Step 9 deletes, so `--fix` on a PR target is ignored with a warning — edits there are discarded minutes later, and reporting findings as "fixed" into a directory that no longer exists is worse than not fixing them. `effective` is what gates Step 6B. An effective `--fix` also floors the effort at **medium**: it edits the user's files, and low runs no verification, so applying an unverified finding is the same mistake as posting one, aimed at their working tree instead of a pull request. It does not force **high** — medium's findings are verified, and the reverse audit high adds hunts for findings that are _missing_, which is not what deciding whether to apply one turns on.
- `severityFloor` + `severityFloorSource` — the posting floor for a PR review: `critical` posts only Criticals (otherwise-postable high-confidence Suggestions are recorded and deferred — Step 6's convergence posture — and so is the one Critical shape the floor defers by its axes, fails-closed on new surface; low-confidence and Nice-to-have findings stay terminal-only as ever), `suggestion` posts Criticals and Suggestions at every round, and `auto` — the default — is the **round-adaptive rule you resolve in Step 6**, where the round is known: `suggestion` through round 5, `critical` from round 6 — **or `critical` from any round once the recovered ledger's `flatRounds` streak has reached its bar** (Step 6's signal-driven trigger: the first-time-finding rate has not fallen for that many consecutive rounds, so the loop is re-deriving the same set and the floor stems it early). The parser cannot resolve `auto` itself (the round comes from the previous posted round's ledger, not fetched yet), so carry the verdict's value forward and resolve it there. Explicit flag beats the `review.severityFloor` setting beats `auto`; a non-PR target has no rounds, so the flag warns and is ignored there. The floor governs what the review **posts**, never what it finds, verifies, or reports in the terminal.
- `topology` + `topologySource` — the shape of the run. `auto` (the default) runs the standing effort-driven pipeline described below. `minimal` runs the single-pass A/B comparison arm (Step 3M) instead — and when it is set, it OVERRIDES the effort dispatch entirely. In this step you run `parse-args` and the **diff capture only** (`fetch-pr` for a same-repo PR, the lightweight `fetch-diff` for a cross-repo PR, or the local capture for a local/file target — exactly as below), then jump straight to **Step 3M**. You SKIP the rest of Step 1's setup — the rules load, `pr-context`, `comment-status`, and the incremental-cache check — and you skip the fan-out, verification, reverse audit, and posting. `minimal` is terminal-only; the parser has already forced `comment.effective`, `fix.effective`, and `resume.effective` to false, and its warnings for that are in `warnings`. There is no configured topology — it is only ever an explicit flag.
- `resume.requested` / `resume.effective` — `--resume` continues an interrupted run of the same PR instead of starting over. `effective` is what gates the resume branch below, and it is a TARGET-SHAPE gate rather than a promise: a cross-repo `pr-url` with no matching remote is `effective: true` but routes to lightweight mode, which never calls `fetch-pr` — item 3 below owns telling the user the flag is inert there. `requested && !effective` means a local or file target, or `--topology minimal` (a fresh single pass neither continues nor consumes an interrupted run), already warned in `warnings`. The resolved effort source controls continuity: a target default is omitted so the interrupted run stays pinned to its recorded level; an explicit, remembered, configured, or comment-forced level is passed through, and a mismatch makes `fetch-pr` refuse the resume and run fresh at that required level. It is never the flag for a re-review after the PR gained commits: that is a plain run whose delta the incremental check scopes, and a moved head refuses the resume (`head-moved`), spending the run's one head-movement restart.
- `resume.requested` / `resume.effective` — `--resume` continues an interrupted run of the same PR instead of starting over. `effective` is what gates the resume branch below, and it is a TARGET-SHAPE gate rather than a promise: a cross-repo `pr-url` with no matching remote is `effective: true` but routes to lightweight mode, which never calls `fetch-pr` — item 3 below owns telling the user the flag is inert there. `requested && !effective` means a local or file target, or `--topology minimal` (a fresh single pass neither continues nor consumes an interrupted run), already warned in `warnings`. The resolved effort source controls continuity: a target default is omitted so the interrupted run stays pinned to its recorded level; an explicit, remembered, configured, or comment-forced level is passed through, and a mismatch makes `fetch-pr` refuse the resume and run fresh at that required level. It is never the flag for a re-review after the PR gained commits: that is a plain run whose delta the incremental check scopes at high effort (lower efforts review the full diff), and a moved head refuses the resume (`head-moved`), spending the run's one head-movement restart.
- `warnings` — surface every entry to the user, word for word. When a warning says the last explicitly typed effort was reused, relay it as the opening line before starting the review.
- `extraTokens` / `unknownFlags` — leftover input the parser refused to guess about; mention them to the user rather than silently dropping them.
@@ -943,22 +943,49 @@ describe('bundled review skill', () => {
// departure from copy-verbatim, so pin its boundary sentences too —
// widening it re-opens the transcribed-argument incident.
const body = skillBody();
expect(body).toContain('Route conversational intent before the parser');
expect(body).toContain('never guess, and never pass the prose through');
expect(body).toContain(
'Route conversational intent, guarded by the parser',
);
expect(body).toContain(
'never guess, and never review the prose as a file path',
);
expect(body).toContain(
'any input already carrying a number, URL, or path goes to the parser untouched',
);
expect(body).toContain("keep the CLI's args file untouched");
expect(body).toContain('copy it, do not reconstruct it');
// The routing fires on a NON-EMPTY request only: a bare `/review` is a
// local review, and the empty-stdin rule owns it. Without this boundary
// the clause reads as "no classifiable target → resolve a PR", which
// would hijack every bare local review into the last PR.
// Resolution is for PR re-review requests only, and a bare `/review`
// stays a local review. Without these boundaries the clause reads as
// "no classifiable target → resolve a PR", hijacking local runs.
expect(body).toContain(
'When a non-empty request holds no classifiable target',
'Resolution is for requests to review a PULL REQUEST again',
);
expect(body).toContain(
'a bare `/review` with no arguments is a local review, not intent',
'holds no classifiable target — no PR number, no PR URL, no file path',
);
expect(body).toContain(
'a bare `/review` with no arguments is a local review',
);
// Verification F1 (prose local-write hijack): "review my local changes
// --fix" matches the no-target trigger, and resolving a PR into it
// disarms --fix. The guard is mechanical — the ORIGINAL verdict's
// fix.effective — not a wording guess; prose local intent without
// --fix is excluded too. The hash-decoration rule closes the "#15998"
// copy ambiguity the same report measured.
expect(body).toContain(
'when the original verdict has `fix.effective: true`',
);
expect(body).toContain('never resolve a PR into it');
expect(body).toContain(
'Prose that asks for a LOCAL review is not re-review intent',
);
expect(body).toContain(
'a `#` prefix is reference decoration, not part of the target',
);
// Verification F2: delta scoping is high-only; the clause must not
// promise it to lower efforts.
expect(body).toContain(
'at high effort the incremental check below scopes the delta',
);
// The resolved invocation keeps the flags the input already carries,
// so the parser still sees them — but carrying a flag is NOT posting