fix(advisor): restore advisor consults for claude and codex - #377
Conversation
Advisor consults never completed. Three independent defects were in the path, each verified against a running desktop build. 1. `stave_consult_advisor` resolved grants in the Electron main process, but grants are issued in the host-service child process, so every consult failed with `unknown-consult-key`. The consult now forwards over `provider.consult-advisor`, the same seam already used by `provider.skip-advisor` and `respond-approval`. The host-service request backstop is raised to 15 minutes so it cannot pre-empt the runtime's own 10-minute `advisor-timeout` outcome with a transport error the primary cannot read. 2. Codex config override keys were quoted. Codex splits an override key on `.` and takes each segment verbatim without parsing TOML quoting, so `mcp_servers."name".enabled` registered a new server whose name contained quote characters and had no transport, failing config load for every isolated thread. Keys are now bare, MCP disables move to a nested value so names with dots stay exact, and servers injected by the plugin runtime are removed with `features.apps=false` rather than named. The same quoting bug silently no-opted every bundled-plugin disable; those are corrected too. 3. Provider debug logging killed the host service. Its stdout is the protocol frame channel and `console.debug` writes there, producing `invalid message frame header`. A stdout guard now claims the channel so only the frame layer holds the real writer and all other stdout writes are redirected to stderr. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0229b22cd0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return { | ||
| "features.apps": false, | ||
| ...buildCodexMcpDisableConfigOverrides( | ||
| reachable.filter((name) => configured.has(name)), |
There was a problem hiding this comment.
When an isolated Codex turn runs in a project with a project-scoped MCP server, this intersection can omit it: mcpServerStatus/list is queried without a cwd on the shared App Server spawned at process.cwd(), while config/read and the eventual thread use args.cwd. A server present only in the target project's configuration therefore appears in configured but not reachable, remains enabled at thread/start, and can give Advisor or secondary read-only turns network or mutation tools despite the isolation guarantee. Build disables for every server returned by the cwd-specific configuration, using the catalog only to handle injected servers that lack configuration entries.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in b588ccf.
Reproduced against codex-cli 0.146.0 with an App Server started in a neutral directory and projsrv declared only in the project layer:
catalog_no_cwd : ["globalsrv"] # mcpServerStatus/list
catalog_with_cwd : ["globalsrv"] # cwd param is ignored
config_with_cwd : ["globalsrv","projsrv"] # config/read {cwd}
And confirmed the leak is real rather than masked by override replacement — nested mcp_servers overrides merge, so projsrv survived into the isolated thread:
{mcp_servers:{globalsrv:{enabled:false}}} -> thread/start OK, projsrv still live
{mcp_servers:{globalsrv:{...}, projsrv:{enabled:false}}} -> thread/start OK
{mcp_servers:{globalsrv:{...}, codex_apps:{enabled:false}}} -> FAILED: invalid transport in `mcp_servers.codex_apps`
The third case is why the intersection existed, but it points at the right source rather than at the catalog: config/read {cwd} is already exactly the set of names that have an mcp_servers entry, so it is both complete and safe to name. Injected servers such as codex_apps stay covered by features.apps = false. The mcpServerStatus/list request is now dropped from this path entirely.
Regression coverage added at both levels — resolveCodexIsolationConfigOverrides and the thread/start params built by runCodexReadOnlyPromptWithClient — each verified red before the fix.
`resolveCodexIsolationConfigOverrides` gated the disable set on the
`mcpServerStatus/list` catalog. That catalog belongs to the shared App
Server process and accepts no cwd, so a server declared in a project
config layer between the thread's cwd and its repo root never appears in
it. Intersecting it with `config/read {cwd}` dropped exactly those
servers from the override, leaving them reachable inside a thread that
advertises isolation.
Source the names from `config/read {cwd}` alone. It resolves the project
layers, and it is also precisely the set of names that have an
`mcp_servers` entry — which is what makes them safe to name at all.
Codex-injected servers such as `codex_apps` still have no entry and are
still neutralized by `features.apps = false`, so nothing gains a
transport-less table. The catalog request is dropped entirely.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2 participants
Summary
Advisor consults never completed. Three independent defects sat in the path — one broke every consult, one broke Codex isolated threads, and one crashed the host service whenever Provider Debug Logging was on. All three are fixed and verified against a running desktop build.
Changes
Fixes
stave_consult_advisorto the host-service process instead of resolving grants in Electron main. Grants are issued in the child process, so every consult previously failed withunknown-consult-key. The consult now travels overprovider.consult-advisor, the same seamprovider.skip-advisorandrespond-approvalalready use.provider.consult-advisorrequest backstop to 15 minutes. It previously equalled the runtime's own 10-minute ceiling for the highest effort tiers, so the backstop could fire first and replace the runtime'sadvisor-timeoutresult with a transport error the primary cannot read..and takes each segment verbatim without parsing TOML quoting, somcp_servers."name".enabledregistered a new server whose name contained quote characters and had no transport — failing config load for every isolated thread (Advisor and secondary review). MCP disables now go through a nested value, which keeps names containing dots exact.features.apps=falserather than by name.codex_appshas nomcp_serversentry to disable, so naming it created a transport-less table and reproduced the same load failure. Isolation is genuinely stronger:read_mcp_resource,list_mcp_resources, andrequest_plugin_installdisappear from isolated threads.plugins."chrome@openai-bundled".enabled=falsecreated a decoy entry and left the real plugin enabled — every Codex turn had been sending a disable that did nothing.console.debugwrites there, so enabling Provider Debug Logging producedinvalid message frame headerand collapsed the host service. A stdout guard now claims the channel, leaving the real writer to the frame layer and redirecting all other stdout writes to stderr.Test Evidence
bun run typecheck— clean.Focused suites —
147 pass / 0 failacross the 10 advisor and Codex test files. 22 new regression tests, each confirmed Red before the fix:tests/advisor-consult-process-boundary.test.ts— main must not importproviders/advisor-consult; the host service must dispatch it; the backstop must exceed the longest advisor timeout across every effort tier.tests/codex-config-override-keys.test.ts— pins "no quote characters in override keys" as an invariant.tests/codex-isolation-config-overrides.test.ts,tests/host-service-stdout-guard.test.ts,tests/provider-runtime-advisor.test.ts,tests/local-mcp-service-bridge.test.ts.Full suite:
3745 pass / 12 fail. The 12 failures match the pre-existing baseline onmainand all pass when run in isolation — parallel-execution contamination, untouched by this branch.Manual verification in an isolated desktop build (separate
--user-data-dirandHOME, sandbox.codex/config.tomlrestored byte-for-byte from the real one):ok:true· 7.8sok:true· 8.2sok:true· 6.1s · 0 frame-header errors, 19 debug lines on stderrBefore/after on the same instance and same task: the prior build returned
{"ok":false,"code":"advisor-failed","message":"failed to load configuration: invalid transport in mcp_servers.\"codex_apps\""}; the fixed build returned{"ok":true,...}.~/.codex/config.tomland~/.stave/local-mcp.jsonchecksums were identical before and after, confirming the plugin disables do not leak into user global config.Notes
bun run typecheckdoes not coverelectron/—tsconfig.json'sincludeissrconly. Theelectron/changes were verified separately under a temporary config; per-file error counts match the baseline exactly (208 pre-existing, 0 added). The coverage gap is a repository-level issue and is left out of scope here.electron/main/runtime-profile.tscomputes a-devuserData path and then discards it, sodev:desktopruns against production userData with no single-instance lock. Manual verification had to launch build output with explicit isolation flags to work around it.