Repository navigation
fix(mcp): install the Codex plugin, and report the real cause when Codex fails - #1295
Merged
Merged
Conversation
`spawnSync` splits the cause across `error`, `stderr` and `stdout`, and sets none of them when the process merely exits non-zero. The install, MCP add and plugin uninstall paths read `stderr` alone, so a failure to spawn reported `Codex plugin install failed:` with an empty reason and filed an exception carrying nothing. `removeServer` already read `error`. Classify failures that live in the user's environment — a CLI too old for plugins, a broken install, an unreadable config, permissions, a full disk, an unreachable GitHub — and return a hint naming the fix instead of reporting them. Report what remains under a constant message, with the varying detail in properties and home directories replaced by `~`, so one root cause stays one issue rather than one per user. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
🧙 Wizard CIRun the Wizard CI and test your changes against wizard-workbench example apps by replying with a GitHub comment using one of the following commands: Test all apps:
Test all apps in a directory:
Test an individual app:
Show more apps
Test against a Context Mill branch:
Add Results will be posted here when complete. |
`installPlugin` ran `codex plugin marketplace add` and stopped. That registers the catalog; it does not install from it, so `codex plugin list` reported `posthog@posthog not installed` while the wizard reported success. The command exits 0, so nothing was ever captured. `isPluginInstalled` read `[marketplaces.posthog]` from config.toml — the catalog, not the plugin — so a re-run reported "already installed" and never corrected the state. It now asks `codex plugin list -m posthog`. `removePlugin` removes the plugin and the marketplace, and acts when either is present, so a user left with a registered marketplace and no plugin still gets it cleared. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
gewenyu99
changed the title
`plugin marketplace add` and `plugin add` clone git repositories and take seconds. Running them through `spawnSync` blocked the event loop, so the TUI spinner painted one frame and froze for the whole install. Every codex invocation now goes through `runCodex`, which wraps `execFile`. `findCodexBinary` stays synchronous: it is one fast call, and `supportsPlugin` is a synchronous contract. Prefer stderr over the error message when reporting a failure. `execFile` leads its message with `Command failed: <cmd>`, and the TUI shows the first line, so reporting it would show the user our own invocation instead of what codex said. The message remains the fallback, since a process that never starts reports only there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Hinting takes a failure out of error tracking, so a pattern that claims too much retires one of our bugs into advice nobody can act on — and the only evidence is that the exception count fell, which reads as the fix working. Three patterns claimed too much. Serde wording (`invalid type`, `is no longer supported`, `must contain at least one`) says nothing about whose file codex choked on, so a broken PostHog/ai-plugin manifest was blamed on the user's config.toml. `git clone ... failed` matched a deleted or renamed PostHog/ai-plugin and told the user to check their network. And the "too old for plugins" hint fired during `mcp add`, where the rejected argument is a flag we passed, naming a plugin command that has nothing to do with what failed. Each entry now says which stages it applies to and what disqualifies it, and anything it cannot claim confidently goes back to being reported. The stage names are a union, because a hint scoped to a stage nobody passes is silently dead — which is how `marketplace add` lost its hint while this was being written. A hinted failure now emits an event, so over-matching stays countable after it leaves error tracking. `runCodex` reported `error.message` when neither stream spoke, and execFile always sets one: it leads with `Command failed:` and the full binary path. That path contains the user's home directory, so the detail this change exists to make constant was per-machine. Report the exit status instead. A kill of ours now outranks whatever the streams got out before we cut them off, rather than reading as codex rejecting the install. Bound every invocation: a 120s timeout, a 16MB output ceiling instead of execFile's 1MB default, and stdin closed, since a codex that waits on input never settles the promise. `plugin add` now reads codex's already-installed wording like every other spawn here. `removeServer` and the config.toml write path interpolated their reason into the exception message, filing one issue per user; both now report a constant message with the scrubbed detail in properties. Nine of the ten behaviours above are mutation-verified: reverting each one fails at least one test. Closing the child's stdin is not — the mocked execFile never creates a child — so it rests on the mechanism. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
|
|
||
| // Registering the catalog does not install from it. Skipping this left the | ||
| // user with a marketplace, no skills, and a wizard reporting success. | ||
| const result = await runCodex(binary, ['plugin', 'add', PLUGIN_REF]); |
There was a problem hiding this comment.
Medium: Unverified remote plugin installation
An attacker who compromises the marketplace repository can publish malicious skills and have the wizard automatically install them into Codex, where they are loaded with the user's Codex tool permissions. Resolve the marketplace and plugin to an immutable, reviewed revision or verify a trusted digest/signature before enabling the plugin.
PR overviewThis pull request updates the Codex MCP setup flow to install the Codex plugin and surface the underlying cause when Codex installation fails. One security issue remains open after one was addressed. The setup flow installs a plugin from a remote marketplace without pinning or verifying its contents, so a compromised marketplace could cause malicious code to be loaded with the user’s Codex tool permissions. Open issues (1)
Fixed/addressed: 1 · PR risk: 8/10 |
`redactSecrets` masked our own key shapes only — `Bearer ...` and `ph[xspc]_...` — from a time when the text reaching it came from CLIs we invoked ourselves. The plugin stages clone git repositories and quote the user's own config, so the text is no longer only ours: git echoes the remote it failed on, credentials and all, and `runCodex` now merges stdout into the `details` property that leaves the machine. `https://user:ghp_...@github.com/PostHog/ai-plugin.git` matches no entry in `EXPECTED_FAILURES`, so nothing turns it into a hint first and the token is reported verbatim. Each added pattern is a shape a CLI here actually prints — git remote userinfo, GitHub tokens, and the OpenAI keys codex already reports on — rather than a guess at every possible secret. A test pins that an ordinary URL survives untouched, since over-redaction hides the cause we report for. Mutation-verified: restoring the two original patterns fails 3 of the 4 new tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
sarahxsanders
approved these changes
Sep 22, 2026
| }, | ||
| { | ||
| match: | ||
| /EACCES|EPERM|permission denied|read-only file system|not permitted/i, |
Collaborator
There was a problem hiding this comment.
not blocking, but my robot found this
permission denied here matches git's SSH clone failure, could ass an unless the way you did on the network entry below. more of a cheap guard than something to worry about
`isPluginInstalled` answered about the plugin alone, and `index.ts` asks it to decide whether a client has anything left to remove. A machine with our marketplace registered and no plugin — the state this branch exists to correct — answered false, so Codex never appeared for removal and `removePlugin` never ran. The orphaned catalog stayed for good, and `removePlugin` already knew how to clear it. Widening that answer alone would have reintroduced the defect at the top of this branch, because `installPlugin` asked the same method whether to skip the install. They are two questions: what is left on the machine, and what is actually serving. So `isPluginPresent` keeps the narrow answer for the install decision and for removal's plugin step, and `isPluginInstalled` reports either half for discovery. This changes a contract this branch chose on purpose — a marketplace-only machine reporting false so the install would run. That behaviour is intact and still tested; only the question discovery asks has moved. Also covers `isMarketplaceRegistered`'s catch, which lost its test when `isPluginInstalled` moved off config.toml. A fresh machine has no `~/.codex/config.toml`, and an exception out of discovery would take down every client's removal, not just Codex's. Mutation-verified, each one failing at least one test: narrowing discovery back, widening removal's plugin step, widening the install check, and letting the config read throw. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
gewenyu99
added a commit
that referenced
this pull request
Sep 23, 2026
Brings in the eight main fixes since beccc79 (#1275, #1280, #1284, #1294, #1295, #1301, #1302, #1305). One conflict: agentic detection keeps main's two-attempt retry loop from #1305 and adds A1's progress emitter to the agent it initializes. Generated-By: PostHog Desktop Task-Id: d14e92bb-6ee1-49b5-8502-39cb80079589
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.
Two defects on the Codex plugin install path. Same method, same client, and the second builds on helpers the first introduces.
1. The plugin never installed
installPluginrancodex plugin marketplace add PostHog/ai-pluginand stopped. That registers the catalog; it does not install from it:The wizard then reported
✔ Plugin installed for: Codex. The Codex plugin carries 164 skills and no MCP server (pluginBundlesMcpServer(): false), so every Codex user who ranmcp addhas the server and none of the skills.isPluginInstalledread[marketplaces.posthog]fromconfig.toml— the catalog, not the plugin — so it confirmed the false success, and a re-run reported "already installed" and never corrected the state.marketplace addexits 0, so this produced no exceptions at all. It appears nowhere in error tracking; it was found by inspecting a machine after a successful-looking run.2. A failure reported nothing
spawnSyncsplits a failure acrosserror,stderrandstdout, and sets none of them when the process merely exits non-zero. Three of four call sites readstderralone, so a failure to spawn reportedCodex plugin install failed:with an empty reason and filed an exception carrying nothing. 18 events across 18 users in 30 days, live on 5.52.4.removeServeralready readresult.error.Of ~139 Codex install failures in 30 days (~132 users), roughly 100 are conditions the wizard cannot fix and reported anyway:
spawn … ENOENT, broken installunexpected argument 'marketplace'config.tomlinvalid — bad types,wire_api,model_catalogEPERM,ENOSPC, missingOPENAI_API_KEYChanges
codex plugin add posthog@posthogafter the marketplace is registered. Idempotent — twice exits 0.codex plugin list -m posthog, viaisPluginPresent. Users already in the broken state get the plugin on their nextmcp add. Reading the CLI's own status rather than a filesystem proxy is deliberate: this defect was caused by trusting a proxy for the real thing. (Section 5 splits this from what discovery asks — the two are not the same question.)isMarketplaceRegisteredkeeps theconfig.tomlcheck, used to skip a redundant marketplace add. Registration moved intoregisterMarketplace, carrying the stale-cache retry with it.removePluginremoves plugin and marketplace, acting when either is present, so a user left with a registered marketplace and no plugin still gets it cleared.runCodexmergesstderrandstdout, preferring stderr because that is what codex actually said, and falls back todescribeExecFailure— the exit status, signal or timeout — when neither stream spoke or when the kill was ours. So a reason is never blank, at every call site that previously droppederror.stageanddetailsin properties, and home directories replaced by~, so one root cause is one issue instead of one per user.expectedFailureHintandscrubHomePathslive inresults.tsbesideredactSecretsandsummarizeFailure— the module that already owns turning raw CLI failure text into something reportable. The pattern table stays incodex.ts, since the wording is Codex's.3. A frozen spinner
plugin marketplace addandplugin addclone git repositories and take seconds. Running them throughspawnSyncblocked the event loop, so the TUI spinner painted one frame and froze for the whole install — measured at 1 painted frame against 50 on an async path.Every codex invocation now goes through
runCodex, wrappingexecFile.findCodexBinarystays synchronous: one fast call, andsupportsPluginis a synchronous contract. Verified against the real CLI: 28 event-loop ticks during an install/remove cycle that previously yielded none.Reporting also prefers stderr over the error message.
execFileleads its message withCommand failed: <cmd>, andsummarizeFailureshows the first line, so reporting the message would have shown the user our own invocation instead of what codex said. The message stays the fallback, since a process that never starts reports only there.The spinner component itself rides along in #1294 (register the marketplace before the Claude Code plugin install), which carries it as one bullet — this PR makes it able to animate for Codex.
4. Hints that silenced our own failures
A hint is not a neutral reclassification — it takes the failure out of error tracking. So the cost of a pattern matching too much is asymmetric: one of our bugs becomes advice the user cannot act on, and the only evidence is that the exception count fell, which reads exactly like the fix working.
Three patterns claimed too much. Reproduced by running the table against the text each case produces:
--bearer-token-env-varduringmcp addcodex plugin marketplace add PostHog/ai-pluginPostHog/ai-plugin's own manifest is broken (serde wording)~/.codex/config.tomlPostHog/ai-plugindeleted or renamedEvery entry now declares the
stagesit applies to and the wording that disqualifies it (unless), and anything it cannot claim confidently goes back to being reported. Stage names are a union type, because a hint scoped to a stage nobody passes is silently dead — which is howmarketplace addlost its hint while this was being written. A hinted failure emitswizard: mcp expected failure hinted, so over-matching stays countable once it leaves error tracking.Two more reporting defects in the same layer:
runCodexfell back toerror.messagewhen neither stream spoke, andexecFilealways sets one — leading withCommand failed:and the full binary path. The per-machine detail this PR exists to eliminate was reintroduced one layer down, and theexited with codefallback was unreachable. The guarding test passednew Error(''), a shapeexecFilenever emits.removeServerand theconfig.tomlwrite path still interpolated their reason into the exception message, so a config path filed one issue per user. Both now report a constant message with the scrubbed detail in properties.Also bounded: a 120s timeout, a 16MB output ceiling instead of
execFile's 1MB default (a verbose clone hit it, and the kill read as codex rejecting the install), and the child's stdin closed, since a codex that waits on input never settles the promise.plugin addnow reads codex's already-installed wording like every other spawn here.5. One method, two questions
isPluginInstalledwas asked two things by two callers, and one return valuecannot answer both.
getInstalledClientsinindex.tsasks whether a client has anything left toremove — its comment says "for removal purposes". Section 1 narrowed the method
to the plugin alone, so a machine with our marketplace registered and no plugin
— the state this PR exists to correct — answered
false. Codex never appearedfor removal,
removePluginnever ran, and the orphaned catalog stayed for good.removePluginalready knew how to clear it; nothing ever called it.Widening the answer alone would have reintroduced the defect at the top of this
PR, because
installPluginasked the same method whether to skip the install.So the two questions are now separate:
isPluginPresentkeeps the narrow answerfor the install decision and for removal's plugin step, and
isPluginInstalledreports either half for discovery.
This changes a contract section 1 chose on purpose. The behaviour it was chosen
for — a marketplace-only machine still getting the plugin installed — is intact
and still tested; only the question discovery asks has moved.
Also covers
isMarketplaceRegistered's catch, which lost its test whenisPluginInstalledmoved offconfig.toml. A fresh machine has no~/.codex/config.toml, and an exception out of discovery would take down everyclient's removal, not just Codex's.
6. Secrets in what a failed clone prints back
redactSecretsmasked our own key shapes only, from a time when the textreaching it came from CLIs we invoked ourselves. The plugin stages clone git
repositories and quote the user's own config, so the text is no longer only
ours: git echoes the remote it failed on, credentials and all, and
runCodexmerges stdout into the
detailsproperty that leaves the machine.https://user:ghp_...@github.com/PostHog/ai-plugin.gitmatches no entry inEXPECTED_FAILURES, so nothing turns it into a hint first and the token isreported verbatim. Added: git remote userinfo, GitHub tokens, and the OpenAI
keys codex already reports on — each a shape a CLI here actually prints, not a
guess at every possible secret. A test pins that an ordinary URL survives
untouched, since over-redaction hides the cause we report for.
Not changed
url is not supported for stdio in mcp_servers.posthog(5 events / 4 users) looked like the config we write being malformed. Writing exactly whatwriteServerSectionemits into a sandboxCODEX_HOMEand running codex-cli 0.154.0 against it lists the server asenabled, so the config is valid — those events are Codex versions predating URL-based servers. Nothing to fix.A failed probe reads as "not installed".
isPluginInstalledreturningfalsefor a probe that merely failed letsremovePluginskipplugin remove, clear the marketplace under it, and report success. Left alone here: the same shape is at six sites, three outside this diff, includingMCPClient.ts:59which five clients inherit. Fixing one site would leave five clients lying and make this one inconsistent with the convention. Tracked in #1298.Test plan
pnpm test— 3157 pass.pnpm typecheck,pnpm lintclean (thecodex.test.tsnon-null-assertion warning is gone).results.test.ts:scrubHomePathsover unix, windows and no-home text;expectedFailureHintfirst-match, no-match and empty-text cases.codex.test.tsinstall/removal: plugin installed after the marketplace is registered; a user who already has the marketplace still gets the plugin; an already-installed plugin left alone; a failingplugin addreported rather than claimed as success; removal clears plugin and marketplace; removal still clears a marketplace with no plugin; nothing to do when neither is present.codex.test.tsreporting: a spawn error yields an actionable reason; an unrecognised spawn error reports its own text; a bare non-zero exit reports its status; stdout-only failures are read; one case per environment class hints without reporting; an unexpected failure reports under the constant message with~-scrubbed detail.The
isPluginInstalledtests replace three that asserted the old contract. One of the replacements has since been re-pointed by section 5: a registered marketplace with no plugin now reportstrue, because that is what discovery needs, with the install decision covered separately. Four install tests were rewritten for the two-step flow: a blanketstatus: 1mock now lands on the marketplace-add stage, so they route per-command to keep their original intent.results.test.tshint scoping: a scoped entry preferred on its stage, skipped on another, ignored when no stage is given, and reporting when only a mismatched scoped entry exists;unlesshinting and disqualifying.codex.test.tswhat the table refuses to claim: no plugin advice when an old CLI rejects anmcp addflag; a broken plugin manifest reported rather than blamed on the config (3 wordings); a missing plugin repository reported rather than blamed on the network; an unrecognised failure that only mentionsconfig.tomlstill reported; a genuine config failure still hinted; hinted failures counted, with~-scrubbed detail.codex.test.tswhen neither stream spoke: the exit status reported instead of our own invocation; no home directory in the reported detail; a bearer token echoed back is redacted; every invocation bounded by a timeout and an output ceiling; our own kill outranking the output it interrupted.Mutation checks: stubbing out the
plugin addcall fails 6 tests; restoring the oldresult.status !== 0guard andstderr-only read fails the 4 covering that defect. For section 4, each of ten behaviours was reverted in turn — nine fail at least one test. The tenth, closing the child's stdin, is not covered: the mockedexecFilenever creates a child, so it rests on the mechanism and would need an integration test.Section 5: a registered marketplace with no plugin reported for removal; nothing reported when neither is present; an unreadable
config.tomltreated as no marketplace rather than throwing. Section 6: credentials in a git remote, bare GitHub tokens, another provider's key, and an ordinary URL left intact.Mutation checks for sections 5 and 6, each failing at least one test: narrowing discovery back to the plugin alone; widening removal's plugin step; widening the install check (the original defect); letting the config read throw; and restoring the two original redaction patterns.
Against the real codex-cli 0.155.1, in throwaway
CODEX_HOMEsandboxes, confirming the contracts this PR depends on:plugin addbeforemarketplace addexits 1 (plugin \posthog` was not found in marketplace `posthog`), so the ordering is load-bearing;plugin addtwice exits 0;plugin removeon an absent plugin exits 0; and the STATUS column readsnot installed/installed, enabled`, lowercase.Against the real codex-cli 0.154.0, in throwaway
CODEX_HOMEsandboxes:{success:true}alreadyInstalled{success:true}{success:true}alreadyInstalled{success:true}The second row is the state this fixes; the old code reported
truefor it and did nothing.Acceptance
The install defect is not measurable from error tracking — it exits 0. Verify by inspection:
codex plugin list -m posthog # expect: installed, enabledThe blank-reason defect is measurable:
Expect zero, and the remainder should collapse toward a single fingerprint.
Total Codex install exceptions will not fall by the full ~100 projected from the original table. Section 4 moved three classes back into reporting — a broken plugin manifest, a missing plugin repository, and any failure that merely mentions
config.toml— so those now surface asCodex plugin install failedrather than being hinted away. That is the intended direction: a hint that hides our own bug is worse than an exception. Watchwizard: mcp expected failure hintedalongside the exception count; a hinted class growing without a matching support signal is the tell that a pattern has started over-matching again.🤖 Generated with Claude Code