johanzander
diff --git a/‎.claude/skills/backlog/SKILL.md‎
Lines changed: 31 additions & 1 deletion b/‎.claude/skills/backlog/SKILL.md‎
Lines changed: 31 additions & 1 deletion
diff --git a/‎backend/tests/test_backlog_digest.py‎
Lines changed: 57 additions & 0 deletions b/‎backend/tests/test_backlog_digest.py‎
Lines changed: 57 additions & 0 deletions
diff --git a/‎backend/tests/test_backlog_rhythm.py‎
Lines changed: 201 additions & 10 deletions b/‎backend/tests/test_backlog_rhythm.py‎
Lines changed: 201 additions & 10 deletions
@@ -204,6 +204,7 @@ Actions, and who does what:
 | Action | Do |
 |---|---|
 | `resume_implementation` | `/implement-issue <n>`. **The action that produces a ready PR** — Step 11 requests the review, acts on the verdict and runs `gh pr ready`. Covers a draft needing a first review, a rework, an approved PR that never got flipped, *and* a worktree whose session died |
+| `mark_ready` | `gh pr ready <n>`, then report it. APPROVED, green, still a draft — the loop stopped one command short. **The one action no board decision can defer**, because it is a pipeline failure rather than a priority |
 | `awaiting_maintainer` | nothing; report it. Out of draft **and carrying an APPROVED review** is the finish line |
 | `request_review` | out of draft but Stage 4 never ran. `scripts/request-pr-review.sh <n>`. **Never report an unreviewed PR as ready to merge** — the draft flag is not a review, and a maintainer who flips it because a PR looks stuck routes around the one gate the pipeline is built on |
 | `rework_review` | out of draft with changes requested: `/implement-issue <n>` to address them, then a fresh review |
@@ -217,11 +218,40 @@ Actions, and who does what:
 
 **This pass does not drive the review loop, and must not learn to.**
 `implement-issue` owns a PR from its first commit to `gh pr ready`; Step 11
-already requests the review, acts on the verdict and flips the PR. So every
+already requests the review, acts on the verdict and flips the PR. So an
 unfinished draft resolves to one action — hand it back — and a second copy of
 that loop is never built here. It is the same argument that put resume in Step 0
 instead of a separate skill: two copies of one loop means one of them goes stale.
 
+**The one carve-out is `mark_ready`, and it earned it.** An APPROVED, green,
+still-draft PR used to hand back like any other, and that is exactly why #629
+sat finished-but-draft: the remedy on offer was a whole `implement-issue`
+session, and nobody spends one of those to run a single command. `gh pr ready`
+is a terminal action, not a loop, so naming it here duplicates nothing.
+
+## Deferring a PR — how a decision gets recorded once
+
+**PRs go on the board too, and carry the same `Priority` and `Awaiting` fields
+issues do.** Before that, a judgement about a PR had nowhere to live: "#167 and
+#354 are blocked", "#437 and #490 are lower priority, later" were real
+decisions, and every pass re-reported all four as due because nothing recorded
+them. The same conversation happened every 30 minutes.
+
+| Decision | Set on the PR card | Effect |
+|---|---|---|
+| blocked / parked on a call | `Awaiting: discussion` (or `upstream`) | suppressed from actions |
+| later, not never | `Priority: P4` | suppressed from actions |
+| actively being driven | no `Awaiting`, `P1`–`P3` | reported every tick |
+
+Suppressed PRs are **counted and listed**, never dropped — the pass ends with
+`deferred: 4 (#490 priority P4; #167 awaiting discussion; …)` so the item stays
+findable and the reason travels with it. Losing the item would trade one
+failure for another.
+
+`mark_ready` ignores all of this, per the carve-out above. `awaiting_maintainer`
+does not: an approved PR waiting on a merge is not broken, it is the
+maintainer's call when to take it, and `P4` is how they say "later".
+
 **Restarting a stalled issue is always a resume, never a fresh start.** Step 0
 re-enters at the earliest incomplete step. A restart runs Step 4, which branches
 from `origin/main` and deletes commits that exist nowhere else — an audit found 8
 
