Skip to content

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

Closed
cubicj wants to merge 32 commits into
openai:mainfrom
cubicj:config-driven-sandbox
Closed

cubicj wants to merge 32 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

dkundel-openai and others added 30 commits March 30, 2026 09:42
Co-authored-by: Codex <[email protected]>
Added instructions for reloading plugins and setup command.
…#13)

On Windows, Node.js `spawnSync` without `shell: true` uses
`CreateProcess`, which only resolves `.exe` files. npm installs global
tools (like `codex`) as `.cmd` shims, so `spawnSync("codex", ...)`
returns ENOENT even when codex is correctly installed and on PATH.

Adding `shell: process.platform === "win32"` routes through `cmd.exe`
on Windows, which properly resolves `.cmd`, `.bat`, and PATHEXT entries.
No behavior change on macOS/Linux since the condition evaluates to false.
* chore: bump plugin version to 1.0.1

* chore: bump plugin metadata version to 1.0.1
* fix: add shell and windowsHide options for Windows spawn in app-server

On Windows, spawn("codex", ["app-server"]) fails with ENOENT because
Node.js cannot resolve .cmd shims without shell: true. This adds
platform-gated shell and windowsHide options to the app-server spawn
call, and uses terminateProcessTree for proper process tree cleanup
since shell: true wraps the child in cmd.exe.

Without terminateProcessTree, plain SIGTERM only kills cmd.exe and
leaves the actual codex node process orphaned — verified with 274+
zombie node.exe processes accumulating on Windows.

Fixes #32
Fixes #46


* fix: guard terminateProcessTree against PID reuse after process exit

ChildProcess.killed only reflects whether .kill() was called by this
process — it stays false when the child exits on its own. On Windows,
where PIDs are recycled quickly, the 50 ms timer could fire after
cmd.exe has exited and its PID has been reassigned, causing taskkill
to terminate an unrelated process.

Add an exitCode === null check so the tree-kill path is skipped once
the child has already exited.


---------
* fix: make test suite portable across platforms

Replace hardcoded macOS path with fileURLToPath for cross-platform
compatibility. Add .cmd wrapper creation and platform-aware PATH
separator in test fixtures so the fake codex binary is discoverable
on Windows. Add shell and windowsHide options to the test helper
run() function to match production behavior.

Test results on Windows improve from 12/64 pass to 59/64 pass.


* fix: skip shell for absolute-path commands to avoid Windows space-in-path breakage

When `process.execPath` resolves to a path with spaces (e.g.,
`C:\Program Files\nodejs\node.exe`), `shell: true` causes cmd.exe
to split the path at the space. Guard with `path.isAbsolute()` so
only bare command names (which need `.cmd` shim resolution) use the
shell.


---------

Co-authored-by: Dominik Kundel <[email protected]>
* fix: skip untracked directories in review context

* fix: skip broken untracked symlinks in reviews
* Use app-server auth status for Codex readiness

* fix: reuse existing app server for auth checks
…provided (#159)

`SpawnedCodexAppServerClient.initialize()` passes `this.options.env` to
`spawn()`, but no caller ever sets `env` in options. In Node.js, passing
`undefined` for `env` gives the child process **no** environment variables,
breaking any model provider that relies on env vars (e.g. DATABRICKS_TOKEN).

Fall back to `process.env` when `this.options.env` is not set, matching the
existing pattern in `broker-lifecycle.mjs` and `codex-companion.mjs`.

Co-authored-by: Isaac

Co-authored-by: Bhuvanesh Sridharan <[email protected]>
…#126)

* fix: gracefully handle unsupported thread/name/set on older Codex CLI

Codex CLI v0.118.0 does not recognize the thread/name/set JSON-RPC method,
causing startThread() to throw. Thread naming is cosmetic (for job log
labels) and should not block thread creation. Wraps the call in try/catch
so it fails silently on older CLI versions.

Fixes #119

* refactor: only suppress unsupported-method errors for thread/name/set

Address Codex review feedback: the bare catch swallowed all errors
including auth, network, and server failures. Now only suppresses
errors containing 'unknown variant' or 'unknown method' (the specific
error older CLI versions return) and rethrows everything else.
* fix: avoid embedding large adversarial review diffs

* fix: preserve untracked content in lightweight review

* address comments

* fix: handle ENOBUFS type check in git diff sizing
* codex: honor --cwd when reporting session runtime

* codex: keep session runtime lookup scoped to callers

---------

Co-authored-by: VOIDXAI <[email protected]>
* fix: declare model in codex-rescue agent frontmatter

The codex-rescue agent had no model field, leaving Claude Code to assign
whatever default it chooses. As a thin forwarding wrapper that issues a
single Bash call, this agent is well-suited to the haiku tier; declaring
it explicitly ensures a predictable cost profile and tier guarantee.


* Update codex-rescue.md

---------

Co-authored-by: Dominik Kundel <[email protected]>
Closes #77

Updated README to replace unsupported 'xhigh' value with 'high' to prevent configuration errors.
Unquoted \$ARGUMENTS in the ! shell commands allowed shell metacharacters
in user-supplied job IDs to be expanded before Node received them (e.g.,
`task-123; malicious-cmd` would execute the trailing command). This is
inconsistent with review.md and adversarial-review.md, which both wrap
"$ARGUMENTS" in double quotes.
…on (#234) (#235)

* fix: route /codex:rescue through the Agent tool to stop Skill recursion (#234)

`/codex:rescue` previously combined two things that together caused a hang:

- `context: fork` in the frontmatter, which spawns a `general-purpose`
  subagent for the command body.
- Body prose "Route this request to the `codex:codex-rescue` subagent."
  without naming the transport.

When the main agent called `Skill(codex:rescue)` programmatically, the
fork resolved the ambiguous prose by trying `Skill(codex:codex-rescue)`
(unknown skill) and then falling back to `Skill(codex:rescue)`, which
re-entered this command and hung the session until the user cancelled.
No Codex job was ever created.

Naming the transport as `Agent(codex:codex-rescue)` alone is not enough:
forked general-purpose subagents do not expose the `Agent` tool, so the
forked runner cannot reach the subagent that way either. The minimal fix
is therefore two coordinated changes:

- Drop `context: fork` so the command body runs inline in the calling
  agent's context, where `Agent` is in scope.
- Say explicitly "use the `Agent` tool with `subagent_type:
  "codex:codex-rescue"`", and call out that `Skill(codex:codex-rescue)`
  and `Skill(codex:rescue)` are not valid routing paths. Add `Agent`
  to `allowed-tools` so the call does not prompt for permission.

Everything else in rescue.md (resume-candidate check, flag handling,
background/foreground semantics, operating rules) is unchanged. The
`codex:codex-rescue` subagent itself is unchanged.

Tests pin the new allow-list, the explicit `subagent_type`, the ban on
`Skill(codex:codex-rescue)`, and the absence of `context: fork`. The
existing "run the `codex:codex-rescue` subagent in the background"
assertion continues to hold since that sentence still reads correctly
with the Agent-tool transport.

Fixes #234

* test: match quoted result and cancel command arguments

---------

Co-authored-by: Dominik Kundel <[email protected]>
* Remove shell expansion for git commands

* Version bump
@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 <[email protected]>

* 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 <[email protected]>

---------

Co-authored-by: axisrow <[email protected]>
Co-authored-by: Claude <[email protected]>
@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 <[email protected]>

@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".

@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