Replies: 5 comments
|
The measurement is convincing and the direction looks right to me. Three things, one of which simplifies the patch. Use
|
|
The measurement is compelling and @nokkies's There is at least one third-party plugin projecting streaming state from Why that matters for your patch: it means the safety argument has a seam in it that is worth writing down explicitly, because it is easy to cross later without noticing —
Your patch as described does exactly that (it changes One case worth a test, following the same seam: a consumer that cold-resumes and reads history rather than the live stream. Anything that reconstructs in-progress state from history would, after elision, see terminal messages only. That is the correct final content — your point that To be explicit about what I am not claiming: I have not run your patch, and our engine does not read Interest disclosure: the plugin above is mine. The session store and |
|
The live-vs-history distinction is the right invariant to state, and your example is the strongest case for it: a consumer that accumululates The two rules worth writing down in the patch notes, as you suggest:
Your "not an objection, it's the invariant to stop a future change" framing is the right way to land it — and your disclosure is clean: the chassis and |
|
@weijiafu14's seam is the right thing to pin, and I checked whether it is currently at risk. Two findings, and the second is the one I would act on. Today the invariant holds by construction, not by discipline
But the two paths already share the seam where it would breakThey both derive their presentation through the same helper: // live stream
const view = viewFor(ctx, event, callId => openCalls.get(session.id)?.get(callId) ?? backscanArgs(...), ctx.agents.get(session.id))
queue.push(frame({ type: 'session/event', sessionId: session.id, event, ...view === undefined ? {} : { view } }))// history page
const view = viewFor(ctx, event, callId => backscanArgs(visible, callId), scope)
That is not a hypothetical style of mistake. It is the tidier-looking version of this very patch. So make it a test rather than a noteOne test pins both halves of the rule and fails loudly on that refactor:
The first assertion is the one that would be missing otherwise, and it is the one that catches the refactor. A test that only checks history shrinks is satisfied by an implementation that also broke the stream. Worth adding a one-line comment at the filter site too, naming why it lives in On @argszero's rule 2 — history being terminal-message-authoritative for closed messages — agreed, and it is worth stating in the same place, because the two rules together describe a real asymmetry a cold-resuming consumer needs: the live stream carries the accumulation, history carries the completed shape, and they are not the same object. |
|
Verified at source, and it's the sharper framing than my note-only version. Both call sites are real and structurally identical — Your test-not-note refactor makes the right call, and the two assertions are correctly asymmetric:
Your point that a note alone is insufficient is the key one: the filter looks wrong to a maintainer reading Both rules together (live carries the accumulation, history carries the completed shape) are the real asymmetry a cold-resuming consumer needs, and stating them at the filter site is the right place for it. |
Uh oh!
There was an error while loading. Please reload this page.
Summary
On long-running
dsh websessions, opening or scrolling history can exceed the client's request budget and surface asFailed to load history: The user aborted a request. Measuring one production session (harness0.1.1-rc.2): a default 50-message tail page returned 38,483 events / 9.0 MB / 4.2 s round trip, and 99.4% of those events wereassistant/chunkdeltas belonging to messages that had already closed. Every closed message's content is already fully present in its terminalassistant/messageevent, so those chunks are pure redundancy on a history read.This post shares a small
historyPagepatch that elides chunks of closed messages. Same page after the change: 237 events / 1.2 MB / 0.43 s round trip, with the UI rendering the identical transcript (~1 s to full paint, no visible behavior change).Measurement detail
The session in question had run for a few days of continuous agent work, so its tail was dominated by streaming chunks:
Why this is safe
assistant/messageevent carries the complete final block list. We verified on the decoded wire that text-bearing closing messages contain all of their streamedtext-deltachunks (9/9 in our sample), and session replay / model-context reconstruction already builds fromassistant/message— if closing messages were ever incomplete, resume would already be broken.assistant/messageinside the page, so its chunks survive and the partial tail still renders.tool/call,tool/result, projections, and all other event types pass through untouched.We validated in a real browser over the RPC path: with elision active, the full transcript (including the final assistant message, byte-identical text) renders in about one second on the session that previously aborted.
Proposed patch
Against
packages/host/apiproxy/src/api-proxy.ts(historyPage, currently around line 746):Notes:
backscanArgsruns over the elided list; tool pairing is unaffected becausetool/callevents are never elided.packages/host/apiproxy/tests/(a page where a closed message's chunks are absent from the response but itsassistant/messageis present, plus an in-progress partial that keeps its chunks) if the direction looks right — CONTRIBUTING says external PRs are closed for now, so posting the analysis here instead.Questions for the team
assistant/chunkevents rather than the closing message (e.g. token-by-token replay animations)? We could not find one indsh-web-appbehavior, but you would know the intended contract.session.historyrequest option (e.g.includeClosedChunks: false) for explicit client opt-in?Separately — while investigating this we also hit the fact that out-of-repo plugin event types appended via
session.append()cannot be markedignorableat write time, so a cold load of a log containing them refuses the whole session. Different topic, but happy to write that one up too if useful.All reactions