Skip to content

fix(server): show diffs for ACP edits that send oldText/newText - #13579

Merged
juliusmarminge merged 1 commit into
t3code/codex-turn-mappingfrom
v2/acp-legacy-diff
Sep 25, 2026
Merged

juliusmarminge merged 1 commit into
t3code/codex-turn-mappingfrom
v2/acp-legacy-diff

Conversation

@juliusmarminge

@juliusmarminge juliusmarminge commented Sep 25, 2026 •

Copy link
Copy Markdown
Member

Grok's file edits reached the V2 timeline with no diff. Grok sends tool-call diff content as { type: "diff", path, oldText, newText }, but the ACP adapter read only patch.text, so the file_change item had no diffStr.

Is it part of the ACP spec, or a v1 vs v2 difference?

It's a v1 vs v2 difference. Each shape belongs to exactly one protocol version. T3 vendors both schemas in effect-acp (generator pins: schema-v1.21.0 and schema-v2.0.0-alpha.3).

  • ACP v1 (stable, schema-v1.21.0): ToolCallContent diff is { type: "diff", path, oldText?: string | null, newText }. oldText is documented as "The original content (None for new files)". There is no patch field. See schema-v1.gen.ts#L5185 and Diff at #L8202.
  • ACP v2 draft (schema-v2.0.0-alpha.3): the diff is { type: "diff", changes: DiffChange[], patch?: { format: "git_patch", text } | null }. It has no path, oldText or newText. changes is authoritative, and "Clients MUST handle diffs where patch is omitted or null". See schema.gen.ts#L5863, Diff at #L8528 and DiffPatch at #L2803.
  • T3's version-neutral type already accepts both variants (compat.ts#L249), and AcpRuntimeModel keeps both (#L379). T3 asks for v2 in initialize (client.ts#L728) but falls back to v1 when the agent answers with the v1 response shape (#L1124-L1128). A v1 session's tool_call updates pass through unchanged (normalizeV1SessionUpdate). The one place the v1 diff was lost was the adapter's file_change projection.

What Grok sends

Grok is on ACP v1. It builds on agent-client-protocol 0.10.4, and at f0e3be1 it answers initialize with ProtocolVersion::V1 (acp_agent.rs:533). Every file edit is acp::Diff::new(path, new_text).old_text(...), which is the v1 shape:

Grok's own TUI renders these by diffing old_text/new_text itself (xai-grok-pager-diff/src/lib.rs:317).

The live recording from #13562 (Grok 1.0.41) shows the same. initialize answers protocolVersion: 1 (transcript line 3). The write reports { type: "diff", path: "<workspace>/.codex-probe-write-action.txt", oldText: "", newText: "codex app-server approval fixture." } on its pending and completed updates (lines 95 and 103).

So Grok is behind the v2 spec rather than off-spec. It sends the correct shape for the version it negotiates.

Fix

T3 follows ACP v2. It should still render what a provider actually sends after negotiating v1. acpToolCallDiffPatch replaces structuredDiffPatch and stays at the adapter boundary:

  • v2: a patch.text is returned as sent, exactly as before.
  • v1: otherwise, each { path, oldText, newText } entry becomes a unified patch through the server's existing diff (jsdiff 8.0.3) dependency, already used in azureDevOpsDiff.ts. No new library. oldText null or absent diffs against /dev/null. Multiple files are joined into one patch, and unchanged entries are skipped.
  • Cost: an edit with text on both sides is capped at 1,000 edits of diff search, so a large rewrite keeps no patch text and cannot stall the event loop. A created or emptied file needs no search and is never capped: 20k lines take about 45 ms.

For Grok's recorded write the item now carries:

--- <workspace>/.codex-probe-write-action.txt
+++ <workspace>/.codex-probe-write-action.txt
@@ -0,0 +1,1 @@
+codex app-server approval fixture.
\ No newline at end of file

(oldText: "" means Grok says the file existed and was empty, so the header names the file rather than /dev/null.)

The shared tool_call_read_only_on_request assertion no longer skips a file_change that carries no content. #13562 added that skip only because of this bug.

Scope: diffStr stays in persistence and is stripped from the wire projection (WireProjection.ts). Today it is read by the orchestrator MCP timeline text (OrchestratorMcpService.ts:672), not by the web or mobile timeline. This PR fixes what the ACP adapter records. It does not change any client.

Verification

  • Before the fix, the tightened assertion fails only the Grok variant: AssertionError: the approved file_change must carry what it wrote: expected undefined to not equal undefined (OrchestratorReplayFixtures.integration.test.ts -t tool_call_read_only_on_request: 1 failed, 3 passed). With the fix: 4 passed.
  • Suites: vp test run on OrchestratorReplayFixtures.integration.test.ts, OrchestratorReplayFixtures.contract.test.ts, AcpAdapterV2.test.ts, GrokAdapterV2.test.ts, AcpRegistryAdapterV2.test.ts and AcpRuntimeModel.test.ts: 6 files, 268 tests passed.
  • Unit tests: acpToolCallDiffPatch has pure-logic tests for the multi-file v1 patch with /dev/null, v2 patch pass-through, and the rewrite cap.
  • Checks: vp exec tsc --noEmit -p . in apps/server gave 0 error TS / warning TS. vp run knip:check is clean. vp lint on the touched files adds no new warnings (the three it reports in AcpAdapterV2.test.ts are pre-existing). vp fmt is clean.
  • Not run: repo-wide suites, and a new live Grok recording (the test(server): assert the read-only on-request write by its permission #13562 recording already contains the v1 edit).

Model: Claude Opus 5.5 (Claude Code)

🤖 Generated with Claude Code


Devin Review

ACP v1 tool-call diff content is { type: "diff", path, oldText, newText };
v2 replaced it with { changes, patch }. The ACP adapter read only the v2
patch.text, so file changes from agents that negotiate v1 (Grok answers
initialize with protocolVersion 1) reached the timeline with no diff.

Build the patch from oldText/newText with the server's existing jsdiff
dependency (/dev/null for a new file), keeping v2 patch text as sent.
The Grok read-only on-request replay now requires the file_change to carry
what it wrote.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Sep 25, 2026
@github-actions

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: e1d0d09 · 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: Approved at e1d0d09

Macroscope's review found this PR approvable — This is a small, self-contained ACP adapter bug fix that adds generated diff text for legacy edit payloads while preserving existing v2 handling. The runtime impact is limited to file-change presentation, with targeted tests and a safeguard against expensive large rewrites.

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

@juliusmarminge
juliusmarminge merged commit 1f2f91d into t3code/codex-turn-mapping Sep 25, 2026
24 of 25 checks passed
@juliusmarminge
juliusmarminge deleted the v2/acp-legacy-diff branch September 25, 2026 06:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M 30-99 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