Skip to content

refactor(server): remove the generic ACP client fs guard and write grants - #13633

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

juliusmarminge merged 2 commits into
t3code/codex-turn-mappingfrom
v2/acp-remove-client-fs-guard

Conversation

@juliusmarminge

@juliusmarminge juliusmarminge commented Sep 25, 2026 •

Copy link
Copy Markdown
Member

Since #13623 only Antigravity serves client fs, and its handlers confine every request to the workspace and attachments dir. Antigravity also asks through session/request_permission before each edit. The generic fs policy guard in front of those handlers, and the write grants that let an fs write follow an approved edit, were built for agents that routed all their file work through T3. None do anymore.

Part of the ACP work ("rely on the agent's own sandboxes and permission models rather than implementing our own, like Claude and Codex"). Restacked directly onto t3code/codex-turn-mapping after #13623 merged; it does not depend on #13629.

What changed

  • AcpAdapterV2: removed guardClientFsRead/guardClientFsWrite. An opted-in flavor's fs handlers are registered directly.
  • AcpClientPolicy: removed acpClientReadDisposition, acpClientWriteDisposition, and the write-root grants (allowsWrite, canonical write roots, MAX_GRANTED_WRITE_ROOTS). makeAcpClientPolicyGrants now records only command approvals. The module doc now says what T3 still mediates.
  • Kept: acpPermissionDisposition (answers every agent's permission prompts), the MCP elicitation disposition, acpClientExecuteDisposition with command grants, and AcpClientTerminals for Devin's client terminals. The path canonicalization helpers stay because the workspace-write sandbox still uses them to check permission-request locations.
  • AcpProviderCapabilitiesV2 now describes client-boundary truthfully: ACP agents run their own tools, and T3 only answers their permission requests by policy.
  • New test AntigravityAdapterV2 client file system under restrictive policies > serves in-workspace reads and writes and still refuses outside paths. Under a read-only sandbox and under approval-required, Antigravity reads and writes inside the workspace and is refused outside it. With the guard still in place the write is denied and this test fails, which pins the delegation described below.
  • Tests removed, and what covers them now:
    • AcpAdapterV2 > confines client-mediated writes under an explicit workspace-write sandbox and > serves client-mediated reads without approval in approval-required mode exercised the removed guard through a test-only fs handler. Antigravity's containment is covered by AntigravityAdapterV2 client file system (fix(server): Antigravity keeps its workspace containment #13613). Non-opted-in agents are covered by the method-not-found test (refactor(server): ACP client fs and terminals are opt-in per flavor #13623).
    • AcpClientPolicy.test.ts > client-mediated dispositions (fs parts) and the write-grant cases of makeAcpClientPolicyGrants tested only removed functions. The terminal disposition and command-grant cases were rewritten against the smaller API. AcpAdapterV2 > does not turn an unknown permission approval into an execute grant still covers the grant end to end.

Decisions (override if you disagree)

  • Antigravity's client fs handlers are not policy-gated anymore. Antigravity asks for each edit itself, and its handlers confine paths to the workspace and attachments dir. Under a read-only sandbox policy, an approved Antigravity edit is now written, where before the guard denied it. Antigravity's native mode (default for approval-required) still asks first.
  • Devin's client terminals keep T3's execute guard until Devin's own ask mode over ACP is probed.

Verification

In apps/server, with TMPDIR under /home:

  • vp test run src/orchestration-v2/Adapters/{AcpAdapterV2,AcpRegistryAdapterV2,AntigravityAdapterV2,GrokAdapterV2}.test.ts src/provider/acp/AcpClientPolicy.test.ts src/provider/acp/AcpClientTerminals.test.ts src/orchestration-v2/testkit/OrchestratorReplayFixtures.integration.test.ts: 251 passed, including all 87 replay fixtures.
  • vp exec tsc --noEmit -p .: clean. vp run knip:check: clean. vp lint on the touched files: only pre-existing warnings.
  • Not run: live Antigravity or Devin (neither installed here).

Restack verification (head fc7e66b, on 6107b66): vp test run on AcpAdapterV2, AcpRegistryAdapterV2, AntigravityAdapterV2, GrokAdapterV2, AcpClientPolicy, AcpClientTerminals: 166 passed. OrchestratorReplayFixtures -t "grok|antigravity|acpRegistry": 21 passed. tsc --noEmit -p apps/server: clean. knip:check: clean. vp lint on touched files: only the pre-existing unused NodePath import in AcpAdapterV2.ts.

Model: Claude Opus 5.5 (Claude Code)

🤖 Generated with Claude Code


Devin Review

@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:L 100-499 changed lines (additions + deletions). labels Sep 25, 2026
guardClientFsWrite(request.path).pipe(
Effect.andThen(clientFileSystem.writeTextFile(request, sessionCwd)),
),
clientFileSystem.writeTextFile(request, sessionCwd),

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.

Removing the guard changes how opted-in fs/* requests behave, but this PR deletes the focused adapter tests without replacing them. Could you add a test using the test filesystem layer that exercises an opted-in write outside a workspace-write sandbox (and a read in approval-required mode) to verify the intended delegation?

Posted via Macroscope — Effect Service Conventions

@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.2 KiB — 2.0 KiB ✅
Codex Live turn WebSocket decoded — 20.4 KiB — 29.3 KiB ✅
Codex Live turn messages — 2 — 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: 7cb3cd4 · 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.

@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 removes T3's generic ACP filesystem authorization guard, allowing the opted-in Antigravity path to rely on native agent approvals while retaining provider-specific path containment. Because this changes a production filesystem security boundary and the exact delegation behavior lacks the requested workspace-write adapter coverage, it warrants human review.

You can add or adjust custom eligibility rules. Learn more.

@juliusmarminge
juliusmarminge force-pushed the v2/acp-remove-client-fs-guard branch from 4bef658 to bd9cc73 Compare September 25, 2026 11:23
@juliusmarminge
juliusmarminge force-pushed the v2/acp-remove-client-fs-guard branch from bd9cc73 to 7cb3cd4 Compare September 25, 2026 17:24
juliusmarminge and others added 2 commits September 25, 2026 16:32
…ants

Client fs is now opt-in and only Antigravity serves it, from handlers that
confine requests to the workspace, while the agent asks before its own
edits. Drop the fs policy guard in front of them, the fs dispositions, and
the approval grants that let a client write follow an approved edit. The
execute disposition and command grants stay for Devin's client terminals.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…s own approvals

Without the generic guard, an opted-in flavor serves its fs requests under
any runtime policy, confined to the workspace. Under a read-only sandbox
and under approval-required, Antigravity reads and writes inside the
workspace and is still refused outside it.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@juliusmarminge
juliusmarminge force-pushed the v2/acp-remove-client-fs-guard branch from 7cb3cd4 to fc7e66b Compare September 25, 2026 23:37
@juliusmarminge
juliusmarminge changed the base branch from v2/acp-native-modes to t3code/codex-turn-mapping September 25, 2026 23:37
@github-actions github-actions Bot added size:XXL 1,000+ changed lines (additions + deletions). and removed size:L 100-499 changed lines (additions + deletions). labels Sep 25, 2026
@juliusmarminge
juliusmarminge merged commit 86e3487 into t3code/codex-turn-mapping Sep 25, 2026
13 checks passed
@juliusmarminge
juliusmarminge deleted the v2/acp-remove-client-fs-guard branch September 25, 2026 23:39
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