Sign up
View stargazersSign up

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

merged7
main
GitHub fork: fix/codex-keep-launch-mcp-servers
opened23d agobyLlnieuwenhuis
3 files changed
View on GitHubTwo-way sync enabledPR last imported 7d ago

Fixes #15828.

Codex thread config previously sent a whole mcp_servers table containing only T3's server. Thread overrides share the process launch-argument override layer, so this replaced MCP servers supplied through instance launch arguments.

Use the dotted mcp_servers.t3-code key at the shared adapter boundary. Thread start, resume, fork, and rollback's resume retain launch-argument servers while configuring T3's session endpoint and authorization. Main's 10-second startup grace and 3,900-second T3 tool timeout are preserved. Replay matching ignores both historical whole-table MCP config and dotted mcp_servers.* keys, preserving comparisons for unrelated thread config.

Maintainer takeover of @lnieuwenhuis's implementation in #15839, preserving contributor commits and merging main. This remains one adapter/replay concern on the original PR. The shared server path covers web, desktop, mobile, remote connections, and scheduled turns without client or contract changes.

Verification on headless Linux:

  • Replaced one static helper expectation with two behavior regressions in the existing adapter test file: an actual adapter thread-start request with the dotted key, and replay of historical/dotted recordings with different endpoint/auth/extra-server settings. The replay test also confirms unrelated config differences still fail. Both tests fail against main and pass after restoring the fix. Test cases in the selected suites: 171 → 172; changed test files: 1 → 1.
  • 172 adapter/fixture tests and 24 actual Codex orchestrator replay cases pass. Scoped lint and formatting, server and app-server package typechecks, and whitespace checks pass. CI identified a helper export left unused by the test rewrite; it is now private, and a server-only Knip export check passes. Lint reports two existing unused-variable warnings outside the changed region.
  • Installed Codex 0.163.0 app-server with isolated temporary homes and local HTTP/stdio MCP fixtures: the baseline whole-table request starts only t3-code; the dotted request generated by the production adapter helper starts both t3-code and the launch-argument server extra. Both reach ready. All other generated thread settings are identical. The fixture loader exposes the private helper without changing its body. No account credentials or model inference were used.
  • Two independent full-diff review rounds with Claude Opus 5.5 and GPT-6 Sol found no code blockers.

Screenshots do not apply to this protocol/configuration change. Live verification covers thread start; resume/fork/rollback share the inspected helper and are covered by recorded orchestration flows, rather than separate live account runs. macOS, Windows, iOS, Android, and Electron desktop were not exercised. The author's original Codex 0.160 experiment was not rerun. No real account session or user MCP server was used.

Unmerged. Current-head CI passes, including the repaired Knip check. Macroscope requests human approval of enabling launch-argument MCP servers alongside T3’s server.

Implementation: GPT-6.1 Sol in Codex. Tests: GPT-6 Astra in Codex. Reviews: Claude Opus 5.5 in Claude Code and GPT-6 Sol in Codex.

Checks

21 successful · 11 skipped
All checks passed
3c34a97

Added 1 commit

  • fix(codex): launch-arg MCP servers survive the thread's t3-code server157d583
MmacroscopeappBot23d ago
Synced from GitHub

Approvability

Verdict: Needs Human Approval

Macroscope's review found this PR requires human approval — This focused Codex MCP fix changes configuration precedence across thread start, resume, fork, and rollback, allowing previously suppressed launch-argument MCP servers to start alongside T3’s server. The scope and tests are strong, but the activated runtime work is substantial enough to merit human review.

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

CcoderabbitaiBot23d ago
Synced from GitHub

Review in Change Stack →

Warning

Review limit reached

Only developers with an assigned seat can use this organization's usage-based review budget, and seats here are assigned manually. Ask an admin to assign a seat, or change the review continuation mode in Billing.

Next included review available in 4 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: efc26623-681a-455f-b29c-7e597ceac9f2
📥 Commits

Reviewing files that changed from the base of the PR and between 8cb87d61bf89ea4529e5fdce36084857a71863c3 and 3c34a9750e0dc01e3030ada5136bc8b2eece7d43.

📒 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

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: fa8513b4-e904-4267-b770-b665c2c428be
📥 Commits

Reviewing files that changed from the base of the PR and between 39dd5d4d808aae13c6d6a1c57a2ce45174d6cb95 and 8cb87d61bf89ea4529e5fdce36084857a71863c3.

