diff --git a/packages/hooks/hooks-claude/tests/bridge.spec.ts b/packages/hooks/hooks-claude/tests/bridge.spec.ts index 0cc4458f52..0b6afd7e4c 100644 --- a/packages/hooks/hooks-claude/tests/bridge.spec.ts +++ b/packages/hooks/hooks-claude/tests/bridge.spec.ts @@ -322,17 +322,29 @@ describe('hooks-claude bridge — load resilience', () => { }) it('disposing the bridge fiber removes its listeners (HMR safety)', async () => { - const dir = writeConfig({ UserPromptSubmit: [{ hooks: [{ type: 'command', command: 'true' }] }] }) + // A BLOCKING UserPromptSubmit hook: if the listener leaked past dispose it + // would veto the prompt (0 model requests) and log a hook/invoked. Build the + // ctx WITHOUT the harness's own bridge mount so this is the ONLY mount, then + // dispose it — a leaked listener fails the test (a no-op `true` hook would + // pass even leaked, so it proved nothing). + const dir = writeConfig({ UserPromptSubmit: [{ hooks: [{ type: 'command', command: 'exit 2' }] }] }) const adapter = new MockAdapter([textResponse('ok')]) - const ctx = await harness(dir, adapter) + const ctx = new Context() + await ctx.plugin(LlmService) + await ctx.plugin(SessionStore) + await ctx.plugin(SystemPrompt) + await ctx.plugin(ToolRegistry) + await ctx.plugin(AgentRegistry) + await ctx.plugin(AgentLoop, { agents: [] }) + await ctx.plugin(LocalBashExecutor, { timeoutMs: 10_000 }) const fiber = await ctx.plugin(HooksClaude, { configPath: join(dir, 'hooks.json') }) await fiber.dispose() - // After disposing this second mount, the FIRST mount's listeners still work, - // but the disposed one contributed none — assert no leaked listener throws. + ctx.llm.registerAdapter(['mock'], adapter) const agent = ctx.agentLoop.create(AgentId('a1'), { model: 'mock' }) agent.send([{ type: 'text', text: 'go' }]) await waitForIdle(ctx, agent) - expect(adapter.requests.length).toBeGreaterThanOrEqual(1) + expect(adapter.requests).toHaveLength(1) // not blocked → the listener is gone + expect(events(agent).some(e => e.type === 'hook/invoked')).toBe(false) // no hook ran }) it('has the namespace-plugin export shape (no stray default) so the Loader keeps name/inject/apply', () => { diff --git a/packages/hooks/hooks-codex/src/index.ts b/packages/hooks/hooks-codex/src/index.ts index e3e2ac020b..3ef1d2806a 100644 --- a/packages/hooks/hooks-codex/src/index.ts +++ b/packages/hooks/hooks-codex/src/index.ts @@ -110,14 +110,18 @@ export function apply(ctx: Context, config: Config): void { // Discard a `hookSpecificOutput` block naming a different event. expectedEventName: point, }, () => performance.now()) - // Codex's SessionStart/UserPromptSubmit treat a clean hook's PLAIN + // 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 + // merge + contextFrom path carry it. Gated exactly like the codec's own + // structured-stdout parse: only on a clean `exitCode === 0` (a non-zero + // exit is an error, not context — an `echo x; exit 2` must not inject + // `x`), only when stdout is non-JSON (`!startsWith('{')` — a structured + // hook's raw JSON is never dumped as prose), and never clobbering an + // explicit additionalContext from a JSON block. + if (opts.plainStdoutAsContext === true && output.exitCode === 0 + && output.additionalContext === undefined && output.stdout.length > 0 && !output.stdout.startsWith('{')) { output.additionalContext = output.stdout } diff --git a/packages/hooks/hooks-codex/tests/bridge.spec.ts b/packages/hooks/hooks-codex/tests/bridge.spec.ts index f62fa4d66c..0148da104e 100644 --- a/packages/hooks/hooks-codex/tests/bridge.spec.ts +++ b/packages/hooks/hooks-codex/tests/bridge.spec.ts @@ -132,9 +132,14 @@ describe('hooks-codex bridge', () => { expect(adapter.requests).toHaveLength(1) }) - it('disposing the bridge fiber is clean (HMR safety)', async () => { + it('disposing the bridge fiber removes its listeners (HMR safety)', async () => { const dir = configDir() - writeHooks(dir, { UserPromptSubmit: [{ hooks: [{ type: 'command', command: 'true' }] }] }) + // A BLOCKING UserPromptSubmit hook: if the listener leaked past dispose, it + // would veto the prompt (0 model requests) and log a hook/invoked. After a + // clean dispose the turn must proceed untouched — this fails loudly on a leak + // (a no-op `true` hook would pass even with a leaked listener). + const deny = script(dir, 'deny.sh', '#!/usr/bin/env bash\nexit 2\n') + writeHooks(dir, { UserPromptSubmit: [{ hooks: [{ type: 'command', command: deny }] }] }) const adapter = new MockAdapter([textResponse('ok')]) const ctx = new Context() await ctx.plugin(LlmService) @@ -150,7 +155,8 @@ describe('hooks-codex bridge', () => { const agent = ctx.agentLoop.create(AgentId('a1'), { model: 'mock' }) agent.send([{ type: 'text', text: 'go' }]) await waitForIdle(ctx, agent) - expect(adapter.requests).toHaveLength(1) + expect(adapter.requests).toHaveLength(1) // not blocked → the listener is gone + expect(events(agent).some(e => e.type === 'hook/invoked')).toBe(false) // no hook ran }) it('has the namespace-plugin export shape (no stray default) so the Loader keeps name/inject/apply', () => { diff --git a/packages/hooks/hooks-codex/tests/coverage.spec.ts b/packages/hooks/hooks-codex/tests/coverage.spec.ts index c32d0b3b91..bc4008ab1e 100644 --- a/packages/hooks/hooks-codex/tests/coverage.spec.ts +++ b/packages/hooks/hooks-codex/tests/coverage.spec.ts @@ -179,12 +179,15 @@ describe('hooks-codex coverage — decision mapping paths', () => { it('SessionStart with no additionalContext is a no-op (contextFrom empty)', async () => { const d = dir() - hooks(d, { SessionStart: [{ hooks: [{ type: 'command', command: sh(d, 's.sh', '#!/usr/bin/env bash\nexit 0\n') }] }] }) + // The hook touches a marker so we can wait for it to ACTUALLY FINISH before + // asserting absence — a completed turn alone would not prove the detached + // session-start hook ran, making the absence check a false pass. + const marker = join(d, 'ss-ran') + hooks(d, { SessionStart: [{ hooks: [{ type: 'command', command: sh(d, 's.sh', `#!/usr/bin/env bash\ntouch "${marker}"\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' }) - // A completed turn proves session-start already ran; the clean no-output hook - // injected nothing, so no context/message exists. + await waitFor(() => existsSync(marker)) // the clean no-output hook has finished agent.send([{ type: 'text', text: 'go' }]); await waitForIdle(ctx, agent) expect(events(agent).some(e => e.type === 'context/message')).toBe(false) }) @@ -347,6 +350,37 @@ describe('hooks-codex coverage — decision mapping paths', () => { expect(JSON.stringify(adapter.requests[0]!.messages)).toContain('extra guidance from a plain hook') }) + it('a NON-clean SessionStart hook (exit 2) does NOT inject its stdout as context', async () => { + // The plain-stdout→context fold is gated on exitCode === 0, matching the + // codec's structured-stdout rule. SessionStart is an EMIT (cannot block), so + // an `echo stale; exit 2` here is the exact case the gate guards: without it, + // the non-clean hook's stdout would wrongly inject "stale". A marker lets us + // wait for the detached hook to finish before asserting absence. + const d = dir() + const marker = join(d, 'ran') + hooks(d, { SessionStart: [{ hooks: [{ type: 'command', command: sh(d, 'b.sh', `#!/usr/bin/env bash\ntouch "${marker}"\necho "stale"\nexit 2\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 waitFor(() => existsSync(marker)) // the exit-2 hook has finished + expect(events(agent).some(e => e.type === 'context/message' + && e.data.content.some(b => b.type === 'text' && b.text.includes('stale')))).toBe(false) + }) + + it('a UserPromptSubmit hook with a non-blocking error exit (1) + stdout does NOT inject it', async () => { + // Exit 1 is a non-blocking error (no decision), so the prompt is NOT blocked + // and the handler falls through to the context path — the gate must still + // suppress the error hook's stdout ("stale" never reaches the model). + const d = dir() + hooks(d, { UserPromptSubmit: [{ hooks: [{ type: 'command', command: sh(d, 'e.sh', '#!/usr/bin/env bash\necho "stale"\nexit 1\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(adapter.requests).toHaveLength(1) // exit 1 is non-blocking → the turn ran + expect(JSON.stringify(adapter.requests[0]!.messages)).not.toContain('stale') + }) + 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') }] }] })