fix(dashboard): bound chat history by messages, not stream progress - #4327
Conversation
Design Review (Fable 5) — ✅ PASSDesign-level review of Design-Verdict: PASS Read-time normalization is the right shape: it fixes the bound, 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 Suggestions
[DESIGN-REVIEWED] 3be1f8f |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
3c18f4e to
e4d18b9
Compare
|
Disposition for the Design Review suggestion on 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: 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 Pinned by a new test, Gates after the change: 9/9 in the new file, 4128 passed / 2 skipped across every test file referencing |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsThe candidate concerns the resume path calling
Step 2 — verified the core correctness claim myself: No findings. [OPUS-REVIEWED] 3be1f8f Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
First Principles Review (Fable 5) — 🟡 CONCERNSPremise-level review of I've read the contract, the intent file, the patch, and the surrounding code ( 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 shipsIntent: a bounded read of a streaming session should return the last N messages, not a mid-sentence fragment — a FIX (#4306).
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] 3be1f8f |
e4d18b9 to
d818826
Compare
|
Disposition for the GPT finding on the Verified reachable before changing anything. Verifying it turned up something that makes the fix stronger than the suggestion. The function now reduces both wire-only roles that make a row count diverge from a message count, and is renamed Three tests, each mutation-verified against the mutant that owns it:
Gates after the change: 11/11 in the new file, 4130 passed / 2 skipped across every test file referencing One neighbouring defect found while tracing this, not touched here: |
047caed to
8ac751b
Compare
|
Disposition for the First Principles CONCERNS -- the sibling is fixed at The sibling claim checks out exactly as described. 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 It also removes a unit mismatch rather than only a bound: that cursor is Cost at that call site: the reduction is Two tests, both mutation-verified against reverting the reduction at that call site: the in-flight reply is not truncated by the bound, and On the duplication note -- The PR title, body and CHANGELOG entry are updated -- all three described a single endpoint. |
|
Disposition for the blocking GPT finding at The defect reproduces, and it needs no streamingTwo completed turns are enough. Disk holds the four real messages; the window holds the same four plus one retained It is pre-existing, not introduced hereSubstituting main's exact expression at that line -- The duplication is identical in both. What this branch changes is The prescribed fix is insufficient
That holds only while every non-persisted row is one the reduction removes. It is not:
Where the correct fix already livesThat walk is exactly what #4137 implements, in this same function: 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:
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. |
8ac751b to
f3ef34e
Compare
f3ef34e to
5ff3f4a
Compare
`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
5ff3f4a to
3be1f8f
Compare
Duplication finding: resolved by #4137, verified on this headThe blocking finding (settled messages duplicated once Recap of the disagreement, for the record. The finding was real, but it described counted the un-persisted The remedy suggested here -- collapse the whole memory window first, then So rather than reimplement it here, this branch waited for #4137 and is now Re-verified on the current head rather than assumed. Probe: two settled turns, Confirmed non-vacuously by asserting a wrong expectation and reading the actual Two things changed in this PR as a consequence of #4137's shape, both visible in
|
First Principles CONCERNS: one deferred with an issue, one wording concessionSubtraction: unify the fold into
|
bolichen97
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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.
…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
1. What is the problem?
chunkis a wire-only role appended once per streamed delta, so a reply stillbeing 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}appliedlimitto raw rows. A caller asking forthe 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}/resumefilled 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 thewhole 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
chunkruns fold to one row,doneterminators drop. A bound then countsdisplayed messages, the in-flight text arrives whole, and
totaland the pagingcursor stop counting stream progress.
Chain from symptom to root cause:
limitselected raw rows, and hundreds of them belong to one message._prepare_messages)ran after the bound, at render time.
reorders them rather than adding a second notion of size.
_collapse_wire_rows(new, inchat_utils.pybeside_prepare_messages) isoutput-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 helperplaces 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.
doneis already excluded there by_UNOWED_WINDOW_ROLES, so the remaining workon that path is folding the chunk runs. The
donedrop stays load-bearing onresume, 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_uncachedreturnsNonefor every wire-only role), so thescroll-back paging path is unaffected and
next_beforekeeps its meaning as araw 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 settledturns 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:
done-> the terminator and run-splitting tests failAlso re-ran the duplication probe from the review thread below (two settled
turns, all four rows persisted, window carrying those four plus one
doneperturn,
?limit=50): the response is exactly['u1','a1','u2','a2']with norepeat. That defect was pre-existing on
mainand is fixed by #4137, which hasnow 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_detailorresume(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.
donerows accumulate one per settled turn in the live window and nothing everremoves 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.