[JacobStephens2]

Summary

Review's mergeability gate now goes through the pull-request owner. Ordinary Delivery, Fix and Review share one implementation of snapshot validation and mergeability polling. Review's stronger rules live in a private Readiness context, so callers pass no flags.

 Review Delivery (src/processes/review/delivery.rs)
   require_mergeable(head)
-    loop: unchanged_head → pr_snapshot(url) → own ensure! → unchanged_head → decide → poll::sleep
+    PullRequest::on_pull_request(pr, branch, base, github).ensure_review_ready(head)

 PullRequest (src/pull_request.rs)
   ensure_ready_and_mergeable / is_mergeable   → settled_ready_snapshot(Readiness::Delivery)
+  ensure_review_ready(head)                    → settled_ready_snapshot(Readiness::Review { head })
   settled_ready_snapshot(readiness)            # one loop: shared Backoff and adapter pause
+    Review eligibility read (exact head, fixed branch names)
     numbered observation → capture + validate (+ Review: exact URL and head, merged = refusal)
+    Review eligibility read
     decide: Yes / No / Unknown → Delivery: wait until grace ends · Review: wait until GitHub answers
   Adapter
+    review_eligibility(pr)                     # existing GitHub::review_pull_request

What stays the same:

• Ordinary Delivery and Fix: the same messages, the same waiting line and the same bounded grace period.
• Review's diagnostics: pull request changed during Review run readiness, captured pull request disappeared during Review run and the conflict text. A merged target is still a failed check, never AlreadyMerged.
• Review's waiting: no waiting line, and while mergeability is unknown it waits until GitHub answers, as long as the wait is not interrupted.
• The rest of Review: the checks before push and after posting its result, CI watch, Repairs, recommend-only behaviour, the marker and notifications are untouched.

GitHub now derives Clone, so Review can hand the owner an adapter that keeps its interruption policy. docs/code-navigation.md now names the shared readiness owner and its tests.

Evidence

Execution evidence

• Before (e4471a29, the Spec review's diagnostic finding): cargo test --bin thirdshift pull_request::tests::review_readiness_refusal_keeps_its_single_line_diagnostic

  assertion `left == right` failed
    left: "pull request changed during Review run readiness: PR head commit is later, not watched"
   right: "pull request changed during Review run readiness"
  test result: FAILED. 0 passed; 1 failed
  

  After (1b4db089): cargo test --bin thirdshift pull_request::tests

  test result: ok. 46 passed; 0 failed; 0 ignored; 0 measured; 1825 filtered out
  

  The new Review readiness tests use the scripted adapter and go through ensure_review_ready and ensure_ready_and_mergeable. They cover:
  • the exact head and exact URL, compared with ordinary readiness;
  • eligibility changes before and after the numbered observation, including the read order;
  • every common snapshot refusal during Unknown polling, including that a merged target is not AlreadyMerged;
  • a Base branch tip that advances;
  • eventual mergeability, compared with ordinary readiness failing at zero grace;
  • a scripted interrupted pause;
  • the conflict text;
  • a head commit that moves on a later poll.
• cargo test --test review_run: test result: ok. 37 passed; 0 failed; 2 ignored. cargo clippy --all-targets reports no warnings.
• Full suite. The host has no C toolchain, so every run used a node:22 (Debian) container with the host's rustup toolchain.
  • scripts/affected-tests origin/main selects the full Rust suite. I passed origin/main because the local main ref is stale; with main the selector also picked up unrelated site changes.
  • It stopped at the first failing binary, so I reran with cargo test --no-fail-fast: 4414 passed, 103 failed.
  • Running the same 17 failing binaries on origin/main in the same container gave 100 of those 103 failures. They come from the container itself: a session-sweep Permission denied warning, and could not identify the Unix user, because the uid has no passwd entry there.
  • The other 3 (activity_log, launch_pull) passed when rerun on this branch. Their earlier failures came from the same intermittent sweep warning.
  • No failure is in a readiness, Review, Fix or Delivery path. CI on this PR is the clean-environment check.

Merge Danger

Door: two-way

A behaviour-preserving refactor with no data, config or migration changes. Reverting the two commits restores the old Review loop.

Blast Radius: readiness

Every Self-merge (Ticket and Fix Delivery) and every Review run's push and final readiness pass through the changed loop. For Review, the observation now selects the PR by number instead of URL, as ordinary Delivery already does.

Unaddressed findings

Review reports: Standards 8 findings, Spec 5. Addressed:

• Spec 1: Review keeps its single-line diagnostic; the reviewer's test fails before the fix and is kept.
• Spec 2: each refusal during Unknown polling is asserted, and a head moved on a later poll is covered.
• Spec 4: the validation runs are listed above.
• Spec 5 and Standards 8: the double space in docs/code-navigation.md is fixed.
• Standards 2: the Review observation's open-state guard is commented and its refusal collapsed.
• Standards 5: the URL comparison no longer rebuilds a PullRequestUrl.

Standards

• 1. Duplicated eligibility comparison with compatible/unchanged_head. Declined. The spec keeps those owners: "Review's checks before push and after posting its result, CI watch, Repair accounting … continue unchanged", and decision 10 says "push, CI watch, Repairs, result posting, recommend-only checks and final head guards stay with their existing owners". Only the readiness gate moves (src/processes/review/delivery.rs, require_mergeable).
• 3. Three branch points on Readiness, and a no-op eligibility call for Delivery. Declined. Decision 5: "Generalize the private settled-readiness implementation, not the public interface with flags … The internal context carries the genuine differences". Decision 7 requires the shared module to own the read order (settled_ready_snapshot in src/pull_request.rs). A separate Review loop would bring back the second polling implementation that the issue deletes.
• 4. Unreachable Mergeable::Unknown arm in ensure_review_ready. Declined. It now bail!s with the ordinary "has not worked out" message rather than panicking. A new settled-mergeability type would widen the shared snapshot format, which decision 6 rules out: "No additional transport abstraction or expanded mergeability snapshot format is required."
• 6. #[derive(Clone)] on GitHub. No change needed; the reviewer accepted it. A clone keeps the caller's interruption policy (src/github.rs, struct GitHub).
• 7. Divergent Change in src/pull_request.rs. Declined. Decision 1 is "Concentrate readiness in the existing pull-request module", and the module doc and navigation entry say so.

Spec

• 3. #[derive(Clone)] on GitHub is scope creep. Declined. It is the smallest way to give PullRequest::on_pull_request an owned adapter with the Review run's interruption policy (decision 4: "Review can construct the numbered owner from its captured pull request"). It changes no behaviour, and the Standards reviewer accepted it.

Security

None unaddressed. The Security review compared Review readiness before and after the change. It found the same exact-head, exact-URL, number, branch-name, open, draft, fork and Trusted-account refusals, with the eligibility read on every poll, and no new trust boundary.

No finding needed a Spec or Day shift decision, so I filed no issue.

Review coverage: both reviewers read all four changed files. There are no generated-output exclusions and no unread changed files.

Closes #2149

Built with claude 2.1.322 · claude-opus-5-5 · high

Security review outcome

No unaddressed introduced findings.