diff --git a/app/src/components/channels/mcp/InstallDialog.test.tsx b/app/src/components/channels/mcp/InstallDialog.test.tsx index 2016c3743..27c429063 100644 --- a/app/src/components/channels/mcp/InstallDialog.test.tsx +++ b/app/src/components/channels/mcp/InstallDialog.test.tsx @@ -5,11 +5,13 @@ import InstallDialog from './InstallDialog'; const mockRegistryGet = vi.fn(); const mockInstall = vi.fn(); +const mockConnect = vi.fn(); vi.mock('../../../services/api/mcpClientsApi', () => ({ mcpClientsApi: { registryGet: (...args: unknown[]) => mockRegistryGet(...args), install: (...args: unknown[]) => mockInstall(...args), + connect: (...args: unknown[]) => mockConnect(...args), }, })); @@ -25,6 +27,8 @@ describe('InstallDialog', () => { beforeEach(() => { mockRegistryGet.mockReset(); mockInstall.mockReset(); + mockConnect.mockReset(); + mockConnect.mockResolvedValue({ server_id: 'srv-1', status: 'connected', tools: [] }); }); it('shows loading state while fetching detail', () => { @@ -107,6 +111,40 @@ describe('InstallDialog', () => { env: { API_KEY: 'my-api-key', SECRET_TOKEN: 'my-secret' }, config: undefined, }); + // Auto-connect on success (issue #3039 gap B3). + expect(mockConnect).toHaveBeenCalledWith('srv-1'); + expect(onSuccess).toHaveBeenCalledWith(installedServer); + }); + + it('still reports success when auto-connect fails (best-effort)', async () => { + const installedServer = { + server_id: 'srv-1', + ...DETAIL, + command_kind: 'node' as const, + command: 'node', + args: [], + env_keys: ['API_KEY', 'SECRET_TOKEN'], + installed_at: 1000, + }; + mockRegistryGet.mockResolvedValue(DETAIL); + mockInstall.mockResolvedValue(installedServer); + mockConnect.mockRejectedValue(new Error('spawn failed')); + + const onSuccess = vi.fn(); + render( + {}} /> + ); + + await waitFor(() => screen.getByLabelText('API_KEY')); + fireEvent.change(screen.getByLabelText('API_KEY'), { target: { value: 'k' } }); + fireEvent.change(screen.getByLabelText('SECRET_TOKEN'), { target: { value: 's' } }); + + await act(async () => { + fireEvent.click(screen.getByRole('button', { name: 'Install' })); + }); + + expect(mockConnect).toHaveBeenCalledWith('srv-1'); + // A connect failure must NOT block the install success callback. expect(onSuccess).toHaveBeenCalledWith(installedServer); }); diff --git a/app/src/components/channels/mcp/InstallDialog.tsx b/app/src/components/channels/mcp/InstallDialog.tsx index f0a54fd5a..eec962212 100644 --- a/app/src/components/channels/mcp/InstallDialog.tsx +++ b/app/src/components/channels/mcp/InstallDialog.tsx @@ -115,6 +115,20 @@ const InstallDialog = ({ qualifiedName, prefillEnv, onSuccess, onCancel }: Insta config: parsedConfig, }); log('install success server_id=%s', server.server_id); + // Best-effort auto-connect — fire-and-forget so the dialog closes + // immediately on install success. A slow or failing handshake must + // never block onSuccess (the detail view surfaces errors and offers + // a Connect retry). + 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) + ) + ); onSuccess(server); } catch (err) { const msg = err instanceof Error ? err.message : t('mcp.install.failedInstall'); diff --git a/app/src/components/channels/mcp/InstalledServerDetail.test.tsx b/app/src/components/channels/mcp/InstalledServerDetail.test.tsx index dd464c97f..b08be7dce 100644 --- a/app/src/components/channels/mcp/InstalledServerDetail.test.tsx +++ b/app/src/components/channels/mcp/InstalledServerDetail.test.tsx @@ -6,12 +6,14 @@ import InstalledServerDetail from './InstalledServerDetail'; const mockConnect = vi.fn(); const mockDisconnect = vi.fn(); const mockUninstall = vi.fn(); +const mockUpdateEnv = vi.fn(); vi.mock('../../../services/api/mcpClientsApi', () => ({ mcpClientsApi: { connect: (...args: unknown[]) => mockConnect(...args), disconnect: (...args: unknown[]) => mockDisconnect(...args), uninstall: (...args: unknown[]) => mockUninstall(...args), + updateEnv: (...args: unknown[]) => mockUpdateEnv(...args), configAssist: vi.fn(), }, })); @@ -33,6 +35,7 @@ describe('InstalledServerDetail', () => { mockConnect.mockReset(); mockDisconnect.mockReset(); mockUninstall.mockReset(); + mockUpdateEnv.mockReset(); }); it('renders server name and description', () => { @@ -201,6 +204,78 @@ describe('InstalledServerDetail', () => { expect(screen.getByText('Timed out')).toBeInTheDocument(); }); + // ---------------------------------------------------------------------- + // Env reconfiguration (issue #3039) + // ---------------------------------------------------------------------- + + it('opens the reconfigure form with one input per env key', () => { + render( + {}} /> + ); + fireEvent.click(screen.getByRole('button', { name: 'Reconfigure' })); + expect(screen.getByLabelText('API_KEY')).toBeInTheDocument(); + expect(screen.getByLabelText('DB_URL')).toBeInTheDocument(); + expect(screen.getByRole('button', { name: 'Save & reconnect' })).toBeInTheDocument(); + }); + + it('validates that every env key is filled before saving', async () => { + render( + {}} /> + ); + fireEvent.click(screen.getByRole('button', { name: 'Reconfigure' })); + // Fill only one of the two keys. + fireEvent.change(screen.getByLabelText('API_KEY'), { target: { value: 'k' } }); + await act(async () => { + fireEvent.click(screen.getByRole('button', { name: 'Save & reconnect' })); + }); + expect(screen.getByText('"DB_URL" is required')).toBeInTheDocument(); + expect(mockUpdateEnv).not.toHaveBeenCalled(); + }); + + it('calls updateEnv with all values and shows success on reconnect', async () => { + mockUpdateEnv.mockResolvedValue({ + server_id: 'srv-1', + status: 'connected', + env_keys: ['API_KEY', 'DB_URL'], + tools: [], + }); + render( + {}} /> + ); + fireEvent.click(screen.getByRole('button', { name: 'Reconfigure' })); + fireEvent.change(screen.getByLabelText('API_KEY'), { target: { value: 'new-key' } }); + fireEvent.change(screen.getByLabelText('DB_URL'), { target: { value: 'new-url' } }); + await act(async () => { + fireEvent.click(screen.getByRole('button', { name: 'Save & reconnect' })); + }); + expect(mockUpdateEnv).toHaveBeenCalledWith({ + server_id: 'srv-1', + env: { API_KEY: 'new-key', DB_URL: 'new-url' }, + }); + await waitFor(() => + expect(screen.getByText('Environment updated and reconnected.')).toBeInTheDocument() + ); + }); + + it('surfaces an error when reconnect after update fails', async () => { + mockUpdateEnv.mockResolvedValue({ + server_id: 'srv-1', + status: 'disconnected', + env_keys: ['API_KEY', 'DB_URL'], + error: 'bad token', + }); + render( + {}} /> + ); + fireEvent.click(screen.getByRole('button', { name: 'Reconfigure' })); + fireEvent.change(screen.getByLabelText('API_KEY'), { target: { value: 'k' } }); + fireEvent.change(screen.getByLabelText('DB_URL'), { target: { value: 'u' } }); + await act(async () => { + fireEvent.click(screen.getByRole('button', { name: 'Save & reconnect' })); + }); + await waitFor(() => expect(screen.getByText('bad token')).toBeInTheDocument()); + }); + // ---------------------------------------------------------------------- // Tool Execution Playground gating (PR review fix) // ---------------------------------------------------------------------- diff --git a/app/src/components/channels/mcp/InstalledServerDetail.tsx b/app/src/components/channels/mcp/InstalledServerDetail.tsx index d4641e4ee..4d304e23b 100644 --- a/app/src/components/channels/mcp/InstalledServerDetail.tsx +++ b/app/src/components/channels/mcp/InstalledServerDetail.tsx @@ -33,7 +33,15 @@ const InstalledServerDetail = ({ const [error, setError] = useState(null); const [confirmUninstall, setConfirmUninstall] = useState(false); const [showAssistant, setShowAssistant] = useState(false); - const [suggestedEnv, setSuggestedEnv] = useState | null>(null); + // Reconfigure form: when open, renders one input per env key so the user can + // supply replacement values and reconnect without uninstall/reinstall + // (issue #3039 env-reconfiguration). Values are never pre-filled from the + // server (we only ever hold key names) — except when the config assistant + // suggests values, which seed `reconfigValues` for the user to confirm. + const [reconfigOpen, setReconfigOpen] = useState(false); + const [reconfigValues, setReconfigValues] = useState>({}); + const [showReconfig, setShowReconfig] = useState>({}); + const [reconfigDone, setReconfigDone] = useState(false); // When non-null, the Tool Execution Playground modal is rendered for // this tool. Cleared on close. Only meaningful while the server is // connected (the gate is enforced at the McpToolList rendering site). @@ -107,10 +115,58 @@ const InstalledServerDetail = ({ }); }, [server.server_id, runBusy, onUninstalled]); - const handleApplySuggestedEnv = useCallback((env: Record) => { - setSuggestedEnv(env); - log('suggested_env applied, keys=%o', Object.keys(env)); - }, []); + const openReconfigure = useCallback( + (prefill?: Record) => { + const initial: Record = {}; + const initialVisibility: Record = {}; + for (const key of server.env_keys) { + initial[key] = prefill?.[key] ?? ''; + initialVisibility[key] = false; + } + setReconfigValues(initial); + setShowReconfig(initialVisibility); + setReconfigDone(false); + setReconfigOpen(true); + }, + [server.env_keys] + ); + + // The config assistant suggests values — seed the reconfigure form with them + // so the user can confirm/complete before we persist + reconnect. Suggested + // sets may be partial; the form requires every key so a reconnect never drops + // a required var (issue #3039 gap B6 — suggested values were never persisted). + const handleApplySuggestedEnv = useCallback( + (env: Record) => { + log('suggested_env received, opening reconfigure form keys=%o', Object.keys(env)); + setShowAssistant(false); + openReconfigure(env); + }, + [openReconfigure] + ); + + const handleSaveReconfigure = useCallback(() => { + void runBusy(async () => { + // Replace-all semantics (update_env DELETEs then INSERTs): every key must + // have a value or the server loses required env on reconnect. Mirror the + // install dialog's validation. + for (const key of server.env_keys) { + if (!reconfigValues[key]?.trim()) { + throw new Error(t('mcp.install.missingRequired').replace('{key}', key)); + } + } + log('reconfigure save server_id=%s', server.server_id); + const result = await mcpClientsApi.updateEnv({ + server_id: server.server_id, + env: reconfigValues, + }); + setTools(result.tools ?? []); + if (result.status !== 'connected') { + throw new Error(result.error ?? t('mcp.detail.reconfigureReconnectFailed')); + } + setReconfigDone(true); + setReconfigOpen(false); + }); + }, [server.env_keys, server.server_id, reconfigValues, runBusy, t]); return (
@@ -152,16 +208,10 @@ const InstalledServerDetail = ({
)} - {/* Suggested env notice */} - {suggestedEnv && ( -
-

{t('mcp.detail.suggestedEnvReady')}

-

- {t('mcp.detail.suggestedEnvBody').replace( - '{keys}', - Object.keys(suggestedEnv).join(', ') - )} -

+ {/* Reconfigure success notice */} + {reconfigDone && ( +
+ {t('mcp.detail.reconfigureSuccess')}
)} @@ -224,21 +274,75 @@ const InstalledServerDetail = ({ )}
- {/* Env keys (names only) */} + {/* Env keys (names only) + reconfigure affordance */} {server.env_keys.length > 0 && ( -
-

- {t('mcp.detail.envVars')} -

-
- {server.env_keys.map(key => ( - - {key} - - ))} +
+
+

+ {t('mcp.detail.envVars')} +

+
+ {!reconfigOpen && ( +
+ {server.env_keys.map(key => ( + + {key} + + ))} +
+ )} + {reconfigOpen && ( +
+

+ {t('mcp.detail.reconfigureHint')} +

+ {server.env_keys.map(key => ( +
+ +
+ + setReconfigValues(prev => ({ ...prev, [key]: e.target.value })) + } + placeholder={t('mcp.install.enterValue').replace('{key}', key)} + disabled={busy} + className="flex-1 rounded-lg border border-stone-200 dark:border-neutral-700 bg-white dark:bg-neutral-900 px-3 py-1.5 text-xs text-stone-800 dark:text-neutral-100 placeholder:text-stone-400 dark:placeholder:text-neutral-500 focus:outline-none focus:ring-2 focus:ring-primary-500/40 disabled:opacity-50" + /> + +
+
+ ))} + +
+ )}
)} diff --git a/app/src/components/channels/mcp/McpServersTab.test.tsx b/app/src/components/channels/mcp/McpServersTab.test.tsx index bef365e46..e5567afdf 100644 --- a/app/src/components/channels/mcp/McpServersTab.test.tsx +++ b/app/src/components/channels/mcp/McpServersTab.test.tsx @@ -83,6 +83,7 @@ describe('McpServersTab', () => { mockStatus.mockReset(); mockInstall.mockReset(); mockConnect.mockReset(); + mockConnect.mockResolvedValue({ server_id: '', status: 'connected', tools: [] }); mockDisconnect.mockReset(); mockUninstall.mockReset(); mockRegistryGet.mockReset(); diff --git a/app/src/lib/i18n/ar.ts b/app/src/lib/i18n/ar.ts index 9f59dfcaa..762032b50 100644 --- a/app/src/lib/i18n/ar.ts +++ b/app/src/lib/i18n/ar.ts @@ -1122,6 +1122,13 @@ const messages: TranslationMap = { 'mcp.detail.confirmUninstallAction': 'نعم، قم بإلغاء التثبيت', 'mcp.detail.uninstall': 'قم بإلغاء التثبيت', 'mcp.detail.envVars': 'متغيرات البيئة', + 'mcp.detail.reconfigure': 'إعادة التهيئة', + 'mcp.detail.reconfigureHint': + 'أدخل قيمًا جديدة لكل متغير، ثم أعد الاتصال. سيؤدي ذلك إلى استبدال القيم المخزَّنة.', + 'mcp.detail.saveReconnect': 'حفظ وإعادة الاتصال', + 'mcp.detail.reconfigureSaving': 'جارٍ الحفظ…', + 'mcp.detail.reconfigureSuccess': 'تم تحديث البيئة وإعادة الاتصال.', + 'mcp.detail.reconfigureReconnectFailed': 'تم الحفظ، لكن فشلت إعادة الاتصال بالقيم الجديدة.', 'mcp.detail.tools': 'الأدوات', 'onboarding.skipForNow': 'التخطي الآن', 'onboarding.localAI.continueWithCloud': 'متابعة مع السحابة', diff --git a/app/src/lib/i18n/bn.ts b/app/src/lib/i18n/bn.ts index 21c1cd4fc..a96af1eeb 100644 --- a/app/src/lib/i18n/bn.ts +++ b/app/src/lib/i18n/bn.ts @@ -1138,6 +1138,14 @@ const messages: TranslationMap = { 'mcp.detail.confirmUninstallAction': 'হ্যাঁ, আনইনস্টল করুন', 'mcp.detail.uninstall': 'আনইনস্টল', 'mcp.detail.envVars': 'এনভায়রনমেন্ট ভেরিয়েবল', + 'mcp.detail.reconfigure': 'পুনঃকনফিগার করুন', + 'mcp.detail.reconfigureHint': + 'প্রতিটি ভেরিয়েবলের জন্য নতুন মান লিখুন, তারপর পুনরায় সংযোগ করুন। এটি সংরক্ষিত মান প্রতিস্থাপন করে।', + 'mcp.detail.saveReconnect': 'সংরক্ষণ ও পুনঃসংযোগ', + 'mcp.detail.reconfigureSaving': 'সংরক্ষণ করা হচ্ছে…', + 'mcp.detail.reconfigureSuccess': 'এনভায়রনমেন্ট আপডেট ও পুনঃসংযোগ করা হয়েছে।', + 'mcp.detail.reconfigureReconnectFailed': + 'সংরক্ষিত হয়েছে, তবে নতুন মান দিয়ে পুনঃসংযোগ ব্যর্থ হয়েছে।', 'mcp.detail.tools': 'টুলস', 'onboarding.skipForNow': 'এখনই এড়িয়ে যান', 'onboarding.localAI.continueWithCloud': 'ক্লাউডের সাথে চালিয়ে যান', diff --git a/app/src/lib/i18n/de.ts b/app/src/lib/i18n/de.ts index 95fa2102a..b29d21565 100644 --- a/app/src/lib/i18n/de.ts +++ b/app/src/lib/i18n/de.ts @@ -1173,6 +1173,14 @@ const messages: TranslationMap = { 'mcp.detail.confirmUninstallAction': 'Ja, deinstallieren', 'mcp.detail.uninstall': 'Deinstallieren', 'mcp.detail.envVars': 'Umgebungsvariablen', + 'mcp.detail.reconfigure': 'Neu konfigurieren', + 'mcp.detail.reconfigureHint': + 'Geben Sie neue Werte für jede Variable ein und stellen Sie die Verbindung wieder her. Dadurch werden die gespeicherten Werte ersetzt.', + 'mcp.detail.saveReconnect': 'Speichern und neu verbinden', + 'mcp.detail.reconfigureSaving': 'Wird gespeichert…', + 'mcp.detail.reconfigureSuccess': 'Umgebung aktualisiert und neu verbunden.', + 'mcp.detail.reconfigureReconnectFailed': + 'Gespeichert, aber das Neuverbinden mit den neuen Werten ist fehlgeschlagen.', 'mcp.detail.tools': 'Werkzeuge', 'onboarding.skipForNow': 'Vorerst überspringen', 'onboarding.localAI.continueWithCloud': 'Fahren Sie mit der Cloud fort.', diff --git a/app/src/lib/i18n/en.ts b/app/src/lib/i18n/en.ts index 3443c761b..202932bcc 100644 --- a/app/src/lib/i18n/en.ts +++ b/app/src/lib/i18n/en.ts @@ -1267,6 +1267,13 @@ const en: TranslationMap = { 'mcp.detail.confirmUninstallAction': 'Yes, uninstall', 'mcp.detail.uninstall': 'Uninstall', 'mcp.detail.envVars': 'Environment variables', + 'mcp.detail.reconfigure': 'Reconfigure', + 'mcp.detail.reconfigureHint': + 'Enter new values for every variable, then reconnect. This replaces the stored values.', + 'mcp.detail.saveReconnect': 'Save & reconnect', + 'mcp.detail.reconfigureSaving': 'Saving…', + 'mcp.detail.reconfigureSuccess': 'Environment updated and reconnected.', + 'mcp.detail.reconfigureReconnectFailed': 'Saved, but reconnecting with the new values failed.', 'mcp.detail.tools': 'Tools', 'onboarding.skipForNow': 'Skip for Now', 'onboarding.localAI.continueWithCloud': 'Continue with Cloud', diff --git a/app/src/lib/i18n/es.ts b/app/src/lib/i18n/es.ts index 44e40d448..41f775290 100644 --- a/app/src/lib/i18n/es.ts +++ b/app/src/lib/i18n/es.ts @@ -1170,6 +1170,14 @@ const messages: TranslationMap = { 'mcp.detail.confirmUninstallAction': 'Sí, desinstalar', 'mcp.detail.uninstall': 'Desinstalar', 'mcp.detail.envVars': 'Variables ambientales', + 'mcp.detail.reconfigure': 'Reconfigurar', + 'mcp.detail.reconfigureHint': + 'Introduce nuevos valores para cada variable y vuelve a conectar. Esto reemplaza los valores guardados.', + 'mcp.detail.saveReconnect': 'Guardar y reconectar', + 'mcp.detail.reconfigureSaving': 'Guardando…', + 'mcp.detail.reconfigureSuccess': 'Entorno actualizado y reconectado.', + 'mcp.detail.reconfigureReconnectFailed': + 'Guardado, pero no se pudo reconectar con los nuevos valores.', 'mcp.detail.tools': 'Herramientas', 'onboarding.skipForNow': 'Saltar por ahora', 'onboarding.localAI.continueWithCloud': 'Continuar con la nube', diff --git a/app/src/lib/i18n/fr.ts b/app/src/lib/i18n/fr.ts index 8c209e1aa..555351444 100644 --- a/app/src/lib/i18n/fr.ts +++ b/app/src/lib/i18n/fr.ts @@ -1172,6 +1172,14 @@ const messages: TranslationMap = { 'mcp.detail.confirmUninstallAction': 'Oui, désinstaller', 'mcp.detail.uninstall': 'Désinstaller', 'mcp.detail.envVars': "Variables d'environnement", + 'mcp.detail.reconfigure': 'Reconfigurer', + 'mcp.detail.reconfigureHint': + 'Saisissez de nouvelles valeurs pour chaque variable, puis reconnectez-vous. Cela remplace les valeurs enregistrées.', + 'mcp.detail.saveReconnect': 'Enregistrer et reconnecter', + 'mcp.detail.reconfigureSaving': 'Enregistrement…', + 'mcp.detail.reconfigureSuccess': 'Environnement mis à jour et reconnecté.', + 'mcp.detail.reconfigureReconnectFailed': + 'Enregistré, mais la reconnexion avec les nouvelles valeurs a échoué.', 'mcp.detail.tools': 'Outils', 'onboarding.skipForNow': "Passer pour l'instant", 'onboarding.localAI.continueWithCloud': 'Continuer avec Cloud', diff --git a/app/src/lib/i18n/hi.ts b/app/src/lib/i18n/hi.ts index 9174de2fc..67193b297 100644 --- a/app/src/lib/i18n/hi.ts +++ b/app/src/lib/i18n/hi.ts @@ -1139,6 +1139,14 @@ const messages: TranslationMap = { 'mcp.detail.confirmUninstallAction': 'हाँ, अनइंस्टॉल करें', 'mcp.detail.uninstall': 'अनइंस्टॉल करें', 'mcp.detail.envVars': 'पर्यावरण चर', + 'mcp.detail.reconfigure': 'पुन: कॉन्फ़िगर करें', + 'mcp.detail.reconfigureHint': + 'हर वेरिएबल के लिए नए मान दर्ज करें, फिर पुन: कनेक्ट करें। यह संग्रहीत मानों को बदल देता है।', + 'mcp.detail.saveReconnect': 'सहेजें और पुन: कनेक्ट करें', + 'mcp.detail.reconfigureSaving': 'सहेजा जा रहा है…', + 'mcp.detail.reconfigureSuccess': 'एनवायरनमेंट अपडेट हो गया और पुन: कनेक्ट हो गया।', + 'mcp.detail.reconfigureReconnectFailed': + 'सहेजा गया, लेकिन नए मानों के साथ पुन: कनेक्ट करना विफल रहा।', 'mcp.detail.tools': 'उपकरण', 'onboarding.skipForNow': 'अभी के लिए छोड़ें', 'onboarding.localAI.continueWithCloud': 'बादल के साथ जारी रखें', diff --git a/app/src/lib/i18n/id.ts b/app/src/lib/i18n/id.ts index 4b885c4df..4e58df701 100644 --- a/app/src/lib/i18n/id.ts +++ b/app/src/lib/i18n/id.ts @@ -1146,6 +1146,14 @@ const messages: TranslationMap = { 'mcp.detail.confirmUninstallAction': 'Ya, hapus instalan', 'mcp.detail.uninstall': 'Hapus instalan', 'mcp.detail.envVars': 'Variabel lingkungan', + 'mcp.detail.reconfigure': 'Konfigurasi ulang', + 'mcp.detail.reconfigureHint': + 'Masukkan nilai baru untuk setiap variabel, lalu hubungkan kembali. Ini menggantikan nilai yang tersimpan.', + 'mcp.detail.saveReconnect': 'Simpan dan hubungkan kembali', + 'mcp.detail.reconfigureSaving': 'Menyimpan…', + 'mcp.detail.reconfigureSuccess': 'Lingkungan diperbarui dan terhubung kembali.', + 'mcp.detail.reconfigureReconnectFailed': + 'Tersimpan, tetapi gagal menghubungkan kembali dengan nilai baru.', 'mcp.detail.tools': 'Alat', 'onboarding.skipForNow': 'Lewati Sekarang', 'onboarding.localAI.continueWithCloud': 'Lanjutkan dengan Cloud', diff --git a/app/src/lib/i18n/it.ts b/app/src/lib/i18n/it.ts index 2ccf962a5..db8e40e9b 100644 --- a/app/src/lib/i18n/it.ts +++ b/app/src/lib/i18n/it.ts @@ -1166,6 +1166,14 @@ const messages: TranslationMap = { 'mcp.detail.confirmUninstallAction': 'Sì, disinstalla', 'mcp.detail.uninstall': 'Disinstalla', 'mcp.detail.envVars': 'Variabili di ambiente', + 'mcp.detail.reconfigure': 'Riconfigura', + 'mcp.detail.reconfigureHint': + 'Inserisci nuovi valori per ogni variabile, quindi riconnetti. Questo sostituisce i valori memorizzati.', + 'mcp.detail.saveReconnect': 'Salva e riconnetti', + 'mcp.detail.reconfigureSaving': 'Salvataggio…', + 'mcp.detail.reconfigureSuccess': 'Ambiente aggiornato e riconnesso.', + 'mcp.detail.reconfigureReconnectFailed': + 'Salvato, ma la riconnessione con i nuovi valori non è riuscita.', 'mcp.detail.tools': 'Strumenti', 'onboarding.skipForNow': 'Salta per ora', 'onboarding.localAI.continueWithCloud': 'Continua con Cloud', diff --git a/app/src/lib/i18n/ko.ts b/app/src/lib/i18n/ko.ts index 33e179022..85716911d 100644 --- a/app/src/lib/i18n/ko.ts +++ b/app/src/lib/i18n/ko.ts @@ -1136,6 +1136,13 @@ const messages: TranslationMap = { 'mcp.detail.confirmUninstallAction': '예, 제거합니다.', 'mcp.detail.uninstall': '제거', 'mcp.detail.envVars': '환경 변수', + 'mcp.detail.reconfigure': '재구성', + 'mcp.detail.reconfigureHint': + '모든 변수에 새 값을 입력한 다음 다시 연결하세요. 저장된 값이 대체됩니다.', + 'mcp.detail.saveReconnect': '저장 후 다시 연결', + 'mcp.detail.reconfigureSaving': '저장 중…', + 'mcp.detail.reconfigureSuccess': '환경이 업데이트되고 다시 연결되었습니다.', + 'mcp.detail.reconfigureReconnectFailed': '저장했지만 새 값으로 다시 연결하지 못했습니다.', 'mcp.detail.tools': '도구', 'onboarding.skipForNow': '지금 건너뛰기', 'onboarding.localAI.continueWithCloud': '클라우드 계속하기', diff --git a/app/src/lib/i18n/pl.ts b/app/src/lib/i18n/pl.ts index 1eaa55f23..ed3783c91 100644 --- a/app/src/lib/i18n/pl.ts +++ b/app/src/lib/i18n/pl.ts @@ -1159,6 +1159,14 @@ const messages: TranslationMap = { 'mcp.detail.confirmUninstallAction': 'Tak, odinstaluj', 'mcp.detail.uninstall': 'Odinstaluj', 'mcp.detail.envVars': 'Zmienne środowiskowe', + 'mcp.detail.reconfigure': 'Skonfiguruj ponownie', + 'mcp.detail.reconfigureHint': + 'Wprowadź nowe wartości dla każdej zmiennej, a następnie połącz ponownie. Spowoduje to zastąpienie zapisanych wartości.', + 'mcp.detail.saveReconnect': 'Zapisz i połącz ponownie', + 'mcp.detail.reconfigureSaving': 'Zapisywanie…', + 'mcp.detail.reconfigureSuccess': 'Środowisko zaktualizowane i połączone ponownie.', + 'mcp.detail.reconfigureReconnectFailed': + 'Zapisano, ale ponowne połączenie z nowymi wartościami nie powiodło się.', 'mcp.detail.tools': 'Narzędzia', 'onboarding.skipForNow': 'Pomiń na razie', 'onboarding.localAI.continueWithCloud': 'Kontynuuj z chmurą', diff --git a/app/src/lib/i18n/pt.ts b/app/src/lib/i18n/pt.ts index 1612b29a1..e9d9299d2 100644 --- a/app/src/lib/i18n/pt.ts +++ b/app/src/lib/i18n/pt.ts @@ -1170,6 +1170,13 @@ const messages: TranslationMap = { 'mcp.detail.confirmUninstallAction': 'Sim, desinstalar', 'mcp.detail.uninstall': 'Desinstalar', 'mcp.detail.envVars': 'Variáveis de ambiente', + 'mcp.detail.reconfigure': 'Reconfigurar', + 'mcp.detail.reconfigureHint': + 'Insira novos valores para cada variável e reconecte. Isso substitui os valores armazenados.', + 'mcp.detail.saveReconnect': 'Salvar e reconectar', + 'mcp.detail.reconfigureSaving': 'Salvando…', + 'mcp.detail.reconfigureSuccess': 'Ambiente atualizado e reconectado.', + 'mcp.detail.reconfigureReconnectFailed': 'Salvo, mas a reconexão com os novos valores falhou.', 'mcp.detail.tools': 'Ferramentas', 'onboarding.skipForNow': 'Ignorar por agora', 'onboarding.localAI.continueWithCloud': 'Continuar com a nuvem', diff --git a/app/src/lib/i18n/ru.ts b/app/src/lib/i18n/ru.ts index 5ded85544..4e5735659 100644 --- a/app/src/lib/i18n/ru.ts +++ b/app/src/lib/i18n/ru.ts @@ -1153,6 +1153,14 @@ const messages: TranslationMap = { 'mcp.detail.confirmUninstallAction': 'Да, удалить', 'mcp.detail.uninstall': 'Удалить', 'mcp.detail.envVars': 'Переменные среды', + 'mcp.detail.reconfigure': 'Перенастроить', + 'mcp.detail.reconfigureHint': + 'Введите новые значения для каждой переменной, затем переподключитесь. Это заменит сохранённые значения.', + 'mcp.detail.saveReconnect': 'Сохранить и переподключиться', + 'mcp.detail.reconfigureSaving': 'Сохранение…', + 'mcp.detail.reconfigureSuccess': 'Окружение обновлено, выполнено переподключение.', + 'mcp.detail.reconfigureReconnectFailed': + 'Сохранено, но переподключиться с новыми значениями не удалось.', 'mcp.detail.tools': 'Инструменты', 'onboarding.skipForNow': 'Пропустить сейчас', 'onboarding.localAI.continueWithCloud': 'Продолжить с Облако', diff --git a/app/src/lib/i18n/zh-CN.ts b/app/src/lib/i18n/zh-CN.ts index 34b5aa53c..f1bb9fd90 100644 --- a/app/src/lib/i18n/zh-CN.ts +++ b/app/src/lib/i18n/zh-CN.ts @@ -1086,6 +1086,12 @@ const messages: TranslationMap = { 'mcp.detail.confirmUninstallAction': '是的，卸载', 'mcp.detail.uninstall': '卸载', 'mcp.detail.envVars': '环境变量', + 'mcp.detail.reconfigure': '重新配置', + 'mcp.detail.reconfigureHint': '为每个变量输入新值，然后重新连接。这将替换已存储的值。', + 'mcp.detail.saveReconnect': '保存并重新连接', + 'mcp.detail.reconfigureSaving': '正在保存…', + 'mcp.detail.reconfigureSuccess': '环境已更新并重新连接。', + 'mcp.detail.reconfigureReconnectFailed': '已保存，但使用新值重新连接失败。', 'mcp.detail.tools': '工具', 'onboarding.skipForNow': '暂时跳过', 'onboarding.localAI.continueWithCloud': '继续使用云', diff --git a/app/src/pages/Skills.tsx b/app/src/pages/Skills.tsx index 78032c7ee..1c8cd7a3b 100644 --- a/app/src/pages/Skills.tsx +++ b/app/src/pages/Skills.tsx @@ -2,6 +2,7 @@ import { useCallback, useEffect, useMemo, useState } from 'react'; import { useLocation, useNavigate } from 'react-router-dom'; import ChannelSetupModal from '../components/channels/ChannelSetupModal'; +import McpServersTab from '../components/channels/mcp/McpServersTab'; import ComposioConnectModal from '../components/composio/ComposioConnectModal'; import { composioToolkitMeta, @@ -276,35 +277,6 @@ function ChannelTile({ def, status, icon, testId, onOpen }: ChannelTileProps) { ); } -function McpComingSoonPanel() { - const { t } = useT(); - return ( - - - - } - title={t('skills.mcpComingSoon.title')} - description={t('skills.mcpComingSoon.description')} - footer={ - - {t('common.comingSoon')} - - } - /> - ); -} - function ComposioApiKeyEmptyState({ onOpenSettings }: { onOpenSettings: () => void }) { const { t } = useT(); return ( @@ -1130,7 +1102,12 @@ export default function Skills() { {t('channels.mcp.description')}

- + {/* The real browse/install/manage surface (issue #3039). + McpServersTab manages its own height via h-full, so it + needs a sized container to fill. */} +
+ +
)} diff --git a/app/src/pages/__tests__/Skills.mcp-coming-soon.test.tsx b/app/src/pages/__tests__/Skills.mcp-coming-soon.test.tsx index 95becce43..4ad0721d9 100644 --- a/app/src/pages/__tests__/Skills.mcp-coming-soon.test.tsx +++ b/app/src/pages/__tests__/Skills.mcp-coming-soon.test.tsx @@ -1,4 +1,4 @@ -import { fireEvent, screen } from '@testing-library/react'; +import { fireEvent, screen, waitFor } from '@testing-library/react'; import { describe, expect, it, vi } from 'vitest'; import '../../test/mockDefaultSkillStatusHooks'; @@ -34,14 +34,31 @@ vi.mock('../../lib/composio/hooks', () => ({ }), })); +vi.mock('../../services/api/mcpClientsApi', () => ({ + mcpClientsApi: { + installedList: vi.fn().mockResolvedValue([]), + status: vi.fn().mockResolvedValue([]), + registrySearch: vi.fn().mockResolvedValue({ servers: [], page: 1, total_pages: 1 }), + registryGet: vi.fn().mockResolvedValue(null), + install: vi.fn().mockResolvedValue({}), + connect: vi.fn().mockResolvedValue({ tools: [] }), + disconnect: vi.fn().mockResolvedValue({}), + uninstall: vi.fn().mockResolvedValue({}), + configAssist: vi.fn().mockResolvedValue({}), + }, +})); + describe('Skills page — MCP tab', () => { - it('shows a coming soon placeholder for MCP server management', () => { + it('renders the live MCP servers tab (not a coming-soon placeholder)', async () => { renderWithProviders(, { initialEntries: ['/skills'] }); fireEvent.click(screen.getByRole('tab', { name: 'MCP Servers' })); - expect(screen.getAllByRole('heading', { name: 'MCP Servers' })).toHaveLength(2); - expect(screen.getByText(/MCP server management is coming soon/i)).toBeInTheDocument(); - expect(screen.getByText('Coming Soon')).toBeInTheDocument(); + await waitFor(() => { + expect( + screen.getByText('No MCP servers installed yet.') || + screen.getByText('Loading MCP servers...') + ).toBeInTheDocument(); + }); }); }); diff --git a/app/src/services/api/mcpClientsApi.test.ts b/app/src/services/api/mcpClientsApi.test.ts index 4ddffa7b4..71c436bb0 100644 --- a/app/src/services/api/mcpClientsApi.test.ts +++ b/app/src/services/api/mcpClientsApi.test.ts @@ -274,6 +274,65 @@ describe('mcpClientsApi', () => { }); }); + describe('updateEnv', () => { + it('calls update_env and returns reconnect status', async () => { + mockCallCoreRpc.mockResolvedValueOnce({ + server_id: 'srv-1', + status: 'connected', + env_keys: ['API_KEY'], + tools: [], + }); + + const { mcpClientsApi } = await import('./mcpClientsApi'); + const result = await mcpClientsApi.updateEnv({ + server_id: 'srv-1', + env: { API_KEY: 'rotated' }, + }); + + expect(mockCallCoreRpc).toHaveBeenCalledWith({ + method: 'openhuman.mcp_clients_update_env', + params: { server_id: 'srv-1', env: { API_KEY: 'rotated' } }, + }); + expect(result.status).toBe('connected'); + expect(result.env_keys).toEqual(['API_KEY']); + }); + }); + + describe('registry settings', () => { + it('registrySettingsGet returns the is-set booleans', async () => { + mockCallCoreRpc.mockResolvedValueOnce({ + smithery_api_key_set: true, + mcp_official_token_set: false, + mcp_official_base: null, + }); + + const { mcpClientsApi } = await import('./mcpClientsApi'); + const result = await mcpClientsApi.registrySettingsGet(); + + expect(mockCallCoreRpc).toHaveBeenCalledWith({ + method: 'openhuman.mcp_clients_registry_settings_get', + params: {}, + }); + expect(result.smithery_api_key_set).toBe(true); + expect(result.mcp_official_token_set).toBe(false); + }); + + it('registrySettingsSet forwards only the provided fields', async () => { + mockCallCoreRpc.mockResolvedValueOnce({ + smithery_api_key_set: true, + mcp_official_token_set: false, + }); + + const { mcpClientsApi } = await import('./mcpClientsApi'); + await mcpClientsApi.registrySettingsSet({ smithery_api_key: 'sk-x' }); + + expect(mockCallCoreRpc).toHaveBeenCalledWith({ + method: 'openhuman.mcp_clients_registry_settings_set', + params: { smithery_api_key: 'sk-x' }, + }); + }); + }); + describe('configAssist', () => { it('calls config_assist and returns reply', async () => { mockCallCoreRpc.mockResolvedValueOnce({ diff --git a/app/src/services/api/mcpClientsApi.ts b/app/src/services/api/mcpClientsApi.ts index 6d41ff11e..89f2fa0dd 100644 --- a/app/src/services/api/mcpClientsApi.ts +++ b/app/src/services/api/mcpClientsApi.ts @@ -70,6 +70,21 @@ interface ConfigAssistResult { suggested_env?: Record; } +interface UpdateEnvResult { + server_id: string; + status: 'connected' | 'disconnected'; + env_keys: string[]; + tools?: McpTool[]; + error?: string; +} + +/** Non-secret registry-credentials snapshot. Secret *values* are never returned. */ +export interface RegistrySettings { + smithery_api_key_set: boolean; + mcp_official_token_set: boolean; + mcp_official_base?: string | null; +} + // --------------------------------------------------------------------------- // API // --------------------------------------------------------------------------- @@ -135,6 +150,57 @@ export const mcpClientsApi = { return result.server; }, + /** + * Replace the stored env values for an installed server and reconnect so the + * new credentials take effect (reconfigure / rotate keys without + * uninstall+reinstall). `status` is `connected` when the reconnect succeeded. + */ + updateEnv: async (params: { + server_id: string; + env: Record; + }): Promise => { + log('update_env server_id=%s env_keys=%o', params.server_id, Object.keys(params.env)); + const result = await callCoreRpc({ + method: 'openhuman.mcp_clients_update_env', + params, + }); + log('update_env status=%s', result.status); + return result; + }, + + /** Read which registry credentials are configured (booleans only, no values). */ + registrySettingsGet: async (): Promise => { + log('registry_settings_get'); + const result = await callCoreRpc({ + method: 'openhuman.mcp_clients_registry_settings_get', + params: {}, + }); + log( + 'registry_settings_get smithery=%s official=%s', + result.smithery_api_key_set, + result.mcp_official_token_set + ); + return result; + }, + + /** + * Persist registry credentials. Omit a field to leave it unchanged, pass an + * empty string to clear it. Secrets are write-only — the response is the same + * non-secret snapshot as registrySettingsGet. + */ + registrySettingsSet: async (params: { + smithery_api_key?: string; + mcp_official_base?: string; + mcp_official_token?: string; + }): Promise => { + log('registry_settings_set fields=%o', Object.keys(params)); + const result = await callCoreRpc({ + method: 'openhuman.mcp_clients_registry_settings_set', + params, + }); + return result; + }, + /** Uninstall a server by ID. */ uninstall: async (server_id: string): Promise => { log('uninstall server_id=%s', server_id); diff --git a/app/src/services/api/mcpSetupApi.test.ts b/app/src/services/api/mcpSetupApi.test.ts new file mode 100644 index 000000000..c97662d4d --- /dev/null +++ b/app/src/services/api/mcpSetupApi.test.ts @@ -0,0 +1,88 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +const mockCallCoreRpc = vi.fn(); + +vi.mock('../coreRpcClient', () => ({ + callCoreRpc: (...args: unknown[]) => mockCallCoreRpc(...args), +})); + +describe('mcpSetupApi', () => { + beforeEach(() => { + mockCallCoreRpc.mockReset(); + }); + + it('search calls mcp_setup_search', async () => { + mockCallCoreRpc.mockResolvedValueOnce({ servers: [], page: 1, total_pages: 1 }); + const { mcpSetupApi } = await import('./mcpSetupApi'); + await mcpSetupApi.search({ query: 'notion' }); + expect(mockCallCoreRpc).toHaveBeenCalledWith({ + method: 'openhuman.mcp_setup_search', + params: { query: 'notion' }, + }); + }); + + it('get unwraps the server detail', async () => { + const server = { qualified_name: 'q', display_name: 'd', connections: [] }; + mockCallCoreRpc.mockResolvedValueOnce({ server }); + const { mcpSetupApi } = await import('./mcpSetupApi'); + const result = await mcpSetupApi.get('q'); + expect(mockCallCoreRpc).toHaveBeenCalledWith({ + method: 'openhuman.mcp_setup_get', + params: { qualified_name: 'q' }, + }); + expect(result).toEqual(server); + }); + + it('requestSecret returns an opaque ref', async () => { + mockCallCoreRpc.mockResolvedValueOnce({ ref: 'secret://abc', key_name: 'NOTION_API_KEY' }); + const { mcpSetupApi } = await import('./mcpSetupApi'); + const result = await mcpSetupApi.requestSecret({ + key_name: 'NOTION_API_KEY', + prompt: 'Paste your token', + }); + expect(mockCallCoreRpc).toHaveBeenCalledWith({ + method: 'openhuman.mcp_setup_request_secret', + params: { key_name: 'NOTION_API_KEY', prompt: 'Paste your token' }, + }); + expect(result.ref).toBe('secret://abc'); + }); + + it('submitSecret forwards the ref + value', async () => { + mockCallCoreRpc.mockResolvedValueOnce({ ref: 'secret://abc', fulfilled: true }); + const { mcpSetupApi } = await import('./mcpSetupApi'); + const result = await mcpSetupApi.submitSecret({ ref_id: 'secret://abc', value: 'tok' }); + expect(mockCallCoreRpc).toHaveBeenCalledWith({ + method: 'openhuman.mcp_setup_submit_secret', + params: { ref_id: 'secret://abc', value: 'tok' }, + }); + expect(result.fulfilled).toBe(true); + }); + + it('testConnection returns ok + tools', async () => { + mockCallCoreRpc.mockResolvedValueOnce({ ok: true, tools: [{ name: 'search' }] }); + const { mcpSetupApi } = await import('./mcpSetupApi'); + const result = await mcpSetupApi.testConnection({ + qualified_name: 'q', + env_refs: { NOTION_API_KEY: 'secret://abc' }, + }); + expect(mockCallCoreRpc).toHaveBeenCalledWith({ + method: 'openhuman.mcp_setup_test_connection', + params: { qualified_name: 'q', env_refs: { NOTION_API_KEY: 'secret://abc' } }, + }); + expect(result.ok).toBe(true); + }); + + it('installAndConnect commits the install', async () => { + mockCallCoreRpc.mockResolvedValueOnce({ server_id: 'srv-1', status: 'connected', tools: [] }); + const { mcpSetupApi } = await import('./mcpSetupApi'); + const result = await mcpSetupApi.installAndConnect({ + qualified_name: 'q', + env_refs: { NOTION_API_KEY: 'secret://abc' }, + }); + expect(mockCallCoreRpc).toHaveBeenCalledWith({ + method: 'openhuman.mcp_setup_install_and_connect', + params: { qualified_name: 'q', env_refs: { NOTION_API_KEY: 'secret://abc' } }, + }); + expect(result.status).toBe('connected'); + }); +}); diff --git a/app/src/services/api/mcpSetupApi.ts b/app/src/services/api/mcpSetupApi.ts new file mode 100644 index 000000000..abe560c86 --- /dev/null +++ b/app/src/services/api/mcpSetupApi.ts @@ -0,0 +1,129 @@ +/** + * Typed RPC wrapper for the MCP **setup-agent** domain (`mcp_setup_*`). + * + * These RPCs power the agent-native "install MCP server with assistant" flow: + * search → get → request_secret (out-of-band native prompt) → test_connection → + * install_and_connect. Secret *values* never travel through here as plaintext + * the agent sees — `request_secret` returns an opaque `secret://` ref that + * the UI fulfils via `submitSecret`, and only refs are passed to + * `testConnection` / `installAndConnect`. + * + * Centralises the `openhuman.mcp_setup_` method-name strings so + * components never spell them out directly (issue #3039 gap B4). + */ +import debug from 'debug'; + +import type { + McpTool, + SmitheryServer, + SmitheryServerDetail, +} from '../../components/channels/mcp/types'; +import { callCoreRpc } from '../coreRpcClient'; + +const log = debug('mcp-setup:api'); + +interface SetupSearchResult { + servers: SmitheryServer[]; + page: number; + total_pages: number; +} + +interface SetupGetResult { + server: SmitheryServerDetail; +} + +interface RequestSecretResult { + ref: string; + key_name: string; +} + +interface SubmitSecretResult { + ref: string; + fulfilled: boolean; +} + +interface TestConnectionResult { + ok: boolean; + tools?: McpTool[]; + error?: string; +} + +interface InstallAndConnectResult { + server_id: string; + status: 'connected' | 'installed_disconnected'; + tools?: McpTool[]; + error?: string; +} + +export const mcpSetupApi = { + /** Search all enabled registries (Smithery + official). */ + search: async (params: { + query?: string; + page?: number; + page_size?: number; + }): Promise => { + log('search params=%o', params); + return callCoreRpc({ method: 'openhuman.mcp_setup_search', params }); + }, + + /** Fetch one server's detail with `required_env_keys` injected. */ + get: async (qualified_name: string): Promise => { + log('get qualified_name=%s', qualified_name); + const result = await callCoreRpc({ + method: 'openhuman.mcp_setup_get', + params: { qualified_name }, + }); + return result.server; + }, + + /** + * Ask the core to request a secret from the user out-of-band. Returns an + * opaque ref; the value is collected by the native SecretPromptDialog and + * submitted via {@link submitSecret}. + */ + requestSecret: async (params: { + key_name: string; + prompt: string; + }): Promise => { + log('request_secret key_name=%s', params.key_name); + return callCoreRpc({ + method: 'openhuman.mcp_setup_request_secret', + params, + }); + }, + + /** UI-side: fulfil a pending `request_secret` with the user-entered value. */ + submitSecret: async (params: { ref_id: string; value: string }): Promise => { + // Intentionally NOT logging the value. + log('submit_secret ref_id=%s', params.ref_id); + return callCoreRpc({ method: 'openhuman.mcp_setup_submit_secret', params }); + }, + + /** Dry-run a candidate install (spawn, list tools, tear down — nothing persisted). */ + testConnection: async (params: { + qualified_name: string; + env_refs: Record; + }): Promise => { + log('test_connection qualified_name=%s', params.qualified_name); + const result = await callCoreRpc({ + method: 'openhuman.mcp_setup_test_connection', + params, + }); + log('test_connection ok=%s', result.ok); + return result; + }, + + /** Commit the install + secrets, then connect and return the tool list. */ + installAndConnect: async (params: { + qualified_name: string; + env_refs: Record; + }): Promise => { + log('install_and_connect qualified_name=%s', params.qualified_name); + const result = await callCoreRpc({ + method: 'openhuman.mcp_setup_install_and_connect', + params, + }); + log('install_and_connect status=%s', result.status); + return result; + }, +}; diff --git a/docs/MCP_SETUP_AGENT.md b/docs/MCP_SETUP_AGENT.md index eb454a44d..b0db4910c 100644 --- a/docs/MCP_SETUP_AGENT.md +++ b/docs/MCP_SETUP_AGENT.md @@ -1,12 +1,16 @@ -# MCP Setup Agent — design sketch +# MCP Setup Agent A sub-agent that walks the user through installing, configuring, and connecting an MCP server from one of the upstream registries (`mcp_registry::registries`: Smithery, modelcontextprotocol/registry). -This document is a **design sketch** for follow-up implementation. Nothing -here is wired up yet beyond the underlying primitives in -`src/openhuman/mcp_registry/`. +**Status: implemented** (issue #3039). The agent archetype, its five +`mcp_setup_*` tools, the opaque-secret request/submit flow, and the +`install_and_connect` commit path are all live. The orchestrator delegates to +it via the `setup_mcp_server` delegate (the `mcp_setup` entry in +`src/openhuman/agent_registry/agents/orchestrator/agent.toml`), so a chat turn +like *"set up the Notion MCP server"* routes here. The sections below describe +how the flow works; paths reflect the shipped layout. --- @@ -31,7 +35,7 @@ subprocess, and the persistence. ## Tool surface -Four tools registered behind a `mcp_setup_*` namespace. All tool inputs +Five tools registered behind a `mcp_setup_*` namespace. All tool inputs and outputs are JSON; secret values **never** appear in either direction. | Tool | Input | Output | Notes | @@ -40,7 +44,7 @@ and outputs are JSON; secret values **never** appear in either direction. | `mcp_setup_get` | `{ qualified_name }` | `{ detail, required_env_keys }` | Wraps `registry_get`; pre-computes `required_env_keys` from the `config_schema` (same logic as `ops::collect_required_env_keys`). | | `mcp_setup_request_secret` | `{ key_name, prompt }` | `{ ref: "secret://" }` | Triggers an out-of-band UI prompt. Returns an opaque ref; raw value is held in a process-local in-memory map keyed by ref. | | `mcp_setup_test_connection` | `{ qualified_name, env_refs: { KEY: "secret://…" } }` | `{ ok, tools?: [McpTool], error?: string }` | Spawns the candidate subprocess in a **scratch** workspace, resolves refs to values just-in-time, runs `initialize` + `tools/list`, tears it down. No persistence. | -| `mcp_setup_install_and_connect` | `{ qualified_name, env_refs }` | `{ server_id, status, tools: [McpTool] }` | Resolves refs, persists the install + `mcp_client_env` rows, calls `connections::connect`. Refs are consumed (removed from the in-memory map) regardless of outcome. | +| `mcp_setup_install_and_connect` | `{ qualified_name, env_refs }` | `{ server_id, status, tools: [McpTool] }` | Resolves refs, persists the install + `mcp_client_env` rows, calls `connections::connect`. Refs are always consumed (removed from the in-memory map) regardless of outcome — on failure the agent must re-prompt via `mcp_setup_request_secret`. | --- @@ -68,10 +72,11 @@ Lifecycle of `SETUP_SECRETS`: - Process-local `OnceLock>>`. - Entries TTL out after, say, 15 min (defends against stranded secrets if the conversation is abandoned mid-flow). -- `mcp_setup_install_and_connect` consumes refs on success: pulls each - value, writes it to the `mcp_client_env` table (existing persistence, - already keyed by `server_id`), removes the ref. On failure refs are - left intact so the agent can retry without re-prompting the user. +- `mcp_setup_install_and_connect` consumes refs regardless of outcome: + pulls each value, writes it to the `mcp_client_env` table (existing + persistence, already keyed by `server_id`), and removes the ref from + the in-memory map. On failure the agent should re-prompt via + `mcp_setup_request_secret` to collect fresh refs for a retry. - On core shutdown the map is dropped — refs do not survive restart. `RefId` is a short random hex string. **No structure or hint of the @@ -96,17 +101,18 @@ values. ## Where the agent lives -Follow the existing sub-agent pattern (`src/openhuman/agent/harness/`): +Follows the existing sub-agent pattern (`src/openhuman/agent_registry/`): -- New archetype TOML at `app/src/lib/ai/agents/mcp_setup.toml` (loaded by - `AgentDefinitionRegistry::init_global`). -- Prompt + tool allowlist scoped tight: only the four `mcp_setup_*` tools - plus the standard `chat` / `ask_user` primitives. **No** general - filesystem, network, or shell tools — the agent shouldn't be able to - exfiltrate a leaked ref even if one shows up. -- Triggered by the main agent via `spawn_subagent("mcp_setup", { goal })` - or by an explicit UI affordance ("Add MCP server…" button that opens a - thread pinned to this archetype). +- Archetype TOML at `src/openhuman/agent_registry/agents/mcp_setup/agent.toml` + (loaded by the agent registry loader). +- Prompt + tool allowlist scoped tight: only the five `mcp_setup_*` tools + plus `ask_user_clarification`. **No** general filesystem, network, or shell + tools — the agent shouldn't be able to exfiltrate a leaked ref even if one + shows up. (`submit_secret` is intentionally NOT in the agent allowlist — the + UI calls it out-of-band via the socket bridge.) +- Triggered by the orchestrator's `setup_mcp_server` delegate (the `mcp_setup` + entry in `agent_registry/agents/orchestrator/agent.toml`), or directly from + chat when the user asks to add/install/set up an MCP server. --- @@ -127,7 +133,8 @@ Following the project's `Specify → Rust → JSON-RPC → UI → tests` flow: modal listening on a new socket event `mcp_setup_request_secret`), submit POSTs the value to a Tauri command that calls into core to register the ref. -4. **Archetype** + system prompt at `app/src/lib/ai/agents/mcp_setup.toml`. +4. **Archetype** + system prompt at + `src/openhuman/agent_registry/agents/mcp_setup/agent.toml`. 5. **Tests**: - Unit: ref lifecycle (mint → resolve → consume → TTL expiry). - Integration (`tests/mcp_registry_e2e.rs` style): full flow against diff --git a/docs/TEST-COVERAGE-MATRIX.md b/docs/TEST-COVERAGE-MATRIX.md index e910e2558..d667b76d0 100644 --- a/docs/TEST-COVERAGE-MATRIX.md +++ b/docs/TEST-COVERAGE-MATRIX.md @@ -418,6 +418,9 @@ Canonical mapping of every product feature to its test source(s). Drives gap-fil | 11.1.6 | SearXNG MCP search | RU | `src/openhuman/integrations/searxng.rs`, `src/openhuman/mcp_server/tools.rs`, `src/openhuman/tools/schemas.rs` | ✅ | Self-hosted search config, normalized results, MCP argument validation, and mocked HTTP execution | | 11.1.7 | Bundled prompt resources | RU | `src/openhuman/mcp_server/resources.rs`, `src/openhuman/mcp_server/protocol.rs` | ✅ | `resources/list` catalog + `resources/read` happy path, -32002 unknown URI, -32602 missing param, catalog-mirrors-BUILTINS parity test | | 11.1.8 | Resource templates list | RU | `src/openhuman/mcp_server/resources.rs`, `src/openhuman/mcp_server/protocol.rs` | ✅ | `resources/templates/list` returns `{resourceTemplates: []}` (static catalog), tolerates unknown/cursor params | +| 11.1.10 | MCP registry install→connect→tool_call | RI | `tests/json_rpc_e2e.rs` (`mcp_clients_install_connect_tool_call_happy_path`), `tests/mcp_registry_e2e.rs`, `src/openhuman/mcp_registry/setup_ops.rs` (this PR, #3039) | ✅ | HTTP-RPC happy path install→connect→tool_call→update_env against `test-mcp-stub`; transport-aware install (stdio + http_remote) via `build_install_transport` | +| 11.1.11 | MCP env reconfigure + registry creds | RI/VU | `tests/json_rpc_e2e.rs` (`mcp_clients_registry_settings_roundtrip`), `src/openhuman/mcp_registry/registries/mcp_official.rs`, `app/src/components/channels/mcp/InstalledServerDetail.test.tsx` (this PR, #3039) | ✅ | `update_env` persist+reconnect; `registry_settings` get/set with secrets write-only (config-first, env-fallback); reconfigure form validation | +| 11.1.12 | MCP UI surface + setup-agent client | VU | `app/src/components/channels/mcp/InstallDialog.test.tsx`, `app/src/services/api/mcpClientsApi.test.ts`, `app/src/services/api/mcpSetupApi.test.ts` (this PR, #3039) | ✅ | Skills `?tab=mcp` renders `McpServersTab` (not Coming Soon); auto-connect on install (best-effort); typed `mcpSetupApi` wrapper | diff --git a/gitbooks/developing/architecture/mcp-registry.md b/gitbooks/developing/architecture/mcp-registry.md index 105d31746..4d7beada1 100644 --- a/gitbooks/developing/architecture/mcp-registry.md +++ b/gitbooks/developing/architecture/mcp-registry.md @@ -1,14 +1,15 @@ --- description: >- The dynamic, user-facing side of MCP-client support — discover servers on - Smithery.ai, persist installs to SQLite, supervise local-spawn subprocess - lifecycle, surface their tools to agents via the unified tool registry. + Smithery and the official MCP registry, persist installs to SQLite, supervise + local-spawn subprocess lifecycle, surface their tools to agents via the + unified tool registry. icon: plug --- # MCP Registry (`src/openhuman/mcp_registry/`) -`src/openhuman/mcp_registry/` is the **dynamic, user-facing** half of OpenHuman's Model Context Protocol client support. It lets a user browse the Smithery.ai MCP registry, install a chosen server, persist that choice to SQLite, and (for servers launched as local subprocesses) supervise the subprocess lifecycle. Installed servers' tools are surfaced to agents via the unified tool registry (`crate::openhuman::tool_registry`). +`src/openhuman/mcp_registry/` is the **dynamic, user-facing** half of OpenHuman's Model Context Protocol client support. It lets a user browse the supported upstream registries (Smithery and the official modelcontextprotocol registry), install a chosen server, persist that choice to SQLite, and (for servers launched as local subprocesses or HTTP-remote endpoints) supervise the connection lifecycle. Installed servers' tools are surfaced to agents via the unified tool registry (`crate::openhuman::tool_registry`). > **Naming note**: the Rust module path is `mcp_registry`, but the RPC namespace and on-disk SQLite filename are still `mcp_clients` for backward compatibility with existing frontend code and stored user state. Grep both names when chasing call sites. @@ -16,7 +17,7 @@ This module is paired with `src/openhuman/mcp_client/` — the **transport libra ```text ┌───────────────────────────────────────────────┐ - Smithery.ai ──► registries/ + registry.rs (10-min SQLite cache)│ + Registries ───► registries/ + registry.rs (10-min SQLite cache)│ └────────────────────┬──────────────────────────┘ │ browse / install ▼ @@ -47,13 +48,16 @@ This module is paired with `src/openhuman/mcp_client/` — the **transport libra ## Server transport model -Today every `InstalledServer` is a **local subprocess** launched by `npx`, `uvx`, or a direct binary (see `types::CommandKind`). The connection is **stdio JSON-RPC**, owned by `connections.rs`. +An `InstalledServer` carries a `transport: Transport` discriminator (`types.rs`) with two variants: -HTTP-remote MCP servers (the majority of what Smithery actually lists) are **not yet modelled** as an `InstalledServer` variant. Adding a remote transport variant is planned follow-up work; after that, the registry will hold both kinds and `connections.rs` will dispatch by transport. +- **`Stdio`** — a local subprocess launched by `npx`, `uvx`, or a direct binary (see `types::CommandKind`), speaking **stdio JSON-RPC**. +- **`HttpRemote { url }`** — a hosted server (the majority of what Smithery lists), dialled over streamable HTTP by `mcp_client::McpHttpClient`. + +`connections.rs` dispatches on the transport. Both the manual install dialog (`mcp_clients_install`) and the setup-agent path (`mcp_setup_install_and_connect`) pick the best connection via `setup_ops::pick_connection` (published stdio → any stdio → published http_remote → any http_remote) and build the transport with `setup_ops::build_install_transport`, so the two paths behave identically. ## Boot-time spawn -`boot::spawn_installed_servers` is called from `bootstrap_core_runtime` so every local-spawn server is connected as soon as the core comes up. Errors are logged per-server and **never block boot** — a broken MCP install should not gate the desktop app starting. +`boot::spawn_installed_servers` is called from `bootstrap_core_runtime` so every installed server is connected as soon as the core comes up. Errors are logged per-server and **never block boot** — a broken MCP install should not gate the desktop app starting. The lifecycle log subscriber (`bus::init`) is registered alongside the other domain subscribers in `register_domain_subscribers` so those connect events are observed. ## Layout @@ -62,7 +66,7 @@ HTTP-remote MCP servers (the majority of what Smithery actually lists) are **not | `types.rs` | Data structures: `InstalledServer`, `McpTool`, `ConnStatus`, Smithery DTOs, etc. | | `store.rs` | SQLite persistence — `mcp_clients.db`, CRUD over `InstalledServer` rows. | | `registry.rs` | Smithery HTTP client with a 10-minute SQLite cache so re-browsing doesn't hammer the upstream registry. | -| `registries/` | Adapters for the upstream registries this code can browse (currently Smithery). | +| `registries/` | Adapters for the upstream registries this code can browse: Smithery (`smithery.rs`) + the official modelcontextprotocol registry (`mcp_official.rs`). Each reads optional auth config-first with an env-var fallback (`mcp_client.registry_auth`). | | `connections.rs` | Global in-process connection registry. Wraps `crate::openhuman::mcp_client::McpStdioClient` — there is no separate stdio client implementation here. | | `boot.rs` | Boot-time spawn (`spawn_installed_servers`) called from `bootstrap_core_runtime`. | | `setup.rs` / `setup_ops.rs` | "Setup agent" support — the small agent that walks a user through configuring a freshly installed server (env vars, secrets, first connect). | @@ -96,7 +100,7 @@ Everything else — `boot`, `bus`, `connections`, `store`, `setup`, `setup_ops` ## Called by - `bootstrap_core_runtime` (via `boot::spawn_installed_servers`). -- Frontend Skills UI — the (currently stubbed) MCP servers panel will dispatch through `ops.rs` over the `openhuman.mcp_clients_*` RPC namespace. +- Frontend Skills UI — the **MCP** tab at `/skills?tab=mcp` (`McpServersTab`) dispatches through `ops.rs` over the `openhuman.mcp_clients_*` RPC namespace: browse, install (auto-connects), connect/disconnect, status, tool_call, `update_env` (reconfigure + reconnect), and `registry_settings_get` / `registry_settings_set` (Smithery / official-registry credentials; secret values are write-only). The agent-native flow uses `openhuman.mcp_setup_*` via the `mcp_setup` sub-agent (orchestrator delegate `setup_mcp_server`). - The setup agent in `setup_ops.rs` — for first-connect onboarding. ## Tests diff --git a/src/core/jsonrpc.rs b/src/core/jsonrpc.rs index 97489f3d5..3a7060df6 100644 --- a/src/core/jsonrpc.rs +++ b/src/core/jsonrpc.rs @@ -1926,8 +1926,15 @@ fn register_domain_subscribers( // calls instead of importing `run_tool_call_loop` directly. crate::openhuman::agent::bus::register_agent_handlers(); + // MCP clients lifecycle subscriber: logs McpServer{Installed,Connected, + // Disconnected} + McpClientToolExecuted for observability. The boot-time + // spawn of installed servers (boot::spawn_installed_servers) runs later + // in bootstrap_core_runtime; this subscriber must be live before then so + // those connect events are observed (issue #3039 gap A1). + crate::openhuman::mcp_registry::bus::init(); + log::info!( - "[event_bus] domain subscribers registered (webhook, channel, health, conversation, composio, restart, proactive, agent, session_expired)" + "[event_bus] domain subscribers registered (webhook, channel, health, conversation, composio, restart, proactive, agent, session_expired, mcp_client)" ); }); } diff --git a/src/openhuman/about_app/catalog_data.rs b/src/openhuman/about_app/catalog_data.rs index 5b1c1b938..97fd6038a 100644 --- a/src/openhuman/about_app/catalog_data.rs +++ b/src/openhuman/about_app/catalog_data.rs @@ -1043,13 +1043,13 @@ pub(super) const CAPABILITIES: &[Capability] = &[ name: "Browse MCP Server Registry", domain: "channels", category: CapabilityCategory::Channels, - description: "Search and discover MCP servers from the Smithery.ai public registry.", - how_to: "Channels > MCP Servers > Browse Registry", + description: "Search and discover MCP servers from the Smithery.ai and official modelcontextprotocol registries.", + how_to: "Skills > MCP > Browse catalog", status: CapabilityStatus::Beta, privacy: Some(CapabilityPrivacy { leaves_device: true, data_kind: PrivacyDataKind::Metadata, - destinations: &["Smithery.ai registry API"], + destinations: &["Smithery.ai registry API", "modelcontextprotocol registry API"], }), }, Capability { @@ -1057,30 +1057,38 @@ pub(super) const CAPABILITIES: &[Capability] = &[ name: "Install MCP Servers", domain: "channels", category: CapabilityCategory::Channels, - description: "Install MCP servers locally. Required env vars are stored encrypted and never included in logs or responses.", - how_to: "Channels > MCP Servers > Install", + description: "Install MCP servers locally — both local stdio subprocesses and hosted HTTP-remote servers. Required env vars are stored encrypted and never included in logs or responses. Can also be done conversationally via the MCP setup assistant.", + how_to: "Skills > MCP > Browse catalog > Install, or ask the assistant to \"set up the MCP server\"", status: CapabilityStatus::Beta, privacy: LOCAL_CREDENTIALS, }, Capability { id: "channels.mcp_server_connect", - name: "Connect / Disconnect MCP Servers", + name: "Connect / Reconfigure MCP Servers", domain: "channels", category: CapabilityCategory::Channels, - description: "Spawn and manage MCP server subprocesses via the stdio JSON-RPC protocol.", - how_to: "Channels > MCP Servers > Connect", + description: "Spawn and manage MCP server connections (stdio subprocess or HTTP-remote). Reconfigure stored env vars and reconnect without uninstalling.", + how_to: "Skills > MCP > select a server > Connect / Reconfigure", status: CapabilityStatus::Beta, - privacy: None, + privacy: Some(CapabilityPrivacy { + leaves_device: true, + data_kind: PrivacyDataKind::Derived, + destinations: &["Configured MCP endpoint(s)"], + }), }, Capability { id: "channels.mcp_tool_call", name: "Invoke MCP Server Tools", domain: "channels", category: CapabilityCategory::Channels, - description: "Call tools exposed by connected MCP servers. Results are surfaced to the agent.", - how_to: "Human > ask the assistant to use a tool from a connected MCP server", + description: "Call tools exposed by connected MCP servers. Tools are surfaced to the agent and runnable from the tool playground.", + how_to: "Skills > MCP > select a connected server > Tools > Try, or ask the assistant in Chat", status: CapabilityStatus::Beta, - privacy: None, + privacy: Some(CapabilityPrivacy { + leaves_device: true, + data_kind: PrivacyDataKind::Derived, + destinations: &["Configured MCP endpoint(s)"], + }), }, Capability { id: "settings.configure_ai", diff --git a/src/openhuman/agent_registry/agents/orchestrator/agent.toml b/src/openhuman/agent_registry/agents/orchestrator/agent.toml index 8817c60e0..1680ad60a 100644 --- a/src/openhuman/agent_registry/agents/orchestrator/agent.toml +++ b/src/openhuman/agent_registry/agents/orchestrator/agent.toml @@ -66,6 +66,14 @@ subagents = [ # so there is exactly one canonical route — through this delegate — # which keeps the venue-specific approval-gate prompt in scope. "markets_agent", + # MCP setup specialist (#3039). Synthesised into a `delegate_setup_mcp_server` + # tool at agent-build time. Route any "install / add / set up / connect an MCP + # server" request here — the agent owns the full flow: search registries → + # request required secrets via a native dialog (raw values never enter the + # agent context) → test the connection → commit the install + connect. Without + # this entry the agent-native install path (issue #3039 gap B5) is unreachable + # from chat. + "mcp_setup", # NOTE: `summarizer` used to be listed here for the runtime-only # oversized-tool-result hook. That path is currently disabled # (`context.summarizer_payload_threshold_tokens = 0`) after recursive diff --git a/src/openhuman/config/schema/tools.rs b/src/openhuman/config/schema/tools.rs index 4f516bc5d..97cf42c47 100644 --- a/src/openhuman/config/schema/tools.rs +++ b/src/openhuman/config/schema/tools.rs @@ -388,6 +388,11 @@ pub struct McpClientConfig { /// Identity block sent during initialize. #[serde(default)] pub client_identity: McpClientIdentityConfig, + /// Optional auth/overrides for the MCP *registry* browse APIs (Smithery + + /// the official modelcontextprotocol/registry). Each value falls back to + /// the corresponding env var when unset (issue #3039 gap A6). + #[serde(default)] + pub registry_auth: McpRegistryAuthConfig, } impl Default for McpClientConfig { @@ -396,10 +401,35 @@ impl Default for McpClientConfig { enabled: defaults::default_true(), servers: Vec::new(), client_identity: McpClientIdentityConfig::default(), + registry_auth: McpRegistryAuthConfig::default(), } } } +/// Registry-browse auth + endpoint overrides. Lets a user who hits Smithery +/// rate limits (or needs an authenticated official-registry endpoint) supply +/// credentials from the desktop app instead of editing env vars. Each field is +/// config-first with an env-var fallback so existing CI/Docker deployments that +/// only set env vars keep working unchanged. +/// +/// Secrets are write-only over RPC: the getter reports whether each secret is +/// *set* (a boolean) and never echoes the value back. +#[derive(Debug, Clone, Default, Serialize, Deserialize, JsonSchema)] +#[serde(default)] +pub struct McpRegistryAuthConfig { + /// Smithery API key. Falls back to `SMITHERY_API_KEY`. + #[serde(default)] + pub smithery_api_key: Option, + /// Base URL override for the official registry. Falls back to + /// `MCP_OFFICIAL_REGISTRY_BASE` (non-secret). + #[serde(default)] + pub mcp_official_base: Option, + /// Bearer token for the official registry. Falls back to + /// `MCP_OFFICIAL_REGISTRY_TOKEN`. + #[serde(default)] + pub mcp_official_token: Option, +} + #[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] #[serde(default)] pub struct SeltzConfig { diff --git a/src/openhuman/mcp_registry/README.md b/src/openhuman/mcp_registry/README.md index 74f57e225..24f9d57a4 100644 --- a/src/openhuman/mcp_registry/README.md +++ b/src/openhuman/mcp_registry/README.md @@ -114,7 +114,9 @@ The in-process connection registry (`connections.rs`) is a `OnceLock any stdio > + // published http_remote > any http_remote — using the same picker the + // setup-agent path uses. Previously this path was stdio-only, so most + // Smithery listings (HTTP-remote) could not be installed from the manual + // install dialog at all; now both transports work identically + // (issue #3039 gap A2). + 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() + ) + })?; + let (transport, command_kind, command, args) = + super::setup_ops::build_install_transport(qualified_name.trim(), picked)?; // Derive required env keys from provided map + schema let env_keys: Vec = env.keys().cloned().collect(); @@ -148,12 +152,6 @@ pub async fn mcp_clients_install( .map(|d| d.as_millis() as i64) .unwrap_or(0); - // The legacy install path only ever picked stdio connections (see the - // `c.r#type == "stdio"` filter above), so legacy installs continue to - // be stdio-only. HTTP-remote installs go through the newer - // `setup_ops::mcp_setup_install_and_connect` setup-agent path, which - // picks the right transport based on what the registry actually - // exposes. let server = InstalledServer { server_id: server_id.clone(), qualified_name: qualified_name.trim().to_string(), @@ -167,7 +165,7 @@ pub async fn mcp_clients_install( config: config_value, installed_at: now_ms, last_connected_at: None, - transport: super::types::Transport::Stdio, + transport, }; store::insert_server(config, &server).map_err(|e| e.to_string())?; @@ -317,6 +315,169 @@ pub async fn mcp_clients_disconnect(server_id: String) -> Result, +) -> Result, String> { + if server_id.trim().is_empty() { + return Err("server_id must not be empty".to_string()); + } + let server_id = server_id.trim(); + + tracing::debug!( + "[mcp-client] update_env server_id={} env_keys={:?}", + server_id, + env.keys().collect::>() + ); + + // Persist first so the new values survive even if the reconnect fails. + store::set_env_values(config, server_id, &env).map_err(|e| e.to_string())?; + + // Drop any live session so the reconnect picks up the new env. + connections::disconnect(server_id).await; + let _ = publish_global(DomainEvent::McpServerDisconnected { + server_id: server_id.to_string(), + reason: Some("env reconfigured".to_string()), + }); + + let mut server = store::get_server(config, server_id).map_err(|e| e.to_string())?; + + // Keep the install record's `env_keys` list in sync with the values we just + // wrote — `set_env_values` replaces the value table wholesale, so the + // key-name list shown in the UI (and returned below) must track it too. + let mut new_keys: Vec = env.keys().cloned().collect(); + new_keys.sort(); + if server.env_keys != new_keys { + server.env_keys = new_keys; + store::update_server_env_keys(config, server_id, &server.env_keys) + .map_err(|e| e.to_string())?; + } + + match connections::connect(config, &server).await { + Ok(tools) => { + let tool_count = tools.len() as u32; + let _ = publish_global(DomainEvent::McpServerConnected { + server_id: server_id.to_string(), + tool_count, + }); + Ok(RpcOutcome::new( + json!({ + "server_id": server_id, + "status": "connected", + "env_keys": server.env_keys, + "tools": tools, + }), + vec![format!( + "update_env reconnected server_id={server_id} tools={tool_count}" + )], + )) + } + Err(err) => Ok(RpcOutcome::new( + json!({ + "server_id": server_id, + "status": "disconnected", + "env_keys": server.env_keys, + "error": err.to_string(), + }), + vec![format!( + "update_env persisted env for server_id={server_id} but reconnect failed: {err}" + )], + )), + } +} + +// ── registry settings ────────────────────────────────────────────────────── + +/// Build the non-secret registry-settings snapshot: booleans reporting whether +/// each credential is set (from config OR env) plus the user-configured +/// official-registry base URL. Secret *values* are never included. +fn registry_settings_snapshot(config: &Config) -> Value { + json!({ + "smithery_api_key_set": + super::registries::smithery::smithery_api_key(config).is_some(), + "mcp_official_token_set": + super::registries::mcp_official::auth_token(config).is_some(), + "mcp_official_base": + config + .mcp_client + .registry_auth + .mcp_official_base + .clone() + .filter(|s| !s.trim().is_empty()), + }) +} + +/// Report which registry credentials are configured. NEVER returns secret +/// values — only `*_set` booleans + the (non-secret) base URL override +/// (issue #3039 gap A6). +pub async fn mcp_clients_registry_settings_get( + config: &Config, +) -> Result, String> { + tracing::debug!("[mcp-client] registry_settings_get"); + Ok(RpcOutcome::new( + registry_settings_snapshot(config), + vec!["registry_settings_get".to_string()], + )) +} + +/// Persist registry credentials to config (issue #3039 gap A6). +/// +/// Per-field semantics: `None` leaves the stored value unchanged; `Some(s)` +/// sets it, where an empty/whitespace string clears the value (falling back to +/// the env var, if any). Secrets are write-only — the response is the same +/// non-secret snapshot as the getter, never the values just written. +pub async fn mcp_clients_registry_settings_set( + config: &mut Config, + smithery_api_key: Option, + mcp_official_base: Option, + mcp_official_token: Option, +) -> Result, String> { + fn apply(field: &mut Option, update: Option) { + if let Some(value) = update { + let trimmed = value.trim(); + *field = if trimmed.is_empty() { + None + } else { + Some(trimmed.to_string()) + }; + } + } + + tracing::debug!( + "[mcp-client] registry_settings_set smithery_key_present={} official_token_present={} official_base_present={}", + smithery_api_key.is_some(), + mcp_official_token.is_some(), + mcp_official_base.is_some() + ); + + let auth = &mut config.mcp_client.registry_auth; + apply(&mut auth.smithery_api_key, smithery_api_key); + apply(&mut auth.mcp_official_base, mcp_official_base); + apply(&mut auth.mcp_official_token, mcp_official_token); + + config.save().await.map_err(|e| e.to_string())?; + + Ok(RpcOutcome::new( + registry_settings_snapshot(config), + vec!["registry_settings_set saved".to_string()], + )) +} + // ── status ───────────────────────────────────────────────────────────────────── pub async fn mcp_clients_status(config: &Config) -> Result, String> { diff --git a/src/openhuman/mcp_registry/registries/mcp_official.rs b/src/openhuman/mcp_registry/registries/mcp_official.rs index 0b5d9b0f4..63e427b12 100644 --- a/src/openhuman/mcp_registry/registries/mcp_official.rs +++ b/src/openhuman/mcp_registry/registries/mcp_official.rs @@ -157,7 +157,7 @@ impl Registry for McpOfficialRegistry { } }; - let body = fetch_page(q, limit, cursor_for_request.as_deref()).await?; + let body = fetch_page(config, q, limit, cursor_for_request.as_deref()).await?; let parsed: OfficialListResponse = serde_json::from_str(&body) .with_context(|| format!("Failed to parse MCP official response: {body}"))?; let next_cursor = parsed.next_cursor().map(str::to_string); @@ -187,11 +187,14 @@ impl Registry for McpOfficialRegistry { let client = http_client()?; let url = format!( "{}/v0/servers/{}", - base_url(), + base_url(config), urlencoding_encode(qualified_name) ); tracing::debug!("[mcp-official] get fetching {url}"); - let req = apply_auth(client.get(&url).header("Accept", "application/json")); + let req = apply_auth( + config, + client.get(&url).header("Accept", "application/json"), + ); let resp = req.send().await.context("MCP official get failed")?; let status = resp.status(); @@ -214,7 +217,7 @@ impl Registry for McpOfficialRegistry { /// Fetch one page from the registry, optionally with a cursor. Returns the /// raw response body so callers can both parse it and write it to the SQLite /// response cache. -async fn fetch_page(q: &str, limit: u32, cursor: Option<&str>) -> Result { +async fn fetch_page(config: &Config, q: &str, limit: u32, cursor: Option<&str>) -> Result { // `q` is user-typed search input — log presence + length only so the // diagnostic doesn't leak query text into log aggregators. tracing::debug!( @@ -225,7 +228,7 @@ async fn fetch_page(q: &str, limit: u32, cursor: Option<&str>) -> Result ); let client = http_client()?; - let url = format!("{}/v0/servers", base_url()); + let url = format!("{}/v0/servers", base_url(config)); let mut req = client.get(&url).header("Accept", "application/json"); if !q.is_empty() { req = req.query(&[("search", q)]); @@ -234,7 +237,7 @@ async fn fetch_page(q: &str, limit: u32, cursor: Option<&str>) -> Result if let Some(c) = cursor { req = req.query(&[("cursor", c)]); } - req = apply_auth(req); + req = apply_auth(config, req); let resp = req.send().await.context("MCP official search failed")?; let status = resp.status(); @@ -302,7 +305,7 @@ async fn walk_cursor_for_page( body } _ => { - let body = fetch_page(q, limit, cursor.as_deref()).await?; + let body = fetch_page(config, q, limit, cursor.as_deref()).await?; let _ = store::set_cached(config, &cache_key, &body); net_fetches += 1; body @@ -359,21 +362,43 @@ fn http_client() -> Result { .context("Failed to build MCP official HTTP client") } -fn base_url() -> String { - std::env::var("MCP_OFFICIAL_REGISTRY_BASE") - .ok() - .filter(|s| !s.is_empty()) +/// Effective official-registry base URL: config-first +/// (`mcp_client.registry_auth.mcp_official_base`), then the +/// `MCP_OFFICIAL_REGISTRY_BASE` env var, then the hard-coded default +/// (issue #3039 gap A6). +fn base_url(config: &Config) -> String { + config + .mcp_client + .registry_auth + .mcp_official_base + .clone() + .filter(|s| !s.trim().is_empty()) + .or_else(|| { + std::env::var("MCP_OFFICIAL_REGISTRY_BASE") + .ok() + .filter(|s| !s.is_empty()) + }) .unwrap_or_else(|| DEFAULT_BASE.to_string()) } -fn auth_token() -> Option { - std::env::var("MCP_OFFICIAL_REGISTRY_TOKEN") - .ok() - .filter(|s| !s.is_empty()) +/// Effective official-registry bearer token: config-first, then the +/// `MCP_OFFICIAL_REGISTRY_TOKEN` env var (issue #3039 gap A6). +pub(crate) fn auth_token(config: &Config) -> Option { + config + .mcp_client + .registry_auth + .mcp_official_token + .clone() + .filter(|s| !s.trim().is_empty()) + .or_else(|| { + std::env::var("MCP_OFFICIAL_REGISTRY_TOKEN") + .ok() + .filter(|s| !s.is_empty()) + }) } -fn apply_auth(builder: reqwest::RequestBuilder) -> reqwest::RequestBuilder { - if let Some(token) = auth_token() { +fn apply_auth(config: &Config, builder: reqwest::RequestBuilder) -> reqwest::RequestBuilder { + if let Some(token) = auth_token(config) { builder.bearer_auth(token) } else { builder @@ -716,4 +741,27 @@ mod tests { "renamed `server` field must surface as parse error" ); } + + /// A config-set base URL overrides both the env var and the default + /// (issue #3039 gap A6: config-first, env-fallback). + #[test] + fn base_url_prefers_config_override() { + let mut config = crate::openhuman::config::Config::default(); + config.mcp_client.registry_auth.mcp_official_base = + Some("https://registry.example.test".to_string()); + assert_eq!(base_url(&config), "https://registry.example.test"); + + // A blank config value is ignored (falls back to env / default) — + // asserted env-independently so an ambient env override can't flake it. + config.mcp_client.registry_auth.mcp_official_base = Some(" ".to_string()); + assert_ne!(base_url(&config), " "); + } + + /// A config-set token is returned without touching the env var. + #[test] + fn auth_token_prefers_config_override() { + let mut config = crate::openhuman::config::Config::default(); + config.mcp_client.registry_auth.mcp_official_token = Some("tok-config".to_string()); + assert_eq!(auth_token(&config).as_deref(), Some("tok-config")); + } } diff --git a/src/openhuman/mcp_registry/registries/smithery.rs b/src/openhuman/mcp_registry/registries/smithery.rs index 389ae8746..5da97904c 100644 --- a/src/openhuman/mcp_registry/registries/smithery.rs +++ b/src/openhuman/mcp_registry/registries/smithery.rs @@ -66,7 +66,7 @@ impl Registry for SmitheryRegistry { ("pageSize", &page_size.to_string()), ]) .header("Accept", "application/json"); - req = apply_auth(req); + req = apply_auth(config, req); let resp = req.send().await.context("Smithery search request failed")?; let status = resp.status(); @@ -116,7 +116,10 @@ impl Registry for SmitheryRegistry { "{SMITHERY_BASE}/servers/{}", urlencoding_encode(qualified_name) ); - let req = apply_auth(client.get(&url).header("Accept", "application/json")); + let req = apply_auth( + config, + client.get(&url).header("Accept", "application/json"), + ); let resp = req.send().await.context("Smithery get request failed")?; let status = resp.status(); @@ -150,14 +153,25 @@ fn http_client() -> Result { .context("Failed to build Smithery HTTP client") } -fn smithery_api_key() -> Option { - std::env::var("SMITHERY_API_KEY") - .ok() - .filter(|s| !s.is_empty()) +/// Effective Smithery API key: config-first (`mcp_client.registry_auth`), then +/// the `SMITHERY_API_KEY` env var (issue #3039 gap A6). Empty strings are +/// treated as unset so a blank config/env value doesn't send `Bearer `. +pub(crate) fn smithery_api_key(config: &Config) -> Option { + config + .mcp_client + .registry_auth + .smithery_api_key + .clone() + .filter(|s| !s.trim().is_empty()) + .or_else(|| { + std::env::var("SMITHERY_API_KEY") + .ok() + .filter(|s| !s.is_empty()) + }) } -fn apply_auth(builder: reqwest::RequestBuilder) -> reqwest::RequestBuilder { - if let Some(key) = smithery_api_key() { +fn apply_auth(config: &Config, builder: reqwest::RequestBuilder) -> reqwest::RequestBuilder { + if let Some(key) = smithery_api_key(config) { builder.bearer_auth(key) } else { builder diff --git a/src/openhuman/mcp_registry/schemas.rs b/src/openhuman/mcp_registry/schemas.rs index ac802bed9..df4b95136 100644 --- a/src/openhuman/mcp_registry/schemas.rs +++ b/src/openhuman/mcp_registry/schemas.rs @@ -20,12 +20,15 @@ pub fn all_controller_schemas() -> Vec { schemas("registry_get"), schemas("installed_list"), schemas("install"), + schemas("update_env"), schemas("uninstall"), schemas("connect"), schemas("disconnect"), schemas("status"), schemas("tool_call"), schemas("config_assist"), + schemas("registry_settings_get"), + schemas("registry_settings_set"), // Setup-agent surface (mcp_setup namespace, lives in setup_ops.rs). setup_schemas("search"), setup_schemas("get"), @@ -54,6 +57,10 @@ pub fn all_registered_controllers() -> Vec { schema: schemas("install"), handler: handle_install, }, + RegisteredController { + schema: schemas("update_env"), + handler: handle_update_env, + }, RegisteredController { schema: schemas("uninstall"), handler: handle_uninstall, @@ -78,6 +85,14 @@ pub fn all_registered_controllers() -> Vec { schema: schemas("config_assist"), handler: handle_config_assist, }, + RegisteredController { + schema: schemas("registry_settings_get"), + handler: handle_registry_settings_get, + }, + RegisteredController { + schema: schemas("registry_settings_set"), + handler: handle_registry_settings_set, + }, RegisteredController { schema: setup_schemas("search"), handler: handle_setup_search, @@ -216,6 +231,54 @@ pub fn schemas(function: &str) -> ControllerSchema { }], }, + "update_env" => ControllerSchema { + namespace: "mcp_clients", + function: "update_env", + description: "Replace the stored env values for an installed server and reconnect so the new credentials take effect (reconfigure / rotate keys without reinstalling).", + inputs: vec![ + FieldSchema { + name: "server_id", + ty: TypeSchema::String, + comment: "UUID of the installed server to reconfigure.", + required: true, + }, + FieldSchema { + name: "env", + ty: TypeSchema::Map(Box::new(TypeSchema::String)), + comment: "Replacement environment variable values. Stored encrypted and never returned.", + required: true, + }, + ], + outputs: vec![ + FieldSchema { + name: "server_id", + ty: TypeSchema::String, + comment: "The reconfigured server id.", + required: true, + }, + FieldSchema { + name: "status", + ty: TypeSchema::Enum { + variants: vec!["connected", "disconnected"], + }, + comment: "`connected` if the reconnect succeeded, `disconnected` if env was saved but reconnect failed.", + required: true, + }, + FieldSchema { + name: "env_keys", + ty: TypeSchema::Array(Box::new(TypeSchema::String)), + comment: "Env key names after the update (values omitted).", + required: true, + }, + FieldSchema { + name: "tools", + ty: TypeSchema::Array(Box::new(TypeSchema::Ref("McpTool"))), + comment: "Tools exposed after reconnect (present only when status=connected).", + required: false, + }, + ], + }, + "uninstall" => ControllerSchema { namespace: "mcp_clients", function: "uninstall", @@ -399,6 +462,79 @@ pub fn schemas(function: &str) -> ControllerSchema { ], }, + "registry_settings_get" => ControllerSchema { + namespace: "mcp_clients", + function: "registry_settings_get", + description: "Report which registry credentials are configured (Smithery key, official-registry base/token). Never returns secret values — only `*_set` booleans plus the non-secret base URL override.", + inputs: vec![], + outputs: vec![ + FieldSchema { + name: "smithery_api_key_set", + ty: TypeSchema::Bool, + comment: "True when a Smithery API key is set (config or env).", + required: true, + }, + FieldSchema { + name: "mcp_official_token_set", + ty: TypeSchema::Bool, + comment: "True when an official-registry bearer token is set (config or env).", + required: true, + }, + FieldSchema { + name: "mcp_official_base", + ty: TypeSchema::Option(Box::new(TypeSchema::String)), + comment: "User-configured official-registry base URL override (non-secret).", + required: false, + }, + ], + }, + + "registry_settings_set" => ControllerSchema { + namespace: "mcp_clients", + function: "registry_settings_set", + description: "Persist registry credentials. Per field: omit to leave unchanged, empty string to clear, value to set. Secrets are write-only; the response is the same non-secret snapshot as registry_settings_get.", + inputs: vec![ + FieldSchema { + name: "smithery_api_key", + ty: TypeSchema::Option(Box::new(TypeSchema::String)), + comment: "New Smithery API key (empty string clears).", + required: false, + }, + FieldSchema { + name: "mcp_official_base", + ty: TypeSchema::Option(Box::new(TypeSchema::String)), + comment: "New official-registry base URL override (empty string clears).", + required: false, + }, + FieldSchema { + name: "mcp_official_token", + ty: TypeSchema::Option(Box::new(TypeSchema::String)), + comment: "New official-registry bearer token (empty string clears).", + required: false, + }, + ], + outputs: vec![ + FieldSchema { + name: "smithery_api_key_set", + ty: TypeSchema::Bool, + comment: "True when a Smithery API key is set after the update.", + required: true, + }, + FieldSchema { + name: "mcp_official_token_set", + ty: TypeSchema::Bool, + comment: "True when an official-registry token is set after the update.", + required: true, + }, + FieldSchema { + name: "mcp_official_base", + ty: TypeSchema::Option(Box::new(TypeSchema::String)), + comment: "Official-registry base URL override after the update.", + required: false, + }, + ], + }, + // Handled by setup_schemas() — surface a clearer error rather than // falling through to the generic unknown sink. "setup_search" @@ -482,6 +618,18 @@ fn handle_install(params: Map) -> ControllerFuture { }) } +fn handle_update_env(params: Map) -> ControllerFuture { + Box::pin(async move { + let config = config_rpc::load_config_with_timeout().await?; + let server_id = read_required::(¶ms, "server_id")?; + let env = read_required::>(¶ms, "env")?; + to_json( + crate::openhuman::mcp_registry::ops::mcp_clients_update_env(&config, server_id, env) + .await?, + ) + }) +} + fn handle_uninstall(params: Map) -> ControllerFuture { Box::pin(async move { let config = config_rpc::load_config_with_timeout().await?; @@ -552,6 +700,34 @@ fn handle_config_assist(params: Map) -> ControllerFuture { }) } +fn handle_registry_settings_get(params: Map) -> ControllerFuture { + Box::pin(async move { + let _ = params; + let config = config_rpc::load_config_with_timeout().await?; + to_json( + crate::openhuman::mcp_registry::ops::mcp_clients_registry_settings_get(&config).await?, + ) + }) +} + +fn handle_registry_settings_set(params: Map) -> ControllerFuture { + Box::pin(async move { + let smithery_api_key = read_optional::(¶ms, "smithery_api_key")?; + let mcp_official_base = read_optional::(¶ms, "mcp_official_base")?; + let mcp_official_token = read_optional::(¶ms, "mcp_official_token")?; + let mut config = config_rpc::load_config_with_timeout().await?; + to_json( + crate::openhuman::mcp_registry::ops::mcp_clients_registry_settings_set( + &mut config, + smithery_api_key, + mcp_official_base, + mcp_official_token, + ) + .await?, + ) + }) +} + // ── mcp_setup_* schemas + handlers ──────────────────────────────────────────── /// All setup-agent schemas under the `mcp_setup` RPC namespace. Kept in a diff --git a/src/openhuman/mcp_registry/schemas_tests.rs b/src/openhuman/mcp_registry/schemas_tests.rs index 2fb8b693e..a3c77fa78 100644 --- a/src/openhuman/mcp_registry/schemas_tests.rs +++ b/src/openhuman/mcp_registry/schemas_tests.rs @@ -67,8 +67,9 @@ fn schemas_unknown_function_returns_placeholder() { #[test] fn all_controller_schemas_covers_expected_methods() { let schemas = all_controller_schemas(); - // 10 mcp_clients + 6 mcp_setup - assert_eq!(schemas.len(), 16); + // 13 mcp_clients (incl. update_env + registry_settings_get/set, #3039) + // + 6 mcp_setup. + assert_eq!(schemas.len(), 19); let mcp_clients_count = schemas .iter() .filter(|s| s.namespace == "mcp_clients") @@ -77,14 +78,19 @@ fn all_controller_schemas_covers_expected_methods() { .iter() .filter(|s| s.namespace == "mcp_setup") .count(); - assert_eq!(mcp_clients_count, 10); + assert_eq!(mcp_clients_count, 13); assert_eq!(mcp_setup_count, 6); + // The #3039 additions are present. + let functions: Vec<_> = schemas.iter().map(|s| s.function).collect(); + assert!(functions.contains(&"update_env")); + assert!(functions.contains(&"registry_settings_get")); + assert!(functions.contains(&"registry_settings_set")); } #[test] fn all_registered_controllers_has_handler_per_schema() { let controllers = all_registered_controllers(); - assert_eq!(controllers.len(), 16); + assert_eq!(controllers.len(), 19); } #[test] diff --git a/src/openhuman/mcp_registry/setup_ops.rs b/src/openhuman/mcp_registry/setup_ops.rs index 29aff2264..c51377d95 100644 --- a/src/openhuman/mcp_registry/setup_ops.rs +++ b/src/openhuman/mcp_registry/setup_ops.rs @@ -271,26 +271,7 @@ pub async fn mcp_setup_install_and_connect( // Branch on the picked transport. Stdio installs still populate // command/args (current behavior). HTTP-remote installs leave them // empty and stash the deployment URL in `transport`. - let (transport, command_kind, command, args) = match picked.transport_kind() { - "http_remote" => { - let url = picked.deployment_url.clone().unwrap_or_default(); - if url.is_empty() { - return Err(format!( - "server `{q}` http_remote connection has empty deployment_url" - )); - } - ( - Transport::HttpRemote { url }, - CommandKind::Node, // unused for HTTP, but a sensible default - String::new(), - Vec::new(), - ) - } - _ => { - let (kind, command, args) = resolve_command(q, Some(picked)); - (Transport::Stdio, kind, command, args) - } - }; + let (transport, command_kind, command, args) = build_install_transport(q, picked)?; // Consume refs only after `registry_get` succeeds — that way a // misconfigured server name doesn't burn the user's collected @@ -441,10 +422,43 @@ pub(super) fn pick_connection(connections: &[SmitheryConnection]) -> Option<&Smi .find(|c| c.transport_kind() == "http_remote") } +/// Resolve the persisted [`Transport`] + launch command for an install from the +/// connection the picker selected. Stdio installs populate `command`/`args`; +/// HTTP-remote installs leave them empty and stash the deployment URL in the +/// `Transport`. Shared by the setup-agent path +/// ([`mcp_setup_install_and_connect`]) and the manual install dialog path +/// ([`super::ops::mcp_clients_install`]) so both transports behave identically +/// (issue #3039 gap A2). +pub(super) fn build_install_transport( + qualified_name: &str, + picked: &SmitheryConnection, +) -> Result<(Transport, CommandKind, String, Vec), String> { + match picked.transport_kind() { + "http_remote" => { + let url = picked.deployment_url.clone().unwrap_or_default(); + if url.is_empty() { + return Err(format!( + "server `{qualified_name}` http_remote connection has empty deployment_url" + )); + } + Ok(( + Transport::HttpRemote { url }, + CommandKind::Node, // unused for HTTP, but a sensible default + String::new(), + Vec::new(), + )) + } + _ => { + let (kind, command, args) = resolve_command(qualified_name, Some(picked)); + Ok((Transport::Stdio, kind, command, args)) + } + } +} + /// Normalise a [`SmitheryConnection::r#type`] string into the same vocabulary /// the persisted [`Transport`] enum uses. The registry side uses `"http"` /// in its DTOs; we route those into the `"http_remote"` install path. -trait ConnectionKind { +pub(super) trait ConnectionKind { fn transport_kind(&self) -> &str; } @@ -527,4 +541,45 @@ mod tests { let conns = vec![conn("websocket-future", true, None)]; assert!(pick_connection(&conns).is_none()); } + + /// `build_install_transport` turns a picked stdio connection into a + /// `Transport::Stdio` with a resolved launch command (npx default when the + /// example_config has no command). Shared by both install paths (gap A2). + #[test] + fn build_install_transport_stdio_resolves_command() { + let picked = conn("stdio", true, None); + let (transport, _kind, command, args) = + build_install_transport("@scope/echo", &picked).expect("stdio install resolves"); + assert_eq!(transport, Transport::Stdio); + assert_eq!(command, "npx"); + assert_eq!(args, vec!["-y".to_string(), "@scope/echo".to_string()]); + } + + /// A picked http_remote connection becomes `Transport::HttpRemote { url }` + /// with empty command/args — the manual install dialog can now install + /// Smithery's HTTP-only listings (gap A2). + #[test] + fn build_install_transport_http_remote_uses_deployment_url() { + let picked = conn("http", true, Some("https://x.io/mcp")); + let (transport, _kind, command, args) = + build_install_transport("io.x/remote", &picked).expect("http install resolves"); + assert_eq!( + transport, + Transport::HttpRemote { + url: "https://x.io/mcp".to_string() + } + ); + assert!(command.is_empty()); + assert!(args.is_empty()); + } + + /// An http_remote connection with no deployment URL is a hard error rather + /// than installing an undialable server. + #[test] + fn build_install_transport_http_remote_requires_url() { + let picked = conn("http", true, None); + let err = build_install_transport("io.x/remote", &picked) + .expect_err("missing deployment_url must error"); + assert!(err.contains("deployment_url"), "got: {err}"); + } } diff --git a/src/openhuman/mcp_registry/store.rs b/src/openhuman/mcp_registry/store.rs index 2b5948558..6f06ddcbf 100644 --- a/src/openhuman/mcp_registry/store.rs +++ b/src/openhuman/mcp_registry/store.rs @@ -163,6 +163,23 @@ pub fn insert_server_conn(conn: &Connection, server: &InstalledServer) -> Result Ok(()) } +/// 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, +/// while the key-name list shown in list/status responses lives on the server +/// row. A plain `insert_server` would conflict on the primary key. +pub fn update_server_env_keys(config: &Config, server_id: &str, env_keys: &[String]) -> Result<()> { + let env_keys_json = serde_json::to_string(env_keys)?; + with_connection(config, |conn| { + conn.execute( + "UPDATE mcp_servers SET env_keys_json = ?2 WHERE server_id = ?1", + params![server_id, env_keys_json], + ) + .context("Failed to update mcp_server env_keys")?; + Ok(()) + }) +} + pub fn list_servers(config: &Config) -> Result> { with_connection(config, |conn| list_servers_conn(conn)) } diff --git a/tests/json_rpc_e2e.rs b/tests/json_rpc_e2e.rs index 26e6e3c76..de38d5e02 100644 --- a/tests/json_rpc_e2e.rs +++ b/tests/json_rpc_e2e.rs @@ -8317,6 +8317,319 @@ async fn mcp_clients_lifecycle() { rpc_join.abort(); } +/// MCP clients **happy path** over real JSON-RPC: install → connect → tool_call +/// → update_env (reconnect) → disconnect against a real stdio MCP subprocess +/// (the `test-mcp-stub` binary), with the registry lookup served hermetically +/// from the SQLite detail cache (issue #3039 acceptance: "JSON-RPC E2E — +/// happy-path install/connect/tool_call against stub server over HTTP RPC"). +/// +/// 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. +#[tokio::test] +async fn mcp_clients_install_connect_tool_call_happy_path() { + 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 registry detail cache so `registry_get` resolves offline to a + // stdio connection whose command is the hermetic stub binary. The config we + // load here resolves the same workspace dir the RPC handlers use, so the + // cache row lands in the DB the install path reads. + let stub_path = env!("CARGO_BIN_EXE_test-mcp-stub"); + let qualified_name = "@openhuman-test/echo"; + let detail = serde_json::json!({ + "qualifiedName": qualified_name, + "displayName": "Test Echo", + "description": "Stub MCP server for the json_rpc_e2e happy path.", + "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 resolves the stub command from the seeded detail ────────── + let install = post_json_rpc( + &rpc_base, + 9920, + "openhuman.mcp_clients_install", + json!({ "qualified_name": qualified_name, "env": {} }), + ) + .await; + let install_result = assert_no_jsonrpc_error(&install, "mcp_clients_install (happy path)"); + let install_body = install_result.get("result").unwrap_or(install_result); + let server_id = install_body + .get("server") + .and_then(|s| s.get("server_id")) + .and_then(Value::as_str) + .expect("install returns a server.server_id") + .to_string(); + + // ── 2. connect spawns the stub and lists its one `echo` tool ───────────── + let connect = post_json_rpc( + &rpc_base, + 9921, + "openhuman.mcp_clients_connect", + json!({ "server_id": server_id }), + ) + .await; + let connect_result = assert_no_jsonrpc_error(&connect, "mcp_clients_connect (happy path)"); + let connect_body = connect_result.get("result").unwrap_or(connect_result); + assert_eq!( + connect_body.get("status").and_then(Value::as_str), + Some("connected"), + "connect should report connected: {connect_body}" + ); + let tools = connect_body + .get("tools") + .and_then(Value::as_array) + .expect("connect returns a tools array"); + assert!( + tools + .iter() + .any(|t| t.get("name").and_then(Value::as_str) == Some("echo")), + "stub should advertise the echo tool: {tools:?}" + ); + + // ── 3. tool_call echoes the input back, is_error=false ─────────────────── + let tool_call = post_json_rpc( + &rpc_base, + 9922, + "openhuman.mcp_clients_tool_call", + json!({ + "server_id": server_id, + "tool_name": "echo", + "arguments": { "message": "hello over rpc" } + }), + ) + .await; + let tc_result = assert_no_jsonrpc_error(&tool_call, "mcp_clients_tool_call (happy path)"); + let tc_body = tc_result.get("result").unwrap_or(tc_result); + assert_eq!( + tc_body.get("is_error"), + Some(&json!(false)), + "echo tool_call should not be an error: {tc_body}" + ); + assert!( + tc_body.to_string().contains("hello over rpc"), + "echo tool_call should round-trip the input payload: {tc_body}" + ); + + // ── 4. update_env reconfigures + reconnects (no uninstall/reinstall) ───── + let update_env = post_json_rpc( + &rpc_base, + 9923, + "openhuman.mcp_clients_update_env", + json!({ "server_id": server_id, "env": { "EXAMPLE_TOKEN": "rotated" } }), + ) + .await; + let ue_result = assert_no_jsonrpc_error(&update_env, "mcp_clients_update_env (happy path)"); + let ue_body = ue_result.get("result").unwrap_or(ue_result); + assert_eq!( + ue_body.get("status").and_then(Value::as_str), + Some("connected"), + "update_env should reconnect: {ue_body}" + ); + let env_keys = ue_body + .get("env_keys") + .and_then(Value::as_array) + .expect("update_env returns env_keys"); + assert!( + env_keys.iter().any(|k| k.as_str() == Some("EXAMPLE_TOKEN")), + "update_env should persist the new env key: {env_keys:?}" + ); + + // Verify the reconnected session is still functional: call echo again. + let tool_call2 = post_json_rpc( + &rpc_base, + 9925, + "openhuman.mcp_clients_tool_call", + json!({ + "server_id": server_id, + "tool_name": "echo", + "arguments": { "message": "hello after reconfigure" } + }), + ) + .await; + let tc2_result = + assert_no_jsonrpc_error(&tool_call2, "mcp_clients_tool_call (after update_env)"); + let tc2_body = tc2_result.get("result").unwrap_or(tc2_result); + assert_eq!( + tc2_body.get("is_error"), + Some(&json!(false)), + "echo tool_call after reconfigure should not be an error: {tc2_body}" + ); + assert!( + tc2_body.to_string().contains("hello after reconfigure"), + "echo tool_call after reconfigure should round-trip the input payload: {tc2_body}" + ); + + // ── 5. disconnect cleans up the subprocess ─────────────────────────────── + let disconnect = post_json_rpc( + &rpc_base, + 9924, + "openhuman.mcp_clients_disconnect", + json!({ "server_id": server_id }), + ) + .await; + let disc_result = assert_no_jsonrpc_error(&disconnect, "mcp_clients_disconnect (happy path)"); + let disc_body = disc_result.get("result").unwrap_or(disc_result); + assert_eq!( + disc_body.get("status").and_then(Value::as_str), + Some("disconnected"), + "disconnect should report disconnected: {disc_body}" + ); + + 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). +#[tokio::test] +async fn mcp_clients_registry_settings_roundtrip() { + 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 _smithery_guard = EnvVarGuard::unset("SMITHERY_API_KEY"); + let _official_token_guard = EnvVarGuard::unset("MCP_OFFICIAL_REGISTRY_TOKEN"); + + 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); + write_min_config(&openhuman_home.join("users").join("local"), &mock_origin); + + 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; + + // Initially nothing is set. + let get1 = post_json_rpc( + &rpc_base, + 9930, + "openhuman.mcp_clients_registry_settings_get", + json!({}), + ) + .await; + let get1_result = assert_no_jsonrpc_error(&get1, "registry_settings_get (initial)"); + let get1_body = get1_result.get("result").unwrap_or(get1_result); + assert_eq!(get1_body.get("smithery_api_key_set"), Some(&json!(false))); + assert_eq!(get1_body.get("mcp_official_token_set"), Some(&json!(false))); + + // Set a key + base override. + let set1 = post_json_rpc( + &rpc_base, + 9931, + "openhuman.mcp_clients_registry_settings_set", + json!({ + "smithery_api_key": "sk-secret-value", + "mcp_official_base": "https://registry.example.test" + }), + ) + .await; + let set1_result = assert_no_jsonrpc_error(&set1, "registry_settings_set (set)"); + let set1_body = set1_result.get("result").unwrap_or(set1_result); + assert_eq!(set1_body.get("smithery_api_key_set"), Some(&json!(true))); + assert_eq!( + set1_body.get("mcp_official_base").and_then(Value::as_str), + Some("https://registry.example.test"), + ); + // The secret value is NEVER echoed back anywhere in the response. + assert!( + !set1.to_string().contains("sk-secret-value"), + "registry_settings_set must not echo the secret value" + ); + + // Read-after-write: verify getter reflects the persisted state. + let get2 = post_json_rpc( + &rpc_base, + 9933, + "openhuman.mcp_clients_registry_settings_get", + json!({}), + ) + .await; + let get2_result = assert_no_jsonrpc_error(&get2, "registry_settings_get (after set)"); + let get2_body = get2_result.get("result").unwrap_or(get2_result); + assert_eq!(get2_body.get("smithery_api_key_set"), Some(&json!(true))); + assert_eq!( + get2_body.get("mcp_official_base").and_then(Value::as_str), + Some("https://registry.example.test"), + ); + // Getter must never return the raw secret. + assert!( + !get2.to_string().contains("sk-secret-value"), + "registry_settings_get must not return the secret value" + ); + + // Clearing with an empty string flips the boolean back to false. + let set2 = post_json_rpc( + &rpc_base, + 9932, + "openhuman.mcp_clients_registry_settings_set", + json!({ "smithery_api_key": "" }), + ) + .await; + let set2_result = assert_no_jsonrpc_error(&set2, "registry_settings_set (clear)"); + let set2_body = set2_result.get("result").unwrap_or(set2_result); + assert_eq!(set2_body.get("smithery_api_key_set"), Some(&json!(false))); + // The base override persists across the clear of an unrelated field. + assert_eq!( + set2_body.get("mcp_official_base").and_then(Value::as_str), + Some("https://registry.example.test"), + ); + + // Read-after-clear: verify getter reflects the cleared state. + let get3 = post_json_rpc( + &rpc_base, + 9934, + "openhuman.mcp_clients_registry_settings_get", + json!({}), + ) + .await; + let get3_result = assert_no_jsonrpc_error(&get3, "registry_settings_get (after clear)"); + let get3_body = get3_result.get("result").unwrap_or(get3_result); + assert_eq!(get3_body.get("smithery_api_key_set"), Some(&json!(false))); + // Base override should persist even after clearing the API key. + assert_eq!( + get3_body.get("mcp_official_base").and_then(Value::as_str), + Some("https://registry.example.test"), + ); + + mock_join.abort(); + rpc_join.abort(); +} + /// Proxy config corruption recovery (PR #1563 guard). /// /// Verifies that when the config.toml on disk is corrupted *after* the core