fix(review): count a fix-induced re-report as first-time work

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

@@ -5577,6 +5577,47 @@ describe('buildLedger', () => {
     expect(l.findings.map((f) => f.id)).toEqual(['R1-2', 'R1-3']);
     expect(l.findings[0]?.title).toBe('still leaking');
   });
+
+  it('keeps the fix-induced marking out of the carried entry title', () => {
+    // The marking is machine vocabulary about how to COUNT the comment, not
+    // part of the claim. Left in, it rides the work list into the next round,
+    // where "R1-2 (fix-induced) the retry guard drops a valid case" is the
+    // text Step 6 re-locates the claim by and the text the status table
+    // prints — the token outliving the round it described, on every carried
+    // entry, forever.
+    const l = buildLedger(
+      4,
+      [
+        {
+          path: 'src/retry.ts',
+          line: 9,
+          body: '**[Critical]** R1-2: (fix-induced) the guard drops a valid case',
+        },
+      ],
+      [],
+    );
+    expect(l.findings[0].id).toBe('R1-2');
+    expect(l.findings[0].title).toBe('the guard drops a valid case');
+    expect(l.findings[0].title).not.toContain('fix-induced');
+
+    // ...and the stripping happens ONLY beside an id. With no id there is no
+    // entry for the token to qualify, so it is ordinary claim text and must
+    // survive into the title — stripping it there would edit a finding's own
+    // words on the strength of a word it happened to open with.
+    const idless = buildLedger(
+      4,
+      [
+        {
+          path: 'src/retry.ts',
+          line: 9,
+          body: '**[Critical]** (fix-induced) a brand new hole',
+        },
+      ],
+      [],
+    );
+    expect(idless.findings[0].id).toBe('R4-1');
+    expect(idless.findings[0].title).toBe('(fix-induced) a brand new hole');
+  });
 });
 
 describe('the ledger marker reaches the POSTED body', () => {
@@ -9730,6 +9771,48 @@ describe('convergence diagnosis reaches the POSTED body', () => {
     expect(r.postedFresh).toBe(1);
   });
 
+  it('counts a fix-induced re-report as first-time work', () => {
+    // Issue #9674. A carried id means two different things since the
+    // fix-induced disposition shipped: a claim re-asserted, and a NEW defect
+    // wearing the id of the entry whose fix produced it. Reading the id alone
+    // called both re-posts, so the trend's baseline fell on exactly the
+    // churning pull requests where new work was not falling.
+    //
+    // Both arms over ONE fixture: same id, same work list, same everything
+    // but the marking. Without the differential the assertion would pass on
+    // a count that simply never moves.
+    const round = (body: string) => {
+      sideFile({
+        round: 3,
+        posted: 1,
+        fresh: 1,
+        findings: [{ id: 'R2-1', sev: 'C', file: 'src/p.ts', title: 'x' }],
+      });
+      return composeReview({
+        planPath: plan(),
+        modelId: 'm',
+        criticalsInline: 1,
+        suggestionsInline: 0,
+        draftedComments: [{ path: 'src/p.ts', line: 1, body }],
+      });
+    };
+    const stillStands = round('**[Critical]** R2-1: still open');
+    expect(parseLedger(stillStands.body)!.fresh).toBe(0);
+    expect(stillStands.postedFresh).toBe(0);
+
+    const fixInduced = round(
+      '**[Critical]** R2-1: (fix-induced) the fix opened a new hole',
+    );
+    expect(parseLedger(fixInduced.body)!.fresh).toBe(1);
+    expect(fixInduced.postedFresh).toBe(1);
+    // ...and the id still carries, so the author still reads one thread for
+    // the site. Counting it first-time is a change to the COUNT, never to
+    // which finding the comment is.
+    const marker = parseLedger(fixInduced.body)!;
+    expect(marker.findings.map((x) => x.id)).toEqual(['R2-1']);
+    expect(marker.findings[0].title).toBe('the fix opened a new hole');
+  });
+
   it('re-mints a stray id, and keeps one a shortened list may have shed', () => {
     // A claimed id naming no entry in a COMPLETE work list is a stray, and
     // recording it mints a finding under a round that never held it — which
@@ -10761,9 +10844,13 @@ describe('the convergence census and the non-convergence finding', () => {
   it("never borrows the posting trend's words for its own count", () => {
     // The two counts in one body: this blocker counts DEFECTS newly
     // identified, the convergence diagnosis counts inline comments POSTED
-    // for the first time, and they legitimately differ — a fix-induced
-    // defect re-reported under a carried id is new here and a re-post
-    // there. They collided in VOCABULARY, not arithmetic: "findings first
+    // for the first time, and they legitimately differ — this one takes
+    // every finding the round newly identified, the trend only those that
+    // reached the pull request as a first-time comment. (A fix-induced
+    // re-report used to be the sharpest case of the two diverging; since
+    // #9674 the trend counts a MARKED one as first-time too, and an
+    // unmarked carried id is still a re-post there.) They collided in
+    // VOCABULARY, not arithmetic: "findings first
     // filed in round 4" sat beside "2 of them reported for the first time"
     // over the same round, so one body published two numbers under one
     // phrase and neither could be trusted. Pin the separation from both
@@ -11079,4 +11166,99 @@ describe('draftedFindingsOf — the drafts as the convergence diagnosis reads th
       ]),
     ).toEqual([{ file: '' }, { file: '' }]);
   });
+
+  it('reads the fix-induced marking beside the id it qualifies', () => {
+    const [d] = draftedFindingsOf([
+      critical({ body: '**[Critical]** R1-2: (fix-induced) the new hole' }),
+    ]);
+    expect(d.carriedId).toBe('R1-2');
+    expect(d.fixInduced).toBe(true);
+    // A still-stands re-post is the SAME id shape without the marking, and
+    // must stay a re-post: the whole point of the token is that a carried id
+    // no longer answers the first-time question on its own.
+    const [plain] = draftedFindingsOf([
+      critical({ body: '**[Critical]** R1-2: the same old claim' }),
+    ]);
+    expect(plain.carriedId).toBe('R1-2');
+    expect(plain.fixInduced).toBeUndefined();
+  });
+
+  it('tolerates case and inner spacing in the marking', () => {
+    // It governs a COUNT, never which finding a comment is, so the reading is
+    // deliberately lenient. A spelling it still misses costs the count and
+    // nothing else — the id is already in hand by then.
+    for (const marked of [
+      '**[Critical]** R1-2: (Fix-Induced) x',
+      '**[Critical]** R1-2: ( fix-induced ) x',
+      '**[Critical]** R1-2: (FIX-INDUCED)- x',
+    ]) {
+      const [d] = draftedFindingsOf([critical({ body: marked })]);
+      expect(d.carriedId).toBe('R1-2');
+      expect(d.fixInduced).toBe(true);
+    }
+  });
+
+  it('ignores the marking where there is no id to qualify', () => {
+    // Nothing induced a defect that names no previous entry, and the comment
+    // is already counted first-time by the absent id. Honouring the token
+    // there would let a stray parenthetical speak about an entry the comment
+    // does not name.
+    const [d] = draftedFindingsOf([
+      critical({ body: '**[Critical]** (fix-induced) a brand new hole' }),
+    ]);
+    expect(d.carriedId).toBeUndefined();
+    expect(d.fixInduced).toBeUndefined();
+  });
+
+  it('never emits the marking without the id it qualifies', () => {
+    // The shape invariant, pinned where it is actually reachable: a SECOND
+    // draft under an id this round already spent has its `carriedId` dropped
+    // (the ledger mints it a fresh one), and the marking must go with it.
+    // Kept, the finding would claim to have been induced by an entry it no
+    // longer names — a shape this field's own contract forbids.
+    //
+    // This case is why the assertion above could not carry the whole rule:
+    // there the id is absent because the body has none, and the two gates
+    // that enforce this — the readback's and the projection's — masked each
+    // other, so neither could be reddened alone.
+    const both = draftedFindingsOf([
+      critical({ body: '**[Critical]** R2-1: (fix-induced) first' }),
+      critical({ body: '**[Critical]** R2-1: (fix-induced) second' }),
+    ]);
+    expect(both[0].carriedId).toBe('R2-1');
+    expect(both[0].fixInduced).toBe(true);
+    expect(both[1].carriedId).toBeUndefined();
+    expect(both[1].fixInduced).toBeUndefined();
+  });
+
+  it('never lets the marking cost the id', () => {
+    // The reason the token sits AFTER the separator instead of inside the id
+    // grammar. `LEDGER_ID_READBACK` is shared with `idFor`, so widening it to
+    // swallow a parenthetical would put the ledger's carry on the same regex
+    // as a model-written adjective: a spacing the wider grammar failed to
+    // anticipate would stop matching the id and the finding would be silently
+    // renumbered.
+    //
+    // Written as a DIFFERENTIAL rather than as a list of ids to expect. The
+    // first cut asserted `R1-2:(fix-induced) x` carries its id and reddened —
+    // correctly: `R1-2:claim` loses the id with no marking anywhere near it,
+    // because the shared grammar wants whitespace after the separator. That
+    // is pre-existing and not this token's business. What IS this token's
+    // business is that it changes nothing: every shape reads exactly the id
+    // it would have read with the marking deleted.
+    const idOf = (body: string) =>
+      draftedFindingsOf([critical({ body })])[0]?.carriedId;
+    for (const [marked, bare] of [
+      ['**[Critical]** R1-2: (fix-induced) x', '**[Critical]** R1-2: x'],
+      ['**[Critical]** R1-2 (fix-induced) x', '**[Critical]** R1-2 x'],
+      ['**[Critical]** R1-2: (fix induced) x', '**[Critical]** R1-2: x'],
+      ['**[Critical]** R1-2: (fix-induced x', '**[Critical]** R1-2: x'],
+      ['**[Critical]** R1-2:(fix-induced) x', '**[Critical]** R1-2:x'],
+    ]) {
+      expect(idOf(marked)).toBe(idOf(bare));
+    }
+    // ...and the differential is not vacuously true because every arm is
+    // undefined: the prescribed shape does carry its id.
+    expect(idOf('**[Critical]** R1-2: (fix-induced) x')).toBe('R1-2');
+  });
 });

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

@@ -1638,19 +1638,29 @@ export function aboveChurnBar(
  * **It counts DEFECTS, and it must not borrow the posting trend's words.**
  * The same body carries the convergence diagnosis, which counts inline
  * comments POSTED for the first time — and the two numbers legitimately
- * differ: a fix-induced defect re-reported under the id it came from is
- * newly identified here and a re-post there, so a round can newly identify
- * six defects while posting two first-time comments. Both readings are
- * right; what broke was the vocabulary, when this sentence said "findings
- * first filed" beside the diagnosis's "reported for the first time" and one
- * body published two numbers under one phrase. Hence "defects … newly
- * identified" — distinct words for a distinct quantity.
+ * differ: this count takes every finding the round newly identified, the
+ * trend only those that reached the pull request as a first-time comment, so
+ * a round can newly identify six defects while posting fewer than six
+ * first-time comments (some ride body Criticals, some deferrals). Both
+ * readings are right; what broke was the vocabulary, when this sentence said
+ * "findings first filed" beside the diagnosis's "reported for the first
+ * time" and one body published two numbers under one phrase. Hence "defects
+ * … newly identified" — distinct words for a distinct quantity.
  *
- * Do NOT "reconcile" the two by changing either count. Excluding carried-id
- * re-reports from `fresh` would put `induced` outside it and every such
- * census would be refused as impossible; counting them as first-time POSTS
- * would tell the volume trend a re-post is new work, which is the reading
- * `isFreshDraft` exists to refuse. The divergence is the design.
+ * Do NOT "reconcile" the two by changing either count WHOLESALE. Excluding
+ * carried-id re-reports from `fresh` would put `induced` outside it and
+ * every such census would be refused as impossible; and reading a carried id
+ * as first-time work by inference would tell the volume trend that every
+ * re-assertion of a standing finding is new work, which is the reading
+ * `isFreshDraft` exists to refuse.
+ *
+ * What DID change (#9674) is narrower than either, and is not an inference:
+ * a fix-induced re-report is MARKED as such in the comment body, and the
+ * trend counts a marked one as first-time. That is a distinction the round
+ * asserts, not one the id implies — an unmarked carried id is still a
+ * re-post to the trend, exactly as before. The divergence between the two
+ * counts is still the design; only the false premise that a carried id can
+ * mean just one thing is gone.
  */
 export function nonConvergenceCritical(
   census: { fresh: number; induced: number },
@@ -5278,25 +5288,59 @@ export const composeReviewCommand: CommandModule = {
 };
 
 /**
- * The first line of what follows the severity marker, minus any carried id.
- * A carried-forward finding names its ORIGINAL id right after the marker —
- * `**[Critical]** R1-2: the same claim, re-reported` — and reading it 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.
+ * The fix-induced marking, read from the head of the CLAIM — after the id and
+ * its separator, never inside the id grammar.
+ *
+ * Placing it there is the whole point. `LEDGER_ID_READBACK` is shared by
+ * `idFor`, so widening it to swallow a parenthetical would put the ledger's
+ * carry on the same regex as a model-written adjective: a spelling or spacing
+ * the wider grammar failed to anticipate (`R1-2(Fix-Induced):`) would stop
+ * matching the id at all, and the finding would be silently renumbered — the
+ * exact failure "one finding, one name" exists to prevent. Read here, the id
+ * is already in hand and nothing about this token can cost it: an unrecognised
+ * marking leaves the draft counted as a re-post, which is what every round did
+ * before this existed.
+ *
+ * Case-insensitive, and tolerant of inner spacing, because it governs only
+ * whether a comment counts as first-time work — never which finding it is.
+ */
+const FIX_INDUCED_READBACK = /^\(\s*fix-induced\s*\)[:.,-]?\s*/i;
+
+/**
+ * The id a claim line carries, whether that id fronts a NEW defect, and the
+ * claim itself with both stripped.
+ *
+ * `fixInduced` is the answer to a question the id alone cannot settle. Step 6
+ * re-reports two different things under a previous entry's id: a finding that
+ * STILL STANDS — the same claim, re-asserted — and a fix-induced defect, which
+ * is new work wearing the id of the entry whose fix produced it. The volume
+ * trend counts comments posted for the first time, and reading the id alone
+ * called both of them re-posts, so the trend's baseline fell on exactly the
+ * churning pull requests where new work was not falling at all.
  *
  * Module-level rather than a closure inside the ledger builder, because the
  * builder is no longer its only consumer: the convergence diagnosis reads the
  * same id to tell a re-posted still-standing finding from fresh activity, and
  * a second restatement would let one end call a comment carried while the
  * other calls it new.
  */
-function readClaim(rest: string): { id?: string; title: string } {
+function readClaim(rest: string): {
+  id?: string;
+  fixInduced: boolean;
+  title: string;
+} {
   const line = rest.split('\n')[0].trim();
   const carried = LEDGER_ID_READBACK.exec(line);
+  const afterId = (carried ? line.slice(carried[0].length) : line).trim();
+  // Only ever a marking on a CARRIED id. On a fresh finding there is no
+  // entry for the defect to have been induced by, so the token would be
+  // decoration — and honouring it there would let a stray parenthetical add
+  // a first-time count the round already gets for that comment anyway.
+  const marked = carried ? FIX_INDUCED_READBACK.exec(afterId) : null;
   return {
     id: carried?.[1],
-    title: (carried ? line.slice(carried[0].length) : line).trim(),
+    fixInduced: marked !== null,
+    title: (marked ? afterId.slice(marked[0].length) : afterId).trim(),
   };
 }
 
@@ -5379,7 +5423,7 @@ export function draftedFindingsOf(drafted: unknown): DraftedFinding[] {
   const seen = new Set<string>();
   for (const c of drafted as Array<{ path?: unknown; body?: unknown }>) {
     if (severityOf(c) === null) continue;
-    const { id } = readClaim(ledgerClaimLine(c.body));
+    const { id, fixInduced } = readClaim(ledgerClaimLine(c.body));
     // The same length bound `idFor` applies before it will carry an id: an
     // id the serializer refuses is one no work list can hold, so treating it
     // as a re-post here would call a finding carried that the ledger mints
@@ -5392,6 +5436,12 @@ export function draftedFindingsOf(drafted: unknown): DraftedFinding[] {
     out.push({
       file: typeof c.path === 'string' ? c.path : '',
       ...(carried === undefined ? {} : { carriedId: carried }),
+      // Only alongside the id it qualifies. A second draft under an id this
+      // round already spent has its `carriedId` dropped just above — the
+      // ledger mints it a fresh one — so it is first-time work by the id
+      // alone, and carrying the marking without the id would state a
+      // relationship to an entry this comment no longer names.
+      ...(carried !== undefined && fixInduced ? { fixInduced: true } : {}),
     });
   }
   return out;