From db87940e244fbeb82a6a9e2d02e45f73b9aa17a7 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Sat, 24 Oct 2026 23:38:53 -0700 Subject: [PATCH] v1.91.198.0 fix(outside-review): a reviewer's own "not ready" verdict floors advisory-only findings (#3329, port of #3364) (#3376) * honor not-ready review verdicts with advisory findings The classifier now honors the last recognized not-ready verdict regardless of parsed severity, returning findings and exit code 3 while retaining the highest severity in the summary. Advisory findings with a ready verdict or no verdict keep their existing behavior. Regression coverage and troubleshooting documentation describe this behavior across the execution, structured, and review gates. Fixes #3329 * docs(changelog): outside reviewer not-ready verdict floors advisory findings (#3329, port of #3364) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --------- Co-authored-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- CHANGELOG.md | 14 ++++ VERSION | 2 +- agents-digest/gstack-AGENTS.md | 2 +- docs/troubleshooting.md | 6 +- lib/outside-review-result.ts | 3 +- package.json | 2 +- test/outside-review-unparsed-severity.test.ts | 76 ++++++++++++++++++- 7 files changed, 97 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 31e7f205f..2d8b94fa3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,19 @@ # Changelog +## [1.91.198.0] - 2026-10-25 + +**A second-opinion reviewer that says "not ready" is no longer reported as clean.** When the outside reviewer (Codex or Claude Code) ended its reply with `Verdict: not ready` but listed only advisory P2 or P3 findings, `lib/outside-review-result.ts` printed `VERDICT: clean` and exited 0, so /ship and /codex carried on as if the reviewer had approved. The not-ready check only ran when no severity parsed at all, so a parsed P2 or P3 switched it off (#3329). Ported from #3364 by @mvanhorn. + +### Fixed +- **`lib/outside-review-result.ts`**: when the reply's last recognized `Verdict:` line says `not ready`, the result is `findings` (exit 3) whatever severities parsed, and `FINDINGS:` still names the highest one. Advisory P2/P3 findings with a ready verdict, or with no verdict, stay `clean`. A `Verdict:` line quoted in a code fence or inline code still does not count, and the proposal and spec gates ignore it as before. +- **`docs/troubleshooting.md`**: the "Every review gate" rule under *How the verdict is read from a review* now says a not-ready reply blocks even when P2 or P3 parsed. + +### Tests +- `test/outside-review-unparsed-severity.test.ts` covers the issue's reply (numbered bold P2 plus P3, then `Verdict: not ready`) on the execution, structured and review gates, P3-only and unlabeled not-ready replies, the last verdict winning in both directions, bold, bulleted and heading verdict lines, and an unparsed label beside a parsed P2. Five of its tests fail on main's classifier and pass with the fix. Controls that stay `clean`: P2/P3 with `Verdict: ready` or no verdict, a not-ready verdict inside a code fence or inline code, and `ready-one-p3.txt`. Missing review markers, a failed execution, and the proposal and spec gates keep their old results. + +### Gate integrity +- One gate edit, and it strengthens the gate: the pinned `bare-dash-p2.txt` fixture (two P2 lines, then `Verdict: not ready (2 P2)`) moves from `VERDICT: clean`, exit 0 to `VERDICT: findings`, exit 3. Garry approved this reversal on 2026-10-24. No threshold, budget, timeout, trial count or case kind changed. + ## [1.91.197.0] - 2026-10-25 **Pregate gives touched Rust files a test lane, and runs a nested pnpm or vitest package with its own runner (#3344).** In a mixed Cargo and JavaScript repo, every touched `.rs` file failed `lanes` with "has no test lane", so /ship could not get a receipt, and a nested package's vitest tests ran under `bun test` from the repo root (about 98 false failures in the reporter's repo). Ported from #3346 by vilano93, whose two commits carry the feature; our commits on top close four findings from a security-focused review of the port and make the new fixtures hermetic. diff --git a/VERSION b/VERSION index a82223dcb..cbf5f645e 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.91.197.0 +1.91.198.0 diff --git a/agents-digest/gstack-AGENTS.md b/agents-digest/gstack-AGENTS.md index 7d73ae158..b9cfe8b0d 100644 --- a/agents-digest/gstack-AGENTS.md +++ b/agents-digest/gstack-AGENTS.md @@ -1,4 +1,4 @@ -# gstack digest v1.91.197.0 — regenerate/re-copy after upgrading gstack +# gstack digest v1.91.198.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/troubleshooting.md b/docs/troubleshooting.md index 48d90fcf2..53a2daf92 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -170,8 +170,10 @@ the readiness dashboard and the PR body, and never count it as a pass. findings. - **Every review gate.** These rules apply to every gate that summarises a review, including the `execution` gate behind `/codex` consult and challenge. - A reply whose last `Verdict:` line says `not ready` but carries no readable - severity prints `FINDINGS: unspecified` and exits 3 like a blocking finding. + A reply whose last `Verdict:` line says `not ready` is `findings` and exits + 3 even when readable advisory severities (P2 or P3) were parsed; `FINDINGS:` + names the highest one. A not-ready reply with no readable severity still + prints `FINDINGS: unspecified` and exits 3. - **No findings.** A review with no tag and no label is `clean` only when its conclusion says so: the last sentence (or its last ", and" clause), or the `because` clause of its `Recommendation:` line, opens with `NO_FINDINGS`, diff --git a/lib/outside-review-result.ts b/lib/outside-review-result.ts index 8409c73b2..ad76233ab 100644 --- a/lib/outside-review-result.ts +++ b/lib/outside-review-result.ts @@ -263,7 +263,8 @@ export function classifyOutsideReview(input: OutsideReviewInput): OutsideReviewC } if (!reviewed) return result(blocking ? 'findings' : 'clean'); const unparsed = firstUnparsed(code); - const notReady = !findings.highest && verdictNotReady(plainCode); + // A last explicit not-ready blocks even when the only parsed severities are advisory. + const notReady = verdictNotReady(plainCode); if (blocking || notReady) return { ...result('findings'), ...(unparsed ? { unparsed } : {}) }; if (unparsed) return { ...result('unverified', 'unparsed_severity', unparsed), unparsed }; if ((input.gate === 'structured' || input.gate === 'review') && !findings.highest && !concludesNoFindings(code)) { diff --git a/package.json b/package.json index 4333f42c5..e1801b105 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "gstack", - "version": "1.91.197", + "version": "1.91.198", "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/outside-review-unparsed-severity.test.ts b/test/outside-review-unparsed-severity.test.ts index bca28a2cc..82e7d0f1a 100644 --- a/test/outside-review-unparsed-severity.test.ts +++ b/test/outside-review-unparsed-severity.test.ts @@ -5,12 +5,14 @@ * FINDINGS: none` for all of them. The fixtures in * test/fixtures/outside-review-severity/codex-3105/ are synthesized from the * forms the issue documents (it describes 13 replies but does not include them). + * #3329: that same gate then printed `clean` for a parsed advisory finding + * whose last `Verdict:` line says `not ready`. `bare-dash-p2.txt` is unchanged. */ import { describe, expect, test } from 'bun:test'; import { spawnSync } from 'node:child_process'; import * as fs from 'node:fs'; import * as path from 'node:path'; -import { classifyOutsideReview, type OutsideGate } from '../lib/outside-review-result'; +import { classifyOutsideReview, validateOutsideReview, type OutsideGate } from '../lib/outside-review-result'; import { GATE_OUTCOMES } from '../lib/gate-outcomes'; const ROOT = path.resolve(import.meta.dir, '..'); @@ -26,7 +28,7 @@ describe('#3105 Codex reply shapes under the execution gate', () => { for (const [name, status, stdout] of [ ['plan-bold-dash.txt', 3, 'VERDICT: findings\nFINDINGS: P1\n'], ['diff-numbered-dash.txt', 3, 'VERDICT: findings\nFINDINGS: P1\n'], - ['bare-dash-p2.txt', 0, 'VERDICT: clean\nFINDINGS: P2\n'], + ['bare-dash-p2.txt', 3, 'VERDICT: findings\nFINDINGS: P2\n'], ['ready-one-p3.txt', 0, 'VERDICT: clean\nFINDINGS: P3\n'], ['table-p1.txt', 3, 'VERDICT: findings\nFINDINGS: P1\n'], ['not-ready-unlabeled.txt', 3, 'VERDICT: findings\nFINDINGS: unspecified\n'], @@ -112,3 +114,73 @@ describe('#3105 severity forms are read by every gate that summarises a review', expect(classifyOutsideReview({ text: proposal, gate: 'proposal' }).verdict).toBe('clean'); }); }); + +describe('#3329 a last not-ready Verdict blocks even with advisory findings', () => { + const ISSUE = '1. **P2 — The docs lag the new flag.**\n2. **P3 — Rename the helper.**\nVerdict: not ready (1 P1/P2)'; + const P2 = '1. **P2 — The docs lag the new flag.**'; + const textFor = (gate: OutsideGate, text: string) => gate === 'review' ? `${text}\n${REC}` : text; + const see = (text: string, gate: OutsideGate) => { + const r = classifyOutsideReview({ text: textFor(gate, text), gate }); + return { verdict: r.verdict as string, highest: r.findings.highest as string | null, reason: (r.reason ?? null) as string | null, unparsed: r.unparsed ?? null }; + }; + + test('numbered bold P2 plus P3 with not ready is findings/P2 on execution, structured and review', () => { + for (const gate of ['execution', 'structured', 'review'] as const) + expect({ gate, ...see(ISSUE, gate) }).toEqual({ gate, verdict: 'findings', highest: 'P2', reason: null, unparsed: null }); + expect(validateOutsideReview(ISSUE, 'structured')).toEqual({ completed: true, gate: 'fail' }); + }); + + test('P3-only and unlabeled not-ready replies block; P0/P1 block even when the last Verdict says ready', () => { + for (const gate of ['execution', 'structured', 'review'] as const) { + expect({ gate, ...see('1. **P3 — Rename the helper.**\nVerdict: not ready', gate) }).toEqual({ gate, verdict: 'findings', highest: 'P3', reason: null, unparsed: null }); + expect({ gate, ...see('The retry path can double-send.\nVerdict: not ready', gate) }).toEqual({ gate, verdict: 'findings', highest: null, reason: null, unparsed: null }); + } + expect(see('1. **P1 — The cap is ignored.**\nVerdict: ready', 'review')).toMatchObject({ verdict: 'findings', highest: 'P1' }); + expect(see('**P0 — Secrets reach the log.**\nVerdict: ready', 'execution')).toMatchObject({ verdict: 'findings', highest: 'P0' }); + }); + + test('advisory P2/P3 with ready or no Verdict stay clean', () => { + for (const text of [ + `${P2}\n2. **P3 — Rename the helper.**\nVerdict: ready`, + `${P2}\n2. **P3 — Rename the helper.**`, + '1. **P3 — Rename the helper.**\nVerdict: ready (1 P3)', + ]) { + const r = see(text, 'execution'); + expect({ text, verdict: r.verdict, highest: r.highest }).toEqual({ text, verdict: 'clean', highest: text.includes('P2') ? 'P2' : 'P3' }); + } + expect(see(`${P2}\nVerdict: ready`, 'review').verdict).toBe('clean'); + expect(validateOutsideReview(`${P2}\nVerdict: ready`, 'structured')).toEqual({ completed: true, gate: 'pass' }); + }); + + test('the last recognized Verdict wins, including supported formatting; quotes do not', () => { + expect(see(`${P2}\nVerdict: ready\nVerdict: not ready (1 P1/P2)`, 'execution')).toMatchObject({ verdict: 'findings', highest: 'P2' }); + expect(see(`${P2}\nVerdict: not ready (1 P1/P2)\nVerdict: ready`, 'execution').verdict).toBe('clean'); + for (const verdictLine of ['**Verdict:** not ready', '- Verdict: not ready', '### Verdict: not ready']) + expect({ verdictLine, ...see(`${P2}\n${verdictLine}`, 'execution') }).toMatchObject({ verdict: 'findings', highest: 'P2' }); + expect(see(`${P2}\nVerdict: not ready\n### Verdict: ready`, 'execution').verdict).toBe('clean'); + const fence = ['```', 'Verdict: not ready (2 P2)', '```'].join('\n'); + expect(see(`${P2}\nVerdict: ready\n${fence}`, 'execution').verdict).toBe('clean'); + expect(see(`${fence}\n${P2}\nVerdict: not ready`, 'execution')).toMatchObject({ verdict: 'findings', highest: 'P2' }); + expect(see(`${P2}\nVerdict: ready\nThe rubric says \`Verdict: not ready\`.`, 'execution').verdict).toBe('clean'); + expect(see(`${P2}\nThe rubric says \`Verdict: not ready\`.`, 'execution').verdict).toBe('clean'); + expect(see(`No issues found.\n${fence}`, 'review').verdict).toBe('clean'); + }); + + test('parsed P2 plus an unparsed label and not-ready still blocks and keeps the diagnostic', () => { + const text = `${P2}\n## P1/P2 findings\nVerdict: not ready`; + expect(see(text, 'execution')).toEqual({ verdict: 'findings', highest: 'P2', reason: null, unparsed: 'line 2: "## P1/P2 findings"' }); + expect(see(`${P2}\n## P1/P2 findings`, 'structured')).toMatchObject({ verdict: 'unverified', reason: 'unparsed_severity', unparsed: 'line 2: "## P1/P2 findings"' }); + }); + + test('missing markers and failed execution stay unavailable; proposal and spec ignore the verdict', () => { + expect(classifyOutsideReview({ text: `${P2}\nVerdict: not ready`, gate: 'review' }).reason).toBe('missing_markers'); + const failed = classifyOutsideReview({ text: textFor('review', `${P2}\nVerdict: not ready`), gate: 'review', exit: 1, stderr: 'fatal: provider exploded' }); + expect(failed.verdict).toBe('unavailable'); + expect(failed.reason).toBe('execution_failed'); + expect(classifyOutsideReview({ text: `${P2}\nVerdict: not ready\n${REC}`, gate: 'proposal' }).verdict).toBe('clean'); + const passing = classifyOutsideReview({ text: `SCORE: 8\nAMBIGUITIES: NONE\n${P2}\nVerdict: not ready`, gate: 'spec' }); + expect([passing.verdict, passing.score]).toEqual(['clean', 8]); + const failing = classifyOutsideReview({ text: 'SCORE: 6\nAMBIGUITIES: NONE\nVerdict: ready', gate: 'spec' }); + expect([failing.verdict, failing.score]).toEqual(['findings', 6]); + }); +});