diff --git a/.changeset/pretooluse-abort-fail-closed.md b/.changeset/pretooluse-abort-fail-closed.md new file mode 100644 index 00000000..67454498 --- /dev/null +++ b/.changeset/pretooluse-abort-fail-closed.md @@ -0,0 +1,5 @@ +--- +"@openrouter/agent": patch +--- + +Fail closed when a blocking hook chain is aborted. If a `PreToolUse` or `UserPromptSubmit` chain stops early because its signal was aborted (an `asyncTimeout` on detached work expiring, or `abortInflight()`), the emit result now reports `blocked: true` instead of `blocked: false`, so handlers that never ran (for example a later deny guard) can no longer be skipped into an approval. diff --git a/packages/agent/src/lib/hooks-emit.ts b/packages/agent/src/lib/hooks-emit.ts index a3c6eaef..7a050717 100644 --- a/packages/agent/src/lib/hooks-emit.ts +++ b/packages/agent/src/lib/hooks-emit.ts @@ -65,7 +65,8 @@ function isPlainMutableObject(value: unknown): value is Record * - Cooperative abort via `context.signal`: the chain checks * `signal.aborted` between handlers and bails out when set so * `abortInflight()` has a deterministic effect on the chain itself (not just - * on handlers that happen to consult the signal). + * on handlers that happen to consult the signal). For hooks with a block + * field, bailing out before every handler ran reports `blocked: true`. * * Payload isolation: the initial payload is shallow-cloned before entering the * chain so mutation piping (via {@link HOOK_BEHAVIOR}) doesn't mutate the @@ -109,6 +110,12 @@ export async function executeHandlerChain( // Checked before each handler so synchronous chains stop promptly instead // of running to completion with only advisory notice to each handler. if (context.signal.aborted) { + // A hook that can block (PreToolUse, UserPromptSubmit) fails closed: + // the handlers left unrun may include the guard that would have + // blocked, so an aborted chain must not read as an approval. + if (behavior?.blockField) { + blocked = true; + } break; } diff --git a/packages/agent/src/lib/hooks-types.ts b/packages/agent/src/lib/hooks-types.ts index 6cfb99a7..55a95f4e 100644 --- a/packages/agent/src/lib/hooks-types.ts +++ b/packages/agent/src/lib/hooks-types.ts @@ -120,7 +120,11 @@ export interface EmitResult { readonly pending: Promise[]; /** The payload after all mutation piping has been applied. */ readonly finalPayload: P; - /** True if any handler triggered a block/reject short-circuit. */ + /** + * True if any handler triggered a block/reject short-circuit, or if the + * chain of a blocking hook (PreToolUse, UserPromptSubmit) was aborted + * before every handler ran. + */ readonly blocked: boolean; /** * True if any handler's result actually piped a mutation into the payload diff --git a/packages/agent/tests/unit/hooks-contract-fixes.test.ts b/packages/agent/tests/unit/hooks-contract-fixes.test.ts index 6fde4667..e9d5885e 100644 --- a/packages/agent/tests/unit/hooks-contract-fixes.test.ts +++ b/packages/agent/tests/unit/hooks-contract-fixes.test.ts @@ -382,3 +382,140 @@ describe('asyncTimeout and abortInflight semantics', () => { expect(signal?.aborted).toBe(false); }); }); + +describe('blocking hooks fail closed when the chain is aborted', () => { + function registerTimeoutThenGuard(manager: HooksManager, guard: () => void) { + manager.on('PreToolUse', { + handler: () => ({ + async: true as const, + work: new Promise(() => {}), + asyncTimeout: 10, + }), + }); + manager.on('PreToolUse', { + handler: () => new Promise((resolve) => setTimeout(resolve, 50)), + }); + manager.on('PreToolUse', { + handler: () => { + guard(); + return { + block: 'policy forbids this call', + }; + }, + }); + } + + it('reports PreToolUse as blocked when a detached timeout skips a later guard', async () => { + vi.useFakeTimers(); + vi.spyOn(console, 'warn').mockImplementation(() => {}); + const manager = new HooksManager(); + const guard = vi.fn(); + registerTimeoutThenGuard(manager, guard); + + const pending = manager.emit( + 'PreToolUse', + { + toolName: 't', + toolInput: {}, + }, + { + toolName: 't', + }, + ); + await vi.advanceTimersByTimeAsync(60); + const result = await pending; + + expect(guard).not.toHaveBeenCalled(); + expect(result.blocked).toBe(true); + }); + + it('reports PreToolUse as blocked when abortInflight() fires mid-chain', async () => { + const manager = new HooksManager(); + const guard = vi.fn(); + manager.on('PreToolUse', { + handler: () => { + manager.abortInflight('shutdown'); + }, + }); + manager.on('PreToolUse', { + handler: () => { + guard(); + return { + block: 'policy forbids this call', + }; + }, + }); + + const result = await manager.emit( + 'PreToolUse', + { + toolName: 't', + toolInput: {}, + }, + { + toolName: 't', + }, + ); + + expect(guard).not.toHaveBeenCalled(); + expect(result.blocked).toBe(true); + }); + + it('still runs every PreToolUse handler when detached work settles in time', async () => { + const manager = new HooksManager(); + const guard = vi.fn(); + manager.on('PreToolUse', { + handler: () => ({ + async: true as const, + work: Promise.resolve(), + asyncTimeout: 10, + }), + }); + manager.on('PreToolUse', { + handler: () => { + guard(); + }, + }); + + const result = await manager.emit( + 'PreToolUse', + { + toolName: 't', + toolInput: {}, + }, + { + toolName: 't', + }, + ); + + expect(guard).toHaveBeenCalledOnce(); + expect(result.blocked).toBe(false); + }); + + it('does not mark non-blocking hooks as blocked when aborted', async () => { + const manager = new HooksManager(); + manager.on('PostToolUse', { + handler: () => { + manager.abortInflight('shutdown'); + }, + }); + manager.on('PostToolUse', { + handler: () => {}, + }); + + const result = await manager.emit( + 'PostToolUse', + { + toolName: 't', + toolInput: {}, + toolOutput: null, + durationMs: 1, + }, + { + toolName: 't', + }, + ); + + expect(result.blocked).toBe(false); + }); +});