Skip to content

Fix #113: verify-approval cannot pass after the PR is merged - #137

Merged
pseudoseed merged 10 commits into
mainfrom
builder/bugfix-113
Aug 24, 2026
Merged

Fix #113: verify-approval cannot pass after the PR is merged#137
pseudoseed merged 10 commits into
mainfrom
builder/bugfix-113

Conversation

@pseudoseed

Copy link
Copy Markdown
Owner

Summary

porch approve <id> verify-approval was unpassable after a normal merge. It re-ran review's pr_exists against the current worktree branch, which is almost never the PR head once the PR is merged.

Fixes #113

Root Cause

The SPIR verify phase has no checks. After the pr gate is approved and the PR merges, the project is still in review until someone runs porch done. Approving verify-approval ran checks for state.phase (review), whose first check is pr_exists.

That check asks git branch --show-current in whichever worktree findStatusPath returns. After merge that is almost never the PR head. The forge script already accepts MERGED (cluesmith#568). Against builder/spir-83 it returns true. The lookup key is the wrong branch, and the post-merge gate should not run that check at all.

Reproduced on project 83 (PR #104 MERGED, still in review). porch check 83 from this worktree failed pr_exists in 0.6s. From main it passed only because findStatusPath hit air-106, whose unrelated merged PR #132 satisfied the check.

Fix

When porch approve is asked for verify-approval and the project is still in review with pr already approved, enter verify first. The checks that then run are verify's (none). The existing auto-advance can reach verified.

Refuses if pr is not approved, so the pr gate cannot be skipped.

Test Plan

  • Regression test: bugfix-113-verify-approval-merged-pr.test.ts
    • Review's pr_exists is false. Without the fix, approve exits 1. With it, the gate approves and phase becomes verified.
    • Refuses when pr is still pending.
    • Still works when already in verify.
  • Confirmed the test fails without the fix and passes with it.
  • tsc --noEmit clean.
  • Porch unit tests: 587 passed.
  • Full @cluesmith/codev suite: 6023 passed, 48 skipped. v2: 167 passed.

After merge the project is still in review. Approving verify-approval
was running review's pr_exists against the current worktree branch,
which is almost never the PR head.
Fails without the fix: approve re-runs review's pr_exists (false) and
exits 1. Passes with it: gate approves and phase becomes verified.
@pseudoseed

Copy link
Copy Markdown
Owner Author

Integration review — approved

One source file, +219/-0, all 7 checks green including Unit Tests at 3m2s.

Problem. porch approve <id> verify-approval could not pass. It re-ran the review phase's
pr_exists check, which failed.

Root cause. Worth recording, because the issue title states it wrongly and the builder
found the real one. The title says the check fails "because merging the PR closes it" — but
pr_exists is gh pr list --state all --head "$(git branch --show-current)", and --state all already matches a merged PR (that is what #16 / upstream cluesmith#1331 was about). The actual
cause is the --head argument: git branch --show-current runs in whichever worktree
findStatusPath returns, and after a merge that is almost never the PR head branch. So the
search is for the wrong head, not for the wrong state.

Fix. approve() enters the verify phase before running checks when the gate is
verify-approval and the pr gate is already approved. Verify has no checks in either spir or
aspir (confirmed in both protocol.json files), so nothing re-runs and the existing
auto-advance reaches verified.

Two things I checked before approving, because this mutates state inside approve():

  1. It cannot become a gate bypass. The new block sits at line ~874, and the
    if (!hasHumanFlag) { ...; process.exit(1) } guard is at ~863. The flag is validated first,
    so no phase transition or commit can happen without --a-human-explicitly-approved-this.
  2. Writing build_complete = true inside approve() is not a new invention. The existing
    block at index.ts:840 already does exactly this with a
    chore(porch): <id> verify build-complete (auto) commit. The change follows the idiom
    already in the file rather than adding a second one.

The new throw for any other phase replaces a path that would previously run the wrong
phase's checks and fail confusingly. Clearer failure, not a new restriction.

Merge with --merge. Do not squash.

pr_exists already matches merged PRs via --state all. The failure is
--head resolving to the wrong branch after merge.
@pseudoseed
pseudoseed merged commit 9d55fc7 into main Aug 24, 2026
pseudoseed added a commit that referenced this pull request Aug 24, 2026
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 verify-approval cannot pass: pr_exists fails because merging the PR closes it

1 participant