Skip to content

fix(codex): make MCP startup work in Codex - #81

Open
sVIKs wants to merge 1 commit into
corezoid:developfrom
sVIKs:fix/codex-mcp-startup
Open

fix(codex): make MCP startup work in Codex#81
sVIKs wants to merge 1 commit into
corezoid:developfrom
sVIKs:fix/codex-mcp-startup

Conversation

@sVIKs

@sVIKs sVIKs commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

What & why

Codex can install the Simulator plugin, but the MCP server failed before the JSON-RPC handshake because .mcp.json launched it through $CLAUDE_PLUGIN_ROOT/mcp-server/run.sh.

In Codex 0.145, CLAUDE_PLUGIN_ROOT is not exposed to MCP subprocesses, so the shell resolves that command to /mcp-server/run.sh, exits immediately, and Codex reports the startup failure as connection closed: initialize response.

This change keeps Claude Code working and adds Codex-safe plugin-root resolution:

  • .mcp.json still honors $CLAUDE_PLUGIN_ROOT first for Claude Code compatibility.
  • If that is unavailable, it resolves the installed Codex plugin root from CODEX_HOME or ~/.codex/plugins/cache/simulator/simulator/*.
  • The launcher preserves the user's current project directory as SIMULATOR_WORK_DIR; plugin root and workspace root are no longer conflated.
  • Local dev-checkout fallbacks remain supported.
  • CI now smoke-tests Claude-style and Codex-style launcher resolution.
  • Docs clarify the MCP-vs-skill difference and the changelog records the Codex startup fix.

A plain cwd: "." would not be sufficient here: in Codex it makes $PWD the plugin cache directory, which would break workspace-sensitive MCP operations.

Type of change

  • Bug fix
  • New MCP tool / capability
  • Skill behaviour
  • Docs
  • Chore / refactor

Checklist

  • make build && make vet && make test pass locally
  • If I added/renamed a tool — N/A, no tool changes
  • If I changed skill frontmatter — N/A, no skill frontmatter changes; make discovery produces no public/ drift
  • Reference docs that skills load at runtime stay under plugins/simulator/docs/ — N/A, untouched
  • No tokens or .env committed; TLS verification left on by default
  • No version bump per the current release-time flow; CHANGELOG.md ## [Unreleased] entry added

Notes for reviewers

Verified both host paths explicitly:

  • Claude compatibility smoke: env-var CLAUDE_PLUGIN_ROOT and text-substituted $CLAUDE_PLUGIN_ROOT; both complete initialize + tools/list with 148 tools.
  • Codex install smoke: temporary HOME, codex plugin marketplace add, codex plugin add simulator@simulator, command from codex mcp list, then initialize + tools/list with 148 tools.

Additional local checks:

  • python3 -m json.tool over all plugin/marketplace MCP manifests
  • sh -n plugins/simulator/mcp-server/run.sh
  • launcher matrix: valid CLAUDE_PLUGIN_ROOT, invalid CLAUDE_PLUGIN_ROOT + Codex fallback, ~/.codex, CODEX_HOME, local plugin-root layout, local repo-root layout
  • make build
  • make vet
  • make test
  • make discovery + git diff --exit-code -- public/

make lint remains advisory and currently reports pre-existing Go lint issues; CI marks that step continue-on-error.

@gh-corezoid

Copy link
Copy Markdown
Contributor

AI Review

Fixes MCP server startup failure in Codex by making .mcp.json discover the installed plugin root via CODEX_HOME/~/.codex when CLAUDE_PLUGIN_ROOT is absent, while preserving the user's working directory as SIMULATOR_WORK_DIR.

Checklist

Check Result
U1 — Conventional commit format ✅ pass
U2 — No leaked credentials ✅ pass
U3 — No merge commits ✅ pass
U4 — PR targets correct base branch (develop) ✅ pass
U5 — Build & test verification (Go changes) ⬜ skip — no *.go files changed
U6 — Architectural & design consequences ⚠️ warning (see below)
S1 — No manual edits to public/ ⬜ skip — no public/ changes
S2 — API path parameter names match OpenAPI spec ⬜ skip — no internal/tools/ changes
S3 — New tools have eval scenarios ⬜ skip — no new tools
S4 — Discovery artifacts committed if source changed ⬜ skip — no internal/ changes
S5 — All six manifest files version-synced ⬜ skip — no manifest files in diff
S6 — README and ARCHITECTURE §4 updated for new tools ⬜ skip — no new or renamed tools

Issues found

U6 — warning: 1,100-character one-liner embedded in .mcp.json is a long-term maintainability risk

The new args value in plugins/simulator/.mcp.json encodes the entire plugin-root resolution logic as a single ~1,100-character shell string inside a JSON array element. Today it works, but:

  • Any future change (new fallback path, debugging, adding a log line) requires touching a line that is very hard to read or diff reliably.
  • If the path logic ever needs to handle a space in CODEX_CACHE_ROOT (theoretically possible on Windows-style paths or non-standard setups), the for dir in $(ls -td "$CODEX_CACHE_ROOT"/...) pattern would misbehave because ls output is split on the custom IFS regardless of quoting in the glob argument.

A concrete failure scenario: a developer tries to add a fourth fallback (e.g. XDG_DATA_HOME), mis-escapes one quote in the JSON one-liner, and silently breaks the Claude path. The error only surfaces at MCP startup time, not at make test time.

Suggested fix (advisory only — not blocking): extract the body into plugins/simulator/mcp-server/launcher.sh, and set .mcp.json to sh "$CLAUDE_PLUGIN_ROOT/mcp-server/launcher.sh" (Claude) or an equivalent short stub. The CI test already reads .mcp.json dynamically, so it would cover the new script automatically. If keeping it self-contained is an explicit goal (as noted in the PR body), consider at least adding a comment block above the JSON file explaining the resolution priority.


No error-severity issues found. The fix is correct and well-tested for the three main resolution paths (Claude CLAUDE_PLUGIN_ROOT, Codex $HOME/.codex, Codex $CODEX_HOME). LGTM subject to the advisory above.


This review was generated automatically. A human maintainer should still make the merge decision.

@bazyk

bazyk commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

Hi @sVIKs — thanks for this fix. Two things are needed before it can be merged:

1. Rebase on develop to resolve conflicts. #90 landed on develop and edited the same spots this PR touches, so GitHub now reports the PR as CONFLICTING. The overlaps are:

  • CHANGELOG.md — both add entries under the ## [Unreleased] heading. Keep both; just place your ### Fixed block alongside the new ### Added block.
  • README.md — the MCP-tools table and the engines tree both gained new rows/lines. Keep both sets of changes.
git fetch origin
git rebase origin/develop
# resolve CHANGELOG.md and README.md (keep both sides), then:
git rebase --continue
git push --force-with-lease

2. Heads-up on the workflow file. This PR modifies .github/workflows/ci.yml, so merging it requires a token/account with the GitHub workflow scope. Once the rebase is done and CI is green, please merge via the GitHub web UI (or ping a maintainer with a workflow-scoped token) — a plain repo-scoped token is refused for workflow-file changes.

After the rebase, the CI run may again sit at action_required (external-contributor gate) — a maintainer will approve it. The code itself looks good; it's only these mechanics left.

@sVIKs
sVIKs force-pushed the fix/codex-mcp-startup branch from ec8106a to ae78ac0 Compare August 11, 2026 13:59
@sVIKs

sVIKs commented Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto current develop (6f9db4c) in ae78ac0 and force-pushed with lease.

The only manual conflict was CHANGELOG.md. Its current Unreleased telemetry and graph import/export entries were preserved, and the Codex startup fix was added alongside them under Fixed. The README and CI changes merged cleanly and retain the newer develop content.

Fresh verification on the rebased branch:

make build                          PASS
make vet                            PASS
make test                           PASS
go test -race -count=1 ./...        PASS
make discovery                      PASS
git diff --exit-code -- public/     PASS
git diff --check origin/develop...  PASS

A direct stdio initialize -> tools/list smoke test against the freshly built branch binary negotiated MCP successfully and returned 148 tools, including exportGraph, importGraph, uploadGraphFile, and getTaskStatus. The PR description was updated from the pre-rebase count of 144 to 148.

The workflow-file note is understood; no merge was attempted.

@gh-corezoid

Copy link
Copy Markdown
Contributor

AI Review

Re-review after synchronize: fixes MCP server startup failure in Codex by making .mcp.json discover the installed plugin root via CODEX_HOME/~/.codex when CLAUDE_PLUGIN_ROOT is absent, while preserving the user's working directory as SIMULATOR_WORK_DIR. The PR is now rebased and merges cleanly onto develop.

Checklist

Check Result
U1 — Conventional commit format ✅ pass
U2 — No leaked credentials ✅ pass
U3 — No merge commits ✅ pass
U4 — PR targets correct base branch (develop) ✅ pass
U5 — Build & test verification (Go changes) ⬜ skip — no *.go files changed
U6 — Architectural & design consequences ⚠️ warning (see below — same as previous review, still unresolved)
S1 — No manual edits to public/ ⬜ skip — no public/ changes
S2 — API path parameter names match OpenAPI spec ⬜ skip — no internal/tools/ changes
S3 — New tools have eval scenarios ⬜ skip — no new tools
S4 — Discovery artifacts committed if source changed ⬜ skip — no internal/ changes
S5 — All six manifest files version-synced ⬜ skip — no manifest files in diff
S6 — README and ARCHITECTURE §4 updated for new tools ⬜ skip — no new or renamed tools

Issues found

U6 — warning: 1,100-character one-liner embedded in .mcp.json is a long-term maintainability risk (unresolved from previous review)

The args value in plugins/simulator/.mcp.json still encodes the entire plugin-root resolution logic as a single ~1,100-character shell string inside a JSON array element. The IFS-newline trick (IFS=$(printf '\n.'); IFS=${IFS%.}) is actually handled correctly — splitting on newlines protects paths with spaces — so this is a style/maintainability concern only, not a correctness issue:

  • Any future change (new fallback path, adding a log line) requires editing a line that is nearly impossible to read or diff reliably in GitHub's UI.
  • A mis-escaped quote while editing will silently break the Claude path at MCP startup time, which is the hardest failure mode to debug.

Suggested fix (advisory only — not blocking): extract the body into plugins/simulator/mcp-server/launcher.sh and point .mcp.json to it. The existing CI smoke test (MCP launcher resolves plugin root and preserves workspace) already reads .mcp.json dynamically, so it would cover the extracted script automatically.


No error-severity issues found. The fix is correct and well-tested; the PR is clean and mergeable onto develop. LGTM subject to the advisory above.


This review was generated automatically. A human maintainer should still make the merge decision.

@sVIKs

sVIKs commented Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

The remaining warning is understood and intentionally not changed in this PR.

The inline code is the bootstrap resolver: when CLAUDE_PLUGIN_ROOT is absent, the process does not yet know the installed plugin root, so it also cannot locate mcp-server/launcher.sh. Pointing .mcp.json at that script would therefore reintroduce the same Codex startup failure unless the root-resolution expression remained inline.

Using cwd: "." is not equivalent for this plugin. It can make the launcher reachable, but changes PWD from the user workspace to the plugin cache. Simulator uses SIMULATOR_WORK_DIR for workspace-sensitive behavior, so that approach fixes executable discovery by breaking workspace discovery.

The current bootstrap has explicit guards, preserves the original PWD before resolving plugin files, handles paths with spaces through newline-only splitting, and is executed verbatim by CI for Claude, default Codex home, and custom CODEX_HOME layouts. After the rebase it was also exercised through a fresh Codex-installed cache and completed initialize -> tools/list with 148 tools.

Extraction would improve source formatting only after the plugin root is already known; it would not remove the root-discovery logic from .mcp.json. Given that constraint and the reviewer confirmation that this is maintainability-only and non-blocking, keeping the tested self-contained bootstrap is the smaller and more portable change.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants