Skip to content

test(server): assert the read-only on-request write by its permission - #13562

Merged
juliusmarminge merged 1 commit into
t3code/codex-turn-mappingfrom
v2/grok-read-only-on-request
Sep 25, 2026
Merged

juliusmarminge merged 1 commit into
t3code/codex-turn-mappingfrom
v2/grok-read-only-on-request

Conversation

@juliusmarminge

@juliusmarminge juliusmarminge commented Sep 25, 2026 •

Copy link
Copy Markdown
Member

The shared tool_call_read_only_on_request assertion pinned a shell command_execution and a command request, so it only held when the agent chose the shell. The prompt allows "a local shell command or file edit", and live Grok 1.0.41 uses its write tool, which asks for a file change. That is why #13537 left the Grok variant synthetic.

Change

The fixture tests the permission, not the tool. One assertion (tool_call_read_only_on_request/output.ts) now covers Codex, Claude, Grok and the ACP registry agent, replacing the Codex-shaped and Claude-specific copies:

  • the run completes
  • exactly one runtime request, resolved with accept, of kind command or file-change (whichever the provider's tool needs)
  • exactly one approval card, for that request and kind
  • a completed command_execution (for command) or file_change (for file-change) targeting .codex-probe-write-action.txt
  • wherever that item carries content (command input, newStr / diffStr), it contains the requested text

The Grok variant is re-recorded live: write → session/request_permission (kind: "edit", location = the probe file) → allow-once → fs/write_text_file with the exact text. The registry transcript is synthetic; its command now writes the requested text, so the content check covers it too.

Findings, not fixed here

  • An approved write can't read its own file. In this recording, after the user approved the write, T3 denied Grok's fs/read_text_file on the same path twice ("requires approval…"). The cause is in AcpClientPolicy. With any approval policy other than never, acpOperationDisposition returns ask for every operation, reads included. An accepted file-change records only a write grant for its locations, so the read falls through to deny. Grok continued and wrote the file anyway. There is a broader consequence: Grok never asks permission for its read_file tool and routes reads through client fs/read_text_file, so under on-request or approval-required policies T3 denies every Grok read. The narrow fix is to let a file-change grant cover reads of the same locations. The broader question is whether a read-only or workspace sandbox should still ask for reads when approval is on-request. That's a policy call, so I left it for a maintainer.
  • ACP v1 diffs lose their content. Grok's edit reports content: [{ type: "diff", path, oldText, newText }] (the ACP v1 shape). The adapter's file_change projection reads only the v2 patch.text, so the Grok file change reaches the timeline with no diffStr / newStr. The assertion therefore checks content only where the projection carries it.

Verification

  • vp test run src/orchestration-v2/testkit/OrchestratorReplayFixtures.integration.test.ts: 85 passed (all four tool_call_read_only_on_request variants).
  • The new assertion rejects the old synthetic registry command (printf fixture > …): the replay fails on the content check.
  • Grok variant recorded live against Grok 1.0.41.
  • vp exec tsc --noEmit -p . in apps/server: 0 error TS / warning TS. vp run knip:check: clean. vp lint / vp fmt on touched files: clean.
  • Not run: repo-wide suites.

Stacked on #13537.

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
macroscopeapp[bot]
macroscopeapp Bot previously approved these changes Sep 25, 2026
@macroscopeapp

macroscopeapp Bot commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at cddebc6

Macroscope's review found this PR approvable — This PR strengthens deterministic replay tests so the existing read-only/on-request policy is asserted by permission outcome regardless of whether a provider chooses a command or file-edit tool. All changes are limited to test assertions and replay transcripts, with no production runtime, product-default, schema, or deployment impact.

Notes:

  • Diff unchanged. Approvability was decided on eligibility alone.

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.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: cddebc6 · 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.

Base automatically changed from v2/grok-recorder to t3code/codex-turn-mapping September 25, 2026 05:26
@macroscopeapp
macroscopeapp Bot dismissed their stale review September 25, 2026 05:26

Dismissing prior approval to re-evaluate a38b183

The shared tool_call_read_only_on_request assertion pinned a shell
command and a `command` request, so it only held when the agent picked
the shell. The prompt allows a shell command or a file edit; live Grok
uses its write tool, which asks for a file change.

One assertion now covers Codex, Claude, Grok and the ACP registry
agent: exactly one request, accepted and resolved, of kind command or
file-change; one approval card for it; a completed command_execution
or file_change for the probe file whose content, when projected, is
the requested text. The Grok variant is re-recorded live, and the
registry's synthetic command now writes the requested text.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
macroscopeapp[bot]
macroscopeapp Bot previously approved these changes Sep 25, 2026
@juliusmarminge
juliusmarminge force-pushed the v2/grok-read-only-on-request branch from a38b183 to cddebc6 Compare September 25, 2026 05:32
@macroscopeapp
macroscopeapp Bot dismissed their stale review September 25, 2026 05:32

Dismissing prior approval to re-evaluate cddebc6

@juliusmarminge
juliusmarminge merged this pull request into t3code/codex-turn-mapping Sep 25, 2026
24 checks passed
@juliusmarminge
juliusmarminge deleted the v2/grok-read-only-on-request branch September 25, 2026 05:39
juliusmarminge added a commit that referenced this pull request Sep 25, 2026
…#13562)

Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L 100-499 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