fix(web-shell): allow shell commands in new tasks without a session - #7724
Conversation
Previously, typing a ! command in a brand-new task showed "No active session yet" because requireActiveSessionForLocalCommand() rejected the command when no session existed. Replace it with ensureSessionForPrompt() — the same lazy session creation path used by regular messages — so the session is created on demand before the shell command executes.
|
Thanks for the PR! Template looks good ✓ Problem: observed UX limitation — Direction: aligned. Lazy session creation via Size: not applicable — changes are in Approach: the scope feels right. The two changes (lazy creation + mid-turn queuing) are tightly coupled — both are about removing artificial restrictions on Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：已观测到的 UX 限制—— 方向：对齐。通过 规模：不适用——改动在 方案：范围合理。两个改动（懒创建 + 回合中排队）紧密耦合——都是关于移除 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Code ReviewIndependent proposal: given the problem (shell commands blocked in new tasks and during turns), I would (1) reuse Comparison with the diff: the PR's approach matches my independent proposal. The implementation is clean and follows existing patterns:
No correctness bugs, security holes, or regressions found. No AGENTS.md violations — the code stays in Test coverage is thorough: 20+ cases covering lazy creation, deduplication, FIFO drain, session switch, cancel (pre-drain and mid-drain), disconnect, error continuation, language change survival, generation staleness, retry after cancel, and the race between session-id render commit and attach resolution. The tests exercise the actual concurrent scenarios that make this feature non-trivial. CI Test EvidenceAll checks completed on
macOS/Windows tests and integration tests are skipped (fork PR, expected). The unit suite and web-shell E2E smoke both pass. No failures. Real-scenario testing: N/A for this CI run. The PR touches Web Shell UI behavior; a maintainer can check out the branch in a disposable environment to verify the new-task and mid-turn queuing flows interactively. The sandboxed 中文说明代码审查独立方案： 给定问题（shell 命令在新任务和回合中被阻止），我会 (1) 复用 与 diff 的比较： PR 方案与我的独立提案一致。实现干净，遵循现有模式：
未发现正确性 bug、安全漏洞或回归。无 AGENTS.md 违规——代码在 测试覆盖 全面：20+ 用例覆盖懒创建、去重、FIFO drain、session 切换、取消（drain 前和 drain 中）、断连、错误继续、语言切换存活、generation 过期、取消后重试、以及 session-id 渲染提交与 attach 解析之间的竞态。测试覆盖了使此功能非平凡的实际并发场景。 CI 测试证据
真实场景测试： 本次 CI 运行不适用。PR 触及 Web Shell UI 行为；维护者可在隔离环境中 checkout 分支以交互验证新任务和回合中排队流程。沙箱 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 4/5 — solid, well-tested implementation that reuses existing infrastructure cleanly; the drain machinery is the minimum complexity the concurrent scenarios require. Stepping back: this PR removes two artificial restrictions on The drain loop is the most complex part, and it earns its complexity: cancel mid-drain, session switch mid-drain, disconnect, commands queued while a drain is running — each is a real scenario a user can hit, and each is handled with the generation counter + After 8 review rounds from the author and multiple autofix iterations, the earlier blocking findings (drain machinery robustness, cancel/switch edge cases) have been addressed. The diff is focused — every change serves the stated goal, no drive-by refactors. Not 5/5 only because four mutable refs for queue management in an already-large App component is a lot of state to reason about — but that's inherent to the feature, not a design flaw. If I had to maintain this in six months, the generation-counter pattern and the thorough test suite would make it straightforward. CI is green on the reviewed commit. Approving. ✅ 中文说明置信度：4/5 —— 实现扎实、测试充分，干净地复用了现有基础设施；drain 机制是并发场景所需的最小复杂度。 退一步看：这个 PR 移除了 Drain 循环是最复杂的部分，但其复杂度是必要的：drain 中取消、drain 中切换 session、断连、drain 运行时排队新命令——每个都是用户可能遇到的真实场景，每个都通过 generation 计数器 + 经过作者 8 轮审查和多次 autofix 迭代，早期的阻塞性发现（drain 机制健壮性、取消/切换边界情况）已得到解决。Diff 聚焦——每个改动都服务于既定目标，无顺手重构。 未给 5/5 仅因为在一个已经很大的 App 组件中用四个可变 ref 管理队列状态需要较多推理——但这是功能本身决定的，不是设计缺陷。如果六个月后维护这段代码，generation 计数器模式和全面的测试套件会让工作变得简单。 CI 在审查的提交上全绿。批准。✅ — Qwen Code · qwen3.8-max-preview Reviewed at |
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / afterℹ️ No screenshot changed against the PR base — but this PR edits 2 render-shaping files:
Either the change has no visual effect (logic, plumbing, a state the scenarios never reach), or no scenario renders this UI — in which case the preview cannot see it, and an empty result is a coverage gap rather than a clean bill of health. To make it visible, add a scenario to Full-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
— 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.
LGTM, looks ready to ship — CI landed green after the review. ✅
Review + verification report (Linux, built workspace, jsdom component harness)Verdict: the fix is correct and minimal — I proved the behavior delta with a probe test against the real Code review reasoning
Verification evidence (commit 229276e, workspace built from source)I inserted a probe test into the existing it('lazily creates a session for ! shell commands in a new task', async () => {
mockConnection.sessionId = undefined;
renderApp();
await flush();
await act(async () => {
testState.latestChatEditorProps?.onSubmit('!echo hi');
await vi.waitFor(() => {
expect(mockSessionActions.sendShellCommand).toHaveBeenCalledWith('echo hi');
});
});
expect(mockSessionActions.createSession).toHaveBeenCalled();
});
Suggestion (non-blocking)The PR description says the behavior is "covered by unit tests", but the diff contains no test. Since a session-less (Scope note: no browser was available in this environment, so verification used the component-level harness rather than a click-through; the harness drives the same |
When a turn is running, ! commands are now queued client-side and executed sequentially after the turn finishes, instead of showing an error toast. A drain effect watches streamingState returning to idle and runs pending commands via sendShellCommand(). The user gets an info toast confirming the command was queued.
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
| queuedShellCommandsRef.current.push(cmd); | ||
| pushToast('info', t('queue.shellQueued')); | ||
| return true; |
There was a problem hiding this comment.
[Suggestion] Queued shell commands have no user visibility or cancellation mechanism, unlike the existing prompt queue which renders in QueuedPromptDisplay with delete/edit/insert controls. — Failure scenario: User types !rm -rf build/ during a long turn. A transient info toast appears and disappears. The user forgets. When the turn ends, the drain effect auto-executes the destructive command with no confirmation and no way to cancel. Regular prompts queued during a turn are displayed with onDelete, onInsert, onEdit — shell commands bypass this system entirely.
中文说明
[Suggestion] 排队的 shell 命令没有用户可见性或取消机制，而现有的 prompt 队列通过 QueuedPromptDisplay 提供删除/编辑/插入控件。—— 失败场景：用户在长时间回合中输入 !rm -rf build/，出现一个短暂的 info toast 后消失。用户忘记了。回合结束后，drain effect 自动执行该破坏性命令，没有确认也没有取消方式。回合中排队的普通 prompt 会显示 onDelete、onInsert、onEdit 控件 —— shell 命令完全绕过了这套系统。
— qwen3.7-max via Qwen Code /review
|
@qwen-code /takeover |
qwen-code-dev-bot
added
the
autofix/takeover
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。移除 |
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/100 轮）。改动内容与我反驳保留之处如下： Autofix review round — PR #7724Addressed the automated reviewer's findings on the shell-command queueing change. One correctness fix, four small UX/consistency improvements that mirror the existing prompt path, and new test coverage. One finding is left open for a maintainer decision (below). No conflicts ( Findings addressed
Open question for a maintainer (left unresolved)
Conflict notesNone — Verification
中文说明Autofix 审查轮次 — PR #7724处理了自动审查器针对 shell 命令排队改动提出的意见。包含一处正确性修复、四处镜像现有 prompt 路径的小型 UX/一致性改进，以及新增的测试覆盖。有一项发现留给维护者决定（见下文）。无冲突（ 已处理的发现
留给维护者的开放问题（保持未解决）
冲突说明无 — 验证
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline. 1 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here.
中文说明
已审查——无阻断问题。 建议见行内评论。 1 条建议级发现无法锚定到改动行，已丢弃；此处无需进一步处理。
— qwen3.7-max via Qwen Code /review
Review:
|
| Check | Result |
|---|---|
npx vitest run client/App.test.tsx |
✅ 177/177 pass |
npx tsc --noEmit -p packages/web-shell/tsconfig.json |
✅ clean |
npx prettier --check on the 3 changed files |
✅ clean |
| Mutation: neuter the session-switch clear effect | ✅ drops queued ! commands when the session changes fails → test is load-bearing |
Mutation: return early in the drain effect |
✅ queues a ! command during a turn and drains it fails → test is load-bearing |
Dangling queue.shellBlocked references |
✅ none remain |
The design is sound in one important respect worth calling out: promptBlocked is defined as exactly streamingStateRef.current !== 'idle' (App.tsx:4933), and the drain guard is exactly streamingState !== 'idle'. Because the enqueue predicate and the drain predicate are the same condition, there is no "queued but never drained" class of bug — a common failure mode for this pattern. The ref-sync effect (streamingStateRef.current = streamingState) is also declared before the drain effect, so the ref can't be stale at drain time.
🔴 Blocking: a mid-drain session switch runs the remaining commands against the new session
The clear-on-switch effect only protects commands still sitting in queuedShellCommandsRef. The drain empties the ref synchronously and then iterates a local cmds array with await between each command:
queuedShellCommandsRef.current = [];
void (async () => {
for (const cmd of cmds) {
try {
await sessionActions.sendShellCommand(cmd); // <- resolves the session lazily, each timesendShellCommand resolves sessionRef.current at call time (packages/webui/src/daemon/session/actions.ts:1094), not at effect time. So once the loop is running, every command after the first targets whichever session is current at that moment — potentially a different workspace's daemon and cwd. The window is as wide as the slowest command in the queue (!npm run build, !pytest, …).
Verified with a probe test appended to App.test.tsx on the PR branch: queue !first + !second during a turn; go idle so the drain starts and blocks on a pending first; switch connection.sessionId to session-2; resolve first. Result:
PROBE calls: [["first"],["second"]]
second is dispatched after the switch. This is the same hazard da71f01 set out to close — it only closed the pre-drain half.
Suggested fix — pin the session for the whole drain:
useEffect(() => {
if (streamingState !== 'idle') return;
const cmds = queuedShellCommandsRef.current;
if (cmds.length === 0) return;
queuedShellCommandsRef.current = [];
const drainSessionId = connectionRef.current.sessionId;
void (async () => {
for (const cmd of cmds) {
// The session can change while an earlier command is still running.
if (connectionRef.current.sessionId !== drainSessionId) return;
try {
await sessionActions.sendShellCommand(cmd);
} catch (error: unknown) {
reportError(error, 'Failed to execute shell command');
}
}
})();
}, [streamingState, sessionActions, reportError]);connectionRef.current is assigned during render (App.tsx:2940), so it is always current. Please add a regression test for the mid-drain case alongside the existing pre-drain one.
🟠 Queued shell commands are invisible and uncancellable
The PR's stated rationale is parity with regular queued messages, but the two differ in a way that matters:
- Queued prompts render in the queued-messages list with a per-item delete (
ChatPane.tsx:654-656,prompts={queuedPrompts} onDelete={removeQueuedPrompt}). - Queued shell commands live only in a ref. No list, no delete, no count. The only feedback is one transient toast.
Concretely: a user starts a long turn, types !rm -rf build/ (the PR's own test fixture), then thinks better of it and presses Stop. handleCancel → sessionActions.cancel() aborts the prompt controller keyed on session.sessionId (actions.ts:465); the shell entry is keyed ${sessionId}:shell and the client-side queue isn't touched at all. The turn goes idle → the drain fires → the command the user tried to abort executes anyway.
At minimum this deserves a decision in the PR: either surface queued shell commands in the existing queued-messages UI, or drop the queue on cancel. Silently deferring destructive commands past a Stop is the part I'd push back on hardest after the blocker above.
Related, smaller: the queue is unbounded and undeduplicated — hold Enter on !ls during a long turn and you get N toasts and N sequential executions when it ends.
🟡 Smaller notes
- Copy-paste from
sendPrompt. ThesetSessionListReloadToken+ 2 sdelayedReloadTimerRefblock is duplicated verbatim fromApp.tsx:3171-3179. Both sites share the same timer ref, so they already interact; extracting ascheduleSessionListReload()helper would keep them from drifting. - Error label can mislead. The catch reads
connectionRef.current.sessionIdat rejection time.connectionRefis updated during render, but the webui provider'ssessionRefis updated synchronously on attach — so asendShellCommandfailure that lands before React re-renders reports'Failed to create session for shell command'. Capturing the outcome ofensureSessionForPrompt()in a local (e.g. alet created = falseset in the.then) would make the label exact. - No unmount cleanup on the drain. A tab close mid-drain leaves the loop running. Low impact, but a
let cancelled = false+ cleanup would match the surrounding effect style and would fall out of the blocker fix anyway. - i18n. EN and ZH updated symmetrically and no
queue.shellBlockedreference survives. NoteMessagesisRecord<string, MessageValue>andcheck-i18nonly coverspackages/cli, so web-shell key drift isn't CI-gated — worth remembering, nothing to change here.
Test coverage
Good density for the happy paths, and both new effects are mutation-verified as load-bearing. Gaps: (a) the mid-drain switch above, (b) ordering with more than one queued command (the drain claims sequential FIFO but only ever exercises a single command), (c) drain-time failure — sendShellCommand rejecting on command 1 should still run command 2, which the code does but nothing asserts.
Verdict
The direction is right and the state machine is cleaner than I expected — the enqueue/drain predicates being literally the same expression removes a whole class of bugs. Please fix the mid-drain session pinning before merge, and make a deliberate call on the Stop-vs-queue semantics.
中文说明
概述
本 PR 修改 App.tsx 中 handleSubmit 的 ! 分支：(1) 新任务中 ! 命令改走 ensureSessionForPrompt() 懒创建 session；(2) 回合运行期间不再硬拦截，而是压入客户端 queuedShellCommandsRef，待 streamingState 回到 idle 后由 effect 顺序 drain。第三个提交新增了「切换 session 时清空队列」的 effect。
本地验证结果（worktree @ da71f01a）
vitest run client/App.test.tsx：177/177 通过tsc --noEmit：干净；prettier --check：干净- 变异测试：禁用「切换清空」effect → 对应用例失败；禁用 drain → 对应用例失败。两个新 effect 均被测试真实覆盖。
- 无遗留的
queue.shellBlocked引用。
值得肯定的一点：promptBlocked 的定义正是 streamingState !== 'idle'（App.tsx:4933），与 drain 的判据完全一致，因此不存在「入队后永不 drain」这类常见缺陷；ref 同步 effect 也声明在 drain 之前，不会读到陈旧值。
🔴 阻塞问题：drain 过程中切换 session，剩余命令会打到新 session
清空 effect 只保护「尚未开始 drain」的队列。drain 会先同步清空 ref，再对本地数组逐条 await。而 sendShellCommand 是在调用时刻解析 sessionRef.current（actions.ts:1094），并非 effect 建立时绑定。因此一旦循环开始，第一条之后的命令会打到「当时」的 session —— 可能是另一个 workspace 的 daemon 和 cwd。窗口期等于队列中最慢那条命令的耗时。
已用探针测试在 PR 分支上确认：回合中排入 !first 与 !second，转 idle 后 drain 阻塞在 pending 的 first，此时把 connection.sessionId 切到 session-2，再放行 first —— 结果 PROBE calls: [["first"],["second"]]，second 在切换之后仍被派发。
建议在 drain 开始时固定 session（正文中已给出补丁），并补一条针对 drain 中途切换的回归测试。
🟠 排队的 shell 命令不可见、不可取消
普通排队消息在 ChatPane.tsx:654-656 有列表与逐条删除按钮；排队的 shell 命令只存在于 ref 中，无列表、无删除、无计数，仅有一次瞬时 toast。
具体场景：用户在长回合中输入 !rm -rf build/（正是本 PR 测试里用的命令），随后反悔按 Stop。cancel() 中止的是以 session.sessionId 为 key 的 controller（actions.ts:465），shell 用的是 ${sessionId}:shell，客户端队列更是完全不受影响。回合转 idle 后 drain 照常执行 —— 用户试图取消的命令依然运行了。
建议二选一：把排队的 shell 命令并入现有排队消息 UI，或在 cancel 时丢弃队列。另外队列无上限也无去重。
🟡 其他
setSessionListReloadToken+ 2 秒delayedReloadTimerRef逻辑与App.tsx:3171-3179完全重复，两处共用同一个 timer ref，建议抽成scheduleSessionListReload()。- catch 中的错误文案在 reject 时刻读
connectionRef.current.sessionId，可能把「命令执行失败」误报成「创建 session 失败」；建议用局部变量记录ensureSessionForPrompt()的结果。 - drain 没有 unmount 清理。
- EN/ZH 对称更新，无遗留 key；但
Messages是Record<string, MessageValue>，check-i18n只覆盖packages/cli，web-shell 的 key 漂移没有 CI 兜底。
测试覆盖
新增用例均为「有效测试」（变异可杀）。缺口：drain 中途切换 session、多条命令的顺序性（当前只测了单条）、drain 中第一条失败后第二条仍应执行。
结论
方向正确，状态机比预期更干净。合并前请修复 drain 期间的 session 固定问题，并就 Stop 与队列的语义做一个明确决定。
|
🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 2/100 轮）。改动内容与我反驳保留之处如下： Review feedback addressed🔴 Mid-drain session switch (blocking, @wenshao)Decision: Implemented. The drain effect now pins Regression test added: 🟠 Queued shell commands invisible/uncancellable (@wenshao)Decision: Implemented the minimal safe fix — drop the queue on cancel.
The broader UI visibility question (surfacing queued shell commands in the queued-messages list with per-item delete) and the unbounded/undeduplicated queue note are left as potential follow-up work — they are larger scope and not required for the safety guarantee. 🟡 Copy-paste from sendPrompt (@wenshao) + session-list reload duplication ([rc:3651416710])Decision: Implemented. Extracted 🟡 Missing
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
中文说明
已审查——无阻断问题。 建议见行内评论。
— qwen3.7-max via Qwen Code /review
|
🤖 Addressed the latest review feedback (round 3/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 3/100 轮）。改动内容与我反驳保留之处如下： Review feedback addressed[rc:3651619941]
|
|
🔀 Base updated: red check(s) [Test (ubuntu-latest, Node 22.x)] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base：红色检查 [Test (ubuntu-latest, Node 22.x)] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main，CI 将重新运行。 |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
[Critical] packages/web-shell/client/App.tsx:3884-3917 — existing Critical (comment 3651817795) still stands: the drain effect's dependency on streamingState causes self-cancellation when sendShellCommand sets promptStatus('waiting'), which changes streamingState from 'idle' to 'waiting', triggering the effect cleanup (cancelled = true). After the first command's await resolves, the loop exits and remaining queued commands are silently dropped. The FIFO drain test does not catch this because sendShellCommand is mocked with mockResolvedValue(undefined) which never triggers any promptStatus change.
— qwen3.7-max via Qwen Code /review
Review — R7 @
|
| retry submit | sendShellCommand calls |
|
|---|---|---|
| at head | false |
[["c"]] |
| line reverted | false |
[] — retry silently swallowed |
The user-visible bug it prevents: slow/hung session creation in a new task → user presses Stop → retries !cmd → without the reset, the retry is dropped with no feedback and stays dropped even once creation lands.
Drop-in regression test (red with the line reverted, green at head)
it('runs a retry submitted while session creation is still in flight after cancel', async () => {
mockConnection.sessionId = undefined;
let release!: () => void;
const gate = new Promise<void>((r) => {
release = r;
});
mockSessionActions.createSession.mockImplementation(() =>
gate.then(() => {
mockConnection.sessionId = 'session-1';
return { sessionId: 'session-1' };
}),
);
renderApp({});
await flush();
await act(async () => {
testState.latestChatEditorProps?.onSubmit('!a');
await Promise.resolve();
});
await act(async () => {
testState.latestChatEditorProps?.onCancel?.();
await Promise.resolve();
});
// Retry while creation is STILL in flight — the only state in which
// shellSubmitInFlightRef stays true unless handleCancel resets it.
await act(async () => {
testState.latestChatEditorProps?.onSubmit('!c');
await Promise.resolve();
});
await act(async () => {
release();
await Promise.resolve();
});
await flush();
expect(mockSessionActions.sendShellCommand).toHaveBeenCalledWith('c');
});🟡 Three more untested guards (surviving mutants)
| Mutation | Verdict |
|---|---|
App.tsx:3957 — make the finally lock release unconditional |
Survives. The guard is real: it stops an old drain's finally from unblocking a newer drain that took the lock after a cancel/switch. Worth a test or a comment saying it is deliberately defensive. |
App.tsx:3933 — connectionRef.current.sessionId !== drainSessionId → false |
Survives, including aborts remaining commands if the session changes mid-drain (App.test.tsx:1394). That test actually passes via the generation check, since the session-switch effect bumps it. The check looks redundant with the generation bump; either delete it or name the test for what it verifies. |
App.tsx:3915 — drop the prev === 'idle' half of the transition guard |
Survives. Plausibly redundant now that isDrainingRef + the empty-queue check exist. Either way it is unverified complexity. |
🟡 Carried over from R4 — still open
- Stop cannot abort a running shell command.
cancel()abortsactivePromptsRef.get(session.sessionId)(packages/webui/src/daemon/session/actions.ts:465), butsendShellCommandregisters its controller under`${sessionId}:shell`(actions.ts:1103-1106). Outside this diff, but this PR makes it far more reachable: Stop during a drain clears the queue yet leaves the in-flight command running. - Queued shell commands have no UI and the queue is unbounded. Regular queued prompts render in
QueuedPromptDisplay; a queued!command produces one toast and then becomes invisible — not listable, editable, or individually removable, with no cap on how many can pile up during a long turn. Given these are shell commands, "I can't see what I queued and can only discard all of it" is a sharper gap than it is for prose prompts.
Minor
- On a session switch mid-drain both the effect (
3902) and the drain (3941) can toast, giving the user two "N queued shell commands will not run." toasts with different counts. - The PR body's "Environment" line still says 173 tests; the suite is at 194.
Verdict: the headline feature and the R6 blocker are in good shape, and the mutation matrix is much stronger than in earlier rounds. Requesting changes for the disconnect drop-path leak, and asking for the handleCancel coverage gap to be closed since that commit is currently protected by a test that cannot fail.
中文说明
结论
R7 @ b0a7c519。 方法：PR head 独立 worktree，全量 client/App.test.tsx（194/194 通过），对所有新增 guard 做 15 个变异体矩阵 + 定向 A/B 探针。15 个变异体杀掉 11 个。
✅ R6 的 blocker 已修复，且覆盖真实有效
c51919b 在 App.tsx:3897 加入懒创建豁免。我重跑了 R6 变异体（把条件改成 if (true)）——被杀掉，杀它的是 runs the ! command in a new task even when the session-id render commits before attach resolves。该测试跨越了 act() 边界，因此不再对 commit/passive-effect 时序失明。blocker 确实以正确的理由关闭。
FIFO 顺序、drain 中途重读队列、重复提交守卫、cancel/切换会话时的清空、return !needsSession 均有变异覆盖；i18n 的 EN/ZH 两侧都有新 key，无残留 queue.shellBlocked。
🔴 Blocker：断连退出路径会泄漏队列并破坏 FIFO
App.tsx:3931-3943。drain 有三种退出原因：其中两种（会话切换效果、handleCancel 引起的 generation bump）会在 bump 处清空 queuedShellCommandsRef；第三种 status !== 'connected'（3934）不会。于是被 f182512 的批次重读（3950）捡起来的、drain 中途排入的命令会存活下来。
实测复现（探针输出，非推测）：回合中排入 a、b → drain 执行 a → a 运行期间用户又排入 c → 断连 → a 完成。
断开后调用 = [["a"]]
toast = [["warning","1 queued shell command will not run."]]
--- 重连 + 一次 非 idle→idle 转换 ---
最终调用 = [["a"],["c"]]
用户排的是 a, b, c：a 执行了，b 被丢弃并提示"不会执行"，而 c 之后却执行了 —— 较早的命令被丢、较晚的反而跑了，顺序错乱，且跨越了一次断连。c 也从未计入丢弃提示，导致上报数量偏少。
修复（已验证：修复前红 / 修复后绿，加上新测试后全量 195/195）：
- const dropped = batch.length - i;
+ const dropped =
+ batch.length - i + queuedShellCommandsRef.current.length;
+ queuedShellCommandsRef.current = [];回归测试见上方英文折叠块。
🟠 最新 commit 的守卫确实有用，但其测试无法发现它
删掉 handleCancel 中的 shellSubmitInFlightRef.current = false;（App.tsx:6097，即完整回退最新 commit）——194 个测试全绿，包括为它新增的那个。
原因（App.test.tsx:1930）：测试在重试之前就调用了 resolveCreate()，此时 (a) 提交的 .finally() 已清掉标志，(b) mockConnection.sessionId 已设置使重试的 needsSession 为 false —— 而守卫位于 if (needsSession) 内部，该路径根本到不了。
这一行确实有用。重试时创建仍在进行中的 A/B：
| 重试返回 | sendShellCommand 调用 |
|
|---|---|---|
| head | false |
[["c"]] |
| 回退该行 | false |
[] —— 重试被静默吞掉 |
对应的用户可见问题：新任务中会话创建缓慢/卡住 → 用户按 Stop → 重试 !cmd → 若无该重置，重试被无提示丢弃，且即使创建最终成功也不会执行。回归测试见上方英文折叠块。
🟡 另外三个未被测试覆盖的守卫（存活变异体）
App.tsx:3957把finally的锁释放改为无条件 —— 存活。该守卫是真实需要的：防止旧 drain 的finally解锁在 cancel/切换后接手的新 drain。建议补测试或加注释说明这是刻意的防御。App.tsx:3933的sessionId !== drainSessionId改为false—— 存活，连名为aborts remaining commands if the session changes mid-drain（App.test.tsx:1394）的测试也杀不掉它：该测试其实是靠 generation 检查通过的。此检查与 generation bump 重复，建议删除或改测试名。App.tsx:3915去掉prev === 'idle'—— 存活。在已有isDrainingRef和空队列检查后可能已冗余，无论如何目前未被验证。
🟡 R4 遗留、仍未解决
- Stop 无法中止正在运行的 shell 命令。
cancel()中止的是activePromptsRef.get(session.sessionId)（packages/webui/src/daemon/session/actions.ts:465），而sendShellCommand把 controller 注册在`${sessionId}:shell`（actions.ts:1103-1106）。虽在本 diff 之外，但本 PR 让它更容易被触发：drain 期间按 Stop 会清空队列，正在执行的那条却继续跑。 - 排队的 shell 命令没有 UI，队列无上限。 普通排队消息有
QueuedPromptDisplay；排队的!命令只有一个 toast，之后完全不可见——无法查看、编辑或单条删除，长回合中也没有数量上限。考虑到这些是 shell 命令，"看不见排了什么、只能全部丢弃"比对普通消息要严重得多。
次要
- 会话切换发生在 drain 中途时，效果（
3902）与 drain（3941）可能各弹一次 toast，给出两条数字不同的"N queued shell commands will not run."。 - PR 描述的 Environment 仍写 173 个测试，实际已 194。
总体： 主体功能与 R6 blocker 状态良好，变异矩阵也比前几轮扎实得多。因断连退出路径的队列泄漏请求变更，并希望补上 handleCancel 的覆盖缺口——该 commit 目前由一个不可能失败的测试"保护"。
|
🤖 Addressed the latest review feedback (round 14/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 14/100 轮）。改动内容与我反驳保留之处如下： Autofix review round — R7 feedback addressedAddressed the two change-requested items from the R7 review; deferred the 🔴 Blocker — disconnect drop-path leaked the queue and broke FIFO — FIXEDThe drain bails for three reasons. Two of them (a generation bump from the Fix in - const dropped = batch.length - i;
+ const dropped =
+ batch.length - i + queuedShellCommandsRef.current.length;
+ queuedShellCommandsRef.current = [];Added the regression test 🟠
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Not reviewed: Agent 0: Issue fidelity & root-cause ownership, Agent 4: Performance & efficiency, Agent 7: Build & test verification — its prompt was built, but no agent on record was launched with it.
— qwen3.7-max via Qwen Code /review
Review — R8 @
|
| dispatched | toast | |
|---|---|---|
head 7695a1a6 |
[["a"]] — x silently lost |
2 queued shell commands will not run. |
| only the R8 hunk reverted | [["a"],["x"]] ✅ |
1 queued shell command will not run. |
Nothing but the R8 hunk differs between those two rows, so this is a regression introduced by this commit, not a pre-existing gap. The count is wrong too: the user is told 2 commands were dropped, and one of them is the command they submitted after pressing Stop.
Probe 2 (session switch) reproduces identically and is worse — !x is queued against session-2 and is silently discarded by session-1's stale drain. Head [["a"]], reverted [["a"],["x"]].
Fix — scope the wipe to the one reason that needs it:
+ const generationChanged =
+ drainGenerationRef.current !== generation;
if (
- drainGenerationRef.current !== generation ||
+ generationChanged ||
connectionRef.current.sessionId !== drainSessionId ||
connectionRef.current.status !== 'connected'
) {
- const dropped =
- batch.length - i + queuedShellCommandsRef.current.length;
- queuedShellCommandsRef.current = [];
+ let dropped = batch.length - i;
+ if (!generationChanged) {
+ // The two generation-bump sites (session switch, cancel) wipe
+ // the queue themselves, so anything parked here was queued
+ // after the drop and is fresh user intent. A disconnect has no
+ // such wipe — clear it so a reconnect cannot resurrect a newer
+ // command behind an already-dropped older one.
+ dropped += queuedShellCommandsRef.current.length;
+ queuedShellCommandsRef.current = [];
+ }Verified with this patch applied: the R7 regression test drops the whole queue when the drain bails, preserving FIFO integrity still passes, both probes pass, and the suite is 198/198 (196 + 2 probes). eslint client/App.tsx clean.
Please land both probes as regression tests — without them nothing in the suite distinguishes "wipe on disconnect" from "wipe on every bail", which is exactly how this regression got in.
💡 Suggestions (not blocking)
- Stop cannot abort a running shell command.
cancel()readsactivePromptsRef.current.get(session.sessionId)(packages/webui/src/daemon/session/actions.ts:465), butsendShellCommandregisters its controller under`${session.sessionId}:shell`(actions.ts:1103-1106), so the lookup misses. Pre-existing and daemon-side, so out of this PR's scope — but this PR makes it far more reachable (the drain now fires shell commands automatically right after a turn), and it is precisely what holds the blocker's window open. Worth a follow-up. - Queued
!commands are invisible. Queued prompts getQueuedPromptDisplay; queued shell commands get one info toast and then nothing — no list, no way to remove one, and the queue is unbounded. The toast also fires once per command, so queueing three yields three toasts. - Double drop-toast. On a session switch or cancel mid-drain, the wipe site pushes
queue.shellDroppedand then the drain bail pushes a second one. Cosmetic, but two warnings for one event reads as a bug. - Clean elsewhere:
queue.shellDroppedis correctly pluralised in EN, present in both EN and ZH, and there is no leftoverqueue.shellBlocked. ThescheduleDelayedSessionListReloadextraction (App.tsx:3157-3164) is a behaviour-preserving refactor that correctly threads the new dep.
Verdict: request changes — one blocker (regression introduced by 7695a1a6), with a validated one-hunk fix and two red/green probes above.
中文说明
评审 — R8 @ 7695a1a6
范围： packages/web-shell/client/{App.tsx,App.test.tsx,i18n.tsx}（+1184/−23）。相对 R7 的增量仅为 7695a1a6（App.tsx +4/−1，App.test.tsx +113）。
方法： 在 PR HEAD 的独立 worktree 中运行完整 client/App.test.tsx（196/196 通过）；对 R8 与 b0a7c519 的改动做变异矩阵；新增两个 A/B 探针。
✅ R7 的两项发现均已关闭，且关闭原因正确
阻塞项（断连丢弃路径泄漏队列）已修复。 App.tsx:3936-3938 现在把仍驻留队列中的命令计入 dropped 并清空队列。变异 M1（回退该 hunk）被杀死，且恰好只由新增测试 drops the whole queue when the drain bails, preserving FIFO integrity 捕获。两个更细的变异确认两半都是承重的：M3（保留计数、去掉清空）与 M4（保留清空、去掉计数）均被杀死。
空测试问题已关闭。 R7 中 b0a7c519 往 handleCancel 加的 shellSubmitInFlightRef.current = false 能在自己的测试下存活（allows new shell commands after cancel during session creation 在重试之前就 resolve 了 session 创建，该保护分支不可达）。新增的 runs a retry submitted while session creation is still in flight after cancel 正好命中该状态：变异 M2（删除该行）现在被杀死。
🔴 阻塞项 —— R8 的清空作用于全部三种 bail 原因，但其中两种已经清空过了
App.tsx:3931-3945。drain 有三种 bail 原因。其中两种——来自 session 切换 effect（3892-3893）或 handleCancel（6095-6096）的 generation 自增——在自增处已经清空过 queuedShellCommandsRef。只有 status !== 'connected' 没有自己的清空动作，那正是 R7 的泄漏点。
因此，新加的无条件 queuedShellCommandsRef.current = [] 会连丢弃事件之后入队的命令一并吃掉——对那两种 generation 自增原因而言，那是用户新的意图，而不是旧批次的残留。
这个窗口是真实存在的，因为 Stop 无法中止正在运行的 shell 命令（见下方第一条建议）。drain 手上那条命令还在跑的时候，输入框仍处于阻塞态，用户下一条 ! 命令就会入队——然后在那条命令终于 resolve、陈旧的 drain 醒来 bail 时被销毁。
探针 1（取消）： 入队 !a、!b → idle → drain 派发 a（挂起）→ 按 Stop → a 仍在运行时提交 !x → resolve a → idle。
| 实际派发 | toast | |
|---|---|---|
HEAD 7695a1a6 |
[["a"]] —— x 被静默丢弃 |
2 queued shell commands will not run. |
| 仅回退 R8 hunk | [["a"],["x"]] ✅ |
1 queued shell command will not run. |
两行之间只有 R8 hunk 不同，所以这是本次提交引入的回归，而非既有缺口。计数也是错的：用户被告知丢了 2 条，其中一条恰恰是他们按下 Stop 之后才提交的。
探针 2（切换 session） 表现完全一致，而且更糟——!x 是针对 session-2 入队的，却被 session-1 的陈旧 drain 静默丢弃。HEAD [["a"]]，回退后 [["a"],["x"]]。
修复 —— 把清空收窄到真正需要它的那一种原因（补丁见上方英文部分的 diff）。
已验证：打上该补丁后，R7 的回归测试 drops the whole queue when the drain bails, preserving FIFO integrity 仍然通过，两个探针均通过，整套 198/198（196 + 2 个探针），eslint client/App.tsx 无告警。
建议把两个探针作为回归测试一并合入——否则测试套件里没有任何东西能区分"仅断连时清空"和"每次 bail 都清空"，这个回归正是这样溜进来的。
💡 建议（非阻塞）
- Stop 无法中止正在运行的 shell 命令。
cancel()读的是activePromptsRef.current.get(session.sessionId)（packages/webui/src/daemon/session/actions.ts:465），而sendShellCommand把 controller 注册在`${session.sessionId}:shell`下（actions.ts:1103-1106），查找必然落空。这是既有的 daemon 侧问题，不属于本 PR 范围——但本 PR 大幅提高了它的可达性（drain 现在会在回合结束后自动发起 shell 命令），而且它正是让上述阻塞项窗口一直敞开的原因。值得开一个后续 PR。 - 排队的
!命令不可见。 排队的 prompt 有QueuedPromptDisplay；排队的 shell 命令只有一个 info toast，之后就没了——没有列表、无法移除某一条，队列也没有上限。而且 toast 是每条命令弹一次，排三条就弹三次。 - 重复的丢弃 toast。 在 drain 进行中切换 session 或取消时，清空点会推一次
queue.shellDropped，随后 drain bail 又推一次。虽属观感问题，但一个事件弹两条警告看起来像 bug。 - 其余部分干净：
queue.shellDropped的英文单复数处理正确，EN/ZH 均已补齐，没有遗留的queue.shellBlocked。scheduleDelayedSessionListReload的抽取（App.tsx:3157-3164）是保持行为不变的重构，并正确补上了新的依赖项。
结论：请求修改 —— 一个阻塞项（由 7695a1a6 引入的回归），上文已给出经过验证的单 hunk 修复与两个 red/green 探针。
…M#7724) The unconditional queue wipe in the drain bail path destroyed commands queued after a cancel or session switch, because those generation-bump sites already wipe the queue themselves. Only the disconnect case lacks its own wipe, so scope the drain-bail wipe to that case alone. Adds two regression probes: commands queued after cancel and after a session switch while a drain command is still in flight must survive the stale drain's bail and be dispatched by the next drain.
|
🤖 Addressed the latest review feedback (round 15/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 15/100 轮）。改动内容与我反驳保留之处如下： R8 Review — Address Summary🔴 Blocker — unconditional queue wipe destroys post-drop commands (Fixed)Finding: The R8 commit Decision: Required — fixed. Change: Scoped the queue wipe to the one bail reason that needs it ( Added two regression probes requested by the reviewer:
💡 Suggestions (Declined)
Conflict notesNo conflicts ( Verification
中文说明R8 评审 — 处理总结🔴 阻塞项 — 无条件清空队列销毁了丢弃事件之后入队的命令（已修复）发现： R8 提交 决定： 必需项 — 已修复。 改动： 将队列清空收窄到真正需要它的那一种 bail 原因（ 新增了评审者要求的两个回归探针：
💡 建议（已拒绝）
冲突说明无冲突（ 验证
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
| if ( | ||
| generationChanged || | ||
| connectionRef.current.sessionId !== drainSessionId || | ||
| connectionRef.current.status !== 'connected' | ||
| ) { |
There was a problem hiding this comment.
[Suggestion] If sendShellCommand returns a promise that never settles (daemon process crashes mid-command, WebSocket stalls without tearing down), the drain loop is permanently stuck — isDrainingRef stays true, no new drain can start, and all subsequently queued shell commands accumulate indefinitely with no toast or log. — Concrete cost: user queues commands during a turn, daemon hangs, commands are silently lost with no operator-visible signal.
Consider adding a per-command timeout (e.g. Promise.race with a 30-second timeout) or resetting isDrainingRef when connectionRef.current.status transitions to disconnected so a reconnect can start a fresh drain.
中文说明
如果 sendShellCommand 返回的 promise 永远不会 settle（daemon 进程崩溃、WebSocket 挂起但未断开），drain 循环将永久卡住——isDrainingRef 保持为 true，新的 drain 无法启动，后续排队的命令将无限积累且无任何提示。建议添加单命令超时机制，或在连接状态变为 disconnected 时重置 isDrainingRef。
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Declined — the drain loop already checks connectionRef.current.status !== 'connected' between commands, which handles disconnection. A permanently unsettled sendShellCommand promise indicates a transport/daemon bug that should be fixed at that layer, not papered over with a UI-level timeout. A per-command timeout would also create false positives on legitimately slow commands (large builds, long-running scripts).
中文说明
拒绝 — drain 循环已在每条命令之间检查 connectionRef.current.status !== 'connected'，可以处理断开连接的情况。sendShellCommand 返回永不 settle 的 promise 表明是传输层/daemon 的 bug，应在该层修复，而非用 UI 层超时来掩盖。单命令超时还会对合法慢命令（大型构建、长时间运行的脚本）产生误报。
QwenLM#7724) connectionRef.current.sessionId is only updated at render time, but the .then() microtask after ensureSessionForPrompt() runs before React commits the setConnection state update — so the post-creation cleanup (editor clear, session-change dispatch, sidebar reload) never executed for lazily-created sessions. Have ensureSessionForPrompt() return the allocated session ID and use it directly in the submit handler. Also include the command text in drain-loop error toasts and assert the queue-path return value in the queueing test.
|
🤖 Addressed the latest review feedback (round 16/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 16/100 轮）。改动内容与我反驳保留之处如下： Review feedback addressed[Critical] Stale
|
doudouOUC
left a comment
doudouOUC
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.
Reviewed. Suggestions are inline. 1 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here.
中文说明
已审查。 建议见行内评论。 1 条建议级发现无法锚定到改动行，已丢弃；此处无需进一步处理。
— qwen3.7-max via Qwen Code /review
|
🤖 Addressed the latest review feedback (round 17/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 17/100 轮）。改动内容与我反驳保留之处如下： Review feedback addressed[rc:3654508028] Suggestion: test
|
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
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix review round — no action neededThis round had no actionable feedback on PR #7724.
No code change was made and nothing was committed. The branch is unchanged. 中文说明Autofix 审查轮次 —— 无需处理本轮针对 PR #7724 没有可处理的反馈。
未做任何代码修改，也未提交任何内容。分支保持不变。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
…ads the report, no agent (QwenLM#7751) * feat(review): add script-lint — deterministic linters over a diff's executable scripts A diff's shell — a `.sh` file, a Makefile recipe, a Dockerfile `RUN`, a GitHub Actions `run:` block — is code, and its bugs (an unquoted `$x` that word-splits, a `${PIPESTATUS[1]}` read after the array was reset) are the class a reviewer misses by reading a long YAML and catches by running the checker. Measured: a model told in prose to "run the workflow scripts" reads instead (0/4 executed). So the execution is a command, not a request. `qwen review script-lint` reads the plan, dispatches shellcheck / actionlint / hadolint by file type over the changed executable files, filters every finding to whether its line is one the diff changed (`inDiff`), and reports JSON. A linter that is not installed is disclosed as skipped, never a clean bill. It is not GitHub-specific — shellcheck applies to shell wherever it appears; actionlint/hadolint are front-ends for two embeds. This is the command only; the agent, roster requirement and coverage gate that make it a non-skippable step follow. * feat(review): make script-lint a required, coverage-gated review step The script-lint command exists; nothing ran it. This wires it into the review the way build-test is wired, so a diff that changes an executable script cannot be certified without its linters having run. - A `script-lint` agent role (agent-briefs): reads no diff, runs the command, reports from its JSON. `inDiff` findings are the PR's; `skipped` (a linter not installed) is disclosed as unreviewed, never clean. Rules are not injected into it, same as Build & Test — it reports a tool's verdict, not a read. - agent-prompt welds the exact `qwen review script-lint --plan/--worktree/--out` into that agent's brief with absolute paths, guarding an absent PR number out of the --out name — the same treatment, and the same traps avoided, as the build-test block it sits beside. - The roster requires the agent whenever the diff carries a file a linter owns by path (a `.sh`/`.bash`, a `.github/workflows/*`, a Dockerfile) and the review has a worktree to lint in. Detected by the command's own `pathTool`, so the roster and the command cannot disagree about what counts. A pure-TS diff does not require it; a diff-only review (no tree) cannot run it, so does not. - Coverage needs no new code: it derives missing roles from the roster generically (`BRIEFS[role].label`), so a required script-lint agent that did not run exit-3s check-coverage like any other. The role carries its three labels for that. End-to-end on a crafted diff (a workflow plus a `deploy.sh` with `rm -rf $TARGET`): the roster requires script-lint, and the command blocks on the SC2086 on the changed line while disclosing the workflow as skipped where actionlint is absent. SKILL.md documents the step; tests cover the roster requirement, the brief weld, and the subcommand registration. * fix(review): harden script-lint after review — context lines, fail-closed, symlinks, quoting Addresses the review findings on this PR: - inDiff was keyed off the plan's hunk ranges, which include git's three context lines, so a pre-existing diagnostic near a real change was marked this PR's and could block it. Classify off the diff's added-line ranges (context excluded), parsed from the diff; fall back to the plan hunks only when the diff is absent. - The command wrote the report to --out and printed only "Wrote ...", while the agent's brief (and the roster's generated command, which passes --out) says to read the JSON it prints. Match build-test: write the file AND always print JSON. - runTool failed open — every non-ENOENT failure (EACCES, a signal, maxBuffer, an unexpected status) fell through as empty stdout and became ok:true. Fail closed: such a run is `errored`, which forces ok:false; ENOENT alone stays "not installed". - The shebang read slurped the whole file and followed symlinks — a changed `hang.sh` -> /dev/zero would hang the reviewer. Read only regular files (lstat, no follow) and only the first block. - The welded command interpolated plan/worktree/out as bare words; a worktree path with a space would split. Quote them with shellQuotePath. - Harden the checker environment: shellcheck --norc + drop SHELLCHECK_OPTS, so a PR-controlled .shellcheckrc or inherited opts cannot suppress SC2086. - The roster required the agent for a pure-deletion .sh (a mandatory no-op); gate on added lines. Drop the Makefile-recipe claim (no detector backs it). Fix the `ok` JSDoc (info blocks too, not just error/warning). - Tests: inject the tool runner (no binary needed) to cover actionlint/hadolint normalisation, the three fail-closed paths, and the context-line classification. * feat(review): make script-lint a deterministic gate — orchestrator runs it, compose-review is the authority The review of QwenLM#7749 landed three architectural findings (#10/#11/#12): the executable-script lint was run by an AGENT, so its execution rested on the model's honor system, the model decided each finding's severity, and an uninstalled checker was only disclosed in prose. All three are the exact "a rule a model is asked to remember will eventually not be remembered" trap the feature exists to close — and a measurement on PR QwenLM#7724 confirmed it: the strongest model's Step-3 agents, given the diff and the worktree, missed all three execution-confirmed bugs, one attacker persona walking into a double-execute and declaring it correct. So take the model out of the gate entirely: - The orchestrator runs `qwen review script-lint` as a deterministic step (like presubmit) and writes the report next to the plan. No agent. - `compose-review` derives the report path from the plan and reads it as the SOLE authority: a finding on a changed line above `style` is a pre-confirmed `[lint]` Critical (needs no verifier — the tool already ran); an uninstalled or crashed checker is unreviewed scope that caps a would-be Approve; and — proof it ran — a diff that carries an executable script but produced no readable report is itself unreviewed (fail closed). A diff-only review, which has no worktree to run it, is exempt. Nothing here comes from the input JSON a model wrote. Removed the `script-lint` agent role (agent-briefs, agent-prompt weld, roster requirement) and its SKILL.md agent entry; the roster's `hasExecutableScript` is now exported as the shared predicate the gate reads. The `script-lint` command itself is unchanged — it was always the deterministic engine; only its trigger and consumer moved. * fix(review): address the second review pass on script-lint - parseFindings failed OPEN on non-empty unparseable output (a version skew or a deprecation line before the JSON) → `[]` → recorded as a clean `checked` file. Return null on that path and push it to `errored`, fail-closed like a bad exit. - hadolint had no config isolation while shellcheck gets `--norc`; a PR-controlled `.hadolint.yaml` could `ignored:` its findings away. Add `--no-config`. - pathTool recognised only `.sh`/`.bash`, but the shebang regex matches four shells — a `.ksh`/`.dash` file never required the lint. Add those extensions so the gate's owed-predicate and the command cannot disagree. - Pin `r.ok` in the not-installed test, and add a mock test for the unparseable path. * fix(review): address the deterministic-gate re-review — real correctness bugs on the write path The second review pass on the gate found real bugs, including one this PR introduced. Fixed: - hadolint has no `--no-config` flag — a prior round added it, so hadolint exited 2 (usage error) on EVERY Dockerfile and reported it `errored`. Reverted to the plain working invocation; hadolint config isolation needs a verified mechanism and is tracked separately. - actionlint's JSON anchors each diagnostic at the `run:` key line (not the changed shell line) and flattens ShellCheck severity, so a style nit read as a blocking `error` and a real finding read as pre-existing. Until that source mapping is parsed and verified, a workflow is deferred to `skipped` (unreviewed) and actionlint is never run — shellcheck still covers standalone shell. - A recognised path that `firstLineOf` refused (a `hook.sh` symlink, a fifo) was silently dropped from the report, so an empty report read as clean. It is now recorded as `skipped`. `firstLineOf` distinguishes a true deletion (skip) from an irregular file (record). - The gate's owed-predicate excluded any file with zero added lines, so a deletion-only edit that breaks a surviving `.sh` was treated as not-owed. Keyed on the post-image (`fileLines`) now: a surviving script is owed, only a true deletion is exempt. - compose-review trusted any body-Critical string containing `[lint]` as deterministic — a model-written or injected claim could launder itself past verification. Provenance decides now: only `scriptLintGate`'s own findings are deterministic (tracked by count); `[lint]` is no longer a trusted tag. - A stale report from an earlier review of an older commit could certify the current one if the lint step were skipped. The report records its `headSha` (`git rev-parse HEAD`); compose-review rejects it as stale when it disagrees with the plan's `fetchedSha`. * fix(review): third gate re-review — read the report before the owed-predicate, fail closed on all paths - The gate returned early on `hasExecutableScript` (path-only), so a shebang script the command DID lint (`.husky/pre-commit`, detected by `#!`) had its findings dropped — the gate never read the report. Read the report first; the path-predicate now only gates the no-report fail-closed case. Findings from any script the report names are processed. - The staleness guard was a no-op when `report.headSha` was absent (git unreadable): `planSha && report.headSha && …` short-circuited, so an unverifiable report certified new code. It now fails closed on a missing headSha when the plan names a commit. - The plan-parse catch failed OPEN (returned "nothing owed") while every other path fails closed. It now discloses "could not read the plan" as unreviewed. - hadolint had no config isolation. Point `HADOLINT_CONFIG` at an empty file so a PR-added `.hadolint.yaml` cannot suppress findings — an env var, benign if the tool ignores it (unlike the invalid `--no-config` flag from the last round). - `buildNote` hardcoded "not installed" for every skipped file; `skipped` now mixes reasons (missing tool, irregular file, deferred checker), so it summarises by tool and leaves the specific reason on each entry. * fix(review): R3 — deferred (non-capping) actionlint, local-flow staleness, disclosure - actionlint deferral capped EVERY workflow-touching PR (≈15% of merges): a deferred workflow went to `skipped` → unreviewed → no Approve, on a checker present but deliberately not run, which "install the tool" cannot fix. Split a third `deferred` state — disclosed in the note, but the gate never reads it, so it does not cap. `skipped`/`errored` still cap. - The staleness guard was PR-only: `fetchedSha` is written by fetch-pr, not capture-local, so a local review short-circuited the check and a stale local report could certify new code — the exact fail-open, in the local flow. capture-local now records the local HEAD as `fetchedSha`. - The local path was armed (reviewMode `local`) but SKILL's `--worktree` was unfillable there. SKILL now covers a local review (worktree = the project root) and the derived report name. - Nits: the skipped/irregular reasons no longer lead with the path (the gate prefixes it — was printing it twice); merged the two `./lib/roster.js` imports. - Tests: deferred does not cap; the staleness guard protects a local review too. * fix(review): R4 — disclose deferred lint without capping; bind report freshness to diff content B1 — the deferred checker was silent. R3 split a third `deferred` state so a workflow's embedded shell (actionlint, whose source-mapping this env can't verify) no longer caps the verdict — but the disclosure half was never wired: `scriptLintGate` returned only `{criticals, unreviewed}`, so a workflow-only PR went quiet on a file no checker examined. That is #10's original complaint ("only disclosed in prose") returning as disclosed nowhere. Add a third channel: the gate now returns `disclosed`, populated from `report.deferred`, and `composeReview` renders it in the body on every verdict — including Approve — without pushing it into `cappedBy`. SKILL.md's contract paragraph now documents the deferred state alongside unreviewed scope. B2 — the staleness guard keyed on HEAD. A local review is defined by uncommitted work, so `git rev-parse HEAD` is not a content identity for what it reviews: two reviews at the same HEAD with different working-tree content shared a `fetchedSha` and a stale clean report could certify broken code. Bind freshness to the diff's content instead: `runScriptLint` stamps the report with a sha256 of the captured diff (`diffHashOf`), and the gate re-hashes the plan's current diff and rejects a mismatch. Correct for a PR (a later commit → a different diff) and for local uncommitted work alike. `capture-local`'s `fetchedSha` plumbing is removed. Tests: the DEFERRED-only case now asserts the disclosure is present and non-capping (it previously pinned the silence). New composeReview gate tests kill the surviving mutants — the gate critical stands with no verifier (provenance, not the `[lint]` marker), and an errored checker caps a would-be Approve to Comment. New runScriptLint tests pin the diff-hash stamp. 972 review tests, ESLint --max-warnings 0, tsc, Prettier clean. * fix(review): R5 — freshness fails closed when neither side has a hash; pin config isolation The staleness guard was `report.diffHash !== planDiffHash`. When the plan names no readable diff AND the report carries no hash, both are `undefined` and `undefined !== undefined` is false — so the guard did not fire and an arbitrary hashless report was accepted as this review's, its findings promoted to `[lint]` Criticals. Every other branch of this gate fails closed; this one inverted, on the exact unverifiable case its own comment claimed to reject. Fixed with `!planDiffHash || …`, and the stale `headShaOf` JSDoc left stacked above `diffHashOf` is removed. The gate's happy-path tests were green *because* of that hole: `writePlan`/ `writeReport` (and the composeReview `gateReadyPlan`/`writeGateReport`) wrote no diff and no hash, so every test that didn't override them ran through the fail-open branch — proving the gate works on an *unverifiable* report, not a fresh one. The fixtures are now FRESH by default: a captured diff exists (`DIFF` is a real file; coverage only string-matches it, so this is transparent to it) and both plan and report bind to its hash. A freshness test overrides one side to model staleness, and a new test pins the both-undefined case directly. Also closes the last mutation gap R5 flagged three rounds running: the config isolation (`--norc`, `SHELLCHECK_OPTS` scrub, `HADOLINT_CONFIG` → empty file) is a security property — a PR-added linter config must not suppress the finding the gate blocks on — but it lived inside `runTool`, behind the spawn an injected runner bypasses. Extracted `buildToolInvocation` (pure argv + env) so all three defences are asserted without a binary; each was hand-mutated to confirm its test fails when the defence is deleted. 976 review tests, ESLint --max-warnings 0, tsc, Prettier clean. * test(review): pin the plan-parse disclosure — the last surviving gate mutant `scriptLintGate` on an unreadable plan fails closed and pushes its own reason into `unreviewed`. The coverage machinery already caps an unreadable plan, so the verdict and its cap are identical with or without this line — what it loses when deleted is the specific "could not read the plan" sentence in the body. That made it a disclosure guarantee with no test, and the one gate mutant left standing after five rounds. Pin it directly on the gate. 977 review tests, ESLint/tsc/Prettier clean. * fix(review): harden script-lint tmp-file + spawn, leak-proof the tests (R8 suggestions) Four suggestions from the re-review, all on script-lint: - Symlink race on the hadolint empty-config: it was written to a fixed `tmpdir()/qwen-review-hadolint-empty.yaml`, so on a shared runner a pre-planted symlink there would have `writeFileSync` follow it and truncate the target. Write it inside a fresh `mkdtempSync` directory instead — a 0700 dir with a random suffix that cannot pre-exist, so the write is safe and the path unpredictable. - `runTool`'s `spawnSync` had no `timeout`, unlike the sibling runners in build-test.ts and test-efficacy.ts: a crafted script that hangs a linter would block the review until the outer CI job timeout reclaimed the runner. Added a 120s bound; a timeout kills with SIGTERM, which the existing `r.signal` branch already turns into a fail-closed error. - Two test-cleanup leaks: script-lint.test.ts's symlink test made a second temp dir cleaned only by an inline `rmSync` a failing assertion would skip, and script-lint.mock.test.ts used inline `fresh()`/`clean()` with no hook. Both now tear down in `afterEach`, which runs even when a test throws. 977 review tests, ESLint --max-warnings 0, tsc, Prettier clean. * fix(review): hadolint isolation fails closed, not to a plantable path (R8) R8 caught that the previous commit's fallback reopened the vector it closed. When `mkdtempSync` failed, `emptyHadolintConfig` returned a FIXED `tmpdir()/qwen-review-hadolint-none/x`. That path is a config hadolint *reads*, so an attacker who plants an `ignored:` file there gets those ignores honoured — the opposite threat direction from the write-truncation the mkdtemp move fixed. There is now no predictable fallback: on failure `emptyHadolintConfig` returns `undefined`, the env var is set only for a hadolint run and only when a private config exists, and `runTool` fails the hadolint run CLOSED (errored) rather than lint against a config it cannot vouch for. Also from R8: - The timeout comment claimed the `r.signal` branch handles a timeout. On Node a timeout sets BOTH `r.error` (ETIMEDOUT) and `r.signal` (SIGTERM), and `r.error` is checked first — so it is reported through the error branch. Comment corrected; still fail-closed either way. - Both hardening changes were untested. `buildToolInvocation` now returns `timeoutMs` so the bound is one asserted value, and two tests pin it: HADOLINT_CONFIG is a fresh 0700 mkdtemp path (not the old fixed name), and the timeout is 120s. Each fails under the corresponding mutation. - The mkdtemp dir is swept at process exit, so the fixed-file-to-per-run-dir change does not leak into the OS tmpdir. 979 review tests, ESLint --max-warnings 0, tsc, Prettier clean. * test(review): pin the hadolint fail-closed guard in its own file (R9) The `if (tool === 'hadolint' && !emptyHadolintConfig())` guard is what the previous commit exists to add, yet deleting it left the suite green: `emptyHadolintConfig` caches at module scope, so by the time any test in script-lint.mock.test.ts runs the cache is warm and the failure path is unreachable from that file. A dedicated file gets a fresh module registry — it points TMPDIR at a path that does not exist before importing, so the first `emptyHadolintConfig()` call fails to mkdtemp and the guard fires. Two tests, split so a mutation attributes cleanly: (1) buildToolInvocation sets no HADOLINT_CONFIG (the env logic, holds either way); (2) runScriptLint errors a Dockerfile closed rather than lint it unisolated (the guard). Deleting the guard reddens (2) and leaves (1) green. Also from R9: register the process-exit cleanup right after mkdtempSync, before the write, so a writeFileSync that throws still leaves the temp dir swept, not leaked. 981 review tests, ESLint --max-warnings 0, tsc, Prettier clean. * fix(review): isolate hadolint via --config, not the ignored HADOLINT_CONFIG env (R10) Verified against real hadolint 2.14.0: the binary does not read `HADOLINT_CONFIG` (its `strings` list it among no `HADOLINT_*` config vars; `-V` reports "No configuration was specified"). It reads `--config`, then a `.hadolint.yaml` in the process CWD, then XDG. Because a local review runs with `--worktree .`, the linter ran inside the reviewed tree and honoured the diff's OWN `.hadolint.yaml` — so the env-based "isolation" was a silent no-op and a PR could suppress its own Dockerfile findings (a DL3018 `ignored:` made the finding vanish with nothing disclosed). Isolate on the channel hadolint actually reads: pass `--config <private neutral file>`, which overrides both the cwd and XDG configs. The config content is now `ignored: []` rather than empty — `--config` rejects an empty file ("empty YAML stream"). The mkdtemp 0700 dir and the fail-closed guard are kept (the config is a path hadolint reads, so both still matter); the no-op `HADOLINT_CONFIG` / `HADOLINT_NO_COLOR` env vars are dropped. Tests updated to assert the real channel: hadolint's argv carries `--config` at a fresh 0700 mkdtemp path holding `ignored: []`, and the isolation-unavailable path adds no `--config` (and still fails closed). shellcheck's `--norc` / `SHELLCHECK_OPTS` isolation is unchanged — it was verified sound against the real binary. 981 review tests, ESLint/tsc/Prettier clean. * fix(review): address maintainer review — worktree containment, hunk-fallback false positive, zsh/shebang docs From @yiliang114's review of the script-lint gate: - P1 shebang/zsh: `toolFor` intentionally omits zsh (and fish) — shellcheck refuses both (`SC1071: ShellCheck only supports sh/bash/dash/ksh`), so routing a `#!/usr/bin/env zsh` hook to it would make every zsh file a bogus SC1071 `[lint]` Critical on its shebang. Documented the deliberate exclusion rather than adding zsh. - P1 hunk fallback: `inDiff` fell back to the plan's context-inclusive hunks whenever `addedRanges.get(path)` missed — including when the diff parsed fine but a path was unmatched, which could promote a pre-existing finding on a context line to a blocker. Now a parsed diff that does not mention a path yields `[]` (nothing added → nothing blocks); the context-inclusive `hunksOf` fallback is used only when NO diff parsed at all (a report the freshness guard already rejects as stale), and that degraded path now logs a warning. - P2 worktree containment: `resolve(join(worktree, path))` now refuses a path that escapes the worktree (`../../etc/passwd`), disclosing it as skipped rather than stat-ing/linting a file outside the reviewed tree. - P2 roster/gate shebang gap: made explicit in the gate's no-report branch that a shebang-only script (which `hasExecutableScript` cannot see) is covered by the always-run contract, not the `owed` predicate. Two items left as tracked follow-ups per the reviewer's own framing (both flagged low-priority / design): an aggregate cross-file time budget (the per-file 120s bound is the main defense), and per-severity finding classification (the v1 all-or-nothing stance is documented and defensible). 983 review tests, ESLint/tsc/Prettier clean. * fix(review): harden the script-lint gate against the adversarial review round From the Codex GPT-5 /review pass. Five fixed: - Provenance by IDENTITY, not count (compose-review): the gate's `[lint]` criticals are now tracked as a separate list rather than pushed into `bodyCriticals` and removed by a count subtraction. The old `(filtered) − gateCount` misfired when a model claim carried a `[build]/[test]/[probe]` tag (filtered out before the subtract) or a gate finding's own text contained one — erasing an unrelated claim's verification requirement. Model criticals alone decide the verify count. - Distinguish unknown size from deletion (roster): `fileLines: 0` is a real post-image count only in pr-worktree; in local/diff-only the report builder writes 0 for EVERY file, so keying deletion on it read a surviving `deploy.sh` as deleted and let a missing report pass uncapped. `hasExecutableScript` now trusts `fileLines` only in pr-worktree and owes any path-detected script otherwise. - Hash the bytes used for range mapping (script-lint): the diff was read twice — ranges before the linters, hash after — a TOCTOU window where a concurrent recapture could pair snapshot A's ranges with snapshot B's hash. Read the diff once into one buffer and derive both from it; an unparseable diff now yields no `diffHash` (gate fails closed) instead of context-inclusive hunks. - Escape PR-controlled paths in the body (compose-review): a diff filename can carry a newline, `@mention`, HTML or Markdown; the gate's criticals/unreviewed/disclosed strings rendered it verbatim into the posted body. Paths and linter messages now render in an inline code span with backticks and newlines stripped (`mdField`). - Classify lstat failures (script-lint): only ENOENT/ENOTDIR is "missing" (deleted); EACCES/EIO/ELOOP is now `irregular` (skipped, fail closed) rather than silently read as a deletion into an `ok: true` "nothing changed". Three left as reasoned follow-ups: verifier-provenance for `[build]/[test]/[probe]` tags (a pre-existing verification-model concern, not this gate); actionlint's native workflow diagnostics (deferring the whole tool loses them, but separating native from embedded-shell output needs an actionlint-output parser this env can't verify); and cryptographic authentication of the report file (the orchestrator runs the deterministic command and overwrites any pre-planted file; full anti-forgery is a broader harness-trust change). 985 review tests, ESLint/tsc/Prettier clean. * fix(review): clean error from the script-lint handler on a bad plan Wrap `runScriptLint` in the command handler in try/catch, matching sibling build-test: a missing or invalid plan makes it throw, and yargs' default handler would print a stack trace the orchestrator has to parse. Emit the one-line message and a non-zero exit instead; the gate still fails closed on the absent report. * fix(review): scrub HADOLINT_* env, canonicalize worktree paths, portable isolation tests Follow-up adversarial round on the previous fixes: - Scrub HADOLINT_* from the child env (script-lint): hadolint 2.14 MERGES config from HADOLINT_IGNORE / HADOLINT_OVERRIDE_* / HADOLINT_CONFIG with the explicit --config, so an inherited one could suppress the findings the neutral config forces. Drop every HADOLINT_* var, mirroring the SHELLCHECK_OPTS scrub — configuration comes from our --config alone. Hostile-env regression test added (this also makes the HADOLINT_CONFIG assertion hermetic under an inherited value). - Canonicalize the worktree-containment check (script-lint): the lexical startsWith guard is defeated by a symlinked ANCESTOR — a directory replaced with a symlink leaves a child path lexically inside but resolving outside, and lstat/the linter follow it. Add a realpathSync check against the canonical worktree; a path that cannot be canonicalised is handled as missing downstream. Symlink-escape test added. - Portable isolation tests: override TEMP/TMP as well as TMPDIR (os.tmpdir() ignores TMPDIR on Windows, leaving the fail-closed path unexercised there), and guard the 0o700 mode-bit assertion with process.platform !== 'win32'. Left tracked: fail-closed on a PARTIALLY parsed diff — narrow (capture writes the diff in one writeFileSync, not incrementally) and already mitigated (a truncation that later completes changes the hash, so the freshness guard rejects the report). 987 review tests, ESLint/tsc/Prettier clean. --------- Co-authored-by: verify <verify@local>
|
@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. ✅
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Review at 3a1b506
Reviewed the full diff — the queue/drain machinery and the ! branch in the app component, the i18n changes, and all 24 new tests — and independently verified locally at this head: client/App.test.tsx 200/200 green.
What I checked, beyond reading the diff:
- Lazy-creation fencing. Traced the microtask/effect ordering between
clearPreparation, the submit's.thengeneration check, and the session-switch wipe effect. Both possible orderings are safe: if the wipe effect runs beforeclearPreparation, thepreparingSessionIdRefexemption skips the bump; if it runs after, the inline command has already been dispatched and the late bump is a no-op. The dangerous interleaving (bump before the generation check with the preparing ref already cleared) cannot occur — the check andclearPreparationare adjacent microtasks that a React effect cannot interleave. - Drain single-flight + generation fencing.
isDrainingRefprevents a second drain from cancelling an in-flight batch; thefinallyblock only releases the lock if the generation is unchanged, so a cancel/switch that hands the lock to a newer drain isn't unblocked prematurely. FIFO across mid-drain enqueues holds. - Stuck-drain recoverability.
sendShellCommandregisters anAbortControllerundersessionId:shell(webui session actions), so Stop always aborts a hung command; the drain also checksstatus !== 'connected'between commands. This confirms the earlier decline of the never-settling-promise thread is sound. - Cancel semantics. Stop wipes the queue with a count toast, fences the in-flight inline command via
generationAtSubmit, and resetsshellSubmitInFlightRef— and a retry submitted while the old creation is still in flight safely dedupes onto the same creation promise (covered by a dedicated test). - No dangling references.
queue.shellBlockedis fully removed; the newshellQueued/shellDroppedkeys exist in both EN and ZH. - Unresolved threads (2, both Suggestion-level, non-blocking): queue visibility/cancellation UI is documented as out of scope in the PR body and Stop provides an escape hatch; the stuck-drain concern is addressed per above.
The ensureSessionForPrompt return-type widening and the scheduleDelayedSessionListReload extraction are behavior-preserving for the existing prompt path.
No blocking issues. LGTM. ✅
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Review at 3a1b506
Reviewed the full diff — the queue/drain machinery and the ! branch in the app component, the i18n changes, and all 24 new tests — and independently verified locally at this head: client/App.test.tsx 200/200 green.
What I checked, beyond reading the diff:
- Lazy-creation fencing. Traced the microtask/effect ordering between
clearPreparation, the submit's.thengeneration check, and the session-switch wipe effect. Both possible orderings are safe: if the wipe effect runs beforeclearPreparation, thepreparingSessionIdRefexemption skips the bump; if it runs after, the inline command has already been dispatched and the late bump is a no-op. The dangerous interleaving (bump before the generation check with the preparing ref already cleared) cannot occur — the check andclearPreparationare adjacent microtasks that a React effect cannot interleave. - Drain single-flight + generation fencing.
isDrainingRefprevents a second drain from cancelling an in-flight batch; thefinallyblock only releases the lock if the generation is unchanged, so a cancel/switch that hands the lock to a newer drain isn't unblocked prematurely. FIFO across mid-drain enqueues holds. - Stuck-drain recoverability.
sendShellCommandregisters anAbortControllerundersessionId:shell(webui session actions), so Stop always aborts a hung command; the drain also checksstatus !== 'connected'between commands. This confirms the earlier decline of the never-settling-promise thread is sound. - Cancel semantics. Stop wipes the queue with a count toast, fences the in-flight inline command via
generationAtSubmit, and resetsshellSubmitInFlightRef— and a retry submitted while the old creation is still in flight safely dedupes onto the same creation promise (covered by a dedicated test). - No dangling references.
queue.shellBlockedis fully removed; the newshellQueued/shellDroppedkeys exist in both EN and ZH. - Unresolved threads (2, both Suggestion-level, non-blocking): queue visibility/cancellation UI is documented as out of scope in the PR body and Stop provides an escape hatch; the stuck-drain concern is addressed per above.
The ensureSessionForPrompt return-type widening and the scheduleDelayedSessionListReload extraction are behavior-preserving for the existing prompt path.
No blocking issues. LGTM. ✅
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
— qwen3.7-max via Qwen Code /review
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
中文说明
未发现问题。LGTM！✅
— qwen3.7-max via Qwen Code /review
|
Released in v0.21.1. |
6 participants
What this PR does
Improves the
!(shell command) experience in Web Shell in two ways:!command in a brand-new task now lazily creates a session (via the sameensureSessionForPrompt()path used by regular messages) instead of showing "No active session yet" and rejecting the input.!commands with an error toast while a turn is running, they are now queued client-side and executed sequentially after the turn finishes. The user gets an info toast confirming the command was queued.Why it's needed
Previously,
!commands had two unnecessary restrictions:!commands. Since shell commands execute via a separate API (sendShellCommand, notsubmitPrompt), they can be safely queued client-side and drained when the turn completes — no daemon-side changes needed.Reviewer Test Plan
How to verify
New task flow:
!echo helloand press Enter.helloin the chat.Mid-turn queuing:
!echo queuedand press Enter.!commands during a turn — they execute sequentially in order after the turn ends.Evidence (Before & After)
N/A (requires a running daemon to demonstrate; covered by unit tests).
Tested on
Environment (optional)
npx tsc --noEmit+npx vitest run client/App.test.tsx(173 tests passed).Risk & Scope
ensureSessionForPrompt()(createSessionPromiseRef), so rapid!commands only create one session.queuedShellCommandsRef). They are lost on page refresh or session switch — this is acceptable since they are transient, fire-and-forget commands.promptStatusclobbering, Stop-button cancellation, and interleaved output rendering).Linked Issues
N/A
中文说明
本 PR 做了什么
改进 Web Shell 中
!（shell 命令）的体验：!命令时，通过ensureSessionForPrompt()按需创建 session 后执行，不再弹出"尚无活动会话"并拒绝输入。!命令不再被拦截报错，而是排入客户端队列，回合结束后按顺序自动执行。用户会看到 info toast 确认命令已排队。为什么需要
之前
!命令有两个不必要的限制：!命令也能如此。Shell 命令走独立的 API（sendShellCommand，非submitPrompt），可以安全地在客户端排队、回合结束后 drain，无需 daemon 侧改动。审阅测试计划
如何验证
新任务流程：
!echo hello回车。hello。回合中排队：
!echo queued回车。!命令——回合结束后按顺序逐条执行。证据（前后对比）
N/A（需要运行中的 daemon 演示；已由单元测试覆盖）。
测试环境
环境（可选）
npx tsc --noEmit+npx vitest run client/App.test.tsx（173 个测试通过）。风险与范围
ensureSessionForPrompt()的去重机制（createSessionPromiseRef），连续!命令只创建一个 session。queuedShellCommandsRef）中，页面刷新或切换 session 会丢失——对于即发即忘的 shell 命令来说可以接受。promptStatus覆写、Stop 按钮联动取消、并发输出渲染）。关联 Issue
N/A