Skip to content

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

Merged
waleedkadous merged 7 commits into
mainfrom
builder/bugfix-887
May 27, 2026
Merged

Fix #887: BUGFIX add pr gate (close v3.1.4 pr_ready timing gap)#888
waleedkadous merged 7 commits into
mainfrom
builder/bugfix-887

Conversation

@waleedkadous
Copy link
Copy Markdown
Contributor

Summary

v3.1.4 (PR #874) added the canonical pr_ready_for_human signal but the BUGFIX-specific set-point was timing-wrong — it fired only 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 and never surfaced in Needs Attention.

This PR adds "gate": "pr" to BUGFIX's pr phase (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.

Fixes #887

Root Cause

The setter design in #874 baked in a BUGFIX special case because BUGFIX had no pr gate. AIR's gate-request-time setter (index.ts:418) demonstrably gets the signal timing right; BUGFIX was wired to the advanceProtocolPhase terminal-exit setter (index.ts:453) instead, which only fires post-merge.

Adding a pr gate to BUGFIX makes it semantically identical to AIR, so the gate-request setter handles it correctly.

Fix

Protocol JSON (both copies, bumped to 1.2.0)

  • codev/protocols/bugfix/protocol.json + skeleton — added "gate": "pr" to pr phase

Porch code cleanup (BUGFIX-no-gate branches now redundant)

  • index.ts:advanceProtocolPhase — removed the advancingFromPrPhase snapshot and the two conditional pr_ready_for_human setters
  • index.ts done() gate-request — simplified gate === 'pr' || isPrCreatingPhase(...) to gate === 'pr'
  • next.ts:handleVerifyApproved — same simplification, dropped the no-gate-advance pr-readiness logic
  • next.ts — removed the BUGFIX-specific terminal-state return (BUGFIX now follows AIR's merge-task path)
  • protocol.ts:isPrCreatingPhase — collapsed to phase?.gate === 'pr'; JSDoc updated
  • protocol.ts:normalizePhase — no longer computes hasPrConsultation
  • types.ts:ProtocolPhase — removed hasPrConsultation field

Builder prompt

  • 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 v1.2.0 with gate: "pr"; rewrote BUGFIX cases to test the gate-request path (matching AIR); updated classifier describe block to reflect the single-marker invariant. RESEARCH false-classification regression tests preserved.
  • next.test.ts — updated the BUGFIX-terminal-state test to assert the new merge-task emission and the approved-gate-on-terminal-state shape (was: gates: {} → now: gates: { pr: approved })

Docs

Test Plan

  • Regression test added — pr-ready-872.test.ts BUGFIX cases now exercise the gate-request setter at index.ts:486-493 (the same path AIR uses)
  • Build passes (npx tsc clean)
  • All porch tests pass (334/334)
  • All overview tests pass (148/148) including the derivePrReady BUGFIX-fallback path
  • Combined porch + overview suite: 482/482 passing

Flow After Fix

  1. Builder runs CMAP → porch done <id> → auto-requests pr gate, sets pr_ready_for_human: true, PR surfaces in Needs Attention
  2. Architect: porch approve <id> pr → gate approved, pr_ready_for_human: false
  3. Builder runs porch done <id> → advances prverified; the next porch next emits the merge task
  4. Builder merges PR → notifies architect

This matches AIR exactly.

Out of Scope

  • Removing derivePrReady BUGFIX-fallback in overview.ts — left for graceful degradation of in-flight v3.1.4 projects (the issue body explicitly defers to v3.2+)
  • codev/protocols/bugfix/protocol.md long-form workflow doc — not in the issue scope (still describes the manual "afx send LGTM merge it" flow that's redundant with the gate; can be updated in a follow-up)

v3.1.4 PR #874 wired BUGFIX's pr_ready_for_human=true to the
advanceProtocolPhase terminal-exit setter because BUGFIX had no
`pr` gate. That set-point 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.

This adds "gate": "pr" to BUGFIX's pr phase (same shape as AIR),
so the canonical signal fires via the gate-request setter at the
right moment — when CMAP completes and the PR is waiting for a
reviewer. The BUGFIX-no-gate branches in advanceProtocolPhase,
handleVerifyApproved, and isPrCreatingPhase are now redundant and
removed; hasPrConsultation field dropped from ProtocolPhase. The
derivePrReady BUGFIX fallback in overview.ts is intentionally left
in place as graceful-degradation for in-flight v3.1.4 projects.

Fixes #887
Codex CMAP nit on PR #888 — the comment near approve() still
described BUGFIX as having no pr gate, which is no longer true
after #887. Updated to call out the unified pr-gate invariant
across all five PR-emitting protocols.

Refs #887
@waleedkadous
Copy link
Copy Markdown
Contributor Author

Approved. The diff is exactly what the issue scoped — gate added, hasPrConsultation field genuinely removed (not just commented out), advanceProtocolPhase cleanly simplified. Keeping the RESEARCH negative-classification regression tests under the new single-marker rule is the right call — they still pin the invariant ("phases with non-pr gates and/or non-review consultation are correctly excluded") even though the original iter-2 framing changed.

Please merge with --admin bypass.

@waleedkadous waleedkadous merged commit f3ebdbf into main May 27, 2026
6 checks passed
@waleedkadous waleedkadous deleted the builder/bugfix-887 branch May 27, 2026 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

porch: BUGFIX should have a pr gate (closes the v3.1.4 pr_ready_for_human timing gap)

1 participant