Skip to content

Resolve the task sandbox from config.toml and add task --read-only#508

Open
cubicj wants to merge 3 commits into
openai:mainfrom
cubicj:config-driven-sandbox
Open

Resolve the task sandbox from config.toml and add task --read-only#508
cubicj wants to merge 3 commits into
openai:mainfrom
cubicj:config-driven-sandbox

Conversation

@cubicj

@cubicj cubicj commented Jul 15, 2026

Copy link
Copy Markdown

Summary

The plugin always overrides the user's configured sandbox_mode with hardcoded sandbox values (#482). buildThreadParams/buildResumeParams fall back to "read-only" whenever the caller passes nothing, and the task path hardcodes the --write ternary to "workspace-write"/"read-only", so a sandbox_mode in ~/.codex/config.toml never reaches any job. Users who want config-driven sandboxes (e.g. danger-full-access for trusted delegation setups) end up patching the installed plugin by hand.

Change

Task paths now send sandbox: null when the caller does not ask for a specific sandbox, letting the app-server resolve it from config.toml — the same mechanism the plugin already relies on for model (model: options.model ?? null falls back to the configured model today). An explicit --write still forces workspace-write, and both review paths (native and adversarial) deliberately keep their pinned read-only sandbox.

  • lib/codex.mjs: buildThreadParams and buildResumeParams change options.sandbox ?? "read-only" to options.sandbox ?? null. ThreadStartParams.sandbox is declared SandboxMode | null, so this is type-clean.
  • codex-companion.mjs: the task path changes request.write ? "workspace-write" : "read-only" to request.write ? "workspace-write" : request.readOnly ? "read-only" : null.

Because omitting --write now defers to config instead of guaranteeing read-only, the same change adds a task --read-only flag as the per-run way to pin the read-only sandbox: it is mutually exclusive with --write (a combined call fails with a clear error), survives the background request round trip into the detached worker, and applies to resumed threads. The rescue command/agent/skill texts are updated one line each so that read-only intent means passing --read-only rather than merely omitting --write. The stop-review-gate hook's internal task now passes --read-only explicitly, so every review-shaped run — native review, adversarial review, and the stop-time gate task — keeps a pinned read-only sandbox regardless of config.

Behavior only changes for users who set sandbox_mode in config.toml: when it is absent, the app-server's own default for an unset sandbox remains read-only, so stock installations keep today's behavior. Inheriting a permissive mode from config is a deliberate user choice made in their own Codex configuration, and #506 (resolved model/effort/sandbox capture, from #481) makes the inherited value visible on every job record. An explicit per-run escalation flag (#145, PR #147) is complementary to this config-driven default and intentionally out of scope here.

Verification

  • Live probe against codex-cli 0.145.0-alpha.12: thread/start with sandbox: null and config.toml sandbox_mode = "danger-full-access" returned the config-resolved {"type":"dangerFullAccess"} sandbox in the response echo; explicit values still override.
  • npm test: 94/94 passing. The fake-codex fixture now records thread/start/thread/resume params, with assertions covering every path: a plain task sends sandbox: null, --write sends workspace-write, --read-only pins read-only on fresh, resumed, and background-worker threads, --write --read-only is rejected, and native review, adversarial review, and the stop-review-gate hook's task all still send read-only.
  • npm run build (typecheck via tsconfig.app-server.json): clean.
  • git diff --check against the base commit: clean.

Related Issues

Fixes #482

