feat: give issues and PRs an explicit state machine, so the backlog loop can progress work - #658
Conversation
…lumn stalled the fleet Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017uvZsYtTvvyhGFxjGc29kM
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017uvZsYtTvvyhGFxjGc29kM
…e-flight scan Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017uvZsYtTvvyhGFxjGc29kM
pr_for returned matches[0], silently discarding every PR past the first --
guaranteed to happen since the no-auto-close rule means an issue routinely
carries a beta PR and a separate graduation PR. Replaced with prs_for, which
returns the full sorted set as `prs: [{number, mergeable, isDraft}]`, and
taught both the open-PR matcher and the merged-PR refs scan the `Refs #N`
verb an intermediate PR is required to use. backlog-rhythm.sh and its tests
follow the same shape change.
…o null Review round 1 on Task 2: prs_for emitted isDraft on every PR object, but the open-PR gh pr list --json list never asked gh for that field, so it was permanently null in production and only ever populated in tests because the fixture handed the field over regardless of what --json actually requested. Added isDraft to the --json selection, and added a test that inspects the digest's real gh pr list invocation (failing loudly if isDraft is absent from --json) rather than trusting a fixture that would mask the same regression again.
column() gains In Verification for an issue whose fix merged to main but has not yet reached a stable release, and stops letting Awaiting rewrite the phase -- Status is the phase, Awaiting is the wait, and they are now orthogonal. Ready for Dev still requires no blocker and no wait, so an unsettled item still cannot read as dispatchable.
…eeds exactly Adds a top-level in_flight_files map (path -> [PR numbers]) to backlog-digest.sh, built from one `gh pr diff --name-only` per open PR. No error suppression on that call: a PR whose diff cannot be read must fail the digest, not silently read as touching nothing.
Awaiting is now signed: reporter/upstream/discussion mean someone else owes us and stay quiet, while maintainer means the loop cannot advance without a human decision and must rank first in the pass. Two more triggers derive an escalation the same way: two handed-back implementation sessions (resume_count >= 2), and three CHANGES_REQUESTED review rounds without an intervening approval. Both are guarded so the same item never also reports resume_implementation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017uvZsYtTvvyhGFxjGc29kM
test_two_resume_handoffs_escalate asserted only that escalated fired, never that resume_implementation was absent, so the and .resume_count < 2 guard on resume_implementation had no test that would catch its removal. Add the missing negative assertion; verified it fails with the guard removed and passes with it restored. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017uvZsYtTvvyhGFxjGc29kM
sort_by(.action, ...) put dispatchable before resume_implementation for no reason beyond d < m < r, so every backlog-rhythm pass led with start new work and buried finish started work -- the inverse of the empty-the-board- from-the-right flow policy. Each action now gets a numeric rank field (0 = escalations, then rightmost column first, unmatched actions rank last at 6) and actions sort on rank before action name.
set_awaiting, add_card, set_priority, move_card and triage_labels were landing last only because they fell through an unlabeled else branch -- correct by position, but any future action would silently inherit last place the same way, with nothing to flag it. Name all five at rank 6 explicitly (a no-op for current ordering), and move the catch-all to a distinct rank 9 so an action with no rank branch sorts conspicuously after grooming instead of blending into it.
8 open PRs, all drafts, 6 conflicting, none ever approved -- the observable shape of unbounded WIP. In Progress and In Review count as one piece of work (a branch and its PR at two stages); In Verification does not count, since it has already merged and is only waiting on a release. dispatchable is suppressed while over the limit, and the WIP line is now reported on every human-readable pass, including the quiet path, so a suppressed queue is never silent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017uvZsYtTvvyhGFxjGc29kM
Review finding on the WIP-limit commit: the exclusion was correct but unpinned, so a mutation widening the column filter to include In Verification would pass the existing suite silently. This is a deliberate semantic (already merged to main, waiting only on a release, occupying no implementation slot) rather than an accident of implementation, and exactly the kind of thing a future maintainer could plausibly "fix" the wrong way. Verified the new test discriminates: temporarily widened the jq select to include In Verification, confirmed the test fails (count 4/over True instead of 0/False), reverted, confirmed it passes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017uvZsYtTvvyhGFxjGc29kM
…evented Task 9 of the issue/PR state machine plan. backlog-rhythm.sh now checks a Ready for Dev candidate's predicted_files against in_flight_files (Task 5) before reporting dispatchable: a clash queues the candidate behind the in-flight PR, an unpredicted touch-set blocks dispatch outright, and two Ready items that would collide with each other are folded into one cluster. Also closes the gap the removed 2>/dev/null || true left in backlog-digest.sh: both scripts run under set -euo pipefail, so one PR whose diff gh cannot read used to abort the whole rhythm pass and block all triage/dispatch. A failed gh pr diff is now recorded in undiffable_prs instead, and backlog-rhythm.sh suppresses dispatchable fleet-wide (like the WIP limit) while emitting one undiffable_pr action per affected PR, reported in both JSON and human output. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017uvZsYtTvvyhGFxjGc29kM
…s a step not a session Every open PR resolves to one action today -- run /implement-issue at $1-4 a session -- which is why 8 PRs sit open, 6 conflicting, none approved. advance-pr moves Step 11's review loop out into its own skill: one transition-table row per PR fact, exactly one state advanced per invocation, then exit. Three callers reach it: implement-issue, backlog-rhythm.sh, and the maintainer directly -- one copy of the loop instead of a second one going stale, per backlog/SKILL.md's own argument.
…an PRs
Fix round 1 review findings on advance-pr:
- gh project item-edit's ID-based form requires --project-id alongside
--id/--field-id/--single-select-option-id or it fails outright
("project-id must be provided"); add the gh project view resolution step.
- gh project item-list defaults to --limit 30 and truncates silently past
it (57 items on the live board today); pass --limit 200 like
backlog-digest.sh does.
- a PR with no Refs #N / Closes #N has no issue to write Awaiting onto;
stop and report it rather than let the lookup silently return nothing.
Step 11 now invokes /advance-pr repeatedly instead of restating the verdict-handling loop, per the plan in .superpowers/sdd/2026-08-18-issue-pr-state-machine/. Also requires Refs #<issue> (not Closes) on every non-graduation PR, teaches Step 0 to open an issue for bare refactors/TODO items, and posts the resume-handoff marker on resume so double-handoffs are detectable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017uvZsYtTvvyhGFxjGc29kM
Fix round 1 review found three surviving lines that told a reader to run `gh pr ready` directly, or credited Step 11 itself with the flip, bypassing advance-pr for exactly the case Task 11 moved out of this skill. Worst was the Step 0 resume table, which told a resumed session to run `gh pr ready` on an APPROVED PR with no mergeability re-check or push-after-approval rule -- both now live only in advance-pr -- which is the #609 failure reintroduced. Reworded all four to route through advance-pr while keeping each line's original reasoning. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017uvZsYtTvvyhGFxjGc29kM
…s Step 2/3 context A bare /advance-pr <n> or a backlog-rhythm dispatch has no Step 2 diagnosis or Step 3 scope assessment to distinguish a real review finding from a decision already made and rejected deliberately -- exactly the failure superpowers:receiving-code-review exists to prevent. Split the transition table's CHANGES_REQUESTED row in two: rework in place only when the caller is implement-issue Step 11 (holds that context); otherwise collect the findings and hand back to /implement-issue <n> rather than guess.
Rewrites backlog/SKILL.md's states, rhythm action table, flow policy and issue-card deferral to match backlog-rhythm.sh/backlog-digest.sh as they actually run today, including two gaps found along the way: resume_implementation never calls /advance-pr directly (Step 0 does, via implement-issue Step 11), and predicted_files has no production data source yet, so the collision gate can only ever report needs_touch_set. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017uvZsYtTvvyhGFxjGc29kM
The rhythm pass handed every unfinished draft PR to a whole implement-issue session, which is the gap between this design and its own headline promise: advancing a PR costs a step, not a session. Splits the resume_implementation PR branch by verdict, using the same reviewDecision/last-non-COMMENTED-review seam already used elsewhere: a draft needing only a mechanical next step (no review yet, review in flight, approved but not green) now routes to /advance-pr <n>; a draft carrying CHANGES_REQUESTED still routes to /implement-issue <n>, because only that session holds the Step 2/3 context needed to tell a real review finding apart from a decision already made and rejected on purpose. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017uvZsYtTvvyhGFxjGc29kM
Seven fixes from a review of the advance-pr/backlog-rhythm/implement-issue branch: - CONFLICTING draft PRs now route to /advance-pr instead of a sweep-prs handoff that writes nothing and re-emits forever (non-draft still goes to sweep-prs). - request-pr-review.sh gates its trigger comment on the existing review_run_state() detector so a review already in flight is never re-triggered by a second caller. - advance-pr never edits code: CHANGES_REQUESTED always hands back to implement-issue, which is the session holding the Step 2/3 context; this supersedes the earlier "rework in place" carve-out. - Restored the gh api pulls/<n>/comments command in implement-issue Step 11 so inline review findings are actually fetched (they never show up in --json reviews or --json comments). - implement-issue's hard-cap text now says the 3-round escalation is derived by backlog-rhythm.sh, not written by advance-pr, matching advance-pr's own "two escalations this skill owns" section. - mark_ready's detail now names /advance-pr instead of a bare gh pr ready, so the mergeability re-check and push-after-approval rule can't be skipped at the one call site an unattended pass reads. - Deferral (Awaiting/P4) recorded on an issue card now suppresses its PR's actions the same way a pr_board card does, matching backlog SKILL.md's documented "one card per unit of work, on the issue" model. Every guard added or changed here was verified by mutation: inverted or removed, confirmed the covering test reddens, restored, confirmed green. See .superpowers/sdd/2026-08-18-issue-pr-state-machine/final-fix-report.md (gitignored, not part of this commit) for the per-finding evidence. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017uvZsYtTvvyhGFxjGc29kM
backlog/SKILL.md still routed every CONFLICTING PR to sweep-prs; the rhythm script has branched on isDraft since the advance-pr split, so a conflicted draft now goes to /advance-pr and only a non-draft still goes to sweep-prs. Narrow advance-pr's "never edits code" absolute (and its implement-issue echo) to what is actually true: it never reworks a review verdict, not that it never touches the diff at all -- merging main to resolve a conflict and fixing a red check stay mechanical rows. Also corrects a stale "blind rework" justification and an operator-facing rhythm detail string that still said advance-pr does the rework. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017uvZsYtTvvyhGFxjGc29kM
…-pr-state-machine
|
@claude-bot review |
There was a problem hiding this comment.
Review summary
Linked issue: none — this is self-directed infra work (backlog/PR tooling), not tied to a single GitHub issue. It carries its own design doc (docs/superpowers/specs/2026-08-18-issue-pr-state-machine-design.md) and 13-task plan (docs/superpowers/plans/2026-08-18-issue-pr-state-machine.md), which is this project's established substitute for a root-cause writeup on a change this structural — consistent with prior tooling PRs (#656, #654, #651) that also skip an issue link and a CHANGELOG entry.
Scope: Large diff, but coherent — one state-machine redesign (Status/Awaiting orthogonality, WIP limit, collision gate, advance-pr extraction) touching only .claude/skills/, scripts/backlog-*.sh, scripts/request-pr-review.sh, their tests, and the two doc artifacts. No drive-by changes to unrelated code. CI is green (Fast tests, E2E, Docker boot, Code quality all pass); the PR body's "Unrelated" section correctly disclaims a pre-existing flaky test (test_dismiss_persists_across_requests) rather than silently leaving it unmentioned.
Would the tests catch a regression? Yes. I read through test_backlog_digest.py, test_backlog_rhythm.py, and test_request_pr_review.py in full. Coverage is unusually thorough for the new logic:
prs_for/multi-PR join (test_an_issue_with_several_prs_reports_all_of_them,test_issue_matched_by_two_prs_emits_one_item_with_both_prs) — regression-tests the exactpr_for→$matches[0]bug called out in the PR body.column()precedence (test_an_open_pr_outranks_a_merged_one,test_a_wait_no_longer_rewrites_the_phase,test_a_wait_no_longer_outranks_a_live_worktree) — I traced these against the actualcolumn()jq function (scripts/backlog-digest.sh:393-401) and the assertions match its elif order exactly.- WIP limit boundary (
test_over_the_wip_limit_suppresses_dispatch,test_in_verification_does_not_count_as_wip,test_a_branch_and_its_pr_count_as_one) and the collision gate (queued_behind/cluster/needs_touch_set/undiffable_pr) are each exercised at their edges, not just the happy path. request-pr-review.sh's new in-flight gate has both a positive and negative test (test_an_in_flight_review_is_not_re_triggered,test_no_in_flight_run_still_triggers_normally), each proven by an independent side channel (empty token → must never reachgh-agent.sh) rather than by re-asserting internal state — satisfies the "test must be seen to fail without its fix" bar.
These tests verify observable behavior (which action/column/detail string comes out) rather than internal field names or algorithm internals, in line with docs/agents/testing.md.
Rule compliance: No Python source files are touched (tests only), so the Optional/hasattr/sensor-mapping/camelCase rules don't apply here. I didn't find a workaround-shaped fix (no new parameter/flag/second construction site added just to route around an ordering problem) — e.g. the request-pr-review.sh in-flight gate fixes the double-trigger race directly by checking run state before posting, rather than adding a retry/backoff band-aid. scripts/backlog-digest.sh's new gh pr diff loop correctly avoids swallowing errors (undiffable_prs records failures as data instead of 2>/dev/null || true, per its own comment) and doesn't let one bad PR abort the whole digest.
Correctness: Traced the core jq logic (column(), prs_for, merged_pr_for, the WIP/collision-gate block in backlog-rhythm.sh) by hand against the tests and found nothing wrong. request-pr-review.sh's > → >= cutoff change is correct given since can now be set to the in-flight run's own createdAt.
No blockers found. Approving.
Gives issues and PRs an explicit state machine, so every state has one cheap next action and the Product Owner loop can actually progress work.
Why
The fleet had stalled completely: 8 open PRs, all drafts, 6 CONFLICTING, none ever approved. Four causes, each measurable:
if $pr != null then "In Review", tested first, outranking everything. A never-reviewed draft, a conflicted draft and an approved-and-waiting PR all reported the same state./implement-issueat $1-4. Nobody spends five of those in a tick, so the queue only grew.sort_by(.action, ...)putdispatchableaboveresume_implementationbecaused < m < r-- every pass led with "start new work" and buried "finish started work".A fifth defect surfaced during design:
pr_forreturned$matches[0], discarding every PR after the first -- while the project's own no-auto-close rule guarantees an issue has several.What changed
State model.
Statusis the phase,Awaitingis the wait, and they are now orthogonal.In Verificationnames the merged-to-main-but-unreleased period that previously had no representation.Awaitingbecomes signed:reporter/upstream/discussionstay quiet because someone else owes us;maintaineris loud and ranks first, inverting a field that previously made an item blocked on you quieter.advance-pr(new skill). The single copy of the review loop, extracted fromimplement-issueStep 11 -- not duplicated, moved. It advances exactly one PR by one state and exits, never re-reading the implementation context. Three callers:implement-issue, the rhythm pass, and you.Flow policy. Actions rank right-to-left across the board (escalations first, then finish before start). WIP limit of 3 counting
In Progress+In Reviewtogether. A collision gate that refuses to dispatch work touching a file already in flight, and clusters two Ready items that would fight over one file into a single unit.One issue, many PRs. Every PR carries
Refs #N; only the graduation PR carriesCloses #N. The issue's column derives from all its open PRs.Test plan
test_backlog_digest.py,test_backlog_rhythm.py,test_request_pr_review.py./scripts/quality-check.shgreen after mergingorigin/mainKnown gaps, deliberately not fixed here
predicted_fileshas a producer. The consumption side is specified and tested; nothing populates it yet, soneeds_touch_setfires for every Ready item anddispatchablenever appears from the script alone. It fails shut -- it blocks dispatch rather than waving collisions through. The tests fabricate that field, so their passing is not coverage of the missing half.pr_boardis still read by the scripts although the design retires PR cards. Deferral now reads both card types, so this is safe today and safe to remove later.request-pr-review.shnow checks for an in-flight run before posting, but GitHub creates the run seconds after the comment lands, so two callers inside that window can still double-trigger. Closing it needs a check on recent review comments, not runs.Ordering after merge
In VerificationStatus option and themaintainerAwaiting option to Project . #1, then delete the now-stale note inadvance-pr/SKILL.mdthat saysmaintainerdoes not exist yet.pr_boardremoval.$pr_boardand un-parks every deferred PR.Until step 1, the digest computes
In Verificationandmove_cardfires for it every tick with no valid target.Unrelated
backend/tests/test_dashboard_api.py::TestHistoricalDataStatus::test_dismiss_persists_across_requestsis order-dependent and fails in some suite orderings. Proven unrelated to this branch: the file is byte-identical to the branch base and it fails with this branch's test files excluded entirely. It passes on this branch as it stands.🤖 Generated with Claude Code
https://claude.ai/code/session_017uvZsYtTvvyhGFxjGc29kM