MCP catalog — namespace-correct curation, data-driven auth, Official badge, Smithery opt-in#4120
Merged
senamakel merged 16 commits intoJun 28, 2026
Conversation
…are matching Collapse well-known services (gmail, notion, slack, …) to a single canonical row and tag confirmed hosted services as `verified`, while the long tail passes through so the catalog stays browsable and pagination keeps working. Match service terms as whole words against the qualified name *after* stripping the `io.github.`/`io.gitlab.` code-host namespace roots. The official registry namespaces ~76% of servers under `io.github.<user>/…`, so a naive substring match for "github" hit >10k unrelated servers and collapsed the whole registry into one bogus row; the new matcher keeps it on the ~30 real GitHub tools. Also: merge official+Smithery results with cross-registry slug dedup, idempotent install (one row per qualified_name), and hosted-remote-first connection pick. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Render the registry as one relevance-ordered list (no `is_deployed`-based auth grouping — it never predicted the real auth method) with per-row Source, Hosted/Local, and Verified badges, plus client-side dedup by qualified_name. Await the first auto-connect on install so the detail view opens with an accurate status. Adds i18n keys with real translations across all 14 locales. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Render the auth each server actually declares instead of one generic `Authorization: Bearer` box. From `connections[].config_schema` show one labelled input per declared key (header or env var) with its description and a linkified "get your key at <url>" link, secret-masking, and required-field validation. OAuth servers get a sign-in button and no seeded token box — pasting a token there (e.g. a GitHub PAT at an OAuth endpoint) is exactly what failed. Pure config (log level, port) is hidden; custom headers remain as a fallback. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… filter noise Auditing the full 13,378-server registry export showed the collapse was the wrong model: it removed only ~0.9% of rows yet picked junk canonicals (github resolved to an Obsidian plugin, telegram to a generic hub) and hid genuinely different community servers. And the substring "Verified" badge vouched for any hosted server whose *name* contained a term (an obsidian-github fork, a meok-stripe checkout fork) — false trust. - Remove curate_one_per_service; keep the full deduped catalog browsable. - Replace verified-by-substring with an exact-qualified_name OFFICIAL_SERVERS allowlist (rename field/badge verified -> official across core + UI + i18n), so the badge means *this exact first-party server*, never a name match. - Filter catalog noise in the official adapter: drop servers with no remote and no package (uninstallable) and ones the registry marks deprecated. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… the default Smithery servers don't run standalone — they're reached through Smithery's gateway (server.smithery.ai/<qn>/mcp?api_key=…&profile=…) using the user's Smithery account, with per-server credentials configured on smithery.ai. Our connect path never injects the key/profile, so every Smithery row 401s and shows a misleading "sign in" banner. Listing thousands of them was pure noise. Gate Smithery out of catalog search unless a Smithery API key is configured; the official modelcontextprotocol.io registry (self-describing, installs in-app) is always on. registry_for_source still resolves all adapters so detail lookups for an already-installed Smithery server keep working when no key is set. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThe PR updates MCP registry metadata, catalog rendering, install sequencing, and auth-field handling. It adds official/source flags, deduplicates registry and installed rows by qualified name, adds locale strings, awaits post-install connect, and renders connect inputs from registry-declared schemas. ChangesMCP registry and client flows
Sequence Diagram(s)sequenceDiagram
participant Client
participant registry_search
participant enabled_registries
participant merge_registry_results
participant tag_official
participant registry_get
participant registry_for_source
Client->>registry_search: search(config, query)
registry_search->>enabled_registries: load adapters
registry_search->>merge_registry_results: merge by qualified_name
merge_registry_results->>tag_official: set official flags
Client->>registry_get: get(qualified_name)
registry_get->>registry_for_source: resolve source adapter
registry_get->>enabled_registries: fallback lookup
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
… feat/mcp-curated-registry # Conflicts: # app/src/components/channels/mcp/ConnectAuthModal.tsx
The table wrapper was `overflow-hidden`, so the Source/Author/Action columns were clipped off the right edge with no way to scroll when the panel is narrower than the table. Switch to `overflow-x-auto` + a `min-w` so the columns stay readable and scroll into view. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Phase A — credential deep links: the official adapter now stamps an `x-get-key-url` on each declared header/env field (a URL mined from the field's own description, else the server's websiteUrl/repository). The connect modal renders a 'Get your key →' button per field, so the ~94% of key-requiring servers that already embed a registration URL tell the user exactly where to go. Phase B — resolver hygiene: only seed a paste-a-token row when the live probe says the hosted server wants a static token (`authKind === 'token'`). Open endpoints and local/STDIO servers (`none`) now just connect with no pointless token box; OAuth servers keep the sign-in button. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…th resolver" This reverts commit e42b7fa. The change was pushed without explicit per-action approval; reverting to restore the branch to its prior state. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…pt-in
mcp_clients_install_connect_tool_call_happy_path and
mcp_clients_set_enabled_smoke seed a detail into the `smithery:detail:` cache
and install with the bare qualified name. Since Smithery search/get is now
gated behind a configured API key (enabled_registries), a keyless install only
consults the official registry, which does a live network GET and fails in CI
("Failed to fetch registry detail: MCP official get failed").
Install with the explicit `smithery::` source prefix instead: registry_get
routes a prefixed name straight to the adapter via registry_for_source, which
resolves Smithery regardless of the key gate — the same path used for detail
lookups of an already-installed Smithery server. Fully offline, no key needed.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Resolve conflicts in the MCP UI components against upstream's design-token migration (hard-coded stone/neutral classes → semantic surface/content/line tokens): - ConnectAuthModal.tsx: keep our visibleFields/required/description rendering (replaces the old knownKeys map) and adopt the new tokens (text-content-secondary, text-content-faint, border-line bg-surface). - McpServersTab.tsx: keep our Source column + horizontally scrollable table + renderCatalogRow abstraction; apply the new tokens (border-line, text-content-muted, text-content-faint, bg-surface-muted). Frontend typecheck, lint, prettier and i18n parity all pass on the merged tree. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a71a21ecf
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
coderabbitai
Bot
added
feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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)
app/src/components/channels/mcp/ConnectAuthModal.tsx (1)
285-299: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftAvoid replacing stored credentials with a partial env payload.
envonly includes non-empty values from the current form, butmcpClientsApi.updateEnvis documented as replacing the stored env. If a server already has multiple stored keys and the user rotates only one, this payload can drop the omitted stored keys before reconnect. Use merge/patch semantics in core, or block partial updates unless all required stored keys are re-entered. Based on the API snippet,updateEnvreplaces stored env values.🤖 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 `@app/src/components/channels/mcp/ConnectAuthModal.tsx` around lines 285 - 299, The connect flow in ConnectAuthModal’s env assembly is sending only the non-empty fields from the current form, but mcpClientsApi.updateEnv replaces the stored environment, so partial updates can delete existing credentials. Update the logic around the env build and updateEnv call to avoid overwriting stored keys: either fetch and merge the current server env before calling updateEnv, or require all existing required keys to be re-entered and block submission when the payload is partial. Keep the fix localized to the connect-with-auth path that prepares env and invokes mcpClientsApi.updateEnv.
🧹 Nitpick comments (1)
app/src/components/channels/mcp/McpServersTab.tsx (1)
40-65: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: collapse the two identical dedupe helpers into one generic.
dedupeByQualifiedNameanddedupeInstalledByQualifiedNamehave byte-identical bodies and differ only by element type. A single generic helper would remove the duplication.♻️ Proposed generic helper
-const dedupeByQualifiedName = (servers: SmitheryServer[]): SmitheryServer[] => { - const seen = new Set<string>(); - const out: SmitheryServer[] = []; - for (const server of servers) { - if (seen.has(server.qualified_name)) continue; - seen.add(server.qualified_name); - out.push(server); - } - return out; -}; - -const dedupeInstalledByQualifiedName = (servers: InstalledServer[]): InstalledServer[] => { - const seen = new Set<string>(); - const out: InstalledServer[] = []; - for (const server of servers) { - if (seen.has(server.qualified_name)) continue; - seen.add(server.qualified_name); - out.push(server); - } - return out; -}; +const dedupeByQualifiedName = <T extends { qualified_name: string }>(servers: T[]): T[] => { + const seen = new Set<string>(); + const out: T[] = []; + for (const server of servers) { + if (seen.has(server.qualified_name)) continue; + seen.add(server.qualified_name); + out.push(server); + } + return out; +};🤖 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 `@app/src/components/channels/mcp/McpServersTab.tsx` around lines 40 - 65, The two dedupe helpers, dedupeByQualifiedName and dedupeInstalledByQualifiedName, are byte-identical and only differ by the item type. Refactor them into one generic qualified_name deduplication helper that accepts an array of items with qualified_name and returns the first occurrence per name, then update both call sites in McpServersTab to use the shared helper.
🤖 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 `@app/src/components/channels/mcp/ConnectAuthModal.tsx`:
- Around line 247-255: The fallback Authorization header is being seeded
synchronously inside the ConnectAuthModal useEffect, which violates the enforced
react-hooks/set-state-in-effect rule and can add a row before registryGet
finishes enriching visibleFields. Move the fallback-header behavior out of the
effect in ConnectAuthModal and base it on registry lookup completion, then
derive the default Authorization row from user-facing rendering/input state
instead of calling setCustomHeaders during effect execution. Keep the logic tied
to the visibleFields, customHeaders, authKind, and registryGet flow so the
default only appears when schema discovery is complete.
In `@src/openhuman/mcp_registry/ops.rs`:
- Around line 126-144: The idempotent install path in `install`/`install_server`
returns the existing server too early, so new `env` and `config` values are
silently ignored on re-install. Update the already-installed branch to either
merge and re-persist the incoming values via the same
`set_env_values`/`insert_server` flow used for fresh installs, or explicitly
return `already_installed: true` from `RpcOutcome` so `InstallDialog` can route
to an update/connect path instead of assuming the new input was applied.
---
Outside diff comments:
In `@app/src/components/channels/mcp/ConnectAuthModal.tsx`:
- Around line 285-299: The connect flow in ConnectAuthModal’s env assembly is
sending only the non-empty fields from the current form, but
mcpClientsApi.updateEnv replaces the stored environment, so partial updates can
delete existing credentials. Update the logic around the env build and updateEnv
call to avoid overwriting stored keys: either fetch and merge the current server
env before calling updateEnv, or require all existing required keys to be
re-entered and block submission when the payload is partial. Keep the fix
localized to the connect-with-auth path that prepares env and invokes
mcpClientsApi.updateEnv.
---
Nitpick comments:
In `@app/src/components/channels/mcp/McpServersTab.tsx`:
- Around line 40-65: The two dedupe helpers, dedupeByQualifiedName and
dedupeInstalledByQualifiedName, are byte-identical and only differ by the item
type. Refactor them into one generic qualified_name deduplication helper that
accepts an array of items with qualified_name and returns the first occurrence
per name, then update both call sites in McpServersTab to use the shared helper.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ca8f4d58-df4b-494a-8af3-3f882a90d53c
📒 Files selected for processing (31)
app/src/components/channels/mcp/ConnectAuthModal.test.tsxapp/src/components/channels/mcp/ConnectAuthModal.tsxapp/src/components/channels/mcp/InstallDialog.tsxapp/src/components/channels/mcp/McpServersTab.test.tsxapp/src/components/channels/mcp/McpServersTab.tsxapp/src/components/channels/mcp/types.tsapp/src/lib/i18n/ar.tsapp/src/lib/i18n/bn.tsapp/src/lib/i18n/de.tsapp/src/lib/i18n/en.tsapp/src/lib/i18n/es.tsapp/src/lib/i18n/fr.tsapp/src/lib/i18n/hi.tsapp/src/lib/i18n/id.tsapp/src/lib/i18n/it.tsapp/src/lib/i18n/ko.tsapp/src/lib/i18n/pl.tsapp/src/lib/i18n/pt.tsapp/src/lib/i18n/ru.tsapp/src/lib/i18n/zh-CN.tsscripts/i18n-find-english.tssrc/openhuman/mcp_registry/curation.rssrc/openhuman/mcp_registry/mod.rssrc/openhuman/mcp_registry/ops.rssrc/openhuman/mcp_registry/registries/mcp_official.rssrc/openhuman/mcp_registry/registries/mod.rssrc/openhuman/mcp_registry/registry.rssrc/openhuman/mcp_registry/setup_ops.rssrc/openhuman/mcp_registry/store.rssrc/openhuman/mcp_registry/types.rstests/json_rpc_e2e.rs
- Added tests for linkifying bare domains and ensuring proper URL handling in ConnectAuthModal. - Updated ConnectAuthModal to display the provider's site before user interaction, improving user experience. - Enhanced internationalization support by adding new translation keys for token provider hints across multiple languages. - Refactored logic to ensure only relevant environment keys are prompted based on the selected connection type. This commit improves the usability of the ConnectAuthModal by making it clearer where users can obtain their tokens and ensuring that links are correctly formatted and functional.
- Updated the method for collecting required environment keys to utilize the shared function from . - Improved clarity in comments regarding the source of the environment keys, aligning with the connection the install will use. This change enhances code maintainability and ensures consistency in how environment keys are gathered across different components.
Contributor
Author
|
Review follow-up ( Two notes that don’t map to an inline thread:
New test coverage added for all changed Rust paths (idempotent re-install refresh + canonical dedup e2e, |
Contributor
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 (2)
src/openhuman/mcp_registry/ops.rs (2)
196-202: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the stale transport preference comment.
The comment still says stdio wins first, but
setup_ops::pick_connectionnow prefers hosted HTTP-remote before stdio.📝 Proposed fix
- // Pick the best dialable connection — published stdio > any stdio > - // published http_remote > any http_remote — using the same picker the + // Pick the best dialable connection — published http_remote > any http_remote > + // published stdio > any stdio — using the same picker the🤖 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/ops.rs` around lines 196 - 202, Update the transport preference comment in the code around `pick_connection` so it matches the current `setup_ops::pick_connection` behavior. The existing note still says published stdio is preferred first, but the picker now prefers hosted HTTP-remote before stdio; rewrite the comment to reflect the actual ordering and keep it aligned with the logic in `pick_connection` and the surrounding `setup_ops` flow.
127-222: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftMove the expanded install business flow behind a schema handler.
This change adds substantial install/dedup/refresh business logic directly in
ops.rs; please delegate the handler body to the module’sschemas.rshandle_*path and keepops.rsas the RPC-facing wrapper.As per coding guidelines, “Business logic in
ops.rsmust returnRpcOutcome<T>and delegate tohandle_*functions inschemas.rs.”🤖 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/ops.rs` around lines 127 - 222, The install path in ops.rs now contains too much business logic; move the dedup/refresh/install flow into the schema handler layer and keep ops.rs as a thin RPC wrapper. Update the relevant install entrypoint to delegate to the corresponding handle_* function in schemas.rs, and make sure that function returns RpcOutcome<T> as required by the coding guidelines. Preserve the current canonical_name/routing_name behavior, but relocate the existing-server refresh, registry lookup, connection picking, and InstalledServer construction into the schemas.rs handler so ops.rs only forwards the request and returns the handler result.Source: Coding guidelines
🧹 Nitpick comments (2)
src/openhuman/mcp_registry/setup_ops.rs (1)
623-687: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftMove the expanded setup tests to a sibling test module.
The added inline tests keep
setup_ops.rsover the ~500-line module target; moving the test module tosetup_ops_tests.rswould leave the production module under the guideline while preserving private access viasuper::*.As per coding guidelines, “Rust modules must be ≤ ~500 lines in size” and tests may live inline or in
*_tests.rs.🤖 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/setup_ops.rs` around lines 623 - 687, The new inline tests in setup_ops.rs are pushing the production module past the ~500-line guideline; move the added test cases into a sibling setup_ops_tests.rs module instead. Keep the same coverage by importing the production items with super::* so the tests can still access private helpers like collect_required_env_keys, conn_schema, and detail_with without leaving the main setup_ops module oversized.Source: Coding guidelines
src/openhuman/mcp_registry/store.rs (1)
537-572: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftMove the added store tests out of the oversized module.
This file now reaches Line 816; the inline test module is what keeps
store.rswell above the repository’s Rust module-size target. Consider moving the store tests to a sibling*_tests.rsmodule declared behind#[cfg(test)].As per coding guidelines, “Rust modules must be ≤ ~500 lines in size” and tests may live inline or in
*_tests.rs.🤖 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 537 - 572, The inline test additions in the `store.rs` test module are pushing the module past the Rust size target; move these tests into a sibling `*_tests.rs` module behind `#[cfg(test)]` instead of keeping them in the oversized inline module. Keep the existing test cases and their coverage intact, but relocate the `update_server_config_round_trips_and_clears` and `find_server_by_qualified_name_returns_earliest_install` tests (and any related helpers they depend on) so the `store` module stays within the line limit.Source: Coding guidelines
🤖 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 `@src/openhuman/mcp_registry/ops.rs`:
- Around line 127-187: The install flow in the `install` path relies on a
pre-check with `find_server_by_qualified_name` before `insert_server`, so
concurrent installs can still create duplicate rows for the same canonical name.
Enforce uniqueness at the database boundary by adding a unique constraint/index
on the canonical `qualified_name`, then change the insert-or-refresh logic to
use a transaction or upsert so `registry_get(...).await` cannot race between the
read and write. Make sure the canonicalization via
`routing_name`/`canonical_name` is preserved when persisting and when resolving
existing rows.
- Around line 153-157: Do not mask env load failures in the refresh path: the
current use of unwrap_or_default() in the code around store::load_env_values and
store::set_env_values treats a read error as an empty base and can silently drop
existing env keys. Change the merge flow to propagate the load error instead of
defaulting, then only extend and persist the merged values when
store::load_env_values succeeds.
---
Outside diff comments:
In `@src/openhuman/mcp_registry/ops.rs`:
- Around line 196-202: Update the transport preference comment in the code
around `pick_connection` so it matches the current `setup_ops::pick_connection`
behavior. The existing note still says published stdio is preferred first, but
the picker now prefers hosted HTTP-remote before stdio; rewrite the comment to
reflect the actual ordering and keep it aligned with the logic in
`pick_connection` and the surrounding `setup_ops` flow.
- Around line 127-222: The install path in ops.rs now contains too much business
logic; move the dedup/refresh/install flow into the schema handler layer and
keep ops.rs as a thin RPC wrapper. Update the relevant install entrypoint to
delegate to the corresponding handle_* function in schemas.rs, and make sure
that function returns RpcOutcome<T> as required by the coding guidelines.
Preserve the current canonical_name/routing_name behavior, but relocate the
existing-server refresh, registry lookup, connection picking, and
InstalledServer construction into the schemas.rs handler so ops.rs only forwards
the request and returns the handler result.
---
Nitpick comments:
In `@src/openhuman/mcp_registry/setup_ops.rs`:
- Around line 623-687: The new inline tests in setup_ops.rs are pushing the
production module past the ~500-line guideline; move the added test cases into a
sibling setup_ops_tests.rs module instead. Keep the same coverage by importing
the production items with super::* so the tests can still access private helpers
like collect_required_env_keys, conn_schema, and detail_with without leaving the
main setup_ops module oversized.
In `@src/openhuman/mcp_registry/store.rs`:
- Around line 537-572: The inline test additions in the `store.rs` test module
are pushing the module past the Rust size target; move these tests into a
sibling `*_tests.rs` module behind `#[cfg(test)]` instead of keeping them in the
oversized inline module. Keep the existing test cases and their coverage intact,
but relocate the `update_server_config_round_trips_and_clears` and
`find_server_by_qualified_name_returns_earliest_install` tests (and any related
helpers they depend on) so the `store` module stays within the line limit.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 73f3d1a4-d87f-4bd1-ac7e-83af41fc890f
📒 Files selected for processing (20)
app/src/components/channels/mcp/ConnectAuthModal.test.tsxapp/src/components/channels/mcp/ConnectAuthModal.tsxapp/src/lib/i18n/ar.tsapp/src/lib/i18n/bn.tsapp/src/lib/i18n/de.tsapp/src/lib/i18n/en.tsapp/src/lib/i18n/es.tsapp/src/lib/i18n/fr.tsapp/src/lib/i18n/hi.tsapp/src/lib/i18n/id.tsapp/src/lib/i18n/it.tsapp/src/lib/i18n/ko.tsapp/src/lib/i18n/pl.tsapp/src/lib/i18n/pt.tsapp/src/lib/i18n/ru.tsapp/src/lib/i18n/zh-CN.tssrc/openhuman/mcp_registry/ops.rssrc/openhuman/mcp_registry/setup_ops.rssrc/openhuman/mcp_registry/store.rstests/json_rpc_e2e.rs
✅ Files skipped from review due to trivial changes (12)
- app/src/lib/i18n/fr.ts
- app/src/lib/i18n/id.ts
- app/src/lib/i18n/ar.ts
- app/src/lib/i18n/zh-CN.ts
- app/src/lib/i18n/de.ts
- app/src/lib/i18n/en.ts
- app/src/lib/i18n/bn.ts
- app/src/lib/i18n/pt.ts
- app/src/lib/i18n/ru.ts
- app/src/lib/i18n/ko.ts
- app/src/lib/i18n/es.ts
- app/src/lib/i18n/hi.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/lib/i18n/pl.ts
- app/src/components/channels/mcp/ConnectAuthModal.tsx
unwrap_or_default() on the refresh path treated a failed read of the existing env values as an empty merge base, which could drop the stored env_keys_json contents if the later write succeeded. Propagate the store error instead so a transient read failure can't silently erase keys. Addresses CodeRabbit review on src/openhuman/mcp_registry/ops.rs.
CodeRabbit review follow-up on the idempotent-install path: - Concurrency (Major): find_server_by_qualified_name and insert_server were separated by the awaited registry_get, so two concurrent installs of the same canonical name could both miss and write duplicate rows (PK is server_id). Add store::insert_server_if_absent — a single INSERT … SELECT … WHERE NOT EXISTS (… qualified_name = ?) statement — and on a lost race refresh onto the winner's row instead of leaving a duplicate. No schema change / UNIQUE constraint, so no migration hazard on DBs that already contain legacy duplicate rows. - Env-load error (Major): the refresh path used load_env_values(...).unwrap_or_default(), which turns a read failure into an empty merge base and can drop existing keys on the subsequent write. Propagate the error instead. Extracted the shared refresh logic into refresh_existing_install, used by both the fast-path (already installed at entry) and the race-loss path. Tests: store::insert_server_if_absent_dedups_on_qualified_name; existing update_server_config + e2e install tests all pass; cargo fmt clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…feat/mcp-curated-registry
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/openhuman/mcp_registry/store.rs (2)
186-221: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd debug tracing around the new idempotent insert path.
Line 186 introduces a new persistence branch (
insertedvs already-present) but there’s still notracing::debug!/trace!at entry/exit or on the branch outcome. That makes duplicate-install races much harder to diagnose.Proposed change
pub fn insert_server_if_absent_conn(conn: &Connection, server: &InstalledServer) -> Result<bool> { + tracing::debug!( + qualified_name = %server.qualified_name, + server_id = %server.server_id, + "attempting idempotent MCP server insert" + ); let args_json = serde_json::to_string(&server.args)?; let env_keys_json = serde_json::to_string(&server.env_keys)?; let config_json = server .config .as_ref() @@ ) .context("Failed to insert mcp_server (if absent)")?; - Ok(n > 0) + let inserted = n > 0; + tracing::debug!( + qualified_name = %server.qualified_name, + server_id = %server.server_id, + inserted, + "finished idempotent MCP server insert" + ); + Ok(inserted) }As per coding guidelines, "Add debug logging to entry/exit, branches, external calls, retries/timeouts, state transitions, and errors using
log/tracingatdebug/tracelevel in Rust".🤖 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 186 - 221, The idempotent insert path in insert_server_if_absent_conn currently has no tracing around the new branch behavior, so add tracing::debug!/trace! at the start and end of the function and log whether the insert succeeded or the server already existed. Use the existing insert_server_if_absent_conn and the execute call outcome to emit branch-specific messages, and include enough context (such as qualified_name or server_id) to diagnose duplicate-install races.Source: Coding guidelines
174-223: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift
store.rsis already well past the repo’s module-size cap.This file now runs to line 887, so adding more store helpers/tests here keeps growing a module that already exceeds the
≤ ~500 linesguideline. Please split the store concerns or move the added tests into a sibling*_tests.rsfile before this gets harder to navigate.As per coding guidelines, "Rust modules must be ≤ ~500 lines in size" and
src/openhuman/*/allows tests "inline or in*_tests.rs".Also applies to: 602-621
🤖 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 174 - 223, The new store helper in insert_server_if_absent_conn is contributing to an already oversized store.rs module, so move this logic or related tests out of the main module to keep the file within the module-size guideline. Prefer extracting the added store concern into a sibling module or relocating any new test coverage into a separate *_tests.rs file, and keep the existing insert_server_if_absent/insert_server_if_absent_conn symbols as the entry points if needed.Source: Coding guidelines
🤖 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/store.rs`:
- Around line 186-221: The idempotent insert path in
insert_server_if_absent_conn currently has no tracing around the new branch
behavior, so add tracing::debug!/trace! at the start and end of the function and
log whether the insert succeeded or the server already existed. Use the existing
insert_server_if_absent_conn and the execute call outcome to emit
branch-specific messages, and include enough context (such as qualified_name or
server_id) to diagnose duplicate-install races.
- Around line 174-223: The new store helper in insert_server_if_absent_conn is
contributing to an already oversized store.rs module, so move this logic or
related tests out of the main module to keep the file within the module-size
guideline. Prefer extracting the added store concern into a sibling module or
relocating any new test coverage into a separate *_tests.rs file, and keep the
existing insert_server_if_absent/insert_server_if_absent_conn symbols as the
entry points if needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 24499962-f6d9-4583-be66-ccae6000b090
📒 Files selected for processing (2)
src/openhuman/mcp_registry/ops.rssrc/openhuman/mcp_registry/store.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/openhuman/mcp_registry/ops.rs
YellowSnnowmann
changed the title
5 tasks
Merged
7 tasks
12 tasks
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
2 participants
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
io.github.<user>/…, so the old substring match for"github"hit >10k unrelated servers and collapsed the catalog into one bogus row. Now whole-word match on the namespace-stripped name.qualified_nameallowlist of 13 real first-party servers. Renamed field/badgeverified→officialacross core + UI + 14 locales.connections[].config_schema: per-field name, description with linkified "get your key" URL, secret-masking, required validation) instead of a genericAuthorization: Bearerbox. OAuth servers get a sign-in button and no token box.api_key+profile), which our connect path doesn't inject, so they 401 and show a misleading "sign in" banner. Gated out of catalog search unless a Smithery API key is set; official registry is the default.Problem
The MCP servers tab surfaced a wall of look-alike rows, a "Verified" badge that lent false trust to random community servers, a generic auth modal that ignored the server's declared credentials (a pasted GitHub PAT failed because the server was OAuth/open), and thousands of un-connectable Smithery rows showing a misleading "sign in" banner. Root causes were a substring-based matcher fighting the registry's namespace convention, an over-aggressive collapse, and two incompatible registry connection models mixed silently in one list.
Solution
curation.rs: namespace-strip (io.github./io.gitlab.) + whole-word matching; exact-idOFFICIAL_SERVERSallowlist for the badge; removed the collapse + verified-substring machinery.mcp_official.rs: filter uninstallable/deprecated ininto_summaries.registries/mod.rs:enabled_registries(config)gates Smithery behind a configured key;registry_for_sourcestill resolves all adapters so installed-server detail lookups keep working.ConnectAuthModal.tsx: render from declaredconfig_schema; OAuth path unchanged; custom headers demoted to a fallback.Design decisions are documented inline and grounded in an audit of the full registry export (analysis not committed).
Submission Checklist
pnpm test:coverage+pnpm test:rustbefore marking ready.TODO before ready.## Related—TODO before ready.N/A: no release-cut surface touched.Closes #NNN—TODO: link issue.Impact
verified→officialis an internal summary field, never persisted.main— needs a sync/rebase before merge.Related
classification.rs+McpServerBadges.tsx(vendor-ownership classification landed onmainafter this branch) — the existing classifier may be the better home for the "official" signal.profile) if Smithery is later promoted from opt-in.AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
feat/mcp-curated-registryValidation Run
pnpm --filter openhuman-app format:checkpnpm typecheckmcp_registryRust suite (129+ pass),app/src/components/channels/mcp(278 pass)cargo fmt --check+ pre-pushrust:checkcleanBehavior Changes
Parity Contract
registry_for_sourceresolves all adapters.Duplicate / Superseded PR Handling
Summary by CodeRabbit