feat(broker): MCP client manager — connect, discover, callTool (M5a-1)#50
Conversation
New node-only package: the MCP CLIENT manager. Connects outward to operator-declared MCP servers, discovers their tools into a namespaced ToolManifest (with a stable anti-rug-pull hash), and calls them behind a narrow API. - config.ts: operator-authored connectors config (loadConnectorsConfig + parseConnectorsConfig). env values are process-env NAMES, never literals; validated and never echoed. - names.ts: connector:tool ids + provider-safe connector__tool names, both inside a 64-char budget; collisions fail loud. - manifest.ts: ToolManifest + deterministic hash; readOnlyHint absent => mutation (fail-safe). - broker.ts: lazy connect, pooled/warm clients, capped-backoff reconnect, stdio + Streamable-HTTP (SSE fallback) transports, callTool timeout + isError normalization. - fixture/: in-repo fake MCP server (stdio child + in-process HTTP), CI needs no network. - Depends only on @modelcontextprotocol/sdk + @boardstate/server (types only). Closes #38
…, changeset, lockfile)
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 46 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughChangesThe PR adds the new MCP broker
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Host
participant McpBroker
participant Transport
participant MCPServer
Host->>McpBroker: listTools() or callTool()
McpBroker->>Transport: connect with retry/backoff
Transport->>MCPServer: initialize and request tools
MCPServer-->>McpBroker: tool manifest or call result
McpBroker-->>Host: normalized result or typed error
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
evaOS review status: stale headPR: #50 - feat(broker): MCP client manager — connect, discover, callTool (M5a-1) evaOS review stopped because this queued head is no longer the live PR head. Automation note: agents should wait for this comment to reach PR URL: #50 |
evaOS review status: stale headPR: #50 - feat(broker): MCP client manager — connect, discover, callTool (M5a-1) evaOS review stopped because this queued head is no longer the live PR head. Automation note: agents should wait for this comment to reach PR URL: #50 |
![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: c837db16df
ℹ️ 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".
| const url = new URL(connector.url as string); | ||
| const headers: Record<string, string> = {}; | ||
| for (const [key, value] of Object.entries(connector.headers ?? {})) { | ||
| headers[key] = resolveHeaderValue(connector.name, key, value, env); |
There was a problem hiding this comment.
In defaultTransportFactory, HTTP header interpolation is performed against the raw process env instead of the connector's env reference map. With the documented/accepted config shape headers: { Authorization: "Bearer ${PD_TOKEN}" }, env: { PD_TOKEN: "OFFICE_PD_TOKEN" }, this looks for process.env.PD_TOKEN and fails even when OFFICE_PD_TOKEN is set, making authenticated HTTP connectors configured through the intended indirection unusable.
Useful? React with 👍 / 👎.
| /** Discover one connector's tools (connecting lazily). */ | ||
| private async discover(name: string): Promise<DiscoveredTool[]> { | ||
| const client = await this.ensureConnected(name); | ||
| const result = await client.listTools(); |
There was a problem hiding this comment.
For MCP servers that paginate tools/list, this reads only the first response and drops every page behind result.nextCursor; the protocol's tools/list operation supports cursor/nextCursor pagination. A connector with more tools than the server's page size will build an incomplete manifest and hash, so later tools are neither exposed nor covered by the grant snapshot.
Useful? React with 👍 / 👎.
…must re-pend Adversarial verify refuted the anti-rug-pull claim: readOnlyHint never entered the hashed tuples, so a tool silently becoming a mutation kept its grant. The classification decides direct-execute vs pending-action downstream; it is part of the callable surface. Regression test pins the flip.
…state into m5/broker-core
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/broker/src/broker.ts`:
- Around line 285-296: Update the catch-all handling around the client.callTool
invocation in the broker tool-call method so only responses explicitly marked
isError are represented as BrokerToolError, while transport, protocol,
connector, and other unexpected failures use BrokerConnectError or a new
BrokerCallError. Preserve the existing timeout and BrokerError passthrough
behavior, and update the relevant error construction and types in errors.ts so
callers can distinguish tool-level failures from communication failures.
- Around line 309-321: Update Broker.close and ensureConnected so shutdown
coordinates with in-flight connections: await each runtime.connecting promise,
close any client it produces, and prevent its completion handler from assigning
runtime.client after close begins. Preserve idempotency and ensure clients
already connected are also closed before close() resolves.
- Around line 262-268: listTools() currently awaits each runtime discovery
sequentially, unnecessarily serializing connector network requests. Refactor
listTools() to start discover(name) for all entries in this.runtimes
concurrently, await the collection with Promise.all, and populate the discovered
map while preserving each runtime name and result before calling
buildManifest().
- Around line 208-243: In connectWithBackoff, add a per-attempt timeout around
client.connect(transport), using the configured timeout value and a deadline
mechanism such as Promise.race; on expiry, reject with a descriptive timeout
error so the existing catch closes the client, records lastError, and continues
retries and transport fallback. Ensure the timeout is cleaned up when connect
completes.
In `@packages/broker/src/config.ts`:
- Around line 137-146: Header values currently allow literal secrets, unlike the
env-reference-only discipline. Update the headers validation in the connector
configuration parsing logic to reject literal values for authentication-related
headers unless they consist solely of a valid ${ENV_NAME} reference; preserve
the existing string-map validation for other headers and use BrokerConfigError
with the connector name for invalid auth header values.
- Around line 15-20: Update ENV_REF_PATTERN and the related validation in
config.ts to enforce a maximum identifier length consistent with
CONNECTOR_NAME_PATTERN’s 1–64 character limit, while preserving the existing
allowed characters and first-character rules. Ensure all env-reference
validation paths, including the logic around the affected lines, use this
bounded pattern so over-long secret-like literals are rejected before
resolveEnvRefs or resolveHeaderValue can echo them in BrokerConnectError
messages.
In `@packages/broker/src/names.test.ts`:
- Line 54: Update the comment near the sanitization collision case to state that
`_` remains a legal character while `.` is replaced with `_`, causing both `x.y`
and `x_y` to sanitize to `x_y`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a1fa22e0-2e30-4986-832f-12dccc82fb86
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (20)
.changeset/broker-mcp-client-manager.mdpackages/broker/package.jsonpackages/broker/src/broker.test.tspackages/broker/src/broker.tspackages/broker/src/broker.wire.test.tspackages/broker/src/config.test.tspackages/broker/src/config.tspackages/broker/src/errors.tspackages/broker/src/fixture/fake-mcp-server.tspackages/broker/src/fixture/http-harness.tspackages/broker/src/fixture/stdio-entry.tspackages/broker/src/index.tspackages/broker/src/manifest.test.tspackages/broker/src/manifest.tspackages/broker/src/names.test.tspackages/broker/src/names.tspackages/broker/tsconfig.jsonpackages/broker/tsdown.config.tspackages/broker/vitest.config.tsvitest.config.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: CodeRabbit / Review
⚠️ CI failures not shown inline (4)
GitHub Actions: CI / 0_test (24).txt: feat(broker): MCP client manager — connect, discover, callTool (M5a-1)
Conclusion: failure
##[group]Run pnpm lint
�[36;1mpnpm lint�[0m
shell: /usr/bin/bash -e {0}
env:
PNPM_HOME: /home/runner/setup-pnpm/node_modules/.bin
##[endgroup]
$ oxlint . && prettier --check .
##[warning]Identifier 'buildWidgetApprovalsSource' is imported but never used.
Found 1 warning and 0 errors.
Finished in 25ms on 250 files with 95 rules using 4 threads.
Checking formatting...
[�[33mwarn�[39m] docs/ROADMAP.md
[�[33mwarn�[39m] Code style issues found in the above file. Run Prettier with --write to fix.
[ELIFECYCLE] Command failed with exit code 1.
##[error]Process completed with exit code 1.
GitHub Actions: CI / test (22): feat(broker): MCP client manager — connect, discover, callTool (M5a-1)
Conclusion: failure
##[group]Run pnpm lint
�[36;1mpnpm lint�[0m
shell: /usr/bin/bash -e {0}
env:
PNPM_HOME: /home/runner/setup-pnpm/node_modules/.bin
##[endgroup]
$ oxlint . && prettier --check .
##[warning]Identifier 'buildWidgetApprovalsSource' is imported but never used.
Found 1 warning and 0 errors.
Finished in 24ms on 250 files with 95 rules using 4 threads.
Checking formatting...
[�[33mwarn�[39m] docs/ROADMAP.md
[�[33mwarn�[39m] Code style issues found in the above file. Run Prettier with --write to fix.
[ELIFECYCLE] Command failed with exit code 1.
##[error]The operation was canceled.
GitHub Actions: CI / test (24): feat(broker): MCP client manager — connect, discover, callTool (M5a-1)
Conclusion: failure
##[group]Run pnpm lint
�[36;1mpnpm lint�[0m
shell: /usr/bin/bash -e {0}
env:
PNPM_HOME: /home/runner/setup-pnpm/node_modules/.bin
##[endgroup]
$ oxlint . && prettier --check .
##[warning]Identifier 'buildWidgetApprovalsSource' is imported but never used.
Found 1 warning and 0 errors.
Finished in 25ms on 250 files with 95 rules using 4 threads.
Checking formatting...
[�[33mwarn�[39m] docs/ROADMAP.md
[�[33mwarn�[39m] Code style issues found in the above file. Run Prettier with --write to fix.
[ELIFECYCLE] Command failed with exit code 1.
##[error]Process completed with exit code 1.
GitHub Actions: CI / 1_test (22).txt: feat(broker): MCP client manager — connect, discover, callTool (M5a-1)
Conclusion: failure
##[group]Run pnpm lint
�[36;1mpnpm lint�[0m
shell: /usr/bin/bash -e {0}
env:
PNPM_HOME: /home/runner/setup-pnpm/node_modules/.bin
##[endgroup]
$ oxlint . && prettier --check .
##[warning]Identifier 'buildWidgetApprovalsSource' is imported but never used.
Found 1 warning and 0 errors.
Finished in 24ms on 250 files with 95 rules using 4 threads.
Checking formatting...
[�[33mwarn�[39m] docs/ROADMAP.md
[�[33mwarn�[39m] Code style issues found in the above file. Run Prettier with --write to fix.
[ELIFECYCLE] Command failed with exit code 1.
##[error]The operation was canceled.
🧰 Additional context used
🪛 LanguageTool
.changeset/broker-mcp-client-manager.md
[style] ~26-~26: Try elevating your writing by using a synonym here.
Context: ...o a typed BrokerToolError. Depends only on @modelcontextprotocol/sdk and `@bo...
(ONLY_SOLELY)
🪛 markdownlint-cli2 (0.22.1)
.changeset/broker-mcp-client-manager.md
[warning] 5-5: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
🔇 Additional comments (21)
packages/broker/src/config.ts (1)
85-153: LGTM! Unknown-field rejection, transport-specific requirements, duplicate-name checks, URL validation, and file/JSON error mapping intoBrokerConfigErrorall match the test contract inconfig.test.ts.Also applies to: 160-181, 187-205
packages/broker/src/errors.ts (1)
1-75: LGTM!packages/broker/src/broker.ts (1)
1-256: LGTM! Namespace resolution, fail-safereadOnlyHint/discovery mapping, reconnect-on-drop viaonclose, andisErrornormalization all line up with the manifest/naming contracts and the test suite.Also applies to: 298-343
packages/broker/src/broker.test.ts (1)
1-183: LGTM! Solid coverage of config-only refusal, fail-safe readOnly discovery, dual-name calling, isError normalization, hard timeout, flaky-connect backoff, reconnect-after-drop, pooling, and rug-pull hash detection — matches the broker.ts contract closely.packages/broker/src/fixture/fake-mcp-server.ts (1)
1-157: LGTM!packages/broker/src/fixture/http-harness.ts (1)
1-71: LGTM!packages/broker/src/broker.wire.test.ts (1)
1-121: LGTM!packages/broker/src/names.ts (2)
1-31: LGTM!Also applies to: 52-99
33-50: 🎯 Functional CorrectnessNo issue: connector names already exclude
:
packages/broker/src/config.tslimits connector names to^[A-Za-z0-9._-]{1,64}$, soparseManifestIdcannot mis-split a config-authored connector name containing:.> Likely an incorrect or invalid review comment.packages/broker/src/manifest.ts (2)
1-5: LGTM!Also applies to: 11-70, 85-115
6-9: 🔒 Security & Privacy
readOnlyis intentionally excluded frommanifestHash. The hash tracks tool ids and canonical input schemas only;readOnlyHintis advisory scheduling metadata and its drift does not affect the grant boundary.> Likely an incorrect or invalid review comment.packages/broker/src/manifest.test.ts (1)
1-110: LGTM!packages/broker/vitest.config.ts (1)
1-6: LGTM!vitest.config.ts (1)
4-4: LGTM!.changeset/broker-mcp-client-manager.md (1)
1-28: LGTM!packages/broker/src/config.test.ts (1)
1-145: LGTM!packages/broker/src/index.ts (1)
1-42: LGTM!packages/broker/src/fixture/stdio-entry.ts (1)
1-20: LGTM!packages/broker/package.json (1)
1-46: LGTM!packages/broker/tsdown.config.ts (1)
1-10: LGTM!packages/broker/tsconfig.json (1)
1-8: LGTM!
| private async connectWithBackoff(runtime: ConnectorRuntime): Promise<Client> { | ||
| const { maxConnectAttempts, initialBackoffMs, maxBackoffMs } = this.options; | ||
| let lastError: unknown; | ||
| for (let attempt = 0; attempt < maxConnectAttempts; attempt += 1) { | ||
| if (attempt > 0) { | ||
| await sleep(Math.min(initialBackoffMs * 2 ** (attempt - 1), maxBackoffMs)); | ||
| } | ||
| // Fresh transports each attempt — a failed/closed transport can't be reused. The | ||
| // http builder returns [streamable, sse]; we try them in order (SSE fallback). | ||
| const built = this.options.transportFactory(runtime.config, this.options.env); | ||
| const transports = Array.isArray(built) ? built : [built]; | ||
| for (const transport of transports) { | ||
| const client = new Client({ name: CLIENT_NAME, version: CLIENT_VERSION }); | ||
| // A transport drop after a successful connect evicts the warm client so the NEXT | ||
| // use reconnects (rather than calling a dead client). | ||
| client.onclose = () => { | ||
| if (runtime.client === client) { | ||
| runtime.client = undefined; | ||
| } | ||
| }; | ||
| try { | ||
| await client.connect(transport); | ||
| return client; | ||
| } catch (error) { | ||
| lastError = error; | ||
| await client.close().catch(() => {}); | ||
| } | ||
| } | ||
| } | ||
| throw new BrokerConnectError( | ||
| `connector "${runtime.config.name}" failed to connect after ${maxConnectAttempts} attempt(s): ${ | ||
| lastError instanceof Error ? lastError.message : String(lastError) | ||
| }`, | ||
| { cause: lastError }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does @modelcontextprotocol/sdk Client.connect() accept a timeout option in v1.29.0?
💡 Result:
In the @modelcontextprotocol/sdk version 1.29.0, the Client.connect method does not accept a timeout option [1][2][3][4]. The Client.connect(transport) method in v1.x is used to establish the connection and perform the initial MCP handshake, but its signature accepts only the transport object [1][4]. While the SDK does support timeouts for individual protocol requests (such as callTool), these are configured when calling those specific methods (e.g., passing a timeout option to callTool), not during the initial connection [5][6]. Please note that the documentation for the newer MCP TypeScript SDK v2 may show different API signatures [1][2][3]; ensure you are referencing the documentation specific to the v1.x branch if you are using version 1.29.0 [7][8].
Citations:
- 1: https://ts.sdk.modelcontextprotocol.io/v2/clients/connect.html
- 2: https://ts.sdk.modelcontextprotocol.io/v2/api/@modelcontextprotocol/client/client/client.html
- 3: https://ts.sdk.modelcontextprotocol.io/v2/classes/_modelcontextprotocol_client.client_client.Client.html
- 4: https://ts.sdk.modelcontextprotocol.io/documents/client.html
- 5: https://ts.sdk.modelcontextprotocol.io/v2/documents/Documents.Client_Guide.html
- 6: https://github.com/modelcontextprotocol/typescript-sdk/blob/HEAD/docs/client.md
- 7: https://github.com/modelcontextprotocol/typescript-sdk/releases/tag/v1.29.0
- 8: chore: bump version to 1.29.0 modelcontextprotocol/typescript-sdk#1820
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect relevant broker code and nearby call sites.
git ls-files 'packages/broker/src/broker.ts' 'packages/broker/src/**/*.ts' | sed -n '1,20p'
echo
echo '--- broker.ts outline ---'
ast-grep outline packages/broker/src/broker.ts --view expanded || true
echo
echo '--- connectWithBackoff / connect usages ---'
rg -n "connectWithBackoff|client\.connect\(|callTool\(|ensureConnected|timeout" packages/broker/src/broker.ts packages/broker/src -g '!**/*.map' || true
echo
echo '--- broker.ts excerpt ---'
sed -n '180,320p' packages/broker/src/broker.tsRepository: 100yenadmin/boardstate
Length of output: 11690
Add a per-attempt connect timeout
packages/broker/src/broker.ts:229client.connect(transport)can block indefinitely during the initial handshake; thecallTool()timeout does not cover this path, so a black-holed endpoint can stallensureConnected()and stop retries from ever running.@modelcontextprotocol/sdkv1.29.0 does not expose aconnecttimeout option here, so wrap the connect attempt in a deadline (for examplePromise.race) or a transport-level timeout.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/broker/src/broker.ts` around lines 208 - 243, In connectWithBackoff,
add a per-attempt timeout around client.connect(transport), using the configured
timeout value and a deadline mechanism such as Promise.race; on expiry, reject
with a descriptive timeout error so the existing catch closes the client,
records lastError, and continues retries and transport fallback. Ensure the
timeout is cleaned up when connect completes.
| async listTools(): Promise<ToolManifest> { | ||
| const discovered = new Map<string, DiscoveredTool[]>(); | ||
| for (const name of this.runtimes.keys()) { | ||
| discovered.set(name, await this.discover(name)); | ||
| } | ||
| return buildManifest(discovered); | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
listTools() discovers connectors sequentially — consider parallelizing.
for (const name of this.runtimes.keys()) {
discovered.set(name, await this.discover(name));
}
Each connector's connect+listTools() round-trip is awaited before starting the next. With several configured connectors this serializes N network round-trips where they could run concurrently.
♻️ Proposed refactor
- const discovered = new Map<string, DiscoveredTool[]>();
- for (const name of this.runtimes.keys()) {
- discovered.set(name, await this.discover(name));
- }
+ const names = [...this.runtimes.keys()];
+ const results = await Promise.all(names.map((name) => this.discover(name)));
+ const discovered = new Map(names.map((name, i) => [name, results[i]]));
return buildManifest(discovered);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async listTools(): Promise<ToolManifest> { | |
| const discovered = new Map<string, DiscoveredTool[]>(); | |
| for (const name of this.runtimes.keys()) { | |
| discovered.set(name, await this.discover(name)); | |
| } | |
| return buildManifest(discovered); | |
| } | |
| async listTools(): Promise<ToolManifest> { | |
| const names = [...this.runtimes.keys()]; | |
| const results = await Promise.all(names.map((name) => this.discover(name))); | |
| const discovered = new Map(names.map((name, i) => [name, results[i]])); | |
| return buildManifest(discovered); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/broker/src/broker.ts` around lines 262 - 268, listTools() currently
awaits each runtime discovery sequentially, unnecessarily serializing connector
network requests. Refactor listTools() to start discover(name) for all entries
in this.runtimes concurrently, await the collection with Promise.all, and
populate the discovered map while preserving each runtime name and result before
calling buildManifest().
| let result: Awaited<ReturnType<Client["callTool"]>>; | ||
| try { | ||
| result = await client.callTool({ name: tool, arguments: args }, undefined, { timeout }); | ||
| } catch (error) { | ||
| if (error instanceof McpError && error.code === ErrorCode.RequestTimeout) { | ||
| throw new BrokerTimeoutError(`tool "${id}" timed out after ${timeout}ms`); | ||
| } | ||
| if (error instanceof BrokerError) { | ||
| throw error; | ||
| } | ||
| throw new BrokerToolError(id, error instanceof Error ? error.message : String(error)); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Catch-all wraps arbitrary call failures as BrokerToolError, contradicting its own docstring.
BrokerToolError's doc contract (errors.ts) is "a tool call the server answered with isError: true." But here (Line 295), ANY error from client.callTool() that isn't an McpError/timeout or an existing BrokerError — including transport drops mid-call, protocol errors (e.g. unknown-method), or a crashed connector — gets funneled into BrokerToolError too. Callers switching on BrokerToolError to mean "the tool itself failed" will misclassify connection/protocol failures as tool-level failures.
Consider reusing BrokerConnectError (or a new BrokerCallError) for the non-timeout, non-isError branch so consumers can distinguish "the server explicitly said isError" from "something broke talking to the server."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/broker/src/broker.ts` around lines 285 - 296, Update the catch-all
handling around the client.callTool invocation in the broker tool-call method so
only responses explicitly marked isError are represented as BrokerToolError,
while transport, protocol, connector, and other unexpected failures use
BrokerConnectError or a new BrokerCallError. Preserve the existing timeout and
BrokerError passthrough behavior, and update the relevant error construction and
types in errors.ts so callers can distinguish tool-level failures from
communication failures.
| /** Close every warm client. Idempotent; safe to call in a `finally`. */ | ||
| async close(): Promise<void> { | ||
| const closing: Promise<void>[] = []; | ||
| for (const runtime of this.runtimes.values()) { | ||
| const client = runtime.client; | ||
| runtime.client = undefined; | ||
| runtime.connecting = undefined; | ||
| if (client) { | ||
| closing.push(client.close().catch(() => {})); | ||
| } | ||
| } | ||
| await Promise.all(closing); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
close() doesn't wait for in-flight connects — a client can leak after close() resolves.
If a connector has a connect in flight (runtime.connecting set, runtime.client still undefined), close() (Lines 313-315) just discards the reference — it never awaits runtime.connecting or closes whatever client eventually results. When that pending connectWithBackoff(...) later resolves, its .then handler in ensureConnected (Lines 193-198) unconditionally does runtime.client = client;, resurrecting a live client (an open stdio child process or HTTP/SSE connection) on a broker that the caller believes is fully closed. Nothing will ever close it.
This is exploitable simply by calling close() while a listTools()/callTool()-triggered connect is still pending (e.g. during shutdown).
🔧 Proposed fix
async close(): Promise<void> {
const closing: Promise<void>[] = [];
for (const runtime of this.runtimes.values()) {
+ const pending = runtime.connecting;
const client = runtime.client;
runtime.client = undefined;
runtime.connecting = undefined;
if (client) {
closing.push(client.close().catch(() => {}));
}
+ if (pending) {
+ closing.push(pending.then((c) => c.close().catch(() => {}), () => {}));
+ }
}
await Promise.all(closing);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** Close every warm client. Idempotent; safe to call in a `finally`. */ | |
| async close(): Promise<void> { | |
| const closing: Promise<void>[] = []; | |
| for (const runtime of this.runtimes.values()) { | |
| const client = runtime.client; | |
| runtime.client = undefined; | |
| runtime.connecting = undefined; | |
| if (client) { | |
| closing.push(client.close().catch(() => {})); | |
| } | |
| } | |
| await Promise.all(closing); | |
| } | |
| /** Close every warm client. Idempotent; safe to call in a `finally`. */ | |
| async close(): Promise<void> { | |
| const closing: Promise<void>[] = []; | |
| for (const runtime of this.runtimes.values()) { | |
| const pending = runtime.connecting; | |
| const client = runtime.client; | |
| runtime.client = undefined; | |
| runtime.connecting = undefined; | |
| if (client) { | |
| closing.push(client.close().catch(() => {})); | |
| } | |
| if (pending) { | |
| closing.push(pending.then((c) => c.close().catch(() => {}), () => {})); | |
| } | |
| } | |
| await Promise.all(closing); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/broker/src/broker.ts` around lines 309 - 321, Update Broker.close
and ensureConnected so shutdown coordinates with in-flight connections: await
each runtime.connecting promise, close any client it produces, and prevent its
completion handler from assigning runtime.client after close begins. Preserve
idempotency and ensure clients already connected are also closed before close()
resolves.
| // We validate that every env value is a syntactically valid env-var reference | ||
| // (`^[A-Za-z_][A-Za-z0-9_]*$`). This is a fail-safe, not a secret detector: a literal | ||
| // that merely LOOKS like an identifier can't be told apart from a real reference, so we | ||
| // document the contract and reject everything that is obviously not a reference (spaces, | ||
| // `-`, `.`, `/`, `=`, `+`, over-long strings — the shapes real tokens take). Resolution | ||
| // happens later, in the broker, and a resolved value is never echoed into an error/log. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
ENV_REF_PATTERN doesn't actually reject "over-long strings" as documented.
The header comment (Lines 15-20) explicitly lists "over-long strings" as one of the shapes rejected by the env-ref fail-safe, but ENV_REF_PATTERN (Line 56) has no length bound — it accepts any string matching ^[A-Za-z_][A-Za-z0-9_]*$, however long. Many real secret formats (e.g. ghp_<40 hex chars>, other underscore-delimited API keys) are pure [A-Za-z0-9_] and will pass this check as if they were env-var names.
The downstream impact is worse than a bypassed fail-safe: in broker.ts, resolveEnvRefs/resolveHeaderValue do env[sourceName] and, when the lookup misses, throw BrokerConnectError with a message that embeds sourceName verbatim — i.e. the "reference" is echoed straight into the error text. If that reference is actually a mis-pasted literal secret that slipped past this regex, it gets logged/surfaced via the error, which directly violates the security invariant stated in errors.ts ("broker errors must NEVER carry a resolved env-var VALUE").
Add a length bound consistent with real env-var naming conventions (and with CONNECTOR_NAME_PATTERN's {1,64} budget) to close the gap:
🛡️ Proposed fix
-const ENV_REF_PATTERN = /^[A-Za-z_][A-Za-z0-9_]*$/;
+const ENV_REF_PATTERN = /^[A-Za-z_][A-Za-z0-9_]{0,63}$/;Also applies to: 56-56, 66-82
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/broker/src/config.ts` around lines 15 - 20, Update ENV_REF_PATTERN
and the related validation in config.ts to enforce a maximum identifier length
consistent with CONNECTOR_NAME_PATTERN’s 1–64 character limit, while preserving
the existing allowed characters and first-character rules. Ensure all
env-reference validation paths, including the logic around the affected lines,
use this bounded pattern so over-long secret-like literals are rejected before
resolveEnvRefs or resolveHeaderValue can echo them in BrokerConnectError
messages.
| if (raw.headers !== undefined) { | ||
| if ( | ||
| !isPlainObject(raw.headers) || | ||
| Object.values(raw.headers).some((v) => typeof v !== "string") | ||
| ) { | ||
| throw new BrokerConfigError(`connector "${name}": headers must be a string map`); | ||
| } | ||
| config.headers = raw.headers as Record<string, string>; | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
headers values aren't held to the same env-ref-only discipline as env.
Per Lines 6-13, the whole point of the env reference scheme is that "the config file therefore holds no secrets and is safe to commit." But headers (Lines 137-145) only requires a string map — an operator can put a literal Authorization: "Bearer sk-..." directly in headers with no validation preventing it, even though this is the most likely place a bearer token ends up. This silently breaks the "safe to commit" guarantee for HTTP connectors specifically.
Consider applying an analogous fail-safe to header values (e.g., reject headers whose value doesn't consist solely of ${ENV_NAME} interpolation for any header name that looks auth-related, or at minimum document loudly that headers literals are NOT covered by the secret-hygiene guarantee and are the operator's responsibility).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/broker/src/config.ts` around lines 137 - 146, Header values
currently allow literal secrets, unlike the env-reference-only discipline.
Update the headers validation in the connector configuration parsing logic to
reject literal values for authentication-related headers unless they consist
solely of a valid ${ENV_NAME} reference; preserve the existing string-map
validation for other headers and use BrokerConfigError with the connector name
for invalid auth header values.
| }); | ||
|
|
||
| it("fails loud when two ids collapse onto one provider name", () => { | ||
| // `x.y` and `x_y` both sanitize to `x_y` (`.` and `_` are out-of-charset → `_`). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the sanitization explanation.
The comment says both . and _ are out of charset, but the expected provider name and collision case rely on _ remaining legal; only . is replaced.
Proposed comment fix
- // `x.y` and `x_y` both sanitize to `x_y` (`.` and `_` are out-of-charset → `_`).
+ // `x.y` and `x_y` both sanitize to `x_y` (`.` is replaced with `_`; `_` is legal).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // `x.y` and `x_y` both sanitize to `x_y` (`.` and `_` are out-of-charset → `_`). | |
| // `x.y` and `x_y` both sanitize to `x_y` (`.` is replaced with `_`; `_` is legal). |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/broker/src/names.test.ts` at line 54, Update the comment near the
sanitization collision case to state that `_` remains a legal character while
`.` is replaced with `_`, causing both `x.y` and `x_y` to sanitize to `x_y`.
Adversarial invariant verify — verdict
Mergeable once CI is green. |
evaOS review status: closed or merged before reviewPR: #50 - feat(broker): MCP client manager — connect, discover, callTool (M5a-1) evaOS review stopped because the PR closed or merged before this queued head could be reviewed. Automation note: agents should wait for this comment to reach PR URL: #50 Details: state=closed |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 30f22e2959
ℹ️ 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".
| for (const runtime of this.runtimes.values()) { | ||
| const client = runtime.client; | ||
| runtime.client = undefined; | ||
| runtime.connecting = undefined; |
There was a problem hiding this comment.
When close() is called while ensureConnected() has a handshake in flight, this discards the only handle to that promise instead of waiting for it or closing the client it produces. The fulfillment path still assigns runtime.client afterward, so shutdown/abort paths can leave a stdio child or HTTP transport alive after close() has returned. Please await/close in-flight connects or mark the broker closed before dropping the promise.
Useful? React with 👍 / 👎.
| } | ||
| const out: Record<string, string> = {}; | ||
| for (const [key, value] of Object.entries(env)) { | ||
| if (typeof value !== "string" || !ENV_REF_PATTERN.test(value)) { |
There was a problem hiding this comment.
When an operator accidentally pastes an identifier-shaped secret (for example a long alphanumeric/underscore API key) into env, this regex accepts it as a reference because there is no length cap. The later missing-env error in resolveEnvRefs includes sourceName, so that secret is written into the exception despite the no-secrets-in-errors contract; bound accepted refs or avoid echoing them.
Useful? React with 👍 / 👎.
| const { connector, tool } = parseManifestId(id); | ||
| const provider = toProviderName(connector, tool); | ||
| const clash = providerToId.get(provider); | ||
| if (clash !== undefined && clash !== id) { |
There was a problem hiding this comment.
If a connector's tools/list response contains the same tool name twice, both entries get the same id/providerName; buildProviderNameMap only rejects clashes where the existing id differs, so the manifest can advertise duplicate provider tool names and later provider requests can fail. Treat repeated ids from one connector as a collision instead of emitting both entries.
Useful? React with 👍 / 👎.
| if (value !== null && typeof value === "object") { | ||
| const out: Record<string, unknown> = {}; | ||
| for (const key of Object.keys(value as Record<string, unknown>).sort()) { | ||
| out[key] = canonicalize((value as Record<string, unknown>)[key]); |
There was a problem hiding this comment.
When a server exposes a tool whose JSON Schema includes a property named __proto__ (for example under properties), assigning it into a normal object invokes the prototype setter instead of creating an own key. The canonicalized schema then serializes as if that property were absent, so adding/removing/changing that parameter can leave manifest.hash unchanged and bypass the grant rug-pull check; build the accumulator with a null prototype or define own properties explicitly.
Useful? React with 👍 / 👎.
1 participant
Closes #38
Summary
New node-only package
@boardstate/broker— the MCP client manager (epic #37, M5a-1). It connects outward to operator-declared external MCP servers, discovers their tools into a namespacedToolManifest, and calls them behind a narrow API the host/server layers consume. This is the substrate later M5 issues build on.What's inside
config.ts— operator-authored connectors config (invariant M3 — Reference app: plug in GLM (flagship) #8):loadConnectorsConfig(path)+ programmaticparseConnectorsConfig/new McpBroker(config). A connector name not in the config is inert.envvalues are process-env var NAMES (references), validated up front and never echoed into errors/logs.names.ts—connector:toolmanifest ids + provider-safeconnector__toolnames (invertstoAgentToolName), both inside a 64-char budget; sanitization collisions fail loud (BrokerNameCollisionError).manifest.ts—ToolManifest+ a deterministic manifest hash over sorted (id + canonical input-schema) pairs (the anti-rug-pull anchor).readOnlyHintabsent ⇒ treated as a mutation (fail-safe, mirrorsAgentTool.readOnlyin server/host.ts).broker.ts—McpBroker: lazy connect on first use, warm/pooled clients, capped exponential-backoff reconnect,StdioClientTransport+StreamableHTTPClientTransportwith SSE fallback;callTool(id, args, {timeout})resolves the client, strips the namespace, enforces a hard timeout, and normalizesisError: trueinto a typedBrokerToolError. Accepts either aconnector:toolid or a provider-safe name.fixture/— in-repo fake MCP server usable two ways (stdio child + in-process Streamable HTTP on loopback); readOnly + mutating (hint-less) +isError+sleeptools, runtime rug-pull toggle. CI needs no network.Dependency boundary (epic invariant)
Depends only on
@modelcontextprotocol/sdkand@boardstate/server(types only —import type, erased fromdist, verified absent from built JS). No import of core/host/lit/schema.Test evidence (scoped gates, local — repo is on LEXAR)
pnpm --filter @boardstate/broker test→ 37 passed / 5 files (config, names, manifest, broker unit, wire-contract).pnpm typecheck→ clean.oxlint packages/broker→ clean.prettier --checkon touched files → clean.Notes for the shepherd
docs/ROADMAP.mdfailsprettier --checkat the branch point (49ebafd) — it is not in this diff. Left untouched (orchestrator-owned roadmap doc); flag if the CI lint job needs it reformatted separately.vitest.config.tsgains"broker"in the node-project list sopnpm test(explicit project list) runs the suite in CI.Summary by CodeRabbit