Skip to content

feat: execute the approved payload for gated MCP calls - #2106

Draft
OliverBryant wants to merge 6 commits into
xorbitsai:mainfrom
OliverBryant:feat/frozen-payload-store
Draft

feat: execute the approved payload for gated MCP calls#2106
OliverBryant wants to merge 6 commits into
xorbitsai:mainfrom
OliverBryant:feat/frozen-payload-store

Conversation

@OliverBryant

Copy link
Copy Markdown
Contributor

Part of #2001 (step B); implements the fix direction of #1585. Downstream tracking: xorbitsai/xagent-saas#972 / #976.

Why

Approving a connector action does not currently bind anything. The pause stops the turn, but the arguments are written again by the model afterwards, so what executes is a re-derivation of what was shown.

That is not theoretical. A Toby user approved a LinkedIn post and a different post went out; the prod trace (xagent-saas#973) shows three mcp_LinkedIn_create_post executions across three tasks, copy going 853 → 641 characters, and a different generated image each time.

What

A gated MCP call is frozen before it runs and executed verbatim on approval.

  • The gate is a host-installed hook, not a call into the web layer. run_json_async is reconstructed and executed inside the sandbox for every npx/uvx connector (sandboxed_tool/tool_runner.py), where sqlalchemy is not installed — anything reaching for a database from the adapter would turn every sandboxed tool call into a ModuleNotFoundError. That seam is also the off switch: no registration means no policy, no row, and no behavior change.
  • Freezing happens after arguments are normalized and runtime-bound values are merged, and before any execution attempt, so what is stored is exactly what would have run and the two retry paths inherit the decision instead of each needing their own gate.
  • Resuming re-enters _execute_mcp_call, not run_json_async: re-running the gate would pause the resumed call a second time.
  • Settlement claims the row with UPDATE ... WHERE status = 'pending' and acts on rowcount, so a double-click publishes once. Expiry is evaluated when an answer arrives rather than swept — a pending row is harmless where it sits, and deleting on a timer would race the approval arriving at that moment.
  • Only an exact approve runs the payload. Free text containing the word does not: the pause offers two option values, so anything else reaching the callback did not come from those buttons.

Gating uses the write annotations from #2069. Only an explicit read-only declaration skips the gate; destructive and undeclared are both writes, and undeclared is the common case since annotations are optional.

What this does and does not guarantee

If a call is gated, the arguments that execute are the ones that were shown, byte for byte. It does not guarantee that every dangerous call is gated — that decision rests on a server's own annotations, which the MCP spec says a client must not trust from an untrusted server.

Consistent with that, a failing hook lets the call through rather than blocking it. Failing closed here would let one database error strand every connector in a workspace behind an approval nobody can grant, trading bounded loss of gating for unbounded loss of function. The host owns the decision to fail closed on its own side, where it can tell a policy miss from an outage.

Storage

A dedicated table rather than a row in task_interaction_requests. That table would otherwise be the right home, but staging a row there requires a resolved checkpoint anchor, and the checkpoint for this pause does not exist yet when the gate runs — it is written by the pattern's pause handler after the tool returns. The two records also answer different questions: that one tracks the conversation with the user, this one holds the bytes to execute.

The migration follows the house shape (offline/online fork on context.as_sql, a guard making upgrade() idempotent over a create_all-built schema, a guard that skips cleanly when the parent table is absent). All three shapes were exercised on SQLite, and the alembic head stays single.

Testing

  • 20 new tests drive the real adapter, the real hooks and a real database; assertions are on the arguments the connector received, never on the pause alone.
  • 636 pass across tests/core/tools/adapters/ plus the new suite; the migration suites pass and the heads check is green.
  • Six mutations verified, each killed by the test that claims the behavior: the gate not pausing; resume executing something other than the frozen arguments; the claim ignoring rowcount so a double-click publishes twice; expiry not enforced; free text counting as approval; undeclared treated as read-only.
  • mypy is back to the repository baseline with no findings in these files; the two it did raise are fixed rather than silenced.

Nothing writes rows until a host installs the gate, so this PR changes no behavior on its own.

@gemini-code-assist

Copy link
Copy Markdown
Contributor

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

OliverBryant added 3 commits September 4, 2026 12:44
A gated MCP call is frozen before it runs and executed verbatim on
approval, so what a person approves is what reaches the connector. Today an
approval only pauses the conversation: the arguments are written again by
the model afterwards, which is how an approved LinkedIn post went out with
rewritten copy and a regenerated image.

The gate is a host-installed hook rather than a direct call into the web
layer. run_json_async is reconstructed and executed inside the sandbox for
every npx/uvx connector, where sqlalchemy is not installed, so anything
reaching for a database from the adapter would turn every sandboxed tool
call into a ModuleNotFoundError. That seam is also the off switch: no
registration means no policy, no row, and no behavior change.

Freezing happens after arguments are normalized and runtime-bound values
are merged, and before any execution attempt, so what is stored is exactly
what would have run and the retry paths inherit the decision. Resuming
re-enters _execute_mcp_call rather than run_json_async: re-running the gate
would pause the resumed call a second time.

Settlement claims the row with UPDATE ... WHERE status = 'pending' and acts
on rowcount, so a double-click executes once; a second answer finds no
pending row and reports the call already settled. Expiry is evaluated when
an answer arrives rather than swept: a pending row is harmless where it
sits, and deleting on a timer would race the approval arriving at that
moment.

Only an exact "approve" runs the payload. Free text that merely contains
the word does not: the pause offers two option values, so anything else
reaching the callback did not come from those buttons.

The gate consults the server's own write annotations, which the MCP spec
says a client must not trust from an untrusted server. What this guarantees
is narrower and stated in the module: if a call is gated, the arguments
that execute are the ones that were shown. It does not guarantee that every
dangerous call is gated, and a failing hook lets the call through rather
than stranding every connector in a workspace behind an approval nobody can
grant.

Migration follows the house shape: an offline/online fork on context.as_sql,
a guard that makes upgrade() idempotent over a create_all-built schema, and
a guard that skips cleanly when the parent table is absent. All three shapes
exercised on SQLite.
Twenty tests drive the real adapter, the real hooks and a real database.
The assertions are on the arguments the connector actually received, never
on the pause alone: a model that rewrote its arguments after the approval
is only visible as an execution that differs from what was frozen, which is
exactly the incident this mechanism exists to prevent.

Covered: a gated call pauses without executing; approval executes the
frozen arguments; rejection never executes; a second answer does not
execute again; an expired approval does not execute; only the exact option
value grants (free text containing "approve" does not); a read-only
declaration is not gated while coercible non-booleans and silence are; no
policy, no installation, and a failing hook all leave execution untouched;
an unknown interaction id and a missing host hook execute nothing.

Six mutations verified, each killed by the test that claims the behavior:
the gate not pausing, resume executing something other than the frozen
arguments, the claim ignoring rowcount so a double-click publishes twice,
expiry not being enforced, free text counting as approval, and undeclared
tools being treated as read-only.

The two mypy findings this raised are fixed rather than silenced: the model
declares bare Column(...) rather than Mapped[...], so an instance attribute
is typed as the descriptor, and Result does not declare rowcount on the
generic protocol. Both are read through a narrow cast with the reason
stated. mypy is back to the baseline count with none in these files.
…head

xorbitsai#2050 landed 20260902_oauth_flow_generation on main after this branch was
cut, so both revisions claimed the same parent and alembic saw two heads.
The rebase alone does not fix that -- down_revision is data in the file,
not something git reconciles -- so it is repointed at the new head.

Repointed rather than merged: a merge revision would permanently record a
branch that only ever existed because two PRs were open at once.

Upgrade and downgrade re-exercised on SQLite over a create_all-built schema.
@OliverBryant
OliverBryant force-pushed the feat/frozen-payload-store branch from e63f4c9 to e327381 Compare September 4, 2026 04:46
The guard caught this new table exactly as designed, so the question it
raises is whether the flag is needed here or was copied from a neighbour.
It is needed: without none_as_null, serialization runs before binding and a
Python None reaches the column as the JSON text "null", which NOT NULL does
not reject. Verified by construction -- dropping the flag lets an insert
with arguments=None commit, which would leave a frozen call whose payload
is a JSON null and whose approval would execute nothing.

The docstring now records why each entry is on the list rather than only
that it is, so the next table to trip this has the test to argue against.

@rogercloud rogercloud left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR adds an opt-in host-installed write gate for MCP tools: it normalizes and freezes a payload in a new database row, pauses ReAct for approval, and attempts to replay the stored arguments once. It also adds the FrozenToolCall model/migration plus direct adapter/store tests intended to make approval durable across checkpoint/resume. The frozen-row idea addresses the re-derivation bug, but the current integration leaves supported sandbox transports and several resume/authorization contracts incorrect.

Blocking: yes — recommended event: REQUEST_CHANGES

Update summary

This is technically a re-review because the complete history export has one conversation root, but entry 5535707187 is only a gemini-code-assist[bot] daily-quota warning; there are zero formal reviews and zero inline roots, so no substantive prior findings, replies, or waivers exist to mark fixed. The four commits currently at HEAD are af330a9d (the host gate, frozen-call model/store/migration, and adapter pause/resume path), 9f4a3ea (direct adapter/store regression tests and test-oriented gate adjustments), e327381 (chains the new migration from 20260902_oauth_flow_generation), and 786898a (adds frozen_tool_calls.arguments to the none_as_null schema inventory). No substantive prior finding was fixed or carried forward; all findings below are new in this pass.

Round 0 approach verdict

Verdict: wrong-direction. The dedicated frozen-call table, durable pause-before-execution, and short conditional claim are sensible building blocks. The integration boundary is inverted, though: the only gate consultation is inside MCPToolAdapter.run_json_async, even though supported npx/uvx tools reconstruct that adapter in a guest process where the host hook and database do not exist; the resume callback also introduces a result-producing external operation without extending ReAct's result-delivery or per-interaction response contracts. The fix should move preparation, policy, persistence, and resume orchestration to a host-side boundary above both direct and sandboxed transports, then dispatch a serializable prepared call and project its structured result through the normal task flow.

Confirmed findings

1. Sandboxed npx/uvx MCP writes bypass the host gate

Location: src/xagent/core/tools/adapters/vibe/mcp_adapter.py:1352
Severity: critical
Blocking: yes

  • Reachable trigger: A supported web task has a sandbox lease, the MCP connection is stdio with command basename npx or uvx, a host installs the documented write gate with a policy requiring approval, and the model invokes a destructive or undeclared MCP tool. This is the production route selected by should_sandbox_mcp_connection and load_mcp_tools_as_agent_tools.
  • Concrete impact: SandboxedToolWrapper dispatches the call to tool_runner.py, which imports and reconstructs a fresh MCPToolAdapter in the guest. That process has no process-global _HOOK, so consult_write_gate returns None and the connector executes immediately. No frozen row or waiting_for_user result is created; the host wrapper also has no resume_user_interaction, so the supported npx/uvx class is neither gated nor resumable.
  • Caller/type/invariant evidence: The host hook and SQLAlchemy store are intentionally host-only, while the sandbox serializes the adapter class and constructor data rather than module state. The guest therefore cannot inherit the host hook, and no wrapper capability forwards a host resume callback. The existing sandbox contract does not establish any alternate policy propagation.
  • PR causation: The new consultation at line 1352 is introduced by this PR; the pre-existing sandbox reconstruction is where this new call actually runs, so the newly promised gate is ineffective on this supported transport.
  • Specific fix: Move normalization, trusted runtime-argument merge, write classification, and freezing to a host boundary above both direct adapters and SandboxedToolWrapper. Persist a serializable prepared-call envelope, expose host-side resume, and dispatch the approved envelope into the guest without re-gating. Add a real npx/uvx wrapper/runner seam test proving no execution before approval and successful host-side resume.

2. ReAct discards the real resumed connector result

Location: src/xagent/core/tools/adapters/vibe/mcp_adapter.py:1704
Severity: major
Blocking: yes

  • Reachable trigger: A direct, non-sandboxed gated MCP write is approved through the normal ReAct same-task resume path and the connector returns a success, connector error, expiry, cancellation, or settled result.
  • Concrete impact: resume_user_interaction returns the host hook's real connector result, but _deliver_pending_tool_interaction_responses only awaits the callback and then removes/checkpoints the pending response identifiers. It never attaches the result to the original tool_call_id, execution context, tool ledger, or trace. The resumed model can therefore report an unverified success, fail to surface a real failure, or re-call the tool because context still says the call is waiting.
  • Caller/type/invariant evidence: The established callback protocol supports callbacks that mutate state and return None; this PR's write-gate hook explicitly returns the external tool result. ReAct's delivery caller has no result projection path, while normal tool execution uses _backfill_result, ledger transitions, ExecutionContext.add_tool_result, and tool-end/error tracing. No caller invariant makes the callback return value visible elsewhere.
  • PR causation: The PR adds the first production callback that performs the suspended external operation and relies on its return value, but leaves the existing result-discarding ReAct consumer unchanged.
  • Specific fix: Extend the resumable-interaction protocol with a structured completion outcome. Use the saved tool_call_id to finalize the suspended call through the normal result/ledger/context/trace path before replanning, including explicit success, connector-error, rejected, expired, settled, and missing-hook outcomes. Add a checkpoint/restore/ReAct integration test that asserts the resumed result reaches the next model/final task result.

3. Approved resume bypasses current authorization and the normal retry/error envelope

Location: src/xagent/core/tools/adapters/vibe/mcp_adapter.py:1722
Severity: major
Blocking: yes

  • Reachable trigger: A supported adapter uses the documented allow_users contract, the initial call is frozen while the identity is allowed, and before approval the current identity is missing/different/revoked; separately, an approved call encounters a delegated/resolver HTTP 401 or another supported MCP session/transport exception.
  • Concrete impact: _execute_frozen_call invokes _execute_mcp_call directly, so a pending approval can still execute after the current authorization boundary no longer allows that identity. The same path skips the existing delegated/resolver refresh-and-retry logic and the safe connector-error mapping; an exception can fail the resumed task. The store has already committed the row as executed before network I/O, so the approval is terminal and cannot be retried through the same interaction.
  • Caller/type/invariant evidence: Ordinary run_json_async performs _is_user_allowed, establishes UserContext, invokes _retry_after_authorization_failure, and maps unhandled failures to the established safe MCP result. The web resume route's task-owner check prevents a different task user from taking over, but it is not an allow_users recheck and the core resume protocol carries no universal authorization guarantee. MCP transport and credential-refresh failures are legitimate external-boundary outcomes.
  • PR causation: _execute_frozen_call and its direct low-level re-entry are new in this PR; the base adapter already contained the authorization, retry, and error envelope that the new resume path bypasses.
  • Specific fix: Factor a shared prepared-execution helper used by both ordinary and resumed calls. It should recheck current authorization, establish UserContext, retain authorization refresh/retry, and preserve safe error mapping while intentionally skipping only argument derivation and gate consultation. Validate authorization before a terminal claim, or add explicit voided/retryable failure semantics instead of unconditionally consuming the row.

4. The frozen row is not bound to a stable connector/account identity

Location: src/xagent/web/services/frozen_tool_call_store.py:135
Severity: major
Blocking: yes

  • Reachable trigger: While an approval is pending, the same-name MCP connector's URL, auth, headers, environment, or other configuration changes; OAuth is reauthorized to another provider account; or a sole connector is deleted and recreated under the old name. A supported runtime teardown/rebuild then exposes the same runtime tool name from the current connection.
  • Concrete impact: The store's claim predicate contains only interaction_id and status='pending', and the resumed adapter executes the frozen arguments through its current connection. The approved write can therefore target a different endpoint or account than the one shown to the approver; account-relative calls such as create_post do not carry the account identity in their arguments.
  • Caller/type/invariant evidence: The row stores mutable tool_name/server_name but no MCP server ID, association lifecycle generation, authenticated subject, OAuth grant/account generation, endpoint generation, or configuration fingerprint. ReAct resolves the callback by runtime name, while UserMCPServer.lifecycle_generation exists specifically as an immutable lifecycle identity but is not carried through the adapter/factory or compared at resume. The task-owner check mitigates cross-user task takeover, not connector replacement within the same task.
  • PR causation: The new frozen-row/resume design uses the opaque interaction ID as the sole lookup/claim identity and omits stable connector identity; the mutable connector behavior itself is pre-existing, but this PR makes it security- and correctness-relevant.
  • Specific fix: Persist an immutable connector association identity (at minimum server ID plus association lifecycle generation) together with an authenticated account/subject and a connection/config generation or equivalent fingerprint that changes on reauthorization and endpoint/auth changes. Preserve it through WebToolConfig/ToolFactory into the adapter, compare it atomically before claiming, and void without execution on mismatch. Add same-name mutation, account replacement, delete/recreate, and rename coverage.

5. One response can authorize every gated call in a concurrent batch

Location: src/xagent/core/tools/adapters/vibe/mcp_adapter.py:1375
Severity: major
Blocking: yes

  • Reachable trigger: Parallel execution is enabled, two consecutive non-read-only MCP calls are marked concurrency_safe, and both independently reach the new gate in one model response. ReAct supports this batch shape and creates one pending row/interaction ID per call.
  • Concrete impact: ReAct's response queue copies one raw response string to every pending callback. A bare approve is accepted by every adapter and executes every independently frozen external write from one response; a structured per-field answer is not routed and is rejected by the exact parser, voiding all rows. There is no explicit approve-all consent object, so the user cannot independently approve the distinct operations.
  • Caller/type/invariant evidence: concurrency_safe is an execution/idempotence property, not an authorization aggregation contract. The waiting protocol stores separate interaction IDs and the requirements describe one response for one suspended call; nothing establishes that one response means approve-all. The backend fan-out remains unchanged, but this PR introduces the production callback that interprets the broadcast token as permission for an external write.
  • PR causation: The new per-call confirm options and exact approve parser make the existing multi-waiting response fan-out an authorization defect; before this PR MCP calls did not use that callback to spend external writes.
  • Specific fix: Serialize approval-requiring writes, or carry a structured response map keyed by interaction ID/field and deliver only each selected machine value to its row. If approve-all is desired, make it an explicit grouped operation and machine value. Add a real concurrent pause/resume test covering approve-one/reject-one.

6. The built-in confirm UI cannot emit the accepted approval token

Location: src/xagent/core/tools/adapters/vibe/mcp_adapter.py:1371
Severity: major
Blocking: yes

  • Reachable trigger: A host deployment installs the documented gate, a direct MCP write is gated, and the user answers through the repository's normal task web UI. The new interaction declares type: "confirm" with machine option values approve and reject.
  • Concrete impact: The existing ClarificationForm renders confirm as a boolean switch, ignores interaction.options, and submits human-readable text such as Approve: Yes (or a localized equivalent). The adapter accepts only stripped/case-folded exact approve, so the affirmative click is interpreted as rejection; the store atomically transitions the row to voided and never executes the approved write.
  • Caller/type/invariant evidence: The frontend, WebSocket, backend, and ReAct waiting queue carry a plain answer string and do not translate display labels to option values. There is no upstream invariant that rewrites the built-in form's text into approve; the generic form's existing contract is intentionally human-readable. This finding is for direct host adapters; the separate sandbox boundary is covered by finding 1.
  • PR causation: The incompatible machine-valued interaction and exact-token parser are introduced by this PR while the generic confirm renderer/transport remains unchanged.
  • Specific fix: Preserve a transport-neutral machine-answer envelope through the form, WebSocket, backend, and runner, and have the renderer submit the selected option value while retaining human-readable transcript text separately. Do not broaden the parser to accept arbitrary localized labels, because that loses the explicit-consent invariant.

7. Expiry is sampled before database waits

Location: src/xagent/web/services/frozen_tool_call_store.py:116
Severity: major
Blocking: yes

  • Reachable trigger: Approval arrives shortly before the 24-hour deadline while the database connection is being checked out or the claim waits behind a writer/row lock. PostgreSQL pool checkout and SQLite writer-lock waits are supported deployment behavior; the same delay can occur between the stale expiry check and the unguarded update.
  • Concrete impact: now is captured before opening the session and before db.get; expired is computed from that old timestamp, and the update predicates only on interaction ID and pending status. A row can therefore be marked executed and its MCP network call started after expires_at, while settled_at records the earlier time. This violates the model invariant that a pending row past its expiry must not execute.
  • Caller/type/invariant evidence: Expiry is intentionally enforced by this consumer; there is no sweeper or upstream check that voids late rows. Database lock/checkout waits are legitimate finite delays, and neither backend transaction behavior supplies an expiry predicate at the claim linearization point.
  • PR causation: The new store introduces the pre-session timestamp, Python-only expiry decision, and claim update without an expiry condition.
  • Specific fix: Make the claim atomically require status='pending' and expires_at > a live claim-time clock, using dialect-appropriate statement-time semantics, and atomically transition expired pending rows to voided. Merely moving now after db.get is insufficient if update/commit can still wait.

8. Synchronous gate database I/O blocks the async event loop

Location: src/xagent/web/services/frozen_tool_call_store.py:85
Severity: major
Blocking: yes

  • Reachable trigger: An async run_json_async call or resume callback invokes the installed synchronous store hook while a database checkout, insert, update, or commit is slow or the synchronous pool is exhausted. These waits can occur under ordinary pool/database contention, not just malformed input.
  • Concrete impact: The event-loop thread is blocked for the DB wait/timeout, stalling unrelated agent tasks, lease heartbeats, and WebSocket work in that process. Under pool pressure this can amplify an availability incident; if the broad gate exception path catches the timeout, execution can also fall through ungated.
  • Caller/type/invariant evidence: The new async adapter calls consult_write_gate inline, and the hook starts a synchronous get_session_local() path at line 85 before performing session/commit I/O. The resume closure repeats synchronous SELECT/UPDATE/COMMIT work. There is no offloading boundary or invariant that makes these operations nonblocking; repository async owners use explicit bounded cancellation-safe DB offload patterns.
  • PR causation: The synchronous hook API and inline session work are introduced by this PR in an async hot path.
  • Specific fix: Make the gate/resume hook awaitable and run synchronous SQLAlchemy work through the repository's bounded, cancellation-safe offload primitive (or use an async session), preserving task-lease cancellation semantics for both initial gate and resume settlement.

9. Migration parity is claimed but not enforced (non-blocking)

Location: src/xagent/migrations/versions/20260903_add_frozen_tool_calls.py:24
Severity: minor
Blocking: no

  • Reachable trigger: A future maintainer changes a frozen-call model column, named constraint/index, type/default/nullability, schema behavior, or online/offline branch in only one of the two independent schema definitions. The migration comment says model/migration parity tests compare names and expressions, but no target-specific parity or migration test exists; the new service suite builds only Base.metadata.create_all().
  • Concrete impact: The two supported schema construction paths can silently drift, and the migration's online/offline, missing-parent, custom-schema, idempotence, and downgrade branches remain unasserted. There is no demonstrated current DDL mismatch—the model and migration agree by inspection—so this is a maintenance-contract/test-quality issue rather than a merge blocker.
  • Caller/type/invariant evidence: Normal startup upgrades existing databases with Alembic and builds fresh schemas with create_all, so both paths are real supported shapes. Existing migration parity suites are specific to task_interaction_requests; generic whole-chain tests skip or fail to assert this new table's guarded branches.
  • PR causation: The migration and its inaccurate parity-test claim are introduced by this PR, as is the direct-only service test suite that does not exercise the revision.
  • Specific fix: Add a focused migration/model parity suite following the existing migration conventions, covering SQLite/PostgreSQL online creation, offline DDL, idempotent upgrade/downgrade, missing parent, create-all-first, custom schema/search path, and downgrade residue. Alternatively remove the claim that those tests enforce parity; adding the tests is preferable because both schema paths are supported.

Blocking status & recommended decision

The following confirmed issues independently block merge:

  • src/xagent/core/tools/adapters/vibe/mcp_adapter.py:1352 — critical — supported sandboxed npx/uvx writes execute immediately without the host approval gate. [new]
  • src/xagent/core/tools/adapters/vibe/mcp_adapter.py:1704 — major — the approved external write runs without its result reaching the suspended tool/task flow. [new]
  • src/xagent/core/tools/adapters/vibe/mcp_adapter.py:1722 — major — approved resume bypasses current authorization, refresh/retry, and safe error handling. [new]
  • src/xagent/web/services/frozen_tool_call_store.py:135 — major — a frozen write can execute through a replacement endpoint or account with the same runtime name. [new]
  • src/xagent/core/tools/adapters/vibe/mcp_adapter.py:1375 — major — one response can authorize multiple independent concurrent external writes. [new]
  • src/xagent/core/tools/adapters/vibe/mcp_adapter.py:1371 — major — the built-in affirmative approval action voids the row instead of executing it. [new]
  • src/xagent/web/services/frozen_tool_call_store.py:116 — major — database wait races allow execution after the explicit approval expiry. [new]
  • src/xagent/web/services/frozen_tool_call_store.py:85 — major — synchronous gate/resume database waits block the async loop and can fail open. [new]

The migration parity item is minor and non-blocking.
Blocking: yes — REQUEST_CHANGES

# attempt. What the gate freezes is therefore exactly what would
# have run, and the retry paths below inherit the decision
# instead of each needing their own gate.
decision = consult_write_gate(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Critical, blocking — sandboxed npx/uvx calls bypass the gate. With the host gate installed, a supported stdio npx/uvx call is reconstructed by tool_runner.py in a fresh process where _HOOK is unset, so this consult_write_gate returns no decision and the write executes without a row or approval; the host wrapper has no resume capability either. Please move normalization/freezing to a host boundary above the sandbox, dispatch an approved prepared envelope through the guest without re-gating, expose host-side resume, and add a real wrapper/runner seam test.

"error": "This approval can no longer be completed.",
}
approved = _approval_response_is_grant(response)
return await hook(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Major, blocking — the resumed result is dropped. For an approved direct gated call, this callback returns the connector result, but ReAct's _deliver_pending_tool_interaction_responses only awaits it and checkpoints identifiers; it never attaches the value to the original tool_call_id/context/ledger/trace. The model can replan without knowing whether the external write succeeded. Please extend the resume protocol with structured completion and project success/error/cancel outcomes through the normal post-tool flow, then add a checkpoint/restore ReAct integration test.

from .....web.user_context import UserContext

with UserContext(self._get_current_user_id()).set_context():
return await self._execute_mcp_call(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Major, blocking — approved execution skips current auth and the normal MCP envelope. If allow_users is revoked before approval, or a delegated/resolver 401 or transport error occurs, this direct low-level call bypasses _is_user_allowed, refresh/retry, and safe error mapping; the row was already committed as executed, so the failure is terminal. Please share a prepared-execution helper with the ordinary path, recheck authorization before terminal claim, and preserve retry/error semantics.

executor: Callable[[Mapping[str, Any]], Any],
) -> Any:
session_local = get_session_local()
now = datetime.now(UTC)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Major, blocking — expiry is checked against a stale clock. Near the 24-hour deadline, DB checkout or lock wait can occur after now is sampled before the session/read; this old value can make expired false, and the later update has no expiry predicate, allowing the row and network call to execute after expires_at. Please make claim linearization atomically require pending plus a live claim-time expiry check and void expired rows.

"message": decision.message or f"Approve running {self.name}?",
"interactions": [
{
"type": "confirm",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Major, blocking — the built-in confirm cannot emit the accepted value. The normal ClarificationForm renders confirm as a boolean switch, ignores options, and sends Approve: Yes/localized display text, while the parser accepts only exact approve; an affirmative click therefore voids the row instead of executing the write. Please carry the selected machine option value through form/WebSocket/backend/runner and keep display text separate rather than broadening the parser.

result = db.execute(
update(FrozenToolCall)
.where(
FrozenToolCall.interaction_id == interaction_id,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Major, blocking — the claim is not connector-identity bound. While this row is pending, changing a same-name MCP URL/auth/env, reauthorizing to another account, or deleting/recreating the connector lets a rebuilt adapter execute these frozen arguments through the current connection; this predicate checks only interaction_id and pending. Please persist and carry an immutable server/association identity plus account/config generation, compare it atomically before claim, and void on mismatch.

"field": "approve",
"label": "Approve",
"options": [
{"label": "Approve", "value": "approve"},

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Major, blocking — one approval token can spend every concurrent row. Two supported concurrency_safe MCP writes can pause together, while ReAct fans one raw response to every pending callback; a bare approve therefore executes all independently frozen writes (and a per-field answer is rejected by the exact parser). Please serialize approval-requiring writes or route machine decisions by interaction ID; only make approve-all an explicit grouped consent, and test approve-one/reject-one.

return None
interaction_id = f"ftc_{uuid.uuid4().hex}"
now = datetime.now(UTC)
session_local = get_session_local()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Major, blocking — synchronous DB I/O runs on the async loop. This hook starts get_session_local() and then performs synchronous session/commit work inline from async gate/resume paths; legitimate pool or writer waits stall other agents, lease heartbeats, and WebSocket work, and a timeout can fall through the fail-open gate. Please use an awaitable hook with bounded cancellation-safe DB offload or an async session, including resume settlement.

The one CHECK and the named foreign key are rendered inline inside
op.create_table on both backends, so no ALTER TABLE ADD CONSTRAINT is ever
emitted and downgrade() is a single op.drop_table. The name-for-name
contract with the model's __table_args__ is enforced by the

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor, non-blocking — the stated parity tests are absent. This migration says model/migration names and CHECK expressions are enforced by parity tests, but the added suite uses create_all() and no target migration test exercises this revision's online/offline/idempotent/missing-parent/schema/downgrade branches, so future drift can pass CI. The current shapes agree, so this is not a runtime blocker; please add focused migration/model parity coverage or remove the claim.

OliverBryant added 2 commits September 7, 2026 14:51
Rework in progress for the round's wrong-direction verdict. Committed to
preserve it while the saas submodule pointer moves; the storage half is
expected to change again, see below.

What the review established, and what this changes. The gate lived inside
MCPToolAdapter.run_json_async, but a supported npx/uvx connector never runs
that method in this process: the sandbox serializes the adapter class and
tool_runner.py rebuilds it with importlib and cloudpickle in a guest, then
calls it there. Host module state -- the hook, a database session -- does
not exist on that side, so every sandboxed write executed ungated. The
first version even cited the guest reconstruction as its rationale and
still placed the gate where the guest runs it.

WriteGateTool now wraps whatever the loader produced, applied once where
the direct and sandboxed branches converge. SandboxedToolWrapper already
wraps the adapter, so wrapping outside it covers both transports
identically and the guest cannot reach the gate at all. Verified: both a
bare adapter and a sandbox-wrapped one pause with zero execution.

A replay re-enters the target's own run_json_async under a ContextVar
rather than reaching into _execute_mcp_call, so the authorization check,
UserContext, the delegated-credential retry and the safe error mapping all
still run. A ContextVar and not an attribute: the replay is awaited on the
same task, and instance state would leak across concurrent calls.

A gated tool reports concurrency_safe=False so ReAct runs it alone. That is
not a scheduling workaround: the pattern's own I5 contract makes the flag an
idempotency promise, and a call parked for approval is not idempotent. It
also closes the finding that one "approve" authorized every frozen write in
a batch, because ReAct copies one answer to every pending interaction.

The frozen row now carries a connector identity and the resume compares it
fail-closed, so an approval cannot be spent against a connector that was
repointed, reauthorized to another account, or recreated under its name.

Known incomplete: the 16 existing store tests drive the removed in-adapter
gate and still fail. More importantly, xorbitsai#1080 landed an equivalent approval
record in the saas layer (exact action, canonical arguments, the shown
preview, trusted identities, a deterministic digest) with no consumer yet,
so this PR's own frozen_tool_calls storage is likely to be replaced by
consuming that instead of running a second mechanism beside it.
…tore

xorbitsai/xagent-saas#1080 landed an approval record for one exact external
action -- canonical arguments, the preview actually shown, trusted Toby
identities, a deterministic digest -- and hands the stored action back to an
actor-bound continuation. It states it has no consumer yet: "a later
consumer must build the action, validate its trusted context, compare it
with the requested tool call, and execute it."

That is this PR. Running a second frozen-payload store beside it would give
two records of the same approval, two digests, and two chances to drift, so
the table, its migration, the store's freezing half and their tests are
removed. What stays is the half xorbitsai#1080 does not cover:

- Holding execution at a boundary above both transports. xorbitsai#1080 governs what
  was approved; nothing there stops a connector call from running before the
  answer arrives, and the sandboxed npx/uvx route in particular rebuilds the
  adapter in a guest process.
- Replaying through the target's own run_json_async, so authorization, the
  delegated-credential retry and the safe error mapping still apply.
- Refusing to be batched, so one answer cannot authorize several writes.
- Connector identity. xorbitsai#1080's trusted context carries workspace, channel,
  conversation, task, application, EndUser and Slack user -- who approved,
  in which conversation. It does not carry which MCP server or which OAuth
  account the write will reach, and an account-relative call such as
  create_post does not name its account in its arguments. So the identity
  comparison stays here, still fail-closed.

The xagent side is now only the seam: no database, no saas import, storage
supplied entirely by the host. That is also what keeps it correct inside the
sandbox, where sqlalchemy is not installed.

Verified end to end on both transports: gated calls pause with zero
execution, an approval replays the stored arguments byte for byte, and a
gated tool reports concurrency_safe=False.

Tests are not rewritten yet -- the previous suite drove the removed
in-adapter gate and went with it. The gate therefore has no coverage at this
commit, which is why the PR is going back to draft rather than up for
re-review.
@OliverBryant

Copy link
Copy Markdown
Contributor Author

Back to draft while I act on the round-0 verdict — the direction changed twice, so rather than let a re-review chase a moving target, here is where it stands.

The verdict was right, and about the thing I had argued for

The gate sat in MCPToolAdapter.run_json_async, and a supported npx/uvx connector never runs that method in this process: the sandbox serializes the adapter class and tool_runner.py rebuilds it with importlib + cloudpickle in a guest, then calls it there. No host hook, no database. Every sandboxed write executed ungated.

What makes this worse than an oversight is that the PR cited the guest reconstruction as its rationale — "sqlalchemy is not installed in the sandbox, so the gate must be a host-installed hook" — and then placed the gate exactly where the guest runs it. The fact was right; the conclusion drawn from it was not.

Fixed as you described: WriteGateTool wraps whatever the loader produced, applied once where the direct and sandboxed branches converge. SandboxedToolWrapper already wraps the adapter, so wrapping outside it covers both transports identically and the guest cannot reach the gate at all. Verified on both: gated calls pause with zero execution.

Findings 3 and 5 follow from the same move. The replay re-enters the target's own run_json_async under a ContextVar instead of reaching into _execute_mcp_call, so _is_user_allowed, UserContext, the delegated-credential retry and the safe error mapping all still run. And a gated tool now reports concurrency_safe=False, so ReAct runs it alone — not as a scheduling workaround but because the pattern's own I5 note makes that flag an idempotency promise, and a call parked for approval is not idempotent.

A second change of direction, which is why this is draft

While reworking, xorbitsai/xagent-saas#1080 merged: an approval record for one exact external action — canonical arguments, the preview actually shown, trusted Toby identities, a deterministic digest — handing the stored action to an actor-bound continuation. It says it has no consumer yet, and that a later consumer must compare the action with the requested tool call and execute it.

So this PR should be that consumer rather than a second store beside it. The table, its migration, the freezing half of the store and their tests are removed; two records of one approval would mean two digests and two chances to drift.

What remains is the half #1080 does not cover: holding execution at a boundary above both transports, replaying through the normal path, refusing to be batched, and connector identity. On that last one I initially assumed #1080's digest subsumed finding 4 and was wrong: its trusted context carries workspace, channel, conversation, task, application, EndUser and Slack user — who approved, in which conversation — not which MCP server or OAuth account the write will reach. An account-relative call like create_post does not name its account in its arguments, so that comparison stays here and stays fail-closed.

The xagent side is now only the seam: no database, no saas import, storage supplied entirely by the host — which is also what makes it correct inside the sandbox.

Not done

Tests. The previous suite drove the removed in-adapter gate and went with it, so the gate has no coverage at this commit. Rewriting it against the new boundary — including the npx/uvx wrapper/runner seam test you asked for, identity mismatch, and the batch case — is the next step, and it is why CI being green here means nothing yet.

Finding 2 (ReAct discarding the resumed result) is deliberately not in this PR. It extends the resumable-interaction protocol in react.py, which every HITL tool shares, so it is going up separately rather than inside an integration change.

@OliverBryant
OliverBryant marked this pull request as draft September 7, 2026 07:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants