Repository navigation
fix(ui-web): keep a late delta out of the next turn's step - #1107
Conversation
|
Windows compatibility review — PR #1107 @ Scope: full base-to-head diff — Result: no Windows compatibility issue found (and none is expected from this change: it is platform-neutral TypeScript). What I checked: Verification (native Windows 11, isolated worktree Limitations: only the focused test file was run; the full |
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; suggestions only, and they are marked inline.
I reviewed the full base-to-head diff and the surrounding stop/reset/send, stage dispatch, transcript-folding, and subscription event-order paths. I also checked the relevant history, the Web UI vocabulary and architecture rules in AGENTS.md / CONTEXT-MAP.md / ui-web/CONTEXT.md, backward compatibility with the current target branch, and that the regression test exercises the production phase machine without weakening existing assertions. The change is platform-neutral and merges cleanly with current main.
One narrow follow-up is marked inline: the visual step boundary is fixed, but the late delta can remain in the runtime's preview buffer. That does not block this targeted transcript-step fix.
Verification:
- PR head:
npm test -- --run src/state/session/runtime.cancel.test.ts- 18 passed;npm run type-check- passed. - Clean synthetic merge with current
github/main: focused cancel suite - 32 passed; fullnpm test- 260 files / 4,471 tests passed;npm run type-check- passed.
62b3213 to
d22ba3b
Compare
|
Windows compatibility re-review — PR #1107 @ Scope: the branch was force-updated and rebased onto current main; the PR's own diff at this head is Result: no Windows-specific issue. This is in-memory ui-web session state — step sealing, the runtime's Verification: none executable on this host. The changed tests live in Coverage gaps: the UI tests this PR changes were not executed (see above), and I could not run |
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; suggestions only, and they are marked inline.
No new findings on this revision. I reviewed the new commit and the full target diff, including the stop/reset/send callers, phase and turnId lifecycle, cancel-refusal holding path, event ordering, transcript sealing/folding, relevant history, current-main merge compatibility, and the repository's AGENTS.md / CONTEXT-MAP.md / ui-web/CONTEXT.md constraints. The expanded tests preserve the production state machine and add coverage rather than weakening existing assertions.
The revision fixes the holding-state hole in the prior phase guard, covers both event openers and direct sends, and seals a late thought before releasing it. The previously recorded narrow rt.say metadata follow-up remains nonblocking; its thread is already resolved and I am not reopening or repeating it.
Verification at d22ba3b76afe:
npm test -- --run src/state/session/runtime.cancel.test.ts: 39 passed.npm run type-check: passed.npm test: 260 files / 4,478 tests passed.git diff --check github/main...HEAD: passed; merge tree with currentgithub/maincompleted cleanly.
|
I pushed a follow-up commit onto this branch as a maintainer edit, after rebasing it onto current Why: What the follow-up commit changes:
Please |
A stopped turn's token.delta can still be in flight: it calls ensureStep after the fold cleared rt.st, so it opens a step of its own, and neither turn.started nor message.start dropped it. The next turn's first delta was then written into that step, reading a-tailb1 where the issue expects a clean boundary. Both opening stages drop a step left open by a turn that is no longer streaming. The guard is the streaming phase, so a delegated turn opening inside a live parent keeps the parent's open step.
…first A refused cancel that holds the lane keeps the phase at streaming with no turn open, so the phase guard let the next turn write into the stopped turn's late step again. Key the drop on turnId instead, which only a fold clears: every opening (a send, message.start and turn.started) lets go of a step opened while no turn is open, and a frame that opens inside a running turn still keeps that turn's step. Seal the step before letting go of it, so a late thought no longer reads as live until the page reloads. Cover both opening frames after a late delta, both while a refusal holds the lane, the seal on all three openings, and an opening frame inside a turn still running.
A stopped turn's late token.delta also appends to the say buffer after the fold reset it. When the next turn opened through message.start or turn.started, its text was appended to that tail, so the session row's preview, touchSession and the completion notification read the stopped turn's tail followed by the new answer. dropStaleSteps now flushes the say buffer along with the step. Assert the buffer after the next turn's first delta, and the session row's preview once that turn completes.
d22ba3b to
2b89441
Compare
|
A second maintainer push, rebuilt on the same
The description is updated to match. As before, please |
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
The new commit fixes the only standing follow-up: the stale-step boundary now clears the stopped turn's say buffer without clearing output when an opening frame arrives inside a live turn. The tests cover both event openers, the held-lane path, live-turn preservation, and the rail preview consumed at completion. I found no new issue.
I reviewed the new delta and full target diff, callers and state lifecycle, relevant history, backward compatibility, test strength, merge compatibility, and the AGENTS.md / CONTEXT-MAP.md / ui-web/CONTEXT.md constraints. The prior thread has a closing reply and remains resolved.
Verification at 2b89441e43d0:
npm test -- --run src/state/session/runtime.cancel.test.ts: 39 passed.npm run type-check: passed.npm test: 260 files / 4,478 tests passed.git diff --check github/main...HEAD: passed.- Merge tree with current
github/main: clean.
dropStaleSteps clears rt.steps so the next turn's fold is handed only its own steps. A turn stopped before its first token has no prose of its own, so store.finishTurn's `said` fallback walks that list and would promote the stopped turn's late tail as this turn's answer. Assert the accumulator after both opening frames, and the steps the fold receives when the next turn ends without prose. Removing the line fails the four clean-step cases and the new fold case.
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
No new findings. The new commit is test-only and strengthens the regression contract: both opening frames must empty the runtime step list before new output, and a prose-free next turn must hand zero stale steps to the fold. The harness change observes the production call without altering its behavior or weakening existing assertions.
I reviewed the new delta, the resulting full target diff, the fold/stop callers exercised by the new case, test strength, merge compatibility, and the applicable repository and Web UI constraints. The previously closed thread remains resolved.
Verification at bf7ee31e2ae2:
npm test -- --run src/state/session/runtime.cancel.test.ts: 40 passed.npm run type-check: passed.git diff --check github/main...HEAD: passed.- Merge tree with current
github/main: clean.
LivXue
left a comment
There was a problem hiding this comment.
Approved at bf7ee31e2.
The review of this PR's final delta found one nonblocking test-coverage gap: the rt.steps clear in dropStaleSteps was not pinned by any test, and a next turn that ends without prose would otherwise hand the stopped turn's stale step to the fold, letting its said fallback promote the stopped turn's tail as that turn's answer. The gap is fixed in bf7ee31e2 (test-only) and pinned by the four clean-step assertions plus the no-prose fold case; removing the line now fails five cases.
The full ui-web diff was read for this approval; the PR touches no raven/agent/ file, so no dispatch-tool or DAG-skill surface is involved. The review at this head reports no new findings, and the focused and full UI suites pass locally.
Summary
After a stop, a
token.deltaof the stopped turn can still be in flight. It callsensureStep, findsrt.st === null(the fold cleared it) and opens a step of its own; the next turn opens throughturn.startedormessage.start, and neither dropped that step, so the next turn's first delta was written into the stopped turn's tail (one step readinga-tailb1).Every opening (a send,
message.startandturn.started) now drops a step that was opened while no turn is open, so the next turn starts on a clean boundary whichever event opens it. The guard isrt.turnId === nullrather than the phase: a refused cancel that holds the lane (busy: true) keeps the phase atstreamingwith no turn open, and that is the path where the stopped turn's late frames arrive. A frame that opens inside a running turn keeps that turn's open step. The dropped step is sealed first, so a late thought does not keep reading as live, and the stopped turn's late text is flushed from the say buffer, so the next turn's session-row preview and completion notification do not open with it.ui-web/src/state/session/runtime.ts:dropStaleStepsreturns while a turn is open (rt.turnIdset), otherwise seals the open step and clears it, the step accumulator and the say buffer;send()calls it beforereset().ui-web/src/state/session/stages.ts:message.startandturn.startedcall it before their bookkeeping.ui-web/src/state/session/runtime.cancel.test.ts: the harness records the text each step receives, whether it was sealed, and how many steps each fold is handed, and provides theadvanceTurnfake. Tests drive the sequence from the issue with both opening frames, the same sequence while a refusal holds the lane, the seal of a late thought on all three openings, and an opening frame inside a turn still running; the late-delta and held-lane tests also assert the step accumulator and the say buffer, and the session row's preview once the next turn completes; one case ends the next turn before its first token and asserts the fold is handed no stale step.Type
Verification
Run on the rebased head (current
mainplus this branch):vitest run src/state/session-> 13 files, 287 passed.Full
ui-webvitest -> 260 files, 4479 tests passed.Red checks, one at a time: restoring the
turn.phase() !== 'streaming'guard fails the two held-lane tests; removing the seal fails the three seal tests; removing thesend()call fails the send seal test; removing themessage.startorturn.startedcall fails that opener's three tests; dropping theturnIdcheck fails the running-turn test; dropping the say-buffer flush fails the four late-delta and held-lane tests withexpected 'a-tailb1' to be 'b1'; removing thert.stepsclear fails the four clean-step cases and the no-prose fold case.tsc --noEmit -p tsconfig.json-> clean.eslinton the three changed files -> clean.make check-commits check-large-files check-source-language-> pass.Not run: a live browser session.
Relevant tests pass locally
Relevant lint / type checks pass locally
User-facing docs or screenshots are updated when needed
Risk
User-visible behaviour: a stopped turn's late tail no longer merges into the next turn's text, nor into its session-row preview and completion notification. The tail still opens its own step, so nothing that already streamed on the stage disappears.
A frame that opens while a turn is open on this page (its
turnIdset) keeps that turn's open step; a lane held by a refused cancel has no turn open and drops it.Rollback: revert the squash commit; it is one helper plus three call sites.
Security impact considered
Backward compatibility considered
Rollback path is clear for risky changes
Related Issues
Fixes #1061