@cubicj
cubicj requested a review from a team July 15, 2026 10:59
axisrow added a commit to axisrow/codex-plugin-cc that referenced this pull request Jul 21, 2026
…d-only (port openai#508) (#33)

* feat(sandbox): resolve task sandbox from config.toml + add task --read-only (port openai#508)

Port of @cubicj's openai#508. The plugin hardcoded the
sandbox ("read-only"/"workspace-write"), ignoring sandbox_mode in
~/.codex/config.toml. Now the task path sends sandbox: null when the caller
does not pin one, letting the app-server resolve it from config — the same
mechanism already used for model. Explicit --write still forces
workspace-write; review paths keep their pinned read-only.

- buildThreadParams/buildResumeParams: sandbox ?? null (config-driven default)
- task --read-only flag: per-run read-only pin, mutually exclusive with --write
- stop-review-gate hook's internal task passes --read-only explicitly so the
  gate stays read-only regardless of config
- rescue command/agent/skill docs updated for --read-only

Merged onto the fork (conflicts in codex.mjs, codex-companion.mjs, rescue.md,
fake-codex-fixture, runtime.test): kept our reasoning-validation +
turn-timeout-ms + model aliases + --cwd, layered cubicj's sandbox-null +
--read-only on top. Our fake-codex fixture already recorded sandbox in
lastThreadStart/lastThreadResume, so openai#508's assertions pass as-is.

Verified: npm run build clean; npm test 174/171 pass / 3 pre-existing (was
4 — the background test that previously asserted a foreign model-luna value
now passes). All 6 new sandbox tests green.

Co-Authored-By: Claude <noreply@anthropic.com>

* fix(sandbox): fail closed when a read-only pin is not honored on resume (cycle-review critical)

Codex adversarial review of #33 flagged — and a real-codex smoke confirmed —
that Codex 0.144.6 keeps a loaded thread's sandbox and ignores thread/resume
overrides: `task --read-only --resume-last` on a write-capable thread ran
with workspace-write access, violating the --read-only contract.

Verified empirically (codex-cli 0.144.6): write-thread -> resume with
--read-only returned resolved.sandbox = workspaceWrite. The code never
compared the resolved sandbox to the explicit pin.

Fix: assertExplicitSandboxHonored() in runAppServerTurn compares an explicit
read-only pin to the app-server's resolved sandbox and throws on mismatch —
fail closed instead of running write-capable under a read-only flag. Only the
read-only pin is enforced (a write request on a read-only resolved sandbox is
a downgrade, not a safety leak). TDD: a resume-ignores-sandbox fake-codex
behavior reproduces the leak (RED), the assertion turns it green.

Also confirms the port's cubicj claim: a plain task with sandbox:null resolves
to read-only from config (smoke: resolved readOnly), so stock behavior is
unchanged.

Co-Authored-By: Claude <noreply@anthropic.com>

---------

Co-authored-by: axisrow <axisrow@users.noreply.github.com>
Co-authored-by: Claude <noreply@anthropic.com>
@axisrow

axisrow commented Jul 21, 2026

Copy link
Copy Markdown

@cubicj — just wanted to say thanks for this PR (and #506). I maintain a fork (axisrow/codex-plugin-cc) where the plugin has drifted enough from upstream that merging here has been slow, and your config-driven sandbox work is genuinely useful — I ported #508 into my fork this week.

One thing came up while porting that's worth flagging back to you, since it's about the --read-only path this PR introduces: Codex 0.144.6 ignores the sandbox override on thread/resume for an already-loaded thread. I verified it against a real app-server: a thread created under workspace-write, then resumed with --read-only, returns resolved.sandbox = workspaceWrite and would run write-capable despite the explicit read-only pin. So task --read-only --resume-last on a prior write thread silently violates the flag.

My fork's fix (in case it's useful): after the thread/start-or-resume response, compare the requested pin to the app-server's resolved sandbox and fail closed on mismatch — --read-only requested but resolved ≠ readOnly → throw, don't start the turn. Scoped to the read-only pin only (a write request landing on a read-only resolved sandbox is a downgrade, not a leak). The stock config-default path your PR adds is fine and unchanged — a plain task still resolves to readOnly from config.

Fork PR (port + the guard), with attribution to this PR: axisrow#33. Pure sharing — take it, adapt it, or set it aside. Either way, thanks for the PRs; they're the kind of substantive contribution this repo needs more of.

axisrow added a commit to axisrow/codex-plugin-cc that referenced this pull request Jul 21, 2026
Ships accumulated fixes since fork.4:
- #29 broker-busy hang (PR #30): bound broker handshake
- #31 spawned-app-server handshake (PR #32): bound request() primitive
- openai#508 config-driven sandbox + --read-only (PR #33) + resume-sandbox fail-closed
- #34 model/effort in /codex:rescue launch line (PR #35)
- #28 turn-timeout gaps (already in fork.4 base)

Co-Authored-By: Claude <noreply@anthropic.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c6207a5144

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread plugins/codex/scripts/codex-companion.mjs
@cubicj
cubicj marked this pull request as draft July 22, 2026 01:57
@cubicj
cubicj marked this pull request as ready for review July 22, 2026 01:59
@cubicj

cubicj commented Jul 22, 2026

Copy link
Copy Markdown
Author

@axisrow
Thanks for porting the PR and for flagging this — I reproduced it before replying, with a JSON-RPC probe against a directly spawned app-server on codex-cli 0.145.0-alpha.29. The boundary is exactly the "already-loaded" condition you described: the resume sandbox param only applies when the server loads the thread from its rollout, and once the thread is in memory, resume is a re-attach that echoes current state. That matters here because the plugin's shared broker keeps one long-lived app-server per session, so task --read-only --resume-last after a write-capable run is precisely the loaded-thread case.

I've pushed a guard in the same fail-closed shape you described: after thread/start/thread/resume, the explicit read-only pin is compared against the echoed SandboxPolicy and the turn is refused on mismatch (enforceReadOnlySandbox in plugins/codex/scripts/lib/codex.mjs), scoped to the read-only pin only, with a resume-ignores-sandbox fixture behavior and a regression test. The echo stayed truthful in every case I probed, including the ones where the pin was ignored, which is what makes the check reliable.

The silent ignore itself looks like an app-server issue — thread/resume accepting a sandbox param and disregarding it leaves callers no signal short of checking the echo, which is what the guard does. Thanks again for sending the finding back upstream.

@axisrow

axisrow commented Jul 22, 2026

Copy link
Copy Markdown

Thanks for reproducing it independently on 0.145.0-alpha.29 — that confirms the loaded-thread boundary and makes the guard feel a lot less like a one-off workaround. Appreciate you carrying it upstream.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Hardcoded sandbox values always override config.toml sandbox_mode

2 participants