fix: stop the rhythm surface reporting live work as stalled, and stale approvals as merge-ready

Two defects, both in the same direction: the surface told the maintainer to
act on work that was already handled.

REVIEW STATE (addresses the Stage 4 review on this PR, which reproduced it).
The previous commit asked "is there an APPROVED review anywhere" before
consulting `reviewDecision`. GitHub never rewrites an old review when a later
round requests changes, so an approved-then-reworked PR keeps its stale
APPROVED entry forever and was reported "nothing left but your merge" — the
exact failure the commit set out to close, reintroduced by the fix for it.

The review proposed keying on `reviewDecision` instead. That alone is also
wrong here, and measurably: `reviewDecision` is only populated when the repo
REQUIRES reviews, and this one does not. It reads CHANGES_REQUESTED for
#619/#620/#614 but "" for #490, which carries two genuine APPROVED reviews —
so keying on it alone reports an approved PR as never reviewed. Neither
signal is sufficient, and each fails toward "merge it", so the order is the
whole content of the rule: trust `reviewDecision` when set, otherwise fall
back to the LAST non-COMMENTED review. Last, not any — same staleness trap.

SESSION LIVENESS. `resume_implementation` keyed off `session == null`, and
`session` comes from `claude agents`, which lists BACKGROUND agents only. A
session started in the terminal — `claude`, then `/implement-issue <n>` — is a
foreground session and never appears; even a background agent carries a
generated descriptive name rather than the `issue-<n>` the dispatch convention
promises. Measured: 41 worktrees on disk, `claude agents --json` returning one
entry. So every worktree read as abandoned, and #624 was reported "no live
session, /implement-issue 624 to resume" while actively being worked — routing
a second session onto a branch the advice itself calls the only copy.

The worktree LOCK is what tracks a live session: git records
`locked claude session <name> (pid N start ...)`, and 4 of those 41 were
locked — exactly the four live sessions, foreground and background alike.

Live effect: #626 moves request_review -> rework_review (the review landed),
#490 stays awaiting_maintainer despite its empty reviewDecision, and #624 is
no longer reported as stalled.

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_digest.py

@@ -59,15 +59,24 @@ def _run_expect_failure(bin_dir: Path, **extra_env: str) -> subprocess.Completed
     )
 
 
-def _porcelain(*worktrees: tuple[str, str | None]) -> str:
+def _porcelain(*worktrees: tuple[str, str | None], locked: tuple[str, ...] = ()) -> str:
     """Build `git worktree list --porcelain` output for a main checkout
-    followed by the given (path, branch) pairs. branch=None means detached."""
+    followed by the given (path, branch) pairs. branch=None means detached.
+
+    Paths named in `locked` get a `locked` record in the real shape Claude Code
+    writes it -- `locked claude session <name> (pid N start ...)`, a reason
+    string rather than the bare keyword, which a `== "locked"` match would miss.
+    """
     records = [
         "worktree /repo\nHEAD 0000000000000000000000000000000000000\nbranch refs/heads/main"
     ]
     for path, branch in worktrees:
         lines = [f"worktree {path}", "HEAD 1111111111111111111111111111111111111"]
         lines.append(f"branch refs/heads/{branch}" if branch else "detached")
+        if path in locked:
+            lines.append(
+                "locked claude session wt (pid 97626 start Mon Aug 17 15:50:14 2026)"
+            )
         records.append("\n".join(lines))
     return "\n\n".join(records) + "\n"
 
@@ -892,6 +901,46 @@ def test_issue_with_no_card_reports_null_status_and_is_an_orphan(
     assert [o["ref"] for o in orphans] == ["624"]
 
 
+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
+    its worktree reads as abandoned."""
+    issue = _issue(624)
+    _write_shim(bin_dir, "gh", _gh_shim([issue], [], []))
+    _write_shim(
+        bin_dir,
+        "git",
+        _git_shim(
+            _porcelain(
+                ("/repo/worktrees/issue-624", "fix/issue-624-pwl"),
+                locked=("/repo/worktrees/issue-624",),
+            )
+        ),
+    )
+
+    item = _run(bin_dir)["items"][0]
+
+    assert item["worktree"] == "/repo/worktrees/issue-624"
+    assert item["worktree_locked"] is True
+    # ...and the name-matched session is still null, which is the whole point.
+    assert item["session"] is None
+
+
+def test_an_unlocked_worktree_is_reported_as_unlocked(bin_dir: Path) -> None:
+    issue = _issue(625)
+    _write_shim(bin_dir, "gh", _gh_shim([issue], [], []))
+    _write_shim(
+        bin_dir,
+        "git",
+        _git_shim(_porcelain(("/repo/worktrees/issue-625", "fix/issue-625"))),
+    )
+
+    item = _run(bin_dir)["items"][0]
+
+    assert item["worktree"] == "/repo/worktrees/issue-625"
+    assert item["worktree_locked"] is False
+
+
 def test_issue_with_a_card_is_not_an_orphan(bin_dir: Path) -> None:
     issue = _issue(602)
     card = {"content": {"number": 602}, "status": "Backlog", "priority": "P1"}

