From 8cef24cea9684c5e97e658bc404063f712daf47f Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Sun, 1 Nov 2026 06:10:20 -0800 Subject: [PATCH] v1.92.29.0 fix(ship): review collector skips negated severity words and records the reviewer's stated severity (GST-766) (#3787) * test(review): a negated severity word in a reader's prose is not a finding (GST-766, failing first) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * fix(review): collect skips a severity word a negation governs, so closing prose like 'found nothing CRITICAL' is no finding (GST-766) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * docs(changelog): GST-766 collector skips negated severity words Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * fix: pre-landing review fixes (GST-766: no negation carry over a comma or slash; quantifiers and Markdown between negation and label; a field label or a located line is kept) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * fix: Step 11 review fixes (GST-766: a located line keeps every label; a negated label must end the clause or name a kind of finding; bounded windows) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * fix: fix-check fixes (GST-766: drop the located-line and tail-word rules that brought phantoms back; carry negation over a comma or slash; numeric zero is no negation) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * test: a finding keeps its reviewer's stated severity and Blocks (GST-766 part a, failing first) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * feat: record the reviewer's stated severity and Blocks beside the collected label (GST-766 part a) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * fix: pass 2 review fix (GST-766: a line whose labels are all negated closes the open finding, so the next finding's Severity field no longer attaches to the previous one) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * fix: Step 11 pass 2 fix (GST-766: a line whose labels are all negated always heads the next finding, indented or not) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * docs: review-policy describes negated labels and the stated record (GST-766) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * revert: keep the pinned Step 9.4 brief unchanged (GST-766); stated stays in merged.jsonl and the deferred renderer Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * docs: review-policy notes that Step 9.4 does not copy stated yet (GST-766) 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> --- CHANGELOG.md | 5 ++++ VERSION | 2 +- agents-digest/gstack-AGENTS.md | 2 +- docs/review-policy.md | 11 +++++++- lib/review-ledger.ts | 40 ++++++++++++++++++++++++++--- lib/review-scope.ts | 5 +++- package.json | 2 +- test/review-dispatch.test.ts | 46 ++++++++++++++++++++++++++++++++++ test/review-scope.test.ts | 7 ++++++ 9 files changed, 112 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7320235eb..aea2f170c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,10 @@ # Changelog +## [1.92.29.0] - 2026-11-01 + +- `/ship`'s review collector no longer turns a severity word in a negation into a finding. A fix-check reader that closed with "found nothing CRITICAL" was recorded as a CRITICAL finding with no file or line, and the judge can't drop a finding, so the run carried a phantom blocker that could stop the review loop at its cap (GST-766, from GEVA-92 on gstack-evals #108). A label right after "no", "nothing", "not", "none", "zero", "without" or "never" (optionally followed by a word like "new" or "remaining", and through Markdown emphasis or brackets), and a label joined to it by a comma, a slash, "or", "nor" or "and", now counts as no finding. A label that opens a field ("High: …") is still collected, and a prose finding worded around a negated label still arrives through its Severity or Blocks field. +- Each finding `/ship` collects now keeps the severity and Blocks its reviewer wrote (`stated` in merged.jsonl), and a deferred-review issue for a finding that carries them shows them next to the collected label, as in "CRITICAL (the reviewer wrote Severity: Low, Blocks: no)". Step 9.4 doesn't copy `stated` into its findings file yet, because that instruction lives in a reviewer brief that can change only after a no-loss replay. HARN-1 and GSTA-135 saw FIXABLE findings collected as CRITICAL while the reviewer's text said "Severity: Low. Blocks: no.", and nothing in the run showed the disagreement. What blocks a review pass is unchanged. + ## [1.92.28.0] - 2026-11-01 **A PR reviewed before #3654 keeps its /ship receipt after it merges main and moves its entry into `changelog.d/` (GST-343, #3606).** #3656 taught carry that a fragment is release metadata, but the base merge also brought in `changelog.d/README.md`. Carry counted that README as a product change, dropped the fragment rule for the whole range, and refused with RECEIPT_CARRY_PRODUCT_CHANGED although the product patch-id was unchanged. diff --git a/VERSION b/VERSION index 801577c9b..304ceb3b1 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.92.28.0 +1.92.29.0 diff --git a/agents-digest/gstack-AGENTS.md b/agents-digest/gstack-AGENTS.md index d6abbb5a0..713bc8924 100644 --- a/agents-digest/gstack-AGENTS.md +++ b/agents-digest/gstack-AGENTS.md @@ -1,4 +1,4 @@ -# gstack digest v1.92.28.0 — regenerate/re-copy after upgrading gstack +# gstack digest v1.92.29.0 — regenerate/re-copy after upgrading gstack Behavioral rules from gstack (https://github.com/garrytan/gstack), compressed for agent hosts without a full skill install. The full skills add workflows, diff --git a/docs/review-policy.md b/docs/review-policy.md index f372598db..6e23cedff 100644 --- a/docs/review-policy.md +++ b/docs/review-policy.md @@ -469,7 +469,7 @@ ones, and the arm lost blockers. `bin/gstack-review-dispatch` closes both ends: 3. `collect` hashes each reader's raw output and appends its findings to `merged.jsonl` with the label as written (a JSON line's `severity`, or the most severe of a prose line's Critical, High, Medium, Low, P0-P3, FIXABLE or INVESTIGATE - wherever they stand, headings and table rows included; a top-level list item under a + wherever they stand outside a negation (below), headings and table rows included; a top-level list item under a label-only heading takes the stronger of the two). A `Severity:` or `Blocks:` line continues the finding above it until a list item, heading, bold paragraph opener or a second line for the same field starts another: `Severity` raises its label, @@ -478,6 +478,15 @@ ones, and the arm lost blockers. `bin/gstack-review-dispatch` closes both ends: adversarial voice's FIXABLE collects as CRITICAL (normalizer v2, GSTA-167). 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 label a negation word governs ("found nothing CRITICAL", "no new **High** or Critical issues", "No P0/P1 issues": + no, nothing, not, none, zero, without or never, optionally followed by new, remaining, other, further, additional, more + or at, through Markdown, and carried to labels joined by a comma, slash, or, nor or and) is no finding unless it opens a + field (`High:`, GST-766). A line whose labels are all negated closes the open finding and heads the next, so a finding + worded around a negated label still arrives through its Severity or Blocks field. 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-review issue 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 + instruction lives in a reviewer brief that changes only after a no-loss replay. What blocks is unchanged. A phantom finding costs a fold; a lost one costs the gate. A reply with no finding counts as a review only with the clean marker its own brief asks for: `NO FINDINGS` alone on a line for the specialists, the batched specialist, the targeted reader, the Red Team and fix-check, the `Recommendation:` diff --git a/lib/review-ledger.ts b/lib/review-ledger.ts index 2c04bc512..18528a198 100644 --- a/lib/review-ledger.ts +++ b/lib/review-ledger.ts @@ -59,12 +59,31 @@ const stronger = (a: string, b: string) => ((rank(a) ?? 0) >= (rank(b) ?? 0) ? a /** A prose voice's `Blocks: yes` (GSTA-167 normalizer v2): the finding is CRITICAL whatever its label; `Blocks: no` never lowers one. */ const BLOCKS_YES = /(? (role === 'adversarial-native' && l.toUpperCase() === 'FIXABLE' ? 'CRITICAL' : l); - const strongest = (t: string) => [...t.matchAll(ALL_LABELS)].map(m => named(m[1]!)).reduce((a, b) => (a === undefined ? b : stronger(a, b)), undefined); + const asserted = (t: string) => { + const all = [...t.matchAll(ALL_LABELS)]; + let end = -1, negated = false; + return all.filter(m => { + const at = m.index!; + negated = !/^\s*:/.test(t.slice(at + m[0].length, at + m[0].length + 8).replace(MARKUP, '')) + && (NEGATED.test(t.slice(Math.max(0, at - 60), at).replace(MARKUP, '')) || (negated && NEGATION_LIST.test(t.slice(end, at).replace(MARKUP, '')))); + end = at + m[0].length; + return !negated; + }); + }; + const strongest = (t: string) => asserted(t).map(m => named(m[1]!)).reduce((a, b) => (a === undefined ? b : stronger(a, b)), undefined); const push = (label: string, raw: string, fields: string[] = []): Finding => { const f = { label, raw, fields: new Set(fields), list: LIST_ITEM.test(raw) }; out.push(f); return f; }; for (const line of output.replace(/\r\n/g, '\n').split('\n')) { const t = line.trim(); @@ -121,6 +151,7 @@ export function parseFindings(reader: string, output: string): Array<{ label: st if (typeof o.severity === 'string') { current = push(o.severity, t); continue; } } catch { /* prose that starts with a brace */ } } + if (!label && !blocks && !(section && listItem) && ANY_LABEL.test(t)) { current = null; head = t; continue; } const detail = listItem && /^[-*+]\s/.test(t) && !locus && !label && !section && !!current && !current.list && !current.fields.size; if (detail && !blocks) { current!.raw += ` | ${t}`; continue; } const fresh = (listItem && !detail) || (!indented && !detail && (t.startsWith('**') || afterGap)) || (!!current?.fields.size && locus); @@ -164,7 +195,10 @@ export function collect(ledger: string, reader: string, output: string, mergedFi const sha = promptHash(output); const prior = readLedger(ledger).filter(e => e.event === 'output' && e.reader === reader).length; const wrongTree = !unavailable && tree !== undefined && !tree.ok; - const lines = unavailable || wrongTree ? [] : parseFindings(reader, output).map((f, i) => ({ id: `${reader}#${prior ? `${prior + 1}.` : ''}${i + 1}`, reader, label: f.label, raw: f.raw, output_sha256: sha })); + const lines = unavailable || wrongTree ? [] : parseFindings(reader, output).map((f, i) => { + const s = stated(f.raw); + return { id: `${reader}#${prior ? `${prior + 1}.` : ''}${i + 1}`, reader, label: f.label, ...(s ? { stated: s } : {}), raw: f.raw, output_sha256: sha }; + }); const review = !unavailable && !wrongTree && (lines.length > 0 || hasCleanMarker(reader, output)); fs.appendFileSync(mergedFile, lines.map(l => JSON.stringify(l) + '\n').join(''), { mode: 0o600 }); appendEvent(ledger, { event: 'output', reader, sha256: sha, file: outputFile ? path.resolve(outputFile) : null, merged: path.resolve(mergedFile), ids: lines.map(l => l.id), review, ...(unavailable ? { unavailable } : {}), diff --git a/lib/review-scope.ts b/lib/review-scope.ts index 45c4c0165..e011ea4e7 100644 --- a/lib/review-scope.ts +++ b/lib/review-scope.ts @@ -28,6 +28,8 @@ export const HUNK_CONTEXT_LINES = 3; export interface ScopeFinding { fingerprint?: string; path?: string; line?: number; severity?: string; category?: string; summary?: string; problem?: string; fix?: string; specialist?: string; advisory?: boolean; test_stub?: string; + /** The severity and Blocks the reviewer's own text stated, from its merged.jsonl line (GST-766); shown, never used to classify. */ + stated?: { severity?: string; blocks?: string }; /** The changed line that causes a defect reported elsewhere (a broken caller, a consumer of a removed export); the locus follows it. */ cause?: { path?: string; line?: number }; [key: string]: unknown; @@ -252,10 +254,11 @@ export function renderDeferredIssue(f: ScopedFinding, ctx: DeferContext): { titl const title = `Deferred review finding: ${findingWhere(f)} — ${text}`.slice(0, 120); const origin = ctx.pr ? `${ctx.repo ? `${ctx.repo}#${ctx.pr}` : `PR #${ctx.pr}`}` : ctx.branch ? `branch \`${ctx.branch}\`` : 'a /ship run'; const marker = ``; + const said = [typeof f.stated?.severity === 'string' ? `Severity: ${oneLine(f.stated.severity)}` : '', typeof f.stated?.blocks === 'string' ? `Blocks: ${oneLine(f.stated.blocks)}` : ''].filter(Boolean); const body = [ `Filed by /ship's pre-landing review of ${origin}${ctx.head ? ` (head ${ctx.head.slice(0, 12)})` : ''}. The finding is in code that PR did not change, so it was deferred here instead of being fixed there (review scope frozen after pass 1).`, '', - `- **Severity:** ${oneLine(f.severity ?? 'unrated')}`, + `- **Severity:** ${oneLine(f.severity ?? 'unrated')}${said.length ? ` (the reviewer wrote ${said.join(', ')})` : ''}`, `- **Where:** \`${findingWhere(f)}\``, `- **Finding:** ${text}`, ...(f.fix ? [`- **Suggested fix:** ${oneLine(f.fix)}`] : []), diff --git a/package.json b/package.json index ebecaa84c..b928548a3 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "gstack", - "version": "1.92.28", + "version": "1.92.29", "description": "Garry's Stack — Claude Code skills + fast headless browser. One repo, one install, entire AI engineering workflow.", "license": "MIT", "type": "module", diff --git a/test/review-dispatch.test.ts b/test/review-dispatch.test.ts index 7a5cf7c41..3229c82ee 100644 --- a/test/review-dispatch.test.ts +++ b/test/review-dispatch.test.ts @@ -249,6 +249,37 @@ describe('gstack-review-dispatch sent, collect, judge and verify: nothing change expect(judge(m, [{ id: 'r#1', severity: 'INFORMATIONAL', reason: 'a real defect' }]).ok).toBe(true); }); + // Value: protects=a reader's closing prose that negates a severity word ("found nothing CRITICAL") is not collected as a finding, + // while a label stated as a finding still is; fails_when=collect records the negated word as a CRITICAL the judge cannot drop; + // why_new=GST-766: GEVA-92's fix-check#5 on gstack-evals #108 (1.92.17.0) became a phantom CRITICAL; seam=parseFindings; no_claim=prose voices' summaries that state a count + test('a negated severity word is not a finding: GEVA-92 fix-check#5 and its negated forms', () => { + const fixCheck = [ + 'REVIEWED_TREE: bedd511efa0fa16c9277821073c68e438dc760d2 /workspace/gstack-evals', + '{"severity":"INFORMATIONAL","path":"wiki/board/README.md","line":229,"summary":"Section 9\'s opening still says every ruling is recorded in the approvals file."}', + '{"severity":"INFORMATIONAL","path":"wiki/board/README.md","line":113,"summary":"The tstack row still runs on full stacks-r1 and is not labelled raw."}', + '', + 'I checked these against the post, the frozen texts and the pinned code, and found nothing CRITICAL. The arithmetic is consistent throughout:', + '- The new secondary: 47/60, 15/60, 5/40, 7/40, 4/40 and 11.5/40.', + '- The minimum p of 2^-15.', + ].join('\n'); + expect(parseFindings('fix-check', fixCheck).map(f => f.label)).toEqual(['INFORMATIONAL', 'INFORMATIONAL']); + for (const prose of ['Found no CRITICAL issues in the fix range.', 'Nothing CRITICAL or HIGH remains.', 'This is not CRITICAL.', 'I found zero Critical findings, and none High.', 'The retry path is safe; without CRITICAL impact.', + 'No new CRITICAL issues introduced by the fix.', 'No remaining CRITICAL or HIGH findings.', '**No** CRITICAL issues found.', 'No **CRITICAL** issues found.', 'No `CRITICAL` issue.', 'No [P1] issues remain.', 'Nothing at CRITICAL severity remains.', + 'No CRITICAL regressions introduced by the fix.', 'Nothing CRITICAL to report.', 'There is nothing CRITICAL in the fix range.', 'No CRITICAL — the fix is sound.', 'Found no CRITICAL or HIGH regressions.', 'No HIGH-risk changes.', + 'No CRITICAL/HIGH findings.', 'No P0/P1 issues.', 'No CRITICAL, HIGH or MEDIUM issues.', 'Checked collect() at lib/review-ledger.ts:189; no CRITICAL issues remain.', 'No CRITICAL issues; served on localhost:3000.']) + expect([prose, parseFindings('fix-check', prose)]).toEqual([prose, []]); + expect(parseFindings('r', 'Not CRITICAL, but High: the cache drops a row (c.ts:4)').map(f => f.label)).toEqual(['High']); + expect(parseFindings('r', 'No CRITICAL, High: the cache drops a row (c.ts:4)').map(f => f.label)).toEqual(['High']); + expect(parseFindings('r', 'Nothing CRITICAL / HIGH: race in q.ts:7').map(f => f.label)).toEqual(['HIGH']); + expect(parseFindings('r', 'No CRITICAL lock guards the write in Makefile:7\nSeverity: CRITICAL').map(f => f.label)).toEqual(['CRITICAL']); + expect(parseFindings('r', '1. auth.ts:3 LOW: log line too verbose\nNo CRITICAL lock guards the write in Makefile:7\nSeverity: CRITICAL').map(f => [f.label, f.raw])).toEqual([['LOW', '1. auth.ts:3 LOW: log line too verbose'], ['CRITICAL', 'No CRITICAL lock guards the write in Makefile:7 | Severity: CRITICAL']]); + expect(parseFindings('r', '1. auth.ts:3 LOW: log line too verbose\nNo CRITICAL issues remain.').map(f => f.label)).toEqual(['LOW']); + expect(parseFindings('outside-voice', '- a.ts:3 race. Severity: Low. Blocks: no\n Separately, nothing CRITICAL about b.ts:9, but it leaks fds.\n Severity: High').map(f => [f.label, f.raw])).toEqual([['Low', '- a.ts:3 race. Severity: Low. Blocks: no'], ['High', 'Separately, nothing CRITICAL about b.ts:9, but it leaks fds. | Severity: High']]); + expect(parseFindings('r', '1. A CRITICAL race in queue.ts:78 that no lock covers').map(f => f.label)).toEqual(['CRITICAL']); + expect(parseFindings('r', '- A divide by zero CRITICAL bug in parse()').map(f => f.label)).toEqual(['CRITICAL']); + expect(parseFindings('adversarial-native', 'FIXABLE: no timeout on the fetch in a.ts:3').map(f => f.label)).toEqual(['CRITICAL']); + }); + // Value: protects=normalizer v2 in production (GSTA-167 part 1, certified by round 5's G1): a prose voice's Blocks: yes reaches // merged.jsonl as CRITICAL, so judge refuses a lead that keeps it below the floor; Blocks: no never lowers a label; // fails_when=a Blocks: yes finding is collected at its Medium or INVESTIGATE label, a Blocks line with no finding before it is @@ -438,6 +469,21 @@ describe('gstack-review-dispatch sent, collect, judge and verify: nothing change .toEqual([['CRITICAL', '**Race in a.ts:3 (Medium).** | - the writer holds no lock | Severity: Medium. Blocks: yes']]); }); + // Value: protects=the reviewer's own Severity and Blocks fields travel beside the collected label in merged.jsonl (GST-766 part a: + // HARN-1 on harness-evals#200 and GSTA-135 on GST-718 saw FIXABLE collected as CRITICAL while the text said "Severity: Low. Blocks: + // no.", and the run record did not show the disagreement); fails_when=a merged line drops the stated fields or they change the + // label; why_new=collect kept only the label; seam=collect; no_claim=which of the two should block (part b, a gate change) + test('a finding keeps the severity and Blocks its reviewer stated beside the collected label', () => { + const d = tmp(); + const native = fs.readFileSync(path.join(ROOT, 'test/fixtures/review-ledger/gsta-196-native.txt'), 'utf8'); + const r = collect(path.join(d, 'l.jsonl'), 'adversarial-native', native, path.join(d, 'm.jsonl')); + expect(r.lines.map(l => [l.label, l.stated])).toEqual([ + ['High', { severity: 'High', blocks: 'no' }], ['CRITICAL', { severity: 'Low', blocks: 'no' }], ['CRITICAL', { severity: 'Low', blocks: 'no' }], ['Low', { severity: 'Low', blocks: 'no' }]]); + expect(fs.readFileSync(path.join(d, 'm.jsonl'), 'utf8').trim().split('\n').map(l => JSON.parse(l).stated)).toEqual(r.lines.map(l => l.stated)); + const json = collect(path.join(d, 'l2.jsonl'), 'fix-check', '{"severity":"INFORMATIONAL","path":"a.ts","line":3,"summary":"Severity: High is the wrong word here"}\n', path.join(d, 'm2.jsonl')); + expect(json.lines.map(l => [l.label, l.stated])).toEqual([['INFORMATIONAL', undefined]]); + }); + // Value: protects=a non-review the same dispatch later answers with a review is voided (counted, VERIFY clean), and a failed outside // voice recorded with --unavailable reads UNAVAILABLE (missing coverage, never a review) without blocking, while --unavailable is // refused for any other reader; fails_when=one outside-voice timeout or one refusal makes VERIFY dirty for good, or --unavailable diff --git a/test/review-scope.test.ts b/test/review-scope.test.ts index fb283a78e..ec9108865 100644 --- a/test/review-scope.test.ts +++ b/test/review-scope.test.ts @@ -166,6 +166,13 @@ describe('scope decisions', () => { describe('deferral rendering', () => { const finding: ScopedFinding = { fingerprint: 'lib/legacy.ts:7:data-loss', path: 'lib/legacy.ts', line: 7, severity: 'CRITICAL', category: 'data-loss', summary: 'Unbounded delete\nwithout a where clause', fix: 'add the tenant filter', specialist: 'security', locus: 'outside', kind: 'defect', blocking: true, scope: 'defer' }; + test('a deferred issue shows the severity and Blocks the reviewer stated beside the collected label (GST-766)', () => { + const issue = renderDeferredIssue({ ...finding, stated: { severity: 'Low', blocks: 'no' } }, { repo: 'acme/app', pr: 42 }); + expect(issue.body).toContain('- **Severity:** CRITICAL (the reviewer wrote Severity: Low, Blocks: no)'); + expect(renderDeferredIssue({ ...finding, stated: { blocks: 'yes\n- **Injected:** x' } }, {}).body).toContain('- **Severity:** CRITICAL (the reviewer wrote Blocks: yes - **Injected:** x)'); + expect(renderDeferredIssue(finding, {}).body).toContain('- **Severity:** CRITICAL\n'); + }); + test('the issue names the origin PR, the finding and a marker that keeps a rerun from filing twice; the body section references and never closes', () => { const issue = renderDeferredIssue(finding, { repo: 'acme/app', pr: 42, branch: 'feat/x', head: 'a'.repeat(40) }); expect(issue.title).toBe('Deferred review finding: lib/legacy.ts:7 — Unbounded delete without a where clause');