fix(sse): strip every <omniModel> tag, not just the first (#454) (#3248)

Strip ALL <omniModel> tags before forwarding to provider (global regex variant).

Integrated into release/v3.8.12. Thanks @MikeTuev.
This commit is contained in:
MikeTuev
2026-06-06 05:21:40 +05:00
committed by GitHub
parent 1d28c0f13d
commit c2520bf5b7
2 changed files with 70 additions and 5 deletions

View File

@@ -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;
/**
* 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;
});

View File

@@ -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"
);
});
});