Deepen Delivery's pull request lifecycle #507

What to build

Deepen Delivery's pull request module so a Run, and a Spec run delivering its Spec PR, keeps hold of one identified pull request throughout Delivery. Readiness, Self-merge, failed-merge confirmation and Failed run draft conversion must act on that same pull request.

Today the Base branch is checked initially, but later readiness checks omit it and operations select again by Issue branch. A pull request retargeted during CI can merge into another branch while thirdshift reports success and closes the issue. An observed target change must instead fail Delivery through the existing Failed run path.

This is one complete refactor and regression fix, sized for one fresh implement session.

Acceptance criteria

• One Delivery-owned pull request module holds the captured repository, pull request number and URL, expected Issue branch and expected Base branch. Delivery, the Repair loop and Failed run handling use it; they no longer reconstruct its identity or readiness contract.
• The first successful pull request observation after the opening session binds its identity. Once bound, every later observation, body edit, ready/draft transition, merge attempt and merge confirmation selects that number in that repository. A missing or closed original never causes a switch to a newer pull request from the same Issue branch.
• The initial ready transition and every final readiness/mergeability observation validate the same repository's Issue branch, expected Base branch, open state and ready state where required. Mergeability polling revalidates those facts on every snapshot, including while mergeability remains unknown.
• For both a Run left ready for review and a Merge run, retargeting the captured pull request during CI fails before successful delivery. The Merge run issues no merge request, closes no issue and deletes no branch on origin; the unrelated target branch stays unchanged. Failed run work is preserved and the captured open pull request becomes a draft.
• Retargeting during unknown-mergeability polling also fails. Closing the captured pull request and opening a replacement from the same Issue branch cannot make Delivery succeed or make its later operations modify that replacement.
• Self-merge still requests a merge commit at the exact watched head, without auto-merge or local branch deletion. A fresh invalid/unreadable pre-merge observation stops Delivery immediately; it is not a Policy refusal and starts no additional Repair.
• A failed/interrupted merge request is reconciled using a completion observation of the same numbered pull request. Only its merged state at the requested head, expected Base branch and expected Issue branch establishes success. A different pull request, head or target never establishes success, and the recorded Command interruption remains set.
• Missing/closed/draft/unmergeable pull requests and mergeability timeouts retain their existing failure behavior. The built-with annotation remains idempotent and warning-only; the before-ready callback, Policy refusal behavior, Failed run preservation, and confirmed Self-merge finishing retain their existing meaning.
• Tests exercise the module's interface through a scripted adapter and verify number-selected GitHub transport. CLI regressions cover late retargeting and replacement identity for Run/Spec PR Delivery. Existing merge, CI, Failed run, interruption, Foreign commit, and Spec PR regressions pass.
• The glossary's Delivery and Self-merge entries describe the captured pull request identity and validation of its expected branches. This documentation is implemented with this Ticket; no ADR is reopened.
• Formatting, linting with warnings denied, and the full test suite pass.

Blocked by

None (can start immediately).

Decisions

The design tree was settled in four rounds: scope and constraints → ownership and interface → observations and transitions → failure handling, tests and documentation. Every branch below is settled; there is no pending human decision.

Round 1: scope and constraints

1. Concentrate pull request policy inside Delivery. The module owns identity, annotation, ready-state validation, mergeability waiting, merge-request confirmation and Failed run draft handling. Worktree synchronization, CI classification, Repair budgets, sessions, issue closure and branch deletion stay with their existing modules, because they are separate domain responsibilities.
2. Use the same module for a Run and Spec PR Delivery. Both already take the Delivery path, so this gives leverage without duplicating rules. The Spec PR module's earlier draft creation and Tickets checklist lifecycle remain its responsibility: a Spec PR before Delivery has a different lifecycle.
3. Publish one standalone Ticket. The change has one cohesive seam and can land with its regression tests and glossary update in one session. A Spec and separate migration Tickets would add coordination without enabling independently useful slices. No open issue covers this deepening; the open issues' titles and bodies were checked again before publication.

