From 877ff2fd8680d2a4aea30256324af58d0e3560cf Mon Sep 17 00:00:00 2001 From: wenshao Date: Wed, 26 Aug 2026 21:07:32 +0800 Subject: [PATCH 1/5] =?UTF-8?q?feat(review):=20carry=20the=20fix's=20premi?= =?UTF-8?q?ses=20beside=20its=20claim=20=E2=80=94=20add=20fixConstraint?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `fixWitness` (#9596) pins what a suggested fix claims: the test that reds when the fix is removed. Nothing pinned what a fix assumes — the premises it newly introduces — and those are a different defect class that passes a witnessed test cleanly. Two Criticals on the merged nested-approval fix (#9793) were both of this class and both had a fixWitness-grade test: a hand-picked `hops < 16` lineage cap below the user-configurable `MAX_SUBAGENT_DEPTH_LIMIT = 100` reintroduced the very hang the fix was for, and parking several runtimes' approvals on one registry entry broke a `callId` uniqueness dedup and resolve relied on elsewhere, routing a user's answer to the wrong agent. The agent that found each defect had already run the greps that name the fact, so the marginal cost of carrying it is close to zero. The finding gains one optional field beside `fixWitness`, carried the same way: `fixConstraint` — an existing fact the fix must not violate, with its source. It reaches the finder brief (`FINDING_FORMAT`), the skill's Step 6 list and Step 4 aggregate slot, the artifact field list, and Step 7's posting rule, so the human fixer reading the comment — the loop with no other brake — sees it. Two properties differ from the sibling field, and both are deliberate. It is omitted when nothing was observed, never `N/A`: an absent constraint carries no information and would lengthen every posted comment (#9177). `validateFindings` normalises a literal `N/A`/`none` to absence so the poster keys on presence alone, and a finder that copies the fixWitness habit cannot hand it a placeholder to post. And its evidence bar is the `witness` bar, not the `fixWitness` bar — quote the constant or give the `file:line`, or omit — because the costs are asymmetric: a wrong fixWitness is one test not written; a wrong constraint is confidently-stated misdirection the fixer will follow. "Be careful about concurrency" is forbidden in the field the way "this looks risky" is forbidden in the failure scenario. Like fixWitness, the field never gates reporting. Pinned at every site: the validator (round-trip, snake_case, placeholder drop, and the drop's narrowness), the built finder brief (format line, omission clause, evidence bar, forbidden prose, claim/premise split), and the skill corpus (Step 6 item, aggregate slot, field list, posting rule, and the no-`N/A` silence clause). DESIGN.md records the incident. Closes #10153 --- .../src/commands/review/agent-prompt.test.ts | 32 ++++++++++++++ .../cli/src/commands/review/agent-prompt.ts | 5 ++- .../cli/src/commands/review/findings.test.ts | 43 +++++++++++++++++++ packages/cli/src/commands/review/findings.ts | 40 +++++++++++++++++ .../core/src/skills/bundled/review/DESIGN.md | 6 +++ .../core/src/skills/bundled/review/SKILL.md | 6 ++- .../src/skills/bundled/review/SKILL.test.ts | 42 ++++++++++++++++++ .../bundled/review/references/posting.md | 2 + 8 files changed, 173 insertions(+), 3 deletions(-) diff --git a/packages/cli/src/commands/review/agent-prompt.test.ts b/packages/cli/src/commands/review/agent-prompt.test.ts index 04bb4b4a279..9dbbcc762cb 100644 --- a/packages/cli/src/commands/review/agent-prompt.test.ts +++ b/packages/cli/src/commands/review/agent-prompt.test.ts @@ -2775,6 +2775,38 @@ describe('buildRoleBrief — every agent, not just the territory ones', () => { ); }); + it('welds the fix-constraint format into the launched finder briefs', () => { + // The premise half of #10153, pinned where it reaches the agents. Four + // clauses have to survive together: the format has to ASK for the fact, + // the omission has to stay an omission (a finder copying the Fix witness + // habit would write `N/A` and lengthen every comment), the evidence bar + // has to stay at witness grade (a wrong constraint is misdirection the + // fixer follows, so prose with no source is forbidden outright), and the + // field must not become a bar on reporting. + const brief = buildRoleBrief(PLAN, '1a'); + expect(brief).toContain( + '**Fix constraint:** { // The #9788 split moved the language-pitfall CHECKLIST and wrapper/proxy // routing out of 1a, but the falsy-zero shape is general correctness, not diff --git a/packages/cli/src/commands/review/agent-prompt.ts b/packages/cli/src/commands/review/agent-prompt.ts index abfd292b3af..bdc04c4573d 100644 --- a/packages/cli/src/commands/review/agent-prompt.ts +++ b/packages/cli/src/commands/review/agent-prompt.ts @@ -381,6 +381,7 @@ const FINDING_FORMAT = `Format each finding using this structure: - **Failure scenario:** - **Suggested fix:** - **Fix witness:** +- **Fix constraint:** - **Severity:** Critical | Suggestion | Nice to have - **Confidence:** high | low @@ -395,7 +396,9 @@ const FINDING_FORMAT = `Format each finding using this structure: **The failure scenario is the finding's evidence, and it gates reporting.** For a quality finding, state the concrete cost instead of a crash — what is duplicated, wasted, or made harder to change — or quote the rule it violates. A **Suggestion** or **Nice to have** whose failure scenario you cannot fill in concretely **is not a finding: do not report it.** A suspected **Critical** whose trigger you cannot pin down IS still reported, at \`Confidence: low\`, with the scenario naming the mechanism and what remains uncertain — a later verification stage rules on it. "This looks risky", with no nameable trigger and no nameable cost, is how a hallucinated finding reaches a pull request. -**A fix that adds a guard owes a test that fails without it — say so in the finding.** The fix round is this loop's largest single source of its own next round: measured across six multi-round pull requests, roughly a third of every post-first-round finding was introduced by the fix immediately before it, and the dominant shape was a guard or branch added with no test of its own. The suite re-runs only the tests that exist, so an unwitnessed guard passes every gate and its hole comes back as next round's finding. So when your **Suggested fix** adds or changes a guard, a branch, or a behaviour, fill **Fix witness** with the test that must go red without it — the file and what it asserts — and, where you can, the mutation that proves it: remove the guard, run that test, watch it fail. Write \`N/A\` when there is genuinely nothing to pin — a rename, a comment, a docs line, a type-only change, a fix whose whole content is deleting code. **This field never gates reporting**: a finding whose fix you cannot pin is still filed, with \`N/A\`. It is an acceptance criterion for the author, not a bar for you.`; +**A fix that adds a guard owes a test that fails without it — say so in the finding.** The fix round is this loop's largest single source of its own next round: measured across six multi-round pull requests, roughly a third of every post-first-round finding was introduced by the fix immediately before it, and the dominant shape was a guard or branch added with no test of its own. The suite re-runs only the tests that exist, so an unwitnessed guard passes every gate and its hole comes back as next round's finding. So when your **Suggested fix** adds or changes a guard, a branch, or a behaviour, fill **Fix witness** with the test that must go red without it — the file and what it asserts — and, where you can, the mutation that proves it: remove the guard, run that test, watch it fail. Write \`N/A\` when there is genuinely nothing to pin — a rename, a comment, a docs line, a type-only change, a fix whose whole content is deleting code. **This field never gates reporting**: a finding whose fix you cannot pin is still filed, with \`N/A\`. It is an acceptance criterion for the author, not a bar for you. + +**A fix that introduces a premise owes the fact it must respect — with its source, or not at all.** Fix witness pins the fix's *claim*: does it do what it says. Nothing pins the fix's *premises* — the assumptions a fix newly introduces — and those pass a witnessed test cleanly. Two Criticals on one merged fix each had the test that reds without the guard, and were still wrong: a hand-picked \`hops < 16\` lineage cap sat below the user-configurable \`MAX_SUBAGENT_DEPTH_LIMIT = 100\`, so deep lineages silently got back the hang the fix was for; and parking several runtimes' approvals on one registry entry broke a \`callId\` uniqueness that dedup and resolve relied on elsewhere, so a user's answer reached the wrong agent. So when your **Suggested fix** introduces a bound, shares a resource, or changes a shape, and you have already seen the existing fact it must not violate — a configured limit any new bound must stay within, a second site that reads the same field, a uniqueness the resource's key currently guarantees — fill **Fix constraint** with that fact and where it lives. The bar is the **Witness** bar, not the Fix witness bar: **quote the constant or give the \`file:line\`, or omit the line.** The costs are asymmetric — a wrong Fix witness is one test not written; a wrong constraint is confidently-stated misdirection the fixer will follow. "Be careful about concurrency", "keep this consistent with the other path" — a caution with no quoted fact and no location — is forbidden in this field exactly as "this looks risky" is forbidden in the failure scenario. And when you observed nothing, **omit the line entirely** — not \`N/A\`, not "none observed": an absent constraint says nothing and would lengthen every finding. Like Fix witness, this field never gates reporting.`; /** * What not to report. diff --git a/packages/cli/src/commands/review/findings.test.ts b/packages/cli/src/commands/review/findings.test.ts index e5dd074d0f3..ca80db0e0a6 100644 --- a/packages/cli/src/commands/review/findings.test.ts +++ b/packages/cli/src/commands/review/findings.test.ts @@ -2105,4 +2105,47 @@ describe('validateFindings — the canonical artifact round-trips', () => { ).toBe('N/A'); expect(validateFindings([{ ...base }])[0].fixWitness).toBeUndefined(); }); + + it('keeps fixConstraint, and drops the N/A its sibling field allows', () => { + // The fact the fix must not violate — the premise half of #10153, beside + // the claim half `fixWitness` carries. It round-trips like every sibling + // so Step 7's comment body reads it from data rather than re-deriving a + // constant or a file:line the finder already quoted. + const constraint = + 'any bound here must be <= MAX_SUBAGENT_DEPTH_LIMIT = 100 ' + + '(packages/core/src/config/config.ts:1533)'; + const [f] = validateFindings([ + { + ...base, + fixWitness: 'N/A', + fixConstraint: constraint, + }, + ]); + expect(f.fixConstraint).toBe(constraint); + expect(f.fixWitness).toBe('N/A'); + expect( + validateFindings([{ ...base, fix_constraint: constraint }])[0] + .fixConstraint, + ).toBe(constraint); + // Absence stays absence: the field has no `N/A` form, because an empty + // constraint carries no information and would lengthen every posted + // comment (#9177). A finder that copies the fixWitness habit and writes + // the placeholder anyway must not hand the poster a "constraint" — the + // literal is normalised to absence so presence alone is the signal. + expect(validateFindings([{ ...base }])[0].fixConstraint).toBeUndefined(); + for (const placeholder of ['N/A', 'n/a', 'NA', 'none', 'None.', ' N/A ']) { + expect( + validateFindings([{ ...base, fixConstraint: placeholder }])[0] + .fixConstraint, + ).toBeUndefined(); + } + // And the drop is narrow: a real constraint that merely CONTAINS one of + // the words survives — the bar for the field is a quoted constant or a + // file:line, and neither collapses to a placeholder. + expect( + validateFindings([ + { ...base, fixConstraint: 'none of the callers pass 0 (src/a.ts:12)' }, + ])[0].fixConstraint, + ).toBe('none of the callers pass 0 (src/a.ts:12)'); + }); }); diff --git a/packages/cli/src/commands/review/findings.ts b/packages/cli/src/commands/review/findings.ts index ee1910478b3..f651b16ab7f 100644 --- a/packages/cli/src/commands/review/findings.ts +++ b/packages/cli/src/commands/review/findings.ts @@ -128,6 +128,25 @@ export interface Finding { * recorded string instead of transcribing it twice more. */ fixWitness?: string; + /** + * An existing fact the fix must not violate, with its source — the quoted + * constant or a `file:line`: a configured limit any new bound must stay + * within, a second site that reads the field a shape change touches, a + * uniqueness a newly shared resource's key currently guarantees. + * + * `fixWitness` pins the fix's CLAIM — does it do what it says. This pins + * the fix's PREMISES — do the assumptions it newly introduces hold. Those + * are a different defect class and pass a witnessed test cleanly: two + * Criticals on one merged fix each had the test that reds without the + * guard, and were still wrong — a hand-picked lineage cap below the + * user-configurable `MAX_SUBAGENT_DEPTH_LIMIT`, and a shared registry that + * broke a `callId` uniqueness relied on elsewhere (#10153). + * + * Absence is the whole signal: there is no `N/A` form, because an empty + * constraint carries no information and would lengthen every comment, so + * the validator drops a literal `N/A` rather than carrying it. + */ + fixConstraint?: string; /** Free-form kebab-case tag (`correctness`, `security`, `test-coverage`, …). */ category?: string; /** Every location, in report order. A standalone finding has exactly one. */ @@ -183,6 +202,13 @@ function asString(o: Record, key: string): string | undefined { return typeof v === 'string' && v.trim() !== '' ? v : undefined; } +/** `N/A`, `n/a`, `NA`, `none` — the placeholders a field with no `N/A` form + * must not carry. Kept narrow on purpose: a real constraint quotes a constant + * or a `file:line`, and none of those collapse to one of these words. */ +function isNotApplicable(v: string): boolean { + return /^(n\/?a|none)\.?$/i.test(v.trim()); +} + /** A non-empty array of non-empty strings, or undefined; anything else fails * with the finding's index so a typo'd shape is named rather than dropped. */ function stringArray( @@ -422,6 +448,19 @@ export function validateFindings(raw: unknown): Finding[] { // a later round comparing what it asked for against what landed. const fixWitness = asString(o, 'fixWitness') ?? asString(o, 'fix_witness'); + // And `fixConstraint` beside it — the existing fact the fix must not + // violate. Unlike `fixWitness` it has no `N/A` form: absence is the whole + // signal, and a finder that copies the sibling field's habit and writes + // `N/A` here would otherwise hand Step 7 a "constraint" to post. The + // literal is normalised to absence so the comment body can key on + // presence alone. + const fixConstraintRaw = + asString(o, 'fixConstraint') ?? asString(o, 'fix_constraint'); + const fixConstraint = + fixConstraintRaw && !isNotApplicable(fixConstraintRaw) + ? fixConstraintRaw + : undefined; + return { id, severity, @@ -434,6 +473,7 @@ export function validateFindings(raw: unknown): Finding[] { failureScenario, ...(witness ? { witness } : {}), ...(fixWitness ? { fixWitness } : {}), + ...(fixConstraint ? { fixConstraint } : {}), ...(asString(o, 'suggestedFix') || asString(o, 'suggested_fix') ? { suggestedFix: (asString(o, 'suggestedFix') ?? diff --git a/packages/core/src/skills/bundled/review/DESIGN.md b/packages/core/src/skills/bundled/review/DESIGN.md index cc393c1208b..f983ce6e188 100644 --- a/packages/core/src/skills/bundled/review/DESIGN.md +++ b/packages/core/src/skills/bundled/review/DESIGN.md @@ -1168,3 +1168,9 @@ Provenance analysis of six multi-round takeover pull requests attributed each po The fix landed first on the loop side (#9578: the autofix agent must mutation-probe each new guard before it commits), and that half reaches exactly one fixer. The reviewer-side half has to reach all of them, because most pull requests are not fixed by a bot the review can configure: whoever fixes a finding — contributor, maintainer, or agent — reads only the comment. So the acceptance criterion moves into the finding itself (`fixWitness`) and into the posted comment: name the test that must go red if the fix is removed. A fixer told the criterion closes the hole in the round they are already working; a fixer not told it ships the guard and meets the criterion as a finding next round. Two properties keep the rule from costing what it saves. It **never gates reporting** — a finding whose fix cannot be pinned is filed with `N/A`, because a bar on reporting would trade rounds for missed defects, and the reviewer's own evidence rule (`witness`) is the one that governs what confirms. And it is one sentence of ordinary prose at the end of the body, not a section: the comment budget is the scarce resource this review already trims the deferral list to protect. + +### The fix whose premises were wrong (#10153) + +`fixWitness` pins the fix's **claim** — does the fix do what it says. Nothing pinned the fix's **premises** — the assumptions a fix newly introduces — and those are a different defect class that passes a `fixWitness`-grade check cleanly. Two Criticals on one merged nested-approval fix (#9793) were both of this class; both had a test that went red when the fix was removed, and both were still wrong. A hand-picked `hops < 16` lineage-walk cap sat below the user-configurable `MAX_SUBAGENT_DEPTH_LIMIT = 100`, so deep lineages silently reintroduced the very hang the fix was for — the test proved the bridge worked, not that the bound was right. And parking approvals from several runtimes onto one registry entry broke a `callId` uniqueness (`call_qwen_1` restarts per conversation) that dedup and resolve relied on elsewhere, so a user's answer was routed to the wrong agent — the test proved approvals parked, not that the key was still unique. The shapes are **a new constant that must respect an existing configured limit** and **a newly shared resource invalidating a uniqueness assumption held elsewhere**, and the agent that found each defect had already run the greps that would have named the fact, so carrying it costs close to nothing. + +So the finding gains a second optional field beside `fixWitness`, carried the same way (`fixConstraint`): an existing fact the fix must not violate, with its source. Two properties differ from its sibling, and both are deliberate. It is **omitted when nothing was observed, never `N/A`** — `fixWitness` means something as `N/A` ("nothing to pin"), an absent constraint carries no information, and comment volume is a measured concern (#9177); the command drops the literal so the comment body can key on presence alone. And its evidence bar is the **`witness` bar, not the `fixWitness` bar** — quote the constant or give the `file:line`, or omit — because the failure costs are asymmetric: a wrong `fixWitness` is one test not written, a wrong constraint is confidently-stated misdirection the fixer will follow, so "be careful about concurrency" is forbidden in the field exactly as "this looks risky" is forbidden in the failure scenario. The field reaches the fixer who reads the posted comment, which is the loop with no other brake; the `--fix` path's own audit of a fix's premises is #10154. A fix that invents a new mechanism rather than following the suggested one is outside both. diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index a2c2aa44398..e81702d1cc9 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -685,6 +685,7 @@ For each pattern group: - **Witness:** ` line; the witness rule reads an aggregate exactly as it reads a standalone finding> - **Suggested fix:** - **Fix witness:** + - **Fix constraint:** - **Severity:** **Aggregation must not drop the anchors.** Each merged finding arrived with its own `Anchor`, and Step 7 posts one comment per location — so it needs one anchor per location, not one for the group. An aggregated entry sent to `resolve-anchors` with no `anchor` is a hard failure: the subcommand validates every entry and **throws on the whole batch**, so a single anchorless aggregate takes down the resolution of every other finding in the review. Carry the anchors through into the aggregate's `locations[]` — one entry per location, each with its own `anchor` — and Step 6's `findings --to-anchors` performs the expansion mechanically: one resolver request per location, ids suffixed `-1`, `-2`, … (resolutions are joined back to findings by id, so these must be unique — a suffix that collides with another finding's id is refused at projection, and the subcommand rejects duplicates besides). @@ -801,8 +802,9 @@ For each **individual** finding, include: 5. **Witness** — for a Critical: the observed output that settled the verdict, trimmed to the deciding lines — the probe's two sides, the A/B's quote pair, the sweep count over the real population, the failing test text — or the verifier's `not run — ` line (Step 4's witness rule). A Suggestion carries one when a run produced it; it is not owed one. 6. **Suggested fix** — Concrete code suggestion when possible 7. **Fix witness** — the test that must go RED if that fix is removed (file + the behaviour it pins), or `N/A` when the fix adds no guard, branch or behaviour a test can pin. This is the ACCEPTANCE CRITERION for whoever fixes it, not the reviewer's evidence — `Witness` above is the evidence, and the two never substitute for each other. +8. **Fix constraint** — an existing fact the fix must not violate, with its source (the quoted constant or `file:line`): a configured limit a new bound must stay within, a second site that reads the field a shape change touches, a uniqueness a newly shared resource's key must keep. `Fix witness` pins the fix's claim; this pins its premises — the class that passes a witnessed test and is still wrong. Omit it when none was observed — never `N/A` — and never without a source: a caution with no quoted fact ("be careful about concurrency") is not a constraint, and a wrong one is misdirection the fixer will follow. -For **pattern-aggregated** findings, use the aggregated format from Step 4 (Pattern, Occurrences, Example, Failure scenario, Witness, Suggested fix, Fix witness, Severity) with the source tag added. +For **pattern-aggregated** findings, use the aggregated format from Step 4 (Pattern, Occurrences, Example, Failure scenario, Witness, Suggested fix, Fix witness, Fix constraint, Severity) with the source tag added. Group high-confidence findings first. Then add a separate section: @@ -938,7 +940,7 @@ Write every confirmed finding — high and low confidence alike — as a JSON ar **One finding, one name.** A high-effort PR review also writes the incremental cache's cross-round `findings` ledger (Step 8), whose ids are `R-` — use those same ids here: a finding that will enter the ledger gets its `R-` as the artifact `id`, and a carried-forward finding keeps the id it already has. Two id schemes for one finding is how "R1-2" in next round's report and "f7" in this round's outcome ledger turn out to be the same defect that nobody can join. A finding the convergence posture deferred is still a confirmed finding and enters this artifact with all its fields — the deferral is a posting decision recorded in the compose state, never a severity change and never a reason to leave the artifact — but under its own id sequence, `D-`, **never consuming an `R-`**: the `R` counter must predict `buildLedger`, which numbers POSTED findings only, and a deferred finding holding `R6-2` would hand next round a ledger whose `R6-2` names a different defect than this round's artifact — the exact join "one finding, one name" exists to keep. -Each entry carries `id` (unique — outcomes and resolved anchors both join on it), `severity`, `confidence`, `source`, `summary`, `failureScenario`, and either `file`/`line`/`anchor` or, for a pattern aggregate, a `locations[]` array with **one entry per location** (`suggestedFix`, `fixWitness`, `category`, `shortSummary` and `witness` are optional; `shortSummary` is derived from `summary` when absent; `witness` is the Step 4 witness — the executed evidence, or its `not run — ` line — carried as data so the report and the comment bodies quote one recorded string instead of transcribing it twice more; `fixWitness` is the acceptance criterion the finding format asks for — the test that must go red if the suggested fix is removed, or `N/A` — carried for the same reason and read back by Step 7's comment body). The command validates the shape, refuses a duplicate id, refuses a finding with no failure scenario, sorts by severity → confidence → file → line → id, and writes counts nobody then recomputes by hand. Read the artifact for the numbers you quote in the Summary. This is a **canonicalization**, not a gate: it does not decide the verdict — `compose-review` does that, from the same findings — and it does not run at low effort, where the pass is unverified and emits no verdict. +Each entry carries `id` (unique — outcomes and resolved anchors both join on it), `severity`, `confidence`, `source`, `summary`, `failureScenario`, and either `file`/`line`/`anchor` or, for a pattern aggregate, a `locations[]` array with **one entry per location** (`suggestedFix`, `fixWitness`, `fixConstraint`, `category`, `shortSummary` and `witness` are optional; `shortSummary` is derived from `summary` when absent; `witness` is the Step 4 witness — the executed evidence, or its `not run — ` line — carried as data so the report and the comment bodies quote one recorded string instead of transcribing it twice more; `fixWitness` is the acceptance criterion the finding format asks for — the test that must go red if the suggested fix is removed, or `N/A` — carried for the same reason and read back by Step 7's comment body; `fixConstraint` is the existing fact the fix must not violate, with its source — present only when the finder observed one, with no `N/A` form (the command drops the literal), and read back by the same comment body). The command validates the shape, refuses a duplicate id, refuses a finding with no failure scenario, sorts by severity → confidence → file → line → id, and writes counts nobody then recomputes by hand. Read the artifact for the numbers you quote in the Summary. This is a **canonicalization**, not a gate: it does not decide the verdict — `compose-review` does that, from the same findings — and it does not run at low effort, where the pass is unverified and emits no verdict. **Then speak the same list to the client, in-band — one `report_findings` tool call.** The artifact is the canonical record, but it is a file on disk registered after the fact (Step 8); every client rendering this session live — the TUI, the Web Shell transcript, an ACP host — otherwise sees only the prose restatement, which is the transcription surface the artifact exists to close. Immediately after the artifact is written, call the `report_findings` tool once (load it via `tool_search` if it is not in your tool list) — each call replaces the whole list, and Step 6B re-issues it with outcomes after a fix run — with `level` set to this review's effort and one entry per finding **copied from the artifact you just wrote** — `id`, `severity`, `confidence`, `source`, `file`/`line` (a pattern aggregate passes its first location; the artifact keeps the rest), `summary`, `shortSummary`, `failureScenario`, `category` — never re-typed from the terminal prose: the artifact is the oracle, and a re-derived severity here is the same drift the marker rule below closes. A finding the convergence posture deferred is still a finding — report it under its `D-` id like any other. **The tool's contract is harder-bounded than the artifact's, and a violation refuses the whole call**: at most 50 findings, with per-field length caps the schema states. When the artifact outgrows those bounds, do not let the call die on them — pass the first 50 findings in artifact order (the artifact is already sorted most-severe-first) and say in the terminal summary how many the cap cut, and shorten an over-cap `summary`/`failureScenario` — or `outcomeNote` on the Step 6B re-report — to fit rather than dropping the entry (the artifact keeps the full-length text, so nothing is lost by a delivery-only shortening). This is the one sanctioned departure from copy-verbatim, and it is a departure of length only, never of severity, confidence, or meaning — a bounded list delivered beats a complete list refused. This call is UI delivery, not bookkeeping: it persists nothing and decides nothing, and a failure (or an environment where the tool is not registered and `tool_search` cannot find it) is disclosed and moved past — never a reason to touch the artifact, the compose state, or the verdict, exactly the rule `record_artifact` follows in Step 8. diff --git a/packages/core/src/skills/bundled/review/SKILL.test.ts b/packages/core/src/skills/bundled/review/SKILL.test.ts index d9aabab1b8b..d9b80502347 100644 --- a/packages/core/src/skills/bundled/review/SKILL.test.ts +++ b/packages/core/src/skills/bundled/review/SKILL.test.ts @@ -816,6 +816,48 @@ describe('bundled review skill', () => { ); }); + it('pins the fix-constraint field in all three of its halves', () => { + // The premise half of #10153, beside the fix-witness claim half above. + // The same three clauses have to survive together: + // 1. the finding format has to ASK for the fact (Step 6, and the Step 4 + // aggregate slot Step 6 points at), + // 2. the comment has to CARRY it — a constraint recorded and never + // posted reaches no fixer, and the human fixer reading the comment + // is the loop this field exists for, and + // 3. the two properties that make it different from its sibling must + // hold at both sites: omitted rather than `N/A` (comment volume, + // #9177), and witness-grade evidence — a quoted constant or a + // file:line — rather than a caution the fixer would follow. + const body = skillBody(); + expect(body).toContain( + '**Fix constraint** — an existing fact the fix must not violate, with its source', + ); + expect(body).toContain( + 'Omit it when none was observed — never `N/A` — and never without a source', + ); + expect(body).toContain( + '- **Fix constraint:** { // Attribution needs the DISPOSITION and the two-operand test together. // With only the disposition, a round folds any adjacent defect into an diff --git a/packages/core/src/skills/bundled/review/references/posting.md b/packages/core/src/skills/bundled/review/references/posting.md index 40a7ec51c8a..a1effb12513 100644 --- a/packages/core/src/skills/bundled/review/references/posting.md +++ b/packages/core/src/skills/bundled/review/references/posting.md @@ -40,6 +40,8 @@ Also skip this step (independently of the gate above) if the review target is no **And a comment whose fix adds a guard carries the test that must pin it.** When the finding's `fixWitness` is anything other than `N/A`, the posted body closes with it, in one sentence of ordinary prose: name the test that must fail if the fix is removed, and ask for the mutation that proves it (remove the guard, run that test, confirm it reds). One sentence, after the suggestion block — not a heading, not a checklist. This is the reviewer-side half of a measured loop: roughly a third of every post-first-round finding on six multi-round pull requests was introduced by the fix immediately before it, overwhelmingly as a guard or branch with no test of its own, and the deterministic gate re-runs only the tests that exist — so an unwitnessed guard passes every gate and returns as next round's finding. A fixer who is told the acceptance criterion closes it in THIS round; one who is not, does not (measured; DESIGN.md — The fix round that wrote the next round's findings (#9578)). The line reaches every fixer — a contributor, a maintainer, any bot — which is the point: the review cannot assume the fix comes from something it can configure. A finding whose `fixWitness` is `N/A` adds nothing (do not write "no test needed" — silence says it), and this sentence never changes what the comment reports or at what severity. +**And a comment whose fix rests on an existing fact carries that fact.** When the finding has a `fixConstraint`, the posted body carries it in one sentence of ordinary prose beside the fix-witness sentence: state the fact the fix must not violate and its source — the quoted constant, the `file:line` — copied from the artifact, not re-derived. `fixWitness` pins the fix's claim; this pins its premises, the class that passed a witnessed test and was still wrong twice on one merged fix — a hand-picked bound below a configurable limit, and a newly shared registry that broke a `callId` uniqueness relied on elsewhere (measured; DESIGN.md — The fix whose premises were wrong (#10153)). The evidence bar is the `witness` bar, and it holds at posting as it did at finding: a constraint that names no constant and no `file:line` is not posted — a wrong `fixWitness` costs one unwritten test; a wrong constraint is misdirection the fixer will follow. A finding with no `fixConstraint` adds nothing — no `N/A`, no "no constraints observed"; the sentence is absent and the comment is that much shorter — and, like the fix-witness sentence, this one never changes what the comment reports or at what severity. + **Resolve every anchor before you submit — do not post the line numbers the agents reported.** GitHub rejects the whole review with a 422 if any comment's `(path, line)` falls outside every hunk of that file, and it does so all-or-nothing: one miscounted anchor takes every Critical in the review down with it. The line is therefore computed from the diff, not carried over from an agent. The resolver input already exists — Step 6's `findings --to-anchors` wrote it from the artifact, one entry per anchored location of every high-confidence Critical and Suggestion (do NOT hand-project it from the artifact's `locations[]`: the resolver wants `path` where the artifact stores `file`, and a hand projection once produced all-null anchors). Run the resolver: ```bash From 7adb5eeb7e61356f0b3c367cebbe7a128116e03f Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Wed, 26 Aug 2026 16:29:51 +0000 Subject: [PATCH 2/5] fix(review): drop the omission literals the pipeline names for fixConstraint (#10168) Co-authored-by: Qwen-Coder --- .../cli/src/commands/review/findings.test.ts | 17 ++++++++++++++++- packages/cli/src/commands/review/findings.ts | 13 +++++++++---- 2 files changed, 25 insertions(+), 5 deletions(-) diff --git a/packages/cli/src/commands/review/findings.test.ts b/packages/cli/src/commands/review/findings.test.ts index ca80db0e0a6..b1554d4c662 100644 --- a/packages/cli/src/commands/review/findings.test.ts +++ b/packages/cli/src/commands/review/findings.test.ts @@ -2133,7 +2133,22 @@ describe('validateFindings — the canonical artifact round-trips', () => { // the placeholder anyway must not hand the poster a "constraint" — the // literal is normalised to absence so presence alone is the signal. expect(validateFindings([{ ...base }])[0].fixConstraint).toBeUndefined(); - for (const placeholder of ['N/A', 'n/a', 'NA', 'none', 'None.', ' N/A ']) { + for (const placeholder of [ + 'N/A', + 'n/a', + 'NA', + 'none', + 'None.', + ' N/A ', + // The omission literals the finding format and the posting rule name + // — the finder told to omit the line is the one most likely to write + // one, and carried through it would hand Step 7 a "constraint" that + // names no constant and no file:line. + 'none observed', + 'None observed', + 'None observed.', + 'no constraints observed', + ]) { expect( validateFindings([{ ...base, fixConstraint: placeholder }])[0] .fixConstraint, diff --git a/packages/cli/src/commands/review/findings.ts b/packages/cli/src/commands/review/findings.ts index f651b16ab7f..077de7d5781 100644 --- a/packages/cli/src/commands/review/findings.ts +++ b/packages/cli/src/commands/review/findings.ts @@ -202,11 +202,16 @@ function asString(o: Record, key: string): string | undefined { return typeof v === 'string' && v.trim() !== '' ? v : undefined; } -/** `N/A`, `n/a`, `NA`, `none` — the placeholders a field with no `N/A` form - * must not carry. Kept narrow on purpose: a real constraint quotes a constant - * or a `file:line`, and none of those collapse to one of these words. */ +/** `N/A`, `n/a`, `NA`, `none`, `none observed`, `no constraints observed` — + * the placeholders a field with no `N/A` form must not carry; the two long + * ones are the exact omission literals the pipeline itself names, so the + * finder told to omit the line is the one the drop catches. Kept narrow on + * purpose: a real constraint quotes a constant or a `file:line`, and none + * of those collapse to one of these words. */ function isNotApplicable(v: string): boolean { - return /^(n\/?a|none)\.?$/i.test(v.trim()); + return /^(none observed|no constraints observed|n\/?a|none)\.?$/i.test( + v.trim(), + ); } /** A non-empty array of non-empty strings, or undefined; anything else fails From 8679b58da34b6986ead198dbd0765a74c2e546c5 Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Wed, 26 Aug 2026 19:43:17 +0000 Subject: [PATCH 3/5] fix(review): keep fixConstraint through dedup merges and body-Critical relocation (#10168) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both round-1 Criticals name sites where the new field can be dropped on its way to the fixer. Step 4's deduplication and the two pair-loop dedup bullets kept the most detailed description, the highest severity, and the source tags — never a fix-side field — so two agents reporting one root cause lost the constraint only the less detailed copy recorded, before canonicalization ever saw the record. The merge rule now keeps every sourced constraint: consistent ones combined, conflicting ones adjudicated explicitly instead of silently discarded. The posting rule also scoped the constraint sentence to inline comment bodies, but a Critical whose locations all fail anchor resolution moves to bodyCriticals — the finding's sole published copy. The posting rule and the compose-state field now both require that entry to append the constraint sentence, copied from the artifact. Each requirement is pinned by a static SKILL.test.ts assertion that reds when its clause is removed (mutation-probed), and both new tests red against the pre-fix prose. Co-authored-by: Qwen-Coder --- .../core/src/skills/bundled/review/SKILL.md | 8 ++-- .../src/skills/bundled/review/SKILL.test.ts | 44 +++++++++++++++++++ .../bundled/review/references/posting.md | 2 +- 3 files changed, 49 insertions(+), 5 deletions(-) diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index e81702d1cc9..a8d2247e962 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -621,7 +621,7 @@ Then skip Steps 4 and 5 entirely and go to Step 6 with these adjustments: ### Deduplication -Before verification, merge findings that refer to the same issue (same file, same line range, same root cause) even if reported by different agents. Keep the most detailed description and note which agents flagged it. When severities differ across merged items, use the **highest severity** — never let deduplication downgrade severity. **If a merged finding includes any deterministic source** (`[build]`, `[test]`), treat the entire merged finding as pre-confirmed — retain all source tags for reporting, preserve deterministic severity as authoritative, and skip verification. +Before verification, merge findings that refer to the same issue (same file, same line range, same root cause) even if reported by different agents. Keep the most detailed description and note which agents flagged it. When severities differ across merged items, use the **highest severity** — never let deduplication downgrade severity. **Deduplication merges the fix side too: keep every sourced `fixConstraint` the merged findings carry.** Combine consistent constraints into one; when two conflict, adjudicate explicitly — re-read the named sources and keep the constraint the code actually bears — instead of silently discarding one with the less-detailed report. The most-detailed-description pick is about the claim's wording and cannot see a premise only another agent's copy recorded, and canonicalization receives only the deduplicated record: a constraint dropped here reads as absent at posting, leaving the unconstrained fix this field exists to prevent. **If a merged finding includes any deterministic source** (`[build]`, `[test]`), treat the entire merged finding as pre-confirmed — retain all source tags for reporting, preserve deterministic severity as authoritative, and skip verification. ### Batch verification @@ -713,13 +713,13 @@ One anomaly the builder flags but does not refuse (#9242): a per-chunk build on **The convergence pair — 3A (whole-diff form).** Rounds 1 and 2 launch **in one response** — together with Step 4's verifier shards (Step 4 names this) — each built by its own `agent-prompt` call: `--round 1` and `--round 2`, the **same** `--findings` file. This is not a loosened criterion; it is the serial shape's own arithmetic made concurrent: a dry round leaves the cumulative list unchanged, so round 2's launch input was already substantively identical to round 1's — the same entries, at most with verification tags the merge had cleared in between — an independent rerun that the serial shape bought with a full round of wall clock, and that one budget-gated run could no longer afford at all, shipping a capped verdict for want of a second dry audit it had time to run in parallel but not in series (measured; DESIGN.md — The serial convergence pair). What the two-consecutive-dry criterion demands is unchanged: two independent, substantively-dry audits of the whole diff. The one delta the pair does introduce is the same one-round suppression window the pipelined loop already accepts (the merge bullet in the termination rules): the round-2 member audits with entries a verifier may be rejecting mid-flight still on its do-not-re-report list. - **Both members dry** (substantive receipts, per the termination rules): the audit has converged. Wait for the riding verifiers' verdicts, apply the final merge, and proceed to Step 6. -- **Either member reports findings**: the pair is one reporting round. Its members could not see each other, so first dedup the pair against itself (same defect, same location, same root cause keeps one, at the highest severity), merge into the cumulative list, and continue serially: the pair's verifiers ride with round 3's auditor — verify builds over the **deduped union**, sharded per Step 4's `verifyShard` exactly as any reporting round's findings are, **every shard passed as `--round 2`** (the pair's later label; never one build per member — the dedup already merged cross-member findings, and a per-member split would put one entry in front of two verifiers) — and convergence now needs two consecutive dry rounds from round 3 on. A dry member of a reporting pair is **not** carried forward as half of that evidence — its dry predates the other member's findings entering the list. One exception, and it is the retroactively-dry rule below, not a third rule: if a later merge retires the pair in full — every finding from both members rejected — the pair counts as the dry predecessor, and round 3's dry return ends the loop. +- **Either member reports findings**: the pair is one reporting round. Its members could not see each other, so first dedup the pair against itself (same defect, same location, same root cause keeps one, at the highest severity; a sourced `fixConstraint` on either copy survives the merge — Step 4's rule), merge into the cumulative list, and continue serially: the pair's verifiers ride with round 3's auditor — verify builds over the **deduped union**, sharded per Step 4's `verifyShard` exactly as any reporting round's findings are, **every shard passed as `--round 2`** (the pair's later label; never one build per member — the dedup already merged cross-member findings, and a per-member split would put one entry in front of two verifiers) — and convergence now needs two consecutive dry rounds from round 3 on. A dry member of a reporting pair is **not** carried forward as half of that evidence — its dry predates the other member's findings entering the list. One exception, and it is the retroactively-dry rule below, not a third rule: if a later merge retires the pair in full — every finding from both members rejected — the pair counts as the dry predecessor, and round 3's dry return ends the loop. - The substantive-return check applies per member, relaunch-once included. A twice-whiffed member makes the pair not dry — silence is not convergence evidence — and its scope joins the outstanding-whiffed-scopes list exactly as for any round. - If the deadline gate refuses one of the pair's builds (exit 4) and admits the other, launch the admitted member alone and treat the refusal as the budget stop it is (the termination rules below). If it refuses BOTH builds, nothing launches: the remaining budget cannot cover even one round plus the reserve, the first refusal's stop marker is the stop, and the two refusals each name their own round's stop entry — proceed to Step 6 and relay the MARKER's entry only (it holds the first refusal, and it is the one `compose-review` renders). The single-refusal split is defensive only: while the runtime's tool-concurrency pool holds both whole-diff members at once, the gate prices the paired round 2 at one round's wall, so it admits no dearer than the round 1 just admitted and that split cannot currently fire — the rule exists so a future pricing change degrades to the serial shape instead of to a guess. **The convergence pair — 3B (per-chunk form).** On 3B the pair applies per chunk. Launch `--all-chunks --round 1` **and** `--all-chunks --round 2` **in the same response** — both fan out to every chunk (rounds 1 and 2 always do, and the retirement schedule only reads history from round 3, so round 2's build needs nothing round 1 has produced yet), so each chunk's two establishing audits run concurrently instead of a round-wall apart. This is the same arithmetic as 3A read per territory: a chunk dry in round 1 leaves its slice of the cumulative list unchanged, so that chunk's round-2 auditor re-runs substantively the same audit — one round's wall the serial shape paid on every chunked review (measured; DESIGN.md — The serial 3B convergence rounds). The convergence contract is unchanged and reads per chunk through the retirement ledger: a chunk dry in both members holds its two-consecutive-dry certificate, and a pair dry on **every** chunk converges at the round-3 `--all-chunks` build (`CONVERGED`, exit 5) exactly as an all-dry pair does on 3A. Same one-round suppression window, per chunk (a round-2 auditor audits with entries a verifier may be clearing mid-flight). The launch coupling holds too: both members ride with the Step 4 verifier shards (Step 4 names this). -- **Any auditor in either member reports findings**: the pair is one reporting round, exactly as on 3A — wait for BOTH fan-outs to return in full before the dedup (every chunk has an auditor in each member, and members cannot see each other across rounds either), dedup the pair against itself across rounds **and** chunks (same defect, same location, same root cause keeps one, at the highest severity), and merge the union into the cumulative list once. The pair's verifiers ride round 3's `--all-chunks` build: one batch over the **deduped union**, sharded per Step 4's `verifyShard`, **every shard passed as `--round 2`** (the pair's later label — never one build per member). Round 2's auditors are already in flight when round 1's returns land, so the pipelined k/k+1 rule below does not launch them again; this bullet is the pair's only transition. Convergence then reads per chunk through the retirement ledger as above: a chunk that reported in either member holds no certificate and stays under every-round audit, and the pair counts as one reporting round for the retroactively-dry rule — retired only when every finding from **both** members is rejected. +- **Any auditor in either member reports findings**: the pair is one reporting round, exactly as on 3A — wait for BOTH fan-outs to return in full before the dedup (every chunk has an auditor in each member, and members cannot see each other across rounds either), dedup the pair against itself across rounds **and** chunks (same defect, same location, same root cause keeps one, at the highest severity; a sourced `fixConstraint` on any copy survives the merge — Step 4's rule), and merge the union into the cumulative list once. The pair's verifiers ride round 3's `--all-chunks` build: one batch over the **deduped union**, sharded per Step 4's `verifyShard`, **every shard passed as `--round 2`** (the pair's later label — never one build per member). Round 2's auditors are already in flight when round 1's returns land, so the pipelined k/k+1 rule below does not launch them again; this bullet is the pair's only transition. Convergence then reads per chunk through the retirement ledger as above: a chunk that reported in either member holds no certificate and stays under every-round audit, and the pair counts as one reporting round for the retroactively-dry rule — retired only when every finding from **both** members is rejected. - If the deadline gate refuses one member's `--all-chunks` build (exit 4) and admits the other's, launch the admitted member alone and take the stop. The gate prices the round-2 build as the pair's wall — both fan-outs in waves of the runtime's tool-concurrency pool — so this split fires exactly when the pair plus the reserve does not fit but one round still does, and the admitted round alone keeps the serial shape. If it refuses BOTH builds, nothing launches: the remaining budget cannot cover even one round plus the reserve, the first refusal's stop marker is the stop, and the two refusals each name their own round's stop entry — proceed to Step 6 and relay the MARKER's entry only (it holds the first refusal, and it is the one `compose-review` renders). **Do not write the reverse auditor's prompt. Ask for it — and hand it the findings so far so it prints the whole block:** @@ -967,7 +967,7 @@ Each entry carries `id` (unique — outcomes and resolved anchors both join on i It prints a `Verdict:` line to stderr. **That line is the verdict — print it, and nothing else.** It writes nothing, posts nothing, and needs no authorisation, so run it on every verified review — **high and medium** — whether or not you are going to post. The state file is the same one Step 7 uses (every field is listed just below): your findings and the states you established — the body Criticals, the discarded suggestions, the `cannot tell` blockers, the unreviewed dimensions, the `planPath`, the `findingsPath` (high effort — the cumulative reverse-audit findings file, for the `— [unverified]` check), the presubmit flags, the model id. It does **not** take the coverage or the inline counts, and it **refuses** a state JSON carrying `criticalsInline`/`suggestionsInline`. It derives coverage from the harness's transcripts, and it **counts** the inline findings from `--comments`: write the drafted inline comments to that file first — the same `[{path, line, body, …}]` array the Step 7 payload will carry, each body opening with its `**[Critical]**`/`**[Suggestion]**` marker; a review with nothing anchored inline passes a file containing `[]`. A report-only run has read Approve over a blocker its own report listed (measured; DESIGN.md — The Approve over a relocated Critical); counted from the draft, that finding cannot fall out of the computation. **If the comment set changes after composing** — an anchor fails to resolve, a finding relocates to the body, a comment is dropped — update the comments file (and the state), and run `compose-review` again: the verdict must be computed from the set you actually post, and Step 7's `submit` recounts from the payload to hold you to it. - **Not `criticalsInline` / `suggestionsInline`.** `submit` counts those off the `**[Critical]**` / `**[Suggestion]**` prefixes of the comments you attached — a number beside a list is a number that can disagree with the list, and one did. A `state` that supplies either is refused. -- `bodyCriticals` — descriptions of unmappable or 422-relocated Criticals (their only copy lives in the body; they count toward `C` like anchored ones); a `Critical` entry placed in `deferredSuggestions` is relocated here, never deferred. +- `bodyCriticals` — descriptions of unmappable or 422-relocated Criticals (their only copy lives in the body; they count toward `C` like anchored ones); a `Critical` entry placed in `deferredSuggestions` is relocated here, never deferred; an entry whose finding carries a `fixConstraint` appends the constraint sentence, copied from the artifact — the only published copy of the finding must not post without the premise its fix rests on. - `suggestionsDiscarded` — how MANY Suggestions lost their anchors to offline validation or the 422 recovery: a count (non-negative integer). The list of discarded items itself is also accepted and counted by its length (`[]` is zero). They still count toward `S`: dropping every anchor must never upgrade the verdict. - `suggestionsDroppedAsDuplicates` — one entry per **confirmed** Suggestion you did not re-post because it is already reported on the PR (a prior round, a concurrent reviewer, an overlap drop), each naming the finding and where it already lives — never the finding's own text: Suggestion text must never appear in the review `body`, because `.github/workflows/qwen-autofix.yml` does not filter review bodies, so a Suggestion copied into the body would be handed to the autofix bot (full rule in `references/posting.md`); the carve-out for this account is exactly that name + location, e.g. `R1-2 loose review-config pins — already reported (comment 3788857379)`. Use this INSTEAD of bumping `suggestionsDiscarded` for duplicate drops: the two render different sentences, and the discarded one asserts an anchor failure that never happened. They still count toward `S`. - `cannotTellCriticals` — one line per existing PR Critical whose Step 6 re-check landed on `cannot tell` (location + what could not be determined). diff --git a/packages/core/src/skills/bundled/review/SKILL.test.ts b/packages/core/src/skills/bundled/review/SKILL.test.ts index d9b80502347..5a63b983c4e 100644 --- a/packages/core/src/skills/bundled/review/SKILL.test.ts +++ b/packages/core/src/skills/bundled/review/SKILL.test.ts @@ -858,6 +858,50 @@ describe('bundled review skill', () => { ); }); + it('keeps every sourced fixConstraint through the dedup merge', () => { + // R1-2 (#10168): the merge rules kept the most detailed description, the + // highest severity, and the source tags — never a fix-side field. Two + // agents reporting one root cause then lost the constraint only the less + // detailed copy recorded, before canonicalization ever saw the finding: + // the presence-keyed posting rule read "absent" on the deduplicated + // record and posted the unconstrained fix the field exists to prevent. + // The requirement has to stand at all three merging sites — Step 4's + // paragraph and the two pair-loop bullets that merge on their own + // wording — or a pair-merge ships green under the Step 4 pin while + // dropping the field the same way. + const body = skillBody(); + expect(body).toContain( + '**Deduplication merges the fix side too: keep every sourced `fixConstraint` the merged findings carry.**', + ); + expect(body).toContain('when two conflict, adjudicate explicitly'); + expect(body).toContain( + 'a sourced `fixConstraint` on either copy survives the merge', + ); + expect(body).toContain( + 'a sourced `fixConstraint` on any copy survives the merge', + ); + }); + + it('carries the fixConstraint onto a Critical relocated into the body', () => { + // R1-1 (#10168): the carry rule was scoped to inline comment bodies, but + // a confirmed Critical whose locations all fail anchor resolution moves + // to `bodyCriticals` — the review body becomes its sole published copy, + // and a constraint the entry does not carry reaches no fixer. The + // requirement must stand at both sites the routing is spoken: the + // posting rule that performs the move, and the compose-state field that + // receives it. + const body = skillBody(); + expect(body).toContain( + 'a Critical carrying a `fixConstraint` that moves to `bodyCriticals`', + ); + expect(body).toContain( + 'appends the same constraint sentence to that entry, copied from the artifact', + ); + expect(body).toContain( + 'an entry whose finding carries a `fixConstraint` appends the constraint sentence', + ); + }); + it('pins the fix-induced disposition and both of its operands', () => { // Attribution needs the DISPOSITION and the two-operand test together. // With only the disposition, a round folds any adjacent defect into an diff --git a/packages/core/src/skills/bundled/review/references/posting.md b/packages/core/src/skills/bundled/review/references/posting.md index a1effb12513..47ed2f9555c 100644 --- a/packages/core/src/skills/bundled/review/references/posting.md +++ b/packages/core/src/skills/bundled/review/references/posting.md @@ -40,7 +40,7 @@ Also skip this step (independently of the gate above) if the review target is no **And a comment whose fix adds a guard carries the test that must pin it.** When the finding's `fixWitness` is anything other than `N/A`, the posted body closes with it, in one sentence of ordinary prose: name the test that must fail if the fix is removed, and ask for the mutation that proves it (remove the guard, run that test, confirm it reds). One sentence, after the suggestion block — not a heading, not a checklist. This is the reviewer-side half of a measured loop: roughly a third of every post-first-round finding on six multi-round pull requests was introduced by the fix immediately before it, overwhelmingly as a guard or branch with no test of its own, and the deterministic gate re-runs only the tests that exist — so an unwitnessed guard passes every gate and returns as next round's finding. A fixer who is told the acceptance criterion closes it in THIS round; one who is not, does not (measured; DESIGN.md — The fix round that wrote the next round's findings (#9578)). The line reaches every fixer — a contributor, a maintainer, any bot — which is the point: the review cannot assume the fix comes from something it can configure. A finding whose `fixWitness` is `N/A` adds nothing (do not write "no test needed" — silence says it), and this sentence never changes what the comment reports or at what severity. -**And a comment whose fix rests on an existing fact carries that fact.** When the finding has a `fixConstraint`, the posted body carries it in one sentence of ordinary prose beside the fix-witness sentence: state the fact the fix must not violate and its source — the quoted constant, the `file:line` — copied from the artifact, not re-derived. `fixWitness` pins the fix's claim; this pins its premises, the class that passed a witnessed test and was still wrong twice on one merged fix — a hand-picked bound below a configurable limit, and a newly shared registry that broke a `callId` uniqueness relied on elsewhere (measured; DESIGN.md — The fix whose premises were wrong (#10153)). The evidence bar is the `witness` bar, and it holds at posting as it did at finding: a constraint that names no constant and no `file:line` is not posted — a wrong `fixWitness` costs one unwritten test; a wrong constraint is misdirection the fixer will follow. A finding with no `fixConstraint` adds nothing — no `N/A`, no "no constraints observed"; the sentence is absent and the comment is that much shorter — and, like the fix-witness sentence, this one never changes what the comment reports or at what severity. +**And a comment whose fix rests on an existing fact carries that fact.** When the finding has a `fixConstraint`, the posted body carries it in one sentence of ordinary prose beside the fix-witness sentence: state the fact the fix must not violate and its source — the quoted constant, the `file:line` — copied from the artifact, not re-derived. `fixWitness` pins the fix's claim; this pins its premises, the class that passed a witnessed test and was still wrong twice on one merged fix — a hand-picked bound below a configurable limit, and a newly shared registry that broke a `callId` uniqueness relied on elsewhere (measured; DESIGN.md — The fix whose premises were wrong (#10153)). The evidence bar is the `witness` bar, and it holds at posting as it did at finding: a constraint that names no constant and no `file:line` is not posted — a wrong `fixWitness` costs one unwritten test; a wrong constraint is misdirection the fixer will follow. A finding with no `fixConstraint` adds nothing — no `N/A`, no "no constraints observed"; the sentence is absent and the comment is that much shorter — and, like the fix-witness sentence, this one never changes what the comment reports or at what severity. And the rule follows the finding to every PR-facing copy: a Critical carrying a `fixConstraint` that moves to `bodyCriticals` — unmappable, or relocated by the 422 recovery — appends the same constraint sentence to that entry, copied from the artifact, because that entry is the finding's sole published copy, and a premise lost there is one no fixer ever sees. **Resolve every anchor before you submit — do not post the line numbers the agents reported.** GitHub rejects the whole review with a 422 if any comment's `(path, line)` falls outside every hunk of that file, and it does so all-or-nothing: one miscounted anchor takes every Critical in the review down with it. The line is therefore computed from the diff, not carried over from an agent. The resolver input already exists — Step 6's `findings --to-anchors` wrote it from the artifact, one entry per anchored location of every high-confidence Critical and Suggestion (do NOT hand-project it from the artifact's `locations[]`: the resolver wants `path` where the artifact stores `file`, and a hand projection once produced all-null anchors). Run the resolver: From e4a7d8a9e03a768ee72343ad8e20a8bf69e024c0 Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Wed, 26 Aug 2026 22:33:57 +0000 Subject: [PATCH 4/5] fix(review): keep the whole fix side through dedup merges and body-Critical relocation (#10168) --- .../core/src/skills/bundled/review/SKILL.md | 8 +-- .../src/skills/bundled/review/SKILL.test.ts | 51 ++++++++++++++----- .../bundled/review/references/posting.md | 2 +- 3 files changed, 42 insertions(+), 19 deletions(-) diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index a8d2247e962..b5ad8f50b7f 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -621,7 +621,7 @@ Then skip Steps 4 and 5 entirely and go to Step 6 with these adjustments: ### Deduplication -Before verification, merge findings that refer to the same issue (same file, same line range, same root cause) even if reported by different agents. Keep the most detailed description and note which agents flagged it. When severities differ across merged items, use the **highest severity** — never let deduplication downgrade severity. **Deduplication merges the fix side too: keep every sourced `fixConstraint` the merged findings carry.** Combine consistent constraints into one; when two conflict, adjudicate explicitly — re-read the named sources and keep the constraint the code actually bears — instead of silently discarding one with the less-detailed report. The most-detailed-description pick is about the claim's wording and cannot see a premise only another agent's copy recorded, and canonicalization receives only the deduplicated record: a constraint dropped here reads as absent at posting, leaving the unconstrained fix this field exists to prevent. **If a merged finding includes any deterministic source** (`[build]`, `[test]`), treat the entire merged finding as pre-confirmed — retain all source tags for reporting, preserve deterministic severity as authoritative, and skip verification. +Before verification, merge findings that refer to the same issue (same file, same line range, same root cause) even if reported by different agents. Keep the most detailed description and note which agents flagged it. When severities differ across merged items, use the **highest severity** — never let deduplication downgrade severity. **Deduplication merges the fix side too: keep every `fixWitness` and every sourced `fixConstraint` the merged findings carry.** Combine consistent constraints into one; when two conflict, adjudicate explicitly — re-read the named sources and keep the constraint the code actually bears — instead of silently discarding one with the less-detailed report. The most-detailed-description pick is about the claim's wording and cannot see a fix-side sentence only another agent's copy recorded, and canonicalization receives only the deduplicated record: a witness or a constraint dropped here reads as absent at posting, leaving the unwitnessed guard or the unconstrained fix these fields exist to prevent. **If a merged finding includes any deterministic source** (`[build]`, `[test]`), treat the entire merged finding as pre-confirmed — retain all source tags for reporting, preserve deterministic severity as authoritative, and skip verification. ### Batch verification @@ -713,13 +713,13 @@ One anomaly the builder flags but does not refuse (#9242): a per-chunk build on **The convergence pair — 3A (whole-diff form).** Rounds 1 and 2 launch **in one response** — together with Step 4's verifier shards (Step 4 names this) — each built by its own `agent-prompt` call: `--round 1` and `--round 2`, the **same** `--findings` file. This is not a loosened criterion; it is the serial shape's own arithmetic made concurrent: a dry round leaves the cumulative list unchanged, so round 2's launch input was already substantively identical to round 1's — the same entries, at most with verification tags the merge had cleared in between — an independent rerun that the serial shape bought with a full round of wall clock, and that one budget-gated run could no longer afford at all, shipping a capped verdict for want of a second dry audit it had time to run in parallel but not in series (measured; DESIGN.md — The serial convergence pair). What the two-consecutive-dry criterion demands is unchanged: two independent, substantively-dry audits of the whole diff. The one delta the pair does introduce is the same one-round suppression window the pipelined loop already accepts (the merge bullet in the termination rules): the round-2 member audits with entries a verifier may be rejecting mid-flight still on its do-not-re-report list. - **Both members dry** (substantive receipts, per the termination rules): the audit has converged. Wait for the riding verifiers' verdicts, apply the final merge, and proceed to Step 6. -- **Either member reports findings**: the pair is one reporting round. Its members could not see each other, so first dedup the pair against itself (same defect, same location, same root cause keeps one, at the highest severity; a sourced `fixConstraint` on either copy survives the merge — Step 4's rule), merge into the cumulative list, and continue serially: the pair's verifiers ride with round 3's auditor — verify builds over the **deduped union**, sharded per Step 4's `verifyShard` exactly as any reporting round's findings are, **every shard passed as `--round 2`** (the pair's later label; never one build per member — the dedup already merged cross-member findings, and a per-member split would put one entry in front of two verifiers) — and convergence now needs two consecutive dry rounds from round 3 on. A dry member of a reporting pair is **not** carried forward as half of that evidence — its dry predates the other member's findings entering the list. One exception, and it is the retroactively-dry rule below, not a third rule: if a later merge retires the pair in full — every finding from both members rejected — the pair counts as the dry predecessor, and round 3's dry return ends the loop. +- **Either member reports findings**: the pair is one reporting round. Its members could not see each other, so first dedup the pair against itself (same defect, same location, same root cause keeps one, at the highest severity; a `fixWitness`/sourced `fixConstraint` on either copy survives the merge — Step 4's rule), merge into the cumulative list, and continue serially: the pair's verifiers ride with round 3's auditor — verify builds over the **deduped union**, sharded per Step 4's `verifyShard` exactly as any reporting round's findings are, **every shard passed as `--round 2`** (the pair's later label; never one build per member — the dedup already merged cross-member findings, and a per-member split would put one entry in front of two verifiers) — and convergence now needs two consecutive dry rounds from round 3 on. A dry member of a reporting pair is **not** carried forward as half of that evidence — its dry predates the other member's findings entering the list. One exception, and it is the retroactively-dry rule below, not a third rule: if a later merge retires the pair in full — every finding from both members rejected — the pair counts as the dry predecessor, and round 3's dry return ends the loop. - The substantive-return check applies per member, relaunch-once included. A twice-whiffed member makes the pair not dry — silence is not convergence evidence — and its scope joins the outstanding-whiffed-scopes list exactly as for any round. - If the deadline gate refuses one of the pair's builds (exit 4) and admits the other, launch the admitted member alone and treat the refusal as the budget stop it is (the termination rules below). If it refuses BOTH builds, nothing launches: the remaining budget cannot cover even one round plus the reserve, the first refusal's stop marker is the stop, and the two refusals each name their own round's stop entry — proceed to Step 6 and relay the MARKER's entry only (it holds the first refusal, and it is the one `compose-review` renders). The single-refusal split is defensive only: while the runtime's tool-concurrency pool holds both whole-diff members at once, the gate prices the paired round 2 at one round's wall, so it admits no dearer than the round 1 just admitted and that split cannot currently fire — the rule exists so a future pricing change degrades to the serial shape instead of to a guess. **The convergence pair — 3B (per-chunk form).** On 3B the pair applies per chunk. Launch `--all-chunks --round 1` **and** `--all-chunks --round 2` **in the same response** — both fan out to every chunk (rounds 1 and 2 always do, and the retirement schedule only reads history from round 3, so round 2's build needs nothing round 1 has produced yet), so each chunk's two establishing audits run concurrently instead of a round-wall apart. This is the same arithmetic as 3A read per territory: a chunk dry in round 1 leaves its slice of the cumulative list unchanged, so that chunk's round-2 auditor re-runs substantively the same audit — one round's wall the serial shape paid on every chunked review (measured; DESIGN.md — The serial 3B convergence rounds). The convergence contract is unchanged and reads per chunk through the retirement ledger: a chunk dry in both members holds its two-consecutive-dry certificate, and a pair dry on **every** chunk converges at the round-3 `--all-chunks` build (`CONVERGED`, exit 5) exactly as an all-dry pair does on 3A. Same one-round suppression window, per chunk (a round-2 auditor audits with entries a verifier may be clearing mid-flight). The launch coupling holds too: both members ride with the Step 4 verifier shards (Step 4 names this). -- **Any auditor in either member reports findings**: the pair is one reporting round, exactly as on 3A — wait for BOTH fan-outs to return in full before the dedup (every chunk has an auditor in each member, and members cannot see each other across rounds either), dedup the pair against itself across rounds **and** chunks (same defect, same location, same root cause keeps one, at the highest severity; a sourced `fixConstraint` on any copy survives the merge — Step 4's rule), and merge the union into the cumulative list once. The pair's verifiers ride round 3's `--all-chunks` build: one batch over the **deduped union**, sharded per Step 4's `verifyShard`, **every shard passed as `--round 2`** (the pair's later label — never one build per member). Round 2's auditors are already in flight when round 1's returns land, so the pipelined k/k+1 rule below does not launch them again; this bullet is the pair's only transition. Convergence then reads per chunk through the retirement ledger as above: a chunk that reported in either member holds no certificate and stays under every-round audit, and the pair counts as one reporting round for the retroactively-dry rule — retired only when every finding from **both** members is rejected. +- **Any auditor in either member reports findings**: the pair is one reporting round, exactly as on 3A — wait for BOTH fan-outs to return in full before the dedup (every chunk has an auditor in each member, and members cannot see each other across rounds either), dedup the pair against itself across rounds **and** chunks (same defect, same location, same root cause keeps one, at the highest severity; a `fixWitness`/sourced `fixConstraint` on any copy survives the merge — Step 4's rule), and merge the union into the cumulative list once. The pair's verifiers ride round 3's `--all-chunks` build: one batch over the **deduped union**, sharded per Step 4's `verifyShard`, **every shard passed as `--round 2`** (the pair's later label — never one build per member). Round 2's auditors are already in flight when round 1's returns land, so the pipelined k/k+1 rule below does not launch them again; this bullet is the pair's only transition. Convergence then reads per chunk through the retirement ledger as above: a chunk that reported in either member holds no certificate and stays under every-round audit, and the pair counts as one reporting round for the retroactively-dry rule — retired only when every finding from **both** members is rejected. - If the deadline gate refuses one member's `--all-chunks` build (exit 4) and admits the other's, launch the admitted member alone and take the stop. The gate prices the round-2 build as the pair's wall — both fan-outs in waves of the runtime's tool-concurrency pool — so this split fires exactly when the pair plus the reserve does not fit but one round still does, and the admitted round alone keeps the serial shape. If it refuses BOTH builds, nothing launches: the remaining budget cannot cover even one round plus the reserve, the first refusal's stop marker is the stop, and the two refusals each name their own round's stop entry — proceed to Step 6 and relay the MARKER's entry only (it holds the first refusal, and it is the one `compose-review` renders). **Do not write the reverse auditor's prompt. Ask for it — and hand it the findings so far so it prints the whole block:** @@ -967,7 +967,7 @@ Each entry carries `id` (unique — outcomes and resolved anchors both join on i It prints a `Verdict:` line to stderr. **That line is the verdict — print it, and nothing else.** It writes nothing, posts nothing, and needs no authorisation, so run it on every verified review — **high and medium** — whether or not you are going to post. The state file is the same one Step 7 uses (every field is listed just below): your findings and the states you established — the body Criticals, the discarded suggestions, the `cannot tell` blockers, the unreviewed dimensions, the `planPath`, the `findingsPath` (high effort — the cumulative reverse-audit findings file, for the `— [unverified]` check), the presubmit flags, the model id. It does **not** take the coverage or the inline counts, and it **refuses** a state JSON carrying `criticalsInline`/`suggestionsInline`. It derives coverage from the harness's transcripts, and it **counts** the inline findings from `--comments`: write the drafted inline comments to that file first — the same `[{path, line, body, …}]` array the Step 7 payload will carry, each body opening with its `**[Critical]**`/`**[Suggestion]**` marker; a review with nothing anchored inline passes a file containing `[]`. A report-only run has read Approve over a blocker its own report listed (measured; DESIGN.md — The Approve over a relocated Critical); counted from the draft, that finding cannot fall out of the computation. **If the comment set changes after composing** — an anchor fails to resolve, a finding relocates to the body, a comment is dropped — update the comments file (and the state), and run `compose-review` again: the verdict must be computed from the set you actually post, and Step 7's `submit` recounts from the payload to hold you to it. - **Not `criticalsInline` / `suggestionsInline`.** `submit` counts those off the `**[Critical]**` / `**[Suggestion]**` prefixes of the comments you attached — a number beside a list is a number that can disagree with the list, and one did. A `state` that supplies either is refused. -- `bodyCriticals` — descriptions of unmappable or 422-relocated Criticals (their only copy lives in the body; they count toward `C` like anchored ones); a `Critical` entry placed in `deferredSuggestions` is relocated here, never deferred; an entry whose finding carries a `fixConstraint` appends the constraint sentence, copied from the artifact — the only published copy of the finding must not post without the premise its fix rests on. +- `bodyCriticals` — descriptions of unmappable or 422-relocated Criticals (their only copy lives in the body; they count toward `C` like anchored ones); a `Critical` entry placed in `deferredSuggestions` is relocated here, never deferred; an entry whose finding carries a `fixWitness` or a `fixConstraint` appends the corresponding sentence, copied from the artifact — the only published copy of the finding must not post without the fix's witness or the premise it rests on. - `suggestionsDiscarded` — how MANY Suggestions lost their anchors to offline validation or the 422 recovery: a count (non-negative integer). The list of discarded items itself is also accepted and counted by its length (`[]` is zero). They still count toward `S`: dropping every anchor must never upgrade the verdict. - `suggestionsDroppedAsDuplicates` — one entry per **confirmed** Suggestion you did not re-post because it is already reported on the PR (a prior round, a concurrent reviewer, an overlap drop), each naming the finding and where it already lives — never the finding's own text: Suggestion text must never appear in the review `body`, because `.github/workflows/qwen-autofix.yml` does not filter review bodies, so a Suggestion copied into the body would be handed to the autofix bot (full rule in `references/posting.md`); the carve-out for this account is exactly that name + location, e.g. `R1-2 loose review-config pins — already reported (comment 3788857379)`. Use this INSTEAD of bumping `suggestionsDiscarded` for duplicate drops: the two render different sentences, and the discarded one asserts an anchor failure that never happened. They still count toward `S`. - `cannotTellCriticals` — one line per existing PR Critical whose Step 6 re-check landed on `cannot tell` (location + what could not be determined). diff --git a/packages/core/src/skills/bundled/review/SKILL.test.ts b/packages/core/src/skills/bundled/review/SKILL.test.ts index 5a63b983c4e..4b8afe30457 100644 --- a/packages/core/src/skills/bundled/review/SKILL.test.ts +++ b/packages/core/src/skills/bundled/review/SKILL.test.ts @@ -858,48 +858,71 @@ describe('bundled review skill', () => { ); }); - it('keeps every sourced fixConstraint through the dedup merge', () => { + it('keeps the fix side — fixWitness and sourced fixConstraint — through the dedup merge', () => { // R1-2 (#10168): the merge rules kept the most detailed description, the // highest severity, and the source tags — never a fix-side field. Two // agents reporting one root cause then lost the constraint only the less // detailed copy recorded, before canonicalization ever saw the finding: // the presence-keyed posting rule read "absent" on the deduplicated // record and posted the unconstrained fix the field exists to prevent. - // The requirement has to stand at all three merging sites — Step 4's - // paragraph and the two pair-loop bullets that merge on their own - // wording — or a pair-merge ships green under the Step 4 pin while - // dropping the field the same way. + // R3-1: the sibling field dies the same death — the fix-witness sentence + // is presence-keyed too, so a witness only the discarded copy recorded + // is silently omitted and the fix ships unwitnessed (#9578). The + // preservation therefore names BOTH fields at all three merging sites — + // Step 4's paragraph and the two pair-loop bullets that merge on their + // own wording — or a pair-merge ships green under the Step 4 pin while + // dropping the field the same way. The adjudication sentence stays + // constraint-specific: two sourced constraints can conflict as claims + // about the code, and the rule that settles them re-reads the sources; + // this pin only guards that nothing fix-side is silently discarded. const body = skillBody(); expect(body).toContain( - '**Deduplication merges the fix side too: keep every sourced `fixConstraint` the merged findings carry.**', + '**Deduplication merges the fix side too: keep every `fixWitness` and every sourced `fixConstraint` the merged findings carry.**', ); expect(body).toContain('when two conflict, adjudicate explicitly'); expect(body).toContain( - 'a sourced `fixConstraint` on either copy survives the merge', + '`fixWitness`/sourced `fixConstraint` on either copy survives the merge', ); expect(body).toContain( - 'a sourced `fixConstraint` on any copy survives the merge', + '`fixWitness`/sourced `fixConstraint` on any copy survives the merge', ); }); - it('carries the fixConstraint onto a Critical relocated into the body', () => { + it('carries the fix side onto a Critical relocated into the body', () => { // R1-1 (#10168): the carry rule was scoped to inline comment bodies, but // a confirmed Critical whose locations all fail anchor resolution moves // to `bodyCriticals` — the review body becomes its sole published copy, - // and a constraint the entry does not carry reaches no fixer. The - // requirement must stand at both sites the routing is spoken: the + // and a constraint the entry does not carry reaches no fixer. R3-1: the + // fix-witness sentence is presence-keyed the same way and dies the same + // death, so the carry covers both fix-side sentences. R3-2: the cover is + // scoped to the two moves the orchestrator performs — on an Aone target + // `submit` itself relocates an unanchorable Critical through a one-line + // entry rebuilt from the claim line alone, a channel neither sentence + // rides — and that residue must stay a named acceptance, never the + // universal promise ("every PR-facing copy") the channel contradicts. + // The requirement must stand at both sites the routing is spoken: the // posting rule that performs the move, and the compose-state field that // receives it. const body = skillBody(); expect(body).toContain( - 'a Critical carrying a `fixConstraint` that moves to `bodyCriticals`', + 'the rule follows the finding through the two moves the orchestrator performs', ); expect(body).toContain( - 'appends the same constraint sentence to that entry, copied from the artifact', + 'a Critical carrying either fix-side sentence — the fix-witness or the constraint sentence — that moves to `bodyCriticals`', ); expect(body).toContain( - 'an entry whose finding carries a `fixConstraint` appends the constraint sentence', + 'appends the same sentence to that entry, copied from the artifact', ); + expect(body).toContain( + 'an entry whose finding carries a `fixWitness` or a `fixConstraint` appends the corresponding sentence', + ); + // The disclosed residue: Aone performs no server-side anchor validation, + // so submit relocates at submit time through the claim line alone, and + // the rule names the loss instead of promising past it. + expect(body).toContain( + 'relocates an unanchorable Critical into the body as a one-line entry rebuilt from the claim line alone', + ); + expect(body).toContain('the loss is a named acceptance, not a silent one'); }); it('pins the fix-induced disposition and both of its operands', () => { diff --git a/packages/core/src/skills/bundled/review/references/posting.md b/packages/core/src/skills/bundled/review/references/posting.md index 47ed2f9555c..efbdd4dcc08 100644 --- a/packages/core/src/skills/bundled/review/references/posting.md +++ b/packages/core/src/skills/bundled/review/references/posting.md @@ -40,7 +40,7 @@ Also skip this step (independently of the gate above) if the review target is no **And a comment whose fix adds a guard carries the test that must pin it.** When the finding's `fixWitness` is anything other than `N/A`, the posted body closes with it, in one sentence of ordinary prose: name the test that must fail if the fix is removed, and ask for the mutation that proves it (remove the guard, run that test, confirm it reds). One sentence, after the suggestion block — not a heading, not a checklist. This is the reviewer-side half of a measured loop: roughly a third of every post-first-round finding on six multi-round pull requests was introduced by the fix immediately before it, overwhelmingly as a guard or branch with no test of its own, and the deterministic gate re-runs only the tests that exist — so an unwitnessed guard passes every gate and returns as next round's finding. A fixer who is told the acceptance criterion closes it in THIS round; one who is not, does not (measured; DESIGN.md — The fix round that wrote the next round's findings (#9578)). The line reaches every fixer — a contributor, a maintainer, any bot — which is the point: the review cannot assume the fix comes from something it can configure. A finding whose `fixWitness` is `N/A` adds nothing (do not write "no test needed" — silence says it), and this sentence never changes what the comment reports or at what severity. -**And a comment whose fix rests on an existing fact carries that fact.** When the finding has a `fixConstraint`, the posted body carries it in one sentence of ordinary prose beside the fix-witness sentence: state the fact the fix must not violate and its source — the quoted constant, the `file:line` — copied from the artifact, not re-derived. `fixWitness` pins the fix's claim; this pins its premises, the class that passed a witnessed test and was still wrong twice on one merged fix — a hand-picked bound below a configurable limit, and a newly shared registry that broke a `callId` uniqueness relied on elsewhere (measured; DESIGN.md — The fix whose premises were wrong (#10153)). The evidence bar is the `witness` bar, and it holds at posting as it did at finding: a constraint that names no constant and no `file:line` is not posted — a wrong `fixWitness` costs one unwritten test; a wrong constraint is misdirection the fixer will follow. A finding with no `fixConstraint` adds nothing — no `N/A`, no "no constraints observed"; the sentence is absent and the comment is that much shorter — and, like the fix-witness sentence, this one never changes what the comment reports or at what severity. And the rule follows the finding to every PR-facing copy: a Critical carrying a `fixConstraint` that moves to `bodyCriticals` — unmappable, or relocated by the 422 recovery — appends the same constraint sentence to that entry, copied from the artifact, because that entry is the finding's sole published copy, and a premise lost there is one no fixer ever sees. +**And a comment whose fix rests on an existing fact carries that fact.** When the finding has a `fixConstraint`, the posted body carries it in one sentence of ordinary prose beside the fix-witness sentence: state the fact the fix must not violate and its source — the quoted constant, the `file:line` — copied from the artifact, not re-derived. `fixWitness` pins the fix's claim; this pins its premises, the class that passed a witnessed test and was still wrong twice on one merged fix — a hand-picked bound below a configurable limit, and a newly shared registry that broke a `callId` uniqueness relied on elsewhere (measured; DESIGN.md — The fix whose premises were wrong (#10153)). The evidence bar is the `witness` bar, and it holds at posting as it did at finding: a constraint that names no constant and no `file:line` is not posted — a wrong `fixWitness` costs one unwritten test; a wrong constraint is misdirection the fixer will follow. A finding with no `fixConstraint` adds nothing — no `N/A`, no "no constraints observed"; the sentence is absent and the comment is that much shorter — and, like the fix-witness sentence, this one never changes what the comment reports or at what severity. And the rule follows the finding through the two moves the orchestrator performs: a Critical carrying either fix-side sentence — the fix-witness or the constraint sentence — that moves to `bodyCriticals` — unmappable, or relocated by the 422 recovery — appends the same sentence to that entry, copied from the artifact, because that entry is the finding's sole published copy, and a witness or a premise lost there is one no fixer ever sees. A third move exists and carries neither sentence: on an Aone target the platform performs no server-side anchor validation, so `submit` validates every anchor itself and relocates an unanchorable Critical into the body as a one-line entry rebuilt from the claim line alone — a channel structurally too narrow for either sentence. Closing that channel is the structured body-Critical projection, larger than this rule; until then the loss is a named acceptance, not a silent one. **Resolve every anchor before you submit — do not post the line numbers the agents reported.** GitHub rejects the whole review with a 422 if any comment's `(path, line)` falls outside every hunk of that file, and it does so all-or-nothing: one miscounted anchor takes every Critical in the review down with it. The line is therefore computed from the diff, not carried over from an agent. The resolver input already exists — Step 6's `findings --to-anchors` wrote it from the artifact, one entry per anchored location of every high-confidence Critical and Suggestion (do NOT hand-project it from the artifact's `locations[]`: the resolver wants `path` where the artifact stores `file`, and a hand projection once produced all-null anchors). Run the resolver: From 82539fb2c4599108f76920c7019486c225002666 Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Thu, 27 Aug 2026 02:45:46 +0000 Subject: [PATCH 5/5] fix(review): pin the constraint sentence's mandate, order, and loss channels (#10168) Three suggestions on the fixConstraint posting paragraph, one cluster. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The carry-mandate clause — the operative half of the rule — had no assertion: the heading was pinned but the sentence was not, so an edit weakening "the posted body carries it" shipped green. The placement instruction, "beside the fix-witness sentence", underdetermined the position: the fix-witness rule closes the body with the witness, so immediately before it is the only consistent place, and a finding whose `fixWitness` is `N/A` had no referent to stand beside at all. The paragraph now fixes the order, names the N/A fallback (the constraint sentence takes the witness's place after the suggestion block), and states the same order for a `bodyCriticals` entry that carries both sentences. The paragraph also scoped the accepted loss to the Aone submit-time relocation, leaving two structurally identical channels unnamed: the deferral rendering (a `DeferredEntry` holds no fix-side field and the line caps at 240 characters) and the duplicate-drop account (name and location only, never the finding's own text). Both are now named beside the Aone residue; the loss stays a named acceptance, not a silent one. Each new clause is pinned by a toContain assertion in SKILL.test.ts; six mutation probes (weaken the mandate, delete the placement, the fallback, the order clause, or either channel naming) all red the tests, and the restored suite is green. Co-authored-by: Qwen-Coder --- .../src/skills/bundled/review/SKILL.test.ts | 30 +++++++++++++++++++ .../bundled/review/references/posting.md | 2 +- 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/packages/core/src/skills/bundled/review/SKILL.test.ts b/packages/core/src/skills/bundled/review/SKILL.test.ts index 4b8afe30457..71a0389c96d 100644 --- a/packages/core/src/skills/bundled/review/SKILL.test.ts +++ b/packages/core/src/skills/bundled/review/SKILL.test.ts @@ -856,6 +856,21 @@ describe('bundled review skill', () => { expect(body).toContain( 'A finding with no `fixConstraint` adds nothing — no `N/A`, no "no constraints observed"', ); + // R4-2: half 2's operative sentence — the heading is pinned above, but + // the mandate itself was not, so weakening "carries it" shipped green. + expect(body).toContain( + 'When the finding has a `fixConstraint`, the posted body carries it in one sentence of ordinary prose', + ); + // R4-1: the witness closes the body, so the constraint's only consistent + // place is immediately before it — and a finding whose `fixWitness` is + // `N/A` has no witness sentence to stand beside, so the constraint takes + // that place itself, after the suggestion block. + expect(body).toContain( + 'immediately before the fix-witness sentence, which still closes the body', + ); + expect(body).toContain( + 'the constraint sentence takes its place after the suggestion block', + ); }); it('keeps the fix side — fixWitness and sourced fixConstraint — through the dedup merge', () => { @@ -913,6 +928,11 @@ describe('bundled review skill', () => { expect(body).toContain( 'appends the same sentence to that entry, copied from the artifact', ); + // R4-1: an entry that carries both sentences appends them in the inline + // order — the constraint before the witness. + expect(body).toContain( + 'the constraint before the witness when the finding carries both', + ); expect(body).toContain( 'an entry whose finding carries a `fixWitness` or a `fixConstraint` appends the corresponding sentence', ); @@ -923,6 +943,16 @@ describe('bundled review skill', () => { 'relocates an unanchorable Critical into the body as a one-line entry rebuilt from the claim line alone', ); expect(body).toContain('the loss is a named acceptance, not a silent one'); + // R4: the named residue covers the two further exits that carry neither + // sentence — the typed deferral line (a `DeferredEntry` holds no + // fix-side field) and the duplicate-drop account (a name-and-location + // pointer, never the finding's own text). + expect(body).toContain( + 'a finding carried into `deferredSuggestions` renders as the typed one-line entry', + ); + expect(body).toContain( + 'a Suggestion dropped as a duplicate posts a name-and-location account only', + ); }); it('pins the fix-induced disposition and both of its operands', () => { diff --git a/packages/core/src/skills/bundled/review/references/posting.md b/packages/core/src/skills/bundled/review/references/posting.md index efbdd4dcc08..14928b49e74 100644 --- a/packages/core/src/skills/bundled/review/references/posting.md +++ b/packages/core/src/skills/bundled/review/references/posting.md @@ -40,7 +40,7 @@ Also skip this step (independently of the gate above) if the review target is no **And a comment whose fix adds a guard carries the test that must pin it.** When the finding's `fixWitness` is anything other than `N/A`, the posted body closes with it, in one sentence of ordinary prose: name the test that must fail if the fix is removed, and ask for the mutation that proves it (remove the guard, run that test, confirm it reds). One sentence, after the suggestion block — not a heading, not a checklist. This is the reviewer-side half of a measured loop: roughly a third of every post-first-round finding on six multi-round pull requests was introduced by the fix immediately before it, overwhelmingly as a guard or branch with no test of its own, and the deterministic gate re-runs only the tests that exist — so an unwitnessed guard passes every gate and returns as next round's finding. A fixer who is told the acceptance criterion closes it in THIS round; one who is not, does not (measured; DESIGN.md — The fix round that wrote the next round's findings (#9578)). The line reaches every fixer — a contributor, a maintainer, any bot — which is the point: the review cannot assume the fix comes from something it can configure. A finding whose `fixWitness` is `N/A` adds nothing (do not write "no test needed" — silence says it), and this sentence never changes what the comment reports or at what severity. -**And a comment whose fix rests on an existing fact carries that fact.** When the finding has a `fixConstraint`, the posted body carries it in one sentence of ordinary prose beside the fix-witness sentence: state the fact the fix must not violate and its source — the quoted constant, the `file:line` — copied from the artifact, not re-derived. `fixWitness` pins the fix's claim; this pins its premises, the class that passed a witnessed test and was still wrong twice on one merged fix — a hand-picked bound below a configurable limit, and a newly shared registry that broke a `callId` uniqueness relied on elsewhere (measured; DESIGN.md — The fix whose premises were wrong (#10153)). The evidence bar is the `witness` bar, and it holds at posting as it did at finding: a constraint that names no constant and no `file:line` is not posted — a wrong `fixWitness` costs one unwritten test; a wrong constraint is misdirection the fixer will follow. A finding with no `fixConstraint` adds nothing — no `N/A`, no "no constraints observed"; the sentence is absent and the comment is that much shorter — and, like the fix-witness sentence, this one never changes what the comment reports or at what severity. And the rule follows the finding through the two moves the orchestrator performs: a Critical carrying either fix-side sentence — the fix-witness or the constraint sentence — that moves to `bodyCriticals` — unmappable, or relocated by the 422 recovery — appends the same sentence to that entry, copied from the artifact, because that entry is the finding's sole published copy, and a witness or a premise lost there is one no fixer ever sees. A third move exists and carries neither sentence: on an Aone target the platform performs no server-side anchor validation, so `submit` validates every anchor itself and relocates an unanchorable Critical into the body as a one-line entry rebuilt from the claim line alone — a channel structurally too narrow for either sentence. Closing that channel is the structured body-Critical projection, larger than this rule; until then the loss is a named acceptance, not a silent one. +**And a comment whose fix rests on an existing fact carries that fact.** When the finding has a `fixConstraint`, the posted body carries it in one sentence of ordinary prose immediately before the fix-witness sentence, which still closes the body — and when the finding's `fixWitness` is `N/A`, where no fix-witness sentence stands, the constraint sentence takes its place after the suggestion block: state the fact the fix must not violate and its source — the quoted constant, the `file:line` — copied from the artifact, not re-derived. `fixWitness` pins the fix's claim; this pins its premises, the class that passed a witnessed test and was still wrong twice on one merged fix — a hand-picked bound below a configurable limit, and a newly shared registry that broke a `callId` uniqueness relied on elsewhere (measured; DESIGN.md — The fix whose premises were wrong (#10153)). The evidence bar is the `witness` bar, and it holds at posting as it did at finding: a constraint that names no constant and no `file:line` is not posted — a wrong `fixWitness` costs one unwritten test; a wrong constraint is misdirection the fixer will follow. A finding with no `fixConstraint` adds nothing — no `N/A`, no "no constraints observed"; the sentence is absent and the comment is that much shorter — and, like the fix-witness sentence, this one never changes what the comment reports or at what severity. And the rule follows the finding through the two moves the orchestrator performs: a Critical carrying either fix-side sentence — the fix-witness or the constraint sentence — that moves to `bodyCriticals` — unmappable, or relocated by the 422 recovery — appends the same sentence to that entry, copied from the artifact — the constraint before the witness when the finding carries both — because that entry is the finding's sole published copy, and a witness or a premise lost there is one no fixer ever sees. A third move exists and carries neither sentence: on an Aone target the platform performs no server-side anchor validation, so `submit` validates every anchor itself and relocates an unanchorable Critical into the body as a one-line entry rebuilt from the claim line alone — a channel structurally too narrow for either sentence. Closing that channel is the structured body-Critical projection, larger than this rule. Two more moves exist and carry neither sentence: a finding carried into `deferredSuggestions` renders as the typed one-line entry — a `DeferredEntry` holds no fix-side field, and the line is capped at 240 characters — and a Suggestion dropped as a duplicate posts a name-and-location account only, never the finding's own text — both structurally too narrow for either sentence; until a deferral entry grows the fields, the loss is a named acceptance, not a silent one. **Resolve every anchor before you submit — do not post the line numbers the agents reported.** GitHub rejects the whole review with a 422 if any comment's `(path, line)` falls outside every hunk of that file, and it does so all-or-nothing: one miscounted anchor takes every Critical in the review down with it. The line is therefore computed from the diff, not carried over from an agent. The resolver input already exists — Step 6's `findings --to-anchors` wrote it from the artifact, one entry per anchored location of every high-confidence Critical and Suggestion (do NOT hand-project it from the artifact's `locations[]`: the resolver wants `path` where the artifact stores `file`, and a hand projection once produced all-null anchors). Run the resolver: