diff --git a/CHANGELOG.md b/CHANGELOG.md index db107f2..faf228e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ## Unreleased +- Stop silently dropping `role: "developer"` messages (for example OMP advisor steering notes, reminders, and nudges). `/alpha/generate` only accepts `user`, `assistant`, and `tool` roles, so developer messages are now forwarded as `user` messages with identical content in the same chronological position instead of disappearing from the request. + ## 0.6.0 - 2026-08-25 - Allow switching from a vision-capable model to a text-only model by omitting historical image tool results while preserving their text output; direct image prompts still fail clearly. diff --git a/package-lock.json b/package-lock.json index 126d407..e2df0a0 100644 --- a/package-lock.json +++ b/package-lock.json @@ -473,7 +473,7 @@ "version": "25.6.0", "resolved": "https://registry.npmjs.org/@types/node/-/node-25.6.0.tgz", "integrity": "sha512-+qIYRKdNYJwY3vRCZMdJbPLJAtGjQBudzZzdzwQYkEPQd+PJGixUL5QfvCLDaULoLv+RhT3LDkwEfKaAkgSmNQ==", - "devOptional": true, + "dev": true, "license": "MIT", "dependencies": { "undici-types": "~7.19.0" @@ -589,7 +589,7 @@ "version": "7.19.2", "resolved": "https://registry.npmjs.org/undici-types/-/undici-types-7.19.2.tgz", "integrity": "sha512-qYVnV5OEm2AW8cJMCpdV20CDyaN3g0AjDlOGf1OW4iaDEx8MwdtChUp4zu4H0VP3nDRF/8RKWH+IPp9uW0YGZg==", - "devOptional": true, + "dev": true, "license": "MIT" } } diff --git a/src/converters.ts b/src/converters.ts index 6cf71ac..0df5434 100644 --- a/src/converters.ts +++ b/src/converters.ts @@ -229,7 +229,12 @@ export function messagesToCC( const { callIds, resultIds } = toolCallState(messages) for (const message of messages ?? []) { - if (message.role === "user") { + if (message.role === "user" || message.role === "developer") { + // Hosts such as OMP steer the agent by injecting developer-role messages + // (advisor notes, reminders, nudges) mid-conversation. /alpha/generate + // only accepts user, assistant, and tool roles, so degrade the role to + // user instead of dropping the message. Content and chronological + // position are preserved; system-prompt hoisting would change semantics. out.push({ role: "user", content: userContentToCommandCode(message.content, allowImages), diff --git a/tests/fixtures/advisory-injector-extension.ts b/tests/fixtures/advisory-injector-extension.ts new file mode 100644 index 0000000..f88d5ed --- /dev/null +++ b/tests/fixtures/advisory-injector-extension.ts @@ -0,0 +1,41 @@ +/** + * Minimal OMP extension used by tests/test-omp-compat.mjs. + * + * Appends a custom advisor message to the session on session start, mirroring + * how the OMP Advisor runtime injects steering notes. OMP converts custom + * messages to `role: "developer"` LLM messages before handing them to the + * provider. Types are declared inline so the fixture has no runtime or + * typecheck dependency on OMP packages. + */ + +interface AdvisorInjectorSendMessage { + ( + message: { + customType: string + content: string + display: boolean + attribution: string + }, + options?: { triggerTurn?: boolean }, + ): void +} + +interface AdvisorInjectorApi { + on: (event: "session_start", handler: () => void | Promise) => void + sendMessage: AdvisorInjectorSendMessage +} + +export default function advisoryInjectorExtension(pi: AdvisorInjectorApi): void { + pi.on("session_start", async () => { + pi.sendMessage( + { + customType: "advisor", + content: + '\nStop and correct the benchmark.\n', + display: true, + attribution: "agent", + }, + { triggerTurn: false }, + ) + }) +} diff --git a/tests/test-omp-compat.mjs b/tests/test-omp-compat.mjs index ae64d1c..ced7c03 100644 --- a/tests/test-omp-compat.mjs +++ b/tests/test-omp-compat.mjs @@ -19,7 +19,10 @@ import { fileURLToPath } from "node:url" const __dirname = dirname(fileURLToPath(import.meta.url)) const PROJECT_DIR = resolve(__dirname, "..") const EXT_PATH = resolve(PROJECT_DIR, "index.ts") +const ADVISORY_EXT_PATH = resolve(PROJECT_DIR, "tests/fixtures/advisory-injector-extension.ts") const TEST_MODEL = "deepseek/deepseek-v4-flash" +const ADVISORY_XML = + '\nStop and correct the benchmark.\n' function findOmpBinary() { if (process.env.OMP_BIN) return process.env.OMP_BIN @@ -45,7 +48,11 @@ const tempHome = mkdtempSync(join(tmpdir(), "omp-cc-home-")) let requestCount = 0 let modelListRequestCount = 0 let lastRequestBody +let requestBodies = [] let lastRequestHeaders = {} +// When true the mock Provider API answers 403 upgrade_required so the +// transport router falls back to the legacy /alpha/generate transport. +let providerUpgradeRequired = false const server = createServer((req, res) => { if (req.method === "GET" && req.url === "/provider/v1/models") { @@ -77,7 +84,52 @@ const server = createServer((req, res) => { return } - if (req.method !== "POST" || req.url !== "/provider/v1/chat/completions") { + if (req.method === "POST" && req.url === "/provider/v1/chat/completions") { + requestCount += 1 + lastRequestHeaders = Object.fromEntries( + Object.entries(req.headers).map(([key, value]) => [ + key, + Array.isArray(value) ? value.join(", ") : (value ?? ""), + ]), + ) + + let body = "" + req.on("data", (chunk) => { + body += chunk.toString("utf-8") + }) + req.on("end", () => { + try { + lastRequestBody = JSON.parse(body) + requestBodies.push(lastRequestBody) + } catch { + lastRequestBody = undefined + } + + if (providerUpgradeRequired) { + res.writeHead(403, { "Content-Type": "application/json; charset=utf-8" }) + res.end(JSON.stringify({ error: { code: "upgrade_required" } })) + return + } + + res.writeHead(200, { + "Content-Type": "text/event-stream; charset=utf-8", + "Transfer-Encoding": "chunked", + }) + res.write( + `data: ${JSON.stringify({ id: "mock", object: "chat.completion.chunk", choices: [{ index: 0, delta: { role: "assistant", content: "mock-omp-ok" }, finish_reason: null }] })}\n\n`, + ) + res.write( + `data: ${JSON.stringify({ id: "mock", object: "chat.completion.chunk", choices: [{ index: 0, delta: {}, finish_reason: "stop" }] })}\n\n`, + ) + res.write( + `data: ${JSON.stringify({ id: "mock", object: "chat.completion.chunk", choices: [], usage: { prompt_tokens: 1, completion_tokens: 1, total_tokens: 2 } })}\n\n`, + ) + res.end("data: [DONE]\n\n") + }) + return + } + + if (req.method !== "POST" || req.url !== "/alpha/generate") { res.writeHead(404) res.end("Not found") return @@ -91,31 +143,27 @@ const server = createServer((req, res) => { ]), ) - let body = "" + let generateBody = "" req.on("data", (chunk) => { - body += chunk.toString("utf-8") + generateBody += chunk.toString("utf-8") }) req.on("end", () => { try { - lastRequestBody = JSON.parse(body) + lastRequestBody = JSON.parse(generateBody) + requestBodies.push(lastRequestBody) } catch { lastRequestBody = undefined } res.writeHead(200, { - "Content-Type": "text/event-stream; charset=utf-8", + "Content-Type": "text/plain; charset=utf-8", "Transfer-Encoding": "chunked", }) + res.write(`${JSON.stringify({ type: "text-delta", text: "mock-omp-ok" })}\n`) res.write( - `data: ${JSON.stringify({ id: "mock", object: "chat.completion.chunk", choices: [{ index: 0, delta: { role: "assistant", content: "mock-omp-ok" }, finish_reason: null }] })}\n\n`, + `${JSON.stringify({ type: "finish", finishReason: "stop", totalUsage: { inputTokens: 1, outputTokens: 1 } })}\n`, ) - res.write( - `data: ${JSON.stringify({ id: "mock", object: "chat.completion.chunk", choices: [{ index: 0, delta: {}, finish_reason: "stop" }] })}\n\n`, - ) - res.write( - `data: ${JSON.stringify({ id: "mock", object: "chat.completion.chunk", choices: [], usage: { prompt_tokens: 1, completion_tokens: 1, total_tokens: 2 } })}\n\n`, - ) - res.end("data: [DONE]\n\n") + res.end() }) }) @@ -176,7 +224,10 @@ try { const listOutput = result.stdout || result.stderr assert.match(listOutput, /commandcode/) assert.match(listOutput, /deepseek\/deepseek-v4-flash/) - assert.equal(modelListRequestCount, 1) + // The failed flag attempt may already load the extension and fetch the + // catalog once before the subcommand fallback runs, so only assert that + // the mock catalog was actually consulted. + assert.ok(modelListRequestCount >= 1) assert.doesNotThrow(() => accessSync(join(tempHome, ".omp", "agent", "commandcode-models.json"), constants.R_OK), ) @@ -184,6 +235,7 @@ try { console.log("[omp-compat] print mode through real extension and mock API") requestCount = 0 + requestBodies = [] const print = await runOmp( ["-e", EXT_PATH, "-p", "say mock token", "--model", `commandcode/${TEST_MODEL}`], 30_000, @@ -199,6 +251,64 @@ try { assert.equal(lastRequestBody?.model, TEST_MODEL) assert.ok(Array.isArray(lastRequestBody?.messages)) + console.log("[omp-compat] developer advisory reaches the legacy generate request body") + requestCount = 0 + requestBodies = [] + providerUpgradeRequired = true + const advisoryRun = await runOmp( + [ + "-e", + EXT_PATH, + "-e", + ADVISORY_EXT_PATH, + "-p", + "say mock token", + "--model", + `commandcode/${TEST_MODEL}`, + "--no-tools", + "--no-title", + ], + 30_000, + ) + assert.equal(advisoryRun.code, 0, advisoryRun.stderr) + assert.match(advisoryRun.stdout, /mock-omp-ok/) + + const promptBodies = requestBodies.filter((body) => + JSON.stringify(body?.params?.messages ?? []).includes("say mock token"), + ) + assert.ok(promptBodies.length >= 1, "expected at least one generate request with the prompt") + + for (const body of promptBodies) { + const messages = body?.params?.messages ?? [] + const advisoryMessages = messages.filter((message) => + JSON.stringify(message).includes("Stop and correct the benchmark."), + ) + assert.equal(advisoryMessages.length, 1, "the advisory should survive conversion exactly once") + const advisoryMessage = advisoryMessages[0] + assert.equal(advisoryMessage.role, "user") + const advisoryText = + typeof advisoryMessage.content === "string" + ? advisoryMessage.content + : (advisoryMessage.content ?? []) + .map((part) => (part?.type === "text" ? part.text : "")) + .join("\n") + assert.equal(advisoryText, ADVISORY_XML, "advisory content must arrive verbatim") + + const advisoryIndex = messages.indexOf(advisoryMessage) + const promptIndex = messages.findIndex((message) => + JSON.stringify(message).includes("say mock token"), + ) + assert.ok( + advisoryIndex < promptIndex, + "advisory must keep its chronological position relative to the prompt", + ) + assert.doesNotMatch( + String(body?.params?.system ?? ""), + /Stop and correct the benchmark| server.close(resolve)) diff --git a/tests/test-pure-functions.ts b/tests/test-pure-functions.ts index 96e742a..bc320d4 100644 --- a/tests/test-pure-functions.ts +++ b/tests/test-pure-functions.ts @@ -741,6 +741,89 @@ describe("messagesToCC()", () => { it("handles empty conversations", () => { assert.deepEqual(messagesToCC([]), []) }) + + it("keeps developer messages instead of dropping them", () => { + const result = messagesToCC([ + { role: "user", content: "start" }, + { role: "developer", content: "mid-conversation steering note" }, + ]) + + assert.deepEqual(result, [ + { role: "user", content: "start" }, + { role: "user", content: "mid-conversation steering note" }, + ]) + }) + + it("converts developer text parts with the same shape as user content", () => { + const result = messagesToCC([ + { + role: "developer", + content: [{ type: "text", text: "reminder one" }], + }, + ]) + + assert.deepEqual(result, [ + { + role: "user", + content: [{ type: "text", text: "reminder one" }], + }, + ]) + }) + + it("preserves advisory XML verbatim in the serialized request messages", () => { + const advisory = + '\nStop and correct the benchmark.\n' + const serialized = JSON.stringify(messagesToCC([{ role: "developer", content: advisory }])) + + assert.deepEqual(JSON.parse(serialized), [{ role: "user", content: advisory }]) + }) + + it("keeps developer advisories in chronological position without hoisting", () => { + const advisory = + '\nStop and correct the benchmark.\n' + const result = messagesToCC([ + { role: "user", content: "run the benchmark" }, + { + role: "assistant", + content: [ + { type: "text", text: "running it" }, + { type: "toolCall", id: "c1", name: "bash", arguments: { command: "bench" } }, + ], + }, + { + role: "toolResult", + toolCallId: "c1", + toolName: "bash", + content: [{ type: "text", text: "benchmark output" }], + }, + { role: "developer", content: advisory }, + { role: "user", content: "continue" }, + ]) + + assert.deepEqual(result, [ + { role: "user", content: "run the benchmark" }, + { + role: "assistant", + content: [ + { type: "text", text: "running it" }, + { type: "tool-call", toolCallId: "c1", toolName: "bash", input: { command: "bench" } }, + ], + }, + { + role: "tool", + content: [ + { + type: "tool-result", + toolCallId: "c1", + toolName: "bash", + output: { type: "text", value: "benchmark output" }, + }, + ], + }, + { role: "user", content: advisory }, + { role: "user", content: "continue" }, + ]) + }) }) describe("parseStreamEventLine()", () => { diff --git a/tests/test-stream.ts b/tests/test-stream.ts index a639980..040acb2 100644 --- a/tests/test-stream.ts +++ b/tests/test-stream.ts @@ -707,6 +707,56 @@ describe("streamCommandCode — request serialization", () => { assert.equal(headers["x-session-id"], undefined) }) + it("sends developer advisories as user messages in position, without system hoisting", async () => { + server.mockResponse({ + type: "success", + events: [JSON.stringify({ type: "finish", finishReason: "stop" })], + }) + const { streamCommandCode } = createTestDeps({ apiBase: server.baseUrl() }) + const advisory = + '\nStop and correct the benchmark.\n' + const context = makeContext({ + messages: [ + { role: "user", content: "run the benchmark" }, + { + role: "assistant", + content: [ + { type: "text", text: "running it" }, + { type: "toolCall", id: "c1", name: "bash", arguments: { command: "bench" } }, + ], + }, + { + role: "toolResult", + toolCallId: "c1", + toolName: "bash", + content: [{ type: "text", text: "benchmark output" }], + }, + { role: "developer", content: advisory }, + { role: "user", content: "continue" }, + ], + }) + + await collectEvents(streamCommandCode(makeModel(), context, { apiKey: "mock-key" })) + + const body = server.lastRequestBody() + assert.deepEqual( + objectAt(body, ["params", "messages", "3"]), + { role: "user", content: advisory }, + "developer advisory should arrive as an in-position user message with identical content", + ) + assert.deepEqual(objectAt(body, ["params", "messages", "4"]), { + role: "user", + content: "continue", + }) + assert.equal(objectAt(body, ["params", "messages", "5"]), undefined) + assert.equal( + objectAt(body, ["params", "system"]), + "You are a test assistant.", + "advisory must not be hoisted into the system prompt", + ) + assert.doesNotMatch(String(objectAt(body, ["params", "system"])), /advisory/) + }) + it("forwards explicit temperature and stable session metadata", async () => { server.mockResponse({ type: "success",