Round 2: ownership and interface

4. Create one owner before the opening session, but bind identity lazily. The owner lives outside the Sessions scope and is borrowed by the ordinary Delivery adapters, then reused by Failed run handling after sessions end. A fresh opening session creates its PR, so initial discovery must wait. The first successful post-session observation, normally for the annotation after the push, captures the number and URL; an absent or failed read has not bound anything and may be tried again.
5. Capture ownership separately from target validity. Initial discovery must establish the expected Issue branch in the issue's repository and reject a fork PR. A wrong Base branch still binds that PR, allowing the established Failed run behavior to draft it. Once captured, neither disappearance nor transport failure permits another branch-selected discovery.
6. Offer meaningful PR operations, not raw steps. The module's interface is: write the built-with annotation; mark the identified PR ready; ensure it is still ready and mergeable; attempt Self-merge at a supplied watched head; finish the PR part of a Failed run with the existing keep-ready choice. It returns immutable PR information for reporting and post-merge accounting. Selection, snapshot fields, polling and completion reads sit behind the seam, because callers should not have to sequence those facts.
7. Accept the existing GitHub transport as the production adapter and a scripted adapter in tests. Two adapters justify the internal seam. No general-purpose new transport abstraction is needed, and the outer Delivery/Repair-loop effect seams remain useful for their orchestration tests.

Round 3: observations and transitions

8. Read coherent PR snapshots. One numbered GitHub observation supplies number, URL, state, Issue branch, Base branch, draft state, mergeability, head commit and whether the head is from a fork. Keep the existing branch-selection pull request representation usable; add the richer observation without forcing unrelated callers to learn it. The locality gain comes from validating one snapshot instead of combining a branch-selected state read with a separate mergeability read.
9. Validate afresh at each ordinary gate. Initial readiness requires an open PR with the expected branches, then marks that number ready only if needed. Final readiness requires open, ready, expected branches and mergeable. Each unknown-mergeability poll repeats the whole validation with the existing grace period and interruptible waiting. No earlier snapshot authorizes a later ready or merge transition.
10. Keep annotation and callback semantics. The annotation uses the captured number and the current body, preserving unrelated text. A missing PR is still a no-op for annotation, and annotation failure is still a warning; readiness supplies the decisive error. The before-ready callback stays after annotation and before marking ready, so the Spec PR checklist continues to be restored at the same point.
11. Validate immediately before a numbered merge request. The request retains the merge-commit strategy and watched-head guard from ADR 0004. Distinguish a terminal pre-merge gate error from a refused request: only an actual failed merge request goes through the existing bounded rounds after a failed merge. This prevents an invalid PR from becoming a Policy refusal or spending Repairs.
12. Move failed-request reconciliation into the PR module. The GitHub adapter supplies a single merge attempt and a completion snapshot; the module decides whether the same PR merged at the requested head and expected branches. Completion reads ignore interruption without changing ordinary views or the recorded request. Preserve the current success of an explicit successful guarded merge command; do not add a new mandatory read after every successful request, which would introduce a new failure after a completed merge.
13. State the supported race guarantee precisely. Number selection prevents switching to a replacement PR. Fresh observations catch retargeting during CI, polling and before merge. GitHub's documented merge request offers a head precondition and no expected-base precondition, so a concurrent retarget between the final read and server merge remains a remote race; this Ticket does not claim an atomic target guard or add rollback. This is an inference from the documented parameters and the installed CLI's flags. GitHub merge documentation.

Round 4: failure handling, tests and documentation

