diff --git a/changelog.d/capy-a-readers-no-defect.md b/changelog.d/capy-a-readers-no-defect.md index ee1f2f1ce..aedef3776 100644 --- a/changelog.d/capy-a-readers-no-defect.md +++ b/changelog.d/capy-a-readers-no-defect.md @@ -1 +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. 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 named defect, a list item or a Severity or Blocks field keeps the line at its label. (GST-435) +- **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 f31ce846c..c8c71a472 100644 --- a/docs/review-policy.md +++ b/docs/review-policy.md @@ -714,12 +714,15 @@ ones, and the arm lost blockers. `bin/gstack-review-dispatch` closes both ends: lands on the finding; a numbered item is its own finding. 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, 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". A quantifier that points - at a finding elsewhere ("nothing else", "no other", "remaining", "new"), a named defect ("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 such a sentence, with no clean marker, is still no review. Any other negated severity word + and no exception clause, that does not continue a sentence from the line above, 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 diff --git a/lib/review-ledger.ts b/lib/review-ledger.ts index 723243f52..11df41a70 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+/; @@ -103,12 +103,14 @@ export const summaryCandidate = (t: string) => ANY_LABEL.test(t) && !ANY_LABEL.t && 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 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: 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 (none for a pure denial), so - * the denied label is no finding. A quantifier that points at a finding elsewhere (`nothing else`, `no other`, `remaining`), a - * named defect (`No auth check guards the admin route, so nothing CRITICAL`) or any other labelled sentence keeps the line as it was. + * CRITICAL..."): a prose line that does not continue a sentence from the line above, 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])`; @@ -121,14 +123,15 @@ const SUMMARY_ITEM = `(?:(?:(?:I|we)\\s+(?:also\\s+)?(?:found|find|see|saw|spott + `(?:(?:one|two|three|four|five|six|seven|eight|nine|ten|\\d+|a\\s+few|several|some|an?)\\s+${SEV_LIST}\\s+${NOUN}|${DENY})` + `|(?:nothing|none\\s+of\\s+(?:them|these|the\\s+(?:findings|fixes|notes))|no\\s+(?:finding|issue)s?)\\s+(?:is\\s+|are\\s+)?(?:blocking|blocks?)(?:\\s+(?:landing|the\\s+merge|merging))?)` + `(?:\\s+(?:here|found|to\\s+report|remain(?:s|ing)?|in\\s+(?:this|the)\\s+(?:diff|change|changes|fix|fixes|patch|range|PR|review|pass|commit)))*`; -const SUMMARY_SENTENCE = new RegExp(`^(?:(?:so|overall|in\\s+short|in\\s+summary|in\\s+total|to\\s+sum\\s+up)[,:]?\\s+)?${SUMMARY_ITEM}(?:(?:\\s*[,;]\\s*|\\s+)(?:and\\s+)?${SUMMARY_ITEM})*\\s*[.!…]*$`, 'i'); -const DENIED = new RegExp(DENY, 'gi'); -export const closingSummary = (t: string): string[] | undefined => { +const SUMMARY_SENTENCE = new RegExp(`^(?:(?:so|overall|in\\s+short|in\\s+summary|in\\s+total|to\\s+sum\\s+up)[,:]?\\s+)?${SUMMARY_ITEM}(?:(?:\\s*[,;]\\s*|\\s+)(?:and\\s+)?${SUMMARY_ITEM})*\\s*[.!…]*$`); +const DENIED = new RegExp(DENY, 'g'); +export const closingSummary = (t: string): { labels: string[]; prose: boolean } | undefined => { 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) || /(? ANY_LABEL.test(s)); - if (!labelled.length || !labelled.every(s => SUMMARY_SENTENCE.test(s.trim()))) return undefined; - return labelled.flatMap(s => [...s.replace(DENIED, ' ').matchAll(ALL_LABELS)].map(m => m[1]!)); + const sentences = t.split(SENTENCE).map(s => 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 @@ -172,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); @@ -180,10 +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 = closingSummary(t); - if (summary) return strongestOf(summary.map(named)); + 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])!; @@ -191,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 = !HEADING.test(prev) && (/[,;(]$/.test(prev) || (/[\p{L}\p{N}]$/u.test(prev) && /^\p{Ll}/u.test(t))); + 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/review-closing-summary.test.ts b/test/review-closing-summary.test.ts index 985d7ddc4..5bf8d9bc2 100644 --- a/test/review-closing-summary.test.ts +++ b/test/review-closing-summary.test.ts @@ -16,16 +16,18 @@ describe("a reader's no-defect closing sentence is not a finding (GST-435)", () // 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...', - 'I found no CRITICAL defects. Here is what I checked:', 'I found nothing CRITICAL. I checked these and found them sound:', 'Neither finding is CRITICAL.', + 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 only their structured findings, and nothing blocks', () => { - for (const [name, json] of [['gst-435-closing-nothing-critical.txt', 3], ['gst-435-closing-no-defect.txt', 2]] as const) { + 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'); @@ -40,7 +42,8 @@ describe("a reader's no-defect closing sentence is not a finding (GST-435)", () 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']]); + 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 }); }); @@ -51,9 +54,21 @@ describe("a reader's no-defect closing sentence is not a finding (GST-435)", () expect(labels('None of the fixes blocks landing; I found four INFORMATIONAL defects and no CRITICAL.')).toEqual(['INFORMATIONAL']); }); - test('a summary-only reply with no clean marker is still no review', () => { + // 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', 'Both fixes hold. I found nothing CRITICAL.\n', path.join(d, 'm.jsonl')); + 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 }); }); @@ -78,8 +93,13 @@ describe("a reader's no-defect closing sentence is not a finding (GST-435)", () 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(['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']); + 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 eb933e1fa..ac76810ad 100644 --- a/test/review-dispatch.test.ts +++ b/test/review-dispatch.test.ts @@ -640,7 +640,8 @@ describe('gstack-review-dispatch sent, collect, judge and verify: nothing change 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']]]); + 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 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); diff --git a/test/review-ledger-invariant.test.ts b/test/review-ledger-invariant.test.ts index 95df302f5..82de0b054 100644 --- a/test/review-ledger-invariant.test.ts +++ b/test/review-ledger-invariant.test.ts @@ -21,7 +21,7 @@ const NO_DEFECT_SUMMARIES: Record = { 'Nothing CRITICAL to report.': [], 'No CRITICAL/HIGH findings.': [], 'No P0/P1 issues.': [], 'No CRITICAL, HIGH or MEDIUM issues.': [], '1. auth.ts:3 LOW: log line too verbose\nNo CRITICAL issues remain.': ['LOW'], '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': ['INFORMATIONAL', 'INFORMATIONAL'], + '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'), };