@@ -901,6 +901,63 @@ def test_issue_with_no_card_reports_null_status_and_is_an_orphan(
     assert [o["ref"] for o in orphans] == ["624"]
 
 
+def test_pr_cards_are_emitted_separately_from_issue_cards(bin_dir: Path) -> None:
+    """`content.type` is the load-bearing bit, and it is confirmed against a
+    real card rather than assumed: an added PR reports "PullRequest" with
+    number/title/url/repository alongside it.
+
+    Board membership for PRs is what gives a decision about a PR somewhere to
+    live -- without it "#437 is lower priority" had nowhere to be recorded, so
+    every rhythm pass re-reported it as due.
+    """
+    issue = _issue(601)
+    cards = [
+        {
+            "content": {"number": 601, "type": "Issue"},
+            "status": "Backlog",
+            "priority": "P2",
+        },
+        {
+            "content": {"number": 437, "type": "PullRequest"},
+            "status": "In Review",
+            "priority": "P4",
+        },
+        {
+            "content": {"number": 167, "type": "PullRequest"},
+            "status": "Backlog",
+            "awaiting": "discussion",
+        },
+    ]
+    _write_shim(bin_dir, "gh", _gh_shim([issue], [], cards))
+
+    digest = _run(bin_dir)
+
+    assert digest["pr_board"] == [
+        {
+            "number": 437,
+            "board_status": "In Review",
+            "priority": "P4",
+            "awaiting": None,
+        },
+        {
+            "number": 167,
+            "board_status": "Backlog",
+            "priority": None,
+            "awaiting": "discussion",
+        },
+    ]
+    # The issue card is untouched by the split and still drives the item.
+    assert digest["items"][0]["priority"] == "P2"
+
+
+def test_pr_board_is_empty_when_no_prs_are_carded(bin_dir: Path) -> None:
+    issue = _issue(601)
+    card = {"content": {"number": 601, "type": "Issue"}, "status": "Backlog"}
+    _write_shim(bin_dir, "gh", _gh_shim([issue], [], [card]))
+
+    assert _run(bin_dir)["pr_board"] == []
+
+
 def test_a_locked_worktree_is_reported_as_locked(bin_dir: Path) -> None:
     """The lock is the liveness signal `claude agents` cannot provide: it lists
     background agents only, so a foreground `/implement-issue` is invisible and
 
@@ -64,9 +64,27 @@ def _comment(days: int, *, is_reporter: bool = False, is_bot: bool = False) -> d
     }
 
 
-def _run(tmp_path: Path, items: list, prs: list | None = None, **env: str) -> dict:
+def _run(
+    tmp_path: Path,
+    items: list,
+    prs: list | None = None,
+    pr_board: list | None = None,
+    **env: str,
+) -> dict:
     digest = tmp_path / "digest.json"
-    digest.write_text(json.dumps({"counts": {}, "items": items, "orphans": []}))
+    digest.write_text(
+        json.dumps(
+            {
+                "counts": {},
+                "items": items,
+                "orphans": [],
+                # Board cards for PRs. Defaults to empty, which is also what an
+                # older digest produces -- the script tolerates its absence, so
+                # every pre-existing test exercises the no-deferral path.
+                "pr_board": pr_board or [],
+            }
+        )
+    )
     prs_file = tmp_path / "prs.json"
     prs_file.write_text(json.dumps(prs or []))
 
@@ -346,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
@@ -356,19 +378,24 @@ def test_every_unfinished_draft_resolves_to_one_handoff(tmp_path: Path) -> None:
     through to `gh pr ready`, and its Step 11 already requests the review, acts
     on the verdict and flips the PR.
 
-    So a draft needing a FIRST review, a draft needing REWORK, and a draft that
-    is already APPROVED but never got flipped all resolve to the same action:
-    hand it back to the skill that owns it. Step 0 re-enters at the right step.
-    Re-implementing any of that here would be a second copy of one loop, which
-    is how one of them goes stale.
+    So a draft needing a FIRST review and a draft needing REWORK both resolve
+    to the same action: hand it back to the skill that owns it. Step 0 re-enters
+    at the right step. Re-implementing any of that here would be a second copy
+    of one loop, which is how one of them goes stale.
+
+    THE APPROVED-BUT-DRAFT CASE IS NOW CARVED OUT, deliberately — see
+    `test_an_approved_draft_is_never_deferred`. It used to route here too, and
+    that is precisely why #629 sat approved, green and draft: the remedy on
+    offer was a whole `implement-issue` session, and nobody spends one of those
+    to run a single command. `gh pr ready` is not a review loop, so naming it
+    directly does not duplicate one.
     """
-    approved_but_draft = _pr(615, reviews=[{"state": "APPROVED"}])
     never_reviewed = _pr(619, reviews=[])
     changes_requested = _pr(
         614, reviews=[{"state": "APPROVED"}, {"state": "CHANGES_REQUESTED"}]
     )
 
-    for pr in (approved_but_draft, never_reviewed, changes_requested):
+    for pr in (never_reviewed, changes_requested):
         actions = _actions_for(_run(tmp_path, [], [pr]), pr["number"])
         assert actions == {"resume_implementation"}, pr["number"]
 
@@ -377,7 +404,9 @@ def test_the_handoff_names_the_issue_to_resume(tmp_path: Path) -> None:
     """`/implement-issue <n>` takes an issue number, so the action has to carry
     one — otherwise the loop reports work nobody can pick up."""
     item = _item(592, pr=615, column="In Review", last_comment=_comment(1))
-    pr = _pr(615, reviews=[{"state": "APPROVED"}])
+    # Not an approved draft: that case is `mark_ready` now and carries no
+    # issue, because `gh pr ready <n>` needs only the PR number.
+    pr = _pr(615, reviews=[{"state": "CHANGES_REQUESTED"}])
 
     action = next(
         a for a in _run(tmp_path, [item], [pr])["actions"] if a.get("pr") == 615
@@ -526,3 +555,165 @@ def test_conflicting_pr_is_flagged_over_its_review_state(tmp_path: Path) -> None
     never fired" and nobody investigates."""
     pr = _pr(437, mergeable="CONFLICTING", reviews=[])
     assert "resolve_conflict" in _actions_for(_run(tmp_path, [], [pr]), 437)
+
+
+def _card(number: int, **over: object) -> dict:
+    card: dict = {
+        "number": number,
+        "board_status": "Backlog",
+        "priority": None,
+        "awaiting": None,
+    }
+    card.update(over)
+    return card
+
+
+def test_a_pr_awaiting_something_is_deferred_not_reported(tmp_path: Path) -> None:
+    """The reason the board holds PRs at all. "#167 and #354 are blocked" was a
+    real decision with nowhere to live, so every pass re-reported them as
+    conflicts needing action and the same conversation happened every tick."""
+    pr = _pr(167, mergeable="CONFLICTING")
+    result = _run(tmp_path, [], [pr], pr_board=[_card(167, awaiting="discussion")])
+
+    assert _actions_for(result, 167) == set()
+    assert [d["pr"] for d in result["deferred"]] == [167]
+    assert result["deferred"][0]["why"] == "awaiting discussion"
+
+
+def test_a_p4_pr_is_deferred(tmp_path: Path) -> None:
+    """ "Lower priority, I intend to get to it later" — #437 and #490."""
+    pr = _pr(437, mergeable="CONFLICTING")
+    result = _run(tmp_path, [], [pr], pr_board=[_card(437, priority="P4")])
+
+    assert _actions_for(result, 437) == set()
+    assert result["deferred"][0]["why"] == "priority P4"
+
+
+def test_an_approved_pr_can_be_deferred(tmp_path: Path) -> None:
+    """An approved PR waiting on a merge is not broken — it is the maintainers
+    call when to take it, and P4 is how they say later. #490 sat approved for a
+    day and was reported every tick as though that were news."""
+    pr = _pr(490, isDraft=False, reviews=[{"state": "APPROVED"}])
+    result = _run(tmp_path, [], [pr], pr_board=[_card(490, priority="P4")])
+
+    assert "awaiting_maintainer" not in _actions_for(result, 490)
+    assert [d["pr"] for d in result["deferred"]] == [490]
+
+
+def test_an_approved_draft_is_never_deferred(tmp_path: Path) -> None:
+    """The one carve-out. APPROVED-and-still-draft is a pipeline failure, not a
+    priority: the loop stopped one command short of finishing. #629 sat in
+    exactly this state while every pass reported it as ordinary unfinished
+    work, because the session that owned it went idle before gh pr ready."""
+    pr = _pr(629, isDraft=True, reviews=[{"state": "APPROVED"}])
+    result = _run(tmp_path, [], [pr], pr_board=[_card(629, priority="P4")])
+
+    assert "mark_ready" in _actions_for(result, 629)
+    assert result["deferred"] == []
+
+
+def test_approved_draft_is_reported_even_with_no_card(tmp_path: Path) -> None:
+    pr = _pr(629, isDraft=True, reviews=[{"state": "APPROVED"}])
+    assert "mark_ready" in _actions_for(_run(tmp_path, [], [pr]), 629)
+
+
+def test_a_stale_approval_does_not_earn_mark_ready(tmp_path: Path) -> None:
+    """The same staleness trap the non-draft rules already guard: an
+    approved-then-reworked draft keeps its APPROVED entry forever."""
+    pr = _pr(
+        700,
+        isDraft=True,
+        reviewDecision="CHANGES_REQUESTED",
+        reviews=[{"state": "APPROVED"}, {"state": "CHANGES_REQUESTED"}],
+    )
+    actions = _actions_for(_run(tmp_path, [], [pr]), 700)
+    assert "mark_ready" not in actions
+    assert "resume_implementation" in actions
+
+
+def test_a_conflicting_approved_draft_is_not_mark_ready(tmp_path: Path) -> None:
+    """A conflicted PR cannot be merged and gets no CI run, so flipping it
+    ready would hand over something unmergeable."""
+    pr = _pr(
+        701, isDraft=True, mergeable="CONFLICTING", reviews=[{"state": "APPROVED"}]
+    )
+    actions = _actions_for(_run(tmp_path, [], [pr]), 701)
+    assert "mark_ready" not in actions
+    assert "resolve_conflict" in actions
+
+
+def test_a_pr_with_no_card_is_reported_as_before(tmp_path: Path) -> None:
+    """Deferral is opt-in: absent a card, nothing changes."""
+    pr = _pr(614, mergeable="CONFLICTING")
+    result = _run(tmp_path, [], [pr], pr_board=[])
+
+    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]