14. Finish failure on the captured identity using completion operations. Worktree preservation still happens first. The PR module then reports the captured URL only if that PR is open, and drafts that number unless the existing Policy refusal keep-ready rule applies. Drafting the captured PR is deliberately allowed when its Base branch is wrong: that is salvage, never permission to mark it ready or merge it. If the session failed before any capture, make one completion discovery by Issue branch; after capture, never rediscover. PR-finishing errors remain progress warnings and preserve the primary cause.
15. Keep confirmed Self-merge finishing outside this module. Branch deletion, issue closure and Claim ending already have completion behavior. They run only once Delivery has established merge success; their retry/warning behavior stays as it is. The immutable expected Base branch supplies the closure comment, because the initial PR record must not become a stale source of target policy.
16. Test the interface as the main test surface. Use scripted snapshots and transition results to test retargeting, unknown-mergeability changes, closed/replaced identities, initial wrong targets, missing PRs, draft changes, and failed/interrupted merge confirmation. Assert which PR is affected and the outcome, rather than private parsing or the ordering of helper calls. Prior art is the existing Delivery scripted adapter and GitHub execution tests.
17. Verify the real adapter's selectors. Extend the existing fake GitHub ready and merge commands to accept numeric PR selection while keeping branch selection for fake agents. Numeric merge resolves the selected PR's actual head branch before reading origin. Its view already supports the richer fields. Retain execution tests proving ordinary cancellation and independent completion, adapting them to the new transport shape and moving PR-policy assertions to the new module.
18. Replace old shallow test surfaces and preserve behavior regressions. Move annotation and readiness-policy tests to the new interface, and remove duplicated helper tests and obsolete branch-selected policy paths after their callers migrate. Keep outer Delivery tests for orchestration and Repair-loop tests for budgets/CI; retain CLI regressions for wrong initial targets, late close/draft changes, unknown mergeability, exact-head merging, confirmed interruption, failed confirmation and Policy refusal. Add a Run replacement/retarget case and a Spec PR case to prove the shared production wiring.
19. Make the glossary update part of implementation; no new domain term or ADR is needed. Delivery and Self-merge already name the concepts, and the refactor follows ADRs 0002, 0004 and 0006. This is a reversible module design, not a new hard-to-reverse trade-off.
20. The Strong call has executable evidence. At reviewed main head f03ac2b, a throwaway CLI regression retargeted the PR to develop from the CI-read hook. Its safety assertion failed: exit status was zero, main remained at its initial commit, develop received the feature merge, and thirdshift deleted the Issue branch and closed the issue. All 163 existing relevant tests passed separately: 49 merge/CI/Failed run regressions, 54 Delivery/Repair-loop tests and 60 Spec run regressions. Both report diagrams were validated with the Mermaid parser. The new regression belongs in implementation, with additional interface tests; the implement session must run the full repository checks after the refactor.

Architecture review

Strength: Strong · Dependencies: mock

Files: src/delivery.rs, src/delivery/repair_loop.rs, src/delivery/pull_request.rs (new), src/github.rs, src/github/execution_tests.rs, tests/fakes/gh.rs, tests/merge_run.rs, tests/ci.rs, tests/failed_run.rs, tests/spec_run.rs, CONTEXT.md

Before

flowchart LR
  D["Delivery"] --> R["Repair loop"]
  D --> F["Failed run"]
  D -. "initial target check" .-> G["GitHub adapter"]
  R -. "branch-selected checks and merge" .-> G
  F -. "branch-selected draft" .-> G
  classDef leak stroke:#dc2626,stroke-width:2px;
  class D,R,F leak

After

flowchart LR
  D["Delivery"] --> R["Repair loop"]
  D --> F["Failed run"]
  D --> P
  R --> P
  F --> P
  subgraph P["Delivery pull request module"]
    direction TB
    I["PR identity"] --> T["PR transitions"]
  end
  P --> G["GitHub adapter"]
  classDef deep stroke-width:3px;
  class P deep

Problem: PR identity and target validation leak across the Delivery seam, allowing a PR retargeted during CI to merge into another Base branch.

Solution: Deepen the shallow PR operations into one Delivery pull request module whose implementation owns identity and transitions behind a small interface.

Wins

• Depth: callers request PR outcomes
• Locality: PR invariants concentrate
• Leverage: Run and Spec PR
• Tests cross the same seam
• GitHub adapter stays mechanical

Deletion test: Removing the deep module restores identity, target checks, polling and merge confirmation across three callers.