feat(hooks): PostModelCall telemetry hook + SessionEnd usage totals#67
Conversation
…to main) Squashed rebase of feat/hooks-manager (fe319a2) onto origin/main (8edae63). Conflicts resolved: - async-params/call-model: keep both allowFinalResponse and hooks options - model-result: runToolWithHooks now propagates executeTool's null (HITL pause); executeToolRound/processApprovalDecisions handle paused calls alongside parse_error/hook_blocked outcomes; broadcastToolResult calls updated for the new (id, source, result) signature; executeToolsIfNeeded merges the Stop-hook/forceResume/SessionEnd machinery with main's stoppedByStopWhen + allowFinalResponse + HITL-pause flow
The hooks integration made handleApprovalCheck pre-execute auto-approve tools on every response and return false, after which the main loop ran the same calls again via executeToolRound (and the round-0 + first-loop double approval check made it 3x total) — a regression for all users, hooks or not. - handleApprovalCheck early-returns when nothing needs an approval gate - PermissionRequest 'allow' defers execution to the normal tool round - PermissionRequest 'deny' records the call id in hookDeniedCalls; the tool round synthesizes a rejected output instead of executing - executeAutoApproveTools only runs when actually pausing for approval (results persisted as unsent for the resume path, as before) - new loop-level regression tests pin exactly-once execution for: plain auto tools (no hooks), auto tools with hooks, hook-allowed gated tools, and hook-denied gated tools
Engine correctness:
- executeHandlerChain: matcher/filter evaluation now runs under the same
error policy as handlers -- a throwing user-supplied filter or function
matcher is logged and the entry skipped in non-strict mode instead of
rejecting the whole emit (addresses Devin review comment on
hooks-emit.ts:119)
- HooksManager.emit: payload validation success now feeds parsed.data into
the chain, so custom hooks with .transform()/.default()/.coerce see the
schema OUTPUT type that zodInfer promises
- EmitResult gains a 'mutated' flag set only when a handler actually piped
a mutation; model-result now uses it instead of reference comparisons
(payload cloning by validation would false-positive those)
- isPlainMutableObject requires a true plain-object prototype, so Date/
Map/class-instance custom payloads pass through un-mangled instead of
being spread into {}
Contract fixes:
- asyncTimeout now aborts the emit's signal on expiry (cooperative
cancellation) and warns; docs updated to state work cannot be forcibly
cancelled -- handlers must observe context.signal
- the emit's AbortController stays registered until detached fire-and-
forget work settles, so abortInflight() reaches async handlers that
outlive the emit
- session teardown (finally block) is wrapped in try/catch so a throwing
SessionEnd handler can never mask the original error (addresses Devin
review comment on model-result.ts finally block); drain() now runs
unconditionally so the approval-resume path (which skips SessionStart)
still awaits fire-and-forget hook work
- SessionEnd is emitted (once, via emitSessionEndOnce) on the no-tools
stream paths (getTextStream etc.), which previously fired SessionStart
with no matching SessionEnd
- custom hooks with result: z.void() skip result validation by schema
shape, matching built-in void hooks (was a hard-coded name list)
- PermissionRequest riskLevel accounts for call-level requireApproval
functions; raw-string (JSON parse failure) arguments fail closed to
ask_user before the hook fires
- matchesTool coerces function-matcher returns to boolean
- empty-string custom hook names are rejected at construction
- off() deletes empty entry lists from the map
- MUTATION_FIELD_MAP/BLOCK_HOOKS/BLOCK_FIELDS are frozen/readonly
API surface trim:
- index.ts no longer exports matchesTool, resolveHooks, BUILT_IN_HOOKS,
BUILT_IN_HOOK_NAMES, or the raw Zod schema objects (mutable, semver
surface); adds isAsyncOutput next to the AsyncOutput type
Tests:
- hooks-contract-fixes.test.ts: parsed.data piping, mutated flag,
void-schema custom hooks, non-plain payloads, asyncTimeout/drain under
fake timers, abortInflight reaching detached work
- hooks-session-lifecycle.test.ts: end-to-end SessionStart config,
Start/End pairing on getText and getTextStream (no tools), SessionEnd
reason max_turns, teardown error masking, inline-config e2e with
matchers
- forceResume cap tests now drive the real executeToolsIfNeeded loop
instead of reimplementing the cap locally
- adversarial tests updated from bug-documenting to regression-pinning
(matcher coercion, filter error policy, empty hook name)
- afterEach(vi.restoreAllMocks) added to hook test files using inline
console spies
…ks-manager subpath export - README: new 'Lifecycle Hooks' feature section covering both usage modes (inline config and HooksManager), all 8 built-in hooks with their result fields and firing semantics, handler chain semantics (matchers, filters, mutation piping, short-circuit, error policy), async fire-and-forget handlers (drain/abortInflight/asyncTimeout/cooperative cancellation via ctx.signal, strict AsyncOutput shape), custom hooks with Zod schema pairs (transform/default honored), and lifecycle pairing guarantees (SessionEnd once per run on every exit path, teardown never masks errors) - hooks-types: document that PostToolUseFailure deliberately does not fire for tools that never ran (PermissionRequest deny, user rejection, PreToolUse block) -- mirrored in the README table - package.json: add @openrouter/agent/hooks-manager subpath export (verified resolvable post-build) - changeset: minor bump for @openrouter/agent describing the feature - test: SessionEnd reason 'user' via state interruption -- covers the last untested reason enum variant
…lomatic complexity The sentrux gate compares complex-function count against a baseline (9); the hooks changes pushed executeHandlerChain and executeToolRound over the cc=15 threshold (11 total). Extract cohesive helpers, no behavior change: - hooks-emit: evaluateEntryGate (matcher/filter under the chain error policy, matcher short-circuits before filter) and classifyHandlerReturn (async-signal / skip / validated-result tagging with schema output substitution) - model-result: executeSingleToolCall (per-call resolution, hook-denial synthesis, preliminary-result wiring, runToolWithHooks dispatch) out of executeToolRound's map closure; runStopHook (Stop emit, appendPrompt aggregation, forceResume cap) out of the executeToolsIfNeeded loop, with MAX_FORCE_RESUME_OVERRIDES lifted to module scope sentrux gate: ✓ no degradation (9 complex functions, matching baseline). All 488 unit tests pass unchanged.
Devin review finding: 'hooks' was added to clientOnlyFields in async-params.ts (the async-function resolution path) but not to the parallel manual-destructuring branch in resolveRequestForContext, whose comment requires the two lists to stay in sync. When ModelResult is constructed directly (bypassing callModel, which destructures hooks out) with a non-async request, the HooksManager object leaked into the payload sent to betaResponsesSend. Destructure 'hooks' out alongside the other client-only fields, and add a regression test that asserts the outgoing request carries no 'hooks' field on BOTH resolution paths (non-async destructuring and async clientOnlyFields). Verified the test fails without the fix.
…ceResume semantics 1. isVoidSchema zod-internals fragility: document that _zod.def.type is zod v4's designated introspection surface (chosen over instanceof, which breaks across duplicated module instances), export the function for testing (not re-exported from the package index), and pin the behavior with direct tests -- z.void() true; object/string/undefined/ unknown/null false -- plus a canary asserting the _zod.def.type surface exists, so a zod upgrade that restructures internals fails loudly instead of silently re-enabling result validation on void hooks. 2. Bare forceResume burns the cap by design: document on StopResult (and in the README) that forceResume alone changes no state, so the stop condition typically re-fires immediately and the 3-override cap is the backstop; pair with appendPrompt or external state the stop condition observes. The cap is deliberately >1 so handlers coordinating external async state get a few iterations. 3. Blocked-tools-count-as-progress: document the choice at the reset site (model receives block/denial feedback = observable forward motion; each reset costs a full model round trip so no hot spin; stopWhen still bounds the run) and pin it with a test driving the real loop with a PreToolUse hook that blocks every call -- the counter resets exactly like a round of real executions, and the tool body never runs.
Perry review blocker: the five no-tools streaming getters (getFullResponsesStream, getTextStream, getItemsStream, getReasoningStream, getToolStream) only called finishHooksSessionForStream() on the happy path. If initStream() threw after SessionStart (e.g. the initial API call fails) or the stream iteration itself threw mid-flight, SessionEnd never fired and drain() never ran, breaking the pair-exactly-once guarantee. - New initStreamGuarded(): wraps initStream() + the 'Stream not initialized' guard; on throw, tears the hook session down with reason 'error' before re-throwing. - Each no-tools streaming block now runs its iteration in try/finally, calling finishHooksSessionForStream with 'error' when the stream threw and 'complete' otherwise. - finishHooksSessionForStream accepts a reason instead of hardcoding 'complete'. Teardown stays emit-once, so the tools path (executeToolsIfNeeded's own finally) is unaffected. Regression tests: initStream failure after SessionStart, and a mid-stream network error, both assert SessionStart/SessionEnd pair exactly once with reason 'error'.
Perry's re-review suggestion: getToolCalls, getToolCallsStream, requiresApproval, getPendingToolCalls, getState, getContextUpdates, and getNewMessagesStream called initStream() raw — if any is the first method called on a ModelResult and the initial API call fails after SessionStart, SessionEnd was skipped (dangling-Start contract violation, same gap as the streaming fix). All seven now route through initStreamGuarded. The guard gains a requireStream option: streaming getters keep the not-initialized invariant; state-inspection methods pass requireStream:false because they are legitimately callable on paused resumes (awaiting_hitl / awaiting_approval) where initStream returns early with neither a stream nor a finalResponse. New test sweeps all seven entry points: init failure -> exactly one SessionStart, one SessionEnd(reason 'error'). Verified red without the src change.
Adds the per-model-call telemetry primitive on top of the lifecycle hook system: PostModelCall fires once per completed model response on every request the loop makes (initial, tool_round follow-ups, empty-final retry, allowFinalResponse final turn, approval resume), carrying responseId (OpenRouter generation id), model, durationMs, turnType, turnNumber, and a normalized usage block (tokens, cached/reasoning details, cost when reported). SessionEnd gains an optional totalUsage aggregate. The initial/resume dispatch is parked (pendingModelCall) because its response materializes later via stream consumption; emit happens at the first materialization site (getInitialResponse, non-streaming branch, getToolCalls, or no-tools stream teardown before SessionEnd). Failed streams emit nothing: there is no completed response to report.
- BUILT_IN_HOOKS keyed by the HookName enum object and typed Record<HookName, HookDefinition> so a new HookName without schemas is a compile error; drop the dead VOID_RESULT_HOOKS name list (superseded by isVoidSchema shape detection) - isAsyncOutput now validates via a strict zod schema (literal true + optional work/asyncTimeout, foreign keys fail) instead of manual key traversal - BuiltInHookDefinitions void results replaced with undefined; removes the noConfusingVoidType suppressions and the InlineHookConfig conditional - emit() payload typed as the schema INPUT (AllHooksIn) while handlers and finalPayload keep the OUTPUT type, so transform/default custom hooks are correctly typed on both sides -- removes the emit-site casts in tests - tool-execution-once: fully-typed OpenResponsesResult fixtures and GetResponseOptions config, requireApproval as a fixture option; single remaining cast is the documented private-internals seam - hooks-resolve-adversarial: @ts-expect-error for deliberate misuse instead of as-casts - void-schema pinning tests moved to dedicated hooks-void-schema.test.ts; canary parses zod internals with a schema instead of an inline cast
…unify behavior registry Four structural improvements, -89 net lines, no observable behavior change except the documented sessionId move: 1. Payload/result types derived from Zod schemas (Readonly<z.infer<...>>), co-located in hooks-schemas.ts and re-exported type-only from hooks-types.ts (public import surface unchanged). Type/runtime drift -- the bug class this PR already fixed once (parsed.data divergence) -- is now structurally impossible. Built-ins use the same derivation as custom hooks (zodInfer). 2. Consumers trust EmitResult<R>: the invariant 'every entry in results passed the result schema' is documented on EmitResult.results, and the four model-result consumer sites (PreToolUse block reason, Stop forceResume/appendPrompt, PermissionRequest decision, UserPromptSubmit reject) read fields directly instead of hand re-narrowing (~30 lines). 3. context.sessionId is the single source of session identity: sessionId removed from all 8 payload schemas; payloadHasSessionId typeguard and payload-over-manager precedence rule deleted; 7x sessionId plumbing lines in model-result gone. The approval-resume path (which bypasses SessionStart but still fires tool hooks) now primes setSessionId. Precedence tests rewritten to pin the new contract, incl. an adversarial case proving a smuggled payload sessionId is stripped, not honored. 4. HOOK_BEHAVIOR table (keyed by HookName, typed Partial<Record<...>>) replaces MUTATION_FIELD_MAP + BLOCK_HOOKS + BLOCK_FIELDS -- BLOCK_HOOKS was fully redundant with BLOCK_FIELDS' key set. A hook's mutation and block semantics are one declaration; typo'd names are compile errors. Folded in: EntryRegistration (field-for-field HookEntry duplicate) deleted; AllHooks/AllHooksIn twin mapped types merged into one table with payload/payloadIn/result fields, removing the K & keyof intersection wart.
The schema-derived-types change introduced a two-way import (hooks-schemas needed HookName/HookDefinition from hooks-types; hooks-types re-exported the derived payload types from hooks-schemas), tripping the structural gate's cycle check. HookName, HookDefinition, and HookRegistry now live in hooks-schemas.ts next to the registry they key; hooks-types re-exports them, so the public import surface (and the deep './hooks-types.js' imports across src and tests) are unchanged. Dependency is now strictly one-directional: hooks-types -> hooks-schemas.
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 PostModelCall not emitted on getToolCallsStream-only consumption
If a caller uses only getToolCallsStream() (line 3825) without ever calling getText(), getResponse(), or another method that triggers executeToolsIfNeeded, the pendingModelCall parked in initStream is never emitted. This method uses initStream() (not initStreamGuarded()) and has no finishHooksSessionForStream teardown. The same applies to getNewMessagesStream() at line 3632. However, both methods eventually call executeToolsIfNeeded() when tools are present, which handles telemetry via getInitialResponse. The gap only exists for the no-tools case on these two methods — a pre-existing pattern that predates this PR. The practical impact is low since these methods are rarely the sole consumption point without tools.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Acknowledged — the no-tools gap on getToolCallsStream()/getNewMessagesStream() as sole consumption points predates this PR (those paths had no teardown site at all). getToolCallsStream without tools is a degenerate call (nothing to stream), so leaving it to the existing pattern; happy to add teardown there in a follow-up if it bites.
| if (this.reusableStream) { | ||
| return consumeStreamForCompletion(this.reusableStream); | ||
| const response = await consumeStreamForCompletion(this.reusableStream); | ||
| await this.emitPendingModelCallOnce(response); | ||
| return response; |
There was a problem hiding this comment.
🔍 Incomplete (response.incomplete) responses still emit PostModelCall
The PR description states "Failed/incomplete streams emit nothing." In getInitialResponse (packages/agent/src/lib/model-result.ts:694-697) the emit happens after consumeStreamForCompletion returns. That helper returns the response object for a response.incomplete event (packages/agent/src/lib/stream-transformers.ts:654-657) rather than throwing, so a stream that terminates with a response.incomplete event WILL emit a PostModelCall. Only truly errored/abnormal streams (no completion event, or a response.failed event) emit nothing. This is likely fine — an incomplete response is still a materialized response with a real generation id and usage — but it's a subtle mismatch with the description's wording that a reviewer may want to confirm.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Correct, and intentional — a response.incomplete response is materialized with a real generation id and consumed tokens, so it should produce a span. Fixed the wording in 2e8ef34: README and the teardown comment now say failed/errored streams emit nothing, while response.incomplete (e.g. max_output_tokens truncation) does emit.
…ency-safe Devin flagged that HooksManager.setSessionId is a single mutable field: two concurrent callModel runs sharing one manager instance (a pattern the README encourages) would clobber each other's context.sessionId — handlers in the earlier run observe the later run's id. - emit() now accepts an optional per-emit sessionId in its emit context, overriding the manager-level default for that emit only - the engine threads this.currentState.id into every lifecycle emit via a new hookEmitContext() helper (all 8 hook emit sites), so runs sharing a manager never leak identity across runs - setSessionId() stays as the default for direct emit() callers (custom hooks on a shared manager), with the last-writer-wins caveat documented on the method, the context type, and the README Tests: per-emit override precedence in hooks-manager.test.ts; a two-run shared-manager regression in model-result-hooks.test.ts priming the manager with run B's id and asserting run A's tool hooks still see run A's state id.
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
- resolve conflicts: keep PostModelCall additions on top of merged hooks base - finishHooksSessionForStream: isolate parked-telemetry materialization in its own try/catch so a telemetry failure (stream buffer without a completion event, or a throwing strict-mode handler) cannot skip SessionEnd or drain - clarify failed vs incomplete semantics in README + code comment: response.incomplete responses DO emit PostModelCall (materialized response with generation id + usage); only failed/errored streams emit nothing - regression test: deltas-only stream still gets SessionEnd, no PostModelCall
| if (this.pendingModelCall) { | ||
| if (this.finalResponse) { | ||
| await this.emitPendingModelCallOnce(this.finalResponse); | ||
| } else if (this.reusableStream?.isComplete) { | ||
| await this.emitPendingModelCallOnce( | ||
| await consumeStreamForCompletion(this.reusableStream), | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🔍 Early stream break yields SessionEnd(complete) but no PostModelCall
On the no-tools streaming paths (getTextStream/getFullResponsesStream), if a consumer breaks out of the loop before the stream fully completes, the finally runs finishHooksSessionForStream('complete') (packages/agent/src/lib/model-result.ts:3402-3431, 3363-3395). Inside teardown (model-result.ts:1305-1313), the parked initial PostModelCall is only emitted when this.reusableStream?.isComplete is true. On an early break the source is not fully drained, so isComplete is false and no PostModelCall fires, yet SessionEnd still reports reason: 'complete' with no totalUsage. This matches the documented "incomplete streams emit nothing" behavior, but the complete reason paired with a truncated/unmeasured model call is arguably misleading for telemetry consumers. Worth confirming this edge case is intended.
Was this helpful? React with 👍 or 👎 to provide feedback.
| // Park PostModelCall telemetry for the resume dispatch (see initStream). | ||
| this.pendingModelCall = { | ||
| startedAt: performance.now(), | ||
| turnType: 'resume', | ||
| turnNumber: turnContext.numberOfTurns, | ||
| }; |
There was a problem hiding this comment.
🔍 Parked resume PostModelCall can be dropped on a paused resume
continueWithUnsentResults parks a pendingModelCall with turnType 'resume' (packages/agent/src/lib/model-result.ts:2911-2916). In executeToolsIfNeeded, if the resume leaves the conversation paused (awaiting_approval/awaiting_hitl), the loop returns early at model-result.ts:2965-2971 before getInitialResponse() is reached, so the parked resume telemetry is never materialized/emitted (SessionEnd still fires in the finally). This is only telemetry loss (no crash), and only on the specific paused-resume-after-dispatch path, but if the resume dispatch actually produced a completed response this represents a missing PostModelCall for a real model call. Confirm whether a resume dispatch can complete a response and still take the paused-return branch.
Was this helpful? React with 👍 or 👎 to provide feedback.
| function extractModelCallUsage(usage: models.Usage | null | undefined): ModelCallUsage | undefined { | ||
| if (!usage) { | ||
| return undefined; | ||
| } | ||
| return { | ||
| inputTokens: usage.inputTokens, | ||
| outputTokens: usage.outputTokens, | ||
| totalTokens: usage.totalTokens, | ||
| cachedTokens: usage.inputTokensDetails?.cachedTokens ?? 0, | ||
| reasoningTokens: usage.outputTokensDetails?.reasoningTokens ?? 0, | ||
| ...(usage.cost !== null && | ||
| usage.cost !== undefined && { | ||
| cost: usage.cost, | ||
| }), | ||
| }; | ||
| } |
There was a problem hiding this comment.
🔍 extractModelCallUsage trusts SDK token fields as defined numbers
extractModelCallUsage (packages/agent/src/lib/model-result.ts:136-151) copies usage.inputTokens/outputTokens/totalTokens directly (only cachedTokens/reasoningTokens get ?? 0 fallbacks). If the server ever returns a usage object with those top-level counts missing/undefined, sessionUsage.inputTokens += undefined would poison the running aggregate with NaN, and the PostModelCallPayloadSchema (which requires number) would reject the emit (warn-and-skip in default mode). Current tests always supply complete usage blocks, so this is not exercised; whether it can occur depends on the SDK Usage type guaranteeing these fields. Low risk but worth a defensive ?? 0.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Summary
Stacked on #7 (base:
feat/hooks-manager) — do not merge before it.Adds the per-model-call telemetry primitive the hooks system was missing, motivated by benchmark/tracing consumers (openbench-style harnesses) that need one span per model call with the OpenRouter generation id, latency, and cost.
onTurnEndstructurally cannot provide this: it only fires on tool-execution turns, so the initial request, the empty-final retry, theallowFinalResponsefinal turn, and approval-resume requests are invisible to it.PostModelCall(new built-in hook)Fires once per completed model response, on every request the loop makes — all five
betaResponsesSendsites:turnTypeinitStreaminitial requestinitialcontinueWithUnsentResults(approval/HITL resume)resumemakeFollowupRequest(tool rounds)tool_roundmakeFinalResponseRequest(allowFinalResponse)finalretryCurrentRequest(empty-final retry)retryPayload:
sessionId,responseId(OpenRouter generation id, deep-linkable),model,durationMs(dispatch → fully materialized response, including stream consumption),turnType,turnNumber, and a normalizedusageblock (inputTokens,outputTokens,totalTokens,cachedTokens,reasoningTokens,cost?) when the server reported usage accounting. Purely observational — void result, no mutation/blocking.SessionEnd.totalUsage(additive)When at least one model call completed,
SessionEndnow carriestotalUsage:modelCalls+ summed usage fields, withcostpresent when any call reported one. Gives session-level budget accounting without the consumer having to fold spans itself.Mechanics worth reviewing
initStream, but its response only materializes when the stream is consumed. The dispatch time + turn labeling are parked (pendingModelCall) and the emit happens at the first materialization site:getInitialResponse(tools path), the non-streaming branch,getToolCalls, or — for no-tools streaming getters — session teardown (finishHooksSessionForStream, beforeSessionEnd), reading the completed response from theReusableReadableStreamretained buffer (newisCompleteaccessor; a completed stream replays from buffer without touching the source).PostModelCall,SessionEnd(reason: 'error'), no-totalUsagebehavior is pinned by a test.makeFollowupRequest/makeFinalResponseRequest/retryCurrentRequestare now onematerializeResponsehelper; each site emits with its ownturnType.costis only present when the request has usage accounting enabled server-side; consumers that need exact spend should enable it on their requests.Test plan
hooks-post-model-call.test.ts: tool-loop emit count + turn labels, usage mapping (cached/reasoning/cost), usage-less responses (counted inmodelCalls, nocost), no-toolsgetTextStream()path,SessionEnd.totalUsageaggregation across calls, failed-request behavior, and a genuinely streaming initial response (response.completedevent)pnpm buildclean; Biome clean