From f3757e1ad34d8503bb76c5e74ed31ad362213e59 Mon Sep 17 00:00:00 2001 From: David Gageot Date: Wed, 9 Sep 2026 15:23:18 +0200 Subject: [PATCH 1/4] feat(hooks): shared events catalog and centralized contracts Add pkg/hooks/events with a stdlib-only catalog of all hook event names, their capabilities (blocking, tool-phase, etc.) and allowed on_error values. pkg/hooks/contracts.go formalizes the Phase type, verdict constants and error sentinels used across config, executor and runtime. pkg/hooks/protocol.go captures the JSON wire-format shared between executor and dispatcher. --- agent-schema.json | 20 +- docs/configuration/agents/index.md | 4 +- docs/configuration/hooks/index.md | 106 ++- docs/configuration/permissions/index.md | 22 +- docs/configuration/user-settings/index.md | 2 +- examples/redact_secrets_hooks.yaml | 4 +- examples/tool_hook_phases.yaml | 58 ++ pkg/config/hcl/hcl.go | 2 + pkg/config/hcl/hcl_test.go | 45 + pkg/config/hooks.go | 2 + pkg/config/hooks_yaml_test.go | 59 ++ pkg/config/latest/types.go | 68 +- pkg/hooks/builtins/builtins.go | 13 +- pkg/hooks/builtins/redact_secrets.go | 10 +- pkg/hooks/builtins/redact_secrets_test.go | 102 ++- pkg/hooks/config.go | 5 +- pkg/hooks/contracts.go | 15 + pkg/hooks/contracts_test.go | 99 +++ pkg/hooks/dispatch_test.go | 32 +- pkg/hooks/events/contracts.go | 82 ++ pkg/hooks/events/contracts_test.go | 45 + pkg/hooks/executor.go | 72 +- pkg/hooks/hooks_test.go | 14 + pkg/hooks/pipeline.go | 2 +- pkg/hooks/pipeline_test.go | 13 +- pkg/hooks/protocol.go | 84 ++ pkg/hooks/tool_phases_test.go | 227 +++++ pkg/hooks/types.go | 55 +- pkg/runtime/hooks.go | 2 + pkg/runtime/runtime.go | 2 +- pkg/runtime/toolexec/confirm.go | 2 +- pkg/runtime/toolexec/dispatcher.go | 120 ++- pkg/runtime/toolexec/tool_hooks.go | 124 +++ .../toolexec/tool_phases_recheck_test.go | 118 +++ pkg/runtime/toolexec/tool_phases_test.go | 793 ++++++++++++++++++ 35 files changed, 2227 insertions(+), 196 deletions(-) create mode 100644 examples/tool_hook_phases.yaml create mode 100644 pkg/hooks/contracts.go create mode 100644 pkg/hooks/contracts_test.go create mode 100644 pkg/hooks/events/contracts.go create mode 100644 pkg/hooks/events/contracts_test.go create mode 100644 pkg/hooks/protocol.go create mode 100644 pkg/hooks/tool_phases_test.go create mode 100644 pkg/runtime/toolexec/tool_hooks.go create mode 100644 pkg/runtime/toolexec/tool_phases_recheck_test.go create mode 100644 pkg/runtime/toolexec/tool_phases_test.go diff --git a/agent-schema.json b/agent-schema.json index 3a6ee2fc40..d4b02e5bf6 100644 --- a/agent-schema.json +++ b/agent-schema.json @@ -684,7 +684,7 @@ "redact_secrets": { "type": "boolean", "default": true, - "description": "Enabled by default. When true (the default), the runtime auto-installs the redact_secrets builtin on all three of pre_tool_use (scrubs detected secrets from tool arguments), before_llm_call (scrubs the messages sent to the LLM), and tool_response_transform (scrubs tool output before it reaches event consumers, the persisted session, the post_tool_use hook input, or the next LLM call). Set to false to opt out. The same hook entries can be authored directly in YAML for finer-grained control \u2014 see the hooks.tool_response_transform / hooks.before_llm_call / hooks.pre_tool_use sections. Detection uses the portcullis ruleset (GitHub PATs, AWS keys, Stripe / Slack / GitLab tokens, JWTs, private keys, etc.). Each detected span is replaced with the literal '[REDACTED]'." + "description": "Enabled by default. When true (the default), the runtime auto-installs the redact_secrets builtin on all three of tool_input_transform (scrubs detected secrets from tool arguments before approval), before_llm_call (scrubs the messages sent to the LLM), and tool_response_transform (scrubs tool output before it reaches event consumers, the persisted session, the post_tool_use hook input, or the next LLM call). Set to false to opt out. The same hook entries can be authored directly in YAML for finer-grained control \u2014 see the hooks.tool_response_transform / hooks.before_llm_call / hooks.tool_input_transform sections. Detection uses the portcullis ruleset (GitHub PATs, AWS keys, Stripe / Slack / GitLab tokens, JWTs, private keys, etc.). Each detected span is replaced with the literal '[REDACTED]'." }, "max_iterations": { "type": "integer", @@ -1369,6 +1369,20 @@ "$ref": "#/definitions/HookMatcherConfig" } }, + "tool_input_transform": { + "type": "array", + "description": "Hooks that run before every tool call, ahead of the deterministic approval pipeline (safety mode, permission rules, --yolo) and of tool_guard. Hooks run sequentially, each seeing the preceding rewrite, and may patch tool arguments via hookSpecificOutput.updated_input exactly like pre_tool_use; the approval pipeline and every later hook see the rewritten arguments. Rewrite-only: verdicts belong on tool_guard. Failures follow the hook's on_error policy (default warn; block denies the call). The redact_secrets builtin's argument-scrubbing leg is auto-injected here. Tool-matched, like pre_tool_use; preempt_yolo is rejected because the event always preempts approval.", + "items": { + "$ref": "#/definitions/HookMatcherConfig" + } + }, + "tool_guard": { + "type": "array", + "description": "Hooks that run after tool_input_transform and before the deterministic approval pipeline, so their verdict cannot be bypassed by any safety mode (including autonomous / --yolo) or permission allow-rule. Hooks run concurrently; hookSpecificOutput.permission_decision verdicts aggregate to the most restrictive (deny > ask > allow). deny rejects the call; ask forces the interactive prompt even when the session already allowed the tool; allow is advisory (the pipeline still runs). hookSpecificOutput.metadata is merged into the confirmation prompt. updated_input is ignored \u2014 use tool_input_transform to rewrite arguments. Hook execution failures fail closed (deny), like pre_tool_use. Tool-matched; preempt_yolo is rejected because the event always preempts approval.", + "items": { + "$ref": "#/definitions/HookMatcherConfig" + } + }, "worktree_create": { "type": "array", "description": "Hooks that run once, just after `docker agent run --worktree` creates a git worktree and before the session starts. They execute inside the new worktree (their working directory is the fresh checkout), and the worktree path, branch, and source repository root are passed in worktree_path / worktree_branch / worktree_source_dir. Use them to prepare the checkout: copy untracked files like .env, install dependencies, warm caches, before the agent begins. A hook may abort the run by blocking (decision=block / continue=false / exit code 2); stdout is surfaced as additional context. Dispatched from the CLI rather than the run loop because the worktree (and the working directory every downstream component captures) must be settled before the runtime and session exist.", @@ -1402,7 +1416,7 @@ }, "preempt_yolo": { "type": "boolean", - "description": "Opt a pre_tool_use entry into firing BEFORE the deterministic approval pipeline (custom allow/ask/deny rules + safety mode). A deny/ask verdict from a preempting hook cannot be bypassed by any safety mode (including autonomous) or permission allow-rules; an allow verdict is advisory (the pipeline still runs Decide() and the rest of pre_tool_use). Default pre_tool_use entries fire AFTER Decide(). Only meaningful on pre_tool_use; ignored on other events. Set it on hooks that implement a security-critical check that must not be bypassed by auto-approval." + "description": "Opt a pre_tool_use entry into firing BEFORE the deterministic approval pipeline (custom allow/ask/deny rules + safety mode). A deny/ask verdict from a preempting hook cannot be bypassed by any safety mode (including autonomous) or permission allow-rules; an allow verdict is advisory (the pipeline still runs Decide() and the rest of pre_tool_use). Default pre_tool_use entries fire AFTER Decide(). Only meaningful on pre_tool_use; rejected on tool_input_transform and tool_guard (which always preempt approval) and ignored on other events. Set it on hooks that implement a security-critical check that must not be bypassed by auto-approval." } }, "required": [ @@ -1420,7 +1434,7 @@ }, "type": { "type": "string", - "description": "Type of hook. 'command' executes a shell command; 'builtin' invokes a named in-process Go function registered by the runtime; 'model' asks an LLM and translates its reply into the hook's native output (used for LLM-as-a-judge pre_tool_use, summarizers, etc., with no Go code). The docker-agent runtime ships these builtins: 'add_context' (context-contributing events: renders Go templates in args against hook input and joins non-empty results with newlines as additional context), 'add_date' (turn_start: today's date), 'add_environment_info' (session_start: cwd, git, OS, arch), 'add_prompt_files' (turn_start: contents of named files looked up in the workdir hierarchy and the home directory; args may include '--depth=' to additionally list, by path only, the same filenames found up to N levels below the working directory), 'add_git_status' (turn_start: `git status --short --branch`), 'add_git_diff' (turn_start: `git diff --stat`, or full diff with args=['full']), 'add_directory_listing' (session_start: top-level entries of cwd), 'add_user_info' (session_start: current OS user and hostname), 'add_recent_commits' (session_start: `git log --oneline -n N`, default N=10, override via args=['']), 'max_iterations' (before_llm_call: hard stop after N model calls; args=[''] required), 'redact_secrets' (pre_tool_use / before_llm_call / tool_response_transform: scrubs detected secrets from tool arguments, outgoing chat content, and tool output \u2014 the same builtin handles all three legs and dispatches on the event; the matching agent-level 'redact_secrets: true' flag auto-injects the entries for all three), 'unload' (on_agent_switch: POSTs `{\"model\": \"\"}` to the previous agent's DMR model endpoints \u2014 e.g. asks Docker Model Runner to release the GPU/RAM held by the just-departing model so the next agent's model can claim it. Pure HTTP, no provider-specific runtime coupling; non-DMR providers are silently skipped. Opt in by adding the entry to the agent's hooks.on_agent_switch list).", + "description": "Type of hook. 'command' executes a shell command; 'builtin' invokes a named in-process Go function registered by the runtime; 'model' asks an LLM and translates its reply into the hook's native output (used for LLM-as-a-judge pre_tool_use, summarizers, etc., with no Go code). The docker-agent runtime ships these builtins: 'add_context' (context-contributing events: renders Go templates in args against hook input and joins non-empty results with newlines as additional context), 'add_date' (turn_start: today's date), 'add_environment_info' (session_start: cwd, git, OS, arch), 'add_prompt_files' (turn_start: contents of named files looked up in the workdir hierarchy and the home directory; args may include '--depth=' to additionally list, by path only, the same filenames found up to N levels below the working directory), 'add_git_status' (turn_start: `git status --short --branch`), 'add_git_diff' (turn_start: `git diff --stat`, or full diff with args=['full']), 'add_directory_listing' (session_start: top-level entries of cwd), 'add_user_info' (session_start: current OS user and hostname), 'add_recent_commits' (session_start: `git log --oneline -n N`, default N=10, override via args=['']), 'max_iterations' (before_llm_call: hard stop after N model calls; args=[''] required), 'redact_secrets' (tool_input_transform or pre_tool_use / before_llm_call / tool_response_transform: scrubs detected secrets from tool arguments, outgoing chat content, and tool output \u2014 the same builtin handles all three legs and dispatches on the event; the matching agent-level 'redact_secrets: true' flag auto-injects the entries on tool_input_transform, before_llm_call, and tool_response_transform), 'unload' (on_agent_switch: POSTs `{\"model\": \"\"}` to the previous agent's DMR model endpoints \u2014 e.g. asks Docker Model Runner to release the GPU/RAM held by the just-departing model so the next agent's model can claim it. Pure HTTP, no provider-specific runtime coupling; non-DMR providers are silently skipped. Opt in by adding the entry to the agent's hooks.on_agent_switch list).", "enum": [ "command", "builtin", diff --git a/docs/configuration/agents/index.md b/docs/configuration/agents/index.md index 05e82ed6a2..560b5e33fd 100644 --- a/docs/configuration/agents/index.md +++ b/docs/configuration/agents/index.md @@ -313,7 +313,7 @@ Multiple processes can share the same `path:` cache file safely. Every `Store` t Secret redaction is enabled by default. The `redact_secrets` field controls scrubbing of detected credentials, tokens, and private keys from an agent's I/O; set it to `false` to opt out. It wires up three complementary defenses: -1. A `pre_tool_use` built-in hook that scrubs detected secrets from the **arguments of every tool call**, before the tool sees them. +1. A `tool_input_transform` built-in hook that scrubs detected secrets from the **arguments of every tool call**, before the tool sees them. 2. A `before_llm_call` built-in hook that scrubs the same patterns from **outgoing chat messages** — message content, multi-part text content, prior reasoning content, and the JSON-encoded arguments of any tool call still in the conversation — before they reach the model provider. 3. A `tool_response_transform` built-in hook that scrubs **tool output at the source**, so the secret never reaches event consumers, the persisted session file, the `post_tool_use` hook input, or the next LLM call. @@ -349,7 +349,7 @@ Each detected span is replaced with the literal string `[REDACTED]`; the surroun > [!NOTE] > **Equivalent hook entry** > -> The default redaction behavior (or an explicit `redact_secrets: true`) auto-registers all three legs of the feature as hook entries. They share the _same_ built-in name (`type: builtin`, `command: redact_secrets`) on `pre_tool_use`, `before_llm_call`, and `tool_response_transform` respectively — the implementation dispatches on the hook event. Set `redact_secrets: false` before wiring hooks manually to avoid also registering the default hooks. You can spell them out by hand to scope a leg to a subset of tools (set `matcher:` to a regex), stack them with other rewriters in a specific order, or enable just one or two legs. See [`examples/redact_secrets_hooks.yaml`](https://github.com/docker/docker-agent/blob/main/examples/redact_secrets_hooks.yaml) for a complete manual wiring and the [Hooks reference](../hooks/index.md#available-built-ins) for the builtin's event coverage. +> The default redaction behavior (or an explicit `redact_secrets: true`) auto-registers all three legs of the feature as hook entries. They share the _same_ built-in name (`type: builtin`, `command: redact_secrets`) on `tool_input_transform`, `before_llm_call`, and `tool_response_transform` respectively — the implementation dispatches on the hook event. Set `redact_secrets: false` before wiring hooks manually to avoid also registering the default hooks. You can spell them out by hand to scope a leg to a subset of tools (set `matcher:` to a regex), stack them with other rewriters in a specific order, or enable just one or two legs. See [`examples/redact_secrets_hooks.yaml`](https://github.com/docker/docker-agent/blob/main/examples/redact_secrets_hooks.yaml) for a complete manual wiring and the [Hooks reference](../hooks/index.md#available-built-ins) for the builtin's event coverage. ## Welcome Message diff --git a/docs/configuration/hooks/index.md b/docs/configuration/hooks/index.md index a9c333b3be..5315f8733a 100644 --- a/docs/configuration/hooks/index.md +++ b/docs/configuration/hooks/index.md @@ -33,7 +33,9 @@ Docker Agent dispatches the following hook events: | Event | When it fires | Can block? | | --------------------------- | --------------------------------------------------------------------------------- | ---------- | -| `pre_tool_use` | Before a tool call executes | Yes | +| `pre_tool_use` | Default lane: approval helper when the safety mode asks; skipped on auto-approved calls | Yes | +| `tool_input_transform` | Before tool guards, permission rules, and safety classification, including auto-approved calls | Yes | +| `tool_guard` | Mandatory checks on transformed arguments, before approval | Yes | | `tool_response_transform` | Between a tool's execution and the runtime's emission/record of the response | No | | `post_tool_use` | After a tool completes — fires for both success and failure | Yes | | `permission_request` | Just before the runtime would prompt the user to approve a tool | Yes | @@ -216,7 +218,7 @@ Built-ins are typically zero-config and faster than equivalent shell hooks becau | `add_recent_commits` | `session_start` | _none_, or `[""]` | Adds `git log --oneline -n N`. `N` defaults to 10; pass a positive integer to override. | | `max_iterations` | `before_llm_call` | `[""]` (required) | Hard-stops the agent after `N` model calls. Stateless: the runtime supplies the iteration counter on every dispatch. | | `snapshot` | `session_start`, `turn_start`, `turn_end`, `pre_tool_use`, `post_tool_use`, `session_end` | _none_ | Records filesystem snapshots in a shadow git repo under the Docker Agent data directory. No-op outside git repos; respects the source repo's ignore rules and skips newly-added files larger than 2 MiB. | -| `redact_secrets` | `pre_tool_use`, `before_llm_call`, `tool_response_transform` | _none_ | Scrubs detected secrets (API keys, tokens, private keys, …) out of tool call arguments, outgoing chat content, and tool output. The same builtin handles all three events and dispatches on the event name. Auto-registered on all three events by `redact_secrets: true` on the agent — see [`examples/redact_secrets_hooks.yaml`](https://github.com/docker/docker-agent/blob/main/examples/redact_secrets_hooks.yaml) for the manual wiring. | +| `redact_secrets` | `tool_input_transform`, `before_llm_call`, `tool_response_transform` | _none_ | Scrubs detected secrets (API keys, tokens, private keys, …) out of tool call arguments, outgoing chat content, and tool output. The same builtin handles all three events and dispatches on the event name. Auto-registered on all three events by `redact_secrets: true` on the agent — see [`examples/redact_secrets_hooks.yaml`](https://github.com/docker/docker-agent/blob/main/examples/redact_secrets_hooks.yaml) for the manual wiring. | | `limit_large_tool_results` | `tool_response_transform`, `session_end` | _none_ | **Always-on safety hook** — automatically injected by the runtime, no configuration required. When a tool result from the `filesystem`, `shell`, `mcp`, or `a2a` categories exceeds 2,000 lines or 50 KiB, the full payload is written to a per-session temp file and replaced in the conversation with a notice plus a bounded excerpt (2,000 lines, up to 50 KiB): the tail for most tools, but the head for the built-in filesystem `read_file`, whose notice suggests a follow-up call with `line`/`limit` to continue reading. The `session_end` leg deletes the temp directory. Internal toolsets (`memory`, `plan`, `tasks`, `think`, …) are not affected. | | `safer_shell` | `pre_tool_use` | _none_ | **Deprecated compatibility shim.** The runtime now classifies every shell command natively (`safe` / `destructive` / `unknown`) and gates it through the session's [safety mode](../permissions/index.md#safety-modes), so this builtin no longer emits verdicts. Pinned entries keep working as pure labellers that attach classification metadata (`safety_label`, `blast_radius`, `category`, `reason`) to the call. Filters by tool name internally (no-op for calls other than `shell` and `run_background_job`). | | `unload` | `on_agent_switch` | _none_ | POSTs `{"model": ""}` to each of the previous agent's DMR model endpoints (`/_unload` by default, overridable per-model via `unload_api`) to free the GPU/RAM the just-departing model was holding. Pure HTTP — reads the model snapshot the runtime ships on `on_agent_switch` and depends on no provider-specific runtime state. Non-DMR providers (OpenAI, Anthropic, …) are silently skipped, so cross-provider chains are safe. Errors are logged and swallowed; agent switching never blocks on a slow or unreachable engine (each call has a 10 s timeout). See [`examples/unload_on_switch.yaml`](https://github.com/docker/docker-agent/blob/main/examples/unload_on_switch.yaml). | @@ -229,7 +231,7 @@ Built-ins are typically zero-config and faster than equivalent shell hooks becau > [!NOTE] > **Auto-injected built-ins** > -> The agent flags `add_date: true`, `add_environment_info: true`, `add_prompt_files: [...]`, and `redact_secrets: true` are shorthands that auto-register the matching built-in hook. You don't need to repeat them under `hooks:` — set the flag _or_ the hook entry(ies), not both. `redact_secrets: true` auto-registers the same builtin on all three of `pre_tool_use`, `before_llm_call`, and `tool_response_transform`; you can also wire any subset of them by hand for finer-grained control (per-tool matchers, ordering with other rewriters, …). Secret redaction is enabled even when `redact_secrets` is omitted; set it to `false` before configuring only selected redaction hooks manually. +> The agent flags `add_date: true`, `add_environment_info: true`, `add_prompt_files: [...]`, and `redact_secrets: true` are shorthands that auto-register the matching built-in hook. You don't need to repeat them under `hooks:` — set the flag _or_ the hook entry(ies), not both. `redact_secrets: true` auto-registers the same builtin on all three of `tool_input_transform`, `before_llm_call`, and `tool_response_transform`; you can also wire any subset of them by hand for finer-grained control (per-tool matchers, ordering with other rewriters, …). Secret redaction is enabled even when `redact_secrets` is omitted; set it to `false` before configuring only selected redaction hooks manually. > > `limit_large_tool_results` is injected unconditionally by the runtime — it is always active and cannot be removed from config. @@ -345,6 +347,8 @@ In addition to the common fields, each event ships its own payload: | Event | Extra fields | | --------------------------- | --------------------------------------------------------------------------------------------------------------------- | +| `tool_input_transform` | `agent_name`, `tool_name`, `tool_use_id`, `tool_category`, `tool_input`, `safety_policy` | +| `tool_guard` | `agent_name`, `tool_name`, `tool_use_id`, `tool_category`, `tool_input`, `safety_policy` | | `pre_tool_use` | `agent_name`, `tool_name`, `tool_use_id`, `tool_input` | | `tool_response_transform` | `tool_name`, `tool_use_id`, `tool_input`, `tool_response` | | `post_tool_use` | `agent_name`, `tool_name`, `tool_use_id`, `tool_input`, `tool_response`, `tool_error` | @@ -385,7 +389,7 @@ Notes: - For [harness agents](../../features/harnesses/index.md), `cost` is the harness's own reported total for the call rather than a computed price, and is present only when the harness reported a non-zero cost (some harnesses, e.g. `codex`, report token counts but no cost — those turns carry `usage` with `cost` absent, even though the recorded message stores `0`). - `after_llm_call` fires for **every** model call, including calls made inside sub-sessions (transferred tasks, background agents, skills). For those, `session_id` is the sub-session's id. Summing `cost` across `after_llm_call` events therefore captures **all** spend, including sub-sessions (and even sub-sessions that error before their cost is persisted). Do **not** add a separately-queried session cost total on top: the runtime's own total already recurses into and includes completed sub-session spend, so combining the two double-counts. Pick one source — the summed hook costs — as the authoritative ledger. - `context_limit` is `0` when the model definition is unavailable (treat `0` as "unknown", not as a real limit). -- `approval_decision` is one of `allow`, `deny`, `canceled`. `approval_source` is a stable classifier of which step decided (e.g. `yolo`, `session_permissions_allow`, `session_permissions_deny`, `team_permissions_allow`, `team_permissions_deny`, `pre_tool_use_hook_allow`, `pre_tool_use_hook_deny`, `readonly_hint`, `user_approved`, `user_approved_session`, `user_approved_safe`, `user_approved_tool`, `user_rejected`, `context_canceled`). +- `approval_decision` is one of `allow`, `deny`, `canceled`. `approval_source` is a stable classifier of which step decided (e.g. `yolo`, `session_permissions_allow`, `session_permissions_deny`, `team_permissions_allow`, `team_permissions_deny`, `pre_tool_use_hook_allow`, `pre_tool_use_hook_deny`, `tool_input_transform_deny`, `tool_guard_deny`, `readonly_hint`, `user_approved`, `user_approved_session`, `user_approved_safe`, `user_approved_tool`, `user_rejected`, `context_canceled`). ## Hook Output @@ -423,17 +427,96 @@ All fields are optional. Returning `{}` (or no output at all) means "do nothing, ### Pre-Tool-Use / Permission-Request Specific Output -The `hook_specific_output` for `pre_tool_use` (and `permission_request`) supports: +The following fields are supported by tool hooks as indicated: | Field | Type | Description | | ---------------------------- | ------ | --------------------------------------- | -| `permission_decision` | string | `allow`, `deny`, or `ask` | +| `permission_decision` | string | `allow`, `deny`, or `ask` for `tool_guard`, `pre_tool_use`, and `permission_request` | | `permission_decision_reason` | string | Explanation for the decision | -| `updated_input` | object | Top-level patch to the current tool input (`pre_tool_use` default lane only); omitted keys are preserved | -| `metadata` | object | (`permission_request` and `pre_tool_use` entries with `preempt_yolo: true` only) string key/value annotations merged onto the tool-call confirmation prompt — see below | +| `updated_input` | object | Top-level patch to the current tool input (`tool_input_transform` or the `pre_tool_use` default lane); omitted keys are preserved | +| `metadata` | object | (`tool_guard`, `permission_request`, and `pre_tool_use` entries with `preempt_yolo: true` only) string key/value annotations merged onto the tool-call confirmation prompt — see below | + +### Tool phases: transform, guard, approve + +Use separate events for operations that must run regardless of approval: + +1. **`tool_input_transform`** runs sequentially before safety classification or + permission checks. Return `hook_specific_output.updated_input` to patch the + arguments. Every later guard, prompt, and tool handler sees the final input. + `permission_decision` and `metadata` have no effect on this event. +2. **`tool_guard`** runs mandatory checks against that input. Matching guards run + concurrently and combine verdicts using **deny > ask > allow**. They cannot + rewrite arguments. +3. Existing **`pre_tool_use` with `preempt_yolo: true`** runs next, followed by + permission rules and the safety-mode decision. +4. Ordinary **`pre_tool_use`** remains an approval helper: it runs only when the + safety mode asks. Auto-approved calls and explicit permission ask rules skip + it. Keep expensive LLM judges here unless they must inspect every call. +5. **`permission_request`** and interactive confirmation remain the fallback. + A mandatory guard's `ask` skips approval helpers and forces confirmation. + +For `tool_guard`: + +- `deny`, `decision: block`, `continue: false`, or exit code `2` rejects the call. +- `ask` requires approval for **this call**, even with `--yolo`, a permission + allow-rule, or a previous “always allow” grant. An explicit policy denial still + wins. Non-interactive sessions deny rather than wait for an unavailable user. +- `allow` is advisory: permission rules and safety mode still apply. +- No verdict leaves approval unchanged. `metadata` enriches confirmation; + guard keys win over static, permission-hook, safety-label, and legacy + preempt-hook metadata. Within the event, the last configured hook wins clashes. + +Both events use the existing tool-name `matcher` syntax and apply to nested +shell actions such as commands embedded in skills. They run once per call; +if a legacy `pre_tool_use` hook changes arguments afterwards, guards and rules +are checked again against the rewritten call. Transforms and approval helpers +are not rerun: legacy rewrites are not automatically re-redacted. A new ask +during revalidation requires fresh approval, not an earlier session grant. +A no-op patch does not trigger another guard invocation. +Prefer `tool_input_transform` for new rewriters so guards only need one pass. + +```yaml +hooks: + tool_input_transform: + - matcher: "shell" + hooks: + - type: command + command: ./normalize-tool-input.sh + on_error: block + tool_guard: + - matcher: "shell" + hooks: + - type: command + command: ./check-tool-policy.sh + timeout: 5 + pre_tool_use: + - matcher: "shell" + hooks: + - type: model + model: openai/gpt-4o-mini + schema: pre_tool_use_decision + prompt: 'May this call be auto-approved? {{ .ToolInput | toJSON }}' +``` + +**Failures:** transform execution errors follow `on_error` (default `warn`); +`on_error: block`, `decision: block`, `continue: false`, and exit `2` prevent +execution. Guard execution errors and timeouts block regardless of `on_error`. +Both retain the existing shell exit-code protocol: nonzero codes other than `2` +are non-blocking, and malformed stdout JSON is not a verdict. A command guard +must explicitly emit a blocking result or exit `2` when it cannot check safely. +Neither event accepts `preempt_yolo`, since both already precede approval. + +The default secret-redaction argument hook now uses `tool_input_transform`, so +redaction also applies under auto-approval. Explicit legacy `pre_tool_use` +redactors keep their conditional behavior; move them to `tool_input_transform` +to cover every call. See [the complete example](https://github.com/docker/docker-agent/blob/main/examples/tool_hook_phases.yaml). ### Preempting auto-approval from `pre_tool_use` +For new mandatory checks, prefer `tool_guard`. The legacy `preempt_yolo` option +remains supported, including its exception for session-scoped “always allow” +grants. Unlike that option, a `tool_guard` ask always requires fresh approval. + `pre_tool_use` entries default to firing AFTER the deterministic approval pipeline (custom allow rules / safety mode), so an auto-approved call skips them entirely. For security-critical checks that MUST run on every @@ -450,7 +533,7 @@ hooks: command: ./security-check.sh ``` -The entry then fires in a dedicated stage 0 BEFORE `Decide()`: +The entry fires after `tool_input_transform` and `tool_guard`, before `Decide()`: - `deny` rejects the call outright; the user is not prompted. - `ask` forces user confirmation. The default `pre_tool_use` lane and @@ -493,7 +576,7 @@ The `hook_specific_output` for `tool_response_transform` supports: | ----------------------- | ------ | --------------------------------------------- | | `updated_tool_response` | string | Rewritten tool output (replaces the original) | -This is the symmetric counterpart of `pre_tool_use`'s `updated_input`, applied to tool **results** instead of tool **arguments**. The rewrite reaches every downstream consumer — event subscribers, the persisted session file, the `post_tool_use` hook input, and the next LLM call. Use it to truncate excessive output, scrub PII, or normalise tool dialects. The built-in `redact_secrets` registers itself on this event as the third leg of the redact_secrets feature. +This is the symmetric counterpart of `tool_input_transform`'s `updated_input`, applied to tool **results** instead of tool **arguments**. The rewrite reaches every downstream consumer — event subscribers, the persisted session file, the `post_tool_use` hook input, and the next LLM call. Use it to truncate excessive output, scrub PII, or normalise tool dialects. The built-in `redact_secrets` registers itself on this event as the third leg of the redact_secrets feature. ### Composing transformations @@ -501,6 +584,7 @@ Hooks for the following events run **sequentially in configuration order**: | Event | Rewrite field | What the next hook receives | | ----- | ------------- | --------------------------- | +| `tool_input_transform` | `updated_input` | `tool_input` with the patch applied | | `pre_tool_use` (default lane) | `updated_input` | `tool_input` with the patch applied | | `before_llm_call` | `updated_messages` | The rewritten `messages` array | | `tool_response_transform` | `updated_tool_response` | The rewritten `tool_response` string | @@ -523,7 +607,7 @@ verdicts do not short-circuit the remaining hooks. Failed invocations contribute no rewrite and keep the existing error-policy behavior. Each hook retains its own timeout, so pipeline latency can add up across hooks. -Other events, including `preempt_yolo: true` checks and `before_compaction`, +Other events, including `tool_guard`, `preempt_yolo: true` checks, and `before_compaction`, continue to run concurrently. Preempting checks do not apply input rewrites; compaction summaries still use the first non-empty result in configuration order. diff --git a/docs/configuration/permissions/index.md b/docs/configuration/permissions/index.md index 3375845289..b04b6a5bfe 100644 --- a/docs/configuration/permissions/index.md +++ b/docs/configuration/permissions/index.md @@ -31,7 +31,7 @@ Every session runs in a **safety mode** that decides what happens when no permis - **`strict`** prompts for every tool call, read-only ones included. Only an `allow:` rule silences a prompt. - **`balanced`** runs safe calls silently and asks about everything else. - **`restricted`** is the fail-closed profile for unattended/headless runs: safe calls run silently and everything else is **denied without asking** — the mode's fallback never prompts. Custom rules still win: an `allow:` rule can approve a destructive/unknown call, a `deny:` rule always blocks, and session-scoped `ask:` rules still prompt (as can a `preempt_yolo` hook). Restricted is defense in depth against unwanted tool calls, not a security boundary — for real isolation use [sandbox mode](../sandbox/index.md). -- **`autonomous`** is the legacy `--yolo` behavior: everything runs. Only `deny:` rules, session-scoped `ask:` rules, and `preempt_yolo` hooks still gate. +- **`autonomous`** is the legacy `--yolo` behavior: everything runs. Only `deny:` rules, session-scoped `ask:` rules, `tool_guard`, and `preempt_yolo` hooks still gate. Pick a mode with the `--safety` flag (`docker-agent run --safety balanced ...`), the `safety_policy` field on session create (`POST /api/sessions`) or mid-session (`PATCH /api/sessions/:id/safety-policy`), or escalate directly from a confirmation prompt (`B` switches to balanced, `A` to autonomous; the `restricted` fallback never prompts, so the mode is only selected via flag/config/API). Sessions that never choose a mode keep the historical default: read-only tools auto-approve, everything else asks. @@ -298,15 +298,17 @@ permissions: Permissions work alongside [hooks](../hooks/index.md). The evaluation order is: -1. Run **`preempt_yolo` pre_tool_use hooks** — security-critical checks that no mode or allow rule can bypass -2. Check **deny** patterns — if matched, tool is blocked -3. Check **allow** patterns — if matched, tool is auto-approved -4. Check **ask** patterns — if matched, the user is prompted directly, skipping the default `pre_tool_use` lane -5. If no rule matched, apply the **[safety mode](#safety-modes)** to the call's safety label — may auto-approve (or, under `restricted`, deny) -6. On a mode "ask", run **pre_tool_use hooks** — hooks can allow, deny, or ask -7. If no decision, **ask user** for confirmation - -Default-lane hooks only see calls the mode routed to "ask"; they cannot override deny decisions or explicit `ask:` rules. +1. Run **`tool_input_transform` hooks** — patch arguments before classification, guards, or permission checks +2. Run **`tool_guard` hooks** — mandatory checks; deny is terminal and ask requires fresh confirmation (unless policy denies the call) +3. Run **`preempt_yolo` pre_tool_use hooks** — legacy mandatory checks, with their existing session-grant exception +4. Check **deny** patterns — if matched, tool is blocked +5. Check **allow** patterns — if matched, tool is auto-approved +6. Check **ask** patterns — if matched, the user is prompted directly, skipping the default `pre_tool_use` lane +7. If no rule matched, apply the **[safety mode](#safety-modes)** to the call's safety label — may auto-approve (or, under `restricted`, deny) +8. On a mode "ask", run **pre_tool_use hooks** — hooks can allow, deny, or ask +9. If no decision, **ask user** for confirmation + +Default-lane hooks only see calls the mode routed to "ask"; they cannot override deny decisions or explicit `ask:` rules. If they rewrite arguments, mandatory checks and permission rules are evaluated again before execution. A new ask during this recheck requires fresh confirmation; earlier grants cannot bypass it. See [tool phases](../hooks/index.md#tool-phases-transform-guard-approve). > [!WARNING] > **Security Note** diff --git a/docs/configuration/user-settings/index.md b/docs/configuration/user-settings/index.md index 6e72f548db..7ca1418a36 100644 --- a/docs/configuration/user-settings/index.md +++ b/docs/configuration/user-settings/index.md @@ -44,7 +44,7 @@ You rarely need to hand-edit this file. Most fields are managed from the TUI's ` | `theme` | string | `default` | Theme name, loaded from a built-in theme or `~/.cagent/themes/.yaml`. The special value `auto` follows the terminal's light/dark background. See [Theming](../../features/tui/index.md#theming). | | `theme_dark` | string | `default` | Theme applied when `theme: auto` and the terminal background is dark. | | `theme_light` | string | `default-light` | Theme applied when `theme: auto` and the terminal background is light. | -| `YOLO` | boolean | `false` | Select the `autonomous` safety fallback globally. Unmatched calls are auto-approved, but deny rules, session-scoped ask rules, and `preempt_yolo` hooks can still block or prompt. Mirrors the `--yolo` flag and the `/yolo` command. Legacy alias for `safety: autonomous`; when both are set, `safety` wins. | +| `YOLO` | boolean | `false` | Select the `autonomous` safety fallback globally. Unmatched calls are auto-approved, but deny rules, session-scoped ask rules, `tool_guard`, and `preempt_yolo` hooks can still block or prompt. Mirrors the `--yolo` flag and the `/yolo` command. Legacy alias for `safety: autonomous`; when both are set, `safety` wins. | | `safety` | string | _unset_ | Default [safety mode](../permissions/index.md#safety-modes) for new sessions: `strict`, `balanced`, `restricted`, or `autonomous` (any other value fails config loading). Wins over the legacy `YOLO` flag. Applied when no explicit `--safety`/`--yolo` flag and no alias safety option was given; wins over the agent YAML's `agents..safety` / `runtime.safety` defaults. Never changes the mode of a resumed session. | | `lean` | boolean | `false` | Make the [lean TUI](../../features/tui/index.md#lean-tui) (simplified, minimal-chrome interface) the default for interactive runs instead of the full TUI. | | `tab_title_max_length` | int | `20` | Maximum display length for tab titles; longer titles are truncated with an ellipsis. | diff --git a/examples/redact_secrets_hooks.yaml b/examples/redact_secrets_hooks.yaml index 0ab152ba14..29548db7fb 100644 --- a/examples/redact_secrets_hooks.yaml +++ b/examples/redact_secrets_hooks.yaml @@ -16,7 +16,7 @@ # (`redact_secrets`); the implementation dispatches on the hook event # so a single registered builtin handles every leg. # -# * pre_tool_use → scrub tool ARGUMENTS before the tool +# * tool_input_transform → scrub tool ARGUMENTS before the tool # process sees them. # * before_llm_call → scrub outgoing CHAT CONTENT before the # model provider sees it. @@ -40,7 +40,7 @@ agents: toolsets: - type: shell hooks: - pre_tool_use: + tool_input_transform: - matcher: "*" hooks: - type: builtin diff --git a/examples/tool_hook_phases.yaml b/examples/tool_hook_phases.yaml new file mode 100644 index 0000000000..53dddb6be4 --- /dev/null +++ b/examples/tool_hook_phases.yaml @@ -0,0 +1,58 @@ +# Requires python3 for the command hooks. Try with --yolo: the guard still runs. +agents: + root: + model: openai/gpt-5-mini + description: Separate input normalization, mandatory checks, and approval assistance + instruction: Help with shell commands. Respect tool policy rejections. + toolsets: + - type: shell + hooks: + tool_input_transform: + - matcher: shell + hooks: + - name: normalize whitespace + type: command + on_error: block + command: | + python3 -c ' + import json, sys + payload = json.load(sys.stdin) + command = payload["tool_input"].get("cmd", "") + print(json.dumps({"hook_specific_output": { + "updated_input": {"cmd": command.strip()} + }})) + ' || exit 2 + tool_guard: + - matcher: shell + hooks: + - name: require confirmation for project commands + type: command + timeout: 5 + command: | + python3 -c ' + import json, sys + payload = json.load(sys.stdin) + command = payload["tool_input"].get("cmd", "") + # Exact matches, not a shell-prefix allowlist. + decision = "allow" if command in ("pwd", "git status") else "ask" + print(json.dumps({"hook_specific_output": { + "permission_decision": decision, + "permission_decision_reason": "Review commands beyond the read-only allowlist", + "metadata": {"policy": "project command review"} + }})) + ' || exit 2 + # This judge only runs when the guard falls through and safety mode asks. + # A guard's "ask" skips it; an "allow" here cannot undo a guard denial. + pre_tool_use: + - matcher: shell + hooks: + - name: approval assistant + type: model + model: openai/gpt-4o-mini + timeout: 15 + schema: pre_tool_use_decision + prompt: | + Decide allow, ask, or deny for this tool call. + Allow only clearly read-only operations; ask when uncertain. + Tool: {{ .ToolName }} + Arguments: {{ .ToolInput | toJSON }} diff --git a/pkg/config/hcl/hcl.go b/pkg/config/hcl/hcl.go index 527746b03f..ba25470123 100644 --- a/pkg/config/hcl/hcl.go +++ b/pkg/config/hcl/hcl.go @@ -194,6 +194,8 @@ var blockRules = map[string]blockRule{ "session_end": {mode: modeList, outKey: "session_end"}, "permission_request": {mode: modeList, outKey: "permission_request"}, "tool_response_transform": {mode: modeList, outKey: "tool_response_transform"}, + "tool_input_transform": {mode: modeList, outKey: "tool_input_transform"}, + "tool_guard": {mode: modeList, outKey: "tool_guard"}, } // lookupRule returns the conversion rule for a block, falling back to a diff --git a/pkg/config/hcl/hcl_test.go b/pkg/config/hcl/hcl_test.go index 54acdc0add..8d019151ef 100644 --- a/pkg/config/hcl/hcl_test.go +++ b/pkg/config/hcl/hcl_test.go @@ -246,3 +246,48 @@ func TestLooksLikeHCL(t *testing.T) { }) } } + +func TestToYAML_ToolPhaseHookBlocksAggregateIntoLists(t *testing.T) { + t.Parallel() + + src := []byte(` +agent "root" { + instruction = "x" + model = "auto" + + hooks { + tool_input_transform { + matcher = "shell" + hook { + type = "builtin" + command = "redact_secrets" + } + } + tool_guard { + hook { + type = "command" + command = "./guard-a.sh" + } + } + tool_guard { + hook { + type = "command" + command = "./guard-b.sh" + } + } + } +} +`) + + m, err := ToMap(src, "test.hcl") + require.NoError(t, err) + agents := m["agents"].(yaml.MapSlice) + root := agents[0].Value.(map[string]any) + hooks := root["hooks"].(map[string]any) + + transform := hooks["tool_input_transform"].([]any) + require.Len(t, transform, 1) + assert.Equal(t, "shell", transform[0].(map[string]any)["matcher"]) + guards := hooks["tool_guard"].([]any) + require.Len(t, guards, 2, "repeated 0-label blocks aggregate into a list") +} diff --git a/pkg/config/hooks.go b/pkg/config/hooks.go index 9b5cee4cb9..3ed7a0b3ed 100644 --- a/pkg/config/hooks.go +++ b/pkg/config/hooks.go @@ -87,6 +87,8 @@ func MergeHooks(base, cli *latest.HooksConfig) *latest.HooksConfig { BeforeCompaction: slices.Concat(base.BeforeCompaction, cli.BeforeCompaction), AfterCompaction: slices.Concat(base.AfterCompaction, cli.AfterCompaction), ToolResponseTransform: slices.Concat(base.ToolResponseTransform, cli.ToolResponseTransform), + ToolInputTransform: slices.Concat(base.ToolInputTransform, cli.ToolInputTransform), + ToolGuard: slices.Concat(base.ToolGuard, cli.ToolGuard), WorktreeCreate: slices.Concat(base.WorktreeCreate, cli.WorktreeCreate), } return merged diff --git a/pkg/config/hooks_yaml_test.go b/pkg/config/hooks_yaml_test.go index 021a19128f..b4705f3ff5 100644 --- a/pkg/config/hooks_yaml_test.go +++ b/pkg/config/hooks_yaml_test.go @@ -199,3 +199,62 @@ after_llm_call: assert.False(t, (&latest.HooksConfig{BeforeLLMCall: cfg.BeforeLLMCall}).IsEmpty()) assert.False(t, (&latest.HooksConfig{AfterLLMCall: cfg.AfterLLMCall}).IsEmpty()) } + +// TestHooksConfig_ToolInputTransformAndToolGuard_YAML pins the two +// pre-approval tool events: both parse as tool-matched entries and +// reject preempt_yolo, which is meaningless on events that always run +// before approval. +func TestHooksConfig_ToolInputTransformAndToolGuard_YAML(t *testing.T) { + t.Parallel() + + const src = ` +tool_input_transform: + - matcher: shell + hooks: + - type: builtin + command: redact_secrets +tool_guard: + - matcher: "*" + hooks: + - type: command + command: ./scripts/guard.sh +` + + var cfg latest.HooksConfig + require.NoError(t, yaml.Unmarshal([]byte(src), &cfg)) + require.NoError(t, cfg.Validate()) + + require.Len(t, cfg.ToolInputTransform, 1) + assert.Equal(t, "shell", cfg.ToolInputTransform[0].Matcher) + assert.Equal(t, "redact_secrets", cfg.ToolInputTransform[0].Hooks[0].Command) + require.Len(t, cfg.ToolGuard, 1) + assert.Equal(t, "./scripts/guard.sh", cfg.ToolGuard[0].Hooks[0].Command) + + assert.False(t, (&latest.HooksConfig{ToolInputTransform: cfg.ToolInputTransform}).IsEmpty()) + assert.False(t, (&latest.HooksConfig{ToolGuard: cfg.ToolGuard}).IsEmpty()) + + preempt := true + for _, tc := range []struct { + name string + cfg latest.HooksConfig + }{ + {"tool_input_transform", latest.HooksConfig{ToolInputTransform: latest.HookMatcherConfigs{{PreemptYolo: &preempt, Hooks: cfg.ToolGuard[0].Hooks}}}}, + {"tool_guard", latest.HooksConfig{ToolGuard: latest.HookMatcherConfigs{{PreemptYolo: &preempt, Hooks: cfg.ToolGuard[0].Hooks}}}}, + } { + err := tc.cfg.Validate() + require.Error(t, err, tc.name) + assert.Contains(t, err.Error(), "hooks."+tc.name+"[0]: preempt_yolo is not valid on "+tc.name) + } + + // Legacy events still accept it (pre_tool_use) or ignore it (others). + legacy := latest.HooksConfig{ + PreToolUse: latest.HookMatcherConfigs{{PreemptYolo: &preempt, Hooks: cfg.ToolGuard[0].Hooks}}, + PostToolUse: latest.HookMatcherConfigs{{PreemptYolo: &preempt, Hooks: cfg.ToolGuard[0].Hooks}}, + } + require.NoError(t, legacy.Validate()) + + // Malformed entries are still caught on the new events. + err := (&latest.HooksConfig{ToolGuard: latest.HookMatcherConfigs{{Matcher: "*"}}}).Validate() + require.Error(t, err) + assert.Contains(t, err.Error(), "hooks.tool_guard[0]: at least one hook is required") +} diff --git a/pkg/config/latest/types.go b/pkg/config/latest/types.go index 74ef0503a0..82d0ca0e3f 100644 --- a/pkg/config/latest/types.go +++ b/pkg/config/latest/types.go @@ -729,14 +729,15 @@ type AgentConfig struct { // session. Takes precedence over the config-wide RuntimeDefaults.Safety. Safety SafetyMode `json:"safety,omitempty" yaml:"safety,omitempty"` // RedactSecrets enables every leg of the redact_secrets feature: - // the pre_tool_use builtin (scrubs tool arguments), the - // before_llm_call hook (scrubs outgoing chat content), and the - // tool_response_transform hook (scrubs tool output before it - // reaches event consumers, the persisted session, the post_tool_use - // hook, or the next LLM call). Equivalent to writing all three - // hook entries by hand — the runtime auto-injects them when this - // flag is true. See pkg/hooks/builtins/redact_secrets.go for the - // hook-side implementation. + // the tool_input_transform builtin (scrubs tool arguments before + // approval), the before_llm_call hook (scrubs outgoing chat + // content), and the tool_response_transform hook (scrubs tool + // output before it reaches event consumers, the persisted session, + // the post_tool_use hook, or the next LLM call). Equivalent to + // writing all three hook entries by hand — the runtime auto-injects + // them when this flag is true. See + // pkg/hooks/builtins/redact_secrets.go for the hook-side + // implementation. // // Pointer (tri-state) so we can distinguish "unset" (nil → default // on) from "explicitly disabled" (false). Use @@ -2770,6 +2771,28 @@ type HooksConfig struct { // pre_tool_use / post_tool_use. ToolResponseTransform HookMatcherConfigs `json:"tool_response_transform,omitempty" yaml:"tool_response_transform,omitempty"` + // ToolInputTransform hooks run before every tool call, ahead of the + // deterministic approval pipeline (safety mode, permission rules, + // --yolo) and of tool_guard. Hooks run sequentially and may patch + // tool arguments via HookSpecificOutput.updated_input, exactly like + // pre_tool_use; the approval pipeline and every later hook see the + // rewritten arguments. Verdicts belong on tool_guard. Failures follow + // on_error (default warn). Tool-matched; preempt_yolo is rejected + // here because the event always preempts approval. + ToolInputTransform HookMatcherConfigs `json:"tool_input_transform,omitempty" yaml:"tool_input_transform,omitempty"` + + // ToolGuard hooks run after tool_input_transform and before the + // deterministic approval pipeline, so their verdict cannot be + // bypassed by any safety mode or permission allow-rule. Hooks run + // concurrently; permission_decision verdicts aggregate to the most + // restrictive (deny > ask > allow). Deny rejects the call, ask forces + // the user prompt even when the session already allowed the tool, + // allow is advisory. metadata is merged into the confirmation + // prompt. updated_input is ignored — use tool_input_transform. Hook + // failures fail closed (deny). Tool-matched; preempt_yolo is + // rejected here because the event always preempts approval. + ToolGuard HookMatcherConfigs `json:"tool_guard,omitempty" yaml:"tool_guard,omitempty"` + // WorktreeCreate hooks run once, just after `docker agent run // --worktree` creates a git worktree and before the session starts. // They execute inside the new worktree (their working directory is @@ -2813,6 +2836,8 @@ func (h *HooksConfig) IsEmpty() bool { len(h.BeforeCompaction) == 0 && len(h.AfterCompaction) == 0 && len(h.ToolResponseTransform) == 0 && + len(h.ToolInputTransform) == 0 && + len(h.ToolGuard) == 0 && len(h.WorktreeCreate) == 0 } @@ -2833,7 +2858,9 @@ type HookMatcherConfig struct { // permission allow-rules; an allow verdict is advisory (the // pipeline still runs Decide() and the rest of pre_tool_use). // Default pre_tool_use entries fire AFTER Decide(), as before. - // Only valid on pre_tool_use; ignored on other events. + // Only valid on pre_tool_use. Rejected on tool_input_transform and + // tool_guard, which always preempt approval; ignored on other + // events. // // Set it on hooks that implement a security-critical check that // must not be bypassed by auto-approval. @@ -3158,6 +3185,20 @@ func (h *HooksConfig) Validate() error { } } + // Validate ToolInputTransform matchers + for i, m := range h.ToolInputTransform { + if err := m.validatePreApproval("tool_input_transform", i); err != nil { + return err + } + } + + // Validate ToolGuard matchers + for i, m := range h.ToolGuard { + if err := m.validatePreApproval("tool_guard", i); err != nil { + return err + } + } + // Validate WorktreeCreate hooks for i, hook := range h.WorktreeCreate { if err := hook.validate("worktree_create", i); err != nil { @@ -3183,6 +3224,15 @@ func (m *HookMatcherConfig) validate(eventType string, index int) error { return nil } +// validatePreApproval validates a matcher on an event that always runs +// before approval, where preempt_yolo is meaningless and rejected. +func (m *HookMatcherConfig) validatePreApproval(eventType string, index int) error { + if m.PreemptYolo != nil { + return fmt.Errorf("hooks.%s[%d]: preempt_yolo is not valid on %s (it always runs before approval)", eventType, index, eventType) + } + return m.validate(eventType, index) +} + // validate validates a HookDefinition func (h *HookDefinition) validate(prefix string, index int) error { if h.Type == "" { diff --git a/pkg/hooks/builtins/builtins.go b/pkg/hooks/builtins/builtins.go index f19e934935..8f210cd626 100644 --- a/pkg/hooks/builtins/builtins.go +++ b/pkg/hooks/builtins/builtins.go @@ -23,7 +23,8 @@ // session_end) — shadow-git snapshots. Installed via // [RegisterSnapshot] (separate entry point) so the embedder receives // a [SnapshotController] to drive /undo, /snapshots, /reset. -// - redact_secrets (pre_tool_use, +// - redact_secrets (tool_input_transform +// or pre_tool_use, // before_llm_call, // tool_response_transform) — scrub secrets // from tool args, outgoing chat content, and @@ -134,8 +135,8 @@ type AgentDefaults struct { // add_prompt_files.go. AddPromptFilesDepth int // RedactSecrets auto-injects the redact_secrets builtin under - // pre_tool_use, before_llm_call, and tool_response_transform — the - // three legs of the feature. Equivalent to writing those three + // tool_input_transform, before_llm_call, and tool_response_transform + // — the three legs of the feature. Equivalent to writing those three // hook entries by hand; the dedup in [hooks.Executor.hooksFor] // makes the auto-injection idempotent against an explicit YAML // entry that already names the same builtin. @@ -188,8 +189,10 @@ func ApplyAgentDefaults(cfg *hooks.Config, d AgentDefaults) *hooks.Config { // inject explicit entries here so the resulting effective // config is self-describing (a user inspecting it sees that // args, messages, and tool output are all covered, without - // having to read the dispatch table). - cfg.PreToolUse = append(cfg.PreToolUse, hooks.MatcherConfig{ + // having to read the dispatch table). Arguments are scrubbed on + // tool_input_transform so the approval pipeline never sees the + // raw secret. + cfg.ToolInputTransform = append(cfg.ToolInputTransform, hooks.MatcherConfig{ Matcher: "*", Hooks: []hooks.Hook{builtinHook(RedactSecrets)}, }) diff --git a/pkg/hooks/builtins/redact_secrets.go b/pkg/hooks/builtins/redact_secrets.go index 5e89785890..d034da42b6 100644 --- a/pkg/hooks/builtins/redact_secrets.go +++ b/pkg/hooks/builtins/redact_secrets.go @@ -17,8 +17,10 @@ import ( // party. The same builtin is registered once and dispatches on // [hooks.Input.HookEventName] so a single name covers all three legs: // -// - [hooks.EventPreToolUse] — scrub tool ARGUMENTS before -// the call leaves the runtime (returns UpdatedInput). +// - [hooks.EventToolInputTransform] — scrub tool ARGUMENTS before +// approval and before the call leaves the runtime (returns +// UpdatedInput). [hooks.EventPreToolUse] is still accepted for +// explicit legacy YAML entries. // - [hooks.EventBeforeLLMCall] — scrub outgoing CHAT CONTENT // before each model call (returns UpdatedMessages). // - [hooks.EventToolResponseTransform] — scrub tool OUTPUT before it @@ -45,7 +47,7 @@ func redactSecrets(_ context.Context, in *hooks.Input, _ []string) (*hooks.Outpu return nil, nil } switch in.HookEventName { - case hooks.EventPreToolUse: + case hooks.EventToolInputTransform, hooks.EventPreToolUse: return redactToolArgs(in), nil case hooks.EventBeforeLLMCall: return redactOutgoingMessages(in), nil @@ -82,7 +84,7 @@ func redactToolArgs(in *hooks.Input) *hooks.Output { return &hooks.Output{ SystemMessage: fmt.Sprintf("redact_secrets: redacted secret material from arguments of tool %q", in.ToolName), HookSpecificOutput: &hooks.HookSpecificOutput{ - HookEventName: hooks.EventPreToolUse, + HookEventName: in.HookEventName, UpdatedInput: updated, }, } diff --git a/pkg/hooks/builtins/redact_secrets_test.go b/pkg/hooks/builtins/redact_secrets_test.go index 12a903e8fa..c56a7def30 100644 --- a/pkg/hooks/builtins/redact_secrets_test.go +++ b/pkg/hooks/builtins/redact_secrets_test.go @@ -19,34 +19,42 @@ func fakeGitHubPAT() string { // TestRedactSecretsScrubsTopLevelStringValue: a recognised secret in // a top-level string argument is replaced and ONLY the rewritten key -// is emitted in UpdatedInput. +// is emitted in UpdatedInput. Both the auto-injected +// tool_input_transform leg and an explicit legacy pre_tool_use entry +// scrub arguments, echoing the dispatching event name back. func TestRedactSecretsScrubsTopLevelStringValue(t *testing.T) { t.Parallel() - secret := fakeGitHubPAT() - - in := &hooks.Input{ - HookEventName: hooks.EventPreToolUse, - ToolName: "shell", - ToolInput: map[string]any{ - "command": "curl -H 'Authorization: token " + secret + "' https://api.github.com", - "timeout": 30, - }, - } + for _, event := range []hooks.EventType{hooks.EventToolInputTransform, hooks.EventPreToolUse} { + t.Run(string(event), func(t *testing.T) { + t.Parallel() - out, err := redactSecrets(t.Context(), in, nil) - require.NoError(t, err) - require.NotNil(t, out, "must return Output when redaction happened") - require.NotNil(t, out.HookSpecificOutput) + secret := fakeGitHubPAT() - updated := out.HookSpecificOutput.UpdatedInput - cmd, ok := updated["command"].(string) - require.True(t, ok, "changed key must appear in UpdatedInput") - assert.NotContains(t, cmd, secret, "raw secret must be gone") - assert.Contains(t, cmd, portcullis.Marker) - assert.NotContains(t, updated, "timeout", - "unchanged keys need no patch") - assert.Equal(t, hooks.EventPreToolUse, out.HookSpecificOutput.HookEventName) + in := &hooks.Input{ + HookEventName: event, + ToolName: "shell", + ToolInput: map[string]any{ + "command": "curl -H 'Authorization: token " + secret + "' https://api.github.com", + "timeout": 30, + }, + } + + out, err := redactSecrets(t.Context(), in, nil) + require.NoError(t, err) + require.NotNil(t, out, "must return Output when redaction happened") + require.NotNil(t, out.HookSpecificOutput) + + updated := out.HookSpecificOutput.UpdatedInput + cmd, ok := updated["command"].(string) + require.True(t, ok, "changed key must appear in UpdatedInput") + assert.NotContains(t, cmd, secret, "raw secret must be gone") + assert.Contains(t, cmd, portcullis.Marker) + assert.NotContains(t, updated, "timeout", + "unchanged keys need no patch") + assert.Equal(t, event, out.HookSpecificOutput.HookEventName) + }) + } } // TestRedactSecretsReturnsNilWhenNothingChanged: clean tool calls @@ -149,21 +157,23 @@ func TestRedactSecretsIsRegistered(t *testing.T) { // TestApplyAgentDefaultsInjectsRedactSecrets: setting the agent flag // must materialise hook entries for ALL THREE legs of the -// redact_secrets feature — pre_tool_use (tool args), before_llm_call -// (outgoing chat), and tool_response_transform (tool output) — each -// pointing at the same redact_secrets builtin. +// redact_secrets feature — tool_input_transform (tool args, before +// approval), before_llm_call (outgoing chat), and +// tool_response_transform (tool output) — each pointing at the same +// redact_secrets builtin. Nothing is injected on pre_tool_use. func TestApplyAgentDefaultsInjectsRedactSecrets(t *testing.T) { t.Parallel() cfg := ApplyAgentDefaults(nil, AgentDefaults{RedactSecrets: true}) require.NotNil(t, cfg) - // Leg 1: pre_tool_use, wildcard matcher. - require.Len(t, cfg.PreToolUse, 1) - assert.Equal(t, "*", cfg.PreToolUse[0].Matcher) - require.Len(t, cfg.PreToolUse[0].Hooks, 1) - assert.Equal(t, hooks.HookTypeBuiltin, cfg.PreToolUse[0].Hooks[0].Type) - assert.Equal(t, RedactSecrets, cfg.PreToolUse[0].Hooks[0].Command) + // Leg 1: tool_input_transform, wildcard matcher. + require.Len(t, cfg.ToolInputTransform, 1) + assert.Equal(t, "*", cfg.ToolInputTransform[0].Matcher) + require.Len(t, cfg.ToolInputTransform[0].Hooks, 1) + assert.Equal(t, hooks.HookTypeBuiltin, cfg.ToolInputTransform[0].Hooks[0].Type) + assert.Equal(t, RedactSecrets, cfg.ToolInputTransform[0].Hooks[0].Command) + assert.Empty(t, cfg.PreToolUse, "argument scrubbing moved off pre_tool_use") // Leg 2: before_llm_call, flat (event is not tool-scoped). require.Len(t, cfg.BeforeLLMCall, 1) @@ -353,3 +363,31 @@ func TestRedactSecretsLenientOnUnsupportedEvent(t *testing.T) { require.NoError(t, err) assert.Nil(t, out) } + +// TestRedactSecretsToolInputTransformEndToEnd wires the auto-injected +// config through a real executor: dispatching tool_input_transform +// yields the scrubbed arguments in Result.ModifiedInput, and nothing +// runs on pre_tool_use anymore. +func TestRedactSecretsToolInputTransformEndToEnd(t *testing.T) { + t.Parallel() + + reg := hooks.NewRegistry() + require.NoError(t, Register(reg)) + cfg := ApplyAgentDefaults(nil, AgentDefaults{RedactSecrets: true}) + exec := hooks.NewExecutorWithRegistry(cfg, t.TempDir(), nil, reg) + assert.False(t, exec.Has(hooks.EventPreToolUse)) + require.True(t, exec.Has(hooks.EventToolInputTransform)) + + secret := fakeGitHubPAT() + result, err := exec.Dispatch(t.Context(), hooks.EventToolInputTransform, &hooks.Input{ + ToolName: "shell", + ToolInput: map[string]any{"cmd": "echo " + secret, "cwd": "/tmp"}, + }) + require.NoError(t, err) + assert.True(t, result.Allowed) + require.NotNil(t, result.ModifiedInput) + cmd, _ := result.ModifiedInput["cmd"].(string) + assert.NotContains(t, cmd, secret) + assert.Contains(t, cmd, portcullis.Marker) + assert.Equal(t, "/tmp", result.ModifiedInput["cwd"], "untouched keys are preserved") +} diff --git a/pkg/hooks/config.go b/pkg/hooks/config.go index f41b3a4183..e5e40486f5 100644 --- a/pkg/hooks/config.go +++ b/pkg/hooks/config.go @@ -15,8 +15,9 @@ type ( // the executor at registry lookup. Hook = latest.HookDefinition // MatcherConfig pairs a tool-name regex with the hooks to run when - // it matches (used by EventPreToolUse, EventPostToolUse, and - // EventPermissionRequest). + // it matches (used by the tool-scoped events: EventPreToolUse, + // EventPostToolUse, EventPermissionRequest, EventToolInputTransform, + // EventToolGuard, and EventToolResponseTransform). MatcherConfig = latest.HookMatcherConfig ) diff --git a/pkg/hooks/contracts.go b/pkg/hooks/contracts.go new file mode 100644 index 0000000000..8ff9dcadff --- /dev/null +++ b/pkg/hooks/contracts.go @@ -0,0 +1,15 @@ +package hooks + +import "github.com/docker/docker-agent/pkg/hooks/events" + +// EventContract returns the public contract, adjusted for internal dispatch lanes. +func EventContract(event EventType) events.Contract { + if event == EventPreToolUsePreYolo { + c, _ := events.Lookup(string(EventPreToolUse)) + c.Rewrite = events.RewriteNone + c.Metadata = true + return c + } + c, _ := events.Lookup(string(event)) + return c +} diff --git a/pkg/hooks/contracts_test.go b/pkg/hooks/contracts_test.go new file mode 100644 index 0000000000..8420789fb5 --- /dev/null +++ b/pkg/hooks/contracts_test.go @@ -0,0 +1,99 @@ +package hooks + +import ( + "context" + "encoding/json" + "os" + "reflect" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/docker/docker-agent/pkg/hooks/events" +) + +func TestEventContractsMatchConfigurationAndSchema(t *testing.T) { + t.Parallel() + + data, err := os.ReadFile("../../agent-schema.json") + require.NoError(t, err) + var schema struct { + Definitions map[string]struct{ Properties map[string]json.RawMessage } + } + require.NoError(t, json.Unmarshal(data, &schema)) + contracts := map[string]events.Contract{} + for c := range events.All() { + require.NotContains(t, contracts, c.Name) + contracts[c.Name] = c + require.Contains(t, schema.Definitions["HooksConfig"].Properties, c.Name) + if c.FailClosed { + assert.True(t, c.CanBlock) + } + } + cfg := &Config{} + v := reflect.ValueOf(cfg).Elem() + require.Len(t, contracts, v.NumField()) + for i := range v.NumField() { + name, _, _ := strings.Cut(v.Type().Field(i).Tag.Get("json"), ",") + c, ok := contracts[name] + require.True(t, ok, name) + if c.ToolMatched { + v.Field(i).Set(reflect.ValueOf([]MatcherConfig{{Hooks: []Hook{{Type: HookTypeCommand, Command: "true"}}}}).Convert(v.Field(i).Type())) + } else { + v.Field(i).Set(reflect.ValueOf([]Hook{{Type: HookTypeCommand, Command: "true"}}).Convert(v.Field(i).Type())) + } + } + require.False(t, cfg.IsEmpty()) + require.NoError(t, cfg.Validate()) + exec := NewExecutor(cfg, "", nil) + for name := range contracts { + assert.True(t, exec.Has(EventType(name)), name) + } + assert.True(t, (*Config)(nil).IsEmpty()) +} + +func TestStrictOutputContractCapabilities(t *testing.T) { + t.Parallel() + + for c := range events.All() { + t.Run(c.Name, func(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name string + out *Output + allowed bool + }{ + {"block", &Output{Decision: "block"}, c.CanBlock}, + {"permission", &Output{HookSpecificOutput: &HookSpecificOutput{PermissionDecision: DecisionDeny}}, c.Permission()}, + {"input", &Output{HookSpecificOutput: &HookSpecificOutput{UpdatedInput: map[string]any{}}}, c.Rewrite == events.RewriteToolInput}, + {"response", &Output{HookSpecificOutput: &HookSpecificOutput{UpdatedToolResponse: new("")}}, c.Rewrite == events.RewriteToolResponse}, + {"context", NewAdditionalContextOutput(EventType(c.Name), "context"), c.Context}, + {"metadata", &Output{HookSpecificOutput: &HookSpecificOutput{Metadata: map[string]string{}}}, c.Metadata}, + {"summary", &Output{HookSpecificOutput: &HookSpecificOutput{Summary: "summary"}}, c.Summary}, + } { + err := validateOutput(EventType(c.Name), tc.out, true) + if tc.allowed { + assert.NoError(t, err, tc.name) + } else { + assert.Error(t, err, tc.name) + } + } + }) + } +} + +func TestEventContractsDoNotSurfaceUnusedContext(t *testing.T) { + t.Parallel() + registry := NewRegistry() + require.NoError(t, registry.RegisterBuiltin("context", func(_ context.Context, in *Input, _ []string) (*Output, error) { + return NewAdditionalContextOutput(in.HookEventName, "unused"), nil + })) + for _, event := range []EventType{EventStop, EventPostToolUse, EventBeforeLLMCall} { + exec := NewExecutorWithRegistry(configWithFlatHook(event, Hook{Type: HookTypeBuiltin, Command: "context"}), "", nil, registry) + result, err := exec.Dispatch(t.Context(), event, &Input{}) + require.NoError(t, err) + assert.Empty(t, result.AdditionalContext) + } +} diff --git a/pkg/hooks/dispatch_test.go b/pkg/hooks/dispatch_test.go index 957c427e1f..172f2aa384 100644 --- a/pkg/hooks/dispatch_test.go +++ b/pkg/hooks/dispatch_test.go @@ -21,21 +21,23 @@ var matcherWildcard = []MatcherConfig{{Matcher: "*", Hooks: trueHook}} // event is a one-line update here — the same one-line update // compileEvents needs. var onlyHooks = map[EventType]*Config{ - EventPreToolUse: {PreToolUse: matcherWildcard}, - EventPostToolUse: {PostToolUse: matcherWildcard}, - EventSessionStart: {SessionStart: trueHook}, - EventTurnStart: {TurnStart: trueHook}, - EventTurnEnd: {TurnEnd: trueHook}, - EventBeforeLLMCall: {BeforeLLMCall: trueHook}, - EventAfterLLMCall: {AfterLLMCall: trueHook}, - EventSessionEnd: {SessionEnd: trueHook}, - EventOnUserInput: {OnUserInput: trueHook}, - EventStop: {Stop: trueHook}, - EventNotification: {Notification: trueHook}, - EventOnError: {OnError: trueHook}, - EventOnMaxIterations: {OnMaxIterations: trueHook}, - EventBeforeCompaction: {BeforeCompaction: trueHook}, - EventAfterCompaction: {AfterCompaction: trueHook}, + EventPreToolUse: {PreToolUse: matcherWildcard}, + EventPostToolUse: {PostToolUse: matcherWildcard}, + EventSessionStart: {SessionStart: trueHook}, + EventTurnStart: {TurnStart: trueHook}, + EventTurnEnd: {TurnEnd: trueHook}, + EventBeforeLLMCall: {BeforeLLMCall: trueHook}, + EventAfterLLMCall: {AfterLLMCall: trueHook}, + EventSessionEnd: {SessionEnd: trueHook}, + EventOnUserInput: {OnUserInput: trueHook}, + EventStop: {Stop: trueHook}, + EventNotification: {Notification: trueHook}, + EventOnError: {OnError: trueHook}, + EventOnMaxIterations: {OnMaxIterations: trueHook}, + EventBeforeCompaction: {BeforeCompaction: trueHook}, + EventAfterCompaction: {AfterCompaction: trueHook}, + EventToolInputTransform: {ToolInputTransform: matcherWildcard}, + EventToolGuard: {ToolGuard: matcherWildcard}, } // TestExecutorHasIsGeneric exercises the generic Has API across every diff --git a/pkg/hooks/events/contracts.go b/pkg/hooks/events/contracts.go new file mode 100644 index 0000000000..ba52b42033 --- /dev/null +++ b/pkg/hooks/events/contracts.go @@ -0,0 +1,82 @@ +// Package events defines the shared contracts for hook configuration and execution. +package events + +import ( + "iter" + "slices" +) + +// Rewrite identifies the payload a sequential hook pipeline can replace. +type Rewrite uint8 + +const ( + RewriteNone Rewrite = iota + RewriteToolInput + RewriteMessages + RewriteToolResponse +) + +// Contract describes an event's configuration shape and runtime capabilities. +type Contract struct { + Name string + ToolMatched bool + CanBlock bool + FailClosed bool + Rewrite Rewrite + Decision bool + PermissionApproval bool + Metadata bool + Context bool + Instructions bool + Summary bool +} + +// Sequential reports whether each hook must receive the preceding rewrite. +func (c Contract) Sequential() bool { return c.Rewrite != RewriteNone } + +// Permission reports whether permission_decision is meaningful on this event. +func (c Contract) Permission() bool { return c.Decision || c.PermissionApproval } + +var contracts = []Contract{ + {Name: "pre_tool_use", ToolMatched: true, CanBlock: true, FailClosed: true, Rewrite: RewriteToolInput, Decision: true}, + {Name: "post_tool_use", ToolMatched: true, CanBlock: true}, + {Name: "permission_request", ToolMatched: true, CanBlock: true, PermissionApproval: true, Metadata: true}, + {Name: "session_start", Context: true, Instructions: true}, + {Name: "user_prompt_submit", CanBlock: true, Context: true}, + {Name: "user_steering_messages_submit", CanBlock: true, Context: true}, + {Name: "user_followup_submit", CanBlock: true, Context: true}, + {Name: "turn_start", Context: true, Instructions: true}, + {Name: "turn_end"}, + {Name: "before_llm_call", CanBlock: true, Rewrite: RewriteMessages}, + {Name: "after_llm_call"}, + {Name: "session_end"}, + {Name: "pre_compact", CanBlock: true, Context: true}, + {Name: "subagent_stop"}, + {Name: "on_user_input"}, + {Name: "stop"}, + {Name: "notification"}, + {Name: "on_error"}, + {Name: "on_max_iterations"}, + {Name: "on_agent_switch"}, + {Name: "on_session_resume"}, + {Name: "on_tool_approval_decision"}, + {Name: "before_compaction", CanBlock: true, Summary: true}, + {Name: "after_compaction"}, + {Name: "tool_response_transform", ToolMatched: true, Rewrite: RewriteToolResponse}, + {Name: "tool_input_transform", ToolMatched: true, CanBlock: true, Rewrite: RewriteToolInput}, + {Name: "tool_guard", ToolMatched: true, CanBlock: true, FailClosed: true, Decision: true, Metadata: true}, + {Name: "worktree_create", CanBlock: true, Context: true}, +} + +// All iterates over public event contracts in configuration order. +func All() iter.Seq[Contract] { return slices.Values(contracts) } + +// Lookup returns a contract by its public event name. +func Lookup(name string) (Contract, bool) { + for _, c := range contracts { + if c.Name == name { + return c, true + } + } + return Contract{}, false +} diff --git a/pkg/hooks/events/contracts_test.go b/pkg/hooks/events/contracts_test.go new file mode 100644 index 0000000000..34920dfea2 --- /dev/null +++ b/pkg/hooks/events/contracts_test.go @@ -0,0 +1,45 @@ +package events + +import ( + "fmt" + "os" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestDocumentedEventContracts(t *testing.T) { + t.Parallel() + data, err := os.ReadFile("../../../docs/configuration/hooks/index.md") + require.NoError(t, err) + var table strings.Builder + table.WriteString("| Event | Execution | Can block | Failure default | Context | Rewrite |\n| --- | --- | --- | --- | --- | --- |\n") + for c := range All() { + execution, block, failure, context, rewrite := "parallel", "no", "warn", "no", "—" + if c.Sequential() { + execution = "sequential" + } + if c.CanBlock { + block = "yes" + } + if c.FailClosed { + failure = "block" + } + if c.Context { + context = "yes" + } + switch c.Rewrite { + case RewriteNone: + case RewriteToolInput: + rewrite = "tool input" + case RewriteMessages: + rewrite = "messages" + case RewriteToolResponse: + rewrite = "tool response" + } + fmt.Fprintf(&table, "| `%s` | %s | %s | %s | %s | %s |\n", c.Name, execution, block, failure, context, rewrite) + } + assert.Contains(t, string(data), table.String(), "update the event-contract table when changing contracts") +} diff --git a/pkg/hooks/executor.go b/pkg/hooks/executor.go index 3e8ce0be9d..c626dc7cb3 100644 --- a/pkg/hooks/executor.go +++ b/pkg/hooks/executor.go @@ -107,6 +107,8 @@ func compileEvents(c *Config) map[EventType][]matcher { EventBeforeCompaction: flat(c.BeforeCompaction), EventAfterCompaction: flat(c.AfterCompaction), EventToolResponseTransform: compileMatchers(c.ToolResponseTransform), + EventToolInputTransform: compileMatchers(c.ToolInputTransform), + EventToolGuard: compileMatchers(c.ToolGuard), EventWorktreeCreate: flat(c.WorktreeCreate), } } @@ -213,12 +215,19 @@ func (e *Executor) Dispatch(ctx context.Context, event EventType, input *Input) if err != nil { span.RecordError(err) span.SetStatus(codes.Error, err.Error()) - return nil, fmt.Errorf("failed to serialize hook input: %w", err) + err = fmt.Errorf("failed to serialize hook input: %w", err) + if isPreApprovalEvent(event) { + // The runtime adapter maps a Dispatch error to "no opinion"; + // pre-approval events must not fall open on a payload bug. + slog.WarnContext(ctx, "Hook input serialization failed; blocking event", "event", event, "error", err) + return &Result{ExitCode: -1, Message: err.Error()}, nil + } + return nil, err } var final *Result switch event { - case EventPreToolUse, EventBeforeLLMCall, EventToolResponseTransform: + case EventPreToolUse, EventBeforeLLMCall, EventToolResponseTransform, EventToolInputTransform: final = e.runPipeline(ctx, event, hooks, *input, inputJSON) default: results := concurrent.MapSlice(hooks, func(hook Hook) hookResult { @@ -395,13 +404,43 @@ func parseStdoutJSON(stdout string) *Output { return &parsed } +// isPreToolUseLane reports whether event is one of the two pre_tool_use +// dispatch lanes, which share verdict semantics. +func isPreToolUseLane(event EventType) bool { + return event == EventPreToolUse || event == EventPreToolUsePreYolo +} + +// isPreApprovalEvent reports whether event always runs before the +// deterministic approval pipeline. A dispatch that cannot even run its +// hooks must block rather than be treated as "no opinion". +func isPreApprovalEvent(event EventType) bool { + return event == EventToolInputTransform || event == EventToolGuard || event == EventPreToolUsePreYolo +} + +// carriesDecision reports whether event's PermissionDecision verdicts +// aggregate into [Result.Decision]. +func carriesDecision(event EventType) bool { + return isPreToolUseLane(event) || event == EventToolGuard +} + +// rewritesToolInput reports whether event honours UpdatedInput. +func rewritesToolInput(event EventType) bool { + return event == EventPreToolUse || event == EventToolInputTransform +} + +// collectsMetadata reports whether event merges hook Metadata into +// [Result.Metadata] for the confirmation prompt. +func collectsMetadata(event EventType) bool { + return event == EventPermissionRequest || event == EventPreToolUsePreYolo || event == EventToolGuard +} + // failClosed reports whether a hook failure on event must deny the -// event. PreToolUse (both lanes) is a hard security boundary: a -// crashed safety hook must not silently allow the call through. -// Every other event surfaces failures as warnings unless the hook -// opts into ErrorPolicyBlock. +// event. PreToolUse (both lanes) and tool_guard are hard security +// boundaries: a crashed safety hook must not silently allow the call +// through. Every other event surfaces failures as warnings unless the +// hook opts into ErrorPolicyBlock. func failClosed(event EventType) bool { - return event == EventPreToolUse || event == EventPreToolUsePreYolo + return isPreToolUseLane(event) || event == EventToolGuard } // stdoutAsContext reports whether plain stdout (non-JSON, exit 0) @@ -443,7 +482,7 @@ func aggregate(results []hookResult, event EventType) *Result { final.Allowed = false final.ExitCode = -1 final.Stderr = r.Stderr - messages = append(messages, fmt.Sprintf("PreToolUse hook failed to execute: %v", r.err)) + messages = append(messages, hookFailureMessage(event, r.err)) } else if policy != ErrorPolicyIgnore { slog.Warn("Hook execution error", "hook", r.hook.DisplayName(), "error", r.err) } @@ -488,13 +527,13 @@ func aggregate(results []hookResult, event EventType) *Result { sysMsgs = append(sysMsgs, out.SystemMessage) } if hso := out.HookSpecificOutput; hso != nil { - if (event == EventPreToolUse || event == EventPreToolUsePreYolo) && hso.PermissionDecision != "" { + if carriesDecision(event) && hso.PermissionDecision != "" { final.Decision, final.DecisionReason = strongerDecision( final.Decision, final.DecisionReason, hso.PermissionDecision, hso.PermissionDecisionReason, ) } - if event == EventPreToolUse || event == EventPreToolUsePreYolo || event == EventPermissionRequest { + if carriesDecision(event) || event == EventPermissionRequest { switch hso.PermissionDecision { case DecisionDeny: final.Allowed = false @@ -510,7 +549,7 @@ func aggregate(results []hookResult, event EventType) *Result { } } } - if event == EventPreToolUse && hso.UpdatedInput != nil { + if rewritesToolInput(event) && hso.UpdatedInput != nil { if final.ModifiedInput == nil { final.ModifiedInput = make(map[string]any) } @@ -535,7 +574,7 @@ func aggregate(results []hookResult, event EventType) *Result { if event == EventToolResponseTransform && hso.UpdatedToolResponse != nil { final.UpdatedToolResponse = hso.UpdatedToolResponse } - if (event == EventPermissionRequest || event == EventPreToolUsePreYolo) && len(hso.Metadata) > 0 { + if collectsMetadata(event) && len(hso.Metadata) > 0 { // Metadata from every matching hook is merged so multiple // hooks can each contribute keys. On a key clash the last // hook in config order wins (results is iterated in @@ -558,6 +597,15 @@ func aggregate(results []hookResult, event EventType) *Result { return final } +// hookFailureMessage keeps the historical wording for pre_tool_use +// lanes and names the event otherwise. +func hookFailureMessage(event EventType, err error) string { + if isPreToolUseLane(event) { + return fmt.Sprintf("PreToolUse hook failed to execute: %v", err) + } + return fmt.Sprintf("%s hook failed to execute: %v", event, err) +} + // decisionWeight ranks PermissionDecision verdicts so [strongerDecision] // can pick the most-restrictive across a chain of pre_tool_use hooks. // Deny > Ask > Allow > "" (no decision). diff --git a/pkg/hooks/hooks_test.go b/pkg/hooks/hooks_test.go index 74df2c83f8..eb3eaa6b1a 100644 --- a/pkg/hooks/hooks_test.go +++ b/pkg/hooks/hooks_test.go @@ -131,6 +131,20 @@ func TestConfigIsEmpty(t *testing.T) { }, expected: false, }, + { + name: "with tool_input_transform", + config: Config{ + ToolInputTransform: []MatcherConfig{{Matcher: "*"}}, + }, + expected: false, + }, + { + name: "with tool_guard", + config: Config{ + ToolGuard: []MatcherConfig{{Matcher: "*"}}, + }, + expected: false, + }, } for _, tt := range tests { diff --git a/pkg/hooks/pipeline.go b/pkg/hooks/pipeline.go index c6bc41b84d..f87f03ef2a 100644 --- a/pkg/hooks/pipeline.go +++ b/pkg/hooks/pipeline.go @@ -42,7 +42,7 @@ func (e *Executor) runPipeline(ctx context.Context, event EventType, hooks []Hoo func rewrittenInput(input Input, event EventType, out *HookSpecificOutput) (Input, bool) { switch event { - case EventPreToolUse: + case EventPreToolUse, EventToolInputTransform: if out.UpdatedInput == nil { return input, false } diff --git a/pkg/hooks/pipeline_test.go b/pkg/hooks/pipeline_test.go index 40590a171b..504bafb8d6 100644 --- a/pkg/hooks/pipeline_test.go +++ b/pkg/hooks/pipeline_test.go @@ -133,7 +133,7 @@ func TestPipelineToolInputEmptyPatch(t *testing.T) { func TestPipelineNoRewrite(t *testing.T) { t.Parallel() - for _, event := range []EventType{EventPreToolUse, EventBeforeLLMCall, EventToolResponseTransform} { + for _, event := range []EventType{EventPreToolUse, EventBeforeLLMCall, EventToolResponseTransform, EventToolInputTransform} { t.Run(string(event), func(t *testing.T) { t.Parallel() exec := pipelineTestExecutor(t, event, []Hook{{Type: HookTypeCommand, Command: "echo '{}'"}}, NewRegistry()) @@ -149,7 +149,7 @@ func TestPipelineNoRewrite(t *testing.T) { func TestPipelineFailuresKeepPriorRewriteAndRunRemainingHooks(t *testing.T) { t.Parallel() - for _, event := range []EventType{EventPreToolUse, EventBeforeLLMCall, EventToolResponseTransform} { + for _, event := range []EventType{EventPreToolUse, EventBeforeLLMCall, EventToolResponseTransform, EventToolInputTransform} { for _, tc := range []struct { name string result HandlerResult @@ -199,7 +199,7 @@ func TestPipelineFailuresKeepPriorRewriteAndRunRemainingHooks(t *testing.T) { assert.True(t, lastRan) assert.Equal(t, !tc.blocked, result.Allowed) switch event { - case EventPreToolUse: + case EventPreToolUse, EventToolInputTransform: assert.Equal(t, "first", result.ModifiedInput["cmd"]) case EventBeforeLLMCall: require.Len(t, result.UpdatedMessages, 1) @@ -241,7 +241,7 @@ func TestPipelineInvalidRewritePreservesDenyAndInput(t *testing.T) { func TestNonTransformEventsRemainConcurrent(t *testing.T) { t.Parallel() - for _, event := range []EventType{EventSessionStart, EventPermissionRequest, EventBeforeCompaction, EventPreToolUsePreYolo} { + for _, event := range []EventType{EventSessionStart, EventPermissionRequest, EventBeforeCompaction, EventPreToolUsePreYolo, EventToolGuard} { t.Run(string(event), func(t *testing.T) { t.Parallel() started := make(chan struct{}, 2) @@ -279,6 +279,7 @@ func TestNonTransformEventsRemainConcurrent(t *testing.T) { exec := NewExecutorWithRegistry(&Config{ SessionStart: hookList, BeforeCompaction: hookList, PermissionRequest: []MatcherConfig{{Hooks: hookList}}, + ToolGuard: []MatcherConfig{{Hooks: hookList}}, PreToolUse: []MatcherConfig{{PreemptYolo: &preempt, Hooks: hookList}}, }, t.TempDir(), nil, registry) result, err := exec.Dispatch(ctx, event, &Input{ToolInput: map[string]any{"cmd": "original"}}) @@ -309,6 +310,10 @@ func pipelineTestExecutor(t *testing.T, event EventType, hookList []Hook, regist cfg.BeforeLLMCall = hookList case EventToolResponseTransform: cfg.ToolResponseTransform = []MatcherConfig{{Hooks: hookList}} + case EventToolInputTransform: + cfg.ToolInputTransform = []MatcherConfig{{Hooks: hookList}} + case EventToolGuard: + cfg.ToolGuard = []MatcherConfig{{Hooks: hookList}} default: t.Fatalf("unexpected event: %s", event) } diff --git a/pkg/hooks/protocol.go b/pkg/hooks/protocol.go new file mode 100644 index 0000000000..5e832fbceb --- /dev/null +++ b/pkg/hooks/protocol.go @@ -0,0 +1,84 @@ +package hooks + +import ( + "encoding/json" + "errors" + "fmt" + "io" + "strings" + + "github.com/docker/docker-agent/pkg/hooks/events" +) + +func parseStdoutJSON(stdout string, strict bool) (*Output, error) { + s := strings.TrimSpace(stdout) + if s == "" { + return nil, nil + } + if !strings.HasPrefix(s, "{") { + if strict { + return nil, errors.New("strict_output requires a JSON object or empty stdout") + } + return nil, nil + } + var parsed Output + decoder := json.NewDecoder(strings.NewReader(s)) + if strict { + decoder.DisallowUnknownFields() + } + if err := decoder.Decode(&parsed); err != nil { + return nil, fmt.Errorf("invalid hook output: %w", err) + } + if err := decoder.Decode(new(any)); err != io.EOF { + return nil, errors.New("hook output must contain a single JSON object") + } + return &parsed, nil +} + +// validateOutput checks verdicts for every hook and capabilities for strict hooks. +func validateOutput(event EventType, out *Output, strict bool) error { + c := EventContract(event) + if strict && out.SuppressOutput { + return errors.New("suppress_output is not supported; use stderr for diagnostics") + } + if out.Decision != "" && out.Decision != DecisionBlockValue { + return fmt.Errorf("invalid decision %q: expected block", out.Decision) + } + if strict && !c.CanBlock && (!out.ShouldContinue() || out.IsBlocked()) { + return fmt.Errorf("%s does not support blocking output", c.Name) + } + hso := out.HookSpecificOutput + if hso == nil { + return nil + } + if strict && hso.HookEventName != "" && string(hso.HookEventName) != c.Name { + return fmt.Errorf("output hook_event_name %q does not match %s", hso.HookEventName, c.Name) + } + switch hso.PermissionDecision { + case "", DecisionAllow, DecisionAsk, DecisionDeny: + default: + return fmt.Errorf("invalid permission_decision %q: expected allow, ask, or deny", hso.PermissionDecision) + } + if !strict { + return nil + } + for _, field := range []struct { + name string + present bool + supported bool + }{ + {"permission_decision", hso.PermissionDecision != "" || hso.PermissionDecisionReason != "", c.Permission()}, + {"updated_input", hso.UpdatedInput != nil, c.Rewrite == events.RewriteToolInput}, + {"updated_messages", hso.UpdatedMessages != nil, c.Rewrite == events.RewriteMessages}, + {"updated_tool_response", hso.UpdatedToolResponse != nil, c.Rewrite == events.RewriteToolResponse}, + {"metadata", hso.Metadata != nil, c.Metadata}, + {"additional_context", hso.AdditionalContext != "", c.Context}, + {"instruction_context", hso.InstructionContext != nil, c.Instructions}, + {"summary", hso.Summary != "", c.Summary}, + } { + if field.present && !field.supported { + return fmt.Errorf("%s does not support %s", c.Name, field.name) + } + } + return nil +} diff --git a/pkg/hooks/tool_phases_test.go b/pkg/hooks/tool_phases_test.go new file mode 100644 index 0000000000..df6c41ad4f --- /dev/null +++ b/pkg/hooks/tool_phases_test.go @@ -0,0 +1,227 @@ +package hooks + +import ( + "context" + "errors" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// failingHandlerRegistry returns a registry whose "fail" hook type +// reports the given handler outcome. +func failingHandlerRegistry(res HandlerResult, err error) *Registry { + registry := NewRegistry() + registry.Register("fail", func(HandlerEnv, Hook) (Handler, error) { + return pipelineHandlerFunc(func(context.Context, []byte) (HandlerResult, error) { + return res, err + }), nil + }) + return registry +} + +func TestToolInputTransformComposesPatchesAndKeepsInputEventName(t *testing.T) { + t.Parallel() + + registry := NewRegistry() + require.NoError(t, registry.RegisterBuiltin("append", func(_ context.Context, in *Input, args []string) (*Output, error) { + assert.Equal(t, EventToolInputTransform, in.HookEventName) + return &Output{HookSpecificOutput: &HookSpecificOutput{ + UpdatedInput: map[string]any{"cmd": in.ToolInput["cmd"].(string) + args[0]}, + }}, nil + })) + exec := NewExecutorWithRegistry(&Config{ToolInputTransform: []MatcherConfig{ + {Matcher: "other", Hooks: []Hook{{Type: HookTypeCommand, Command: "exit 2"}}}, + {Matcher: "shell", Hooks: []Hook{ + {Type: HookTypeCommand, Command: `echo '{"hook_specific_output":{"updated_input":{"cmd":"ls"}}}'`}, + {Type: HookTypeBuiltin, Command: "append", Args: []string{" -l"}}, + }}, + {Matcher: "*", Hooks: []Hook{{Type: HookTypeBuiltin, Command: "append", Args: []string{" -h"}}}}, + }}, t.TempDir(), nil, registry) + original := map[string]any{"cmd": "original", "cwd": "work"} + + result, err := exec.Dispatch(t.Context(), EventToolInputTransform, &Input{ToolName: "shell", ToolInput: original}) + require.NoError(t, err) + assert.True(t, result.Allowed) + assert.Empty(t, result.Decision, "transform never carries a verdict") + assert.Equal(t, map[string]any{"cmd": "ls -l -h", "cwd": "work"}, result.ModifiedInput) + assert.Equal(t, "original", original["cmd"], "caller's input is not mutated") +} + +// Transform failures follow on_error (default warn); exit-code semantics +// are unchanged from every other event. +func TestToolInputTransformFailurePolicy(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + name string + result HandlerResult + err error + onError string + allowed bool + exitCode int + }{ + {name: "error default warn", err: errors.New("boom"), allowed: true}, + {name: "error ignore", err: errors.New("boom"), onError: "ignore", allowed: true}, + {name: "error block", err: errors.New("boom"), onError: "block", allowed: false, exitCode: -1}, + {name: "exit 1", result: HandlerResult{ExitCode: 1}, allowed: true}, + {name: "exit 2", result: HandlerResult{ExitCode: 2, Stderr: "nope"}, allowed: false, exitCode: 2}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + exec := pipelineTestExecutor(t, EventToolInputTransform, []Hook{ + {Type: HookTypeCommand, Command: `echo '{"hook_specific_output":{"updated_input":{"cmd":"first"}}}'`}, + {Type: "fail", Command: "fail", OnError: tc.onError}, + }, failingHandlerRegistry(tc.result, tc.err)) + + result, err := exec.Dispatch(t.Context(), EventToolInputTransform, &Input{ToolInput: map[string]any{"cmd": "original"}}) + require.NoError(t, err) + assert.Equal(t, tc.allowed, result.Allowed) + assert.Equal(t, tc.exitCode, result.ExitCode) + assert.Equal(t, "first", result.ModifiedInput["cmd"], "earlier rewrite survives the failure") + if tc.onError == "block" { + assert.Contains(t, result.Message, "tool_input_transform hook failed to execute") + } + }) + } +} + +func TestToolGuardAggregatesMostRestrictiveVerdict(t *testing.T) { + t.Parallel() + + guard := func(decision Decision, reason, meta string) Hook { + return Hook{Type: HookTypeBuiltin, Command: "verdict", Args: []string{string(decision), reason, meta}} + } + registry := NewRegistry() + require.NoError(t, registry.RegisterBuiltin("verdict", func(_ context.Context, in *Input, args []string) (*Output, error) { + assert.Equal(t, EventToolGuard, in.HookEventName) + assert.Equal(t, "original", in.ToolInput["cmd"], "guards see the same input; no pipeline") + var meta map[string]string + if args[2] != "" { + meta = map[string]string{"from_" + args[0]: args[2], "shared": args[2]} + } + return &Output{HookSpecificOutput: &HookSpecificOutput{ + PermissionDecision: Decision(args[0]), + PermissionDecisionReason: args[1], + Metadata: meta, + UpdatedInput: map[string]any{"cmd": "rewritten by " + args[0]}, + }}, nil + })) + + for _, tc := range []struct { + name string + hooks []Hook + wantVerdict Decision + wantReason string + wantAllowed bool + }{ + { + name: "allow is advisory", + hooks: []Hook{guard(DecisionAllow, "fine", "")}, + wantVerdict: DecisionAllow, wantReason: "fine", wantAllowed: true, + }, + { + name: "ask beats allow and keeps Allowed", + hooks: []Hook{guard(DecisionAllow, "fine", ""), guard(DecisionAsk, "unsure", "")}, + wantVerdict: DecisionAsk, wantReason: "unsure", wantAllowed: true, + }, + { + name: "deny beats ask and denies", + hooks: []Hook{guard(DecisionAsk, "unsure", ""), guard(DecisionDeny, "destructive", ""), guard(DecisionAllow, "fine", "")}, + wantVerdict: DecisionDeny, wantReason: "destructive", wantAllowed: false, + }, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + exec := pipelineTestExecutor(t, EventToolGuard, tc.hooks, registry) + result, err := exec.Dispatch(t.Context(), EventToolGuard, &Input{ToolName: "shell", ToolInput: map[string]any{"cmd": "original"}}) + require.NoError(t, err) + assert.Equal(t, tc.wantVerdict, result.Decision) + assert.Equal(t, tc.wantReason, result.DecisionReason) + assert.Equal(t, tc.wantAllowed, result.Allowed) + assert.False(t, result.PermissionAllowed, "guard allow never auto-approves") + assert.Nil(t, result.ModifiedInput, "guards cannot rewrite input") + if tc.wantVerdict == DecisionDeny { + assert.Contains(t, result.Message, "destructive") + } + }) + } + + t.Run("metadata merges last wins", func(t *testing.T) { + t.Parallel() + exec := pipelineTestExecutor(t, EventToolGuard, []Hook{ + guard(DecisionAsk, "a", "one"), + guard(DecisionAllow, "b", "two"), + }, registry) + result, err := exec.Dispatch(t.Context(), EventToolGuard, &Input{ToolName: "shell", ToolInput: map[string]any{"cmd": "original"}}) + require.NoError(t, err) + assert.Equal(t, map[string]string{"from_ask": "one", "from_allow": "two", "shared": "two"}, result.Metadata) + }) +} + +// Guard failures fail closed regardless of on_error; non-blocking exit +// codes keep their legacy meaning. +func TestToolGuardFailsClosed(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + name string + result HandlerResult + err error + onError string + allowed bool + exitCode int + }{ + {name: "error default", err: errors.New("boom"), allowed: false, exitCode: -1}, + {name: "error ignore still denies", err: errors.New("boom"), onError: "ignore", allowed: false, exitCode: -1}, + {name: "canceled", err: context.Canceled, allowed: false, exitCode: -1}, + {name: "exit 1", result: HandlerResult{ExitCode: 1}, allowed: true}, + {name: "exit 2", result: HandlerResult{ExitCode: 2, Stderr: "nope"}, allowed: false, exitCode: 2}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + exec := pipelineTestExecutor(t, EventToolGuard, []Hook{ + {Type: HookTypeCommand, Command: `echo '{"hook_specific_output":{"permission_decision":"allow","permission_decision_reason":"fine"}}'`}, + {Type: "fail", Command: "fail", OnError: tc.onError}, + }, failingHandlerRegistry(tc.result, tc.err)) + + result, err := exec.Dispatch(t.Context(), EventToolGuard, &Input{ToolName: "shell"}) + require.NoError(t, err) + assert.Equal(t, tc.allowed, result.Allowed) + assert.Equal(t, tc.exitCode, result.ExitCode) + assert.Equal(t, DecisionAllow, result.Decision, "other hooks' verdicts still aggregate") + if tc.err != nil { + assert.Contains(t, result.Message, "tool_guard hook failed to execute") + } + }) + } +} + +// A payload the executor cannot serialize must block pre-approval events +// (the runtime adapter treats a Dispatch error as "no opinion") while +// legacy events keep returning the error. +func TestDispatchSerializationFailureBlocksPreApprovalEvents(t *testing.T) { + t.Parallel() + + cfg := &Config{ + PreToolUse: []MatcherConfig{{Hooks: trueHook}, {Hooks: trueHook, PreemptYolo: new(true)}}, + ToolInputTransform: []MatcherConfig{{Hooks: trueHook}}, + ToolGuard: []MatcherConfig{{Hooks: trueHook}}, + } + exec := NewExecutor(cfg, t.TempDir(), nil) + unserializable := map[string]any{"bad": make(chan int)} + + for _, event := range []EventType{EventToolInputTransform, EventToolGuard, EventPreToolUsePreYolo} { + result, err := exec.Dispatch(t.Context(), event, &Input{ToolName: "shell", ToolInput: unserializable}) + require.NoError(t, err, event) + require.NotNil(t, result, event) + assert.False(t, result.Allowed, event) + assert.Equal(t, -1, result.ExitCode, event) + assert.Contains(t, result.Message, "failed to serialize hook input", event) + } + + result, err := exec.Dispatch(t.Context(), EventPreToolUse, &Input{ToolName: "shell", ToolInput: unserializable}) + require.Error(t, err) + assert.Nil(t, result) +} diff --git a/pkg/hooks/types.go b/pkg/hooks/types.go index f31b5833b4..9b0707029d 100644 --- a/pkg/hooks/types.go +++ b/pkg/hooks/types.go @@ -190,6 +190,32 @@ const ( // Tool-scoped: matchers select which tools the hook runs against, // like pre_tool_use / post_tool_use. EventToolResponseTransform EventType = "tool_response_transform" + // EventToolInputTransform fires before every tool call, ahead of the + // deterministic approval pipeline (--yolo, permission patterns, + // safety mode) and of tool_guard. Hooks run sequentially, each + // seeing the preceding rewrite, and may patch tool arguments via + // [HookSpecificOutput.UpdatedInput] exactly like pre_tool_use. Its + // only job is rewriting: verdicts belong on tool_guard. Failures + // follow the hook's on_error policy (default warn) — the runtime + // honours an explicit block. The redact_secrets builtin's + // argument-scrubbing leg is auto-injected here. + // + // Tool-scoped: matchers select which tools the hook runs against. + EventToolInputTransform EventType = "tool_input_transform" + // EventToolGuard fires after tool_input_transform and before the + // deterministic approval pipeline, so its verdict cannot be bypassed + // by --yolo or permission allow-rules. Hooks run concurrently and + // their [HookSpecificOutput.PermissionDecision] verdicts aggregate to + // the most restrictive (Deny > Ask > Allow) in [Result.Decision]. + // Deny is terminal; Ask forces the user prompt even when the session + // already allowed the tool; Allow is advisory (the pipeline still + // runs). [HookSpecificOutput.Metadata] is merged into + // [Result.Metadata] for the confirmation prompt. UpdatedInput is + // ignored — use tool_input_transform to rewrite arguments. Hook + // execution failures fail closed like pre_tool_use. + // + // Tool-scoped: matchers select which tools the hook runs against. + EventToolGuard EventType = "tool_guard" // EventWorktreeCreate fires once, just after the CLI creates a git // worktree for a `--worktree` run and before the session starts. The // new working directory is reported in [Input.Cwd] (hooks run there) @@ -280,8 +306,9 @@ type Input struct { LastUserMessage string `json:"last_user_message,omitempty"` // Tool-related fields (PreToolUse, PostToolUse, PermissionRequest, - // ToolResponseTransform). ToolCategory identifies the dispatching tool's - // category for builtins that target whole toolsets. + // ToolInputTransform, ToolGuard, ToolResponseTransform). ToolCategory + // identifies the dispatching tool's category for builtins that target + // whole toolsets. ToolCategory string `json:"tool_category,omitempty"` ToolName string `json:"tool_name,omitempty"` ToolUseID string `json:"tool_use_id,omitempty"` @@ -583,9 +610,9 @@ type HookSpecificOutput struct { UpdatedToolResponse *string `json:"updated_tool_response,omitempty"` // Metadata is a set of key/value annotations a - // [EventPermissionRequest] hook contributes to the tool-call - // confirmation prompt. The runtime merges it onto the tool's own - // metadata before emitting the confirmation event, so clients (TUI, + // [EventPermissionRequest] or [EventToolGuard] hook contributes to + // the tool-call confirmation prompt. The runtime merges it onto the + // tool's own metadata before emitting the confirmation event, so clients (TUI, // HTTP) can render extra per-call context. Keys from multiple hooks // are merged; on a key clash the last hook in config order wins (see // aggregate()). Ignored on every other event. @@ -602,8 +629,8 @@ type Result struct { PermissionAllowed bool // Message is feedback to include in the response. Message string - // ModifiedInput is the complete tool input after applying PreToolUse patches. - // Nil means no hook supplied a patch. + // ModifiedInput is the complete tool input after applying PreToolUse + // or ToolInputTransform patches. Nil means no hook supplied a patch. ModifiedInput map[string]any // AdditionalContext is context added by the hooks. AdditionalContext string @@ -633,21 +660,21 @@ type Result struct { UpdatedToolResponse *string // Metadata aggregates the key/value annotations contributed by - // [EventPermissionRequest] hooks. The runtime merges it onto the - // tool's own metadata when emitting the tool-call confirmation - // event. nil when no hook supplied any. + // [EventPermissionRequest] and [EventToolGuard] hooks. The runtime + // merges it onto the tool's own metadata when emitting the tool-call + // confirmation event. nil when no hook supplied any. Metadata map[string]string - // Decision is the most-restrictive PreToolUse verdict reported by - // any matching hook in the chain ("" when no hook produced one). - // Most-restrictive ordering: Deny > Ask > Allow > "". + // Decision is the most-restrictive PreToolUse or ToolGuard verdict + // reported by any matching hook in the chain ("" when no hook + // produced one). Most-restrictive ordering: Deny > Ask > Allow > "". // // The runtime's tool-approval flow consults this BEFORE asking the // user, so an LLM-judge hook that returns Allow can auto-approve a // call that would otherwise prompt, and Ask can force a prompt for // a call that would otherwise auto-run. // - // Always empty for non-PreToolUse events. + // Always empty for events other than pre_tool_use and tool_guard. Decision Decision // DecisionReason is the human-readable rationale paired with // Decision (the reason from the most-restrictive hook). Empty when diff --git a/pkg/runtime/hooks.go b/pkg/runtime/hooks.go index 6e53d88760..b4d7854c58 100644 --- a/pkg/runtime/hooks.go +++ b/pkg/runtime/hooks.go @@ -483,6 +483,8 @@ const ( ApprovalSourceTeamPermissionsDeny = "team_permissions_deny" ApprovalSourcePreToolUseHookAllow = "pre_tool_use_hook_allow" ApprovalSourcePreToolUseHookDeny = "pre_tool_use_hook_deny" + ApprovalSourceToolInputTransformDeny = toolexec.ApprovalSourceToolInputTransformDeny + ApprovalSourceToolGuardDeny = toolexec.ApprovalSourceToolGuardDeny ApprovalSourceReadOnlyHint = "readonly_hint" ApprovalSourceModeBalanced = "mode_balanced" ApprovalSourceModeRestricted = "mode_restricted" diff --git a/pkg/runtime/runtime.go b/pkg/runtime/runtime.go index b378643374..8c199e206b 100644 --- a/pkg/runtime/runtime.go +++ b/pkg/runtime/runtime.go @@ -722,7 +722,7 @@ func NewLocalRuntime(ctx context.Context, agents *team.Team, opts ...Opt) (*Loca // redact_secrets used to live here as a sibling [MessageTransform]; // it now ships entirely as a [hooks.BuiltinFunc] in // pkg/hooks/builtins/redact_secrets.go and is wired into all three - // of pre_tool_use, before_llm_call, and tool_response_transform via + // of tool_input_transform, before_llm_call, and tool_response_transform via // [builtins.ApplyAgentDefaults] (or a user's hooks YAML directly), // so the rewrite path is the same for every leak vector and there // is no flag-only code path to keep in sync. diff --git a/pkg/runtime/toolexec/confirm.go b/pkg/runtime/toolexec/confirm.go index 63430cb457..7c2196ed5d 100644 --- a/pkg/runtime/toolexec/confirm.go +++ b/pkg/runtime/toolexec/confirm.go @@ -95,7 +95,7 @@ func (g *Gate) ConfirmAndRun(ctx context.Context, run tools.ConfirmedRun, exec f return "", tools.ErrConfirmationDenied } - // A pre_tool_use hook may rewrite the action. For skill command expansion, + // Input hooks may rewrite the action. For skill command expansion, // execute the approved command rather than the original closure's command. approvedRun := tools.ConfirmedRun{ ToolName: run.ToolName, diff --git a/pkg/runtime/toolexec/dispatcher.go b/pkg/runtime/toolexec/dispatcher.go index 877161314b..849f970b4c 100644 --- a/pkg/runtime/toolexec/dispatcher.go +++ b/pkg/runtime/toolexec/dispatcher.go @@ -1,6 +1,7 @@ package toolexec import ( + "bytes" "cmp" "context" "encoding/json" @@ -46,6 +47,8 @@ const ( ApprovalSourceTeamPermissionsDeny = "team_permissions_deny" ApprovalSourcePreToolUseHookAllow = "pre_tool_use_hook_allow" ApprovalSourcePreToolUseHookDeny = "pre_tool_use_hook_deny" + ApprovalSourceToolInputTransformDeny = "tool_input_transform_deny" + ApprovalSourceToolGuardDeny = "tool_guard_deny" ApprovalSourcePermissionRequestHookDeny = "permission_request_hook_deny" ApprovalSourcePermissionRequestHookAllow = "permission_request_hook_allow" // ApprovalSourceReadOnlyHint marks the legacy default's @@ -113,7 +116,8 @@ type HookDispatcher interface { // Dispatch fires a tool-related hook (typically [hooks.EventPreToolUse] // or [hooks.EventPostToolUse]). Returning nil is the "carry on with the // original call" signal — used uniformly when no hook is configured, - // the agent is missing, or dispatch failed. + // or the agent is missing. Pre-approval dispatch failures must return a + // blocking result, not nil. Dispatch(ctx context.Context, a *agent.Agent, event hooks.EventType, in *hooks.Input) *hooks.Result // NotifyUserInput is invoked just before the dispatcher blocks waiting @@ -328,6 +332,11 @@ type call struct { preYoloComputed bool preYoloResult *hooks.Result + guardComputed bool + guardResult *hooks.Result + // A new ask after a legacy rewrite cannot be bypassed by earlier grants. + rewrittenInputAsk bool + // Safety-label cache: the classifier result is stable for the // call, and permissionDecision + confirmationMetadata + // notifyApproval all consume it. @@ -423,56 +432,15 @@ func (c *call) run(ctx context.Context) CallOutcome { // approveAndRun runs runTool if the configured approval pipeline allows // it, otherwise records an error or asks the user. // -// The pipeline order is: -// -// 0. pre_tool_use entries with preempt_yolo:true — user-authored -// security hooks that fire BEFORE the deterministic pipeline so a -// Deny or Ask verdict here cannot be bypassed by any safety mode -// (including Autonomous) or permission allow rules. Allow / -// no-opinion fall through. -// 1. [Decide] — custom Deny/Allow/Ask rules win outright; otherwise -// the (safety mode × safety label) table produces the verdict. -// An explicit ask rule goes straight to the user. -// 2. pre_tool_use hooks (LLM-judge, shell scripts, ...) — the -// default lane, consulted ONLY when the mode said Ask. The hook -// can Deny (block), Allow (skip the user prompt) or Ask (force -// the prompt). Hooks may also rewrite tool arguments via -// UpdatedInput, in which case the rewrite is applied here so the -// user prompt and the tool handler both see the modified call. -// 3. legacy read-only auto-approve — sessions that never chose a -// safety mode keep the pre-modes contract: read-only-annotated -// tools run without prompting. Placed after the hook chain so an -// LLM judge still gets a turn on those calls. -// 4. user confirmation — fallback prompt. +// Order: input transforms → mandatory guards → legacy preempt hooks → +// permission rules / safety mode → legacy approval hooks → user confirmation. +// Approval hooks only run when the safety mode asks, not on auto-approved calls. func (c *call) approveAndRun(ctx context.Context, runTool func() CallOutcome) CallOutcome { - // Stage 0: pre_tool_use entries flagged with preempt_yolo:true. - if r := c.consultPreToolUsePreYolo(ctx); r != nil { - switch r.Decision { - case hooks.DecisionDeny: - slog.DebugContext(ctx, "Tool denied by preempt-yolo pre_tool_use hook", "tool", c.tc.Function.Name, "session_id", c.sess.ID, "reason", r.DecisionReason) - c.notifyApproval(ctx, ApprovalDecisionDeny, ApprovalSourcePreToolUseHookDeny) - rejectMsg := "The tool call was rejected by a pre_tool_use hook." - if reason := strings.TrimSpace(r.DecisionReason); reason != "" { - rejectMsg += " Reason: " + reason - } - c.errorResponse(ctx, rejectMsg) - return CallOutcome{} - case hooks.DecisionAsk: - // A session-scoped allow grant (the interactive "T = always - // allow this tool" decision, stored in sess.Permissions) is an - // informed opt-in the user made in response to this very safety - // prompt. Honor it instead of asking again, otherwise "always - // allow" would re-prompt on every matching call. The safety - // mode and the team/config permission layer stay subordinate - // to the preempt-yolo verdict — see [call.sessionPermissionsAllow]. - if c.sessionPermissionsAllow() { - slog.DebugContext(ctx, "preempt-yolo Ask overridden by session permission allow", "tool", c.tc.Function.Name, "session_id", c.sess.ID) - c.notifyApproval(ctx, ApprovalDecisionAllow, ApprovalSourceSessionPermissionsAllow) - return runTool() - } - return c.askUser(ctx, runTool) - } - // DecisionAllow / "" → advisory; fall through to Decide(). + if c.transformToolInput(ctx) { + return CallOutcome{} + } + if outcome, handled := c.runToolGuards(ctx, runTool); handled { + return outcome } // Stage 1: custom rules + (mode × label) table. @@ -547,6 +515,9 @@ func (c *call) permissionArgs() map[string]any { } func (c *call) autoApprovalAfterConfirmationWait() (PermissionDecision, bool) { + if c.mandatoryAsk() { + return PermissionDecision{}, false + } if c.preYoloResult != nil && c.preYoloResult.Decision == hooks.DecisionAsk { // Even under a preempt-yolo Ask, a session-scoped allow grant that // landed while we were blocked on the resume channel (e.g. a @@ -658,8 +629,7 @@ func (c *call) consultPreToolUsePreYolo(ctx context.Context) *hooks.Result { // // UpdatedInput from a hook is applied to c.tc here so every downstream // path (auto-run, user prompt, runToolset) sees the rewritten -// arguments — this is the only place pre-call argument rewriting -// happens. +// arguments. Mandatory guards and rules are rechecked after a rewrite. func (c *call) consultPreToolUseHook(ctx context.Context, runTool func() CallOutcome) (CallOutcome, bool) { if c.d.Hooks == nil { return CallOutcome{}, false @@ -671,7 +641,10 @@ func (c *call) consultPreToolUseHook(ctx context.Context, runTool func() CallOut } // Apply UpdatedInput first so subsequent paths see the rewritten args. - c.applyHookModifiedInput(result) + changed, err := c.applyHookModifiedInput(result) + if err != nil { + slog.WarnContext(ctx, "Failed to marshal modified tool input from hook", "tool", c.tc.Function.Name, "error", err) + } if !result.Allowed { slog.DebugContext(ctx, "Pre-tool hook blocked tool call", "tool", c.tc.Function.Name, "message", result.Message) @@ -681,6 +654,15 @@ func (c *call) consultPreToolUseHook(ctx context.Context, runTool func() CallOut return CallOutcome{}, true } + if changed { + c.rewrittenInputAsk = result.Decision == hooks.DecisionAsk + // Legacy hooks can rewrite after approval checks. Recheck the actual + // arguments once, without rerunning transforms or approval helpers. + if outcome, handled := c.recheckRewrittenInput(ctx, runTool); handled { + return outcome, true + } + } + switch result.Decision { case hooks.DecisionAllow: slog.DebugContext(ctx, "Tool auto-approved by pre_tool_use hook", "tool", c.tc.Function.Name, "reason", result.DecisionReason, "session_id", c.sess.ID) @@ -693,23 +675,24 @@ func (c *call) consultPreToolUseHook(ctx context.Context, runTool func() CallOut return CallOutcome{}, false } -// applyHookModifiedInput applies a hook's UpdatedInput to the in-flight -// tool call. Errors are logged at warn level and otherwise ignored — -// the hook can't crash the call by returning malformed JSON. -func (c *call) applyHookModifiedInput(result *hooks.Result) { +// applyHookModifiedInput replaces arguments with the executor's complete patched input. +func (c *call) applyHookModifiedInput(result *hooks.Result) (bool, error) { if result.ModifiedInput == nil { - return + return false, nil } updated, err := json.Marshal(result.ModifiedInput) if err != nil { - slog.Warn("Failed to marshal modified tool input from hook", "tool", c.tc.Function.Name, "error", err) - return + return false, err + } + // Compare canonical JSON so formatting-only changes don't rerun guards. + original, err := json.Marshal(ParseToolInput(c.tc.Function.Arguments)) + if err == nil && bytes.Equal(original, updated) { + c.tc.Function.Arguments = string(updated) + return false, nil } - slog.Debug("Pre-tool hook modified tool input", "tool", c.tc.Function.Name) c.tc.Function.Arguments = string(updated) - // The rewrite may change the shell command; drop the cached label - // so the confirmation prompt classifies what will actually run. c.labelComputed = false + return true, nil } // notifyApproval forwards the resolved approval decision to the @@ -807,7 +790,7 @@ func (c *call) confirmationMutex() *sync.Mutex { // on the resume channel or for ctx cancellation. Only called when no // permission rule auto-approved the tool. // -// permission_request hooks fire first and may short-circuit the prompt +// permission_request hooks fire first unless a mandatory guard asked, and may short-circuit the prompt // with an explicit allow or deny verdict; returning nothing falls // through to the interactive confirmation. The permission_request // chain is SKIPPED entirely when the preempt-yolo lane of pre_tool_use @@ -816,7 +799,7 @@ func (c *call) confirmationMutex() *sync.Mutex { // hook auto-allow the call would unwind that protection. func (c *call) askUser(ctx context.Context, runTool func() CallOutcome) CallOutcome { var hookMeta map[string]string - if c.preYoloResult == nil || c.preYoloResult.Decision != hooks.DecisionAsk { + if !c.mandatoryAsk() && (c.preYoloResult == nil || c.preYoloResult.Decision != hooks.DecisionAsk) { outcome, handled, meta := c.runPermissionRequestHook(ctx, runTool) if handled { return outcome @@ -934,9 +917,9 @@ func (c *call) runPermissionRequestHook(ctx context.Context, runTool func() Call // confirmationMetadata merges the tool's static metadata (set by the // toolset) with the per-call metadata contributed by permission_request, // the runtime's own safety label, and the preempt-yolo lane of -// pre_tool_use. Merge order — and therefore key-clash precedence — is: +// pre_tool_use and tool_guard. Merge order — and therefore key-clash precedence — is: // -// tool static < permission_request < safety label < pre_tool_use (preempt_yolo) +// tool static < permission_request < safety label < pre_tool_use (preempt_yolo) < tool_guard // // The runtime's classification (safety_label, blast_radius, category, // reason) outranks a policy-level permission_request hook so the @@ -954,6 +937,9 @@ func (c *call) confirmationMetadata(permissionMeta map[string]string) map[string maps.Copy(merged, permissionMeta) maps.Copy(merged, labelMeta) maps.Copy(merged, preemptMeta) + if c.guardResult != nil { + maps.Copy(merged, c.guardResult.Metadata) + } return merged } diff --git a/pkg/runtime/toolexec/tool_hooks.go b/pkg/runtime/toolexec/tool_hooks.go new file mode 100644 index 0000000000..b862725426 --- /dev/null +++ b/pkg/runtime/toolexec/tool_hooks.go @@ -0,0 +1,124 @@ +package toolexec + +import ( + "cmp" + "context" + "fmt" + + "github.com/docker/docker-agent/pkg/hooks" +) + +// transformToolInput runs once, before any guard, classifier, or permission rule. +func (c *call) transformToolInput(ctx context.Context) bool { + if c.d.Hooks == nil { + return false + } + in := NewHooksInput(c.sess, c.tc) + in.ToolCategory = c.tool.Category + result := c.d.Hooks.Dispatch(ctx, c.a, hooks.EventToolInputTransform, in) + if result == nil { + return false + } + if !result.Allowed { + c.blockToolHook(ctx, hooks.EventToolInputTransform, ApprovalSourceToolInputTransformDeny, result.Message) + return true + } + if _, err := c.applyHookModifiedInput(result); err != nil { + c.blockToolHook(ctx, hooks.EventToolInputTransform, ApprovalSourceToolInputTransformDeny, fmt.Sprintf("invalid rewritten input: %v", err)) + return true + } + return false +} + +func (c *call) consultToolGuard(ctx context.Context) *hooks.Result { + if !c.guardComputed { + c.guardComputed = true + if c.d.Hooks != nil { + in := NewHooksInput(c.sess, c.tc) + in.ToolCategory = c.tool.Category + c.guardResult = c.d.Hooks.Dispatch(ctx, c.a, hooks.EventToolGuard, in) + } + } + return c.guardResult +} + +func (c *call) toolGuardAsks() bool { + return c.guardResult != nil && c.guardResult.Decision == hooks.DecisionAsk +} + +// consultMandatoryHooks evaluates both guard lanes without approving or prompting. +func (c *call) consultMandatoryHooks(ctx context.Context) bool { + guard := c.consultToolGuard(ctx) + if guard != nil && (!guard.Allowed || guard.Decision == hooks.DecisionDeny) { + c.blockToolHook(ctx, hooks.EventToolGuard, ApprovalSourceToolGuardDeny, cmp.Or(guard.Message, guard.DecisionReason)) + return true + } + preempt := c.consultPreToolUsePreYolo(ctx) + if preempt != nil && (!preempt.Allowed || preempt.Decision == hooks.DecisionDeny) { + c.blockToolHook(ctx, hooks.EventPreToolUse, ApprovalSourcePreToolUseHookDeny, cmp.Or(preempt.Message, preempt.DecisionReason)) + return true + } + return false +} + +func (c *call) runToolGuards(ctx context.Context, runTool func() CallOutcome) (CallOutcome, bool) { + if c.consultMandatoryHooks(ctx) { + return CallOutcome{}, true + } + preempt := c.preYoloResult + + if c.toolGuardAsks() { + // A mandatory ask cannot turn an explicit policy denial into approval. + decision := c.permissionDecision() + if decision.Outcome == OutcomeDeny { + c.notifyApproval(ctx, ApprovalDecisionDeny, denySourceForDecision(decision)) + c.errorResponse(ctx, denyErrorMessage(decision, c.tc.Function.Name)) + return CallOutcome{}, true + } + return c.askUser(ctx, runTool), true + } + if preempt != nil && preempt.Decision == hooks.DecisionAsk { + // Preserve the legacy exception for informed session-scoped grants. + if c.sessionPermissionsAllow() { + c.notifyApproval(ctx, ApprovalDecisionAllow, ApprovalSourceSessionPermissionsAllow) + return runTool(), true + } + return c.askUser(ctx, runTool), true + } + return CallOutcome{}, false +} + +// Revalidation may deny or require fresh approval, but never auto-approve. +func (c *call) recheckRewrittenInput(ctx context.Context, runTool func() CallOutcome) (CallOutcome, bool) { + c.guardComputed, c.preYoloComputed = false, false + if c.consultMandatoryHooks(ctx) { + return CallOutcome{}, true + } + decision := c.permissionDecision() + if decision.Outcome == OutcomeDeny { + c.notifyApproval(ctx, ApprovalDecisionDeny, denySourceForDecision(decision)) + c.errorResponse(ctx, denyErrorMessage(decision, c.tc.Function.Name)) + return CallOutcome{}, true + } + c.rewrittenInputAsk = c.rewrittenInputAsk || c.toolGuardAsks() || + (c.preYoloResult != nil && c.preYoloResult.Decision == hooks.DecisionAsk) || + (decision.Outcome == OutcomeAsk && decision.Reason == ReasonChecker) + if c.rewrittenInputAsk { + return c.askUser(ctx, runTool), true + } + return CallOutcome{}, false +} + +func (c *call) mandatoryAsk() bool { + return c.toolGuardAsks() || c.rewrittenInputAsk +} + +func (c *call) blockToolHook(ctx context.Context, event hooks.EventType, source, reason string) { + message := fmt.Sprintf("The tool call was rejected by a %s hook.", event) + if reason != "" { + message += " Reason: " + reason + } + c.notifyApproval(ctx, ApprovalDecisionDeny, source) + c.em.EmitHookBlocked(c.tc, c.tool, message, c.a.Name()) + c.errorResponse(ctx, message) +} diff --git a/pkg/runtime/toolexec/tool_phases_recheck_test.go b/pkg/runtime/toolexec/tool_phases_recheck_test.go new file mode 100644 index 0000000000..1c88d458f3 --- /dev/null +++ b/pkg/runtime/toolexec/tool_phases_recheck_test.go @@ -0,0 +1,118 @@ +package toolexec_test + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/docker/docker-agent/pkg/hooks" + "github.com/docker/docker-agent/pkg/runtime/toolexec" + "github.com/docker/docker-agent/pkg/session" + "github.com/docker/docker-agent/pkg/tools" +) + +// rewriteToRmHooks wires the two legacy pre_tool_use lanes so the preempt +// lane has no opinion on the original command and only asks once the +// default lane has rewritten it to a destructive one. +func rewriteToRmHooks() *hooks.Config { + return &hooks.Config{ + PreToolUse: append( + preemptYolo(hook(bVerdictWhen, "rm", "ask", "destructive")), + matchAll(hook(bRewrite, "cmd", "rm -rf /"))..., + ), + } +} + +// After a legacy rewrite the recheck must validate the rewritten arguments +// against permission rules before the preempt lane's Ask path can prompt or +// run the tool. A rewrite may only tighten the verdict: a deny rule that +// matches the rewritten command wins over the preempt Ask and over the +// session-grant exception that Ask carries. +func TestToolPhases_LegacyRewriteRecheckValidatesRulesBeforePreemptAsk(t *testing.T) { + t.Parallel() + + t.Run("preempt ask must not prompt for a denied rewrite", func(t *testing.T) { + t.Parallel() + hd, _ := newPhaseHooks(t, rewriteToRmHooks()) + sess := session.New(session.WithSafetyPolicy(session.SafetyPolicyStrict)) + resume := make(chan toolexec.ResumeRequest, 1) + resume <- toolexec.ResumeRequest{Type: toolexec.ResumeTypeApprove} + d := newDispatcher(hd, resume) + d.Permissions = teamRules(nil, []string{"shell:cmd=rm*"}) + var got []string + em := &captureEmitter{} + + d.Process(t.Context(), sess, []tools.ToolCall{shellCall("x", "ls")}, []tools.Tool{shellTool(&got)}, em) + + assert.Empty(t, got, "rewritten command matches a deny rule and must never run") + requireDenied(t, em, hd, toolexec.ApprovalSourceTeamPermissionsDeny, "denied by permissions configuration") + assert.Equal(t, 2, hd.count(hooks.EventPreToolUsePreYolo), "preempt lane still rechecked on the rewrite") + }) + + t.Run("session grant must not run a denied rewrite", func(t *testing.T) { + t.Parallel() + hd, _ := newPhaseHooks(t, rewriteToRmHooks()) + sess := session.New(session.WithSafetyPolicy(session.SafetyPolicyStrict)) + // The grant is only visible to the preempt Ask shortcut: the + // provider carries the team deny rule alone, so the rules recheck + // must deny the rewrite regardless of what the session allows. + sess.Permissions = &session.PermissionsConfig{Allow: []string{"shell:cmd=rm*"}} + d := newDispatcher(hd, nil) + d.Permissions = teamRules(nil, []string{"shell:cmd=rm*"}) + var got []string + em := &captureEmitter{} + + d.Process(t.Context(), sess, []tools.ToolCall{shellCall("x", "ls")}, []tools.Tool{shellTool(&got)}, em) + + assert.Empty(t, got, "session grant must not run the tool before the rewritten rules are checked") + requireDenied(t, em, hd, toolexec.ApprovalSourceTeamPermissionsDeny, "denied by permissions configuration") + }) + + // Control: without a deny rule the preempt Ask on the rewrite still + // prompts and the user's answer is honored, so the fix must not turn + // the recheck into a blanket deny. + t.Run("preempt ask on the rewrite still prompts when rules allow", func(t *testing.T) { + t.Parallel() + hd, _ := newPhaseHooks(t, rewriteToRmHooks()) + sess := session.New(session.WithSafetyPolicy(session.SafetyPolicyStrict)) + resume := make(chan toolexec.ResumeRequest, 1) + resume <- toolexec.ResumeRequest{Type: toolexec.ResumeTypeReject, Reason: "no thanks"} + var got []string + em := &captureEmitter{} + + newDispatcher(hd, resume).Process(t.Context(), sess, []tools.ToolCall{shellCall("x", "ls")}, []tools.Tool{shellTool(&got)}, em) + + assert.Empty(t, got) + require.Len(t, em.confirmations, 1) + assert.JSONEq(t, `{"cmd":"rm -rf /"}`, em.confirmations[0].Function.Arguments) + require.Len(t, em.responses, 1) + assert.Contains(t, em.responses[0].Output, "no thanks") + }) +} + +func TestToolPhases_RewrittenAskCannotUseEarlierGrants(t *testing.T) { + t.Parallel() + for _, legacyAsk := range []bool{false, true} { + t.Run(map[bool]string{false: "preempt ask", true: "approval hook ask"}[legacyAsk], func(t *testing.T) { + t.Parallel() + cfg := rewriteToRmHooks() + if legacyAsk { + cfg.PreToolUse = matchAll(hook(bRewrite, "cmd", "rm -rf /"), hook(bVerdict, "ask", "review rewritten command")) + } + cfg.PermissionRequest = matchAll(hook(bPermit)) + hd, _ := newPhaseHooks(t, cfg) + sess := session.New(session.WithSafetyPolicy(session.SafetyPolicyStrict)) + sess.Permissions = &session.PermissionsConfig{Allow: []string{"shell:cmd=rm*"}} + resume := make(chan toolexec.ResumeRequest, 1) + resume <- toolexec.ResumeRequest{Type: toolexec.ResumeTypeReject} + d := newDispatcher(hd, resume) + d.Permissions = sessionCheckers + em := &captureEmitter{} + d.Process(t.Context(), sess, []tools.ToolCall{shellCall("x", "ls")}, []tools.Tool{neverRun()}, em) + require.Len(t, em.confirmations, 1) + assert.Zero(t, hd.count(hooks.EventPermissionRequest)) + assert.Equal(t, toolexec.ApprovalSourceUserRejected, lastApproval(t, hd).Source) + }) + } +} diff --git a/pkg/runtime/toolexec/tool_phases_test.go b/pkg/runtime/toolexec/tool_phases_test.go new file mode 100644 index 0000000000..5ff78e075e --- /dev/null +++ b/pkg/runtime/toolexec/tool_phases_test.go @@ -0,0 +1,793 @@ +package toolexec_test + +import ( + "context" + "errors" + "maps" + "strings" + "sync" + "testing" + + "github.com/docker/portcullis" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/docker/docker-agent/pkg/agent" + "github.com/docker/docker-agent/pkg/hooks" + "github.com/docker/docker-agent/pkg/hooks/builtins" + "github.com/docker/docker-agent/pkg/internal/portcullistest" + "github.com/docker/docker-agent/pkg/permissions" + "github.com/docker/docker-agent/pkg/runtime/toolexec" + "github.com/docker/docker-agent/pkg/safety" + "github.com/docker/docker-agent/pkg/session" + "github.com/docker/docker-agent/pkg/tools" +) + +// execHookDispatcher drives the dispatcher through a real +// [hooks.Executor] so the tests exercise the executor's aggregation +// (fail-closed, most-restrictive verdict, pipeline rewrites) instead of +// canned results. It mirrors the runtime adapter: no hooks configured +// for the event → nil, Dispatch error → nil. Notifications reuse the +// stub's recording. +type execHookDispatcher struct { + stubHookDispatcher + + exec *hooks.Executor +} + +func (h *execHookDispatcher) Dispatch(ctx context.Context, _ *agent.Agent, event hooks.EventType, in *hooks.Input) *hooks.Result { + h.mu.Lock() + h.dispatched = append(h.dispatched, event) + h.mu.Unlock() + if !h.exec.Has(event) { + return nil + } + result, err := h.exec.Dispatch(ctx, event, in) + if err != nil { + return nil + } + return result +} + +func (h *execHookDispatcher) count(event hooks.EventType) int { + h.mu.Lock() + defer h.mu.Unlock() + n := 0 + for _, e := range h.dispatched { + if e == event { + n++ + } + } + return n +} + +// phaseRecorder captures what each builtin saw, keyed by the public event +// name the executor hands to handlers. +type phaseRecorder struct { + mu sync.Mutex + inputs map[hooks.EventType][]map[string]any +} + +func (r *phaseRecorder) record(in *hooks.Input) { + r.mu.Lock() + defer r.mu.Unlock() + if r.inputs == nil { + r.inputs = make(map[hooks.EventType][]map[string]any) + } + r.inputs[in.HookEventName] = append(r.inputs[in.HookEventName], maps.Clone(in.ToolInput)) +} + +func (r *phaseRecorder) seen(event hooks.EventType) []map[string]any { + r.mu.Lock() + defer r.mu.Unlock() + return r.inputs[event] +} + +func (r *phaseRecorder) count(event hooks.EventType) int { + return len(r.seen(event)) +} + +func (r *phaseRecorder) cmds(event hooks.EventType) []string { + var out []string + for _, in := range r.seen(event) { + cmd, _ := in["cmd"].(string) + out = append(out, cmd) + } + return out +} + +// Builtin names registered by newPhaseHooks. Each records its input. +const ( + // rewrite : patches one argument. + bRewrite = "rewrite" + // verdict [k=v ...]: a permission_decision plus metadata. + bVerdict = "verdict" + // block : generic decision="block". + bBlock = "block" + // crash: handler error. + bCrash = "crash" + // verdict_when : verdict iff cmd contains substring. + bVerdictWhen = "verdict_when" + // permit: permission_request allow. + bPermit = "permit" +) + +func hook(name string, args ...string) hooks.Hook { + return hooks.Hook{Type: hooks.HookTypeBuiltin, Command: name, Args: args, Timeout: 5} +} + +func matchAll(hs ...hooks.Hook) []hooks.MatcherConfig { + return []hooks.MatcherConfig{{Matcher: "*", Hooks: hs}} +} + +func preemptYolo(hs ...hooks.Hook) []hooks.MatcherConfig { + yes := true + return []hooks.MatcherConfig{{Matcher: "*", Hooks: hs, PreemptYolo: &yes}} +} + +func newPhaseHooks(t *testing.T, cfg *hooks.Config) (*execHookDispatcher, *phaseRecorder) { + t.Helper() + rec := &phaseRecorder{} + reg := hooks.NewRegistry() + must := func(err error) { require.NoError(t, err) } + must(reg.RegisterBuiltin(bRewrite, func(_ context.Context, in *hooks.Input, args []string) (*hooks.Output, error) { + rec.record(in) + return &hooks.Output{HookSpecificOutput: &hooks.HookSpecificOutput{ + UpdatedInput: map[string]any{args[0]: args[1]}, + }}, nil + })) + must(reg.RegisterBuiltin(bVerdict, func(_ context.Context, in *hooks.Input, args []string) (*hooks.Output, error) { + rec.record(in) + var meta map[string]string + for _, kv := range args[2:] { + k, v, _ := strings.Cut(kv, "=") + if meta == nil { + meta = map[string]string{} + } + meta[k] = v + } + return &hooks.Output{HookSpecificOutput: &hooks.HookSpecificOutput{ + PermissionDecision: hooks.Decision(args[0]), + PermissionDecisionReason: args[1], + Metadata: meta, + }}, nil + })) + must(reg.RegisterBuiltin(bBlock, func(_ context.Context, in *hooks.Input, args []string) (*hooks.Output, error) { + rec.record(in) + return &hooks.Output{Decision: hooks.DecisionBlockValue, Reason: args[0]}, nil + })) + must(reg.RegisterBuiltin(bCrash, func(_ context.Context, in *hooks.Input, _ []string) (*hooks.Output, error) { + rec.record(in) + return nil, errors.New("boom") + })) + must(reg.RegisterBuiltin(bVerdictWhen, func(_ context.Context, in *hooks.Input, args []string) (*hooks.Output, error) { + rec.record(in) + if cmd, _ := in.ToolInput["cmd"].(string); !strings.Contains(cmd, args[0]) { + return nil, nil + } + return &hooks.Output{HookSpecificOutput: &hooks.HookSpecificOutput{ + PermissionDecision: hooks.Decision(args[1]), + PermissionDecisionReason: args[2], + }}, nil + })) + must(reg.RegisterBuiltin(bPermit, func(_ context.Context, in *hooks.Input, _ []string) (*hooks.Output, error) { + rec.record(in) + return &hooks.Output{HookSpecificOutput: &hooks.HookSpecificOutput{ + PermissionDecision: hooks.DecisionAllow, + }}, nil + })) + return &execHookDispatcher{exec: hooks.NewExecutorWithRegistry(cfg, t.TempDir(), nil, reg)}, rec +} + +// shellTool records the arguments the handler actually received. +func shellTool(gotArgs *[]string) tools.Tool { + var mu sync.Mutex + return tools.Tool{ + Name: safety.ShellToolName, + Category: "shell", + Handler: func(_ context.Context, tc tools.ToolCall, _ tools.Runtime) (*tools.ToolCallResult, error) { + mu.Lock() + defer mu.Unlock() + *gotArgs = append(*gotArgs, tc.Function.Arguments) + return tools.ResultSuccess("ok"), nil + }, + } +} + +func neverRun() tools.Tool { + return tools.Tool{ + Name: safety.ShellToolName, + Category: "shell", + Handler: func(context.Context, tools.ToolCall, tools.Runtime) (*tools.ToolCallResult, error) { + panic("tool must not run") + }, + } +} + +func shellCall(id, cmd string) tools.ToolCall { + return tools.ToolCall{ID: id, Function: tools.FunctionCall{Name: safety.ShellToolName, Arguments: `{"cmd":"` + cmd + `"}`}} +} + +func teamRules(allow, deny []string) func(*session.Session) []toolexec.NamedChecker { + return staticCheckers(toolexec.NamedChecker{ + Checker: permissions.NewCheckerFromRules(allow, nil, deny), + Source: "permissions configuration", + Tier: toolexec.TierTeam, + }) +} + +func newDispatcher(hd toolexec.HookDispatcher, resume <-chan toolexec.ResumeRequest) *toolexec.Dispatcher { + a := newAgent() + return &toolexec.Dispatcher{ + AgentFor: func(*session.Session) *agent.Agent { return a }, + Hooks: hd, + Resume: resume, + } +} + +func lastApproval(t *testing.T, hd *execHookDispatcher) approvalRecord { + t.Helper() + hd.mu.Lock() + defer hd.mu.Unlock() + require.NotEmpty(t, hd.approvals) + return hd.approvals[len(hd.approvals)-1] +} + +func requireDenied(t *testing.T, em *captureEmitter, hd *execHookDispatcher, source string, fragments ...string) { + t.Helper() + assert.Empty(t, em.confirmations, "denied calls must not prompt") + require.Len(t, em.responses, 1) + assert.True(t, em.responses[0].IsError) + for _, f := range fragments { + assert.Contains(t, em.responses[0].Output, f) + } + assert.Equal(t, approvalRecord{Decision: toolexec.ApprovalDecisionDeny, Source: source}, lastApproval(t, hd)) +} + +// --- tool_input_transform ordering ------------------------------------- + +// The transform's rewrite must be what every later stage sees: guard, +// classifier, rules, legacy hook and the tool itself. +func TestToolPhases_TransformPrecedesEveryStage(t *testing.T) { + t.Parallel() + + t.Run("autonomous: guard and tool see rewritten args", func(t *testing.T) { + t.Parallel() + hd, rec := newPhaseHooks(t, &hooks.Config{ + ToolInputTransform: matchAll(hook(bRewrite, "cmd", "ls")), + ToolGuard: matchAll(hook(bVerdict, "allow", "fine")), + PreToolUse: matchAll(hook(bVerdict, "ask", "legacy")), + }) + sess := session.New(session.WithSafetyPolicy(session.SafetyPolicyAutonomous)) + var got []string + em := &captureEmitter{} + + newDispatcher(hd, nil).Process(t.Context(), sess, []tools.ToolCall{shellCall("x", "rm -rf /")}, []tools.Tool{shellTool(&got)}, em) + + assert.Equal(t, []string{`{"cmd":"ls"}`}, got) + assert.Equal(t, []string{"rm -rf /"}, rec.cmds(hooks.EventToolInputTransform)) + assert.Equal(t, []string{"ls"}, rec.cmds(hooks.EventToolGuard)) + assert.Zero(t, rec.count(hooks.EventPreToolUse), "default lane stays skipped under autonomous") + assert.Empty(t, em.confirmations) + assert.Equal(t, approvalRecord{Decision: toolexec.ApprovalDecisionAllow, Source: toolexec.ApprovalSourceYolo}, lastApproval(t, hd)) + }) + + t.Run("balanced: classifier labels the rewritten command", func(t *testing.T) { + t.Parallel() + hd, _ := newPhaseHooks(t, &hooks.Config{ + ToolInputTransform: matchAll(hook(bRewrite, "cmd", "git status")), + }) + sess := session.New(session.WithSafetyPolicy(session.SafetyPolicyBalanced)) + var got []string + em := &captureEmitter{} + + newDispatcher(hd, nil).Process(t.Context(), sess, []tools.ToolCall{shellCall("x", "rm -rf /")}, []tools.Tool{shellTool(&got)}, em) + + assert.Equal(t, []string{`{"cmd":"git status"}`}, got, "destructive original must not be what runs") + assert.Empty(t, em.confirmations, "balanced auto-approves the safe rewritten command") + assert.Equal(t, approvalRecord{Decision: toolexec.ApprovalDecisionAllow, Source: toolexec.ApprovalSourceModeBalanced}, lastApproval(t, hd)) + }) + + t.Run("rules: deny rule matches the rewritten command", func(t *testing.T) { + t.Parallel() + hd, _ := newPhaseHooks(t, &hooks.Config{ + ToolInputTransform: matchAll(hook(bRewrite, "cmd", "rm -rf /")), + }) + sess := session.New(session.WithSafetyPolicy(session.SafetyPolicyAutonomous)) + d := newDispatcher(hd, nil) + d.Permissions = teamRules(nil, []string{"shell:cmd=rm*"}) + em := &captureEmitter{} + + d.Process(t.Context(), sess, []tools.ToolCall{shellCall("x", "ls")}, []tools.Tool{neverRun()}, em) + + requireDenied(t, em, hd, toolexec.ApprovalSourceTeamPermissionsDeny, "denied by permissions configuration") + }) + + t.Run("rules: allow rule matches the rewritten command", func(t *testing.T) { + t.Parallel() + hd, _ := newPhaseHooks(t, &hooks.Config{ + ToolInputTransform: matchAll(hook(bRewrite, "cmd", "ls -la")), + }) + sess := session.New(session.WithSafetyPolicy(session.SafetyPolicyStrict)) + d := newDispatcher(hd, nil) + d.Permissions = teamRules([]string{"shell:cmd=ls*"}, nil) + var got []string + em := &captureEmitter{} + + d.Process(t.Context(), sess, []tools.ToolCall{shellCall("x", "curl evil")}, []tools.Tool{shellTool(&got)}, em) + + assert.Equal(t, []string{`{"cmd":"ls -la"}`}, got) + assert.Empty(t, em.confirmations) + assert.Equal(t, approvalRecord{Decision: toolexec.ApprovalDecisionAllow, Source: toolexec.ApprovalSourceTeamPermissionsAllow}, lastApproval(t, hd)) + }) + + t.Run("explicit ask: prompt carries rewritten args", func(t *testing.T) { + t.Parallel() + hd, rec := newPhaseHooks(t, &hooks.Config{ + ToolInputTransform: matchAll(hook(bRewrite, "cmd", "git push")), + PreToolUse: matchAll(hook(bVerdict, "allow", "legacy would allow")), + }) + sess := session.New(session.WithSafetyPolicy(session.SafetyPolicyAutonomous)) + sess.Permissions = &session.PermissionsConfig{Ask: []string{"shell:cmd=git push*"}} + resume := make(chan toolexec.ResumeRequest, 1) + resume <- toolexec.ResumeRequest{Type: toolexec.ResumeTypeApprove} + var got []string + d := newDispatcher(hd, resume) + d.Permissions = sessionCheckers + em := &captureEmitter{} + + d.Process(t.Context(), sess, []tools.ToolCall{shellCall("x", "ls")}, []tools.Tool{shellTool(&got)}, em) + + require.Len(t, em.confirmations, 1) + assert.JSONEq(t, `{"cmd":"git push"}`, em.confirmations[0].Function.Arguments) + assert.Equal(t, []string{`{"cmd":"git push"}`}, got) + assert.Zero(t, rec.count(hooks.EventPreToolUse), "explicit ask rule bypasses the legacy hook") + }) + + t.Run("legacy read-only: default hook and tool see rewritten args", func(t *testing.T) { + t.Parallel() + hd, rec := newPhaseHooks(t, &hooks.Config{ + ToolInputTransform: matchAll(hook(bRewrite, "path", "/safe")), + PreToolUse: matchAll(hook(bVerdict, "", "observe only")), + }) + sess := session.New() // legacy default mode + var got string + tool := tools.Tool{ + Name: "read_file", + Annotations: tools.ToolAnnotations{ReadOnlyHint: true}, + Handler: func(_ context.Context, tc tools.ToolCall, _ tools.Runtime) (*tools.ToolCallResult, error) { + got = tc.Function.Arguments + return tools.ResultSuccess("ok"), nil + }, + } + em := &captureEmitter{} + + newDispatcher(hd, nil).Process(t.Context(), sess, []tools.ToolCall{{ + ID: "x", Function: tools.FunctionCall{Name: "read_file", Arguments: `{"path":"/etc/shadow"}`}, + }}, []tools.Tool{tool}, em) + + assert.JSONEq(t, `{"path":"/safe"}`, got) + assert.Empty(t, em.confirmations) + require.Len(t, rec.seen(hooks.EventPreToolUse), 1) + assert.Equal(t, "/safe", rec.seen(hooks.EventPreToolUse)[0]["path"]) + assert.Equal(t, approvalRecord{Decision: toolexec.ApprovalDecisionAllow, Source: toolexec.ApprovalSourceReadOnlyHint}, lastApproval(t, hd)) + }) +} + +func TestToolPhases_TransformReachesGate(t *testing.T) { + t.Parallel() + hd, rec := newPhaseHooks(t, &hooks.Config{ + ToolInputTransform: matchAll(hook(bRewrite, "cmd", "make build")), + ToolGuard: matchAll(hook(bVerdict, "allow", "fine")), + }) + sess := session.New(session.WithSafetyPolicy(session.SafetyPolicyAutonomous)) + em := &captureEmitter{} + g := newGate(sess, em, nil) + g.Hooks = hd + + var executed tools.ConfirmedRun + out, err := g.ConfirmAndRun(t.Context(), echoCommand, func(_ context.Context, run tools.ConfirmedRun) (string, error) { + executed = run + return "built", nil + }) + + require.NoError(t, err) + assert.Equal(t, "built", out) + assert.Equal(t, "make build", executed.Args["cmd"], "gate must execute the transformed command") + assert.Equal(t, echoCommand.Metadata, executed.Metadata) + assert.Equal(t, []string{"make build"}, rec.cmds(hooks.EventToolGuard)) +} + +func TestToolPhases_TransformBlockDenies(t *testing.T) { + t.Parallel() + + t.Run("explicit block", func(t *testing.T) { + t.Parallel() + hd, rec := newPhaseHooks(t, &hooks.Config{ + ToolInputTransform: matchAll(hook(bBlock, "policy says no")), + ToolGuard: matchAll(hook(bVerdict, "allow", "fine")), + }) + sess := session.New(session.WithSafetyPolicy(session.SafetyPolicyAutonomous)) + em := &captureEmitter{} + + newDispatcher(hd, nil).Process(t.Context(), sess, []tools.ToolCall{shellCall("x", "ls")}, []tools.Tool{neverRun()}, em) + + requireDenied(t, em, hd, toolexec.ApprovalSourceToolInputTransformDeny, "tool_input_transform hook", "policy says no") + require.Len(t, em.hookBlocks, 1) + assert.Zero(t, rec.count(hooks.EventToolGuard), "a blocked transform never reaches the guard") + }) + + t.Run("crash follows on_error", func(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name string + onError string + runs bool + }{ + {name: "default warn proceeds", runs: true}, + {name: "block denies", onError: "block"}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + crash := hook(bCrash) + crash.OnError = tc.onError + hd, _ := newPhaseHooks(t, &hooks.Config{ToolInputTransform: matchAll(crash)}) + sess := session.New(session.WithSafetyPolicy(session.SafetyPolicyAutonomous)) + var got []string + em := &captureEmitter{} + + newDispatcher(hd, nil).Process(t.Context(), sess, []tools.ToolCall{shellCall("x", "ls")}, []tools.Tool{shellTool(&got)}, em) + + if tc.runs { + assert.Equal(t, []string{`{"cmd":"ls"}`}, got, "warn policy keeps the original input") + return + } + assert.Empty(t, got) + requireDenied(t, em, hd, toolexec.ApprovalSourceToolInputTransformDeny, "tool_input_transform hook failed to execute") + }) + } + }) +} + +// --- tool_guard ---------------------------------------------------------- + +func TestToolPhases_GuardDenyPreemptsYolo(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + name string + guard hooks.Hook + want []string + }{ + {name: "deny verdict", guard: hook(bVerdict, "deny", "destructive"), want: []string{"tool_guard hook", "destructive"}}, + {name: "generic block", guard: hook(bBlock, "nope"), want: []string{"tool_guard hook", "nope"}}, + {name: "crash fails closed", guard: hook(bCrash), want: []string{"tool_guard hook failed to execute", "boom"}}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + hd, rec := newPhaseHooks(t, &hooks.Config{ + ToolGuard: matchAll(tc.guard), + PreToolUse: matchAll(hook(bVerdict, "allow", "legacy allow")), + }) + sess := session.New() + sess.ToolsApproved = true + d := newDispatcher(hd, nil) + d.Permissions = teamRules([]string{"shell"}, nil) + em := &captureEmitter{} + + d.Process(t.Context(), sess, []tools.ToolCall{shellCall("x", "ls")}, []tools.Tool{neverRun()}, em) + + requireDenied(t, em, hd, toolexec.ApprovalSourceToolGuardDeny, tc.want...) + require.Len(t, em.hookBlocks, 1) + assert.Zero(t, rec.count(hooks.EventPreToolUse)) + assert.Zero(t, hd.count(hooks.EventPermissionRequest)) + }) + } +} + +// A guard Ask is strict: neither an "always allow" session grant nor a +// permission_request allow may silence it. The user is prompted once and +// the guard is dispatched once per call, including for calls queued +// behind another confirmation. +func TestToolPhases_GuardAskForcesPromptDespiteGrants(t *testing.T) { + t.Parallel() + hd, rec := newPhaseHooks(t, &hooks.Config{ + ToolGuard: matchAll(hook(bVerdict, "ask", "needs a human", "blast_radius=high", "category=deploy")), + PermissionRequest: matchAll(hook(bPermit)), + }) + sess := session.New(session.WithSafetyPolicy(session.SafetyPolicyAutonomous)) + sess.Permissions = &session.PermissionsConfig{Allow: []string{"shell:cmd=mkdir*"}} + resume := make(chan toolexec.ResumeRequest, 2) + d := newDispatcher(hd, resume) + d.Permissions = sessionCheckers + var got []string + em := &captureEmitter{} + + // The first answer grants "always allow"; the second call, queued + // behind the confirmation mutex, must still prompt. + resume <- toolexec.ResumeRequest{Type: toolexec.ResumeTypeApproveTool, ToolName: "shell"} + resume <- toolexec.ResumeRequest{Type: toolexec.ResumeTypeApprove} + + d.Process(t.Context(), sess, []tools.ToolCall{ + shellCall("a", "mkdir one"), + shellCall("b", "mkdir two"), + }, []tools.Tool{shellTool(&got)}, em) + + assert.Len(t, em.confirmations, 2, "guard Ask prompts each call even with session allow grants") + assert.Len(t, got, 2, "both calls run once approved") + assert.Equal(t, 2, rec.count(hooks.EventToolGuard), "one guard dispatch per call, none after the prompt wait") + assert.Zero(t, rec.count(hooks.EventPermissionRequest), "permission_request must not fire behind a guard Ask") + for _, meta := range em.confirmationMeta { + assert.Equal(t, "high", meta["blast_radius"], "guard metadata reaches the prompt") + assert.Equal(t, "deploy", meta["category"]) + } + hd.mu.Lock() + defer hd.mu.Unlock() + for _, ap := range hd.approvals { + assert.Equal(t, toolexec.ApprovalDecisionAllow, ap.Decision) + assert.Contains(t, []string{toolexec.ApprovalSourceUserApprovedTool, toolexec.ApprovalSourceUserApproved}, ap.Source) + } +} + +func TestToolPhases_GuardAskOverridesSafeClassification(t *testing.T) { + t.Parallel() + hd, _ := newPhaseHooks(t, &hooks.Config{ + ToolGuard: matchAll(hook(bVerdict, "ask", "double check")), + }) + sess := session.New(session.WithSafetyPolicy(session.SafetyPolicyBalanced)) + resume := make(chan toolexec.ResumeRequest, 1) + resume <- toolexec.ResumeRequest{Type: toolexec.ResumeTypeReject, Reason: "no thanks"} + em := &captureEmitter{} + + newDispatcher(hd, resume).Process(t.Context(), sess, []tools.ToolCall{shellCall("x", "git status")}, []tools.Tool{neverRun()}, em) + + require.Len(t, em.confirmations, 1, "balanced would auto-allow git status; the guard Ask wins") + require.Len(t, em.responses, 1) + assert.Contains(t, em.responses[0].Output, "no thanks") +} + +func TestToolPhases_GuardAllowIsAdvisory(t *testing.T) { + t.Parallel() + + t.Run("cannot bypass a deny rule", func(t *testing.T) { + t.Parallel() + hd, _ := newPhaseHooks(t, &hooks.Config{ + ToolGuard: matchAll(hook(bVerdict, "allow", "looks fine")), + }) + sess := session.New(session.WithSafetyPolicy(session.SafetyPolicyAutonomous)) + d := newDispatcher(hd, nil) + d.Permissions = teamRules(nil, []string{"shell:cmd=rm*"}) + em := &captureEmitter{} + + d.Process(t.Context(), sess, []tools.ToolCall{shellCall("x", "rm -rf /")}, []tools.Tool{neverRun()}, em) + + requireDenied(t, em, hd, toolexec.ApprovalSourceTeamPermissionsDeny, "denied by permissions configuration") + }) + + t.Run("cannot skip the strict-mode prompt", func(t *testing.T) { + t.Parallel() + hd, rec := newPhaseHooks(t, &hooks.Config{ + ToolGuard: matchAll(hook(bVerdict, "allow", "looks fine")), + PreToolUse: matchAll(hook(bVerdict, "", "observe")), + }) + sess := session.New(session.WithSafetyPolicy(session.SafetyPolicyStrict)) + resume := make(chan toolexec.ResumeRequest, 1) + resume <- toolexec.ResumeRequest{Type: toolexec.ResumeTypeApprove} + var got []string + em := &captureEmitter{} + + newDispatcher(hd, resume).Process(t.Context(), sess, []tools.ToolCall{shellCall("x", "git status")}, []tools.Tool{shellTool(&got)}, em) + + require.Len(t, em.confirmations, 1, "guard allow must not auto-approve") + assert.Len(t, got, 1) + assert.Equal(t, 1, rec.count(hooks.EventPreToolUse), "default lane still runs when the mode asks") + }) +} + +func TestToolPhases_GuardAskCannotOverrideExplicitDeny(t *testing.T) { + t.Parallel() + hd, _ := newPhaseHooks(t, &hooks.Config{ + ToolGuard: matchAll(hook(bVerdict, "ask", "let the user decide")), + }) + sess := session.New(session.WithSafetyPolicy(session.SafetyPolicyAutonomous)) + sess.Permissions = &session.PermissionsConfig{Deny: []string{"shell:cmd=rm*"}} + d := newDispatcher(hd, nil) + d.Permissions = sessionCheckers + em := &captureEmitter{} + + d.Process(t.Context(), sess, []tools.ToolCall{shellCall("x", "rm -rf /")}, []tools.Tool{neverRun()}, em) + + requireDenied(t, em, hd, toolexec.ApprovalSourceSessionPermissionsDeny, "denied by session permissions") +} + +// --- legacy pre_tool_use lanes ------------------------------------------ + +// A preempt-yolo hook that fails (Allowed=false without a Deny verdict) +// now blocks instead of being read as "no opinion". +func TestToolPhases_PreemptYoloFailureBlocks(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + name string + h hooks.Hook + want string + }{ + {name: "crash", h: hook(bCrash), want: "PreToolUse hook failed to execute"}, + {name: "generic block", h: hook(bBlock, "stop right there"), want: "stop right there"}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + hd, _ := newPhaseHooks(t, &hooks.Config{PreToolUse: preemptYolo(tc.h)}) + sess := session.New() + sess.ToolsApproved = true + em := &captureEmitter{} + + newDispatcher(hd, nil).Process(t.Context(), sess, []tools.ToolCall{shellCall("x", "ls")}, []tools.Tool{neverRun()}, em) + + requireDenied(t, em, hd, toolexec.ApprovalSourcePreToolUseHookDeny, "pre_tool_use hook", tc.want) + }) + } +} + +// After a legacy pre_tool_use rewrite the guard, preempt lane and rules +// are re-evaluated exactly once on the new arguments; the transformer and +// the default lane are not rerun. +func TestToolPhases_LegacyRewriteRechecksGuardsAndRules(t *testing.T) { + t.Parallel() + + t.Run("deny rule catches the rewritten command", func(t *testing.T) { + t.Parallel() + hd, rec := newPhaseHooks(t, &hooks.Config{ + ToolInputTransform: matchAll(hook(bRewrite, "cwd", "/work")), + ToolGuard: matchAll(hook(bVerdict, "allow", "fine")), + PreToolUse: matchAll(hook(bRewrite, "cmd", "rm -rf /")), + }) + sess := session.New(session.WithSafetyPolicy(session.SafetyPolicyStrict)) + d := newDispatcher(hd, nil) + d.Permissions = teamRules(nil, []string{"shell:cmd=rm*"}) + em := &captureEmitter{} + + d.Process(t.Context(), sess, []tools.ToolCall{shellCall("x", "ls")}, []tools.Tool{neverRun()}, em) + + requireDenied(t, em, hd, toolexec.ApprovalSourceTeamPermissionsDeny, "denied by permissions configuration") + assert.Equal(t, 1, rec.count(hooks.EventToolInputTransform), "transformer runs once") + assert.Equal(t, 1, rec.count(hooks.EventPreToolUse), "default lane runs once") + assert.Equal(t, []string{"ls", "rm -rf /"}, rec.cmds(hooks.EventToolGuard), "guard rechecked on the rewrite") + assert.Equal(t, 2, hd.count(hooks.EventPreToolUsePreYolo), "preempt lane consulted again after the rewrite") + }) + + t.Run("guard denies the rewritten command", func(t *testing.T) { + t.Parallel() + hd, rec := newPhaseHooks(t, &hooks.Config{ + ToolGuard: matchAll(hook(bVerdictWhen, "rm", "deny", "no deletes")), + PreToolUse: matchAll(hook(bRewrite, "cmd", "rm -rf /")), + }) + sess := session.New(session.WithSafetyPolicy(session.SafetyPolicyStrict)) + em := &captureEmitter{} + + newDispatcher(hd, nil).Process(t.Context(), sess, []tools.ToolCall{shellCall("x", "ls")}, []tools.Tool{neverRun()}, em) + + requireDenied(t, em, hd, toolexec.ApprovalSourceToolGuardDeny, "tool_guard hook", "no deletes") + assert.Equal(t, []string{"ls", "rm -rf /"}, rec.cmds(hooks.EventToolGuard)) + assert.Equal(t, 1, rec.count(hooks.EventPreToolUse)) + }) + + t.Run("guard asks about the rewritten command", func(t *testing.T) { + t.Parallel() + hd, rec := newPhaseHooks(t, &hooks.Config{ + ToolGuard: matchAll(hook(bVerdictWhen, "-la", "ask", "review")), + PreToolUse: matchAll(hook(bRewrite, "cmd", "ls -la")), + }) + sess := session.New(session.WithSafetyPolicy(session.SafetyPolicyStrict)) + resume := make(chan toolexec.ResumeRequest, 1) + resume <- toolexec.ResumeRequest{Type: toolexec.ResumeTypeApprove} + var got []string + em := &captureEmitter{} + + newDispatcher(hd, resume).Process(t.Context(), sess, []tools.ToolCall{shellCall("x", "ls")}, []tools.Tool{shellTool(&got)}, em) + + require.Len(t, em.confirmations, 1) + assert.JSONEq(t, `{"cmd":"ls -la"}`, em.confirmations[0].Function.Arguments) + assert.Equal(t, []string{`{"cmd":"ls -la"}`}, got) + assert.Equal(t, 2, rec.count(hooks.EventToolGuard), "once before and once after the rewrite, never after the prompt") + assert.Equal(t, 1, rec.count(hooks.EventPreToolUse)) + }) + + t.Run("no-op rewrite does not rerun the guard", func(t *testing.T) { + t.Parallel() + hd, rec := newPhaseHooks(t, &hooks.Config{ + ToolGuard: matchAll(hook(bVerdict, "allow", "fine")), + PreToolUse: matchAll(hook(bRewrite, "cmd", "ls")), + }) + sess := session.New(session.WithSafetyPolicy(session.SafetyPolicyStrict)) + resume := make(chan toolexec.ResumeRequest, 1) + resume <- toolexec.ResumeRequest{Type: toolexec.ResumeTypeApprove} + var got []string + em := &captureEmitter{} + + newDispatcher(hd, resume).Process(t.Context(), sess, []tools.ToolCall{shellCall("x", "ls")}, []tools.Tool{shellTool(&got)}, em) + + assert.Len(t, got, 1) + assert.Equal(t, 1, rec.count(hooks.EventToolGuard)) + assert.Equal(t, 1, hd.count(hooks.EventPreToolUsePreYolo)) + }) +} + +func TestToolPhases_DefaultLaneSkippedOnAutoApproval(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + name string + sess func() *session.Session + perm func(*session.Session) []toolexec.NamedChecker + }{ + {name: "autonomous", sess: func() *session.Session { return session.New(session.WithSafetyPolicy(session.SafetyPolicyAutonomous)) }}, + {name: "balanced safe", sess: func() *session.Session { return session.New(session.WithSafetyPolicy(session.SafetyPolicyBalanced)) }}, + {name: "allow rule", sess: func() *session.Session { return session.New(session.WithSafetyPolicy(session.SafetyPolicyStrict)) }, perm: teamRules([]string{"shell"}, nil)}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + hd, rec := newPhaseHooks(t, &hooks.Config{ + ToolInputTransform: matchAll(hook(bRewrite, "cwd", "/work")), + ToolGuard: matchAll(hook(bVerdict, "allow", "fine")), + PreToolUse: matchAll(hook(bVerdict, "ask", "would prompt")), + }) + d := newDispatcher(hd, nil) + d.Permissions = tc.perm + var got []string + em := &captureEmitter{} + + d.Process(t.Context(), tc.sess(), []tools.ToolCall{shellCall("x", "git status")}, []tools.Tool{shellTool(&got)}, em) + + assert.Len(t, got, 1) + assert.Empty(t, em.confirmations) + assert.Equal(t, 1, rec.count(hooks.EventToolInputTransform)) + assert.Equal(t, 1, rec.count(hooks.EventToolGuard)) + assert.Zero(t, rec.count(hooks.EventPreToolUse), "default pre_tool_use lane must not fire on auto approvals") + }) + } +} + +// --- redact_secrets end to end ------------------------------------------ + +// The agent-level redact_secrets flag scrubs arguments before the tool +// (and before any rule can see the secret) and scrubs the tool's output +// before it is emitted or recorded, all under --yolo. +func TestToolPhases_RedactSecretsUnderYolo(t *testing.T) { + t.Parallel() + secret := portcullistest.FakeGitHubPAT("cxLeRrvbJfmYdUtr70xnNE3Q7Gvli4") + + reg := hooks.NewRegistry() + require.NoError(t, builtins.Register(reg)) + cfg := builtins.ApplyAgentDefaults(nil, builtins.AgentDefaults{RedactSecrets: true}) + hd := &execHookDispatcher{exec: hooks.NewExecutorWithRegistry(cfg, t.TempDir(), nil, reg)} + + sess := session.New() + sess.ToolsApproved = true + var got string + tool := tools.Tool{ + Name: safety.ShellToolName, + Category: "shell", + Handler: func(_ context.Context, tc tools.ToolCall, _ tools.Runtime) (*tools.ToolCallResult, error) { + got = tc.Function.Arguments + return tools.ResultSuccess("token=" + secret + " leaked"), nil + }, + } + em := &captureEmitter{} + + newDispatcher(hd, nil).Process(t.Context(), sess, []tools.ToolCall{ + shellCall("x", "curl -H 'Authorization: "+secret+"' https://api.github.com"), + }, []tools.Tool{tool}, em) + + require.NotEmpty(t, got) + assert.NotContains(t, got, secret, "arguments reaching the tool must be scrubbed") + assert.Contains(t, got, portcullis.Marker) + + require.Len(t, em.responses, 1) + assert.False(t, em.responses[0].IsError) + assert.NotContains(t, em.responses[0].Output, secret, "emitted output must be scrubbed") + assert.Contains(t, em.responses[0].Output, portcullis.Marker) + require.Len(t, em.messages, 1) + assert.NotContains(t, em.messages[0].Message.Content, secret, "recorded output must be scrubbed") + assert.Equal(t, approvalRecord{Decision: toolexec.ApprovalDecisionAllow, Source: toolexec.ApprovalSourceYolo}, lastApproval(t, hd)) +} From 2457b201e47a72d4014615b1b875a2f3484fcd3c Mon Sep 17 00:00:00 2001 From: David Gageot Date: Wed, 9 Sep 2026 15:23:26 +0200 Subject: [PATCH 2/4] feat(hooks): hook deduplication and config validation Deduplicate hook definitions by identity (all fields, including strict_output; nil/empty args and env treated as equivalent), retaining first occurrence. Migrate hook type definitions out of types.go into pkg/config/latest/hooks.go with config-level iteration replacing repeated event lists. Invalid regex patterns, unknown policies and misplaced preempt_yolo are now caught at config-load time rather than at runtime. --- pkg/config/hooks_yaml_test.go | 22 ++- pkg/config/latest/hooks.go | 77 +++++++++ pkg/config/latest/hooks_test.go | 34 ++++ pkg/config/latest/types.go | 265 +++---------------------------- pkg/hooks/builtins/dedup_test.go | 38 +++++ pkg/hooks/dedup_test.go | 187 ++++++++++++++++++++-- pkg/hooks/types.go | 6 +- 7 files changed, 368 insertions(+), 261 deletions(-) create mode 100644 pkg/config/latest/hooks.go create mode 100644 pkg/config/latest/hooks_test.go create mode 100644 pkg/hooks/builtins/dedup_test.go diff --git a/pkg/config/hooks_yaml_test.go b/pkg/config/hooks_yaml_test.go index b4705f3ff5..34fac75678 100644 --- a/pkg/config/hooks_yaml_test.go +++ b/pkg/config/hooks_yaml_test.go @@ -1,6 +1,7 @@ package config import ( + "reflect" "testing" "github.com/goccy/go-yaml" @@ -243,14 +244,16 @@ tool_guard: } { err := tc.cfg.Validate() require.Error(t, err, tc.name) - assert.Contains(t, err.Error(), "hooks."+tc.name+"[0]: preempt_yolo is not valid on "+tc.name) + assert.Contains(t, err.Error(), "hooks."+tc.name+"[0]: preempt_yolo is only valid on pre_tool_use") } - // Legacy events still accept it (pre_tool_use) or ignore it (others). + // Only pre_tool_use accepts the legacy lane option. legacy := latest.HooksConfig{ PreToolUse: latest.HookMatcherConfigs{{PreemptYolo: &preempt, Hooks: cfg.ToolGuard[0].Hooks}}, PostToolUse: latest.HookMatcherConfigs{{PreemptYolo: &preempt, Hooks: cfg.ToolGuard[0].Hooks}}, } + require.ErrorContains(t, legacy.Validate(), "preempt_yolo is only valid on pre_tool_use") + legacy.PostToolUse = nil require.NoError(t, legacy.Validate()) // Malformed entries are still caught on the new events. @@ -258,3 +261,18 @@ tool_guard: require.Error(t, err) assert.Contains(t, err.Error(), "hooks.tool_guard[0]: at least one hook is required") } + +func TestMergeHooksIncludesEveryEvent(t *testing.T) { + t.Parallel() + base, extra := &latest.HooksConfig{}, &latest.HooksConfig{} + for _, cfg := range []*latest.HooksConfig{base, extra} { + v := reflect.ValueOf(cfg).Elem() + for _, field := range v.Fields() { + field.Set(reflect.MakeSlice(field.Type(), 1, 1)) + } + } + merged := reflect.ValueOf(MergeHooks(base, extra)).Elem() + for i := range merged.NumField() { + assert.Equal(t, 2, merged.Field(i).Len(), merged.Type().Field(i).Name) + } +} diff --git a/pkg/config/latest/hooks.go b/pkg/config/latest/hooks.go new file mode 100644 index 0000000000..a084e7d273 --- /dev/null +++ b/pkg/config/latest/hooks.go @@ -0,0 +1,77 @@ +package latest + +import ( + "fmt" + "iter" + "reflect" + "strings" + + "github.com/docker/docker-agent/pkg/hooks/events" +) + +// Events iterates over populated hook events using their persisted names. +func (h *HooksConfig) Events() iter.Seq2[string, HookMatcherConfigs] { + return func(yield func(string, HookMatcherConfigs) bool) { + if h == nil { + return + } + v := reflect.ValueOf(h).Elem() + for i := range v.NumField() { + if v.Field(i).Len() == 0 { + continue + } + name, _, _ := strings.Cut(v.Type().Field(i).Tag.Get("json"), ",") + var matchers HookMatcherConfigs + switch hooks := v.Field(i).Interface().(type) { + case HookDefinitions: + matchers = HookMatcherConfigs{{Hooks: hooks}} + case HookMatcherConfigs: + matchers = hooks + } + if !yield(name, matchers) { + return + } + } + } +} + +// IsEmpty reports whether no hook events are configured. +func (h *HooksConfig) IsEmpty() bool { + for range h.Events() { + return false + } + return true +} + +// Validate checks hook definitions and event-specific options. +func (h *HooksConfig) Validate() error { + for event, matchers := range h.Events() { + contract, ok := events.Lookup(event) + if !ok { + return fmt.Errorf("hooks.%s: unknown event", event) + } + if !contract.CanBlock { + for _, matcher := range matchers { + for _, hook := range matcher.Hooks { + if hook.OnError == "block" { + return fmt.Errorf("hooks.%s: on_error block is not supported by this event", event) + } + } + } + } + for i, matcher := range matchers { + if contract.ToolMatched { + if err := matcher.validate(event, i); err != nil { + return err + } + } else { + for j, hook := range matcher.Hooks { + if err := hook.validate(event, j); err != nil { + return err + } + } + } + } + } + return nil +} diff --git a/pkg/config/latest/hooks_test.go b/pkg/config/latest/hooks_test.go new file mode 100644 index 0000000000..76fa4d423c --- /dev/null +++ b/pkg/config/latest/hooks_test.go @@ -0,0 +1,34 @@ +package latest + +import ( + "testing" + + "github.com/goccy/go-yaml" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestHooksValidateContracts(t *testing.T) { + t.Parallel() + for _, tc := range []struct{ name, config, want string }{ + {"matcher", `pre_tool_use: [{matcher: "[", hooks: [{type: command, command: true}]}]`, "invalid matcher"}, + {"policy", `turn_start: [{type: command, command: true, on_error: blok}]`, "on_error must"}, + {"negative timeout", `turn_start: [{type: command, command: true, timeout: -1}]`, "timeout must"}, + {"unsupported block", `stop: [{type: command, command: true, on_error: block}]`, "not supported"}, + {"invalid lane", `post_tool_use: [{preempt_yolo: false, hooks: [{type: command, command: true}]}]`, "only valid on pre_tool_use"}, + {"strict output", `before_llm_call: [{type: command, command: true, strict_output: true, on_error: block}]`, ""}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + var cfg HooksConfig + require.NoError(t, yaml.Unmarshal([]byte(tc.config), &cfg)) + err := cfg.Validate() + if tc.want != "" { + require.ErrorContains(t, err, tc.want) + } else { + require.NoError(t, err) + assert.True(t, cfg.BeforeLLMCall[0].StrictOutput) + } + }) + } +} diff --git a/pkg/config/latest/types.go b/pkg/config/latest/types.go index 82d0ca0e3f..97f5bc1156 100644 --- a/pkg/config/latest/types.go +++ b/pkg/config/latest/types.go @@ -7,6 +7,7 @@ import ( "fmt" "maps" "math" + "regexp" "slices" "strconv" "strings" @@ -2806,41 +2807,6 @@ type HooksConfig struct { WorktreeCreate HookDefinitions `json:"worktree_create,omitempty" yaml:"worktree_create,omitempty"` } -// IsEmpty returns true if no hooks are configured -func (h *HooksConfig) IsEmpty() bool { - if h == nil { - return true - } - return len(h.PreToolUse) == 0 && - len(h.PostToolUse) == 0 && - len(h.PermissionRequest) == 0 && - len(h.SessionStart) == 0 && - len(h.UserPromptSubmit) == 0 && - len(h.UserSteeringMessagesSubmit) == 0 && - len(h.UserFollowupSubmit) == 0 && - len(h.TurnStart) == 0 && - len(h.TurnEnd) == 0 && - len(h.BeforeLLMCall) == 0 && - len(h.AfterLLMCall) == 0 && - len(h.SessionEnd) == 0 && - len(h.PreCompact) == 0 && - len(h.SubagentStop) == 0 && - len(h.OnUserInput) == 0 && - len(h.Stop) == 0 && - len(h.Notification) == 0 && - len(h.OnError) == 0 && - len(h.OnMaxIterations) == 0 && - len(h.OnAgentSwitch) == 0 && - len(h.OnSessionResume) == 0 && - len(h.OnToolApprovalDecision) == 0 && - len(h.BeforeCompaction) == 0 && - len(h.AfterCompaction) == 0 && - len(h.ToolResponseTransform) == 0 && - len(h.ToolInputTransform) == 0 && - len(h.ToolGuard) == 0 && - len(h.WorktreeCreate) == 0 -} - // HookMatcherConfig represents a hook matcher with its hooks. // Used for tool-related hooks (PreToolUse, PostToolUse). type HookMatcherConfig struct { @@ -2858,9 +2824,7 @@ type HookMatcherConfig struct { // permission allow-rules; an allow verdict is advisory (the // pipeline still runs Decide() and the rest of pre_tool_use). // Default pre_tool_use entries fire AFTER Decide(), as before. - // Only valid on pre_tool_use. Rejected on tool_input_transform and - // tool_guard, which always preempt approval; ignored on other - // events. + // Only valid on pre_tool_use; rejected on every other event. // // Set it on hooks that implement a security-critical check that // must not be bypassed by auto-approval. @@ -2967,6 +2931,9 @@ type HookDefinition struct { // OnError controls non-fail-closed hook failures: warn (default), ignore, or block. OnError string `json:"on_error,omitempty" yaml:"on_error,omitempty"` + // StrictOutput requires JSON output and validates event-specific capabilities. + StrictOutput bool `json:"strict_output,omitempty" yaml:"strict_output,omitempty"` + // Model is the model spec ("provider/model", e.g. "openai/gpt-4o-mini") // invoked by Type==model hooks. Required for that type, ignored // otherwise. @@ -3008,209 +2975,16 @@ func (h *HookDefinition) DisplayName() string { return h.Type } -// Validate validates the HooksConfig -func (h *HooksConfig) Validate() error { - // Validate PreToolUse matchers - for i, m := range h.PreToolUse { - if err := m.validate("pre_tool_use", i); err != nil { - return err - } - } - - // Validate PostToolUse matchers - for i, m := range h.PostToolUse { - if err := m.validate("post_tool_use", i); err != nil { - return err - } - } - - // Validate PermissionRequest matchers - for i, m := range h.PermissionRequest { - if err := m.validate("permission_request", i); err != nil { - return err - } - } - - // Validate SessionStart hooks - for i, hook := range h.SessionStart { - if err := hook.validate("session_start", i); err != nil { - return err - } - } - - // Validate UserPromptSubmit hooks - for i, hook := range h.UserPromptSubmit { - if err := hook.validate("user_prompt_submit", i); err != nil { - return err - } - } - - // Validate UserSteeringMessagesSubmit hooks - for i, hook := range h.UserSteeringMessagesSubmit { - if err := hook.validate("user_steering_messages_submit", i); err != nil { - return err - } - } - - // Validate UserFollowupSubmit hooks - for i, hook := range h.UserFollowupSubmit { - if err := hook.validate("user_followup_submit", i); err != nil { - return err - } - } - - // Validate TurnStart hooks - for i, hook := range h.TurnStart { - if err := hook.validate("turn_start", i); err != nil { - return err - } - } - - // Validate TurnEnd hooks - for i, hook := range h.TurnEnd { - if err := hook.validate("turn_end", i); err != nil { - return err - } - } - - // Validate BeforeLLMCall hooks - for i, hook := range h.BeforeLLMCall { - if err := hook.validate("before_llm_call", i); err != nil { - return err - } - } - - // Validate AfterLLMCall hooks - for i, hook := range h.AfterLLMCall { - if err := hook.validate("after_llm_call", i); err != nil { - return err - } - } - - // Validate SessionEnd hooks - for i, hook := range h.SessionEnd { - if err := hook.validate("session_end", i); err != nil { - return err - } - } - - // Validate PreCompact hooks - for i, hook := range h.PreCompact { - if err := hook.validate("pre_compact", i); err != nil { - return err - } - } - - // Validate SubagentStop hooks - for i, hook := range h.SubagentStop { - if err := hook.validate("subagent_stop", i); err != nil { - return err - } - } - - // Validate OnUserInput hooks - for i, hook := range h.OnUserInput { - if err := hook.validate("on_user_input", i); err != nil { - return err - } - } - - // Validate Stop hooks - for i, hook := range h.Stop { - if err := hook.validate("stop", i); err != nil { - return err - } - } - - // Validate Notification hooks - for i, hook := range h.Notification { - if err := hook.validate("notification", i); err != nil { - return err - } - } - - // Validate OnError hooks - for i, hook := range h.OnError { - if err := hook.validate("on_error", i); err != nil { - return err - } - } - - // Validate OnMaxIterations hooks - for i, hook := range h.OnMaxIterations { - if err := hook.validate("on_max_iterations", i); err != nil { - return err - } - } - - // Validate OnAgentSwitch hooks - for i, hook := range h.OnAgentSwitch { - if err := hook.validate("on_agent_switch", i); err != nil { - return err - } - } - - // Validate OnSessionResume hooks - for i, hook := range h.OnSessionResume { - if err := hook.validate("on_session_resume", i); err != nil { - return err - } - } - - // Validate OnToolApprovalDecision hooks - for i, hook := range h.OnToolApprovalDecision { - if err := hook.validate("on_tool_approval_decision", i); err != nil { - return err - } - } - - // Validate BeforeCompaction hooks - for i, hook := range h.BeforeCompaction { - if err := hook.validate("before_compaction", i); err != nil { - return err - } - } - - // Validate AfterCompaction hooks - for i, hook := range h.AfterCompaction { - if err := hook.validate("after_compaction", i); err != nil { - return err - } - } - - // Validate ToolResponseTransform matchers - for i, m := range h.ToolResponseTransform { - if err := m.validate("tool_response_transform", i); err != nil { - return err - } - } - - // Validate ToolInputTransform matchers - for i, m := range h.ToolInputTransform { - if err := m.validatePreApproval("tool_input_transform", i); err != nil { - return err - } - } - - // Validate ToolGuard matchers - for i, m := range h.ToolGuard { - if err := m.validatePreApproval("tool_guard", i); err != nil { - return err - } +// validate validates a HookMatcherConfig +func (m *HookMatcherConfig) validate(eventType string, index int) error { + if m.PreemptYolo != nil && eventType != "pre_tool_use" { + return fmt.Errorf("hooks.%s[%d]: preempt_yolo is only valid on pre_tool_use", eventType, index) } - - // Validate WorktreeCreate hooks - for i, hook := range h.WorktreeCreate { - if err := hook.validate("worktree_create", i); err != nil { - return err + if m.Matcher != "" && m.Matcher != "*" { + if _, err := regexp.Compile("^(?:" + m.Matcher + ")$"); err != nil { + return fmt.Errorf("hooks.%s[%d]: invalid matcher: %w", eventType, index, err) } } - - return nil -} - -// validate validates a HookMatcherConfig -func (m *HookMatcherConfig) validate(eventType string, index int) error { if len(m.Hooks) == 0 { return fmt.Errorf("hooks.%s[%d]: at least one hook is required", eventType, index) } @@ -3224,17 +2998,14 @@ func (m *HookMatcherConfig) validate(eventType string, index int) error { return nil } -// validatePreApproval validates a matcher on an event that always runs -// before approval, where preempt_yolo is meaningless and rejected. -func (m *HookMatcherConfig) validatePreApproval(eventType string, index int) error { - if m.PreemptYolo != nil { - return fmt.Errorf("hooks.%s[%d]: preempt_yolo is not valid on %s (it always runs before approval)", eventType, index, eventType) - } - return m.validate(eventType, index) -} - // validate validates a HookDefinition func (h *HookDefinition) validate(prefix string, index int) error { + if h.OnError != "" && h.OnError != "warn" && h.OnError != "ignore" && h.OnError != "block" { + return fmt.Errorf("hooks.%s[%d]: on_error must be warn, ignore, or block", prefix, index) + } + if h.Timeout < 0 { + return fmt.Errorf("hooks.%s[%d]: timeout must not be negative", prefix, index) + } if h.Type == "" { return fmt.Errorf("hooks.%s[%d]: type is required", prefix, index) } diff --git a/pkg/hooks/builtins/dedup_test.go b/pkg/hooks/builtins/dedup_test.go new file mode 100644 index 0000000000..1abd5316b9 --- /dev/null +++ b/pkg/hooks/builtins/dedup_test.go @@ -0,0 +1,38 @@ +package builtins_test + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/docker/docker-agent/pkg/hooks" + "github.com/docker/docker-agent/pkg/hooks/builtins" +) + +func TestAgentDefaultsDeduplicateIdenticalHooks(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + name string + hook hooks.Hook + want int + }{ + {name: "identical", hook: hooks.Hook{Type: hooks.HookTypeBuiltin, Command: builtins.AddDate}, want: 1}, + {name: "empty collections", hook: hooks.Hook{Type: hooks.HookTypeBuiltin, Command: builtins.AddDate, Args: []string{}, Env: map[string]string{}}, want: 1}, + {name: "named", hook: hooks.Hook{Name: "my date", Type: hooks.HookTypeBuiltin, Command: builtins.AddDate}, want: 2}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + cfg := builtins.ApplyAgentDefaults(&hooks.Config{TurnStart: []hooks.Hook{tc.hook}}, builtins.AgentDefaults{AddDate: true}) + registry := hooks.NewRegistry() + require.NoError(t, builtins.Register(registry)) + exec := hooks.NewExecutorWithRegistry(cfg, "", nil, registry) + for range 2 { + result, err := exec.Dispatch(t.Context(), hooks.EventTurnStart, &hooks.Input{}) + require.NoError(t, err) + assert.Len(t, result.InstructionContext, tc.want) + } + }) + } +} diff --git a/pkg/hooks/dedup_test.go b/pkg/hooks/dedup_test.go index 324b2c3d50..652a307304 100644 --- a/pkg/hooks/dedup_test.go +++ b/pkg/hooks/dedup_test.go @@ -2,6 +2,9 @@ package hooks import ( "context" + "maps" + "reflect" + "strings" "sync/atomic" "testing" @@ -9,14 +12,8 @@ import ( "github.com/stretchr/testify/require" ) -// TestExecutorDedupsByTypeCommandArgs pins that two structurally identical -// hook entries collapse to one invocation, while two builtin hooks with -// the SAME name but DIFFERENT Args remain distinct and both fire. -// -// This is the contract the runtime relies on when WithAddPromptFiles -// auto-injects a hook AND a user explicitly authors another -// add_prompt_files entry with a different file list: both must run. -func TestExecutorDedupsByTypeCommandArgs(t *testing.T) { +// Identical definitions collapse; different builtin arguments remain distinct. +func TestExecutorDedupsIdenticalDefinitions(t *testing.T) { t.Parallel() var calls atomic.Int32 @@ -42,6 +39,178 @@ func TestExecutorDedupsByTypeCommandArgs(t *testing.T) { _, err := exec.Dispatch(t.Context(), EventSessionStart, &Input{SessionID: "s"}) require.NoError(t, err) - // Three distinct (command, args) tuples -> three invocations. + // Three distinct definitions produce three invocations. assert.Equal(t, int32(3), calls.Load()) } + +func TestHookIdentityIncludesEveryField(t *testing.T) { + t.Parallel() + + base := Hook{ + Name: "hook", Type: HookTypeModel, Command: "command", Args: []string{"arg"}, + Timeout: 5, Env: map[string]string{"PROFILE": "first"}, WorkingDir: "first", + OnError: "warn", Model: "test/first", Prompt: "first", Schema: "first", + } + changes := map[string]func(*Hook){ + "Name": func(h *Hook) { h.Name = "other" }, + "Type": func(h *Hook) { h.Type = HookTypeCommand }, + "Command": func(h *Hook) { h.Command = "other" }, + "Args": func(h *Hook) { h.Args = []string{"other"} }, + "Timeout": func(h *Hook) { h.Timeout = 10 }, + "Env": func(h *Hook) { h.Env = map[string]string{"PROFILE": "other"} }, + "WorkingDir": func(h *Hook) { h.WorkingDir = "other" }, + "OnError": func(h *Hook) { h.OnError = "block" }, + "Model": func(h *Hook) { h.Model = "test/other" }, + "Prompt": func(h *Hook) { h.Prompt = "other" }, + "Schema": func(h *Hook) { h.Schema = "other" }, + "StrictOutput": func(h *Hook) { h.StrictOutput = true }, + } + // Adding a config field must also extend identity and its coverage. + for field := range reflect.TypeFor[Hook]().Fields() { + require.Contains(t, changes, field.Name) + } + for name, change := range changes { + t.Run(name, func(t *testing.T) { + t.Parallel() + other := base + change(&other) + exec := NewExecutor(&Config{SessionStart: []Hook{base, other, base, other}}, "", nil) + assert.Equal(t, []Hook{base, other}, exec.hooksFor(EventSessionStart, "")) + }) + } +} + +func TestHookIdentityCollectionsAndBoundaries(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + name string + first Hook + second Hook + duplicates bool + }{ + { + name: "nil and empty collections", duplicates: true, + first: Hook{Type: HookTypeBuiltin, Command: "count"}, + second: Hook{Type: HookTypeBuiltin, Command: "count", Args: []string{}, Env: map[string]string{}}, + }, + { + name: "equal collection contents", duplicates: true, + first: Hook{Args: []string{"a", "b"}, Env: map[string]string{"a": "1", "b": "2"}}, + second: Hook{Args: []string{"a", "b"}, Env: map[string]string{"b": "2", "a": "1"}}, + }, + {name: "argument order", first: Hook{Args: []string{"a", "b"}}, second: Hook{Args: []string{"b", "a"}}}, + {name: "empty argument", first: Hook{}, second: Hook{Args: []string{""}}}, + {name: "empty environment value", first: Hook{}, second: Hook{Env: map[string]string{"PROFILE": ""}}}, + {name: "argument separator", first: Hook{Args: []string{"a\x00b"}}, second: Hook{Args: []string{"a", "b"}}}, + {name: "command separator", first: Hook{Command: "a\x00b"}, second: Hook{Command: "a", Args: []string{"b"}}}, + {name: "explicit timeout", first: Hook{}, second: Hook{Timeout: 60}}, + {name: "explicit error policy", first: Hook{}, second: Hook{OnError: "warn"}}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + exec := NewExecutor(&Config{SessionStart: []Hook{tc.first, tc.second}}, "", nil) + want := []Hook{tc.first} + if !tc.duplicates { + want = append(want, tc.second) + } + assert.Equal(t, want, exec.hooksFor(EventSessionStart, "")) + }) + } +} + +func TestExecutorDedupIsPerDispatchAndKeepsFirstMatch(t *testing.T) { + t.Parallel() + + first := Hook{Type: HookTypeBuiltin, Command: "append", Args: []string{"-first"}} + second := Hook{Type: HookTypeBuiltin, Command: "append", Args: []string{"-second"}} + registry := NewRegistry() + require.NoError(t, registry.RegisterBuiltin("append", func(_ context.Context, in *Input, args []string) (*Output, error) { + return &Output{HookSpecificOutput: &HookSpecificOutput{ + UpdatedInput: map[string]any{"cmd": in.ToolInput["cmd"].(string) + args[0]}, + }}, nil + })) + exec := NewExecutorWithRegistry(&Config{ + PreToolUse: []MatcherConfig{ + {Matcher: "other", Hooks: []Hook{second}}, + {Matcher: "shell", Hooks: []Hook{first}}, + {Matcher: "*", Hooks: []Hook{second, first}}, + }, + SessionStart: []Hook{first}, + }, "", nil, registry) + assert.Equal(t, []Hook{first}, exec.hooksFor(EventSessionStart, "")) + assert.Equal(t, []Hook{second, first}, exec.hooksFor(EventPreToolUse, "other")) + for range 2 { + result, err := exec.Dispatch(t.Context(), EventPreToolUse, &Input{ToolName: "shell", ToolInput: map[string]any{"cmd": "original"}}) + require.NoError(t, err) + assert.Equal(t, "original-first-second", result.ModifiedInput["cmd"]) + } +} + +func TestExecutorRunsDistinctCommandEnvironments(t *testing.T) { + t.Parallel() + + firstDir, secondDir := t.TempDir(), t.TempDir() + command := emitContextEnvPwdCmd("HOOK_PROFILE") + exec := NewExecutor(&Config{SessionStart: []Hook{ + {Type: HookTypeCommand, Command: command, WorkingDir: firstDir, Env: map[string]string{"HOOK_PROFILE": "first"}}, + {Type: HookTypeCommand, Command: command, WorkingDir: secondDir, Env: map[string]string{"HOOK_PROFILE": "second"}}, + }}, "", nil) + result, err := exec.Dispatch(t.Context(), EventSessionStart, &Input{}) + require.NoError(t, err) + assert.Contains(t, result.AdditionalContext, "first:") + assert.Contains(t, result.AdditionalContext, "second:") +} + +func TestExecutorRunsDistinctBuiltinEnvironments(t *testing.T) { + t.Parallel() + + registry := NewRegistry() + require.NoError(t, registry.RegisterBuiltin("env", func(ctx context.Context, _ *Input, _ []string) (*Output, error) { + for _, entry := range EnvFromContext(ctx) { + if strings.HasPrefix(entry, "HOOK_PROFILE=") { + return NewAdditionalContextOutput(EventSessionStart, entry), nil + } + } + return nil, nil + })) + first := Hook{Type: HookTypeBuiltin, Command: "env", Env: map[string]string{"HOOK_PROFILE": "first"}} + second := first + second.Env = maps.Clone(first.Env) + second.Env["HOOK_PROFILE"] = "second" + exec := NewExecutorWithRegistry(&Config{SessionStart: []Hook{first, second}}, "", nil, registry) + result, err := exec.Dispatch(t.Context(), EventSessionStart, &Input{}) + require.NoError(t, err) + assert.Equal(t, "HOOK_PROFILE=first\nHOOK_PROFILE=second", result.AdditionalContext) +} + +func TestExecutorRunsDistinctModelHooks(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + name string + first Hook + }{ + {name: "prompt", first: Hook{Model: "test/judge", Prompt: "first", Schema: ShapePreToolUseDecision}}, + {name: "model", first: Hook{Model: "test/other", Prompt: "second", Schema: ShapePreToolUseDecision}}, + {name: "schema", first: Hook{Model: "test/judge", Prompt: "second"}}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + first := tc.first + first.Type = HookTypeModel + second := Hook{Type: HookTypeModel, Model: "test/judge", Prompt: "second", Schema: ShapePreToolUseDecision} + client := &fakeClient{reply: `{"decision":"deny","reason":"policy"}`} + registry := NewRegistry() + registry.Register(HookTypeModel, NewModelFactory(client)) + exec := NewExecutorWithRegistry(&Config{PreToolUse: []MatcherConfig{{Hooks: []Hook{first, second, second}}}}, "", nil, registry) + result, err := exec.Dispatch(t.Context(), EventPreToolUse, &Input{}) + require.NoError(t, err) + assert.Equal(t, 2, client.calls) + assert.Equal(t, "test/judge", client.gotModel) + assert.Equal(t, "second", client.gotUser) + assert.False(t, result.Allowed) + assert.Equal(t, DecisionDeny, result.Decision) + }) + } +} diff --git a/pkg/hooks/types.go b/pkg/hooks/types.go index 9b0707029d..eebbafdec4 100644 --- a/pkg/hooks/types.go +++ b/pkg/hooks/types.go @@ -338,10 +338,10 @@ type Input struct { // applies the rewrite before the actual provider call. Messages []chat.Message `json:"messages,omitempty"` - // SessionStart specific: "startup", "resume", "clear", "compact". + // SessionStart specific: "startup". // PreCompact specific: "manual", "auto", "overflow", "tool_overflow". Source string `json:"source,omitempty"` - // SessionEnd specific: "clear", "logout", "prompt_input_exit", "other". + // SessionEnd specific: "stream_ended". // TurnEnd specific: "normal", "continue", "steered", "error", // "canceled", "hook_blocked", "loop_detected". Reason string `json:"reason,omitempty"` @@ -578,7 +578,7 @@ type HookSpecificOutput struct { // UpdatedInput is a top-level patch; omitted keys are preserved. UpdatedInput map[string]any `json:"updated_input,omitempty"` - // PostToolUse / SessionStart / TurnStart / Stop fields. + // Context-contributing events (see EventContract). AdditionalContext string `json:"additional_context,omitempty"` InstructionContext []InstructionContext `json:"instruction_context,omitempty"` From 6b1e6ad28c1a1ba1d34a119de5d702bdd03f798e Mon Sep 17 00:00:00 2001 From: David Gageot Date: Wed, 9 Sep 2026 15:23:34 +0200 Subject: [PATCH 3/4] feat(hooks): mandatory tool phases with guards, transforms and strict output Introduce a fixed execution order for tool hooks: tool_input_transform runs unconditionally first (rewrites input), tool_guard runs mandatory approval (guards and rules are rechecked after any input rewrite), then normal on_error/approval hooks proceed. Security guards and pre_tool_use hooks fail closed on unexpected outcomes. Parent-context cancellation is now reported as canceled rather than denied. Consistent errors are returned for unexpected exit codes, malformed JSON and invalid verdicts. opt-in strict_output validates that hook scripts only emit recognized JSON event types; stop and post_tool_use hooks are detected as non-consuming contexts and strict mode flags that. Experimental WASM does not yet implement native mandatory tool phases (documented). --- pkg/hooks/builtins/builtins.go | 4 +- pkg/hooks/config.go | 3 +- pkg/hooks/executor.go | 273 ++++++++--------------- pkg/hooks/failure_test.go | 126 +++++++++++ pkg/hooks/hooks_test.go | 42 ++-- pkg/hooks/model_handler.go | 2 +- pkg/hooks/pipeline.go | 12 +- pkg/hooks/pipeline_test.go | 9 +- pkg/hooks/tool_phases_test.go | 15 +- pkg/runtime/hooks.go | 3 + pkg/runtime/toolexec/dispatcher.go | 36 ++- pkg/runtime/toolexec/tool_ask_test.go | 82 +++++++ pkg/runtime/toolexec/tool_cancel_test.go | 187 ++++++++++++++++ pkg/runtime/toolexec/tool_hooks.go | 62 +++-- pkg/runtime/toolexec/tool_phases_test.go | 2 +- 15 files changed, 593 insertions(+), 265 deletions(-) create mode 100644 pkg/hooks/failure_test.go create mode 100644 pkg/runtime/toolexec/tool_ask_test.go create mode 100644 pkg/runtime/toolexec/tool_cancel_test.go diff --git a/pkg/hooks/builtins/builtins.go b/pkg/hooks/builtins/builtins.go index 8f210cd626..8e04f0141a 100644 --- a/pkg/hooks/builtins/builtins.go +++ b/pkg/hooks/builtins/builtins.go @@ -138,8 +138,8 @@ type AgentDefaults struct { // tool_input_transform, before_llm_call, and tool_response_transform // — the three legs of the feature. Equivalent to writing those three // hook entries by hand; the dedup in [hooks.Executor.hooksFor] - // makes the auto-injection idempotent against an explicit YAML - // entry that already names the same builtin. + // makes auto-injection idempotent against an identical explicit + // entry (including its name and per-hook options). RedactSecrets bool } diff --git a/pkg/hooks/config.go b/pkg/hooks/config.go index e5e40486f5..c6e87f8cb8 100644 --- a/pkg/hooks/config.go +++ b/pkg/hooks/config.go @@ -5,8 +5,7 @@ import ( ) // The persisted hooks types live next to the config schema; the -// runtime uses these short aliases. Adding a new event is a one-line -// change on [latest.HooksConfig] plus one line in compileEvents. +// runtime uses these short aliases. Event capabilities live in pkg/hooks/events. type ( // Config is the hooks configuration for an agent. Config = latest.HooksConfig diff --git a/pkg/hooks/executor.go b/pkg/hooks/executor.go index c626dc7cb3..bb85ad52b2 100644 --- a/pkg/hooks/executor.go +++ b/pkg/hooks/executor.go @@ -2,12 +2,12 @@ package hooks import ( "context" - "encoding/json" "errors" "fmt" "log/slog" "maps" "regexp" + "slices" "strings" "go.opentelemetry.io/otel" @@ -16,6 +16,7 @@ import ( "go.opentelemetry.io/otel/trace" "github.com/docker/docker-agent/pkg/concurrent" + "github.com/docker/docker-agent/pkg/hooks/events" "github.com/docker/docker-agent/pkg/telemetry/genai" ) @@ -68,49 +69,17 @@ func NewExecutorWithRegistry(config *Config, workingDir string, env []string, re } } -// compileEvents builds the per-event matcher lookup. This is the only -// place in the runtime that enumerates events; the persisted side -// owns the struct itself, its IsEmpty, and validate, all on -// [latest.HooksConfig]. Adding a new event is a one-line change here. +// compileEvents compiles the persisted event lists once per executor. func compileEvents(c *Config) map[EventType][]matcher { - flat := func(hooks []Hook) []matcher { - if len(hooks) == 0 { - return nil + compiled := make(map[EventType][]matcher) + for event, matchers := range c.Events() { + if EventType(event) == EventPreToolUse { + compiled[EventPreToolUse], compiled[EventPreToolUsePreYolo] = splitPreToolUseByPreemptYolo(matchers) + } else { + compiled[EventType(event)] = compileMatchers(matchers) } - return []matcher{{hooks: hooks}} - } - preToolUseDefault, preToolUsePreYolo := splitPreToolUseByPreemptYolo(c.PreToolUse) - return map[EventType][]matcher{ - EventPreToolUse: preToolUseDefault, - EventPreToolUsePreYolo: preToolUsePreYolo, - EventPostToolUse: compileMatchers(c.PostToolUse), - EventPermissionRequest: compileMatchers(c.PermissionRequest), - EventSessionStart: flat(c.SessionStart), - EventUserPromptSubmit: flat(c.UserPromptSubmit), - EventUserSteeringMessagesSubmit: flat(c.UserSteeringMessagesSubmit), - EventUserFollowupSubmit: flat(c.UserFollowupSubmit), - EventTurnStart: flat(c.TurnStart), - EventTurnEnd: flat(c.TurnEnd), - EventBeforeLLMCall: flat(c.BeforeLLMCall), - EventAfterLLMCall: flat(c.AfterLLMCall), - EventSessionEnd: flat(c.SessionEnd), - EventPreCompact: flat(c.PreCompact), - EventSubagentStop: flat(c.SubagentStop), - EventOnUserInput: flat(c.OnUserInput), - EventStop: flat(c.Stop), - EventNotification: flat(c.Notification), - EventOnError: flat(c.OnError), - EventOnMaxIterations: flat(c.OnMaxIterations), - EventOnAgentSwitch: flat(c.OnAgentSwitch), - EventOnSessionResume: flat(c.OnSessionResume), - EventOnToolApprovalDecision: flat(c.OnToolApprovalDecision), - EventBeforeCompaction: flat(c.BeforeCompaction), - EventAfterCompaction: flat(c.AfterCompaction), - EventToolResponseTransform: compileMatchers(c.ToolResponseTransform), - EventToolInputTransform: compileMatchers(c.ToolInputTransform), - EventToolGuard: compileMatchers(c.ToolGuard), - EventWorktreeCreate: flat(c.WorktreeCreate), } + return compiled } // splitPreToolUseByPreemptYolo buckets pre_tool_use matcher entries @@ -171,6 +140,12 @@ func (e *Executor) Has(event EventType) bool { // handlers) and aggregation reuses the EventPreToolUse branches. // Tracing keeps the lane visible via the span name. func (e *Executor) Dispatch(ctx context.Context, event EventType, input *Input) (*Result, error) { + if EventContract(event).Name == "" { + return nil, fmt.Errorf("unknown hook event %q", event) + } + if input == nil { + return nil, errors.New("hook input must not be nil") + } hooks := e.hooksFor(event, input.ToolName) if len(hooks) == 0 { return &Result{Allowed: true}, nil @@ -216,22 +191,26 @@ func (e *Executor) Dispatch(ctx context.Context, event EventType, input *Input) span.RecordError(err) span.SetStatus(codes.Error, err.Error()) err = fmt.Errorf("failed to serialize hook input: %w", err) - if isPreApprovalEvent(event) { - // The runtime adapter maps a Dispatch error to "no opinion"; - // pre-approval events must not fall open on a payload bug. - slog.WarnContext(ctx, "Hook input serialization failed; blocking event", "event", event, "error", err) - return &Result{ExitCode: -1, Message: err.Error()}, nil + results := make([]hookResult, len(hooks)) + for i, hook := range hooks { + results[i] = hookResult{hook: hook, err: err} + } + final := aggregate(results, event) + if EventContract(event).CanBlock { + final.Allowed = false + final.ExitCode = -1 + final.Message = err.Error() } - return nil, err + annotateHookSpan(span, event, final) + return final, nil } var final *Result - switch event { - case EventPreToolUse, EventBeforeLLMCall, EventToolResponseTransform, EventToolInputTransform: + if EventContract(event).Sequential() { final = e.runPipeline(ctx, event, hooks, *input, inputJSON) - default: + } else { results := concurrent.MapSlice(hooks, func(hook Hook) hookResult { - return e.runHook(ctx, hook, inputJSON) + return e.runHook(ctx, event, hook, inputJSON) }) final = aggregate(results, event) } @@ -261,7 +240,7 @@ func annotateHookSpan(span trace.Span, event EventType, r *Result) { if r.DecisionReason != "" { attrs = append(attrs, attribute.String("cagent.hook.decision_reason", r.DecisionReason)) } - if event == EventPermissionRequest { + if EventContract(event).PermissionApproval { attrs = append(attrs, attribute.Bool("cagent.hook.permission_allowed", r.PermissionAllowed)) } if r.ModifiedInput != nil { @@ -287,40 +266,38 @@ func annotateHookSpan(span trace.Span, event EventType, r *Result) { span.SetAttributes(attrs...) } -// hooksFor returns the deduplicated list of hooks that should run for -// (event, toolName). Dedup by (type, command, args) catches the common -// case of an explicit YAML hook overlapping a runtime auto-injected -// one (e.g. WithAddDate plus a user-authored add_date entry). +// hooksFor keeps the first matching occurrence of each complete definition. +// Identical user-authored and auto-injected hooks run only once per dispatch. func (e *Executor) hooksFor(event EventType, toolName string) []Hook { - seen := make(map[string]bool) var hooks []Hook for _, m := range e.events[event] { if !m.matches(toolName) { continue } for _, h := range m.hooks { - key := dedupKey(h) - if seen[key] { + if slices.ContainsFunc(hooks, func(existing Hook) bool { return sameHook(existing, h) }) { continue } - seen[key] = true hooks = append(hooks, h) } } return hooks } -// dedupKey returns a deterministic key identifying a hook by (type, command, args). -func dedupKey(h Hook) string { - var b strings.Builder - b.WriteString(h.Type) - b.WriteByte(0) - b.WriteString(h.Command) - for _, a := range h.Args { - b.WriteByte(0) - b.WriteString(a) - } - return b.String() +// sameHook compares configured values without expanding env or working_dir. +func sameHook(a, b Hook) bool { + return a.Name == b.Name && + a.Type == b.Type && + a.Command == b.Command && + slices.Equal(a.Args, b.Args) && + a.Timeout == b.Timeout && + maps.Equal(a.Env, b.Env) && + a.WorkingDir == b.WorkingDir && + a.OnError == b.OnError && + a.StrictOutput == b.StrictOutput && + a.Model == b.Model && + a.Prompt == b.Prompt && + a.Schema == b.Schema } // hookResult is the outcome of a single hook invocation: the raw @@ -338,7 +315,7 @@ type hookResult struct { // runHook resolves the hook's [HookType] in the registry, applies its // timeout, and returns the structured outcome. JSON-on-stdout is parsed // into [Output] when the handler didn't already provide one. -func (e *Executor) runHook(ctx context.Context, hook Hook, inputJSON []byte) hookResult { +func (e *Executor) runHook(ctx context.Context, event EventType, hook Hook, inputJSON []byte) hookResult { factory, ok := e.registry.Lookup(hook.Type) if !ok { return hookResult{hook: hook, err: fmt.Errorf("unsupported hook type: %s", hook.Type)} @@ -383,112 +360,55 @@ func (e *Executor) runHook(ctx context.Context, hook Hook, inputJSON []byte) hoo // Fall back to the legacy "parse JSON from stdout" protocol. if r.Output == nil && r.ExitCode == 0 { - r.Output = parseStdoutJSON(r.Stdout) + r.Output, err = parseStdoutJSON(r.Stdout, hook.StrictOutput) + if err != nil { + return markFailed(err) + } } - return r -} - -// parseStdoutJSON returns a parsed [Output] when stdout begins with '{' -// and decodes cleanly, or nil otherwise. Used for the legacy "JSON on -// stdout" hook protocol where handlers don't pre-populate -// [HandlerResult.Output]. -func parseStdoutJSON(stdout string) *Output { - s := strings.TrimSpace(stdout) - if !strings.HasPrefix(s, "{") { - return nil + if r.ExitCode != 0 && r.ExitCode != 2 { + return markFailed(fmt.Errorf("exited with status %d", r.ExitCode)) } - var parsed Output - if err := json.Unmarshal([]byte(s), &parsed); err != nil { - return nil + if r.Output != nil && r.ExitCode == 0 { + if err := validateOutput(event, r.Output, hook.StrictOutput); err != nil { + return markFailed(err) + } } - return &parsed -} - -// isPreToolUseLane reports whether event is one of the two pre_tool_use -// dispatch lanes, which share verdict semantics. -func isPreToolUseLane(event EventType) bool { - return event == EventPreToolUse || event == EventPreToolUsePreYolo -} - -// isPreApprovalEvent reports whether event always runs before the -// deterministic approval pipeline. A dispatch that cannot even run its -// hooks must block rather than be treated as "no opinion". -func isPreApprovalEvent(event EventType) bool { - return event == EventToolInputTransform || event == EventToolGuard || event == EventPreToolUsePreYolo -} - -// carriesDecision reports whether event's PermissionDecision verdicts -// aggregate into [Result.Decision]. -func carriesDecision(event EventType) bool { - return isPreToolUseLane(event) || event == EventToolGuard -} - -// rewritesToolInput reports whether event honours UpdatedInput. -func rewritesToolInput(event EventType) bool { - return event == EventPreToolUse || event == EventToolInputTransform -} - -// collectsMetadata reports whether event merges hook Metadata into -// [Result.Metadata] for the confirmation prompt. -func collectsMetadata(event EventType) bool { - return event == EventPermissionRequest || event == EventPreToolUsePreYolo || event == EventToolGuard -} - -// failClosed reports whether a hook failure on event must deny the -// event. PreToolUse (both lanes) and tool_guard are hard security -// boundaries: a crashed safety hook must not silently allow the call -// through. Every other event surfaces failures as warnings unless the -// hook opts into ErrorPolicyBlock. -func failClosed(event EventType) bool { - return isPreToolUseLane(event) || event == EventToolGuard -} - -// stdoutAsContext reports whether plain stdout (non-JSON, exit 0) -// from a hook should be routed into Result.AdditionalContext. It is -// the runtime's emit site that decides whether AdditionalContext is -// surfaced; events that don't consume it MUST drop plain stdout so -// hook authors don't think their output mattered when it would have -// been thrown away. -func stdoutAsContext(event EventType) bool { - switch event { - case EventPostToolUse, - EventSessionStart, - EventUserPromptSubmit, - EventUserSteeringMessagesSubmit, - EventUserFollowupSubmit, - EventTurnStart, - EventPreCompact, - EventStop, - EventWorktreeCreate: - return true - } - return false + return r } // aggregate combines per-hook results into a single [Result]. func aggregate(results []hookResult, event EventType) *Result { + contract := EventContract(event) final := &Result{Allowed: true} var messages, contexts, sysMsgs []string for _, r := range results { + if r.err == nil && r.ExitCode != 0 && r.ExitCode != 2 { + r.err = fmt.Errorf("exited with status %d", r.ExitCode) + } switch { case r.err != nil: policy := ErrorPolicy(r.hook.OnError) if policy == "" { policy = ErrorPolicyWarn } - if failClosed(event) || policy == ErrorPolicyBlock { + if contract.FailClosed || (contract.CanBlock && policy == ErrorPolicyBlock) { slog.Warn("Hook failed; blocking event", "hook", r.hook.DisplayName(), "error", r.err) final.Allowed = false final.ExitCode = -1 final.Stderr = r.Stderr - messages = append(messages, hookFailureMessage(event, r.err)) + messages = append(messages, hookFailureMessage(event, fmt.Errorf("hook %q: %w", r.hook.DisplayName(), r.err))) } else if policy != ErrorPolicyIgnore { slog.Warn("Hook execution error", "hook", r.hook.DisplayName(), "error", r.err) + sysMsgs = append(sysMsgs, hookFailureMessage(event, fmt.Errorf("hook %q: %w", r.hook.DisplayName(), r.err))) } continue case r.ExitCode == 2: + if !contract.CanBlock { + sysMsgs = append(sysMsgs, fmt.Sprintf("%s hook %q returned exit 2, but this event cannot block", contract.Name, r.hook.DisplayName())) + continue + } final.Allowed = false final.ExitCode = 2 if r.Stderr != "" { @@ -497,27 +417,26 @@ func aggregate(results []hookResult, event EventType) *Result { } continue - case r.ExitCode != 0: - slog.Debug("Hook returned non-zero exit code", "exit_code", r.ExitCode, "stderr", r.Stderr) - continue - case r.Output == nil: // Plain stdout becomes AdditionalContext only for events // whose runtime consumes it. - if r.Stdout != "" && stdoutAsContext(event) { + if r.Stdout != "" && contract.Context { contexts = append(contexts, strings.TrimSpace(r.Stdout)) } continue } out := r.Output - if !out.ShouldContinue() { + if !contract.CanBlock && (!out.ShouldContinue() || out.IsBlocked()) { + sysMsgs = append(sysMsgs, fmt.Sprintf("%s hook %q returned a block, but this event cannot block", contract.Name, r.hook.DisplayName())) + } + if contract.CanBlock && !out.ShouldContinue() { final.Allowed = false if out.StopReason != "" { messages = append(messages, out.StopReason) } } - if out.IsBlocked() { + if contract.CanBlock && out.IsBlocked() { final.Allowed = false if out.Reason != "" { messages = append(messages, out.Reason) @@ -527,13 +446,16 @@ func aggregate(results []hookResult, event EventType) *Result { sysMsgs = append(sysMsgs, out.SystemMessage) } if hso := out.HookSpecificOutput; hso != nil { - if carriesDecision(event) && hso.PermissionDecision != "" { + if !contract.Permission() && hso.PermissionDecision != "" { + sysMsgs = append(sysMsgs, fmt.Sprintf("%s hook %q returned permission_decision, but this event does not support approval decisions", contract.Name, r.hook.DisplayName())) + } + if contract.Decision && hso.PermissionDecision != "" { final.Decision, final.DecisionReason = strongerDecision( final.Decision, final.DecisionReason, hso.PermissionDecision, hso.PermissionDecisionReason, ) } - if carriesDecision(event) || event == EventPermissionRequest { + if contract.Decision || contract.PermissionApproval { switch hso.PermissionDecision { case DecisionDeny: final.Allowed = false @@ -541,21 +463,21 @@ func aggregate(results []hookResult, event EventType) *Result { messages = append(messages, hso.PermissionDecisionReason) } case DecisionAllow: - if event == EventPermissionRequest { + if contract.PermissionApproval { final.PermissionAllowed = true - } - if hso.PermissionDecisionReason != "" { - contexts = append(contexts, hso.PermissionDecisionReason) + if final.DecisionReason == "" { + final.DecisionReason = hso.PermissionDecisionReason + } } } } - if rewritesToolInput(event) && hso.UpdatedInput != nil { + if contract.Rewrite == events.RewriteToolInput && hso.UpdatedInput != nil { if final.ModifiedInput == nil { final.ModifiedInput = make(map[string]any) } maps.Copy(final.ModifiedInput, hso.UpdatedInput) } - if event == EventBeforeCompaction && hso.Summary != "" && final.Summary == "" { + if contract.Summary && hso.Summary != "" && final.Summary == "" { // First non-empty summary in CONFIG ORDER wins. Hooks run // concurrently (see runHook above), but we iterate // `results` in the order they were configured — the index @@ -568,13 +490,13 @@ func aggregate(results []hookResult, event EventType) *Result { // is to skip the LLM entirely). final.Summary = hso.Summary } - if event == EventBeforeLLMCall && len(hso.UpdatedMessages) > 0 { + if contract.Rewrite == events.RewriteMessages && len(hso.UpdatedMessages) > 0 { final.UpdatedMessages = hso.UpdatedMessages } - if event == EventToolResponseTransform && hso.UpdatedToolResponse != nil { + if contract.Rewrite == events.RewriteToolResponse && hso.UpdatedToolResponse != nil { final.UpdatedToolResponse = hso.UpdatedToolResponse } - if collectsMetadata(event) && len(hso.Metadata) > 0 { + if contract.Metadata && len(hso.Metadata) > 0 { // Metadata from every matching hook is merged so multiple // hooks can each contribute keys. On a key clash the last // hook in config order wins (results is iterated in @@ -584,10 +506,12 @@ func aggregate(results []hookResult, event EventType) *Result { } maps.Copy(final.Metadata, hso.Metadata) } - if hso.AdditionalContext != "" { + if contract.Context && hso.AdditionalContext != "" { contexts = append(contexts, hso.AdditionalContext) } - final.InstructionContext = append(final.InstructionContext, hso.InstructionContext...) + if contract.Instructions { + final.InstructionContext = append(final.InstructionContext, hso.InstructionContext...) + } } } @@ -597,13 +521,8 @@ func aggregate(results []hookResult, event EventType) *Result { return final } -// hookFailureMessage keeps the historical wording for pre_tool_use -// lanes and names the event otherwise. func hookFailureMessage(event EventType, err error) string { - if isPreToolUseLane(event) { - return fmt.Sprintf("PreToolUse hook failed to execute: %v", err) - } - return fmt.Sprintf("%s hook failed to execute: %v", event, err) + return fmt.Sprintf("%s hook failed to execute: %v", EventContract(event).Name, err) } // decisionWeight ranks PermissionDecision verdicts so [strongerDecision] diff --git a/pkg/hooks/failure_test.go b/pkg/hooks/failure_test.go new file mode 100644 index 0000000000..566f9a8703 --- /dev/null +++ b/pkg/hooks/failure_test.go @@ -0,0 +1,126 @@ +package hooks + +import ( + "context" + "errors" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestHookFailurePolicyMatrix(t *testing.T) { + t.Parallel() + + for _, event := range []EventType{EventPreToolUse, EventToolGuard, EventToolInputTransform, EventBeforeLLMCall} { + for _, policy := range []string{"", "warn", "ignore", "block"} { + for _, tc := range []struct { + name string + result HandlerResult + err error + }{ + {name: "execution", err: errors.New("failure")}, + {name: "timeout", err: context.DeadlineExceeded}, + {name: "exit 1", result: HandlerResult{ExitCode: 1}}, + {name: "missing dependency", result: HandlerResult{ExitCode: 127}}, + {name: "malformed JSON", result: HandlerResult{Stdout: `{"decision":`}}, + {name: "bad decision", result: HandlerResult{Stdout: `{"decision":"blok"}`}}, + {name: "bad permission", result: HandlerResult{Stdout: `{"hook_specific_output":{"permission_decision":"alow"}}`}}, + } { + t.Run(string(event)+"/"+policy+"/"+tc.name, func(t *testing.T) { + t.Parallel() + exec := pipelineTestExecutor(t, event, []Hook{{Type: "fail", Command: "test", OnError: policy}}, failingHandlerRegistry(tc.result, tc.err)) + result, err := exec.Dispatch(t.Context(), event, &Input{}) + require.NoError(t, err) + blocked := EventContract(event).FailClosed || policy == "block" + assert.Equal(t, !blocked, result.Allowed) + switch { + case blocked: + assert.Equal(t, -1, result.ExitCode) + assert.Contains(t, result.Message, string(event)+" hook failed") + assert.Contains(t, result.Message, `hook "test"`) + case policy == "ignore": + assert.Empty(t, result.SystemMessage) + default: + assert.Contains(t, result.SystemMessage, "hook failed") + } + }) + } + } + } +} + +func TestStrictHookOutputProtocol(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name, stdout string + strict, fail bool + }{ + {name: "plain compatibility", stdout: "context"}, + {name: "strict plain", stdout: "context", strict: true, fail: true}, + {name: "empty", strict: true}, + {name: "JSON", stdout: `{}`, strict: true}, + {name: "unknown field", stdout: `{"unknown_field":false}`, strict: true, fail: true}, + {name: "unknown nested field", stdout: `{"hook_specific_output":{"updated_inpu":{}}}`, strict: true, fail: true}, + {name: "multiple objects", stdout: `{} {}`, fail: true}, + {name: "JSON log suffix", stdout: `{} done`, fail: true}, + {name: "array", stdout: `[]`, strict: true, fail: true}, + {name: "null", stdout: `null`, strict: true, fail: true}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + _, err := parseStdoutJSON(tc.stdout, tc.strict) + if tc.fail { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + }) + } +} + +func TestStrictHookRejectsWrongEventAndUnsupportedOutput(t *testing.T) { + t.Parallel() + for _, stdout := range []string{ + `{"hook_specific_output":{"hook_event_name":"stop"}}`, + `{"hook_specific_output":{"updated_input":{"cmd":"rewrite"}}}`, + } { + exec := pipelineTestExecutor(t, EventToolGuard, []Hook{{Type: "fail", Command: "guard", StrictOutput: true}}, failingHandlerRegistry(HandlerResult{Stdout: stdout}, nil)) + result, err := exec.Dispatch(t.Context(), EventToolGuard, &Input{}) + require.NoError(t, err) + assert.False(t, result.Allowed) + assert.Nil(t, result.ModifiedInput) + } +} + +func TestUnsupportedBlockOutputsWarn(t *testing.T) { + t.Parallel() + for _, res := range []HandlerResult{ + {ExitCode: 2}, + {Stdout: `{"decision":"block"}`}, + {Stdout: `{"continue":false}`}, + {Stdout: `{"hook_specific_output":{"permission_decision":"deny"}}`}, + } { + exec := NewExecutorWithRegistry(&Config{Stop: []Hook{{Type: "fail", Command: "stop"}}}, "", nil, failingHandlerRegistry(res, nil)) + result, err := exec.Dispatch(t.Context(), EventStop, &Input{}) + require.NoError(t, err) + assert.True(t, result.Allowed) + assert.NotEmpty(t, result.SystemMessage) + } +} + +func TestOutputEventNameIsCompatibilityMetadata(t *testing.T) { + t.Parallel() + out := &Output{HookSpecificOutput: &HookSpecificOutput{HookEventName: EventTurnStart, AdditionalContext: "context"}} + require.NoError(t, validateOutput(EventSessionStart, out, false)) + require.Error(t, validateOutput(EventSessionStart, out, true)) +} + +func TestInvalidDispatchInput(t *testing.T) { + t.Parallel() + exec := NewExecutor(&Config{PreToolUse: []MatcherConfig{{Hooks: trueHook}}}, "", nil) + _, err := exec.Dispatch(t.Context(), EventPreToolUse, nil) + require.ErrorContains(t, err, "input must not be nil") + _, err = exec.Dispatch(t.Context(), EventType("unknown"), &Input{}) + require.ErrorContains(t, err, "unknown hook event") +} diff --git a/pkg/hooks/hooks_test.go b/pkg/hooks/hooks_test.go index eb3eaa6b1a..69731727aa 100644 --- a/pkg/hooks/hooks_test.go +++ b/pkg/hooks/hooks_test.go @@ -576,28 +576,21 @@ func TestExecuteStop(t *testing.T) { result, err := exec.Dispatch(t.Context(), EventStop, input) require.NoError(t, err) assert.True(t, result.Allowed) - assert.Contains(t, result.AdditionalContext, "model stopped") + assert.Empty(t, result.AdditionalContext) } func TestExecuteStopReceivesResponseContent(t *testing.T) { t.Parallel() - - config := &Config{ - Stop: []Hook{ - {Type: HookTypeCommand, Command: printStdinJSONFieldCmd("stop_response"), Timeout: 5}, - }, - } - - exec := NewExecutor(config, t.TempDir(), nil) - input := &Input{ - SessionID: "test-session", - StopResponse: "final answer content", - } - - result, err := exec.Dispatch(t.Context(), EventStop, input) + // Exercise the process protocol directly: stop's stdout is observational. + factory, ok := NewRegistry().Lookup(HookTypeCommand) + require.True(t, ok) + handler, err := factory(HandlerEnv{WorkingDir: t.TempDir()}, Hook{Command: printStdinJSONFieldCmd("stop_response")}) require.NoError(t, err) - assert.True(t, result.Allowed) - assert.Contains(t, result.AdditionalContext, "final answer content") + input, err := (&Input{HookEventName: EventStop, StopResponse: "final answer content"}).ToJSON() + require.NoError(t, err) + result, err := handler.Run(t.Context(), input) + require.NoError(t, err) + assert.Contains(t, result.Stdout, "final answer content") } func TestExecuteNotification(t *testing.T) { @@ -674,14 +667,10 @@ func TestExecuteHooksWithContextCancellation(t *testing.T) { // silently allowed. assert.False(t, result.Allowed) assert.Equal(t, -1, result.ExitCode) - assert.Contains(t, result.Message, "PreToolUse hook failed to execute") + assert.Contains(t, result.Message, "pre_tool_use hook failed to execute") } -// A hook that exits with a non-zero, non-2 code is a non-blocking error: -// it is reported as such in the result but does not deny the tool call. -// Pair this with TestExecuteHooksWithContextCancellation, which asserts the -// opposite for execution failures (timeout, spawn error). -func TestExecutePreToolUseAllowsNonBlockingExitCode(t *testing.T) { +func TestExecutePreToolUseFailsClosedOnNonzeroExit(t *testing.T) { t.Parallel() config := &Config{ @@ -704,7 +693,8 @@ func TestExecutePreToolUseAllowsNonBlockingExitCode(t *testing.T) { result, err := exec.Dispatch(t.Context(), EventPreToolUse, input) require.NoError(t, err) - assert.True(t, result.Allowed) + assert.False(t, result.Allowed) + assert.Contains(t, result.Message, "exited with status 1") } // TestPlainStdoutBecomesAdditionalContext pins the contract that a @@ -722,10 +712,10 @@ func TestPlainStdoutBecomesAdditionalContext(t *testing.T) { t.Parallel() contextEvents := []EventType{ - EventSessionStart, EventTurnStart, EventPostToolUse, EventStop, + EventSessionStart, EventTurnStart, } observationalEvents := []EventType{ - EventBeforeLLMCall, EventAfterLLMCall, EventOnError, + EventBeforeLLMCall, EventAfterLLMCall, EventOnError, EventPostToolUse, EventStop, EventOnMaxIterations, EventNotification, EventOnUserInput, EventSessionEnd, EventBeforeCompaction, EventAfterCompaction, EventTurnEnd, } diff --git a/pkg/hooks/model_handler.go b/pkg/hooks/model_handler.go index c7388cfc4e..0df1958b2a 100644 --- a/pkg/hooks/model_handler.go +++ b/pkg/hooks/model_handler.go @@ -99,7 +99,7 @@ func lookupSchema(name string) *latest.StructuredOutput { } // defaultShape passes the model's reply through as additional_context. -// Useful for turn_start summarizers, post_tool_use commentary, etc. — +// Useful for turn_start summarizers and other context events. — // any event where the runtime consumes AdditionalContext. func defaultShape(raw string, in *Input) (*Output, error) { if in == nil { diff --git a/pkg/hooks/pipeline.go b/pkg/hooks/pipeline.go index f87f03ef2a..2633b2fb63 100644 --- a/pkg/hooks/pipeline.go +++ b/pkg/hooks/pipeline.go @@ -4,6 +4,8 @@ import ( "context" "fmt" "maps" + + "github.com/docker/docker-agent/pkg/hooks/events" ) // runPipeline passes each accepted rewrite to the next hook without changing @@ -11,7 +13,7 @@ import ( func (e *Executor) runPipeline(ctx context.Context, event EventType, hooks []Hook, input Input, inputJSON []byte) *Result { results := make([]hookResult, 0, len(hooks)) for _, hook := range hooks { - results = append(results, e.runHook(ctx, hook, inputJSON)) + results = append(results, e.runHook(ctx, event, hook, inputJSON)) r := &results[len(results)-1] if r.err != nil || r.ExitCode != 0 || r.Output == nil || r.Output.HookSpecificOutput == nil { continue @@ -41,8 +43,8 @@ func (e *Executor) runPipeline(ctx context.Context, event EventType, hooks []Hoo } func rewrittenInput(input Input, event EventType, out *HookSpecificOutput) (Input, bool) { - switch event { - case EventPreToolUse, EventToolInputTransform: + switch EventContract(event).Rewrite { + case events.RewriteToolInput: if out.UpdatedInput == nil { return input, false } @@ -51,12 +53,12 @@ func rewrittenInput(input Input, event EventType, out *HookSpecificOutput) (Inpu input.ToolInput = make(map[string]any) } maps.Copy(input.ToolInput, out.UpdatedInput) - case EventBeforeLLMCall: + case events.RewriteMessages: if len(out.UpdatedMessages) == 0 { return input, false } input.Messages = out.UpdatedMessages - case EventToolResponseTransform: + case events.RewriteToolResponse: if out.UpdatedToolResponse == nil { return input, false } diff --git a/pkg/hooks/pipeline_test.go b/pkg/hooks/pipeline_test.go index 504bafb8d6..980e875951 100644 --- a/pkg/hooks/pipeline_test.go +++ b/pkg/hooks/pipeline_test.go @@ -197,7 +197,8 @@ func TestPipelineFailuresKeepPriorRewriteAndRunRemainingHooks(t *testing.T) { }) require.NoError(t, err) assert.True(t, lastRan) - assert.Equal(t, !tc.blocked, result.Allowed) + blocked := tc.blocked && EventContract(event).CanBlock || tc.result.ExitCode == 1 && EventContract(event).FailClosed + assert.Equal(t, !blocked, result.Allowed) switch event { case EventPreToolUse, EventToolInputTransform: assert.Equal(t, "first", result.ModifiedInput["cmd"]) @@ -285,7 +286,11 @@ func TestNonTransformEventsRemainConcurrent(t *testing.T) { result, err := exec.Dispatch(ctx, event, &Input{ToolInput: map[string]any{"cmd": "original"}}) require.NoError(t, err) require.NoError(t, ctx.Err(), "both hooks must start before either finishes") - assert.Equal(t, "first\nsecond", result.AdditionalContext) + if EventContract(event).Context { + assert.Equal(t, "first\nsecond", result.AdditionalContext) + } else { + assert.Empty(t, result.AdditionalContext) + } assert.Nil(t, result.ModifiedInput) if event == EventBeforeCompaction { assert.Equal(t, "first", result.Summary) diff --git a/pkg/hooks/tool_phases_test.go b/pkg/hooks/tool_phases_test.go index df6c41ad4f..34999b6068 100644 --- a/pkg/hooks/tool_phases_test.go +++ b/pkg/hooks/tool_phases_test.go @@ -160,8 +160,7 @@ func TestToolGuardAggregatesMostRestrictiveVerdict(t *testing.T) { }) } -// Guard failures fail closed regardless of on_error; non-blocking exit -// codes keep their legacy meaning. +// Guard failures, including unexpected exit codes, fail closed regardless of on_error. func TestToolGuardFailsClosed(t *testing.T) { t.Parallel() @@ -176,7 +175,7 @@ func TestToolGuardFailsClosed(t *testing.T) { {name: "error default", err: errors.New("boom"), allowed: false, exitCode: -1}, {name: "error ignore still denies", err: errors.New("boom"), onError: "ignore", allowed: false, exitCode: -1}, {name: "canceled", err: context.Canceled, allowed: false, exitCode: -1}, - {name: "exit 1", result: HandlerResult{ExitCode: 1}, allowed: true}, + {name: "exit 1", result: HandlerResult{ExitCode: 1}, allowed: false, exitCode: -1}, {name: "exit 2", result: HandlerResult{ExitCode: 2, Stderr: "nope"}, allowed: false, exitCode: 2}, } { t.Run(tc.name, func(t *testing.T) { @@ -198,9 +197,7 @@ func TestToolGuardFailsClosed(t *testing.T) { } } -// A payload the executor cannot serialize must block pre-approval events -// (the runtime adapter treats a Dispatch error as "no opinion") while -// legacy events keep returning the error. +// A payload the executor cannot serialize must block every block-capable event. func TestDispatchSerializationFailureBlocksPreApprovalEvents(t *testing.T) { t.Parallel() @@ -212,7 +209,7 @@ func TestDispatchSerializationFailureBlocksPreApprovalEvents(t *testing.T) { exec := NewExecutor(cfg, t.TempDir(), nil) unserializable := map[string]any{"bad": make(chan int)} - for _, event := range []EventType{EventToolInputTransform, EventToolGuard, EventPreToolUsePreYolo} { + for _, event := range []EventType{EventToolInputTransform, EventToolGuard, EventPreToolUsePreYolo, EventPreToolUse} { result, err := exec.Dispatch(t.Context(), event, &Input{ToolName: "shell", ToolInput: unserializable}) require.NoError(t, err, event) require.NotNil(t, result, event) @@ -220,8 +217,4 @@ func TestDispatchSerializationFailureBlocksPreApprovalEvents(t *testing.T) { assert.Equal(t, -1, result.ExitCode, event) assert.Contains(t, result.Message, "failed to serialize hook input", event) } - - result, err := exec.Dispatch(t.Context(), EventPreToolUse, &Input{ToolName: "shell", ToolInput: unserializable}) - require.Error(t, err) - assert.Nil(t, result) } diff --git a/pkg/runtime/hooks.go b/pkg/runtime/hooks.go index b4d7854c58..dd2fc7d98b 100644 --- a/pkg/runtime/hooks.go +++ b/pkg/runtime/hooks.go @@ -123,6 +123,9 @@ func (r *LocalRuntime) dispatchHook( } if err != nil { slog.WarnContext(ctx, "Hook execution failed", "event", event, "agent", a.Name(), "error", err) + if hooks.EventContract(event).CanBlock { + return &hooks.Result{ExitCode: -1, Message: err.Error()} + } return nil } diff --git a/pkg/runtime/toolexec/dispatcher.go b/pkg/runtime/toolexec/dispatcher.go index 849f970b4c..5d5d294d15 100644 --- a/pkg/runtime/toolexec/dispatcher.go +++ b/pkg/runtime/toolexec/dispatcher.go @@ -390,10 +390,8 @@ func (c *call) run(ctx context.Context) CallOutcome { slog.DebugContext(ctx, "Processing tool call", "agent", c.a.Name(), "tool", c.tc.Function.Name, "session_id", c.sess.ID) if ctx.Err() != nil { - msg := c.cancellationMessage(ctx) - c.errorResponse(ctx, msg) - span.SetStatus(codes.Ok, msg) - return c.cancellationOutcome(ctx) + span.SetStatus(codes.Ok, c.cancellationMessage(ctx)) + return c.canceled(ctx) } // After a handoff the model may hallucinate tools it saw earlier in @@ -436,8 +434,8 @@ func (c *call) run(ctx context.Context) CallOutcome { // permission rules / safety mode → legacy approval hooks → user confirmation. // Approval hooks only run when the safety mode asks, not on auto-approved calls. func (c *call) approveAndRun(ctx context.Context, runTool func() CallOutcome) CallOutcome { - if c.transformToolInput(ctx) { - return CallOutcome{} + if outcome, handled := c.transformToolInput(ctx); handled { + return outcome } if outcome, handled := c.runToolGuards(ctx, runTool); handled { return outcome @@ -636,6 +634,9 @@ func (c *call) consultPreToolUseHook(ctx context.Context, runTool func() CallOut } result := c.d.Hooks.Dispatch(ctx, c.a, hooks.EventPreToolUse, NewHooksInput(c.sess, c.tc)) + if outcome, canceled := c.hookCanceled(ctx); canceled { + return outcome, true + } if result == nil { return CallOutcome{}, false } @@ -647,10 +648,7 @@ func (c *call) consultPreToolUseHook(ctx context.Context, runTool func() CallOut } if !result.Allowed { - slog.DebugContext(ctx, "Pre-tool hook blocked tool call", "tool", c.tc.Function.Name, "message", result.Message) - c.notifyApproval(ctx, ApprovalDecisionDeny, ApprovalSourcePreToolUseHookDeny) - c.em.EmitHookBlocked(c.tc, c.tool, result.Message, c.a.Name()) - c.errorResponse(ctx, "Tool call blocked by hook: "+result.Message) + c.blockToolHook(ctx, hooks.EventPreToolUse, ApprovalSourcePreToolUseHookDeny, cmp.Or(result.Message, result.DecisionReason)) return CallOutcome{}, true } @@ -885,28 +883,20 @@ func (c *call) runPermissionRequestHook(ctx context.Context, runTool func() Call ToolInput: ParseToolInput(c.tc.Function.Arguments), SafetyPolicy: string(c.sess.GetSafetyPolicy()), }) + if outcome, canceled := c.hookCanceled(ctx); canceled { + return outcome, true, nil + } if result == nil { return CallOutcome{}, false, nil } if !result.Allowed { - slog.DebugContext(ctx, "Tool denied by permission_request hook", "tool", toolName, "session_id", c.sess.ID, "reason", result.Message) - // Stamp the deny on the runtime.tool.call span via notifyApproval - // before returning. Without this the span would end with status - // Ok and no cagent.approval.* attrs — denied-by-hook calls would - // look identical to successful ones in trace dashboards, while - // pre_tool_use deny does emit the attrs. Symmetry matters. - c.notifyApproval(ctx, ApprovalDecisionDeny, ApprovalSourcePermissionRequestHookDeny) - rejectMsg := "The tool call was rejected by a permission_request hook." - if reason := strings.TrimSpace(result.Message); reason != "" { - rejectMsg += " Reason: " + reason - } - c.errorResponse(ctx, rejectMsg) + c.blockToolHook(ctx, hooks.EventPermissionRequest, ApprovalSourcePermissionRequestHookDeny, result.Message) return CallOutcome{}, true, nil } if result.PermissionAllowed { - slog.DebugContext(ctx, "Tool auto-approved by permission_request hook", "tool", toolName, "session_id", c.sess.ID, "reason", result.AdditionalContext) + slog.DebugContext(ctx, "Tool auto-approved by permission_request hook", "tool", toolName, "session_id", c.sess.ID, "reason", result.DecisionReason) c.notifyApproval(ctx, ApprovalDecisionAllow, ApprovalSourcePermissionRequestHookAllow) return runTool(), true, nil } diff --git a/pkg/runtime/toolexec/tool_ask_test.go b/pkg/runtime/toolexec/tool_ask_test.go new file mode 100644 index 0000000000..d55a89b8fa --- /dev/null +++ b/pkg/runtime/toolexec/tool_ask_test.go @@ -0,0 +1,82 @@ +package toolexec_test + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/docker/docker-agent/pkg/hooks" + "github.com/docker/docker-agent/pkg/runtime/toolexec" + "github.com/docker/docker-agent/pkg/session" + "github.com/docker/docker-agent/pkg/tools" +) + +// With nobody at the keyboard a mandatory Ask must deny, even under +// autonomous mode and with a matching session grant. +func TestToolPhases_MandatoryAskDeniesNonInteractive(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + name string + cfg *hooks.Config + }{ + {name: "tool_guard ask", cfg: &hooks.Config{ToolGuard: matchAll(hook(bVerdict, "ask", "needs a human"))}}, + {name: "preempt ask", cfg: &hooks.Config{PreToolUse: preemptYolo(hook(bVerdict, "ask", "needs a human"))}}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + tc.cfg.PermissionRequest = matchAll(hook(bPermit)) + hd, rec := newPhaseHooks(t, tc.cfg) + sess := session.New(session.WithSafetyPolicy(session.SafetyPolicyAutonomous)) + sess.NonInteractive = true + sess.Permissions = &session.PermissionsConfig{Allow: []string{"shell:cmd=rm*"}} + d := newDispatcher(hd, nil) + d.Permissions = sessionCheckers + em := &captureEmitter{} + + d.Process(t.Context(), sess, []tools.ToolCall{shellCall("x", "rm -rf / && echo")}, []tools.Tool{neverRun()}, em) + + requireDenied(t, em, hd, toolexec.ApprovalSourceNonInteractiveDeny, "requires user confirmation but the session is non-interactive") + assert.Empty(t, em.hookBlocks, "a non-interactive deny is not a hook block") + assert.Zero(t, rec.count(hooks.EventPermissionRequest), "permission_request must not fire behind a mandatory ask") + }) + } +} + +// When both mandatory lanes ask, the user is prompted once with both lanes' +// metadata (guard wins a key clash), permission_request stays skipped and the +// session grant does not silence the prompt. +func TestToolPhases_GuardAndPreemptAskPromptOnce(t *testing.T) { + t.Parallel() + hd, rec := newPhaseHooks(t, &hooks.Config{ + ToolGuard: matchAll(hook(bVerdict, "ask", "guard says review", "category=deploy", "owner=guard")), + PreToolUse: append( + preemptYolo(hook(bVerdict, "ask", "preempt says review", "category=fs-delete", "blast_radius=high")), + matchAll(hook(bVerdict, "allow", "legacy would allow"))..., + ), + PermissionRequest: matchAll(hook(bPermit)), + }) + sess := session.New(session.WithSafetyPolicy(session.SafetyPolicyAutonomous)) + sess.Permissions = &session.PermissionsConfig{Allow: []string{"shell:cmd=mkdir*"}} + resume := make(chan toolexec.ResumeRequest, 1) + resume <- toolexec.ResumeRequest{Type: toolexec.ResumeTypeApprove} + d := newDispatcher(hd, resume) + d.Permissions = sessionCheckers + var got []string + em := &captureEmitter{} + + d.Process(t.Context(), sess, []tools.ToolCall{shellCall("x", "mkdir out")}, []tools.Tool{shellTool(&got)}, em) + + require.Len(t, em.confirmations, 1) + assert.Equal(t, []string{`{"cmd":"mkdir out"}`}, got) + meta := em.confirmationMeta[0] + assert.Equal(t, "deploy", meta["category"], "guard metadata outranks the preempt lane") + assert.Equal(t, "guard", meta["owner"]) + assert.Equal(t, "high", meta["blast_radius"], "preempt metadata still reaches the prompt") + assert.Equal(t, 1, rec.count(hooks.EventToolGuard)) + assert.Equal(t, 1, hd.count(hooks.EventPreToolUsePreYolo)) + assert.Zero(t, rec.count(hooks.EventPermissionRequest)) + assert.Zero(t, hd.count(hooks.EventPreToolUse), "default lane is skipped once a mandatory lane asks") + assert.Equal(t, approvalRecord{Decision: toolexec.ApprovalDecisionAllow, Source: toolexec.ApprovalSourceUserApproved}, lastApproval(t, hd)) +} diff --git a/pkg/runtime/toolexec/tool_cancel_test.go b/pkg/runtime/toolexec/tool_cancel_test.go new file mode 100644 index 0000000000..28bf3f9a5b --- /dev/null +++ b/pkg/runtime/toolexec/tool_cancel_test.go @@ -0,0 +1,187 @@ +package toolexec_test + +import ( + "context" + "sync" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/docker/docker-agent/pkg/hooks" + "github.com/docker/docker-agent/pkg/runtime/toolexec" + "github.com/docker/docker-agent/pkg/session" + "github.com/docker/docker-agent/pkg/tools" +) + +// bWait blocks until its context dies and reports the context error, so the +// dispatcher sees exactly what the executor's fail-closed path produces. +const bWait = "wait" + +// newWaitHooks builds a real executor whose hooks block. started is closed +// once the first hook is running so tests can cancel mid-dispatch. +func newWaitHooks(t *testing.T, cfg *hooks.Config) (*execHookDispatcher, <-chan struct{}) { + t.Helper() + started := make(chan struct{}) + var once sync.Once + reg := hooks.NewRegistry() + require.NoError(t, reg.RegisterBuiltin(bWait, func(ctx context.Context, _ *hooks.Input, _ []string) (*hooks.Output, error) { + once.Do(func() { close(started) }) + <-ctx.Done() + return nil, ctx.Err() + })) + return &execHookDispatcher{exec: hooks.NewExecutorWithRegistry(cfg, t.TempDir(), nil, reg)}, started +} + +func cancelWhenStarted(t *testing.T, started <-chan struct{}) context.Context { + t.Helper() + ctx, cancel := context.WithCancel(t.Context()) + t.Cleanup(cancel) + go func() { + select { + case <-started: + cancel() + case <-ctx.Done(): + } + }() + return ctx +} + +func requireCanceled(t *testing.T, em *captureEmitter, hd *execHookDispatcher) { + t.Helper() + assert.Empty(t, em.confirmations) + assert.Empty(t, em.hookBlocks, "a cancellation is not a hook denial") + require.Len(t, em.responses, 1) + assert.True(t, em.responses[0].IsError) + assert.Contains(t, em.responses[0].Output, "canceled by the user") + assert.Equal(t, approvalRecord{Decision: toolexec.ApprovalDecisionCanceled, Source: toolexec.ApprovalSourceContextCanceled}, lastApproval(t, hd)) +} + +// The executor fails closed when its context dies, so a user cancel that +// lands while a pre-execution hook runs must still be reported as a +// cancellation, never as a hook denial — and the tool must not run. +func TestToolPhases_ParentCancelDuringHookIsCanceled(t *testing.T) { + t.Parallel() + + blocking := hook(bWait) + blocking.OnError = "block" // events that do not fail closed on their own + + for _, tc := range []struct { + name string + cfg *hooks.Config + policy session.SafetyPolicy + }{ + {name: "tool_input_transform", cfg: &hooks.Config{ToolInputTransform: matchAll(blocking)}, policy: session.SafetyPolicyAutonomous}, + {name: "tool_guard", cfg: &hooks.Config{ToolGuard: matchAll(hook(bWait))}, policy: session.SafetyPolicyAutonomous}, + {name: "pre_tool_use preempt lane", cfg: &hooks.Config{PreToolUse: preemptYolo(hook(bWait))}, policy: session.SafetyPolicyAutonomous}, + {name: "pre_tool_use default lane", cfg: &hooks.Config{PreToolUse: matchAll(hook(bWait))}, policy: session.SafetyPolicyStrict}, + {name: "permission_request", cfg: &hooks.Config{PermissionRequest: matchAll(blocking)}, policy: session.SafetyPolicyStrict}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + hd, started := newWaitHooks(t, tc.cfg) + sess := session.New(session.WithSafetyPolicy(tc.policy)) + var got []string + em := &captureEmitter{} + + stop, msg := newDispatcher(hd, nil).Process(cancelWhenStarted(t, started), sess, []tools.ToolCall{shellCall("x", "ls")}, []tools.Tool{shellTool(&got)}, em) + + assert.False(t, stop) + assert.Empty(t, msg) + assert.Empty(t, got, "tool must not run after a cancel") + requireCanceled(t, em, hd) + }) + } +} + +// A cancel during one call's guard cancels its batch siblings too, exactly +// like a cancel during a confirmation prompt. +func TestToolPhases_ParentCancelDuringGuardCancelsBatch(t *testing.T) { + t.Parallel() + hd, started := newWaitHooks(t, &hooks.Config{ToolGuard: matchAll(hook(bWait))}) + sess := session.New(session.WithSafetyPolicy(session.SafetyPolicyAutonomous)) + var got []string + em := &captureEmitter{} + + newDispatcher(hd, nil).Process(cancelWhenStarted(t, started), sess, []tools.ToolCall{ + shellCall("a", "ls"), + shellCall("b", "pwd"), + }, []tools.Tool{shellTool(&got)}, em) + + assert.Empty(t, got) + assert.Empty(t, em.hookBlocks) + require.Len(t, em.responses, 2) + for _, r := range em.responses { + assert.True(t, r.IsError) + assert.Contains(t, r.Output, "canceled") + } + hd.mu.Lock() + defer hd.mu.Unlock() + require.Len(t, hd.approvals, 2) + for _, ap := range hd.approvals { + assert.Equal(t, approvalRecord{Decision: toolexec.ApprovalDecisionCanceled, Source: toolexec.ApprovalSourceContextCanceled}, ap) + } +} + +func TestGate_ParentCancelDuringGuardIsCanceled(t *testing.T) { + t.Parallel() + hd, started := newWaitHooks(t, &hooks.Config{ToolGuard: matchAll(hook(bWait))}) + sess := session.New(session.WithSafetyPolicy(session.SafetyPolicyAutonomous)) + em := &captureEmitter{} + g := newGate(sess, em, nil) + g.Hooks = hd + + ran := false + _, err := g.ConfirmAndRun(cancelWhenStarted(t, started), echoCommand, func(context.Context, tools.ConfirmedRun) (string, error) { + ran = true + return "", nil + }) + + require.ErrorIs(t, err, context.Canceled) + require.NotErrorIs(t, err, tools.ErrConfirmationDenied) + assert.False(t, ran) + assert.Empty(t, em.hookBlocks) + assert.Empty(t, em.responses, "an unprompted cancellation has no synthetic call lifecycle") + assert.Equal(t, approvalRecord{Decision: toolexec.ApprovalDecisionCanceled, Source: toolexec.ApprovalSourceContextCanceled}, lastApproval(t, hd)) +} + +// A hook's own timeout only cancels the hook's context: with the parent +// alive it is a real failure and the mandatory lanes keep failing closed. +func TestToolPhases_HookTimeoutStaysDenied(t *testing.T) { + t.Parallel() + + slow := hook(bWait) + slow.Timeout = 1 + + for _, tc := range []struct { + name string + cfg *hooks.Config + source string + want []string + }{ + { + name: "tool_guard", + cfg: &hooks.Config{ToolGuard: matchAll(slow)}, + source: toolexec.ApprovalSourceToolGuardDeny, + want: []string{"tool_guard hook failed to execute", "timed out after 1s"}, + }, + { + name: "pre_tool_use preempt lane", + cfg: &hooks.Config{PreToolUse: preemptYolo(slow)}, + source: toolexec.ApprovalSourcePreToolUseHookDeny, + want: []string{"pre_tool_use hook failed to execute", "timed out after 1s"}, + }, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + hd, _ := newWaitHooks(t, tc.cfg) + sess := session.New(session.WithSafetyPolicy(session.SafetyPolicyAutonomous)) + em := &captureEmitter{} + + newDispatcher(hd, nil).Process(t.Context(), sess, []tools.ToolCall{shellCall("x", "ls")}, []tools.Tool{neverRun()}, em) + + requireDenied(t, em, hd, tc.source, tc.want...) + require.Len(t, em.hookBlocks, 1) + }) + } +} diff --git a/pkg/runtime/toolexec/tool_hooks.go b/pkg/runtime/toolexec/tool_hooks.go index b862725426..93d4c4e0e6 100644 --- a/pkg/runtime/toolexec/tool_hooks.go +++ b/pkg/runtime/toolexec/tool_hooks.go @@ -4,30 +4,35 @@ import ( "cmp" "context" "fmt" + "log/slog" + "strings" "github.com/docker/docker-agent/pkg/hooks" ) // transformToolInput runs once, before any guard, classifier, or permission rule. -func (c *call) transformToolInput(ctx context.Context) bool { +func (c *call) transformToolInput(ctx context.Context) (CallOutcome, bool) { if c.d.Hooks == nil { - return false + return CallOutcome{}, false } in := NewHooksInput(c.sess, c.tc) in.ToolCategory = c.tool.Category result := c.d.Hooks.Dispatch(ctx, c.a, hooks.EventToolInputTransform, in) + if outcome, canceled := c.hookCanceled(ctx); canceled { + return outcome, true + } if result == nil { - return false + return CallOutcome{}, false } if !result.Allowed { c.blockToolHook(ctx, hooks.EventToolInputTransform, ApprovalSourceToolInputTransformDeny, result.Message) - return true + return CallOutcome{}, true } if _, err := c.applyHookModifiedInput(result); err != nil { c.blockToolHook(ctx, hooks.EventToolInputTransform, ApprovalSourceToolInputTransformDeny, fmt.Sprintf("invalid rewritten input: %v", err)) - return true + return CallOutcome{}, true } - return false + return CallOutcome{}, false } func (c *call) consultToolGuard(ctx context.Context) *hooks.Result { @@ -47,23 +52,29 @@ func (c *call) toolGuardAsks() bool { } // consultMandatoryHooks evaluates both guard lanes without approving or prompting. -func (c *call) consultMandatoryHooks(ctx context.Context) bool { +func (c *call) consultMandatoryHooks(ctx context.Context) (CallOutcome, bool) { guard := c.consultToolGuard(ctx) + if outcome, canceled := c.hookCanceled(ctx); canceled { + return outcome, true + } if guard != nil && (!guard.Allowed || guard.Decision == hooks.DecisionDeny) { c.blockToolHook(ctx, hooks.EventToolGuard, ApprovalSourceToolGuardDeny, cmp.Or(guard.Message, guard.DecisionReason)) - return true + return CallOutcome{}, true } preempt := c.consultPreToolUsePreYolo(ctx) + if outcome, canceled := c.hookCanceled(ctx); canceled { + return outcome, true + } if preempt != nil && (!preempt.Allowed || preempt.Decision == hooks.DecisionDeny) { c.blockToolHook(ctx, hooks.EventPreToolUse, ApprovalSourcePreToolUseHookDeny, cmp.Or(preempt.Message, preempt.DecisionReason)) - return true + return CallOutcome{}, true } - return false + return CallOutcome{}, false } func (c *call) runToolGuards(ctx context.Context, runTool func() CallOutcome) (CallOutcome, bool) { - if c.consultMandatoryHooks(ctx) { - return CallOutcome{}, true + if outcome, handled := c.consultMandatoryHooks(ctx); handled { + return outcome, true } preempt := c.preYoloResult @@ -91,8 +102,8 @@ func (c *call) runToolGuards(ctx context.Context, runTool func() CallOutcome) (C // Revalidation may deny or require fresh approval, but never auto-approve. func (c *call) recheckRewrittenInput(ctx context.Context, runTool func() CallOutcome) (CallOutcome, bool) { c.guardComputed, c.preYoloComputed = false, false - if c.consultMandatoryHooks(ctx) { - return CallOutcome{}, true + if outcome, handled := c.consultMandatoryHooks(ctx); handled { + return outcome, true } decision := c.permissionDecision() if decision.Outcome == OutcomeDeny { @@ -113,11 +124,32 @@ func (c *call) mandatoryAsk() bool { return c.toolGuardAsks() || c.rewrittenInputAsk } +// hookCanceled turns a parent cancellation observed after a hook dispatch +// into the canceled outcome. Hook executors fail closed when their context +// dies, so without this check a user cancel would be recorded as a hook +// denial. A hook's own timeout only cancels the child context and still +// reads as a failure. +func (c *call) hookCanceled(ctx context.Context) (CallOutcome, bool) { + if ctx.Err() == nil { + return CallOutcome{}, false + } + slog.DebugContext(ctx, "Context cancelled during hook dispatch", "tool", c.tc.Function.Name, "session_id", c.sess.ID) + return c.canceled(ctx), true +} + +// canceled records the cancellation verdict and returns the matching outcome. +func (c *call) canceled(ctx context.Context) CallOutcome { + c.notifyApproval(ctx, ApprovalDecisionCanceled, ApprovalSourceContextCanceled) + c.errorResponse(ctx, c.cancellationMessage(ctx)) + return c.cancellationOutcome(ctx) +} + func (c *call) blockToolHook(ctx context.Context, event hooks.EventType, source, reason string) { message := fmt.Sprintf("The tool call was rejected by a %s hook.", event) - if reason != "" { + if reason = strings.TrimSpace(reason); reason != "" { message += " Reason: " + reason } + slog.DebugContext(ctx, "Tool blocked by hook", "tool", c.tc.Function.Name, "event", event, "reason", reason, "session_id", c.sess.ID) c.notifyApproval(ctx, ApprovalDecisionDeny, source) c.em.EmitHookBlocked(c.tc, c.tool, message, c.a.Name()) c.errorResponse(ctx, message) diff --git a/pkg/runtime/toolexec/tool_phases_test.go b/pkg/runtime/toolexec/tool_phases_test.go index 5ff78e075e..443dd68af3 100644 --- a/pkg/runtime/toolexec/tool_phases_test.go +++ b/pkg/runtime/toolexec/tool_phases_test.go @@ -612,7 +612,7 @@ func TestToolPhases_PreemptYoloFailureBlocks(t *testing.T) { h hooks.Hook want string }{ - {name: "crash", h: hook(bCrash), want: "PreToolUse hook failed to execute"}, + {name: "crash", h: hook(bCrash), want: "pre_tool_use hook failed to execute"}, {name: "generic block", h: hook(bBlock, "stop right there"), want: "stop right there"}, } { t.Run(tc.name, func(t *testing.T) { From a5dc872361ffd60ea2fab1cf1e550f1d5d0af0f7 Mon Sep 17 00:00:00 2001 From: David Gageot Date: Wed, 9 Sep 2026 15:23:39 +0200 Subject: [PATCH 4/4] docs(hooks): update schema, docs and examples for hook phases Update agent-schema.json with new hook fields (strict_output, guard phase). Revise hooks documentation with phase ordering, compatibility notes (no-op scripts must exit 0, on_error:block rejected on non-blocking events, customized built-ins are distinct from automatic defaults). Add tool_hook_phases.yaml example demonstrating the full phase pipeline. --- agent-schema.json | 9 +- docs/configuration/agents/index.md | 3 + docs/configuration/hooks/index.md | 127 ++++++++++++++++++++++++++--- examples/hooks.yaml | 4 +- examples/redact_secrets_hooks.yaml | 4 +- examples/tool_hook_phases.yaml | 6 +- 6 files changed, 133 insertions(+), 20 deletions(-) diff --git a/agent-schema.json b/agent-schema.json index d4b02e5bf6..1c4b89013d 100644 --- a/agent-schema.json +++ b/agent-schema.json @@ -1416,7 +1416,7 @@ }, "preempt_yolo": { "type": "boolean", - "description": "Opt a pre_tool_use entry into firing BEFORE the deterministic approval pipeline (custom allow/ask/deny rules + safety mode). A deny/ask verdict from a preempting hook cannot be bypassed by any safety mode (including autonomous) or permission allow-rules; an allow verdict is advisory (the pipeline still runs Decide() and the rest of pre_tool_use). Default pre_tool_use entries fire AFTER Decide(). Only meaningful on pre_tool_use; rejected on tool_input_transform and tool_guard (which always preempt approval) and ignored on other events. Set it on hooks that implement a security-critical check that must not be bypassed by auto-approval." + "description": "Opt a pre_tool_use entry into firing BEFORE the deterministic approval pipeline (custom allow/ask/deny rules + safety mode). A deny/ask verdict from a preempting hook cannot be bypassed by any safety mode (including autonomous) or permission allow-rules; an allow verdict is advisory (the pipeline still runs Decide() and the rest of pre_tool_use). Default pre_tool_use entries fire AFTER Decide(). Only valid on pre_tool_use; rejected on every other event. Set it on hooks that implement a security-critical check that must not be bypassed by auto-approval." } }, "required": [ @@ -1471,7 +1471,7 @@ }, "on_error": { "type": "string", - "description": "How non-fail-closed hook failures are handled. 'warn' logs and continues (default), 'ignore' continues silently, and 'block' denies the event.", + "description": "Policy for execution errors, timeouts, unexpected nonzero exits and invalid output. 'warn' reports and continues (default), 'ignore' continues silently, and 'block' denies block-capable events. pre_tool_use and tool_guard always fail closed. on_error=block is rejected on observational events.", "enum": [ "warn", "ignore", @@ -1479,6 +1479,11 @@ ], "default": "warn" }, + "strict_output": { + "type": "boolean", + "description": "Require a single JSON output object (or empty stdout), reject unknown fields and outputs unsupported by the hook event. Failures follow on_error; pre_tool_use and tool_guard fail closed.", + "default": false + }, "model": { "type": "string", "description": "Model spec ('provider/model', e.g. 'openai/gpt-4o-mini') invoked by type=model hooks. Required for that type, ignored otherwise." diff --git a/docs/configuration/agents/index.md b/docs/configuration/agents/index.md index 560b5e33fd..c3033d2397 100644 --- a/docs/configuration/agents/index.md +++ b/docs/configuration/agents/index.md @@ -53,6 +53,9 @@ agents: handoffs: [list] # Optional: agent names this agent can hand off to force_handoff: string # Optional: agent that always receives the conversation when this agent stops hooks: # Optional: lifecycle hooks + tool_input_transform: [list] + tool_guard: [list] + permission_request: [list] pre_tool_use: [list] tool_response_transform: [list] post_tool_use: [list] diff --git a/docs/configuration/hooks/index.md b/docs/configuration/hooks/index.md index 5315f8733a..f8e3b50d0d 100644 --- a/docs/configuration/hooks/index.md +++ b/docs/configuration/hooks/index.md @@ -67,6 +67,49 @@ Docker Agent dispatches the following hook events: > > `pre_compact` and `before_compaction` both fire just before a compaction. `pre_compact` is the original event and is best-suited to _steering_ the LLM-generated summary by appending guidance via `additional_context`. `before_compaction` is the newer, structured event: it carries the input/output token counts, the model's context limit, and a `compaction_reason` so handlers can decide based on real session pressure, and it can _replace_ the LLM-generated summary verbatim via `hook_specific_output.summary`. +## Event contracts + +These contracts describe the native runtime; the experimental WASM runtime does +not yet implement the mandatory tool phases. + +The shared catalog in `pkg/hooks/events` drives configuration validation, +dispatch strategy, output aggregation, and strict output validation. Tests keep +this table, the configuration fields, and the JSON schema synchronized. +“Context” means additional context is consumed by the runtime; `worktree_create` +shows it to the CLI user. The internal preempting `pre_tool_use` lane is parallel, +collects metadata, and does not rewrite input. + +| Event | Execution | Can block | Failure default | Context | Rewrite | +| --- | --- | --- | --- | --- | --- | +| `pre_tool_use` | sequential | yes | block | no | tool input | +| `post_tool_use` | parallel | yes | warn | no | — | +| `permission_request` | parallel | yes | warn | no | — | +| `session_start` | parallel | no | warn | yes | — | +| `user_prompt_submit` | parallel | yes | warn | yes | — | +| `user_steering_messages_submit` | parallel | yes | warn | yes | — | +| `user_followup_submit` | parallel | yes | warn | yes | — | +| `turn_start` | parallel | no | warn | yes | — | +| `turn_end` | parallel | no | warn | no | — | +| `before_llm_call` | sequential | yes | warn | no | messages | +| `after_llm_call` | parallel | no | warn | no | — | +| `session_end` | parallel | no | warn | no | — | +| `pre_compact` | parallel | yes | warn | yes | — | +| `subagent_stop` | parallel | no | warn | no | — | +| `on_user_input` | parallel | no | warn | no | — | +| `stop` | parallel | no | warn | no | — | +| `notification` | parallel | no | warn | no | — | +| `on_error` | parallel | no | warn | no | — | +| `on_max_iterations` | parallel | no | warn | no | — | +| `on_agent_switch` | parallel | no | warn | no | — | +| `on_session_resume` | parallel | no | warn | no | — | +| `on_tool_approval_decision` | parallel | no | warn | no | — | +| `before_compaction` | parallel | yes | warn | no | — | +| `after_compaction` | parallel | no | warn | no | — | +| `tool_response_transform` | sequential | no | warn | no | tool response | +| `tool_input_transform` | sequential | yes | warn | no | tool input | +| `tool_guard` | parallel | yes | block | no | — | +| `worktree_create` | parallel | yes | warn | yes | — | + ## Configuration You can configure hooks directly in an agent YAML file under the agent's `hooks:` block: @@ -128,6 +171,27 @@ stop: command: "./scripts/log-response.sh" ``` +### Hook identity and deduplication + +For each event dispatch, identical matching hook definitions run once, at the +position of the first match. Identity includes every hook field: `name`, `type`, +`command`, `args`, `timeout`, `env`, `working_dir`, `on_error`, `strict_output`, `model`, `prompt`, +and `schema`. Sharing a name or command alone does not make two hooks duplicates. + +Hooks with different model prompts, environments, working directories, or other +options all run. To deliberately run otherwise identical hooks twice, give them +different names. Repeated dispatches still run the hooks again. + +Comparison uses configured values, without expanding environment variables or +paths. Environment map ordering does not matter; argument ordering does. Empty +and omitted `args` or `env` are equivalent. Explicit options such as `timeout: 60` +and `on_error: warn` remain distinct from omitted options. + +This also applies to automatic built-ins: an identical explicit entry runs only +once, but adding a name or changing an option makes it a separate invocation. +Disable the corresponding agent flag when you want a custom entry *instead of* +the automatic default. + ## Global (user-level) hooks Global hooks let you apply the same hook configuration to every agent you run. Define them in your user config file at `~/.config/cagent/config.yaml` under `settings.hooks`: @@ -353,7 +417,7 @@ In addition to the common fields, each event ships its own payload: | `tool_response_transform` | `tool_name`, `tool_use_id`, `tool_input`, `tool_response` | | `post_tool_use` | `agent_name`, `tool_name`, `tool_use_id`, `tool_input`, `tool_response`, `tool_error` | | `permission_request` | `agent_name`, `tool_name`, `tool_use_id`, `tool_input` | -| `session_start` | `source` — one of `startup`, `resume`, `clear`, `compact` | +| `session_start` | `source` — `startup` for each run stream | | `user_prompt_submit` | `prompt` — the text the user just submitted | | `user_steering_messages_submit` | `steering_messages` — the drained steering messages, in submission order | | `user_followup_submit` | `prompt` — the text of the dequeued follow-up message | @@ -361,7 +425,7 @@ In addition to the common fields, each event ships its own payload: | `turn_end` | `agent_name`, `reason` — one of `normal`, `continue`, `steered`, `error`, `canceled`, `hook_blocked`, `loop_detected` | | `before_llm_call` | `iteration` — 1-based run-loop iteration counter (the model call this hook is gating), `model_id` | | `after_llm_call` | `agent_name`, `stop_response`, `last_user_message`, `model_id`, `usage`, `cost` | -| `session_end` | `reason` — one of `clear`, `logout`, `prompt_input_exit`, `other` | +| `session_end` | `reason` — `stream_ended` | | `pre_compact` | `source` — one of `manual`, `auto`, `overflow`, `tool_overflow` | | `before_compaction` | `input_tokens`, `output_tokens`, `context_limit`, `compaction_reason` (one of `threshold`/`overflow`/`manual`) | | `after_compaction` | `input_tokens`, `output_tokens`, `context_limit`, `compaction_reason`, `summary` | @@ -420,7 +484,7 @@ All fields are optional. Returning `{}` (or no output at all) means "do nothing, | ----------------- | ------- | ----------------------------------------------- | | `continue` | boolean | Whether to continue execution (default: `true`) | | `stop_reason` | string | Message to show when `continue=false` | -| `suppress_output` | boolean | Hide stdout from transcript | +| `suppress_output` | boolean | Legacy compatibility field; has no effect and is rejected when true in strict mode | | `system_message` | string | Warning message to display to user | | `decision` | string | For blocking: `block` to prevent operation | | `reason` | string | Explanation for the decision | @@ -471,7 +535,8 @@ shell actions such as commands embedded in skills. They run once per call; if a legacy `pre_tool_use` hook changes arguments afterwards, guards and rules are checked again against the rewritten call. Transforms and approval helpers are not rerun: legacy rewrites are not automatically re-redacted. A new ask -during revalidation requires fresh approval, not an earlier session grant. +during revalidation requires fresh approval, not an earlier session grant; it +also skips `permission_request` approval helpers. A no-op patch does not trigger another guard invocation. Prefer `tool_input_transform` for new rewriters so guards only need one pass. @@ -501,9 +566,9 @@ hooks: **Failures:** transform execution errors follow `on_error` (default `warn`); `on_error: block`, `decision: block`, `continue: false`, and exit `2` prevent execution. Guard execution errors and timeouts block regardless of `on_error`. -Both retain the existing shell exit-code protocol: nonzero codes other than `2` -are non-blocking, and malformed stdout JSON is not a verdict. A command guard -must explicitly emit a blocking result or exit `2` when it cannot check safely. +Unexpected nonzero exits (including `1` and `127`), malformed JSON, and invalid +verdicts are failures too: guards deny; transforms follow `on_error`. +Successful no-op hooks must exit `0`. Neither event accepts `preempt_yolo`, since both already precede approval. The default secret-redaction argument hook now uses `tool_input_transform`, so @@ -634,7 +699,7 @@ not receive raw secrets. See [the example configuration](https://github.com/dock ### Context-Contributing Events -For `session_start`, `user_prompt_submit`, `user_steering_messages_submit`, `user_followup_submit`, `turn_start`, `post_tool_use`, `pre_compact`, and `stop`, hooks may set `hook_specific_output.additional_context` to inject text into the conversation. `turn_start` context is **transient** (recomputed every turn, never persisted); `session_start` context **persists** for the life of the session. `user_steering_messages_submit` and `user_followup_submit` context is **transient** like `user_prompt_submit` — it is spliced into the steered/follow-up turn only and never persisted. (`worktree_create` also surfaces stdout, but to the CLI user rather than the conversation — the session doesn't exist yet.) +For `session_start`, `user_prompt_submit`, `user_steering_messages_submit`, `user_followup_submit`, `turn_start`, and `pre_compact`, hooks may set `hook_specific_output.additional_context` to inject text into the conversation. `turn_start` context is **transient** (recomputed every turn, never persisted); `session_start` context **persists** for the life of the session. `user_steering_messages_submit` and `user_followup_submit` context is **transient** like `user_prompt_submit` — it is spliced into the steered/follow-up turn only and never persisted. (`worktree_create` also surfaces stdout, but to the CLI user rather than the conversation — the session doesn't exist yet.) ### Before-Compaction Specific Output @@ -653,7 +718,7 @@ Returning `decision: "block"` (or exit code 2) instead vetoes the compaction ent ### Plain Text Output -For `session_start`, `user_prompt_submit`, `user_steering_messages_submit`, `user_followup_submit`, `turn_start`, `post_tool_use`, `pre_compact`, and `stop` hooks, plain text written to stdout (i.e., output that is not valid JSON) is captured as additional context for the agent. For `pre_compact` it is appended to the compaction prompt; for the others it is spliced into the conversation as a (transient or persisted) system message depending on the event. +For `session_start`, `user_prompt_submit`, `user_steering_messages_submit`, `user_followup_submit`, `turn_start`, and `pre_compact` hooks, plain text written to stdout (i.e., output that does not start with `{`) is captured as additional context for the agent. For `pre_compact` it is appended to the compaction prompt; for the others it is spliced into the conversation as a (transient or persisted) system message depending on the event. ## Exit Codes @@ -663,7 +728,7 @@ Hook exit codes have special meaning: | --------- | -------------------------------------- | | `0` | Success — continue normally | | `2` | Blocking error — stop the operation | -| Other | Error — logged but execution continues | +| Other | Failure — follows `on_error`; security guards fail closed | ## Per-hook options @@ -684,14 +749,52 @@ hooks: on_error: warn # warn | ignore | block ``` -`pre_tool_use` is fail-closed for safety: a failed pre-tool hook blocks the tool call regardless of `on_error`. +`pre_tool_use` (both lanes) and `tool_guard` fail closed on **all failures**, +including exit codes such as `1` or `127`, regardless of `on_error`. Other events +apply `on_error` consistently to execution errors, timeouts, unexpected nonzero +exits, malformed JSON, and invalid verdicts. `warn` reports the hook name and +event (also as a UI warning where the runtime has an event sink); `ignore` stays +silent. `block` is accepted only on events capable of stopping an operation. +Exit `2` is an explicit block on those events, not a recoverable error. + +**Compatibility:** scripts that previously exited nonzero to signal “no opinion” +must now exit `0`. Use empty stdout or `{}` for a successful no-op. Parent +cancellation is reported as cancellation, not a policy denial; a hook's own +timeout remains a failure. + +Set `strict_output: true` for hooks that implement the structured protocol: + +```yaml +hooks: + tool_guard: + - matcher: shell + hooks: + - name: project policy + type: command + command: ./check-command.sh + strict_output: true + timeout: 5 +``` + +Strict hooks accept one JSON object or empty stdout. They reject plain text, +unknown fields, event-name mismatches, invalid decisions, and output fields the +event cannot consume (for example, `updated_input` on `tool_guard`). Direct Go +outputs and model outputs receive the same capability checks. Without strict +mode, plain text remains supported for context events and unknown fields remain +compatible; malformed JSON beginning with `{` and invalid decisions are still +failures. JSON followed by log text is also invalid; send diagnostics to stderr. +Configuration loading validates matchers and error policies at +load time rather than silently dropping invalid rules. + +`stop` and `post_tool_use` do not consume additional context; use a context event +such as `turn_start` instead. Strict mode makes this mistake an error. `working_dir` and `env` apply to `command` and `builtin` hooks. For `builtin` hooks, `working_dir` is resolved with the same logic as `command` hooks (absolute path wins; relative paths join onto the executor directory). `working_dir` accepts `~`, `$VAR`, `${VAR}` and `${env.VAR}`; `env` values expand only the plain `${env.VAR}` form (resolved from the OS process environment), keeping any other `$` literal (see [Variable Expansion in Config Fields](../overview/index.md#variable-expansion-in-config-fields)). A `working_dir` that expands to an empty string (e.g. an unset variable) falls back to the executor's directory with a warning. For `model` hooks, both fields are accepted by the schema but have no effect: model hooks render a prompt template and call the LLM API directly — no subprocess is spawned and no file I/O is performed, so working directory and environment variables have no applicable semantics. > [!WARNING] > **Performance** > -> Hooks run synchronously and can slow down agent execution. Keep hook scripts fast and efficient. Consider using `suppress_output: true` for logging hooks to reduce noise. +> Hooks run synchronously and can slow down agent execution. Keep hook scripts fast and efficient. Write diagnostics to stderr and protocol output to stdout. > [!NOTE] > **Session End and Cancellation** diff --git a/examples/hooks.yaml b/examples/hooks.yaml index 2ad5ac8aa6..7698a57172 100644 --- a/examples/hooks.yaml +++ b/examples/hooks.yaml @@ -60,8 +60,8 @@ # Requirements: the command-style hooks below pipe stdin through `jq` for # convenient JSON access. If you don't have jq installed (`brew install jq` # on macOS, `apt-get install jq` on Debian/Ubuntu), a hook will exit non-zero -# and the runtime will log a 'Hook execution error' warning but otherwise -# continue — use plain `awk`/`sed`/`grep` or any other parser if you prefer. +# and fail-closed pre_tool_use hooks will deny the call; other events +# follow on_error — use plain `awk`/`sed`/`grep` or any other parser if you prefer. # # Try these prompts: # "Run: echo hello" → allowed diff --git a/examples/redact_secrets_hooks.yaml b/examples/redact_secrets_hooks.yaml index 29548db7fb..da90d6c65e 100644 --- a/examples/redact_secrets_hooks.yaml +++ b/examples/redact_secrets_hooks.yaml @@ -2,8 +2,8 @@ # directly, instead of through the agent-level `redact_secrets` flag # (which is enabled by default and auto-injects the same entries). # -# Auto-injection is idempotent against manually-written entries that -# name the same builtin, so spelling them out by hand is safe and is +# Auto-injection deduplicates identical entries (including names and +# per-hook options), so spelling these defaults out by hand is safe and is # useful when you want to: # # * scope the rewrite to a subset of tools (set `matcher:` to a diff --git a/examples/tool_hook_phases.yaml b/examples/tool_hook_phases.yaml index 53dddb6be4..a1d07f9f16 100644 --- a/examples/tool_hook_phases.yaml +++ b/examples/tool_hook_phases.yaml @@ -12,6 +12,7 @@ agents: hooks: - name: normalize whitespace type: command + strict_output: true on_error: block command: | python3 -c ' @@ -21,12 +22,13 @@ agents: print(json.dumps({"hook_specific_output": { "updated_input": {"cmd": command.strip()} }})) - ' || exit 2 + ' tool_guard: - matcher: shell hooks: - name: require confirmation for project commands type: command + strict_output: true timeout: 5 command: | python3 -c ' @@ -40,7 +42,7 @@ agents: "permission_decision_reason": "Review commands beyond the read-only allowlist", "metadata": {"policy": "project command review"} }})) - ' || exit 2 + ' # This judge only runs when the guard falls through and safety mode asks. # A guard's "ask" skips it; an "allow" here cannot undo a guard denial. pre_tool_use: