diff --git a/packages/cli/src/commands/review/compose-review.test.ts b/packages/cli/src/commands/review/compose-review.test.ts index 6813e8c2ddd..7b409bbdab7 100644 --- a/packages/cli/src/commands/review/compose-review.test.ts +++ b/packages/cli/src/commands/review/compose-review.test.ts @@ -51,6 +51,7 @@ import { buildLedger, repositoryContextGate, scriptLintGate, + withoutGateReposts, testPlanGate, composeReviewCommand, describeChunkGap, @@ -2827,6 +2828,1312 @@ describe('composeReviewCommand handler (the CLI glue)', () => { } }); + it('surfaces the persistently-critical advisory when the loop will not converge (#9410)', async () => { + // The carried telemetry shows the shape: a Critical stood in the + // previous round's work-list, one stands again this round, and the + // two-round posting window is present and not shrinking. The advisory + // must surface on all three surfaces — the composed JSON field, the body + // disclosure, and the terminal RESIDUAL-RISK line — and it must be + // advisory-only: it never moves the event, never caps. + const dir = mkdtempSync(join(tmpdir(), 'compose-converge-')); + const inputPath = join(dir, 'compose.json'); + const commentsPath = join(dir, 'comments.json'); + const planPath = join(dir, 'plan.json'); + writeFileSync(planPath, JSON.stringify({ prNumber: 8255 }), 'utf8'); + // The operator's `auto` floor is engaged at round 7 — without an + // engaged floor the advisory's floor-futility claim is unprovable and + // the signal degrades open to silence (#9410). + writeFileSync( + inputPath, + JSON.stringify({ modelId: MODEL, planPath, severityFloor: 'auto' }), + 'utf8', + ); + // One Critical this round. + writeFileSync( + commentsPath, + JSON.stringify([ + { path: 'a.ts', line: 1, body: '**[Critical]** standing blocker' }, + ]), + 'utf8', + ); + const stderr = () => + (writeStderrLine as ReturnType).mock.calls.map((c) => + String(c[0]), + ); + const stdoutJson = () => + JSON.parse( + (writeStdoutLine as ReturnType).mock.calls + .map((c) => String(c[0])) + .join('\n'), + ) as { + residualRisk?: { + shape: string; + recommendation: string; + criticals: number; + fresh: number; + prevFresh: number; + }; + event?: string; + cappedBy?: string[]; + body?: string; + }; + try { + // The predecessor carried a Critical and posted 1; this round posts 1 + // (flat, not shrinking) — the persistently-critical conjunction. + (writeStderrLine as ReturnType).mockClear(); + (writeStdoutLine as ReturnType).mockClear(); + writeFileSync( + join(dir, 'qwen-review-pr-8255-prev-ledger.json'), + JSON.stringify({ + v: 1, + round: 6, + findings: [{ id: 'R6-1', sev: 'C', file: 'x.ts', title: 'blocker' }], + posted: 1, + fresh: 1, + }), + 'utf8', + ); + await runComposeReviewCommand({ + input: inputPath, + comments: commentsPath, + }); + // Terminal RESIDUAL-RISK line, advisory-only and self-disclaiming. + const conv = stderr().filter((l) => l.startsWith('RESIDUAL-RISK: ')); + expect(conv).toHaveLength(1); + expect(conv[0]).toContain('land-with-residual-risk'); + expect(conv[0]).toContain('does not block'); + // ONE record on a line-oriented channel, like the VOLUME line above + // it. The advisory carries a markdown table for the body, so printed + // verbatim this was one labelled line followed by six unlabelled + // ones (#9526). + expect(conv[0]).not.toContain('\n'); + // Collapsed, not dropped: the inventory's three columns still reach + // the operator on the round where the body budget sheds the table. + for (const column of [ + 'attack surface', + 'attacker-dependency', + 'blast radius', + ]) { + expect(conv[0]).toContain(column); + } + // The claim the whole conjunction exists to license, stated + // POSITIVELY — every other fixture only pins its absence, so a + // template that stopped emitting it shipped green. + expect(conv[0]).toContain('The severity floor will not converge it'); + // Structured field on the composed JSON. + const composed = stdoutJson(); + expect(composed.residualRisk).toMatchObject({ + shape: 'persistently-critical', + recommendation: 'land-with-residual-risk', + criticals: 1, + fresh: 1, + prevFresh: 1, + }); + // Body disclosure rides too, carrying the same recommendation code. + expect(composed.body).toContain('land-with-residual-risk'); + expect(composed.body).toContain( + 'The severity floor will not converge it', + ); + // ADVISORY ONLY — the guarantee the feature rests on, and the one + // nothing pinned. A fired advisory must leave the event exactly where + // the findings put it and must add nothing to `cappedBy`: this round + // stands behind an unverified Critical, so the event is the COMMENT + // the verification cap produces and the cap list names that cap and + // nothing about convergence. + expect(composed.event).toBe('COMMENT'); + expect(composed.cappedBy ?? []).not.toContain('convergence'); + expect((composed.cappedBy ?? []).join('\n')).not.toContain( + 'residual-risk', + ); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + it('stays silent on the persistently-critical advisory when the loop IS converging (#9410)', async () => { + // Same shape as above except the volume is SHRINKING — the loop is + // working its Criticals down, so no advisory fires. Every degraded arm + // (shrinking volume, no prior Critical, missing window) is fail-open to + // silence; this pins the shrinking arm end to end. + const dir = mkdtempSync(join(tmpdir(), 'compose-converge-no-')); + const inputPath = join(dir, 'compose.json'); + const commentsPath = join(dir, 'comments.json'); + const planPath = join(dir, 'plan.json'); + writeFileSync(planPath, JSON.stringify({ prNumber: 8255 }), 'utf8'); + writeFileSync( + inputPath, + JSON.stringify({ modelId: MODEL, planPath, severityFloor: 'auto' }), + 'utf8', + ); + writeFileSync( + commentsPath, + JSON.stringify([ + { path: 'a.ts', line: 1, body: '**[Critical]** standing blocker' }, + ]), + 'utf8', + ); + const stderr = () => + (writeStderrLine as ReturnType).mock.calls.map((c) => + String(c[0]), + ); + const stdoutJson = () => + JSON.parse( + (writeStdoutLine as ReturnType).mock.calls + .map((c) => String(c[0])) + .join('\n'), + ) as { residualRisk?: unknown; body?: string }; + try { + // The predecessor carried a Critical but posted MORE (3) than this + // round (1): the volume is shrinking, the loop is converging. The + // floor is engaged (round 7 of `auto`), so the silence is pinned on + // the volume arm alone, not on a missing engagement. + (writeStderrLine as ReturnType).mockClear(); + (writeStdoutLine as ReturnType).mockClear(); + writeFileSync( + join(dir, 'qwen-review-pr-8255-prev-ledger.json'), + JSON.stringify({ + v: 1, + round: 6, + findings: [{ id: 'R6-1', sev: 'C', file: 'x.ts', title: 'blocker' }], + posted: 3, + fresh: 3, + }), + 'utf8', + ); + await runComposeReviewCommand({ + input: inputPath, + comments: commentsPath, + }); + expect( + stderr().filter((l) => l.startsWith('RESIDUAL-RISK: ')), + ).toHaveLength(0); + // A POSITIVE sentinel beside the absences: `prevLedgerFacts` swallows + // every recovery failure into round 0 with no volume, which would let + // three other arms produce this same silence and leave the volume arm + // pinned by nothing. The VOLUME line quoting the predecessor proves + // the ledger really was recovered, so the silence is the shrinking + // window and not a fixture that never loaded. + expect(stderr().find((l) => l.startsWith('VOLUME: '))).toContain( + '(previous round: 3)', + ); + const composed = stdoutJson(); + expect(composed.residualRisk).toBeUndefined(); + expect(composed.body ?? '').not.toContain('land-with-residual-risk'); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + it('surfaces the advisory on a REQUEST_CHANGES round — the motivating shape (#9410)', async () => { + // The motivating shape (PR 9226): verified Criticals standing every + // round compose REQUEST_CHANGES every round. A deterministic [build] + // body Critical earns its Request changes without a verifier, so the + // event is REQUEST_CHANGES — the branch the wiring must not leave + // silent. The only Critical arrives via bodyCriticals (criticalsInline + // is 0), so the body-only term of thisCriticals is load-bearing here: + // dropping it from the sum silently un-fires the advisory. + const dir = mkdtempSync(join(tmpdir(), 'compose-converge-rc-')); + const inputPath = join(dir, 'compose.json'); + const commentsPath = join(dir, 'comments.json'); + const planPath = join(dir, 'plan.json'); + writeFileSync(planPath, JSON.stringify({ prNumber: 8255 }), 'utf8'); + writeFileSync( + inputPath, + JSON.stringify({ + modelId: MODEL, + planPath, + severityFloor: 'auto', + bodyCriticals: ['[build] tsc fails on the merge commit'], + }), + 'utf8', + ); + writeFileSync(commentsPath, '[]', 'utf8'); + const stderr = () => + (writeStderrLine as ReturnType).mock.calls.map((c) => + String(c[0]), + ); + const stdoutJson = () => + JSON.parse( + (writeStdoutLine as ReturnType).mock.calls + .map((c) => String(c[0])) + .join('\n'), + ) as { + event?: string; + residualRisk?: { + shape: string; + recommendation: string; + criticals: number; + fresh: number; + prevFresh: number; + }; + body?: string; + }; + try { + // The predecessor carried a Critical and posted 0; this round posts 0 + // inline (the blocker rides the body) — flat, not shrinking. Round 7 + // of `auto`: the floor is engaged, so the advisory's floor claim is + // provable and all three surfaces must carry it. + (writeStderrLine as ReturnType).mockClear(); + (writeStdoutLine as ReturnType).mockClear(); + writeFileSync( + join(dir, 'qwen-review-pr-8255-prev-ledger.json'), + JSON.stringify({ + v: 1, + round: 6, + findings: [{ id: 'R6-1', sev: 'C', file: 'x.ts', title: 'blocker' }], + posted: 0, + fresh: 0, + }), + 'utf8', + ); + await runComposeReviewCommand({ + input: inputPath, + comments: commentsPath, + }); + const composed = stdoutJson(); + expect(composed.event).toBe('REQUEST_CHANGES'); + expect(composed.residualRisk).toMatchObject({ + shape: 'persistently-critical', + recommendation: 'land-with-residual-risk', + criticals: 1, + fresh: 0, + prevFresh: 0, + }); + expect(composed.body).toContain('land-with-residual-risk'); + expect( + stderr().filter((l) => l.startsWith('RESIDUAL-RISK: ')), + ).toHaveLength(1); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + it('stays silent on the advisory before the severity floor engages (#9410)', async () => { + // Round 2 under the default `auto` floor: the persistence and volume + // halves BOTH hold (a carried Critical stands again, the window is flat + // at 2/2), but the floor does not engage until round 6 — before + // engagement the advisory's "the floor will not converge it" claim is + // unprovable, so the signal degrades open to silence exactly like a + // missing volume. + const dir = mkdtempSync(join(tmpdir(), 'compose-converge-pre-')); + const inputPath = join(dir, 'compose.json'); + const commentsPath = join(dir, 'comments.json'); + const planPath = join(dir, 'plan.json'); + writeFileSync(planPath, JSON.stringify({ prNumber: 8255 }), 'utf8'); + // A KNOWN `auto` floor: the silence must come from the round-2 floor + // not being engaged yet, not from the floor being absent. + writeFileSync( + inputPath, + JSON.stringify({ modelId: MODEL, planPath, severityFloor: 'auto' }), + 'utf8', + ); + writeFileSync( + commentsPath, + JSON.stringify([ + { path: 'a.ts', line: 1, body: '**[Critical]** standing blocker' }, + { path: 'b.ts', line: 2, body: '**[Suggestion]** also posted' }, + ]), + 'utf8', + ); + const stderr = () => + (writeStderrLine as ReturnType).mock.calls.map((c) => + String(c[0]), + ); + const stdoutJson = () => + JSON.parse( + (writeStdoutLine as ReturnType).mock.calls + .map((c) => String(c[0])) + .join('\n'), + ) as { residualRisk?: unknown; body?: string }; + try { + (writeStderrLine as ReturnType).mockClear(); + (writeStdoutLine as ReturnType).mockClear(); + writeFileSync( + join(dir, 'qwen-review-pr-8255-prev-ledger.json'), + JSON.stringify({ + v: 1, + round: 1, + findings: [{ id: 'R1-1', sev: 'C', file: 'x.ts', title: 'blocker' }], + posted: 2, + fresh: 2, + }), + 'utf8', + ); + await runComposeReviewCommand({ + input: inputPath, + comments: commentsPath, + }); + expect( + stderr().filter((l) => l.startsWith('RESIDUAL-RISK: ')), + ).toHaveLength(0); + // The same positive sentinel: this silence must be the round-2 floor, + // not a predecessor that failed to load. + expect(stderr().find((l) => l.startsWith('VOLUME: '))).toContain( + '(previous round: 2)', + ); + const composed = stdoutJson(); + expect(composed.residualRisk).toBeUndefined(); + expect(composed.body ?? '').not.toContain('land-with-residual-risk'); + // The floor-futility claim must not publish before the floor ran. + expect(composed.body ?? '').not.toContain('The severity floor will not'); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + it('stays silent when the previous work-list held no Critical (#9526)', async () => { + // Every OTHER conjunct holds — the floor is engaged (round 7 of `auto`), + // this round stands behind a Critical, the window is flat at 1/1 — and + // the predecessor's work-list carries Suggestions only. "Persistently" + // critical means the Critical STOOD before; a round introducing its + // first one is a loop that has not yet had a chance to converge, and + // telling its operator to land with residual risk is the false fire the + // module's header forbids. Pins the persistence conjunct end to end: + // every earlier fixture carries sev `C` in the prev ledger, so replacing + // the derivation with a bare `true` shipped the whole suite green. + const dir = mkdtempSync(join(tmpdir(), 'compose-converge-nosev-')); + const inputPath = join(dir, 'compose.json'); + const commentsPath = join(dir, 'comments.json'); + const planPath = join(dir, 'plan.json'); + writeFileSync(planPath, JSON.stringify({ prNumber: 8255 }), 'utf8'); + writeFileSync( + inputPath, + JSON.stringify({ modelId: MODEL, planPath, severityFloor: 'auto' }), + 'utf8', + ); + writeFileSync( + commentsPath, + JSON.stringify([ + { path: 'a.ts', line: 1, body: '**[Critical]** first blocker' }, + ]), + 'utf8', + ); + const stderr = () => + (writeStderrLine as ReturnType).mock.calls.map((c) => + String(c[0]), + ); + const stdoutJson = () => + JSON.parse( + (writeStdoutLine as ReturnType).mock.calls + .map((c) => String(c[0])) + .join('\n'), + ) as { residualRisk?: unknown; body?: string }; + try { + (writeStderrLine as ReturnType).mockClear(); + (writeStdoutLine as ReturnType).mockClear(); + writeFileSync( + join(dir, 'qwen-review-pr-8255-prev-ledger.json'), + JSON.stringify({ + v: 1, + round: 6, + findings: [{ id: 'R6-1', sev: 'S', file: 'x.ts', title: 'nit' }], + posted: 1, + fresh: 1, + }), + 'utf8', + ); + await runComposeReviewCommand({ + input: inputPath, + comments: commentsPath, + }); + expect( + stderr().filter((l) => l.startsWith('RESIDUAL-RISK: ')), + ).toHaveLength(0); + // The positive sentinel: the predecessor WAS recovered, so the silence + // is its Critical-free work-list and not a fixture that never loaded. + expect(stderr().find((l) => l.startsWith('VOLUME: '))).toContain( + '(previous round: 1)', + ); + const composed = stdoutJson(); + expect(composed.residualRisk).toBeUndefined(); + expect(composed.body ?? '').not.toContain('land-with-residual-risk'); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + it('stays silent when the floor is ABSENT rather than resolved (#9526)', async () => { + // The one input where the two floor readings disagree. The state names + // no floor at all: the REPORTING reading folds absence into `auto` (so + // the round would describe itself as running a resolved critical floor + // from round 6), while ENFORCEMENT is strict and moves nothing — and the + // advisory's "The severity floor will not converge it" is a claim about + // Suggestions having actually left the posting set. Wiring the reporting + // reading here publishes that claim over a round whose enforcement + // backstop never ran, so this fixture is what holds the two apart: every + // other advisory fixture passes `severityFloor: 'auto'` explicitly and + // the swap ships green against all of them. + const dir = mkdtempSync(join(tmpdir(), 'compose-converge-nofloor-')); + const inputPath = join(dir, 'compose.json'); + const commentsPath = join(dir, 'comments.json'); + const planPath = join(dir, 'plan.json'); + writeFileSync(planPath, JSON.stringify({ prNumber: 8255 }), 'utf8'); + // No `severityFloor` key AT ALL — genuine absence, not a spelling drift. + writeFileSync( + inputPath, + JSON.stringify({ modelId: MODEL, planPath }), + 'utf8', + ); + writeFileSync( + commentsPath, + JSON.stringify([ + { path: 'a.ts', line: 1, body: '**[Critical]** standing blocker' }, + ]), + 'utf8', + ); + const stderr = () => + (writeStderrLine as ReturnType).mock.calls.map((c) => + String(c[0]), + ); + const stdoutJson = () => + JSON.parse( + (writeStdoutLine as ReturnType).mock.calls + .map((c) => String(c[0])) + .join('\n'), + ) as { residualRisk?: unknown; body?: string }; + try { + (writeStderrLine as ReturnType).mockClear(); + (writeStdoutLine as ReturnType).mockClear(); + writeFileSync( + join(dir, 'qwen-review-pr-8255-prev-ledger.json'), + JSON.stringify({ + v: 1, + round: 6, + findings: [{ id: 'R6-1', sev: 'C', file: 'x.ts', title: 'blocker' }], + posted: 1, + fresh: 1, + }), + 'utf8', + ); + await runComposeReviewCommand({ + input: inputPath, + comments: commentsPath, + }); + expect( + stderr().filter((l) => l.startsWith('RESIDUAL-RISK: ')), + ).toHaveLength(0); + expect(stderr().find((l) => l.startsWith('VOLUME: '))).toContain( + '(previous round: 1)', + ); + const composed = stdoutJson(); + expect(composed.residualRisk).toBeUndefined(); + expect(composed.body ?? '').not.toContain('land-with-residual-risk'); + // And the unprovable claim itself never reaches the body. + expect(composed.body ?? '').not.toContain('The severity floor will not'); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + it('stays silent on the round the floor ENGAGES on (#9526)', async () => { + // The posture-change arm, end to end. The predecessor recorded floor + // `o` — it was still posting Suggestions — and this round runs under + // the engaged floor, so the two volumes are not two points on one + // loop's trend: the drop between them is the Suggestions leaving the + // posting set. Firing here publishes "the severity floor will not + // converge it" after the floor has run for exactly one round. + const dir = mkdtempSync(join(tmpdir(), 'compose-converge-posture-')); + const inputPath = join(dir, 'compose.json'); + const commentsPath = join(dir, 'comments.json'); + const planPath = join(dir, 'plan.json'); + writeFileSync(planPath, JSON.stringify({ prNumber: 8255 }), 'utf8'); + writeFileSync( + inputPath, + JSON.stringify({ modelId: MODEL, planPath, severityFloor: 'auto' }), + 'utf8', + ); + writeFileSync( + commentsPath, + JSON.stringify([ + { path: 'a.ts', line: 1, body: '**[Critical]** standing blocker' }, + ]), + 'utf8', + ); + const stderr = () => + (writeStderrLine as ReturnType).mock.calls.map((c) => + String(c[0]), + ); + const stdoutJson = () => + JSON.parse( + (writeStdoutLine as ReturnType).mock.calls + .map((c) => String(c[0])) + .join('\n'), + ) as { residualRisk?: unknown; body?: string }; + try { + (writeStderrLine as ReturnType).mockClear(); + (writeStdoutLine as ReturnType).mockClear(); + writeFileSync( + join(dir, 'qwen-review-pr-8255-prev-ledger.json'), + JSON.stringify({ + v: 1, + round: 6, + findings: [{ id: 'R6-1', sev: 'C', file: 'x.ts', title: 'blocker' }], + posted: 1, + fresh: 1, + // Every other conjunct holds; ONLY the recorded posture differs. + floor: 'o', + }), + 'utf8', + ); + await runComposeReviewCommand({ + input: inputPath, + comments: commentsPath, + }); + expect( + stderr().filter((l) => l.startsWith('RESIDUAL-RISK: ')), + ).toHaveLength(0); + // The predecessor WAS recovered — the silence is its posture, not a + // fixture that never loaded. + expect(stderr().find((l) => l.startsWith('VOLUME: '))).toContain( + '(previous round: 1)', + ); + const composed = stdoutJson(); + expect(composed.residualRisk).toBeUndefined(); + expect(composed.body ?? '').not.toContain('land-with-residual-risk'); + expect(composed.body ?? '').not.toContain('The severity floor will not'); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + it('stays silent while the FRESH rate is falling under re-posts (#9526)', async () => { + // Step 6 re-posts every still-standing ledger Critical under its + // ORIGINAL id, so the posting TOTAL only ever rises. Round 6 posted 5 + // first-time Criticals; the author fixed 3, and round 7 re-posts the 2 + // that stand and drafts 4 new ones. Fresh 5 -> 4 is a loop converging, + // but the total went 5 -> 6, and a window measured on totals fired + // `land-with-residual-risk` over it. + const dir = mkdtempSync(join(tmpdir(), 'compose-converge-fresh-')); + const inputPath = join(dir, 'compose.json'); + const commentsPath = join(dir, 'comments.json'); + const planPath = join(dir, 'plan.json'); + writeFileSync(planPath, JSON.stringify({ prNumber: 8255 }), 'utf8'); + writeFileSync( + inputPath, + JSON.stringify({ modelId: MODEL, planPath, severityFloor: 'auto' }), + 'utf8', + ); + writeFileSync( + commentsPath, + JSON.stringify([ + // Re-posts: the carried id is what marks them as not-new. + { path: 'f1.ts', line: 1, body: '**[Critical]** R6-1: still standing' }, + { path: 'f2.ts', line: 1, body: '**[Critical]** R6-2: still standing' }, + ...[1, 2, 3, 4].map((n) => ({ + path: `n${n}.ts`, + line: 1, + body: `**[Critical]** brand new ${n}`, + })), + ]), + 'utf8', + ); + const stderr = () => + (writeStderrLine as ReturnType).mock.calls.map((c) => + String(c[0]), + ); + const stdoutJson = () => + JSON.parse( + (writeStdoutLine as ReturnType).mock.calls + .map((c) => String(c[0])) + .join('\n'), + ) as { residualRisk?: unknown; body?: string }; + try { + (writeStderrLine as ReturnType).mockClear(); + (writeStdoutLine as ReturnType).mockClear(); + writeFileSync( + join(dir, 'qwen-review-pr-8255-prev-ledger.json'), + JSON.stringify({ + v: 1, + round: 6, + posted: 5, + fresh: 5, + floor: 'c', + findings: [1, 2, 3, 4, 5].map((n) => ({ + id: `R6-${n}`, + sev: 'C', + file: `f${n}.ts`, + title: `blocker ${n}`, + })), + }), + 'utf8', + ); + await runComposeReviewCommand({ + input: inputPath, + comments: commentsPath, + }); + // The total ROSE — this is exactly the input the old window fired on. + expect(stderr().find((l) => l.startsWith('VOLUME: '))).toContain( + '6 inline comment(s) this round (4 reported for the first time) (previous round: 5)', + ); + expect( + stderr().filter((l) => l.startsWith('RESIDUAL-RISK: ')), + ).toHaveLength(0); + const composed = stdoutJson(); + expect(composed.residualRisk).toBeUndefined(); + expect(composed.body ?? '').not.toContain('land-with-residual-risk'); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + it('stays silent while the standing BACKLOG is clearing (#9526)', async () => { + // The blind spot a fresh-only window leaves, and the regression that + // moving to fresh counts would otherwise introduce. The reviewer found + // nothing new in either round — fresh 0 against fresh 0, which "not + // falling" reads as stuck — while the author cleared 2 of 5 standing + // Criticals. The posting total (5 -> 3) used to catch this; only the + // Critical count coming down catches it now. + const dir = mkdtempSync(join(tmpdir(), 'compose-converge-backlog-')); + const inputPath = join(dir, 'compose.json'); + const commentsPath = join(dir, 'comments.json'); + const planPath = join(dir, 'plan.json'); + writeFileSync(planPath, JSON.stringify({ prNumber: 8255 }), 'utf8'); + writeFileSync( + inputPath, + JSON.stringify({ modelId: MODEL, planPath, severityFloor: 'auto' }), + 'utf8', + ); + writeFileSync( + commentsPath, + JSON.stringify( + [1, 2, 3].map((n) => ({ + path: `f${n}.ts`, + line: 1, + body: `**[Critical]** R6-${n}: still standing`, + })), + ), + 'utf8', + ); + const stderr = () => + (writeStderrLine as ReturnType).mock.calls.map((c) => + String(c[0]), + ); + const stdoutJson = () => + JSON.parse( + (writeStdoutLine as ReturnType).mock.calls + .map((c) => String(c[0])) + .join('\n'), + ) as { residualRisk?: unknown; body?: string }; + try { + (writeStderrLine as ReturnType).mockClear(); + (writeStdoutLine as ReturnType).mockClear(); + writeFileSync( + join(dir, 'qwen-review-pr-8255-prev-ledger.json'), + JSON.stringify({ + v: 1, + round: 6, + posted: 5, + // Nothing new last round either — so the fresh window is flat at + // zero and cannot tell this loop from a stuck one. + fresh: 0, + floor: 'c', + findings: [1, 2, 3, 4, 5].map((n) => ({ + id: `R6-${n}`, + sev: 'C', + file: `f${n}.ts`, + title: `blocker ${n}`, + })), + }), + 'utf8', + ); + await runComposeReviewCommand({ + input: inputPath, + comments: commentsPath, + }); + expect(stderr().find((l) => l.startsWith('VOLUME: '))).toContain( + '3 inline comment(s) this round (0 reported for the first time) (previous round: 5)', + ); + expect( + stderr().filter((l) => l.startsWith('RESIDUAL-RISK: ')), + ).toHaveLength(0); + const composed = stdoutJson(); + expect(composed.residualRisk).toBeUndefined(); + expect(composed.body ?? '').not.toContain('land-with-residual-risk'); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + it('fires at zero fresh when the backlog HOLDS — the purest shape (#9526)', async () => { + // The other side of the backlog veto, and the shape this whole feature + // exists to name: the same Criticals re-posted round after round, the + // reviewer finding nothing new, nothing clearing. Fresh 0 against + // fresh 0 and the backlog flat at 3 — a loop the floor cannot converge. + const dir = mkdtempSync(join(tmpdir(), 'compose-converge-stuck-')); + const inputPath = join(dir, 'compose.json'); + const commentsPath = join(dir, 'comments.json'); + const planPath = join(dir, 'plan.json'); + writeFileSync(planPath, JSON.stringify({ prNumber: 8255 }), 'utf8'); + writeFileSync( + inputPath, + JSON.stringify({ modelId: MODEL, planPath, severityFloor: 'auto' }), + 'utf8', + ); + writeFileSync( + commentsPath, + JSON.stringify( + [1, 2, 3].map((n) => ({ + path: `f${n}.ts`, + line: 1, + body: `**[Critical]** R6-${n}: still standing`, + })), + ), + 'utf8', + ); + const stdoutJson = () => + JSON.parse( + (writeStdoutLine as ReturnType).mock.calls + .map((c) => String(c[0])) + .join('\n'), + ) as { + residualRisk?: { + shape: string; + recommendation: string; + criticals: number; + fresh: number; + prevFresh: number; + }; + body?: string; + }; + try { + (writeStderrLine as ReturnType).mockClear(); + (writeStdoutLine as ReturnType).mockClear(); + writeFileSync( + join(dir, 'qwen-review-pr-8255-prev-ledger.json'), + JSON.stringify({ + v: 1, + round: 6, + posted: 3, + fresh: 0, + floor: 'c', + findings: [1, 2, 3].map((n) => ({ + id: `R6-${n}`, + sev: 'C', + file: `f${n}.ts`, + title: `blocker ${n}`, + })), + }), + 'utf8', + ); + await runComposeReviewCommand({ + input: inputPath, + comments: commentsPath, + }); + const composed = stdoutJson(); + expect(composed.residualRisk).toMatchObject({ + shape: 'persistently-critical', + recommendation: 'land-with-residual-risk', + criticals: 3, + fresh: 0, + prevFresh: 0, + }); + expect(composed.body).toContain('land-with-residual-risk'); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + it("stays silent when the predecessor's `c` stamp was a fold, not enforcement (#9526)", async () => { + // The stamp and the engagement test are two different readings. A round + // >= 6 whose state named no floor at all is stamped `c` by the REPORTING + // fold, while the strict enforcement backstop moved nothing and + // Suggestions posted normally. Paired against this round's enforcement + // reading, that stamp let an un-enforced predecessor pass as an engaged + // one and the advisory published "the severity floor will not converge + // it" against a window whose far end still included Suggestions. + // + // The Suggestion left standing in that round's work-list is the fact the + // stamp cannot carry, and it is what makes this fixture silent. + const dir = mkdtempSync(join(tmpdir(), 'compose-converge-foldstamp-')); + const inputPath = join(dir, 'compose.json'); + const commentsPath = join(dir, 'comments.json'); + const planPath = join(dir, 'plan.json'); + writeFileSync(planPath, JSON.stringify({ prNumber: 8255 }), 'utf8'); + // THIS round names the floor, so enforcement really is engaged here. + writeFileSync( + inputPath, + JSON.stringify({ modelId: MODEL, planPath, severityFloor: 'critical' }), + 'utf8', + ); + writeFileSync( + commentsPath, + JSON.stringify( + [1, 2, 3, 4].map((n) => ({ + path: `n${n}.ts`, + line: 1, + body: `**[Critical]** new blocker ${n}`, + })), + ), + 'utf8', + ); + const stderr = () => + (writeStderrLine as ReturnType).mock.calls.map((c) => + String(c[0]), + ); + const stdoutJson = () => + JSON.parse( + (writeStdoutLine as ReturnType).mock.calls + .map((c) => String(c[0])) + .join('\n'), + ) as { residualRisk?: unknown; body?: string }; + try { + (writeStderrLine as ReturnType).mockClear(); + (writeStdoutLine as ReturnType).mockClear(); + writeFileSync( + join(dir, 'qwen-review-pr-8255-prev-ledger.json'), + JSON.stringify({ + v: 1, + round: 6, + posted: 4, + fresh: 4, + // The stamp the reporting fold writes for a round that named no + // floor — every other conjunct is arranged to hold, so this + // fixture is silent on the work-list evidence alone. + floor: 'c', + findings: [ + { id: 'R6-1', sev: 'C', file: 'a.ts', title: 'b1' }, + { id: 'R6-2', sev: 'C', file: 'b.ts', title: 'b2' }, + { id: 'R6-3', sev: 'C', file: 'c.ts', title: 'b3' }, + // Enforcement never ran, so this posted and is in the list. + { id: 'R6-4', sev: 'S', file: 'd.ts', title: 'nit' }, + ], + }), + 'utf8', + ); + await runComposeReviewCommand({ + input: inputPath, + comments: commentsPath, + }); + // The predecessor WAS recovered — the silence is its Suggestion, not a + // fixture that never loaded. + expect(stderr().find((l) => l.startsWith('VOLUME: '))).toContain( + '(previous round: 4)', + ); + expect( + stderr().filter((l) => l.startsWith('RESIDUAL-RISK: ')), + ).toHaveLength(0); + const composed = stdoutJson(); + expect(composed.residualRisk).toBeUndefined(); + expect(composed.body ?? '').not.toContain('land-with-residual-risk'); + expect(composed.body ?? '').not.toContain('The severity floor will not'); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + it("will not read a PURE-FOREIGN work-list as this account's history (#9526)", async () => { + // Recovery adopts the highest-round marker whoever posted it. Where that + // marker was not merged over this account's own findings, this account's + // entries are in no work list at all — the state `openCriticals` already + // refuses to infer across. Every prev-round fact this signal reads comes + // off that list, so an own round-6 marker that was a clean LGTM, plus a + // foreign same-round marker carrying Criticals and no Suggestions, was + // enough to publish `land-with-residual-risk` over this account's own + // LGTM. The two control arms are the point: the fix must withhold the + // stranger's list WITHOUT silencing a list this account can claim. + const arms = [ + { + label: 'pure-foreign', + flags: { foreign: true, merged: false }, + fires: false, + }, + { + label: 'own list', + flags: { foreign: false, merged: false }, + fires: true, + }, + // A merged foreign list keeps this account's own certified entries + // under their own ids, which is what makes it speak for this account. + { + label: 'merged foreign', + flags: { foreign: true, merged: true }, + fires: true, + }, + ]; + const observed: Array<{ + arm: string; + recommendation: string | undefined; + terminalLines: number; + }> = []; + for (const arm of arms) { + const dir = mkdtempSync(join(tmpdir(), 'compose-converge-foreign-')); + const inputPath = join(dir, 'compose.json'); + const commentsPath = join(dir, 'comments.json'); + const planPath = join(dir, 'plan.json'); + writeFileSync(planPath, JSON.stringify({ prNumber: 8255 }), 'utf8'); + writeFileSync( + inputPath, + JSON.stringify({ modelId: MODEL, planPath, severityFloor: 'auto' }), + 'utf8', + ); + writeFileSync( + commentsPath, + JSON.stringify([ + { path: 'a.ts', line: 1, body: '**[Critical]** our own new blocker' }, + ]), + 'utf8', + ); + const stdoutJson = () => + JSON.parse( + (writeStdoutLine as ReturnType).mock.calls + .map((c) => String(c[0])) + .join('\n'), + ) as { residualRisk?: unknown }; + try { + (writeStderrLine as ReturnType).mockClear(); + (writeStdoutLine as ReturnType).mockClear(); + writeFileSync( + join(dir, 'qwen-review-pr-8255-prev-ledger.json'), + JSON.stringify({ + v: 1, + round: 6, + // This account's own round posted nothing — a clean LGTM. + posted: 0, + fresh: 0, + floor: 'c', + // ...while the list that won recovery holds a stranger's + // Critical and, notably, no Suggestion to give the posture away. + findings: [ + { id: 'R6-1', sev: 'C', file: 'x.ts', title: 'their blocker' }, + ], + ...arm.flags, + }), + 'utf8', + ); + await runComposeReviewCommand({ + input: inputPath, + comments: commentsPath, + }); + const composed = stdoutJson(); + const rr = composed.residualRisk as + | { recommendation?: string } + | undefined; + // The arm label rides IN the assertion, so a failure names which arm + // moved rather than pointing at a line inside the loop. + observed.push({ + arm: arm.label, + recommendation: rr?.recommendation, + terminalLines: ( + writeStderrLine as ReturnType + ).mock.calls + .map((c) => String(c[0])) + .filter((l) => l.startsWith('RESIDUAL-RISK: ')).length, + }); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + } + expect(observed).toEqual([ + { arm: 'pure-foreign', recommendation: undefined, terminalLines: 0 }, + { + arm: 'own list', + recommendation: 'land-with-residual-risk', + terminalLines: 1, + }, + { + arm: 'merged foreign', + recommendation: 'land-with-residual-risk', + terminalLines: 1, + }, + ]); + }); + + it('discloses that a fired reading came off a TRUNCATED work-list (#9526)', async () => { + // `prevLedgerFacts` carries shortened lists on purpose — the marker's + // byte budget sheds findings on exactly the deep-work-list rounds this + // advisory exists for. It still fires there, and the paragraph says + // which of its readings came off an incomplete list: "no Suggestion, so + // the floor was enforcing" and "the backlog is not shrinking" are read + // off ABSENCE, and a shortened list can only lose entries. + const dir = mkdtempSync(join(tmpdir(), 'compose-converge-trunc-')); + const inputPath = join(dir, 'compose.json'); + const commentsPath = join(dir, 'comments.json'); + const planPath = join(dir, 'plan.json'); + writeFileSync(planPath, JSON.stringify({ prNumber: 8255 }), 'utf8'); + writeFileSync( + inputPath, + JSON.stringify({ modelId: MODEL, planPath, severityFloor: 'auto' }), + 'utf8', + ); + writeFileSync( + commentsPath, + JSON.stringify([ + { path: 'a.ts', line: 1, body: '**[Critical]** standing blocker' }, + ]), + 'utf8', + ); + const stderr = () => + (writeStderrLine as ReturnType).mock.calls.map((c) => + String(c[0]), + ); + const stdoutJson = () => + JSON.parse( + (writeStdoutLine as ReturnType).mock.calls + .map((c) => String(c[0])) + .join('\n'), + ) as { residualRisk?: { prevTruncated?: boolean }; body?: string }; + try { + (writeStderrLine as ReturnType).mockClear(); + (writeStdoutLine as ReturnType).mockClear(); + writeFileSync( + join(dir, 'qwen-review-pr-8255-prev-ledger.json'), + JSON.stringify({ + v: 1, + round: 6, + posted: 1, + fresh: 1, + floor: 'c', + findings: [{ id: 'R6-1', sev: 'C', file: 'x.ts', title: 'blocker' }], + // What the serializer records when the byte budget shed entries — + // the list that came back is known-incomplete. + dropped: 4, + }), + 'utf8', + ); + await runComposeReviewCommand({ + input: inputPath, + comments: commentsPath, + }); + // It STILL fires: the gate is not restored, because a whole-list + // requirement would silence exactly these rounds. + const composed = stdoutJson(); + expect(composed.residualRisk?.prevTruncated).toBe(true); + // ...and both the body and the terminal record say what it rests on. + expect(composed.body).toContain('truncated to fit the marker'); + expect(composed.body).toContain('read off a list known to be incomplete'); + const line = stderr().find((l) => l.startsWith('RESIDUAL-RISK: ')) ?? ''; + expect(line).toContain('truncated to fit the marker'); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + it('counts a relocated Critical toward the advisory (#9410)', async () => { + // This round's only Critical arrives through the deferral channel's + // RELOCATED arm — a deferred entry with severity Critical is relocated + // back into the posting set. The relocated term of `thisCriticals` is + // load-bearing here: deleting `+ relocatedCriticals.length` from the + // sum un-fires the advisory, and every earlier firing fixture composed + // rounds with `relocatedCriticals === 0`, so the mutant shipped green. + const dir = mkdtempSync(join(tmpdir(), 'compose-converge-reloc-')); + const inputPath = join(dir, 'compose.json'); + const commentsPath = join(dir, 'comments.json'); + const planPath = join(dir, 'plan.json'); + writeFileSync(planPath, JSON.stringify({ prNumber: 8255 }), 'utf8'); + writeFileSync( + inputPath, + JSON.stringify({ + modelId: MODEL, + planPath, + severityFloor: 'auto', + deferredSuggestions: [ + { + file: 'src/auth.ts', + line: 88, + // Deterministic source: the relocated Critical blocks without a + // verifier record, keeping the round REQUEST_CHANGES like the + // sibling [build] fixture. + source: 'test', + severity: 'Critical', + title: 'red on the merge', + }, + ], + }), + 'utf8', + ); + writeFileSync(commentsPath, '[]', 'utf8'); + const stderr = () => + (writeStderrLine as ReturnType).mock.calls.map((c) => + String(c[0]), + ); + const stdoutJson = () => + JSON.parse( + (writeStdoutLine as ReturnType).mock.calls + .map((c) => String(c[0])) + .join('\n'), + ) as { + event?: string; + residualRisk?: { + shape: string; + recommendation: string; + criticals: number; + fresh: number; + prevFresh: number; + }; + body?: string; + }; + try { + // The predecessor carried a Critical and posted 0; this round posts 0 + // inline (the relocated blocker rides the body) — flat, not + // shrinking. Round 7 of `auto`: the floor is engaged. + (writeStderrLine as ReturnType).mockClear(); + (writeStdoutLine as ReturnType).mockClear(); + writeFileSync( + join(dir, 'qwen-review-pr-8255-prev-ledger.json'), + JSON.stringify({ + v: 1, + round: 6, + findings: [{ id: 'R6-1', sev: 'C', file: 'x.ts', title: 'blocker' }], + posted: 0, + fresh: 0, + }), + 'utf8', + ); + await runComposeReviewCommand({ + input: inputPath, + comments: commentsPath, + }); + const composed = stdoutJson(); + expect(composed.event).toBe('REQUEST_CHANGES'); + expect(composed.residualRisk).toMatchObject({ + shape: 'persistently-critical', + recommendation: 'land-with-residual-risk', + criticals: 1, + fresh: 0, + prevFresh: 0, + }); + // The relocated blocker and the advisory both ride the body; the + // terminal carries the advisory line. + expect(composed.body).toContain('relocated from the deferral channel'); + expect(composed.body).toContain('land-with-residual-risk'); + expect( + stderr().filter((l) => l.startsWith('RESIDUAL-RISK: ')), + ).toHaveLength(1); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + it("counts the script-lint gate's standing Critical — advisory and work-list (#9526)", async () => { + // The deterministic gate posts a body-only [lint] Critical every round + // while the model drafts nothing — the standing-blocker loop the signal + // exists to name. The count must see the gate's Critical exactly like + // the verdict's own `c` does, and the carried work-list must record + // sev 'C' for it, or the whole conjunction holds semantically while the + // advisory stays silent and the next round's persistence half is blind. + const dir = mkdtempSync(join(tmpdir(), 'compose-converge-gate-')); + const inputPath = join(dir, 'compose.json'); + const commentsPath = join(dir, 'comments.json'); + const planPath = join(dir, 'plan.json'); + // A worktree arms the gate (pr-worktree, not diff-only); the report + // binds to the plan diff's hash so the gate reads it as fresh. + const diffPath = join(dir, 'the.diff'); + writeFileSync( + diffPath, + 'diff --git a/deploy.sh b/deploy.sh\n@@ -0,0 +1 @@\n+x\n', + 'utf8', + ); + const diffHash = createHash('sha256') + .update(readFileSync(diffPath)) + .digest('hex'); + writeFileSync( + planPath, + JSON.stringify({ + prNumber: 8255, + worktreePath: '.qwen/tmp/review-pr-8255', + diffPathAbsolute: diffPath, + }), + 'utf8', + ); + writeFileSync( + join(dir, 'qwen-review-pr-8255-script-lint.json'), + JSON.stringify({ + checked: [ + { + path: 'deploy.sh', + tool: 'shellcheck', + findings: [ + { + line: 1, + code: 'SC2086', + level: 'info', + message: 'quote the variable', + inDiff: true, + }, + ], + }, + ], + skipped: [], + errored: [], + deferred: [], + ok: false, + note: '', + diffHash, + }), + 'utf8', + ); + writeFileSync( + inputPath, + JSON.stringify({ modelId: MODEL, planPath, severityFloor: 'auto' }), + 'utf8', + ); + // The model drafts nothing: the gate's [lint] blocker is the round's + // only Critical and posts body-only, so the inline volume is 0. + writeFileSync(commentsPath, '[]', 'utf8'); + const stderr = () => + (writeStderrLine as ReturnType).mock.calls.map((c) => + String(c[0]), + ); + const stdoutJson = () => + JSON.parse( + (writeStdoutLine as ReturnType).mock.calls + .map((c) => String(c[0])) + .join('\n'), + ) as { + event?: string; + residualRisk?: { + shape: string; + recommendation: string; + criticals: number; + fresh: number; + prevFresh: number; + }; + body?: string; + }; + try { + // The predecessor carried a Critical and posted 0; this round posts 0 + // inline (the gate blocker rides the body) — flat, not shrinking. + // Round 7 of `auto`: the floor is engaged. + (writeStderrLine as ReturnType).mockClear(); + (writeStdoutLine as ReturnType).mockClear(); + writeFileSync( + join(dir, 'qwen-review-pr-8255-prev-ledger.json'), + JSON.stringify({ + v: 1, + round: 6, + findings: [{ id: 'R6-1', sev: 'C', file: 'x.ts', title: 'blocker' }], + posted: 0, + fresh: 0, + }), + 'utf8', + ); + await runComposeReviewCommand({ + input: inputPath, + comments: commentsPath, + }); + const composed = stdoutJson(); + expect(composed.event).toBe('REQUEST_CHANGES'); + expect(composed.residualRisk).toMatchObject({ + shape: 'persistently-critical', + recommendation: 'land-with-residual-risk', + criticals: 1, + fresh: 0, + prevFresh: 0, + }); + expect(composed.body).toContain('land-with-residual-risk'); + expect( + stderr().filter((l) => l.startsWith('RESIDUAL-RISK: ')), + ).toHaveLength(1); + // The marker records the gate Critical as sev 'C' in the work-list, + // so a second gate-only round recovers the persistence half instead + // of reading "no prior Critical" over a round that posted one. + const ledger = parseLedger(composed.body ?? ''); + expect(ledger?.findings.some((f) => f.sev === 'C')).toBe(true); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + it('honours review.attribution=false through the handler (wiring)', async () => { // Third wiring leg: deleting the attribution argument from the // composeReviewCommand call leaves the direct composeReview test and the @@ -4778,6 +6085,169 @@ describe('bilingual body — recovered from the live PR when the plan omits the }); }); +describe('a standing gate Critical enters the posting set exactly once (#9526)', () => { + // Putting the gate's Criticals into the carried work-list is what created + // this: from that round on, SKILL Step 6's still-standing rule tells the + // model to re-post the entry under its original id while `composeReview` + // re-derives the same Critical from the report. `buildLedger` keys by + // claimed id and the regenerated copy claims none, so it minted a second + // id beside the carried one and the pair compounded every round. + function gateFixture() { + const dir = mkdtempSync(join(tmpdir(), 'compose-gate-once-')); + const diffPath = join(dir, 'the.diff'); + writeFileSync( + diffPath, + 'diff --git a/deploy.sh b/deploy.sh\n@@ -0,0 +1 @@\n+x\n', + 'utf8', + ); + const diffHash = createHash('sha256') + .update(readFileSync(diffPath)) + .digest('hex'); + const planPath = join(dir, 'plan.json'); + writeFileSync( + planPath, + JSON.stringify({ + prNumber: 8255, + worktreePath: '.qwen/tmp/review-pr-8255', + diffPathAbsolute: diffPath, + }), + 'utf8', + ); + writeFileSync( + join(dir, 'qwen-review-pr-8255-script-lint.json'), + JSON.stringify({ + checked: [ + { + path: 'deploy.sh', + tool: 'shellcheck', + findings: [ + { + line: 1, + code: 'SC2086', + level: 'info', + message: 'quote the variable', + inDiff: true, + }, + ], + }, + ], + skipped: [], + errored: [], + deferred: [], + ok: false, + note: '', + diffHash, + }), + 'utf8', + ); + return { dir, planPath }; + } + + it('does not compound the work-list or the body across rounds', () => { + const { dir, planPath } = gateFixture(); + try { + const gateLine = scriptLintGate(planPath).criticals[0]!; + const renders = (body: string) => (body.match(/SC2086/g) ?? []).length; + const seen: Array<{ round: number; ids: string[]; renders: number }> = []; + let carried: string[] = []; + for (let round = 1; round <= 3; round++) { + const r = composeReview({ + planPath, + env: ENV, + modelId: MODEL, + criticalsInline: 0, + suggestionsInline: 0, + // A SKILL-compliant run re-posts every still-standing work-list + // entry under its original id. That is the input under test. + bodyCriticals: carried.map((id) => `${id}: ${gateLine}`), + }); + const led = parseLedger(r.body)!; + seen.push({ + round, + ids: led.findings.map((f) => `${f.id}:${f.sev}`), + renders: renders(r.body), + }); + carried = led.findings.map((f) => f.id); + writeFileSync( + join(dir, 'qwen-review-pr-8255-prev-ledger.json'), + JSON.stringify({ + v: 1, + round: led.round, + posted: 0, + fresh: 0, + floor: 'o', + findings: led.findings, + }), + 'utf8', + ); + } + // ONE entry and one rendering per round, for one lint finding. Before + // the dedup this read [R1-1] / [R1-1,R2-1] / [R1-1,R2-1,R3-1] with the + // body rendering 1, 2 and 3 copies of the same blocker. + expect(seen).toEqual([ + { round: 1, ids: ['R1-1:C'], renders: 2 }, + { round: 2, ids: ['R2-1:C'], renders: 2 }, + { round: 3, ids: ['R3-1:C'], renders: 2 }, + ]); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + it('keeps the deterministic copy, so a proven blocker pulls no verify cap', () => { + // `[lint]` is not in `DETERMINISTIC_TAG_RE` (`[build]`/`[test]`/ + // `[probe]` only), so the model's re-post counts toward + // `criticalsNeedingVerify`. Dropping it only from the BODY while + // provenance still saw it left a linter-proven blocker pulling the + // unverified-blocker cap on every re-post round. + const { dir, planPath } = gateFixture(); + try { + const gateLine = scriptLintGate(planPath).criticals[0]!; + const r = composeReview({ + planPath, + env: ENV, + modelId: MODEL, + criticalsInline: 0, + suggestionsInline: 0, + bodyCriticals: [`R1-1: ${gateLine}`], + }); + expect(r.cappedBy).not.toContain('criticals-unverified'); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + it("matches the locator, not the model's wording", () => { + // A re-post is model prose: it carries the entry forward without being + // required to reproduce the message, or the gate's `mdField` backticks, + // byte for byte. An exact-match rule stopped deduping the moment the + // wording drifted — which is the common case, not the edge. + const { dir, planPath } = gateFixture(); + try { + const gate = scriptLintGate(planPath).criticals; + expect( + withoutGateReposts( + [ + 'R1-1: `deploy.sh`:1 SC2086 — reworded by the model [lint]', + 'R1-2: deploy.sh:1 SC2086 — and without the backticks [lint]', + ], + gate, + ), + ).toEqual([]); + // A DIFFERENT finding in the same file is not the same finding. + expect( + withoutGateReposts(['R1-3: `deploy.sh`:9 SC2115 — other [lint]'], gate), + ).toEqual(['R1-3: `deploy.sh`:9 SC2115 — other [lint]']); + // No gate findings: nothing is dropped. + expect(withoutGateReposts(['R1-1: anything'], [])).toEqual([ + 'R1-1: anything', + ]); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); +}); + describe('scriptLintGate — the deterministic gate reads the report', () => { // Unit-level: the gate turns the orchestrator's report into verdict inputs // (compose-review's coverage machinery is exercised elsewhere). A same-repo plan @@ -7024,9 +8494,9 @@ describe("composeReview — the composed body fits GitHub's limit", () => { it('trims the deferral display ALONE when that is enough — the order is observable', () => { // Without this shape the ordering policy has no guard: a mutant that // makes the not-reviewed disclosures yield WITH the deferral display - // (trim 2 → 1) leaves a byte-identical body whenever both must go, so + // (trim 3 → 1) leaves a byte-identical body whenever both must go, so // the whole suite passed under it. Here dropping rank 1 alone fits, so - // rank 2 must survive. + // rank 3 must survive. const blocker = 'B'.repeat(64_200); const r = composeReview( base({ @@ -7506,7 +8976,7 @@ describe("composeReview — the composed body fits GitHub's limit", () => { }); it('points at the findings artifact only when the deferral list is what went', () => { - // Rank 2 drops alone on any run with disclosures and no posture + // Rank 3 drops alone on any run with disclosures and no posture // deferrals. The unconditional pointer then told the author to read // "deferred findings in this run's findings artifact" — of which there // are none. The sibling stderr line had the condition all along. @@ -7530,6 +9000,178 @@ describe("composeReview — the composed body fits GitHub's limit", () => { // operator sent to a list that does not exist is the same false record // in the channel the operator actually reads. expect(r.remediation.join('\n')).not.toContain('findings artifact'); + // The rank-3-only tail clause: a trimmed disclosure section survives + // nowhere but the terminal summary, and the line must say exactly + // that — naming an advisory copy for an advisory that was never + // trimmed is the same false record in the other direction. + expect( + r.remediation.some( + (l) => + l.startsWith('body budget:') && l.includes('their only other copy'), + ), + ).toBe(true); + }); + + it('names the trimmed advisory for itself — never a deferral list that does not exist (#9410)', () => { + // The fired advisory shape with ZERO deferrals: the advisory is the + // only trimmable section, so the posted notice must name IT. Sharing + // the deferral display's rank posted "the deferred-findings list did + // not fit ... and deferred findings in this run's findings artifact" — + // asserting a list that never existed while the dropped advisory went + // unnamed (R1-3) — and the advisory's body-budget yield at rung 2 had + // no oracle at all (R1-9). + 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: 6, + findings: [{ id: 'R6-1', sev: 'C', file: 'x.ts', title: 'blocker' }], + posted: 0, + fresh: 0, + // An ANCHORED predecessor, so the mechanism-health note (rank -1) + // stays silent and the advisory is the only trimmable section — + // which is the whole point of this test. Without it the chain reads + // as two consecutive withholds and a second section drops beside + // the one under examination. + sha: 'deadbeef00112233445566778899aabbccddeeff', + }), + ); + // Sized against the PR-named budget (65,536 − margin − marker + // reserve): the body overflows WITH the advisory and fits once the + // advisory yields — the rung-2 exit under test. + const blocker = 'B'.repeat(56_200); + // Direct input, not `base()`: its default `planPath: coveredPlan()` + // re-writes this very plan file and erases the prNumber the side file + // hangs off. + const r = composeReview({ + planPath, + env: ENV, + modelId: MODEL, + criticalsInline: 0, + suggestionsInline: 0, + severityFloor: 'auto', + bodyCriticals: [blocker], + unreviewedDimensions: ['security'], + }); + expect(r.body.length).toBeLessThanOrEqual(LIMIT); + expect(r.body).toContain(blocker); + // The shape fired — round 7 of `auto`, a carried Critical stands + // again, the window is flat at 0/0. + expect(r.residualRisk).toMatchObject({ + shape: 'persistently-critical', + recommendation: 'land-with-residual-risk', + criticals: 1, + fresh: 0, + prevFresh: 0, + }); + // The advisory yielded to the budget, and the notice names what + // actually went — the advisory, by its own name. + expect(r.body).not.toContain('land-with-residual-risk'); + expect(r.body).toContain( + 'the persistently-critical convergence advisory did not fit', + ); + expect(r.body).toContain('(1 section(s))'); + expect(r.body).toContain('Nothing blocking was trimmed.'); + // No false record: no deferral-list name, no artifact pointer, no + // deferralList flag — the body this notice describes held no + // deferrals at all. + expect(r.body).not.toContain('the deferred-findings list'); + expect(r.body).not.toContain('findings artifact'); + expect(r.remediation.join('\n')).not.toContain('findings artifact'); + expect(r.bodyTrim).toEqual({ + sections: 1, + deferralList: false, + fold: false, + truncated: false, + }); + // The advisory yields BEFORE the not-reviewed disclosures, which keep + // their place in the body — the ranks are distinct, in this order. + expect(r.body).toContain('Not reviewed: security'); + // The operator's copy names the loss too, on the same channel the + // other budget lines ride. + expect( + r.remediation.some( + (l) => + l.startsWith('body budget:') && + l.includes('persistently-critical convergence advisory') && + // The tail clause is the branch under test. Rank 3 did NOT go + // here — the disclosures keep their place in the body — so every + // section that went (the advisory) does have a durable copy, and + // "their only other copy" would be a false record. The artifact + // is deliberately not named: this run holds no deferral list. + l.includes( + 'though every section that went also has a durable copy elsewhere', + ), + ), + ).toBe(true); + }); + + it('warns for the disclosures when the advisory went with them (#9526)', () => { + // The COMBINED drop the rank-2 keying got wrong. With ranks 2 and 3 + // both gone, a tail keyed on the advisory said "another copy — the + // advisory also rides the composed JSON": true of the advisory, false + // of the disclosures beside it, and the disclosures are the half that + // survives nowhere but the terminal summary. The sentence exists to + // tell the operator what they must repeat, so under-warning about + // exactly that half is the false-record class it is meant to refuse. + 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: 6, + findings: [{ id: 'R6-1', sev: 'C', file: 'x.ts', title: 'blocker' }], + posted: 0, + fresh: 0, + }), + ); + // A rank-3 section wide enough that shedding the advisory alone does + // not bring the body back under budget — so rung 2 goes on to rank 3 + // and both are in `droppedRanks`. Sized off the disclosure block rather + // than off the advisory: a one-section window would make the fixture + // turn on a few characters of prose. + const dimensions = Array.from( + { length: 30 }, + (_, i) => `dimension-number-${i}-with-a-long-name`, + ); + const blocker = 'B'.repeat(56_200); + const r = composeReview({ + planPath, + env: ENV, + modelId: MODEL, + criticalsInline: 0, + suggestionsInline: 0, + severityFloor: 'auto', + bodyCriticals: [blocker], + unreviewedDimensions: dimensions, + }); + expect(r.body.length).toBeLessThanOrEqual(LIMIT); + expect(r.body).toContain(blocker); + // Both ranks went, and nothing was cut — the tail cut has a notice of + // its own and would change the subject of the line under test. + expect(r.bodyTrim.truncated).toBe(false); + expect(r.body).not.toContain('land-with-residual-risk'); + const line = r.remediation.find((l) => l.startsWith('body budget:')) ?? ''; + expect(line).toContain('the persistently-critical convergence advisory'); + expect(line).toContain('the not-reviewed and non-blocking disclosures'); + // The branch under test: rank 3 is among the dropped, so the terminal + // summary IS the only other copy of that half, and the line must say + // so rather than reporting the advisory's spare copies for both. + expect(line).toContain( + 'which is the only other copy of the disclosures among them', + ); + expect(line).not.toContain( + 'though every section that went also has a durable copy elsewhere', + ); + // Still no deferral list on this run, so still no artifact pointer. + expect(line).not.toContain('findings artifact'); }); it('keeps the verdict-qualifying opener through a truncation', () => { @@ -7629,8 +9271,8 @@ describe("composeReview — the composed body fits GitHub's limit", () => { it('ranks the plan-gate disclosures with the not-reviewed ones, not with the deferral list', () => { // `deferredBlock`, `testPlanBlock` and `repositoryContextBlock` all - // carry `trim: 2`, and no overflow fixture carried any of them — so - // both mutations shipped green: `2 → 1` drops the disclosure WITH the + // carry `trim: 3`, and no overflow fixture carried any of them — so + // both mutations shipped green: `3 → 1` drops the disclosure WITH the // deferral display (inverting the documented order), and deleting the // tag makes it un-trimmable, sending a borderline body to the cut. const withContext = (blocker: string) => @@ -7659,7 +9301,7 @@ describe("composeReview — the composed body fits GitHub's limit", () => { }); // Self-calibrating rather than pinned to a byte size: scan a range and - // require BOTH shapes to exist. `trim: 2 → 1` removes the first (the + // require BOTH shapes to exist. `trim: 3 → 1` removes the first (the // block would go with the deferral display); deleting the tag removes // the second (the block would never yield). // Fine-grained on purpose: the rank-1-only window is as wide as the @@ -7674,7 +9316,7 @@ describe("composeReview — the composed body fits GitHub's limit", () => { !r.bodyTrim.truncated && r.body.includes('Repository proof boundary'), ); - const goesWithRank2 = runs.find( + const goesWithRank3 = runs.find( (r) => r.bodyTrim.deferralList && !r.body.includes('Repository proof boundary'), @@ -7683,8 +9325,8 @@ describe("composeReview — the composed body fits GitHub's limit", () => { expect(runs[0].bodyTrim.sections).toBe(0); expect(runs[0].body).toContain('Repository proof boundary'); expect(survivesRank1).toBeDefined(); - expect(goesWithRank2).toBeDefined(); - expect(goesWithRank2!.bodyTrim.sections).toBeGreaterThan( + expect(goesWithRank3).toBeDefined(); + expect(goesWithRank3!.bodyTrim.sections).toBeGreaterThan( survivesRank1!.bodyTrim.sections, ); }); diff --git a/packages/cli/src/commands/review/compose-review.ts b/packages/cli/src/commands/review/compose-review.ts index 6919225826a..47c86012dff 100644 --- a/packages/cli/src/commands/review/compose-review.ts +++ b/packages/cli/src/commands/review/compose-review.ts @@ -88,11 +88,14 @@ import { } from './lib/ledger.js'; import { mdField } from './lib/md-field.js'; import { + convergenceAdvisory, + convergenceAssessment, diagnoseConvergence, isFreshDraft, recommendationsFor, renderConvergenceDiagnosis, renderMechanismHealth, + type ConvergenceAssessment, type Recommendation, type CriticalFloorKind, type DraftedFinding, @@ -477,6 +480,13 @@ export function criticalFloorKind( * posture the state never named cannot move a finding out of the posting * set. `floorEnforcedReroute` acts on this; the reporting reading above is * what the round says about itself. + * + * The residual-risk signal (#9410) reads THIS one, not the reporting + * reading: its "the severity floor will not converge this loop" claim is + * about Suggestions actually having left the posting set, which is what a + * strict reading is. Shared rather than restated for the reason the whole + * pair exists — two spellings of one predicate is the drift class + * `normalizeSeverityFloor` above was extracted to prevent. */ export function criticalFloorInEffect( severityFloor: unknown, @@ -891,6 +901,24 @@ export interface ComposeReviewResult { * nothing", which is why absence is distinct from zero here. */ prevPostedInline?: number; + /** + * The persistently-critical convergence assessment (#9410), present only + * when the carried telemetry shows the loop is in that shape: Criticals + * stood in the previous round's work-list AND stand again this round, with + * the two-round posting window present and not shrinking. Advisory only — + * it never moves the event, never caps, never blocks; it surfaces the + * `land-with-residual-risk` recommendation and a residual-risk inventory + * scaffold for the maintainer's risk-acceptance decision. Absent whenever + * the shape is not provable; every input degrades open, so absence is the + * fail-safe reading, never a suppressed finding. + * + * Named for its exit rather than for `convergence` above, which is the + * loop-settling OBSERVATION's rendered paragraph: two features share the + * word, they can fire in the same round, and one field name over both + * would have made the composed JSON — and every consumer keying on it — + * unable to say which it was reading. + */ + residualRisk?: ConvergenceAssessment; /** * What the body budget had to give up to fit GitHub's limit, when it did. * On the result because `verdictLine` — printed to stderr, persisted in @@ -2034,10 +2062,33 @@ function ledgerMarkerFor( body?: unknown; }>, [ - ...ingestEntryList(input.bodyCriticals, 'bodyCriticals'), + // The same rule the body applied, through the same statement of + // it: a re-post of a claim the gate regenerates below is dropped + // here too, or the work-list grows a second entry for one blocker + // every round. + ...withoutGateReposts( + ingestEntryList(input.bodyCriticals, 'bodyCriticals'), + scriptLintGate(input.planPath).criticals, + ), // The same split the body performed: a relocated Critical is a // posted, counted blocker and must enter the work list. ...splitDeferralChannel(input.deferredSuggestions).relocated, + // The gate's Criticals, for the same reason: a gate Critical is a + // posted, counted blocker too — leaving it out let the next + // round's persistence half read "no prior Critical" over a round + // that posted one (#9526). + // + // A SECOND invocation, not the body composer's result — the two + // live in different functions and nothing passes the value across. + // What makes them agree is that `scriptLintGate` is pure in + // `planPath` and its inputs (the plan JSON, the report, the diff) + // are immutable for the length of one synchronous compose; it is + // NOT the single-origin discipline `postedInline` gets one line + // below. So the standing hazard is an edit, not a race: anything + // that filters, caps, or carves out what the BODY pushes must + // change this list too, or the posted body and the carried work + // list stop describing the same round (R4-1). + ...scriptLintGate(input.planPath).criticals, ], carriedWorkList, ), @@ -2530,6 +2581,18 @@ function composeReviewBody( // `[probe]` tag (filtered out before the subtract) or a gate finding's own text // contains one, erasing an unrelated claim's verification requirement. Identity, // not arithmetic, decides provenance. + // + // The gate runs BEFORE that capture, and its regenerated claims are used to + // drop the model's re-posts of them first. Dropping them later would leave + // the re-post out of the body while `modelBodyCriticals` still counted it + // toward `criticalsNeedingVerify` — a blocker the linter proved would go on + // pulling the unverified cap through a copy that no longer posts. + const gate = input.planPath + ? scriptLintGate(input.planPath) + : { criticals: [], unreviewed: [], disclosed: [] }; + const ownAfterGateDedup = withoutGateReposts(bodyCriticals, gate.criticals); + bodyCriticals.length = 0; + bodyCriticals.push(...ownAfterGateDedup); const modelBodyCriticals = [...bodyCriticals]; // input's, captured before the gate // Disclosed-but-non-capping notes from the gate (a deferred checker). Rendered // in the body on every verdict, but never fed into the cap. @@ -2543,7 +2606,8 @@ function composeReviewBody( // affected review impossible to approve. const repositoryContextNotes: string[] = []; if (input.planPath) { - const gate = scriptLintGate(input.planPath); + // The gate ran above, where its claims were needed to dedup the model's + // re-posts before provenance was taken. ONE invocation, reused here. bodyCriticals.push(...gate.criticals); // render + count toward `c`, deterministic unreviewed.push(...gate.unreviewed); gateDisclosed.push(...gate.disclosed); @@ -3160,6 +3224,118 @@ function composeReviewBody( ? renderConvergenceDiagnosis(diagnosis) : undefined; const recommendations = diagnosis ? recommendationsFor(diagnosis) : undefined; + // The persistently-critical residual-risk signal (#9410): computed, never + // decided. Computed HERE, beside the observation's own derivation, for two + // reasons that now coincide. It counts every Critical this round stands + // behind, and `bodyCriticals` is only complete once the relocated push and + // the script-lint gate push have both joined it — the SAME array, with the + // SAME semantics, the verdict's `c` counts below; a count taken before + // them read a gate-only round (a standing deterministic [lint] blocker the + // floor can never converge) as standing behind zero Criticals, so the + // advisory the shape exists to surface never fired on it (#9526). And its + // window runs on `postedFresh`, which is derived here. The persistence + // half, the fresh pair, the recorded floor and the backlog all come off + // the SAME recovered predecessor the loop-settling observation above + // reads, so the two features cannot disagree about what a round held. + // Advisory only: it cannot move the event or cap the verdict; it only + // surfaces. + // + // Every input degrades open to "no assessment" WITH ONE EXCEPTION, stated + // here because a blanket claim is the kind of false record this module + // polices. Two facts are read off the predecessor's work-list by ABSENCE — + // "no Suggestion in it, so the floor was enforcing" and "the backlog is + // not shrinking" — and a list the marker's byte budget SHORTENED can only + // lose entries, so both lean toward firing. The gate is not restored for + // it (a whole-list requirement would silence the advisory on exactly the + // deep-work-list rounds it exists for, which are the rounds that get + // shortened); `prevTruncated` rides instead, and the paragraph discloses + // that those two readings came off an incomplete list. + // A PURE-FOREIGN work-list is a stranger's, not a shortened version of + // this account's. Recovery adopts the highest-round marker whoever posted + // it, and where that marker was NOT merged over this account's own + // findings, this account's entries are in no work list at all — the same + // state `openCriticals` above refuses to infer across, for the same + // reason. Every prev-round fact this signal reads comes off that list, so + // reading it there let a stranger's Criticals stand in for this account's: + // an own round-6 marker that was a clean LGTM, a foreign same-round marker + // carrying Criticals and no Suggestions winning recovery, and one Critical + // drafted this round were enough to publish "Criticals stood in the + // previous round's work-list and stand again this round — + // land-with-residual-risk" over this account's own LGTM (#9526). + // + // Merged foreign lists are NOT withheld: the union keeps this account's + // own certified entries under their own ids, which is exactly the part + // that makes the list speak for this account again. + const pureForeignPrev = + convergence?.prev.foreign === true && convergence?.prev.merged !== true; + const residualRisk = convergenceAssessment({ + // The persistence half, read straight off the recovered work list rather + // than off a flag derived beside it: `prev.findings` is already gated on + // the round (a round-0 work list is no work list) and already through the + // ledger's own admission test, and a second derivation would be free to + // drift from the list the observation clusters over. A list the marker's + // byte budget truncated may have shed the very Critical that proves + // persistence — that costs a missed advisory, which is the fail-safe + // direction and the direction every other conjunct degrades in too. + prevHadCritical: + pureForeignPrev || !convergence + ? undefined + : convergence.prev.findings.some((f) => f.sev === 'C') || undefined, + // The floor-engagement conjunct is computed by the SAME predicate the + // enforcement backstop keys on (#9410): the advisory's "the floor will + // not converge it" claim is provable only where the floor is actually + // running, so a pre-engagement round degrades open to silence. The + // ENFORCEMENT reading, not the reporting one beside it — the claim is + // about Suggestions actually having been moved out of the posting set, + // not about the posture the round describes itself as running. + floorEngaged: criticalFloorInEffect( + input.severityFloor, + input.contextUnavailable === true, + prevRound, + ), + thisCriticals: criticalsInline + bodyCriticals.length, + // The FRESH counts, not the posting totals — the number this file's own + // `postedFresh` docstring calls "the number the convergence trend runs + // on ... so the next round can compare like with like". Step 6 re-posts + // every standing Critical under its original id, so the total only ever + // rises: measured on totals a loop whose new findings fell 5 -> 4 still + // posted MORE comments than the round before, and the advisory fired + // "the severity floor will not converge it" over a converging loop. + // The same pair the observation above trends on, so the two features + // cannot disagree about what this round produced. + fresh: postedFresh, + prevFresh: convergence?.prev.fresh, + // Off the same recovered predecessor — the marker records the floor its + // round ran under, and the observation above compares the pair for + // exactly this reason. + prevFloor: convergence?.prev.floor, + // The stamp above is the REPORTING reading and folds an absent floor to + // `auto`, so it says `c` on a round >= 6 the enforcement backstop never + // touched. A Suggestion still standing in that round's work-list is the + // fact the stamp cannot carry: enforcement moves drafted Suggestions out + // of the posting set before the marker is built, so its presence means + // the floor was not running (#9526). + prevPostedSuggestion: + convergence && !pureForeignPrev + ? convergence.prev.findings.some((f) => f.sev === 'S') + : undefined, + // The standing backlog, counted off the same recovered work-list the + // persistence half reads. It is what keeps the fresh window honest at + // its blind spot: a round finding nothing new while the author clears + // blockers sits at fresh 0 against fresh 0, and only the Critical count + // falling says the loop is moving. + prevCriticals: + convergence && !pureForeignPrev + ? convergence.prev.findings.filter((f) => f.sev === 'C').length + : undefined, + // Not a conjunct — it decides nothing about whether the signal fires. + // It is what lets the paragraph qualify the two readings it takes off + // that list's ABSENCES ("no Suggestion, so the floor enforced"; "the + // backlog is not shrinking"), which are the two inputs that lean toward + // firing when the marker's byte budget shortened the list. + prevTruncated: + convergence && !pureForeignPrev ? convergence.prev.truncated : undefined, + }); let event: ReviewEvent = baseEvent; if (event === 'APPROVE' && cappedBy.length > 0) event = 'COMMENT'; @@ -3294,6 +3470,10 @@ 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 persistently-critical convergence advisory', + zh: 'persistently-critical 收敛建议', + }, 1: { en: 'the deferred-findings list', zh: '延后发现清单' }, 2: { en: 'the not-reviewed and non-blocking disclosures', @@ -3316,7 +3496,7 @@ function composeReviewBody( const named = ranks.map((r) => RANK_NAMES[r]).filter(Boolean); const en = named.map((n) => n.en).join(' and '); const zh = named.map((n) => n.zh).join('与'); - // "Nothing blocking was trimmed" is true of the RANKS — both are + // "Nothing blocking was trimmed" is true of the RANKS — all are // non-blocking by construction. It is not true of the tail cut below, // which can reach blocker text, so the claim is dropped exactly when a // cut happened and the truncation notice takes over the subject. @@ -3330,9 +3510,10 @@ function composeReviewBody( zh: '被裁剪的均非阻断内容。', }; // The artifact pointer is about the deferral list, so it rides only when - // that list is what went. Rank 2 can drop alone — it does, on any run - // with disclosures and no posture deferrals — and the unconditional - // pointer then sent the author to read a list that does not exist. + // that list is what went. Every other trim rank can drop alone — trim + // rank 2 does on any run with disclosures and no posture deferrals, trim + // rank 0 on a fired zero-deferral round — and the unconditional pointer + // then sent the author to read a list that does not exist. const artifact = ranks.includes(1) ? { en: `, and deferred findings in this run's findings artifact`, @@ -3355,8 +3536,10 @@ function composeReviewBody( * degrade, and the ORDER of the degradation is the policy: the bilingual * fold yields FIRST (it is a translation of the English above it, so it * costs the author nothing the body does not still say), then parts by - * ascending `trim` rank (the deferral display before the not-reviewed - * disclosures), the blockers and the caps never, and every drop is + * ascending `trim` rank (the mechanism-health note, then the residual-risk + * advisory, then the deferral display, then the not-reviewed disclosures, + * then the convergence observation), the blockers and the caps never, and + * every drop is * disclosed with its count and its kind — a list silently shortened reads * as a list that was complete. * @@ -3376,12 +3559,24 @@ function composeReviewBody( * Every exit of `render` that dropped a rank owes this line — the * last-resort path drops ranks AND cuts, and a stderr record naming only * the cut leaves the kinds it dropped disclosed nowhere but the body. - * Rank 1 has a second durable copy (each deferral is a `D-` - * entry in the findings artifact) and trim rank 3 has one too (the composed - * result carries the paragraph, and the command prints it as - * `CONVERGENCE:`); a trimmed disclosure section survives nowhere but the - * terminal summary, so ask for it there rather than pointing at an - * artifact that does not carry it. + * Four of the five TRIM ranks keep a second durable copy, and the + * ladder's order now follows that fact almost exactly: trim rank -1's + * health note and trim rank 0's residual-risk advisory both ride the + * composed result and print as `HEALTH:` and `RESIDUAL-RISK:`, trim rank + * 1's deferrals are each a `D-` entry in the findings artifact, + * and trim rank 3's observation rides the composed result too (and + * prints as `CONVERGENCE:`) — it is last for the arithmetic its own block + * explains, not for want of a copy. Trim rank 2 is the exception — a + * trimmed disclosure section survives nowhere but the terminal summary, + * so ask for it there rather than pointing at an artifact that does not + * carry it. + * + * Which is why the tail clause keys on trim rank 2, the one rank with + * nothing behind it. Keyed on the advisory instead it read "another copy + * — the advisory also rides the composed JSON" over a combined drop that + * took the disclosures with it, telling the operator the trimmed set was + * backed up when the half of it that is NOT backed up was exactly the + * half this sentence exists to rescue. */ const noteTrimmedRanks = (droppedRanks: number[]): void => { if (droppedRanks.length === 0) return; @@ -3394,8 +3589,18 @@ function composeReviewBody( (droppedRanks.includes(1) ? `the deferred findings are in the findings artifact; ` : '') + - `repeat the trimmed sections in your terminal summary, which is ` + - `their only other copy`, + `repeat the trimmed sections in your terminal summary` + + (droppedRanks.includes(2) + ? droppedRanks.length === 1 + ? `, which is their only other copy` + : `, which is the only other copy of the disclosures among them` + : // Deliberately unnamed here: the ONE place the artifact may be + // named is the rank-1 clause above, which rides only when the + // deferral list actually went. Naming it in this tail sent the + // operator to a `D-` list that does not exist on a + // rank-0-or-2-only drop — the same false record the clause + // above was split out to refuse. + `, though every section that went also has a durable copy elsewhere`), ); }; const render = (parts: Bi[], sep: string): string => { @@ -4083,6 +4288,39 @@ function composeReviewBody( ] : []; + // The persistently-critical residual-risk advisory (#9410): disclosed on + // every event the shape can reach when the carried telemetry shows the + // loop will not self-converge via the floor — the assessment only fires + // when this round stands behind a Critical, which is REQUEST_CHANGES by + // construction (or COMMENT when an unverified arm softens it), so those + // two branches render the block and the composed-JSON field rides every + // branch's return object. Non-capping and advisory-only — it never moves + // the event, never caps, and its own text disclaims it ("does not + // block"). Bounded by construction: fixed prose plus a count, no model + // text, so it cannot balloon the body it rides. + // + // `trim: 0` — its OWN rank, and the slot the observation vacated when it + // moved to last. The order here is by what a dropped block costs its + // reader, and this one costs the least after the health note: the + // maintainer it is written for receives it whole on the terminal + // `RESIDUAL-RISK:` line AND in the composed JSON, which the persisted + // artifact carries. The deferral list below it keeps one copy (the + // findings artifact), the disclosures below that keep none but the + // terminal report, and the observation last is the author's only sentence + // about the shape of the loop. Sharing a rank with any of them is what + // the trim notice cannot survive: it names what a rank drops, and rank + // 1's findings-artifact pointer is true only of the deferral list — a + // dropped advisory once posted a notice naming a deferral list that never + // existed. + const residualRiskBlock: Bi[] = residualRisk + ? [ + { + trim: 0, + ...convergenceAdvisory(residualRisk), + }, + ] + : []; + // The not-reviewed disclosures yield after the deferral display and before // the convergence observation: they say what the review could not certify, // which the verdict's own cap already carries, so trimming them costs @@ -4104,7 +4342,8 @@ function composeReviewBody( // arithmetic. Rendered bilingually this paragraph runs 603 characters when // only the volume signal fired, 1,510 with three clusters, and 2,372 with // the clusters, the evidence caveats and the land reading together — - // against a body budget of 56,830. Shed second (it was rank 0), it could + // against a body budget of 56,830. Shed second (it was trim rank 0, the + // slot the residual-risk advisory holds now), it could // pay for at most 4% of an overflow, so any overflow larger than itself // spent it and then went on to spend the deferral list and the // not-reviewed disclosures anyway. On the rounds this fires on — the @@ -4219,6 +4458,7 @@ function composeReviewBody( ...deferredSuggestionsBlock, ...convergenceBlock, ...healthBlock, + ...residualRiskBlock, ...continuityBlock, ...bodyCriticalBlock, ]; @@ -4248,6 +4488,7 @@ function composeReviewBody( lowSignal, scopeUnproven, dimensionGapsAreDepthOnly, + ...(residualRisk ? { residualRisk } : {}), }; } @@ -4335,6 +4576,7 @@ function composeReviewBody( lowSignal, scopeUnproven, dimensionGapsAreDepthOnly, + ...(residualRisk ? { residualRisk } : {}), }; } @@ -4504,7 +4746,13 @@ function composeReviewBody( clauses.push(...convergenceBlock); clauses.push(...healthBlock); - // 6g. Resumed-run continuity (non-capping) — reused work that COUNTS as + // 6g. Persistently-critical residual-risk advisory (non-capping, advisory + // only) — the exit for a loop the observation above has run out of + // postures to suggest. It follows the observation because it answers + // the question the observation leaves open. + clauses.push(...residualRiskBlock); + + // 6h. Resumed-run continuity (non-capping) — reused work that COUNTS as // reviewed, disclosed so the author knows two attempts fed this verdict. clauses.push(...continuityBlock); @@ -4571,6 +4819,7 @@ function composeReviewBody( lowSignal, scopeUnproven, dimensionGapsAreDepthOnly, + ...(residualRisk ? { residualRisk } : {}), }; } @@ -4685,10 +4934,18 @@ interface Bi { * How readily this part yields when the composed body would exceed * GitHub's limit — LOWER goes first, absent never goes. The order is a * policy, not a convenience: a body that cannot post loses its blockers, - * so the display of findings the review deliberately did NOT request - * (the deferral list, rank 1) yields before the disclosures of what went - * unreviewed (rank 2), and the blockers, the caps, and the sentences that - * qualify the verdict never yield at all. + * so the order runs by what a dropped block costs its reader: the + * operator-facing mechanism-health note (trim rank -1), then the + * persistently-critical advisory (trim rank 0 — the maintainer has it + * whole on the terminal line and in the composed JSON), then the display + * of findings the review deliberately did NOT request (the deferral list, + * trim rank 1, kept whole in the findings artifact), then the disclosures + * of what went unreviewed (trim rank 2, which have no other durable + * copy), and the convergence observation last (trim rank 3 — see its own + * block for why the cheapest paragraph is shed last). The blockers, the + * caps, and the sentences that qualify the verdict never yield at all. + * + * `keep` above is a DIFFERENT axis; a number here is a `trim` rank. */ trim?: number; } @@ -4747,6 +5004,59 @@ export function repositoryContextGate(planPath: string): string[] { * the model's input JSON, and the plan itself decides whether the lint was owed: * this is what takes the model out of both the block decision and the proof it ran. */ +/** + * The model's own body Criticals, minus any that RE-POST a claim this + * round's script-lint gate regenerates anyway. + * + * Putting the gate's Criticals into the carried work-list (#9526) is what + * made this necessary: from that round on, SKILL Step 6's still-standing + * rule tells the model to re-post the entry under its original id, while + * `composeReviewBody` re-derives the same Critical from the report — so one + * blocker rendered twice, `buildLedger` minted a second id beside the + * carried one because the regenerated copy claims none, and the pair + * compounded every round: `[R1-1]`, `[R1-1, R2-1]`, `[R1-1, R2-1, R3-1]` + * for a single lint finding, inflating the residual-risk count and the + * marker's byte budget with it. + * + * The GATE's copy is the one kept, not the model's. The gate re-derives it + * from a report bound to this diff's hash, so the re-post is structurally + * redundant — and the model's copy is untrusted prose that `[lint]` does + * not exempt from verification (`DETERMINISTIC_TAG_RE` covers `[build]`, + * `[test]` and `[probe]` only), so keeping THAT one instead pulled the + * unverified-blocker cap on every round a pipeline-proven blocker stood. + * The id chain is not preserved for these entries, deliberately: a gate + * finding is regenerated from the report every round, and what the next + * round needs from the work-list is that a Critical stood, not which id it + * stood under. + * + * The carried id is stripped through the ledger's OWN readback — a second + * spelling of that is the drift class `lib/ledger.ts`'s header exists to + * prevent — and what is matched is the gate line's LOCATOR, the + * `` `path`:line CODE `` it opens with, not the whole rendered string. A + * re-post is model-written prose: it carries the entry forward but is not + * required to reproduce the message byte for byte, and an exact-match rule + * silently stopped deduping the moment the wording drifted — which is the + * common case, not the edge. Two findings sharing a path, a line AND a + * checker code are the same finding. + */ +export function withoutGateReposts( + ownBodyCriticals: readonly string[], + gateCriticals: readonly string[], +): string[] { + const locator = (entry: string): string => { + const claim = entry.trim().replace(LEDGER_ID_READBACK, '').trim(); + const dash = claim.indexOf(' — '); + // Backticks stripped: the gate renders the path through `mdField`, and a + // re-post that types the locator plainly names the same finding. + return (dash === -1 ? claim : claim.slice(0, dash)) + .replace(/`/g, '') + .trim(); + }; + const regenerated = new Set(gateCriticals.map(locator).filter((k) => k)); + if (regenerated.size === 0) return [...ownBodyCriticals]; + return ownBodyCriticals.filter((c) => !regenerated.has(locator(c))); +} + export function scriptLintGate(planPath: string): { criticals: string[]; unreviewed: string[]; @@ -5288,6 +5598,30 @@ export const composeReviewCommand: CommandModule = { if (result.health) { writeStderrLine(`HEALTH: ${result.health.en}`); } + // The persistently-critical residual-risk advisory (#9410), when the + // carried telemetry shows the loop will not self-converge via the floor. + // Its OWN label, not the CONVERGENCE line's: both can fire in the same + // round, and one label over two different paragraphs is a terminal + // record neither an operator nor a parser can split back apart. + // Advisory only, like the VOLUME line beside it — facts plus the one + // recommendation that fits, never a threshold, never a decision: the + // land-with-residual-risk exit is the maintainer's to take. Printed only + // when the shape is provable; absence is the fail-safe reading. + if (result.residualRisk) { + // ONE line, like `VOLUME:`, `FIX:` and `CONVERGENCE:` beside it. The + // advisory carries a blank markdown table for the body, so printed + // verbatim it spread one labelled record over seven lines — six of + // them unlabelled, which is a record no line-oriented reader (an + // operator scanning, a `grep`, a log collector) can put back + // together. Collapsed rather than dropped: the pipes survive, so the + // inventory's three columns are still all there on the round where + // the body budget shed the formatted copy and this line is the copy. + writeStderrLine( + `RESIDUAL-RISK: ${convergenceAdvisory(result.residualRisk) + .en.replace(/\s+/g, ' ') + .trim()}`, + ); + } writeStderrLine(verdictLine(result)); }, }; diff --git a/packages/cli/src/commands/review/lib/convergence.test.ts b/packages/cli/src/commands/review/lib/convergence.test.ts index 6cb3745d4ec..f63f017b252 100644 --- a/packages/cli/src/commands/review/lib/convergence.test.ts +++ b/packages/cli/src/commands/review/lib/convergence.test.ts @@ -12,8 +12,12 @@ import { renderConvergenceDiagnosis, renderMechanismHealth, MAX_RENDERED_CLUSTERS, + convergenceAssessment, + convergenceAdvisory, + LAND_WITH_RESIDUAL_RISK, type ConvergenceDiagnosis, type DraftedFinding, + type ConvergenceFacts, } from './convergence.js'; import { LEDGER_MAX_ROUND, type LedgerFinding } from './ledger.js'; @@ -1131,3 +1135,268 @@ describe('isFreshDraft — a carried id no longer answers on its own', () => { ).toBe(true); }); }); + +// The persistently-critical signal is advisory telemetry: every input +// degrades OPEN, so the tests pin both the firing conjunction and each +// degraded arm individually — a false fire would tell an operator to land a +// loop that is still converging, and a missed fire is the silent status quo +// this module exists to end. + +const FIRE: ConvergenceFacts = { + prevHadCritical: true, + thisCriticals: 2, + fresh: 3, + prevFresh: 3, + floorEngaged: true, + prevFloor: 'c', + // The predecessor's work-list was Critical-only, which is what an engaged + // floor leaves behind — the stamp above cannot say so on its own. + prevPostedSuggestion: false, + // Equal to `thisCriticals`, so the backlog veto abstains and every other + // arm below is pinned on its own. A firing default whose backlog was + // already shrinking would make each `toBeNull()` below pass for the + // wrong reason. + prevCriticals: 2, + // A WHOLE predecessor list, so the two absence-derived readings above are + // evidence and the advisory publishes them unqualified. + prevTruncated: false, +}; + +describe('convergenceAssessment', () => { + it('fires on the full conjunction — persistent Criticals, fresh rate not falling', () => { + const a = convergenceAssessment(FIRE); + expect(a).not.toBeNull(); + expect(a?.shape).toBe('persistently-critical'); + expect(a?.recommendation).toBe(LAND_WITH_RESIDUAL_RISK); + expect(a?.criticals).toBe(2); + expect(a?.fresh).toBe(3); + expect(a?.prevFresh).toBe(3); + }); + + it('fires when the fresh rate is RISING — rising is not falling either', () => { + expect( + convergenceAssessment({ ...FIRE, fresh: 5, prevFresh: 3 }), + ).not.toBeNull(); + }); + + it('suppresses when the previous round was NOT recovered — undefined is not false', () => { + // A second round introducing its first Critical must not read as + // "persistent": there is no prior work-list to have carried one. + expect( + convergenceAssessment({ ...FIRE, prevHadCritical: undefined }), + ).toBeNull(); + }); + + it('suppresses when the previous work-list had no Critical', () => { + // Criticals appeared only THIS round — being worked for the first time, + // not persisted. + expect( + convergenceAssessment({ ...FIRE, prevHadCritical: false }), + ).toBeNull(); + }); + + it('suppresses when this round posts no Critical', () => { + expect(convergenceAssessment({ ...FIRE, thisCriticals: 0 })).toBeNull(); + }); + + it('suppresses when the severity floor is NOT engaged — its futility claim would be unprovable', () => { + // The advisory asserts the floor "will not converge" the loop; before + // the floor has run, the loop may still converge once it does, and a + // guess is the false fire this module must never ship. + expect(convergenceAssessment({ ...FIRE, floorEngaged: false })).toBeNull(); + }); + + it('suppresses when floor engagement is UNKNOWN — absence degrades open', () => { + expect( + convergenceAssessment({ ...FIRE, floorEngaged: undefined }), + ).toBeNull(); + }); + + it('suppresses when either fresh count is missing — a gap says nothing', () => { + // Reachable without tampering: a marker written before the fresh count + // shipped records only the total, and there is no honest way to read a + // trend off one end of a window. + expect(convergenceAssessment({ ...FIRE, fresh: undefined })).toBeNull(); + expect(convergenceAssessment({ ...FIRE, prevFresh: undefined })).toBeNull(); + }); + + it('suppresses when the fresh rate is FALLING — a converging loop', () => { + // Criticals present but the new ones drying up: the loop is settling. + // Measured on posting TOTALS this arm was unreachable — Step 6 re-posts + // every standing Critical, so the total only ever rises and a loop + // whose fresh findings fell 5 -> 4 still posted more comments than the + // round before, firing `land-with-residual-risk` over a converging + // loop. + expect( + convergenceAssessment({ ...FIRE, fresh: 1, prevFresh: 3 }), + ).toBeNull(); + }); + + it('suppresses when the standing backlog is SHRINKING — the fresh window is blind to it', () => { + // The blind spot the fresh window alone leaves: a reviewer finding + // nothing new for two rounds while the author clears blockers sits at + // fresh 0 against fresh 0, which "not falling" reads as stuck. Only the + // Critical count coming down says the loop is moving. + expect( + convergenceAssessment({ + ...FIRE, + fresh: 0, + prevFresh: 0, + thisCriticals: 3, + prevCriticals: 5, + }), + ).toBeNull(); + }); + + it('abstains on the backlog when the previous count is unknown', () => { + // A veto on positive evidence only. The work-list the count comes off + // is the one the marker's byte budget may have shortened, and an + // undercount can only hide shrinkage — never invent it — so an unknown + // predecessor must not silence a loop that is genuinely stuck. + expect( + convergenceAssessment({ ...FIRE, prevCriticals: undefined }), + ).not.toBeNull(); + }); + + it('fires at zero fresh on both rounds — the purest form of the shape', () => { + // Criticals standing round after round with nothing new found is not a + // quiet loop, it is the shape itself, so this must fire — which is why + // this signal does NOT carry the sibling diagnosis's `prev.fresh > 0` + // requirement. That module is about a loop GENERATING work; this one is + // about work that never clears. The backlog holding steady (not + // shrinking) is what separates it from a backlog being worked down. + expect( + convergenceAssessment({ + prevHadCritical: true, + thisCriticals: 3, + fresh: 0, + prevFresh: 0, + floorEngaged: true, + prevFloor: 'c', + prevPostedSuggestion: false, + prevCriticals: 3, + prevTruncated: false, + }), + ).not.toBeNull(); + }); +}); + +it('suppresses when the previous round posted under a DIFFERENT floor', () => { + // The round the floor engages on compares a Critical-only window + // against a predecessor that was still posting Suggestions. That + // movement is the posture, not the loop — and "the severity floor will + // not converge it" is not a claim one round of the floor can support. + expect(convergenceAssessment({ ...FIRE, prevFloor: 'o' })).toBeNull(); +}); + +it('suppresses when the predecessor still posted a Suggestion — the stamp lied', () => { + // The recorded floor is the REPORTING reading, which folds an absent + // `severityFloor` into `auto` and stamps `c` on any round >= 6 — even one + // the strict enforcement backstop never touched, where Suggestions posted + // normally. Paired against this round's enforcement reading, that stamp + // let an un-enforced predecessor pass as an engaged one and the advisory + // published "the severity floor will not converge it" against a window + // whose far end still included Suggestions. A Suggestion in the + // work-list is the fact the stamp cannot carry: enforcement moves drafted + // Suggestions out of the posting set before the marker is built, so an + // engaged round's list is Critical-only. + expect( + convergenceAssessment({ ...FIRE, prevPostedSuggestion: true }), + ).toBeNull(); +}); + +it('still evaluates when the predecessor work-list is unreadable', () => { + // Unknown abstains, like every other fact read off that list — a marker + // this round could not recover says nothing about what the floor did. + expect( + convergenceAssessment({ ...FIRE, prevPostedSuggestion: undefined }), + ).not.toBeNull(); +}); + +it('still evaluates when the previous floor was never recorded', () => { + // Read like the sibling diagnosis in this module: a floor that was not + // recorded is not a floor that DIFFERS. A marker written before the + // field existed must evaluate exactly as it did before this conjunct, + // or the advisory goes silent on every loop carrying an older marker. + expect( + convergenceAssessment({ ...FIRE, prevFloor: undefined }), + ).not.toBeNull(); +}); + +it('fires on a truncated predecessor, and says the reading came off one', () => { + // The gate is deliberately NOT restored: a whole-list requirement would + // silence the advisory on exactly the deep-work-list rounds it exists for, + // which are the rounds the marker's byte budget shortens. What a shortened + // list changes is what may be CLAIMED — "no Suggestion, so the floor was + // enforcing" and "the backlog is not shrinking" are both read off absence, + // and absence in a shortened list is not evidence. + const a = convergenceAssessment({ ...FIRE, prevTruncated: true }); + expect(a).not.toBeNull(); + expect(a?.prevTruncated).toBe(true); + const { en, zh } = convergenceAdvisory(a!); + expect(en).toContain('truncated to fit the marker'); + expect(en).toContain('read off a list known to be incomplete'); + expect(zh).toContain('为适配 marker 被截断'); + // And a WHOLE list publishes the readings unqualified. + const whole = convergenceAdvisory(convergenceAssessment(FIRE)!); + expect(whole.en).not.toContain('truncated to fit the marker'); + expect(whole.zh).not.toContain('为适配 marker 被截断'); +}); + +describe('convergenceAdvisory', () => { + it('renders a RISING window in the right direction, in both languages', () => { + // Every equal-count fixture reads the same number twice, so swapping + // the two interpolations keeps them all green while inverting the trend + // a maintainer reads when making the land decision. A rising window + // (fresh 5, previous 3) fires and must read this-round-first. + const a = convergenceAssessment({ ...FIRE, fresh: 5, prevFresh: 3 }); + expect(a).not.toBeNull(); + const { en, zh } = convergenceAdvisory(a!); + expect(en).toContain('this round 5, previous 3'); + expect(zh).toContain('本轮 5'); + expect(zh).toContain('上一轮 3'); + }); + + it('names the recommendation code and disclaims itself, in both languages', () => { + const a = convergenceAssessment(FIRE); + expect(a).not.toBeNull(); + const { en, zh } = convergenceAdvisory(a!); + for (const text of [en, zh]) { + expect(text).toContain(LAND_WITH_RESIDUAL_RISK); + expect(text).toContain('persistently'); + } + // Advisory-only contract: it must say it blocks nothing. + expect(en).toContain('does not block'); + expect(zh).toContain('不阻断'); + // The scaffold names the three maintainer dimensions — in BOTH + // languages. Pinned only in English, a zh scaffold that lost a column + // shipped green, and the Chinese reader is the one who cannot fall back + // to the other half of the paragraph. + expect(en).toContain('attack surface'); + expect(en).toContain('attacker-dependency'); + expect(en).toContain('blast radius'); + expect(zh).toContain('攻击面'); + expect(zh).toContain('攻击者依赖性'); + expect(zh).toContain('影响范围'); + // The claim the recommendation rests on, positively, in both. + expect(en).toContain('The severity floor will not converge it'); + expect(zh).toContain('severity floor 无法使其收敛'); + // Bounded by construction: the facts ride as numbers, never model + // prose — and the zh Critical COUNT is its own interpolation slot, not + // a repeat of the volume beside it. `FIRE` is deliberately asymmetric + // (2 Criticals, volume 3/3) so a template reading the wrong slot shows. + expect(en).toContain('2 Critical(s)'); + expect(en).toContain('this round 3, previous 3'); + expect(zh).toContain('本轮 2 条 Critical'); + expect(zh).toContain('本轮 3，上一轮 3'); + // The numbers are FIRST-TIME findings, and the sentence must say so — + // reported as "the posting volume" they described a total the signal + // does not measure, which is the false record this pipeline refuses. + expect(en).toContain('the rate of first-time findings is not falling'); + expect(en).toContain('the standing Critical backlog is not shrinking'); + expect(en).not.toContain('posting volume'); + expect(zh).toContain('首次发现的速率没有下降'); + expect(zh).toContain('未决 Critical 积压没有减少'); + expect(zh).not.toContain('发布音量'); + }); +}); diff --git a/packages/cli/src/commands/review/lib/convergence.ts b/packages/cli/src/commands/review/lib/convergence.ts index 887951f39e1..021e8815510 100644 --- a/packages/cli/src/commands/review/lib/convergence.ts +++ b/packages/cli/src/commands/review/lib/convergence.ts @@ -894,3 +894,331 @@ export function renderConvergenceDiagnosis(d: ConvergenceDiagnosis): { zh: `收敛情况：${factsZh}。${reasonZh}${caveatZh}${adviceZh}（仅为观察——本轮评审未因此扣留任何内容。）`, }; } + +// --------------------------------------------------------------------------- +// The convergence EXIT, past the diagnosis above. +// +// Everything above answers "is this loop settling, and if not, why", and its +// handling advice ends at a posture the operator can still change — including +// dropping the round to a Critical-only floor. What follows picks up where +// that advice has already been taken and the loop STILL does not settle: the +// floor is engaged, the Suggestions are gone, and the volume has flatlined on +// Criticals that never clear. The diagnosis names the shape; this names the +// way out of it (#9410). +// --------------------------------------------------------------------------- + +// Persistently-critical loop detection — the convergence exit the severity +// floor cannot provide (#9410). +// +// The floor (round 6 onward, or an explicit `critical` floor) removes +// Suggestions from posting, so a healthy loop's posting volume shrinks to its +// Criticals and then to zero as those Criticals get fixed. But a loop whose +// Criticals never clear — the security-sensitive PR under adversarial review +// that PR 9226 ran for twelve rounds — posts Criticals every round forever: +// the floor engages, the Suggestions stop, and the volume flatlines at the +// Critical count instead of falling. The floor has done its job and the loop +// STILL does not converge, and nothing before this module said so. +// +// This module names that shape. It is DATA the operator rules on, never +// authority: it computes one fact from the carried telemetry (Criticals in +// the previous round's work-list AND this round, the severity floor +// engaged, and the two-round posting window not shrinking) and, when it +// fires, surfaces the ONE recommendation +// that fits — `land-with-residual-risk`, merge and accept the residual risk. +// It decides nothing: it cannot block a post, cannot merge, cannot close, and +// holds no numeric threshold (the "two-round window" is the shortest one the +// ledger's own `posted`/`prevPosted` pair can express, not a tuned constant). +// Every input degrades OPEN — a missing volume or an unrecovered previous +// round costs a missed advisory, never a false one and never a changed post. + +/** + * The facts the signal reads, all carried by the compose boundary — nothing + * here reads a file or asks the model. + * + * `prevHadCritical` is `undefined` (not `false`) when no previous round was + * recovered: "no prior work-list" is not "the previous round had no + * Criticals". Both `false` and `undefined` suppress the signal (the guard + * is `!== true`); `undefined` marks "no previous round recovered" for + * readability, and production only ever yields `true | undefined`. + */ +export interface ConvergenceFacts { + /** Did the PREVIOUS round's carried work-list hold a Critical? */ + prevHadCritical: boolean | undefined; + /** + * Critical findings THIS round posts — inline, body-only, and relocated + * (deferred Critical markers restored to the posting set). + */ + thisCriticals: number; + /** + * How many of THIS round's comments report a finding for the FIRST time. + * + * The FRESH count, not the posting total, and for the reason the sibling + * diagnosis above measures its own trend on the same number: Step 6 + * re-posts every still-standing ledger Critical under its ORIGINAL id, so + * the re-post floor only ever rises. Measured on totals, a loop whose new + * findings collapsed from five to one still posts more comments than the + * round before — and this signal would read that as "not shrinking" and + * recommend landing with residual risk over a loop that is converging. + */ + fresh: number | undefined; + /** The PREVIOUS round's fresh count (the ledger's two-round window). */ + prevFresh: number | undefined; + /** + * How many Criticals stood in the PREVIOUS round's carried work-list, when + * that can be counted. + * + * The backlog, and it is here because the fresh window alone cannot see + * it. A loop whose reviewer finds nothing new for two rounds while the + * author clears blockers — the healthiest state a still-Critical PR can be + * in — has fresh 0 on both sides, which "not shrinking" reads as stuck. + * The standing count is what tells the two apart. + * + * A veto, not a requirement: it suppresses on OBSERVED shrinkage and + * abstains otherwise. That is what keeps it sound over a work-list the + * marker's byte budget shortened — an undercounted predecessor can only + * make the shrinkage harder to observe, never invent one. + */ + prevCriticals: number | undefined; + /** + * Was the previous round's work-list known to be INCOMPLETE — shed by the + * marker's byte budget, or refused by the admission test? + * + * It changes nothing about whether the signal fires, and that is + * deliberate: requiring a whole list would silence the advisory on the + * deep-work-list rounds it exists for, which are precisely the rounds the + * budget shortens (measured at up to 35 shed per round). What it changes + * is what the advisory may CLAIM. Two of the facts read off that list — + * "no Suggestion, so the floor was enforcing" and "the backlog is not + * shrinking" — are read off ABSENCE, and absence in a shortened list is + * not evidence. Both therefore lean toward FIRING here, against the + * fail-open direction every other input has, so the rendered paragraph + * discloses it rather than publishing an unqualified reading. The sibling + * diagnosis in this file qualifies its own recurrence reading on the same + * fact, for the same reason. + */ + prevTruncated: boolean | undefined; + /** + * Is the severity floor ENGAGED this round — an explicit `critical` + * floor, or `auto` from round 6 with the round knowable? The advisory + * claims the floor "will not converge" the loop; that claim is provable + * only where the floor is actually running, so a disengaged floor (early + * `auto` rounds, an explicit `suggestion`, an unknowable round) + * suppresses the signal — fail open, like every other conjunct. + */ + floorEngaged: boolean | undefined; + /** + * The posting floor the PREVIOUS round ran under, when its marker + * recorded one. The volume window is a two-round comparison, and two + * rounds that posted under different postures are not two points on one + * loop's trend: the round the floor engages on drops its Suggestions, so + * its volume falls against a predecessor that still posted them, and the + * round after an operator loosens the floor rises for the same reason. + * Neither movement is the loop. + * + * Read the way the sibling diagnosis in this file reads it — a floor that + * was never recorded is not a floor that DIFFERS, so a pre-field marker + * evaluates exactly as it did before this conjunct existed. + */ + prevFloor: 'c' | 'o' | undefined; + /** + * Did the PREVIOUS round's work-list still carry a Suggestion? + * + * The direct evidence that the floor was NOT enforcing there, and it is + * needed because the recorded `prevFloor` above cannot supply it. That + * stamp is written from the REPORTING reading, which folds an absent + * `severityFloor` into `auto` and so stamps `c` on any round >= 6 whose + * state named no floor at all — while the strict enforcement reading + * moved nothing and Suggestions posted normally. Pairing that stamp + * against this round's enforcement reading let a genuinely un-enforced + * predecessor pass as an engaged one, and the advisory then published + * "the severity floor will not converge it" against a window whose far + * end still included Suggestions (#9526). + * + * The work-list settles it without either reading: enforcement moves + * drafted Suggestions out of the posting set before the marker is built, + * so an engaged round's list is Critical-only and an un-enforced one is + * not. Suppresses on the POSITIVE observation, so the two ways it can be + * wrong land on opposite sides and only one of them fires: a shortened + * list that shed its Suggestion reads as engaged (bounded by the same + * truncation caveat the backlog veto carries), while a pathless + * Suggestion that an engaged round left inline reads as un-enforced and + * costs one round of silence. + */ + prevPostedSuggestion: boolean | undefined; +} + +/** The one shape this module detects. */ +export type ConvergenceShape = 'persistently-critical'; + +/** + * The one recommendation that fits a persistently-critical loop. Spelled as + * a stable code because the operator's tooling keys on it: it names the exit + * (land — merge — with the residual risk accepted), never an action the tool + * takes itself. + */ +export const LAND_WITH_RESIDUAL_RISK = 'land-with-residual-risk'; + +/** The fired assessment, all fields pure facts about the loop. */ +export interface ConvergenceAssessment { + shape: ConvergenceShape; + recommendation: typeof LAND_WITH_RESIDUAL_RISK; + /** Critical findings this round posts — what the residual inventory covers. */ + criticals: number; + /** Findings this round reported for the first time. */ + fresh: number; + /** The previous round's, the other end of the window. */ + prevFresh: number; + /** + * The predecessor's work-list was known-incomplete, so the two readings + * taken off its ABSENCES are weaker than the rest. Carried onto the + * assessment because the paragraph has to disclose it — see the field of + * the same name on `ConvergenceFacts`. + */ + prevTruncated: boolean; +} + +/** + * Detect the persistently-critical shape, or return null when the loop is not + * (provably) in it. + * + * Fires only on the conjunction, and every conjunct degrades open: + * - the previous round's work-list held a Critical (`prevHadCritical === + * true` — an UNrecovered previous round is `undefined` and suppresses the + * signal, so a second round introducing its first Critical cannot read as + * "persistent"); + * - this round posts at least one Critical; + * - the severity floor is ENGAGED this round (`floorEngaged === true`) — + * the advisory's "the floor will not converge it" claim is provable only + * where the floor is actually running; before engagement the loop may + * still converge once it does, so a disengaged floor suppresses the + * signal; + * - the previous round posted under the SAME engaged floor. Two facts say + * so and both must hold: its recorded floor is not `o`, and its + * work-list carried no Suggestion. The stamp alone is not enough — it is + * written from the reporting reading, which folds an absent floor into + * `auto` and stamps `c` on a round enforcement never touched. The round + * the floor engages on compares a Critical-only window against a + * predecessor that still posted Suggestions, and "the floor will not + * converge it" is not a claim one round of the floor can support; + * - the two-round FRESH window is present and NOT shrinking — both counts + * recorded, and this round's at least the previous round's. A falling + * rate of new findings is a converging loop even with Criticals present, + * and a missing count says nothing, so both fail open. Fresh rather than + * total, because Step 6 re-posts every standing Critical and the total + * therefore only ever rises; + * - and the standing Critical backlog is not observably shrinking. The + * fresh window cannot see this one: a loop finding nothing new while the + * author clears blockers sits at fresh 0 on both sides, which "not + * shrinking" reads as stuck. This conjunct vetoes on observed shrinkage + * and abstains when the predecessor's count is unknown. + * + * No threshold anywhere: "not shrinking" is `fresh >= prevFresh` over the + * shortest window the ledger carries, the backlog veto is a plain `<`, and + * "persistent" is two consecutive rounds with Criticals — the minimum + * evidence for each claim, derived from the carried telemetry, never tuned. + * + * One deliberate difference from the sibling diagnosis above, which also + * runs on fresh counts: it additionally requires `prev.fresh > 0`, because + * it is about a loop GENERATING work. This one must fire at fresh 0 on both + * sides — Criticals standing round after round with nothing new is not a + * quiet loop, it is the persistently-critical shape itself, and the backlog + * veto is what separates it from a backlog being cleared. + */ +export function convergenceAssessment( + facts: ConvergenceFacts, +): ConvergenceAssessment | null { + const { + prevHadCritical, + thisCriticals, + fresh, + prevFresh, + floorEngaged, + prevFloor, + prevPostedSuggestion, + prevCriticals, + prevTruncated, + } = facts; + if (prevHadCritical !== true) return null; + if (thisCriticals <= 0) return null; + if (floorEngaged !== true) return null; + // This round is `c` by the line above, so a RECORDED `o` predecessor is a + // posture change and its window is not a comparable point. Unrecorded + // stays evaluable, like the sibling diagnosis above. + if (prevFloor !== undefined && prevFloor !== 'c') return null; + // And a `c` STAMP is not proof the floor enforced: the stamp comes from + // the reporting fold. A Suggestion in the predecessor's work-list is the + // proof, and it says the floor did not. + if (prevPostedSuggestion === true) return null; + if (fresh === undefined || prevFresh === undefined) return null; + if (fresh < prevFresh) return null; + // The backlog veto. Positive evidence only: an unknown predecessor count + // abstains rather than suppressing, and a shortened work-list can only + // hide shrinkage, never manufacture it. + if (prevCriticals !== undefined && thisCriticals < prevCriticals) { + return null; + } + return { + shape: 'persistently-critical', + recommendation: LAND_WITH_RESIDUAL_RISK, + criticals: thisCriticals, + fresh, + prevFresh, + prevTruncated: prevTruncated === true, + }; +} + +/** + * The advisory sentence, bilingual — one rendering shared by the body clause + * and the terminal line so the two surfaces cannot drift. Pure facts plus the + * recommendation code; it names the exit, then disclaims itself: advisory + * only, blocks nothing. The residual-risk inventory is scaffolded as a blank + * three-column table (attack surface · attacker-dependency · blast radius) + * for the maintainer to complete — the tool cannot judge those dimensions, + * and a scaffold it pre-filled would be a verdict it has no authority to + * make. Bounded by construction: fixed prose plus a count, no model text. + * + * Led by "Residual risk", not by "Convergence": the loop-settling + * observation above already opens its paragraph that way, both can render + * into the SAME body, and two paragraphs with one opening word is a body + * whose reader cannot tell which one is speaking. The lead-in matches the + * recommendation it carries and the terminal label it prints under. + */ +export function convergenceAdvisory(a: ConvergenceAssessment): { + en: string; + zh: string; +} { + const en = + `Residual risk: this loop is persistently critical — Criticals stood in ` + + `the previous round's work-list and stand again this round (${a.criticals} ` + + `Critical(s)), the rate of first-time findings is not falling (this ` + + `round ${a.fresh}, previous ${a.prevFresh}), and the standing Critical ` + + `backlog is not shrinking${ + a.prevTruncated + ? ` — though the previous round's work list was truncated to fit the ` + + `marker, so "the backlog is not shrinking" and "the floor was ` + + `enforcing" are both read off a list known to be incomplete` + : '' + }. The severity floor will not ` + + `converge it. Recommendation: \`${a.recommendation}\` — the exit is a ` + + `maintainer risk-acceptance decision (merge, carrying the residual risk), ` + + `not another review round. Residual-risk inventory for that decision ` + + `(maintainer to complete):\n\n` + + `| standing Critical | attack surface | attacker-dependency | blast radius |\n` + + `| --- | --- | --- | --- |\n` + + `| (each standing Critical) | … | … | … |\n\n` + + `Advisory only — it does not block this review.`; + const zh = + `残余风险：本循环处于 persistently-critical 形态——上一轮工作清单中的 Critical ` + + `本轮依然存在（本轮 ${a.criticals} 条 Critical），首次发现的速率没有下降（本轮 ` + + `${a.fresh}，上一轮 ${a.prevFresh}），且未决 Critical 积压没有减少${ + a.prevTruncated + ? `——但上一轮的工作清单为适配 marker 被截断，因此「积压没有减少」与` + + `「floor 已在执法」都是从一份已知不完整的清单上读出的` + : '' + }。severity floor 无法使其收敛。` + + `建议：\`${a.recommendation}\`——出口是 maintainer 的风险接受决定（合入并` + + `承担残余风险），而非再开一轮评审。供该决定使用的残余风险清单（maintainer 填写）：` + + `按每条未决 Critical 列出「攻击面 · 攻击者依赖性 · 影响范围」三栏。` + + `仅为建议——不阻断本次评审。`; + return { en, zh }; +} diff --git a/packages/cli/src/commands/review/save-artifact.test.ts b/packages/cli/src/commands/review/save-artifact.test.ts index afc6c163c54..f0fea9c6d0b 100644 --- a/packages/cli/src/commands/review/save-artifact.test.ts +++ b/packages/cli/src/commands/review/save-artifact.test.ts @@ -526,6 +526,86 @@ describe('saveReviewArtifact', () => { ).toThrow(/health\.zh/); }); + it('carries the residual-risk advisory into the artifact (#9526)', () => { + // For the reason its sibling paragraph above is carried: rank 2 sheds + // before the not-reviewed disclosures, so the rounds that fire the + // advisory are exactly the long rounds whose body is most likely to drop + // it — and a maintainer reading `.qwen/reviews` to make the + // `land-with-residual-risk` call would otherwise find a "did not fit" + // breadcrumb and none of the facts behind it. + const paths = fixture(); + writeJson(paths.composed, { + ...verdict, + residualRisk: { + shape: 'persistently-critical', + recommendation: 'land-with-residual-risk', + criticals: 2, + fresh: 3, + prevFresh: 3, + }, + }); + saveReviewArtifact({ ...paths, target: 'local', effort: 'medium' }); + const saved = JSON.parse(readFileSync(paths.out, 'utf8')); + expect(saved.verdict.residualRisk).toEqual({ + shape: 'persistently-critical', + recommendation: 'land-with-residual-risk', + criticals: 2, + fresh: 3, + prevFresh: 3, + // Absent in the composed JSON reads as "not disclosed", never as a + // refusal — an artifact written before the caveat existed still saves. + prevTruncated: false, + }); + }); + + it('refuses a residual-risk advisory of the wrong shape (#9526)', () => { + // Shape-checked rather than passed through, like every other field on + // this boundary: the composed JSON is a file on disk between two + // processes, and a consumer reading `criticals` off a hand-edited + // artifact must not read a string. Absence stays absence — a round that + // did not fire the signal is not a malformed round. + const paths = fixture(); + saveReviewArtifact({ ...paths, target: 'local', effort: 'medium' }); + expect( + 'residualRisk' in JSON.parse(readFileSync(paths.out, 'utf8')).verdict, + ).toBe(false); + for (const bad of [ + { + shape: 'something-else', + recommendation: 'land-with-residual-risk', + criticals: 1, + posted: 1, + prevPosted: 1, + }, + { + shape: 'persistently-critical', + recommendation: 'merge-it', + criticals: 1, + posted: 1, + prevPosted: 1, + }, + { + shape: 'persistently-critical', + recommendation: 'land-with-residual-risk', + criticals: '1', + posted: 1, + prevPosted: 1, + }, + { + shape: 'persistently-critical', + recommendation: 'land-with-residual-risk', + criticals: 1, + posted: -1, + prevPosted: 1, + }, + ]) { + writeJson(paths.composed, { ...verdict, residualRisk: bad }); + expect(() => + saveReviewArtifact({ ...paths, target: 'local', effort: 'medium' }), + ).toThrow(/residualRisk/); + } + }); + 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 3a0d276c2bc..43aa304a13a 100644 --- a/packages/cli/src/commands/review/save-artifact.ts +++ b/packages/cli/src/commands/review/save-artifact.ts @@ -34,7 +34,9 @@ import { REVIEWS_DIR } from './lib/paths.js'; import { isSameFile } from './lib/same-file.js'; import { volumeOf } from './lib/ledger.js'; import { + LAND_WITH_RESIDUAL_RISK, RECOMMENDATION_CODES, + type ConvergenceAssessment, type Recommendation, } from './lib/convergence.js'; import { writeStderrLine, writeStdoutLine } from '../../utils/stdioHelpers.js'; @@ -56,6 +58,18 @@ interface PersistedVerdict * here would advertise a field no artifact contains and license a * consumer into an always-undefined branch. The two-round window stays * recoverable from the marker chain inside `body`. + * + * `residualRisk` is NOT omitted, and for the reason its sibling + * `convergence` is not: the artifact is where a trimmed round's record + * lives. Not the same rank, though — `convergence` is rank 0 and sheds + * before everything, while this one is rank 2 and yields after the fold + * and the deferral display. What they share is that both CAN go, and the + * body is then not a copy of either. "The advisory rides the persisted body" is true of every round + * except the ones that most need the durable copy — a maintainer reading + * `.qwen/reviews` to make the `land-with-residual-risk` call would find a + * "did not fit" breadcrumb and no facts. The validator carries and + * shape-checks it below, so the type advertises nothing the artifact does + * not hold. */ postedInline?: number; /** @@ -362,6 +376,63 @@ function validateVerdict(value: unknown): PersistedVerdict { zh: string(h['zh'], 'Composed verdict.health.zh'), }; } + // The residual-risk advisory, carried for the same reason its sibling + // paragraph above is: rank 2 sheds before the not-reviewed disclosures, so + // the rounds that fire it are exactly the long, deep-work-list rounds whose + // body is most likely to drop it — and the durable record is then the only + // place the facts survive. Shape-checked rather than passed through: the + // composed JSON is a file on disk between two processes, and a consumer + // reading `criticals` off a hand-edited artifact must not read a string. + // The recommendation is pinned to the ONE code this module issues; a + // future second recommendation widens this check deliberately rather than + // arriving unannounced in a durable record. + const rawResidualRisk = verdict['residualRisk']; + let residualRisk: ConvergenceAssessment | undefined; + if (rawResidualRisk !== undefined && rawResidualRisk !== null) { + const r = object(rawResidualRisk, 'Composed verdict.residualRisk'); + const shape = string(r['shape'], 'Composed verdict.residualRisk.shape'); + if (shape !== 'persistently-critical') { + throw new Error( + "Composed verdict.residualRisk.shape must be 'persistently-critical'.", + ); + } + const recommendation = string( + r['recommendation'], + 'Composed verdict.residualRisk.recommendation', + ); + if (recommendation !== LAND_WITH_RESIDUAL_RISK) { + throw new Error( + `Composed verdict.residualRisk.recommendation must be '${LAND_WITH_RESIDUAL_RISK}'.`, + ); + } + // Through the ledger's own volume reader, like every other count that + // crosses this boundary: the caps are what keep a hand-edited artifact + // from re-displaying a number no round could have posted. + const counts: Record<'criticals' | 'fresh' | 'prevFresh', number> = { + criticals: 0, + fresh: 0, + prevFresh: 0, + }; + for (const key of ['criticals', 'fresh', 'prevFresh'] as const) { + const n = volumeOf(r[key]); + if (n === undefined) { + throw new Error( + `Composed verdict.residualRisk.${key} must be a non-negative integer.`, + ); + } + counts[key] = n; + } + // The caveat is a boolean the paragraph turns on, so absence reads as + // "not disclosed" rather than refusing an artifact written before the + // field existed — the same absence semantics its numeric siblings get + // one boundary up. + residualRisk = { + shape: 'persistently-critical', + recommendation: LAND_WITH_RESIDUAL_RISK, + ...counts, + prevTruncated: r['prevTruncated'] === true, + }; + } // 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; @@ -425,6 +496,7 @@ function validateVerdict(value: unknown): PersistedVerdict { ...(convergence === undefined ? {} : { convergence }), ...(recommendations === undefined ? {} : { recommendations }), ...(health === undefined ? {} : { health }), + ...(residualRisk === undefined ? {} : { residualRisk }), lowSignal: lowSignal === null ? null diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index 15479f586d3..878e90aca23 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -941,7 +941,7 @@ The rules it applies — so you can read the line it gives you, not so you can a - **Request changes** — one or more high-confidence Criticals, anchored or in the body, **whose verification is on record** (a deterministic `[build]`/`[test]` finding is pre-confirmed and needs none). - **Comment** — suggestions but no blockers, **or** an Approve that a cap took away: an uncoverable chunk, a chunk nobody read, a dimension nobody reviewed, a **reverse audit that never ran**, an existing blocker you could not rule on, a PR whose discussion you could not read. A review that did not read part of the diff — or never looked for what it missed — cannot certify it. **Or a Request changes whose blockers were never verified**: the findings still post, disclosed as unverified, but an unverified finding must not become a public blocker — a run whose verifier never launched posted a CHANGES_REQUESTED onto an external contributor's PR over a Critical its own body disclosed as unverified, and this row is what stops the next one. -**The body it returns already fits GitHub's limit.** A review body over 65,536 characters is rejected by the API **whole** — every blocker it carries with it — so `compose-review` measures the composed body (holding room for the ledger marker it appends) and, when it would overflow, trims in a fixed order: **the Chinese fold first** — it is a translation of the English above it, so dropping it costs no content at all — then the deferral display, then the not-reviewed disclosures, and **the blockers, the undecided-blocker list and the sentences that qualify the verdict never**. Every trim is disclosed at the top of the body — naming which kinds went, above the sentences that refer to them — and repeated on stderr; if the un-trimmable remainder still overflows, the body is truncated with a loud notice rather than posted as a rejection — and **that notice rides above the cut, with the others**, so nothing the cut left open can swallow it and no part of this has to model how the page renders. That last cut has an order of its own: it spends the sentences the author already received in an earlier round — the undecided-blocker list — before this round's body Criticals, which exist in no other place the author can reach. You do not shorten anything yourself to help it — a finding you drop is a finding lost, while **a finding it trims stays whole in the findings artifact** (each deferral is its own `D-` entry there). **A trimmed disclosure section is not a finding and has no other durable copy** — the artifact persists findings, counts and the trimmed body, so the not-reviewed, deferred-checker, Test-Plan and repository-context text exists nowhere else once the body drops it. The stderr line names which kinds went: **say in your Step 6 terminal summary what was trimmed and what it said.** That summary is the copy. +**The body it returns already fits GitHub's limit.** A review body over 65,536 characters is rejected by the API **whole** — every blocker it carries with it — so `compose-review` measures the composed body (holding room for the ledger marker it appends) and, when it would overflow, trims in a fixed order: **the Chinese fold first** — it is a translation of the English above it, so dropping it costs no content at all — then the mechanism-health note, then the residual-risk advisory, then the deferral display, then the not-reviewed disclosures, then the convergence observation, and **the blockers, the undecided-blocker list and the sentences that qualify the verdict never**. Every trim is disclosed at the top of the body — naming which kinds went, above the sentences that refer to them — and repeated on stderr; if the un-trimmable remainder still overflows, the body is truncated with a loud notice rather than posted as a rejection — and **that notice rides above the cut, with the others**, so nothing the cut left open can swallow it and no part of this has to model how the page renders. That last cut has an order of its own: it spends the sentences the author already received in an earlier round — the undecided-blocker list — before this round's body Criticals, which exist in no other place the author can reach. You do not shorten anything yourself to help it — a finding you drop is a finding lost, while **a finding it trims stays whole in the findings artifact** (each deferral is its own `D-` entry there). **A trimmed disclosure section is not a finding and has no other durable copy** — the artifact persists findings, counts and the trimmed body, so the not-reviewed, deferred-checker, Test-Plan and repository-context text exists nowhere else once the body drops it. The convergence paragraphs are the exception in the other direction: the mechanism-health note, the observation and the residual-risk advisory all ride the composed verdict and print on stderr under their own `HEALTH:`, `CONVERGENCE:` and `RESIDUAL-RISK:` labels, so a round that shed them still has them — the stderr line says which of the trimmed kinds that applies to. The stderr line names which kinds went: **say in your Step 6 terminal summary what was trimmed and what it said.** That summary is the copy. **Why this is a command and not a paragraph.** It was a paragraph, and the paragraph was skipped. A run once printed an Approve it had composed itself, from prose, on a review whose gate had just refused (measured; DESIGN.md — The paraphrased roster prompt). There is now one place a verdict exists. Skipping the command does not get you a different one; it gets you none. diff --git a/packages/core/src/skills/bundled/review/SKILL.test.ts b/packages/core/src/skills/bundled/review/SKILL.test.ts index 540fda370af..424cebfa799 100644 --- a/packages/core/src/skills/bundled/review/SKILL.test.ts +++ b/packages/core/src/skills/bundled/review/SKILL.test.ts @@ -460,8 +460,12 @@ describe('bundled review skill', () => { const body = skillBody(); expect(body).toContain('rejected by the API **whole**'); expect(body).toContain('**the Chinese fold first**'); + // All four ranks, in the order the ladder actually drops them. The + // enumeration named two of them while the code had four, so a reader + // taking the skill at its word placed the advisory and the observation + // wherever seemed reasonable — and the ranks are the policy. expect(body).toContain( - 'then the deferral display, then the not-reviewed disclosures', + 'then the mechanism-health note, then the residual-risk advisory, then the deferral display, then the not-reviewed disclosures, then the convergence observation', ); // The other half of the policy. A "simplify the prose" edit turning // `never` into `last` would leave every prefix pin matching while the @@ -492,6 +496,13 @@ describe('bundled review skill', () => { expect(body).toContain( '**A trimmed disclosure section is not a finding and has no other durable copy**', ); + // ...and the exception, so the terminal-summary duty above is asked for + // where it is actually owed. Both convergence paragraphs keep a copy on + // the composed verdict and on stderr, which is why the trim line names + // WHICH of the dropped kinds the summary is the only copy of. + expect(body).toContain( + 'the mechanism-health note, the observation and the residual-risk advisory all ride the composed verdict', + ); expect(body).toContain( '**say in your Step 6 terminal summary what was trimmed and what it said.**', );