diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index bd1ba96b6e2..30b954c0de7 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -173,13 +173,13 @@ Based on the parsed `target.type`: Worktree isolation: all subsequent steps (agents, build/test) operate inside `worktreePath`, not the user's working tree. Cache and reports (Step 8) are written to the **main project directory**, not the worktree. - **Incremental review check** (high effort only — neither low nor medium consults or updates the cache): read `.qwen/review-cache/pr-.json` **before** `fetch-pr` (it is a local file; nothing about it needs the fetch) and, when it holds a `lastCommitSha`, pass BOTH fields to the fetch verbatim: `--since --since-model ` (omit `--since-model` when the cache has no `lastModelId`; do not substitute anything for it). **Copy them; do not compare them to anything.** The same-model gate is ruled inside `fetch-pr`, over the identity the runtime published — "clean up to `lastCommitSha`" is the recorded identity's verdict, and the command validates an anchor against the HISTORY, never against who certified it, so an anchor from another identity is ancestrally perfect and would scope this round past code it never reviewed. A hand-applied version of that gate was wrong every time it was written, because `{{model}}` interpolates the BARE model id while every identity the CLI records is provider-qualified: two provider configurations exposing one model name compared equal and passed each other's gate. When the gate refuses, the report says `cross-model-anchor` and the round reviews the full diff. Read the cache's `findings` ledger either way (Step 6 owes each entry a ruling; the work list carries across models, only the anchor does not). **You never run `git` against an anchor yourself** — no `git diff ..HEAD`, no `cat-file`, no `merge-base --is-ancestor`: the command validates the anchor against the fetched history and computes the scoped diff and chunk plan in one pass, because a hand-run check is one a run can skip, and the hand-computed delta was exactly the shape this skill forbids everywhere else (the diff is a file the CLI writes, never a command you run). The report's `incremental` field is the decision; act on it with `lastModelId` from the cache and the current model ID (`{{model}}`): - - `effective: true` (no `upToDate`) → the report's diff and plan ARE the incremental scope (`since..head`); continue with them exactly as with a full plan. The file set is **widened by one import hop**: a still-clean source file that imports a changed one re-enters the scope with its own full-range hunks, because the round before cleared it against the callee's OLD shape. `incremental.scope` names each file's class — `deltaFiles` (touched since the anchor), `interaction[]` (widened back in, each with the edges that did it), `contextFileCount` (weighed and passed over) — and a chunk brief built for an interaction file points its agent at that seam instead of a from-scratch re-review. On a round the capture resolved to the **critical posting posture** the report also carries `incremental.posture: "critical"` with its `postureCause` (`round`, `flat-trend`, or `explicit` — the same facts Step 6's floor resolution reads, predicted from the side file and the CLI-recorded invocation), and the round runs the **fix-audit shape**: see Step 1's topology note and Step 5's posture narrowing. **Also read the cache's `findings` ledger** (older caches have none — then there is nothing to track): these are the previous round's findings with their ids, and Step 6 owes each of them a ruling this round. (Reachable only under a matching identity: the gate inside the command is what keeps a cross-model anchor from scoping anything.) + - `effective: true` (no `upToDate`) → the report's diff and plan ARE the incremental scope (the sliced scope the report publishes: `deltaFiles`, widened by `interaction[]`); continue with them exactly as with a full plan. The file set is **widened by one import hop**: a still-clean source file that imports a changed one re-enters the scope with its own full-range hunks, because the round before cleared it against the callee's OLD shape. `incremental.scope` names each file's class — `deltaFiles` (touched since the anchor), `interaction[]` (widened back in, each with the edges that did it), `contextFileCount` (weighed and passed over) — and a chunk brief built for an interaction file points its agent at that seam instead of a from-scratch re-review. On a round the capture resolved to the **critical posting posture** the report also carries `incremental.posture: "critical"` with its `postureCause` (`round`, `flat-trend`, or `explicit` — the same facts Step 6's floor resolution reads, predicted from the side file and the CLI-recorded invocation), and the round runs the **fix-audit shape**: see Step 1's topology note and Step 5's posture narrowing. **Also read the cache's `findings` ledger** (older caches have none — then there is nothing to track): these are the previous round's findings with their ids, and Step 6 owes each of them a ruling this round. (Reachable only under a matching identity: the gate inside the command is what keeps a cross-model anchor from scoping anything.) - `upToDate: true` **and** `comment.effective` is false (no `--comment` flag, and `review.comment` not enabled in settings) → inform the user "No new changes since last review" (this branch consumes no plan, so it holds even when `diffPath` is null). **Before the cleanup, write the stop sidecar** so `qwen review run` reads the round as DECIDED instead of exiting 1 "Review did not complete" over it: when the environment carries `QWEN_REVIEW_RUN_ID`, write `.qwen/tmp/qwen-review-pr--stop.json` containing exactly `{"reason": "", "runId": ""}` — the same reason+runId contract `capture-local` writes for local stops, runId copied verbatim (the parent's reader is nonce-fenced and discards any other stamp); without that variable no parent is reading and the file is not written. `cleanup` deliberately KEEPS this run's sidecar (it spares a `stop.json` whose `runId` matches the environment) so the parent can still read the decision after the child exits — do not remove it by hand; the next run's cleanup collects it. Then run `"${QWEN_CODE_CLI:-qwen}" review cleanup pr-` to remove the worktree just created, and stop. **This branch does not apply on a resumed run** (`resumed: true` from the resume branch below): a continuation's `incremental` field is the interrupted attempt's history, not this run's decision, and taking the stop/cleanup here would destroy the very state `--resume` reused. - `upToDate: true` **but** `comment.effective` is true (the `--comment` flag or the `review.comment` setting) → run the full review anyway — the report already holds the full-range diff and plan for exactly this flow, unless `diffPath` is null, which is the ordinary degraded state (partial coverage, disclosed) rather than a scoping fact. Inform the user: "No new code changes. Running review to post inline comments." - `reason: cross-model-anchor` → the cached anchor was certified by another identity, so it was not used. Continue on the full-range plan (or, when `diffPath` is null, on the degraded state its siblings name). The command already said which identity certified it and which is running; repeat that to the user rather than restating it from the cache. - `effective: false` → the anchor was refused and the report says why. **Every reason names a CAUSE** — `not-an-ancestor` (a rebase or force-push); `unknown-commit`; `behind-merge-base` (the base moved past the anchor, e.g. a partial merge landed, and scoping to it would review base history the PR does not contain); `nothing-to-narrow` (the narrowing found nothing it could publish — all deterministic and all safe, because the round keeps the full range: an ordinary "undo per feedback" revert that puts lines back the way the base had them, so the PR's own diff no longer displays the undone FILE at all (a file the PR still displays does not refuse — the join fails closed and publishes its section whole instead); a capture on either side whose bytes do not survive a UTF-8 round trip; a delta the parser cannot read; and a fail-closed refusal where the two captures key the same change differently — a path or a rename git resolves differently across the two ranges — so narrowing would drop a change the PR's diff displays); `base-untrusted` (the base could not be fetched, so the clamp that keeps an anchor from scoping wider than the PR's diff could not be ruled); `capture-failed` (a capture threw, or the base fetch or merge-base resolution failed); `partition-failed` (the diff would not tile). **Whether a PLAN exists is a separate field: `diffPath`.** Non-null → the diff and plan are the full range; continue as a full review. Null → no diff exists at all: that is the `diffPath: null` degraded state (partial coverage, disclosed), whatever the reason says. Do not read one field for both facts — a reason that meant "planless" as well as "why" is what put deterministic refusals into the retry class below. The previous round's ledger is still owed its rulings in every refusal. - - **When the cache has no anchor, the PR itself carries one** (high effort only, same as the cache). The file being absent is the NORMAL state everywhere except the machine that ran the last review — CI, another clone, a colleague's checkout — and it used to mean the incremental range silently degraded to the full diff every time, which is precisely the cost incremental review exists to avoid. The anchor now rides the posted review: the machine ledger's marker carries `sha`, the head the last clean round reviewed, and `pr-context` writes it into the side file `qwen-review-pr--prev-ledger.json` with the rest of the ledger. So when the cache had no anchor to pass — including the case where it HELD one that the cache-path gate withheld, because `lastModelId` was another model's: the marker may carry an anchor THIS model certified, and a round that stops at the cache would never look — **or the anchor it passed was refused** (`incremental.effective: false` — a rebase or force-push retires a cached anchor exactly when another environment may have posted a newer round whose marker still holds a valid one): proceed with the setup batch as usual, and when the side file lands with a `sha` — **different from the one already refused, OR the same sha when the refusal was infrastructure** (`base-untrusted`, `capture-failed`: the anchor was never ruled invalid, and the component that failed — a base fetch, a merge-base resolution, a capture — is re-run by the re-run. One shape of `capture-failed` retries ONCE, not forever: a base-less refusal (a null `mergeBaseSha`) means the base fetch failed (`baseFetchFailed: true`) and no local base ref remained, or `git merge-base` itself failed on a non-answer exit. The failed component IS re-run by the re-run, but the exit status cannot split the members — git exits 128 identically for a transient fetch fault and for a deterministic refusal (the base branch deleted on the remote — the refspec fetch fails every time), and the merge-base probe folds its surface failures the same way — so a second refusal of the same shape on the same sha is the deterministic member. Retry that one, once. Every other reason is deterministic for the same sha and must NOT be retried: a validity refusal re-refuses; a planless `partition-failed` always carries a `mergeBaseSha` — with no base nothing is captured and an empty diff cannot fail to tile — so both ranges were in hand and both refused to tile, which the re-run reproduces exactly, do not retry it; `nothing-to-narrow` re-narrows identically: the same two captures select the same hunks, and a capture that failed a UTF-8 round trip fails it again — and its base-less shape (a null `mergeBaseSha` with `baseFetchFailed: false`) is NOT retryable: the fetch succeeded and `git merge-base` found no common ancestor at all (a cross-fork PR with unrelated history), which a re-run reproduces exactly) —, **re-run the `fetch-pr` command from above with `--since ` — REPLACING any `--since` it already carries, never appending a second one** (a repeated flag is one flag with two values; the CLI takes the last, but a command that reads as two anchors is a command nobody can check) — the PR ref is already fetched so the re-run is cheap, and it rebuilds the worktree, diff and chunk plan scoped to the delta, with the validation the old flow asked you to hand-run (`cat-file`, `merge-base --is-ancestor`) inside the command where it cannot be skipped. Then act on the new report's `incremental` field exactly as the cache path above does (**the same-model gate on this path is RULED FOR YOU, not left to you to apply**: the marker carries `model` beside its `sha` — the identity that certified the range — and `pr-context`'s ledger section states the verdict outright, either "the same-model contract HOLDS" or "**Do NOT pass the anchor above as `--since`**". Obey that sentence and do not compare the two identities yourself: the marker's `model` is a PROVIDER-QUALIFIED identity (`@`) while `{{model}}` above is the bare model id, so they are not the same kind of string — comparing them by hand either never matches, which throws away this whole recovery path, or matches loosely, which accepts another provider's same-named model and scopes past code it never reviewed. A ledger section that states no verdict — because the side file survived from an earlier round the recovery could not re-vouch — is a mismatch: review the full range. The ledger's round is used only for precedence, and an `upToDate` anchor from the side file stops only when `comment.effective` is false **and the side file carries no `anchorFromRound`** — a grafted anchor that resolves to the head means the round it was carried for closed at a head its source had already certified, so `sha..HEAD` re-covers nothing, and the stop would abandon that round's owed work list without a ruling, with every later round at the same head repeating the same stop: proceed instead as when `comment.effective` is true (the re-run report already holds the full-range diff and plan) and rule every ledger entry). The decision lands AFTER the setup batch but BEFORE any agent launches, which is where the money is (a same-SHA stop still runs `cleanup`; it just fires three cheap commands later than the cache's fast path would have). An anchor that fails validation falls back to the full diff with the reason in the report, exactly as a rebased cache sha does. Two edges, both decided for you: if the side file's `round` is **higher** than the cache's, prefer the side file's sha — the cache is stale by a round some other environment posted; and a side file with no `sha` field means no anchor is recoverable. When the last posted round was fail-closed (`compose-review` withholds the anchor then — Step 8 names the conditions) and its work list survived whole, `pr-context` grafts the anchor forward from the most recent EARLIER own marker that carries one — the withhold is about the fail-closed round's own range, while the earlier round's "clean up to `sha`" stays true, and scoping `sha..HEAD` re-covers the gap (the ledger section says "anchoring at", never "reviewed at", when the anchor was carried forward this way, and names the round it was carried from). So a missing `sha` means a shape the graft refuses or cannot reach — the winning work list was truncated by the marker's size caps (a partial work list must not certify a range — the dropped entries would fall outside the grafted scope and retire silently), the only anchored own marker is the winner's own round (one round cannot both certify and withhold), the winner ran at the same head the candidate sha certifies (grafting it would hand Step 1 a same-sha stop that abandons the work list the winner still owes), every own round on the PR closed without an anchor, the only markers are other accounts' (the sha never crosses accounts), or the markers predate the field — and the review is full-range. (The side file may also carry `commitId` — the previous review's own `commit_id`. That is Step 6's **age reference** for the convergence posture, present even on fail-closed rounds; it is never an anchor, and scoping the diff to it would skip exactly the range a fail-closed round could not certify.) + - **When the cache has no anchor, the PR itself carries one** (high effort only, same as the cache). The file being absent is the NORMAL state everywhere except the machine that ran the last review — CI, another clone, a colleague's checkout — and it used to mean the incremental range silently degraded to the full diff every time, which is precisely the cost incremental review exists to avoid. The anchor now rides the posted review: the machine ledger's marker carries `sha`, the head the last clean round reviewed, and `pr-context` writes it into the side file `qwen-review-pr--prev-ledger.json` with the rest of the ledger. So when the cache had no anchor to pass — including the case where it HELD one that the cache-path gate withheld, because `lastModelId` was another model's: the marker may carry an anchor THIS model certified, and a round that stops at the cache would never look — **or the anchor it passed was refused** (`incremental.effective: false` — a rebase or force-push retires a cached anchor exactly when another environment may have posted a newer round whose marker still holds a valid one): proceed with the setup batch as usual, and when the side file lands with a `sha` — **different from the one already refused, OR the same sha when the refusal was infrastructure** (`base-untrusted`, `capture-failed`: the anchor was never ruled invalid, and the component that failed — a base fetch, a merge-base resolution, a capture — is re-run by the re-run. One shape of `capture-failed` retries ONCE, not forever: a base-less refusal (a null `mergeBaseSha`) means the base fetch failed (`baseFetchFailed: true`) and no local base ref remained, or `git merge-base` itself failed on a non-answer exit. The failed component IS re-run by the re-run, but the exit status cannot split the members — git exits 128 identically for a transient fetch fault and for a deterministic refusal (the base branch deleted on the remote — the refspec fetch fails every time), and the merge-base probe folds its surface failures the same way — so a second refusal of the same shape on the same sha is the deterministic member. Retry that one, once. Every other reason is deterministic for the same sha and must NOT be retried: a validity refusal re-refuses; a planless `partition-failed` always carries a `mergeBaseSha` — with no base nothing is captured and an empty diff cannot fail to tile — so both ranges were in hand and both refused to tile, which the re-run reproduces exactly, do not retry it; `nothing-to-narrow` re-narrows identically: the same two captures select the same hunks, and a capture that failed a UTF-8 round trip fails it again — and its base-less shape (a null `mergeBaseSha` with `baseFetchFailed: false`) is NOT retryable: the fetch succeeded and `git merge-base` found no common ancestor at all (a cross-fork PR with unrelated history), which a re-run reproduces exactly) —, **re-run the `fetch-pr` command from above with `--since --since-model ` — both values come from the side file's `sha`/`model` pair, REPLACING any `--since` and any `--since-model` it already carries, never appending a second one** (a repeated flag is one flag with two values; the CLI takes the last, but a command that reads as two anchors is a command nobody can check) — the PR ref is already fetched so the re-run is cheap, and it rebuilds the worktree, diff and chunk plan scoped to the delta, with the validation the old flow asked you to hand-run (`cat-file`, `merge-base --is-ancestor`) inside the command where it cannot be skipped. Then act on the new report's `incremental` field exactly as the cache path above does (**the same-model gate on this path is RULED FOR YOU, not left to you to apply**: the marker carries `model` beside its `sha` — the identity that certified the range — and `pr-context`'s ledger section states the verdict outright, either "the same-model contract HOLDS" or "**Do NOT pass the anchor above as `--since`**". Obey that sentence and do not compare the two identities yourself: the marker's `model` is a PROVIDER-QUALIFIED identity (`@`) while `{{model}}` above is the bare model id, so they are not the same kind of string — comparing them by hand either never matches, which throws away this whole recovery path, or matches loosely, which accepts another provider's same-named model and scopes past code it never reviewed. A ledger section that states no verdict — because the side file survived from an earlier round the recovery could not re-vouch — is a mismatch: review the full range. The ledger's round is used only for precedence, and an `upToDate` anchor from the side file stops only when `comment.effective` is false **and the side file carries no `anchorFromRound`** — a grafted anchor that resolves to the head means the round it was carried for closed at a head its source had already certified, so `sha..HEAD` re-covers nothing, and the stop would abandon that round's owed work list without a ruling, with every later round at the same head repeating the same stop: proceed instead as when `comment.effective` is true (the re-run report already holds the full-range diff and plan) and rule every ledger entry). The decision lands AFTER the setup batch but BEFORE any agent launches, which is where the money is (a same-SHA stop still runs `cleanup`; it just fires three cheap commands later than the cache's fast path would have). An anchor that fails validation falls back to the full diff with the reason in the report, exactly as a rebased cache sha does. Two edges, both decided for you: if the side file's `round` is **higher** than the cache's, prefer the side file's sha — the cache is stale by a round some other environment posted; and a side file with no `sha` field means no anchor is recoverable. When the last posted round was fail-closed (`compose-review` withholds the anchor then — Step 8 names the conditions) and its work list survived whole, `pr-context` grafts the anchor forward from the most recent EARLIER own marker that carries one — the withhold is about the fail-closed round's own range, while the earlier round's "clean up to `sha`" stays true, and scoping `sha..HEAD` re-covers the gap (the ledger section says "anchoring at", never "reviewed at", when the anchor was carried forward this way, and names the round it was carried from). So a missing `sha` means a shape the graft refuses or cannot reach — the winning work list was truncated by the marker's size caps (a partial work list must not certify a range — the dropped entries would fall outside the grafted scope and retire silently), the only anchored own marker is the winner's own round (one round cannot both certify and withhold), the winner ran at the same head the candidate sha certifies (grafting it would hand Step 1 a same-sha stop that abandons the work list the winner still owes), every own round on the PR closed without an anchor, the only markers are other accounts' (the sha never crosses accounts), or the markers predate the field — and the review is full-range. (The side file may also carry `commitId` — the previous review's own `commit_id`. That is Step 6's **age reference** for the convergence posture, present even on fail-closed rounds; it is never an anchor, and scoping the diff to it would skip exactly the range a fail-closed round could not certify.) - **Resuming an interrupted run (`--resume`)**: when `parse-args` reported `resume.effective: true`, append `--resume` to the `fetch-pr` command above (when the plan's wall is the clock — not under the environment's epoch — `fetch-pr` then notes on stderr how much of that wall this continuation has left, or that it has run out, and which builds the gates will now refuse — information about the run ahead, not an instruction: never drop `--resume` or add `--deadline` over it), and decide `--effort` off `effortSource`, not off whether the word `--effort` was typed. Pass the resolved level whenever `effortSource` is `explicit`, `last_used`, `configured`, or `forced-by-comment` (the `--comment` flag or the `review.comment` setting forces high — parse-args announces "running at high effort"); omit it ONLY when `effortSource` is `default`. `fetch-pr` cannot tell a passed-through default from a chosen level: the interrupted run may have recorded a different one, and handing it the resolved default refuses the resume (`effort-mismatch`) whose fresh fall-through discards the very state `--resume` exists to save — blaming an effort nobody asked for. Omitted, the continuation pins to the recorded level. A level this invocation actually requires — a user's explicit `--effort`, the project's remembered level, a configured `review.effort`, or the high that `--comment` forces — that differs from the recorded one is NOT a passed-through default: pass it, so a mismatch refuses the resume (`effort-mismatch`) and runs fresh at the level this invocation needs. That is right — different effort is different work, and posting authority raising the required depth is different work too, never a silent pin. Omitting a `forced-by-comment` high is the trap: `fetch-pr` has no `--comment` input and reads `requestedEffort` only from `--effort`, so the null would pin the continuation at the recorded sub-high level while `--comment` stays effective — the "effective comment at medium effort" state the medium-tier rules call impossible, posting nothing (medium skips posting) or posting from a pipeline missing the high-only passes the forcing exists to guarantee. `fetch-pr` rules on the interrupted attempt's on-disk state itself (worktree still at `fetchedSha` and clean, diff bytes unchanged, PR head unmoved, the same model identity running — `model-mismatch` otherwise — and resume cap unspent — every probe is a fact it gathers, none is yours to assert) and prints one JSON line on stdout. Branch on it: - **`{"resumed": true, ...}`** — this run continues the interrupted one. The report at the `--out` path is the PREVIOUS attempt's, deliberately left untouched (its mtime is the run epoch every downstream fence keys on); read it for the worktree, plan and diff, which are all reused. The report's `incremental` field is now HISTORY, not a decision to re-take: a resumed run proceeds on the reused plan and does NOT re-enter the incremental check above — in particular it never takes the `upToDate: true` stop/cleanup branch, which runs `cleanup pr-` and would destroy the exact worktree and lease `--resume` just saved (the interrupted attempt was a `--comment` full review of an up-to-date PR; resuming it without `--comment` effective in THIS invocation would otherwise route it straight into "No new changes since last review" and abandon it). Then rebuild your working state from disk before launching anything: diff --git a/packages/core/src/skills/bundled/review/SKILL.test.ts b/packages/core/src/skills/bundled/review/SKILL.test.ts index bc744e7b100..55f10e595ec 100644 --- a/packages/core/src/skills/bundled/review/SKILL.test.ts +++ b/packages/core/src/skills/bundled/review/SKILL.test.ts @@ -413,10 +413,46 @@ describe('bundled review skill', () => { '**Whether a PLAN exists is a separate field: `diffPath`.**', ); // …and the re-run instruction, including the flag-replacement rule that - // keeps a second `--since` from reading as two anchors. + // keeps a second `--since`/`--since-model` from reading as two anchors. expect(body).toContain( - 'REPLACING any `--since` it already carries, never appending a second one', + 'REPLACING any `--since` and any `--since-model` it already carries, ' + + 'never appending a second one', ); + // The marker recovery re-run must carry BOTH fields, taken from the side + // file's `sha`/`model` pair. The command "above" it carries neither on this + // path (no cache), or another model's when the cache anchor was withheld — + // so a reader who copies the model half from there is refused as + // `cross-model-anchor` and reviews the full diff. Sliced to THIS sentence + // — the same `--since ` words occur in the cache path, where they are + // already correct — so the assertion cannot be satisfied by that other + // occurrence. + const reRunAt = body.indexOf( + 're-run the `fetch-pr` command from above with', + ); + expect(reRunAt).toBeGreaterThan(-1); + const reRunSentence = body.slice( + reRunAt, + body.indexOf('never appending a second one', reRunAt), + ); + expect(reRunSentence).toContain('--since-model'); + }); + + it('describes the effective incremental scope without the retired range model', () => { + // The `effective: true` bullet used to call the report's diff `since..head` + // — the full-range model the narrowing replaced. Naming that range invites + // a reader to hand the anchor to git, which this same section forbids; the + // published scope is `incremental.scope`'s file classes. + const body = skillBody(); + const at = body.indexOf('`effective: true` (no `upToDate`)'); + expect(at).toBeGreaterThan(-1); + const rest = body.slice(at + 1); + const nextBullet = rest.indexOf('\n - `'); + expect(nextBullet).toBeGreaterThan(-1); + const bullet = rest.slice(0, nextBullet); + expect(bullet).not.toMatch(/since\.\.head/); + expect(bullet).toContain('`deltaFiles`'); + expect(bullet).toContain('`interaction[]`'); + expect(bullet).toContain('`contextFileCount`'); }); it('pins which refusal reasons the recovery flow may retry', () => {