📒 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; 1 remain after this review.

📝 Walkthrough

The Codex thread configuration now sets the T3 MCP server with a dotted key. Replay normalization excludes nested and dotted MCP server configuration keys. Adapter tests cover session MCP configuration and replay matching.

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, timeout, and authorization header. Replay normalization filters mcp_servers and keys prefixed by mcp_servers.. Tests check the session MCP values in thread/start parameters and confirm replay accepts nested or dotted MCP entries while still detecting a difference in other configuration.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to 8cb87

The change has no identified merge-blocking issue after normal checks. Resume, fork, and rollback were not separately live-tested, but no failure on those paths is established.

### Architecture Summary

Architecture risk: 🔵 Low · up to 8cb87

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: Removed the test asserting thread-scoped parameters contained the cwd, model, and MCP server authorization configuration.
  • observed — Modified behavior in apps/server/src/orchestration-v2/Adapters/CodexAdapterV2.test.ts: Added a replay test that sets a session MCP server and checks that thread/start parameters include its URL, authorization header, and timeout alongside the existing configuration.
  • observed — Modified behavior in apps/server/src/orchestration-v2/Adapters/CodexAdapterV2.test.ts: Added replay assertions for both nested and dotted historical MCP configuration keys. The replay accepts those MCP entries when other configuration matches, and reports a frame mismatch when tools.update_plan.enabled differs.
  • observed — Modified behavior in apps/server/src/orchestration-v2/Adapters/CodexAdapterV2.ts: The T3 MCP server configuration now uses the mcp_servers.t3-code key instead of a mcp_servers object containing t3-code; its URL, timeout, and authorization header are unchanged.
Pre-merge checks: Passed 4
Check nameStatusExplanation
Title checkPassedThe title clearly describes the Codex MCP preservation fix and uses a concise conventional-commit format.
Linked Issues checkPassedIssue #15828 requires Codex provider threads to keep instance launch-argument MCP servers and add T3's t3-code server. codexThreadRuntimeParams now sends only mcp_servers.t3-code with the existi…
Out of Scope Changes checkPassedThe changed replay normalizer removes historical whole-table mcp_servers and dotted mcp_servers.* keys before comparison. It still compares unrelated thread configuration, as shown by the regressi…
Description checkPassedThe description covers the problem, implementation, scope, linked issue, verification results, limitations, and tooling. It does not include an explicit maintainer approval comment as requested by the…
Full details: Linked Issues check

Explanation

Issue #15828 requires Codex provider threads to keep instance launch-argument MCP servers and add T3's t3-code server. codexThreadRuntimeParams now sends only mcp_servers.t3-code with the existing endpoint, authorization, 10-second startup grace, and 3,900-second timeout. The shared helper covers thread start, resume, fork, and rollback resume. The adapter regression test checks the generated thread/start request, and the reported live test confirms both servers reach ready. No global Codex configuration change was added.

Full details: Out of Scope Changes check

Explanation

The changed replay normalizer removes historical whole-table mcp_servers and dotted mcp_servers.* keys before comparison. It still compares unrelated thread configuration, as shown by the regression test. This change supports replay of the new adapter configuration and historical recordings. The adapter test replaces a stale shape assertion with behavior coverage. The whole-PR changes remain within the MCP configuration and replay scope of issue #15828.

Full details: Description check

Explanation

The description covers the problem, implementation, scope, linked issue, verification results, limitations, and tooling. It does not include an explicit maintainer approval comment as requested by the Scope and approval template, but the description is otherwise substantially complete.

✨ 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.

Added 2 commits

  • Merge remote-tracking branch 'upstream/main' into fix/codex-keep-launch-mcp-servers9f2fc88
  • Merge remote-tracking branch 'upstream/main' into fix/codex-keep-launch-mcp-servers39dd5d4
JJulius Marminge8d ago

Taking over this one for batch codex-01. I will resolve the adapter and replay conflicts against current main, keep the MCP override fix focused, rebuild minimal regression coverage, and run independent reviews before pushing here. The PR will remain unmerged.

Added 2 commits

  • Merge remote-tracking branch 'origin/main' into fix/codex-keep-launch-mcp-serverse2d17fc
  • test(codex): verify MCP overrides through adapter and replay8cb87d6

Added 2 commits

  • fix(codex): keep thread runtime helper privatebdbe41a
  • Merge remote-tracking branch 'origin/main' into fix/codex-keep-launch-mcp-servers3c34a97