Skip to content

fix(dashboard): bound chat history by messages, not stream progress - #4327

Merged
bolichen97 merged 1 commit into
mainfrom
fix/collapse-unflushed-tail-4306
Aug 20, 2026
Merged

fix(dashboard): bound chat history by messages, not stream progress#4327
bolichen97 merged 1 commit into
mainfrom
fix/collapse-unflushed-tail-4306

Conversation

@chenmingwei23

@chenmingwei23 chenmingwei23 commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

1. What is the problem?

chunk is a wire-only role appended once per streamed delta, so a reply still
being typed occupies hundreds of rows that render as a single message. Two
endpoints bounded the live message window by raw row count before reducing
it
, so a bound meant to count messages counted stream progress instead:

  • GET /api/chat/slots/{slot} applied limit to raw rows. A caller asking for
    the last N messages of a running session spent its whole budget inside the
    unfinished reply and got a mid-sentence fragment with none of the conversation
    behind it.
  • POST /api/chat/slots/{slot}/resume filled its 200-row bound the same way.
    This one is worse: it truncates the in-flight reply itself to the newest 200
    deltas, so the reply comes back beginning mid-sentence.

Fixes #4306.

2. Why this issue matters to the user

Mid-response is the normal condition for anything watching an agent work, not
an edge case. The Worlds scene thread popover polls this endpoint on a 2s
interval (useSceneInteraction.tsx), so the fragment is what it sees for the
whole duration of every reply. On resume, a user reloading the page while a
reply is streaming gets that reply with its opening cut off.

3. How our fix solves it

Reduce the wire-only rows before bounding, on both paths: consecutive
chunk runs fold to one row, done terminators drop. A bound then counts
displayed messages, the in-flight text arrives whole, and total and the paging
cursor stop counting stream progress.

Chain from symptom to root cause:

  1. Symptom: a bounded read during streaming returns a fragment.
  2. Because limit selected raw rows, and hundreds of them belong to one message.
  3. Because the reduction that makes one row mean one message (_prepare_messages)
    ran after the bound, at render time.
  4. Root cause: the bound and the reduction were in the wrong order. The fix
    reorders them rather than adding a second notion of size.

_collapse_wire_rows (new, in chat_utils.py beside _prepare_messages) is
output-equivalent to leaving those rows for the render pass, so the rendered
response shape is unchanged -- it only moves the fold to before the bound, where
it can decide what the bound counts. It applies no redaction, rewrites no other
role, and never mutates the live window (a folded row is a new dict, and those
rows are shared with the running slot).

