Skip to content

sandbox: add yolo-mode option - #3

Closed
sorelmitra-goodleap wants to merge 1 commit into
andreidavid:mainfrom
sorelmitra-goodleap:sorelmitra/add-yolo-mode-option
Closed

sandbox: add yolo-mode option#3
sorelmitra-goodleap wants to merge 1 commit into
andreidavid:mainfrom
sorelmitra-goodleap:sorelmitra/add-yolo-mode-option

Conversation

@sorelmitra-goodleap

Copy link
Copy Markdown

Codex reviews can fail when --full-auto adds another sandbox inside an already sandboxed Claude session.

  • Add opt-in YOLO mode while keeping --full-auto as the default.
  • Add commands to enable, disable, and show the setting because Claude renders Boolean plugin options as text fields.

Co-authored-by: OpenAI Codex <noreply@openai.com>

@andreidavid andreidavid left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

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

  1. Model the option as a sandbox-mode enum, not a boolean. Proposed: a string option sandbox_mode with values workspace-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-auto deprecated ("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-access is likely the right fix for the nested-sandbox problem — probing 0.144.5 I found --dangerously-bypass-approvals-and-sandbox additionally skips codex's trusted-directory gate, i.e. bypass relaxes more than the sandbox, and your scenario only needs the sandbox part relaxed; (c) shipping yolo_mode: true and 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's case validate — unknown/unset falls through to the sandboxed default, which keeps it fail-safe. --sandbox X is two argv tokens, so the single-token CODEX_REVIEW_MODE_FLAG becomes an array, expanded "${CODEX_REVIEW_MODE_ARGS[@]}" at both invocation sites (the timeout and non-timeout branches). Unset keeps --full-auto for 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].

  2. 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) bypass makes 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-review and /codex-review-plan still 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 your status subcommand does; slash-command Bash calls don't receive the hook env vars).

  3. 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 receives CLAUDE_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
  4. 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-plugins is sufficient (vs. needing a restart)? The failure mode otherwise is silent: status reads settings.json and reports one thing while the hook quietly does another.

Smaller items

  1. chmod --reference is 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).
  2. Missing CHANGELOG.md entry (CONTRIBUTING asks for one with behavior changes).
  3. Hardcoded plugin ID codex-review@andreidavid in the helper — fine to keep, but worth a comment acknowledging forks (inline suggestion).
  4. configure-yolo-mode.sh isn't a hook, so when it gets renamed for the enum, consider a scripts/ dir rather than hooks/scripts/.

Comment thread README.md

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.

@andreidavid andreidavid Jul 24, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

(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).

Suggested change
- `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

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

--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:

Suggested change
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"

@andreidavid andreidavid Jul 24, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

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.)

Suggested change
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"

Comment on lines +38 to +40
if [ "${CLAUDE_PLUGIN_OPTION_YOLO_MODE:-false}" = "true" ]; then
CODEX_REVIEW_MODE_FLAG="--dangerously-bypass-approvals-and-sandbox"
fi

@andreidavid andreidavid Jul 24, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

(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) ;;
esac

Unknown/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.

@sorelmitra-goodleap

Copy link
Copy Markdown
Author

I've addressed all these in sorelmitra/add-sandbox-mode-option

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.

3 participants