fix(tools): one decision per pending action, a tool surface closed at compile time; pending actions with before and after #108

2026-11-12T17:36:13Z

james.maes commented

2026-11-12 17:36:13 +00:00

Closes the findings of a third independent review of the tool boundary and builds the pending-action package (A3) of the actions design on top. One pull request, as asked. Each fix is described by the guarantee it gives.

Review findings

#	Guarantee	Proven by
1	A pending action is confirmed or cancelled once, by exactly one request, on one server or several. The decision is an insert under a unique key (new table `assistant_pending_action_claim`); a monitor covers threads of one server. Whatever fails after the claim ends in `FAILED` or `EXECUTED` with a tool-call row, never in `RUNNING`.	`PendingActionDecisionTest`: two confirmations held at the same point run the change once (fails with the previous claim logic: both ran); confirm or cancel, never both; a status that cannot be read back; a claim that cannot be marked.
2	A tool cannot be run around the tool-call service from code outside the assistant's packages. `execute`, `log` and the new `withdraw` are package-private; `ToolCallRequest`, `ConfirmedToolCall` and `UiApiToolbox.Context` are final classes with no public constructor, made only with the service's permit; the runtime has no public getter for the registry, the gateway or its tool families.	`HostToolFamilyTest`, in a package outside the assistant's: a host's family runs through the service; each former way around the service is compiled and the compiler refuses it; a factory called without the permit throws. `ToolCallArchitectureTest`.
3	A signed-in person is one person. With no HTTP request, the authentication module resolves the session's own id, and the person is made only when that is the user the session names. The turn engine no longer makes a person; `new AssistantApi(runtime, session)` makes one only when it needs to act as the person, and answers 403 when the check fails.	`SignedInPersonTest`.
4	Only the credential a request's session was resolved from is forwarded to the UI API.	`SignedInPersonTest`.
5	A tool call that was cut short leaves no proposal anybody could confirm: what it proposed is withdrawn, what it proposes later is refused.	`CutShortProposalTest` (both cases).
6	The policy a tool call runs under is read fresh, once per call. No policy row means the code defaults (the design: a row only narrows). The README states what `policyCacheSeconds` still covers.	`PendingActionDecisionTest.aToolSwitchedOffIsRefusedOnTheNextCall`.
7	The hard budget is read on every call with one aggregate query per budget; no usage row is read.	`PendingActionDecisionTest.theHardBudgetIsReadOnEveryCallWithoutReadingUsageRows`, `BudgetServiceTest` unchanged.
8	The input kept in the tool-call log is redacted with the administrator's custom patterns too.	`PendingActionDecisionTest.aLogRowsInputIsRedactedWithTheAdministratorsPatterns`.

Pending actions (design package A3)

update_record and delete_record read the record as the person when they propose. A record the person cannot read gets that read's own answer and no card.
The card shows the table, the record and each field before and after (a delete: what the record holds), after the exposure rules of get_record.
What was approved is what is written: the proposal keeps a version of the record (a digest of the whole record, never values). A confirmed update or delete reads the record again and refuses when it changed, is gone or cannot be read; the record is left as it is.
actionKind: record_create, record_update, record_delete, or the proposing tool's name, so later kinds need no schema change.
GET /assistant/actions?status=&conversationId=: the person's own list, expiry evaluated on read. UI API extension assistant 1.9.0, additive.
approvalRetentionDays is now read: retention deletes decided and lapsed pending actions older than it, with or without a conversation.

Tests: PendingActionsTest.

Schema (a host must apply this before deploying)

The "Pending actions" block at the end of docs/schema-postgres.sql, guarded and safe to re-run:

assistant_pending_action: action_kind VARCHAR(64), before_json TEXT, record_version VARCHAR(64); indexes (user_id, status) and (create_date).
New table assistant_pending_action_claim with UNIQUE (pending_action_id) and a cascading foreign key.

SchemaForeignKeyPostgresTest applies the whole DDL to PostgreSQL.

Where this differs from what was asked or from the design

Finding 3 asked for creation from a bare session to be limited to two callers at compile time. Table customizers and processes of this module also bind the API to the session QQQ gave them, and the scheduler's login is a public seam, so the restriction is made on the session instead of the caller: no caller can make a person from a session that was not resolved. The assistant's own permission checks still read the session object they are given.
The stale check compares the whole record, not only the fields being changed as the design says: a digest of one field could be searched for its value.
The channel and client_name columns of the design arrive with the MCP proposals, not here.

Not closed

