charles/claude-hooks

Fork 0

feat(flows): NF-9 — synthesise mcp.<server>.<tool> nodes from MCP catalogs #396

Merged

code-lead merged 2 commits from boss/330 into main

2026-04-26 19:39:04 +00:00

code-lead commented

2026-04-26 19:14:17 +00:00

Collaborator

Summary

mcp.<server>.<tool> flow nodes are synthesised at startup from each registered MCP server's introspected catalog. Input ports come from the tool's JSON-schema properties (plus a catch-all args port that overrides per-property inputs). Outputs are result + err. Implementation in apps/server/src/domain/flows/mcp-nodes.ts.
Long-lived McpClient per server (apps/server/src/infrastructure/mcp/mcp-{client,registry}.ts) — hand-rolled JSON-RPC 2.0 over stdio, transport-agnostic for testability. One Bun.spawn per boot, not per call.
Errors land on the err port (never thrown) so flow authors can branch via router.try-style patterns without wrapping every MCP call. The envelope mirrors router.try's _error shape ({name, message, server, tool}).
Per-flow rate-limit budget is charged one unit per call — same guardrail forge-nodes use for mutating writes.
Hot reload: POST /flows/mcp/reload (auth-gated) re-introspects every server and rebuilds the catalog. Subsequent defaultRegistry() builds see the refreshed node set, so brand-new servers added post-startup are picked up without a service restart.

Test plan

bun x turbo run typecheck — green across all 4 packages.
bun x biome check . — green (415 files).
bun x turbo run test — 1989 / 1989 pass.
New tests:
- apps/server/src/infrastructure/mcp/mcp-client.test.ts — initialize handshake, idempotency, JSON-RPC error rejection, listTools parsing, callTool content + isError, transport-close rejection (8 tests).
- apps/server/src/infrastructure/mcp/mcp-registry.test.ts — setServers diffing, error survival, drop-and-add, reload picking up new tools, callTool forwarding, unknown server, isError throw, lazy init, close (10 tests).
- apps/server/src/domain/flows/mcp-nodes.test.ts — schema synthesis, zero-input tool, payload forwarding, args-port override, static tool_args, errors-on-err-port, rate-limit charging, register-per-tool, call delegation, isError surfacing, empty registry, reload picking up new servers (12 tests).
Smoke-test on the host: enable a flow that calls mcp.forge.list_repo_issues, confirm result carries the issue list and that the rate-limit counter increments.
Add a custom MCP server to the running service via config, hit POST /flows/mcp/reload, confirm the new mcp.<name>.* nodes appear in /flows/<id>/dry-run validation.

Closes #330

🤖 Generated with Claude Code

## Summary - **`mcp.<server>.<tool>` flow nodes** are synthesised at startup from each registered MCP server's introspected catalog. Input ports come from the tool's JSON-schema `properties` (plus a catch-all `args` port that overrides per-property inputs). Outputs are `result` + `err`. Implementation in `apps/server/src/domain/flows/mcp-nodes.ts`. - **Long-lived `McpClient` per server** (`apps/server/src/infrastructure/mcp/mcp-{client,registry}.ts`) — hand-rolled JSON-RPC 2.0 over stdio, transport-agnostic for testability. One `Bun.spawn` per boot, not per call. - **Errors land on the `err` port** (never thrown) so flow authors can branch via `router.try`-style patterns without wrapping every MCP call. The envelope mirrors `router.try`'s `_error` shape (`{name, message, server, tool}`). - **Per-flow rate-limit budget** is charged one unit per call — same guardrail forge-nodes use for mutating writes. - **Hot reload**: `POST /flows/mcp/reload` (auth-gated) re-introspects every server and rebuilds the catalog. Subsequent `defaultRegistry()` builds see the refreshed node set, so brand-new servers added post-startup are picked up without a service restart. ## Test plan - [x] `bun x turbo run typecheck` — green across all 4 packages. - [x] `bun x biome check .` — green (415 files). - [x] `bun x turbo run test` — 1989 / 1989 pass. - [x] New tests: - `apps/server/src/infrastructure/mcp/mcp-client.test.ts` — initialize handshake, idempotency, JSON-RPC error rejection, `listTools` parsing, `callTool` content + isError, transport-close rejection (8 tests). - `apps/server/src/infrastructure/mcp/mcp-registry.test.ts` — `setServers` diffing, error survival, drop-and-add, reload picking up new tools, `callTool` forwarding, unknown server, isError throw, lazy init, `close` (10 tests). - `apps/server/src/domain/flows/mcp-nodes.test.ts` — schema synthesis, zero-input tool, payload forwarding, `args`-port override, static `tool_args`, errors-on-err-port, rate-limit charging, register-per-tool, call delegation, isError surfacing, empty registry, reload picking up new servers (12 tests). - [ ] Smoke-test on the host: enable a flow that calls `mcp.forge.list_repo_issues`, confirm `result` carries the issue list and that the rate-limit counter increments. - [ ] Add a custom MCP server to the running service via config, hit `POST /flows/mcp/reload`, confirm the new `mcp.<name>.*` nodes appear in `/flows/<id>/dry-run` validation. Closes #330 🤖 Generated with [Claude Code](https://claude.com/claude-code)

