fix(session): stop persisting empty-shell assistant messages and repair the readers that assumed the row exists

[wqymi]

Why

Read this first: this PR does not fix a live outage. The justification it was opened with has expired, and the honest current one is narrower.

What actually motivated it (evidence kept, conclusion retired)

A turn killed by a hard provider error leaves an assistant row carrying only bookkeeping parts (step-start / step-finish / patch) or no parts at all. In one live session (ses_0c3e12b6affeoSCn4d940Q5gIT) that produced 123 zero-part APIError assistant rows plus 3 rows whose only parts were step-finish (×2) or patch (×1), and 62 messages in that slice carry messages.<N>: user messages must have non-empty content (newest at messages.260 / messages.230). That history is real and stays on the record.

What has not survived is the causal story built on top of it — that these shells are still being replayed into later requests and still escalating into 400s. Two independent measurements retired it:

1. This PR's own replay experiment. Replaying that session's real persisted history (4228 main-slice messages) through the current converter and the full pre-send pipeline (toModelMessages → ProviderTransform.message, Bedrock model):

window=4228 outgoing=6335 emptyContent=[]
window=300  outgoing=422  emptyContent=[]
window=262  outgoing=368  emptyContent=[]
[bedrock pipeline] window=262 outgoing=369 problematic=[]

Today's main emits no zero-content message from that history.

2. The read side is behaviourally redundant. All four of this PR's read-side assertions were ported into a probe and run on pristine main: 4 pass / 0 fail. main already closes that path at

• session/message-v2.ts:699 — if (msg.parts.length === 0) continue
• session/message-v2.ts:912 — if (assistantMessage.parts.length > 0)
• session/message-v2.ts:941 — result.filter((msg) => msg.parts.some((part) => part.type !== "step-start"))
• provider/transform.ts:364, wired at :973 — merged fix(provider): never send a message with empty content (the Bedrock/Anthropic 400) — producer + backstop + inbox invariant #1948's ensureNonEmptyContent

⇒ The 400s are already prevented on main. Nothing in this PR is required to stop them.

So what is left worth merging

The read side: making an incidental guard explicit. main's protection is a side effect of a parts.some(...) filter that exists for a different reason. Delete that incidental filter and the difference shows up immediately:

converter source	incidental parts.some(...) filter removed
pristine main	2 fail — filters out messages with only ignored parts, drops a user message whose parts all convert to nothing
this PR	31 pass

Same observable behaviour today; different resilience to the next edit of that filter. That is the read side's entire value, and it is a clarity argument, not a bug fix.

The write side: a structural trade, with a cost this PR paid in public. Instead of requiring every reader of "the last message" to remember to filter an empty shell, the shell does not exist. But the trade is real and runs both ways:

it exchanges "every reader must filter empty content" for "every reader must handle the row being absent."

That is not a hypothetical cost. firePostSession (prompt.ts) is a second, independent reader that re-derives the turn's final assistant from the persisted slice. With the row deleted its findLast skipped the hole to an older successful assistant, finalIsError went false, and lifecycleStatus published "completed" for a failed turn — breaking merged #1851's contract and silently mis-reporting the session.post plugin outcome. CI caught it; inspection had not. Commit 128849986 fixed that reader by consulting droppedShell first.

Deleting a persisted row therefore creates an obligation to audit every reader that assumed it existed. That audit is below, and it is what discharges the cost.

Reader audit — every site that reads "the last message"

Re-derived on the branch head (line numbers below are from this branch, not main). Verdict per site, with the reasoning tied to file:line.

Two measurements against the local 5.4 GB DB decide most of it:

• The droppable population — errored, non-aborted, no substantive content — is 210 rows: APIError 160, UnknownError 37, InvalidOutputError 12, ContentFilterError 1, ContextOverflowError 1.
• 195 of those carry finish = NULL; 15 carry finish set. ctx.assistantMessage.finish is only assigned in case "finish-step" (processor.ts:588), which a hard 400 never reaches — but InvalidOutputError &co. do. So finish-gated readers are not automatically safe, and are assessed as affected.

#	site	verdict	reasoning
1	checkpoint.ts:258 findLastIndex(assistant && finish !== undefined)	affected, not broken	For the 15 finish-set shells, deletion moves lastAsstIdx earlier. Removing an element can never make findLastIndex select a later message, and startIdx = lastAsstIdx - 1 then only walks backward ⇒ the boundary can only move earlier or stay. Boundary = first message to preserve, so it can only preserve more, never discard more. No data loss possible in this direction. Separately, the boundary ID can never dangle: it is always ≤ lastAsstIdx - 1, i.e. strictly before the last assistant, while both deletions (cleanup, sweepOrphanAssistants) only ever remove a row that is last in its slice.
2	compaction.ts:241 findLastIndex(m => m.info.id === parentID)	unaffected	ID-equality lookup for the compaction parent, which must be a user message (throw at :243 otherwise). Deleting an assistant cannot change which row has id === parentID; the index shifts but is consumed within the same post-deletion array.
3	compaction.ts:255 findLastIndex(i < parentIdx && role === "user" && parts.some(compaction))	unaffected	Predicate filters role === "user"; the deleted row is an assistant and can never be selected.
4	compaction.ts:296 messages.at(-1)?.info.id === parentID	affected, strictly more correct	This is the one boundary site where the hole is visible: with a shell sitting after the compaction parent, at(-1) was the shell and the parent marker was fed to the summarizer; with the shell gone, at(-1) is the parent and slice(0, -1) correctly drops the marker. The check exists precisely to exclude the marker, so the deletion makes it fire as intended. Direction is exclusion of a synthetic marker, not of user content.
5	llm-request-prefix.ts:50 findLast(m => role === "user")	unaffected	Filters role === "user"; no index arithmetic, no positional dependency.
6	message-v2.ts:1023 slice.at(-1) (was :996 on main)	unaffected	Pagination cursor in page(). slice is the DB rows actually returned; tail is the oldest row of the page and is encoded as the cursor. A deleted row is simply absent from the result set — the cursor still names a row that exists.
7	prompt.ts:787 findLast(msg => role === "user")	unaffected	insertReminders' user-message target; filters role === "user".
8	prompt.ts:824 findLast(msg => role === "assistant")	affected, not broken	Consumed only at :944 as assistantMessage?.info.agent === "plan". With a plan-mode shell deleted, this reads the previous assistant; the two branches diverge only at the moment plan mode is entered, where the effect is that the full plan prompt is injected onto the new user message instead of the short "still active" reminder. No duplicate stacking (the :950 guard is per-message), no data loss, and the fuller prompt is the safer of the two.
9	prompt.ts:950 input.messages.at(-1) !== userMessage	unaffected	Detects "fresh user turn". Within a turn, the current assistant row exists while steps run — cleanup deletes only at turn end. Across turns, sweepOrphanAssistants runs before createUserMessage (prompt.ts:2474), so the new user message is last either way.
10	prompt.ts:1473 findLast(assistant && !!finish)	affected, not broken	For the 15 finish-set shells, lastFinished becomes an older real assistant. Effect: its tokens feed contextPressureLevel and slice(lastFinishedIndex + 1) covers more messages ⇒ pressure reads higher ⇒ a shorter MCP tool-search description. Conservative direction, and a real assistant's token counts are the more accurate input than a shell's.

Sites beyond the ten, in the same class as firePostSession (post-turn result derivation) — this is where the risk actually lives:

site	verdict	reasoning
prompt.ts:4188 final = droppedShell ? … : lastAssistant(…)	affected, already handled	The fix in 128849986. Verified reachable on every drop, not just some: a drop implies assistantMessage.error, which forces return "stop" at processor.ts:901, which forces outcome === "break" at prompt.ts:4003 — the only branch that assigns droppedShell. The droppedShell = undefined reset on the continue path is therefore unreachable after a drop.
actor/spawn.ts:276 if (info?.role === "assistant" && info.error) fail(…)	affected, already handled	Consumes runLoop's return value, so the same droppedShell patch is what lets a subagent's failed turn still surface as a failure instead of an older success. Worth naming: one fix repaired two readers.
actor/waiter.ts:67, actor/group.ts:99	unaffected	Both gated on success (entry.lastOutcome === "success" / outcome === "success") before reading the last assistant. A dropped turn is never a success.
prompt.ts goalGate findLast(assistant)?.info.id	affected, correct as-is	The goal verdict marker is anchored to the newest surviving assistant. It must be: the marker is a message ID rendered by the TUI, so anchoring it to the deleted row would produce a dangling reference. Not a defect.
prompt.ts:695 predict	affected, more correct	The per-turn assistant set is now empty ⇒ return "", instead of running a prediction off a contentless shell.
prompt.ts:3983 assistantFinalText(handle.message, MessageV2.parts(handle.message.id)) and :4014 parts:	unaffected	cleanup runs as Effect.ensuring (processor.ts:896), so these read the row after deletion and get []. Classification only inspects parts for tool / text; [] and [step-start, step-finish] are indistinguishable to it, and a shell has no final text either way.
prompt.ts:3123-3131 loop's backward scan for lastAssistant / lastFinished, consumed by classifyAssistantStep at :3192	affected, not broken	The classifier's guard #4 (phase === "existing-assistant" && !(lastUser.id < assistant.id) → continue) absorbs the hole: the older assistant predates lastUser, so it classifies continue, exactly as the 195 finish-NULL shells did via guard #2 on main. Across turns both sides agree for the same reason.
tool/session.ts:83,94, tool/session.ts:1183-1184, acp/agent.ts:99	unaffected	Cosmetic/derived only — fork watermark + agentName baseline, set-mode agent rewrite, context-limit lookup. A shell carries the same agent/providerID/modelID as the neighbouring real assistant.

