Skip to content

Support codex_exec configuration aliases and fix sandbox propagation - #220

Open
RohithPariki wants to merge 1 commit into
microsoft:mainfrom
RohithPariki:fix-issue-209
Open

Support codex_exec configuration aliases and fix sandbox propagation#220
RohithPariki wants to merge 1 commit into
microsoft:mainfrom
RohithPariki:fix-issue-209

Conversation

@RohithPariki

Copy link
Copy Markdown

Fixes #209

Bug description

previously, SkillOpt with codex_exec backend skipped all rollouts or failed because:

  1. codex_harness.py skipped passing the --sandbox flag whenever --full-auto was active (the default), causing the Codex CLI to default to workspace-write (requiring bwrap, which fails in Docker).
  2. the user-provided YAML configurations (like sandbox: danger-full-access or codex_cli_bin) were not mapped correctly into the internal codex_exec_* keys during flattening, rendering them ineffective.

Changes made

  • codex_harness.py: Updated _run_codex_cli_exec to always append the --sandbox parameter alongside --full-auto, enabling user-specified sandboxes (e.g., danger-full-access) to work in Docker.
  • config.py: Expanded _FLATTEN_MAP to properly map model.sandbox, model.codex_sandbox, model.codex_cli_bin, and model.codex_path aliases to codex_exec_sandbox and codex_exec_path.
  • backend_config.py & codex_backend.py: Updated environment variables to seamlessly map both CODEX_EXEC_* and CODEX_* variable names and to synchronize them when configure_codex_exec() is called.
  • trainer.py: Passed all valid alias keys to configure_codex_exec() during the training setup.
  • tests: Added comprehensive unit tests in test_codex_config_aliases.py covering flattening, env variable loading, and CLI sandbox parameter passing.

@Yif-Yang

Copy link
Copy Markdown
Contributor

Thanks for taking this on—the config mismatch and sandbox propagation are worth fixing. There are a few items we need addressed before merge:

  1. Please rebase this branch onto current main and drop commit 87f46c0; it is the complete, unrelated fix: narrow broad exception handlers to prevent silent error swallowing #216 change set and makes this PR stacked on another open PR.
  2. Please do not solve the sandbox issue by combining --sandbox with --full-auto. OpenAI’s current Codex documentation marks codex exec --full-auto as deprecated in favor of an explicit --sandbox mode, and the 0.147 release removes that compatibility flag. Please translate the intended sandbox/approval behavior into explicit supported arguments/config instead, and cover both full_auto settings without relying on the legacy flag.
  3. Please wire and test alias precedence through the real entry points. At present the environment aliases can still be overwritten when trainer.py / scripts/eval_only.py call configure_codex_exec() with default "codex" and "workspace-write"; flat aliases are also handled differently by training and eval-only. One end-to-end config test for each entry point would make the contract clear. Please either avoid the broad model.sandbox alias or document it explicitly as Codex-only and validate its allowed values.
  4. The added test file currently fails Ruff (12 I001/F401/W293 findings) and leaks module/environment state into later tests. Please use fixtures/monkeypatch with restoration, remove the unused pytest import and the except Exception: pass, and leave the global Codex config unchanged after each test.
  5. Since [Bug] codex_exec backend: rollouts skipped (calls=0) + bwrap sandbox blocks file reads despite danger-full-access #209 is specifically a Docker/bwrap failure, please report a real Codex CLI smoke test (including CLI version) showing that danger-full-access reaches the process and resolves the reported scenario; the current subprocess mocks only verify argv construction. Please also add the accepted aliases and the security warning for danger-full-access to the config docs.

Relevant Codex CLI documentation: https://learn.chatgpt.com/docs/non-interactive-mode#permissions-and-safety

Once those are separated and covered, we’ll be happy to re-review the focused fix.

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.

[Bug] codex_exec backend: rollouts skipped (calls=0) + bwrap sandbox blocks file reads despite danger-full-access

2 participants