From 2bcf3e8582f4d1a428575de0135fa59f64638557 Mon Sep 17 00:00:00 2001 From: Dustin Healy <54083382+dustinhealy@users.noreply.github.com> Date: Sat, 23 May 2026 14:11:13 -0700 Subject: [PATCH] =?UTF-8?q?=F0=9F=AA=9F=20fix:=20Apply=20Admin-Panel=20Con?= =?UTF-8?q?fig=20Overrides=20To=20YAML-Defined=20MCP=20Servers=20(#13173)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * ๐ŸชŸ fix: Apply Admin-Panel Config Overrides To YAML-Defined MCP Servers Admin-panel saves of MCP server fields for YAML-defined servers were silently dropped by the registry. ensureConfigServers filtered out any merged config entry whose name appeared in YAML, so overrides such as iconPath, title, and description never reached getAllServerConfigs even though the override row had been written to the configs collection and the AppConfig merge layer had produced the correct merged result. The filter is removed and replaced with a content-equivalence short-circuit in ensureSingleConfigServer. YAML-defined servers whose merged config matches the YAML cache entry skip lazy-init, so unmodified YAML servers still avoid a redundant inspection round trip. The new private helper matchesYamlConfig reuses the existing content-hash function on configurable fields only. getAllServerConfigs now overlays config-tier entries onto the YAML base while preserving user-DB entries (source: 'user'), giving precedence of YAML, then Config tier, then User DB. The docstring is updated to describe the new order. Multi-tenancy is already enforced upstream of the registry by the AppConfig layer, so the registry stays tenant-agnostic and overrides remain isolated per tenant. Tests cover the new behavior: config-tier override on YAML-defined server flows through to getAllServerConfigs, YAML servers without effective overrides skip lazy-init, user-DB entries win over config-tier overlays, pure config-tier servers still lazy-init, and the merged config passed to lazy-init preserves all YAML fields when the override only adds new ones. * ๐Ÿ› ๏ธ fix: Address Review Feedback For YAML Override Precedence Both Copilot and Codex flagged matchesYamlConfig as broken in production: the cached YAML config carries inspector-derived defaults (requiresOAuth defaulted to false when YAML omits it, serverInstructions rewritten from the YAML toggle to the fetched server-instructions string, and so on) that are absent from appConfig.mcpConfig. The content-hash comparison reports a mismatch for every YAML server with no admin-panel override, so ensureSingleConfigServer re-inspects all of them anyway. The optimization never fires in practice and reintroduces the source tagging it was supposed to avoid. Remove the short-circuit and the matchesYamlConfig helper. YAML-defined servers that appear in the merged config go through lazy-init like any other entry. A smarter optimization that overlays cosmetic-only override fields onto the YAML cache without re-inspection belongs in a follow-up once the boundary between configurable fields and inspector-derived fields is well defined. Update the existing ensureConfigServers tests that asserted the old filter behavior (should exclude YAML servers from config-source detection, should return empty when all servers are YAML) to assert the new behavior: YAML servers pass through ensureConfigServers and are lazy-initialized when they appear in the merged config. Make the inspector mock more realistic by spreading the raw input first and overlaying only runtime fields, so the test fixtures match production where the inspector preserves configurable fields. Drop the companion test in MCPServersRegistry.test.ts that asserted the short-circuit fires for unchanged YAML servers; the hand-crafted fixture skipped inspector defaults and was not representative. Copilot also flagged that getServerConfig short-circuits to configServers before checking the user DB, so the precedence enforced in getAllServerConfigs (user-DB beats config-tier) was bypassed in the single-server lookup. When configServers carries an entry and a userId is available, check the user DB first and prefer a source: 'user' entry so per-user servers are never shadowed by an admin-panel override. * ๐Ÿ›ก๏ธ fix: Harden Admin Override Overlay Against Failure Stubs Hardens the admin-panel override path against transient inspect failures and removes a defensive branch that guarded an impossible state. The getAllServerConfigs overlay now skips failed-inspection stubs so a healthy YAML or DB entry stays visible during the 5-minute retry window instead of being clobbered by a stub. When the overlay does land, the base entry's source tier is preserved, which keeps Tools/mcp.js routing its failed-inspection recovery to the correct storage location. The user-DB precedence block in getServerConfig is removed: configServers is built from appConfig.mcpConfig which only ever carries admin-tier entries, so the DB lookup defended a state that cannot occur via the current call graph. The dead yamlServerNames memoization is also gone. Adds two regression tests covering inspection-failure preservation and source-tier preservation on successful overlay, and adds a debug log when an admin override is suppressed by a user-tier entry. The makeParsedConfig test factory now honors overrides correctly. * ๐Ÿงช test: Strengthen Admin Override Coverage And Docs Adds an end-to-end regression test that chains MCPServerInspector.inspect failure through ensureConfigServers and getAllServerConfigs, asserting the healthy YAML base entry survives a transient inspect failure intact. The previous regression test hand-built a failure stub and skipped ensureSingleConfigServer, leaving the production chain itself untested. The getAllServerConfigs docstring now spells out both overlay guards (failed-stub skip and user-tier preservation) and the source-field preservation contract that downstream recovery logic depends on. The yamlLangfuseConfig test fixture is frozen so a future test cannot mutate it and contaminate sibling tests in the describe block. * ๐Ÿ”ง fix: Skip Lazy-Init For Unchanged YAML MCP Servers Adds an admin-configurable-field equivalence check so YAML-defined MCP servers that carry no admin override skip lazy-init in ensureConfigServers. This avoids the per-request inspect storm and keeps unmodified YAML servers out of the config-tier cache, so admin saves that touch unrelated overrides no longer evict and tear down those YAML connections. A second guard in getServerConfig prevents failed-inspection stubs in configServers from shadowing the healthy YAML base entry for the duration of the retry window. The aggregate path already had this guard via getAllServerConfigs; this brings the single-server path to parity, so Tools/mcp.js recovery routes to YAML reinspection rather than bailing on the config retry timer. Adds three regression tests covering the unmodified-YAML skip, the admin-override lazy-init trigger, and the failure-stub fallthrough. Updates two existing ensureConfigServers tests that previously documented the now-incorrect "always lazy-init YAML" behavior. * ๐Ÿ”“ feat: Expose baseOnly Flag On Admin Config Base Endpoint The admin getBaseConfig handler now reads req.query.baseOnly and forwards it to getAppConfig so an admin panel client can request the un-merged YAML and AppService base configuration without DB overrides applied. The flag is opt-in; existing callers see no behaviour change because the default remains the merged response. The query value is coerced through String() so Express array forms like baseOnly=true&baseOnly=true are treated as false rather than truthy by accident. A handler test pins the forwarding behaviour and the default-merged behaviour against future regressions. * ๐Ÿงน fix: Address Codex Review Findings On MCP Registry Precedence Path Four follow-ups from the Codex review of PR #13173: P1. getServerConfig now preserves the configServers candidate as a last-resort fallback when both YAML cache and user DB return nothing, so admin-defined config-only servers carrying inspectionFailed=true still surface the failure stub to callers in api/server/services/Tools/mcp.js that rely on it to return the still-unreachable message. The not-found memoization is preserved. P2a. proxy is added to ADMIN_CONFIGURABLE_FIELDS so an admin override on SSE/streamable-http proxy is no longer treated as an unchanged YAML server and correctly triggers lazy-init. P2b. isUnmodifiedYamlServer now treats absent-on-rawConfig fields as equal, so inspector-derived values on the cached YAML entry (notably requiresOAuth filled in by detectOAuth at startup) do not force unmodified YAML servers to re-init on every request. P3. getBaseConfig parses ?baseOnly strictly against the literal string true instead of String-coercing, so array shapes like baseOnly[]=true no longer pass through. Regression tests cover all four paths. * ๐Ÿงน fix: Drop Misleading Shadow Warning On Config Vs User-DB Collisions The Config-tier branch of warnOnOperatorManagedNameCollisions logged that Config MCP servers shadow DB-backed servers, but getAllServerConfigs actually preserves the user-tier entry on a Config-vs-user collision and skips the override. The warning was describing the opposite of what the code does and would mislead operational debugging. The YAML-tier call is unchanged because YAML still legitimately shadows DB-backed servers. The per-entry debug log inside the collision branch already captures the actual outcome. Test renamed and rewritten to assert the user-tier entry is preserved and no shadow warning is emitted. * ๐Ÿ”’ fix: Keep Tenant-Scoped configServers Candidate Out Of The Global Read-Through Cache The prior fix for surfacing inspectionFailed stubs from admin-defined config-only servers wrote the per-call configServers candidate into readThroughCache when YAML and DB both missed. The cache key is keyed by serverName plus userId, so a failed stub from one tenant could satisfy a later no-userId lookup made by another tenant before any configServers resolution ran. getServerConfig now caches only the global YAML/DB resolution (still caching undefined to memoize not-found lookups) and uses the candidate strictly as an unmemoized function-level fallback that surfaces the failure stub to the caller without leaking it across tenants. Regression test exercises a no-userId call after a tenant-scoped failure and asserts the cache returns undefined rather than the stub, and that a second tenant sees their own healthy candidate. * ๐Ÿ”„ fix: Mirror getAllServerConfigs Precedence Exactly In getServerConfig getServerConfig was short-circuiting with the configServers candidate on every healthy lookup, which made single-server callers diverge from the aggregate path for name collisions between config-tier overrides and user-DB entries. The aggregate path preserves the user-tier entry on such collisions, so single-server callers saw the admin override while list views saw the user server for the same name. getServerConfig now resolves the YAML/DB base first and applies the same four-step precedence used in getAllServerConfigs: 1. user-tier base wins absolutely over a config-tier candidate 2. healthy YAML/DB base wins over a failed (inspectionFailed) candidate 3. healthy candidate overlays its fields onto the base, preserving the base entry's source tag so downstream recovery routes to the correct storage location 4. with no base, the candidate is returned as-is for config-only servers readThroughCache still memoizes only the global YAML/DB lookup, so the per-call configServers candidate never enters the cache and the tenant-isolation guarantee from the previous fix is preserved. Regression tests cover the user-wins-over-config case and the YAML-overlay-with-yaml-source-preserved case. * โšก perf: Batch YAML Cache Read In ensureConfigServers isUnmodifiedYamlServer was calling cacheConfigsRepo.get(serverName) per entry. In the Redis aggregate-key backend, get() is implemented as getAll() then map lookup, so N concurrent per-server lookups inflate into N full-map reads and deserializations on every ensureConfigServers pass. The loop now takes a single getAll() snapshot at the top and hands it into a synchronous isUnmodifiedYamlServer helper, turning O(n) remote reads into O(1) regardless of how many MCP entries are resolved. The snapshot also gives the unchanged-YAML comparison one consistent view of YAML across all entries. Regression test spies on cacheConfigsRepo.get and asserts it is never called from ensureConfigServers, with getAll called exactly once. --- packages/api/src/admin/config.handler.spec.ts | 35 ++ packages/api/src/admin/config.ts | 3 + .../src/mcp/registry/MCPServersRegistry.ts | 167 +++++++-- .../__tests__/MCPServersRegistry.test.ts | 317 +++++++++++++++++- .../__tests__/ensureConfigServers.test.ts | 171 +++++++++- 5 files changed, 650 insertions(+), 43 deletions(-) diff --git a/packages/api/src/admin/config.handler.spec.ts b/packages/api/src/admin/config.handler.spec.ts index 8f4e2002a5..a052544d11 100644 --- a/packages/api/src/admin/config.handler.spec.ts +++ b/packages/api/src/admin/config.handler.spec.ts @@ -769,5 +769,40 @@ describe('createAdminConfigHandlers', () => { expect(res.statusCode).toBe(200); expect(res.body!.config).toEqual({ interface: { modelSelect: true } }); }); + + it('forwards baseOnly=true to getAppConfig when query param is the literal string "true"', async () => { + const getAppConfig = jest.fn().mockResolvedValue({ interface: { modelSelect: true } }); + const { handlers } = createHandlers({ getAppConfig }); + const req = mockReq({ query: { baseOnly: 'true' } }); + const res = mockRes(); + + await handlers.getBaseConfig(req, res); + + expect(res.statusCode).toBe(200); + expect(getAppConfig).toHaveBeenCalledWith(expect.objectContaining({ baseOnly: true })); + }); + + it('forwards baseOnly=false when the query param is missing, non-"true", or an array', async () => { + const cases: Array> = [ + {}, + { baseOnly: 'false' }, + { baseOnly: '1' }, + { baseOnly: ['true'] }, + { baseOnly: ['true', 'true'] }, + { baseOnly: { nested: 'true' } }, + ]; + + for (const query of cases) { + const getAppConfig = jest.fn().mockResolvedValue({ interface: { modelSelect: true } }); + const { handlers } = createHandlers({ getAppConfig }); + const req = mockReq({ query }); + const res = mockRes(); + + await handlers.getBaseConfig(req, res); + + expect(res.statusCode).toBe(200); + expect(getAppConfig).toHaveBeenCalledWith(expect.objectContaining({ baseOnly: false })); + } + }); }); }); diff --git a/packages/api/src/admin/config.ts b/packages/api/src/admin/config.ts index c1ce2cb13f..dc07a9d18f 100644 --- a/packages/api/src/admin/config.ts +++ b/packages/api/src/admin/config.ts @@ -108,6 +108,7 @@ export interface AdminConfigDeps { role?: string; userId?: string; tenantId?: string; + baseOnly?: boolean; }) => Promise; /** Invalidate all config-related caches after a mutation. */ invalidateConfigCaches?: (tenantId?: string) => Promise; @@ -211,8 +212,10 @@ export function createAdminConfigHandlers(deps: AdminConfigDeps) { return res.status(501).json({ error: 'Base config endpoint not configured' }); } + const baseOnly = (req.query as Record).baseOnly === 'true'; const appConfig = await getAppConfig({ tenantId: user.tenantId, + baseOnly, }); return res.status(200).json({ config: appConfig }); } catch (error) { diff --git a/packages/api/src/mcp/registry/MCPServersRegistry.ts b/packages/api/src/mcp/registry/MCPServersRegistry.ts index 81c59878ff..6c0e95b6ad 100644 --- a/packages/api/src/mcp/registry/MCPServersRegistry.ts +++ b/packages/api/src/mcp/registry/MCPServersRegistry.ts @@ -17,6 +17,62 @@ import { withTimeout } from '~/utils'; /** How long a failure stub is considered fresh before re-attempting inspection (5 minutes). */ const CONFIG_STUB_RETRY_MS = 5 * 60 * 1000; +/** + * Fields an admin override can legitimately set. Used to detect whether a + * resolved entry differs from its YAML base so unmodified YAML servers can + * skip lazy-init (avoids per-request inspect storms and prevents these + * servers from being cached in the config tier). + */ +const ADMIN_CONFIGURABLE_FIELDS = [ + 'type', + 'command', + 'args', + 'env', + 'stderr', + 'url', + 'headers', + 'proxy', + 'requiresOAuth', + 'apiKey', + 'oauth', + 'oauth_headers', + 'title', + 'description', + 'iconPath', + 'startup', + 'chatMenu', + 'serverInstructions', + 'customUserVars', + 'timeout', + 'sseReadTimeout', + 'initTimeout', +] as const; + +function deepEqual(a: unknown, b: unknown): boolean { + if (a === b) return true; + if (a == null || b == null) return a === b; + if (typeof a !== typeof b) return false; + if (typeof a !== 'object') return false; + if (Array.isArray(a)) { + if (!Array.isArray(b) || a.length !== b.length) return false; + for (let i = 0; i < a.length; i++) { + if (!deepEqual(a[i], b[i])) return false; + } + return true; + } + if (Array.isArray(b)) return false; + const aObj = a as Record; + const bObj = b as Record; + const aKeys = Object.keys(aObj); + const bKeys = Object.keys(bObj); + if (aKeys.length !== bKeys.length) return false; + for (const key of aKeys) { + if (!Object.prototype.hasOwnProperty.call(bObj, key)) return false; + if (!deepEqual(aObj[key], bObj[key])) return false; + } + return true; +} + const CONFIG_SERVER_INIT_TIMEOUT_MS = (() => { const raw = process.env.MCP_INIT_TIMEOUT_MS; if (raw == null) { @@ -133,38 +189,58 @@ export class MCPServersRegistry { } /** - * Returns the config for a single server. When `configServers` is provided, config-source - * servers are resolved from it directly (no global state, no cross-tenant race). + * Returns the config for a single server, mirroring the precedence used by + * getAllServerConfigs so list views and single-server lookups agree on + * the same name: + * 1. user-tier base entry wins absolutely over a config-tier candidate + * 2. healthy YAML/DB base wins over a failed (inspectionFailed) candidate + * 3. healthy candidate overlays its fields onto the base, preserving the + * base entry's source tag so downstream recovery routes correctly + * 4. with no base, the candidate is returned as-is (config-only server) + * + * readThroughCache memoizes only the global YAML/DB lookup; the per-call + * configServers candidate is tenant-scoped and is never cached, so a + * failed stub from one tenant can never satisfy a no-userId lookup from + * another. */ public async getServerConfig( serverName: string, userId?: string, configServers?: Record, ): Promise { - if (configServers?.[serverName]) { - return configServers[serverName]; - } + const candidate = configServers?.[serverName]; const cacheKey = this.getReadThroughCacheKey(serverName, userId); - + let base: t.ParsedServerConfig | undefined; if (await this.readThroughCache.has(cacheKey)) { - return await this.readThroughCache.get(cacheKey); + base = await this.readThroughCache.get(cacheKey); + } else { + const configFromYaml = await this.cacheConfigsRepo.get(serverName); + if (configFromYaml) { + base = configFromYaml; + } else { + base = await this.dbConfigsRepo.get(serverName, userId); + } + await this.readThroughCache.set(cacheKey, base); } - const configFromYaml = await this.cacheConfigsRepo.get(serverName); - if (configFromYaml) { - await this.readThroughCache.set(cacheKey, configFromYaml); - return configFromYaml; - } - - const configFromDB = await this.dbConfigsRepo.get(serverName, userId); - await this.readThroughCache.set(cacheKey, configFromDB); - return configFromDB; + if (!candidate) return base; + if (base?.source === 'user') return base; + if (candidate.inspectionFailed) return base ?? candidate; + return base ? { ...candidate, source: base.source } : candidate; } /** - * Returns all server configs visible to the given user. - * Operator-managed servers (YAML + Config) override User DB servers on name collisions. + * Returns the full server config map after merging YAML cache, Config-tier overrides, + * and User-DB entries. + * + * Precedence (lowest to highest): YAML cache > Config-tier overrides (success only) > User DB. + * Two guards keep the merge safe: + * 1. Config-tier entries carrying `inspectionFailed: true` never overlay an existing + * base entry; the healthy base is preserved for the duration of the retry window. + * 2. User-DB entries (`source: 'user'`) are never replaced by Config-tier overlays. + * On a successful overlay the base entry's `source` field is preserved so downstream + * recovery logic routes to the correct storage location. */ public async getAllServerConfigs( userId?: string, @@ -175,8 +251,17 @@ export class MCPServersRegistry { return this.getBaseServerConfigs(userId, role); } const base = await this.getBaseServerConfigs(userId, role); - this.warnOnOperatorManagedNameCollisions(configServers, base, 'Config'); - return { ...base, ...configServers }; + const result: Record = { ...base }; + for (const [name, override] of Object.entries(configServers)) { + if (result[name]?.source === 'user') { + logger.debug(`[MCP][config][${name}] Admin override shadowed by user-tier entry`); + continue; + } + if (override.inspectionFailed && result[name]) continue; + const baseSource = result[name]?.source; + result[name] = baseSource ? { ...override, source: baseSource } : override; + } + return result; } /** @@ -393,19 +478,16 @@ export class MCPServersRegistry { return {}; } - const yamlNames = await this.getYamlServerNames(); - const configServerEntries = Object.entries(resolvedMcpConfig).filter( - ([name]) => !yamlNames.has(name), - ); - - if (configServerEntries.length === 0) { - return {}; - } - const result: Record = {}; + /** Single snapshot of the YAML cache for the whole pass: in the Redis aggregate-key backend, every per-name get() reads and deserializes the full map, so N concurrent per-server lookups would issue N full-map reads. The snapshot also keeps the unchanged-YAML comparison consistent against one view of YAML across all entries. */ + const yamlSnapshot = await this.cacheConfigsRepo.getAll(); + const settled = await Promise.allSettled( - configServerEntries.map(async ([serverName, rawConfig]) => { + Object.entries(resolvedMcpConfig).map(async ([serverName, rawConfig]) => { + if (this.isUnmodifiedYamlServer(yamlSnapshot, serverName, rawConfig)) { + return; + } const parsed = await this.ensureSingleConfigServer(serverName, rawConfig); if (parsed) { result[serverName] = parsed; @@ -421,6 +503,31 @@ export class MCPServersRegistry { return result; } + /** + * Returns true when `rawConfig` matches the YAML cache entry for this server + * on every admin-configurable field, so an unmodified YAML-defined server + * can skip lazy-init and avoid being re-inspected or shadowed in the + * config tier. + */ + private isUnmodifiedYamlServer( + yamlSnapshot: Record, + serverName: string, + rawConfig: t.MCPOptions, + ): boolean { + const yamlEntry = yamlSnapshot[serverName]; + if (!yamlEntry || yamlEntry.source !== 'yaml') { + return false; + } + const yamlRecord = yamlEntry as unknown as Record; + const rawRecord = rawConfig as unknown as Record; + /** rawConfig is the pre-inspection MCPOptions; absent fields mean the admin didn't override and shouldn't count as a diff against inspector-derived values on the cached YAML entry. */ + return ADMIN_CONFIGURABLE_FIELDS.every((field) => { + const rawVal = rawRecord[field]; + if (rawVal === undefined) return true; + return deepEqual(yamlRecord[field], rawVal); + }); + } + /** * Ensures a single config-source server is initialized. * Cache key is scoped by config hash to prevent cross-tenant poisoning. diff --git a/packages/api/src/mcp/registry/__tests__/MCPServersRegistry.test.ts b/packages/api/src/mcp/registry/__tests__/MCPServersRegistry.test.ts index a1298dcd6e..b1a86864b0 100644 --- a/packages/api/src/mcp/registry/__tests__/MCPServersRegistry.test.ts +++ b/packages/api/src/mcp/registry/__tests__/MCPServersRegistry.test.ts @@ -141,7 +141,7 @@ describe('MCPServersRegistry', () => { } }); - it('should warn when config servers shadow DB servers', async () => { + it('should preserve the user-tier entry over a config-tier override on the same name without emitting a misleading shadow warning', async () => { const warnSpy = jest.spyOn(logger, 'warn').mockImplementation(); const configServer = { ...testParsedConfig, @@ -152,10 +152,11 @@ describe('MCPServersRegistry', () => { jest.spyOn(registry['dbConfigsRepo'], 'getAll').mockResolvedValue({ slack: dbConfig }); try { - await registry.getAllServerConfigs('user-1', { slack: configServer }); + const result = await registry.getAllServerConfigs('user-1', { slack: configServer }); - expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('Config MCP server')); - expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('slack')); + expect(result.slack.source).toBe('user'); + expect(result.slack.title).toBe('User Slack'); + expect(warnSpy).not.toHaveBeenCalledWith(expect.stringContaining('Config MCP server')); } finally { warnSpy.mockRestore(); } @@ -402,4 +403,312 @@ describe('MCPServersRegistry', () => { }); }); }); + + describe('admin-panel overrides for YAML-defined servers', () => { + const yamlLangfuseConfig = Object.freeze({ + type: 'streamable-http', + url: 'https://langfuse.com/api/public/mcp', + requiresOAuth: false, + source: 'yaml', + updatedAt: FIXED_TIME, + }) as t.ParsedServerConfig; + + it('flows config-tier override on a YAML-defined server through to getAllServerConfigs', async () => { + await registry['cacheConfigsRepo'].add('langfuse-docs', yamlLangfuseConfig); + + const overrideRawConfig: t.MCPOptions = { + type: 'streamable-http', + url: 'https://langfuse.com/api/public/mcp', + requiresOAuth: false, + iconPath: 'https://example.com/icon.svg', + }; + + const configServers = await registry.ensureConfigServers({ + 'langfuse-docs': overrideRawConfig, + }); + + expect(configServers['langfuse-docs']).toBeDefined(); + expect(configServers['langfuse-docs'].iconPath).toBe('https://example.com/icon.svg'); + + const result = await registry.getAllServerConfigs('user-1', configServers); + + expect(result['langfuse-docs']).toBeDefined(); + expect(result['langfuse-docs'].iconPath).toBe('https://example.com/icon.svg'); + expect(result['langfuse-docs'].source).toBe('yaml'); + }); + + it('preserves user-DB tier (source: "user") over config-tier overrides', async () => { + const userDbEntry: t.ParsedServerConfig = { + type: 'streamable-http', + url: 'https://user-defined.example.com/mcp', + requiresOAuth: false, + source: 'user', + dbId: 'user-db-id-123', + updatedAt: FIXED_TIME, + }; + + jest + .spyOn(registry['dbConfigsRepo'], 'getAll') + .mockResolvedValue({ 'shared-server': userDbEntry }); + + const configTierOverride: t.ParsedServerConfig = { + type: 'streamable-http', + url: 'https://admin-override.example.com/mcp', + requiresOAuth: false, + source: 'config', + iconPath: 'https://example.com/admin-icon.svg', + updatedAt: FIXED_TIME, + }; + + const result = await registry.getAllServerConfigs('user-1', { + 'shared-server': configTierOverride, + }); + + expect(result['shared-server']).toBeDefined(); + expect(result['shared-server'].source).toBe('user'); + expect(result['shared-server'].url).toBe('https://user-defined.example.com/mcp'); + expect(result['shared-server'].dbId).toBe('user-db-id-123'); + }); + + it('still runs lazy-init for pure config-tier servers not present in YAML', async () => { + const configOnlyRawConfig: t.MCPOptions = { + type: 'streamable-http', + url: 'https://config-only.example.com/mcp', + requiresOAuth: false, + iconPath: 'https://example.com/config-only-icon.svg', + }; + + const inspectSpy = jest.spyOn(MCPServerInspector, 'inspect'); + inspectSpy.mockClear(); + + const result = await registry.ensureConfigServers({ + 'config-only-server': configOnlyRawConfig, + }); + + expect(inspectSpy).toHaveBeenCalledTimes(1); + expect(inspectSpy).toHaveBeenCalledWith( + 'config-only-server', + configOnlyRawConfig, + undefined, + undefined, + undefined, + ); + expect(result['config-only-server']).toBeDefined(); + expect(result['config-only-server'].iconPath).toBe( + 'https://example.com/config-only-icon.svg', + ); + expect(result['config-only-server'].source).toBe('config'); + }); + + it('preserves YAML base entry when config-tier override reports inspectionFailed', async () => { + const yamlSeed: t.ParsedServerConfig = { + type: 'streamable-http', + url: 'https://langfuse.com/api/public/mcp', + requiresOAuth: false, + headers: { Authorization: 'Bearer yaml-token' }, + source: 'yaml', + tools: 'yaml_tool_a, yaml_tool_b', + capabilities: '{"tools":{"listChanged":true}}', + updatedAt: FIXED_TIME, + }; + await registry['cacheConfigsRepo'].add('langfuse-docs', yamlSeed); + + const failedOverride: t.ParsedServerConfig = { + type: 'streamable-http', + url: 'https://langfuse.com/api/public/mcp', + requiresOAuth: false, + source: 'config', + inspectionFailed: true, + updatedAt: FIXED_TIME, + }; + + const result = await registry.getAllServerConfigs('user-1', { + 'langfuse-docs': failedOverride, + }); + + expect(result['langfuse-docs']).toBeDefined(); + expect(result['langfuse-docs'].source).toBe('yaml'); + expect(result['langfuse-docs'].inspectionFailed).toBeUndefined(); + expect(result['langfuse-docs'].tools).toBe('yaml_tool_a, yaml_tool_b'); + expect(result['langfuse-docs'].capabilities).toBe('{"tools":{"listChanged":true}}'); + expect((result['langfuse-docs'] as t.StreamableHTTPOptions).headers).toEqual({ + Authorization: 'Bearer yaml-token', + }); + }); + + it('healthy YAML entry survives end-to-end when inspect throws for YAML server', async () => { + await registry['cacheConfigsRepo'].add('langfuse-docs', yamlLangfuseConfig); + const inspectSpy = jest.spyOn(MCPServerInspector, 'inspect'); + inspectSpy.mockClear(); + inspectSpy.mockRejectedValueOnce(new Error('network timeout')); + + const configServers = await registry.ensureConfigServers({ + 'langfuse-docs': { + type: 'streamable-http', + url: 'https://langfuse.com/api/public/mcp', + iconPath: 'https://example.com/icon.svg', + } as t.MCPOptions, + }); + const result = await registry.getAllServerConfigs('user-1', configServers); + + expect(result['langfuse-docs'].source).toBe('yaml'); + expect(result['langfuse-docs'].inspectionFailed).toBeUndefined(); + expect(result['langfuse-docs'].url).toBe('https://langfuse.com/api/public/mcp'); + }); + + it('preserves YAML source tag when config-tier override succeeds', async () => { + const yamlSeed: t.ParsedServerConfig = { + type: 'streamable-http', + url: 'https://langfuse.com/api/public/mcp', + requiresOAuth: false, + source: 'yaml', + updatedAt: FIXED_TIME, + }; + await registry['cacheConfigsRepo'].add('langfuse-docs', yamlSeed); + + const successfulOverride: t.ParsedServerConfig = { + type: 'streamable-http', + url: 'https://langfuse.com/api/public/mcp', + requiresOAuth: false, + source: 'config', + iconPath: 'https://example.com/icon.svg', + updatedAt: FIXED_TIME, + }; + + const result = await registry.getAllServerConfigs('user-1', { + 'langfuse-docs': successfulOverride, + }); + + expect(result['langfuse-docs']).toBeDefined(); + expect(result['langfuse-docs'].source).toBe('yaml'); + expect(result['langfuse-docs'].iconPath).toBe('https://example.com/icon.svg'); + }); + + it('skips lazy-init for YAML server with no admin override', async () => { + const yamlSeed: t.ParsedServerConfig = { + type: 'streamable-http', + url: 'https://langfuse.com/api/public/mcp', + requiresOAuth: false, + source: 'yaml', + updatedAt: FIXED_TIME, + }; + await registry['cacheConfigsRepo'].add('langfuse-docs', yamlSeed); + + const yamlRawConfig: t.MCPOptions = { + type: 'streamable-http', + url: 'https://langfuse.com/api/public/mcp', + requiresOAuth: false, + }; + + const inspectSpy = jest.spyOn(MCPServerInspector, 'inspect'); + inspectSpy.mockClear(); + + const result = await registry.ensureConfigServers({ + 'langfuse-docs': yamlRawConfig, + }); + + expect(inspectSpy).not.toHaveBeenCalled(); + expect(result['langfuse-docs']).toBeUndefined(); + }); + + it('runs lazy-init for YAML server with admin override', async () => { + const yamlSeed: t.ParsedServerConfig = { + type: 'streamable-http', + url: 'https://langfuse.com/api/public/mcp', + requiresOAuth: false, + source: 'yaml', + updatedAt: FIXED_TIME, + }; + await registry['cacheConfigsRepo'].add('langfuse-docs', yamlSeed); + + const overrideRawConfig: t.MCPOptions = { + type: 'streamable-http', + url: 'https://langfuse.com/api/public/mcp', + requiresOAuth: false, + iconPath: 'https://x.com/icon.svg', + }; + + const inspectSpy = jest.spyOn(MCPServerInspector, 'inspect'); + inspectSpy.mockClear(); + + await registry.ensureConfigServers({ + 'langfuse-docs': overrideRawConfig, + }); + + expect(inspectSpy).toHaveBeenCalledTimes(1); + }); + + it('getServerConfig falls through to YAML on failure stub', async () => { + const yamlSeed: t.ParsedServerConfig = { + type: 'streamable-http', + url: 'https://langfuse.com/api/public/mcp', + requiresOAuth: false, + source: 'yaml', + tools: 'yaml_tool_a', + updatedAt: FIXED_TIME, + }; + await registry['cacheConfigsRepo'].add('langfuse-docs', yamlSeed); + + const failureStub: t.ParsedServerConfig = { + type: 'streamable-http', + url: 'https://langfuse.com/api/public/mcp', + requiresOAuth: false, + source: 'config', + inspectionFailed: true, + updatedAt: FIXED_TIME, + }; + + const result = await registry.getServerConfig('langfuse-docs', 'user-1', { + 'langfuse-docs': failureStub, + }); + + expect(result).toBeDefined(); + expect(result?.source).toBe('yaml'); + expect(result?.inspectionFailed).toBeUndefined(); + expect(result?.tools).toBe('yaml_tool_a'); + }); + + it('passes a fully merged config to lazy-init when override only adds new fields', async () => { + const yamlSeed: t.ParsedServerConfig = { + type: 'streamable-http', + url: 'https://langfuse.com/api/public/mcp', + requiresOAuth: false, + headers: { Authorization: 'Bearer yaml-token' }, + source: 'yaml', + updatedAt: FIXED_TIME, + }; + await registry['cacheConfigsRepo'].add('langfuse-docs', yamlSeed); + + const mergedRawConfig: t.MCPOptions = { + type: 'streamable-http', + url: 'https://langfuse.com/api/public/mcp', + requiresOAuth: false, + headers: { Authorization: 'Bearer yaml-token' }, + iconPath: 'https://example.com/icon.svg', + }; + + const inspectSpy = jest.spyOn(MCPServerInspector, 'inspect'); + inspectSpy.mockClear(); + + const result = await registry.ensureConfigServers({ + 'langfuse-docs': mergedRawConfig, + }); + + expect(inspectSpy).toHaveBeenCalledTimes(1); + const firstCallArgs = inspectSpy.mock.calls[0]; + expect(firstCallArgs[0]).toBe('langfuse-docs'); + const passedConfig = firstCallArgs[1] as t.StreamableHTTPOptions & { + iconPath?: string; + requiresOAuth?: boolean; + }; + expect(passedConfig.type).toBe('streamable-http'); + expect(passedConfig.url).toBe('https://langfuse.com/api/public/mcp'); + expect(passedConfig.requiresOAuth).toBe(false); + expect(passedConfig.headers).toEqual({ Authorization: 'Bearer yaml-token' }); + expect(passedConfig.iconPath).toBe('https://example.com/icon.svg'); + + expect(result['langfuse-docs']).toBeDefined(); + expect(result['langfuse-docs'].iconPath).toBe('https://example.com/icon.svg'); + }); + }); }); diff --git a/packages/api/src/mcp/registry/__tests__/ensureConfigServers.test.ts b/packages/api/src/mcp/registry/__tests__/ensureConfigServers.test.ts index 9b601dcee2..5db4218a27 100644 --- a/packages/api/src/mcp/registry/__tests__/ensureConfigServers.test.ts +++ b/packages/api/src/mcp/registry/__tests__/ensureConfigServers.test.ts @@ -22,24 +22,21 @@ const mockMongoose = {} as typeof import('mongoose'); const sseConfig: t.MCPOptions = { type: 'sse', url: 'https://mcp.example.com/sse', -} as unknown as t.MCPOptions; +}; const altSseConfig: t.MCPOptions = { type: 'sse', url: 'https://mcp.other-tenant.com/sse', -} as unknown as t.MCPOptions; +}; const yamlConfig: t.MCPOptions = { type: 'stdio', command: 'node', args: ['tools.js'], -} as unknown as t.MCPOptions; +}; function makeParsedConfig(overrides: Partial = {}): t.ParsedServerConfig { return { - type: 'sse', - url: 'https://mcp.example.com/sse', - requiresOAuth: false, tools: 'tool_a, tool_b', capabilities: '{}', initDuration: 42, @@ -91,8 +88,9 @@ describe('MCPServersRegistry โ€” ensureConfigServers', () => { ).toEqual({}); }); - it('should exclude YAML servers from config-source detection', async () => { + it('should skip unchanged YAML-named servers but still process config-only servers', async () => { await registry.addServer('yaml_server', yamlConfig, 'CACHE'); + inspectSpy.mockClear(); const result = await registry.ensureConfigServers({ yaml_server: yamlConfig, @@ -101,9 +99,17 @@ describe('MCPServersRegistry โ€” ensureConfigServers', () => { expect(result).toHaveProperty('config_server'); expect(result).not.toHaveProperty('yaml_server'); + expect(inspectSpy).toHaveBeenCalledTimes(1); + expect(inspectSpy).toHaveBeenCalledWith( + 'config_server', + sseConfig, + undefined, + undefined, + undefined, + ); }); - it('should return empty when all servers are YAML', async () => { + it('should skip lazy-init for YAML-named servers with no admin override', async () => { await registry.addServer('yaml_a', yamlConfig, 'CACHE'); await registry.addServer('yaml_b', yamlConfig, 'CACHE'); inspectSpy.mockClear(); @@ -113,10 +119,74 @@ describe('MCPServersRegistry โ€” ensureConfigServers', () => { yaml_b: yamlConfig, }); - expect(result).toEqual({}); + expect(result).not.toHaveProperty('yaml_a'); + expect(result).not.toHaveProperty('yaml_b'); expect(inspectSpy).not.toHaveBeenCalled(); }); + it('should lazy-init YAML-named server when admin override changes a field', async () => { + await registry.addServer('yaml_a', yamlConfig, 'CACHE'); + inspectSpy.mockClear(); + + const overrideConfig: t.MCPOptions = { ...yamlConfig, iconPath: 'https://x.com/icon.svg' }; + const result = await registry.ensureConfigServers({ + yaml_a: overrideConfig, + }); + + expect(result).toHaveProperty('yaml_a'); + expect(inspectSpy).toHaveBeenCalledTimes(1); + }); + + it('should lazy-init YAML server when admin overrides only the proxy field', async () => { + await registry.addServer('yaml_remote', sseConfig, 'CACHE'); + inspectSpy.mockClear(); + + const overrideConfig: t.MCPOptions = { + ...sseConfig, + proxy: 'http://proxy.example.com:8080', + } as t.MCPOptions; + const result = await registry.ensureConfigServers({ + yaml_remote: overrideConfig, + }); + + expect(result).toHaveProperty('yaml_remote'); + expect(inspectSpy).toHaveBeenCalledTimes(1); + }); + + it('should not re-init YAML server when only the difference is an inspector-derived field absent from rawConfig', async () => { + const yamlWithInferred: t.MCPOptions = { + ...sseConfig, + requiresOAuth: false, + } as t.MCPOptions; + await registry.addServer('yaml_remote', yamlWithInferred, 'CACHE'); + inspectSpy.mockClear(); + + const result = await registry.ensureConfigServers({ + yaml_remote: sseConfig, + }); + + expect(result).not.toHaveProperty('yaml_remote'); + expect(inspectSpy).not.toHaveBeenCalled(); + }); + + it('should issue a single batched YAML cache read regardless of how many servers are checked', async () => { + await registry.addServer('yaml_a', yamlConfig, 'CACHE'); + await registry.addServer('yaml_b', yamlConfig, 'CACHE'); + await registry.addServer('yaml_c', yamlConfig, 'CACHE'); + const cacheGetSpy = jest.spyOn(registry['cacheConfigsRepo'], 'get'); + const cacheGetAllSpy = jest.spyOn(registry['cacheConfigsRepo'], 'getAll'); + + await registry.ensureConfigServers({ + yaml_a: yamlConfig, + yaml_b: yamlConfig, + yaml_c: yamlConfig, + config_only: sseConfig, + }); + + expect(cacheGetSpy).not.toHaveBeenCalled(); + expect(cacheGetAllSpy).toHaveBeenCalledTimes(1); + }); + it('should lazy-initialize a config-source server and tag source as config', async () => { const result = await registry.ensureConfigServers({ my_server: sseConfig }); @@ -305,6 +375,89 @@ describe('MCPServersRegistry โ€” ensureConfigServers', () => { expect(config?.source).toBe('config'); }); + it('should surface inspectionFailed stub for config-only server when no YAML/DB fallback exists', async () => { + inspectSpy.mockRejectedValueOnce(new Error('connection refused')); + const configServers = await registry.ensureConfigServers({ bad_config_only: sseConfig }); + expect(configServers.bad_config_only.inspectionFailed).toBe(true); + + const config = await registry.getServerConfig('bad_config_only', undefined, configServers); + expect(config).toBeDefined(); + expect(config?.inspectionFailed).toBe(true); + expect(config?.source).toBe('config'); + }); + + it('should prefer healthy YAML entry over inspectionFailed config-tier stub on the same name', async () => { + await registry.addServer('shared_name', yamlConfig, 'CACHE'); + + inspectSpy.mockRejectedValueOnce(new Error('connection refused')); + const configServers = await registry.ensureConfigServers({ shared_name: sseConfig }); + expect(configServers.shared_name.inspectionFailed).toBe(true); + + const config = await registry.getServerConfig('shared_name', undefined, configServers); + expect(config).toBeDefined(); + expect(config?.inspectionFailed).toBeUndefined(); + expect(config?.source).toBe('yaml'); + }); + + it('should prefer the user-tier DB entry over a config-tier candidate on the same name', async () => { + const dbConfig = makeParsedConfig({ + source: 'user', + title: 'User Slack', + } as Partial); + jest.spyOn(registry['dbConfigsRepo'], 'get').mockResolvedValue(dbConfig); + + const configCandidate = makeParsedConfig({ + source: 'config', + title: 'Config Slack', + } as Partial); + + const result = await registry.getServerConfig('slack', 'user-1', { + slack: configCandidate, + }); + expect(result?.source).toBe('user'); + expect((result as unknown as { title: string }).title).toBe('User Slack'); + }); + + it('should overlay healthy admin override fields onto a YAML base while preserving yaml source', async () => { + const yamlEntry = makeParsedConfig({ + ...sseConfig, + source: 'yaml', + title: 'YAML Title', + } as unknown as Partial); + await registry['cacheConfigsRepo'].add('shared', yamlEntry); + + const adminOverride = makeParsedConfig({ + ...sseConfig, + source: 'config', + title: 'Admin Title', + } as unknown as Partial); + + const result = await registry.getServerConfig('shared', undefined, { + shared: adminOverride, + }); + expect(result?.source).toBe('yaml'); + expect((result as unknown as { title: string }).title).toBe('Admin Title'); + }); + + it('should not leak a tenant-scoped failure stub into subsequent no-configServers calls', async () => { + inspectSpy.mockRejectedValueOnce(new Error('connection refused')); + const tenantA = await registry.ensureConfigServers({ tenant_a_only: sseConfig }); + expect(tenantA.tenant_a_only.inspectionFailed).toBe(true); + + const firstLookup = await registry.getServerConfig('tenant_a_only', undefined, tenantA); + expect(firstLookup?.inspectionFailed).toBe(true); + expect(firstLookup?.source).toBe('config'); + + const leakedLookup = await registry.getServerConfig('tenant_a_only'); + expect(leakedLookup).toBeUndefined(); + + const tenantB = await registry.ensureConfigServers({ tenant_a_only: altSseConfig }); + const tenantBLookup = await registry.getServerConfig('tenant_a_only', undefined, tenantB); + expect((tenantBLookup as unknown as { url: string }).url).toBe( + 'https://mcp.other-tenant.com/sse', + ); + }); + it('should not cross-contaminate between tenant configServers maps', async () => { const tenantA = await registry.ensureConfigServers({ srv: sseConfig }); const tenantB = await registry.ensureConfigServers({ srv: altSseConfig });