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 } + ); +});