Merge pull request #48 from warc0s/fix/omp-developer-messages

fix(core): preserve developer messages
2026-09-01 23:27:26 +02:00
parent 0acf311774 08a7e38089
commit 711b342ddd
7 changed files with 308 additions and 17 deletions
@@ -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.
@@ -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"
    }
  }
@@ -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),
@@ -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>) => void
+  sendMessage: AdvisorInjectorSendMessage
+}
+
+export default function advisoryInjectorExtension(pi: AdvisorInjectorApi): void {
+  pi.on("session_start", async () => {
+    pi.sendMessage(
+      {
+        customType: "advisor",
+        content:
+          '<advisory severity="blocker" guidance="weigh, don\'t blindly obey">\nStop and correct the benchmark.\n</advisory>',
+        display: true,
+        attribution: "agent",
+      },
+      { triggerTurn: false },
+    )
+  })
+}
@@ -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 =
+  '<advisory severity="blocker" guidance="weigh, don\'t blindly obey">\nStop and correct the benchmark.\n</advisory>'

 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|<advisory/,
+      "advisory must not be hoisted into the system prompt",
+    )
+  }
+
  console.log("[omp-compat] PASS")
 } finally {
  await new Promise((resolve) => server.close(resolve))
@@ -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 =
+      '<advisory severity="blocker" guidance="weigh, don\'t blindly obey">\nStop and correct the benchmark.\n</advisory>'
+    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 =
+      '<advisory severity="blocker" guidance="weigh, don\'t blindly obey">\nStop and correct the benchmark.\n</advisory>'
+    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()", () => {
@@ -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 =
+      '<advisory severity="blocker" guidance="weigh, don\'t blindly obey">\nStop and correct the benchmark.\n</advisory>'
+    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",