Add gateway_search/gateway_execute: aggregate downstream MCP servers behind 2 tools - #104
Merged
Conversation
…real rmcp client transport Task 6 of the gateway-registry plan. Adds a minimal `gateway-fixture-server` bin (single `echo` tool, same tool_router/tool_handler macro shape already proven in src/mcp_server.rs) and McpStdioBackend::discover(), which spawns it via rmcp's TokioChildProcess + ServiceExt::serve and calls list_all_tools() over the real MCP stdio transport. Backend::call() stays a todo!() stub, implemented in Task 7.
… deps so gateway-fixture-server builds standalone
… real MCP client Replaces the todo!() with a real tools/call RPC over the already-established rmcp client connection: builds CallToolRequestParams, dispatches via call_tool(), and surfaces is_error results (or transport failures) as GatewayError::Upstream. Falls back to serializing result.content when the server doesn't return structured_content. New tests live in tests/mcp_stdio_call.rs (integration test, not inline in src/mcp_stdio.rs) for the same reason Task 6 relocated the discover() tests: CARGO_BIN_EXE_gateway-fixture-server is only populated by Cargo for integration-test/bench targets, not the lib's own unit-test binary.
…ogether Registry::open_default/open_in_memory build backends from GatewayConfig, ensure_fresh() debounces discover()+db::rebuild() on a 60s window, search() delegates to the BM25 index, and execute() dispatches to the resolved Backend with fuzzy "did you mean" suggestions on unknown server/tool names. Deviations from the brief: - The four fixture-spawning tests moved to a new integration test file (crates/gateway-registry/tests/registry.rs) instead of an inline #[cfg(test)] module, since CARGO_BIN_EXE_gateway-fixture-server is only populated by Cargo for integration-test/bench targets (same reason Tasks 6-7 relocated their fixture tests). - open_in_memory is a plain public method (not #[cfg(test)]-gated), since a #[cfg(test)] item isn't visible to a separate tests/*.rs integration crate. It's also a legitimate constructor outside tests (embedded/ephemeral registries), mirroring db::open_in_memory's existing lack of test-gating. - Added Clone to HttpToolConfig's derive list — build_backends' HttpApi arm needs tools.clone() to build HttpApiBackend, and the brief's assumed derives predated this field's actual definition in config.rs.
…areMcp Adds gateway_search/gateway_execute MCP tools to AgentflareMcp, backed by a tokio::sync::Mutex<Option<gateway_registry::Registry>> (async, unlike skills_registry's std::sync::Mutex, since Registry::ensure_fresh/execute are async fns that await downstream MCP calls). ensure_gateway_registry() lazily opens/refreshes the registry and returns the locked guard directly, since an async operation can't be threaded through a sync FnOnce(&Registry) -> T callback without unstable async-closure machinery. Deviation from brief: rusqlite::Connection is Send but not Sync (RefCell-based statement cache), so gateway_registry::Registry (which held conn: Connection directly) was not Sync, making &Registry non-Send and violating the Send bound rmcp's #[tool] macro requires on returned futures once gateway_execute held `reg: &Registry` across `.await`. Fixed by wrapping Registry's conn field in std::sync::Mutex<Connection> (crates/gateway-registry/src/registry.rs), mirroring how McpStdioBackend already wraps its connection state in a Mutex for the same reason. The mutex is only ever locked for synchronous rusqlite calls and never held across an await point.
… error mapping ServerNotFound/ToolNotFound are caller-fixable input mistakes and stay invalid_params; NotImplemented/Connection/Upstream/Sqlite are infrastructure failures and now map to internal_error, mirroring skill_load's existing two-bucket pattern for LoadError.
…jection failures Addresses two findings from the final whole-branch review: 1. gateway_execute held the whole-Registry tokio Mutex guard across an unbounded downstream await, with no timeout anywhere in the crate. One hung/slow backend wedged every server's gateway_search/gateway_execute, not just its own. McpStdioBackend now wraps connect/discover/call in a 30s tokio::time::timeout (test-overridable via with_timeout), mapping to a new GatewayError::Timeout variant that falls into gateway_execute's existing internal_error catch-all. Root cause of a hang discovered while testing this fix: tokio::process does not kill a spawned child on drop unless kill_on_drop(true) is set, so a hung downstream process outlived the client-side timeout as an orphaned zombie. Added kill_on_drop(true) to the spawned Command. 2. Secret-injection failures were silently swallowed across three layers: build_backends only injected a secret when both auth_ref and auth_env were set, but the design spec's own example config sets only auth_ref; resolve_gateway_secrets discarded get_secret's Err (wrong/missing vault passphrase looked identical to "no secret configured"); a typo'd auth_ref hit the same silent path. config.rs now rejects auth_ref without a paired auth_env at parse time instead of silently no-op'ing, and both remaining silent-failure points now log which server/secret failed to resolve.
📝 WalkthroughWalkthroughThis PR adds a new ChangesGateway Registry and MCP Integration
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant AgentflareMcp
participant Registry
participant McpStdioBackend
participant DownstreamTool
Client->>AgentflareMcp: gateway_execute(server, tool, args)
AgentflareMcp->>Registry: ensure_fresh() / execute(server, tool, args)
Registry->>McpStdioBackend: call(tool, args)
McpStdioBackend->>DownstreamTool: stdio call_tool
DownstreamTool-->>McpStdioBackend: result
McpStdioBackend-->>Registry: JSON value
Registry-->>AgentflareMcp: JSON value
AgentflareMcp-->>Client: response
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
crates/gateway-registry/src/mcp_stdio.rs (2)
109-121: 🎯 Functional Correctness | 🔵 TrivialLocal argument-validation failure is classified as
Upstream.Rejecting non-object/non-null
argshappens entirely locally, before any downstream I/O — it isn't actually an error the downstream server produced. Tagging it asGatewayError::Upstreamconflates a caller/client-side mistake with genuine downstream failures, which could matter forgateway_execute's error classification/retry behavior built on top of this crate. Consider a distinct variant (e.g.InvalidArgument/BadRequest) iferror.rshas one, or add one, so callers can tell "your JSON was malformed" apart from "the downstream tool failed."🤖 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 `@crates/gateway-registry/src/mcp_stdio.rs` around lines 109 - 121, The non-object args check in mcp_stdio::call is a local validation error, not a downstream failure, so it should not return GatewayError::Upstream. Update the error path in call to use a distinct caller-side variant such as InvalidArgument or BadRequest from error.rs, or add one if missing, so gateway_execute can distinguish malformed JSON from real upstream failures. Keep the change scoped to the args parsing branch in call and preserve the existing object/null handling.
49-144: 🚀 Performance & Scalability | 🔵 TrivialAll calls to one backend fully serialize.
self.runningis a plaintokio::sync::Mutex, and its guard is held for the entire duration of each awaited RPC (ensure_connected,discover,call). Two concurrentgateway_execute/gateway_searchcalls hitting the same downstream server will queue behind each other rather than running concurrently over the same connection, even though MCP's JSON-RPC framing is generally designed to multiplex requests by id. Given this is v1 and correctness is more important than throughput here, this is likely fine for now, but worth keeping in mind if the gateway is expected to fan out many concurrent calls to the same backend.🤖 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 `@crates/gateway-registry/src/mcp_stdio.rs` around lines 49 - 144, The MCP stdio backend currently serializes all requests because `McpStdioClient::ensure_connected`, `discover`, and `call` hold the `self.running` mutex across the full awaited RPC flow. Refactor so the lock is only used to read or update the connection state, then release it before awaiting `serve`, `list_all_tools`, or `call_tool` on the `running` handle. Keep `self.running` in place for connection setup/state management, but avoid holding the `tokio::sync::Mutex` during the actual RPCs so concurrent calls to the same backend can proceed.
🤖 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 `@crates/gateway-registry/src/mcp_stdio.rs`:
- Around line 86-144: The timeout handling in discover and call leaves the
cached RunningService in self.running unchanged, so a hung stdio process is
reused after a timeout. Update the timeout branches in mcp_stdio::discover and
mcp_stdio::call to clear or replace the cached service in self.running before
returning GatewayError::Timeout, so the next request will force a fresh
reconnect through ensure_connected. Keep the fix localized around the existing
timeout wrapper and the running.lock() state.
In `@crates/gateway-registry/src/registry.rs`:
- Around line 66-92: In ensure_fresh, a failed backend.discover() currently
causes db::rebuild to drop that server’s previously indexed tools, so update the
refresh flow to preserve the last known ServerTools for backends that fail
transiently instead of rebuilding from only the successful discoveries. Also
replace the serial backend.discover().await loop with concurrent per-backend
discovery while keeping the same skip-and-log handling for individual failures,
then rebuild the index from the combined successful and preserved entries.
In `@crates/gateway-registry/src/search.rs`:
- Around line 43-61: Clamp the `limit` in `search` before converting it to
`i64`, since `limit as i64` can wrap and turn into a negative SQLite `LIMIT`,
effectively removing the cap. Update the `search` function in
`crates/gateway-registry/src/search.rs` to bound the requested limit to a safe
maximum before passing it into `stmt.query_map`, so oversized inputs cannot
produce unbounded results for `gateway_search`.
In `@crates/gateway-registry/src/truncate.rs`:
- Around line 9-26: The truncation logic in truncate_if_needed uses a raw slice
of the pretty-printed JSON string, but the fragment is later embedded in the
"data" field and re-escaped during serialization, so the final output can still
exceed max_chars. Update truncate_if_needed (and reuse find_safe_cut_point if
needed) to account for JSON escaping overhead by measuring the serialized size
of the truncated Value and reducing the cut until the escaped response fits
within the budget.
In `@src/mcp_server.rs`:
- Around line 236-243: The failures in resolve_gateway_secrets are still being
hidden by returning an empty HashMap on open_db and list_secrets errors. Update
resolve_gateway_secrets in mcp_server to log those two error paths to stderr
(similar to the get_secret handling) before returning, so a locked or broken
gateway_secrets DB is visible instead of looking like no secrets are configured.
- Around line 228-234: The `load_gateway_config` function is swallowing TOML
parse failures by using
`gateway_registry::parse_config(&s).unwrap_or_default()`, which turns a
malformed `gateway.toml` into an empty config with no signal. Update
`load_gateway_config` to distinguish between a missing file and a
present-but-invalid file: keep returning
`gateway_registry::GatewayConfig::default()` only when `std::fs::read_to_string`
fails, but when `parse_config` returns an error, surface it (for example with
`eprintln!`) and avoid silently discarding the config. Use the existing
`load_gateway_config` and `gateway_registry::parse_config` symbols to locate the
change.
---
Nitpick comments:
In `@crates/gateway-registry/src/mcp_stdio.rs`:
- Around line 109-121: The non-object args check in mcp_stdio::call is a local
validation error, not a downstream failure, so it should not return
GatewayError::Upstream. Update the error path in call to use a distinct
caller-side variant such as InvalidArgument or BadRequest from error.rs, or add
one if missing, so gateway_execute can distinguish malformed JSON from real
upstream failures. Keep the change scoped to the args parsing branch in call and
preserve the existing object/null handling.
- Around line 49-144: The MCP stdio backend currently serializes all requests
because `McpStdioClient::ensure_connected`, `discover`, and `call` hold the
`self.running` mutex across the full awaited RPC flow. Refactor so the lock is
only used to read or update the connection state, then release it before
awaiting `serve`, `list_all_tools`, or `call_tool` on the `running` handle. Keep
`self.running` in place for connection setup/state management, but avoid holding
the `tokio::sync::Mutex` during the actual RPCs so concurrent calls to the same
backend can proceed.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d1206e99-e3ec-4b0d-8b53-be8cf13f762c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
.superpowers/sdd/progress.mdCargo.tomlcrates/gateway-registry/Cargo.tomlcrates/gateway-registry/src/backend.rscrates/gateway-registry/src/config.rscrates/gateway-registry/src/db.rscrates/gateway-registry/src/error.rscrates/gateway-registry/src/lib.rscrates/gateway-registry/src/mcp_stdio.rscrates/gateway-registry/src/registry.rscrates/gateway-registry/src/search.rscrates/gateway-registry/src/truncate.rscrates/gateway-registry/src/types.rscrates/gateway-registry/tests/fixtures/fixture_server.rscrates/gateway-registry/tests/mcp_stdio_call.rscrates/gateway-registry/tests/mcp_stdio_discover.rscrates/gateway-registry/tests/mcp_stdio_timeout.rscrates/gateway-registry/tests/registry.rssrc/cli/gateway.rssrc/cli/mod.rssrc/gateway_secrets.rssrc/main.rssrc/mcp_server.rs
Resolves conflicts from master's crate-renaming (agentflare-* package names for crates.io publishing). Renamed gateway-registry's own package to agentflare-gateway-registry to match, following the same package = "..." aliasing convention already used for agent-registry/skill-registry so all existing `use gateway_registry::...` call sites keep working; updated gateway-registry's own integration tests (which link the lib by its real package-derived name, not the alias) to use agentflare_gateway_registry directly, matching skill-registry's own tests' existing convention.
Fixes across gateway-registry and the gateway MCP tools: - mcp_stdio: clear cached connection on timeout so the next call reconnects instead of reusing a possibly wedged child process - registry: ensure_fresh falls back to a server's previously-indexed tools when its discover() call fails transiently, instead of dropping them - search: clamp limit before casting to i64 so it can't wrap negative and silently defeat SQLite's LIMIT - truncate: account for JSON string-escaping overhead so the reserialized envelope actually fits within max_chars, not just the raw cut region - mcp_server: log open_db/list_secrets failures in resolve_gateway_secrets, and log gateway.toml parse failures separately from a missing file - mcp_stdio/error: add GatewayError::InvalidArgument for local args validation, mapped to invalid_params in gateway_execute like ServerNotFound/ToolNotFound, instead of misclassified as Upstream
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@crates/gateway-registry/src/registry.rs`:
- Around line 92-95: The refresh path in `GatewayRegistry::rebuild` is
swallowing `db::server_tools` read failures by using `unwrap_or_default()`,
which can turn an error into an empty `previous` set and accidentally drop
preserved tools. Change this read to propagate the DB error instead of
defaulting, so the rebuild aborts on failure and keeps the current index intact;
use the existing `server_tools` call and the `previous` lookup in `rebuild` as
the place to update.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8d267400-802b-422a-8b46-ed71615630c7
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
Cargo.tomlcrates/gateway-registry/Cargo.tomlcrates/gateway-registry/src/db.rscrates/gateway-registry/src/error.rscrates/gateway-registry/src/mcp_stdio.rscrates/gateway-registry/src/registry.rscrates/gateway-registry/src/search.rscrates/gateway-registry/src/truncate.rscrates/gateway-registry/tests/fixtures/fixture_server.rscrates/gateway-registry/tests/mcp_stdio_call.rscrates/gateway-registry/tests/mcp_stdio_discover.rscrates/gateway-registry/tests/mcp_stdio_timeout.rscrates/gateway-registry/tests/registry.rssrc/main.rssrc/mcp_server.rs
✅ Files skipped from review due to trivial changes (1)
- src/main.rs
🚧 Files skipped from review as they are similar to previous changes (9)
- crates/gateway-registry/tests/mcp_stdio_discover.rs
- crates/gateway-registry/Cargo.toml
- crates/gateway-registry/tests/mcp_stdio_call.rs
- Cargo.toml
- crates/gateway-registry/tests/registry.rs
- crates/gateway-registry/src/search.rs
- crates/gateway-registry/src/error.rs
- crates/gateway-registry/src/mcp_stdio.rs
- src/mcp_server.rs
Comment on lines
+92
to
+95
| let previous = { | ||
| let conn = self.conn.lock().expect("gateway registry db mutex poisoned"); | ||
| db::server_tools(&conn, name).unwrap_or_default() | ||
| }; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Propagate fallback read errors instead of wiping preserved tools.
unwrap_or_default() turns a server_tools DB read failure into an empty fallback; since rebuild is full-replace, the refresh can then delete the server’s last-known-good tools. Return the DB error so the refresh aborts and leaves the current index intact.
Proposed fix
let previous = {
let conn = self.conn.lock().expect("gateway registry db mutex poisoned");
- db::server_tools(&conn, name).unwrap_or_default()
+ db::server_tools(&conn, name)?
};📝 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.
Suggested change
| let previous = { | |
| let conn = self.conn.lock().expect("gateway registry db mutex poisoned"); | |
| db::server_tools(&conn, name).unwrap_or_default() | |
| }; | |
| let previous = { | |
| let conn = self.conn.lock().expect("gateway registry db mutex poisoned"); | |
| db::server_tools(&conn, name)? | |
| }; |
🤖 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 `@crates/gateway-registry/src/registry.rs` around lines 92 - 95, The refresh
path in `GatewayRegistry::rebuild` is swallowing `db::server_tools` read
failures by using `unwrap_or_default()`, which can turn an error into an empty
`previous` set and accidentally drop preserved tools. Change this read to
propagate the DB error instead of defaulting, so the rebuild aborts on failure
and keeps the current index intact; use the existing `server_tools` call and the
`previous` lookup in `rebuild` as the place to update.
4 tasks
This was referenced Jul 8, 2026
Merged
1 task
Merged
This was referenced Jul 11, 2026
This was referenced Jul 21, 2026
This was referenced Aug 13, 2026
4 tasks
getappz
added a commit
that referenced
this pull request
Aug 21, 2026
#573) Same architectural gap as skill-registry (#519/PR #572), which gateway-registry's own doc comments say it mirrors: hand-rolled apply_schema() with no user_version tracking. Audited the git history (#104 -> #158 -> #347) -- the tools table's columns have never changed since creation, so there's no live "no such column" bug today, but the next column addition would hit the identical class of bug. Migrates to agentflare-db-kit's open_file/open_memory with a real migration list: 0001_initial replays the original (#104) narrow schema, 0002_fts_triggers unconditionally drops and recreates tools_fts as the external-content shape with sync triggers plus a backfill. No ALTER TABLE/migration hook needed here (unlike #519) since tools's columns are stable -- DROP ... IF EXISTS before a fresh CREATE is correct regardless of which pre-migration shape existed. Added GatewayError::DbInit for db_kit::open::Error and its error_kind match arm. All 60 gateway-registry unit tests pass, including the existing legacy-standalone-FTS conversion test; clippy (with the CI gate flags) and fmt are clean; the full agentflare binary compiles. Agentflare-Agent: claude-code_2-1-237_agent Agentflare-Branch: task/520-fix-gateway-registry-adopt-agentflare-db Agentflare-Item: 520
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
crates/gateway-registry: a SQLite+FTS5 manifest of downstream MCP tools, BM25 search, and a realrmcpMCP client (spawns child processes, real protocol handshake,tools/list/tools/call) with aBackendenum reserved for future non-stdio backend kinds.gateway_searchandgateway_execute, intoAgentflareMcpso any number of downstream MCP servers can be aggregated behind a fixed 2-tool surface — adding a 10th downstream server costs the calling LLM nothing extra in schema tokens.auth_cryptAES-256-GCM primitive) and anagentflare gateway secret set/list/removeCLI subcommand for downstream-server credentials.Test plan
cargo test --workspace)gateway-registrycrate suite: 38 tests (lib + integration, including real spawned-fixture-process tests and a timeout/hang regression test)mcp_servertests: 15 tests, including the new gateway tool testsagentflare.exebinary: spawned a fixture MCP server, drove the real stdio JSON-RPC protocol by hand (initialize→gateway_search→gateway_execute), confirmed the full tool round-tripSummary by CodeRabbit