Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -271,9 +271,12 @@ the untouched user key survives.

```bash
bun x tsc --noEmit
bun test tests/provider-routes*.test.ts tests/management*.test.ts tests/config*.test.ts
bun test tests/management-provider-validation.test.ts tests/management*.test.ts tests/config*.test.ts
```

(There is no `tests/provider-routes*.test.ts`; an unmatched glob aborts the run under zsh.
The management-provider validation suite is where this path's coverage lives.)

## Delivery

Branch `codex/1409-preserve-context-window-overrides`, PR against `dev`,
Expand Down
23 changes: 23 additions & 0 deletions src/server/management/provider-routes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,12 @@ export async function handleProviderRoutes(ctx: ManagementContext): Promise<Resp
}
// Catalog providers (e.g. ollama-cloud) carry a models + vision/reasoning classification the GUI
// doesn't send — merge it in so the sidecars are gated correctly.
// Sample request ownership BEFORE enrichment. Enrichment fills absent fields from the
// registry seed, after which "the client omitted this" and "the registry supplied it" are
// indistinguishable — so a carry-over guard written as `prov.x === undefined` after this
// call can never fire.
const submittedContextWindow = Object.hasOwn(prov, "contextWindow");
const submittedModelContextWindows = Object.hasOwn(prov, "modelContextWindows");
enrichProviderFromCatalog(name, prov);
const { saveConfigPreservingClaudeCode: save } = await import("../../config");
// Overwriting an existing provider must not drop its multi-key pool: carry it over, then
Expand All @@ -361,6 +367,23 @@ export async function handleProviderRoutes(ctx: ManagementContext): Promise<Resp
// erase hand-edited per-model prices from Logs/Usage estimates.
const existingCosts = config.providers[name]?.modelCosts;
if (existingCosts && !prov.modelCosts) prov.modelCosts = existingCosts;
// ...and to hand-edited context windows. `ProviderPayload` (gui/src/provider-payload.ts)
// has no member for either field, so the add/edit form structurally cannot send them:
// absence in the request means "not carried", never "the user deleted it". Deletion goes
// through PATCH with an explicit null (#1409).
const existing = config.providers[name];
if (!submittedContextWindow && existing?.contextWindow !== undefined) {
prov.contextWindow = existing.contextWindow;
}
if (existing?.modelContextWindows) {
// When the client did send a map, its keys win and the user's other keys survive. When
// it did not, 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.
prov.modelContextWindows = submittedModelContextWindows
? { ...existing.modelContextWindows, ...(prov.modelContextWindows ?? {}) }
: { ...existing.modelContextWindows };
Comment on lines +383 to +385

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve registry refreshes when carrying context windows

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 👍 / 👎.

}
config.providers[name] = stripRegistryOnlyStaticHeaders(name, prov);
if (body.setDefault === true) config.defaultProvider = name;
save(config);
Expand Down
114 changes: 114 additions & 0 deletions tests/management-provider-validation.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -427,6 +427,120 @@ describe("provider management validation", () => {
}
});

// #1409: the add/edit form's 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 the reporter found in
// place of their deepseek-v4-flash override.
describe("provider POST overwrite preserves hand-edited context windows (#1409)", () => {
async function seedProvider(url: URL, extra: Record<string, unknown>): Promise<Response> {
return fetch(new URL("/api/providers", url), {
method: "POST",
headers: { "content-type": "application/json" },
body: JSON.stringify({
name: "opencode-go",
provider: { adapter: "openai-chat", baseUrl: "https://opencode.ai/zen/go/v1", apiKey: "k", ...extra },
}),
});
}

function freshHome(): void {
if (existsSync(TEST_DIR)) rmSync(TEST_DIR, { recursive: true });
mkdirSync(TEST_DIR, { recursive: true });
process.env.OPENCODEX_HOME = TEST_DIR;
saveConfig(config("127.0.0.1"));
}

test("an omitted modelContextWindows keeps the user's map, without registry seed keys", async () => {
freshHome();
const server = startServer(0);
try {
expect((await seedProvider(server.url, { modelContextWindows: { "deepseek-v4-flash": 900000 } })).status).toBe(200);
expect((await seedProvider(server.url, {})).status).toBe(200);

// The user's key survives, and the registry seed is NOT persisted into user config:
// router.ts fills registry values beneath user entries at resolve time, so writing
// them here would be a side effect of an unrelated save.
expect(loadConfig().providers["opencode-go"]?.modelContextWindows).toEqual({ "deepseek-v4-flash": 900000 });
} finally {
await server.stop(true);
}
});

test("a submitted modelContextWindows updates that key and keeps the others", async () => {
freshHome();
const server = startServer(0);
try {
expect((await seedProvider(server.url, { modelContextWindows: { "deepseek-v4-flash": 900000 } })).status).toBe(200);
expect((await seedProvider(server.url, { modelContextWindows: { "kimi-k3": 300000 } })).status).toBe(200);

expect(loadConfig().providers["opencode-go"]?.modelContextWindows)
.toEqual({ "deepseek-v4-flash": 900000, "kimi-k3": 300000 });
} finally {
await server.stop(true);
}
});

test("an omitted contextWindow keeps the user's scalar", async () => {
freshHome();
const server = startServer(0);
try {
expect((await seedProvider(server.url, { contextWindow: 777000 })).status).toBe(200);
expect((await seedProvider(server.url, {})).status).toBe(200);

expect(loadConfig().providers["opencode-go"]?.contextWindow).toBe(777000);
} finally {
await server.stop(true);
}
});

test("a submitted contextWindow still wins", async () => {
freshHome();
const server = startServer(0);
try {
expect((await seedProvider(server.url, { contextWindow: 777000 })).status).toBe(200);
expect((await seedProvider(server.url, { contextWindow: 512000 })).status).toBe(200);

expect(loadConfig().providers["opencode-go"]?.contextWindow).toBe(512000);
} finally {
await server.stop(true);
}
});

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();
Comment on lines +510 to +517

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 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.

Suggested change
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.

} finally {
await server.stop(true);
}
});

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();
Comment on lines +523 to +537

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ 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.

} finally {
await server.stop(true);
}
});
});

test("provider management accepts modelCosts on the canonical openai provider", async () => {
if (existsSync(TEST_DIR)) rmSync(TEST_DIR, { recursive: true });
mkdirSync(TEST_DIR, { recursive: true });
Expand Down
Loading