fix(codexcli-mcp): strip empty env tables from generated config#1622
Merged
Conversation
The codex CLI rejects empty `[mcp_servers.X.env]` headers on remote
(sse/http/streamable_http) transports with:
"env is not supported for streamable_http"
When a rulesync source has `"env": {}` on a server (intentionally or
left over from another tool's shape), the codex generator passes it
through and smol-toml serialises it as an empty TOML table — which
breaks codex 0.130+ on startup.
Make `removeEmptyEntries` recursive so empty nested objects (like an
empty `env`) are stripped at any depth. Arrays are preserved verbatim
since an empty array can be a meaningful explicit override.
This also cleans up codex output for stdio servers (no more empty
`[mcp_servers.X.env]` headers when no env vars are needed) — codex
tolerates them there, but they were always functionally inert.
Tests cover:
- empty `env` on sse server is stripped, file content has no `.env]` header
- empty `env` on stdio server is stripped
- populated `env` table is preserved
Self-review polish before marking dyoshikawa#1622 ready: 1. Parameterize the remote-transport test over all three transports (sse, http, streamable_http) that codex CLI rejects empty env on. Previously only `sse` was tested, leaving `http` and `streamable_http` susceptible to silent regression if the strip logic is ever made transport-aware. 2. Remove the brittle `expect(...).not.toContain('.env]')` file-content assertion. The structural `expect(server?.env).toBeUndefined()` check is sufficient and is not vulnerable to false-pass on unrelated server names containing `.env` (or false-fail on benign formatting changes). 3. Add defensive assertions that non-env fields (`type`, `url`, `command`, `args`) survive the strip in all three test cases. Guards against a future over-aggressive strip that accidentally drops other server fields. No functional change to the source code.
Owner
|
@sirmacik Thank you! |
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The codex generator currently emits an empty
[mcp_servers.X.env]header for any server whoseenvis{}. Codex CLI 0.130+ rejects this on remote (sse/http/streamable_http) transports with:This is a hard startup failure for any source
mcp.jsonthat has a remote MCP server with an emptyenv(intentionally, or left over from another tool's schema shape).Repro
After
rulesync generate -g:Fix
Make
removeEmptyEntriesincodexcli-mcp.tsrecursive so empty nested objects (env: {}) are stripped at any depth, not just at the top level. Arrays are preserved verbatim because an empty array can be a meaningful explicit override.Side effect: stdio servers also get cleaner output (no more inert empty
[mcp_servers.X.env]headers). Codex tolerates those, so this is purely cosmetic for stdio.Tests added
should strip empty env object from remote (sse) server— repro testshould strip empty env object from stdio server (clean output)should preserve populated env table on stdio serverAll 54 codex MCP tests pass.
pnpm cicheckis clean.Risk
Minimal — recursing into nested objects is a generalization of the existing behaviour (which already strips empty top-level server configs). No existing test relied on empty
env: {}being preserved inside a server config.