✨ feat(kanban): give workers a sanctioned running->review handoff verb - #124
Merged
Conversation
A worker that finishes its lane had no sanctioned way to move its own card to review. kanban_complete is wrong at a lane boundary — it means the work item is finished (done == merged/accepted), which is premature — so a completed rework would park in blocked until an orchestrator hand-moved it. Add a non-terminal handoff that MOVEs the card running/ready -> review and assigns the reviewer from the card's state_owners owner map: - kanban_db.submit_for_review(): atomic guarded UPDATE (status IN running/ready), clears the claim lock so the review dispatch's claim_review_task can pick it up, ends the worker run with a non-terminal handed_off outcome, and emits status_changed/assigned events. Its only status target is a literal 'review' — there is no code path to done, and it never touches the PR (no undraft, no merge). - kanban_db.resolve_review_owner(): reads state_owners["review"] from the card's audit trail (code -> lamport, writing -> perkins), falling back to the code reviewer for un-stamped cards. - kanban_submit_for_review worker tool + the `hermes kanban review` CLI verb, both resolving the reviewer from the owner map with an optional explicit override. The review-lane dispatch, acceptance gate, and PR webhook are unchanged; the dispatcher already spawns the review agent for status='review' cards, so a handed-off card flows straight into review with no human.
cwest
commented
Aug 17, 2026
cwest
left a comment
cwest
left a comment
Owner
Author
There was a problem hiding this comment.
No changes needed. The handoff does exactly what the card asks and structurally cannot do the things it must not.
The negative contract is enforced in the SQL, not the docstring: submit_for_review's only SET target is a literal 'review', the guard is WHERE status IN ('running','ready'), and claim_lock/claim_expires/worker_pid are cleared in the same atomic UPDATE. A terminal card matches zero rows and is left alone, so no TOCTOU race can drag a done card back. There is no code path to done, undraft, or merge, and this layer touches no GitHub surface at all.
The review-lane path is unchanged: claim_review_task still keys on status='review' AND claim_lock IS NULL, which is precisely the state the handoff leaves the card in. I ran the three changed test files against the PR head in a throwaway clone: 418 passed, 0 failed, including the E2E case that hands off and then reclaims via claim_review_task and asserts the card goes running again.
On CI: the two red Python slices are not this change. Slice 2 is test_models.py asserting a live OpenRouter catalog entry (qwen/qwen3.7-max) that the fetch now filters out — a pre-existing catalog change-detector, reproduces identically on the PR tree, and this diff touches no model code. Slice 8 and the OSV scan are setup-uv / download-artifact 429/503 infra flakes, not test failures. check-attribution is a repo-wide branch scan flagging an unmapped email on other branches; this PR's single commit is authored by the mapped team identity. Two small notes inline, neither blocking.
cwest
marked this pull request as ready for review
August 17, 2026 14:53
…rce-read The submit_for_review negative-control test asserted on inspect.getsource() to prove the string 'done' never appears — a source-text assertion that false-fails on a harmless rename/comment and false-passes if a path to 'done' is reached via a helper. Replace it with a behavioral guard: call submit_for_review on a card in every non-handoffable status (done, review, blocked, triage, todo, scheduled, archived) and assert the call returns False with status AND assignee unchanged, plus a positive half asserting the only produced status is 'review'. Verified as a real guard by mutation: widening the SQL WHERE to admit 'done' makes the [done] case fail. Also document the first-match (not last-write) owner-map resolution in resolve_review_owner: the map is stamped once at submit and not re-negotiated per lane, so the earliest parseable map is authoritative.
cwest
commented
Aug 17, 2026
cwest
left a comment
cwest
left a comment
Owner
Author
There was a problem hiding this comment.
The rework lands both requested changes cleanly, and the negative-control test is now real coverage rather than a source-text grep.
The handoff guard's negative contract is behavioral: the parametrized test drives submit_for_review against every non-handoffable status (done, review, blocked, triage, todo, scheduled, archived) and asserts it returns False with status and assignee both unchanged, alongside a positive half confirming the only status it can produce is review. I verified this is a genuine guard and not a vacuous test by mutation — widening the SQL WHERE to admit done makes exactly the [done] case fail (the verb drags a settled card back to review), so the test would catch a done-reaching regression. The guard itself is right where the contract belongs: SET status = 'review' is a literal, WHERE id = ? AND status IN ('running','ready') forbids any terminal or already-moved card from matching, and the claim lock is cleared atomically in the same UPDATE. There is no code path to done, and nothing in this layer touches the PR.
The first-match note on resolve_review_owner reads well and correctly documents why the earliest parseable owner map wins, with a pointer to switch to newest-first if a re-stamp flow is ever added.
Changed-file tests pass on this head (425 in the kanban suite, including the new parametrized cases). The two red checks are not from this change: the model-catalog tests in slice 2 reproduce identically on main and this diff touches no model code, and the label gate clears once this review lands.
No changes needed.
… test The negative control for submit_for_review asserted that a settled card in any non-handoffable status cannot be dragged to review, but did not pin down *why* the call is refused. Make the fixture self-evidently a settled card: assert claim_lock/claim_expires/worker_pid/current_run_id are all NULL before the call, so the SQL status clause is provably the only thing standing between the call and a successful write. Now a mutation that widens the guard to admit done/blocked/review turns exactly those parametrizations RED — the control measures the status guard, not some incidental precondition. Verified: plant the widened WHERE -> [done]/[review]/[blocked] + terminal-card case go RED; revert -> all green. 425 changed-file tests pass; ruff clean.
cwest
added a commit
that referenced
this pull request
Aug 17, 2026
… work reaches the running gateway (#126) * 🐛 fix(gateway): let the agent:end hook block or rewrite a reply (#114) The agent:end hook fired via emit(), which discards handler return values, and passed only the first 500 chars of the reply. A handler could record a violation but never stop the reply, and a violation buried past char 500 was structurally invisible. Mirror the proven command:* decision protocol: - Add response_full to the agent:end context (untruncated) alongside the existing response field (kept capped at 500 for backward compatibility). - Dispatch agent:end via emit_collect() so handler decisions are honored. - decision=deny suppresses the reply and surfaces the handler message back into the loop; decision=rewrite substitutes the reply; anything else, None, or a non-dict is a no-op so record-only handlers are unaffected. - Wrap the whole dispatch so a handler that raises, times out, or returns garbage falls through to sending the reply unchanged. A broken predicate can never silence the agent. Decision handling and context building are extracted into two module-level helpers so the deny/rewrite/no-op/wedge-safety paths are unit-tested. * 🐛 fix(kanban): reclaim a wedged worker in minutes and release a claim on lane-exit (#119) A worker's claim could gate the next lane for up to a full hour, starving the review column: the author had pushed and the card had already been MOVED out of its lane, yet the author's claim still blocked the next worker from spawning. Two independent defects, two fixes. 1. Heartbeat-staleness threshold was 60m of pure slack. release_stale_claims reclaims a live-PID worker whose last_heartbeat_at is older than DEFAULT_CLAIM_HEARTBEAT_MAX_STALE_SECONDS, but at 60m a worker that stopped heartbeating 14 minutes ago was still "fresh", so its claim was extended instead of reclaimed. The 60m was legacy slack from before the chunk-level activity bridge: _touch_activity now refreshes last_heartbeat_at at the start of every API call and on every stream delta (rate-limited to 60s), so a genuinely active worker — including one inside a single long tool-free LLM call — is never more than ~60s stale from ordinary traffic. Lower the threshold to 5 min (5x the bridge cadence): a wedged worker is reclaimed in minutes, while a healthy-but-slow worker's fresh heartbeat still extends its claim. 2. A claim leaked onto a non-running lane was never released. Every legitimate claim path sets claim_lock in the same transaction as status='running', so a claim on a card that is NOT running can only be one that leaked across a lane transition (e.g. a running->review MOVE that updated status/assignee but left the prior worker's claim_lock/expires/worker_pid on the row). The review-spawn path requires claim_lock IS NULL, so that dangling claim starved the lane, and neither the TTL scan nor the crashed-worker scan could see it (both scan status='running' only). release_stale_claims now clears any claim on a non-running card, in its current lane, with no TTL wait and no PID check — restoring the invariant "claim state belongs only to running cards" that the dashboard status-set path already enforces. Tests: reproduce the exact 14-minute wedge (reclaimed, not extended); a negative control proving a fresh-heartbeat expired-TTL worker is still extended (guards the spawn-then-reclaim regression); and the lane-exit leak (a review card carrying a departed worker's claim is freed so the review-spawn predicate matches). * fix(kanban): derive worktree branch names from the card title Worktree branches for cards without a project link fell through to a bare wt/<task-id>, producing opaque refs like t-39521e0e that are unreadable in a branch list or preview dashboard. Meaningful naming already existed but was gated behind a project link, so the opaque form was the default for most cards rather than a rare fallback. Derive the name from the card title on every path, matching the slug rules already used for project-linked cards. The bare id now appears only when a title is absent or slugs away to nothing. Both worktree provisioning call sites are covered; a single-site fix would have left the second path emitting the old shape. * fix(kanban): route a review handoff to the reviewer, not back to the author A worker signalling dependency_wait to hand off finished work was parked in todo, which the recompute_ready sweep promoted to ready, which made the dispatcher respawn the author on already-complete work. One card cycled through that loop three times before an operator broke it by hand. Dependency waits whose reason names a review or signoff now land in review. * fix(kanban): keep an acceptance park sticky so the sweep cannot unpark it recompute_ready promotes any blocked card whose parents are all done, which is vacuously true for a parentless card. _has_sticky_block was the guard against that, but it only treated onecard:move_card as a decisive park — acceptance emits onecard:accept_card, so an accepted card was unparked seconds after the PASS and the reviewer re-parked it in a loop. Recognize both one-card verbs as decisive. * Revert "🐛 fix(gateway): let the agent:end hook block or rewrite a reply (#114)" This reverts commit 0136ddc032877cb0b8e1afcefee844b3d18c0f0e. * ✨ feat(kanban): give workers a sanctioned running->review handoff verb (#124) * ✨ feat(kanban): give workers a sanctioned running->review handoff verb A worker that finishes its lane had no sanctioned way to move its own card to review. kanban_complete is wrong at a lane boundary — it means the work item is finished (done == merged/accepted), which is premature — so a completed rework would park in blocked until an orchestrator hand-moved it. Add a non-terminal handoff that MOVEs the card running/ready -> review and assigns the reviewer from the card's state_owners owner map: - kanban_db.submit_for_review(): atomic guarded UPDATE (status IN running/ready), clears the claim lock so the review dispatch's claim_review_task can pick it up, ends the worker run with a non-terminal handed_off outcome, and emits status_changed/assigned events. Its only status target is a literal 'review' — there is no code path to done, and it never touches the PR (no undraft, no merge). - kanban_db.resolve_review_owner(): reads state_owners["review"] from the card's audit trail (code -> lamport, writing -> perkins), falling back to the code reviewer for un-stamped cards. - kanban_submit_for_review worker tool + the `hermes kanban review` CLI verb, both resolving the reviewer from the owner map with an optional explicit override. The review-lane dispatch, acceptance gate, and PR webhook are unchanged; the dispatcher already spawns the review agent for status='review' cards, so a handed-off card flows straight into review with no human. * 🧪 test(kanban): make the handoff negative control behavioral, not source-read The submit_for_review negative-control test asserted on inspect.getsource() to prove the string 'done' never appears — a source-text assertion that false-fails on a harmless rename/comment and false-passes if a path to 'done' is reached via a helper. Replace it with a behavioral guard: call submit_for_review on a card in every non-handoffable status (done, review, blocked, triage, todo, scheduled, archived) and assert the call returns False with status AND assignee unchanged, plus a positive half asserting the only produced status is 'review'. Verified as a real guard by mutation: widening the SQL WHERE to admit 'done' makes the [done] case fail. Also document the first-match (not last-write) owner-map resolution in resolve_review_owner: the map is stamped once at submit and not re-negotiated per lane, so the earliest parseable map is authoritative. * 🧪 test(kanban): prove the handoff status guard is the sole gate under test The negative control for submit_for_review asserted that a settled card in any non-handoffable status cannot be dragged to review, but did not pin down *why* the call is refused. Make the fixture self-evidently a settled card: assert claim_lock/claim_expires/worker_pid/current_run_id are all NULL before the call, so the SQL status clause is provably the only thing standing between the call and a successful write. Now a mutation that widens the guard to admit done/blocked/review turns exactly those parametrizations RED — the control measures the status guard, not some incidental precondition. Verified: plant the widened WHERE -> [done]/[review]/[blocked] + terminal-card case go RED; revert -> all green. 425 changed-file tests pass; ruff clean. * revert(gateway): re-hold the agent:end hook decision path after merge The merge of origin/main re-introduced the agent:end block/rewrite hook change, which was deliberately held out of the running install by a signed revert (eccbae1, 2026-08-16). Re-apply that hold so merging upstream does not silently resurrect code that was intentionally kept out of the running gateway. This reverts the agent:end decision-path change on this branch only; it does not affect the same change on origin/main. Net effect on the running install: unchanged (the hook stays record-only), while the rest of merged main — the running->review handoff verb and the worker reclaim fix — lands normally. Reverts the content of b7514ce; tree-identical to the prior hold. * fix(kanban): complete owner-map reader reconciliation after merge The merge of origin/main brought in a review-owner resolver that assumed no owner map is stamped until submit. This fork stamps a kind_source=defaulted owner map at the create_task chokepoint (the owner-map birth guarantee), so the incoming naive first-match scan read that defaulted stamp and (a) ignored a later intentional stamp and (b) never honored the caller's default for a map-less card. Reconcile to the fork's existing, auto-stamp-aware reader: - resolve_review_owner now delegates to _review_owner_from_owner_map, so an intentional submit stamp (or prose Routing (owner map): {…}) wins over the defaulted chokepoint stamp, matching every other owner-map reader in this module. Removed the orphaned naive _parse_owner_map the merge introduced (its only caller was the old resolver; the fork's _lane_owner_from_map_body already does the parse). - Renamed the incoming test helper _stamp_owner_map -> _stamp_owner_map_str to stop it colliding with the fork's kwargs-based _stamp_owner_map of the same name (Python kept only the last def, breaking the incoming positional callers with a TypeError). - Updated the fallback test to the fork's contract: a plain code card resolves to its defaulted-code reviewer; the caller default applies only to a genuinely map-less card. - Corrected three stale wt/<id> branch-name assertions to the title-derived wt/<id>-<slug> form the fork already ships. Also documents the merge-based reconciliation procedure in docs/reconciling-fork-with-upstream-main.md so the next release is mechanical. * test(kanban): fix last stale wt/<id> worktree branch assertion The merge-reconciliation commit corrected three stale wt/<id> assertions in test_kanban_db.py but missed a fourth, in a file the merge diff did not otherwise touch. _resolve_worktree_workspace now derives the fallback branch name from the card title (wt/<id>-<slug>), so the occupied-path fallback case for a 'second sibling' card yields wt/<id>-second-sibling, not the bare wt/<id>. Update the assertion to match; the same-branch reuse case on line 172 correctly keeps the bare wt/<id> (it returns the actual pre-existing checkout's branch, not a freshly derived name).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
A worker that finishes its lane had no sanctioned way to move its own card to
review.kanban_completeis wrong at a lane boundary — it means the work item is finished (done== merged/accepted), which is premature — so a completed rework parked inblockeduntil an orchestrator hand-moved it.What
A non-terminal handoff that MOVEs the card
running/ready→reviewand assigns the reviewer from the card'sstate_ownersowner map.kanban_db.submit_for_review()— atomic guardedUPDATE ... WHERE status IN ('running','ready'); clears the claim lock so the review dispatch'sclaim_review_taskcan pick it up; ends the worker run with a non-terminalhanded_offoutcome; emitsstatus_changed/assignedevents. Its only status target is a literal'review'— there is no code path todone, and it never touches the PR (no undraft, no merge).kanban_db.resolve_review_owner()— readsstate_owners["review"]from the card's audit trail (code →lamport, writing →perkins), falling back to the code reviewer for un-stamped/legacy cards.kanban_submit_for_reviewworker tool + thehermes kanban reviewCLI verb — both resolve the reviewer from the owner map with an optional explicit override.Done when
A worker hands off to the review lane in one sanctioned call, and a completed rework no longer parks in
blockedwaiting on an orchestrator.Verification
review+lamport(from the owner map), claim cleared, and the dispatcher'sclaim_review_taskthen succeeds → the review agent spawns. No human intervention.done/undraft/merge path.submit_for_review's only SET target is'review'(test asserts'done'never appears in its source and the card isreviewafter a call); the tool schema carries nostatus/merge/undraft/doneparameter; a terminal card is refused.status='review'cards and force-loadssdlc-review; a handed-off card flows straight into that unchanged path.Tests
New coverage in
tests/hermes_cli/test_kanban_db.py,tests/tools/test_kanban_tools.py,tests/hermes_cli/test_kanban_cli.py(RED→GREEN). Full kanban + toolsets + registry surface: 1015 tests pass, 0 failures.ruffclean;pyrightdelta zero (21 pre-existing errors intoolsets.py, identical onmain).