diff --git a/docs/design/review-divergence-sentinel.md b/docs/design/review-divergence-sentinel.md new file mode 100644 index 00000000000..a5ec209b3e1 --- /dev/null +++ b/docs/design/review-divergence-sentinel.md @@ -0,0 +1,116 @@ +# Review successor-chain sentinel (issue #9905) + +## Problem + +The `/review` cross-round ledger tracks finding ids, carries, and +supersession, but nothing reads the _lineage_: when the fix for round N's +Critical produces round N+1's Critical on the same subsystem, every round +looks locally normal while the subsystem is diverging. On #9659 the +stop-round blocker-dating chain produced three generations of successor +Criticals (R9-1 → R10-2/3/4 → R11-4/R11-6 → R12-1/R12-2); recognizing the +divergence took a manual round-count analysis and cost four rounds of +patch-and-regress before the mechanism was deleted and the open-finding +count collapsed. + +The convergence module's recurrence cluster already says "the file sees +findings again" — it cannot say "the fix closed one and the mechanism grew +another", because no round records the _closures_. + +## Design + +A deterministic lineage check inside `diagnoseConvergence`, fed by a +bounded closure list the ledger marker carries. No model judgement: it is +a walk over data the pipeline already writes. + +### The marker carries one `closed` list + +`Ledger` gains an optional `closed` array. An entry `{r, id, f}` records a +**Critical** that was open in the previous round's work list and is absent +from this round's: composing round `r` diffs the recovered list against the +built ledger and appends one entry per vanished Critical id. Suggestions +are not tracked — Critical churn is the signal. `fixed` and `superseded` +both read as closure; a positional diff cannot tell them apart, and the +advisory note does not need it to. + +The list carries **one generation, no carry-forward**: the check at round +N reads closures at N (minted while composing) and N-1 (read back off the +previous marker), so older entries are dead bytes. Bounds, matching the +marker's "footnote, never a payload" contract: + +- count: `LEDGER_MAX_CLOSED` (50) entries — one full work list — newest + kept; +- bytes: in the serializer's shed cascade the closures go **after** the + volume telemetry and **before** the anchor pair and the work list — + advisory data never costs a re-review or a ruling. Shedding them never + sets `dropped`: closures certify no range. + +Minting is suppressed wherever absence from the posting set does not +mean "ruled fixed" — the same honesty legs the `openCriticals` gate +applies to the identical inference: an incomplete recovered work list +(`dropped`/rejected entries — a missing id may be truncation, not a +ruling), a diff-only round that could not rule, a round that publicly +answered "cannot tell" on a Critical, and a pure-foreign previous list +whose entries are a stranger's. Minting also closes on CLAIM identity, +not id identity: a claim the round re-posts under a re-minted id (a +regenerated gate Critical, a model re-post the readback lost) still +stands. The re-post channels — the deferral channel and the +floor-enforced reroute — join on the ID they carry: an entry whose title +bears the original finding id keeps that claim standing under whichever +severity or path re-posts it, and an entry that bears none leaves the +round unable to prove what it re-posts, so the mint fails closed and +mints nothing that round. Thin history stays silent rather than +guesses. + +`parseLedger` validates closures through `isLedgerClosure` (caps and the +round bound, mirrored from the serializer); `prevLedgerFacts` carries them +into compose through the same admission test on the side-file route. + +### The check (K = 2, current round included) + +Inside `diagnoseConvergence`, beside the recurrence cluster: a file F +fires a **successor chain** when + +- F closed a Critical in the previous round (`prev.closed` with + `r === round - 1`), and +- F closes a Critical this round (`closuresThisRound` with `r === round`), + and +- this round posts a **fresh** Critical on F (the built ledger's + `R-*` ids — the built id's round IS the first-reported round, + after readback and admission). + +The cluster's own file rules apply unchanged: the `(body)`/`(unknown)` +stand-ins never participate, and the `k` flag still separates a real path +spelled like one. + +Checked against the #9659 evidence: composing round 11 closes R10-2/3/4 +(r=11) on top of R9-1's closure at r=10, while R11-4/R11-6 land on the +same file — the note fires at round 11, one round before the manual +analysis caught the rebound. + +### Output (advisory, first-class) + +The note never moves the event and never withholds the anchor: + +- `ConvergenceDiagnosis.successorChains` carries the typed chains + (`{file, generations, newIds}`), rendered by `renderSuccessorChain` as + `R9-1 → R10-2/R10-3 → R11-4`; +- the convergence observation (posted body + the stderr `CONVERGENCE:` + line) leads with the ⚠️ Divergence sentence naming the subsystem and + the chain, bilingually, `mdField`-escaped like every other + PR-controlled segment; +- `recommendationsFor` emits a new closed-vocabulary code, + `successor-chain`, with the chain in its `basis` — the machine-readable + half the autofix tooling reads. It rides the composed artifact through + the existing `recommendations` validation, which checks codes against + the runtime list. + +### What it deliberately does not do + +- It cannot distinguish `fixed` from `superseded` or a dropped carry — + closure is positional. The note is advisory, so a rare false positive + costs one sentence; the carry-forward machinery (`LEDGER_ID_READBACK`, + the stray-id rules) is what keeps carries honest. +- It never caps the verdict and never withholds the anchor. A divergence + is a property of the code under review, not of what this round read. +- File-level matching only; the ledger records no symbols (the issue's + "symbol, if recorded" — it is not recorded). diff --git a/packages/cli/src/commands/review/compose-review.test.ts b/packages/cli/src/commands/review/compose-review.test.ts index e60e6cc817f..ca5c895ba19 100644 --- a/packages/cli/src/commands/review/compose-review.test.ts +++ b/packages/cli/src/commands/review/compose-review.test.ts @@ -29,9 +29,11 @@ import { import { getGhHost, setGhHost } from './lib/gh.js'; import { BRIEFS } from './lib/agent-briefs.js'; import { + LEDGER_MAX_CLOSED, LEDGER_MAX_FILE, LEDGER_MAX_ID, LEDGER_MAX_ROUND, + LEDGER_MAX_TITLE, LEDGER_MAX_VOLUME, parseLedger, serializeLedger, @@ -13073,6 +13075,30 @@ describe('convergence diagnosis reaches the POSTED body', () => { ], ['a truncated work list', {}, { dropped: 3 }], ['a pure-foreign work list', {}, { foreign: true }], + ['an anonymously adopted work list', {}, { anonymousAdoption: true }], + [ + 'a re-post the work list cannot place', + { + // `auto` licences the deferral channel at round 5 — an ABSENT floor + // beside a non-empty list caps with `unlicensed-deferral`, and the + // cap leg would withhold the ending instead of the leg under test. + severityFloor: 'auto' as const, + deferredSuggestions: [ + { + file: 'src/a.ts', + line: 5, + source: 'review', + severity: 'Suggestion', + title: 'the claim, restated without its id', + } as DeferredEntry, + ], + }, + { + findings: [ + { id: 'R2-1', sev: 'C', file: 'src/a.ts', title: 'the claim' }, + ], + }, + ], ])('withholds land-and-defer over %s', (_label, inputOver, sideOver) => { // Each arm starts from the shape that DOES offer the ending and flips // exactly one leg, so the assertion measures that leg and not a sibling @@ -13188,6 +13214,1050 @@ describe('convergence diagnosis reaches the POSTED body', () => { expect(r.body).toContain('already resolve to a critical posting floor'); expect(r.body).not.toContain('--severity-floor critical'); }); + + // The successor chain (#9905) through the path GitHub receives: the side + // file carries the previous round's minted closures, this round closes + // another same-file Critical and posts a fresh one — the note names the + // subsystem and the chain on the body, and the marker carries this + // round's closures forward. These run over COVERED plans: the mint now + // obeys the fail-closed predicate the anchor applies, so a round that + // cannot show it read the diff mints nothing — a bare plan here would + // hide every leg behind that one. + const coveredPrev = (prev: Record) => + coveredWithLedger({ v: 1, ...prev }); + + it('emits the divergence note on the #9659 rebound shape, and carries the closures forward', () => { + const r = composeReview({ + planPath: coveredPrev({ + round: 10, + findings: [ + { id: 'R10-2', sev: 'C', file: 'src/mechanism.ts', title: 'gen 2' }, + ], + closed: [{ r: 10, id: 'R9-1', f: 'src/mechanism.ts' }], + }), + env: ENV, + modelId: MODEL, + criticalsInline: 1, + suggestionsInline: 0, + draftedComments: [ + { path: 'src/mechanism.ts', line: 9, body: '**[Critical]** gen 3' }, + ], + }); + expect(r.body).toContain('⚠️ Divergence:'); + expect(r.body).toContain('`src/mechanism.ts`'); + expect(r.body).toContain('`R9-1 → R10-2 → R11-1`'); + expect(r.body).toContain("raising the pattern with the mechanism's owner"); + expect((r.recommendations ?? []).map((x) => x.code)).toContain( + 'successor-chain', + ); + // Advisory only: the verdict and its caps are untouched, and the + // marker carries both closure generations for the next round's check — + // this round's own mint only, no carry-forward of the older one. + expect(r.cappedBy.every((c) => !c.includes('divergence'))).toBe(true); + expect(parseLedger(r.body)?.closed).toEqual([ + { r: 11, id: 'R10-2', f: 'src/mechanism.ts' }, + ]); + }); + + it("discloses the fresh generation's identity gap on the divergence note", () => { + // The chain's new generation carries ids THIS round stamped — the + // fresh scan admits no carried id by construction — and a re-voice of + // a still-open claim whose readback lost its carried id is textually + // indistinguishable from a new Critical: the shape a blanket + // suppression cannot separate from the legitimate rebound. The note + // discloses the gap instead of asserting the generation is new. + const r = composeReview({ + planPath: coveredPrev({ + round: 10, + findings: [ + { id: 'R10-2', sev: 'C', file: 'src/mechanism.ts', title: 'gen 2' }, + ], + closed: [{ r: 10, id: 'R9-1', f: 'src/mechanism.ts' }], + }), + env: ENV, + modelId: MODEL, + criticalsInline: 1, + suggestionsInline: 0, + draftedComments: [ + { path: 'src/mechanism.ts', line: 9, body: '**[Critical]** gen 3' }, + ], + }); + expect(r.body).toContain('⚠️ Divergence:'); + expect(r.body).toContain( + "the chain's newest generation carries ids stamped this round", + ); + }); + + it('stays silent on the first rebound — one closure generation is normal', () => { + const r = composeReview({ + planPath: coveredPrev({ + round: 10, + findings: [ + { id: 'R10-2', sev: 'C', file: 'src/mechanism.ts', title: 'gen 2' }, + ], + }), + env: ENV, + modelId: MODEL, + criticalsInline: 1, + suggestionsInline: 0, + draftedComments: [ + { path: 'src/mechanism.ts', line: 9, body: '**[Critical]** gen 3' }, + ], + }); + expect(r.body).not.toContain('⚠️ Divergence:'); + // …but the closure IS recorded, so the next rebound can see it. + expect(parseLedger(r.body)?.closed).toEqual([ + { r: 11, id: 'R10-2', f: 'src/mechanism.ts' }, + ]); + }); + + it('mints no closures over a truncated previous list', () => { + const r = composeReview({ + planPath: coveredPrev({ + round: 10, + findings: [ + { id: 'R10-2', sev: 'C', file: 'src/mechanism.ts', title: 'gen 2' }, + ], + dropped: 2, + }), + env: ENV, + modelId: MODEL, + criticalsInline: 1, + suggestionsInline: 0, + draftedComments: [ + { path: 'src/mechanism.ts', line: 9, body: '**[Critical]** gen 3' }, + ], + }); + expect(r.body).not.toContain('⚠️ Divergence:'); + expect(parseLedger(r.body)?.closed).toBeUndefined(); + }); + + it('mints no closures on a cannot-tell round — absence is a DECLINED ruling', () => { + // The mint's honesty legs, leg one: a round that publicly answered + // "cannot tell" on a Critical declined to rule on it — the id is absent + // from the posting set by construction, but absence there is not + // "ruled fixed". The sibling `openCriticals` gate withholds the same + // inference under the same state, and the anchor's fail-closed + // predicate — which this round engages via its cap — subsumes the leg + // at the marker and the diagnosis. + const r = composeReview({ + planPath: coveredPrev({ + round: 10, + findings: [ + { id: 'R10-2', sev: 'C', file: 'src/mechanism.ts', title: 'gen 2' }, + ], + }), + env: ENV, + modelId: MODEL, + criticalsInline: 1, + suggestionsInline: 0, + draftedComments: [ + { path: 'src/mechanism.ts', line: 9, body: '**[Critical]** gen 3' }, + ], + cannotTellCriticals: [ + 'R10-2 — the claim could not be verified either way', + ], + }); + expect(r.cappedBy).toContain('cannot-tell-existing-critical'); + expect(parseLedger(r.body)?.closed).toBeUndefined(); + }); + + it('mints no closures on a context-unavailable round — nothing was re-read', () => { + // Leg two: a diff-only round could not re-read the context the previous + // work list was ruled under, so a vanished id is not a ruling there + // either — the same state the sibling gate cites `cannot-tell` beside. + const r = composeReview({ + planPath: coveredPrev({ + round: 10, + findings: [ + { id: 'R10-2', sev: 'C', file: 'src/mechanism.ts', title: 'gen 2' }, + ], + }), + env: ENV, + modelId: MODEL, + criticalsInline: 1, + suggestionsInline: 0, + contextUnavailable: true, + draftedComments: [ + { path: 'src/mechanism.ts', line: 9, body: '**[Critical]** gen 3' }, + ], + }); + expect(r.cappedBy).toContain('context-unavailable'); + expect(parseLedger(r.body)?.closed).toBeUndefined(); + }); + + it('mints no closures on a fail-closed round — absence may be unread territory', () => { + // The anchor's fail-closed predicate binds the mint too: a closure is + // the inference "ruled fixed", and a round that cannot show it READ the + // whole diff cannot support that inference — the vanished id may be + // sitting in the territory nobody re-read. cappedBy/scopeUnproven are + // only known after the body is composed, so the gate applies where they + // are known — at the diagnosis and at the marker — which is where this + // assertion meets it. The bare plan this describe writes proves no + // coverage, which is exactly the unproven-scope shape. + sideFile({ + round: 10, + findings: [ + { id: 'R10-2', sev: 'C', file: 'src/mechanism.ts', title: 'gen 2' }, + ], + closed: [{ r: 10, id: 'R9-1', f: 'src/mechanism.ts' }], + }); + const r = composeReview({ + planPath: plan(), + modelId: 'm', + criticalsInline: 1, + suggestionsInline: 0, + draftedComments: [ + { path: 'src/mechanism.ts', line: 9, body: '**[Critical]** gen 3' }, + ], + }); + expect(r.scopeUnproven).toBe(true); + expect(parseLedger(r.body)?.closed).toBeUndefined(); + // …and the chain's THIS-round generation is gated with it: closedPrev + // alone never fires the note. + expect(r.body).not.toContain('⚠️ Divergence:'); + }); + + it('mints no closures over a PURE-FOREIGN previous list', () => { + // Leg three (#9526): a list recovered from another account's marker, + // NOT merged over this account's own, is a stranger's — its unreposted + // Criticals are not rulings this account made, and minting closures + // over them seeds the sentinel with a lineage this loop never produced. + const r = composeReview({ + planPath: coveredPrev({ + round: 10, + foreign: true, + findings: [ + { id: 'R10-2', sev: 'C', file: 'src/mechanism.ts', title: 'gen 2' }, + ], + }), + env: ENV, + modelId: MODEL, + criticalsInline: 1, + suggestionsInline: 0, + draftedComments: [ + { path: 'src/mechanism.ts', line: 9, body: '**[Critical]** gen 3' }, + ], + }); + expect(parseLedger(r.body)?.closed).toBeUndefined(); + }); + + it('still mints closures over a MERGED foreign list — the union keeps own entries', () => { + // The leg's other edge: a MERGED list protects this account's own + // certified entries under their own ids — the round re-rules them entry + // by entry, so a vanished one WAS ruled. Over-tightening the leg to + // `foreign !== true` would disarm the mint on exactly the merged rounds + // the union exists to protect. + const r = composeReview({ + planPath: coveredPrev({ + round: 10, + foreign: true, + merged: true, + findings: [ + { id: 'R10-2', sev: 'C', file: 'src/mechanism.ts', title: 'gen 2' }, + ], + }), + env: ENV, + modelId: MODEL, + criticalsInline: 1, + suggestionsInline: 0, + draftedComments: [ + { path: 'src/mechanism.ts', line: 9, body: '**[Critical]** gen 3' }, + ], + }); + expect(parseLedger(r.body)?.closed).toEqual([ + { r: 11, id: 'R10-2', f: 'src/mechanism.ts' }, + ]); + }); + + it('mints no closures over an ANONYMOUSLY ADOPTED previous list', () => { + // The pure-foreign honesty leg reads `foreign` off the side file's + // stamp — but the anonymous whole-write persists a stranger's adopted + // list `foreign: false` (an UNKNOWN identity is not a foreign author, + // deliberate for the disclosure caveat). The mint is a second consumer + // of that stamp the rationale never addressed: recovered under a + // `getCurrentUser()` blip with no readable side file, a stranger's + // Criticals walk through the mint as own, and the positional diff mints + // closures over entries this round never engaged — where absence means + // "never ruled on", not "ruled fixed". The persist seam records the + // unverifiable adoption machine-readably, and this leg reads it like + // pure-foreign. + const adopted = composeReview({ + planPath: coveredPrev({ + round: 10, + anonymousAdoption: true, + findings: [ + { id: 'R10-2', sev: 'C', file: 'src/mechanism.ts', title: 'gen 2' }, + ], + }), + env: ENV, + modelId: MODEL, + criticalsInline: 1, + suggestionsInline: 0, + draftedComments: [ + { path: 'src/mechanism.ts', line: 9, body: '**[Critical]** gen 3' }, + ], + }); + expect(parseLedger(adopted.body)?.closed).toBeUndefined(); + // Control: the identical list WITHOUT the adoption stamp mints as + // before — the leg reads the recorded adoption, not a shape every + // pre-telemetry predecessor also has. + const own = composeReview({ + planPath: coveredPrev({ + round: 10, + findings: [ + { id: 'R10-2', sev: 'C', file: 'src/mechanism.ts', title: 'gen 2' }, + ], + }), + env: ENV, + modelId: MODEL, + criticalsInline: 1, + suggestionsInline: 0, + draftedComments: [ + { path: 'src/mechanism.ts', line: 9, body: '**[Critical]** gen 3' }, + ], + }); + expect(parseLedger(own.body)?.closed).toEqual([ + { r: 11, id: 'R10-2', f: 'src/mechanism.ts' }, + ]); + }); + + it('mints no closure for a standing claim re-minted under a fresh id', () => { + // Claim identity, not id identity. A re-post that loses its carried id + // in the readback — the gate's regenerated blockers render path-first + // with no id (#9526's renumbering walk), a model re-post can drop it — + // gets a FRESH id in the build: round 10 posted the claim as R10-1, + // round 11 re-voices it as R11-1, and R10-1 is absent from the posting + // set. Read absent-by-id alone, the claim mints a closure every round + // of its life, in the very body that re-posts it open. The mint joins + // on the locator projection instead, sees the claim still standing in + // the SAME build, and stays silent. + const r = composeReview({ + planPath: coveredWithLedger({ + v: 1, + round: 10, + findings: [ + { + id: 'R10-1', + sev: 'C', + file: 'src/f.ts', + title: 'the standing claim', + }, + ], + }), + env: ENV, + modelId: MODEL, + criticalsInline: 1, + suggestionsInline: 0, + draftedComments: [ + { + path: 'src/f.ts', + line: 3, + body: '**[Critical]** the standing claim', + }, + ], + }); + // The claim IS on the work list again — under this round's fresh id … + const marker = parseLedger(r.body)!; + expect( + marker.findings.some( + (f) => f.id === 'R11-1' && f.title === 'the standing claim', + ), + ).toBe(true); + // … so it is NOT closed. + expect(marker.closed).toBeUndefined(); + expect(r.body).not.toContain('⚠️ Divergence:'); + }); + + it('mints no closure for a standing claim whose locator outruns the title cap', () => { + // The cap-stage half of the claim-identity join: the PREVIOUS side's + // titles were sliced to LEDGER_MAX_TITLE at write time, so a claim + // whose locator exceeds the cap must be projected from the SAME + // capped form this round — projecting the uncapped build title misses + // the capped previous one, and the claim mints a closure in the very + // body that re-posts it. This is the gate blocker's shape — the script + // lint gate renders `path:line CODE — message [lint]`, and a deep + // path's locator prefix alone outruns the cap — reproduced here over + // a covered round, where the mint is not fail-closed and the join + // alone decides. + const longClaim = `claim whose locator outruns the cap ${'x'.repeat(60)}`; + expect(longClaim.length).toBeGreaterThan(LEDGER_MAX_TITLE); + const r = composeReview({ + planPath: coveredWithLedger({ + v: 1, + round: 10, + findings: [ + { id: 'R10-1', sev: 'C', file: 'src/f.ts', title: longClaim }, + ], + }), + env: ENV, + modelId: MODEL, + criticalsInline: 1, + suggestionsInline: 0, + draftedComments: [ + { path: 'src/f.ts', line: 3, body: `**[Critical]** ${longClaim}` }, + ], + }); + expect(r.cappedBy).toEqual([]); + // The claim IS on the work list again — its title carrying the same + // write-time cap the previous marker's did … + const marker = parseLedger(r.body)!; + expect( + marker.findings.some( + (f) => f.title === longClaim.slice(0, LEDGER_MAX_TITLE), + ), + ).toBe(true); + // … so it is NOT closed. + expect(marker.closed).toBeUndefined(); + expect(r.body).not.toContain('⚠️ Divergence:'); + }); + + it('mints no closure for a claim re-filed through the deferral channel', () => { + // Same hazard class as the gate blocker, other channel: the typed + // deferral channel carries no id field, and a re-file whose title + // carries no readable id leaves the round unable to PROVE which claim + // the entry re-posts — it may be the vanished sibling, it may be the + // standing one. A closure is the inference "ruled fixed", and doubt + // defeats it: the mint withholds every closure the round cannot + // certify, rather than text-matching the entry against the previous + // list shape by shape. The id-carrying siblings below are the shape + // that still mints beside a re-file. + const r = composeReview({ + planPath: coveredPrev({ + round: 10, + findings: [ + { id: 'R10-1', sev: 'C', file: 'src/auth.ts', title: 'auth bypass' }, + { id: 'R10-2', sev: 'C', file: 'src/auth.ts', title: 'token leak' }, + ], + }), + env: ENV, + modelId: MODEL, + criticalsInline: 0, + suggestionsInline: 0, + deferredSuggestions: [ + { + file: 'src/auth.ts', + line: 88, + source: 'test', + severity: 'Critical', + title: 'auth bypass', + }, + ], + }); + const marker = parseLedger(r.body)!; + // The re-filed claim rides the work list, fresh-stamped ... + expect(marker.findings.some((f) => f.sev === 'C')).toBe(true); + // ... and the re-file's title carries NO readable id, so the round + // cannot prove which claim the entry re-posts — the mint fails closed + // for the whole round, withholding even the truly vanished sibling. + expect(marker.closed).toBeUndefined(); + expect(r.body).not.toContain('⚠️ Divergence:'); + }); + + it('mints no closure for a long claim re-filed under its carried id through the deferral channel', () => { + // The typed-channel join's projection symmetry, same root as the reroute + // arm: a re-filed deferral title can CARRY the id, so a window taken + // BEFORE the id is stripped is short by the prefix — the previous + // list's id-less, write-capped locator never meets it, and the + // still-standing claim mints a closure in the body that re-posts it. + // The lead again puts the dash across the cap boundary, so the entry + // side must cap BEFORE locating, the serializer's order. + const lead = `the re-filed claim ${'x'.repeat(LEDGER_MAX_TITLE - 21)}`; + expect(lead).toHaveLength(LEDGER_MAX_TITLE - 2); + const r = composeReview({ + planPath: coveredPrev({ + round: 10, + findings: [ + { + id: 'R10-1', + sev: 'C', + file: 'src/auth.ts', + title: `${lead} — the original wording`, + }, + { id: 'R10-2', sev: 'C', file: 'src/auth.ts', title: 'token leak' }, + ], + }), + env: ENV, + modelId: MODEL, + criticalsInline: 0, + suggestionsInline: 0, + deferredSuggestions: [ + { + file: 'src/auth.ts', + line: 88, + source: 'test', + severity: 'Critical', + title: `R10-1: ${lead} — re-filed under the channel`, + }, + ], + }); + const marker = parseLedger(r.body)!; + // The re-filed claim rides the work list, fresh-stamped … + expect(marker.findings.some((f) => f.sev === 'C')).toBe(true); + // … so its still-standing original mints no closure — while a truly + // vanished same-file Critical beside it still does. + expect(marker.closed).toEqual([{ r: 11, id: 'R10-2', f: 'src/auth.ts' }]); + }); + + it('mints no closure for a Critical re-voiced as a floor-stripped Suggestion', () => { + // The floor reroute strips a drafted Suggestion that RE-VOICES a + // previous Critical back to the deferral channel: the claim leaves the + // posting set by construction, never reaches the build, and is absent + // from `input.deferredSuggestions` — every join the mint has on it is + // blind, and the absence mints a closure in the very body whose + // deferral list still carries the claim. The reroute output must fold + // into the standing-claim join like the typed channel does. + const r = composeReview({ + planPath: coveredPrev({ + round: 10, + findings: [ + { id: 'R10-1', sev: 'C', file: 'src/f.ts', title: 'claim X' }, + { id: 'R10-2', sev: 'C', file: 'src/f.ts', title: 'claim Y' }, + ], + closed: [{ r: 10, id: 'R9-1', f: 'src/f.ts' }], + }), + env: ENV, + modelId: MODEL, + severityFloor: 'critical', + criticalsInline: 1, + suggestionsInline: 0, + draftedComments: [ + { path: 'src/f.ts', line: 5, body: '**[Critical]** gen 3' }, + { + path: 'src/f.ts', + line: 5, + body: '**[Suggestion]** R10-1: claim X — now looks minor', + }, + ], + }); + // The reroute fired — the claim is gone from the posting set … + expect(r.floorEnforced).toEqual([1]); + expect(r.body).toContain('src/f.ts:5'); + // … and still it mints no closure, while the genuinely vanished + // same-file Critical beside it does. + expect(parseLedger(r.body)?.closed).toEqual([ + { r: 11, id: 'R10-2', f: 'src/f.ts' }, + ]); + // The sentinel's lineage is exactly the true one — the re-voiced claim + // is NOT in the chain's second generation. + expect(r.body).toContain('R9-1 → R10-2 → R11-1'); + expect(r.body).not.toContain('R10-1/R10-2'); + }); + + it('mints no closure for a long claim re-voiced as a floor-stripped Suggestion', () => { + // The reroute join's projection symmetry, at the boundary: the rerouted + // entry's title CARRIES the id (`R10-1: …`) by construction, so a window + // taken BEFORE the id is stripped is short by the prefix — the previous + // list's id-less, write-capped locator never meets it, and the + // still-standing claim mints a closure in the very body whose deferral + // line still carries it. The lead is chosen so the dash also straddles + // the cap boundary: the stored title's window ends mid-dash, so the + // entry side must cap BEFORE locating — the serializer's order — or the + // two windows disagree again at exactly this lead length. + const lead = `the re-voiced claim ${'x'.repeat(LEDGER_MAX_TITLE - 22)}`; + expect(lead).toHaveLength(LEDGER_MAX_TITLE - 2); + const r = composeReview({ + planPath: coveredPrev({ + round: 10, + findings: [ + { + id: 'R10-1', + sev: 'C', + file: 'src/f.ts', + title: `${lead} — the original wording`, + }, + ], + closed: [{ r: 10, id: 'R9-1', f: 'src/f.ts' }], + }), + env: ENV, + modelId: MODEL, + severityFloor: 'critical', + criticalsInline: 1, + suggestionsInline: 0, + draftedComments: [ + { path: 'src/f.ts', line: 5, body: '**[Critical]** gen 2' }, + { + path: 'src/f.ts', + line: 5, + body: `**[Suggestion]** R10-1: ${lead} — now looks minor`, + }, + ], + }); + // The reroute fired — the claim is gone from the posting set … + expect(r.floorEnforced).toEqual([1]); + // … and still it mints no closure … + expect(parseLedger(r.body)?.closed).toBeUndefined(); + // … and the sentinel carries no fabricated lineage for it: the false + // closure would have fired the chain over the round-10 generation. + expect(r.body).not.toContain('⚠️ Divergence:'); + }); + + // R4-1: the closure mint's claim-identity defense was an unbounded + // text-matching surface — one hand-rolled projection per re-posting + // channel, and the space of re-post shapes (moved paths, dash-less + // bodies, severity changes) cannot be enumerated and closed one entrance + // at a time. The class fix joins on EXPLICIT IDENTITY — the carried id a + // re-post channel's title may bear — and fails closed where the channel + // bears none: the round cannot prove what an id-less entry re-posts, so + // it certifies nothing. The witnesses below pin each demonstrated + // entrance, id-less (fail closed) and id-carrying (the exact join). + + it('fails closed for a moved-path re-file the deferral channel carries without an id', () => { + // Entrance one: the relocated join keyed on (file, claim), so a claim + // re-filed at a MOVED path slipped every conjunct and minted a closure + // over the very body re-posting it open. The id-less re-file proves + // nothing about which claim it carries — neither one closes. + const r = composeReview({ + planPath: coveredPrev({ + round: 10, + findings: [ + { id: 'R10-1', sev: 'C', file: 'src/auth.ts', title: 'auth bypass' }, + { id: 'R10-2', sev: 'C', file: 'src/auth.ts', title: 'token leak' }, + ], + closed: [{ r: 10, id: 'R9-1', f: 'src/auth.ts' }], + }), + env: ENV, + modelId: MODEL, + criticalsInline: 0, + suggestionsInline: 0, + deferredSuggestions: [ + { + file: 'src/moved/auth.ts', + line: 12, + source: 'test', + severity: 'Critical', + title: 'auth bypass', + }, + ], + }); + expect(parseLedger(r.body)?.closed).toBeUndefined(); + // The fabricated closure would have fired the chain one link later — + // the sentinel reads the marker this round writes. + expect(r.body).not.toContain('⚠️ Divergence:'); + }); + + it('mints the vanished closure beside a moved-path re-file that carries its id', () => { + // The same entrance with identity PROVEN: the re-filed entry bears the + // original id, the mint joins on the id set, and the moved path costs + // nothing — while the truly vanished sibling still closes. + const r = composeReview({ + planPath: coveredPrev({ + round: 10, + findings: [ + { id: 'R10-1', sev: 'C', file: 'src/auth.ts', title: 'auth bypass' }, + { id: 'R10-2', sev: 'C', file: 'src/auth.ts', title: 'token leak' }, + ], + }), + env: ENV, + modelId: MODEL, + criticalsInline: 0, + suggestionsInline: 0, + deferredSuggestions: [ + { + file: 'src/moved/auth.ts', + line: 12, + source: 'test', + severity: 'Critical', + title: 'R10-1: auth bypass', + }, + ], + }); + const marker = parseLedger(r.body)!; + expect(marker.findings.some((f) => f.sev === 'C')).toBe(true); + expect(marker.closed).toEqual([{ r: 11, id: 'R10-2', f: 'src/auth.ts' }]); + }); + + it('fails closed for a dash-less claim re-voiced as a floor-stripped Suggestion', () => { + // Entrance two: the reroute join projected the WHOLE marker-stripped + // body collapsed to one line while the previous side projected only + // the claim line, so a dash-less claim line never met. The re-voice + // bears no id and the round cannot tell it from any vanished claim — + // nothing closes, and no fabricated lineage fires in the same round. + const r = composeReview({ + planPath: coveredPrev({ + round: 10, + findings: [ + { + id: 'R10-1', + sev: 'C', + file: 'src/f.ts', + title: 'dashless claim text', + }, + { id: 'R10-2', sev: 'C', file: 'src/f.ts', title: 'claim Y' }, + ], + closed: [{ r: 10, id: 'R9-1', f: 'src/f.ts' }], + }), + env: ENV, + modelId: MODEL, + severityFloor: 'critical', + criticalsInline: 1, + suggestionsInline: 0, + draftedComments: [ + { path: 'src/f.ts', line: 5, body: '**[Critical]** gen 3' }, + { + path: 'src/f.ts', + line: 5, + body: + '**[Suggestion]** dashless claim text\n\n' + + 'Failure scenario: it still fails.\n' + + 'Suggested fix: do the other thing.', + }, + ], + }); + expect(r.floorEnforced).toEqual([1]); + expect(parseLedger(r.body)?.closed).toBeUndefined(); + expect(r.body).not.toContain('⚠️ Divergence:'); + }); + + it('mints the vanished closure beside a dash-less re-voice that carries its id', () => { + // The same entrance with identity proven: the rerouted body leads with + // the carried id, so the claim stands by the id set no matter how the + // rest of the body collapses — and the true lineage is the only one + // the sentinel names. + const r = composeReview({ + planPath: coveredPrev({ + round: 10, + findings: [ + { + id: 'R10-1', + sev: 'C', + file: 'src/f.ts', + title: 'dashless claim text', + }, + { id: 'R10-2', sev: 'C', file: 'src/f.ts', title: 'claim Y' }, + ], + closed: [{ r: 10, id: 'R9-1', f: 'src/f.ts' }], + }), + env: ENV, + modelId: MODEL, + severityFloor: 'critical', + criticalsInline: 1, + suggestionsInline: 0, + draftedComments: [ + { path: 'src/f.ts', line: 5, body: '**[Critical]** gen 3' }, + { + path: 'src/f.ts', + line: 5, + body: + '**[Suggestion]** R10-1: dashless claim text\n\n' + + 'Failure scenario: it still fails.\n' + + 'Suggested fix: do the other thing.', + }, + ], + }); + expect(r.floorEnforced).toEqual([1]); + expect(parseLedger(r.body)?.closed).toEqual([ + { r: 11, id: 'R10-2', f: 'src/f.ts' }, + ]); + expect(r.body).toContain('R9-1 → R10-2 → R11-1'); + expect(r.body).not.toContain('R10-1/R10-2'); + }); + + it('fails closed for a previous Critical re-voiced by a Suggestion-severity deferral entry', () => { + // Entrance three: the typed-deferral join filtered severity Critical + // before keying, so a Suggestion-severity entry re-voicing a previous + // Critical — the convergence-posture deferral flow — was invisible. + // Id-less, the round cannot tell which claim the entry carries, and + // the mint withholds everything. + const r = composeReview({ + planPath: coveredPrev({ + round: 10, + findings: [ + { id: 'R10-1', sev: 'C', file: 'src/f.ts', title: 'claim X' }, + { id: 'R10-2', sev: 'C', file: 'src/f.ts', title: 'claim Y' }, + ], + }), + env: ENV, + modelId: MODEL, + severityFloor: 'critical', + criticalsInline: 0, + suggestionsInline: 0, + deferredSuggestions: [ + { + file: 'src/f.ts', + line: 5, + source: 'review', + severity: 'Suggestion', + title: 'claim X', + }, + ], + }); + expect(parseLedger(r.body)?.closed).toBeUndefined(); + }); + + it('mints the vanished closure beside a Suggestion-severity re-voice that carries its id', () => { + // The same entrance with identity proven — severity is irrelevant to + // the id join: a re-voice stands under whichever severity re-voices + // it, and only the truly vanished sibling closes. + const r = composeReview({ + planPath: coveredPrev({ + round: 10, + findings: [ + { id: 'R10-1', sev: 'C', file: 'src/f.ts', title: 'claim X' }, + { id: 'R10-2', sev: 'C', file: 'src/f.ts', title: 'claim Y' }, + ], + }), + env: ENV, + modelId: MODEL, + severityFloor: 'critical', + criticalsInline: 0, + suggestionsInline: 0, + deferredSuggestions: [ + { + file: 'src/f.ts', + line: 5, + source: 'review', + severity: 'Suggestion', + title: 'R10-1: claim X', + }, + ], + }); + expect(parseLedger(r.body)?.closed).toEqual([ + { r: 11, id: 'R10-2', f: 'src/f.ts' }, + ]); + }); + + it('fails closed for a deferral re-file whose carried id is absent from the previous list', () => { + // The id join's membership leg: an entry bearing an id the recovered + // previous list never held — a renumbered or re-minted id — proves + // nothing about which claim it re-posts, and shields nothing: the + // still-standing claim it actually re-posts is absent from + // `postedIds`, from `repostedIds` (wrong id), and from + // `standingClaims`, so it mints a closure in the very round that + // re-posts it open. `buildLedger`'s `isCarry` applies exactly this + // membership test to the same class of model-written ids, calling a + // non-member a stray; the re-post join now agrees. The mint's gate + // already requires `carriedWorkList.complete`, so absence from the + // previous id space is provable whenever the mint runs. + const r = composeReview({ + planPath: coveredPrev({ + round: 10, + findings: [ + { + id: 'R10-1', + sev: 'C', + file: 'src/auth.ts', + title: 'auth bypass in the login flow', + }, + { id: 'R10-2', sev: 'C', file: 'src/auth.ts', title: 'token leak' }, + ], + }), + env: ENV, + modelId: MODEL, + criticalsInline: 0, + suggestionsInline: 0, + deferredSuggestions: [ + { + file: 'src/auth.ts', + line: 88, + source: 'test', + severity: 'Critical', + title: 'R11-1: auth bypass', + }, + ], + }); + expect(parseLedger(r.body)?.closed).toBeUndefined(); + expect(r.body).not.toContain('⚠️ Divergence:'); + }); + + it('mints the vanished closure beside a deferral re-file carrying a listed id', () => { + // The membership leg's control arm: an id the previous list DID hold + // still joins — the re-filed claim stands by it, and only the truly + // vanished sibling closes. + const r = composeReview({ + planPath: coveredPrev({ + round: 10, + findings: [ + { + id: 'R10-1', + sev: 'C', + file: 'src/auth.ts', + title: 'auth bypass in the login flow', + }, + { id: 'R10-2', sev: 'C', file: 'src/auth.ts', title: 'token leak' }, + ], + }), + env: ENV, + modelId: MODEL, + criticalsInline: 0, + suggestionsInline: 0, + deferredSuggestions: [ + { + file: 'src/auth.ts', + line: 88, + source: 'test', + severity: 'Critical', + title: 'R10-1: auth bypass in the login flow', + }, + ], + }); + expect(parseLedger(r.body)?.closed).toEqual([ + { r: 11, id: 'R10-2', f: 'src/auth.ts' }, + ]); + }); + + it("names no re-minted re-post as the chain's fresh generation", () => { + // The fresh side's mirror of the mint's claim-identity join: a re-post + // whose readback lost the carried id is stamped with a FRESH id in the + // build, and the chain's fresh scan keyed on the id alone counts the + // still-standing claim as a new Critical — firing the divergence note + // over a claim the same body's work list says never left. + const r = composeReview({ + planPath: coveredPrev({ + round: 10, + findings: [ + { id: 'R10-2', sev: 'C', file: 'src/f.ts', title: 'gen 2' }, + { + id: 'R10-3', + sev: 'C', + file: 'src/f.ts', + title: 'the standing claim', + }, + ], + closed: [{ r: 10, id: 'R9-1', f: 'src/f.ts' }], + }), + env: ENV, + modelId: MODEL, + criticalsInline: 1, + suggestionsInline: 0, + draftedComments: [ + { + path: 'src/f.ts', + line: 9, + body: '**[Critical]** the standing claim', + }, + ], + }); + // The marker's mint already defends the standing claim … + expect(parseLedger(r.body)?.closed).toEqual([ + { r: 11, id: 'R10-2', f: 'src/f.ts' }, + ]); + // … and the chain reads the same evidence: its "fresh" generation is a + // re-mint, so the note stays silent. + expect(r.body).not.toContain('⚠️ Divergence:'); + }); + + it('mints no closure for a same-id re-post whose wording drifted', () => { + // The id-exact conjunct the locator joins cannot replace: a re-post + // that CARRIES its id keeps it in the build, but a redrafted claim + // line projects to a DIFFERENT locator — the claim-identity joins + // miss it, and only the id check keeps the still-standing entry out + // of the closures. + const r = composeReview({ + planPath: coveredWithLedger({ + v: 1, + round: 10, + findings: [ + { id: 'R10-1', sev: 'C', file: 'src/f.ts', title: 'claim Y' }, + ], + }), + env: ENV, + modelId: MODEL, + criticalsInline: 1, + suggestionsInline: 0, + draftedComments: [ + { + path: 'src/f.ts', + line: 3, + body: '**[Critical]** R10-1: claim Y, restated with new wording', + }, + ], + }); + const marker = parseLedger(r.body)!; + expect(marker.findings.some((f) => f.id === 'R10-1')).toBe(true); + expect(marker.closed).toBeUndefined(); + }); + + it('mints a closure only for the Critical half of a mixed work list', () => { + // Suggestions are not tracked — Critical churn is the signal. Every + // sibling fixture's work list is Critical-only, which left the mint's + // `f.sev === 'C'` conjunct unwitnessed: a fixed Suggestion, or a + // `--severity-floor critical` round that moved one out of the posting + // set, would mint a closure, and two such rounds plus a fresh + // same-file Critical would fire the note over Suggestion churn. + const r = composeReview({ + planPath: coveredPrev({ + round: 10, + findings: [ + { id: 'R10-2', sev: 'C', file: 'src/mechanism.ts', title: 'gen 2' }, + { id: 'R10-3', sev: 'S', file: 'src/mechanism.ts', title: 'polish' }, + ], + }), + env: ENV, + modelId: MODEL, + criticalsInline: 1, + suggestionsInline: 0, + draftedComments: [ + { path: 'src/mechanism.ts', line: 9, body: '**[Critical]** gen 3' }, + ], + }); + expect(parseLedger(r.body)?.closed).toEqual([ + { r: 11, id: 'R10-2', f: 'src/mechanism.ts' }, + ]); + }); + + it('mints nothing when only the Suggestion vanishes, and no note fires', () => { + const r = composeReview({ + planPath: coveredPrev({ + round: 10, + findings: [ + { id: 'R10-2', sev: 'C', file: 'src/mechanism.ts', title: 'gen 2' }, + { id: 'R10-3', sev: 'S', file: 'src/mechanism.ts', title: 'polish' }, + ], + }), + env: ENV, + modelId: MODEL, + criticalsInline: 1, + suggestionsInline: 0, + draftedComments: [ + { + path: 'src/mechanism.ts', + line: 9, + body: '**[Critical]** R10-2: gen 2', + }, + ], + }); + expect(parseLedger(r.body)?.closed).toBeUndefined(); + expect(r.body).not.toContain('⚠️ Divergence:'); + }); + + it('caps a planted side-file closure list, keeping the NEWEST entries', () => { + // The side file is the same untrusted shape arriving by another route — + // and the route a planted `qwen-review-pr--prev-ledger.json` takes, + // bypassing the serializer's write-side cap. The read applies the count + // cap like its two siblings, or an unbounded valid `closed` array flows + // into the chain join and builds unbounded id arrays in the diagnosis. + // Sixty planted entries, cap fifty: the chain's first generation shows + // six ids and names the forty-four the cap shed — not the uncapped + // fifty-four. And WHICH fifty: the NEWEST — an oldest-kept cap renders + // the very same tail while shedding the end the chain exists to read. + const closed = Array.from({ length: LEDGER_MAX_CLOSED + 10 }, (_, i) => ({ + r: 10, + id: `R9-${i}`, + f: 'src/a.ts', + })); + const r = composeReview({ + planPath: coveredPrev({ + round: 10, + findings: [{ id: 'R10-1', sev: 'C', file: 'src/a.ts', title: 'x' }], + closed, + }), + env: ENV, + modelId: MODEL, + criticalsInline: 1, + suggestionsInline: 0, + draftedComments: [ + { path: 'src/a.ts', line: 9, body: '**[Critical]** again' }, + ], + }); + expect(r.body).toContain('⚠️ Divergence:'); + expect(r.body).toContain('… (+44)'); + expect(r.body).not.toContain('… (+54)'); + // The rendered ids themselves pin the keeping-the-NEWEST direction: + // `R9-10` is the first survivor of the cap, `R9-9` the first shed. + expect(r.body).toContain('R9-10'); + expect(r.body).not.toContain('R9-9'); + }); }); describe('the convergence census and the non-convergence finding', () => { diff --git a/packages/cli/src/commands/review/compose-review.ts b/packages/cli/src/commands/review/compose-review.ts index 0fe28e75a9a..fcf03a8eeb7 100644 --- a/packages/cli/src/commands/review/compose-review.ts +++ b/packages/cli/src/commands/review/compose-review.ts @@ -73,7 +73,11 @@ import type { TestPlanReport } from './test-plan.js'; import { LEDGER_BODY_FILE, LEDGER_ID_READBACK, + LEDGER_MAX_CLOSED, LEDGER_MAX_ID, + LEDGER_MAX_TITLE, + claimLocator, + isLedgerClosure, isLedgerFinding, isStandInName, normalizeLedgerFinding, @@ -84,6 +88,7 @@ import { streakOf, volumeOf, type Ledger, + type LedgerClosure, type LedgerFinding, } from './lib/ledger.js'; import { mdField } from './lib/md-field.js'; @@ -1521,12 +1526,14 @@ export function composeReview( const prevForConvergence = { ...(prevFacts.posted === undefined ? {} : { posted: prevFacts.posted }), findings: prevFacts.findings, + closed: prevFacts.closed, truncated: prevFacts.truncated, complete: prevRound > 0 && !prevFacts.truncated, round: prevRound, anchored: prevFacts.anchored, foreign: prevFacts.foreign, merged: prevFacts.merged, + anonymousAdoption: prevFacts.anonymousAdoption, ...(prevFacts.floor === undefined ? {} : { floor: prevFacts.floor }), ...(prevFacts.fresh === undefined ? {} : { fresh: prevFacts.fresh }), }; @@ -1658,6 +1665,118 @@ export function composeReview( })(), }; } + // The posted work list, built ONCE here rather than inside the marker: + // three consumers share one id space — the marker that stamps it, the + // closure mint below, and the successor-chain check inside the diagnosis, + // which reads this round's findings as the build stamped them. + const carriedWorkList = { + ids: new Set(prevFacts.findings.map((f) => f.id)), + // A round that recovered NO predecessor knows nothing about which ids + // were real — the id space is shared across environments and a first + // round on a machine with no side file is the ordinary case — so it + // cannot call a claimed id a stray. Only a recovered, untruncated list + // is evidence of absence. + complete: prevRound > 0 && !prevFacts.truncated, + }; + const postedLedger = buildPostedLedger( + effective, + Math.min(prevRound + 1, LEDGER_MAX_ROUND), + carriedWorkList, + ); + // The closures this round mints (#9905): the previous work list's + // Criticals this round does not re-post — `fixed` and `superseded` both + // read as closure, and a positional diff needs no more. Minted ONLY where + // absence from the posting set MEANS "ruled fixed" — the same honesty + // rule the anchor applies, one consumer down, with the SAME legs the + // sibling `openCriticals` gate applies to the identical inference: a + // PARTIAL previous list (a vanished id may be the byte budget, not a + // ruling) and a PURE-FOREIGN list whose entries are a stranger's, not a + // shortened version of this account's (#9526), and an ANONYMOUSLY ADOPTED + // list — the persist seam's machine-readable record that the file's + // findings were adopted with no identity to vouch them, which walks + // through this inference exactly like a pure-foreign one. The anchor's + // fail-closed + // predicate — `anchorFailsClosed(cappedBy, scopeUnproven, …)` — binds the + // closures too, at the diagnosis and the marker where `cappedBy` and + // `scopeUnproven` are known (this function returns before the body + // computes them): a closure is the inference "ruled fixed", and a round + // that could not show it READ the diff — or that publicly answered + // "cannot tell" on a Critical — cannot support it. In all of them the + // mint stays silent rather than guesses — thin history stays silent. + const postedIds = new Set(postedLedger?.findings.map((f) => f.id) ?? []); + // Claim identity, not id identity: a claim this round RE-POSTS without a + // carried id — a gate Critical regenerated from the report, a model + // re-post the readback lost — gets a FRESH id in the build (or no build + // entry at all), so the original is absent from `postedIds` while the + // claim still stands. Read absent-by-id alone, a still-standing blocker + // mints a closure every round of its life, in the very body that + // re-posts it open. So the join ALSO runs on the locator projection the + // gate-repost dedup uses — over the SAME build the marker stamps, so the + // record and the mint cannot disagree about what closed, with the SAME + // write-capped window on both sides: the previous list's titles were + // sliced to LEDGER_MAX_TITLE at write time (and again at read), so a + // build-side projection over the uncapped title never meets a previous + // locator that outruns the cap. + const standingClaims = new Set( + (postedLedger?.findings ?? []) + .map((g) => claimLocator(g.title.slice(0, LEDGER_MAX_TITLE))) + .filter((k) => k !== ''), + ); + // The re-post channels — the typed deferral channel and the reroute the + // floor enforcement feeds into it — carry claims OUTSIDE the build's id + // space, and their join is an ID join, never a text projection: four + // review rounds each patched a hand-rolled projection here, and each + // generation of fixes grew the next defect (R4-1: a moved-path re-file, + // a dash-less collapsed body, a Suggestion-severity re-voice — three + // entrances one probe round named, in a space of re-post shapes that + // cannot be enumerated and closed one entrance at a time). An entry + // whose title BEARS the original id proves which claim it re-posts — + // severity, path, and wording are all irrelevant to that readback; an + // entry that bears none proves nothing — it may carry ANY vanished + // claim — so the round fails closed and mints no closure at all: the + // same honesty leg the mint applies to a PARTIAL previous list, one + // element the round cannot account for suppressing the whole inference. + // The cost is a true closure withheld beside an id-less re-post; the + // opposite error is a fabricated lineage the sentinel fires one round + // later. Parse ONLY where the build above already parsed the same + // channel — a null build returned before any parse, so this adds no + // throw the round did not already have. + const repostEntries = [ + ...(postedLedger === null + ? [] + : toDeferredEntries(input.deferredSuggestions)), + ...reroute.entries, + ]; + const repostedIds = new Set(); + let repostUnidentified = false; + for (const e of repostEntries) { + const carried = LEDGER_ID_READBACK.exec(e.title)?.[1]; + // The same membership test `isCarry` applies in the build: an id the + // complete previous list never held is a stray — a renumbered or + // re-minted token, not a carry — and reading it as one would shield a + // claim the entry does not name while the claim it actually re-posts + // mints a closure. Over a partial list the mint is already silent, so + // the verdict this leg reaches there is inert. + if (carried === undefined || !carriedWorkList.ids.has(carried)) + repostUnidentified = true; + else repostedIds.add(carried); + } + const closuresThisRound: LedgerClosure[] = + carriedWorkList.complete && + postedLedger !== null && + !(prevFacts.foreign === true && prevFacts.merged !== true) && + prevFacts.anonymousAdoption !== true && + !repostUnidentified + ? prevFacts.findings + .filter( + (f) => + f.sev === 'C' && + !postedIds.has(f.id) && + !repostedIds.has(f.id) && + !standingClaims.has(claimLocator(f.title)), + ) + .map((f) => ({ r: postedLedger.round, id: f.id, f: f.file })) + : []; // Is the loop settling? Measured from facts this round already holds — the // previous work list and volume from the side file, this round's drafts — // and rendered as an observation. It changes nothing about what the round @@ -1678,6 +1797,14 @@ export function composeReview( reroute, { prev: prevForConvergence, + // The chain's two closure generations and its new side, all from the + // one build above — the marker stamps exactly these closures, so the + // note and the record cannot disagree. + closuresThisRound, + repostUnidentified, + ...(postedLedger === null + ? {} + : { thisRoundFindings: postedLedger.findings }), // Read from the same input `floorEnforcedReroute` just acted on, through // the one predicate both share — so the advice cannot recommend a floor // the enforcement above already applied, nor name it a way the @@ -1721,21 +1848,13 @@ export function composeReview( result.dimensionGapsAreDepthOnly ?? false, attribution, runtimeModelId, - prevRound, prevFacts.src0, postedInline, result.postedFresh, prevFacts.posted, floorKind, - { - ids: new Set(prevFacts.findings.map((f) => f.id)), - // A round that recovered NO predecessor knows nothing about which ids - // were real — the id space is shared across environments and a first - // round on a machine with no side file is the ordinary case — so it - // cannot call a claimed id a stray. Only a recovered, untruncated list - // is evidence of absence. - complete: prevRound > 0 && !prevFacts.truncated, - }, + postedLedger, + closuresThisRound, churnRounds, flatRounds, ); @@ -1914,9 +2033,11 @@ const EMPTY_PREV_FACTS = { churnRounds: 0, flatRounds: 0, findings: [] as LedgerFinding[], + closed: [] as LedgerClosure[], truncated: false, foreign: false, merged: false, + anonymousAdoption: false, anchored: false, }; @@ -1969,12 +2090,25 @@ function prevLedgerFacts( * as "the previous round found nothing". */ findings: LedgerFinding[]; + /** + * The Criticals the previous round closed — its marker's minted closures, + * validated through the ledger's own admission test like the findings. + * Feeds the successor-chain signal (#9905). Empty when nothing was + * recovered or the marker predates the field — silence, never a guess. + */ + closed: LedgerClosure[]; /** Its marker shed findings to fit the byte budget: the list is partial. */ truncated: boolean; /** It was recovered from a marker this account did not post. */ foreign: boolean; /** That marker was merged over this account's own findings. */ merged: boolean; + /** + * Its findings were adopted by an ANONYMOUS whole-write — recovery ran + * with no identity to vouch them, so the closure mint reads the list + * like a pure-foreign one. Absent on files a pre-telemetry writer made. + */ + anonymousAdoption: boolean; /** The posting floor it ran under, when its marker recorded one. */ floor?: 'c' | 'o'; /** How many of its comments were findings reported for the first time. */ @@ -2022,6 +2156,7 @@ function prevLedgerFacts( ) as Ledger & { foreign?: unknown; merged?: unknown; + anonymousAdoption?: unknown; anchorFromRound?: unknown; }; const round = @@ -2108,6 +2243,24 @@ function prevLedgerFacts( graftRefusedThisRound = inc.effective === false || inc.upToDate === true; } + // The previous round's minted closures, through the ledger's own + // admission test on the same route as the findings — the side file is + // the same untrusted shape arriving by another route, and a closure + // claiming a round past the file's own is a squat the parser refuses. + // The count cap binds here as on the two sibling routes (`parseLedger` + // and the serializer): the caps exist for the hand-edited or planted + // file, which is bound by no mint, and this route is the one a planted + // `qwen-review-pr--prev-ledger.json` arrives by. + // Travels with the round like the work list does: a file with no + // usable round is one this read cannot place, and its closures would + // seed the successor-chain check for a round this read calls 0. + const closed = + round === 0 || !Array.isArray(prev.closed) + ? [] + : prev.closed + .filter((c): c is LedgerClosure => isLedgerClosure(c, round)) + .slice(-LEDGER_MAX_CLOSED); + return { round, src0, @@ -2121,6 +2274,7 @@ function prevLedgerFacts( // for a round this read calls 0 — the posted body would cite rounds 5 // and up beside a marker stamping round 1. findings: round === 0 ? [] : findings, + closed, // The marker had to shed findings to fit its byte budget, so what came // back is known-incomplete (measured at up to 35 shed per round on the // worst PRs this diagnosis speaks to). Carried rather than dropped: the @@ -2137,6 +2291,9 @@ function prevLedgerFacts( // rendering says so rather than publishing the citation bare. foreign: round !== 0 && prev.foreign === true, merged: round !== 0 && prev.merged === true, + // Travels with the findings it qualifies and the round, for the same + // reason both of those do. + anonymousAdoption: round !== 0 && prev.anonymousAdoption === true, // The previous round's anchor, as a yes/no THIS round can use. Two // consecutive withholds are the shape the self-check discloses; the // sha itself is Step 1's business, not this read's. A CERTIFIED @@ -2204,6 +2361,73 @@ export function anchorFailsClosed( ); } +/** + * This round's work list as the marker will stamp it. Extracted from the + * marker builder so the closure mint and the successor-chain check read the + * SAME build — a second `buildLedger` call composed beside the first is the + * drift class the marker's own id space cannot survive (the two reads would + * disagree the moment either leg's inputs are edited). + * + * Null when the review names no PR — exactly the marker's own condition: a + * local review has no previous round to mint closures against, no marker to + * carry them, and no script-lint gate to read (its planPath is absent, and + * the gate demands one). + */ +function buildPostedLedger( + input: ComposeReviewInput, + round: number, + carriedWorkList: { ids: ReadonlySet; complete: boolean }, +): Ledger | null { + const planPath = input.planPath; + if (planPath === undefined || !planNamesPr(planPath)) return null; + return buildLedger( + // Capped by the caller, because the round is the id space and the parser + // refuses an id from past the cap: an uncapped stamp of prevRound + 1 met + // the serializer's round clamp at exactly LEDGER_MAX_ROUND and produced a + // marker whose own parser dropped every finding — invisibly, with the + // anchor still riding. The recovery path already refuses rounds above the + // cap, so prevRound can reach it only AT the cap, where staying there + // loses id uniqueness across those rounds and nothing else — against a + // counter no real PR approaches. + round, + (input.draftedComments ?? []) as Array<{ + path?: unknown; + line?: unknown; + body?: unknown; + }>, + [ + // The same rule the body applied, through the same statement of + // it: a re-post of a claim the gate regenerates below is dropped + // here too, or the work-list grows a second entry for one blocker + // every round. + ...withoutGateReposts( + ingestEntryList(input.bodyCriticals, 'bodyCriticals'), + scriptLintGate(planPath).criticals, + ), + // The same split the body performed: a relocated Critical is a + // posted, counted blocker and must enter the work list. + ...splitDeferralChannel(input.deferredSuggestions).relocated, + // The gate's Criticals, for the same reason: a gate Critical is a + // posted, counted blocker too — leaving it out let the next + // round's persistence half read "no prior Critical" over a round + // that posted one (#9526). + // + // A SECOND invocation, not the body composer's result — the two + // live in different functions and nothing passes the value across. + // What makes them agree is that `scriptLintGate` is pure in + // `planPath` and its inputs (the plan JSON, the report, the diff) + // are immutable for the length of one synchronous compose; it is + // NOT the single-origin discipline `postedInline` gets one line + // below. So the standing hazard is an edit, not a race: anything + // that filters, caps, or carves out what the BODY pushes must + // change this list too, or the posted body and the carried work + // list stop describing the same round (R4-1). + ...scriptLintGate(planPath).criticals, + ], + carriedWorkList, + ); +} + /** * The next round's marker, or null when this review has no PR to carry one. * Round number comes from the side file `pr-context` wrote from the PREVIOUS @@ -2216,19 +2440,36 @@ function ledgerMarkerFor( dimensionGapsAreDepthOnly: boolean, attribution: boolean, runtimeModelId: string | undefined, - prevRound: number, prevSrc0: number, postedInline: number, freshInline: number, prevPostedInline: number | undefined, floorKind: CriticalFloorKind | undefined, - carriedWorkList: { ids: ReadonlySet; complete: boolean }, + /** + * This round's work list AS BUILT by the caller — one id space shared + * with the closure mint and the successor-chain check, never a second + * build that could disagree with either. Null exactly when this review + * has no PR to carry a marker — the same condition this function's own + * `null` return names. + */ + postedLedger: Ledger | null, + /** + * The closures the caller minted over the recovered previous list. The + * marker carries them so the next round's sentinel reads one generation + * back (#9905). Advisory data, but NOT like the findings: a closure is + * the inference "ruled fixed", and a fail-closed round supports no such + * inference — a vanished id may sit in the territory nobody re-read — so + * the field rides only when the anchor's own predicate lets the anchor + * ride, and the serializer's cascade sheds it before the anchor. + */ + closed: LedgerClosure[], churnRounds: number, flatRounds: number, ): string | null { try { if (!input.planPath) return null; if (!planNamesPr(input.planPath)) return null; + if (postedLedger === null) return null; const plan = JSON.parse(readFileSync(input.planPath, 'utf8')) as { fetchedSha?: unknown; srcDiffLines?: unknown; @@ -2338,52 +2579,11 @@ function ledgerMarkerFor( ? certifying : undefined; return serializeLedger({ - ...buildLedger( - // Capped, because the round is the id space and the parser refuses an - // id from past the cap: an uncapped stamp of prevRound + 1 met the - // serializer's round clamp at exactly LEDGER_MAX_ROUND and produced a - // marker whose own parser dropped every finding — invisibly, with the - // anchor still riding. The recovery path already refuses rounds above - // the cap, so prevRound can reach it only AT the cap, where staying - // there loses id uniqueness across those rounds and nothing else — - // against a counter no real PR approaches. - Math.min(prevRound + 1, LEDGER_MAX_ROUND), - (input.draftedComments ?? []) as Array<{ - path?: unknown; - line?: unknown; - body?: unknown; - }>, - [ - // The same rule the body applied, through the same statement of - // it: a re-post of a claim the gate regenerates below is dropped - // here too, or the work-list grows a second entry for one blocker - // every round. - ...withoutGateReposts( - ingestEntryList(input.bodyCriticals, 'bodyCriticals'), - scriptLintGate(input.planPath).criticals, - ), - // The same split the body performed: a relocated Critical is a - // posted, counted blocker and must enter the work list. - ...splitDeferralChannel(input.deferredSuggestions).relocated, - // The gate's Criticals, for the same reason: a gate Critical is a - // posted, counted blocker too — leaving it out let the next - // round's persistence half read "no prior Critical" over a round - // that posted one (#9526). - // - // A SECOND invocation, not the body composer's result — the two - // live in different functions and nothing passes the value across. - // What makes them agree is that `scriptLintGate` is pure in - // `planPath` and its inputs (the plan JSON, the report, the diff) - // are immutable for the length of one synchronous compose; it is - // NOT the single-origin discipline `postedInline` gets one line - // below. So the standing hazard is an edit, not a race: anything - // that filters, caps, or carves out what the BODY pushes must - // change this list too, or the posted body and the carried work - // list stop describing the same round (R4-1). - ...scriptLintGate(input.planPath).criticals, - ], - carriedWorkList, - ), + ...postedLedger, + // Gated by the SAME predicate the diagnosis applies (composeReviewBody): + // one shared decision, evaluated once per consumer, over the one set + // of cap inputs the caller passes both. + ...(!failClosed && closed.length > 0 ? { closed } : {}), // The pair falls together: a sha with no model reads to the next // round as a pre-field marker rather than as "nobody certified this". ...(shaCandidate && !identityDrifted ? { sha: shaCandidate } : {}), @@ -2565,6 +2765,22 @@ function composeReviewBody( prev: PrevRound; floor?: 'c' | 'o'; criticalFloorKind?: CriticalFloorKind; + /** + * The closures this round mints and this round's built work list — the + * successor-chain check's two inputs (#9905), both computed by the + * caller from the one `buildPostedLedger` build the marker stamps. + * Absent in direct-call tests: the signal reads `[]` and stays silent. + */ + closuresThisRound?: readonly LedgerClosure[]; + thisRoundFindings?: readonly LedgerFinding[]; + /** + * Whether a re-post channel carried an element the recovered work list + * cannot place — an entry bearing no id that list held. The caller's + * closure mint fails closed over that state, and the `land-and-defer` + * gate below rests on the identical absence inference, so it withholds + * on it too — the same honesty leg, one consumer over. + */ + repostUnidentified?: boolean; /** * Whether the CODE backstop enforces the floor this round reports. The * two readings differ by one thing — the reporting one folds an absent @@ -3464,6 +3680,18 @@ function composeReviewBody( posted: postedInline, prev: convergence.prev, drafts: draftedFindingsOf(input.draftedComments), + // The fail-closed leg the mint's comment names: cappedBy and + // scopeUnproven are only known here, after the body's caps were + // computed, so this is where the gate applies for the note — the + // marker applies the same predicate for the record (ledgerMarkerFor), + // and the two cannot disagree about what closed. + ...(convergence.closuresThisRound === undefined || + anchorFailsClosed(cappedBy, scopeUnproven, dimensionGapsAreDepthOnly) + ? {} + : { closuresThisRound: convergence.closuresThisRound }), + ...(convergence.thisRoundFindings === undefined + ? {} + : { thisRoundFindings: convergence.thisRoundFindings }), ...(convergence.floor === undefined ? {} : { floor: convergence.floor }), @@ -3499,6 +3727,14 @@ function composeReviewBody( // - a PURE-FOREIGN list (foreign, not merged over this account's // own): this account's entries are in no work list at all, so its // own open Criticals cannot be re-posted. + // - an ANONYMOUSLY ADOPTED list: the persist seam's record that the + // findings were adopted with no identity to vouch them. The same + // leg the closure mint reads — the list walks like a pure-foreign + // one through every absence inference. + // - an UNACCOUNTED re-post: a deferral or reroute entry this round + // posted bears no id the recovered work list held, so a vanished + // Critical may be the claim it re-voices. The caller's mint fails + // closed over the same state. // // Passed anyway, the body carries "no Critical is open" beside its // own disclosure of what it could not read, and the artifact tells a @@ -3509,7 +3745,11 @@ function composeReviewBody( dimensionGapsAreDepthOnly, ) && convergence.prev.complete === true && - !(convergence.prev.foreign === true && convergence.prev.merged !== true) + !( + convergence.prev.foreign === true && convergence.prev.merged !== true + ) && + convergence.prev.anonymousAdoption !== true && + convergence.repostUnidentified !== true ? { openCriticals } : {}), }) @@ -5476,18 +5716,9 @@ export function withoutGateReposts( ownBodyCriticals: readonly string[], gateCriticals: readonly string[], ): string[] { - const locator = (entry: string): string => { - const claim = entry.trim().replace(LEDGER_ID_READBACK, '').trim(); - const dash = claim.indexOf(' — '); - // Backticks stripped: the gate renders the path through `mdField`, and a - // re-post that types the locator plainly names the same finding. - return (dash === -1 ? claim : claim.slice(0, dash)) - .replace(/`/g, '') - .trim(); - }; - const regenerated = new Set(gateCriticals.map(locator).filter((k) => k)); + const regenerated = new Set(gateCriticals.map(claimLocator).filter((k) => k)); if (regenerated.size === 0) return [...ownBodyCriticals]; - return ownBodyCriticals.filter((c) => !regenerated.has(locator(c))); + return ownBodyCriticals.filter((c) => !regenerated.has(claimLocator(c))); } export function scriptLintGate(planPath: string): { diff --git a/packages/cli/src/commands/review/lib/convergence.test.ts b/packages/cli/src/commands/review/lib/convergence.test.ts index 28d167df445..8bb472c4586 100644 --- a/packages/cli/src/commands/review/lib/convergence.test.ts +++ b/packages/cli/src/commands/review/lib/convergence.test.ts @@ -19,7 +19,12 @@ import { type DraftedFinding, type ConvergenceFacts, } from './convergence.js'; -import { LEDGER_MAX_ROUND, type LedgerFinding } from './ledger.js'; +import { + LEDGER_MAX_FILE, + LEDGER_MAX_ROUND, + LEDGER_MAX_TITLE, + type LedgerFinding, +} from './ledger.js'; const f = (id: string, file: string): LedgerFinding => ({ id, @@ -654,6 +659,446 @@ describe('diagnoseConvergence — the trigger table', () => { }); }); +// The successor chain (#9905): the sharper form of recurrence — not "the +// file sees findings again" but "each of the last two rounds CLOSED a +// Critical here and this round posts another". The truth table pins both +// the #9659 rebound shape that must fire and every near-miss that must +// stay silent. +describe('diagnoseConvergence — the successor chain (#9905)', () => { + /** A Critical the ledger carries. */ + const c = (id: string, file: string): LedgerFinding => ({ + id, + sev: 'C', + file, + title: 't', + }); + const closedAt = (r: number, id: string, file: string) => ({ + r, + id, + f: file, + }); + + it('fires on the #9659 rebound shape — closed in each of the last two rounds, fresh Critical now', () => { + // Round 11 of the measured chain: R9-1 closed at r=10, R10-2 closes + // now (r=11), and the round posts a fresh R11-4 on the same file. + const r = diagnoseConvergence({ + round: 11, + posted: 1, + prev: { + findings: [c('R10-2', 'src/mechanism.ts')], + closed: [closedAt(10, 'R9-1', 'src/mechanism.ts')], + }, + drafts: [], + closuresThisRound: [closedAt(11, 'R10-2', 'src/mechanism.ts')], + thisRoundFindings: [ + { ...c('R11-4', 'src/mechanism.ts'), title: 'gen 3' }, + ], + })!; + expect(r.successorChains).toEqual([ + { + file: 'src/mechanism.ts', + generations: [['R9-1'], ['R10-2']], + newIds: ['R11-4'], + }, + ]); + expect(recommendationsFor(r).map((x) => x.code)).toContain( + 'successor-chain', + ); + expect( + recommendationsFor(r).find((x) => x.code === 'successor-chain')?.basis, + ).toContain('R9-1 → R10-2 → R11-4'); + }); + + it('renders the note naming the subsystem, the chain, and the advice', () => { + const r = diagnoseConvergence({ + round: 11, + posted: 1, + prev: { + findings: [c('R10-2', 'src/mechanism.ts')], + closed: [closedAt(10, 'R9-1', 'src/mechanism.ts')], + }, + drafts: [], + closuresThisRound: [closedAt(11, 'R10-2', 'src/mechanism.ts')], + thisRoundFindings: [ + { ...c('R11-4', 'src/mechanism.ts'), title: 'gen 3' }, + ], + })!; + const text = renderConvergenceDiagnosis(r); + expect(text.en).toContain('⚠️ Divergence:'); + expect(text.en).toContain('`src/mechanism.ts`'); + expect(text.en).toContain('`R9-1 → R10-2 → R11-4`'); + expect(text.en).toContain("raising the pattern with the mechanism's owner"); + expect(text.zh).toContain('⚠️ 发散：'); + expect(text.zh).toContain('提给该机制的负责人'); + // The sentences state what was MEASURED — one closure generation in the + // PREVIOUS round and one THIS round (r === round - 1 and r === round), + // never "exactly one per round" and never the two rounds before this + // one. A generation can close several Criticals (the chain beside the + // sentence renders them), and dating the closures to rounds 9-10 beside + // a chain whose R10-2 was BORN in round 10 refutes itself. + expect(text.en).toContain( + 'closed Critical(s) in the previous round and again this round', + ); + expect(text.en).not.toContain('in each of the last two rounds'); + expect(text.zh).toContain('在上一轮和本轮各关闭了至少一个 Critical'); + expect(text.zh).not.toContain('过去两轮'); + const basis = recommendationsFor(r).find( + (x) => x.code === 'successor-chain', + )?.basis; + expect(basis).toContain( + 'closed Critical(s) in the previous round and again this round', + ); + expect(basis).not.toContain('in each of the last two rounds'); + }); + + it('stays silent with only ONE round of closures on the file', () => { + // The first successor generation: R1-1 closes now, R2-1 lands — + // locally normal, and the sentinel says nothing until it repeats. + expect( + diagnoseConvergence({ + round: 2, + posted: 1, + prev: { findings: [c('R1-1', 'src/a.ts')] }, + drafts: [], + closuresThisRound: [closedAt(2, 'R1-1', 'src/a.ts')], + thisRoundFindings: [{ ...c('R2-1', 'src/a.ts'), title: 'gen 2' }], + }), + ).toBeNull(); + }); + + it('stays silent when the closures are on a DIFFERENT file', () => { + expect( + diagnoseConvergence({ + round: 3, + posted: 1, + prev: { + findings: [c('R2-1', 'src/a.ts')], + closed: [closedAt(2, 'R1-1', 'src/a.ts')], + }, + drafts: [], + closuresThisRound: [closedAt(3, 'R2-1', 'src/a.ts')], + thisRoundFindings: [{ ...c('R3-1', 'src/b.ts'), title: 'new claim' }], + }), + ).toBeNull(); + }); + + it('stays silent when the previous generation is offered a STALE closure', () => { + // Side-file admission accepts any closure with `1 ≤ r ≤ markerRound`, + // so a hand-edited or planted file — the route `compose-review.ts` + // itself names — can carry one from an EARLIER round. It must not + // stand in for the missing previous generation, or the note fires on + // one real closure plus a stale one. + expect( + diagnoseConvergence({ + round: 3, + posted: 1, + prev: { + findings: [c('R2-1', 'src/a.ts')], + closed: [closedAt(1, 'R1-1', 'src/a.ts')], + }, + drafts: [], + closuresThisRound: [closedAt(3, 'R2-1', 'src/a.ts')], + thisRoundFindings: [{ ...c('R3-1', 'src/a.ts'), title: 'gen 2' }], + }), + ).toBeNull(); + }); + + it('stays silent when the current generation is offered a STALE closure', () => { + // The mirror: a previous-round `r` inside THIS round's minted set does + // not supply the current generation either. + expect( + diagnoseConvergence({ + round: 3, + posted: 1, + prev: { + findings: [c('R2-1', 'src/a.ts')], + closed: [closedAt(2, 'R1-1', 'src/a.ts')], + }, + drafts: [], + closuresThisRound: [closedAt(2, 'R2-1', 'src/a.ts')], + thisRoundFindings: [{ ...c('R3-1', 'src/a.ts'), title: 'gen 2' }], + }), + ).toBeNull(); + }); + + it('stays silent when the same-file finding is CARRIED, not new', () => { + // A still-standing Critical keeps its original id; the chain needs a + // FRESH one — otherwise the note would fire every round over a file + // whose Critical nobody ever fixes. + expect( + diagnoseConvergence({ + round: 3, + posted: 1, + prev: { + findings: [c('R2-1', 'src/a.ts')], + closed: [closedAt(2, 'R1-1', 'src/a.ts')], + }, + drafts: [], + closuresThisRound: [closedAt(3, 'R2-1', 'src/a.ts')], + thisRoundFindings: [{ ...c('R1-9', 'src/a.ts'), title: 'new claim' }], + }), + ).toBeNull(); + }); + + it('stays silent on the stand-ins — they name no subsystem', () => { + for (const pseudo of ['(body)', '(unknown)']) { + expect( + diagnoseConvergence({ + round: 3, + posted: 1, + prev: { + findings: [c('R2-1', pseudo)], + closed: [closedAt(2, 'R1-1', pseudo)], + }, + drafts: [], + closuresThisRound: [closedAt(3, 'R2-1', pseudo)], + thisRoundFindings: [c('R3-1', pseudo)], + }), + ).toBeNull(); + } + }); + + it('stays silent on a REAL file spelled like a stand-in — its closures are undisambiguatable', () => { + // The fresh side respects `k` — a real path spelled like a stand-in + // still counts. But `LedgerClosure` carries no `k`: a closure stamped + // `(body)` is a body-only closure OR a closure on that real file, and + // nothing parts the two. Joining them to the real file's chain would + // post a lineage over a mechanism the closures never anchored on, so + // stand-in-named closures never join — silence, never a guess. + const real: LedgerFinding = { ...c('R3-1', '(body)'), k: 1 }; + expect( + diagnoseConvergence({ + round: 3, + posted: 1, + prev: { + findings: [c('R2-1', '(body)')], + closed: [closedAt(2, 'R1-1', '(body)')], + }, + drafts: [], + closuresThisRound: [closedAt(3, 'R2-1', '(body)')], + thisRoundFindings: [{ ...real, title: 'gen 3' }], + }), + ).toBeNull(); + }); + + it('reaches across the file cap — closures are capped, the fresh side is not', () => { + // Every closure route caps `f` at LEDGER_MAX_FILE while the fresh side + // keys on the RAW built path, so an exact join is permanently silent on + // a path past the cap — disarming the advisory for exactly the deep + // subsystems it exists for, while the recurrence cluster one function + // up still fires via its prefix fallback. The join mirrors `priorFor`. + const longPath = `src/${'deep/'.repeat(60)}mechanism.ts`; + expect(longPath.length).toBeGreaterThan(LEDGER_MAX_FILE); + const capped = longPath.slice(0, LEDGER_MAX_FILE); + const r = diagnoseConvergence({ + round: 3, + posted: 1, + prev: { + findings: [{ ...c('R2-1', longPath), file: capped }], + closed: [closedAt(2, 'R1-1', capped)], + }, + drafts: [], + closuresThisRound: [closedAt(3, 'R2-1', capped)], + thisRoundFindings: [{ ...c('R3-1', longPath), title: 'gen 3' }], + })!; + expect(r.successorChains).toEqual([ + { + file: longPath, + generations: [['R1-1'], ['R2-1']], + newIds: ['R3-1'], + }, + ]); + }); + + it('stays silent when the fresh id is a re-mint of a claim still standing', () => { + // The fresh side's mirror of the mint's claim-identity join: a re-post + // whose readback lost the carried id gets a FRESH `R-` stamp + // in the build, so id alone counts a still-standing claim as the chain's + // new generation — narrating divergence over a claim that never left + // the work list, in a body whose own marker says it still stands. + expect( + diagnoseConvergence({ + round: 3, + posted: 1, + prev: { + findings: [ + c('R2-1', 'src/a.ts'), + { ...c('R1-9', 'src/a.ts'), title: 'the standing claim' }, + ], + closed: [closedAt(2, 'R1-1', 'src/a.ts')], + }, + drafts: [], + closuresThisRound: [closedAt(3, 'R2-1', 'src/a.ts')], + thisRoundFindings: [ + { ...c('R3-1', 'src/a.ts'), title: 'the standing claim' }, + ], + }), + ).toBeNull(); + + // The same defense at the cap stage: the previous side's titles were + // sliced to LEDGER_MAX_TITLE at write time, so the fresh side must + // project from the SAME capped form — the uncapped projection of a + // long claim never meets the capped previous one, and the re-mint + // counts as a new generation. + const longClaim = `the standing claim ${'x'.repeat(70)}`; + expect(longClaim.length).toBeGreaterThan(LEDGER_MAX_TITLE); + expect( + diagnoseConvergence({ + round: 3, + posted: 1, + prev: { + findings: [ + c('R2-1', 'src/a.ts'), + { + ...c('R1-9', 'src/a.ts'), + title: longClaim.slice(0, LEDGER_MAX_TITLE), + }, + ], + closed: [closedAt(2, 'R1-1', 'src/a.ts')], + }, + drafts: [], + closuresThisRound: [closedAt(3, 'R2-1', 'src/a.ts')], + thisRoundFindings: [{ ...c('R3-1', 'src/a.ts'), title: longClaim }], + }), + ).toBeNull(); + }); + + it('stays silent when the fresh finding is a Suggestion', () => { + expect( + diagnoseConvergence({ + round: 3, + posted: 1, + prev: { + findings: [c('R2-1', 'src/a.ts')], + closed: [closedAt(2, 'R1-1', 'src/a.ts')], + }, + drafts: [], + closuresThisRound: [closedAt(3, 'R2-1', 'src/a.ts')], + thisRoundFindings: [{ ...f('R3-1', 'src/a.ts'), title: 'new claim' }], + }), + ).toBeNull(); + }); + + it('stays silent on a truncated previous list — no closures were minted', () => { + // The mint rule's downstream face: with a partial work list the caller + // mints nothing, and the carried history alone can never complete a + // chain (the check needs BOTH generations). + expect( + diagnoseConvergence({ + round: 3, + posted: 1, + prev: { + findings: [], + closed: [closedAt(2, 'R1-1', 'src/a.ts')], + truncated: true, + }, + drafts: [], + closuresThisRound: [], + thisRoundFindings: [c('R3-1', 'src/a.ts')], + }), + ).toBeNull(); + }); + + it('names every fresh Critical of the diverging file in one chain', () => { + const r = diagnoseConvergence({ + round: 3, + posted: 2, + prev: { + findings: [c('R2-1', 'src/a.ts')], + closed: [closedAt(2, 'R1-1', 'src/a.ts')], + }, + drafts: [], + closuresThisRound: [closedAt(3, 'R2-1', 'src/a.ts')], + thisRoundFindings: [ + { ...c('R3-1', 'src/a.ts'), title: 'claim one' }, + { ...c('R3-2', 'src/a.ts'), title: 'claim two' }, + ], + })!; + expect(r.successorChains).toHaveLength(1); + expect(r.successorChains[0]!.newIds).toEqual(['R3-1', 'R3-2']); + expect( + recommendationsFor(r).find((x) => x.code === 'successor-chain')?.basis, + ).toContain('R1-1 → R2-1 → R3-1/R3-2'); + }); + + it('caps each rendered generation at six ids, naming the overflow', () => { + // `MAX_CHAIN_IDS_PER_GENERATION` binds the paragraph and the basis + // through the one renderer; no fixture before this one built a + // generation wider than two ids, so deleting the slice and its `… (+N)` + // suffix shipped green. + const r = diagnoseConvergence({ + round: 3, + posted: 1, + prev: { + findings: [], + closed: Array.from({ length: 7 }, (_, i) => + closedAt(2, `R1-${i + 1}`, 'src/a.ts'), + ), + }, + drafts: [], + closuresThisRound: [closedAt(3, 'R2-1', 'src/a.ts')], + thisRoundFindings: [{ ...c('R3-1', 'src/a.ts'), title: 'new claim' }], + })!; + expect( + recommendationsFor(r).find((x) => x.code === 'successor-chain')?.basis, + ).toContain('R1-1/R1-2/R1-3/R1-4/R1-5/R1-6 … (+1) → R2-1 → R3-1'); + }); + + it('orders chains by measured work, never by ledger-insertion order', () => { + // Four diverging files; the LAST-inserted one carries the most fresh + // Criticals. Both consumers slice the list at MAX_RENDERED_CLUSTERS, so + // an unsorted list lets the map's insertion order — the order the files + // first appeared in the build — choose which subsystems the note names. + const files = ['src/a.ts', 'src/b.ts', 'src/c.ts', 'src/d.ts']; + const r = diagnoseConvergence({ + round: 3, + posted: 6, + prev: { + findings: [], + closed: files.map((f, i) => closedAt(2, `R1-${i + 1}`, f)), + }, + drafts: [], + closuresThisRound: files.map((f, i) => closedAt(3, `R2-${i + 1}`, f)), + thisRoundFindings: [ + c('R3-1', 'src/a.ts'), + c('R3-2', 'src/b.ts'), + c('R3-3', 'src/c.ts'), + c('R3-4', 'src/d.ts'), + c('R3-5', 'src/d.ts'), + c('R3-6', 'src/d.ts'), + ], + })!; + expect(r.successorChains.map((x) => x.file)).toEqual([ + 'src/d.ts', + 'src/a.ts', + 'src/b.ts', + 'src/c.ts', + ]); + // The render and the basis read the same sorted list: the + // multi-Critical subsystem is named first, one single-Critical file + // lands in the tail. + const text = renderConvergenceDiagnosis(r); + expect(text.en).toContain('`src/a.ts`'); + expect(text.en.indexOf('`src/d.ts`')).toBeLessThan( + text.en.indexOf('`src/a.ts`'), + ); + expect(text.en).toContain('and 1 more'); + expect(text.en).not.toContain('`src/c.ts`'); + expect(text.zh).toContain('`src/d.ts`'); + expect(text.zh).toContain('；另有 1 个'); + expect(text.zh).not.toContain('`src/c.ts`'); + const basis = recommendationsFor(r).find( + (x) => x.code === 'successor-chain', + )?.basis; + expect(basis).toContain('src/d.ts'); + // Both consumers slice at MAX_RENDERED_CLUSTERS: with four chains the + // basis names the rendered three and elides the rest — the suffix is + // the only witness that the recommendations half applies the cap. + expect(basis).toContain(', …'); + expect(basis).not.toContain('src/c.ts'); + }); +}); + describe('recommendationsFor — measurement to advice, no constants', () => { const base: ConvergenceDiagnosis = { round: 6, @@ -662,6 +1107,7 @@ describe('recommendationsFor — measurement to advice, no constants', () => { prevPosted: 4, prevFresh: 2, clusters: [{ file: 'src/a.ts', priorRounds: [3, 5], thisRound: 2 }], + successorChains: [], volumeNotShrinking: true, truncatedEvidence: false, foreignEvidence: false, @@ -850,6 +1296,7 @@ describe('renderConvergenceDiagnosis — what the author reads', () => { prevPosted: 4, prevFresh: 2, clusters: [{ file: 'src/a.ts', priorRounds: [3, 5], thisRound: 2 }], + successorChains: [], volumeNotShrinking: true, truncatedEvidence: false, foreignEvidence: false, @@ -903,6 +1350,24 @@ describe('renderConvergenceDiagnosis — what the author reads', () => { expect(r.zh).toContain('根因'); expect(r.zh).toContain('拆成单独的 PR'); expect(r.zh).not.toMatch(/重构|重写|重新设计/); + // The chain advice renders into the SAME paragraph and binds by the + // SAME contract — a chainless fixture left this branch blind to it. + const chained = renderConvergenceDiagnosis({ + ...base, + successorChains: [ + { + file: 'src/mechanism.ts', + generations: [['R5-1'], ['R6-1']], + newIds: ['R6-2'], + }, + ], + }); + expect(chained.en).toContain('⚠️ Divergence:'); + expect(chained.en).not.toMatch( + /refactor|rewrite|extract .* class|redesign/i, + ); + expect(chained.zh).toContain('⚠️ 发散：'); + expect(chained.zh).not.toMatch(/重构|重写|重新设计/); }); it('falls back to the volume reading when nothing recurs', () => { @@ -1003,6 +1468,7 @@ describe('renderConvergenceDiagnosis — what the author reads', () => { posted: 3, fresh: 3, clusters: [], + successorChains: [], volumeNotShrinking: false, truncatedEvidence: true, foreignEvidence: true, @@ -1016,6 +1482,7 @@ describe('renderConvergenceDiagnosis — what the author reads', () => { posted: 3, fresh: 3, clusters: [], + successorChains: [], volumeNotShrinking: false, truncatedEvidence: false, foreignEvidence: true, @@ -1116,6 +1583,45 @@ describe('renderConvergenceDiagnosis — what the author reads', () => { expect(r.zh).toContain('上述轮次与其计数'); }); + it('qualifies the closure lineage a foreign marker supplies to the chain', () => { + // The chain's previous generation is read off `prev.closed` — entries + // recovered from the SAME marker the rounds and counts caveat already + // qualifies. A round-2 shape fires the chain while the cluster signal + // is still gated to round 3 and no previous volume was recorded, so + // without a closures leg the disclosure misses exactly the ids a + // planted or merged-foreign list chose. + const chained: ConvergenceDiagnosis = { + round: 2, + posted: 1, + fresh: 1, + clusters: [], + successorChains: [ + { + file: 'src/a.ts', + generations: [['R0-1'], ['R1-1']], + newIds: ['R2-1'], + }, + ], + volumeNotShrinking: false, + truncatedEvidence: false, + foreignEvidence: true, + mergedEvidence: false, + }; + const r = renderConvergenceDiagnosis(chained); + expect(r.en).toContain('the closure lineage named above'); + expect(r.en).toContain("may not be this account's own"); + expect(r.zh).toContain('上述闭包血缘'); + // The merged list protects this account's own entries under their own + // ids, so the caveat says "some of", exactly as for rounds and counts. + const merged = renderConvergenceDiagnosis({ + ...chained, + mergedEvidence: true, + }); + expect(merged.en).toContain( + 'some of the closure lineage named above may not be', + ); + }); + it('summarises the tail instead of listing every cluster', () => { const many = Array.from({ length: MAX_RENDERED_CLUSTERS + 2 }, (_, i) => ({ file: `f${i}.ts`, @@ -1134,6 +1640,7 @@ describe('renderConvergenceDiagnosis — what the author reads', () => { posted: 3, fresh: 3, clusters: base.clusters, + successorChains: [], volumeNotShrinking: false, truncatedEvidence: false, foreignEvidence: false, diff --git a/packages/cli/src/commands/review/lib/convergence.ts b/packages/cli/src/commands/review/lib/convergence.ts index b363cf88b5b..f326358a3f8 100644 --- a/packages/cli/src/commands/review/lib/convergence.ts +++ b/packages/cli/src/commands/review/lib/convergence.ts @@ -32,7 +32,10 @@ import { LEDGER_ID_TOKEN, LEDGER_MAX_FILE, LEDGER_MAX_ROUND, + LEDGER_MAX_TITLE, + claimLocator, isStandInName, + type LedgerClosure, type LedgerFinding, } from './ledger.js'; import { mdField } from './md-field.js'; @@ -96,6 +99,28 @@ export interface DraftedFinding { fixInduced?: boolean; } +/** + * One subsystem whose Criticals keep regrowing (#9905): a file that closed + * Critical(s) in the previous round and again this round — the previous + * marker's minted closures (`r === round - 1`) and this round's + * (`r === round`) — while posting a fresh Critical on it now. That is the + * patch-and-regress shape: each fix grew the next Critical in the same + * mechanism, which is a claim about the MECHANISM, not about any one + * finding. + */ +export interface SuccessorChain { + /** The subsystem, named by the file every generation landed on. */ + file: string; + /** + * The closure ids of each of the two rounds, oldest first — + * `[round N-1's, round N's]`. `newIds` is the chain's final generation + * but is NOT in this array; the renderer appends it. + */ + generations: string[][]; + /** The fresh Critical ids this round posts on the file. */ + newIds: string[]; +} + /** What the previous round left behind, and how far it can be trusted. */ export interface PrevRound { /** Inline comments the previous round posted, when it recorded the number. */ @@ -131,6 +156,15 @@ export interface PrevRound { * by exactly the part the union protected. */ merged?: boolean; + /** + * Its findings were adopted by an ANONYMOUS whole-write — recovery ran + * with no identity to vouch them (`pr-context` stamps the record). Not + * `foreign` to the disclosure caveat — an unknown identity is not a + * foreign author — but the absence-based inferences read the list like a + * pure-foreign one: a vanished id may sit in a list no identity vouched, + * so the closure mint and the `land-and-defer` gate both withhold. + */ + anonymousAdoption?: boolean; /** * The posting floor it ran under, when its marker recorded one. A round * that posted under a different floor is not a comparable point on this @@ -156,6 +190,13 @@ export interface PrevRound { * and on a model-identity drift. */ anchored?: boolean; + /** + * The Criticals that round closed — the closures its marker minted (each + * carries `r === round`). Absent on every marker written before the field + * existed, which reads as "no closures recorded" and silences the + * successor-chain signal: thin history stays silent rather than guesses. + */ + closed?: readonly LedgerClosure[]; } export interface ConvergenceDiagnosis { @@ -175,6 +216,13 @@ export interface ConvergenceDiagnosis { prevFresh?: number; /** Files that carried findings before and carry more now. */ clusters: RecurrenceCluster[]; + /** + * Subsystems whose Criticals keep regrowing (#9905) — the sharper form of + * a recurrence cluster: not "the file sees findings again" but "the last + * two rounds each CLOSED a Critical here and this round posts another", + * the patch-and-regress chain where each fix grows the next Critical. + */ + successorChains: SuccessorChain[]; /** True when this round's volume did not fall below the previous round's. */ volumeNotShrinking: boolean; /** Carried through from `PrevRound` so the rendering can disclose them. */ @@ -248,11 +296,14 @@ export type CriticalFloorKind = * spend the code set's only real property — that a code means a fact was * observed — on a constant. * - * That is the whole menu: eleven codes in the design, four emitted here, - * seven named above. + * That is the whole menu: twelve codes in the design, five emitted here + * (`successor-chain` joined the four existing codes with #9905 — its + * measurement is the closure lineage the marker now records), seven named + * above. */ export const RECOMMENDATION_CODES = [ 'root-cause-triage', + 'successor-chain', 'land-and-defer', 'batch-fixes', 'stem-surface', @@ -440,6 +491,23 @@ export function diagnoseConvergence(input: { * previous work list this round does not re-post was fixed. */ openCriticals?: number; + /** + * The closures THIS round mints — the previous work list's Criticals this + * round does not re-post — computed by the caller from the same built + * ledger the marker stamps, so the marker's `closed` field and this + * signal can never disagree about what closed when. Empty whenever the + * previous work list is incomplete: a vanished id in a truncated list + * may be the budget, not a ruling, so a partial list mints nothing. + */ + closuresThisRound?: readonly LedgerClosure[]; + /** + * This round's findings AS THE BUILT LEDGER stamps them. The successor + * chain reads its new side from here — the built ids, post readback and + * admission — rather than from the drafts, so the note's `R-` + * references are the ones the posted review and the next round's work + * list actually use. + */ + thisRoundFindings?: readonly LedgerFinding[]; }): ConvergenceDiagnosis | null { const priorByFile = new Map>(); for (const f of input.prev.findings) { @@ -551,6 +619,103 @@ export function diagnoseConvergence(input: { (a.file < b.file ? -1 : a.file > b.file ? 1 : 0), ); + // The successor chain (#9905): a file that CLOSED a Critical in each of + // the last two rounds — this round's minted set (`r === round`) and the + // previous marker's (`r === round - 1`) — while this round posts a FRESH + // Critical on it. The cluster signal above says "the file sees findings + // again"; this says "the fix closed one and the mechanism grew another", + // twice in a row — the #9659 rebound shape, where every finding was + // individually correct and the subsystem was diverging. Joined by file, + // deterministically, like the cluster. + const successorChains: SuccessorChain[] = []; + const closedNow = input.closuresThisRound ?? []; + const closedPrev = input.prev.closed ?? []; + const thisRoundFindings = input.thisRoundFindings ?? []; + // The fresh side's claim-identity defense — the mint's mirror. A re-post + // whose readback lost the carried id is stamped with a FRESH id in the + // build (see `idFor`), so `birthRound === round` alone counts a + // still-standing claim as the chain's new generation — the note firing + // over a claim the same round's work list says never left. Join on the + // locator projection the mint uses, over the recovered previous list: + // PRESENT in it is evidence regardless of its completeness, and its + // titles are already write-capped, so the built side caps before + // projecting the way the serializer would. + if ( + input.round >= 2 && + closedNow.length > 0 && + closedPrev.length > 0 && + thisRoundFindings.length > 0 + ) { + const standingPrevClaims = new Set( + input.prev.findings + .map((g) => claimLocator(g.title)) + .filter((k) => k !== ''), + ); + const freshCriticalsByFile = new Map(); + for (const f of thisRoundFindings) { + if (f.sev !== 'C') continue; + // The stand-in rule the cluster join applies: a pathless or body-only + // Critical names no subsystem. `k` is respected the same way — the + // built ledger flags a REAL path that happens to spell like one. + if (isStandInName(f.file) && f.k !== 1) continue; + // The built id's round IS the first-reported round: a fresh finding + // was stamped this round, a carried one keeps its original id. + if (birthRound(f.id) !== input.round) continue; + // A re-mint of a claim still standing in the previous list is a + // re-post, not a new generation — see `standingPrevClaims` above. + const locator = claimLocator(f.title.slice(0, LEDGER_MAX_TITLE)); + if (locator !== '' && standingPrevClaims.has(locator)) continue; + const ids = freshCriticalsByFile.get(f.file); + if (ids) ids.push(f.id); + else freshCriticalsByFile.set(f.file, [f.id]); + } + for (const [file, newIds] of freshCriticalsByFile) { + const generations: string[][] = []; + for (const [closures, r] of [ + [closedPrev, input.round - 1], + [closedNow, input.round], + ] as const) { + const idsAt = closures + .filter( + (c) => + c.r === r && + // Stand-in-named closures never join: `LedgerClosure` carries + // no `k`, so a body-only closure is indistinguishable from a + // closure on a REAL file spelled like the stand-in. Joining + // them would post a lineage over a mechanism the closures + // never anchored on — silence, never a guess. + !isStandInName(c.f) && + // Closures are capped at LEDGER_MAX_FILE on every route while + // this side keys on the RAW built path, so an exact join is + // permanently silent past the cap — mirror `priorFor`'s slice + // fallback, or the advisory disarms on exactly the deep + // subsystems it exists for. + (c.f === file || + (file.length > LEDGER_MAX_FILE && + c.f === file.slice(0, LEDGER_MAX_FILE))), + ) + .map((c) => c.id); + if (idsAt.length === 0) { + generations.length = 0; + break; + } + generations.push(idsAt); + } + if (generations.length === 0) continue; + successorChains.push({ file, generations, newIds }); + } + // Sorted, like the cluster above, so WHICH diverging subsystems the two + // `.slice(0, MAX_RENDERED_CLUSTERS)` consumers name is a measured + // property, never the map's insertion order: the most new work first, + // closure volume second, the code-unit path the tie-break. + successorChains.sort( + (a, b) => + b.newIds.length - a.newIds.length || + b.generations.flat().length - a.generations.flat().length || + (a.file < b.file ? -1 : a.file > b.file ? 1 : 0), + ); + } + // A round that produced NO fresh finding is the observation a convergence // trend most wants, not a symptom: zero new work is where a settling loop // lands, and `0 >= 0` would otherwise narrate "the volume is not falling" @@ -592,7 +757,12 @@ export function diagnoseConvergence(input: { fresh.length > 0 && fresh.length >= input.prev.fresh; - if (clusters.length === 0 && !volumeNotShrinking) return null; + if ( + clusters.length === 0 && + !volumeNotShrinking && + successorChains.length === 0 + ) + return null; return { ...(input.openCriticals === undefined @@ -606,6 +776,7 @@ export function diagnoseConvergence(input: { : { prevPosted: input.prev.posted }), ...(input.prev.fresh === undefined ? {} : { prevFresh: input.prev.fresh }), clusters, + successorChains, volumeNotShrinking, truncatedEvidence: input.prev.truncated === true, foreignEvidence: input.prev.foreign === true, @@ -616,6 +787,28 @@ export function diagnoseConvergence(input: { }; } +/** + * The per-generation id bound for a rendered chain, so a round that closed + * a whole family on the file cannot grow the paragraph unboundedly. + */ +const MAX_CHAIN_IDS_PER_GENERATION = 6; + +/** + * The chain as the paragraph and the recommendation's basis render it — + * `R9-1 → R10-2/R10-3 → R11-4`, oldest generation first, the new ids last. + * ONE renderer for both, so the prose and the machine-readable half cannot + * disagree about the lineage they name. + */ +export function renderSuccessorChain(c: SuccessorChain): string { + const renderGeneration = (ids: string[]): string => { + const shown = ids.slice(0, MAX_CHAIN_IDS_PER_GENERATION).join('/'); + return ids.length > MAX_CHAIN_IDS_PER_GENERATION + ? `${shown} … (+${ids.length - MAX_CHAIN_IDS_PER_GENERATION})` + : shown; + }; + return [...c.generations, c.newIds].map(renderGeneration).join(' → '); +} + /** * The handling recommendations this diagnosis matches — measurement to * advice, with zero constants and zero decisions. @@ -635,6 +828,17 @@ export function recommendationsFor(d: ConvergenceDiagnosis): Recommendation[] { basis: `${d.clusters.length} file(s) carried findings in earlier rounds and carry new ones now: ${shown.join(', ')}${d.clusters.length > shown.length ? ', …' : ''}`, }); } + if (d.successorChains.length > 0) { + const shown = d.successorChains + .slice(0, MAX_RENDERED_CLUSTERS) + .map((c) => `${c.file} (${renderSuccessorChain(c)})`); + out.push({ + code: 'successor-chain', + basis: + `${d.successorChains.length} subsystem(s) closed Critical(s) in the previous round and again this round, and post a new one now: ` + + `${shown.join(', ')}${d.successorChains.length > shown.length ? ', …' : ''}`, + }); + } if (d.volumeNotShrinking) { out.push({ code: 'batch-fixes', @@ -764,6 +968,27 @@ export function renderConvergenceDiagnosis(d: ConvergenceDiagnosis): { // recurrence and a flat trend together. const reasonsEn: string[] = []; const reasonsZh: string[] = []; + if (d.successorChains.length > 0) { + // The sharpest shape, named first: not "the file sees findings again" + // but "the last two rounds each CLOSED a Critical here and this round + // posts another" — the chain is named in full because the ids are the + // evidence the author acts on. `mdField` on every segment: the file is + // PR-controlled and the ids arrive over the marker. + const shown = d.successorChains.slice(0, MAX_RENDERED_CLUSTERS); + const more = d.successorChains.length - shown.length; + const chainsEn = shown + .map((c) => `${mdField(c.file)} (${mdField(renderSuccessorChain(c))})`) + .join('; '); + const chainsZh = shown + .map((c) => `${mdField(c.file)}(${mdField(renderSuccessorChain(c))})`) + .join('；'); + reasonsEn.push( + `⚠️ Divergence: the same subsystem closed Critical(s) in the previous round and again this round, and posts a new one now — ${chainsEn}${more > 0 ? `; and ${more} more` : ''}.`, + ); + reasonsZh.push( + `⚠️ 发散：同一子系统在上一轮和本轮各关闭了至少一个 Critical，本轮又出现了新的 Critical——${chainsZh}${more > 0 ? `；另有 ${more} 个` : ''}。`, + ); + } if (d.clusters.length > 0) { reasonsEn.push( `Findings keep coming back to the same files: ${clusterEn}${more > 0 ? `, and ${more} more file(s)` : ''}.`, @@ -783,11 +1008,14 @@ export function renderConvergenceDiagnosis(d: ConvergenceDiagnosis): { // affects only the WORK LIST, so it qualifies the recurrence reading // alone. Provenance is broader: a foreign marker carries the previous // round's VOLUME too, and the volume reading cites that number as this - // loop's own baseline — gated on clusters, the disclosure never reached - // exactly the branch an attacker-supplied count controls. + // loop's own baseline — and its MINTED CLOSURES too, which the chain + // cites as this loop's own lineage. Gated on clusters, the disclosure + // never reached exactly the branches an attacker-supplied count or + // closure list controls. const citesWorkList = d.clusters.length > 0; const citesPrevVolume = d.prevPosted !== undefined || d.prevFresh !== undefined; + const citesClosures = d.successorChains.length > 0; const caveatsEn: string[] = []; const caveatsZh: string[] = []; // Truncation qualifies BOTH readings, not only the recurrence one: the @@ -816,12 +1044,25 @@ export function renderConvergenceDiagnosis(d: ConvergenceDiagnosis): { ); caveatsZh.push(`上一轮的工作清单为放进标记而被截断，${what.zh}`); } - if (d.foreignEvidence && (citesWorkList || citesPrevVolume)) { - const what = citesWorkList - ? citesPrevVolume - ? { en: `those rounds and its counts`, zh: `上述轮次与其计数` } - : { en: `those rounds`, zh: `上述轮次` } - : { en: `those counts`, zh: `该计数` }; + if ( + d.foreignEvidence && + (citesWorkList || citesPrevVolume || citesClosures) + ) { + const partsEn: string[] = []; + const partsZh: string[] = []; + if (citesWorkList) { + partsEn.push(`those rounds`); + partsZh.push(`上述轮次`); + } + if (citesPrevVolume) { + partsEn.push(citesWorkList ? `its counts` : `those counts`); + partsZh.push(citesWorkList ? `其计数` : `该计数`); + } + if (citesClosures) { + partsEn.push(`the closure lineage named above`); + partsZh.push(`上述闭包血缘`); + } + const what = { en: partsEn.join(` and `), zh: partsZh.join(`与`) }; caveatsEn.push( d.mergedEvidence ? `the previous round was recovered from a marker this account did not post and merged over this account's own entries, so some of ${what.en} may not be this account's own` @@ -833,6 +1074,20 @@ export function renderConvergenceDiagnosis(d: ConvergenceDiagnosis): { : `上一轮的数据来自并非本账号发布的标记，${what.zh}可能不属于本账号`, ); } + // The chain's newest generation is fresh-stamped BY CONSTRUCTION — the + // fresh scan admits no carried id — and a re-voice of a still-open claim + // whose readback lost its id is textually indistinguishable from a new + // Critical. Blanket suppression cannot separate that shape from the + // legitimate rebound the signal exists for, so the note discloses the + // identity gap instead of asserting the generation is new. + if (d.successorChains.length > 0) { + caveatsEn.push( + `the chain's newest generation carries ids stamped this round — a still-open claim re-voiced without its carried id reads the same there as a new Critical`, + ); + caveatsZh.push( + `链条最新一代携带的 id 由本轮铸造——一个未解决断言若在不携带原 id 的情况下被重新表述，在那里与新的 Critical 无法区分`, + ); + } const caveatEn = caveatsEn.length > 0 ? ` (Evidence: ${caveatsEn.join('; ')}.)` : ''; const caveatZh = @@ -879,6 +1134,8 @@ export function renderConvergenceDiagnosis(d: ConvergenceDiagnosis): { const clusterAdviceEn = `A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time.`; const clusterAdviceZh = `一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。`; + const chainAdviceEn = `A mechanism whose fix grows the next Critical is diverging, not converging — raising the pattern with the mechanism's owner before the next round tends to end the loop faster than patching it again.`; + const chainAdviceZh = `每次修复都长出下一个 Critical 的机制是在发散而非收敛——先把这一模式提给该机制的负责人，通常比继续打补丁更快结束循环。`; const landEn = `No Critical finding is open on this round, so merging and moving the remaining Suggestion threads to a follow-up issue is available as an ending — a merged pull request cannot diverge further.`; const landZh = `本轮没有未决的 Critical，因此"合入后把剩余 Suggestion 线程转到后续 issue"是一个可选的结束方式——已合入的 PR 不会继续发散。`; @@ -887,6 +1144,7 @@ export function renderConvergenceDiagnosis(d: ConvergenceDiagnosis): { // describe different rounds — and this module's whole claim is that its // advice is matched to what it measured. const adviceEn = [ + has('successor-chain') ? chainAdviceEn : null, has('root-cause-triage') ? clusterAdviceEn : null, has('batch-fixes') ? floorEn : null, has('land-and-defer') ? landEn : null, @@ -894,6 +1152,7 @@ export function renderConvergenceDiagnosis(d: ConvergenceDiagnosis): { .filter(Boolean) .join(' '); const adviceZh = [ + has('successor-chain') ? chainAdviceZh : null, has('root-cause-triage') ? clusterAdviceZh : null, has('batch-fixes') ? floorZh : null, has('land-and-defer') ? landZh : null, diff --git a/packages/cli/src/commands/review/lib/ledger.test.ts b/packages/cli/src/commands/review/lib/ledger.test.ts index 88e6f7c7c1e..9a9f20b5dbd 100644 --- a/packages/cli/src/commands/review/lib/ledger.test.ts +++ b/packages/cli/src/commands/review/lib/ledger.test.ts @@ -22,6 +22,7 @@ import { LEDGER_MAX_ROUND, LEDGER_MAX_VOLUME, LEDGER_MAX_ID, + LEDGER_MAX_CLOSED, LEDGER_ID_SHAPE, isLedgerFinding, type Ledger, @@ -710,6 +711,161 @@ describe('LEDGER_ID_READBACK', () => { }); }); +// The closure list the divergence sentinel (#9905) rides on the marker: +// validated and capped on both ends like the findings, but with no +// `dropped` counterpart — closures certify nothing, so a truncated history +// has no completeness claim to protect. +describe('ledger marker — the closure list (#9905)', () => { + it('round-trips the closures through serialize and parse', () => { + const marker = serializeLedger({ + ...LEDGER, + closed: [ + { r: 2, id: 'R1-1', f: 'src/a.ts' }, + { r: 2, id: 'R1-2', f: 'src/c.ts' }, + ], + }); + const back = parseLedger(`body\n\n${marker}`)!; + expect(back.closed).toEqual([ + { r: 2, id: 'R1-1', f: 'src/a.ts' }, + { r: 2, id: 'R1-2', f: 'src/c.ts' }, + ]); + expect(back.dropped).toBeUndefined(); + expect(back.findings).toHaveLength(2); + }); + + it('omits the field entirely when there are no closures', () => { + const back = parseLedger(serializeLedger(LEDGER))!; + expect(back.closed).toBeUndefined(); + expect(JSON.stringify(back)).not.toContain('closed'); + }); + + it('drops malformed entries and a closure claiming a FUTURE round', () => { + // Same squat rule as the findings: the marker's round is the newest + // state it can describe, so a closure past it is not one. + const back = parseLedger( + '', + )!; + expect(back.closed).toEqual([{ r: 2, id: 'R1-1', f: 'a.ts' }]); + }); + + it('binds the count cap on read, keeping the NEWEST entries', () => { + // Fed a RAW marker, not a serialize round-trip: the write side already + // sheds to the cap, so a round-tripped list never reaches the + // parse-side slice — the half that binds a hand-edited or planted + // marker no serializer ever capped. + const closed = Array.from({ length: LEDGER_MAX_CLOSED + 10 }, (_, i) => ({ + r: 2, + id: `R1-${i}`, + f: 'a.ts', + })); + const back = parseLedger( + ``, + )!; + expect(back.closed).toHaveLength(LEDGER_MAX_CLOSED); + expect(back.closed![0]!.id).toBe('R1-10'); + expect(back.closed![LEDGER_MAX_CLOSED - 1]!.id).toBe('R1-59'); + }); + + it('refuses closure ids the finding grammar refuses — squats, links, empty', () => { + // The admission test applies the SAME grammar and id-round bounds + // `isLedgerFinding` does in this file: a hand-edited or forged marker + // must not plant arbitrary ≤24-char tokens into the posted advisory and + // the machine-readable `basis` — an empty id renders a blank generation + // (` → `), `R9999-1` spells a round nobody ran, a link or mention rides + // the route whose own comment names the planted-marker threat — while + // the ledger's admission test refuses every one of them as a finding. + // The one honest entry survives. + const longFile = 'z'.repeat(LEDGER_MAX_FILE + 1); + const back = parseLedger( + '', + )!; + // `R2-1` at r=2 spells a finding closed the very round it was minted — + // the pipeline's own writer cannot produce that shape (a closure at + // round r closes a finding OPEN in round r-1's work list), so the + // admission test refuses it like the other planted tokens. + expect(back.closed).toEqual([{ r: 2, id: 'R1-2', f: 'a.ts' }]); + }); + + it('admits a same-round closure id ONLY at the round cap — the escape hatch', () => { + // At the cap, consecutive rounds re-stamp the same `R-*` id space, + // so a minted closure's id round EQUALS its closure round — admitted + // only via the escape hatch. Below the cap the same shape spells a + // finding closed the very round it was minted, and the admission test + // refuses it like the planted `R2-1` above. + const atCap = parseLedger( + ``, + )!; + expect(atCap.closed).toEqual([ + { r: LEDGER_MAX_ROUND, id: `R${LEDGER_MAX_ROUND}-1`, f: 'a.ts' }, + ]); + const belowCap = parseLedger( + ``, + )!; + expect(belowCap.closed ?? []).toEqual([]); + }); + + it('sheds closures BEFORE the anchor pair, and never sets `dropped` for them', () => { + // The cascade order is the priority order: advisory history goes before + // the anchor (a full re-review) and the work list (a ruling owed). A + // marker that paid a finding — or its anchor — for a divergence hint + // would invert the module's priorities, and `dropped` over a trimmed + // hint would withhold the anchor over advisory data. + const wide: Ledger = { + v: 1, + round: 9, + // Sized so the findings ALONE nearly fill the byte budget: the + // closures overflow it, so the shed order is observable — the + // closures go, the anchor and the work list stay. + findings: Array.from({ length: 21 }, (_, i) => ({ + id: `R9-${i}`, + sev: 'C' as const, + file: 'p/'.repeat(100).slice(0, LEDGER_MAX_FILE), + line: 99999, + title: 'x'.repeat(LEDGER_MAX_TITLE), + })), + // Ids the admission test ADMITS — the shape and length legal, the + // id round below the closure round: an invalid id is dropped by the + // serializer's admission filter before the byte cascade ever runs, + // and this test would pass vacuously over the shed stage it names. + closed: Array.from({ length: LEDGER_MAX_CLOSED }, (_, i) => ({ + r: 9, + id: `R8-${i}`, + f: 'p/'.repeat(100).slice(0, LEDGER_MAX_FILE), + })), + sha: 'deadbeef00112233', + }; + const marker = serializeLedger(wide); + expect(marker.length).toBeLessThanOrEqual(LEDGER_MAX_BYTES); + const back = parseLedger(`body\n\n${marker}`)!; + expect(back.closed ?? []).toHaveLength(0); + expect(back.sha).toBe('deadbeef00112233'); + expect(back.findings).toHaveLength(21); + expect(back.dropped).toBeUndefined(); + }); +}); + // `src0` is the approach signal's baseline — see `Ledger.src0`. It is the one // field that survives truncation, because the ruling that withholds an anchor // from a partial finding list does not extend to a measurement of the diff. diff --git a/packages/cli/src/commands/review/lib/ledger.ts b/packages/cli/src/commands/review/lib/ledger.ts index 6f77a944808..7b74e4d5235 100644 --- a/packages/cli/src/commands/review/lib/ledger.ts +++ b/packages/cli/src/commands/review/lib/ledger.ts @@ -57,10 +57,39 @@ export interface LedgerFinding { title: string; } +/** + * One Critical that LEFT the work list at round `r`: open in round `r-1`'s + * marker, absent from round `r`'s. Compact field names (`r`/`f`) on the same + * body-bytes discipline as the findings. + */ +export interface LedgerClosure { + /** The round whose marker the finding is absent from. */ + r: number; + /** The closed finding's id — its ORIGINAL round's id, e.g. `R9-1`. */ + id: string; + /** The closed finding's file. */ + f: string; +} + export interface Ledger { v: 1; round: number; findings: LedgerFinding[]; + /** + * The Criticals that LEFT the work list at THIS marker's round — open in + * the previous round's marker, absent from this one. The divergence + * sentinel (#9905) reads them: `fixed` and `superseded` both read as + * closure, a positional diff cannot tell them apart, and the advisory + * note does not need it to. + * + * This round's own minted set only — NO carry-forward: the sentinel's + * K=2 check reads closures at round N (minted while composing) and N-1 + * (read back off this field), so older entries are dead bytes. Absent on + * every ledger written before the field existed, which reads as "no + * closures recorded" and silences the sentinel — the honest posture on + * thin history. + */ + closed?: LedgerClosure[]; /** * How many findings the size cap dropped, when it dropped any. Absent means * the list is complete — which is the claim the next round acts on, so the @@ -328,6 +357,22 @@ export const LEDGER_ID_READBACK = new RegExp( */ export const LEDGER_ID_SHAPE = new RegExp(`^${LEDGER_ID_TOKEN}$`); +/** + * The claim LOCATOR of a work-list entry or a built finding's title: the + * carried id stripped through the readback above, the part before the first + * ' — ' kept, backticks gone. Stated once because every consumer that joins + * claims by it — the gate-repost dedup, the closure mint (#9905), and the + * successor chain's fresh side — must project the SAME shape, and a second + * spelling is the drift class this module's header exists to prevent. + */ +export function claimLocator(entry: string): string { + const claim = entry.trim().replace(LEDGER_ID_READBACK, '').trim(); + const dash = claim.indexOf(' — '); + // Backticks stripped: the gate renders the path through `mdField`, and a + // re-post that types the locator plainly names the same finding. + return (dash === -1 ? claim : claim.slice(0, dash)).replace(/`/g, '').trim(); +} + /** Caps keep the marker a footnote, never a payload: GitHub's body limit is * 65,536 chars and the marker rides inside it. Every cap binds BOTH halves — * the serializer so the write side is bounded, the parser so a hand-edited @@ -378,6 +423,13 @@ export const LEDGER_MAX_MODEL = 64; * (`R2-1`), and anything longer is not one. */ export const LEDGER_MAX_ID = 24; +/** + * The closure list's bound. One round's closures are bounded by the previous + * work list's size (`LEDGER_MAX_FINDINGS`), so fifty covers every mintable + * round; the cap exists for the hand-edited marker, which is bound by no + * mint. The byte cap below still binds the whole marker either way. + */ +export const LEDGER_MAX_CLOSED = 50; /** * The highest round a marker may claim. * @@ -494,6 +546,7 @@ export function serializeLedger(ledger: Ledger): string { dropped: number, anchor: boolean, volume: 'both' | 'posted' | 'none', + closures: boolean, ): string => { const payload: Ledger = { v: 1, @@ -502,6 +555,22 @@ export function serializeLedger(ledger: Ledger): string { round: roundOut, findings, }; + // The closure list rides with the findings but is NOT one of them: it + // feeds an advisory note, so the byte budget sheds it ahead of the + // anchor pair and the work list (see the cascade below), and shedding + // it never sets `dropped` — closures certify no range, so there is no + // completeness claim to protect. Newest-kept, like the findings. + if (closures) { + const closedOut = (ledger.closed ?? []) + .filter((c) => isLedgerClosure(c, roundOut)) + .slice(-LEDGER_MAX_CLOSED) + .map((c) => ({ + r: c.r, + id: c.id.slice(0, LEDGER_MAX_ID), + f: c.f.slice(0, LEDGER_MAX_FILE), + })); + if (closedOut.length > 0) payload.closed = closedOut; + } // The volume telemetry rides OUTSIDE the truncation rule that governs the // anchor — it qualifies no range, so a partial work list says nothing // about it — but it is the FIRST thing the byte budget sheds (see the @@ -579,7 +648,7 @@ export function serializeLedger(ledger: Ledger): string { // 51 findings in, 24 kept, and it said 26 missing. const total = ledger.findings.length; let kept = capped.length; - let marker = render(capped, total - kept, true, 'both'); + let marker = render(capped, total - kept, true, 'both', true); if (marker.length > LEDGER_MAX_BYTES) { // Between "both volumes" and "no volume" there is a rung worth having: // the CARRIED value goes first, this round's own count second. The @@ -588,7 +657,7 @@ export function serializeLedger(ledger: Ledger): string { // and stamps as its own `prevPosted` — so shedding them as one unit // dropped a count that still fitted, and broke the chain a round // earlier than the budget required. - marker = render(capped, total - kept, true, 'posted'); + marker = render(capped, total - kept, true, 'posted', true); } if (marker.length > LEDGER_MAX_BYTES) { // The VOLUME sheds first — it is the only thing here that decides @@ -599,7 +668,13 @@ export function serializeLedger(ledger: Ledger): string { // marker that fit WITH its anchor over the cap and make the re-render // pay with the anchor instead — trading a full-diff re-review for a // trend line. - marker = render(capped, total - kept, true, 'none'); + marker = render(capped, total - kept, true, 'none', true); + } + if (marker.length > LEDGER_MAX_BYTES) { + // The closure list sheds next: it feeds the sentinel's advisory note, + // and losing it costs one round of lineage, while everything below it + // — the anchor, the work list — is a claim the next round acts on. + marker = render(capped, total - kept, true, 'none', false); } if (marker.length > LEDGER_MAX_BYTES) { // Shed the anchor PAIR before any finding: `dropped` withholds the pair @@ -608,11 +683,11 @@ export function serializeLedger(ledger: Ledger): string { // first keeps the whole work list — recovery degrades to the full diff, // which the findings survive — and findings start going only when the // anchorless form still exceeds the cap. - marker = render(capped, total - kept, false, 'none'); + marker = render(capped, total - kept, false, 'none', false); } while (marker.length > LEDGER_MAX_BYTES && kept > 0) { kept--; - marker = render(capped.slice(0, kept), total - kept, false, 'none'); + marker = render(capped.slice(0, kept), total - kept, false, 'none', false); } return marker; } @@ -677,6 +752,54 @@ export function isLedgerFinding( ); } +/** + * Is this a closure entry this pipeline would admit, against the round the + * marker claims? The same rules as the findings, minus the fields a closure + * does not carry: the caps bind on read as on write, a closure claiming a + * round past the marker's own is a squat — the marker's round IS the newest + * state it can describe — and the id obeys the SAME grammar and id-round + * bounds `isLedgerFinding` applies in this file. Skipping them here let a + * hand-edited or forged marker plant arbitrary ≤24-char tokens — an empty + * string, `R9999-1` spelling a round nobody ran, a live link or mention — + * into the posted advisory and the machine-readable `basis`, through the + * route whose comment names the planted-marker threat. + */ +export function isLedgerClosure( + c: unknown, + markerRound: number, +): c is LedgerClosure { + const e = c as LedgerClosure | null | undefined; + if ( + !e || + typeof e !== 'object' || + !Number.isInteger(e.r) || + e.r < 1 || + e.r > markerRound || + typeof e.id !== 'string' || + e.id.length > LEDGER_MAX_ID || + !LEDGER_ID_SHAPE.test(e.id) || + typeof e.f !== 'string' || + e.f === '' || + e.f.length > LEDGER_MAX_FILE + ) { + return false; + } + const idRound = Number(e.id.slice(1).split('-')[0]); + if (!Number.isSafeInteger(idRound)) return false; + if (idRound < 1 || idRound > Math.min(markerRound, LEDGER_MAX_ROUND)) { + return false; + } + // The id must also be OLDER than the closure: a closure at round r + // records a finding still OPEN in round r-1's work list, so its mint + // round is strictly below r — equality is only possible at the round cap, + // where consecutive rounds re-stamp the same `R-*` space. The mint + // (compose-review) copies ids out of the recovered work list, so its own + // output cannot violate this; the cross-check binds the hand-edited and + // planted routes, where an `idRound >= r` entry seeds the chain join with + // a generation that never stood open. + return idRound < e.r || e.r >= LEDGER_MAX_ROUND; +} + /** * A finding normalised to the caps the serializer writes under. Applied on * READ too: the caps are the serializer's contract, and neither a @@ -830,10 +953,18 @@ export function parseLedger(body: string | undefined): Ledger | null { freshRaw !== undefined && posted !== undefined && freshRaw <= posted ? freshRaw : undefined; + // The closure history, validated like the findings: the caps and the + // round bind on read as on write. No `dropped` counterpart — closures + // certify nothing, so a truncated history has no completeness claim to + // protect, and the sentinel reads absence as silence either way. + const closed = (Array.isArray(raw.closed) ? raw.closed : []) + .filter((c): c is LedgerClosure => isLedgerClosure(c, raw.round)) + .slice(-LEDGER_MAX_CLOSED); return { v: 1, round: raw.round, findings, + ...(closed.length > 0 ? { closed } : {}), ...(dropped ? { dropped } : {}), ...(sha ? { sha } : {}), ...(model ? { model } : {}), diff --git a/packages/cli/src/commands/review/pr-context-persist.test.ts b/packages/cli/src/commands/review/pr-context-persist.test.ts index 45bf81d225a..62c63572912 100644 --- a/packages/cli/src/commands/review/pr-context-persist.test.ts +++ b/packages/cli/src/commands/review/pr-context-persist.test.ts @@ -235,6 +235,241 @@ describe('persistRecoveredLedger', () => { } }); + it('a FOREIGN winner carries no planted closures into the side file', () => { + // The closure seam's strip half, end to end: any account that can submit + // a review can post a marker carrying `closed`, and recovery adopts the + // highest round inside the headroom. If the foreign winner's PLANTED + // closures rode the identity-known write into the side file, the next + // compose would feed them to the divergence sentinel as THIS loop's own + // lineage — and the anonymous write would stamp them `foreign: false`, + // laundering the provenance past every downstream guard. The planted + // list must not survive. The own list from the round-GAP own marker is + // not restored either: its entries stamp `r` = 3, and the compose this + // recovery feeds reads exactly `r` = 4 off it — dead bytes, like volume. + const dir = mkdtempSync(join(tmpdir(), 'prev-ledger-')); + const side = join(dir, 'side.json'); + try { + const ownMarker = + ''; + const plantedMarker = + ''; + const { recovered } = recoverLedger( + [ + { + id: 1, + user: { login: 'bot' }, + submitted_at: '2026-01-01T00:00:00Z', + body: ownMarker, + }, + { + id: 2, + user: { login: 'stranger' }, + submitted_at: '2026-01-02T00:00:00Z', + body: plantedMarker, + }, + ], + 'bot', + ); + expect(recovered?.foreign).toBe(true); + persistRecoveredLedger(side, recovered, { + noOwnReview: false, + identityKnown: true, + }); + const written = JSON.parse(readFileSync(side, 'utf8')); + expect(written.round).toBe(4); + expect(written.closed).toBeUndefined(); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + it("a SAME-round union restores this account's own closures to disk", () => { + // The restore half: the own marker describes the SAME round the foreign + // winner claims (the winner's own closures were stripped at the seam). + // The union exists so a foreign marker cannot erase own data — the own + // mint is exactly the generation the next compose reads `r === round` + // off — so it comes back beside the same-round volume, and the restore + // only means anything if it survives the identity-known write. Left + // unrestored, one interleaved same-round foreign marker silently + // disarms the divergence sentinel for this account's next round. + const dir = mkdtempSync(join(tmpdir(), 'prev-ledger-')); + const side = join(dir, 'side.json'); + try { + const ownMarker = + ''; + const foreignMarker = + ''; + const { recovered } = recoverLedger( + [ + { + id: 1, + user: { login: 'bot' }, + submitted_at: '2026-01-01T00:00:00Z', + body: ownMarker, + }, + { + id: 2, + user: { login: 'stranger' }, + submitted_at: '2026-01-02T00:00:00Z', + body: foreignMarker, + }, + ], + 'bot', + ); + expect(recovered?.foreign).toBe(true); + expect(recovered?.merged).toBe(true); + persistRecoveredLedger(side, recovered, { + noOwnReview: false, + identityKnown: true, + }); + const written = JSON.parse(readFileSync(side, 'utf8')); + expect(written.round).toBe(4); + // The OWN mint, not the stranger's planted list. + expect(written.closed).toEqual([{ r: 4, id: 'R3-1', f: 'a.ts' }]); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + it('an ANONYMOUS whole-write sheds the closures it cannot vouch', () => { + // Without a `me` every marker walks as foreign, so the recovery strip + // already fires upstream — but this seam defends ITSELF rather than + // depending on that invariant holding forever: it is the one path where + // a whole foreign ledger is written to the file, and kept closures + // would be stamped `foreign: false` here, laundering a stranger's + // lineage past every provenance guard downstream. Fed DIRECTLY, with a + // closure-carrying ledger the upstream strip never saw, the shed must + // hold on its own — exactly as the churn shed beside it is pinned. + const dir = mkdtempSync(join(tmpdir(), 'prev-ledger-')); + const side = join(dir, 'side.json'); + try { + persistRecoveredLedger( + side, + { + ledger: { + v: 1, + round: 4, + findings: [{ id: 'R4-1', sev: 'S', file: 'a.ts', title: 't' }], + closed: [{ r: 4, id: 'R3-1', f: 'a.ts' }], + }, + commitId: 'c'.repeat(40), + reviewId: 44, + foreign: true, + merged: false, + }, + { noOwnReview: false, identityKnown: false }, + ); + const written = JSON.parse(readFileSync(side, 'utf8')); + expect(written.round).toBe(4); + expect(written.closed).toBeUndefined(); + expect(written.foreign).toBe(false); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + it('an ANONYMOUS higher-round advance sheds the closures with the volume', () => { + // The counter-advance branch keeps the file's OWN findings but sheds + // every fact ABOUT the round it leaves behind — anchor, volume, streak. + // Each closure is the same class: stamped with the round that minted + // it, which this advance passes. Kept, it rides a rebuilt file whose + // round no longer describes it. + const dir = mkdtempSync(join(tmpdir(), 'prev-ledger-')); + const side = join(dir, 'side.json'); + try { + writeFileSync( + side, + JSON.stringify({ + ...ledger, + round: 7, + reviewId: 100, + posted: 4, + closed: [{ r: 7, id: 'R6-1', f: 'a.ts' }], + }), + ); + persistRecoveredLedger( + side, + { + ledger: { + v: 1, + round: 8, + findings: [{ id: 'R8-1', sev: 'S', file: 'x.ts', title: 'theirs' }], + }, + commitId: 'c'.repeat(40), + reviewId: 200, + foreign: false, + merged: false, + }, + { noOwnReview: false, identityKnown: false }, + ); + const written = JSON.parse(readFileSync(side, 'utf8')); + expect(written.round).toBe(8); + expect(written.findings).toEqual(ledger.findings); + expect(written.closed).toBeUndefined(); + expect(written.posted).toBeUndefined(); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + it('an OWN winner carries its closures across the seam untouched', () => { + // The strip ternary's KEEP branch: an own marker wins recovery — the + // routine case, every ordinary own round recovers its previous marker + // as non-foreign — and its `closed` list must cross into the side file + // whole. The four sibling seam tests pin the foreign strip, the union + // restore, and the two anonymous sheds; none pins this branch, so + // hoisting `withoutClosures` out of the ternary's foreign arm strips + // every own round's closures before compose ever reads them — the + // divergence sentinel disarmed for this account with the suite green. + const dir = mkdtempSync(join(tmpdir(), 'prev-ledger-')); + const side = join(dir, 'side.json'); + try { + const ownMarker = + ''; + const olderMarker = + ''; + const { recovered } = recoverLedger( + [ + { + id: 1, + user: { login: 'stranger' }, + submitted_at: '2026-01-01T00:00:00Z', + body: olderMarker, + }, + { + id: 2, + user: { login: 'bot' }, + submitted_at: '2026-01-02T00:00:00Z', + body: ownMarker, + }, + ], + 'bot', + ); + expect(recovered?.foreign).toBe(false); + expect(recovered?.merged).toBe(false); + persistRecoveredLedger(side, recovered, { + noOwnReview: false, + identityKnown: true, + }); + const written = JSON.parse(readFileSync(side, 'utf8')); + expect(written.round).toBe(4); + expect(written.closed).toEqual([{ r: 4, id: 'R3-1', f: 'a.ts' }]); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + it('records that the winning marker came from another account', () => { // The convergence diagnosis CITES the round numbers carried in this work // list, in a body this account posts. Recovery adopts the highest-round @@ -1080,6 +1315,99 @@ describe('persistRecoveredLedger', () => { } }); + it('an ANONYMOUS whole-write records the adoption machine-readably', () => { + // The anonymous whole-write adopts a stranger's list `foreign: false` + // — right for the disclosure caveat (an UNKNOWN identity is not a + // foreign author) — but compose-review's closure mint is a second + // consumer of that stamp and cannot tell the adopted list from this + // account's own. The flag is the record the mint's honesty leg reads, + // and it rides only on this branch: this is the one write where the + // adoption happened. + const dir = mkdtempSync(join(tmpdir(), 'prev-ledger-')); + const side = join(dir, 'side.json'); + try { + persistRecoveredLedger( + side, + { + ledger: { ...ledger, round: 4 }, + commitId: null, + reviewId: 40, + foreign: true, + merged: false, + }, + { noOwnReview: false, identityKnown: false }, + ); + const written = JSON.parse(readFileSync(side, 'utf8')); + expect(written.anonymousAdoption).toBe(true); + expect(written.foreign).toBe(false); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + it('an identity-known write clears the adoption flag — an anonymous advance keeps it', () => { + // The flag describes the list the file carries. An identity-KNOWN + // recovery replaces the file with a list the union vouched — the + // adoption is over, the flag must not ride along. An anonymous + // counter-advance keeps the file's findings verbatim, so the flag + // describing them sticks exactly like `foreign` does. + const dir = mkdtempSync(join(tmpdir(), 'prev-ledger-')); + const side = join(dir, 'side.json'); + try { + writeFileSync( + side, + JSON.stringify({ + ...ledger, + round: 5, + reviewId: 50, + foreign: false, + merged: false, + anonymousAdoption: true, + }), + ); + // Anonymous higher round: the kept list is still the adopted one. + persistRecoveredLedger( + side, + { + ledger: { + v: 1, + round: 6, + findings: [{ id: 'R6-1', sev: 'S', file: 'x.ts', title: 'theirs' }], + }, + commitId: null, + reviewId: 60, + foreign: false, + merged: false, + }, + { noOwnReview: false, identityKnown: false }, + ); + expect(JSON.parse(readFileSync(side, 'utf8')).anonymousAdoption).toBe( + true, + ); + // Identity-known recovery: the union vouches the list again. + persistRecoveredLedger( + side, + { + ledger: { + v: 1, + round: 7, + findings: [{ id: 'R7-1', sev: 'S', file: 'own.ts', title: 'own' }], + }, + commitId: null, + reviewId: 70, + foreign: false, + merged: false, + }, + { noOwnReview: false, identityKnown: true }, + ); + expect( + JSON.parse(readFileSync(side, 'utf8')).anonymousAdoption, + ).toBeUndefined(); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + it('keeps the src0 baseline through the recovered write', () => { // compose-review's approach signal reads src0 from this side file; this // writer is the joint in the marker→parse→persist→read chain the diff --git a/packages/cli/src/commands/review/pr-context.ts b/packages/cli/src/commands/review/pr-context.ts index e8c7bf77685..485d50b6b66 100644 --- a/packages/cli/src/commands/review/pr-context.ts +++ b/packages/cli/src/commands/review/pr-context.ts @@ -1046,7 +1046,11 @@ export function recoverLedger( if (!best) return { recovered: null, sawOwnReview }; // The anchor never crosses accounts. Dropped here, at the recovery seam, so // no consumer downstream has to remember the rule. The churn state is the - // same class of claim and crosses with it — see `stripChurnState`. + // same class of claim and crosses with it — see `stripChurnState` — and so + // are the closures: a closure records what LEFT a work list the account + // that minted it certified, so a stranger's `closed` is their ruling-shaped + // history, not this loop's — adopted, it feeds the divergence sentinel a + // lineage this loop never produced. See `withoutClosures`. // The anchor is stripped whenever the winner is foreign, INCLUDING the // anonymous case: without a `me` every marker walks as foreign, and a // drive-by anchor must not decide which lines this pipeline stops looking @@ -1056,7 +1060,12 @@ export function recoverLedger( // `gh api user` break this account's own trend chain for two rounds — and // record its own marker as a stranger's. let ledger = best.foreign - ? stripChurnState(stripAnchor(best.ledger)) + ? (withoutClosures( + stripChurnState(stripAnchor(best.ledger)) as unknown as Record< + string, + unknown + >, + ) as unknown as Ledger) : best.ledger; if (me && best.foreign) ledger = stripForeignVolume(ledger); // A FOREIGN winner never DISPLACES this account's own findings — it is @@ -1111,7 +1120,20 @@ export function recoverLedger( ...ledger, ...pickChurnState(bestOwn.ledger), ...(bestOwn.ledger.round === ledger.round - ? pickVolume(bestOwn.ledger as unknown as Record) + ? { + ...pickVolume(bestOwn.ledger as unknown as Record), + // The closures come back under the SAME gate as the volume, for + // the same reason: each entry is stamped `r` = the round that + // minted it, and the compose this recovery feeds reads exactly + // `r === winner's round` off it — own closures from the winner's + // own round are the generation the sentinel needs, own closures + // from any OTHER round are dead bytes, and the foreign winner's + // were stripped above, so nothing foreign enters through the + // restore. A round gap reads as "not recorded", like volume. + ...(bestOwn.ledger.closed === undefined + ? {} + : { closed: bestOwn.ledger.closed }), + } : {}), }; } @@ -1294,7 +1316,10 @@ export function persistedAnchorSha(sideFilePath: string): string | null { * the next review (the healthy foreign-winner path strips it at the * recovery seam for the same reason). A same-round anonymous winner * changes nothing. With no readable file there is nothing to protect, - * and the anonymous recovery is written whole, exactly as before. + * and the anonymous recovery is written whole, exactly as before — + * stamped `anonymousAdoption: true`, the machine-readable record the + * closure mint's honesty leg reads, because `foreign: false` there is + * right for the disclosure caveat but cannot vouch the findings. * * Every write is write-temp-then-rename: a failure mid-write must leave the * previous file intact, never a truncated one that parses as no round and @@ -1418,11 +1443,12 @@ export function persistRecoveredLedger( commitId: _droppedCommitId, ...rest } = existing; - // Both groups through their shared projections, not a second + // All three groups through their shared projections, not a second // hand-kept list: the volume group grew twice and this branch was // updated neither time, and the churn group carries a streak that - // DECIDES a blocker. - const kept = withoutChurn(withoutVolume(rest)); + // DECIDES a blocker. The closures go with them: each is a fact + // about the round this advance leaves behind, exactly like volume. + const kept = withoutClosures(withoutChurn(withoutVolume(rest))); mkdirSync(dirname(sideFilePath), { recursive: true }); writeAtomic( JSON.stringify( @@ -1537,20 +1563,24 @@ export function persistRecoveredLedger( : {}), } : {}; - // The anonymous whole-write sheds BOTH groups. The volume can genuinely - // arrive here — recovery keeps it on an anonymous walk on purpose, since - // "foreign" then means only "this run could not ask who" — and the churn - // cannot, because recovery strips it from every marker when there is no - // `me`. Shedding it anyway costs nothing and makes the seam defend - // itself instead of depending on that upstream invariant holding - // forever: this is the one path where a whole foreign ledger is written - // to the file, so a loosened strip would land a stranger's streak here - // intact and arm the blocker off someone else's count. + // The anonymous whole-write sheds ALL THREE groups. The volume can + // genuinely arrive here — recovery keeps it on an anonymous walk on + // purpose, since "foreign" then means only "this run could not ask + // who" — and the churn and the closures cannot, because recovery + // strips them from every marker when there is no `me`. Shedding them + // anyway costs nothing and makes the seam defend itself instead of + // depending on that upstream invariant holding forever: this is the + // one path where a whole foreign ledger is written to the file, so a + // loosened strip would land a stranger's streak here intact and arm + // the blocker off someone else's count — and a stranger's closure + // lineage here intact, stamped `foreign: false`. const recoveredOut = identityKnown ? recovered.ledger - : (withoutChurn( - withoutVolume( - recovered.ledger as unknown as Record, + : (withoutClosures( + withoutChurn( + withoutVolume( + recovered.ledger as unknown as Record, + ), ), ) as unknown as Ledger); writeAtomic( @@ -1623,6 +1653,16 @@ export function persistRecoveredLedger( (!recovered.foreign && existing?.['merged'] === true && recovered.ledger.findings.length > 0), + // The unverifiable adoption, recorded machine-readably for the + // one consumer the `foreign` rationale above never addressed: + // compose-review's closure mint, which reads that stamp to + // decide whether absence can mean "ruled fixed". An anonymously + // adopted stranger's list carries `foreign: false` — right for + // the caveat — and would walk through the mint as own without + // this flag. Rides ONLY on this branch: it is the one write + // where the adoption happened, and an identity-KNOWN whole + // write replaces the file with a list the union vouched. + ...(!identityKnown ? { anonymousAdoption: true } : {}), }, null, 2, @@ -1821,6 +1861,30 @@ function stripForeignVolume(ledger: Ledger): Ledger { ) as unknown as Ledger; } +/** + * The same record with its closure list removed — ONE statement of the + * scoping decision three seams make for `closed`, on the same discipline the + * churn and volume groups get from `CHURN_FIELDS`/`VOLUME_FIELDS`. + * + * A closure is a ruling-shaped fact about the account whose round minted it: + * what LEFT a work list that account certified, when. Recovery crosses + * accounts, and the field crosses with it — left scoped, a foreign winner's + * closures ride into the side file as this loop's own history, feed the + * divergence sentinel a lineage this loop never produced, and the anonymous + * whole-write stamps them `foreign: false`, laundering the provenance past + * every guard that could still see it. The union restores this account's OWN + * closures beside the volume (same-round gate, same reason), and the two + * anonymous writes in `persistRecoveredLedger` shed the field beside the + * volume and the churn — each entry is a fact about a round those writes no + * longer describe. Absence degrades to silence: the sentinel reads no + * closures exactly as a pre-field marker does. + */ +function withoutClosures>(record: T): T { + const out = { ...record }; + delete out['closed']; + return out; +} + /** * Whether the recovered anchor may scope this round, and the routing that * follows from it — computed here, for the reason `renderLedgerSection` diff --git a/packages/core/src/skills/bundled/review/DESIGN.md b/packages/core/src/skills/bundled/review/DESIGN.md index cc393c1208b..4c0db16a79d 100644 --- a/packages/core/src/skills/bundled/review/DESIGN.md +++ b/packages/core/src/skills/bundled/review/DESIGN.md @@ -538,6 +538,17 @@ So the authoritative ledger now travels in the posted body as an HTML-comment ma Two consequences of those boundaries are worth naming rather than discovering. Ids are **carried, not renumbered**: a still-standing finding is re-reported under the id it already has, that id is written into the comment right after the severity marker, and `buildLedger` reads it back — because a ledger that renumbered by position would key the next round's work list to ids the report riding beside it never used, and `R1-2 names the same claim in every round` is the entire payoff. And own-account recovery means a PR reviewed from **two** accounts — a maintainer locally, a bot in CI — keeps two independent ledgers, each with its own round counter and its own `R2-1`; that is the honest reading of "only this account's reviews can claim what this account stood behind", but it does mean the ids are scoped to the account that wrote them, not to the PR. +## Why the marker carries a closure list (the successor-chain sentinel) + +The ledger tracks ids, carries and supersession, but nothing read the _lineage_. On #9659 the finding counts converged cleanly to one by round 9, then rebounded to 6, 6, 4 — and the rebound was one subsystem: the stop-round blocker-dating chain produced three generations of successor Criticals (R9-1 → R10-2/3/4 → R11-4/R11-6 → R12-1/R12-2), each generation caused by the previous round's fix growing the mechanism. Every finding was individually correct, so every round looked locally normal; recognizing the divergence took a manual round-count analysis and cost four rounds of patch-and-regress. The recurrence cluster says "the file sees findings again"; what it cannot say is "the fix closed one and the mechanism grew another" — that claim needs the _closures_, which no round recorded. + +So the marker now carries one bounded `closed` list: composing round N diffs the recovered work list against the findings it is about to post, and each vanished Critical id is recorded as `{r: N, id, f}` — `fixed` and `superseded` both read as closure, and a positional diff cannot tell them apart, which the advisory note does not need. The sentinel is then a pure walk inside `diagnoseConvergence` (K=2, the current round included): a fresh Critical on a file that closed one at round N-1 AND closes one at round N fires a `successor-chain` recommendation naming the subsystem and the chain (`R9-1 → R10-2 → R11-4`), rendered into the convergence observation the body already carries. On the #9659 shape it fires at round 11, one round before the human analysis caught it. The design choices, each deliberate: + +- **One generation, no carry-forward.** The check reads closures at N (minted while composing) and N-1 (read back off the previous marker), so the marker carries only its own round's mint — older entries are dead bytes. +- **Advisory, never a cap, never the anchor.** A divergence is a property of the code under review, not of what this round read. It sheds ABOVE the anchor pair in the byte-budget cascade — a full-diff re-review costs more than a lost lineage hint — and shedding it never sets `dropped`, because closures certify no range. +- **Thin history stays silent.** Minting closes on claim identity, not id identity (a claim re-posted under a re-minted id still stands), and stays silent wherever absence from the posting set is not a ruling: a previous list with `dropped` set (a missing id may be the budget, not a ruling), a diff-only round, a round that answered "cannot tell" on a Critical, and a pure-foreign previous list. Every marker written before the field existed simply has none. +- **Only Criticals, only real files.** Suggestion churn is not the patch-and-regress signal, and the `(body)`/`(unknown)` stand-ins name no subsystem — the check applies the cluster join's own `k`-flag rule, so a real file spelled like a stand-in still clusters. + ## Why `--resume` continues a run from the harness's records, not a checkpoint file A long local review that dies part-way — a dropped connection, a killed terminal — used to have to start over: on a large PR, minutes of re-fetching, re-chunking and re-launching before the first finding existed again. `--resume` continues it instead, and it stores no new checkpoint state at all, because the persistent checkpoint already existed: the prompt records are the obligations, the harness transcripts are the evidence (each ends with the agent's own final text), the per-round `.findings.md` files are the orchestrator's cumulative state, and the plan's mtime is the run epoch every fence keys on. What was missing was only that nothing read them back.