From 8870da431357aea4195805ad2d9ebee2ae7953a1 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Wed, 1 Jul 2026 10:48:23 +0800 Subject: [PATCH] =?UTF-8?q?fix(hooks):=20address=20Codex=20review=20?= =?UTF-8?q?=E2=80=94=20Stop=20force-continue,=20Codex=20tool=5Fname=20+=20?= =?UTF-8?q?plain-stdout=20context,=20defer=20continue:false?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-1 Codex review findings on the bridges: - Stop force-continue (both bridges): a blocking Stop hook with EMPTY stderr yielded decision 'deny' + reason undefined, and the `&& reason !== undefined` guard let the turn STOP — the opposite of a blocking Stop hook. Force-continue on any deny; fall back to a generic steering line when there is no reason. - Codex payload tool_name: hardcoded "Bash" disagreed with the exec.name matcher subject, so a real Codex `matcher:"Bash"` never fired against the harness's lowercase `bash` tool. Use exec.name in both payload builders (matches the matcher subject and the sibling CC bridge). Doc/RFC updated. - Codex plain-stdout context: SessionStart/UserPromptSubmit are documented to treat a clean hook's PLAIN (non-JSON) stdout as additionalContext, but nothing folded it. runPoint now folds plain stdout into context for those two events, gated on the codec's JSON gate so structured stdout is never dumped as prose. - continue:false is deferred, not honored: the seams have no hard-halt primitive yet. TODO(hook-continue-false) at both bridges + an RFC deferred note; the two tests now assert the LOG records the halt request AND that the run is NOT actually halted (no longer misleading). - README concurrency wording: hooks run SERIALLY (deliberate — adjacent invoked/result log pairs, order-independent fold), not concurrently. Fixed the CC README claim + an RFC note. Regression guards proven red on the unfixed code, then reverted. The mismatched- hookEventName discard (also flagged) is fixed in dsh-hook-protocol and merged down. --- .../feature/2026-06-30-hook-bridges.md | 7 +- packages/hooks/hooks-claude/README.md | 2 +- packages/hooks/hooks-claude/src/index.ts | 18 +++- .../hooks/hooks-claude/tests/coverage.spec.ts | 33 ++++++- packages/hooks/hooks-codex/README.md | 2 +- packages/hooks/hooks-codex/src/index.ts | 40 ++++++-- .../hooks/hooks-codex/tests/coverage.spec.ts | 92 ++++++++++++++++++- 7 files changed, 174 insertions(+), 20 deletions(-) diff --git a/docs/rfc/implemented/feature/2026-06-30-hook-bridges.md b/docs/rfc/implemented/feature/2026-06-30-hook-bridges.md index a3e01c17be..1e782f9b67 100644 --- a/docs/rfc/implemented/feature/2026-06-30-hook-bridges.md +++ b/docs/rfc/implemented/feature/2026-06-30-hook-bridges.md @@ -15,7 +15,7 @@ The framing that shapes the whole design: **a bridge is a faithfulness adapter, Two independent plugins in the `packages/hooks/` group, each a function/namespace plugin (`name`/`inject`/`Config`/`apply`, NO default export — see [postmortem 0001](../../../postmortem/0001-acp-default-export-drops-inject.md)) injecting only `bash`: - **`dsh-hooks-claude`** — the CC dialect. Seven hook points: `SessionStart`, `UserPromptSubmit`, `PreToolUse`, `PostToolUse`, `Stop`, `SubagentStart`, `SubagentStop`. Owns CC's per-event stdin payloads (a base of `session_id`/`cwd`/`hook_event_name` plus per-event fields), CC's env + `${CLAUDE_PLUGIN_ROOT}`/`${CLAUDE_PROJECT_DIR}` substitution, and the literal-or-regex matcher mode. A CC hook's stdin carries a **trailing newline**. -- **`dsh-hooks-codex`** — the Codex dialect: a deliberate SUBSET. Five hook points (`PreToolUse`, `PostToolUse`, `SessionStart`, `UserPromptSubmit`, `Stop` — no subagent/notification/compaction), an always-regex matcher, snake_case payloads with `turn_id`/`model`/`permission_mode` extras written WITHOUT a trailing newline, no env and no `${…}` substitution, and a block-only decision model (a Codex hook can never pre-approve, so `allow`/`ask` are not honored). Codex hardcodes a tool call's `tool_name` to `"Bash"` and `tool_input` to `{ command }`. +- **`dsh-hooks-codex`** — the Codex dialect: a deliberate SUBSET. Five hook points (`PreToolUse`, `PostToolUse`, `SessionStart`, `UserPromptSubmit`, `Stop` — no subagent/notification/compaction), an always-regex matcher, snake_case payloads with `turn_id`/`model`/`permission_mode` extras written WITHOUT a trailing newline, no env and no `${…}` substitution, and a block-only decision model (a Codex hook can never pre-approve, so `allow`/`ask` are not honored). A tool call's payload carries the real `tool_name` (the value the matcher tests, so a config's tool matcher fires) in Codex's `tool_input: { command }` shape. ### Outcome → Decision mapping @@ -44,8 +44,13 @@ The config is parsed ONCE at load; a read/parse failure logs and registers nothi - **Tool-input rewrite.** A CC/Codex `updatedInput` is logged + warned, not honored — input rewrite is a deferred consistency-design problem ([the pre-tool-input-rewrite RFC](../../proposed/feature/2026-06-30-pre-tool-input-rewrite.md)), because the pre-execution args are read by `tool/call` audit + `assistant/message` history + ACP/tool-bash presentation, so an honest rewrite is a design unit, not a field. - **Stop loop-guard** (`TODO(stop-loop-guard)`). CC/Codex break an infinite force-continue with `stop_hook_active` (true once a Stop hook fired this run) plus a max-consecutive cap; both are deferred. Today `stop_hook_active` is always `false`, so a Stop hook that unconditionally blocks would force-continue every step — a hook author must self-limit until the guard lands. - **Permission `ask`** degrades to `deny` at the `tools/pre-execute` seam (`FIXME(permissions)` in the interception-seams RFC) — there is no interactive permission prompt yet. +- **Hook `continue:false` (hard halt).** A hook can ask to halt the whole run (CC/Codex `continue:false`); the shared merge folds it into `MergedHookOutcome.stop`/`stopReason`, but no bridge acts on it (`TODO(hook-continue-false)`) — the interception seams have no "hard-halt the agent" primitive yet (a Decision blocks/steers a single point, not the run). Deferred with the loop-guard work; the halt request is recorded in the `hook/result` log, and the hook keeps its per-point effect (decision/context) meanwhile. - **Config discovery.** The path is explicit in `cordis.yml`; the full multi-layer CC/Codex precedence walk and the trust/hash model are not reimplemented (`TODO`). +### Multiple hooks on one point run serially, not concurrently + +The reference engines run a point's matched hooks concurrently and fold the results. These bridges run them **serially** (`await` per hook inside the match loop) and fold with the same most-restrictive merge. Serial is deliberate: it keeps each hook's `hook/invoked`/`hook/result` pair adjacent and in a deterministic order in the session log, and the fold is order-independent for the decision (`deny > ask > allow`) so the outcome matches. The cost is latency (hook *N* waits for hook *N−1*) and that per-hook timeouts are not overlapped — acceptable for the hook counts real configs use; revisit if a config ever fans out enough for the wall-clock to matter. + ## Consequences The bridges are thin and readable standalone: the correctness-critical halves (matcher semantics, exit-code contract, merge precedence) live in the shared `dsh-hook-protocol`, so each bridge is just config-parse + payload-build + outcome-map. Each is covered at per-file 100% — config-parse branches as unit tests, and the seam mappings end-to-end through the REAL loop + REAL `dsh-bash-local` + REAL shell scripts from a temp `hooks.json` (a scripted mock MODEL is the only stand-in), plus a real-Loader export-shape guard so a stray default export can't silently drop `inject`. Because the seams already carry typed Decisions, a future native plugin needs none of this bridge machinery — it returns a Decision directly. diff --git a/packages/hooks/hooks-claude/README.md b/packages/hooks/hooks-claude/README.md index 02cdfc9894..126573c6be 100644 --- a/packages/hooks/hooks-claude/README.md +++ b/packages/hooks/hooks-claude/README.md @@ -39,7 +39,7 @@ The config is parsed **once** at load. A read/parse failure is contained — the | `SubagentStart` | `subagent/start` (emit) | additionalContext → `agent.inject()` into the live child | | `SubagentStop` | `subagent/end` (emit) | observe-only | -The matcher subject is the tool name (`PreToolUse`/`PostToolUse`), the session source (`SessionStart`), or the child's agent type (`SubagentStart`/`SubagentStop`); `UserPromptSubmit`/`Stop` ignore matchers. Multiple file-configured hooks on one point run concurrently and fold most-restrictively (`deny > ask > allow`, see `dsh-hook-protocol`). +The matcher subject is the tool name (`PreToolUse`/`PostToolUse`), the session source (`SessionStart`), or the child's agent type (`SubagentStart`/`SubagentStop`); `UserPromptSubmit`/`Stop` ignore matchers. Multiple file-configured hooks on one point run **serially, in config order**, and fold most-restrictively (`deny > ask > allow`, see `dsh-hook-protocol`); serial keeps each hook's `hook/invoked`/`hook/result` pair adjacent in the log, and the fold is order-independent for the decision (see the RFC's "run serially, not concurrently" note). ## Context source diff --git a/packages/hooks/hooks-claude/src/index.ts b/packages/hooks/hooks-claude/src/index.ts index 5e8478e2b7..7e37f1fba0 100644 --- a/packages/hooks/hooks-claude/src/index.ts +++ b/packages/hooks/hooks-claude/src/index.ts @@ -161,6 +161,14 @@ export function apply(ctx: Context, config: Config): void { return mergeHookOutputs(outputs) } + // TODO(hook-continue-false): the merge computes `merged.stop`/`stopReason` from + // a hook's `continue:false`, but no seam below honors it — there is no + // "hard-halt the whole agent" primitive on the interception seams yet (a + // Decision can block/deny/steer a single point, not stop the run). Honoring it + // needs that primitive; deferred with the loop-guard work. Until then a + // `continue:false` hook still has its per-point effect (its decision/context), + // and the halt request is recorded in the `hook/result` log but not acted on. + /** Build a HookContext from accumulated additionalContext strings, or undefined when none. */ function contextFrom(merged: MergedHookOutcome): HookContext | undefined { if (merged.additionalContext.length === 0) return undefined @@ -224,9 +232,13 @@ export function apply(ctx: Context, config: Config): void { // step — a hook author must self-limit until the guard lands. --- ctx.on('agent/turn-continuation', async (agent, turn, _default, next): Promise => { const merged = await runPoint('Stop', '', stopPayload(agent), { agent, turn }) - if (merged.decision === 'deny' && merged.reason !== undefined) { - // A blocking Stop hook forces continuation, feeding its reason as next-step steering. - return { action: 'continue', reason: { content: [{ type: 'text', text: merged.reason }], source: PLUGIN_SOURCE } } + if (merged.decision === 'deny') { + // A blocking Stop hook forces continuation. It carries its reason as + // next-step steering; a blocking hook that emitted no reason (exit 2, empty + // stderr) still forces the turn to continue — the block is what matters, so + // fall back to a generic steering line rather than letting the turn stop. + const text = merged.reason ?? 'continue: blocked by Stop hook' + return { action: 'continue', reason: { content: [{ type: 'text', text }], source: PLUGIN_SOURCE } } } return next() }) diff --git a/packages/hooks/hooks-claude/tests/coverage.spec.ts b/packages/hooks/hooks-claude/tests/coverage.spec.ts index 29dda6b850..9b48e6d645 100644 --- a/packages/hooks/hooks-claude/tests/coverage.spec.ts +++ b/packages/hooks/hooks-claude/tests/coverage.spec.ts @@ -148,6 +148,25 @@ describe('hooks-claude coverage — Stop continuation + subagent inject/catch', expect(JSON.stringify(adapter.requests[1]!.messages)).toContain('continue please') }) + it('a Stop hook that blocks with EMPTY stderr still forces continuation (no reason required)', async () => { + // Regression: a blocking Stop hook (exit 2) with no stderr yields decision + // 'deny' + reason undefined; the turn must STILL force-continue (the block is + // what matters), not silently stop. Self-limit to one block so it can't loop. + const d = dir() + const marker = join(d, 'fired') + const s = sh(d, 'stop.sh', `#!/usr/bin/env bash\nif [ -e "${marker}" ]; then exit 0; fi\ntouch "${marker}"\nexit 2\n`) + const path = hooks(d, { Stop: [{ hooks: [{ type: 'command', command: s }] }] }) + const adapter = new MockAdapter([textResponse('one'), textResponse('two')]) + const ctx = await harness(path, adapter) + const agent = ctx.agentLoop.create(AgentId('a1'), { model: 'mock' }) + agent.send([{ type: 'text', text: 'go' }]) + await waitForIdle(ctx, agent) + // A second model request ran → the empty-reason block forced continuation. + expect(adapter.requests).toHaveLength(2) + // The steering carried the fallback reason (no stderr to use). + expect(JSON.stringify(adapter.requests[1]!.messages)).toContain('blocked by Stop hook') + }) + it('SubagentStart additionalContext is injected into a REGISTERED live child', async () => { const d = dir() const s = sh(d, 'sa.sh', '#!/usr/bin/env bash\necho \'{"hookSpecificOutput":{"hookEventName":"SubagentStart","additionalContext":"child guidance"}}\'\n') @@ -329,18 +348,26 @@ describe('hooks-claude coverage — schema-bypass default + unspawnable hook', ( }) describe('hooks-claude coverage — continue:false, context arm, no-cwd', () => { - it('a hook with {"continue":false} and no decision records decision "stop"', async () => { + it('a {"continue":false} hook is RECORDED as decision "stop" but does not halt the run (TODO(hook-continue-false))', async () => { + // Honoring `continue:false` (hard-halt the whole run) is deferred — there is + // no such primitive on the interception seams yet. So this asserts the LOG + // faithfully records the halt request (decision "stop"), AND that the run is + // NOT actually halted: the tool still runs and the turn completes normally. const d = dir() const s = sh(d, 'stop.sh', '#!/usr/bin/env bash\necho \'{"continue":false,"stopReason":"halt"}\'\n') const path = hooks(d, { PreToolUse: [{ hooks: [{ type: 'command', command: s }] }] }) const adapter = new MockAdapter([toolCallResponse('c1', 'echo', {}), textResponse('done')]) const ctx = await harness(path, adapter) - ctx.tools.register(defineTool({ name: 'echo', description: 'e', parameters: {}, async execute() { return [{ type: 'text', text: 'ok' }] } })) + let ran = false + ctx.tools.register(defineTool({ name: 'echo', description: 'e', parameters: {}, async execute() { ran = true; return [{ type: 'text', text: 'ok' }] } })) const agent = ctx.agentLoop.create(AgentId('a1'), { model: 'mock' }) agent.send([{ type: 'text', text: 'go' }]) await waitForIdle(ctx, agent) const res = events(agent).find(e => e.type === 'hook/result') - expect(res?.type === 'hook/result' && res.data.decision).toBe('stop') + expect(res?.type === 'hook/result' && res.data.decision).toBe('stop') // recorded + expect(ran).toBe(true) // NOT honored: the tool still ran (halt is deferred) + const turnEnd = events(agent).findLast(e => e.type === 'turn/end') + expect(turnEnd?.type === 'turn/end' && turnEnd.data.reason.kind).toBe('completed') // ran to completion }) it('a PostToolUse hook that BOTH blocks AND attaches additionalContext', async () => { diff --git a/packages/hooks/hooks-codex/README.md b/packages/hooks/hooks-codex/README.md index 39d4fcd5ca..e20b3a211d 100644 --- a/packages/hooks/hooks-codex/README.md +++ b/packages/hooks/hooks-codex/README.md @@ -43,7 +43,7 @@ The config is parsed **once** at load; a read/parse failure is contained (logs + | `PostToolUse` | `tools/post-execute` (waterfall) | `block` → `block` with feedback; additionalContext → `accept` with context | | `Stop` | `agent/turn-continuation` (waterfall) | a blocking Stop hook forces `continue` with the reason as next-step steering | -Codex hardcodes a tool call's `tool_name` to `"Bash"` and `tool_input` to `{ command }` (extracted from the call's arguments, or `''` when absent). The matcher subject is the tool name (`PreToolUse`/`PostToolUse`) or the session source (`SessionStart`); `UserPromptSubmit`/`Stop` ignore matchers. +A tool call's payload carries the real `tool_name` (the same value the matcher tests) and Codex's `tool_input: { command }` shape (the `command` arg when present, else `''`). The matcher subject is the tool name (`PreToolUse`/`PostToolUse`) or the session source (`SessionStart`); `UserPromptSubmit`/`Stop` ignore matchers. ## Context source diff --git a/packages/hooks/hooks-codex/src/index.ts b/packages/hooks/hooks-codex/src/index.ts index 127511389d..b61e60bdfb 100644 --- a/packages/hooks/hooks-codex/src/index.ts +++ b/packages/hooks/hooks-codex/src/index.ts @@ -86,7 +86,7 @@ export function apply(ctx: Context, config: Config): void { point: string, matchQuery: string, payload: unknown, - opts: { agent?: Agent; turn?: number; signal?: AbortSignal }, + opts: { agent?: Agent; turn?: number; signal?: AbortSignal; plainStdoutAsContext?: boolean }, ): Promise { const groups: MatcherGroup[] = parsed[point] ?? [] const outputs: HookOutput[] = [] @@ -108,6 +108,17 @@ export function apply(ctx: Context, config: Config): void { defaultTimeoutMs, trailingNewline: false, // Codex writes stdin WITHOUT a trailing newline. }, () => performance.now()) + // Codex's SessionStart/UserPromptSubmit treat a clean hook's PLAIN + // (non-JSON) stdout as additionalContext. The codec keeps that raw text on + // `output.stdout` but only sets `additionalContext` from a JSON + // `hookSpecificOutput`, so fold plain stdout in here and let the shared + // merge + contextFrom path carry it. Guarded on the codec's own JSON gate + // (stdout starting with `{`) so a structured hook's raw JSON is never + // injected as prose, and it never clobbers an explicit additionalContext. + if (opts.plainStdoutAsContext === true && output.additionalContext === undefined + && output.stdout.length > 0 && !output.stdout.startsWith('{')) { + output.additionalContext = output.stdout + } outputs.push(output) if (session && opts.turn !== undefined) { const stderrSummary = summarize(output.stderr) @@ -124,6 +135,12 @@ export function apply(ctx: Context, config: Config): void { return mergeHookOutputs(outputs) } + // TODO(hook-continue-false): the merge computes `merged.stop`/`stopReason` from + // a hook's `continue:false`, but no seam below honors it — there is no + // "hard-halt the whole agent" primitive on the interception seams yet. Deferred + // with the loop-guard work; until then a `continue:false` hook keeps its + // per-point effect and the halt request is recorded in `hook/result`, not acted on. + function contextFrom(merged: MergedHookOutcome): HookContext | undefined { if (merged.additionalContext.length === 0) return undefined const content: ContentBlock[] = merged.additionalContext.map(text => ({ type: 'text', text })) @@ -132,7 +149,7 @@ export function apply(ctx: Context, config: Config): void { // SessionStart: emit. Codex passes a plain-stdout hook's output as additionalContext. ctx.on('agent/session-start', (agent, source) => { - void runPoint('SessionStart', source, { ...base(agent, 'SessionStart', model), source }, { agent }) + void runPoint('SessionStart', source, { ...base(agent, 'SessionStart', model), source }, { agent, plainStdoutAsContext: true }) .then((merged) => { const context = contextFrom(merged) if (context) agent.inject(context.content, { source: context.source }) @@ -143,7 +160,7 @@ export function apply(ctx: Context, config: Config): void { // UserPromptSubmit → PromptDecision. Codex can only BLOCK (no allow/ask). ctx.on('agent/prompt-submit', async (agent, content, _source, next): Promise => { const turn = lastTurn(agent) - const merged = await runPoint('UserPromptSubmit', '', { ...turnBase(agent, 'UserPromptSubmit', model), prompt: blocksToText(content) }, { agent, turn }) + const merged = await runPoint('UserPromptSubmit', '', { ...turnBase(agent, 'UserPromptSubmit', model), prompt: blocksToText(content) }, { agent, turn, plainStdoutAsContext: true }) if (merged.decision === 'deny') return { kind: 'block', reason: merged.reason ?? 'blocked by UserPromptSubmit hook' } const context = contextFrom(merged) if (context) return { kind: 'allow', additionalContext: context } @@ -176,8 +193,12 @@ export function apply(ctx: Context, config: Config): void { // loop-guard (stop_hook_active + a max-consecutive cap) is deferred. ctx.on('agent/turn-continuation', async (agent, turn, _default, next): Promise => { const merged = await runPoint('Stop', '', { ...turnBase(agent, 'Stop', model), stop_hook_active: false, last_assistant_message: null }, { agent, turn }) - if (merged.decision === 'deny' && merged.reason !== undefined) { - return { action: 'continue', reason: { content: [{ type: 'text', text: merged.reason }], source: PLUGIN_SOURCE } } + if (merged.decision === 'deny') { + // A blocking Stop hook forces continuation; a block with no reason (exit 2, + // empty stderr) still forces it — fall back to a generic steering line + // rather than letting the turn stop. + const text = merged.reason ?? 'continue: blocked by Stop hook' + return { action: 'continue', reason: { content: [{ type: 'text', text }], source: PLUGIN_SOURCE } } } return next() }) @@ -226,10 +247,13 @@ function commandOf(args: unknown): string { } function preToolPayload(exec: ToolExecution, model: string): Record { - // Codex hardcodes tool_name to "Bash" and tool_input to { command }. - return { ...turnBase(exec.agent, 'PreToolUse', model), tool_name: 'Bash', tool_input: { command: commandOf(exec.arguments) }, tool_use_id: exec.callId } + // `tool_name` is the REAL tool name (matching the `exec.name` matcher subject); + // a hardcoded constant would disagree with what the matcher tests and make a + // config's tool matcher never fire. `tool_input` keeps Codex's `{ command }` + // shape (its shell payload), derived from the call's `command` arg when present. + return { ...turnBase(exec.agent, 'PreToolUse', model), tool_name: exec.name, tool_input: { command: commandOf(exec.arguments) }, tool_use_id: exec.callId } } function postToolPayload(exec: ToolExecution, result: ToolExecutionResult, model: string): Record { - return { ...turnBase(exec.agent, 'PostToolUse', model), tool_name: 'Bash', tool_input: { command: commandOf(exec.arguments) }, tool_use_id: exec.callId, tool_response: blocksToText(result.content) } + return { ...turnBase(exec.agent, 'PostToolUse', model), tool_name: exec.name, tool_input: { command: commandOf(exec.arguments) }, tool_use_id: exec.callId, tool_response: blocksToText(result.content) } } diff --git a/packages/hooks/hooks-codex/tests/coverage.spec.ts b/packages/hooks/hooks-codex/tests/coverage.spec.ts index 732e0a7c61..d10f2d7df1 100644 --- a/packages/hooks/hooks-codex/tests/coverage.spec.ts +++ b/packages/hooks/hooks-codex/tests/coverage.spec.ts @@ -217,16 +217,21 @@ describe('hooks-codex coverage — decision mapping paths', () => { expect(events(agent).some(e => e.type === 'hook/invoked')).toBe(false) }) - it('a {"continue":false} hook with no decision records decision "stop"', async () => { + it('a {"continue":false} hook is RECORDED as "stop" but does not halt the run (TODO(hook-continue-false))', async () => { + // Honoring `continue:false` is deferred — the seams have no hard-halt + // primitive. Assert the LOG records the halt request AND that the run is not + // actually halted (the tool still runs, the turn completes). const d = dir() hooks(d, { PreToolUse: [{ hooks: [{ type: 'command', command: sh(d, 's.sh', '#!/usr/bin/env bash\necho \'{"continue":false,"stopReason":"halt"}\'\n') }] }] }) const adapter = new MockAdapter([toolCallResponse('c1', 'Bash', { command: 'x' }), textResponse('done')]) const ctx = await harness(join(d, 'hooks.json'), adapter) - ctx.tools.register(defineTool({ name: 'Bash', description: 'b', parameters: { command: { type: 'string' } }, async execute() { return [{ type: 'text', text: 'ok' }] } })) + let ran = false + ctx.tools.register(defineTool({ name: 'Bash', description: 'b', parameters: { command: { type: 'string' } }, async execute() { ran = true; return [{ type: 'text', text: 'ok' }] } })) const agent = ctx.agentLoop.create(AgentId('a1'), { model: 'mock' }) agent.send([{ type: 'text', text: 'go' }]); await waitForIdle(ctx, agent) const res = events(agent).find(e => e.type === 'hook/result') - expect(res?.type === 'hook/result' && res.data.decision).toBe('stop') + expect(res?.type === 'hook/result' && res.data.decision).toBe('stop') // recorded + expect(ran).toBe(true) // NOT honored: the tool still ran (halt is deferred) }) it('PreToolUse deny with EMPTY stderr uses the default reason (?? right arm)', async () => { @@ -305,4 +310,85 @@ describe('hooks-codex coverage — decision mapping paths', () => { const res = events(agent).find(e => e.type === 'hook/result') expect(res?.type === 'hook/result' && 'exitCode' in res.data).toBe(false) }) + + it('a blocking Stop hook with EMPTY stderr still forces continuation (no reason required)', async () => { + // Regression: an exit-2 Stop hook with no stderr yields decision 'deny' + + // reason undefined; the turn must STILL force-continue, not silently stop. + const d = dir() + const marker = join(d, 'fired') + hooks(d, { Stop: [{ hooks: [{ type: 'command', command: sh(d, 's.sh', `#!/usr/bin/env bash\nif [ -e "${marker}" ]; then exit 0; fi\ntouch "${marker}"\nexit 2\n`) }] }] }) + const adapter = new MockAdapter([textResponse('one'), textResponse('two')]) + const ctx = await harness(join(d, 'hooks.json'), adapter) + const agent = ctx.agentLoop.create(AgentId('a1'), { model: 'mock' }) + agent.send([{ type: 'text', text: 'go' }]); await waitForIdle(ctx, agent) + expect(adapter.requests).toHaveLength(2) // empty-reason block forced continuation + expect(JSON.stringify(adapter.requests[1]!.messages)).toContain('blocked by Stop hook') + }) + + it('a clean UserPromptSubmit hook that prints PLAIN stdout injects it as context', async () => { + // Codex feeds a SessionStart/UserPromptSubmit hook's PLAIN (non-JSON) stdout + // as additionalContext (unlike CC, which needs a JSON hookSpecificOutput). + const d = dir() + hooks(d, { UserPromptSubmit: [{ hooks: [{ type: 'command', command: sh(d, 'ctx.sh', '#!/usr/bin/env bash\necho "extra guidance from a plain hook"\nexit 0\n') }] }] }) + const adapter = new MockAdapter([textResponse('ok')]) + const ctx = await harness(join(d, 'hooks.json'), adapter) + const agent = ctx.agentLoop.create(AgentId('a1'), { model: 'mock' }) + agent.send([{ type: 'text', text: 'go' }]); await waitForIdle(ctx, agent) + expect(JSON.stringify(adapter.requests[0]!.messages)).toContain('extra guidance from a plain hook') + }) + + it('a clean SessionStart hook that prints PLAIN stdout injects it (not JSON)', async () => { + const d = dir() + hooks(d, { SessionStart: [{ hooks: [{ type: 'command', command: sh(d, 'ss.sh', '#!/usr/bin/env bash\necho "session preamble"\nexit 0\n') }] }] }) + const adapter = new MockAdapter([textResponse('ok')]) + const ctx = await harness(join(d, 'hooks.json'), adapter) + const agent = ctx.agentLoop.create(AgentId('a1'), { model: 'mock' }) + await new Promise(r => setTimeout(r, 60)) + agent.send([{ type: 'text', text: 'go' }]); await waitForIdle(ctx, agent) + expect(JSON.stringify(adapter.requests[0]!.messages)).toContain('session preamble') + }) + + it('a clean hook that prints JSON is NOT injected as prose (plain-stdout gate)', async () => { + // A structured (JSON) stdout must go through the hookSpecificOutput path, not + // be dumped verbatim as context — the `!startsWith('{')` gate guards this. + const d = dir() + hooks(d, { UserPromptSubmit: [{ hooks: [{ type: 'command', command: sh(d, 'j.sh', '#!/usr/bin/env bash\necho \'{"unrelated":"json"}\'\nexit 0\n') }] }] }) + const adapter = new MockAdapter([textResponse('ok')]) + const ctx = await harness(join(d, 'hooks.json'), adapter) + const agent = ctx.agentLoop.create(AgentId('a1'), { model: 'mock' }) + agent.send([{ type: 'text', text: 'go' }]); await waitForIdle(ctx, agent) + expect(JSON.stringify(adapter.requests[0]!.messages)).not.toContain('unrelated') + }) + + it('the PreToolUse payload carries the REAL tool name (matches the matcher subject)', async () => { + // Regression: the payload once hardcoded tool_name "Bash", disagreeing with + // the exec.name matcher subject — a config matcher on the real name would + // then never fire. Capture the payload and assert tool_name === the real name. + const d = dir() + const cap = join(d, 'payload') + hooks(d, { PreToolUse: [{ hooks: [{ type: 'command', command: sh(d, 'cap.sh', `#!/usr/bin/env bash\ncat > "${cap}"\nexit 0\n`) }] }] }) + const adapter = new MockAdapter([toolCallResponse('c1', 'shell', { command: 'ls' }), textResponse('done')]) + const ctx = await harness(join(d, 'hooks.json'), adapter) + ctx.tools.register(defineTool({ name: 'shell', description: 'b', parameters: { command: { type: 'string' } }, async execute() { return [{ type: 'text', text: 'ok' }] } })) + const agent = ctx.agentLoop.create(AgentId('a1'), { model: 'mock' }) + agent.send([{ type: 'text', text: 'go' }]); await waitForIdle(ctx, agent) + const payload = JSON.parse(readFileSync(cap, 'utf8')) as { tool_name: string; tool_input: { command: string } } + expect(payload.tool_name).toBe('shell') + expect(payload.tool_input.command).toBe('ls') + }) + + it('a Codex matcher on the REAL tool name fires (matcher subject === payload tool_name)', async () => { + // A regex matcher matching the real tool name must select the hook — proving + // the matcher subject and the payload tool_name agree. + const d = dir() + hooks(d, { PreToolUse: [{ matcher: 'shell', hooks: [{ type: 'command', command: sh(d, 'd.sh', '#!/usr/bin/env bash\nexit 2\n') }] }] }) + const adapter = new MockAdapter([toolCallResponse('c1', 'shell', { command: 'ls' }), textResponse('done')]) + const ctx = await harness(join(d, 'hooks.json'), adapter) + let ran = false + ctx.tools.register(defineTool({ name: 'shell', description: 'b', parameters: { command: { type: 'string' } }, async execute() { ran = true; return [{ type: 'text', text: 'ok' }] } })) + const agent = ctx.agentLoop.create(AgentId('a1'), { model: 'mock' }) + agent.send([{ type: 'text', text: 'go' }]); await waitForIdle(ctx, agent) + expect(ran).toBe(false) // the matcher fired → the hook denied the tool + expect(events(agent).some(e => e.type === 'hook/invoked' && e.data.point === 'PreToolUse')).toBe(true) + }) })