#16579

hooks: deny a real shell command through a real Cursor hook

Draftvorporealwants to merge intofrom

Description

Summary

Second PR of the agent hooks Phase 1 stack. It proves the PR 1 scaffold end to end with real parts instead of test doubles: a minimal Cursor hooks.json parser that understands only beforeShellExecution, a minimal runner that spawns a real process with the JSON payload on stdin, and one explicit way to point the registry at a hooks file. A real script denies a real shell command in a test, through try_to_execute_action, with no test doubles anywhere in the path. Whatever the slice taught about PR 1's shared types was applied back to PR 1 (two small amendments, listed below).

  • crates/ai/src/hooks/adapters/cursor: CursorAdapter. config.rs reads version and the hooks object, keeps the beforeShellExecution definitions (command, timeout, matcher, failClosed), reports other event names as warnings, and validates matchers at parse time. decode.rs turns a process outcome into effects by Cursor's rules: exit 2 denies, exit 0 means "read the JSON", everything else follows the definition's failure policy, and output that is not a valid decision denies. matches tests the shell command text with an unanchored, case-sensitive regex (cached per pattern); encode_input produces Cursor's documented stdin shape. The adapter is registered in the format table.
  • crates/ai/src/hooks/runner/command.rs: CommandHookRunner. Runs the configured command string through /bin/sh -c (Unix, in its own process group) or cmd.exe /C (Windows), with the invocation's working directory, the inherited process environment plus the invocation's variables, the captured login shell PATH when one is known, and WARP_HOOK_PROTOCOL_VERSION. Stdin, stdout, and stderr are piped and driven together with the exit wait so a hook that fills one pipe before touching another cannot deadlock. The hook's exit decides the outcome as soon as it arrives; its pipes get one more second to drain in case a background process it started still holds them. The invocation timeout, and the cancel flag (polled every 50 ms), kill the whole process group and return TimedOut or Cancelled. kill_on_drop(true) so a dropped future does not leave the direct child running.
  • crates/ai/src/hooks/registry.rs: HookRegistry::install_explicit_source(path, format, kind), in its own impl block. Reads the file, parses it with the adapter for the format, turns capability warnings into diagnostics, and publishes a new snapshot at revision + 1 in which this file's bindings replace any loaded from the same file before. Relative commands resolve against the file's directory.
  • app/src/ai/blocklist/hooks/mod.rs: HookRuntime::install_command_runner, called from initialize_app right after LocalShellState is registered. It installs CommandHookRunner and, when the hooks flag is on, starts the interactive login shell PATH capture; the result lands in a slot the runner reads on every invocation.
  • Tests: config, decode, and adapter unit tests; runner tests with real /bin/sh scripts; e2e_slice_tests.rs in ai (a hooks.json on disk, a script, the real loader, dispatcher, and runner); and one app test that drives a real deny through try_to_execute_action and ends in RequestCommandOutputResult::HookDenied with the script's agent_message.

Decisions

