Skip to content

refactor(server): ACP client fs and terminals are opt-in per flavor - #13623

Merged
juliusmarminge merged 2 commits into
t3code/codex-turn-mappingfrom
v2/acp-client-fs-opt-in
Sep 25, 2026
Merged

juliusmarminge merged 2 commits into
t3code/codex-turn-mappingfrom
v2/acp-client-fs-opt-in

Conversation

@juliusmarminge

@juliusmarminge juliusmarminge commented Sep 25, 2026 •

Copy link
Copy Markdown
Member

V2 offered client fs (read and write) to every ACP agent, and client terminals to every registry agent. Agents that honor those capabilities route their file and shell work through T3, where T3 runs it with the server's own privileges and a policy guard of its own, not through the agent's permission model. Grok is one: it uses AcpSessionFs only when the client advertises both fs capabilities and otherwise falls back to its own LocalFs (agent_ops.rs:4160, :4193-4205), and does the same for terminals (:4209-4224, AcpTerminalRunner vs TerminalRunner).

Layer 2 of the ACP stack ("rely on the agent's own sandboxes and permission models rather than implementing our own, like Claude and Codex"). Stacked on #13613.

What changed

  • Client fs is opt-in per flavor. clientCapabilities.fs follows the flavor's clientFileSystem (fix(server): Antigravity keeps its workspace containment #13613), and without it no fs handler is registered, so effect-acp answers a stray fs/* request with method-not-found. OpenCode and Kilo send one after approved edits whatever the capability says. Only Antigravity sets clientFileSystem.
  • Client terminals are opt-in too. AcpRegistryAdapterV2 passes clientTerminals only for Devin. Grok and Antigravity never had them.
  • The generic, unconfined AcpClientFs.ts and its test are deleted, since nothing uses them now. The runtime-policy guards stay in front of Antigravity's handlers and Devin's terminals. Removing them is a later layer.
  • The Grok recorder now pins the clientCapabilities T3 advertises instead of writing <any>, so a transcript records whether Grok was offered fs or terminals.
  • tool_call_read_only_on_request and todo_list for Grok were the only Grok fixtures with fs/* frames. I re-recorded both live against grok 1.0.41. Grok now reads and writes the workspace itself: todo_list has no permission requests, and tool_call_read_only_on_request asks once (session/request_permission, kind: "edit") for its own write. A new shared assertion, assertNoAcpClientFileOrTerminalRequests, checks that the initialize advertises neither capability and that no fs/* or terminal/* frame occurs. The old Grok assertion ("T3 must serve Grok's client-mediated read") is replaced by these checks.
  • T3 still answers every permission prompt by policy. Nothing changed there.

Known risks

  • OpenCode and Kilo send a stray fs/write_text_file after an approved edit whatever the capability says, as an un-awaited promise with no catch (opencode/src/acp/permission.ts writeProposedEdit, void this.input.connection.writeTextFile(...); Kilo packages/opencode/src/acp/permission.ts:121). T3 now answers method-not-found, so that promise rejects unhandled inside the agent process. Whether Bun kills the process on that is unverified: neither agent's ACP mode is installed here. Before this change the same write ran with the server's privileges.

Decisions (override if you disagree)

  • Antigravity keeps client fs with its own containment handler (fix(server): Antigravity keeps its workspace containment #13613) until someone probes whether its default mode asks with a diff when fs is off.
  • Devin keeps client terminals (it needs them, and reportedly has no ask mode over ACP) until it is probed. Devin loses client fs.
  • Unmapped registry agents get fs:false/terminal:false, and T3 answers their permission prompts by policy.

Overlap

  • The Grok replay-gates branch (v2/grok-gates) also edits record-grok-acp-replay-fixture.ts and fixtures/shared.ts, in different hunks. Its new grok_monitor recording has no fs/* frames and keeps clientCapabilities: "<any>", which still matches.
  • The stack sits on fix(server): V2 Grok launches in the thread's permission mode #13616 (Grok runtime mode), so both fixtures were recorded with its launch flags (--permission-mode default for this read-only on-request policy).

Verification

All commands ran in apps/server with TMPDIR under /home:

  • vp test run src/orchestration-v2/testkit/OrchestratorReplayFixtures.integration.test.ts: 87/87 passed, including the two re-recorded Grok fixtures. Before re-recording, the old transcripts failed as expected (the replay agent waited for fs/* responses T3 no longer serves).
  • vp test run src/orchestration-v2/testkit/OrchestratorReplayFixtures.contract.test.ts: passed.
  • vp test run src/orchestration-v2/Adapters/{AcpAdapterV2,AcpRegistryAdapterV2,AntigravityAdapterV2,GrokAdapterV2}.test.ts src/provider/Drivers/AntigravityDriver.test.ts: 153 passed. The two adapter tests for the fs policy guard now opt in through a test clientFileSystem. New tests:
    • AcpAdapterV2 > answers fs requests method-not-found when the flavor does not opt into client fs: the ACP mock agent (new T3_ACP_CLIENT_FS_PROBE_PATH behavior) sends fs/write_text_file then fs/read_text_file on a prompt whatever the client advertised. The initialize carries fs: false, terminal: false, both requests get -32601, and no file is written. With the old always-on generic handlers put back, the test fails.
    • AcpRegistryAdapterV2 > offers client terminals to Devin only and client fs to no registry agent: Devin advertises terminal: true, gemini terminal: false, both fs: false. Giving every registry agent terminals fails it.
  • Live recordings: node scripts/record-grok-acp-replay-fixture.ts --scenario tool_call_read_only_on_request and --scenario todo_list with T3_GROK_BIN=~/.local/bin/grok, re-run after rebasing onto fix(server): V2 Grok launches in the thread's permission mode #13616. Both passed their live assertions. One earlier todo_list take (before the rebase) wrote the todo list once, already completed, and failed the fixture's "two plan updates" check; that was the model's choice, not this change.
  • vp exec tsc --noEmit -p .: clean. vp run knip:check: clean. vp lint on the touched files: only warnings that already exist on the base branch.
  • Not run: the live registry agents and Antigravity (not installed here), and repo-wide checks.

Model: Claude Opus 5.5 (Claude Code)

🤖 Generated with Claude Code

@macroscopeapp

macroscopeapp Bot commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Macroscope skipped reviewing this pull request. Per-review cost limit exceeded (workspace setting).

This review would cost an estimated $40.87, which exceeds your per-review limit of $15.00.

The top 3 files driving up this estimate:

File Diff Size Estimate
apps/server/src/orchestration-v2/testkit/fixtures/todo_list/grok_transcript.ndjson 521.54KB $26.08
apps/server/src/orchestration-v2/testkit/fixtures/tool_call_read_only_on_request/grok_transcript.ndjson 281.93KB $14.10
apps/server/src/orchestration-v2/Adapters/AcpAdapterV2.ts 5.23KB $0.26

Tip

To get this pull request reviewed, you can:

  1. Comment @macroscope-app on this PR to request a manual review (monthly spend limits still apply).
  2. Exclude the file(s) above from review by adding a pattern to your .macroscope/ignore.md — note that creating this file replaces Macroscope's built-in default ignores rather than extending them.
  3. Raise your cost limit in your workspace billing settings.

Turn off this reminder going forward

@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:XXL 1,000+ changed lines (additions + deletions). labels Sep 25, 2026
@macroscopeapp

macroscopeapp Bot commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Not approved

Macroscope's review found this PR not approvable — This PR changes production defaults for ACP filesystem and terminal capabilities, shifting existing agents between T3-mediated and agent-owned file and shell execution. The added coverage reduces uncertainty, but the default behavior change requires human review.

Not approved because:

  • Per-review cost limit exceeded (workspace setting). Approvability relies on correctness review in order to determine eligibility

Review your spending limits in Billing settings, or comment @macroscope-app review this PR to bypass the limit and review now. You can add or adjust custom eligibility rules. Learn more.

@github-actions

github-actions Bot commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

ℹ️ No successful main baseline artifact is available yet. This run establishes the initial measurement.

Provider Metric Main baseline This PR Impact PR ceiling
Codex Total thread wire — 4.9 KiB — 6.8 KiB ✅
Codex Thread snapshot wire — 3.7 KiB — 4.9 KiB ✅
Codex Live turn WebSocket wire — 1.1 KiB — 2.0 KiB ✅
Codex Live turn WebSocket decoded — 20.4 KiB — 29.3 KiB ✅
Codex Live turn messages — 1 — 8 ✅
Claude Total thread wire — 4.9 KiB — 6.8 KiB ✅
Claude Thread snapshot wire — 3.7 KiB — 4.9 KiB ✅
Claude Live turn WebSocket wire — 1.2 KiB — 2.0 KiB ✅
Claude Live turn WebSocket decoded — 20.8 KiB — 29.3 KiB ✅
Claude Live turn messages — 2 — 8 ✅

Baseline: unavailable · PR result: c8ec40f · Source CI: success

Scenario and decoded snapshot size

10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.

  • Codex decoded thread snapshot: 106.1 KiB
  • Claude decoded thread snapshot: 106.4 KiB

Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

@juliusmarminge
juliusmarminge force-pushed the v2/antigravity-containment branch from e5a3e33 to dcb5ce1 Compare September 25, 2026 09:40
@@ -5329,25 +5333,23 @@ export function makeAcpAdapterV2(options: AcpAdapterV2Options): ProviderAdapterV
Effect.succeed(request),
requestContext.requestId,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This changes the no-clientFileSystem path to leave both handlers unregistered, but the existing filesystem tests now explicitly opt in, so they no longer exercise that path. Could you add a focused adapter test that omits clientFileSystem and verifies read/write requests are rejected without touching disk?

Posted via Macroscope — Effect Service Conventions

juliusmarminge and others added 2 commits September 25, 2026 16:00
Every ACP agent was offered client fs, so agents like Grok routed their file
reads and writes through T3 instead of their own permission model. Now only
Antigravity (its own workspace-contained handlers) gets client fs and only
Devin gets client terminals; everyone else reads, writes and runs commands
itself and asks through session/request_permission. The unconfined generic
fs handlers are gone, so a stray fs request gets method-not-found.

The Grok recorder pins the clientCapabilities T3 advertises, and the two
Grok fixtures that carried fs frames are re-recorded live against grok
1.0.41: Grok asks once for its own write and sends no fs or terminal
requests.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The mock agent can now send fs/write_text_file and fs/read_text_file on
every prompt regardless of what the client advertised. A generic flavor
without clientFileSystem answers both with method-not-found and nothing is
written. A registry test pins client terminals to Devin and client fs to no
registry agent.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@juliusmarminge
juliusmarminge merged commit 6107b66 into t3code/codex-turn-mapping Sep 25, 2026
24 checks passed
@juliusmarminge
juliusmarminge deleted the v2/acp-client-fs-opt-in branch September 25, 2026 23:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XXL 1,000+ changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant