Skip to content

fix(codex): launch-arg MCP servers survive the thread's t3-code server - #15839

Open
lnieuwenhuis wants to merge 3 commits into
pingdotgg:mainfrom
lnieuwenhuis:fix/codex-keep-launch-mcp-servers
Open

lnieuwenhuis wants to merge 3 commits into
pingdotgg:mainfrom
lnieuwenhuis:fix/codex-keep-launch-mcp-servers

Conversation

@lnieuwenhuis

Copy link
Copy Markdown
Contributor

Fixes #15828

Codex thread config sent mcp_servers: { "t3-code": ... }. Codex applies per-thread config in the same layer as the process's -c flags, so that single-segment key replaced the whole mcp_servers table, and servers added through instance launch args (-c mcp_servers.extra.command=...) never started.

The thread config now uses the dotted key "mcp_servers.t3-code", the same way CODEX_THREAD_CONFIG already sets tools.update_plan.enabled. It's built in one place, codexThreadRuntimeParams, which covers thread/start, thread/resume and thread/fork. The replay matcher in effect-codex-app-server drops mcp_servers.* keys as well as the whole-table key, so recordings keep ignoring the per-machine URL and auth header.

Verification

  • Probed a real codex app-server (codex-cli 0.160.0, isolated CODEX_HOME) launched with -c mcp_servers.extra.command="cat", then sent thread/start with each config shape and collected mcpServer/startupStatus/updated notifications:
    • whole table (mcp_servers: { "t3-code": ... }): only t3-code starts
    • dotted key ("mcp_servers.t3-code": ...): both t3-code and extra start
  • vp test run on CodexAdapterV2.test.ts (asserts the dotted key and that no mcp_servers table is sent), CodexReplayFixtures.integration.test.ts, and the ThreadFork / OrchestratorReplayRecovery / ThreadMergeBack integration tests: all pass.
  • effect-codex-app-server typecheck clean. t3 typecheck is clean apart from the duplicate Option import on main, fixed separately in fix(server): the server starts again after a duplicate Option import #15837.

Model/harness: Claude Opus 5.5 / Claude Code.

Thread config is applied in the same layer as Codex -c launch flags, so a
whole mcp_servers table replaced servers added via -c mcp_servers.<name>.*.
Send the t3-code server under the dotted key mcp_servers.t3-code instead,
and keep replay matching ignoring host-supplied MCP server config keys.

Fixes pingdotgg#15828
@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:S 10-29 changed lines (additions + deletions). labels Oct 5, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 157d583

Macroscope's review found this PR approvable — This is a small, localized Codex bug fix that preserves launch-argument MCP servers while still injecting the T3 server, with no schema, deployment, product-default, or static-analysis changes. The replay adjustment and updated unit expectation are correspondingly narrow.

No code changes detected at 39dd5d4. Prior analysis still applies.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Oct 5, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

⚙️ Run configuration
  • Configuration used: Repository: pingdotgg/t3code/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 96a9dfe3-5726-4a10-946b-6072848741ad
📥 Commits

Reviewing files that changed from the base of the PR and between 9f2fc88 and 39dd5d4.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: pingdotgg/t3code/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 0e7011a5-45dd-46c8-abc9-5ea304e2c616
📥 Commits

Reviewing files that changed from the base of the PR and between ad5178a and 157d583.

📒 Files selected for processing (3)
  • apps/server/src/orchestration-v2/Adapters/CodexAdapterV2.test.ts
  • apps/server/src/orchestration-v2/Adapters/CodexAdapterV2.ts
  • packages/effect-codex-app-server/src/replay.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.


📝 Walkthrough

Walkthrough

The Codex thread runtime configuration now uses a dotted key for the t3-code MCP server. Thread request normalization excludes mcp_servers and keys prefixed with mcp_servers.. The adapter test fixture uses the dotted key.

Changes

Codex MCP configuration override

Layer / File(s) Summary
Configure and normalize thread MCP servers
apps/server/src/orchestration-v2/Adapters/CodexAdapterV2.ts, packages/effect-codex-app-server/src/replay.ts, apps/server/src/orchestration-v2/Adapters/CodexAdapterV2.test.ts
The thread runtime config sets t3-code under mcp_servers.t3-code, preserving its URL and Authorization header. Request normalization filters mcp_servers and all mcp_servers.* keys. The test fixture uses the dotted key.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~8 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to 157d5

The configuration change is mergeable; no actionable current-head risk was established.

Architecture Summary

Architecture risk: 🔵 Low · up to 157d5

The change affects 2 systems.

Changed systems: apps/server, packages/effect-codex-app-server

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

  • observed — apps/server (service) was modified; 2 changed files map to changed impact.
  • observed — packages/effect-codex-app-server (library) was modified; 1 changed file maps to changed impact.

Before / after behavior

  • observed — Modified behavior in apps/server/src/orchestration-v2/Adapters/CodexAdapterV2.test.ts: The test fixture changes the MCP server config from a nested mcp_servers object to the flattened mcp_servers.t3-code key, preserving the endpoint and Authorization header.
  • observed — Modified behavior in apps/server/src/orchestration-v2/Adapters/CodexAdapterV2.ts: The MCP server configuration changes from a nested mcp_servers table to the dotted mcp_servers.t3-code key; its URL and authorization header remain the same.
  • observed — Modified behavior in packages/effect-codex-app-server/src/replay.ts: Thread request normalization now filters out mcp_servers and all mcp_servers.* config keys; previously it removed only the exact mcp_servers key.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 3 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title identifies the main change: preserving MCP servers configured through launch arguments when the thread adds the t3-code server.
Description check ✅ Passed The description explains the problem, the fix, and the verification results. It links issue #15828, but does not explicitly state maintainer approval or explain why the fix qualifies as a small, obvio…
Linked Issues check ✅ Passed Issue #15828 requires Codex threads to retain MCP servers configured with instance -c flags while adding T3’s t3-code server. codexThreadRuntimeParams now sends mcp_servers.t3-code instead of …
Out of Scope Changes check ✅ Passed All changes relate to issue #15828. The replay matcher now ignores both whole-table and dotted mcp_servers config keys, so per-machine MCP settings do not cause replay request mismatches. The adapte…
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S 10-29 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Codex per-thread mcp_servers override replaces MCP servers added with -c launch args

1 participant