🛡️ feat: Lock YAML-Defined MCP Servers In Admin Panel#63
Conversation
Detects servers that come from the YAML config by reading the un-merged base config keys threaded from getBaseConfigFn's new yamlMcpKeys signal, so admin overrides on cosmetic fields like title or iconPath no longer flip the detection and re-enable rename or delete on YAML-defined servers. For YAML-source servers the rename pencil and trash button are hidden and the transport-type selector is disabled because changing it breaks the Zod union. All other fields including url, command, args, env, stderr, headers, apiKey, and oauth remain editable to support the multi-tenant model where YAML provides global defaults and the admin panel is the per-tenant customization surface. Inspector-derived fields are hidden from the editor since admin overrides of those are silently ignored at the registry layer. MCP entry edits now write per-leaf paths under mcpServers.<name>.<field> so single-field reset, baseline-equality dirty detection, and rename orphan cleanup all work correctly through the existing global Save bar. The rename path enumerates leaves recursively so nested per-leaf data like headers and oauth survive rename intact. Dotted server names are rejected at create and rename time because the path model treats dots as delimiters, and a custom anyOfSchema wrapper resolves Zod union sub-schemas during per-field validation without relying on a specific zod major version.
dustinhealy
force-pushed
the
fix/mcp-per-entry-save
branch
from
deb88a2 to
2a1ba73
Compare
Replace the inline danger banner in McpServersRenderer with the same toast channel ConfigPage uses for save errors. ConfigTabContent forwards an optional onValidationError prop down to record-type renderers, ConfigPage wires it to showToast, and the renderer fires toasts instead of holding local error state. Dot, prototype, and existing-name guards remain to prevent dotted names from corrupting the per-leaf write path on save.
Drop file-header docblocks, section dividers, and JSDocs that restated what well-named identifiers already say. Keep only the WHY comments that explain non-obvious behavior: MongoDB \$unset subtree collapse, zod v3/v4 cross-version anyOfSchema, container-delete prune bypass, ref-stable callbacks for memo bail, and YAML-source detection fallback. 171 added comment lines down to 21.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b3a1cf4b80
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
anyOfSchema now keeps its candidate options on _def so resolveSubSchema can keep walking when a path crosses more than one union segment; the union walker also recurses into each candidate so records, arrays, and nested unions resolve through their own case instead of relying on shape lookup. Without this, validateFieldValue silently returned success for any leaf reached through a multi-candidate union. editsByEntry now resolves the entry key by longest-prefix match against baseRecord keys, so legacy server names that already contain dots stay attributed to their real entry rather than getting carved up at the first dot. New names are still blocked from containing dots at create and rename time. Regression tests cover the through-union nested-field path in config.test.ts and the dotted legacy entry editsByEntry plus remove behavior in McpServersRenderer.test.tsx.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a083be3b2c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
A per-leaf write under a dotted server name (mcpServers.legacy.dotted.url
for entry "legacy.dotted") is indistinguishable on the save side from a
nested mcpServers.legacy.dotted.url path, because the fields PATCH
endpoint parses fieldPath as dot-delimited segments. Pre-existing
dotted entries previously survived because the renderer saved the
whole mcpServers record; per-leaf granularity broke that.
McpEntryRow now flags entryKey.includes('.') as a dotted-legacy entry
and renders it disabled with rename and delete hidden. The entry
remains visible so admins know it exists and can fix it via YAML or
the underlying datastore. entryOnChange and handleWholeEntryChange
short-circuit for dotted keys, and handleRemove and handleRename
add the same guard as defense-in-depth so no programmatic caller
can emit a colliding fieldPath.
Regression tests cover the disabled-input render, missing rename and
delete buttons, and the no-onChange-writes invariant.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f29f22734
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The record useMemo walked baseRecord first then appended every edited entry, so changing a single field bumped its row to the bottom of the list. The merged loop now walks baseRecord and applies the edits overlay in place, with newly-created entries appended in their editsByEntry insertion order. baselineContainerPaths walks the structured config to record every container path (including orphaned mcpServers.foo with no leaves under it). The dirty check now treats an entry-path undefined as a container delete when either the leaf-derived intermediates or the structured container set knows about it, so an orphaned empty entry can actually be removed instead of being pruned as a no-op write. Regression tests cover the in-place ordering for an edited entry, the bottom-append ordering for a freshly-created entry, and the existing handleRemove flow continues to assert leaf cleanup for non-empty entries.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f0f6712b53
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The prune branch in handleFieldChange dropped only the current leaf path from editedValues when its value returned to baseline, leaving any ancestor `undefined` write in place. After a delete-then-recreate sequence where every recreated field happens to match its baseline, the leftover `mcpServers.<name>: undefined` survived and the record overlay continued to hide the resurrected entry. The prune branch now mirrors the dedup loop in the non-prune branch: on a baseline-match drop, any ancestor entry whose value is `undefined` is removed too. Non-undefined ancestors are untouched so explicit subtree overrides keep their semantics.
A LibreChat without the companion baseOnly support returns the same merged config payload for both /api/admin/config/base and the ?baseOnly=true variant. The previous code treated every key from the baseOnly response as YAML-defined, which on legacy backends locked admin-only servers against rename and delete. getBaseConfigFn now checks dbOverrides.mcpServers first. When admin overrides on mcpServers exist, the baseOnly response is only treated as authoritative if it differs from the merged config; when the two are byte-identical despite known overrides, the backend is legacy and yamlMcpKeys stays undefined so the renderer falls back to its safer "lock nothing" path. When no admin overrides exist on mcpServers the two responses are legitimately identical, so baseOnly is used unchanged and YAML servers remain locked.
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
The non-prune dedup loop only cleared ancestor entries (existing keys that path descends from). A subsequent parent-level write (e.g. an edit to mcpServers.bar.headers as a whole) left previously-emitted per-leaf writes like mcpServers.bar.headers.Authorization in editedValues. Both reached the field PATCH endpoint as separate entries and raced order-dependently, letting the stale leaf overwrite the user's parent edit. The loop now also removes descendants of the current path so a write at any subtree level supersedes any deeper entries beneath it.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 51a2001. Configure here.
handleCreate and handleRename listed onValidationError and localize in their useCallback dependency arrays. ConfigPage passes onValidationError as an inline arrow each render, and useLocalize returns a fresh function reference each render, so the callbacks were recreated on every parent re-render. That defeated memo(McpEntryRow) because the onRename prop changed identity every cycle and every row re-rendered even when its data was untouched. Mirror the existing editedValuesRef / baseRecordRef / recordRef pattern with refs for both onValidationError and localize, then drop those unstable values from the callback dep lists. handleCreate, handleRemove, and handleRename are now stable as long as onChange and path are stable, which they are.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dfdde5d18e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Per-leaf save granularity bypasses the whole-object Zod validation that previously caught missing required siblings, so an admin could pick transport sse and submit without a url; the resulting per-field PATCH succeeded because type=sse is a valid enum on its own and no sibling check ran on the server. The persisted entry was invalid for its transport but stuck around. CreateMcpServerDialog.handleSubmit now consults REQUIRED_BY_TRANSPORT for the effective transport (http normalized to streamable-http) and surfaces a missing-required-field error inline before onSave runs. The dialog's submit-disabled invariant already covers the always- required type field; this adds the transport-specific sibling check so a partial create never reaches the field-PATCH endpoint.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22e211e671
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!seen.has(entryPath)) { | ||
| onChange(entryPath, undefined); |
There was a problem hiding this comment.
When editing a role/group scope, deleting a non-YAML MCP server that is inherited from the base config now records mcpServers.<key> as undefined; handleConfirmSave treats that as a reset and calls the scope DELETE endpoint, which only removes an existing scope override. If the entry exists only in the base config, the DELETE is a no-op and the server reappears after save (renames have the same old-key cleanup problem, leaving the inherited entry plus the new copy). This needs a scope-level replacement/tombstone instead of a reset for inherited entries.
Useful? React with 👍 / 👎.
The scope-edit save path treats an undefined field write as a reset that calls the profile DELETE endpoint, which can only remove an existing scope override. Deleting an inherited base entry from a role/group scope therefore left it visible after save because no scope override existed to remove; renaming had the same problem and created a parallel scope entry alongside the surviving inherited original. Per-leaf saves cannot express a scope-level tombstone, so the proper fix needs backend support that does not exist yet. Add a scopeMode prop on FieldRendererProps and ConfigTabContentProps, plumb isEditingScope through ConfigPage so the MCP renderer knows when it is editing a scope, and treat scopeMode the same as isYamlSource / isDottedLegacy for the identity lock: the create button is disabled, an inline note explains the limitation, and McpEntryRow hides rename and delete on every entry. Field edits still produce per-leaf scope overrides as expected. Regression tests cover the create-disabled state and the absence of rename/delete affordances in scope mode, plus that a field edit still emits a per-leaf write so the user can still customize values per scope.
2 participants
Summary
Admins editing MCP server entries from the panel could rename or delete YAML-defined servers, creating duplicate entries (because the YAML still declared the original) and breaking the silent-drop fix that just shipped to LibreChat. They could also lose nested data on rename, and single-field resets were silently dropped because the per-entry write strategy collided with the global Save bar's baseline-equality dirty pruning.
What was happening
The renderer wrote MCP edits as whole-entry blobs at
mcpServers.<name>, which (a) broke single-field reset because the path didn't match how flatBaseline stores leaves, (b) made rename orphan cleanup miss descendant edits, and (c) carried inspector-derived fields liketoolsandcapabilitiesinto the save payload even though the LibreChat registry ignores them on the way back in.YAML detection used an absence-of-override heuristic that produced false positives on YAML servers with any admin override on cosmetic fields like
titleoriconPath, re-enabling rename and delete on those.Fix
Detection now reads the un-merged YAML key set directly from a new
getBaseConfigFnchannel that calls LibreChat's/api/admin/config/basewith?baseOnly=true(companion PR danny-avila/LibreChat#13173). Identity is locked iff the name appears in that set, regardless of how many fields the admin has since overridden.For YAML-source servers the rename pencil and trash button are hidden, the transport-type selector is disabled because changing it breaks the Zod union, and inspector-derived fields are hidden from the editor. All other fields including url, command, args, env, stderr, headers, apiKey, and oauth remain editable to support the per-tenant deployment model where YAML provides global defaults and the admin panel is the per-tenant customization surface.
MCP entry edits now write per-leaf paths under
mcpServers.<name>.<field>through every code path (create, edit, remove, rename). The rename path enumerates leaves recursively so nested data likeheaders.Authorizationsurvives rename intact. Dotted,__proto__,constructor, andprototypenames are rejected at create and rename time. A custom anyOfSchema wrapper resolves Zod union sub-schemas during per-field validation so a streamable-http server'stypefield no longer fails union resolution against an unrelated Stdio branch literal.Testing
bun run testpasses 577/577 across 14 suites (18 new inMcpServersRenderer.test.tsxcovering per-leaf writes, YAML detection, the lock matrix, dotted-name rejection, rename-with-nested-data, remove-with-nested-data, create-then-edit-then-rename, and the yamlBaseKeys-absent fallback).Manual verification flow:
kapawith no overrides: rename and trash hidden, transport type disabled, every other field editable.kapawith admin title override: identity still locked, url and headers still editable.testmcp1: fully editable including rename and delete.foo.bar,__proto__, or pre-existing key: rejected with the appropriate banner message.Companion PR
Requires danny-avila/LibreChat#13173 deployed for the YAML-only signal. The detection falls back to "lock nothing" if
yamlMcpKeysis absent (older LibreChat without?baseOnlysupport), which is safer than the previous heuristic that produced false positives.Note
Medium Risk
Changes how MCP and general config edits are batched and persisted (delete/rename semantics, MongoDB unset behavior) and depend on a companion LibreChat
baseOnlyAPI for correct YAML locking.Overview
Locks YAML-defined MCP servers in the admin panel using server names from
getBaseConfigFn(?baseOnly=true/yamlMcpKeys), with a safe fallback when that API is missing. YAML entries keep rename/delete and transporttypelocked while other fields stay editable; inspector-only fields are hidden from the editor.MCP editing is per-leaf (
mcpServers.<name>.<field>) for create, edit, remove, and rename—including nested paths and entry-level$unset—instead of replacing whole server objects. ConfigPage dirty-state logic is tightened so container deletes and overlapping parent/child edits are not dropped before save.Scope editing disables creating/removing/renaming MCP servers but still allows field overrides. Role/group and validation UX add toasts, stricter server names, and transport required-field checks in the create dialog. Server-side Zod union traversal (
anyOfSchema/resolveSubSchema) improves per-field MCP validation; a largeMcpServersRenderertest suite covers the new behavior.Reviewed by Cursor Bugbot for commit 7f26300. Bugbot is set up for automated code reviews on this repo. Configure here.