Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions packages/opencode/src/inbox/render.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,14 @@ export function renderInboxRow(row: InboxRow): string {
// Pre-rendered notification text — sender produced the full
// <actor-notification>...</actor-notification> 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 <inbox> element so
// the LLM can route by sender; the wrapper format mirrors the
Expand All @@ -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 `<inbox from="${sender}" sent_at="${sentAt}">\n${content.text ?? "(empty)"}\n</inbox>`
return `<inbox from="${sender}" sent_at="${sentAt}">\n${content.text || "(empty)"}\n</inbox>`
}

export function renderActorNotification(event: {
Expand Down
11 changes: 11 additions & 0 deletions packages/opencode/src/tool/actor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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", '<to_actor_id> "<content>" [--session <id>] [--type <t>]', 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,
Expand Down
201 changes: 201 additions & 0 deletions packages/opencode/test/inbox/empty-notification-part.test.ts
Original file line number Diff line number Diff line change
@@ -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<Inbox.Service | Session.Service | ActorRegistry.Service | Bus.Service, never>

async function withInbox(directory: string, fn: (rt: RT) => Promise<void>) {
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 <id> "" --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)
})
})
})
41 changes: 41 additions & 0 deletions packages/opencode/test/session/message-v2.test.ts
Original file line number Diff line number Diff line change
@@ -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"
Expand Down Expand Up @@ -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.<N>: 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"

Expand Down
14 changes: 14 additions & 0 deletions packages/opencode/test/tool/actor.shell.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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", () => {
Expand Down
Loading