feat: add HooksManager lifecycle hook system to callModel#7
feat: add HooksManager lifecycle hook system to callModel#7mattapperson wants to merge 10 commits intomainfrom
Conversation
Implements an extensible hook system for callModel inspired by the Claude Agent SDK hooks pattern. Supports inline config for quick declarative usage and a HooksManager class for custom hooks, dynamic registration, and programmatic emit. Built-in hooks: PreToolUse, PostToolUse, PostToolUseFailure, UserPromptSubmit, Stop, PermissionRequest, SessionStart, SessionEnd. Features: tool matchers (string/RegExp/function), filter predicates, mutation piping, short-circuit on block/reject, async fire-and-forget handlers, configurable error handling, and custom hook definitions via Zod schema pairs.
81 edge-case tests covering handler chain execution, manager lifecycle, tool matchers, resolver normalization, and async output detection. Surfaces real issues: filter throws bypass error handling, global RegExp stateful .test(), and function matcher lacking boolean coercion.
test: add adversarial unit tests for hooks system
Resolves conflicts in src/index.ts (keep both hooks exports and new OpenRouter/SDKOptions exports) and src/inner-loop/call-model.ts (keep both resolveHooks and Tool type imports). Adapts hooks files to Biome linter rules introduced on main: import ordering, block statements, and sparse-array/void-type suppressions where the behavior is intentional.
Fixes CI failure on `validate` job by applying Biome's auto-fixes for import sorting in async-params.ts and model-result.ts, and formatting in hooks-matchers.ts, hooks-schemas.ts, model-result.ts, and conversation-state.test.ts.
mattapperson
left a comment
mattapperson
left a comment
There was a problem hiding this comment.
reviewed, no issues found
Integrate the Turborepo monorepo migration (src/ -> packages/agent/src/, tests/ -> packages/agent/tests/, tsconfig.json -> tsconfig.base.json) with the HooksManager lifecycle hook system from this PR. All hooks files (hooks-*.ts) and their tests were relocated under packages/agent/ to match the new workspace layout. Renamed the re-exported SDK HookContext to SDKHookContext in packages/agent/src/index.ts to disambiguate from the lifecycle hook context we already export from lib/hooks-types.ts. The SDK-level fix from ef15761 (normalize hooks constructor option) is preserved intact from main.
Library internals:
- AsyncOutput.work field enables true fire-and-forget; chain tracks
background work with timeout instead of advertising it cosmetically
- HooksManager.emit now validates payloads and handler results against
their Zod schemas; stored customHooks registry drives custom-hook
validation
- MUTATION_FIELD_MAP scoped per-hook so only PreToolUse maps
mutatedInput->toolInput and only UserPromptSubmit maps
mutatedPrompt->prompt; custom hooks opt out by default
- pendingAsync converted to a self-cleaning Set so it no longer leaks
across long sessions
- Per-emit AbortController tracked on the manager; abortInflight()
cancels every in-flight emit's HookContext.signal
- Matcher without toolName now fails closed (skipped) instead of open
- isBlockTriggered rejects empty strings to stay consistent with the
truthy reason search in model-result
- registerEntry hidden behind a module-private symbol + helper so
resolveHooks can still register inline configs without exposing a
back door to user code
model-result integration:
- Fire Pre/Post/PostFailure hooks from every tool-execution path by
routing through a new runToolWithHooks helper (auto, auto-approved,
and manually approved calls are all covered)
- Emit PermissionRequest in handleApprovalCheck; allow/deny/ask_user
decisions gate the approval flow, denied calls synthesize an error
result without executing
- UserPromptSubmit now fires before stateful input-wrapping, handles
both string and structured message-array inputs, and applies
mutations back to the base request
- Cap Stop forceResume at 3 consecutive overrides to prevent infinite
loops when a stop condition keeps firing
- Check .some(forceResume) across all Stop results and honor
appendPrompt by injecting a user message into the next turn
- SessionStart config populated with {hasTools, hasApproval, hasState}
rather than hardcoded undefined
- Swap Date.now() -> performance.now() for durationMs timing
robert-j-y
left a comment
robert-j-y
left a comment
There was a problem hiding this comment.
1. SessionEnd fires on exactly one code path: a completed run that had tools AND made at least one tool call AND didn't throw.
packages/agent/src/lib/model-result.ts:1985-2184 — the execute IIFE has no try/catch/finally (grepped: only two try blocks in the file, at line 1587 and 2758, both unrelated). The only SessionEnd emit is at line 2178, at the very bottom, after four unconditional early returns:
- line 1990: resuming from approval, still pending
- line 2007:
!this.options.tools?.length || !hasToolCalls— a plain chat response with no tool calls never emitsSessionEnd - line 2015: paused for approval
- line 2021: no executable tool calls
Plus any throw in the loop (tool exec, hook emit, state save, followup request) rejects the promise and skips SessionEnd + hooksManager.drain(). SessionEndPayload.reason at hooks-types.ts:177 declares 'user' | 'error' | 'max_turns' | 'complete'; only 'complete' is ever emitted, and then only on the happy-tool path.
Fix: wrap the IIFE body in try { ... } finally { if (this.hooksManager) { await this.hooksManager.emit('SessionEnd', { sessionId, reason }); await this.hooksManager.drain(); } }, and pick reason based on exit path ('complete' on normal end, 'error' in the catch, 'max_turns' when the loop exits via shouldStopExecution without a Stop override).
2. Stop payload reason is hardcoded to 'end_turn' — verified as the only emit site.
model-result.ts:2043-2046:
const stopResult = await this.hooksManager.emit('Stop', {
reason: 'end_turn' as const,
sessionId: this.currentState?.id ?? '',
});Grepped the whole file: one Stop emit site. StopPayload at hooks-types.ts:154 declares 'end_turn' | 'max_tokens' | 'stop_sequence' — the other two are unreachable. Also: shouldStopExecution() fires off arbitrary stopWhen conditions (default stepCountIs(DEFAULT_MAX_STEPS)), which semantically is closer to 'max_turns' than 'end_turn'. The three enum values are Anthropic-style stop reasons and don't cleanly map to this engine's stop causes.
Fix: either derive reason from the actual cause (stop-condition name vs. response finish_reason) and expand the schema to cover step-count/tool-absence cases, or drop the two unreachable variants until they're wired.
3. forceResumeCount is a lifetime counter; comment says "without new progress".
model-result.ts:2031 declares let forceResumeCount = 0; outside the while. Only mutation is forceResumeCount++ at line 2083. Never reset. So a Stop hook that force-resumes at turn 2, runs 50 clean turns, force-resumes at turn 53, runs 50 more, force-resumes at turn 104 — gets cut off on the third call despite 100 turns of real progress in between.
The console.warn at line 2079 asserts "forceResume honored 3 times without new progress", which the code does not enforce.
Fix: either reset forceResumeCount = 0 at the top of each while iteration that doesn't enter the shouldStopExecution() branch, or change the warning text and the comment at 2026-2030 to say "3 times total this session".
mattapperson
left a comment
mattapperson
left a comment
There was a problem hiding this comment.
reviewed, no issues found
…n emit Disambiguates the lifecycle hook context type from the SDK-level HookContext re-exported from @openrouter/sdk. Tightens executeHandlerChain to avoid spread-cloning non-plain payloads, threads hookName into async-work tracking for clearer rejection logs, and prefers payload sessionId over the manager's stored value. Narrows StopPayload.reason to 'max_turns' to match the actual emission site.
mattapperson
left a comment
mattapperson
left a comment
There was a problem hiding this comment.
Adversarial pass surfaced seven issues across the emit engine, matcher layer, and SessionStart/End pairing.
Address seven PR review threads on feat/hooks-manager:
- emit() snapshots its entries list before running the chain, so
handlers that call off()/unsubscribe mid-chain can't shift indices
and silently skip the next handler.
- matchesTool resets `matcher.lastIndex = 0` before .test(), making
/g and /y RegExps idempotent across successive emits.
- isAsyncOutput now requires `async: true` AND no foreign keys. A
handler that returns { async: true, mutatedInput: ... } is treated
as a result, not fire-and-forget, so mutation piping isn't dropped.
- executeToolsIfNeeded guards the SessionEnd emit on a new
sessionStartEmitted flag so an initStream throw before SessionStart
can't produce a dangling SessionEnd for Start/End-paired handlers.
- executeHandlerChain pushes `validation.data` onto results when a
schema is present, so custom hooks using .transform/.default/.catch
see the parsed output rather than the raw input.
- resolveHooks warns and skips non-built-in hook names in inline
config to catch typos like `PretoolUse` vs `PreToolUse`; custom
hooks still require a HooksManager instance.
- Adversarial test for mid-emit on() now asserts the snapshotted
behavior (late handler not invoked) instead of the former
index-drift footgun.
Existing adversarial tests for the old permissive behaviors
(/g drift, arbitrary inline hook names, `async: true` + extras
treated as fire-and-forget) are updated to pin the tighter
invariants.
mattapperson
left a comment
mattapperson
left a comment
There was a problem hiding this comment.
Reviewed the hooks engine and model-result integration; 5 concerns worth a second look on abort semantics, argument normalization, parse-error handling, clone depth, and timer lifetime.
- executeHandlerChain checks context.signal.aborted between handlers so
abortInflight() has a deterministic effect on the chain itself rather
than relying on each handler to consult the signal. An already-aborted
emit skips every handler.
- Added a Node-only unref() on the setTimeout handle backing async-work
tracking so a leaked fire-and-forget hook whose work never settles
can't hold the event loop open for the full 30s timeout window. Guarded
for browsers that return a plain number from setTimeout.
- Clarified the shallow-clone docstring on executeHandlerChain to make
the top-level-only isolation explicit; nested mutations by handlers
still reach the caller, so handlers MUST pipe mutations via the
documented result fields.
- runToolWithHooks rejects raw-string toolCall.arguments (the shape the
parser leaves behind on a JSON.parse failure) before any hook fires
and returns a 'parse_error' result with a prebuilt synthetic error
output. executeToolRound, executeAutoApproveTools, and
processApprovalDecisions all route through the same helper, so every
execution path fails closed consistently.
- runToolWithHooks tracks mutation application explicitly instead of
comparing finalPayload.toolInput by reference to the pre-coercion
arguments. Tools that distinguish "no args" from "{}" no longer see
`{}` when arguments were null/undefined and no handler mutated them.
Tests: +5 new cases (2 abort-propagation, 1 parse_error, 2 mutation
detection). Full suite 404 pass, lint clean, build clean.
mattapperson
left a comment
mattapperson
left a comment
There was a problem hiding this comment.
reviewed, no issues found
2 participants
Summary
callModelinspired by the Claude Agent SDK hooks patternPreToolUse,PostToolUse,PostToolUseFailure,UserPromptSubmit,Stop,PermissionRequest,SessionStart,SessionEndonTurnStart,onTurnEnd,requireApprovalremain unchangedRe-creation of OpenRouterTeam/openrouter-web#16735 on the standalone agent SDK repo.
Test plan
pnpm build) passes cleanlypnpm lint) passes cleanly