Commit Graph

12 Commits

  • [codex] Move config loading into codex-config (#19487)
    ## Why
    
    Config loading had become split across crates: `codex-config` owned the
    config types and merge logic, while `codex-core` still owned the loader
    that assembled the layer stack. This change consolidates that
    responsibility in `codex-config`, so the crate that defines config
    behavior also owns how configs are discovered and loaded.
    
    To make that move possible without reintroducing the old dependency
    cycle, the shell-environment policy types and helpers that
    `codex-exec-server` needs now live in `codex-protocol` instead of
    flowing through `codex-config`.
    
    This also makes the migrated loader tests more deterministic on machines
    that already have managed or system Codex config installed by letting
    tests override the system config and requirements paths instead of
    reading the host's `/etc/codex`.
    
    ## What Changed
    
    - moved the config loader implementation from `codex-core` into
    `codex-config::loader` and deleted the old `core::config_loader` module
    instead of leaving a compatibility shim
    - moved shell-environment policy types and helpers into
    `codex-protocol`, then updated `codex-exec-server` and other downstream
    crates to import them from their new home
    - updated downstream callers to use loader/config APIs from
    `codex-config`
    - added test-only loader overrides for system config and requirements
    paths so loader-focused tests do not depend on host-managed config state
    - cleaned up now-unused dependency entries and platform-specific cfgs
    that were surfaced by post-push CI
    
    ## Testing
    
    - `cargo test -p codex-config`
    - `cargo test -p codex-core config_loader_tests::`
    - `cargo test -p codex-protocol -p codex-exec-server -p
    codex-cloud-requirements -p codex-rmcp-client --lib`
    - `cargo test --lib -p codex-app-server-client -p codex-exec`
    - `cargo test --no-run --lib -p codex-app-server`
    - `cargo test -p codex-linux-sandbox --lib`
    - `cargo shear`
    - `just bazel-lock-check`
    
    ## Notes
    
    - I did not chase unrelated full-suite failures outside the migrated
    loader surface.
    - `cargo test -p codex-core --lib` still hits unrelated proxy-sensitive
    failures on this machine, and Windows CI still shows unrelated
    long-running/timeouting test noise outside the loader migration itself.
  • Build remote exec env from exec-server policy (#17216)
    ## Summary
    - add an exec-server `envPolicy` field; when present, the server starts
    from its own process env and applies the shell environment policy there
    - keep `env` as the exact environment for local/embedded starts, but
    make it an overlay for remote unified-exec starts
    - move the shell-environment-policy builder into `codex-config` so Core
    and exec-server share the inherit/filter/set/include behavior
    - overlay only runtime/sandbox/network deltas from Core onto the
    exec-server-derived env
    
    ## Why
    Remote unified exec was materializing the shell env inside Core and
    forwarding the whole map to exec-server, so remote processes could
    inherit the orchestrator machine's `HOME`, `PATH`, etc. This keeps the
    base env on the executor while preserving Core-owned runtime additions
    like `CODEX_THREAD_ID`, unified-exec defaults, network proxy env, and
    sandbox marker env.
    
    ## Validation
    - `just fmt`
    - `git diff --check`
    - `cargo test -p codex-exec-server --lib`
    - `cargo test -p codex-core --lib unified_exec::process_manager::tests`
    - `cargo test -p codex-core --lib exec_env::tests`
    - `cargo test -p codex-core --lib exec_env_tests` (compile-only; filter
    matched 0 tests)
    - `cargo test -p codex-config --lib shell_environment` (compile-only;
    filter matched 0 tests)
    - `just bazel-lock-update`
    
    ## Known local validation issue
    - `just bazel-lock-check` is not runnable in this checkout: it invokes
    `./scripts/check-module-bazel-lock.sh`, which is missing.
    
    ---------
    
    Co-authored-by: Codex <noreply@openai.com>
    Co-authored-by: pakrym-oai <pakrym@openai.com>
  • fix: preserve platform-specific core shell env vars (#16707)
    ## Why
    
    We were seeing failures in the following tests as part of trying to get
    all the tests running under Bazel on Windows in CI
    (https://github.com/openai/codex/pull/16528):
    
    ```
    suite::shell_command::unicode_output::with_login
    suite::shell_command::unicode_output::without_login
    ```
    
    Certainly `PATHEXT` should have been included in the extra `CORE_VARS`
    list, so we fix that up here, but also take things a step further for
    now by forcibly ensuring it is set on Windows in the return value of
    `create_env()`. Once we get the Windows Bazel build working reliably
    (i.e., after #16528 is merged), we should come back to this and confirm
    we can remove the special case in `create_env()`.
    
    ## What
    
    - Split core env inheritance into `COMMON_CORE_VARS` plus
    platform-specific allowlists for Windows and Unix in
    [`exec_env.rs`](https://github.com/openai/codex/blob/1b55c88fbf585b32cd553cb9d02ec817f2ad6ebc/codex-rs/core/src/exec_env.rs#L45-L81).
    - Preserve `PATHEXT`, `USERNAME`, and `USERPROFILE` on Windows, and
    `HOME` / locale vars on Unix.
    - Backfill a default `PATHEXT` in `create_env()` on Windows if the
    parent env does not provide one, so child process launch still works in
    stripped-down Bazel environments.
    - Extend the Windows exec-env test to assert mixed-case `PathExt`
    survives case-insensitive core filtering, and document why the
    shell-command Unicode test goes through a child process.
    
    ## Verification
    
    - `cargo test -p codex-core exec_env::tests`
  • [codex] Remove codex-core config type shim (#16529)
    ## Why
    
    This finishes the config-type move out of `codex-core` by removing the
    temporary compatibility shim in `codex_core::config::types`. Callers now
    depend on `codex-config` directly, which keeps these config model types
    owned by the config crate instead of re-expanding `codex-core` as a
    transitive API surface.
    
    ## What Changed
    
    - Removed the `codex-rs/core/src/config/types.rs` re-export shim and the
    `core::config::ApprovalsReviewer` re-export.
    - Updated `codex-core`, `codex-cli`, `codex-tui`, `codex-app-server`,
    `codex-mcp-server`, and `codex-linux-sandbox` call sites to import
    `codex_config::types` directly.
    - Added explicit `codex-config` dependencies to downstream crates that
    previously relied on the `codex-core` re-export.
    - Regenerated `codex-rs/core/config.schema.json` after updating the
    config docs path reference.
  • fix: move inline codex-rs/core unit tests into sibling files (#14444)
    ## Why
    PR #13783 moved the `codex.rs` unit tests into `codex_tests.rs`. This
    applies the same extraction pattern across the rest of `codex-rs/core`
    so the production modules stay focused on runtime code instead of large
    inline test blocks.
    
    Keeping the tests in sibling files also makes follow-up edits easier to
    review because product changes no longer have to share a file with
    hundreds or thousands of lines of test scaffolding.
    
    ## What changed
    - replaced each inline `mod tests { ... }` in `codex-rs/core/src/**`
    with a path-based module declaration
    - moved each extracted unit test module into a sibling `*_tests.rs`
    file, using `mod_tests.rs` for `mod.rs` modules
    - preserved the existing `cfg(...)` guards and module-local structure so
    the refactor remains structural rather than behavioral
    
    ## Testing
    - `cargo test -p codex-core --lib` (`1653 passed; 0 failed; 5 ignored`)
    - `just fix -p codex-core`
    - `cargo fmt --check`
    - `cargo shear`
  • Inject CODEX_THREAD_ID into the terminal environment (#10096)
    Inject CODEX_THREAD_ID (when applicable) into the terminal environment
    so that the agent (and skills) can refer to the current thread / session
    ID.
    
    Discussion:
    https://openai.slack.com/archives/C095U48JNL9/p1769542492067109
  • fix: preserve core env vars on Windows (#8897)
    This updates core shell environment policy handling to match Windows
    case-insensitive variable names and adds a Windows-only regression test,
    so Path/TEMP are no longer dropped when inherit=core.
  • Added allow-expect-in-tests / allow-unwrap-in-tests (#2328)
    This PR:
    * Added the clippy.toml to configure allowable expect / unwrap usage in
    tests
    * Removed as many expect/allow lines as possible from tests
    * moved a bunch of allows to expects where possible
    
    Note: in integration tests, non `#[test]` helper functions are not
    covered by this so we had to leave a few lingering `expect(expect_used`
    checks around
  • feat: introduce support for shell_environment_policy in config.toml (#1061)
    To date, when handling `shell` and `local_shell` tool calls, we were
    spawning new processes using the environment inherited from the Codex
    process itself. This means that the sensitive `OPENAI_API_KEY` that
    Codex needs to talk to OpenAI models was made available to everything
    run by `shell` and `local_shell`. While there are cases where that might
    be useful, it does not seem like a good default.
    
    This PR introduces a complex `shell_environment_policy` config option to
    control the `env` used with these tool calls. It is inevitably a bit
    complex so that it is possible to override individual components of the
    policy so without having to restate the entire thing.
    
    Details are in the updated `README.md` in this PR, but here is the
    relevant bit that explains the individual fields of
    `shell_environment_policy`:
    
    | Field | Type | Default | Description |
    | ------------------------- | -------------------------- | ------- |
    -----------------------------------------------------------------------------------------------------------------------------------------------
    |
    | `inherit` | string | `core` | Starting template for the
    environment:<br>`core` (`HOME`, `PATH`, `USER`, …), `all` (clone full
    parent env), or `none` (start empty). |
    | `ignore_default_excludes` | boolean | `false` | When `false`, Codex
    removes any var whose **name** contains `KEY`, `SECRET`, or `TOKEN`
    (case-insensitive) before other rules run. |
    | `exclude` | array&lt;string&gt; | `[]` | Case-insensitive glob
    patterns to drop after the default filter.<br>Examples: `"AWS_*"`,
    `"AZURE_*"`. |
    | `set` | table&lt;string,string&gt; | `{}` | Explicit key/value
    overrides or additions – always win over inherited values. |
    | `include_only` | array&lt;string&gt; | `[]` | If non-empty, a
    whitelist of patterns; only variables that match _one_ pattern survive
    the final step. (Generally used with `inherit = "all"`.) |
    
    
    In particular, note that the default is `inherit = "core"`, so:
    
    * if you have extra env variables that you want to inherit from the
    parent process, use `inherit = "all"` and then specify `include_only`
    * if you have extra env variables where you want to hardcode the values,
    the default `inherit = "core"` will work fine, but then you need to
    specify `set`
    
    This configuration is not battle-tested, so we will probably still have
    to play with it a bit. `core/src/exec_env.rs` has the critical business
    logic as well as unit tests.
    
    Though if nothing else, previous to this change:
    
    ```
    $ cargo run --bin codex -- debug seatbelt -- printenv OPENAI_API_KEY
    # ...prints OPENAI_API_KEY...
    ```
    
    But after this change it does not print anything (as desired).
    
    One final thing to call out about this PR is that the
    `configure_command!` macro we use in `core/src/exec.rs` has to do some
    complex logic with respect to how it builds up the `env` for the process
    being spawned under Landlock/seccomp. Specifically, doing
    `cmd.env_clear()` followed by `cmd.envs(&$env_map)` (which is arguably
    the most intuitive way to do it) caused the Landlock unit tests to fail
    because the processes spawned by the unit tests started failing in
    unexpected ways! If we forgo `env_clear()` in favor of updating env vars
    one at a time, the tests still pass. The comment in the code talks about
    this a bit, and while I would like to investigate this more, I need to
    move on for the moment, but I do plan to come back to it to fully
    understand what is going on. For example, this suggests that we might
    not be able to spawn a C program that calls `env_clear()`, which would
    be...weird. We may still have to fiddle with our Landlock config if that
    is the case.