Three defects the audit found, fixed in 3338fb03e

1. sweepOrphanAssistants could delete a live subagent's assistant row. It read agentID: "*" and removed the session-global newest incomplete assistant. SessionProcessor publishes status for the main slice alone — the very fact the adjacent sweepOrphanToolParts comment calls load-bearing — so a background actor can be mid-turn while session status reads idle. Its in-flight row is then exactly what the sweep deletes, out from under its own processor, which keeps calling updatePart against that messageID. The old stamping behaviour was recoverable; deleting is not. Now scoped to the main slice, which is all the idle gate proves.

2. Narrowing the sweep to the trailing message stranded every non-trailing orphan forever. The sweep runs before createUserMessage, so a single-slice orphan is trailing — but one is left non-trailing by a concurrent subagent row, or by a previous source: "spawn" | "hook" prompt that skipped the sweep block. The TUI's pending marker (routes/session/index.tsx:195) derives from the newest incomplete assistant regardless of what follows it, so a stranded row keeps rendering fresh messages as stuck QUEUED — the exact symptom the age-independent sweep was introduced to prevent. Measured on the local 5.4 GB DB: 85 such rows (78 mid-slice, 7 last-in-slice-but-not-last-in-session) across 33 sessions carrying 2+ incomplete assistants.

⚠️ This means an assertion introduced by 64bcd6d34 — "leaves history whose last message is a user message untouched" — pinned the regression rather than a requirement. Its stated rationale covered only the replay path, which the read side already closes, and not the stuck-QUEUED path. It has been replaced, and that replacement is called out here rather than made quietly.

3. filterCompactedEffect widened a fork boundary when the watermark row was deleted. message-v2.ts:1109 resolved a fork's contextWatermark by exact message ID and fell through to the full parent history when the row was missing. A context: "full" subagent captures that watermark from Session.lastMainMessageID (actor/spawn.ts:728), so a spawn issued inside a turn captures that turn's in-flight assistant — precisely the row the empty-shell guard then deletes. The child silently inherited more context than it was registered for: a boundary shift, not a display bug. MessageID is monotonic ascending, so the boundary survives the row; the fallback now truncates by ID. The resolved path is byte-identical and stays authoritative for array order, which is by time_created and can diverge from ID order (insertRebuildBoundary back-dates time.created while allocating an ascending ID).

What changed

Write side — SessionProcessor.cleanup

Do not persist a contentless errored assistant; delete the row instead. The error is not swallowed: halt already published it on Session.Event.Error and reset session status, and a new Handle.dropped flag makes SessionPrompt report the turn from the in-memory message.

A user abort is deliberately exempt: cancelling is a normal outcome the transcript should keep showing (pinned by the pre-existing records aborted errors when prompt is cancelled mid-stream regression test), and an errored assistant can never reach a provider anyway because toModelMessages' error gate drops it.

Write side — sweepOrphanAssistants

Age gate removed (the caller establishes idleness), and orphans are deleted rather than stamped. Scope: the main slice, and every incomplete assistant in it, not just the trailing one — see defects 1 and 2 above for why both halves of that are load-bearing.

Read side — restatement, plus one real fix

• toModelMessages emits a user message only when it converted to at least one part, symmetric with the existing assistant guard.
• The assistant error gate uses a shared exported hasSubstantiveAssistantContent predicate instead of an inline part-type check that counted step-finish and patch as content — the assistant-side mirror of hasSubstantiveContent in session/prompt.ts.
• filterCompactedEffect no longer discards a fork boundary when the watermark row is gone (defect 3). This one is a behaviour fix, not a restatement.

Tests — each verified as a real regression test

Every test was proved by reverting only its corresponding src change.

