diff --git a/CHANGELOG.md b/CHANGELOG.md index c2f9cc25dc..0641e622b6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ ### 🔧 Bug Fixes - **fix(api):** relay worker now binds the SSRF guard to a stable `const` name so minified standalone (Docker) builds resolve it ([#6149](https://github.com/diegosouzapw/OmniRoute/issues/6149)) — the Vercel/Deno relay generators embedded the shared `resolveRelayTarget` guard as a bare `${fn.toString()}` declaration while the worker body called the hardcoded literal name; SWC minification mangled the source function's name, so the deployed worker defined `` but still called `resolveRelayTarget` → `ReferenceError`. Both templates now emit `const resolveRelayTarget = ${fn.toString()};` (the const name is a template literal, immune to minification). Regression guard: `tests/unit/relay-minified-fn-6149.test.ts` (4). (thanks @SeaXen) +- **fix(backend):** memory injection now keeps the injected system message **first** for providers that require it (via a `PROVIDERS_SYSTEM_MUST_BE_FIRST` capability), instead of the cache-safe mid-array splice that made strict providers reject the request with a 400 ([#6135](https://github.com/diegosouzapw/OmniRoute/issues/6135)). Regression guard: `tests/unit/memory-system-first-6135.test.ts`. - **fix(services):** 9Router embed panel no longer 404s (optional catch-all route) and the supervisor probes the port before spawning to avoid raw EADDRINUSE ([#6205](https://github.com/diegosouzapw/OmniRoute/issues/6205)). Regression guards: `tests/unit/ninerouter-embed-port-6205.test.ts`, `tests/unit/services/ServiceSupervisor.test.ts`. (thanks @jonlwheat2-gif) ### ⚡ Performance & Infrastructure diff --git a/src/lib/memory/injection.ts b/src/lib/memory/injection.ts index b9a6199bb5..5eaa918ed8 100644 --- a/src/lib/memory/injection.ts +++ b/src/lib/memory/injection.ts @@ -56,6 +56,29 @@ export function providerSupportsSystemMessage(provider: string | null | undefine return !PROVIDERS_WITHOUT_SYSTEM_MESSAGE.has(normalized); } +/** + * Providers that accept a system-role message ONLY at index 0 (a `system` + * message at any later position is rejected with HTTP 400). For these, the + * cache-safe mid-array splice (which inserts memory just before the last user + * turn) is unsafe in multi-turn conversations, so memory must be merged into / + * prepended as the leading system message instead. See #6135. + * + * Populated with the Xiaomi MiMo endpoint (provider id `xiaomi-mimo`, registry + * alias `mimo`, serving mimo-v2.5) confirmed live to 400 on a non-first system + * message. Add other providers here only when they are documented as strict. + */ +const PROVIDERS_SYSTEM_MUST_BE_FIRST = new Set(["xiaomi-mimo", "mimo"]); + +/** + * Returns true when the given provider requires the system message to be first. + * Falls back to false for unknown/null providers (preserves current behavior). + */ +export function systemMessageMustBeFirst(provider: string | null | undefined): boolean { + if (!provider) return false; + const normalized = provider.toLowerCase().trim(); + return PROVIDERS_SYSTEM_MUST_BE_FIRST.has(normalized); +} + /** * Format memories into a single labeled context string. * Format: "Memory context: \n..." @@ -92,6 +115,46 @@ export interface InjectMemoryOptions { cacheSafe?: boolean; } +/** + * #6135: place the memory as a leading system message for providers that reject + * a non-first system role — merging into an existing index-0 system message when + * present, else prepending. Split out of injectMemory to keep it flat. + */ +function injectSystemFirst( + request: ChatRequest, + messages: ChatMessage[], + memoryText: string, + count: number +): ChatRequest { + log.info("memory.injection.injected", { count, strategy: "system-first", model: request.model }); + const first = messages[0]; + if (first && first.role === "system") { + const merged: ChatMessage = { ...first, content: `${memoryText}\n${first.content}` }; + return { ...request, messages: [merged, ...messages.slice(1)] }; + } + const memorySystemMessage: ChatMessage = { role: "system", content: memoryText }; + return { ...request, messages: [memorySystemMessage, ...messages] }; +} + +/** + * Place a memory message at the #3890 cache-safe anchor (just before the last + * user turn) when one exists, else prepend it. Shared by the system and user + * injection strategies to keep injectMemory flat. + */ +function placeMessage( + request: ChatRequest, + messages: ChatMessage[], + msg: ChatMessage, + cacheSafeIndex: number +): ChatRequest { + if (cacheSafeIndex >= 0) { + const next = [...messages]; + next.splice(cacheSafeIndex, 0, msg); + return { ...request, messages: next }; + } + return { ...request, messages: [msg, ...messages] }; +} + export function injectMemory( request: ChatRequest, memories: Memory[], @@ -116,38 +179,27 @@ export function injectMemory( // back to a leading message when caching is off or there is no user turn to anchor on. const cacheSafeIndex = options.cacheSafe ? messages.findLastIndex((m) => m.role === "user") : -1; - if (providerSupportsSystemMessage(provider)) { - // Strategy 1: inject as a system message. - // Prepending before any existing system messages keeps memory context - // accessible without overriding the caller's own system instructions. - const memorySystemMessage: ChatMessage = { role: "system", content: memoryText }; - log.info("memory.injection.injected", { - count: memories.length, - strategy: cacheSafeIndex >= 0 ? "system-cache-safe" : "system", - model: request.model, - }); - if (cacheSafeIndex >= 0) { - const next = [...messages]; - next.splice(cacheSafeIndex, 0, memorySystemMessage); - return { ...request, messages: next }; - } - return { ...request, messages: [memorySystemMessage, ...messages] }; - } else { - // Strategy 2 (fallback): inject as a user message. - // Used for providers like o1-mini that reject the system role. - const memoryUserMessage: ChatMessage = { role: "user", content: memoryText }; - log.info("memory.injection.injected", { - count: memories.length, - strategy: cacheSafeIndex >= 0 ? "user-cache-safe" : "user", - model: request.model, - }); - if (cacheSafeIndex >= 0) { - const next = [...messages]; - next.splice(cacheSafeIndex, 0, memoryUserMessage); - return { ...request, messages: next }; - } - return { ...request, messages: [memoryUserMessage, ...messages] }; + const supportsSystem = providerSupportsSystemMessage(provider); + + // #6135: strict providers reject a system message at a non-zero index. Never + // apply the cache-safe mid-array splice for these — keep the system message + // first (extracted to injectSystemFirst to keep this function flat). + if (supportsSystem && systemMessageMustBeFirst(provider)) { + return injectSystemFirst(request, messages, memoryText, memories.length); } + + // Strategy 1 (system): prepend before existing system messages, preserving the + // caller's own instructions. Strategy 2 (user, e.g. o1-mini): inject as a user + // message. Both honor the #3890 cache-safe anchor via placeMessage. + const role: ChatMessage["role"] = supportsSystem ? "system" : "user"; + const memoryMessage: ChatMessage = { role, content: memoryText }; + const base = supportsSystem ? "system" : "user"; + log.info("memory.injection.injected", { + count: memories.length, + strategy: cacheSafeIndex >= 0 ? `${base}-cache-safe` : base, + model: request.model, + }); + return placeMessage(request, messages, memoryMessage, cacheSafeIndex); } /** diff --git a/tests/unit/memory-system-first-6135.test.ts b/tests/unit/memory-system-first-6135.test.ts new file mode 100644 index 0000000000..371ccc8d8a --- /dev/null +++ b/tests/unit/memory-system-first-6135.test.ts @@ -0,0 +1,115 @@ +/** + * Tests for #6135: cache-safe memory injection inserts a system message at a + * non-zero index, which strict providers (e.g. xiaomi-mimo / alias `mimo`, + * serving mimo-v2.5) reject with HTTP 400. + * + * For providers flagged system-message-must-be-first, the injected system + * message MUST remain at index 0 (merged into an existing leading system + * message, or prepended) instead of being spliced before the last user turn. + * Non-flagged providers keep the existing cache-safe placement unchanged. + */ + +import { describe, it } from "node:test"; +import assert from "node:assert/strict"; +import { + injectMemory, + systemMessageMustBeFirst, +} from "../../src/lib/memory/injection.ts"; +import type { ChatMessage, ChatRequest } from "../../src/lib/memory/injection.ts"; +import { MemoryType } from "../../src/lib/memory/types.ts"; +import type { Memory } from "../../src/lib/memory/types.ts"; + +function mem(content: string): Memory { + return { + id: `mem-${content}`, + content, + type: MemoryType.FACTUAL, + apiKeyId: "k", + createdAt: "2026-01-01T00:00:00.000Z", + updatedAt: "2026-01-01T00:00:00.000Z", + importance: 0.5, + } as unknown as Memory; +} + +// Multi-turn conversation with >= 2 user turns → findLastIndex(user) > 0. +function multiTurn(): ChatRequest { + return { + model: "mimo-v2.5", + messages: [ + { role: "system", content: "SYSTEM PROMPT" } as ChatMessage, + { role: "user", content: "turn 1 question" }, + { role: "assistant", content: "turn 1 answer" }, + { role: "user", content: "turn 2 question" }, + ], + }; +} + +describe("injectMemory system-must-be-first (#6135)", () => { + it("flags xiaomi-mimo (and alias mimo) as system-must-be-first", () => { + assert.equal(systemMessageMustBeFirst("xiaomi-mimo"), true); + assert.equal(systemMessageMustBeFirst("mimo"), true); + // default: unlisted providers keep current (non-first-constrained) behavior + assert.equal(systemMessageMustBeFirst("anthropic"), false); + assert.equal(systemMessageMustBeFirst(null), false); + }); + + it("keeps the injected system message at index 0 for a flagged provider even under cacheSafe", () => { + const out = injectMemory(multiTurn(), [mem("dark mode")], "xiaomi-mimo", { + cacheSafe: true, + }); + + // The system message must be first... + assert.equal( + out.messages.findIndex((m) => m.role === "system"), + 0 + ); + // ...and there must be NO system message at any index > 0. + const strayIdx = out.messages.findIndex((m, i) => i > 0 && m.role === "system"); + assert.equal(strayIdx, -1); + // Memory context is present in the leading system message. + assert.ok(out.messages[0].content.includes("Memory context")); + assert.ok(out.messages[0].content.includes("dark mode")); + }); + + it("merges memory into an existing leading system message (single system, still first)", () => { + const out = injectMemory(multiTurn(), [mem("dark mode")], "mimo", { + cacheSafe: true, + }); + // Exactly one system message, at index 0, carrying both memory + original. + const systemCount = out.messages.filter((m) => m.role === "system").length; + assert.equal(systemCount, 1); + assert.equal(out.messages[0].role, "system"); + assert.ok(out.messages[0].content.includes("Memory context")); + assert.ok(out.messages[0].content.includes("SYSTEM PROMPT")); + // Last user turn preserved at the tail. + assert.equal(out.messages[out.messages.length - 1].content, "turn 2 question"); + }); + + it("prepends a leading system message when there is no existing one (flagged provider)", () => { + const req: ChatRequest = { + model: "mimo-v2.5", + messages: [ + { role: "user", content: "turn 1 question" }, + { role: "assistant", content: "turn 1 answer" }, + { role: "user", content: "turn 2 question" }, + ], + }; + const out = injectMemory(req, [mem("dark mode")], "xiaomi-mimo", { cacheSafe: true }); + assert.equal(out.messages[0].role, "system"); + assert.ok(out.messages[0].content.includes("Memory context")); + assert.equal( + out.messages.findIndex((m, i) => i > 0 && m.role === "system"), + -1 + ); + }); + + it("regression: a NON-flagged provider keeps the existing cache-safe placement", () => { + const req = multiTurn(); + const out = injectMemory(req, [mem("dark mode")], "anthropic", { cacheSafe: true }); + // Existing behavior: memory inserted just before the last user message (index 3). + assert.equal(out.messages[3].role, "system"); + assert.ok(out.messages[3].content.includes("Memory context")); + assert.equal(out.messages[4].content, "turn 2 question"); + assert.equal(out.messages.length, 5); + }); +});