🐛 fix(kanban): clear the active_pr respawn guard on an outer-loop feedback bounce - #72
Conversation
…dback bounce
Routing an accepted card back to its author on outer-loop feedback had no
code path that cleared the active_pr respawn guard. A raw move_card
blocked/acceptance -> ready emits only status_changed, never the unblocked
cutoff event check_respawn_guard honors, so the guard stayed active_pr (the
PR is genuinely open) and the dispatcher refused to spawn the author every
tick. Repeated churn inflated block_recurrences past BLOCK_RECURRENCE_LIMIT
and escalated the card to triage, where a naive block->unblock cutoff
silently no-ops.
Add route_feedback_to_author, a sanctioned caller-driven primitive that
performs the whole outer-loop bounce atomically, composed from existing
building blocks so the inner review-bounce loop, the reviewer PASS ->
acceptance path (accept_task), and the merge -> done path are all unchanged:
1. reset_block_recurrences (frees a triage card, prevents re-escalation),
2. reassign to the author (assigned event), fenced on a transitionable lane,
3. unblock_task (emits the unblocked cutoff AFTER the PR-URL comment ts,
which is what clears the active_pr / recent_success guard),
4. recompute_ready + an audit comment naming the PR + feedback.
Recovers a card from blocked, acceptance (blocked + acceptance owner), and
triage. Idempotent: a second call on an already-routed card is a clean no-op.
Behavior-contract tests assert the guard clears (check_respawn_guard is None),
the card is spawnable via the real dispatch_once path, the triage counter
resets, and a reverted guard-clear leaves active_pr and fails.
cwest
left a comment
There was a problem hiding this comment.
The primitive is built entirely from existing sanctioned pieces, and each one behaves the way this code relies on. reset_block_recurrences frees a triage card and zeroes the loop counter; the fenced reassign emits the assigned event; unblock_task accepts blocked/scheduled/triage and writes the unblocked event now, causally after the PR-URL handoff comment. That ordering is the whole mechanism: check_respawn_guard sets its pr_cutoff to max(24h window, latest unblocked ts), so an unblock stamped after the PR comment pushes the comment out of the scan window and the active_pr guard clears. Same story for recent_success. Read the guard against integration HEAD and it lines up exactly.
Scope is right. The triage transition in unblock_task is already live, and this change doesn't re-touch it. Nothing wires the primitive into a live dispatch path yet, which matches the plan to add the caller separately. The inner auto_route_review_bounce path, accept_task, and the merge-to-done path are all left alone, and the surrounding kanban suite stays green.
Tests assert behavior, not snapshots: guard cleared from the acceptance lane, the routed card actually spawning through dispatch_once, triage recovery with the counter reset, idempotency, the blank-author raise, the missing-card no-op. I reproduced the RED check independently by removing the unblock step from a scratch copy of the file: five tests fail with the guard still active, which is the load-bearing behavior. Restored, then ran the new file (9 pass) and test_kanban_db.py (271 pass). All CI checks pass on this SHA.
No changes needed.
…dback bounce (#72) Routing an accepted card back to its author on outer-loop feedback had no code path that cleared the active_pr respawn guard. A raw move_card blocked/acceptance -> ready emits only status_changed, never the unblocked cutoff event check_respawn_guard honors, so the guard stayed active_pr (the PR is genuinely open) and the dispatcher refused to spawn the author every tick. Repeated churn inflated block_recurrences past BLOCK_RECURRENCE_LIMIT and escalated the card to triage, where a naive block->unblock cutoff silently no-ops. Add route_feedback_to_author, a sanctioned caller-driven primitive that performs the whole outer-loop bounce atomically, composed from existing building blocks so the inner review-bounce loop, the reviewer PASS -> acceptance path (accept_task), and the merge -> done path are all unchanged: 1. reset_block_recurrences (frees a triage card, prevents re-escalation), 2. reassign to the author (assigned event), fenced on a transitionable lane, 3. unblock_task (emits the unblocked cutoff AFTER the PR-URL comment ts, which is what clears the active_pr / recent_success guard), 4. recompute_ready + an audit comment naming the PR + feedback. Recovers a card from blocked, acceptance (blocked + acceptance owner), and triage. Idempotent: a second call on an already-routed card is a clean no-op. Behavior-contract tests assert the guard clears (check_respawn_guard is None), the card is spawnable via the real dispatch_once path, the triage counter resets, and a reverted guard-clear leaves active_pr and fails. (cherry picked from commit eb026be)
Why
The OUTER feedback loop — a human's feedback on an accepted card, routed back to the author for a revision — had no code path that cleared the
active_prrespawn guard, so it wedged every time and required a hand-run block→unblock→reassign dance.Root cause (
hermes_cli/kanban_db.py):auto_route_review_bounce, but it fires ONLY for thereview-changes-requestedreason. A human-feedback bounce is a different reason, so it never fires.move_cardblocked/acceptance → ready emits onlystatus_changed, never theunblockedcutoff eventcheck_respawn_guardhonors. So the guard staysactive_pr(the PR is genuinely open) and the dispatcher refuses to spawn the author every tick (respawn_guarded {active_pr}).block_recurrencespastBLOCK_RECURRENCE_LIMIT→ the loop breaker escalates the card totriage, where a naive block→unblock cutoff silently no-ops.What
Adds
route_feedback_to_author, a sanctioned caller-driven primitive that performs the whole outer-loop bounce atomically, composed from existing sanctioned building blocks so the INNER loop, the reviewer PASS → acceptance path (accept_task), and the merge → done path are all left unchanged:reset_block_recurrences— frees atriagecard and prevents immediate re-escalation on the next same-cause block;assigned), fenced on the card still being in a transitionable lane (blocked/scheduled/triage);unblock_task— emits theunblockedcutoff event NOW, causally AFTER the PR-URL handoff comment, which becomes thepr_cutoffcheck_respawn_guardhonors so theactive_pr(andrecent_success) guard clears and the nextdispatchspawns the author;recompute_ready+ an[audit]comment naming the PR + feedback.Recovers a card from
blocked, acceptance (blocked+ acceptance owner), ANDtriage. Idempotent: once routed off the parked lane, a second call is a clean no-op (returnsFalse, no duplicate audit comment). A blank author raises rather than guessing; a missing card returnsFalse.The triage-transition half of
unblock_taskis already live (it acceptsblocked/scheduled/triage), so this change is scoped to the outer-loop auto-clear primitive only — it does not re-implement the triage transition.Testing
New behavior-contract tests (
tests/hermes_cli/test_kanban_route_feedback_to_author.py, 9 tests):check_respawn_guard(...) is Noneafter the route);dispatch_oncespawn path (not a live spawn);triagewith an inflatedblock_recurrences(counter reset to 0);blockedlane;False;auto_route_review_bouncepath still works.RED check: reverting the
unblock_taskcutoff step leaves the guardactive_prand the guard-clear tests fail — proving the cutoff emission is the load-bearing behavior.Full
tests/hermes_cli/test_kanban_db.py(280) and the surrounding kanban lifecycle suites pass with zero regressions. (One pre-existing macOS-only test,test_signal_handler_kanban_worker.py, depends on Linux/procfor PID liveness and cannot pass on Darwin; it is byte-identical to base and unrelated to this change.)