-
Notifications
You must be signed in to change notification settings - Fork 3k
feat(web-shell): run read-only info commands immediately mid-turn #8496
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
508e2b0
8672498
57eeeda
54e8676
7206e8e
8782aad
548879c
b1a935e
eb6f075
84a7e91
acaa555
f7d04f6
15f3e00
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -38,3 +38,115 @@ describe('daemon transcript rewind', () => { | |
| expect(state.activeAssistantBlockId).toBeUndefined(); | ||
| }); | ||
| }); | ||
|
|
||
| describe('status event while an assistant block is streaming', () => { | ||
| it('finalizes the active assistant block by default', () => { | ||
| const state = reduceDaemonTranscriptEvents( | ||
| createDaemonTranscriptState({ now: 1 }), | ||
| [ | ||
| { type: 'user.text.delta', text: 'question' }, | ||
| { type: 'assistant.text.delta', text: 'answering' }, | ||
| { type: 'status', text: 'mid-stream status' }, | ||
| { type: 'assistant.text.delta', text: ' more' }, | ||
| { type: 'assistant.done' }, | ||
| ], | ||
| { now: 1 }, | ||
| ); | ||
|
|
||
| expect(state.blocks.map((block) => block.kind)).toEqual([ | ||
| 'user', | ||
| 'assistant', | ||
| 'status', | ||
| 'assistant', | ||
| ]); | ||
| }); | ||
|
|
||
| it('keeps the assistant block active when clearActiveText is false', () => { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] R4-3: The it('keeps the thought block active when clearActiveText is false', () => {
// user.text.delta → thought.text.delta → { type: 'status', clearActiveText: false }
// → thought.text.delta → assistant.done
// assert: a single thought block with merged text, still active after the status event
});中文说明
— qwen3.8-max via Qwen Code /review (v0.21.5) |
||
| const state = reduceDaemonTranscriptEvents( | ||
| createDaemonTranscriptState({ now: 1 }), | ||
| [ | ||
| { type: 'user.text.delta', text: 'question' }, | ||
| { type: 'assistant.text.delta', text: 'answering' }, | ||
| { type: 'status', text: 'mid-stream status', clearActiveText: false }, | ||
| { type: 'assistant.text.delta', text: ' more' }, | ||
| { | ||
| type: 'assistant.usage', | ||
| usage: { inputTokens: 3, outputTokens: 5 }, | ||
| }, | ||
| { type: 'assistant.done' }, | ||
| ], | ||
| { now: 1 }, | ||
| ); | ||
|
|
||
| expect(state.blocks.map((block) => block.kind)).toEqual([ | ||
| 'user', | ||
| 'assistant', | ||
| 'status', | ||
| ]); | ||
| const assistant = state.blocks[1]; | ||
| if (assistant.kind !== 'assistant') throw new Error('expected assistant'); | ||
| expect(assistant.text).toBe('answering more'); | ||
| expect(assistant.usage).toEqual({ | ||
| inputTokens: 3, | ||
| outputTokens: 5, | ||
| cachedTokens: 0, | ||
| }); | ||
| }); | ||
|
|
||
| it('resets the active user block even when clearActiveText is false', () => { | ||
| let state = reduceDaemonTranscriptEvents( | ||
| createDaemonTranscriptState({ now: 1 }), | ||
| [{ type: 'user.text.delta', text: '/stats' }], | ||
| { now: 1 }, | ||
| ); | ||
| state = reduceDaemonTranscriptEvents( | ||
| state, | ||
| [{ type: 'status', text: 'stats output', clearActiveText: false }], | ||
| { now: 1 }, | ||
| ); | ||
|
|
||
| expect(state.activeUserBlockId).toBeUndefined(); | ||
|
|
||
| // A peer client's prompt echo must open its own user block instead of | ||
| // merging into the local command echo. | ||
| state = reduceDaemonTranscriptEvents( | ||
| state, | ||
| [{ type: 'user.text.delta', text: 'fix the bug' }], | ||
| { now: 1 }, | ||
| ); | ||
|
|
||
| expect(state.blocks.map((block) => block.kind)).toEqual([ | ||
| 'user', | ||
| 'status', | ||
| 'user', | ||
| ]); | ||
| expect( | ||
| state.blocks.map((block) => ('text' in block ? block.text : '')), | ||
| ).toEqual(['/stats', 'stats output', 'fix the bug']); | ||
| }); | ||
| }); | ||
|
|
||
| describe('status event while a thought block is streaming', () => { | ||
| it('keeps the thought block active when clearActiveText is false', () => { | ||
| const state = reduceDaemonTranscriptEvents( | ||
| createDaemonTranscriptState({ now: 1 }), | ||
| [ | ||
| { type: 'user.text.delta', text: 'question' }, | ||
| { type: 'thought.text.delta', text: 'thinking' }, | ||
| { type: 'status', text: 'mid-stream status', clearActiveText: false }, | ||
| { type: 'thought.text.delta', text: ' more' }, | ||
| { type: 'assistant.done' }, | ||
| ], | ||
| { now: 1 }, | ||
| ); | ||
|
|
||
| expect(state.blocks.map((block) => block.kind)).toEqual([ | ||
| 'user', | ||
| 'thought', | ||
| 'status', | ||
| ]); | ||
| const thought = state.blocks[1]; | ||
| if (thought.kind !== 'thought') throw new Error('expected thought'); | ||
| expect(thought.text).toBe('thinking more'); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Critical] The
clearActiveText: falseopt-out skips the entireclearActiveText(state)call — includingstate.activeUserBlockId = undefined. All three new dispatch sites pass the flag unconditionally, also when idle. Pre-PR, an idle status dispatch resetactiveUserBlockId; post-PR, running/stats,/aboutor/context(or clicking the status-bar context indicator) while idle leaves the local command echo block as the active user block indefinitely.Failure scenario: Web Shell client B is idle; its user runs
/stats→ echo block E becomesactiveUserBlockId; the result dispatch leaves the pointer at E. A peer client (TUI or a second web tab on the same daemon session) then submits a prompt: the bridge echo arrives as a mergeableuser.text.delta(nosourceRecordIds, noqwenDiscreteMessage),canMergeTextDeltapasses, and the peer's prompt text is appended onto E — rendering/stats<peer prompt>in one user block. SinceapplyTurnCollapsebounds turns by user messages, the peer's entire turn (assistant text, tool steps, token usage) groups under the/statsecho's turn, corrupting turn boundaries and per-turn metrics. Verified by a reducer probe at the reviewed commit: the PR arm merged into one user block (/statsfix the bug) where the pre-PR control arm produced separate user blocks.Suggested fix — probe-verified (flips the repro back to separate blocks while keeping this PR's reducer tests green): in
appendStatusBlock, keep the assistant/thought block but drop the user pointer on the opt-out path:(Alternative: pass
clearActiveText: falsefrom App.tsx only while streaming — idle dispatches have no streaming block to protect.)中文说明
clearActiveText: false选项跳过了整个clearActiveText(state)调用——包括state.activeUserBlockId = undefined。三处新的 dispatch 都无条件传入该标志,空闲时也是如此。本 PR 之前,空闲时的 status dispatch 会重置activeUserBlockId;现在,空闲时运行/stats、/about或/context(或点击状态栏的 context 指示器)会让本地命令回显块无限期地保持为活跃用户块。失败场景:Web Shell 客户端 B 空闲时运行
/stats→ 回显块 E 成为activeUserBlockId;结果 dispatch 使指针一直停留在 E。此时同一 daemon 会话上的对端客户端(TUI 或第二个网页标签页)提交提示词:桥的回显以可合并的user.text.delta到达(无sourceRecordIds、无qwenDiscreteMessage),canMergeTextDelta通过,对端的提示词文本被追加到 E 上——一个用户块渲染出/stats<对端提示词>。由于applyTurnCollapse以用户消息为回合边界,对端的整个回合(assistant 文本、工具步骤、token 用量)都会归入/stats回显所在的回合,破坏回合边界与逐回合统计。已在被审提交上用 reducer 探针验证:PR 分支合并为一个用户块(/statsfix the bug),而 PR 前的对照组产生独立的用户块。建议修复(已用探针验证——复现恢复为独立块,且本 PR 的 reducer 测试仍全绿):在
appendStatusBlock中保留 assistant/thought 块,但在 opt-out 路径上清掉用户指针:(备选方案:仅在流式时才从 App.tsx 传
clearActiveText: false——空闲 dispatch 没有需要保护的流式块。)— qwen3.8-max via Qwen Code /review (v0.21.5)