fix(mcp): write env-sourced MCP headers into Codex's runtime fields

[Eden (edenfunf)]

What

A header whose value references an environment variable now reaches Codex in a form Codex resolves at server start, instead of being copied into http_headers as an unexpanded placeholder.

Given headers: { Authorization: "Bearer ${TOKEN}", X-Api-Key: "${API_KEY}", X-Mixed: "prefix-${SOME_VAR}-suffix", X-Static: "literal-value" }, ~/.codex/config.toml goes from

[mcp_servers.bridge]
url = "http://127.0.0.1:4521/mcp"
id = ""

[mcp_servers.bridge.http_headers]
Authorization = "Bearer ${TOKEN}"
X-Api-Key = "${API_KEY}"
X-Mixed = "prefix-${SOME_VAR}-suffix"
X-Static = "literal-value"

to

[mcp_servers.bridge]
url = "http://127.0.0.1:4521/mcp"
id = ""
bearer_token_env_var = "TOKEN"

[mcp_servers.bridge.http_headers]
X-Static = "literal-value"

[mcp_servers.bridge.env_http_headers]
X-Api-Key = "API_KEY"

with one warning naming X-Mixed, which no Codex field can express.

Why

Codex documents http_headers as a map of header names to static values. CodexClientAdapter does not override _supports_runtime_env_substitution (base.py defaults it to False), so _resolve_variable_placeholders leaves ${VAR} untouched and the literal text is what the server receives. Claude Code's adapter writes the same placeholder into headers, where expansion is documented, so one manifest authenticated for one target and silently failed for the other -- apm install reported success for both.

How

Implements the scope recorded on #2984, in _format_server_config's remote branch:

• Authorization: "Bearer ${VAR}" -> bearer_token_env_var = "VAR".
• A value that is exactly ${VAR} -> env_http_headers = { "<Header>" = "VAR" }.
• Anything else still holding a placeholder -> skipped with a warning naming the headers, in the same "what happened, what to do" shape as the adapter's other skip warnings.
• A value with no placeholder -> unchanged, still http_headers, which is what that field means.

Both match regexes are built from base._ENV_VAR_RE, so the two spellings APM accepts (${VAR} and ${env:VAR}) cannot drift apart here; the Bearer scheme matches case-insensitively per RFC 7235. bearer_token_env_var is inserted before the dict-valued keys so tomlkit emits the scalar ahead of the sub-tables it renders.

Authorization: "${VAR}" (no scheme) deliberately routes to env_http_headers, not bearer_token_env_var -- the latter would make Codex send a Bearer prefix the manifest never declared.

One coupled change, flagged on the issue before writing it. http_headers sits on the extra: passthrough denylist (_HARNESS_EXTRA_ALIASES) as a harness alias of the modeled headers field -- the boundary from #1670 / PR #1765 that stops a transitive dependency smuggling a modeled field through passthrough. bearer_token_env_var and env_http_headers were not on it; I confirmed on main that both pass straight through into config.toml today. That is inert only while APM never writes them. This PR makes them APM-rendered aliases for headers, so leaving them off would open exactly the hole the denylist exists to close: an extra: block could reach an Authorization header that headers modeling never sees. They are added to the same frozenset, and docs/.../manifest-schema.md, which enumerates those reserved keys, is updated to match. Happy to split this out if you would rather review it separately.

_warn_input_variables coverage is unchanged: ${input:VAR} is disjoint from _ENV_VAR_RE by design, so those values still land in http_headers and still warn.

Test

Live verification -- real apm install -g -t codex against a self-defined remote dep, sandboxed HOME, on main before and this branch after:

Header value	Before	After
Authorization: Bearer ${TOKEN}	http_headers literal	bearer_token_env_var = "TOKEN"
X-Api-Key: ${API_KEY}	http_headers literal	env_http_headers
X-Env-Alt: ${env:ALT_VAR}	http_headers literal	env_http_headers
X-Mixed: prefix-${SOME_VAR}-suffix	http_headers literal, silent	skipped, one warning naming it
X-Static: literal-value	http_headers	http_headers (unchanged)

