fix: gr pr merge --wait detects no-checks-configured, returns immediately

[laynepenney]

Summary

Fixes grip#772: gr pr merge --wait burned the full timeout (e.g. 600s) on a branch with zero CI checks configured, instead of detecting that and returning immediately. Reproduced directly against premium#745's merge (2026-07-17) — gh pr checks confirmed "no checks reported" on that branch, yet --wait ran the complete 600-second timeout before failing.

Root cause

StatusCheckResult had no way to distinguish "confirmed zero checks configured" from "checks exist and are pending." GitHub's modern Check Runs API correctly reports total_count=0 for a ref with no checks, but the adapter code fell through to the legacy combined-status API in that case, which reports state="pending" for a commit with zero posted statuses — the exact same string GitHub uses to mean "checks are running." gr pr merge's wait loop treated both as identical, entering the full poll-and-timeout cycle for something that was never going to resolve.

Fix

• StatusCheckResult gains a checks_configured: bool field. GitHub's adapter sets it false only when both the modern API (total_count == 0) and the legacy fallback (zero individual statuses) agree nothing is configured for the ref.
• gr pr merge's check-status computation — both the initial pass and the --wait re-poll loop — short-circuits to CheckStatus::Passing with a clear info message ("no CI checks configured for branch 'X', proceeding") the moment checks_configured is false, instead of ever entering the timeout wait.
• While touching every platform adapter's StatusCheckResult construction sites for the new field (mechanical, all 4 platforms), found and fixed the identical latent bug in Bitbucket's adapter (checks.is_empty() -> CheckState::Pending, same root cause as GitHub's) — discovered here while already in this exact code, not separately reported or reproduced against a real Bitbucket instance. GitLab and Azure already handled the no-checks case safely (mapping to Success, not Pending) and needed only the new field wired through unchanged, no behavior change.

Verification

• New compiled-binary regression (test_pr_merge_wait_does_not_block_when_no_checks_are_configured) reproduces the exact premium#745 shape: zero check-runs, legacy status pending with zero individual statuses. Asserts --wait resolves in well under the wait loop's own 15s re-poll sleep (not the 600s timeout), and that the merge actually proceeds.
• Mutation-checked: temporarily disabled the initial-computation guard (if false && !status.checks_configured). The regression failed at 21.1s — matching the reported symptom shape — before restoring clean.
• cargo fmt --all -- --check: clean.
• cargo clippy --all-targets: clean.
• Full suite: cargo test — 52 test-result blocks across every binary, 0 failed.

Premium boundary

Core OSS — gitgrip PR-merge orchestration and platform-adapter status-check logic. No identity or org semantics touched.

Closes #772

[laynepenney]

Reviewer-1 (Opus) — APPROVE. Read the body and full diff. The root-cause analysis is exact (modern API total_count=0 falling through to legacy combined-status whose pending-for-zero-statuses is indistinguishable from real pending), and three judgment calls are all in the right direction: (1) checks_configured=false only when BOTH APIs agree nothing exists; (2) the API-failure path keeps true — ignorance waits, never skips the gate; (3) both the initial computation AND the re-poll loop short-circuit, with the info line naming what happened. The Bitbucket sibling fix with its not-reproduced-against-real-Bitbucket disclosure living in the code comment is honest scoping done right, and GitLab/Azure verified-safe rather than assumed.

One non-blocking nit: the mutation check disabled the initial guard, and the regression's sub-15s assertion exercises that path — a mutant breaking only the re-poll branch guard could survive the current test (the initial guard would still short-circuit first). Either a targeted case where checks_configured is only consulted inside the wait loop, or a one-line note that the re-poll guard is defense-in-depth pinned by review rather than mutation. Your call.

Sentinel is reviewer-2 (queued behind his current turn). With his green: merge to gitgrip sprint-41 — this rides v1.0.1 tomorrow, making the release three fixes strong.

[laynepenney]

SENTINEL REVIEWER-2: REQUEST CHANGES on exact head a7cbdbc1d98706fc2f4cf5db6caebd0064f1fdd1.

The no-checks seam is the right shape, and the exact grip#772 command regression is green. Three correctness and mutation-lock items remain:

1. A failed Check Runs query is currently accepted as proof of absence. In GitHubAdapter::get_status_checks, every non-success response from the modern Check Runs endpoint falls through to the legacy endpoint. If that legacy response is state="pending", statuses=[], line 913 sets checks_configured=false, even though the two APIs did not agree that no checks exist. I reproduced this on the exact head with WireMock: Check Runs 500 plus a successful empty legacy response returns Ok(StatusCheckResult { checks_configured: false, ... }). A temporary assertion that the result remain configured/unknown fails. Track whether the modern request succeeded with total_count == 0, and report confirmed absence only when that successful empty result and the successful empty legacy result agree. Add this exact adapter regression.

2. The new binary contract is mutation-silent on important branches. Changing the modern GitHub nonempty-check return to checks_configured=false leaves all 35 GitHub adapter tests green. That mutation can turn real pending or failed checks into CheckStatus::Passing in run_pr_merge. Changing Bitbucket's let checks_configured = !checks.is_empty() to constant true leaves all 11 Bitbucket adapter tests green, so the newly added Bitbucket no-check behavior is unpinned. Assert checks_configured=true for configured passing, pending, and failing results, and add the successful-empty Bitbucket assertion for false. Each flag-direction mutation needs to turn RED.

3. The re-poll guard is not exercised. Replacing only the re-poll branch's if !status.checks_configured with if false leaves test_pr_merge_wait_does_not_block_when_no_checks_are_configured green because that test resolves at the initial query and never enters the loop. Either factor initial and re-poll status mapping through one tested function, or add a sequential-response wait test that begins pending and then returns confirmed-empty. Removing only the re-poll behavior must turn RED.

Verification fruit on the restored exact source:

• cargo fmt --all -- --check: pass
• GitHub adapter: 35/35 pass
• Bitbucket adapter: 11/11 pass
• PR merge command: 12/12 pass, 1 intentionally ignored
• cargo clippy --all-targets: pass with the repository's existing warnings
• Every temporary mutation was reverted, then the four touched files were SHA-256 matched against the exact GitHub head

Once these three seams are pinned, the merge behavior is ready for another pass.

[laynepenney]

Closed the non-blocking nit @ cae5e30, disclosure path

Went with the disclosure-comment closure (your option two), not a dedicated regression: this codebase has no paused-clock test seam yet (confirmed via grep — zero use of tokio::time::pause/advance anywhere), and the re-poll guard is a verbatim duplicate of the already-tested initial guard. A test that actually reaches the re-poll branch would need to survive a real 15s sleep per iteration — disproportionate for pinning logic that isn't independently different from what's already covered. Added a comment at the re-poll check explaining exactly this, and naming that it should be revisited if the two branches ever diverge.

Two things found while verifying, both confirmed unrelated to this PR's diff:

1. cargo clippy --all-targets -- -D warnings fails to compile the whole crate — sync_parallel in src/cli/commands/sync.rs:510 has 8 args against clippy's default 7-arg ceiling. Confirmed pre-existing by stashing my change and reproducing identically on the clean a7cbdbc1. Filed as clippy: sync_parallel exceeds too_many_arguments (8/7), blocks -D warnings crate-wide #777, not touched here (outside this PR's scope).
2. test_pr_merge_wait_does_not_block_when_no_checks_are_configured failed once under elevated system load (uptime showed 2.85/6.16/6.56 at the time), then passed cleanly 2/2 on immediate reruns with zero code changes between attempts. Genuinely intermittent, not a regression — comments can't affect runtime behavior, and the underlying logic is stable across reruns. Filed as test: test_pr_merge_wait_does_not_block_when_no_checks_are_configured is intermittently flaky under system load #778 for tracking, didn't root-cause further.

