Classify watchdog-aborted turns, give MCP calls their own budget, and pace retryable failures - #104
Merged
Merged
Conversation
Reproduces both gaps before the fix: - A watchdog-aborted turn records failureClass: null, so a caller cannot tell "the turn never finished" from "the turn ran and produced nothing" (#100). - An MCP tool call shares the generic quick-tool inactivity budget, so a call that is merely slow kills the whole turn along with everything the model had accumulated (#100). - A retryable capacity failure carries no pacing, so a controller that branches on retryable can re-dispatch immediately into the same capacity window (#103). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A turn the watchdog interrupts now records failureClass: "stalled", so a caller can tell a killed turn from one that ran and found nothing. The stall is read from the turn's own state rather than matched out of the error text, and it never triggers the capacity model fallback. A retryable failure now carries retryAfterMs. Capacity rejections get a 60s floor because fallback resolution is deterministic: an immediate retry resolves the same models and fails the same way. Pacing is dropped wherever retryable is gated off, so it never appears with retryable: false; state drift keeps no pacing, since re-running it is not time-gated. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The turn's inactivity watchdog measured every quick tool against one 90s budget, so an MCP call that was merely slow cost the whole turn and everything the model had accumulated on it. MCP calls now carry their own budget (180s, CODEX_MCP_TOOL_STALL_TIMEOUT_MS), and the silence window is the most patient quick tool in flight, since the watchdog is asking whether anything at all is happening on the turn. The turn backstop still bounds it: a tool budget above the backstop clamps down to it, unless the backstop is switched off, in which case there is no outer bound to clamp against. The wall-clock max-in-flight cap is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The failure contract is what automated callers branch on, so it now has a table of its own: what each class means and whether repeating is safe. The retry-pacing rule is stated in the same place, since retryable without a pace is what let a controller spin. The four stall budgets were undocumented, including the new MCP one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review and task paths each classified twice — once to decide the capacity fallback, once to build the result — so a change to the contract had four sites to keep in agreement, and the stalled branch tested turnProducedNothing twice over. One helper now owns the whole rule: status, structural stall, the produced-nothing retry guard, and dropping pacing from a retry that is not on offer. Deep review round 1 finding. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
patriyang
added a commit
that referenced
this pull request
Aug 11, 2026
… the ones they should not be offered (#111) 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: ``` node plugins/codex/scripts/codex-companion.mjs task --json \ "Call the codegraph MCP status tool once and report the indexed file count..." → "failureMessage": "user rejected MCP tool call" ``` No user was asked. Tracing the JSONL against a raw `codex app-server`, the app-server sends a server→client **request** `mcpServer/elicitation/request` carrying `_meta.codex_approval_kind: "mcp_tool_call"`, and `AppServerClientBase.handleServerRequest` answered *every* 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. `handleServerRequest` now answers elicitations: - **approval-kind** (`_meta.codex_approval_kind` present) → `{ action: "accept", content: {}, _meta: null }`. These threads run `approvalPolicy: "never"` with nobody at the keyboard; an approval gate has no one to answer it. - **anything else** — a real data-collection form, or `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. - **every other server method** keeps `-32601`, which is the right answer for a method the client genuinely 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_SERVERS` Comma-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: - **The mechanism the issue guessed does not work.** `config: { mcp_servers: {} }` on `thread/start` leaves every server attached — same at the CLI (`codex exec -c 'mcp_servers={}'`). - **`mcp_servers.<name>.enabled = false` does work**, as a bare dotted key, in both the `thread/start` `config` map 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 *new* server table, so emitting a quoted key would break `thread/start` for the whole run instead of just leaving one server enabled. Hyphens are fine unquoted. Also fixed while in those two functions: `buildThreadParams` / `buildResumeParams` only built `params.config` when `writableRoots` was non-empty, silently dropping `options.config` otherwise. 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 on `main`; +8 here). Tests were written first and confirmed red: reverting only `app-server.mjs` reds the three elicitation tests, and reverting only `codex.mjs` reds the disable tests. New coverage asserts the recorded client reply on the wire (accept / decline / `-32601`) and the recorded `thread/start` and `thread/resume` config 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: | Run | Result | | --- | --- | | before the fix | `user rejected MCP tool call` | | after, env unset | `CALLED 42` — the tool actually ran | | after, `CODEX_DISABLED_MCP_SERVERS=codegraph` | `NO_CODEGRAPH` — not offered | ## Left alone - **Probing servers for workspace fitness** (issue #105 option 3). It depends on servers answering a health question promptly, which is the exact thing the server behind #100 failed to do. - **`mcp_servers.<name>.tool_timeout_sec` / `startup_timeout_sec`**, which `codex mcp list --json` shows exist per server. A more targeted lever than the global budgets from #104 for the original stall, but a separate change — worth its own issue. - **A wildcard "disable everything" value.** Named servers only until something asks for more. ## Collision note `#102` was 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.
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
1 participant
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 #100
Closes #103
Both issues land on one surface: the machine-readable failure contract a
caller reads off a job record. #100 is a turn that dies with nothing to
act on; #103 is a failure that says "retry" without saying "when". A
controller branching on either one today makes a bad decision.
#100 — one wedged tool call cost the whole turn, silently
Reported from a
deep-reviewthat called an MCP server which neverreturned. The 90s tool-in-flight watchdog fired, the turn was
interrupted, and the caller got
Codex review failed— after 134activity events of investigation. Two review rounds went to zero signal.
Three sub-questions in the issue, answered separately:
Can we fail the tool call instead of the turn? No — not at this
layer. The plugin is an app-server RPC client;
AppServerMethodMapinapp-server-protocol.d.tsexposesturn/interruptand nothingper-item. There is no way to hand one tool call a timeout and let the
turn keep going. Left alone rather than faked.
Should an aborted turn be distinguishable in the payload? Yes, and
it now is. A watchdog abort records
failureClass: "stalled", read offthe turn's own state rather than matched out of the error text, so
"the review never finished" no longer looks identical to "the review
found nothing". It is
retryableonly when the turn produced nothing atall, and it never triggers the capacity model fallback.
Is 90s right for MCP specifically? Not when it is shared with web
search. MCP calls get their own inactivity budget, defaulting to 180s
(
CODEX_MCP_TOOL_STALL_TIMEOUT_MS), because they can legitimately runlonger and the blast radius of cutting one short is the entire turn.
With several quick tools in flight the most patient one sets the window
— the watchdog is asking whether anything is happening. The turn
backstop still bounds it: a tool budget above the backstop clamps to it,
unless the backstop is switched off, in which case there is nothing to
clamp against.
The fourth sub-question — filtering MCP servers per workspace — is a
policy call about what a reviewer should be allowed to reach, not a bug
fix, so it is not in here.
#103 —
retryable: truewith no paceretryableis correctly defined (the turn produced nothing, sorepeating is safe) but says nothing about when, and backup-model
resolution is deterministic: re-dispatching immediately resolves the
same pair of models and can fail identically. That is precisely the
shape of a
fix-issue-batchfan-out, where correlated capacityrejections are the norm.
Retryable failures now carry
retryAfterMs, with a 60s floor forcapacity. It is a fixed floor rather than a parsed
Retry-After: thereis no evidence the app-server surfaces a hint today, and inventing a
parser for one would be speculative. Pacing is dropped wherever
retryableis gated off, so it never appears alongsideretryable: false;state-driftstays retryable with no pacing, since waiting isnot what fixes it.
Verification
Failing tests were committed first (a41eff0) and are visible as the
first commit on this branch:
hung-toolturn recordedfailureClass: null— now"stalled"its own budget and completes
retryAfterMs— now pacedRetry after: 60snode --test --test-concurrency=1 tests/*.test.mjs— 346 pass, 0 fail.Two pre-existing tests were adjusted deliberately, not to make red go
away: the
classifyFailureMessageshape assertion gained the new thirdfield, and the hung-tool watchdog test now pins
CODEX_MCP_TOOL_STALL_TIMEOUT_MSso it still proves the tool budgetfired rather than passing on the turn backstop wearing the same label.
Out of scope
The issue's secondary observation —
pytestandpythonmissing fromthe review shell's PATH, only
python3present — is a property of thelogin shell the review runs in, not something the plugin sets. No code
change here.
Plugin version bumped to 1.0.42.
🤖 Generated with Claude Code