[Bugfix #1166] Consult skip stubs and no-verdict output parse as SKIPPED, never as approval

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();

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');
 }
 

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 () => {

packages/codev/src/commands/porch/__tests__/agy-skip-progression.test.ts

@@ -1,39 +1,48 @@
 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)',
     'no response before timeout',
   ];
 
   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/);
   });
 });

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');
+  });
 });

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');
 }

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}\``);
       }
     }

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