The compile-time guarantee is for code in other packages. Code that declares itself inside the assistant's packages, or uses reflection, is not stopped; sealing the jar would break the published test-jar, which shares those packages.
SignedInPerson.ofRequest takes the session the route's session setup put on the thread; that only the route calls it is still an ArchUnit rule.
Data access rows, profiles and providers stay cached for policyCacheSeconds across servers (stated in the README).
The UI API has no conditional write: a change made between the confirmation's read and its write is not detected.
A proposal stored before the upgrade (no version) is refused for an update or a delete, and the person asks again.

Existing tests

ChatToolSetTest and ToolCallChecksTest pass unchanged. Expectations that changed:

UiApiToolsSecurityTest denial matrix: for a person who cannot read the record, update_record and delete_record are now refused when proposed (by the read of the record) instead of when confirmed.
ToolCallServiceTest: SignedInPerson's factory for a session takes the instance too.
ToolRegistryTest: the assertion on ToolCallService.getRegistry() is gone with the getter.
ToolCallArchitectureTest: "only the service makes what a tool runs with" now also asserts there is no public constructor.

Build: ./mvnw --batch-mode --no-transfer-progress -s <settings> -T1C clean verify, green on this head (Checkstyle, all modules, JaCoCo gates).

🤖 Generated with Claude Code

Closes the findings of a third independent review of the tool boundary and builds the pending-action package (A3) of the actions design on top. One pull request, as asked. Each fix is described by the guarantee it gives. ## Review findings | # | Guarantee | Proven by | |---|---|---| | 1 | A pending action is confirmed or cancelled once, by exactly one request, on one server or several. The decision is an insert under a unique key (new table `assistant_pending_action_claim`); a monitor covers threads of one server. Whatever fails after the claim ends in `FAILED` or `EXECUTED` with a tool-call row, never in `RUNNING`. | `PendingActionDecisionTest`: two confirmations held at the same point run the change once (fails with the previous claim logic: both ran); confirm or cancel, never both; a status that cannot be read back; a claim that cannot be marked. | | 2 | A tool cannot be run around the tool-call service from code outside the assistant's packages. `execute`, `log` and the new `withdraw` are package-private; `ToolCallRequest`, `ConfirmedToolCall` and `UiApiToolbox.Context` are final classes with no public constructor, made only with the service's permit; the runtime has no public getter for the registry, the gateway or its tool families. | `HostToolFamilyTest`, in a package outside the assistant's: a host's family runs through the service; each former way around the service is compiled and the compiler refuses it; a factory called without the permit throws. `ToolCallArchitectureTest`. | | 3 | A signed-in person is one person. With no HTTP request, the authentication module resolves the session's own id, and the person is made only when that is the user the session names. The turn engine no longer makes a person; `new AssistantApi(runtime, session)` makes one only when it needs to act as the person, and answers 403 when the check fails. | `SignedInPersonTest`. | | 4 | Only the credential a request's session was resolved from is forwarded to the UI API. | `SignedInPersonTest`. | | 5 | A tool call that was cut short leaves no proposal anybody could confirm: what it proposed is withdrawn, what it proposes later is refused. | `CutShortProposalTest` (both cases). | | 6 | The policy a tool call runs under is read fresh, once per call. No policy row means the code defaults (the design: a row only narrows). The README states what `policyCacheSeconds` still covers. | `PendingActionDecisionTest.aToolSwitchedOffIsRefusedOnTheNextCall`. | | 7 | The hard budget is read on every call with one aggregate query per budget; no usage row is read. | `PendingActionDecisionTest.theHardBudgetIsReadOnEveryCallWithoutReadingUsageRows`, `BudgetServiceTest` unchanged. | | 8 | The input kept in the tool-call log is redacted with the administrator's custom patterns too. | `PendingActionDecisionTest.aLogRowsInputIsRedactedWithTheAdministratorsPatterns`. | ## Pending actions (design package A3) - `update_record` and `delete_record` read the record as the person when they propose. A record the person cannot read gets that read's own answer and no card. - The card shows the table, the record and each field before and after (a delete: what the record holds), after the exposure rules of `get_record`. - What was approved is what is written: the proposal keeps a version of the record (a digest of the whole record, never values). A confirmed update or delete reads the record again and refuses when it changed, is gone or cannot be read; the record is left as it is. - `actionKind`: `record_create`, `record_update`, `record_delete`, or the proposing tool's name, so later kinds need no schema change. - `GET /assistant/actions?status=&conversationId=`: the person's own list, expiry evaluated on read. UI API extension `assistant` 1.9.0, additive. - `approvalRetentionDays` is now read: retention deletes decided and lapsed pending actions older than it, with or without a conversation. Tests: `PendingActionsTest`. ## Schema (a host must apply this before deploying) The "Pending actions" block at the end of `docs/schema-postgres.sql`, guarded and safe to re-run: - `assistant_pending_action`: `action_kind VARCHAR(64)`, `before_json TEXT`, `record_version VARCHAR(64)`; indexes `(user_id, status)` and `(create_date)`. - New table `assistant_pending_action_claim` with `UNIQUE (pending_action_id)` and a cascading foreign key. `SchemaForeignKeyPostgresTest` applies the whole DDL to PostgreSQL. ## Where this differs from what was asked or from the design - **Finding 3** asked for creation from a bare session to be limited to two callers at compile time. Table customizers and processes of this module also bind the API to the session QQQ gave them, and the scheduler's login is a public seam, so the restriction is made on the session instead of the caller: no caller can make a person from a session that was not resolved. The assistant's own permission checks still read the session object they are given. - **The stale check** compares the whole record, not only the fields being changed as the design says: a digest of one field could be searched for its value. - The `channel` and `client_name` columns of the design arrive with the MCP proposals, not here. ## Not closed - The compile-time guarantee is for code in other packages. Code that declares itself inside the assistant's packages, or uses reflection, is not stopped; sealing the jar would break the published test-jar, which shares those packages. - `SignedInPerson.ofRequest` takes the session the route's session setup put on the thread; that only the route calls it is still an ArchUnit rule. - Data access rows, profiles and providers stay cached for `policyCacheSeconds` across servers (stated in the README). - The UI API has no conditional write: a change made between the confirmation's read and its write is not detected. - A proposal stored before the upgrade (no version) is refused for an update or a delete, and the person asks again. ## Existing tests `ChatToolSetTest` and `ToolCallChecksTest` pass unchanged. Expectations that changed: - `UiApiToolsSecurityTest` denial matrix: for a person who cannot read the record, `update_record` and `delete_record` are now refused when proposed (by the read of the record) instead of when confirmed. - `ToolCallServiceTest`: `SignedInPerson`'s factory for a session takes the instance too. - `ToolRegistryTest`: the assertion on `ToolCallService.getRegistry()` is gone with the getter. - `ToolCallArchitectureTest`: "only the service makes what a tool runs with" now also asserts there is no public constructor. Build: `./mvnw --batch-mode --no-transfer-progress -s <settings> -T1C clean verify`, green on this head (Checkstyle, all modules, JaCoCo gates). 🤖 Generated with [Claude Code](https://claude.com/claude-code)

