From eda4e2f45be2c8d2532a42c389d2be4b83793c1e Mon Sep 17 00:00:00 2001 From: wenshao Date: Sun, 23 Aug 2026 00:51:33 +0800 Subject: [PATCH 1/4] fix(review): count a fix-induced re-report as first-time work MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #9674. A carried id has meant two different things since the fix-induced disposition shipped, and the volume trend's first-time count read both as re-posts. One is a re-post: a finding re-asserted under the id it already had. The other is a new defect wearing the id of the entry whose fix produced it, carried deliberately so the author reads one thread per churning site instead of a new one every round. Counting that as a re-post made the trend understate new work exactly where the loop was creating the most of it — measured on the pull request that introduced the disposition, a round that newly identified six defects and re-reported four of them under earlier ids recorded a first-time count of two. Neither count moves. They measure different things and both readings are correct, which is why the two reconciliations the issue rules out stay ruled out: excluding carried-id re-reports from the census would put the attributed count outside it and every such census would be refused as impossible, and counting them as first-time posts wholesale would tell the trend a re-assertion is new work. What was missing is the distinction itself, so the comment now carries it: a fix-induced re-report is marked, and the reader of drafted comments passes that through to the count. The marking sits after the id and its separator, never inside the id grammar. That grammar is shared with the ledger's own carry, so widening it to swallow a parenthetical would put a finding's identity on the same regex as a model-written adjective — a spacing the wider grammar failed to anticipate would stop matching the id and silently renumber the finding. Read after the id, nothing about the token can cost it, and the reading is correspondingly lenient about case and spacing because it governs only whether a comment counts as first-time work. An unrecognised marking leaves the draft counted as a re-post, which is what every round did before this existed; a marking wrongly added to a still-stands is the expensive direction, so the skill restricts the token rather than offering it as a way to flag any carried finding as interesting. The token is stripped from the claim before it reaches the work list. Left in, it would ride into the next round as part of the text Step 6 re-locates the claim by and the status table prints — machine vocabulary about how to count one round, outliving the round it described. Beside no id it is ordinary claim text and survives, because there is no entry there for it to qualify and editing a finding's own words on the strength of a word it opened with is not this token's business. --- .../commands/review/compose-review.test.ts | 178 ++++++++++++++++++ .../cli/src/commands/review/compose-review.ts | 54 +++++- .../commands/review/lib/convergence.test.ts | 55 ++++++ .../src/commands/review/lib/convergence.ts | 31 +++ .../core/src/skills/bundled/review/SKILL.md | 2 +- .../src/skills/bundled/review/SKILL.test.ts | 19 ++ 6 files changed, 335 insertions(+), 4 deletions(-) diff --git a/packages/cli/src/commands/review/compose-review.test.ts b/packages/cli/src/commands/review/compose-review.test.ts index 475399226ca..5a29e6f62e5 100644 --- a/packages/cli/src/commands/review/compose-review.test.ts +++ b/packages/cli/src/commands/review/compose-review.test.ts @@ -5577,6 +5577,47 @@ describe('buildLedger', () => { expect(l.findings.map((f) => f.id)).toEqual(['R1-2', 'R1-3']); expect(l.findings[0]?.title).toBe('still leaking'); }); + + it('keeps the fix-induced marking out of the carried entry title', () => { + // The marking is machine vocabulary about how to COUNT the comment, not + // part of the claim. Left in, it rides the work list into the next round, + // where "R1-2 (fix-induced) the retry guard drops a valid case" is the + // text Step 6 re-locates the claim by and the text the status table + // prints — the token outliving the round it described, on every carried + // entry, forever. + const l = buildLedger( + 4, + [ + { + path: 'src/retry.ts', + line: 9, + body: '**[Critical]** R1-2: (fix-induced) the guard drops a valid case', + }, + ], + [], + ); + expect(l.findings[0].id).toBe('R1-2'); + expect(l.findings[0].title).toBe('the guard drops a valid case'); + expect(l.findings[0].title).not.toContain('fix-induced'); + + // ...and the stripping happens ONLY beside an id. With no id there is no + // entry for the token to qualify, so it is ordinary claim text and must + // survive into the title — stripping it there would edit a finding's own + // words on the strength of a word it happened to open with. + const idless = buildLedger( + 4, + [ + { + path: 'src/retry.ts', + line: 9, + body: '**[Critical]** (fix-induced) a brand new hole', + }, + ], + [], + ); + expect(idless.findings[0].id).toBe('R4-1'); + expect(idless.findings[0].title).toBe('(fix-induced) a brand new hole'); + }); }); describe('the ledger marker reaches the POSTED body', () => { @@ -9730,6 +9771,48 @@ describe('convergence diagnosis reaches the POSTED body', () => { expect(r.postedFresh).toBe(1); }); + it('counts a fix-induced re-report as first-time work', () => { + // Issue #9674. A carried id means two different things since the + // fix-induced disposition shipped: a claim re-asserted, and a NEW defect + // wearing the id of the entry whose fix produced it. Reading the id alone + // called both re-posts, so the trend's baseline fell on exactly the + // churning pull requests where new work was not falling. + // + // Both arms over ONE fixture: same id, same work list, same everything + // but the marking. Without the differential the assertion would pass on + // a count that simply never moves. + const round = (body: string) => { + sideFile({ + round: 3, + posted: 1, + fresh: 1, + findings: [{ id: 'R2-1', sev: 'C', file: 'src/p.ts', title: 'x' }], + }); + return composeReview({ + planPath: plan(), + modelId: 'm', + criticalsInline: 1, + suggestionsInline: 0, + draftedComments: [{ path: 'src/p.ts', line: 1, body }], + }); + }; + const stillStands = round('**[Critical]** R2-1: still open'); + expect(parseLedger(stillStands.body)!.fresh).toBe(0); + expect(stillStands.postedFresh).toBe(0); + + const fixInduced = round( + '**[Critical]** R2-1: (fix-induced) the fix opened a new hole', + ); + expect(parseLedger(fixInduced.body)!.fresh).toBe(1); + expect(fixInduced.postedFresh).toBe(1); + // ...and the id still carries, so the author still reads one thread for + // the site. Counting it first-time is a change to the COUNT, never to + // which finding the comment is. + const marker = parseLedger(fixInduced.body)!; + expect(marker.findings.map((x) => x.id)).toEqual(['R2-1']); + expect(marker.findings[0].title).toBe('the fix opened a new hole'); + }); + it('re-mints a stray id, and keeps one a shortened list may have shed', () => { // A claimed id naming no entry in a COMPLETE work list is a stray, and // recording it mints a finding under a round that never held it — which @@ -11079,4 +11162,99 @@ describe('draftedFindingsOf — the drafts as the convergence diagnosis reads th ]), ).toEqual([{ file: '' }, { file: '' }]); }); + + it('reads the fix-induced marking beside the id it qualifies', () => { + const [d] = draftedFindingsOf([ + critical({ body: '**[Critical]** R1-2: (fix-induced) the new hole' }), + ]); + expect(d.carriedId).toBe('R1-2'); + expect(d.fixInduced).toBe(true); + // A still-stands re-post is the SAME id shape without the marking, and + // must stay a re-post: the whole point of the token is that a carried id + // no longer answers the first-time question on its own. + const [plain] = draftedFindingsOf([ + critical({ body: '**[Critical]** R1-2: the same old claim' }), + ]); + expect(plain.carriedId).toBe('R1-2'); + expect(plain.fixInduced).toBeUndefined(); + }); + + it('tolerates case and inner spacing in the marking', () => { + // It governs a COUNT, never which finding a comment is, so the reading is + // deliberately lenient. A spelling it still misses costs the count and + // nothing else — the id is already in hand by then. + for (const marked of [ + '**[Critical]** R1-2: (Fix-Induced) x', + '**[Critical]** R1-2: ( fix-induced ) x', + '**[Critical]** R1-2: (FIX-INDUCED)- x', + ]) { + const [d] = draftedFindingsOf([critical({ body: marked })]); + expect(d.carriedId).toBe('R1-2'); + expect(d.fixInduced).toBe(true); + } + }); + + it('ignores the marking where there is no id to qualify', () => { + // Nothing induced a defect that names no previous entry, and the comment + // is already counted first-time by the absent id. Honouring the token + // there would let a stray parenthetical speak about an entry the comment + // does not name. + const [d] = draftedFindingsOf([ + critical({ body: '**[Critical]** (fix-induced) a brand new hole' }), + ]); + expect(d.carriedId).toBeUndefined(); + expect(d.fixInduced).toBeUndefined(); + }); + + it('never emits the marking without the id it qualifies', () => { + // The shape invariant, pinned where it is actually reachable: a SECOND + // draft under an id this round already spent has its `carriedId` dropped + // (the ledger mints it a fresh one), and the marking must go with it. + // Kept, the finding would claim to have been induced by an entry it no + // longer names — a shape this field's own contract forbids. + // + // This case is why the assertion above could not carry the whole rule: + // there the id is absent because the body has none, and the two gates + // that enforce this — the readback's and the projection's — masked each + // other, so neither could be reddened alone. + const both = draftedFindingsOf([ + critical({ body: '**[Critical]** R2-1: (fix-induced) first' }), + critical({ body: '**[Critical]** R2-1: (fix-induced) second' }), + ]); + expect(both[0].carriedId).toBe('R2-1'); + expect(both[0].fixInduced).toBe(true); + expect(both[1].carriedId).toBeUndefined(); + expect(both[1].fixInduced).toBeUndefined(); + }); + + it('never lets the marking cost the id', () => { + // The reason the token sits AFTER the separator instead of inside the id + // grammar. `LEDGER_ID_READBACK` is shared with `idFor`, so widening it to + // swallow a parenthetical would put the ledger's carry on the same regex + // as a model-written adjective: a spacing the wider grammar failed to + // anticipate would stop matching the id and the finding would be silently + // renumbered. + // + // Written as a DIFFERENTIAL rather than as a list of ids to expect. The + // first cut asserted `R1-2:(fix-induced) x` carries its id and reddened — + // correctly: `R1-2:claim` loses the id with no marking anywhere near it, + // because the shared grammar wants whitespace after the separator. That + // is pre-existing and not this token's business. What IS this token's + // business is that it changes nothing: every shape reads exactly the id + // it would have read with the marking deleted. + const idOf = (body: string) => + draftedFindingsOf([critical({ body })])[0]?.carriedId; + for (const [marked, bare] of [ + ['**[Critical]** R1-2: (fix-induced) x', '**[Critical]** R1-2: x'], + ['**[Critical]** R1-2 (fix-induced) x', '**[Critical]** R1-2 x'], + ['**[Critical]** R1-2: (fix induced) x', '**[Critical]** R1-2: x'], + ['**[Critical]** R1-2: (fix-induced x', '**[Critical]** R1-2: x'], + ['**[Critical]** R1-2:(fix-induced) x', '**[Critical]** R1-2:x'], + ]) { + expect(idOf(marked)).toBe(idOf(bare)); + } + // ...and the differential is not vacuously true because every arm is + // undefined: the prescribed shape does carry its id. + expect(idOf('**[Critical]** R1-2: (fix-induced) x')).toBe('R1-2'); + }); }); diff --git a/packages/cli/src/commands/review/compose-review.ts b/packages/cli/src/commands/review/compose-review.ts index dd3563b76ef..f15686379dd 100644 --- a/packages/cli/src/commands/review/compose-review.ts +++ b/packages/cli/src/commands/review/compose-review.ts @@ -5249,12 +5249,54 @@ export const composeReviewCommand: CommandModule = { * a second restatement would let one end call a comment carried while the * other calls it new. */ -function readClaim(rest: string): { id?: string; title: string } { +/** + * The fix-induced marking, read from the head of the CLAIM — after the id and + * its separator, never inside the id grammar. + * + * Placing it there is the whole point. `LEDGER_ID_READBACK` is shared by + * `idFor`, so widening it to swallow a parenthetical would put the ledger's + * carry on the same regex as a model-written adjective: a spelling or spacing + * the wider grammar failed to anticipate (`R1-2(Fix-Induced):`) would stop + * matching the id at all, and the finding would be silently renumbered — the + * exact failure "one finding, one name" exists to prevent. Read here, the id + * is already in hand and nothing about this token can cost it: an unrecognised + * marking leaves the draft counted as a re-post, which is what every round did + * before this existed. + * + * Case-insensitive, and tolerant of inner spacing, because it governs only + * whether a comment counts as first-time work — never which finding it is. + */ +const FIX_INDUCED_READBACK = /^\(\s*fix-induced\s*\)[:.,-]?\s*/i; + +/** + * The id a claim line carries, whether that id fronts a NEW defect, and the + * claim itself with both stripped. + * + * `fixInduced` is the answer to a question the id alone cannot settle. Step 6 + * re-reports two different things under a previous entry's id: a finding that + * STILL STANDS — the same claim, re-asserted — and a fix-induced defect, which + * is new work wearing the id of the entry whose fix produced it. The volume + * trend counts comments posted for the first time, and reading the id alone + * called both of them re-posts, so the trend's baseline fell on exactly the + * churning pull requests where new work was not falling at all. + */ +function readClaim(rest: string): { + id?: string; + fixInduced: boolean; + title: string; +} { const line = rest.split('\n')[0].trim(); const carried = LEDGER_ID_READBACK.exec(line); + const afterId = (carried ? line.slice(carried[0].length) : line).trim(); + // Only ever a marking on a CARRIED id. On a fresh finding there is no + // entry for the defect to have been induced by, so the token would be + // decoration — and honouring it there would let a stray parenthetical add + // a first-time count the round already gets for that comment anyway. + const marked = carried ? FIX_INDUCED_READBACK.exec(afterId) : null; return { id: carried?.[1], - title: (carried ? line.slice(carried[0].length) : line).trim(), + fixInduced: marked !== null, + title: (marked ? afterId.slice(marked[0].length) : afterId).trim(), }; } @@ -5337,7 +5379,7 @@ export function draftedFindingsOf(drafted: unknown): DraftedFinding[] { const seen = new Set(); for (const c of drafted as Array<{ path?: unknown; body?: unknown }>) { if (severityOf(c) === null) continue; - const { id } = readClaim(ledgerClaimLine(c.body)); + const { id, fixInduced } = readClaim(ledgerClaimLine(c.body)); // The same length bound `idFor` applies before it will carry an id: an // id the serializer refuses is one no work list can hold, so treating it // as a re-post here would call a finding carried that the ledger mints @@ -5350,6 +5392,12 @@ export function draftedFindingsOf(drafted: unknown): DraftedFinding[] { out.push({ file: typeof c.path === 'string' ? c.path : '', ...(carried === undefined ? {} : { carriedId: carried }), + // Only alongside the id it qualifies. A second draft under an id this + // round already spent has its `carriedId` dropped just above — the + // ledger mints it a fresh one — so it is first-time work by the id + // alone, and carrying the marking without the id would state a + // relationship to an entry this comment no longer names. + ...(carried !== undefined && fixInduced ? { fixInduced: true } : {}), }); } return out; diff --git a/packages/cli/src/commands/review/lib/convergence.test.ts b/packages/cli/src/commands/review/lib/convergence.test.ts index 88c75f29dcc..6cb3745d4ec 100644 --- a/packages/cli/src/commands/review/lib/convergence.test.ts +++ b/packages/cli/src/commands/review/lib/convergence.test.ts @@ -7,6 +7,7 @@ import { describe, it, expect } from 'vitest'; import { diagnoseConvergence, + isFreshDraft, recommendationsFor, renderConvergenceDiagnosis, renderMechanismHealth, @@ -1076,3 +1077,57 @@ describe('renderConvergenceDiagnosis — what the author reads', () => { expect(r.zh).not.toContain('上一轮发布了'); }); }); + +describe('isFreshDraft — a carried id no longer answers on its own', () => { + // Issue #9674. Two different things reach this function under a previous + // round's id: a claim re-asserted (`still stands`) and a NEW defect wearing + // the id of the entry whose fix produced it (`fix-induced`). Only the first + // is a re-post. + const carried = new Set(['R2-1']); + + it('reads a plain carried id as a re-post, as it always did', () => { + expect(isFreshDraft({ file: 'a.ts', carriedId: 'R2-1' }, 4, carried)).toBe( + false, + ); + }); + + it('reads a fix-induced carried id as first-time work', () => { + expect( + isFreshDraft( + { file: 'a.ts', carriedId: 'R2-1', fixInduced: true }, + 4, + carried, + ), + ).toBe(true); + }); + + it('leaves a finding with no id fresh either way', () => { + // The marking adds nothing where the id is absent — that comment is + // already first-time by the id alone — and must not subtract either. + expect(isFreshDraft({ file: 'a.ts' }, 4, carried)).toBe(true); + expect(isFreshDraft({ file: 'a.ts', fixInduced: true }, 4, carried)).toBe( + true, + ); + }); + + it('holds at the round cap, where a plain re-post still reads carried', () => { + // The cap arm returns false for a carried id minted at or past the cap. + // The marking has to reach its answer BEFORE that arm, or the one place + // the counter stops advancing is the one place a fix-induced re-report + // silently stops counting. + expect( + isFreshDraft( + { file: 'a.ts', carriedId: `R${LEDGER_MAX_ROUND}-1` }, + LEDGER_MAX_ROUND, + new Set([`R${LEDGER_MAX_ROUND}-1`]), + ), + ).toBe(false); + expect( + isFreshDraft( + { file: 'a.ts', carriedId: `R${LEDGER_MAX_ROUND}-1`, fixInduced: true }, + LEDGER_MAX_ROUND, + new Set([`R${LEDGER_MAX_ROUND}-1`]), + ), + ).toBe(true); + }); +}); diff --git a/packages/cli/src/commands/review/lib/convergence.ts b/packages/cli/src/commands/review/lib/convergence.ts index f526a56b5b9..887951f39e1 100644 --- a/packages/cli/src/commands/review/lib/convergence.ts +++ b/packages/cli/src/commands/review/lib/convergence.ts @@ -81,6 +81,19 @@ export interface DraftedFinding { * which has no id until this round's ledger is built. */ carriedId?: string; + /** + * The carried id fronts a NEW defect — one the fix for that entry + * introduced — rather than a re-assertion of the entry's own claim. + * + * A carried id means two things since the fix-induced disposition shipped, + * and only one of them is a re-post. Without this the trend counted both, + * so a round that newly identified six defects, four of them re-reported + * under the ids whose fixes produced them, recorded a first-time count of + * two — the baseline falling on exactly the churning pull requests where + * new work was not falling at all. Read off the comment body beside the id, + * so a marking the reader cannot see is one the count does not get either. + */ + fixInduced?: boolean; } /** What the previous round left behind, and how far it can be trusted. */ @@ -302,6 +315,24 @@ export function isFreshDraft( ): boolean { const minted = birthRound(d?.carriedId); if (minted === undefined) return true; + // A fix-induced re-report is first-time work wearing an earlier id. The id + // is bookkeeping — it keeps one churning site on one thread instead of + // opening a new one every round — and reading it as a re-post is what made + // this count understate new work precisely where the loop was creating the + // most of it. Placed before every arm below on purpose: the membership test + // exists to stop a STRAY id from hiding real work and this id is not stray, + // and the round-cap arm is the one place the counter stops advancing, which + // must not also be the place this stops counting. + // + // It is the one input here that can only come from the model asserting + // something, so weigh it the way the caller's own asymmetry note does. An + // OMITTED marking degrades to the reading every round had before it + // existed — a re-post, one round of silence. A marking wrongly added to a + // still-stands would narrate divergence at the steady state, which is the + // expensive error, and that is why the skill restricts the token to a + // re-report that genuinely is fix-induced rather than offering it as a way + // to flag any carried finding as interesting. + if (d?.fixInduced === true) return true; // The id must NAME an entry in the work list it claims to carry forward. // Step 6 teaches the model to lead a re-post with `R1-2: `, and // models emit stray ids at the head of a claim line — so a genuinely new diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index 12ce90ae375..1d0c75860c3 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -798,7 +798,7 @@ The ledger has two sources, in priority order: **the PR itself** — `pr-context - **fixed** — the mechanism can no longer fire. Say so, by id, in one line: `R1-2 fixed by `. Do not re-report it as a finding. The sibling-entrance rule from the re-check below applies here unchanged: for a divergence-class entry, `fixed` is a ruling about the family's entrances, checked one by one — for a **bounded** family a still-open sibling becomes a fresh `R-` entry (for an unbounded surface, apply the bounded/unbounded rule below instead of filing the sibling), never a reason to withhold the original's `fixed`. - **still stands** — re-report it **under its original id**, updating the location if the code moved. It keeps its severity; a still-standing Critical blocks exactly as a new one would. Write that id into the re-report itself, immediately after the severity marker — `**[Critical]** R1-2: ` — and into the body entry if it cannot be anchored (`R1-2 `). That prefix is not decoration: `compose-review` reads it back out of the comment when it builds the marker, and it is the only way an id survives into the machine ledger the next round recovers. Omit it and the same claim comes back renumbered, which is exactly what carrying the id forward exists to prevent. - **cannot tell** — say so by id; a previous-round _Critical_ you cannot rule on joins `cannotTellCriticals` (it caps like any undecided blocker), a Suggestion is just disclosed. -- **fix-induced** — the entry's own reported input is closed, but the change that closed it opened a new defect at the same site. Re-report the NEW defect **under the original id**, exactly as `still stands` prescribes (`**[Critical]** R1-2: `), with the new anchor and the new claim; the status line carries both facts — `R1-2 fix-induced — the round-2 fix closed the reported input and opened at ; carried forward under R1-2`. See the fix-induced rule below for when this applies and when it must not. +- **fix-induced** — the entry's own reported input is closed, but the change that closed it opened a new defect at the same site. Re-report the NEW defect **under the original id**, with the new anchor and the new claim, and **mark it `(fix-induced)` right after the id's colon** — `**[Critical]** R1-2: (fix-induced) `. The id is written exactly as `still stands` prescribes; the marking is the one difference, and it is not decoration. A carried id now fronts two different things — a claim re-asserted, and a NEW defect wearing the id of the entry whose fix produced it — and the volume trend counts comments posted for the FIRST time. Unmarked, a fix-induced re-report reads to that count as a re-post, so a round that newly identified six defects and re-reported four of them under earlier ids records a first-time count of two: the trend falls on exactly the churning pull requests where new work is not falling. Write the marking only on a re-report that IS fix-induced — never on a `still stands`, where the claim genuinely is the old one — and note that a marking the machine misreads costs only the count (the id still carries, and the finding still posts); the status line carries both facts — `R1-2 fix-induced — the round-2 fix closed the reported input and opened at ; carried forward under R1-2`. See the fix-induced rule below for when this applies and when it must not. - **superseded by ``** — the entry is a member of a family that collapsed into one class-level finding (the bounded/unbounded rule below). Record `superseded by ` in the status table; do **not** re-report it and do **not** count it toward `cannotTellCriticals` — the open class finding is the single blocker that carries the family, so the block is preserved without re-enumerating. This is the disposition for a prior sibling that resurfaces in the re-check below after the collapse: it is neither `still stands` (which would re-enumerate and re-carry its id) nor `fixed` (its own mechanism is not closed until the structural change lands) nor `cannot tell` (which would cap the verdict every round until then). Because it is consequence-free (no block, no `cannotTellCriticals` cap, no re-report — and `buildLedger` ingests only re-posted findings, so it leaves no trace), do not take it without verifying the family was actually collapsed into the cited `` and this entry genuinely belongs to it; a mis-applied `superseded` retires a live blocker silently. **Bounded family → enumerate; unbounded family → collapse to one class-level finding.** This rule governs **both** sibling-entrance paths — the ledger `fixed` ruling above and the open-blocker re-check below — so the two cannot disagree. **Boundedness is a property of the SURFACE, not of the round count**: a family is unbounded when its entrances cannot be enumerated and closed one by one — hand-rolled parsing of untrusted input, matching of a rendered format, a re-implemented grammar. (Recurrence across rounds is a _signal_ that prompts the question, never the definition — a finite family can recur twice; an infinite one is unbounded on round one.) For a **bounded** family, enumerate: a still-open sibling is a fresh finding, exactly as the two paths already say. For an **unbounded** one, do not file sibling N — **collapse the whole family into one class-level finding under a single stable id**: `the surface is unbounded; close it structurally — a real parser / the tool's authoritative output / a fail-closed decision — not entrance by entrance`. **The class finding carries one demonstrated entrance as its witness** — the concrete input and the line(s) producing the wrong outcome — so it clears Step 4's high-confidence bar and posts (a shape with no concrete corner confirms only low, and low-confidence findings are terminal-only — they never post and never reach the ledger this backstop reads); the entrance is the class's evidence, not a separate finding. That one finding **supersedes** the family's prior sibling ids: rule each `superseded by ` (the disposition above), fold it in as evidence, and do not re-report it under its own id — the class id is the only one that carries forward, so the next round's **ledger marker** recovers one entry, not N, and a prior sibling that resurfaces on the PR as its own thread is ruled `superseded`, not re-posted. **A brand-new sibling found in the current round** — by a Step 3 finder or Step 5 auditor over the incremental diff, while the class finding is already on the ledger and open — folds the same way: into the class finding's re-report as evidence under the class id at Step 6 rendering, never filed under its own id. **Its severity is the demonstrated risk of the shape** (Agent 3b's rule), Critical when the surface can be fooled into a wrong result, its own severity otherwise — an infinite surface is not automatically a blocker. **Supersession preserves the strongest evidence**: collapse a family only when the class finding is filed at **at least the highest severity AND confidence any absorbed sibling demonstrated** — a proven high-confidence Critical entrance must not be retired behind a low-confidence or non-Critical class finding (which never posts, so nothing carries the block and the defect stays live at a zero-Critical verdict). If the class finding cannot carry that strength, keep the prior Critical open until an equally-strong verified class finding replaces it. Rule the class finding `fixed` only when the structural change lands, never when the latest entrance is patched. (Agent 3b's enumeration-trap check files this same finding _prospectively_ in round 1, before the siblings accumulate; this rule is its cross-round backstop for a family already being enumerated.) diff --git a/packages/core/src/skills/bundled/review/SKILL.test.ts b/packages/core/src/skills/bundled/review/SKILL.test.ts index ce8c85fdc65..e9dcce629c6 100644 --- a/packages/core/src/skills/bundled/review/SKILL.test.ts +++ b/packages/core/src/skills/bundled/review/SKILL.test.ts @@ -764,6 +764,25 @@ describe('bundled review skill', () => { expect(body).toContain('Count the second in `fresh` but not `induced`'); }); + it('pins the fix-induced comment marking and why it is not decoration', () => { + // Issue #9674. The marking is what parts a fix-induced re-report from a + // still-stands re-post for the volume trend's first-time count; without + // the instruction the module's reader finds nothing to read and the + // trend silently understates new work on churning pull requests again. + // Both halves pinned: the FORMAT (what to write) and the RESTRICTION + // (never on a still-stands, where the claim really is the old one). + const body = skillBody(); + expect(body).toContain( + "mark it `(fix-induced)` right after the id's colon", + ); + expect(body).toContain( + '**[Critical]** R1-2: (fix-induced) ', + ); + expect(body).toContain( + 'Write the marking only on a re-report that IS fix-induced — never on a `still stands`', + ); + }); + it('pins the census contract and the module-owns-the-verdict split', () => { // The census is the numerator/denominator the non-convergence finding is // computed from, and three clauses have to survive together: what to From e2633e29d93c2fe2a73fa7d71fc8c18d9a84af4a Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Sat, 22 Aug 2026 20:43:56 +0000 Subject: [PATCH 2/4] fix(review): resolve orphaned readback doc and record the fresh-count seam (#9744) Co-authored-by: Qwen-Coder --- .../cli/src/commands/review/compose-review.ts | 20 ++++++------------- .../cli/src/commands/review/lib/ledger.ts | 14 +++++++++++++ 2 files changed, 20 insertions(+), 14 deletions(-) diff --git a/packages/cli/src/commands/review/compose-review.ts b/packages/cli/src/commands/review/compose-review.ts index 145481beca4..0197a5bfd28 100644 --- a/packages/cli/src/commands/review/compose-review.ts +++ b/packages/cli/src/commands/review/compose-review.ts @@ -5277,20 +5277,6 @@ export const composeReviewCommand: CommandModule = { }, }; -/** - * The first line of what follows the severity marker, minus any carried id. - * A carried-forward finding names its ORIGINAL id right after the marker — - * `**[Critical]** R1-2: the same claim, re-reported` — and reading it back - * here is what makes the machine ledger agree with the report it rides in, - * instead of renumbering the entry to a fresh `R-` the report - * never used. - * - * Module-level rather than a closure inside the ledger builder, because the - * builder is no longer its only consumer: the convergence diagnosis reads the - * same id to tell a re-posted still-standing finding from fresh activity, and - * a second restatement would let one end call a comment carried while the - * other calls it new. - */ /** * The fix-induced marking, read from the head of the CLAIM — after the id and * its separator, never inside the id grammar. @@ -5321,6 +5307,12 @@ const FIX_INDUCED_READBACK = /^\(\s*fix-induced\s*\)[:.,-]?\s*/i; * trend counts comments posted for the first time, and reading the id alone * called both of them re-posts, so the trend's baseline fell on exactly the * churning pull requests where new work was not falling at all. + * + * Module-level rather than a closure inside the ledger builder, because the + * builder is no longer its only consumer: the convergence diagnosis reads the + * same id to tell a re-posted still-standing finding from fresh activity, and + * a second restatement would let one end call a comment carried while the + * other calls it new. */ function readClaim(rest: string): { id?: string; diff --git a/packages/cli/src/commands/review/lib/ledger.ts b/packages/cli/src/commands/review/lib/ledger.ts index 172d9722e7f..8eba98702ee 100644 --- a/packages/cli/src/commands/review/lib/ledger.ts +++ b/packages/cli/src/commands/review/lib/ledger.ts @@ -163,6 +163,20 @@ export interface Ledger { * which leaves the trend unevaluable rather than measured on the wrong * number. * + * One accepted seam: the counting RULE changed once — a fix-induced + * re-report (a carried id fronting a NEW defect) moved from re-post to + * first-time — and nothing parts an old-rule marker from a new one, on + * purpose. A loop in flight at the change compares one round counted under + * each rule for exactly one round; the old rule UNDERCOUNTED (it dropped + * the marked re-reports), so the mixed comparison can only fire the volume + * advisory spuriously, never mask a genuine one — and the advisory decides + * nothing, names itself an observation, and heals the round after, when + * both points are counted under the new rule. A marker version was not + * paid for that: `parseLedger` refuses any `v` it does not know, so + * bumping it for a count no gate reads would cost an old reader the WHOLE + * marker — work list, anchor, streak — the same reason every field added + * since has degraded by absence instead. + * * Rides and sheds with `posted`, which it qualifies. */ fresh?: number; From f8f4677b0b24df7eb19110956f6db994b858c413 Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Sat, 22 Aug 2026 23:14:58 +0000 Subject: [PATCH 3/4] fix(review): name the zero-prev masking round in the fresh-count seam note (#9744) Co-authored-by: Qwen-Coder --- packages/cli/src/commands/review/lib/ledger.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/packages/cli/src/commands/review/lib/ledger.ts b/packages/cli/src/commands/review/lib/ledger.ts index 8eba98702ee..3ce73ae97f0 100644 --- a/packages/cli/src/commands/review/lib/ledger.ts +++ b/packages/cli/src/commands/review/lib/ledger.ts @@ -168,8 +168,11 @@ export interface Ledger { * first-time — and nothing parts an old-rule marker from a new one, on * purpose. A loop in flight at the change compares one round counted under * each rule for exactly one round; the old rule UNDERCOUNTED (it dropped - * the marked re-reports), so the mixed comparison can only fire the volume - * advisory spuriously, never mask a genuine one — and the advisory decides + * the marked re-reports), so the mixed comparison can fire the volume + * advisory spuriously — and, when the undercount reaches 0 because the + * predecessor's whole new output was re-reports the old rule dropped, the + * `prev.fresh > 0` restart guard suppresses the comparison, masking the + * advisory for that one round. Either way the advisory decides * nothing, names itself an observation, and heals the round after, when * both points are counted under the new rule. A marker version was not * paid for that: `parseLedger` refuses any `v` it does not know, so From 0d8b5e1877dccbfbb3f3eaf320afb88f02ee66c2 Mon Sep 17 00:00:00 2001 From: wenshao Date: Sun, 23 Aug 2026 09:27:03 +0800 Subject: [PATCH 4/4] docs(review): stop the blocker's docblock forbidding what this PR ships MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three passages written when a carried id could only mean one thing now forbid the rule this branch adds. The blocker's docblock rules out "counting them as first-time POSTS" as one of two reconciliations that must never be made; the skill's census paragraph says the volume trend is the count "where a carried id is a re-post"; and a test comment restates the same premise. Each was true before a fix-induced re-report could be marked, and each now tells the next reader to undo the code beside it. The distinction the passages were protecting is real and stays. What they ruled out was reading a carried id as first-time work BY INFERENCE, which would count every re-assertion of a standing finding as new work — still wrong, and still what `isFreshDraft` refuses. What this branch added is narrower and is not an inference: the round marks the re-report, and only a marked one counts. An unmarked carried id is a re-post to the trend exactly as before, so the two counts still diverge by design; what is gone is the premise that a carried id can mean only one thing. The census paragraph gains the consequence that follows for whoever writes the comment: a fix-induced finding counted in the census but left unmarked in the body is counted by neither number. Prose only — no logic, no test assertion changes. Reported twice by the review as a deferred finding and left standing under the code-age rule, which is correct as a posting decision and not a reason to leave a contradiction in the file. --- .../commands/review/compose-review.test.ts | 10 ++++-- .../cli/src/commands/review/compose-review.ts | 34 ++++++++++++------- .../core/src/skills/bundled/review/SKILL.md | 2 +- 3 files changed, 30 insertions(+), 16 deletions(-) diff --git a/packages/cli/src/commands/review/compose-review.test.ts b/packages/cli/src/commands/review/compose-review.test.ts index 5a29e6f62e5..1ad244c64d8 100644 --- a/packages/cli/src/commands/review/compose-review.test.ts +++ b/packages/cli/src/commands/review/compose-review.test.ts @@ -10844,9 +10844,13 @@ describe('the convergence census and the non-convergence finding', () => { it("never borrows the posting trend's words for its own count", () => { // The two counts in one body: this blocker counts DEFECTS newly // identified, the convergence diagnosis counts inline comments POSTED - // for the first time, and they legitimately differ — a fix-induced - // defect re-reported under a carried id is new here and a re-post - // there. They collided in VOCABULARY, not arithmetic: "findings first + // for the first time, and they legitimately differ — this one takes + // every finding the round newly identified, the trend only those that + // reached the pull request as a first-time comment. (A fix-induced + // re-report used to be the sharpest case of the two diverging; since + // #9674 the trend counts a MARKED one as first-time too, and an + // unmarked carried id is still a re-post there.) They collided in + // VOCABULARY, not arithmetic: "findings first // filed in round 4" sat beside "2 of them reported for the first time" // over the same round, so one body published two numbers under one // phrase and neither could be trusted. Pin the separation from both diff --git a/packages/cli/src/commands/review/compose-review.ts b/packages/cli/src/commands/review/compose-review.ts index 0197a5bfd28..03dd7a1f9d8 100644 --- a/packages/cli/src/commands/review/compose-review.ts +++ b/packages/cli/src/commands/review/compose-review.ts @@ -1638,19 +1638,29 @@ export function aboveChurnBar( * **It counts DEFECTS, and it must not borrow the posting trend's words.** * The same body carries the convergence diagnosis, which counts inline * comments POSTED for the first time — and the two numbers legitimately - * differ: a fix-induced defect re-reported under the id it came from is - * newly identified here and a re-post there, so a round can newly identify - * six defects while posting two first-time comments. Both readings are - * right; what broke was the vocabulary, when this sentence said "findings - * first filed" beside the diagnosis's "reported for the first time" and one - * body published two numbers under one phrase. Hence "defects … newly - * identified" — distinct words for a distinct quantity. + * differ: this count takes every finding the round newly identified, the + * trend only those that reached the pull request as a first-time comment, so + * a round can newly identify six defects while posting fewer than six + * first-time comments (some ride body Criticals, some deferrals). Both + * readings are right; what broke was the vocabulary, when this sentence said + * "findings first filed" beside the diagnosis's "reported for the first + * time" and one body published two numbers under one phrase. Hence "defects + * … newly identified" — distinct words for a distinct quantity. * - * Do NOT "reconcile" the two by changing either count. Excluding carried-id - * re-reports from `fresh` would put `induced` outside it and every such - * census would be refused as impossible; counting them as first-time POSTS - * would tell the volume trend a re-post is new work, which is the reading - * `isFreshDraft` exists to refuse. The divergence is the design. + * Do NOT "reconcile" the two by changing either count WHOLESALE. Excluding + * carried-id re-reports from `fresh` would put `induced` outside it and + * every such census would be refused as impossible; and reading a carried id + * as first-time work by inference would tell the volume trend that every + * re-assertion of a standing finding is new work, which is the reading + * `isFreshDraft` exists to refuse. + * + * What DID change (#9674) is narrower than either, and is not an inference: + * a fix-induced re-report is MARKED as such in the comment body, and the + * trend counts a marked one as first-time. That is a distinction the round + * asserts, not one the id implies — an unmarked carried id is still a + * re-post to the trend, exactly as before. The divergence between the two + * counts is still the design; only the false premise that a carried id can + * mean just one thing is gone. */ export function nonConvergenceCritical( census: { fresh: number; induced: number }, diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index 3b71f3e0ef6..f1e5372fe38 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -811,7 +811,7 @@ The ledger has two sources, in priority order: **the PR itself** — `pr-context **What it buys.** The ledger stops spending one id per round on a single churning site, so the marker's fifty-entry work list holds more distinct claims; the author reads one thread per site instead of a new one each round; and the count this produces — how many of the round's findings were fix-induced — is what the non-convergence rule below reads. That count is the honest measure of a loop's productivity, and it is not available to a review that renumbers everything every round. -**Count the round as you rule it, and hand the two numbers over.** While you walk the findings above, keep a running census of exactly two numbers. **`fresh`** — how many DEFECTS this round NEWLY IDENTIFIED, counted over what the round REPORTS: the inline comments drafted for posting, the body Criticals, and the deferrals — the three channels `compose-review` cross-checks the number against: a `fresh` larger than everything reported across all three is refused as no census at all. The check is one-sided — an under-count passes it — so accuracy below that ceiling is yours to keep. Fix-induced findings count whether they took a previous id or a new one (they are new defects; the id is bookkeeping) — which is why this count is **not** the marker's `fresh`, the volume trend's count of comments POSTED for the first time, where a carried id is a re-post. Two different quantities that legitimately differ on exactly the churning rounds this mechanism is for; the blocker says "newly identified" and the trend says "reported for the first time" so one body never publishes two numbers under one phrase. NOT counted: the entries you ruled `still stands`, `fixed`, `cannot tell` or `superseded`; findings you confirmed but that were dropped as duplicates of already-reported findings — they RESTATE a defect an earlier round identified (the duplicates paragraph DISCLOSES the confirmation; it is not a fourth reporting channel), so they are not newly identified; and findings that reach no channel — low-confidence findings are terminal-only, and a draft discarded as unanchorable posts nothing. Deferrals take `D-` ids in the artifact, but they ARE reports — count the ones MINTED this round: a deferral whose defect first appeared in an earlier round and is deferred again is not `fresh`, and counting re-deferrals every round inflates `fresh` with old never-induced findings, delaying the blocker precisely on the long-lived critical-floor pull requests this mechanism exists for. **`induced`** — how many of those `fresh` findings the fix-induced rule above **attributed**: the ones that TOOK a previous entry's id under that rule — the spent-id second defect of the guardrail above counts in `fresh` but not `induced`, exactly as it says there. `induced` is a SUBSET of `fresh` and can never exceed it. **It is the attributed count, not the count of findings on new lines**, and the difference is the whole precision of the mechanism: a pull request whose author pushed a new feature between rounds has most of its new findings on new lines and has NOT created them out of the review — there is no previous entry to trace them to, so they are `fresh` and not `induced`. A bar built on the looser number would block a pull request for growing. Carry the pair into the compose state as `convergence: {"fresh": N, "induced": M}` — one object, two integers, no prose. Omit the field entirely when you could not measure it: no `commitId`, no worktree, the **context-unavailable** state (the module refuses a census under it anyway, symmetric with round 1), or an age reference that failed validation. **Omitting is not the same as zero**, though both carry the streak: `compose-review` resets the streak only on a measured below-bar census with at least 4 `fresh` — absence, a malformed pair, and a census too small to be a trend (fewer than 4 `fresh`, zeros included) all carry the count untouched, because "could not measure" is not "measured and converging". Writing `{"fresh": 0, "induced": 0}` for a round you did not measure does not erase the standing claim — it states a measurement the round never made, a found-nothing reading of a round that could not measure. Omit the field: absence is the honest signal. +**Count the round as you rule it, and hand the two numbers over.** While you walk the findings above, keep a running census of exactly two numbers. **`fresh`** — how many DEFECTS this round NEWLY IDENTIFIED, counted over what the round REPORTS: the inline comments drafted for posting, the body Criticals, and the deferrals — the three channels `compose-review` cross-checks the number against: a `fresh` larger than everything reported across all three is refused as no census at all. The check is one-sided — an under-count passes it — so accuracy below that ceiling is yours to keep. Fix-induced findings count whether they took a previous id or a new one (they are new defects; the id is bookkeeping) — which is why this count is **not** the marker's `fresh`, the volume trend's count of comments POSTED for the first time: an UNMARKED carried id is a re-post there, and only the `(fix-induced)` marking on the comment tells it otherwise — so a fix-induced finding you count here but leave unmarked in the body is counted by neither. Two different quantities that legitimately differ on exactly the churning rounds this mechanism is for; the blocker says "newly identified" and the trend says "reported for the first time" so one body never publishes two numbers under one phrase. NOT counted: the entries you ruled `still stands`, `fixed`, `cannot tell` or `superseded`; findings you confirmed but that were dropped as duplicates of already-reported findings — they RESTATE a defect an earlier round identified (the duplicates paragraph DISCLOSES the confirmation; it is not a fourth reporting channel), so they are not newly identified; and findings that reach no channel — low-confidence findings are terminal-only, and a draft discarded as unanchorable posts nothing. Deferrals take `D-` ids in the artifact, but they ARE reports — count the ones MINTED this round: a deferral whose defect first appeared in an earlier round and is deferred again is not `fresh`, and counting re-deferrals every round inflates `fresh` with old never-induced findings, delaying the blocker precisely on the long-lived critical-floor pull requests this mechanism exists for. **`induced`** — how many of those `fresh` findings the fix-induced rule above **attributed**: the ones that TOOK a previous entry's id under that rule — the spent-id second defect of the guardrail above counts in `fresh` but not `induced`, exactly as it says there. `induced` is a SUBSET of `fresh` and can never exceed it. **It is the attributed count, not the count of findings on new lines**, and the difference is the whole precision of the mechanism: a pull request whose author pushed a new feature between rounds has most of its new findings on new lines and has NOT created them out of the review — there is no previous entry to trace them to, so they are `fresh` and not `induced`. A bar built on the looser number would block a pull request for growing. Carry the pair into the compose state as `convergence: {"fresh": N, "induced": M}` — one object, two integers, no prose. Omit the field entirely when you could not measure it: no `commitId`, no worktree, the **context-unavailable** state (the module refuses a census under it anyway, symmetric with round 1), or an age reference that failed validation. **Omitting is not the same as zero**, though both carry the streak: `compose-review` resets the streak only on a measured below-bar census with at least 4 `fresh` — absence, a malformed pair, and a census too small to be a trend (fewer than 4 `fresh`, zeros included) all carry the count untouched, because "could not measure" is not "measured and converging". Writing `{"fresh": 0, "induced": 0}` for a round you did not measure does not erase the standing claim — it states a measurement the round never made, a found-nothing reading of a round that could not measure. Omit the field: absence is the honest signal. **You count; the module rules.** `compose-review` owns the threshold, the streak and the finding — do not compute a verdict from these numbers yourself, do not mention convergence in your Summary on the strength of them, and do not adjust what you post because of them. When two rounds come in counted against the bar, the module appends its own body Critical (`This pull request is not converging…`) with the counts, and the event becomes `REQUEST_CHANGES`. **That finding is the module's, and the narrated-away-cap rule covers it exactly**: it is not yours to soften, re-word, delete from the body, or explain away in the Summary, any more than a cap is — if you believe it is wrong, the answer is a corrected census, never a corrected verdict. It is deterministic by provenance (this module counted it from its own marker and your census), so no verifier is owed and none will ever exist for it; it carries no anchor because the claim is about the pull request, not a line.