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
17 changes: 12 additions & 5 deletions open-sse/services/comboAgentMiddleware.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,20 +36,27 @@ interface Message {

// Handles both actual newlines (U+000A) and literal \n sequences injected
// by combo.ts streaming around the <omniModel> 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>([^<]+)<\/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
// <omniModel> 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>([^<]+)<\/omniModel>(?:\\n|\n|\r)*/g;

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.

medium

To avoid duplicating the complex regular expression pattern, you can dynamically construct CACHE_TAG_PATTERN_GLOBAL from CACHE_TAG_PATTERN.source. This ensures that any future updates to the pattern only need to be made in one place, preventing potential desynchronization bugs.

Suggested change
const CACHE_TAG_PATTERN_GLOBAL = /(?:\\n|\n|\r)*<omniModel>([^<]+)<\/omniModel>(?:\\n|\n|\r)*/g;
const CACHE_TAG_PATTERN_GLOBAL = new RegExp(CACHE_TAG_PATTERN.source, "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() };
}
Comment on lines 58 to 60

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.

medium

For performance and memory efficiency, consider checking CACHE_TAG_PATTERN.test(msg.content) before copying the message object and performing the regex replacement. This matches the pattern used in stripModelTags and avoids unnecessary object allocations and regex execution for the vast majority of assistant messages that do not contain any tags.

Suggested change
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() };
}
if (msg.role === "assistant" && typeof msg.content === "string" && CACHE_TAG_PATTERN.test(msg.content)) {
return { ...msg, content: msg.content.replace(CACHE_TAG_PATTERN_GLOBAL, "").trimEnd() };
}

return msg;
});
Expand Down Expand Up @@ -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;
});
Expand Down
58 changes: 58 additions & 0 deletions tests/unit/combo-omnimodel-tag-stripping.test.ts
Original file line number Diff line number Diff line change
@@ -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 <omniModel> 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 — <omniModel> tag stripping (non-global regex bug)", () => {
test("stripModelTags removes ALL <omniModel> tags from a single message", () => {
const messages = [
{
role: "user",
content:
"first <omniModel>claude/claude-opus-4-8</omniModel> " +
"middle <omniModel>claude/claude-opus-4-8</omniModel> " +
"end <omniModel>claude/claude-opus-4-8</omniModel>",
},
];

const stripped = stripModelTags(messages);
const remaining = (String(stripped[0].content).match(/<omniModel>/g) || []).length;

assert.equal(remaining, 0, "Provider must never see any <omniModel> tag (#454)");
assert.ok(
!String(stripped[0].content).includes("<omniModel>"),
"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 <omniModel>old/model-a</omniModel> more <omniModel>old/model-b</omniModel>",
},
];

const result = injectModelTag(messages, "new/model");
const tagCount = (String(result[1].content).match(/<omniModel>/g) || []).length;

assert.equal(tagCount, 1, "Exactly one (the freshly pinned) tag should remain");
assert.ok(
String(result[1].content).includes("<omniModel>new/model</omniModel>"),
"The remaining tag must be the new pin"
);
assert.ok(
!String(result[1].content).includes("old/model"),
"All previous pins must be cleaned"
);
});
});