Repository navigation
fix(core): accept a partial model limit in legacy provider config - #60291
Closed
craigalbrecht wants to merge 1 commit into
Closed
craigalbrecht wants to merge 1 commit into
craigalbrecht wants to merge 1 commit into
Conversation
normalize.migrateProviders decodes each legacy provider with
decodeValue(ConfigProviderV1.Info, ...) and returns [] when it fails, so one
model that fails validation drops the entire provider and all of its other
models.
The V1 schema required both context and output in limit, while V2 makes all
three optional (schema/config/provider.ts). A config that overrides only the
output limit -- 'limit': { 'output': 16000 } -- is therefore valid in V2 but
failed V1 decoding, silently removing every model in that provider. Observed
on this box as 3,648 'configuration normalization diagnostic' warnings and
providers disappearing at runtime.
Make context and output optional in the V1 schema and guard them in
migrateModel the way input already was.
Regression test asserts a provider survives with a partial limit and keeps
both its models.
|
Thanks for tracking this down and writing up the diagnosis so clearly! I think this duplicates an existing pull request:
So I'm closing this one in favour of #53129. If you think it's missing something, such as your specific case of a provider with one partial-limit model and one full-limit model, please review it, test it or comment there. The real-world symptoms you saw (the warning counts and the |
Contributor
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
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.
Issue for this PR
Closes #60290
Type of change
What does this PR do?
normalize.migrateProvidersdecodes each legacy provider withdecodeValue(ConfigProviderV1.Info, ...)and returns[]when decoding fails, so a single model that fails validation removes the entire provider and all of its other models.ConfigProviderV1.Model.limitrequired bothcontextandoutput, while the V2 schema makes all three optional. A config that overrides only the output limit —"limit": { "output": 16000 }, a normal V2 shape — therefore failed V1 decoding and silently deleted its provider.This makes
contextandoutputoptional in the V1 schema and guardsundefinedinmigrateModel, matching howinputwas already handled.Symptom seen in the wild: 3,648
configuration normalization diagnosticwarnings over two days, and two providers whose only offending models setlimit.outputalone — those providers then threwModelUnavailableErrorat runtime while their siblings worked.How did you verify your code works?
A regression test in
packages/core/test/config/normalization.test.tsasserts that a provider with one partial-limit model and one full-limit model survives with both models present.It fails on current
dev:and passes with the change:
tsc -binpackages/coreis clean. The widertest/config/suite has two failures (excludes home-level claude…,deduplicates global ecosystem directories…) that fail identically on pristine source because they inspect the machine's home directory — not caused by this change.Visual
No visual change. This is config normalization: the diff makes two schema fields optional and guards
undefinedin the migration, so providers decode instead of being discarded.Screenshots / recordings
N/A — no UI surface.
Checklist