fix: mark_ready needs green checks, not just a clean merge

Found by running the rule against the live fleet on its first pass, which is
the only reason it was caught before the PR merged.

`mergeable` reports whether the branch merges cleanly and nothing else, so it
reads MERGEABLE while CI is still running or has failed outright. #633 was
APPROVED, MERGEABLE and had Algorithm tests and E2E still IN_PROGRESS, and the
rule duly reported "gh pr ready 633 — then it is the maintainers to merge".
GitHub itself disagreed: `mergeStateStatus` was BLOCKED.

Flipping a red or pending PR out of draft is worse than leaving it there.
`ready` is supposed to mean the maintainer can merge without checking anything
else, and that claim is the only thing making the flag worth setting.

So `mark_ready` now also requires every check to have concluded SUCCESS,
SKIPPED or NEUTRAL. SKIPPED is green on purpose: this repo path-filters
Algorithm tests and Docker build, so every backend-only PR skips them and
treating that as not-green would withhold the action from almost everything.
An empty rollup is green too — a PR with no checks configured has nothing
failing.

The deferred list mirrors the same condition, so an approved-but-pending PR
with a P4 card appears in exactly one place rather than both.

Live effect: #633 falls back to resume_implementation until its CI settles,
which is the correct answer and the one the first version got wrong.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012LExo6fcbup75vtc9NfoAR

Author: johanzander

backend/tests/test_backlog_rhythm.py

@@ -364,6 +364,10 @@ def _pr(number: int, **over: object) -> dict:
         "reviewDecision": None,
         "reviews": [],
         "author": {"login": "johanzander"},
+        # Defaults to green, so a test that cares about checks says so. An
+        # empty rollup also reads as green, which is correct: a PR with no
+        # checks configured has nothing failing.
+        "statusCheckRollup": [{"name": "CI", "conclusion": "SUCCESS"}],
     }
     pr.update(over)
     return pr
@@ -645,3 +649,71 @@ def test_a_pr_with_no_card_is_reported_as_before(tmp_path: Path) -> None:
 
     assert "resolve_conflict" in _actions_for(result, 614)
     assert result["deferred"] == []
+
+
+def test_mark_ready_needs_green_checks_not_just_a_clean_merge(
+    tmp_path: Path,
+) -> None:
+    """`mergeable` reports only whether the branch merges cleanly, so it reads
+    MERGEABLE while checks are still running. #633 was APPROVED and MERGEABLE
+    with Algorithm tests and E2E in progress, and the first version of this
+    rule duly said to flip it — which would hand the maintainer a PR marked
+    ready whose CI had not finished."""
+    pr = _pr(
+        633,
+        isDraft=True,
+        reviews=[{"state": "APPROVED"}],
+        statusCheckRollup=[
+            {"name": "Fast tests", "conclusion": "SUCCESS"},
+            {"name": "Algorithm tests", "conclusion": ""},
+        ],
+    )
+    actions = _actions_for(_run(tmp_path, [], [pr]), 633)
+    assert "mark_ready" not in actions
+    assert "resume_implementation" in actions
+
+
+def test_mark_ready_is_withheld_on_a_failing_check(tmp_path: Path) -> None:
+    pr = _pr(
+        634,
+        isDraft=True,
+        reviews=[{"state": "APPROVED"}],
+        statusCheckRollup=[
+            {"name": "Fast tests", "conclusion": "SUCCESS"},
+            {"name": "E2E tests", "conclusion": "FAILURE"},
+        ],
+    )
+    assert "mark_ready" not in _actions_for(_run(tmp_path, [], [pr]), 634)
+
+
+def test_a_skipped_check_still_counts_as_green(tmp_path: Path) -> None:
+    """Path-filtered jobs correctly do not run — every backend-only PR in this
+    repo skips Algorithm tests and Docker build, so treating SKIPPED as
+    not-green would withhold mark_ready from almost every PR."""
+    pr = _pr(
+        635,
+        isDraft=True,
+        reviews=[{"state": "APPROVED"}],
+        statusCheckRollup=[
+            {"name": "Fast tests", "conclusion": "SUCCESS"},
+            {"name": "Algorithm tests", "conclusion": "SKIPPED"},
+        ],
+    )
+    assert "mark_ready" in _actions_for(_run(tmp_path, [], [pr]), 635)
+
+
+def test_a_pending_approved_draft_is_not_counted_as_deferred(
+    tmp_path: Path,
+) -> None:
+    """The deferred list mirrors the mark_ready carve-out, so a card-deferred
+    PR that is approved-but-pending must appear in exactly one place."""
+    pr = _pr(
+        636,
+        isDraft=True,
+        reviews=[{"state": "APPROVED"}],
+        statusCheckRollup=[{"name": "CI", "conclusion": ""}],
+    )
+    result = _run(tmp_path, [], [pr], pr_board=[_card(636, priority="P4")])
+
+    assert _actions_for(result, 636) == set()
+    assert [d["pr"] for d in result["deferred"]] == [636]

