Deepen pull-request readiness for Review runs #2149

What to build

Deepen the existing pull-request module so a Review run checks readiness through the same implementation of captured identity, ownership, open state, draft state and Base branch validation used by Ticket Delivery and Fix runs. Today Review implements those checks and its mergeability polling separately, so a change to the readiness rules must be carried to both implementations.

Keep the existing behavior of each caller: a Review run stands behind the exact head it reviewed or pushed, refuses an ineligible pull request, and leaves conflicts for the author to update. Ordinary Delivery and Fix retain their current readiness and Self-merge behavior. This is one standalone Ticket, sized for one fresh session; it needs no release or host action.

Acceptance criteria

• Review's mergeability gate delegates to the existing pull-request module. Delete its independent snapshot-readiness predicate and polling loop; ordinary Delivery, Fix and Review share the common snapshot validation and mergeability polling implementation.
• The shared module exposes a purposeful Review readiness operation taking the exact expected head. The existing numbered owner supplies the captured repository, pull-request number and URL, head branch and Base branch. Caller-visible boolean options or a general callback protocol are unnecessary.
• Each Review poll preserves this order: fresh Review eligibility and exact-head check; numbered mergeability observation; validation of that observation; fresh Review eligibility and exact-head check; mergeability decision. Validation failure stops before the final eligibility read. Existing trust checks run on every poll, and failed or missing observations cannot grant readiness.
• Review refuses a changed number or exact URL, changed head branch or Base branch name, changed head commit, fork, draft, closed or merged target, or author who is no longer a Trusted account. Base branch tip advancement alone remains allowed. A merged target remains a failed Review readiness check, never successful completion through the ordinary Delivery AlreadyMerged handling.
• Review retains interruptible waiting while mergeability is Unknown, without imposing ordinary Delivery's grace deadline. Ordinary Delivery and Fix keep their bounded grace behavior. Preserve each caller's conflict/failure diagnostics and waiting-line behavior, and the existing state-sensitive Backoff cadence.
• Review's checks before push and after posting its result, CI watch, Repair accounting, recommend-only behavior, publication marker, notification and retained-work behavior continue unchanged. The shared readiness operation neither marks a draft ready nor merges, pushes, repairs or comments.
• Tests through the shared module's caller-facing readiness operations cover Review's exact-head/exact-URL requirements, eligibility changes before and after a numbered observation, all common snapshot refusals during Unknown polling, allowed Base tip advancement, eventual mergeability, interruption and ordinary bounded versus Review unbounded waiting. Use the existing scripted adapter with deterministic observations and pauses, rather than real timing deadlines.
• Keep the existing CLI regressions for Review CI/readiness, author and head changes, Base branch advancement, conflicts, push leases and final publication. Keep ordinary Delivery/Fix readiness and merge regressions. Replace any superseded private readiness-helper tests with interface tests; do not duplicate them at every internal step.
• Update the domain-to-code navigation entry to identify the shared readiness owner and its tests. No new glossary term or ADR is needed: this deepens an existing module and preserves the current decisions. If user-facing wording needs correction to describe the preserved behavior, update its source and any generated output together.
• Run the focused pull-request module and Review CLI tests during implementation, then scripts/affected-tests main for final local validation as required by the repository.

Blocked by

None (can start immediately).

Decisions

The design tree was settled in three rounds. Each round resolved the full frontier below; no decision requires a person.

Round 1: scope and ownership

1. Concentrate readiness in the existing pull-request module. It already owns numbered capture, ownership validation, readiness polling and ordinary Delivery/Fix behavior. Extending it deletes a second implementation; extracting only a comparison helper would leave ordering and waiting knowledge in Review and produce little depth.
2. Preserve behavior rather than choose a new waiting policy. Review currently waits on Unknown until a known answer or interruption; ordinary Delivery has a grace deadline. Both are supported by the current code. Preserve that distinction as an internal, explicit readiness context, because standardizing it would be a separate user-visible change.
3. Keep the work to one standalone Ticket. The change is confined to readiness ownership, its existing observation adapter, one Review delegation and their tests. It needs neither staged rollout nor an integration branch. A Spec and child Tickets would add no independently useful slice.

Round 2: interface, observations and outcomes

