mirror of
https://github.com/deepseek-ai/deepseek-harness
synced 2026-08-15 21:04:50 +00:00
fix(hooks-codex): gate plain-stdout→context on a clean exit; harden HMR + absence tests
Round-2 Codex review of the round-1 fixes: - (A) The Codex plain-stdout→additionalContext fold (F1) was not gated on exit code, so a NON-clean hook's stdout still injected: a SessionStart `echo stale; exit 2` (an emit — cannot block) wrongly injected "stale", and a UserPromptSubmit `exit 1` (non-blocking error → falls through to context) did too. Gate the fold on `output.exitCode === 0`, matching the codec's own structured-stdout rule. Guard tests for both paths, proven red without the gate. - (B) The Codex "SessionStart no-context no-op" absence test was unsound (a completed turn doesn't prove the detached hook finished). It now touches a marker and waitFor()s it before asserting no context. - (B) Both HMR tests used a no-op `true` hook, so a leaked listener would still pass. They now use a BLOCKING (exit 2) UserPromptSubmit hook and assert the post-dispose turn is NOT blocked and logs no hook/invoked — a leaked listener fails loudly.
This commit is contained in:
@@ -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', () => {
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
@@ -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', () => {
|
||||
|
||||
@@ -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') }] }] })
|
||||
|
||||
Reference in New Issue
Block a user