From add8e6fab61b4a6e95f21782f9df690963538f93 Mon Sep 17 00:00:00 2001 From: wenshao Date: Fri, 21 Aug 2026 09:33:50 +0800 Subject: [PATCH 1/8] feat(review): give the convergence observation a machine-readable half MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The diagnosis could tell a human why a loop was not settling and gave a caller nothing to act on. This adds the two remaining pieces of that work item: matched recommendations as a closed code set, and the round's own report on whether its machinery is working. **`recommendations: [{code, basis}]`** — measurement to advice, with no constants and no decisions. Four codes are matched from facts this round already holds: `root-cause-triage` (files that carried findings before and carry new ones now), `batch-fixes` and `stem-surface` (a trend that is not falling, the latter only where a floor rung is left to take), and `land-and-defer` (a round that posts no Critical — a loop whose blockers are all fixed can end by merging, and a merged pull request cannot diverge). Every entry carries the deterministic fact it was matched from. The set is DERIVED from the diagnosis rather than stored on it, and the paragraph's prose is generated from the same derivation, so the codes a caller wires and the sentences a human reads cannot describe different rounds. It rides the composed result and the durable artifact. The design's menu is larger, and the codes left unimplemented are each absent for a stated reason rather than forgotten — `split` needs the diff-topology test, `reset-drift`/`rescope` need `srcDelta`, `fix-pipeline` needs marker content-hash dedup, `reduce-cadence` is two thresholds, and `re-anchor` was matched to a cause this repository's measurements did not bear out. The last one is now DISCLOSED instead: an anchor chain that has stopped is stated as a fact and prescribes nothing. **Mechanism health** — a pipeline that has stopped and one with nothing to do are both silent, so the round says what it can see about itself. Two checks: a posting floor the reporting reading resolved to critical while the enforcement backstop failed open (the default configuration's standing gap, invisible from either side alone — and deliberately not a count of what posted, since the deterministic `[test]`/`[build]` carve-out is the mechanism working), and two consecutive rounds withholding the incremental anchor (every later round re-reads the whole diff until one closes cleanly). Stated, never acted on. The anchor decision moves to one shared predicate so the disclosure and the marker cannot describe different rounds. Also clears the seven Suggestions deferred through rounds 5-7 of #9461: a dead `floorKnown` parameter and its dead helper, two comments still describing the fold that was narrowed, the drafted-id path missing the length bound `idFor` applies (which let the two ends disagree about one comment over a shortened list), an English cluster clause whose plural form read the new-finding count as another round, a comment opening the wrong validator block, and the merged-provenance wiring's missing end-to-end assertion. Ten mutations, each verified to turn a named test red. --- .../commands/review/compose-review.test.ts | 173 ++++++++++++- .../cli/src/commands/review/compose-review.ts | 235 +++++++++++++----- .../commands/review/lib/convergence.test.ts | 135 +++++++++- .../src/commands/review/lib/convergence.ts | 223 ++++++++++++++++- .../src/commands/review/save-artifact.test.ts | 26 ++ .../cli/src/commands/review/save-artifact.ts | 27 +- 6 files changed, 736 insertions(+), 83 deletions(-) diff --git a/packages/cli/src/commands/review/compose-review.test.ts b/packages/cli/src/commands/review/compose-review.test.ts index 6fcb3384eb6..e5352db1040 100644 --- a/packages/cli/src/commands/review/compose-review.test.ts +++ b/packages/cli/src/commands/review/compose-review.test.ts @@ -9099,7 +9099,7 @@ describe('convergence diagnosis reaches the POSTED body', () => { }); expect(r.body).toContain('Convergence:'); expect(r.body).toContain( - '`src/a.ts` (findings in rounds 2, 4, 2 more now)', + '`src/a.ts` (findings in rounds 2, 4; 2 more now)', ); // An observation, not a gate: the verdict and its caps are untouched. expect(r.cappedBy).not.toContain('convergence'); @@ -9287,7 +9287,7 @@ describe('convergence diagnosis reaches the POSTED body', () => { ], }); expect(r.body).toContain('Convergence:'); - expect(r.body).toContain('findings in round 2, 1 more now'); + expect(r.body).toContain('findings in round 2; 1 more now'); }); it('discloses a work list that was truncated or recovered from elsewhere', () => { @@ -9817,6 +9817,175 @@ describe('convergence diagnosis reaches the POSTED body', () => { expect(parseLedger(r.body)?.floor).toBe('o'); }); + it('carries the matched recommendations on the composed result', () => { + // The machine-readable half: a caller applies ITS policy to these codes + // without parsing prose, and without this module owning a threshold. + sideFile({ + round: 4, + posted: 9, + fresh: 9, + findings: [{ id: 'R2-1', sev: 'S', file: 'src/a.ts', title: 'x' }], + }); + const r = composeReview({ + planPath: plan(), + modelId: 'm', + criticalsInline: 0, + suggestionsInline: 1, + draftedComments: [ + { path: 'src/a.ts', line: 1, body: '**[Suggestion]** again' }, + ], + }); + const codes = (r.recommendations ?? []).map((x) => x.code); + expect(codes).toContain('root-cause-triage'); + // No Critical posts this round, so the ending is available and named. + expect(codes).toContain('land-and-defer'); + expect(r.body).toContain('No Critical finding is open on this round'); + // Every code carries the fact it was matched from. + for (const rec of r.recommendations ?? []) { + expect(rec.basis.length).toBeGreaterThan(0); + } + }); + + it('emits no recommendations on a round that produced no diagnosis', () => { + sideFile({ round: 4, posted: 9, fresh: 9, findings: [] }); + const r = composeReview({ + planPath: plan(), + modelId: 'm', + criticalsInline: 0, + suggestionsInline: 1, + draftedComments: [ + { path: 'src/new.ts', line: 1, body: '**[Suggestion]** unrelated' }, + ], + }); + expect(r.body).not.toContain('Convergence:'); + expect(r.recommendations).toBeUndefined(); + }); + + it('discloses a posture that is engaged in name and not in effect', () => { + // The floor resolved to critical and Suggestion-level findings posted + // inline anyway — a mechanism failure, which is otherwise indis- + // tinguishable from a round with nothing to do. + // The default configuration: the state names no floor, so the reporting + // reading folds to `auto` and resolves critical from round 6 while the + // enforcement backstop — strict on purpose — fails open. + sideFile({ round: 5, posted: 1, fresh: 1, floor: 'c', findings: [] }); + const r = composeReview({ + planPath: plan(), + modelId: 'm', + criticalsInline: 1, + suggestionsInline: 1, + draftedComments: [ + { path: 'a.ts', line: 1, body: '**[Critical]** boom' }, + { path: 'b.ts', line: 2, body: '**[Suggestion]** nit' }, + ], + }); + expect(r.floorEnforced).toEqual([]); + expect(r.body).toContain('Mechanism health:'); + expect(r.body).toContain('engaged in name and not in effect'); + + // With the floor NAMED, both readings agree and nothing is disclosed. + const named = composeReview({ + planPath: plan(), + modelId: 'm', + severityFloor: 'auto', + criticalsInline: 1, + suggestionsInline: 1, + draftedComments: [ + { path: 'a.ts', line: 1, body: '**[Critical]** boom' }, + { path: 'b.ts', line: 2, body: '**[Suggestion]** nit' }, + ], + }); + expect(named.body).not.toContain('engaged in name and not in effect'); + }); + + it('names the merged provenance end to end, not only in the unit', () => { + // The wiring runs pr-context -> side file -> prevLedgerFacts -> the + // rendered caveat, and only the last hop had an assertion. + sideFile({ + round: 4, + posted: 9, + fresh: 9, + foreign: true, + merged: true, + findings: [{ id: 'R2-1', sev: 'S', file: 'src/a.ts', title: 'x' }], + }); + const r = composeReview({ + planPath: plan(), + modelId: 'm', + criticalsInline: 0, + suggestionsInline: 1, + draftedComments: [ + { path: 'src/a.ts', line: 1, body: '**[Suggestion]** again' }, + ], + }); + expect(r.body).toContain("merged over this account's own entries"); + expect(r.body).toContain('so some of those rounds'); + }); + + it('discloses an anchor chain that has stopped', () => { + // Two consecutive withholds mean every later round re-reads the whole + // diff until one closes cleanly — the closed loop measured at 119 + // minutes on a PR whose code had not changed a line. The plan here + // names no fetched sha and the round caps, so this round withholds too. + sideFile({ round: 4, posted: 9, fresh: 9, findings: [] }); + const r = composeReview({ + planPath: plan(), + modelId: 'm', + criticalsInline: 1, + suggestionsInline: 0, + draftedComments: [{ path: 'a.ts', line: 1, body: '**[Critical]** boom' }], + }); + expect(parseLedger(r.body)?.sha).toBeUndefined(); + expect(r.body).toContain('Mechanism health:'); + expect(r.body).toContain('re-reads the whole diff'); + + // A predecessor that DID anchor is a chain that has not stopped. + sideFile({ + round: 4, + posted: 9, + fresh: 9, + sha: 'deadbeef00112233', + findings: [], + }); + const anchored = composeReview({ + planPath: plan(), + modelId: 'm', + criticalsInline: 1, + suggestionsInline: 0, + draftedComments: [{ path: 'a.ts', line: 1, body: '**[Critical]** boom' }], + }); + expect(anchored.body).not.toContain('re-reads the whole diff'); + }); + + it('agrees with the ledger about an out-of-bounds claimed id', () => { + // `idFor` refuses to carry an id the serializer would reject and mints a + // fresh one. Read as a re-post here, the marker's own work list would + // gain a round-N entry that entered no fresh count — one end calling a + // comment carried while the other calls it new. The list is SHORTENED + // on purpose: over a whole one the stray-id rescue already reaches this + // draft, so the bound is what carries the case here. + const long = `R2-${'9'.repeat(24)}`; + sideFile({ + round: 4, + posted: 9, + fresh: 9, + dropped: 3, + findings: [{ id: 'R2-1', sev: 'S', file: 'src/a.ts', title: 'x' }], + }); + const r = composeReview({ + planPath: plan(), + modelId: 'm', + criticalsInline: 1, + suggestionsInline: 0, + draftedComments: [ + { path: 'src/a.ts', line: 1, body: `**[Critical]** ${long}: boom` }, + ], + }); + expect(parseLedger(r.body)?.findings.map((x) => x.id)).toEqual(['R5-1']); + expect(r.postedFresh).toBe(1); + expect(parseLedger(r.body)?.fresh).toBe(1); + }); + it('names an auto-resolved floor the way the enforcement note does', () => { // `auto` is the DEFAULT, so the explicit-flag wording claims a flag that // was never passed — beside a floor-enforcement note in the same body diff --git a/packages/cli/src/commands/review/compose-review.ts b/packages/cli/src/commands/review/compose-review.ts index 6d153581b20..75f44a290b2 100644 --- a/packages/cli/src/commands/review/compose-review.ts +++ b/packages/cli/src/commands/review/compose-review.ts @@ -88,7 +88,10 @@ import { mdField } from './lib/md-field.js'; import { diagnoseConvergence, isFreshDraft, + recommendationsFor, renderConvergenceDiagnosis, + renderMechanismHealth, + type Recommendation, type CriticalFloorKind, type DraftedFinding, type PrevRound, @@ -450,8 +453,8 @@ export function criticalFloorKind( // module had to guess at is the direction that loses work; the fail-open // there is pre-existing and stays. const raw = normalizeSeverityFloor(severityFloor); - // Only genuine ABSENCE folds. A present-but-unrecognisable value is a - // state this module cannot read, and folding it made the body contradict + // Only genuine ABSENCE folds — a present-but-unrecognisable value is a + // state this module cannot read, and folding THAT made the body contradict // itself: the volume advice said the round "already resolves to a critical // posting floor" while the deferral-licence clause in the same body said // the floor carried no recognisable value and the enforcement backstop — @@ -505,12 +508,6 @@ function floorResolvesCritical( return undefined; } -/** Did the state name a floor this module recognises at all? */ -function severityFloorKnown(severityFloor: unknown): boolean { - const raw = normalizeSeverityFloor(severityFloor); - return raw === 'critical' || raw === 'suggestion' || raw === 'auto'; -} - /** * The posting floor, enforced in code — the backstop for the posture SKILL * Step 6 resolves in prose. @@ -839,6 +836,17 @@ export interface ComposeReviewResult { * from the side file has no other copy anywhere. */ convergence?: { en: string; zh: string }; + /** + * The handling recommendations this round's diagnosis matched, as a closed + * code set with the deterministic fact each was matched from. + * + * The machine-readable half of the observation, and the point of the whole + * advisory: a caller applies ITS policy to these — stop the automatic + * loop, hand to a human, open a follow-up issue — without parsing prose, + * and without this module owning a threshold or a decision. Absent when no + * signal fired, exactly like the paragraph. + */ + recommendations?: Recommendation[]; /** * The previous round's `postedInline`, recovered from the side file when * it recorded one. Absent on round 1, on a recovery miss, and on any @@ -1350,6 +1358,8 @@ export function composeReview( findings: prevFacts.findings, truncated: prevFacts.truncated, complete: prevRound > 0 && !prevFacts.truncated, + round: prevRound, + anchored: prevFacts.anchored, foreign: prevFacts.foreign, merged: prevFacts.merged, ...(prevFacts.floor === undefined ? {} : { floor: prevFacts.floor }), @@ -1361,6 +1371,11 @@ export function composeReview( // enforcement note in the same body contradicts. floor: floorKind === undefined ? ('o' as const) : ('c' as const), ...(floorKind === undefined ? {} : { criticalFloorKind: floorKind }), + floorEnforcementEngaged: criticalFloorInEffect( + input.severityFloor, + input.contextUnavailable === true, + prevRound, + ), }, ); // The ledger marker rides the body THIS function returns, because this — not @@ -1393,7 +1408,6 @@ export function composeReview( result.postedFresh, prevFacts.posted, floorKind, - severityFloorKnown(input.severityFloor), { ids: new Set(prevFacts.findings.map((f) => f.id)), // A round that recovered NO predecessor knows nothing about which ids @@ -1428,6 +1442,7 @@ const EMPTY_PREV_FACTS = { truncated: false, foreign: false, merged: false, + anchored: false, }; /** @@ -1467,6 +1482,8 @@ function prevLedgerFacts(planPath: string | undefined): { floor?: 'c' | 'o'; /** How many of its comments were findings reported for the first time. */ fresh?: number; + /** Whether it carried an incremental anchor at all. */ + anchored: boolean; } { try { if (!planPath) return EMPTY_PREV_FACTS; @@ -1547,6 +1564,10 @@ function prevLedgerFacts(planPath: string | undefined): { // rendering says so rather than publishing the citation bare. foreign: round !== 0 && prev.foreign === true, merged: round !== 0 && prev.merged === true, + // The previous round's anchor, as a yes/no. Two consecutive withholds + // are the shape the self-check discloses; the sha itself is Step 1's + // business, not this read's. + anchored: round !== 0 && typeof prev.sha === 'string' && prev.sha !== '', // Travels with the volume it qualifies, and with the round, for the // same reason both of those do. ...(round === 0 || @@ -1568,6 +1589,29 @@ function prevLedgerFacts(planPath: string | undefined): { } } +/** + * Does this round withhold the incremental anchor? + * + * The ONE statement of that decision. The marker acts on it; the + * mechanism-health self-check READS it, because two consecutive withholds + * mean the next round re-reads the whole diff and the round after that — + * the closed loop measured at 119 minutes and 34M tokens on a PR whose code + * had not changed a line. A restatement in the self-check would let the + * disclosure describe a round the marker anchored, or stay silent on one it + * did not. + */ +export function anchorFailsClosed( + cappedBy: string[], + scopeUnproven: boolean, + dimensionGapsAreDepthOnly: boolean, +): boolean { + return ( + scopeUnproven || + !dimensionGapsAreDepthOnly || + cappedBy.some((cap) => cap !== 'unreviewed-dimension') + ); +} + /** * 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 @@ -1585,7 +1629,6 @@ function ledgerMarkerFor( freshInline: number, prevPostedInline: number | undefined, floorKind: CriticalFloorKind | undefined, - floorKnown: boolean, carriedWorkList: { ids: ReadonlySet; complete: boolean }, ): string | null { try { @@ -1624,10 +1667,11 @@ function ledgerMarkerFor( // only claim is about lines. When the machine coverage evidence does show // doubt about the reading itself, `scopeUnproven` carries it here and the // anchor is withheld exactly as before. - const failClosed = - scopeUnproven || - !dimensionGapsAreDepthOnly || - cappedBy.some((cap) => cap !== 'unreviewed-dimension'); + const failClosed = anchorFailsClosed( + cappedBy, + scopeUnproven, + dimensionGapsAreDepthOnly, + ); const shaCandidate = !failClosed && typeof plan.fetchedSha === 'string' ? plan.fetchedSha @@ -1727,9 +1771,10 @@ function ledgerMarkerFor( // critical floor and the volume under an open one are not two points // on one trend. Decides nothing, sheds with the volume it qualifies. // The RESOLVED posture, folded the way every consumer folds it: an - // absent or unrecognisable floor reads as `auto` throughout this - // module, and `auto` resolves determinately from the round number and - // the context state. Recording it only when the state NAMED a floor + // ABSENT floor reads as `auto` throughout this module (a present but + // unrecognisable one reads as nothing at all — see + // `criticalFloorKind`), and `auto` resolves determinately from the + // round number and the context state. Recording it only when the state NAMED a floor // left the guard blind under the DEFAULT configuration — where the // posture genuinely transitions at round 6 and again on a transient // context failure — so a real posture change read as loop divergence, @@ -1833,6 +1878,14 @@ function composeReviewBody( prev: PrevRound; floor?: 'c' | 'o'; criticalFloorKind?: CriticalFloorKind; + /** + * Whether the CODE backstop enforces the floor this round reports. The + * two readings differ by one thing — the reporting one folds an absent + * floor to `auto` and the enforcement one does not — so under the + * default configuration the prose posture engages while the backstop + * fails open. That gap is a mechanism fact, not a loop fact. + */ + floorEnforcementEngaged?: boolean; } | null = null, ): ComposeReviewResult { // The posting set this body describes — `input` here is already the @@ -1845,51 +1898,6 @@ function composeReviewBody( // the shared reader's own docstring exists to prevent. `?? 0` is // unreachable for an array length; it keeps the type honest. const postedInline = volumeOf((input.draftedComments ?? []).length) ?? 0; - const diagnosis = convergence - ? diagnoseConvergence({ - // Clamped like every other public round surface in this function — - // the ledger marker stamp and the deferred-posture clause both clamp - // identically. An unclamped `+1` at the cap names round 10001 in the - // posted prose beside a marker stamping 10000, with this round's own - // findings stamped `R10000-*`. - round: Math.min(prevRound + 1, LEDGER_MAX_ROUND), - // The SAME count the marker and the VOLUME line carry, not a second - // derivation of it. - posted: postedInline, - prev: convergence.prev, - drafts: draftedFindingsOf(input.draftedComments), - ...(convergence.floor === undefined - ? {} - : { floor: convergence.floor }), - ...(convergence.criticalFloorKind === undefined - ? {} - : { criticalFloorKind: convergence.criticalFloorKind }), - }) - : null; - // A fact about the round, not about the diagnosis: it rides in the marker - // whether or not a signal fired, because the NEXT round's trend needs this - // round's point either way. - const carriedIds = convergence - ? new Set( - convergence.prev.findings - .map((f) => f?.id) - .filter((id): id is string => typeof id === 'string'), - ) - : undefined; - const postedFresh = - volumeOf( - draftedFindingsOf(input.draftedComments).filter((d) => - isFreshDraft( - d, - Math.min(prevRound + 1, LEDGER_MAX_ROUND), - carriedIds, - convergence?.prev.complete === true, - ), - ).length, - ) ?? 0; - const convergenceNote = diagnosis - ? renderConvergenceDiagnosis(diagnosis) - : undefined; const criticalsInline = toCount(input.criticalsInline, 'criticalsInline'); const suggestionsInline = toCount( input.suggestionsInline, @@ -2621,13 +2629,69 @@ function composeReviewBody( } } + // `C` — every Critical this review posts anywhere, inline or body. Named + // here because two consumers need it: the verdict below, and the + // convergence diagnosis, whose `land-and-defer` recommendation turns on + // exactly this fact. Two derivations of one count is the drift class this + // file's header exists to prevent — and it is computed HERE, after the + // last `bodyCriticals.push`, because the stray-marker leg and the + // script-lint gate both add blockers after the list is declared. + const openCriticals = criticalsInline + bodyCriticals.length; + const diagnosis = convergence + ? diagnoseConvergence({ + // Clamped like every other public round surface in this function — + // the ledger marker stamp and the deferred-posture clause both clamp + // identically. An unclamped `+1` at the cap names round 10001 in the + // posted prose beside a marker stamping 10000, with this round's own + // findings stamped `R10000-*`. + round: Math.min(prevRound + 1, LEDGER_MAX_ROUND), + // The SAME count the marker and the VOLUME line carry, not a second + // derivation of it. + posted: postedInline, + prev: convergence.prev, + drafts: draftedFindingsOf(input.draftedComments), + ...(convergence.floor === undefined + ? {} + : { floor: convergence.floor }), + ...(convergence.criticalFloorKind === undefined + ? {} + : { criticalFloorKind: convergence.criticalFloorKind }), + openCriticals, + }) + : null; + // A fact about the round, not about the diagnosis: it rides in the marker + // whether or not a signal fired, because the NEXT round's trend needs this + // round's point either way. + const carriedIds = convergence + ? new Set( + convergence.prev.findings + .map((f) => f?.id) + .filter((id): id is string => typeof id === 'string'), + ) + : undefined; + const postedFresh = + volumeOf( + draftedFindingsOf(input.draftedComments).filter((d) => + isFreshDraft( + d, + Math.min(prevRound + 1, LEDGER_MAX_ROUND), + carriedIds, + convergence?.prev.complete === true, + ), + ).length, + ) ?? 0; + const convergenceNote = diagnosis + ? renderConvergenceDiagnosis(diagnosis) + : undefined; + const recommendations = diagnosis ? recommendationsFor(diagnosis) : undefined; + // `C` counts every Critical the review posts anywhere — inline or body. // `S` counts every *confirmed* Suggestion — anchored, discarded, or dropped // as an already-reported duplicate: the verdict reflects the findings the // review confirmed, not the ones that anchored or were worth re-posting, so // neither dropping every anchor nor every duplicate may upgrade the event // to APPROVE. - const c = criticalsInline + bodyCriticals.length; + const c = openCriticals; const s = suggestionsInline + suggestionsDiscarded + @@ -3663,6 +3727,34 @@ function composeReviewBody( // "deferred-findings list" that never existed and point the author at // artifact entries that do not exist. Its own rank names itself, carries // no artifact pointer, and leaves `deferralList` false. + // Is the MECHANISM working? A pipeline that has stopped and one with + // nothing to do are both silent, so the round says what it can see about + // its own machinery. Computed here, after the caps are final: the anchor + // decision reads them, and `cappedBy` is still being appended to well + // below where the diagnosis is composed. + const healthNote = convergence + ? renderMechanismHealth({ + // Nominally engaged, mechanically not: the floor resolved to + // critical and Suggestion-level findings posted inline anyway. + // The REPORTING reading resolved the floor to critical and the + // enforcement backstop did not — the two differ only in folding an + // absent floor to `auto`, so this is the default configuration's + // standing gap, and it is invisible from either side alone. Not a + // count of what posted: `floorEnforcedReroute` deliberately leaves + // deterministic `[test]`/`[build]` findings inline at any floor, and + // reading those as a malfunction would flag the carve-out working. + postureNotEngaging: + convergence.criticalFloorKind !== undefined && + convergence.floorEnforcementEngaged === false, + // Two consecutive withholds — this round's decision read through the + // marker's OWN predicate, and the recovered round's recorded anchor. + anchorChainBroken: + !convergence.prev.anchored && + (convergence.prev.round ?? 0) > 0 && + anchorFailsClosed(cappedBy, scopeUnproven, dimensionGapsAreDepthOnly), + }) + : null; + const healthBlock: Bi[] = healthNote ? [{ ...healthNote, trim: 0 }] : []; const convergenceBlock: Bi[] = convergenceNote ? [{ ...convergenceNote, trim: 0 }] : []; @@ -3698,6 +3790,7 @@ function composeReviewBody( ...unlicensedDeferralBlock, ...deferredSuggestionsBlock, ...convergenceBlock, + ...healthBlock, ...continuityBlock, ...bodyCriticalBlock, ]; @@ -3719,6 +3812,7 @@ function composeReviewBody( ...(convergenceNote === undefined ? {} : { convergence: convergenceNote }), + ...(recommendations === undefined ? {} : { recommendations }), bodyTrim, lowSignal, scopeUnproven, @@ -3756,6 +3850,8 @@ function composeReviewBody( ...unlicensedDeferralBlock, ...deferredSuggestionsBlock, ...convergenceBlock, + ...healthBlock, + ...healthBlock, ...continuityBlock, ], notReviewedParts.length || @@ -3771,6 +3867,7 @@ function composeReviewBody( // right only because another rule makes its input impossible is a // trap for whoever changes that other rule. convergenceBlock.length || + healthBlock.length || continuityBlock.length ? '\n\n' : ' ', @@ -3790,6 +3887,7 @@ function composeReviewBody( ...(convergenceNote === undefined ? {} : { convergence: convergenceNote }), + ...(recommendations === undefined ? {} : { recommendations }), bodyTrim, lowSignal, scopeUnproven, @@ -3961,6 +4059,7 @@ function composeReviewBody( // 6f. Convergence observation (non-capping) — is this loop settling, and if // not, what shape is it. About the review HISTORY, not the diff. clauses.push(...convergenceBlock); + clauses.push(...healthBlock); // 6g. Resumed-run continuity (non-capping) — reused work that COUNTS as // reviewed, disclosed so the author knows two attempts fed this verdict. @@ -4021,6 +4120,7 @@ function composeReviewBody( postedInline, postedFresh, ...(convergenceNote === undefined ? {} : { convergence: convergenceNote }), + ...(recommendations === undefined ? {} : { recommendations }), bodyTrim, lowSignal, scopeUnproven, @@ -4811,7 +4911,14 @@ function draftedFindingsOf(drafted: unknown): DraftedFinding[] { for (const c of drafted as Array<{ path?: unknown; body?: unknown }>) { if (severityOf(c) === null) continue; const { id } = readClaim(ledgerClaimLine(c.body)); - const carried = id !== undefined && !seen.has(id) ? id : undefined; + // The same length bound `idFor` applies before it will carry an id: an + // id the serializer refuses is one no work list can hold, so treating it + // as a re-post here would call a finding carried that the ledger mints + // fresh — the two ends disagreeing about one comment. + const usable = + id !== undefined && id.length <= LEDGER_MAX_ID ? id : undefined; + const carried = + usable !== undefined && !seen.has(usable) ? usable : undefined; if (carried !== undefined) seen.add(carried); out.push({ file: typeof c.path === 'string' ? c.path : '', diff --git a/packages/cli/src/commands/review/lib/convergence.test.ts b/packages/cli/src/commands/review/lib/convergence.test.ts index c6316353c6f..142f98d2588 100644 --- a/packages/cli/src/commands/review/lib/convergence.test.ts +++ b/packages/cli/src/commands/review/lib/convergence.test.ts @@ -7,7 +7,9 @@ import { describe, it, expect } from 'vitest'; import { diagnoseConvergence, + recommendationsFor, renderConvergenceDiagnosis, + renderMechanismHealth, MAX_RENDERED_CLUSTERS, type ConvergenceDiagnosis, type DraftedFinding, @@ -647,6 +649,133 @@ describe('diagnoseConvergence — the trigger table', () => { }); }); +describe('recommendationsFor — measurement to advice, no constants', () => { + const base: ConvergenceDiagnosis = { + round: 6, + posted: 4, + fresh: 2, + prevPosted: 4, + prevFresh: 2, + clusters: [{ file: 'src/a.ts', priorRounds: [3, 5], thisRound: 2 }], + volumeNotShrinking: true, + truncatedEvidence: false, + foreignEvidence: false, + mergedEvidence: false, + }; + + it('matches each code to the fact it names, and names it', () => { + const r = recommendationsFor(base); + expect(r.map((x) => x.code)).toEqual([ + 'root-cause-triage', + 'batch-fixes', + 'stem-surface', + ]); + // Every basis is a deterministic fact, not a judgement. + expect(r[0].basis).toContain('src/a.ts'); + expect(r[1].basis).toContain('round 6 produced 2 first-time finding(s)'); + expect(r[2].basis).toContain('did not resolve to critical'); + }); + + it('offers the floor rung only where a rung is left to take', () => { + const atFloor = recommendationsFor({ + ...base, + criticalFloorKind: 'explicit', + }); + expect(atFloor.map((x) => x.code)).not.toContain('stem-surface'); + expect(atFloor.map((x) => x.code)).toContain('batch-fixes'); + }); + + it('matches land-and-defer only on a round with no open blocker', () => { + expect( + recommendationsFor({ ...base, openCriticals: 0 }).map((x) => x.code), + ).toContain('land-and-defer'); + expect( + recommendationsFor({ ...base, openCriticals: 2 }).map((x) => x.code), + ).not.toContain('land-and-defer'); + // Absent is not zero: an unrecorded count is not a count of none. + expect(recommendationsFor(base).map((x) => x.code)).not.toContain( + 'land-and-defer', + ); + }); + + it('matches nothing a signal did not fire', () => { + const volumeOnly = recommendationsFor({ + ...base, + clusters: [], + }); + expect(volumeOnly.map((x) => x.code)).not.toContain('root-cause-triage'); + const clusterOnly = recommendationsFor({ + ...base, + volumeNotShrinking: false, + }); + expect(clusterOnly.map((x) => x.code)).toEqual(['root-cause-triage']); + }); + + it('is what the paragraph renders, not a second list beside it', () => { + // Derived rather than stored, so the codes a caller wires and the prose + // a human reads cannot describe different rounds. + const withLand = { ...base, openCriticals: 0 }; + const prose = renderConvergenceDiagnosis(withLand); + expect(prose.en).toContain('shared root cause'); + expect(prose.en).toContain('Batching the remaining fixes'); + expect(prose.en).toContain('--severity-floor critical'); + expect(prose.en).toContain('No Critical finding is open on this round'); + expect(prose.zh).toContain('本轮没有未决的 Critical'); + // ...and the narrowed floor case drops exactly the rung it dropped. + const atFloor = renderConvergenceDiagnosis({ + ...base, + clusters: [], + criticalFloorKind: 'explicit', + }); + expect(atFloor.en).not.toContain('dropping this PR'); + }); +}); + +describe('renderMechanismHealth — is the machinery working', () => { + it('says nothing when nothing is wrong with it', () => { + expect( + renderMechanismHealth({ + postureNotEngaging: false, + anchorChainBroken: false, + }), + ).toBeNull(); + }); + + it('states a posture that is engaged in name and not in effect', () => { + const r = renderMechanismHealth({ + postureNotEngaging: true, + anchorChainBroken: false, + })!; + expect(r.en).toContain('engaged in name and not in effect'); + expect(r.zh).toContain('名义上生效、实际未生效'); + // Stated, never prescribed. + expect(r.en).toContain('Stated, not acted on'); + expect(r.en).not.toMatch(/should |must |re-anchor/i); + }); + + it('states an anchor chain that has stopped', () => { + const r = renderMechanismHealth({ + postureNotEngaging: false, + anchorChainBroken: true, + })!; + expect(r.en).toContain('re-reads the whole diff'); + expect(r.zh).toContain('重读整个 diff'); + // The design once prescribed a re-anchor round here; the measurements + // did not bear out its premise, so the shape is disclosed and nothing + // is recommended. + expect(r.en).not.toContain('raise'); + }); + + it('states both when both hold', () => { + const r = renderMechanismHealth({ + postureNotEngaging: true, + anchorChainBroken: true, + })!; + expect(r.en).toContain('engaged in name'); + expect(r.en).toContain('re-reads the whole diff'); + }); +}); + describe('renderConvergenceDiagnosis — what the author reads', () => { const base: ConvergenceDiagnosis = { round: 6, @@ -667,7 +796,7 @@ describe('renderConvergenceDiagnosis — what the author reads', () => { 'round 6 posted 4 inline comment(s), 2 of them reported for the first time', ); expect(r.en).toContain('the previous round posted 4'); - expect(r.en).toContain('`src/a.ts` (findings in rounds 3, 5, 2 more now)'); + expect(r.en).toContain('`src/a.ts` (findings in rounds 3, 5; 2 more now)'); expect(r.zh).toContain('第 6 轮发布了 4 条行内评论,其中 2 条是首次提出'); expect(r.zh).toContain('第 3、5 轮已出过发现,本轮又有 2 条'); }); @@ -680,8 +809,8 @@ describe('renderConvergenceDiagnosis — what the author reads', () => { ...base, clusters: [{ file: 'src/a.ts', priorRounds: [4], thisRound: 1 }], }); - expect(one.en).toContain('`src/a.ts` (findings in round 4, 1 more now)'); - expect(one.en).not.toContain('in rounds 4,'); + expect(one.en).toContain('`src/a.ts` (findings in round 4; 1 more now)'); + expect(one.en).not.toContain('in rounds 4;'); }); it('says the observation withheld nothing — scoped to the observation', () => { diff --git a/packages/cli/src/commands/review/lib/convergence.ts b/packages/cli/src/commands/review/lib/convergence.ts index f541355796f..a956fe72b99 100644 --- a/packages/cli/src/commands/review/lib/convergence.ts +++ b/packages/cli/src/commands/review/lib/convergence.ts @@ -129,6 +129,14 @@ export interface PrevRound { * The number the trend is about — see `fresh` on the diagnosis. */ fresh?: number; + /** Its own round number; 0 when nothing was recovered. */ + round?: number; + /** + * Whether it carried an incremental anchor. Read only by the + * mechanism-health check: two consecutive withholds mean every later round + * re-reads the whole diff until one closes cleanly. + */ + anchored?: boolean; } export interface ConvergenceDiagnosis { @@ -165,11 +173,83 @@ export interface ConvergenceDiagnosis { * unavailable, which an unconditional-sounding claim would misstate. */ criticalFloorKind?: CriticalFloorKind; + /** + * Blockers THIS round posts — inline plus body. A fact about the round + * being composed, not about the recovered list: a Critical in the previous + * work list this round does not re-post was fixed. + */ + openCriticals?: number; } /** How a round's posting floor came to be `critical`. */ export type CriticalFloorKind = 'explicit' | 'auto-resolved'; +/** + * The closed set of handling recommendations this module can match. + * + * Closed on purpose: a caller wires actions to these codes without parsing + * prose, so the vocabulary is a contract. Matching is measurement → advice, + * with zero constants and zero decisions — every entry carries the factual + * basis it was matched from, and none of them is a claim about how the code + * should be restructured. + * + * The design's menu is larger than this. The codes NOT emitted here are the + * ones whose evidence this round does not hold, and each is absent for a + * stated reason rather than forgotten: + * + * - `split` needs the diff-topology test (a separate work item); this module + * can see that a cluster recurs, not that its hunks are separable. + * - `reset-drift` / `rescope` need `srcDelta`, which is gated on the anchor + * write-side work. + * - `fix-pipeline` needs marker content-hash dedup to tell a repost storm + * from real volume. + * - `reduce-cadence` is matched to "many rounds, small per-round increments", + * and both halves are thresholds — the one thing this module does not own. + * Its threshold-free reading ("healthy but oversampled") is also a round + * that produces no diagnosis at all, so there is no paragraph to carry it. + * - `re-anchor` was matched to an anchorless chain on the premise that agent + * budget caps dominate it. Measured on this repository the dominant causes + * were a non-converged reverse audit and skipped integration tests, which + * one raised-budget round does not clear — so the chain is DISCLOSED below + * as mechanism health and prescribes nothing. + */ +export type RecommendationCode = + | 'root-cause-triage' + | 'land-and-defer' + | 'batch-fixes' + | 'stem-surface'; + +/** One matched recommendation and the measurement that matched it. */ +export interface Recommendation { + code: RecommendationCode; + /** The deterministic fact this was matched from — never a judgement. */ + basis: string; +} + +/** + * What the round can say about the MECHANISM, as opposed to about the loop. + * + * A pipeline that has stopped working is indistinguishable from one with + * nothing to do: both are silent. These are the shapes where the round can + * see its own machinery failing, and they are stated as facts with no + * prescription attached. + */ +export interface MechanismHealth { + /** + * The floor resolved to `critical`, and Suggestions posted inline anyway. + * The posture is nominally engaged and mechanically is not. + */ + postureNotEngaging: boolean; + /** + * This round withholds the incremental anchor and the round it recovered + * had none either. Two consecutive withholds mean the next round re-reads + * the whole diff, and the round after that, until something clears it — + * the closed loop measured at 119 minutes and 34M tokens on a PR whose + * code had not changed a line. + */ + anchorChainBroken: boolean; +} + /** * Is this draft a finding reported for the FIRST time? * @@ -279,6 +359,13 @@ export function diagnoseConvergence(input: { */ floor?: 'c' | 'o'; criticalFloorKind?: CriticalFloorKind; + /** + * Blockers THIS round posts — inline plus body. The one fact + * `land-and-defer` turns on, and it is a fact about the round being + * composed rather than about the recovered list: a Critical in the + * previous work list this round does not re-post was fixed. + */ + openCriticals?: number; }): ConvergenceDiagnosis | null { const priorByFile = new Map>(); for (const f of input.prev.findings) { @@ -432,7 +519,11 @@ export function diagnoseConvergence(input: { fresh.length >= input.prev.fresh; if (clusters.length === 0 && !volumeNotShrinking) return null; + return { + ...(input.openCriticals === undefined + ? {} + : { openCriticals: input.openCriticals }), round: input.round, posted: input.posted, fresh: fresh.length, @@ -451,6 +542,91 @@ export function diagnoseConvergence(input: { }; } +/** + * The handling recommendations this diagnosis matches — measurement to + * advice, with zero constants and zero decisions. + * + * DERIVED from the diagnosis rather than stored on it. Carried as a field, + * the same round would have two representations of one thing, and a caller + * (or a test) could hold a diagnosis whose codes and whose facts describe + * different rounds. Derived, the paragraph a human reads and the codes a + * caller wires cannot disagree, because there is only one of them. + */ +export function recommendationsFor(d: ConvergenceDiagnosis): Recommendation[] { + const out: Recommendation[] = []; + if (d.clusters.length > 0) { + const shown = d.clusters.slice(0, MAX_RENDERED_CLUSTERS).map((c) => c.file); + out.push({ + code: 'root-cause-triage', + 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.volumeNotShrinking) { + out.push({ + code: 'batch-fixes', + basis: `round ${d.round} produced ${d.fresh} first-time finding(s); the previous round produced ${d.prevFresh}`, + }); + // The floor rung is offered only where there is a rung left to take. + if (d.criticalFloorKind === undefined) { + out.push({ + code: 'stem-surface', + basis: `the posting floor for this round did not resolve to critical`, + }); + } + } + // Decidable, and decidable ONLY from this round: a loop whose blockers are + // all fixed can end by merging, and a merged pull request cannot diverge + // further. Absent `openCriticals` is not zero — an unrecorded count is not + // a count of none. + if (d.openCriticals === 0) { + out.push({ + code: 'land-and-defer', + basis: `this round posts no Critical finding(s)`, + }); + } + return out; +} + +/** + * The mechanism-health disclosure, or null when nothing is wrong with the + * machinery itself. + * + * Separate from the loop reading on purpose. A diverging loop is a fact + * about the WORK; these are facts about the pipeline, and they are the + * shapes where a failure is otherwise indistinguishable from having nothing + * to do — both are silent. Stated, never prescribed: what to do about a + * posture that is not engaging, or an anchor chain that has stopped, is the + * operator's call, and the one prescription the design once carried here + * (`re-anchor`) was matched to a cause the measurements did not bear out. + */ +export function renderMechanismHealth( + h: MechanismHealth, +): { en: string; zh: string } | null { + const en: string[] = []; + const zh: string[] = []; + if (h.postureNotEngaging) { + en.push( + `the posting floor for this round resolved to critical, and Suggestion-level findings posted inline anyway — the posture is engaged in name and not in effect`, + ); + zh.push( + `本轮的发布下限解析为 critical,但仍有 Suggestion 级发现以行内评论发布——该姿态名义上生效、实际未生效`, + ); + } + if (h.anchorChainBroken) { + en.push( + `this round withholds the incremental anchor and the round it recovered had none either, so the next review re-reads the whole diff — and will keep doing so until a round closes cleanly`, + ); + zh.push( + `本轮扣留了增量锚点,而它恢复到的那一轮也没有锚点,因此下一次评审将重读整个 diff——并会一直如此,直到某一轮干净收尾`, + ); + } + if (en.length === 0) return null; + return { + en: `Mechanism health: ${en.join('; ')}. (Stated, not acted on — this changes nothing about what the round posts.)`, + zh: `机制健康:${zh.join(';')}。(仅陈述,不据此行动——这不改变本轮发布的任何内容。)`, + }; +} + /** How many clusters the rendered paragraph names before summarising. */ export const MAX_RENDERED_CLUSTERS = 3; @@ -481,7 +657,7 @@ export function renderConvergenceDiagnosis(d: ConvergenceDiagnosis): { const clusterEn = shown .map( (c) => - `${mdField(c.file)} (findings in round${c.priorRounds.length > 1 ? 's' : ''} ${c.priorRounds.join(', ')}, ${c.thisRound} more now)`, + `${mdField(c.file)} (findings in round${c.priorRounds.length > 1 ? 's' : ''} ${c.priorRounds.join(', ')}; ${c.thisRound} more now)`, ) .join('; '); const clusterZh = shown @@ -594,6 +770,9 @@ export function renderConvergenceDiagnosis(d: ConvergenceDiagnosis): { // checked. And it names the posture the way that round actually got it — // `auto` is the DEFAULT, so wording an auto-resolved floor as an explicit // setting claims a flag nobody passed. + const matched = recommendationsFor(d); + const has = (code: RecommendationCode): boolean => + matched.some((r) => r.code === code); const batchEn = `Batching the remaining fixes and verifying them before the next push`; const batchZh = `把剩余修复攒成一批、验证后再推送`; const alreadyEn: Record = { @@ -604,26 +783,44 @@ export function renderConvergenceDiagnosis(d: ConvergenceDiagnosis): { explicit: `本 PR 的评审已处于 \`--severity-floor critical\``, 'auto-resolved': `本 PR 的评审已解析为 critical 发布下限`, }; - const floorEn = - d.criticalFloorKind === undefined - ? `${batchEn}, or dropping this PR's reviews to \`--severity-floor critical\`, keeps the loop from re-deriving the same set.` - : `${batchEn} keeps the loop from re-deriving the same set; ${alreadyEn[d.criticalFloorKind]}.`; - const floorZh = - d.criticalFloorKind === undefined - ? `${batchZh},或将本 PR 的评审降到 \`--severity-floor critical\`,可以避免循环反复推导同一组发现。` - : `${batchZh},可以避免循环反复推导同一组发现;${alreadyZh[d.criticalFloorKind]}。`; + // The floor rung rides the batching sentence when it was MATCHED — the + // same condition, read off the set rather than re-derived from the flag. + const stem = has('stem-surface'); + const floorEn = stem + ? `${batchEn}, or dropping this PR's reviews to \`--severity-floor critical\`, keeps the loop from re-deriving the same set.` + : `${batchEn} keeps the loop from re-deriving the same set${ + d.criticalFloorKind === undefined + ? '' + : `; ${alreadyEn[d.criticalFloorKind]}` + }.`; + const floorZh = stem + ? `${batchZh},或将本 PR 的评审降到 \`--severity-floor critical\`,可以避免循环反复推导同一组发现。` + : `${batchZh},可以避免循环反复推导同一组发现${ + d.criticalFloorKind === undefined + ? '' + : `;${alreadyZh[d.criticalFloorKind]}` + }。`; 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 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 不会继续发散。`; + + // The prose is generated FROM the matched set, not beside it. Two lists + // would let the paragraph a human reads and the codes a caller wires + // describe different rounds — and this module's whole claim is that its + // advice is matched to what it measured. const adviceEn = [ - d.clusters.length > 0 ? clusterAdviceEn : null, - d.volumeNotShrinking ? floorEn : null, + has('root-cause-triage') ? clusterAdviceEn : null, + has('batch-fixes') ? floorEn : null, + has('land-and-defer') ? landEn : null, ] .filter(Boolean) .join(' '); const adviceZh = [ - d.clusters.length > 0 ? clusterAdviceZh : null, - d.volumeNotShrinking ? floorZh : null, + has('root-cause-triage') ? clusterAdviceZh : null, + has('batch-fixes') ? floorZh : null, + has('land-and-defer') ? landZh : null, ] .filter(Boolean) .join(''); diff --git a/packages/cli/src/commands/review/save-artifact.test.ts b/packages/cli/src/commands/review/save-artifact.test.ts index 5dc01980290..c6f66116a07 100644 --- a/packages/cli/src/commands/review/save-artifact.test.ts +++ b/packages/cli/src/commands/review/save-artifact.test.ts @@ -467,6 +467,32 @@ describe('saveReviewArtifact', () => { expect(saved.verdict.convergence.zh).toBe('收敛情况:…'); }); + it('carries the matched recommendation codes into the artifact', () => { + // The machine-readable half. Dropped by the allow-list, a caller reading + // the durable record sees the prose and not the codes it would key on. + const paths = fixture(); + writeJson(paths.composed, { + ...verdict, + recommendations: [ + { code: 'root-cause-triage', basis: '2 file(s) …' }, + { code: 'land-and-defer', basis: 'this round posts no Critical …' }, + ], + }); + saveReviewArtifact({ ...paths, target: 'local', effort: 'medium' }); + const saved = JSON.parse(readFileSync(paths.out, 'utf8')); + expect( + saved.verdict.recommendations.map((r: { code: string }) => r.code), + ).toEqual(['root-cause-triage', 'land-and-defer']); + expect(saved.verdict.recommendations[0].basis).toBe('2 file(s) …'); + rmSync(paths.out, { force: true }); + + // A present value of the wrong shape is refused like every sibling. + writeJson(paths.composed, { ...verdict, recommendations: 'nope' }); + expect(() => + saveReviewArtifact({ ...paths, target: 'local', effort: 'medium' }), + ).toThrow(/recommendations/); + }); + it('PRESERVES an absent postedFresh and refuses a present one of the wrong shape', () => { // Same distinction as its sibling: a round that recorded no fresh count // is not a round that produced none. diff --git a/packages/cli/src/commands/review/save-artifact.ts b/packages/cli/src/commands/review/save-artifact.ts index ebf6322a83b..eed4c6e1e03 100644 --- a/packages/cli/src/commands/review/save-artifact.ts +++ b/packages/cli/src/commands/review/save-artifact.ts @@ -33,6 +33,7 @@ import { EFFORT_LEVELS, type ReviewEffort } from './parse-args.js'; import { REVIEWS_DIR } from './lib/paths.js'; import { isSameFile } from './lib/same-file.js'; import { volumeOf } from './lib/ledger.js'; +import type { Recommendation } from './lib/convergence.js'; import { writeStderrLine, writeStdoutLine } from '../../utils/stdioHelpers.js'; interface PersistedVerdict @@ -292,7 +293,6 @@ function validateVerdict(value: unknown): PersistedVerdict { 'Composed verdict.postedInline must be a non-negative integer.', ); } - // The fresh count reads by the same rules as the total it is part of. // The convergence paragraph is the ONE clause the overflow ladder sheds // first, and the artifact is where a trimmed round's record lives. Dropped // by this allow-list, the durable record of a round whose body shed it @@ -307,6 +307,30 @@ function validateVerdict(value: unknown): PersistedVerdict { }; } // The fresh count reads by the same rules as the total it is part of. + // The machine-readable half of the observation. Dropped by this + // allow-list, a caller reading the durable record sees the prose and not + // the codes it would key on. + const rawRecs = verdict['recommendations']; + let recommendations: Recommendation[] | undefined; + if (rawRecs !== undefined && rawRecs !== null) { + if (!Array.isArray(rawRecs)) { + throw new Error('Composed verdict.recommendations must be an array.'); + } + recommendations = rawRecs.map((entry, i) => { + const r = object(entry, `Composed verdict.recommendations[${i}]`); + return { + code: string( + r['code'], + `Composed verdict.recommendations[${i}].code`, + ) as Recommendation['code'], + basis: string( + r['basis'], + `Composed verdict.recommendations[${i}].basis`, + ), + }; + }); + } + // The fresh count reads by the same rules as the total it is part of. const rawFresh = verdict['postedFresh']; const freshAbsent = rawFresh === undefined || rawFresh === null; const postedFresh = freshAbsent ? undefined : volumeOf(rawFresh); @@ -367,6 +391,7 @@ function validateVerdict(value: unknown): PersistedVerdict { ...(postedInline === undefined ? {} : { postedInline }), ...(postedFresh === undefined ? {} : { postedFresh }), ...(convergence === undefined ? {} : { convergence }), + ...(recommendations === undefined ? {} : { recommendations }), lowSignal: lowSignal === null ? null From 3fc1adbdbb653b37a17a288e79b76aa2b79b2522 Mon Sep 17 00:00:00 2001 From: wenshao Date: Fri, 21 Aug 2026 10:09:38 +0800 Subject: [PATCH 2/8] fix(review): the posture disclosure needs the manifestation it asserts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 1 found two blockers, both in the mechanism-health half. The posture check held on EVERY default-config round from 6 on — the reporting and enforcement readings differ only in folding an absent floor to `auto`, so the gap itself is permanent there — while the sentence it renders asserts that a Suggestion posted inline because of it. On a Criticals-only round, or on an APPROVE, that claim was simply false: the clause accused the posture of failing on rounds where it was not asked to do anything. It now requires all three conjuncts, because the sentence asserts all three. Gating on the posted count is safe precisely where the enforcement reading is false: `floorEnforcedReroute` never ran, so no inline Suggestion can be its deliberate deterministic carve-out — and when enforcement does engage, the second conjunct is false and the carve-out can never trip it. The APPROVE branch also spread the health block twice. Removed — and the branch's invariant is now written down beside it: neither half can fire there (the posture half needs a posted Suggestion, which makes the event COMMENT; the anchor half needs a fail-closed scope, which caps the verdict off this branch), verified by probe rather than argued. The spread is kept for symmetry with the convergence block above it, which carries the same invariant, so a later check that CAN fire here does not have to rediscover the wiring. The duplicate has no test, and deliberately so: the branch admits no shape where the block is non-empty, so any test for it would assert a state the code cannot reach. The manifestation gate is pinned three ways — the clause renders once, a Criticals-only round is silent, and a nothing-to-report round is silent while its anchor-chain disclosure still stands. --- .../commands/review/compose-review.test.ts | 30 +++++++++++++++ .../cli/src/commands/review/compose-review.ts | 37 ++++++++++++++----- 2 files changed, 58 insertions(+), 9 deletions(-) diff --git a/packages/cli/src/commands/review/compose-review.test.ts b/packages/cli/src/commands/review/compose-review.test.ts index e5352db1040..ee0fc7106f9 100644 --- a/packages/cli/src/commands/review/compose-review.test.ts +++ b/packages/cli/src/commands/review/compose-review.test.ts @@ -9883,6 +9883,36 @@ describe('convergence diagnosis reaches the POSTED body', () => { expect(r.body).toContain('Mechanism health:'); expect(r.body).toContain('engaged in name and not in effect'); + // The clause renders ONCE. It is spread into three body-assembly + // branches, and a second spread in one of them printed it twice. + expect(r.body.split('engaged in name and not in effect')).toHaveLength(2); + + // A round that posted NO Suggestion is a round where the gap had no + // manifestation — and the sentence asserts one. The first two conjuncts + // hold on every default-config round from 6 on, so stopping there + // accused the posture of failing on rounds where it was not even asked + // to do anything. + const criticalsOnly = composeReview({ + planPath: plan(), + modelId: 'm', + criticalsInline: 1, + suggestionsInline: 0, + draftedComments: [{ path: 'a.ts', line: 1, body: '**[Critical]** boom' }], + }); + expect(criticalsOnly.body).not.toContain('engaged in name'); + + // Neither is a round with nothing to report at all. (Its anchor chain + // disclosure still stands — that check is about the machinery and does + // not depend on what the round found.) + const nothing = composeReview({ + planPath: plan(), + modelId: 'm', + criticalsInline: 0, + suggestionsInline: 0, + draftedComments: [], + }); + expect(nothing.body).not.toContain('engaged in name'); + // With the floor NAMED, both readings agree and nothing is disclosed. const named = composeReview({ planPath: plan(), diff --git a/packages/cli/src/commands/review/compose-review.ts b/packages/cli/src/commands/review/compose-review.ts index 75f44a290b2..792bc1110c2 100644 --- a/packages/cli/src/commands/review/compose-review.ts +++ b/packages/cli/src/commands/review/compose-review.ts @@ -3736,16 +3736,26 @@ function composeReviewBody( ? renderMechanismHealth({ // Nominally engaged, mechanically not: the floor resolved to // critical and Suggestion-level findings posted inline anyway. - // The REPORTING reading resolved the floor to critical and the - // enforcement backstop did not — the two differ only in folding an - // absent floor to `auto`, so this is the default configuration's - // standing gap, and it is invisible from either side alone. Not a - // count of what posted: `floorEnforcedReroute` deliberately leaves - // deterministic `[test]`/`[build]` findings inline at any floor, and - // reading those as a malfunction would flag the carve-out working. + // The REPORTING reading resolved the floor to critical, the + // enforcement backstop did not, AND a Suggestion posted inline + // because of it. All three, because the sentence asserts all three. + // + // The first two hold on EVERY default-config round from 6 on — the + // readings differ only in folding an absent floor to `auto` — so + // stopping there accused a Criticals-only round, and an APPROVE + // round, of a manifestation that had not happened. The gap without + // a consequence is not a malfunction anyone can act on; the gap + // WITH one is. + // + // Gating on the posted count is safe here precisely because the + // enforcement reading is false: `floorEnforcedReroute` never ran, so + // no inline Suggestion can be its deliberate deterministic + // carve-out. When enforcement DOES engage, the second conjunct is + // false and the carve-out can never trip this. postureNotEngaging: convergence.criticalFloorKind !== undefined && - convergence.floorEnforcementEngaged === false, + convergence.floorEnforcementEngaged === false && + suggestionsInline > 0, // Two consecutive withholds — this round's decision read through the // marker's OWN predicate, and the recovered round's recorded anchor. anchorChainBroken: @@ -3849,9 +3859,18 @@ function composeReviewBody( ...repositoryContextBlock, ...unlicensedDeferralBlock, ...deferredSuggestionsBlock, + // Both of these are spread for symmetry with the branches above and + // cannot actually fire here — the same shape as the convergence + // invariant this branch already carries. The posture half needs a + // Suggestion to have posted, which makes the event COMMENT; the + // anchor half needs a fail-closed scope, which caps the verdict off + // this branch. Verified by probe (event APPROVE, health block + // empty). Kept rather than dropped so a later reader adding a check + // that CAN fire here does not have to rediscover the wiring — and + // spread ONCE: a second spread printed the clause twice on any round + // that did reach it. ...convergenceBlock, ...healthBlock, - ...healthBlock, ...continuityBlock, ], notReviewedParts.length || From 7603bf4c15879c04277090c76141bab222d66957 Mon Sep 17 00:00:00 2001 From: wenshao Date: Fri, 21 Aug 2026 13:28:34 +0800 Subject: [PATCH 3/8] fix(review): close round 2 on the machine-readable half MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two blockers. The posture check still accused a round the posture was running correctly. SKILL Step 6 excludes a `[build]`/`[test]`/`[probe]` finding by source at ANY floor — it is pre-confirmed and stays inline whether or not the floor engaged — so a fully compliant round that defers every deferrable Suggestion and posts one such finding satisfied all three conjuncts. My previous round's argument for the gate was true and beside the point: when the code-side reroute has failed open, the MODEL-side posture is the layer in charge, and it carries the same carve-out. The count now excludes deterministic findings, read off the claim line through the same projection `floorEnforcedReroute` uses. The health block shared the convergence paragraph's trim rank while having no copy outside the posted body — the exact false-record class the convergence paragraph's own fix exists to close. Worse at the sharpest corner: with no diagnosis firing, rank 0 held ONLY the health note, so the trim notice named "the convergence observation" for a section that never existed. The block gets its own rank with its own name (shed before the paragraph, because its reader is the operator who has the terminal line), and the note now rides `ComposeReviewResult`, the `HEALTH:` terminal line and the durable artifact. Also: the anchor-chain check states what it measures and names what it cannot see (the scope is the only withholding leg visible from the body composer; a plan with no fetched sha, an unreadable plan, and a model identity drift also withhold, and are decided where the marker is built); `recommendations[].code` is checked against the closed set instead of cast into it; the absent-code list now covers the whole eleven-code menu; and the comment claiming an absent floor reads as `auto` "throughout this module" now says which reading folds and which does not. Nine mutations, each verified to turn a named test red. Two of them needed the fixtures fixed first: the health field and the codes are spread into three separately-maintained result constructions, and the tests only reached one of them. --- .../commands/review/compose-review.test.ts | 131 ++++++++++++++++++ .../cli/src/commands/review/compose-review.ts | 81 +++++++++-- .../commands/review/lib/convergence.test.ts | 8 ++ .../src/commands/review/lib/convergence.ts | 30 +++- .../src/commands/review/save-artifact.test.ts | 25 ++++ .../cli/src/commands/review/save-artifact.ts | 41 +++++- 6 files changed, 298 insertions(+), 18 deletions(-) diff --git a/packages/cli/src/commands/review/compose-review.test.ts b/packages/cli/src/commands/review/compose-review.test.ts index ee0fc7106f9..8e899feeda7 100644 --- a/packages/cli/src/commands/review/compose-review.test.ts +++ b/packages/cli/src/commands/review/compose-review.test.ts @@ -10016,6 +10016,137 @@ describe('convergence diagnosis reaches the POSTED body', () => { expect(parseLedger(r.body)?.fresh).toBe(1); }); + it('does not accuse the posture over a finding the posture itself exempts', () => { + // SKILL Step 6 excludes a `[build]`/`[test]`/`[probe]` finding by source + // at any floor: it is pre-confirmed and stays inline whether or not the + // floor engaged. A fully compliant round that defers every deferrable + // Suggestion and posts one such finding is the posture working, not + // failing — and when the code-side reroute has failed open, the + // model-side posture is the layer carrying that same carve-out. + sideFile({ round: 5, posted: 1, fresh: 1, floor: 'c', findings: [] }); + const deterministic = composeReview({ + planPath: plan(), + modelId: 'm', + criticalsInline: 0, + suggestionsInline: 1, + draftedComments: [ + { path: 'a.ts', line: 1, body: '**[Suggestion]** [test] suite is red' }, + ], + }); + expect(deterministic.body).not.toContain('engaged in name'); + + // A Suggestion the floor WOULD have deferred still fires it. + const deferrable = composeReview({ + planPath: plan(), + modelId: 'm', + criticalsInline: 0, + suggestionsInline: 1, + draftedComments: [ + { path: 'a.ts', line: 1, body: '**[Suggestion]** a plain nit' }, + ], + }); + expect(deferrable.body).toContain('engaged in name'); + }); + + it('leaves a terminal copy of the health note the ladder sheds first', () => { + // The note has its own rank BELOW the convergence paragraph, so it is + // the first thing shed — and the trim notice points the reader at a + // terminal report that must actually hold it. + sideFile({ round: 4, posted: 9, fresh: 9, findings: [] }); + const r = composeReview({ + planPath: plan(), + modelId: 'm', + criticalsInline: 1, + suggestionsInline: 0, + draftedComments: [{ path: 'a.ts', line: 1, body: '**[Critical]** boom' }], + }); + expect(r.body).toContain('Mechanism health:'); + expect(r.health?.en).toContain('Mechanism health:'); + expect(r.health?.zh).toContain('机制健康:'); + }); + + it('names the health note in the trim notice, not the convergence one', () => { + // With no diagnosis firing, rank -1 holds ONLY this note. Sharing rank 0 + // made the notice name "the convergence observation" for a section that + // never existed in the body. + sideFile({ round: 4, posted: 9, fresh: 9, findings: [] }); + const r = composeReview({ + planPath: plan(), + modelId: 'm', + criticalsInline: 1, + suggestionsInline: 0, + bodyCriticals: ['B'.repeat(56_000)], + unreviewedDimensions: ['security'], + draftedComments: [{ path: 'a.ts', line: 1, body: '**[Critical]** boom' }], + }); + expect(r.body.length).toBeLessThanOrEqual(65536); + expect(r.body).not.toContain('Mechanism health:'); + expect(r.body).toContain('the mechanism-health note'); + expect(r.body).not.toContain('the convergence observation'); + // ...and the copy the notice points at exists. + expect(r.health?.en).toContain('Mechanism health:'); + }); + + it('keeps quiet on a round whose scope closed cleanly', () => { + // The chain is TWO withholds. A round that anchors clears it, however + // unanchored its predecessor was. + const planPath = coveredPlan(['verify', 'reverse-audit'], { + prNumber: 8255, + fetchedSha: 'deadbeef00112233', + }); + writeFileSync( + join(dirname(planPath), 'qwen-review-pr-8255-prev-ledger.json'), + JSON.stringify({ v: 1, round: 4, findings: [], posted: 0, fresh: 0 }), + ); + const r = composeReview({ + planPath, + env: ENV, + modelId: MODEL, + criticalsInline: 1, + suggestionsInline: 0, + draftedComments: [{ path: 'a.ts', line: 1, body: '**[Critical]** boom' }], + }); + expect(parseLedger(r.body)?.sha).toBe('deadbeef00112233'); + expect(r.body).not.toContain('re-reads the whole diff'); + }); + + it('carries the codes on a REQUEST_CHANGES result too', () => { + // Three separately-maintained result constructions; only one was pinned. + const planPath = coveredPlan(['verify', 'reverse-audit'], { + prNumber: 8255, + fetchedSha: 'deadbeef00112233', + }); + writeFileSync( + join(dirname(planPath), 'qwen-review-pr-8255-prev-ledger.json'), + JSON.stringify({ + v: 1, + round: 5, + posted: 9, + fresh: 9, + findings: [{ id: 'R2-1', sev: 'C', file: 'src/a.ts', title: 'x' }], + }), + ); + const r = composeReview({ + planPath, + env: ENV, + modelId: MODEL, + criticalsInline: 1, + suggestionsInline: 1, + draftedComments: [ + { path: 'src/a.ts', line: 1, body: '**[Critical]** a new one' }, + { path: 'src/b.ts', line: 2, body: '**[Suggestion]** a plain nit' }, + ], + }); + expect(r.event).toBe('REQUEST_CHANGES'); + expect((r.recommendations ?? []).map((x) => x.code)).toContain( + 'root-cause-triage', + ); + // ...and this branch's own copy of the health note. It is round 6 under + // the default configuration, so the posture gap is real and manifested. + expect(r.body).toContain('engaged in name and not in effect'); + expect(r.health?.en).toContain('Mechanism health:'); + }); + it('names an auto-resolved floor the way the enforcement note does', () => { // `auto` is the DEFAULT, so the explicit-flag wording claims a flag that // was never passed — beside a floor-enforcement note in the same body diff --git a/packages/cli/src/commands/review/compose-review.ts b/packages/cli/src/commands/review/compose-review.ts index 792bc1110c2..97aa701c92d 100644 --- a/packages/cli/src/commands/review/compose-review.ts +++ b/packages/cli/src/commands/review/compose-review.ts @@ -847,6 +847,18 @@ export interface ComposeReviewResult { * signal fired, exactly like the paragraph. */ recommendations?: Recommendation[]; + /** + * The mechanism-health disclosure, when one fired — the SAME text the body + * carries, returned so a terminal copy exists. + * + * The overflow ladder sheds this paragraph before every other, and its + * notice tells the author the trimmed sections "still hold — read them in + * the terminal report". That was a false record while this text lived only + * inside the body composer, exactly as it was for the convergence + * paragraph: a disclosure derived from the round's own caps has no other + * copy anywhere unless the result carries one. + */ + health?: { en: string; zh: string }; /** * The previous round's `postedInline`, recovered from the side file when * it recorded one. Absent on round 1, on a recovery miss, and on any @@ -1771,10 +1783,12 @@ function ledgerMarkerFor( // critical floor and the volume under an open one are not two points // on one trend. Decides nothing, sheds with the volume it qualifies. // The RESOLVED posture, folded the way every consumer folds it: an - // ABSENT floor reads as `auto` throughout this module (a present but + // ABSENT floor reads as `auto` in the REPORTING reading (a present but // unrecognisable one reads as nothing at all — see // `criticalFloorKind`), and `auto` resolves determinately from the - // round number and the context state. Recording it only when the state NAMED a floor + // round number and the context state. The ENFORCEMENT reading folds + // nothing and fails open on both; the gap between the two is what the + // mechanism-health check discloses. Recording it only when the state NAMED a floor // left the guard blind under the DEFAULT configuration — where the // posture genuinely transitions at round 6 and again on a transient // context failure — so a real posture change read as loop divergence, @@ -2916,6 +2930,7 @@ function composeReviewBody( /** What a rank drops, in the author's words — the note names it. */ const RANK_NAMES: Record = { + [-1]: { en: 'the mechanism-health note', zh: '机制健康说明' }, 0: { en: 'the convergence observation', zh: '收敛情况观察' }, 1: { en: 'the deferred-findings list', zh: '延后发现清单' }, 2: { @@ -3747,15 +3762,20 @@ function composeReviewBody( // a consequence is not a malfunction anyone can act on; the gap // WITH one is. // - // Gating on the posted count is safe here precisely because the - // enforcement reading is false: `floorEnforcedReroute` never ran, so - // no inline Suggestion can be its deliberate deterministic - // carve-out. When enforcement DOES engage, the second conjunct is - // false and the carve-out can never trip this. + // The count EXCLUDES deterministic findings, through the same + // projection `floorEnforcedReroute` reads. Arguing that the code-side + // reroute never ran (so nothing inline can be ITS carve-out) is true + // and beside the point: when the enforcement reading is false the + // model-side posture is the layer in charge, and SKILL Step 6 carries + // the same carve-out — a `[build]`/`[test]`/`[probe]` finding is + // pre-confirmed and stays inline at any floor. A fully compliant + // round that defers every deferrable Suggestion and posts one + // `[test]` finding would otherwise be accused of a failure that is + // the posture working as specified. postureNotEngaging: convergence.criticalFloorKind !== undefined && convergence.floorEnforcementEngaged === false && - suggestionsInline > 0, + deferrableSuggestionsInline(input.draftedComments) > 0, // Two consecutive withholds — this round's decision read through the // marker's OWN predicate, and the recovered round's recorded anchor. anchorChainBroken: @@ -3764,7 +3784,13 @@ function composeReviewBody( anchorFailsClosed(cappedBy, scopeUnproven, dimensionGapsAreDepthOnly), }) : null; - const healthBlock: Bi[] = healthNote ? [{ ...healthNote, trim: 0 }] : []; + // Its OWN rank, shed before the convergence paragraph. Sharing rank 0 made + // the notice name "the convergence observation" for a body whose rank-0 + // content was only this note — a section that never existed. It goes first + // because its primary reader is the operator, who has the `HEALTH:` + // terminal line, while the convergence paragraph's recommendations are + // addressed to the author reading the PR. + const healthBlock: Bi[] = healthNote ? [{ ...healthNote, trim: -1 }] : []; const convergenceBlock: Bi[] = convergenceNote ? [{ ...convergenceNote, trim: 0 }] : []; @@ -3823,6 +3849,9 @@ function composeReviewBody( ? {} : { convergence: convergenceNote }), ...(recommendations === undefined ? {} : { recommendations }), + ...(healthNote === null || healthNote === undefined + ? {} + : { health: healthNote }), bodyTrim, lowSignal, scopeUnproven, @@ -3907,6 +3936,9 @@ function composeReviewBody( ? {} : { convergence: convergenceNote }), ...(recommendations === undefined ? {} : { recommendations }), + ...(healthNote === null || healthNote === undefined + ? {} + : { health: healthNote }), bodyTrim, lowSignal, scopeUnproven, @@ -4140,6 +4172,9 @@ function composeReviewBody( postedFresh, ...(convergenceNote === undefined ? {} : { convergence: convergenceNote }), ...(recommendations === undefined ? {} : { recommendations }), + ...(healthNote === null || healthNote === undefined + ? {} + : { health: healthNote }), bodyTrim, lowSignal, scopeUnproven, @@ -4854,6 +4889,11 @@ export const composeReviewCommand: CommandModule = { if (result.convergence) { writeStderrLine(`CONVERGENCE: ${result.convergence.en}`); } + // The same promise for the same reason: this block is the FIRST thing the + // overflow ladder sheds, and the notice points the reader here. + if (result.health) { + writeStderrLine(`HEALTH: ${result.health.en}`); + } writeStderrLine(verdictLine(result)); }, }; @@ -4899,6 +4939,29 @@ function ledgerClaimLine(body: unknown): string { ); } +/** + * Inline Suggestions the posting floor WOULD have deferred — every + * Suggestion-severity draft whose claim line carries no deterministic tag. + * + * The posture excludes a `[build]`/`[test]`/`[probe]` finding by source at + * any floor: it is pre-confirmed, and it stays inline whether or not the + * floor engaged. Counting it as evidence that the floor failed to act reads + * the posture working as specified as the posture failing — and the tag is + * read off the CLAIM LINE only, the same window `floorEnforcedReroute` uses, + * because the body's tail is writable surface a footer can forge. + */ +function deferrableSuggestionsInline(drafted: unknown): number { + if (!Array.isArray(drafted)) return 0; + let n = 0; + for (const c of drafted as Array<{ body?: unknown }>) { + if (severityOf(c) !== 'suggestion') continue; + const claim = carriedClaimLine(typeof c.body === 'string' ? c.body : ''); + if (claim !== null && DETERMINISTIC_TAG_RE.test(claim)) continue; + n++; + } + return n; +} + /** * This round's drafts in the shape the convergence diagnosis reads. * diff --git a/packages/cli/src/commands/review/lib/convergence.test.ts b/packages/cli/src/commands/review/lib/convergence.test.ts index 142f98d2588..c08f4aafba2 100644 --- a/packages/cli/src/commands/review/lib/convergence.test.ts +++ b/packages/cli/src/commands/review/lib/convergence.test.ts @@ -721,6 +721,14 @@ describe('recommendationsFor — measurement to advice, no constants', () => { expect(prose.en).toContain('--severity-floor critical'); expect(prose.en).toContain('No Critical finding is open on this round'); expect(prose.zh).toContain('本轮没有未决的 Critical'); + // ...and the negative side: an open blocker means the ending is not + // available, so the sentence must not render. + const withBlocker = renderConvergenceDiagnosis({ + ...base, + openCriticals: 2, + }); + expect(withBlocker.en).not.toContain('No Critical finding is open'); + expect(withBlocker.zh).not.toContain('本轮没有未决的 Critical'); // ...and the narrowed floor case drops exactly the rung it dropped. const atFloor = renderConvergenceDiagnosis({ ...base, diff --git a/packages/cli/src/commands/review/lib/convergence.ts b/packages/cli/src/commands/review/lib/convergence.ts index a956fe72b99..01e3ae3bca2 100644 --- a/packages/cli/src/commands/review/lib/convergence.ts +++ b/packages/cli/src/commands/review/lib/convergence.ts @@ -212,6 +212,13 @@ export type CriticalFloorKind = 'explicit' | 'auto-resolved'; * were a non-converged reverse audit and skipped integration tests, which * one raised-budget round does not clear — so the chain is DISCLOSED below * as mechanism health and prescribes nothing. + * - `human-triage` is matched to "any shape" in the design, which makes it + * advice no measurement selected. Emitting it on every diagnosis would + * 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. */ export type RecommendationCode = | 'root-cause-triage' @@ -241,11 +248,20 @@ export interface MechanismHealth { */ postureNotEngaging: boolean; /** - * This round withholds the incremental anchor and the round it recovered - * had none either. Two consecutive withholds mean the next round re-reads - * the whole diff, and the round after that, until something clears it — - * the closed loop measured at 119 minutes and 34M tokens on a PR whose - * code had not changed a line. + * This round's SCOPE did not close cleanly — which withholds the + * incremental anchor — and the round it recovered carried none either. Two + * consecutive withholds mean the next round re-reads the whole diff, and + * the round after that, until something clears it: the closed loop + * measured at 119 minutes and 34M tokens on a PR whose code had not + * changed a line. + * + * A stated limit: the scope is the only withholding leg visible from here. + * The marker also withholds when the plan carries no fetched sha, when it + * cannot be read, and when the round's model identity drifted — those are + * decided where the marker is built, with the plan in hand, and a round + * withheld only by one of them is a chain this check does not see. It + * under-reports rather than over-reports, and the wording claims only what + * it measured. */ anchorChainBroken: boolean; } @@ -614,10 +630,10 @@ export function renderMechanismHealth( } if (h.anchorChainBroken) { en.push( - `this round withholds the incremental anchor and the round it recovered had none either, so the next review re-reads the whole diff — and will keep doing so until a round closes cleanly`, + `this round's scope did not close cleanly, so it withholds the incremental anchor — and the round it recovered had none either, so the next review re-reads the whole diff and will keep doing so until a round closes cleanly`, ); zh.push( - `本轮扣留了增量锚点,而它恢复到的那一轮也没有锚点,因此下一次评审将重读整个 diff——并会一直如此,直到某一轮干净收尾`, + `本轮的作用域未能干净收尾,因而扣留了增量锚点,而它恢复到的那一轮也没有锚点,因此下一次评审将重读整个 diff——并会一直如此,直到某一轮干净收尾`, ); } if (en.length === 0) return null; diff --git a/packages/cli/src/commands/review/save-artifact.test.ts b/packages/cli/src/commands/review/save-artifact.test.ts index c6f66116a07..b31005c350d 100644 --- a/packages/cli/src/commands/review/save-artifact.test.ts +++ b/packages/cli/src/commands/review/save-artifact.test.ts @@ -491,6 +491,31 @@ describe('saveReviewArtifact', () => { expect(() => saveReviewArtifact({ ...paths, target: 'local', effort: 'medium' }), ).toThrow(/recommendations/); + + // ...and the code is checked against the closed set, not cast into it: a + // set a caller wires actions to is a contract, and a cast writes + // whatever string it was handed under a type that says otherwise. + writeJson(paths.composed, { + ...verdict, + recommendations: [{ code: 'make-coffee', basis: 'x' }], + }); + expect(() => + saveReviewArtifact({ ...paths, target: 'local', effort: 'medium' }), + ).toThrow(/recommendation codes/); + }); + + it('carries the mechanism-health note into the artifact', () => { + // The first clause the overflow ladder sheds, so the artifact may be its + // only durable copy on the rounds it fires. + const paths = fixture(); + writeJson(paths.composed, { + ...verdict, + health: { en: 'Mechanism health: …', zh: '机制健康:…' }, + }); + saveReviewArtifact({ ...paths, target: 'local', effort: 'medium' }); + const saved = JSON.parse(readFileSync(paths.out, 'utf8')); + expect(saved.verdict.health.en).toBe('Mechanism health: …'); + expect(saved.verdict.health.zh).toBe('机制健康:…'); }); it('PRESERVES an absent postedFresh and refuses a present one of the wrong shape', () => { diff --git a/packages/cli/src/commands/review/save-artifact.ts b/packages/cli/src/commands/review/save-artifact.ts index eed4c6e1e03..f51e803b8ec 100644 --- a/packages/cli/src/commands/review/save-artifact.ts +++ b/packages/cli/src/commands/review/save-artifact.ts @@ -213,6 +213,31 @@ function event(value: unknown, label: string): ReviewEvent { return value; } +const RECOMMENDATION_CODES: ReadonlySet = new Set([ + 'root-cause-triage', + 'land-and-defer', + 'batch-fixes', + 'stem-surface', +]); + +/** + * A recommendation code, checked against the closed set rather than cast + * into it. The set is a contract a caller wires actions to, and a cast + * writes whatever string it was handed into the durable record under a type + * that says otherwise — the shape every sibling closed vocabulary in this + * validator refuses. + */ +function recommendationCode( + value: unknown, + label: string, +): Recommendation['code'] { + const code = string(value, label); + if (!RECOMMENDATION_CODES.has(code)) { + throw new Error(`${label} must be one of the known recommendation codes.`); + } + return code as Recommendation['code']; +} + function validateVerdict(value: unknown): PersistedVerdict { const verdict = object(value, 'Composed verdict'); const downgradedFrom = verdict['downgradedFrom']; @@ -319,10 +344,10 @@ function validateVerdict(value: unknown): PersistedVerdict { recommendations = rawRecs.map((entry, i) => { const r = object(entry, `Composed verdict.recommendations[${i}]`); return { - code: string( + code: recommendationCode( r['code'], `Composed verdict.recommendations[${i}].code`, - ) as Recommendation['code'], + ), basis: string( r['basis'], `Composed verdict.recommendations[${i}].basis`, @@ -331,6 +356,17 @@ function validateVerdict(value: unknown): PersistedVerdict { }); } // The fresh count reads by the same rules as the total it is part of. + // Same reasoning as the paragraph above, and more so: this block is the + // FIRST thing the ladder sheds. + const rawHealth = verdict['health']; + let health: { en: string; zh: string } | undefined; + if (rawHealth !== undefined && rawHealth !== null) { + const h = object(rawHealth, 'Composed verdict.health'); + health = { + en: string(h['en'], 'Composed verdict.health.en'), + zh: string(h['zh'], 'Composed verdict.health.zh'), + }; + } const rawFresh = verdict['postedFresh']; const freshAbsent = rawFresh === undefined || rawFresh === null; const postedFresh = freshAbsent ? undefined : volumeOf(rawFresh); @@ -392,6 +428,7 @@ function validateVerdict(value: unknown): PersistedVerdict { ...(postedFresh === undefined ? {} : { postedFresh }), ...(convergence === undefined ? {} : { convergence }), ...(recommendations === undefined ? {} : { recommendations }), + ...(health === undefined ? {} : { health }), lowSignal: lowSignal === null ? null From db1faef550f459ed61df3c976c055a2a121622e7 Mon Sep 17 00:00:00 2001 From: wenshao Date: Fri, 21 Aug 2026 16:29:37 +0800 Subject: [PATCH 4/8] fix(review): close round 3 on the machine-readable half MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The blocker: `land-and-defer` matched on a zero that was not a confirmed zero. A round capped `cannot-tell-existing-critical` posts no Criticals precisely BECAUSE existing ones could not be ruled on — the entries ride their own channel, are never counted, and were never shown fixed — and `findings-unverified-at-compose` is the same shape. The body would then say "Unresolved, please confirm:" and "no Critical is open" at once, and the artifact would tell a machine consumer to merge. The count is passed only when the blocker state is established, and the module's own "absent is not zero" rule withholds the code otherwise. Four of the seven suggestions are corrections to things this branch itself introduced, three of them last round: - the health note's own rank made the convergence block's "shed before every other" comment false; it now says which one goes first and why; - the accusation counted pathless Suggestions, which the floor structurally cannot defer — no deferral entry can be built without a path — so it accused the posture of failing to move something it has nowhere to move; - the anchor-chain sentence attributed the withhold to the round's SCOPE while the predicate it reads also fires on a dimension gap and on verdict caps; it now says only that the round did not close cleanly, and the docstring names all three legs; - and the comment misplacement I claimed to have cleared was reintroduced one line up, by an insert anchored on the statement below its own comment. Also: the closed code vocabulary is declared once and the type derived from it, rather than a union and a runtime set kept in step by hand; the `HEALTH:` terminal line and the health validator's refusal path are pinned. Six mutations, each verified to turn a named test red. One needed the fixture fixed first — every test drove a Suggestion that had a path. --- .../commands/review/compose-review.test.ts | 44 +++++++++++++++++++ .../cli/src/commands/review/compose-review.ts | 27 ++++++++++-- .../src/commands/review/lib/convergence.ts | 30 ++++++++----- .../src/commands/review/save-artifact.test.ts | 7 +++ .../cli/src/commands/review/save-artifact.ts | 16 +++---- 5 files changed, 101 insertions(+), 23 deletions(-) diff --git a/packages/cli/src/commands/review/compose-review.test.ts b/packages/cli/src/commands/review/compose-review.test.ts index 8e899feeda7..23abec3c55b 100644 --- a/packages/cli/src/commands/review/compose-review.test.ts +++ b/packages/cli/src/commands/review/compose-review.test.ts @@ -2711,6 +2711,11 @@ describe('composeReviewCommand handler (the CLI glue)', () => { expect(lines.some((l) => l.startsWith('CONVERGENCE: Convergence:'))).toBe( true, ); + // Its sibling, for the same reason: the health note is the FIRST thing + // the ladder sheds, and the trim notice points the reader here. + expect(lines.some((l) => l.startsWith('HEALTH: Mechanism health:'))).toBe( + true, + ); } finally { rmSync(dir, { recursive: true, force: true }); } @@ -10035,6 +10040,19 @@ describe('convergence diagnosis reaches the POSTED body', () => { }); expect(deterministic.body).not.toContain('engaged in name'); + // A PATHLESS Suggestion is excluded for the same reason by a different + // route: it cannot become a deferral entry at all, so no floor could + // have moved it — the same structural exclusion `floorEnforcedReroute` + // makes. + const pathless = composeReview({ + planPath: plan(), + modelId: 'm', + criticalsInline: 0, + suggestionsInline: 1, + draftedComments: [{ line: 1, body: '**[Suggestion]** a plain nit' }], + }); + expect(pathless.body).not.toContain('engaged in name'); + // A Suggestion the floor WOULD have deferred still fires it. const deferrable = composeReview({ planPath: plan(), @@ -10147,6 +10165,32 @@ describe('convergence diagnosis reaches the POSTED body', () => { expect(r.health?.en).toContain('Mechanism health:'); }); + it('withholds land-and-defer while a blocker could not be ruled on', () => { + // A round capped `cannot-tell-existing-critical` posts zero Criticals + // precisely BECAUSE existing ones could not be ruled on: the entries + // ride their own channel, are never counted, and were never shown fixed. + // Passed as a confirmed zero, the body would carry "Unresolved, please + // confirm:" and "no Critical is open" at once, and the artifact would + // tell a machine consumer to merge. + sideFile({ round: 5, posted: 1, fresh: 1, findings: [] }); + const r = composeReview({ + planPath: plan(), + modelId: 'm', + criticalsInline: 0, + suggestionsInline: 1, + cannotTellCriticals: ['a.ts:12 — an existing blocker, unruled'], + draftedComments: [ + { path: 'a.ts', line: 1, body: '**[Suggestion]** a plain nit' }, + ], + }); + expect(r.cappedBy).toContain('cannot-tell-existing-critical'); + expect(r.body).toContain('Convergence:'); + expect(r.body).not.toContain('No Critical finding is open'); + expect((r.recommendations ?? []).map((x) => x.code)).not.toContain( + 'land-and-defer', + ); + }); + it('names an auto-resolved floor the way the enforcement note does', () => { // `auto` is the DEFAULT, so the explicit-flag wording claims a flag that // was never passed — beside a floor-enforcement note in the same body diff --git a/packages/cli/src/commands/review/compose-review.ts b/packages/cli/src/commands/review/compose-review.ts index 97aa701c92d..10d58074397 100644 --- a/packages/cli/src/commands/review/compose-review.ts +++ b/packages/cli/src/commands/review/compose-review.ts @@ -2670,7 +2670,18 @@ function composeReviewBody( ...(convergence.criticalFloorKind === undefined ? {} : { criticalFloorKind: convergence.criticalFloorKind }), - openCriticals, + // Only when the blocker state is ESTABLISHED. A round capped + // `cannot-tell-existing-critical` posts zero Criticals precisely + // BECAUSE existing ones could not be ruled on — the entries ride + // their own channel, are never counted here, and were never shown + // fixed — and `findings-unverified-at-compose` is the same shape. + // The module's own rule then withholds `land-and-defer`: an absent + // count is not a count of none. Passed anyway, the body would carry + // "Unresolved, please confirm:" and "no Critical is open" at once, + // and the artifact would tell a machine consumer to merge. + ...(cannotTell.length === 0 && !findingsUnverifiedAtCompose + ? { openCriticals } + : {}), }) : null; // A fact about the round, not about the diagnosis: it rides in the marker @@ -3729,7 +3740,9 @@ function composeReviewBody( // paragraph here that comments on the SHAPE of the review history rather // than on the diff. // - // `trim: 0` — its OWN rank, shed before every other. An untagged block + // `trim: 0` — its own rank, shed before every other EXCEPT the + // mechanism-health note below it (rank -1, and see there for why it goes + // first). An untagged block // ranks with the blockers and the verdict-qualifying sentences, and the // rounds this fires on are precisely the high-volume rounds most likely to // overflow: unranked, an advisory paragraph that decides nothing survived @@ -4949,14 +4962,22 @@ function ledgerClaimLine(body: unknown): string { * the posture working as specified as the posture failing — and the tag is * read off the CLAIM LINE only, the same window `floorEnforcedReroute` uses, * because the body's tail is writable surface a footer can forge. + * + * A pathless comment is excluded for the same reason by a different route: + * it cannot become a deferral entry at all, so no floor could have moved it. */ function deferrableSuggestionsInline(drafted: unknown): number { if (!Array.isArray(drafted)) return 0; let n = 0; - for (const c of drafted as Array<{ body?: unknown }>) { + for (const c of drafted as Array<{ body?: unknown; path?: unknown }>) { if (severityOf(c) !== 'suggestion') continue; const claim = carriedClaimLine(typeof c.body === 'string' ? c.body : ''); if (claim !== null && DETERMINISTIC_TAG_RE.test(claim)) continue; + // A pathless comment cannot become a deferral entry, so the floor leaves + // it inline at any posture — the same structural exclusion the reroute + // makes, and counting it would accuse the floor of failing to move + // something it has nowhere to move to. + if (typeof c.path !== 'string' || c.path.trim() === '') continue; n++; } return n; diff --git a/packages/cli/src/commands/review/lib/convergence.ts b/packages/cli/src/commands/review/lib/convergence.ts index 01e3ae3bca2..962238f2d3a 100644 --- a/packages/cli/src/commands/review/lib/convergence.ts +++ b/packages/cli/src/commands/review/lib/convergence.ts @@ -220,11 +220,19 @@ export type CriticalFloorKind = 'explicit' | 'auto-resolved'; * That is the whole menu: eleven codes in the design, four emitted here, * seven named above. */ -export type RecommendationCode = - | 'root-cause-triage' - | 'land-and-defer' - | 'batch-fixes' - | 'stem-surface'; +export const RECOMMENDATION_CODES = [ + 'root-cause-triage', + 'land-and-defer', + 'batch-fixes', + 'stem-surface', +] as const; + +/** + * Derived from the runtime list above, not declared beside it: a validator + * needs the membership check and a caller needs the type, and two hand-kept + * copies of a closed vocabulary drift the moment one gains a code. + */ +export type RecommendationCode = (typeof RECOMMENDATION_CODES)[number]; /** One matched recommendation and the measurement that matched it. */ export interface Recommendation { @@ -248,14 +256,16 @@ export interface MechanismHealth { */ postureNotEngaging: boolean; /** - * This round's SCOPE did not close cleanly — which withholds the - * incremental anchor — and the round it recovered carried none either. Two + * This round did not close cleanly — unproven scope, a dimension gap that + * is not depth-only, or any verdict cap other than an unreviewable + * dimension — which withholds the incremental anchor, and the round it + * recovered carried none either. Two * consecutive withholds mean the next round re-reads the whole diff, and * the round after that, until something clears it: the closed loop * measured at 119 minutes and 34M tokens on a PR whose code had not * changed a line. * - * A stated limit: the scope is the only withholding leg visible from here. + * A stated limit: those are the only withholding legs visible from here. * The marker also withholds when the plan carries no fetched sha, when it * cannot be read, and when the round's model identity drifted — those are * decided where the marker is built, with the plan in hand, and a round @@ -630,10 +640,10 @@ export function renderMechanismHealth( } if (h.anchorChainBroken) { en.push( - `this round's scope did not close cleanly, so it withholds the incremental anchor — and the round it recovered had none either, so the next review re-reads the whole diff and will keep doing so until a round closes cleanly`, + `this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had none either, so the next review re-reads the whole diff and will keep doing so until a round closes cleanly`, ); zh.push( - `本轮的作用域未能干净收尾,因而扣留了增量锚点,而它恢复到的那一轮也没有锚点,因此下一次评审将重读整个 diff——并会一直如此,直到某一轮干净收尾`, + `本轮未能干净收尾,因而扣留了增量锚点,而它恢复到的那一轮也没有锚点,因此下一次评审将重读整个 diff——并会一直如此,直到某一轮干净收尾`, ); } if (en.length === 0) return null; diff --git a/packages/cli/src/commands/review/save-artifact.test.ts b/packages/cli/src/commands/review/save-artifact.test.ts index b31005c350d..443727cb240 100644 --- a/packages/cli/src/commands/review/save-artifact.test.ts +++ b/packages/cli/src/commands/review/save-artifact.test.ts @@ -516,6 +516,13 @@ describe('saveReviewArtifact', () => { const saved = JSON.parse(readFileSync(paths.out, 'utf8')); expect(saved.verdict.health.en).toBe('Mechanism health: …'); expect(saved.verdict.health.zh).toBe('机制健康:…'); + rmSync(paths.out, { force: true }); + + // A present value of the wrong shape is refused, like every sibling. + writeJson(paths.composed, { ...verdict, health: { en: 'x' } }); + expect(() => + saveReviewArtifact({ ...paths, target: 'local', effort: 'medium' }), + ).toThrow(/health\.zh/); }); it('PRESERVES an absent postedFresh and refuses a present one of the wrong shape', () => { diff --git a/packages/cli/src/commands/review/save-artifact.ts b/packages/cli/src/commands/review/save-artifact.ts index f51e803b8ec..14455af0457 100644 --- a/packages/cli/src/commands/review/save-artifact.ts +++ b/packages/cli/src/commands/review/save-artifact.ts @@ -33,7 +33,10 @@ import { EFFORT_LEVELS, type ReviewEffort } from './parse-args.js'; import { REVIEWS_DIR } from './lib/paths.js'; import { isSameFile } from './lib/same-file.js'; import { volumeOf } from './lib/ledger.js'; -import type { Recommendation } from './lib/convergence.js'; +import { + RECOMMENDATION_CODES, + type Recommendation, +} from './lib/convergence.js'; import { writeStderrLine, writeStdoutLine } from '../../utils/stdioHelpers.js'; interface PersistedVerdict @@ -213,13 +216,6 @@ function event(value: unknown, label: string): ReviewEvent { return value; } -const RECOMMENDATION_CODES: ReadonlySet = new Set([ - 'root-cause-triage', - 'land-and-defer', - 'batch-fixes', - 'stem-surface', -]); - /** * A recommendation code, checked against the closed set rather than cast * into it. The set is a contract a caller wires actions to, and a cast @@ -232,7 +228,7 @@ function recommendationCode( label: string, ): Recommendation['code'] { const code = string(value, label); - if (!RECOMMENDATION_CODES.has(code)) { + if (!(RECOMMENDATION_CODES as readonly string[]).includes(code)) { throw new Error(`${label} must be one of the known recommendation codes.`); } return code as Recommendation['code']; @@ -355,7 +351,6 @@ function validateVerdict(value: unknown): PersistedVerdict { }; }); } - // The fresh count reads by the same rules as the total it is part of. // Same reasoning as the paragraph above, and more so: this block is the // FIRST thing the ladder sheds. const rawHealth = verdict['health']; @@ -367,6 +362,7 @@ function validateVerdict(value: unknown): PersistedVerdict { zh: string(h['zh'], 'Composed verdict.health.zh'), }; } + // The fresh count reads by the same rules as the total it is part of. const rawFresh = verdict['postedFresh']; const freshAbsent = rawFresh === undefined || rawFresh === null; const postedFresh = freshAbsent ? undefined : volumeOf(rawFresh); From 2e2ef87a380fd1c5825b2c2b909ed10a0c2c0de5 Mon Sep 17 00:00:00 2001 From: wenshao Date: Fri, 21 Aug 2026 19:39:42 +0800 Subject: [PATCH 5/8] =?UTF-8?q?fix(review):=20close=20round=204=20?= =?UTF-8?q?=E2=80=94=20land-and-defer=20needs=20the=20scope=20established?= =?UTF-8?q?=20too?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The blocker: last round's gate covered two of the three unestablished shapes. An unproven scope — a chunk nobody read, an uncoverable chunk, an idle agent, context unavailable — also posts zero Criticals while prior-round blockers sit unread, and the non-repost inference then reads them as fixed. Such a round carried "cannot show that any of the diff was read" and "no Critical is open" in one body, and the artifact told a machine consumer to merge over the unreviewed chunk. The diagnosis moves below `scopeUnproven` so the gate can read it. Also: the anchor-chain sentence claimed a clean close ENDS the full-diff re-reads, which the suite's own passing tests contradict — the marker also withholds on a missing fetched sha and on a model-identity drift, both of which a cleanly-closed round can carry. It now says "until a round's marker carries an anchor again", matching the docstring beside it. And the duplicate comment I reported fixed last round was only half-fixed: the file carried the sentence twice, and I deleted the copy above the health block while leaving the one that opens the recommendations block. Removed. Six mutations. Three needed the fixtures fixed first, all the same shape: the `cannot-tell` leg was measured against a bare plan whose unproven scope withheld the code anyway, and the round's own `land-and-defer` test likewise never had an established scope. A test that cannot fail without the line it names is not a test of that line. --- .../commands/review/compose-review.test.ts | 96 +++++++++++++-- .../cli/src/commands/review/compose-review.ts | 114 ++++++++++-------- .../commands/review/lib/convergence.test.ts | 6 + .../src/commands/review/lib/convergence.ts | 4 +- .../cli/src/commands/review/save-artifact.ts | 1 - 5 files changed, 160 insertions(+), 61 deletions(-) diff --git a/packages/cli/src/commands/review/compose-review.test.ts b/packages/cli/src/commands/review/compose-review.test.ts index 23abec3c55b..73873bd5ffa 100644 --- a/packages/cli/src/commands/review/compose-review.test.ts +++ b/packages/cli/src/commands/review/compose-review.test.ts @@ -9825,15 +9825,27 @@ describe('convergence diagnosis reaches the POSTED body', () => { it('carries the matched recommendations on the composed result', () => { // The machine-readable half: a caller applies ITS policy to these codes // without parsing prose, and without this module owning a threshold. - sideFile({ - round: 4, - posted: 9, - fresh: 9, - findings: [{ id: 'R2-1', sev: 'S', file: 'src/a.ts', title: 'x' }], + // A COVERED plan: `land-and-defer` needs an established scope as well as + // an established blocker count, so a round that cannot show the diff was + // read never offers merging as an ending. + const planPath = coveredPlan(['verify', 'reverse-audit'], { + prNumber: 8255, + fetchedSha: 'deadbeef00112233', }); + writeFileSync( + join(dirname(planPath), 'qwen-review-pr-8255-prev-ledger.json'), + JSON.stringify({ + v: 1, + round: 4, + posted: 9, + fresh: 9, + findings: [{ id: 'R2-1', sev: 'S', file: 'src/a.ts', title: 'x' }], + }), + ); const r = composeReview({ - planPath: plan(), - modelId: 'm', + planPath, + env: ENV, + modelId: MODEL, criticalsInline: 0, suggestionsInline: 1, draftedComments: [ @@ -10172,18 +10184,84 @@ describe('convergence diagnosis reaches the POSTED body', () => { // Passed as a confirmed zero, the body would carry "Unresolved, please // confirm:" and "no Critical is open" at once, and the artifact would // tell a machine consumer to merge. + // A COVERED plan on purpose: with an unproven scope the sibling leg + // would withhold the code anyway, and this assertion would not be + // measuring the cannot-tell leg at all. + const planPath = coveredPlan(['verify', 'reverse-audit'], { + prNumber: 8255, + fetchedSha: 'deadbeef00112233', + }); + writeFileSync( + join(dirname(planPath), 'qwen-review-pr-8255-prev-ledger.json'), + JSON.stringify({ v: 1, round: 5, posted: 1, fresh: 1, findings: [] }), + ); + const r = composeReview({ + planPath, + env: ENV, + modelId: MODEL, + criticalsInline: 0, + suggestionsInline: 1, + cannotTellCriticals: ['a.ts:12 — an existing blocker, unruled'], + draftedComments: [ + { path: 'a.ts', line: 1, body: '**[Suggestion]** a plain nit' }, + ], + }); + expect(r.scopeUnproven).toBe(false); + expect(r.cappedBy).toContain('cannot-tell-existing-critical'); + expect(r.body).toContain('Convergence:'); + expect(r.body).not.toContain('No Critical finding is open'); + expect((r.recommendations ?? []).map((x) => x.code)).not.toContain( + 'land-and-defer', + ); + }); + + it('withholds land-and-defer while the round cannot show the diff was read', () => { + // An unproven scope means prior-round Criticals sitting in the unread + // territory are read as fixed by the non-repost inference alone. A + // machine consumer keyed on the code would be told to merge over an + // unreviewed chunk. sideFile({ round: 5, posted: 1, fresh: 1, findings: [] }); const r = composeReview({ planPath: plan(), modelId: 'm', criticalsInline: 0, suggestionsInline: 1, - cannotTellCriticals: ['a.ts:12 — an existing blocker, unruled'], draftedComments: [ { path: 'a.ts', line: 1, body: '**[Suggestion]** a plain nit' }, ], }); - expect(r.cappedBy).toContain('cannot-tell-existing-critical'); + expect(r.scopeUnproven).toBe(true); + expect(r.body).toContain('Convergence:'); + expect(r.body).not.toContain('No Critical finding is open'); + expect((r.recommendations ?? []).map((x) => x.code)).not.toContain( + 'land-and-defer', + ); + }); + + it('withholds land-and-defer while a finding is still unverified', () => { + // The second unestablished shape the gate names, and it had no test: a + // cumulative findings file still carrying an `— [unverified]` tag means + // the verifier never ruled, so the round's zero is not a confirmed zero. + const planPath = coveredPlan(['verify', 'reverse-audit'], { + prNumber: 8255, + fetchedSha: 'deadbeef00112233', + }); + writeFileSync( + join(dirname(planPath), 'qwen-review-pr-8255-prev-ledger.json'), + JSON.stringify({ v: 1, round: 5, posted: 1, fresh: 1, findings: [] }), + ); + const r = composeReview({ + planPath, + env: ENV, + modelId: MODEL, + findingsPath: findingsFile(TAGGED), + criticalsInline: 0, + suggestionsInline: 1, + draftedComments: [ + { path: 'a.ts', line: 1, body: '**[Suggestion]** a plain nit' }, + ], + }); + expect(r.cappedBy).toContain('findings-unverified-at-compose'); expect(r.body).toContain('Convergence:'); expect(r.body).not.toContain('No Critical finding is open'); expect((r.recommendations ?? []).map((x) => x.code)).not.toContain( diff --git a/packages/cli/src/commands/review/compose-review.ts b/packages/cli/src/commands/review/compose-review.ts index 10d58074397..10fcb0c49b9 100644 --- a/packages/cli/src/commands/review/compose-review.ts +++ b/packages/cli/src/commands/review/compose-review.ts @@ -2651,6 +2651,55 @@ function composeReviewBody( // last `bodyCriticals.push`, because the stray-marker leg and the // script-lint gate both add blockers after the list is declared. const openCriticals = criticalsInline + bodyCriticals.length; + + // `C` counts every Critical the review posts anywhere — inline or body. + // `S` counts every *confirmed* Suggestion — anchored, discarded, or dropped + // as an already-reported duplicate: the verdict reflects the findings the + // review confirmed, not the ones that anchored or were worth re-posting, so + // neither dropping every anchor nor every duplicate may upgrade the event + // to APPROVE. + const c = openCriticals; + const s = + suggestionsInline + + suggestionsDiscarded + + suggestionsDroppedAsDuplicates.length; + + const baseEvent: ReviewEvent = + c >= 1 ? 'REQUEST_CHANGES' : s >= 1 ? 'COMMENT' : 'APPROVE'; + + // Caps: states outside this run's confirmed count that forbid an + // approval. A REQUEST_CHANGES earned by a confirmed Critical is never + // softened by them. + const cappedBy: string[] = []; + if (cannotTell.length > 0) cappedBy.push('cannot-tell-existing-critical'); + if (missingReceipts.length > 0) cappedBy.push('chunk-nobody-read'); + if (uncoverable.length > 0) cappedBy.push('uncoverable-chunk'); + if (unreviewed.length + coverageEntries.length > 0) { + cappedBy.push('unreviewed-dimension'); + } + if (contextUnavailable) cappedBy.push('context-unavailable'); + if (unlicensedDeferral !== null) cappedBy.push('unlicensed-deferral'); + if (criticalsUnverified) cappedBy.push('criticals-unverified'); + if (findingsUnverifiedAtCompose) { + cappedBy.push('findings-unverified-at-compose'); + } + + // Is there any doubt that the whole diff was READ? That is a narrower + // question than "did anything cap the verdict", and it is the only one the + // incremental anchor needs — see `ledgerMarkerFor`. Every entry counted here + // is machine-derived (recomputed from the harness's own transcripts a few + // hundred lines above), never the orchestrator's prose: an agent that made + // no tool call, one launched without the diff in its prompt, one that never + // opened it, a chunk with no receipt, a plan or transcript set that could + // not be read, a context fetch that failed. `budgetEntry` is excluded on + // purpose — a disclosed budget gap is the ceiling working, and it says + // something about DEPTH, not about which lines were read. + const scopeUnproven = + missingReceipts.length > 0 || + uncoverable.length > 0 || + contextUnavailable || + coverageEntries.some((entry) => entry !== budgetEntry); + const diagnosis = convergence ? diagnoseConvergence({ // Clamped like every other public round surface in this function — @@ -2679,7 +2728,22 @@ function composeReviewBody( // count is not a count of none. Passed anyway, the body would carry // "Unresolved, please confirm:" and "no Critical is open" at once, // and the artifact would tell a machine consumer to merge. - ...(cannotTell.length === 0 && !findingsUnverifiedAtCompose + // Only when the blocker state is ESTABLISHED — which needs the + // SCOPE established too. A round capped + // `cannot-tell-existing-critical` posts zero Criticals precisely + // BECAUSE existing ones could not be ruled on; + // `findings-unverified-at-compose` is the same shape; and an + // unproven scope (a chunk nobody read, an uncoverable chunk, an idle + // agent, context unavailable) means prior-round Criticals sitting in + // the unread territory are read as fixed by the non-repost inference + // alone. The module's own rule then withholds `land-and-defer`: an + // absent count is not a count of none. Passed anyway, the body would + // carry "cannot show that any of the diff was read" and "no Critical + // is open" at once, and the artifact would tell a machine consumer + // to merge over an unreviewed chunk. + ...(cannotTell.length === 0 && + !findingsUnverifiedAtCompose && + !scopeUnproven ? { openCriticals } : {}), }) @@ -2710,54 +2774,6 @@ function composeReviewBody( : undefined; const recommendations = diagnosis ? recommendationsFor(diagnosis) : undefined; - // `C` counts every Critical the review posts anywhere — inline or body. - // `S` counts every *confirmed* Suggestion — anchored, discarded, or dropped - // as an already-reported duplicate: the verdict reflects the findings the - // review confirmed, not the ones that anchored or were worth re-posting, so - // neither dropping every anchor nor every duplicate may upgrade the event - // to APPROVE. - const c = openCriticals; - const s = - suggestionsInline + - suggestionsDiscarded + - suggestionsDroppedAsDuplicates.length; - - const baseEvent: ReviewEvent = - c >= 1 ? 'REQUEST_CHANGES' : s >= 1 ? 'COMMENT' : 'APPROVE'; - - // Caps: states outside this run's confirmed count that forbid an - // approval. A REQUEST_CHANGES earned by a confirmed Critical is never - // softened by them. - const cappedBy: string[] = []; - if (cannotTell.length > 0) cappedBy.push('cannot-tell-existing-critical'); - if (missingReceipts.length > 0) cappedBy.push('chunk-nobody-read'); - if (uncoverable.length > 0) cappedBy.push('uncoverable-chunk'); - if (unreviewed.length + coverageEntries.length > 0) { - cappedBy.push('unreviewed-dimension'); - } - if (contextUnavailable) cappedBy.push('context-unavailable'); - if (unlicensedDeferral !== null) cappedBy.push('unlicensed-deferral'); - if (criticalsUnverified) cappedBy.push('criticals-unverified'); - if (findingsUnverifiedAtCompose) { - cappedBy.push('findings-unverified-at-compose'); - } - - // Is there any doubt that the whole diff was READ? That is a narrower - // question than "did anything cap the verdict", and it is the only one the - // incremental anchor needs — see `ledgerMarkerFor`. Every entry counted here - // is machine-derived (recomputed from the harness's own transcripts a few - // hundred lines above), never the orchestrator's prose: an agent that made - // no tool call, one launched without the diff in its prompt, one that never - // opened it, a chunk with no receipt, a plan or transcript set that could - // not be read, a context fetch that failed. `budgetEntry` is excluded on - // purpose — a disclosed budget gap is the ceiling working, and it says - // something about DEPTH, not about which lines were read. - const scopeUnproven = - missingReceipts.length > 0 || - uncoverable.length > 0 || - contextUnavailable || - coverageEntries.some((entry) => entry !== budgetEntry); - // Is every dimension gap the orchestrator disclosed about DEPTH rather than // about which lines were read? // diff --git a/packages/cli/src/commands/review/lib/convergence.test.ts b/packages/cli/src/commands/review/lib/convergence.test.ts index c08f4aafba2..64121da3517 100644 --- a/packages/cli/src/commands/review/lib/convergence.test.ts +++ b/packages/cli/src/commands/review/lib/convergence.test.ts @@ -768,6 +768,12 @@ describe('renderMechanismHealth — is the machinery working', () => { })!; expect(r.en).toContain('re-reads the whole diff'); expect(r.zh).toContain('重读整个 diff'); + // The termination condition is "an anchor again", not "a clean close": + // the marker also withholds on a missing fetched sha and on a model + // identity drift, both of which a cleanly-closed round can carry. + expect(r.en).toContain("until a round's marker carries an anchor again"); + expect(r.en).not.toContain('until a round closes cleanly'); + expect(r.zh).toContain('直到某一轮的标记重新带上锚点'); // The design once prescribed a re-anchor round here; the measurements // did not bear out its premise, so the shape is disclosed and nothing // is recommended. diff --git a/packages/cli/src/commands/review/lib/convergence.ts b/packages/cli/src/commands/review/lib/convergence.ts index 962238f2d3a..10373fd9fad 100644 --- a/packages/cli/src/commands/review/lib/convergence.ts +++ b/packages/cli/src/commands/review/lib/convergence.ts @@ -640,10 +640,10 @@ export function renderMechanismHealth( } if (h.anchorChainBroken) { en.push( - `this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had none either, so the next review re-reads the whole diff and will keep doing so until a round closes cleanly`, + `this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had none either, so the next review re-reads the whole diff and will keep doing so until a round's marker carries an anchor again`, ); zh.push( - `本轮未能干净收尾,因而扣留了增量锚点,而它恢复到的那一轮也没有锚点,因此下一次评审将重读整个 diff——并会一直如此,直到某一轮干净收尾`, + `本轮未能干净收尾,因而扣留了增量锚点,而它恢复到的那一轮也没有锚点,因此下一次评审将重读整个 diff——并会一直如此,直到某一轮的标记重新带上锚点`, ); } if (en.length === 0) return null; diff --git a/packages/cli/src/commands/review/save-artifact.ts b/packages/cli/src/commands/review/save-artifact.ts index 14455af0457..b2844362f10 100644 --- a/packages/cli/src/commands/review/save-artifact.ts +++ b/packages/cli/src/commands/review/save-artifact.ts @@ -327,7 +327,6 @@ function validateVerdict(value: unknown): PersistedVerdict { zh: string(c['zh'], 'Composed verdict.convergence.zh'), }; } - // The fresh count reads by the same rules as the total it is part of. // The machine-readable half of the observation. Dropped by this // allow-list, a caller reading the durable record sees the prose and not // the codes it would key on. From bc798bec9b740955d0f651d1bb9aad9bdaf81585 Mon Sep 17 00:00:00 2001 From: wenshao Date: Fri, 21 Aug 2026 23:48:47 +0800 Subject: [PATCH 6/8] fix(review): one gate for land-and-defer, with every leg named once MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three rounds added one leg at a time to the same gate, and each addition left the previous rationale describing a gate that no longer existed. Round 5 found three more legs missing. This replaces the stack with one condition and one comment listing every leg, so the next one cannot be added in a place the others do not mention. The legs round 5 named: - a whiffed dimension withholds the anchor but was not withheld here, so the artifact told a machine consumer to merge over lines nobody re-read. The gate now reads the marker's OWN `anchorFailsClosed` predicate rather than enumerating its legs — which also SUBSUMES the two blocker states the gate listed separately, since both are caps and neither is `unreviewed-dimension`. Those conjuncts are removed rather than left as dead code that reads like extra protection. - a truncated work list makes the non-repost inference unsound: a shed Critical is neither re-posted nor ruled on. The same `complete` flag the freshness rule already reads. - a pure-foreign list holds none of this account's entries, so its own open Criticals cannot be re-posted at all. Also the three stale comments this branch left behind: the superseded "until one closes cleanly" wording in the `PrevRound.anchored` docstring and in a test comment (the rendered text and its pin say "until a round's marker carries an anchor again", and the negative pin is widened to a regex that catches both spellings); and the health computation's claim that `cappedBy` is still being appended below it, which the previous round's move made false — it now names the real constraint, `dimensionGapsAreDepthOnly`. And the fixture whose own numbers proved the list incomplete (`fresh: 9` over a one-entry work list) no longer blesses an inference conditioned on completeness; it uses a shape the pipeline's writer can produce. Six mutations, one per leg, each verified to turn a named test red. The new arms are table-driven from the shape that DOES offer the ending, flipping exactly one leg per arm — the fixture failure that let three of these legs ship unpinned was arms that would have withheld the code anyway. --- .../commands/review/compose-review.test.ts | 58 +++++- .../cli/src/commands/review/compose-review.ts | 170 ++++++++++-------- .../commands/review/lib/convergence.test.ts | 4 +- .../src/commands/review/lib/convergence.ts | 4 +- 4 files changed, 156 insertions(+), 80 deletions(-) diff --git a/packages/cli/src/commands/review/compose-review.test.ts b/packages/cli/src/commands/review/compose-review.test.ts index 73873bd5ffa..451cc0f4de9 100644 --- a/packages/cli/src/commands/review/compose-review.test.ts +++ b/packages/cli/src/commands/review/compose-review.test.ts @@ -9834,11 +9834,17 @@ describe('convergence diagnosis reaches the POSTED body', () => { }); writeFileSync( join(dirname(planPath), 'qwen-review-pr-8255-prev-ledger.json'), + // A shape the pipeline's own writer can produce: `buildLedger` records + // every posted finding, so `fresh` never exceeds the work list absent + // `dropped`. The assertions turn on the cluster leg and the blocker + // count, so this changes nothing they measure — but a fixture whose + // own numbers prove the list incomplete must not be the one that + // blesses an inference conditioned on it being complete. JSON.stringify({ v: 1, round: 4, posted: 9, - fresh: 9, + fresh: 1, findings: [{ id: 'R2-1', sev: 'S', file: 'src/a.ts', title: 'x' }], }), ); @@ -9971,7 +9977,8 @@ describe('convergence diagnosis reaches the POSTED body', () => { it('discloses an anchor chain that has stopped', () => { // Two consecutive withholds mean every later round re-reads the whole - // diff until one closes cleanly — the closed loop measured at 119 + // diff until a round's marker carries an anchor again — the closed loop + // measured at 119 // minutes on a PR whose code had not changed a line. The plan here // names no fetched sha and the round caps, so this round withholds too. sideFile({ round: 4, posted: 9, fresh: 9, findings: [] }); @@ -10269,6 +10276,53 @@ describe('convergence diagnosis reaches the POSTED body', () => { ); }); + it.each([ + [ + 'a whiffed dimension', + { unreviewedDimensions: ['security — the relaunch returned nothing'] }, + {}, + ], + ['a truncated work list', {}, { dropped: 3 }], + ['a pure-foreign work list', {}, { foreign: true }], + ])('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 + // that would have withheld the code anyway. + const planPath = coveredPlan(['verify', 'reverse-audit'], { + prNumber: 8255, + fetchedSha: 'deadbeef00112233', + }); + const side = { + v: 1, + round: 4, + posted: 9, + fresh: 1, + findings: [{ id: 'R2-1', sev: 'S', file: 'src/a.ts', title: 'x' }], + ...sideOver, + }; + writeFileSync( + join(dirname(planPath), 'qwen-review-pr-8255-prev-ledger.json'), + JSON.stringify(side), + ); + const r = composeReview({ + planPath, + env: ENV, + modelId: MODEL, + criticalsInline: 0, + suggestionsInline: 1, + draftedComments: [ + { path: 'src/a.ts', line: 1, body: '**[Suggestion]** again' }, + ], + ...inputOver, + }); + // The paragraph still renders — only the ending is withheld. + expect(r.body).toContain('Convergence:'); + expect(r.body).not.toContain('No Critical finding is open'); + expect((r.recommendations ?? []).map((x) => x.code)).not.toContain( + 'land-and-defer', + ); + }); + it('names an auto-resolved floor the way the enforcement note does', () => { // `auto` is the DEFAULT, so the explicit-flag wording claims a flag that // was never passed — beside a floor-enforcement note in the same body diff --git a/packages/cli/src/commands/review/compose-review.ts b/packages/cli/src/commands/review/compose-review.ts index 10fcb0c49b9..4707e79904a 100644 --- a/packages/cli/src/commands/review/compose-review.ts +++ b/packages/cli/src/commands/review/compose-review.ts @@ -2700,80 +2700,6 @@ function composeReviewBody( contextUnavailable || coverageEntries.some((entry) => entry !== budgetEntry); - const diagnosis = convergence - ? diagnoseConvergence({ - // Clamped like every other public round surface in this function — - // the ledger marker stamp and the deferred-posture clause both clamp - // identically. An unclamped `+1` at the cap names round 10001 in the - // posted prose beside a marker stamping 10000, with this round's own - // findings stamped `R10000-*`. - round: Math.min(prevRound + 1, LEDGER_MAX_ROUND), - // The SAME count the marker and the VOLUME line carry, not a second - // derivation of it. - posted: postedInline, - prev: convergence.prev, - drafts: draftedFindingsOf(input.draftedComments), - ...(convergence.floor === undefined - ? {} - : { floor: convergence.floor }), - ...(convergence.criticalFloorKind === undefined - ? {} - : { criticalFloorKind: convergence.criticalFloorKind }), - // Only when the blocker state is ESTABLISHED. A round capped - // `cannot-tell-existing-critical` posts zero Criticals precisely - // BECAUSE existing ones could not be ruled on — the entries ride - // their own channel, are never counted here, and were never shown - // fixed — and `findings-unverified-at-compose` is the same shape. - // The module's own rule then withholds `land-and-defer`: an absent - // count is not a count of none. Passed anyway, the body would carry - // "Unresolved, please confirm:" and "no Critical is open" at once, - // and the artifact would tell a machine consumer to merge. - // Only when the blocker state is ESTABLISHED — which needs the - // SCOPE established too. A round capped - // `cannot-tell-existing-critical` posts zero Criticals precisely - // BECAUSE existing ones could not be ruled on; - // `findings-unverified-at-compose` is the same shape; and an - // unproven scope (a chunk nobody read, an uncoverable chunk, an idle - // agent, context unavailable) means prior-round Criticals sitting in - // the unread territory are read as fixed by the non-repost inference - // alone. The module's own rule then withholds `land-and-defer`: an - // absent count is not a count of none. Passed anyway, the body would - // carry "cannot show that any of the diff was read" and "no Critical - // is open" at once, and the artifact would tell a machine consumer - // to merge over an unreviewed chunk. - ...(cannotTell.length === 0 && - !findingsUnverifiedAtCompose && - !scopeUnproven - ? { openCriticals } - : {}), - }) - : null; - // A fact about the round, not about the diagnosis: it rides in the marker - // whether or not a signal fired, because the NEXT round's trend needs this - // round's point either way. - const carriedIds = convergence - ? new Set( - convergence.prev.findings - .map((f) => f?.id) - .filter((id): id is string => typeof id === 'string'), - ) - : undefined; - const postedFresh = - volumeOf( - draftedFindingsOf(input.draftedComments).filter((d) => - isFreshDraft( - d, - Math.min(prevRound + 1, LEDGER_MAX_ROUND), - carriedIds, - convergence?.prev.complete === true, - ), - ).length, - ) ?? 0; - const convergenceNote = diagnosis - ? renderConvergenceDiagnosis(diagnosis) - : undefined; - const recommendations = diagnosis ? recommendationsFor(diagnosis) : undefined; - // Is every dimension gap the orchestrator disclosed about DEPTH rather than // about which lines were read? // @@ -2825,6 +2751,95 @@ function composeReviewBody( ...splicedForBudgetPhrase, ].every((entry) => isNonDiffDimensionGap(entry) || isRelayedStopEntry(entry)); + const diagnosis = convergence + ? diagnoseConvergence({ + // Clamped like every other public round surface in this function — + // the ledger marker stamp and the deferred-posture clause both clamp + // identically. An unclamped `+1` at the cap names round 10001 in the + // posted prose beside a marker stamping 10000, with this round's own + // findings stamped `R10000-*`. + round: Math.min(prevRound + 1, LEDGER_MAX_ROUND), + // The SAME count the marker and the VOLUME line carry, not a second + // derivation of it. + posted: postedInline, + prev: convergence.prev, + drafts: draftedFindingsOf(input.draftedComments), + ...(convergence.floor === undefined + ? {} + : { floor: convergence.floor }), + ...(convergence.criticalFloorKind === undefined + ? {} + : { criticalFloorKind: convergence.criticalFloorKind }), + // Passed ONLY when this round established BOTH what it reviewed and + // what blockers remain. `land-and-defer` rests on one inference — + // "a Critical in the previous work list this round does not re-post + // was fixed" — and every leg below is a state where that inference + // is unsound, so the module's own "an absent count is not a count of + // none" rule withholds the code. + // + // Named in ONE place because they were added one at a time over + // three review rounds, and each addition left the previous rationale + // describing a gate that no longer existed: + // + // - `anchorFailsClosed`: the round cannot certify the lines it read + // — unproven scope, a whiffed dimension, or any verdict cap other + // than an unreviewable one. Prior-round Criticals sitting in the + // territory nobody re-read are then "not re-posted" for a reason + // that is not "fixed". Read through the marker's OWN predicate so + // a leg added there cannot be forgotten here — and it already + // SUBSUMES the two blocker states this gate first listed + // separately: `cannot-tell-existing-critical` and + // `findings-unverified-at-compose` are both caps, and neither is + // `unreviewed-dimension`, so each fails the predicate on its own. + // Listing them again would be dead conjuncts that read as extra + // protection. + // - a work list that is not COMPLETE: shed entries are unknown, so a + // Critical that fell out of the ledger is neither re-posted nor + // ruled on. The same flag the freshness rule already reads. + // - 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. + // + // Passed anyway, the body carries "no Critical is open" beside its + // own disclosure of what it could not read, and the artifact tells a + // machine consumer to merge. + ...(!anchorFailsClosed( + cappedBy, + scopeUnproven, + dimensionGapsAreDepthOnly, + ) && + convergence.prev.complete === true && + !(convergence.prev.foreign === true && convergence.prev.merged !== true) + ? { openCriticals } + : {}), + }) + : null; + // A fact about the round, not about the diagnosis: it rides in the marker + // whether or not a signal fired, because the NEXT round's trend needs this + // round's point either way. + const carriedIds = convergence + ? new Set( + convergence.prev.findings + .map((f) => f?.id) + .filter((id): id is string => typeof id === 'string'), + ) + : undefined; + const postedFresh = + volumeOf( + draftedFindingsOf(input.draftedComments).filter((d) => + isFreshDraft( + d, + Math.min(prevRound + 1, LEDGER_MAX_ROUND), + carriedIds, + convergence?.prev.complete === true, + ), + ).length, + ) ?? 0; + const convergenceNote = diagnosis + ? renderConvergenceDiagnosis(diagnosis) + : undefined; + const recommendations = diagnosis ? recommendationsFor(diagnosis) : undefined; + let event: ReviewEvent = baseEvent; if (event === 'APPROVE' && cappedBy.length > 0) event = 'COMMENT'; // The caps that reach a Request changes — because they remove the premise @@ -3774,8 +3789,11 @@ function composeReviewBody( // Is the MECHANISM working? A pipeline that has stopped and one with // nothing to do are both silent, so the round says what it can see about // its own machinery. Computed here, after the caps are final: the anchor - // decision reads them, and `cappedBy` is still being appended to well - // below where the diagnosis is composed. + // decision reads `dimensionGapsAreDepthOnly`, which is computed after the + // caps and after the event demotion. (`cappedBy` itself is complete far + // above this point — every push site sits with the cap block. A later cap + // added below the demotion would keep an APPROVE that must be capped, so + // this comment does not license one.) const healthNote = convergence ? renderMechanismHealth({ // Nominally engaged, mechanically not: the floor resolved to diff --git a/packages/cli/src/commands/review/lib/convergence.test.ts b/packages/cli/src/commands/review/lib/convergence.test.ts index 64121da3517..88c75f29dcc 100644 --- a/packages/cli/src/commands/review/lib/convergence.test.ts +++ b/packages/cli/src/commands/review/lib/convergence.test.ts @@ -772,7 +772,9 @@ describe('renderMechanismHealth — is the machinery working', () => { // the marker also withholds on a missing fetched sha and on a model // identity drift, both of which a cleanly-closed round can carry. expect(r.en).toContain("until a round's marker carries an anchor again"); - expect(r.en).not.toContain('until a round closes cleanly'); + // Broad on purpose: the superseded wording drifted into two comments + // as "until one closes cleanly", which an exact-string pin missed. + expect(r.en).not.toMatch(/until (a round|one) closes cleanly/); expect(r.zh).toContain('直到某一轮的标记重新带上锚点'); // The design once prescribed a re-anchor round here; the measurements // did not bear out its premise, so the shape is disclosed and nothing diff --git a/packages/cli/src/commands/review/lib/convergence.ts b/packages/cli/src/commands/review/lib/convergence.ts index 10373fd9fad..f526a56b5b9 100644 --- a/packages/cli/src/commands/review/lib/convergence.ts +++ b/packages/cli/src/commands/review/lib/convergence.ts @@ -134,7 +134,9 @@ export interface PrevRound { /** * Whether it carried an incremental anchor. Read only by the * mechanism-health check: two consecutive withholds mean every later round - * re-reads the whole diff until one closes cleanly. + * re-reads the whole diff until a round's marker carries an anchor again — + * which a clean close does not guarantee, because the marker also + * withholds on a missing fetched sha and on a model-identity drift. */ anchored?: boolean; } From 771e52e375664c78794e665b1b69270a5b73f581 Mon Sep 17 00:00:00 2001 From: wenshao Date: Fri, 21 Aug 2026 19:04:01 +0000 Subject: [PATCH 7/8] test(review): pin the two land-and-defer gate legs the mutants survived Co-authored-by: Qwen-Coder --- .../commands/review/compose-review.test.ts | 81 +++++++++++++++++++ 1 file changed, 81 insertions(+) diff --git a/packages/cli/src/commands/review/compose-review.test.ts b/packages/cli/src/commands/review/compose-review.test.ts index 451cc0f4de9..9634337b6ae 100644 --- a/packages/cli/src/commands/review/compose-review.test.ts +++ b/packages/cli/src/commands/review/compose-review.test.ts @@ -10323,6 +10323,87 @@ describe('convergence diagnosis reaches the POSTED body', () => { ); }); + it('still offers the ending when the only cap is the depth-only dimension', () => { + // The positive side of the gate's `!anchorFailsClosed` conjunct: the + // build-and-test dimension gap caps every round in this repository, and + // the gate passes `openCriticals` through it — tightened to + // `cappedBy.length === 0`, the machine-readable merge ending would never + // fire in production and nothing would redden. + const planPath = coveredPlan(['verify', 'reverse-audit'], { + prNumber: 8255, + fetchedSha: 'deadbeef00112233', + }); + writeFileSync( + join(dirname(planPath), 'qwen-review-pr-8255-prev-ledger.json'), + JSON.stringify({ + v: 1, + round: 4, + posted: 9, + fresh: 1, + findings: [{ id: 'R2-1', sev: 'S', file: 'src/a.ts', title: 'x' }], + }), + ); + const r = composeReview({ + planPath, + env: ENV, + modelId: MODEL, + criticalsInline: 0, + suggestionsInline: 1, + draftedComments: [ + { path: 'src/a.ts', line: 1, body: '**[Suggestion]** again' }, + ], + unreviewedDimensions: [ + 'build-and-test — the integration suite never ran', + ], + }); + expect(r.cappedBy).toEqual(['unreviewed-dimension']); + expect(r.dimensionGapsAreDepthOnly).toBe(true); + expect(parseLedger(r.body)?.sha).toBe('deadbeef00112233'); + expect(r.body).toContain('No Critical finding is open'); + expect((r.recommendations ?? []).map((x) => x.code)).toContain( + 'land-and-defer', + ); + }); + + it('still offers the ending over a foreign work list merged over this one', () => { + // The provenance leg withholds on a PURE-FOREIGN list — this account's + // entries are in no work list at all — but a MERGED foreign list + // protects them under their own ids. Simplified to `foreign !== true`, + // the ending would silently disappear from rounds whose merged list is + // complete and certified. + const planPath = coveredPlan(['verify', 'reverse-audit'], { + prNumber: 8255, + fetchedSha: 'deadbeef00112233', + }); + writeFileSync( + join(dirname(planPath), 'qwen-review-pr-8255-prev-ledger.json'), + JSON.stringify({ + v: 1, + round: 4, + posted: 9, + fresh: 1, + foreign: true, + merged: true, + findings: [{ id: 'R2-1', sev: 'S', file: 'src/a.ts', title: 'x' }], + }), + ); + const r = composeReview({ + planPath, + env: ENV, + modelId: MODEL, + criticalsInline: 0, + suggestionsInline: 1, + draftedComments: [ + { path: 'src/a.ts', line: 1, body: '**[Suggestion]** again' }, + ], + }); + expect(r.body).toContain("merged over this account's own entries"); + expect(r.body).toContain('No Critical finding is open'); + expect((r.recommendations ?? []).map((x) => x.code)).toContain( + 'land-and-defer', + ); + }); + it('names an auto-resolved floor the way the enforcement note does', () => { // `auto` is the DEFAULT, so the explicit-flag wording claims a flag that // was never passed — beside a floor-enforcement note in the same body From 78f1e3031a6c6cb3c0408e69b78ea0de5920852a Mon Sep 17 00:00:00 2001 From: wenshao Date: Fri, 21 Aug 2026 21:31:10 +0000 Subject: [PATCH 8/8] test(review): pin the draft projections directly, one ledger setup helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 6's two actionable suggestions, both test-side: - `deferrableSuggestionsInline` and `draftedFindingsOf` were pinned only through composeReview. Both are exported and unit-tested directly: the severity gate, the claim-line-only deterministic-tag window, the pathless exclusion, the LEDGER_MAX_ID bound, the first-keeps dedupe, and a parity test asserting the count equals the exact set `floorEnforcedReroute` moves when the floor engages — the divergence the suggestion names. - The identical covered-plan + prev-ledger setup pasted at 12 sites collapses to one `coveredWithLedger()` beside `coveredPlan()`, deriving the side-file name from the same prNumber. A typo in a hand-coupled name failed nothing — the reader swallows ENOENT and the test silently measured round 1; broken at the one place now, 13 tests redden. Five mutation probes (helper name, pathless gate, deterministic gate, id bound, dedupe), each verified red and restored to green. Co-authored-by: Qwen-Coder --- .../commands/review/compose-review.test.ts | 353 +++++++++++------- .../cli/src/commands/review/compose-review.ts | 4 +- 2 files changed, 228 insertions(+), 129 deletions(-) diff --git a/packages/cli/src/commands/review/compose-review.test.ts b/packages/cli/src/commands/review/compose-review.test.ts index 9634337b6ae..a9a7347ba9e 100644 --- a/packages/cli/src/commands/review/compose-review.test.ts +++ b/packages/cli/src/commands/review/compose-review.test.ts @@ -30,6 +30,7 @@ import { getGhHost, setGhHost } from './lib/gh.js'; import { BRIEFS } from './lib/agent-briefs.js'; import { LEDGER_MAX_FILE, + LEDGER_MAX_ID, LEDGER_MAX_ROUND, LEDGER_MAX_VOLUME, parseLedger, @@ -38,6 +39,9 @@ import { import { countInlineFindings } from './lib/inline-counts.js'; import { composeReview, + deferrableSuggestionsInline, + draftedFindingsOf, + floorEnforcedReroute, isNonDiffDimensionGap, buildLedger, repositoryContextGate, @@ -468,6 +472,26 @@ function coveredPlan( return p; } +/** + * `coveredPlan()` with the previous round's ledger on disk beside it. The + * side-file name is derived from the same `prNumber` the plan carries: the + * reader swallows ENOENT, so a name spelled independently at a call site + * can typo into an unread side file — and the test then silently measures + * round 1 instead of the leg its assertions claim to pin. + */ +function coveredWithLedger(prev: Record): string { + const prNumber = 8255; + const p = coveredPlan(['verify', 'reverse-audit'], { + prNumber, + fetchedSha: 'deadbeef00112233', + }); + writeFileSync( + join(dirname(p), `qwen-review-pr-${prNumber}-prev-ledger.json`), + JSON.stringify(prev), + ); + return p; +} + /** Agents given the diff, that never opened it — and said so at length. */ function idlePlan(): string { transcript('a1', goodPrompt(1), { @@ -6446,14 +6470,7 @@ describe('composeReview — convergence-posture deferrals (typed channel; disclo // stays on the record and the incremental anchor still rides. And the // opener must not claim "No issues found" over findings the same body // lists two paragraphs down. - const planPath = coveredPlan(['verify', 'reverse-audit'], { - prNumber: 8255, - fetchedSha: 'deadbeef00112233', - }); - writeFileSync( - join(dirname(planPath), 'qwen-review-pr-8255-prev-ledger.json'), - JSON.stringify({ v: 1, round: 5, findings: [] }), - ); + const planPath = coveredWithLedger({ v: 1, round: 5, findings: [] }); const r = composeReview({ planPath, env: ENV, @@ -6491,14 +6508,11 @@ describe('composeReview — convergence-posture deferrals (typed channel; disclo // which round this is. The sibling test above pins the marker's // round-trip at the cap; without THIS pin the Math.min mutation on the // clause side ships green. - const planPath = coveredPlan(['verify', 'reverse-audit'], { - prNumber: 8255, - fetchedSha: 'deadbeef00112233', + const planPath = coveredWithLedger({ + v: 1, + round: LEDGER_MAX_ROUND, + findings: [], }); - writeFileSync( - join(dirname(planPath), 'qwen-review-pr-8255-prev-ledger.json'), - JSON.stringify({ v: 1, round: LEDGER_MAX_ROUND, findings: [] }), - ); const r = composeReview({ planPath, env: ENV, @@ -6877,14 +6891,7 @@ describe('composeReview — convergence-posture deferrals (typed channel; disclo // round it derives itself — this pins the legal rounds-2-5 shape end to // end (a round-resolved `suggestion` would have been refused as the // operator's override — the shipped round-5 regression). - const planPath = coveredPlan(['verify', 'reverse-audit'], { - prNumber: 8255, - fetchedSha: 'deadbeef00112233', - }); - writeFileSync( - join(dirname(planPath), 'qwen-review-pr-8255-prev-ledger.json'), - JSON.stringify({ v: 1, round: 2, findings: [] }), - ); + const planPath = coveredWithLedger({ v: 1, round: 2, findings: [] }); const r = composeReview({ planPath, env: ENV, @@ -9341,20 +9348,13 @@ describe('convergence diagnosis reaches the POSTED body', () => { // COMMENT either way. REQUEST_CHANGES — unfixed Criticals, round after // round — is the feature's primary audience, and its copy of the list // was unasserted: deleting the splice left the whole suite green. - const planPath = coveredPlan(['verify', 'reverse-audit'], { - prNumber: 8255, - fetchedSha: 'deadbeef00112233', + const planPath = coveredWithLedger({ + v: 1, + round: 4, + posted: 9, + fresh: 9, + findings: [{ id: 'R2-1', sev: 'C', file: 'src/a.ts', title: 'x' }], }); - writeFileSync( - join(dirname(planPath), 'qwen-review-pr-8255-prev-ledger.json'), - JSON.stringify({ - v: 1, - round: 4, - posted: 9, - fresh: 9, - findings: [{ id: 'R2-1', sev: 'C', file: 'src/a.ts', title: 'x' }], - }), - ); const r = composeReview({ planPath, env: ENV, @@ -9828,26 +9828,19 @@ describe('convergence diagnosis reaches the POSTED body', () => { // A COVERED plan: `land-and-defer` needs an established scope as well as // an established blocker count, so a round that cannot show the diff was // read never offers merging as an ending. - const planPath = coveredPlan(['verify', 'reverse-audit'], { - prNumber: 8255, - fetchedSha: 'deadbeef00112233', + // A shape the pipeline's own writer can produce: `buildLedger` records + // every posted finding, so `fresh` never exceeds the work list absent + // `dropped`. The assertions turn on the cluster leg and the blocker + // count, so this changes nothing they measure — but a fixture whose + // own numbers prove the list incomplete must not be the one that + // blesses an inference conditioned on it being complete. + const planPath = coveredWithLedger({ + v: 1, + round: 4, + posted: 9, + fresh: 1, + findings: [{ id: 'R2-1', sev: 'S', file: 'src/a.ts', title: 'x' }], }); - writeFileSync( - join(dirname(planPath), 'qwen-review-pr-8255-prev-ledger.json'), - // A shape the pipeline's own writer can produce: `buildLedger` records - // every posted finding, so `fresh` never exceeds the work list absent - // `dropped`. The assertions turn on the cluster leg and the blocker - // count, so this changes nothing they measure — but a fixture whose - // own numbers prove the list incomplete must not be the one that - // blesses an inference conditioned on it being complete. - JSON.stringify({ - v: 1, - round: 4, - posted: 9, - fresh: 1, - findings: [{ id: 'R2-1', sev: 'S', file: 'src/a.ts', title: 'x' }], - }), - ); const r = composeReview({ planPath, env: ENV, @@ -10127,14 +10120,13 @@ describe('convergence diagnosis reaches the POSTED body', () => { it('keeps quiet on a round whose scope closed cleanly', () => { // The chain is TWO withholds. A round that anchors clears it, however // unanchored its predecessor was. - const planPath = coveredPlan(['verify', 'reverse-audit'], { - prNumber: 8255, - fetchedSha: 'deadbeef00112233', + const planPath = coveredWithLedger({ + v: 1, + round: 4, + findings: [], + posted: 0, + fresh: 0, }); - writeFileSync( - join(dirname(planPath), 'qwen-review-pr-8255-prev-ledger.json'), - JSON.stringify({ v: 1, round: 4, findings: [], posted: 0, fresh: 0 }), - ); const r = composeReview({ planPath, env: ENV, @@ -10149,20 +10141,13 @@ describe('convergence diagnosis reaches the POSTED body', () => { it('carries the codes on a REQUEST_CHANGES result too', () => { // Three separately-maintained result constructions; only one was pinned. - const planPath = coveredPlan(['verify', 'reverse-audit'], { - prNumber: 8255, - fetchedSha: 'deadbeef00112233', + const planPath = coveredWithLedger({ + v: 1, + round: 5, + posted: 9, + fresh: 9, + findings: [{ id: 'R2-1', sev: 'C', file: 'src/a.ts', title: 'x' }], }); - writeFileSync( - join(dirname(planPath), 'qwen-review-pr-8255-prev-ledger.json'), - JSON.stringify({ - v: 1, - round: 5, - posted: 9, - fresh: 9, - findings: [{ id: 'R2-1', sev: 'C', file: 'src/a.ts', title: 'x' }], - }), - ); const r = composeReview({ planPath, env: ENV, @@ -10194,14 +10179,13 @@ describe('convergence diagnosis reaches the POSTED body', () => { // A COVERED plan on purpose: with an unproven scope the sibling leg // would withhold the code anyway, and this assertion would not be // measuring the cannot-tell leg at all. - const planPath = coveredPlan(['verify', 'reverse-audit'], { - prNumber: 8255, - fetchedSha: 'deadbeef00112233', + const planPath = coveredWithLedger({ + v: 1, + round: 5, + posted: 1, + fresh: 1, + findings: [], }); - writeFileSync( - join(dirname(planPath), 'qwen-review-pr-8255-prev-ledger.json'), - JSON.stringify({ v: 1, round: 5, posted: 1, fresh: 1, findings: [] }), - ); const r = composeReview({ planPath, env: ENV, @@ -10249,14 +10233,13 @@ describe('convergence diagnosis reaches the POSTED body', () => { // The second unestablished shape the gate names, and it had no test: a // cumulative findings file still carrying an `— [unverified]` tag means // the verifier never ruled, so the round's zero is not a confirmed zero. - const planPath = coveredPlan(['verify', 'reverse-audit'], { - prNumber: 8255, - fetchedSha: 'deadbeef00112233', + const planPath = coveredWithLedger({ + v: 1, + round: 5, + posted: 1, + fresh: 1, + findings: [], }); - writeFileSync( - join(dirname(planPath), 'qwen-review-pr-8255-prev-ledger.json'), - JSON.stringify({ v: 1, round: 5, posted: 1, fresh: 1, findings: [] }), - ); const r = composeReview({ planPath, env: ENV, @@ -10288,22 +10271,14 @@ describe('convergence diagnosis reaches the POSTED body', () => { // 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 // that would have withheld the code anyway. - const planPath = coveredPlan(['verify', 'reverse-audit'], { - prNumber: 8255, - fetchedSha: 'deadbeef00112233', - }); - const side = { + const planPath = coveredWithLedger({ v: 1, round: 4, posted: 9, fresh: 1, findings: [{ id: 'R2-1', sev: 'S', file: 'src/a.ts', title: 'x' }], ...sideOver, - }; - writeFileSync( - join(dirname(planPath), 'qwen-review-pr-8255-prev-ledger.json'), - JSON.stringify(side), - ); + }); const r = composeReview({ planPath, env: ENV, @@ -10329,20 +10304,13 @@ describe('convergence diagnosis reaches the POSTED body', () => { // the gate passes `openCriticals` through it — tightened to // `cappedBy.length === 0`, the machine-readable merge ending would never // fire in production and nothing would redden. - const planPath = coveredPlan(['verify', 'reverse-audit'], { - prNumber: 8255, - fetchedSha: 'deadbeef00112233', + const planPath = coveredWithLedger({ + v: 1, + round: 4, + posted: 9, + fresh: 1, + findings: [{ id: 'R2-1', sev: 'S', file: 'src/a.ts', title: 'x' }], }); - writeFileSync( - join(dirname(planPath), 'qwen-review-pr-8255-prev-ledger.json'), - JSON.stringify({ - v: 1, - round: 4, - posted: 9, - fresh: 1, - findings: [{ id: 'R2-1', sev: 'S', file: 'src/a.ts', title: 'x' }], - }), - ); const r = composeReview({ planPath, env: ENV, @@ -10371,22 +10339,15 @@ describe('convergence diagnosis reaches the POSTED body', () => { // protects them under their own ids. Simplified to `foreign !== true`, // the ending would silently disappear from rounds whose merged list is // complete and certified. - const planPath = coveredPlan(['verify', 'reverse-audit'], { - prNumber: 8255, - fetchedSha: 'deadbeef00112233', + const planPath = coveredWithLedger({ + v: 1, + round: 4, + posted: 9, + fresh: 1, + foreign: true, + merged: true, + findings: [{ id: 'R2-1', sev: 'S', file: 'src/a.ts', title: 'x' }], }); - writeFileSync( - join(dirname(planPath), 'qwen-review-pr-8255-prev-ledger.json'), - JSON.stringify({ - v: 1, - round: 4, - posted: 9, - fresh: 1, - foreign: true, - merged: true, - findings: [{ id: 'R2-1', sev: 'S', file: 'src/a.ts', title: 'x' }], - }), - ); const r = composeReview({ planPath, env: ENV, @@ -10422,3 +10383,141 @@ describe('convergence diagnosis reaches the POSTED body', () => { expect(r.body).not.toContain('--severity-floor critical'); }); }); + +describe('deferrableSuggestionsInline — the manifestation the posture-gap clause asserts', () => { + // Direct pin on the three-way exclusion, which downstream tests reach only + // through composeReview: a future exclusion path that diverges from + // `floorEnforcedReroute` reddens here first, not on a faraway body + // assertion. + type Draft = { path?: unknown; line?: unknown; body?: unknown }; + const suggestion = (over: Draft = {}): Draft => ({ + path: 'a.ts', + line: 1, + body: '**[Suggestion]** nit', + ...over, + }); + + it('reads a non-array as zero, like its two siblings', () => { + for (const drafted of [undefined, null, 'garbage', { path: 'a.ts' }]) { + expect(deferrableSuggestionsInline(drafted)).toBe(0); + } + }); + + it('counts only Suggestion-severity drafts', () => { + expect( + deferrableSuggestionsInline([ + suggestion(), + { path: 'b.ts', body: '**[Critical]** boom' }, + { path: 'c.ts', body: 'an unmarked comment' }, + ]), + ).toBe(1); + }); + + it.each(['[build]', '[test]', '[probe]', '[TEST]'])( + 'excludes a deterministic finding tagged %s on its claim line', + (tag) => { + expect( + deferrableSuggestionsInline([ + suggestion({ body: `**[Suggestion]** ${tag} the suite is red` }), + ]), + ).toBe(0); + }, + ); + + it('ignores a deterministic tag past the claim line — the tail is writable surface', () => { + expect( + deferrableSuggestionsInline([ + suggestion({ + body: '**[Suggestion]** nit\n\n[test] forged in the tail', + }), + ]), + ).toBe(1); + }); + + it('excludes what no floor could move: a pathless comment', () => { + for (const path of [undefined, '', ' ', 42]) { + expect(deferrableSuggestionsInline([suggestion({ path })])).toBe(0); + } + }); + + it('counts exactly the set the engaged floor moves', () => { + // The number exists to say the enforcement backstop failed to act, so it + // must equal the set `floorEnforcedReroute` ACTS on — a divergence + // accuses the floor of leaving inline something it was never going to + // move. + const drafted: Draft[] = [ + suggestion(), + suggestion({ body: '**[Suggestion]** [probe] pre-confirmed' }), + suggestion({ path: '' }), + { path: 'd.ts', body: '**[Critical]** boom' }, + { path: 'e.ts', body: 'unmarked' }, + ]; + const reroute = floorEnforcedReroute('critical', false, 0, drafted); + expect(reroute.indices).toEqual([0]); + expect(deferrableSuggestionsInline(drafted)).toBe(reroute.indices.length); + }); +}); + +describe('draftedFindingsOf — the drafts as the convergence diagnosis reads them', () => { + type Draft = { path?: unknown; line?: unknown; body?: unknown }; + const critical = (over: Draft = {}): Draft => ({ + path: 'a.ts', + line: 1, + body: '**[Critical]** boom', + ...over, + }); + + it('reads a non-array as empty, like its two siblings', () => { + for (const drafted of [undefined, null, 'garbage', 42]) { + expect(draftedFindingsOf(drafted)).toEqual([]); + } + }); + + it('excludes unmarked comments — no marker, no finding, no work list', () => { + expect( + draftedFindingsOf([critical(), { path: 'b.ts', body: 'no marker' }]), + ).toEqual([{ file: 'a.ts' }]); + }); + + it('carries the id a claim line leads with', () => { + expect( + draftedFindingsOf([ + critical({ body: '**[Critical]** R2-1: still open' }), + ]), + ).toEqual([{ file: 'a.ts', carriedId: 'R2-1' }]); + }); + + it('re-mints an id past the ledger cap, the way idFor does', () => { + // Exactly at the cap the id travels; one char over it cannot enter any + // work list, so the diagnosis must read the comment as fresh — the two + // ends of the pipeline agreeing about one comment. + const atCap = `R2-${'9'.repeat(LEDGER_MAX_ID - 3)}`; + const overCap = `R2-${'9'.repeat(LEDGER_MAX_ID - 2)}`; + expect(atCap).toHaveLength(LEDGER_MAX_ID); + expect(overCap).toHaveLength(LEDGER_MAX_ID + 1); + expect( + draftedFindingsOf([ + critical({ body: `**[Critical]** ${atCap}: still open` }), + critical({ body: `**[Critical]** ${overCap}: still open` }), + ]), + ).toEqual([{ file: 'a.ts', carriedId: atCap }, { file: 'a.ts' }]); + }); + + it('dedupes a claimed id the way the ledger keeps the FIRST of them', () => { + expect( + draftedFindingsOf([ + critical({ body: '**[Critical]** R2-1: still open' }), + critical({ path: 'b.ts', body: '**[Critical]** R2-1: voiced again' }), + ]), + ).toEqual([{ file: 'a.ts', carriedId: 'R2-1' }, { file: 'b.ts' }]); + }); + + it('anchors a pathless draft to the empty string, never to a stringified seam', () => { + expect( + draftedFindingsOf([ + critical({ path: undefined }), + critical({ path: 42 }), + ]), + ).toEqual([{ file: '' }, { file: '' }]); + }); +}); diff --git a/packages/cli/src/commands/review/compose-review.ts b/packages/cli/src/commands/review/compose-review.ts index 4707e79904a..2c7364b74b3 100644 --- a/packages/cli/src/commands/review/compose-review.ts +++ b/packages/cli/src/commands/review/compose-review.ts @@ -5000,7 +5000,7 @@ function ledgerClaimLine(body: unknown): string { * A pathless comment is excluded for the same reason by a different route: * it cannot become a deferral entry at all, so no floor could have moved it. */ -function deferrableSuggestionsInline(drafted: unknown): number { +export function deferrableSuggestionsInline(drafted: unknown): number { if (!Array.isArray(drafted)) return 0; let n = 0; for (const c of drafted as Array<{ body?: unknown; path?: unknown }>) { @@ -5035,7 +5035,7 @@ function deferrableSuggestionsInline(drafted: unknown): number { * model-written state JSON, and a non-array reaching `.map` throws out of * `composeReviewBody` and loses the whole round. */ -function draftedFindingsOf(drafted: unknown): DraftedFinding[] { +export function draftedFindingsOf(drafted: unknown): DraftedFinding[] { if (!Array.isArray(drafted)) return []; const out: DraftedFinding[] = []; // Deduped exactly as `idFor` dedupes: the ledger keeps the FIRST comment