diff --git a/packages/cli/src/commands/review/compose-review.test.ts b/packages/cli/src/commands/review/compose-review.test.ts index 6fcb3384eb6..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), { @@ -2711,6 +2735,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 }); } @@ -6441,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, @@ -6486,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, @@ -6872,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, @@ -9099,7 +9111,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 +9299,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', () => { @@ -9336,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, @@ -9817,6 +9822,549 @@ 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. + // 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. + // 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' }], + }); + const r = composeReview({ + planPath, + env: ENV, + modelId: MODEL, + 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'); + + // 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(), + 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 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: [] }); + 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('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 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(), + 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 = coveredWithLedger({ + 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 = coveredWithLedger({ + 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('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. + // 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 = coveredWithLedger({ + 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, + draftedComments: [ + { path: 'a.ts', line: 1, body: '**[Suggestion]** a plain nit' }, + ], + }); + 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 = coveredWithLedger({ + 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( + 'land-and-defer', + ); + }); + + 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 = coveredWithLedger({ + v: 1, + round: 4, + posted: 9, + fresh: 1, + findings: [{ id: 'R2-1', sev: 'S', file: 'src/a.ts', title: 'x' }], + ...sideOver, + }); + 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('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 = coveredWithLedger({ + 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 = 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' }], + }); + 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 @@ -9835,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 6d153581b20..2c7364b74b3 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,29 @@ 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 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 @@ -1350,6 +1370,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 +1383,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 +1420,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 +1454,7 @@ const EMPTY_PREV_FACTS = { truncated: false, foreign: false, merged: false, + anchored: false, }; /** @@ -1467,6 +1494,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 +1576,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 +1601,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 +1641,6 @@ function ledgerMarkerFor( freshInline: number, prevPostedInline: number | undefined, floorKind: CriticalFloorKind | undefined, - floorKnown: boolean, carriedWorkList: { ids: ReadonlySet; complete: boolean }, ): string | null { try { @@ -1624,10 +1679,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 +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 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` 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. 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, @@ -1833,6 +1892,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 +1912,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 +2643,22 @@ 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; + // `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 + @@ -2720,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 @@ -2852,6 +2972,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: { @@ -3650,7 +3771,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 @@ -3663,6 +3786,58 @@ 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 `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 + // critical and Suggestion-level findings posted inline anyway. + // 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. + // + // 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 && + 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: + !convergence.prev.anchored && + (convergence.prev.round ?? 0) > 0 && + anchorFailsClosed(cappedBy, scopeUnproven, dimensionGapsAreDepthOnly), + }) + : null; + // 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 }] : []; @@ -3698,6 +3873,7 @@ function composeReviewBody( ...unlicensedDeferralBlock, ...deferredSuggestionsBlock, ...convergenceBlock, + ...healthBlock, ...continuityBlock, ...bodyCriticalBlock, ]; @@ -3719,6 +3895,10 @@ function composeReviewBody( ...(convergenceNote === undefined ? {} : { convergence: convergenceNote }), + ...(recommendations === undefined ? {} : { recommendations }), + ...(healthNote === null || healthNote === undefined + ? {} + : { health: healthNote }), bodyTrim, lowSignal, scopeUnproven, @@ -3755,7 +3935,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, ...continuityBlock, ], notReviewedParts.length || @@ -3771,6 +3962,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 +3982,10 @@ function composeReviewBody( ...(convergenceNote === undefined ? {} : { convergence: convergenceNote }), + ...(recommendations === undefined ? {} : { recommendations }), + ...(healthNote === null || healthNote === undefined + ? {} + : { health: healthNote }), bodyTrim, lowSignal, scopeUnproven, @@ -3961,6 +4157,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 +4218,10 @@ function composeReviewBody( postedInline, postedFresh, ...(convergenceNote === undefined ? {} : { convergence: convergenceNote }), + ...(recommendations === undefined ? {} : { recommendations }), + ...(healthNote === null || healthNote === undefined + ? {} + : { health: healthNote }), bodyTrim, lowSignal, scopeUnproven, @@ -4735,6 +4936,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)); }, }; @@ -4780,6 +4986,37 @@ 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. + * + * 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. + */ +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 }>) { + 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; +} + /** * This round's drafts in the shape the convergence diagnosis reads. * @@ -4798,7 +5035,7 @@ function ledgerClaimLine(body: unknown): string { * 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 @@ -4811,7 +5048,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..88c75f29dcc 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,149 @@ 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 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, + 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 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"); + // 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 + // 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 +812,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 +825,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..f526a56b5b9 100644 --- a/packages/cli/src/commands/review/lib/convergence.ts +++ b/packages/cli/src/commands/review/lib/convergence.ts @@ -129,6 +129,16 @@ 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 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; } export interface ConvergenceDiagnosis { @@ -165,11 +175,109 @@ 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. + * - `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 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 { + 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 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: 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 + * 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; +} + /** * Is this draft a finding reported for the FIRST time? * @@ -279,6 +387,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 +547,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 +570,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 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——并会一直如此，直到某一轮的标记重新带上锚点`, + ); + } + 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 +685,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 +798,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 +811,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..443727cb240 100644 --- a/packages/cli/src/commands/review/save-artifact.test.ts +++ b/packages/cli/src/commands/review/save-artifact.test.ts @@ -467,6 +467,64 @@ 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/); + + // ...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('机制健康：…'); + 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', () => { // 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..b2844362f10 100644 --- a/packages/cli/src/commands/review/save-artifact.ts +++ b/packages/cli/src/commands/review/save-artifact.ts @@ -33,6 +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 { + RECOMMENDATION_CODES, + type Recommendation, +} from './lib/convergence.js'; import { writeStderrLine, writeStdoutLine } from '../../utils/stdioHelpers.js'; interface PersistedVerdict @@ -212,6 +216,24 @@ function event(value: unknown, label: string): ReviewEvent { return value; } +/** + * 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 as readonly string[]).includes(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']; @@ -292,7 +314,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 @@ -306,6 +327,40 @@ function validateVerdict(value: unknown): PersistedVerdict { zh: string(c['zh'], 'Composed verdict.convergence.zh'), }; } + // 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: recommendationCode( + r['code'], + `Composed verdict.recommendations[${i}].code`, + ), + basis: string( + r['basis'], + `Composed verdict.recommendations[${i}].basis`, + ), + }; + }); + } + // 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'), + }; + } // 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; @@ -367,6 +422,8 @@ function validateVerdict(value: unknown): PersistedVerdict { ...(postedInline === undefined ? {} : { postedInline }), ...(postedFresh === undefined ? {} : { postedFresh }), ...(convergence === undefined ? {} : { convergence }), + ...(recommendations === undefined ? {} : { recommendations }), + ...(health === undefined ? {} : { health }), lowSignal: lowSignal === null ? null