mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-08-04 06:52:47 +00:00
2bcf3e8582
Some checks are pending
Docker Dev Branch Images Build / build (Dockerfile, lc-dev, node) (push) Waiting to run
Docker Dev Branch Images Build / build (Dockerfile.multi, lc-dev-api, api-build) (push) Waiting to run
GitNexus Index / index (push) Waiting to run
GitNexus Index / post-index (push) Blocked by required conditions
* 🪟 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. |
||
|---|---|---|
| .. | ||
| src | ||
| types | ||
| .gitignore | ||
| babel.config.cjs | ||
| jest.config.mjs | ||
| jest.setup.cjs | ||
| package.json | ||
| rollup.config.js | ||
| tsconfig-paths-bootstrap.mjs | ||
| tsconfig.build.json | ||
| tsconfig.json | ||
| tsconfig.spec.json | ||