feat(hooks): let a PostToolUse hook withhold a tool's output - #752
Closed
tamirkiviti13 wants to merge 4 commits into
Closed
feat(hooks): let a PostToolUse hook withhold a tool's output#752tamirkiviti13 wants to merge 4 commits into
tamirkiviti13 wants to merge 4 commits into
Conversation
…ion id Two things the Claude Code hooks bridge computed and then discarded. **UserPromptSubmit blocks were ignored.** The hook engine resolves a `blocked` verdict (exit code 1, or a `deny`/`block` stdout control payload) for UserPromptSubmit just as it does for PreToolUse, but the bridge never read the flag: `_collect_context_stdout` skips blocked results, so a blocking prompt hook fell through to the "nothing to add" path and the prompt reached the model unchanged. A hook author writing the documented `exit 1` got a silent no-op — the worst failure mode for something people reach for as a policy control. A block now replaces the prompt with a notice carrying the hook's reason, so the original text never reaches the model. The turn still runs, on the replacement — there is no callback-driven way to cancel one — and the notice tells the model to relay the block and stop. Pending SessionStart context is deliberately left in the buffer on a block: that prompt never ran, so its context still belongs to the next prompt that does. **Every event reported the same session id.** `_build_stdin_payload` reads `session_id` from `EventData.context`, but only UserPromptSubmit and Stop/SubagentStop ever put one there — so tool events fell back to the literal `"codepuppy-session"` and a hook script could not tell one run from another, nor pair a PreToolUse with its PostToolUse. `run_with_mcp` already mints a per-run `group_id`; it just had no way to reach a callback that pydantic-ai invokes deep inside the run. It is now published through a ContextVar (`code_puppy.session_context`), which the bridge reads when building any event's context. ContextVars are copied into a task at `create_task` time and writes stay task-local, so a sub-agent run gets its own id and can never clobber its parent's. The wrapper restores the previous id on the way out. Events fired outside any run, such as SessionStart at boot, keep the existing placeholder. Docs updated: UserPromptSubmit was missing from the event table, and Stop/SubagentStop were listed as blocking when the bridge only observes them.
Builds on the previous commit, which stopped a block from being ignored but could only substitute the prompt — the turn still ran, on replacement text. Claude Code, whose semantics this bridge targets, erases the prompt and processes nothing. This closes that gap. A blocking hook now cancels the turn outright: no agent is built, no LLM call is made, and the reason is surfaced to the user. The signal is a `PromptBlocked` returned from a `user_prompt_submit` callback, which `run_with_mcp` returns `None` for — the shape it already returns for a cancelled run, and one `cli_runner` already guards in three places. A return value rather than an exception because `_trigger_callbacks` catches `Exception` per callback and appends `None`, so a raise never reaches the runtime. Nested runs are the exception, and deliberately so. A plugin making its own internal `run_with_mcp` call dereferences the result — `shell_safety` does `result.output` with no None guard — so cancelling one would break the caller with an AttributeError. Those fall back to substitution. The prompt text is withheld from the model either way. Sub-agents are unaffected: they go through `temp_agent.run()` and never fire UserPromptSubmit at all. Tests cover the top-level cancellation (asserting nothing downstream of the block runs), the nested fallback, and that a plain-string return still replaces the prompt as before.
Two defects found while exercising the hooks end-to-end against a real
agent run, plus a message-quality fix. All three are in the path the
previous commits touch.
**Stop never fired.** ``_SUBAGENT_NAMES`` contained ``code-puppy`` and
``code_puppy`` — the name of the DEFAULT agent — and the classifier
substring-matched the agent name against it. Every top-level turn was
therefore reported as ``SubagentStop`` and a ``Stop`` hook was dead
config. Confirmed live: a plain ``code-puppy -p "..."`` run emitted
``SubagentStop``.
The name was never a sound signal in either direction: a sub-agent can
be called anything, and the default agent collides with the list.
Classification now uses ``subagent_context.is_subagent()`` — the depth
ContextVar that actually tracks nesting — and keeps the name list only
as a fallback for a run ended outside the context manager, with the two
default-agent names removed. The existing test asserting ``code-puppy``
implies ``SubagentStop`` encoded the bug and is updated.
**The end-of-turn payload had no response.** ``agent_run_end`` receives
``response_text``, and it reached ``EventData.context``, but
``_build_stdin_payload`` only promotes ``result`` and ``duration_ms`` —
so a Stop hook fired with ``tool_input: {}`` and nothing to inspect.
Claude Code hands end-of-turn hooks a ``transcript_path``; code puppy
has no transcript file, so the response itself is the equivalent, and
without it end-of-turn review is impossible.
**Block reasons leaked internals.** ``blocking_reason`` is a diagnostic
string — ``Hook '<full command line>' failed: <stderr>`` — and it was
what the user saw on a blocked prompt and what the model saw on a
blocked tool call. It exposes the hook's path and says "failed" for a
hook that deliberately blocked. Both now prefer the blocking hook's own
stderr, falling back to the diagnostic form when there is none.
PostToolUse was purely observational: `on_post_tool_call` fired from the
`finally` of the `_call_tool` wrapper, after `return result` had already
decided the value, and its return was discarded. A hook could see a
secret in a tool's output and had no way to keep it out of the model.
It now runs on the success path, before the result is handed back, and a
`{"blocked": True, "reason": ...}` verdict substitutes a notice naming
the reason. The mechanism is the one already used a few lines above to
prepend PreToolUse stdout to a tool result, so the path is well-trodden.
Scope is deliberately narrow and the docs say so: the tool has ALREADY
run and its side effects have happened. This governs what reaches the
model and the message history — which is what keeps secrets in tool
output out of the transcript and out of the provider's logs. `PreToolUse`
remains the way to stop a call from happening.
The `finally` still fires on the exception path, where there is no result
to withhold and the notification stays observational; a flag stops a
successful call from notifying twice.
Additive for existing consumers. All eight `post_tool_call` registrants
(run stats, subagent panel, ACP bridge, heartbeat, frontend emitter,
quick-resume, herdr, the hooks bridge) are observational and return
nothing, so none of them change behaviour.
Verified end-to-end against a live agent: `cat` a file containing a
marker, hook blocks, and the agent reports it cannot see the contents —
where previously it echoed the secret straight back.
Author
|
Folding this into #747 rather than keeping it separate. I originally split it out on the grounds that withholding tool output is a capability Claude Code doesn't have, so it deserved its own debate. That was the wrong yardstick. Code puppy's own #747 now carries all four commits and has been retitled and rewritten around that. Nothing is dropped. |
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.
PostToolUsecurrently cannot affect anything.on_post_tool_callfires from thefinallyof the_call_toolwrapper — afterreturn resulthas already decided the value — and its return is discarded. A hook can see a secret in a tool's output and has no way to keep it out of the model.This makes a
{"blocked": True, "reason": ...}verdict from apost_tool_callcallback substitute a notice for the result.What this does and does not do
The tool has already run by the time
PostToolUsefires, and this does not change that. Side effects have happened — the file was read, the command executed. What the verdict controls is where the output goes: it stays out of the model's context, out of the message history, and out of your provider's logs.PreToolUseremains the way to stop a call from happening at all.That distinction is stated in the docs rather than left for someone to discover.
Implementation
The callback moves from the
finallyonto the success path, before the result is handed back. The mechanism is the one already used a few lines above to prependPreToolUsestdout to a tool result, so this isn't new plumbing — just the same rewrite point, with a verdict attached.The
finallystill fires on the exception path, where there is no result to withhold and the notification stays observational. A flag prevents a successful call from notifying twice.Additive for existing consumers. All eight
post_tool_callregistrants — run stats, subagent panel, ACP bridge, timestamp heartbeat, frontend emitter, quick-resume, herdr, and the hooks bridge — are observational and return nothing, so none of them change behaviour._run_post_tool_callnever raises: a hook that throws leaves the result untouched.Honest framing
Unlike the other hook work I've sent, this is not a Claude Code parity fix. Claude Code's
PostToolUsecannot withhold a result either — exit 2 there feeds stderr back to the model alongside the result. This is a capability request, so it's a separate PR rather than bundled with the compat fixes, and easy to decline on its own merits.Stacking
This sits on top of the three-commit hooks-blocking branch (
fix/user-prompt-submit-block-and-session-id), because it reuses the_block_reasonhelper introduced there. Until that lands, this PR's diff shows those commits too; the commit unique to this PR is the last one. Both branches are rebased onto currentmain.Testing
Six new tests: the bridge verdict, the runtime substitution, pass-through when allowed, and a hook that throws.
Full suite on current
main+ this branch: 7124 passed, 8 skipped.ruff checkclean;ruff format --checkflags only two files this branch doesn't touch (agent_creator_agent.pyand its test, already unformatted onmain).Verified end-to-end against a live agent and a real model, on 0.0.702:
cata file containing a marker → hook blocks → the agent replies "I can't see what's in that file", where before this change it echoed the secret straight back.Stoppayload confirms the secret is absent from the agent's final response, so it never entered the transcript either.PostToolUsestill receives the realtool_result, which is what makes it scannable in the first place — only what flows onward is replaced.The withheld output is still printed in the operator's own terminal by code puppy's shell display, which runs before the hook. That is expected: this protects the model context, not the local screen.