Skip to content

fix: address CodeRabbit findings on registry-fallback search (#190 follow-up) - #195

Merged
getappz merged 2 commits into
masterfrom
fix/registry-fallback-coderabbit-followup
Jul 15, 2026
Merged

fix: address CodeRabbit findings on registry-fallback search (#190 follow-up)#195
getappz merged 2 commits into
masterfrom
fix/registry-fallback-coderabbit-followup

Conversation

@getappz

@getappz getappz commented Jul 15, 2026

Copy link
Copy Markdown
Owner

Summary

Follow-up to #190 (merged) addressing CodeRabbit review comments that weren't resolved before merge:

  • Major: the MCP Registry endpoint was /v0/servers, which doesn't exist — the real endpoint is /v0.1/servers. The registry-fallback search has been hitting a dead URL and silently returning nothing since Fallback MCP registry search #190 merged.
  • Remote selection now filters for remote_type == "streamable-http" instead of blindly taking the first remote (could previously surface a deprecated SSE remote this codebase can't speak to).
  • skill-registry's registry-fallback hits now get source: "mcp_registry" instead of an empty string, so callers can distinguish them from local skills.
  • The registry HTTP call now runs via tokio::task::spawn_blocking instead of inline on the async task — the main binary's tokio runtime only enables the rt feature (single-threaded), so the blocking ureq call was stalling every other in-flight request for up to 3s, not just ones needing the registry lock (which Fallback MCP registry search #190 already fixed).

Test plan

  • cargo test --workspace — all passing, new regression tests added (select_remote_url transport filtering, endpoint URL, mcp_registry provenance)
  • cargo clippy --workspace --all-targets -- -D warnings -A unsafe_code -A clippy::pedantic — clean
  • cargo fmt --check — clean

Summary by CodeRabbit

  • New Features
    • Enhanced fallback search reliability by switching to the stable registry endpoint.
    • Fallback results now prefer compatible streamable HTTP remotes.
    • Registry fallback results are clearly tagged with a distinct source label.
  • Bug Fixes
    • Remote registry lookups no longer block other server operations.
    • Remote search failures now gracefully fall back to empty results instead of disrupting merging behavior.

- 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.
@coderabbitai

coderabbitai Bot commented Jul 15, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: a25b7381-a686-4fc4-8b9f-568fcf8effd7

📥 Commits

Reviewing files that changed from the base of the PR and between 802ca09 and d728b4e.

📒 Files selected for processing (1)
  • src/mcp_server.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/mcp_server.rs

📝 Walkthrough

Walkthrough

The registry fallback now uses /v0.1/servers, selects streamable-http remotes, marks fallback hits with mcp_registry, and runs MCP registry searches asynchronously through spawn_blocking.

Changes

Registry fallback search

Layer / File(s) Summary
Stable endpoint and remote selection
crates/gateway-registry/src/registry_search.rs
Registry searches use /v0.1/servers and select a streamable-http remote, with tests for selection and endpoint configuration.
Fallback hit provenance
crates/skill-registry/src/search.rs
Merged registry hits now set SkillHit.source to mcp_registry, with corresponding test coverage.
Asynchronous MCP searches
src/mcp_server.rs
The skill tool is asynchronous, and both skill and tool registry fallbacks use awaited spawn_blocking calls; affected tests are asynchronous.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant AgentflareMcp
  participant spawn_blocking
  participant search_registry
  AgentflareMcp->>spawn_blocking: run registry search
  spawn_blocking->>search_registry: search query and result limit
  search_registry-->>spawn_blocking: registry hits or failure
  spawn_blocking-->>AgentflareMcp: awaited fallback results
Loading

Possibly related PRs

  • getappz/agentflare#92: Introduced the skill-registry search types and MCP skill-search integration used by the fallback metadata change.
  • getappz/agentflare#190: Introduced the fallback registry search logic updated here.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly reflects the follow-up fixes to registry-fallback search and CodeRabbit review comments.
Description check ✅ Passed The summary and test plan are present, but the reviewer notes section is missing and the clippy command differs from the template.
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.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/registry-fallback-coderabbit-followup

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

@getappz
getappz merged commit 6499361 into master Jul 15, 2026
14 checks passed
@getappz
getappz deleted the fix/registry-fallback-coderabbit-followup branch July 15, 2026 09:24
getappz added a commit that referenced this pull request Jul 15, 2026
- Backfill items_fts for items that already existed before this
  migration ran -- the AFTER INSERT/UPDATE/DELETE triggers only keep
  the index in sync going forward, so without a backfill every item
  created before this migration would be permanently invisible to
  item(search) until it happened to be touched again.
- Clamp item::search's limit via flare_search_kit::clamped_limit instead
  of a raw `limit as i64` cast -- usize::MAX (or any caller-supplied
  value that wraps negative on cast) turns into SQL "LIMIT -1", which
  SQLite treats as unlimited, defeating the cap. Same class of bug this
  codebase already guards against in gateway-registry/skill-registry's
  own search() via the same helper.
- Fix the limit field's schema description, which said "omit for no
  limit" -- true for item(list), but item(search) defaults to 20
  (clamped to 1000) when omitted, not unlimited.
- Resolve merge conflicts against master's already-fixed compact.rs/
  memory/mcp.rs (this branch predates #192/#195 and carried duplicate,
  pre-fix copies of that code) by taking master's versions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant