diff --git a/e2e/mcp.spec.ts b/e2e/mcp.spec.ts new file mode 100644 index 00000000..713237f3 --- /dev/null +++ b/e2e/mcp.spec.ts @@ -0,0 +1,162 @@ +import { test, expect } from '@playwright/test'; +import type { Locator, Page } from '@playwright/test'; +import { LIVE, LIVE_SKIP_REASON } from './live'; + +/** + * MCP server journeys (WS12) from the MCP audit. They need a real LibreChat + * backend whose librechat.yaml defines the remote server E2E_MCP_REMOTE + * (with an Authorization header) and the stdio server E2E_MCP_STDIO: run + * with E2E_LIVE=1 and E2E_BASE_URL. + */ +const REMOTE = process.env.E2E_MCP_REMOTE ?? 'github'; +const PLAIN = process.env.E2E_MCP_PLAIN ?? 'cloudflare'; +const STDIO = process.env.E2E_MCP_STDIO ?? 'everything'; + +const unsavedBar = (page: Page) => page.getByRole('region', { name: 'Unsaved changes' }); + +async function openMcp(page: Page) { + await page.goto('/configuration?tab=mcp'); + await page.waitForLoadState('networkidle'); +} + +/** The expanded card of the server `key`. */ +async function card(page: Page, key: string): Promise { + const entry = page.locator(`[data-mcp-server="${key}"]`); + await expect(entry).toHaveCount(1); + const header = entry.locator('button[aria-expanded]').first(); + if ((await header.getAttribute('aria-expanded')) !== 'true') await header.click(); + return entry; +} + +async function openGroup(entry: Locator, title: string) { + const trigger = entry.locator('.settings-group-header button', { hasText: title }).first(); + if ((await trigger.getAttribute('aria-expanded')) !== 'true') await trigger.click(); +} + +async function openMenu(page: Page, key: string) { + await page + .locator(`[data-mcp-server="${key}"]`) + .getByRole('button', { name: `Actions for ${key}` }) + .click(); +} + +async function menu(page: Page, key: string, item: string | RegExp) { + await openMenu(page, key); + await page.getByRole('menuitem', { name: item }).click(); +} + +async function review(page: Page) { + await unsavedBar(page) + .getByRole('button', { name: /^Review & save$/ }) + .click(); + return page.getByRole('dialog'); +} + +async function saveAndWait(page: Page) { + const dialog = page.getByRole('dialog'); + await dialog.getByRole('button', { name: /^Save$/ }).click(); + await expect(dialog).toHaveCount(0); +} + +async function setText(field: Locator, value: string) { + await field.fill(value); + await field.press('Tab'); +} + +test.describe('Configuration: MCP servers', () => { + test.skip(!LIVE, LIVE_SKIP_REASON); + test.describe.configure({ mode: 'serial', timeout: 120_000 }); + + test('turning Show in chat menu on and back to Default leaves nothing to save (MCP-6)', async ({ + page, + }) => { + await openMcp(page); + const entry = await card(page, PLAIN); + await openGroup(entry, 'Display'); + const choice = entry.locator(`#mcpServers-${PLAIN}-chatMenu`); + await choice.getByRole('button', { name: 'On', exact: true }).click(); + await expect(unsavedBar(page)).toBeVisible(); + await choice.getByRole('button', { name: /^Default/ }).click(); + await expect(unsavedBar(page)).toHaveCount(0); + }); + + test('a librechat.yaml header and the transport cannot be removed or changed (MCP-5)', async ({ + page, + }) => { + await openMcp(page); + const entry = await card(page, REMOTE); + await expect(entry.locator(`#mcpServers-${REMOTE}-headers`)).toBeVisible(); + await expect( + entry.locator(`#mcpServers-${REMOTE}-headers`).getByRole('button', { name: /^Delete / }), + ).toHaveCount(0); + await expect(entry.locator(`select#mcpServers-${REMOTE}-type`)).toHaveCount(0); + await openMenu(page, REMOTE); + await expect(page.getByRole('menuitem', { name: 'Delete' })).toHaveAttribute( + 'aria-disabled', + 'true', + ); + }); + + test('a stdio server is read-only (D10)', async ({ page }) => { + await openMcp(page); + const entry = await card(page, STDIO); + await expect(entry.getByRole('note').first()).toContainText('local process'); + await expect(entry.locator(`input#mcpServers-${STDIO}-command`)).toHaveCount(0); + await openGroup(entry, 'Display'); + await expect(entry.locator(`input#mcpServers-${STDIO}-title`)).toHaveCount(0); + }); + + test('create, rename with an edit, and delete a server (MCP-4, MCP-9, MCP-12)', async ({ + page, + }) => { + const name = `e2e-${Date.now()}`; + const renamed = `${name}-2`; + await openMcp(page); + + await page.getByRole('button', { name: 'Create MCP server' }).click(); + const create = page.getByRole('dialog'); + await expect(create.locator('#mcpDraft-transport-streamable-http')).toBeChecked(); + await expect(create.locator('#mcpDraft-transport-stdio')).toBeDisabled(); + await create.getByLabel('Server name').fill(name); + await expect(create).toContainText('Still needed: Server URL'); + await setText(create.locator('#mcpDraft-url'), 'https://mcp.e2e.example/mcp'); + await create.getByRole('button', { name: 'API key', exact: true }).click(); + const key = create.locator('#mcpDraft-apiKey-key'); + await expect(key).toHaveAttribute('type', 'password'); + await setText(key, 'sk-e2e-secret'); + await create.getByRole('button', { name: 'Create', exact: true }).click(); + await expect(create).toHaveCount(0); + + let dialog = await review(page); + await expect(dialog).toContainText(`Added MCP server ${name}`); + await expect(dialog).not.toContainText('sk-e2e-secret'); + await saveAndWait(page); + + await page.reload(); + await page.waitForLoadState('networkidle'); + await menu(page, name, 'Rename'); + const input = page.getByRole('textbox', { name: `Rename ${name}` }); + await input.fill(renamed); + await input.press('Enter'); + const entry = await card(page, renamed); + await openGroup(entry, 'Display'); + await setText(entry.locator(`#mcpServers-${renamed}-title`), 'Renamed in e2e'); + dialog = await review(page); + await expect(dialog).toContainText(`(${name} → ${renamed})`); + await expect(dialog).toContainText('Renamed in e2e'); + await saveAndWait(page); + + await menu(page, renamed, 'Delete'); + const ghost = page.locator(`[data-mcp-deleted="${renamed}"]`); + await expect(ghost).toContainText('Will be deleted'); + await ghost.getByRole('button', { name: `Undo deleting ${renamed}` }).click(); + await expect(page.locator(`[data-mcp-server="${renamed}"]`)).toHaveCount(1); + await menu(page, renamed, 'Delete'); + dialog = await review(page); + await expect(dialog).toContainText(`Removed MCP server ${renamed}`); + await saveAndWait(page); + await page.reload(); + await page.waitForLoadState('networkidle'); + await expect(page.locator(`[data-mcp-server="${renamed}"]`)).toHaveCount(0); + }); +}); diff --git a/src/components/configuration/ConfigPage.tsx b/src/components/configuration/ConfigPage.tsx index 6a1f7bf6..5ce998d6 100644 --- a/src/components/configuration/ConfigPage.tsx +++ b/src/components/configuration/ConfigPage.tsx @@ -607,6 +607,11 @@ export function ConfigPage({ filterPaths={filterPaths} valueLayers={layers.valueLayers} visibilityEdits={edits.visibilityEdits} + onHideField={permissions.canAssign ? edits.handleHideField : undefined} + onUnhideField={ + permissions.canAssign ? edits.handleUnhideField : undefined + } + onRenameEntry={edits.handleRenameEntry} /> diff --git a/src/components/configuration/ConfigTabContent.tsx b/src/components/configuration/ConfigTabContent.tsx index c63dc972..005d974c 100644 --- a/src/components/configuration/ConfigTabContent.tsx +++ b/src/components/configuration/ConfigTabContent.tsx @@ -78,6 +78,9 @@ export function ConfigTabContent({ unknownSettings, valueLayers, visibilityEdits, + onHideField, + onUnhideField, + onRenameEntry, visibleSectionIds, filterPaths, filterText, @@ -270,6 +273,9 @@ export function ConfigTabContent({ yamlBaseKeys: baseRecordKeys?.[dataKey], onValidationError, editSessionId, + onHideField, + onUnhideField, + onRenameEntry, }; const content = ( <> diff --git a/src/components/configuration/configMeta.ts b/src/components/configuration/configMeta.ts index 4704c876..b513a5b9 100644 --- a/src/components/configuration/configMeta.ts +++ b/src/components/configuration/configMeta.ts @@ -173,7 +173,7 @@ export const SECTION_META: Record = { }, mcpSettings: { titleKey: 'com_config_section_mcp_settings', - descriptionKey: 'com_config_section_mcp_settings_desc', + descriptionKey: 'com_config_mcp_settings_intro', tab: 'mcp', order: 20, }, diff --git a/src/components/configuration/edits.test.ts b/src/components/configuration/edits.test.ts index 18f80541..25d7b810 100644 --- a/src/components/configuration/edits.test.ts +++ b/src/components/configuration/edits.test.ts @@ -14,7 +14,7 @@ import { } from './edits'; import { getKeyedCollection, getValueAtPath, planConfigChanges } from '@/utils'; import { extractSchemaTree, getSchemaPathSet } from '@/server/config'; -import { issueDisplayPath, formatIssue } from './issues'; +import { issueDisplayPath, formatIssue, describeIssues } from './issues'; import { summarizeChanges } from './summary'; import { findSchemaField } from './controls'; @@ -428,6 +428,17 @@ describe('mergeIndexedArrayEdits', () => { }); describe('issue display and summary', () => { + it("shows a plain list item's first problem on the list row", () => { + const issues: t.ConfigIssue[] = [ + { path: 'mcpSettings.allowedAddresses.0', code: 'custom', message: 'must include a port' }, + { path: 'mcpSettings.allowedAddresses.0', code: 'custom', message: 'is not private' }, + ]; + const { fieldErrors } = describeIssues(issues, [], (key, params) => + key === 'com_config_issue_list_item' ? `Item ${params?.n}: ${params?.message}` : key, + ); + expect(fieldErrors['mcpSettings.allowedAddresses']).toBe('Item 1: must include a port'); + }); + const edit0 = captureCollectionEdit(custom, root); const collections: t.PendingCollection[] = [ { collection: custom, items: [endpoints[1], endpoints[0], endpoints[2]], edit: edit0 }, diff --git a/src/components/configuration/issues.ts b/src/components/configuration/issues.ts index a9044c2e..525414f2 100644 --- a/src/components/configuration/issues.ts +++ b/src/components/configuration/issues.ts @@ -83,6 +83,23 @@ export function formatIssue( return { inline, summary }; } +/** + * A plain list's item an issue is about (`mcpSettings.allowedAddresses.0`): + * the list row shows the item's first problem ("Item 1: must include a + * port"), since the list control has no row per item. Entries of keyed + * collections show their issues in their own cards. + */ +function listItemOf( + issue: t.ConfigIssue, + path: string, +): { listPath: string; index: number } | undefined { + if (issueCollection(issue)) return undefined; + const dot = path.lastIndexOf('.'); + const segment = path.slice(dot + 1); + if (dot < 0 || !/^\d+$/.test(segment)) return undefined; + return { listPath: path.slice(0, dot), index: Number(segment) }; +} + /** Per-path inline messages and a summary line per issue, for the errors map and the review. */ export function describeIssues( issues: t.ConfigIssue[], @@ -96,6 +113,13 @@ export function describeIssues( const { inline, summary } = formatIssue(issue, path, localize); fieldErrors[path] = fieldErrors[path] ? `${fieldErrors[path]}; ${inline}` : inline; if (!summaries.includes(summary)) summaries.push(summary); + const item = listItemOf(issue, path); + if (item && !fieldErrors[item.listPath]) { + fieldErrors[item.listPath] = localize('com_config_issue_list_item', { + n: item.index + 1, + message: issuePredicate(issue, localize), + }); + } } return { fieldErrors, summaries }; } diff --git a/src/components/configuration/meta/index.ts b/src/components/configuration/meta/index.ts index f2cae690..6fbf9bf7 100644 --- a/src/components/configuration/meta/index.ts +++ b/src/components/configuration/meta/index.ts @@ -5,6 +5,7 @@ import { SECRET_CONFIG_KEYS } from '@/constants'; import { ENDPOINT_FIELD_META } from './endpoints'; import { GENERIC_FIELD_META } from './generic'; import { SPEC_FIELD_META } from './specs'; +import { MCP_FIELD_META } from './mcp'; /** Registries merged in order; area files are appended here (WS2 owns, WS9–13 contribute). */ const META_SOURCES: t.FieldMetaRegistry[] = [ @@ -12,6 +13,7 @@ const META_SOURCES: t.FieldMetaRegistry[] = [ PROVIDER_FIELD_META, ENDPOINT_FIELD_META, SPEC_FIELD_META, + MCP_FIELD_META, ]; const REGISTRY = buildFieldMetaRegistry(META_SOURCES); diff --git a/src/components/configuration/meta/mcp.ts b/src/components/configuration/meta/mcp.ts new file mode 100644 index 00000000..68c2ceae --- /dev/null +++ b/src/components/configuration/meta/mcp.ts @@ -0,0 +1,129 @@ +import type * as t from '@/types'; + +const SERVER = 'mcpServers.{}'; + +/** Plain-language labels of server fields, where the schema key alone reads poorly. */ +const LABELS: Readonly> = { + type: 'com_config_mcp_label_type', + url: 'com_config_mcp_label_url', + startup: 'com_config_mcp_label_startup', + chatMenu: 'com_config_mcp_label_chatMenu', + iconPath: 'com_config_mcp_label_iconPath', + timeout: 'com_config_mcp_label_timeout', + initTimeout: 'com_config_mcp_label_initTimeout', + sseReadTimeout: 'com_config_mcp_label_sseReadTimeout', + requiresOAuth: 'com_config_mcp_label_requiresOAuth', + requestHeaders: 'com_config_mcp_label_requestHeaders', + proxy: 'com_config_mcp_label_proxy', + oauth_headers: 'com_config_mcp_label_oauth_headers', + customUserVars: 'com_config_mcp_label_customUserVars', + 'apiKey.key': 'com_config_mcp_label_apiKey_key', + 'apiKey.source': 'com_config_mcp_label_apiKey_source', + 'apiKey.authorization_type': 'com_config_mcp_label_apiKey_authorization_type', + 'apiKey.custom_header': 'com_config_mcp_label_apiKey_custom_header', + 'obo.scopes': 'com_config_mcp_label_obo_scopes', +}; + +const ENUM_LABELS: t.FieldMetaRegistry = { + [`${SERVER}.type`]: { + enumLabels: { + 'streamable-http': 'com_config_mcp_transport_streamable_http', + http: 'com_config_mcp_transport_http', + sse: 'com_config_mcp_transport_sse', + websocket: 'com_config_mcp_transport_websocket', + stdio: 'com_config_mcp_transport_stdio', + }, + }, + [`${SERVER}.apiKey.source`]: { + enumLabels: { + admin: 'com_config_mcp_enum_source_admin', + user: 'com_config_mcp_enum_source_user', + }, + }, + [`${SERVER}.apiKey.authorization_type`]: { + enumLabels: { + bearer: 'com_config_mcp_enum_auth_bearer', + basic: 'com_config_mcp_enum_auth_basic', + custom: 'com_config_mcp_enum_auth_custom', + }, + }, + [`${SERVER}.oauth.token_exchange_method`]: { + enumLabels: { + default_post: 'com_config_mcp_enum_exchange_post', + basic_auth_header: 'com_config_mcp_enum_exchange_basic', + }, + }, +}; + +/** What LibreChat's MCP connection uses when a server sets no timeout (`connection.ts`). */ +const RUNTIME_DEFAULTS: t.FieldMetaRegistry = { + [`${SERVER}.timeout`]: { runtimeDefault: 60_000 }, + [`${SERVER}.initTimeout`]: { runtimeDefault: 30_000 }, +}; + +const CONTROLS: t.FieldMetaRegistry = { + [`${SERVER}.description`]: { multiline: true }, + [`${SERVER}.oauth.client_secret`]: { secret: true }, + [`${SERVER}.url`]: { width: 'L' }, + [`${SERVER}.proxy`]: { width: 'L' }, + [`${SERVER}.iconPath`]: { width: 'L' }, +}; + +/** The create dialog's draft root, named in the tri-states it holds ("New server: Startup"). */ +const DRAFT: t.FieldMetaRegistry = { + mcpDraft: { labelKey: 'com_config_mcp_draft_label' }, +}; + +/** Catalog recovery numbers whose label repeated the unit the input already shows. */ +const CATALOG_NUMBERS = [ + 'discoveryTimeoutMs', + 'discoverySettleGraceMs', + 'reauthRetryMs', + 'generationReadTimeoutMs', + 'authorizationFenceTimeoutMs', + 'authorizationFenceRetryIntervalMs', +]; + +/** MCP settings (MCP-13): an operational-tuning label and help for catalog recovery, plain labels for its knobs. */ +function settingsMeta(): t.FieldMetaRegistry { + const entries: t.FieldMetaRegistry = { + 'mcpSettings.allowedDomains': { descriptionKey: 'com_config_mcp_settings_domains_desc' }, + 'mcpSettings.allowedAddresses': { descriptionKey: 'com_config_mcp_settings_addresses_desc' }, + 'mcpSettings.catalogRecovery': { + labelKey: 'com_config_mcp_label_catalogRecovery', + descriptionKey: 'com_config_desc_mcpSettings_catalogRecovery', + }, + }; + for (const key of CATALOG_NUMBERS) { + entries[`mcpSettings.catalogRecovery.${key}`] = { + labelKey: `com_config_mcp_label_${key}`, + unit: 'ms', + }; + } + return entries; +} + +function labelMeta(): t.FieldMetaRegistry { + return Object.fromEntries( + Object.entries(LABELS).map(([key, labelKey]) => [`${SERVER}.${key}`, { labelKey }]), + ); +} + +/** + * MCP server and MCP settings metadata (WS12): plain labels, enum labels, + * runtime defaults, multiline and secret inputs, and control widths. Merged + * after the generic registry, so its entries win. + */ +export const MCP_FIELD_META: t.FieldMetaRegistry = [ + labelMeta(), + ENUM_LABELS, + RUNTIME_DEFAULTS, + CONTROLS, + DRAFT, + settingsMeta(), +].reduce((merged, registry) => { + for (const [pattern, meta] of Object.entries(registry)) { + merged[pattern] = { ...merged[pattern], ...meta }; + } + return merged; +}, {}); diff --git a/src/components/configuration/review/diff.ts b/src/components/configuration/review/diff.ts index 32df1f0d..b99eefd7 100644 --- a/src/components/configuration/review/diff.ts +++ b/src/components/configuration/review/diff.ts @@ -34,6 +34,8 @@ const ALLOWLIST_KEYS: ReadonlySet = new Set([ const INDEX_SEGMENT_RE = /^\d+$/; +const NO_RENAMES: Readonly> = {}; + interface DiffContext { input: t.ChangeSummaryInput; sourcePath: string; @@ -531,7 +533,7 @@ export function summarizeChanges(input: t.ChangeSummaryInput): t.ChangeSummaryIt items.push(entryItem('added', entry, undefined, unflattenObject(leaves), ctx)); } for (const path of input.resets) items.push(resetItem(path, input)); - return pairRecordRenames(items); + return pairRecordRenames(items, input.renames ?? NO_RENAMES); } /** An entry change of a whole entry (added or removed), not one of its fields. */ @@ -539,31 +541,78 @@ function isWholeEntry(item: t.ChangeSummaryItem, kind: t.ChangeKind): boolean { return item.kind === kind && item.entryPath === item.path && item.collectionPath != null; } +/** The fields a renamed entry changes as well, as rows under its new name. */ +function renamedFieldChanges( + renamed: t.ChangeSummaryItem, + before: t.ConfigRecord, + after: t.ConfigRecord, +): t.ChangeSummaryItem[] { + const beforeLeaves = flattenObject(before); + const afterLeaves = flattenObject(after); + const leaves = [...new Set([...Object.keys(beforeLeaves), ...Object.keys(afterLeaves)])]; + return leaves + .filter((leaf) => !isConfigValueEqual(beforeLeaves[leaf], afterLeaves[leaf])) + .map((leaf) => { + const path = `${renamed.path}.${leaf}`; + return { + path, + kind: leafKind(beforeLeaves[leaf], afterLeaves[leaf]), + before: beforeLeaves[leaf], + after: afterLeaves[leaf], + sourcePath: renamed.sourcePath, + secret: isSecretPath(path) || undefined, + collectionPath: renamed.collectionPath, + entryPath: renamed.entryPath, + entryIdentity: renamed.entryIdentity, + previousIdentity: renamed.previousIdentity, + }; + }); +} + /** - * A record entry removed and one added with the same value (an MCP server + * A record entry removed and one added in the same save (an MCP server * renamed: the old key's leaves are removed, the new key's written) read as - * one rename. + * one rename: the entry the panel recorded as renamed from the removed one + * (`renames`), whose other changes are listed under it, else an added entry + * with the same value as a removed one. A shared URL or command alone is no + * proof: a server deleted and an unrelated one added for the same endpoint + * stay a removal and an addition. */ -function pairRecordRenames(items: t.ChangeSummaryItem[]): t.ChangeSummaryItem[] { +function pairRecordRenames( + items: t.ChangeSummaryItem[], + renames: Readonly>, +): t.ChangeSummaryItem[] { const removed = items.filter((item) => isWholeEntry(item, 'removed')); if (removed.length === 0) return items; const paired = new Set(); - const result = items.map((item): t.ChangeSummaryItem => { - if (!isWholeEntry(item, 'added') || !isConfigRecord(item.after)) return item; - const match = removed.find( + const findMatch = ( + item: t.ChangeSummaryItem, + matches: (candidate: t.ChangeSummaryItem) => boolean, + ) => + removed.find( (candidate) => !paired.has(candidate) && candidate.collectionPath === item.collectionPath && - isConfigValueEqual(candidate.before, item.after), + matches(candidate), ); - if (!match) return item; + const result = items.flatMap((item): t.ChangeSummaryItem[] => { + if (!isWholeEntry(item, 'added') || !isConfigRecord(item.after)) return [item]; + const renamedFrom = item.entryPath ? renames[item.entryPath] : undefined; + const match = + findMatch(item, (candidate) => candidate.entryPath === renamedFrom) ?? + findMatch(item, (candidate) => isConfigValueEqual(candidate.before, item.after)); + if (!match) return [item]; paired.add(match); - return { + const renamed: t.ChangeSummaryItem = { ...item, kind: 'renamed', before: match.before, previousIdentity: match.entryIdentity, }; + if (isConfigValueEqual(match.before, item.after) || !isConfigRecord(match.before)) { + return [renamed]; + } + return [renamed, ...renamedFieldChanges(renamed, match.before, item.after)]; }); return result.filter((item) => !paired.has(item)); } diff --git a/src/components/configuration/review/group.ts b/src/components/configuration/review/group.ts index b097c472..27655441 100644 --- a/src/components/configuration/review/group.ts +++ b/src/components/configuration/review/group.ts @@ -201,7 +201,9 @@ function valueLabelsAt( const discriminated = discriminatorLabelsAt(path, ctx); if (discriminated) return discriminated; const field = findSchemaField(ctx.fields, path); - if (!field || !field.type.startsWith('enum')) return undefined; + if (!field || !(field.type.startsWith('enum') || field.type.startsWith('union(literal'))) { + return undefined; + } const options = getFieldOptions(field, ctx.localize); return options.length > 0 ? Object.fromEntries(options.map((option) => [option.value, option.label])) @@ -234,6 +236,7 @@ function addedDetails(item: t.ChangeSummaryItem, key: string, ctx: t.ReviewLabel after: value, secret: isSecretPath(path) || undefined, }, + valueLabels: valueLabelsAt(path, ctx), }; }); } diff --git a/src/components/configuration/review/revert.ts b/src/components/configuration/review/revert.ts index d42b4099..16c329a2 100644 --- a/src/components/configuration/review/revert.ts +++ b/src/components/configuration/review/revert.ts @@ -1,9 +1,10 @@ import type * as t from '@/types'; -import { getValueAtPath, isConfigRecord, deepSerializeKVPairs } from '@/utils'; +import { getValueAtPath, flattenObject, isConfigRecord, deepSerializeKVPairs } from '@/utils'; import { isConfigValueEqual, normalizeForSave } from '../edits'; -import { identityKeyFor } from './diff'; +import { identityKeyFor, isSecretPath } from './diff'; const INDEX_SEGMENT_RE = /^\d+$/; +const PREVIEW_SUFFIX = 'Preview'; function childOf(value: t.ConfigValue, segment: string): t.ConfigValue { if (Array.isArray(value)) @@ -159,6 +160,68 @@ function pendingUnder(editedValues: t.FlatConfigMap, path: string): string[] { return Object.keys(editedValues).filter((key) => key === path || key.startsWith(`${path}.`)); } +/** The masked preview the admin API returns for a stored secret (`oauth.client_secretPreview`). */ +function isSecretPreview(leaf: string): boolean { + return leaf.endsWith(PREVIEW_SUFFIX) && isSecretPath(leaf.slice(0, -PREVIEW_SUFFIX.length)); +} + +/** Whether a removed value is only what a rename never carries: stored secrets' previews. */ +function isPreviewOnly(value: t.ConfigValue, path: string): boolean { + if (!isConfigRecord(value)) return isSecretPreview(path); + return Object.keys(flattenObject(value, path)).every(isSecretPreview); +} + +/** + * The writes that give an entry under its old key the changes it got under + * its new one: a setting it dropped is removed whole (an authentication + * method switched off goes with its stored secret), and within an object it + * kept, each leaf that changed is written (or removed). Stored secrets' + * previews are no change: a rename never carries them. + */ +function renamedChanges( + before: t.ConfigRecord, + after: t.ConfigRecord, + previousPath: string, +): Array<{ path: string; value: t.ConfigValue }> { + return [...new Set([...Object.keys(before), ...Object.keys(after)])].flatMap((key) => { + const was = before[key]; + const now = after[key]; + if (isConfigValueEqual(was, now)) return []; + if (now === undefined) + return isPreviewOnly(was, key) ? [] : [{ path: `${previousPath}.${key}`, value: undefined }]; + if (!isConfigRecord(was) || !isConfigRecord(now)) + return [{ path: `${previousPath}.${key}`, value: now }]; + const wasLeaves = flattenObject(was, key); + const nowLeaves = flattenObject(now, key); + return [...new Set([...Object.keys(wasLeaves), ...Object.keys(nowLeaves)])] + .filter((leaf) => !isConfigValueEqual(wasLeaves[leaf], nowLeaves[leaf])) + .filter((leaf) => nowLeaves[leaf] !== undefined || !isSecretPreview(leaf)) + .map((leaf) => ({ path: `${previousPath}.${leaf}`, value: nowLeaves[leaf] })); + }); +} + +/** + * Undoes a record entry's rename (an MCP server is its new key's writes plus + * the old key's removal) and keeps the entry's other changes, which the + * review lists as rows of their own: they are written under the old key again. + */ +function revertRecordRename( + item: t.ChangeSummaryItem, + editedValues: t.FlatConfigMap, + entryPath: string, + previousPath: string, +): t.ReviewRevertAction { + const discard = [ + ...pendingUnder(editedValues, entryPath), + ...pendingUnder(editedValues, previousPath), + ]; + if (!isConfigRecord(item.before) || !isConfigRecord(item.after)) { + return { op: 'discard', paths: discard }; + } + const writes = renamedChanges(item.before, item.after, previousPath); + return { op: 'unrename', entryPath, discard, writes }; +} + /** * The pending edit a row comes from. A keyed collection is reviewed as its * whole pending list, but its entries are edited at their own positions @@ -193,13 +256,20 @@ export function planRevert( const isPart = isPending && item.path.startsWith(`${source}.`); const isOrder = isPending && item.path === source && item.collectionPath === source; const isRename = isPending && item.path === source && item.kind === 'renamed'; - /** A renamed record entry (an MCP server) is its new key's writes plus the old key's removals. */ if (!isPending && item.kind === 'renamed' && item.previousIdentity && item.collectionPath) { const previousPath = `${item.collectionPath}.${item.previousIdentity}`; - return { - op: 'discard', - paths: [...pendingUnder(editedValues, source), ...pendingUnder(editedValues, previousPath)], - }; + return revertRecordRename(item, editedValues, source, previousPath); + } + /** A field a renamed record entry also changed: the new key goes back to the old key's value. */ + const isRenamedField = + item.kind !== 'renamed' && + item.previousIdentity != null && + item.entryPath != null && + !INDEX_SEGMENT_RE.test(item.entryPath.slice(item.entryPath.lastIndexOf('.') + 1)); + if (isRenamedField && item.path in editedValues) { + return item.before === undefined + ? { op: 'discard', paths: [item.path] } + : { op: 'set', path: item.path, value: item.before }; } if (!isPart && !isOrder && !isRename) { const paths = pendingUnder(editedValues, source); diff --git a/src/components/configuration/review/review.test.ts b/src/components/configuration/review/review.test.ts index fa827d69..37c4e25b 100644 --- a/src/components/configuration/review/review.test.ts +++ b/src/components/configuration/review/review.test.ts @@ -7,6 +7,7 @@ import translation from '@/locales/en/translation.json'; import { extractSchemaTree } from '@/server/config'; import { parseIssueMessages } from './issues'; import { formatReviewValue } from './format'; +import { flattenObject } from '@/utils'; import { planRevert } from './revert'; const tree = extractSchemaTree(configSchema as t.ZodSchemaLike); @@ -312,6 +313,124 @@ describe('summarizeChanges: MCP servers and secrets', () => { ]); }); + it('reads a server renamed and edited in the same save as a rename plus its field change', () => { + const dbsrv = { type: 'streamable-http', url: 'https://db.example/mcp', title: 'DB' }; + const renamed = { ...dbsrv, title: 'Docs' }; + const items = summarizeChanges( + makeInput({ + baseline: { mcpServers: { dbsrv } }, + ownAt: (path) => (path === 'mcpServers.dbsrv' ? dbsrv : undefined), + saves: Object.entries(renamed).map(([key, value]) => ({ + fieldPath: `mcpServers.dbsrv2.${key}`, + value, + })), + resets: ['mcpServers.dbsrv'], + renames: { 'mcpServers.dbsrv2': 'mcpServers.dbsrv' }, + }), + ); + expect(items).toEqual([ + expect.objectContaining({ + kind: 'renamed', + path: 'mcpServers.dbsrv2', + previousIdentity: 'dbsrv', + }), + expect.objectContaining({ + kind: 'changed', + path: 'mcpServers.dbsrv2.title', + entryPath: 'mcpServers.dbsrv2', + before: 'DB', + after: 'Docs', + }), + ]); + const edited = { + 'mcpServers.dbsrv': undefined, + ...Object.fromEntries(Object.entries(renamed).map(([k, v]) => [`mcpServers.dbsrv2.${k}`, v])), + }; + expect(planRevert(items[1], edited, { mcpServers: { dbsrv } })).toEqual({ + op: 'set', + path: 'mcpServers.dbsrv2.title', + value: 'DB', + }); + }); + + it('undoes only the rename of a server, keeping its field changes under the old name', () => { + const dbsrv = { + type: 'streamable-http', + url: 'https://db.example/mcp', + title: 'DB', + description: 'Docs DB', + oauth: { client_id: 'id', client_secretPreview: 'sec...1234' }, + apiKey: { source: 'admin', keyPreview: '...4321' }, + }; + const renamed = { + type: 'streamable-http', + url: 'https://db2.example/mcp', + title: 'DB', + oauth: { client_id: 'id' }, + }; + const leaves = Object.entries(flattenObject(renamed, 'mcpServers.dbsrv2')); + const items = summarizeChanges( + makeInput({ + baseline: { mcpServers: { dbsrv } }, + ownAt: (path) => (path === 'mcpServers.dbsrv' ? dbsrv : undefined), + saves: leaves.map(([fieldPath, value]) => ({ fieldPath, value })), + resets: ['mcpServers.dbsrv'], + renames: { 'mcpServers.dbsrv2': 'mcpServers.dbsrv' }, + }), + ); + const edited: t.FlatConfigMap = { + 'mcpServers.dbsrv': undefined, + ...Object.fromEntries(leaves), + }; + const row = items.find((item) => item.kind === 'renamed')!; + expect(planRevert(row, edited, { mcpServers: { dbsrv } })).toEqual({ + op: 'unrename', + entryPath: 'mcpServers.dbsrv2', + discard: [...leaves.map(([path]) => path), 'mcpServers.dbsrv'], + writes: [ + { path: 'mcpServers.dbsrv.url', value: 'https://db2.example/mcp' }, + { path: 'mcpServers.dbsrv.description', value: undefined }, + { path: 'mcpServers.dbsrv.apiKey', value: undefined }, + ], + }); + }); + + it('keeps a removed server and a new one for the same URL apart when nothing renamed it', () => { + const dbsrv = { type: 'streamable-http', url: 'https://db.example/mcp', title: 'DB' }; + const items = summarizeChanges( + makeInput({ + baseline: { mcpServers: { dbsrv } }, + ownAt: (path) => (path === 'mcpServers.dbsrv' ? dbsrv : undefined), + saves: [ + { fieldPath: 'mcpServers.dbsrv-oauth.type', value: 'streamable-http' }, + { fieldPath: 'mcpServers.dbsrv-oauth.url', value: 'https://db.example/mcp' }, + { fieldPath: 'mcpServers.dbsrv-oauth.requiresOAuth', value: true }, + ], + resets: ['mcpServers.dbsrv'], + }), + ); + expect(items.map((item) => [item.kind, item.path]).sort()).toEqual([ + ['added', 'mcpServers.dbsrv-oauth'], + ['removed', 'mcpServers.dbsrv'], + ]); + }); + + it('keeps a removed server and an unrelated new one apart', () => { + const dbsrv = { type: 'sse', url: 'https://db.example/sse' }; + const items = summarizeChanges( + makeInput({ + baseline: { mcpServers: { dbsrv } }, + ownAt: (path) => (path === 'mcpServers.dbsrv' ? dbsrv : undefined), + saves: [ + { fieldPath: 'mcpServers.other.type', value: 'sse' }, + { fieldPath: 'mcpServers.other.url', value: 'https://other.example/sse' }, + ], + resets: ['mcpServers.dbsrv'], + }), + ); + expect(items.map((item) => item.kind).sort()).toEqual(['added', 'removed']); + }); + it('keeps field changes of an existing server under that server', () => { const [item] = summarizeChanges( makeInput({ baseline, saves: [{ fieldPath: 'mcpServers.github.timeout', value: 45000 }] }), @@ -415,7 +534,8 @@ describe('buildReviewModel', () => { const model = buildReviewModel(summarizeChanges(makeInput({ baseline, saves })), ctx(saves)); const [row] = model.tabs[0].sections[0].rows; expect(row.label).toBe('acme'); - expect(row.details?.map((d) => d.label)).toEqual(['Type', 'URL']); + expect(row.details?.map((d) => d.label)).toEqual(['Transport', 'Server URL']); + expect(row.details?.[0].valueLabels?.['streamable-http']).toBe('Streamable HTTP'); expect(model.count).toBe(1); }); diff --git a/src/components/configuration/search.ts b/src/components/configuration/search.ts index fcd37180..814fc3df 100644 --- a/src/components/configuration/search.ts +++ b/src/components/configuration/search.ts @@ -17,8 +17,7 @@ import { import { isEnvReference, getValueAtPath, isConfigRecord, maskSecretValues } from '@/utils'; import { getFieldMeta, labelForPath, describeField } from './meta'; import { formatFieldValue } from './controls'; -import { inferTransportType } from './sections/mcp/validation'; -import { isMcpFieldShown } from './sections/mcpFieldMeta'; +import { isMcpFieldShown, transportOf } from './sections/mcp/transport'; import { specSearchFields } from './sections/specs/params'; import { getEntryTitle } from './fields/entries'; @@ -282,7 +281,7 @@ export function buildConfigSearchIndex( function entryFields(section: t.SchemaField, entry: t.ConfigValue): t.SchemaField[] { const fields = section.children ?? []; if (section.key !== 'mcpServers' || !isConfigRecord(entry)) return fields; - const transport = inferTransportType(entry); + const transport = transportOf(entry); return fields.filter((field) => isMcpFieldShown(field.key, transport)); } diff --git a/src/components/configuration/sections/McpServersRenderer.tsx b/src/components/configuration/sections/McpServersRenderer.tsx deleted file mode 100644 index 301e72f8..00000000 --- a/src/components/configuration/sections/McpServersRenderer.tsx +++ /dev/null @@ -1,1111 +0,0 @@ -import { isProcessMCPServerConfig } from 'librechat-data-provider'; -import { memo, useRef, useMemo, useState, useEffect, useContext, useCallback } from 'react'; -import type { ReactNode } from 'react'; -import type * as t from '@/types'; -import { - setLeaf, - deepClone, - isPlainObject, - resolveEntryKey, - inferTransportType, - validateMcpCrossField, - REQUIRED_BY_TRANSPORT, -} from './mcp/validation'; -import { - isMcpFieldShown, - INSPECTOR_DERIVED, - YAML_LOCKED_FIELDS, - PROCESS_LOCKED_FIELDS, -} from './mcpFieldMeta'; -import { useCollapsibleSection } from '../useCollapsibleSection'; -import { DecorativeIcon, FormDialog } from '@/components/shared'; -import { RowFilterContext } from '@/contexts/RowFilterContext'; -import { cn, isRowVisible, isUnsafeConfigKey, isConfigValueEqual } from '@/utils'; -import { ObjectEntryCard } from '../fields/ObjectEntryCard'; -import { renderInlineField, toFieldId } from '../renderer'; -import { renderCollapsible } from '../renderCollapsible'; -import { useLocalize, useLowerEntryKeys, useRecordEntryOverrides } from '@/hooks'; -import { SelectField } from '../fields/SelectField'; -import { SettingsRow } from '../SettingsRow'; - -const TRANSPORT_TYPES = ['streamable-http', 'sse', 'websocket', 'stdio'] as const; - -/** - * Transport choices. stdio servers run a local process, which only - * librechat.yaml can define (D10): the choice is shown, disabled, unless the - * server already is one. - */ -function transportOptions(current: string, localize: t.LocalizeFn): t.SelectOption[] { - return TRANSPORT_TYPES.map((value) => { - const isLocked = value === 'stdio' && current !== 'stdio'; - const key = isLocked ? 'stdio_locked' : value.replace('-', '_'); - return { value, label: localize(`com_config_mcp_transport_${key}`), disabled: isLocked }; - }); -} - -const ALWAYS_REQUIRED = new Set(['type']); - -/** Stable empty record used as the fallback for `baseRecord`/`parentValue` when no data is available, so the downstream `useMemo` chain on `editsByEntry`/`record` does not re-fire on every render with a fresh `{}` literal. */ -const EMPTY_RECORD: Record = Object.freeze({}) as Record< - string, - t.ConfigValue ->; - -function withFieldOverrides(field: t.SchemaField, transportType: string): t.SchemaField { - if (ALWAYS_REQUIRED.has(field.key)) { - return { ...field, isOptional: false }; - } - const transportRequired = REQUIRED_BY_TRANSPORT[transportType]; - if (transportRequired?.has(field.key)) { - return { ...field, isOptional: false }; - } - return field; -} - -function applyLeafOverlay( - base: Record, - leafEdits: Array<[string[], t.ConfigValue]>, -): Record { - const cloned = deepClone(base); - for (const [segments, value] of leafEdits) { - setLeaf(cloned, segments, value); - } - return cloned; -} - -function enumerateLeafPaths( - obj: Record, - prefix: string[] = [], - seen: WeakSet = new WeakSet(), -): Array<{ segments: string[]; value: t.ConfigValue }> { - if (seen.has(obj)) return []; - seen.add(obj); - const out: Array<{ segments: string[]; value: t.ConfigValue }> = []; - for (const [k, v] of Object.entries(obj)) { - const next = [...prefix, k]; - if (isPlainObject(v)) { - out.push(...enumerateLeafPaths(v, next, seen)); - } else { - out.push({ segments: next, value: v }); - } - } - return out; -} - -const MCP_FIELD_GROUPS: t.FieldGroupDef[] = [ - { - labelKey: 'com_config_group_connection', - fields: ['type', 'url', 'command', 'args', 'headers', 'env', 'cwd', 'stderr', 'requiresOAuth'], - defaultExpanded: true, - }, - { - labelKey: 'com_config_group_authentication', - fields: [], - defaultExpanded: false, - children: [ - { - labelKey: 'com_config_group_api_key', - fields: ['apiKey'], - defaultExpanded: true, - }, - { - labelKey: 'com_config_group_oauth', - fields: ['oauth', 'oauth_headers'], - defaultExpanded: false, - }, - ], - }, - { - labelKey: 'com_config_group_server_options', - fields: [ - 'title', - 'description', - 'startup', - 'chatMenu', - 'serverInstructions', - 'timeout', - 'sseReadTimeout', - 'initTimeout', - 'iconPath', - ], - defaultExpanded: false, - }, -]; - -function flattenGroupFields( - fields: t.SchemaField[], - parentValue: t.ConfigValue, - parentPath: string, - onChange: (path: string, value: t.ConfigValue) => void, - localize: (key: string, interpolation?: Record) => string, - transportType: string, - disabled?: boolean, - collectionRenderOverrides?: Record, - lockedKeys?: Set, - rowFilter: t.PathIndex | null = null, -): ReactNode[] { - const values = isPlainObject(parentValue) ? parentValue : {}; - - const nodes: ReactNode[] = []; - for (const field of fields) { - const fieldDisabled = disabled || (lockedKeys?.has(field.key) ?? false); - if (field.key === 'type') { - if (!isRowVisible(rowFilter, `${parentPath}.${field.key}`)) continue; - const fieldId = toFieldId(`${parentPath}.${field.key}`); - const label = localize(`com_config_field_${field.key}`); - const explicitValue = typeof values.type === 'string' ? values.type : ''; - const rawValue = explicitValue || inferTransportType(values); - const displayValue = rawValue === 'http' ? 'streamable-http' : rawValue; - nodes.push( -