diff --git a/changelog.d/capy-keep-a-numbered-review.md b/changelog.d/capy-keep-a-numbered-review.md new file mode 100644 index 000000000..644d8d766 --- /dev/null +++ b/changelog.d/capy-keep-a-numbered-review.md @@ -0,0 +1 @@ +- **/ship's review gate no longer blocks on a Low finding just because the native reviewer wrote it as a numbered item with a nested list.** When the native reviewer wrote `1. **FIXABLE. …**`, then a sub-list, then a closing paragraph with `Severity: Low. Blocks: no.`, the collector split that paragraph into a second record. The numbered item was left with no fields, so its FIXABLE label counted as CRITICAL and stopped /ship until an owner ruled on it (6 findings collected as 9 in one alphaclaw run). The collector now keeps that first closing paragraph on its finding. The join stays narrow so it can't hide a real blocker. It applies only to the native reviewer's items, ends after the first closing paragraph, never takes in a bold-led paragraph, a verdict line, a list item, a heading or a file:line, and never lowers an item that names CRITICAL or P0. diff --git a/docs/review-policy.md b/docs/review-policy.md index 8f500d245..4cfdc181d 100644 --- a/docs/review-policy.md +++ b/docs/review-policy.md @@ -913,7 +913,11 @@ ones, and the arm lost blockers. `bin/gstack-review-dispatch` closes both ends: never split off as a second record (GST-1031); the line right under the heading that carries those fields, no other label and no file:line joins it at the stronger label (not under a heading that names CRITICAL or P0 itself), the way an unlocated paragraph after a blank line already does. An unnumbered, unlabeled bullet with no file:line under a paragraph finding whose Severity or Blocks line has not come yet is that finding's detail, so the field line after it - lands on the finding; a numbered item is its own finding. + lands on the finding; a numbered item is its own finding. A native adversarial numbered list item (`1. **FIXABLE. …**`) + with no fields of its own keeps, after an unlabeled nested list, its indented lines and the first paragraph after a blank + line until its fields come, so that paragraph's `Severity` and `Blocks` bind to the item instead of forming a second record; + an item naming CRITICAL or P0 is never lowered, and a list item, heading, lead label, file:line, bold paragraph opener, + verdict line or any later paragraph starts the next finding (ALPH-93). A reader's own no-defect closing sentence is no finding at the label it denies (GST-435): a prose line with no list, heading, bold or lead label, no `Severity` or `Blocks` field, no FIXABLE or INVESTIGATE, no file:line or path and no exception clause, with no line above it in its paragraph that runs on (no closing period) or names an diff --git a/lib/review-ledger.ts b/lib/review-ledger.ts index b0333d351..a0fcd0a54 100644 --- a/lib/review-ledger.ts +++ b/lib/review-ledger.ts @@ -179,7 +179,11 @@ export interface MergedLine { id: string; reader: string; label: string; stated? * under it, located or listed or bold, until its fields come (GST-1031), so its Severity and Blocks never split off as a second record; * the line right under it that carries those fields, no other label and no file:line joins it at the stronger label, as a paragraph after * a blank line does (a located line there is another finding), unless the heading itself names CRITICAL or P0, whose blocker another - * subject's Blocks: no must not lift. A prose line that only names a severity in passing is a summary (summaryLine). + * subject's Blocks: no must not lift. The native voice's numbered list item ("1. **FIXABLE. …**") with no fields of its own owns, after an + * unlabelled nested list, its indented lines and the first paragraph after a blank line (ALPH-93), until its fields come; their Severity + * and Blocks bind to it as a field line's do, except that an item naming CRITICAL or P0 is never lowered. A list item, heading, lead label, + * file:line, bold margin lead, verdict line or any later margin paragraph there starts the next finding. + * A prose line that only names a severity in passing is a summary (summaryLine). * A native FIXABLE finding that states `Blocks: no` and one Severity field collects at that field on every class (GST-1031 ruling A); * one with no Blocks: no, no Severity field or only a FIXABLE field stays CRITICAL. fixableRaises false (a code or docs review with no * risk, error, history or floor reason, GST-987) also drops FIXABLE's raise from label headings and leads. @@ -188,7 +192,7 @@ export interface MergedLine { id: string; reader: string; label: string; stated? * nothing; elsewhere it collects as above. A line led by a bold, unlabelled lead ("**1. Coverage:** …") is read for a closing summary after it. */ function parseLabelled(reader: string, output: string, fixableRaises: boolean): Array<{ label: string; raw: string; shaped: boolean }> { - type Finding = { label: string; raw: string; fields: Set; list: boolean; lead?: string; fixable?: boolean; heading?: boolean; section?: boolean; context?: string }; + type Finding = { label: string; raw: string; fields: Set; list: boolean; lead?: string; fixable?: boolean; heading?: boolean; item?: boolean; nested?: boolean; closed?: boolean; section?: boolean; context?: string }; const out: Finding[] = []; let section: string | null = null; let current: Finding | null = null; @@ -269,6 +273,21 @@ function parseLabelled(reader: string, output: string, fixableRaises: boolean): else if (label) current.label = stronger(current.label, label); continue; } + if (current?.item && !current.fields.size && !HEADING.test(t) && !NUMBERED_HEAD.test(t) && !t.startsWith('{')) { + if (indented && LIST_ITEM.test(t) && !label && !blocks) { current.nested = true; continue; } + if (current.nested && (indented || (afterGap && !t.startsWith('**'))) && !(afterGap && current.closed) && !LIST_ITEM.test(t) && !LEAD_LABEL.test(t) + && !LOCUS.test(t) && !quietSummary(t)) { + const pinned = [...current.raw.split(' | ')[0]!.matchAll(ALL_LABELS)].some(m => rank(m[1]) === 4); + current.raw += ` | ${t}`; + if (afterGap) current.closed = true; + if (blocks) current.label = 'CRITICAL'; + else if (field !== undefined && !critical(current.raw) && !pinned) current.label = strongestOf([current.lead, label])!; + else if (label) current.label = stronger(current.label, label); + if (field !== undefined) current.fields.add('severity'); + if (t.match(BLOCKS_FIELD)) current.fields.add('blocks'); + continue; + } + } const fresh = (listItem && !detail) || (!indented && !detail && (t.startsWith('**') || afterGap)) || (!!current?.fields.size && locus); if (fresh) { current = null; head = t; } const inherited = section && listItem ? section : undefined; @@ -279,6 +298,7 @@ function parseLabelled(reader: string, output: string, fixableRaises: boolean): current.section = !!inherited; if (continued) current.context = before; current.heading = !indented && NUMBERED_HEAD.test(t); + current.item = role === 'adversarial-native' && !indented && /^\d+[.)]\s/.test(t) && field === undefined && !t.match(BLOCKS_FIELD); if (!indented) head = t; continue; } diff --git a/test/review-native-nested-list.test.ts b/test/review-native-nested-list.test.ts new file mode 100644 index 000000000..b52276b49 --- /dev/null +++ b/test/review-native-nested-list.test.ts @@ -0,0 +1,83 @@ +import { describe, expect, test } from 'bun:test'; +import * as fs from 'node:fs'; +import * as os from 'node:os'; +import * as path from 'node:path'; +import { collect, ledgerBlockers, parseFindings, statesNoBlocks } from '../lib/review-ledger'; + +const native = (output: string, raises = true) => parseFindings('adversarial-native', output, raises); +/** A numbered finding with a bold lead, a body line, an indented sub-list, a blank line and a closing paragraph holding its fields. */ +const nested = (n: number, tag: string, close: string, indent = ' ') => [ + `${n}. **${tag}. The retry queue drops a job on shutdown** (\`queue.ts\` \`drainJobs\`). The drain loop exits before the last flush:`, + ' - The in-flight job is never written back.', + ' - The next boot sees no record of it.', + '', + `${indent}${close}`, +].join('\n'); +const LOW = 'So a job queued in the last second is lost, which is rare and already retried upstream. Severity: Low. Blocks: no.'; +const reply = (findings: string[]) => ['**Findings**', '', ...findings.flatMap(f => [f, '']), 'Recommendation: Ship after one fix round on the drain loop.'].join('\n'); +const blockers = (output: string) => { + const d = fs.mkdtempSync(path.join(os.tmpdir(), 'nested-list-')); + const [ledger, merged, out] = ['l.jsonl', 'm.jsonl', 'out.txt'].map(f => path.join(d, f)) as [string, string, string]; + fs.writeFileSync(out, output); + const c = collect(ledger, 'adversarial-native', output, merged, out); + const result = { labels: c.lines.map(l => l.label), blockers: ledgerBlockers(ledger).length }; + fs.rmSync(d, { recursive: true, force: true }); + return result; +}; + +describe('a numbered native finding keeps its closing paragraph after a nested list', () => { + // Value: protects=a native finding written as "N. **FIXABLE. ...**", a body line, an indented sub-list, a blank line, then a paragraph + // carrying "Severity: Low. Blocks: no." stays one record, so its Severity binds to it and GST-1031 ruling A collects it at Low; + // fails_when=the paragraph splits off as a second record and the heading's FIXABLE collects CRITICAL as a blocker; why_new=ALPH-93 + // (alphaclaw#387): 6 native findings collected as 9 and a Low FIXABLE blocked /ship until an owner ruling; seam=parseFindings, + // collect and ledgerBlockers; no_claim=other list-continuation spellings + test('the closing paragraph joins its finding, indented or at the margin', () => { + for (const indent of [' ', '']) + for (const raises of [true, false]) { + const found = native(reply([nested(1, 'FIXABLE', LOW, indent), nested(2, 'INVESTIGATE', 'The fix is a final flush before exit. Severity: Medium. Blocks: no.', indent)]), raises); + expect([indent, raises, found.map(f => f.label)]).toEqual([indent, raises, ['Low', 'Medium']]); + expect(found[0]!.raw.startsWith('1. **FIXABLE.')).toBe(true); + expect(found[0]!.raw.endsWith(`| ${LOW}`)).toBe(true); + expect(found.map(f => statesNoBlocks(f.raw))).toEqual([true, true]); + } + expect(blockers(reply([nested(1, 'FIXABLE', LOW)]))).toEqual({ labels: ['Low'], blockers: 0 }); + }); + + // Value: protects=the fail-closed holds around that join: a FIXABLE whose paragraph has no Severity field, or says Blocks: yes, still + // collects CRITICAL and counts as a blocker; a later numbered item, a lead-labelled paragraph or a located paragraph stays its own + // record; a heading naming CRITICAL is never lowered by the paragraph; fails_when=any of these merges, lowers or stops blocking; + // why_new=ALPH-93; seam=parseFindings, collect and ledgerBlockers; no_claim=that each control is a real finding + test('the join never weakens a blocker or swallows a separate finding', () => { + expect(native(reply([nested(1, 'FIXABLE', 'So a job queued in the last second is lost. Blocks: no.')])).map(f => f.label)).toEqual(['CRITICAL']); + expect(native(reply([nested(1, 'FIXABLE', 'So a job queued in the last second is lost.')])).map(f => f.label)).toEqual(['CRITICAL']); + for (const close of ['So jobs are lost on every deploy. Severity: Low. Blocks: yes.', 'So jobs are lost. Blocks: yes. Severity: Low.']) + expect(blockers(reply([nested(1, 'FIXABLE', close)]))).toEqual({ labels: ['CRITICAL'], blockers: 1 }); + expect(native(reply([nested(1, 'CRITICAL', LOW)]), false).map(f => f.label)).toEqual(['CRITICAL']); + expect(native(reply([nested(1, 'FIXABLE', LOW), nested(2, 'FIXABLE', 'So restarts leak a lock file. Severity: Medium. Blocks: yes.')])).map(f => f.label)) + .toEqual(['Low', 'CRITICAL']); + for (const later of ['**High:** the lock file in lock.ts leaks on restart. Blocks: no.', 'Separately, lock.ts:12 leaks the lock file on restart. Severity: High. Blocks: no.']) { + const found = native([nested(1, 'FIXABLE', LOW), '', later].join('\n')); + expect([later, found.map(f => f.label)]).toEqual([later, ['Low', 'High']]); + } + const located = native([nested(1, 'FIXABLE', 'Also lock.ts:12 leaks on restart. Severity: Low. Blocks: no.', ''), ''].join('\n')); + expect(located.map(f => f.label)).toEqual(['CRITICAL', 'Low']); + }); + + // Value: protects=the join's bounds: a bold-led margin paragraph or a second paragraph after the closing one is its own finding, so its + // Severity and Blocks: no never lower the FIXABLE item; an item naming CRITICAL is never lowered, and other readers' numbered items + // never join; fails_when=any of these collects as one Low record and the blocker leaves the ledger; why_new=the GST-2174 /ship review + // (specialist and Red Team probes); seam=parseFindings and ledgerBlockers; no_claim=that each control is a real finding + test('the join ends at the first closing paragraph and never lowers a CRITICAL item', () => { + for (const [close, later] of [['So jobs are lost.', '**The lock file leaks on restart.** A second process cannot start. Severity: Low. Blocks: no.'], + ['So a queued job is lost.', 'The lock file leaks on restart. Severity: Low. Blocks: no.']]) { + const output = [nested(1, 'FIXABLE', close!), '', later].join('\n'); + expect([later, native(output).map(f => f.label)]).toEqual([later, ['CRITICAL', 'Low']]); + expect(blockers(output).blockers).toBe(1); + } + const critical = ['1. Webhook routes skip signature validation, a CRITICAL gap.', ' - first detail', ' - second detail', '', 'The log line omits the locale. Severity: Low.'].join('\n'); + for (const reader of ['outside-voice', 'red-team', 'specialist/testing']) + expect([reader, parseFindings(reader, critical).map(f => f.label)]).toEqual([reader, ['CRITICAL', 'Low']]); + expect(native(critical).map(f => f.label)[0]).toBe('CRITICAL'); + expect(blockers(critical).blockers).toBe(1); + }); +});