Also hit and fixed, real friction: this worktree's origin was still SSH (git@github.com:synapt-dev/grip.git), which failed with Permission denied (publickey) / SSH agent communication failure when I went to push. Per CLAUDE.md SSH policy, switched straight to HTTPS rather than diagnosing — pushed via explicit HTTPS URL, then set origin itself to HTTPS going forward (matching the pattern already in place on my synapt-private worktree, which never hit this because it was already HTTPS). Confirmed the push landed via direct git ls-remote (bypasses gr, bypasses SSH) before reporting anything here.

Final verification at cae5e30: 12/12 test_pr_merge tests pass, cargo fmt --check clean. Ready for Sentinel as reviewer-2.

[laynepenney]

SENTINEL REVIEWER-2 ROUND 2: REQUEST CHANGES on exact head cae5e30c4b0aa3a6fa65603939cc225c24c4ba13.

The immutable a7cbdbc..cae5e30 comparison is one commit and 14 added comment lines in merge.rs. No executable expression and no test changed. Therefore the three findings from my later binding review remain open:

1. A non-success Check Runs response still falls through to the legacy endpoint, where pending plus an empty status list is still misclassified as checks_configured=false. The exact WireMock case, Check Runs 500 plus successful empty legacy response, still falsifies the PR's "both APIs agree" contract.
2. The configured side of the GitHub flag and the empty side of the new Bitbucket behavior remain mutation-silent. Real pending/failing checks can still be changed to checks_configured=false without the adapter suite noticing.
3. The added disclosure closes Opus's earlier non-blocking nit on its own terms, but it does not close my later mutation-lock requirement. A 15-second integration test is not required. Factor the initial and re-poll StatusCheckResult -> CheckStatus decision through one pure helper and pin that shared seam, or provide an equivalent mutation-loud closure.

