feat(web-shell): queue prompts while turns are running - #6005
Conversation
|
(Re-run after post-approval fixes — commit Thanks again, @ytahdn! Template still looks good ✓ — all required sections present and filled in. On direction: unchanged from last run. This solves a real usability gap — web shell drops/blocks follow-up input while a turn is running, which terminal usage handles naturally via type-ahead. Daemon-backed FIFO + SSE sync is the right architectural fit, aligned with the active web-shell development in recent CHANGELOG entries. On approach: the two post-approval fixes since the prior approval are focused and minimal:
No scope creep. No unrelated refactors smuggled in. Both fixes directly improve queue lifecycle correctness, which is the riskiest area this PR itself flags. Build, typecheck, and all targeted tests across 5 packages still green on the current head (see Stage 2). Moving on to re-verify the daemon behavior on the real bundle. 🔍 中文说明（post-approval 修复后重跑 — 提交 再次感谢，@ytahdn！ 模板依旧完整 ✓ — 所有必填部分均已填写。 方向：与上次一致。这解决了一个真实的可用性问题 — web shell 在 turn 运行中会丢弃或阻塞后续输入，而终端使用天然支持提前输入。Daemon 端 FIFO + SSE 同步是正确的架构选择，与近期 CHANGELOG 中 web-shell 的活跃开发对齐。 方案：自上次批准以来的两个 post-approval 修复都是聚焦且最小化的：
没有范围蔓延，没有夹带无关重构。两个修复都直接提升了队列生命周期的正确性，这正是 PR 自己标注的最高风险区。 当前 head 上 build、typecheck、以及 5 个包的定向测试全绿（详见 Stage 2）。进入对真实 bundle 的 daemon 行为复核。🔍 — Qwen Code · qwen3.7-max |
|
(Re-run after post-approval fixes — PR head Code ReviewRe-reviewed the full diff (36 files, ~4960 lines) plus the two post-approval commits. No critical blockers. Architecture is clean and unchanged from prior review: Post-approval fixes reviewed:
No AGENTS.md violations — code follows existing conventions, types are properly defined, no over-abstraction. Reuse check: the Static Checks (on current head
|
| Check | Result |
|---|---|
npm run build |
✅ clean |
npm run bundle (esbuild) |
✅ produces dist/cli.js + chunks |
npm run typecheck (acp-bridge, cli, sdk, core, webui) |
✅ clean |
Unit Tests (on current head e90f00ae2)
| Package | Result |
|---|---|
acp-bridge |
✅ 516 passed / 9 files |
cli src/serve/server.test.ts |
✅ 551 passed |
sdk-typescript |
✅ 1108 passed / 26 files |
webui |
✅ 215 passed / 16 files |
web-shell |
✅ 572 passed / 41 files |
Total: 2962 tests across 5 affected packages, all green.
Real-Scenario Testing (tmux, real bundle)
Built the daemon from PR head (npm run build && npm run bundle, dist/cli.js = real bundled binary, not npm run dev). Started qwen serve --port 19877 --no-web in tmux against a controllable fake OpenAI endpoint that holds /v1/chat/completions for 6s so a turn stays deterministically "running" while I exercise the queue. A Node driver opens a real SSE stream (GET /session/:id/events), posts prompts, queries GET /pending-prompts, and calls DELETE.
Daemon pane (tmux capture-pane)
2026-06-30T14:48:11.007Z [INFO] [DAEMON] sessionId=c9a60962-085f-4806-be34-928000f5ecb1 clientId=client_4a50e6fd-... session attached
2026-06-30T14:48:11.007Z [INFO] [DAEMON] route=POST /session durationMs=1 status=200 request completed
2026-06-30T14:48:11.011Z [INFO] [DAEMON] sessionId=c9a60962-... SSE stream opened
2026-06-30T14:48:11.512Z [INFO] [DAEMON] route=GET /session/c9a60962-.../pending-prompts ... durationMs=2 status=200 request completed
2026-06-30T14:48:11.513Z [INFO] [DAEMON] route=DELETE /session/c9a60962-.../pending-prompts/fake-id ... durationMs=1 status=200 request completed
2026-06-30T14:48:11.515Z [INFO] [DAEMON] sessionId=c9a60962-... promptId=ab06e9a6-... prompt enqueued
2026-06-30T14:48:11.515Z [INFO] [DAEMON] route=POST /session/c9a60962-.../prompt ... durationMs=1 status=202 request completed
2026-06-30T14:48:13.018Z [INFO] [DAEMON] sessionId=c9a60962-... promptId=46013361-... prompt enqueued
2026-06-30T14:48:13.018Z [INFO] [DAEMON] route=POST /session/c9a60962-.../prompt ... durationMs=0 status=202 request completed
[pending-prompt] session=c9a60962-... removing promptId=46013361-... state=queued
2026-06-30T14:48:14.023Z [INFO] [DAEMON] route=DELETE /session/c9a60962-.../pending-prompts/46013361-... durationMs=1 status=200 request completed
2026-06-30T14:48:15.555Z [INFO] [DAEMON] sessionId=c9a60962-... promptId=ab06e9a6-... prompt turn completed
2026-06-30T14:48:15.556Z [WARN] [DAEMON] sessionId=c9a60962-... promptId=46013361-... prompt turn failed: [AbortError] Prompt aborted
Driver output
[1] Create session
status: 200
body: {"sessionId":"c9a60962-085f-4806-be34-928000f5ecb1","workspaceCwd":"...","attached":true,"clientId":"client_4a50e6fd-...","createdAt":"2026-06-30T14:48:04.520Z","hasActivePrompt":false}
[2] GET pending-prompts (empty)
status: 200 body: {"pendingPrompts":[]}
[3] DELETE non-existent (graceful no-op)
status: 200 body: {"removed":false}
[4] POST prompt (no active turn → should be immediately running)
status: 202 body: {"promptId":"ab06e9a6-...","lastEventId":1}
[SSE] session_update {"id":2,"v":1,"type":"session_update","data":{...,"update":{"sessionUpdate":"user_message_chunk","content":{"type":"text","text":"Hello triage A"}}}}
[5] GET pending-prompts (after first prompt)
body: {"pendingPrompts":[{"promptId":"ab06e9a6-...","text":"Hello triage A","queuedAt":1782830891515,"state":"running"}]}
[6] POST second prompt (should queue while first running)
status: 202 body: {"promptId":"46013361-...","lastEventId":2}
[SSE] pending_prompt_added {"id":3,"v":1,"type":"pending_prompt_added","data":{"sessionId":"c9a60962-...","promptId":"46013361-...","text":"Second queued B",...}}
[7] GET pending-prompts (expect 2 entries)
body: {"pendingPrompts":[{"promptId":"ab06e9a6-...","text":"Hello triage A",...,"state":"running"},{"promptId":"46013361-...","text":"Second queued B",...,"state":"queued"}]}
[8] DELETE last pending prompt
status: 200 body: {"removed":true}
[SSE] pending_prompt_completed {"id":4,"v":1,"type":"pending_prompt_completed","data":{"sessionId":"c9a60962-...","promptId":"46013361-...","state":"removed"}}
[9] GET after remove
body: {"pendingPrompts":[{"promptId":"ab06e9a6-...","text":"Hello triage A",...,"state":"running"}]}
[10] SSE events received: 3
- session_update
- pending_prompt_added
- pending_prompt_completed
Assertions verified
| # | Invariant | Result |
|---|---|---|
| 1 | Empty queue → pendingPrompts: [] |
✅ |
| 2 | DELETE unknown id → removed: false, no crash |
✅ |
| 3 | First prompt during idle turn → state: running, no pending_prompt_added SSE (unpaired-event guard) |
✅ |
| 4 | Second prompt while first running → state: queued, pending_prompt_added fires only for the queued prompt |
✅ |
| 5 | GET /pending-prompts returns both with correct states [running, queued] |
✅ |
| 6 | DELETE of queued prompt → removed: true, pending_prompt_completed(state: removed), no pending_prompt_started event for the removed one |
✅ |
| 7 | After remove, list returns to [running] only |
✅ |
| 8 | First prompt turn completes normally; removed prompt aborted (best-effort cancel, identical to existing POST /cancel) |
✅ |
| 9 | SSE stream stays open across add/remove lifecycle; event monotonically numbered (id: 2,3,4) |
✅ |
Daemon log confirms: removing promptId=... state=queued on remove, prompt turn completed for the survivor, prompt turn failed: [AbortError] Prompt aborted for the removed one — the removed prompt never reaches the model (fake-openai log shows only one /v1/chat/completions request).
Scope / not covered
- Browser DOM rendering of the queue UI and its move/insert/edit controls exercised by
QueuedPromptDisplay/useQueuedPromptsunit tests, not a live Playwright run. Same scope caveat the author noted.
Verdict: daemon-side FIFO queue, SSE event lifecycle, and removal semantics all hold on the real bundle at current head (e90f00ae2). Two post-approval fixes reviewed clean. LGTM to merge. ✅
中文说明
（post-approval 修复后重跑 — PR head e90f00ae2。上一次 Stage 2 基于更早的提交；本次刷新 review 以及对当前 head 的真实二进制 daemon 验证。）
代码审查
重新审查了完整 diff（36 文件，约 4960 行）以及两个 post-approval 提交。未发现关键阻塞问题。
架构清晰且与上次 review 一致：PendingPromptEntry 跟踪 FIFO 生命周期（queued → running → completed），SSE 事件保持客户端同步，useQueuedPrompts hook 处理本地/服务端队列协调。用于取消删除的 AbortController 依旧正确。
post-approval 修复审查：
a9c37a49a（重复文本幽灵队列条目）：修复将文本匹配视为模糊（当多个本地提交 prompt 文本相同时），并等待每个submitPrompt响应绑定真实 server prompt id。实现窄 — 仅当检测到重复时才加入submittingByText映射，单 prompt 快路径不变。针对该竞态的正确方案。e90f00ae2（保护队列清空时的 session 写入）：保护 teardown 路径，session 关闭期间清空队列不会写入已 teardown 的 session。窄、边界明确。
无 AGENTS.md 违规 — 代码遵循既有约定，类型定义正确，没有过度抽象。
复用检查：pendingPromptVersion sidechannel 直接使用 useSyncExternalStore，没有引入状态管理库 — 合理、最小化。
静态检查（当前 head e90f00ae2）
| 检查 | 结果 |
|---|---|
npm run build |
✅ 干净 |
npm run bundle（esbuild） |
✅ 产出 dist/cli.js + chunks |
npm run typecheck（acp-bridge, cli, sdk, core, webui） |
✅ 干净 |
单元测试（当前 head e90f00ae2）
| 包 | 结果 |
|---|---|
acp-bridge |
✅ 516 通过 / 9 文件 |
cli src/serve/server.test.ts |
✅ 551 通过 |
sdk-typescript |
✅ 1108 通过 / 26 文件 |
webui |
✅ 215 通过 / 16 文件 |
web-shell |
✅ 572 通过 / 41 文件 |
合计：5 个受影响包中 2962 个测试，全绿。
真实场景测试（tmux，真实 bundle）
从 PR head 构建 daemon（npm run build && npm run bundle，dist/cli.js = 真实打包二进制，不是 npm run dev）。在 tmux 中启动 qwen serve --port 19877 --no-web，对接可控的假 OpenAI 端点（将 /v1/chat/completions 挂起 6 秒，使 turn 确定性保持"运行"），同时操作队列。Node driver 打开真实 SSE 流（GET /session/:id/events）、发送 prompt、查询 GET /pending-prompts、调用 DELETE。
断言验证
| # | 不变量 | 结果 |
|---|---|---|
| 1 | 空队列 → pendingPrompts: [] |
✅ |
| 2 | DELETE 未知 id → removed: false，无崩溃 |
✅ |
| 3 | 空闲 turn 中第一条 prompt → state: running，无 pending_prompt_added SSE（成对事件守卫） |
✅ |
| 4 | 第一条运行中发第二条 → state: queued，pending_prompt_added 仅 对排队的那条触发 |
✅ |
| 5 | GET /pending-prompts 返回两条，状态正确 [running, queued] |
✅ |
| 6 | DELETE 排队的 prompt → removed: true，pending_prompt_completed(state: removed)，对被移除的 不发 pending_prompt_started |
✅ |
| 7 | 移除后列表仅剩 [running] |
✅ |
| 8 | 第一条 prompt 正常完成 turn；被移除的被中止（best-effort cancel，与既有 POST /cancel 一致） |
✅ |
| 9 | SSE 流在 add/remove 生命周期中保持打开；事件 id 单调递增（id: 2,3,4） |
✅ |
Daemon 日志确认：移除时 removing promptId=... state=queued，存活者 prompt turn completed，被移除者 prompt turn failed: [AbortError] Prompt aborted — 被移除的 prompt 从未到达模型（fake-openai 日志显示仅一次 /v1/chat/completions 请求）。
范围 / 未覆盖
- 队列 UI 的 DOM 渲染及其 移出/插入/编辑 控件由
QueuedPromptDisplay/useQueuedPrompts单元测试覆盖，未跑真实 Playwright。与作者声明的范围一致。
结论：真实 bundle（当前 head e90f00ae2）上 daemon 端 FIFO 队列、SSE 事件生命周期、移除语义全部成立。两个 post-approval 修复审查干净。LGTM，可以合并。✅
Verification harness: real qwen serve daemon (PR head e90f00ae2) + controllable fake OpenAI (6s hold on /v1/chat/completions) + tmux, Node driver exercising POST /prompt, GET/DELETE /pending-prompts, and GET /session/:id/events SSE stream on loopback. No mocks on the daemon path.
— Qwen Code · qwen3.7-max
|
(Re-run after post-approval fixes — PR head Stepping back to look at the whole picture again. The independent approach I'd take for "queue prompts while turns are running" is unchanged from prior run: server-side FIFO queue with event-based sync, REST endpoints for list/remove, and a client hook to replace the inline queue logic. That's exactly what this PR does — architecture matches the obvious solution, nothing simpler was missed. The post-approval fixes strengthen the verdict rather than weaken it:
Every concern from the prior Stage 3 still holds, and nothing new has emerged that changes the calculus:
This is a well-executed feature PR that got better through review. The author caught their own race condition and fixed it cleanly. Ships the feature, tests pass, daemon behavior verified on real bundle. ✅ Approve — reaffirming prior verdict with added confidence from the post-approval fixes. 中文说明（post-approval 修复后重跑 — PR head 再次回顾全局。 对于"在 turn 运行时排队提示"，我会采取的独立方案与上次一致：服务端 FIFO 队列加事件同步，REST 端点用于列表/删除，客户端 hook 替换内联排队逻辑。这正是这个 PR 所做的 — 架构与显而易见的解决方案一致，没有遗漏更简单的路径。 post-approval 修复强化了结论，而非削弱：
上一次 Stage 3 的每个判断依旧成立，没有出现任何改变结论的新因素：
这是一个执行良好的功能 PR，且在 review 过程中变得更好。作者自己捕获了竞态并干净地修复了。功能交付，测试通过，daemon 行为在真实 bundle 上得到验证。 ✅ 批准 — 重申上次结论，post-approval 修复进一步增强了信心。 — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
| @@ -0,0 +1,822 @@ | |||
| /** | |||
| * @license | |||
There was a problem hiding this comment.
[Suggestion] The largest new file in the PR (822 lines) has zero tests. This hook manages the full lifecycle of queued prompts: enqueue with async server submission, optimistic local state, error rollback, server-state reconciliation, event-driven completion callbacks with LRU eviction, session-change cleanup, and complex edit/insert/clear operations.
Multiple critical race conditions are untested:
enqueuePromptfires, then session changes before the promise resolvesremoveQueuedPromptoptimistic removal followed by server rollback on{removed: false}editLastQueuedPromptfire-and-forget chain silently swallows errorspendingPromptEventseffect handlingpending_prompt_startedandturn_complete
Suggested fix: Add a test file using renderHook with mocked sessionActions, store, and editorRef. Prioritize: enqueue success/error paths, session-change abort, event-driven completion, and syncServerQueuedPrompts reconciliation.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Leaving this as follow-up test coverage rather than expanding this fix commit further. The current PR now has targeted coverage for the sidechannel and bridge lifecycle, and the remaining hook-level renderHook coverage can be added separately without changing the product behavior.
| options?.images ?? [] | ||
| ).map((img) => ({ | ||
| data: img.data, | ||
| mimeType: img.mimeType || img.mediaType || img.media_type || 'image/*', |
There was a problem hiding this comment.
[Suggestion] Image normalization and prompt request building are duplicated verbatim between submitPrompt (here) and sendPrompt (line 200). Both use the same img.mimeType || img.mediaType || img.media_type || 'image/*' fallback chain, store.appendLocalUserMessage, and promptRequest construction. Extract a shared helper to prevent future drift.
— glm-5.2 via Qwen Code /review
There was a problem hiding this comment.
Leaving this refactor out of this PR to keep the review-fix commit scoped. The duplicated normalization is existing/localized logic and can be extracted in a separate cleanup without changing queue behavior.
| state: isQueued ? 'queued' : 'running', | ||
| releaseSlot: releasePromptSlot, | ||
| }; | ||
| entry.pendingPromptList.push(pendingEntry); |
There was a problem hiding this comment.
[Suggestion] No daemon-side logging for the pending prompt queue lifecycle. Only removePendingPrompt has a writeStderrLine log. The add (pendingPromptList.push), started (pendingEntry.state = 'running'), and completed (result.finally) transitions are invisible to operators. Add writeStderrLine calls at each transition including sessionId, promptId, state, and queueDepth.
— glm-5.2 via Qwen Code /review
There was a problem hiding this comment.
Leaving daemon lifecycle logging as a follow-up observability improvement. This PR already avoids adding extra runtime logging beyond the existing removal path, keeping the functional queue change focused.
| // genuinely queued (another prompt is already running/queued) — | ||
| // the first prompt on an idle session starts immediately and | ||
| // doesn't need a queue event. | ||
| const promptId = context?.promptId ?? randomUUID(); |
There was a problem hiding this comment.
[Suggestion] promptId is now always defined via context?.promptId ?? randomUUID(). Before this PR it was context?.promptId (potentially undefined), and broadcastTurnComplete/broadcastTurnError only included promptId when truthy. Now ALL turn_complete/turn_error events include a promptId, and parsePendingPromptEvent in pendingPromptVersion.ts accepts any terminal event with a string promptId. This means ALL terminal events (including regular blocking prompts and internal continuations) are added to the pending-prompt events buffer (capped at 200) and tracked by completedPromptIdsRef (capped at 100), increasing eviction pressure on entries that matter. Consider only assigning pendingEntry.promptId when the prompt is genuinely queued (isQueued === true).
— glm-5.2 via Qwen Code /review
There was a problem hiding this comment.
Leaving this as a follow-up protocol cleanup. The current bounded buffers tolerate terminal events with promptId, and changing promptId assignment semantics is broader than the review-fix scope.
| [], | ||
| ); | ||
|
|
||
| const removeServerPromptForAction = useCallback( |
There was a problem hiding this comment.
[Suggestion] Three functions (removeQueuedPrompt, editLastQueuedPrompt, clearQueuedPrompts) each inline the same server-prompt-removal error-handling pattern (add to removingServerPromptIdsRef, call sessionActions.removePendingPrompt, handle success/failure with refreshPendingPrompts fallback + restoreQueuedPrompts) that removeServerPromptForAction already encapsulates. editQueuedPrompt and insertQueuedPrompt correctly delegate. Refactoring the remaining three to use removeServerPromptForAction would eliminate ~80 lines of duplicated logic and ensure the error path stays consistent.
— glm-5.2 via Qwen Code /review
There was a problem hiding this comment.
Leaving this refactor out of this PR to avoid changing working removal flows while addressing review issues. The duplicate paths are now error-reported consistently; consolidating them can be a separate cleanup.
| (prompt) => prompt.serverState !== 'submitting', | ||
| ); | ||
| if (clearablePrompts.length === 0) return false; | ||
| for (const prompt of clearablePrompts) { |
There was a problem hiding this comment.
[Suggestion] clearQueuedPrompts fires a separate sessionActions.removePendingPrompt() HTTP DELETE request for each queued prompt. For a queue of N prompts, this creates N parallel HTTP requests (browsers limit to 6 concurrent connections per origin). Consider adding a batch DELETE /session/:id/pending-prompts endpoint, or firing the deletions sequentially to avoid connection saturation.
— glm-5.2 via Qwen Code /review
There was a problem hiding this comment.
Leaving batch deletion as a follow-up API optimization. The current queue cap keeps the number of deletes bounded, and adding a new batch endpoint would expand the PR surface.
|
Addressed the pending prompt review feedback in 9db3dc6. Fixed:
Not changed intentionally:
|
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
[Critical] First-to-idle prompt stuck in UI queue (race condition)
isQueued = entry.pendingPromptCount > 1 means the first prompt on an idle session gets NO SSE lifecycle events (pending_prompt_added/started/completed). If the HTTP POST response arrives while streamingState is still 'streaming' (previous turn_complete SSE not yet processed by the client), enqueuePrompt's .then() marks the prompt as serverState: 'queued'. No SSE events will ever promote it out of that state — the prompt runs and completes on the daemon but appears stuck in the queue UI.
Fix: either always publish pending_prompt_added for every accepted prompt, or add a streamingState transition listener in useQueuedPrompts that calls refreshPendingPrompts when transitioning to 'idle' with a non-empty queue.
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No new high-confidence issues found in this review pass. Downgraded from Approve to Comment: CI still running.
Tests verified: bridge.test.ts (314/314 passed), pendingPromptVersion.test.ts + DaemonSessionProvider.test.tsx (115/115 passed).
Needs Human Review (low-confidence findings, may warrant maintainer judgment):
-
Queued prompt removal test does not assert
pending_prompt_completedevent or verifyreleaseSlot()decrementspendingPromptCount— a regression in capacity release would go undetected. -
DELETE /session/:id/pending-prompts/:promptIdcancels running turns without API-level distinction from queue removal — any authorized API consumer can cancel active work via a queue management endpoint. -
enqueuePromptsession-mismatch cleanup callssessionActions.removePendingPromptwith the old session ID, butsessionActionsis bound to the new session client, so cleanup returns{removed: false}silently. -
removeQueuedPromptsilently returns forserverState === 'submitting'with no user feedback, whileclearQueuedPromptshandles submitting prompts via abort.
— qwen3.7-max via Qwen Code /review
DragonnZhang
left a comment
DragonnZhang
left a comment
There was a problem hiding this comment.
Automated code review — no high-confidence critical findings in the changed code at this commit.
Reviewed bridge.ts pending prompt list, session.ts connection-close detection, and bridgeTypes.ts type definitions. Key observations:
releasePromptSlotis idempotent viapromptSlotReleasedguard, preventing double-decrement whenremovePendingPromptcallstarget.releaseSlot()andresult.finally()also callsreleasePromptSlot().isQueued = entry.pendingPromptCount > 1is evaluated after increment — correctly identifies prompts that must wait behind an in-flight turn.- Connection close detection (
res.once('close', onResClose)) is intentionally scoped to the pre-dispatch window; the handler is removed afterbridge.sendPromptreturns, so connection drops during response streaming do not abort a running turn (consistent with the existing cancel-is-explicit semantics). pending_prompt_completedwithstate: 'removed'is always published byremovePendingPrompt, even for the first (non-queued) prompt — this provides clients an explicit terminal signal on API-triggered removal, which differs from normal completion but is consistent with theremovedstate semantics.
No critical issues found.
Generated by Claude Code
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
|
@qwen-code /resolve |
… main The PR refactors web-shell prompt queueing from client-side drain logic to server-side pending prompts via the useQueuedPrompts hook. Main added a transports bundle and its own inline queue. Resolutions: - sdk-typescript/scripts/build.js: keep PR's 132KB daemon bundle budget (for queue feature) plus main's 48KB transports budget (independent) - App.tsx: drop main's inline queue refs, pop/peek/drain logic, and midTurnInjected reconciliation — all superseded by useQueuedPrompts; remove now-unused canDrainQueue import and duplicate QueuedPromptDisplay import; keep PR's editLastQueuedPrompt/clearQueuedPrompts callbacks - ChatEditor.tsx: keep PR's () => boolean return type for onPopQueuedMessages and the separate onClearQueuedMessages prop - QueuedPromptDisplay.tsx: keep PR's full implementation with server-state tracking, spinner states, and QueuedPrompt interface - useComposerCore.ts: keep PR's () => boolean signature for onPopQueuedMessages plus onClearQueuedMessages prop
|
Qwen Code resolved the merge conflicts and pushed the branch update. Merge Conflict Resolution Summary — PR #6005OverviewPR #6005 ( Conflicts and Resolutions1.
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM. After thorough multi-dimensional review (correctness, security, performance, test coverage, code quality), all identified issues have either been addressed in recent commits (signal forwarding in actions.ts, abort test in sessionRoutes.test.ts, dead releaseSlot field removal, insertQueuedPrompt rejection feedback, queue reconciliation identity preservation) or acknowledged as intentional follow-ups (SSE turn_complete without promptId, extractPromptText length bound).
The daemon-backed prompt queue architecture is well-structured. The FIFO enforcement via semaphore, SSE queue state broadcasting, and ACP bridge integration are solid. Test coverage is comprehensive.
DragonnZhang
left a comment
DragonnZhang
left a comment
There was a problem hiding this comment.
Incremental review: 40ebe88a..b922821e (2 commits)
Commit 81e10fc92 — test(webui): update stale session queue cleanup expectation
Commit b922821ee — fix(web-shell): preserve queue reconciliation identity
Summary
This incremental is clean. Three focused changes, no regressions found.
-
Queue reconciliation identity preservation (
useQueuedPrompts.ts): SplittingshouldHidePromptintohasDisplayedPrompt(branches 1 & 2) andserverPrompt.state === 'running' || hasDisplayedPrompt(branch 3) correctly prevents visual flicker. A running-but-not-yet-displayed prompt retains its local queue identity instead of being removed and re-added. ThesubmittingMatches.length === 1guard avoids incorrect associations when duplicate texts are queued simultaneously. Both changes are sound. -
Bridge observability (
bridge.ts): Added stderr log for queued prompts removed before agent forward. Diagnostic-only, no logic change. -
Test fix (
DaemonSessionProvider.test.tsx): Correctly aligns the stale-sessionremovePendingPromptexpectation with the implementation that was updated in commit40ebe88a1(which stopped passingclientId). The test was stale since that commit.
No high-confidence issues found in this incremental.
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
No critical review findings on the latest commit. The previous duplicate-text reconciliation and running-prompt visibility blockers are fixed.
Downgraded from Approve to Comment because GitHub checks are still pending: Test (ubuntu-latest, Node 22.x), delay-automatic-review.
Focused verification passed locally:
- packages/webui: DaemonSessionProvider.test.tsx and pendingPromptVersion.test.ts
- packages/web-shell: QueuedPromptDisplay.test.tsx and localCommandQueue.test.ts
- packages/acp-bridge: bridge.test.ts
-- GPT-5 via Qwen Code /review
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Found two blocking regressions in the current queue handling.
| }; | ||
| continue; | ||
| } | ||
| if (serverPrompt.state === 'running' || hasDisplayedPrompt) { |
There was a problem hiding this comment.
[Critical] The duplicate-text guard is still incomplete. When two local submitting prompts share text, submittingMatches.length is greater than 1, so the single-match branch is skipped, but execution falls through to this server-row path for each pending prompt. That leaves the original submitting rows in place and adds server-backed duplicates that lack the local images and onComplete callback; if the started event later matches one of those synthetic rows by serverPromptId, the transcript can lose image attachments or completion cleanup.
| if (serverPrompt.state === 'running' || hasDisplayedPrompt) { | |
| if (submittingMatches.length > 1) { | |
| continue; | |
| } | |
| if (serverPrompt.state === 'running' || hasDisplayedPrompt) { |
— GPT-5 via Qwen Code /review
There was a problem hiding this comment.
Thanks for catching this. I agree this remaining duplicate-text fallthrough should be fixed, but I will leave it out of this PR and address it in a follow-up PR so we do not keep expanding the current review-fix surface.
| _images: PromptImage[] | undefined, | ||
| sink: LocalEchoSink, | ||
| ): boolean { | ||
| if (isStreaming) { |
There was a problem hiding this comment.
[Critical] This suppressed path still reports the submit as accepted. useComposerCore clears the editor for any onSubmit result other than false, and local echo commands such as /context, /stats, /status, /tools, /skills, and /bug call this helper. During a turn they now return true without enqueueing, appending, or showing queue.commandBlocked, so the typed command disappears with no effect or feedback.
Please route these callers through the same blocked-command path used by the other local slash commands, or otherwise return false from the editor submit path after surfacing a visible toast/status so the draft remains in the composer.
— GPT-5 via Qwen Code /review
✅ Local real-binary verification (daemon FIFO queue) — recommend mergeI verified this PR by building the real Setup
Results — 24/24 daemon-layer assertions pass
Representative evidence (real daemon logs): Fake-model request log: removed-prompt marker One thing worth flagging (not a defect)Removing a running prompt forwards cancel and clears daemon-visible state immediately, but does not tear down the in-flight upstream model HTTP request. I confirmed the pre-existing Consumer layers + static checks (clean checkout of PR head)
Scope / not covered
Verdict: the daemon-side queue, SSE event lifecycle, removal semantics, and the cap‑bypass invariant all hold on the real binary. LGTM to merge. 🇨🇳 中文版（完整对应）✅ 本地真实二进制验证（daemon FIFO 队列）— 建议合并我用 PR head（ 环境
结果 — daemon 层 24/24 断言通过
代表性证据（真实 daemon 日志）： 假模型请求日志：被移除 prompt 的标记在所有请求体中 一个值得说明的点（不是缺陷）移除运行中的 prompt 会转发 cancel 并立即清空 daemon 可见状态，但不会强行中断正在进行的上游模型 HTTP 请求。我确认既有的 消费侧 + 静态检查（PR head 干净检出）
范围 / 未覆盖
结论：daemon 端队列、SSE 事件生命周期、移除语义、以及防绕过上限的不变量，在真实二进制上全部成立。LGTM，可以合并。 Verification harness: real |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Missing test coverage for packages/web-shell/client/hooks/useQueuedPrompts.ts — the 942-line hook managing the entire pending prompt queue lifecycle (server sync, SSE event handling, completion callbacks, session-switch cleanup) has no test file. This is the central state management for the feature and should have tests covering at least: (1) session-switch cleanup, (2) syncServerQueuedPrompts merging with submitting-match dedup, (3) refreshPendingPrompts stale-response discard, and (4) completion callback dispatch on pending_prompt_completed events.
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
Real-binary daemon verification on current head (e90f00ae2) confirms the FIFO queue lifecycle, SSE event ordering, and removal semantics end-to-end. Post-approval fixes (a9c37a49a, e90f00ae2) are minimal and correct. 2962 tests across 5 affected packages all green; build/typecheck/bundle clean.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Code Review Summary
This PR adds daemon-backed prompt queuing for web shell sessions — a significant architectural change touching 36 files (+3274/-504). The overall design is sound: server-side FIFO queue, SSE events for real-time sync, and proper authorization at session boundaries.
Tests: All 2190 existing tests pass (acp-bridge: 316, cli/server: 551, sdk: 1108, webui: 215). ESLint clean, no new TypeScript errors.
Key concerns:
- 5 Critical findings around data loss, authorization gaps, and missing test coverage
- 4 Suggestions for robustness improvements
- 3 Nice-to-have items for code hygiene
The most impactful issues are the SSE/POST race that can create duplicate queue entries (F2), the stale-session authorization bypass (F4), and the complete lack of tests for the 944-line orchestrator hook (F3).
Reviewed by qwen3.7-max
| p.serverState === 'submitting' && | ||
| p.text === serverPrompt.text, | ||
| ); | ||
| if (submittingMatches.length === 1) { |
There was a problem hiding this comment.
[Critical] syncServerQueuedPrompts matches server prompts to local "submitting" entries by text equality (p.text === serverPrompt.text). When the user rapidly sends two prompts with identical text, submittingMatches.length > 1 and the code falls through to next.push(...), creating a duplicate queue entry.
The SSE pending_prompt_added event arrives before the HTTP POST response returns, so the timing window is real. Users see their message twice in the queue, and deleting one leaves an orphan.
Suggested fix: Use a correlation token instead of text matching. Have submitPrompt return the server-assigned promptId from the POST response and stamp it on the local entry in the .then() handler. The SSE sync path should match on a client-generated correlation ID rather than text equality.
Reviewed by qwen3.7-max
| prompt: ReadonlyArray<Record<string, unknown>>, | ||
| ): string { | ||
| if (!Array.isArray(prompt)) return ''; | ||
| let hasImage = false; |
There was a problem hiding this comment.
[Critical] extractPromptText returns immediately on finding the first text block (return block['text']). Multi-text-block prompts (valid in the ACP protocol) show truncated text in the queue UI.
More critically, the pending_prompt_started event handler in useQueuedPrompts uses this text to match against local queue entries and call store.appendLocalUserMessage(text, ...). If the server's extracted text doesn't match the client's full prompt text, the text-match fallback (item.text === eventText) fails, and the local user message silently disappears from the chat transcript.
Suggested fix: Concatenate all text blocks:
const texts: string[] = [];
let hasImage = false;
for (const block of prompt) {
if (block['type'] === 'text' && typeof block['text'] === 'string')
texts.push(block['text']);
if (block['type'] === 'image') hasImage = true;
}
return texts.join('') || (hasImage ? '[image]' : '');Reviewed by qwen3.7-max
| editLastQueuedPrompt: () => boolean; | ||
| clearQueuedPrompts: () => boolean; | ||
| } | ||
|
|
There was a problem hiding this comment.
[Critical] No test file exists for this 944-line hook, which is the primary UI-side orchestrator for the entire prompt queuing feature. Many complex branches are entirely untested:
syncServerQueuedPrompts: merge logic with 5+ branches (existing match, submitting match, running skip, new push)enqueuePrompt: success path with session-change cleanup, streaming-state branching, error recovery with text restorationremoveServerPromptForAction: multi-step async with guard refs, refresh failure recovery, partial error handlingclearQueuedPrompts: batch removal withPromise.all, partial failure rollback, session-change guardsettleCompletionCallback: race between completion event arrival andonCompleteregistration- Event handling effect:
pending_prompt_started,turn_complete,turn_error,pending_prompt_completedwithremovedstate
Bugs in any of these branches could cause lost prompts, duplicated prompts, stale queue state, or leaked abort controllers.
Suggested fix: Add useQueuedPrompts.test.ts using renderHook with mocked sessionActions, store, and editorRef. Cover at minimum: (1) enqueue success, (2) enqueue error with text restoration, (3) session change clears queue, (4) removeServerPromptForAction with removed: false refresh path, (5) clearQueuedPrompts partial failure rollback, (6) settleCompletionCallback race.
Reviewed by qwen3.7-max
| const session = sessionRef.current; | ||
| if (!session) return { removed: false }; | ||
| if (opts?.sessionId && session.sessionId !== opts.sessionId) { | ||
| return await session.client.removePendingPrompt( |
There was a problem hiding this comment.
[Critical] In the stale-session path (when opts.sessionId differs from the current session), this calls session.client.removePendingPrompt(opts.sessionId, promptId) with only two arguments — no clientId is passed.
DaemonClient.removePendingPrompt accepts a third opts?: { clientId?: string } parameter, but it's omitted here. The DELETE request arrives at the server with no X-Qwen-Client-Id header. resolveTrustedClientId(entry, undefined) returns undefined without checking entry.clientIds — the session-level authorization is entirely bypassed.
This means any web UI client holding the bearer token can remove pending prompts from any session on the daemon, even sessions it has never registered with.
Suggested fix: Pass the current session's clientId:
return await session.client.removePendingPrompt(
opts.sessionId,
promptId,
{ clientId: session.clientId },
);Reviewed by qwen3.7-max
| // Abort the prompt: for 'queued' prompts the FIFO will skip | ||
| // dispatch on the `signal.aborted` check; for 'running' prompts | ||
| // this triggers the cancel path. | ||
| target.abortController.abort( |
There was a problem hiding this comment.
[Critical] When a running prompt is removed, abortController.abort() triggers the FIFO's onAbort handler, which calls cancelPendingForSession(sessionId) (clears ALL pending prompts) and entry.connection.cancel({ sessionId }) (cancels the entire ACP session turn — stops the agent's current response, interrupts all in-flight tool calls, terminates the turn).
The API surface exposes DELETE /session/:id/pending-prompts/:promptId which suggests granular, per-prompt control. But when promptId identifies the currently running prompt, the effect is session-wide. Destructive tool calls (file writes, shell commands) may be interrupted mid-execution, leaving the workspace in an inconsistent state.
Suggested fix: Distinguish queued vs. running state in removePendingPrompt. For running prompts, either (a) return an error indicating the prompt is already running and cannot be removed through this endpoint, or (b) require the existing cancel endpoint for running prompts.
Reviewed by qwen3.7-max
| if (options?.retry) { | ||
| promptRequest['retry'] = true; | ||
| } | ||
| const accepted = await session.submitPrompt( |
There was a problem hiding this comment.
[Suggestion] submitPrompt accepts an AbortSignal in options?.signal but never forwards it to session.submitPrompt(). The signal is only checked AFTER the HTTP request completes (line 288: if (options?.signal?.aborted)), so the admission POST request cannot be cancelled mid-flight.
The test at DaemonSessionProvider.test.tsx:1008 explicitly asserts expect(signal).toBeUndefined(), which codifies this behavior.
Impact: When a user cancels a queued prompt while the admission HTTP request is in flight, the request completes and the prompt enters the daemon queue before the post-submission cleanup removes it. This wastes network/daemon resources.
Suggested fix: Forward the signal:
const accepted = await session.submitPrompt(
promptRequest as Parameters<typeof session.submitPrompt>[0],
options?.signal,
);Then update the test to expect the signal to be passed through.
Reviewed by qwen3.7-max
| @@ -15,7 +15,6 @@ describe('appendOrDeferLocalUserMessage', () => { | |||
| undefined, | |||
| { | |||
There was a problem hiding this comment.
[Suggestion] The enqueue mock (const enqueue = vi.fn()) is created in all three test cases but is never passed to the function under test. The LocalEchoSink interface no longer has an enqueue field, so the mock is entirely disconnected from the code path. The expect(enqueue).not.toHaveBeenCalled() assertions are vacuously true — they will pass regardless of implementation.
Suggested fix: Remove the const enqueue = vi.fn() declarations and the corresponding expect(enqueue).not.toHaveBeenCalled() lines from all three tests.
Reviewed by qwen3.7-max
| store.appendLocalUserMessage(text, toStoreImages(prompt?.images)); | ||
| } | ||
| } | ||
| void refreshPendingPrompts(); |
There was a problem hiding this comment.
[Suggestion] When a pending_prompt_started event arrives, both useEffect hooks call refreshPendingPrompts(), firing two parallel GET /session/:id/pending-prompts requests for the same data:
- The
pendingPromptVersioneffect: the event increments the version counter, triggering the refresh - The
pendingPromptEventseffect: explicitly callsrefreshPendingPrompts()here
On high-latency connections or when multiple prompts drain rapidly, this doubles network traffic for queue reconciliation.
Suggested fix: In the pendingPromptEvents effect, skip refreshPendingPrompts() when the event type is one that also bumps the version counter (pending_prompt_added, pending_prompt_started, pending_prompt_completed), since the version effect already handles the refresh. Only call it for turn_complete / turn_error events that do NOT bump the version.
Reviewed by qwen3.7-max
| queuedMessages?: string[]; | ||
| onPopQueuedMessages?: () => string | null; | ||
| onPopQueuedMessages?: () => boolean; | ||
| onClearQueuedMessages?: () => boolean; |
There was a problem hiding this comment.
[Nice to have] onClearQueuedMessages is defined in ChatEditorProps (here), passed from App.tsx:3636 with clearQueuedPrompts, and defined in useComposerCore options (useComposerCore.ts:758), but it is never destructured from props, never passed to useComposerCore, and never invoked anywhere.
This is dead code — the prop chain adds API surface without functionality.
Suggested fix: Either remove the prop entirely from all three locations (ChatEditorProps, useComposerCore options, App.tsx call site), or wire it into the Escape key handler so pressing Escape when queued prompts exist clears them.
Reviewed by qwen3.7-max
| target.isEditing || | ||
| target.isRemoving | ||
| ) { | ||
| return true; |
There was a problem hiding this comment.
[Nice to have] editLastQueuedPrompt returns true when the target is busy (submitting, isEditing, or isRemoving), but does NOT restore the text to the editor. The caller in useComposerCore.ts interprets true as "handled — consume the keypress."
The user presses Up arrow expecting to edit the last queued prompt, but nothing visible happens — the text does not appear in the editor, and the keypress is consumed so the cursor does not move either. No error, no toast, no feedback.
Suggested fix: Either return false for busy prompts (letting the Up arrow fall through to normal cursor movement) or show a toast explaining the prompt is busy.
Reviewed by qwen3.7-max
6 participants
What this PR does
Adds daemon-backed prompt queuing for web shell sessions so messages submitted while a turn is already running are accepted into a server-side FIFO queue instead of being tracked only in local UI state. The web shell now shows queued prompts with controls to move them out of the queue, insert them into the current session for the next model call, or move them back into the composer for editing. Shell commands are kept out of the queue while a turn is active.
This also extends the daemon HTTP and SDK surfaces with pending prompt list/remove APIs and structured pending-prompt SSE events. The web UI bridge consumes those events to keep queue state, user-message display, completion callbacks, and loading state synchronized across session updates without repeatedly polling the pending-prompts endpoint.
Why it's needed
When users send another message during an active turn, the browser needs a reliable queue that survives shared-session and multi-client scenarios. Keeping queued prompts only in local web-shell state makes deletion, editing, session switching, and cross-client visibility fragile. Moving the queue to the daemon gives the UI a single source of truth and lets users manage pending work before the daemon starts executing it.
Reviewer Test Plan
How to verify
Start a daemon-backed web shell session, send a prompt that keeps the model busy, then type another normal prompt. Confirm the second prompt appears in the queue, the composer send button remains available while text is present, and the queued prompt moves into the transcript when the daemon starts it. Queue two prompts and confirm that completion of the first queued prompt does not cause duplicate pending-prompts requests before the second starts. Try moving a queued prompt out of the queue, inserting it into the current session, and editing it back into the composer; each action should first remove the server queue entry and should show the expected UI feedback. Try a shell command during an active turn and confirm it is not queued.
Evidence (Before & After)
Before: prompts typed during an active web-shell turn were handled locally and could not be reliably inspected, removed, edited, or synchronized across daemon clients. After: queued prompts are represented by daemon pending-prompt state, exposed through SDK/HTTP APIs, reflected in the web-shell queue UI, and synchronized by structured SSE events.
Tested on
Environment (optional)
Local verification used package-level TypeScript checks, targeted unit tests, SDK event tests, ACP bridge pending prompt tests, and a webui package build.
Risk & Scope
Linked Issues
N/A
中文说明
What this PR does
为 web shell 会话增加 daemon 侧 prompt 排队能力。当一个 turn 正在运行时，新提交的普通消息会进入服务端 FIFO 队列，而不是只保存在本地 UI 状态里。web shell 现在会展示排队消息，并支持将消息移出队列、插入当前会话并在下次模型调用生效，或移出队列后放回输入框重新编辑。Shell 命令在 turn 运行中不会进入队列。
这个 PR 也扩展了 daemon HTTP 与 SDK 能力，新增 pending prompt 列表/移除 API，以及结构化的 pending-prompt SSE 事件。web UI bridge 会消费这些事件，用来同步队列状态、用户消息展示、完成回调和 loading 状态，同时避免对 pending-prompts 接口做重复轮询。
Why it's needed
用户在当前 turn 运行时继续发送消息，需要一个可靠的队列来支持共享 session 和多客户端场景。只把排队消息保存在 web-shell 本地状态里，会让删除、编辑、切换 session 和跨客户端可见性变得脆弱。把队列放到 daemon 后，UI 有了统一的数据源，用户也能在 daemon 开始执行前管理待处理的消息。
Reviewer Test Plan
How to verify
启动 daemon-backed web shell session，先发送一个会让模型保持忙碌的 prompt，然后再输入一条普通 prompt。确认第二条 prompt 会出现在队列里；输入框有内容时发送按钮仍可用；当 daemon 开始执行这条排队 prompt 时，它会进入对话区。连续排两条 prompt，确认上一条完成不会在下一条 started 前触发重复 pending-prompts 请求。尝试将排队 prompt 移出队列、插入当前会话、以及移出队列后放回输入框编辑；这些操作都应该先移除服务端队列项，并展示对应 UI 反馈。运行中尝试输入 shell 命令，确认它不会进入队列。
Evidence (Before & After)
Before：web-shell turn 运行中输入的 prompt 主要由本地状态处理，无法可靠地检查、移除、编辑或在多个 daemon 客户端之间同步。After：排队 prompt 由 daemon pending-prompt 状态表示，通过 SDK/HTTP API 暴露，在 web-shell 队列 UI 中展示，并通过结构化 SSE 事件同步。
Tested on
Environment (optional)
本地验证包括 package 级 TypeScript 检查、定向单元测试、SDK 事件测试、ACP bridge pending prompt 测试，以及 webui package build。
Risk & Scope
Linked Issues
N/A