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

2026-11-12T16:38:07Z

james.maes commented

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

Closes what an independent security review found in engine.ToolCallService after #106. The chat's turn engine is the only caller today and builds everything correctly, so none of this was reachable; it is closed before an MCP endpoint calls the service.

The rule: every 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 that ran has a log row with redacted input.

What changed, numbered as in the review

A caller cannot supply a policy. The service reads it (PolicyService.current(false).tools(), narrowed for a scheduled task's conversation as the chat narrows it) and builds the tool Context itself. A caller hands it a tools.registry.ToolCall (person, id, name, input). Every change always shows a confirmation card (owner, 2026-11-12): ToolPolicy.writesRequireConfirmation is removed and UiApiToolbox always proposes. The assistant_policy.writesRequireConfirmation column stays and is no longer read. No migration.
Identity cannot be mixed. New SignedInPerson binds a session to the credentials forwarded to the UI API. No public constructor: ofRequest(Context) (only AssistantSpecs, ArchUnit) or ofSession(session). The context's gateway is always runtime.getUiApiGateway(). AssistantApi.withCredentials and TurnEngine.withCredentials are gone.
The hard budget is read on every call, not once per scope. executeAndLog(call, conversationId, hipaaRoute) makes the scope itself.
confirm(person, pendingActionId) replaces executeConfirmed, confirmationRefusal and logConfirmed. The service loads the stored action of that person, checks it, claims it once, runs it, marks it and writes the row. AssistantApi.confirmAction is a thin caller and the only one (ArchUnit).
The toolboxes cannot be called around the service. Their entry points are package-private, the runtime has no getter for a toolbox (each family is handed its own), and ArchUnit forbids calls into tools.. from outside it except through the registry. The assistant's own reads (starter prompts, knowledge re-read) use the new read-only UiApiToolbox.read.
A call that ran always has its row. TurnEngine.executeTools writes each started call's row in a per-call finally: after an interruption of the engine, after a runtime exception while a result is processed or logged, and for a call cut short by its time limit (the real outcome when the worker finished). A row that cannot be written for a call that changed data is logged at ERROR.
A confirmed action never sticks in RUNNING. Whatever fails after the claim ends in FAILED with a row.
The scope is checked. The conversation and turn a scope names must be the call's person's; the row's client channel is the service's. hipaaRoute stays the caller's, and the javadoc says why.
AssistantQBitConfig.withToolFamily throws once the QBit is produced.

What a host notices

Writes require confirmation on the policy row does nothing any more; a host that had switched it off gets the card again.
Java API: AssistantApi.withCredentials, AssistantRuntime.getToolbox() / getKnowledgeTool() / getBulkLoadTools() and ToolCallScope.clientChannel() are gone; ToolPolicy has three components. A host's own ToolFamily compiles unchanged.
No schema change.

Tests

ToolCallServiceTest: one per review item, on the service directly.
ToolCallRowsTest (item 6): all three fail on develop, pass here.
ToolCallArchitectureTest: the rules for items 2, 4 and 5 (checked not to be vacuous).
ChatToolSetTest and ToolCallChecksTest pass with unchanged expectations (ChatToolSetTest builds a ToolPolicy without the removed argument).
Changed expectations: UiApiToolsSecurityTest.testWriteDeniedIsTheSameDenial and testDenialMatrix ran writes with confirmation off; they now propose and confirm. ToolCallServiceTest.theHardBudgetIsCheckedByTheService asserted that an admitted scope keeps running after the hard limit; it now asserts the refusal.

Build: ./mvnw --batch-mode --no-transfer-progress -s <settings> -T1C clean verify green on this head (all eight reactor modules, 584 tests, 0 failures, JaCoCo gates met).

Not closed here

The service cannot verify that forwarded credentials belong to the session; it relies on SignedInPerson.ofRequest being the only place they are bound. An MCP route has to be added to that ArchUnit rule deliberately.
The confirm route never compared the record with the proposal, and still does not.
A cut-short call whose worker ignores the interruption for more than a second still gets a "Timed out" row.
"Loud" is an ERROR log line; the QBit has no operator alert for this.

🤖 Generated with Claude Code

Closes what an independent security review found in `engine.ToolCallService` after #106. The chat's turn engine is the only caller today and builds everything correctly, so none of this was reachable; it is closed before an MCP endpoint calls the service. The rule: every 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 that ran has a log row with redacted input. ## What changed, numbered as in the review 1. **A caller cannot supply a policy.** The service reads it (`PolicyService.current(false).tools()`, narrowed for a scheduled task's conversation as the chat narrows it) and builds the tool `Context` itself. A caller hands it a `tools.registry.ToolCall` (person, id, name, input). **Every change always shows a confirmation card** (owner, 2026-11-12): `ToolPolicy.writesRequireConfirmation` is removed and `UiApiToolbox` always proposes. The `assistant_policy.writesRequireConfirmation` column stays and is no longer read. No migration. 2. **Identity cannot be mixed.** New `SignedInPerson` binds a session to the credentials forwarded to the UI API. No public constructor: `ofRequest(Context)` (only `AssistantSpecs`, ArchUnit) or `ofSession(session)`. The context's gateway is always `runtime.getUiApiGateway()`. `AssistantApi.withCredentials` and `TurnEngine.withCredentials` are gone. 3. **The hard budget is read on every call**, not once per scope. `executeAndLog(call, conversationId, hipaaRoute)` makes the scope itself. 4. **`confirm(person, pendingActionId)`** replaces `executeConfirmed`, `confirmationRefusal` and `logConfirmed`. The service loads the stored action of that person, checks it, claims it once, runs it, marks it and writes the row. `AssistantApi.confirmAction` is a thin caller and the only one (ArchUnit). 5. **The toolboxes cannot be called around the service.** Their entry points are package-private, the runtime has no getter for a toolbox (each family is handed its own), and ArchUnit forbids calls into `tools..` from outside it except through the registry. The assistant's own reads (starter prompts, knowledge re-read) use the new read-only `UiApiToolbox.read`. 6. **A call that ran always has its row.** `TurnEngine.executeTools` writes each started call's row in a per-call `finally`: after an interruption of the engine, after a runtime exception while a result is processed or logged, and for a call cut short by its time limit (the real outcome when the worker finished). A row that cannot be written for a call that changed data is logged at ERROR. 7. **A confirmed action never sticks in RUNNING.** Whatever fails after the claim ends in FAILED with a row. 8. **The scope is checked.** The conversation and turn a scope names must be the call's person's; the row's client channel is the service's. `hipaaRoute` stays the caller's, and the javadoc says why. 9. `AssistantQBitConfig.withToolFamily` throws once the QBit is produced. ## What a host notices - **Writes require confirmation** on the policy row does nothing any more; a host that had switched it off gets the card again. - Java API: `AssistantApi.withCredentials`, `AssistantRuntime.getToolbox()` / `getKnowledgeTool()` / `getBulkLoadTools()` and `ToolCallScope.clientChannel()` are gone; `ToolPolicy` has three components. A host's own `ToolFamily` compiles unchanged. - No schema change. ## Tests - `ToolCallServiceTest`: one per review item, on the service directly. - `ToolCallRowsTest` (item 6): all three fail on `develop`, pass here. - `ToolCallArchitectureTest`: the rules for items 2, 4 and 5 (checked not to be vacuous). - `ChatToolSetTest` and `ToolCallChecksTest` pass with unchanged expectations (`ChatToolSetTest` builds a `ToolPolicy` without the removed argument). - Changed expectations: `UiApiToolsSecurityTest.testWriteDeniedIsTheSameDenial` and `testDenialMatrix` ran writes with confirmation off; they now propose and confirm. `ToolCallServiceTest.theHardBudgetIsCheckedByTheService` asserted that an admitted scope keeps running after the hard limit; it now asserts the refusal. Build: `./mvnw --batch-mode --no-transfer-progress -s <settings> -T1C clean verify` green on this head (all eight reactor modules, 584 tests, 0 failures, JaCoCo gates met). ## Not closed here - The service cannot verify that forwarded credentials belong to the session; it relies on `SignedInPerson.ofRequest` being the only place they are bound. An MCP route has to be added to that ArchUnit rule deliberately. - The confirm route never compared the record with the proposal, and still does not. - A cut-short call whose worker ignores the interruption for more than a second still gets a "Timed out" row. - "Loud" is an ERROR log line; the QBit has no operator alert for this. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

james.maes added 1 commit 2026-11-12 16:38:07 +00:00

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

maven / build (push) Successful in 6m51s

Details

build / maven (push) Successful in 6m51s

Details

7c9176fcc8

An independent security review of engine.ToolCallService (after PR 106)
found what a caller could still hand it. The chat's turn engine is the
only caller today and builds everything correctly, so none of it was
reachable; it is closed before an MCP endpoint calls the service. The
rule: every 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 that ran has a log row with redacted input.

Numbered as in the review:

 1. A caller cannot supply a policy. The service reads it
    (PolicyService.current(false).tools(), narrowed for a scheduled
    task's conversation) and builds the tool Context itself; a caller
    hands it a tools.registry.ToolCall (person, id, name, input).
    Every change always shows a confirmation card (owner, 2026-11-12):
    ToolPolicy.writesRequireConfirmation is removed and UiApiToolbox
    always proposes. The assistant_policy column stays and is no longer
    read; no migration.
 2. New SignedInPerson binds a session to the credentials forwarded to
    the UI API. No public constructor: ofRequest(Context), only for
    AssistantSpecs (ArchUnit), or ofSession(session). The context's
    gateway is always runtime.getUiApiGateway(). AssistantApi and
    TurnEngine lose withCredentials.
 3. The hard budget is read on every call, not once per scope.
    executeAndLog(call, conversationId, hipaaRoute) makes the scope.
 4. confirm(person, pendingActionId) replaces executeConfirmed,
    confirmationRefusal and logConfirmed: the service loads the stored
    action of that person, checks it, claims it once, runs it, marks it
    and writes the row. AssistantApi.confirmAction is a thin caller and
    the only one (ArchUnit).
 5. The toolboxes cannot be called around the service: their entry
    points are package-private, the runtime has no getter for a toolbox
    (each family is handed its own), and ArchUnit forbids calls into
    tools.. from outside it except through the registry. The assistant's
    own reads (starter prompts, knowledge re-read) use the new read-only
    UiApiToolbox.read.
 6. TurnEngine.executeTools writes each started call's row in a per-call
    finally: after an interruption of the engine, after a runtime
    exception while a result is processed or logged, and for a call cut
    short by its time limit, with the real outcome when the worker
    finished. A row that cannot be written for a call that changed data
    is logged at ERROR.
 7. Whatever fails after a confirmation's claim ends in FAILED with a
    row, never in RUNNING.
 8. The service checks that the scope's conversation and turn belong to
    the call's person and works out the row's client channel itself
    (ToolCallScope.clientChannel is gone). hipaaRoute stays the
    caller's; the javadoc says why.
 9. AssistantQBitConfig.withToolFamily throws once the QBit is produced.

Tests: ToolCallServiceTest (one per item, on the service directly),
ToolCallRowsTest (item 6; all three fail on develop), and
ToolCallArchitectureTest (items 2, 4, 5). ChatToolSetTest and
ToolCallChecksTest pass with unchanged expectations.
UiApiToolsSecurityTest's two tests that ran writes with confirmation off
now propose and confirm.

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

james.maes merged commit 3802c5be6c into develop

2026-11-12 16:44:46 +00:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: QRun-IO/qbit-assistant#107