From b244e6d11a6466650c5147cd7dc023859968c4b5 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Wed, 4 Nov 2026 01:55:42 -0800 Subject: [PATCH] fix(review): collect stops turning a reader's no-defect closing sentence into a CRITICAL finding (#3990) * fix(ship): a reader's no-defect closing sentence is not a CRITICAL finding (GST-435) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * docs(changelog): fragment for GST-435 Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * fix(ship): linear summary grammar, keep a summary line with other prose on record, skip wrapped continuations (GST-435 pass-1 red team) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> Review-Finding: p1/red-team#1 reader=red-team Review-Finding: p1/red-team#2 reader=red-team Review-Finding: p1/red-team#3 reader=red-team Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * fix(ship): a summary line under a run-on or exception line in its paragraph keeps its label (GST-435 outside voice) Review-Finding: p1/outside-voice#2 reader=outside-voice Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * docs(ship): closingSummary checks only the line directly above (GST-435 fix-check note) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * Revert "docs(ship): closingSummary checks only the line directly above (GST-435 fix-check note)" This reverts commit daf526b94a16b4d6037226697c41d91d20be0876. --------- Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- changelog.d/capy-a-readers-no-defect.md | 1 + docs/review-policy.md | 16 ++- lib/review-ledger.ts | 45 ++++++- .../gst-435-closing-no-defect.txt | 7 ++ .../gst-435-closing-nothing-critical.txt | 9 ++ test/review-closing-summary.test.ts | 110 ++++++++++++++++++ test/review-dispatch.test.ts | 55 +++++---- test/review-ledger-invariant.test.ts | 14 +++ 8 files changed, 226 insertions(+), 31 deletions(-) create mode 100644 changelog.d/capy-a-readers-no-defect.md create mode 100644 test/fixtures/review-ledger/gst-435-closing-no-defect.txt create mode 100644 test/fixtures/review-ledger/gst-435-closing-nothing-critical.txt create mode 100644 test/review-closing-summary.test.ts diff --git a/changelog.d/capy-a-readers-no-defect.md b/changelog.d/capy-a-readers-no-defect.md new file mode 100644 index 000000000..aedef3776 --- /dev/null +++ b/changelog.d/capy-a-readers-no-defect.md @@ -0,0 +1 @@ +- **A reviewer's "I found nothing CRITICAL" no longer counts as a CRITICAL finding.** /ship's review collect step used to turn a reader's own closing sentence into a blocking finding when the sentence only denied a severity ("I found nothing CRITICAL.", "I found no defect that is CRITICAL.", "Neither finding is CRITICAL."), so the review loop counted blockers that named no defect and a Captain had to grant an exception. A sentence that is wholly a denial, a count of findings or "nothing blocks", with no file, line, exception clause or field on its line, now collects nothing at the label it denies; when the same line also holds other prose, that line stays on record as an INFORMATIONAL note instead of a blocker. Every real blocker still collects: a file:line, an exception clause ("except the token leak in log.ts:3"), "nothing else" or "no other", a defect named in the same sentence, another labelled sentence, a list item or a Severity or Blocks field keeps the line at its label. (GST-435) diff --git a/docs/review-policy.md b/docs/review-policy.md index 4d7f1e250..758145cd8 100644 --- a/docs/review-policy.md +++ b/docs/review-policy.md @@ -712,8 +712,20 @@ ones, and the arm lost blockers. `bin/gstack-review-dispatch` closes both ends: `Severity` or `Blocks` line comes, so those fields never split off as a second record (GST-1031). 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. - A negated severity word still collects at its label ("found nothing CRITICAL", "No P0/P1 issues"), so a reader's - closing summary is a known phantom: it collects at that label. When it is a summary candidate (no lead label, no + 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 + exception, and whose every labelled sentence is + wholly a denial ("I found nothing CRITICAL.", "I found no defect that is CRITICAL.", "No P0/P1 issues.", "Neither + finding is CRITICAL."), a count ("I found three INFORMATIONAL issues and nothing CRITICAL" collects at + INFORMATIONAL) or "nothing blocks", with labels spelled as collect spells them. Such a line collects nothing when it + is only those sentences; when it also holds an unlabelled sentence ("Here is what I checked:", or one that may + describe a defect) it stays on record at INFORMATIONAL, never as a blocker. A quantifier that points at a finding + elsewhere ("nothing else", "no other", "remaining", "new"), a named defect in the same sentence ("No auth check + guards the admin route, so nothing CRITICAL") or any other labelled sentence on the line keeps the whole line at its + label, and a reply whose only text is a no-defect sentence, with no clean marker, is still no review. Any other negated severity word + still collects at its label, so a closing summary with a clause of its own ("I checked these against the post and + found nothing CRITICAL") is a known phantom: it collects at that label. When it is a summary candidate (no lead label, no `Severity` or `Blocks` field, and no file:line or exception clause in any sentence naming a label: "so nothing is CRITICAL", "the test covers the CRITICAL fix"), collect marks it `summary_candidate` and lead judgment may lower it with a reason, which judge records as `dismissed` (GST-1031); otherwise lead judgment can fold it only into a finding diff --git a/lib/review-ledger.ts b/lib/review-ledger.ts index 483a18ddc..bea5628c0 100644 --- a/lib/review-ledger.ts +++ b/lib/review-ledger.ts @@ -90,7 +90,7 @@ export const statesNoBlocks = (raw: string) => { if (raw.startsWith('{') || BLOC /** * A summary candidate (GST-1031): a prose finding whose every severity word is denied ("so nothing is CRITICAL", "No CRITICAL issues") * or names a fix ("the test covers the CRITICAL fix"), with no lead label, no Severity or Blocks field, no FIXABLE or INVESTIGATE tag, - * and no file:line or exception clause in any sentence naming a label. A defect called CRITICAL ("this is CRITICAL") is none. It still collects at its label (a blocker named in another sentence stays on record, GST-435); lead + * and no file:line or exception clause in any sentence naming a label. A defect called CRITICAL ("this is CRITICAL") is none. It still collects at its label (a blocker named in another sentence stays on record), unless closingSummary reads it as a no-defect sentence (GST-435); lead * judgment may lower it with a reason, which judge records as dismissed. "No CRITICAL issues except the token leak in log.ts:3" is no candidate. */ const SENTENCE = /(?<=[.!?])\s+/; @@ -101,6 +101,38 @@ const DENIED_OR_FIXED = new RegExp(`(?:(? ANY_LABEL.test(t) && !ANY_LABEL.test(t.replace(DENIED_OR_FIXED, ' ')) && !t.startsWith('{') && !t.startsWith('**') && !LIST_ITEM.test(t) && !HEADING.test(t) && !LEAD_LABEL.test(t) && !t.match(SEVERITY_FIELD) && !t.match(STATED_BLOCKS) && [...t.matchAll(ALL_LABELS)].every(m => !!rank(m[1])) && t.split(SENTENCE).filter(s => ANY_LABEL.test(s)).every(s => !PATH_LOCUS.test(s) && !EXCEPTION.test(s)); +/** + * A reader's own no-defect closing sentence (GST-435: gstack-evals #194 "I found nothing CRITICAL.", #202 "I found no defect that is + * CRITICAL..."): a prose line whose paragraph has no line above it that runs on or names an exception, 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, whose every labelled sentence is + * wholly one of these clauses, its labels spelled as ALL_LABELS spells them: a denial (`no CRITICAL issues`, `no defect that is + * CRITICAL`, `nothing at CRITICAL`, `there are no High findings`, `neither finding is CRITICAL`), a count (`I found three + * INFORMATIONAL issues`) or `nothing blocks`. It yields the labels its counts name and whether the line also holds an unlabelled + * sentence, which may describe a defect, so parseFindings keeps that line on record at INFORMATIONAL; the denied label is never a + * finding. A quantifier that points at a finding elsewhere (`nothing else`, `no other`, `remaining`), a named defect in the same + * sentence (`No auth check guards the admin route, so nothing CRITICAL`) or any other labelled sentence keeps the line as it was. + */ +const SEV = `[*\`]*(?:CRITICAL|INFORMATIONAL|Critical|Informational|High|HIGH|Medium|MEDIUM|Low|LOW|P[0-3])[*\`]*`; +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 DENY = `(? { + if (t.startsWith('{') || t.startsWith('**') || LIST_ITEM.test(t) || HEADING.test(t) || LEAD_LABEL.test(t) || t.match(SEVERITY_FIELD) || t.match(STATED_BLOCKS) || BLOCKS_YES.test(t) + || LOCUS.test(t) || PATH_LOCUS.test(t) || EXCEPTION.test(t) || /(? s.trim()); + const labelled = sentences.filter(s => ANY_LABEL.test(s)).map(s => s.replace(/^[A-Z](?=[a-z])/, c => c.toLowerCase())); + if (!labelled.length || !labelled.every(s => SUMMARY_SENTENCE.test(s))) return undefined; + return { labels: labelled.flatMap(s => [...s.replace(DENIED, ' ').matchAll(ALL_LABELS)].map(m => m[1]!)), prose: labelled.length < sentences.length }; +}; /** * The native reader's own verdict line (GSTA-258 L4: #3901 "There are three low-severity findings, all FIXABLE, and none blocks."): a prose * line, no list item, heading, lead label, file:line, Severity or Blocks field, whose only labels are FIXABLE or INVESTIGATE and which says @@ -143,6 +175,7 @@ export function parseFindings(reader: string, output: string, fixableRaises = tr let current: Finding | null = null; let head = ''; let gap = false; + let prev = ''; let treeSeen = false; const role = reader.split('/')[0]; const named = (l: string) => (role === 'adversarial-native' && l.toUpperCase() === 'FIXABLE' ? 'CRITICAL' : l); @@ -151,8 +184,10 @@ export function parseFindings(reader: string, output: string, fixableRaises = tr const fieldOf = (t: string) => { const ls = [...t.matchAll(SEVERITY_FIELD)].flatMap(m => (m[1] ?? m[2]!).match(FIELD_LABELS)!); return ls.length === 1 && (fixableRaises || role !== 'adversarial-native' || rank(ls[0]!)) ? ls[0] : undefined; }; const critical = (t: string) => BLOCKS_YES.test(t) || (fixableRaises && role === 'adversarial-native' && /(? { const l = LEAD_LABEL.exec(t)?.[1]; return l === undefined || !fixableRaises ? l : named(l); }; - const labelOf = (t: string) => { + const labelOf = (t: string, continued: boolean) => { if (role === 'adversarial-native' && quietSummary(t)) return 'FIXABLE'; + const summary = continued ? undefined : closingSummary(t); + if (summary) return strongestOf([...summary.labels.map(named), summary.prose ? 'INFORMATIONAL' : undefined]); const field = fieldOf(t); if (field === undefined) return strongest(t); return critical(t) ? 'CRITICAL' : strongestOf([leadOf(t), field])!; @@ -160,13 +195,15 @@ export function parseFindings(reader: string, output: string, fixableRaises = tr const push = (label: string, raw: string, fields: string[] = [], lead?: string): Finding => { const f = { label, raw, fields: new Set(fields), list: LIST_ITEM.test(raw), lead }; out.push(f); return f; }; for (const line of output.replace(/\r\n/g, '\n').split('\n')) { const t = line.trim(); - if (!t) { gap = true; continue; } + if (!t) { gap = true; prev = ''; continue; } if (!treeSeen && isReviewedTreeLine(t)) { treeSeen = true; continue; } const afterGap = gap; + const continued = !!prev && !HEADING.test(prev) && (EXCEPTION.test(prev) || !/[.!?…}]["'”’)\]*_`]*$/.test(prev)); + prev = t; gap = false; if (/^NO FINDINGS\.?$/.test(t) || /^Recommendation:/i.test(t)) { current = null; continue; } const indented = line !== line.trimStart(); - const label = labelOf(t); + const label = labelOf(t, continued); const blocks = BLOCKS_YES.test(t); const listItem = !indented && LIST_ITEM.test(t); const locus = LOCUS.test(t); diff --git a/test/fixtures/review-ledger/gst-435-closing-no-defect.txt b/test/fixtures/review-ledger/gst-435-closing-no-defect.txt new file mode 100644 index 000000000..ae8ae8533 --- /dev/null +++ b/test/fixtures/review-ledger/gst-435-closing-no-defect.txt @@ -0,0 +1,7 @@ +{"severity":"INFORMATIONAL","path":"wiki/plans/p/README.md","line":91,"summary":"The run-order step still says the run goes on when every linked log holds only no-contract-skill lines, where the readout stops it."} +{"severity":"INFORMATIONAL","path":"wiki/plans/p/hooks.ts.txt","line":46,"summary":"ls() now swallows every readdirSync error, so an unreadable arm folder is skipped without a word."} + +I found no defect that is CRITICAL. Here is what I checked: +- **The silent-hook rule.** All five outcomes the hook can trace are in the logged count, so a working hook can't be read as silent. +- **The lost rule.** Its result feeds only the tally and the exposure label, and no other caller reads it. +- **Tests weakened.** None. diff --git a/test/fixtures/review-ledger/gst-435-closing-nothing-critical.txt b/test/fixtures/review-ledger/gst-435-closing-nothing-critical.txt new file mode 100644 index 000000000..032a76047 --- /dev/null +++ b/test/fixtures/review-ledger/gst-435-closing-nothing-critical.txt @@ -0,0 +1,9 @@ +{"severity":"INFORMATIONAL","path":"wiki/plans/p/hooks.ts.txt","line":97,"summary":"The watch still checks a path and then opens it, so a folder swapped between the two can race the check."} +{"severity":"INFORMATIONAL","path":"wiki/plans/p/hooks.ts.txt","line":186,"summary":"lost only looks at sessions that have a delta.json, so a stopped session whose log was missed reads log_lost=no."} +{"severity":"INFORMATIONAL","path":"wiki/plans/p/README.md","line":173,"summary":"The new sentence says a delta.json listing is enough, which does not hold when the agent can write its own state root."} + +I found nothing CRITICAL. I checked these and found them sound: +- **Attachment hashes:** they match the README front matter, and the wiki check passes on this tree. +- **Missing-list parsing:** the parse at line 187 matches the hook's trace format (stop-hook.ts:134). +- **State root:** it's a real directory, so the realpath check doesn't refuse real logs. +- **Dry run:** the planted-link case does catch the old behaviour. diff --git a/test/review-closing-summary.test.ts b/test/review-closing-summary.test.ts new file mode 100644 index 000000000..3fc18879c --- /dev/null +++ b/test/review-closing-summary.test.ts @@ -0,0 +1,110 @@ +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, judge, ledgerBlockers, parseFindings, readJsonl, recordJudgment, summaryCandidate, type MergedLine } from '../lib/review-ledger'; + +const ROOT = path.resolve(import.meta.dir, '..'); +const fixture = (name: string) => fs.readFileSync(path.join(ROOT, 'test/fixtures/review-ledger', name), 'utf8'); +const labels = (output: string, reader = 'fix-check') => parseFindings(reader, output).map(f => f.label); + +describe("a reader's no-defect closing sentence is not a finding (GST-435)", () => { + // Value: protects=a fix-check's own closing sentence that denies every severity it names ("I found nothing CRITICAL", + // "I found no defect that is CRITICAL") collects nothing, so the review loop stops counting a blocker that names no defect; + // fails_when=any of these sentences collects a finding; why_new=gstack-evals #194 (blocking_found=4 from "I found nothing + // CRITICAL") and #202 (fix-check#3 from "I found no defect that is CRITICAL...") each needed a Captain exception because judge + // refuses to lower a collected CRITICAL; seam=parseFindings and collect; no_claim=that every summary shape is recognized (one + // that opens with a defect clause, "None of them breaks a caller, so there's nothing at CRITICAL", still collects as a candidate) + test('the quoted closing sentences and plain negations collect nothing', () => { + for (const line of ['I found nothing CRITICAL.', 'I found nothing CRITICAL', 'I found no defect that is CRITICAL.', 'I found no defect that is CRITICAL...', 'Neither finding is CRITICAL.', + 'No CRITICAL issues.', 'no CRITICAL issues', 'Nothing blocking, nothing CRITICAL.', 'Nothing blocking; no CRITICAL issues.', 'There are no CRITICAL findings.', + 'No CRITICAL or High findings to report.', "So there's nothing at CRITICAL.", 'Nothing here is CRITICAL.', 'Neither fix is CRITICAL.', 'Nothing in this diff is CRITICAL.', + 'No CRITICAL findings remain.', 'We found no P0 or P1 issues.', 'None of the fixes blocks landing, and nothing is CRITICAL.']) + expect([line, labels(line)]).toEqual([line, []]); + for (const line of ['I found no CRITICAL defects. Here is what I checked:', 'I found nothing CRITICAL. I checked these and found them sound:', + 'No CRITICAL issues. The webhook signature check was removed entirely.', 'The admin route never checks the session token. I found nothing CRITICAL.']) + expect([line, labels(line)]).toEqual([line, ['INFORMATIONAL']]); + }); + + test('the #194 and #202 fix-check replies collect no blocker: the closing line stays on record as a note', () => { + for (const [name, json] of [['gst-435-closing-nothing-critical.txt', 4], ['gst-435-closing-no-defect.txt', 3]] as const) { + const d = fs.mkdtempSync(path.join(os.tmpdir(), 'gst435-')); + const ledger = path.join(d, 'l.jsonl'); + const merged = path.join(d, 'm.jsonl'); + fs.writeFileSync(path.join(d, 'out.txt'), fixture(name)); + const c = collect(ledger, 'fix-check', fixture(name), merged, path.join(d, 'out.txt')); + expect([name, c.review, c.lines.map(l => l.label)]).toEqual([name, true, Array(json).fill('INFORMATIONAL')]); + fs.writeFileSync(path.join(d, 'j.jsonl'), c.lines.map(l => JSON.stringify({ id: l.id, severity: l.label })).join('\n') + '\n'); + expect(recordJudgment(ledger, c.lines, readJsonl(path.join(d, 'j.jsonl')), path.join(d, 'j.jsonl'), merged).ok).toBe(true); + expect(ledgerBlockers(ledger)).toEqual([]); + fs.rmSync(d, { recursive: true, force: true }); + } + const d = fs.mkdtempSync(path.join(os.tmpdir(), 'gst435-')); + const merged = path.join(d, 'm.jsonl'); + const r = collect(path.join(d, 'l.jsonl'), 'fix-check', fixture('geva-77-fix-check.txt'), merged); + expect(r.lines.map(l => [l.id, l.label])).toEqual([['fix-check#1', 'INFORMATIONAL'], ['fix-check#2', 'INFORMATIONAL'], ['fix-check#3', 'INFORMATIONAL']]); + expect(r.lines[2]!.raw).toStartWith('Neither finding is CRITICAL.'); + expect(judge(readJsonl(merged), r.lines.map(l => ({ id: l.id, severity: l.label }))).ok).toBe(true); + fs.rmSync(d, { recursive: true, force: true }); + }); + + test('a count summary keeps the label it counts and drops only the one it denies', () => { + expect(labels('I found three INFORMATIONAL issues and nothing CRITICAL.')).toEqual(['INFORMATIONAL']); + expect(labels('I found one High issue and nothing CRITICAL.')).toEqual(['High']); + expect(labels('None of the fixes blocks landing; I found four INFORMATIONAL defects and no CRITICAL.')).toEqual(['INFORMATIONAL']); + }); + + // Value: protects=collect stays linear on a crafted reply: a long run of joined labels that ends in a non-matching token parses in + // milliseconds; fails_when=the summary grammar backtracks exponentially (each label matching two ways), which made 24 labels take 19 s; + // why_new=GST-435 pass-1 red team measured it; seam=parseFindings; no_claim=a timing bound on any other parser path + test('a long run of joined labels parses in linear time', () => { + for (const join of [' and ', ' or ', ', ', '/']) { + const line = 'No ' + Array(40).fill('CRITICAL').join(join) + ' x'; + const t = performance.now(); + expect(labels(line)).toEqual(['CRITICAL']); + expect(performance.now() - t).toBeLessThan(250); + } + }); + + test('a reply whose only text is a no-defect sentence, with no clean marker, is still no review', () => { + const d = fs.mkdtempSync(path.join(os.tmpdir(), 'gst435-')); + const r = collect(path.join(d, 'l.jsonl'), 'fix-check', 'I found nothing CRITICAL.\n', path.join(d, 'm.jsonl')); + expect([r.review, r.lines]).toEqual([false, []]); + fs.rmSync(d, { recursive: true, force: true }); + }); + + // Value: protects=a real blocker keeps collecting at its label: a sentence that names a file:line or a defect, an exception clause + // (GST-796), an "other"/"else" quantifier that implies a blocker named elsewhere, a list item, heading, bold opener, field or + // Blocks: yes, or any second labelled sentence that is not itself a denial; fails_when=any of them collects nothing or a weaker + // label; why_new=GST-766's negation drop lost real blockers and was reverted; seam=parseFindings; no_claim=that each is a real finding + test('negative controls: a locus, a named defect, an exception, a quantifier or a structured line keeps its label', () => { + const kept: Array<[string, string]> = [['The token leak in log.ts:3 is CRITICAL.', 'CRITICAL'], ['I found nothing CRITICAL in auth.ts:9.', 'CRITICAL'], + ['No CRITICAL issues (a.ts:3).', 'CRITICAL'], ['I found nothing CRITICAL. The token leak in log.ts:3 is CRITICAL.', 'CRITICAL'], + ['I found nothing CRITICAL. The retry loop never ends, which is High.', 'CRITICAL'], ['No CRITICAL issues except the token leak in log.ts:3', 'CRITICAL'], + ['There is no CRITICAL issue other than the race in a.ts:3.', 'CRITICAL'], ['Nothing CRITICAL apart from the unescaped query.', 'CRITICAL'], + ['No new CRITICAL findings, but the race still blocks.', 'CRITICAL'], ['I found no defect that is CRITICAL, but the retry loop never ends.', 'CRITICAL'], + ['No auth check guards the admin route, so nothing CRITICAL.', 'CRITICAL'], ['There is no CRITICAL lock around the refund write, so two workers double-pay.', 'CRITICAL'], + ['No input validation is CRITICAL.', 'CRITICAL'], ['Nothing guards the write path the failure of which is CRITICAL', 'CRITICAL'], ['Nothing else is CRITICAL.', 'CRITICAL'], + ['No other issue is CRITICAL.', 'CRITICAL'], ['No remaining finding is CRITICAL.', 'CRITICAL'], ['None of the blockers are CRITICAL.', 'CRITICAL'], ['This is not CRITICAL.', 'CRITICAL'], + ['Otherwise nothing CRITICAL.', 'CRITICAL'], ['- No CRITICAL issues.', 'CRITICAL'], ['1. No CRITICAL issues.', 'CRITICAL'], ['**No CRITICAL issues.**', 'CRITICAL'], + ['CRITICAL: none of the writes are atomic.', 'CRITICAL'], ['No CRITICAL issues. Severity: High', 'High'], ['No CRITICAL issues. Blocks: yes', 'CRITICAL'], + ['{"severity":"CRITICAL","summary":"I found nothing CRITICAL"}', 'CRITICAL'], ['No CRITICAL — the upload handler drops auth.', 'CRITICAL']]; + for (const [line, label] of kept) expect([line, labels(line)]).toEqual([line, [label]]); + expect(labels('1. Token leak in log.ts:3 must be fixed before landing.\n\nNothing else is CRITICAL.')).toEqual(['CRITICAL']); + expect(labels('The token leak blocks the merge.\nNo other issue is CRITICAL.')).toEqual(['CRITICAL']); + expect(labels('## CRITICAL\n- No CRITICAL issues in the fix.')).toEqual(['CRITICAL']); + expect(labels('I found nothing CRITICAL. Here is what I checked:\n- **Auth:** the token check at auth.ts:9 is CRITICAL and missing.')).toEqual(['INFORMATIONAL', 'CRITICAL']); + expect(labels('No FIXABLE findings.', 'adversarial-native')).toEqual(['CRITICAL']); + expect(labels('Apart from the SQL injection in db.ts:40, I found\nnothing CRITICAL.')).toEqual(['CRITICAL']); + for (const wrap of ['Apart from the SQL injection in db.ts:40 —\nnothing CRITICAL.\nRecommendation: request changes because the SQL injection exposes the database', + 'Apart from the SQL injection in db.ts:40\nI found nothing CRITICAL.', 'Apart from the SQL injection in `db.ts:40`\nnothing CRITICAL.', 'The guard was removed (see auth)\nno CRITICAL issue.', + 'Apart from the SQL injection in db.ts:40, the rest is fine.\nI found nothing CRITICAL.', '1. auth.ts:3 LOW: log line too verbose\nNo CRITICAL issues remain.']) + expect([wrap, labels(wrap).at(-1)]).toEqual([wrap, 'CRITICAL']); + expect(labels('The rest of the diff is fine.\nI found nothing CRITICAL.')).toEqual([]); + expect(labels('The webhook check is gone and I found\nno CRITICAL issues.')).toEqual(['CRITICAL']); + expect(labels('I found nothing CRITICAL and two high issues.')).toEqual(['CRITICAL']); + expect(labels('## Summary\nI found nothing CRITICAL.')).toEqual([]); + expect(labels('High risk in the retry path. No CRITICAL issues.')).toEqual(['CRITICAL']); + expect(summaryCandidate('No auth check guards the admin route, so nothing CRITICAL.')).toBe(true); + }); +}); diff --git a/test/review-dispatch.test.ts b/test/review-dispatch.test.ts index 662d1ddc5..3301e3a1a 100644 --- a/test/review-dispatch.test.ts +++ b/test/review-dispatch.test.ts @@ -339,11 +339,11 @@ describe('gstack-review-dispatch sent, collect, judge and verify: nothing change expect([prose, reader, parseFindings(reader, prose).map(f => f.label)]).toEqual([prose, reader, ['CRITICAL']]); }); - // Value: protects=a negated severity word still collects at its label, so a reader's closing prose ("found nothing CRITICAL") is a - // known phantom lead judgment folds and never a dropped line (GSTA-167 S1 reverts GST-766's negation drop); fails_when=any negated - // form collects nothing or a weaker label, except under the finding's own Severity field, which decides it (GST-976); - // why_new=GST-766's drop lost blockers named in exception clauses; seam=parseFindings; no_claim=that a negated line is a real - // finding (judge folds phantoms) + // Value: protects=a negated severity word still collects at its label, so a reader's closing prose ("found nothing CRITICAL" after a + // clause of its own) is a known phantom lead judgment folds and never a dropped line (GSTA-167 S1 reverts GST-766's negation drop), + // except a sentence that is wholly a no-defect summary (GST-435), which is no finding; fails_when=any other negated form collects + // nothing or a weaker label, except under the finding's own Severity field, which decides it (GST-976); why_new=GST-766's drop lost + // blockers named in exception clauses; seam=parseFindings; no_claim=that a negated line is a real finding (judge folds phantoms) test('a negated severity word still collects at its label: GEVA-92 fix-check#5 and its negated forms are known phantoms', () => { const fixCheck = [ 'REVIEWED_TREE: bedd511efa0fa16c9277821073c68e438dc760d2 /workspace/gstack-evals', @@ -355,14 +355,16 @@ describe('gstack-review-dispatch sent, collect, judge and verify: nothing change '- The minimum p of 2^-15.', ].join('\n'); expect(parseFindings('fix-check', fixCheck).map(f => f.label)).toEqual(['INFORMATIONAL', 'INFORMATIONAL', 'CRITICAL']); - const kept: Array<[string, string]> = [['Found no CRITICAL issues in the fix range.', 'CRITICAL'], ['Nothing CRITICAL or HIGH remains.', 'CRITICAL'], ['This is not CRITICAL.', 'CRITICAL'], + const kept: Array<[string, string]> = [['Found no CRITICAL issues in the fix range.', 'CRITICAL'], ['This is not CRITICAL.', 'CRITICAL'], ['I found zero Critical findings, and none High.', 'Critical'], ['The retry path is safe; without CRITICAL impact.', 'CRITICAL'], ['No new CRITICAL issues introduced by the fix.', 'CRITICAL'], - ['No remaining CRITICAL or HIGH findings.', 'CRITICAL'], ['**No** CRITICAL issues found.', 'CRITICAL'], ['No **CRITICAL** issues found.', 'CRITICAL'], ['No `CRITICAL` issue.', 'CRITICAL'], - ['No [P1] issues remain.', 'P1'], ['Nothing at CRITICAL severity remains.', 'CRITICAL'], ['No CRITICAL regressions introduced by the fix.', 'CRITICAL'], ['Nothing CRITICAL to report.', 'CRITICAL'], + ['No remaining CRITICAL or HIGH findings.', 'CRITICAL'], ['**No** CRITICAL issues found.', 'CRITICAL'], + ['No [P1] issues remain.', 'P1'], ['No CRITICAL regressions introduced by the fix.', 'CRITICAL'], ['There is nothing CRITICAL in the fix range.', 'CRITICAL'], ['No CRITICAL — the fix is sound.', 'CRITICAL'], ['Found no CRITICAL or HIGH regressions.', 'CRITICAL'], ['No HIGH-risk changes.', 'HIGH'], - ['No CRITICAL/HIGH findings.', 'CRITICAL'], ['No P0/P1 issues.', 'P0'], ['No CRITICAL, HIGH or MEDIUM issues.', 'CRITICAL'], ['Checked collect() at lib/review-ledger.ts:189; no CRITICAL issues remain.', 'CRITICAL'], ['No CRITICAL issues; served on localhost:3000.', 'CRITICAL']]; for (const [prose, label] of kept) expect([prose, parseFindings('fix-check', prose).map(f => f.label)]).toEqual([prose, [label]]); + for (const prose of ['Nothing CRITICAL or HIGH remains.', 'No **CRITICAL** issues found.', 'No `CRITICAL` issue.', 'Nothing at CRITICAL severity remains.', 'Nothing CRITICAL to report.', + 'No CRITICAL/HIGH findings.', 'No P0/P1 issues.', 'No CRITICAL, HIGH or MEDIUM issues.']) + expect([prose, parseFindings('fix-check', prose).map(f => f.label)]).toEqual([prose, []]); expect(parseFindings('r', 'Not CRITICAL, but High: the cache drops a row (c.ts:4)').map(f => f.label)).toEqual(['CRITICAL']); expect(parseFindings('r', 'No CRITICAL, High: the cache drops a row (c.ts:4)').map(f => f.label)).toEqual(['CRITICAL']); expect(parseFindings('r', 'Nothing CRITICAL / HIGH: race in q.ts:7').map(f => f.label)).toEqual(['CRITICAL']); @@ -618,32 +620,35 @@ describe('gstack-review-dispatch sent, collect, judge and verify: nothing change expect(parseFindings('adversarial-native', 'No rate limit is FIXABLE (login.ts:9)').map(f => f.label)).toEqual(['CRITICAL']); }); - // Value: protects=a reader's own closing summary is a known phantom that keeps its label (HARN-15's Red Team and fix-check replies, - // GEVA-77's "Neither finding is CRITICAL.", "found nothing CRITICAL"): it collects, and judge refuses to drop or lower it; the - // follow-up tags such lines summary_candidate so judge may dismiss a tagged one on record; fails_when=a summary line collects nothing - // or a weaker label, which would mean a drop path is back; why_new=GST-435 removed the drop after it lost blockers in three review - // rounds; seam=parseFindings, collect and judge; no_claim=that a phantom is free (it stays CRITICAL in the judged set) - test('known phantoms: closing summaries keep their label until the summary_candidate follow-up', () => { + // Value: protects=a reader's own closing summary that opens with a clause of its own is a known phantom that keeps its label (HARN-15's + // ship2 Red Team "Red-team pass is done: …", harness-evals #152's "…, so there's nothing at CRITICAL"): it collects, and judge refuses + // to drop or lower it unless it is a summary_candidate lowered with a reason; a sentence that is wholly a no-defect summary (GEVA-77's + // "Neither finding is CRITICAL.", HARN-15 ship2 p2's count summaries) is no finding at the label it denies (GST-435); + // fails_when=a phantom collects nothing or a weaker label, or a wholly no-defect sentence still collects its denied label; + // why_new=GST-435 removed the negation drop after it lost blockers in three review rounds, then narrowed it to whole summary + // sentences; seam=parseFindings, collect and judge; no_claim=that a phantom is free (it stays CRITICAL in the judged set) + test('known phantoms: closing summaries with a clause of their own keep their label; wholly no-defect sentences collect none', () => { const fixture = (name: string) => fs.readFileSync(path.join(ROOT, 'test/fixtures/review-ledger', name), 'utf8'); - const replies: Array<[string, string, number]> = [['red-team', 'harn15-ship2-red-team.txt', 3], ['red-team', 'harn15-ship2-p2-red-team.txt', 3], - ['fix-check', 'harn15-ship2-p2-fix-check.txt', 4], ['fix-check', 'harn15-152-p2-fix-check.txt', 2]]; - for (const [reader, name, json] of replies) { + const replies: Array<[string, string, string, number]> = [['red-team', 'harn15-ship2-red-team.txt', 'CRITICAL', 3], ['red-team', 'harn15-ship2-p2-red-team.txt', 'INFORMATIONAL', 3], + ['fix-check', 'harn15-ship2-p2-fix-check.txt', 'INFORMATIONAL', 4], ['fix-check', 'harn15-152-p2-fix-check.txt', 'CRITICAL', 2]]; + for (const [reader, name, summary, json] of replies) { const found = parseFindings(reader, fixture(name)); - expect([name, found.map(f => f.label)]).toEqual([name, ['CRITICAL', ...Array(json).fill('INFORMATIONAL')]]); + expect([name, found.map(f => f.label)]).toEqual([name, [summary, ...Array(json).fill('INFORMATIONAL')]]); expect(found[0]!.raw).toMatch(/(no|nothing( at)?) CRITICAL/); } const fc = fixture('geva-77-fix-check.txt'); const d = tmp(); const merged = path.join(d, 'merged.jsonl'); const r = collect(path.join(d, 'l.jsonl'), 'fix-check', fc, merged); - expect([r.review, r.lines.map(l => [l.id, l.label])]).toEqual([true, [['fix-check#1', 'INFORMATIONAL'], ['fix-check#2', 'INFORMATIONAL'], ['fix-check#3', 'CRITICAL']]]); + expect([r.review, r.lines.map(l => [l.id, l.label])]).toEqual([true, [['fix-check#1', 'INFORMATIONAL'], ['fix-check#2', 'INFORMATIONAL'], ['fix-check#3', 'INFORMATIONAL']]]); expect(r.lines[2]!.raw).toStartWith('Neither finding is CRITICAL.'); - const judged = path.join(d, 'judged.jsonl'); - fs.writeFileSync(judged, r.lines.map(l => JSON.stringify({ id: l.id, severity: 'INFORMATIONAL' })).join('\n') + '\n'); - expect(judge(readJsonl(merged), readJsonl(judged)).ok).toBe(false); + const phantom = collect(path.join(d, 'l2.jsonl'), 'fix-check', fixture('harn15-152-p2-fix-check.txt'), path.join(d, 'm2.jsonl')); + fs.writeFileSync(path.join(d, 'j2.jsonl'), phantom.lines.map(l => JSON.stringify({ id: l.id, severity: 'INFORMATIONAL' })).join('\n') + '\n'); + expect(judge(readJsonl(path.join(d, 'm2.jsonl')), readJsonl(path.join(d, 'j2.jsonl'))).ok).toBe(false); for (const line of ['There are no CRITICAL findings.', 'No CRITICAL or High findings to report.', 'I found nothing CRITICAL.', "So there's nothing at CRITICAL.", - 'Neither finding is CRITICAL.', 'Nothing here is CRITICAL.', 'Neither fix is CRITICAL.', 'Nothing in this diff is CRITICAL.', 'This is not CRITICAL.']) - expect([line, parseFindings('outside-voice', line).map(f => f.label)]).toEqual([line, ['CRITICAL']]); + 'Neither finding is CRITICAL.', 'Nothing here is CRITICAL.', 'Neither fix is CRITICAL.', 'Nothing in this diff is CRITICAL.']) + expect([line, parseFindings('outside-voice', line).map(f => f.label)]).toEqual([line, []]); + expect(parseFindings('outside-voice', 'This is not CRITICAL.').map(f => f.label)).toEqual(['CRITICAL']); expect(parseFindings('outside-voice', 'Severity: Low (not High)').map(f => f.label)).toEqual(['Low']); }); diff --git a/test/review-ledger-invariant.test.ts b/test/review-ledger-invariant.test.ts index 13496e1c2..8cae63c3f 100644 --- a/test/review-ledger-invariant.test.ts +++ b/test/review-ledger-invariant.test.ts @@ -12,6 +12,18 @@ const LEAD = new RegExp(`(?:^|\\| )[^\\p{L}\\p{N}]*(?:\\d+[.)]\\s*[^\\p{L}\\p{N} const HEADING = new RegExp(`^(?:#{1,6}\\s*|\\*\\*)(?:${LABELS})\\b`, 'm'); const LABEL = new RegExp(`${LABELS}`, 'gi'); const LEAD_NO_FIXABLE = new RegExp(LEAD.source.replace('|FIXABLE|INVESTIGATE', ''), 'u'); +/** + * GST-435: a reader's own no-defect closing sentence is no finding at the label it denies, so these pinned inputs collect fewer or + * weaker records than main; each is held to its exact new labels on both classes. Every other input keeps main as its floor. + */ +const NO_DEFECT_SUMMARIES: Record = { + 'Nothing CRITICAL or HIGH remains.': [], 'No **CRITICAL** issues found.': [], 'No `CRITICAL` issue.': [], 'Nothing at CRITICAL severity remains.': [], + 'Nothing CRITICAL to report.': [], 'No CRITICAL/HIGH findings.': [], 'No P0/P1 issues.': [], 'No CRITICAL, HIGH or MEDIUM issues.': [], + 'There are no CRITICAL findings.': [], 'No CRITICAL or High findings to report.': [], + 'I found nothing CRITICAL.': [], "So there's nothing at CRITICAL.": [], 'Neither finding is CRITICAL.': [], 'Nothing here is CRITICAL.': [], 'Neither fix is CRITICAL.': [], + 'Nothing in this diff is CRITICAL.': [], 'review-ledger/geva-77-fix-check.txt': Array(3).fill('INFORMATIONAL'), + 'review-ledger/harn15-ship2-p2-red-team.txt': Array(4).fill('INFORMATIONAL'), 'review-ledger/harn15-ship2-p2-fix-check.txt': Array(5).fill('INFORMATIONAL'), +}; describe('parseFindings is never weaker than main (GST-976 invariant)', () => { // Value: protects=no input the suite pins, no reader fixture and no Severity field shape collects below the label main (82e9d7ec) @@ -32,6 +44,8 @@ describe('parseFindings is never weaker than main (GST-976 invariant)', () => { for (const e of entries as Array<{ reader: string; output?: string; fixture?: string; main: string[] }>) { const output = e.output ?? fs.readFileSync(path.join(ROOT, 'test/fixtures', e.fixture!), 'utf8'); const got = parseFindings(e.reader, output, raises); + const summary = NO_DEFECT_SUMMARIES[e.fixture ?? output]; + if (summary) { if (JSON.stringify(got.map(f => f.label)) !== JSON.stringify(summary)) bad.push(`GST-435 ${JSON.stringify(got.map(f => f.label))} ${JSON.stringify(output.slice(0, 80))}`); continue; } if (!e.main.length) continue; if (got.length !== e.main.length) { bad.push(`count ${got.length}/${e.main.length} ${JSON.stringify(output.slice(0, 80))}`); continue; } got.forEach((f, i) => {