Stop the review collector reading severity words in prose as findings (GST-2122) (#4218)

* Stop the review collector reading severity words in prose as findings (GST-2122)

Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>

* Keep native Blocks: no fields and numbered finding headings when the collector drops prose (GST-2122)

Review-Finding: p1/specialist-batched#1 reader=testing
Review-Finding: p1/red-team#2 reader=red-team
Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>

* Name numbered finding headings in the GST-2122 structure list

Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>

* Keep prose severity outside clean replies; read denials after bold leads (GST-2122)

Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>

---------

Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
This commit is contained in:
Garry Tanandcapy-ai[bot] authored and GitHub committed 2026-11-09 14:13:19 +00:00
1 parent 6473a17de4
commit 324135dfb1
5 files changed
+189 -7

No files matched your search

@@ -0,0 +1 @@
- **A reader's clean reply, or a sentence denying a severity, no longer stops /ship.** The review collector took a severity word anywhere in a reader's reply as a finding at that label. #4208's pass-2 red team returned `NO FINDINGS` but quoted `CRITICAL (the reviewer wrote …)` in a coverage note, and an alphaclaw fix-check wrote "No fix introduced a CRITICAL defect". Both collected as CRITICAL, judge may not drop a collected finding, and `gstack-review-plan` printed `NEXT: stop` (GST-2122). Now a reply that says `NO FINDINGS` keeps only its structured findings, which still collect at their label. Those are JSON objects (malformed ones included), lead labels, label headings, numbered headings, `Severity` fields, `Blocks: yes`, the native FIXABLE or INVESTIGATE tag, and anything naming a file:line. Its other prose collects nothing. A denial that a change introduced a severity is a no-defect clause, including after a bold lead such as `**1. Coverage:**`. Other prose keeps its label as before, so a real prose blocker such as "Neither fix clears the P0." still blocks.
+6 -1
View File
@@ -937,7 +937,12 @@ ones, and the arm lost blockers. `bin/gstack-review-dispatch` closes both ends:
can't tell a finding's own fields from another subject's. Otherwise lead judgment can fold it only into a finding
at least as severe, and it counts toward what blocks. GST-766 skipped negated labels and lost
real blockers named in an exception clause ("No CRITICAL issues except the token leak in log.ts:3"); GSTA-167
reverted that. Each prose finding's line also
reverted that. A reply that says `NO FINDINGS` keeps only its structured findings (GST-2122). Those are JSON objects
(malformed JSON included), lead labels, label headings, numbered finding headings, `Severity` fields, `Blocks: yes`,
the native voice's FIXABLE or INVESTIGATE tag, and anything naming a file:line, which still collect at their label.
Its other prose collects nothing. Outside such a reply prose collects as described above. A denial that a change
introduced a severity ("No fix introduced a CRITICAL defect") is a no-defect clause too, and a line led by a bold,
unlabelled lead ("**1. Coverage:** …", "- **Callers:** …") is read for those clauses after its lead. Each prose finding's line also
records the Severity and Blocks its own text states as `stated`, beside the collected label and never in its place;
a classified finding that carries `stated` shows it in its `## Deferred findings` line next to the label ("CRITICAL (the
reviewer wrote Severity: Low, Blocks: no)"). /ship's Step 9.4 does not copy `stated` into its findings file yet: that
+22 -6
View File
@@ -128,7 +128,7 @@ const SEV = `[*\`]*(?:CRITICAL|INFORMATIONAL|Critical|Informational|High|HIGH|Me
const SEV_LIST = `${SEV}(?:\\s*(?:,|/|\\bor\\b|\\band\\b|\\bnor\\b)\\s*${SEV})*(?![A-Za-z0-9])`;
const NOUN = '(?:issues?|findings?|defects?|bugs?|problems?|regressions?|blockers?|concerns?|items?|notes?|ones?)';
const RATES = rates(SEV_LIST, '(?:severity|level|floor|bar|threshold)');
const DENY = `(?<![A-Za-z])(?:(?:no|zero)\\s+${SEV_LIST}(?:\\s+${NOUN})?|(?:no|zero)\\s+${NOUN}\\s+(?:(?:that\\s+is|that's|that\\s+are|which\\s+is|which\\s+are|at|rated)\\s+(?:at\\s+)?${SEV_LIST}(?:\\s+(?:severity|level))?|(?:(?:that|which)\\s+)?${RATES})`
const DENY = `(?<![A-Za-z])(?:(?:no|neither|none\\s+of\\s+(?:the|these|them|those))(?:\\s+[A-Za-z]+){1,2}?\\s+(?:introduced|introduces|caused|causes|created|creates|added|adds|left|leaves)\\s+(?:a|an|any)\\s+${SEV_LIST}\\s+${NOUN}|(?:no|zero)\\s+${SEV_LIST}(?:\\s+${NOUN})?|(?:no|zero)\\s+${NOUN}\\s+(?:(?:that\\s+is|that's|that\\s+are|which\\s+is|which\\s+are|at|rated)\\s+(?:at\\s+)?${SEV_LIST}(?:\\s+(?:severity|level))?|(?:(?:that|which)\\s+)?${RATES})`
+ `|nothing\\s+(?:at\\s+)?${SEV_LIST}(?:\\s+(?:severity|level))?|there(?:'s|\\s+is|\\s+are|\\s+was|\\s+were)\\s+(?:still\\s+)?(?:no|zero|nothing(?:\\s+at)?)\\s+${SEV_LIST}(?:\\s+${NOUN})?`
+ `|(?:nothing|none\\s+of\\s+(?:them|these|those|the\\s+(?:findings|fixes|notes|issues|changes))|neither(?:\\s+(?:finding|fix|note|issue|change|one)|\\s+of\\s+(?:them|these|the\\s+(?:findings|fixes|notes))))`
+ `(?:\\s+(?:here|in\\s+(?:this|the)\\s+(?:diff|change|fix|patch|range|PR)))?\\s+(?:(?:is|are|was|were)\\s+(?:at\\s+)?${SEV_LIST}|${RATES}))`;
@@ -183,9 +183,12 @@ export interface MergedLine { id: string; reader: string; label: string; stated?
* 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.
* In a reply that says NO FINDINGS (GST-2122) a finding with no structure (JSON, lead label, label heading, numbered finding heading,
* Severity or Blocks: yes field, the native voice's FIXABLE or INVESTIGATE, or a file:line on it or on the line it runs on from) collects
* nothing; elsewhere it collects as above. A line led by a bold, unlabelled lead ("**1. Coverage:** …") is read for a closing summary after it.
*/
export function parseFindings(reader: string, output: string, fixableRaises = true): Array<{ label: string; raw: string }> {
type Finding = { label: string; raw: string; fields: Set<string>; list: boolean; lead?: string; fixable?: boolean; heading?: boolean };
function parseLabelled(reader: string, output: string, fixableRaises: boolean): Array<{ label: string; raw: string; shaped: boolean }> {
type Finding = { label: string; raw: string; fields: Set<string>; list: boolean; lead?: string; fixable?: boolean; heading?: boolean; section?: boolean; context?: string };
const out: Finding[] = [];
let section: string | null = null;
let current: Finding | null = null;
@@ -202,7 +205,8 @@ export function parseFindings(reader: string, output: string, fixableRaises = tr
const leadOf = (t: string) => { const l = LEAD_LABEL.exec(t)?.[1]; return l === undefined || !fixableRaises ? l : named(l); };
const labelOf = (t: string, continued: boolean) => {
if (role === 'adversarial-native' && quietSummary(t)) return 'FIXABLE';
const summary = continued ? undefined : closingSummary(t);
const led = /^(?:(?:[-*+]|\d+[.)])\s+)?\*\*([^*]+?)(?::\*\*|\*\*:)\s+(.+)$/.exec(t);
const summary = continued ? undefined : closingSummary(led && !ANY_LABEL.test(led[1]!) ? led[2]! : t);
if (summary) return strongestOf([...summary.labels.map(named), summary.prose ? 'INFORMATIONAL' : undefined]);
const field = fieldOf(t);
if (field === undefined) return strongest(t);
@@ -215,6 +219,7 @@ export function parseFindings(reader: string, output: string, fixableRaises = tr
if (!treeSeen && isReviewedTreeLine(t)) { treeSeen = true; continue; }
const afterGap = gap;
const continued = !!prev && !HEADING.test(prev) && (EXCEPTION.test(prev) || !/[.!?…}]["'”’)\]*_`]*$/.test(prev));
const before = prev;
prev = t;
gap = false;
if (/^NO FINDINGS\.?$/.test(t) || /^Recommendation:/i.test(t)) { current = null; continue; }
@@ -271,6 +276,8 @@ export function parseFindings(reader: string, output: string, fixableRaises = tr
if (found) {
current = push(blocks ? 'CRITICAL' : found, t, [], strongestOf([leadOf(t), inherited, fieldOf(t)]));
current.fixable = inherited?.toUpperCase() === 'FIXABLE';
current.section = !!inherited;
if (continued) current.context = before;
current.heading = !indented && NUMBERED_HEAD.test(t);
if (!indented) head = t;
continue;
@@ -283,11 +290,20 @@ export function parseFindings(reader: string, output: string, fixableRaises = tr
if (!fresh && !current && (!LIST_ITEM.test(head) || locus)) head = t;
}
const unblocked = (f: Finding) => fixableRaises || role !== 'adversarial-native' || !(f.fixable || f.label.toUpperCase() === 'FIXABLE' || /(?<![A-Za-z0-9])FIXABLE(?![A-Za-z0-9])/.test(f.raw)) || statesNoBlocks(f.raw);
const relaxed = fixableRaises && role === 'adversarial-native' ? parseFindings(reader, output, false) : undefined;
const relaxed = fixableRaises && role === 'adversarial-native' ? parseLabelled(reader, output, false) : undefined;
const statesSeverity = (raw: string) => { const ls = [...raw.matchAll(SEVERITY_FIELD)].flatMap(m => (m[1] ?? m[2]!).match(FIELD_LABELS)!); return ls.length === 1 && !!rank(ls[0]); };
const shaped = (f: Finding) => f.section || f.heading || [...f.raw.split(' | '), f.context ?? ''].some(t => t.startsWith('{') || LEAD_LABEL.test(t) || META.test(t) || !!t.match(STATED_SEVERITY) || BLOCKS_YES.test(t)
|| LOCUS.test(t) || PATH_LOCUS.test(t) || (role === 'adversarial-native' && /(?<![A-Za-z0-9])(?:FIXABLE|INVESTIGATE)(?![A-Za-z0-9])/i.test(t)));
return out.map((f, i) => {
const own = relaxed?.length === out.length && relaxed[i]!.raw === f.raw && statesNoBlocks(f.raw) && statesSeverity(f.raw) ? relaxed[i]!.label : undefined;
return { label: own ?? (unblocked(f) ? f.label : 'CRITICAL'), raw: f.raw };
return { label: own ?? (unblocked(f) ? f.label : 'CRITICAL'), raw: f.raw, shaped: shaped(f) };
});
}
export function parseFindings(reader: string, output: string, fixableRaises = true): Array<{ label: string; raw: string }> {
const clean = output.split('\n').some(l => /^NO FINDINGS\.?$/.test(l.trim()));
return parseLabelled(reader, output, fixableRaises).flatMap(({ label, raw, shaped }) => {
return shaped || !clean ? [{ label, raw }] : [];
});
}
@@ -0,0 +1,17 @@
REVIEWED_TREE: 35aa2e6c1f8a972d6541e40cd27074c054a4971a /workspace/gstack
NO FINDINGS
Scope note, outside the findings block: the diff from a832db7 changes two files. I read `lib/review-scope.ts` in full. I reviewed `test/review-scope.test.ts` in summary mode only, as the fixture rule requires (4 lines added, 4 removed). Those test edits change the deferral-rendering expectations so `severity` stays the plain collected label and the new `stated` field carries what the reviewer wrote, including a newline-injection case.
Here's what I checked across the integration boundaries:
- **Callers:** `deferredRecord` and `renderDeferredSection` are only called from `bin/gstack-review-scope` (`defer` and `render`), so nothing else depended on the old severity format.
- **PR body:** the rendered `## Deferred findings` line comes out byte-identical to before (`CRITICAL (the reviewer wrote …)`), and `stated` still goes through `oneLine`.
- **`DEFERRED:` stderr line:** it now prints the plain label instead of the label with the reviewer suffix attached. That's an improvement, and nothing parses it.
- **Older files:** a `deferred.json` written before this change still renders the same, because its severity string already had the suffix baked in.
- **Docs:** `docs/review-policy.md` already describes `stated` this way.
The one gap I found, too small to report as a finding: if one /ship run wrote `deferred.json` with the new code and rendered it with the old code, the reviewer's stated text would be dropped silently. That would need the code to change in the middle of a run, so it's unrealistic.
`bun test test/review-scope.test.ts` passes on this tree: 36 pass, 0 fail.
+143
View File
@@ -0,0 +1,143 @@
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 } from '../lib/review-ledger';
const DIR = path.resolve(import.meta.dir, 'fixtures/review-ledger/gst-2122');
const read = (f: string) => fs.readFileSync(path.join(DIR, f), 'utf8');
const labels = (reader: string, output: string, raises = true) => parseFindings(reader, output, raises).map(f => f.label);
const TREE = 'REVIEWED_TREE: 35aa2e6c1f8a972d6541e40cd27074c054a4971a /workspace/gstack';
/** alphaclaw #385/#386's fix-check sentence, verbatim; the reply around it is rebuilt in the brief's shape (the raw reply was not kept). */
const ALPHACLAW = 'No fix introduced a CRITICAL defect.';
const fixCheck = (body: string) => `${TREE}\n\n${body}\n`;
function blockersOf(reader: string, output: string): string[] {
const d = fs.mkdtempSync(path.join(os.tmpdir(), 'gst2122-'));
try {
const ledger = path.join(d, 'l.jsonl');
fs.writeFileSync(path.join(d, 'out.txt'), output);
collect(ledger, reader, output, path.join(d, 'm.jsonl'), path.join(d, 'out.txt'));
return ledgerBlockers(ledger).map(b => b.id);
} finally { fs.rmSync(d, { recursive: true, force: true }); }
}
describe('a clean reply or a denial in reader prose is never a blocker (GST-2122)', () => {
// Value: protects=a reader that returned NO FINDINGS counts no finding however its notes quote a severity: #4208's pass-2 red-team
// reply quoted "CRITICAL (the reviewer wrote …)" in a coverage bullet and collected as p2/red-team#1 CRITICAL, which judge may not
// drop, so gstack-review-plan printed NEXT: stop; fails_when=that reply collects any finding or the ledger counts a blocker;
// why_new=#4208 /ship stopped on a phantom; seam=parseFindings, collect and ledgerBlockers; no_claim=other readers' replies
test("#4208's NO FINDINGS red-team reply collects nothing", () => {
const out = read('gsta-435-pr4208-p2_red-team.txt');
for (const raises of [true, false]) expect([raises, labels('red-team', out, raises)]).toEqual([raises, []]);
expect(blockersOf('red-team', out)).toEqual([]);
});
// Value: protects=a sentence that is wholly a denial that a change introduced a severity collects at no label: alphaclaw #385/#386's
// "No fix introduced a CRITICAL defect" collected CRITICAL and stopped /ship; alone on its line it collects nothing, beside an
// unlabelled sentence it stays on record at INFORMATIONAL (GST-435), and under NO FINDINGS it collects nothing; fails_when=any of
// these collects above INFORMATIONAL or counts a blocker; why_new=two alphaclaw PRs stopped on it; seam=parseFindings, collect and
// ledgerBlockers; no_claim=that every denial spelling is read
test("alphaclaw's fix-check sentence is no blocker", () => {
expect(labels('fix-check', fixCheck(ALPHACLAW))).toEqual([]);
const prose = fixCheck(`I re-read each fix against its finding and the tests that cover it. ${ALPHACLAW}`);
expect(labels('fix-check', prose)).toEqual(['INFORMATIONAL']);
expect(blockersOf('fix-check', prose)).toEqual([]);
const clean = fixCheck(`NO FINDINGS\n\n${ALPHACLAW}`);
expect(labels('fix-check', clean)).toEqual([]);
expect(blockersOf('fix-check', clean)).toEqual([]);
});
// Value: protects=prose under NO FINDINGS that names several severities, in quotes, code spans and bullets, collects nothing;
// fails_when=any of it collects; why_new=the negative control for the NO FINDINGS rule; seam=parseFindings; no_claim=none
test('a NO FINDINGS reply with severity words in its prose collects nothing', () => {
const out = fixCheck([
'NO FINDINGS',
'',
'Coverage note: the "CRITICAL path" in auth.ts is unchanged, and I checked whether the retry change rises to High.',
'- **Callers:** the P0 alert path (`CRITICAL (the reviewer wrote Blocks: no)`) still renders the same.',
'The old High finding from pass 1 stays fixed.',
].join('\n'));
for (const reader of ['red-team', 'fix-check', 'specialist-batched', 'targeted']) expect([reader, labels(reader, out)]).toEqual([reader, []]);
});
});
describe('the structured finding formats still collect at their label (GST-2122 hold)', () => {
// Value: protects=a real CRITICAL in each brief's documented format still collects CRITICAL and counts a blocker: a JSON object for
// the JSON readers, a Severity/Blocks field for the prose voices; fails_when=any collects lower or counts no blocker;
// why_new=the prose rule narrows a fail-closed parser, so each documented format is pinned first; seam=parseFindings, collect
// and ledgerBlockers; no_claim=out-of-format replies beyond the cases below
test('a documented-format CRITICAL collects CRITICAL', () => {
const json = '{"severity":"CRITICAL","confidence":9,"path":"lib/a.ts","line":3,"category":"auth","summary":"skips the token check","fingerprint":"lib/a.ts:3:auth","specialist":"red-team"}';
for (const reader of ['red-team', 'specialist-batched', 'targeted', 'fix-check']) {
expect([reader, labels(reader, fixCheck(json))]).toEqual([reader, ['CRITICAL']]);
expect([reader, blockersOf(reader, fixCheck(json))]).toEqual([reader, [`${reader}#1`]]);
}
const prose = fixCheck('1. The handler at api.ts:9 skips the auth check. Severity: Critical. Blocks: yes');
for (const reader of ['outside-voice', 'adversarial-native']) expect([reader, labels(reader, prose)]).toEqual([reader, ['CRITICAL']]);
expect(blockersOf('outside-voice', prose)).toEqual(['outside-voice#1']);
});
// Value: protects=fail-closed on a structured finding that contradicts NO FINDINGS or is malformed: a JSON CRITICAL, a lead-label
// CRITICAL, a `Blocks: yes` and a truncated JSON object all still collect CRITICAL; fails_when=any collects lower or disappears;
// why_new=hold tests before narrowing the scanner; seam=parseFindings; no_claim=none
test('structured findings beside NO FINDINGS, and malformed JSON, stay CRITICAL', () => {
const cases = [
'NO FINDINGS\n{"severity":"CRITICAL","path":"lib/a.ts","line":3,"summary":"x"}',
'NO FINDINGS\n\nCRITICAL: the token is logged at log.ts:3.',
'NO FINDINGS\n\nThe token is logged at log.ts:3. Blocks: yes',
'{"severity":"CRITICAL","path":"lib/a.ts","line":3,"summary":"cut off',
];
for (const c of cases) expect([c, labels('red-team', fixCheck(c))]).toEqual([c, ['CRITICAL']]);
});
// Value: protects=outside a NO FINDINGS reply prose severity collects as before this change (fail-closed): #4075's surviving fix-check
// blocker "Neither fix clears the P0." still collects P0 and counts a blocker; fails_when=it collects lower or counts none;
// why_new=the Captain's narrowing of GST-2122 (a real prose blocker must not drop to a note); seam=parseFindings, collect and
// ledgerBlockers; no_claim=that every prose blocker is real (judge folds phantoms)
test("#4075's prose blocker still blocks", () => {
const out = fixCheck('Neither fix clears the P0.');
expect(labels('fix-check', out)).toEqual(['P0']);
expect(blockersOf('fix-check', out)).toEqual(['fix-check#1']);
});
// Value: protects=the bypass is bounded to clean replies: prose with no finding structure collects at its label in a reply with no
// NO FINDINGS marker and nothing in one that has it; fails_when=the unmarked reply collects lower or the marked one collects
// anything; why_new=the NO FINDINGS rule's one bypass, pinned; seam=parseFindings; no_claim=that unstructured prose under NO
// FINDINGS is never a real defect
test('unstructured prose keeps its label unless the reply says NO FINDINGS', () => {
for (const reader of ['red-team', 'fix-check', 'outside-voice']) {
expect([reader, labels(reader, fixCheck('The handler skips the auth check, which is CRITICAL.'))]).toEqual([reader, ['CRITICAL']]);
expect([reader, labels(reader, fixCheck('NO FINDINGS\n\nThe handler skips the auth check, which is CRITICAL.'))]).toEqual([reader, []]);
}
});
// Value: protects=a bold-led coverage line that only denies a severity collects nothing, a numbered heading included, under NO
// FINDINGS or not (the readers' negative controls on #4218 pass 2, adversarial-native#2 and red-team#1), while a bold-led line that
// rates a defect keeps its label; fails_when=a denial collects anything or the rated line collects lower; why_new=numbered headings
// counted as structure and brought the GST-2122 stop back for "**1. Coverage:** no fix introduced a CRITICAL defect."; seam=parseFindings;
// no_claim=that every denial spelling is read
test('a denial after a bold lead collects nothing, headings included', () => {
for (const reader of ['adversarial-native', 'red-team', 'fix-check']) {
expect([reader, labels(reader, fixCheck('**1. Coverage:** no fix introduced a CRITICAL defect.'))]).toEqual([reader, []]);
expect([reader, labels(reader, fixCheck('NO FINDINGS\n\n**1. Coverage:** no fix introduced a CRITICAL defect.'))]).toEqual([reader, []]);
expect([reader, labels(reader, fixCheck('- **Callers:** none of the fixes introduced a High regression.'))]).toEqual([reader, []]);
expect([reader, labels(reader, fixCheck('- **Auth:** the token check at auth.ts:9 is CRITICAL and missing.'))]).toEqual([reader, ['CRITICAL']]);
}
});
// Value: protects=dropping unshaped prose under NO FINDINGS never shifts the native voice's relaxed parse off its findings, so a FIXABLE
// `Severity: Low. Blocks: no` finding after such prose keeps its field; fails_when=it collects CRITICAL; why_new=/ship pass 1 on #4218
// (specialist-batched#1) found the drop misaligned the two parses; seam=parseFindings; no_claim=none
test('a NO FINDINGS reply with prose keeps a native Blocks: no finding at its field', () => {
const out = fixCheck('NO FINDINGS\n\nThe retry path looks High risk overall.\n\nFIXABLE: the log line at a.ts:3 is verbose. Severity: Low. Blocks: no');
expect(labels('adversarial-native', out)).toEqual(['Low']);
});
// Value: protects=a numbered finding heading (GST-1031, GST-1657) is finding structure, so its label stands without a file:line;
// fails_when=it collects INFORMATIONAL; why_new=/ship pass 1 on #4218 (red-team#2); seam=parseFindings; no_claim=bullets or wrapped loci
test('a numbered finding heading keeps its label', () => {
expect(labels('adversarial-native', fixCheck('**Finding 1 (CRITICAL): the session token is logged in plaintext**'))).toEqual(['CRITICAL']);
expect(labels('outside-voice', fixCheck('### 1. Session token logged in plaintext (P1)'))).toEqual(['P1']);
});
});