james.maes added 1 commit 2026-11-12 17:36:14 +00:00

fix(tools): one decision per pending action, a tool surface closed at compile time; feat: pending actions with before and after

maven / build (push) Successful in 6m19s

Details

build / maven (push) Successful in 6m19s

Details

0dc325cd2c

Closes the findings of a third independent review of the tool boundary and
builds the pending-action package of the actions design on top.

Guarantees:
- A pending action is confirmed or cancelled once, by exactly one request,
  on one server or several: the decision is an insert under a unique key
  (new table assistant_pending_action_claim). Whatever fails after the claim
  ends in FAILED or EXECUTED with a tool-call row, never in RUNNING.
- A tool cannot be run around the tool-call service from code outside the
  assistant's packages: execute, log and withdraw are package-private; a
  tool call request, a confirmed tool call and a tool context have no public
  constructor and are made only with the service's permit; the runtime has
  no public getter for the tool registry, the UI API gateway or its tool
  families. A host's own ToolFamily and AssistantTool compile unchanged.
- A signed-in person is one person: a session with no HTTP request is made a
  person only when the authentication module resolves the session's own id
  to the user the session names. The turn engine no longer makes a person.
- Only the credential a request's session was resolved from is forwarded to
  the UI API.
- A tool call that was cut short leaves no proposal anybody could confirm.
- The policy a tool call runs under is read fresh on every call.
- The hard budget is read on every call with one aggregate query per budget.
- The input kept in the tool-call log is redacted with the administrator's
  custom patterns too.

Pending actions (design package A3):
- update_record and delete_record read the record as the person when they
  propose; the card shows each field before and after (a delete: what the
  record holds), after the exposure rules of get_record.
- What was approved is what is written: the proposal keeps a version of the
  record (a digest, never values); a confirmed update or delete reads the
  record again and refuses when it changed, is gone or cannot be read.
- actionKind (record_create, record_update, record_delete, or the proposing
  tool's name), GET /assistant/actions (the person's own list), and
  approvalRetentionDays is now read by the retention process.
- UI API extension assistant 1.9.0 (additive).

Schema: three columns and two indexes on assistant_pending_action, and the
table assistant_pending_action_claim (docs/schema-postgres.sql, UPGRADE.md).

ChatToolSetTest and ToolCallChecksTest pass unchanged.

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

james.maes merged commit ad6d854d22 into develop

2026-11-12 17:49:08 +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#108