diff --git a/packages/cli/src/commands/review/compose-review.ts b/packages/cli/src/commands/review/compose-review.ts index a53fcb253ec..43f2cfd47ad 100644 --- a/packages/cli/src/commands/review/compose-review.ts +++ b/packages/cli/src/commands/review/compose-review.ts @@ -66,6 +66,7 @@ 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, @@ -73,6 +74,7 @@ import { import { CRITICAL_PREFIX, SUGGESTION_PREFIX, + carriedClaimLine, countInlineFindings, severityOf, unmarkedComments, @@ -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-` 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 @@ -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-` 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(), @@ -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), diff --git a/packages/cli/src/commands/review/lib/inline-counts.ts b/packages/cli/src/commands/review/lib/inline-counts.ts index 3ac4626e723..480579b4bda 100644 --- a/packages/cli/src/commands/review/lib/inline-counts.ts +++ b/packages/cli/src/commands/review/lib/inline-counts.ts @@ -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; diff --git a/packages/cli/src/commands/review/lib/ledger.test.ts b/packages/cli/src/commands/review/lib/ledger.test.ts index dffea3bc0fa..ff727cd77b8 100644 --- a/packages/cli/src/commands/review/lib/ledger.test.ts +++ b/packages/cli/src/commands/review/lib/ledger.test.ts @@ -13,6 +13,7 @@ import { serializeLedger, parseLedger, stripLedgerMarker, + LEDGER_ID_READBACK, LEDGER_MAX_FINDINGS, LEDGER_MAX_FILE, LEDGER_MAX_TITLE, @@ -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); + }); +}); diff --git a/packages/cli/src/commands/review/lib/ledger.ts b/packages/cli/src/commands/review/lib/ledger.ts index e3422ad1974..e63df875403 100644 --- a/packages/cli/src/commands/review/lib/ledger.ts +++ b/packages/cli/src/commands/review/lib/ledger.ts @@ -84,6 +84,29 @@ export interface Ledger { */ const SHA_RE = /^[0-9a-f]{7,64}$/; +/** + * Grammar of a ledger finding id (`R-`). 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*`, +); + /** 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 diff --git a/packages/cli/src/commands/review/presubmit.test.ts b/packages/cli/src/commands/review/presubmit.test.ts index 3fa49b867f1..a9a89d2b197 100644 --- a/packages/cli/src/commands/review/presubmit.test.ts +++ b/packages/cli/src/commands/review/presubmit.test.ts @@ -364,6 +364,27 @@ describe('presubmitCommand', () => { } }); + // Shared by both existing-comment classification describes; the wider + // `id?` entry type covers the carried-id (#9208) tests unchanged. + async function presubmitWithComments( + comments: Array>, + newFindings: Array<{ path: string; line: number; id?: string }>, + ) { + ghApiAllMock.mockReturnValue(comments); + ghApiMock.mockReturnValue(null); + readFileSyncMock.mockReturnValue(JSON.stringify(newFindings)); + const handler = presubmitCommand.handler; + if (!handler) throw new Error('presubmit handler missing'); + await handler({ + ...baseArgs, + 'new-findings': '/tmp/findings.json', + } as unknown as Parameters[0]); + const [, content] = writeFileSyncMock.mock.calls.find( + ([path]) => path === '/tmp/presubmit.json', + ) ?? [null, null]; + return JSON.parse(String(content)); + } + it('sets downgradeApprove — not just a reason — when every check was skipped', async () => { // The bug this guards was found by dogfooding /review on this very change: // `downgradeReasons` gained a "CI did not run" entry while `downgradeApprove` @@ -703,24 +724,6 @@ describe('presubmitCommand', () => { // visual duplicate while a live comment sat on the same (path, line). // Authorship of the reviewing account's own top-level comments is the // footer-independent fallback. - async function presubmitWithComments( - comments: Array>, - newFindings: Array<{ path: string; line: number }>, - ) { - ghApiAllMock.mockReturnValue(comments); - ghApiMock.mockReturnValue(null); - readFileSyncMock.mockReturnValue(JSON.stringify(newFindings)); - const handler = presubmitCommand.handler; - if (!handler) throw new Error('presubmit handler missing'); - await handler({ - ...baseArgs, - 'new-findings': '/tmp/findings.json', - } as unknown as Parameters[0]); - const [, content] = writeFileSyncMock.mock.calls.find( - ([path]) => path === '/tmp/presubmit.json', - ) ?? [null, null]; - return JSON.parse(String(content)); - } const FINDINGS = [{ path: 'a.ts', line: 12 }]; @@ -852,6 +855,544 @@ describe('presubmitCommand', () => { expect(result.blockOnExistingComments).toBe(true); }); }); + + describe('existing-comment classification — carried-id re-posts (#9208)', () => { + // The overlap gate used to be purely location-based: a Step 6 ledger + // re-post lands on the original thread's line by construction, collided + // with the very comment it re-posts, and was dropped — so the carried id + // never rode the round's ledger marker. A re-post is recognized by its + // `R-` id appearing in the existing comment at the same + // location; those comments are additionally bucketed as `repost` with the + // matched ids so the drop rule can exempt them. + + const CARRIED_COMMENT = { + id: 7, + body: '**[Critical]** R3-2: eq-form rescue asymmetry _— model via Qwen Code /review (v0.21.3)_', + path: 'src/parse-args.ts', + line: 44, + commit_id: 'abc123', + user: { login: 'qwen-code-ci-bot' }, + }; + + it('marks an id-matched overlap comment as a re-post target', async () => { + const result = await presubmitWithComments( + [CARRIED_COMMENT], + [{ path: 'src/parse-args.ts', line: 44, id: 'R3-2' }], + ); + expect(result.existingComments.byBucket.overlap).toBe(1); + expect(result.existingComments.byBucket.repost).toBe(1); + expect(result.existingComments.repost[0].id).toBe(7); + expect(result.existingComments.repost[0].matchedIds).toEqual(['R3-2']); + // Still an overlap as far as the block gate goes: other findings at + // the same location without the carried id are still dropped. + expect(result.blockOnExistingComments).toBe(true); + }); + + it('reports only the matched ids when several findings share the location', async () => { + const result = await presubmitWithComments( + [CARRIED_COMMENT], + [ + { path: 'src/parse-args.ts', line: 44, id: 'R3-2' }, + { path: 'src/parse-args.ts', line: 44, id: 'R4-1' }, + ], + ); + expect(result.existingComments.byBucket.overlap).toBe(1); + expect(result.existingComments.byBucket.repost).toBe(1); + expect(result.existingComments.repost[0].matchedIds).toEqual(['R3-2']); + }); + + it("does not treat a different account's colliding id as a re-post", async () => { + // Ledger ids are per-account: another reviewer's `R3-2` at the same + // line is a plain location overlap, not a re-post target — exempting + // it would post the duplicate the gate exists to prevent. + const result = await presubmitWithComments( + [{ ...CARRIED_COMMENT, user: { login: 'maintainer-dev' } }], + [{ path: 'src/parse-args.ts', line: 44, id: 'R3-2' }], + ); + expect(result.existingComments.byBucket.overlap).toBe(1); + expect(result.existingComments.byBucket.repost).toBe(0); + // The report names the author: an authorship-refused exemption must be + // self-explanatory next to a drop line whose visible id matches. + expect(result.existingComments.overlap[0].user).toBe('maintainer-dev'); + }); + + it('extracts the carried id from a carried body longer than the 80-char report excerpt', async () => { + // The report's `CommentSummary.body` is an 80-char excerpt, but + // extraction reads the FULL body. A carried id leads the claim line + // right after the severity marker by construction, so it extracts + // however long the claim runs after it. + const longClaim = 'x'.repeat(90); + const result = await presubmitWithComments( + [ + { + ...CARRIED_COMMENT, + body: `**[Critical]** R3-2: ${longClaim} _— model via Qwen Code /review_`, + }, + ], + [{ path: 'src/parse-args.ts', line: 44, id: 'R3-2' }], + ); + expect(result.existingComments.byBucket.repost).toBe(1); + expect(result.existingComments.repost[0].matchedIds).toEqual(['R3-2']); + }); + + it('keeps the id-less fallback off when the only id token sits past the 80-char summary slice (#9212)', async () => { + // The no-token check reads the FULL body, not the 80-char + // `CommentSummary.body` excerpt: a long id-less claim whose one + // id-shaped cross-reference lands past char 80 is still not a truly + // id-less original, and the fallback must stay off. + const longClaim = 'x'.repeat(90); + const result = await presubmitWithComments( + [ + { + ...CARRIED_COMMENT, + body: `**[Critical]** ${longClaim} (see R3-2 for context) _— model via Qwen Code /review_`, + }, + ], + [{ path: 'src/parse-args.ts', line: 44, id: 'R3-2' }], + ); + expect(result.existingComments.byBucket.overlap).toBe(1); + expect(result.existingComments.byBucket.repost).toBe(0); + }); + + it('extracts the carried id from a Suggestion-severity re-post (#9212)', async () => { + // Both severity markers must strip: every other carried body in the + // suite leads with **[Critical]**, which left the Suggestion half of + // the marker strip invisible. + const result = await presubmitWithComments( + [ + { + ...CARRIED_COMMENT, + body: '**[Suggestion]** R3-2: eq-form rescue asymmetry _— model via Qwen Code /review_', + }, + ], + [{ path: 'src/parse-args.ts', line: 44, id: 'R3-2' }], + ); + expect(result.existingComments.byBucket.overlap).toBe(1); + expect(result.existingComments.byBucket.repost).toBe(1); + expect(result.existingComments.repost[0].matchedIds).toEqual(['R3-2']); + }); + + it('extracts the carried id when a colon follows the marker directly (#9212)', async () => { + // The strip tolerates a colon right after the severity marker — the + // shape the compose side's own fixtures use ('**[Critical]**: ...'). + // The strip is now one shared statement (carriedClaimLine), and this + // pins its colon branch on the presubmit side, which no fixture here + // exercised before (#9212 review). + const result = await presubmitWithComments( + [ + { + ...CARRIED_COMMENT, + body: '**[Critical]**: R3-2: eq-form rescue asymmetry _— model via Qwen Code /review_', + }, + ], + [{ path: 'src/parse-args.ts', line: 44, id: 'R3-2' }], + ); + expect(result.existingComments.byBucket.overlap).toBe(1); + expect(result.existingComments.byBucket.repost).toBe(1); + expect(result.existingComments.repost[0].matchedIds).toEqual(['R3-2']); + }); + + it('reads no carried id out of an unmarked body (#9212)', async () => { + // A re-post always leads with its severity marker — `submit` refuses + // to post an unmarked finding — so an unmarked body is not a re-post + // even when its first line opens with an id-shaped token: exempting + // it would vouch a comment that was never the finding's thread. + // buildLedger skips unmarked bodies when WRITING the ledger; the + // shared strip refuses them when READING it back, so the two ends can + // no longer disagree about what "marked" means (#9212 review). + const result = await presubmitWithComments( + [ + { + ...CARRIED_COMMENT, + body: 'R3-2: discussed offline, keeping this thread _— model via Qwen Code /review_', + }, + ], + [{ path: 'src/parse-args.ts', line: 44, id: 'R3-2' }], + ); + expect(result.existingComments.byBucket.overlap).toBe(1); + expect(result.existingComments.byBucket.repost).toBe(0); + }); + + it('does not exempt a lone comment that only cross-references the id mid-body (#9212)', async () => { + // "see R3-2 for context" mentions the id without being its thread. The + // strict prefix match must not fire on it, and the id-less fallback + // must not swallow it either: the body still carries an id-shaped + // token, so the comment is not a truly id-less original. Single + // comment on purpose — at the unambiguous count the fallback WOULD + // fire if it keyed on the prefix extractor's [] alone. + const referenced = { + ...CARRIED_COMMENT, + body: '**[Critical]** unrelated claim (see R3-2 for context) _— model via Qwen Code /review_', + }; + const result = await presubmitWithComments( + [referenced], + [{ path: 'src/parse-args.ts', line: 44, id: 'R3-2' }], + ); + expect(result.existingComments.byBucket.overlap).toBe(1); + expect(result.existingComments.byBucket.repost).toBe(0); + }); + + it('does not match an id embedded in a longer hyphen run (#9212)', async () => { + // `R3-2-1` leads the claim line, but it is not the ledger id `R3-2`: + // the prefix readback requires the id to end the token, and the + // hyphen-run token keeps the id-less fallback off too. + const extended = { + ...CARRIED_COMMENT, + body: '**[Critical]** R3-2-1: extended claim _— model via Qwen Code /review_', + }; + const result = await presubmitWithComments( + [extended], + [{ path: 'src/parse-args.ts', line: 44, id: 'R3-2' }], + ); + expect(result.existingComments.byBucket.overlap).toBe(1); + expect(result.existingComments.byBucket.repost).toBe(0); + }); + + it('sees an id wrapped in Markdown emphasis as an id token (#9212)', async () => { + // `_R3-2_` defeats `\b` anchors (`_` is a word character), but the + // id-less fallback's no-token check is unbounded on purpose: the + // mention still marks the comment as belonging to a specific finding. + const emphasised = { + ...CARRIED_COMMENT, + body: '**[Critical]** unrelated claim (see _R3-2_ for context) _— model via Qwen Code /review_', + }; + const result = await presubmitWithComments( + [emphasised], + [{ path: 'src/parse-args.ts', line: 44, id: 'R3-2' }], + ); + expect(result.existingComments.byBucket.overlap).toBe(1); + expect(result.existingComments.byBucket.repost).toBe(0); + }); + + it('exempts an id-less first-round original when the target is unambiguous (#9208)', async () => { + // First-round originals carry no id token in the body (buildLedger + // assigns first-round ids positionally). With exactly one own-account + // comment at the location and one carried finding, the re-post must + // still be exempted instead of dropped. + const result = await presubmitWithComments( + [ + { + ...CARRIED_COMMENT, + body: '**[Critical]** some claim without an id', + }, + ], + [{ path: 'src/parse-args.ts', line: 44, id: 'R3-2' }], + ); + expect(result.existingComments.byBucket.overlap).toBe(1); + expect(result.existingComments.byBucket.repost).toBe(1); + expect(result.existingComments.repost[0].matchedIds).toEqual(['R3-2']); + }); + + it('keeps the strict match when the id-less target is ambiguous (#9208)', async () => { + // Two id-less own-account comments at the same location: the re-post + // target is ambiguous, so no exemption — the drop stays visible in the + // drop log rather than silently picking one thread. + const first = { + ...CARRIED_COMMENT, + id: 10, + body: '**[Critical]** claim A without an id', + }; + const second = { + ...CARRIED_COMMENT, + id: 11, + body: '**[Critical]** claim B without an id', + }; + const result = await presubmitWithComments( + [first, second], + [{ path: 'src/parse-args.ts', line: 44, id: 'R3-2' }], + ); + expect(result.existingComments.byBucket.overlap).toBe(2); + expect(result.existingComments.byBucket.repost).toBe(0); + }); + + it('keeps the id-less exemption off when several findings share the location (#9212)', async () => { + // Two CARRIED findings at one id-less location: `wantedIds.size === 1` + // is the unambiguity precondition, and exempting here would re-post + // BOTH findings under one thread that belongs to only one of them. + const result = await presubmitWithComments( + [ + { + ...CARRIED_COMMENT, + body: '**[Critical]** some claim without an id', + }, + ], + [ + { path: 'src/parse-args.ts', line: 44, id: 'R3-2' }, + { path: 'src/parse-args.ts', line: 44, id: 'R4-1' }, + ], + ); + expect(result.existingComments.byBucket.overlap).toBe(1); + expect(result.existingComments.byBucket.repost).toBe(0); + }); + + it('keeps the id-less exemption off when the current user login is unknown (#9212)', async () => { + // With no authenticated login the authorship gate cannot vouch for ANY + // comment, so the exemption must stay off even at an unambiguous + // location — the drop still applies and stays visible. The bodies keep + // their footer so the comments ARE recognized; only the gate can + // block. The second comment covers the author-less shape (`user` + // absent, e.g. a deleted account): with the login unknown it must not + // be counted as own-account and ride the fallback either. + currentUserMock.mockReturnValue(''); + const result = await presubmitWithComments( + [ + { + ...CARRIED_COMMENT, + body: '**[Critical]** some claim without an id _— model via Qwen Code /review_', + }, + { + ...CARRIED_COMMENT, + id: 8, + user: undefined, + body: '**[Critical]** author-less claim without an id _— model via Qwen Code /review_', + }, + ], + [{ path: 'src/parse-args.ts', line: 44, id: 'R3-2' }], + ); + expect(result.existingComments.byBucket.overlap).toBe(2); + expect(result.existingComments.byBucket.repost).toBe(0); + }); + + it('keeps an author-less carried id out of the repost gate when the login is unknown (#9212)', async () => { + // The unknown-login test above covers id-LESS bodies; this one pins + // the gate itself: with no authenticated login, a comment whose + // author is absent (`user` undefined, e.g. a deleted account) must + // not ride its carried id into the repost bucket. If the + // `currentUserLogin !== ''` guard were forced true, the author-less + // comparison degenerates to `'' === ''` and WOULD match — nothing + // may be vouched as own-account while the login is unknown, id + // match or not (R2-7, #9212 review). + currentUserMock.mockReturnValue(''); + const result = await presubmitWithComments( + [ + { + ...CARRIED_COMMENT, + user: undefined, + body: '**[Critical]** R3-2: eq-form rescue asymmetry _— model via Qwen Code /review_', + }, + ], + [{ path: 'src/parse-args.ts', line: 44, id: 'R3-2' }], + ); + expect(result.existingComments.byBucket.overlap).toBe(1); + expect(result.existingComments.byBucket.repost).toBe(0); + }); + + it('counts no comment as own while the login is unknown (#9212)', async () => { + // The ambiguity pre-count is wrapped in the same unknown-login guard + // as the gate: with no authenticated login, nothing may be vouched + // own-account. An author-less comment must not ride the degenerate + // `'' === ''` comparison into the count and fire the id-less fallback + // on a comment nobody proved belongs to this account (#9212 review). + currentUserMock.mockReturnValue(''); + const result = await presubmitWithComments( + [ + { + ...CARRIED_COMMENT, + user: undefined, + body: '**[Critical]** author-less claim without an id _— model via Qwen Code /review_', + }, + ], + [{ path: 'src/parse-args.ts', line: 44, id: 'R3-2' }], + ); + expect(result.existingComments.byBucket.overlap).toBe(1); + expect(result.existingComments.byBucket.repost).toBe(0); + }); + + it('counts only current-SHA own comments for the id-less fallback (#9212)', async () => { + // The ambiguity count must ignore this account's comments at OTHER + // SHAs: a stale same-location comment of the same account inflates + // the count to 2 and disables the fallback if the commit filter in + // the counting loop is dropped (#9212 review). + const result = await presubmitWithComments( + [ + { + ...CARRIED_COMMENT, + id: 10, + body: '**[Critical]** current claim without an id', + }, + { + ...CARRIED_COMMENT, + id: 11, + commit_id: 'stale-sha', + body: '**[Critical]** stale claim without an id', + }, + ], + [{ path: 'src/parse-args.ts', line: 44, id: 'R3-2' }], + ); + expect(result.existingComments.byBucket.overlap).toBe(1); + expect(result.existingComments.byBucket.repost).toBe(1); + expect(result.existingComments.repost[0].id).toBe(10); + expect(result.existingComments.repost[0].matchedIds).toEqual(['R3-2']); + }); + + it('counts only own-account comments for the id-less fallback (#9212)', async () => { + // Another Qwen account's comment at the same location must not + // inflate the ambiguity count: dropping the login filter in the + // counting loop reaches 2 and disables the fallback for a genuinely + // unambiguous own-account original (#9212 review). + const result = await presubmitWithComments( + [ + { + ...CARRIED_COMMENT, + id: 10, + body: '**[Critical]** own claim without an id', + }, + { + ...CARRIED_COMMENT, + id: 11, + user: { login: 'qwen-other-bot' }, + body: '**[Critical]** other-account claim without an id _— model via Qwen Code /review_', + }, + ], + [{ path: 'src/parse-args.ts', line: 44, id: 'R3-2' }], + ); + expect(result.existingComments.byBucket.overlap).toBe(2); + expect(result.existingComments.byBucket.repost).toBe(1); + expect(result.existingComments.repost[0].id).toBe(10); + expect(result.existingComments.repost[0].matchedIds).toEqual(['R3-2']); + }); + + it('matches authorship case-insensitively through gate and count (#9212)', async () => { + // The login comparison lowercases both sides at BOTH sites — the + // repost gate and the ambiguity-count loop. This fixture rides the + // id-less FALLBACK, which passes through both comparisons, so + // dropping `.toLowerCase()` at either site breaks it: the case + // variant of the same account must still count as own-account + // (#9212 review). + const result = await presubmitWithComments( + [ + { + ...CARRIED_COMMENT, + user: { login: 'Qwen-Code-CI-Bot' }, + body: '**[Critical]** case-variant claim without an id', + }, + ], + [{ path: 'src/parse-args.ts', line: 44, id: 'R3-2' }], + ); + expect(result.existingComments.byBucket.overlap).toBe(1); + expect(result.existingComments.byBucket.repost).toBe(1); + expect(result.existingComments.repost[0].matchedIds).toEqual(['R3-2']); + }); + + it('extracts the carried id when the claim line starts past the 80-char summary slice (#9212)', async () => { + // Extraction reads the FULL body, not the 80-char `CommentSummary.body` + // excerpt: padding after the marker can push the id-led claim line + // past char 80, where reading the excerpt would find no id, drop the + // strict match, and — the body still carries an id token — the id-less + // fallback cannot rescue it either (#9212 review). + const padding = ' '.repeat(70); + const result = await presubmitWithComments( + [ + { + ...CARRIED_COMMENT, + body: `**[Critical]**${padding}R3-2: eq-form rescue asymmetry _— model via Qwen Code /review_`, + }, + ], + [{ path: 'src/parse-args.ts', line: 44, id: 'R3-2' }], + ); + expect(result.existingComments.byBucket.overlap).toBe(1); + expect(result.existingComments.byBucket.repost).toBe(1); + expect(result.existingComments.repost[0].matchedIds).toEqual(['R3-2']); + }); + + it('exempts the carried finding when a fresh id-less finding shares the location (#9212)', async () => { + // The findings file carries ids on carried-forward findings only; the + // fresh finding of this round omits it. The exemption must still fire + // on the single CARRIED id, not be crowded out by the fresh entry. + const result = await presubmitWithComments( + [ + { + ...CARRIED_COMMENT, + body: '**[Critical]** some claim without an id', + }, + ], + [ + { path: 'src/parse-args.ts', line: 44, id: 'R1-2' }, + { path: 'src/parse-args.ts', line: 44 }, + ], + ); + expect(result.existingComments.byBucket.overlap).toBe(1); + expect(result.existingComments.byBucket.repost).toBe(1); + expect(result.existingComments.repost[0].matchedIds).toEqual(['R1-2']); + }); + + it('matches a prior re-post as the target when it carries the id (#9212)', async () => { + // A same-SHA re-run sees the original AND the re-post already made for + // it; only the comment leading with the id is a strict match. The + // duplication this can produce on further re-runs is disclosed in the + // Known-limitation paragraph — detecting "already re-posted" needs a + // carry-forward channel and is a follow-up. + const original = { + ...CARRIED_COMMENT, + id: 10, + body: '**[Critical]** some claim without an id', + }; + const priorRepost = { + ...CARRIED_COMMENT, + id: 11, + body: '**[Critical]** R1-2: the same claim, re-reported _— model via Qwen Code /review_', + }; + const result = await presubmitWithComments( + [original, priorRepost], + [{ path: 'src/parse-args.ts', line: 44, id: 'R1-2' }], + ); + expect(result.existingComments.byBucket.overlap).toBe(2); + expect(result.existingComments.byBucket.repost).toBe(1); + expect(result.existingComments.repost[0].id).toBe(11); + expect(result.existingComments.repost[0].matchedIds).toEqual(['R1-2']); + }); + + it('counts a replied-to original toward the id-less ambiguity decision (#9212)', async () => { + // A replied-to original is bucketed `resolved`, but it is still an + // original at the location: leaving it out of the count handed the + // exemption to a sibling comment belonging to a DIFFERENT finding. + const repliedOriginal = { + ...CARRIED_COMMENT, + id: 10, + body: '**[Critical]** claim A without an id', + }; + const maintainerReply = { + id: 12, + body: 'fixing this, thanks', + path: 'src/parse-args.ts', + line: 44, + commit_id: 'abc123', + in_reply_to_id: 10, + user: { login: 'maintainer-dev' }, + }; + const siblingOriginal = { + ...CARRIED_COMMENT, + id: 11, + body: '**[Critical]** claim B without an id', + }; + const result = await presubmitWithComments( + [repliedOriginal, maintainerReply, siblingOriginal], + [{ path: 'src/parse-args.ts', line: 44, id: 'R2-1' }], + ); + expect(result.existingComments.byBucket.resolved).toBe(1); + expect(result.existingComments.byBucket.overlap).toBe(1); + expect(result.existingComments.byBucket.repost).toBe(0); + }); + + it('does not match a different carried id', async () => { + const result = await presubmitWithComments( + [CARRIED_COMMENT], + [{ path: 'src/parse-args.ts', line: 44, id: 'R3-9' }], + ); + expect(result.existingComments.byBucket.overlap).toBe(1); + expect(result.existingComments.byBucket.repost).toBe(0); + }); + + it('does not match an id carried at a different location', async () => { + const result = await presubmitWithComments( + [{ ...CARRIED_COMMENT, line: 45 }], + [{ path: 'src/parse-args.ts', line: 44, id: 'R3-2' }], + ); + expect(result.existingComments.byBucket.overlap).toBe(0); + expect(result.existingComments.byBucket.repost).toBe(0); + expect(result.existingComments.byBucket.noConflict).toBe(1); + }); + }); }); // The PR advancing mid-review means commits exist that no agent read. An @@ -1015,6 +1556,28 @@ describe('parseFindingsFile (via mocked fs)', () => { ['[{"line":5}]', null], // entry without a string path → reject WHOLE file ['[{"path":"a.ts","line":5}]', [{ path: 'a.ts', line: 5 }]], ['[{"path":"a.ts"}]', [{ path: 'a.ts', line: 0 }]], // missing line → 0 + // Carried ledger id for the re-post exemption (#9208); a present-but- + // non-string id rejects the WHOLE file, same fail-safe as `path`. + [ + '[{"path":"a.ts","line":5,"id":"R3-2"}]', + [{ path: 'a.ts', line: 5, id: 'R3-2' }], + ], + ['[{"path":"a.ts","line":5,"id":42}]', null], + // `null` means "no id" (JSON has no undefined) — a missing optional + // field, not a malformed file; the entry survives without an id. + ['[{"path":"a.ts","line":5,"id":null}]', [{ path: 'a.ts', line: 5 }]], + // Present-but-misshapen ids are rejected too: a typo'd id can never + // match the extractor, and accepting it would silently disable the + // re-post exemption (#9212 review). + ['[{"path":"a.ts","line":5,"id":"r3-2"}]', null], + ['[{"path":"a.ts","line":5,"id":"R3-2 "}]', null], + ['[{"path":"a.ts","line":5,"id":""}]', null], + // The SHAPE check is fully anchored: a padded or prefixed id contains a + // valid `R\d+-\d+` substring, so dropping either anchor would accept it + // — and an accepted `' R3-2'` can never round-trip against the + // extractor's `'R3-2'`, silently disabling the exemption (#9212 review). + ['[{"path":"a.ts","line":5,"id":" R3-2"}]', null], + ['[{"path":"a.ts","line":5,"id":"XR3-2"}]', null], ['[]', []], ]; it.each(cases)('rejects/normalizes %s', (raw, expected) => { diff --git a/packages/cli/src/commands/review/presubmit.ts b/packages/cli/src/commands/review/presubmit.ts index 6e9e6d8ba31..e3bd8c870ad 100644 --- a/packages/cli/src/commands/review/presubmit.ts +++ b/packages/cli/src/commands/review/presubmit.ts @@ -21,11 +21,24 @@ import { ensureAuthenticated, setGhHost, } from './lib/gh.js'; -import { severityOf } from './lib/inline-counts.js'; +import { carriedClaimLine, severityOf } from './lib/inline-counts.js'; +import { LEDGER_ID_READBACK, LEDGER_ID_TOKEN } from './lib/ledger.js'; interface FindingAnchor { path: string; line: number; + /** + * Ledger id (`R-`) — carried-forward findings ONLY. The + * orchestrator omits it on fresh findings of the current round: a fresh id + * can never appear in a comment posted before this round, and admitting one + * here would let a brand-new claim ride the id-less exemption into an + * unrelated thread, or crowd `wantedIds` past the single-carried-finding + * precondition and disable the exemption for a genuine re-post (#9212 + * review). Matching a carried id against an existing comment at the same + * location is what marks that comment a re-post target instead of a + * duplicate (#9208). + */ + id?: string; } interface CommentSummary { @@ -34,8 +47,46 @@ interface CommentSummary { line: number; commit_id: string; body: string; + /** + * The comment author's login, when known. The authorship gate refuses + * re-post exemptions on another account's comment; naming the author in the + * report is what makes that refusal self-explanatory — without it the drop + * line quotes a comment whose visible id matches the dropped finding, and + * nothing in the report says authorship is why (#9212 review). + */ + user?: string; + /** + * Set only on `repost` entries: the carried ledger ids a new finding at the + * same location re-posts (#9208). Usually the ids carried in this comment's + * body; on the id-less fallback (a truly id-less own-account original at an + * unambiguous location) it is the location's single wanted id instead + * (#9212 review). + */ + matchedIds?: string[]; } +/** Exact-shape check for ids read from the --new-findings file. */ +const LEDGER_ID_SHAPE = new RegExp(`^${LEDGER_ID_TOKEN}$`); +/** The carried id this comment's claim line leads with, if any. */ +function extractCarriedIds(body: string): string[] { + const line = carriedClaimLine(body) ?? ''; + const carried = LEDGER_ID_READBACK.exec(line); + return carried ? [carried[1]] : []; +} + +/** + * ANY ledger-id-shaped token, anywhere in the body — deliberately UNBOUNDED. + * The id-less fallback may only fire for a comment with NO id token at all, + * so any mention keeps the comment out of it: a mid-body cross-reference + * ("see R3-2 for context"), a hyphen run ("R3-2-1"), or a Markdown-emphasised + * `_R3-2_` (the `\b` anchors miss it — `_` is a word character). The prefix + * extractor returning [] cannot tell those apart from a truly id-less + * original, and a false positive here is the safe direction: the finding + * stays dropped and VISIBLE in the drop log instead of riding the fallback + * into an unrelated thread (#9212 review). + */ +const ANY_CARRIED_ID = new RegExp(LEDGER_ID_TOKEN); + interface RawComment { id: number; body?: string; @@ -133,8 +184,8 @@ interface PresubmitArgs { * treats an unknown finding set as at-risk, so a malformed file downgrades * the verdict rather than proving a false all-clear. A shorter-than-real list * would be the dangerous outcome (a dropped finding reads as disjoint), so - * any entry lacking a string `path` rejects the WHOLE file rather than being - * skipped. + * any entry lacking a string `path` — or carrying a non-null, non-string + * or misshapen `id` — rejects the WHOLE file rather than being skipped. */ export function parseFindingsFile(path: string): FindingAnchor[] | null { let parsed: unknown; @@ -153,10 +204,25 @@ export function parseFindingsFile(path: string): FindingAnchor[] | null { ) { return null; } - const e = entry as { path: string; line?: unknown }; + const e = entry as { path: string; line?: unknown; id?: unknown }; + // Same fail-safe as `path`: an `id` of the wrong type or shape is a + // malformed file, and silently ignoring it would let a carried re-post + // read as a fresh duplicate at the very location it belongs (a typo'd + // id can never match the extractor, silently disabling the exemption). + // `null` means "no id" — JSON has no `undefined`, so a producer that + // emits the key uniformly uses null for id-less findings; that is a + // missing optional field, not a malformed file. + if ( + e.id !== undefined && + e.id !== null && + (typeof e.id !== 'string' || !LEDGER_ID_SHAPE.test(e.id)) + ) { + return null; + } out.push({ path: e.path, line: typeof e.line === 'number' ? e.line : 0, + ...(typeof e.id === 'string' ? { id: e.id } : {}), }); } return out; @@ -391,13 +457,60 @@ export function classifyCi(checkRuns: CheckRun[], statuses: CommitStatus[]) { function classifyExistingComments( qwenComments: RawComment[], repliedToIds: Set, - newFindingKeys: Set, + newFindings: FindingAnchor[], commitSha: string, + currentUserLogin: string, ) { const buckets: Record< - 'stale' | 'resolved' | 'overlap' | 'noConflict', + 'stale' | 'resolved' | 'overlap' | 'repost' | 'noConflict', CommentSummary[] - > = { stale: [], resolved: [], overlap: [], noConflict: [] }; + > = { stale: [], resolved: [], overlap: [], repost: [], noConflict: [] }; + + const newFindingKeys = new Set(newFindings.map((f) => `${f.path}:${f.line}`)); + // Location → carried ids of the findings anchored there. Only findings with + // an id participate, and the orchestrator writes ids ONLY on carried + // findings (SKILL.md — the findings file): a fresh `R-` + // cannot appear in a comment posted before this round, so every id here is + // a genuine re-post signal, and `wantedIds.size === 1` below means exactly + // one CARRIED finding at the location (#9212 review). + const carriedIdsByLocation = new Map>(); + for (const f of newFindings) { + if (f.id === undefined) continue; + const key = `${f.path}:${f.line}`; + const ids = carriedIdsByLocation.get(key) ?? new Set(); + ids.add(f.id); + carriedIdsByLocation.set(key, ids); + } + + // Own-account Qwen comments per location at the current SHA. A count of + // exactly one makes an id-less original unambiguous as a re-post target + // (#9212 review). Replied-to comments COUNT: a replied-to original is + // still an original, and leaving it out of the ambiguity count handed the + // id-less exemption to a sibling comment belonging to a different finding + // (#9212 review). + // + // The unknown-login skip is a deliberate short-circuit, not a correctness + // boundary: the count is consumed at exactly ONE site — the id-less + // fallback inside the repost gate — and that gate itself requires a known + // login, so while the login is unknown the map built here is never + // consulted. The mutant that forces this guard true is provably + // equivalent (R6-7, #9212 review); keep the guard as defense in depth + // against a future move of the read site out of the gate. + const ownOverlapCountByLocation = new Map(); + if (currentUserLogin !== '') { + for (const c of qwenComments) { + if ( + c.commit_id === commitSha && + (c.user?.login ?? '').toLowerCase() === currentUserLogin.toLowerCase() + ) { + const key = `${c.path ?? ''}:${c.line ?? 0}`; + ownOverlapCountByLocation.set( + key, + (ownOverlapCountByLocation.get(key) ?? 0) + 1, + ); + } + } + } for (const c of qwenComments) { const summary: CommentSummary = { @@ -406,14 +519,56 @@ function classifyExistingComments( line: c.line ?? 0, commit_id: c.commit_id ?? '', body: (c.body || '').slice(0, 80), + ...(c.user?.login ? { user: c.user.login } : {}), }; - // Priority: Stale > Resolved > Overlap > NoConflict. + // Priority: Stale > Resolved > Overlap (+ Repost) > NoConflict. if (c.commit_id !== commitSha) { buckets.stale.push(summary); } else if (repliedToIds.has(c.id)) { buckets.resolved.push(summary); } else if (newFindingKeys.has(`${c.path}:${c.line}`)) { + // Overlap stays location-based: a same-line finding with a DIFFERENT + // claim is still dropped (the drop log now names this comment so the + // false positive is visible — #9208). Repost is the additional, id-based + // bucket: a Step 6 ledger re-post lands on the original thread's line by + // construction and carries the original id in its prefix, so an id match + // marks the re-post target and exempts that finding from the drop. buckets.overlap.push(summary); + const wantedIds = carriedIdsByLocation.get(`${c.path}:${c.line}`); + // Ledger ids are per-account — two reviewers of the same PR keep two + // independent ledgers, each with its own `R2-1` — so only THIS + // account's comments can carry a re-post of its own finding. A + // different account's colliding id at the same line must stay a + // plain location overlap (#9212 review). + if ( + wantedIds && + currentUserLogin !== '' && + (c.user?.login ?? '').toLowerCase() === currentUserLogin.toLowerCase() + ) { + const matchedIds = extractCarriedIds(c.body || '').filter((id) => + wantedIds.has(id), + ); + if (matchedIds.length > 0) { + buckets.repost.push({ ...summary, matchedIds }); + } else if ( + !ANY_CARRIED_ID.test(c.body || '') && + wantedIds.size === 1 && + ownOverlapCountByLocation.get(`${c.path}:${c.line}`) === 1 + ) { + // First-round originals can carry NO id token in their body + // (buildLedger assigns first-round ids positionally), so the body + // match alone would drop exactly the re-post this gate protects. + // When the target is unambiguous — a TRULY id-less own comment (no + // carried id at all, so it cannot belong to a different finding), + // one carried finding, and exactly one own-account comment at this + // location — treat it as the re-post target. A comment carrying + // SOME OTHER id is a different finding's thread and keeps the + // strict match; ambiguous cases (several id-less comments or + // several carried ids at one line) keep the strict body match too, + // staying dropped and visible in the drop log (#9212 review). + buckets.repost.push({ ...summary, matchedIds: [...wantedIds] }); + } + } } else { buckets.noConflict.push(summary); } @@ -509,7 +664,7 @@ async function runPresubmit(args: PresubmitArgs): Promise { : null; // A path was given but did not parse into a usable list. The drift path // already fails safe (findingPaths=null → anchorsAtRisk true), but the SAME - // null silently empties `newFindingKeys` below, disabling the existing- + // null collapses to an empty finding list below, disabling the existing- // comment overlap check — a run then can't tell "no overlaps" from "the // dedup input was garbage", and may re-post comments a prior run already // made. Surface it (report flag + downgrade reason) instead of degrading in @@ -575,15 +730,12 @@ async function runPresubmit(args: PresubmitArgs): Promise { if (c.in_reply_to_id) repliedToIds.add(c.in_reply_to_id); } - const newFindingKeys = new Set( - (newFindings ?? []).map((f) => `${f.path}:${f.line}`), - ); - const buckets = classifyExistingComments( qwenComments, repliedToIds, - newFindingKeys, + newFindings ?? [], commitSha, + me, ); // --- Downgrade decisions ---------------------------------------------- @@ -630,9 +782,18 @@ async function runPresubmit(args: PresubmitArgs): Promise { stale: buckets.stale.length, resolved: buckets.resolved.length, overlap: buckets.overlap.length, + repost: buckets.repost.length, noConflict: buckets.noConflict.length, }, overlap: buckets.overlap, + // Overlap comments that a new finding at the same location re-posts — + // the drop rule exempts those findings (#9208). Matched by the carried + // ledger id the comment's claim line leads with, or — when the target + // is unambiguous — by the id-less fallback for a truly id-less + // own-account original (#9212 review). A comment appears here IN + // ADDITION TO `overlap`; the double count is deliberate (one comment, + // two roles). + repost: buckets.repost, stale: buckets.stale, resolved: buckets.resolved, noConflict: buckets.noConflict, @@ -702,7 +863,7 @@ export const presubmitCommand: CommandModule = { .option('new-findings', { type: 'string', describe: - 'Path to a JSON file shaped as [{path, line}, ...] — when provided, existing comments are checked for same-(path, line) overlap with the new findings.', + "Path to a JSON file shaped as [{path, line, id?}, ...] — when provided, existing comments are checked for same-(path, line) overlap with the new findings. `id` is the finding's carried ledger id (`R-`) and belongs on CARRIED-forward findings only — omit it on fresh findings of this round: an id-matched own-account comment at the same location is additionally reported in `repost` so the drop rule can exempt the re-post, and a fresh id could only corrupt that match.", }), handler: async (argv) => { setGhHost((argv as { host?: string }).host); diff --git a/packages/core/src/skills/bundled/review/DESIGN.md b/packages/core/src/skills/bundled/review/DESIGN.md index c6a00638ce3..108ff17076d 100644 --- a/packages/core/src/skills/bundled/review/DESIGN.md +++ b/packages/core/src/skills/bundled/review/DESIGN.md @@ -262,7 +262,7 @@ PR #6486: the one job that would have exercised the new `Ctrl+F` hotkey — `Int **Current behavior:** the deterministic logic lives in `packages/cli/src/commands/review/` as TypeScript subcommands of the `qwen` CLI: -- `qwen review presubmit ` — emits a single JSON report with `isSelfPr`, `ciStatus`, `existingComments` (4 buckets), `downgradeApprove`, `downgradeRequestChanges`, `downgradeReasons`, `blockOnExistingComments`. SKILL.md only describes the schema and how to apply the report. +- `qwen review presubmit ` — emits a single JSON report with `isSelfPr`, `ciStatus`, `existingComments` (5 buckets), `downgradeApprove`, `downgradeRequestChanges`, `downgradeReasons`, `blockOnExistingComments`. SKILL.md only describes the schema and how to apply the report. - `qwen review cleanup ` — removes the worktree, branch ref, and per-target temp files. Idempotent. **Why subcommands rather than `.mjs` scripts in the skill bundle:** @@ -658,7 +658,7 @@ Key implementation detail: Step 7 must use the owner/repo extracted from the URL 1. **A summary comment can never collapse.** GitHub marks an inline review thread **Outdated** and folds it away as soon as the author edits the line it is anchored to. So an addressed inline finding removes itself from the page. An issue comment has no such lifecycle — it sits in the PR conversation permanently, one extra comment whether or not its rows still apply. PATCHing it to "all suggestions addressed" replaces the content but not the comment. The very mechanism intended to prevent clutter _was_ the clutter. 2. **A Markdown table cannot carry a one-click fix.** GitHub renders a ` ```suggestion ` fence as an applicable change only inside a review comment on a diff line; in an issue comment it degrades to a plain code block. Suggestion-level findings — mechanical, localized cleanups — are precisely the class that benefits most from one-click apply, so the split withheld the feature from the findings that most needed it. The table's cramped "Suggested fix" column also degraded badly as the suggestion count grew. -The convergence concern that motivated the summary is real but narrower than it looked: GitHub's Outdated-collapse handles every suggestion the author actually acts on, which is the common case. What remains is a suggestion the author declines and leaves untouched — its line does not change, so the thread stays open and a later run can post a near-duplicate. That residue is bounded by the presubmit Overlap check (`blockOnExistingComments`), which blocks submission when a new finding lands on the same `(path, line)` as a live Qwen comment on the same commit. +The convergence concern that motivated the summary is real but narrower than it looked: GitHub's Outdated-collapse handles every suggestion the author actually acts on, which is the common case. What remains is a suggestion the author declines and leaves untouched — its line does not change, so the thread stays open and a later run can post a near-duplicate. That residue is bounded by the presubmit Overlap check (`blockOnExistingComments`), which blocks submission when a new finding lands on the same `(path, line)` as a live Qwen comment on the same commit — with one deliberate exception (#9208): a carried-forward ledger finding that re-posts its own original thread carries the original's ledger id and is bucketed `repost` (exempted) instead of blocked; otherwise the carried re-post of a declined suggestion would itself be dropped as a location overlap and the finding would never reach the page. **Trade-off:** diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index a3194407d1f..d366b3c0e2c 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -982,10 +982,10 @@ Use the **HEAD commit SHA** captured in Step 1. If not captured, fall back to `" **Run pre-submission checks**: the bundled `qwen review presubmit` subcommand performs self-PR detection, CI / build status classification, and existing-Qwen-comment classification in one pass — three deterministic gh-API queries collapsed into a single JSON report. Read the report to drive the rest of Step 7. -Optionally write the `(path, line)` anchors of the comments you're about to post — every Critical and Suggestion finding headed for the `comments` array — so existing-comment Overlap can be detected: +Optionally write the `(path, line)` anchors of the comments you're about to post — every Critical and Suggestion finding headed for the `comments` array — so existing-comment Overlap can be detected. An entry for a **carried-forward** finding keeps the finding's ledger `id` (its `R-`); an entry for a **fresh** finding of THIS round omits `id` — a fresh id cannot appear in any comment posted before this round, and carrying one would let the new claim ride the re-post exemption into an unrelated thread, or crowd out a genuine re-post's single-id precondition. The carried `id` is what lets a Step 6 re-post be recognized and exempted from the overlap drop: ```bash -echo '[{"path":"src/foo.ts","line":42}, ...]' > .qwen/tmp/qwen-review-{target}-findings.json +echo '[{"path":"src/foo.ts","line":42,"id":"R3-2"}, ...]' > .qwen/tmp/qwen-review-{target}-findings.json ``` Then run: @@ -1010,8 +1010,22 @@ Read `.qwen/tmp/qwen-review-{target}-presubmit.json`. Schema: }; existingComments: { total: number; - byBucket: { stale, resolved, overlap, noConflict: number }; - overlap: Comment[]; // BLOCK on submit if non-empty + byBucket: { stale, resolved, overlap, repost, noConflict: number }; + // repost entries are a SUBSET of overlap and + // are counted in both: every re-post target + // is also an overlap + // Comment = { id, path, line, commit_id, + // body — an 80-char excerpt, + // user? — the author login when known } + overlap: Comment[]; // BLOCK on submit — except a finding whose + // id matches a repost entry at the same + // location (see repost below) + repost: (Comment & { matchedIds: string[] })[]; + // overlap comments matched as re-post + // targets — by a carried-id prefix in the + // claim line, or (when unambiguous) a truly + // id-less own-account original — exempt + // those findings from the drop (see below) stale: Comment[]; // log "Skipped N stale ..." resolved: Comment[]; // log "Skipped N replied-to ..." noConflict: Comment[]; // log "Found N prior with no overlap ..." @@ -1020,6 +1034,7 @@ Read `.qwen/tmp/qwen-review-{target}-presubmit.json`. Schema: downgradeRequestChanges: boolean; // submit COMMENT instead of REQUEST_CHANGES (self-PR only) downgradeReasons: string[]; // human-readable; join with '; ' for body blockOnExistingComments: boolean; // one or more overlaps — drop those findings + // (except carried-id re-posts, see below) findingsFileInvalid: boolean; // the --new-findings file was unreadable: // overlap dedup ran on an empty set (dupes // possible) and anchor-risk defaulted to @@ -1043,9 +1058,9 @@ Read `.qwen/tmp/qwen-review-{target}-presubmit.json`. Schema: **Apply the report:** -- `blockOnExistingComments=true` → **an overlap is a duplicate; the disposal is deterministic — do not ask the user.** Drop each finding whose `(path, line)` appears in `existingComments.overlap` from your `comments` array — the inline counts follow automatically, because `submit` counts the comments you actually attach, so a dropped Critical is simply no longer there to count (and a dropped Critical that was already on the PR does not belong in `state.bodyCriticals` either). List the dropped findings in the terminal summary as "already reported at :", and submit the remainder without pausing. This decision point has been improvised as an interactive question, which stalls a headless run forever (measured; DESIGN.md — The interactive overlap question); the Exclusion Criteria already forbid re-reporting discussed issues, so there is nothing to ask. (If dropping overlaps leaves zero findings, that is still not a question: submit with an empty `comments` array like any other run — `submit` composes the body from `state`, and a run with nothing to add posts whatever that computes. A recap like "all already reported, N resolved by ``, two still standing" goes in the **terminal summary**, not the PR: `compose-review` has no free-text body field to carry it (see Step 7 — you do not author PR-facing prose), and it is never a `gh pr comment` — a hand-posted issue comment bypasses the authorisation gate, the downgrade semantics, and the `posted` contract all at once.) +- `blockOnExistingComments=true` → **an overlap is a duplicate; the disposal is deterministic — do not ask the user.** Drop each finding whose `(path, line)` appears in `existingComments.overlap` from your `comments` array — **except a finding whose `id` appears in `matchedIds` of an `existingComments.repost` entry at the same location**: that is a Step 6 ledger re-post, and re-posting under the original id is exactly how the id survives into the next round's marker — GitHub stacks it in the original thread, which is where it belongs. The inline counts follow automatically, because `submit` counts the comments you actually attach, so a dropped Critical is simply no longer there to count (and a dropped Critical that was already on the PR does not belong in `state.bodyCriticals` either). List each dropped finding in the terminal summary as "already reported at : — comment (by ): ", taking ``, `` (omit the `(by )` slot when the entry carries no `user`), and the 80-char `` from the overlapping comment (`existingComments.overlap` entries carry all three), and submit the remainder without pausing. Naming the author is what makes an authorship-refused re-post exemption self-explanatory: the drop line then shows a DIFFERENT author next to the matching id. Name the comment on EVERY drop — that is what makes a same-line false positive visible to the operator instead of a bare location. This decision point has been improvised as an interactive question, which stalls a headless run forever (measured; DESIGN.md — The interactive overlap question); the Exclusion Criteria already forbid re-reporting discussed issues, so there is nothing to ask. (If dropping overlaps leaves zero findings, that is still not a question: submit with an empty `comments` array like any other run — `submit` composes the body from `state`, and a run with nothing to add posts whatever that computes. A recap like "all already reported, N resolved by ``, two still standing" goes in the **terminal summary**, not the PR: `compose-review` has no free-text body field to carry it (see Step 7 — you do not author PR-facing prose), and it is never a `gh pr comment` — a hand-posted issue comment bypasses the authorisation gate, the downgrade semantics, and the `posted` contract all at once.) - `downgradeApprove` / `downgradeRequestChanges` / `downgradeReasons` → **do not apply these by hand.** Copy them into the `presubmit` field of the `compose-review` input (below); the subcommand owns the semantics its tests pin — a downgrade fires only when the verdict it names is the one on the table (a Suggestion-only review is already Comment, so nothing is downgraded and no "Downgraded" sentence is emitted), the downgrade sentence carries the reasons, and a downgraded Request changes keeps its body Criticals after the sentence so the self-PR downgrade never erases the only copy of a blocker. -- `headDrift.drifted=true` → **commits nobody reviewed are on the PR; the verdict can no longer certify the pull request as it stands.** The Approve cap has already fired through the downgrade machinery (the reason names both SHAs — it rides into the body with the other reasons; never hand-apply). What happens to the _submission_ is decided by **`headDrift.anchorsAtRisk`, which presubmit computes — do not re-derive it by hand**: pass `--new-findings` so it has your anchors, and it rules fail-safe on every hole a hand intersection falls into (a truncated `filesTouched` list (measured; DESIGN.md — The 283-file drift cap), the compare API's own 300-file ceiling, a `diverged` force-push, an unavailable compare, or a missing findings list). **`--new-findings` must carry EVERY finding's file, not only the inline-anchored ones** — a body-only Critical (one that could not be mapped to a diff line) still names a file, and if that file is omitted a drift touching it reads as `anchorsAtRisk=false`; include one `{path, line}` per body Critical (any placeholder `line`, e.g. `1` — presubmit intersects on `path` only). **`anchorsAtRisk=true`**: the anchors themselves are at risk and the findings may already be fixed — apply the 422-recovery rule _proactively_: abandon this submission, say so, and restart at the new SHA from Step 1's `fetch-pr`. **`anchorsAtRisk=false`**: submit as planned — the review is of `fetchedSha` (`submit` posts that very SHA as `commit_id`), the body's downgrade sentence says so, and if GitHub still answers 422 the recovery path below takes over. Name the drift in the terminal summary either way. +- `headDrift.drifted=true` → **commits nobody reviewed are on the PR; the verdict can no longer certify the pull request as it stands.** The Approve cap has already fired through the downgrade machinery (the reason names both SHAs — it rides into the body with the other reasons; never hand-apply). What happens to the _submission_ is decided by **`headDrift.anchorsAtRisk`, which presubmit computes — do not re-derive it by hand**: pass `--new-findings` so it has your anchors, and it rules fail-safe on every hole a hand intersection falls into (a truncated `filesTouched` list (measured; DESIGN.md — The 283-file drift cap), the compare API's own 300-file ceiling, a `diverged` force-push, an unavailable compare, or a missing findings list). **`--new-findings` must carry EVERY finding's file, not only the inline-anchored ones** — a body-only Critical (one that could not be mapped to a diff line) still names a file, and if that file is omitted a drift touching it reads as `anchorsAtRisk=false`; include one `{path, line}` per body Critical (any placeholder `line`, e.g. `1`, and NO `id` — the drift intersection keys on `path` only, but the carried-id re-post exemption intersects on `(path, line)` plus id, so a placeholder line carrying an id could alias an inline finding's location and corrupt its exemption; a body-only Critical is never posted inline and can never be a re-post target). **`anchorsAtRisk=true`**: the anchors themselves are at risk and the findings may already be fixed — apply the 422-recovery rule _proactively_: abandon this submission, say so, and restart at the new SHA from Step 1's `fetch-pr`. **`anchorsAtRisk=false`**: submit as planned — the review is of `fetchedSha` (`submit` posts that very SHA as `commit_id`), the body's downgrade sentence says so, and if GitHub still answers 422 the recovery path below takes over. Name the drift in the terminal summary either way. > **The restart bound is per-review and covers BOTH restart paths — this proactive drift restart AND the reactive 422 recovery below.** Track it as one fact: a review restarts **at most once** for head movement, whichever path triggers it. If a run that already restarted once reaches a drift restart _or_ a 422 again, do NOT restart a second time — submit at that run's reviewed SHA with the drift named (the Approve cap holds either way). A live PR that keeps moving must not be able to starve the review in an unbounded restart loop; one clean re-read is the review, a second is the PR outrunning it. @@ -1061,7 +1076,7 @@ Read `.qwen/tmp/qwen-review-{target}-presubmit.json`. Schema: - **Self-PR**: GitHub rejects both `APPROVE` and `REQUEST_CHANGES` on your own PR (HTTP 422); `COMMENT` is the only accepted event. Critical and Suggestion findings still appear as inline `comments` regardless, so substantive feedback is preserved. - **CI failure / pending**: the LLM review reads code statically and cannot see runtime test failures. Approving on red CI is misleading; pending CI means the verdict is premature. -- **Overlap with existing comments**: posting on the same `(path, line)` as an existing Qwen comment produces visual duplicates, so overlapping findings are dropped rather than re-posted. Stale-commit and replied-to comments are skipped silently — they're false-positive overlap from line-based matching. +- **Overlap with existing comments**: posting on the same `(path, line)` as an existing Qwen comment produces visual duplicates, so overlapping findings are dropped rather than re-posted — with one exception by construction: a carried-id re-post belongs in the original thread (GitHub stacks same-line comments there), so a finding whose ledger id matches the existing comment at its location is exempted via `existingComments.repost`, and every drop names the overlapping comment so a same-line false positive stays visible. The match reads the id as the claim-line PREFIX (mirroring how the ledger marker reads it back), and a truly id-less OWN-account original is still matched when the target is unambiguous — exactly one own-account comment at the location and exactly one carried finding there (round-1 originals carry no id token; without this fallback their re-post would read as a plain overlap and be dropped). **Known limitation — the residue is the AMBIGUOUS case only**: an id-less original at a location with several own-account comments, or several carried ids at the location, or an id-less original whose body still mentions ANY ledger-id-shaped token (even a cross-reference — any token marks the comment as belonging to a specific finding's thread, so the fallback stays off), cannot be matched as a re-post target; the re-post of such a finding reads as a plain location overlap and is dropped — visibly, the drop log names the comment. A same-SHA re-run after an already-posted re-post can match that earlier re-post as the target and post a second copy (the two are structurally indistinguishable); the lineage self-heals next round through the new comment's prefix. A replied-to original still counts toward the ambiguity decision but is itself bucketed `resolved`, never a target. Stale-commit and replied-to comments are skipped silently — they're false-positive overlap from line-based matching. ⚠️ **Severity routing — high-confidence Critical AND Suggestion findings both go inline, pinned to the exact code line.** They are distinguished by the `**[Critical]**` / `**[Suggestion]**` prefix in the comment body, not by where they are posted.