Output re-parsed with tomllib to confirm the scalar-before-sub-tables ordering is valid TOML. Installing the same manifest with -t claude,codex confirms Claude Code's .claude.json is byte-identical to before, so the parity gap closes without moving the other adapter.

Denylist, verified live: extra: { bearer_token_env_var: ATTACKER_VAR, env_http_headers: {...} } reaches config.toml on main; on this branch it is dropped with the existing reserved passthrough key(s) ignored warning.

Unit tests -- 6 new in TestCodexClientAdapter (tests/unit/test_mcp_client_factory.py) covering each routing decision including the two that must not happen (Authorization: ${VAR} staying out of bearer_token_env_var; Bearer ${VAR} on a non-Authorization header being skipped), plus a literal-value regression guard. 1 new in TestAdapterRealPathShadowGuard (tests/unit/test_mcp_from_dict_unknown_keys.py) for the passthrough boundary, sitting beside the existing http_headers one. All 7 fail on main and pass here.

Updated test -- test_format_server_config_remote_with_headers in tests/integration/test_wave2_adapters_coverage.py asserted "http_headers" in result for a fixture whose value is ${MY_TOKEN}, i.e. it pinned the buggy behavior. Its intent (headers reach the config) is preserved; it now asserts the field that actually works, with a docstring saying why.

Suites -- tests/unit + tests/test_console.py (22622 passed), tests/integration/test_wave2_adapters_coverage.py + test_core_smoke.py + tests/red_team/ (713 passed), CI quality ratchets (90 passed). ruff check src tests and ruff format --check clean. Three tests/unit failures (test_install_safety.py::TestAbsolutePathGuard::test_accepts_unix_absolute, TestSuffixGuard::test_accepts_lib_apm_suffix, test_view_command.py::test_view_versions_bare_registry_flag_forces_registry) reproduce identically on an unmodified main checkout and are unrelated.

Issue: #2984 (status/accepted, claimed in this comment).

[Copilot]

Copilot review overview

🟡 Changes recommended

One or more issues must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 · 1 · 1

Open (3)

• Update architecture guards for the expanded denylist · New
• Support or reject shell-default environment placeholders · New
• Update docs for runtime Codex header resolution · New

What changed in this PR

Fixes Codex MCP header rendering so environment-backed values use Codex runtime fields instead of static placeholders.

Changes:

• Maps bearer and exact environment placeholders to Codex fields.
• Rejects unsupported mixed placeholders and blocks passthrough injection.
• Updates tests, schema documentation, and changelog.

File	Description
tests/​unit/​test_mcp_from_dict_unknown_keys.py	Updated as part of this pull request.
tests/​unit/​test_mcp_client_factory.py	Updated as part of this pull request.
tests/​integration/​test_wave2_adapters_coverage.py	Updated as part of this pull request.
src/​apm_cli/​models/​dependency/​mcp.py	Updated as part of this pull request.
src/​apm_cli/​adapters/​client/​codex.py	Updated as part of this pull request.
docs/​src/​content/​docs/​reference/​manifest-schema.md	Updated as part of this pull request.
CHANGELOG.md	Updated as part of this pull request.

---

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Eden (edenfunf)]

Confirmed and fixed in ef4d1c4 -- tests/integration/test_architecture_owner_rule_mutations.py was not in the set I had been running, so I missed it. It fails three ways on the pushed revision: the guard's fixed needle, the pre-mutation clean assertion, and the mutation case itself.

FAILED ...[hooks-integrations-mcp-passthrough-denylist]: mutation_writes.mcp_passthrough_denylist already violates unmutated
FAILED ...test_owner_rules_report_nothing_before_mutation
FAILED ...test_owner_rule_catches_its_guard_mutation[hooks-integrations-mcp-passthrough-denylist]

The guard pins the literal verbatim via _has_fixed, and adding two aliases pushed it past the 100-char line limit, so ruff reflowed it to a block and the needle no longer matched.

Rather than weaken the guard to a per-alias substring check -- "id", is not unique in that file, so that would have made it laxer than it is today -- I kept it an exact pin and made the literal pinnable: the explanatory comment moves above the assignment, members are sorted, and the block is comment-free. A note in the source says why, so the next person adding an alias knows the guard is watching:

