sandbox: add yolo-mode option - #3
Conversation
Co-authored-by: OpenAI Codex <noreply@openai.com>
There was a problem hiding this comment.
Thanks for this — it addresses a real problem (Codex's Landlock/Seatbelt sandbox failing inside an already-sandboxed Claude session), and the implementation is in good shape.
Before reviewing I verified the plugin-options mechanism against the current Claude Code docs (plugins-reference), since it's not widely used yet: userConfig in plugin.json, storage under pluginConfigs["codex-review@andreidavid"].options in user-scope settings.json (exactly where your script writes — note project-scope is ignored for this key since v2.1.207), and export to hook processes as CLAUDE_PLUGIN_OPTION_<KEY> are all real and correctly named. I also ran the CI checks on your branch: bats 54/54 green, shellcheck clean, claude plugin validate passes. The default path is untouched, the feature fails safe, and the args-capture extension to the test stub is a nice minimal way to test this.
Requesting changes: one design-level ask (the shape of the option) plus a few small items. Happy to push any of these to the branch myself if you're short on time — just say the word.
(Edited: item 1 below was originally a closing "for the record" heads-up that said the boolean was fine to land as-is; after more thought I'd rather get the option's shape right now than migrate a shipped setting later. The inline comments on the README and the hook were updated to match. Apologies if you read the earlier version.)
Must-fix
-
Model the option as a sandbox-mode enum, not a boolean. Proposed: a string option
sandbox_modewith valuesworkspace-write/danger-full-access/bypass, mapped in the hook:CODEX_REVIEW_MODE_ARGS=(--full-auto) case "${CLAUDE_PLUGIN_OPTION_SANDBOX_MODE:-}" in workspace-write) CODEX_REVIEW_MODE_ARGS=(--sandbox workspace-write) ;; danger-full-access) CODEX_REVIEW_MODE_ARGS=(--sandbox danger-full-access) ;; bypass) CODEX_REVIEW_MODE_ARGS=(--dangerously-bypass-approvals-and-sandbox) ;; esac
Why: (a) codex 0.144.5 marks
--full-autodeprecated ("use--sandbox workspace-write"), so a boolean whose "off" position means a deprecated flag bakes yesterday's CLI into the option's meaning; (b)danger-full-accessis likely the right fix for the nested-sandbox problem — probing 0.144.5 I found--dangerously-bypass-approvals-and-sandboxadditionally skips codex's trusted-directory gate, i.e.bypassrelaxes more than the sandbox, and your scenario only needs the sandbox part relaxed; (c) shippingyolo_mode: trueand switching to an enum later means a user-facing settings migration — cheaper to get the shape right now; (d) bonus: a string option sidesteps the open question of how a boolean value serializes into the env var.Notes: the manifest schema has no enum type, so declare it
"type": "string"and let the hook'scasevalidate — unknown/unset falls through to the sandboxed default, which keeps it fail-safe.--sandbox Xis two argv tokens, so the single-tokenCODEX_REVIEW_MODE_FLAGbecomes an array, expanded"${CODEX_REVIEW_MODE_ARGS[@]}"at both invocation sites (thetimeoutand non-timeoutbranches). Unset keeps--full-autofor now so nothing changes for older codex CLIs; the deprecated flag then disappears from the default path whenever the minimum codex version gets bumped. This also dovetails with collapsing the three commands into one/codex-review-sandbox-mode [workspace-write|danger-full-access|bypass|status]. -
Sharpen the security framing and keep the docs honest about scope — see the inline suggestion on the README (updated to the enum shape). Two parts: (a)
bypassmakes unattended reviews run unsandboxed on untrusted input (the reviewed diff itself), so the prompt-injection consequence and "only inside a disposable container/VM" guidance should be explicit; (b) the option only affects the post-commit hook —/codex-reviewand/codex-review-planstill hardcode--full-auto, so in the environment this PR targets, manual reviews still hit the same sandbox failure. Either scope the docs to "automatic post-commit reviews" (the suggestion does that) or extend the skill/plan command to honor the setting too (they can read settings.json the way yourstatussubcommand does; slash-command Bash calls don't receive the hook env vars). -
SessionStart visibility. The plugin already warns at session start when reviews silently won't run; "reviews silently run with elevated access" deserves the same one line. Roughly, in
session-start-check.sh(it receivesCLAUDE_PLUGIN_OPTION_*like any hook):case "${CLAUDE_PLUGIN_OPTION_SANDBOX_MODE:-}" in danger-full-access|bypass) emit "codex-review plugin: sandbox_mode=$CLAUDE_PLUGIN_OPTION_SANDBOX_MODE -- automatic reviews run with elevated access." ;; esac
-
One live end-to-end confirmation. The bats tests inject the env var themselves, so they pass regardless of whether the harness actually delivers the value. Can you confirm you've observed the full cycle live: set the option →
/reload-plugins→ commit → expected flag actually in codex's argv? And that/reload-pluginsis sufficient (vs. needing a restart)? The failure mode otherwise is silent:statusreads settings.json and reports one thing while the hook quietly does another.
Smaller items
chmod --referenceis GNU-only — silently no-ops on macOS, so settings.json drifts to mktemp's 0600 there. CONTRIBUTING bans GNU-only flags; inline suggestion with a portable equivalent (applies unchanged to the enum version of the helper).- Missing
CHANGELOG.mdentry (CONTRIBUTING asks for one with behavior changes). - Hardcoded plugin ID
codex-review@andreidavidin the helper — fine to keep, but worth a comment acknowledging forks (inline suggestion). configure-yolo-mode.shisn't a hook, so when it gets renamed for the enum, consider ascripts/dir rather thanhooks/scripts/.
|
|
||
| Plugin option: | ||
|
|
||
| - `yolo_mode` — Defaults to `false`, which runs Codex with `--full-auto`. When `true`, reviews run with `--dangerously-bypass-approvals-and-sandbox`. This gives Codex unrestricted filesystem and command access. Useful if you run Claude in yolo mode in a sandbox. |
There was a problem hiding this comment.
(Updated for the enum request — review item 1.) This suggestion rewrites the bullet for sandbox_mode; apply it together with the code change, not before. It keeps the two things the original comment asked for: explicit security framing (unattended + untrusted input, so the prompt-injection consequence is spelled out) and scope honesty (/codex-review and /codex-review-plan still hardcode --full-auto — if you extend those to honor the setting instead, adjust the last sentence).
| - `yolo_mode` — Defaults to `false`, which runs Codex with `--full-auto`. When `true`, reviews run with `--dangerously-bypass-approvals-and-sandbox`. This gives Codex unrestricted filesystem and command access. Useful if you run Claude in yolo mode in a sandbox. | |
| - `sandbox_mode` — `workspace-write` (default: sandboxed, today's behavior), `danger-full-access` (no filesystem sandbox; usually the right fix when Codex's own sandbox fails inside an already-sandboxed Claude session), or `bypass` (`--dangerously-bypass-approvals-and-sandbox`: unrestricted filesystem, command, and network access, and it also skips Codex's trusted-directory check). Reviews run unattended on untrusted input — with `bypass`, a prompt-injection payload in a reviewed change could execute arbitrary commands — so only use it inside a container/VM you consider disposable. Applies to automatic post-commit reviews only; manual `/codex-review` and `/codex-review-plan` always use the sandboxed default. |
| .pluginConfigs[$plugin_id].options.yolo_mode = $value | ||
| ' "$SETTINGS_FILE" > "$SETTINGS_TMP" | ||
|
|
||
| chmod --reference="$SETTINGS_FILE" "$SETTINGS_TMP" 2>/dev/null || true |
There was a problem hiding this comment.
--reference is GNU-only — on macOS/BSD this silently no-ops (the 2>/dev/null || true guard hides it) and settings.json ends up with mktemp's 0600 after the mv. Harmless functionally, but CONTRIBUTING bans GNU-only flags. Portable equivalent:
| chmod --reference="$SETTINGS_FILE" "$SETTINGS_TMP" 2>/dev/null || true | |
| ORIG_MODE=$(stat -c %a "$SETTINGS_FILE" 2>/dev/null || stat -f %Lp "$SETTINGS_FILE" 2>/dev/null || true) | |
| if [ -n "$ORIG_MODE" ]; then | |
| chmod "$ORIG_MODE" "$SETTINGS_TMP" | |
| fi |
(stat -c is GNU, stat -f %Lp is BSD/macOS; the trailing || true keeps set -e happy if both fail, and the if instead of && avoids the same set -e trip on the guard itself.)
| set -euo pipefail | ||
|
|
||
| ACTION="${1:-status}" | ||
| PLUGIN_ID="codex-review@andreidavid" |
There was a problem hiding this comment.
Worth acknowledging that this only matches the canonical marketplace name — someone installing from a fork under a different marketplace name gets a toggle that writes a key the harness never reads. (The hook path itself is unaffected either way: the harness computes the CLAUDE_PLUGIN_OPTION_* env var from the real plugin ID.)
| PLUGIN_ID="codex-review@andreidavid" | |
| # Plugin ID as installed from the canonical marketplace. If you install this | |
| # plugin from a fork under a different marketplace name, adjust this to match | |
| # (only this helper cares -- the hook reads the CLAUDE_PLUGIN_OPTION_* env var). | |
| PLUGIN_ID="codex-review@andreidavid" |
| if [ "${CLAUDE_PLUGIN_OPTION_YOLO_MODE:-false}" = "true" ]; then | ||
| CODEX_REVIEW_MODE_FLAG="--dangerously-bypass-approvals-and-sandbox" | ||
| fi |
There was a problem hiding this comment.
(Updated for the enum request — review item 1; the earlier version of this comment suggested a defensive boolean match, now superseded.)
With sandbox_mode as a string option, this block becomes a mode-to-argv map. Deliberately not a one-click suggestion, because the change spans more than these three lines: --sandbox X is two argv tokens, so the single-token CODEX_REVIEW_MODE_FLAG (line 37) needs to become an array, expanded as "${CODEX_REVIEW_MODE_ARGS[@]}" at both invocation sites below (the timeout and non-timeout branches). Shape:
CODEX_REVIEW_MODE_ARGS=(--full-auto)
case "${CLAUDE_PLUGIN_OPTION_SANDBOX_MODE:-}" in
workspace-write) CODEX_REVIEW_MODE_ARGS=(--sandbox workspace-write) ;;
danger-full-access) CODEX_REVIEW_MODE_ARGS=(--sandbox danger-full-access) ;;
bypass) CODEX_REVIEW_MODE_ARGS=(--dangerously-bypass-approvals-and-sandbox) ;;
esacUnknown/unset values fall through to the sandboxed default, which doubles as validation for the free-text option (the manifest schema has no enum type). Nice side effect vs. the boolean: the "how does a boolean serialize into the env var" question becomes moot — strings pass through verbatim.
|
I've addressed all these in sorelmitra/add-sandbox-mode-option |
Codex reviews can fail when
--full-autoadds another sandbox inside an already sandboxed Claude session.--full-autoas the default.