feat(mcp): finish the per-config connection lookups (#5701) - #5817
feat(mcp): finish the per-config connection lookups (#5701)#5817ntdatt812 wants to merge 1 commit into
Conversation
`connections::connect` resolves per-config via `host::for_config`, while the by-server-id lookups read the process-global `host::try_service`. A host that connects through the facade could never see the connection it just made. `main` is already midway through the answer: `all_connected_tools_for_config` and `disconnect_for_config` both exist, and `disconnect_for_config`'s own docs describe this exact trap. Neither was present at `523ef352a`, the revision the issue was written against, which is why the issue reads it as one odd function out of eight rather than as a migration with two of seven done. This finishes it — the same shape, next to each ambient counterpart: connected_overview_for_config server_tools_for_config is_connected_for_config auth_hint_for_config last_error_for_config Additive, so nothing existing changes signature and no caller has to move. Each `Err` arm logs the workspace-resolution failure instead of folding it silently into `false` / `None` / `Vec::new()`, which is the issue's second ask: "the service is not up" and "the server is not connected" should not be indistinguishable. `server_tools_for_config` breaks the mechanical `_for_config` suffix on purpose: `tools_for_config` would sit directly below `all_connected_tools_for_config` and read as its every-server sibling. Test: `per_config_lookups_see_a_per_config_connection` opens two workspaces so `try_service` has no honest answer, connects through the facade in one, and asserts the per-config lookups see it and that the other workspace does not. Verified discriminating — reimplementing one variant over `try_service` turns it red. `cargo test --features mcp --test mcp_registry_e2e` 17 passed. `cargo check --lib --features mcp` and `--no-default-features` both clean.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughThe MCP connections facade adds five configuration-scoped lookup functions. Each resolves the host for the supplied workspace configuration and returns a benign default when no host exists. An end-to-end test verifies isolation between two workspaces. ChangesPer-config MCP connection lookups
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change adds per-configuration connection lookups without changing existing interfaces, with the supplied checks passing. No actionable merge-blocking risk remains beyond normal review. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Linked Issues checkExplanation The PR satisfies issue Warning Your free Security trial is over. An organization admin can activate billing to continue. Comment |
tinysweeper
Bot
added
the
priority: p3
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
1 participant
Closes #5701.
The issue is right, but
mainhas movedconnections::connectresolves per-config throughhost::for_configwhile the by-server-id lookups read the process-globalhost::try_service, so a host that connects through the facade can never see the connection it just made.Since the issue was filed,
mainalready grew two of the answers:all_connected_tools_for_configdisconnect_for_confige34226cb0, 2026-08-23 16:35 UTCThe issue was opened at 19:14 UTC the same day, ~2h40m after
disconnect_for_configlanded, and listsdisconnectamong the broken seven. That is not a mistake on the reporter's part — I checkedgit cat-file -p 523ef352a:src/openhuman/mcp/registry/mod.rs, the revision the issue names, and neither_for_configvariant exists there. The issue is accurate for its base and stale againstmain.That changes what needs doing. This is not one odd function out of eight; it is a migration with two of seven done.
disconnect_for_config's own doc comment already spells out the trap the issue spends a page reconstructing:So the three resolutions the issue proposes — take a config on all eight, take a
&McpHoston all eight, or makeconnectread the global — are all breaking, while the maintainers had already chosen a fourth: add variants, leave the ambient forms alone. This follows that.What this adds
Five functions, each next to its ambient counterpart, same shape as the two that exist:
Purely additive — no existing signature changes and no caller moves.
Each
Errarm now logs the workspace-resolution failure rather than folding it silently intofalse/None/Vec::new(). That is the issue's second ask: "the service is not up" and "the server is not connected" are different facts and currently share a return value.One naming judgment call for you to overrule if you disagree:
server_tools_for_config, nottools_for_config. The mechanical suffix would puttools_for_configdirectly beneathall_connected_tools_for_config, where it reads as that function's every-server sibling rather than as the one-server form.Test
per_config_lookups_see_a_per_config_connectionintests/mcp_registry_e2e.rs.Making it discriminate was the interesting part. With a single workspace open,
try_servicehappily returns that lone host, so a wrong implementation routed through the global would still pass. The test therefore opens two workspaces first and asserts the precondition explicitly:With two open and no default,
try_servicerefuses to guess — which is the state an embedder holding its own per-config service is in permanently. It then connects through the facade in workspace A and asserts the per-config lookups see it, that workspace B does not, and thatdisconnect_for_configdrops it.Verified red against the wrong implementation: reimplementing
is_connected_for_configovertry_service()(the way the ambient form does) turns it red —panicked at tests/mcp_registry_e2e.rs:723: the workspace that connected must see its own connection.Verification
cargo test --features mcp --test mcp_registry_e2e— 17 passed.cargo check --lib --features mcp— clean.cargo check --lib --no-default-features— clean.stub.rsdeliberately gets no additions: its docs say it mirrors only the surface always-compiled callers depend on, and these five have no such caller yet. The disabled build is the drift catcher and it is green.cargo fmt --all— clean.Summary by CodeRabbit
New Features
Bug Fixes