Repository navigation
🧩 fix: Import and Save Enum Values the Bundled Schema Doesn't List Yet - #198
Merged
Merged
Conversation
A librechat.yaml written for a LibreChat newer than the panel's bundled librechat-data-provider failed to import with "Invalid enum value", for example `interface.retentionMode: ephemeral` (LibreChat dev has it, the bundled 0.8.524 does not). The import parse now tolerates an invalid_enum_value issue whose received value is text, keeps the YAML as written and reports each such value with its path and the values the panel lists (unknownEnumValues). Every other issue still fails the import: a number where an enum belongs, a wrong type, a union none of whose branches fits, and a mixed YAML where such errors come with unknown values. The wizard's review step names, in a non-blocking notice, each such value the import writes and its path: one in a row left checked, and not inside a key the import leaves out. The notice says the value is imported as written, that LibreChat v0.8.8 and later refuse a value they don't know when they save it, and that earlier releases store it without checking, so the admin should check the spelling. The save pre-flight (planConfigChanges) treats the same issues as non-blocking, for field and entry validation and for the whole-section pass, so the plan writes the value: an imported one, or a select or capability list that already holds one. Enum issues carry the value they refused (received), except inside a union's branch, which still blocks. LibreChat validates these writes from v0.8.8 (CONFIG_OVERRIDE_INVALID), and its refusal is still mapped onto the field, with issueParams where the server sends them. v0.8.5 to v0.8.7 serve the admin config API without validating values, and the panel can't tell them from v0.8.8: none of them serves the config meta route (v0.8.9-rc1 is the first). Blocking on a server without it would block v0.8.8, whose data-provider (0.8.527) is ahead of the bundled one, so on v0.8.5 to v0.8.7 the notice is the only check. The tests use made-up members (`someday`, `teleport`), so they keep testing the same case after a data-provider bump. Supersedes #112 by @dustinhealy, reusing its tolerance policy (which it took from #82), not its code. Closes #72
Contributor
Author
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ 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". |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request Template
Summary
Fixes #72: a librechat.yaml written for a LibreChat release newer than the panel's bundled
librechat-data-providerfails to import withInvalid enum value. The capabilities named in #72 (subagents,skills) are known to today's bundle, but the lockstep problem is still there. LibreChatdevacceptsinterface.retentionMode: ephemeral, the bundled 0.8.524 doesn't, and every enum member LibreChat adds breaks import again until the panel ships a bump.This PR supersedes #112 by @dustinhealy. It reuses that PR's tolerance policy, which #112 credits to #82: only an
invalid_enum_valueissue whose received value is text is tolerated, and only when no other error remains. It is written fresh on today'smain, which already returns the parsed YAML without schema defaults, so #112's strip-and-restore step isn't needed.Import parse (
parseConfigYaml). The schema's issues are split in one pass:invalid_enum_valueissue whose received value is text is tolerated. The YAML is kept exactly as written, and each such value is reported in a newunknownEnumValueslist on the parse result, with its path and the values the panel lists there.Import wizard. The review step shows a non-blocking notice naming each such value the import writes, with its path and the values the panel lists. A value in a row the admin unchecks, or inside a key the import leaves out (a librechat.yaml-only section such as Turnstile, a list entry the target hides), isn't named. The notice says:
Save pre-flight (
planConfigChanges). Every save and import runs this pre-flight, and before this PR it refused the same values. Without this part, the wizard would accept the YAML and then block Import with "must be one of …".received), except inside a union's branch, which still blocks as a union failure.issueParams.Which LibreChat checks the value.
CONFIG_OVERRIDE_INVALIDandinvalid_enum_valueat the field's path.GET /api/admin/config/meta; v0.8.9-rc1 is the first release that does.The editors already show a value they don't list:
SelectFieldadds it as an option, andMultiEnumField(#162) keeps it as an extra checkbox. They needed no change.Not carried over from #112. The
ListFieldandSelectFieldchanges:mainalready renders such values. Unknown keys are a separate matter: onmainthey are already kept and offered unchecked in the review.Closes #72
Change Type
Testing
The unit and component tests use made-up members (
retentionMode: someday,teleportin the capability list), notephemeral, so they keep testing the same case after the panel bumpslibrechat-data-provider.Unit and component tests:
src/server/config.test.ts,parseConfigYaml:interface.retentionMode, imports as written. Both values are reported with their paths and the listed values.src/utils/changes.test.ts, the pre-flight:received, but a union's branch doesn't.interface.retentionMode, a wholeinterfacesection holding it (the section pass), and a capability list where another capability is toggled next to a stored unknown member.reasoning_effortin a model spec preset (a keyed collection entry) is written too.src/server/config/save.test.ts:applyConfigChangesstores the unknown value against the simulated admin API. When the server refuses a value, the refusal lands oninterface.retentionModeas "is not one of the allowed values", or withissueParamsasmust be one of "all", "temporary", and nothing is written.src/components/configuration/wizard/__tests__/Preview.test.tsx:src/components/configuration/__tests__/ImportYamlDialog.test.tsx, the real parse, preview and pre-flight together:interface.retentionModeshows the notice, raises no problems, and imports{ fieldPath: 'interface.retentionMode', value: 'someday' }into Base.Each part was reverted on its own to confirm these tests fail without it: the parse change, the pre-flight change (and the section-pass filter alone), the notice, and the notice's filter (and its skipped-key half alone).
Live check with a production build against LibreChat
dev:interface.retentionMode: ephemeralinto BaseretentionMode: ephemeral, and the Retention mode select showsephemeral. Onmain, the parse refuses this YAML with "Config validation failed" (interface.retentionMode: Invalid enum value …), as the parse test shows with this change reverted.retentionMode: ephemeral,turnstile.options.size: huge,webSearch.searchProvider: bing,teleportin the capability listteleport. Turnstile's site key and size are listed only under "2 keys won't be imported … read from librechat.yaml only". Unchecking Retention mode drops it from the notice (2 values), and the button reads "Import 2 settings".retentionMode: forever, andteleportin the capability list)e2e/import.spec.ts)No console errors or failed requests in these runs, and the notice reads correctly in light and dark mode.
Two checks couldn't be run live as described:
devlist the same agent capabilities, so there is no capability LibreChat knows and the panel doesn't. "Toggling another capability keeps the unknown member in the written list" is covered by the pre-flight test above. In the live run, the refused capability write shows the panel sending the list withteleportkept in it.ephemeral: it validates writes, and its data-provider doesn't list the value yet. So the refusal was checked with a value LibreChatdevdoesn't know.CONFIG_OVERRIDE_INVALIDbody:invalid_enum_valueat the field's path, with noissueParams. That is the unworded case insave.test.ts.On a fresh database, the second test in
e2e/import.spec.tscan time out before it opens the wizard. ItsensureUserProfilehelper looks for "Create profile" as soon as the network is idle, before the button renders, so the USER profile is never created. Once the profile exists, all 6 pass. This PR doesn't change the spec or the helper.Test Configuration:
dev, MongoDB replica set.bun run build+bun server.ts).npx tsc --noEmit,npx eslint src/ e2e/ --max-warnings 0,npx prettier --checkon the changed files,bun run test:lint-rules,npx vitest run(3358 tests),bun run build.Checklist