Derive ProviderCatalogEntry from ProviderSettings instead of duplicating its field list - #391
Merged
TheGreatAxios merged 2 commits intoAug 8, 2026
Conversation
…ing its field list src/config/index.ts:95-130 (ProviderCatalogEntry) and src/config/settings.ts:21-45 (ProviderSettings) independently declared the same provider record, so a field added to one silently never reached the catalog conversion in catalogEntryAsProviderSettings/buildProviderCatalog. ProviderCatalogEntry is now Omit<ProviderSettings, "name" | "contextWindow"> plus its required name and the OAuth-only markers (codexProfile/codexAccountId/xaiProfile) that have no settings.json counterpart. Added a round-trip test in src/config.test.ts that fails if a shared field stops surviving buildProviderCatalog -> catalogEntryAsProviderSettings.
TheGreatAxios
force-pushed
the
cl-5680-providercatalogentry-and-providersettings-are-parallel-type
branch
from
August 8, 2026 18:56
6338a55 to
e1e344e
Compare
The round-trip test omitted keyless, anthropic, and opencodeGo, so it could not actually catch a dropped optional field despite claiming to exercise every field. Split anthropic/opencodeGo into their own cases since both also normalize baseURL in buildProviderCatalog, and cover keyless in the base case. Also correct the ProviderCatalogEntry doc comment: the Omit/intersection only forces literals to supply a newly required field: an optional field can still be silently dropped by a hand-written conversion, which is why the round-trip test exists.
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
1 participant
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.
Summary
ProviderCatalogEntry(src/config/index.ts:95-130) andProviderSettings(src/config/settings.ts:21-45) declared the same provider record independently. A field added to one and forgotten in the other is silently dropped wherever a settings provider is converted to a catalog entry, with no compiler error — the exact gap this fixes.ProviderCatalogEntryis nowOmit<ProviderSettings, "name" | "contextWindow"> & { name: string; codexProfile?; codexAccountId?; xaiProfile? }.namebecomes required (every catalog entry resolves to a concrete provider id);contextWindowis excluded because it was already never forwarded bybuildProviderCatalog/catalogEntryAsProviderSettings— a settings-only override, not surfaced to the /agent modal. The three OAuth-profile markers stay explicit because they have noProviderSettingscounterpart (Codex/xAI OAuth entries are never written to settings.json).Bug this closes
Before this change there was no link between the two types at all — a conversion function (
catalogEntryAsProviderSettings) already existed, but its return type andProviderCatalogEntry's field list were two independent hand-written lists with no compiler tie. That absence of a link was the bug: nothing would catch a newProviderSettingsfield never being added toProviderCatalogEntry, or vice versa.Test plan
round-trips every ProviderSettings field a catalog entry can carry through buildProviderCatalog and backinsrc/config.test.ts, which pushes a full-field provider throughbuildProviderCatalogthencatalogEntryAsProviderSettingsand asserts the fields survive intact — this now fails if a shared field stops making it through either conversion.bun run typecheck— no new errors (pre-existing unrelated arktype-version errors present on main across vendor/tui code).bun run build— clean.bun run test— 1 pre-existing, unrelated failure (detectLanguageServerAvailable, environment-dependent), everything else passes including the new test.