diff --git a/packages/opencode/src/inbox/inbox.ts b/packages/opencode/src/inbox/inbox.ts index e55c1f6d6..2d0eb99ba 100644 --- a/packages/opencode/src/inbox/inbox.ts +++ b/packages/opencode/src/inbox/inbox.ts @@ -248,6 +248,44 @@ export const layer: Layer.Layer< return 0 } + // Render BEFORE writing anything, and drop any row that renders blank. + // The drain is the one user-message producer that does NOT go through + // SessionPrompt.createUserMessage, so `hasSubstantiveContent` never sees + // it — a blank render would persist a user message whose only part is + // {type:"text",text:""}. The AI SDK's user branch filters empty text + // parts out with NO backfill, so that message reaches the provider as + // `content: []` and is rejected ("user messages must have non-empty + // content"). renderInboxRow already substitutes a placeholder for a + // blank body; this is the structural invariant that keeps the shape + // unreachable no matter what any future row type renders. + const rendered = rows.flatMap((row) => { + const text = renderInboxRow(row) + if (text.trim().length > 0) return [{ row, text }] + log.warn("inbox.drain: dropping row that rendered blank (would produce an empty user text part)", { + sessionID, + actorID, + rowID: row.id, + type: row.type, + }) + return [] + }) + + // Every row rendered blank: consume them (they carry no information and + // must not be re-drained forever) without writing a message at all. A + // zero-part user message would be skipped downstream anyway, so writing + // one is pure litter. + if (rendered.length === 0) { + yield* Effect.sync(() => + Database.use((db) => + db + .delete(InboxTable) + .where(inArray(InboxTable.id, rows.map((r) => r.id))) + .run(), + ), + ) + return 0 + } + // Non-transactional crash window: updateMessage + updatePart commit // before the inbox DELETE. A crash between them re-renders the same // rows on next drain — LLM sees duplicated notifications. Tolerable; @@ -265,14 +303,14 @@ export const layer: Layer.Layer< agent: seed.agent, model: seed.model, }) - for (const row of rows) { + for (const entry of rendered) { yield* sessions.updatePart({ id: PartID.ascending(), messageID: msgID, sessionID, type: "text" as const, synthetic: true, - text: renderInboxRow(row), + text: entry.text, }) } yield* Effect.sync(() => @@ -284,7 +322,7 @@ export const layer: Layer.Layer< ), ) - return rows.length + return rendered.length }) const impl = Service.of({ send, drain }) diff --git a/packages/opencode/src/inbox/render.ts b/packages/opencode/src/inbox/render.ts index aa4c2df2c..3fd6498e3 100644 --- a/packages/opencode/src/inbox/render.ts +++ b/packages/opencode/src/inbox/render.ts @@ -1,11 +1,21 @@ import type { InboxRow } from "./inbox.sql" +// A blank body must fall back to the placeholder, not just a missing one. +// `?? placeholder` only catches null/undefined, but a blank body is stored as +// "" (or whitespace) — and for actor_notification the body is passed through +// RAW, so "" would become a user text part with text:"". The AI SDK's user +// branch filters empty text parts out with no backfill, leaving `content: []` +// and a provider 400 ("user messages must have non-empty content"). +function blankTo(text: string | undefined, placeholder: string) { + return text !== undefined && text.trim().length > 0 ? text : placeholder +} + export function renderInboxRow(row: InboxRow): string { if (row.type === "actor_notification") { // Pre-rendered notification text — sender produced the full // ... wrapper. const content = row.content as { text?: string } - return content.text ?? "(no notification body)" + return blankTo(content.text, "(no notification body)") } // Default: type === "text" or unknown — wrap as element so // the LLM can route by sender; the wrapper format mirrors the @@ -15,7 +25,7 @@ export function renderInboxRow(row: InboxRow): string { ? `${row.sender_session_id}:${row.sender_actor_id ?? "?"}` : "system" const sentAt = new Date(row.created_at).toISOString() - return `\n${content.text ?? "(empty)"}\n` + return `\n${blankTo(content.text, "(empty)")}\n` } export function renderActorNotification(event: { diff --git a/packages/opencode/src/provider/transform.ts b/packages/opencode/src/provider/transform.ts index 19b0ec80e..48f8e5a00 100644 --- a/packages/opencode/src/provider/transform.ts +++ b/packages/opencode/src/provider/transform.ts @@ -47,6 +47,28 @@ function sdkKey(npm: string): string | undefined { return undefined } +// Providers that hard-reject an empty text/reasoning BLOCK inside an otherwise +// non-empty message ("text content blocks must be non-empty"). The AI SDK's own +// filter does not save us here: its user branch drops empty text parts, but its +// assistant branch KEEPS an empty text part that carries providerOptions, and it +// never inspects `reasoning` parts at all — so an empty reasoning block reaches +// the provider untouched for every npm package. +// +// The original list was `@ai-sdk/anthropic` + `@ai-sdk/amazon-bedrock` only, +// which missed the two other ways to reach the same Anthropic API: +// `@ai-sdk/google-vertex/anthropic` and Claude via `@openrouter/ai-sdk-provider`. +// Stripping an empty block is information-preserving for any provider, so the +// list errs on the side of including a provider rather than excluding one. +function stripsEmptyParts(model: Provider.Model): boolean { + return [ + "@ai-sdk/anthropic", + "@ai-sdk/amazon-bedrock", + "@ai-sdk/google-vertex/anthropic", + "@openrouter/ai-sdk-provider", + "@ai-sdk/openai-compatible", + ].includes(model.api.npm) +} + function normalizeMessages( msgs: ModelMessage[], model: Provider.Model, @@ -54,7 +76,7 @@ function normalizeMessages( ): ModelMessage[] { // Anthropic rejects messages with empty content - filter out empty string messages // and remove empty text/reasoning parts from array content - if (model.api.npm === "@ai-sdk/anthropic" || model.api.npm === "@ai-sdk/amazon-bedrock") { + if (stripsEmptyParts(model)) { msgs = msgs .map((msg) => { if (typeof msg.content === "string") { @@ -270,6 +292,92 @@ function supportsCacheMarkers(model: Provider.Model): boolean { // not an assistant prefill. const CONTINUATION_PROMPT = "Continue." +// Backfill text for a message whose content is structurally present but carries +// nothing a provider will accept. Same string as the continuation prompt: both +// mean "there is no new instruction here, keep going". +const EMPTY_CONTENT_PLACEHOLDER = CONTINUATION_PROMPT + +// Mirrors the AI SDK's OWN user-content filter. `ai@6` builds the wire payload in +// `convertToLanguageModelMessage`, whose user branch is: +// +// content: message.content.map((part) => convertPartToLanguageModelPart(part, ...)) +// .filter((part) => part.type !== "text" || part.text !== "") +// +// It runs AFTER every transform here and does NOT backfill, so a user message +// whose only text part is "" reaches the provider as `content: []` — exactly the +// shape observed in the live Bedrock 400 payload. Note the asymmetry: the SDK's +// assistant branch keeps an empty text part when it carries providerOptions +// (`|| part.providerOptions != null`); the user branch has no such escape, so +// even an empty text part holding a cache_control marker is stripped. +// +// Consequence: emptiness of a user message CANNOT be judged by `content.length` +// — at this layer the offending message is a length-1 array that looks fine. It +// must be judged by what SURVIVES this filter. +function sdkVisibleUserParts(content: readonly any[]): readonly any[] { + return content.filter((part) => !part || part.type !== "text" || part.text !== "") +} + +// The SDK's ASSISTANT branch uses a slightly looser predicate — an empty text +// part survives when it carries providerOptions: +// .filter((part) => part.type !== "text" || part.text !== "" || part.providerOptions != null) +// so assistant emptiness has to be judged against that rule, not the user one. +function sdkVisibleAssistantParts(content: readonly any[]): readonly any[] { + return content.filter( + (part) => !part || part.type !== "text" || part.text !== "" || part.providerOptions != null, + ) +} + +// True when a message will reach the provider with no usable content. +function hasNoSendableContent(msg: ModelMessage): boolean { + const content = msg.content as unknown + if (typeof content === "string") return content === "" + if (!Array.isArray(content)) return true + // Judge each role by the SDK's own post-filter view (see the notes above). + if (msg.role === "user") return sdkVisibleUserParts(content).length === 0 + if (msg.role === "assistant") return sdkVisibleAssistantParts(content).length === 0 + return content.length === 0 +} + +// THE global pre-send content invariant: no message may reach the provider with +// empty content. This layer never existed before — `normalizeContentArray` only +// guards content SHAPE, `normalizeMessages` only strips empty parts and only for +// `@ai-sdk/anthropic`/`@ai-sdk/amazon-bedrock` (so a Bedrock-backed gateway on any +// other npm got no protection at all), and `ensureTrailingUserMessage` inspects +// only the trailing assistant. An empty user message fell through all three seams +// and produced `messages.: user messages must have non-empty content`. +// +// Policy is per-role and deliberately asymmetric: +// - user → BACKFILL a minimal non-empty text turn. Dropping it would make +// the request end with an assistant message, which Bedrock rejects +// as a prefill — trading this 400 for the prefill 400. +// - assistant → DROP. It is residue with nothing to preserve, and the trailing +// user guard that runs next re-establishes the prefill invariant. +// - tool → LEAVE UNTOUCHED. A tool message's content must be `tool-result` +// blocks keyed to a preceding `tool-call`; we cannot synthesize a +// valid one, and injecting text would break tool_use/tool_result +// pairing (a different 400). The SDK's empty-text filter does not +// apply to the tool branch, and no empty tool message exists in +// any observed transcript, so there is nothing to repair here. +// +// Provider-agnostic on purpose: the AI SDK applies its stripping filter for every +// provider, so gating this on an npm package name is what created the hole. +export function ensureNonEmptyContent(msgs: ModelMessage[]): ModelMessage[] { + const result: ModelMessage[] = [] + for (const msg of msgs) { + if (!hasNoSendableContent(msg)) { + result.push(msg) + continue + } + if (msg.role === "assistant") continue + if (msg.role === "tool") { + result.push(msg) + continue + } + result.push({ ...msg, content: [{ type: "text", text: EMPTY_CONTENT_PLACEHOLDER }] } as ModelMessage) + } + return result +} + // True when an assistant ModelMessage carries no renderable content (no text and // no tool-call) — pure residue we can drop without losing anything. function isEmptyAssistant(msg: ModelMessage): boolean { @@ -292,17 +400,46 @@ function isEmptyAssistant(msg: ModelMessage): boolean { // user turn is appended so the list ends with a user message. Runs at the // pre-send choke point in `message()`, so it also self-heals history that // already ends in an assistant turn. +// +// ORDERING CONTRACT: `ensureNonEmptyContent` MUST run before this function. +// This guard only establishes "the list ends with user/tool"; it says nothing +// about whether that trailing message has usable content. Running it first and +// resolving emptiness second would let this function return a list ending in an +// empty user message (which is what shipped, and what produced the live 400), +// and resolving emptiness afterwards could drop that message again and re-open +// the prefill 400. Emptiness first, prefill second — the two cannot fight. export function ensureTrailingUserMessage(msgs: ModelMessage[]): ModelMessage[] { // Drop only trailing EMPTY assistant residue (nothing to preserve). let end = msgs.length while (end > 0 && isEmptyAssistant(msgs[end - 1])) end-- const trimmed = end === msgs.length ? msgs : msgs.slice(0, end) const last = trimmed[trimmed.length - 1] - // Already ends with user or tool (or empty) — safe to send as-is. + // Already ends with a user or tool message, so this is not a prefill. Their + // content is guaranteed non-empty by `ensureNonEmptyContent` (see the ordering + // contract above) — an empty trailing message is NOT safe to send as-is. if (!last || last.role !== "assistant") return trimmed // A content-bearing assistant is legitimately last: keep it and append a // minimal user turn so the request ends with a user message. - return [...trimmed, { role: "user", content: CONTINUATION_PROMPT }] + // + // The content MUST be an array of parts, never a bare string. `message()` is + // typed for `ModelMessage[]` (where `content: string` is legal) but it does not + // run on `ModelMessage[]` — it runs inside the `wrapLanguageModel` middleware on + // `args.params.prompt`, a `LanguageModelV3Prompt`, whose user content is + // `Array`. That mismatch is silenced by the + // `@ts-expect-error` at session/llm.ts:670 (and session/prompt.ts:596). + // + // A bare string there is not merely untidy, it is THE producer of the 400: + // @ai-sdk/anthropic's user branch does `for (let j = 0; j < content.length; j++)` + // and `switch (part.type)` with cases for only `text`/`file` and NO default + // (dist/index.mjs:2320-2408 on 3.0.82), so a string is iterated as individual + // characters whose `.type` is `undefined`, nothing is pushed, and the message + // goes out as `{"role":"user","content":[]}` — the exact trailing message in the + // observed failing request. + // + // `ensureNonEmptyContent` cannot save this: per the ordering contract it runs + // BEFORE this function, and "Continue." is not empty by any predicate, so the + // append is never re-inspected. + return [...trimmed, { role: "user", content: [{ type: "text", text: CONTINUATION_PROMPT }] } as ModelMessage] } // Hard prune of the trailing assistant run, discarding its content. Unlike @@ -454,17 +591,36 @@ function applyCaching(msgs: ModelMessage[], model: Provider.Model): ModelMessage return msgs } -// Minimal crash guard: ensure msg.content is never a non-string non-array value -// (object, undefined, null) that would blow up downstream `.map()` calls. +// Minimal crash guard: for the roles it can repair, ensure msg.content is never a +// non-string non-array value (object, undefined, null) that would blow up +// downstream `.map()` calls. NOT a blanket guarantee — `tool` is deliberately +// exempt (see below), so downstream code must still not assume array content. // Strings are valid ModelMessage content (the AI SDK accepts content: string | -// Array) and are left untouched. Only genuinely-invalid types are normalized -// to a safe empty array so every downstream path can safely call `.map()`. +// Array) and are left untouched. Only genuinely-invalid types are normalized. +// +// Invalid content is BACKFILLED, not blanked, for roles the provider requires to +// be non-empty. Emitting `content: []` here would trade a crash for a 400 +// ("user messages must have non-empty content"), and blanking a user turn also +// re-opens the trailing-assistant prefill 400 once the empty message is dropped +// downstream. An assistant gets `[]` because it carries no obligation: the +// non-empty invariant drops empty assistant residue and the trailing-user guard +// then re-establishes the prefill invariant. +// +// A `tool` message is left EXACTLY as-is, matching ensureNonEmptyContent's +// per-role policy: injecting a text part into a tool message breaks tool_use / +// tool_result pairing, which trades one 400 for another, and emitting `content: +// []` is itself illegal for a tool result. Only `user` gets the backfill. +// "Exactly as-is" is the load-bearing part: `[]` is NOT an acceptable substitute, +// and leaving the value untouched is what makes this guard and +// `ensureNonEmptyContent` reach the same outcome on the same input +// (`hasNoSendableContent` returns true for non-array content, and the tool branch +// there re-pushes the message unchanged). function normalizeContentArray(msgs: ModelMessage[]): ModelMessage[] { return msgs.map((msg) => { if (typeof msg.content === "string" || Array.isArray(msg.content)) return msg - // object / undefined / null — not a valid ModelMessage content shape; - // wrap in an empty array so .map() downstream never throws. - return { ...msg, content: [] } as ModelMessage + if (msg.role === "assistant") return { ...msg, content: [] } as ModelMessage + if (msg.role === "user") return { ...msg, content: [{ type: "text", text: EMPTY_CONTENT_PLACEHOLDER }] } as ModelMessage + return msg }) } @@ -810,6 +966,11 @@ export function message(msgs: ModelMessage[], model: Provider.Model, options: Re msgs = limitImages(msgs, model) msgs = normalizeMessages(msgs, model, options) msgs = forceAnthropicReasoningContent(msgs, model) + // Ordering is load-bearing (see ensureTrailingUserMessage's ordering contract): + // resolve EMPTY content first, then the trailing-assistant/prefill invariant. + // Emptiness is provider-agnostic because the AI SDK strips empty user text + // parts for every provider, downstream of everything here. + msgs = ensureNonEmptyContent(msgs) // SAFE prefill guard: never let the request end with an assistant (prefill) // message a provider (e.g. Bedrock) would reject, without deleting a completed // reply. Drops only empty residue; appends a continuation user turn otherwise. diff --git a/packages/opencode/src/session/goal.ts b/packages/opencode/src/session/goal.ts index 19a986cf8..4bba0e3de 100644 --- a/packages/opencode/src/session/goal.ts +++ b/packages/opencode/src/session/goal.ts @@ -158,7 +158,16 @@ export const layer = Layer.effect( // Convert the conversation to native model messages so the judge sees the // real tool calls/results/images — same context the working agent had. - const conversation = yield* MessageV2.toModelMessagesEffect(input.msgs, resolved) + // + // `ensureNonEmptyContent` is applied by hand here because this is the ONE + // persisted-parts→provider site that does not run `ProviderTransform.message`: + // `model: language` below is the RAW model, with no `wrapLanguageModel` and no + // middleware anywhere in this file, so the pre-send invariant that every other + // build site inherits from the middleware would otherwise be absent. An empty + // user message here reaches the judge's provider unrepaired. + const conversation = ProviderTransform.ensureNonEmptyContent( + yield* MessageV2.toModelMessagesEffect(input.msgs, resolved), + ) // Diagnostic: dump the FULL message array sent to the judge. Long strings // (e.g. base64 image data) are clipped with a length marker so the log diff --git a/packages/opencode/src/session/llm.ts b/packages/opencode/src/session/llm.ts index 5c3b32ac5..15ee3670c 100644 --- a/packages/opencode/src/session/llm.ts +++ b/packages/opencode/src/session/llm.ts @@ -667,7 +667,12 @@ const live: Layer.Layer< { specificationVersion: "v3" as const, async transformParams(args) { - if (args.type === "stream") { + // `generate || stream`, matching session/prompt.ts:597. This file's + // only SDK entrypoint is `streamText` (:599), so narrowing to + // "stream" is not an active hole today — but it would silently drop + // the whole transform, including the empty-content invariant, the + // moment a non-streaming call is added here. + if (args.type === "generate" || args.type === "stream") { // @ts-expect-error args.params.prompt = ProviderTransform.message(args.params.prompt, input.model, options) } diff --git a/packages/opencode/src/tool/actor.ts b/packages/opencode/src/tool/actor.ts index bd9b4bf63..81edc6118 100644 --- a/packages/opencode/src/tool/actor.ts +++ b/packages/opencode/src/tool/actor.ts @@ -178,6 +178,24 @@ const mapActorVerb = Effect.fn("mapActorVerb")(function* (verb: string | undefin const { flags, rest } = yield* extractNamedFlags(args, ["session", "type"], line) if (rest.length !== 2) return yield* actorArityError("send", ' "" [--session ] [--type ]', rest, line) + // NOT the layer that makes a blank body unreachable — `parameters` DOES + // re-validate a shell-parsed op. shell-wrap.ts calls `def.execute(parsed)` + // on the def produced by Tool.init, which is wrap()-decorated, and wrap() + // runs `parameters.parse(args)` inside execute — so `content: + // z.string().min(1)` already rejects `actor send x ""` (verified: with + // this guard removed the shell route still enqueues nothing and reports + // `Too small: expected string to have >=1 characters → at + // operation.content`). + // + // This guard earns its place for two other reasons: it turns that generic + // zod dump into one specific, teachable message, and `.trim()` also + // rejects whitespace-only bodies, which `min(1)` accepts. + if (rest[1].trim() === "") + return yield* Effect.fail({ + kind: "flag" as const, + line, + detail: "actor: send: content must not be empty", + }) return { operation: { action: "send" as const, diff --git a/packages/opencode/test/inbox/drain-no-empty-part.test.ts b/packages/opencode/test/inbox/drain-no-empty-part.test.ts new file mode 100644 index 000000000..8c499e1f5 --- /dev/null +++ b/packages/opencode/test/inbox/drain-no-empty-part.test.ts @@ -0,0 +1,222 @@ +import { afterEach, describe, expect, test } from "bun:test" +import { Layer, ManagedRuntime } from "effect" +import { Inbox } from "../../src/inbox" +import { renderInboxRow } from "../../src/inbox/render" +import type { InboxRow } from "../../src/inbox/inbox.sql" +import { defaultModelRef } from "../../src/inbox/inbox-ref" +import { ActorRegistry } from "../../src/actor/registry" +import { Session } from "../../src/session" +import { Bus } from "../../src/bus" +import { Instance } from "../../src/project/instance" +import { MessageID, SessionID } from "../../src/session/schema" +import { ProviderID, ModelID } from "../../src/provider/schema" +import { tmpdir } from "../fixture/fixture" + +// The inbox drain is the ONE user-message producer that does not go through +// SessionPrompt.createUserMessage, so `hasSubstantiveContent` never inspects +// it. A blank `actor_notification` body would therefore be persisted verbatim +// as a user message whose only part is {type:"text",text:""} — and ai@6's +// `convertToLanguageModelMessage` user branch filters empty text parts out with +// NO backfill, so that message would reach the provider as `content: []` and be +// rejected with `messages.: user messages must have non-empty content`. +// +// No caller can supply such a body today (see +// empty-notification-reachability.test.ts — the actor tool's shell route IS +// re-validated against `content: z.string().min(1)`), so this closes a latent +// defence gap. These tests pin the structural invariant that keeps the shape +// unreachable regardless of what any future row type renders: the drain never +// persists a blank text part, for any row content. + +const base = Layer.mergeAll(Session.defaultLayer, ActorRegistry.defaultLayer, Bus.defaultLayer) +const testLayer = Inbox.layer.pipe(Layer.provide(base), Layer.provideMerge(base)) + +afterEach(async () => { + defaultModelRef.current = undefined + await Instance.disposeAll() +}) + +type RT = ManagedRuntime.ManagedRuntime + +async function withInbox(directory: string, fn: (rt: RT) => Promise) { + return Instance.provide({ + directory, + fn: async () => { + const rt = ManagedRuntime.make(testLayer) + try { + await fn(rt) + } finally { + await rt.dispose() + } + }, + }) +} + +async function seedRealMessage(rt: RT, sessionID: SessionID, actorID: string) { + return rt.runPromise( + Session.Service.use((sessions) => + sessions.updateMessage({ + id: MessageID.ascending(), + role: "user" as const, + sessionID, + agentID: actorID, + time: { created: Date.now() }, + agent: "general", + model: { providerID: ProviderID.make("test"), modelID: ModelID.make("test-model") }, + }), + ), + ) +} + +async function registerActor(rt: RT, sessionID: SessionID, actorID: string) { + return rt.runPromise( + ActorRegistry.Service.use((reg) => + reg.register({ + sessionID, + actorID, + mode: "subagent", + parentActorID: undefined, + agent: "general", + description: "test", + contextMode: "none", + contextWatermark: undefined, + background: false, + lifecycle: "ephemeral", + }), + ), + ) +} + +function row(overrides: Partial): InboxRow { + return { + id: "01JTESTROW", + receiver_session_id: SessionID.make("ses_receiver"), + receiver_actor_id: "main", + sender_session_id: SessionID.make("ses_sender"), + sender_actor_id: "explore-1", + type: "text", + content: { text: "hello" }, + created_at: 1_700_000_000_000, + ...overrides, + } as InboxRow +} + +describe("renderInboxRow never returns a blank string", () => { + // `content.text ?? placeholder` only catches null/undefined. An empty body is + // stored as "" and, for actor_notification, passed through RAW. + test("actor_notification with an empty body falls back to the placeholder", () => { + const rendered = renderInboxRow(row({ type: "actor_notification", content: { text: "" } })) + expect(rendered).toBe("(no notification body)") + expect(rendered.trim().length).toBeGreaterThan(0) + }) + + test("actor_notification with a whitespace-only body falls back to the placeholder", () => { + expect(renderInboxRow(row({ type: "actor_notification", content: { text: " \n\t " } }))).toBe( + "(no notification body)", + ) + }) + + test("actor_notification with a missing body still falls back", () => { + expect(renderInboxRow(row({ type: "actor_notification", content: {} }))).toBe("(no notification body)") + }) + + test("actor_notification with a real body is passed through verbatim", () => { + const body = "\nchild completed.\n" + expect(renderInboxRow(row({ type: "actor_notification", content: { text: body } }))).toBe(body) + }) + + test("a text row with an empty body renders the (empty) placeholder inside the wrapper", () => { + const rendered = renderInboxRow(row({ type: "text", content: { text: "" } })) + expect(rendered).toContain("(empty)") + expect(rendered.trim().length).toBeGreaterThan(0) + }) +}) + +describe("Inbox.drain never persists an empty user text part", () => { + test("a blank actor_notification body yields a non-blank synthetic part", async () => { + await using tmp = await tmpdir({ git: true }) + await withInbox(tmp.path, async (rt) => { + const session = await rt.runPromise(Session.Service.use((s) => s.create())) + await registerActor(rt, session.id, "actor-empty") + await seedRealMessage(rt, session.id, "actor-empty") + + // This is the reachable producer: a blank body reaches the inbox (the + // `actor` tool's JSON path guards it with .min(1), but the shell path did + // not, and inbox.send itself does not validate). + await rt.runPromise( + Inbox.Service.use((inbox) => + inbox.send({ + receiverSessionID: session.id, + receiverActorID: "actor-empty", + type: "actor_notification", + content: "", + }), + ), + ) + + const count = await rt.runPromise(Inbox.Service.use((inbox) => inbox.drain(session.id, "actor-empty"))) + // The notification is NOT lost — it is rendered with a placeholder. + expect(count).toBe(1) + + const msgs = await rt.runPromise( + Session.Service.use((sessions) => sessions.messages({ sessionID: session.id, agentID: "actor-empty" })), + ) + const synthetic = msgs + .filter((m) => m.info.role === "user") + .flatMap((m) => m.parts) + .filter((p) => p.type === "text" && p.synthetic) + + expect(synthetic.length).toBe(1) + // THE INVARIANT: no persisted user text part may be empty or blank. + for (const part of synthetic) { + expect(part.type === "text" && part.text).not.toBe("") + expect(part.type === "text" && part.text.trim().length).toBeGreaterThan(0) + } + }) + }) + + test("a blank body mixed with a real one keeps both parts non-blank", async () => { + await using tmp = await tmpdir({ git: true }) + await withInbox(tmp.path, async (rt) => { + const session = await rt.runPromise(Session.Service.use((s) => s.create())) + await registerActor(rt, session.id, "actor-mixed") + await seedRealMessage(rt, session.id, "actor-mixed") + + await rt.runPromise( + Inbox.Service.use((inbox) => + inbox.send({ + receiverSessionID: session.id, + receiverActorID: "actor-mixed", + type: "actor_notification", + content: "", + }), + ), + ) + await rt.runPromise( + Inbox.Service.use((inbox) => + inbox.send({ + receiverSessionID: session.id, + receiverActorID: "actor-mixed", + type: "actor_notification", + content: "\nreal body\n", + }), + ), + ) + + const count = await rt.runPromise(Inbox.Service.use((inbox) => inbox.drain(session.id, "actor-mixed"))) + expect(count).toBe(2) + + const msgs = await rt.runPromise( + Session.Service.use((sessions) => sessions.messages({ sessionID: session.id, agentID: "actor-mixed" })), + ) + const texts = msgs + .filter((m) => m.info.role === "user") + .flatMap((m) => m.parts) + .filter((p) => p.type === "text" && p.synthetic) + .map((p) => (p.type === "text" ? p.text : "")) + + expect(texts.length).toBe(2) + expect(texts.every((t) => t.trim().length > 0)).toBe(true) + expect(texts).toContain("(no notification body)") + }) + }) +}) diff --git a/packages/opencode/test/inbox/empty-notification-part.test.ts b/packages/opencode/test/inbox/empty-notification-part.test.ts new file mode 100644 index 000000000..285c98dfd --- /dev/null +++ b/packages/opencode/test/inbox/empty-notification-part.test.ts @@ -0,0 +1,205 @@ +import { afterEach, describe, expect, test } from "bun:test" +import { Layer, ManagedRuntime } from "effect" +import { Inbox } from "../../src/inbox" +import { renderInboxRow } from "../../src/inbox/render" +import { defaultModelRef } from "../../src/inbox/inbox-ref" +import type { InboxRow } from "../../src/inbox/inbox.sql" +import { ActorRegistry } from "../../src/actor/registry" +import { Session } from "../../src/session" +import { Bus } from "../../src/bus" +import { Instance } from "../../src/project/instance" +import { MessageID, SessionID } from "../../src/session/schema" +import { ProviderID, ModelID } from "../../src/provider/schema" +import { tmpdir } from "../fixture/fixture" + +// Latent defence gap in the inbox render/drain path (NOT an observed producer — +// see empty-notification-reachability.test.ts: the actor tool's shell route is +// re-validated against `content: z.string().min(1)`, so no caller could supply a +// blank body). +// +// Inbox.drain writes ONE synthetic `role:"user"` message and then one text part +// per queued row, with `text: renderInboxRow(row)` persisted verbatim — +// bypassing createUserMessage/hasSubstantiveContent entirely. renderInboxRow +// used `content.text ?? "(no notification body)"`, and `??` does not catch `""`, +// so a body-less `actor_notification` row would render to exactly `""`. With a +// single queued row that yields `parts: [{type:"text",text:""}]` — length 1, so +// every `parts.length === 0` guard misses it — which `ai`'s +// convertToLanguageModelMessage then filters to `content: []`, the shape a +// provider rejects with "user messages must have non-empty content". +// +// See test/session/message-v2.test.ts for the SDK-boundary half of the proof. + +const base = Layer.mergeAll(Session.defaultLayer, ActorRegistry.defaultLayer, Bus.defaultLayer) +const testLayer = Inbox.layer.pipe(Layer.provide(base), Layer.provideMerge(base)) + +afterEach(async () => { + defaultModelRef.current = undefined + await Instance.disposeAll() +}) + +type RT = ManagedRuntime.ManagedRuntime + +async function withInbox(directory: string, fn: (rt: RT) => Promise) { + return Instance.provide({ + directory, + fn: async () => { + const rt = ManagedRuntime.make(testLayer) + try { + await fn(rt) + } finally { + await rt.dispose() + } + }, + }) +} + +async function seedRealMessage(rt: RT, sessionID: SessionID, actorID: string) { + return rt.runPromise( + Session.Service.use((sessions) => + sessions.updateMessage({ + id: MessageID.ascending(), + role: "user" as const, + sessionID, + agentID: actorID, + time: { created: Date.now() }, + agent: "general", + model: { providerID: ProviderID.make("test"), modelID: ModelID.make("test-model") }, + }), + ), + ) +} + +function row(type: string, text: string | undefined): InboxRow { + return { + id: "01AAA", + receiver_session_id: "ses_x", + receiver_actor_id: "main", + sender_session_id: "ses_y", + sender_actor_id: "general-1", + type, + content: text === undefined ? {} : { text }, + created_at: 0, + } as unknown as InboxRow +} + +describe("inbox render never yields an empty part text", () => { + test("a body-less actor_notification renders the placeholder, not an empty string", () => { + expect(renderInboxRow(row("actor_notification", ""))).toBe("(no notification body)") + expect(renderInboxRow(row("actor_notification", undefined))).toBe("(no notification body)") + }) + + test("a body-less text row renders a non-empty wrapper", () => { + expect(renderInboxRow(row("text", ""))).toContain("(empty)") + expect(renderInboxRow(row("text", undefined))).toContain("(empty)") + }) + + test("every row type/body combination renders non-empty", () => { + for (const type of ["actor_notification", "text", "unknown-future-type"]) { + for (const text of ["", undefined, " ", "real body"]) { + expect(renderInboxRow(row(type, text)).length).toBeGreaterThan(0) + } + } + }) +}) + +describe("Inbox.drain never persists an empty user text part", () => { + test("draining a body-less actor_notification writes a non-empty synthetic part", async () => { + await using tmp = await tmpdir({ git: true }) + await withInbox(tmp.path, async (rt) => { + const session = await rt.runPromise(Session.Service.use((s) => s.create())) + await rt.runPromise( + ActorRegistry.Service.use((reg) => + reg.register({ + sessionID: session.id, + actorID: "actor-empty", + mode: "subagent", + parentActorID: undefined, + agent: "general", + description: "empty-body notification", + contextMode: "none", + contextWatermark: undefined, + background: false, + lifecycle: "ephemeral", + }), + ), + ) + await seedRealMessage(rt, session.id, "actor-empty") + + // Constructed, not reachable through the actor tool: the shell route IS + // re-validated against `content: z.string().min(1)` (see + // empty-notification-reachability.test.ts). This calls Inbox.send directly + // to pin what the layers BELOW the entry point do with a blank body, so the + // render/drain invariants are proven independently of any caller's guard. + await rt.runPromise( + Inbox.Service.use((inbox) => + inbox.send({ + receiverSessionID: session.id, + receiverActorID: "actor-empty", + content: "", + type: "actor_notification", + }), + ), + ) + + expect(await rt.runPromise(Inbox.Service.use((inbox) => inbox.drain(session.id, "actor-empty")))).toBe(1) + + const msgs = await rt.runPromise( + Session.Service.use((sessions) => sessions.messages({ sessionID: session.id, agentID: "actor-empty" })), + ) + const drained = msgs.findLast((m) => m.info.role === "user" && m.parts.some((p) => p.type === "text" && p.synthetic)) + expect(drained).toBeDefined() + const textParts = drained!.parts.filter((p) => p.type === "text") + expect(textParts.length).toBe(1) + // The whole point: this part must not be "" — a length-1 parts array whose + // only text is empty is the shape that reaches a provider as `content: []`. + expect(textParts[0].type === "text" && textParts[0].text).toBe("(no notification body)") + expect(drained!.parts.every((p) => p.type !== "text" || p.text !== "")).toBe(true) + }) + }) + + test("a mixed drain (empty + real bodies) leaves no empty text part behind", async () => { + await using tmp = await tmpdir({ git: true }) + await withInbox(tmp.path, async (rt) => { + const session = await rt.runPromise(Session.Service.use((s) => s.create())) + await rt.runPromise( + ActorRegistry.Service.use((reg) => + reg.register({ + sessionID: session.id, + actorID: "actor-mixed", + mode: "subagent", + parentActorID: undefined, + agent: "general", + description: "mixed bodies", + contextMode: "none", + contextWatermark: undefined, + background: false, + lifecycle: "ephemeral", + }), + ), + ) + await seedRealMessage(rt, session.id, "actor-mixed") + + for (const body of ["", "a real notification", ""]) { + await rt.runPromise( + Inbox.Service.use((inbox) => + inbox.send({ + receiverSessionID: session.id, + receiverActorID: "actor-mixed", + content: body, + type: "actor_notification", + }), + ), + ) + } + + expect(await rt.runPromise(Inbox.Service.use((inbox) => inbox.drain(session.id, "actor-mixed")))).toBe(3) + + const msgs = await rt.runPromise( + Session.Service.use((sessions) => sessions.messages({ sessionID: session.id, agentID: "actor-mixed" })), + ) + const drained = msgs.findLast((m) => m.info.role === "user" && m.parts.some((p) => p.type === "text" && p.synthetic)) + expect(drained!.parts.filter((p) => p.type === "text").length).toBe(3) + expect(drained!.parts.every((p) => p.type !== "text" || p.text !== "")).toBe(true) + }) + }) +}) diff --git a/packages/opencode/test/inbox/empty-notification-reachability.test.ts b/packages/opencode/test/inbox/empty-notification-reachability.test.ts new file mode 100644 index 000000000..32c571a0a --- /dev/null +++ b/packages/opencode/test/inbox/empty-notification-reachability.test.ts @@ -0,0 +1,207 @@ +import { afterEach, describe, expect } from "bun:test" +import { Effect, Layer } from "effect" +import { Agent } from "../../src/agent/agent" +import { Bus } from "../../src/bus" +import { Config } from "../../src/config" +import { Provider } from "../../src/provider" +import * as CrossSpawnSpawner from "../../src/effect/cross-spawn-spawner" +import { Instance } from "../../src/project/instance" +import { Session } from "../../src/session" +import { SessionCheckpoint } from "../../src/session/checkpoint" +import { Database, and, eq } from "../../src/storage" +import { MessageID, type SessionID } from "../../src/session/schema" +import { ActorTool, parseActorScript } from "../../src/tool/actor" +import { shellWrap } from "../../src/tool/shell-wrap" +import { ActorRegistry } from "../../src/actor/registry" +import { TaskRegistry } from "../../src/task/registry" +import { ActorWaiter } from "../../src/actor/waiter" +import { Inbox } from "../../src/inbox" +import { InboxTable } from "../../src/inbox/inbox.sql" +import { Team } from "../../src/team" +import { Truncate } from "../../src/tool" +import { ToolRegistry } from "../../src/tool" +import { provideTmpdirInstance } from "../fixture/fixture" +import { testEffect } from "../lib/effect" + +// Reachability probe for the body-less `actor_notification` inbox row — the +// suspected producer of `messages.: user messages must have non-empty content`. +// +// test/inbox/empty-notification-part.test.ts proves what happens ONCE such a row +// exists (renderInboxRow → drain → a `text: ""` part). It calls Inbox.send +// directly, so it does NOT establish that any caller can supply an empty body. +// This file closes that gap at the only entry point that takes both `content` +// and `type` from the model: the `actor` tool's `send` action. +// +// The decisive case is `via the real shellWrap route` below. Driving +// `def.execute` directly is NOT a substitute: it presupposes the very question +// (does the shell route reach the wrap-decorated execute?). The real composition +// is `shellWrap(Tool.init(actor))` — registry.ts wires `actor: Tool.init(actor)` +// (the wrap-decorated def, see tool.ts `define` → `wrap`) into `s.builtin`, +// `all()`/`available()` only filter that array, and registry.ts then applies +// `shellWrap` to that same object. So `shell-wrap.ts`'s `def.execute(parsed)` is +// the wrap-decorated execute, and `wrap()` runs `toolInfo.parameters.parse(args)` +// on the shell-parsed op. A shell-mode op IS re-validated. +// +// Consequence, established by a revert probe on this file (drop the +// parseActorScript guard in src/tool/actor.ts and re-run): the shell route still +// enqueues nothing, because `content: z.string().min(1)` fails closed. The +// parse-level guard is therefore a message-quality improvement (a specific, +// teachable error instead of a generic zod dump), NOT the layer that makes a +// blank body unreachable. An empty `actor_notification` body was never reachable +// through the tool, so the render.ts/drain fixes in this PR close a LATENT +// defence gap rather than a live producer. + +afterEach(async () => { + await Instance.disposeAll() +}) + +const inboxDeps = Layer.mergeAll(Bus.layer, ActorRegistry.defaultLayer, Session.defaultLayer) + +const it = testEffect( + Layer.mergeAll( + Agent.defaultLayer, + Bus.layer, + Config.defaultLayer, + Provider.defaultLayer, + CrossSpawnSpawner.defaultLayer, + Session.defaultLayer, + Truncate.defaultLayer, + ToolRegistry.defaultLayer, + ActorRegistry.defaultLayer, + ActorWaiter.layer.pipe( + Layer.provide(Bus.layer), + Layer.provide(ActorRegistry.defaultLayer), + Layer.provide(Session.defaultLayer), + ), + Team.defaultLayer, + SessionCheckpoint.defaultLayer, + TaskRegistry.defaultLayer, + Inbox.layer.pipe(Layer.provide(inboxDeps)), + ), +) + +function ctxFor(sessionID: SessionID) { + return { + sessionID, + messageID: MessageID.ascending(), + agent: "build", + abort: new AbortController().signal, + extra: {}, + messages: [], + metadata: () => Effect.void, + ask: () => Effect.void, + } +} + +const registerActor = Effect.fn(function* (sessionID: SessionID) { + const registry = yield* ActorRegistry.Service + const actorID = yield* registry.allocateActorID(sessionID, "general") + yield* registry.register({ + sessionID, + actorID, + mode: "subagent", + agent: "general", + description: "reachability probe", + contextMode: "none", + background: true, + lifecycle: "ephemeral", + }) + yield* registry.updateStatus(sessionID, actorID, { status: "running" }) + return actorID +}) + +const rowsFor = (sessionID: SessionID, actorID: string) => + Effect.sync(() => + Database.use((db) => + db + .select() + .from(InboxTable) + .where(and(eq(InboxTable.receiver_session_id, sessionID), eq(InboxTable.receiver_actor_id, actorID))) + .all(), + ), + ) + +describe("empty actor_notification body: reachability", () => { + it.live( + 'the shell-parsed `actor send ""` is rejected by parseActorScript', + provideTmpdirInstance(() => + Effect.gen(function* () { + const exit = yield* Effect.exit(parseActorScript('actor send main "" --type actor_notification')) + expect(exit._tag).toBe("Failure") + }), + ), + ) + + // DECISIVE CASE. Exercises the production composition end-to-end: + // shellWrap(wrap-decorated actor def).execute({ script }). No inbox row may be + // written for a blank body. Revert the parseActorScript guard and this still + // passes — which is what proves zod, not the parse guard, is load-bearing. + it.live( + 'via the real shellWrap route, `actor send ""` enqueues nothing', + provideTmpdirInstance(() => + Effect.gen(function* () { + const sessions = yield* Session.Service + const chat = yield* sessions.create({ title: "chat" }) + const actorID = yield* registerActor(chat.id) + + const def = yield* Effect.flatMap(ActorTool, (tool) => tool.init()) + const shell = shellWrap({ ...def, id: "actor" }) + + const exit = yield* Effect.exit( + shell.execute({ script: `actor send ${actorID} ""` }, ctxFor(chat.id) as never), + ) + // shell-wrap converts a per-command failure into a *successful* result + // carrying an error report, so assert on the observable side effect + // rather than the exit tag: nothing may be enqueued. + expect(yield* rowsFor(chat.id, actorID)).toHaveLength(0) + if (exit._tag === "Success") { + expect(exit.value.output).not.toContain("inboxID") + } + }), + ), + ) + + it.live( + "the operation-level zod min(1) rejects an empty body inside def.execute", + provideTmpdirInstance(() => + Effect.gen(function* () { + const sessions = yield* Session.Service + const chat = yield* sessions.create({ title: "chat" }) + const actorID = yield* registerActor(chat.id) + + const def = yield* Effect.flatMap(ActorTool, (tool) => tool.init()) + + // Hand def.execute the exact op a shell-parsed call would produce. + // wrap()'s parameters.parse must reject it. + const exit = yield* Effect.exit( + def.execute( + { operation: { action: "send", to_actor_id: actorID, content: "", type: "actor_notification" } }, + ctxFor(chat.id), + ), + ) + expect(exit._tag).toBe("Failure") + expect(yield* rowsFor(chat.id, actorID)).toHaveLength(0) + }), + ), + ) + + it.live( + "a non-empty body still goes through the shell route, so the guards are not over-broad", + provideTmpdirInstance(() => + Effect.gen(function* () { + const sessions = yield* Session.Service + const chat = yield* sessions.create({ title: "chat" }) + const actorID = yield* registerActor(chat.id) + + const def = yield* Effect.flatMap(ActorTool, (tool) => tool.init()) + const shell = shellWrap({ ...def, id: "actor" }) + const result = yield* shell.execute( + { script: `actor send ${actorID} "real body"` }, + ctxFor(chat.id) as never, + ) + expect(result.output).toContain("inboxID") + expect(yield* rowsFor(chat.id, actorID)).toHaveLength(1) + }), + ), + ) +}) diff --git a/packages/opencode/test/provider/empty-content-wire.test.ts b/packages/opencode/test/provider/empty-content-wire.test.ts new file mode 100644 index 000000000..80ba206d1 --- /dev/null +++ b/packages/opencode/test/provider/empty-content-wire.test.ts @@ -0,0 +1,114 @@ +import { describe, expect, test } from "bun:test" +import { createAnthropic } from "@ai-sdk/anthropic" +import { ProviderTransform } from "../../src/provider" + +// WIRE-LEVEL proof of the `{"role":"user","content":[]}` producer. +// +// Asserting on `MessageV2.toModelMessages` output is two transformation layers +// short of the wire. What the provider actually receives is built by +// @ai-sdk/anthropic's `convertToAnthropicMessagesPrompt` from a +// `LanguageModelV3Prompt`. So these tests capture the real outbound HTTP body by +// injecting `fetch` into the provider factory. +// +// The bug: `ensureTrailingUserMessage` appended `{ role: "user", content: +// "Continue." }` — a BARE STRING. `ProviderTransform.message` is typed for +// `ModelMessage[]` (string content legal) but runs on a `LanguageModelV3Prompt` +// (user content must be an array of parts); the mismatch is silenced by the +// `@ts-expect-error` at session/llm.ts:670. @ai-sdk/anthropic then iterates the +// string CHARACTER BY CHARACTER (`for (let j = 0; j < content.length; j++)` with +// a `switch (part.type)` that has cases for only text/file and no default), so +// nothing is pushed and the message ships as `content: []`. + +const model = { + id: "anthropic/claude-3-5-sonnet", + providerID: "anthropic", + api: { id: "claude-3-5-sonnet-20241022", url: "https://api.anthropic.com", npm: "@ai-sdk/anthropic" }, + name: "Claude 3.5 Sonnet", + capabilities: { + temperature: true, + reasoning: false, + attachment: true, + toolcall: true, + input: { text: true, audio: false, image: true, video: false, pdf: true }, + output: { text: true, audio: false, image: false, video: false, pdf: false }, + interleaved: false, + }, + cost: { input: 0.003, output: 0.015, cache: { read: 0.0003, write: 0.00375 } }, + limit: { context: 200000, output: 8192 }, + status: "active", + options: {}, + headers: {}, +} as any + +const reply = { + id: "msg_1", + type: "message", + role: "assistant", + model: "claude-3-5-sonnet-20241022", + content: [{ type: "text", text: "ok" }], + stop_reason: "end_turn", + usage: { input_tokens: 1, output_tokens: 1 }, +} + +// Sends `prompt` through the real provider and returns the parsed HTTP body. +async function outbound(prompt: unknown) { + let captured: any + const anthropic = createAnthropic({ + apiKey: "test-key", + fetch: (async (_url: any, init: any) => { + captured = JSON.parse(init.body as string) + return new Response(JSON.stringify(reply), { headers: { "content-type": "application/json" } }) + }) as any, + }) + await anthropic("claude-3-5-sonnet-20241022").doGenerate({ prompt } as any) + return captured +} + +// A conversation that ends with a content-bearing assistant — the one condition +// under which `ensureTrailingUserMessage` appends a continuation turn. +const endsWithAssistant = [ + { role: "user", content: [{ type: "text", text: "hi" }] }, + { role: "assistant", content: [{ type: "text", text: "done" }] }, +] + +describe("the trailing continuation turn reaches the wire with non-empty content", () => { + test("CONTROL: a bare-string user content is shipped as content: [] (the producer)", async () => { + const body = await outbound([...endsWithAssistant, { role: "user", content: "Continue." }]) + const last = body.messages[body.messages.length - 1] + expect(last.role).toBe("user") + // Proof of the defect mechanism, and proof this test would catch a regression. + expect(last.content).toEqual([]) + }) + + test("CONTROL: the part-array form is shipped intact", async () => { + const body = await outbound([ + ...endsWithAssistant, + { role: "user", content: [{ type: "text", text: "Continue." }] }, + ]) + const last = body.messages[body.messages.length - 1] + expect(last.content).toEqual([{ type: "text", text: "Continue." }]) + }) + + test("ensureTrailingUserMessage's appended turn survives to the wire", async () => { + // @ts-expect-error mirrors session/llm.ts:670 — message() is typed for + // ModelMessage[] but is applied to a LanguageModelV3Prompt in production. + const transformed = ProviderTransform.message(endsWithAssistant, model, {}) + const body = await outbound(transformed) + const last = body.messages[body.messages.length - 1] + expect(last.role).toBe("user") + // `applyCaching` may also attach a cache_control marker to the last part; + // what matters is that a real text part is present at all. + expect(last.content).toHaveLength(1) + expect(last.content[0].type).toBe("text") + expect(last.content[0].text).toBe("Continue.") + }) + + test("NO message reaches the wire with empty content", async () => { + // @ts-expect-error see above + const body = await outbound(ProviderTransform.message(endsWithAssistant, model, {})) + const empty = body.messages + .map((msg: any, index: number) => ({ index, role: msg.role, length: msg.content.length })) + .filter((entry: any) => entry.length === 0) + expect(empty).toEqual([]) + }) +}) diff --git a/packages/opencode/test/provider/transform.test.ts b/packages/opencode/test/provider/transform.test.ts index 67df9eee8..dbed55468 100644 --- a/packages/opencode/test/provider/transform.test.ts +++ b/packages/opencode/test/provider/transform.test.ts @@ -1859,9 +1859,15 @@ describe("ProviderTransform.message - anthropic empty content filtering", () => const result = ProviderTransform.message(msgs, openaiModel, {}) - expect(result).toHaveLength(3) - expect(result[0].content).toBe("") - expect(result[1].content).toHaveLength(1) + // The anthropic-only empty-PART stripping still does not run for other + // providers (that is what this test guards), but the provider-agnostic + // non-empty-content invariant DOES: npm-gating it is exactly what let an + // empty-content message reach a Bedrock-backed gateway. Both empty assistant + // messages here are residue with nothing to preserve, so they are dropped and + // the request correctly ends with the real user turn. + expect(result).toHaveLength(1) + expect(result[0].role).toBe("user") + expect(result[0].content).toBe("next") }) test("splits anthropic assistant messages when text trails tool calls", () => { @@ -4531,28 +4537,46 @@ describe("ProviderTransform.message - non-array content guard (j.map is not a fu expect(result[0].content).toBe("next") }) - test("undefined content is normalized to empty array (crash guard)", () => { + test("undefined content is normalized to a NON-EMPTY array (crash guard + content invariant)", () => { const msgs = [{ role: "user", content: undefined }] as any[] const result = ProviderTransform.message(msgs, genericModel, {}) expect(result).toHaveLength(1) + // The crash guard still holds: content is always an array so `.map()` is safe. expect(Array.isArray(result[0].content)).toBe(true) - expect(result[0].content).toEqual([]) + // ...but it must NOT be blanked to `[]`. A user message with empty content is + // rejected by Bedrock/Anthropic ("user messages must have non-empty content"), + // and dropping it instead would end the request on an assistant (prefill 400). + // Invalid user content is therefore BACKFILLED with a minimal text turn. + expect((result[0].content as any[]).length).toBeGreaterThan(0) + expect((result[0].content as any[])[0]).toMatchObject({ type: "text", text: "Continue." }) }) - test("null content is normalized to empty array (crash guard)", () => { + test("null content is normalized to a NON-EMPTY array (crash guard + content invariant)", () => { const msgs = [{ role: "user", content: null }] as any[] const result = ProviderTransform.message(msgs, genericModel, {}) expect(result).toHaveLength(1) + // The crash guard still holds: content is always an array so `.map()` is safe. expect(Array.isArray(result[0].content)).toBe(true) - expect(result[0].content).toEqual([]) + // ...but it must NOT be blanked to `[]`. A user message with empty content is + // rejected by Bedrock/Anthropic ("user messages must have non-empty content"), + // and dropping it instead would end the request on an assistant (prefill 400). + // Invalid user content is therefore BACKFILLED with a minimal text turn. + expect((result[0].content as any[]).length).toBeGreaterThan(0) + expect((result[0].content as any[])[0]).toMatchObject({ type: "text", text: "Continue." }) }) - test("object content is normalized to empty array (crash guard)", () => { + test("object content is normalized to a NON-EMPTY array (crash guard + content invariant)", () => { const msgs = [{ role: "user", content: { type: "text", text: "oops" } }] as any[] const result = ProviderTransform.message(msgs, genericModel, {}) expect(result).toHaveLength(1) + // The crash guard still holds: content is always an array so `.map()` is safe. expect(Array.isArray(result[0].content)).toBe(true) - expect(result[0].content).toEqual([]) + // ...but it must NOT be blanked to `[]`. A user message with empty content is + // rejected by Bedrock/Anthropic ("user messages must have non-empty content"), + // and dropping it instead would end the request on an assistant (prefill 400). + // Invalid user content is therefore BACKFILLED with a minimal text turn. + expect((result[0].content as any[]).length).toBeGreaterThan(0) + expect((result[0].content as any[])[0]).toMatchObject({ type: "text", text: "Continue." }) }) test("already-array content passes through unchanged", () => { @@ -4583,6 +4607,49 @@ describe("ProviderTransform.message - non-array content guard (j.map is not a fu ] as any[] expect(() => ProviderTransform.message(msgs, genericModel, {})).not.toThrow() }) + + // Policy pin, not a reachability claim: tool messages are always built with + // array content, so this input does not occur in normal use. It is pinned + // because normalizeContentArray must agree with ensureNonEmptyContent, which + // deliberately leaves tool messages untouched — injecting a text part into a + // tool message breaks tool_use/tool_result pairing (trading one 400 for + // another), and `content: []` is itself illegal for a tool result. + test("a tool message with non-array content is never text-backfilled", () => { + const msgs = [ + { role: "user", content: [{ type: "text", text: "run it" }] }, + { role: "assistant", content: [{ type: "tool-call", toolCallId: "c1", toolName: "bash", input: {} }] }, + { role: "tool", content: undefined }, + ] as any[] + const tool = ProviderTransform.message(msgs, genericModel, {}).find((m) => m.role === "tool") + expect(tool).toBeDefined() + expect(Array.isArray(tool!.content) && tool!.content.some((p: any) => p.type === "text")).toBe(false) + }) + + // Strengthens the pin above, which only rules out a TEXT part and would still + // pass if tool content were rewritten to `[]` — the other outcome the policy + // rejects (an empty tool content is itself illegal for providers that require + // the result block). Assert the value is the SAME reference, i.e. untouched. + test("POLICY PIN: non-array tool content is left byte-identical, not rewritten to [] (all invalid shapes)", () => { + for (const content of [undefined, null, { type: "tool-result", value: "x" }]) { + const msgs = [ + { role: "user", content: [{ type: "text", text: "run it" }] }, + { role: "assistant", content: [{ type: "tool-call", toolCallId: "c1", toolName: "bash", input: {} }] }, + { role: "tool", content }, + ] as any[] + const tool = ProviderTransform.message(msgs, genericModel, {}).find((m) => m.role === "tool") + expect(tool).toBeDefined() + expect(tool!.content).toBe(content as any) + } + }) + + // The two guards in this file that decide what to do with a provider-rejectable + // message must not disagree about the tool role — that disagreement was the + // finding. Pin the agreement itself, so changing only one of them fails here. + test("POLICY PIN: normalizeContentArray and ensureNonEmptyContent agree on a non-array tool message", () => { + const msgs = [{ role: "tool", content: undefined }] as any[] + expect(ProviderTransform.ensureNonEmptyContent(msgs)[0].content).toBeUndefined() + expect(ProviderTransform.message(msgs, genericModel, {}).find((m) => m.role === "tool")?.content).toBeUndefined() + }) }) describe("ProviderTransform.message - interleaved field: openrouter exclusion", () => { @@ -4682,3 +4749,227 @@ describe("ProviderTransform.message - interleaved field: empty reasoning still s expect(result[0].providerOptions?.openaiCompatible?.reasoning_content).toBe("") }) }) + +// Regression suite for the live Bedrock 400 +// `messages.: user messages must have non-empty content`. +// +// Root mechanism (verified verbatim against ai@6.0.168, convertToLanguageModelMessage): +// the SDK's USER branch strips empty text parts with no backfill — +// .filter((part) => part.type !== "text" || part.text !== "") +// — and it runs AFTER every ProviderTransform step. So a user message whose only +// text part is "" leaves our transform looking like a healthy length-1 array and +// arrives at the provider as `content: []`. +// +// Every test asserts BOTH invariants together, because fixing either one alone +// re-opens the other's 400: +// (1) no message reaches the provider with empty content, and +// (2) the request still ends with a user/tool message (no assistant prefill). +describe("ProviderTransform.message - non-empty content invariant (paired with the prefill invariant)", () => { + const modelFor = (npm: string, providerID = "anthropic", apiID = "claude-opus-5") => + ({ + id: `${providerID}/${apiID}`, + providerID, + api: { id: apiID, url: "https://example.invalid", npm }, + name: apiID, + capabilities: { + temperature: true, + reasoning: false, + attachment: true, + toolcall: true, + input: { text: true, audio: false, image: true, video: false, pdf: true }, + output: { text: true, audio: false, image: false, video: false, pdf: false }, + interleaved: false, + }, + cost: { input: 0.003, output: 0.015, cache: { read: 0.0003, write: 0.00375 } }, + limit: { context: 200000, output: 8192 }, + status: "active", + options: {}, + headers: {}, + }) as any + + // Emptiness as the PROVIDER sees it: replicate the AI SDK's user-content filter + // so these assertions catch the real failure shape, not just `content.length`. + const sdkVisible = (msg: any) => { + if (typeof msg.content === "string") return msg.content === "" ? [] : [{ type: "text", text: msg.content }] + if (!Array.isArray(msg.content)) return [] + if (msg.role !== "user") return msg.content + return msg.content.filter((p: any) => !p || p.type !== "text" || p.text !== "") + } + + const expectBothInvariants = (result: any[]) => { + const empty = result + .map((m, i) => ({ i, role: m.role, visible: sdkVisible(m).length })) + .filter((r) => r.visible === 0) + expect(empty).toEqual([]) + // Prefill invariant: must not end with an assistant message. + expect(result.length).toBeGreaterThan(0) + expect(result[result.length - 1].role).not.toBe("assistant") + } + + // The exact shape captured off the wire in the live incident: a content-bearing + // assistant followed by a user turn that the SDK would empty to `content: []`. + const liveIncidentShape = () => [ + { role: "user", content: [{ type: "text", text: "how many open PRs?" }] }, + { + role: "assistant", + content: [{ type: "text", text: "## 8 个 OPEN PR ..." }], + }, + { role: "user", content: [{ type: "text", text: "" }] }, + ] as any[] + + for (const npm of ["@ai-sdk/anthropic", "@ai-sdk/amazon-bedrock", "@ai-sdk/openai-compatible", "@ai-sdk/openai"]) { + test(`history ending in a content-bearing assistant + SDK-emptied user turn is repaired (${npm})`, () => { + const result = ProviderTransform.message(liveIncidentShape(), modelFor(npm), {}) + expectBothInvariants(result) + // The user turn is BACKFILLED, never dropped — dropping it would end the + // request on the assistant and trade this 400 for the prefill 400. + const last = result[result.length - 1] + expect(last.role).toBe("user") + // The assistant's completed reply is still present. + expect(JSON.stringify(result)).toContain("## 8 个 OPEN PR") + }) + } + + test("history ending in a content-bearing assistant (no trailing user) keeps the reply and appends a user turn", () => { + const msgs = [ + { role: "user", content: [{ type: "text", text: "hi" }] }, + { role: "assistant", content: [{ type: "text", text: "## 8 个 OPEN PR ..." }] }, + ] as any[] + + const result = ProviderTransform.message(msgs, modelFor("@ai-sdk/anthropic"), {}) + expectBothInvariants(result) + expect(JSON.stringify(result)).toContain("## 8 个 OPEN PR") + }) + + test("a message whose parts are all non-convertible/ignored (empty array content) is repaired, not dropped into a prefill", () => { + const msgs = [ + { role: "user", content: [{ type: "text", text: "start" }] }, + { role: "assistant", content: [{ type: "text", text: "done" }] }, + // Every part was ignored/non-convertible upstream — arrives already empty. + { role: "user", content: [] }, + ] as any[] + + const result = ProviderTransform.message(msgs, modelFor("@ai-sdk/openai-compatible"), {}) + expectBothInvariants(result) + expect(result[result.length - 1].role).toBe("user") + }) + + test("an empty text part carrying a cache_control marker is still repaired (SDK strips it despite providerOptions)", () => { + const msgs = [ + { role: "assistant", content: [{ type: "text", text: "reply" }] }, + { + role: "user", + content: [{ type: "text", text: "", providerOptions: { anthropic: { cacheControl: { type: "ephemeral" } } } }], + }, + ] as any[] + + const result = ProviderTransform.message(msgs, modelFor("@ai-sdk/anthropic"), {}) + expectBothInvariants(result) + }) + + for (const bad of [undefined, null, { some: "object" }] as any[]) { + test(`ModelMessage arriving with content=${JSON.stringify(bad) ?? "undefined"} is backfilled for user, not blanked`, () => { + const msgs = [ + { role: "user", content: [{ type: "text", text: "start" }] }, + { role: "assistant", content: [{ type: "text", text: "reply" }] }, + { role: "user", content: bad }, + ] as any[] + + const result = ProviderTransform.message(msgs, modelFor("@ai-sdk/anthropic"), {}) + expectBothInvariants(result) + expect(result[result.length - 1].role).toBe("user") + }) + } + + test("empty-string user content is backfilled rather than removed", () => { + const msgs = [ + { role: "assistant", content: [{ type: "text", text: "reply" }] }, + { role: "user", content: "" }, + ] as any[] + + const result = ProviderTransform.message(msgs, modelFor("@ai-sdk/openai"), {}) + expectBothInvariants(result) + }) + + test("empty assistant residue is dropped and the prefill invariant still holds", () => { + const msgs = [ + { role: "user", content: [{ type: "text", text: "hi" }] }, + { role: "assistant", content: [] }, + ] as any[] + + const result = ProviderTransform.message(msgs, modelFor("@ai-sdk/anthropic"), {}) + expectBothInvariants(result) + expect(result).toHaveLength(1) + expect(result[0].role).toBe("user") + }) + + test("a trailing tool message is left alone and satisfies both invariants", () => { + const msgs = [ + { role: "user", content: [{ type: "text", text: "hi" }] }, + { + role: "assistant", + content: [{ type: "tool-call", toolCallId: "call_1", toolName: "read", input: {} }], + }, + { + role: "tool", + content: [{ type: "tool-result", toolCallId: "call_1", toolName: "read", output: { type: "text", value: "ok" } }], + }, + ] as any[] + + const result = ProviderTransform.message(msgs, modelFor("@ai-sdk/anthropic"), {}) + expectBothInvariants(result) + expect(result[result.length - 1].role).toBe("tool") + }) +}) + +describe("ProviderTransform.message - end-to-end through the AI SDK's own wire conversion", () => { + // The strongest form of the regression: run our transform output through the + // real ai@6 prompt conversion and assert the WIRE payload has no empty content. + // This is the layer that produced the incident and that unit-level assertions on + // `content.length` cannot see. + test("no wire message has empty content, and the wire still ends with a user turn", async () => { + const { convertToLanguageModelPrompt } = await import("ai/internal") + const model = { + id: "anthropic/claude-opus-5", + providerID: "anthropic", + // Deliberately NOT @ai-sdk/anthropic: the anthropic-only empty-part filter in + // normalizeMessages would mask the defect. The live incident hit a + // Bedrock-backed gateway on a non-anthropic npm, which had no protection. + api: { id: "claude-opus-5", url: "https://example.invalid", npm: "@ai-sdk/openai-compatible" }, + name: "claude-opus-5", + capabilities: { + temperature: true, + reasoning: false, + attachment: true, + toolcall: true, + input: { text: true, audio: false, image: true, video: false, pdf: true }, + output: { text: true, audio: false, image: false, video: false, pdf: false }, + interleaved: false, + }, + cost: { input: 0.003, output: 0.015, cache: { read: 0.0003, write: 0.00375 } }, + limit: { context: 200000, output: 8192 }, + status: "active", + options: {}, + headers: {}, + } as any + + const msgs = [ + { role: "user", content: [{ type: "text", text: "how many open PRs?" }] }, + { role: "assistant", content: [{ type: "text", text: "## 8 个 OPEN PR ..." }] }, + { role: "user", content: [{ type: "text", text: "" }] }, + ] as any[] + + const out = ProviderTransform.message(msgs, model, {}) + const wire = (await convertToLanguageModelPrompt({ + prompt: { messages: out, system: undefined }, + supportedUrls: {}, + download: undefined, + })) as any[] + + const empty = wire + .map((m, i) => ({ i, role: m.role, len: Array.isArray(m.content) ? m.content.length : -1 })) + .filter((r) => r.len === 0) + expect(empty).toEqual([]) + expect(wire[wire.length - 1].role).not.toBe("assistant") + }) +}) diff --git a/packages/opencode/test/session/message-v2.test.ts b/packages/opencode/test/session/message-v2.test.ts index 525e2ad74..cfa5648cc 100644 --- a/packages/opencode/test/session/message-v2.test.ts +++ b/packages/opencode/test/session/message-v2.test.ts @@ -1,5 +1,6 @@ import { describe, expect, test } from "bun:test" import { APICallError } from "ai" +import { convertToLanguageModelPrompt } from "ai/internal" import { MessageV2 } from "../../src/session/message-v2" import { ProviderTransform } from "../../src/provider" import type { Provider } from "../../src/provider" @@ -211,6 +212,46 @@ describe("session.message-v2.toModelMessage", () => { ]) }) + // Mechanism pin for the empty-user-content provider 400. Companion to the + // zero-part test above: a zero-part user message is DROPPED by our layer (so + // the transient state between Inbox.drain's `updateMessage` and its first + // `updatePart` can never reach a provider), but a message whose only part is + // `text: ""` survives at parts.length === 1 — invisible to every + // `parts.length === 0` / `content.length === 0` check — and is only reduced to + // `content: []` later, inside the SDK's own per-role filter on the way to the + // provider (ai@6.0.168 dist/index.mjs:1424, convertToLanguageModelMessage: + // `.filter((part) => part.type !== "text" || part.text !== "")`, no backfill). + // `content: []` is what a provider rejects with + // "messages.: user messages must have non-empty content". + test("an empty-text-only user message survives our layer at length 1 and only collapses at the SDK boundary", async () => { + const input: MessageV2.WithParts[] = [ + { + info: userInfo("m-empty-text"), + parts: [ + { + ...basePart("m-empty-text", "p1"), + type: "text", + text: "", + }, + ] as MessageV2.Part[], + }, + ] + + // Our layer: still length 1, so nothing on our side can see it as "empty". + const ours = await MessageV2.toModelMessages(input, model) + expect(ours).toStrictEqual([{ role: "user", content: [{ type: "text", text: "" }] }]) + + // The SDK step that actually runs between us and the provider. + const wire = await convertToLanguageModelPrompt({ + prompt: { messages: ours }, + supportedUrls: {}, + download: async () => [], + }) + expect(wire.length).toBe(1) + expect(wire[0].role).toBe("user") + expect(wire[0].content).toStrictEqual([]) + }) + test("filters out messages with only ignored parts", async () => { const messageID = "m-user" diff --git a/packages/opencode/test/tool/actor.shell.test.ts b/packages/opencode/test/tool/actor.shell.test.ts index d6f8a441c..ded1a9a60 100644 --- a/packages/opencode/test/tool/actor.shell.test.ts +++ b/packages/opencode/test/tool/actor.shell.test.ts @@ -198,6 +198,36 @@ describe("actor.shell.parse: send", () => { expect(err.kind).toBe("arity") expect(err.detail).toContain("to_actor_id") }) + + // A blank body is already unreachable via the tool: `parameters` DOES re-validate + // a shell-parsed op (shell-wrap.ts calls `def.execute(parsed)` on the + // wrap()-decorated def, and wrap() runs `parameters.parse` inside execute), so + // `content: z.string().min(1)` rejects it — see + // test/inbox/empty-notification-reachability.test.ts for the end-to-end proof. + // These cases pin the parse-level guard, which exists to turn a generic zod dump + // into one specific, teachable message and to also reject whitespace-only bodies + // (which `min(1)` accepts). + for (const script of [ + 'actor send main ""', + 'actor send main "" --type actor_notification', + 'actor send main " " --type actor_notification', + ]) { + test(`send rejects a blank content: ${script}`, async () => { + const exit = await Effect.runPromise(Effect.exit(parseActorScript(script))) + expect(exit._tag).toBe("Failure") + const cause: any = (exit as any).cause + const fail = cause.reasons?.find?.((r: any) => r._tag === "Fail") ?? cause + const err = fail.error ?? fail + expect(err.detail).toContain("content must not be empty") + }) + } + + test("send still accepts a short non-blank content (guard is not over-broad)", async () => { + const out = await parse('actor send main "0" --type actor_notification') + expect(out).toEqual([ + { operation: { action: "send", to_actor_id: "main", content: "0", type: "actor_notification" } }, + ]) + }) }) describe("actor.shell.parse: full parity flags", () => {