-
Notifications
You must be signed in to change notification settings - Fork 7.2k
Configurable default login shell for shell tools #8938
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
There was a problem hiding this comment.
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: 1924aee486
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@pikkili, thanks for the contribution. I'm not sure this is the right approach to fix this problem, but I will discuss with my colleagues. In the meantime, please fix the CI failures. |
|
Thanks for the review and for flagging the approach. I pushed follow‑up commits to address the CI failures and will keep an eye on the checks. |
|
@pikkili, there are some new merge conflicts. Could you please resolve those? Thanks! |
|
Resolved the conflict in codex-rs/core/src/config/mod.rs, rebased on origin/main, and pushed the updated branch. Fast checks re‑run locally. |
|
Thanks for the contribution. I've discussed with my colleagues, and we've decided to pass on this PR. We don't think this is the right long-term solution for this problem. |
|
Thanks @etraut-openai for the review and for discussing internally. We’d appreciate guidance on the preferred long‑term approach. At Oracle we use ADE (our internal VCS; toolchain paths are set when entering an ADE view), but "bash -lc" reinitializes PATH and Codex can no longer see the ADE toolchain, so it can’t run builds/tests to verify its own changes. As a result we frequently see non‑compilable changes that require developer intervention to validate, and this also blocks using Codex reliably in our automation workflows. We tried a workaround by adding explicit instructions in AGENTS.md to always use non‑login shells, but that isn’t reliable in long or complex sessions. We want to align with Codex’s intended direction. Thank you for your time. |
|
We haven't settled on a concrete direction here. We've had reports of numerous issues relating to login shells, and many of the solutions we've explored have bad side effects (they fix some issues but break other use cases). We're exploring whether there's a more general solution that can be baked into the model intelligence. In the meantime, we don't want to paint ourselves into a corner by exposing hard-coded configuration knobs like the one you've proposed in this PR. Thanks for explaining your use case. That's helpful. Here are some potential ideas for an intermediate workaround.
I don't know enough about your particular configuration or workflow to say which of these might be viable or palatable, but I'm hoping that there's an idea here that you can use. |
|
Thank you, @etraut-openai. If you have (or plan to create) an issue tracker, please keep us in the loop so we can track this internally as well. At this point, we have explored several workarounds, but none are particularly compelling:
Summary: As a related note, we also ran into additional issues when building from within Codex due to process hardening introduced in newer versions. Codex now sanitizes loader-related environment variables (LD_) at process startup as a defense-in-depth measure. This removes LD_LIBRARY_PATH before any build tools execute. In contained environments like ours, we have historically relied on libraries provided via LD_ paths, so this change is particularly impactful. |
|
Thanks for the details. I have some good news for you about the environment variable ( |
|
Thank you @etraut-openai — if I may ask one more question: In production automation flows (diagnose → fix → build/test → submit PR → respond to review → merge), what split do you recommend between: Internally we’ve been debating simplicity (one Codex does everything) vs determinism/cost/reliability (orchestrator owns execution). Curious what your team’s recommended best practice is. |
Summary
Adds [shell].default_login to control the default login‑shell behavior for shell, local_shell, and shell_command when a tool call does not
specify login. Explicit login=true|false in the tool call always overrides the config.
Motivation
In curated environments, PATH and other env vars are intentionally inherited. The current default login shell can reinitialize PATH and
override the inherited environment. A config‑level default lets users preserve the inherited environment when needed while keeping existing
behavior unchanged if the option is absent.
Behavior
uses default_login.
Docs (external)
In‑repo docs are link‑only (per #8662). Please update external docs:
Config reference (https://developers.openai.com/codex/config-reference)
[shell]
default_login = true
default_login (boolean, default true): When true, shell commands run with login semantics (e.g., bash -lc). When false, Codex avoids login
shells by default. Explicit login values on a tool call still override this default.
Config sample (https://developers.openai.com/codex/config-sample)
[shell]
Default when tool calls omit
login. Default: truedefault_login = true
Tests
Issue