diff --git a/app/src/components/channels/mcp/ConnectAuthModal.test.tsx b/app/src/components/channels/mcp/ConnectAuthModal.test.tsx index 926af0f346..2e0d03852d 100644 --- a/app/src/components/channels/mcp/ConnectAuthModal.test.tsx +++ b/app/src/components/channels/mcp/ConnectAuthModal.test.tsx @@ -282,4 +282,172 @@ describe('ConnectAuthModal', () => { // Non-fatal: the modal still renders and shows the declared key field. expect(await screen.findByLabelText('Authorization')).toBeInTheDocument(); }); + + it('renders a declared field from config_schema with a linkified description', async () => { + mockRegistryGet.mockResolvedValue({ + qualified_name: 'acme/test-server', + display_name: 'Test Server', + connections: [ + { + type: 'http', + config_schema: { + properties: { + 'X-API-Key': { + description: 'AnomalyArmor key. Generate at https://app.anomalyarmor.ai/api-key', + 'x-secret': true, + }, + }, + required: ['X-API-Key'], + }, + }, + ], + required_env_keys: [], + }); + render( {}} onConnected={() => {}} />); + // The exact declared key is rendered as a labelled input… + expect(await screen.findByLabelText('X-API-Key')).toBeInTheDocument(); + // …with its description, and the "get your key" URL becomes a clickable link. + expect(screen.getByText(/Generate at/)).toBeInTheDocument(); + fireEvent.click(screen.getByRole('button', { name: 'https://app.anomalyarmor.ai/api-key' })); + expect(mockOpenUrl).toHaveBeenCalledWith('https://app.anomalyarmor.ai/api-key'); + }); + + it('linkifies a bare-domain "get your key" hint (no scheme) and opens it as https', async () => { + // Registry copy often omits the scheme (e.g. the Apify-hosted GitHub server's + // "Free tier at console.apify.com"). The bare domain must still be a clickable + // link, or the user has no idea where the token comes from. + mockRegistryGet.mockResolvedValue({ + qualified_name: 'acme/test-server', + display_name: 'Test Server', + connections: [ + { + type: 'http', + config_schema: { + properties: { + 'X-API-Key': { + description: 'Apify API token. Free tier at console.apify.com', + 'x-secret': true, + }, + }, + required: ['X-API-Key'], + }, + }, + ], + required_env_keys: [], + }); + render( {}} onConnected={() => {}} />); + await screen.findByLabelText('X-API-Key'); + // The ↗ affordance is aria-hidden, so the link's accessible name is the + // bare domain; clicking opens it with an https scheme prepended. + const link = screen.getByRole('button', { name: 'console.apify.com' }); + fireEvent.click(link); + expect(mockOpenUrl).toHaveBeenCalledWith('https://console.apify.com'); + }); + + it('surfaces the HTTP-remote provider host up front and links to the provider site', async () => { + // A server that declares no auth schema but has a hosted endpoint: name the + // provider before the user clicks Connect and eats a 401 just to learn it. + mockRegistryGet.mockResolvedValue({ + qualified_name: 'acme/test-server', + display_name: 'Test Server', + connections: [{ type: 'http', deployment_url: 'https://mcp.lona.agency/mcp' }], + required_env_keys: [], + }); + render( {}} onConnected={() => {}} />); + await screen.findByRole('dialog'); + const host = await screen.findByRole('button', { name: 'mcp.lona.agency' }); + fireEvent.click(host); + // Links to the provider's site (leading `mcp.` dropped), not the raw endpoint. + expect(mockOpenUrl).toHaveBeenCalledWith('https://lona.agency'); + }); + + it('does not linkify file-like tokens in a description', async () => { + mockRegistryGet.mockResolvedValue({ + qualified_name: 'acme/test-server', + display_name: 'Test Server', + connections: [ + { + type: 'http', + config_schema: { + properties: { + 'X-API-Key': { + description: 'Put the key in config.json, then visit example.com', + 'x-secret': true, + }, + }, + required: ['X-API-Key'], + }, + }, + ], + required_env_keys: [], + }); + render( {}} onConnected={() => {}} />); + await screen.findByLabelText('X-API-Key'); + // The real domain is a link; the filename is left as plain prose. + expect(screen.getByRole('button', { name: 'example.com' })).toBeInTheDocument(); + expect(screen.queryByRole('button', { name: 'config.json' })).not.toBeInTheDocument(); + }); + + it('does not strip a two-label provider host down to a public suffix', async () => { + mockRegistryGet.mockResolvedValue({ + qualified_name: 'acme/test-server', + display_name: 'Test Server', + connections: [{ type: 'http', deployment_url: 'https://server.io/mcp' }], + required_env_keys: [], + }); + render( {}} onConnected={() => {}} />); + await screen.findByRole('dialog'); + const host = await screen.findByRole('button', { name: 'server.io' }); + fireEvent.click(host); + // `server.io` must stay intact — never reduced to the bare TLD `https://io`. + expect(mockOpenUrl).toHaveBeenCalledWith('https://server.io'); + }); + + it('offers a "Where do I get the token?" pointer that opens the config assistant', async () => { + mockDetectAuth.mockResolvedValue({ kind: 'none', grant_types: [] }); + render( {}} onConnected={() => {}} />); + await screen.findByRole('dialog'); + fireEvent.click(screen.getByRole('button', { name: /Where do I get the token/ })); + await waitFor(() => { + // The connect modal plus the stacked config-help modal. + expect(screen.getAllByRole('dialog').length).toBeGreaterThan(1); + }); + }); + + it('blocks Connect until a required declared field is filled', async () => { + mockRegistryGet.mockResolvedValue({ + qualified_name: 'acme/test-server', + display_name: 'Test Server', + connections: [ + { + type: 'http', + config_schema: { + properties: { 'X-API-Key': { 'x-secret': true } }, + required: ['X-API-Key'], + }, + }, + ], + required_env_keys: [], + }); + render( {}} onConnected={() => {}} />); + await screen.findByLabelText('X-API-Key'); + await act(async () => { + fireEvent.click(screen.getByRole('button', { name: 'Connect' })); + }); + // A clear, per-field error — not a silent failed connect. + await waitFor(() => { + expect(screen.getByText('"X-API-Key" is required')).toBeInTheDocument(); + }); + expect(mockUpdateEnv).not.toHaveBeenCalled(); + expect(mockConnect).not.toHaveBeenCalled(); + }); + + it('does not seed a token box for OAuth servers', async () => { + mockDetectAuth.mockResolvedValue({ kind: 'oauth', grant_types: ['authorization_code'] }); + render( {}} onConnected={() => {}} />); + // OAuth servers get a sign-in button and no auto-seeded Authorization row — + // pasting a token there is exactly what fails (e.g. a GitHub PAT vs OAuth). + await screen.findByRole('button', { name: 'Sign in with browser' }); + expect(screen.queryByDisplayValue('Authorization')).not.toBeInTheDocument(); + }); }); diff --git a/app/src/components/channels/mcp/ConnectAuthModal.tsx b/app/src/components/channels/mcp/ConnectAuthModal.tsx index e6956a2168..bc8ae78273 100644 --- a/app/src/components/channels/mcp/ConnectAuthModal.tsx +++ b/app/src/components/channels/mcp/ConnectAuthModal.tsx @@ -1,32 +1,33 @@ /** - * ConnectAuthModal — opened when the user clicks "Connect" on an installed MCP - * server. Lets the user supply auth before connecting: + * ConnectAuthModal — opened when the user clicks "Connect"/"Sign in" on an + * installed MCP server. It renders the auth the *server itself declares*, so + * each server asks for exactly what it needs instead of a one-size-fits-all box: * - * • Known fields — keys the registry declares as required (`required_env_keys`, - * e.g. an `Authorization` header for an HTTP-remote server) plus any keys - * already stored on the install. Rendered as secret inputs. - * • Custom headers — free-form name/value rows for servers whose registry - * metadata declares no auth (mislabelled remotes like inference.sh) where - * the user nonetheless has a token to paste (e.g. `Authorization: Bearer …`). + * • OAuth — when a live probe (`detect_auth`) sees a browser sign-in + * challenge, we show a single "Sign in" button and no token field. + * • Declared fields — the registry's `connections[].config_schema` lists each + * required input by name (`Authorization`, `X-API-Key`, `GITHUB_TOKEN`, …) + * with a human description that often carries a "get your key at " + * link. We render one labelled (secret) input per field and linkify the URL. + * • Custom headers — a free-form fallback for mislabelled remotes that declare + * no auth but still want a header (kept behind the declared fields). * - * On submit, non-empty values are persisted via `update_env` (which stores them - * encrypted and reconnects); for HTTP-remote installs each entry becomes a - * request header on connect (see core `build_http_auth`). When the user supplies - * nothing, we just connect — open servers need no auth. - * - * This is the upfront (non-reactive) auth step: it always appears on Connect so - * the user can set credentials before the first attempt, rather than only after - * a 401. + * Only fields the server marks `required` or `isSecret` are shown; pure config + * (log level, port…) is left to the server's defaults. On submit, non-empty + * values persist via `update_env` (stored encrypted, then reconnect); for + * HTTP-remote installs each entry becomes a request header (core + * `build_http_auth`). With nothing supplied we just connect — open servers need + * no auth. */ import debug from 'debug'; -import { useCallback, useEffect, useState } from 'react'; +import { type ReactNode, useCallback, useEffect, useMemo, useState } from 'react'; import { useT } from '../../../lib/i18n/I18nContext'; import { mcpClientsApi } from '../../../services/api/mcpClientsApi'; import { openUrl } from '../../../utils/openUrl'; import Button from '../../ui/Button'; import ConfigHelpModal from './ConfigHelpModal'; -import type { InstalledServer, McpTool } from './types'; +import type { InstalledServer, McpTool, SmitheryServerDetail } from './types'; const log = debug('mcp-clients:connect-auth'); @@ -37,6 +38,18 @@ interface ConnectAuthModalProps { onConnected: (tools: McpTool[]) => void; } +/** An auth input the server declares it needs. */ +interface AuthField { + /** Header or env-var name, e.g. `Authorization`, `X-API-Key`, `GITHUB_TOKEN`. */ + name: string; + /** Human hint from the registry (may contain a "get your key" URL). */ + description?: string; + /** Masked input + never echoed back. */ + secret: boolean; + /** Must be supplied (unless already stored on the install). */ + required: boolean; +} + interface CustomHeader { id: number; name: string; @@ -55,29 +68,135 @@ const applyScheme = (scheme: 'bearer' | 'raw', value: string): string => { return v; }; +/** Whether a field name is an `Authorization` header (offers a Bearer scheme). */ +const isAuthorizationField = (name: string): boolean => name.toLowerCase() === 'authorization'; + +/** Merge a field into a by-name map: keep the first description/secret seen, + * OR the `required` flag (any source marking it required wins). */ +const upsertField = (map: Map, f: AuthField): void => { + const prev = map.get(f.name); + if (!prev) { + map.set(f.name, f); + return; + } + map.set(f.name, { + name: f.name, + description: prev.description ?? f.description, + secret: prev.secret || f.secret, + required: prev.required || f.required, + }); +}; + +/** Extract declared auth fields from a registry detail's connection schemas + * (and its flattened `required_env_keys`, for back-compat). */ +const fieldsFromDetail = (detail: SmitheryServerDetail): AuthField[] => { + const map = new Map(); + for (const conn of detail.connections ?? []) { + const schema = conn.config_schema as + | { + properties?: Record; + required?: string[]; + } + | undefined; + const props = schema?.properties; + const required = Array.isArray(schema?.required) ? schema!.required! : []; + if (props && typeof props === 'object') { + for (const [name, prop] of Object.entries(props)) { + upsertField(map, { + name, + description: prop?.description, + secret: prop?.['x-secret'] === true, + required: required.includes(name), + }); + } + } + } + for (const name of detail.required_env_keys ?? []) { + upsertField(map, { name, secret: true, required: true }); + } + return Array.from(map.values()); +}; + +// Linkify the "get your key at " hint registries put in field +// descriptions. Matches full http(s)/`www.` URLs AND bare domains like +// `console.apify.com` — registry copy frequently omits the scheme, and a bare +// domain rendered as grey text reads as prose, leaving the user with no idea it +// is where the token comes from. The bare-domain arm requires a 2–24 letter TLD +// so version strings (`v1.2`) and abbreviations (`e.g.`) are not linkified. +const URL_BODY = + '(?:https?:\\/\\/|www\\.)[^\\s)]+|(?:[a-z0-9](?:[a-z0-9-]*[a-z0-9])?\\.)+[a-z]{2,24}(?:\\/[^\\s)]*)?'; +const URL_SPLIT_RE = new RegExp(`(${URL_BODY})`, 'gi'); +const URL_MATCH_RE = new RegExp(`^(?:${URL_BODY})$`, 'i'); + +// Common file extensions that masquerade as a bare domain (`config.json`, +// `Node.js`, `report.pdf`). A schemeless, path-less token ending in one of these +// is prose, not a link — don't turn "set this in config.json" into a dead link. +const FILE_EXT_RE = + /\.(?:json|jsonc|ya?ml|toml|lock|md|mdx|txt|csv|tsv|pdf|docx?|xlsx?|pptx?|png|jpe?g|gif|svg|webp|ico|mp[34]|mov|zip|tar|gz|tgz|rs|js|jsx|mjs|cjs|ts|tsx|py|rb|go|java|php|c|cpp|h|hpp|sh|bash|env|ini|cfg|conf|log|html?|css|scss|sass|xml|sql)$/i; + +/** Give a matched link a scheme so the OS opens it (bare domains → https). */ +const withScheme = (s: string): string => (/^https?:\/\//i.test(s) ? s : `https://${s}`); + +/** Whether a token matched by URL_SPLIT_RE is really a link worth rendering. + * Tokens with a scheme or a path are always links; a bare `name.ext` token + * whose extension is a known file type (`config.json`, `Node.js`) is not. */ +const isLinkLike = (s: string): boolean => { + if (!URL_MATCH_RE.test(s)) return false; + if (/^https?:\/\//i.test(s) || s.includes('/')) return true; + return !FILE_EXT_RE.test(s); +}; + +/** Host of the server's HTTP-remote endpoint, if the detail declares one — the + * provider the token comes from (and the host a 401 would name). */ +const hostFromDetail = (detail: SmitheryServerDetail): string | null => { + const url = detail.connections?.find( + c => c.type === 'http' && typeof c.deployment_url === 'string' && c.deployment_url.length > 0 + )?.deployment_url; + if (!url) return null; + try { + return new URL(url).host || null; + } catch { + return null; + } +}; + +/** Best-effort signup/site URL for a provider host: drop a leading + * `mcp.`/`api.`/`server.`/`www.` label so `mcp.lona.agency` → the provider's + * site `https://lona.agency` rather than the bare MCP endpoint. Only strips + * when ≥2 labels remain, so a two-label host like `server.io` is never reduced + * to a bare public suffix (`https://io`). */ +const providerUrlFromHost = (host: string): string => { + const stripped = host.replace(/^(?:mcp|api|server|www)\./i, ''); + const safe = stripped.split('.').length >= 2 ? stripped : host; + return `https://${safe}`; +}; + +/** Blank Authorization row offered as a starting point for servers that declare + * no auth schema. Rendered as a default rather than seeded into state from an + * effect (which `react-hooks/set-state-in-effect` forbids); the first edit + * materializes it into `customHeaders`. */ +const FALLBACK_HEADER: CustomHeader = { id: 0, name: 'Authorization', value: '', scheme: 'bearer' }; + const ConnectAuthModal = ({ server, onClose, onConnected }: ConnectAuthModalProps) => { const { t } = useT(); - // Declared/known keys: union of the registry's required keys and any keys - // already on the install. Seeded from the install immediately; refined by a - // best-effort registry_get (which also carries newly-declared headers). - // `__`-prefixed keys are internal bookkeeping (OAuth refresh bundle) — never - // render them as input fields. - const [knownKeys, setKnownKeys] = useState( - server.env_keys.filter(k => !k.startsWith('__')) + // Declared auth fields. Seeded from the install's stored keys (names only), + // then enriched by a best-effort registry_get that carries each field's + // description / secret / required metadata. `__`-prefixed keys are internal + // bookkeeping (OAuth refresh bundle) — never render them. + const [fields, setFields] = useState(() => + server.env_keys + .filter(k => !k.startsWith('__')) + .map(name => ({ name, secret: true, required: false })) ); const [values, setValues] = useState>({}); const [reveal, setReveal] = useState>({}); - // Per-declared-field scheme (the registry doesn't say Bearer vs raw — only a - // free-text description — so the user picks). Defaults to Bearer for an - // `Authorization` header (the overwhelming case), raw for anything else. - const [knownSchemes, setKnownSchemes] = useState>({}); - // Memoized on `knownSchemes` so callbacks that depend on it (e.g. - // `handleConnect`) re-derive the right scheme after the user flips the - // dropdown — a stale closure here would send the wrong prefix on submit. + // Per-Authorization-field scheme (the registry gives a free-text description, + // not Bearer-vs-raw — so the user picks). Defaults to Bearer. + const [authSchemes, setAuthSchemes] = useState>({}); const schemeFor = useCallback( - (key: string): 'bearer' | 'raw' => - knownSchemes[key] ?? (key.toLowerCase() === 'authorization' ? 'bearer' : 'raw'), - [knownSchemes] + (name: string): 'bearer' | 'raw' => + authSchemes[name] ?? (isAuthorizationField(name) ? 'bearer' : 'raw'), + [authSchemes] ); const [customHeaders, setCustomHeaders] = useState([]); const [nextId, setNextId] = useState(1); @@ -87,11 +206,19 @@ const ConnectAuthModal = ({ server, onClose, onConnected }: ConnectAuthModalProp // the token/header fields. `detecting` until the probe returns. const [authKind, setAuthKind] = useState<'detecting' | 'none' | 'token' | 'oauth'>('detecting'); const [oauthWaiting, setOauthWaiting] = useState(false); - // Opens the stacked configuration-help chat modal. const [showConfigHelp, setShowConfigHelp] = useState(false); + // Host of the server's HTTP-remote endpoint (from the registry detail's + // deployment_url). Surfaced as a "get your token from this provider" hint so + // the user learns where the credential comes from BEFORE a 401 round-trip — + // the same host the failed-connect error reveals, shown up front. + const [endpointHost, setEndpointHost] = useState(null); + + // Only surface fields the server actually wants from the user: required + // inputs and secrets. Pure config (log level, port, …) keeps server defaults. + const visibleFields = useMemo(() => fields.filter(f => f.required || f.secret), [fields]); // Probe how this server authenticates so we render the right control. The - // registry can't tell us (it mislabels), so we ask the server. + // registry can't always tell us, so we ask the server. useEffect(() => { let cancelled = false; void (async () => { @@ -151,18 +278,23 @@ const ConnectAuthModal = ({ server, onClose, onConnected }: ConnectAuthModalProp })(); }, [server.server_id, onConnected, onClose, t]); - // Best-effort: pull the registry's declared required keys so a server that - // *labels* its auth (e.g. `Authorization`) shows that field even if it was - // installed with no env. Network failures are non-fatal — we fall back to the - // install's own env_keys and the custom-header rows. + // Best-effort: pull the registry's declared fields (names + descriptions + + // secret/required), so a server that labels its auth shows tailored inputs. + // Network failures are non-fatal — we keep the install's own keys. useEffect(() => { let cancelled = false; void (async () => { try { const detail = await mcpClientsApi.registryGet(server.qualified_name); if (cancelled) return; - const declared = detail.required_env_keys ?? []; - setKnownKeys(prev => Array.from(new Set([...prev, ...declared]))); + setEndpointHost(hostFromDetail(detail)); + const declared = fieldsFromDetail(detail); + setFields(prev => { + const map = new Map(); + for (const f of prev) upsertField(map, f); + for (const f of declared) upsertField(map, f); + return Array.from(map.values()); + }); } catch (err) { log('registry_get failed (non-fatal): %s', err instanceof Error ? err.message : err); } @@ -172,13 +304,17 @@ const ConnectAuthModal = ({ server, onClose, onConnected }: ConnectAuthModalProp }; }, [server.qualified_name]); - // Seed a blank custom-header row when the server declares nothing, so the - // user has an obvious place to paste a token (the mislabelled-remote case). - useEffect(() => { - if (knownKeys.length === 0 && customHeaders.length === 0) { - setCustomHeaders([{ id: 0, name: 'Authorization', value: '', scheme: 'bearer' }]); - } - }, [knownKeys.length, customHeaders.length]); + // Offer a blank custom-header row only when the server declares nothing AND + // isn't an OAuth sign-in — i.e. the mislabelled-remote case where the user + // nonetheless has a token to paste. OAuth servers get the sign-in button, not + // a token box (this is what stops "paste a token and hope" failures). Derived + // rather than seeded from an effect (forbidden by react-hooks/set-state-in- + // effect) and gated on `detecting` so the row never flashes before discovery. + const offerFallbackHeader = + visibleFields.length === 0 && authKind !== 'oauth' && authKind !== 'detecting'; + // What to render: real headers once the user has any, else the fallback row. + const displayHeaders = + customHeaders.length === 0 && offerFallbackHeader ? [FALLBACK_HEADER] : customHeaders; const addCustomHeader = useCallback(() => { setCustomHeaders(prev => [...prev, { id: nextId, name: '', value: '', scheme: 'bearer' }]); @@ -189,7 +325,29 @@ const ConnectAuthModal = ({ server, onClose, onConnected }: ConnectAuthModalProp setCustomHeaders(prev => prev.filter(h => h.id !== id)); }, []); + // Patch a header by id, materializing the fallback row into state on first + // edit so the user's input persists without ever seeding state in an effect. + const patchHeader = useCallback( + (id: number, patch: Partial) => + setCustomHeaders(prev => { + const base = prev.length === 0 && offerFallbackHeader ? [FALLBACK_HEADER] : prev; + return base.map(x => (x.id === id ? { ...x, ...patch } : x)); + }), + [offerFallbackHeader] + ); + const handleConnect = useCallback(() => { + // A required field is only "missing" when it's blank now AND wasn't already + // stored on the install (re-opening Connect shouldn't force re-entry). + const stored = new Set(server.env_keys); + const missing = visibleFields.find( + f => f.required && !stored.has(f.name) && !values[f.name]?.trim() + ); + if (missing) { + setError(t('mcp.install.missingRequired').replace('{key}', missing.name)); + return; + } + setBusy(true); setError(null); void (async () => { @@ -197,9 +355,9 @@ const ConnectAuthModal = ({ server, onClose, onConnected }: ConnectAuthModalProp // Build the env/header map: declared values + named custom headers, // skipping blanks so we never store empty keys. const env: Record = {}; - for (const key of knownKeys) { - const v = values[key]?.trim(); - if (v) env[key] = applyScheme(schemeFor(key), v); + for (const f of visibleFields) { + const v = values[f.name]?.trim(); + if (v) env[f.name] = applyScheme(schemeFor(f.name), v); } for (const h of customHeaders) { const name = h.name.trim(); @@ -230,7 +388,40 @@ const ConnectAuthModal = ({ server, onClose, onConnected }: ConnectAuthModalProp setBusy(false); } })(); - }, [knownKeys, values, customHeaders, schemeFor, server.server_id, onConnected, onClose, t]); + }, [ + visibleFields, + values, + customHeaders, + schemeFor, + server.server_id, + server.env_keys, + onConnected, + onClose, + t, + ]); + + // Render a field description, linkifying any URL or bare domain so the user + // can jump straight to the "get your key" page. Links are underlined and + // carry an ↗ affordance so they read as actionable, not as plain prose. + const renderDescription = useCallback( + (text: string): ReactNode => + text.split(URL_SPLIT_RE).map((part, i) => + isLinkLike(part) ? ( + + ) : ( + {part} + ) + ), + [] + ); return (
)} - {/* Declared / known fields */} - {knownKeys.length > 0 && ( + {/* Where-to-get-credentials hint. Shown for non-OAuth servers: it names + the provider host up front (from the registry's deployment_url) so + the user isn't sent on a 401 round-trip just to discover where the + token comes from, and offers the per-server config assistant. */} + {authKind !== 'oauth' && ( +
+ {endpointHost && ( +

+ {t('mcp.connectAuth.tokenProvider')}{' '} + +

+ )} + +
+ )} + + {/* Declared fields — one labelled input per key the server asks for. */} + {visibleFields.length > 0 && (

{t('mcp.connectAuth.requiredLabel')}

- {knownKeys.map(key => ( -
- + {visibleFields.map(field => ( +
+
+ + {field.required && ( + + )} +
+ {field.description && ( +

+ {renderDescription(field.description)} +

+ )}
- + {isAuthorizationField(field.name) && ( + + )} setValues(prev => ({ ...prev, [key]: e.target.value }))} - placeholder={t('mcp.install.enterValue').replace('{key}', key)} + id={`auth-${field.name}`} + type={field.secret && !reveal[field.name] ? 'password' : 'text'} + value={values[field.name] ?? ''} + onChange={e => setValues(prev => ({ ...prev, [field.name]: e.target.value }))} + placeholder={t('mcp.install.enterValue').replace('{key}', field.name)} disabled={busy} // Suppress Chromium password-manager autofill so a token saved // for one MCP doesn't pre-fill another's field. @@ -315,21 +552,25 @@ const ConnectAuthModal = ({ server, onClose, onConnected }: ConnectAuthModalProp data-form-type="other" className="flex-1 rounded-lg border border-line bg-surface px-3 py-1.5 text-xs text-content placeholder:text-stone-400 dark:placeholder:text-neutral-500 focus:outline-none focus:ring-2 focus:ring-primary-500/40 disabled:opacity-50" /> - + {field.secret && ( + + )}
))}
)} - {/* Custom headers */} + {/* Custom headers — free-form fallback for servers that declare no auth. */}

@@ -343,22 +584,18 @@ const ConnectAuthModal = ({ server, onClose, onConnected }: ConnectAuthModalProp {t('mcp.connectAuth.addHeader')}

- {customHeaders.length === 0 && ( + {displayHeaders.length === 0 && (

{t('mcp.connectAuth.customHeadersEmpty')}

)} - {customHeaders.map(h => ( + {displayHeaders.map(h => (
{/* Row 1: header name + scheme + remove */}
- setCustomHeaders(prev => - prev.map(x => (x.id === h.id ? { ...x, name: e.target.value } : x)) - ) - } + onChange={e => patchHeader(h.id, { name: e.target.value })} placeholder={t('mcp.connectAuth.headerName')} disabled={busy} autoComplete="off" @@ -369,13 +606,7 @@ const ConnectAuthModal = ({ server, onClose, onConnected }: ConnectAuthModalProp /> - setCustomHeaders(prev => - prev.map(x => (x.id === h.id ? { ...x, value: e.target.value } : x)) - ) - } + onChange={e => patchHeader(h.id, { value: e.target.value })} placeholder={t('mcp.connectAuth.headerValue')} disabled={busy} // Suppress Chromium password-manager autofill (token leakage diff --git a/app/src/components/channels/mcp/InstallDialog.tsx b/app/src/components/channels/mcp/InstallDialog.tsx index a17c1cd70c..dd7ecbf646 100644 --- a/app/src/components/channels/mcp/InstallDialog.tsx +++ b/app/src/components/channels/mcp/InstallDialog.tsx @@ -133,16 +133,22 @@ const InstallDialog = ({ qualifiedName, prefillEnv, onSuccess, onCancel }: Insta config: parsedConfig, }); log('install success server_id=%s', server.server_id); - void mcpClientsApi - .connect(server.server_id) - .then(() => log('auto-connect success server_id=%s', server.server_id)) - .catch((connectErr: unknown) => - log( - 'auto-connect failed server_id=%s: %s', - server.server_id, - connectErr instanceof Error ? connectErr.message : String(connectErr) - ) + // Await the first connect so the detail view opens with an accurate + // status rather than a stale "disconnected" that races the background + // attempt. A failed connect is expected for servers that need auth + // (OAuth sign-in / token) — we keep the install and let the detail view + // surface the error and prompt to connect, so we never treat a connect + // failure as an install failure. + try { + await mcpClientsApi.connect(server.server_id); + log('auto-connect success server_id=%s', server.server_id); + } catch (connectErr: unknown) { + log( + 'auto-connect failed server_id=%s: %s', + server.server_id, + connectErr instanceof Error ? connectErr.message : String(connectErr) ); + } onSuccess(server); } catch (err) { const msg = err instanceof Error ? err.message : t('mcp.install.failedInstall'); diff --git a/app/src/components/channels/mcp/McpServersTab.test.tsx b/app/src/components/channels/mcp/McpServersTab.test.tsx index 8f80e21f67..01f5dd8170 100644 --- a/app/src/components/channels/mcp/McpServersTab.test.tsx +++ b/app/src/components/channels/mcp/McpServersTab.test.tsx @@ -134,6 +134,21 @@ describe('McpServersTab', () => { expect(screen.getByText('Author')).toBeInTheDocument(); }); + it('collapses duplicate installed servers to one row per qualified_name', async () => { + // Two legacy installs of the same service (distinct server_id, same slug). + const dupA = { ...SERVERS[0], server_id: 'srv-1', installed_at: 1 }; + const dupB = { ...SERVERS[0], server_id: 'srv-2', installed_at: 2 }; + mockInstalledList.mockResolvedValue([dupA, dupB]); + mockStatus.mockResolvedValue([]); + + await renderAndWaitForLoad(); + vi.useRealTimers(); + + await waitFor(() => expect(screen.getAllByText('File Server')).toHaveLength(1)); + // The Installed chip count reflects the collapsed list. + expect(screen.getByText('Installed (1)')).toBeInTheDocument(); + }); + it('renders filter chips — All, Installed, Registry', async () => { mockInstalledList.mockResolvedValue([]); mockStatus.mockResolvedValue([]); @@ -235,7 +250,12 @@ describe('McpServersTab', () => { mockStatus.mockResolvedValue([]); mockRegistrySearch.mockResolvedValue({ servers: [ - { qualified_name: 'acme/new-srv', display_name: 'New Server', description: 'A new server' }, + { + qualified_name: 'acme/new-srv', + display_name: 'New Server', + description: 'A new server', + official: true, + }, ], page: 1, total_pages: 1, @@ -250,11 +270,167 @@ describe('McpServersTab', () => { expect(screen.getByText('Install')).toBeInTheDocument(); }); + it('renders only one row per qualified_name when the registry returns duplicates', async () => { + mockInstalledList.mockResolvedValue([]); + mockStatus.mockResolvedValue([]); + mockRegistrySearch.mockResolvedValue({ + servers: [ + { qualified_name: 'acme/new-srv', display_name: 'New Server', official: true }, + { qualified_name: 'acme/new-srv', display_name: 'New Server', official: true }, + ], + page: 1, + total_pages: 1, + }); + + await renderAndWaitForLoad(); + vi.useRealTimers(); + + await waitFor(() => expect(screen.getByText('New Server')).toBeInTheDocument()); + expect(screen.getAllByText('New Server')).toHaveLength(1); + }); + + it('dedupes registry rows across "load more" pages that overlap', async () => { + mockInstalledList.mockResolvedValue([]); + mockStatus.mockResolvedValue([]); + mockRegistrySearch.mockImplementation(({ page }: { page: number }) => + Promise.resolve( + page === 1 + ? { + servers: [ + { qualified_name: 'acme/a', display_name: 'Server A', official: true }, + { qualified_name: 'acme/b', display_name: 'Server B', official: true }, + ], + page: 1, + total_pages: 2, + } + : { + // page 2 overlaps page 1 on acme/b and adds acme/c + servers: [ + { qualified_name: 'acme/b', display_name: 'Server B', official: true }, + { qualified_name: 'acme/c', display_name: 'Server C', official: true }, + ], + page: 2, + total_pages: 2, + } + ) + ); + + await renderAndWaitForLoad(); + vi.useRealTimers(); + + await waitFor(() => screen.getByText('Server A')); + fireEvent.click(screen.getByText('Load more')); + + await waitFor(() => expect(screen.getByText('Server C')).toBeInTheDocument()); + expect(screen.getAllByText('Server B')).toHaveLength(1); + }); + + it('distinguishes look-alike registry rows by slug and source badge', async () => { + mockInstalledList.mockResolvedValue([]); + mockStatus.mockResolvedValue([]); + mockRegistrySearch.mockResolvedValue({ + servers: [ + { + qualified_name: 'waystation/gmail', + display_name: 'gmail', + source: 'mcp_official', + official: true, + }, + { + qualified_name: 'mintmcp/gmail', + display_name: 'gmail', + source: 'smithery', + official: true, + }, + ], + page: 1, + total_pages: 1, + }); + + await renderAndWaitForLoad(); + vi.useRealTimers(); + + // Both rows share the display name "gmail"... + await waitFor(() => expect(screen.getAllByText('gmail')).toHaveLength(2)); + // ...but the unique slug and the registry-source badge tell them apart. + expect(screen.getByText('waystation/gmail')).toBeInTheDocument(); + expect(screen.getByText('mintmcp/gmail')).toBeInTheDocument(); + expect(screen.getByText('Official')).toBeInTheDocument(); + expect(screen.getByText('Smithery')).toBeInTheDocument(); + }); + + it('renders the registry as one list (no auth-method grouping) in registry order', async () => { + mockInstalledList.mockResolvedValue([]); + mockStatus.mockResolvedValue([]); + mockRegistrySearch.mockResolvedValue({ + servers: [ + { qualified_name: 'a/local-srv', display_name: 'Local One', is_deployed: false }, + { qualified_name: 'b/hosted-srv', display_name: 'Hosted One', is_deployed: true }, + ], + page: 1, + total_pages: 1, + }); + + await renderAndWaitForLoad(); + vi.useRealTimers(); + + await waitFor(() => screen.getByText('Hosted One')); + // The misleading auth-method group headers are gone — `is_deployed` does not + // predict whether a server uses browser sign-in or wants a pasted token, so + // we no longer split on it. The real requirement surfaces on install. + expect(screen.queryByText('Browser sign-in')).not.toBeInTheDocument(); + expect(screen.queryByText('Token / API key')).not.toBeInTheDocument(); + // Rows keep their registry order (relevance), regardless of transport. + const tableText = screen.getByRole('table').textContent ?? ''; + expect(tableText.indexOf('Local One')).toBeLessThan(tableText.indexOf('Hosted One')); + // Per-row Hosted/Local hint badges still render. + expect(screen.getByText('Hosted')).toBeInTheDocument(); + expect(screen.getByText('Local')).toBeInTheDocument(); + }); + + it('renders every returned row and badges only the official one, with no Show all toggle', async () => { + mockInstalledList.mockResolvedValue([]); + mockStatus.mockResolvedValue([]); + // The core keeps the full deduped catalog and marks the canonical + // first-party server with `official`. The tab renders every returned row + // (no client-side filtering) and badges only the official one. + mockRegistrySearch.mockResolvedValue({ + servers: [ + { + qualified_name: 'com.notion/mcp', + display_name: 'Notion', + is_deployed: true, + official: true, + }, + { + qualified_name: 'ai.smithery/smithery-notion', + display_name: 'Community Notion', + is_deployed: true, + official: false, + }, + ], + page: 1, + total_pages: 1, + }); + + await renderAndWaitForLoad(); + vi.useRealTimers(); + + await waitFor(() => screen.getByText('Notion')); + // The community row still renders — nothing is hidden. + expect(screen.getByText('Community Notion')).toBeInTheDocument(); + // Exactly one row carries the ✓ Official badge. + expect(screen.getByText(/Official/)).toBeInTheDocument(); + // No verified/all toggle exists. + expect(screen.queryByText('Show all')).not.toBeInTheDocument(); + expect(screen.queryByText('Verified only')).not.toBeInTheDocument(); + }); + it('navigates to install view when a registry server row is clicked', async () => { mockInstalledList.mockResolvedValue([]); mockStatus.mockResolvedValue([]); mockRegistrySearch.mockResolvedValue({ - servers: [{ qualified_name: 'acme/new-srv', display_name: 'New Server' }], + servers: [{ qualified_name: 'acme/new-srv', display_name: 'New Server', official: true }], page: 1, total_pages: 1, }); @@ -275,7 +451,7 @@ describe('McpServersTab', () => { mockInstalledList.mockResolvedValue([]); mockStatus.mockResolvedValue([]); mockRegistrySearch.mockResolvedValue({ - servers: [{ qualified_name: 'acme/new-srv', display_name: 'New Server' }], + servers: [{ qualified_name: 'acme/new-srv', display_name: 'New Server', official: true }], page: 1, total_pages: 1, }); @@ -306,7 +482,7 @@ describe('McpServersTab', () => { mockInstalledList.mockResolvedValue([]); mockStatus.mockResolvedValue([]); mockRegistrySearch.mockResolvedValue({ - servers: [{ qualified_name: 'acme/new-srv', display_name: 'New Server' }], + servers: [{ qualified_name: 'acme/new-srv', display_name: 'New Server', official: true }], page: 1, total_pages: 1, }); @@ -388,7 +564,7 @@ describe('McpServersTab', () => { mockInstalledList.mockRejectedValueOnce(new Error('Transient error')); mockStatus.mockResolvedValue([]); mockRegistrySearch.mockResolvedValue({ - servers: [{ qualified_name: 'acme/new-srv', display_name: 'New Server' }], + servers: [{ qualified_name: 'acme/new-srv', display_name: 'New Server', official: true }], page: 1, total_pages: 1, }); diff --git a/app/src/components/channels/mcp/McpServersTab.tsx b/app/src/components/channels/mcp/McpServersTab.tsx index bea68fabdd..66be10b150 100644 --- a/app/src/components/channels/mcp/McpServersTab.tsx +++ b/app/src/components/channels/mcp/McpServersTab.tsx @@ -30,6 +30,40 @@ type View = type FilterChip = 'all' | 'installed' | 'registry'; +/** + * Collapse catalog entries to a single row per `qualified_name`. The registry + * can return the same server within a page or across paginated "load more" + * fetches; without this the unified table renders duplicate rows and React + * collides on the `catalog-` key. First occurrence wins so the + * earliest (highest-ranked) result is the one kept. + */ +const dedupeByQualifiedName = (servers: SmitheryServer[]): SmitheryServer[] => { + const seen = new Set(); + 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; +}; + +/** + * Collapse installed servers to one row per `qualified_name`. Install is + * idempotent in the core now, but pre-existing double-installs can linger on + * disk; the first occurrence (earliest install) is kept. + */ +const dedupeInstalledByQualifiedName = (servers: InstalledServer[]): InstalledServer[] => { + const seen = new Set(); + 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 STATUS_DOT: Record = { connected: 'bg-sage-500', connecting: 'bg-amber-400', @@ -39,6 +73,56 @@ const STATUS_DOT: Record = { disabled: 'bg-surface-strong', }; +// Maps an upstream registry id to its i18n label key. The official +// modelcontextprotocol.io registry is highlighted (sage) over Smithery so the +// authoritative source reads as the "top" one. +const SOURCE_LABEL_KEY: Record = { + mcp_official: 'mcp.tab.source.official', + smithery: 'mcp.tab.source.smithery', +}; + +/** Pill attributing a catalog row to the registry it came from. */ +const SourceBadge = ({ source }: { source?: string }) => { + const { t } = useT(); + const labelKey = source ? SOURCE_LABEL_KEY[source] : undefined; + if (!labelKey) { + return —; + } + const isOfficial = source === 'mcp_official'; + return ( + + {t(labelKey)} + + ); +}; + +/** + * Tiny hint on how a catalog row is reached: a hosted server (has an HTTP + * endpoint) can offer browser sign-in; a local server is run on-device and + * configured with a pasted token. This mirrors the `is_deployed` sort that + * floats hosted servers to the top. + */ +const TransportHintBadge = ({ deployed }: { deployed?: boolean }) => { + const { t } = useT(); + const hosted = !!deployed; + return ( + + {t(hosted ? 'mcp.tab.transport.hosted' : 'mcp.tab.transport.local')} + + ); +}; + const McpServersTab = () => { const { t } = useT(); const [servers, setServers] = useState([]); @@ -94,7 +178,7 @@ const McpServersTab = () => { }); if (seq !== requestSeqRef.current) return; const incoming = result.servers ?? []; - setCatalogServers(prev => (append ? [...prev, ...incoming] : incoming)); + setCatalogServers(prev => dedupeByQualifiedName(append ? [...prev, ...incoming] : incoming)); setCatalogPage(result.page); setCatalogTotalPages(result.total_pages); } catch (err) { @@ -198,8 +282,14 @@ const McpServersTab = () => { const selectedConnStatus = view.mode === 'detail' ? statuses.find(s => s.server_id === view.serverId) : undefined; + // One installed row per service. Install is idempotent server-side now, but + // legacy double-installs can still exist on disk; collapse them by + // qualified_name (servers arrive earliest-first) so the list stays "one per + // thing". Raw `servers` is kept for server_id-keyed detail/status lookups. + const installedView = dedupeInstalledByQualifiedName(servers); + // Filter installed servers by search - const filteredInstalled = servers.filter(s => { + const filteredInstalled = installedView.filter(s => { if (!searchQuery.trim()) return true; const q = searchQuery.toLowerCase(); return ( @@ -209,9 +299,83 @@ const McpServersTab = () => { ); }); - // Filter out catalog servers already installed + // Catalog rows (minus already-installed), in the registry's relevance order. + // We deliberately do NOT split by auth method here: `is_deployed` only tells + // us a server has a hosted endpoint, not whether it signs in via the browser + // or wants a pasted token — that's only known once the install dialog fetches + // the detail (`required_env_keys`) or the connect attempt hits an OAuth + // challenge. Each row carries a Hosted/Local hint badge instead; the real + // auth requirement surfaces in the install flow. const installedNames = new Set(servers.map(s => s.qualified_name)); - const filteredCatalog = catalogServers.filter(s => !installedNames.has(s.qualified_name)); + const availableCatalog = catalogServers.filter(s => !installedNames.has(s.qualified_name)); + const showRegistry = activeChip === 'all' || activeChip === 'registry'; + + const renderCatalogRow = (server: SmitheryServer) => ( + handleSelectInstall(server.qualified_name)} + onKeyDown={e => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + handleSelectInstall(server.qualified_name); + } + }}> + +
+ {server.icon_url ? ( + + ) : ( + + 🔌 + + )} +
+ + + {server.display_name} + + {server.official && ( + + ✓ {t('mcp.tab.officialBadge')} + + )} + + + {/* The registry is full of look-alike names (a dozen "gmail" + servers); the slug is the unique identifier that tells them + apart. */} + + {server.qualified_name} + + {server.description && ( + + {server.description} + + )} +
+
+ + + + + + + {deriveAuthor(server.qualified_name) ?? '—'} + + + + + {t('mcp.install.button')} + + + + ); const statusMap = new Map(statuses.map(s => [s.server_id, s])); @@ -328,14 +492,21 @@ const McpServersTab = () => {
)} - {/* Table */} -
- + {/* Table — horizontally scrollable so the Source/Author/Action columns + aren't clipped when the panel is narrower than the table's natural + width (the wrapper was `overflow-hidden`, which cut them off with no + way to scroll). `min-w` keeps the columns readable rather than + crushing them. */} +
+
+ @@ -385,6 +556,9 @@ const McpServersTab = () => { + handleSelectInstall(server.qualified_name)} - onKeyDown={e => { - if (e.key === 'Enter' || e.key === ' ') { - e.preventDefault(); - handleSelectInstall(server.qualified_name); - } - }}> - - - - - ))} + {/* Registry servers — one relevance-ordered list. Each row shows a + Hosted/Local hint; the real auth method appears on install. */} + {showRegistry && availableCatalog.map(renderCatalogRow)}
{t('mcp.tab.column.name')} + {t('mcp.tab.column.source')} + {t('mcp.tab.column.author')} + — + {deriveAuthor(server.qualified_name) ?? '—'} @@ -399,62 +573,9 @@ const McpServersTab = () => { ); })} - {/* Registry servers */} - {(activeChip === 'all' || activeChip === 'registry') && - filteredCatalog.map(server => ( -
-
- {server.icon_url ? ( - - ) : ( - - 🔌 - - )} -
- - {server.display_name} - - {server.description && ( - - {server.description} - - )} -
-
- 		- - {deriveAuthor(server.qualified_name) ?? '—'} - - - - {t('mcp.install.button')} - -
@@ -466,7 +587,7 @@ const McpServersTab = () => { {t('mcp.installed.empty')}
)} - {activeChip === 'registry' && filteredCatalog.length === 0 && !catalogLoading && ( + {activeChip === 'registry' && availableCatalog.length === 0 && !catalogLoading && (
@@ -477,7 +598,7 @@ const McpServersTab = () => { )} {activeChip === 'all' && filteredInstalled.length === 0 && - filteredCatalog.length === 0 && + availableCatalog.length === 0 && !catalogLoading && (
SmitheryServerSummary { + SmitheryServerSummary { + qualified_name: qualified_name.to_string(), + display_name: qualified_name.to_string(), + description: None, + icon_url: None, + use_count: 0, + is_deployed: true, + source: "mcp_official".to_string(), + official: false, + extra: Default::default(), + } + } + + #[test] + fn tags_only_exact_canonical_servers() { + let mut servers = vec![ + server("io.github.github/github-mcp-server"), // official + server("ai.smithery/Hint-Services-obsidian-github-mcp"), // 'github' in name, NOT official + server("com.notion/mcp"), // official + server("ai.smithery/smithery-notion"), // community + server("io.github.CSOAI-ORG/meok-stripe-acp-checkout-mcp"), // 'stripe' in name, NOT official + ]; + + tag_official(&mut servers); + + assert!(servers[0].official); + assert!( + !servers[1].official, + "a name merely containing 'github' must not be marked official" + ); + assert!(servers[2].official); + assert!(!servers[3].official); + assert!( + !servers[4].official, + "a name merely containing 'stripe' must not be marked official" + ); + } +} diff --git a/src/openhuman/mcp_registry/mod.rs b/src/openhuman/mcp_registry/mod.rs index f207c53f36..5a481c276b 100644 --- a/src/openhuman/mcp_registry/mod.rs +++ b/src/openhuman/mcp_registry/mod.rs @@ -53,6 +53,7 @@ pub mod boot; pub mod bus; pub mod connections; +mod curation; pub mod oauth; pub mod ops; mod registries; diff --git a/src/openhuman/mcp_registry/ops.rs b/src/openhuman/mcp_registry/ops.rs index 42b3c9f654..f05dce43b1 100644 --- a/src/openhuman/mcp_registry/ops.rs +++ b/src/openhuman/mcp_registry/ops.rs @@ -70,8 +70,9 @@ pub async fn mcp_clients_registry_get( .map_err(|e| e.to_string())?; // Augment the response with required_env_keys derived from the connection - // config_schema so the frontend install dialog can build its input form. - let required_env_keys = collect_required_env_keys(&detail); + // the install will actually use (shared with the setup-agent path), so the + // frontend install dialog prompts only for what the picked transport needs. + let required_env_keys = super::setup_ops::collect_required_env_keys(&detail); let mut server_value = serde_json::to_value(&detail).map_err(|e| format!("serialization error: {e}"))?; if let Some(obj) = server_value.as_object_mut() { @@ -107,6 +108,58 @@ pub async fn mcp_clients_installed_list(config: &Config) -> Result, + config_value: &Option, + canonical_name: &str, +) -> Result, String> { + let mut refreshed = false; + if !env.is_empty() { + let mut merged = store::load_env_values(config, &existing.server_id) + .map_err(|e| format!("Failed to load existing env values: {e}"))?; + merged.extend(env.clone()); + store::set_env_values(config, &existing.server_id, &merged).map_err(|e| e.to_string())?; + let mut keys: Vec = merged.keys().cloned().collect(); + keys.sort(); + if existing.env_keys != keys { + store::update_server_env_keys(config, &existing.server_id, &keys) + .map_err(|e| e.to_string())?; + existing.env_keys = keys; + } + refreshed = true; + } + if let Some(cfg) = config_value.clone() { + store::update_server_config(config, &existing.server_id, Some(&cfg)) + .map_err(|e| e.to_string())?; + existing.config = Some(cfg); + refreshed = true; + } + tracing::debug!( + "[mcp-client] install no-op{} for {} (server_id={})", + if refreshed { + " (refreshed env/config)" + } else { + "" + }, + canonical_name, + existing.server_id + ); + Ok(RpcOutcome::new( + json!({ "server": existing, "already_installed": true }), + vec![format!("already installed qualified_name={canonical_name}")], + )) +} + pub async fn mcp_clients_install( config: &Config, qualified_name: String, @@ -123,8 +176,35 @@ pub async fn mcp_clients_install( env.keys().collect::>() ); - // Fetch registry detail to resolve command/args/env_keys - let detail = registry::registry_get(config, qualified_name.trim()) + // A source-routed install (`::`, e.g. + // `smithery::@org/server`) carries a registry prefix purely so + // `registry_get` can route to the right adapter. The catalog stores and + // dedups on the bare qualified_name, so the prefix must be stripped before + // the idempotency check and when persisting — otherwise a server installed + // once via the catalog (bare name) and again via a source-routed name would + // write a second row for the same service. + let routing_name = qualified_name.trim(); + let canonical_name = routing_name + .split_once("::") + .map(|(_, rest)| rest) + .unwrap_or(routing_name); + + // Idempotent install: one server per (bare) qualified_name. If this service + // is already installed, refresh any supplied env/config onto the existing + // row instead of writing a second one (the table PK is server_id, so nothing + // else prevents duplicates). The refresh matters because the install dialog + // awaits connect() right after install — a user re-running it to replace an + // expired token must not silently reconnect with the stale secret. + if let Some(existing) = + store::find_server_by_qualified_name(config, canonical_name).map_err(|e| e.to_string())? + { + return refresh_existing_install(config, existing, &env, &config_value, canonical_name); + } + + // Fetch registry detail to resolve command/args/env_keys. Use the full + // routing name (with any `::` prefix) so registry_get reaches the + // correct adapter even when that registry is search-gated. + let detail = registry::registry_get(config, routing_name) .await .map_err(|e| format!("Failed to fetch registry detail: {e}"))?; @@ -137,11 +217,11 @@ pub async fn mcp_clients_install( let picked = super::setup_ops::pick_connection(&detail.connections).ok_or_else(|| { format!( "server `{}` exposes neither stdio nor http_remote connections; nothing to install", - qualified_name.trim() + canonical_name ) })?; let (transport, command_kind, command, args) = - super::setup_ops::build_install_transport(qualified_name.trim(), picked)?; + super::setup_ops::build_install_transport(canonical_name, picked)?; // Derive required env keys from provided map + schema let env_keys: Vec = env.keys().cloned().collect(); @@ -154,7 +234,7 @@ pub async fn mcp_clients_install( let server = InstalledServer { server_id: server_id.clone(), - qualified_name: qualified_name.trim().to_string(), + qualified_name: canonical_name.to_string(), display_name: detail.display_name.clone(), description: detail.description.clone(), icon_url: detail.icon_url.clone(), @@ -169,7 +249,18 @@ pub async fn mcp_clients_install( enabled: true, }; - store::insert_server(config, &server).map_err(|e| e.to_string())?; + // Insert only if no row for this canonical name exists yet, atomically — the + // `find_server_by_qualified_name` above and this insert are separated by the + // awaited `registry_get`, so two concurrent installs of the same service + // could otherwise both miss and write duplicate rows (the table PK is + // `server_id`, which doesn't prevent that). If we lost that race, refresh + // onto the row the winner created instead of leaving a duplicate. + if !store::insert_server_if_absent(config, &server).map_err(|e| e.to_string())? { + let existing = store::find_server_by_qualified_name(config, canonical_name) + .map_err(|e| e.to_string())? + .ok_or_else(|| "install raced but the existing row could not be found".to_string())?; + return refresh_existing_install(config, existing, &env, &server.config, canonical_name); + } store::set_env_values(config, &server_id, &env).map_err(|e| e.to_string())?; tracing::debug!( @@ -720,9 +811,9 @@ pub async fn mcp_clients_config_assist( .await .map_err(|e| format!("Failed to fetch registry detail: {e}"))?; - // Collect required env keys from connections (if already known) or from any - // registered schema in the connection detail. - let required_env_keys: Vec = collect_required_env_keys(&detail); + // Collect required env keys from the connection the install will use (shared + // with the setup-agent + install-dialog paths). + let required_env_keys: Vec = super::setup_ops::collect_required_env_keys(&detail); let system_prompt = build_config_assist_system_prompt( &detail.display_name, @@ -777,26 +868,6 @@ fn build_config_assist_system_prompt( ) } -fn collect_required_env_keys(detail: &super::types::SmitheryServerDetail) -> Vec { - let mut keys = Vec::new(); - for conn in &detail.connections { - // Collect required inputs from every connection type. stdio inputs are - // process env vars; http / http_remote inputs are request headers - // (e.g. `Authorization`) declared via the registry's `remotes[].headers` - // — both are user-supplied secrets the install form must render. - if let Some(schema) = &conn.config_schema { - if let Some(props) = schema.get("properties").and_then(Value::as_object) { - for key in props.keys() { - if !keys.contains(key) { - keys.push(key.clone()); - } - } - } - } - } - keys -} - /// Invoke a lightweight inference call for config_assist. /// Uses the existing `inference` domain to run a structured-output chat turn. async fn invoke_config_assist_agent( @@ -923,7 +994,7 @@ mod tests { source: "smithery".to_string(), extra: Default::default(), }; - let keys = collect_required_env_keys(&detail); + let keys = crate::openhuman::mcp_registry::setup_ops::collect_required_env_keys(&detail); assert!(keys.contains(&"API_KEY".to_string())); assert!(keys.contains(&"ENDPOINT".to_string())); } diff --git a/src/openhuman/mcp_registry/registries/mcp_official.rs b/src/openhuman/mcp_registry/registries/mcp_official.rs index 787cebdb58..64d576e73c 100644 --- a/src/openhuman/mcp_registry/registries/mcp_official.rs +++ b/src/openhuman/mcp_registry/registries/mcp_official.rs @@ -468,6 +468,12 @@ impl OfficialListResponse { let mut seen = std::collections::HashSet::new(); self.servers .into_iter() + .filter(|env| { + // Drop servers that can't actually be installed (no hosted + // remote and no package) and ones the registry marks + // deprecated — both are noise in the catalog. + env.is_installable() && !env.is_deprecated() + }) .filter_map(|env| { if seen.insert(env.server.name.clone()) { Some(env.server.into_summary()) @@ -515,10 +521,30 @@ struct OfficialMetadata { struct OfficialServerEnvelope { server: OfficialServer, #[serde(default, rename = "_meta")] - #[allow(dead_code)] meta: Option, } +impl OfficialServerEnvelope { + /// A server is installable when it offers at least one way to connect: a + /// hosted remote or an installable package. A handful of registry entries + /// declare neither and can never be installed — they're catalog noise. + fn is_installable(&self) -> bool { + !self.server.remotes.is_empty() || !self.server.packages.is_empty() + } + + /// `true` when the registry marks this version deprecated. The status lives + /// at `_meta["io.modelcontextprotocol.registry/official"].status`; absent + /// meta (e.g. legacy cache) is treated as not-deprecated. + fn is_deprecated(&self) -> bool { + self.meta + .as_ref() + .and_then(|m| m.get("io.modelcontextprotocol.registry/official")) + .and_then(|o| o.get("status")) + .and_then(Value::as_str) + == Some("deprecated") + } +} + #[derive(Debug, Clone, Default, Deserialize)] struct OfficialServer { /// Reverse-DNS-style identifier, e.g. `io.github.foo/server-bar`. @@ -566,6 +592,7 @@ impl OfficialServer { use_count: 0, is_deployed: !self.remotes.is_empty(), source: SOURCE_MCP_OFFICIAL.to_string(), + official: false, // tagged later by the registry dispatcher extra: std::collections::HashMap::new(), } } @@ -1205,6 +1232,7 @@ mod tests { "server": { "name": "io.github.someuser/untitled-tool", "description": "A server without a title field", + "packages": [{ "registryType": "npm", "identifier": "untitled-tool" }], }, "_meta": {} } @@ -1243,10 +1271,11 @@ mod tests { /// `into_summaries` — only the first occurrence survives. #[test] fn list_response_deduplicates_by_name() { + let pkg = json!([{ "registryType": "npm", "identifier": "dup" }]); let raw = json!({ "servers": [ - { "server": { "name": "io.github.x/dup", "title": "First" } }, - { "server": { "name": "io.github.x/dup", "title": "Second" } }, + { "server": { "name": "io.github.x/dup", "title": "First", "packages": pkg } }, + { "server": { "name": "io.github.x/dup", "title": "Second", "packages": pkg } }, ] }); let parsed: OfficialListResponse = serde_json::from_value(raw).unwrap(); @@ -1255,6 +1284,36 @@ mod tests { assert_eq!(summaries[0].display_name, "First"); } + /// `into_summaries` drops servers that can't be installed (no remote, no + /// package) and ones the registry marks deprecated — catalog noise. + #[test] + fn list_response_filters_unusable_and_deprecated() { + let raw = json!({ + "servers": [ + { + "server": { "name": "ok/installable", + "packages": [{ "registryType": "npm", "identifier": "x" }] }, + "_meta": { "io.modelcontextprotocol.registry/official": { "status": "active" } } + }, + // No remote and no package → unusable, dropped. + { "server": { "name": "bad/unusable", "title": "Nope" }, "_meta": {} }, + // Installable but deprecated → dropped. + { + "server": { "name": "old/deprecated", + "remotes": [{ "url": "https://x.example/mcp" }] }, + "_meta": { "io.modelcontextprotocol.registry/official": { "status": "deprecated" } } + } + ] + }); + let parsed: OfficialListResponse = serde_json::from_value(raw).unwrap(); + let summaries = parsed.into_summaries(); + let slugs: Vec<_> = summaries + .iter() + .map(|s| s.qualified_name.as_str()) + .collect(); + assert_eq!(slugs, vec!["ok/installable"]); + } + #[test] fn into_detail_populates_example_config_for_packages() { let server: OfficialServer = serde_json::from_value(json!({ diff --git a/src/openhuman/mcp_registry/registries/mod.rs b/src/openhuman/mcp_registry/registries/mod.rs index 4e48c0e251..3d4ca91ba7 100644 --- a/src/openhuman/mcp_registry/registries/mod.rs +++ b/src/openhuman/mcp_registry/registries/mod.rs @@ -56,20 +56,58 @@ pub trait Registry: Send + Sync { async fn get(&self, config: &Config, qualified_name: &str) -> Result; } -/// All registries currently enabled for the user. -/// Official modelcontextprotocol.io is primary; Smithery is a fallback for -/// servers not yet listed on the official registry. -pub fn enabled_registries() -> Vec> { +/// Every registry adapter, unconditionally. Used to resolve a `source` id back +/// to its adapter even when that registry isn't part of the default search set +/// (e.g. `registry_get` for an already-installed Smithery server). +fn all_registries() -> Vec> { vec![ Box::new(mcp_official::McpOfficialRegistry), Box::new(smithery::SmitheryRegistry), ] } -/// Resolve a registry by [`Registry::source`] id. Used by `registry_get` to -/// route a fetch back to the upstream that produced the qualified name. +/// Registries that participate in catalog **search** for this user. The +/// official modelcontextprotocol.io registry is always on; Smithery is included +/// only when a Smithery API key is configured. +/// +/// Why gate Smithery: its servers don't run standalone — they're reached +/// through Smithery's gateway (`server.smithery.ai//mcp?api_key=…&profile=…`) +/// using the user's Smithery account, with per-server credentials configured on +/// smithery.ai. Without a key they can't connect in-app, so listing thousands +/// of them only yields un-installable rows with a misleading "sign in" banner. +/// Surface them only once the user has opted in by setting a key. +pub fn enabled_registries(config: &Config) -> Vec> { + let mut registries: Vec> = vec![Box::new(mcp_official::McpOfficialRegistry)]; + if smithery::smithery_api_key(config).is_some() { + registries.push(Box::new(smithery::SmitheryRegistry)); + } + registries +} + +/// Resolve a registry by [`Registry::source`] id. Searches *all* adapters (not +/// just the search-enabled ones) so a source-routed `registry_get` for an +/// already-installed server resolves regardless of the Smithery-key gate. pub fn registry_for_source(source: &str) -> Option> { - enabled_registries() - .into_iter() - .find(|r| r.source() == source) + all_registries().into_iter().find(|r| r.source() == source) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn registry_for_source_resolves_every_adapter_even_when_search_gated() { + // `registry_for_source` must resolve Smithery regardless of the + // search-time key gate, so detail lookups for an already-installed + // Smithery server keep working when no key is set. + assert_eq!( + registry_for_source(SOURCE_MCP_OFFICIAL).map(|r| r.source()), + Some(SOURCE_MCP_OFFICIAL) + ); + assert_eq!( + registry_for_source(SOURCE_SMITHERY).map(|r| r.source()), + Some(SOURCE_SMITHERY) + ); + assert!(registry_for_source("nope").is_none()); + } } diff --git a/src/openhuman/mcp_registry/registry.rs b/src/openhuman/mcp_registry/registry.rs index 2c9feb2eaf..b344dc4ade 100644 --- a/src/openhuman/mcp_registry/registry.rs +++ b/src/openhuman/mcp_registry/registry.rs @@ -11,6 +11,8 @@ //! `"::"` (e.g. `"mcp_official::io.github.foo/bar"`). //! Without a prefix we ask every registry and return the first hit. +use std::collections::HashSet; + use anyhow::Result; use futures::future::join_all; @@ -30,23 +32,61 @@ pub async fn registry_search( page: u32, page_size: u32, ) -> Result<(Vec, u32)> { - let registries = enabled_registries(); + let registries = enabled_registries(config); let queries = registries .iter() .map(|r| r.search(config, query, page, page_size)); let results = join_all(queries).await; + let labelled = results + .into_iter() + .enumerate() + .map(|(idx, res)| (registries[idx].source(), res)) + .collect(); + + let (mut merged, mut total_pages) = merge_registry_results(labelled); + + // Keep the full deduped catalog browsable — no one-per-service collapse + // (it hides genuinely different community servers and barely trims noise). + // Just badge the canonical first-party server for each known service so the + // official one is easy to spot without throwing any alternatives away. + super::curation::tag_official(&mut merged); + + if total_pages == 0 { + total_pages = page.max(1); + } + Ok((merged, total_pages)) +} + +/// Merge per-registry search results into one list, dropping exact +/// `qualified_name` duplicates. Registries are passed in priority order +/// (official before Smithery), and the first occurrence of a slug wins — so a +/// package listed on both registries collapses to the higher-priority copy and +/// the UI never shows the same slug twice. `total_pages` is the max reported +/// across the registries that succeeded. Failed registries are logged and +/// skipped so one flaky upstream can't blank the catalog. +fn merge_registry_results( + results: Vec<(&'static str, Result<(Vec, u32)>)>, +) -> (Vec, u32) { let mut merged: Vec = Vec::new(); + let mut seen: HashSet = HashSet::new(); let mut total_pages: u32 = 0; - for (idx, res) in results.into_iter().enumerate() { - let source = registries[idx].source(); + let mut dropped: usize = 0; + + for (source, res) in results { match res { - Ok((mut servers, pages)) => { + Ok((servers, pages)) => { tracing::debug!( "[mcp-registry] {source} search ok servers={} pages={pages}", servers.len() ); - merged.append(&mut servers); + for server in servers { + if seen.insert(server.qualified_name.clone()) { + merged.push(server); + } else { + dropped += 1; + } + } total_pages = total_pages.max(pages); } Err(err) => { @@ -55,10 +95,10 @@ pub async fn registry_search( } } - if total_pages == 0 { - total_pages = page.max(1); + if dropped > 0 { + tracing::debug!("[mcp-registry] dropped {dropped} cross-registry duplicate slug(s)"); } - Ok((merged, total_pages)) + (merged, total_pages) } /// Fetch a server detail. If `qualified_name` starts with `"::"` we @@ -76,7 +116,7 @@ pub async fn registry_get(config: &Config, qualified_name: &str) -> Result = None; - for registry in enabled_registries() { + for registry in enabled_registries(config) { match registry.get(config, qualified_name).await { Ok(detail) => return Ok(detail), Err(err) => { @@ -90,3 +130,76 @@ pub async fn registry_get(config: &Config, qualified_name: &str) -> Result SmitheryServerSummary { + SmitheryServerSummary { + qualified_name: qualified_name.to_string(), + display_name: qualified_name.to_string(), + description: None, + icon_url: None, + use_count: 0, + is_deployed: false, + source: source.to_string(), + official: false, + extra: Default::default(), + } + } + + #[test] + fn merge_keeps_higher_priority_duplicate_and_drops_the_rest() { + // `dup/server` is listed on both registries; official is passed first. + let results = vec![ + ( + "mcp_official", + Ok(( + vec![ + summary("dup/server", "mcp_official"), + summary("off/only", "mcp_official"), + ], + 3, + )), + ), + ( + "smithery", + Ok(( + vec![ + summary("dup/server", "smithery"), + summary("smi/only", "smithery"), + ], + 5, + )), + ), + ]; + + let (merged, total_pages) = merge_registry_results(results); + + let slugs: Vec<_> = merged.iter().map(|s| s.qualified_name.as_str()).collect(); + assert_eq!(slugs, vec!["dup/server", "off/only", "smi/only"]); + // The surviving duplicate is the official copy (first occurrence wins). + let dup = merged + .iter() + .find(|s| s.qualified_name == "dup/server") + .unwrap(); + assert_eq!(dup.source, "mcp_official"); + // total_pages is the max across registries. + assert_eq!(total_pages, 5); + } + + #[test] + fn merge_skips_failed_registries_without_blanking_results() { + let results = vec![ + ("mcp_official", Err(anyhow::anyhow!("upstream 500"))), + ("smithery", Ok((vec![summary("smi/only", "smithery")], 2))), + ]; + + let (merged, total_pages) = merge_registry_results(results); + + assert_eq!(merged.len(), 1); + assert_eq!(merged[0].qualified_name, "smi/only"); + assert_eq!(total_pages, 2); + } +} diff --git a/src/openhuman/mcp_registry/setup_ops.rs b/src/openhuman/mcp_registry/setup_ops.rs index 1bf11aad2f..a3a9497f86 100644 --- a/src/openhuman/mcp_registry/setup_ops.rs +++ b/src/openhuman/mcp_registry/setup_ops.rs @@ -357,18 +357,26 @@ fn parse_ref_map(raw: HashMap) -> Result Vec { +pub(crate) fn collect_required_env_keys( + detail: &super::types::SmitheryServerDetail, +) -> Vec { + // Derive required inputs from the connection the install will actually use, + // not from every connection. Hosted HTTP-remote is now preferred over a + // local stdio package (see `pick_connection`), so a server that offers both + // must not demand the stdio package's env vars for an install that connects + // over HTTP and never consumes them. `config_schema.properties` carries the + // stdio env vars or the HTTP remote's headers depending on the picked + // transport, so reading the picked connection covers both. + let Some(conn) = pick_connection(&detail.connections) else { + return Vec::new(); + }; let mut keys = Vec::new(); - for conn in &detail.connections { - if conn.r#type != "stdio" { - continue; - } - let Some(schema) = conn.config_schema.as_ref() else { - continue; - }; - let Some(props) = schema.get("properties").and_then(Value::as_object) else { - continue; - }; + if let Some(props) = conn + .config_schema + .as_ref() + .and_then(|schema| schema.get("properties")) + .and_then(Value::as_object) + { for k in props.keys() { if !keys.contains(k) { keys.push(k.clone()); @@ -385,42 +393,47 @@ const _: Option = None; /// Choose the best [`SmitheryConnection`] from a registry detail response. /// -/// Preference order: -/// 1. **Published `stdio`** — no behaviour regression for any server that -/// used to install before HTTP-remote support landed. -/// 2. **Any `stdio`** (even unpublished) — also pre-existing fallback. -/// 3. **Published `http_remote`** — the new path. Smithery serves ~99% of -/// their listings as HTTP-remote. -/// 4. **Any `http_remote`** — last-resort. +/// Preference order — **hosted remote first**: +/// 1. **Published `http_remote`** — the hosted endpoint. Connecting here avoids +/// spawning a local subprocess (no node/uvx runtime, no package resolution, +/// no broken-package failures) and routes auth to the server's OAuth/token +/// challenge, which is far more likely to succeed than a local spawn. +/// 2. **Any `http_remote`** (even unpublished). +/// 3. **Published `stdio`** — local subprocess fallback for servers with no +/// hosted endpoint. +/// 4. **Any `stdio`**. /// 5. `None` — nothing dialable. /// -/// Stdio is preferred because it's privacy-strict (everything runs locally) -/// and because most of the existing OpenHuman ecosystem assumes stdio. HTTP -/// is the fallback that finally lets a Smithery-only server install at all. +/// Rationale: most registry servers expose both a stdio package and a hosted +/// remote. Preferring stdio meant the local subprocess had to spawn and find +/// its runtime + credentials, which fails for the majority of community +/// servers. Preferring the hosted remote eliminates that failure class. The +/// trade-off is that data flows to the hosted endpoint and a network is +/// required — a deliberate choice favouring "it connects" over local-first. pub(super) fn pick_connection(connections: &[SmitheryConnection]) -> Option<&SmitheryConnection> { // Treat the canonical wire names ("stdio", "http") AND the persisted // dispatch kinds ("http_remote") as equivalent — registry payloads // historically use "http" while our `Transport` discriminator uses // "http_remote". `transport_kind` normalises that mapping. - let stdio_pub = connections - .iter() - .find(|c| c.transport_kind() == "stdio" && c.published); - if stdio_pub.is_some() { - return stdio_pub; - } - let stdio_any = connections.iter().find(|c| c.transport_kind() == "stdio"); - if stdio_any.is_some() { - return stdio_any; - } let http_pub = connections .iter() .find(|c| c.transport_kind() == "http_remote" && c.published); if http_pub.is_some() { return http_pub; } - connections + let http_any = connections + .iter() + .find(|c| c.transport_kind() == "http_remote"); + if http_any.is_some() { + return http_any; + } + let stdio_pub = connections .iter() - .find(|c| c.transport_kind() == "http_remote") + .find(|c| c.transport_kind() == "stdio" && c.published); + if stdio_pub.is_some() { + return stdio_pub; + } + connections.iter().find(|c| c.transport_kind() == "stdio") } /// Resolve the persisted [`Transport`] + launch command for an install from the @@ -488,24 +501,48 @@ mod tests { } } - /// Stdio wins when both transports are offered, even when stdio is - /// unpublished and http is published. This pins the "no regression - /// for existing stdio installs" promise. + /// Hosted remote wins when both transports are offered — connecting to the + /// endpoint avoids the local-spawn failure class that broke most servers. #[test] - fn pick_connection_prefers_stdio_over_http() { + fn pick_connection_prefers_http_over_stdio() { let conns = vec![ + conn("stdio", true, None), conn("http", true, Some("https://x.io/mcp")), - conn("stdio", false, None), ]; - let picked = pick_connection(&conns).expect("stdio should be picked"); - assert_eq!(picked.r#type, "stdio"); + let picked = pick_connection(&conns).expect("http_remote should be picked"); + assert_eq!(picked.transport_kind(), "http_remote"); + } + + /// Even an *unpublished* hosted remote beats a published stdio package — + /// the hosted endpoint is still preferred over spawning locally. + #[test] + fn pick_connection_prefers_unpublished_http_over_published_stdio() { + let conns = vec![ + conn("stdio", true, None), + conn("http", false, Some("https://x.io/mcp")), + ]; + let picked = pick_connection(&conns).expect("http_remote should be picked"); + assert_eq!(picked.transport_kind(), "http_remote"); + } + + /// Published http beats unpublished http. + #[test] + fn pick_connection_prefers_published_http_first() { + let conns = vec![ + conn("http", false, Some("https://any.io/mcp")), + conn("http", true, Some("https://pub.io/mcp")), + ]; + let picked = pick_connection(&conns).expect("published http should win"); + assert!(picked.published); + assert_eq!(picked.deployment_url.as_deref(), Some("https://pub.io/mcp")); } - /// Published stdio beats unpublished stdio. + /// With no hosted remote, stdio is the fallback — published stdio first. #[test] - fn pick_connection_prefers_published_stdio_first() { + fn pick_connection_falls_back_to_stdio_when_no_http() { let conns = vec![conn("stdio", false, None), conn("stdio", true, None)]; let picked = pick_connection(&conns).expect("published stdio should win"); + assert_eq!(picked.transport_kind(), "stdio"); assert!(picked.published); } @@ -583,4 +620,69 @@ mod tests { .expect_err("missing deployment_url must error"); assert!(err.contains("deployment_url"), "got: {err}"); } + + fn conn_schema(kind: &str, url: Option<&str>, props: &[&str]) -> SmitheryConnection { + let properties: serde_json::Map = props + .iter() + .map(|k| (k.to_string(), serde_json::json!({ "type": "string" }))) + .collect(); + SmitheryConnection { + r#type: kind.to_string(), + deployment_url: url.map(String::from), + config_schema: Some(serde_json::json!({ "properties": properties })), + example_config: None, + published: true, + extra: std::collections::HashMap::new(), + } + } + + fn detail_with(conns: Vec) -> super::super::types::SmitheryServerDetail { + super::super::types::SmitheryServerDetail { + qualified_name: "@t/s".to_string(), + display_name: "T".to_string(), + description: None, + icon_url: None, + connections: conns, + source: "smithery".to_string(), + extra: Default::default(), + } + } + + /// Mixed-transport server: install prefers the hosted http connection, so the + /// required keys must be the http connection's headers — NOT the stdio + /// package's env vars the install will never consume. + #[test] + fn required_env_keys_uses_picked_http_connection_headers() { + let detail = detail_with(vec![ + conn_schema("stdio", None, &["STDIO_KEY"]), + conn_schema("http", Some("https://x.io/mcp"), &["Authorization"]), + ]); + let keys = collect_required_env_keys(&detail); + assert_eq!(keys, vec!["Authorization".to_string()]); + assert!(!keys.contains(&"STDIO_KEY".to_string())); + } + + /// Stdio-only server still surfaces its declared env vars. + #[test] + fn required_env_keys_stdio_only_returns_its_keys() { + let detail = detail_with(vec![conn_schema("stdio", None, &["API_KEY", "ENDPOINT"])]); + let keys = collect_required_env_keys(&detail); + assert!(keys.contains(&"API_KEY".to_string())); + assert!(keys.contains(&"ENDPOINT".to_string())); + } + + /// A picked http connection with no declared schema needs no user input. + #[test] + fn required_env_keys_http_without_schema_is_empty() { + let detail = detail_with(vec![conn("http", true, Some("https://x.io/mcp"))]); + assert!(collect_required_env_keys(&detail).is_empty()); + } + + /// No dialable connection (only an unknown transport) → no keys, mirroring + /// the install path which would reject the server entirely. + #[test] + fn required_env_keys_empty_when_no_dialable_connection() { + let detail = detail_with(vec![conn("ws", true, Some("wss://x.io"))]); + assert!(collect_required_env_keys(&detail).is_empty()); + } } diff --git a/src/openhuman/mcp_registry/store.rs b/src/openhuman/mcp_registry/store.rs index 87cf4e8610..4a59c91ee5 100644 --- a/src/openhuman/mcp_registry/store.rs +++ b/src/openhuman/mcp_registry/store.rs @@ -171,6 +171,56 @@ pub fn insert_server_conn(conn: &Connection, server: &InstalledServer) -> Result Ok(()) } +/// Insert a server row only if no row with the same `qualified_name` already +/// exists, in a single atomic statement. The `mcp_clients_install` flow checks +/// `find_server_by_qualified_name` before inserting, but an awaited +/// `registry_get` sits between that read and the write, so two concurrent +/// installs of the same service could both miss and insert (the PK is +/// `server_id`, which doesn't prevent duplicate `qualified_name`s). `INSERT … +/// SELECT … WHERE NOT EXISTS` closes that window without a schema change. +/// Returns `true` if this call inserted the row, `false` if one already existed. +pub fn insert_server_if_absent(config: &Config, server: &InstalledServer) -> Result { + with_connection(config, |conn| insert_server_if_absent_conn(conn, server)) +} + +pub fn insert_server_if_absent_conn(conn: &Connection, server: &InstalledServer) -> Result { + 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() + .map(serde_json::to_string) + .transpose()?; + let n = conn + .execute( + "INSERT INTO mcp_servers + (server_id, qualified_name, display_name, description, icon_url, + command_kind, command, args_json, env_keys_json, config_json, + installed_at, last_connected_at, transport, deployment_url, enabled) + SELECT ?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9, ?10, ?11, ?12, ?13, ?14, ?15 + WHERE NOT EXISTS (SELECT 1 FROM mcp_servers WHERE qualified_name = ?2)", + params![ + server.server_id, + server.qualified_name, + server.display_name, + server.description, + server.icon_url, + server.command_kind.as_str(), + server.command, + args_json, + env_keys_json, + config_json, + server.installed_at, + server.last_connected_at, + server.transport.dispatch_kind(), + server.transport.deployment_url(), + server.enabled as i64, + ], + ) + .context("Failed to insert mcp_server (if absent)")?; + Ok(n > 0) +} + /// Update only the `env_keys` list for an installed server. Used by /// `mcp_clients_update_env` to keep the persisted key-name list in sync with a /// reconfigure — the env *values* live in the separate `mcp_client_env` table, @@ -188,6 +238,34 @@ pub fn update_server_env_keys(config: &Config, server_id: &str, env_keys: &[Stri }) } +/// Update only the `config_json` blob for an installed server. Used by the +/// idempotent re-install path so a second install carrying new config refreshes +/// the existing row instead of dropping it — a plain `insert_server` would +/// conflict on the primary key. `None` clears the stored config. +pub fn update_server_config( + config: &Config, + server_id: &str, + value: Option<&serde_json::Value>, +) -> Result<()> { + with_connection(config, |conn| { + update_server_config_conn(conn, server_id, value) + }) +} + +pub fn update_server_config_conn( + conn: &Connection, + server_id: &str, + value: Option<&serde_json::Value>, +) -> Result<()> { + let config_json = value.map(serde_json::to_string).transpose()?; + conn.execute( + "UPDATE mcp_servers SET config_json = ?2 WHERE server_id = ?1", + params![server_id, config_json], + ) + .context("Failed to update mcp_server config")?; + Ok(()) +} + pub fn list_servers(config: &Config) -> Result> { with_connection(config, |conn| list_servers_conn(conn)) } @@ -207,6 +285,37 @@ pub fn list_servers_conn(conn: &Connection) -> Result> { Ok(servers) } +/// First installed server with this qualified name, if any. The schema allows +/// multiple installs of the same `qualified_name` (the PK is `server_id`), so +/// this returns the earliest by `installed_at` — used to keep install +/// idempotent (one install per service). +pub fn find_server_by_qualified_name( + config: &Config, + qualified_name: &str, +) -> Result> { + with_connection(config, |conn| { + find_server_by_qualified_name_conn(conn, qualified_name) + }) +} + +pub fn find_server_by_qualified_name_conn( + conn: &Connection, + qualified_name: &str, +) -> Result> { + let mut stmt = conn.prepare( + "SELECT server_id, qualified_name, display_name, description, icon_url, + command_kind, command, args_json, env_keys_json, config_json, + installed_at, last_connected_at, transport, deployment_url, enabled + FROM mcp_servers WHERE qualified_name = ?1 + ORDER BY installed_at ASC LIMIT 1", + )?; + let mut rows = stmt.query(params![qualified_name])?; + match rows.next()? { + Some(row) => Ok(Some(map_server_row(row)?)), + None => Ok(None), + } +} + pub fn get_server(config: &Config, server_id: &str) -> Result { with_connection(config, |conn| get_server_conn(conn, server_id)) } @@ -475,6 +584,64 @@ mod tests { assert_eq!(servers[0].command_kind, CommandKind::Node); } + #[test] + fn update_server_config_round_trips_and_clears() { + let (_f, conn) = open_test_conn(); + insert_server_conn(&conn, &sample_server("srv-cfg")).unwrap(); + // sample_server starts with no config. + assert_eq!(get_server_conn(&conn, "srv-cfg").unwrap().config, None); + // Setting a config blob persists and reads back identically. + let cfg = serde_json::json!({ "mode": "fast", "n": 3 }); + update_server_config_conn(&conn, "srv-cfg", Some(&cfg)).unwrap(); + assert_eq!(get_server_conn(&conn, "srv-cfg").unwrap().config, Some(cfg)); + // None clears it back to NULL. + update_server_config_conn(&conn, "srv-cfg", None).unwrap(); + assert_eq!(get_server_conn(&conn, "srv-cfg").unwrap().config, None); + } + + #[test] + fn insert_server_if_absent_dedups_on_qualified_name() { + let (_f, conn) = open_test_conn(); + // First install of a service inserts the row. + let mut first = sample_server("srv-a"); + first.qualified_name = "@dup/server".to_string(); + assert!(insert_server_if_absent_conn(&conn, &first).unwrap()); + // A second install of the SAME qualified_name (different server_id) is a + // no-op — the count stays at one and the original row survives. + let mut second = sample_server("srv-b"); + second.qualified_name = "@dup/server".to_string(); + assert!(!insert_server_if_absent_conn(&conn, &second).unwrap()); + let rows: Vec<_> = list_servers_conn(&conn) + .unwrap() + .into_iter() + .filter(|s| s.qualified_name == "@dup/server") + .collect(); + assert_eq!(rows.len(), 1); + assert_eq!(rows[0].server_id, "srv-a"); + } + + #[test] + fn find_server_by_qualified_name_returns_earliest_install() { + let (_f, conn) = open_test_conn(); + // Two installs of the same service (different ids, different times). + let mut early = sample_server("srv-early"); + early.installed_at = 100; + let mut late = sample_server("srv-late"); + late.installed_at = 200; + // Insert the later one first to prove ordering is by installed_at. + insert_server_conn(&conn, &late).unwrap(); + insert_server_conn(&conn, &early).unwrap(); + + let found = find_server_by_qualified_name_conn(&conn, "@test/server") + .unwrap() + .expect("server present"); + assert_eq!(found.server_id, "srv-early"); + + assert!(find_server_by_qualified_name_conn(&conn, "@nope/missing") + .unwrap() + .is_none()); + } + #[test] fn get_server_not_found() { let (_f, conn) = open_test_conn(); diff --git a/src/openhuman/mcp_registry/types.rs b/src/openhuman/mcp_registry/types.rs index 1692df76e5..ad4803b29a 100644 --- a/src/openhuman/mcp_registry/types.rs +++ b/src/openhuman/mcp_registry/types.rs @@ -239,6 +239,12 @@ pub struct SmitheryServerSummary { /// originating upstream. #[serde(default)] pub source: String, + /// `true` when this is the canonical first-party server for a well-known + /// service (exact `qualified_name` match in `super::curation`). The UI + /// badges it "Official"; everything else is shown without a badge. Set by + /// the dispatcher; never trusted from the wire. + #[serde(default)] + pub official: bool, /// Raw extra fields preserved for future use. #[serde(flatten, default)] pub extra: std::collections::HashMap, @@ -500,6 +506,7 @@ mod tests { use_count: 10, is_deployed: true, source: "mcp_official".to_string(), + official: false, extra: Default::default(), }; let v = serde_json::to_value(&s).unwrap(); diff --git a/tests/json_rpc_e2e.rs b/tests/json_rpc_e2e.rs index 67949d71e7..4cd23bda56 100644 --- a/tests/json_rpc_e2e.rs +++ b/tests/json_rpc_e2e.rs @@ -10357,6 +10357,12 @@ async fn mcp_clients_lifecycle() { /// No npx, no network: we pre-seed `smithery:detail:` with a detail whose /// stdio `exampleConfig.command` points at the stub binary, so /// `mcp_clients_install` resolves the launch command to the stub. +/// +/// Smithery is now opt-in (only enabled when an API key is set), so we install +/// with the explicit `smithery::` source prefix. `registry_get` routes a +/// prefixed name straight to that 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. #[tokio::test] async fn mcp_clients_install_connect_tool_call_happy_path() { let _env_lock = json_rpc_e2e_env_lock(); @@ -10410,7 +10416,7 @@ async fn mcp_clients_install_connect_tool_call_happy_path() { &rpc_base, 9920, "openhuman.mcp_clients_install", - json!({ "qualified_name": qualified_name, "env": {} }), + json!({ "qualified_name": format!("smithery::{qualified_name}"), "env": {} }), ) .await; let install_result = assert_no_jsonrpc_error(&install, "mcp_clients_install (happy path)"); @@ -10564,6 +10570,9 @@ async fn mcp_clients_set_enabled_smoke() { write_min_config(&user_scoped_dir, &mock_origin); // Seed the registry detail cache so install resolves offline to the stub. + // Smithery is opt-in (gated on an API key), so install routes via the + // explicit `smithery::` source prefix below — `registry_for_source` resolves + // the adapter regardless of the key gate. let stub_path = env!("CARGO_BIN_EXE_test-mcp-stub"); let qualified_name = "@openhuman-test/echo-set-enabled"; let detail = serde_json::json!({ @@ -10595,7 +10604,7 @@ async fn mcp_clients_set_enabled_smoke() { &rpc_base, 9940, "openhuman.mcp_clients_install", - json!({ "qualified_name": qualified_name, "env": {} }), + json!({ "qualified_name": format!("smithery::{qualified_name}"), "env": {} }), ) .await; let install_result = @@ -10650,6 +10659,151 @@ async fn mcp_clients_set_enabled_smoke() { rpc_join.abort(); } +/// `mcp_clients_install` idempotency (issue #4120 review): a re-install of the +/// same service (a) collapses to ONE row and returns `already_installed:true` +/// even when the first install used a source-prefixed name and the second used +/// the bare name (canonical dedup), and (b) MERGES new env onto the existing row +/// so re-running the dialog to replace an expired token doesn't drop the user's +/// other stored keys. +#[tokio::test] +async fn mcp_clients_install_idempotent_refresh_and_canonical_dedup() { + let _env_lock = json_rpc_e2e_env_lock(); + let tmp = tempdir().expect("tempdir"); + let home = tmp.path(); + let openhuman_home = home.join(".openhuman"); + + let _home_guard = EnvVarGuard::set_to_path("HOME", home); + let _workspace_guard = EnvVarGuard::unset("OPENHUMAN_WORKSPACE"); + let _backend_url_guard = EnvVarGuard::unset("BACKEND_URL"); + let _vite_backend_guard = EnvVarGuard::unset("VITE_BACKEND_URL"); + + let (mock_addr, mock_join) = serve_on_ephemeral(mock_upstream_router()).await; + let mock_origin = format!("http://{}", mock_addr); + write_min_config(&openhuman_home, &mock_origin); + let user_scoped_dir = openhuman_home.join("users").join("local"); + write_min_config(&user_scoped_dir, &mock_origin); + + // Seed the smithery detail cache so the FIRST (prefixed) install resolves + // offline. The second install hits the idempotency branch before registry_get, + // so it needs no cache. + let stub_path = env!("CARGO_BIN_EXE_test-mcp-stub"); + let qualified_name = "@openhuman-test/echo-reinstall"; + let detail = serde_json::json!({ + "qualifiedName": qualified_name, + "displayName": "Test Echo Reinstall", + "description": "Stub for install idempotency.", + "connections": [{ + "type": "stdio", + "published": true, + "exampleConfig": { "command": stub_path, "args": [] } + }] + }); + let seed_config = openhuman_core::openhuman::config::load_config_with_timeout() + .await + .expect("load config for cache seed"); + openhuman_core::openhuman::mcp_registry::store::set_cached( + &seed_config, + &format!("smithery:detail:{qualified_name}"), + &detail.to_string(), + ) + .expect("seed smithery detail cache"); + + let (rpc_addr, rpc_join) = serve_on_ephemeral(build_core_http_router(false)).await; + let rpc_base = format!("http://{}", rpc_addr); + tokio::time::sleep(Duration::from_millis(100)).await; + + // ── 1. install via the source-prefixed name with two env keys ──────────── + let install1 = post_json_rpc( + &rpc_base, + 9960, + "openhuman.mcp_clients_install", + json!({ + "qualified_name": format!("smithery::{qualified_name}"), + "env": { "TOKEN": "old", "KEEP": "v1" } + }), + ) + .await; + let r1 = assert_no_jsonrpc_error(&install1, "mcp_clients_install (first)"); + let b1 = peel_logs_envelope(r1); + let server_id = b1 + .get("server") + .and_then(|s| s.get("server_id")) + .and_then(Value::as_str) + .expect("first install returns server_id") + .to_string(); + // Stored qualified_name is the bare (canonical) name, not the prefixed one. + assert_eq!( + b1.get("server") + .and_then(|s| s.get("qualified_name")) + .and_then(Value::as_str), + Some(qualified_name), + "install should store the canonical (bare) qualified_name: {b1}" + ); + + // ── 2. re-install via the BARE name with a rotated token ───────────────── + let install2 = post_json_rpc( + &rpc_base, + 9961, + "openhuman.mcp_clients_install", + json!({ "qualified_name": qualified_name, "env": { "TOKEN": "new" } }), + ) + .await; + let r2 = assert_no_jsonrpc_error(&install2, "mcp_clients_install (re-install)"); + let b2 = peel_logs_envelope(r2); + assert_eq!( + b2.get("already_installed"), + Some(&json!(true)), + "re-install should be flagged already_installed: {b2}" + ); + assert_eq!( + b2.get("server") + .and_then(|s| s.get("server_id")) + .and_then(Value::as_str), + Some(server_id.as_str()), + "re-install should return the same server_id: {b2}" + ); + // Env merged: the rotated key AND the untouched first-install key survive. + let env_keys: Vec = b2 + .get("server") + .and_then(|s| s.get("env_keys")) + .and_then(Value::as_array) + .expect("re-install returns env_keys") + .iter() + .filter_map(|v| v.as_str().map(String::from)) + .collect(); + assert!( + env_keys.contains(&"TOKEN".to_string()) && env_keys.contains(&"KEEP".to_string()), + "re-install should merge env (KEEP preserved, TOKEN present): {env_keys:?}" + ); + + // ── 3. exactly one installed row for this service (no duplicate) ───────── + let listed = post_json_rpc( + &rpc_base, + 9962, + "openhuman.mcp_clients_installed_list", + json!({}), + ) + .await; + let lb = peel_logs_envelope(assert_no_jsonrpc_error( + &listed, + "mcp_clients_installed_list", + )); + let count = lb + .get("installed") + .and_then(Value::as_array) + .expect("installed list") + .iter() + .filter(|s| s.get("qualified_name").and_then(Value::as_str) == Some(qualified_name)) + .count(); + assert_eq!( + count, 1, + "prefixed + bare install must collapse to one row: {lb}" + ); + + mock_join.abort(); + rpc_join.abort(); +} + /// Registry settings RPC: the getter reports `*_set` booleans without ever /// echoing secret values; the setter persists and clears them (issue #3039 /// gap A6).