scripts/backlog-rhythm.sh

@@ -59,7 +59,7 @@ if [ -n "${RHYTHM_PRS_FILE:-}" ]; then
     prs=$(cat "$RHYTHM_PRS_FILE")
 else
     prs=$(gh pr list --repo "$repo" --state open --limit 100 \
-        --json number,title,isDraft,mergeable,reviewDecision,reviews,author)
+        --json number,title,isDraft,mergeable,reviewDecision,reviews,author,statusCheckRollup)
 fi
 
 actions=$(printf '%s' "$digest" | jq \
@@ -250,7 +250,21 @@ actions=$(printf '%s' "$digest" | jq \
       | ([ .reviews[]? | select(.state != "COMMENTED") ] | last | .state?) as $verdict
       | (.reviewDecision == "APPROVED"
          or ($verdict == "APPROVED" and .reviewDecision != "CHANGES_REQUESTED")) as $is_approved
-      | (.isDraft and $is_approved and .mergeable != "CONFLICTING") as $approved_draft
+      # CI MUST BE GREEN, not merely unconflicted. `mergeable` reports only
+      # whether the branch merges cleanly, so it reads MERGEABLE while checks
+      # are still running or have failed -- #633 was APPROVED and MERGEABLE
+      # with Algorithm tests and E2E still in progress, and an earlier version
+      # of this rule duly said to flip it. Flipping a red or pending PR ready
+      # is worse than leaving it draft: `ready` is supposed to mean the
+      # maintainer can merge without checking anything.
+      #
+      # SKIPPED counts as fine (a path-filtered job that correctly did not
+      # run); anything still QUEUED or IN_PROGRESS is not a verdict yet.
+      | ([ .statusCheckRollup[]?
+           | select((.conclusion // "") != "SUCCESS" and (.conclusion // "") != "SKIPPED"
+                    and (.conclusion // "") != "NEUTRAL") ] | length == 0) as $checks_green
+      | (.isDraft and $is_approved and $checks_green
+         and .mergeable != "CONFLICTING") as $approved_draft
       | (if $approved_draft
          then {pr: .number, action: "mark_ready",
                why: "APPROVED and green, still a draft — Step 11 never ran gh pr ready",
@@ -355,6 +369,10 @@ actions=$(printf '%s' "$digest" | jq \
         # counted here as well.
         | select((.isDraft
                   and .mergeable != "CONFLICTING"
+                  and ([ .statusCheckRollup[]?
+                         | select((.conclusion // "") != "SUCCESS"
+                                  and (.conclusion // "") != "SKIPPED"
+                                  and (.conclusion // "") != "NEUTRAL") ] | length == 0)
                   and (.reviewDecision == "APPROVED"
                        or (([ .reviews[]? | select(.state != "COMMENTED") ] | last | .state?) == "APPROVED"
                            and .reviewDecision != "CHANGES_REQUESTED"))) | not)