fix: make backlog grooming reflect what actually blocks an issue - #623
Conversation
The digest misclassified enough of the board that grooming could not be trusted, and every misclassification pushed work in the same direction: towards looking more ready than it was. Measured against the live board, Ready went 1 -> 0 and In Progress 5 -> 1. Five defects, each traced to a real item. 1. `analyzed` was tested BEFORE any wait, so an analysed-but-blocked item reported Ready. #96 was labelled `analyzed`, prioritised P2, carried no blocking label, and still could not be built because its approach was undecided. It read as dispatchable, a session was dispatched at it, and that session deadlocked on three design questions. Waits now outrank `analyzed`. 2. The board's `Awaiting` field was never read. 17 items have it set (`discussion` x11, `reporter` x6) and the digest derived its own value from labels instead, so recorded grooming had no effect on anything. The field is now authoritative, with `awaiting_source` and `awaiting_suggested` exposed so an unset field can be reconciled rather than silently invented. 3. `awaiting: discussion` was returned for ANY human comment, which is not a blocker -- thanks, a "me too" and a follow-up question all pushed an item to Analysis. Only a recorded wait or a blocking label does that now. 4. A worktree left on disk pinned its issue to In Progress forever. #593, #571, #542 and #466 all reported In Progress while their PRs (#618, #579, #591, #517) had merged. Staleness is decided by comparing the worktree's own branch against merged PRs -- an exact match, deliberately not the fuzzy issue-number match used to associate a worktree with an issue. 5. `blocked` did not fail Ready, so #571 reported Ready for Dev while labelled `blocked`. Definition of Ready criterion 5 now holds, including an unresolved `Blocked by #N`. Ready for Dev also finally requires a Priority, which the design always specified and the code deferred "until a board exists". It exists. New: `last_comment` {author, days, is_reporter, is_bot}. Without it the digest could not represent the transition that matters most to grooming -- the reporter answering us. A count and a date cannot tell that from a nudge we posted, which is why #621 crossed the Definition of Ready line unnoticed. REJECTED while building this: mapping a merged PR to Done. It reclassified 7 open issues (#118, #120, #403 among them) as finished, and it contradicts this project's rule that beta PRs omit `Closes #N` until graduation -- an open issue with a merged fix is the NORMAL state. `merged_pr` is reported; it moves no column, and a test pins that. Merged PR bodies are reduced to their closing references before reaching jq; passing 200 of them through --argjson overflows the argument list. Tests: 25 pass. The `gh` shim now branches on the full argument string, because `pr list` is called twice with different `--state` values and matching only the subcommand returned the open list for both -- which would have made every open PR look merged. Fixtures gained `createdAt` on comments, which real gh always sends and whose absence failed `strptime` rather than testing anything. `_run` now surfaces the digest's stderr instead of a bare "exit status 5".
|
@claude-bot review |
![bess-manager-claude-bot[bot]](https://avatars.githubusercontent.com/in/3024553?s=60&v=4){kind=small}
| # label and an unresolved `Blocked by #N` line both fail it, so neither | ||
| # item can be Ready however far its analysis got. #571 was reporting | ||
| # `Ready for Dev` while labelled `blocked`. | ||
| | (($labels | index("blocked")) != null or ((blocked_by | length) > 0)) as $blocked |
There was a problem hiding this comment.
blocked_by (used here to compute $blocked) does not actually check whether the referenced issue is resolved — it only checks whether the text Blocked by #N is still present in the body:
def blocked_by:
[ (.body // "") | scan("(?i)blocked by #(\\d+)") | .[0] | tonumber ];But both the commit message and SKILL.md claim this enforces unresolved blockers: "Definition of Ready criterion 5 now holds, including an unresolved Blocked by #N" and (in the code comment two lines above) "an unresolved Blocked by #N line". If issue #500 that #702 is "Blocked by" gets closed, #702's body still says Blocked by #500, so blocked_by still returns [500], $blocked stays true, and the item is permanently stuck out of Ready for Dev — even though the DoR criterion is now satisfied.
This is exactly the class of misclassification the PR is trying to eliminate (item looks less ready than it is, this time), and it's cheap to fix with data the script already has: $issues is already the full open-issue list (gh issue list --state open), so blocked_by numbers that are not in $issues are resolved (closed or nonexistent) and should not block. Something like:
def blocked_by:
[ (.body // "") | scan("(?i)blocked by #(\\d+)") | .[0] | tonumber ]
| map(select(. as $n | $open_numbers | index($n) != null));(with $open_numbers passed down as [$issues[].number]). No test in this PR exercises the case where the blocking issue has since closed — test_blocked_by_reference_is_never_ready only asserts the reference is detected, not that it's actually unresolved.
| if $pr != null then "In Review" | ||
| elif $blocked then "Analysis" | ||
| elif $awaiting != null then "Analysis" | ||
| elif $wt_live then "In Progress" |
There was a problem hiding this comment.
Observation, not a blocker: $blocked and $awaiting are both tested before $wt_live:
def column($labels; $pr; $wt_live; $awaiting; $priority; $blocked):
if $pr != null then "In Review"
elif $blocked then "Analysis"
elif $awaiting != null then "Analysis"
elif $wt_live then "In Progress"So an item with a live (unmerged) worktree that also carries blocked or a board awaiting value reports Analysis, not In Progress — the worktree fields (worktree, worktree_branch, stale_worktree: false) are still in the payload, but the column hides that there's active, un-landed work. No test in this PR exercises blocked/awaiting + a live worktree together, so I can't tell if this is intentional ("a wait always wins, even over live code") or an oversight from reordering for the analyzed case. Worth confirming the intent, since the PO's board-reconciliation table in SKILL.md doesn't call this combination out either.
There was a problem hiding this comment.
Summary
No linked issue — this is self-directed backlog-tooling work, not a Stage 3 fix against a Stage 2 diagnosis, so there's no root-cause diagnosis to match against. Judged on its own claims and the live-board evidence in the PR description instead.
Verified locally (worktree checkout of e479ca2):
.venv/bin/pytest backend/tests/test_backlog_digest.py→ 25/25 pass.- Reverted
scripts/backlog-digest.shto the pre-PR version and reran the same suite → 13 fail (Ready/Ready for Dev,In review/In Reviewcasing,KeyErroronlast_comment/stale_worktree/merged_pr), confirming the new tests are genuinely RED without the fix and GREEN with it — satisfiesdocs/agents/testing.md's "must be seen to fail without its fix." black --check,ruff check,mypy,shellcheckall clean on the changed files.- Tests assert on
column/awaiting/blocked/stale_worktreeoutput values, not internal field names or algorithm boundaries — behavior-focused, per testing rules.
Is the change minimal / does it hold for all inputs? Mostly, with one real gap (inline comment, scripts/backlog-digest.sh:267): blocked_by is a pure text-scan for Blocked by #N in the issue body — it never checks whether issue #N is actually still open. Both the commit message and SKILL.md claim this now enforces an "unresolved" blocker, but a Blocked by #N reference to an issue that has since closed will permanently pin the item out of Ready for Dev, which is the same class of misclassification (item reads wrong relative to its real state) this PR sets out to fix — just in the opposite direction. It's cheap to fix with data the script already fetches ($issues is already the open-issue list, so membership there tells you if #N is still open). Requesting changes on this one specifically since it contradicts an explicit claim made in both the commit message and the docs.
Second, lower-severity comment on scripts/backlog-digest.sh:240: $blocked/$awaiting are tested before $wt_live in column(), so a live (unmerged) worktree on a blocked/awaiting item reports Analysis, hiding that there's active undelivered code. Might be intentional (a wait should win regardless), but it's untested and not documented in the SKILL.md reconciliation table — asking for confirmation of intent, not blocking on it.
Scope: single script + its test file + the skill doc that describes it, no new classes, no touches outside this "digest" boundary — reads as a local, contained fix, consistent with the Debugging Protocol's scope categories even though the PR body doesn't spell that framing out explicitly (minor, not blocking — this isn't a Stage-3 issue-fix flow).
Rule compliance: no Optional[x], no hasattr/getattr-with-default, no new classes, no hardcoded entity IDs (n/a — no HA sensor code touched), no exception-string matching. Comments in the jq are dense but consistently WHY (defect number, real issue cited), not WHAT.
Otherwise this is a well-evidenced fix — each of the five defects is traced to a specific mis-scored issue number, the rejected "merged PR → Done" approach is explained rather than silently dropped, and the test-harness bugs found along the way (the gh shim's $1 $2 matching, check=True swallowing stderr) are real and independently useful.
Review of #623 found a real gap, and it contradicted an explicit claim: both the commit message and SKILL.md said this enforced an "unresolved" blocker, while `blocked_by` was a pure text scan that never checked whether the blocker was still open. A `Blocked by #N` line is never edited out of an issue body once N lands, so that scan pins the item out of Ready for Dev permanently. That is the same failure this script exists to fix, pointing the other way: an item reading wrong relative to its real state. `$issues` is already the open-issue list, so membership decides it with no extra API call. `blocked_by` keeps the raw parse so the reference stays visible; the new `blocked_by_open` is the subset that actually blocks, and `$blocked` reads that. Also pins the precedence question the review asked me to confirm rather than guess at: a recorded wait DOES outrank a live worktree in `column`, because unsettled scope must not read as progress. The risk is hiding active undelivered code, so the worktree stays reported on the item — the wait changes the column, not the evidence. Now tested and documented in the reconciliation table instead of being implied. 27 tests pass.
|
@claude-bot review |
There was a problem hiding this comment.
Summary
Re-review at a4fdef629 ("fix: only a still-open Blocked by #N fails Ready"). Both blockers from the prior review round at e479ca2 are resolved:
blocked_by_openwas added (scripts/backlog-digest.sh), so a closedBlocked by #Nreference no longer permanently pins an item out of Ready — confirmed bytest_closed_blocked_by_reference_does_not_block.- The
wt_livevsawaiting/blockedprecedence question was answered withtest_a_wait_outranks_a_live_worktree_but_the_worktree_stays_visible, which documents the intent is deliberate.
(Note: an earlier gh pr review --comment I posted on this PR with body "test permission check - will be replaced" was accidental tool-testing on my part — please disregard it.)
Verified locally (worktree of a4fdef629): .venv/bin/pytest backend/tests/test_backlog_digest.py → 27 passed; black --check and ruff check clean on the changed test file.
Root-cause match: yes — each of the 5 defects is traced to a real issue number, and the fixes (wait-outranks-analyzed precedence, board Awaiting as authoritative, dropping bare-human-comment-as-discussion, stale-worktree via merged-branch match, blocked gating Ready) map directly onto them.
Would the tests catch a regression? Mostly yes for the described defects. One gap below.
Correctness bug — blocked has no negation handling and now has real effect for the first time
scripts/backlog-digest.sh:
def blocked_by:
[ (.body // "") | scan("(?i)blocked by #(\\d+)") | .[0] | tonumber ];
feeds
| (($labels | index("blocked")) != null or (($bb_open | length) > 0)) as $blocked
which column() tests unconditionally, ahead of the analyzed check:
def column($labels; $pr; $wt_live; $awaiting; $priority; $blocked):
if $pr != null then "In Review"
elif $blocked then "Analysis"
...
Two related issues:
- No negation awareness. The regex matches the substring
blocked by #Nregardless of surrounding words. An issue body containing"not blocked by #50 anymore"or"no longer blocked by #12"(a very natural way to update an issue once a blocker resolves) still matches, and — as long as #50/#12 is still open —blocked_by_openwill treat it as a live blocker. - It's no longer inert. On
main,blocked_bywas extracted but never fed intocolumn()— it was report-only. This PR is what first gives it teeth ($blockedgatescolumn()), so this false-positive risk is new, not merely latent. - It applies to every open issue, not just triaged ones. Because
$blockedis checked before theanalyzed/priority branch, an issue with no labels at all that happens to mention "blocked by #N" in its body moves fromBacklogstraight toAnalysis— a classification change with no human triage behind it, which is a milder version of the same "item reads wrong relative to its real state" failure mode this PR exists to fix, just in the other direction.
None of the new tests cover negated phrasing or an untriaged issue with an incidental "blocked by" mention — test_blocked_label_is_never_ready and test_open_blocked_by_reference_is_never_ready both use issues already labelled analyzed. Worth at least a word-boundary/negation guard (e.g. reject a match preceded by "not "/"no longer ") or restricting the body-scan path to labelled/triaged issues, whichever matches the intended design.
Secondary — unverified factual claim replacing an explicit "unconfirmed" warning
.claude/skills/backlog/SKILL.md changes:
Board writes need `BESS_PO_TOKEN` with `project` scope. The custom-field JSON
shape is confirmed: `gh project item-list --format json` puts each single-select
value at the item's top level, so `.priority` and `.awaiting` both read directly.
This replaces text that explicitly called the shape unconfirmed and told the reader to verify it "on the first real run." Per CLAUDE.md's Verification Before Action ("ALWAYS verify against actual source code/repos before making assumptions about APIs... do not guess"), I'd expect to see the evidence that backed dropping that caveat — none of the new tests exercise the real gh CLI shape (_gh_shim fabricates project_items in exactly the shape the jq expects, so it's self-fulfilling on this specific assumption). If the shape is wrong, .priority?/.awaiting? silently resolve to null for every item with no error — the same silent-degradation failure this PR is fixing, just reintroduced. I'd guess this actually was verified live (the measured before/after board table in the PR description depends on priority/awaiting reads working), but the PR doesn't show that evidence — worth a one-line pointer to it for the next reader, not necessarily blocking.
Minor / non-blocking
human_comments(scripts/backlog-digest.sh, unchanged, outside this diff's hunks) is now dead code — nothing calls it any more sinceawaiting_from_labelsdropped thediscussion-from-any-human-comment branch. The comment oncomments: (.comments | length)("awaiting: discussionabove is driven by human_comments only") is now stale/incorrect, sincediscussionis no longer computed that way at all.- PR description says "25 tests pass (was 18)"; the actual suite has 27 tests. Doesn't affect correctness, just an inaccurate count in the writeup.
Scope: single script + its test file + the skill doc describing it — no new classes, nothing outside the digest's existing boundary. Reads as a local, contained fix; the PR description doesn't use the rules.md "local/structural" framing explicitly, but the change doesn't need it — nothing here moves responsibility to a different owner.
Rule compliance: no Optional[x], no hasattr/getattr-with-default, no new classes, no exception-string matching (n/a — no Python exception handling touched), black/ruff clean.
Requesting changes primarily for the blocked negation/scope issue — it's a concrete, if narrow, misclassification path in a PR whose entire point is eliminating exactly that class of bug.
Every follow-up rule in the backlog skill had been written down and NONE had
ever fired. The 14-day reporter chase, the 28-day park, the reporter-replied
re-check, the stale-worktree handoff: all decoration, because each needed a
model to notice it and nothing scheduled one.
So the noticing is deterministic now and lives in scripts/backlog-rhythm.sh.
Every rule is a comparison over the digest — no judgement, no tokens. A quiet
backlog prints "RHYTHM: nothing due." for the cost of one process, which is
what makes it worth running on a timer at all. The PO agent is needed only to
ACT, and only when something is due.
The pass covers BOTH halves of the path to an approvable PR:
Issue side — recheck_ready, nudge_reporter, park, surface_discussion,
set_awaiting, set_priority, triage_labels, dispatchable.
PR side — and this is the half that actually hands the maintainer something:
mark_ready approved but still a draft <- the finish line
awaiting_maintainer approved and out of draft
request_review draft with no review at all
rework changes requested
resolve_conflict CONFLICTING (produces no CI run, so it reads as
"checks never fired" and nobody investigates)
Those two states were invisible in practice. #615 and #617 sat APPROVED and
still drafts overnight with nothing left but the merge; #619 was never
reviewed at all. Nothing was watching either transition.
Ordering is load-bearing. PR actions come first because they are closest to
the finish line, and recheck_ready outranks the chases: nudging someone who
has already replied is the worst output this pass could produce.
Quiet time is measured from the LAST COMMENT, not updatedAt — a label change
or a board move bumps updatedAt, so an issue nobody has spoken on for a month
would look active and never age into a chase.
A bare COMMENTED review is not treated as a verdict, because the review bot
posts its inline notes as one before the summary.
Against the live board the pass finds 30 due actions, including PR #490
awaiting the maintainer, #162 park (quiet 54d), three stale worktrees and
three conflicted PRs.
RHYTHM_DIGEST_FILE / RHYTHM_PRS_FILE are test seams, the same shape as
BESS_ENV_FILE in gh-agent.sh. 16 tests pin the rules, including that a quiet
backlog is a noop and that a reply beats the chase.
Still not wired to a schedule — that is the invocation, not the logic, and it
is deliberately a separate step.
…icating it The previous commit built request_review / mark_ready / rework into the Rhythm pass, which is a second copy of implement-issue Step 11. That contradicts the argument used to put resume in Step 0 rather than in a separate skill: two copies of one review loop means one of them goes stale. It also mis-diagnosed the symptom. #615 and #617 did not sit APPROVED-but-draft because nothing was watching for that state; they sat there because the sessions that owned them exited before Step 11 finished. The fix belongs where the loop already lives. So every unfinished draft now resolves to ONE action, `resume_implementation`, carrying the issue number so the handoff is directly runnable. Step 0 re-enters at the earliest incomplete step, whether the PR needs a first review, a rework, or just the ready flag it never got. Two fleet-level exceptions stay in the pass, because implement-issue deliberately does not widen to them: `awaiting_maintainer` (report only) and `resolve_conflict` (sweep-prs). Adds the stalled-work rule this was missing: a LIVE worktree with no session behind it is an implementation that stopped mid-flight -- the machine restarted, the session was killed, or the agent exited between steps. Nothing picked these up, and an audit found 34 such worktrees, 8 holding real unpushed commits and one with 32. Against the live board it finds #466 and #602. `pr == null` guards that rule so work with a PR is reported once, by the PR branch, rather than twice. It is always a RESUME, never a restart: Step 4 branches fresh from origin/main and would delete commits that exist nowhere else. The detail string says so, and a test pins it. SKILL.md gains the reasons a future pass must not re-learn this: do not drive the review loop here; a session reporting `working` may have written nothing (three dispatches produced zero writes in one day while reporting healthy state); and read `claude agents --json` unsandboxed, since ~/.claude/jobs is sandbox-denied and a sandboxed listing returned 1 session where the truth was 17. 19 rhythm tests, 27 digest tests, gate green.
Review of #623 found a real misclassification path in a PR whose whole point is eliminating them. `blocked_by` was a free `scan` over the issue body, so it matched the substring regardless of what preceded it. "not blocked by #500 anymore" and "no longer blocked by #500" both registered as live blockers -- and those are the natural way to update an issue once its blocker resolves, so the false positive fired exactly when the blocker was GONE. The item would be pinned out of Ready for Dev permanently. The severity is new, not latent: on main `blocked_by` was extracted and never fed into `column()`, so a bad parse was inert. Gating `column()` on it is what gave it teeth. Matched per LINE and anchored to the line start now, optionally bulleted, which is the convention the skill documents ("a `Blocked by #N` line in the issue body"). Anchoring rejects the negations without a blacklist that would only cover the phrasings someone happened to think of. It also fixes the reviewer's third point: an untriaged issue merely mentioning a blocker in prose no longer moves Backlog -> Analysis with no human triage behind it. Four tests: the bulleted form still counts, three negated phrasings do not, and an incidental mid-sentence mention does not reclassify. Also from the same review: - `human_comments` became dead code when `awaiting` stopped deriving `discussion` from comment activity. Removed, and the stale comment on `comments:` that still described that mechanism is corrected. - SKILL.md claimed the board's custom-field JSON shape was "confirmed" where the previous text had explicitly said unconfirmed, without showing the evidence. It was verified live; the command and its result are now recorded, with a note that the tests fabricate that shape and so cannot prove it. 49 tests pass (27 digest + 19 rhythm, plus the 3 new negation cases).
|
@claude-bot review |
`implement-issue` is used for TODO.md items and refactors, not only for issues, so a draft PR with no linked issue is normal rather than a defect. The pass reported "no issue references this PR; finish it by hand", which left every self-directed PR with no owner in the loop -- exactly how #620, #622 and #623 all ended up driven by hand today. No flag distinguishes the two cases: GitHub numbers issues and PRs from ONE sequence per repo, so a bare number is already unambiguous and Step 0 can resolve whichever it is. An earlier draft of this used `--pr <n>`; that distinction carries no information. Where an issue IS linked it is still named, because it carries the diagnosis. Where none is, the PR number is the handle, and a strong one: it holds the branch, the diff, the scope assessment and the review verdict, which is everything Step 0 reads. Step 0 accepting a PR number is a matching change to implement-issue's SKILL.md, which lives on the fix/review-verdict-placeholder branch (#622) where Step 0 was added. Both have to land for the loop to cover this case.
This skill is used for TODO.md items and for refactors that never had an issue, so "issue number" was too narrow a contract. Step 0 already keys off observable state; a PR is simply another entry point to it, and the stronger one -- it carries the branch, the diff, the `## Scope assessment` and the review verdict, which is everything Step 0 reads. No flag is needed. GitHub numbers issues and PRs from ONE sequence per repository, so a bare number is unambiguous: try `gh pr view <n>`, fall back to `gh issue view <n>`. An earlier draft used `--pr <n>`; that distinction carries no information. Why it matters beyond tidiness: scripts/backlog-rhythm.sh hands unfinished drafts back to this skill, and for a PR with no linked issue it had nothing to hand -- it reported "no issue references this PR; finish it by hand". That left every self-directed PR with no owner in the loop, which is how #620, #622 and #623 all ended up driven by hand in one session. Where an issue IS linked, nothing changes: it is still read for the diagnosis. Where none is, Step 2's root cause comes from the maintainer's own framing rather than a Stage 2 comment, and Step 9 records it in the PR body as usual.
The Stage 4 reviewer ran out of turns instead of reviewing. On PR #623 the run ended with `Execution failed: Reached maximum number of turns (60)` after posting only an early scratch comment, so the PR carried a stray review and no verdict while the run was already dead. Waiting 16 minutes looked like a slow review and was a failed one -- and scripts/request-pr-review.sh cannot tell those apart, because a dead run and a thinking one are both silence. The cap is hit because this reviewer does real verification: it checks the branch out, runs the suite, REVERTS the fix to confirm the tests go red without it, then runs black/ruff/mypy/shellcheck. That is the behaviour worth paying for, and its cost scales with the diff. 120 leaves headroom for a multi-file PR while still bounding a runaway. Deliberately a one-line PR so it can be merged on its own: workflows triggered by `issue_comment` run from the DEFAULT branch, so this has no effect on any review until it is on main. Every other open PR is waiting behind it.
|
@claude-bot review |
There was a problem hiding this comment.
Review
Scope: this PR touches only internal PO tooling (scripts/backlog-digest.sh, new scripts/backlog-rhythm.sh, their tests, and .claude/skills/backlog/SKILL.md) — no Python, TypeScript, or optimizer code. Most of the standing checklist (Architecture compliance, Error handling, API layer, HA sensor access) is not applicable to this diff.
Root cause vs. fix. No linked tracking issue — this is self-directed maintenance, same as #620/#622 per the new test file's own comment. The PR body documents five separate defects, each pinned to a real board item (#96, #593/#571/#542/#466, #621, #118/#120/#403) with before/after board counts. I spot-verified the two riskiest claims against the actual diff:
column()inscripts/backlog-digest.sh(scripts/backlog-digest.sh:258 in the diff) now checks$blockedand$awaitingbefore$wt_live, which is the fix for defect 1. I mutated a local copy to restore the old ordering (wt_livebeforeawaiting) and confirmedtest_a_wait_outranks_a_live_worktree_but_the_worktree_stays_visiblefails without the fix ('In Progress' != 'Analysis') — genuinely RED without the change, not a test that happens to pass either way.worktree_is_stale()compares the worktree's own branch against--state mergedPRs by exactheadRefName, deliberately distinct from the fuzzy issue-number match used elsewhere — correctly reasoned in the comment, and covered by both the stale and non-stale worktree tests.
Tests. Checked out the branch and ran the full suite: pytest backend/tests/test_backlog_digest.py backend/tests/test_backlog_rhythm.py → 49 passed (30 + 19). These assert on the digest's/rhythm's JSON output (column, awaiting, blocked, action lists) — outcomes, not internal jq mechanics — so they'd survive an equivalent rewrite of the jq. shellcheck on both scripts is clean.
Minimal / no scope creep. The digest fix (defects 1–5) is tight and matches the stated root causes. backlog-rhythm.sh is a genuinely new capability (245 lines, a new "unattended pass" verb) rather than a pure bug fix — the PR body is explicit that this is deliberate ("Nothing runs any of this yet" / scheduling is out of scope) and ties it back to the same underlying finding (rules that never fire because nothing schedules them). Given there's no external requester to scope against here, I'm not treating this as a blocking scope violation, but flagging it since a stricter reading of "fix: ..." in the title would say the rhythm script belongs in a separate PR.
Nit (non-blocking): the inline board-priority lookup in the item comprehension
| ([ $board.items[]? | select(.content.number? == $i.number) | .priority? ][0] // null) as $prio
(scripts/backlog-digest.sh, right after awaiting_from_board) duplicates that function's select/filter/index-0 shape for a different field. Pulling it into a matching priority_from_board($n) def would read more consistently — no correctness impact.
Verification claim staleness (non-blocking): the PR body's Verification section says "25 tests pass (was 18)"; the current head has 30 in test_backlog_digest.py alone (49 combined with the new rhythm file), consistent with the two later commits (8bcd675, fdee734) landing after that paragraph was written. Numbers are directionally right, just not refreshed.
No rule violations found (no Optional[x], no hasattr/getattr fallbacks — N/A here since there's no Python; no exception string matching; no hardcoded credentials; no injection vectors — the gh shim heredocs use quoted delimiters and --argjson, consistent with the rest of the file). Approving.
Third round on the same finding, and the previous two "fixes" were asserted rather than demonstrated -- verification was quality-check.sh plus `bash -n`, neither of which executes the decision path. So this commit changes the mechanism AND adds the missing tests. The grace window was the wrong instrument, and the review named the reason: it competed against the ORIGINAL deadline instead of extending it, so a COMMENTED first seen in the last `grace` seconds of the window could never satisfy the `elapsed >= grace` branch -- the loop exited first and reported "no review landed", which was false. Observed live on #622: the stub landed 12:15:14, the real CHANGES_REQUESTED 12:16:39, and the script returned the stub. Sizing it differently would not have helped. The gap that matters is not placeholder-to-summary (16s on #622, 50s on #617) but placeholder-to-END-OF-RUN: the bot posts an early permission-check comment within a couple of minutes and works for five to eight more. No constant is both short enough to return a real COMMENT promptly and long enough never to pre-empt a summary. So ask the run instead. `review_run_state` reads the PR Review workflow run started since the trigger: running -> a COMMENTED decides nothing; keep waiting finished -> a COMMENTED last word IS the verdict, per pr-review.yml's own three-verdict contract (APPROVE / REQUEST_CHANGES / COMMENT) failed -> report at once; do not burn the timeout on a dead run none -> the trigger never reached the workflow, a different fault That last one matters as much as the first. A dead run and a thinking one are both silence if you only poll for reviews, which is how #623's run -- already failed on "Reached maximum number of turns (60)" -- was waited on for 16 minutes. Also from this review round: - pr-review.yml no longer REQUIRES `gh api` for inline comments. The reviewer reported that `gh api` is permission-gated and unavailable to it, and that it probed with `gh pr review` to find out -- which submits, and is where the stray "test permission check" reviews came from. The prompt now states the one hard rule (submit exactly ONE review, never probe with it), prefers `gh api` for inline notes, and says to fold findings into the summary with file:line when it is unavailable, rather than falling back to a second review. - SKILL.md Step 0 keyed a resume signal on `## Scope assessment`, which only CI mode writes into the PR body. An interactive-mode PR never carries it, so the row would not match for most PRs this skill opens. It now keys on the PR existing, which is what actually proves Step 9 was reached. 7 new tests, REVIEW_POLL_INTERVAL added as their seam. Two of them are the discriminating pair: identical reviews (COMMENTED only), opposite outcomes, differing only in run state -- so the decision is provably driven by the new signal and not by timing. The shim applies `--jq` like real gh does; an earlier version echoed raw JSON and the script reported it as a verdict.
…sume for dead sessions (#622) * fix: wait for a terminal review verdict, not the bot's placeholder `request-pr-review.sh` took the LAST review newer than its trigger and called it the verdict. The review bot posts its inline notes first, as a COMMENTED review whose body is "Inline notes below; summary review to follow.", then submits the real APPROVED/CHANGES_REQUESTED summary seconds later. Measured on PR #617: placeholder at 06:57:13Z, APPROVED at 06:58:03Z — 50 seconds apart. Any poll landing in that window returned COMMENTED. `implement-issue` Step 11 then saw a non-APPROVED verdict and skipped `gh pr ready`, so an approved PR stayed a draft with nothing left to do but the merge. PR #615 sat that way overnight: CHANGES_REQUESTED, fixed, APPROVED at 21:13, still a draft the next morning. Filter on state BEFORE taking `last`, so only APPROVED or CHANGES_REQUESTED ends the wait. Verified against #617's real review history by simulating a poll at 06:57:30Z, when the placeholder was the newest review: the old expression returns COMMENTED, the new one returns empty and keeps waiting. The timeout path also conflated two opposite faults that printed the same message — a review that started and never summarised, versus a trigger that never reached the workflow. It now reports which one happened. PR #619 is currently the second kind, and that was invisible before. Step 11's own text told the agent to act on `COMMENTED`, so it is corrected to match; a bare COMMENTED can no longer reach the caller at all. * feat: resume an issue whose session died mid-flight (implement-issue Step 0) Sessions die mid-issue routinely and nothing picked them up. A fleet audit found 34 worktrees whose sessions had exited: 8 with real unpushed commits and no PR (one with 32 commits), plus three PRs sitting green-or-reviewed with no owner left. #615 was APPROVED and still a draft the next morning; #614 carried CHANGES_REQUESTED with nobody to act on it. `sweep-prs` refuses that job by design, so the work simply stopped. This lives in `implement-issue` rather than a new skill because the loop that acts on review feedback is Step 11 and already lives here. A second skill would duplicate it, and duplicating a review loop is how one of them goes stale. Step 0 keys off state observable from OUTSIDE the dead session — branch, worktree, commits, PR body sections, CI status, review verdict — and re-enters at the earliest incomplete step. The one thing that dies with the session is Step 2's diagnosis, which Step 11 depends on holding; it is recoverable only because this skill already forces it to be written down (the Stage 2 analyze comment, and the PR body's `## Scope assessment` and `## Test plan`). When those do not reconstruct a coherent approach, Step 0 STOPS rather than re-diagnosing on top of commits encoding decisions it cannot see. Hard rules, each from an observed failure: - never run Step 4's fresh-from-origin/main worktree when a branch for the issue already has commits — that deletes them - never reset or force-push a resumed branch; its commits are the only copy - check for a live session unscoped AND unsandboxed: a sandboxed `claude agents --json` returned 1 session where the truth was 17, because ~/.claude/jobs is sandbox-denied, so every other session read as dead - treat uncommitted tracked changes as unfinished work; WIP-commit first - if the same issue has died twice, say so and stop CI mode gets a Step 0 row too: Stage 3 is re-triggered by hand, so a second `@claude-bot fix` on an issue that already has a has-fix-pr PR is a resume, not a restart, and must not open a second PR. * fix: resolve an ambiguous COMMENTED review by time, and stop emitting it Review of #622 found the previous commit's fix incomplete, and it was right. `pr-review.yml:75` documents COMMENT as a legitimate FINAL verdict ("questions/observations only"), submitted with `gh pr review --comment`, which produces the same state == "COMMENTED" as the bot's inline-notes placeholder. Treating every COMMENTED as non-terminal therefore swallowed a real COMMENT verdict: the loop waited out the full timeout and reported "never submitted a summary" while a summary with findings sat on the PR. That over-generalised "the placeholder is COMMENTED" into "COMMENTED is always the placeholder". Fixed at the source and in the consumer. Source: pr-review.yml step 3 permitted `gh pr review` for inline notes, and that is what submits the extra review. It now requires `gh api .../pulls/N/comments`, so exactly ONE review is submitted per run -- the step 4 summary. No placeholder means no ambiguity. Consumer: the script no longer decides by state alone, and deliberately does NOT parse the placeholder's body -- that text is bot-generated prose with no contract behind it. APPROVED/CHANGES_REQUESTED return immediately; a COMMENTED-only state is held `grace` seconds (180, against observed placeholder-to-summary gaps of 16s on #622 and 50s on #617) to let a summary supersede it, and is returned as the verdict if none does. The grace window is what keeps this correct for reviews already on older PRs and if the bot regresses. Verified against #622's real review history: with both reviews visible the decisive branch returns CHANGES_REQUESTED and grace is never entered; in a window containing only the placeholder the COMMENTED branch finds it while the decisive branch is empty, so grace holds. The timeout message is also now correct rather than merely different: a COMMENTED-only run can no longer reach it, so reaching it means no review of any state was submitted -- a trigger fault, which is what #619 hit twice. SKILL.md's Step 11 said "It will never hand you COMMENTED"; a COMMENTED that now reaches the caller IS the verdict, so it is documented as carrying findings and not earning the ready flag, same as CHANGES_REQUESTED. * feat: Step 0 resolves a bare number to an issue OR a pull request This skill is used for TODO.md items and for refactors that never had an issue, so "issue number" was too narrow a contract. Step 0 already keys off observable state; a PR is simply another entry point to it, and the stronger one -- it carries the branch, the diff, the `## Scope assessment` and the review verdict, which is everything Step 0 reads. No flag is needed. GitHub numbers issues and PRs from ONE sequence per repository, so a bare number is unambiguous: try `gh pr view <n>`, fall back to `gh issue view <n>`. An earlier draft used `--pr <n>`; that distinction carries no information. Why it matters beyond tidiness: scripts/backlog-rhythm.sh hands unfinished drafts back to this skill, and for a PR with no linked issue it had nothing to hand -- it reported "no issue references this PR; finish it by hand". That left every self-directed PR with no owner in the loop, which is how #620, #622 and #623 all ended up driven by hand in one session. Where an issue IS linked, nothing changes: it is still read for the diagnosis. Where none is, Step 2's root cause comes from the maintainer's own framing rather than a Stage 2 comment, and Step 9 records it in the PR body as usual. * fix: decide a COMMENTED review by run state, and test the decision Third round on the same finding, and the previous two "fixes" were asserted rather than demonstrated -- verification was quality-check.sh plus `bash -n`, neither of which executes the decision path. So this commit changes the mechanism AND adds the missing tests. The grace window was the wrong instrument, and the review named the reason: it competed against the ORIGINAL deadline instead of extending it, so a COMMENTED first seen in the last `grace` seconds of the window could never satisfy the `elapsed >= grace` branch -- the loop exited first and reported "no review landed", which was false. Observed live on #622: the stub landed 12:15:14, the real CHANGES_REQUESTED 12:16:39, and the script returned the stub. Sizing it differently would not have helped. The gap that matters is not placeholder-to-summary (16s on #622, 50s on #617) but placeholder-to-END-OF-RUN: the bot posts an early permission-check comment within a couple of minutes and works for five to eight more. No constant is both short enough to return a real COMMENT promptly and long enough never to pre-empt a summary. So ask the run instead. `review_run_state` reads the PR Review workflow run started since the trigger: running -> a COMMENTED decides nothing; keep waiting finished -> a COMMENTED last word IS the verdict, per pr-review.yml's own three-verdict contract (APPROVE / REQUEST_CHANGES / COMMENT) failed -> report at once; do not burn the timeout on a dead run none -> the trigger never reached the workflow, a different fault That last one matters as much as the first. A dead run and a thinking one are both silence if you only poll for reviews, which is how #623's run -- already failed on "Reached maximum number of turns (60)" -- was waited on for 16 minutes. Also from this review round: - pr-review.yml no longer REQUIRES `gh api` for inline comments. The reviewer reported that `gh api` is permission-gated and unavailable to it, and that it probed with `gh pr review` to find out -- which submits, and is where the stray "test permission check" reviews came from. The prompt now states the one hard rule (submit exactly ONE review, never probe with it), prefers `gh api` for inline notes, and says to fold findings into the summary with file:line when it is unavailable, rather than falling back to a second review. - SKILL.md Step 0 keyed a resume signal on `## Scope assessment`, which only CI mode writes into the PR body. An interactive-mode PR never carries it, so the row would not match for most PRs this skill opens. It now keys on the PR existing, which is what actually proves Step 9 was reached. 7 new tests, REVIEW_POLL_INTERVAL added as their seam. Two of them are the discriminating pair: identical reviews (COMMENTED only), opposite outcomes, differing only in run state -- so the decision is provably driven by the new signal and not by timing. The shim applies `--jq` like real gh does; an earlier version echoed raw JSON and the script reported it as a verdict. * fix: give the verdict tests their own env file so they pass in CI The new test file could not pass in CI, and the review caught it with the failing run: `Fast tests` was red on this PR while local `quality-check.sh` was green. Reproduced both sides before fixing. Cause: request-pr-review.sh posts its trigger through scripts/gh-agent.sh, which reads a real BESS_AGENT_TOKEN and exits 1 before `gh` is reached. Shimming `gh` on PATH does not help -- gh-agent.sh is invoked by a repo-relative path, not looked up on PATH. Worse, it resolves its env file from the MAIN checkout (`dirname $(git rev-parse --git-common-dir)`), so this worktree having no `.env` of its own was irrelevant: the developer's real token was read anyway. CI provisions no `.env` and no such secret, so the suite failed unconditionally there. The abandoned `(d / "scripts").mkdir(...)` in the fixture was an attempt at this that did nothing. Fixed with the seam gh-agent.sh already documents for exactly this (`BESS_ENV_FILE`, "a seam tests use to point at a fixture .env instead"), so no production code changes: each test now supplies its own env file carrying a dummy token. Also adds a test that PINS that dependency, because the fix is otherwise invisible and could be dropped again silently: point the seam at an empty file and the script must fail before polling, naming the missing token. Worth recording how I nearly mis-verified this: my first attempt set BESS_ENV_FILE from OUTSIDE pytest and saw the tests still pass, which looked like proof of CI-safety. It was not -- the test sets that variable in the subprocess env, so it overrides any outer value and the experiment could not fail. The in-test pin above is the version that can actually discriminate. Full backend suite: 507 passed. * fix: an unreadable run state must not promote a placeholder to a verdict Review of #622 found a real correctness bug, reproduced against the shipped script: `review_run_state` falls back to `unknown` whenever `gh run list` itself fails -- network blip, rate limit, transient auth error -- and the COMMENTED branch tested only `state = running`, so `unknown` fell through to the else and reported the bot's placeholder as the verdict. That re-opens the exact race this script exists to close, gated on API flakiness instead of timing. "I could not tell whether the reviewer is still working" must never mean "it finished". It now waits, same as `running`: waiting costs one more poll, and a genuine COMMENT verdict still returns as soon as the state resolves. A test drives it, with the shim making `run list` exit 1 rather than returning a run shape -- which is what the failure actually looks like. That test also exposed a second defect in the same path: the timeout branch ends with `gh run list ... >&2` for diagnostics, and under `set -e` a failing `gh` -- precisely the `unknown` case -- aborted the script with exit 1 instead of the exit 2 that means "no verdict". Diagnostics must not decide the exit code, so it is `|| true` now. Also fixes the doc/implementation mismatch the review flagged: SKILL.md still described the 180s hold from the superseded commit, which would actively mislead the next reader about how Step 11 decides. It now describes the run-state mechanism, including that an unreadable state waits. 9 tests. Worth noting the `pr-review.yml` change from this PR is already working: this round the bot folded its findings into a single summary review and posted no stray placeholder.
2 participants
Problem
The digest misclassified enough of the board that grooming couldn't be trusted — and every misclassification pushed the same way: work looked more ready than it was.
Measured against the live board (Project #1):
Ready for Dev: 0is the real finding. Nothing in the backlog is dispatchable — everything analysed is blocked or awaiting something. The single item that looked ready was a false positive, which is exactly why dispatching kept failing.Five defects, each traced to a real item
1.
analyzedwas tested before any wait. So an analysed-but-blocked item reportedReady. #96 was labelledanalyzed, prioritisedP2, carried no blocking label — and still wasn't implementable, because how to build it was undecided. It read as dispatchable, a session was dispatched at it, and that session deadlocked on three design questions with nobody there to answer. Waits now outrankanalyzed.2. The board's
Awaitingfield was never read. 17 items have it set (discussion×11,reporter×6); the digest derived its own value from labels instead. So grooming that had already been done had no effect on anything. The field is now authoritative, withawaiting_sourceandawaiting_suggestedexposed so an unset field gets reconciled rather than silently invented.3.
awaiting: discussionfired on any human comment — which is not a blocker. Thanks, a "me too", a follow-up question all pushed an item into Analysis. Only a recorded wait or a blocking label does that now.4. A worktree left on disk pinned its issue to
In Progressforever. #593, #571, #542 and #466 all reportedIn Progresswhile their PRs (#618, #579, #591, #517) had already merged. Staleness is now decided by comparing the worktree's own branch against merged PRs — an exact match, deliberately not the fuzzy issue-number match used to associate a worktree with an issue. Those answer different questions and conflating them is what caused defect 6 below.5.
blockeddidn't fail Ready. #571 reportedReady for Devwhile carrying theblockedlabel. Definition of Ready criterion 5 now holds, including an unresolvedBlocked by #N.Plus:
Ready for Devfinally requires aPriority— which the design always specified and the code deferred "until a board exists". It exists, and every item carries P1–P4.New signal: who spoke last
last_comment: {author, days, is_reporter, is_bot}.Without it the digest could not represent the transition that matters most to grooming — the reporter answering us. A comment count and a last-activity date cannot distinguish "the reporter attached the debug log we asked for" from "we posted a nudge" from "the reporter asked something new". All three just increment a number. #621 crossed the Definition of Ready line and nothing noticed.
A change I made and then rejected
I initially mapped a merged PR to
Done. It reclassified 7 open issues as finished (#118, #120, #403 among them), and it contradicts this project's own rule that beta PRs omitCloses #Nuntil graduation — so an open issue with a merged fix is the normal state, not a completed one.merged_pris now reported for information, moves no column, and a test pins that.Verification
./scripts/quality-check.shgreen —Errors: 0,Warnings: 0. 25 tests pass (was 18), including one per defect above and both halves of the stale-worktree rule, so the fix can't hide live work.Three test-harness bugs fixed along the way, each of which had been masking real behaviour:
ghshim matched on$1 $2, so bothpr listcalls returned the open list — every open PR would have looked merged. It now branches on the full argument string.createdAt, which realghalways sends. Their absence failedstrptimeinstead of testing anything._runusedcheck=True, so a jq failure surfaced as a barereturned non-zero exit status 5with the actual error swallowed. It now reports the digest's stderr.One implementation note: merged PR bodies are reduced to their closing references before reaching jq — passing 200 of them through
--argjsonoverflows the argument list (/usr/bin/jq: Argument list too long).Not in this PR
Nothing runs any of this yet. The design (
docs/superpowers/specs/2026-08-15-backlogger-agent-design.md:86-91) puts follow-up on a local/loop /backlog"Rhythm" surface and predicted this exact failure — "If reports start going stale, that is the signal to move Rhythm to an Actions cron". That condition has fired. Scheduling it is the remaining half and is deliberately separate, since this PR is what makes the signals worth acting on.🤖 Generated with Claude Code