feat(review): carry the round ledger in the posted review body

packages/cli/src/commands/review/compose-review.test.ts

@@ -18,8 +18,11 @@ import { join } from 'node:path';
 import { createHash } from 'node:crypto';
 import { promptRecordDir, briefPath } from './lib/prompt-record.js';
 import { getGhHost, setGhHost } from './lib/gh.js';
+import { parseLedger } from './lib/ledger.js';
+import { countInlineFindings } from './lib/inline-counts.js';
 import {
   composeReview,
+  buildLedger,
   scriptLintGate,
   testPlanGate,
   composeReviewCommand,
@@ -3122,3 +3125,174 @@ describe('testPlanGate — Test Plan rulings, disclosed but never capping', () =
     expect(notes[5]).toBe('and 3 more');
   });
 });
+
+describe('buildLedger', () => {
+  it('numbers findings round-scoped, inline first then body Criticals', () => {
+    const l = buildLedger(
+      3,
+      [
+        {
+          path: 'src/a.ts',
+          line: 12,
+          body: '**[Critical]**: double free\ndetail',
+        },
+        { path: 'src/b.ts', line: 4, body: '**[Suggestion]** untested guard' },
+        { path: 'src/c.ts', body: 'no marker — not a finding' },
+      ],
+      ['`src/d.ts` unanchorable blocker'],
+    );
+    expect(l.round).toBe(3);
+    expect(l.findings).toEqual([
+      {
+        id: 'R3-1',
+        sev: 'C',
+        file: 'src/a.ts',
+        line: 12,
+        title: 'double free',
+      },
+      {
+        id: 'R3-2',
+        sev: 'S',
+        file: 'src/b.ts',
+        line: 4,
+        title: 'untested guard',
+      },
+      {
+        id: 'R3-3',
+        sev: 'C',
+        file: '(body)',
+        title: '`src/d.ts` unanchorable blocker',
+      },
+    ]);
+  });
+
+  it('classifies through `severityOf`, whitespace and all', () => {
+    // The ledger restated the severity predicate as a bare `startsWith`, while
+    // `countInlineFindings` — the count the VERDICT is computed from — trims
+    // first. A Critical whose body opened with a newline was therefore counted,
+    // posted, blocked the merge, and was silently missing from the ledger,
+    // shifting the id of every finding after it.
+    const drafted = [
+      { path: 'src/a.ts', line: 1, body: '\n  **[Critical]** leading space' },
+      { path: 'src/b.ts', line: 2, body: '**[Suggestion]** plain' },
+    ];
+    expect(countInlineFindings(drafted)).toEqual({
+      criticalsInline: 1,
+      suggestionsInline: 1,
+    });
+    expect(buildLedger(1, drafted, []).findings).toEqual([
+      {
+        id: 'R1-1',
+        sev: 'C',
+        file: 'src/a.ts',
+        line: 1,
+        title: 'leading space',
+      },
+      { id: 'R1-2', sev: 'S', file: 'src/b.ts', line: 2, title: 'plain' },
+    ]);
+  });
+
+  it('keeps a carried-forward id instead of renumbering it by position', () => {
+    // Step 6 re-reports a still-standing finding under its ORIGINAL id, so the
+    // report says `R1-2 still stands` — and a ledger that renumbered it `R3-1`
+    // handed the next round a work list keyed by ids the report never used,
+    // which is the whole thing `R1-2 names the same claim every round` promised.
+    const l = buildLedger(
+      3,
+      [
+        { path: 'a.ts', line: 4, body: '**[Critical]** R1-2: still leaking' },
+        { path: 'b.ts', body: '**[Suggestion]** brand new this round' },
+        { path: 'c.ts', body: '**[Critical]** R2-1 — moved but the same' },
+      ],
+      ['R1-5) the unanchorable one, still open'],
+    );
+    expect(l.findings.map((f) => `${f.id}|${f.title}`)).toEqual([
+      'R1-2|still leaking',
+      'R3-1|brand new this round',
+      'R2-1|— moved but the same',
+      'R1-5|the unanchorable one, still open',
+    ]);
+  });
+
+  it('never issues one id twice, however the comments are worded', () => {
+    // A duplicated carried id (a copy-paste, or a title that merely opens like
+    // one) must not collapse two claims onto one ledger entry.
+    const l = buildLedger(
+      2,
+      [
+        { path: 'a.ts', body: '**[Critical]** R1-1: one' },
+        { path: 'b.ts', body: '**[Critical]** R1-1: two, same id' },
+      ],
+      [],
+    );
+    expect(l.findings.map((f) => f.id)).toEqual(['R1-1', 'R2-1']);
+  });
+});
+
+describe('the ledger marker reaches the POSTED body', () => {
+  // The feature was inert end to end: the marker was appended in the CLI
+  // handler, after composeReview() returned, so it reached only the composed
+  // JSON on disk — and `submit` posts what the PURE function returns. Every
+  // assertion here goes through composeReview, the path GitHub receives.
+  let dir: string;
+  beforeEach(() => {
+    dir = mkdtempSync(join(tmpdir(), 'ledger-e2e-'));
+  });
+  afterEach(() => rmSync(dir, { recursive: true, force: true }));
+
+  const plan = (over: Record<string, unknown> = {}) => {
+    const p = join(dir, 'plan.json');
+    writeFileSync(p, JSON.stringify({ prNumber: 8255, ...over }));
+    return p;
+  };
+
+  it('appends the marker to the body composeReview returns', () => {
+    const r = composeReview({
+      planPath: plan(),
+      modelId: 'm',
+      criticalsInline: 0,
+      suggestionsInline: 0,
+      draftedComments: [
+        { path: 'src/a.ts', line: 3, body: '**[Suggestion]** untested guard' },
+      ],
+    });
+    expect(r.body).toContain('<!-- qwen-review-ledger ');
+    const ledger = parseLedger(r.body)!;
+    expect(ledger.round).toBe(1);
+    expect(ledger.findings).toEqual([
+      {
+        id: 'R1-1',
+        sev: 'S',
+        file: 'src/a.ts',
+        line: 3,
+        title: 'untested guard',
+      },
+    ]);
+  });
+
+  it('counts the round from the side file pr-context recovered, +1', () => {
+    writeFileSync(
+      join(dir, 'qwen-review-pr-8255-prev-ledger.json'),
+      JSON.stringify({ v: 1, round: 4, findings: [] }),
+    );
+    const r = composeReview({
+      planPath: plan(),
+      modelId: 'm',
+      criticalsInline: 0,
+      suggestionsInline: 0,
+      draftedComments: [{ path: 'a.ts', body: '**[Critical]** boom' }],
+    });
+    expect(parseLedger(r.body)?.round).toBe(5);
+  });
+
+  it('carries NO marker on a local review — there is no PR to hold it', () => {
+    const r = composeReview({
+      planPath: plan({ prNumber: undefined }),
+      modelId: 'm',
+      criticalsInline: 0,
+      suggestionsInline: 0,
+      draftedComments: [{ path: 'a.ts', body: '**[Critical]** boom' }],
+    });
+    expect(r.body).not.toContain('qwen-review-ledger');
+  });
+});

