feat(runtime): add trusted PreToolUse policy hooks - #2910
Conversation
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Codex automated review
The central ownership choice looks sound: one Runtime Host-composed dispatcher enters through ToolRuntime after preflight and before T1, so a denial cannot grant permission or create a durable attempt. The real-process and end-to-end coverage also exercises more than a fake-only seam.
I found three reproducible contract gaps, all rated P2 below. I did not find a P0/P1 issue, low-value test block worth deleting, or an independent product slice that would become easier to verify if split from this end-to-end backend contract.
Disclosure: This is an automated review performed by Codex using delegated adversarial review passes and a final evidence check. It has not been independently verified by Astro-Han or another human reviewer, does not constitute human approval, and does not represent the final judgment of a human reviewer.
7589253 to
c138db7
Compare
c138db7 to
eeba92f
Compare
|
I think the runtime integration is directionally sound, especially the single
I am not concerned that trust hashes the command definition rather than executable contents; treating integrity of a user-trusted local command as the user responsibility is a reasonable boundary. |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
This head fixes the earlier snapshot lifetime, Host-wide concurrency, and cross-process recursion issues, and the pre-T1 denial boundary is well placed. One process-ownership gap remains, and the branch currently conflicts with main in the Runtime event/TurnOrigin authority.
The first-principles model should be that the hook runner owns one killable OS job/process group for the entire command lifetime, independent of whether the original child PID has already exited. Rebase first, preserve both current TurnOrigin validation and the new hook event schema, then make termination target that durable ownership unit and test a root that spawns a grandchild and exits before timeout.
Reviewed with Codex using two independent review passes; I verified the process-tree path and current merge conflict against this head and current main.
中文
当前 head 已修复之前的 snapshot 生命周期、Host 全局并发和跨进程递归问题，T1 之前的拒绝边界也正确。仍有一个进程所有权缺口，并且当前分支与 main 的 Runtime event/TurnOrigin 权威冲突。
第一性原理下，hook runner 应拥有一个在整个命令生命周期内都可终止的 OS job/process group，不依赖原始 child PID 是否已经退出。先 rebase 并保留两边的权威逻辑，再按该 ownership unit 终止，并测试 root 先退出、grandchild 仍存活时的 timeout。
本次由 Codex 进行两轮独立审查，并核对了当前 head 与最新 main。
| let inputError: string | undefined; | ||
| const timer = setTimeout(() => { | ||
| timedOut = true; | ||
| lifecycle.terminate(); |
There was a problem hiding this comment.
P1 — Timeout can leave a grandchild running after the root exits. lifecycle.terminate() ultimately skips process-group termination once the original child reports exited. If that child spawned a grandchild inheriting stdio and exited first, timeout/abort can complete without killing the surviving process. Track a killable process group/job independently of the root PID's exit state, and add a real grandchild regression where the root exits before timeout.
|
/agentic_review |
Code Review by Qodo
1. Malformed project config bypasses policy
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| const [userConfig, projectConfig, trust] = await Promise.all([ | ||
| input.userConfig.get(), | ||
| readHookConfigFile(join(input.header.cwd, '.maka', 'hooks.json')), | ||
| input.trust.get(), |
There was a problem hiding this comment.
1. Malformed project config bypasses policy 🐞 Bug ⛨ Security
Fix-now: loadSnapshot loads user and project configuration together, so a malformed, oversized,
unreadable, or invalid project .maka/hooks.json rejects the entire snapshot; ToolRuntime then
fails open and dispatches the tool without enforcing otherwise-valid trusted user hooks. An
untrusted project can trigger this bypass with content such as {} or more than 128 handlers,
without gaining hook trust.
Agent Prompt
## Issue description
An invalid project hook file currently rejects the shared snapshot load, causing the caller's fail-open handling to skip trusted user hooks and dispatch the tool.
## Issue Context
Project configuration is untrusted until exact-definition trust is established and must not control whether separately configured user policies are loaded. Load the project configuration independently and, on read, parse, or normalization failure, use the empty hook configuration for that source while preserving the successfully loaded user configuration and trust snapshot; reuse the existing per-source loaders, keep fail-open handling for genuine hook execution/runtime failures, and add no new public configuration or authority.
## Fix Focus Areas
- packages/runtime-host/src/server/host-hook-composition.ts[69-96]
- packages/runtime/src/tool-runtime.ts[1267-1298]
- packages/runtime-host/src/__tests__/host-hook-composition.test.ts[19-108]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| try { | ||
| text = await readFile(path, 'utf8'); | ||
| } catch (error) { | ||
| if ((error as NodeJS.ErrnoException).code === 'ENOENT') return createDefaultHookConfig(); |
There was a problem hiding this comment.
2. Config limit reads unbounded file 🐞 Bug ⛨ Security
Fix-now: readHookConfigFile calls readFile before enforcing the 1 MiB limit, so a project-controlled .maka/hooks.json can force the host to allocate and decode an arbitrarily large file on Turn snapshot creation. This defeats the PR's bounded configuration contract and can cause severe memory pressure before the file is rejected.
Agent Prompt
## Issue description
The project configuration size limit is checked only after `readFile` has allocated and decoded the entire file.
## Issue Context
Reuse a bounded file-read seam that opens the file, rejects oversized/non-regular inputs before allocation, and preferably avoids following symlinks. A new bounded-read helper is necessary because `readFile` cannot enforce a pre-allocation byte limit; this adds one internal helper and associated filesystem edge-case tests, but no public surface or configuration.
## Fix Focus Areas
- packages/storage/src/hook-config-store.ts[29-40]
- packages/storage/src/__tests__/hook-config-store.test.ts[38-85]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| const stdout = result.stdout.trim(); | ||
| if (!stdout) return auditFor(definition, input, 'allowed', durationMs); |
There was a problem hiding this comment.
3. Dropped stdout becomes allow 🐞 Bug ⛨ Security
Fix-now: an oversized unterminated stdout line is deliberately erased by BashTailBuffer, but the runner discards the buffer's hasDroppedUnsafe() state and the engine interprets the resulting empty stdout as an allow. A hook that emits a deny decision plus sufficient same-line output can therefore be converted from deny to allow.
Agent Prompt
## Issue description
Unsafe stdout truncation becomes indistinguishable from genuinely empty stdout, which changes a policy decision into allow.
## Issue Context
Reuse `BashTailBuffer.hasDroppedUnsafe()` and the existing command failure path. The smallest correction is to return an existing failure indication after materializing stdout when unsafe content was dropped; no new decision type or public surface is needed.
## Fix Focus Areas
- packages/runtime/src/hooks/command-runner.ts[56-61]
- packages/runtime/src/hooks/command-runner.ts[93-109]
- packages/runtime/src/hooks/engine.ts[193-206]
- packages/runtime/src/__tests__/hooks-engine.test.ts[184-212]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| const abort = () => { | ||
| aborted = true; | ||
| lifecycle.terminate(); | ||
| }; | ||
| abortSignal.addEventListener('abort', abort, { once: true }); |
There was a problem hiding this comment.
4. Abort can miss spawned hook 🐞 Bug ☼ Reliability
Fix-now: cancellation is checked before spawning and then subscribed only after the child and lifecycle are created, with no post-subscription recheck. If the Turn aborts in that interval, the event is missed and the hook process can continue until normal exit or timeout instead of being terminated with the Turn.
Agent Prompt
## Issue description
An abort between the initial check and listener registration is never observed by the hook runner.
## Issue Context
Make the smallest local correction: after registering the existing listener, immediately recheck `abortSignal.aborted` and invoke the same termination callback. No new state, branch authority, or public API is required.
## Fix Focus Areas
- packages/runtime/src/hooks/command-runner.ts[34-50]
- packages/runtime/src/hooks/command-runner.ts[75-93]
- packages/runtime/src/__tests__/hooks-engine.test.ts[265-284]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| return this.serial(async () => { | ||
| const current = await this.read(); | ||
| const next = normalizeHookTrust({ | ||
| version: HOOK_TRUST_VERSION, |
There was a problem hiding this comment.
5. Concurrent revocation can be lost 🐞 Bug ⛨ Security
Fix-now: FileHookTrustStore protects read-modify-write only with an instance-local queue, so separate stores for the same state root can read the same trust file and overwrite each other's atomic renames. A concurrent revoke(A) and trust(B) can end with [A,B], silently restoring the revoked executable definition.
Agent Prompt
## Issue description
Trust mutations from separate store instances are not serialized, allowing lost additions and, critically, lost revocations.
## Issue Context
Consolidate mutation authority for each canonical trust-file path and reuse the storage package's keyed write-queue seam rather than adding another per-instance queue. If multiple host processes may mutate this file, the same invariant also requires a filesystem lock or a single host-owned mutation authority; this introduces lock lifecycle/error handling only if process-level consolidation is insufficient.
## Fix Focus Areas
- packages/storage/src/hook-trust-store.ts[22-24]
- packages/storage/src/hook-trust-store.ts[70-112]
- packages/storage/src/hook-trust-store.ts[147-159]
- packages/storage/src/write-queue.ts[1-25]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
3 participants
Summary
Introduce the backend foundation for user- and project-configurable
PreToolUsepolicy hooks.This PR implements the first two backend phases of #2908:
ToolRuntimeintegration point before durable dispatch (T1) and tool side effects;The goal is to support policies such as blocking
git push, rejecting dangerous shell commands, or protecting project paths without allowing Hooks to bypass Maka's existing permission, sandbox, and durable-execution boundaries.Refs #2908
Configuration and trust contract
Runtime Host loads and merges:
<State Root>/hooks.json;<cwd>/.maka/hooks.json.V1 intentionally supports only
PreToolUsecommand handlers. Matchers are bounded to*, exact-name unions such asBash|Write, and trailing-prefix matches such asmcp__github__*. Commands must be absolute executable paths and are spawned with argv directly, never through an implicit shell.{ "version": 1, "hooks": { "PreToolUse": [ { "matcher": "Bash|Write|Edit|apply_patch", "hooks": [ { "id": "project-policy", "type": "command", "command": "/absolute/path/to/check-tool-policy", "args": [], "timeoutMs": 3000, "enabled": true } ] } ] } }Each enabled definition is normalized and hashed from its source, project identity, event, matcher, command, arguments, and timeout. Trust records live in
<State Root>/hook-trust.json; an execution-relevant change produces a new hash and is skipped until that exact definition is trusted again.The effective Hook set is snapshotted once per Turn, so configuration changes cannot alter policy midway through an active Turn.
Execution semantics
Each matching trusted handler receives versioned JSON on stdin containing the session, Turn, run, tool call, validated tool input, cwd, permission mode, and origin.
Decision handling is deliberately narrow:
exit 0with empty stdoutexit 0with valid structured outputallowordenydecisionexit 2skipped_untrustedMatching handlers execute concurrently under a global limit. Any explicit denial wins, and multiple denial reasons are returned in stable configuration order rather than process-completion order.
The Hook gate runs in
ToolRuntime.executeTool()after the existing admission/preflight guards and beforeprepareDurableToolAttempt():An allow result therefore does not grant permission or weaken any existing execution boundary.
Security and failure boundaries
shell: false, the Host-resolved cwd, and a minimal environment allowlist.hookCompletedRuntimeEvents; raw tool input and complete process output are not copied into the transcript.Verification
Local verification:
2755 passed / 9 skipped / 0 failed831 passed / 0 failedThe added tests cover:
exit 0,exit 2, and structured allow/deny output;git pushpolicy fixture spanning config, trust, Runtime Host,ToolRuntime, SQLite audit persistence, and the synthetic tool result.CI for
7589253f5is green:Follow-up scope
This PR does not add the product-facing Hooks settings page, trust-review dialog, diagnostics UI, fixture-based “Test hook” action, or migration documentation. Those remain the third delivery phase tracked in #2908. Until that surface lands, this PR should be reviewed as the backend contract and runtime enforcement path, not as the complete end-user workflow.
Checklist
Does this PR entail a change in behavior?
PreToolUseHooks can deny a tool before durable dispatch