Repository navigation
fix(codex): forward ${VAR} stdio env values with env_vars instead of writing them - #3406
Abhishek Acharya (abhishekacharya200) wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved issues let transitive dependencies request credentials outside the modeled env field and prevent upgrading or removing legacy managed entries, leaving plaintext secrets in place.
3 open issues
What changed in this PR
This PR addresses #3393 by using Codex’s runtime env_vars field for same-name environment references in self-defined stdio MCP servers, rather than writing resolved values into config.toml.
Changes:
- Separates forwarded references from values that retain install-time resolution, while preserving argument substitution.
- Adds forwarding and TOML-output tests and updates install-flow expectations for Codex.
- Records the behavior change in the changelog; registry-backed servers remain unchanged.
| File | Description |
|---|---|
tests/unit/test_codex_adapter_phase3.py |
Adds coverage for forwarding, literal values, argument resolution, and written TOML. |
tests/unit/install/test_mcp_integrator_install_flow.py |
Expects Codex runtime forwarding while preserving Claude’s existing behavior. |
src/apm_cli/adapters/client/codex.py |
Splits stdio environment entries and emits forwarded names through env_vars. |
CHANGELOG.md |
Documents the new Codex stdio environment behavior. |
Review effort: Balanced
| if forwarded_env: | ||
| stdio_config["env_vars"] = forwarded_env | ||
| stdio_config["env"] = written_env |
There was a problem hiding this comment.
Fixed in 95c6d93. A new adapter hook, matches_legacy_managed_server_config, lets the self defined branch of compare_native_mcp_connection treat an entry from the older formatter as an update. It only accepts a value that is the literal ${VAR} or the current environment value, since that is all the old formatter could write. Anything else stays a conflict, so manual edits are kept. One limit: if the variable changed after the old write, the entry also reports a conflict, which is the safe side. Tests are in test_mcp_ownership.py.
| if forwarded_env: | ||
| stdio_config["env_vars"] = forwarded_env | ||
| stdio_config["env"] = written_env | ||
| self._merge_extra(stdio_config, server_info) |
There was a problem hiding this comment.
Fixed in 95c6d93. env_vars is now in _HARNESS_EXTRA_ALIASES, the literal in the architecture guard is updated, and a new test checks that extra: {env_vars: [...]} does not reach the rendered config.
| forwarded_env: list[str] = [] | ||
| for name, raw_value in raw_env.items(): | ||
| if isinstance(raw_value, str): | ||
| reference = _ENV_VAR_RE.fullmatch(raw_value.strip()) |
There was a problem hiding this comment.
Fixed in 95c6d93. The match now runs on raw_value directly, so a value with surrounding whitespace or a trailing newline keeps install time resolution. Tests cover both cases.
A self-defined stdio dependency that declared a secret as
`FIGMA_API_KEY: ${FIGMA_API_KEY}` had the value resolved at install time and
written to `.codex/config.toml`, which sits in the repository. When the
variable was unset, the literal `${FIGMA_API_KEY}` was written instead and
handed to the server as text.
Codex can read a stdio server's variables from its own environment through
`env_vars`, so a value authored as exactly `${VAR}` under the key `VAR` is now
forwarded instead of resolved. Every other value, including a reference under
a different key, keeps the existing install-time resolution.
Args keep resolving against the forwarded names when the value is available,
so an argument that references the variable is unchanged.
e949407 to
9acc197
Compare
…tter
env_vars now represents the modeled env field, so passthrough can no
longer request launch environment variables. Only an exact ${VAR} value
is forwarded, so authored whitespace keeps install time resolution. A
stdio entry the older formatter wrote counts as managed when its value
is the literal reference or the current environment value; anything
else stays a conflict, so manual edits are preserved.
Fixes #3393
Problem
A self-defined stdio dependency that declares a secret as
has the value resolved at install time and written into
.codex/config.toml, a file that lives in the repository. If the variable is unset during install, the literal${FIGMA_API_KEY}is written instead and Codex hands that string to the server. Codex can already read a stdio server's variables from its own environment throughenv_vars, the waybearer_token_env_varis used for remote headers since #3042.Change
_format_server_confignow splits a self-defined stdioenvblock:${VAR}(also${env:VAR}) under the keyVARis forwarded asenv_vars = ["VAR"]and is not resolved, prompted for, or written toconfig.toml.[mcp_servers.<name>.env], including a reference under a different key such asTOKEN: ${FIGMA_API_KEY}.<FIGMA_API_KEY>inargsbehaves as before.env_varsis emitted before the[env]sub-table so tomlkit cannot nest it.Validation
tests/unit/test_codex_adapter_phase3.pycover forwarding (set and unset variable,${env:VAR}spelling), literal values, the mismatched-key case, arg resolution, and the written TOML. Five of the seven fail onmainand all pass with the change.test_self_defined_stdio_env_placeholders_resolve_from_process_envasserted the old contract for Codex, so it now assertsenv_varsplus the absence of the secret for Codex and is unchanged for Claude.tests/unit/test_codex_adapter_phase3.py+tests/unit/install/test_mcp_integrator_install_flow.py: 113 passed.tests/unit: failure set identical tomainexcept for the new tests, i.e. no regressions. (25 pre-existing failures in this Windows checkout, from files outside the sparse clone.)ruff checkandruff format --checkclean on all four touched files.Scope
Only the self-defined stdio route is covered. Registry packages take
environment_variablesfrom the registry schema and resolve through a separate path, which this PR leaves untouched.