Skip to content

feat(mcp-registry): InstalledServer HTTP-remote transport#2603

Open
justinhsu1477 wants to merge 2 commits into
tinyhumansai:mainfrom
justinhsu1477:feat/mcp-installed-server-http-remote
Open

feat(mcp-registry): InstalledServer HTTP-remote transport#2603
justinhsu1477 wants to merge 2 commits into
tinyhumansai:mainfrom
justinhsu1477:feat/mcp-installed-server-http-remote

Conversation

@justinhsu1477
Copy link
Copy Markdown
Contributor

@justinhsu1477 justinhsu1477 commented May 25, 2026

Closes the named follow-up TODO from #2559:

`InstalledServer` HTTP-remote transport variant (most Smithery servers are HTTP-remote; current install path is stdio-only)

The gap this fixes

Before this PR, the setup-agent install path (`mcp_setup_test_connection` + `mcp_setup_install_and_connect`) hard-filtered registry detail responses to `c.r#type == "stdio"`. Smithery — which hosts the bulk of the official MCP ecosystem — serves ~99% of its listings as HTTP-remote. UX was:

User browses Smithery → picks a server → clicks install → almost every attempt fails with "no stdio connection."

`McpHttpClient` (Streamable HTTP + OAuth + SSE per the MCP spec) already exists in `mcp_client` and is wired for static-config servers. This PR extends the install / persistence / connect plumbing in `mcp_registry` to dispatch through it.

What changed

File Change
`types.rs` New `Transport::{Stdio, HttpRemote { url }}` enum with `dispatch_kind` strings (`"stdio"` / `"http_remote"`) and a `parse(kind, url)` inverse that defaults unknown / missing values to `Stdio` (migration safety hatch). `InstalledServer` grows a `transport` field, `#[serde(default)]`-backed so legacy serialised payloads still load.
`store.rs` `mcp_servers` table grows two additive columns (`transport`, `deployment_url`) via post-`CREATE TABLE` `ALTER TABLE ADD COLUMN`, gated by a `PRAGMA table_info` lookup so the migration is idempotent across launches. Insert + list + get carry the new columns. Row mapper falls back to `Transport::Stdio` when `transport` is missing.
`setup_ops.rs` New `pick_connection` helper: published stdio → any stdio → published http_remote → any http_remote → None. Stdio always wins when offered, so no behavior regression for any server that already installs today. Both setup ops branch on the picked transport and dispatch to either `McpStdioClient` or `McpHttpClient`. `ConnectionKind` trait normalises the registry-side `"http"` / `"sse"` strings into the persisted `"http_remote"` dispatch kind.
`connections.rs` New `ActiveClient::{Stdio(Arc<...>), Http(Arc<...>)}` enum with a shared async surface. `connect()` branches on `server.transport` — empty deployment_url for HTTP bails loudly. Both branches still run `initialize` + `list_tools` synchronously so misconfigured servers fail at connect, not at first call.
`ops.rs` Legacy `mcp_clients_install` direct path was already stdio-filtered; pinned to `Transport::Stdio` explicitly. HTTP support flows through the newer setup-agent path.

Tests

64/64 pass in `cargo test --lib mcp_registry` (was 57; +7 new):

  • `transport_dispatch_kind_strings_are_stable` — column-value pinning
  • `transport_parse_falls_back_to_stdio_for_unknown_kinds` — migration safety
  • `transport_deployment_url_accessor` — Stdio↔HttpRemote never cross
  • `installed_server_defaults_transport_to_stdio_on_missing_field` — legacy JSON still loads
  • `pick_connection_prefers_stdio_over_http` / `_prefers_published_stdio_first` / `_falls_back_to_http_remote_when_no_stdio` / `_returns_none_for_only_unknown_kinds` — preference order
  • `connection_kind_normalises_http_variants` — `http` / `sse` / `http_remote` → `http_remote` mapping
  • `http_remote_server_roundtrips_with_url_preserved` — SQLite write+read
  • `list_servers_preserves_per_row_transport` — mixed stdio + http rows
  • `additive_migration_recovers_pre_migration_row_as_stdio` — builds pre-migration schema, runs `init_schema` twice, confirms legacy row loads as Stdio

`cargo check --lib` + `cargo fmt --check` clean. No protocol-level behavior change for any server that already installs today.

Out of scope (follow-ups)

  • Legacy `mcp_clients_install` direct path doesn't yet dispatch HTTP — most callers should use the setup agent; explicit follow-up if anyone needs the legacy path too.
  • No UI changes — the existing install dialog calls the same RPC and will start succeeding for HTTP-only Smithery servers as soon as this lands.
  • OAuth flow for HTTP-remote installs is handled by `McpHttpClient`'s existing auth layer; no new auth plumbing needed here.

Refs #2559.

Summary by CodeRabbit

  • New Features
    • Added HTTP-remote MCP server support alongside local subprocess (stdio).
    • Persisted server transport and deployment URL in the database.
  • Improvements
    • Enhanced setup/install/test flows to validate and select transports, with clearer transport handling.
    • Better connection selection logic that prefers published stdio, then http_remote variants.
  • Tests
    • Added unit and e2e tests covering transport behavior, selection precedence, and migrations.

Review Change Stack

Closes the named follow-up TODO from tinyhumansai#2559:
> InstalledServer HTTP-remote transport variant (most Smithery servers
> are HTTP-remote; current install path is stdio-only)

Before this change, the setup-agent install path (`mcp_setup_test_connection`
+ `mcp_setup_install_and_connect`) hard-filtered registry detail responses
to `c.r#type == "stdio"`. Smithery — which hosts the bulk of the official
MCP ecosystem — serves ~99% of its listings as HTTP-remote, so the UX
was: user browses Smithery, picks a server, clicks install, and almost
every attempt failed at "no stdio connection".

The `mcp_client` module already has `McpHttpClient` (Streamable HTTP +
OAuth + SSE per the MCP spec) wired and tested for the static-config
servers. This change extends the *install / persistence / connect*
plumbing in `mcp_registry` to dispatch through it.

## What changed

**types.rs** — new `Transport::{Stdio, HttpRemote { url }}` enum with
serialisation-safe `dispatch_kind()` strings (`"stdio"` / `"http_remote"`)
and a `parse(kind, url)` inverse that defaults unknown / missing values
to `Stdio` (the migration safety hatch). `InstalledServer` grows a
`transport: Transport` field, defaulted via `#[serde(default)]` so legacy
serialised payloads from before this change still load.

**store.rs** — `mcp_servers` table grows two additive columns
(`transport`, `deployment_url`) via post-`CREATE TABLE` `ALTER TABLE
ADD COLUMN`, gated by a `PRAGMA table_info` lookup so the migration is
idempotent across launches. Insert + list + get carry the new columns.
The row mapper falls back to `Transport::Stdio` when `transport` is
missing — pre-migration rows continue to load as stdio.

**setup_ops.rs** — new `pick_connection` helper picks the best
`SmitheryConnection` from a registry detail. Preference order:
published stdio → any stdio → published http_remote → any http_remote →
None. Stdio always wins when offered, so no behavior regression for any
server that already installed pre-PR. `mcp_setup_test_connection` +
`mcp_setup_install_and_connect` branch on the picked transport and
dispatch to either `McpStdioClient` or `McpHttpClient`. A
`ConnectionKind` trait normalises the registry-side `"http"` / `"sse"`
strings into the persisted `"http_remote"` dispatch kind so the picker
and the install record agree on vocabulary.

**connections.rs** — new `ActiveClient::{Stdio(Arc<...>), Http(Arc<...>)}`
enum wraps the two clients with a shared async surface
(`list_tools` / `call_tool` / `close_session`). `connect()` now
branches on `server.transport`:
- `Stdio` — current subprocess spawn (unchanged).
- `HttpRemote { url }` — `McpHttpClient::new(url, 30)` dial. Empty URL
  bails loudly.

Both paths still run `initialize` + `list_tools` synchronously so a
misconfigured server fails at connect, not at first call.

**ops.rs** — the legacy `mcp_clients_install` direct-install RPC was
already stdio-filtered; pinned its constructed records to
`Transport::Stdio` so the legacy path is explicit about scope. HTTP
support flows through the newer setup-agent path. Wiring the legacy
path to HTTP-remote is a small follow-up (out of scope here to keep
the diff focused on the migration + dispatch).

## Tests

64/64 pass in `cargo test --lib mcp_registry` (was 57; +7 new):

* `transport_dispatch_kind_strings_are_stable` — pins the persisted
  column values.
* `transport_parse_falls_back_to_stdio_for_unknown_kinds` — the
  migration safety hatch (empty / garbage / `"stdio"` all → Stdio,
  `"http_remote"` carries the URL through).
* `transport_deployment_url_accessor` — confirms stdio → None,
  http_remote → Some(url), the two never get crossed.
* `installed_server_defaults_transport_to_stdio_on_missing_field` —
  legacy JSON payloads (no `transport` key) still deserialise.
* `pick_connection_prefers_stdio_over_http` /
  `_prefers_published_stdio_first` /
  `_falls_back_to_http_remote_when_no_stdio` /
  `_returns_none_for_only_unknown_kinds` — preference order pinned.
* `connection_kind_normalises_http_variants` — the
  `"http"` / `"sse"` / `"http_remote"` → `"http_remote"` mapping.
* `http_remote_server_roundtrips_with_url_preserved` — SQLite write +
  read for the new transport, deployment_url preserved.
* `list_servers_preserves_per_row_transport` — mixed stdio + http
  rows don't cross-contaminate at SELECT.
* `additive_migration_recovers_pre_migration_row_as_stdio` — builds
  a pre-migration schema, inserts a legacy row, runs `init_schema`
  twice (idempotency), and confirms the legacy row loads as
  `Transport::Stdio`.

`cargo check --lib` + `cargo fmt --check` clean. No protocol-level
behavior change for any server that already installs today (stdio still
wins the picker when offered).

## Out of scope

* Legacy `mcp_clients_install` direct path doesn't yet dispatch HTTP
  (still stdio-filtered) — most callers should switch to the setup
  agent; explicit follow-up if anyone needs the legacy path too.
* No UI changes — the existing install dialog calls the same RPC and
  will start succeeding for HTTP-only Smithery servers as soon as this
  lands.
* OAuth flow for HTTP-remote installs is handled by `McpHttpClient`'s
  existing auth layer; no new auth plumbing here.
@justinhsu1477 justinhsu1477 requested a review from a team May 25, 2026 05:52
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 25, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 05938f6b-7013-4979-ae91-09f028ac3509

📥 Commits

Reviewing files that changed from the base of the PR and between 44542e3 and ad4d1b6.

📒 Files selected for processing (1)
  • tests/mcp_registry_e2e.rs

📝 Walkthrough

Walkthrough

Extends MCP support from stdio-only to dual-transport (stdio and HTTP-remote): adds a Transport enum, persists transport metadata with a DB migration, dispatches connections via an ActiveClient enum, adds setup selection and transport-aware test/install flows, and updates tests.

Changes

MCP HTTP-Remote Transport Support

Layer / File(s) Summary
Transport type definition & serialization
src/openhuman/mcp_registry/types.rs, tests/*
Introduces Transport enum (Stdio vs HttpRemote with URL), implements dispatch_kind, parse (with Stdio fallback), and deployment_url; extends InstalledServer with transport: Transport (serde default) and updates tests/e2e to include transport.
Database persistence layer
src/openhuman/mcp_registry/store.rs
Adds idempotent schema migration to add transport and deployment_url columns via PRAGMA table_info, updates INSERT/SELECT to persist and retrieve transport metadata, implements map_server_row parsing with Stdio fallback, and adds tests for round-trip, per-row isolation, and pre-migration recovery.
Connection registry transport dispatch
src/openhuman/mcp_registry/connections.rs
Refactors registry to use ActiveClient enum wrapping McpStdioClient or McpHttpClient; connect branches on server.transport, validates HTTP-remote URLs, initializes chosen client (30s timeout for HTTP), caches tools, and stores the ActiveClient; debug logs include transport kind.
Setup operations & connection selection
src/openhuman/mcp_registry/setup_ops.rs
Adds pick_connection with transport-kind normalization and deterministic preference; mcp_setup_test_connection and mcp_setup_install_and_connect now branch by transport—stdio resolves command and uses McpStdioClient, HTTP validates deployment_url and uses McpHttpClient; includes unit tests for selection precedence and normalization.
Legacy install integration
src/openhuman/mcp_registry/ops.rs
Updates legacy install RPC to explicitly set InstalledServer.transport = Transport::Stdio when constructing servers and documents that HTTP-remote installs follow the separate setup pathway.

Sequence Diagrams

sequenceDiagram
  participant Agent as Setup Agent
  participant Setup as Setup Handler
  participant Picker as pick_connection
  participant Client as Transport Client
  Agent->>Setup: mcp_setup_test_connection(connections)
  Setup->>Picker: pick_connection(connections)
  Picker-->>Setup: selected SmitheryConnection
  alt selected.transport == Stdio
    Setup->>Client: resolve_command + McpStdioClient init
  else selected.transport == HttpRemote
    Setup->>Client: validate deployment_url + McpHttpClient init
  end
  Client->>Client: list_tools
  Client-->>Setup: tools OR error
  Setup->>Client: close_session
  Setup-->>Agent: RpcOutcome(ok, tools)
Loading
sequenceDiagram
  participant Client as Registry Client
  participant Registry as Connection Registry
  participant StdioClient as McpStdioClient
  participant HttpClient as McpHttpClient
  Client->>Registry: connect(config, server)
  Registry->>Registry: check server.transport
  alt server.transport == Stdio
    Registry->>StdioClient: new + initialize
    StdioClient-->>Registry: ready
  else server.transport == HttpRemote
    Registry->>Registry: validate deployment_url non-empty
    Registry->>HttpClient: new + initialize(url, 30s timeout)
    HttpClient-->>Registry: ready
  end
  Registry->>Registry: list_tools + cache
  Registry-->>Client: Vec~McpTool~
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested labels

rust-core

Suggested reviewers

  • senamakel

Poem

🐰 Hop, hop, the transports sing,
Stdio towns and HTTP wings.
DB remembers each server's road,
Registry routes the proper load.
Tools listed, sessions close—hooray!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely identifies the main change: adding HTTP-remote transport support to InstalledServer in the mcp-registry module.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot added feature Net-new user-facing capability or product behavior. working A PR that is being worked on by the team. labels May 25, 2026
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (2)
src/openhuman/mcp_registry/store.rs (1)

246-269: 💤 Low value

Verify column index alignment with SELECT statement.

The map_server_row function uses positional indices (12, 13) for transport columns. These indices must match the SELECT column order in list_servers_conn and get_server_conn. The SELECT at lines 172-175 and 191-194 shows transport at position 12 and deployment_url at position 13 — this looks correct but is fragile if the SELECT order changes.

Consider adding a brief inline comment mapping indices to column names to aid future maintenance.

🤖 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 `@src/openhuman/mcp_registry/store.rs` around lines 246 - 269, map_server_row
currently uses positional indices 12 and 13 to pull the transport columns which
must match the SELECT column order in list_servers_conn and get_server_conn; add
a brief inline comment in map_server_row that documents "index 12 = transport,
index 13 = deployment_url" (and mention the corresponding SELECTs in
list_servers_conn/get_server_conn) so future changes to SELECT ordering are
obvious, and optionally note that these are post-migration columns to explain
the use of Option<String> for row.get; this makes the index-to-column mapping
explicit and reduces fragility when the SELECT projection is edited.
src/openhuman/mcp_registry/connections.rs (1)

107-114: 💤 Low value

Unused env values for HTTP-remote connections.

The env variable is loaded and logged for all connections but only used for Transport::Stdio. For HTTP-remote, the comment at lines 140-144 explains that env values (OAuth tokens) are handled by McpHttpClient's own auth config. This is fine, but the unconditional load and log at lines 107-114 may be slightly misleading for HTTP-remote servers where env_map will typically be empty.

This is a minor observation — the current behavior is correct and the logging helps with debugging.

🤖 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 `@src/openhuman/mcp_registry/connections.rs` around lines 107 - 114, The code
unconditionally calls store::load_env_values(config, &server.server_id) into env
and logs its keys even for HTTP-remote transports where env values are unused;
to avoid misleading logs and unnecessary work, only load and log env when the
transport will use them (e.g., inside the Transport::Stdio branch or before code
that constructs StdioConnector), and skip the load/log for HTTP-remote where
McpHttpClient handles auth; move the call to store::load_env_values and the
tracing::debug into the Stdio-specific path (or guard it with a match on
server.transport) and keep McpHttpClient creation unchanged.
🤖 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.

Nitpick comments:
In `@src/openhuman/mcp_registry/connections.rs`:
- Around line 107-114: The code unconditionally calls
store::load_env_values(config, &server.server_id) into env and logs its keys
even for HTTP-remote transports where env values are unused; to avoid misleading
logs and unnecessary work, only load and log env when the transport will use
them (e.g., inside the Transport::Stdio branch or before code that constructs
StdioConnector), and skip the load/log for HTTP-remote where McpHttpClient
handles auth; move the call to store::load_env_values and the tracing::debug
into the Stdio-specific path (or guard it with a match on server.transport) and
keep McpHttpClient creation unchanged.

In `@src/openhuman/mcp_registry/store.rs`:
- Around line 246-269: map_server_row currently uses positional indices 12 and
13 to pull the transport columns which must match the SELECT column order in
list_servers_conn and get_server_conn; add a brief inline comment in
map_server_row that documents "index 12 = transport, index 13 = deployment_url"
(and mention the corresponding SELECTs in list_servers_conn/get_server_conn) so
future changes to SELECT ordering are obvious, and optionally note that these
are post-migration columns to explain the use of Option<String> for row.get;
this makes the index-to-column mapping explicit and reduces fragility when the
SELECT projection is edited.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 378a0e9a-ca4d-41ff-b589-a1c27a7e309e

📥 Commits

Reviewing files that changed from the base of the PR and between 03d6898 and 44542e3.

📒 Files selected for processing (5)
  • src/openhuman/mcp_registry/connections.rs
  • src/openhuman/mcp_registry/ops.rs
  • src/openhuman/mcp_registry/setup_ops.rs
  • src/openhuman/mcp_registry/store.rs
  • src/openhuman/mcp_registry/types.rs

coderabbitai[bot]
coderabbitai Bot previously approved these changes May 25, 2026
…xture

`InstalledServer` grew a `transport` field in the previous commit but
`tests/mcp_registry_e2e.rs::make_installed_server` was missed because the
integration test target isn't compiled by `cargo check --lib` /
`cargo test --lib`. CI surfaced the E0063 on
`test / Rust Core Tests + Quality`.

Pinning the test fixture to `Transport::Stdio` matches the existing test
intent — it dials a local stub subprocess.
@coderabbitai coderabbitai Bot added the rust-core Core Rust runtime in src/: CLI, core_server, shared infrastructure. label May 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature Net-new user-facing capability or product behavior. rust-core Core Rust runtime in src/: CLI, core_server, shared infrastructure. working A PR that is being worked on by the team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant