diff --git a/open-sse/services/comboAgentMiddleware.ts b/open-sse/services/comboAgentMiddleware.ts index 5c08ea47c5..1b86b1cffd 100644 --- a/open-sse/services/comboAgentMiddleware.ts +++ b/open-sse/services/comboAgentMiddleware.ts @@ -36,20 +36,27 @@ interface Message { // Handles both actual newlines (U+000A) and literal \n sequences injected // by combo.ts streaming around the tag (#531). Non-global so that -// .exec() and .test() stay stateless; callers that need full replacement use -// String.prototype.replace() which replaces all non-overlapping matches. +// .exec() and .test() stay stateless (a global regex carries lastIndex between +// calls and would skip matches). const CACHE_TAG_PATTERN = /(?:\\n|\n|\r)*([^<]+)<\/omniModel>(?:\\n|\n|\r)*/; +// Global variant for .replace() callers that must strip EVERY tag. A non-global +// regex only removes the first match, so a single message carrying more than one +// tag (e.g. an Open WebUI follow-up/title request that inlines the +// whole chat history) leaked the remaining tags to the provider — defeating the +// cache-session protection stripModelTags exists to enforce (#454). +const CACHE_TAG_PATTERN_GLOBAL = /(?:\\n|\n|\r)*([^<]+)<\/omniModel>(?:\\n|\n|\r)*/g; + /** * Inject the model tag into the last assistant message (or append a new one). * Only modifies string content — does not touch array content to avoid breaking * Claude/Gemini multi-part message formats. */ export function injectModelTag(messages: Message[], providerModel: string): Message[] { - // Remove any existing tag first to avoid duplication on context compaction + // Remove any existing tags first to avoid duplication on context compaction const cleaned = messages.map((msg) => { if (msg.role === "assistant" && typeof msg.content === "string") { - return { ...msg, content: msg.content.replace(CACHE_TAG_PATTERN, "").trimEnd() }; + return { ...msg, content: msg.content.replace(CACHE_TAG_PATTERN_GLOBAL, "").trimEnd() }; } return msg; }); @@ -147,7 +154,7 @@ export function applyToolFilter( export function stripModelTags(messages: Message[]): Message[] { return messages.map((msg) => { if (typeof msg.content === "string" && CACHE_TAG_PATTERN.test(msg.content)) { - return { ...msg, content: msg.content.replace(CACHE_TAG_PATTERN, "").trimEnd() }; + return { ...msg, content: msg.content.replace(CACHE_TAG_PATTERN_GLOBAL, "").trimEnd() }; } return msg; }); diff --git a/tests/unit/combo-omnimodel-tag-stripping.test.ts b/tests/unit/combo-omnimodel-tag-stripping.test.ts new file mode 100644 index 0000000000..fc6d5fa3c0 --- /dev/null +++ b/tests/unit/combo-omnimodel-tag-stripping.test.ts @@ -0,0 +1,58 @@ +import { describe, test } from "node:test"; +import assert from "node:assert/strict"; +import { + stripModelTags, + injectModelTag, +} from "../../open-sse/services/comboAgentMiddleware.ts"; + +// Regression for the non-global CACHE_TAG_PATTERN bug: `.replace()` with a +// non-global regex only removes the FIRST match, so messages carrying more than +// one tag (e.g. an Open WebUI follow-up/title request that inlines the +// whole chat history) leaked the remaining tags straight to the provider — exactly +// what stripModelTags is documented to prevent (#454). +describe("comboAgentMiddleware — tag stripping (non-global regex bug)", () => { + test("stripModelTags removes ALL tags from a single message", () => { + const messages = [ + { + role: "user", + content: + "first claude/claude-opus-4-8 " + + "middle claude/claude-opus-4-8 " + + "end claude/claude-opus-4-8", + }, + ]; + + const stripped = stripModelTags(messages); + const remaining = (String(stripped[0].content).match(//g) || []).length; + + assert.equal(remaining, 0, "Provider must never see any tag (#454)"); + assert.ok( + !String(stripped[0].content).includes(""), + "Stripped content should contain no tag fragments" + ); + }); + + test("injectModelTag does not leave duplicate tags when the message already has several", () => { + const messages = [ + { role: "user", content: "Continue" }, + { + role: "assistant", + content: + "answer old/model-a more old/model-b", + }, + ]; + + const result = injectModelTag(messages, "new/model"); + const tagCount = (String(result[1].content).match(//g) || []).length; + + assert.equal(tagCount, 1, "Exactly one (the freshly pinned) tag should remain"); + assert.ok( + String(result[1].content).includes("new/model"), + "The remaining tag must be the new pin" + ); + assert.ok( + !String(result[1].content).includes("old/model"), + "All previous pins must be cleaned" + ); + }); +});