fix(inbox): stop the drain from persisting an empty user text part (the Bedrock 400 producer) - #1963
fix(inbox): stop the drain from persisting an empty user text part (the Bedrock 400 producer)#1963wqymi wants to merge 1 commit into
Conversation
…he Bedrock 400 producer) `64a128523` made createUserMessage refuse to PRODUCE an empty-content user message, and #1948 adds a pre-send backstop. Neither covers `Inbox.drain` — the one user-message producer that does NOT go through createUserMessage, so `hasSubstantiveContent` (prompt.ts:4386) never inspects it. This closes that producer. Root cause: - renderInboxRow (inbox/render.ts:8) used `content.text ?? "(no notification body)"`. `??` only catches null/undefined; a blank body is stored as "" (inbox.ts:162 `content: { text: input.content }`), and an `actor_notification` body is passed through RAW with no wrapper. So a blank notification rendered to "". - Inbox.drain (inbox/inbox.ts:268-277) wrote one text part per row unconditionally, so that "" was persisted as a user message whose only part is {type:"text",text:""} — parts.length === 1, which every `content.length === 0` check misses. - ai@6.0.168's convertToLanguageModelMessage user branch filters empty text parts out with NO backfill (`.filter(p => p.type !== "text" || p.text !== "")`), so that message reaches the provider as `content: []` and is rejected with `messages.<N>: user messages must have non-empty content`. - The blank body is reachable: the `actor` tool's JSON path guards content with `z.string().min(1)` (tool/actor.ts:454) but the SHELL path (tool/actor.ts:177-189) did not, and shell-wrap.ts:121 hands a parsed shell op straight to execute without re-validating it against `parameters`. Changes (three layers, outermost first): 1. tool/actor.ts — the shell `send` parser rejects a blank content, restoring parity with the JSON path's `.min(1)`. 2. inbox/render.ts — a `blankTo` helper substitutes the placeholder for any BLANK body, not just a missing one, for both row kinds. A blank notification is now visible to the model as "(no notification body)" rather than silently empty. 3. inbox/inbox.ts — the drain renders BEFORE writing anything and drops any row that renders blank; if every row renders blank it consumes the rows without writing a message at all. This is the structural invariant that keeps the fatal shape unreachable regardless of what a future row type renders. Tests: - test/inbox/drain-no-empty-part.test.ts (new): renderInboxRow never returns a blank string (empty / whitespace-only / missing body, plus verbatim passthrough for a real body); end-to-end, a blank actor_notification drained into a session yields exactly one NON-blank synthetic part, and a blank mixed with a real one keeps both parts non-blank. - test/tool/actor.shell.test.ts: the shell `send` parser rejects "" and whitespace-only content, and still accepts a short non-blank one. #1948's ensureNonEmptyContent is deliberately left intact — defence in depth.
…sts a blank part Consolidates the "empty content reaches the provider" family into one PR. Supersedes #1963, keeping its stronger version wherever the two overlapped: - src/inbox/render.ts: `blankTo()` helper, so any BLANK body (not merely a missing one) gets the placeholder. Replaces the bare `??` -> `||` change. - src/inbox/inbox.ts: the drain renders BEFORE writing and drops any row that renders blank; if every row renders blank it consumes them without writing a message at all. This is the structural invariant — the drain is the one user-message producer that bypasses createUserMessage/hasSubstantiveContent (src/session/prompt.ts:2164, :4386), so it needs its own guarantee. - src/tool/actor.ts: keeps the shell-boundary rejection, upgraded to `.trim()` so whitespace-only bodies are rejected too (zod's `min(1)` accepts " "). Also corrects a comment that asserted the opposite of the truth: a shell-parsed op IS re-validated against `parameters`. shell-wrap.ts calls `def.execute(parsed)` on the def from Tool.init, which is wrap()-decorated, and wrap() runs `parameters.parse(args)` inside execute. Verified end-to-end: with the parse-level guard removed, the real shellWrap route still enqueues nothing and reports "Too small: expected string to have >=1 characters -> at operation.content". A blank `actor send` was never reachable, so the render/drain work closes a LATENT defence gap rather than a live producer. New test test/inbox/empty-notification-reachability.test.ts drives that composition. The send-side backstop `ensureNonEmptyContent` (src/provider/transform.ts) stays: it remains the guard for the other message producers.
|
Folded into #1948 and closing this in favour of it. The user's rule is that one defect family is one PR, and this PR and #1948 were editing the same two files ( Nothing from here was dropped — this PR's version won every overlap:
Verified in the fold: reverting Two corrections to the framing carried over, worth recording here:
The guard added here is kept on its own merits: it turns a generic zod dump into one specific, teachable message, and Branch |
|
Closing in favour of #1948, which now carries this entire defect family in one PR (the user's rule: one defect family, one PR — not scattered across three). Nothing here is lost. #1963's version won every overlap:
Verified the fold did not neutralise anything: reverting Two corrections applied while folding, both worth reading:
The branch |
1 participant
What this is
The producer behind the live Bedrock 400
messages.<N>: user messages must have non-empty content.64a128523("never PRODUCE an empty-content user message") guardedcreateUserMessage, and #1948 adds a pre-send backstop. Neither coversInbox.drain— the one user-message producer that does not go throughcreateUserMessage, sohasSubstantiveContent(prompt.ts:4386) never inspects it. This closes that producer.#1948's
ensureNonEmptyContentis deliberately left intact — defence in depth is wanted here.Root cause
renderInboxRow(src/inbox/render.ts:8) usedcontent.text ?? "(no notification body)".??only catchesnull/undefined; a blank body is stored as""(inbox.ts:162content: { text: input.content }), and anactor_notificationbody is passed through RAW with no wrapper. So a blank notification rendered to"".Inbox.drain(src/inbox/inbox.ts:268-277) wrote one text part per row unconditionally, so that""was persisted as a user message whose only part is{type:"text",text:""}—parts.length === 1, which everycontent.length === 0check misses.ai@6.0.168'sconvertToLanguageModelMessageuser branch filters empty text parts out with no backfill (.filter(p => p.type !== "text" || p.text !== "")), so that message reaches the provider ascontent: []→ the 400.actortool's JSON path guards content withz.string().min(1)(src/tool/actor.ts:454) but the shell path (src/tool/actor.ts:177-189) did not, andshell-wrap.ts:121hands a parsed shell op straight toexecutewithout re-validating it againstparameters.Changes (three layers, outermost first)
src/tool/actor.ts— the shellsendparser rejects a blank content, restoring parity with the JSON path's.min(1).src/inbox/render.ts— ablankTohelper substitutes the placeholder for any blank body, not just a missing one, for both row kinds. A blank notification is now visible to the model as(no notification body)rather than silently empty, so nothing is lost.src/inbox/inbox.ts— the drain renders before writing anything and drops any row that renders blank; if every row renders blank it consumes the rows without writing a message at all. This is the structural invariant that keeps the fatal shape unreachable regardless of what a future row type renders.Tests
test/inbox/drain-no-empty-part.test.ts(new, 7 tests):renderInboxRownever returns a blank string (empty / whitespace-only / missing body, plus verbatim passthrough for a real body); end-to-end, a blankactor_notificationdrained into a session yields exactly one non-blank synthetic part, and a blank mixed with a real one keeps both parts non-blank.test/tool/actor.shell.test.ts(+4): the shellsendparser rejects""and whitespace-only content, and still accepts a short non-blank one.Each test proven to fail without its src change
render.tsaloneexpect(count).toBe(1)→Received: 0render.tsandinbox.ts(= pristinemain)expect(received).not.toBe("")→Expected: not ""tool/actor.tsaloneexpect(exit._tag).toBe("Failure")→Received: "Success"Verification
bun typecheckexit 0. Affected suites (test/inbox/,test/tool/actor.shell.test.ts,test/tool/session-tool.test.ts): 127 pass / 1 skip / 0 fail.Full
bun testwas run on a superset of this diff; its 15 failures are all pre-existing load-flaky families on the build box (theprompt-effectloop/cancel/busy suite,Vcsbranch watchers, workflow timeouts) and none touch inbox, render, or the actor shell parser.Not in scope
Defect B —
recoverSessionArgssilently rewriting a correctsendinto acreate— ships separately as #1962.