Skip to content

fix: start the title thread with the configured MCP servers disabled - #661

Open
Pearcekieser wants to merge 1 commit into
agentclientprotocol:mainfrom
Pearcekieser:fix/title-thread-mcp-servers
Open

Pearcekieser wants to merge 1 commit into
agentclientprotocol:mainfrom
Pearcekieser:fix/title-thread-mcp-servers

Conversation

@Pearcekieser

Copy link
Copy Markdown

Problem

TitleGenerator.generateAndPersist starts its ephemeral thread with threadStart({cwd, ephemeral: true}) and no config. Codex then loads the user's config.toml and starts every server in [mcp_servers] for that thread. The title turn uses an output schema and no tools, so those processes do nothing. A user with ten configured servers pays ten extra processes per titled session.

#629 releases the title thread after the turn, so these processes exit 60 s later instead of living until codex-acp exits. It does not stop them from starting. This PR is the other half and does not depend on #629.

Change

  • Before the ephemeral thread/start, TitleGenerator reads the effective config for the session's cwd with config/read and passes config: {mcp_servers: {<name>: {enabled: false}, ...}} for every configured name.
  • With no configured servers, or when config/read fails, the thread starts as before, with no config. Title generation stays best-effort.
  • The names come from the effective config only. Codex merges the thread config onto the loaded layers, so mcp_servers: {} disables nothing. Disabling a name no layer defines fails the whole thread/start with invalid transport in mcp_servers.<name>, so a hard-coded list or a union of all layers would be unsafe.

Tests

  • TitleGenerator.test.ts: the title thread is started with each configured server disabled, with no config when none is configured, and with no config when config/read fails. The first test fails without the change.
  • npm run typecheck and npm run build pass. npm test: 1869 passed, 36 skipped, 1 failed. The failure is JsonSnapshot.test.ts > leaves values nested thousands deep to JSON.stringify, which fails the same way on main (636b082) on this host.
  • Real Codex 0.160.1 against a local fake Responses provider, with 2 recording MCP servers in config.toml and 3 sessions on one codex-acp process. Each session sent session/new with a session MCP server, one prompt and session/close:
build title-thread MCP starts left after all sessions closed (+75 s) titles written
main 6 6, until codex-acp exits 3
#629 6 0, each exits 60 s after the title turn 3
this PR 0 0 3
this PR + #629 0 0 3

In every run the main thread's servers started and stopped the same way, 60 s after session/close.

  • Direct codex app-server probe of thread/start {ephemeral: true} with 2 configured servers: no config started 2, {"mcp_servers": {}} started 2, nested {name: {enabled: false}} started 0, the dotted mcp_servers.<name>.enabled form started 0, and an undefined name failed thread/start with -32600.

Related to #628 and #629.

@VitaliBabkin

Copy link
Copy Markdown

This is also a correctness and isolation problem, on top of the resource cost. The title thread ignores the session config, so it starts MCP servers the client explicitly disabled.

What we see

Our ACP client launches codex-acp with CODEX_CONFIG containing mcp_servers.<name>.enabled = false for a server that must not run inside a Codex session. In our case that server is the client itself: it is an MCP server that can open agent sessions, and running it inside the agent would let the agent spawn agents recursively. The main thread honours this, because newSession passes config to thread/start. The title thread does not. TitleGenerator.generateAndPersist calls threadStart({ cwd, ephemeral: true }), so the disabled server starts from config.toml anyway, gets initialize from codex-mcp-client, and stays up for the life of the codex-acp process (#628).

Reproduced

Reproduced on codex-acp 2.6.0 with bundled Codex 0.166.0, on Windows 11:

  1. session/new (lean or standard config), then one prompt.
  2. No disabled server is running during the turn.
  3. On turn/completed, codex-acp sends thread/start and turn/start for a new ephemeral thread. In the same second, the app-server log shows the disabled server's startup line, and the process appears as a child of codex.exe.
  4. It is never stopped while the session lives.

A client that sets the title explicitly via session/set_title right after session/new avoids it, because the generator is then skipped. That is our workaround for now. Clients that don't know about this get the disabled servers started silently.

This PR fixes the startup half, and #629 fixes the lifetime half. Could a maintainer take a look at both? Something the client disabled should not start anywhere in the session, and that seems worth a patch release.

This branch has not been deployed

No deployments
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.

2 participants