🐛 fix(kanban): stop active_pr guard re-arming on the recovery's own cutoff comment - #77
Conversation
cwest
left a comment
There was a problem hiding this comment.
The mechanics here are right and the tests are clean, but the fix misses the recovery path the PR is written to fix.
The outer-loop recovery primitive route_feedback_to_author writes its PR-URL audit comment with author="orchestrator" (kanban_db.py:4059) — not dispatcher, hollis, or onecard. add_comment stores that string verbatim, no canonicalization. So the carve-out set ("dispatcher", "hollis", "onecard") does not match the author that actually lands the same-second PR-URL comment on the wedged card.
Verified against the live card t_11132357: the PR-URL comment at ts 1784754011 is authored by orchestrator. Reproduced against this PR's head code in a temp HERMES_HOME — an orchestrator-authored PR-URL comment at the unblock cutoff still returns active_pr:
author=orchestrator, same-second cutoff -> check_respawn_guard: active_pr
That is the exact scenario the "Done when" requires to clear to None. The three new tests pass only because they use hollis / onecard-orchestrator as the comment author, which aren't the author the real recovery emits — so the suite is green while the wedge the card describes stays wedged.
Add "orchestrator" to _RESPAWN_GUARD_RECOVERY_AUTHOR_PREFIXES, and add a test whose PR-URL comment is authored by orchestrator at the same-second cutoff (mirroring what route_feedback_to_author at line 4059 produces) so the contract is pinned to the real emitter. Better still, drive the RED test through route_feedback_to_author itself rather than hand-inserting a comment, so the test can't drift from the actual author the recovery writes.
Everything else checks out: the SQL splice is correct, dispatcher% correctly subsumes the old != 'dispatcher' carve-out, the genuine dup-PR case still guards, and closed/merged fail-open is untouched. CI note: 8 test slices were still in progress at review time — those need to conclude green too.
…utoff comment The active_pr respawn guard wedged a ready card that legitimately reuses an open PR for a revision round (the outer feedback loop). After a correct block -> route_feedback_to_author -> unblock (which emits the unblocked cutoff event), check_respawn_guard still returned active_pr every tick, so the dispatcher refused to spawn the author for hours (live 2026-07-22, PR NousResearch#143). Root cause: the dup-PR scan clears only for PR-URL comments strictly before pr_cutoff = max(window, latest_unblock_ts), but the sanctioned recovery writes its own audit comment naming the existing PR URL at the same second as the unblocked event it emits. Kanban timestamps are second-granular, so that comment lands at created_at == pr_cutoff and the >= window scan re-counts it as a fresh active PR -- the recovery's own cutoff-marking comment re-arms the guard it is trying to clear. The prior carve-out excluded only the literal 'dispatcher' author; it did not cover the orchestrator/recovery actors that drive the outer loop -- crucially route_feedback_to_author, which stamps its PR-URL audit comment with author='orchestrator'. Fix: _RESPAWN_GUARD_RECOVERY_AUTHOR_PREFIXES now covers (dispatcher, hollis, onecard, orchestrator) and excludes any comment whose author prefix-matches that set from the dup-PR scan (author NOT LIKE '<name>%', covering profile-suffixed variants). 'orchestrator' is the author the real outer-loop primitive emits, so a same-second reopen audit no longer re-trips the guard. None of these actors ever opens a PR, so this is analogous to the review-status carve-out. A genuine builder/author PR-URL comment at/after the cutoff is not in the set and still re-arms active_pr, so duplicate-PR protection is preserved; the review-status skip and closed/merged fail-open behavior are untouched. Single-file change plus behavior-contract regression tests, including one driven through route_feedback_to_author itself so the test cannot drift from the real recovery emitter. Full test_kanban_db.py and the kanban surface green, 0 regressions.
cwest
left a comment
There was a problem hiding this comment.
The rework closes the gap from the last round. The carve-out now covers the author the recovery actually stamps: route_feedback_to_author writes its PR-URL audit comment as author="orchestrator" (kanban_db.py:4059), and orchestrator is now in _RESPAWN_GUARD_RECOVERY_AUTHOR_PREFIXES, so the same-second reopen audit at the unblock cutoff no longer re-arms active_pr. The prefix-LIKE scan and param order are correct, and the old author != 'dispatcher' filter is fully subsumed by dispatcher%.
The added test drives route_feedback_to_author itself rather than hand-inserting the comment, so it stays pinned to the real emitter and can't drift. I confirmed the teeth: reverting the constant to the pre-fix dispatcher-only value fails all three "does not re-arm" tests (including the driven-through one) with active_pr, while the non-recovery author case keeps guarding — so the duplicate-PR protection is intact. With the fix in place the full test_kanban_db.py runs 280 passed, 0 failed. The review-status skip and the closed/merged fail-open path are untouched.
No changes needed.
…utoff comment (#77) The active_pr respawn guard wedged a ready card that legitimately reuses an open PR for a revision round (the outer feedback loop). After a correct block -> route_feedback_to_author -> unblock (which emits the unblocked cutoff event), check_respawn_guard still returned active_pr every tick, so the dispatcher refused to spawn the author for hours (live 2026-07-22, PR NousResearch#143). Root cause: the dup-PR scan clears only for PR-URL comments strictly before pr_cutoff = max(window, latest_unblock_ts), but the sanctioned recovery writes its own audit comment naming the existing PR URL at the same second as the unblocked event it emits. Kanban timestamps are second-granular, so that comment lands at created_at == pr_cutoff and the >= window scan re-counts it as a fresh active PR -- the recovery's own cutoff-marking comment re-arms the guard it is trying to clear. The prior carve-out excluded only the literal 'dispatcher' author; it did not cover the orchestrator/recovery actors that drive the outer loop -- crucially route_feedback_to_author, which stamps its PR-URL audit comment with author='orchestrator'. Fix: _RESPAWN_GUARD_RECOVERY_AUTHOR_PREFIXES now covers (dispatcher, hollis, onecard, orchestrator) and excludes any comment whose author prefix-matches that set from the dup-PR scan (author NOT LIKE '<name>%', covering profile-suffixed variants). 'orchestrator' is the author the real outer-loop primitive emits, so a same-second reopen audit no longer re-trips the guard. None of these actors ever opens a PR, so this is analogous to the review-status carve-out. A genuine builder/author PR-URL comment at/after the cutoff is not in the set and still re-arms active_pr, so duplicate-PR protection is preserved; the review-status skip and closed/merged fail-open behavior are untouched. Single-file change plus behavior-contract regression tests, including one driven through route_feedback_to_author itself so the test cannot drift from the real recovery emitter. Full test_kanban_db.py and the kanban surface green, 0 regressions. (cherry picked from commit 48c07af)
Why
The
active_prrespawn guard wedges areadycard that legitimately reuses an open PR for a revision round — the outer feedback loop, common on writing cards where a human sends the accepted piece back to the author for a revision. The sanctioned recovery could not clear it.Live symptom 2026-07-22 (card
t_11132357, PR NousResearch#143): after a correctblock→route_feedback_to_author→unblock(which emits theunblockedcutoff event),check_respawn_guardstill returnedactive_prevery tick, so the dispatcher refused to spawn the author for hours.Root cause
check_respawn_guard'sactive_prbranch clears only for PR-URL comments strictly beforepr_cutoff = max(window, latest_unblock_ts). But the sanctioned recovery writes its own audit/steer comment that names the existing PR URL at the same second as theunblockedevent it emits (verified live: latestunblockedts == latestpull/143comment ts). Kanban timestamps are second-granular, so that recovery comment lands atcreated_at == pr_cutoffand the>=window scan re-counts it as a fresh active PR — the recovery's own cutoff-marking comment re-arms the guard it is trying to clear. No operator move can win: any comment the recovery writes must reference the PR.The prior carve-out excluded only the literal
dispatcherauthor (its auto-route audit is the same class). It did not cover the orchestrator/recovery actors that drive the outer loop.What
_RESPAWN_GUARD_RECOVERY_AUTHOR_PREFIXES = ("dispatcher", "hollis", "onecard").author NOT LIKE '<name>%', so profile-suffixed variants likedispatcher-defaultare covered).None of these actors ever opens a PR, so excluding their at/after-cutoff PR-URL audit is exactly analogous to the existing review-status carve-out. This does not weaken genuine duplicate-PR protection: a real builder/author PR-URL comment (e.g.
eckert) at/after the cutoff is not in the set and still re-armsactive_pr. Thereview-status skip and the closed/merged fail-open behavior are untouched.Testing
RED-first, then GREEN. 3 new behavior-contract regression tests in
tests/hermes_cli/test_kanban_db.py:hollis) PR-URL audit at the same-second cutoff → guard clears toNone(wasactive_pr);onecard-orchestrator) → clears;eckert) PR-URL comment at/after the cutoff → stillactive_pr(dup-PR protection preserved).Real
kanban_db, tempHERMES_HOME, no mocks. Fulltest_kanban_db.py+ the whole kanban surface (40 files, 899 tests) green, 0 regressions, ruff clean.Single file changed:
hermes_cli/kanban_db.py(+ its tests + aPATCHES.mdrow). Base:cwest/integration.