Skip to content
Closed
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
15 changes: 9 additions & 6 deletions open-sse/handlers/chatCore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1291,9 +1291,14 @@ function isCopilotClient(
export function extractSystemRoleMessages(payload: Record<string, unknown>): void {
if (!Array.isArray(payload.messages)) return;
const messages = payload.messages as Array<{ role?: unknown; content?: unknown }>;
const systemMessages = messages.filter(
(m) => typeof m.role === "string" && m.role.toLowerCase() === "system"
);
// Treat both `system` and `developer` as system-equivalent (OpenAI's Responses
// API renamed system → developer). Anthropic rejects either as a chat role, so
// both must be lifted into the top-level `system` field — parity with the
// normal-path extractSystemMessagesToBody closure.
const isSystemRole = (role: unknown): boolean =>
typeof role === "string" &&
(role.toLowerCase() === "system" || role.toLowerCase() === "developer");
Comment on lines +1298 to +1300

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

The isSystemRole helper logic is now duplicated with the internal extractSystemMessagesToBody closure found later in this file (lines 2670-2673). While the current implementation correctly fixes the missing developer role regression, consider moving this helper to a module-level scope or a shared utility file to maintain a single source of truth for system-equivalent roles and improve maintainability across the handler.

const systemMessages = messages.filter((m) => isSystemRole(m.role));
if (systemMessages.length === 0) return;

const extraBlocks: Array<Record<string, unknown>> = [];
Expand All @@ -1318,9 +1323,7 @@ export function extractSystemRoleMessages(payload: Record<string, unknown>): voi
payload.system = extraBlocks;
}
}
payload.messages = messages.filter(
(m) => typeof m.role !== "string" || m.role.toLowerCase() !== "system"
);
payload.messages = messages.filter((m) => !isSystemRole(m.role));
}

export async function handleChatCore({
Expand Down
53 changes: 36 additions & 17 deletions tests/unit/semantic-cache.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
import { describe, it } from "node:test";
import assert from "node:assert/strict";
import { generateSignature, isCacheable } from "../../src/lib/semanticCache.ts";
import {
generateSignature,
isCacheableForRead,
isCacheableForWrite,
} from "../../src/lib/semanticCache.ts";

describe("Semantic Cache", () => {
describe("generateSignature", () => {
Expand Down Expand Up @@ -69,35 +73,50 @@ describe("Semantic Cache", () => {
});
});

describe("isCacheable", () => {
it("returns true for non-streaming temp=0 requests", () => {
assert.equal(isCacheable({ stream: false, temperature: 0 }, null), true);
describe("isCacheableForRead", () => {
// #2536 superseded isCacheable with read/write variants that cache both
// streaming and non-streaming requests and require an explicit numeric
// temperature: 0 (omitted temperature is treated as non-deterministic).
it("returns true for temperature=0 (streaming or not)", () => {
assert.equal(isCacheableForRead({ stream: false, temperature: 0 }, null), true);
assert.equal(isCacheableForRead({ stream: true, temperature: 0 }, null), true);
});

it("returns true when temperature is undefined (defaults to 0)", () => {
assert.equal(isCacheable({ stream: false }, null), true);
});

it("returns false for streaming requests", () => {
assert.equal(isCacheable({ stream: true, temperature: 0 }, null), false);
});

it("returns false when stream is not explicitly false", () => {
assert.equal(isCacheable({ temperature: 0 }, null), false);
it("returns false when temperature is omitted (provider default may be non-deterministic)", () => {
assert.equal(isCacheableForRead({ stream: false }, null), false);
});

it("returns false for non-zero temperature", () => {
assert.equal(isCacheable({ stream: false, temperature: 0.7 }, null), false);
assert.equal(isCacheableForRead({ temperature: 0.7 }, null), false);
});

it("returns false when no-cache header is set", () => {
const headers = new Headers({ "x-omniroute-no-cache": "true" });
assert.equal(isCacheable({ stream: false, temperature: 0 }, headers), false);
assert.equal(isCacheableForRead({ temperature: 0 }, headers), false);
});

it("returns true when no-cache header is absent", () => {
const headers = new Headers({});
assert.equal(isCacheable({ stream: false, temperature: 0 }, headers), true);
assert.equal(isCacheableForRead({ temperature: 0 }, headers), true);
});
});

describe("isCacheableForWrite", () => {
it("returns true for temperature=0 responses", () => {
assert.equal(isCacheableForWrite({ temperature: 0 }, null), true);
});

it("returns false when temperature is omitted", () => {
assert.equal(isCacheableForWrite({ stream: false }, null), false);
});

it("returns false for non-zero temperature", () => {
assert.equal(isCacheableForWrite({ temperature: 0.7 }, null), false);
});

it("returns false when no-cache header is set", () => {
const headers = new Headers({ "x-omniroute-no-cache": "true" });
assert.equal(isCacheableForWrite({ temperature: 0 }, headers), false);
});
});
});
18 changes: 18 additions & 0 deletions tests/unit/system-role-extraction.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,24 @@ test("extractSystemRoleMessages moves role=system to top-level system", () => {
assert.deepEqual(payload.system, [{ type: "text", text: "Memory context: foo" }]);
});

test("extractSystemRoleMessages also lifts role=developer (OpenAI Responses system alias)", () => {
const payload = {
messages: [
{ role: "developer", content: "Dev instructions" },
{ role: "system", content: "Sys context" },
{ role: "user", content: "hello" },
],
};
extractSystemRoleMessages(payload);
// both developer and system are removed from messages and lifted into system
assert.equal(payload.messages.length, 1);
assert.equal(payload.messages[0].role, "user");
assert.deepEqual(payload.system, [
{ type: "text", text: "Dev instructions" },
{ type: "text", text: "Sys context" },
]);
});

test("extractSystemRoleMessages merges with existing top-level system string", () => {
const payload = {
system: "You are Claude.",
Expand Down