From ce1d5cf1309fb0a0f528b1c983458d8e06c0bd57 Mon Sep 17 00:00:00 2001 From: pseudo Date: Sat, 11 Jul 2026 00:41:23 -0600 Subject: [PATCH] [Bugfix #1166] Skip stubs and no-verdict output parse as SKIPPED, never as approval MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A consult lane that never produced a review (agy skip artifact, or output with no parseable VERDICT line) previously parsed as COMMENT, which allApprove counts as an approving reviewer — so a skipped/garbage lane masqueraded as a passing review at porch gates. - New Verdict member SKIPPED: emitted explicitly by agySkipContent, and detected on legacy stubs via the 'lane skipped' SUMMARY marker; the no-verdict fallback now returns SKIPPED instead of COMMENT. - allApprove excludes SKIPPED lanes (progression on remaining reviewers, Spec 778 guarantee preserved) but blocks when EVERY lane skipped. - porch next() emits a dedicated re-run task for the all-skipped round (a rebuttal is the wrong remediation for feedback that does not exist). - Gate message discloses reduced coverage instead of 'All reviewers approved!' when a lane was skipped; SKIPPED annotated in verdict listings and history icons. Empty/near-empty output still parses as REQUEST_CHANGES (crashed lane = retry, not skip). Explicit APPROVE/REQUEST_CHANGES are never reinterpreted. Found via entriq #2467 / spec 2458: the skipped gemini lane, re-run manually, returned REQUEST_CHANGES with the round's most serious finding on a money-critical spec; the stub would have passed it. Co-Authored-By: Claude Fable 5 --- packages/codev/src/__tests__/consult.test.ts | 8 +-- packages/codev/src/commands/consult/index.ts | 19 +++++-- .../__tests__/agy-porch-progression.test.ts | 38 +++++++++++-- .../__tests__/agy-skip-progression.test.ts | 57 ++++++++++++++----- .../porch/__tests__/parse-verdict.test.ts | 51 ++++++++++++++++- packages/codev/src/commands/porch/next.ts | 32 ++++++++++- packages/codev/src/commands/porch/prompts.ts | 3 +- packages/codev/src/commands/porch/types.ts | 7 ++- packages/codev/src/commands/porch/verdict.ts | 54 +++++++++++++++--- 9 files changed, 228 insertions(+), 41 deletions(-) diff --git a/packages/codev/src/__tests__/consult.test.ts b/packages/codev/src/__tests__/consult.test.ts index ac8cde623..713c78314 100644 --- a/packages/codev/src/__tests__/consult.test.ts +++ b/packages/codev/src/__tests__/consult.test.ts @@ -317,7 +317,7 @@ describe('consult command', () => { expect(threw).toBe(false); // non-blocking: resolves, never throws const written = stdoutSpy.mock.calls.map(c => String(c[0])).join(''); - expect(written).toContain('VERDICT: COMMENT'); + expect(written).toContain('VERDICT: SKIPPED'); expect(written).toMatch(/skipped/i); } finally { stdoutSpy.mockRestore(); @@ -862,7 +862,7 @@ describe('consult command', () => { } }); - it('skips non-blockingly (VERDICT: COMMENT) when agy is unauthenticated', async () => { + it('skips non-blockingly (VERDICT: SKIPPED) when agy is unauthenticated', async () => { const { consult, spawn } = await loadAgy(); spawn.mockClear(); spawn.mockReturnValueOnce(makeFakeAgyProc({ @@ -876,7 +876,7 @@ describe('consult command', () => { try { await consult({ model: 'gemini', prompt: 'review' }); } catch { threw = true; } expect(threw).toBe(false); // non-blocking const written = stdoutSpy.mock.calls.map(c => String(c[0])).join(''); - expect(written).toContain('VERDICT: COMMENT'); + expect(written).toContain('VERDICT: SKIPPED'); expect(written).toMatch(/not authenticated/i); } finally { stdoutSpy.mockRestore(); @@ -899,7 +899,7 @@ describe('consult command', () => { try { await consult({ model: 'gemini', prompt: 'review' }); } catch { threw = true; } expect(threw).toBe(false); // non-blocking const written = stdoutSpy.mock.calls.map(c => String(c[0])).join(''); - expect(written).toContain('VERDICT: COMMENT'); + expect(written).toContain('VERDICT: SKIPPED'); expect(written).toMatch(/timed out/i); } finally { stdoutSpy.mockRestore(); diff --git a/packages/codev/src/commands/consult/index.ts b/packages/codev/src/commands/consult/index.ts index 28d3bfc0a..6a1864c89 100644 --- a/packages/codev/src/commands/consult/index.ts +++ b/packages/codev/src/commands/consult/index.ts @@ -701,19 +701,26 @@ export function resolveAgyBin(): string | null { return null; } -/** Non-blocking skip artifact: porch's verdict parser treats COMMENT as non-blocking. */ +/** + * Lane-skip artifact: porch parses VERDICT: SKIPPED as "this lane produced no + * review" — non-blocking for progression (the remaining reviewers still apply) + * but excluded from approval math, so a skip can never masquerade as a passing + * review (entriq #2467: a COMMENT stub nearly passed a defective money-critical + * spec through a gate). The `SUMMARY: ... lane skipped` marker doubles as the + * legacy-stub detector in porch's parseVerdict — keep both in sync. + */ function agySkipContent(reason: string): string { return [ '---', - 'VERDICT: COMMENT', + 'VERDICT: SKIPPED', `SUMMARY: Gemini lane skipped — ${reason}`, 'CONFIDENCE: LOW', '---', '', - `The Gemini (Antigravity \`agy\`) reviewer was skipped: ${reason}.`, - 'This is a non-blocking skip; the remaining reviewers still apply. To enable the', - 'Gemini lane, install the CLI (https://antigravity.google/cli/install.sh) and run', - '`agy` once to sign in.', + `The Gemini (Antigravity \`agy\`) reviewer was skipped: ${reason}. No review was`, + 'produced; porch records this lane as SKIPPED (not run) and the remaining', + 'reviewers still apply. To enable the Gemini lane, install the CLI', + '(https://antigravity.google/cli/install.sh) and run `agy` once to sign in.', ].join('\n'); } diff --git a/packages/codev/src/commands/porch/__tests__/agy-porch-progression.test.ts b/packages/codev/src/commands/porch/__tests__/agy-porch-progression.test.ts index c71ec9b92..6c4493758 100644 --- a/packages/codev/src/commands/porch/__tests__/agy-porch-progression.test.ts +++ b/packages/codev/src/commands/porch/__tests__/agy-porch-progression.test.ts @@ -122,7 +122,7 @@ describe('porch progression with a skipped agy/gemini lane (drives next())', () fs.mkdirSync(path.dirname(statusPath), { recursive: true }); writeState(statusPath, state); - // gemini lane = the real skip artifact agy emits when unavailable → COMMENT + // gemini lane = the real skip artifact agy emits when unavailable → SKIPPED writeReviews(testDir, state, { gemini: _agySkipContent('agy CLI not found'), codex: APPROVE, @@ -131,13 +131,43 @@ describe('porch progression with a skipped agy/gemini lane (drives next())', () const res = await next(testDir, '0778'); - // Porch advanced: it requested the human `pr` gate ("All reviewers approved!"), - // NOT a rebuttal/re-iteration. The skipped lane did not block progression. + // Porch advanced: it requested the human `pr` gate, NOT a rebuttal / + // re-iteration. The skipped lane did not block progression — but the gate + // message must disclose the reduced coverage instead of claiming a full + // 3-way approval (entriq #2467). expect(res.status).toBe('gate_pending'); expect(res.gate).toBe('pr'); const subjects = (res.tasks ?? []).map(t => t.subject).join(' | '); expect(subjects).not.toMatch(/rebuttal/i); - expect((res.tasks ?? []).map(t => t.description).join('\n')).toMatch(/All reviewers approved/); + const descriptions = (res.tasks ?? []).map(t => t.description).join('\n'); + expect(descriptions).toMatch(/All effective reviewers approved/); + expect(descriptions).toMatch(/SKIPPED/); + expect(descriptions).not.toMatch(/^All reviewers approved!/m); + }); + + it('does NOT advance when EVERY lane skipped (zero real reviews)', async () => { + const state = makeState(); + const statusPath = getStatusPath(testDir, state.id, state.title); + fs.mkdirSync(path.dirname(statusPath), { recursive: true }); + writeState(statusPath, state); + + // All three lanes emit skip artifacts — no review actually happened. + writeReviews(testDir, state, { + gemini: _agySkipContent('agy CLI not found'), + codex: _agySkipContent('no response before timeout'), + claude: _agySkipContent('no response before timeout'), + }); + + const res = await next(testDir, '0778'); + + // The gate must not pass on zero evidence, and the remediation is to + // re-run the consultations — not to write a rebuttal against feedback + // that does not exist. + expect(res.status).toBe('tasks'); + expect(res.gate).toBeUndefined(); + const subjects = (res.tasks ?? []).map(t => t.subject).join(' | '); + expect(subjects).toMatch(/skipped/i); + expect(subjects).not.toMatch(/rebuttal/i); }); it('does NOT mask a genuine REQUEST_CHANGES (gemini skipped, codex blocks)', async () => { diff --git a/packages/codev/src/commands/porch/__tests__/agy-skip-progression.test.ts b/packages/codev/src/commands/porch/__tests__/agy-skip-progression.test.ts index 8f801f797..48eb7850a 100644 --- a/packages/codev/src/commands/porch/__tests__/agy-skip-progression.test.ts +++ b/packages/codev/src/commands/porch/__tests__/agy-skip-progression.test.ts @@ -1,20 +1,22 @@ import { describe, it, expect } from 'vitest'; -import { parseVerdict, allApprove } from '../verdict'; +import { parseVerdict, allApprove, effectiveReviews } from '../verdict'; import { _agySkipContent } from '../../consult/index.js'; import type { ReviewResult } from '../types.js'; /** - * Phase-progression guarantee for the agy backend (Spec 778). + * Phase-progression guarantee for the agy backend (Spec 778), hardened per + * entriq #2467. * * When the Antigravity CLI (`agy`) is missing, unauthenticated, or times out, the - * gemini consult lane emits a non-blocking skip artifact instead of failing the run. - * Porch parses that artifact as COMMENT, and `allApprove` treats COMMENT as - * non-blocking — so a SPIR/ASPIR/BUGFIX phase still advances on the strength of the - * remaining reviewers (codex + claude). These tests pin that contract end-to-end - * against the REAL skip artifact, so a regression in either the artifact wording or - * the verdict parser is caught. + * gemini consult lane emits a skip artifact instead of failing the run. Porch + * parses that artifact as SKIPPED: non-blocking (a SPIR/ASPIR/BUGFIX phase still + * advances on the strength of the remaining reviewers) but NEVER counted as an + * approving review — the original stub carried VERDICT: COMMENT, which counts as + * approval, so a lane that never ran masqueraded as a passing third reviewer. + * These tests pin the contract end-to-end against the REAL skip artifact, so a + * regression in either the artifact wording or the verdict parser is caught. */ -describe('agy skip is non-blocking for porch progression', () => { +describe('agy skip parses as SKIPPED and is non-blocking for porch progression', () => { const skipReasons = [ 'agy CLI not found', 'authentication required (OAuth)', @@ -22,18 +24,25 @@ describe('agy skip is non-blocking for porch progression', () => { ]; for (const reason of skipReasons) { - it(`real skip artifact (${reason}) parses as COMMENT`, () => { - expect(parseVerdict(_agySkipContent(reason))).toBe('COMMENT'); + it(`real skip artifact (${reason}) parses as SKIPPED, never as a review verdict`, () => { + expect(parseVerdict(_agySkipContent(reason))).toBe('SKIPPED'); }); } + it('legacy skip stubs (VERDICT: COMMENT + skip SUMMARY marker) also parse as SKIPPED', () => { + // Artifacts written by older consult versions still on disk / older installs. + const legacy = _agySkipContent('agy CLI not found').replace('VERDICT: SKIPPED', 'VERDICT: COMMENT'); + expect(parseVerdict(legacy)).toBe('SKIPPED'); + }); + it('a 3-way phase with gemini skipped still passes (2-way effective)', () => { const reviews: ReviewResult[] = [ { model: 'gemini', verdict: parseVerdict(_agySkipContent('agy CLI not found')), file: '/tmp/g.md' }, { model: 'codex', verdict: 'APPROVE', file: '/tmp/c.md' }, { model: 'claude', verdict: 'APPROVE', file: '/tmp/cl.md' }, ]; - expect(reviews[0].verdict).toBe('COMMENT'); + expect(reviews[0].verdict).toBe('SKIPPED'); + expect(effectiveReviews(reviews)).toHaveLength(2); expect(allApprove(reviews)).toBe(true); }); @@ -46,10 +55,30 @@ describe('agy skip is non-blocking for porch progression', () => { expect(allApprove(reviews)).toBe(false); }); - it('skip artifact is self-describing (names the lane and the remediation)', () => { + it('a skip never counts as the approving vote: skip + APPROVE advances on the real review alone', () => { + // The entriq #2467 shape: with the old COMMENT stub this was recorded as a + // 2-approval round when only ONE review actually happened. + const reviews: ReviewResult[] = [ + { model: 'gemini', verdict: parseVerdict(_agySkipContent('agy CLI not found')), file: '/tmp/g.md' }, + { model: 'claude', verdict: 'APPROVE', file: '/tmp/cl.md' }, + ]; + expect(effectiveReviews(reviews)).toHaveLength(1); + expect(allApprove(reviews)).toBe(true); + }); + + it('ALL lanes skipped = zero real reviews = the gate must NOT pass', () => { + const reviews: ReviewResult[] = [ + { model: 'gemini', verdict: parseVerdict(_agySkipContent('agy CLI not found')), file: '/tmp/g.md' }, + { model: 'codex', verdict: parseVerdict(_agySkipContent('no response before timeout')), file: '/tmp/c.md' }, + ]; + expect(effectiveReviews(reviews)).toHaveLength(0); + expect(allApprove(reviews)).toBe(false); + }); + + it('skip artifact is self-describing (names the lane, the non-run, and the remediation)', () => { const content = _agySkipContent('authentication required'); expect(content).toMatch(/Gemini lane skipped/); - expect(content).toMatch(/non-blocking/); + expect(content).toMatch(/No review was/); expect(content).toMatch(/antigravity\.google/); }); }); diff --git a/packages/codev/src/commands/porch/__tests__/parse-verdict.test.ts b/packages/codev/src/commands/porch/__tests__/parse-verdict.test.ts index a4f7eafb4..da27e40bb 100644 --- a/packages/codev/src/commands/porch/__tests__/parse-verdict.test.ts +++ b/packages/codev/src/commands/porch/__tests__/parse-verdict.test.ts @@ -115,9 +115,58 @@ VERDICT: APPROVE`; expect(parseVerdict(output)).toBe('APPROVE'); }); - it('returns COMMENT when no verdict is found in a long output (ran but no verdict)', () => { + it('returns SKIPPED when no verdict is found in a long output (ran but no verdict)', () => { + // Previously this fell back to COMMENT, which counts as an APPROVING + // reviewer — so a lane that went off-task or emitted garbage silently + // passed the gate (entriq #2467). SKIPPED = not a review: non-blocking + // for the other lanes, but never counted as approval. const output = `Review text that is long enough to pass the minimum length threshold for parsing. But it does not contain any VERDICT: line because the reviewer went off-task or didn't write one.`; + expect(parseVerdict(output)).toBe('SKIPPED'); + }); + + it('parses explicit SKIPPED verdict', () => { + const output = `--- +VERDICT: SKIPPED +SUMMARY: Gemini lane skipped — agy CLI not found +CONFIDENCE: LOW +--- + +The reviewer was skipped; no review was produced.`; + expect(parseVerdict(output)).toBe('SKIPPED'); + }); + + it('reinterprets a legacy skip stub (VERDICT: COMMENT + skip SUMMARY marker) as SKIPPED', () => { + const output = `--- +VERDICT: COMMENT +SUMMARY: Gemini lane skipped — authentication required (OAuth) +CONFIDENCE: LOW +--- + +The Gemini (Antigravity \`agy\`) reviewer was skipped: authentication required (OAuth). +This is a non-blocking skip; the remaining reviewers still apply.`; + expect(parseVerdict(output)).toBe('SKIPPED'); + }); + + it('does NOT reinterpret a real COMMENT review that merely mentions a skipped lane', () => { + const output = `Review text long enough for the threshold. Note: an unrelated CI lane skipped a job. + +--- +VERDICT: COMMENT +SUMMARY: Minor suggestions only +CONFIDENCE: MEDIUM +---`; expect(parseVerdict(output)).toBe('COMMENT'); }); + + it('does NOT reinterpret an explicit REQUEST_CHANGES even if a skip marker appears', () => { + const output = `Review text long enough for the threshold. + +--- +VERDICT: REQUEST_CHANGES +SUMMARY: Gemini lane skipped — but an earlier pass found real issues +CONFIDENCE: HIGH +---`; + expect(parseVerdict(output)).toBe('REQUEST_CHANGES'); + }); }); diff --git a/packages/codev/src/commands/porch/next.ts b/packages/codev/src/commands/porch/next.ts index 0223dcbe2..c5a5249d2 100644 --- a/packages/codev/src/commands/porch/next.ts +++ b/packages/codev/src/commands/porch/next.ts @@ -33,7 +33,7 @@ import { allPlanPhasesComplete, } from './plan.js'; import { buildPhasePrompt } from './prompts.js'; -import { parseVerdict, allApprove } from './verdict.js'; +import { parseVerdict, allApprove, effectiveReviews } from './verdict.js'; import { loadCheckOverrides } from './config.js'; import { loadConfig } from '../../lib/config.js'; import { getResolver, type ArtifactResolver } from './artifacts.js'; @@ -575,6 +575,32 @@ async function handleBuildVerify( }; } + // All review FILES are in — but a skip artifact is not a review. If every + // lane skipped (agy missing, no parseable VERDICT, ...), zero reviews + // actually happened and the gate must not pass on zero evidence. A rebuttal + // is the wrong remediation (there is no feedback to rebut); the fix is to + // make at least one lane produce a real review. See entriq #2467. + if (reviews.length > 0 && effectiveReviews(reviews).length === 0) { + const skippedInfo = reviews + .map(r => `- ${path.basename(r.file)} (SKIPPED — no review produced)`) + .join('\n'); + return { + status: 'tasks', + ...baseResponse, + tasks: [{ + subject: `All review lanes were skipped — a real review is required`, + activeForm: `Recovering skipped review lanes`, + description: + `Every consultation lane emitted a skip artifact or unparseable output, so no review actually happened:\n\n${skippedInfo}\n\n` + + `Fix at least one lane, then re-run its consultation so the file contains a real VERDICT (APPROVE / REQUEST_CHANGES / COMMENT):\n` + + `1. Delete the skip artifact file(s) listed above.\n` + + `2. Fix the lane (install/authenticate the CLI) and re-run the consult command, OR run the review manually with a working model and write its output (with a VERDICT line and provenance) to the same file path.\n` + + `3. Call \`porch next ${state.id}\` again.\n\n` + + `Do NOT hand-write a verdict without running a real review.`, + }], + }; + } + // All reviews in — parse verdicts and decide if (allApprove(reviews)) { // All approve — advance @@ -766,7 +792,7 @@ async function handleVerifyApproved( tasks: [{ subject: `Request human approval: ${gateName}`, activeForm: `Requesting ${gateName} approval`, - description: `All reviewers approved!\n\nReviewer verdicts:\n${formatVerdicts(reviews)}\n\nSTOP and wait for human approval.`, + description: `${effectiveReviews(reviews).length < reviews.length ? `All effective reviewers approved (${reviews.length - effectiveReviews(reviews).length} lane(s) SKIPPED — reduced review coverage)!` : 'All reviewers approved!'}\n\nReviewer verdicts:\n${formatVerdicts(reviews)}\n\nSTOP and wait for human approval.`, }], }; } @@ -850,6 +876,6 @@ async function handleOncePhase( */ function formatVerdicts(reviews: ReviewResult[]): string { return reviews - .map(r => ` ${r.model}: ${r.verdict}`) + .map(r => ` ${r.model}: ${r.verdict}${r.verdict === 'SKIPPED' ? ' (lane not run — no review produced)' : ''}`) .join('\n'); } diff --git a/packages/codev/src/commands/porch/prompts.ts b/packages/codev/src/commands/porch/prompts.ts index fd8502e76..e6c55ab15 100644 --- a/packages/codev/src/commands/porch/prompts.ts +++ b/packages/codev/src/commands/porch/prompts.ts @@ -150,7 +150,8 @@ function buildHistoryHeader(history: IterationRecord[], currentIteration: number lines.push('**Reviews:**'); for (const review of record.reviews) { const icon = review.verdict === 'APPROVE' ? '✓' : - review.verdict === 'COMMENT' ? '💬' : '✗'; + review.verdict === 'COMMENT' ? '💬' : + review.verdict === 'SKIPPED' ? '⊘' : '✗'; lines.push(`- ${review.model} (${icon} ${review.verdict}): \`${review.file}\``); } } diff --git a/packages/codev/src/commands/porch/types.ts b/packages/codev/src/commands/porch/types.ts index 2dfab5ec6..828e5301e 100644 --- a/packages/codev/src/commands/porch/types.ts +++ b/packages/codev/src/commands/porch/types.ts @@ -116,8 +116,13 @@ export interface PlanPhase { * * CONSULT_ERROR: Consultation failed (API key missing, network error, timeout) * Not a valid review - triggers retry, not REQUEST_CHANGES + * SKIPPED: The lane emitted a skip artifact (e.g. agy CLI missing) — no + * review was actually produced. Non-blocking for progression, + * but excluded from approval math so a skip can never count as + * an approving reviewer (issue: a skip stub with VERDICT: COMMENT + * masqueraded as a passing third lane). */ -export type Verdict = 'APPROVE' | 'REQUEST_CHANGES' | 'COMMENT' | 'CONSULT_ERROR'; +export type Verdict = 'APPROVE' | 'REQUEST_CHANGES' | 'COMMENT' | 'CONSULT_ERROR' | 'SKIPPED'; /** * Review result with file path diff --git a/packages/codev/src/commands/porch/verdict.ts b/packages/codev/src/commands/porch/verdict.ts index 8465638e4..0637bba3f 100644 --- a/packages/codev/src/commands/porch/verdict.ts +++ b/packages/codev/src/commands/porch/verdict.ts @@ -6,6 +6,14 @@ import type { Verdict, ReviewResult } from './types.js'; +/** + * Marker line emitted by consult's lane-skip artifacts (see agySkipContent in + * consult/index.ts): `SUMMARY: lane skipped — `. Detected + * independently of the VERDICT line so legacy skip stubs that shipped with + * `VERDICT: COMMENT` are still recognized as SKIPPED, not as a passing review. + */ +const SKIP_SUMMARY_MARKER = /^SUMMARY:.*\blane skipped\b/im; + /** * Parse verdict from consultation output. * @@ -13,13 +21,25 @@ import type { Verdict, ReviewResult } from './types.js'; * VERDICT: APPROVE * VERDICT: REQUEST_CHANGES * VERDICT: COMMENT + * VERDICT: SKIPPED * * Also handles markdown formatting like: * **VERDICT: APPROVE** * *VERDICT: APPROVE* * - * Safety: If no explicit verdict found (empty output, crash, malformed), - * defaults to REQUEST_CHANGES to prevent proceeding with unverified code. + * A lane that did not actually review parses as SKIPPED, never as a passing + * verdict. That covers three shapes: + * - an explicit `VERDICT: SKIPPED` artifact, + * - a legacy skip stub (`VERDICT: COMMENT` + the skip SUMMARY marker), + * - output with no VERDICT line at all (the consult ran but produced no + * parseable review). + * SKIPPED is non-blocking for progression but excluded from approval math — + * see allApprove. Previously the no-verdict fallback returned COMMENT, which + * counted as an approving reviewer; a skip stub nearly passed a defective + * money-critical spec through a gate that way (entriq #2467). + * + * Empty or near-empty output still means the consult itself failed — + * REQUEST_CHANGES, so a crashed lane gets retried rather than skipped. */ export function parseVerdict(output: string): Verdict { // Empty or very short output = something went wrong @@ -37,14 +57,28 @@ export function parseVerdict(output: string): Verdict { // Match "VERDICT: " but NOT template "VERDICT: [APPROVE | ...]" if (stripped.startsWith('VERDICT:') && !stripped.includes('[')) { const value = stripped.substring('VERDICT:'.length).trim(); + if (value.startsWith('SKIPPED')) return 'SKIPPED'; if (value.startsWith('REQUEST_CHANGES')) return 'REQUEST_CHANGES'; if (value.startsWith('APPROVE')) return 'APPROVE'; - if (value.startsWith('COMMENT')) return 'COMMENT'; + if (value.startsWith('COMMENT')) { + // Legacy skip stubs said `VERDICT: COMMENT`; the skip SUMMARY marker is + // authoritative for those. Only COMMENT is reinterpreted — an explicit + // APPROVE/REQUEST_CHANGES stays what it says. + return SKIP_SUMMARY_MARKER.test(output) ? 'SKIPPED' : 'COMMENT'; + } } } - // No valid VERDICT: line found but the consult ran — treat as COMMENT (non-blocking skip) - return 'COMMENT'; + // No valid VERDICT: line found — the lane produced no parseable review. + // SKIPPED: does not block the other reviewers, does not count as approval. + return 'SKIPPED'; +} + +/** + * The reviews that actually happened: everything except lane skips. + */ +export function effectiveReviews(reviews: ReviewResult[]): ReviewResult[] { + return reviews.filter(r => r.verdict !== 'SKIPPED'); } /** @@ -53,9 +87,15 @@ export function parseVerdict(output: string): Verdict { * Returns true only if ALL reviewers explicitly APPROVE. * COMMENT counts as approve (non-blocking feedback). * CONSULT_ERROR and REQUEST_CHANGES block approval. + * + * SKIPPED lanes are excluded: a skip neither approves nor blocks, so a phase + * still advances on the strength of the remaining reviewers (the Spec 778 + * progression guarantee). But if EVERY lane skipped, no review happened at + * all, and a verify gate must not pass on zero evidence. */ export function allApprove(reviews: ReviewResult[]): boolean { if (reviews.length === 0) return true; // No verification = auto-approve - return reviews.every(r => r.verdict === 'APPROVE' || r.verdict === 'COMMENT'); + const effective = effectiveReviews(reviews); + if (effective.length === 0) return false; // Every lane skipped = zero real reviews + return effective.every(r => r.verdict === 'APPROVE' || r.verdict === 'COMMENT'); } -