Repository navigation
mcpx: provider-keyed model settings and model profiles (#1925 slice 2) - #4836
Conversation
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. |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 26 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. CodeRabbit Verify
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
CI coverage note: fresh PR checks show only |
What
Slice 2 of #1925: provider-keyed model settings and named model profiles with ordered cross-provider fallback, per the 2026-11-03 owner ruling on #1925.
Changes
model.providers(new,internal/settings/model.go): JSON object keyed by provider name with per-providerbaseURLandapiKeyEnv. Falls back to the singularmodel.baseURL/model.apiKeyEnvfor the provider named bymodel.provider, so single-provider configurations keep working unchanged.model.profiles(new): JSON object of named orderedprovider/modelchains, each optionally bounded by a profilebudgetand per-attempttimeout.internal/model:Configgains the two settings payloads;Resolvereturns a router when either is configured. Resolution accepts a profile name or aprovider/modelid frommodel.defaultor a per-operation override, tries profile entries in order, skips unconfigured providers and unset explicitly-named key variables, and fails withErrUnavailable(no fabricated output) when the whole chain is exhausted.model.apiKeyEnvor a provider-keyed entry.internal/daemon: policy carries both settings; the model layer activates for profile-only configurations; the answering provider is reported in theusedlabel.docs/configuration.mdprose,docs/consumer.md, and the generated settings inventory.Tests
New in
internal/model:model.providers/model.profilesJSON rejectedNew in
internal/settings: registry presence, kind/default/scope, and that the singularmodel.*settings are unchanged.New in
internal/daemon: generation through a named profile withmodel.provider: none.Verification (laptop)
go test ./internal/model -count=1— PASS (also-race -shuffle=on -count=5on the resolver/profile tests)go test ./internal/daemon -run 'TestGenerateModelLayer|TestGenerateUsesNamedProfile' -count=1— PASSgo test ./internal/settings -run 'TestModelProfiles|TestEffectExemptionsOnlyShrink|TestTheDocumentationListsEverySetting' -count=1— PASSgo testinpkgs/mcpx) is the merge authority for the full suite.Known pre-existing test gaps on main (not from this diff)
The full
internal/settingspackage currently fails on main for reasons unrelated to this slice: the compendium manifest is missing three docs, the env-read/script-env setter guards disagree ininternal/daemonandinternal/execsvc, and the effect guard still lists five unclassifiedproto.*/script.*settings. This PR adds the model settings to the effect exemption ratchet (pin 193 → 201) because no settings-driven e2e probe exists for them yet.Risk
Low. Additive settings;
model.provider: nonewith nothing else configured keeps the deterministic path exactly as before.Implements slice 2 of #1925 (task 6913). The parent issue remains open for the explicitly deferred plugin backend and sampling.answerLocally slices.
Open in [code]smith · Need help on this PR? Tag
@codesmith-botwith what you need.