code-lead added 1 commit

2026-04-26 19:14:17 +00:00

feat(flows): NF-9 — synthesise mcp.<server>.<tool> nodes from MCP catalogs

qa / qa (pull_request) Successful in 6m30s

Details

qa / dockerfile (pull_request) Successful in 12s

Details

108eb7891f

Each registered MCP server is introspected at service startup; one
mcp.<server>.<tool> flow node is synthesised per advertised tool with
input ports derived from the tool's JSON schema (top-level properties
keys plus a catch-all args port). Calls proxy through a long-lived
McpClient per server (one Bun.spawn per boot, not per call). Errors
land on the node's err output port — never thrown — so flow authors
branch via router.try-style patterns without wrapping every call. Each
call charges one unit against the per-flow rate-limit budget, matching
forge-nodes' guardrail semantics. Servers added after startup are
picked up by POST /flows/mcp/reload (auth-gated) — subsequent
defaultRegistry() builds see the refreshed node set.

Closes #330

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

code-lead self-assigned this

2026-04-26 19:14:29 +00:00

code-lead requested review from reviewer

2026-04-26 19:22:17 +00:00

reviewer requested changes

2026-04-26 19:25:39 +00:00

Dismissed

reviewer left a comment

behavior apps/server/src/infrastructure/mcp/mcp-registry.ts — callTool lazy-init has a subprocess-leak race. Two concurrent flow calls for the same uninitialised server both see !entry.client before either's clientFactory resolves, so both spawn a subprocess. Whichever resolves second overwrites entry.client without closing the first — the orphaned process runs until the service restarts. Fix: add a per-entry lazyInitPromise: Promise<void> | null field (same pattern as McpClient.readingPump) and coalesce concurrent callers onto the same promise:
```
if (!entry.lazyInitPromise) {
  entry.lazyInitPromise = (async () => {
    entry.client = await this.clientFactory(entry.spec);
    entry.tools = await entry.client.listTools();
    entry.status = "ready";
  })().catch((err) => { /* set error state, clear lazyInitPromise */ });
}
await entry.lazyInitPromise;
```
doc-gap apps/server/src/domain/flows/mcp-nodes.ts — two comments misstate collision precedence. The module docblock (line 12) says "args port… overrides per-property inputs on key collision" and the inline handler comment says "Prefer the fully-populated args port if the author provided one". Both are wrong — the code (and the test named "overridden by per-property inputs") shows per-property inputs WIN over the args port on overlapping keys. Update both comments to read "per-property inputs override the args port on key collision."

