fix(models): context window controls for metadata-poor providers (#1073) - #1223
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
|
✅ Deterministic PR hygiene checks passed. |
![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: 5b01de549c
ℹ️ 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".
| <span className="muted mono text-label">{t("models.active", { active: activeCount, total: rows.length })}</span> | ||
| </button> | ||
| <div className="row models-provider-actions"> | ||
| {!isNative && ( |
There was a problem hiding this comment.
When a valid custom model is assigned to the openai provider, /api/models combines that non-native row with the native OpenAI rows, causing every(row.native) to classify the group as non-native and render this new button. Any Apply then PATCHes context fields onto canonical openai, but providerManagementConfigError requires that provider to remain equal to its canonical seed and returns 400, so the dashboard exposes a control that can never save. Gate the control on the provider's forward auth mode or identity rather than aggregate row nativeness.
AGENTS.md reference: gui/AGENTS.md:L10-L10
Useful? React with 👍 / 👎.
|
|
||
| const openContextSettings = (group: ProviderModelGroup<ModelRow>) => { | ||
| const modelIds = [...new Set([ | ||
| ...group.rows.map(model => model.id), |
There was a problem hiding this comment.
When a provider has a custom model, this adds that row to the per-model context picker even though catalog assembly deliberately skips applyProviderConfigHints for custom rows. For a custom-only model—or any custom model with its own context window—the PATCH succeeds and persists modelContextWindows[id], but the generated catalog continues using the custom model's separate customModels[].contextWindow, so the UI reports success for an ineffective change. Exclude custom rows that cannot inherit provider hints, or route their edits through the existing custom-model editor.
AGENTS.md reference: gui/AGENTS.md:L10-L10
Useful? React with 👍 / 👎.
03bc98d to
9d95125
Compare
5b01de5 to
39962e2
Compare
github-actions
Bot
changed the title
✅ READY
|
github-actions
Bot
changed the title
39962e2 to
67790a5
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 39962e2049
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| <span className="muted mono text-label">{t("models.active", { active: activeCount, total: rows.length })}</span> | ||
| </button> | ||
| <div className="row models-provider-actions"> | ||
| {!isNative && ( |
There was a problem hiding this comment.
When the catalog includes combo aliases and there is no real provider named combo, those rows are grouped under the synthetic combo provider, but PATCH /api/providers?name=combo returns 404 because the management route requires hasOwnProvider(config.providers, name). Showing this new button for every non-native group therefore exposes a dialog whose Apply can never save for normal combo groups; gate it on a configured provider summary rather than !isNative alone.
AGENTS.md reference: gui/AGENTS.md:L10-L10
Useful? React with 👍 / 👎.
…reverting concurrent changes Adopts #1203 by @estelledc — the first three commits are theirs, cherry-picked with authorship intact. The approach was right: expose the existing `providers.<id>.contextWindow` / `modelContextWindows` contract at the management and UI layers without touching catalog derivation, which already materializes those values when upstream metadata is absent. Four corrections, all found by independent audit. **Only the selected model was saved.** The drafts map held edits for every model but the PATCH was keyed on `contextModelId`, so a value typed into model A and then abandoned by switching to B vanished — no error, no warning. The PR's own test pinned that as correct. It now sends every model the user typed into. **But "every model that differs" would have been wrong the other way.** The 10s poll can refresh a field while the modal is open; diffing drafts against live state would then call an untouched field dirty and revert someone else's change. Two conditions are required: the user touched it, AND the value differs from what the modal opened with. Both apply to the provider default too, which was previously sent unconditionally and would stamp a stale number over a concurrent update. The snapshot holds canonical numbers, not the raw text. Retyping 64000 as "64,000" is not an edit, and treating it as one would resurrect the same stale-write. When nothing survives the comparison, no PATCH is sent at all and the feedback says so rather than claiming an update. **`Number.isInteger(1e100)` is true.** Both the management validator and the form accepted it; it would persist and serialize into the catalog as an enormous number that can make Codex reject the file. Both now require a safe integer. The default is only validated when touched, so a value inherited from a hand-edited config cannot block an unrelated per-model save. **An override for a model that left live discovery was unreachable.** It sat in the drafts map, absent from the picker, impossible to inspect or clear. Tests: the exact #1073 reproduction is split in two, because a single case setting `modelContextWindows` keeps passing with the provider-wide fallback deleted. Ablations were driven red in their real defect shape rather than as artificially strong mutants — notably, comparing against live `groups` while keeping the touched guard is only visible when a field is edited, reverted, and changed server-side, which the suite now covers. Translated provider docs (ko/ja/ru/zh-cn) described both fields as caps only, which reads as the opposite of the fix for non-English users. Co-authored-by: zhouxun <zhouxun.13@bytedance.com> Closes #1073
react-doctor's prefer-module-scope-pure-function, and it is right: the function closes over nothing, so rebuilding it on every render is wasted work. The prepush doctor gate rejected the push over it.
67790a5 to
b07a582
Compare
0d7e0ca to
912cb99
Compare
2 participants
Summary
A provider whose
/modelsreturns nothing but ids leaves the routed catalog on its conservative 128K fallback. The backend already honorsproviders.<id>.contextWindowandmodelContextWindows—configuredContextWindowreads them and the value is materialized when upstream metadata is absent — butGET /api/providersnever returned those fields and the PATCH mask never accepted them, so the Models page had nowhere to save. Hand-editingconfig.jsonworked; the dashboard did not.Adopts #1203 by @estelledc — the first three commits are theirs, cherry-picked with authorship intact. The approach was right: expose the existing contract at the management and UI layers without touching catalog derivation, and leave
providerContextCapsas the separate ceiling it already is.Four corrections, all from independent audit
Only the selected model was saved. The drafts map held edits for every model, but the PATCH was keyed on the current picker selection. A value typed into model A and then abandoned by switching to B vanished with no error and no warning. The PR's own test pinned that as correct behaviour.
"Every model that differs" would have been wrong the other way. The 10s poll can refresh a field while the modal is open; diffing drafts against live state would call an untouched field dirty and revert someone else's change. Two conditions are now required — the user touched it, and the value differs from what the modal opened with. Both apply to the provider default too, which was previously sent unconditionally.
The snapshot holds canonical numbers rather than raw text, so retyping
64000as64,000is not an edit. When nothing survives the comparison, no PATCH is sent and the feedback says so instead of claiming an update.Number.isInteger(1e100)is true. Both the validator and the form accepted it; it would persist and serialize into the catalog as an enormous number that can make Codex reject the file. Both now require a safe integer. The default is validated only when touched, so a value inherited from a hand-edited config cannot block an unrelated per-model save.An override for a model that left live discovery was unreachable — present in the drafts map, absent from the picker, impossible to inspect or clear.
Verification
bun run typecheck,bun run lint:gui,bun run doctor:gui,bun run privacy:scan— all clean.bun run test— 9761 pass, 8 skip, 0 fail across 607 files.modelContextWindowskeeps passing with the provider-wide fallback deleted — the per-model value is chosen first, so one test cannot defend both branches.groupsis only visible when a field is edited, reverted, and changed server-side, which the suite now covers explicitly.Docs
Translated provider docs (ko/ja/ru/zh-cn) described both fields as caps only, which reads as the opposite of the fix for non-English users. Synced with the English source.
Checklist
Stacked on #1220. Replaces #1203.
Closes #1073