fix(tools): the tool-call service makes its own checks, for any caller #106

2026-11-12T15:51:54Z

james.maes commented

2026-11-12 15:51:54 +00:00

What

engine.ToolCallService trusted its caller for several checks. The chat's turn engine made them around the service; a caller with no chat turn (the MCP endpoint, later) would have skipped them. This PR makes the service safe for any caller. It does not build MCP.

The rule that now holds for any caller: a tool call runs as the signed-in person with their current permissions and budget, never as the assistant; a refusal happens before any data action; every call is logged with redacted input.

The review findings, one by one

#	Finding	Change	Test
1	`execute` never checked the offer; `KernelToolFamily.execute` has no policy check	`execute` builds the offer from the request's policy and session (once per scope) and refuses a tool that is not offered now, with the refusal of an unknown tool	`ToolCallServiceTest.aToolThatIsNotOfferedIsRefusedByTheServiceItself`
2	A null policy switched the tool switch off	The service refuses a request without a policy; `UiApiToolbox`, `KnowledgeTool` and `BulkLoadTools` refuse when handed a context without one	`ToolCallServiceTest.aCallWithoutAPolicyIsRefused`
3	`session` and `context` were independent	The service refuses a request whose context was made for another session; `ConfirmedToolCall` carries the confirming session and is checked the same way	`ToolCallServiceTest.aCallWhoseSessionAndContextAreTwoPeopleIsRefused`
4	Logging and redaction were the caller's job	New `executeAndLog`; redaction and the row moved from `TurnEngine` into the service (`log(request, outcome)`); the `present_results` special case is `AssistantTool.inputForLog`	`ToolCallServiceTest.executeAndLogWritesTheRowWithRedactedInputAlsoForARefusedCall`, `aCallThatNamesNoPersonIsRefusedAndHasNoRow`
5	The javadoc claimed checks that were not made	True now (finding 1), and the javadoc lists what the service enforces and what a caller supplies	(documentation)
6	The confirm route checked neither the kill switch nor the tool switch	Both are checked before the pending action is claimed (503 `KILL_SWITCH`, 403), and again inside `executeConfirmed`	`ToolCallServiceTest.aConfirmationWhileTheKillSwitchIsOnIsRefusedAndTheActionStaysPending`, `aConfirmationOfAToolThePolicySwitchedOffIsRefusedAndTheActionStaysPending`
7	`ToolRegistry.discover` scanned the classpath	Explicit list `AssistantRuntime.ownToolFamilies()` plus `AssistantQBitConfig.withToolFamily`; names logged once at startup; a duplicate name or an empty registry stops the start	`ToolRegistryTest.aFamilyThatIsOnlyOnTheClasspathIsNotRegisteredOrOffered`, `aHostRegistersAFamilyOnTheConfig`, `onlyTheFamiliesGivenAreRegistered`, `aRegistryRefusesWhatItCannotHold`
8	`Map.of` schemas serialise in a per-JVM order	`ToolRegistry.definitions` sorts the keys of every object of an input schema (lists keep their order)	`ToolRegistryTest.theToolListIsTheSameBytesWhateverOrderASchemaWasBuiltIn`
9	Access, kill switch and hard budget lived only in the chat turn	The service checks all three itself; the rest is listed in its javadoc	`ToolCallServiceTest.assistantAccessIsRequiredByTheService`, `theKillSwitchIsCheckedByTheService`, `theHardBudgetIsCheckedByTheService`

The behaviours these tests pin were also asserted against origin/develop (21f634c) with a scratch probe written to develop's API: all 13 assertions failed there.

What a caller with no chat turn still has to supply

From the service's javadoc:

