Repository navigation
🤖 fix: clearer Codex MCP auth warnings and Windows-safe env names for plugin servers - #6690
Conversation
… on stdio servers, compare env names without case on Windows
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2454b9b2db
ℹ️ 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".
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
…der#6804) Closes coder#6713 This PR does the three follow-ups from the coder#6690 readiness review (plugin env checks). 1. **A. `=` in `env` keys:** `normalizeStdioEntry` (`mcpConfig.ts`) now refuses an `env` key that is empty or contains `=`, as `env_vars` names already do. A key such as `PLUGIN_DATA=/x` would set `PLUGIN_DATA` in the joined env block. On Windows libuv sorts that block, so Xum writing the reserved values last does not protect them. The `env` checks moved, unchanged otherwise, into `stdioEnvError`, so that `normalizeStdioEntry` does not grow (CRAP gate). 2. **B. Case on Windows:** `withForwardedHostEnv` (`mcpServerManager.ts`) drops a forwarded `env_vars` name whose `envNameKey` matches an `env` key. On Windows, `TOKEN` and `token` are one variable, so before this change both keys reached the env block. Now the `env` value wins, as the docs say. Elsewhere names stay case-sensitive. 3. **C. Loader list:** `LOADER_ENV_NAME` (`openaiMcpAuth.ts`) adds `BASH_ENV` (stdio servers start through `bash -c`, which runs the file it names), plus `LD_AUDIT`, `NODE_PATH` and `PYTHONPATH`. `ENV` is not added: only interactive shells read it, and `bash -c` is not interactive. The "Plugin server environment" section of `docs/config/mcp-servers.mdx` now says that the review warns about these names and that Windows compares them without case. ## Validation - Tests first. Both new tests failed before the fix: an `=` key loaded and the new loader names had no warning (`mcpConfig.test.ts`), and on win32 both case variants reached the env (`mcpServerManager.test.ts`). - `mcpConfig`, `openaiMcpAuth` and `mcpServerManager` suites: 443 pass. `make typecheck`, `make static-check`, `MUX_ESLINT_CONCURRENCY=1 make lint-eslint` and `make crap-check` pass. ## Risks Low. A plugin whose `env` has a key with `=` no longer loads that server. Such a key could not set a usable variable anyway. --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-opus-5-5` • Thinking: `high`_
1 participant
Summary
Four follow-ups from the #6487 reviews for Codex
.mcp.jsonplugin servers. Closes #6646.Changes
openaiMcpAuth.ts, aheaderNamethat is not a valid header name, or isHost, now gets its own warning ("must be a valid HTTP header name other than Host"). Before, it got "needs headerScheme custom_header", even when the scheme was custom_header. Acustom_headerscheme without a valid name is now dropped as well, not only when the name is missing. Nothing reads these fields yet, because Xum has no api_key support.oauth,scopesoroauth_resourcenow gets a review warning that names them, with or withoutenv_vars. Before, the fields were dropped silently. For this, the loader now reports translation warnings for stdio servers too. The remote OAuth handling moved, unchanged, intoremoteOAuthMessages.env_varsnames that change how the server loads code (LD_PRELOAD,LD_LIBRARY_PATH,DYLD_*,NODE_OPTIONS) still load, but the review names them in a warning (defense in depth).env_varsand forenvkeys now ignores case there, soplugin_datacannot shadowPLUGIN_DATA. Duplicateenv_varsnames that differ only in case are merged, keeping the first spelling. Other platforms are unchanged.Validation
New tests failed on main and pass now:
openaiMcpAuth.test.ts);mcpConfig.test.ts);process.platformset towin32in the test, as other tests in the repo do. The same test checks that Linux keeps case-sensitive names.Risks
Low. Items 1 to 3 only add or reword warnings. Item 4 refuses a server on Windows only when it names a case variant of a reserved variable.
Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high• Cost:$91.52