From 6f5eb600ca26fc1ae31740fea6cfa36a4cb6b310 Mon Sep 17 00:00:00 2001 From: garrytan Date: Sat, 3 Oct 2026 20:49:00 +0000 Subject: [PATCH] B1b: structured outside-review gate fails on P0 as well as P1 validateOutsideReview() derived its structured pass/fail from a [P1] match, so a review reporting only [P0] findings passed. The wrapper now reads the classifier's findings verdict (P0 or P1), and the /review and /ship structured-gate prose requests [P0]-[P3] tags and fails on P0/P1; untagged output (OUTSIDE_STATUS: unverified) is missing coverage. The ship clarity pin moves from 'structured P1' to 'structured P0/P1' with the behavior. --- lib/outside-review-result.ts | 2 +- scripts/resolvers/outside-voice-steps.ts | 6 +++--- test/outside-review-result.test.ts | 7 +++++++ test/ship-workflow-clarity.test.ts | 2 +- 4 files changed, 12 insertions(+), 5 deletions(-) diff --git a/lib/outside-review-result.ts b/lib/outside-review-result.ts index 8e10fe822..4d2b6ff1d 100644 --- a/lib/outside-review-result.ts +++ b/lib/outside-review-result.ts @@ -92,7 +92,7 @@ export function validateOutsideReview(text: string, gate: OutsideGate): { comple return { completed: false, reason }; } if (gate === 'spec') return { completed: true, score: checked.score, gate: checked.verdict === 'clean' ? 'pass' : 'fail' }; - if (gate === 'structured') return { completed: true, gate: /\[P1\]|^P1:/m.test(plainReview(text)) ? 'fail' : 'pass' }; + if (gate === 'structured') return { completed: true, gate: checked.verdict === 'findings' ? 'fail' : 'pass' }; return { completed: true }; } diff --git a/scripts/resolvers/outside-voice-steps.ts b/scripts/resolvers/outside-voice-steps.ts index 05f216737..1290c02ad 100644 --- a/scripts/resolvers/outside-voice-steps.ts +++ b/scripts/resolvers/outside-voice-steps.ts @@ -151,7 +151,7 @@ Outside prompt (supply repository context from the parent): ${outsideVoiceInvocation(ctx, { timeoutMs: 540000, nativeAlreadyRequired: true, diffCommand: 'DIFF_BASE=$(git merge-base origin/ HEAD) && git diff "$DIFF_BASE"' })} -Present the full output verbatim. ${isShip ? 'An unavailable outside challenge does not block shipping by itself; supported findings still enter Step 11, and the structured P1 and non-convergence gates still apply.' : 'This outside challenge is informational; supported findings still enter Step 5 Fix-First, whose approval and convergence gates apply.'} +Present the full output verbatim. ${isShip ? 'An unavailable outside challenge does not block shipping by itself; supported findings still enter Step 11, and the structured P0/P1 and non-convergence gates still apply.' : 'This outside challenge is informational; supported findings still enter Step 5 Fix-First, whose approval and convergence gates apply.'} **Error handling:** Only this optional outside adversarial pass is non-blocking; native completion and structured-review decisions still apply. ${outsideVoiceFailurePolicy(ctx, { timeoutMinutes: 9, onTimeout: 'missing-coverage', stderrOnEmpty: true, fallback: 'none', escape: 1 })} @@ -168,14 +168,14 @@ function adversarialStructuredReview(ctx: TemplateContext, isShip: boolean): str If \`CODEX_MODE\` is \`ready\` and either \`DIFF_TOTAL >= 200\` or the user requested the override above: -Prepare a structured review prompt requesting severity-tagged findings ([P1], [P2], [P3]) or an explicit NO_FINDINGS conclusion. Preserve the base-branch scope including committed changes and working-tree changes. +Prepare a structured review prompt requesting severity-tagged findings ([P0]-[P3]) or an explicit NO_FINDINGS conclusion. Preserve the base-branch scope including committed changes and working-tree changes. ${outsideVoiceInvocation(ctx, { timeoutMs: 540000, nativeAlreadyRequired: true, structuredBase: '', gate: 'structured', diffCommand: 'DIFF_BASE=$(git merge-base HEAD) && git diff "$DIFF_BASE"' })} ${outsideVoiceFor(ctx).id === 'codex' ? 'The Codex backend uses `codex review --base` without a positional prompt: those arguments are mutually exclusive. Never drop --base to resolve an argv error; prompt-only review changes the diff scope.' : 'The Claude Code backend receives the parent-captured base diff, including committed and working-tree changes, because review mode cannot execute git.'} Present output under \`${outsideVoiceFor(ctx).label.toUpperCase()} SAYS (code review):\` inside a \`tool-output\` fence. -Only a completed response with severity tags or an explicit no-findings conclusion establishes the gate. P1 findings (\`[P1]\` or native \`P1:\` labels) → GATE: FAIL. Completed without P1 → GATE: PASS. Refusal, failure, or missing markers → GATE: MISSING COVERAGE; preserve the existing user decision flow. +Only a completed response with severity tags or an explicit no-findings conclusion establishes the gate. P0/P1 findings (\`[P0]\`/\`[P1]\` or native \`P0:\`/\`P1:\` labels; \`VERDICT: findings\`) → GATE: FAIL. Completed without P0/P1 → GATE: PASS. Refusal, failure, missing markers or \`OUTSIDE_STATUS: unverified\` → GATE: MISSING COVERAGE; preserve the existing user decision flow. If GATE is FAIL, use AskUserQuestion: \`\`\` diff --git a/test/outside-review-result.test.ts b/test/outside-review-result.test.ts index cb987c471..4dea9c97a 100644 --- a/test/outside-review-result.test.ts +++ b/test/outside-review-result.test.ts @@ -73,6 +73,13 @@ describe('validateOutsideReview keeps the text-only shape for direct importers', expect(validateOutsideReview('[P2] nit', 'structured')).toEqual({ completed: true, gate: 'pass' }); expect(validateOutsideReview('[P1] bug', 'structured')).toEqual({ completed: true, gate: 'fail' }); }); + + test('B1b: the structured gate fails on P0 as well as P1', () => { + for (const text of ['[P0] corrupts every row', 'P0: corrupts every row', '**P0:** corrupts every row', '[P3] nit\n[P0] corrupts every row']) { + expect(validateOutsideReview(text, 'structured')).toEqual({ completed: true, gate: 'fail' }); + } + expect(validateOutsideReview('[P2] naming\n[P3] nit', 'structured')).toEqual({ completed: true, gate: 'pass' }); + }); }); describe('outside-review-result CLI', () => { diff --git a/test/ship-workflow-clarity.test.ts b/test/ship-workflow-clarity.test.ts index ec654fb2e..6578410c0 100644 --- a/test/ship-workflow-clarity.test.ts +++ b/test/ship-workflow-clarity.test.ts @@ -140,7 +140,7 @@ test.each(ALL_HOST_CONFIGS.map(({ name }) => name))('%s: ship adversarial approv test('outside challenge availability never resets fix limits or bypasses blocking owners', () => { const adversarial = compact(readTemplate('ship/sections/adversarial.md')); expect(adversarial).toMatch(/an unavailable outside challenge does not block shipping by itself/i); - expect(adversarial).toMatch(/structured P1 and non-convergence gates still apply/i); + expect(adversarial).toMatch(/structured P0\/P1 and non-convergence gates still apply/i); expect(adversarial).toMatch(/returning here never resets Step 9's three-cycle fix limit/i); const standaloneReview = readTemplate('review/sections/adversarial.md'); expect(standaloneReview).toContain('Step 5 Fix-First');