rate limiting (the chat: RateLimiter, turns per minute and concurrent turns per person);
how many calls one request may make, how many at once and how long one may take (the chat: maxToolCallsPerTurn, maxParallelToolCalls, toolCallTimeoutSeconds, the turn's deadline), and the row of a call it stopped itself (log with the result it gave instead);
classification: keeping the scope's data classification up to date from every result and persisting it (the chat: TurnClassification.observe and persist), and a truthful ToolCallScope.hipaaRoute();
the scope: one per request, never shared by two people, with the client channel its calls are logged under;
the session: the signed-in person's own, from the route's session setup. The service compares the context's session id with the session; it cannot check request credentials the context forwards to the UI API;
the policy's tool settings in force for the request. The service refuses a missing policy, but does not compare the one it is given with the administrator's row.

Decisions a reviewer should look at

The turn engine does not call executeAndLog itself. It runs calls in parallel under its own time limit and writes the rows in call order afterwards, and it also writes rows for calls that never finished (timed out, stopped). It uses the two halves of the same path, execute and log(request, outcome); the redaction, outcome, summary, person and channel are the service's in both. A new ArchUnit rule (onlyTheTurnEngineRunsACallAndLogsItInTwoSteps) keeps every other class on executeAndLog.
Sorting is of every key, not only of Map.of maps. BulkLoadTools copies a Map.of into a LinkedHashMap, which keeps the random order, so "sort only unordered maps" would not have been enough. The visible effect is that properties are listed alphabetically in every tool's schema, and the cached request prefix changes once.
The hard budget is checked when a scope's first call arrives, not on every call, and it only reads (checkConfiguredBudget and the new BudgetService.stoppedReason; BudgetService.evaluate stores a budget event and raises an alert, which would have taken a warning away from the model call that reports it).
A confirmed write is not stopped by the hard budget. It spends no model budget, and the confirm route never checked it.
ContextBuilder.tools(null) still lists every tool. ChatToolSetTest pins that (scenario everythingNoPolicy). It only lists; nothing runs without a policy.

Stricter than before, in the chat

A turn whose first model call takes the person over a budget row's limit (assistant_budget, state STOPPED) now has that response's tool calls refused (assistant:budget). Before, they ran and the next model call stopped the turn. The policy's own hard limits already stopped the turn before its tools.
A tool call that starts after the kill switch went on, and before the running turn's poll noticed it, is refused (assistant:killSwitch) instead of run. The turn is cancelled either way.

Existing tests

ChatToolSetTest and ToolCallChecksTest: unchanged, pass.
ToolRegistryTest: familiesUnderTheToolsPackageAreFound asserted the classpath scan and is replaced by onlyTheFamiliesGivenAreRegistered. aToolThatProposesNothingCannotBeConfirmed passes the confirming session to ConfirmedToolCall; its expectations are unchanged.
Setup only, no expectation changed: AssistantChannelWallTest, BulkLoadToolsTest, and the tool(...) helpers of UiApiToolsSecurityTest, AnalysisSaveTest and AnalysisToolsTest handed a tool a null policy; they hand it ToolPolicy.defaults().

Build

./mvnw --batch-mode --no-transfer-progress -s <settings> -T1C clean verify on the head commit: BUILD SUCCESS, all 8 modules, exit code 0 (qbit-assistant: 446 tests, 0 failures, 0 errors; enforcer guardrails, ArchUnit and the JaCoCo check passed).

🤖 Generated with Claude Code

## What `engine.ToolCallService` trusted its caller for several checks. The chat's turn engine made them around the service; a caller with no chat turn (the MCP endpoint, later) would have skipped them. This PR makes the service safe for any caller. It does not build MCP. The rule that now holds for any caller: a tool call runs as the signed-in person with their current permissions and budget, never as the assistant; a refusal happens before any data action; every call is logged with redacted input. ## The review findings, one by one | # | Finding | Change | Test | | --- | --- | --- | --- | | 1 | `execute` never checked the offer; `KernelToolFamily.execute` has no policy check | `execute` builds the offer from the request's policy and session (once per scope) and refuses a tool that is not offered now, with the refusal of an unknown tool | `ToolCallServiceTest.aToolThatIsNotOfferedIsRefusedByTheServiceItself` | | 2 | A null policy switched the tool switch off | The service refuses a request without a policy; `UiApiToolbox`, `KnowledgeTool` and `BulkLoadTools` refuse when handed a context without one | `ToolCallServiceTest.aCallWithoutAPolicyIsRefused` | | 3 | `session` and `context` were independent | The service refuses a request whose context was made for another session; `ConfirmedToolCall` carries the confirming session and is checked the same way | `ToolCallServiceTest.aCallWhoseSessionAndContextAreTwoPeopleIsRefused` | | 4 | Logging and redaction were the caller's job | New `executeAndLog`; redaction and the row moved from `TurnEngine` into the service (`log(request, outcome)`); the `present_results` special case is `AssistantTool.inputForLog` | `ToolCallServiceTest.executeAndLogWritesTheRowWithRedactedInputAlsoForARefusedCall`, `aCallThatNamesNoPersonIsRefusedAndHasNoRow` | | 5 | The javadoc claimed checks that were not made | True now (finding 1), and the javadoc lists what the service enforces and what a caller supplies | (documentation) | | 6 | The confirm route checked neither the kill switch nor the tool switch | Both are checked before the pending action is claimed (503 `KILL_SWITCH`, 403), and again inside `executeConfirmed` | `ToolCallServiceTest.aConfirmationWhileTheKillSwitchIsOnIsRefusedAndTheActionStaysPending`, `aConfirmationOfAToolThePolicySwitchedOffIsRefusedAndTheActionStaysPending` | | 7 | `ToolRegistry.discover` scanned the classpath | Explicit list `AssistantRuntime.ownToolFamilies()` plus `AssistantQBitConfig.withToolFamily`; names logged once at startup; a duplicate name or an empty registry stops the start | `ToolRegistryTest.aFamilyThatIsOnlyOnTheClasspathIsNotRegisteredOrOffered`, `aHostRegistersAFamilyOnTheConfig`, `onlyTheFamiliesGivenAreRegistered`, `aRegistryRefusesWhatItCannotHold` | | 8 | `Map.of` schemas serialise in a per-JVM order | `ToolRegistry.definitions` sorts the keys of every object of an input schema (lists keep their order) | `ToolRegistryTest.theToolListIsTheSameBytesWhateverOrderASchemaWasBuiltIn` | | 9 | Access, kill switch and hard budget lived only in the chat turn | The service checks all three itself; the rest is listed in its javadoc | `ToolCallServiceTest.assistantAccessIsRequiredByTheService`, `theKillSwitchIsCheckedByTheService`, `theHardBudgetIsCheckedByTheService` | The behaviours these tests pin were also asserted against `origin/develop` (21f634c) with a scratch probe written to develop's API: all 13 assertions failed there. ## What a caller with no chat turn still has to supply From the service's javadoc: - rate limiting (the chat: `RateLimiter`, turns per minute and concurrent turns per person); - how many calls one request may make, how many at once and how long one may take (the chat: `maxToolCallsPerTurn`, `maxParallelToolCalls`, `toolCallTimeoutSeconds`, the turn's deadline), and the row of a call it stopped itself (`log` with the result it gave instead); - classification: keeping the scope's data classification up to date from every result and persisting it (the chat: `TurnClassification.observe` and `persist`), and a truthful `ToolCallScope.hipaaRoute()`; - the scope: one per request, never shared by two people, with the client channel its calls are logged under; - the session: the signed-in person's own, from the route's session setup. The service compares the context's session id with the session; it cannot check request credentials the context forwards to the UI API; - the policy's tool settings in force for the request. The service refuses a missing policy, but does not compare the one it is given with the administrator's row. ## Decisions a reviewer should look at - **The turn engine does not call `executeAndLog` itself.** It runs calls in parallel under its own time limit and writes the rows in call order afterwards, and it also writes rows for calls that never finished (timed out, stopped). It uses the two halves of the same path, `execute` and `log(request, outcome)`; the redaction, outcome, summary, person and channel are the service's in both. A new ArchUnit rule (`onlyTheTurnEngineRunsACallAndLogsItInTwoSteps`) keeps every other class on `executeAndLog`. - **Sorting is of every key, not only of `Map.of` maps.** `BulkLoadTools` copies a `Map.of` into a `LinkedHashMap`, which keeps the random order, so "sort only unordered maps" would not have been enough. The visible effect is that properties are listed alphabetically in every tool's schema, and the cached request prefix changes once. - **The hard budget is checked when a scope's first call arrives**, not on every call, and it only reads (`checkConfiguredBudget` and the new `BudgetService.stoppedReason`; `BudgetService.evaluate` stores a budget event and raises an alert, which would have taken a warning away from the model call that reports it). - **A confirmed write is not stopped by the hard budget.** It spends no model budget, and the confirm route never checked it. - **`ContextBuilder.tools(null)` still lists every tool.** `ChatToolSetTest` pins that (scenario `everythingNoPolicy`). It only lists; nothing runs without a policy. ## Stricter than before, in the chat - A turn whose first model call takes the person over a budget row's limit (`assistant_budget`, state STOPPED) now has that response's tool calls refused (`assistant:budget`). Before, they ran and the next model call stopped the turn. The policy's own hard limits already stopped the turn before its tools. - A tool call that starts after the kill switch went on, and before the running turn's poll noticed it, is refused (`assistant:killSwitch`) instead of run. The turn is cancelled either way. ## Existing tests - `ChatToolSetTest` and `ToolCallChecksTest`: unchanged, pass. - `ToolRegistryTest`: `familiesUnderTheToolsPackageAreFound` asserted the classpath scan and is replaced by `onlyTheFamiliesGivenAreRegistered`. `aToolThatProposesNothingCannotBeConfirmed` passes the confirming session to `ConfirmedToolCall`; its expectations are unchanged. - Setup only, no expectation changed: `AssistantChannelWallTest`, `BulkLoadToolsTest`, and the `tool(...)` helpers of `UiApiToolsSecurityTest`, `AnalysisSaveTest` and `AnalysisToolsTest` handed a tool a null policy; they hand it `ToolPolicy.defaults()`. ## Build `./mvnw --batch-mode --no-transfer-progress -s <settings> -T1C clean verify` on the head commit: **BUILD SUCCESS**, all 8 modules, exit code 0 (qbit-assistant: 446 tests, 0 failures, 0 errors; enforcer guardrails, ArchUnit and the JaCoCo check passed). 🤖 Generated with [Claude Code](https://claude.com/claude-code)

james.maes added 1 commit 2026-11-12 15:51:54 +00:00

fix(tools): the tool-call service makes its own checks, for any caller

maven / build (push) Successful in 6m45s

Details

build / maven (push) Successful in 6m45s

Details

d42c536dcf

engine.ToolCallService trusted its caller for several checks. The chat's
turn engine made them around the service; a caller with no chat turn (an
MCP endpoint, later) would have skipped them. The rule that now holds for
any caller: a tool call runs as the signed-in person with their current
permissions and budget, a refusal happens before any data action, and the
call is logged with redacted input.

 1. execute refuses a tool the person is not offered now. It builds the
    offer from the request's policy and session (once per scope). Before,
    only the turn engine's allowed-tools set did, and navigate and
    search_help ran although the policy had switched them off.
 2. A request without a policy is refused. UiApiToolbox, KnowledgeTool
    and BulkLoadTools read a missing policy as "every tool"; they refuse.
 3. A request whose context was made for another session than its own is
    refused, for a call and for a confirmation. ConfirmedToolCall carries
    the confirming session.
 4. New executeAndLog: runs the call and writes the assistant_tool_call
    row, redacting the input itself, also for a refused call. Redaction
    and the row moved out of TurnEngine (redactInput is gone; what a
    presentation logs is AssistantTool.inputForLog). The turn engine runs
    its calls in parallel under its own time limit and writes rows in
    call order, so it uses the two halves, execute and
    log(request, outcome); an ArchUnit rule keeps every other class on
    executeAndLog. The chat's rows are unchanged.
 5. The service's javadoc says what it enforces and what a caller still
    supplies (rate limiting, per-request call limits and timeouts,
    classification).
 6. The confirm route refuses while the kill switch is on (503
    KILL_SWITCH) and when the policy no longer allows the tool (403),
    before the pending action is claimed, so it stays pending.
 7. Tool families are registered explicitly: the QBit's own list
    (AssistantRuntime.ownToolFamilies) plus
    AssistantQBitConfig.withToolFamily for a host or another QBit. The
    classpath scan is gone. The registered names are logged once at
    startup; a duplicate name or an empty registry stops the start.
 8. ToolRegistry.definitions sorts the keys of every object of an input
    schema, so two JVMs send the same bytes (a Map.of schema iterated in
    a per-JVM order, and the provider mappings serialise in iteration
    order).
 9. The service itself requires assistant access, checks the kill switch
    (fresh) and the hard budget (the policy's hard limits and a stopped
    budget row, read without a budget event or alert, when a scope's
    first call arrives). New BudgetService.stoppedReason.

Tests: ToolCallServiceTest (fixes 1 to 4, 6, 9, called with no turn),
ToolRegistryTest (7, 8), ToolCallArchitectureTest (the new rule).
ChatToolSetTest and ToolCallChecksTest pass unchanged. Test helpers that
handed a tool a null policy now hand it the defaults.

Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>

james.maes merged commit 441e8218fd into develop

2026-11-12 15:56:46 +00:00

james.maes referenced this pull request

2026-11-12 16:38:07 +00:00

fix(tools): the tool-call service owns the policy, the identity and the confirmation #107

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: QRun-IO/qbit-assistant#106