Fix #887: BUGFIX add pr gate (close v3.1.4 pr_ready timing gap)

codev-skeleton/protocols/bugfix/prompts/pr.md

@@ -89,8 +89,11 @@ Send a **single** notification that includes the PR link and each model's verdic
 afx send architect "PR #<number> ready for review (fixes issue #{{issue.number}}). CMAP: gemini=<APPROVE|REQUEST_CHANGES>, codex=<APPROVE|REQUEST_CHANGES>, claude=<APPROVE|REQUEST_CHANGES>"
 ```
 
-**This is the only notification you send.** After this, your work is done — the architect
-takes it from here (reviews, merges, cleans up).
+Then run `porch done <project-id>` to auto-request the `pr` gate. The PR surfaces
+in Needs Attention from this point; **STOP and wait** for the architect to call
+`porch approve <project-id> pr`. After gate approval, porch will emit a merge task
+(via the next `porch next` call) — follow it to merge the PR and advance to
+`verified`.
 
 ## Signals
 

codev-skeleton/protocols/bugfix/protocol.json

@@ -1,7 +1,7 @@
 {
   "$schema": "../../protocol-schema.json",
   "name": "bugfix",
-  "version": "1.1.0",
+  "version": "1.2.0",
   "description": "Lightweight protocol for minor bugfixes using GitHub Issues",
   "input": {
     "type": "github-issue",
@@ -80,6 +80,7 @@
         "parallel": true,
         "max_rounds": 1
       },
+      "gate": "pr",
       "transition": {
         "on_complete": null
       }

codev/projects/bugfix-887-porch-bugfix-should-have-a-pr-/status.yaml

@@ -0,0 +1,16 @@
+id: bugfix-887
+title: porch-bugfix-should-have-a-pr-
+protocol: bugfix
+phase: pr
+plan_phases: []
+current_plan_phase: null
+gates:
+  pr:
+    status: pending
+    requested_at: '2026-05-27T14:09:02.204Z'
+iteration: 1
+build_complete: false
+history: []
+started_at: '2026-05-27T13:41:02.865Z'
+updated_at: '2026-05-27T14:09:02.204Z'
+pr_ready_for_human: true

codev/protocols/bugfix/prompts/pr.md

@@ -89,8 +89,11 @@ Send a **single** notification that includes the PR link and each model's verdic
 afx send architect "PR #<number> ready for review (fixes issue #{{issue.number}}). CMAP: gemini=<APPROVE|REQUEST_CHANGES>, codex=<APPROVE|REQUEST_CHANGES>, claude=<APPROVE|REQUEST_CHANGES>"
 ```
 
-**This is the only notification you send.** After this, your work is done — the architect
-takes it from here (reviews, merges, cleans up).
+Then run `porch done <project-id>` to auto-request the `pr` gate. The PR surfaces
+in Needs Attention from this point; **STOP and wait** for the architect to call
+`porch approve <project-id> pr`. After gate approval, porch will emit a merge task
+(via the next `porch next` call) — follow it to merge the PR and advance to
+`verified`.
 
 ## Signals
 

codev/protocols/bugfix/protocol.json

