Skip to content

fix(config): retain numeric MCP timeout entries - #50911

Closed
Veld101 wants to merge 1 commit into
anomalyco:devfrom
Veld101:fix-mcp-timeout
Closed

Veld101 wants to merge 1 commit into
anomalyco:devfrom
Veld101:fix-mcp-timeout

Conversation

@Veld101

@Veld101 Veld101 commented Sep 23, 2026

Copy link
Copy Markdown

Issue for this PR

Closes #50807

Type of change

  • Bug fix
  • New feature
  • Refactor / code improvement
  • Documentation

What does this PR do?

A timeout on an MCP server entry was taking down more than the timeout.

The V2-to-V1 compatibility layer only accepted the legacy { catalog, execution } shape, which broke two things:

  • A plain numeric timeout on a server failed decodeValue(Server, ...), so the entire server was dropped from the normalized config. That is the report: the 5s default could not be raised, and slow tools/list handshakes kept timing out.
  • A numeric global mcp.timeout was worse. decodeRecord(60000) returned nothing, so timeout was treated as a server named "timeout" and the resulting config failed V1 validation with ConfigInvalidError.

request is the current field on the V2 schema (packages/core/src/config/mcp.ts), and it is what migrateMcp writes when it converts a V1 number, so I accepted it alongside the legacy keys instead of only handling the number.

Changes:

  • Timeout is now Union([PositiveInt, Struct{ startup?, request?, catalog?, execution? }]).
  • lowerTimeout passes a number through, prefers request, and keeps the existing catalog/execution behavior.
  • A numeric mcp.timeout is recognized as the global timeout rather than being treated as a server entry.

I followed the runtime path as well: packages/opencode/src/mcp/index.ts reads the per-server timeout for McpCatalog.defs (tool discovery) and falls back to experimental.mcp_timeout, so the lowered number does reach listTools(..., { timeout }).

How did you verify your code works?

  • Wrote the tests first and confirmed all four fail without the source change, including the end-to-end one.
  • bun test test/config/v2-compat.test.ts test/config/config.test.ts --timeout 30000 from packages/opencode — 147 pass, 0 fail; existing snapshot fixtures unchanged.
  • bun typecheck from packages/opencode — clean.
  • oxlint on the two changed files — 0 errors (3 pre-existing warnings in untouched functions).

The new tests cover a numeric per-server timeout, a { request } per-server timeout, a numeric global timeout, and a config file loaded through Config.use.get().

I did not test against a real remote MCP server with a slow handshake, since I do not have one. The behavior under test is the config normalization, which is where the server was being lost.

Screenshots / recordings

Not a UI change.

Checklist

  • I have tested my changes locally
  • I have not included unrelated changes in this PR

V2-to-V1 normalization dropped any MCP server whose timeout was a plain number, because the compat Timeout schema only accepted the legacy {catalog, execution} struct. A numeric global mcp.timeout was worse: it was treated as a server entry and failed the whole config with ConfigInvalidError.

Accept PositiveInt alongside the struct, lower a numeric or {request} timeout to the V1 number, and treat a numeric global timeout as the global timeout.
@github-actions

Copy link
Copy Markdown
Contributor

The following comment was made by an LLM, it may be inaccurate:

@kvnloo

kvnloo commented Sep 25, 2026

Copy link
Copy Markdown

I checked the compatibility-lowering change on this head. The intended mappings are present for the three important cases:

  • numeric per-server timeout stays numeric
  • request-shaped timeout lowers to its request value
  • global timeout lowers to experimental.mcp_timeout

The invariant looks right by source/test inspection: V2 MCP timeout configuration should not silently disappear during V2 → V1 lowering.

I haven't independently run the full v2-compat test suite here, so treating this as source-level verification rather than behavioral proof.

@Veld101

Veld101 commented Sep 28, 2026

Copy link
Copy Markdown
Author

Superseded by #51853 on �2. dev is not where merges land (recent merges are overwhelmingly into v2), so this fix is carried in the v2 PR instead. Closing to keep the queue focused; reopen or cherry-pick from that branch if needed.

@Veld101 Veld101 closed this Sep 28, 2026
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.

mcp: server entries with a "timeout" field are silently dropped from the normalized config

2 participants