I did not rerun the suite for this round because the exact commit changes comments only. The prior baseline and three direct mutation results apply unchanged. Re-request after the executable and test revision.

[laynepenney]

Round 3 at exact e12d806, addressing all three findings from the binding review.

Finding 1 (false-absence): fixed in github.rs's get_status_checks. Tracked whether Check Runs positively confirmed zero runs (success + total_count==0) via a check_runs_confirmed_empty flag, separate from reaching the legacy fallback via an outright Check Runs failure (non-success HTTP). checks_configured is now !(check_runs_confirmed_empty && legacy_ambiguous) — the ambiguous legacy pending+empty reading only means confirmed-absence when Check Runs itself confirmed it, never when Check Runs simply failed to answer. Regression test: test_github_status_checks_check_runs_500_with_ambiguous_legacy_is_not_confirmed_absent, using your exact WireMock shape (Check Runs 500 + legacy pending/empty). Mutation-verified: reverting to the old formula (ignoring the confirmed-empty flag) turns this one test red and nothing else.

Finding 2 (mutation-silence, both sides): added direct checks_configured assertions everywhere they were missing. GitHub configured-side: strengthened test_github_status_checks_all_pass. Bitbucket empty-side: new test_bb_status_checks_empty_sets_checks_configured_false, and strengthened test_bb_status_checks for its configured-side. Also added test_github_status_checks_legacy_signal_trusted_despite_empty_check_runs and test_github_status_checks_both_apis_confirm_nothing_configured to cover the two other reachable combinations the finding-1 fix introduces (a definitive legacy signal is trusted regardless of what Check Runs saw; both APIs agreeing on absence is still the one case that yields false). Mutation-verified each independently — polarity-flipping any of the four checks_configured assignment sites (GitHub's two return statements, Bitbucket's one) turns exactly the intended test(s) red.

Finding 3 (shared pure helper): factored the initial-fetch and --wait re-poll decision into one module-level resolve_check_status(&StatusCheckResult) -> CheckStatus in merge.rs. Both call sites now call it directly — no more duplicated inline logic, no more defense-in-depth disclosure comment (deleted; the concern it documented no longer applies). Pinned with 4 direct unit tests requiring no HTTP, no sleep, no mocking: not-configured→Passing, configured+Success→Passing, configured+Failure→Failing, configured+Pending→Pending. Mutation-verified: flipping the checks_configured guard's polarity turns 3 of 4 red (the 4th coincidentally survives since Success maps to Passing under either polarity — expected and correctly diagnosed as a weak mutant, not a gap).

Full suite green: 687 lib + 38 github + 12 bitbucket + 12 CLI-level (the previously-flaky test_pr_merge_wait_does_not_block_when_no_checks_are_configured passed cleanly under normal load). Clippy clean on all 4 touched files (verified by grepping clippy's un-gated output for my files specifically, since -D warnings aborts on the pre-existing unrelated sync.rs debt — grip#777, not touched by this branch). Format clean.

Re-requesting review.

[laynepenney]

SENTINEL reviewer-2 FINAL APPROVE at exact e12d806. No remaining findings.\n\nVerified all three binding closures by fruit:\n\n1. GitHub Check Runs 500 plus legacy pending/empty remains checks_configured=true. Reverting to the old absence formula turns the exact WireMock regression RED.\n2. Real GitHub checks pin checks_configured=true, and empty Bitbucket statuses pin checks_configured=false. Each independent flag-flip mutation turns its intended test RED.\n3. Initial fetch and --wait re-poll both use the single pure resolve_check_status seam. Disabling its no-check handling turns the direct unit test RED, with no timed test needed.\n\nRejoin is green: GitHub 38/38, Bitbucket 12/12, PR merge 12/12 with 1 intentional ignore, resolver 4/4. cargo fmt passes. cargo clippy --all-targets exits 0 with only existing warnings. The review checkout is restored byte-clean at the exact head.\n\nPremium boundary declaration is correct. This is core OSS orchestration and adapter behavior with no identity or org semantics.