packages/cli/src/commands/review/compose-review.ts

@@ -41,10 +41,16 @@ import {
 } from './lib/roster.js';
 import { diffHashOf, type ScriptLintReport } from './script-lint.js';
 import type { TestPlanReport } from './test-plan.js';
+import {
+  serializeLedger,
+  type Ledger,
+  type LedgerFinding,
+} from './lib/ledger.js';
 import {
   CRITICAL_PREFIX,
   SUGGESTION_PREFIX,
   countInlineFindings,
+  severityOf,
   unmarkedComments,
   type DraftedComment,
 } from './lib/inline-counts.js';
@@ -144,6 +150,13 @@ export interface ComposeReviewInput {
     downgradeRequestChanges?: boolean;
     downgradeReasons?: string[];
   };
+  /**
+   * The drafted inline comments this review is posting — the ledger's own
+   * input. A seam like `criticalsInline`, filled by the two CLI boundaries
+   * from the same array they count, never by the model's state JSON (the
+   * handler strips it, as it does `env` and `prBodyFetcher`).
+   */
+  draftedComments?: Array<{ path?: unknown; line?: unknown; body?: unknown }>;
   /** Model id for the footer, e.g. `qwen3.7-max`. */
   modelId: string;
 }
@@ -238,6 +251,69 @@ function toBool(value: unknown, field: string): boolean {
 export function composeReview(
   input: ComposeReviewInput,
   cliVersion = 'unknown',
+): ComposeReviewResult {
+  const result = composeReviewBody(input, cliVersion);
+  // The ledger marker rides the body THIS function returns, because this — not
+  // the CLI handler — is what `submit` calls and posts. Appending it in the
+  // handler left the feature inert end to end: the marker reached only the
+  // composed JSON on disk, which nothing in the posting path reads, so no
+  // posted review ever carried one and every round recovered `null`.
+  const marker = ledgerMarkerFor(input);
+  return marker ? { ...result, body: `${result.body}\n\n${marker}` } : result;
+}
+
+/**
+ * The next round's marker, or null when this review has no PR to carry one.
+ * Round number comes from the side file `pr-context` wrote from the PREVIOUS
+ * posted round (+1) — never from the model, never from this input.
+ */
+function ledgerMarkerFor(input: ComposeReviewInput): string | null {
+  try {
+    if (!input.planPath) return null;
+    const plan = JSON.parse(readFileSync(input.planPath, 'utf8')) as {
+      prNumber?: unknown;
+    };
+    const pr = plan?.prNumber;
+    const isPr =
+      (typeof pr === 'number' && Number.isInteger(pr) && pr > 0) ||
+      (typeof pr === 'string' && /^\d+$/.test(pr));
+    if (!isPr) return null;
+    let prevRound = 0;
+    try {
+      const prev = JSON.parse(
+        readFileSync(
+          join(
+            dirname(input.planPath),
+            `qwen-review-pr-${pr}-prev-ledger.json`,
+          ),
+          'utf8',
+        ),
+      ) as Ledger;
+      if (Number.isInteger(prev.round) && prev.round > 0)
+        prevRound = prev.round;
+    } catch {
+      // No previous posted round recovered: this is round 1.
+    }
+    return serializeLedger(
+      buildLedger(
+        prevRound + 1,
+        (input.draftedComments ?? []) as Array<{
+          path?: unknown;
+          line?: unknown;
+          body?: unknown;
+        }>,
+        toStringList(input.bodyCriticals, 'bodyCriticals'),
+      ),
+    );
+  } catch {
+    // A carry-forward convenience, never worth failing the verdict over.
+    return null;
+  }
+}
+
+function composeReviewBody(
+  input: ComposeReviewInput,
+  cliVersion: string,
 ): ComposeReviewResult {
   const criticalsInline = toCount(input.criticalsInline, 'criticalsInline');
   const suggestionsInline = toCount(
@@ -1622,6 +1698,9 @@ export const composeReviewCommand: CommandModule = {
     // fail-safe. Stripping it here keeps the register the CLI's own, not the
     // caller's, which is the whole point of the seam.
     delete parsed.prBodyFetcher;
+    // Same reasoning: the ledger's contents are the comments this run drafted,
+    // read from `--comments` below — not something a state JSON may assert.
+    delete parsed.draftedComments;
     // The inline counts are counted, not accepted — `submit` has refused them
     // since the count-beside-the-comments bug, and this boundary refusing them
     // too is what makes the Step 6 line and the posted verdict the same
@@ -1644,6 +1723,7 @@ export const composeReviewCommand: CommandModule = {
       {
         ...parsed,
         ...countInlineFindings(drafted),
+        draftedComments: drafted,
       },
       await getCliVersion(),
     );
@@ -1680,6 +1760,87 @@ export const composeReviewCommand: CommandModule = {
   },
 };
 
+/**
+ * A carried-forward finding names its ORIGINAL id right after the severity
+ * marker — `**[Critical]** R1-2: the same claim, re-reported`. Step 6 already
+ * mandates re-reporting a still-standing entry under the id it has; reading
+ * that id back here is what makes the machine ledger agree with the report it
+ * rides in, instead of renumbering the entry to a fresh `R<round>-<n>` the
+ * report never used.
+ */
+const CARRIED_ID_RE = /^(R\d+-\d+)[:.)\]]?(?=\s|$)\s*/;
+
+/**
+ * The next round's ledger: every finding this review is posting as its own —
+ * the drafted inline comments plus the body Criticals. Low-confidence findings
+ * never reach either input (they are terminal-only), so the ledger holds only
+ * claims the review stands behind, which is what the next round re-asserts.
+ */
+export function buildLedger(
+  round: number,
+  drafted: Array<{ path?: unknown; line?: unknown; body?: unknown }>,
+  bodyCriticals: string[],
+): Ledger {
+  const findings: LedgerFinding[] = [];
+  const taken = new Set<string>();
+  let next = 0;
+  /** A carried id if it is free, else the next unused id of THIS round. */
+  const idFor = (carried: string | undefined): string => {
+    if (carried && !taken.has(carried)) {
+      taken.add(carried);
+      return carried;
+    }
+    let id: string;
+    do {
+      id = `R${round}-${++next}`;
+    } while (taken.has(id));
+    taken.add(id);
+    return id;
+  };
+  /** The first line of what follows the severity marker, minus any carried id. */
+  const titleOf = (rest: string): { id?: string; title: string } => {
+    const line = rest.split('\n')[0].trim();
+    const carried = CARRIED_ID_RE.exec(line);
+    return {
+      id: carried?.[1],
+      title: (carried ? line.slice(carried[0].length) : line).trim(),
+    };
+  };
+
+  for (const c of drafted) {
+    // ONE severity predicate for the whole package. `severityOf` trims leading
+    // whitespace before matching, and it is what `countInlineFindings` — the
+    // count the verdict is computed from — and the unmarked-comment gate both
+    // use. A second `startsWith` here disagreed on exactly that whitespace: a
+    // Critical whose body opened with a newline was counted, was posted, and
+    // was silently absent from the ledger, shifting every id after it.
+    const sev = severityOf(c);
+    if (!sev) continue;
+    const marker = sev === 'critical' ? CRITICAL_PREFIX : SUGGESTION_PREFIX;
+    const body = (typeof c.body === 'string' ? c.body : '').trimStart();
+    const { id: carried, title } = titleOf(
+      body.slice(marker.length).replace(/^:?\s*/, ''),
+    );
+    findings.push({
+      id: idFor(carried),
+      sev: sev === 'critical' ? 'C' : 'S',
+      file: typeof c.path === 'string' ? c.path : '(unknown)',
+      ...(typeof c.line === 'number' ? { line: c.line } : {}),
+      title,
+    });
+  }
+  for (const b of bodyCriticals) {
+    const { id: carried, title } = titleOf(b);
+    findings.push({
+      id: idFor(carried),
+      sev: 'C',
+      file: '(body)',
+      title,
+    });
+  }
+  return { v: 1, round, findings };
+}
+
 /** The terminal verdict, in the words Step 6 is told to print. */
 export function verdictLine(r: ComposeReviewResult): string {
   const label: Record<ReviewEvent, string> = {