refactor(tools): one tool registry (no behaviour change); build against QQQ 4.1.0-RC.32 #105

Merged

james.maes merged 2 commits from feat/tool-registry into develop 2026-11-12 15:15:52 +00:00

Conversation 0 Commits 2 Files Changed 29

+3093 -196

james.maes commented 2026-11-12 00:02:25 +00:00

Owner

Copy link

Copy source

Packages A0 and A1 of the approved design docs/actions-and-mcp.md (draft PR #104, section 12.2), as one pull request with one commit each.

A0: build against QQQ 4.1.0-RC.32 (f848092)

qqq.snapshot.version moves from 4.1.0-RC.25 to 4.1.0-RC.32, the version GG Forge pins. Root pom.xml only: nothing else had to change. The whole suite was green on this commit alone before the refactor started (541 tests), and the chat's tool list is byte for byte the same under RC.25 and RC.32.

A1: one tool registry, no behaviour change (f035d63)

Every tool is registered once, in tools.registry.ToolRegistry: its name, kind (READ, PROPOSE, DISPLAY), the checks it runs, its definition (description, input schema) and how one call of it runs. "Run one tool call for this person under this policy, and log it" moves out of the turn engine into engine.ToolCallService, which the confirm route uses too.

UiApiToolbox, AnalysisTools, BulkLoadTools, KnowledgeTool, PresentResultsTool and the core kernel are untouched apart from one added static method (UiApiToolbox.notAWriteTool, the text executeConfirmedWrite already answers). They keep all of their behaviour; five small family classes register their tools.

What moved where

Before	After
ContextBuilder.tools(policy, turnId): one block per family, into a TreeMap by name	ToolRegistry.definitions(new ToolOffer(policy, session, turnId)); each block is that family's AssistantTool.definition(offer). The registry is sorted by name, so the order is the same.
ContextBuilder.isStrict	ToolRegistry.isStrict (same expression)
TurnEngine.executeTools: if(!allowedTools.contains(name)) builds the "Unknown tool" result	ToolCallService.refusal(...) (same condition, same result)
TurnEngine.executeTools: if-chain present_results / kernel / search_knowledge / bulk-load / else toolbox	ToolCallService.execute(new ToolCallRequest(...)); each branch is its family's AssistantTool.execute. The three data families keep the QContext.init / QContext.clear around the call (ToolCallRequest.inPersonsContext); the kernel and the presenter have none, as before. The presenter is still one per turn (ToolCallScope.state).
TurnEngine.recordToolResult: store.insert(new AssistantToolCall()...)	ToolCallService.log(new ToolCallService.Finished(...)), same fields. BulkLoadTools.NAMES.contains(...) ? forLog(...) is the bulk tools' AssistantTool.forLog; "search_knowledge".equals(...) ? "GET /table/{tableName}/{primaryKey}" is the knowledge tool's operationWithoutDetail.
TurnEngine.hipaaRoute(), classification, conversation and turn ids passed to the families	TurnEngine.TurnToolScope implements ToolCallScope (read when the tool asks, as before)
AssistantApi.confirmAction: if-chain bulk writes / analysis writes / else executeConfirmedWrite	ToolCallService.executeConfirmed(new ConfirmedToolCall(...)); each branch is its family's executeConfirmed. A name that proposes nothing still gets "Not a write tool: x".
AssistantApi.confirmAction: store.insert(new AssistantToolCall()...)	ToolCallService.logConfirmed(action, userId, outcome), same fields
new ContextBuilder(config, kernel, toolbox).withKnowledgeTool(...).withBulkLoadTools(...)	new ContextBuilder(config, kernel, toolRegistry)

Untouched: AssistantApi.startTurn (assistant access, then the kill switch, then budgets, then the rate limit), TurnEngine's tool-call limit, parallelism, time limits, events, message parts and classification, every tool's own code, the store, the schema, the config defaults.

How to check "no behaviour change"

1. ChatToolSetTest: the chat's tool list (names, order, descriptions, input schemas, strict flags) in six scenarios (everything a host can switch on; no policy; a restricting policy; strict tool names; a person who is not offered the knowledge tool; a host with none of the optional metadata) equals src/test/resources/tool-registry/chat-tools-before-registry.txt. That file was written by the same scenarios on the code of origin/develop at e02658d before any production code changed (once under RC.25, once under RC.32: identical), and is not regenerated from the code under test.
2. ToolCallChecksTest passes unchanged on the code before the refactor (run on e02658d) and after. It uses the UI API test server's record of every request the assistant makes, so "nothing ran" is observed:
   • a person without assistant.hasAccess is refused (403) before a conversation, a model call, a UI API request or a log row exists, and before the kill switch is looked at;
   • a tool the policy does not allow is not offered, a call to it is refused without a request, and the "Unknown tool" row is logged;
   • the assistant's data access refuses before the person's request is made (same refusal for a person who could and one who could not read the table on screen);
   • past that, the call is the person's own request (their session, the assistant/<turnId> channel) and its refusal is the operation's own (403 for one person, 200 for another);
   • a write is only proposed in the turn and runs once, with the confirming session, leaving the "Confirmed" row.
3. No existing test was changed. The existing ArchUnit rules already bind the new package (it is under tools..); ToolCallArchitectureTest adds: outside the tools package only ToolCallService runs a tool, and only ToolCallService constructs an AssistantToolCall.

The registry's shape

• AssistantTool: name(), kind(), checks(), definition(ToolOffer) (null when this person is not offered the tool now), execute(ToolCallRequest), and for a proposing tool executeConfirmed(ConfirmedToolCall).
• ToolFamily: tools(AssistantRuntime). A public class under com.kingsrook.qbits.assistant.tools that implements it (with a public no-argument constructor) is found when the runtime starts (QQQ's ClassPathUtils, the scan host applications already rely on for their metadata). A later package registers a tool family by adding files; no existing class names it.
• ToolRegistry: every tool, sorted by name; refuses two tools of one name; the runtime does not start if no family is found.
• ToolCallService (in engine): refusal, execute, executeConfirmed, log, logConfirmed.
• Kinds: PROPOSE for create_record, update_record, delete_record, save_analysis, bulk_profile_save, bulk_load_run; DISPLAY for present_results and navigate; READ for the other sixteen (24 tools; render_widget is one of them).

One thing differs that no caller can observe: the families are asked for their definitions in tool-name order instead of data tools, kernel, knowledge, presentation, bulk-load. Each is still asked once per request, and anyWritableTable() is still evaluated once for the data tools and once for the bulk-load tools.

Where the design and the code disagreed (the code won)

• ToolCallService is in engine, not tools.registry, and the kernel's two tools are registered by engine.KernelToolFamily: ArchitectureTest.toolsOnlyThroughTheUiApi forbids anything under tools.. from depending on AssistantStore (the log row) or AiToolKernel.
• The service does not check the kill switch or a "classification for the channel" (design 7.6): today the kill switch is checked by the API before a turn and by the turn engine's stop check before every model call and tool step, and no tool call is refused on classification. Those stay where they are; the service holds what the turn engine did per call.
• No channel on a tool yet (design 7.6 "the channels it serves"): there is one channel. "Offered" and "definition" are one method because every family decides them together.
• Proposal contract types (design 12.2): only ConfirmedToolCall was needed to move the confirm route; nothing else was added.
• "Byte for byte" (design 12.2): KnowledgeTool.definition() and the bulk-load profile schema are built with Map.of, whose iteration order changes between JVM runs, so the comparison is on JSON with object keys sorted (list order kept). Outside this PR's scope, and not checked further: if a provider serialises those maps in iteration order, the tool prefix differs between two JVMs today.
• present_results is offered whatever the policy's allowed tools say (the code has no policy check for it). Kept, and pinned by the "restrictedByPolicy" scenario.
• CLAUDE.md says the default QQQ version must resolve from Maven Central. Neither RC.25, RC.32 nor qbit-build-parent 2.2.1 is there; all resolve from the QRun-IO Gitea registry that CI adds. Not changed here.
• The design names two branches; this is one PR on feat/tool-registry, as asked.

Build

The command CI runs for a branch (mvn -T1C verify, from KofTwentyTwo/ci-workflows maven.yaml@v1), run locally through the wrapper on head f035d63 from a clean tree:

./mvnw --batch-mode --no-transfer-progress --update-snapshots -s <scratch settings> -T1C clean verify

BUILD SUCCESS, 555 tests, 0 failures, 0 errors, 0 skipped, and "All coverage checks have been met" (JaCoCo) in all seven modules:

Module	Tests
qbit-assistant-provider-api	16
qbit-assistant	432 (418 before, plus the 14 new)
qbit-assistant-rag-pgvector	8
qbit-assistant-provider-anthropic	23
qbit-assistant-provider-openai	53
qbit-assistant-provider-bedrock	11
qbit-assistant-provider-vertex	12

The scratch settings file adds only the public QRun-IO registry (https://git.kof22.com/api/packages/QRun-IO/maven), where the parent POM and QQQ 4.1.0-RC.32 resolve from; CI adds the same registry. The gates this repository's build has are the enforcer guardrails of qbit-build-parent, the tests and the JaCoCo check; none was skipped.

Not in this PR: no new tool, no new permission name, no schema change, no change to defaults.

🤖 Generated with Claude Code

Packages **A0** and **A1** of the approved design `docs/actions-and-mcp.md` (draft PR #104, section 12.2), as one pull request with one commit each. ## A0: build against QQQ 4.1.0-RC.32 (`f848092`) `qqq.snapshot.version` moves from 4.1.0-RC.25 to 4.1.0-RC.32, the version GG Forge pins. Root `pom.xml` only: nothing else had to change. The whole suite was green on this commit alone before the refactor started (541 tests), and the chat's tool list is byte for byte the same under RC.25 and RC.32. ## A1: one tool registry, no behaviour change (`f035d63`) Every tool is registered once, in `tools.registry.ToolRegistry`: its name, kind (`READ`, `PROPOSE`, `DISPLAY`), the checks it runs, its definition (description, input schema) and how one call of it runs. "Run one tool call for this person under this policy, and log it" moves out of the turn engine into `engine.ToolCallService`, which the confirm route uses too. `UiApiToolbox`, `AnalysisTools`, `BulkLoadTools`, `KnowledgeTool`, `PresentResultsTool` and the core kernel are untouched apart from one added static method (`UiApiToolbox.notAWriteTool`, the text `executeConfirmedWrite` already answers). They keep all of their behaviour; five small family classes register their tools. ### What moved where | Before | After | |---|---| | `ContextBuilder.tools(policy, turnId)`: one block per family, into a `TreeMap` by name | `ToolRegistry.definitions(new ToolOffer(policy, session, turnId))`; each block is that family's `AssistantTool.definition(offer)`. The registry is sorted by name, so the order is the same. | | `ContextBuilder.isStrict` | `ToolRegistry.isStrict` (same expression) | | `TurnEngine.executeTools`: `if(!allowedTools.contains(name))` builds the "Unknown tool" result | `ToolCallService.refusal(...)` (same condition, same result) | | `TurnEngine.executeTools`: if-chain `present_results` / kernel / `search_knowledge` / bulk-load / else toolbox | `ToolCallService.execute(new ToolCallRequest(...))`; each branch is its family's `AssistantTool.execute`. The three data families keep the `QContext.init` / `QContext.clear` around the call (`ToolCallRequest.inPersonsContext`); the kernel and the presenter have none, as before. The presenter is still one per turn (`ToolCallScope.state`). | | `TurnEngine.recordToolResult`: `store.insert(new AssistantToolCall()...)` | `ToolCallService.log(new ToolCallService.Finished(...))`, same fields. `BulkLoadTools.NAMES.contains(...) ? forLog(...)` is the bulk tools' `AssistantTool.forLog`; `"search_knowledge".equals(...) ? "GET /table/{tableName}/{primaryKey}"` is the knowledge tool's `operationWithoutDetail`. | | `TurnEngine.hipaaRoute()`, `classification`, conversation and turn ids passed to the families | `TurnEngine.TurnToolScope implements ToolCallScope` (read when the tool asks, as before) | | `AssistantApi.confirmAction`: if-chain bulk writes / analysis writes / else `executeConfirmedWrite` | `ToolCallService.executeConfirmed(new ConfirmedToolCall(...))`; each branch is its family's `executeConfirmed`. A name that proposes nothing still gets "Not a write tool: x". | | `AssistantApi.confirmAction`: `store.insert(new AssistantToolCall()...)` | `ToolCallService.logConfirmed(action, userId, outcome)`, same fields | | `new ContextBuilder(config, kernel, toolbox).withKnowledgeTool(...).withBulkLoadTools(...)` | `new ContextBuilder(config, kernel, toolRegistry)` | Untouched: `AssistantApi.startTurn` (assistant access, then the kill switch, then budgets, then the rate limit), `TurnEngine`'s tool-call limit, parallelism, time limits, events, message parts and classification, every tool's own code, the store, the schema, the config defaults. ### How to check "no behaviour change" 1. `ChatToolSetTest`: the chat's tool list (names, order, descriptions, input schemas, strict flags) in six scenarios (everything a host can switch on; no policy; a restricting policy; strict tool names; a person who is not offered the knowledge tool; a host with none of the optional metadata) equals `src/test/resources/tool-registry/chat-tools-before-registry.txt`. That file was written by the same scenarios on the code of `origin/develop` at `e02658d` before any production code changed (once under RC.25, once under RC.32: identical), and is not regenerated from the code under test. 2. `ToolCallChecksTest` passes unchanged on the code before the refactor (run on `e02658d`) and after. It uses the UI API test server's record of every request the assistant makes, so "nothing ran" is observed: - a person without `assistant.hasAccess` is refused (403) before a conversation, a model call, a UI API request or a log row exists, and before the kill switch is looked at; - a tool the policy does not allow is not offered, a call to it is refused without a request, and the "Unknown tool" row is logged; - the assistant's data access refuses before the person's request is made (same refusal for a person who could and one who could not read the table on screen); - past that, the call is the person's own request (their session, the `assistant/<turnId>` channel) and its refusal is the operation's own (403 for one person, 200 for another); - a write is only proposed in the turn and runs once, with the confirming session, leaving the "Confirmed" row. 3. No existing test was changed. The existing ArchUnit rules already bind the new package (it is under `tools..`); `ToolCallArchitectureTest` adds: outside the tools package only `ToolCallService` runs a tool, and only `ToolCallService` constructs an `AssistantToolCall`. ### The registry's shape - `AssistantTool`: `name()`, `kind()`, `checks()`, `definition(ToolOffer)` (null when this person is not offered the tool now), `execute(ToolCallRequest)`, and for a proposing tool `executeConfirmed(ConfirmedToolCall)`. - `ToolFamily`: `tools(AssistantRuntime)`. A public class under `com.kingsrook.qbits.assistant.tools` that implements it (with a public no-argument constructor) is found when the runtime starts (QQQ's `ClassPathUtils`, the scan host applications already rely on for their metadata). **A later package registers a tool family by adding files; no existing class names it.** - `ToolRegistry`: every tool, sorted by name; refuses two tools of one name; the runtime does not start if no family is found. - `ToolCallService` (in `engine`): `refusal`, `execute`, `executeConfirmed`, `log`, `logConfirmed`. - Kinds: `PROPOSE` for `create_record`, `update_record`, `delete_record`, `save_analysis`, `bulk_profile_save`, `bulk_load_run`; `DISPLAY` for `present_results` and `navigate`; `READ` for the other sixteen (24 tools; `render_widget` is one of them). One thing differs that no caller can observe: the families are asked for their definitions in tool-name order instead of data tools, kernel, knowledge, presentation, bulk-load. Each is still asked once per request, and `anyWritableTable()` is still evaluated once for the data tools and once for the bulk-load tools. ### Where the design and the code disagreed (the code won) - **`ToolCallService` is in `engine`, not `tools.registry`**, and the kernel's two tools are registered by `engine.KernelToolFamily`: `ArchitectureTest.toolsOnlyThroughTheUiApi` forbids anything under `tools..` from depending on `AssistantStore` (the log row) or `AiToolKernel`. - **The service does not check the kill switch or a "classification for the channel"** (design 7.6): today the kill switch is checked by the API before a turn and by the turn engine's stop check before every model call and tool step, and no tool call is refused on classification. Those stay where they are; the service holds what the turn engine did per call. - **No channel on a tool yet** (design 7.6 "the channels it serves"): there is one channel. "Offered" and "definition" are one method because every family decides them together. - **Proposal contract types** (design 12.2): only `ConfirmedToolCall` was needed to move the confirm route; nothing else was added. - **"Byte for byte"** (design 12.2): `KnowledgeTool.definition()` and the bulk-load profile schema are built with `Map.of`, whose iteration order changes between JVM runs, so the comparison is on JSON with object keys sorted (list order kept). Outside this PR's scope, and not checked further: if a provider serialises those maps in iteration order, the tool prefix differs between two JVMs today. - **`present_results` is offered whatever the policy's allowed tools say** (the code has no policy check for it). Kept, and pinned by the "restrictedByPolicy" scenario. - `CLAUDE.md` says the default QQQ version must resolve from Maven Central. Neither RC.25, RC.32 nor `qbit-build-parent` 2.2.1 is there; all resolve from the QRun-IO Gitea registry that CI adds. Not changed here. - The design names two branches; this is one PR on `feat/tool-registry`, as asked. ## Build The command CI runs for a branch (`mvn -T1C verify`, from `KofTwentyTwo/ci-workflows` `maven.yaml@v1`), run locally through the wrapper on head `f035d63` from a clean tree: ``` ./mvnw --batch-mode --no-transfer-progress --update-snapshots -s <scratch settings> -T1C clean verify ``` **BUILD SUCCESS**, 555 tests, 0 failures, 0 errors, 0 skipped, and "All coverage checks have been met" (JaCoCo) in all seven modules: | Module | Tests | |---|---| | qbit-assistant-provider-api | 16 | | qbit-assistant | 432 (418 before, plus the 14 new) | | qbit-assistant-rag-pgvector | 8 | | qbit-assistant-provider-anthropic | 23 | | qbit-assistant-provider-openai | 53 | | qbit-assistant-provider-bedrock | 11 | | qbit-assistant-provider-vertex | 12 | The scratch settings file adds only the public QRun-IO registry (`https://git.kof22.com/api/packages/QRun-IO/maven`), where the parent POM and QQQ 4.1.0-RC.32 resolve from; CI adds the same registry. The gates this repository's build has are the enforcer guardrails of `qbit-build-parent`, the tests and the JaCoCo check; none was skipped. Not in this PR: no new tool, no new permission name, no schema change, no change to defaults. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

james.maes added 2 commits 2026-11-12 00:02:26 +00:00

build: build against QQQ 4.1.0-RC.32 ... f848092759

Moves qqq.snapshot.version from 4.1.0-RC.25 to 4.1.0-RC.32, the version
applications (GG Forge) pin. No behaviour change.

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

refactor(tools): one tool registry, no behaviour change ... f035d6356b

Every tool is registered once in tools.registry.ToolRegistry: its name, its
kind (READ, PROPOSE, DISPLAY), the checks it runs, its definition and how one
call of it runs. The chat's tool list, a tool call of a turn and a confirmed
write all look a tool up there; the turn engine and the confirm route no
longer choose a tool family with an if-chain.

"Run one tool call for this person under this policy, and log it" moves
out of TurnEngine into engine.ToolCallService, which the confirm route uses
too: the refusal of a tool that is not offered, the call (with the person's
own session) and the assistant_tool_call row.

What moved where:
 - ContextBuilder.tools: the per-family blocks become
   ToolRegistry.definitions(ToolOffer); each family's block is its
   AssistantTool.definition (UiApiToolFamily, engine.KernelToolFamily,
   KnowledgeToolFamily, PresentResultsToolFamily, BulkLoadToolFamily).
   isStrict moves to ToolRegistry.isStrict.
 - TurnEngine.executeTools: the 'Unknown tool' refusal is
   ToolCallService.refusal; the dispatch is ToolCallService.execute; each
   branch is its family's AssistantTool.execute (the data families keep the
   QContext init and clear around the call; the presenter stays one per turn
   through ToolCallScope.state).
 - TurnEngine.recordToolResult: the row is built by ToolCallService.log; the
   bulk-load log reduction and search_knowledge's operation are the tools'
   forLog and operationWithoutDetail.
 - AssistantApi.confirmAction: the if-chain is
   ToolCallService.executeConfirmed; the row is ToolCallService.logConfirmed.

UiApiToolbox, AnalysisTools, BulkLoadTools, KnowledgeTool, PresentResultsTool
and the core kernel keep all of their behaviour. A tool family is a class
under com.kingsrook.qbits.assistant.tools that implements ToolFamily; it is
found at startup, so a new family is new files only. The kernel family lives
in engine and is named by the runtime, because nothing under tools may touch
the kernel (ArchitectureTest).

Tests: ChatToolSetTest compares the chat's tool list (names, order,
descriptions, schemas, strict flags, in six scenarios) with the list written
from origin/develop at e02658d before any production code changed.
ToolCallChecksTest (permission, tool switch, data access, the person's own
request, confirmation, and the log row of each refusal) passes unchanged on
the code before and after. ToolRegistryTest and ToolCallArchitectureTest cover
the registry and bind 'only ToolCallService runs or logs a tool call'. No
existing test changed.

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

james.maes merged commit 21f634c167 into develop 2026-11-12 15:15:52 +00:00

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

architecture

Architecture review: coupling, cohesion, UI API

bug

Something isn't working

dependencies

documentation

Improvements or additions to documentation

duplicate

This issue or pull request already exists

enhancement

New feature or request

github_actions

Pull requests that update GitHub Actions code

good first issue

Good for newcomers

help wanted

Extra attention is needed

invalid

This doesn't seem right

java

Pull requests that update java code

question

Further information is requested

release:rc

Merging this develop PR cuts the next release candidate

secuurity

ux

Admin UX / information architecture

wontfix

This will not be worked on

No labels

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

forge-recovery (Forge Recovery) james.maes (James Maes)

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: QRun-IO/qbit-assistant#105