revert	observed failure
processor.ts	(fail) session.processor effect tests do not retry unknown json errors — expect(handle.dropped).toBe(true) → expect(received).toBe(expected) (received undefined); same for both abort tests on toBe(false)
prompt.ts sweep (original)	(fail) sweepOrphanAssistants > drops a trailing incomplete assistant regardless of age — expect(after.find(...)).toBeUndefined()
prompt.ts sweep (this round)	(fail) sweepOrphanAssistants > drops a NON-trailing incomplete assistant in the main slice — error: expect(received).toBeUndefined() / Received: { · and (fail) sweepOrphanAssistants > never touches a non-main slice, whose actor may still be mid-turn — error: expect(received).toBeDefined() / Received: undefined, i.e. pristine really does delete the live actor's row
message-v2.ts watermark	(fail) filterCompactedEffect contextWatermark > truncates at the boundary even after the watermark row is deleted — expect(received).toEqual(expected), diff + "msg_…5005hQbvxh2RBXSFkF" (the post-boundary message leaking in) · and (fail) … inherits nothing when the deleted watermark predates every surviving message — expect(received).toBe(expected) / Expected: false / Received: true
message-v2.ts converter	converter suite still 31 pass — reported honestly: on this head those protections already exist, so the new converter tests are characterization tests. Their load-bearing value is the 2-fail/31-pass table above.

Verification

• bun typecheck — exit 0, 12/12 tasks.
• Scoped run (prompt-sweep, context-watermark, message-v2, processor-effect, prompt-effect, compaction-agent-scope, rebuild-microcompact, checkpoint-boundary, checkpoint-main-slice, checkpoint-child-session): 126 pass / 5 skip / 0 fail vs the identical command on the pristine branch head 123 pass / 5 skip / 0 fail (+3 net new cases, zero failures either side). prompt-effect.test.ts — the suite carrying fix(mcp): negotiate per-turn lifecycle notifications #1851's turn-lifecycle contract — is in that set and green.
• Earlier rounds: sibling empty-content suites 25 pass / 1 fail, that failure being whitespace-only text skips loop and returns empty parts (5000 ms timeout), which reproduces identically on pristine main ⇒ pre-existing.

No assertion was weakened. One assertion was replaced — 64bcd6d34's non-trailing-orphan case — and it is reported as a finding above rather than adjusted in silence.

Deliberately left alone

• Existing prettier disagreements in the touched files — they pre-exist on main (verified by stashing and re-running prettier --check).
• ProviderTransform.normalizeMessages / ensureTrailingUserMessage — already correct.
• checkpoint.ts:258 and prompt.ts:1473 are left as-is: both are affected only for the 15 finish-carrying shells, and in both cases the shift is provably in the conservative direction (preserve more / assume more pressure). Fixing them would add a droppedShell dependency to two hot paths for no behavioural gain.

Title

Narrowed to the write side plus the reader repairs. The original "and replaying" half is now handled by main independently (message-v2.ts:699/912/941 + #1948's ensureNonEmptyContent), so claiming it would overstate what this PR contributes.

Supersedes the closed PR #1702 (branch 348 commits behind main); this is a fresh branch off latest origin/main.

---

Rebase onto origin/main (b816708)

Rebased 1da2f1ba2 → 729c51768 (onto f91eb6334) → 64bcd6d34 (onto b81670870, after #1782 merged mid-rebase). Two rounds because main moved.

Conflicts and how they were resolved

1. processor.ts cleanup — co-located, kept both, order is load-bearing.
#1960 added a DB-driven second pass finalizing orphaned tool parts; this PR adds the empty-shell guard. Both landed right after ctx.toolcalls = {} and collapsed onto the shared brace. #1960's pass now runs first, then the guard. Reason recorded inline at processor.ts:797: the guard ends in an early return, so running it first would skip #1960's finalization on the paths where the row survives. The reverse ordering is also unnecessary — hasSubstantiveAssistantContent counts any tool part as content regardless of state, so a turn that really called a tool is never dropped and the guard can only fire where #1960's loop is a no-op. Guarded by #1960's own suite: prompt-orphan-tool-parts 6 pass / 0 fail, identical to the pristine baseline.

2. prompt.ts interface + call site — co-located, kept both.
#1960 added sweepOrphanToolParts on the line below sweepOrphanAssistants, whose signature this PR narrows (immediate removed). Kept #1960's new member and the narrowed signature; the idle gate moved to the caller while sweepOrphanToolParts stays self-gated. Both now read the main slice only, and 3338fb03e records that shared scope and why it is load-bearing for both.

3. prompt.ts runLoop — a genuine contradiction, resolved in main's favour.
#1782 (b81670870) is a revert: it deleted the empty-step guard (emptyStepStreak, hardHalt, isEmptyStep, EMPTY_STEP_MAX_RECOVERY) from prompt.ts. Those symbols appear as context in this PR's hunks. Taking this PR's side wholesale would have resurrected code that was deliberately reverted. Resolution: honour the revert — kept only droppedShell and its outcome === "break" assignment, dropped emptyStepStreak/hardHalt entirely. Verified: 0 occurrences of all four reverted symbols remain in prompt.ts.