Skip to content

fix: MCP pool stability and auth schema edge cases - #1149

Open
Fahremback wants to merge 1 commit into
CodebuffAI:mainfrom
Fahremback:fix/mcp-pool-and-auth
Open

fix: MCP pool stability and auth schema edge cases#1149
Fahremback wants to merge 1 commit into
CodebuffAI:mainfrom
Fahremback:fix/mcp-pool-and-auth

Conversation

@Fahremback

Copy link
Copy Markdown

This splits out the non-controversial fixes from #1148 as requested. Includes the client-pool timeout/dedup logic + tests, and defensive schema updates for auth.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@codebuff-team

Copy link
Copy Markdown
Contributor

The MCPClientPool addition (common/src/mcp/client-pool.ts) is the strong part of this PR: clean dedup-on-connect logic, timeout handling via withTimeout, and a leak-avoidance path that closes a stale connection if it resolves after a timeout fires. The four tests in client-pool.test.ts cover the real edge cases — concurrent dedup, retry-after-failure, partial/full close, and timeout-then-retry — and they read as if they'd actually catch a regression, not just pad coverage. This part is ready to be ported.

Two things worth tightening before/at port time:

  1. This diff adds the pool but doesn't wire it into any existing MCP connection path. That's fine if the integration lives in the parent PR (feat: Hermes architecture, Agentic UI, and CLI optimizations #1148) and this really is just the isolated primitive, but as a standalone diff there's no way to see it's actually exercised by real callers. Worth saying explicitly in the PR body which call site will consume this.

  2. cli/src/utils/auth.ts: relaxing name/email from z.string() to z.string().nullish() is a real behavior change with no test and no explanation of what response was failing validation. "Defensive" schema loosening on auth data is exactly the kind of change that can silently paper over a real upstream bug (e.g., a provider returning malformed user info) rather than fix it. Please include a repro or a comment on which auth flow produces null/missing name or email, and ideally a regression test in the same style as the pool tests.

Net: the pool code is a clean, mergeable-quality contribution. The auth.ts hunk is small but needs justification.

@codebuff-team codebuff-team added bot:triaged Classified by the community triage bot pr:port-candidate Worth porting into the private source tree labels Aug 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot:triaged Classified by the community triage bot pr:port-candidate Worth porting into the private source tree

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants