diff --git a/packages/opencode/src/inbox/render.ts b/packages/opencode/src/inbox/render.ts index aa4c2df2c..e283e9c09 100644 --- a/packages/opencode/src/inbox/render.ts +++ b/packages/opencode/src/inbox/render.ts @@ -5,7 +5,14 @@ export function renderInboxRow(row: InboxRow): string { // Pre-rendered notification text — sender produced the full // ... wrapper. const content = row.content as { text?: string } - return content.text ?? "(no notification body)" + // `||` not `??`: an EMPTY body is exactly as unusable as a missing one, and + // `??` let `""` through. Inbox.drain persists this return value verbatim as + // the ONLY text part of a synthetic `role:"user"` message, so a `""` here + // produced `parts: [{type:"text",text:""}]` — length 1, so every + // `parts.length === 0` guard misses it — which `ai`'s + // convertToLanguageModelMessage then filters down to `content: []`, + // yielding a provider 400 ("user messages must have non-empty content"). + return 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 +22,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${content.text || "(empty)"}\n` } export function renderActorNotification(event: { diff --git a/packages/opencode/src/tool/actor.ts b/packages/opencode/src/tool/actor.ts index bd9b4bf63..c4136e5ab 100644 --- a/packages/opencode/src/tool/actor.ts +++ b/packages/opencode/src/tool/actor.ts @@ -178,6 +178,17 @@ 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) + // Parity with the JSON path's `content: z.string().min(1)`. shell-wrap + // calls def.execute(parsed) directly, so a shell-parsed op is NEVER + // re-validated against `parameters` — without this, `actor send main ""` + // queued a body-less inbox row that drain() then rendered into an + // unusable synthetic user text part. + if (rest[1] === "") + 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/empty-notification-part.test.ts b/packages/opencode/test/inbox/empty-notification-part.test.ts new file mode 100644 index 000000000..082564bc7 --- /dev/null +++ b/packages/opencode/test/inbox/empty-notification-part.test.ts @@ -0,0 +1,201 @@ +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" + +// Producer of the empty-user-content provider 400. +// +// 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 rendered 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") + + // The reachable trigger: `actor send "" --type actor_notification`. + // The JSON path's `content: z.string().min(1)` is bypassed in shell mode + // (shell-wrap calls def.execute(parsed) without re-validating), so an + // empty body did reach Inbox.send in production. + 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/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..905030d77 100644 --- a/packages/opencode/test/tool/actor.shell.test.ts +++ b/packages/opencode/test/tool/actor.shell.test.ts @@ -198,6 +198,20 @@ describe("actor.shell.parse: send", () => { expect(err.kind).toBe("arity") expect(err.detail).toContain("to_actor_id") }) + + // The JSON path declares `content: z.string().min(1)`, but shell-wrap routes a + // shell-parsed op straight to def.execute WITHOUT re-validating it against + // `parameters` — so an empty token used to reach Inbox.send, queue a body-less + // row, and become an unusable synthetic user text part after drain(). Reject it + // here so the model gets a loud, self-correctable error instead. + test("send with an empty content token is rejected (parity with the JSON min(1))", async () => { + const exit = await Effect.runPromise(Effect.exit(parseActorScript('actor send main "" --type actor_notification'))) + 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") + }) }) describe("actor.shell.parse: full parity flags", () => {