fix(hooks): address Codex review — Stop force-continue, Codex tool_name + plain-stdout context, defer continue:false
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.
This commit is contained in:
@@ -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
|
||||
|
||||
|
||||
@@ -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<ContinuationDecision> => {
|
||||
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()
|
||||
})
|
||||
|
||||
@@ -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 () => {
|
||||
|
||||
Reference in New Issue
Block a user