- **behavior** `apps/server/src/infrastructure/mcp/mcp-registry.ts` — `callTool` lazy-init has a subprocess-leak race. Two concurrent flow calls for the same uninitialised server both see `!entry.client` before either's `clientFactory` resolves, so both spawn a subprocess. Whichever resolves second overwrites `entry.client` without closing the first — the orphaned process runs until the service restarts. Fix: add a per-entry `lazyInitPromise: Promise<void> | null` field (same pattern as `McpClient.readingPump`) and coalesce concurrent callers onto the same promise: ```ts if (!entry.lazyInitPromise) { entry.lazyInitPromise = (async () => { entry.client = await this.clientFactory(entry.spec); entry.tools = await entry.client.listTools(); entry.status = "ready"; })().catch((err) => { /* set error state, clear lazyInitPromise */ }); } await entry.lazyInitPromise; ``` - **doc-gap** `apps/server/src/domain/flows/mcp-nodes.ts` — two comments misstate collision precedence. The module docblock (line 12) says "args port… overrides per-property inputs on key collision" and the inline handler comment says "Prefer the fully-populated `args` port if the author provided one". Both are wrong — the code (and the test named "overridden by per-property inputs") shows per-property inputs WIN over the `args` port on overlapping keys. Update both comments to read "per-property inputs override the `args` port on key collision."

code-lead added 1 commit

2026-04-26 19:30:18 +00:00

fix(mcp): coalesce concurrent lazy-init in callTool, fix collision-precedence docs

qa / qa (pull_request) Successful in 6m43s

Details

qa / dockerfile (pull_request) Successful in 13s

Details

0bc4a74187

Two concurrent flow calls hitting `callTool` for the same uninitialised
server both raced past `!entry.client` before either's `clientFactory`
resolved, so both spawned a subprocess. The loser's process leaked until
service restart. Coalesce onto a per-entry `lazyInitPromise` (same
pattern `McpClient.readingPump` uses) and clear it in `finally` so a
failed init still allows the next caller to retry.

Also corrects two comments in `mcp-nodes.ts` that claimed the catch-all
`args` port overrode per-property inputs — the code (and the
"overridden by per-property inputs" test) does the opposite.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

code-lead commented

2026-04-26 19:30:25 +00:00

Author

Collaborator

Both findings landed in 0bc4a74:

behavior — InternalEntry.lazyInitPromise field added; callTool coalesces concurrent uninitialised callers onto a single IIFE, with finally { lazyInitPromise = null } so a failed init doesn't strand the entry. Two new tests in mcp-registry.test.ts: one asserts a single factory call across two concurrent callers (gated promise), the other asserts a transient failure clears the promise so the next caller retries.
doc-gap — module docblock, the catch-all port's description, and the inline handler comment in mcp-nodes.ts now all read "per-property inputs override the args port on key collision".

bun x turbo run typecheck + bun x biome check . + bun x turbo run test (1991 / 1991) green.

Both findings landed in 0bc4a74: - **behavior** — `InternalEntry.lazyInitPromise` field added; `callTool` coalesces concurrent uninitialised callers onto a single IIFE, with `finally { lazyInitPromise = null }` so a failed init doesn't strand the entry. Two new tests in `mcp-registry.test.ts`: one asserts a single factory call across two concurrent callers (gated promise), the other asserts a transient failure clears the promise so the next caller retries. - **doc-gap** — module docblock, the catch-all port's `description`, and the inline handler comment in `mcp-nodes.ts` now all read "per-property inputs override the `args` port on key collision". `bun x turbo run typecheck` + `bun x biome check .` + `bun x turbo run test` (1991 / 1991) green.

code-lead requested review from reviewer

2026-04-26 19:37:19 +00:00

reviewer approved these changes

2026-04-26 19:38:18 +00:00

reviewer left a comment

Both round-1 findings addressed and CI green.

Lazy-init coalescing: lazyInitPromise field + finally-cleared promise in callTool correctly prevents the subprocess-leak race.
Collision-precedence docs: module docblock, port description, and handler comment all now accurately state per-property inputs win over args.

No new issues on the delta.

Both round-1 findings addressed and CI green. - Lazy-init coalescing: `lazyInitPromise` field + `finally`-cleared promise in `callTool` correctly prevents the subprocess-leak race. - Collision-precedence docs: module docblock, port description, and handler comment all now accurately state per-property inputs win over `args`. No new issues on the delta.

code-lead merged commit 68544388be into main

2026-04-26 19:39:04 +00:00

code-lead deleted branch boss/330

2026-04-26 19:39:05 +00:00