On the detail path the reduction runs inside the same worker thread as
_append_unflushed_tail (#4137) and covers the whole corpus, because that helper
places an owed row at the disk index it belongs to -- owed rows are not a
contiguous suffix, so a slice-scoped reduction would miss the interleaved ones.
done is already excluded there by _UNOWED_WINDOW_ROLES, so the remaining work
on that path is folding the chunk runs. The done drop stays load-bearing on
resume, which reads the window directly.

On resume this also puts both cursor terms in message units, since persisted rows
carry no wire-only role. Persisted history never carries these rows at all
(_build_message_entry_uncached returns None for every wire-only role), so the
scroll-back paging path is unaffected and next_before keeps its meaning as a
raw index. No cursor-space change, and no frontend change.

4. What tests we did

test/test_slot_detail_inflight_tail.py -- 13 tests over a fixture of 10 settled
turns plus a 300-delta in-flight segment: the bounded detail branch, the resume
branch, the fold itself, and the shapes that must not change.

Mutation-verified per mutant rather than by coverage:

  • dropping the detail reduction -> 5 tests fail
  • dropping the resume reduction -> 2 resume tests fail
  • keeping done -> the terminator and run-splitting tests fail
  • widening the fold over chained history -> caught by the output-equivalence test

Also re-ran the duplication probe from the review thread below (two settled
turns, all four rows persisted, window carrying those four plus one done per
turn, ?limit=50): the response is exactly ['u1','a1','u2','a2'] with no
repeat. That defect was pre-existing on main and is fixed by #4137, which has
now landed; this branch is rebased on it.

Gates: 13/13 on the new file; 13729 passed / 42 skipped across all 208 test files
referencing chat_utils, chat_handlers, _prepare_messages, slot_detail or
resume (including #4137's own tests); isort, flake8, black gate, and mypy
(1003 files) clean.

5. Any other suggestions on the work

The disk-side cost of a long transcript -- reading the whole chained history on
the unbounded branch -- is a separate concern and belongs to #4134. This PR only
changes what a bound counts, not how much is read.

done rows accumulate one per settled turn in the live window and nothing ever
removes them (appended at 9 turn-end sites, never persisted). Dropping them at
read time is correct for these endpoints, but the accumulation itself looks worth
a follow-up: the window carries one dead row per turn for the life of the slot.

@chenmingwei23
chenmingwei23 requested a review from a team as a code owner August 18, 2026 13:14
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Aug 18, 2026
@github-actions

github-actions Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

Design-level review of 3be1f8fb25058ca3e4ba6df0415a64c25f6d9aa3 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

Read-time normalization is the right shape: it fixes the bound, total, and cursor in one unit without touching the live-window representation.

The fix is aimed at a real, recurring condition (a poller mid-stream), and the collapsed index space stays consistent across both handlers — disk rows carry no wire-only roles, so _disk_older_count and the collapsed corpus count the same thing, which the old code did not.

Suggestions

  • _collapse_wire_rows and _prepare_messages' inline accumulator are now two implementations of one fold contract (chunk-run merge, done skipped without flushing) that must agree forever; refactoring _prepare_messages to consume _collapse_wire_rows output would make the equivalence structural instead of test-pinned.

[DESIGN-REVIEWED] 3be1f8f

@github-actions

github-actions Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 3be1f8fb25058ca3e4ba6df0415a64c25f6d9aa3 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 3be1f8f

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 3be1f8fb25058ca3e4ba6df0415a64c25f6d9aa3: <one-sentence reason>

@chenmingwei23
chenmingwei23 force-pushed the fix/collapse-unflushed-tail-4306 branch from 3c18f4e to e4d18b9 Compare August 18, 2026 13:24
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Disposition for the Design Review suggestion on _collapse_chunk_runs -- fixed, both halves, at e4d18b9d9.

The finding was right on both counts and the trigger is nameable: a multi-MB session with tens of thousands of chained rows, where the fold sat on the event loop between the threaded history read and the render offload.

Quadratic concatenation. The fold rebuilt the merged string once per delta, so it copied the text accumulated so far on every row -- quadratic in the reply size, and a long reply is hundreds of deltas. It now buffers the run and joins once:

def _merged(run: list[dict]) -> dict:
    if len(run) == 1:
        return run[0]
    return {**run[0], "content": "".join(m.get("content", "") for m in run)}

Whole-history work on the loop. This is the half that mattered more, and the suggestion is right that this endpoint is the wrong place to add it. The fold is now scoped to the un-flushed window tail rather than the assembled list:

all_msgs = list(all_msgs) + _collapse_chunk_runs(slot.messages[-unflushed:])

That makes it window-sized instead of history-sized, and it costs nothing in coverage: chunk is never persisted, so the chained rows hold no run to fold. Folding them was defensive over-reach on my part -- and inconsistent besides, since the unbounded branch does not fold disk rows either and leaves any transient role there to the render pass. Doing the same on this branch is the smaller and more consistent behaviour.

Not moved into the worker thread, which was the suggestion's other option: the tail read has to stay on the loop for the snapshot discipline the surrounding comment describes, and total / has_more / next_before are derived from the folded list before _render closes over them, so folding inside the thread would mean moving the slice and the cursor arithmetic in there too. Scoping to the tail removes the cost without that restructure.

Pinned by a new test, test_disk_rows_are_not_folded, which fails if the fold is widened back over the chained history. Worth noting that its first version passed for the wrong reason -- it left unflushed at zero, so the branch never executed -- and the mutation check is what caught that; it now carries an un-flushed row so the fold really runs, and total distinguishes the two placements.

Gates after the change: 9/9 in the new file, 4128 passed / 2 skipped across every test file referencing chat_utils, chat_handlers, _prepare_messages or slot_detail; isort, flake8 and mypy (986 files) clean.

@github-actions

github-actions Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed 3be1f8fb25058ca3e4ba6df0415a64c25f6d9aa3 — this comment is updated in place on each push.

Review details

The candidate concerns the resume path calling _collapse_wire_rows synchronously on the event loop. Falsifying it:

  • (a) input: a large in-flight window — plausible.
  • (b) call path: api_chat_slot_resume_collapse_wire_rows(existing.messages) on the loop — confirmed at line 4625.
  • (c) observable wrong outcome: fails. The window is capped (_MAX_SLOT_MESSAGES), the loop is a single O(window) string join, and the very next line (4628) already runs _prepare_messages — a heavier redaction pass — unoffloaded on the same loop. So the collapse is strictly cheaper than work already accepted here, and the no-blocking-call-on-event-loop rule targets blocking syscalls / large file IO, not bounded in-memory loops. The candidate's own analysis concedes all of this ("likely acceptable rather than a violation"). No observable failure; drops below 80.

Step 2 — verified the core correctness claim myself: _collapse_wire_rows is output-equivalent to _prepare_messages' chunk/done handling (run folding across an interleaved done, done-drop, no input mutation, order preserved), and both call sites now count collapsed units consistently for total/cursor/has_more. No groundable defect found.

No findings.

[OPUS-REVIEWED] 3be1f8f

Verdict parsed from the review's SHA-scoped output markers for commit 3be1f8fb25058ca3e4ba6df0415a64c25f6d9aa3.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 3be1f8fb25058ca3e4ba6df0415a64c25f6d9aa3: <one-sentence reason>

@github-actions

github-actions Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 3be1f8fb25058ca3e4ba6df0415a64c25f6d9aa3 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

I've read the contract, the intent file, the patch, and the surrounding code (_prepare_messages, _UNOWED_WINDOW_ROLES, the resume paths, and the other window-bounding sites). Final review follows.

First-Principles-Verdict: CONCERNS

The fix is aimed at the stated cause, but the chunk-fold semantics now live in two implementations that a test must hold in agreement.

What this change ships

Intent: a bounded read of a streaming session should return the last N messages, not a mid-sentence fragment — a FIX (#4306).

  1. Bounded slot-detail fetch during streaming returns N whole messages, not one fragment — justified
  2. Resume during streaming returns the in-flight reply whole, not its newest 200 deltas — justified
  3. total/next_before on both paths now count displayed messages, not stream progress — declared, justified
  4. done terminator rows no longer consume a resume-bound slot — declared, justified
  5. New private helper _collapse_wire_rows — 2 consumers counted (chat_handlers.py:1690, chat_handlers.py:4625); partial second spelling of _prepare_messages' fold

Watch

  • The fold+done-drop semantics are now spelled twice: _collapse_wire_rows and _prepare_messages' chunk_text accumulator (chat_utils.py:1708-1758). The PR pins their agreement with tests (test_a_done_row_does_not_split_a_run), which is the tell that both must be maintained in lockstep and will diverge silently if one changes.
  • The deeper cause — the live window stores one row per streamed delta — taxes other consumers too: grep != "chunk" in chat_runner.py counts 10 turn-end strip sites, and the description itself flags the unbounded done accumulation. A general fix is genuinely larger (the window doubles as the SSE replay buffer); accepted-and-deferred, but this fix sits at mechanism level relative to that cause, not root as §3 of the description claims.

Subtractions

  • Delete _prepare_messages' chunk_text accumulator and its two flush sites (chat_utils.py:1708-1720, 1755-1758): make it call _collapse_wire_rows as its first pass, then map the single surviving chunk row to a redacted streaming row — one spelling of the fold, and the output-equivalence test becomes unnecessary rather than load-bearing.

[FIRST-PRINCIPLES-REVIEWED] 3be1f8f

@chenmingwei23
chenmingwei23 force-pushed the fix/collapse-unflushed-tail-4306 branch from e4d18b9 to d818826 Compare August 18, 2026 13:35
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Disposition for the GPT finding on the done terminator -- fixed at d81882654.

Verified reachable before changing anything. done is appended at nine turn-end sites across chat_runner.py and chat_orchestrator.py, nothing anywhere removes it from the window, and it is never persisted -- so it accumulates one row per completed turn and arrives at the slice through the un-flushed tail. Counted as a row it consumes a slot and renders as nothing, so the smallest reproduction is a single settled turn: ten persisted messages plus one done in the window, ?limit=5, four messages back. That is the same class this PR exists to fix, one row at a time instead of hundreds.

Verifying it turned up something that makes the fix stronger than the suggestion. _prepare_messages skips done without flushing its chunk accumulator, so a terminator landing between two deltas does not split the message there. The fold treated it as any other non-chunk row and therefore did split -- the rendered text came out the same only because the render pass re-accumulated across the boundary. Dropping done in the reduction removes the slot consumption and makes the two agree by construction rather than by luck.

The function now reduces both wire-only roles that make a row count diverge from a message count, and is renamed _collapse_wire_rows to say so. Output equivalence still holds and is now the documented reason for each half: fold a chunk run because _prepare_messages reads nothing from those rows but content, drop done because it skips them without flushing.

Three tests, each mutation-verified against the mutant that owns it:

  • test_a_done_terminator_does_not_shrink_the_page -- the end-to-end case above; fails if done rows are kept
  • test_done_rows_drop
  • test_a_done_row_does_not_split_a_run -- pins the accumulator agreement, not just the row count

Gates after the change: 11/11 in the new file, 4130 passed / 2 skipped across every test file referencing chat_utils, chat_handlers, _prepare_messages or slot_detail; isort, flake8, mypy (986 files) clean. The PR body is updated -- it claimed the reduction "drops no done row", which this change makes false.

One neighbouring defect found while tracing this, not touched here: unflushed = mem_len - current_session_disk assumes the un-persisted rows are exactly the trailing N, but wire-only rows are interspersed through the window rather than sitting at its end, so slot.messages[-unflushed:] can take trailing rows that are already on disk. That is #4137's subject and it is a live PR, so it stays out of this one.

@chenmingwei23
chenmingwei23 force-pushed the fix/collapse-unflushed-tail-4306 branch 2 times, most recently from 047caed to 8ac751b Compare August 18, 2026 13:48
@chenmingwei23 chenmingwei23 changed the title fix(dashboard): bound slot detail by messages, not stream progress fix(dashboard): bound chat history by messages, not stream progress Aug 18, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Disposition for the First Principles CONCERNS -- the sibling is fixed at 8ac751b61, and the duplication note is acknowledged as accurate.

The sibling claim checks out exactly as described. api_chat_slot_resume's existing-slot branch took existing.messages[-200:] and only then folded, on the same live window. It is worse than the case this PR started from: the detail handler loses the messages around an in-flight reply, but a resume landing during a segment longer than 200 deltas fills the bound entirely and returns one streaming row holding only the newest 200 deltas, so the earlier part of the reply is dropped from the response. total and next_before counted deltas there too.

Fixed rather than deferred, for three reasons: it is the same root cause at a second call site rather than new scope, the reduction is one call, and the in-repo consumer tolerates the resulting total change -- resumeFromHistory (website/src/store/chatSlice.ts:1434) stores next_before opaquely, derives hasMore from cursor !== null && has_more, and does no arithmetic against total.

It also removes a unit mismatch rather than only a bound: that cursor is _disk_older_count + (total - len(recent)), and _disk_older_count counts persisted rows, which carry no wire-only role. Pairing it with a raw window length mixed persisted units with row units; after the reduction both terms are message counts. Note this is the narrow, transient-row half of the overstatement described on #4134 -- the un-persisted-but-real rows that #4134's accounting decision is actually about are untouched here, so that decision is not pre-empted.

Cost at that call site: the reduction is O(window) on the event loop, and the window is capped. The _prepare_messages redaction pass on the following line already runs there over 200 rows with a per-row regex battery, so it remains the larger cost either way -- this does not introduce a new class of loop work the way reducing the chained history would have.

Two tests, both mutation-verified against reverting the reduction at that call site: the in-flight reply is not truncated by the bound, and total plus the cursor count messages.

On the duplication note -- _collapse_wire_rows and _prepare_messages both encoding the run rule -- the read is right and so is the conclusion that it is divergence risk rather than removable duplication. Worth adding that the two rules are no longer merely parallel: _prepare_messages skips done without flushing its accumulator, so a terminator between two deltas does not split the message there. Dropping done in the reduction is what makes the two agree; treating it as an ordinary run-breaking row, which is where this started, split the run and matched only because the render pass re-accumulated across the boundary. The docstring now states that as the reason for each half, and a test pins the cross-terminator case specifically rather than just the row count.

The PR title, body and CHANGELOG entry are updated -- all three described a single endpoint.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Disposition for the blocking GPT finding at chat_handlers.py:1215 -- the defect is real and confirmed, it is pre-existing on main, and the prescribed fix does not close it. Escalating rather than patching, because the correct fix is already written in an open PR against the same function.

The defect reproduces, and it needs no streaming

Two completed turns are enough. Disk holds the four real messages; the window holds the same four plus one retained done per turn, so unflushed = 6 - 4 = 2 and slot.messages[-2:] is [a2, done] -- a suffix whose first row is already on disk.

GET /api/chat/slots/<slot>?limit=50
contents = ['u1', 'a1', 'u2', 'a2', 'a2']    <- a2 repeated

It is pre-existing, not introduced here

Substituting main's exact expression at that line -- list(all_msgs) + list(slot.messages[-unflushed:]) -- and re-running the same probe gives byte-identical output:

main's expression:   contents = ['u1', 'a1', 'u2', 'a2', 'a2']   total = 6
this branch:         contents = ['u1', 'a1', 'u2', 'a2', 'a2']   total = 5

The duplication is identical in both. What this branch changes is total alone, from a raw row count to a message count, which is the intended effect. So the row duplication is main's behaviour at that line and not a regression from the reduction; the finding is correctly identified but incorrectly attributed to this change.

The prescribed fix is insufficient

Fix: Slice the collapsed window from current_session_disk, not the raw window by unflushed.

That holds only while every non-persisted row is one the reduction removes. It is not: permission is never persisted and is rendered, so the reduction cannot drop it without deleting an approval card from the response. A permission row retained from an earlier turn therefore keeps the collapsed window longer than the persisted prefix, and the slice duplicates again:

window (collapsed):  ['user', 'assistant', 'permission', 'user', 'assistant']   len 5
current_session_disk: 4
window[4:]:          ['a2']                                                     <- still duplicated

queued and streaming are in the same category. So the index arithmetic cannot be repaired by choosing a different offset -- identifying the un-flushed suffix requires walking the window and matching rows against the disk read, which also has to account for the redaction-on-load transform that makes a persisted row differ from its window copy.

Where the correct fix already lives

That walk is exactly what #4137 implements, in this same function: _append_unflushed_tail plus _same_persisted_body, roughly 150 lines, handling both the load-time redaction transform and the coarse-clock ts collision that makes a foreign row look like the window's own. It is open, MERGEABLE, and awaiting review.

Reimplementing it here would mean two competing implementations of a subtle reconciliation in one function and a guaranteed conflict, so this is a sequencing decision for a maintainer rather than something to settle inside this PR. The options, as I see them:

  1. Review and land fix(dashboard): stop the un-flushed tail duplicating persisted rows #4137 first, then rebase this PR on it. The duplication leaves main, this line inherits the corrected tail, and the finding resolves without either PR growing.
  2. Stack this PR on fix(dashboard): stop the un-flushed tail duplicating persisted rows #4137's branch so the reviewer sees the corrected tail now, accepting that it cannot merge before fix(dashboard): stop the un-flushed tail duplicating persisted rows #4137.
  3. Narrow this PR to the resume path only. That branch reads existing.messages alone with no disk reconciliation, so it carries none of this defect -- but it also drops the fix for the originally reported symptom, leaving the bounded detail fetch as it is.

I recommend 1. I am not posting an override: the finding is real, and an override is for a false positive or an inapplicable one.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention merge conflict Branch has merge conflicts with its base — author must resolve before merge and removed readiness: checking Automated validation is still running labels Aug 18, 2026
@chenmingwei23
chenmingwei23 force-pushed the fix/collapse-unflushed-tail-4306 branch from 8ac751b to f3ef34e Compare August 19, 2026 03:01
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention merge conflict Branch has merge conflicts with its base — author must resolve before merge and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running merge conflict Branch has merge conflicts with its base — author must resolve before merge labels Aug 19, 2026
@chenmingwei23
chenmingwei23 force-pushed the fix/collapse-unflushed-tail-4306 branch from f3ef34e to 5ff3f4a Compare August 20, 2026 04:20
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Aug 20, 2026
@github-actions github-actions Bot added merge conflict Branch has merge conflicts with its base — author must resolve before merge and removed merge conflict Branch has merge conflicts with its base — author must resolve before merge labels Aug 20, 2026
`chunk` is a wire-only role appended once per streamed delta, so a reply
still being typed occupies hundreds of rows that render as one message.
Two endpoints bounded that live window by raw row count before reducing
it, so a bound meant to count messages counted stream progress instead.

GET /api/chat/slots/{slot} applied `limit` to raw rows, so a caller
asking for the last N messages of a running session spent its budget
inside the unfinished reply and received a mid-sentence fragment with
none of the conversation behind it. POST /api/chat/slots/{slot}/resume
filled its 200-row bound the same way, truncating the in-flight reply
itself to the newest 200 deltas.

Reduce the wire-only rows before bounding on both paths: consecutive
`chunk` runs fold to one row, and `done` terminators drop. A bound then
counts displayed messages, the in-flight text arrives whole, and `total`
and the paging cursor stop counting stream progress.

On the detail path the reduction runs in the same worker thread as
`_append_unflushed_tail` and covers the whole corpus, because that helper
places an owed row at the disk index it belongs to -- owed rows are not a
contiguous suffix, so a slice-scoped reduction would miss the interleaved
ones. `done` is already excluded there by `_UNOWED_WINDOW_ROLES`, so the
remaining work on that path is folding the chunk runs; the `done` drop
stays load-bearing on resume, which reads the window directly.

The reduction is output-equivalent to leaving those rows for the render
pass, so the rendered response shape is unchanged -- it only moves the
fold to before the bound, where it can decide what the bound counts. It
never mutates the live window: a folded row is a new dict.

On resume this also puts both cursor terms in message units, since
persisted rows carry no wire-only role. Persisted history never carries
these rows, so the scroll-back paging path is unaffected and
`next_before` keeps its meaning as a raw index.

Closes #4306
@chenmingwei23
chenmingwei23 force-pushed the fix/collapse-unflushed-tail-4306 branch from 5ff3f4a to 3be1f8f Compare August 20, 2026 08:34
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 20, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Duplication finding: resolved by #4137, verified on this head

The blocking finding (settled messages duplicated once done rows inflate the
raw tail) is fixed, and the fix is not mine: #4137 landed and deleted the
arithmetic the finding was about.

Recap of the disagreement, for the record. The finding was real, but it described
a defect that existed on main independently of this PR: the expression

unflushed = mem_len - max(0, disk_len - slot._disk_older_count)

counted the un-persisted done rows on the memory side only, so it over-counted
the tail and re-selected already-persisted rows. Running main's own expression
against a two-turn slot produced ['u1','a1','u2','a2','a2']. This branch
inherited that expression; it did not introduce it.

The remedy suggested here -- collapse the whole memory window first, then
subtract -- was insufficient: a retained permission row renders, so it cannot
be dropped, and it keeps the collapsed window longer than the persisted prefix,
so window[current_session_disk:] still yielded a duplicate. The correct fix
needed identity matching between window rows and disk rows, which is exactly what
#4137 built (_append_unflushed_tail + _same_persisted_body).

So rather than reimplement it here, this branch waited for #4137 and is now
rebased on it. The subtraction is gone entirely -- current_session_disk no
longer appears in the file -- and limit is applied to the corpus that helper
returns, after this PR's reduction.

Re-verified on the current head rather than assumed. Probe: two settled turns,
all four rows persisted, window holding those same four rows plus one done per
turn, ?limit=50:

response: ['u1', 'a1', 'u2', 'a2']      (no repeat)

Confirmed non-vacuously by asserting a wrong expectation and reading the actual
list back out of the failure, so the check is not passing because the branch
never ran.

Two things changed in this PR as a consequence of #4137's shape, both visible in
the diff:

  1. The detail-path reduction now covers the whole corpus and runs inside the same
    worker thread as _append_unflushed_tail, instead of being scoped to a
    trailing slice on the event loop. It has to: that helper places an owed row at
    the disk index it belongs to, so owed rows are not a contiguous suffix and a
    slice would miss the interleaved ones. Putting it in the thread also means the
    wider scope adds nothing to the loop.

  2. done is already excluded on that path by _UNOWED_WINDOW_ROLES, so the
    reduction's remaining job there is folding the chunk runs. The done drop
    stays load-bearing on the resume path, which reads the window directly. The
    test that had pinned the old tail-only scoping was rewritten to pin the new
    contract -- its stated rationale ("it runs on the event loop") is no longer
    true, so keeping it would have pinned a premise that had expired.

@github-actions github-actions Bot removed the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Aug 20, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

First Principles CONCERNS: one deferred with an issue, one wording concession

Subtraction: unify the fold into _prepare_messages -- accepted, deferred to #4665

The duplication is real and this PR introduces it. I checked the two spellings
against each other rather than taking the shape on faith: _prepare_messages'
accumulator relies on done hitting continue WITHOUT flushing chunk_text,
which is the same property _collapse_wire_rows depends on when it drops done
before folding. That is why they agree, and the suggested first-pass composition
would work -- after _collapse_wire_rows there is at most one chunk row per
run, so the accumulator's cross-row state stops being necessary and the flush
becomes a per-row emit.

Deferred rather than done here, for one reason: _prepare_messages is the render
path for every history response, not only the two endpoints this PR bounds. That
refactor is behavior-preserving by design, but its blast radius is every history
consumer, while this PR's is two bounding sites. Bundling them would put a
shared-render refactor and a bug fix behind one review.

Filed as #4665 with the concrete shape. On the "will diverge silently" risk: the
drift is pinned by a test rather than by vigilance --
test_a_done_row_does_not_split_a_run fails if either spelling stops treating
done as non-flushing. So the invariant has a guard while the unification waits.

"Mechanism level, not root" -- correct, and the description overclaims

Conceded. The deeper cause is that the live window stores one row per streamed
delta, which is also what drives the turn-end != "chunk" strip sites and the
unbounded per-turn done accumulation. This PR reorders the bound and the
reduction; it does not change that storage shape. So section 3's "chain from
symptom to root cause" framing claims more than the diff does -- the ordering is
the proximate cause, not the root.

I am recording that here instead of rewriting the body: a body edit re-triggers
the codex lane on this same SHA, and that verdict is non-deterministic, so
rewording a passage for accuracy can re-roll a clean verdict into a spurious red
for no merge benefit. Treat this comment as the correction to section 3.

Also noted, no action

The done accumulation itself (one dead row per settled turn, for the life of
the slot) is already called out in section 5 as follow-up material, and the
window's double duty as the SSE replay buffer is why I am not proposing a storage
change alongside a bounding fix.

@github-actions github-actions Bot added readiness: passed Eligible automated validation passed for the current revision and removed readiness: checking Automated validation is still running labels Aug 20, 2026
@bolichen97
bolichen97 enabled auto-merge (squash) August 20, 2026 09:04

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tier 1 auto-approve: fix (3 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean (CodeQL + Semgrep success, 0 alerts), security checklist all-NO, AI reviewers green. Category: fix with clear root cause — chat-history bounding was applied to raw wire rows (chunk/done), so a still-streaming reply filled the window and returned a mid-sentence fragment; the fix reduces wire-only rows before the existing 200-message bound (cap value unchanged) so the bound, total and cursor all count displayed messages.

@bolichen97
bolichen97 merged commit 0063c95 into main Aug 20, 2026
68 of 70 checks passed
@bolichen97
bolichen97 deleted the fix/collapse-unflushed-tail-4306 branch August 20, 2026 09:04
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Aug 20, 2026

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tier 1 auto-approve: fix (3 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: fix with clear root cause -- folds wire-only chunk/done rows before the limit slice so a bounded slot fetch counts displayed messages, not stream progress.

@iamwhatever iamwhatever left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tier 1 auto-approve: fix (3 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean (Semgrep + CodeQL both success, 0 alerts), security checklist all-NO, AI reviewers green. Category: bounds chat history by message count instead of stream progress in dashboard chat_handlers/chat_utils, clear root cause, test-covered.

encomjp pushed a commit to encomjp/kirocrew-customapi that referenced this pull request Aug 22, 2026
…irodotdev#4327)

`chunk` is a wire-only role appended once per streamed delta, so a reply
still being typed occupies hundreds of rows that render as one message.
Two endpoints bounded that live window by raw row count before reducing
it, so a bound meant to count messages counted stream progress instead.

GET /api/chat/slots/{slot} applied `limit` to raw rows, so a caller
asking for the last N messages of a running session spent its budget
inside the unfinished reply and received a mid-sentence fragment with
none of the conversation behind it. POST /api/chat/slots/{slot}/resume
filled its 200-row bound the same way, truncating the in-flight reply
itself to the newest 200 deltas.

Reduce the wire-only rows before bounding on both paths: consecutive
`chunk` runs fold to one row, and `done` terminators drop. A bound then
counts displayed messages, the in-flight text arrives whole, and `total`
and the paging cursor stop counting stream progress.

On the detail path the reduction runs in the same worker thread as
`_append_unflushed_tail` and covers the whole corpus, because that helper
places an owed row at the disk index it belongs to -- owed rows are not a
contiguous suffix, so a slice-scoped reduction would miss the interleaved
ones. `done` is already excluded there by `_UNOWED_WINDOW_ROLES`, so the
remaining work on that path is folding the chunk runs; the `done` drop
stays load-bearing on resume, which reads the window directly.

The reduction is output-equivalent to leaving those rows for the render
pass, so the rendered response shape is unchanged -- it only moves the
fold to before the bound, where it can decide what the bound counts. It
never mutates the live window: a folded row is a new dict.

On resume this also puts both cursor terms in message units, since
persisted rows carry no wire-only role. Persisted history never carries
these rows, so the scroll-back paging path is unaffected and
`next_before` keeps its meaning as a raw index.

Closes kirodotdev#4306
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Collapse chat rows before applying limit in the slot detail handler

3 participants