4. Use a Review-specific operation on the existing numbered pull-request owner. Its only additional caller knowledge is the expected head. The module chooses Review's internal readiness context; normal callers retain their existing operations. Review can construct the numbered owner from its captured pull request without altering Review admission or session state.
5. Generalize the private settled-readiness implementation, not the public interface with flags. Share snapshot validation, observation sequencing and mergeability polling. The internal context carries the genuine differences: Review's exact head/URL, extra eligibility observations, wait limit, diagnostics and progress policy. Other pull-request operations keep their existing validation and typed outcomes.
6. Extend the existing private observation adapter with the existing Review eligibility read. The production GitHub adapter and scripted test adapter already justify this seam. Keep the Review eligibility read's Trusted account and repository checks in their existing implementation. The pull-request module compares its returned branch names and head against the capture. No additional transport abstraction or expanded mergeability snapshot format is required.
7. Retain the two fresh eligibility reads around the validated snapshot. Review's current sequence catches a moved head or changed trust before and after the numbered observation. Moving all three reads into an opaque adapter call would move validation after the final read and change failure ordering. The shared module owns that order instead.
8. Retain Review's stronger exact-head and exact-URL constraints. Ordinary Delivery can observe a new branch head as work progresses and validates a URL structurally. Review is endorsing one delivered head and currently requires the captured URL exactly. These are explicit context rules, not reasons for a second ownership implementation.
9. Keep Base branch names fixed while allowing its tip to advance. Review compares CI with the Base commit contained by its branch and intentionally does not require a stable Base tip. Comparing the entire Review metadata record would incorrectly reject valid work.
10. Keep consumer outcomes and side effects unchanged. A merged target fails Review even though ordinary Delivery has typed merged-completion reconciliation. Review conflicts retain the instruction for the author to update the branch. Preserve the current ordinary waiting lines and Review diagnostics. Readiness remains read-only; push, CI watch, Repairs, result posting, recommend-only checks and final head guards stay with their existing owners.
11. Keep Backoff and interruption policy at their existing seams. The shared readiness implementation uses the existing cadence and adapter pause. Review's unbounded wait never becomes an uninterruptible wait, and ordinary grace handling is unchanged. Reimplementing cadence here would create another duplicated policy.

Round 3: testing, documentation and completion

12. The interface is the test surface. Exercise the normal and Review readiness operations through the existing scripted adapter, including pre-read and post-validation eligibility outcomes. Test the returned refusal/readiness and observed effects, not extracted predicate helpers. This concentrates verification with the policy it protects.
13. Use deterministic observations to distinguish wait policies. With zero scripted ordinary grace, Unknown must fail ordinary readiness; the same Review setup must continue through Unknown to a later mergeable answer. A scripted pause interruption must stop Review. No real sleeps or short subprocess deadlines are needed. Preserve the existing Backoff tests rather than reproducing their timing matrix here.
14. Retain end-to-end evidence at the CLI seam. Existing Review tests already cover changes during CI, Base advancement, conflicts, receive-time movement, local preservation and final publication. They prove the caller uses the shared gate without losing Review behavior. Readiness-specific interleavings belong in scripted adapter tests; the fake GitHub tool needs no new generic read hook.
15. Update navigation; do not invent a domain concept or reopen an ADR. Pull-request ownership and Review runs already exist. This reversible refactor preserves the glossary's Review/Delivery semantics and aligns with ADR 0024's single implementation of safety rules. It does not merit a new architectural decision record. Documentation edits belong to this Ticket's implementation, not the Architecture review.
16. Validate the implementation at its scope, then use repository selection. Run the pull-request module and Review CLI tests for fast feedback, followed by the required affected-tests runner. This Architecture review changed no repository source and makes no claim that implementation tests have already run.

Architecture review

Strength: Strong · Dependencies: mock

Files: src/pull_request.rs, src/processes/review/delivery.rs, src/processes/review.rs, src/github/review.rs, tests/review_run.rs, docs/code-navigation.md

Before

flowchart LR
  T[Ticket Delivery] --> P[Pull-request module]
  F[Fix Delivery] --> P
  R[Review Delivery] -. readiness implementation .-> G[GitHub adapter]
  P --> G
  P --> S[Scripted adapter]
  classDef leak stroke:#dc2626,stroke-width:2px;
  class R leak

After

flowchart LR
  T[Ticket Delivery] --> P[Pull-request module]
  F[Fix Delivery] --> P
  R[Review Delivery] -->|Review readiness interface| P
  P --> G[GitHub adapter]
  P --> S[Scripted adapter]
  classDef deep stroke-width:3px;
  class P deep

Problem: Review's shallow readiness module repeats ownership and polling implementation across a seam already owned by the pull-request module.

Solution: Increase the existing module's depth with one Review readiness interface, retaining its stronger eligibility rules behind the existing adapter seam.

Wins

• Locality: one snapshot validation implementation
• Leverage: Delivery, Fix and Review
• Tests cross the caller interface

Evidence: At review base b439833359987333528e93d71dcf3a426dbf7d2b, Review readiness repeats the checks and polling owned by ordinary readiness and ownership validation; both polling implementations changed in #2138.

Deletion test: Deleting the resulting shared module would spread the captured-identity, readiness and polling rules back into Delivery, Fix and Review; deleting only a comparison helper would leave that complexity distributed.

Priority: Both implementations are active change sites, their differences are explicit in the code, and the entire behavior-preserving design is settled without a product decision.

Covered by: None. All 188 open issue titles and bodies were checked and the issue inventory was refreshed immediately before publication; no open issue proposes this deepening. ADR 0024 is respected.