Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 13 additions & 17 deletions packages/cli/src/commands/review/compose-review.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,13 +66,15 @@ import { layerAuditGate } from './lib/layer-audit-gate.js';
import { diffHashOf, type ScriptLintReport } from './script-lint.js';
import type { TestPlanReport } from './test-plan.js';
import {
LEDGER_ID_READBACK,
serializeLedger,
type Ledger,
type LedgerFinding,
} from './lib/ledger.js';
import {
CRITICAL_PREFIX,
SUGGESTION_PREFIX,
carriedClaimLine,
countInlineFindings,
severityOf,
unmarkedComments,
Expand Down Expand Up @@ -2834,16 +2836,6 @@ 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
Expand Down Expand Up @@ -2871,10 +2863,17 @@ export function buildLedger(
taken.add(id);
return id;
};
/** The first line of what follows the severity marker, minus any carried id. */
/**
* 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.
*/
const titleOf = (rest: string): { id?: string; title: string } => {
const line = rest.split('\n')[0].trim();
const carried = CARRIED_ID_RE.exec(line);
const carried = LEDGER_ID_READBACK.exec(line);
return {
id: carried?.[1],
title: (carried ? line.slice(carried[0].length) : line).trim(),
Expand Down Expand Up @@ -2902,11 +2901,8 @@ export function buildLedger(
// 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*/, ''),
);
const line = carriedClaimLine(typeof c.body === 'string' ? c.body : '');
const { id: carried, title } = titleOf(line ?? '');
const file = typeof c.path === 'string' ? c.path : '(unknown)';
findings.push({
id: idFor(carried),
Expand Down
24 changes: 24 additions & 0 deletions packages/cli/src/commands/review/lib/inline-counts.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,30 @@ export function severityOf(
return null;
}

/**
* The claim line a marked finding leads with: the severity marker, any
* colon/whitespace right after it, and every line past the first stripped.
* Null when the body opens with neither marker — `submit` refuses to post an
* unmarked finding, so an unmarked body is not a finding and has no claim
* line to read back.
*
* The ONE statement of the readback strip. compose-review's ledger builder
* and presubmit's carried-id extractor both feed this line to
* `LEDGER_ID_READBACK`, so the no-marker decision and the slice order can no
* longer drift between the write side and the read sides — the drift the
* shared regex removed for the id half (#9212 review).
*/
export function carriedClaimLine(body: string): string | null {
const sev = severityOf({ body });
if (!sev) return null;
const marker = sev === 'critical' ? CRITICAL_PREFIX : SUGGESTION_PREFIX;
const rest = body
.trimStart()
.slice(marker.length)
.replace(/^:?\s*/, '');
return rest.split('\n')[0].trim();
}

/** How many drafted comments open with each severity marker. */
export function countInlineFindings(comments: readonly DraftedComment[]): {
criticalsInline: number;
Expand Down
25 changes: 25 additions & 0 deletions packages/cli/src/commands/review/lib/ledger.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
serializeLedger,
parseLedger,
stripLedgerMarker,
LEDGER_ID_READBACK,
LEDGER_MAX_FINDINGS,
LEDGER_MAX_FILE,
LEDGER_MAX_TITLE,
Expand Down Expand Up @@ -289,3 +290,27 @@ describe('ledger marker', () => {
expect(stripLedgerMarker(body)).toBe(body);
});
});

// The prefix-anchored readback both ledger read sides share wholesale:
// compose-review's ledger builder and presubmit's re-post extractor.
describe('LEDGER_ID_READBACK', () => {
// The shared regex's docstring claims the tolerated terminator set cannot
// drift between the two ends — which only holds if the set ITSELF is
// pinned: deleting a terminator from the class survives both consuming
// suites, and a prose-variant re-post then fails extraction at both ends
// and is dropped as a plain location overlap, re-creating #9208 with
// every consumer green (#9212 review).
const cases: Array<[string, string | null]> = [
['R3-2: claim', 'R3-2'],
['R3-2. claim', 'R3-2'],
['R3-2) claim', 'R3-2'],
['R3-2] claim', 'R3-2'],
['R3-2 claim', 'R3-2'],
['R3-2', 'R3-2'],
['R3-2-1: extended run', null],
['see R3-2: cross-reference', null],
];
it.each(cases)('reads %j as %j', (line, expected) => {
expect(LEDGER_ID_READBACK.exec(line)?.[1] ?? null).toBe(expected);
});
});
23 changes: 23 additions & 0 deletions packages/cli/src/commands/review/lib/ledger.ts
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,29 @@ export interface Ledger {
*/
const SHA_RE = /^[0-9a-f]{7,64}$/;

/**
* Grammar of a ledger finding id (`R<round>-<n>`). Shared by every site
* that reads carried ids — compose-review's re-post prefix parser and
* presubmit's carried-id extractor — so the two ends cannot drift: a
* divergence makes re-posts read as plain overlaps and get dropped,
* silently re-creating #9208.
*/
export const LEDGER_ID_TOKEN = String.raw`R\d+-\d+`;

/**
* Prefix-anchored readback of a carried id off the claim line: the write side
* guarantees the id leads the line right after the severity marker, so the
* read sides key on that same position. Shared WHOLESALE — terminator
* included — by compose-review's ledger builder and presubmit's re-post
* extractor, so the tolerated terminator set cannot drift on one end only
* (#9212 review). The earlier `\b`-bounded whole-body scan also matched
* cross-references ("see R3-2 for context") and ids embedded in longer
* hyphen runs, exempting a re-post under an unrelated thread.
*/
export const LEDGER_ID_READBACK = new RegExp(
`^(${LEDGER_ID_TOKEN})[:.)\\]]?(?=\\s|$)\\s*`,
);
Comment thread
yiliang114 marked this conversation as resolved.

/** Caps keep the marker a footnote, never a payload: GitHub's body limit is
* 65,536 chars and the marker rides inside it. Every cap binds BOTH halves —
* the serializer so the write side is bounded, the parser so a hand-edited
Expand Down
Loading
Loading