Repository navigation
Conversation
- Add ureq HTTP client for registry.modelcontextprotocol.io - search_with_fallback: local BM25 first, registry if under limit - SkillHit gains install_hint/remote_url for registry hits - Wire into skill tool handler
📝 WalkthroughWalkthroughAdds official MCP Registry HTTP fallback search, registry provenance and installation metadata, quota-limited merging for gateway and skill results, and MCP server wiring for both skill and tool search actions. ChangesRegistry fallback search
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
🤖 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_search.rs`:
- Line 8: Update the REGISTRY_BASE constant to use the stable /v0.1/servers
endpoint instead of /v0/servers, preserving the existing registry search and
listing behavior.
- Around line 64-68: Update the remote selection logic that consumes
RegistryRemote so it selects only entries with remote_type equal to
"streamable-http" before deriving remote_url, rather than taking the first
remote unconditionally. Preserve the existing URL handling for the selected
streamable HTTP remote and avoid surfacing sse entries.
- Around line 88-95: Update the registry request in the search flow around the
ureq::get call to use an explicit finite timeout that bounds both connection and
response-read operations. Preserve the existing empty-vector fallback for
request and JSON parsing failures.
In `@crates/gateway-registry/src/search.rs`:
- Around line 122-148: Update search_with_fallback to return
rusqlite::Result<Vec<ToolHit>> instead of Vec<ToolHit>, and propagate failures
from search with ?. Keep the existing local-result limit check and registry
fallback behavior for successful searches, returning the assembled results
through the Result type.
In `@crates/skill-registry/src/search.rs`:
- Around line 82-94: The synchronous remote fallback in search_with_fallback
must not run while SQLite mutexes are held. In
crates/skill-registry/src/search.rs lines 82-94, split local lookup from remote
supplementation or make the API asynchronous; in
crates/gateway-registry/src/registry.rs lines 178-182, release conn after local
search before invoking the remote phase; and in src/mcp_server.rs line 795, move
remote supplementation outside with_fresh_registry so skills_registry is
released before HTTP I/O.
- Around line 97-111: Update the SkillHit construction in the search result
mapping so fallback or remote registry hits use the stable registry source value
instead of an empty string. Change only the source field while preserving the
existing name, metadata, and install_hint mapping.
🪄 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: 11ec746d-b5fb-4813-8a39-faf9afbe03ae
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
crates/gateway-registry/Cargo.tomlcrates/gateway-registry/src/lib.rscrates/gateway-registry/src/registry.rscrates/gateway-registry/src/registry_search.rscrates/gateway-registry/src/search.rscrates/skill-registry/Cargo.tomlcrates/skill-registry/src/load.rscrates/skill-registry/src/search.rssrc/mcp_server.rs
| use crate::search::InstallHint; | ||
| use serde::Deserialize; | ||
|
|
||
| const REGISTRY_BASE: &str = "https://registry.modelcontextprotocol.io/v0/servers"; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '1,220p' crates/gateway-registry/src/registry_search.rsRepository: getappz/agentflare
Length of output: 4137
🌐 Web query:
MCP Registry search endpoint /v0/servers /v0.1/servers official documentation
💡 Result:
The Model Context Protocol (MCP) Registry provides a RESTful API for discovering and accessing MCP server metadata [1][2]. The official registry, hosted at https://registry.modelcontextprotocol.io, utilizes the /v0.1/servers endpoint for listing and searching servers [3][4][5]. There is no active /v0/servers endpoint in the current API specification; /v0.1/servers is the established standard for querying server information [3][4][5]. The GET /v0.1/servers endpoint supports the following query parameters to facilitate searching and filtering: - search: Performs a case-insensitive substring search on server names (e.g.,?search=filesystem) [3][6]. - updated_since: Filters servers updated after a specific RFC 3339 timestamp [3][4][6]. - version: Filters by version, supporting specific versions or the keyword latest [3][6]. - limit: Specifies the maximum number of items to return [4][6]. - cursor: Used for cursor-based pagination, allowing clients to retrieve subsequent pages of results [4][6]. Official documentation for the MCP Registry API, including the OpenAPI specification, can be found at the following resources: - Official Registry API Reference: https://registry.modelcontextprotocol.io/docs [7][8] - OpenAPI Specification: https://registry.modelcontextprotocol.io/openapi.yaml [9] - Registry GitHub Documentation: https://github.com/modelcontextprotocol/registry/tree/main/docs [2][10]
Citations:
- 1: https://modelcontextprotocol.io/registry/about
- 2: https://github.com/modelcontextprotocol/registry?tab=readme-ov-file
- 3: https://github.com/modelcontextprotocol/registry/blob/main/docs/reference/api/official-registry-api.md
- 4: https://modelcontextprotocol.io/registry/registry-aggregators
- 5: https://github.com/modelcontextprotocol/registry/blob/main/docs/reference/api/generic-registry-api.md
- 6: https://github.com/modelcontextprotocol/registry/blob/dc736892/docs/reference/api/openapi.yaml
- 7: https://registry.modelcontextprotocol.io/docs
- 8: https://registry.modelcontextprotocol.io/
- 9: https://modelcontextprotocol.io/registry/faq.md
- 10: https://github.com/modelcontextprotocol/registry
Use the stable /v0.1/servers endpoint. The Registry API’s search/listing endpoint is /v0.1/servers; /v0 is the development API and can change or disappear, which would break fallback search.
🤖 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_search.rs` at line 8, Update the
REGISTRY_BASE constant to use the stable /v0.1/servers endpoint instead of
/v0/servers, preserving the existing registry search and listing behavior.
| struct RegistryRemote { | ||
| #[serde(rename = "type")] | ||
| remote_type: String, | ||
| url: String, | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- registry_search outline ---'
ast-grep outline crates/gateway-registry/src/registry_search.rs --view expanded || true
echo
echo '--- registry_search relevant lines ---'
sed -n '1,220p' crates/gateway-registry/src/registry_search.rs | cat -n
echo
echo '--- search RegistryRemote/remotes usage ---'
rg -n "RegistryRemote|remotes|streamable-http|sse|remote_type" crates/gateway-registry -SRepository: getappz/agentflare
Length of output: 21407
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- file slice: registry_search.rs ---\n'
nl -ba crates/gateway-registry/src/registry_search.rs | sed -n '1,220p'
printf '\n--- all references to the selected remote_url / remote_type in gateway-registry ---\n'
rg -n "remote_url|remote_type|streamable-http|sse|remotes" crates/gateway-registry -SRepository: getappz/agentflare
Length of output: 236
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- search.rs around RegistryHit usage ---\n'
python3 - <<'PY'
from pathlib import Path
p = Path('crates/gateway-registry/src/search.rs')
lines = p.read_text().splitlines()
for start, end in [(1,260)]:
for i,l in enumerate(lines[start-1:end], start):
print(f"{i:5} {l}")
PY
printf '\n--- registry.rs remote_url references ---\n'
python3 - <<'PY'
from pathlib import Path
p = Path('crates/gateway-registry/src/registry.rs')
lines = p.read_text().splitlines()
for i,l in enumerate(lines,1):
if 'remote_url' in l or 'RegistryHit' in l or 'streamable-http' in l or 'sse' in l:
lo = max(1, i-8); hi = min(len(lines), i+12)
print(f"\n## around line {i}")
for j in range(lo, hi+1):
print(f"{j:5} {lines[j-1]}")
PY
printf '\n--- tests referencing remote_url ---\n'
rg -n "remote_url|streamable-http|sse" crates/gateway-registry/src crates/gateway-registry/tests -SRepository: getappz/agentflare
Length of output: 27694
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- search.rs around RegistryHit usage ---\n'
python3 - <<'PY'
from pathlib import Path
p = Path('crates/gateway-registry/src/search.rs')
lines = p.read_text().splitlines()
for i,l in enumerate(lines,1):
if 'RegistryHit' in l or 'remote_url' in l or 'Streamable HTTP URL' in l:
lo = max(1, i-20)
hi = min(len(lines), i+30)
print(f"\n## around line {i}")
for j in range(lo, hi+1):
print(f"{j:5} {lines[j-1]}")
PY
printf '\n--- registry.rs remote_url references ---\n'
python3 - <<'PY'
from pathlib import Path
p = Path('crates/gateway-registry/src/registry.rs')
lines = p.read_text().splitlines()
for i,l in enumerate(lines,1):
if 'remote_url' in l or 'RegistryHit' in l or 'streamable-http' in l or 'sse' in l:
lo = max(1, i-12)
hi = min(len(lines), i+20)
print(f"\n## around line {i}")
for j in range(lo, hi+1):
print(f"{j:5} {lines[j-1]}")
PY
printf '\n--- tests referencing remote_url or transport ---\n'
rg -n "remote_url|streamable-http|sse|remotes" crates/gateway-registry/src crates/gateway-registry/tests -SRepository: getappz/agentflare
Length of output: 27060
🌐 Web query:
MCP Registry remote servers streamable-http sse remotes type documentation
💡 Result:
In the Model Context Protocol (MCP) registry, remote servers are configured using a server.json file which includes a remotes property to define how clients connect to the server [1][2]. This property accepts the type of transport, specifically streamable-http or sse [1][3]. Streamable HTTP (introduced in the 2025-03-26 protocol revision) is the modern, recommended transport for remote MCP servers [4][5]. It is designed to be stateless and uses a single HTTP endpoint, making it well-suited for load balancing and serverless deployment environments [5]. It replaces the older HTTP+SSE transport (from 2024-11-05), which has been deprecated [4]. SSE (Server-Sent Events) is an earlier transport mechanism that typically requires maintaining long-lived connections, which can be more complex to deploy behind standard web infrastructure compared to the single-endpoint streamable-http design [5]. Key documentation and technical resources: - Registry Configuration: The official MCP registry documentation details how to specify these transports within the server.json file [1][2]. It supports variable templating (e.g., for multi-tenant setups) within the URL property of the remotes configuration [2]. - Implementation: The MCP TypeScript SDK provides a WebStandardStreamableHTTPServerTransport class that allows servers to implement the Streamable HTTP specification using standard Web APIs [6]. - Migration: Current specifications advise that new implementations should use Streamable HTTP, while existing implementations should migrate away from the deprecated HTTP+SSE transport [4]. Clients may attempt to support both for backward compatibility by checking for specific HTTP status codes or endpoints, though this is considered legacy behavior [4]. Top results: [1][4][2][5]
Citations:
- 1: https://modelcontextprotocol.io/registry/remote-servers
- 2: https://github.com/modelcontextprotocol/registry/blob/9afbaacdfdf8966d73de09a795076fb0386c5c3d/docs/reference/server-json/generic-server-json.md
- 3: https://developer.konghq.com/catalog/mcp-registry/
- 4: https://mcp-staging.mintlify.app/specification/draft/basic/transports/streamable-http
- 5: https://agentscamp.com/guides/mcp/deploy-remote-mcp-server
- 6: https://ts.sdk.modelcontextprotocol.io/v2/api/@modelcontextprotocol/server/server/streamableHttp.html
Select the streamable-http remote server.remotes.into_iter().next() drops remote_type, so an sse entry can be surfaced as remote_url. Filter on remote_type == "streamable-http" or carry the transport with the URL.
🤖 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_search.rs` around lines 64 - 68, Update
the remote selection logic that consumes RegistryRemote so it selects only
entries with remote_type equal to "streamable-http" before deriving remote_url,
rather than taking the first remote unconditionally. Preserve the existing URL
handling for the selected streamable HTTP remote and avoid surfacing sse
entries.
| pub fn search_with_fallback( | ||
| conn: &Connection, | ||
| query: &str, | ||
| limit: usize, | ||
| mode: MatchMode, | ||
| ) -> Vec<ToolHit> { | ||
| let local = search(conn, query, limit, mode).unwrap_or_default(); | ||
| if local.len() >= limit { | ||
| return local; | ||
| } | ||
| let remaining = limit.saturating_sub(local.len()); | ||
| let registry = search_registry(query, remaining); | ||
| let mut results = local; | ||
| for hit in registry { | ||
| results.push(ToolHit { | ||
| server: hit.server, | ||
| tool: String::new(), | ||
| description: hit.description, | ||
| input_schema: Value::Null, | ||
| score: hit.score, | ||
| source: HitSource::Registry, | ||
| install_hint: hit.install_hint, | ||
| remote_url: hit.remote_url, | ||
| }); | ||
| } | ||
| results | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Preserve local search errors instead of treating them as zero hits.
Line 128 converts database failures into successful remote fallback results. This hides index corruption/query failures and prevents the gateway and MCP layers from using their existing error-conversion paths. Return rusqlite::Result<Vec<ToolHit>> and apply ? to search.
Proposed fix
pub fn search_with_fallback(
conn: &Connection,
query: &str,
limit: usize,
mode: MatchMode,
-) -> Vec<ToolHit> {
- let local = search(conn, query, limit, mode).unwrap_or_default();
+) -> rusqlite::Result<Vec<ToolHit>> {
+ let local = search(conn, query, limit, mode)?;
if local.len() >= limit {
- return local;
+ return Ok(local);
}
...
- results
+ Ok(results)
}📝 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.
| pub fn search_with_fallback( | |
| conn: &Connection, | |
| query: &str, | |
| limit: usize, | |
| mode: MatchMode, | |
| ) -> Vec<ToolHit> { | |
| let local = search(conn, query, limit, mode).unwrap_or_default(); | |
| if local.len() >= limit { | |
| return local; | |
| } | |
| let remaining = limit.saturating_sub(local.len()); | |
| let registry = search_registry(query, remaining); | |
| let mut results = local; | |
| for hit in registry { | |
| results.push(ToolHit { | |
| server: hit.server, | |
| tool: String::new(), | |
| description: hit.description, | |
| input_schema: Value::Null, | |
| score: hit.score, | |
| source: HitSource::Registry, | |
| install_hint: hit.install_hint, | |
| remote_url: hit.remote_url, | |
| }); | |
| } | |
| results | |
| } | |
| pub fn search_with_fallback( | |
| conn: &Connection, | |
| query: &str, | |
| limit: usize, | |
| mode: MatchMode, | |
| ) -> rusqlite::Result<Vec<ToolHit>> { | |
| let local = search(conn, query, limit, mode)?; | |
| if local.len() >= limit { | |
| return Ok(local); | |
| } | |
| let remaining = limit.saturating_sub(local.len()); | |
| let registry = search_registry(query, remaining); | |
| let mut results = local; | |
| for hit in registry { | |
| results.push(ToolHit { | |
| server: hit.server, | |
| tool: String::new(), | |
| description: hit.description, | |
| input_schema: Value::Null, | |
| score: hit.score, | |
| source: HitSource::Registry, | |
| install_hint: hit.install_hint, | |
| remote_url: hit.remote_url, | |
| }); | |
| } | |
| Ok(results) | |
| } |
🤖 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/search.rs` around lines 122 - 148, Update
search_with_fallback to return rusqlite::Result<Vec<ToolHit>> instead of
Vec<ToolHit>, and propagate failures from search with ?. Keep the existing
local-result limit check and registry fallback behavior for successful searches,
returning the assembled results through the Result type.
| results.push(SkillHit { | ||
| name: hit.server, | ||
| source: String::new(), | ||
| description: hit.description, | ||
| est_tokens: 0, | ||
| compressed: false, | ||
| score: hit.score, | ||
| install_hint: hit.install_hint.map(|h| { | ||
| if let Some(runtime) = h.runtime_hint { | ||
| format!("{} {}", runtime, h.identifier) | ||
| } else { | ||
| format!("{}:{}", h.registry_type, h.identifier) | ||
| } | ||
| }), | ||
| remote_url: hit.remote_url, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Mark fallback hits as registry-sourced.
source: String::new() drops provenance for remote results. Clients cannot reliably distinguish these non-loadable registry entries from local skills; emit a stable registry source value.
Suggested change
- source: String::new(),
+ source: "mcp_registry".to_string(),📝 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.
| results.push(SkillHit { | |
| name: hit.server, | |
| source: String::new(), | |
| description: hit.description, | |
| est_tokens: 0, | |
| compressed: false, | |
| score: hit.score, | |
| install_hint: hit.install_hint.map(|h| { | |
| if let Some(runtime) = h.runtime_hint { | |
| format!("{} {}", runtime, h.identifier) | |
| } else { | |
| format!("{}:{}", h.registry_type, h.identifier) | |
| } | |
| }), | |
| remote_url: hit.remote_url, | |
| results.push(SkillHit { | |
| name: hit.server, | |
| source: "mcp_registry".to_string(), | |
| description: hit.description, | |
| est_tokens: 0, | |
| compressed: false, | |
| score: hit.score, | |
| install_hint: hit.install_hint.map(|h| { | |
| if let Some(runtime) = h.runtime_hint { | |
| format!("{} {}", runtime, h.identifier) | |
| } else { | |
| format!("{}:{}", h.registry_type, h.identifier) | |
| } | |
| }), | |
| remote_url: hit.remote_url, |
🤖 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/skill-registry/src/search.rs` around lines 97 - 111, Update the
SkillHit construction in the search result mapping so fallback or remote
registry hits use the stable registry source value instead of an empty string.
Change only the source field while preserving the existing name, metadata, and
install_hint mapping.
|
Re-reviewing (same commit `67c06d74` I looked at earlier as the task/60 worktree, now formalized here) — none of my prior findings have changed since the code is byte-identical:
Also: this PR is now genuinely conflicting with `master` (`mergeStateStatus: DIRTY`) — `master` picked up the FTS5/BM25 consolidation into `flare-search-kit` (#189) which touched the same `search.rs` files this PR modifies. Will need a rebase/merge regardless of the above. Recommend fixing #1 (timeout + don't hold the lock across I/O) before merge. |
- Don't hold any lock (skills_registry mutex or gateway Registry's conn mutex) across the fallback HTTP call to registry.modelcontextprotocol.io: local search now runs and releases its lock before the network fallback fires, in both the skill and tool MCP actions. - Add a 3s timeout to the registry HTTP request so a slow/unreachable registry can't hang a search indefinitely. - Wire the fallback into tool_search (the "tool"/"search" MCP action) too, matching the item's original scope -- gateway_registry::Registry's fallback method existed but was dead code, never called anywhere. - Fix score semantics: local bm25() scores are ascending (lower = better, no DESC), but registry hits got a flat 0.5 with a comment claiming that ranked them below local results -- backwards, since 0.5 > every negative local score. Registry hits now score f64::MAX, so they always sort last under the same ascending convention if the merged list is ever re-sorted. - Resolve merge conflicts from master's flare-search-kit consolidation (#189) by rebuilding gateway-registry/search.rs and skill-registry's merge logic against the new shared MatchMode/fts_query/clamped_limit.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/gateway-registry/src/registry_search.rs (1)
117-117: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRemote selection still ignores transport type.
server.remotes.into_iter().next().map(|r| r.url)picks the first remote regardless ofremote_type, so ansseentry can be surfaced asremote_urleven though the field is documented as a Streamable HTTP URL. Flagged on a previous commit; still unaddressed.🐛 Proposed fix
- let remote_url = server.remotes.into_iter().next().map(|r| r.url); + let remote_url = server + .remotes + .into_iter() + .find(|r| r.remote_type == "streamable-http") + .map(|r| r.url);🤖 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_search.rs` at line 117, Update remote selection at the `server.remotes` assignment to filter entries by the supported Streamable HTTP transport type before extracting `url`, rather than taking the first remote unconditionally. Preserve `remote_url` as `None` when no matching remote exists.
♻️ Duplicate comments (1)
crates/gateway-registry/src/registry_search.rs (1)
9-9: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winStill using the unstable
/v0endpoint.
REGISTRY_BASEremains.../v0/servers. Per the MCP Registry docs,/v0is explicitly called unstable/deprecated in favor of/v0.1/servers; this was already flagged on a previous commit and remains unaddressed.🐛 Proposed fix
-const REGISTRY_BASE: &str = "https://registry.modelcontextprotocol.io/v0/servers"; +const REGISTRY_BASE: &str = "https://registry.modelcontextprotocol.io/v0.1/servers";🤖 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_search.rs` at line 9, Update the REGISTRY_BASE constant to use the stable MCP Registry `/v0.1/servers` endpoint instead of the deprecated `/v0/servers` path.
🤖 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/skill-registry/src/search.rs`:
- Around line 62-85: Update merge_registry_hits when constructing
registry-fallback SkillHit values so source is populated with the established
registry provenance identifier instead of an empty string. Preserve the existing
local-hit sources and all other fallback-hit mapping behavior.
In `@src/mcp_server.rs`:
- Around line 1969-1983: The registry fallback branch in tool() must not execute
synchronous search_registry on a Tokio worker thread. In src/mcp_server.rs lines
1969-1983, wrap the fallback lookup and registry hit merge in
tokio::task::spawn_blocking, await its result, and propagate both join and
search errors while preserving the existing local-hit and limit behavior; apply
the same handling at the sibling call site in src/mcp_server.rs lines 794-805.
---
Outside diff comments:
In `@crates/gateway-registry/src/registry_search.rs`:
- Line 117: Update remote selection at the `server.remotes` assignment to filter
entries by the supported Streamable HTTP transport type before extracting `url`,
rather than taking the first remote unconditionally. Preserve `remote_url` as
`None` when no matching remote exists.
---
Duplicate comments:
In `@crates/gateway-registry/src/registry_search.rs`:
- Line 9: Update the REGISTRY_BASE constant to use the stable MCP Registry
`/v0.1/servers` endpoint instead of the deprecated `/v0/servers` path.
🪄 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: d59f14ce-5002-4663-81a1-101016a53431
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
crates/gateway-registry/Cargo.tomlcrates/gateway-registry/src/lib.rscrates/gateway-registry/src/registry_search.rscrates/gateway-registry/src/search.rscrates/skill-registry/Cargo.tomlcrates/skill-registry/src/lib.rscrates/skill-registry/src/search.rssrc/mcp_server.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/skill-registry/Cargo.toml
| pub fn merge_registry_hits( | ||
| mut local: Vec<SkillHit>, | ||
| limit: usize, | ||
| registry: Vec<gateway_registry::registry_search::RegistryHit>, | ||
| ) -> Vec<SkillHit> { | ||
| let remaining = limit.saturating_sub(local.len()); | ||
| local.extend(registry.into_iter().take(remaining).map(|hit| SkillHit { | ||
| name: hit.server, | ||
| source: String::new(), | ||
| description: hit.description, | ||
| est_tokens: 0, | ||
| compressed: false, | ||
| score: gateway_registry::REGISTRY_FALLBACK_SCORE, | ||
| install_hint: hit.install_hint.map(|h| { | ||
| if let Some(runtime) = h.runtime_hint { | ||
| format!("{} {}", runtime, h.identifier) | ||
| } else { | ||
| format!("{}:{}", h.registry_type, h.identifier) | ||
| } | ||
| }), | ||
| remote_url: hit.remote_url, | ||
| })); | ||
| local | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Registry-fallback hits still lose provenance (source: String::new()).
Flagged on a previous commit and still present: local SkillHits carry a real source (e.g. "claude-user"), but registry-fallback hits get an empty string, so clients can't tell these apart from a local skill with a blank source.
🐛 Proposed fix
local.extend(registry.into_iter().take(remaining).map(|hit| SkillHit {
name: hit.server,
- source: String::new(),
+ source: "mcp_registry".to_string(),
description: hit.description,🤖 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/skill-registry/src/search.rs` around lines 62 - 85, Update
merge_registry_hits when constructing registry-fallback SkillHit values so
source is populated with the established registry provenance identifier instead
of an empty string. Preserve the existing local-hit sources and all other
fallback-hit mapping behavior.
| let limit = req.limit.unwrap_or(5); | ||
| let local = { | ||
| let guard = self.ensure_gateway_registry().await?; | ||
| let reg = guard.as_ref().expect("ensured above"); | ||
| reg.search(&query, limit, mode) | ||
| .map_err(|e| ErrorData::internal_error(e.to_string(), None))? | ||
| }; | ||
| let hits = if local.len() < limit { | ||
| let remaining = limit - local.len(); | ||
| let registry = | ||
| gateway_registry::registry_search::search_registry(&query, remaining); | ||
| gateway_registry::merge_registry_hits(local, limit, registry) | ||
| } else { | ||
| local | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== search_registry definitions and call sites ==\n'
rg -n "fn search_registry|search_registry\(" src | sed -n '1,200p'
printf '\n== mcp_server.rs around reported lines ==\n'
sed -n '760,830p' src/mcp_server.rs
printf '\n---\n'
sed -n '1940,2005p' src/mcp_server.rs
printf '\n== gateway_registry modules that may define search_registry ==\n'
fd -a "registry_search" src || true
fd -a "gateway_registry" src || trueRepository: getappz/agentflare
Length of output: 7012
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== registry_search implementation =='
file=$(fd -a "registry_search" src | head -n 1)
if [ -n "${file:-}" ]; then
wc -l "$file"
sed -n '1,220p' "$file"
fi
echo '== mcp_server.rs function signatures around the affected code =='
sed -n '740,815p' src/mcp_server.rs
echo '---'
sed -n '1940,1995p' src/mcp_server.rsRepository: getappz/agentflare
Length of output: 6551
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== search_registry occurrences across repo =='
rg -n "search_registry" .
echo
echo '== registry_search / ureq / blocking client hints =='
rg -n "ureq|reqwest::blocking|blocking::Client|timeout" . \
-g '!target' -g '!node_modules' -g '!dist' -g '!build'Repository: getappz/agentflare
Length of output: 22744
Offload the registry fallback lookup in tool() src/mcp_server.rs#L1969-L1983 still calls gateway_registry::registry_search::search_registry inline; that function uses synchronous ureq with a 3s timeout, so each miss can block a Tokio worker thread. Move this branch to tokio::task::spawn_blocking.
📍 Affects 1 file
src/mcp_server.rs#L1969-L1983(this comment)src/mcp_server.rs#L794-L805
🤖 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/mcp_server.rs` around lines 1969 - 1983, The registry fallback branch in
tool() must not execute synchronous search_registry on a Tokio worker thread. In
src/mcp_server.rs lines 1969-1983, wrap the fallback lookup and registry hit
merge in tokio::task::spawn_blocking, await its result, and propagate both join
and search errors while preserving the existing local-hit and limit behavior;
apply the same handling at the sibling call site in src/mcp_server.rs lines
794-805.
- Fix the MCP Registry endpoint: /v0/servers is not a real endpoint,
registry.modelcontextprotocol.io only serves /v0.1/servers -- the
fallback search has been silently hitting a dead URL and returning
nothing since it merged.
- Select only the "streamable-http" remote when picking remote_url,
instead of blindly taking the first one -- a deprecated HTTP+SSE
remote could be surfaced instead, which this codebase can't speak to.
Extracted select_remote_url() so this is unit-testable.
- Give skill-registry's fallback hits a real source ("mcp_registry")
instead of an empty string, so clients can tell them apart from a
local skill that happens to have a blank source.
- Move the registry HTTP call off the async runtime: skill()/tool()'s
search actions now run search_registry via tokio::task::spawn_blocking
instead of calling the synchronous ureq request inline. The prior fix
already stopped holding a lock across that call, but the main binary's
tokio runtime only enables the "rt" feature (single-threaded), so the
blocking call itself was still stalling every other in-flight request
for up to 3s, not just ones contending for the registry lock.
…ly mentions the item number (#603) * Fixed find_by_item_marker false-positive (item #190): added PullRequest.body field, filter search hits locally against the literal pr_footer suffix instead of trusting GitHub search phrase-match; regression test added; 17/17 pulls tests pass. Agentflare-Branch: task/190-duplicate-work-preflight-check-false-pos Agentflare-Item: 190-duplicate-work-preflight-check-false-pos * chore: retrigger CI (CodeQL runner failed to acquire a hosted runner, transient infra failure not a code issue) Agentflare-Agent: claude-code Agentflare-Branch: task/190-duplicate-work-preflight-check-false-pos Agentflare-Item: 190 Agentflare-Session: c5a4ab79-7ae7-4faf-b526-71ee9f9b5e37 * fix: remove duplicate PullRequest.body field GitHub's server-side branch-update merge silently produced a real compile error: item #70's fix (PR #608) and this item's own fix both independently added a body: Option<String> field to PullRequest for the same reason (marks_item-based ownership verification), at different insertion points. A textual 3-way merge can't detect that as a semantic duplicate -- it merged both additions cleanly with no conflict markers, leaving two body fields on the same struct (E0124/E0062). CI's earlier green build/clippy checkmarks predate this merge landing; the field duplication was never actually built against. Kept the single field (with the more complete doc comment), verified both call sites (github::pulls::marks_item/find_by_item_marker and worktree.rs's four PR-ownership checks) already use it identically via pr.body.as_deref() -- the two fixes are complementary, not conflicting, once deduplicated. Verified: cargo build/clippy/fmt clean against CI's exact invocations; 22 github::pulls + 14 worktree:: + 2 item_pr_failure + 246 mcp_server tests all pass. Agentflare-Branch: task/190-duplicate-work-preflight-check-false-pos Agentflare-Item: 190-duplicate-work-preflight-check-false-pos * chore: retrigger CI (ci workflow never fired for b341c3e — webhook delivery gap, not a code/filter issue) Agentflare-Agent: claude-code Agentflare-Branch: task/190-duplicate-work-preflight-check-false-pos Agentflare-Item: 190 Agentflare-Session: c5a4ab79-7ae7-4faf-b526-71ee9f9b5e37 --------- Co-authored-by: shiva <shiva@gosysinfo.tech>
1 participant
Auto-opened on
item donefor 019f6293-b426-7140-8e21-e38115f0ee11.Summary by CodeRabbit