From a72d862af45108dbb7ad59571398bf69b7c4b105 Mon Sep 17 00:00:00 2001 From: garrytan Date: Sat, 31 Oct 2026 02:31:30 +0000 Subject: [PATCH] fix(review): a later finding's fields never land on the one before it; explicit Severity wins its line /ship pass 2 Red Team (PR #3660): prose after a finding's Severity/Blocks field, a post-blank paragraph or a line in an indented block starts the next finding, and a line's explicit Severity: label decides it over other label words on the line. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- lib/review-ledger.ts | 19 ++++++++++++------- test/review-dispatch.test.ts | 11 +++++++++++ 2 files changed, 23 insertions(+), 7 deletions(-) diff --git a/lib/review-ledger.ts b/lib/review-ledger.ts index 3fcf9e91f..4e494708f 100644 --- a/lib/review-ledger.ts +++ b/lib/review-ledger.ts @@ -58,8 +58,9 @@ 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 = /(? [...t.matchAll(ALL_LABELS)].map(m => m[1]!).reduce((a, b) => (a === undefined ? b : stronger(a, b)), undefined); +const SEVERITY_FIELD = new RegExp(`[Ss]everity\\**\\s*:\\**\\s*(${LABELS})(?![A-Za-z0-9])`); +/** A line's explicit `Severity:` label, else the most severe label on it, so a stray `Low-level` or `[P2]` never decides it. */ +const strongest = (t: string) => 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; @@ -68,8 +69,9 @@ export interface MergedLine { id: string; reader: string; label: string; raw: st /** * 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). 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 paragraph opener or a second line for the same field starts another: `Severity` raises its + * 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. @@ -80,12 +82,15 @@ export function parseFindings(reader: string, output: string): Array<{ label: st let section: string | null = null; let current: Finding | null = null; let head = ''; + let gap = false; const role = reader.split('/')[0]; const named = (l: string | undefined) => (role === 'adversarial-native' && l?.toUpperCase() === 'FIXABLE' ? 'CRITICAL' : l); 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(); - if (!t) continue; + if (!t) { gap = true; continue; } + const afterGap = gap; + gap = false; if (/^NO FINDINGS\.?$/.test(t) || /^Recommendation:/i.test(t)) { current = null; continue; } const indented = line !== line.trimStart(); const label = named(strongest(t)); @@ -121,8 +126,8 @@ export function parseFindings(reader: string, output: string): Array<{ label: st if (listItem) head = t; continue; } - if (listItem || (!indented && t.startsWith('**'))) { current = null; head = t; } - else if (!indented) head = t; + if (listItem || (!indented && (t.startsWith('**') || afterGap)) || current?.fields.size) { current = null; head = t; } + else if (!indented || (!current && !LIST_ITEM.test(head))) head = t; } return out.map(({ label, raw }) => ({ label, raw })); } diff --git a/test/review-dispatch.test.ts b/test/review-dispatch.test.ts index 145e8cd34..8973955e2 100644 --- a/test/review-dispatch.test.ts +++ b/test/review-dispatch.test.ts @@ -294,6 +294,17 @@ describe('gstack-review-dispatch sent, collect, judge and verify: nothing change expect(parseFindings('outside-voice', '2. Blocks: yes').map(f => f.label)).toEqual(['CRITICAL']); expect(parseFindings('outside-voice', '> Blocks: yes').map(f => f.label)).toEqual(['CRITICAL']); }); + // Value: protects=a later finding's Severity/Blocks line never lands on the finding before it, and an explicit Severity label + // decides its line (#3660 pass 2 Red Team); fails_when=the later blocker's text is lost and the earlier finding takes its + // CRITICAL, or 'Severity: Low (not High)' collects as High; seam=parseFindings + test('prose after a finding\'s field, a blank-line paragraph or an indented block starts the next finding; an explicit Severity label wins', () => { + const raws = (o: string) => parseFindings('outside-voice', o).map(f => [f.label, f.raw]); + const two = [['Low', 'A in a.ts:1. | Severity: Low'], ['CRITICAL', 'B in b.ts:2. | Blocks: yes']]; + 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']]); + }); 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']);