The literal below is pinned verbatim by the architecture guard mutation_writes.mcp_passthrough_denylist: keep it comment-free and sorted.

The mutation fixture drops one member line ( "environment",\n -> empty), which is unique in the file and keeps the recorded intent, "Shared MCP model stops denying the OpenCode environment alias."

tests/integration/test_architecture_owner_rule_mutations.py now passes in full (353), and both architecture suites together under --dist loadgroup pass (362).

[Eden (edenfunf)]

Correct, and this was the most important of the three -- thanks. Fixed in ef4d1c4.

Verified before changing anything: the issue's own reproduction value fell straight through to static http_headers with no warning, so the PR would not have fixed the manifest it was opened for.

issue repro ${VAR:-} -> {'url': 'https://mcp.context7.com/mcp', 'id': '', 'http_headers': {'Authorization': '${CONTEXT7_API_KEY:-}'}}

Of your two options I took the second. The recorded scope excludes changing _resolve_variable_placeholders semantics for adapters that do support runtime substitution, and teaching _ENV_VAR_RE the shell-default spelling would change what every one of them translates, so the report is widened in the Codex adapter instead:

_CODEX_UNSUPPORTED_REFERENCE_RE = re.compile(r"\$\{(?!input:)")

Any reference still standing after resolution is no more static than a ${VAR} Codex could resolve, so it is reported rather than written. ${input:...} is excluded by the lookahead: those are collected elsewhere and carry their own warning, and sweeping them into this path would have cost them their header. There is a test pinning that specifically.

Current routing, all eight shapes exercised through _format_server_config:

Value	Result
Bearer ${TOKEN} on Authorization	bearer_token_env_var = "TOKEN"
${API_KEY}	env_http_headers
${env:API_KEY}	env_http_headers
${TOKEN} on Authorization	env_http_headers (no undeclared Bearer prefix)
Bearer ${TOKEN} on another header	skipped + warning
a-${V}-b	skipped + warning
${CONTEXT7_API_KEY:-}	skipped + warning
Bearer ${TOKEN:-}	skipped + warning
${input:proj}	http_headers, existing input-variable warning
us-east-1	http_headers

The warning now names the shape too: "Literal text around the reference, or a shell-style default such as ${VAR:-}, has no Codex equivalent; export the whole header value as one variable." Confirmed end to end with a real apm install -g -t codex using the issue's context7 declaration.

[Eden (edenfunf)]

Agreed, and both surfaces are updated in ef4d1c4.

Worth recording what I found while checking it: section 4.2.4 was already inaccurate before this PR. It claims Codex resolves ${VAR} at install time, but that was only ever true of env. Remote headers were passed through verbatim -- which is exactly the bug this PR fixes. Both verified directly:

codex stdio env  -> {'env': {'TOKEN': 'resolved-at-install'}, ...}   # install-time, as documented
codex headers    -> {'http_headers': {'Authorization': 'Bearer ${TOKEN}'}}  # verbatim, not resolved

So the fix is to split the two rather than to restate the old claim. The table rows now read "env: resolved at install time from env (or interactive prompt). Codex headers: see note below", with a dedicated bullet:

Codex headers are the exception. Codex does read a remote server's headers from the environment, through dedicated fields rather than from inside http_headers, which it documents as static values. APM writes Authorization: "Bearer ${VAR}" as bearer_token_env_var and a value that is exactly ${VAR} as env_http_headers; Codex resolves both at server start, so no token is written to config.toml. A value Codex cannot express that way -- literal text around the reference, or a shell-style default such as ${VAR:-} -- is skipped with a warning instead of being written as static text.

The Codex/Gemini/Cursor bullet is narrowed to env rather than rewritten wholesale, because the install-time claim stays true for Gemini and Cursor headers -- they go through _resolve_env_variable, a different path from the one this PR touches. I did not want to restate their behaviour on the strength of a Codex change.

packages/apm-guide/.apm/skills/apm-usage/dependencies.md is updated in the same commit; its Codex: resolved at install time. line now distinguishes env from a remote server's headers.

The other doc hunk already in this PR (manifest-schema.md line 613, the reserved-passthrough-key list) is a separate surface and stays as it was.