🐛 fix(gateway): let the agent:end hook block or rewrite a reply - #114
Merged
Conversation
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.
cwest
commented
Aug 4, 2026
cwest
left a comment
cwest
left a comment
Owner
Author
There was a problem hiding this comment.
The mechanism is right and it mirrors the command:* decision protocol already in this file rather than inventing a new one. emit_collect gives per-handler exception isolation, the call-site try/except is a second layer, and response_full is added without disturbing the capped response field, so record-only consumers are untouched. I ran the touched-module suite (test_agent_end_hook_decisions.py + test_hooks.py, 40 passed) and the command-dispatch suite (16 passed) against the head commit, plus ruff on the changed files, all clean. The deny/rewrite/no-op and wedge-safety paths are all covered, including the planted-positive backtest through a real emit_collect.
Two checks are red for reasons that are not this change: the test_models.py failures in slice 2/8 reproduce identically on main HEAD (the OpenRouter catalog no longer carries the model ids those fixtures assert), and check-attribution is asking for casey@geeknest.com to be added to the contributor allowlist — a mirror/allowlist condition, not an authorship problem with this commit, which is signed and verified. main is not branch-protected, so neither gates the merge, but both are worth clearing separately so the aggregate reads green.
One thing to keep in mind when the live handler is later flipped to return decisions: on deny the handler's message becomes the reply that is delivered and stored as the assistant turn (same as the command:* deny path this mirrors) rather than being fed back to the model for a fresh turn. That is the correct shape for this protocol, but it means the deny message itself is what the recipient sees — so the handler should return text meant for a human, or use rewrite with a corrected reply. The generic fallback string only fires on a deny with no message. No change needed here; noting it for the handler work.
| if isinstance(message, str) and message.strip(): | ||
| return message, "deny" | ||
| return ( | ||
| "That reply was blocked by a policy hook. Revise it and try again.", |
Owner
Author
There was a problem hiding this comment.
On deny with no handler message, this fallback string is delivered to the recipient as the reply (and stored as the assistant turn), since the call site substitutes it into response. That matches the command:* deny path this mirrors, so it is fine here — just flagging that when the live handler is armed, a real deny should carry a message written for a human, since the deny text is what gets shown, not a prompt fed back to the model.
cwest
marked this pull request as ready for review
August 4, 2026 14:18
cwest
added a commit
that referenced
this pull request
Aug 16, 2026
… cadence (#122) * 🐛 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). * ✨ feat(agent): re-inject a short voice contract mid-session on a turn cadence A voice/style rule composed into the system prompt is cached once per session and reused byte-for-byte every turn, so a long conversation's early voice contract decays as context grows — nothing re-asserts it. Add an opt-in layer that periodically re-injects a SHORT, hard-capped reminder by riding the current user message's api_content sidecar, the one channel that costs at most the current turn's cache boundary and never the cached system prefix. - format_voice_contract_reminder / should_inject_voice_contract: pure helpers (bounding + cadence) in agent/turn_context.py. The rendered block is hard-capped at MAX_VOICE_CONTRACT_CHARS=600 with head-kept truncation, mirroring the TodoStore bounding precedent; the cadence fires on positive multiples of the interval only — never turn 1, never every turn. - compose_user_api_content gains a voice_contract param, appended as a third injection source beside memory prefetch and pre_llm_call plugin context, so the sidecar==wire drift invariant already guaranteed there carries the exact bytes. - config.yaml agent.voice_contract.{interval,text}; default OFF (interval 0) since it changes prompt bytes for every fired turn. Composition-path over a pre_llm_call plugin: a first-class config-driven persona feature whose turn counter already lives on the agent, and the sanctioned compose seam is what guarantees byte-stability. Behavior contracts assert the invariants, not snapshots: the composed system prompt is byte-identical across a fired re-injection, no synthetic user message is inserted (role alternation holds), the persisted sidecar equals the bytes sent, the reminder appears only at the configured cadence, and the payload never exceeds the cap even with an oversized configured contract. E2E through the real prologue against a temp home.
This was referenced Aug 17, 2026
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
The
agent:endhook could observe an outgoing reply but never stop it, and itonly ever saw the first 500 characters of the reply. Both were verified against
live behavior:
Defect 1 —
agent:endcannot refuse. The hook fired viaemit(), which perits own docstring discards handler return values.
emit_collect()(the variantthat returns handler decisions) was wired only to
command:*events. So ahandler on
agent:endcould record a violation but never block the reply.Defect 2 — the handler saw only 500 characters. The context passed
response = (response or "")[:500], so a policy trigger buried in the middle ofa long reply was structurally invisible.
What
Mirror the proven
command:*decision protocol already in this file — no newmechanism was invented.
response_full(untruncated) to theagent:endcontext, alongside theexisting
responsefield, which stays capped at 500 chars so existingconsumers are unaffected.
agent:endviaemit_collect()so handler decisions are honored.decision == "deny"suppresses the reply and surfaces the handler'smessageback into the loop so the agent must revise;
decision == "rewrite"substitutes the handler's
response; anything else,None, or a non-dict isa no-op, so record-only handlers keep working exactly as before.
garbage falls through to sending the reply unchanged — a broken predicate
can never silence the agent. This is the highest-risk path and is covered by
tests.
The decision handling and context building are extracted into two small
module-level helpers (
_apply_agent_end_hook_decisions,_agent_end_hook_context) so every path is unit-tested.Tests (TDD, RED first)
New
tests/gateway/test_agent_end_hook_decisions.py:denysuppresses the original reply and surfaces the handler message.rewritesubstitutes the reply; a rewrite with no replacement text is a no-op(never blanks the reply).
response_fullcarries the untruncated reply whileresponsestays capped at500.
None/garbage/non-dict still lets the reply through.response_fulland MISSED via the truncatedresponse, dispatched through areal
HookRegistry.emit_collect.Verification (in the worktree, against the project venv):
pytest tests/gateway/test_agent_end_hook_decisions.py tests/gateway/test_hooks.py— 40 passed.pytest tests/gateway/test_gateway_command_dispatch_minimal.py tests/gateway/test_unknown_command.py— 16 passed (command:* path unaffected).ruff checkon touched files — clean.ty checkon touched files — clean;gateway/run.pydiagnostics unchanged (172 → 172, all pre-existing).RESTART GATE
gateway/run.pyand the hook tree load once at gateway startup and do NOThot-reload. This change is not live on merge — it requires a gateway restart,
which only Casey can perform. Do not treat merge as deployment.
Scope
Out of scope (intentionally not touched): the artifact-handle gate's detection
logic (already correct), and the record-only-to-decision flip of the live
~/.hermes/hooks/artifact-handle-watch/handler.py, which lives in a separateedit-in-place repo (
cwest/hermes-config) and is handled outside this PR.NOT merged.