From daf4fc22427a1e0bcf7235e59733c7ad9b5f4963 Mon Sep 17 00:00:00 2001 From: garrytan Date: Thu, 5 Nov 2026 02:33:59 +0000 Subject: [PATCH] fix(review): a fence is body only when closed, inside a prose finding and free of JSON findings; drop the heading field merge (review round 1) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- changelog.d/gst-1406-fixable-blocks-no.md | 2 +- changelog.d/gst-1409-fenced-code-body.md | 2 +- lib/review-ledger.ts | 35 +++++++++-------------- test/review-gst1031.test.ts | 15 ++++++---- 4 files changed, 25 insertions(+), 29 deletions(-) diff --git a/changelog.d/gst-1406-fixable-blocks-no.md b/changelog.d/gst-1406-fixable-blocks-no.md index 1e514bf95..b94e6e9c2 100644 --- a/changelog.d/gst-1406-fixable-blocks-no.md +++ b/changelog.d/gst-1406-fixable-blocks-no.md @@ -1,3 +1,3 @@ ### Fixed -- **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. When the native reader writes those fields after prose in the heading's own paragraph, collect now keeps them on the heading's record (at its label) instead of splitting them into a second record. Other readers, a finding that states `Blocks: yes`, no `Blocks` or a Severity that isn't a severity (such as `Severity: FIXABLE`), and a lowering below the stated Severity or with no reason are still refused as downgrades. See docs/review-policy.md. +- **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`, no `Blocks` or a Severity that isn't a severity (such as `Severity: FIXABLE`), and a lowering below the stated Severity or with no reason are still refused as downgrades. See docs/review-policy.md. diff --git a/changelog.d/gst-1409-fenced-code-body.md b/changelog.d/gst-1409-fenced-code-body.md index 0d79832d4..cec9974fb 100644 --- a/changelog.d/gst-1409-fenced-code-body.md +++ b/changelog.d/gst-1409-fenced-code-body.md @@ -1,3 +1,3 @@ ### Fixed -- **Code a reviewer quotes in a fence no longer turns into findings (GST-1409).** When a reader quoted a probe input or a checklist inside a ``` or ~~~ fence, `gstack-review-dispatch collect` parsed each quoted line. A quoted `### 1. FIXABLE: …` heading or a quoted `Blocks: yes` became a phantom CRITICAL that lead judgment couldn't lower, and the real finding lost its own Severity and Blocks fields. Now a fence that opens inside a finding is that finding's body: its lines collect nothing, and the record notes `(code block: lines)`. A JSON finding line inside a fence still collects, and a fence with no finding open parses as before. Checked against 334 stored reader outputs (668 comparisons): 664 collect the same, and the 4 that change each drop only fenced quotes. +- **Code a reviewer quotes in a fence no longer turns into findings (GST-1409).** When a reader quoted a probe input or a checklist inside a ``` or ~~~ fence, `gstack-review-dispatch collect` parsed each quoted line. A quoted `### 1. FIXABLE: …` heading or a quoted `Blocks: yes` became a phantom CRITICAL that lead judgment couldn't lower, and the real finding lost its own Severity and Blocks fields. Now a closed fence (a matching marker at least as long) that opens inside a prose finding is that finding's body: its lines collect nothing, and the record notes `(code block: lines)`. Everything else parses as before: a fence with no finding open, a fence that never closes, and a fence holding a `"severity":` key. Checked against 336 stored reader outputs (672 comparisons): 668 collect the same, and the 4 that change each drop only fenced quotes. diff --git a/lib/review-ledger.ts b/lib/review-ledger.ts index 52c6f73a5..0fe6e94f8 100644 --- a/lib/review-ledger.ts +++ b/lib/review-ledger.ts @@ -182,8 +182,6 @@ export function parseFindings(reader: string, output: string, fixableRaises = tr let gap = false; let prev = ''; let treeSeen = false; - let fenced = -1; - let fenceHost: Finding | null = null; const role = reader.split('/')[0]; const named = (l: string) => (role === 'adversarial-native' && l.toUpperCase() === 'FIXABLE' ? 'CRITICAL' : l); const strongestOf = (labels: Array) => labels.reduce((a, b) => (a === undefined ? b : b === undefined ? a : stronger(a, b)), undefined); @@ -200,20 +198,21 @@ export function parseFindings(reader: string, output: string, fixableRaises = tr return critical(t) ? 'CRITICAL' : strongestOf([leadOf(t), field])!; }; 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 all = output.replace(/\r\n/g, '\n').split('\n'); + const closes = new Map(); + for (let i = 0, open = -1, mark = ''; i < all.length; i++) { + const m = /^\s*(`{3,}|~{3,})(.*)$/.exec(all[i]!); + if (!m) continue; + if (open < 0) { open = i; mark = m[1]!; } + else if (m[1]![0] === mark[0] && m[1]!.length >= mark.length && !m[2]!.trim()) { closes.set(open, i); open = -1; } + } + for (let i = 0; i < all.length; i++) { + const line = all[i]!; const t = line.trim(); - const fence = /^(?:```|~~~)/.test(t); - if (fenced >= 0 || (fence && (current ? !current.raw.startsWith('{') : !!head))) { - if (fenced < 0) { fenced = 0; fenceHost = current; continue; } - if (fence) { - const note = `(code block: ${fenced} lines)`; - if (fenceHost) fenceHost.raw += ` | ${note}`; else if (fenced) head += ` | ${note}`; - fenced = -1; - continue; - } - const o = t.startsWith('{') ? (() => { try { return JSON.parse(t) as Record; } catch { return null; } })() : null; - if (o && typeof o.severity === 'string') current = push(o.severity, t, [], o.severity); - else fenced++; + const end = closes.get(i); + if (end !== undefined && current && !current.raw.startsWith('{') && !all.slice(i + 1, end).some(l => /"severity"\s*:/i.test(l))) { + current.raw += ` | (code block: ${end - i - 1} lines)`; + i = end; continue; } if (!t) { gap = true; prev = ''; continue; } @@ -243,12 +242,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 (role === 'adversarial-native' && current?.heading && !current.fields.size && !blocks && fieldOf(t) !== undefined && statesNoBlocks(t) && !LEAD_LABEL.test(t) && !LIST_ITEM.test(t) && !HEADING.test(t) && !NUMBERED_HEAD.test(t)) { - current.fields.add('severity'); - current.label = stronger(current.label, 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('{')) { diff --git a/test/review-gst1031.test.ts b/test/review-gst1031.test.ts index c91c1d36a..a4a113cec 100644 --- a/test/review-gst1031.test.ts +++ b/test/review-gst1031.test.ts @@ -118,11 +118,6 @@ describe('lead judgment may lower a native finding to the Severity its reader st 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 }); expect(run('adversarial-native', OUT.replace('Severity: Medium', 'Severity: FIXABLE'), 'Low', 'r')).toMatchObject({ ok: false, dismissed: 0 }); }); - test('fields written in the heading paragraph after prose stay on the heading record, which keeps its label, so it can be lowered', () => { - const same = '**1. Title (FIXABLE).** body in lib/x.ts.\nMore prose. Severity: Medium. Blocks: no.\n'; - expect(run('adversarial-native', same, 'Medium', 'its reader stated Severity: Medium, Blocks: no')).toEqual({ labels: ['CRITICAL'], ok: true, dismissed: 1, blockers: 0 }); - expect(run('red-team', '**1. Title (CRITICAL).** body in lib/x.ts.\nMore prose. Severity: Low. Blocks: no.\n', 'CRITICAL')).toMatchObject({ labels: ['CRITICAL', 'Low'] }); - }); }); describe('fenced code in a reply is body text of the open finding (GST-1409)', () => { @@ -134,7 +129,15 @@ describe('fenced code in a reply is body text of the open finding (GST-1409)', ( test('a quoted heading and Blocks words inside a fence collect nothing; the finding keeps its fields', () => { for (const raises of [true, false]) { expect(labels('adversarial-native', ['Intro.', '', '**1. Real issue (FIXABLE).** body.', '', ...fence, '', 'Severity: Medium', 'Blocks: no', ''].join('\n'), raises)).toEqual(['Medium']); - expect(labels('red-team', ['**1. Real issue.** body in a.ts:3.', ...fence, 'Severity: Low. Blocks: no.', ''].join('\n'), raises)).toEqual(['Low']); + expect(labels('red-team', ['**1. Real issue (Medium).** body in a.ts:3.', ...fence, 'Severity: Low. Blocks: no.', ''].join('\n'), raises)).toEqual(['Low']); + } + }); + test('blocker-loss controls: a fence with only an intro before it, an unclosed fence, fenced JSON and a shorter closer still collect', () => { + for (const reader of ['red-team', 'adversarial-native']) { + expect(labels(reader, ['Here are my findings:', '```markdown', '**1. SQL injection in a.ts:3 (CRITICAL).** x', 'Blocks: yes', '```', ''].join('\n'), true)).toContain('CRITICAL'); + expect(labels(reader, ['**1. Minor naming (INFORMATIONAL).** in a.ts:1', '```ts', 'const x = 1;', '', '**2. Token leak in log.ts:3 (CRITICAL).** Blocks: yes', ''].join('\n'), true)).toContain('CRITICAL'); + expect(labels(reader, ['**1. Minor naming in a.ts:1 (INFORMATIONAL).** Rename it.', '```json', '{', ' "severity": "CRITICAL", "path": "log.ts", "line": 3,', ' "summary": "token leak"', '}', '```', ''].join('\n'), true)).toContain('CRITICAL'); + expect(labels(reader, ['**1. Minor naming in a.ts:1 (INFORMATIONAL).** Rename it.', '````', '```', '````', '', '**2. Token leak in log.ts:3 (CRITICAL).** Blocks: yes', ''].join('\n'), true)).toContain('CRITICAL'); } }); test('controls: a fence with no open finding still parses, and a finding after the fence closes stands on its own', () => {