diff --git a/codev-skeleton/protocols/bugfix/prompts/pr.md b/codev-skeleton/protocols/bugfix/prompts/pr.md index 71ed0f2ec..d70d8dfdf 100644 --- a/codev-skeleton/protocols/bugfix/prompts/pr.md +++ b/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 # ready for review (fixes issue #{{issue.number}}). CMAP: gemini=, codex=, claude=" ``` -**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 ` 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 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 diff --git a/codev-skeleton/protocols/bugfix/protocol.json b/codev-skeleton/protocols/bugfix/protocol.json index d2b107ae0..0abd36ee5 100644 --- a/codev-skeleton/protocols/bugfix/protocol.json +++ b/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 } diff --git a/codev/projects/bugfix-887-porch-bugfix-should-have-a-pr-/status.yaml b/codev/projects/bugfix-887-porch-bugfix-should-have-a-pr-/status.yaml new file mode 100644 index 000000000..0cbde6de4 --- /dev/null +++ b/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 diff --git a/codev/protocols/bugfix/prompts/pr.md b/codev/protocols/bugfix/prompts/pr.md index 71ed0f2ec..d70d8dfdf 100644 --- a/codev/protocols/bugfix/prompts/pr.md +++ b/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 # ready for review (fixes issue #{{issue.number}}). CMAP: gemini=, codex=, claude=" ``` -**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 ` 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 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 diff --git a/codev/protocols/bugfix/protocol.json b/codev/protocols/bugfix/protocol.json index c6e2d45c8..cac575cbd 100644 --- a/codev/protocols/bugfix/protocol.json +++ b/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 } diff --git a/codev/state/bugfix-887_thread.md b/codev/state/bugfix-887_thread.md new file mode 100644 index 000000000..258d7fd68 --- /dev/null +++ b/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 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." diff --git a/docs/releases/v3.1.4-jacobean.md b/docs/releases/v3.1.4-jacobean.md index d09d71aea..17dadabdd 100644 --- a/docs/releases/v3.1.4-jacobean.md +++ b/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. diff --git a/packages/codev/src/commands/porch/__tests__/next.test.ts b/packages/codev/src/commands/porch/__tests__/next.test.ts index e5651c3ae..877500f8a 100644 --- a/packages/codev/src/commands/porch/__tests__/next.test.ts +++ b/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 () => { diff --git a/packages/codev/src/commands/porch/__tests__/pr-ready-872.test.ts b/packages/codev/src/commands/porch/__tests__/pr-ready-872.test.ts index 6cd59cc3e..a5cf67047 100644 --- a/packages/codev/src/commands/porch/__tests__/pr-ready-872.test.ts +++ b/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 () => { diff --git a/packages/codev/src/commands/porch/index.ts b/packages/codev/src/commands/porch/index.ts index eb4de0680..375091c7b 100644 --- a/packages/codev/src/commands/porch/index.ts +++ b/packages/codev/src/commands/porch/index.ts @@ -30,7 +30,6 @@ import { getPhaseGate, isPhased, isBuildVerify, - isPrCreatingPhase, getVerifyConfig, } from './protocol.js'; import { @@ -484,11 +483,11 @@ export async function done(workspaceRoot: string, projectId: string, resolver?: } if (!state.gates[gate].requested_at) { state.gates[gate].requested_at = new Date().toISOString(); - // Issue #872: AIR pr (once-phase, gate=pr) reaches the human-review + // Issue #872: AIR / BUGFIX pr (once-phase, gate=pr) reaches the human-review // bottleneck here — done auto-requests the `pr` gate. Set the canonical // pr-ready signal in the same write so consumers don't have to wait for // a subsequent state mutation to learn the PR is ready for a reviewer. - if (gate === 'pr' || isPrCreatingPhase(protocol, state.phase)) { + if (gate === 'pr') { state.pr_ready_for_human = true; } await writeStateAndCommit(statusPath, state, `chore(porch): ${state.id} ${gate} gate-requested`); @@ -518,16 +517,10 @@ export async function done(workspaceRoot: string, projectId: string, resolver?: } async function advanceProtocolPhase(workspaceRoot: string, state: ProjectState, protocol: Protocol, statusPath: string, resolver?: ArtifactResolver): Promise { - // Issue #872: BUGFIX runs CMAP on a once-phase with no gate. Calling `done` - // on its `pr` phase advances directly to `verified`, and the human is now - // the reviewer bottleneck. Snapshot this before mutating state.phase so the - // pr-ready signal fires in the same write that records the advance. - const advancingFromPrPhase = isPrCreatingPhase(protocol, state.phase); const nextPhase = getNextPhase(protocol, state.phase); if (!nextPhase) { state.phase = 'verified'; - if (advancingFromPrPhase) state.pr_ready_for_human = true; await writeStateAndCommit(statusPath, state, `chore(porch): ${state.id} protocol complete`); console.log(''); console.log(chalk.green.bold('🎉 PROTOCOL COMPLETE')); @@ -538,7 +531,6 @@ async function advanceProtocolPhase(workspaceRoot: string, state: ProjectState, state.phase = nextPhase.id; state.build_complete = false; state.iteration = 1; - if (advancingFromPrPhase) state.pr_ready_for_human = true; // If entering a phased phase (implement), extract plan phases if (isPhased(protocol, nextPhase.id)) { @@ -743,11 +735,12 @@ export async function approve( state.gates[gateName].status = 'approved'; state.gates[gateName].approved_at = new Date().toISOString(); - // Issue #872: human approving the pr gate means they've acted — the PR is no - // longer waiting on a reviewer, so the pr-ready signal must go false in the - // same write that records the approval. Clearing on gate-approval covers all - // four protocols with an explicit `pr` gate; BUGFIX has no gate and stays - // true until the architect cleans up the worktree (cleanup deletes status). + // Issue #872 / #887: human approving the pr gate means they've acted — the + // PR is no longer waiting on a reviewer, so the pr-ready signal must go + // false in the same write that records the approval. Clearing on + // gate-approval covers all five PR-emitting protocols (SPIR / ASPIR / PIR / + // AIR / BUGFIX), each of which now carries `gate: "pr"` on its PR-creating + // phase — #887 closed the BUGFIX special case. if (gateName === 'pr') { state.pr_ready_for_human = false; } diff --git a/packages/codev/src/commands/porch/next.ts b/packages/codev/src/commands/porch/next.ts index 667ee5aea..0223dcbe2 100644 --- a/packages/codev/src/commands/porch/next.ts +++ b/packages/codev/src/commands/porch/next.ts @@ -20,7 +20,6 @@ import { getPhaseGate, isPhased, isBuildVerify, - isPrCreatingPhase, getBuildConfig, getVerifyConfig, getOnCompleteConfig, @@ -248,16 +247,6 @@ export async function next(workspaceRoot: string, projectId: string): Promise p.id === 'verify'); @@ -762,8 +751,9 @@ async function handleVerifyApproved( state.history = []; // Issue #872: when CMAP completes for the PR-creating phase, expose a // canonical `pr_ready_for_human=true` so consumers don't have to derive - // it from the protocol-specific gate shape. - if (gateName === 'pr' || isPrCreatingPhase(protocol, state.phase)) { + // it from the protocol-specific gate shape. All five PR-emitting protocols + // carry `gate: "pr"` on their PR-creating phase (#887 closed the BUGFIX gap). + if (gateName === 'pr') { state.pr_ready_for_human = true; } await writeStateAndCommit(statusPath, state, `chore(porch): ${state.id} ${gateName} gate-requested`); @@ -781,14 +771,10 @@ async function handleVerifyApproved( }; } - // No gate — advance to next phase directly. If we're advancing out of a - // PR-creating phase (BUGFIX: pr → verified after CMAP), set the canonical - // pr-ready signal before the write so consumers pick it up immediately. - const advancingFromPrPhase = isPrCreatingPhase(protocol, state.phase); + // No gate — advance to next phase directly. const nextPhase = getNextPhase(protocol, state.phase); if (!nextPhase) { state.phase = 'verified'; - if (advancingFromPrPhase) state.pr_ready_for_human = true; await writeStateAndCommit(statusPath, state, `chore(porch): ${state.id} protocol complete`); return next(workspaceRoot, projectId); } @@ -797,7 +783,6 @@ async function handleVerifyApproved( state.iteration = 1; state.build_complete = false; state.history = []; - if (advancingFromPrPhase) state.pr_ready_for_human = true; await writeStateAndCommit(statusPath, state, `chore(porch): ${state.id} ${state.phase} phase-transition`); return next(workspaceRoot, projectId); } diff --git a/packages/codev/src/commands/porch/protocol.ts b/packages/codev/src/commands/porch/protocol.ts index c648aca7a..cd01ac65e 100644 --- a/packages/codev/src/commands/porch/protocol.ts +++ b/packages/codev/src/commands/porch/protocol.ts @@ -219,17 +219,6 @@ function normalizePhase(p: unknown): ProtocolPhase { }; } - // A phase carries a "PR consultation" iff its consultation block has - // `on: "review"`. The bare presence of a consultation block is NOT - // sufficient — RESEARCH's investigate / critique phases carry consultation - // for non-PR purposes (type: "investigation" / "critique"), and matching - // those would mis-set pr_ready_for_human in `isPrCreatingPhase`. - let hasPrConsultation = false; - if (phase.consultation && typeof phase.consultation === 'object') { - const consultationObj = phase.consultation as Record; - hasPrConsultation = consultationObj.on === 'review'; - } - return { id: phase.id as string, name: (phase.name as string) || phase.id as string, @@ -241,7 +230,6 @@ function normalizePhase(p: unknown): ProtocolPhase { gate: gateName, checks: checks.length > 0 ? checks : undefined, next, - hasPrConsultation, }; } @@ -432,24 +420,18 @@ export function getOnCompleteConfig(protocol: Protocol, phaseId: string): OnComp /** * Is this phase the one that creates the PR and runs CMAP at PR time? * - * Two markers identify the PR-creating phase across the bundled protocols: - * - `gate === 'pr'` — SPIR/ASPIR/PIR review, AIR pr (covers protocols with - * an explicit PR-review gate). - * - `consultation.on === 'review'` — BUGFIX pr (once-phase that runs CMAP - * via prompted builder steps and has no gate). The narrow `on === 'review'` - * check matters: RESEARCH's `investigate` and `critique` phases also carry - * consultation blocks but for non-PR purposes (`type: "investigation"` / - * `"critique"`). Matching bare consultation presence would mis-flag those - * phases as PR-creating and leak `pr_ready_for_human: true` into research - * state. + * All five PR-emitting protocols (SPIR / ASPIR / PIR / AIR / BUGFIX) mark + * their PR-creating phase with `gate: "pr"`. BUGFIX was the historical + * exception (gateless once-phase, identified via `consultation.on === 'review'` + * as a secondary marker) until issue #887 normalized it onto the same gate + * shape as AIR; the single-marker invariant now holds across all bundled + * protocols. Adding a new PR-emitting protocol means landing `gate: "pr"` + * on its PR-creating phase. * * Used by porch to set `pr_ready_for_human` on transitions out of this phase's - * CMAP-emitting state. Adding a new protocol with a CMAP-emitting PR phase - * means landing either marker (preferred: a `consultation` block with - * `on: "review"` for once-phases, or `gate: "pr"` for build_verify phases). + * CMAP-emitting state. */ export function isPrCreatingPhase(protocol: Protocol, phaseId: string): boolean { const phase = getPhaseConfig(protocol, phaseId); - if (!phase) return false; - return phase.gate === 'pr' || !!phase.hasPrConsultation; + return phase?.gate === 'pr'; } diff --git a/packages/codev/src/commands/porch/types.ts b/packages/codev/src/commands/porch/types.ts index 10b979490..2dfab5ec6 100644 --- a/packages/codev/src/commands/porch/types.ts +++ b/packages/codev/src/commands/porch/types.ts @@ -48,20 +48,6 @@ export interface ProtocolPhase { gate?: string; // Gate name that blocks after this phase checks?: string[]; // Check names to run (keys into protocol.checks) next?: string | null; // Next phase id, or null if terminal - /** - * Whether the phase definition carries a `consultation` block with - * `on: "review"` in protocol.json — the marker that distinguishes a - * PR-creating once-phase (BUGFIX/AIR `pr`) from other CMAP-emitting - * once-phases (e.g. RESEARCH `investigate` / `critique`, which run - * consultation for non-PR purposes). - * - * `consultation.on === 'review'` exists today on both BUGFIX and AIR `pr` - * phases and is absent on RESEARCH's investigation/critique phases. Used by - * `isPrCreatingPhase` together with `gate === 'pr'` to classify the - * PR-creating phase across all five PR-emitting protocols without - * misclassifying RESEARCH or any future non-PR consultation phases. - */ - hasPrConsultation?: boolean; } /** @@ -187,10 +173,10 @@ export interface ProjectState { }; /** * Canonical signal that CMAP for the PR-creating phase has completed and a - * human reviewer is now the bottleneck. Set true the moment porch transitions - * out of the CMAP-emitting state (gate-pending for protocols with a `pr` gate, - * phase advance for protocols without one — currently BUGFIX). Reset to false - * when the rebuttal cycle re-enters CMAP after REQUEST_CHANGES. + * human reviewer is now the bottleneck. Set true the moment porch auto-requests + * the `pr` gate (all five PR-emitting protocols carry `gate: "pr"` on their + * PR-creating phase as of #887). Reset to false on `pr` gate approval, on + * rollback, and when the rebuttal cycle re-enters CMAP after REQUEST_CHANGES. * * Consumers (dashboard NeedsAttentionList, VSCode tree, future surfaces) read * this single boolean instead of deriving "is the PR waiting?" from the