From 5ff6513ca56a605e48adf57222a225204595eb71 Mon Sep 17 00:00:00 2001 From: Nguyen Thanh Dat Date: Tue, 1 Sep 2026 22:07:22 +0700 Subject: [PATCH] fix(combos): send null to clear an agent feature instead of omitting it (#12177) * fix(combos): send null to clear an agent feature instead of omitting it PUT /api/combos/[id] merges its body over the stored record, so an omitted field means "leave unchanged". The combos editor deleted a cleared agent feature from the payload, so unchecking context cache protection -- or emptying the system message or the tool filter -- never persisted: the old value survived the merge and the editor reopened with the toggle still on. updateCombo already deletes any key explicitly set to null, which is how description and context_length are cleared in the same save handler. Use the same shape for the three agent fields, and make them nullable in updateComboSchema so the null survives validation. The clearing logic moves into comboAgentFeatures.ts so it can be tested directly, matching comboQuotaOnlyFallback.ts next to it. Fixes #12158 * chore(changelog): point the fragment at the real PR number --- .../fixes/12177-combo-clear-agent-features.md | 4 + .../dashboard/combos/comboAgentFeatures.ts | 46 +++++ src/app/(dashboard)/dashboard/combos/page.tsx | 22 ++- src/shared/validation/schemas/combo.ts | 9 +- .../combo-clear-agent-features-12158.test.ts | 162 ++++++++++++++++++ 5 files changed, 233 insertions(+), 10 deletions(-) create mode 100644 changelog.d/fixes/12177-combo-clear-agent-features.md create mode 100644 src/app/(dashboard)/dashboard/combos/comboAgentFeatures.ts create mode 100644 tests/unit/combo-clear-agent-features-12158.test.ts diff --git a/changelog.d/fixes/12177-combo-clear-agent-features.md b/changelog.d/fixes/12177-combo-clear-agent-features.md new file mode 100644 index 0000000000..430b1efb21 --- /dev/null +++ b/changelog.d/fixes/12177-combo-clear-agent-features.md @@ -0,0 +1,4 @@ +- **fix(combos):** clearing an agent feature in the combos editor now persists — unchecking + context cache protection, or emptying the system message or tool filter, sends an explicit + `null` instead of dropping the field from the `PUT` body, which the update merge read as + "leave unchanged" ([#12177](https://github.com/diegosouzapw/OmniRoute/pull/12177)) — thanks @foreveryh diff --git a/src/app/(dashboard)/dashboard/combos/comboAgentFeatures.ts b/src/app/(dashboard)/dashboard/combos/comboAgentFeatures.ts new file mode 100644 index 0000000000..3907c2e577 --- /dev/null +++ b/src/app/(dashboard)/dashboard/combos/comboAgentFeatures.ts @@ -0,0 +1,46 @@ +/** + * Agent-features clearing for the combos editor (#399 / #401 / #454, fixed in #12158). + * + * `PUT /api/combos/[id]` merges its body over the stored record, so an omitted field + * means "leave unchanged". Deleting a cleared field from the payload therefore left the + * previous value in the database: unchecking context cache protection, or emptying the + * system message or tool filter, never persisted. Only an explicit `null` reaches + * `updateCombo`'s null-means-delete pass. + * + * On create there is nothing to clear, so an empty field is simply absent — the same + * shape `description` and `context_length` already use in this editor. + */ +export interface AgentFeatureInput { + systemMessage: string; + toolFilter: string; + contextCache: boolean; + isEdit: boolean; +} + +export interface AgentFeaturePatch { + system_message?: string | null; + tool_filter_regex?: string | null; + context_cache_protection?: true | null; +} + +export function buildAgentFeaturePatch({ + systemMessage, + toolFilter, + contextCache, + isEdit, +}: AgentFeatureInput): AgentFeaturePatch { + const patch: AgentFeaturePatch = {}; + + const message = systemMessage.trim(); + if (message) patch.system_message = message; + else if (isEdit) patch.system_message = null; + + const filter = toolFilter.trim(); + if (filter) patch.tool_filter_regex = filter; + else if (isEdit) patch.tool_filter_regex = null; + + if (contextCache) patch.context_cache_protection = true; + else if (isEdit) patch.context_cache_protection = null; + + return patch; +} diff --git a/src/app/(dashboard)/dashboard/combos/page.tsx b/src/app/(dashboard)/dashboard/combos/page.tsx index 16de03043c..7784599528 100644 --- a/src/app/(dashboard)/dashboard/combos/page.tsx +++ b/src/app/(dashboard)/dashboard/combos/page.tsx @@ -18,6 +18,7 @@ import { useCopyToClipboard } from "@/shared/hooks/useCopyToClipboard"; import { FieldLabelWithHelp, WeightTotalBar } from "./parts"; import { ComboTargetOptions } from "./ComboQuotaOnlyFallbackToggle"; import { applyQuotaOnlyFallbackConfig, setQuotaOnlyFallback } from "./comboQuotaOnlyFallback"; +import { buildAgentFeaturePatch } from "./comboAgentFeatures"; import { useComboProxyAssignments } from "./useComboProxyAssignments"; import { ResponseValidationEditor, type ResponseValidationValue } from "./ResponseValidationEditor"; import ReasoningTokenBufferToggle from "./ReasoningTokenBufferToggle"; @@ -3011,13 +3012,20 @@ function ComboFormModal({ isOpen, combo, onClose, onSave, activeProviders, combo saveData.config = configToSave; } - // Agent features (#399 / #401 / #454) - if (agentSystemMessage.trim()) saveData.system_message = agentSystemMessage.trim(); - else delete saveData.system_message; - if (agentToolFilter.trim()) saveData.tool_filter_regex = agentToolFilter.trim(); - else delete saveData.tool_filter_regex; - if (agentContextCache) saveData.context_cache_protection = true; - else delete saveData.context_cache_protection; + // Agent features (#399 / #401 / #454). A cleared field is sent as null on edit + // rather than omitted, because PUT merges over the stored record (#12158). + delete saveData.system_message; + delete saveData.tool_filter_regex; + delete saveData.context_cache_protection; + Object.assign( + saveData, + buildAgentFeaturePatch({ + systemMessage: agentSystemMessage, + toolFilter: agentToolFilter, + contextCache: agentContextCache, + isEdit, + }) + ); // Validate and save context_length if (contextLength !== undefined && contextLength !== null) { diff --git a/src/shared/validation/schemas/combo.ts b/src/shared/validation/schemas/combo.ts index ece9637138..5fd4afe5b1 100644 --- a/src/shared/validation/schemas/combo.ts +++ b/src/shared/validation/schemas/combo.ts @@ -418,9 +418,12 @@ export const updateComboSchema = z isActive: z.boolean().optional(), allowedProviders: z.array(z.string().trim().min(1).max(200)).max(100).optional(), allowedModelFamilies: z.array(z.string().trim().min(1).max(100)).max(100).optional(), - system_message: z.string().max(50000).optional(), - tool_filter_regex: z.string().max(1000).optional(), - context_cache_protection: z.boolean().optional(), + // Nullable like `description` and `context_length` above: an absent field means + // "leave unchanged" because updateCombo merges over the stored record, so clearing + // one needs an explicit null for updateCombo's null-means-delete pass (#12158). + system_message: z.string().max(50000).optional().nullable(), + tool_filter_regex: z.string().max(1000).optional().nullable(), + context_cache_protection: z.boolean().optional().nullable(), context_length: z.number().int().min(1000).max(2000000).optional().nullable(), compressionOverride: comboCompressionOverrideSchema.optional(), dimensions: z diff --git a/tests/unit/combo-clear-agent-features-12158.test.ts b/tests/unit/combo-clear-agent-features-12158.test.ts new file mode 100644 index 0000000000..40b3493f61 --- /dev/null +++ b/tests/unit/combo-clear-agent-features-12158.test.ts @@ -0,0 +1,162 @@ +/** + * #12158 — clearing an "Agent features" field on a combo must persist. + * + * `updateCombo` merges the PUT body over the stored record, so an absent field + * means "leave unchanged" and only an explicit `null` deletes it. `description` + * and `context_length` were already nullable in `updateComboSchema`; the three + * agent fields were not, so unchecking `context_cache_protection` (or clearing + * `system_message` / `tool_filter_regex`) left the old value in place. + */ +import test from "node:test"; +import assert from "node:assert/strict"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; + +const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-combo-clear-12158-")); +process.env.DATA_DIR = TEST_DATA_DIR; + +const { updateComboSchema } = await import("../../src/shared/validation/schemas.ts"); +const core = await import("../../src/lib/db/core.ts"); +const combosDb = await import("../../src/lib/db/combos.ts"); + +async function resetStorage() { + core.resetDbInstance(); + if (fs.existsSync(TEST_DATA_DIR)) { + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true }); + } + fs.mkdirSync(TEST_DATA_DIR, { recursive: true }); +} + +test.beforeEach(async () => { + await resetStorage(); +}); + +test.after(async () => { + core.resetDbInstance(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true }); +}); + +test("updateComboSchema accepts null for each agent feature field", () => { + const parsed = updateComboSchema.parse({ + system_message: null, + tool_filter_regex: null, + context_cache_protection: null, + }); + assert.equal(parsed.system_message, null); + assert.equal(parsed.tool_filter_regex, null); + assert.equal(parsed.context_cache_protection, null); +}); + +test("a null agent field still counts as a field to update", () => { + assert.doesNotThrow(() => updateComboSchema.parse({ context_cache_protection: null })); + assert.throws(() => updateComboSchema.parse({}), /No valid fields to update/); +}); + +test("a set agent feature value is still accepted and still rejects a bad type", () => { + const parsed = updateComboSchema.parse({ + system_message: "be terse", + tool_filter_regex: "^read_", + context_cache_protection: true, + }); + assert.equal(parsed.system_message, "be terse"); + assert.equal(parsed.tool_filter_regex, "^read_"); + assert.equal(parsed.context_cache_protection, true); + assert.throws(() => updateComboSchema.parse({ context_cache_protection: "yes" })); +}); + +test("null clears each agent feature through updateCombo", async () => { + const created = await combosDb.createCombo({ + name: "Agent Features Combo", + models: [{ provider: "openai", model: "gpt-4.1" }], + system_message: "be terse", + tool_filter_regex: "^read_", + context_cache_protection: true, + }); + assert.equal(created.system_message, "be terse"); + assert.equal(created.tool_filter_regex, "^read_"); + assert.equal(created.context_cache_protection, true); + + const cleared = await combosDb.updateCombo(created.id as string, { + system_message: null, + tool_filter_regex: null, + context_cache_protection: null, + }); + assert.ok(cleared); + assert.equal(cleared!.system_message, undefined); + assert.equal(cleared!.tool_filter_regex, undefined); + assert.notEqual(cleared!.context_cache_protection, true); + + // Re-read: this is what the editor reopens with, and what #12158 reported as + // still showing the toggle checked. + const reread = await combosDb.getComboById(created.id as string); + assert.ok(reread); + assert.equal(reread!.system_message, undefined); + assert.equal(reread!.tool_filter_regex, undefined); + assert.notEqual(reread!.context_cache_protection, true); +}); + +test("omitting an agent feature still leaves it unchanged", async () => { + const created = await combosDb.createCombo({ + name: "Untouched Combo", + models: [{ provider: "openai", model: "gpt-4.1" }], + system_message: "be terse", + context_cache_protection: true, + }); + + const updated = await combosDb.updateCombo(created.id as string, { description: "note" }); + assert.ok(updated); + assert.equal(updated!.system_message, "be terse"); + assert.equal(updated!.context_cache_protection, true); +}); + +const { buildAgentFeaturePatch } = + await import("../../src/app/(dashboard)/dashboard/combos/comboAgentFeatures.ts"); + +test("the editor sends null for every cleared agent feature on edit", () => { + assert.deepEqual( + buildAgentFeaturePatch({ + systemMessage: " ", + toolFilter: "", + contextCache: false, + isEdit: true, + }), + { system_message: null, tool_filter_regex: null, context_cache_protection: null } + ); +}); + +test("the editor omits an empty agent feature on create", () => { + assert.deepEqual( + buildAgentFeaturePatch({ + systemMessage: "", + toolFilter: "", + contextCache: false, + isEdit: false, + }), + {} + ); +}); + +test("the editor still sends set agent features, trimmed", () => { + assert.deepEqual( + buildAgentFeaturePatch({ + systemMessage: " be terse ", + toolFilter: " ^read_ ", + contextCache: true, + isEdit: true, + }), + { system_message: "be terse", tool_filter_regex: "^read_", context_cache_protection: true } + ); +}); + +test("clearing one agent feature does not disturb the others", () => { + assert.deepEqual( + buildAgentFeaturePatch({ + systemMessage: "keep me", + toolFilter: "", + contextCache: true, + isEdit: true, + }), + { system_message: "keep me", tool_filter_regex: null, context_cache_protection: true } + ); +});