From d3a3ce4d3eac7a02ce1a5e9f9b289956de3a06d3 Mon Sep 17 00:00:00 2001 From: Saad Date: Thu, 22 Oct 2026 23:00:13 +0300 Subject: [PATCH 1/2] test(orchestration): backfill the tool-loop suite and raise its floors The loop's helpers were tested one in six, every loop case drove one tool call per round, and the gates that end a turn were never observed from outside. Fourteen tests now cover all six helpers with the whole truth table of shouldContinueToolLoop; a two-call round running both executors and pairing two tool_use with two tool_result items in order; one suppressing executor among two yielding one suppress_output after the tool_result; two upserts taking sequential indices and landing in the session; a round returning no client messages yielding no tool_result; no pending call leaking across three rounds; and the three gates: no registry, end_turn beside a tool call, a stream with no stop. The mock-driven cases live at the root, where the layering rule lets a test reach both the orchestrator and the exported mock. The orchestration, tools and global coverage floors ratchet to the measured numbers. Closes #80. Co-Authored-By: Claude Fable 5.1 --- jest.config.js | 7 +- specs/hal-engine-abandon-on-close/spec.md | 6 +- specs/hal-engine-architecture/spec.md | 78 +++++---- specs/hal-engine-config-hooks/spec.md | 10 +- specs/hal-engine-tool-executor-throw/spec.md | 6 +- .../hal-engine-tool-input-validation/spec.md | 2 +- src/orchestration/chatOrchestrator.test.ts | 23 +++ src/orchestration/orchestratorHelpers.test.ts | 88 +++++++++- src/orchestration/toolLoopTestSupport.ts | 22 +++ src/toolLoop.test.ts | 161 ++++++++++++++++++ tsconfig.build.json | 1 + 11 files changed, 356 insertions(+), 48 deletions(-) create mode 100644 src/orchestration/toolLoopTestSupport.ts create mode 100644 src/toolLoop.test.ts diff --git a/jest.config.js b/jest.config.js index 31861357..e89a9fb9 100644 --- a/jest.config.js +++ b/jest.config.js @@ -55,11 +55,12 @@ export default { // because its denominator grew: vertex, mock and global rose, orchestration and its tools fell. // That is a finer instrument reading the same suite, which is why these two moved down. coverageThreshold: { - 'src/orchestration/': {statements: 96.96, branches: 92.89, functions: 97.67, lines: 97.1}, - 'src/orchestration/tools/': {statements: 92.77, branches: 92.3, functions: 92.3, lines: 94.28}, + 'src/orchestration/': {statements: 97.1, branches: 93.42, functions: 97.77, lines: 97.23}, + 'src/orchestration/tools/': {statements: 93.02, branches: 92.59, functions: 92.59, lines: 94.36}, 'src/infrastructure/stores/': {statements: 100, branches: 100, functions: 100, lines: 100}, 'src/providers/vertex/': {statements: 94.23, branches: 84.7, functions: 100, lines: 97.72}, 'src/providers/mock/': {statements: 100, branches: 81.25, functions: 100, lines: 100}, - global: {statements: 91.3, branches: 83.55, functions: 89.79, lines: 91.92}, + // Measured over the files no path floor above claims, which is what Jest holds `global` to. + global: {statements: 95.05, branches: 90.7, functions: 93.75, lines: 95.74}, }, }; diff --git a/specs/hal-engine-abandon-on-close/spec.md b/specs/hal-engine-abandon-on-close/spec.md index 2336594b..de63d5c7 100644 --- a/specs/hal-engine-abandon-on-close/spec.md +++ b/specs/hal-engine-abandon-on-close/spec.md @@ -25,9 +25,9 @@ Closing a socket used to stop nothing. The turn it had started kept generating b ## The run -- `ChatOrchestrator.processMessageStream` takes an optional second argument, `{signal}`, and hands the signal to `provider.sendMessage` as `params.signal`, the very object the caller passed ([validated by: reaches the provider as params.signal, the very object the caller passed](../../src/orchestration/chatOrchestrator.test.ts#L720)). -- A run whose provider stops on the abort ends with that round: `afterModelResponse` does not fire, since there is no finished answer, and `afterSession` fires once ([validated by: skips afterModelResponse when its provider stops on the abort, firing afterSession once](../../src/orchestration/chatOrchestrator.test.ts#L739)). -- An abort is a controlled stop, not a failure: a failure after the signal aborted ends the run quietly, with no `onError`, no `afterModelResponse`, since there is no finished response, and no rethrow, while `afterSession` fires once ([validated by: ends quietly when the provider fails after the signal aborted: no onError, afterModelResponse or rethrow, afterSession once](../../src/orchestration/chatOrchestrator.test.ts#L745)). +- `ChatOrchestrator.processMessageStream` takes an optional second argument, `{signal}`, and hands the signal to `provider.sendMessage` as `params.signal`, the very object the caller passed ([validated by: reaches the provider as params.signal, the very object the caller passed](../../src/orchestration/chatOrchestrator.test.ts#L721)). +- A run whose provider stops on the abort ends with that round: `afterModelResponse` does not fire, since there is no finished answer, and `afterSession` fires once ([validated by: skips afterModelResponse when its provider stops on the abort, firing afterSession once](../../src/orchestration/chatOrchestrator.test.ts#L740)). +- An abort is a controlled stop, not a failure: a failure after the signal aborted ends the run quietly, with no `onError`, no `afterModelResponse`, since there is no finished response, and no rethrow, while `afterSession` fires once ([validated by: ends quietly when the provider fails after the signal aborted: no onError, afterModelResponse or rethrow, afterSession once](../../src/orchestration/chatOrchestrator.test.ts#L746)). - Each tool call's `ToolContext.signal` is composed from the run's signal and the call's own through `AbortSignal.any`, so neither replaces the other: abandoning the run aborts it, and so does the call's deadline ([validated by: aborts each call's context signal when its run is abandoned, the signal still the call's own](../../src/toolDeadline.test.ts#L384), [validated by: answers a call that never settles at toolTimeoutMs with a result naming the tool and the deadline, and aborts its signal](../../src/toolDeadline.test.ts#L100)). - A round whose run is abandoned while a tool hangs ends at once, with a deadline or with `toolTimeoutMs: 0`, and no provider call follows it ([validated by: ends the round at once when its run is abandoned while a tool hangs, with a deadline or with none](../../src/toolDeadline.test.ts#L394)). - A round leaves no listener on the run's signal once its calls have settled, so a long-lived connection does not collect one per tool call ([validated by: leaves no abort listener on the run signal once its calls have settled](../../src/toolDeadline.test.ts#L404)). diff --git a/specs/hal-engine-architecture/spec.md b/specs/hal-engine-architecture/spec.md index a672f486..0c7271d8 100644 --- a/specs/hal-engine-architecture/spec.md +++ b/specs/hal-engine-architecture/spec.md @@ -213,7 +213,7 @@ interface PromptBuilderConfig { ### OrchestratorHooks -Async lifecycle hooks for customizing the orchestration flow. Every hook is optional and receives the current session, and an orchestrator built with none behaves exactly as one built with an empty set ([validated by: works without any hooks configured](../../src/orchestration/chatOrchestrator.test.ts#L447)). Install hooks through `HalEngineConfig.orchestrator.hooks`; `createHalEngine` forwards the set to the orchestrator ([validated by: forwards an orchestrator hook, so one passed through the config actually fires](../../src/config.test.ts#L27)). +Async lifecycle hooks for customizing the orchestration flow. Every hook is optional and receives the current session, and an orchestrator built with none behaves exactly as one built with an empty set ([validated by: works without any hooks configured](../../src/orchestration/chatOrchestrator.test.ts#L448)). Install hooks through `HalEngineConfig.orchestrator.hooks`; `createHalEngine` forwards the set to the orchestrator ([validated by: forwards an orchestrator hook, so one passed through the config actually fires](../../src/config.test.ts#L27)). ```typescript @@ -238,21 +238,21 @@ The hooks fire in this order: beforeSession → beforeUserInput → afterUserInput → beforeModelResponse → ...streaming (beforeToolCall × each tool call, each round)... → onToolBudgetExhausted (only on an exhausted budget) → afterModelResponse → afterSession ``` -- The order above holds for a successful pass ([validated by: calls all hooks in correct order](../../src/orchestration/chatOrchestrator.test.ts#L374)). -- When the stream fails, `afterModelResponse` is skipped and the tail becomes `onError` then `afterSession` ([validated by: error path calls onError then afterSession](../../src/orchestration/chatOrchestrator.test.ts#L414)). -- `beforeSession` runs before any other hook ([validated by: called before anything else](../../src/orchestration/chatOrchestrator.test.ts#L131)). -- `beforeUserInput` receives the content of the last user message ([validated by: receives the last user message content](../../src/orchestration/chatOrchestrator.test.ts#L199)). -- `beforeUserInput` can modify that message by returning a different string ([validated by: modifies user message when returning different value](../../src/orchestration/chatOrchestrator.test.ts#L214)). -- `afterUserInput` receives the message as `beforeUserInput` left it, not as the client sent it ([validated by: receives user message after any beforeUserInput modification](../../src/orchestration/chatOrchestrator.test.ts#L229)). -- `beforeModelResponse` can replace the system prompt, for example loading it from a `PromptStore` ([validated by: replaces system prompt with returned value](../../src/orchestration/chatOrchestrator.test.ts#L246)). +- The order above holds for a successful pass ([validated by: calls all hooks in correct order](../../src/orchestration/chatOrchestrator.test.ts#L375)). +- When the stream fails, `afterModelResponse` is skipped and the tail becomes `onError` then `afterSession` ([validated by: error path calls onError then afterSession](../../src/orchestration/chatOrchestrator.test.ts#L415)). +- `beforeSession` runs before any other hook ([validated by: called before anything else](../../src/orchestration/chatOrchestrator.test.ts#L132)). +- `beforeUserInput` receives the content of the last user message ([validated by: receives the last user message content](../../src/orchestration/chatOrchestrator.test.ts#L200)). +- `beforeUserInput` can modify that message by returning a different string ([validated by: modifies user message when returning different value](../../src/orchestration/chatOrchestrator.test.ts#L215)). +- `afterUserInput` receives the message as `beforeUserInput` left it, not as the client sent it ([validated by: receives user message after any beforeUserInput modification](../../src/orchestration/chatOrchestrator.test.ts#L230)). +- `beforeModelResponse` can replace the system prompt, for example loading it from a `PromptStore` ([validated by: replaces system prompt with returned value](../../src/orchestration/chatOrchestrator.test.ts#L247)). - `beforeModelResponse` can also set `session.activePersona`, and every assistant entry created for that turn carries it, since the hook runs before the first one exists ([validated by: stamps 'p-two' when beforeModelResponse changes the session's activePersona from 'p-one' mid-turn](../../src/transport/ws/assistantPersona.test.ts#L98)). -- `afterModelResponse` receives the collected response text and the usage the provider reported ([validated by: receives collected response text and usage](../../src/orchestration/chatOrchestrator.test.ts#L261)). -- `afterModelResponse` does not run when the stream throws, so it never reports a response that was not delivered ([validated by: not called when stream throws](../../src/orchestration/chatOrchestrator.test.ts#L281)). -- `onError` receives the session and the error ([validated by: called with session and error when stream fails](../../src/orchestration/chatOrchestrator.test.ts#L303)). -- `onError` observes rather than handles: the error still propagates to the caller after it returns ([validated by: error still propagates after onError hook](../../src/orchestration/chatOrchestrator.test.ts#L325)). +- `afterModelResponse` receives the collected response text and the usage the provider reported ([validated by: receives collected response text and usage](../../src/orchestration/chatOrchestrator.test.ts#L262)). +- `afterModelResponse` does not run when the stream throws, so it never reports a response that was not delivered ([validated by: not called when stream throws](../../src/orchestration/chatOrchestrator.test.ts#L282)). +- `onError` receives the session and the error ([validated by: called with session and error when stream fails](../../src/orchestration/chatOrchestrator.test.ts#L304)). +- `onError` observes rather than handles: the error still propagates to the caller after it returns ([validated by: error still propagates after onError hook](../../src/orchestration/chatOrchestrator.test.ts#L326)). - `onToolBudgetExhausted` fires only when the model asks for a tool round the budget refuses: after the last provider call and before `afterModelResponse`, which then receives the hook's sentence at the end of the response text ([validated by: hands afterModelResponse text ending in the sentence, and the usage of a run without the hook](../../src/orchestration/toolBudget.test.ts#L214)). -- `afterSession` runs after everything else completes ([validated by: called after everything completes](../../src/orchestration/chatOrchestrator.test.ts#L170)). -- `afterSession` fires even on error ([validated by: called even when an error occurs](../../src/orchestration/chatOrchestrator.test.ts#L180)). +- `afterSession` runs after everything else completes ([validated by: called after everything completes](../../src/orchestration/chatOrchestrator.test.ts#L171)). +- `afterSession` fires even on error ([validated by: called even when an error occurs](../../src/orchestration/chatOrchestrator.test.ts#L181)). A hook receives the full `session` object, so it has access to `session.authHeaders?.authorization` (the caller's bearer token, when `auth.forwardHeaders` lets it through, as [the tool header allowlist spec](../hal-engine-tool-header-allowlist/spec.md) describes) and every user message verbatim through `session.entries`; the engine redacts nothing before calling a hook. Anything a hook persists becomes the deployer's own data-retention obligation. @@ -380,30 +380,46 @@ Here is what the message handler does when a tool call comes through: 2. The handler creates a `ToolEntry` and sends it to the client via `entry_upsert` 3. The orchestrator executes the tool (via `ToolRegistry.execute()`) 4. Tool results get added to the conversation as a `tool_result` message -5. If the tool returned `clientMessages`, they are forwarded to the frontend as-is, in one `tool_result` chunk ([validated by: forwards the client messages a tool returned as one tool_result chunk](../../src/orchestration/chatOrchestrator.test.ts#L524)) -6. If the tool set `suppressAssistantResponse`, the AI's next reply is kept in session context but hidden from the frontend ([validated by: asks for suppression when the tool says the reply is already handled](../../src/orchestration/chatOrchestrator.test.ts#L554)) -7. The orchestrator re-queries the AI provider with the updated messages, and the chunks of every round reach the client in order ([validated by: runs another round after a tool call and streams both rounds in order](../../src/orchestration/chatOrchestrator.test.ts#L489)) -8. This repeats until the budget is spent: at most `maxToolRounds` tool rounds are executed, and the provider is called at most `maxToolRounds + 1` times, so the default of 5 executes five rounds and makes six model calls, the last of which reads the fifth round's results. A round requested after that is not executed; see [the tool budget spec](../hal-engine-tool-budget/spec.md) ([validated by: executes 5 tool rounds and makes 6 provider calls at the default budget](../../src/orchestration/toolBudget.test.ts#L95), [validated by: stops asking for tools once maxToolRounds is spent](../../src/orchestration/chatOrchestrator.test.ts#L510)) +5. If the tool returned `clientMessages`, they are forwarded to the frontend as-is, in one `tool_result` chunk ([validated by: forwards the client messages a tool returned as one tool_result chunk](../../src/orchestration/chatOrchestrator.test.ts#L525)) +6. If the tool set `suppressAssistantResponse`, the AI's next reply is kept in session context but hidden from the frontend ([validated by: asks for suppression when the tool says the reply is already handled](../../src/orchestration/chatOrchestrator.test.ts#L555)) +7. The orchestrator re-queries the AI provider with the updated messages, and the chunks of every round reach the client in order ([validated by: runs another round after a tool call and streams both rounds in order](../../src/orchestration/chatOrchestrator.test.ts#L490)) +8. This repeats until the budget is spent: at most `maxToolRounds` tool rounds are executed, and the provider is called at most `maxToolRounds + 1` times, so the default of 5 executes five rounds and makes six model calls, the last of which reads the fifth round's results. A round requested after that is not executed; see [the tool budget spec](../hal-engine-tool-budget/spec.md) ([validated by: executes 5 tool rounds and makes 6 provider calls at the default budget](../../src/orchestration/toolBudget.test.ts#L95), [validated by: stops asking for tools once maxToolRounds is spent](../../src/orchestration/chatOrchestrator.test.ts#L511)) ### Whether a round continues -- A round continues only when the model stopped for `tool_use`, named at least one tool, and a `ToolRegistry` is configured ([validated by: continues when the model requested a tool and a registry can run it](../../src/orchestration/orchestratorHelpers.test.ts#L11)). -- With no registry configured the loop stops, whatever the model asked for, because nothing could execute the call ([validated by: stops when no registry is configured, whatever the model asked for](../../src/orchestration/orchestratorHelpers.test.ts#L15)). -- A `tool_use` stop naming no tool stops the loop, rather than re-querying with nothing to add ([validated by: stops when the model named no tool](../../src/orchestration/orchestratorHelpers.test.ts#L19)). -- Any other stop reason ends the loop even when tool calls are pending ([validated by: stops when the model finished for a reason other than tool use](../../src/orchestration/orchestratorHelpers.test.ts#L23)). +- A round continues only when the model stopped for `tool_use`, named at least one tool, and a `ToolRegistry` is configured ([validated by: continues when the model requested a tool and a registry can run it](../../src/orchestration/orchestratorHelpers.test.ts#L18)). +- With no registry configured the loop stops, whatever the model asked for, because nothing could execute the call ([validated by: stops when no registry is configured, whatever the model asked for](../../src/orchestration/orchestratorHelpers.test.ts#L22)). +- A `tool_use` stop naming no tool stops the loop, rather than re-querying with nothing to add ([validated by: stops when the model named no tool](../../src/orchestration/orchestratorHelpers.test.ts#L26)). +- Any other stop reason ends the loop even when tool calls are pending ([validated by: stops when the model finished for a reason other than tool use](../../src/orchestration/orchestratorHelpers.test.ts#L30)). +- The gate is the whole truth table of its three conditions: of the eight combinations of stop reason, pending calls and registry, only `tool_use` with a pending call and a registry continues ([validated by: continues for tool_use with a pending call and a registry alone, and stops for the other seven rows](../../src/orchestration/orchestratorHelpers.test.ts#L61)). +- Seen from outside, a scripted `tool_use` turn with no registry ends after one round: one provider call, no executor, and the next scripted round never streams ([validated by: ends a scripted tool_use turn after one round with no registry: one provider call, the tool_use and stop chunks alone](../../src/toolLoop.test.ts#L152)). +- A round that stops with `end_turn` despite carrying a tool call ends the turn with zero executions ([validated by: ends after one round with zero executions when the stop reason is end_turn despite a tool call: the second scripted round never streams](../../src/orchestration/chatOrchestrator.test.ts#L880)). +- A stream that ends without a `stop` chunk leaves the stop reason empty, so the turn ends after that round with zero executions and no second provider call ([validated by: ends after one round with zero executions when the stream ends without a stop chunk: the second scripted round never streams](../../src/orchestration/chatOrchestrator.test.ts#L884)). +- The helpers the round is built from are pure. `collectText` joins the text chunks of a stream and ignores every other type ([validated by: joins the text chunks of a stream in order and ignores every other chunk type](../../src/orchestration/orchestratorHelpers.test.ts#L87)). +- `collectToolCalls` appends a `tool_use` chunk's call to the pending list and ignores the rest ([validated by: appends the call of a tool_use chunk to the pending list and leaves it alone for text and stop chunks](../../src/orchestration/orchestratorHelpers.test.ts#L95)). +- `extractStopReason` reads a stop chunk and answers `null` otherwise ([validated by: reads the reason off a stop chunk and answers null for a text chunk](../../src/orchestration/orchestratorHelpers.test.ts#L108)). +- `extractUsage` reads a stop chunk's usage and answers `undefined` for a stop without one or any other chunk ([validated by: reads the usage off a stop chunk that reports one, and answers undefined for a stop without usage and for a text chunk](../../src/orchestration/orchestratorHelpers.test.ts#L118)). ### Across rounds -- A tool call's `signature`, an opaque token its provider needs back, is copied onto its `tool_use` block for the next round, and a call without one gets no `signature` key ([validated by: puts sig-1 on the signed call's tool_use block and no signature key on the unsigned one](../../src/orchestration/orchestratorHelpers.test.ts#L29)). -- A call to a tool name the model invented is answered rather than raised, with a result naming it and the registered tools, and the turn goes on ([validated by: answers a call to a tool name the model invented, and the turn goes on to its answer](../../src/orchestration/chatOrchestrator.test.ts#L757)). -- So is a call whose executor throws: it gets a result saying the call failed, its message reaches only the log, as [tool-executor-throw](../hal-engine-tool-executor-throw/spec.md) specifies, and the turn goes on ([validated by: answers a call whose executor threw, and the turn goes on to its answer with afterModelResponse and no onError](../../src/orchestration/chatOrchestrator.test.ts#L770)). +- A tool call's `signature`, an opaque token its provider needs back, is copied onto its `tool_use` block for the next round, and a call without one gets no `signature` key ([validated by: puts sig-1 on the signed call's tool_use block and no signature key on the unsigned one](../../src/orchestration/orchestratorHelpers.test.ts#L36)). +- A call to a tool name the model invented is answered rather than raised, with a result naming it and the registered tools, and the turn goes on ([validated by: answers a call to a tool name the model invented, and the turn goes on to its answer](../../src/orchestration/chatOrchestrator.test.ts#L758)). +- So is a call whose executor throws: it gets a result saying the call failed, its message reaches only the log, as [tool-executor-throw](../hal-engine-tool-executor-throw/spec.md) specifies, and the turn goes on ([validated by: answers a call whose executor threw, and the turn goes on to its answer with afterModelResponse and no onError](../../src/orchestration/chatOrchestrator.test.ts#L771)). - A tool signals a failed call by returning `isError: true` beside its `result`, and the engine marks the failures it substitutes the same way, so the provider is told the call failed instead of leaving the model to infer it from prose; [the tool responses spec](../hal-engine-tool-responses/spec.md) gives each provider's encoding ([validated by: carries isError true from an executor's {result: 'lookup failed', isError: true} onto the tool_result the provider's next call gets](../../src/orchestration/toolResultError.test.ts#L62)). -- The loop ends as soon as a round stops with `end_turn`, and nothing further is asked of the provider ([validated by: stops after one round when nothing asked for a tool](../../src/orchestration/chatOrchestrator.test.ts#L501)). -- An `entry_upsert` a tool returns is appended to the session and its index rewritten to the position it actually landed in, because a tool cannot know how long the session already is ([validated by: appends an upserted entry to the session and rewrites its index to match](../../src/orchestration/chatOrchestrator.test.ts#L537)). -- The response text a hook sees is the text of every round joined, not only the last ([validated by: joins the text of every round, not only the last](../../src/orchestration/chatOrchestrator.test.ts#L566)). -- Usage an earlier round reported is kept when a later round reports none, so a tool round does not erase the token count ([validated by: keeps the usage an earlier round reported when a later round reports none](../../src/orchestration/chatOrchestrator.test.ts#L598)). -- The usage `afterModelResponse` receives is `totalUsage`: the sum of what every provider call in the turn reported, so a turn that ran tools counts every round rather than the last one alone ([validated by: sums the usage of every round, not only the last](../../src/orchestration/chatOrchestrator.test.ts#L616)). -- A round that reports no usage drops out of the sum rather than voiding it: the rounds that did report are still counted, and a turn where nothing reported reports nothing ([validated by: counts every reporting round even when a round between them reports none](../../src/orchestration/chatOrchestrator.test.ts#L638)). +- The loop ends as soon as a round stops with `end_turn`, and nothing further is asked of the provider ([validated by: stops after one round when nothing asked for a tool](../../src/orchestration/chatOrchestrator.test.ts#L502)). +- An `entry_upsert` a tool returns is appended to the session and its index rewritten to the position it actually landed in, because a tool cannot know how long the session already is ([validated by: appends an upserted entry to the session and rewrites its index to match](../../src/orchestration/chatOrchestrator.test.ts#L538)). +- The response text a hook sees is the text of every round joined, not only the last ([validated by: joins the text of every round, not only the last](../../src/orchestration/chatOrchestrator.test.ts#L567)). +- Usage an earlier round reported is kept when a later round reports none, so a tool round does not erase the token count ([validated by: keeps the usage an earlier round reported when a later round reports none](../../src/orchestration/chatOrchestrator.test.ts#L599)). +- The usage `afterModelResponse` receives is `totalUsage`: the sum of what every provider call in the turn reported, so a turn that ran tools counts every round rather than the last one alone ([validated by: sums the usage of every round, not only the last](../../src/orchestration/chatOrchestrator.test.ts#L617)). +- A round that reports no usage drops out of the sum rather than voiding it: the rounds that did report are still counted, and a turn where nothing reported reports nothing ([validated by: counts every reporting round even when a round between them reports none](../../src/orchestration/chatOrchestrator.test.ts#L639)). + +### A round with more than one call + +- A round carrying two calls runs both executors, and the next round's request carries one assistant message with two `tool_use` items and one user message with two `tool_result` items whose ids match, in the order the model asked ([validated by: runs both executors of a two-call round and sends the next round one assistant message with two tool_use items and one user message with two tool_result items whose ids match, in script order](../../src/toolLoop.test.ts#L43)). +- When one of two executors suppresses the answer, exactly one `suppress_output` chunk follows the round's `tool_result` chunk ([validated by: yields exactly one suppress_output chunk, after the tool_result chunk, when one of two executors suppresses](../../src/toolLoop.test.ts#L71)). +- Two `entry_upsert` messages from one round take sequential indices from the session's length at call time, and both entries are readable from the session afterwards ([validated by: gives two upserted entries sequential indices from the session length at call time, and both are readable from the session afterwards](../../src/toolLoop.test.ts#L93)). +- A round whose executors return no client messages yields no `tool_result` chunk at all ([validated by: yields no tool_result chunk for a round whose executors return no client messages](../../src/toolLoop.test.ts#L128)). +- The pending calls are a fresh list each round, so a call from one round never reaches the next round's request ([validated by: carries no pending call from one round into the next: three rounds leave two assistant messages with one tool_use item each, and each executor ran once](../../src/toolLoop.test.ts#L139)). The frontend shows a spinner on the last tool entry while the stream is still processing (`isProcessing` is `true`). The `stream_end` message clears the processing state, which hides the spinner. This works correctly even when tool suppression prevents assistant entries from reaching the frontend. See [tool-responses.md](../hal-engine-tool-responses/spec.md) for details on client messages and suppression. diff --git a/specs/hal-engine-config-hooks/spec.md b/specs/hal-engine-config-hooks/spec.md index 8dc0dba4..77b35a59 100644 --- a/specs/hal-engine-config-hooks/spec.md +++ b/specs/hal-engine-config-hooks/spec.md @@ -12,12 +12,12 @@ - A hook passed through `orchestrator.hooks` is forwarded to the orchestrator and fires ([validated by: forwards an orchestrator hook, so one passed through the config actually fires](../../src/config.test.ts#L27)). - An engine built with no `orchestrator` key at all still processes a message, so the field is optional in fact and not only in the type ([validated by: processes a message when the config declares no orchestrator key at all](../../src/config.test.ts#L103)). - A `beforeModelResponse` hook installed through config is handed the base system prompt built from `prompt`, proving the hook reaches the orchestrator and is wired into the prompt pipeline rather than only being stored on the config ([validated by: hands a config-installed beforeModelResponse hook the built base system prompt, not merely storing it](../../src/config.test.ts#L154)). -- The value the hook returns then replaces the system prompt the provider receives; this is exercised at the orchestrator layer because `HalEngineConfig.provider` takes a `ProviderConfig` and offers no seam to inject a recording provider through config ([validated by: replaces system prompt with returned value](../../src/orchestration/chatOrchestrator.test.ts#L246)). +- The value the hook returns then replaces the system prompt the provider receives; this is exercised at the orchestrator layer because `HalEngineConfig.provider` takes a `ProviderConfig` and offers no seam to inject a recording provider through config ([validated by: replaces system prompt with returned value](../../src/orchestration/chatOrchestrator.test.ts#L247)). - For a session that completes without error the hooks fire in the order `beforeSession` → `beforeUserInput` → `afterUserInput` → `beforeModelResponse` → `afterModelResponse` → `afterSession` ([validated by: fires the lifecycle hooks in documented order for a session that completes without error](../../src/config.test.ts#L188)). -- A `beforeSession` that throws reaches the caller as a rejection, and also runs `onError` with the thrown error and then `afterSession`, because the call sits inside the same `try`/`finally` as the rest of the turn. An `afterSession` hook called under this condition can infer the session never advanced past `beforeSession`: no user-input hooks ran, no model response was generated, and no tool rounds were started — a cleanup hook that allocates in `beforeSession` and frees in `afterSession` will therefore always see the allocation and the release on the same error path ([validated by: runs onError then afterSession when it throws, and still rejects to the caller](../../src/orchestration/chatOrchestrator.test.ts#L141)). -- `afterModelResponse` is called once per user message after the round loop exits and is passed `totalUsage` — the sum of the usage every provider call in the turn reported, not the last round alone ([validated by: sums the usage of every round, not only the last](../../src/orchestration/chatOrchestrator.test.ts#L616)). -- When no provider call in the turn reports usage, `afterModelResponse` is passed `undefined`, not a zeroed total ([validated by: hands afterModelResponse undefined, not a zeroed total, when no round of the turn reports usage](../../src/orchestration/chatOrchestrator.test.ts#L661)). -- Every rejection reaches `onError`: a value that is not already an `Error` is wrapped in one whose `cause` is that original value, so a hook typed against `Error` still fires and a hook that wants the raw value can still reach it, while the caller receives the original rejection unchanged ([validated by: wraps a non-Error rejection in an Error whose cause is the original and still rejects the caller with the original](../../src/orchestration/chatOrchestrator.test.ts#L336)). +- A `beforeSession` that throws reaches the caller as a rejection, and also runs `onError` with the thrown error and then `afterSession`, because the call sits inside the same `try`/`finally` as the rest of the turn. An `afterSession` hook called under this condition can infer the session never advanced past `beforeSession`: no user-input hooks ran, no model response was generated, and no tool rounds were started — a cleanup hook that allocates in `beforeSession` and frees in `afterSession` will therefore always see the allocation and the release on the same error path ([validated by: runs onError then afterSession when it throws, and still rejects to the caller](../../src/orchestration/chatOrchestrator.test.ts#L142)). +- `afterModelResponse` is called once per user message after the round loop exits and is passed `totalUsage` — the sum of the usage every provider call in the turn reported, not the last round alone ([validated by: sums the usage of every round, not only the last](../../src/orchestration/chatOrchestrator.test.ts#L617)). +- When no provider call in the turn reports usage, `afterModelResponse` is passed `undefined`, not a zeroed total ([validated by: hands afterModelResponse undefined, not a zeroed total, when no round of the turn reports usage](../../src/orchestration/chatOrchestrator.test.ts#L662)). +- Every rejection reaches `onError`: a value that is not already an `Error` is wrapped in one whose `cause` is that original value, so a hook typed against `Error` still fires and a hook that wants the raw value can still reach it, while the caller receives the original rejection unchanged ([validated by: wraps a non-Error rejection in an Error whose cause is the original and still rejects the caller with the original](../../src/orchestration/chatOrchestrator.test.ts#L337)). ## Engine startup diff --git a/specs/hal-engine-tool-executor-throw/spec.md b/specs/hal-engine-tool-executor-throw/spec.md index 12da6003..9450af1e 100644 --- a/specs/hal-engine-tool-executor-throw/spec.md +++ b/specs/hal-engine-tool-executor-throw/spec.md @@ -17,7 +17,7 @@ A tool call the registry could not run used to end the conversation. `ToolRegist - It is answered with a `ToolResponse` whose `result` names the attempted name and every registered tool, with no `clientMessages` and no `suppressAssistantResponse` ([validated by: answers an unregistered name with a result naming it and every registered tool, instead of throwing](../../src/orchestration/tools/registry.test.ts#L148)). - With no tool registered, the answer says so instead of ending in an empty list ([validated by: says that no tools are registered when the registry is empty, ending in no empty list](../../src/orchestration/tools/registry.test.ts#L155)). - The attempted name is cut to 64 characters, in the answer and in the one `warn` line, `tool not found` under `tool`, which is the only signal that a prompt or a registration is broken ([validated by: bounds an invented name to 64 characters in its answer and in its one warn line, under tool](../../src/orchestration/tools/registry.test.ts#L161)). -- The model reads the answer as that call's `tool_result`, and the turn goes on to its answer ([validated by: answers a call to a tool name the model invented, and the turn goes on to its answer](../../src/orchestration/chatOrchestrator.test.ts#L757)). +- The model reads the answer as that call's `tool_result`, and the turn goes on to its answer ([validated by: answers a call to a tool name the model invented, and the turn goes on to its answer](../../src/orchestration/chatOrchestrator.test.ts#L758)). ## An executor that throws @@ -27,12 +27,12 @@ A tool call the registry could not run used to end the conversation. `ToolRegist - A throw after the call's `ToolContext.signal` was aborted for any reason but a timeout, as when its turn is abandoned because the socket closed, is no tool failure: it is logged at `info` instead, once, under `tool` as `tool call abandoned`, with the tool's name and the error's type, and writes no `error` line ([validated by: logs a throw after its signal was aborted, not timed out, at info as tool call abandoned, and no error line](../../src/orchestration/tools/registry.test.ts#L211)). - A reason that is no error at all, such as a string or `null`, is no timeout either, so a throw after it is logged as abandoned too ([validated by: logs a throw as abandoned after a signal aborted with a reason that is no error, a string or null](../../src/orchestration/tools/registry.test.ts#L225)). - A throw while the signal is still live, or after it was aborted with a `TimeoutError`, the reason the orchestrator gives at a call's `toolTimeoutMs` deadline, is a failure, logged at `error` as `tool executor threw` ([validated by: keeps a throw at error as tool executor threw while its signal is live and after it timed out](../../src/orchestration/tools/registry.test.ts#L235)). -- The turn goes on to its answer and succeeds: `afterModelResponse` fires and `onError` does not ([validated by: answers a call whose executor threw, and the turn goes on to its answer with afterModelResponse and no onError](../../src/orchestration/chatOrchestrator.test.ts#L770)). +- The turn goes on to its answer and succeeds: `afterModelResponse` fires and `onError` does not ([validated by: answers a call whose executor threw, and the turn goes on to its answer with afterModelResponse and no onError](../../src/orchestration/chatOrchestrator.test.ts#L771)). ## Both - No value from the call's input reaches the answer or any log line, only the input's keys, which the registry already logs ([validated by: keeps every value of the call input out of the answer and the log, for an invented name and a throwing executor](../../src/orchestration/tools/registry.test.ts#L252)). -- A round in which some calls fail still returns every result: a working tool beside them runs once and its result reaches the model unchanged ([validated by: returns every result of a round in which calls fail, the working tool run once and its result unchanged](../../src/orchestration/chatOrchestrator.test.ts#L783)). +- A round in which some calls fail still returns every result: a working tool beside them runs once and its result reaches the model unchanged ([validated by: returns every result of a round in which calls fail, the working tool run once and its result unchanged](../../src/orchestration/chatOrchestrator.test.ts#L784)). - **NIS-2 Article 21, operational resilience.** Over the WebSocket, a turn whose tool name was invented streams its answer and ends with `stream_end`, never an `error` frame ([validated by: completes a turn whose tool name the model invented: its answer streams, then stream_end, with no error frame](../../src/transport/ws/messageHandler.test.ts#L874)). - So does a turn whose tool executor threw: one broken tool degrades one answer instead of ending the conversation ([validated by: completes a turn whose tool executor threw: its answer streams, then stream_end, with no error frame](../../src/transport/ws/messageHandler.test.ts#L880)). diff --git a/specs/hal-engine-tool-input-validation/spec.md b/specs/hal-engine-tool-input-validation/spec.md index f18b5213..a28f4437 100644 --- a/specs/hal-engine-tool-input-validation/spec.md +++ b/specs/hal-engine-tool-input-validation/spec.md @@ -23,7 +23,7 @@ ## Orchestrator integration -- A validation failure becomes a tool result (not a thrown error), so the orchestrator delivers the rejection text to the model and the model can attempt a corrected call; `DEFAULT_MAX_TOOL_ROUNDS = 5` is the bound that stops a retry cycle from spinning ([validated by: resolves processMessage and makes the validation-rejection text visible to the provider on the next round](../../src/orchestration/chatOrchestrator.test.ts#L677)). +- A validation failure becomes a tool result (not a thrown error), so the orchestrator delivers the rejection text to the model and the model can attempt a corrected call; `DEFAULT_MAX_TOOL_ROUNDS = 5` is the bound that stops a retry cycle from spinning ([validated by: resolves processMessage and makes the validation-rejection text visible to the provider on the next round](../../src/orchestration/chatOrchestrator.test.ts#L678)). ## Out of scope diff --git a/src/orchestration/chatOrchestrator.test.ts b/src/orchestration/chatOrchestrator.test.ts index 11a10e44..35775e8e 100644 --- a/src/orchestration/chatOrchestrator.test.ts +++ b/src/orchestration/chatOrchestrator.test.ts @@ -13,6 +13,7 @@ import {AIError} from '../types/ai.js'; import type {ChatSession} from '../types/session.js'; import {PromptBuilder} from '../infrastructure/builders/promptBuilder.js'; import {ToolRegistry} from './tools/registry.js'; +import {twoTools} from './toolLoopTestSupport.js'; import type {ToolResponse} from './tools/registry.js'; import type {OutgoingMessage} from '../types/messages.js'; import type {SessionEntry} from '../types/session.js'; @@ -862,3 +863,25 @@ async function abortedMidStream(end: () => void): Promise<{outcome: string; call return {outcome: await outcomeOf(chunksFrom(stream)), callOrder}; } + +// A scripted first round, then a second round that must never stream; what the loop did is the executions and the chunk types. +const gatedTurn = async (firstRound: MessageChunk[]) => { + const provider = scriptedProvider([firstRound, [{type: 'text', text: 'never'}, STOP_DONE]]); + const {registry, ran} = twoTools({first: {result: 'one'}, second: {result: 'two'}}); + + const chunks = await chunksFrom( + createChatOrchestrator(provider, promptBuilder, registry).processMessageStream(createSession('ask')) + ); + + return {ran, types: chunks.map(c => c.type)}; +}; + +describe('ChatOrchestrator tool loop gating, observed from outside', () => { + it('ends after one round with zero executions when the stop reason is end_turn despite a tool call: the second scripted round never streams', async () => { + expect(await gatedTurn([TOOL_CALL, STOP_DONE])).toEqual({ran: [], types: ['tool_use', 'stop']}); + }); + + it('ends after one round with zero executions when the stream ends without a stop chunk: the second scripted round never streams', async () => { + expect(await gatedTurn([TOOL_CALL])).toEqual({ran: [], types: ['tool_use']}); + }); +}); diff --git a/src/orchestration/orchestratorHelpers.test.ts b/src/orchestration/orchestratorHelpers.test.ts index 8a129d8b..fe7f4dc9 100644 --- a/src/orchestration/orchestratorHelpers.test.ts +++ b/src/orchestration/orchestratorHelpers.test.ts @@ -1,6 +1,13 @@ -import {buildToolCallMessages, shouldContinueToolLoop} from './orchestratorHelpers.js'; +import { + buildToolCallMessages, + collectText, + collectToolCalls, + extractStopReason, + extractUsage, + shouldContinueToolLoop, +} from './orchestratorHelpers.js'; import type {ToolRegistry} from './tools/registry.js'; -import type {ToolCall} from '../types/ai.js'; +import type {MessageChunk, ToolCall} from '../types/ai.js'; // All three conditions matter; dropping the registry check spins the tool loop. @@ -41,3 +48,80 @@ describe('buildToolCallMessages', () => { }); }); }); + +const STOP_FOR_TOOL: MessageChunk = {type: 'stop', stopReason: 'tool_use'}; +const STOP_DONE: MessageChunk = {type: 'stop', stopReason: 'end_turn'}; +const TEXT: MessageChunk = {type: 'text', text: 'word '}; + +const streamOf = async function* (chunks: MessageChunk[]): AsyncGenerator { + yield* chunks; +}; + +describe('shouldContinueToolLoop truth table', () => { + it('continues for tool_use with a pending call and a registry alone, and stops for the other seven rows', () => { + const rows = [ + ['tool_use', A_TOOL, REGISTRY], + ['tool_use', A_TOOL, undefined], + ['tool_use', [], REGISTRY], + ['tool_use', [], undefined], + ['end_turn', A_TOOL, REGISTRY], + ['end_turn', A_TOOL, undefined], + ['end_turn', [], REGISTRY], + ['end_turn', [], undefined], + ] as const; + + expect(rows.map(([reason, calls, registry]) => shouldContinueToolLoop(reason, [...calls], registry))).toEqual([ + true, + false, + false, + false, + false, + false, + false, + false, + ]); + }); +}); + +describe('collectText', () => { + it('joins the text chunks of a stream in order and ignores every other chunk type', async () => { + const tool: MessageChunk = {type: 'tool_use', toolCall: {id: 't1', name: 'lookup', input: {}}}; + + expect(await collectText(streamOf([TEXT, tool, {type: 'text', text: 'by word'}, STOP_DONE]))).toBe('word by word'); + }); +}); + +describe('collectToolCalls', () => { + it('appends the call of a tool_use chunk to the pending list and leaves it alone for text and stop chunks', () => { + const pending: ToolCall[] = []; + const call: ToolCall = {id: 't2', name: 'weather', input: {city: 'here'}}; + + collectToolCalls(TEXT, pending); + collectToolCalls({type: 'tool_use', toolCall: call}, pending); + collectToolCalls(STOP_FOR_TOOL, pending); + + expect(pending).toEqual([call]); + }); +}); + +describe('extractStopReason', () => { + it('reads the reason off a stop chunk and answers null for a text chunk', () => { + expect([extractStopReason(STOP_FOR_TOOL), extractStopReason(STOP_DONE), extractStopReason(TEXT)]).toEqual([ + 'tool_use', + 'end_turn', + null, + ]); + }); +}); + +describe('extractUsage', () => { + it('reads the usage off a stop chunk that reports one, and answers undefined for a stop without usage and for a text chunk', () => { + const usage = {inputTokens: 3, outputTokens: 4, totalTokens: 7}; + + expect([extractUsage({...STOP_DONE, usage}), extractUsage(STOP_DONE), extractUsage(TEXT)]).toEqual([ + usage, + undefined, + undefined, + ]); + }); +}); diff --git a/src/orchestration/toolLoopTestSupport.ts b/src/orchestration/toolLoopTestSupport.ts new file mode 100644 index 00000000..a8e31e87 --- /dev/null +++ b/src/orchestration/toolLoopTestSupport.ts @@ -0,0 +1,22 @@ +import {ToolRegistry} from './tools/registry.js'; +import type {ToolResponse} from './tools/registry.js'; +import type {ToolCall} from '../types/ai.js'; + +/** Two registered tools whose executors record the order they started in; what each answers is the caller's. */ +export function twoTools(answers: {first: ToolResponse; second: ToolResponse}): { + registry: ToolRegistry; + ran: string[]; +} { + const ran: string[] = []; + const registry = new ToolRegistry(); + for (const name of ['first', 'second'] as const) { + registry.register({name, description: `the ${name} tool`, inputSchema: {type: 'object'}}, async () => { + ran.push(name); + return answers[name]; + }); + } + return {registry, ran}; +} + +export const CALL_A: ToolCall = {id: 'call-a', name: 'first', input: {a: 1}}; +export const CALL_B: ToolCall = {id: 'call-b', name: 'second', input: {b: 2}}; diff --git a/src/toolLoop.test.ts b/src/toolLoop.test.ts new file mode 100644 index 00000000..d214fd13 --- /dev/null +++ b/src/toolLoop.test.ts @@ -0,0 +1,161 @@ +import {createChatOrchestrator} from './orchestration/chatOrchestrator.js'; +import {CALL_A, CALL_B, twoTools} from './orchestration/toolLoopTestSupport.js'; +import {PromptBuilder} from './infrastructure/builders/promptBuilder.js'; +import {createMockProvider} from './providers/mock/mockProvider.js'; +import type {MockTurn} from './providers/mock/mockProvider.js'; +import {collectChunks} from './providers/providerTestSupport.js'; +import type {Message, SendMessageParams, ToolCallContent} from './types/ai.js'; +import type {ChatSession, SessionEntry} from './types/session.js'; + +// specs/hal-engine-architecture § Tool Execution Loop: the cases the package's own scripted mock can drive. + +const SENT_AT = '2026-01-01T00:00:00.000Z'; +const promptBuilder = new PromptBuilder({identity: 'You are a test assistant.'}); + +const askSession = (): ChatSession => ({ + sessionId: 's1', + userId: 'u1', + entries: [{role: 'user', content: 'ask', timestamp: SENT_AT}], +}); +const upserted = (content: string): SessionEntry => ({ + role: 'assistant', + content, + timestamp: SENT_AT, + isStreaming: false, +}); + +const toolUseIdsOf = (message: Message): string[][] => { + if (message.role !== 'assistant' || !Array.isArray(message.content)) return []; + const toolUses = message.content.filter((block): block is ToolCallContent => block.type === 'tool_use'); + return [toolUses.map(block => block.toolUseId)]; +}; + +const assistantToolUseIds = (messages: Message[]): string[][] => messages.flatMap(toolUseIdsOf); + +// The package's scripted mock, keeping every request it was sent. +const recordingMock = (turns: MockTurn[]) => { + const sent: SendMessageParams[] = []; + const provider = createMockProvider({type: 'mock', turns, onSendMessage: params => void sent.push(params)}); + return {provider, sent}; +}; + +describe('ChatOrchestrator tool loop, a round with more than one call', () => { + it('runs both executors of a two-call round and sends the next round one assistant message with two tool_use items and one user message with two tool_result items whose ids match, in script order', async () => { + const {provider, sent} = recordingMock([{toolCalls: [CALL_A, CALL_B]}, {text: 'done'}]); + const {registry, ran} = twoTools({first: {result: 'one'}, second: {result: 'two'}}); + + await collectChunks(createChatOrchestrator(provider, promptBuilder, registry).processMessageStream(askSession())); + const {messages} = sent[1]; + const [assistant, user] = messages.slice(-2); + + expect({calls: sent.length, ran, assistant, user}).toEqual({ + calls: 2, + ran: ['first', 'second'], + assistant: { + role: 'assistant', + content: [ + {type: 'tool_use', toolUseId: 'call-a', name: 'first', input: {a: 1}}, + {type: 'tool_use', toolUseId: 'call-b', name: 'second', input: {b: 2}}, + ], + }, + user: { + role: 'user', + content: [ + {type: 'tool_result', toolUseId: 'call-a', content: 'one'}, + {type: 'tool_result', toolUseId: 'call-b', content: 'two'}, + ], + }, + }); + }); + + it('yields exactly one suppress_output chunk, after the tool_result chunk, when one of two executors suppresses', async () => { + const provider = createMockProvider({type: 'mock', turns: [{toolCalls: [CALL_A, CALL_B]}, {text: 'done'}]}); + const {registry} = twoTools({ + first: {result: 'one', clientMessages: [{type: 'entry_commit', index: 0}]}, + second: {result: 'two', suppressAssistantResponse: true}, + }); + + const chunks = await collectChunks( + createChatOrchestrator(provider, promptBuilder, registry).processMessageStream(askSession()) + ); + + expect(chunks.map(c => c.type)).toEqual([ + 'tool_use', + 'tool_use', + 'stop', + 'tool_result', + 'suppress_output', + 'text', + 'stop', + ]); + }); + + it('gives two upserted entries sequential indices from the session length at call time, and both are readable from the session afterwards', async () => { + const [one, two] = [upserted('first card'), upserted('second card')]; + const provider = createMockProvider({type: 'mock', turns: [{toolCalls: [CALL_A, CALL_B]}, {text: 'done'}]}); + const {registry} = twoTools({ + first: { + result: 'one', + clientMessages: [ + {type: 'entry_upsert', index: 999, entry: one}, + {type: 'entry_upsert', index: 999, entry: two}, + ], + }, + second: {result: 'two'}, + }); + const session = askSession(); + const lengthAtCall = session.entries.length; + + const chunks = await collectChunks( + createChatOrchestrator(provider, promptBuilder, registry).processMessageStream(session) + ); + + expect({ + toolResult: chunks.find(c => c.type === 'tool_result'), + appended: session.entries.slice(lengthAtCall), + }).toEqual({ + toolResult: { + type: 'tool_result', + clientMessages: [ + {type: 'entry_upsert', index: lengthAtCall, entry: one}, + {type: 'entry_upsert', index: lengthAtCall + 1, entry: two}, + ], + }, + appended: [one, two], + }); + }); + + it('yields no tool_result chunk for a round whose executors return no client messages', async () => { + const provider = createMockProvider({type: 'mock', turns: [{toolCalls: [CALL_A]}, {text: 'done'}]}); + const {registry} = twoTools({first: {result: 'one'}, second: {result: 'two'}}); + + const chunks = await collectChunks( + createChatOrchestrator(provider, promptBuilder, registry).processMessageStream(askSession()) + ); + + expect(chunks.map(c => c.type)).toEqual(['tool_use', 'stop', 'text', 'stop']); + }); + + it('carries no pending call from one round into the next: three rounds leave two assistant messages with one tool_use item each, and each executor ran once', async () => { + const {provider, sent} = recordingMock([{toolCalls: [CALL_A]}, {toolCalls: [CALL_B]}, {text: 'done'}]); + const {registry, ran} = twoTools({first: {result: 'one'}, second: {result: 'two'}}); + + await collectChunks(createChatOrchestrator(provider, promptBuilder, registry).processMessageStream(askSession())); + + expect({calls: sent.length, ran, toolUseIds: assistantToolUseIds(sent[2].messages)}).toEqual({ + calls: 3, + ran: ['first', 'second'], + toolUseIds: [['call-a'], ['call-b']], + }); + }); + + it('ends a scripted tool_use turn after one round with no registry: one provider call, the tool_use and stop chunks alone', async () => { + const {provider, sent} = recordingMock([{toolCalls: [CALL_A]}, {text: 'never'}]); + + const chunks = await collectChunks( + createChatOrchestrator(provider, promptBuilder).processMessageStream(askSession()) + ); + + expect({calls: sent.length, types: chunks.map(c => c.type)}).toEqual({calls: 1, types: ['tool_use', 'stop']}); + }); +}); diff --git a/tsconfig.build.json b/tsconfig.build.json index a421e6f3..2444a2bc 100644 --- a/tsconfig.build.json +++ b/tsconfig.build.json @@ -14,6 +14,7 @@ "**/*.test.ts", "src/providers/providerTestSupport.ts", "src/orchestration/scriptedProviderTestSupport.ts", + "src/orchestration/toolLoopTestSupport.ts", "src/infrastructure/writeSignalTestSupport.ts", "src/infrastructure/stores/mongo/mongoTestSupport.ts", "src/shared/logCaptureTestSupport.ts", From 25481e3c75c2d4caa989a5e8b7c861283a971244 Mon Sep 17 00:00:00 2001 From: Saad Date: Fri, 23 Oct 2026 11:33:34 +0300 Subject: [PATCH 2/2] test(orchestration): observe the tool the gate names, and keep test support out of src gatedTurn registered first and second while TOOL_CALL names lookup, so its "zero executions" was unobservable; it now registers lookup with a recording executor. The two-tool helper lives in its only consumer, the root-level suite, so no non-test file sits under src/ for the changelog check or the orchestration coverage floor to see; that floor is re-measured without it. Co-Authored-By: Claude Fable 5.1 --- jest.config.js | 2 +- specs/hal-engine-abandon-on-close/spec.md | 6 +- specs/hal-engine-architecture/spec.md | 68 +++++++++---------- specs/hal-engine-config-hooks/spec.md | 10 +-- specs/hal-engine-tool-executor-throw/spec.md | 6 +- .../hal-engine-tool-input-validation/spec.md | 2 +- src/orchestration/chatOrchestrator.test.ts | 10 ++- src/orchestration/toolLoopTestSupport.ts | 22 ------ src/toolLoop.test.ts | 24 ++++++- tsconfig.build.json | 1 - 10 files changed, 76 insertions(+), 75 deletions(-) delete mode 100644 src/orchestration/toolLoopTestSupport.ts diff --git a/jest.config.js b/jest.config.js index e89a9fb9..34667c33 100644 --- a/jest.config.js +++ b/jest.config.js @@ -55,7 +55,7 @@ export default { // because its denominator grew: vertex, mock and global rose, orchestration and its tools fell. // That is a finer instrument reading the same suite, which is why these two moved down. coverageThreshold: { - 'src/orchestration/': {statements: 97.1, branches: 93.42, functions: 97.77, lines: 97.23}, + 'src/orchestration/': {statements: 97.02, branches: 93.42, functions: 97.72, lines: 97.14}, 'src/orchestration/tools/': {statements: 93.02, branches: 92.59, functions: 92.59, lines: 94.36}, 'src/infrastructure/stores/': {statements: 100, branches: 100, functions: 100, lines: 100}, 'src/providers/vertex/': {statements: 94.23, branches: 84.7, functions: 100, lines: 97.72}, diff --git a/specs/hal-engine-abandon-on-close/spec.md b/specs/hal-engine-abandon-on-close/spec.md index de63d5c7..2336594b 100644 --- a/specs/hal-engine-abandon-on-close/spec.md +++ b/specs/hal-engine-abandon-on-close/spec.md @@ -25,9 +25,9 @@ Closing a socket used to stop nothing. The turn it had started kept generating b ## The run -- `ChatOrchestrator.processMessageStream` takes an optional second argument, `{signal}`, and hands the signal to `provider.sendMessage` as `params.signal`, the very object the caller passed ([validated by: reaches the provider as params.signal, the very object the caller passed](../../src/orchestration/chatOrchestrator.test.ts#L721)). -- A run whose provider stops on the abort ends with that round: `afterModelResponse` does not fire, since there is no finished answer, and `afterSession` fires once ([validated by: skips afterModelResponse when its provider stops on the abort, firing afterSession once](../../src/orchestration/chatOrchestrator.test.ts#L740)). -- An abort is a controlled stop, not a failure: a failure after the signal aborted ends the run quietly, with no `onError`, no `afterModelResponse`, since there is no finished response, and no rethrow, while `afterSession` fires once ([validated by: ends quietly when the provider fails after the signal aborted: no onError, afterModelResponse or rethrow, afterSession once](../../src/orchestration/chatOrchestrator.test.ts#L746)). +- `ChatOrchestrator.processMessageStream` takes an optional second argument, `{signal}`, and hands the signal to `provider.sendMessage` as `params.signal`, the very object the caller passed ([validated by: reaches the provider as params.signal, the very object the caller passed](../../src/orchestration/chatOrchestrator.test.ts#L720)). +- A run whose provider stops on the abort ends with that round: `afterModelResponse` does not fire, since there is no finished answer, and `afterSession` fires once ([validated by: skips afterModelResponse when its provider stops on the abort, firing afterSession once](../../src/orchestration/chatOrchestrator.test.ts#L739)). +- An abort is a controlled stop, not a failure: a failure after the signal aborted ends the run quietly, with no `onError`, no `afterModelResponse`, since there is no finished response, and no rethrow, while `afterSession` fires once ([validated by: ends quietly when the provider fails after the signal aborted: no onError, afterModelResponse or rethrow, afterSession once](../../src/orchestration/chatOrchestrator.test.ts#L745)). - Each tool call's `ToolContext.signal` is composed from the run's signal and the call's own through `AbortSignal.any`, so neither replaces the other: abandoning the run aborts it, and so does the call's deadline ([validated by: aborts each call's context signal when its run is abandoned, the signal still the call's own](../../src/toolDeadline.test.ts#L384), [validated by: answers a call that never settles at toolTimeoutMs with a result naming the tool and the deadline, and aborts its signal](../../src/toolDeadline.test.ts#L100)). - A round whose run is abandoned while a tool hangs ends at once, with a deadline or with `toolTimeoutMs: 0`, and no provider call follows it ([validated by: ends the round at once when its run is abandoned while a tool hangs, with a deadline or with none](../../src/toolDeadline.test.ts#L394)). - A round leaves no listener on the run's signal once its calls have settled, so a long-lived connection does not collect one per tool call ([validated by: leaves no abort listener on the run signal once its calls have settled](../../src/toolDeadline.test.ts#L404)). diff --git a/specs/hal-engine-architecture/spec.md b/specs/hal-engine-architecture/spec.md index 0c7271d8..17c268cd 100644 --- a/specs/hal-engine-architecture/spec.md +++ b/specs/hal-engine-architecture/spec.md @@ -213,7 +213,7 @@ interface PromptBuilderConfig { ### OrchestratorHooks -Async lifecycle hooks for customizing the orchestration flow. Every hook is optional and receives the current session, and an orchestrator built with none behaves exactly as one built with an empty set ([validated by: works without any hooks configured](../../src/orchestration/chatOrchestrator.test.ts#L448)). Install hooks through `HalEngineConfig.orchestrator.hooks`; `createHalEngine` forwards the set to the orchestrator ([validated by: forwards an orchestrator hook, so one passed through the config actually fires](../../src/config.test.ts#L27)). +Async lifecycle hooks for customizing the orchestration flow. Every hook is optional and receives the current session, and an orchestrator built with none behaves exactly as one built with an empty set ([validated by: works without any hooks configured](../../src/orchestration/chatOrchestrator.test.ts#L447)). Install hooks through `HalEngineConfig.orchestrator.hooks`; `createHalEngine` forwards the set to the orchestrator ([validated by: forwards an orchestrator hook, so one passed through the config actually fires](../../src/config.test.ts#L27)). ```typescript @@ -238,21 +238,21 @@ The hooks fire in this order: beforeSession → beforeUserInput → afterUserInput → beforeModelResponse → ...streaming (beforeToolCall × each tool call, each round)... → onToolBudgetExhausted (only on an exhausted budget) → afterModelResponse → afterSession ``` -- The order above holds for a successful pass ([validated by: calls all hooks in correct order](../../src/orchestration/chatOrchestrator.test.ts#L375)). -- When the stream fails, `afterModelResponse` is skipped and the tail becomes `onError` then `afterSession` ([validated by: error path calls onError then afterSession](../../src/orchestration/chatOrchestrator.test.ts#L415)). -- `beforeSession` runs before any other hook ([validated by: called before anything else](../../src/orchestration/chatOrchestrator.test.ts#L132)). -- `beforeUserInput` receives the content of the last user message ([validated by: receives the last user message content](../../src/orchestration/chatOrchestrator.test.ts#L200)). -- `beforeUserInput` can modify that message by returning a different string ([validated by: modifies user message when returning different value](../../src/orchestration/chatOrchestrator.test.ts#L215)). -- `afterUserInput` receives the message as `beforeUserInput` left it, not as the client sent it ([validated by: receives user message after any beforeUserInput modification](../../src/orchestration/chatOrchestrator.test.ts#L230)). -- `beforeModelResponse` can replace the system prompt, for example loading it from a `PromptStore` ([validated by: replaces system prompt with returned value](../../src/orchestration/chatOrchestrator.test.ts#L247)). +- The order above holds for a successful pass ([validated by: calls all hooks in correct order](../../src/orchestration/chatOrchestrator.test.ts#L374)). +- When the stream fails, `afterModelResponse` is skipped and the tail becomes `onError` then `afterSession` ([validated by: error path calls onError then afterSession](../../src/orchestration/chatOrchestrator.test.ts#L414)). +- `beforeSession` runs before any other hook ([validated by: called before anything else](../../src/orchestration/chatOrchestrator.test.ts#L131)). +- `beforeUserInput` receives the content of the last user message ([validated by: receives the last user message content](../../src/orchestration/chatOrchestrator.test.ts#L199)). +- `beforeUserInput` can modify that message by returning a different string ([validated by: modifies user message when returning different value](../../src/orchestration/chatOrchestrator.test.ts#L214)). +- `afterUserInput` receives the message as `beforeUserInput` left it, not as the client sent it ([validated by: receives user message after any beforeUserInput modification](../../src/orchestration/chatOrchestrator.test.ts#L229)). +- `beforeModelResponse` can replace the system prompt, for example loading it from a `PromptStore` ([validated by: replaces system prompt with returned value](../../src/orchestration/chatOrchestrator.test.ts#L246)). - `beforeModelResponse` can also set `session.activePersona`, and every assistant entry created for that turn carries it, since the hook runs before the first one exists ([validated by: stamps 'p-two' when beforeModelResponse changes the session's activePersona from 'p-one' mid-turn](../../src/transport/ws/assistantPersona.test.ts#L98)). -- `afterModelResponse` receives the collected response text and the usage the provider reported ([validated by: receives collected response text and usage](../../src/orchestration/chatOrchestrator.test.ts#L262)). -- `afterModelResponse` does not run when the stream throws, so it never reports a response that was not delivered ([validated by: not called when stream throws](../../src/orchestration/chatOrchestrator.test.ts#L282)). -- `onError` receives the session and the error ([validated by: called with session and error when stream fails](../../src/orchestration/chatOrchestrator.test.ts#L304)). -- `onError` observes rather than handles: the error still propagates to the caller after it returns ([validated by: error still propagates after onError hook](../../src/orchestration/chatOrchestrator.test.ts#L326)). +- `afterModelResponse` receives the collected response text and the usage the provider reported ([validated by: receives collected response text and usage](../../src/orchestration/chatOrchestrator.test.ts#L261)). +- `afterModelResponse` does not run when the stream throws, so it never reports a response that was not delivered ([validated by: not called when stream throws](../../src/orchestration/chatOrchestrator.test.ts#L281)). +- `onError` receives the session and the error ([validated by: called with session and error when stream fails](../../src/orchestration/chatOrchestrator.test.ts#L303)). +- `onError` observes rather than handles: the error still propagates to the caller after it returns ([validated by: error still propagates after onError hook](../../src/orchestration/chatOrchestrator.test.ts#L325)). - `onToolBudgetExhausted` fires only when the model asks for a tool round the budget refuses: after the last provider call and before `afterModelResponse`, which then receives the hook's sentence at the end of the response text ([validated by: hands afterModelResponse text ending in the sentence, and the usage of a run without the hook](../../src/orchestration/toolBudget.test.ts#L214)). -- `afterSession` runs after everything else completes ([validated by: called after everything completes](../../src/orchestration/chatOrchestrator.test.ts#L171)). -- `afterSession` fires even on error ([validated by: called even when an error occurs](../../src/orchestration/chatOrchestrator.test.ts#L181)). +- `afterSession` runs after everything else completes ([validated by: called after everything completes](../../src/orchestration/chatOrchestrator.test.ts#L170)). +- `afterSession` fires even on error ([validated by: called even when an error occurs](../../src/orchestration/chatOrchestrator.test.ts#L180)). A hook receives the full `session` object, so it has access to `session.authHeaders?.authorization` (the caller's bearer token, when `auth.forwardHeaders` lets it through, as [the tool header allowlist spec](../hal-engine-tool-header-allowlist/spec.md) describes) and every user message verbatim through `session.entries`; the engine redacts nothing before calling a hook. Anything a hook persists becomes the deployer's own data-retention obligation. @@ -380,10 +380,10 @@ Here is what the message handler does when a tool call comes through: 2. The handler creates a `ToolEntry` and sends it to the client via `entry_upsert` 3. The orchestrator executes the tool (via `ToolRegistry.execute()`) 4. Tool results get added to the conversation as a `tool_result` message -5. If the tool returned `clientMessages`, they are forwarded to the frontend as-is, in one `tool_result` chunk ([validated by: forwards the client messages a tool returned as one tool_result chunk](../../src/orchestration/chatOrchestrator.test.ts#L525)) -6. If the tool set `suppressAssistantResponse`, the AI's next reply is kept in session context but hidden from the frontend ([validated by: asks for suppression when the tool says the reply is already handled](../../src/orchestration/chatOrchestrator.test.ts#L555)) -7. The orchestrator re-queries the AI provider with the updated messages, and the chunks of every round reach the client in order ([validated by: runs another round after a tool call and streams both rounds in order](../../src/orchestration/chatOrchestrator.test.ts#L490)) -8. This repeats until the budget is spent: at most `maxToolRounds` tool rounds are executed, and the provider is called at most `maxToolRounds + 1` times, so the default of 5 executes five rounds and makes six model calls, the last of which reads the fifth round's results. A round requested after that is not executed; see [the tool budget spec](../hal-engine-tool-budget/spec.md) ([validated by: executes 5 tool rounds and makes 6 provider calls at the default budget](../../src/orchestration/toolBudget.test.ts#L95), [validated by: stops asking for tools once maxToolRounds is spent](../../src/orchestration/chatOrchestrator.test.ts#L511)) +5. If the tool returned `clientMessages`, they are forwarded to the frontend as-is, in one `tool_result` chunk ([validated by: forwards the client messages a tool returned as one tool_result chunk](../../src/orchestration/chatOrchestrator.test.ts#L524)) +6. If the tool set `suppressAssistantResponse`, the AI's next reply is kept in session context but hidden from the frontend ([validated by: asks for suppression when the tool says the reply is already handled](../../src/orchestration/chatOrchestrator.test.ts#L554)) +7. The orchestrator re-queries the AI provider with the updated messages, and the chunks of every round reach the client in order ([validated by: runs another round after a tool call and streams both rounds in order](../../src/orchestration/chatOrchestrator.test.ts#L489)) +8. This repeats until the budget is spent: at most `maxToolRounds` tool rounds are executed, and the provider is called at most `maxToolRounds + 1` times, so the default of 5 executes five rounds and makes six model calls, the last of which reads the fifth round's results. A round requested after that is not executed; see [the tool budget spec](../hal-engine-tool-budget/spec.md) ([validated by: executes 5 tool rounds and makes 6 provider calls at the default budget](../../src/orchestration/toolBudget.test.ts#L95), [validated by: stops asking for tools once maxToolRounds is spent](../../src/orchestration/chatOrchestrator.test.ts#L510)) ### Whether a round continues @@ -392,9 +392,9 @@ Here is what the message handler does when a tool call comes through: - A `tool_use` stop naming no tool stops the loop, rather than re-querying with nothing to add ([validated by: stops when the model named no tool](../../src/orchestration/orchestratorHelpers.test.ts#L26)). - Any other stop reason ends the loop even when tool calls are pending ([validated by: stops when the model finished for a reason other than tool use](../../src/orchestration/orchestratorHelpers.test.ts#L30)). - The gate is the whole truth table of its three conditions: of the eight combinations of stop reason, pending calls and registry, only `tool_use` with a pending call and a registry continues ([validated by: continues for tool_use with a pending call and a registry alone, and stops for the other seven rows](../../src/orchestration/orchestratorHelpers.test.ts#L61)). -- Seen from outside, a scripted `tool_use` turn with no registry ends after one round: one provider call, no executor, and the next scripted round never streams ([validated by: ends a scripted tool_use turn after one round with no registry: one provider call, the tool_use and stop chunks alone](../../src/toolLoop.test.ts#L152)). -- A round that stops with `end_turn` despite carrying a tool call ends the turn with zero executions ([validated by: ends after one round with zero executions when the stop reason is end_turn despite a tool call: the second scripted round never streams](../../src/orchestration/chatOrchestrator.test.ts#L880)). -- A stream that ends without a `stop` chunk leaves the stop reason empty, so the turn ends after that round with zero executions and no second provider call ([validated by: ends after one round with zero executions when the stream ends without a stop chunk: the second scripted round never streams](../../src/orchestration/chatOrchestrator.test.ts#L884)). +- Seen from outside, a scripted `tool_use` turn with no registry ends after one round: one provider call, no executor, and the next scripted round never streams ([validated by: ends a scripted tool_use turn after one round with no registry: one provider call, the tool_use and stop chunks alone](../../src/toolLoop.test.ts#L172)). +- A round that stops with `end_turn` despite carrying a tool call ends the turn with zero executions ([validated by: ends after one round with zero executions when the stop reason is end_turn despite a tool call: the second scripted round never streams](../../src/orchestration/chatOrchestrator.test.ts#L884)). +- A stream that ends without a `stop` chunk leaves the stop reason empty, so the turn ends after that round with zero executions and no second provider call ([validated by: ends after one round with zero executions when the stream ends without a stop chunk: the second scripted round never streams](../../src/orchestration/chatOrchestrator.test.ts#L888)). - The helpers the round is built from are pure. `collectText` joins the text chunks of a stream and ignores every other type ([validated by: joins the text chunks of a stream in order and ignores every other chunk type](../../src/orchestration/orchestratorHelpers.test.ts#L87)). - `collectToolCalls` appends a `tool_use` chunk's call to the pending list and ignores the rest ([validated by: appends the call of a tool_use chunk to the pending list and leaves it alone for text and stop chunks](../../src/orchestration/orchestratorHelpers.test.ts#L95)). - `extractStopReason` reads a stop chunk and answers `null` otherwise ([validated by: reads the reason off a stop chunk and answers null for a text chunk](../../src/orchestration/orchestratorHelpers.test.ts#L108)). @@ -403,23 +403,23 @@ Here is what the message handler does when a tool call comes through: ### Across rounds - A tool call's `signature`, an opaque token its provider needs back, is copied onto its `tool_use` block for the next round, and a call without one gets no `signature` key ([validated by: puts sig-1 on the signed call's tool_use block and no signature key on the unsigned one](../../src/orchestration/orchestratorHelpers.test.ts#L36)). -- A call to a tool name the model invented is answered rather than raised, with a result naming it and the registered tools, and the turn goes on ([validated by: answers a call to a tool name the model invented, and the turn goes on to its answer](../../src/orchestration/chatOrchestrator.test.ts#L758)). -- So is a call whose executor throws: it gets a result saying the call failed, its message reaches only the log, as [tool-executor-throw](../hal-engine-tool-executor-throw/spec.md) specifies, and the turn goes on ([validated by: answers a call whose executor threw, and the turn goes on to its answer with afterModelResponse and no onError](../../src/orchestration/chatOrchestrator.test.ts#L771)). +- A call to a tool name the model invented is answered rather than raised, with a result naming it and the registered tools, and the turn goes on ([validated by: answers a call to a tool name the model invented, and the turn goes on to its answer](../../src/orchestration/chatOrchestrator.test.ts#L757)). +- So is a call whose executor throws: it gets a result saying the call failed, its message reaches only the log, as [tool-executor-throw](../hal-engine-tool-executor-throw/spec.md) specifies, and the turn goes on ([validated by: answers a call whose executor threw, and the turn goes on to its answer with afterModelResponse and no onError](../../src/orchestration/chatOrchestrator.test.ts#L770)). - A tool signals a failed call by returning `isError: true` beside its `result`, and the engine marks the failures it substitutes the same way, so the provider is told the call failed instead of leaving the model to infer it from prose; [the tool responses spec](../hal-engine-tool-responses/spec.md) gives each provider's encoding ([validated by: carries isError true from an executor's {result: 'lookup failed', isError: true} onto the tool_result the provider's next call gets](../../src/orchestration/toolResultError.test.ts#L62)). -- The loop ends as soon as a round stops with `end_turn`, and nothing further is asked of the provider ([validated by: stops after one round when nothing asked for a tool](../../src/orchestration/chatOrchestrator.test.ts#L502)). -- An `entry_upsert` a tool returns is appended to the session and its index rewritten to the position it actually landed in, because a tool cannot know how long the session already is ([validated by: appends an upserted entry to the session and rewrites its index to match](../../src/orchestration/chatOrchestrator.test.ts#L538)). -- The response text a hook sees is the text of every round joined, not only the last ([validated by: joins the text of every round, not only the last](../../src/orchestration/chatOrchestrator.test.ts#L567)). -- Usage an earlier round reported is kept when a later round reports none, so a tool round does not erase the token count ([validated by: keeps the usage an earlier round reported when a later round reports none](../../src/orchestration/chatOrchestrator.test.ts#L599)). -- The usage `afterModelResponse` receives is `totalUsage`: the sum of what every provider call in the turn reported, so a turn that ran tools counts every round rather than the last one alone ([validated by: sums the usage of every round, not only the last](../../src/orchestration/chatOrchestrator.test.ts#L617)). -- A round that reports no usage drops out of the sum rather than voiding it: the rounds that did report are still counted, and a turn where nothing reported reports nothing ([validated by: counts every reporting round even when a round between them reports none](../../src/orchestration/chatOrchestrator.test.ts#L639)). +- The loop ends as soon as a round stops with `end_turn`, and nothing further is asked of the provider ([validated by: stops after one round when nothing asked for a tool](../../src/orchestration/chatOrchestrator.test.ts#L501)). +- An `entry_upsert` a tool returns is appended to the session and its index rewritten to the position it actually landed in, because a tool cannot know how long the session already is ([validated by: appends an upserted entry to the session and rewrites its index to match](../../src/orchestration/chatOrchestrator.test.ts#L537)). +- The response text a hook sees is the text of every round joined, not only the last ([validated by: joins the text of every round, not only the last](../../src/orchestration/chatOrchestrator.test.ts#L566)). +- Usage an earlier round reported is kept when a later round reports none, so a tool round does not erase the token count ([validated by: keeps the usage an earlier round reported when a later round reports none](../../src/orchestration/chatOrchestrator.test.ts#L598)). +- The usage `afterModelResponse` receives is `totalUsage`: the sum of what every provider call in the turn reported, so a turn that ran tools counts every round rather than the last one alone ([validated by: sums the usage of every round, not only the last](../../src/orchestration/chatOrchestrator.test.ts#L616)). +- A round that reports no usage drops out of the sum rather than voiding it: the rounds that did report are still counted, and a turn where nothing reported reports nothing ([validated by: counts every reporting round even when a round between them reports none](../../src/orchestration/chatOrchestrator.test.ts#L638)). ### A round with more than one call -- A round carrying two calls runs both executors, and the next round's request carries one assistant message with two `tool_use` items and one user message with two `tool_result` items whose ids match, in the order the model asked ([validated by: runs both executors of a two-call round and sends the next round one assistant message with two tool_use items and one user message with two tool_result items whose ids match, in script order](../../src/toolLoop.test.ts#L43)). -- When one of two executors suppresses the answer, exactly one `suppress_output` chunk follows the round's `tool_result` chunk ([validated by: yields exactly one suppress_output chunk, after the tool_result chunk, when one of two executors suppresses](../../src/toolLoop.test.ts#L71)). -- Two `entry_upsert` messages from one round take sequential indices from the session's length at call time, and both entries are readable from the session afterwards ([validated by: gives two upserted entries sequential indices from the session length at call time, and both are readable from the session afterwards](../../src/toolLoop.test.ts#L93)). -- A round whose executors return no client messages yields no `tool_result` chunk at all ([validated by: yields no tool_result chunk for a round whose executors return no client messages](../../src/toolLoop.test.ts#L128)). -- The pending calls are a fresh list each round, so a call from one round never reaches the next round's request ([validated by: carries no pending call from one round into the next: three rounds leave two assistant messages with one tool_use item each, and each executor ran once](../../src/toolLoop.test.ts#L139)). +- A round carrying two calls runs both executors, and the next round's request carries one assistant message with two `tool_use` items and one user message with two `tool_result` items whose ids match, in the order the model asked ([validated by: runs both executors of a two-call round and sends the next round one assistant message with two tool_use items and one user message with two tool_result items whose ids match, in script order](../../src/toolLoop.test.ts#L63)). +- When one of two executors suppresses the answer, exactly one `suppress_output` chunk follows the round's `tool_result` chunk ([validated by: yields exactly one suppress_output chunk, after the tool_result chunk, when one of two executors suppresses](../../src/toolLoop.test.ts#L91)). +- Two `entry_upsert` messages from one round take sequential indices from the session's length at call time, and both entries are readable from the session afterwards ([validated by: gives two upserted entries sequential indices from the session length at call time, and both are readable from the session afterwards](../../src/toolLoop.test.ts#L113)). +- A round whose executors return no client messages yields no `tool_result` chunk at all ([validated by: yields no tool_result chunk for a round whose executors return no client messages](../../src/toolLoop.test.ts#L148)). +- The pending calls are a fresh list each round, so a call from one round never reaches the next round's request ([validated by: carries no pending call from one round into the next: three rounds leave two assistant messages with one tool_use item each, and each executor ran once](../../src/toolLoop.test.ts#L159)). The frontend shows a spinner on the last tool entry while the stream is still processing (`isProcessing` is `true`). The `stream_end` message clears the processing state, which hides the spinner. This works correctly even when tool suppression prevents assistant entries from reaching the frontend. See [tool-responses.md](../hal-engine-tool-responses/spec.md) for details on client messages and suppression. diff --git a/specs/hal-engine-config-hooks/spec.md b/specs/hal-engine-config-hooks/spec.md index 77b35a59..8dc0dba4 100644 --- a/specs/hal-engine-config-hooks/spec.md +++ b/specs/hal-engine-config-hooks/spec.md @@ -12,12 +12,12 @@ - A hook passed through `orchestrator.hooks` is forwarded to the orchestrator and fires ([validated by: forwards an orchestrator hook, so one passed through the config actually fires](../../src/config.test.ts#L27)). - An engine built with no `orchestrator` key at all still processes a message, so the field is optional in fact and not only in the type ([validated by: processes a message when the config declares no orchestrator key at all](../../src/config.test.ts#L103)). - A `beforeModelResponse` hook installed through config is handed the base system prompt built from `prompt`, proving the hook reaches the orchestrator and is wired into the prompt pipeline rather than only being stored on the config ([validated by: hands a config-installed beforeModelResponse hook the built base system prompt, not merely storing it](../../src/config.test.ts#L154)). -- The value the hook returns then replaces the system prompt the provider receives; this is exercised at the orchestrator layer because `HalEngineConfig.provider` takes a `ProviderConfig` and offers no seam to inject a recording provider through config ([validated by: replaces system prompt with returned value](../../src/orchestration/chatOrchestrator.test.ts#L247)). +- The value the hook returns then replaces the system prompt the provider receives; this is exercised at the orchestrator layer because `HalEngineConfig.provider` takes a `ProviderConfig` and offers no seam to inject a recording provider through config ([validated by: replaces system prompt with returned value](../../src/orchestration/chatOrchestrator.test.ts#L246)). - For a session that completes without error the hooks fire in the order `beforeSession` → `beforeUserInput` → `afterUserInput` → `beforeModelResponse` → `afterModelResponse` → `afterSession` ([validated by: fires the lifecycle hooks in documented order for a session that completes without error](../../src/config.test.ts#L188)). -- A `beforeSession` that throws reaches the caller as a rejection, and also runs `onError` with the thrown error and then `afterSession`, because the call sits inside the same `try`/`finally` as the rest of the turn. An `afterSession` hook called under this condition can infer the session never advanced past `beforeSession`: no user-input hooks ran, no model response was generated, and no tool rounds were started — a cleanup hook that allocates in `beforeSession` and frees in `afterSession` will therefore always see the allocation and the release on the same error path ([validated by: runs onError then afterSession when it throws, and still rejects to the caller](../../src/orchestration/chatOrchestrator.test.ts#L142)). -- `afterModelResponse` is called once per user message after the round loop exits and is passed `totalUsage` — the sum of the usage every provider call in the turn reported, not the last round alone ([validated by: sums the usage of every round, not only the last](../../src/orchestration/chatOrchestrator.test.ts#L617)). -- When no provider call in the turn reports usage, `afterModelResponse` is passed `undefined`, not a zeroed total ([validated by: hands afterModelResponse undefined, not a zeroed total, when no round of the turn reports usage](../../src/orchestration/chatOrchestrator.test.ts#L662)). -- Every rejection reaches `onError`: a value that is not already an `Error` is wrapped in one whose `cause` is that original value, so a hook typed against `Error` still fires and a hook that wants the raw value can still reach it, while the caller receives the original rejection unchanged ([validated by: wraps a non-Error rejection in an Error whose cause is the original and still rejects the caller with the original](../../src/orchestration/chatOrchestrator.test.ts#L337)). +- A `beforeSession` that throws reaches the caller as a rejection, and also runs `onError` with the thrown error and then `afterSession`, because the call sits inside the same `try`/`finally` as the rest of the turn. An `afterSession` hook called under this condition can infer the session never advanced past `beforeSession`: no user-input hooks ran, no model response was generated, and no tool rounds were started — a cleanup hook that allocates in `beforeSession` and frees in `afterSession` will therefore always see the allocation and the release on the same error path ([validated by: runs onError then afterSession when it throws, and still rejects to the caller](../../src/orchestration/chatOrchestrator.test.ts#L141)). +- `afterModelResponse` is called once per user message after the round loop exits and is passed `totalUsage` — the sum of the usage every provider call in the turn reported, not the last round alone ([validated by: sums the usage of every round, not only the last](../../src/orchestration/chatOrchestrator.test.ts#L616)). +- When no provider call in the turn reports usage, `afterModelResponse` is passed `undefined`, not a zeroed total ([validated by: hands afterModelResponse undefined, not a zeroed total, when no round of the turn reports usage](../../src/orchestration/chatOrchestrator.test.ts#L661)). +- Every rejection reaches `onError`: a value that is not already an `Error` is wrapped in one whose `cause` is that original value, so a hook typed against `Error` still fires and a hook that wants the raw value can still reach it, while the caller receives the original rejection unchanged ([validated by: wraps a non-Error rejection in an Error whose cause is the original and still rejects the caller with the original](../../src/orchestration/chatOrchestrator.test.ts#L336)). ## Engine startup diff --git a/specs/hal-engine-tool-executor-throw/spec.md b/specs/hal-engine-tool-executor-throw/spec.md index 9450af1e..12da6003 100644 --- a/specs/hal-engine-tool-executor-throw/spec.md +++ b/specs/hal-engine-tool-executor-throw/spec.md @@ -17,7 +17,7 @@ A tool call the registry could not run used to end the conversation. `ToolRegist - It is answered with a `ToolResponse` whose `result` names the attempted name and every registered tool, with no `clientMessages` and no `suppressAssistantResponse` ([validated by: answers an unregistered name with a result naming it and every registered tool, instead of throwing](../../src/orchestration/tools/registry.test.ts#L148)). - With no tool registered, the answer says so instead of ending in an empty list ([validated by: says that no tools are registered when the registry is empty, ending in no empty list](../../src/orchestration/tools/registry.test.ts#L155)). - The attempted name is cut to 64 characters, in the answer and in the one `warn` line, `tool not found` under `tool`, which is the only signal that a prompt or a registration is broken ([validated by: bounds an invented name to 64 characters in its answer and in its one warn line, under tool](../../src/orchestration/tools/registry.test.ts#L161)). -- The model reads the answer as that call's `tool_result`, and the turn goes on to its answer ([validated by: answers a call to a tool name the model invented, and the turn goes on to its answer](../../src/orchestration/chatOrchestrator.test.ts#L758)). +- The model reads the answer as that call's `tool_result`, and the turn goes on to its answer ([validated by: answers a call to a tool name the model invented, and the turn goes on to its answer](../../src/orchestration/chatOrchestrator.test.ts#L757)). ## An executor that throws @@ -27,12 +27,12 @@ A tool call the registry could not run used to end the conversation. `ToolRegist - A throw after the call's `ToolContext.signal` was aborted for any reason but a timeout, as when its turn is abandoned because the socket closed, is no tool failure: it is logged at `info` instead, once, under `tool` as `tool call abandoned`, with the tool's name and the error's type, and writes no `error` line ([validated by: logs a throw after its signal was aborted, not timed out, at info as tool call abandoned, and no error line](../../src/orchestration/tools/registry.test.ts#L211)). - A reason that is no error at all, such as a string or `null`, is no timeout either, so a throw after it is logged as abandoned too ([validated by: logs a throw as abandoned after a signal aborted with a reason that is no error, a string or null](../../src/orchestration/tools/registry.test.ts#L225)). - A throw while the signal is still live, or after it was aborted with a `TimeoutError`, the reason the orchestrator gives at a call's `toolTimeoutMs` deadline, is a failure, logged at `error` as `tool executor threw` ([validated by: keeps a throw at error as tool executor threw while its signal is live and after it timed out](../../src/orchestration/tools/registry.test.ts#L235)). -- The turn goes on to its answer and succeeds: `afterModelResponse` fires and `onError` does not ([validated by: answers a call whose executor threw, and the turn goes on to its answer with afterModelResponse and no onError](../../src/orchestration/chatOrchestrator.test.ts#L771)). +- The turn goes on to its answer and succeeds: `afterModelResponse` fires and `onError` does not ([validated by: answers a call whose executor threw, and the turn goes on to its answer with afterModelResponse and no onError](../../src/orchestration/chatOrchestrator.test.ts#L770)). ## Both - No value from the call's input reaches the answer or any log line, only the input's keys, which the registry already logs ([validated by: keeps every value of the call input out of the answer and the log, for an invented name and a throwing executor](../../src/orchestration/tools/registry.test.ts#L252)). -- A round in which some calls fail still returns every result: a working tool beside them runs once and its result reaches the model unchanged ([validated by: returns every result of a round in which calls fail, the working tool run once and its result unchanged](../../src/orchestration/chatOrchestrator.test.ts#L784)). +- A round in which some calls fail still returns every result: a working tool beside them runs once and its result reaches the model unchanged ([validated by: returns every result of a round in which calls fail, the working tool run once and its result unchanged](../../src/orchestration/chatOrchestrator.test.ts#L783)). - **NIS-2 Article 21, operational resilience.** Over the WebSocket, a turn whose tool name was invented streams its answer and ends with `stream_end`, never an `error` frame ([validated by: completes a turn whose tool name the model invented: its answer streams, then stream_end, with no error frame](../../src/transport/ws/messageHandler.test.ts#L874)). - So does a turn whose tool executor threw: one broken tool degrades one answer instead of ending the conversation ([validated by: completes a turn whose tool executor threw: its answer streams, then stream_end, with no error frame](../../src/transport/ws/messageHandler.test.ts#L880)). diff --git a/specs/hal-engine-tool-input-validation/spec.md b/specs/hal-engine-tool-input-validation/spec.md index a28f4437..f18b5213 100644 --- a/specs/hal-engine-tool-input-validation/spec.md +++ b/specs/hal-engine-tool-input-validation/spec.md @@ -23,7 +23,7 @@ ## Orchestrator integration -- A validation failure becomes a tool result (not a thrown error), so the orchestrator delivers the rejection text to the model and the model can attempt a corrected call; `DEFAULT_MAX_TOOL_ROUNDS = 5` is the bound that stops a retry cycle from spinning ([validated by: resolves processMessage and makes the validation-rejection text visible to the provider on the next round](../../src/orchestration/chatOrchestrator.test.ts#L678)). +- A validation failure becomes a tool result (not a thrown error), so the orchestrator delivers the rejection text to the model and the model can attempt a corrected call; `DEFAULT_MAX_TOOL_ROUNDS = 5` is the bound that stops a retry cycle from spinning ([validated by: resolves processMessage and makes the validation-rejection text visible to the provider on the next round](../../src/orchestration/chatOrchestrator.test.ts#L677)). ## Out of scope diff --git a/src/orchestration/chatOrchestrator.test.ts b/src/orchestration/chatOrchestrator.test.ts index 35775e8e..13faac70 100644 --- a/src/orchestration/chatOrchestrator.test.ts +++ b/src/orchestration/chatOrchestrator.test.ts @@ -13,7 +13,6 @@ import {AIError} from '../types/ai.js'; import type {ChatSession} from '../types/session.js'; import {PromptBuilder} from '../infrastructure/builders/promptBuilder.js'; import {ToolRegistry} from './tools/registry.js'; -import {twoTools} from './toolLoopTestSupport.js'; import type {ToolResponse} from './tools/registry.js'; import type {OutgoingMessage} from '../types/messages.js'; import type {SessionEntry} from '../types/session.js'; @@ -864,10 +863,15 @@ async function abortedMidStream(end: () => void): Promise<{outcome: string; call return {outcome: await outcomeOf(chunksFrom(stream)), callOrder}; } -// A scripted first round, then a second round that must never stream; what the loop did is the executions and the chunk types. +// A scripted first round, then a second round that must never stream; the registry holds the tool TOOL_CALL names and records every run. const gatedTurn = async (firstRound: MessageChunk[]) => { + const ran: string[] = []; const provider = scriptedProvider([firstRound, [{type: 'text', text: 'never'}, STOP_DONE]]); - const {registry, ran} = twoTools({first: {result: 'one'}, second: {result: 'two'}}); + const registry = new ToolRegistry(); + registry.register({name: 'lookup', description: 'looks things up', inputSchema: {type: 'object'}}, async () => { + ran.push('lookup'); + return {result: 'found'}; + }); const chunks = await chunksFrom( createChatOrchestrator(provider, promptBuilder, registry).processMessageStream(createSession('ask')) diff --git a/src/orchestration/toolLoopTestSupport.ts b/src/orchestration/toolLoopTestSupport.ts deleted file mode 100644 index a8e31e87..00000000 --- a/src/orchestration/toolLoopTestSupport.ts +++ /dev/null @@ -1,22 +0,0 @@ -import {ToolRegistry} from './tools/registry.js'; -import type {ToolResponse} from './tools/registry.js'; -import type {ToolCall} from '../types/ai.js'; - -/** Two registered tools whose executors record the order they started in; what each answers is the caller's. */ -export function twoTools(answers: {first: ToolResponse; second: ToolResponse}): { - registry: ToolRegistry; - ran: string[]; -} { - const ran: string[] = []; - const registry = new ToolRegistry(); - for (const name of ['first', 'second'] as const) { - registry.register({name, description: `the ${name} tool`, inputSchema: {type: 'object'}}, async () => { - ran.push(name); - return answers[name]; - }); - } - return {registry, ran}; -} - -export const CALL_A: ToolCall = {id: 'call-a', name: 'first', input: {a: 1}}; -export const CALL_B: ToolCall = {id: 'call-b', name: 'second', input: {b: 2}}; diff --git a/src/toolLoop.test.ts b/src/toolLoop.test.ts index d214fd13..424c4a77 100644 --- a/src/toolLoop.test.ts +++ b/src/toolLoop.test.ts @@ -1,10 +1,11 @@ import {createChatOrchestrator} from './orchestration/chatOrchestrator.js'; -import {CALL_A, CALL_B, twoTools} from './orchestration/toolLoopTestSupport.js'; +import {ToolRegistry} from './orchestration/tools/registry.js'; +import type {ToolResponse} from './orchestration/tools/registry.js'; import {PromptBuilder} from './infrastructure/builders/promptBuilder.js'; import {createMockProvider} from './providers/mock/mockProvider.js'; import type {MockTurn} from './providers/mock/mockProvider.js'; import {collectChunks} from './providers/providerTestSupport.js'; -import type {Message, SendMessageParams, ToolCallContent} from './types/ai.js'; +import type {Message, SendMessageParams, ToolCall, ToolCallContent} from './types/ai.js'; import type {ChatSession, SessionEntry} from './types/session.js'; // specs/hal-engine-architecture § Tool Execution Loop: the cases the package's own scripted mock can drive. @@ -24,6 +25,25 @@ const upserted = (content: string): SessionEntry => ({ isStreaming: false, }); +/** Two registered tools whose executors record the order they started in; what each answers is the caller's. */ +function twoTools(answers: {first: ToolResponse; second: ToolResponse}): { + registry: ToolRegistry; + ran: string[]; +} { + const ran: string[] = []; + const registry = new ToolRegistry(); + for (const name of ['first', 'second'] as const) { + registry.register({name, description: `the ${name} tool`, inputSchema: {type: 'object'}}, async () => { + ran.push(name); + return answers[name]; + }); + } + return {registry, ran}; +} + +const CALL_A: ToolCall = {id: 'call-a', name: 'first', input: {a: 1}}; +const CALL_B: ToolCall = {id: 'call-b', name: 'second', input: {b: 2}}; + const toolUseIdsOf = (message: Message): string[][] => { if (message.role !== 'assistant' || !Array.isArray(message.content)) return []; const toolUses = message.content.filter((block): block is ToolCallContent => block.type === 'tool_use'); diff --git a/tsconfig.build.json b/tsconfig.build.json index 2444a2bc..a421e6f3 100644 --- a/tsconfig.build.json +++ b/tsconfig.build.json @@ -14,7 +14,6 @@ "**/*.test.ts", "src/providers/providerTestSupport.ts", "src/orchestration/scriptedProviderTestSupport.ts", - "src/orchestration/toolLoopTestSupport.ts", "src/infrastructure/writeSignalTestSupport.ts", "src/infrastructure/stores/mongo/mongoTestSupport.ts", "src/shared/logCaptureTestSupport.ts",