From 7a6de9c5d7dacb2e8bf6c7dc69ed02ce87b6c658 Mon Sep 17 00:00:00 2001 From: garrytan Date: Sat, 31 Oct 2026 02:38:07 +0000 Subject: [PATCH] fix(review): FIXABLE outranks a Severity label on its line; a blocker keeps its own id and subject /ship fix-check (PR #3660): labels are named before the strongest is taken, so a one-line native FIXABLE ... Severity: Medium collects as CRITICAL; a later finding starts at a file:line after the open finding's fields, field bullets stay on their finding, a field line may omit the colon, and continuation prose never replaces a list item's subject. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- lib/review-ledger.ts | 40 +++++++++++++++++----------------- test/prose-voice-floor.test.ts | 3 +++ test/review-dispatch.test.ts | 13 ++++++++++- 3 files changed, 35 insertions(+), 21 deletions(-) diff --git a/lib/review-ledger.ts b/lib/review-ledger.ts index 4e494708f..e8a5633a6 100644 --- a/lib/review-ledger.ts +++ b/lib/review-ledger.ts @@ -58,23 +58,21 @@ const stronger = (a: string, b: string) => ((rank(a) ?? 0) >= (rank(b) ?? 0) ? a /** A prose voice's `Blocks: yes` (GSTA-167 normalizer v2): the finding is CRITICAL whatever its label; `Blocks: no` never lowers one. */ const BLOCKS_YES = /(? SEVERITY_FIELD.exec(t)?.[1] ?? [...t.matchAll(ALL_LABELS)].map(m => m[1]!).reduce((a, b) => (a === undefined ? b : stronger(a, b)), undefined); -/** A line that only continues the finding above it: its `Severity:` or `Blocks:` field. */ -const META = /^(?:[-*+]\s+)?\**\s*(Severity|Blocks)\**\s*:/i; +/** A file:line locus ("auth.ts:9"): prose naming one after a finding's fields starts the next finding. */ +const LOCUS = /[\w./-]+\.\w+:\d+/; +/** A line that is only a finding's `Severity` or `Blocks` field ("Severity: High", "- Blocks: yes", "Severity High"). */ +const META = new RegExp(`^(?:[-*+]\\s+)?\\**\\s*(Severity|Blocks)\\**\\s*(?::|\\s+\\**(?:${LABELS}|yes|no)(?![A-Za-z0-9]))`, 'i'); export interface MergedLine { id: string; reader: string; label: string; raw: string; output_sha256: string } /** * One reader's findings as written: a JSON object per line (severity kept as written); else each prose line carrying a severity or * FIXABLE/INVESTIGATE label, at the most severe label on it, and each top-level list item under a label-only heading (the stronger - * of the two labels; an explicit `Severity:` label wins on its line). A `Severity:` or `Blocks:` line that is not itself a top-level - * list item continues the finding above it until a list item, heading, bold or post-blank paragraph, prose after one of these fields, - * or a second line for the same field starts another: `Severity` raises its - * label, `Blocks: yes` makes it CRITICAL, `Blocks: no` never lowers it. With no finding above, such a line is its own finding - * carrying the paragraph's first line. The native voice's FIXABLE collects as CRITICAL, since a Severity line must not bury it; - * other labels stay as written for lead judgment. + * of the two labels). A `Severity` or `Blocks` field line continues the open finding, once per field: `Severity` raises its label, + * `Blocks: yes` makes it CRITICAL, `Blocks: no` never lowers it; with no finding open it is one, carrying the paragraph's subject. + * A list item, a heading, a bold or post-blank paragraph at the margin, or a line naming a file:line after the open finding's fields + * starts the next finding. An inline `Blocks: yes` is CRITICAL too. The native voice's FIXABLE collects as CRITICAL; other labels + * stay as written for lead judgment. When in doubt the parser merges rather than drops: a lost blocker costs the gate. */ export function parseFindings(reader: string, output: string): Array<{ label: string; raw: string }> { type Finding = { label: string; raw: string; fields: Set }; @@ -84,7 +82,8 @@ export function parseFindings(reader: string, output: string): Array<{ label: st let head = ''; let gap = false; const role = reader.split('/')[0]; - const named = (l: string | undefined) => (role === 'adversarial-native' && l?.toUpperCase() === 'FIXABLE' ? 'CRITICAL' : l); + const named = (l: string) => (role === 'adversarial-native' && l.toUpperCase() === 'FIXABLE' ? 'CRITICAL' : l); + const strongest = (t: string) => [...t.matchAll(ALL_LABELS)].map(m => named(m[1]!)).reduce((a, b) => (a === undefined ? b : stronger(a, b)), undefined); const push = (label: string, raw: string, fields: string[] = []): Finding => { const f = { label, raw, fields: new Set(fields) }; out.push(f); return f; }; for (const line of output.replace(/\r\n/g, '\n').split('\n')) { const t = line.trim(); @@ -93,10 +92,11 @@ export function parseFindings(reader: string, output: string): Array<{ label: st gap = false; if (/^NO FINDINGS\.?$/.test(t) || /^Recommendation:/i.test(t)) { current = null; continue; } const indented = line !== line.trimStart(); - const label = named(strongest(t)); + const label = strongest(t); const blocks = BLOCKS_YES.test(t); const listItem = !indented && LIST_ITEM.test(t); - const meta = listItem ? null : META.exec(t); + const locus = LOCUS.test(t); + const meta = listItem && locus ? null : META.exec(t); if (meta) { const field = meta[1]!.toLowerCase(); if (current && !current.fields.has(field)) { @@ -109,7 +109,7 @@ export function parseFindings(reader: string, output: string): Array<{ label: st continue; } const heading = LABEL_HEADING.exec(t); - if (heading) { section = named(heading[1])!; current = null; head = ''; continue; } + if (heading) { section = named(heading[1]!); current = null; head = ''; continue; } if (!indented && HEADING.test(t)) { section = null; current = null; } if (t.startsWith('{')) { try { @@ -117,17 +117,17 @@ export function parseFindings(reader: string, output: string): Array<{ label: st if (typeof o.severity === 'string') { current = push(o.severity, t); continue; } } catch { /* prose that starts with a brace */ } } + const fresh = listItem || (!indented && (t.startsWith('**') || afterGap)) || (!!current?.fields.size && locus); + if (fresh) { current = null; head = t; } const inherited = section && listItem ? section : undefined; const found = label && inherited ? stronger(inherited, label) : label ?? inherited; if (found) { current = push(blocks ? 'CRITICAL' : found, t); if (!indented) head = t; continue; } if (blocks && !t.startsWith('{')) { - if (current && !listItem) { current.label = 'CRITICAL'; current.raw += ` | ${t}`; continue; } - current = push('CRITICAL', !listItem && head ? `${head} | ${t}` : t); - if (listItem) head = t; + if (current) { current.label = 'CRITICAL'; current.raw += ` | ${t}`; continue; } + current = push('CRITICAL', head && head !== t ? `${head} | ${t}` : t); continue; } - if (listItem || (!indented && (t.startsWith('**') || afterGap)) || current?.fields.size) { current = null; head = t; } - else if (!indented || (!current && !LIST_ITEM.test(head))) head = t; + if (!fresh && !current && (!LIST_ITEM.test(head) || locus)) head = t; } return out.map(({ label, raw }) => ({ label, raw })); } diff --git a/test/prose-voice-floor.test.ts b/test/prose-voice-floor.test.ts index 86e2fae63..0cbe7ad27 100644 --- a/test/prose-voice-floor.test.ts +++ b/test/prose-voice-floor.test.ts @@ -53,6 +53,9 @@ describe('normalizer v2 as collected: Blocks: yes is CRITICAL, Blocks: no never ['FIXABLE: race in a.ts:3\nSeverity: Medium\nBlocks: no', 'CRITICAL'], ['INVESTIGATE: odd lock in b.ts:9\nSeverity: Low\nBlocks: yes', 'CRITICAL'], ['INVESTIGATE: odd lock in b.ts:9\nBlocks: no', 'INVESTIGATE'], + ['FIXABLE: race in a.ts:3. Severity: Medium. Blocks: no', 'CRITICAL'], + ['1. FIXABLE: race in a.ts:3 — Severity: Low, Blocks: no', 'CRITICAL'], + ['- FIXABLE: race in a.ts:3\n- Severity: Medium\n- Blocks: no', 'CRITICAL'], ] as const)('native %j -> %s', (output, want) => expect(parseFindings('adversarial-native', output).map(g => g.label)).toEqual([want])); test.each([ ['1. Drops the last row (q.ts:7). Severity: Critical. Blocks: no', 'Critical'], diff --git a/test/review-dispatch.test.ts b/test/review-dispatch.test.ts index 8973955e2..1daff109e 100644 --- a/test/review-dispatch.test.ts +++ b/test/review-dispatch.test.ts @@ -303,8 +303,19 @@ describe('gstack-review-dispatch sent, collect, judge and verify: nothing change expect(raws('A in a.ts:1.\nSeverity: Low\n\nB in b.ts:2.\nBlocks: yes')).toEqual(two); expect(raws('A in a.ts:1.\nSeverity: Low\nB in b.ts:2.\nBlocks: yes')).toEqual(two); expect(raws('Findings:\n A in a.ts:1.\n Severity: Low\n B in b.ts:2.\n Blocks: yes')).toEqual(two); - expect(raws('Retry loop in queue.ts:4 is fine. Severity: Low (not High)\nBlocks: no')).toEqual([['Low', 'Retry loop in queue.ts:4 is fine. Severity: Low (not High) | Blocks: no']]); }); + // Value: protects=the fix-check's shapes (#3660): a blocker keeps its own id and its subject, field bullets stay on their finding, + // and no Severity label buries a stronger label on its line; fails_when=any of these collects below CRITICAL, merges a later + // blocker into an earlier finding, or loses the finding's file:line; seam=parseFindings + test.each([ + ['A in a.ts:1.\nSeverity: Low\nB in b.ts:2 drops rows. Blocks: yes', [['Low', 'A in a.ts:1. | Severity: Low'], ['CRITICAL', 'B in b.ts:2 drops rows. Blocks: yes']]], + ['**Finding 1.** a.ts:3 races. Severity: Low\n**Finding 2.** b.ts:4 leaks. Blocks: yes', [['Low', '**Finding 1.** a.ts:3 races. Severity: Low'], ['CRITICAL', '**Finding 2.** b.ts:4 leaks. Blocks: yes']]], + ['FIXABLE: race in a.ts:3\nSeverity: Medium\nWhy: the writer races\nBlocks: yes', [['CRITICAL', 'FIXABLE: race in a.ts:3 | Severity: Medium | Blocks: yes']]], + ['- Unlabeled thing in a.ts:3\n- Severity: Medium\n- Blocks: yes', [['CRITICAL', '- Unlabeled thing in a.ts:3 | - Severity: Medium | - Blocks: yes']]], + ['1. Token never expires (auth.ts:9)\nFix: check exp\nBlocks: yes', [['CRITICAL', '1. Token never expires (auth.ts:9) | Blocks: yes']]], + ['1. Retry storm (r.ts:2)\nSeverity High', [['High', '1. Retry storm (r.ts:2) | Severity High']]], + ['CRITICAL: x.ts:4 drops rows (pass 1 had Severity: Low)', [['CRITICAL', 'CRITICAL: x.ts:4 drops rows (pass 1 had Severity: Low)']]], + ])('fix-check shape %j', (output, want) => expect(parseFindings('outside-voice', output).map(f => [f.label, f.raw])).toEqual(want)); test('the native voice\'s FIXABLE collects as CRITICAL under its role with or without a dispatch name', () => { for (const reader of ['adversarial-native', 'adversarial-native/adversarial']) expect(parseFindings(reader, 'FIXABLE: race in a.ts:3\nSeverity: Medium\nBlocks: no').map(f => f.label)).toEqual(['CRITICAL']); expect(parseFindings('outside-voice', 'FIXABLE: race in a.ts:3\nSeverity: Medium').map(f => f.label)).toEqual(['Medium']);