fix(rebuild): make manual /rebuild perform an on-the-spot rebuild with writer-freshness + waiting UI - #1752
Merged
wqymi merged 15 commits intoJul 29, 2026
Conversation
wqymi
force-pushed
the
fix/rebuild-on-the-spot-rebuild-writer-freshness-waiting-ui
branch
6 times, most recently
from
July 20, 2026 12:14
591ef43 to
c676c62
Compare
wqymi
added a commit
that referenced
this pull request
Jul 20, 2026
PR #1752's core commit dropped noReply:true on the manual /rebuild success path (prompt.ts) so the runLoop would run after rebuild. That review-flagged 'deliberate behavior change' is the bug: a manual /rebuild is a user action whose intent is only to free/rebuild context — the user asked no question, so the model produces a spurious 'reply to nothing' turn (e.g. 'Ready for your next request'). Fix: restore noReply:true on the manual-/rebuild success path and clear busy status explicitly (the runLoop's onIdle no longer fires). The boundary is still inserted and the waiting UI ('Rebuilding context…' / 'Writing checkpoint…') still shows — only the spurious reply is gone. The AUTO-triggered rebuild path is untouched: it rebuilds mid-turn inside the runLoop and continues answering the pending user message, which is correct and necessary there. The distinction is structural — the auto path uses continue/return-continue inside loop() and never re-enters prompt(); only the manual handler calls prompt() with the synthetic note. Tests: rebuild-on-the-spot case 1 and case 2 now assert NO model reply after a manual /rebuild (llm.calls === 0, returned role != assistant), while still asserting the boundary insertion and busy-UI messages.
wqymi
force-pushed
the
fix/rebuild-on-the-spot-rebuild-writer-freshness-waiting-ui
branch
from
July 20, 2026 14:41
c676c62 to
b8a879d
Compare
wqymi
added a commit
that referenced
this pull request
Jul 22, 2026
PR #1752's core commit dropped noReply:true on the manual /rebuild success path (prompt.ts) so the runLoop would run after rebuild. That review-flagged 'deliberate behavior change' is the bug: a manual /rebuild is a user action whose intent is only to free/rebuild context — the user asked no question, so the model produces a spurious 'reply to nothing' turn (e.g. 'Ready for your next request'). Fix: restore noReply:true on the manual-/rebuild success path and clear busy status explicitly (the runLoop's onIdle no longer fires). The boundary is still inserted and the waiting UI ('Rebuilding context…' / 'Writing checkpoint…') still shows — only the spurious reply is gone. The AUTO-triggered rebuild path is untouched: it rebuilds mid-turn inside the runLoop and continues answering the pending user message, which is correct and necessary there. The distinction is structural — the auto path uses continue/return-continue inside loop() and never re-enters prompt(); only the manual handler calls prompt() with the synthetic note. Tests: rebuild-on-the-spot case 1 and case 2 now assert NO model reply after a manual /rebuild (llm.calls === 0, returned role != assistant), while still asserting the boundary insertion and busy-UI messages.
wqymi
force-pushed
the
fix/rebuild-on-the-spot-rebuild-writer-freshness-waiting-ui
branch
from
July 22, 2026 17:55
b8a879d to
e43b1ba
Compare
wqymi
added a commit
that referenced
this pull request
Jul 22, 2026
…d user turn
The manual /rebuild handler previously did the right thing (Step A:
rebuildFromCheckpoint → insertRebuildBoundary inserts the legitimate
rebuild boundary as a role:"user" message with a checkpoint part — the
SAME mechanism the auto-overflow and compaction rebuild paths use) but
then fabricated a SECOND, standalone role:"user" turn ("Context rebuilt
from the latest checkpoint…") via prompt({ noReply:true }). The auto
paths never create that: they just `continue` the runLoop to answer the
pending user message. noReply only suppressed the model REPLY; it did
NOT stop createUserMessage from persisting the fabricated user message.
So a manual /rebuild left extra role=user rows in the DB with zero
assistant replies — the earlier noReply commit was a band-aid on the
wrong layer.
Real fix: remove the fabricated prompt({ noReply:true }) call for every
/rebuild outcome. Manual /rebuild now mirrors the auto/compaction path —
insert the boundary and settle — WITHOUT a second user turn:
- success: return the freshly-inserted boundary message; surface
"Context rebuilt…" on the SessionStatus / Bus status channel; go idle.
- no usable checkpoint / degraded: return the existing last user message
(persist nothing new); surface "No checkpoint available…" on the
status channel; go idle.
Manual /rebuild is a user-initiated maintenance action with no pending
question, so after inserting the boundary it returns to idle (no model
turn, no auto-reply) — the auto path `continue`s only because it has a
pending message to answer. The noReply mechanism is preserved for other
callers (e.g. /goal clear).
Tests: rebuild-on-the-spot.test.ts now asserts that after a manual
/rebuild the message table gains EXACTLY ONE new message (the boundary,
role user + checkpoint part) — no fabricated "Context rebuilt…" user
turn and no assistant reply — and that the outcome is surfaced on the
status channel, not as a persisted user message.
Keeps #1752's core value intact: on-the-spot rebuild, writer-freshness,
and the "Rebuilding context…"/"Writing checkpoint…" busy UI.
…h writer-freshness + waiting UI
The /rebuild handler previously called rebuildFromCheckpoint() which only
inserted a boundary marker, then returned via prompt({noReply:true}) —
the runLoop was never entered, so no busy status was set (no spinner)
and no rebuild context was assembled on the spot.
Fix:
- Set session.status busy BEFORE the rebuild work so the TUI spinner
lights up immediately (wired through prompt.ts:2839 pattern → sync.tsx
→ prompt/index.tsx spinner rendering).
- Remove noReply:true on the rebuild-success path so the runLoop actually
runs — the model sees the rebuilt context boundary and produces a
response. The Runner's onIdle callback clears busy status automatically.
- Keep noReply:true only for the no-checkpoint case (no work to do),
with explicit idle status clear since the Runner won't handle it.
- The 3-case checkpoint-freshness semantics are preserved via
renderRebuildContext (checkpoint.ts:1112-1136):
1. Checkpoint exists + no writer → immediate rebuild (REBUILD_WAIT_MS not hit)
2. No checkpoint + writer running → wait FIRST_CHECKPOINT_WAIT_MS
3. Checkpoint exists + writer in-flight → wait REBUILD_WAIT_MS, fallback on timeout
Tests: 3 new tests in rebuild-on-the-spot.test.ts covering case 1
(immediate rebuild), case 2 (no checkpoint returns false), and a
source-level guard verifying busy status wiring and noReply removal.
All existing rebuild tests continue to pass.
…t exists
The previous implementation returned 'no checkpoint available' when
/rebuild fired on a cold session. Per the user's authoritative 3-case
design, case 2 requires actively spawning a checkpoint-writer, waiting
for it to finish, then rebuilding from the freshly-written checkpoint.
Changes:
- When hasCheckpoint() returns false and no writer is running, call
tryStartCheckpointWriter() to spawn one (promptOps stub since the
writer never reads it — it spawns as a subagent via spawnRef).
- Wait for the writer via waitForWriter() (5-min safety bound from
checkpoint.ts:985). On success, fall through to rebuildFromCheckpoint
which now finds the freshly-written checkpoint and inserts the boundary.
On failure/no-writer, show the no-checkpoint message as before.
- The busy status ('Rebuilding context…') is set before any work so the
TUI spinner lights up immediately; cleared by Runner's onIdle for the
rebuild path, or explicitly for the no-checkpoint fallback path.
- Updated case-2 test to assert the new behavior: source-level guard
verifying hasCheckpoint check, tryStartCheckpointWriter call,
waitForWriter call, and rebuildFromCheckpoint after writer success.
The /rebuild handler set status to busy without a message field, so the TUI showed a generic spinner indistinguishable from a normal turn. Add explicit messages so users see what phase they're in: - 'Rebuilding context…' for the initial busy status (all 3 cases) - 'Writing checkpoint…' for the case 2 writer-wait phase The busy type (SessionStatus) already supports an optional message field; the TUI renders it via component/prompt/index.tsx:1853-1859. No TUI changes needed. Updated test to assert both message strings are present in the source.
…include 'reasoning' The interleaved `field` literal was extended to include 'reasoning' and the SDK types were regenerated (upstream via #1819), so the plugin `models` signature's ProviderV2 param (from @mimo-ai/sdk/v2) now accepts the local Provider shape. The `as any` that papered over the prior type break is no longer needed; removing it keeps `bun typecheck` green without suppressing the type (repo rule: avoid `any`).
…grepping prompt.ts source
The prior tests regex-matched the source text of prompt.ts (tryStartCheckpointWriter /
waitForWriter / busy-message strings) and called insertRebuildBoundary directly —
verifying nothing about runtime behavior and breaking on any harmless refactor
(violates AGENTS.md: 'Test actual implementation, do not duplicate logic into tests').
Rewritten to drive SessionPrompt.Service.command({ command: REBUILD }) — the same
path a user hits — against a scripted-LLM Bun.serve stub, asserting observable
outcomes:
- case 1 (checkpoint on disk + watermark): a checkpoint boundary message is
inserted and the handler enters the runLoop (model reply produced).
- case 2 (no checkpoint): a controlled spawnRef writer stub writes a fresh
checkpoint + advances the watermark, exercising the real spawn -> wait ->
rebuild path; asserts the boundary is inserted and the model replies.
- case 2 fallback (no spawnable writer): surfaces the no-checkpoint message with
noReply and inserts no boundary.
- busy status: captures the real 'Rebuilding context…' / 'Writing checkpoint…'
messages off the process-wide GlobalBus (no source-text assertions).
No mocks of the code under test; the spawnRef seam and scripted LLM stub the
system boundaries only (same pattern as checkpoint-rebuild-nonblocking.test.ts /
prompt.test.ts).
PR #1752's core commit dropped noReply:true on the manual /rebuild success path (prompt.ts) so the runLoop would run after rebuild. That review-flagged 'deliberate behavior change' is the bug: a manual /rebuild is a user action whose intent is only to free/rebuild context — the user asked no question, so the model produces a spurious 'reply to nothing' turn (e.g. 'Ready for your next request'). Fix: restore noReply:true on the manual-/rebuild success path and clear busy status explicitly (the runLoop's onIdle no longer fires). The boundary is still inserted and the waiting UI ('Rebuilding context…' / 'Writing checkpoint…') still shows — only the spurious reply is gone. The AUTO-triggered rebuild path is untouched: it rebuilds mid-turn inside the runLoop and continues answering the pending user message, which is correct and necessary there. The distinction is structural — the auto path uses continue/return-continue inside loop() and never re-enters prompt(); only the manual handler calls prompt() with the synthetic note. Tests: rebuild-on-the-spot case 1 and case 2 now assert NO model reply after a manual /rebuild (llm.calls === 0, returned role != assistant), while still asserting the boundary insertion and busy-UI messages.
…d user turn
The manual /rebuild handler previously did the right thing (Step A:
rebuildFromCheckpoint → insertRebuildBoundary inserts the legitimate
rebuild boundary as a role:"user" message with a checkpoint part — the
SAME mechanism the auto-overflow and compaction rebuild paths use) but
then fabricated a SECOND, standalone role:"user" turn ("Context rebuilt
from the latest checkpoint…") via prompt({ noReply:true }). The auto
paths never create that: they just `continue` the runLoop to answer the
pending user message. noReply only suppressed the model REPLY; it did
NOT stop createUserMessage from persisting the fabricated user message.
So a manual /rebuild left extra role=user rows in the DB with zero
assistant replies — the earlier noReply commit was a band-aid on the
wrong layer.
Real fix: remove the fabricated prompt({ noReply:true }) call for every
/rebuild outcome. Manual /rebuild now mirrors the auto/compaction path —
insert the boundary and settle — WITHOUT a second user turn:
- success: return the freshly-inserted boundary message; surface
"Context rebuilt…" on the SessionStatus / Bus status channel; go idle.
- no usable checkpoint / degraded: return the existing last user message
(persist nothing new); surface "No checkpoint available…" on the
status channel; go idle.
Manual /rebuild is a user-initiated maintenance action with no pending
question, so after inserting the boundary it returns to idle (no model
turn, no auto-reply) — the auto path `continue`s only because it has a
pending message to answer. The noReply mechanism is preserved for other
callers (e.g. /goal clear).
Tests: rebuild-on-the-spot.test.ts now asserts that after a manual
/rebuild the message table gains EXACTLY ONE new message (the boundary,
role user + checkpoint part) — no fabricated "Context rebuilt…" user
turn and no assistant reply — and that the outcome is surfaced on the
status channel, not as a persisted user message.
Keeps #1752's core value intact: on-the-spot rebuild, writer-freshness,
and the "Rebuilding context…"/"Writing checkpoint…" busy UI.
`/rebuild` inserts one user message whose parts are a `checkpoint` part plus `synthetic: true` text parts. The TUI's PART_MAPPING covers only text/tool/reasoning, and UserMessage renders only when a NON-synthetic text part exists — so the whole boundary message drew zero rows and the user had no way to tell a rebuild happened, or where. Render the checkpoint part as a one-line badge row, reusing the badge pattern already used for cron fires and actor notifications. Deliberately a render-only fix: rebuild's model-facing context is NOT changed. Compaction gets its visibility for free because it writes a real `summary: true` assistant message (compaction.ts) whose text renders through TextPart; rebuild instead puts its summary inline on the boundary user turn as synthetic text. Both DO reach the model — the user-part converter filters on `!part.ignored`, not `!part.synthetic` (message-v2.ts) — so rebuild's context is already at least as informative as compaction's, without a second LLM call. Copying compaction's assistant-message shape would duplicate that text in context and alter model semantics for no comprehension gain, so only the render layer moves. test/session/rebuild-boundary-model-context.test.ts pins that equivalence: the synthetic rebuild content and the compaction summary assistant turn both survive into the model messages, and filterCompacted keeps the summary message.
wqymi
force-pushed
the
fix/rebuild-on-the-spot-rebuild-writer-freshness-waiting-ui
branch
from
July 27, 2026 12:13
e43b1ba to
054775d
Compare
…turn
Solid's store setter merges plain objects into the existing node, so
`setStore("session_status", id, status)` kept every field the incoming
status omitted. The runner opens each turn with a bare `{type:"busy"}`
(session/run-state.ts:74), which therefore inherited the `message` of the
previous status — the `/rebuild` outcome sentence emitted by
`settle()` (session/prompt.ts:4173) stayed on the spinner for the whole
following turn, describing work that was already over. Wrap the incoming
status in `reconcile()` so each status event is authoritative for the whole
object; the durable confirmation stays the transcript boundary marker.
Harden the footer while there: that sentence wrapped to three lines and
squeezed the context counter into `52.4K/96` instead of `52.4K/960K`. Clamp
the status message to a fixed cell budget (and flatten newlines) and give
the counter `flexShrink={0}` so it is never the thing that gives way.
… compaction
The auto context-overflow path and the manual /rebuild command were asymmetric
in the same situation. Both share rebuildFromCheckpoint, which only checks
hasCheckpoint and returns false — it never starts a writer. Manual /rebuild
handled `false` by starting a writer and waiting for it; the auto path gave up
and called compaction.create. So with no checkpoint, a human asking got one
created, while an automatic overflow silently degraded.
Close it in one shared helper, rebuildEnsuringCheckpoint, used by all three
sites (token-threshold overflow, provider-signalled overflow, manual /rebuild).
It returns a discriminated outcome so there is exactly ONE compaction fallback
condition in the file rather than three lookalikes that can drift:
"rebuilt" boundary inserted; context freed.
"no-checkpoint" no checkpoint AND the writer failed / never ran / the bound
expired. The ONLY state in which a caller may compact.
"insert-failed" a checkpoint DOES exist but the boundary insert refused.
Reported honestly; must NOT compact, since compacting would
amputate history a usable checkpoint was available for.
Manual /rebuild now also compacts on "no-checkpoint". That reverses this
branch's earlier deliberate choice not to, which reasoned that /rebuild means
"rebuild from a checkpoint" so substituting a summary would misreport what
happened; the user overruled it — if the writer genuinely failed, a truncating
compaction beats doing nothing. The substitution is named on the status channel
instead of swapping mechanisms silently, and neither an assistant reply nor a
synthetic user turn is fabricated (3244ca7 stands). The defensive
writer-succeeded-but-insert-failed exit no longer reuses the no-checkpoint text,
which gave false advice there ("continue the conversation and a checkpoint will
be written automatically" — it already was); it now reports its own degraded
state.
Why wait at all, given compaction is cheap: measured, compaction.create costs
p50 0.240ms / mean 0.905ms over 20 calls — it is NOT an LLM summarization call.
It writes one synthetic message, one compaction part, and one bus event
(compaction.ts:499). The LLM summarizer is processCompaction, reached only from
/compact. Because MessageV2.filterCompacted breaks at the marker
(message-v2.ts:1037) and no summary is ever generated, falling back drops all
pre-boundary history unsummarized. So the trade is not slow-vs-slow; it is a
bounded, explained stall versus silently amputating the conversation. The
in-code claim that this fallback was an "LLM-driven lossy summary" was wrong and
is corrected.
Bound: waitForWriter takes no timeout argument (its 5-min bound is hardcoded at
checkpoint.ts:986), so the bound is applied at the call site. Manual keeps 5 min
— a human is watching. Auto gets 3 min: it fires mid-turn unrequested, and 180s
is the top of the 60-180s band the writer documents for itself, so it admits
every writer behaving as designed while refusing to hold an unasked-for turn as
long as a watching human would tolerate. Abandoning the wait does not cancel the
writer, so too tight a bound costs one degraded turn, not the checkpoint.
Reentrancy: the isWriterRunning probe plus tryStartCheckpointWriter's own
"queued" result mean two writers can never start for one session, even across
successive runLoop iterations. The auto sites keep their skipOverflowCheck /
continue shape on "rebuilt" and "no-checkpoint"; on "insert-failed" they fall
through to the model call rather than continue, since nothing freed context and
the provider-signalled handler is the backstop.
Auto overflow now shows "Writing checkpoint…" on the status channel so a
multi-minute mid-turn wait is explained rather than looking frozen.
…hat encoded the old shape
New test/session/auto-overflow-writer-first.test.ts drives the REAL main-agent
overflow branch in the runLoop against a scripted LLM and asserts on what the
session ends up containing — a `checkpoint` boundary part vs a `compaction` one:
- no checkpoint + writer succeeds → checkpoint boundary, zero compactions.
Revert-probed against eb41838: fails with
expect(checkpoints.length).toBe(1) Expected: 1 Received: 0
- no checkpoint + writer genuinely fails → still compacts, zero checkpoints.
Passes both before and after, by design: it guards behaviour the change
preserves, so it must not flip.
Two things were load-bearing to make the first test actually discriminate rather
than pass for the wrong reason:
1. The writer stub must NOT finish instantly. prune.fireCheckpoints runs
immediately before the overflow check (prune.ts:289) and already calls
tryStartCheckpointWriter, which scaffolds an empty template file
(checkpoint.ts:650). So the realistic state at the overflow check is: file
exists, watermark unset, writer in flight. An instant stub would have
advanced the watermark first, and the old code would have rebuilt too.
Effect.forkDaemon does not exist in effect 4.0.0-beta.48 — an earlier draft
used it, silently did nothing, and made the test pass for the wrong reason;
it uses Effect.forkDetach.
2. Seeds must carry agentID "main". The runLoop reads its slice with
agentID "main" (page() filters agent_id), so agentID-less seeds are
invisible and the overflow check never fires.
3. Config must satisfy two independent floors: reserves() adds a 20_000 output
reservation on top of compaction.reserved, so max_context must clear ~20_100
to be honoured at all, AND leave usable > 13_000 or resolveThresholds
rejects the window. max_context 40_000 gives usable 19_900 vs 50_000 seeded.
Deliberately rewritten, NOT quietly edited:
- rebuild-on-the-spot.test.ts "case 2 fallback" asserted
`after.length === countBefore` — i.e. that a manual /rebuild whose writer
could not run must NOT compact. That encoded the tradeoff the user has since
overruled, so the assertion stated the wrong behaviour and had to be
replaced, not tweaked. It now asserts one `compaction` part plus the status
text naming the substitution, and still asserts everything the old test
protected: no runLoop entry, no assistant reply, no fabricated user turn.
Revert-probed: fails with
expect(after.length).toBe(countBefore + 1) Expected: 2 Received: 1
- prompt-rebuild-reset.test.ts and command/rebuild.test.ts are source-grep
guards keyed to the old code TEXT (`const inserted = yield*
rebuildFromCheckpoint(...)`, and the deleted "No checkpoint is available to
rebuild from yet" string). Their invariants are unchanged and still asserted;
only the shape moved. The deleted string is not merely reshaped — it gave
false advice at the one exit that could still reach it.
Scoped run (8 files): 81 pass, 0 fail, loadavg 5.41.
Pristine baseline at eb41838 (same command, 7 pre-existing files): 79 pass,
0 fail, loadavg 4.66 — the +2 are exactly the new tests. bun typecheck exit 0.
… one; name the outcome for what it did Two problems, both about the same discriminator. 1. The compaction gate keyed on a bare hasCheckpoint, which is Bun.file(...).exists() (checkpoint.ts:1021) — and tryStartCheckpointWriter scaffolds an EMPTY TEMPLATE (checkpoint.ts:650) BEFORE it spawns the writer, while prune.fireCheckpoints (prune.ts:289) runs immediately before the overflow check. So 'template on disk, watermark unset' is a NORMAL arrival state, and classifying it as insert-failed skipped the start-and-wait entirely, silently defeating the helper. A usable checkpoint now requires the boundary too, which is what rebuildFromCheckpoint actually reads. 2. The outcome was named 'no-checkpoint', i.e. for the state the attempt STARTED in. At a call site that reads 'if (attempt === "no-checkpoint") compact()', which says the opposite of what the code does — the writer is started and awaited first, and only its failure reaches compaction. Renamed to 'writer-failed' and each call site now says so in one line. A user reading the call site concluded, reasonably, that no checkpoint meant immediate compaction. Behaviour of (2) is unchanged; it is a naming fix, and it is here because a name that makes the call site lie is how the next reader breaks this.
…rd a no-op 5189f19 re-keyed the compaction discriminator off bare `hasCheckpoint` and onto `lastBoundary`, which was the right value to read. It was written as `boundary !== undefined`, and that predicate never excluded anything: `lastBoundary` returns a nullable column behind an unchecked `as MessageID | undefined` cast (checkpoint.ts:1422), so an unset watermark arrives as JS `null`, and `null !== undefined` is true for every session that has a file on disk. The guard therefore behaved exactly like the bare `hasCheckpoint` check it replaced. That matters because the state it was meant to admit is the ORDINARY one. `prune.fireCheckpoints` (prune.ts:289) runs immediately before the overflow check and `tryStartCheckpointWriter` scaffolds an empty template (checkpoint.ts:650) before spawning the writer, so "file present, watermark unset" is how a normal rebuild arrives. Classifying it as `insert-failed` skipped the start-and-wait entirely, and `insert-failed` is the one outcome that neither rebuilds nor compacts — the turn falls through to the model call with the overflow unresolved. Zero checkpoints, zero compactions, no error: the helper was silently defeated in its main case. Two fixes, either of which is sufficient; both kept because the first makes the declared type honest for future callers and the second makes the two readers of this value agree: - checkpoint.ts: return `?? undefined` so the value matches the signature declared at :489 instead of relying on every caller to test truthiness. - prompt.ts: compare by truthiness, the same test `rebuildFromCheckpoint` applies to the same value at :413. Adds the regression test. It fails on 5189f19 with `Expected: 1 Received: 0` and passes here. The sibling writer-first test cannot catch this, because it seeds no checkpoint file and so never reaches the misclassified branch.
…ing instead of an as cast The fix in 95a3d6e was correct but still written as `(row?.col ?? undefined) as MessageID | undefined`, i.e. the thing that made the value honest and the thing that could hide it again were on the same line. Replaced with an annotated local, so the cast is gone: const boundary: MessageID | undefined = row?.last_checkpoint_message_id ?? undefined Measured, not assumed: deleting the `?? undefined` from this line is now a compile error — checkpoint.ts(1450,13): error TS2322: Type '(string & Brand<"MessageID">) | null | undefined' is not assignable to type '(string & Brand<"MessageID">) | undefined' whereas the cast form accepted it silently, which is exactly how the original defect survived typecheck and review. The comment is rewritten around the fact that made this subtle: two independent absences meet in `row?.column` and only one of them is undefined. Drizzle normalises the row-level absence (measured: bun:sqlite's .get() returns null, Drizzle returns undefined) while the column-level SQL NULL is faithfully preserved because the column is nullable. So the union is three-membered, flattening to undefined is the right call for every caller, and the previous comment's claim about 'the as cast below' is no longer true of this code.
Scoped to the one shape this branch actually hit. `.get()` yields undefined for a missing row (Drizzle normalises the driver's null — measured: bun:sqlite returns null, Drizzle returns undefined) while a nullable column's SQL NULL arrives as null, so `row?.column` is a three-membered union and an `as T | undefined` cast silently drops the middle one. Says three things: flatten to undefined at the boundary; write the flattening as an annotation so deleting it is a type error rather than a silent lie; and never discriminate with === / !== undefined, because null !== undefined is true and such a guard does nothing. No repo-wide audit here — 203 '| null' occurrences remain unclassified, and whether any of them is a harmful domain-layer mix is unmeasured.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
/rebuildonly inserted a DB boundary marker and returned — the actual context rebuild was deferred to the NEXT user message, with zero frontend feedback (no spinner, no text).rebuildFromCheckpoint()(which inserts a boundary) thenprompt({noReply:true})which short-circuited before the runLoop ever set busy status.session.statusbusy BEFORE rebuild work so the TUI spinner lights up immediately, with a descriptive message: "Rebuilding context…" (all cases) and "Writing checkpoint…" (case 2 writer-wait phase).noReply:trueon the rebuild-success path so the runLoop actually runs — the model sees the rebuilt context and produces a response; the Runner'sonIdleclears busy status automatically.noReply:trueonly for the no-checkpoint case with explicit idle clear.tryStartCheckpointWriter(), waits for it viawaitForWriter()(5-min safety bound), then rebuilds from the freshly-written checkpoint.3-Case Checkpoint-Freshness Semantics (via
renderRebuildContext)FIRST_CHECKPOINT_WAIT_MS(5 min)REBUILD_WAIT_MS(30 sec), fallback on timeoutFiles Changed
packages/opencode/src/session/prompt.ts:3964-4080—/rebuildhandler rewired with 3-case logic + busy messagespackages/opencode/test/session/rebuild-on-the-spot.test.ts— 3 new testsVerification
bun typecheckfrompackages/opencode: EXIT 0Follow-up fix — manual
/rebuildmust NOT auto-reply (b8a879d)Correction to item 2 above. Live TUI testing confirmed a regression: dropping
noReply:trueon the manual/rebuildsuccess path made the model emit a spurious reply (e.g. "Ready for your next request") after every manual rebuild. A manual/rebuildis a user action whose only intent is to free/rebuild context — the user asked no question, so a model reply is a "reply to nothing". The earlier review already flagged "Dropping noReply so the model responds after rebuild is a deliberate behavior change" — that change was the bug.Trigger-source distinction (the correct behavior):
/rebuild(user-initiated, no pending question) → rebuild, then STOP. No auto-reply.The distinction is structural: the auto path lives inside the runLoop and uses
continue/return "continue"(prompt.ts ~3188/3761) to keep answering the pending user turn — it never re-entersprompt(). Only the manual handler callsprompt()with the synthetic note.Fix (
prompt.tsmanual/rebuildsuccess path):noReply:trueso the boundary is inserted and the waiting UI still shows, but the runLoop is never entered — no spurious reply.onIdleno longer fires sinceloop()doesn't run) — mirrors the existing no-checkpoint paths.Tests (
rebuild-on-the-spot.test.ts): case 1 and case 2 now assert NO model reply after a manual/rebuild(llm.calls === 0, returned message role ≠assistant) while still asserting the boundary insertion and busy-status messages.Verification:
bun typecheckEXIT 0;rebuild-on-the-spot.test.ts4/4 pass; related rebuild/checkpoint tests (prompt-rebuild-loop, prompt-rebuild-reset, checkpoint-rebuild-unify, checkpoint-rebuild-nonblocking, rebuild-microcompact, command/rebuild) all pass. Rebased clean onto latestorigin/main.Real fix — remove the fabricated second user turn (e43b1ba)
This supersedes the
noReplyfollow-up above. ThenoReplyapproach was a band-aid on the wrong layer. Root-causing against the DB revealed the actual defect.Root cause (verified via the message table): The manual
/rebuildhandler did TWO things:rebuildFromCheckpoint()→insertRebuildBoundary()inserts the legitimate rebuild boundary as arole:"user"message carrying acheckpointpart. This is the SHARED mechanism the auto pre-turn overflow (prompt.ts~3205), auto mid-turn (~3778), and manual/rebuildpaths all use.prompt({ parts:[{text:"Context rebuilt from the latest checkpoint…", synthetic:true}], noReply:true })that fabricated a SECOND, standalonerole:"user"turn. The auto/compaction paths NEVER create this — they justcontinue/return "continue"the runLoop to answer the pending user message.noReply:trueonly suppresses the model REPLY;createUserMessagestill PERSISTS the fabricated user message. So after a manual/rebuildthe DB showed extrarole=userrows ("Context rebuilt from the latest checkpoint…") with ZERO assistant replies — the reply was hidden but the fabricated user turn remained.The correct fix: Make manual
/rebuildbehave exactly like the auto/compaction path — insert the boundary and settle transparently, WITHOUT fabricating a second user turn:prompt({…"Context rebuilt…", noReply:true}). ThenoReplyband-aid on the/rebuildpath is gone (still preserved for other callers such as/goalclear)."Context rebuilt from the latest checkpoint…"(success) /"No checkpoint is available to rebuild from yet…"(no checkpoint) are emitted on theSessionStatus/Busbusy-status channel — the same channel that drives"Rebuilding context…"/"Writing checkpoint…"— then the session goes idle./rebuildis a user-initiated maintenance action with NO pending question, so after inserting the boundary it finishes to idle (no model turn, no auto-reply). The auto pathcontinues only because it HAS a pending user message mid-runLoop; the manual handler has none, so it cleanly returns the boundary message and clears busy status. No broken/busy state, no auto-reply.Handler edit (before → after, success path):
(The no-checkpoint and degraded paths likewise drop
prompt({noReply:true}), surface the outcome viasettle(noCheckpointMsg), and return the existing last user message — persisting nothing new.)Tests (
rebuild-on-the-spot.test.ts): now assert that after a manual/rebuildthe message table gains EXACTLY ONE new message — the boundary (role:"user"+checkpointpart) — with no fabricated"Context rebuilt…"user turn and no assistant reply (llm.calls === 0, returned role ≠assistant). The no-checkpoint fallback asserts the count is unchanged (nothing persisted). All assert the outcome is surfaced on the busy-status channel, not as a persisted user message. The busy-message test ("Rebuilding context…"/"Writing checkpoint…") is unchanged and still passes.Verification:
bun typecheckEXIT 0 (full-repo turbo typecheck 12/12 on push);rebuild-on-the-spot.test.ts4/4 pass; 29 related rebuild/checkpoint tests pass (command/rebuild, rebuild-microcompact, checkpoint-rebuild-unify, prompt-rebuild-reset, prompt-rebuild-loop, checkpoint-rebuild-nonblocking, checkpoint-rebuild-v3, checkpoint-rebuild-fallback-decision). Rebased clean onto latestorigin/main. #1752's core value (on-the-spot rebuild + writer-freshness + busy"Rebuilding context…"/"Writing checkpoint…"UI) fully intact.UX completion — make the boundary VISIBLE in the transcript (054775d, folded in from #1940 which is now closed)
The real fix above leaves the manual
/rebuildboundary as the only new message. But that message rendered zero rows in the TUI, so a user who ran/rebuildsaw the spinner, then nothing — no way to tell it happened or where the boundary sits.Why nothing rendered: the boundary is a
role:"user"message whose parts are acheckpointpart plussynthetic: truetext parts.PART_MAPPING(tui/routes/session/index.tsx) covers onlytext/tool/reasoning— there is nocheckpointentry — andUserMessagerenders its text block only when a non-synthetic text part exists. Both conditions fail, so the whole message drew nothing.Why this is render-only: what each mechanism actually sends to the model
The obvious alternative was "reuse compaction's mechanism" — compaction is visible because it writes a real
summary: trueassistant message whose text renders throughTextPart. TracingtoModelMessagesEffect/filterCompactedshows why copying that shape would be a downgrade, not an upgrade:role:user+compactionpartrole:user+checkpointpart"Summary of previous conversation:"(message-v2.ts:709-713) — no content"Summary of previous conversation from checkpoint files:"(message-v2.ts:703-708) plus the rendered checkpoint index + rebuild context + actor registry, carried assynthetic: truetext parts (checkpoint.ts:1465-1497)summary: trueassistant message written byprocessCompaction(compaction.ts:337-364), needing a second LLM callsummaryfilter (message-v2.ts:786-792);filterCompactedstops at the boundary so the newer summary message survives (message-v2.ts:1025-1033)!part.ignored, not!part.synthetic(message-v2.ts:681-685), so all of the rebuild text is sentBoth boundaries therefore deliver a real, model-readable summary; rebuild's is arguably richer (structured index + rebuild context + actors) and cheaper (no summarizer call). The asymmetry was purely in the render layer:
synthetic: truehides text from the transcript, never from the model.So bolting a compaction-style assistant summary onto rebuild would duplicate that text in the context window and change model-facing semantics for zero comprehension gain. Only the render layer moves.
Change: render the
checkpointpart as a one-line badge row inUserMessage, reusing the badge pattern already used for cron fires and actor notifications:tui.session.rebuild_boundary.{label,detail}added for en / zh / zht.test/cli/cmd/tui/rebuild-boundary-marker.test.tspins the copy and asserts zh/zht are genuinely translated (not silently falling back to English through thebasemerge incontext/language.tsx).test/session/rebuild-boundary-model-context.test.ts(new) pins the semantics table above so a future refactor cannot silently change what the model receives: the synthetic rebuild content survives into the model messages, the compactionsummary: trueassistant turn survives into the model messages, andfilterCompactedkeeps the summary message after the boundary.No rebuild or compaction logic changed.
Verification:
bun typecheckEXIT 0 (12/12 turbo tasks).test/cli/cmd/tui/+checkpoint-rebuild-unify+ the new model-context test: 74 pass / 0 fail. All rebuild/checkpoint/message-v2 suites: 55 pass / 0 fail. Rebased clean onto latestorigin/main(eaac8b7ac).Follow-up (
eb4183825): a finished status message latched into the next turn, and squeezed the counterLive verification of this PR surfaced a second, render-layer defect. After
/rebuild, the whole following turn showed the rebuild outcome sentence on the spinner line instead of the current activity, and because that sentence wraps to three lines it squeezed the footer until the context counter rendered clipped as52.4K/96instead of52.4K/960K— the exact number this PR is about.Diagnosed mechanism (not a coalescing race, and not the server holding state):
settle()(session/prompt.ts:4173-4174) emitsbusy{message}thenidle. Server-side that is clean:SessionStatus.setpublishes and then deletes the map entry onidle(session/status.ts:77-80). No debounce, no dedupe, no "keep last message".setStore("session_status", sessionID, status)(cli/cmd/tui/context/sync.tsx:454). Solid's store setter merges a plain object into the existing node —updatePath→mergeStoreNodeonly writesObject.keys(next)(solid-js@1.9.10/store/dist/store.js:201-202,:139-145). Keys absent from the incoming status are retained.idlemerged down to{ type:"idle", message:"Context rebuilt…" }. Invisible, because the spinner row is gated onstatus().type !== "idle"(component/prompt/index.tsx:1884) — which is why it "only cleared when the session went fully idle".{ type: "busy" }with no message (session/run-state.ts:74). Merged over the latched object it became{ type:"busy", message:"Context rebuilt…" }, andbusyMessage()(component/prompt/index.tsx:1898-1901) rendered the stale sentence for the entire turn.Fix — TUI store layer,
sync.tsx. Asession.statusevent is authoritative for the whole status object, so it is wrapped inreconcile()(nextSessionStatus), which drops the fields the new status omits. Chosen over the alternatives because the bug is general (any status that omits a field inherited it —retry'sattempt/nextleaked the same way), not rebuild-specific; and becausesettle()'s emission is left intact for the non-TUI consumers (SDKsession.status, plugin API,runcompletion) and its existing tests. The user-visible confirmation is unaffected: it is the⟲ context rebuilttranscript boundary row added earlier in this PR, which is persisted and verified.Footer hardening (independent of clearing): the status message is clamped to a fixed cell budget with newlines flattened (
component/prompt/footer.ts,clampStatusMessage, 48 cells) and renderedwrapMode="none" flexShrink={1}; the context counter getsflexShrink={0}so it is never the element that gives way.Tests (each proven by reverting only the src change):
test/cli/tui/session-status-store.test.ts— status lifecycle against a real Solid store: a following turn's barebusydoes not inherit the rebuild message;idleclears;busyreplaces;retryfields do not survive. Revert probe (reconcile→ raw status): 3 fail, e.g.Expected {"type":"idle"}/Received {"type":"idle","message":"Context rebuilt from the latest checkpoint. …"}.test/cli/cmd/tui/prompt-footer-status.test.ts— over-long status clamped to the budget, newlines flattened, empty → nothing, and the budget arithmetic leaves room for spinner +esc interrupt+52.4K/960K (5%)on 80 columns. Revert probe (clamp removed): 2 fail,Expected: 48 / Received: 109.test/session/rebuild-on-the-spot.test.ts— now also asserts the terminal status isidle, so/rebuildcannot leave a busy status behind. Revert probe (dropsettle'sidle):Expected: "idle" / Received: "busy".Verification:
bun typecheckEXIT 0.test/cli/tui+test/cli/cmd/tui+rebuild-on-the-spot+run-completion: 214 pass / 0 fail (pristine baseline of the same command: 204 pass / 0 fail — the +10 are the new files). Live in a real TUI onmimo/mimo-v2.5, the turn immediately after/rebuild:No stale sentence, counter intact. And with a status genuinely in flight during the rebuild itself:
Auto overflow now writes a checkpoint before it degrades (closes an asymmetry with manual
/rebuild)The asymmetry. Both paths share
rebuildFromCheckpoint(prompt.ts:398), which only checkshasCheckpointand returnsfalse— it never starts a writer. The two callers then disagreed about whatfalsemeans:/rebuildtreated it as "make one": it calledtryStartCheckpointWriter, waited onwaitForWriter, and only then rebuilt.if (inserted) {…}then straight tocompaction.create.So in the same state — no checkpoint — a human who asked got one created, while an automatic mid-turn overflow silently degraded.
Fix. One shared helper,
rebuildEnsuringCheckpoint, used by all three main-agent sites (token-threshold overflow, provider-signalled overflow, manual/rebuild). It returns a discriminated outcome so there is exactly one compaction fallback condition instead of three lookalikes that can drift:"rebuilt""no-checkpoint""insert-failed"Per the narrowed rule, manual
/rebuildnow also compacts on"no-checkpoint". That reverses this branch's earlier deliberate choice not to (the reasoning being that/rebuildmeans "rebuild from a checkpoint", so substituting a summary would misreport what happened). If the writer genuinely failed, a truncating compaction beats doing nothing. The substitution is named on the status channel rather than swapped in silently, and neither an assistant reply nor a synthetic user turn is fabricated (thenoReplydecision in3244ca732still stands). The defensive writer-succeeded-but-insert-failed exit stopped reusing the no-checkpoint text, which gave false advice there — it said "continue the conversation and a checkpoint will be written automatically" when the checkpoint had already been written and continuing fixes nothing — and now reports its own degraded state."Usable" deliberately means what
rebuildFromCheckpointneeds — a checkpoint file and a watermark — not justhasCheckpoint.tryStartCheckpointWriterscaffolds an empty template file before spawning (checkpoint.ts:650) andprune.fireCheckpointsruns immediately before the overflow check (prune.ts:289), so a content-free template with no watermark is the normal state on arrival. Keying offhasCheckpointalone would have skipped the wait in the exact case the change exists for.Measured cost — and a correction to the premise
The obvious objection is that waiting 60–180 s mid-turn freezes the conversation, and the counter-argument offered was that compaction is also an LLM call, so it's slow-vs-slow. That counter-argument is wrong, and the measurement says so.
compaction.create(compaction.ts:499) performs three local operations — one synthetic message row, onecompactionpart row, one bus event — and no model call. Measured over 20 sequential calls on a 40-message session:(
maxis first-call warmup.) Against the writer wait, which the writer documents for itself atcheckpoint.ts:981as "frequently take 60-180s":compaction.createCompaction is ~250 000–750 000× cheaper. The cost-parity justification does not hold and is withdrawn.
The change still stands, but on quality, not cost. The LLM summarizer is
processCompaction, reached only from/compact— never fromcreate. Nothing generates the summary later either (the soleEvent.Compactedsubscriber,cron-bridge.ts:196, just resets a cache). SinceMessageV2.filterCompacted(message-v2.ts:1037) breaks at the marker, falling back truncates the conversation with no summary at all. So the real trade is a bounded, explained, one-time stall versus silently amputating the entire history — worth waiting for. The in-code comment claiming this fallback was an "LLM-driven lossy summary" that "preserves semantic content via summary" was factually wrong and is corrected. (In-tree corroboration predates this PR:checkpoint-rebuild-unify.test.ts:194already sayscreate"runs purely synthetically (no LLM)".)The bound: 3 min auto, 5 min manual
waitForWritertakes no timeout argument — its 5-minute bound is hardcoded atcheckpoint.ts:986— so the bound is applied by wrapping the call site rather than by passing an argument.waitForWriter's internal bound, i.e. unchanged. A human just typed/rebuildand is watching a spinner.Reentrancy. The
isWriterRunningprobe plustryStartCheckpointWriter's own"queued"result (rather than forking a second writer) mean two writers can never start for one session, even across successive runLoop iterations. On"rebuilt"and"no-checkpoint"the auto sites keep their existingskipOverflowCheck = true; continueshape; on"insert-failed"they fall through to the model call instead of continuing, since nothing freed context and the provider-signalled handler is the backstop. Auto overflow also sets"Writing checkpoint…"on the status channel so a multi-minute mid-turn wait is explained rather than looking frozen. The three subagentcompaction.createsites are untouched — subagents have no checkpoints, so checkpoint-first does not apply.Tests
test/session/auto-overflow-writer-first.test.ts(new) drives the real overflow branch in the runLoop and asserts on what the session contains:eb4183825:expect(checkpoints.length).toBe(1)→Expected: 1 / Received: 0.test/session/rebuild-on-the-spot.test.ts— rewritten, not tweaked. Its "case 2 fallback" test assertedafter.length === countBefore: that a manual/rebuildwhose writer could not run must not compact. That is now the wrong behaviour, so the assertion was replaced rather than edited quietly. It now asserts onecompactionpart plus the status text naming the substitution, and still asserts everything the old test protected (no runLoop entry, no assistant reply, no fabricated user turn). Revert probe:expect(after.length).toBe(countBefore + 1)→Expected: 2 / Received: 1.prompt-rebuild-reset.test.tsandcommand/rebuild.test.tsare source-grep guards keyed to the old code text. Their invariants are unchanged and still asserted; only the shape moved — except the deleted"No checkpoint is available to rebuild from yet"string, which was not reshaped but removed as false advice.Verification.
bun typecheckEXIT 0. Scoped run (rebuild.test.ts,auto-overflow-writer-first,prompt-rebuild-reset,cron-bridge.integration,rebuild-on-the-spot,checkpoint-rebuild-unify,checkpoint-rebuild-v3,overflow): 81 pass / 0 fail at loadavg 5.41. Pristine baseline of the identical command ateb4183825(7 pre-existing files; the new file does not exist there): 79 pass / 0 fail at loadavg 4.66 — the +2 are exactly the new tests, and nothing pre-existing regressed.Scaffolded arrival state: why the guard above was still a no-op (
95a3d6ea9)The commit before this one re-keyed the compaction discriminator off bare
hasCheckpointand ontolastBoundary. That was the right value to read, and itstill did nothing.
The trap.
hasCheckpointis literallyBun.file(checkpointPath(sid)).exists()(
checkpoint.ts:1021), andtryStartCheckpointWriterscaffolds an emptytemplate (
checkpoint.ts:650) before it spawns the writer. Sinceprune.fireCheckpoints(prune.ts:289) runs immediately before the overflowcheck, "checkpoint file on disk, watermark not yet written" is how a normal
rebuild arrives — not a degraded state, the ordinary one.
Why re-keying didn't fix it. The new guard was written as
boundary !== undefined, and that predicate never excluded anything.lastBoundaryreads a nullable column behind an uncheckedas MessageID | undefinedcast (checkpoint.ts:1422), so an unset watermarkarrives as JS
null— the declaredMessageID | undefinedwas simply untrueat runtime.
null !== undefinedistrue, so the guard fired for everysession with a file on disk and behaved exactly like the bare
hasCheckpointcheck it replaced. Nobody had noticed the nullability because every pre-existing
caller happens to test truthiness (
!boundaryatprompt.ts:413,watermarkBefore ?atcheckpoint.ts:1131,boundaryID ?innudgedSinceBoundary); this was the first strict-equality caller.Why it was invisible. The misclassification lands on
insert-failed, which isthe one outcome that neither rebuilds nor compacts —
prompt.ts:3515-3521deliberately falls through to the model call. So the overflow was left unresolved
with zero checkpoints and zero compactions and no error anywhere. A
scaffolded-state seed reproduces exactly that signature:
The overflow branch was entered and
tryStartCheckpointWriterwas neverreached; after the fix the same seed logs
boundary=undefined->tryStart=started->
writerOutcome=success->attempt=rebuilt.What closes it. Two one-liners, either of which is sufficient (verified
independently); both kept because the first makes the declared type honest for
future callers and the second makes the two readers of this value agree:
checkpoint.ts:1431— return?? undefined, so the value matches the signaturedeclared at
:489instead of relying on every caller to test truthiness.prompt.ts:528—if (hasCP && boundary), the same truthiness testrebuildFromCheckpointapplies to the same value at:413.Regression test.
auto-overflow-writer-first.test.ts— "a scaffolded-but-emptycheckpoint file still starts and awaits the writer". It pre-writes the empty
template exactly as
tryStartCheckpointWriterleaves it, then asserts onecheckpoint boundary, zero compactions, and a non-null watermark. Revert probe
against the previous commit's source, verbatim:
The sibling writer-first test cannot catch this: it seeds no checkpoint file, so
it never reaches the misclassified branch and passes with the bug in place.
Verification.
bun typecheckEXIT 0 (confirmed to covertest/— a deliberatetype error in the test file is reported). Scoped run of
auto-overflow-writer-first,rebuild-on-the-spot,prompt-rebuild-reset,command/rebuild: 11 pass / 0 fail. Pristine baseline of the identical commandat
5189f1974: 10 pass / 0 fail — the +1 is exactly the new test, and nothingpre-existing regressed.