backend/tests/test_backlog_rhythm.py

@@ -42,6 +42,9 @@ def _item(number: int, **over: object) -> dict:
         "merged_pr": None,
         "worktree": None,
         "worktree_branch": None,
+        # A live session holds its worktree locked. Defaults to unlocked so the
+        # pre-existing stalled-work tests keep asserting what they always did.
+        "worktree_locked": False,
         "stale_worktree": False,
         "session": None,
         "blocked_by": [],
@@ -247,6 +250,36 @@ def test_a_worktree_with_no_session_is_stalled_work(tmp_path: Path) -> None:
     assert "never restart" in action["detail"]
 
 
+def test_a_locked_worktree_is_never_reported_as_stalled(tmp_path: Path) -> None:
+    """`claude agents` lists background agents only, so a foreground
+    `/implement-issue` started from the terminal is invisible and its worktree
+    reads as abandoned. #624 was reported "no live session" while actively
+    being worked, advising a second session onto the same branch. The lock is
+    what a live session actually holds."""
+    item = _item(
+        624,
+        worktree="/repo/worktrees/fix-issue-624",
+        worktree_branch="fix/issue-624-pwl-window-bisect",
+        worktree_locked=True,
+        session=None,
+    )
+    assert "resume_implementation" not in _actions_for(_run(tmp_path, [item]), 624)
+
+
+def test_an_unlocked_worktree_with_no_session_is_still_stalled(
+    tmp_path: Path,
+) -> None:
+    """The lock must not swallow the case this rule exists for."""
+    item = _item(
+        625,
+        worktree="/repo/worktrees/fix-issue-625",
+        worktree_branch="fix/issue-625",
+        worktree_locked=False,
+        session=None,
+    )
+    assert "resume_implementation" in _actions_for(_run(tmp_path, [item]), 625)
+
+
 def test_a_worktree_with_a_live_session_is_left_alone(tmp_path: Path) -> None:
     item = _item(
         590,
@@ -412,6 +445,67 @@ def test_a_non_draft_with_changes_requested_needs_rework(tmp_path: Path) -> None
     assert "awaiting_maintainer" not in actions
 
 
+def test_a_stale_approval_does_not_survive_a_later_changes_requested(
+    tmp_path: Path,
+) -> None:
+    """GitHub never rewrites an old review when a later round requests
+    changes, so an approved-then-reworked PR keeps its APPROVED entry forever.
+    Asking "is there an APPROVED anywhere" reported a PR with changes
+    outstanding as ready to merge — the same failure, reintroduced by the first
+    attempt at fixing it."""
+    pr = _pr(
+        700,
+        isDraft=False,
+        reviewDecision="CHANGES_REQUESTED",
+        reviews=[{"state": "APPROVED"}, {"state": "CHANGES_REQUESTED"}],
+    )
+    actions = _actions_for(_run(tmp_path, [], [pr]), 700)
+    assert "rework_review" in actions
+    assert "awaiting_maintainer" not in actions
+
+
+def test_an_approval_is_honoured_when_review_decision_is_empty(
+    tmp_path: Path,
+) -> None:
+    """`reviewDecision` is only populated when the repo REQUIRES reviews, and
+    this one does not — #490 carries two real APPROVED reviews and still reads
+    "". Keying on reviewDecision alone would report it as never reviewed."""
+    pr = _pr(
+        490,
+        isDraft=False,
+        reviewDecision="",
+        reviews=[
+            {"state": "COMMENTED"},
+            {"state": "APPROVED"},
+            {"state": "COMMENTED"},
+            {"state": "APPROVED"},
+        ],
+    )
+    actions = _actions_for(_run(tmp_path, [], [pr]), 490)
+    assert "awaiting_maintainer" in actions
+    assert "request_review" not in actions
+
+
+def test_a_stale_approval_loses_to_changes_requested_without_review_decision(
+    tmp_path: Path,
+) -> None:
+    """The same staleness trap with no reviewDecision to lean on: the LAST
+    non-COMMENTED verdict decides, not the presence of an APPROVED."""
+    pr = _pr(
+        701,
+        isDraft=False,
+        reviewDecision="",
+        reviews=[
+            {"state": "APPROVED"},
+            {"state": "COMMENTED"},
+            {"state": "CHANGES_REQUESTED"},
+        ],
+    )
+    actions = _actions_for(_run(tmp_path, [], [pr]), 701)
+    assert "rework_review" in actions
+    assert "awaiting_maintainer" not in actions
+
+
 def test_an_approval_followed_by_notes_still_counts(tmp_path: Path) -> None:
     """A trailing COMMENTED must not un-approve a PR — #490 carries exactly
     this shape (COMMENTED, APPROVED, COMMENTED, APPROVED)."""

scripts/backlog-digest.sh

@@ -80,23 +80,48 @@ merged_prs=$(gh pr list --repo "$repo" --state merged --limit 200 \
       refs: [ (.body // "") | scan("(?i)(?:fixes|closes|resolves) #([0-9]+)") | .[0] | tonumber ]
     } ]')
 
-# Emits, per worktree, a JSON object of {path, branch}. `git worktree list`
-# always emits the main checkout as its first record, and it is excluded
+# Emits, per worktree, a JSON object of {path, branch, locked}. `git worktree
+# list` always emits the main checkout as its first record, and it is excluded
 # here — it is never a task worktree, so it must not appear in the orphan
 # scan below.
+#
+# `locked` is the LIVENESS signal, and it is the only one that works. See the
+# session note below.
 worktrees=$(git worktree list --porcelain | awk '
   BEGIN { RS=""; FS="\n" }
   NR==1 { next }
   {
-    path=""; branch=""
+    path=""; branch=""; locked="false"
     for (i = 1; i <= NF; i++) {
       if ($i ~ /^worktree /)      { path = substr($i, 10) }
       else if ($i ~ /^branch /)   { branch = substr($i, 8); sub(/^refs\/heads\//, "", branch) }
+      else if ($i ~ /^locked/)    { locked = "true" }
     }
-    if (path != "") print path "\t" branch
+    if (path != "") print path "\t" branch "\t" locked
   }
-' | jq -R 'split("\t") | {path: .[0], branch: (.[1] // "")}' | jq -s .)
+' | jq -R 'split("\t") | {path: .[0], branch: (.[1] // ""), locked: (.[2] == "true")}' | jq -s .)
 
+# `claude agents` lists BACKGROUND agents only, and this is the trap that made
+# the rhythm pass tell the maintainer to restart work that was actively
+# running. Two independent reasons it cannot answer "is someone on this":
+#
+#   1. A session started in the terminal — `claude` in a CLI, then
+#      `/implement-issue <n>` — is a foreground session and never appears here
+#      at all. That is how #624 was dispatched.
+#   2. Even a background agent carries a generated descriptive name ("Review PR
+#      and create branch for bess-manager"), not the `issue-<n>` the dispatch
+#      convention promises, so the exact-name match below misses it too.
+#
+# Measured: 41 worktrees on disk, `claude agents --json` returning ONE entry.
+# So `session` was null for essentially every item, every worktree read as
+# abandoned, and `resume_implementation` fired on live sessions — against work
+# whose branch commits the skill itself calls the only copy.
+#
+# The worktree LOCK is the signal that actually tracks a live session: 4 of
+# those 41 were locked, and they were exactly the four live sessions. It is
+# local, needs no process list, and covers foreground and background alike.
+# `session` is kept because a name match is strictly more informative when it
+# does happen; it is no longer what liveness rests on.
 sessions=$(claude agents --json)
 
 # No `--field "Priority"` here: verified against the real CLI just now,
@@ -380,6 +405,10 @@ jq -n \
           merged_pr: $merged_pr,
           worktree: ($wt.path // null),
           worktree_branch: ($wt.branch // null),
+          # A LIVE session holds its worktree locked. This, not `session`, is
+          # what says whether anyone is on the item — see the note where the
+          # worktree list is built.
+          worktree_locked: ($wt.locked // false),
           # A worktree whose own branch has already merged is rot, and
           # `sweep-prs` is what removes it. Flagged so a board pass reports it
           # instead of reading it as active work: #593, #571, #542 and #466 all

scripts/backlog-rhythm.sh

@@ -171,9 +171,18 @@ actions=$(printf '%s' "$digest" | jq \
     # The branch survives, so this is a resume and never a restart -- Step 0
     # re-enters at the earliest incomplete step. Restarting would run Step 4,
     # which branches fresh from origin/main and would delete those commits.
-    (if .worktree != null and (.stale_worktree | not) and .session == null and .pr == null
+    #
+    # A LOCKED worktree means a session is holding it right now, and that check
+    # is what stops this rule from firing on live work. `session` alone could
+    # not: `claude agents` sees background agents only, so a foreground
+    # `/implement-issue` started from the terminal was invisible and its
+    # worktree read as abandoned. #624 was reported "no live session" while
+    # actively being worked, with the advice to re-enter it -- a second session
+    # on the same branch, against work the detail line itself calls the only
+    # copy.
+    (if .worktree != null and (.stale_worktree | not) and (.worktree_locked | not) and .session == null and .pr == null
      then {issue: .number, action: "resume_implementation",
-           why: "worktree \(.worktree_branch) on disk, no live session",
+           why: "worktree \(.worktree_branch) on disk, unlocked, no live session",
            detail: "/implement-issue \(.number) — Step 0 resumes it; never restart, the branch commits are the only copy"}
      else empty end),
 
@@ -222,24 +231,43 @@ actions=$(printf '%s' "$digest" | jq \
          # 11 at all. Stage 4 is the gate the whole pipeline is built around;
          # a surface that routes around it is worse than no surface.
          #
-         # An APPROVED review is the bar, matching Step 11 exactly. Not
-         # `reviewDecision`, which is "" both for never-reviewed and for
-         # approved-then-dismissed, and not COMMENTED, which the bot also
-         # posts as a placeholder before its real verdict (see
-         # request-pr-review.sh).
+         # NEITHER SIGNAL ALONE IS CORRECT, and each fails in the merge-happy
+         # direction, so the ORDER below is the whole content of this rule.
+         #
+         # `reviews[]` is history, not state: GitHub never rewrites an old
+         # review when a later round requests changes, so an
+         # approved-then-reworked PR keeps its stale APPROVED entry forever.
+         # Asking "is there an APPROVED anywhere" first therefore reported
+         # "nothing left but your merge" for a PR with changes outstanding --
+         # exactly the failure this rule exists to prevent, reintroduced by the
+         # first attempt at fixing it.
+         #
+         # `reviewDecision` is the current state, but it is only populated when
+         # the repo REQUIRES reviews, and this one does not: it reads
+         # CHANGES_REQUESTED for #619/#620/#614 and "" for #490, which carries
+         # two real APPROVED reviews. So keying on it alone would report a
+         # genuinely approved PR as never reviewed.
+         #
+         # Hence: trust `reviewDecision` whenever it is set, and only then fall
+         # back to the LAST non-COMMENTED review. Last, not any -- same staleness
+         # trap. COMMENTED is skipped because the bot posts its inline notes as
+         # a COMMENTED review before its real verdict (see request-pr-review.sh).
          elif (.isDraft | not)
-         then (if ([ .reviews[]? | select(.state == "APPROVED") ] | length) > 0
-               then {pr: .number, action: "awaiting_maintainer",
-                     why: "out of draft and approved",
-                     detail: "nothing left but your merge"}
-               elif .reviewDecision == "CHANGES_REQUESTED"
-               then {pr: .number, issue: $issue_no, action: "rework_review",
-                     why: "out of draft but review asked for changes",
-                     detail: "address the review, then request a fresh one"}
-               else {pr: .number, issue: $issue_no, action: "request_review",
-                     why: "out of draft but never reviewed — Stage 4 has not run",
-                     detail: "scripts/request-pr-review.sh \(.number) — do NOT merge on the draft flag alone"}
-               end)
+         then ([ .reviews[]? | select(.state != "COMMENTED") ] | last | .state?) as $last_verdict
+              | (if .reviewDecision == "CHANGES_REQUESTED" or
+                    (.reviewDecision == "" and $last_verdict == "CHANGES_REQUESTED") or
+                    (.reviewDecision == null and $last_verdict == "CHANGES_REQUESTED")
+                 then {pr: .number, issue: $issue_no, action: "rework_review",
+                       why: "out of draft but the current review asks for changes",
+                       detail: "address the review, then request a fresh one"}
+                 elif .reviewDecision == "APPROVED" or $last_verdict == "APPROVED"
+                 then {pr: .number, action: "awaiting_maintainer",
+                       why: "out of draft and approved",
+                       detail: "nothing left but your merge"}
+                 else {pr: .number, issue: $issue_no, action: "request_review",
+                       why: "out of draft but never reviewed — Stage 4 has not run",
+                       detail: "scripts/request-pr-review.sh \(.number) — do NOT merge on the draft flag alone"}
+                 end)
          else {pr: .number, issue: $issue_no, action: "resume_implementation",
                why: "draft PR, review loop unfinished",
                # `implement-issue` is used for TODO.md items and refactors too,