🐛 fix(kanban): auto-advance a PR-open code card to review on clean-exit-after-done - #76
Conversation
…it-after-done A code-author card whose worker opens a PR, pushes, and exits rc=0 without calling a terminal kanban verb reaches detect_crashed_workers' clean-exit- after-done branch: _lane_work_provably_done is True (a PR URL in a recent comment), so the reap correctly declines to count a failure. But it only RELEASED the card to `ready`, which is a dead end for a PR-open card — the active_pr respawn guard holds an open-PR card out of respawn (dup-PR risk) WITHOUT advancing it, so the card wedged in running->ready until an orchestrator hand-staged it to review. Fix: when the provably-done signal is a PR handoff (a pull/<n> URL via _card_has_pr_artifact) AND the card's submit-stage owner map declares a review owner, MOVE the card running->review + that reviewer atomically with the reap, emitting the same status_changed running->review event shape complete_task's author-lane handoff emits, and recording the run as completed. This is exactly the handoff the worker's clean exit skipped. The two other clean-exit-after-done shapes are unchanged: a completed-run proof with no PR, a no-PR edit-in-place card (the prior no-PR fix), and a PR card with no resolvable owner-map reviewer all still release as a benign no-op to ready/review. The auto-advance is gated on both a PR artifact and a resolvable owner-map reviewer, so it narrows to exactly the code-author-opened-a-PR case. Adds behavior/E2E tests exercising both shapes against a temp HERMES_HOME: a PR-open code card with an owner map lands in review + the reviewer with the transition events, and a PR card without an owner map falls back to the benign release-to-ready.
cwest
left a comment
There was a problem hiding this comment.
No changes needed.
The fix lands where the bug lives: the clean-exit-after-done branch in detect_crashed_workers released a provably-done card to ready, which is a dead end for a PR-open card because the active_pr respawn guard holds it out of respawn without ever advancing it. Gating the auto-advance on both a PR artifact and a resolvable review owner from the card's own submit-stage map narrows it to exactly the code-author-opened-a-PR case; a no-PR edit-in-place card or a PR card with no owner-map reviewer still falls through to the benign release-to-ready, so the prior no-PR path is untouched.
The review MOVE mirrors complete_task's canonical author-lane handoff rather than inventing a new shape: run outcome completed, a single status_changed running->review event carrying the assignee, no failure counted, no crashed event. It folds the assignee into the status_changed payload exactly as complete_task does, so both paths look identical on the board. The UPDATE keeps the status='running' AND worker_pid AND claim_lock compare-and-set, so the transition stays race-safe against a concurrent claim.
Tests cover both shapes: the PR-open card asserting review + the owner-map reviewer with the status_changed event and zero failures, and the PR-card-without-owner-map falling back to ready while still surfacing through the clean_exit_after_done side channel. Ran the full file in a throwaway clone at the head SHA: 276 passed, 0 failed. The PR #75 no-PR regression tests pass unchanged. The commit is signed, follows the convention, and carries no AI attribution.
…it-after-done (#76) A code-author card whose worker opens a PR, pushes, and exits rc=0 without calling a terminal kanban verb reaches detect_crashed_workers' clean-exit- after-done branch: _lane_work_provably_done is True (a PR URL in a recent comment), so the reap correctly declines to count a failure. But it only RELEASED the card to `ready`, which is a dead end for a PR-open card — the active_pr respawn guard holds an open-PR card out of respawn (dup-PR risk) WITHOUT advancing it, so the card wedged in running->ready until an orchestrator hand-staged it to review. Fix: when the provably-done signal is a PR handoff (a pull/<n> URL via _card_has_pr_artifact) AND the card's submit-stage owner map declares a review owner, MOVE the card running->review + that reviewer atomically with the reap, emitting the same status_changed running->review event shape complete_task's author-lane handoff emits, and recording the run as completed. This is exactly the handoff the worker's clean exit skipped. The two other clean-exit-after-done shapes are unchanged: a completed-run proof with no PR, a no-PR edit-in-place card (the prior no-PR fix), and a PR card with no resolvable owner-map reviewer all still release as a benign no-op to ready/review. The auto-advance is gated on both a PR artifact and a resolvable owner-map reviewer, so it narrows to exactly the code-author-opened-a-PR case. Adds behavior/E2E tests exercising both shapes against a temp HERMES_HOME: a PR-open code card with an owner map lands in review + the reviewer with the transition events, and a PR card without an owner map falls back to the benign release-to-ready. (cherry picked from commit bffd227)
Why
A code-author card whose worker opens a PR, pushes, and exits rc=0 without calling a terminal kanban verb reaches
detect_crashed_workers'clean_exit_after_donebranch:_lane_work_provably_doneis True (a PR URL in a recent comment), so the reap correctly declines to count a failure. But it only released the card toready— a dead end for a PR-open card, because theactive_prrespawn guard holds an open-PR card out of respawn (dup-PR risk) without advancing it. The card wedged inrunning→readyuntil an orchestrator hand-staged it toreview, making a completed pipeline look like churn.What
In the
clean_exit_after_donepath, when the provably-done signal is a PR handoff (apull/<n>URL via_card_has_pr_artifact) and the card's submit-stage owner map declares areviewowner, MOVE the cardrunning -> review+ that reviewer atomically with the reap — the exact author-lane handoff the worker's clean exit skipped. It emits the samestatus_changed running->reviewevent shapecomplete_task's handoff emits and records the run ascompleted.The other clean-exit-after-done shapes are unchanged and fall through to the benign release:
The auto-advance is gated on both a PR artifact and a resolvable owner-map reviewer, so it narrows to exactly the code-author-opened-a-PR case.
Tests
Two behavior/E2E tests in
tests/hermes_cli/test_kanban_db.pyagainst a tempHERMES_HOMEexercising the real reap path:review+ the reviewer, with thestatus_changed running->review+ assignee events and no failure counted;ready.Full
tests/hermes_cli/test_kanban_db.py(276) green; the broader kanban surface (1226 tests across 60 files) green; 0 regressions. (One macOS-local-only failure intest_signal_handler_kanban_worker.pyis a pre-existing/proc-absent environment defect, untouched by this change and green on Linux/CI.)Base:
cwest/integration.