fix: make a backlog pass runnable without a shell preamble - #626
Conversation
`/loop /backlog` died on its first line, every tick. `backlog-digest.sh` required PROJECT_NUMBER in the environment, but it lives in the gitignored `.env` that nothing exports — so the script exited "the backlog board has not been created yet", which is the most misleading message it could emit: the board is Project #1, populated, and entirely fine. The documented workaround (`set -a; . ./.env; set +a; ...`) cannot survive an unattended pass, because the caller is a skill and not a shell someone typed into. The digest now sources `.env` itself, located via `git rev-parse --path-format=absolute --git-common-dir`. That is one rule rather than a search over candidate paths: `.env` is gitignored, so it exists only in the main checkout and never in a worktree, and the common dir resolves to the main checkout from either. `--path-format=absolute` is load-bearing — git otherwise answers the relative `.git`, whose dirname is `.`, silently resolving against the caller's cwd. The environment still wins over the file, or an explicit `PROJECT_NUMBER=2` would be un-overridable. Two gaps the skill could not close without a second API call by hand: - `board_status` — where the card sits NOW, alongside `column` (where the evidence says it belongs), in the same six Status strings. The `board` verb says "reconcile every card against the derived column, the digest always wins" and had nothing to reconcile against. Seven live mismatches were invisible. - `issue_no_card` — an open issue with no card at all. `Ready for Dev` requires a Priority and priority is a board field, so an off-board issue can never become dispatchable however well analysed, while reading as an ordinary Backlog item. #621 and #624 were both in that state. The rhythm pass told the PO to "set Priority" on those two, i.e. to set a field on a card that does not exist; it now asks for `add_card`, and reports `move_card` for a card the evidence disagrees with. Tests pin all of it. The git shim answers `rev-parse` so `.env` discovery is deterministic — letting the real git through would read the maintainer's own gitignored file, which CI does not have. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012LExo6fcbup75vtc9NfoAR
|
@claude-bot review |
The rhythm surface resolved any non-draft PR to `awaiting_maintainer`, "nothing left but your merge". That rests on an assumption that does not hold: that only Step 11 clears the draft flag, and only after an APPROVED verdict. #626 broke it in the obvious way. It was flipped out of draft by hand because it LOOKED stuck, had zero reviews at the time, and the next pass duly reported it as ready to merge. A Stage 3 CI-mode PR has the same hole from the other direction — it never runs Step 11 at all. Stage 4 is the gate the entire pipeline is built around, so a surface that routes around it is worse than no surface. An APPROVED review is now the bar, matching Step 11 exactly. Not `reviewDecision`, which reads "" both for never-reviewed and for approved-then-dismissed; and not COMMENTED, which the bot also posts as a placeholder before its real verdict. Two new actions carry the cases that used to be silently collapsed into "merge it": `request_review` (Stage 4 never ran) and `rework_review` (out of draft with changes requested). Both fields were already fetched by `gh pr list` and simply unused. Live effect on the current fleet: #490 stays `awaiting_maintainer` — it carries real APPROVED reviews — and #626 becomes `request_review`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012LExo6fcbup75vtc9NfoAR
|
@claude-bot review |
![bess-manager-claude-bot[bot]](https://avatars.githubusercontent.com/in/3024553?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
Reviewed against the linked motivation (no linked GitHub issue; this is a self-directed infra fix for /loop /backlog). The .env-sourcing fix (scripts/backlog-digest.sh) is solid and I reproduced it working end-to-end in a scratch repo — git rev-parse --path-format=absolute --git-common-dir correctly resolves to the main checkout from a worktree, the env-wins-over-file override logic is correct, and the new tests (test_project_number_is_read_from_dotenv, test_environment_wins_over_dotenv, test_a_dotenv_without_the_variable_still_fails_loudly) exercise it properly with a git shim (no real git leaking through, per the PR's own stated concern).
The board_status/column reconciliation addition (issue_no_card orphan, add_card vs set_priority mutual exclusion) is also correct and well-tested.
Blocking issue: the new review-state check contradicts its own reviewDecision signal (scripts/backlog-rhythm.sh:230-238)
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", ...}
else {pr: .number, issue: $issue_no, action: "request_review", ...}
end)This checks "does an APPROVED review exist anywhere in history" before checking reviewDecision. GitHub does not retroactively change an old review's state when a later review (same or different reviewer) requests changes — the old review stays APPROVED in the reviews array, while reviewDecision correctly updates to CHANGES_REQUESTED. So a PR that was approved once, then had changes requested in a later round (a normal rework cycle — the PR already tests exactly this reviews shape for drafts, see test_every_unfinished_draft_resolves_to_one_handoff's changes_requested = _pr(614, reviews=[{"state": "APPROVED"}, {"state": "CHANGES_REQUESTED"}])), falls into the first branch and is reported awaiting_maintainer / "nothing left but your merge" for a non-draft PR.
I reproduced this against the actual PR script:
$ prs.json: [{"isDraft":false,"reviewDecision":"CHANGES_REQUESTED",
"reviews":[{"state":"APPROVED"},{"state":"CHANGES_REQUESTED"}], ...}]
$ bash scripts/backlog-rhythm.sh --json
{
"actions": [{"pr": 700, "action": "awaiting_maintainer",
"why": "out of draft and approved",
"detail": "nothing left but your merge"}]
}
That is precisely the failure mode this PR sets out to close — routing a maintainer to merge on a stale approval instead of the current reviewDecision, undermining "never report an unreviewed PR as ready to merge." The fix should key off reviewDecision == "APPROVED" (or the latest review per reviewer) rather than "any APPROVED review ever submitted." The changes_requested fixture at line 614 in test_backlog_rhythm.py proves the author considered this exact reviews shape, but only wrote a test for it under the draft branch (where it's masked because all draft outcomes collapse to resume_implementation) — there's no equivalent non-draft test, which is how this got through.
Checklist
- Root cause match: the
.env-sourcing half matches the stated root cause (unattended shell has nothing exported) — confirmed correct and reproduced. - Regression coverage: the digest tests would catch a
.env-sourcing regression. The rhythm tests do not cover the review-ordering bug above — see reproduction. - Scope: no scope creep; the
board_status/issue_no_card/review-classification additions are all described in the PR body as gaps found while fixing the primary issue, and stay within the two scripts that already own this domain. - Rule violations: none found (no
Optional[x], nohasattr/silent fallbacks — N/A, bash/jq only; no secrets; comments are WHY-only and consistent with existing file style).
Everything else here is good work — please fix the review-state check (or explain if there's a reason reviewDecision is deliberately not the primary signal that I'm missing) and I'll re-review.
…e 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
|
@claude-bot review |
There was a problem hiding this comment.
Summary
This PR fixes the /loop /backlog unattended-run failure (PROJECT_NUMBER not exported) plus two related gaps found while exercising the digest/rhythm pass for real: missing board-reconciliation fields (board_status, issue_no_card), and a PR review-state rule that could report an unreviewed/stale-approved PR as merge-ready.
Root-cause match: no linked GitHub issue — this is a self-directed infra fix for /loop /backlog, and the PR body's own evidence (exact error text, .env contents, gitignore status) supports the stated root cause: nothing exports PROJECT_NUMBER for an unattended caller. Confirmed by reading scripts/backlog-digest.sh:12-45 — env_file="$(dirname "$(git rev-parse --path-format=absolute --git-common-dir)")/.env" resolves to the main checkout from a worktree, sources it with set -a/set +a, and preserves an explicit environment override (env_project_number/env_po_token captured before sourcing, restored after) — correct and matches the PR's stated intent.
Superseded review: an earlier bot review on this PR (commit eaae079a) flagged scripts/backlog-rhythm.sh for trusting "any APPROVED review ever" ahead of reviewDecision, which would misreport a PR that was approved then had changes requested (reviews=[APPROVED, CHANGES_REQUESTED], reviewDecision="CHANGES_REQUESTED") as awaiting_maintainer. The current HEAD (cee707c2) has since been rewritten to check .reviewDecision first (CHANGES_REQUESTED wins outright; ""/null fall back to the last non-COMMENTED review), and test_a_stale_approval_does_not_survive_a_later_changes_requested (PR #700, the exact reviews shape from that review) now passes. I re-verified this by hand:
elif (.isDraft | not)
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 {..., action: "rework_review", ...}
elif .reviewDecision == "APPROVED" or $last_verdict == "APPROVED"
then {..., action: "awaiting_maintainer", ...}
else {..., action: "request_review", ...}
end)That prior blocker reads resolved.
Would the tests catch a regression? Yes, and I checked this the hard way, not just by reading it: I cloned the branch, reverted scripts/backlog-digest.sh, scripts/backlog-rhythm.sh and the SKILL.md to main, and re-ran the new tests against pre-fix code. 13 of the 15 new tests fail RED exactly as claimed (.env sourcing, board_status/issue_no_card, worktree_locked, and all 5 review-state tests), confirming they exercise real behavior, not tautologies. With the fix restored: 69 passed in test_backlog_digest.py + test_backlog_rhythm.py. Also ran shellcheck on both .sh files (clean) and black --check / ruff check on both test files (clean).
Is the change minimal? The PR bundles three related fixes (.env sourcing, board reconciliation fields, PR review-state ordering) into one PR, but the body documents all three as gaps discovered while actually running the fixed pass end-to-end, and every change stays inside the two scripts (backlog-digest.sh, backlog-rhythm.sh) that already own this domain plus their own tests and skill doc — no scope creep into unrelated code.
Nit (non-blocking)
.claude/skills/backlog/SKILL.md's rhythm "Actions, and who does what" table (~line 203-215) lists resume_implementation, awaiting_maintainer, request_review, rework_review, resolve_conflict, recheck_ready, nudge_reporter, park, surface_discussion, and groups set_awaiting/set_priority/triage_labels — but doesn't add rows for the two new action codes this PR introduces in scripts/backlog-rhythm.sh: add_card and move_card. The skill explicitly says "Do not re-derive these by reading issues; act on what it lists," so an agent working purely off that table has no row for either new code. Low severity — each action's own detail field is self-describing ("add it to Project #1, then set Priority", "move the card to (.column)") — but worth a follow-up for consistency with the rest of the table.
Verdict
No rule violations found (bash/jq only — Optional/hasattr/class/entity-ID rules don't apply; no exception-string matching; no secrets; comments are WHY-only and match file style). No correctness bugs found in the logic I traced, and the prior reviewer's blocking concern is now fixed with a test reproducing that exact scenario. Approving, with the one documentation nit above for a follow-up.
2 participants
Three fixes to the Product Owner tooling, all found by actually running
/loop 30m /backlogagainst the live fleet. Each had the same shape: the surface asserted a state it had not checked, and every failure pointed the same way — toward acting on work that was already handled.1. A backlog pass could not run at all
/loop /backlogdied on its first line, every tick:Every clause of that is wrong. The board is Project #1, populated, 37 cards.
PROJECT_NUMBER=1lives in the gitignored.env, which nothing exports, andbacklog-board-init.shdoes not exist because the board was created by hand.The skill documented a workaround —
set -a; . ./.env; set +a; ./scripts/backlog-digest.sh— but a workaround needing a shell preamble cannot survive an unattended pass, because the caller is a skill, not a shell someone typed into.Fix: the digest sources
.envitself, located viagit rev-parse --path-format=absolute --git-common-dir. That is one rule, not a search over candidate paths:.envis gitignored, so it exists only in the main checkout and never in a worktree, and the common dir resolves to the main checkout from either.--path-format=absoluteis load-bearing — git otherwise answers the relative.git, whose dirname is., silently resolving against the caller's cwd. The environment still wins over the file, or a pinnedPROJECT_NUMBER=2would be un-overridable.Two gaps this exposed
Both were things the
boardverb needed and had to recover with a secondgh project item-listcall by hand — the one thing the digest exists to stop.board_status— where the card sits now, next tocolumn(where the evidence says it belongs), in the same six Status strings. The skill says "reconcile every card against the derived column, the digest always wins" and the digest carried nothing to reconcile against. Seven live mismatches were invisible: #602, #593, #592, #578, #571, and #621/#624 which had no card at all.issue_no_card— an open issue with no card.Ready for Devrequires a Priority, and Priority is a board field, so an off-board issue can never become dispatchable however well analysed, while reading as an ordinary Backlog item.2. An unreviewed PR was reported as ready to merge
The rhythm resolved any non-draft PR to
awaiting_maintainer, "nothing left but your merge". It never looked at reviews.That rests on an assumption which does not hold: that only Step 11 clears the draft flag, and only after an APPROVED verdict. This PR broke it. It was flipped out of draft by hand because it looked stuck, had zero reviews at the time, and the next pass duly reported it as ready to merge — routing straight around Stage 4, the gate the whole pipeline is built on. A Stage 3 CI-mode PR has the same hole from the other side: it never runs Step 11 at all.
Fix: an approving review is the bar, matching Step 11. Two new actions carry what used to collapse into "merge it":
request_review(Stage 4 never ran) andrework_review(changes requested). Both fields were already fetched bygh pr listand simply unused.3. Live work reported as stalled, and stale approvals as merge-ready
3a. Review state — neither signal alone is correct
The first attempt at fix 2 asked "is there an APPROVED review anywhere" before consulting
reviewDecision. The Stage 4 review caught this and reproduced it, correctly: 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 fix 2 set out to close, reintroduced by the fix for it.The review proposed keying on
reviewDecisioninstead. That alone is also wrong here, and measurably so:reviewDecisionis only populated when the repo requires reviews, and this one does not. It readsCHANGES_REQUESTEDfor #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
reviewDecisionwhen set, otherwise fall back to the last non-COMMENTED review. Last, not any — same staleness trap. COMMENTED is skipped because the bot posts inline notes as a COMMENTED review before its real verdict (seerequest-pr-review.sh).3b. Session liveness —
claude agentscannot answer the questionresume_implementationkeyed offsession == null, andsessioncomes fromclaude agents, which lists background agents only. Two independent failures:claude, then/implement-issue <n>— is a foreground session and never appears at all. That is how Beta 10 never goes through initialisation #624 was dispatched."Review PR and create branch for bess-manager"), not theissue-<n>the dispatch convention promises, so the exact-name match misses it too.Measured: 41 worktrees on disk,
claude agents --jsonreturning one entry. Sosessionwas null for essentially everything, every worktree read as abandoned, andresume_implementationfired on live sessions — advising a second session onto a branch its own detail line calls the only copy.Fix: the worktree lock is what tracks a live session. Git records it as
locked claude session <name> (pid 97626 start ...), written by Claude Code itself. 4 of those 41 were locked — exactly the four live sessions, foreground and background alike. It is local and needs no process list.sessionis kept because a name match is strictly more informative when it does happen; it is just no longer what liveness rests on.Live effect on the current fleet
resume_implementation— "no live session"awaiting_maintainer— "nothing left but your merge"rework_reviewreviewDecision: "")awaiting_maintainerawaiting_maintaineradd_cardVerification
./scripts/backlog-digest.shand./scripts/backlog-rhythm.shboth run end-to-end from a clean shell with nothing exported.pytest -m "not slow");./scripts/quality-check.shgreen.test_a_stale_approval_does_not_survive_a_later_changes_requested) and the case their proposed fix would have broken (test_an_approval_is_honoured_when_review_decision_is_empty).rev-parseso.envdiscovery is deterministic — letting the real git through would read the maintainer's own gitignored file, which CI does not have. The porcelain fixture uses the reallocked <reason>shape, not the bare keyword.Scope
Four files, all inside the domain these two scripts already own:
scripts/backlog-digest.sh,scripts/backlog-rhythm.sh, their two test files, and.claude/skills/backlog/SKILL.md(the stale.envpreamble and the changedawaiting_maintainersemantics). No product code touched, so no CHANGELOG entry — this is agent tooling with no user-visible effect.Note for reviewers
The jq program is a single-quoted shell string, so one apostrophe in a comment ends it early and jq reports the useless
Top-level program not given. That cost a debugging round here; there is now a comment saying so.🤖 Generated with Claude Code