fix(providers): keep hand-edited context windows through a POST overwrite - #1535
Conversation
…rite
The dashboard's provider payload type has no member for contextWindow or
modelContextWindows, so an overwrite arrives without them. Registry
enrichment then fills the absent fields from the seed and the stored row
loses the user's values. For opencode-go the seed is exactly
{"kimi-k3": 262144}, which is what #1409 reports finding in place of a
hand-edited deepseek-v4-flash entry.
apiKeyPool and modelCosts are already carried across this path for the
same reason: the form does not send them, so absence must not mean
deletion. These two fields are the same class of user data and were
simply never added.
Ownership is sampled before enrichment. After enrichProviderFromCatalog
runs, an absent field and a registry-seeded one are indistinguishable, so
a guard written as prov.x === undefined afterwards can never fire.
When the client omits the map the stored value is the user's map alone.
Merging the registry seed in would persist seed keys into user config as
a side effect of an unrelated save, and router.ts already fills registry
values beneath user entries at resolve time.
This does not close #1409. The confirmed reproduction is a duplicate-name
POST through Add Provider; the reporter's sequence was an upgrade plus a
later full-config write, which points at the stale whole-document writer
in #1273 and is not established here.
Refs #1409, #1273
|
✅ Deterministic PR hygiene checks passed. |
📝 WalkthroughWalkthroughThe provider POST route now preserves user-supplied context-window settings during overwrites, merges model-specific values, and excludes registry-seeded keys when fields are omitted. Regression tests cover POST and PATCH behavior. The verification command now targets the management-provider validation suite. ChangesProvider context preservation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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 |
![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: 9ae7e21b27
ℹ️ 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".
| prov.modelContextWindows = submittedModelContextWindows | ||
| ? { ...existing.modelContextWindows, ...(prov.modelContextWindows ?? {}) } | ||
| : { ...existing.modelContextWindows }; |
There was a problem hiding this comment.
When a registry provider was originally created, enrichment persisted that release's modelContextWindows seed into the provider row even if the user never edited it. If a later release corrects the canonical seed and the dashboard performs a duplicate-name POST (which omits this field), this assignment replaces the freshly enriched map with the old persisted seed; routedProviderConfig then gives that provider map precedence, so the registry correction never takes effect. Preserve only identifiable user overrides—such as by tracking provenance or normalizing stored seed values—instead of treating the entire existing map as user-owned.
AGENTS.md reference: src/AGENTS.md:L18-L18
Useful? React with 👍 / 👎.
tests/provider-routes*.test.ts does not exist, and an unmatched glob aborts the run under zsh. Point at the suite that actually holds this path's coverage.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/management-provider-validation.test.ts`:
- Around line 523-537: Extend the test “PATCH can still delete a key with an
explicit null” to use the registry-seeded “kimi-k3” key, delete it via PATCH,
then call seedProvider(server.url, {}) and assert it remains absent from
persisted modelContextWindows. Replace the current “deepseek-v4-flash” assertion
so the test detects POST carry-over re-persisting the registry seed.
- Around line 510-517: Update the assertion in the “a brand-new provider still
receives the registry seed” test to verify that
providers["opencode-go"].modelContextWindows exactly matches the documented
{"kimi-k3": 262144} mapping, rather than only checking that the map is defined.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 65ed53c4-5640-4632-9db5-3713e52f20e0
📒 Files selected for processing (3)
devlog/_plan/260812_five_bug_fix_campaign/040_phase4_issue1409_context_window_overrides.mdsrc/server/management/provider-routes.tstests/management-provider-validation.test.ts
| test("a brand-new provider still receives the registry seed", async () => { | ||
| freshHome(); | ||
| const server = startServer(0); | ||
| try { | ||
| expect((await seedProvider(server.url, {})).status).toBe(200); | ||
|
|
||
| // No prior row exists, so enrichment is authoritative and the seed must land. | ||
| expect(loadConfig().providers["opencode-go"]?.modelContextWindows).toBeDefined(); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert the expected registry seed.
Line 517 accepts any defined map. The test passes if registry enrichment writes the wrong model ID or window. Assert the documented {"kimi-k3": 262144} value.
Proposed fix
- expect(loadConfig().providers["opencode-go"]?.modelContextWindows).toBeDefined();
+ expect(loadConfig().providers["opencode-go"]?.modelContextWindows)
+ .toEqual({ "kimi-k3": 262144 });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test("a brand-new provider still receives the registry seed", async () => { | |
| freshHome(); | |
| const server = startServer(0); | |
| try { | |
| expect((await seedProvider(server.url, {})).status).toBe(200); | |
| // No prior row exists, so enrichment is authoritative and the seed must land. | |
| expect(loadConfig().providers["opencode-go"]?.modelContextWindows).toBeDefined(); | |
| test("a brand-new provider still receives the registry seed", async () => { | |
| freshHome(); | |
| const server = startServer(0); | |
| try { | |
| expect((await seedProvider(server.url, {})).status).toBe(200); | |
| // No prior row exists, so enrichment is authoritative and the seed must land. | |
| expect(loadConfig().providers["opencode-go"]?.modelContextWindows) | |
| .toEqual({ "kimi-k3": 262144 }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/management-provider-validation.test.ts` around lines 510 - 517, Update
the assertion in the “a brand-new provider still receives the registry seed”
test to verify that providers["opencode-go"].modelContextWindows exactly matches
the documented {"kimi-k3": 262144} mapping, rather than only checking that the
map is defined.
| test("PATCH can still delete a key with an explicit null", async () => { | ||
| freshHome(); | ||
| const server = startServer(0); | ||
| try { | ||
| expect((await seedProvider(server.url, { modelContextWindows: { "deepseek-v4-flash": 900000 } })).status).toBe(200); | ||
|
|
||
| const patch = await fetch(new URL("/api/providers?name=opencode-go", server.url), { | ||
| method: "PATCH", | ||
| headers: { "content-type": "application/json" }, | ||
| body: JSON.stringify({ modelContextWindows: { "deepseek-v4-flash": null } }), | ||
| }); | ||
| expect(patch.status).toBe(200); | ||
|
|
||
| // Deletion is an explicit null through PATCH, which the POST carry-over must not undo. | ||
| expect(loadConfig().providers["opencode-go"]?.modelContextWindows?.["deepseek-v4-flash"]).toBeUndefined(); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
Test deletion across a later POST overwrite.
The test comment states that POST carry-over must not undo deletion, but the test only checks the immediate PATCH result. Use a registry-seeded key such as "kimi-k3", delete it with PATCH, then call seedProvider(server.url, {}) and assert that "kimi-k3" remains absent from persisted modelContextWindows. The current "deepseek-v4-flash" case cannot detect accidental re-persistence of the opencode-go registry seed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/management-provider-validation.test.ts` around lines 523 - 537, Extend
the test “PATCH can still delete a key with an explicit null” to use the
registry-seeded “kimi-k3” key, delete it via PATCH, then call
seedProvider(server.url, {}) and assert it remains absent from persisted
modelContextWindows. Replace the current “deepseek-v4-flash” assertion so the
test detects POST carry-over re-persisting the registry seed.
|
Full-suite result on the Linux runner at this branch head: All four Linux CI shards are green here as well. |
1 participant
Summary
POST /api/providersloses hand-edited context windows when it overwrites an existing provider.ProviderPayload(gui/src/provider-payload.ts) has no member forcontextWindowormodelContextWindows, so the dashboard's add/edit form structurally cannot send them. On an overwrite those fields are therefore absent,enrichProviderFromCatalog→enrichProviderFromRegistryfills them from the registry seed, and the stored row becomes the seed. Foropencode-gothe seed is exactly{"kimi-k3": 262144}— which is what #1409 reports finding in place of a hand-edited{"deepseek-v4-flash": 900000}.The fix follows the precedent already in this function:
apiKeyPoolandmodelCostsare carried across the same path, with the same rationale — the form does not send them, so absence must not mean deletion. These two fields are the same class of user-owned data and were simply never added to the list.Two details that matter for review:
enrichProviderFromCatalogruns, "the client omitted this field" and "the registry supplied it" are indistinguishable, so a carry-over guard written asprov.x === undefinedafterwards can never fire. An earlier draft had exactly that bug and its test would have stayed red.router.tsalready fills registry values beneath user entries at resolve time viamergeRecordFill.Deletion is unaffected: it goes through PATCH with an explicit
null, which the carry-over must not undo, and that is covered by a test.Why this does not carry
Closes #1409The confirmed reproduction is a duplicate-name POST through the Add Provider modal. The dashboard's ordinary editing surfaces use PATCH (
gui/src/pages/use-providers-crud.ts), andModels.tsxsendsmodelContextWindowsover PATCH, which merges per key and already preserves unmentioned entries.The reporter's sequence was an upgrade, a daemon restart, and a later unrelated full-config write. That points at the stale whole-document writer tracked in #1273, and nothing here rules it in or out. So this PR fixes a real, demonstrated data-loss defect on its own merits, and #1409 gets a comment describing what was confirmed rather than a closure it has not earned.
Verification
On a Linux runner (Bun 1.3.14):
bun x tsc --noEmit— exit 0bun test tests/management-provider-validation.test.ts— 60 pass, 0 failRed-before evidence: with only the test diff applied to unmodified
dev, 3 of the new tests fail.New coverage: an omitted map keeps the user's map and does not gain registry seed keys; a submitted map updates that key while other user keys survive; an omitted scalar
contextWindowis preserved; a submitted scalar still wins; a brand-new provider still receives the registry seed (no regression in enrichment); and PATCH can still delete a key with an explicit null.This touches a management write boundary, so it wants the security review
src/AGENTS.mdasks for — no auth, credential, or transport behavior changes, but the persisted-config write path does.Checklist
Refs #1409, #1273
Summary by CodeRabbit
Bug Fixes
Tests