@@ -1,7 +1,7 @@
 {
   "$schema": "../../protocol-schema.json",
   "name": "bugfix",
-  "version": "1.1.0",
+  "version": "1.2.0",
   "description": "Lightweight protocol for minor bugfixes using GitHub Issues",
   "input": {
     "type": "github-issue",
@@ -82,6 +82,7 @@
         "parallel": true,
         "max_rounds": 1
       },
+      "gate": "pr",
       "transition": {
         "on_complete": null
       }

codev/state/bugfix-887_thread.md

@@ -0,0 +1,56 @@
+# bugfix-887 thread
+
+## Mission
+Close the v3.1.4 BUGFIX timing gap for the canonical `pr_ready_for_human` signal by giving BUGFIX a `pr` gate (same shape as AIR). Issue body laid out the fix precisely.
+
+## Diagnosis (handed to me — confirmed)
+v3.1.4 PR #874 wired BUGFIX's `pr_ready_for_human=true` to the `advanceProtocolPhase` terminal-exit setter (`index.ts:453` pre-fix) because BUGFIX had no `pr` gate. That setter only fires when the builder calls `porch done` post-merge — by which point the human has already acted. BUGFIX PRs sat at `phase: pr, pr_ready_for_human: false` indefinitely, never surfacing in Needs Attention. Verified the architect's claim by reading the existing pr-ready-872 tests + the corresponding code path.
+
+## Changes (single PR, < 300 LOC)
+
+### Protocol JSON (both copies)
+- `codev/protocols/bugfix/protocol.json` + skeleton — bumped to `1.2.0`, added `"gate": "pr"` to pr phase
+
+### Porch code
+- `index.ts:advanceProtocolPhase` — removed the BUGFIX-no-gate snapshot (`advancingFromPrPhase`) and the two conditional setters. Function now just advances; no PR-readiness logic since all PR phases have gates.
+- `index.ts` `done()` gate-request setter — simplified `gate === 'pr' || isPrCreatingPhase(...)` to `gate === 'pr'` (the second clause is now redundant).
+- `next.ts:handleVerifyApproved` — same simplification on the gate-request path; dropped `advancingFromPrPhase` from the no-gate-advance fallback.
+- `next.ts` BUGFIX special-case at terminal state (the "architect takes over" return) — removed. BUGFIX now falls through to the AIR-shaped merge-instructions path.
+- `protocol.ts:isPrCreatingPhase` — collapsed to `phase?.gate === 'pr'`, dropped `hasPrConsultation` field and its computation in `normalizePhase`. JSDoc updated.
+- `types.ts:ProtocolPhase` — removed `hasPrConsultation` field. Updated `pr_ready_for_human` JSDoc.
+
+### Prompts
+- `codev[-skeleton]/protocols/bugfix/prompts/pr.md` — replaced "your work is done, architect takes over" with the new gate-driven flow: porch done → wait for `porch approve <id> pr` → follow porch's emitted merge task.
+
+### Tests
+- `pr-ready-872.test.ts` — bumped BUGFIX fixture to version 1.2.0 with `gate: "pr"`. Rewrote BUGFIX cases to test the gate-request path (matching AIR). Updated classifier describe block to reflect the now-single-marker invariant. Kept the RESEARCH false-classification regression tests intact.
+- `next.test.ts` — renamed/rewrote the "no merge instruction for bugfix" test to "returns merge task for completed bugfix protocol (matches AIR post-#887)". Updated fixture to include `gate: "pr"` and `gates: { pr: approved }` on the terminal state.
+
+### Docs
+- `docs/releases/v3.1.4-jacobean.md` — prepended a "Superseded in v3.1.5 by issue #887" callout to the section that introduced the BUGFIX-specific terminal-exit set-point. The `derivePrReady` BUGFIX-fallback in `overview.ts` is kept intact for in-flight v3.1.4 projects (flagged for v3.2+ removal per the issue body).
+
+## Out of scope (per issue body)
+- Removing `derivePrReady` BUGFIX-fallback in `overview.ts` — left as graceful-degradation shim for in-flight v3.1.4 BUGFIX builders.
+- Re-examining AIR's terminal-exit set-point — AIR uses the gate-request path correctly, no change needed.
+- `codev/protocols/bugfix/protocol.md` long-form workflow doc — out of scope for this issue (still describes manual "afx send LGTM merge it" flow that's now redundant with the gate, but the issue scoped this strictly).
+
+## Test results
+- pr-ready-872.test.ts: 15/15 pass
+- All porch tests: 334/334 pass
+- overview.test.ts: 148/148 pass
+- TypeScript: clean
+
+## Status
+Code complete. PR #888 opened, CMAP-3 verdicts in:
+- codex: APPROVE / HIGH (explicit)
+- claude: APPROVE / HIGH (explicit)
+- gemini: positive PR_SUMMARY endorsement (verdict line truncated in capture; stats db confirms exit 0, no concerns flagged)
+
+Codex flagged one minor nit — stale "BUGFIX has no gate" comment near `approve()` in index.ts. Addressed in commit de92d794.
+
+The fix is end-to-end verified in this very builder: `porch done` post-CMAP requested the `pr` gate and set `pr_ready_for_human: true` in status.yaml. The signal fires at the correct moment now. Architect notified, awaiting `porch approve bugfix-887 pr`.
+
+## After gate approval (planned)
+1. Merge PR #888 via `gh pr merge 888 --merge`
+2. Run `porch done bugfix-887` → advances pr → verified
+3. Notify architect "PR #888 merged. Ready for cleanup."

docs/releases/v3.1.4-jacobean.md

@@ -30,6 +30,8 @@ A new `force_advanced` field on `ProjectState` records when the safety ceiling f
 
 ## Dashboard: canonical `pr_ready_for_human` signal closes BUGFIX gap (PR #874, fixes #872)
 
+> **Superseded in v3.1.5 by issue #887** — the BUGFIX-specific terminal-exit set-point landed here is timing-wrong (fires only when the builder calls `porch done` post-merge, by which point the human reviewer has already acted). v3.1.5 supersedes this by giving BUGFIX a `pr` gate (same shape as AIR), so the canonical signal fires via the gate-request setter — at the *right* moment, when CMAP finishes and the PR is waiting for a reviewer. The fallback path in `derivePrReady` for `phase === 'verified' && protocol === 'bugfix'` remains intact in v3.1.5 as a graceful-degradation shim for in-flight v3.1.4 BUGFIX projects; flagged for removal in v3.2+.
+
 The v3.1.3 Needs Attention filter (introduced in PR #845) gated PR inclusion on `b.blocked === 'PR review'` — the gate label for porch's `pr` gate, which SPIR / ASPIR / PIR / AIR all carry but **BUGFIX does not**. BUGFIX's `pr` phase is terminal with no gate, so the builder transitions cleanly from `pr` to `verified` after CMAP without ever blocking. Result: **every open BUGFIX PR was silently invisible in Needs Attention since v3.1.3 shipped**. Found by reviewing an in-flight BUGFIX PR that the dashboard wasn't surfacing.
 
 The fix moves the "is this PR ready for human review" decision from a protocol-specific derivation to a canonical state-machine signal.

packages/codev/src/commands/porch/__tests__/next.test.ts

@@ -851,13 +851,17 @@ describe('porch next', () => {
   });
 
   // --------------------------------------------------------------------------
-  // Bugfix complete — no merge task, no second notification (#319)
+  // Bugfix complete — merge task emitted post-gate-approval (#887)
+  // Pre-#887 BUGFIX had no `pr` gate and stopped at `verified` with an
+  // "architect takes it from here" summary. After #887 BUGFIX carries the
+  // same `gate: "pr"` as AIR; the builder merges (post-gate-approval) via
+  // the standard merge-task path, mirroring AIR exactly.
   // --------------------------------------------------------------------------
 
-  it('returns no tasks for completed bugfix protocol (no merge instruction)', async () => {
+  it('returns merge task for completed bugfix protocol (matches AIR post-#887)', async () => {
     const bugfixProtocol = {
       name: 'bugfix',
-      version: '1.1.0',
+      version: '1.2.0',
       phases: [
         {
           id: 'investigate',
@@ -875,6 +879,7 @@ describe('porch next', () => {
           id: 'pr',
           name: 'Create PR',
           type: 'once',
+          gate: 'pr',
           transition: { on_complete: null },
         },
       ],
@@ -885,10 +890,17 @@ describe('porch next', () => {
       id: 'builder-bugfix-42',
       title: 'login-spaces',
       protocol: 'bugfix',
-      phase: 'complete',
+      phase: 'verified',
       plan_phases: [],
       current_plan_phase: null,
-      gates: {},
+      // Terminal state retains the approved pr gate (no longer `{}`)
+      gates: {
+        pr: {
+          status: 'approved',
+          requested_at: '2026-05-27T00:00:00.000Z',
+          approved_at: '2026-05-27T00:30:00.000Z',
+        },
+      },
       iteration: 1,
       build_complete: false,
       history: [],
@@ -900,10 +912,9 @@ describe('porch next', () => {
     const result = await next(testDir, 'builder-bugfix-42');
 
     expect(result.status).toBe('complete');
-    // No manual commit-status task — writeStateAndCommit handles it automatically
-    // Must NOT contain merge instructions — bugfix builder doesn't merge
-    expect(result.summary).not.toContain('Merge');
-    expect(result.summary).toContain('architect');
+    expect(result.tasks!.length).toBe(1);
+    expect(result.tasks![0].subject).toContain('Merge');
+    expect(result.tasks![0].description).toContain('pr-merge');
   });
 
   it('returns merge task for completed non-bugfix protocol (no manual commit-status task)', async () => {

packages/codev/src/commands/porch/__tests__/pr-ready-872.test.ts

@@ -188,7 +188,7 @@ const airProtocol = {
 
 const bugfixProtocol = {
   name: 'bugfix',
-  version: '1.0.0',
+  version: '1.2.0',
   phases: [
     {
       id: 'investigate',
@@ -207,6 +207,7 @@ const bugfixProtocol = {
       name: 'Create PR',
       type: 'once',
       consultation: { on: 'review', models: ['gemini', 'codex'], type: 'impl' },
+      gate: 'pr',
       transition: { on_complete: null },
     },
   ],
@@ -231,14 +232,14 @@ describe('Issue #872 — pr_ready_for_human lifecycle', () => {
   // isPrCreatingPhase classifier — must NOT overmatch RESEARCH (iter-2 fix)
   // --------------------------------------------------------------------------
 
-  describe('isPrCreatingPhase classifier (iter-2: narrow consultation.on === "review")', () => {
-    it('returns true for AIR pr (consultation.on === "review")', () => {
+  describe('isPrCreatingPhase classifier (single marker: gate === "pr")', () => {
+    it('returns true for AIR pr (once-phase, gate=pr)', () => {
       setupProtocol(testDir, 'air', airProtocol);
       const protocol = loadProtocol(testDir, 'air');
       expect(isPrCreatingPhase(protocol, 'pr')).toBe(true);
     });
 
-    it('returns true for BUGFIX pr (consultation.on === "review")', () => {
+    it('returns true for BUGFIX pr (once-phase, gate=pr — #887 normalized BUGFIX onto AIR shape)', () => {
       setupProtocol(testDir, 'bugfix', bugfixProtocol);
       const protocol = loadProtocol(testDir, 'bugfix');
       expect(isPrCreatingPhase(protocol, 'pr')).toBe(true);
@@ -250,11 +251,12 @@ describe('Issue #872 — pr_ready_for_human lifecycle', () => {
       expect(isPrCreatingPhase(loadProtocol(testDir, 'spir'), 'review')).toBe(true);
     });
 
-    it('returns FALSE for RESEARCH investigate (consultation present but on is not "review")', () => {
-      // This is the bug architect-side CMAP caught in iter-1: the iter-1
-      // classifier matched bare `consultation` presence, which would have
-      // misclassified RESEARCH's investigation phase as PR-creating and leaked
-      // pr_ready_for_human: true into research state.
+    it('returns FALSE for RESEARCH investigate (consultation present but no pr gate)', () => {
+      // Architect-side CMAP for #872 iter-1 caught a precursor of this case:
+      // the iter-1 classifier matched bare `consultation` presence, which would
+      // have misclassified RESEARCH's investigation phase as PR-creating. After
+      // #887 the classifier checks `gate === 'pr'` alone — RESEARCH carries no
+      // `pr` gate, so the gap is closed by construction.
       setupProtocol(testDir, 'research', researchShapedProtocol);
       const protocol = loadProtocol(testDir, 'research');
       expect(isPrCreatingPhase(protocol, 'investigate')).toBe(false);
@@ -263,8 +265,8 @@ describe('Issue #872 — pr_ready_for_human lifecycle', () => {
     it('returns FALSE for RESEARCH critique (consultation + gate, but gate is not "pr")', () => {
       setupProtocol(testDir, 'research', researchShapedProtocol);
       const protocol = loadProtocol(testDir, 'research');
-      // Critique has a `research-complete` gate, NOT `pr`, and its consultation
-      // block has no `on: "review"`. Both classifier markers miss it.
+      // Critique has a `research-complete` gate, NOT `pr`. The classifier
+      // matches on `gate === 'pr'` only, so this phase is correctly excluded.
       expect(isPrCreatingPhase(protocol, 'critique')).toBe(false);
     });
 
@@ -372,21 +374,62 @@ describe('Issue #872 — pr_ready_for_human lifecycle', () => {
   });
 
   // --------------------------------------------------------------------------
-  // BUGFIX: once-phase pr, NO gate (the regression that motivated #872)
+  // BUGFIX: once-phase pr with gate=pr (post-#887 — same shape as AIR).
+  // The original #872 set-point (`advanceProtocolPhase` terminal-exit) is now
+  // unreachable: BUGFIX no longer transitions to `verified` until after the
+  // gate-request → approve cycle. These tests pin the gate-request path so
+  // future refactors can't silently regress the timing.
   // --------------------------------------------------------------------------
 
-  describe('BUGFIX pr (once-phase, no gate — the #872 regression case)', () => {
-    it('sets pr_ready_for_human=true when done advances pr → verified', async () => {
+  describe('BUGFIX pr (once-phase, gate=pr — #887)', () => {
+    it('sets pr_ready_for_human=true when done auto-requests the pr gate (matches AIR)', async () => {
       setupProtocol(testDir, 'bugfix', bugfixProtocol);
 
-      const state = makeState({ id: 'bugfix-0001', protocol: 'bugfix', phase: 'pr', gates: {} });
+      const state = makeState({
+        id: 'bugfix-0001',
+        protocol: 'bugfix',
+        phase: 'pr',
+        gates: { pr: { status: 'pending' as const } },
+      });
       setupState(testDir, state);
 
       await done(testDir, 'bugfix-0001');
 
       const after = readStateFor(testDir, state);
-      expect(after.phase).toBe('verified');
+      // Builder stays on `pr` with the gate pending until the architect approves.
+      expect(after.phase).toBe('pr');
       expect(after.pr_ready_for_human).toBe(true);
+      expect(after.gates['pr']?.status).toBe('pending');
+      expect(after.gates['pr']?.requested_at).toBeTruthy();
+    });
+
+    it('advances pr → verified after gate approval + done, leaving gates.pr approved', async () => {
+      setupProtocol(testDir, 'bugfix', bugfixProtocol);
+
+      // Builder ran CMAP + porch done, architect approved the gate.
+      const state = makeState({
+        id: 'bugfix-0001',
+        protocol: 'bugfix',
+        phase: 'pr',
+        pr_ready_for_human: true,
+        gates: {
+          pr: {
+            status: 'approved' as const,
+            requested_at: new Date(Date.now() - 60_000).toISOString(),
+            approved_at: new Date().toISOString(),
+          },
+        },
+      });
+      setupState(testDir, state);
+
+      await done(testDir, 'bugfix-0001');
+
+      const after = readStateFor(testDir, state);
+      expect(after.phase).toBe('verified');
+      // The approved gate is preserved on the terminal state (no longer `{}`).
+      expect(after.gates['pr']?.status).toBe('approved');
+      expect(after.gates['pr']?.requested_at).toBeTruthy();
+      expect(after.gates['pr']?.approved_at).toBeTruthy();
     });
 
     it('does NOT set pr_ready_for_human=true when done advances an earlier (non-pr) phase', async () => {