diff --git a/docs/review-policy.md b/docs/review-policy.md index e6865d0f3..182d17bc5 100644 --- a/docs/review-policy.md +++ b/docs/review-policy.md @@ -411,12 +411,15 @@ ones, and the arm lost blockers. `bin/gstack-review-dispatch` closes both ends: prompt and runs `sent` on what it passed: a hash that differs from the rendered one is `BRIEF_MISMATCH`, and that reader's review does not count. 3. `collect` hashes each reader's raw output and appends its findings to - `merged.jsonl` with the label exactly as written (a JSON line's `severity`, or a - prose line's Critical, High, Medium, Low, P0-P3, FIXABLE or INVESTIGATE wherever it - stands, headings and table rows included; a top-level list item under a label-only - heading takes that label). A prose voice's `Blocks: yes` collects as CRITICAL: on - its own line, or on the finding just before it; with no finding there it is one - (normalizer v2, GSTA-167). A phantom finding costs a fold; a lost one costs the gate. + `merged.jsonl` with the label as written (a JSON line's `severity`, or the most + severe of a prose line's Critical, High, Medium, Low, P0-P3, FIXABLE or INVESTIGATE + wherever they stand, headings and table rows included; a top-level list item under a + label-only heading takes the stronger of the two). A `Severity:` or `Blocks:` line + 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 label, + `Blocks: yes` makes it CRITICAL and `Blocks: no` never lowers it; an inline + `Blocks: yes` is CRITICAL too, and with no finding above it is one. The native + adversarial voice's FIXABLE collects as CRITICAL (normalizer v2, GSTA-167). A phantom finding costs a fold; a lost one costs the gate. 4. Lead judgment writes `judged.jsonl`; `judge` refuses a set that drops a merged finding, lowers its severity, folds it into a less severe duplicate, or adds or promotes one without a reason (`JUDGMENT_REJECTED`). A merged file missing any id diff --git a/lib/review-collect.ts b/lib/review-collect.ts index 52dca7dca..1cfc02384 100644 --- a/lib/review-collect.ts +++ b/lib/review-collect.ts @@ -114,7 +114,7 @@ export function scanConsumers(root: string, paths: readonly string[], revs: read const runs = [...revs.map(rev => ({ at: rev, args: [rev, '--'] })), ...(live ? [{ at: '', args: ['--untracked', '--'] }] : [])]; const next = new Set(); for (const run of runs) { - const r = git(root, ['grep', '-I', '-F', '-o', '--null', '--no-line-number', '--no-column', ...patterns, ...run.args]); + const r = git(root, ['grep', '-I', '-F', '-o', '--null', '--no-line-number', '--no-column', '--no-color', ...patterns, ...run.args]); if (r.status !== 0 && r.status !== 1) { errors.push({ code: 'consumer_scan_failed', detail: oneLine(r.err || `git grep exited ${r.status}`), input: `git grep -F ${run.at || '--untracked'}`, repair: 'run the git grep by hand and fix what git reports' }); return 'all'; } for (const line of r.out.split('\n')) { const parts = line.split('\0'); diff --git a/lib/review-ledger.ts b/lib/review-ledger.ts index b73ba4078..3fcf9e91f 100644 --- a/lib/review-ledger.ts +++ b/lib/review-ledger.ts @@ -8,8 +8,9 @@ * judgment each lead-judgment change: an added or promoted finding, with its reason * judge the verdict on the parent's judged set (ok, or the dropped and downgraded ids) * - * Findings reach merged.jsonl only through collect, with each voice's label exactly as written, - * except that a prose voice's `Blocks: yes` collects as CRITICAL (GSTA-167 normalizer v2). + * Findings reach merged.jsonl only through collect, with each voice's label as written (a prose line's most severe one, + * raised by the Severity line below it), except that a prose voice's `Blocks: yes` and the native voice's FIXABLE collect + * as CRITICAL (GSTA-167 normalizer v2). * The parent's judged set may add or promote a finding and may fold a duplicate into one at least * as severe; it never drops or downgrades one. verify replays every check from the ledger alone. */ @@ -60,53 +61,70 @@ const ALL_LABELS = new RegExp(ANY_LABEL.source, 'g'); /** The most severe label on a line, so an explicit `Severity: High` outranks a stray `Low-level` or `[P2]` before it. */ const strongest = (t: string) => [...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)\b/i; +const META = /^(?:[-*+]\s+)?\**\s*(Severity|Blocks)\**\s*:/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). A `Severity:` or `Blocks:` line continues the finding above it in the same list item: `Severity` raises its - * label, `Blocks: yes` makes it CRITICAL (normalizer v2), `Blocks: no` never lowers it. With no finding above, such a line is its - * own finding carrying the item's first line. + * 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 + * 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. */ export function parseFindings(reader: string, output: string): Array<{ label: string; raw: string }> { - const out: Array<{ label: string; raw: string }> = []; + type Finding = { label: string; raw: string; fields: Set }; + const out: Finding[] = []; let section: string | null = null; - let current: { label: string; raw: string } | null = null; + let current: Finding | null = null; let head = ''; + 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 (/^NO FINDINGS\.?$/.test(t) || /^Recommendation:/i.test(t)) { current = null; continue; } const indented = line !== line.trimStart(); - const label = strongest(t); + const label = named(strongest(t)); const blocks = BLOCKS_YES.test(t); - if (META.test(t)) { - if (current) { + const listItem = !indented && LIST_ITEM.test(t); + const meta = listItem ? null : META.exec(t); + if (meta) { + const field = meta[1]!.toLowerCase(); + if (current && !current.fields.has(field)) { + current.fields.add(field); if (blocks) current.label = 'CRITICAL'; else if (label) current.label = stronger(current.label, label); current.raw += ` | ${t}`; - } else if (blocks || label) out.push(current = { label: blocks ? 'CRITICAL' : label!, raw: head ? `${head} | ${t}` : t }); + } else if (blocks || label) current = push(blocks ? 'CRITICAL' : label!, head ? `${head} | ${t}` : t, [field]); + else current = null; continue; } const heading = LABEL_HEADING.exec(t); - if (heading) { section = 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 { const o = JSON.parse(t) as Record; - if (typeof o.severity === 'string') { out.push(current = { label: o.severity, raw: t }); continue; } + if (typeof o.severity === 'string') { current = push(o.severity, t); continue; } } catch { /* prose that starts with a brace */ } } - const listItem = !indented && LIST_ITEM.test(t); const inherited = section && listItem ? section : undefined; const found = label && inherited ? stronger(inherited, label) : label ?? inherited; - if (found) { out.push(current = { label: blocks ? 'CRITICAL' : found, raw: t }); if (!indented) head = t; continue; } - if (listItem) { current = null; head = t; } + 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; + continue; + } + if (listItem || (!indented && t.startsWith('**'))) { current = null; head = t; } + else if (!indented) head = t; } - return out; + return out.map(({ label, raw }) => ({ label, raw })); } /** collect: hash the raw output, append its findings to merged.jsonl and record both. */ diff --git a/test/prose-voice-floor.test.ts b/test/prose-voice-floor.test.ts index 5d9ba24a1..86e2fae63 100644 --- a/test/prose-voice-floor.test.ts +++ b/test/prose-voice-floor.test.ts @@ -50,9 +50,11 @@ describe('normalizer v2 as collected: Blocks: yes is CRITICAL, Blocks: no never // Value: protects=the rule round 5's G1 was scored under, on the path production runs (gstack-review-dispatch collect); // fails_when=Blocks: yes collects below CRITICAL or Blocks: no lowers a label; seam=parseFindings test.each([ - ['FIXABLE: race in a.ts:3\nSeverity: Medium\nBlocks: no', 'Medium'], + ['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'], + ] 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'], ['1. Cache evicts sessions (c.ts:4)\n Severity: High', 'High'], ['1. Retry storm (r.ts:2)\n Severity: Medium\n Blocks: yes', 'CRITICAL'], diff --git a/test/review-collect.test.ts b/test/review-collect.test.ts index 27cd773ee..1e3e180cc 100644 --- a/test/review-collect.test.ts +++ b/test/review-collect.test.ts @@ -154,10 +154,10 @@ describe('GSTA-167 part 4a: which data files code names (scanConsumers)', () => // Value: protects=the scan reads git grep the same way under any user grep config, and a failed grep keeps every data file // code; fails_when=grep.lineNumber/grep.column shift the NUL fields so no consumer matches, or a grep failure drops data to // docs silently; seam=git grep output and exit status - test('grep.lineNumber and grep.column in git config do not hide a consumer', () => { + test('grep.lineNumber, grep.column and color.grep in git config do not change which files count as consumers', () => { const r = repo({ 'src/load.ts': "export const rows = () => require('./data/rows.json');\n" }); - r.git('config', 'grep.lineNumber', 'true'); r.git('config', 'grep.column', 'true'); - const head = r.commit({ 'data/rows.json': rows }); + r.git('config', 'grep.lineNumber', 'true'); r.git('config', 'grep.column', 'true'); r.git('config', 'color.grep', 'always'); + const head = r.commit({ 'data/rows.json': rows, 'docs/notes.md': 'see rows.json and orphan.json\n', 'data/orphan.json': rows }); expect([collectCommit(r.dir, r.baseSha, head).consumed, collectLive(r.dir, 'main').consumed]).toEqual([['data/rows.json'], ['data/rows.json']]); }); test('a git grep failure keeps every data file code and records consumer_scan_failed', () => { diff --git a/test/review-dispatch.test.ts b/test/review-dispatch.test.ts index 720986ff7..145e8cd34 100644 --- a/test/review-dispatch.test.ts +++ b/test/review-dispatch.test.ts @@ -246,7 +246,6 @@ describe('gstack-review-dispatch sent, collect, judge and verify: nothing change ].join('\n'); const got = parseFindings('outside-voice', out); expect(got.map(g => g.label)).toEqual(['CRITICAL', 'CRITICAL', 'High', 'CRITICAL']); - expect(parseFindings('outside-voice', '1. Low-level retry loop in queue.ts:78 drops writes. Severity: Critical. Blocks: no\n- [P2] cache.ts:40 evicts live sessions. Severity: High. Blocks: no').map(g => g.label)).toEqual(['Critical', 'High']); expect(got[1]!.raw).toBe('2. INVESTIGATE: lock order in pool.ts:12 | Blocks: yes'); expect(got[2]!.raw).toBe('3. FIXABLE: stale cache key (cache.ts:4) | Severity: High | Blocks: no'); expect(got[3]!.raw).toBe('4. Token never expires (auth.ts:9) | **Blocks:** yes'); @@ -267,6 +266,42 @@ describe('gstack-review-dispatch sent, collect, judge and verify: nothing change expect(prose.map(p => [p.label, p.raw])).toEqual([['CRITICAL', '1. FIXABLE: race in a.ts:3 | Severity: Low | Blocks: yes'], ['Medium', '2. Unlabeled item about b.ts:9 | Severity: Medium']]); }); + // Value: protects=a prose blocker whose only marker is an inline Blocks: yes still reaches merged.jsonl as CRITICAL (#3660 pass 2 + // Testing finding); fails_when=an unlabeled line carrying Blocks: yes collects nothing; seam=parseFindings + test.each([ + ['1. Token never expires (auth.ts:9). Blocks: yes', '1. Token never expires (auth.ts:9). Blocks: yes'], + ['Token never expires in auth.ts:9 — Blocks: yes', 'Token never expires in auth.ts:9 — Blocks: yes'], + ['1. Token never expires (auth.ts:9)\nFix: check exp **Blocks:** yes', '1. Token never expires (auth.ts:9) | Fix: check exp **Blocks:** yes'], + ])('an inline Blocks: yes with no label is one CRITICAL finding: %j', (output, raw) => { + expect(parseFindings('outside-voice', output)).toEqual([{ label: 'CRITICAL', raw }]); + }); + + // Value: protects=a top-level `- Severity:` bullet is its own finding and keeps its section's floor (#3660 pass 2 outside voice); + // fails_when=two such bullets merge into one id or a Critical heading's item collects as Medium; seam=parseFindings + test('top-level Severity bullets stay separate findings and keep the section label', () => { + const got = parseFindings('outside-voice', ['## Critical findings', '- Severity: Medium — missing owner check at auth.ts:14', '- Severity: Low — token logged at log.ts:3'].join('\n')); + expect(got.map(g => g.label)).toEqual(['Critical', 'Critical']); + }); + // Value: protects=one finding per merged id however a voice lays findings out (#3660 pass 2 native adversarial); fails_when= + // paragraph findings or a sentence starting with "Blocks" fold into the finding above, or a numbered or quoted Blocks line + // collects nothing; seam=parseFindings + test('paragraph findings stay separate, Blocks needs a colon to continue a finding, and a bare Blocks line is its own finding', () => { + const bold = parseFindings('outside-voice', ['**Finding 1.** a.ts:3 drops rows', 'Severity: Low', 'Blocks: no', '**Finding 2.** b.ts:4 leaks', 'Severity: High', 'Blocks: yes'].join('\n')); + expect(bold.map(b => [b.label, b.raw])).toEqual([['Low', '**Finding 1.** a.ts:3 drops rows | Severity: Low | Blocks: no'], ['CRITICAL', '**Finding 2.** b.ts:4 leaks | Severity: High | Blocks: yes']]); + const plain = parseFindings('outside-voice', ['a.ts:3 drops rows', 'Severity: High', 'Blocks: yes', 'b.ts:4 leaks', 'Severity: High', 'Blocks: yes'].join('\n')); + expect(plain.map(p => p.raw)).toEqual(['a.ts:3 drops rows | Severity: High | Blocks: yes', 'b.ts:4 leaks | Severity: High | Blocks: yes']); + expect(parseFindings('outside-voice', ['1. Medium: cache key drift (c.ts:4)', ' Blocks the event loop on every miss - High'].join('\n')).map(f => f.label)).toEqual(['Medium', 'High']); + 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']); + }); + 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']); + }); + test('a non-prefix Blocks: yes under a labeled finding raises it to CRITICAL', () => { + expect(parseFindings('outside-voice', '1. Retry loses writes (queue.ts:78). Severity: Medium\n Fix: reject the batch. Blocks: yes')).toEqual([{ label: 'CRITICAL', raw: '1. Retry loses writes (queue.ts:78). Severity: Medium | Fix: reject the batch. Blocks: yes' }]); + }); + test('verify on a ledger that kept no prompts says so and exits 3', () => { const d = tmp(); const ledger = path.join(d, 'ledger.jsonl');