From 07005f3b2852586915d4a2e28311d89b757c7d9f Mon Sep 17 00:00:00 2001 From: garrytan Date: Thu, 5 Nov 2026 02:10:06 +0000 Subject: [PATCH] fix(review): lead judgment may lower a native finding to the Severity it stated beside Blocks: no; collect keeps its label (GST-1406 review fix) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- changelog.d/gst-1406-fixable-blocks-no.md | 2 +- docs/review-policy.md | 5 ++- lib/review-ledger.ts | 17 ++++----- test/review-gst1031.test.ts | 44 ++++++++++++++++------- 4 files changed, 45 insertions(+), 23 deletions(-) diff --git a/changelog.d/gst-1406-fixable-blocks-no.md b/changelog.d/gst-1406-fixable-blocks-no.md index 9f70775ae..0c223f4e4 100644 --- a/changelog.d/gst-1406-fixable-blocks-no.md +++ b/changelog.d/gst-1406-fixable-blocks-no.md @@ -1,3 +1,3 @@ ### Fixed -- **A native adversarial finding that says `Blocks: no` after some prose is no longer collected as CRITICAL (GST-1406).** `gstack-review-dispatch collect` honored GST-1031's rule (a FIXABLE finding with its own `Severity` and `Blocks: no` collects at that severity) only when the fields started their own line. When the reader wrote them after prose (`… so the docs are wrong. Severity: Medium. Blocks: no.`), the heading's FIXABLE won. If the line was in the heading's paragraph, the fields split off as a second record. Lead judgment can't lower a CRITICAL, so the native record stayed unresolved and /ship's Step 11.5 couldn't bind. Now such a line counts as the finding's fields. A finding with no `Blocks: no`, or with `Blocks: yes`, still collects CRITICAL. +- **Lead judgment can lower a native adversarial finding to the Severity its reader stated beside `Blocks: no` (GST-1406).** When the native reader wrote its `Severity` and `Blocks: no` after prose (`… so the docs are wrong. Severity: Medium. Blocks: no.`), collect kept the heading's FIXABLE as CRITICAL. Judge refused any lowering, so the native record never resolved and /ship's Step 11.5 couldn't bind. Collect still can't tell those fields from another subject's written under the same heading, so it keeps the label. Now `gstack-review-dispatch judge` lets lead judgment lower such a finding to its stated Severity, never below, with a reason. It's recorded as `dismissed`, and the ledger's blocker count follows it. Other readers, a finding that states `Blocks: yes` or no `Blocks`, or a lowering below the stated Severity or with no reason are still refused as downgrades. See docs/review-policy.md. diff --git a/docs/review-policy.md b/docs/review-policy.md index 880187dce..00d941011 100644 --- a/docs/review-policy.md +++ b/docs/review-policy.md @@ -757,7 +757,10 @@ ones, and the arm lost blockers. `bin/gstack-review-dispatch` closes both ends: 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 + with a reason, which judge records as `dismissed` (GST-1031). A native adversarial finding whose own text states + `Blocks: no` and one `Severity` field may also be lowered with a reason to that Severity, never below it, and is + recorded as `dismissed` too (GST-1406): collect keeps the FIXABLE raise when those fields follow prose, because it + 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 diff --git a/lib/review-ledger.ts b/lib/review-ledger.ts index e268bbcad..97824d96d 100644 --- a/lib/review-ledger.ts +++ b/lib/review-ledger.ts @@ -86,6 +86,11 @@ const LEAD_LABEL = new RegExp(`^(?:[^\\p{L}\\p{N}\\s]\\s*|\\d+[.)]\\s*)*(?:\\w[\ const STATED_BLOCKS = /(? { + const ls = [...raw.matchAll(SEVERITY_FIELD)].flatMap(m => (m[1] ?? m[2]!).match(FIELD_LABELS)!); + return reader.split('/')[0] === 'adversarial-native' && statesNoBlocks(raw) && ls.length === 1 && to >= (rank(ls[0]!) ?? Infinity); +}; export const statesNoBlocks = (raw: string) => { if (raw.startsWith('{') || BLOCKS_YES.test(raw)) return false; const said = [...raw.matchAll(BLOCKS_FIELD)].map(m => m[1]!.toLowerCase()); return said.length > 0 && !said.includes('yes'); }; /** * A summary candidate (GST-1031): a prose finding whose every severity word is denied ("so nothing is CRITICAL", "No CRITICAL issues") @@ -222,12 +227,6 @@ export function parseFindings(reader: string, output: string, fixableRaises = tr } const heading = LABEL_HEADING.exec(t); if (heading) { section = fixableRaises ? named(heading[1]!) : heading[1]!; current = null; head = ''; continue; } - if (current?.heading && !current.fields.size && fieldOf(t) !== undefined && !LEAD_LABEL.test(t) && !LIST_ITEM.test(t) && !HEADING.test(t) && !NUMBERED_HEAD.test(t) && !LOCUS.test(t)) { - current.fields.add('severity'); - current.label = blocks ? 'CRITICAL' : critical(current.raw) ? stronger(current.label, label!) : strongestOf([current.lead, label])!; - current.raw += ` | ${t}`; - continue; - } if (current?.heading && !current.fields.size && !label && !blocks && !/^#{1,6}\s/.test(t) && !NUMBERED_HEAD.test(t) && !t.startsWith('{')) { current.raw += ` | ${t}`; continue; } if (!indented && HEADING.test(t)) { section = null; current = null; } if (t.startsWith('{')) { @@ -345,7 +344,7 @@ export function judge(merged: MergedLine[], judged: JudgedLine[]): Judgment { const from = rank(m.label) ?? 0; const to = rank(target.severity); if (to === null) continue; - if (to < from) (!j.duplicate_of && j.reason?.trim() && summaryCandidate(m.raw) ? res.dismissed : res.downgraded).push(m.id); + if (to < from) (!j.duplicate_of && j.reason?.trim() && (summaryCandidate(m.raw) || statedLowering(m.reader, m.raw, to)) ? res.dismissed : res.downgraded).push(m.id); else if (j.duplicate_of) res.folded.push(m.id); else if (to > from) { res.promoted.push(m.id); if (!j.reason?.trim()) res.unexplained.push(m.id); } } @@ -414,6 +413,7 @@ export function ledgerBlockers(ledger: string): LedgerBlocker[] { const ranks = new Map(); const unblocking = new Map(); const raws = new Map(); + const readers = new Map(); const loci = new Map(); for (const o of events.filter(e => e.event === 'output' && Array.isArray(e.ids) && e.ids.length)) { const lines = jsonLines(read(o.merged, `merged file of ${String(o.reader)}`), String(o.merged)) as MergedLine[]; @@ -422,6 +422,7 @@ export function ledgerBlockers(ledger: string): LedgerBlocker[] { ranks.set(id, rank(line.label) ?? 0); if (statesNoBlocks(line.raw)) unblocking.set(id, rank(line.label) ?? 0); raws.set(id, line.raw); + readers.set(id, line.reader); loci.set(id, lineLocus(line.raw)); } } @@ -431,7 +432,7 @@ export function ledgerBlockers(ledger: string): LedgerBlocker[] { if (promptHash(text) !== verdict.judged_sha256) fail(`judged set ${String(verdict.judged)} changed since judge`); for (const j of jsonLines(text, String(verdict.judged)) as JudgedLine[]) { if (j.duplicate_of) { ranks.delete(j.id); continue; } - const dismissed = !!j.reason?.trim() && summaryCandidate(raws.get(j.id) ?? ''); + const dismissed = !!j.reason?.trim() && (summaryCandidate(raws.get(j.id) ?? '') || statedLowering(readers.get(j.id) ?? '', raws.get(j.id) ?? '', rank(j.severity) ?? 0)); ranks.set(j.id, dismissed ? rank(j.severity) ?? 0 : Math.max(ranks.get(j.id) ?? 0, rank(j.severity) ?? 0)); if (!loci.has(j.id)) loci.set(j.id, lineLocus(j.reason ?? '')); } diff --git a/test/review-gst1031.test.ts b/test/review-gst1031.test.ts index f17fd239f..466b0f939 100644 --- a/test/review-gst1031.test.ts +++ b/test/review-gst1031.test.ts @@ -39,19 +39,6 @@ describe('one record per native finding at its stated severity (GST-1031 case 1, } expect(labels('adversarial-native', '## FIXABLE\n1. Leak in a.ts:3\n2. Race in b.ts:4. Blocks: no', true)).toEqual(['CRITICAL', 'CRITICAL']); }); - test('fields that follow body prose on their line still count as the fields (GST-1406)', () => { - // Value: protects=ruling A when the reader writes its Severity and Blocks: no after prose on the same line, in the heading's - // paragraph or after a blank line; fails_when=the heading's FIXABLE collects CRITICAL (an unlowerable blocker that stops Step - // 11.5) or the field line splits off as a second record; why_new=GSTA-275 GST-1276 v4 native pass, finding 1; seam=parseFindings - const head = '**1. Title (FIXABLE).** body in lib/x.ts.'; - for (const raises of [true, false]) { - expect(labels('adversarial-native', [head, '', 'More prose. Severity: Medium. Blocks: no.'].join('\n'), raises)).toEqual(['Medium']); - expect(labels('adversarial-native', [head, '- a', '- b', '', 'More prose. Severity: Medium. Blocks: no, because a reader still reads it.'].join('\n'), raises)).toEqual(['Medium']); - expect(labels('adversarial-native', [head, 'More prose. Severity: Medium. Blocks: no.'].join('\n'), raises)).toEqual(['Medium']); - expect(labels('adversarial-native', [head, '', 'More prose. Severity: Medium.'].join('\n'), raises)).toEqual(['CRITICAL']); - expect(labels('adversarial-native', [head, '', 'More prose. Severity: Medium. Blocks: yes.'].join('\n'), raises)).toEqual(['CRITICAL']); - } - }); test('collect records the stated severity beside the one record', () => { const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gst1031-')); const c = collect(path.join(dir, 'l.jsonl'), 'adversarial-native', fixture('gst-1031-native.txt'), path.join(dir, 'm.jsonl')); @@ -101,6 +88,37 @@ describe('a closing summary is a candidate lead judgment may dismiss on record ( }); }); +describe('lead judgment may lower a native finding to the Severity its reader stated beside Blocks: no (GST-1031 ruling A, GST-1406)', () => { + // Value: protects=a native finding whose reader wrote its Severity and Blocks: no after prose (so collect keeps the FIXABLE raise) + // can be lowered by lead judgment to that Severity, on record with a reason, so the native record can bind; fails_when=that finding + // is an unlowerable CRITICAL (GSTA-275 GST-1276 v4 stopped Step 11.5), or the route lowers a non-native finding, one stating Blocks: + // yes or no Blocks, one below its stated Severity, or one with no reason; why_new=collect cannot tell a heading's own fields from + // another subject's (GST-1406 red team), lead judgment can; seam=judge, ledgerBlockers; no_claim=that a lowering is right (it is on record) + const OUT = ['**1. Title (FIXABLE).** body in lib/x.ts:', '- a', '- b', '', 'Before this diff it stopped. Fix: refuse it. Severity: Medium. Blocks: no, because a reader still reads it.', ''].join('\n'); + const run = (reader: string, output: string, severity: string, reason?: string) => { + const d = fs.mkdtempSync(path.join(os.tmpdir(), 'gst1406-')); + const ledger = path.join(d, 'l.jsonl'); + const merged = path.join(d, 'm.jsonl'); + fs.writeFileSync(path.join(d, 'out.txt'), output); + const c = collect(ledger, reader, output, merged, path.join(d, 'out.txt')); + fs.writeFileSync(path.join(d, 'j.jsonl'), c.lines.map(l => JSON.stringify({ id: l.id, severity, ...(reason ? { reason } : {}) })).join('\n') + '\n'); + const r = recordJudgment(ledger, c.lines, readJsonl(path.join(d, 'j.jsonl')), path.join(d, 'j.jsonl'), merged); + const out = { labels: c.lines.map(l => l.label), ok: r.ok, dismissed: r.dismissed.length, blockers: ledgerBlockers(ledger).length }; + fs.rmSync(d, { recursive: true, force: true }); + return out; + }; + test('lowered to its stated Severity with a reason, it is dismissed on record and leaves the blocker count', () => { + expect(run('adversarial-native', OUT, 'Medium', 'its reader stated Severity: Medium, Blocks: no')).toEqual({ labels: ['CRITICAL'], ok: true, dismissed: 1, blockers: 0 }); + }); + test('negative controls: no reason, below the stated Severity, Blocks: yes, no Blocks, or a non-native reader stay downgrades', () => { + expect(run('adversarial-native', OUT, 'Medium')).toMatchObject({ ok: false, dismissed: 0 }); + expect(run('adversarial-native', OUT, 'Low', 'r')).toMatchObject({ ok: false, dismissed: 0 }); + expect(run('adversarial-native', OUT.replace('Blocks: no', 'Blocks: yes'), 'Medium', 'r')).toMatchObject({ ok: false, dismissed: 0 }); + expect(run('adversarial-native', OUT.replace(' Blocks: no, because a reader still reads it.', ''), 'Medium', 'r')).toMatchObject({ ok: false, dismissed: 0 }); + expect(run('red-team', '**1. Auth bypass lets any user read admin data (CRITICAL).**\n\nThe earlier triage filed it lower. Severity: Low. Blocks: no.\n', 'Low', 'r')).toMatchObject({ ok: false, dismissed: 0 }); + }); +}); + describe('the blocker count keys on Blocks (GST-1031 case 3)', () => { // Value: protects=a CRITICAL its reader marked Blocks: no is not counted as a blocker by the ledger count, the row count or Step // 9.4's classify, while Blocks: yes, an unstated Blocks and a lead promotion still block; fails_when=REVIEW_LOOP reports a