diff --git a/packages/cli/src/commands/review/compose-review.test.ts b/packages/cli/src/commands/review/compose-review.test.ts index a9a7347ba9..79ba7e4c98 100644 --- a/packages/cli/src/commands/review/compose-review.test.ts +++ b/packages/cli/src/commands/review/compose-review.test.ts @@ -2693,8 +2693,9 @@ describe('composeReviewCommand handler (the CLI glue)', () => { it('prints the convergence paragraph the trim notice points at', async () => { // `noteTrimmedRanks` tells the author the shed sections "still hold — - // read them in the terminal report", and rank 0 is the first thing the - // ladder sheds. Without this line that promise names nothing. + // read them in the terminal report". The convergence paragraph is the + // LAST rank the ladder sheds, so this line is the only other copy the + // promise can point at. Without it that promise names nothing. const dir = mkdtempSync(join(tmpdir(), 'compose-convergence-')); const inputPath = join(dir, 'compose.json'); const commentsPath = join(dir, 'comments.json'); @@ -6910,8 +6911,9 @@ describe('composeReview — convergence-posture deferrals (typed channel; disclo describe("composeReview — the composed body fits GitHub's limit", () => { // A POST over 65,536 characters is rejected WHOLE — the review's blockers // included — so the body carries its own budget. What it may drop, and in - // what order, is the policy under test: the deferral display yields first, - // the not-reviewed disclosures second, the blockers and the caps never. + // what order, is the policy under test: the mechanism-health note yields + // first, then the deferral display, then the not-reviewed disclosures, + // then the convergence observation, and the blockers and the caps never. const LIMIT = 65536; /** An unpaired half in EITHER direction — the oracle was one-sided. */ const LONE_SURROGATE = @@ -9369,19 +9371,21 @@ describe('convergence diagnosis reaches the POSTED body', () => { expect(r.body).toContain('Convergence:'); }); - it('yields the whole paragraph before any disclosure that qualifies the verdict', () => { - // The rounds this fires on are the high-volume rounds most likely to - // overflow, and the paragraph decides nothing — so it is the FIRST thing - // the ladder sheds. Untagged it ranked with the blockers and outlived - // the not-reviewed disclosures, which do qualify what was read. + it('outlives every disclosure the ladder can shed', () => { + // It was rank 0 — shed second, right after the mechanism-health note — + // on the reasoning that an advisory paragraph decides nothing. The + // arithmetic refutes that ordering: rendered bilingually the paragraph + // is 603 characters on a volume-only signal and 2,372 at its largest, + // against a 56,830-character budget. Shed early it could pay for at + // most 4% of an overflow, so any overflow bigger than itself spent it + // AND went on to spend the disclosures — and the rounds this fires on + // are the high-volume ones where that is the normal case. It is rank 3 + // now: the last rank to go, because it is the cheapest to keep and the + // only one whose reader is the PR author alone. // - // The blocker is sized so the ladder sheds rank 0 — the convergence - // paragraph, which yields before every other rank — and stops. Shed - // everything and the body is identical whichever order the ladder used, - // so the order would have no guard at all, which is why this constant is - // tuned rather than round. To retune after a body-copy change: raise it - // until `Convergence:` disappears, and stop before `Not reviewed:` does. - // The window is as wide as the paragraph itself. + // The blocker is sized to land in the window where the ladder sheds + // rank 2 and stops. To retune after a body-copy change: raise it until + // `Not reviewed:` disappears, and stop before `Convergence:` does. sideFile({ round: 4, posted: 9, @@ -9392,7 +9396,43 @@ describe('convergence diagnosis reaches the POSTED body', () => { modelId: 'm', criticalsInline: 0, suggestionsInline: 1, - bodyCriticals: ['B'.repeat(55_850)], + bodyCriticals: ['B'.repeat(55_600)], + unreviewedDimensions: ['security'], + draftedComments: [ + { path: 'src/a.ts', line: 1, body: '**[Suggestion]** again' }, + ], + }); + expect(r.body.length).toBeLessThanOrEqual(65536); + expect(r.body).toContain('Convergence:'); + expect(r.body).not.toContain('Not reviewed:'); + // And the notice names what ACTUALLY went. Every notice surface keys on + // the rank, so a rank that sheds the wrong section announces the wrong + // one too. + expect(r.body).toContain('the not-reviewed and non-blocking disclosures'); + expect(r.body).not.toContain('the convergence observation'); + expect(r.bodyTrim.deferralList).toBe(false); + }); + + it('still yields — last, and named — when shedding the rest was not enough', () => { + // Ranked last is not unrankable. A body that cannot hold its blockers + // must still drop an advisory, and being ranked is what makes the trim + // notice say so instead of the paragraph vanishing silently. + // + // Sized one rung past the test above: the ladder sheds rank 2, still + // does not fit, sheds rank 3, and stops before the hard cut. To retune: + // raise it until `Convergence:` disappears, and stop before `TRUNCATED` + // appears. + sideFile({ + round: 4, + posted: 9, + findings: [{ id: 'R2-1', sev: 'S', file: 'src/a.ts', title: 'x' }], + }); + const r = composeReview({ + planPath: plan(), + modelId: 'm', + criticalsInline: 0, + suggestionsInline: 1, + bodyCriticals: ['B'.repeat(56_100)], unreviewedDimensions: ['security'], draftedComments: [ { path: 'src/a.ts', line: 1, body: '**[Suggestion]** again' }, @@ -9400,17 +9440,11 @@ describe('convergence diagnosis reaches the POSTED body', () => { }); expect(r.body.length).toBeLessThanOrEqual(65536); expect(r.body).not.toContain('Convergence:'); - // A lower-ranked disclosure outlives it: the ladder reached this far and - // the paragraph went first. - expect(r.body).toContain('Not reviewed:'); - // And the notice names what ACTUALLY went. Every notice surface keys on - // the rank, so sharing a rank with the deferral list made a round that - // shed only this paragraph announce a deferred-findings list that never - // existed and point the author at artifact entries that do not exist. expect(r.body).toContain('the convergence observation'); - expect(r.body).not.toContain('the deferred-findings list'); - expect(r.body).not.toContain('findings artifact'); - expect(r.bodyTrim.deferralList).toBe(false); + expect(r.body).toContain('the not-reviewed and non-blocking disclosures'); + // The rank path, not the cut: a truncated body would prove nothing + // about the ORDER the ranks went in. + expect(r.body).not.toContain('TRUNCATED'); }); it('stamps the posting floor this round ran under beside its volume', () => { @@ -9512,9 +9546,10 @@ describe('convergence diagnosis reaches the POSTED body', () => { expect(r.body).not.toContain('9999'); }); - it('leaves a terminal copy of the paragraph the ladder sheds first', () => { - // Rank 0 goes first, and the trim notice tells the author the trimmed - // sections "still hold — read them in the terminal report". Unlike the + it('leaves a terminal copy of the paragraph the ladder can shed', () => { + // The paragraph is the LAST rank the ladder sheds, and the trim notice + // tells the author the trimmed sections "still hold — read them in the + // terminal report" whichever rank went. Unlike the // deferral list (findings artifact) and the not-reviewed disclosures // (the model's own inputs), a diagnosis derived from the side file has // no other copy anywhere unless the composed result carries one. @@ -9608,6 +9643,13 @@ describe('convergence diagnosis reaches the POSTED body', () => { // The promise the trim notice makes is about a body that DROPPED the // paragraph. A test that asserts the body still contains it never // reaches the case the copy exists for. + // + // Sized like the two order tests above, and for the same reason: the + // paragraph is the last rank the ladder sheds, so reaching a body that + // dropped it means sizing past every other rank. The window here runs + // 55,825–56,350 — this constant sat at 55,850, twenty-five characters + // above its own floor. To retune after a body-copy change: raise it + // until `Convergence:` disappears, and stop before `TRUNCATED` appears. sideFile({ round: 4, posted: 9, @@ -9619,13 +9661,14 @@ describe('convergence diagnosis reaches the POSTED body', () => { modelId: 'm', criticalsInline: 0, suggestionsInline: 1, - bodyCriticals: ['B'.repeat(55_850)], + bodyCriticals: ['B'.repeat(56_100)], unreviewedDimensions: ['security'], draftedComments: [ { path: 'src/a.ts', line: 1, body: '**[Suggestion]** again' }, ], }); expect(r.body).not.toContain('Convergence:'); + expect(r.body).not.toContain('TRUNCATED'); expect(r.convergence?.en).toContain('Convergence:'); }); @@ -10096,8 +10139,9 @@ describe('convergence diagnosis reaches the POSTED body', () => { }); it('names the health note in the trim notice, not the convergence one', () => { - // With no diagnosis firing, rank -1 holds ONLY this note. Sharing rank 0 - // made the notice name "the convergence observation" for a section that + // With no diagnosis firing, rank -1 holds ONLY this note. Sharing the + // convergence paragraph's rank made the notice name + // "the convergence observation" for a section that // never existed in the body. sideFile({ round: 4, posted: 9, fresh: 9, findings: [] }); const r = composeReview({ diff --git a/packages/cli/src/commands/review/compose-review.ts b/packages/cli/src/commands/review/compose-review.ts index 1b15081088..7d3dd21538 100644 --- a/packages/cli/src/commands/review/compose-review.ts +++ b/packages/cli/src/commands/review/compose-review.ts @@ -829,12 +829,15 @@ export interface ComposeReviewResult { * The convergence paragraph, when a signal fired — the SAME text the body * carries, returned so a terminal copy exists. * - * The overflow ladder sheds this paragraph first, and its notice tells the + * The overflow ladder can shed this paragraph — last of its ranks, and + * see the convergence block below for why last — and its notice tells the * author the trimmed sections "still hold — read them in the terminal * report". That was a false record while this text lived only inside the * body composer: unlike the deferral list (findings artifact) and the * not-reviewed disclosures (the model's own inputs), a diagnosis derived - * from the side file has no other copy anywhere. + * from the side file has no other copy anywhere. Ranking it last does not + * retire this copy — it makes it the one that matters, because the rounds + * that reach rank 3 are the rounds that shed everything. */ convergence?: { en: string; zh: string }; /** @@ -2970,12 +2973,14 @@ function composeReviewBody( /** What a rank drops, in the author's words — the note names it. */ const RANK_NAMES: Record = { [-1]: { en: 'the mechanism-health note', zh: '机制健康说明' }, - 0: { en: 'the convergence observation', zh: '收敛情况观察' }, 1: { en: 'the deferred-findings list', zh: '延后发现清单' }, 2: { en: 'the not-reviewed and non-blocking disclosures', zh: '未审查范围与非阻断披露', }, + // Last, and see the block that carries it for why: it is the smallest + // rank and the only one whose reader is the PR author alone. + 3: { en: 'the convergence observation', zh: '收敛情况观察' }, }; /** @@ -3051,7 +3056,7 @@ function composeReviewBody( * 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 rank 0 has one too (the composed + * entry in the findings artifact) and 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 @@ -3738,8 +3743,10 @@ function composeReviewBody( ...floorEnforcedNote, { // Rank 1: the display of findings the review deliberately did NOT - // request is the first thing to yield when the body overflows — - // the artifact and the terminal report keep every entry whole. + // request is the first CONTENT rank to yield when the body + // overflows — only the operator-facing mechanism-health note + // (rank -1) goes before it — and the artifact and the terminal + // report keep every entry whole. trim: 1, en: `Deferred under the convergence posture (round ${deferredRound}, not a blocker) — recorded, not requested in this round:\n\n${deferredShown .map((entry) => `- ${mdField(entry)}`) @@ -3752,10 +3759,10 @@ function composeReviewBody( : []; // The not-reviewed disclosures yield after the deferral display and before - // nothing else: they say what the review could not certify, which the - // verdict's own cap already carries, so trimming them costs detail rather - // than the claim. (`notReviewedParts` itself stays untagged — the length - // checks below ask about presence, not about rank.) + // the convergence observation: they say what the review could not certify, + // which the verdict's own cap already carries, so trimming them costs + // detail rather than the claim. (`notReviewedParts` itself stays untagged + // — the length checks below ask about presence, not about rank.) const notReviewedForBody: Bi[] = notReviewedParts.map((p) => ({ ...p, trim: 2, @@ -3768,13 +3775,28 @@ function composeReviewBody( // paragraph here that comments on the SHAPE of the review history rather // than on the diff. // - // `trim: 0` — its own rank, shed before every other EXCEPT the - // mechanism-health note below it (rank -1, and see there for why it goes - // first). An untagged block - // ranks with the blockers and the verdict-qualifying sentences, and the - // rounds this fires on are precisely the high-volume rounds most likely to - // overflow: unranked, an advisory paragraph that decides nothing survived - // while the deferral list and the not-reviewed disclosures were spent. + // `trim: 3` — the LAST rank the ladder sheds, and the reason is + // 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 + // 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 + // high-volume ones — that is the normal case, not the edge: the author + // lost the only sentence about the SHAPE of the loop and lost the + // disclosures too. + // + // It is still ranked rather than untagged: if the body genuinely cannot + // hold the blockers, an advisory must yield, and being ranked is what + // makes the trim notice name it when it does. It is ranked LAST because + // it is the cheapest block to keep and the only one whose reader is the + // author of the pull request alone — the deferral list has a second + // durable copy in the findings artifact, the disclosures are restated in + // the terminal report, and the mechanism-health note above it is written + // for the operator, who has the `HEALTH:` line. This paragraph is the + // whole of what this pipeline tells a PR author about a loop that is not + // settling; shedding it early bought almost nothing and cost exactly that. // // A rank of its own, not a share of the deferral list's: every notice // surface keys on the RANK, not on what actually went — the rank's name, @@ -3828,15 +3850,16 @@ function composeReviewBody( anchorFailsClosed(cappedBy, scopeUnproven, dimensionGapsAreDepthOnly), }) : null; - // Its OWN rank, shed before the convergence paragraph. Sharing rank 0 made - // the notice name "the convergence observation" for a body whose rank-0 - // content was only this note — a section that never existed. It goes first + // Its OWN rank, shed before every other. Sharing the convergence + // paragraph's rank made the notice name "the convergence observation" for + // a body whose content at that rank was only this note — a section that + // never existed. It goes first // because its primary reader is the operator, who has the `HEALTH:` // terminal line, while the convergence paragraph's recommendations are // addressed to the author reading the PR. const healthBlock: Bi[] = healthNote ? [{ ...healthNote, trim: -1 }] : []; const convergenceBlock: Bi[] = convergenceNote - ? [{ ...convergenceNote, trim: 0 }] + ? [{ ...convergenceNote, trim: 3 }] : []; // The resumed-run continuity note: the run reused certified work from an @@ -4925,11 +4948,12 @@ export const composeReviewCommand: CommandModule = { : ` (previous round: ${result.prevPostedInline})`), ); // The terminal copy the body's own trim notice promises. The convergence - // paragraph is the first thing the overflow ladder sheds, and unlike the - // deferral list (findings artifact) or the not-reviewed disclosures (the - // model's own inputs) it has no other copy anywhere — so the notice's - // "read them in the terminal report" was a false record until this line - // existed. + // paragraph is the ladder's LAST rank, and unlike the deferral list + // (findings artifact) or the not-reviewed disclosures (the model's own + // inputs) it has no other copy anywhere — so the notice's "read them in + // the terminal report" was a false record until this line existed. Last + // does not mean safe: a body that reaches rank 3 has already shed every + // other rank, which is exactly when this line is the only copy left. if (result.convergence) { writeStderrLine(`CONVERGENCE: ${result.convergence.en}`); } diff --git a/packages/cli/src/commands/review/save-artifact.test.ts b/packages/cli/src/commands/review/save-artifact.test.ts index 443727cb24..afc6c163c5 100644 --- a/packages/cli/src/commands/review/save-artifact.test.ts +++ b/packages/cli/src/commands/review/save-artifact.test.ts @@ -452,8 +452,9 @@ describe('saveReviewArtifact', () => { it('carries the fresh count and the convergence paragraph into the artifact', () => { // Both are new surfaces on the composed result, and the allow-list is // where a new field silently stops existing. The paragraph matters most: - // it is the FIRST clause the overflow ladder sheds, so on the rounds it - // fires the artifact may be the only durable copy. + // the overflow ladder sheds it LAST, so a round that lost it from the + // body lost every other rank too, and the artifact may be the only + // durable copy. const paths = fixture(); writeJson(paths.composed, { ...verdict, diff --git a/packages/cli/src/commands/review/save-artifact.ts b/packages/cli/src/commands/review/save-artifact.ts index b2844362f1..3a0d276c2b 100644 --- a/packages/cli/src/commands/review/save-artifact.ts +++ b/packages/cli/src/commands/review/save-artifact.ts @@ -314,10 +314,11 @@ function validateVerdict(value: unknown): PersistedVerdict { 'Composed verdict.postedInline must be a non-negative integer.', ); } - // The convergence paragraph is the ONE clause the overflow ladder sheds - // first, and the artifact is where a trimmed round's record lives. Dropped - // by this allow-list, the durable record of a round whose body shed it - // held neither copy. + // The convergence paragraph is a clause the overflow ladder can shed — + // its last rank, so a body that shed it shed every other rank too — and + // the artifact is where a trimmed round's record lives. Dropped by this + // allow-list, the durable record of a round whose body shed it held + // neither copy. const rawConvergence = verdict['convergence']; let convergence: { en: string; zh: string } | undefined; if (rawConvergence !== undefined && rawConvergence !== null) {