Cursor adapter

  • permission is required in a beforeShellExecution response (§6.4, research/01 §2.2). Cursor says a response that does not match the event's schema blocks a permission hook, and does not say whether permission may be omitted. A JSON object with no permission, or with a value other than allow, deny, or ask, is treated as malformed and denies even when the definition fails open. Empty stdout is the separate "no output" case and follows failClosed. Also recorded in questions.md.
  • Denial reason (§9.3): on a deny, agent_message is the reason the model reads; when there is no agent_message, user_message is; when there is neither, the reason is Denied by hook <command> (exit 2). for exit 2, Denied by hook <command>. for a JSON deny with no message, and a Warp-written sentence for the other deny paths. Every reason carries the binding it came from. A user_message that is not already the reason is delivered as a user notice. On allow and ask, user_message becomes a user notice and agent_message becomes model context, which validate_capabilities reports as unsupported in Phase 1 rather than dropping silently.
  • Exit 2 with JSON on stdout: the messages are still read. Exit 2 with anything else on stdout: the default reason. Other non-zero exits ignore stdout.
  • ProcessExit::Cancelled always decodes as a deny, regardless of failClosed (§6.4 says cancellation never becomes fail-open execution). The dispatcher's caller drops a cancelled verdict anyway; this only matters if one is ever applied. Also recorded in questions.md.
  • Timeouts, crashes, and non-zero exits other than 2 fail open unless failClosed: true, as Cursor documents. A slow beforeShellExecution hook therefore allows the command by default until strict hook policy (PR 6). Also recorded in questions.md.
  • Prompt hooks (type: "prompt") are skipped one definition at a time, each with a warning diagnostic naming it (beforeShellExecution[0]: prompt hooks are not supported yet; it was skipped); the command hooks beside them load and keep their positions in the array, so merge order is unchanged. This follows Cursor's own headless surfaces, which run command hooks only, and §4.4's view of the handler kind as something to report rather than a reason to refuse the file. The skip happens in the adapter's parser rather than as a registry capability check; PR 3 may move it when it parses the other events. Unknown type values are errors. Also recorded in questions.md.
  • timeout is read as a positive number of seconds (fractions allowed); zero, negative, non-finite, or non-numeric values are errors. The default is HookBoundary::ToolPre.default_timeout() (30 s). The ceiling rule (§6.4) is PR 5's.
  • version must be a positive integer; 0 or a missing version is an error. A missing hooks object is an empty file, not an error, since Cursor does not say it is required.
  • A matcher that does not compile makes the file invalid at parse time (§6.4), so matches never sees one. "" and "*" match everything and are not compiled. Compiled matchers are cached per pattern on the adapter so a regex is not built per envelope.
  • matches returns false for any tool.pre envelope that is not a shell command, and for every other boundary. Unknown events in the file are skipped with a warning diagnostic rather than an error (PR 3 adds them).
  • Stdin shape (§4.2): conversation_id, generation_id (the turn id, or the conversation id while turn_id is None), hook_event_name, cursor_version (Warp's version), workspace_roots (the detected roots, or the working directory when none was detected), user_email (null when unknown), transcript_path (null), command, and cwd. tool_use_id (the action id) is added because Cursor's release notes document it. model is present only when the envelope knows the model; sandbox and model_params are left out rather than invented.
  • Reason and notice text is trimmed and has control characters other than newline and tab removed, so a script cannot put terminal escape sequences into what the model or the user sees. Stdout that is not valid UTF-8 cannot parse as JSON and is malformed.

Runner

  • Shell (§9.1): /bin/sh -c <command> on Unix and cmd.exe /C <command> on Windows. The absolute /bin/sh path keeps the shell lookup independent of the PATH the hook itself receives. The command string is the configuration's literal text; payload bytes only ever travel over stdin.
  • Environment: the runner's own process environment, then the invocation's variables, then PATH from the capture when it returned one, then WARP_HOOK_PROTOCOL_VERSION last. The full environment policy (scrub list, WARP_* identity, CURSOR_* aliases) is PR 4.
  • The exit decides (§9.1 "parse after exit"): as soon as the hook's own exit status arrives, that is the outcome. Its pipes are read for up to one more second (PIPE_GRACE) in case a background process the hook started still holds them; after that, reading stops and whatever arrived so far is the output. Without this, some-logger & exit 2 was reported as a timeout once the deadline passed, and the deny fell through to the fail-open default.
  • A background process that outlives the hook is left alone. Once the hook has been collected its pid, and so its process group id, may belong to someone else, so the group is signalled only while try_status says the hook is still running. That also closes the window on the timeout path where a child that exited a moment before the deadline could have had its recycled pid signalled; a hook found exited at that point is reported by its exit status, not as a timeout.
  • Timeout and cancellation: one SIGKILL to the process group (Unix) plus Child::kill, then up to 2 s waiting for the child to be collected so a pid check afterwards sees it gone. The SIGTERM, wait, then SIGKILL sequence, output limits, and bounded concurrency are PR 4. On Windows only the direct cmd.exe child is killed; descendants can survive a timeout until PR 4 adds Job Objects.
  • A dropped dispatch future kills the direct child through kill_on_drop(true); it does not kill the process group, because async-process only signals the pid. PR 4's SIGTERM, wait, then SIGKILL sequence covers that case.
  • A finished process wins over a deadline that expired in the same poll (select_biased!), so a completed hook is never reported as timed out. The cancel flag is also checked before spawning; a flag that is already set spawns nothing.
  • Output kept on a timeout or cancellation: whatever was read before the kill, so the stderr tail still reaches the log line.
  • Stderr tail: the last 4 KiB as lossy UTF-8, matching PR 4's planned HOOK_STDERR_TAIL.
  • No wasm stand-in: the command module is compiled only with local_fs. PR 1's UnavailableRunner is the runner for builds that cannot spawn processes, and every app use of CommandHookRunner is behind the same feature, so a separate stub type was not needed.
  • The runner's shell program can be replaced in tests (with_shell_program, behind test-util), so a missing shell is testable as a spawn failure.

Registry loader

  • install_explicit_source identifies the source by its canonical path and kind, scopes its bindings Everywhere on the local host, uses the file's directory as the working directory for every binding, and consults no trust policy. Reloading the same file replaces that file's bindings and diagnostics and leaves other sources alone. It does not emit HookRegistryEvent::SnapshotChanged (nothing subscribes yet). Nothing in production calls it; PR 5 replaces or keeps it. Also recorded in questions.md.
  • Capability warnings from validate_capabilities become per-binding warning diagnostics in the snapshot. Every Cursor shell binding gets one today because the contract puts ModelContext in its capabilities and Phase 1 cannot deliver model context.

App wiring

  • PathCapture (amendment 1): LocalShellState::get_interactive_path_env_var needs &mut self and a ModelContext, and returns a future, so a plain fn() -> Option<OsString> cannot call it. PathCapture is now Arc<dyn Fn() -> Option<OsString> + Send + Sync>. HookRuntime::install_command_runner builds the runner with a closure over a shared slot and fills the slot when the capture resolves. Until then, and on surfaces where LocalShellState does not exist, hooks inherit the process PATH.
  • The interactive capture starts at startup, but only when FeatureFlag::AgentHooks is on and LocalShellState is registered, because it spawns an interactive login shell. With the flag off (every channel today) startup does no extra work.
  • The runner is installed from initialize_app right after LocalShellState, rather than where PR 1 registered HookRuntime, because LocalShellState does not exist yet at that point.
  • Dependencies: crates/ai/Cargo.toml gains command under the non-wasm dependencies (it depends only on warp_errors, log, and async-process), and async-process itself so the runner can name the Child type, as lsp already does. Cargo.lock records the new edges.

Departures from architecture.md

  • Amendment to PR 1 (dc9114902): PathCapture in crates/ai/src/hooks/runner/mod.rs became a shared closure instead of a function pointer, for the reason above.
  • Amendment to PR 1 (dcd155bf8): HookConfigError in crates/ai/src/hooks/adapters/mod.rs gained an Unreadable variant for a source file that cannot be read before parsing starts.
  • Follow-through for PR 3's amendment to PR 1 (aa6c0d0a4, formatting 8640b2e15): the minimal Cursor adapter's decode_output takes the new DecodePolicy parameter (unused here) and its bindings set loop_limit: None, so this branch builds on the amended trait. PR 3 replaces this adapter.
  • Follow-through for PR 4's amendment to PR 1 (1eb3e0126): HookProcessOutcome gained stdout_truncated (PR 1 commit 17c9e32b1), so this branch's minimal runner and its decode tests set the field to false where they build outcomes; the minimal runner never truncates.
  • §9.1 describes the runner with a termination grace period, output limits, bounded concurrency, a scrubbed environment, and Windows Job Objects. This PR has a single kill, no output limits, and no concurrency bound. The hook inherits Warp's full process environment with no scrub policy, plus PATH, the invocation's variables, and WARP_HOOK_PROTOCOL_VERSION; every variable in an Oz runner's environment therefore reaches hook scripts until PR 4 lands the scrub list. PR 4 owns all of these, and should also decide what to do about a background process a hook leaves holding its pipes (this PR stops reading after one second and leaves it running).
  • §7.3 and §9.1 say a project file's relative commands resolve against the project root and its bindings apply within that root. The stopgap loader resolves against the file's own directory and applies everywhere, for every source kind. PR 5 owns discovery and scoping.
  • §8 says activation goes through TrustPolicy. The stopgap loader does not consult it. Explicit sources are approved by construction and no production code calls the loader yet.
  • Files outside this PR's ownership that changed: app/src/lib.rs (one guarded call to HookRuntime::install_command_runner after LocalShellState is registered), app/src/ai/blocklist/hooks/mod.rs (the runner wiring the contract asked for; set_runner lost its unused-code allowance because it now has a production caller), app/src/ai/blocklist/action_model/execute_tests.rs (the one test the contract asked for), crates/ai/src/hooks/runner/mod.rs (one pub mod command; line), crates/ai/src/hooks/mod.rs (one test module line), and Cargo.lock (the new dependency edge).

Linked Issue

No linked issue. Part of the agent hooks Phase 1 stack; design is in specs/agent-hooks/architecture.md.

  • The linked issue is labeled ready-to-spec or ready-to-implement.
  • Where appropriate, screenshots or a short video of the implementation are included below (especially for user-visible or UI changes).

Testing

All commands run with CARGO_TARGET_DIR=/Users/david/src/warp/target. The warp commands also set WARP_LOCAL_SKIP_METAL=1, which this machine needs because it has no Metal toolchain.

  • cargo check -p ai — passed
  • cargo nextest run -p ai --features local_fs,test-util -E 'test(hooks)' — passed (95 tests)
  • cargo check -p warp — passed
  • cargo nextest run -p warp -E 'test(hooks)' --no-fail-fast — passed (51 tests)
  • cargo nextest run -p warp --no-fail-fast -E 'test(ai::blocklist::action_model) | test(ai::blocklist::hooks)' — passed (266 tests)
  • cargo clippy -p ai --features local_fs,test-util --all-targets --tests -- -D warnings — passed
  • cargo clippy -p warp --all-targets --tests -- -D warnings — passed
  • cargo check -p ai --target wasm32-unknown-unknown — not usable here: checking ai alone for wasm fails inside warpui_core (feature unification, unrelated to this PR) before reaching ai. CI's whole-workspace wasm clippy is the check for the local_fs gating.
  • ./script/format — ran last

Notes on the tests:

  • The process-backed tests (runner/command_tests.rs, e2e_slice_tests.rs, and the app test) are #[cfg(unix)]: they write #!/bin/sh scripts. Windows gets its .cmd tests with the full runner in PR 4.
  • The app test runs without an environment variable guard; the executor test harness spawns processes fine under App::test. If CI cannot, the fallback the contract allows is a WARP_HOOKS_E2E=1 guard.
  • Two pure parsing tests were marked LEAK by nextest in single runs under heavy parallel load and passed cleanly on every rerun; the same mark appeared on an unrelated pure test in PR 1's runs.

New tests, by module:

  • adapters/cursor/config: a minimal file; timeout, matcher, and failClosed; unknown events skipped and reported; no hooks object; invalid JSON; missing or zero version; missing or blank command; a prompt hook skipped while the command hook after it keeps its position; an unknown hook type; bad timeouts including values that overflow a Duration; a matcher that does not compile; "" and "*" accepted; the event value must be an array.

  • adapters/cursor/decode: exit 2 with no output, with messages, and with unreadable output; exit 0 with allow, deny (with and without a message), and ask; a deny with only a user_message; messages on an allow; empty output under both failure policies; six malformed shapes including a missing permission; stdout that is not UTF-8; five failure kinds under both policies; cancellation; control characters removed.

  • adapters/cursor (adapter): the table entry; bindings declared in file order with the right ids, capabilities, and timeout; a skipped prompt hook becomes a warning and the next binding keeps its index; matcher semantics (unanchored, case-sensitive, match-all forms); non-shell envelopes never match; the full stdin shape; absent-or-null fields when the envelope lacks them; a bad matcher rejects the file.

  • runner/command: stdin delivery and stdout capture; exit code and stderr; stderr tail; a signalled process; working directory, injected PATH, protocol version, and invocation variables; inherited PATH when nothing was captured; the command string runs as written; a timeout kills the script and its background child; the cancel flag stops a running hook; a pre-set flag spawns nothing; a missing shell and a missing working directory are spawn failures; a hook that floods stderr before reading a 300 KB stdin does not deadlock; a hook that exits 2 while a background child holds its pipes is reported as exit 2 with its output, within the pipe grace, and the child is left alone; a hook that never reads stdin still finishes.

  • e2e_slice_tests (criteria 1 to 4): deny with agent_message and a routed user notice; silent exit 2; allow; empty output fails open; non-JSON denies; failClosed with exit 1 denies while fail-open does not; the Cursor stdin shape as the script saw it; the hook runs in the file's directory; a timed-out script is killed and its pid is gone; a fail-closed timeout denies; a non-matching matcher leaves the command alone; reloading a file replaces its bindings; a missing or invalid file is an error that changes nothing.

  • execute_tests (criterion 5): a real script loaded through install_explicit_source and run through CommandHookRunner denies a shell action with HookDenied { command, reason }, and the recording adapter saw nothing.

  • I have manually tested my changes locally with ./script/run

Manual testing is deferred to the PR 9 instructions; the flag is off and no source activates in production yet.

Agent Mode

  • Warp Agent Mode - This PR was created via Warp's AI Agent Mode

CHANGELOG-NONE

Activity

23
D
c575c7f
vorporeal9d ago

WARNING

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite. Learn more

  • #16605
  • #16602
  • #16593
  • #16590
  • #16588
  • #16583
  • #16582
  • #16579 👈 (View in Graphite)
  • #16577
  • #16558
  • master

This stack of pull requests is managed by Graphite. Learn more about stacking.

D
de31aea

This pull request is a draft

Merging is unavailable until it is marked ready for review.