diff --git a/codev/projects/bugfix-113-porch-verify-approval-cannot-p/status.yaml b/codev/projects/bugfix-113-porch-verify-approval-cannot-p/status.yaml new file mode 100644 index 000000000..79f2047ed --- /dev/null +++ b/codev/projects/bugfix-113-porch-verify-approval-cannot-p/status.yaml @@ -0,0 +1,22 @@ +id: bugfix-113 +title: porch-verify-approval-cannot-p +protocol: bugfix +phase: pr +plan_phases: [] +current_plan_phase: null +gates: + pr: + status: approved + requested_at: '2026-08-24T23:03:47.476Z' + approved_at: '2026-08-24T23:19:14.798Z' +iteration: 1 +build_complete: false +history: [] +started_at: '2026-08-24T22:29:49.851Z' +updated_at: '2026-08-24T23:19:14.799Z' +pr_history: + - phase: pr + pr_number: 137 + branch: builder/bugfix-113 + created_at: '2026-08-24T22:58:03.624Z' +pr_ready_for_human: false diff --git a/codev/state/bugfix-113_thread.md b/codev/state/bugfix-113_thread.md new file mode 100644 index 000000000..54886b17c --- /dev/null +++ b/codev/state/bugfix-113_thread.md @@ -0,0 +1,21 @@ +# bugfix-113 thread + +Investigate (2026-08-24). + +Reproduced. `porch check 83` from this worktree fails `pr_exists` in 0.6s. Same command from main passes only because `findStatusPath` hits `.builders/air-106`, whose unrelated merged PR #132 satisfies the check. + +Project 83 is still in `review`. `pr` is approved. PR #104 is MERGED. `verify-approval` is pending. The builder never ran `porch done` after the pr gate, so verify was never entered. + +`porch approve 83 verify-approval` runs checks for `state.phase` (review), not for the phase that owns the gate. Review's first check is `pr_exists`, which asks `git branch --show-current` in whichever worktree owns the status.yaml. After merge that is almost never `builder/spir-83`. + +The issue title's premise is wrong. `pr_exists` already uses `--state all`, so a merged PR matches (#568 / #16 / upstream #1331). The real cause is the `--head` argument: `git branch --show-current` runs in whichever worktree `findStatusPath` returns, and after merge that is almost never the PR head. The search is for the wrong head, not the wrong state. Against `CODEV_BRANCH_NAME=builder/spir-83` the script returns true. + +Verify phase has no checks. Scope is small: approve the named gate's phase checks, and if verify-approval is requested while still in review with `pr` already approved, enter verify first so the existing auto-advance can reach `verified`. + +Fix: `approve()` enters verify when verify-approval is requested from review with `pr` already approved. Review's `pr_exists` no longer runs. Regression test fails without the block (process.exit from `false` pr_exists) and passes with it. + +PR #137: https://github.com/pseudoseed/codev/pull/137 + +CMAP: gemini skipped (agy exit 1, quota). Substitute opencode=APPROVE. codex=COMMENT (branch trails main by a docs merge, no code change). claude=APPROVE. No REQUEST_CHANGES. + +Claude noted a residual: `porch done` / `porch check` still fail review's `pr_exists` after merge when cwd is not the PR head. Left as a follow-up; this bugfix unblocks verify-approval. diff --git a/packages/codev/src/commands/porch/__tests__/bugfix-113-verify-approval-merged-pr.test.ts b/packages/codev/src/commands/porch/__tests__/bugfix-113-verify-approval-merged-pr.test.ts new file mode 100644 index 000000000..eeacc2dd2 --- /dev/null +++ b/packages/codev/src/commands/porch/__tests__/bugfix-113-verify-approval-merged-pr.test.ts @@ -0,0 +1,156 @@ +/** + * Regression test for issue #113: porch approve verify-approval cannot pass + * after a normal merge because it re-runs review's pr_exists. + * + * Reproduction: + * 1. SPIR project is still in review. The pr gate is already approved. + * 2. The PR is merged (closed). That is how you get to verify-approval. + * 3. `porch approve verify-approval` runs checks for state.phase + * (review), whose first check is pr_exists. + * 4. pr_exists keys off `git branch --show-current` in whichever worktree + * findStatusPath returns. After merge that is almost never the PR head, + * so the check fails and the gate cannot be approved. + * + * A test that stops at the pr gate cannot catch this. + */ + +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import * as fs from 'node:fs'; +import * as path from 'node:path'; +import { tmpdir } from 'node:os'; +import { approve } from '../index.js'; +import { writeState, getStatusPath, readState } from '../state.js'; +import type { ProjectState } from '../types.js'; + +function createTestDir(): string { + const dir = path.join(tmpdir(), `porch-113-${Date.now()}-${Math.random().toString(36).slice(2, 8)}`); + fs.mkdirSync(dir, { recursive: true }); + return dir; +} + +function setupProtocol(testDir: string, protocol: object): void { + const protocolDir = path.join(testDir, 'codev', 'protocols', 'spir'); + fs.mkdirSync(protocolDir, { recursive: true }); + fs.writeFileSync(path.join(protocolDir, 'protocol.json'), JSON.stringify(protocol, null, 2)); +} + +function setupState(testDir: string, state: ProjectState): string { + const statusPath = getStatusPath(testDir, state.id, state.title); + fs.mkdirSync(path.dirname(statusPath), { recursive: true }); + writeState(statusPath, state); + return statusPath; +} + +function makeState(overrides: Partial = {}): ProjectState { + return { + id: '83', + title: 'merged-pr-project', + protocol: 'spir', + phase: 'review', + plan_phases: [], + current_plan_phase: null, + gates: { + 'spec-approval': { status: 'approved', approved_at: new Date().toISOString() }, + 'plan-approval': { status: 'approved', approved_at: new Date().toISOString() }, + pr: { status: 'approved', approved_at: new Date().toISOString() }, + 'verify-approval': { status: 'pending' }, + }, + iteration: 1, + build_complete: true, + history: [], + started_at: new Date().toISOString(), + updated_at: new Date().toISOString(), + ...overrides, + }; +} + +// Review's pr_exists is a guaranteed fail. Without the fix, approve +// verify-approval runs this check and cannot pass. With the fix, approve +// enters verify first and never runs it. +const spirProtocol = { + name: 'spir', + version: '1.0.0', + phases: [ + { + id: 'review', + name: 'Review', + type: 'once', + checks: { + pr_exists: { + command: 'false', + description: 'Would fail after merge when the current branch is not the PR head', + }, + }, + gate: 'pr', + next: 'verify', + }, + { + id: 'verify', + name: 'Verify', + type: 'once', + gate: 'verify-approval', + next: null, + }, + ], +}; + +describe('bugfix #113 — verify-approval after merged PR', () => { + let testDir: string; + let exitSpy: ReturnType; + let logSpy: ReturnType; + + beforeEach(() => { + testDir = createTestDir(); + setupProtocol(testDir, spirProtocol); + exitSpy = vi.spyOn(process, 'exit').mockImplementation((code?: number) => { + throw new Error(`process.exit(${code})`); + }); + logSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); + }); + + afterEach(() => { + fs.rmSync(testDir, { recursive: true, force: true }); + exitSpy.mockRestore(); + logSpy.mockRestore(); + }); + + it('approve verify-approval succeeds after pr is approved even when pr_exists would fail', async () => { + const statusPath = setupState(testDir, makeState()); + + await approve(testDir, '83', 'verify-approval', true); + + const updated = readState(statusPath); + expect(updated.gates['verify-approval'].status).toBe('approved'); + expect(updated.gates['verify-approval'].approved_at).toBeDefined(); + expect(updated.phase).toBe('verified'); + }); + + it('approve verify-approval refuses when the pr gate is not approved', async () => { + const statusPath = setupState(testDir, makeState({ + gates: { + 'spec-approval': { status: 'approved', approved_at: new Date().toISOString() }, + 'plan-approval': { status: 'approved', approved_at: new Date().toISOString() }, + pr: { status: 'pending' }, + 'verify-approval': { status: 'pending' }, + }, + })); + + await expect(approve(testDir, '83', 'verify-approval', true)).rejects.toThrow( + /pr gate must be approved first/, + ); + + const updated = readState(statusPath); + expect(updated.gates['verify-approval'].status).toBe('pending'); + expect(updated.phase).toBe('review'); + }); + + it('approve verify-approval still works when already in verify', async () => { + const statusPath = setupState(testDir, makeState({ phase: 'verify' })); + + await approve(testDir, '83', 'verify-approval', true); + + const updated = readState(statusPath); + expect(updated.gates['verify-approval'].status).toBe('approved'); + expect(updated.phase).toBe('verified'); + }); +}); diff --git a/packages/codev/src/commands/porch/index.ts b/packages/codev/src/commands/porch/index.ts index 4d8f67c91..f8d5351eb 100644 --- a/packages/codev/src/commands/porch/index.ts +++ b/packages/codev/src/commands/porch/index.ts @@ -874,6 +874,27 @@ export async function approve( // Run phase checks before approving const protocol = loadProtocol(workspaceRoot, state.protocol); + + // Issue #113: verify-approval is the post-merge gate. After a normal merge + // the project is still in review (porch done after the pr gate is a separate + // step). Approving this gate must not re-run review's pr_exists — that check + // keys off git branch --show-current in whichever worktree findStatusPath + // returns, which after merge is almost never the PR head. Enter verify first + // so the checks below are the verify phase's (none) and the existing + // auto-advance can reach verified. + if (gateName === 'verify-approval' && state.phase !== 'verify') { + if (state.phase === 'review' && state.gates['pr']?.status === 'approved') { + state.phase = 'verify'; + state.build_complete = true; + await writeStateAndCommit(statusPath, state, `chore(porch): ${state.id} verify phase-transition (verify-approval)`); + } else { + throw new Error( + `Cannot approve verify-approval from phase '${state.phase}'. ` + + `The pr gate must be approved first.`, + ); + } + } + const overrides = loadCheckOverrides(workspaceRoot, state.protocol); const phaseConfig = getPhaseConfig(protocol, state.phase); const phaseCheckNames = phaseConfig?.checks ?? [];