feat(code): add Hooks v2 session runtime - #4915
feat(code): add Hooks v2 session runtime#4915Johannes du Plessis (johannes117) wants to merge 1 commit into
Conversation
Freeze hook configuration per session and materialize safe transcript projections so lifecycle integrations can execute against one consistent runtime. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
The Hooks v2 runtime unconditionally loads and executes project-local hook commands from {cwd}/.deepagents/hooks.json without any trust gate, enabling arbitrary command execution when a user opens the CLI in a malicious repository. The timeout-bypass finding is a false positive — subprocesses are already started with start_new_session=True, making killpg(pid, ...) correct.
| diagnostics: list[HookDiagnostic] = [] | ||
| merged: dict[HookEvent, list[MatcherGroup]] = {} | ||
| loaded_paths: list[Path] = [] | ||
|
|
There was a problem hiding this comment.
The default source list unconditionally includes project-local hook configuration:
sources = (
tuple(paths)
if paths is not None
else (
project_hooks_path(cwd),
user_hooks_path(config_dir),
)
)Those project hooks can define command handlers executed via asyncio.create_subprocess_shell(). A malicious repository can commit .deepagents/hooks.json with a lifecycle hook to run arbitrary commands when a user opens the CLI in that checkout — no additional interaction required.
Remediation: Require explicit user approval before including project_hooks_path(cwd). Store a fingerprint of the approved file in the user config directory and re-prompt on change; default to user hooks only until the project file is trusted.
Attack Path
- Attacker commits
.deepagents/hooks.jsonwith aSessionStartcommand payload. - Victim runs the CLI in the repository;
HooksRuntime.create(cwd=cwd)is called. load_hooks_configaddsproject_hooks_path(cwd)to sources without any trust check._read_hooks_documentparses the attacker-controlled file.run_command_handlerpasses thecommandstring toasyncio.create_subprocess_shell(), executing the payload with the victim's privileges.
For more details, see the finding in Corridor.
Provide feedback: Reply with whether this is a valid vulnerability or false positive to help improve Corridor's accuracy.
| self.root = root.expanduser().resolve() | ||
| self.retention_revisions = retention_revisions | ||
| self._buffers: dict[tuple[str, str | None], _TranscriptBuffer] = {} | ||
| self._lock = threading.RLock() | ||
| _ensure_private_directories(self.root, self.root) |
There was a problem hiding this comment.
🟠 Symlinked transcript root escapes the checkout
root.expanduser().resolve() follows a project-controlled .deepagents/transcripts symlink and then treats the resolved destination as the trusted store root. With the default runtime path, a checkout containing .deepagents/transcripts -> /some/outside/dir causes transcript files to be created outside the checkout (and _ensure_private_directories chmods that external directory). I reproduced this with a symlinked project transcript directory: materialize() returned a path under the symlink target and Path.is_relative_to(repo) was false. Reject a pre-existing symlink / enforce that the canonical default root remains beneath the canonical checkout before creating or chmodding anything.
(Refers to lines 130-134)
Your feedback helps Open SWE learn. React with 👍 or 👎 to tell us if this review comment was useful.
| project_hooks_path(cwd), | ||
| user_hooks_path(config_dir), |
There was a problem hiding this comment.
🟡 Home-directory sessions load every hook twice
When cwd is the user's home directory, the project path and default user path are both ~/.deepagents/hooks.json. The loader iterates both entries without deduplicating them, so every matcher group is appended twice and each matching command runs twice. This is especially visible for side-effecting notification and SessionEnd hooks. Canonicalize/deduplicate the source paths while preserving precedence before reading them.
(Refers to lines 95-96)
Your feedback helps Open SWE learn. React with 👍 or 👎 to tell us if this review comment was useful.
| if os.name == "posix" and process.pid is not None: | ||
| try: | ||
| os.killpg(process.pid, signal.SIGKILL) | ||
| except ProcessLookupError: | ||
| pass | ||
| except OSError: | ||
| with suppress(OSError): | ||
| process.kill() | ||
| else: | ||
| with suppress(OSError): | ||
| process.kill() | ||
| with suppress(OSError, TimeoutError): | ||
| await asyncio.wait_for(process.wait(), timeout=_TERMINATE_WAIT_TIMEOUT) |
There was a problem hiding this comment.
🟡 Timed-out hooks leave grandchild processes behind
Killing the entire process group with SIGKILL terminates the direct shell before it can reap its children; in container environments whose PID 1 does not reap orphans promptly, those grandchildren remain as zombies. The two new cleanup tests reproduce this here: both test_runner_kills_process_group_on_timeout and ..._on_cancellation fail because os.kill(grandchild_pid, 0) still succeeds after _terminate() returns. This can accumulate process-table entries across timed-out/cancelled hooks. Terminate descendants first (giving the shell a chance to reap), then force-kill/reap the direct child as a fallback.
(Refers to lines 247-259)
Your feedback helps Open SWE learn. React with 👍 or 👎 to tell us if this review comment was useful.
Closes DCD-70
Hooks v2 now freezes configuration once per session, materializes safe transcript projections, and executes events through a client-owned runtime.
Test plan
uv run --group test pytest tests/unit_tests/hooks— 101 passedmake lintmake check_importsmake test— 10,319 passed; 3 terminal-width assertions failed in untouched Rich rendering tests