Decouple worktree/branch/PR, add verify phase, remove TICK (#653)#674
Merged
waleedkadous merged 27 commits intomainfrom Apr 17, 2026
Merged
Decouple worktree/branch/PR, add verify phase, remove TICK (#653)#674waleedkadous merged 27 commits intomainfrom
waleedkadous merged 27 commits intomainfrom
Conversation
…eedback Extends the spec per architect direction (2026-04-11) and incorporates Gemini/Codex/Claude consultation feedback. Major additions: - Component 6: Post-Merge Verify Phase as a new terminal SPIR/ASPIR phase with a new human-only verify-approval gate and integrated lifecycle state - Three-stage team visibility framing (spec review, code review, post-merge verify) threaded through Problem Statement and Desired State - Implementation Ordering section slicing scope into A/B/C shippable chunks Consult-driven fixes: - Opt-in contradiction: porch next no longer emits checkpoint PR tasks on gate-pending alone; requires explicit porch checkpoint opt-in - porch review -> porch checkpoint rename (avoids collision with review phase) - AI hallucination guard: verify phase emits scaffolding tasks only; AI may not fill verifier entries or sign off on verify-approval - Verify note commit flow via a small verification PR (default) with direct-to-main as opt-out for repos without branch protection - Forge concept inventory: pr-create, pr-comment, pr-is-merged, pr-comments, pr-current-branch - no raw gh calls anywhere - Revision state mechanics: porch feedback resets build_complete and increments iteration (not porch done) - PR comment timing: emitted on porch approve verify-approval (post-gate), not during verify-phase task execution - Stronger verify_note_has_pass check (matches PASS, not just ## Sign-off) - porch verify --skip/--reset promoted to first-class command surface - Backward-compat migration shim for pre-upgrade projects Spec file grew 471 lines net; no other files touched.
…fecycle Applies iter3 consultation feedback (Gemini REQUEST_CHANGES, Codex REQUEST_CHANGES, Claude COMMENT — all HIGH confidence). Factual corrections: - once phase type ALREADY exists in handleOncePhase (next.ts:741) — earlier spec claimed it was new infrastructure. All 3 reviewers flagged. - pr_is_merged cannot be a raw shell command — there is no forge CLI on PATH. Must be intercepted by name in checks.ts (like pr_exists at :262) and dispatched via executeForgeCommand. Codex + Gemini both flagged. Consistency fixes: - Success criterion for checkpoint PR: removed 'when gate becomes pending' contradiction, now explicitly 'after porch checkpoint invoked' (Codex) - external_review state semantics clarified: means 'requested', not 'PR created'. checkpoint_pr field distinguishes (Codex) - ASPIR applicability tightened: ASPIR has no spec/plan gates, so checkpoint PRs are SPIR + TICK only. Verify phase still applies to SPIR + ASPIR + TICK. (Codex + Claude) New content: - Component 6g: Worktree & status.yaml Lifecycle — spells out how the builder worktree survives across the merge boundary, where status.yaml lives, how the verification PR is created off main, local pull-before- approve guard, and the deferred-cleanup invariant in afx cleanup (Codex) - --fail/--skip now have explicit verify-note and PR-comment behavior: both produce verify-note PRs (append for fail, template+waiver for skip), both post closing PR comments on the original checkpoint PR (Codex) - --from-pr security bounds: 50 KB per comment, 100 KB total, 20 comment cap, secret heuristic, interactive confirmation. Prevents secret leakage into committed status.yaml (Codex) - porch feedback now prints the exact afx send command for the architect to copy-paste, removing the two-step error mode (Claude) - max_iterations=1 confirmed feasible: Gemini verified against next.ts that iteration bump + build_complete reset works natively — no runtime change needed (Gemini) Net delta: +86 / -26 lines in the spec file only.
Throws out the 752-line iter3 spec and rewrites around the architect's core reframing (2026-04-12, 12 inline REVIEW comments on iter3). Core insight (addresses REVIEW comments at lines 274, 300): Break the 1-builder = 1-PR assumption. A builder is a persistent worktree that produces MULTIPLE PRs sequentially over its lifetime. Worktree, branch, and PR are three separate things: - Worktree: persistent, keyed by project ID only (.builders/<protocol>-<id>/) - Branch: transient, cut per-stage, deleted after merge - PR: output of a branch, one open per worktree at a time Flow: stage-1 -> PR #1 -> merge -> pull main -> stage-2 -> PR #2 -> ... Simpler model — rewrites around just four components: - A: pr-exists tightening (kept from old spec) - B: worktree/branch/PR decoupling + porch cold-start resume from main - C: OPTIONAL verify phase (no artifact, no template, no sign-off note) - Terminal state renamed integrated -> verified Deleted entirely (do not resurrect): - porch checkpoint, porch feedback commands - Gate sub-states (external_review, feedback_received) - Feedback history, --from-pr, size limits, secret heuristics - Verify note artifact + template + sign-off block - Three-stage rigid team-visibility framing - 1-builder-equals-1-PR accumulating-checkpoint model Architect-builder interaction model (addresses REVIEW comment at line 324): Porch runs in builder context. Architect gives high-level instructions via afx send; builder decides porch operations. ci-channel already delivers merge/CI events to the builder, closing the loop without any porch-side plumbing. Length: 752 -> 154 lines. Within the 150-250 target.
…old-start, clarify interactions Fixes from 3-way consultation (all REQUEST_CHANGES): - Terminal state is 'complete' in codebase, not 'integrated' (Claude) - Architect/porch constraint: verify phase is explicit human-driven exception to builder-runs-porch rule (Gemini, Codex) - Cold-start resume scoped to verify + read-only phases; implement needs a worktree (Claude, Codex) - Cut-and-merge loop is builder-driven, porch-unaware; branch naming is up to the builder (Claude) - porch verify --skip reason is required (Codex) - #662 is prerequisite for worktree path change (Codex) - Backward compat detection: check gates map for verify-approval entry - Constraints acknowledge porch verify as one new subcommand (Claude) - Open questions resolved: cold-start scope, --skip reason Rebuttal written for all 3 reviewers at 653-specify-iter0-rebuttals.md.
…odel, kill TICK, PR tracking Four changes per architect feedback (2026-04-12): 1. Builder stays alive through verify (DEFAULT path): Flipped framing. Builder pulls main after merge, drives verify. Cold-start is the FALLBACK for when the builder has been gone. 2. Single state model (design principle, not just vocab kill): porch's phase + gate status IS the canonical project state for the ENTIRE system. All consumers (afx status, dashboard, reporting, CLAUDE.md tracking, GitHub issue labels) read status.yaml directly. No parallel state vocabulary. No translation layer. No derived states. 3. Kill TICK protocol from scope: TICK becomes redundant once multi-PR worktrees land. Amendments during verify are just another PR from the same worktree. TICK was a workaround for the 1-builder-1-PR constraint. Deprecation of the actual TICK code is a follow-up. 4. status.yaml records PR numbers per stage: Porch records PR history (number, branch, merged status) even though it doesn't drive the git mechanics. Elevated status.yaml commit-at-every-transition to a hard success criterion. 167 lines, within 150-200 target.
…ignment postponed Three changes per architect's final feedback: 1. Remove cold-start resume entirely. If builder terminal is gone, use afx spawn --resume. No reading status.yaml from main without a worktree. status.yaml committed at every transition is kept (needed for worktree persistence across merges). 2. TICK removal is IN SCOPE (Component D). Delete protocol definition, skeleton, and all references. Protocol list after this ships: SPIR, ASPIR, AIR, BUGFIX, MAINTAIN, EXPERIMENT. 3. State alignment (single state model) postponed to follow-up spec. Noted as future work in Desired State section 2. 166 lines. Four slices: A (pr-exists), B (worktree/branch/PR), C (verify phase), D (remove TICK).
Six phases across four slices: 1. pr-exists tightening (Slice A) — 3 forge scripts + test 2. status.yaml commit infrastructure (Slice B) — writeStateAndCommit wrapper replacing 16+ bare writeState calls across next.ts/index.ts 3. PR tracking schema + worktree path normalization (Slice B) 4. Verify phase + terminal state rename complete→verified (Slice C) 5. Remove TICK protocol (Slice D) 6. Documentation and prompt updates Key decisions: - PR recording via optional flags on porch done (no new subcommand) - Worktree path change subsumes #662 (prerequisite is OPEN, no PR) - Verify task is inline in handleOncePhase, not a separate prompt file - No cold-start mechanism (afx spawn --resume is recovery path) - State alignment is explicitly future work
Fixes from 3-way plan consultation (all REQUEST_CHANGES): Phase 1: test rewritten to target pr-exists.sh scripts directly, not stale protocol.json commands (Gemini) Phase 2 writeStateAndCommit: - execFile with args array, not string interpolation (Claude, Codex) - git push -u origin HEAD for upstream tracking (Gemini) - Removed --allow-empty (Claude) - Noted completion task overlap becomes redundant (Codex) Phase 3 porch done --pr/--merged: - Explicit record-only semantics: write PR metadata, exit immediately - No check-running, no phase advancement (Codex, Gemini) - --branch mode path also simplifies (Claude) Phase 4 terminal rename: - Added 3 agent-farm files: overview.ts, status.ts, overview.test.ts (Claude critical finding) - Explicit DO NOT rename: PorchNextResponse.status, PlanPhaseStatus (Codex) - Universal migration: all protocols, not just those with verify (Gemini) - Verify flow clarified: porch done -> gate -> approve (Codex) - Convenience shortcut: porch approve auto-completes done for verify - Risk table: 'Low' -> 'Certain' for dashboard rename (Claude) Phase 5: full-repo TICK search, not just packages/codev/src (Codex) Phase 6: git fetch origin main && git checkout -b (Gemini)
…not-merged PRs GitHub: filter jq output to select only OPEN or MERGED state PRs (keeps --state all from bugfix #568 to fetch all, then filters) GitLab: add jq filter for opened or merged MRs only Gitea: add jq filter excluding closed-not-merged pulls (Gitea merged PRs have state=closed + merged=true) Test: rewritten to validate forge scripts directly instead of reading protocol.json commands (the actual check is intercepted in checks.ts and routed to the scripts, so testing the scripts is the correct level of abstraction) All 2253 unit tests pass.
…efore filtering GitLab: add --all flag so glab mr list returns merged MRs (without it, only open MRs are returned and the jq merged filter is dead code) Gitea: add --state all flag so tea pulls list returns closed/merged pulls (same issue as GitLab) Tests: add assertions that GitLab uses --all and Gitea uses --state all All 10 pr-exists tests pass. Addresses Codex + Gemini iter1 feedback.
…ommit+push at every porch state transition New function in state.ts: writeStateAndCommit() wraps writeState() with git add + commit + push. Uses execFile with args array (no shell injection). Uses git push -u origin HEAD (upstream tracking for new branches). No --allow-empty (failure = logic bug). Replaced all 16 bare writeState() calls across next.ts (9 sites) and index.ts (7 sites) with writeStateAndCommit(). Each call site passes a descriptive commit message: chore(porch): <id> <phase> <event> where event is: phase-transition, build-complete, gate-requested, gate-approved, review-recorded, protocol-complete, init, rollback. advanceProtocolPhase in index.ts converted from sync to async to support the async writeStateAndCommit call. Git operations are skipped in VITEST environment (process.env.VITEST check) so tests work in non-git temp directories. State mutation is still fully tested; only the git IO is skipped. Spec 653 §B.3: "Every phase transition, gate request, gate approval, and verify skip must commit and push status.yaml. Zero gaps." All 2256 tests pass, build clean.
…ask + dead imports Consultation feedback (3/3 flagged same issue): - Removed commitStatusTask block from next.ts — status.yaml is now auto-committed by writeStateAndCommit at every phase transition, so the manual "commit status.yaml" task is redundant - Removed dead writeState imports from next.ts and index.ts (all calls replaced by writeStateAndCommit in prior commit) - Updated 2 tests that asserted commitStatusTask presence: bugfix-complete now expects no tasks; non-bugfix-complete expects only the merge task All 2256 tests pass.
…ktree path normalization PR tracking (types.ts + index.ts): - Added pr_history array to ProjectState for per-stage PR records (phase, pr_number, branch, created_at, merged, merged_at) - Extended porch done with --pr/--branch (record-only: writes PR entry and exits, no phase advancement) and --merged (marks PR entry as merged with timestamp) - CLI handler parses --pr, --branch, --merged flags Worktree path normalization (spawn.ts, subsumes #662): - Spec-based spawns: ${protocol}-${id} (no title suffix) - Bugfix spawns: bugfix-${id} (no title suffix) - --branch mode: same ID-only pattern - --resume migration: tries ID-only path first, falls back to old title-based pattern (prefix search) for backward compat - Removed specSlug variable; porch project name derived inline Build clean, all 2256 tests pass.
…tests Consultation feedback fixes: - parseInt validation: Number.isInteger + > 0 check, throws on bad input - --pr and --merged are mutually exclusive (throws if both provided) - Truthiness checks changed to !== undefined - CLI arg parsing: skip --flaglike args when detecting project ID - Help text updated with --pr/--branch/--merged flags New tests (4 cases in done-verification.test.ts): - Records PR in pr_history (record-only, no phase advancement) - Marks PR as merged with timestamp - Throws when --pr used without --branch - Throws when --merged targets nonexistent PR All 2260 tests pass.
…me, porch verify command
Protocol changes:
- SPIR and ASPIR protocol.json: review.next = "verify" (was null)
- New verify phase: type=once, gate=verify-approval, next=null
- Both codev/ and codev-skeleton/ copies updated
Terminal state rename (complete → verified):
- next.ts: state.phase assignments (2 sites)
- next.ts: completion check handles both 'verified' and 'complete'
- index.ts: advanceProtocolPhase, rollback handler, status badge
- overview.ts: progress calculation (verified=100, verify=98)
- status.ts: styling for verify and verified phases
Verify phase behavior:
- handleOncePhase: custom task text for verify phase with prominent
skip option ("porch verify <id> --skip 'reason'")
- New porch verify subcommand: --skip transitions to verified,
records reason in context.verify_skip_reason
- porch approve verify-approval: auto-completes porch done if
build_complete is false (convenience shortcut)
Backward compatibility:
- readState() auto-migrates phase='complete' to 'verified' on load
- Progress/styling functions handle both 'verified' and 'complete'
- Help text updated with verify command
All 2262 tests pass. Build clean.
Codex feedback fixes: - done() auto-requests verify-approval gate when not yet requested (previously required separate porch gate command) - approve() auto-advances via advanceProtocolPhase after verify-approval (one-command convenience — no separate porch done needed) Tests added: - porch done in verify phase auto-requests verify-approval gate - readState migrates phase:complete to phase:verified (backward compat) - spirProtocol test fixture updated to include verify phase All 2264 tests pass.
Protocol directories deleted: - codev/protocols/tick/ (10 files) - codev-skeleton/protocols/tick/ (10 files) - codev-skeleton/porch/prompts/understand.md (TICK-specific) - codev-skeleton/porch/prompts/verify.md (TICK-specific) Source code updates: - state.ts: removed 'tick' from worktree detection regex - next.ts: updated once-phase comments (TICK → verify) - cli.ts: removed tick from --protocol help text - agent-farm/cli.ts: removed tick from --protocol help text Test updates: - state.test.ts: tick worktree detection test → expects null - next.test.ts: once-phase test uses bugfix instead of tick - overview.test.ts: removed tick progress calculation test Documentation updates (19 files in codev-skeleton): - Removed TICK from protocol lists, examples, and references - Removed TICK amendment history sections from spec/plan templates - Removed --amends flag documentation - Updated protocol-schema.json examples Protocol list after this: SPIR, ASPIR, AIR, BUGFIX, MAINTAIN, EXPERIMENT All 2263 tests pass. Build clean.
…docs Source code: - spawn.ts: removed --amends validation, tick-specific specLookupId, tick protocol examples. --amends now errors with "no longer supported" - cli.ts (agent-farm): removed --amends flag registration - spawn.ts JSDoc: TICK -> ASPIR, AIR Documentation (root + skeleton templates): - CLAUDE.md / AGENTS.md: removed TICK protocol section, tick directory entry, tick spawn examples, "SPIR, TICK, and BUGFIX" -> "SPIR, ASPIR, AIR, and BUGFIX" - codev-skeleton/templates/CLAUDE.md / AGENTS.md: same Addresses Codex + Gemini feedback: deeper TICK references in spawn validation, CLI flags, and root documentation. All 2263 tests pass. Build clean.
…e in builder prompts SPIR and ASPIR builder-prompt.md: - Added Multi-PR Workflow section: persistent worktree, sequential PRs, git fetch origin main pattern (not git checkout main — worktree constraint), PR recording via porch done --pr/--merged - Added Verify Phase section: builder stays alive through verify, porch done signals readiness, architect approves verify-approval, porch verify --skip for opt-out - Updated notifications: "PR merged" now says "Entering verify phase" instead of "Ready for cleanup" Builder role (codev-skeleton/roles/builder.md): - Updated PR merged notification to reference verify phase All 2263 tests pass. Build clean.
…e, multi-PR, TICK removal Addresses all 3 reviewer feedback (Codex/Claude/Gemini REQUEST_CHANGES): codev/roles/builder.md: - SPIR phases now include optional Verify step - PR merged notification → "Entering verify phase" - Removed TICK escalation reference - Added multi-PR workflow + afx spawn --resume section codev-skeleton/roles/builder.md: - Same verify phase and multi-PR updates CLAUDE.md / AGENTS.md: - SPIR/ASPIR descriptions include "(→ Verify)" - afx cleanup → "architect-driven, not automatic" codev/resources/arch.md: - 12 TICK references removed (glossary, protocol section, directory trees, CLI examples, porch description, architectural decisions) - Renumbered remaining architectural decisions All 2263 tests pass. Build clean.
Verify flow (Codex critical finding): - Verify phase task now includes "Step 1: Merge the PR" with forge merge command, before "Step 2: Verify" - Terminal complete state for protocols with verify phase skips the merge task (already happened in verify) - next() creates gate entries when advancing to new phases, fixing upgraded projects that lack verify-approval gate TICK cleanup: - types.ts: updated protocol list comment, amends field marked as deprecated/legacy All 2263 tests pass.
Contributor
Author
Architect Integration ReviewExcellent work on this — the four-slice decomposition is clean, I'd like to see 7 changes before merge. The first 3 are critical, the rest are minor. Critical1. 2. 3. Post-merge state loss in verify phase Minor4. 5. Dead variable 6. Missing 7. TICK compat branches in |
Critical: 1. porch verify --skip restricted to verify phase only (was review|verify) — prevents bypassing the PR gate 2. findStatusPath searches .builders/ worktrees FIRST, falls back to codev/projects/ (main) — fixes stale main copy in multi-PR workflows 3. Post-merge state loss: documented as known limitation. Closed GitHub Issue is the canonical "done" signal. State alignment is future work. Minor: 4. readState() is now pure — migrates complete→verified in-memory only, does not write to disk (callers commit on next mutation) 5. Removed dead positionalId variable from done CLI handler 6. porch approve auto-creates gate entry for upgraded projects when the gate belongs to the current phase (handles verify-approval missing after protocol upgrade) 7. TICK compat branches in overview.ts documented as legacy compat Test updated: findStatusPath preference test flipped to match new worktree-first search order (spec 653 supersedes bugfix 622). All 2263 tests pass. Build clean.
Contributor
Author
Architect Review — Re-reviewAll 7 items verified in the latest push (d23b9bb):
Test updated for new findStatusPath order. CI green. Approved — please merge. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Breaks the "1 builder = 1 branch = 1 PR" assumption. A builder worktree is now persistent and produces multiple sequential PRs over its lifetime.
Four slices:
writeStateAndCommitat every porch transition (16 sites), PR history tracking in status.yaml, worktree path normalized to ID-only (.builders/<protocol>-<id>/)complete→verified,porch verify --skipcommand,verify-approvalhuman-only gateTest plan
pr-existsreturns false for CLOSED-not-merged PRs (GitHub/GitLab/Gitea scripts)porch done --pr N --branch namerecords PR without advancing phaseporch done --merged Nmarks PR as merged--resumefalls back to old title-based pathporch verify --skip "reason"transitions toverifiedporch approve verify-approvalauto-advances to terminal statereadStatemigratesphase: 'complete'→'verified'on loadafx spawn --protocol tickerrors with "no longer supported"afx statusshows correct progress forverifiedprojectsCloses #653
🤖 Generated with Claude Code