From 9cafb5e31fe74443f95b3c46b624b9cb36c83e01 Mon Sep 17 00:00:00 2001 From: andreihasna Date: Sun, 19 Jul 2026 10:36:14 +0300 Subject: [PATCH 1/8] fix: bound standalone hook execution --- hooks/bounded-process.d.ts | 33 +++ hooks/bounded-process.js | 409 +++++++++++++++++++++++++++ hooks/codewith-native-common.test.ts | 154 +++++++++- hooks/codewith-native-common.ts | 55 ++-- hooks/session-start/src/hook.ts | 25 +- hooks/stop-sync/src/hook.ts | 13 +- src/cli/cli.test.ts | 169 ++++++++++- src/cli/index.tsx | 75 +++-- src/hooks/codewith-native.test.ts | 58 +++- src/index.test.ts | 138 +++++++++ src/index.ts | 56 +++- src/lib/profiles.ts | 40 ++- src/lib/registry.ts | 28 ++ 13 files changed, 1154 insertions(+), 99 deletions(-) create mode 100644 hooks/bounded-process.d.ts create mode 100644 hooks/bounded-process.js diff --git a/hooks/bounded-process.d.ts b/hooks/bounded-process.d.ts new file mode 100644 index 0000000..3c4d407 --- /dev/null +++ b/hooks/bounded-process.d.ts @@ -0,0 +1,33 @@ +export type HookNetworkAccess = "deny" | "allow"; + +export interface BoundedProcessOptions { + cwd?: string; + env?: NodeJS.ProcessEnv; + /** Extra non-sensitive names to forward; loader and credential-like names are rejected. */ + envAllowlist?: readonly string[]; + input?: string | Uint8Array; + timeoutMs?: number; + maxInputBytes?: number; + maxStdoutBytes?: number; + maxStderrBytes?: number; + network?: HookNetworkAccess; + /** Override the platform containment binary path, primarily for deterministic validation. */ + containmentExecutable?: string; +} + +export interface BoundedProcessResult { + exitCode: number | null; + signal: NodeJS.Signals | null; + stdout: string; + stderr: string; + timedOut: boolean; + error: string | null; +} + +export const DEFAULT_MAX_INPUT_BYTES: number; +export const DEFAULT_MAX_STDOUT_BYTES: number; +export const DEFAULT_MAX_STDERR_BYTES: number; +export const DEFAULT_TIMEOUT_MS: number; + +export function readBoundedStdin(maxBytes?: number): string; +export function runBoundedProcess(argv: string[], options?: BoundedProcessOptions): Promise; diff --git a/hooks/bounded-process.js b/hooks/bounded-process.js new file mode 100644 index 0000000..897487b --- /dev/null +++ b/hooks/bounded-process.js @@ -0,0 +1,409 @@ +import { spawn } from "node:child_process"; +import { existsSync, readFileSync, readSync } from "node:fs"; +import { delimiter, join } from "node:path"; + +export const DEFAULT_MAX_INPUT_BYTES = 64 * 1024; +export const DEFAULT_MAX_STDOUT_BYTES = 64 * 1024; +export const DEFAULT_MAX_STDERR_BYTES = 64 * 1024; +export const DEFAULT_TIMEOUT_MS = 10_000; + +const MAX_CONCURRENT_PROCESSES = 4; +const MAX_QUEUED_PROCESSES = 32; +const TERMINATION_GRACE_MS = 125; +const INTERNAL_CONTAINED_ENV = "HASNA_HOOKS_INTERNAL_CONTAINED"; +const INTERNAL_NETWORK_ENV = "HASNA_HOOKS_INTERNAL_NETWORK"; + +function isBubblewrapPidNamespace() { + if (process.platform !== "linux") return false; + try { + const initCommand = readFileSync("/proc/1/cmdline", "utf8").split("\0"); + return initCommand[0]?.endsWith("/bwrap") + && initCommand.includes("--die-with-parent") + && initCommand.includes("--unshare-pid"); + } catch { + return false; + } +} + +const INHERITED_CONTAINMENT = process.env[INTERNAL_CONTAINED_ENV] === "1" && isBubblewrapPidNamespace(); +const INHERITED_NETWORK = process.env[INTERNAL_NETWORK_ENV] === "deny" ? "deny" : "allow"; + +const SAFE_ENV_NAMES = Object.freeze([ + "PATH", + "HOME", + "TMPDIR", + "TMP", + "TEMP", + "LANG", + "LC_ALL", + "LC_CTYPE", + "TZ", + "TERM", + "COLORTERM", + "NO_COLOR", + "FORCE_COLOR", + "USER", + "LOGNAME", + "XDG_CONFIG_HOME", + "XDG_CACHE_HOME", + "XDG_DATA_HOME", + "CLAUDE_CODE_TASK_LIST_ID", + "CLAUDE_ENV_FILE", + "CLAUDE_PROJECT_DIR", + "CHECK_TASKS_DISABLED", + "CHECK_TASKS_KEYWORDS", + "CODEWITH_AGENT_NAME", + "CODEWITH_RUN_ID", + "CODEWITH_SUBAGENT", + "CODEWITH_TASK_ID", + "CODEWITH_WORKSPACE_ROOTS", + "CONVERSATIONS_AGENT_ID", + "COST_WATCH_BUDGET", + "HASNA_ACTIVE_REPO_ROOTS", + "HASNA_ACTIVE_WORKTREE_ROOTS", + "HASNA_HOOKS_CACHE_DIR", + "HASNA_HOOKS_CODEWITH_CONFIG_PATH", + "HASNA_HOOKS_DATA_DIR", + "HASNA_HOOKS_DB_PATH", + "HASNA_HOOKS_IDENTITY_CACHE_MS", + "HASNA_HOOKS_SESSION_START_CACHE_MS", + "HASNA_HOOKS_STOP_SYNC_TASK_COMMENT", + "HASNA_REPOS_WORKTREES_ROOT", + "HASNA_RUN_ID", + "HASNA_SUBAGENT", + "HASNA_TASK_ID", + "HASNA_WORKSPACE_ROOTS", + "HOOKS_AGENT_NAME", + "HOOKS_DATA_DIR", + "HOOKS_DB_PATH", + "HOOKS_FLEET_AGENT", + "HOOKS_FLEET_CATCHUP_DISABLE", + "HOOKS_FLEET_GATE_DISABLE", + "HOOKS_FLEET_GATE_TTL_MS", + "HOOKS_FLEET_SINCE", + "HOOKS_FLEET_TIMEOUT_MS", + "HOOKS_RETENTION_DAYS", + "HOOKS_RULES_CHECK_DISABLE", + "HOOKS_RULES_CONFIG_SLUG", + "HOOKS_RULES_EXPECTED_VERSION", + "HOOKS_RULES_FILES", + "HOOKS_SOUND_FILE", + "HOOKS_SPACE", + "RUN_ID", + "SMSG_AGENT_ID", + "SMSG_PROJECT_ID", + "TASK_ID", + "VIRTUAL_ENV", + "NVM_DIR", +]); + +const FORBIDDEN_ENV_NAMES = new Set([ + "BASH_ENV", + "ENV", + "NODE_OPTIONS", + "BUN_OPTIONS", + "LD_PRELOAD", + "LD_LIBRARY_PATH", + "DYLD_INSERT_LIBRARIES", + "DYLD_LIBRARY_PATH", + "RUBYOPT", + "PERL5OPT", + "PYTHONINSPECT", + "PYTHONSTARTUP", +]); + +const SENSITIVE_ENV_NAME = /(?:^|_)(?:API_?KEY|ACCESS_?KEY|PRIVATE_?KEY|SECRET|TOKEN|PASSWORD|PASSWD|CREDENTIALS?|AUTHORIZATION|COOKIE|WEBHOOK|DATABASE_?URL|DB_?URL)(?:_|$)|^(?:AWS|AZURE|GOOGLE|GITHUB|GITLAB|NPM|OPENAI|ANTHROPIC|STRIPE|TWILIO|SLACK)_/i; + +let activeProcesses = 0; +const processQueue = []; + +function positiveInteger(value, fallback, maximum = Number.MAX_SAFE_INTEGER) { + if (!Number.isFinite(value) || value <= 0) return fallback; + return Math.min(Math.floor(value), maximum); +} + +function byteLength(value) { + if (typeof value === "string") return Buffer.byteLength(value); + if (value instanceof Uint8Array) return value.byteLength; + return 0; +} + +function failedResult(message, overrides = {}) { + return { + exitCode: null, + signal: null, + stdout: "", + stderr: "", + timedOut: false, + error: message, + ...overrides, + }; +} + +function acquireProcessSlot(waitMs) { + if (activeProcesses < MAX_CONCURRENT_PROCESSES) { + activeProcesses += 1; + return Promise.resolve("acquired"); + } + if (processQueue.length >= MAX_QUEUED_PROCESSES) return Promise.resolve("full"); + return new Promise((resolve) => { + const entry = { resolve, timer: null, active: true }; + entry.timer = setTimeout(() => { + if (!entry.active) return; + entry.active = false; + const index = processQueue.indexOf(entry); + if (index !== -1) processQueue.splice(index, 1); + resolve("timeout"); + }, Math.max(1, waitMs)); + processQueue.push(entry); + }); +} + +function releaseProcessSlot() { + while (processQueue.length > 0) { + const next = processQueue.shift(); + if (!next?.active) continue; + next.active = false; + clearTimeout(next.timer); + next.resolve("acquired"); + return; + } + activeProcesses = Math.max(0, activeProcesses - 1); +} + +function safeEnvironment(source, extraNames = []) { + const output = {}; + for (const name of extraNames) { + if (!/^[A-Z_][A-Z0-9_]*$/.test(name) || FORBIDDEN_ENV_NAMES.has(name) || SENSITIVE_ENV_NAME.test(name)) { + return { error: `environment name '${name}' is not permitted` }; + } + } + const names = new Set([...SAFE_ENV_NAMES, ...extraNames]); + for (const name of names) { + if (!/^[A-Z_][A-Z0-9_]*$/.test(name) || FORBIDDEN_ENV_NAMES.has(name)) continue; + const value = source?.[name]; + if (typeof value === "string") output[name] = value; + } + if (!output.PATH) output.PATH = "/usr/local/bin:/usr/bin:/bin"; + if (INHERITED_CONTAINMENT) { + output[INTERNAL_CONTAINED_ENV] = "1"; + output[INTERNAL_NETWORK_ENV] = INHERITED_NETWORK; + } + return { env: output }; +} + +function executableOnPath(command, env) { + if (command.includes("/")) return existsSync(command) ? command : null; + for (const directory of (env.PATH || "").split(delimiter)) { + if (!directory) continue; + const candidate = join(directory, command); + if (existsSync(candidate)) return candidate; + } + return null; +} + +function prepareCommand(argv, network, env, containmentExecutable) { + if (process.platform === "win32") { + return { error: "hook command execution is disabled on Windows because descendant process containment is unavailable" }; + } + + if (process.platform === "linux") { + if (INHERITED_CONTAINMENT) { + if (INHERITED_NETWORK === "allow" && network === "deny") { + return { error: "nested hook cannot strengthen network isolation inside an allow-network container" }; + } + return { argv }; + } + const configured = typeof containmentExecutable === "string" ? containmentExecutable : null; + const bwrap = configured + ? (existsSync(configured) ? configured : null) + : (existsSync("/usr/bin/bwrap") ? "/usr/bin/bwrap" : executableOnPath("bwrap", env)); + if (!bwrap) return { error: "contained hook execution requires bubblewrap on Linux" }; + const networkArgs = network === "deny" ? ["--unshare-net"] : []; + env[INTERNAL_CONTAINED_ENV] = "1"; + env[INTERNAL_NETWORK_ENV] = network; + return { + argv: [ + bwrap, + "--new-session", + "--die-with-parent", + "--bind", "/", "/", + "--dev-bind", "/dev", "/dev", + "--unshare-user", + "--unshare-pid", + "--proc", "/proc", + ...networkArgs, + "--", + ...argv, + ], + }; + } + + if (network === "deny" && process.platform === "darwin") { + const sandboxExec = existsSync("/usr/bin/sandbox-exec") ? "/usr/bin/sandbox-exec" : null; + if (!sandboxExec) return { error: "network-denied hook execution requires sandbox-exec on macOS" }; + return { + argv: [sandboxExec, "-p", "(version 1) (allow default) (deny network*)", ...argv], + }; + } + + if (network === "deny") { + return { error: `network-denied hook execution is unavailable on ${process.platform}` }; + } + return { argv }; +} + +function delay(ms) { + return new Promise((resolve) => setTimeout(resolve, ms)); +} + +async function terminateProcessTree(child) { + const pid = child.pid; + const send = (signal) => { + if (typeof pid === "number" && pid > 0) { + try { + process.kill(-pid, signal); + return; + } catch {} + } + try { child.kill(signal); } catch {} + }; + + send("SIGTERM"); + await delay(TERMINATION_GRACE_MS); + send("SIGKILL"); + await delay(25); +} + +function appendCapped(chunks, chunk, state, limit, streamName, stop) { + const buffer = Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk); + const remaining = Math.max(0, limit - state.bytes); + if (remaining > 0) chunks.push(buffer.subarray(0, remaining)); + state.bytes += Math.min(buffer.byteLength, remaining); + if (buffer.byteLength > remaining && !state.error) { + state.error = `${streamName} exceeds ${limit} bytes`; + stop(); + } +} + +export function readBoundedStdin(maxBytes = DEFAULT_MAX_INPUT_BYTES) { + const limit = positiveInteger(maxBytes, DEFAULT_MAX_INPUT_BYTES); + const chunks = []; + let total = 0; + const buffer = Buffer.allocUnsafe(8 * 1024); + while (true) { + const read = readSync(0, buffer, 0, buffer.byteLength, null); + if (read === 0) break; + if (total + read > limit) throw new Error(`hook input exceeds ${limit} bytes`); + chunks.push(Buffer.from(buffer.subarray(0, read))); + total += read; + } + return Buffer.concat(chunks, total).toString("utf8"); +} + +export async function runBoundedProcess(argv, options = {}) { + if (!Array.isArray(argv) || argv.length === 0 || argv.some((part) => typeof part !== "string" || part.length === 0)) { + return failedResult("hook command must be a non-empty argument vector"); + } + + const timeoutMs = positiveInteger(options.timeoutMs, DEFAULT_TIMEOUT_MS, 24 * 60 * 60 * 1000); + const deadline = Date.now() + timeoutMs; + const maxInputBytes = positiveInteger(options.maxInputBytes, DEFAULT_MAX_INPUT_BYTES); + const maxStdoutBytes = positiveInteger(options.maxStdoutBytes, DEFAULT_MAX_STDOUT_BYTES); + const maxStderrBytes = positiveInteger(options.maxStderrBytes, DEFAULT_MAX_STDERR_BYTES); + const input = options.input ?? ""; + if (typeof input !== "string" && !(input instanceof Uint8Array)) { + return failedResult("hook input must be a string or Uint8Array"); + } + if (byteLength(input) > maxInputBytes) { + return failedResult(`hook input exceeds ${maxInputBytes} bytes`); + } + + const network = options.network === "allow" ? "allow" : "deny"; + const sanitized = safeEnvironment(options.env ?? process.env, options.envAllowlist ?? []); + if (sanitized.error) return failedResult(sanitized.error); + const env = sanitized.env; + const prepared = prepareCommand(argv, network, env, options.containmentExecutable); + if (prepared.error) return failedResult(prepared.error); + + const admission = await acquireProcessSlot(Math.max(1, deadline - Date.now())); + if (admission === "full") { + return failedResult(`hook process queue exceeds ${MAX_QUEUED_PROCESSES} waiting commands`); + } + if (admission === "timeout") { + return failedResult(`hook command timed out after ${timeoutMs} ms while waiting for a process slot`, { timedOut: true }); + } + + try { + const executionTimeoutMs = deadline - Date.now(); + if (executionTimeoutMs <= 0) { + return failedResult(`hook command timed out after ${timeoutMs} ms while waiting for a process slot`, { timedOut: true }); + } + let child; + try { + child = spawn(prepared.argv[0], prepared.argv.slice(1), { + cwd: options.cwd, + env, + detached: true, + stdio: ["pipe", "pipe", "pipe"], + }); + } catch { + return failedResult("failed to start hook command"); + } + + const stdoutChunks = []; + const stderrChunks = []; + const stdoutState = { bytes: 0, error: null }; + const stderrState = { bytes: 0, error: null }; + let timedOut = false; + let spawnError = null; + let stopPromise = null; + const stop = () => { + if (!stopPromise) stopPromise = terminateProcessTree(child); + }; + + child.stdout?.on("data", (chunk) => appendCapped(stdoutChunks, chunk, stdoutState, maxStdoutBytes, "hook stdout", stop)); + child.stderr?.on("data", (chunk) => appendCapped(stderrChunks, chunk, stderrState, maxStderrBytes, "hook stderr", stop)); + // A fast-exiting hook may close stdin before the bounded payload flushes. + // EPIPE is an expected child lifecycle outcome and must not crash the host. + child.stdin?.on("error", () => {}); + + const completion = new Promise((resolve) => { + child.once("error", () => { + spawnError = "failed to start hook command"; + resolve({ exitCode: null, signal: null }); + }); + child.once("close", (exitCode, signal) => resolve({ exitCode, signal })); + }); + + const timer = setTimeout(() => { + timedOut = true; + stop(); + }, executionTimeoutMs); + + try { + child.stdin?.end(input); + } catch { + stop(); + } + + const completed = await completion; + clearTimeout(timer); + if (stopPromise) await stopPromise; + + const error = spawnError + || stdoutState.error + || stderrState.error + || (timedOut ? `hook command timed out after ${timeoutMs} ms` : null); + return { + exitCode: completed.exitCode, + signal: completed.signal, + stdout: Buffer.concat(stdoutChunks, stdoutState.bytes).toString("utf8"), + stderr: Buffer.concat(stderrChunks, stderrState.bytes).toString("utf8"), + timedOut, + error, + }; + } finally { + releaseProcessSlot(); + } +} diff --git a/hooks/codewith-native-common.test.ts b/hooks/codewith-native-common.test.ts index 4916e2b..d83246d 100644 --- a/hooks/codewith-native-common.test.ts +++ b/hooks/codewith-native-common.test.ts @@ -1,8 +1,9 @@ import { describe, expect, test } from "bun:test"; import { join } from "path"; -import { mkdtempSync, rmSync } from "fs"; +import { mkdtempSync, readFileSync, readlinkSync, rmSync, writeFileSync } from "fs"; import { tmpdir } from "os"; -import { getAgentName, gitCommandInfo, managedWorktreeInfo } from "./codewith-native-common"; +import { getAgentName, gitCommandInfo, managedWorktreeInfo, runCommand } from "./codewith-native-common"; +import { runBoundedProcess } from "./bounded-process.js"; describe("codewith native common helpers", () => { test("gitCommandInfo detects global option commit/push forms and target cwd", () => { @@ -58,4 +59,153 @@ describe("codewith native common helpers", () => { } } }); + + test("runCommand clears shell startup injection and caps stdout", async () => { + const tmp = mkdtempSync(join(tmpdir(), "hooks-command-env-")); + try { + const startup = join(tmp, "startup.sh"); + writeFileSync(startup, "printf startup-injected"); + const env = { + ...process.env, + BASH_ENV: startup, + ENV: startup, + SYNTHETIC_SECRET_SENTINEL: "must-not-leak", + }; + + const startupResult = await runCommand( + ["/bin/bash", "-c", 'printf "%s" "${SYNTHETIC_SECRET_SENTINEL-unset}:body"'], + { env, timeoutMs: 2_000 }, + ); + expect(startupResult.stdout).toBe("unset:body"); + + const oversized = await runCommand( + [process.execPath, "-e", 'process.stdout.write("x".repeat(70_000))'], + { env, timeoutMs: 2_000 }, + ); + expect(Buffer.byteLength(oversized.stdout)).toBeLessThanOrEqual(64 * 1024); + expect((oversized as any).error).toContain("stdout exceeds"); + } finally { + rmSync(tmp, { recursive: true, force: true }); + } + }); + + test("runCommand timeout kills a spawned descendant", async () => { + const tmp = mkdtempSync(join(tmpdir(), "hooks-command-tree-")); + const sentinel = join(tmp, "descendant-survived"); + try { + const result = await runCommand( + ["/bin/sh", "-c", `(sleep 0.6; printf survived > ${JSON.stringify(sentinel)}) & wait`], + { timeoutMs: 150, network: "allow" }, + ); + expect(result.timedOut).toBe(true); + await Bun.sleep(700); + expect(() => readFileSync(sentinel)).toThrow(); + } finally { + rmSync(tmp, { recursive: true, force: true }); + } + }); + + test("runCommand denies a loopback network stub unless explicitly allowed", async () => { + const server = Bun.serve({ + port: 0, + fetch: () => new Response("reachable"), + }); + const script = `fetch("http://127.0.0.1:${server.port}").then(async r => { console.log(await r.text()) }).catch(() => process.exit(23))`; + try { + const parentNamespace = readlinkSync("/proc/self/ns/net"); + const isolated = await runCommand( + ["/usr/bin/readlink", "/proc/self/ns/net"], + { timeoutMs: 2_000, network: "deny" }, + ); + expect(isolated.exitCode).toBe(0); + expect(isolated.error).toBeNull(); + expect(isolated.stdout.trim()).not.toBe(parentNamespace); + + const denied = await runCommand( + [process.execPath, "-e", script], + { timeoutMs: 2_000, network: "deny" } as any, + ); + expect(denied.exitCode).not.toBe(0); + expect(denied.error).toBeNull(); + expect(denied.stdout).not.toContain("reachable"); + + const allowed = await runCommand( + [process.execPath, "-e", script], + { timeoutMs: 2_000, network: "allow" } as any, + ); + expect(allowed.exitCode).toBe(0); + expect(allowed.stdout.trim()).toBe("reachable"); + } finally { + server.stop(true); + } + }); + + test("runCommand fails closed when network containment is unavailable", async () => { + const tmp = mkdtempSync(join(tmpdir(), "hooks-command-no-sandbox-")); + const sentinel = join(tmp, "executed"); + try { + const result = await runBoundedProcess( + [process.execPath, "-e", `await Bun.write(${JSON.stringify(sentinel)}, "executed")`], + { containmentExecutable: join(tmp, "missing-bwrap"), network: "deny" }, + ); + expect(result.exitCode).toBeNull(); + expect(result.error).toContain("requires bubblewrap"); + expect(() => readFileSync(sentinel)).toThrow(); + } finally { + rmSync(tmp, { recursive: true, force: true }); + } + }); + + test("runBoundedProcess rejects sensitive explicit environment names", async () => { + const tmp = mkdtempSync(join(tmpdir(), "hooks-command-sensitive-env-")); + const sentinel = join(tmp, "executed"); + try { + const result = await runBoundedProcess( + [process.execPath, "-e", `await Bun.write(${JSON.stringify(sentinel)}, "executed")`], + { + env: { OPENAI_API_KEY: "synthetic-not-a-credential" }, + envAllowlist: ["OPENAI_API_KEY"], + network: "allow", + }, + ); + expect(result.exitCode).toBeNull(); + expect(result.error).toContain("not permitted"); + expect(() => readFileSync(sentinel)).toThrow(); + } finally { + rmSync(tmp, { recursive: true, force: true }); + } + }); + + test("runBoundedProcess timeout includes queue wait", async () => { + const tmp = mkdtempSync(join(tmpdir(), "hooks-command-queue-")); + const sentinel = join(tmp, "queued-command-executed"); + try { + const occupied = Array.from({ length: 4 }, () => runBoundedProcess( + ["/bin/sh", "-c", "sleep 0.35"], + { timeoutMs: 2_000, network: "allow" }, + )); + const queued = await runBoundedProcess( + [process.execPath, "-e", `await Bun.write(${JSON.stringify(sentinel)}, "executed")`], + { timeoutMs: 50, network: "allow" }, + ); + + expect(queued.timedOut).toBe(true); + expect(queued.error).toContain("waiting for a process slot"); + expect(() => readFileSync(sentinel)).toThrow(); + const completed = await Promise.all(occupied); + expect(completed.every((result) => result.exitCode === 0 && result.error === null)).toBe(true); + } finally { + rmSync(tmp, { recursive: true, force: true }); + } + }); + + test("runBoundedProcess tolerates EPIPE from an immediate-exit child", async () => { + const result = await runBoundedProcess( + ["/bin/true"], + { input: "x".repeat(64 * 1024), timeoutMs: 2_000, network: "allow" }, + ); + expect(result.exitCode).toBe(0); + expect(result.error).toBeNull(); + expect(result.timedOut).toBe(false); + }); }); diff --git a/hooks/codewith-native-common.ts b/hooks/codewith-native-common.ts index 0820362..4f43d22 100644 --- a/hooks/codewith-native-common.ts +++ b/hooks/codewith-native-common.ts @@ -1,6 +1,7 @@ import { existsSync, mkdirSync, readFileSync, writeFileSync } from "fs"; import { basename, dirname, isAbsolute, join, relative, resolve, sep } from "path"; import { homedir, tmpdir } from "os"; +import { runBoundedProcess, type HookNetworkAccess } from "./bounded-process.js"; export interface CodewithHookInput { session_id?: string; @@ -17,6 +18,7 @@ export interface CodewithHookInput { turn_id?: string; last_assistant_message?: string | null; stop_hook_active?: boolean; + dry_run?: boolean; agent_id?: string; agent_type?: string; agent?: unknown; @@ -44,6 +46,7 @@ export interface CommandResult { stdout: string; stderr: string; timedOut: boolean; + error: string | null; } export function readInput(): CodewithHookInput { @@ -80,34 +83,32 @@ export function commandExists(command: string, env: NodeJS.ProcessEnv = process. export async function runCommand( argv: string[], - options: { cwd?: string; timeoutMs?: number; env?: NodeJS.ProcessEnv } = {} + options: { + cwd?: string; + timeoutMs?: number; + env?: NodeJS.ProcessEnv; + envAllowlist?: readonly string[]; + maxStdoutBytes?: number; + maxStderrBytes?: number; + network?: HookNetworkAccess; + } = {} ): Promise { - const timeoutMs = options.timeoutMs ?? 5000; - const env = options.env ?? process.env; - let proc: ReturnType | null = null; - let timedOut = false; - try { - proc = Bun.spawn(argv, { - cwd: options.cwd, - env, - stdin: "ignore", - stdout: "pipe", - stderr: "pipe", - }); - const timer = setTimeout(() => { - timedOut = true; - try { proc?.kill(); } catch {} - }, timeoutMs); - const [stdout, stderr, exitCode] = await Promise.all([ - new Response(proc.stdout).text(), - new Response(proc.stderr).text(), - proc.exited.catch(() => null), - ]); - clearTimeout(timer); - return { exitCode, stdout, stderr, timedOut }; - } catch (error) { - return { exitCode: null, stdout: "", stderr: error instanceof Error ? error.message : String(error), timedOut }; - } + const result = await runBoundedProcess(argv, { + cwd: options.cwd, + timeoutMs: options.timeoutMs ?? 5000, + env: options.env, + envAllowlist: options.envAllowlist, + maxStdoutBytes: options.maxStdoutBytes, + maxStderrBytes: options.maxStderrBytes, + network: options.network ?? "deny", + }); + return { + exitCode: result.error ? null : result.exitCode, + stdout: result.stdout, + stderr: result.stderr, + timedOut: result.timedOut, + error: result.error, + }; } export function getCommand(input: CodewithHookInput): string { diff --git a/hooks/session-start/src/hook.ts b/hooks/session-start/src/hook.ts index 24f518f..5d90580 100644 --- a/hooks/session-start/src/hook.ts +++ b/hooks/session-start/src/hook.ts @@ -30,10 +30,10 @@ async function conversationsDigest(cwd: string): Promise<{ text: string; warning return { text: "conversations CLI unavailable; no blockers/announcements digest injected.", warnings }; } - const blockers = await runCommand(["conversations", "blockers", "--limit", "10", "-j"], { cwd, timeoutMs: 3500 }); + const blockers = await runCommand(["conversations", "blockers", "--limit", "10", "-j"], { cwd, timeoutMs: 3500, network: "allow" }); const announcements = await runCommand([ "conversations", "digest", "announcements", "--unread", "--since", "7d", "--limit", "10", "--max-bytes", "6000", "-j", - ], { cwd, timeoutMs: 4500 }); + ], { cwd, timeoutMs: 4500, network: "allow" }); const parts: string[] = []; if (blockers.exitCode === 0) { @@ -52,6 +52,10 @@ async function conversationsDigest(cwd: string): Promise<{ text: string; warning async function registerIdentity(input: CodewithHookInput, cwd: string): Promise { const notes: string[] = []; + if (input.dry_run === true) { + notes.push("Dry run: skipped identity registration and heartbeats."); + return notes; + } const agentName = getAgentName(input); if (!agentName) { notes.push("No safe agent name env/input found; skipped identity registration."); @@ -68,24 +72,24 @@ async function registerIdentity(input: CodewithHookInput, cwd: string): Promise< } if (commandExists("conversations")) { - await runCommand(["conversations", "agents", "register", agentName, "--session", input.session_id || `codewith-${Date.now()}`], { cwd, timeoutMs: 2500 }); - await runCommand(["conversations", "agents", "heartbeat", "--from", agentName, "--status", "online"], { cwd, timeoutMs: 2000 }); + await runCommand(["conversations", "agents", "register", agentName, "--session", input.session_id || `codewith-${Date.now()}`], { cwd, timeoutMs: 2500, network: "allow" }); + await runCommand(["conversations", "agents", "heartbeat", "--from", agentName, "--status", "online"], { cwd, timeoutMs: 2000, network: "allow" }); notes.push(`conversations heartbeat attempted for ${agentName}.`); } else { notes.push("conversations CLI unavailable; skipped conversations identity."); } if (commandExists("todos")) { - await runCommand(["todos", "init", agentName], { cwd, timeoutMs: 2500 }); - await runCommand(["todos", "heartbeat", agentName], { cwd, timeoutMs: 2000 }); + await runCommand(["todos", "init", agentName], { cwd, timeoutMs: 2500, network: "allow" }); + await runCommand(["todos", "heartbeat", agentName], { cwd, timeoutMs: 2000, network: "allow" }); notes.push(`todos heartbeat attempted for ${agentName}.`); } else { notes.push("todos CLI unavailable; skipped todos heartbeat."); } if (commandExists("mementos")) { - await runCommand(["mementos", "register-agent", agentName], { cwd, timeoutMs: 2500 }); - await runCommand(["mementos", "heartbeat", agentName], { cwd, timeoutMs: 2000 }); + await runCommand(["mementos", "register-agent", agentName], { cwd, timeoutMs: 2500, network: "allow" }); + await runCommand(["mementos", "heartbeat", agentName], { cwd, timeoutMs: 2000, network: "allow" }); notes.push(`mementos heartbeat attempted for ${agentName}.`); } else { notes.push("mementos CLI unavailable; skipped mementos heartbeat."); @@ -97,7 +101,8 @@ async function registerIdentity(input: CodewithHookInput, cwd: string): Promise< async function buildDigest(input: CodewithHookInput): Promise { const cwd = input.cwd || process.cwd(); - const cached = readCache("session-start-digest", DIGEST_TTL_MS); + const dryRun = input.dry_run === true; + const cached = dryRun ? null : readCache("session-start-digest", DIGEST_TTL_MS); if (cached) return cached; const warnings: string[] = []; @@ -115,7 +120,7 @@ async function buildDigest(input: CodewithHookInput): Promise { ].join("\n\n"), 10_000); const digest = { context, warnings }; - writeCache("session-start-digest", digest); + if (!dryRun) writeCache("session-start-digest", digest); return digest; } diff --git a/hooks/stop-sync/src/hook.ts b/hooks/stop-sync/src/hook.ts index 9c7778a..0a5e60f 100644 --- a/hooks/stop-sync/src/hook.ts +++ b/hooks/stop-sync/src/hook.ts @@ -19,19 +19,19 @@ async function heartbeat(input: CodewithHookInput, cwd: string): Promise { const input = readInput(); const cwd = input.cwd || process.cwd(); + if (input.dry_run === true) { + warn("stop-sync dry run: skipped heartbeats and task comments"); + respond({ continue: true, suppressOutput: true }); + return; + } try { const notes = await heartbeat(input, cwd); const comment = await maybeTaskComment(input, cwd); diff --git a/src/cli/cli.test.ts b/src/cli/cli.test.ts index 6e650df..925c6bb 100644 --- a/src/cli/cli.test.ts +++ b/src/cli/cli.test.ts @@ -1,6 +1,6 @@ import { describe, test, expect, beforeEach, afterEach } from "bun:test"; import { join } from "path"; -import { existsSync, readFileSync, writeFileSync, mkdirSync, rmSync } from "fs"; +import { chmodSync, existsSync, readFileSync, writeFileSync, mkdirSync, mkdtempSync, rmSync } from "fs"; import { homedir, tmpdir } from "os"; const CLI = join(import.meta.dir, "index.tsx"); @@ -44,6 +44,39 @@ async function runJson(...args: string[]): Promise { return JSON.parse(stdout.trim()); } +async function runWithEnv(env: Record, ...args: string[]): Promise<{ stdout: string; stderr: string; exitCode: number }> { + const proc = Bun.spawn([process.execPath, "run", CLI, ...args], { + stdout: "pipe", + stderr: "pipe", + env: { ...process.env, ...env, NO_COLOR: "1" }, + }); + const [stdout, stderr, exitCode] = await Promise.all([ + new Response(proc.stdout).text(), + new Response(proc.stderr).text(), + proc.exited, + ]); + return { stdout, stderr, exitCode }; +} + +async function runWithInputAndEnv( + input: string, + env: Record, + ...args: string[] +): Promise<{ stdout: string; stderr: string; exitCode: number }> { + const proc = Bun.spawn([process.execPath, "run", CLI, ...args], { + stdin: new Response(input), + stdout: "pipe", + stderr: "pipe", + env: { ...process.env, ...env, NO_COLOR: "1" }, + }); + const [stdout, stderr, exitCode] = await Promise.all([ + new Response(proc.stdout).text(), + new Response(proc.stderr).text(), + proc.exited, + ]); + return { stdout, stderr, exitCode }; +} + describe("CLI", () => { describe("hooks --version", () => { test("prints version", async () => { @@ -211,6 +244,26 @@ describe("CLI", () => { expect(stdout).toContain("not found"); }); + test("--dry-run does not migrate profiles or touch settings", async () => { + const home = mkdtempSync(join(tmpdir(), "hooks-cli-dry-run-")); + try { + const legacyProfiles = join(home, ".hooks", "profiles"); + const settings = join(home, ".claude", "settings.json"); + mkdirSync(legacyProfiles, { recursive: true }); + mkdirSync(join(home, ".claude"), { recursive: true }); + writeFileSync(join(legacyProfiles, "legacy.json"), '{"agent_id":"legacy"}\n'); + writeFileSync(settings, '{"sentinel":"unchanged"}\n'); + + const result = await runWithEnv({ HOME: home }, "install", "gitguard", "--dry-run", "--json"); + expect(result.exitCode).toBe(0); + expect(JSON.parse(result.stdout).dryRun).toBe(true); + expect(readFileSync(settings, "utf-8")).toBe('{"sentinel":"unchanged"}\n'); + expect(existsSync(join(home, ".hasna", "hooks", "profiles"))).toBe(false); + } finally { + rmSync(home, { recursive: true, force: true }); + } + }); + test("--json shows install result", async () => { const data = await runJson("install", "nonexistent"); expect(data.failed).toHaveLength(1); @@ -402,6 +455,120 @@ describe("CLI", () => { test("run command exists in help", async () => { const { stdout } = await run("run", "--help"); expect(stdout).toContain("Execute a hook"); + expect(stdout).toContain("--deny-network"); + expect(stdout).not.toContain("--allow-network"); + }); + + test("declared remote hook keeps network by default and accepts explicit denial", async () => { + const home = mkdtempSync(join(tmpdir(), "hooks-cli-run-network-")); + let requests = 0; + const server = Bun.serve({ + hostname: "127.0.0.1", + port: 0, + fetch: () => { + requests += 1; + return new Response("ok"); + }, + }); + try { + mkdirSync(join(home, ".claude"), { recursive: true }); + writeFileSync(join(home, ".claude", "settings.json"), JSON.stringify({ + phoneNotifyConfig: { + enabled: true, + topic: "synthetic-network", + server: `http://127.0.0.1:${server.port}`, + }, + })); + const input = JSON.stringify({ + hook_event_name: "Stop", + cwd: home, + }); + const env = { HOME: home, PATH: process.env.PATH ?? "" }; + + const allowed = await runWithInputAndEnv(input, env, "run", "phonenotify"); + expect(allowed.exitCode).toBe(0); + expect(JSON.parse(allowed.stdout).continue).toBe(true); + expect(requests).toBe(1); + + const denied = await runWithInputAndEnv(input, env, "run", "phonenotify", "--deny-network"); + expect(denied.exitCode).toBe(0); + expect(JSON.parse(denied.stdout).continue).toBe(true); + expect(requests).toBe(1); + } finally { + server.stop(); + rmSync(home, { recursive: true, force: true }); + } + }); + + test("--dry-run propagates before profile touch and hook mutations", async () => { + const home = mkdtempSync(join(tmpdir(), "hooks-cli-run-dry-")); + const bin = join(home, "bin"); + const legacyDir = join(home, ".hooks", "profiles"); + const legacyProfile = join(legacyDir, "legacy.json"); + const mutationLog = join(home, "mutations.log"); + try { + mkdirSync(bin, { recursive: true }); + mkdirSync(legacyDir, { recursive: true }); + const profile = { + agent_id: "legacy", + agent_type: "custom", + name: "synthetic-agent", + created_at: "2026-01-01T00:00:00.000Z", + last_seen_at: "2026-01-01T00:00:00.000Z", + preferences: {}, + }; + writeFileSync(legacyProfile, `${JSON.stringify(profile)}\n`); + const fake = `#!/bin/sh\nprintf '%s\\n' "$0 $*" >> ${JSON.stringify(mutationLog)}\nprintf '{}\\n'\n`; + for (const name of ["conversations", "todos", "mementos"]) { + const path = join(bin, name); + writeFileSync(path, fake); + chmodSync(path, 0o755); + } + + const result = await runWithInputAndEnv( + JSON.stringify({ hook_event_name: "Stop", session_id: "cli-dry-run" }), + { HOME: home, PATH: bin, HASNA_HOOKS_STOP_SYNC_TASK_COMMENT: "1" }, + "run", "stop-sync", "--profile", "legacy", "--dry-run", + ); + expect(result.exitCode).toBe(0); + expect(JSON.parse(result.stdout).continue).toBe(true); + expect(existsSync(mutationLog)).toBe(false); + expect(existsSync(join(home, ".hasna", "hooks", "profiles"))).toBe(false); + expect(readFileSync(legacyProfile, "utf-8")).toBe(`${JSON.stringify(profile)}\n`); + } finally { + rmSync(home, { recursive: true, force: true }); + } + }); + + test("input cap rejects oversized stdin before executing a hook", async () => { + const result = await runWithInputAndEnv("x".repeat(70_000), {}, "run", "stop-sync", "--dry-run"); + expect(result.exitCode).not.toBe(0); + expect(result.stderr).toContain("hook input exceeds 65536 bytes"); + }); + + test("timeout kills a hook's nested descendant tree", async () => { + const home = mkdtempSync(join(tmpdir(), "hooks-cli-run-timeout-")); + const bin = join(home, "bin"); + const sentinel = join(home, "descendant-survived"); + try { + mkdirSync(bin, { recursive: true }); + const fake = `#!/bin/sh\n(/bin/sleep 0.6; printf survived > ${JSON.stringify(sentinel)}) &\nwait\n`; + const conversations = join(bin, "conversations"); + writeFileSync(conversations, fake); + chmodSync(conversations, 0o755); + + const result = await runWithInputAndEnv( + JSON.stringify({ hook_event_name: "SessionStart", session_id: "cli-timeout" }), + { HOME: home, PATH: bin }, + "run", "session-start", "--dry-run", "--timeout-ms", "150", + ); + expect(result.stderr).toContain("timed out"); + expect(result.exitCode).not.toBe(0); + await Bun.sleep(700); + expect(existsSync(sentinel)).toBe(false); + } finally { + rmSync(home, { recursive: true, force: true }); + } }); }); diff --git a/src/cli/index.tsx b/src/cli/index.tsx index b21175a..6002556 100644 --- a/src/cli/index.tsx +++ b/src/cli/index.tsx @@ -8,6 +8,7 @@ import { existsSync, readFileSync } from "fs"; import { join, dirname } from "path"; import { fileURLToPath } from "url"; import { homedir } from "os"; +import { readBoundedStdin, runBoundedProcess } from "../../hooks/bounded-process.js"; const __dirname = dirname(fileURLToPath(import.meta.url)); // Resolve package.json from both source (src/cli/) and built (bin/) locations @@ -22,6 +23,7 @@ import { getHooksByCategory, searchHooks, getHook, + resolveHookNetworkAccess, } from "../lib/registry.js"; import { installHook, @@ -162,8 +164,11 @@ program .command("run") .argument("", "Hook to run") .option("--profile ", "Agent profile ID") + .option("--dry-run", "Run only hooks with native no-write dry-run support", false) + .option("--deny-network", "Further restrict an allow-declared hook to local-only access", false) + .option("--timeout-ms ", "Maximum hook runtime in milliseconds", "10000") .description("Execute a hook (called by AI coding agents)") - .action(async (hook: string, options: { profile?: string }) => { + .action(async (hook: string, options: { profile?: string; dryRun: boolean; denyNetwork: boolean; timeoutMs: string }) => { const meta = getHook(hook); if (!meta) { console.error(JSON.stringify({ error: `Hook '${hook}' not found` })); @@ -178,45 +183,67 @@ program process.exit(1); } + if (options.dryRun && meta.dryRun !== true) { + console.error(JSON.stringify({ error: `Hook '${hook}' does not declare native dry-run support` })); + process.exit(1); + } + + const timeoutMs = Number(options.timeoutMs); + if (!Number.isInteger(timeoutMs) || timeoutMs <= 0) { + console.error(JSON.stringify({ error: "--timeout-ms must be a positive integer" })); + process.exit(1); + } + // Read stdin (agent passes hook context as JSON) - const stdin = await new Response(Bun.stdin.stream()).text(); + let stdin: string; + try { + stdin = readBoundedStdin(); + } catch (error) { + console.error(JSON.stringify({ error: error instanceof Error ? error.message : String(error) })); + process.exit(1); + } - // If profile specified, inject agent data into the hook input + // Dry-run is injected before any mutating profile operation. Invalid input + // is rejected because silently dropping the marker could execute a write. let hookStdin = stdin; - if (options.profile) { - const profile = getProfile(options.profile); - if (profile) { - touchProfile(options.profile); - try { - const input = JSON.parse(stdin); + if (options.profile || options.dryRun) { + let input: Record; + try { + const parsed = JSON.parse(stdin); + if (!parsed || typeof parsed !== "object" || Array.isArray(parsed)) throw new Error("hook input must be a JSON object"); + input = parsed as Record; + } catch { + console.error(JSON.stringify({ error: "Profile and dry-run hook input must be a valid JSON object" })); + process.exit(1); + } + + if (options.dryRun) input.dry_run = true; + if (options.profile) { + const profile = getProfile(options.profile); + if (profile) { input.agent = { agent_id: profile.agent_id, agent_type: profile.agent_type, name: profile.name, preferences: profile.preferences, }; - hookStdin = JSON.stringify(input); - } catch { - // If stdin is not valid JSON, pass through unmodified + if (!options.dryRun) touchProfile(options.profile); } } + hookStdin = JSON.stringify(input); } - // Execute the hook script with bun, passing stdin through - const proc = Bun.spawn(["bun", "run", hookScript], { - stdin: new Response(hookStdin), - stdout: "pipe", - stderr: "pipe", + const result = await runBoundedProcess([process.execPath, "run", hookScript], { + input: hookStdin, + timeoutMs, + network: resolveHookNetworkAccess(meta, options.denyNetwork ? "deny" : undefined), env: process.env, }); - const stdout = await new Response(proc.stdout).text(); - const stderr = await new Response(proc.stderr).text(); - const exitCode = await proc.exited; - - if (stdout) process.stdout.write(stdout); - if (stderr) process.stderr.write(stderr); - process.exit(exitCode); + if (result.stdout) process.stdout.write(result.stdout); + if (result.stderr) process.stderr.write(result.stderr); + if (result.error) process.stderr.write(`[hooks] ${result.error}\n`); + process.exit(result.error ? 1 : (result.exitCode ?? 1)); }); // Install command diff --git a/src/hooks/codewith-native.test.ts b/src/hooks/codewith-native.test.ts index f4859f0..833ef3d 100644 --- a/src/hooks/codewith-native.test.ts +++ b/src/hooks/codewith-native.test.ts @@ -1,5 +1,5 @@ import { describe, test, expect, beforeEach, afterEach } from "bun:test"; -import { chmodSync, mkdirSync, mkdtempSync, rmSync, writeFileSync } from "fs"; +import { chmodSync, existsSync, mkdirSync, mkdtempSync, rmSync, writeFileSync } from "fs"; import { join } from "path"; import { tmpdir } from "os"; @@ -845,4 +845,60 @@ describe("Codewith-native hooks", () => { expect(result.json.continue).toBe(true); expect(result.stderr).toContain("turn-end"); }); + + test("session-start dry-run skips identity mutation and cache writes", async () => { + const bin = join(tmp, "bin"); + const mutationLog = join(tmp, "mutations.log"); + const cache = join(tmp, "cache"); + mkdirSync(bin, { recursive: true }); + const fake = `#!/bin/sh\ncase "$*" in\n "agents register"*|"agents heartbeat"*|"init "*|"heartbeat "*|"register-agent"*) printf '%s\\n' "$0 $*" >> ${JSON.stringify(mutationLog)} ;;\nesac\nprintf '{}\\n'\n`; + for (const name of ["conversations", "todos", "mementos"]) { + const path = join(bin, name); + writeFileSync(path, fake); + chmodSync(path, 0o755); + } + + const result = await runHook("session-start", { + hook_event_name: "SessionStart", + session_id: "sess-dry-run", + cwd: tmp, + dry_run: true, + agent: { name: "synthetic-agent" }, + }, { env: { PATH: bin, HASNA_HOOKS_CACHE_DIR: cache } }); + + expect(result.exitCode).toBe(0); + expect(result.json.continue).toBe(true); + expect(existsSync(mutationLog)).toBe(false); + expect(existsSync(cache)).toBe(false); + }); + + test("stop-sync dry-run skips heartbeats and task comments", async () => { + const bin = join(tmp, "bin"); + const mutationLog = join(tmp, "mutations.log"); + mkdirSync(bin, { recursive: true }); + const fake = `#!/bin/sh\nprintf '%s\\n' "$0 $*" >> ${JSON.stringify(mutationLog)}\nprintf '{}\\n'\n`; + for (const name of ["conversations", "todos", "mementos"]) { + const path = join(bin, name); + writeFileSync(path, fake); + chmodSync(path, 0o755); + } + + const result = await runHook("stop-sync", { + hook_event_name: "Stop", + session_id: "sess-stop-dry-run", + cwd: tmp, + dry_run: true, + task_id: "synthetic-task", + agent: { name: "synthetic-agent" }, + }, { + env: { + PATH: bin, + HASNA_HOOKS_STOP_SYNC_TASK_COMMENT: "1", + }, + }); + + expect(result.exitCode).toBe(0); + expect(result.json.continue).toBe(true); + expect(existsSync(mutationLog)).toBe(false); + }); }); diff --git a/src/index.test.ts b/src/index.test.ts index 817d534..572457e 100644 --- a/src/index.test.ts +++ b/src/index.test.ts @@ -4,12 +4,16 @@ */ import { describe, test, expect } from "bun:test"; +import { chmodSync, existsSync, mkdirSync, mkdtempSync, rmSync, writeFileSync } from "fs"; +import { join } from "path"; +import { tmpdir } from "os"; import { HOOKS, CATEGORIES, getHook, getHooksByCategory, searchHooks, + resolveHookNetworkAccess, installHook, installHooks, getInstalledHooks, @@ -118,4 +122,138 @@ describe("library exports", () => { test("runHook is a function", () => { expect(typeof runHook).toBe("function"); }); + + test("standalone hooks declare the exact audited remote-access set", () => { + expect(HOOKS.filter((hook) => hook.network === "allow").map((hook) => hook.name)).toEqual([ + "packageage", + "phonenotify", + "slacknotify", + "session-start", + "stop-sync", + "announce-start", + "fleet-catchup", + "fleet-blockers-gate", + ]); + expect(getHook("pre-bash")?.network).toBe("deny"); + expect(getHook("worktree-guard")?.network).toBe("deny"); + expect(getHook("gitguard")?.network).toBeUndefined(); + expect(getHook("agentmessages")?.network).toBeUndefined(); + expect(getHook("knowledge-context")?.network).toBeUndefined(); + expect(getHook("agent-rules-version-check")?.network).toBeUndefined(); + for (const shellInterpolatingHook of ["failure-to-task", "announce-stop", "dm-inject"]) { + expect(getHook(shellInterpolatingHook)?.network).toBeUndefined(); + } + for (const detachedProviderHook of ["checktests", "checkfiles", "checkbugs", "checkdocs", "checksecurity"]) { + expect(getHook(detachedProviderHook)?.network).toBeUndefined(); + } + expect(resolveHookNetworkAccess(getHook("session-start")!, "deny")).toBe("deny"); + expect(resolveHookNetworkAccess(getHook("phonenotify")!, "allow")).toBe("allow"); + expect(() => resolveHookNetworkAccess(getHook("pre-bash")!, "allow")).toThrow("cannot be elevated"); + expect(() => resolveHookNetworkAccess(getHook("gitguard")!, "allow")).toThrow("cannot be elevated"); + }); + + test("runHook cannot elevate a local-only guard to network allow", async () => { + let message = ""; + try { + await runHook("pre-bash", { hook_event_name: "PreToolUse" }, { network: "allow" }); + } catch (error) { + message = error instanceof Error ? error.message : String(error); + } + expect(message).toContain("cannot be elevated"); + }); + + test("runHook preserves network access only for a declared remote hook", async () => { + const tmp = mkdtempSync(join(tmpdir(), "hooks-sdk-network-")); + let requests = 0; + const server = Bun.serve({ + hostname: "127.0.0.1", + port: 0, + fetch: () => { + requests += 1; + return new Response("ok"); + }, + }); + try { + mkdirSync(join(tmp, ".claude"), { recursive: true }); + writeFileSync(join(tmp, ".claude", "settings.json"), JSON.stringify({ + phoneNotifyConfig: { + enabled: true, + topic: "synthetic-network", + server: `http://127.0.0.1:${server.port}`, + }, + })); + + const result = await runHook("phonenotify", { + hook_event_name: "Stop", + cwd: tmp, + }, { + env: { HOME: tmp, PATH: process.env.PATH ?? "" }, + }); + + expect(result.exitCode).toBe(0); + expect(result.error).toBeNull(); + expect(result.output.continue).toBe(true); + expect(requests).toBe(1); + + const denied = await runHook("phonenotify", { + hook_event_name: "Stop", + cwd: tmp, + }, { + network: "deny", + env: { HOME: tmp, PATH: process.env.PATH ?? "" }, + }); + expect(denied.exitCode).toBe(0); + expect(denied.output.continue).toBe(true); + expect(requests).toBe(1); + } finally { + server.stop(); + rmSync(tmp, { recursive: true, force: true }); + } + }); + + test("runHook propagates dry-run before stop-sync mutations", async () => { + const tmp = mkdtempSync(join(tmpdir(), "hooks-sdk-dry-run-")); + const bin = join(tmp, "bin"); + const mutationLog = join(tmp, "mutations.log"); + try { + mkdirSync(bin, { recursive: true }); + const fake = `#!/bin/sh\nprintf '%s\\n' "$0 $*" >> ${JSON.stringify(mutationLog)}\nprintf '{}\\n'\n`; + for (const name of ["conversations", "todos", "mementos"]) { + const path = join(bin, name); + writeFileSync(path, fake); + chmodSync(path, 0o755); + } + + const result = await runHook("stop-sync", { + hook_event_name: "Stop", + session_id: "sdk-dry-run", + agent: { agent_id: "synthetic-agent", agent_type: "codewith", name: "synthetic-agent" }, + }, { + dryRun: true, + env: { + PATH: bin, + HOME: tmp, + HASNA_HOOKS_STOP_SYNC_TASK_COMMENT: "1", + }, + }); + + expect(result.exitCode).toBe(0); + expect(result.output.continue).toBe(true); + expect(result.error).toBeNull(); + expect(existsSync(mutationLog)).toBe(false); + } finally { + rmSync(tmp, { recursive: true, force: true }); + } + }); + + test("runHook is routed through the shared input cap", async () => { + const result = await runHook("stop-sync", { + hook_event_name: "Stop", + session_id: "sdk-input-cap", + }, { dryRun: true, maxInputBytes: 1 }); + + expect(result.exitCode).toBe(1); + expect(result.error).toContain("hook input exceeds"); + expect(result.output).toEqual({ raw: "" }); + }); }); diff --git a/src/index.ts b/src/index.ts index feb7cb5..1b1ae18 100644 --- a/src/index.ts +++ b/src/index.ts @@ -16,6 +16,7 @@ export { getHookEvents, getHooksByCategory, searchHooks, + resolveHookNetworkAccess, type HookMeta, type HookEvent, type Category, @@ -60,6 +61,7 @@ export interface HookInput { tool_name?: string; tool_input?: Record; agent?: HookAgentInfo; + dry_run?: boolean; [key: string]: unknown; } @@ -106,21 +108,36 @@ export function removeProjectHook(name: string): boolean { // ── runHook — programmatic hook execution ───────────────────────────────────── import { getHook as _getHook } from "./lib/registry.js"; +import { resolveHookNetworkAccess as _resolveHookNetworkAccess } from "./lib/registry.js"; import { getHookPath as _getHookPath, hookExists as _hookExists } from "./lib/installer.js"; import { join } from "path"; import { existsSync } from "fs"; +import { runBoundedProcess, type HookNetworkAccess } from "../hooks/bounded-process.js"; export interface RunHookOptions { /** Agent profile ID to inject into hook input */ profile?: string; /** Timeout in milliseconds (default: 10000) */ timeout?: number; + /** Propagate a no-write dry-run marker. Unsupported hooks are rejected. */ + dryRun?: boolean; + /** Further restrict an allow-declared hook. A deny-declared hook cannot be elevated. */ + network?: HookNetworkAccess; + /** Source environment; only the runner's strict allowlist is forwarded. */ + env?: NodeJS.ProcessEnv; + /** Additional explicit environment names to forward, excluding unsafe loaders. */ + envAllowlist?: readonly string[]; + maxInputBytes?: number; + maxStdoutBytes?: number; + maxStderrBytes?: number; } export interface RunHookResult { output: HookOutput; stderr: string; exitCode: number; + timedOut: boolean; + error: string | null; } /** @@ -132,11 +149,16 @@ export async function runHook(name: string, input: HookInput, options: RunHookOp const meta = _getHook(name); if (!meta) throw new Error(`Hook '${name}' not found`); + const dryRun = options.dryRun === true || input.dry_run === true; + if (dryRun && meta.dryRun !== true) { + throw new Error(`Hook '${name}' does not declare native dry-run support`); + } + const hookDir = _getHookPath(name); const hookScript = join(hookDir, "src", "hook.ts"); if (!existsSync(hookScript)) throw new Error(`Hook script not found: ${hookScript}`); - let hookInput = { ...input }; + let hookInput: HookInput = { ...input, ...(dryRun ? { dry_run: true } : {}) }; if (options.profile) { const { getProfile } = await import("./lib/profiles.js"); const profile = getProfile(options.profile); @@ -150,27 +172,31 @@ export async function runHook(name: string, input: HookInput, options: RunHookOp } } - const proc = Bun.spawn(["bun", "run", hookScript], { - stdin: new Response(JSON.stringify(hookInput)), - stdout: "pipe", - stderr: "pipe", - env: process.env, + const result = await runBoundedProcess([process.execPath, "run", hookScript], { + input: JSON.stringify(hookInput), + timeoutMs: options.timeout, + network: _resolveHookNetworkAccess(meta, options.network), + env: options.env ?? process.env, + envAllowlist: options.envAllowlist, + maxInputBytes: options.maxInputBytes, + maxStdoutBytes: options.maxStdoutBytes, + maxStderrBytes: options.maxStderrBytes, }); - const [stdoutText, stderrText, exitCode] = await Promise.all([ - new Response(proc.stdout).text(), - new Response(proc.stderr).text(), - proc.exited, - ]); - let output: HookOutput = {}; try { - output = JSON.parse(stdoutText); + output = JSON.parse(result.stdout); } catch { - output = { raw: stdoutText } as HookOutput; + output = { raw: result.stdout } as HookOutput; } - return { output, stderr: stderrText, exitCode }; + return { + output, + stderr: result.stderr, + exitCode: result.error ? 1 : (result.exitCode ?? 1), + timedOut: result.timedOut, + error: result.error, + }; } export { diff --git a/src/lib/profiles.ts b/src/lib/profiles.ts index 2e47716..123c6d6 100644 --- a/src/lib/profiles.ts +++ b/src/lib/profiles.ts @@ -24,31 +24,37 @@ export interface CreateProfileInput { name?: string; } -function resolveProfilesDir(): string { - const newDir = join(homedir(), ".hasna", "hooks", "profiles"); - const oldDir = join(homedir(), ".hooks", "profiles"); +const PROFILES_DIR = join(homedir(), ".hasna", "hooks", "profiles"); +const LEGACY_PROFILES_DIR = join(homedir(), ".hooks", "profiles"); - // Auto-migrate: copy old profiles to new location if needed - if (!existsSync(newDir) && existsSync(oldDir)) { +function migrateProfilesIfNeeded(): void { + // Migration is a mutation and therefore happens only on an explicitly + // mutating profile operation, never merely because the module was imported. + if (!existsSync(PROFILES_DIR) && existsSync(LEGACY_PROFILES_DIR)) { mkdirSync(join(homedir(), ".hasna", "hooks"), { recursive: true }); - cpSync(oldDir, newDir, { recursive: true }); + cpSync(LEGACY_PROFILES_DIR, PROFILES_DIR, { recursive: true }); } - - return newDir; } -const PROFILES_DIR = resolveProfilesDir(); - function ensureProfilesDir(): void { + migrateProfilesIfNeeded(); if (!existsSync(PROFILES_DIR)) { mkdirSync(PROFILES_DIR, { recursive: true }); } } -function profilePath(id: string): string { +function profilePath(id: string, readOnly = false): string { + if (readOnly && !existsSync(PROFILES_DIR) && existsSync(LEGACY_PROFILES_DIR)) { + return join(LEGACY_PROFILES_DIR, `${id}.json`); + } return join(PROFILES_DIR, `${id}.json`); } +function readableProfilesDir(): string { + if (existsSync(PROFILES_DIR)) return PROFILES_DIR; + return LEGACY_PROFILES_DIR; +} + function shortUuid(): string { return crypto.randomUUID().slice(0, 8); } @@ -80,7 +86,7 @@ export function createProfile(input: CreateProfileInput): AgentProfile { } export function getProfile(id: string): AgentProfile | null { - const path = profilePath(id); + const path = profilePath(id, true); try { if (!existsSync(path)) return null; return JSON.parse(readFileSync(path, "utf-8")); @@ -90,15 +96,16 @@ export function getProfile(id: string): AgentProfile | null { } export function listProfiles(): AgentProfile[] { - if (!existsSync(PROFILES_DIR)) return []; + const profilesDir = readableProfilesDir(); + if (!existsSync(profilesDir)) return []; try { - const files = readdirSync(PROFILES_DIR).filter((f) => f.endsWith(".json")); + const files = readdirSync(profilesDir).filter((f) => f.endsWith(".json")); const profiles: AgentProfile[] = []; for (const file of files) { try { - const content = readFileSync(join(PROFILES_DIR, file), "utf-8"); + const content = readFileSync(join(profilesDir, file), "utf-8"); profiles.push(JSON.parse(content)); } catch { // Skip corrupt files @@ -117,6 +124,7 @@ export function updateProfile( id: string, data: Partial> ): AgentProfile | null { + migrateProfilesIfNeeded(); const profile = getProfile(id); if (!profile) return null; @@ -128,6 +136,7 @@ export function updateProfile( } export function deleteProfile(id: string): boolean { + migrateProfilesIfNeeded(); const path = profilePath(id); if (!existsSync(path)) return false; @@ -140,6 +149,7 @@ export function deleteProfile(id: string): boolean { } export function touchProfile(id: string): void { + migrateProfilesIfNeeded(); const profile = getProfile(id); if (!profile) return; diff --git a/src/lib/registry.ts b/src/lib/registry.ts index 5a1067a..28fc2f3 100644 --- a/src/lib/registry.ts +++ b/src/lib/registry.ts @@ -33,6 +33,10 @@ export interface HookMeta { events?: HookEvent[]; matcher: string; tags: string[]; + /** Standalone network policy. Omitted and detached/orphan-style hooks fail closed with network denied. */ + network?: "deny" | "allow"; + /** The hook guarantees that `dry_run: true` performs no mutations. */ + dryRun?: boolean; } export const CATEGORIES = [ @@ -91,6 +95,7 @@ export const HOOKS: HookMeta[] = [ event: "PreToolUse", matcher: "^(Bash|Write|Edit|MultiEdit|NotebookEdit|apply_patch|ApplyPatch|functions\\.apply_patch|mcp__.*)$", tags: ["git", "worktree", "repos", "multi-agent", "safety", "dangerous-ops"], + network: "deny", }, // Code Quality @@ -175,6 +180,7 @@ export const HOOKS: HookMeta[] = [ event: "PreToolUse", matcher: "Bash", tags: ["npm", "packages", "typosquatting", "supply-chain"], + network: "allow", }, { name: "pre-bash", @@ -185,6 +191,7 @@ export const HOOKS: HookMeta[] = [ event: "PreToolUse", matcher: "Bash", tags: ["codewith", "bash", "secrets", "gitleaks", "risky-ops"], + network: "deny", }, // Notifications @@ -197,6 +204,7 @@ export const HOOKS: HookMeta[] = [ event: "Stop", matcher: "", tags: ["notification", "phone", "push", "ntfy"], + network: "allow", }, { name: "agentmessages", @@ -349,6 +357,7 @@ export const HOOKS: HookMeta[] = [ event: "Stop", matcher: "", tags: ["notification", "slack", "webhook", "team"], + network: "allow", }, { name: "soundnotify", @@ -435,6 +444,8 @@ export const HOOKS: HookMeta[] = [ event: "SessionStart", matcher: "", tags: ["codewith", "session", "context", "conversations", "heartbeat"], + network: "allow", + dryRun: true, }, { name: "stop-sync", @@ -445,6 +456,8 @@ export const HOOKS: HookMeta[] = [ event: "Stop", matcher: "", tags: ["codewith", "stop", "heartbeat", "todos", "turn-end"], + network: "allow", + dryRun: true, }, // Code Quality (new) @@ -513,6 +526,7 @@ export const HOOKS: HookMeta[] = [ event: "SessionStart", matcher: "", tags: ["announcement", "start", "register", "messages", "agent-teams"], + network: "allow", }, { name: "announce-stop", @@ -546,6 +560,7 @@ export const HOOKS: HookMeta[] = [ event: "SessionStart", matcher: "", tags: ["fleet", "catchup", "blockers", "announcements", "context", "agent-teams"], + network: "allow", }, { name: "agent-rules-version-check", @@ -568,6 +583,7 @@ export const HOOKS: HookMeta[] = [ event: "PreToolUse", matcher: "", tags: ["fleet", "freeze", "blockers", "gate", "safety", "agent-teams"], + network: "allow", }, ]; @@ -593,3 +609,15 @@ export function searchHooks(query: string): HookMeta[] { export function getHook(name: string): HookMeta | undefined { return HOOKS.find((h) => h.name === name); } + +export function resolveHookNetworkAccess( + hook: HookMeta, + requested?: "deny" | "allow", +): "deny" | "allow" { + const declared = hook.network ?? "deny"; + if (requested === "allow" && declared !== "allow") { + throw new Error(`Hook '${hook.name}' declares local-only network access and cannot be elevated`); + } + if (requested === "deny") return "deny"; + return declared; +} From 5adc52d2746006f14f657189d27a3c17c85444e1 Mon Sep 17 00:00:00 2001 From: andreihasna Date: Sun, 19 Jul 2026 11:59:34 +0300 Subject: [PATCH 2/8] fix: bound MCP hook execution --- hooks/bounded-process.js | 1 - hooks/codewith-native-common.test.ts | 22 ++ src/cli/index.tsx | 2 + src/index.test.ts | 9 + src/index.ts | 4 +- src/lib/registry.test.ts | 8 + src/lib/registry.ts | 19 ++ src/mcp/execution.test.ts | 321 +++++++++++++++++++++++++++ src/mcp/server.ts | 237 +++++++++++++++----- 9 files changed, 560 insertions(+), 63 deletions(-) create mode 100644 src/mcp/execution.test.ts diff --git a/hooks/bounded-process.js b/hooks/bounded-process.js index 897487b..95cef52 100644 --- a/hooks/bounded-process.js +++ b/hooks/bounded-process.js @@ -48,7 +48,6 @@ const SAFE_ENV_NAMES = Object.freeze([ "XDG_CACHE_HOME", "XDG_DATA_HOME", "CLAUDE_CODE_TASK_LIST_ID", - "CLAUDE_ENV_FILE", "CLAUDE_PROJECT_DIR", "CHECK_TASKS_DISABLED", "CHECK_TASKS_KEYWORDS", diff --git a/hooks/codewith-native-common.test.ts b/hooks/codewith-native-common.test.ts index d83246d..80a2aaf 100644 --- a/hooks/codewith-native-common.test.ts +++ b/hooks/codewith-native-common.test.ts @@ -176,6 +176,28 @@ describe("codewith native common helpers", () => { } }); + test("runBoundedProcess does not forward CLAUDE_ENV_FILE without an explicit capability", async () => { + const env = { + PATH: process.env.PATH ?? "", + CLAUDE_ENV_FILE: "/tmp/synthetic-claude-env-file", + }; + const script = 'process.stdout.write(process.env.CLAUDE_ENV_FILE ?? "unset")'; + + const ordinary = await runBoundedProcess( + [process.execPath, "-e", script], + { env, network: "allow" }, + ); + expect(ordinary.exitCode).toBe(0); + expect(ordinary.stdout).toBe("unset"); + + const capable = await runBoundedProcess( + [process.execPath, "-e", script], + { env, envAllowlist: ["CLAUDE_ENV_FILE"], network: "allow" }, + ); + expect(capable.exitCode).toBe(0); + expect(capable.stdout).toBe("/tmp/synthetic-claude-env-file"); + }); + test("runBoundedProcess timeout includes queue wait", async () => { const tmp = mkdtempSync(join(tmpdir(), "hooks-command-queue-")); const sentinel = join(tmp, "queued-command-executed"); diff --git a/src/cli/index.tsx b/src/cli/index.tsx index 6002556..917b06f 100644 --- a/src/cli/index.tsx +++ b/src/cli/index.tsx @@ -24,6 +24,7 @@ import { searchHooks, getHook, resolveHookNetworkAccess, + resolveHookEnvironmentAllowlist, } from "../lib/registry.js"; import { installHook, @@ -238,6 +239,7 @@ program timeoutMs, network: resolveHookNetworkAccess(meta, options.denyNetwork ? "deny" : undefined), env: process.env, + envAllowlist: resolveHookEnvironmentAllowlist(meta), }); if (result.stdout) process.stdout.write(result.stdout); diff --git a/src/index.test.ts b/src/index.test.ts index 572457e..fba15c4 100644 --- a/src/index.test.ts +++ b/src/index.test.ts @@ -14,6 +14,7 @@ import { getHooksByCategory, searchHooks, resolveHookNetworkAccess, + resolveHookEnvironmentAllowlist, installHook, installHooks, getInstalledHooks, @@ -162,6 +163,14 @@ describe("library exports", () => { expect(message).toContain("cannot be elevated"); }); + test("runHook environment capabilities cannot be elevated across hooks", () => { + expect(resolveHookEnvironmentAllowlist(getHook("agentmessages")!)).toEqual(["CLAUDE_ENV_FILE"]); + expect(() => resolveHookEnvironmentAllowlist( + getHook("gitguard")!, + ["CLAUDE_ENV_FILE"], + )).toThrow("does not declare environment capability"); + }); + test("runHook preserves network access only for a declared remote hook", async () => { const tmp = mkdtempSync(join(tmpdir(), "hooks-sdk-network-")); let requests = 0; diff --git a/src/index.ts b/src/index.ts index 1b1ae18..7eb64c3 100644 --- a/src/index.ts +++ b/src/index.ts @@ -17,6 +17,7 @@ export { getHooksByCategory, searchHooks, resolveHookNetworkAccess, + resolveHookEnvironmentAllowlist, type HookMeta, type HookEvent, type Category, @@ -109,6 +110,7 @@ export function removeProjectHook(name: string): boolean { import { getHook as _getHook } from "./lib/registry.js"; import { resolveHookNetworkAccess as _resolveHookNetworkAccess } from "./lib/registry.js"; +import { resolveHookEnvironmentAllowlist as _resolveHookEnvironmentAllowlist } from "./lib/registry.js"; import { getHookPath as _getHookPath, hookExists as _hookExists } from "./lib/installer.js"; import { join } from "path"; import { existsSync } from "fs"; @@ -177,7 +179,7 @@ export async function runHook(name: string, input: HookInput, options: RunHookOp timeoutMs: options.timeout, network: _resolveHookNetworkAccess(meta, options.network), env: options.env ?? process.env, - envAllowlist: options.envAllowlist, + envAllowlist: _resolveHookEnvironmentAllowlist(meta, options.envAllowlist), maxInputBytes: options.maxInputBytes, maxStdoutBytes: options.maxStdoutBytes, maxStderrBytes: options.maxStderrBytes, diff --git a/src/lib/registry.test.ts b/src/lib/registry.test.ts index 47a9eeb..fce4301 100644 --- a/src/lib/registry.test.ts +++ b/src/lib/registry.test.ts @@ -60,6 +60,14 @@ describe("registry", () => { expect(CATEGORIES as readonly string[]).toContain(hook.category); } }); + + test("scopes CLAUDE_ENV_FILE to the source-proven agentmessages hook", () => { + expect(getHook("agentmessages")?.envAllowlist).toEqual(["CLAUDE_ENV_FILE"]); + expect( + HOOKS.filter((hook) => hook.name !== "agentmessages") + .some((hook) => hook.envAllowlist?.includes("CLAUDE_ENV_FILE")), + ).toBe(false); + }); }); describe("CATEGORIES", () => { diff --git a/src/lib/registry.ts b/src/lib/registry.ts index 28fc2f3..34c7b04 100644 --- a/src/lib/registry.ts +++ b/src/lib/registry.ts @@ -37,6 +37,8 @@ export interface HookMeta { network?: "deny" | "allow"; /** The hook guarantees that `dry_run: true` performs no mutations. */ dryRun?: boolean; + /** Extra non-sensitive environment names this hook is explicitly allowed to receive. */ + envAllowlist?: readonly string[]; } export const CATEGORIES = [ @@ -215,6 +217,7 @@ export const HOOKS: HookMeta[] = [ event: "Stop", matcher: "", tags: ["messaging", "agents", "inter-agent"], + envAllowlist: ["CLAUDE_ENV_FILE"], }, // Context Management @@ -621,3 +624,19 @@ export function resolveHookNetworkAccess( if (requested === "deny") return "deny"; return declared; } + +const HOOK_SCOPED_ENV_CAPABILITIES = new Set(["CLAUDE_ENV_FILE"]); + +export function resolveHookEnvironmentAllowlist( + hook: HookMeta, + requested: readonly string[] = [], +): readonly string[] { + const declared = new Set(hook.envAllowlist ?? []); + for (const name of requested) { + if (HOOK_SCOPED_ENV_CAPABILITIES.has(name) && !declared.has(name)) { + throw new Error(`Hook '${hook.name}' does not declare environment capability '${name}'`); + } + declared.add(name); + } + return [...declared]; +} diff --git a/src/mcp/execution.test.ts b/src/mcp/execution.test.ts new file mode 100644 index 0000000..5cccd48 --- /dev/null +++ b/src/mcp/execution.test.ts @@ -0,0 +1,321 @@ +import { afterEach, describe, expect, test } from "bun:test"; +import { chmodSync, existsSync, mkdirSync, mkdtempSync, rmSync, writeFileSync } from "fs"; +import { tmpdir } from "os"; +import { join } from "path"; +import { Client } from "@modelcontextprotocol/sdk/client"; +import { InMemoryTransport } from "@modelcontextprotocol/sdk/inMemory.js"; +import type { HookMeta } from "../lib/registry.js"; +import { createHooksServer } from "./server.js"; + +type ExecutionOverrides = { + getHook: (name: string) => HookMeta | undefined; + getHookPath: (name: string) => string; + getRegisteredHooks: () => string[]; + env?: NodeJS.ProcessEnv; + containmentExecutable?: string; + maxInputBytes?: number; + maxStdoutBytes?: number; + maxStderrBytes?: number; +}; + +const roots: string[] = []; + +function meta(name: string, overrides: Partial = {}): HookMeta { + return { + name, + displayName: name, + description: `test hook ${name}`, + version: "0.0.0", + category: "Security", + event: "PreToolUse", + matcher: "Bash", + tags: ["test"], + ...overrides, + }; +} + +function fixtureHook(root: string, name: string, source: string): string { + const directory = join(root, name); + mkdirSync(join(directory, "src"), { recursive: true }); + const script = join(directory, "src", "hook.ts"); + writeFileSync(script, source); + chmodSync(script, 0o755); + return directory; +} + +async function withServer( + hooks: HookMeta[], + paths: Map, + overrides: Partial = {}, +) { + const hookMap = new Map(hooks.map((hook) => [hook.name, hook])); + const execution: ExecutionOverrides = { + getHook: (name) => hookMap.get(name), + getHookPath: (name) => paths.get(name) ?? join("/missing", name), + getRegisteredHooks: () => hooks.map((hook) => hook.name), + ...overrides, + }; + const server = createHooksServer({ execution } as any); + const client = new Client({ name: "bounded-mcp-test", version: "1.0.0" }); + const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair(); + await Promise.all([client.connect(clientTransport), server.connect(serverTransport)]); + return { client, server }; +} + +function parse(result: any): any { + const text = result.content?.find((entry: any) => entry.type === "text")?.text ?? ""; + try { + return JSON.parse(text); + } catch { + return { raw: text, isError: result.isError === true }; + } +} + +afterEach(() => { + while (roots.length > 0) { + rmSync(roots.pop()!, { recursive: true, force: true }); + } +}); + +describe("bounded MCP hook execution", () => { + test("hooks_run enforces input and output caps before returning", async () => { + const root = mkdtempSync(join(tmpdir(), "hooks-mcp-caps-")); + roots.push(root); + const sentinel = join(root, "executed"); + const paths = new Map([ + ["caps", fixtureHook(root, "caps", ` + const input = await Bun.stdin.text(); + await Bun.write(${JSON.stringify(sentinel)}, input); + process.stdout.write(JSON.stringify({ payload: "x".repeat(256) })); + process.stderr.write("e".repeat(256)); + `)], + ]); + const { client } = await withServer([meta("caps", { network: "allow" })], paths, { + maxInputBytes: 64, + maxStdoutBytes: 80, + maxStderrBytes: 32, + }); + try { + const oversizedInput = parse(await client.callTool({ + name: "hooks_run", + arguments: { name: "caps", input: { payload: "i".repeat(128) } }, + })); + expect(oversizedInput.error).toContain("input exceeds 64 bytes"); + expect(existsSync(sentinel)).toBe(false); + + const oversizedOutput = parse(await client.callTool({ + name: "hooks_run", + arguments: { name: "caps", input: {} }, + })); + expect(oversizedOutput.error).toMatch(/stdout exceeds 80 bytes|stderr exceeds 32 bytes/); + expect(Buffer.byteLength(oversizedOutput.stderr ?? "")).toBeLessThanOrEqual(32); + } finally { + await client.close(); + } + }); + + test("hooks_run timeout kills the hook descendant process tree", async () => { + const root = mkdtempSync(join(tmpdir(), "hooks-mcp-tree-")); + roots.push(root); + const sentinel = join(root, "descendant-survived"); + const paths = new Map([ + ["tree", fixtureHook(root, "tree", ` + const { spawn } = await import("node:child_process"); + spawn("/bin/sh", ["-c", ${JSON.stringify(`sleep 0.5; printf survived > ${sentinel}`)}]); + await new Promise(() => {}); + `)], + ]); + const { client } = await withServer([meta("tree", { network: "allow" })], paths); + try { + const data = parse(await client.callTool({ + name: "hooks_run", + arguments: { name: "tree", input: {}, timeout_ms: 100 }, + })); + expect(data.timedOut).toBe(true); + expect(data.error).toContain("timed out"); + await Bun.sleep(650); + expect(existsSync(sentinel)).toBe(false); + } finally { + await client.close(); + } + }); + + test("hooks_run applies declared deny and allow network policies", async () => { + const root = mkdtempSync(join(tmpdir(), "hooks-mcp-network-")); + roots.push(root); + const server = Bun.serve({ port: 0, fetch: () => new Response("reachable") }); + const source = ` + const input = JSON.parse(await Bun.stdin.text()); + try { + const response = await fetch(input.url); + process.stdout.write(JSON.stringify({ value: await response.text() })); + } catch { process.exit(23); } + `; + const paths = new Map([ + ["local", fixtureHook(root, "local", source)], + ["remote", fixtureHook(root, "remote", source)], + ]); + const { client } = await withServer([ + meta("local"), + meta("remote", { network: "allow" }), + ], paths); + try { + const url = `http://127.0.0.1:${server.port}`; + const denied = parse(await client.callTool({ + name: "hooks_run", + arguments: { name: "local", input: { url } }, + })); + expect(denied.exitCode).not.toBe(0); + expect(denied.output).not.toEqual({ value: "reachable" }); + + const allowed = parse(await client.callTool({ + name: "hooks_run", + arguments: { name: "remote", input: { url } }, + })); + expect(allowed.exitCode).toBe(0); + expect(allowed.output).toEqual({ value: "reachable" }); + } finally { + server.stop(true); + await client.close(); + } + }); + + test("hooks_run fails closed before execution when containment is missing", async () => { + const root = mkdtempSync(join(tmpdir(), "hooks-mcp-containment-")); + roots.push(root); + const sentinel = join(root, "executed"); + const paths = new Map([ + ["local", fixtureHook(root, "local", `await Bun.write(${JSON.stringify(sentinel)}, "executed")`)], + ]); + const { client } = await withServer([meta("local")], paths, { + containmentExecutable: join(root, "missing-bwrap"), + }); + try { + const data = parse(await client.callTool({ + name: "hooks_run", + arguments: { name: "local", input: {} }, + })); + expect(data.error).toContain("requires bubblewrap"); + expect(existsSync(sentinel)).toBe(false); + } finally { + await client.close(); + } + }); + + test("MCP schemas allow further network restriction but reject elevation", async () => { + const root = mkdtempSync(join(tmpdir(), "hooks-mcp-elevation-")); + roots.push(root); + const paths = new Map([ + ["local", fixtureHook(root, "local", 'console.log(JSON.stringify({ decision: "approve" }))')], + ]); + const { client } = await withServer([meta("local")], paths); + try { + const elevated = await client.callTool({ + name: "hooks_run", + arguments: { name: "local", input: {}, network: "allow" }, + }); + expect(elevated.isError).toBe(true); + + const restricted = parse(await client.callTool({ + name: "hooks_run", + arguments: { name: "local", input: {}, network: "deny" }, + })); + expect(restricted.output).toEqual({ decision: "approve" }); + } finally { + await client.close(); + } + }); + + test("hooks_batch_run bounds batch size and shares the process queue deadline", async () => { + const root = mkdtempSync(join(tmpdir(), "hooks-mcp-batch-")); + roots.push(root); + const paths = new Map([ + ["slow", fixtureHook(root, "slow", 'await Bun.sleep(300); console.log("{}")')], + ]); + const hook = meta("slow", { network: "allow" }); + const { client } = await withServer([hook], paths); + try { + const tooMany = await client.callTool({ + name: "hooks_batch_run", + arguments: { hooks: Array.from({ length: 33 }, () => ({ name: "slow", input: {} })) }, + }); + expect(tooMany.isError).toBe(true); + + const data = parse(await client.callTool({ + name: "hooks_batch_run", + arguments: { + hooks: Array.from({ length: 5 }, () => ({ name: "slow", input: {} })), + timeout_ms: 100, + }, + })); + expect(data.count).toBe(5); + expect(data.results.some((result: any) => result.error?.includes("waiting for a process slot"))).toBe(true); + } finally { + await client.close(); + } + }); + + test("hooks_preview skips mutation-unsafe hooks and injects dry_run into capable hooks", async () => { + const root = mkdtempSync(join(tmpdir(), "hooks-mcp-preview-")); + roots.push(root); + const sentinel = join(root, "preview-mutated"); + const source = ` + const input = JSON.parse(await Bun.stdin.text()); + if (input.dry_run !== true) await Bun.write(${JSON.stringify(sentinel)}, "mutated"); + console.log(JSON.stringify({ decision: "approve", dry_run: input.dry_run === true })); + `; + const paths = new Map([ + ["unsafe", fixtureHook(root, "unsafe", source)], + ["safe", fixtureHook(root, "safe", source)], + ]); + const { client } = await withServer([ + meta("unsafe", { network: "allow" }), + meta("safe", { network: "allow", dryRun: true }), + ], paths); + try { + const data = parse(await client.callTool({ + name: "hooks_preview", + arguments: { tool_name: "Bash", tool_input: { command: "echo safe" } }, + })); + expect(data.results.find((result: any) => result.name === "unsafe")).toMatchObject({ + decision: "approve", + skipped: true, + }); + expect(data.results.find((result: any) => result.name === "safe").raw.dry_run).toBe(true); + expect(existsSync(sentinel)).toBe(false); + } finally { + await client.close(); + } + }); + + test("MCP execution exposes CLAUDE_ENV_FILE only to its declared hook capability", async () => { + const root = mkdtempSync(join(tmpdir(), "hooks-mcp-env-")); + roots.push(root); + const envFile = join(root, "claude-env"); + const source = 'console.log(JSON.stringify({ value: process.env.CLAUDE_ENV_FILE ?? "unset" }))'; + const paths = new Map([ + ["agentmessages", fixtureHook(root, "agentmessages", source)], + ["ordinary", fixtureHook(root, "ordinary", source)], + ]); + const { client } = await withServer([ + meta("agentmessages", { network: "allow", envAllowlist: ["CLAUDE_ENV_FILE"] }), + meta("ordinary", { network: "allow" }), + ], paths, { + env: { PATH: process.env.PATH ?? "", CLAUDE_ENV_FILE: envFile }, + }); + try { + const capable = parse(await client.callTool({ + name: "hooks_run", + arguments: { name: "agentmessages", input: {} }, + })); + const ordinary = parse(await client.callTool({ + name: "hooks_run", + arguments: { name: "ordinary", input: {} }, + })); + expect(capable.output.value).toBe(envFile); + expect(ordinary.output.value).toBe("unset"); + } finally { + await client.close(); + } + }); +}); diff --git a/src/mcp/server.ts b/src/mcp/server.ts index dc11b29..58f7205 100644 --- a/src/mcp/server.ts +++ b/src/mcp/server.ts @@ -30,6 +30,9 @@ import { getHooksByCategory, searchHooks, getHook, + resolveHookEnvironmentAllowlist, + resolveHookNetworkAccess, + type HookMeta, type Category, } from "../lib/registry.js"; import { @@ -56,9 +59,91 @@ import { storagePush, storageSync, } from "../storage.js"; +import { + runBoundedProcess, + type BoundedProcessResult, + type HookNetworkAccess, +} from "../../hooks/bounded-process.js"; export const MCP_PORT = 39427; +export interface HooksExecutionOverrides { + getHook?: (name: string) => HookMeta | undefined; + getHookPath?: (name: string) => string; + getRegisteredHooks?: (scope: Scope) => string[]; + env?: NodeJS.ProcessEnv; + containmentExecutable?: string; + maxInputBytes?: number; + maxStdoutBytes?: number; + maxStderrBytes?: number; +} + +export interface HooksServerOptions { + execution?: HooksExecutionOverrides; +} + +interface ExecuteMcpHookOptions { + cwd?: string; + dryRun?: boolean; + timeoutMs?: number; + requestedNetwork?: HookNetworkAccess; + env?: NodeJS.ProcessEnv; + envAllowlist?: readonly string[]; + containmentExecutable?: string; + maxInputBytes?: number; + maxStdoutBytes?: number; + maxStderrBytes?: number; +} + +function failedExecution(error: string): BoundedProcessResult { + return { + exitCode: null, + signal: null, + stdout: "", + stderr: "", + timedOut: false, + error, + }; +} + +export async function executeMcpHook( + meta: HookMeta, + hookScript: string, + input: Record, + options: ExecuteMcpHookOptions = {}, +): Promise { + let network: HookNetworkAccess; + let envAllowlist: readonly string[]; + try { + network = resolveHookNetworkAccess(meta, options.requestedNetwork); + envAllowlist = resolveHookEnvironmentAllowlist(meta, options.envAllowlist); + } catch (error) { + return failedExecution(error instanceof Error ? error.message : String(error)); + } + + const hookInput = options.dryRun ? { ...input, dry_run: true } : input; + return runBoundedProcess([process.execPath, "run", hookScript], { + cwd: options.cwd, + input: JSON.stringify(hookInput), + timeoutMs: options.timeoutMs, + network, + env: options.env ?? process.env, + envAllowlist, + containmentExecutable: options.containmentExecutable, + maxInputBytes: options.maxInputBytes, + maxStdoutBytes: options.maxStdoutBytes, + maxStderrBytes: options.maxStderrBytes, + }); +} + +function parseHookOutput(stdout: string): any { + try { + return JSON.parse(stdout); + } catch { + return stdout ? { raw: stdout } : {}; + } +} + function formatInstallResults(results: InstallResult[], extra?: Record) { const installed = results.filter((r) => r.success).map((r) => r.hook); const failed = results.filter((r) => !r.success).map((r) => ({ hook: r.hook, error: r.error })); @@ -74,12 +159,30 @@ function formatInstallResults(results: InstallResult[], extra?: Record(); -export function createHooksServer(): McpServer { +export function createHooksServer(options: HooksServerOptions = {}): McpServer { const server = new McpServer({ name: "@hasna/hooks", version: pkg.version, }); + const execution = options.execution ?? {}; + const executionGetHook = execution.getHook ?? getHook; + const executionGetHookPath = execution.getHookPath ?? getHookPath; + const executionGetRegisteredHooks = execution.getRegisteredHooks ?? getRegisteredHooks; + const execute = ( + meta: HookMeta, + hookScript: string, + input: Record, + perCall: Pick = {}, + ) => executeMcpHook(meta, hookScript, input, { + ...perCall, + env: execution.env, + containmentExecutable: execution.containmentExecutable, + maxInputBytes: execution.maxInputBytes, + maxStdoutBytes: execution.maxStdoutBytes, + maxStderrBytes: execution.maxStderrBytes, + }); + // --- Tools --- server.tool( @@ -337,20 +440,27 @@ export function createHooksServer(): McpServer { name: z.string().describe("Hook name (e.g. 'gitguard', 'checkpoint')"), input: z.record(z.string(), z.unknown()).default(() => ({})).describe("Hook input as JSON object (HookInput)"), profile: z.string().optional().describe("Agent profile ID to inject into hook input"), - timeout_ms: z.number().default(10000).describe("Timeout in milliseconds (default: 10000)"), + timeout_ms: z.number().int().positive().max(86_400_000).default(10000).describe("Timeout in milliseconds (default: 10000)"), + dry_run: z.boolean().default(false).describe("Require native no-write dry-run support"), + network: z.literal("deny").optional().describe("Further restrict an allow-declared hook to local-only access"), }, - async ({ name, input, profile, timeout_ms }) => { - const meta = getHook(name); + async ({ name, input, profile, timeout_ms, dry_run, network }) => { + const meta = executionGetHook(name); if (!meta) { return { content: [{ type: "text", text: JSON.stringify({ error: `Hook '${name}' not found` }) }] }; } - const hookDir = getHookPath(name); + const hookDir = executionGetHookPath(name); const hookScript = join(hookDir, "src", "hook.ts"); if (!existsSync(hookScript)) { return { content: [{ type: "text", text: JSON.stringify({ error: `Hook script not found: ${hookScript}` }) }] }; } + const wantsDryRun = dry_run || input.dry_run === true; + if (wantsDryRun && meta.dryRun !== true) { + return { content: [{ type: "text", text: JSON.stringify({ error: `Hook '${name}' does not declare native dry-run support` }) }] }; + } + let hookInput = { ...input }; if (profile) { const p = getProfile(profile); @@ -364,26 +474,12 @@ export function createHooksServer(): McpServer { } } - const proc = Bun.spawn(["bun", "run", hookScript], { - stdin: new Response(JSON.stringify(hookInput)), - stdout: "pipe", - stderr: "pipe", - env: process.env, + const result = await execute(meta, hookScript, hookInput, { + dryRun: wantsDryRun, + timeoutMs: timeout_ms, + requestedNetwork: network, }); - - const timeoutPromise = new Promise((resolve) => setTimeout(() => resolve(null), timeout_ms)); - - const result = await Promise.race([ - Promise.all([ - new Response(proc.stdout).text(), - new Response(proc.stderr).text(), - proc.exited, - ]).then(([stdout, stderr, exitCode]) => ({ stdout, stderr, exitCode, timedOut: false })), - timeoutPromise.then(() => { proc.kill(); return { stdout: "", stderr: "", exitCode: -1, timedOut: true }; }), - ]); - - let output: unknown = {}; - try { output = JSON.parse(result.stdout); } catch { output = result.stdout ? { raw: result.stdout } : {}; } + const output = parseHookOutput(result.stdout); return { content: [{ @@ -393,6 +489,7 @@ export function createHooksServer(): McpServer { output, stderr: result.stderr || undefined, exitCode: result.exitCode, + ...(result.error ? { error: result.error } : {}), ...(result.timedOut ? { timedOut: true, timeout_ms } : {}), }), }], @@ -474,17 +571,18 @@ export function createHooksServer(): McpServer { server.tool( "hooks_preview", - "Simulate which installed PreToolUse hooks would fire for a given tool call and what decision each returns. Use this to understand your hook environment before taking an action.", + "Safely simulate installed PreToolUse hooks that declare native no-write dry-run support. Mutation-unsafe hooks are reported as skipped.", { tool_name: z.string().describe("Tool name to simulate (e.g. 'Bash', 'Write', 'Edit')"), tool_input: z.record(z.string(), z.unknown()).default(() => ({})).describe("Tool input to pass to matching hooks"), scope: z.enum(["global", "project"]).default("global").describe("Scope to check"), - timeout_ms: z.number().default(5000).describe("Per-hook timeout in milliseconds"), + timeout_ms: z.number().int().positive().max(86_400_000).default(5000).describe("Per-hook timeout in milliseconds"), + network: z.literal("deny").optional().describe("Further restrict allow-declared hooks to local-only access"), }, - async ({ tool_name, tool_input, scope, timeout_ms }) => { - const registered = getRegisteredHooks(scope); + async ({ tool_name, tool_input, scope, timeout_ms, network }) => { + const registered = executionGetRegisteredHooks(scope); const matchingHooks = registered.filter((name) => { - const meta = getHook(name); + const meta = executionGetHook(name); if (!meta || meta.event !== "PreToolUse") return false; if (!meta.matcher) return true; try { return new RegExp(meta.matcher).test(tool_name); } catch { return false; } @@ -496,24 +594,36 @@ export function createHooksServer(): McpServer { const input = { tool_name, tool_input }; const results = await Promise.all(matchingHooks.map(async (name) => { - const hookDir = getHookPath(name); + const meta = executionGetHook(name)!; + if (meta.dryRun !== true) { + return { + name, + decision: "approve" as const, + skipped: true, + error: `Hook '${name}' does not declare native dry-run support`, + }; + } + + const hookDir = executionGetHookPath(name); const hookScript = join(hookDir, "src", "hook.ts"); if (!existsSync(hookScript)) return { name, decision: "approve", error: "script not found" }; - const proc = Bun.spawn(["bun", "run", hookScript], { - stdin: new Response(JSON.stringify(input)), - stdout: "pipe", stderr: "pipe", env: process.env, + const result = await execute(meta, hookScript, input, { + dryRun: true, + timeoutMs: timeout_ms, + requestedNetwork: network, }); - const timeout = new Promise((r) => setTimeout(() => r(null), timeout_ms)); - const res = await Promise.race([ - Promise.all([new Response(proc.stdout).text(), proc.exited]) - .then(([stdout]) => ({ stdout, timedOut: false })), - timeout.then(() => { proc.kill(); return { stdout: "", timedOut: true }; }), - ]); - if (res.timedOut) return { name, decision: "approve", timedOut: true }; - let output: any = {}; - try { output = JSON.parse(res.stdout); } catch {} + if (result.error) { + return { + name, + decision: "approve" as const, + ...(result.timedOut ? { timedOut: true } : {}), + error: result.error, + exitCode: result.exitCode, + }; + } + const output = parseHookOutput(result.stdout); return { name, decision: output.decision ?? "approve", reason: output.reason, raw: output }; })); @@ -562,35 +672,40 @@ export function createHooksServer(): McpServer { server.tool( "hooks_batch_run", - "Run multiple hooks in parallel in a single call. Returns all results at once — more efficient than N separate hooks_run calls.", + "Run up to 32 hooks through the shared bounded process queue and return all results in one call.", { hooks: z.array(z.object({ name: z.string().describe("Hook name"), input: z.record(z.string(), z.unknown()).default(() => ({})).describe("Hook input JSON"), - })).describe("List of hooks to run with their inputs"), - timeout_ms: z.number().default(10000).describe("Per-hook timeout in milliseconds"), + dry_run: z.boolean().default(false).describe("Require native no-write dry-run support"), + network: z.literal("deny").optional().describe("Further restrict an allow-declared hook"), + })).max(32).describe("List of at most 32 hooks to run with their inputs"), + timeout_ms: z.number().int().positive().max(86_400_000).default(10000).describe("Per-hook timeout in milliseconds"), }, async ({ hooks, timeout_ms }) => { - const results = await Promise.all(hooks.map(async ({ name, input }) => { - const meta = getHook(name); + const results = await Promise.all(hooks.map(async ({ name, input, dry_run, network }) => { + const meta = executionGetHook(name); if (!meta) return { name, error: `Hook '${name}' not found` }; - const hookScript = join(getHookPath(name), "src", "hook.ts"); + const wantsDryRun = dry_run || input.dry_run === true; + if (wantsDryRun && meta.dryRun !== true) { + return { name, error: `Hook '${name}' does not declare native dry-run support` }; + } + const hookScript = join(executionGetHookPath(name), "src", "hook.ts"); if (!existsSync(hookScript)) return { name, error: "script not found" }; - const proc = Bun.spawn(["bun", "run", hookScript], { - stdin: new Response(JSON.stringify(input)), - stdout: "pipe", stderr: "pipe", env: process.env, + const result = await execute(meta, hookScript, input, { + dryRun: wantsDryRun, + timeoutMs: timeout_ms, + requestedNetwork: network, }); - const timeout = new Promise((r) => setTimeout(() => r(null), timeout_ms)); - const res = await Promise.race([ - Promise.all([new Response(proc.stdout).text(), new Response(proc.stderr).text(), proc.exited]) - .then(([stdout, stderr, exitCode]) => ({ stdout, stderr, exitCode, timedOut: false })), - timeout.then(() => { proc.kill(); return { stdout: "", stderr: "", exitCode: -1, timedOut: true }; }), - ]); - - let output: any = {}; - try { output = JSON.parse(res.stdout); } catch { output = res.stdout ? { raw: res.stdout } : {}; } - return { name, output, exitCode: res.exitCode, ...(res.timedOut ? { timedOut: true } : {}) }; + const output = parseHookOutput(result.stdout); + return { + name, + output, + exitCode: result.exitCode, + ...(result.error ? { error: result.error } : {}), + ...(result.timedOut ? { timedOut: true } : {}), + }; })); return { content: [{ type: "text", text: JSON.stringify({ results, count: results.length }) }] }; From a973c49f182ea23c17e1fc42934c5a64e7f016fb Mon Sep 17 00:00:00 2001 From: andreihasna Date: Sun, 19 Jul 2026 12:09:45 +0300 Subject: [PATCH 3/8] fix: preserve safe MCP previews --- src/cli/cli.test.ts | 5 +++++ src/index.test.ts | 2 +- src/lib/registry.test.ts | 25 +++++++++++++++++++++++++ src/lib/registry.ts | 13 ++++++++++++- 4 files changed, 43 insertions(+), 2 deletions(-) diff --git a/src/cli/cli.test.ts b/src/cli/cli.test.ts index 925c6bb..0cdfe5e 100644 --- a/src/cli/cli.test.ts +++ b/src/cli/cli.test.ts @@ -232,6 +232,11 @@ describe("CLI", () => { expect(typeof data.project).toBe("boolean"); }); + test("announce-start remains default-deny", async () => { + const data = await runJson("info", "announce-start"); + expect(data.network).toBeUndefined(); + }); + test("--json returns error for unknown hook", async () => { const data = await runJson("info", "nonexistent"); expect(data.error).toContain("not found"); diff --git a/src/index.test.ts b/src/index.test.ts index fba15c4..109dc75 100644 --- a/src/index.test.ts +++ b/src/index.test.ts @@ -131,7 +131,6 @@ describe("library exports", () => { "slacknotify", "session-start", "stop-sync", - "announce-start", "fleet-catchup", "fleet-blockers-gate", ]); @@ -140,6 +139,7 @@ describe("library exports", () => { expect(getHook("gitguard")?.network).toBeUndefined(); expect(getHook("agentmessages")?.network).toBeUndefined(); expect(getHook("knowledge-context")?.network).toBeUndefined(); + expect(resolveHookNetworkAccess(getHook("announce-start")!)).toBe("deny"); expect(getHook("agent-rules-version-check")?.network).toBeUndefined(); for (const shellInterpolatingHook of ["failure-to-task", "announce-stop", "dm-inject"]) { expect(getHook(shellInterpolatingHook)?.network).toBeUndefined(); diff --git a/src/lib/registry.test.ts b/src/lib/registry.test.ts index fce4301..cc96020 100644 --- a/src/lib/registry.test.ts +++ b/src/lib/registry.test.ts @@ -68,6 +68,30 @@ describe("registry", () => { .some((hook) => hook.envAllowlist?.includes("CLAUDE_ENV_FILE")), ).toBe(false); }); + + test("keeps MCP preview limited to audited read-only PreToolUse hooks", () => { + const preToolUseHooks = HOOKS.filter((hook) => hook.event === "PreToolUse"); + + expect(preToolUseHooks.filter((hook) => hook.dryRun).map((hook) => hook.name)).toEqual([ + "gitguard", + "branchprotect", + "worktree-guard", + "packageage", + "pre-bash", + "tddguard", + "envsetup", + "permissionguard", + "protectfiles", + "promptguard", + "stylescheck", + "conflict-detect", + ]); + expect(preToolUseHooks.filter((hook) => !hook.dryRun).map((hook) => hook.name)).toEqual([ + "checkpoint", + "filelock", + "fleet-blockers-gate", + ]); + }); }); describe("CATEGORIES", () => { @@ -337,6 +361,7 @@ describe("registry", () => { const hook = getHook("announce-start")!; expect(hook.event).toBe("SessionStart"); expect(hook.version).toBe("0.2.0"); + expect(hook.network).toBeUndefined(); }); test("session-start fires on SessionStart", () => { diff --git a/src/lib/registry.ts b/src/lib/registry.ts index 34c7b04..326f9d3 100644 --- a/src/lib/registry.ts +++ b/src/lib/registry.ts @@ -67,6 +67,7 @@ export const HOOKS: HookMeta[] = [ event: "PreToolUse", matcher: "Bash", tags: ["git", "safety", "destructive", "guard"], + dryRun: true, }, { name: "branchprotect", @@ -77,6 +78,7 @@ export const HOOKS: HookMeta[] = [ event: "PreToolUse", matcher: "Write|Edit|NotebookEdit", tags: ["git", "branch", "protection", "main"], + dryRun: true, }, { name: "checkpoint", @@ -98,6 +100,7 @@ export const HOOKS: HookMeta[] = [ matcher: "^(Bash|Write|Edit|MultiEdit|NotebookEdit|apply_patch|ApplyPatch|functions\\.apply_patch|mcp__.*)$", tags: ["git", "worktree", "repos", "multi-agent", "safety", "dangerous-ops"], network: "deny", + dryRun: true, }, // Code Quality @@ -183,6 +186,7 @@ export const HOOKS: HookMeta[] = [ matcher: "Bash", tags: ["npm", "packages", "typosquatting", "supply-chain"], network: "allow", + dryRun: true, }, { name: "pre-bash", @@ -194,6 +198,7 @@ export const HOOKS: HookMeta[] = [ matcher: "Bash", tags: ["codewith", "bash", "secrets", "gitleaks", "risky-ops"], network: "deny", + dryRun: true, }, // Notifications @@ -284,6 +289,7 @@ export const HOOKS: HookMeta[] = [ event: "PreToolUse", matcher: "Edit|Write", tags: ["tdd", "tests", "red-green-refactor", "enforcement"], + dryRun: true, }, // Environment @@ -296,6 +302,7 @@ export const HOOKS: HookMeta[] = [ event: "PreToolUse", matcher: "Bash", tags: ["nvm", "virtualenv", "asdf", "rbenv", "environment", "python", "node"], + dryRun: true, }, // Permissions @@ -308,6 +315,7 @@ export const HOOKS: HookMeta[] = [ event: "PreToolUse", matcher: "Bash", tags: ["permission", "allowlist", "blocklist", "safety", "auto-approve"], + dryRun: true, }, { name: "protectfiles", @@ -318,6 +326,7 @@ export const HOOKS: HookMeta[] = [ event: "PreToolUse", matcher: "Edit|Write|Read|Bash", tags: ["security", "env", "secrets", "keys", "lock-files", "protect"], + dryRun: true, }, { name: "promptguard", @@ -328,6 +337,7 @@ export const HOOKS: HookMeta[] = [ event: "PreToolUse", matcher: "", tags: ["prompt", "injection", "security", "validation", "guard"], + dryRun: true, }, { name: "prompt-guard", @@ -425,6 +435,7 @@ export const HOOKS: HookMeta[] = [ event: "PreToolUse", matcher: "Write|Edit", tags: ["design", "styles", "frontend", "css", "tailwind", "design-system", "anti-patterns"], + dryRun: true, }, // Agent Teams @@ -495,6 +506,7 @@ export const HOOKS: HookMeta[] = [ event: "PreToolUse", matcher: "Edit|Write", tags: ["git", "conflicts", "merge", "safety"], + dryRun: true, }, // Workflow Automation (new) @@ -529,7 +541,6 @@ export const HOOKS: HookMeta[] = [ event: "SessionStart", matcher: "", tags: ["announcement", "start", "register", "messages", "agent-teams"], - network: "allow", }, { name: "announce-stop", From bc9bbba4d0f2430516a318262b2463fe6b84e590 Mon Sep 17 00:00:00 2001 From: andreihasna Date: Sun, 19 Jul 2026 12:51:46 +0300 Subject: [PATCH 4/8] fix: make MCP preview decisions honest --- src/mcp/execution.test.ts | 139 +++++++++++++++++++++++++++++++++++++- src/mcp/server.ts | 62 ++++++++++++++--- 2 files changed, 190 insertions(+), 11 deletions(-) diff --git a/src/mcp/execution.test.ts b/src/mcp/execution.test.ts index 5cccd48..d20d381 100644 --- a/src/mcp/execution.test.ts +++ b/src/mcp/execution.test.ts @@ -4,7 +4,7 @@ import { tmpdir } from "os"; import { join } from "path"; import { Client } from "@modelcontextprotocol/sdk/client"; import { InMemoryTransport } from "@modelcontextprotocol/sdk/inMemory.js"; -import type { HookMeta } from "../lib/registry.js"; +import { getHook, type HookMeta } from "../lib/registry.js"; import { createHooksServer } from "./server.js"; type ExecutionOverrides = { @@ -278,16 +278,151 @@ describe("bounded MCP hook execution", () => { arguments: { tool_name: "Bash", tool_input: { command: "echo safe" } }, })); expect(data.results.find((result: any) => result.name === "unsafe")).toMatchObject({ - decision: "approve", + decision: "indeterminate", skipped: true, }); expect(data.results.find((result: any) => result.name === "safe").raw.dry_run).toBe(true); + expect(data.decision).toBe("indeterminate"); + expect(data.indeterminate_by).toEqual(["unsafe"]); expect(existsSync(sentinel)).toBe(false); } finally { await client.close(); } }); + test("hooks_preview sends the PreToolUse event to real dangerous-command guards", async () => { + const preBash = getHook("pre-bash")!; + const worktreeGuard = getHook("worktree-guard")!; + const paths = new Map([ + ["pre-bash", join(import.meta.dir, "..", "..", "hooks", "pre-bash")], + ["worktree-guard", join(import.meta.dir, "..", "..", "hooks", "worktree-guard")], + ]); + const { client } = await withServer([preBash, worktreeGuard], paths); + try { + const data = parse(await client.callTool({ + name: "hooks_preview", + arguments: { tool_name: "Bash", tool_input: { command: "rm -rf /" } }, + })); + + expect(data.results.find((result: any) => result.name === "pre-bash")).toMatchObject({ + decision: "block", + }); + expect(data.results.find((result: any) => result.name === "worktree-guard")).toMatchObject({ + decision: "block", + }); + expect(data.decision).toBe("block"); + expect(data.blocked_by).toBe("pre-bash"); + } finally { + await client.close(); + } + }); + + test("hooks_preview reports missing scripts and containment failures as indeterminate", async () => { + const root = mkdtempSync(join(tmpdir(), "hooks-mcp-preview-failures-")); + roots.push(root); + const sentinel = join(root, "executed"); + const paths = new Map([ + ["contained", fixtureHook(root, "contained", ` + await Bun.write(${JSON.stringify(sentinel)}, "executed"); + console.log(JSON.stringify({ decision: "approve" })); + `)], + ]); + const { client } = await withServer([ + meta("missing", { dryRun: true }), + meta("contained", { dryRun: true }), + ], paths, { + containmentExecutable: join(root, "missing-bwrap"), + }); + try { + const data = parse(await client.callTool({ + name: "hooks_preview", + arguments: { tool_name: "Bash", tool_input: { command: "echo safe" } }, + })); + + expect(data.results.find((result: any) => result.name === "missing")).toMatchObject({ + decision: "indeterminate", + error: "script not found", + }); + expect(data.results.find((result: any) => result.name === "contained")).toMatchObject({ + decision: "indeterminate", + }); + expect(data.results.find((result: any) => result.name === "contained").error).toContain("requires bubblewrap"); + expect(data.decision).toBe("indeterminate"); + expect(data.indeterminate_by).toEqual(["missing", "contained"]); + expect(existsSync(sentinel)).toBe(false); + } finally { + await client.close(); + } + }); + + test("hooks_preview keeps block precedence over nonzero and timeout failures", async () => { + const root = mkdtempSync(join(tmpdir(), "hooks-mcp-preview-precedence-")); + roots.push(root); + const paths = new Map([ + ["nonzero", fixtureHook(root, "nonzero", "process.exit(7)")], + ["timeout", fixtureHook(root, "timeout", "await new Promise(() => {})")], + ["blocker", fixtureHook(root, "blocker", 'console.log(JSON.stringify({ decision: "block", reason: "dangerous" }))')], + ]); + const { client } = await withServer([ + meta("nonzero", { network: "allow", dryRun: true }), + meta("timeout", { network: "allow", dryRun: true }), + meta("blocker", { network: "allow", dryRun: true }), + ], paths); + try { + const data = parse(await client.callTool({ + name: "hooks_preview", + arguments: { + tool_name: "Bash", + tool_input: { command: "echo safe" }, + timeout_ms: 100, + }, + })); + + expect(data.results.find((result: any) => result.name === "nonzero")).toMatchObject({ + decision: "indeterminate", + exitCode: 7, + }); + expect(data.results.find((result: any) => result.name === "timeout")).toMatchObject({ + decision: "indeterminate", + timedOut: true, + }); + expect(data.results.find((result: any) => result.name === "blocker")).toMatchObject({ + decision: "block", + reason: "dangerous", + }); + expect(data.decision).toBe("block"); + expect(data.blocked_by).toBe("blocker"); + expect(data.indeterminate_by).toEqual(["nonzero", "timeout"]); + } finally { + await client.close(); + } + }); + + test("hooks_preview approves when every matching guard completes and approves", async () => { + const root = mkdtempSync(join(tmpdir(), "hooks-mcp-preview-approve-")); + roots.push(root); + const paths = new Map([ + ["explicit", fixtureHook(root, "explicit", 'console.log(JSON.stringify({ decision: "approve" }))')], + ["continuing", fixtureHook(root, "continuing", "console.log(JSON.stringify({ continue: true }))")], + ]); + const { client } = await withServer([ + meta("explicit", { network: "allow", dryRun: true }), + meta("continuing", { network: "allow", dryRun: true }), + ], paths); + try { + const data = parse(await client.callTool({ + name: "hooks_preview", + arguments: { tool_name: "Bash", tool_input: { command: "echo safe" } }, + })); + + expect(data.results.map((result: any) => result.decision)).toEqual(["approve", "approve"]); + expect(data.decision).toBe("approve"); + expect(data.indeterminate_by).toEqual([]); + } finally { + await client.close(); + } + }); + test("MCP execution exposes CLAUDE_ENV_FILE only to its declared hook capability", async () => { const root = mkdtempSync(join(tmpdir(), "hooks-mcp-env-")); roots.push(root); diff --git a/src/mcp/server.ts b/src/mcp/server.ts index 58f7205..7d29e69 100644 --- a/src/mcp/server.ts +++ b/src/mcp/server.ts @@ -144,6 +144,39 @@ function parseHookOutput(stdout: string): any { } } +type PreviewDecision = "approve" | "block" | "indeterminate"; + +interface PreviewHookResult { + name: string; + decision: PreviewDecision; + skipped?: boolean; + error?: string; + timedOut?: boolean; + exitCode?: number | null; + reason?: string; + raw?: unknown; +} + +function classifyPreviewOutput(name: string, output: any): PreviewHookResult { + const permissionDecision = output?.hookSpecificOutput?.permissionDecision; + const reason = output?.reason + ?? output?.stopReason + ?? output?.hookSpecificOutput?.permissionDecisionReason; + + if (output?.decision === "block" || output?.continue === false || permissionDecision === "deny") { + return { name, decision: "block", reason, raw: output }; + } + if (output?.decision === "approve" || output?.continue === true || permissionDecision === "allow") { + return { name, decision: "approve", reason, raw: output }; + } + return { + name, + decision: "indeterminate", + error: "hook output did not contain an approval or block decision", + raw: output, + }; +} + function formatInstallResults(results: InstallResult[], extra?: Record) { const installed = results.filter((r) => r.success).map((r) => r.hook); const failed = results.filter((r) => !r.success).map((r) => ({ hook: r.hook, error: r.error })); @@ -592,13 +625,13 @@ export function createHooksServer(options: HooksServerOptions = {}): McpServer { return { content: [{ type: "text", text: JSON.stringify({ tool_name, matching_hooks: [], result: "no_hooks_match", decision: "approve" }) }] }; } - const input = { tool_name, tool_input }; - const results = await Promise.all(matchingHooks.map(async (name) => { + const input = { hook_event_name: "PreToolUse", tool_name, tool_input }; + const results: PreviewHookResult[] = await Promise.all(matchingHooks.map(async (name) => { const meta = executionGetHook(name)!; if (meta.dryRun !== true) { return { name, - decision: "approve" as const, + decision: "indeterminate" as const, skipped: true, error: `Hook '${name}' does not declare native dry-run support`, }; @@ -606,7 +639,7 @@ export function createHooksServer(options: HooksServerOptions = {}): McpServer { const hookDir = executionGetHookPath(name); const hookScript = join(hookDir, "src", "hook.ts"); - if (!existsSync(hookScript)) return { name, decision: "approve", error: "script not found" }; + if (!existsSync(hookScript)) return { name, decision: "indeterminate", error: "script not found" }; const result = await execute(meta, hookScript, input, { dryRun: true, @@ -614,20 +647,30 @@ export function createHooksServer(options: HooksServerOptions = {}): McpServer { requestedNetwork: network, }); - if (result.error) { + if (result.error || result.timedOut || result.exitCode !== 0) { + const error = result.error + ?? (result.exitCode === null + ? (result.signal ? `hook terminated by signal ${result.signal}` : "hook did not complete successfully") + : `hook exited with code ${result.exitCode}`); return { name, - decision: "approve" as const, + decision: "indeterminate" as const, ...(result.timedOut ? { timedOut: true } : {}), - error: result.error, + error, exitCode: result.exitCode, }; } const output = parseHookOutput(result.stdout); - return { name, decision: output.decision ?? "approve", reason: output.reason, raw: output }; + return classifyPreviewOutput(name, output); })); const blocked = results.find((r) => r.decision === "block"); + const indeterminate = results.filter((r) => r.decision === "indeterminate"); + const decision: PreviewDecision = blocked + ? "block" + : indeterminate.length > 0 + ? "indeterminate" + : "approve"; return { content: [{ type: "text" as const, @@ -635,9 +678,10 @@ export function createHooksServer(options: HooksServerOptions = {}): McpServer { tool_name, matching_hooks: matchingHooks, results, - decision: blocked ? "block" : "approve", + decision, blocked_by: blocked?.name ?? null, blocked_reason: blocked?.reason ?? null, + indeterminate_by: indeterminate.map((result) => result.name), }), }], }; From 0663b9be1eaaad1cd305230cf84644573d2cf4ee Mon Sep 17 00:00:00 2001 From: andreihasna Date: Sun, 19 Jul 2026 13:03:40 +0300 Subject: [PATCH 5/8] fix: reject ambiguous MCP preview decisions --- src/mcp/execution.test.ts | 53 +++++++++++++++++++++++++++++++++++++++ src/mcp/server.ts | 17 +++++++++++++ 2 files changed, 70 insertions(+) diff --git a/src/mcp/execution.test.ts b/src/mcp/execution.test.ts index d20d381..3c7a381 100644 --- a/src/mcp/execution.test.ts +++ b/src/mcp/execution.test.ts @@ -423,6 +423,59 @@ describe("bounded MCP hook execution", () => { } }); + test("hooks_preview keeps malformed, ambiguous, and signaled guards indeterminate", async () => { + const root = mkdtempSync(join(tmpdir(), "hooks-mcp-preview-hostile-output-")); + roots.push(root); + const paths = new Map([ + ["malformed", fixtureHook(root, "malformed", 'console.log("not-json")')], + ["empty", fixtureHook(root, "empty", "")], + ["nondecision", fixtureHook(root, "nondecision", "console.log(JSON.stringify({ dry_run: true }))")], + ["ambiguous", fixtureHook(root, "ambiguous", ` + console.log(JSON.stringify({ + continue: true, + hookSpecificOutput: { permissionDecision: "ask" }, + })); + `)], + ["signaled", fixtureHook(root, "signaled", 'process.kill(process.pid, "SIGTERM")')], + ]); + const { client } = await withServer([ + meta("malformed", { network: "allow", dryRun: true }), + meta("empty", { network: "allow", dryRun: true }), + meta("nondecision", { network: "allow", dryRun: true }), + meta("ambiguous", { network: "allow", dryRun: true }), + meta("signaled", { network: "allow", dryRun: true }), + ], paths); + try { + const data = parse(await client.callTool({ + name: "hooks_preview", + arguments: { tool_name: "Bash", tool_input: { command: "echo safe" } }, + })); + + expect(data.results.map((result: any) => result.decision)).toEqual([ + "indeterminate", + "indeterminate", + "indeterminate", + "indeterminate", + "indeterminate", + ]); + expect(data.results.find((result: any) => result.name === "ambiguous").error).toContain("ambiguous"); + const signaled = data.results.find((result: any) => result.name === "signaled"); + expect(signaled.decision).toBe("indeterminate"); + expect(signaled.exitCode).not.toBe(0); + expect(signaled.error).toMatch(/signal|exited with code/); + expect(data.decision).toBe("indeterminate"); + expect(data.indeterminate_by).toEqual([ + "malformed", + "empty", + "nondecision", + "ambiguous", + "signaled", + ]); + } finally { + await client.close(); + } + }); + test("MCP execution exposes CLAUDE_ENV_FILE only to its declared hook capability", async () => { const root = mkdtempSync(join(tmpdir(), "hooks-mcp-env-")); roots.push(root); diff --git a/src/mcp/server.ts b/src/mcp/server.ts index 7d29e69..27b340f 100644 --- a/src/mcp/server.ts +++ b/src/mcp/server.ts @@ -166,6 +166,23 @@ function classifyPreviewOutput(name: string, output: any): PreviewHookResult { if (output?.decision === "block" || output?.continue === false || permissionDecision === "deny") { return { name, decision: "block", reason, raw: output }; } + + const hasAmbiguousDecision = output !== null + && typeof output === "object" + && ( + ("decision" in output && output.decision !== "approve" && output.decision !== "block") + || ("continue" in output && typeof output.continue !== "boolean") + || (permissionDecision !== undefined && permissionDecision !== "allow" && permissionDecision !== "deny") + ); + if (hasAmbiguousDecision) { + return { + name, + decision: "indeterminate", + error: "hook output contained an ambiguous decision", + raw: output, + }; + } + if (output?.decision === "approve" || output?.continue === true || permissionDecision === "allow") { return { name, decision: "approve", reason, raw: output }; } From 9bba6350a204c5a65b0ca9c0fc87352d09f86bec Mon Sep 17 00:00:00 2001 From: andreihasna Date: Sun, 19 Jul 2026 13:34:04 +0300 Subject: [PATCH 6/8] fix: surface invalid MCP preview registrations --- src/mcp/execution.test.ts | 112 ++++++++++++++++++++++++++++++++++++++ src/mcp/server.ts | 54 +++++++++++++++--- 2 files changed, 158 insertions(+), 8 deletions(-) diff --git a/src/mcp/execution.test.ts b/src/mcp/execution.test.ts index 3c7a381..adc3898 100644 --- a/src/mcp/execution.test.ts +++ b/src/mcp/execution.test.ts @@ -317,6 +317,118 @@ describe("bounded MCP hook execution", () => { } }); + test("hooks_preview retains missing metadata and invalid matchers as indeterminate", async () => { + const invalid = meta("invalid", { matcher: "[" }); + const { client } = await withServer([invalid], new Map(), { + getRegisteredHooks: () => ["ghost", "invalid"], + }); + try { + const data = parse(await client.callTool({ + name: "hooks_preview", + arguments: { tool_name: "Bash", tool_input: { command: "echo safe" } }, + })); + + expect(data.decision).toBe("indeterminate"); + expect(data.result).not.toBe("no_hooks_match"); + expect(data.matching_hooks).toEqual([]); + expect(data.results).toHaveLength(2); + expect(data.results.find((result: any) => result.name === "ghost")).toMatchObject({ + decision: "indeterminate", + }); + expect(data.results.find((result: any) => result.name === "ghost").error).toContain("metadata"); + expect(data.results.find((result: any) => result.name === "invalid")).toMatchObject({ + decision: "indeterminate", + }); + expect(data.results.find((result: any) => result.name === "invalid").error).toContain("matcher"); + expect(data.indeterminate_by).toEqual(["ghost", "invalid"]); + } finally { + await client.close(); + } + }); + + test("hooks_preview distinguishes valid event and tool nonmatches from corrupt registration", async () => { + const { client } = await withServer([ + meta("different-tool", { matcher: "Write" }), + meta("different-event", { event: "PostToolUse", matcher: "Bash" }), + ], new Map()); + try { + const data = parse(await client.callTool({ + name: "hooks_preview", + arguments: { tool_name: "Bash", tool_input: { command: "echo safe" } }, + })); + + expect(data).toEqual({ + tool_name: "Bash", + matching_hooks: [], + result: "no_hooks_match", + decision: "approve", + }); + } finally { + await client.close(); + } + }); + + test("hooks_preview keeps block precedence while retaining an invalid matcher", async () => { + const root = mkdtempSync(join(tmpdir(), "hooks-mcp-preview-invalid-block-")); + roots.push(root); + const paths = new Map([ + ["blocker", fixtureHook(root, "blocker", 'console.log(JSON.stringify({ decision: "block", reason: "dangerous" }))')], + ]); + const { client } = await withServer([ + meta("blocker", { network: "allow", dryRun: true }), + meta("invalid", { matcher: "[" }), + ], paths); + try { + const data = parse(await client.callTool({ + name: "hooks_preview", + arguments: { tool_name: "Bash", tool_input: { command: "echo safe" } }, + })); + + expect(data.decision).toBe("block"); + expect(data.blocked_by).toBe("blocker"); + expect(data.matching_hooks).toEqual(["blocker"]); + expect(data.results.find((result: any) => result.name === "blocker")).toMatchObject({ + decision: "block", + }); + expect(data.results.find((result: any) => result.name === "invalid")).toMatchObject({ + decision: "indeterminate", + }); + expect(data.indeterminate_by).toEqual(["invalid"]); + } finally { + await client.close(); + } + }); + + test("hooks_preview keeps approval indeterminate when a registered matcher is invalid", async () => { + const root = mkdtempSync(join(tmpdir(), "hooks-mcp-preview-invalid-approve-")); + roots.push(root); + const paths = new Map([ + ["explicit", fixtureHook(root, "explicit", 'console.log(JSON.stringify({ decision: "approve" }))')], + ]); + const { client } = await withServer([ + meta("explicit", { network: "allow", dryRun: true }), + meta("invalid", { matcher: "[" }), + ], paths); + try { + const data = parse(await client.callTool({ + name: "hooks_preview", + arguments: { tool_name: "Bash", tool_input: { command: "echo safe" } }, + })); + + expect(data.decision).toBe("indeterminate"); + expect(data.matching_hooks).toEqual(["explicit"]); + expect(data.results.find((result: any) => result.name === "explicit")).toMatchObject({ + decision: "approve", + }); + expect(data.results.find((result: any) => result.name === "invalid")).toMatchObject({ + decision: "indeterminate", + }); + expect(data.indeterminate_by).toEqual(["invalid"]); + } finally { + await client.close(); + } + }); + test("hooks_preview reports missing scripts and containment failures as indeterminate", async () => { const root = mkdtempSync(join(tmpdir(), "hooks-mcp-preview-failures-")); roots.push(root); diff --git a/src/mcp/server.ts b/src/mcp/server.ts index 27b340f..63e3948 100644 --- a/src/mcp/server.ts +++ b/src/mcp/server.ts @@ -157,6 +157,10 @@ interface PreviewHookResult { raw?: unknown; } +type PreviewHookCandidate = + | { kind: "match"; name: string; meta: HookMeta } + | { kind: "error"; name: string; result: PreviewHookResult }; + function classifyPreviewOutput(name: string, output: any): PreviewHookResult { const permissionDecision = output?.hookSpecificOutput?.permissionDecision; const reason = output?.reason @@ -631,20 +635,54 @@ export function createHooksServer(options: HooksServerOptions = {}): McpServer { }, async ({ tool_name, tool_input, scope, timeout_ms, network }) => { const registered = executionGetRegisteredHooks(scope); - const matchingHooks = registered.filter((name) => { + const candidates: PreviewHookCandidate[] = []; + for (const name of registered) { const meta = executionGetHook(name); - if (!meta || meta.event !== "PreToolUse") return false; - if (!meta.matcher) return true; - try { return new RegExp(meta.matcher).test(tool_name); } catch { return false; } - }); + if (!meta) { + candidates.push({ + kind: "error", + name, + result: { + name, + decision: "indeterminate", + error: "registered hook metadata not found", + }, + }); + continue; + } + if (meta.event !== "PreToolUse") continue; + if (meta.matcher) { + let matches: boolean; + try { + matches = new RegExp(meta.matcher).test(tool_name); + } catch { + candidates.push({ + kind: "error", + name, + result: { + name, + decision: "indeterminate", + error: "registered hook matcher is invalid", + }, + }); + continue; + } + if (!matches) continue; + } + candidates.push({ kind: "match", name, meta }); + } + const matchingHooks = candidates + .filter((candidate) => candidate.kind === "match") + .map((candidate) => candidate.name); - if (matchingHooks.length === 0) { + if (candidates.length === 0) { return { content: [{ type: "text", text: JSON.stringify({ tool_name, matching_hooks: [], result: "no_hooks_match", decision: "approve" }) }] }; } const input = { hook_event_name: "PreToolUse", tool_name, tool_input }; - const results: PreviewHookResult[] = await Promise.all(matchingHooks.map(async (name) => { - const meta = executionGetHook(name)!; + const results: PreviewHookResult[] = await Promise.all(candidates.map(async (candidate) => { + if (candidate.kind === "error") return candidate.result; + const { name, meta } = candidate; if (meta.dryRun !== true) { return { name, From d15053c0933b1b78a0cb4801236637f9412b65fb Mon Sep 17 00:00:00 2001 From: andreihasna Date: Sun, 19 Jul 2026 13:53:23 +0300 Subject: [PATCH 7/8] fix: respect multi-event MCP previews --- src/mcp/execution.test.ts | 146 ++++++++++++++++++++++++++++++++++++++ src/mcp/server.ts | 3 +- 2 files changed, 148 insertions(+), 1 deletion(-) diff --git a/src/mcp/execution.test.ts b/src/mcp/execution.test.ts index adc3898..0fa4f29 100644 --- a/src/mcp/execution.test.ts +++ b/src/mcp/execution.test.ts @@ -368,6 +368,152 @@ describe("bounded MCP hook execution", () => { } }); + test("hooks_preview treats multi-event PreToolUse matcher errors as indeterminate", async () => { + const { client } = await withServer([ + meta("multi-invalid", { + event: "PostToolUse", + events: ["PostToolUse", "PreToolUse"], + matcher: "[", + }), + ], new Map()); + try { + const data = parse(await client.callTool({ + name: "hooks_preview", + arguments: { tool_name: "Bash", tool_input: { command: "echo safe" } }, + })); + + expect(data.decision).toBe("indeterminate"); + expect(data.result).not.toBe("no_hooks_match"); + expect(data.matching_hooks).toEqual([]); + expect(data.results).toHaveLength(1); + expect(data.results[0]).toMatchObject({ + name: "multi-invalid", + decision: "indeterminate", + }); + expect(data.results[0].error).toContain("matcher"); + expect(data.indeterminate_by).toEqual(["multi-invalid"]); + } finally { + await client.close(); + } + }); + + test("hooks_preview executes a valid blocking hook that includes PreToolUse in events", async () => { + const root = mkdtempSync(join(tmpdir(), "hooks-mcp-preview-multi-block-")); + roots.push(root); + const paths = new Map([ + ["multi-blocker", fixtureHook(root, "multi-blocker", 'console.log(JSON.stringify({ decision: "block", reason: "dangerous" }))')], + ]); + const { client } = await withServer([ + meta("multi-blocker", { + event: "PostToolUse", + events: ["PostToolUse", "PreToolUse"], + matcher: "Bash", + network: "allow", + dryRun: true, + }), + ], paths); + try { + const data = parse(await client.callTool({ + name: "hooks_preview", + arguments: { tool_name: "Bash", tool_input: { command: "echo safe" } }, + })); + + expect(data.matching_hooks).toEqual(["multi-blocker"]); + expect(data.results).toHaveLength(1); + expect(data.results[0]).toMatchObject({ + name: "multi-blocker", + decision: "block", + reason: "dangerous", + }); + expect(data.decision).toBe("block"); + expect(data.blocked_by).toBe("multi-blocker"); + } finally { + await client.close(); + } + }); + + test("hooks_preview ignores a valid multi-event hook whose matcher does not match the tool", async () => { + const { client } = await withServer([ + meta("multi-nonmatch", { + event: "PostToolUse", + events: ["PostToolUse", "PreToolUse"], + matcher: "Write", + dryRun: true, + }), + ], new Map()); + try { + const data = parse(await client.callTool({ + name: "hooks_preview", + arguments: { tool_name: "Bash", tool_input: { command: "echo safe" } }, + })); + + expect(data).toEqual({ + tool_name: "Bash", + matching_hooks: [], + result: "no_hooks_match", + decision: "approve", + }); + } finally { + await client.close(); + } + }); + + test("hooks_preview treats nonempty events as authoritative before matcher validation", async () => { + const { client } = await withServer([ + meta("events-exclude-pre", { + event: "PreToolUse", + events: ["PostToolUse"], + matcher: "[", + }), + meta("single-post", { + event: "PostToolUse", + matcher: "[", + }), + ], new Map()); + try { + const data = parse(await client.callTool({ + name: "hooks_preview", + arguments: { tool_name: "Bash", tool_input: { command: "echo safe" } }, + })); + + expect(data).toEqual({ + tool_name: "Bash", + matching_hooks: [], + result: "no_hooks_match", + decision: "approve", + }); + } finally { + await client.close(); + } + }); + + test("hooks_preview falls back to the legacy event for empty and absent events arrays", async () => { + const root = mkdtempSync(join(tmpdir(), "hooks-mcp-preview-legacy-event-")); + roots.push(root); + const approve = 'console.log(JSON.stringify({ decision: "approve" }))'; + const paths = new Map([ + ["empty-events", fixtureHook(root, "empty-events", approve)], + ["legacy-event", fixtureHook(root, "legacy-event", approve)], + ]); + const { client } = await withServer([ + meta("empty-events", { events: [], network: "allow", dryRun: true }), + meta("legacy-event", { network: "allow", dryRun: true }), + ], paths); + try { + const data = parse(await client.callTool({ + name: "hooks_preview", + arguments: { tool_name: "Bash", tool_input: { command: "echo safe" } }, + })); + + expect(data.matching_hooks).toEqual(["empty-events", "legacy-event"]); + expect(data.results.map((result: any) => result.decision)).toEqual(["approve", "approve"]); + expect(data.decision).toBe("approve"); + expect(data.indeterminate_by).toEqual([]); + } finally { + await client.close(); + } + }); + test("hooks_preview keeps block precedence while retaining an invalid matcher", async () => { const root = mkdtempSync(join(tmpdir(), "hooks-mcp-preview-invalid-block-")); roots.push(root); diff --git a/src/mcp/server.ts b/src/mcp/server.ts index 63e3948..5ebb532 100644 --- a/src/mcp/server.ts +++ b/src/mcp/server.ts @@ -30,6 +30,7 @@ import { getHooksByCategory, searchHooks, getHook, + getHookEvents, resolveHookEnvironmentAllowlist, resolveHookNetworkAccess, type HookMeta, @@ -650,7 +651,7 @@ export function createHooksServer(options: HooksServerOptions = {}): McpServer { }); continue; } - if (meta.event !== "PreToolUse") continue; + if (!getHookEvents(meta).includes("PreToolUse")) continue; if (meta.matcher) { let matches: boolean; try { From ef91c5704bd7016fa8044d16f32f31dbae5efddd Mon Sep 17 00:00:00 2001 From: andreihasna Date: Sun, 19 Jul 2026 14:37:16 +0300 Subject: [PATCH 8/8] fix: route agentmessages by hook event --- hooks/bounded-process.js | 2 - src/cli/cli.test.ts | 89 ++++++++++++++++++++++ src/cli/index.tsx | 111 +++++++++++++++++++-------- src/index.test.ts | 41 +++++++++- src/index.ts | 17 +++-- src/lib/installer.test.ts | 111 +++++++++++++++++++++++++-- src/lib/installer.ts | 67 ++++++++++------- src/lib/registry.test.ts | 80 ++++++++++++++++++-- src/lib/registry.ts | 133 ++++++++++++++++++++++++++++++-- src/mcp/execution.test.ts | 154 ++++++++++++++++++++++++++++++++++++++ src/mcp/server.test.ts | 25 +++++++ src/mcp/server.ts | 100 +++++++++++++++++++------ 12 files changed, 826 insertions(+), 104 deletions(-) diff --git a/hooks/bounded-process.js b/hooks/bounded-process.js index 95cef52..cdad08b 100644 --- a/hooks/bounded-process.js +++ b/hooks/bounded-process.js @@ -89,8 +89,6 @@ const SAFE_ENV_NAMES = Object.freeze([ "HOOKS_SOUND_FILE", "HOOKS_SPACE", "RUN_ID", - "SMSG_AGENT_ID", - "SMSG_PROJECT_ID", "TASK_ID", "VIRTUAL_ENV", "NVM_DIR", diff --git a/src/cli/cli.test.ts b/src/cli/cli.test.ts index 0cdfe5e..10ae51c 100644 --- a/src/cli/cli.test.ts +++ b/src/cli/cli.test.ts @@ -355,6 +355,35 @@ describe("CLI", () => { expect(Array.isArray(data.healthy_hooks)).toBe(true); expect(Array.isArray(data.issues)).toBe(true); }); + + test("validates all agentmessages entrypoints, events, and timeouts", async () => { + backupSettings(); + try { + const installed = await runJson("install", "agentmessages", "--overwrite"); + expect(installed.installed).toContain("agentmessages"); + + const healthy = await runJson("doctor"); + expect(healthy.healthy_hooks).toContain("agentmessages"); + expect(healthy.issues.filter((issue: any) => issue.hook === "agentmessages")).toEqual([]); + + const settings = JSON.parse(readFileSync(SETTINGS_PATH, "utf-8")); + const stopCommand = settings.hooks.Stop + .flatMap((entry: any) => entry.hooks ?? []) + .find((hook: any) => hook.command === "hooks run agentmessages"); + stopCommand.timeout = 10; + writeFileSync(SETTINGS_PATH, JSON.stringify(settings, null, 2) + "\n"); + + const unhealthy = await runJson("doctor"); + expect(unhealthy.healthy_hooks).not.toContain("agentmessages"); + expect(unhealthy.issues).toContainEqual({ + hook: "agentmessages", + issue: "Incorrect timeout under Stop (expected 5s)", + severity: "error", + }); + } finally { + restoreSettings(); + } + }); }); describe("hooks update", () => { @@ -464,6 +493,66 @@ describe("CLI", () => { expect(stdout).not.toContain("--allow-network"); }); + test("agentmessages requires an event and routes both event-specific entrypoints", async () => { + const home = mkdtempSync(join(tmpdir(), "hooks-cli-agentmessages-")); + const serviceDir = join(home, ".service", "service-message"); + const envFile = join(home, "claude-env"); + try { + const missingEvent = await runWithInputAndEnv("{}", { HOME: home }, "run", "agentmessages"); + expect(missingEvent.exitCode).not.toBe(0); + expect(missingEvent.stderr).toContain("requires hook_event_name"); + + mkdirSync(join(serviceDir, "agents"), { recursive: true }); + writeFileSync(join(serviceDir, "config.json"), JSON.stringify({ agentId: "synthetic-agent" })); + writeFileSync(join(serviceDir, "agents", "synthetic-agent.json"), JSON.stringify({ + id: "synthetic-agent", + name: "Synthetic Agent", + createdAt: 1, + })); + const sessionStart = await runWithInputAndEnv( + JSON.stringify({ hook_event_name: "SessionStart", session_id: "synthetic-session", cwd: home }), + { + HOME: home, + CLAUDE_ENV_FILE: envFile, + SMSG_AGENT_ID: "must-not-route-session-start", + SMSG_PROJECT_ID: "must-not-route-session-start", + }, + "run", "agentmessages", + ); + expect(sessionStart.exitCode).toBe(0); + expect(JSON.parse(sessionStart.stdout).continue).toBe(true); + expect(readFileSync(envFile, "utf-8")).toContain('export SMSG_AGENT_ID="synthetic-agent"'); + + const projectId = home.split("/").pop()!.toLowerCase().replace(/[^a-z0-9-]/g, "-"); + const inbox = join(serviceDir, "messages", projectId, "inbox", "synthetic-agent"); + mkdirSync(inbox, { recursive: true }); + writeFileSync(join(inbox, "message-1.json"), JSON.stringify({ + id: "message-1", + timestamp: 1, + from: "sender", + to: "synthetic-agent", + project: projectId, + subject: "Synthetic message", + body: "Event-specific Stop entrypoint", + read: false, + })); + const stop = await runWithInputAndEnv( + JSON.stringify({ hook_event_name: "Stop", session_id: "synthetic-session", cwd: home }), + { + HOME: home, + CLAUDE_ENV_FILE: join(home, "must-not-route-stop"), + SMSG_AGENT_ID: "synthetic-agent", + SMSG_PROJECT_ID: projectId, + }, + "run", "agentmessages", + ); + expect(stop.exitCode).toBe(0); + expect(JSON.parse(stop.stdout).stopReason).toContain("Synthetic message"); + } finally { + rmSync(home, { recursive: true, force: true }); + } + }); + test("declared remote hook keeps network by default and accepts explicit denial", async () => { const home = mkdtempSync(join(tmpdir(), "hooks-cli-run-network-")); let requests = 0; diff --git a/src/cli/index.tsx b/src/cli/index.tsx index 917b06f..d06c084 100644 --- a/src/cli/index.tsx +++ b/src/cli/index.tsx @@ -23,8 +23,12 @@ import { getHooksByCategory, searchHooks, getHook, + getHookExecutions, + resolveHookExecution, + resolveHookExecutionTimeoutMs, resolveHookNetworkAccess, resolveHookEnvironmentAllowlist, + type HookExecutionMeta, } from "../lib/registry.js"; import { installHook, @@ -167,34 +171,20 @@ program .option("--profile ", "Agent profile ID") .option("--dry-run", "Run only hooks with native no-write dry-run support", false) .option("--deny-network", "Further restrict an allow-declared hook to local-only access", false) - .option("--timeout-ms ", "Maximum hook runtime in milliseconds", "10000") + .option("--timeout-ms ", "Maximum hook runtime in milliseconds (defaults to the event contract)") .description("Execute a hook (called by AI coding agents)") - .action(async (hook: string, options: { profile?: string; dryRun: boolean; denyNetwork: boolean; timeoutMs: string }) => { + .action(async (hook: string, options: { profile?: string; dryRun: boolean; denyNetwork: boolean; timeoutMs?: string }) => { const meta = getHook(hook); if (!meta) { console.error(JSON.stringify({ error: `Hook '${hook}' not found` })); process.exit(1); } - const hookDir = getHookPath(hook); - const hookScript = join(hookDir, "src", "hook.ts"); - - if (!existsSync(hookScript)) { - console.error(JSON.stringify({ error: `Hook script not found: ${hookScript}` })); - process.exit(1); - } - if (options.dryRun && meta.dryRun !== true) { console.error(JSON.stringify({ error: `Hook '${hook}' does not declare native dry-run support` })); process.exit(1); } - const timeoutMs = Number(options.timeoutMs); - if (!Number.isInteger(timeoutMs) || timeoutMs <= 0) { - console.error(JSON.stringify({ error: "--timeout-ms must be a positive integer" })); - process.exit(1); - } - // Read stdin (agent passes hook context as JSON) let stdin: string; try { @@ -204,6 +194,41 @@ program process.exit(1); } + let routingInput: Record | undefined; + try { + const parsed = JSON.parse(stdin); + if (parsed && typeof parsed === "object" && !Array.isArray(parsed)) { + routingInput = parsed as Record; + } + } catch { + // Legacy single-entrypoint hooks continue to receive non-JSON stdin verbatim. + } + + let execution; + try { + execution = resolveHookExecution(meta, routingInput?.hook_event_name); + } catch (error) { + console.error(JSON.stringify({ error: error instanceof Error ? error.message : String(error) })); + process.exit(1); + } + + let requestedTimeoutMs: number | undefined; + if (options.timeoutMs !== undefined) { + requestedTimeoutMs = Number(options.timeoutMs); + if (!Number.isInteger(requestedTimeoutMs) || requestedTimeoutMs <= 0) { + console.error(JSON.stringify({ error: "--timeout-ms must be a positive integer" })); + process.exit(1); + } + } + const timeoutMs = resolveHookExecutionTimeoutMs(execution, requestedTimeoutMs); + + const hookDir = getHookPath(hook); + const hookScript = join(hookDir, execution.entrypoint); + if (!existsSync(hookScript)) { + console.error(JSON.stringify({ error: `Hook script not found: ${hookScript}` })); + process.exit(1); + } + // Dry-run is injected before any mutating profile operation. Invalid input // is rejected because silently dropping the marker could execute a write. let hookStdin = stdin; @@ -239,7 +264,7 @@ program timeoutMs, network: resolveHookNetworkAccess(meta, options.denyNetwork ? "deny" : undefined), env: process.env, - envAllowlist: resolveHookEnvironmentAllowlist(meta), + envAllowlist: resolveHookEnvironmentAllowlist(meta, [], execution.event), }); if (result.stdout) process.stdout.write(result.stdout); @@ -644,28 +669,52 @@ program continue; } - // Check hook has source const hookDir = getHookPath(name); - const hookScript = join(hookDir, "src", "hook.ts"); - if (!existsSync(hookScript)) { - issues.push({ hook: name, issue: "Missing src/hook.ts in package", severity: "error" }); + let executions: HookExecutionMeta[]; + try { + executions = meta ? getHookExecutions(meta) : []; + for (const execution of executions) { + if (!existsSync(join(hookDir, execution.entrypoint))) { + issues.push({ hook: name, issue: `Missing ${execution.entrypoint} in package`, severity: "error" }); + hookHealthy = false; + } + } + } catch (error) { + issues.push({ + hook: name, + issue: error instanceof Error ? error.message : String(error), + severity: "error", + }); hookHealthy = false; + executions = []; } // Verify correct event registration if (meta && settingsExist) { try { const settings = JSON.parse(readFileSync(settingsPath, "utf-8")); - const eventHooks = settings.hooks?.[meta.event] || []; - const found = eventHooks.some((entry: any) => - entry.hooks?.some((h: any) => { - const match = h.command?.match(/^hooks run ([\w-]+)/); - return match && match[1] === name; - }) - ); - if (!found) { - issues.push({ hook: name, issue: `Not registered under correct event (${meta.event})`, severity: "error" }); - hookHealthy = false; + for (const execution of executions) { + const eventHooks = settings.hooks?.[execution.event] || []; + const registeredCommands = eventHooks.flatMap((entry: any) => + (entry.hooks ?? []).filter((h: any) => { + const match = h.command?.match(/^hooks run ([\w-]+)/); + return match && match[1] === name; + }) + ); + if (registeredCommands.length === 0) { + issues.push({ hook: name, issue: `Not registered under correct event (${execution.event})`, severity: "error" }); + hookHealthy = false; + } else if ( + execution.timeout !== undefined + && !registeredCommands.every((command: any) => command.timeout === execution.timeout) + ) { + issues.push({ + hook: name, + issue: `Incorrect timeout under ${execution.event} (expected ${execution.timeout}s)`, + severity: "error", + }); + hookHealthy = false; + } } } catch {} } diff --git a/src/index.test.ts b/src/index.test.ts index 109dc75..2c3bcb6 100644 --- a/src/index.test.ts +++ b/src/index.test.ts @@ -11,8 +11,11 @@ import { HOOKS, CATEGORIES, getHook, + getHookExecutions, getHooksByCategory, searchHooks, + resolveHookExecution, + resolveHookExecutionTimeoutMs, resolveHookNetworkAccess, resolveHookEnvironmentAllowlist, installHook, @@ -57,6 +60,12 @@ describe("library exports", () => { expect(getHook("gitguard")?.name).toBe("gitguard"); }); + test("event-specific execution helpers are exported", () => { + expect(typeof getHookExecutions).toBe("function"); + expect(typeof resolveHookExecution).toBe("function"); + expect(typeof resolveHookExecutionTimeoutMs).toBe("function"); + }); + test("getHooksByCategory is a function", () => { expect(typeof getHooksByCategory).toBe("function"); expect(getHooksByCategory("Git Safety")).toHaveLength(5); @@ -164,11 +173,39 @@ describe("library exports", () => { }); test("runHook environment capabilities cannot be elevated across hooks", () => { - expect(resolveHookEnvironmentAllowlist(getHook("agentmessages")!)).toEqual(["CLAUDE_ENV_FILE"]); + const agentmessages = getHook("agentmessages")!; + expect(resolveHookEnvironmentAllowlist(agentmessages, [], "SessionStart")).toEqual(["CLAUDE_ENV_FILE"]); + expect(resolveHookEnvironmentAllowlist(agentmessages, [], "Stop")).toEqual([ + "SMSG_AGENT_ID", + "SMSG_PROJECT_ID", + ]); + expect(() => resolveHookEnvironmentAllowlist(agentmessages)).toThrow("requires hook_event_name"); expect(() => resolveHookEnvironmentAllowlist( - getHook("gitguard")!, + agentmessages, + ["SMSG_AGENT_ID"], + "SessionStart", + )).toThrow("does not declare environment capability"); + expect(() => resolveHookEnvironmentAllowlist( + agentmessages, ["CLAUDE_ENV_FILE"], + "Stop", )).toThrow("does not declare environment capability"); + for (const capability of ["CLAUDE_ENV_FILE", "SMSG_AGENT_ID", "SMSG_PROJECT_ID"]) { + expect(() => resolveHookEnvironmentAllowlist( + getHook("gitguard")!, + [capability], + )).toThrow("does not declare environment capability"); + } + }); + + test("runHook requires an explicit event for event-specific hooks", async () => { + let message = ""; + try { + await runHook("agentmessages", {}); + } catch (error) { + message = error instanceof Error ? error.message : String(error); + } + expect(message).toContain("requires hook_event_name"); }); test("runHook preserves network access only for a declared remote hook", async () => { diff --git a/src/index.ts b/src/index.ts index 7eb64c3..f5dd2c6 100644 --- a/src/index.ts +++ b/src/index.ts @@ -14,11 +14,15 @@ export { CATEGORIES, getHook, getHookEvents, + getHookExecutions, getHooksByCategory, searchHooks, + resolveHookExecution, + resolveHookExecutionTimeoutMs, resolveHookNetworkAccess, resolveHookEnvironmentAllowlist, type HookMeta, + type HookExecutionMeta, type HookEvent, type Category, } from "./lib/registry.js"; @@ -109,6 +113,8 @@ export function removeProjectHook(name: string): boolean { // ── runHook — programmatic hook execution ───────────────────────────────────── import { getHook as _getHook } from "./lib/registry.js"; +import { resolveHookExecution as _resolveHookExecution } from "./lib/registry.js"; +import { resolveHookExecutionTimeoutMs as _resolveHookExecutionTimeoutMs } from "./lib/registry.js"; import { resolveHookNetworkAccess as _resolveHookNetworkAccess } from "./lib/registry.js"; import { resolveHookEnvironmentAllowlist as _resolveHookEnvironmentAllowlist } from "./lib/registry.js"; import { getHookPath as _getHookPath, hookExists as _hookExists } from "./lib/installer.js"; @@ -119,7 +125,7 @@ import { runBoundedProcess, type HookNetworkAccess } from "../hooks/bounded-proc export interface RunHookOptions { /** Agent profile ID to inject into hook input */ profile?: string; - /** Timeout in milliseconds (default: 10000) */ + /** Timeout override in milliseconds (defaults to the selected event contract, otherwise 10000) */ timeout?: number; /** Propagate a no-write dry-run marker. Unsupported hooks are rejected. */ dryRun?: boolean; @@ -144,7 +150,7 @@ export interface RunHookResult { /** * Programmatically execute a hook with the given input. - * Spawns the hook's src/hook.ts via bun, passes input as stdin JSON, + * Spawns the entrypoint declared for the input event via bun, passes input as stdin JSON, * and returns the parsed stdout JSON. */ export async function runHook(name: string, input: HookInput, options: RunHookOptions = {}): Promise { @@ -156,8 +162,9 @@ export async function runHook(name: string, input: HookInput, options: RunHookOp throw new Error(`Hook '${name}' does not declare native dry-run support`); } + const execution = _resolveHookExecution(meta, input.hook_event_name); const hookDir = _getHookPath(name); - const hookScript = join(hookDir, "src", "hook.ts"); + const hookScript = join(hookDir, execution.entrypoint); if (!existsSync(hookScript)) throw new Error(`Hook script not found: ${hookScript}`); let hookInput: HookInput = { ...input, ...(dryRun ? { dry_run: true } : {}) }; @@ -176,10 +183,10 @@ export async function runHook(name: string, input: HookInput, options: RunHookOp const result = await runBoundedProcess([process.execPath, "run", hookScript], { input: JSON.stringify(hookInput), - timeoutMs: options.timeout, + timeoutMs: _resolveHookExecutionTimeoutMs(execution, options.timeout), network: _resolveHookNetworkAccess(meta, options.network), env: options.env ?? process.env, - envAllowlist: _resolveHookEnvironmentAllowlist(meta, options.envAllowlist), + envAllowlist: _resolveHookEnvironmentAllowlist(meta, options.envAllowlist, execution.event), maxInputBytes: options.maxInputBytes, maxStdoutBytes: options.maxStdoutBytes, maxStderrBytes: options.maxStderrBytes, diff --git a/src/lib/installer.test.ts b/src/lib/installer.test.ts index 27e9ce7..9e773ac 100644 --- a/src/lib/installer.test.ts +++ b/src/lib/installer.test.ts @@ -15,7 +15,7 @@ import { buildCodewithTomlFragment, isEventSupported, } from "./installer.js"; -import { HOOKS, getHookEvents } from "./registry.js"; +import { HOOKS, getHookEvents, getHookExecutions } from "./registry.js"; const GLOBAL_SETTINGS = join(homedir(), ".claude", "settings.json"); @@ -299,6 +299,82 @@ describe("installer", () => { expect(found).toBe(true); }); + test("agentmessages preserves one generic command across both event registrations", () => { + const result = installHook("agentmessages"); + expect(result.success).toBe(true); + + const settings = JSON.parse(readFileSync(GLOBAL_SETTINGS, "utf-8")); + for (const [event, timeout] of [["SessionStart", 10], ["Stop", 5]] as const) { + const matches = (settings.hooks[event] || []).filter((entry: any) => + entry.hooks?.some((hook: any) => hook.command === "hooks run agentmessages") + ); + expect(matches).toHaveLength(1); + expect(matches[0].hooks).toEqual([{ + type: "command", + command: "hooks run agentmessages", + timeout, + }]); + } + }); + + test("agentmessages overwrite deduplicates generic and package-specific registrations", () => { + const hookDir = getHookPath("agentmessages"); + const settings = existsSync(GLOBAL_SETTINGS) + ? JSON.parse(readFileSync(GLOBAL_SETTINGS, "utf-8")) + : {}; + settings.hooks ??= {}; + settings.hooks.SessionStart = [ + ...(settings.hooks.SessionStart ?? []), + { hooks: [{ type: "command", command: `bun ${join(hookDir, "src", "session-start.ts")}`, timeout: 10 }] }, + { hooks: [{ type: "command", command: "hooks run agentmessages" }] }, + ]; + settings.hooks.Stop = [ + ...(settings.hooks.Stop ?? []), + { hooks: [{ type: "command", command: `bun ${join(hookDir, "src", "check-messages.ts")}`, timeout: 5 }] }, + { hooks: [{ type: "command", command: "hooks run agentmessages --profile synthetic-profile" }] }, + ]; + writeFileSync(GLOBAL_SETTINGS, JSON.stringify(settings, null, 2) + "\n"); + + expect(getRegisteredHooks()).toContain("agentmessages"); + const result = installHook("agentmessages", { overwrite: true }); + expect(result.success).toBe(true); + + const after = JSON.parse(readFileSync(GLOBAL_SETTINGS, "utf-8")); + const commands = Object.values(after.hooks).flatMap((entries: any) => + entries.flatMap((entry: any) => entry.hooks?.map((hook: any) => hook.command) ?? []) + ); + expect(commands.filter((command) => command === "hooks run agentmessages")).toHaveLength(2); + expect(commands.some((command) => command.includes("hook-agentmessages/src/session-start.ts"))).toBe(false); + expect(commands.some((command) => command.includes("hook-agentmessages/src/check-messages.ts"))).toBe(false); + expect(commands.some((command) => command.includes("--profile synthetic-profile"))).toBe(false); + }); + + test("agentmessages overwrite preserves unrelated commands co-located with a direct registration", () => { + const hookDir = getHookPath("agentmessages"); + const settings = existsSync(GLOBAL_SETTINGS) + ? JSON.parse(readFileSync(GLOBAL_SETTINGS, "utf-8")) + : {}; + settings.hooks ??= {}; + settings.hooks.SessionStart = [{ + matcher: "synthetic-shared-entry", + hooks: [ + { type: "command", command: `bun ${join(hookDir, "src", "session-start.ts")}`, timeout: 10 }, + { type: "command", command: "hooks run gitguard" }, + ], + }]; + writeFileSync(GLOBAL_SETTINGS, JSON.stringify(settings, null, 2) + "\n"); + + const result = installHook("agentmessages", { overwrite: true }); + expect(result.success).toBe(true); + + const after = JSON.parse(readFileSync(GLOBAL_SETTINGS, "utf-8")); + const shared = after.hooks.SessionStart.find((entry: any) => entry.matcher === "synthetic-shared-entry"); + expect(shared?.hooks).toEqual([{ type: "command", command: "hooks run gitguard" }]); + expect(after.hooks.SessionStart.flatMap((entry: any) => entry.hooks).filter( + (hook: any) => hook.command === "hooks run agentmessages" + )).toHaveLength(1); + }); + test("Notification hook registers under Notification", () => { installHook("contextrefresh"); // Notification const settings = JSON.parse(readFileSync(GLOBAL_SETTINGS, "utf-8")); @@ -525,11 +601,12 @@ describe("installer", () => { const HOOK_SOURCE_NAMES = HOOKS.map((hook) => hook.name); const CATALOG_ONLY_HOOKS = new Set(["knowledge-context"]); - test("every hook has src/hook.ts in package (except agentmessages)", () => { + test("every hook execution entrypoint exists in its package", () => { for (const name of HOOK_SOURCE_NAMES) { - if (name === "agentmessages") continue; // uses different file structure - const hookScript = join(getHookPath(name), "src", "hook.ts"); - expect(existsSync(hookScript)).toBe(true); + const meta = HOOKS.find((hook) => hook.name === name)!; + for (const execution of getHookExecutions(meta)) { + expect(existsSync(join(getHookPath(name), execution.entrypoint))).toBe(true); + } } }); @@ -591,6 +668,21 @@ describe("installer", () => { expect(result.error).toContain("not supported by target 'gemini'"); }); + test("gemini rejects all of agentmessages instead of partially installing Stop", () => { + const settingsPath = getSettingsPath("global", "gemini"); + const before = existsSync(settingsPath) ? readFileSync(settingsPath, "utf-8") : null; + try { + const result = installHook("agentmessages", { target: "gemini", overwrite: true }); + expect(result.success).toBe(false); + expect(result.error).toContain("SessionStart"); + const after = existsSync(settingsPath) ? readFileSync(settingsPath, "utf-8") : null; + expect(after).toBe(before); + } finally { + if (before === null) rmSync(settingsPath, { force: true }); + else writeFileSync(settingsPath, before); + } + }); + test("hyphenated hook names round-trip install → list → remove (regression)", () => { const result = installHook("fleet-blockers-gate", { overwrite: true }); expect(result.success).toBe(true); @@ -663,6 +755,15 @@ describe("installer", () => { expect(fragment).toContain('statusMessage = "Loading Knowledge context"'); }); + test("agentmessages Codewith fragments preserve event timeout parity", () => { + const fragment = buildCodewithTomlFragment("agentmessages"); + expect(fragment).toContain("[[hooks.SessionStart]]"); + expect(fragment).toContain("[[hooks.Stop]]"); + expect(fragment.match(/command = "hooks run agentmessages"/g)).toHaveLength(2); + expect(fragment.match(/timeout = 10/g)).toHaveLength(1); + expect(fragment.match(/timeout = 5/g)).toHaveLength(1); + }); + test("worktree-guard Codewith matcher covers file tool aliases", () => { const fragment = buildCodewithTomlFragment("worktree-guard"); expect(fragment).toContain("[[hooks.PreToolUse]]"); diff --git a/src/lib/installer.ts b/src/lib/installer.ts index c81aa5f..6a06732 100644 --- a/src/lib/installer.ts +++ b/src/lib/installer.ts @@ -14,7 +14,7 @@ import { existsSync, mkdirSync, readFileSync, writeFileSync } from "fs"; import { join, dirname } from "path"; import { homedir } from "os"; import { fileURLToPath } from "url"; -import { getHook, getHookEvents, type HookEvent } from "./registry.js"; +import { getHook, getHookEvents, getHookExecutions, type HookEvent } from "./registry.js"; const __dirname = dirname(fileURLToPath(import.meta.url)); const HOOKS_DIR = existsSync(join(__dirname, "..", "..", "hooks", "hook-gitguard")) @@ -36,14 +36,27 @@ function shortHookName(name: string): string { return name.startsWith("hook-") ? name.slice("hook-".length) : name; } +function hookNameFromCommand(command: unknown): string | undefined { + if (typeof command !== "string") return undefined; + const genericMatch = command.match(/^hooks run ([\w-]+)(?:\s+--profile\s+[\w-]+)?$/); + if (genericMatch) return genericMatch[1]; + const legacyMatch = command.match(/^hook-([\w-]+)$/); + if (legacyMatch) return legacyMatch[1]; + if ( + /^(?:bun|\/[\w./-]+\/bun) \/[^\s]+\/hook-agentmessages\/src\/(?:session-start|check-messages)\.ts$/.test(command) + ) { + return "agentmessages"; + } + return undefined; +} + function removeHookEntriesByName(entries: any[], hookName: string): any[] { - return entries.filter( - (entry: any) => !entry.hooks?.some((h: any) => { - // [\w-]+ — hook names may contain hyphens (announce-start, fleet-catchup, …) - const match = h.command?.match(/^hooks run ([\w-]+)/); - return match && match[1] === hookName; - }) - ); + return entries.flatMap((entry: any) => { + if (!Array.isArray(entry?.hooks)) return [entry]; + const hooks = entry.hooks.filter((hook: any) => hookNameFromCommand(hook.command) !== hookName); + if (hooks.length === entry.hooks.length) return [entry]; + return hooks.length > 0 ? [{ ...entry, hooks }] : []; + }); } /** @@ -230,10 +243,10 @@ export function buildCodewithTomlFragment(name: string, profile?: string): strin const matcher = codewithMatcher(meta.matcher); const fragments: string[] = []; - for (const event of getHookEvents(meta)) { - const eventKey = getTargetEventName(event, "codewith"); + for (const execution of getHookExecutions(meta)) { + const eventKey = getTargetEventName(execution.event, "codewith"); if (!eventKey) { - throw new Error(`Hook '${shortName}' uses event '${event}', which is not supported by the Codewith target`); + throw new Error(`Hook '${shortName}' uses event '${execution.event}', which is not supported by the Codewith target`); } const lines: string[] = [ @@ -245,7 +258,7 @@ export function buildCodewithTomlFragment(name: string, profile?: string): strin `[[hooks.${eventKey}.hooks]]`, `type = "command"`, `command = ${tomlString(command)}`, - `timeout = ${codewithTimeout(shortName)}`, + `timeout = ${execution.timeout ?? codewithTimeout(shortName)}`, `statusMessage = ${tomlString(codewithStatusMessage(shortName))}`, ); fragments.push(lines.join("\n")); @@ -429,17 +442,19 @@ function registerHook(name: string, scope: Scope = "global", target: WritableJso const meta = getHook(name); if (!meta) return; - const eventKeys = getHookEvents(meta).map((event) => { - const eventKey = getTargetEventName(event, target); + const registrations = getHookExecutions(meta).map((execution) => { + const eventKey = getTargetEventName(execution.event, target); if (eventKey === null) { - throw new Error(`Event '${event}' is not supported by target '${target}'`); + throw new Error(`Event '${execution.event}' is not supported by target '${target}'`); } - return eventKey; + return { eventKey, execution }; }); - const uniqueEventKeys = [...new Set(eventKeys)]; - if (uniqueEventKeys.length === 0) { + if (registrations.length === 0) { throw new Error(`Hook '${name}' has no installable events for target '${target}'`); } + if (new Set(registrations.map(({ eventKey }) => eventKey)).size !== registrations.length) { + throw new Error(`Hook '${name}' maps multiple executions to the same '${target}' event`); + } const settings = readSettings(scope, target); if (!settings.hooks) settings.hooks = {}; @@ -453,11 +468,15 @@ function registerHook(name: string, scope: Scope = "global", target: WritableJso ? `hooks run ${name} --profile ${profile}` : `hooks run ${name}`; - for (const eventKey of uniqueEventKeys) { + for (const { eventKey, execution } of registrations) { if (!settings.hooks[eventKey]) settings.hooks[eventKey] = []; const entry: Record = { - hooks: [{ type: "command", command: hookCommand }], + hooks: [{ + type: "command", + command: hookCommand, + ...(execution.timeout ? { timeout: execution.timeout } : {}), + }], }; if (meta.matcher) { entry.matcher = meta.matcher; @@ -519,12 +538,8 @@ export function getRegisteredHooksForTarget(scope: Scope = "global", target: Sin for (const eventKey of Object.keys(settings.hooks)) { for (const entry of settings.hooks[eventKey]) { for (const hook of entry.hooks || []) { - const newMatch = hook.command?.match(/^hooks run ([\w-]+)(?:\s+--profile\s+[\w-]+)?$/); - const oldMatch = hook.command?.match(/^hook-([\w-]+)$/); - const match = newMatch || oldMatch; - if (match) { - registered.push(match[1]); - } + const name = hookNameFromCommand(hook.command); + if (name) registered.push(name); } } } diff --git a/src/lib/registry.test.ts b/src/lib/registry.test.ts index cc96020..c7b5380 100644 --- a/src/lib/registry.test.ts +++ b/src/lib/registry.test.ts @@ -7,6 +7,9 @@ import { searchHooks, getHook, getHookEvents, + getHookExecutions, + resolveHookExecution, + resolveHookExecutionTimeoutMs, type HookMeta, type Category, } from "./registry.js"; @@ -61,12 +64,77 @@ describe("registry", () => { } }); - test("scopes CLAUDE_ENV_FILE to the source-proven agentmessages hook", () => { - expect(getHook("agentmessages")?.envAllowlist).toEqual(["CLAUDE_ENV_FILE"]); - expect( - HOOKS.filter((hook) => hook.name !== "agentmessages") - .some((hook) => hook.envAllowlist?.includes("CLAUDE_ENV_FILE")), - ).toBe(false); + test("declares event-specific agentmessages entrypoints and capabilities", () => { + const hook = getHook("agentmessages")! as HookMeta & { + executions?: Array<{ + event: string; + entrypoint: string; + timeout?: number; + envAllowlist?: readonly string[]; + }>; + }; + + expect(getHookEvents(hook)).toEqual(["SessionStart", "Stop"]); + expect(hook.executions).toEqual([ + { + event: "SessionStart", + entrypoint: "src/session-start.ts", + timeout: 10, + envAllowlist: ["CLAUDE_ENV_FILE"], + }, + { + event: "Stop", + entrypoint: "src/check-messages.ts", + timeout: 5, + envAllowlist: ["SMSG_AGENT_ID", "SMSG_PROJECT_ID"], + }, + ]); + expect(hook.envAllowlist).toBeUndefined(); + }); + + test("validates execution metadata and requires an explicit event for multi-entrypoint hooks", () => { + const agentmessages = getHook("agentmessages")!; + expect(resolveHookExecution(agentmessages, "SessionStart").entrypoint).toBe("src/session-start.ts"); + expect(resolveHookExecution(agentmessages, "Stop").entrypoint).toBe("src/check-messages.ts"); + expect(resolveHookExecutionTimeoutMs(resolveHookExecution(agentmessages, "SessionStart"))).toBe(10_000); + expect(resolveHookExecutionTimeoutMs(resolveHookExecution(agentmessages, "Stop"))).toBe(5_000); + expect(resolveHookExecutionTimeoutMs(resolveHookExecution(agentmessages, "Stop"), 1_234)).toBe(1_234); + expect(() => resolveHookExecution(agentmessages)).toThrow("requires hook_event_name"); + + const legacy = getHook("gitguard")!; + expect(getHookExecutions(legacy)).toEqual([{ + event: "PreToolUse", + entrypoint: "src/hook.ts", + }]); + + const malformed = (executions: any[], events?: any[]) => ({ + ...agentmessages, + name: "malformed", + ...(events ? { events } : { events: undefined }), + executions, + }) as HookMeta; + expect(() => getHookExecutions(malformed([]))).toThrow("empty executions contract"); + expect(() => getHookExecutions(malformed([ + { event: "Stop", entrypoint: "src/a.ts" }, + { event: "Stop", entrypoint: "src/b.ts" }, + ]))).toThrow("duplicate execution event"); + expect(() => getHookExecutions(malformed([ + { event: "Stop", entrypoint: "../outside.ts" }, + ]))).toThrow("invalid execution entrypoint"); + expect(() => getHookExecutions(malformed([ + { event: "Unsupported", entrypoint: "src/hook.ts" }, + ]))).toThrow("unsupported execution event"); + expect(() => getHookExecutions(malformed([ + { event: "Stop", entrypoint: "src/hook.ts", timeout: 0 }, + ]))).toThrow("invalid execution timeout"); + expect(() => getHookExecutions(malformed([ + { event: "SessionStart", entrypoint: "src/start.ts" }, + { event: "Stop", entrypoint: "src/stop.ts" }, + ], ["Stop", "SessionStart"]))).toThrow("must exactly match"); + expect(() => getHookExecutions(malformed([ + { event: "SessionStart", entrypoint: "src/start.ts" }, + { event: "Stop", entrypoint: "src/stop.ts" }, + ], []))).toThrow("must exactly match"); }); test("keeps MCP preview limited to audited read-only PreToolUse hooks", () => { diff --git a/src/lib/registry.ts b/src/lib/registry.ts index 326f9d3..73dcd4e 100644 --- a/src/lib/registry.ts +++ b/src/lib/registry.ts @@ -23,6 +23,16 @@ export const HOOK_EVENTS: HookEvent[] = [ "SubagentStart", ]; +export interface HookExecutionMeta { + event: HookEvent; + /** Package-relative script path for this event. */ + entrypoint: string; + /** Event-specific timeout in seconds for installation and default execution. */ + timeout?: number; + /** Extra non-sensitive environment names this event handler is explicitly allowed to receive. */ + envAllowlist?: readonly string[]; +} + export interface HookMeta { name: string; displayName: string; @@ -39,6 +49,8 @@ export interface HookMeta { dryRun?: boolean; /** Extra non-sensitive environment names this hook is explicitly allowed to receive. */ envAllowlist?: readonly string[]; + /** Event-specific scripts and capabilities. When present, this is the authoritative event contract. */ + executions?: readonly HookExecutionMeta[]; } export const CATEGORIES = [ @@ -222,7 +234,20 @@ export const HOOKS: HookMeta[] = [ event: "Stop", matcher: "", tags: ["messaging", "agents", "inter-agent"], - envAllowlist: ["CLAUDE_ENV_FILE"], + executions: [ + { + event: "SessionStart", + entrypoint: "src/session-start.ts", + timeout: 10, + envAllowlist: ["CLAUDE_ENV_FILE"], + }, + { + event: "Stop", + entrypoint: "src/check-messages.ts", + timeout: 5, + envAllowlist: ["SMSG_AGENT_ID", "SMSG_PROJECT_ID"], + }, + ], }, // Context Management @@ -605,10 +630,100 @@ export function getHooksByCategory(category: Category): HookMeta[] { return HOOKS.filter((h) => h.category === category); } -export function getHookEvents(hook: HookMeta): HookEvent[] { +const DEFAULT_HOOK_ENTRYPOINT = "src/hook.ts"; + +function legacyHookEvents(hook: HookMeta): HookEvent[] { return hook.events && hook.events.length > 0 ? hook.events : [hook.event]; } +function validateHookExecution(hook: HookMeta, execution: HookExecutionMeta): void { + if (!HOOK_EVENTS.includes(execution.event)) { + throw new Error(`Hook '${hook.name}' declares unsupported execution event '${execution.event}'`); + } + if ( + !execution.entrypoint + || execution.entrypoint.startsWith("/") + || /^[A-Za-z]:\//.test(execution.entrypoint) + || execution.entrypoint.includes("\\") + || execution.entrypoint.includes("\0") + || execution.entrypoint.split("/").some((part) => part === "" || part === "." || part === "..") + ) { + throw new Error(`Hook '${hook.name}' declares invalid execution entrypoint '${execution.entrypoint}'`); + } + if (execution.timeout !== undefined && (!Number.isInteger(execution.timeout) || execution.timeout <= 0)) { + throw new Error(`Hook '${hook.name}' declares invalid execution timeout '${execution.timeout}'`); + } +} + +export function getHookExecutions(hook: HookMeta): HookExecutionMeta[] { + if (hook.executions === undefined) { + return legacyHookEvents(hook).map((event) => ({ + event, + entrypoint: DEFAULT_HOOK_ENTRYPOINT, + ...(hook.envAllowlist ? { envAllowlist: hook.envAllowlist } : {}), + })); + } + if (hook.executions.length === 0) { + throw new Error(`Hook '${hook.name}' declares an empty executions contract`); + } + + const seen = new Set(); + for (const execution of hook.executions) { + validateHookExecution(hook, execution); + if (seen.has(execution.event)) { + throw new Error(`Hook '${hook.name}' declares duplicate execution event '${execution.event}'`); + } + seen.add(execution.event); + } + if (!seen.has(hook.event)) { + throw new Error(`Hook '${hook.name}' executions do not include primary event '${hook.event}'`); + } + if (hook.events !== undefined) { + const executionEvents = hook.executions.map((execution) => execution.event); + if ( + hook.events.length !== executionEvents.length + || hook.events.some((event, index) => event !== executionEvents[index]) + ) { + throw new Error(`Hook '${hook.name}' events must exactly match its authoritative executions`); + } + } + return hook.executions.map((execution) => ({ ...execution })); +} + +export function getHookEvents(hook: HookMeta): HookEvent[] { + return getHookExecutions(hook).map((execution) => execution.event); +} + +export function resolveHookExecution( + hook: HookMeta, + requestedEvent?: unknown, +): HookExecutionMeta { + const executions = getHookExecutions(hook); + let event = requestedEvent; + if (event === undefined) { + if (hook.executions && executions.length > 1) { + throw new Error(`Hook '${hook.name}' requires hook_event_name to select an event-specific entrypoint`); + } + event = hook.event; + } + if (typeof event !== "string" || !HOOK_EVENTS.includes(event as HookEvent)) { + throw new Error(`Hook '${hook.name}' received unsupported hook_event_name '${String(event)}'`); + } + + const execution = executions.find((candidate) => candidate.event === event); + if (!execution) { + throw new Error(`Hook '${hook.name}' does not declare an entrypoint for event '${event}'`); + } + return execution; +} + +export function resolveHookExecutionTimeoutMs( + execution: HookExecutionMeta, + requestedTimeoutMs?: number, +): number { + return requestedTimeoutMs ?? (execution.timeout ? execution.timeout * 1000 : 10_000); +} + export function searchHooks(query: string): HookMeta[] { const q = query.toLowerCase(); return HOOKS.filter( @@ -636,16 +751,24 @@ export function resolveHookNetworkAccess( return declared; } -const HOOK_SCOPED_ENV_CAPABILITIES = new Set(["CLAUDE_ENV_FILE"]); +const HOOK_SCOPED_ENV_CAPABILITIES = new Set([ + "CLAUDE_ENV_FILE", + "SMSG_AGENT_ID", + "SMSG_PROJECT_ID", +]); export function resolveHookEnvironmentAllowlist( hook: HookMeta, requested: readonly string[] = [], + event?: unknown, ): readonly string[] { - const declared = new Set(hook.envAllowlist ?? []); + const execution = resolveHookExecution(hook, event); + const declared = new Set(execution.envAllowlist ?? []); for (const name of requested) { if (HOOK_SCOPED_ENV_CAPABILITIES.has(name) && !declared.has(name)) { - throw new Error(`Hook '${hook.name}' does not declare environment capability '${name}'`); + throw new Error( + `Hook '${hook.name}' does not declare environment capability '${name}' for event '${execution.event}'`, + ); } declared.add(name); } diff --git a/src/mcp/execution.test.ts b/src/mcp/execution.test.ts index 0fa4f29..eab54d7 100644 --- a/src/mcp/execution.test.ts +++ b/src/mcp/execution.test.ts @@ -140,6 +140,45 @@ describe("bounded MCP hook execution", () => { } }); + test("hooks_run defaults to the event timeout and accepts an explicit override", async () => { + const root = mkdtempSync(join(tmpdir(), "hooks-mcp-event-timeout-")); + roots.push(root); + const paths = new Map([ + ["event-timeout", fixtureHook(root, "event-timeout", ` + await Bun.sleep(1_200); + console.log(JSON.stringify({ completed: true })); + `)], + ]); + const hook = meta("event-timeout", { + event: "Stop", + matcher: "", + network: "allow", + executions: [{ event: "Stop", entrypoint: "src/hook.ts", timeout: 1 }], + }); + const { client } = await withServer([hook], paths); + try { + const defaulted = parse(await client.callTool({ + name: "hooks_run", + arguments: { name: "event-timeout", input: { hook_event_name: "Stop" } }, + })); + expect(defaulted.timedOut).toBe(true); + expect(defaulted.timeout_ms).toBe(1_000); + + const overridden = parse(await client.callTool({ + name: "hooks_run", + arguments: { + name: "event-timeout", + input: { hook_event_name: "Stop" }, + timeout_ms: 2_000, + }, + })); + expect(overridden.error).toBeUndefined(); + expect(overridden.output).toEqual({ completed: true }); + } finally { + await client.close(); + } + }); + test("hooks_run applies declared deny and allow network policies", async () => { const root = mkdtempSync(join(tmpdir(), "hooks-mcp-network-")); roots.push(root); @@ -764,4 +803,119 @@ describe("bounded MCP hook execution", () => { await client.close(); } }); + + test("MCP execution routes agentmessages by event and isolates event capabilities", async () => { + const root = mkdtempSync(join(tmpdir(), "hooks-mcp-event-entrypoints-")); + roots.push(root); + const agentmessagesDir = join(root, "agentmessages"); + mkdirSync(join(agentmessagesDir, "src"), { recursive: true }); + writeFileSync(join(agentmessagesDir, "src", "session-start.ts"), ` + console.log(JSON.stringify({ + entrypoint: "session-start", + claudeEnvFile: process.env.CLAUDE_ENV_FILE ?? "unset", + agentId: process.env.SMSG_AGENT_ID ?? "unset", + projectId: process.env.SMSG_PROJECT_ID ?? "unset", + })); + `); + writeFileSync(join(agentmessagesDir, "src", "check-messages.ts"), ` + console.log(JSON.stringify({ + entrypoint: "check-messages", + claudeEnvFile: process.env.CLAUDE_ENV_FILE ?? "unset", + agentId: process.env.SMSG_AGENT_ID ?? "unset", + projectId: process.env.SMSG_PROJECT_ID ?? "unset", + })); + `); + const ordinarySource = ` + console.log(JSON.stringify({ + claudeEnvFile: process.env.CLAUDE_ENV_FILE ?? "unset", + agentId: process.env.SMSG_AGENT_ID ?? "unset", + projectId: process.env.SMSG_PROJECT_ID ?? "unset", + })); + `; + const paths = new Map([ + ["agentmessages", agentmessagesDir], + ["ordinary", fixtureHook(root, "ordinary", ordinarySource)], + ]); + const agentmessages = meta("agentmessages", { + event: "Stop", + events: ["SessionStart", "Stop"], + envAllowlist: undefined, + executions: [ + { + event: "SessionStart", + entrypoint: "src/session-start.ts", + envAllowlist: ["CLAUDE_ENV_FILE"], + }, + { + event: "Stop", + entrypoint: "src/check-messages.ts", + envAllowlist: ["SMSG_AGENT_ID", "SMSG_PROJECT_ID"], + }, + ], + } as Partial & Record); + const envFile = join(root, "claude-env"); + const { client } = await withServer([ + agentmessages, + meta("ordinary"), + ], paths, { + env: { + PATH: process.env.PATH ?? "", + CLAUDE_ENV_FILE: envFile, + SMSG_AGENT_ID: "synthetic-agent", + SMSG_PROJECT_ID: "synthetic-project", + }, + }); + + try { + const missingEvent = parse(await client.callTool({ + name: "hooks_run", + arguments: { + name: "agentmessages", + input: {}, + }, + })); + const sessionStart = parse(await client.callTool({ + name: "hooks_run", + arguments: { + name: "agentmessages", + input: { hook_event_name: "SessionStart" }, + }, + })); + const stop = parse(await client.callTool({ + name: "hooks_run", + arguments: { + name: "agentmessages", + input: { hook_event_name: "Stop" }, + }, + })); + const ordinary = parse(await client.callTool({ + name: "hooks_run", + arguments: { + name: "ordinary", + input: { hook_event_name: "PreToolUse" }, + }, + })); + + expect(missingEvent.error).toContain("requires hook_event_name"); + expect(sessionStart.output).toEqual({ + entrypoint: "session-start", + claudeEnvFile: envFile, + agentId: "unset", + projectId: "unset", + }); + expect(stop.output).toEqual({ + entrypoint: "check-messages", + claudeEnvFile: "unset", + agentId: "synthetic-agent", + projectId: "synthetic-project", + }); + expect(ordinary.output).toEqual({ + claudeEnvFile: "unset", + agentId: "unset", + projectId: "unset", + }); + } finally { + await client.close(); + } + }); }); diff --git a/src/mcp/server.test.ts b/src/mcp/server.test.ts index ae9416d..a3ce9c4 100644 --- a/src/mcp/server.test.ts +++ b/src/mcp/server.test.ts @@ -321,6 +321,31 @@ describe("MCP server", () => { restoreSettings(); }); + test("hooks_doctor validates agentmessages event timeout parity", async () => { + await client.callTool({ + name: "hooks_install", + arguments: { hooks: ["agentmessages"], overwrite: true }, + }); + const healthy = parseResult(await client.callTool({ name: "hooks_doctor", arguments: {} })); + expect(healthy.healthy_hooks).toContain("agentmessages"); + expect(healthy.issues.filter((issue: any) => issue.hook === "agentmessages")).toEqual([]); + + const settings = JSON.parse(readFileSync(SETTINGS_PATH, "utf-8")); + const startCommand = settings.hooks.SessionStart + .flatMap((entry: any) => entry.hooks ?? []) + .find((hook: any) => hook.command === "hooks run agentmessages"); + delete startCommand.timeout; + writeFileSync(SETTINGS_PATH, JSON.stringify(settings, null, 2) + "\n"); + + const unhealthy = parseResult(await client.callTool({ name: "hooks_doctor", arguments: {} })); + expect(unhealthy.healthy_hooks).not.toContain("agentmessages"); + expect(unhealthy.issues).toContainEqual({ + hook: "agentmessages", + issue: "Incorrect timeout under SessionStart (expected 10s)", + severity: "error", + }); + }); + // --- hooks_categories --- test("hooks_categories returns all 5", async () => { diff --git a/src/mcp/server.ts b/src/mcp/server.ts index 5ebb532..59ad251 100644 --- a/src/mcp/server.ts +++ b/src/mcp/server.ts @@ -31,9 +31,13 @@ import { searchHooks, getHook, getHookEvents, + getHookExecutions, + resolveHookExecution, + resolveHookExecutionTimeoutMs, resolveHookEnvironmentAllowlist, resolveHookNetworkAccess, type HookMeta, + type HookExecutionMeta, type Category, } from "../lib/registry.js"; import { @@ -115,9 +119,11 @@ export async function executeMcpHook( ): Promise { let network: HookNetworkAccess; let envAllowlist: readonly string[]; + let execution: HookExecutionMeta; try { + execution = resolveHookExecution(meta, input.hook_event_name); network = resolveHookNetworkAccess(meta, options.requestedNetwork); - envAllowlist = resolveHookEnvironmentAllowlist(meta, options.envAllowlist); + envAllowlist = resolveHookEnvironmentAllowlist(meta, options.envAllowlist, input.hook_event_name); } catch (error) { return failedExecution(error instanceof Error ? error.message : String(error)); } @@ -126,7 +132,7 @@ export async function executeMcpHook( return runBoundedProcess([process.execPath, "run", hookScript], { cwd: options.cwd, input: JSON.stringify(hookInput), - timeoutMs: options.timeoutMs, + timeoutMs: resolveHookExecutionTimeoutMs(execution, options.timeoutMs), network, env: options.env ?? process.env, envAllowlist, @@ -383,24 +389,50 @@ export function createHooksServer(options: HooksServerOptions = {}): McpServer { } const hookDir = getHookPath(name); - if (!existsSync(join(hookDir, "src", "hook.ts"))) { - issues.push({ hook: name, issue: "Missing src/hook.ts in package", severity: "error" }); + let executions: HookExecutionMeta[]; + try { + executions = meta ? getHookExecutions(meta) : []; + for (const execution of executions) { + if (!existsSync(join(hookDir, execution.entrypoint))) { + issues.push({ hook: name, issue: `Missing ${execution.entrypoint} in package`, severity: "error" }); + hookHealthy = false; + } + } + } catch (error) { + issues.push({ + hook: name, + issue: error instanceof Error ? error.message : String(error), + severity: "error", + }); hookHealthy = false; + executions = []; } if (meta && settingsExist) { try { const settings = JSON.parse(readFileSync(settingsPath, "utf-8")); - const eventHooks = settings.hooks?.[meta.event] || []; - const found = eventHooks.some((entry: any) => - entry.hooks?.some((h: any) => { - const match = h.command?.match(/^hooks run ([\w-]+)/); - return match && match[1] === name; - }) - ); - if (!found) { - issues.push({ hook: name, issue: `Not registered under correct event (${meta.event})`, severity: "error" }); - hookHealthy = false; + for (const execution of executions) { + const eventHooks = settings.hooks?.[execution.event] || []; + const registeredCommands = eventHooks.flatMap((entry: any) => + (entry.hooks ?? []).filter((h: any) => { + const match = h.command?.match(/^hooks run ([\w-]+)/); + return match && match[1] === name; + }) + ); + if (registeredCommands.length === 0) { + issues.push({ hook: name, issue: `Not registered under correct event (${execution.event})`, severity: "error" }); + hookHealthy = false; + } else if ( + execution.timeout !== undefined + && !registeredCommands.every((command: any) => command.timeout === execution.timeout) + ) { + issues.push({ + hook: name, + issue: `Incorrect timeout under ${execution.event} (expected ${execution.timeout}s)`, + severity: "error", + }); + hookHealthy = false; + } } } catch {} } @@ -495,7 +527,7 @@ export function createHooksServer(options: HooksServerOptions = {}): McpServer { name: z.string().describe("Hook name (e.g. 'gitguard', 'checkpoint')"), input: z.record(z.string(), z.unknown()).default(() => ({})).describe("Hook input as JSON object (HookInput)"), profile: z.string().optional().describe("Agent profile ID to inject into hook input"), - timeout_ms: z.number().int().positive().max(86_400_000).default(10000).describe("Timeout in milliseconds (default: 10000)"), + timeout_ms: z.number().int().positive().max(86_400_000).optional().describe("Timeout in milliseconds (defaults to the event contract)"), dry_run: z.boolean().default(false).describe("Require native no-write dry-run support"), network: z.literal("deny").optional().describe("Further restrict an allow-declared hook to local-only access"), }, @@ -505,8 +537,15 @@ export function createHooksServer(options: HooksServerOptions = {}): McpServer { return { content: [{ type: "text", text: JSON.stringify({ error: `Hook '${name}' not found` }) }] }; } + let execution; + try { + execution = resolveHookExecution(meta, input.hook_event_name); + } catch (error) { + return { content: [{ type: "text", text: JSON.stringify({ error: error instanceof Error ? error.message : String(error) }) }] }; + } + const hookDir = executionGetHookPath(name); - const hookScript = join(hookDir, "src", "hook.ts"); + const hookScript = join(hookDir, execution.entrypoint); if (!existsSync(hookScript)) { return { content: [{ type: "text", text: JSON.stringify({ error: `Hook script not found: ${hookScript}` }) }] }; } @@ -529,9 +568,10 @@ export function createHooksServer(options: HooksServerOptions = {}): McpServer { } } + const effectiveTimeoutMs = resolveHookExecutionTimeoutMs(execution, timeout_ms); const result = await execute(meta, hookScript, hookInput, { dryRun: wantsDryRun, - timeoutMs: timeout_ms, + timeoutMs: effectiveTimeoutMs, requestedNetwork: network, }); const output = parseHookOutput(result.stdout); @@ -545,7 +585,7 @@ export function createHooksServer(options: HooksServerOptions = {}): McpServer { stderr: result.stderr || undefined, exitCode: result.exitCode, ...(result.error ? { error: result.error } : {}), - ...(result.timedOut ? { timedOut: true, timeout_ms } : {}), + ...(result.timedOut ? { timedOut: true, timeout_ms: effectiveTimeoutMs } : {}), }), }], }; @@ -693,8 +733,18 @@ export function createHooksServer(options: HooksServerOptions = {}): McpServer { }; } + let execution; + try { + execution = resolveHookExecution(meta, "PreToolUse"); + } catch (error) { + return { + name, + decision: "indeterminate" as const, + error: error instanceof Error ? error.message : String(error), + }; + } const hookDir = executionGetHookPath(name); - const hookScript = join(hookDir, "src", "hook.ts"); + const hookScript = join(hookDir, execution.entrypoint); if (!existsSync(hookScript)) return { name, decision: "indeterminate", error: "script not found" }; const result = await execute(meta, hookScript, input, { @@ -780,7 +830,7 @@ export function createHooksServer(options: HooksServerOptions = {}): McpServer { dry_run: z.boolean().default(false).describe("Require native no-write dry-run support"), network: z.literal("deny").optional().describe("Further restrict an allow-declared hook"), })).max(32).describe("List of at most 32 hooks to run with their inputs"), - timeout_ms: z.number().int().positive().max(86_400_000).default(10000).describe("Per-hook timeout in milliseconds"), + timeout_ms: z.number().int().positive().max(86_400_000).optional().describe("Per-hook timeout override in milliseconds"), }, async ({ hooks, timeout_ms }) => { const results = await Promise.all(hooks.map(async ({ name, input, dry_run, network }) => { @@ -790,12 +840,18 @@ export function createHooksServer(options: HooksServerOptions = {}): McpServer { if (wantsDryRun && meta.dryRun !== true) { return { name, error: `Hook '${name}' does not declare native dry-run support` }; } - const hookScript = join(executionGetHookPath(name), "src", "hook.ts"); + let execution; + try { + execution = resolveHookExecution(meta, input.hook_event_name); + } catch (error) { + return { name, error: error instanceof Error ? error.message : String(error) }; + } + const hookScript = join(executionGetHookPath(name), execution.entrypoint); if (!existsSync(hookScript)) return { name, error: "script not found" }; const result = await execute(meta, hookScript, input, { dryRun: wantsDryRun, - timeoutMs: timeout_ms, + timeoutMs: resolveHookExecutionTimeoutMs(execution, timeout_ms), requestedNetwork: network, }); const output = parseHookOutput(result.stdout);