GitHub /builder approves the newest plan on the issue, not the one the approver read

[Julian-Genuario]

Summary

A /builder comment on GitHub is not bound to a specific plan. githubPlanAcceptance (services/api/src/github/router.ts:641-674) picks the newest plan_acceptance proposal on the issue whose Architect run succeeded, and the approval is written against whatever came back:

.where(and(... eq(actionTypes.name, "plan_acceptance"), eq(runs.status, "succeeded"), ...))
.orderBy(desc(proposals.createdAt))
.limit(1)

COMMAND_RE (router.ts:60-61) carries nothing that could disambiguate, and nothing supersedes or expires an older open proposal when a newer Architect run succeeds on the same issue. The API lane is proposal-id-bound (POST /v1/proposals/:id/decide), so this is GitHub-only.

Scenario

A reviewer reads plan P1 (proposal A, published with the facility:architect-plan:<run>:<proposalA> marker). A teammate re-runs /architect; run 2 succeeds and opens proposal B seconds before the reviewer's /builder webhook is processed — or the reviewer simply never noticed the second publication comment. Facility approves B, dispatches the Builder on P2, and the audit trail records that the reviewer approved P2 with a valid plan hash and fresh evidence. Every downstream check in builder-plan-policy.ts passes, because they verify that the approved proposal is consistent, not that it is the one the human read.

Why it matters

Gate 1 is the product invariant ("a person accepts the plan"). Here the person accepted a plan; which one is decided by ordering.

Suggested fix

Either require the proposal id (or a plan-hash prefix) in the command — /builder <proposalA> — with the bare form accepted only when exactly one live proposal exists on the issue; or supersede older open plan_acceptance proposals when a new Architect run succeeds on the same issue, and refuse /builder while more than one is live. Both keep the current UX for the common case.

Adjacent but different from #222/#223 (comments invalidating an approved plan) and #147 (base commit not recorded).

[ophiocus]

This one has real blast radius — approval binding to "newest" instead of "the one approved" is a governance-correctness bug of the same family as the KB races #195 closed (the artifact you validated is not the artifact you act on). Are you planning the fix? Otherwise I'll gladly take it: bind the approval to the plan's identity at approval time, refuse dispatch when the approved plan is not the selected one, and a two-plans-on-one-issue test proving the wrong-binding path dies. Your find, your right of way.

[ophiocus]

Standing down here with a tip of the hat — #259 landed a full implementation while my courtesy clock was still running, and racing it now would help nobody. The supersede-older-proposals design there is a cleaner shape than the bind-at-approval I sketched. Good luck with it, @islahuddinn.

[islahuddinn]

Thanks, @ophiocus — I appreciate you standing down and for the kind words on the supersede design.

I've rebased #259 onto latest main and kept the optional /builder <proposal-id> path alongside the single-live-plan default, so the common case stays unchanged while ambiguous issues fail closed. Happy to adjust anything during review.

[ALVZ93]

I'd like to take this. I confirmed the ordering-dependent selection still exists on current main (73c3ab5).

My proposed scope is to bind the published Gate 1 CTA to its proposal ID (/builder <proposal-id>), resolve that proposal explicitly in the GitHub route, and allow the legacy bare /builder form only when exactly one live plan is eligible for the issue. If more than one live plan exists, routing should fail closed without approving either one. The proposal-ID-bound API lane would remain unchanged.

I'll keep the change focused and add both unit coverage for command/selection behavior and a database-backed regression for the two-open-plan race described here. If you prefer superseding older proposals instead, please say so before the PR is reviewed.