Let plugin threads actually use MCP servers, and let an operator drop the ones they should not be offered - #111
Merged
Conversation
patriyang
force-pushed
the
fix/105-109-mcp-elicitation-and-scoping
branch
from
August 11, 2026 23:41
be37dbf to
c6c8d87
Compare
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.
Closes #109
Closes #105
Two changes on one surface: whether a Codex thread this plugin starts can use the MCP servers it
is offered, and whether the operator can decide which ones it is offered at all.
#105 was filed as an open policy question with three options and a prerequisite nobody had tested.
Probing a live app-server to settle that prerequisite turned up #109, which changes the premise —
so the two land together.
#109 — every MCP tool call was failing as a phantom user rejection
Reproduced through the shipped entry point, before the fix:
No user was asked. Tracing the JSONL against a raw
codex app-server, the app-server sends aserver→client request
mcpServer/elicitation/requestcarrying_meta.codex_approval_kind: "mcp_tool_call", andAppServerClientBase.handleServerRequestansweredevery server request with
-32601. The app-server reads that refusal as a denial rather than as"this client does not implement that method", and reports it to the model as a user rejection.
handleServerRequestnow answers elicitations:_meta.codex_approval_kindpresent) →{ action: "accept", content: {}, _meta: null }.These threads run
approvalPolicy: "never"with nobody at the keyboard; an approval gate has noone to answer it.
mode: "url"→{ action: "decline", content: null, _meta: null }.Fabricating form values would be worse than declining, and a protocol-valid decline still avoids
the phantom rejection.
-32601, which is the right answer for a method the clientgenuinely does not implement.
Both transports inherit this from the base class, so the broker path is covered too — and in shared
mode the broker is the process that answers, which is worth knowing when testing: a broker started
before this change keeps rejecting until it restarts.
This means MCP tool calls from plugin threads are now approved unattended. That is deliberate rather
than incidental, it is the direction the operator picked, and it is stated plainly in the README —
an MCP tool is not confined by the thread's sandbox the way a shell command is. The knob below is
the control.
#105 —
CODEX_DISABLED_MCP_SERVERSComma-separated server names to disable on threads this plugin starts. Unset by default, so nothing
changes for anyone who does not opt in — issue option 1 stays the default, option 2 becomes
available per-server.
Two things worth recording, both established against a live app-server (codex-cli 0.147.0) rather
than inferred from the generated types:
config: { mcp_servers: {} }onthread/startleaves every server attached — same at the CLI (
codex exec -c 'mcp_servers={}').mcp_servers.<name>.enabled = falsedoes work, as a bare dotted key, in both thethread/startconfigmap and-c. That is the lever used here.Names must match
[A-Za-z0-9_-]+; anything else is skipped with a warning. That is not fussiness:-c 'mcp_servers."hermes-vault".enabled=false'fails config loading outright(
invalid transport in mcp_servers."hermes-vault") because the quoted segment is read as a newserver table, so emitting a quoted key would break
thread/startfor the whole run instead of justleaving one server enabled. Hyphens are fine unquoted.
Also fixed while in those two functions:
buildThreadParams/buildResumeParamsonly builtparams.configwhenwritableRootswas non-empty, silently droppingoptions.configotherwise.The map is now built unconditionally and attached only when non-empty — with nothing to say, the
params are byte-identical to before.
Verification
node --test --test-concurrency=1 tests/*.test.mjs— 354 pass, 0 fail (346 onmain; +8 here).Tests were written first and confirmed red: reverting only
app-server.mjsreds the threeelicitation tests, and reverting only
codex.mjsreds the disable tests. New coverage asserts therecorded client reply on the wire (accept / decline /
-32601) and the recordedthread/startandthread/resumeconfig maps, via the fake app-server fixture.End-to-end against a live Codex and the shipped entry point, same workspace, only the env var
differing:
user rejected MCP tool callCALLED 42— the tool actually ranCODEX_DISABLED_MCP_SERVERS=codegraphNO_CODEGRAPH— not offeredLeft alone
health question promptly, which is the exact thing the server behind tool-in-flight watchdog kills the whole review turn when an MCP call never returns, instead of failing the call #100 failed to do.
mcp_servers.<name>.tool_timeout_sec/startup_timeout_sec, whichcodex mcp list --jsonshows exist per server. A more targeted lever than the global budgets from Classify watchdog-aborted turns, give MCP calls their own budget, and pace retryable failures #104 for the original
stall, but a separate change — worth its own issue.
Collision note
#102was in flight in a parallel worktree while this was being built and landed first as #110.This branch is rebased on top of it (version bumped to 1.0.46 rather than 1.0.45), and the full
suite was re-run after the rebase: 361 pass, 0 fail. No overlap between the two changes.