feat(web-shell): approval and ask-user dialogs as in-flow sheets; fix background-agent false failure - #9351
Merged
Merged
Conversation
added 3 commits
August 17, 2026 17:22
…out covering messages When a tool approval or ask-user-question dialog is pending, the composer and the pane status area are hidden and the dialog occupies the composer slot in flow, so the message list shrinks above it instead of being covered by a floating overlay. The dialog width now matches the chat content width. Also, while an agent's launch approval is pending, its row stays compact and is not openable (there is nothing to show yet), and the launch dialog defaults its focus to the one-shot allow instead of the reject button.
…val is pending The background-agent reconciliation probes /subagents/:callId for every active background agent. While the agent's launch approval is unanswered its subagent session cannot exist yet, so the probe 404s and, after the two-miss grace, the reconciliation marked the agent failed — painting a red error on the row while the permission dialog was still up. Skip reconciliation for callIds with an unresolved permission request, and resume probing once the permission resolves.
Collaborator
Collaborator
|
Thanks for the PR! Template looks good ✓ Problem: both halves describe observed behavior with a concrete mechanism. The floating approval overlay covers the message list while the composer stays live underneath it, and the background-agent reconciliation probes Direction: aligned — this is web-shell approval-UX polish plus a false-failure fix on the background-agent card. The reference product's CHANGELOG has no direct "in-flow approval sheet" entry, but the area is actively polished there too (approval-dialog footer fixes, composer draft preservation). One product decision worth maintainer awareness: the agent-launch dialog's default focus moves from Reject to the one-shot Allow, which changes what a quick Enter does on that dialog. The "never default to a permanent allow rule" invariant is preserved (fallback to Reject when no one-shot option exists) and both branches are pinned by tests — but it is a deliberate default change on a permission surface, so calling it out here. Size: no core-module paths (all changes under Approach: scope feels right. Hiding the composer via CSS (kept mounted so the draft survives) and converting the overlay from an absolute-positioned float into an in-flow bottom sheet is the minimal mechanism, and skipping pending-permission callIds in the reconciliation probe list — reusing the existing stale-miss cleanup — is equally minimal. One question: the Risk: no elevated risk signals — none of the changed files match the revert-correlated high-risk path set. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题： 两部分都是已观测到的行为，且机制具体：挂起的审批浮层遮挡消息列表、其下的输入区仍可输入；后台 agent 对账在启动审批未决时探测 方向： 对齐——这是 web-shell 审批 UX 打磨加后台 agent 误报修复。参考产品 CHANGELOG 没有"流内审批面板"的直接条目，但该领域同样在持续打磨（审批弹窗页脚修复、输入草稿保留）。一个值得维护者知晓的产品决策：agent 启动弹窗的默认焦点从「拒绝」改为「允许一次」，改变了快速按 Enter 的结果。"绝不默认落在永久允许规则"的不变式得到保留（无单次选项时回退到拒绝），两个分支都有测试锁定——但这是权限界面上有意的默认值变更，在此特别指出。 规模： 未触及核心模块路径（全部改动位于 方案： 范围合理。用 CSS 隐藏输入区（保持挂载以保留草稿）、把浮层从绝对定位改为流内底部面板，是最小机制；对账探测列表跳过未决权限的 callId——复用已有的过期 miss 清理——同样最小。一个疑问：SystemMessage.module.css 中 风险： 无升级风险信号——改动文件均未命中与回滚相关的高风险路径集合。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
Code reviewI read the diff against the base code. My independent take before reading it was exactly what the PR does — drop the composer out of layout (keep it mounted so the draft survives) and let the overlay flow into the freed slot, plus exclude not-yet-approved agent callIds from the reconciliation probe — so no simpler path was missed. Verified integration points:
No correctness blockers found. One open (non-blocking) question carried over from the gate: the Files changed (15 of 15 shown)
TestingThe testing evidence in this unattended run is the PR's own CI, read through the API — no PR code is built or executed here. The PR was opened ~11 minutes before this pass, so the
Not verified in this run: the visual claim (sheet sits in the composer's slot, nothing covered, width matches) and the live reconciliation behavior — the author tested on macOS via the dev server only (author's claim from the PR table, not independently re-run). Sandboxed verification would settle this: 中文说明代码审查：独立方案与 PR 做法一致（输入区退出布局但保持挂载以保留草稿，浮层流内占据空出的位置，未获批的 agent callId 从对账探测中排除），没有遗漏更简路径。已核对集成点：审批浮层本就是输入区的前一个兄弟节点，去掉绝对定位后自然贴底；宽度上限改为 测试：无人值守运行下的测试证据来自 PR 自身 CI 的 API 读取，此处不构建、不执行任何 PR 代码。PR 提交约 11 分钟， — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Confidence: 4/5 — clean, focused, well-tested change; the only reservation is that CI has not landed on the reviewed commit yet, so the approval is deferred to the green-CI path rather than posted now. Stepping back: this PR does one coherent thing — stop approval dialogs from floating over the conversation — and does it the smallest way available (CSS layout swap, composer kept mounted for the draft, one guard in the reconciliation probe list, one focus-index branch). My independent baseline before reading the diff was the same approach, so there is no simpler path being missed. The tests are the right shape: both focus-default branches, own-launch vs sub-tool approval on the agent row, hide/restore cycles on both views, and the full skip → resume → grace lifecycle for reconciliation. Removing the absolute-positioning machinery actually reduces complexity rather than adding it. The two things keeping this at 4 rather than 5: the visual half of the claim (sheet sits in the composer's slot, nothing covered, width matches) is only evidenced by the author's macOS dev-server run and is jsdom-untestable, and the Verdict: approve. The PR was opened minutes before this pass and the 中文说明置信度：4/5 —— 干净、聚焦、测试充分的改动；唯一的保留是 CI 尚未在受审 commit 上落定，因此批准走"CI 转绿后自动批准"路径，而非现在直接批准。 整体看：这个 PR 只做一件连贯的事——让审批弹窗不再悬浮遮挡对话——并且用的是最小手段（CSS 布局切换、输入区保持挂载以保留草稿、对账探测列表加一道守卫、焦点默认值加一个分支）。我在读 diff 前的独立方案与之相同，没有遗漏更简路径。测试形态正确：焦点默认的两个分支、agent 行的自身启动审批与子工具审批、两个视图的隐藏/恢复循环、对账的跳过→恢复→宽限完整生命周期。去掉绝对定位机制实际上是减少而非增加复杂度。 没到 5 分的两点：视觉效果那一半（面板贴底、无遮挡、宽度一致）只有作者的 macOS 开发服务器运行作为证据，jsdom 无法覆盖； 结论：批准。PR 提交仅数分钟， — Qwen Code · qwen3.8-max Reviewed at |
…composer-on-approval
Collaborator
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / after
Full-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
qwen-code-ci-bot
requested changes
Aug 17, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 5)": none — full chunk and all mandated reads completed within budget..
中文说明
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 5)"：none — full chunk and all mandated reads completed within budget.。
— qwen3.8-max via Qwen Code /review (v0.21.13)
…l agent rows under launch approval Address review findings on the approval-dialog and background-agent changes: - R1-1 (Critical): the pending-permission effect dependency was a fresh Set on every transcript delta, so the reconciliation effect re-ran on each streamed update, bypassing the retry backoff and re-introducing the false-failure the change set out to fix. Depend on a stable sorted-key string instead and rebuild the membership Set inside the effect. - R1-2: add a two-agent reconciliation test pinning that exclusion is per callId (the healthy sibling keeps probing while one agent is under approval). - R1-3: apply the non-openable-while-pending guard to ParallelAgentsGroup rows, which previously bypassed it when agents rendered grouped. - R1-4: add a test that an accumulated missing-agent miss is fully reset when a launch approval engages, so the post-approval grace restarts fresh.
qwen-code-ci-bot
pushed a commit
that referenced
this pull request
Aug 18, 2026
chiga0
reviewed
Aug 18, 2026
chiga0
left a comment
chiga0
left a comment
Collaborator
There was a problem hiding this comment.
Code Review Overview (AI Generated)
PR: #9351 feat(web-shell): approval/ask-user dialogs as in-flow sheets; fix background-agent false failure
Type: New Feature + Bug Fix
Change size: +768/-180 across 16 files
Findings Summary
- Critical/Major: 0 (all prior Critical findings resolved)
- Minor: 1
- Nit: 0
Cross-Validation
All four findings from the prior qwen-code-ci-bot review at SHA 064ca31a were addressed in the final commit f564463:
| Finding | Prior Review | My Assessment |
|---|---|---|
R1-1 (Critical): pendingPermissionCallIds used a fresh Set as effect dep, re-running reconciliation on every transcript delta |
064ca31a |
✅ Fixed — pendingPermissionKey is now a stable sorted join string; Set is rebuilt inside the effect |
| R1-2 (Suggestion): No two-agent test for per-callId exclusion | 064ca31a |
✅ Fixed — new test 'probes only the healthy agent while a sibling launch approval is pending' |
R1-3 (Suggestion): ParallelAgentsGroup rows not guarded while launch approval pending |
064ca31a |
✅ Fixed — approvalPending = pendingApproval?.toolCallId === agent.callId guard applied per-row; pending rows render as aria-disabled div |
| R1-4 (Suggestion): No test for grace-reset when exclusion engages mid-grace | 064ca31a |
✅ Fixed — new test 'resets accumulated missing-agent misses when an approval engages' |
Unique-1 (Minor): notFounds handler doesn't filter excluded callIds — race with a reused round |
— | New finding (see below) |
Unique-1 — Minor: notFounds handler re-adds a miss for an excluded callId when the round is reused
useMessages.ts, line 406 (pre-existing handler, not in diff):
// Current code:
for (const callId of notFounds) {
const misses = (missingAgentMissesRef.current.get(callId) ?? 0) + 1;
missingAgentMissesRef.current.set(callId, misses);
...
}Scenario: The permission block arrives while the Phase-1 reconciliation probe is still in-flight (cachedRound.processed = false). When pendingPermissionKey changes, the effect re-runs with callIds = [] (the agent is excluded). The pre-effect cleanup loop deletes the accumulated miss for callId. So far so good. But because the old round is not yet settled, roundIsReusable = true — the new effect reuses the old Promise. When the in-flight probe resolves with notFounds = [callId], Phase-2's handler runs (Phase-1's handler is suppressed by active = false), and the notFounds loop re-adds miss 1 for callId.
Result: When the approval resolves (Phase 3), missingAgentMissesRef already holds miss 1. A single post-approval 404 reaches miss 2 and crosses MISSING_BACKGROUND_AGENT_GRACE_MISSES = 2 immediately — one grace miss shorter than the intended fresh-restart. The existing R1-4 test doesn't cover this race because it uses await vi.waitFor(callCount === 1) before engaging the approval, so the old round is always processed = true by Phase 2.
Suggested fix — add a guard in the notFounds loop so excluded agents cannot accumulate misses through a reused Promise:
for (const callId of notFounds) {
if (!callIds.includes(callId)) continue; // excluded agents can't miss
const misses = (missingAgentMissesRef.current.get(callId) ?? 0) + 1;
missingAgentMissesRef.current.set(callId, misses);
...
}This is low-severity because: (a) the race window is narrow; (b) the undercount is at most 1 miss; (c) if the agent genuinely doesn't exist, it still fails correctly — just one probe sooner than designed.
Additional Audit Coverage
Areas independently checked beyond the prior review's findings:
approvalOverlayActive && mainView === 'chat'guard in App.tsx: In split viewmainView !== 'chat', so the main composer is not hidden — correct, since split-pane approvals are handled byChatPane's owncomposerHiddenwrapper. ✅getSafeDefaultIndex(options, isAgent)correctness:isAgentmoved before theuseMemohooks (minor refactor). For agent dialogs: prefersallow_once, falls back to reject, never lands onallow_always. For non-agent: existing safe-default logic unchanged. ✅callIds = []edge case in effect: When all background agents are under approval,callIdsis[],Promise.allSettled([])resolves immediately, no retries fire, no misses accumulate. ✅aria-disabledon non-interactive rows:ParallelAgentsGroupandToolLineboth usearia-disabled="true"on divs; since divs have no native focus, keyboard accessibility is preserved. ✅composerHidden=display: none(not unmount): Draft text survives the approval dialog because the composer stays mounted. ✅- CSS width change (
max-width: min(800px, var(--chat-content-width, 800px))→min(100%, var(--chat-content-width, 1000px))): Themin(100%, ...)prevents overflow in narrow viewports. Increasing the fallback from 800px to 1000px is intentional (wider default to match chat width). ✅
Final Verdict
LGTM with one Minor caveat. The core logic is well-implemented: all four Critical/Suggestion issues from the prior review are resolved with clean fixes and regression tests. The one new finding (notFounds race) is a narrow edge case with limited impact; it can be addressed as a follow-up or bundled here. Recommend merging once Unique-1 is addressed or acknowledged.
This review was generated by QoderWork AI
chiga0
previously approved these changes
Aug 18, 2026
chiga0
left a comment
chiga0
left a comment
Collaborator
There was a problem hiding this comment.
All prior Critical/Suggestion findings resolved. One Minor race condition noted (notFounds handler re-adds a miss for an excluded callId when a round is reused) — can be addressed as a follow-up. LGTM.
This review was generated by QoderWork AI
Collaborator
Author
|
@qwen-code /verify |
Collaborator
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 2262 passed · 0 failed · 2262 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：2262 通过 · 0 失败 · 2262 总计 Verification reportPR 9351 verification — merge-readyVerdict: 中文摘要
Central claim and A/BCentral claim: a background agent whose launch approval is still unanswered is excluded from the The same 7 test files (the 6 PR-touched suites plus the pre-existing
The 9 base reds, one per new behavior:
Two pre-existing tests ( Secondary claims verified by the same A/B: composer/pane-footer hidden while a dialog is pending and restored after (App/ChatPane cells), agent-launch dialog default focus on Mutation matrixOne guard reverted per row, targeted suite rerun, file restored from git HEAD with the restore verified clean. Witness:
M1/M5 are the positive controls (mutations known to be caught, caught). M2 shows the stability property is pinned hard: re-introducing a fresh-Set dependency turns 23 pre-existing reconciliation tests red, so the R1-1 fix is not silently load-bearing. Boundary probes (head)Witness:
Reviewer Test Plan walkthrough
Findings (non-blocking)
Not covered
MethodologyEnvironment: the CI Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
…composer-on-approval
qwen-code-ci-bot
pushed a commit
that referenced
this pull request
Aug 18, 2026
chiga0
previously approved these changes
Aug 18, 2026
chiga0
left a comment
chiga0
left a comment
Collaborator
There was a problem hiding this comment.
Re-approval after merging main.
All 7 core feature files (useMessages.ts, ToolGroup.tsx, ParallelAgentsGroup.tsx, ToolApproval.tsx, ChatPane.module.css, AskUserQuestion.module.css, ToolApproval.module.css) are byte-for-byte identical to the f564463 commit I reviewed and approved earlier. The merge brought in only NewSessionDotField / SpecularComposerEffect removals and the animatePlaceholder prop removal from the now-merged #9349 — none of which touch the approval-dialog or background-agent logic.
Previous findings stand: all four bot findings (R1-1 stable dep key, R1-2 two-agent test, R1-3 parallel-group guard, R1-4 grace-reset test) confirmed resolved. The notFounds handler race (narrow undercount window, at most 1 miss) noted as an existing acceptable trade-off.
✅ Approve.
yiliang114
previously approved these changes
Aug 18, 2026
yiliang114
left a comment
yiliang114
left a comment
Collaborator
There was a problem hiding this comment.
LGTM — approving at head 07fcecb.
- Verified the four earlier findings are addressed at this head: R1-1 (the reconciliation effect now depends on the stable
pendingPermissionKeystring instead of a freshSet), R1-2 (two-agent mixed case pins per-callId exclusion), R1-3 (ParallelAgentsGrouprows render non-interactive while their launch approval is pending), R1-4 (miss grace resets when exclusion engages, covered by the pre-miss → approval → post-approval test). - Exclusion is per callId: a sub-tool approval does not exclude a running agent, and miss cleanup keeps the grace semantics consistent. Composer stays mounted while hidden, so drafts survive the sheet.
- Web-shell unit, E2E smoke and visual checks are green on this head.
qwen-code-ci-bot
requested changes
Aug 18, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/web-shell/client/components/messages/ToolGroup.tsx:1238 — [review] the inline-mode branch of the ToolLine pending guard is untestedpackages/web-shell/client/components/messages/ToolGroup.tsx:1215 — [probe] pending-row button→div swap drops keyboard focus to document.body in split viewpackages/web-shell/client/components/messages/ToolGroup.tsx:1188 — [probe] the pending-launch guard is bypassed on nested agent rows because approval is never forwarded into SubAgentPanel/SubToolLine
中文说明
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 3 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
- useMessages: pace the missing-agent grace by wall clock so a re-probe triggered by an unrelated permission change cannot collapse the retry ladder into two immediate misses, and never count misses or retry-budget errors for agents excluded by a pending permission (an in-flight round settles after the exclusion cleanup ran and would otherwise re-add them) - App.module.css: keep the approval sheet above portal-hosted DialogShell modals by restoring its z-index while preserving the in-flow layout - ParallelAgentsGroup: the pending-approval row keeps its status dot, shows a pending label instead of running, and is inert (no pointer, no hover highlight, no rowActive sweep); add a regression test for the inert row
qwen-code-ci-bot
pushed a commit
that referenced
this pull request
Aug 18, 2026
qwen-code-ci-bot
previously requested changes
Aug 18, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
packages/web-shell/client/hooks/useMessages.ts:530 — [probe] A newly approved agent inherits siblings' escalated backoff ladder (retryScopeKey is unchanged by approval transitions)packages/web-shell/client/hooks/useMessages.ts:530 — [probe] pendingPermissionKey collects ALL unresolved permissions (any tool kind), so unrelated approvals fire immediate reconciliation probe bursts
中文说明
收敛姿态下延后（第 3 轮，非阻断）——已记录，本轮不要求修改：共 2 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
qwen-code-ci-bot
pushed a commit
that referenced
this pull request
Aug 19, 2026
chiga0
approved these changes
Aug 19, 2026
chiga0
left a comment
chiga0
left a comment
Collaborator
There was a problem hiding this comment.
Code Review Overview (AI Generated)
PR: #9351 feat(web-shell): approval/ask-user dialogs as in-flow sheets; fix background-agent false failure
Type: New Feature + Bug Fix
Change size: +1079/-182 across 18 files
HEAD: 2c360d3b04
Findings Summary
- Critical/Major: 0
- Minor/Nit: 2 (new, noted below — non-blocking)
Key Observations
All 13 prior Critical/Suggestion findings from three bot-review rounds (R1-1 through R3-3) are confirmed resolved at HEAD. The wall-clock pacing (missTimestampsRef / errorTimestampsRef), straddle-window guards, and stable dep-key fix together close a set of subtle race conditions in the reconciliation loop. The composer-hide approach (display:none, kept mounted) correctly preserves form drafts, and the in-flow bottom-sheet layout is a clean improvement over the previous floating overlay.
Cross-Validation
| Finding | Prior Review | My Assessment |
|---|---|---|
R1-1: Set dep re-ran effect on every delta |
bot R1 | ✅ Confirmed fixed — pendingPermissionKey stable sorted string |
| R1-2: No two-agent per-callId exclusion test | bot R1 | ✅ Confirmed fixed — sibling-probe test added |
R1-3: ParallelAgentsGroup missing pending guard |
bot R1 | ✅ Confirmed fixed — non-interactive div path added |
| R1-4: No grace-reset test | bot R1 | ✅ Confirmed fixed — miss-reset test added |
R2-1: straddle notFounds unchecked against callIds |
bot R2 | ✅ Confirmed fixed — if (!callIds.includes(callId)) continue |
R2-2: approvalOverlay z-index lost on in-flow conversion |
bot R2 | ✅ Confirmed fixed — position: relative; z-index: calc(...) retained |
| R2-3: every permission change fired immediate probe burst | bot R2 | ✅ Confirmed fixed — missTimestampsRef wall-clock gate |
R2-4: ParallelAgentsGroup pending guard untested |
bot R2 | ✅ Confirmed fixed — keeps the pending-approval row inert test |
R2-5: .row[aria-disabled] missing cursor/hover suppression |
bot R2 | ✅ Confirmed fixed — ParallelAgentsGroup.module.css updated |
R2-6: pending row showed running label instead of pending |
bot R2 | ✅ Confirmed fixed — statusLabel = t('subagent.pending') |
R3-1: permanent 4xx straddle unfiltered from resolutionSnapshot |
bot R3 | ✅ Confirmed fixed — resolutions.delete(callId) for excluded callIds |
R3-2: errorAttempts not wall-clock paced |
bot R3 | ✅ Confirmed fixed — errorTimestampsRef same pacing as missTimestamps |
| R3-3: no in-flight-round straddle test | bot R3 | ✅ Confirmed fixed — does not count a late 404 after approval engages |
Nit-1 (new): ToolGroup.module.css not updated with [aria-disabled='true'] cursor reset for .lineButton |
— | New — the R2-5 fix landed in ParallelAgentsGroup.module.css; the mirror <div className={lineButton} aria-disabled="true"> in ToolGroup.tsx (subagentDetails path) still shows pointer cursor on hover |
Nit-2 (new): non-subagentDetails path in ToolGroup.tsx pending div has no aria-disabled attribute |
— | New — subagentDetails path correctly sets aria-disabled="true"; the inline-expand path omits it (behaviour is correct, ARIA signal is missing) |
Additional Audit Coverage
Areas checked beyond existing findings:
- CSS
max-widthfallback change (800→1000px):min(100%, ...)prevents overflow; the wider default is intentional and safe. getSafeDefaultIndexedge case: noallow_once+ no reject → fallback to index 0 (could beallow_always). Edge case is practically unreachable for agent dialogs.pendingPermissionKeysort stability:.sort().join('|')is order-independent — correct.- Three-ref sync on connection reset (
missingAgentMissesRef,missTimestampsRef,errorTimestampsRef): all three cleared together — no orphan state.
Deferred (pre-existing, non-blocking)
ToolGroup.tsx:1238— inline-mode pending guard untestedToolGroup.tsx:1215— button→div swap may drop keyboard focus todocument.bodyToolGroup.tsx:1188— pending guard not forwarded into nestedSubAgentPaneluseMessages.ts:530—retryScopeKeyunchanged on approval transition; newly approved agent inherits siblings' backoff ladderuseMessages.ts:530—pendingPermissionKeycollects all tool permissions, so any non-agent approval triggers a reconciliation probe burst
Final Verdict
✅ Approve. All Critical findings resolved; test coverage is thorough (8+ new regression tests). Two new Nits (ToolGroup.module.css cursor, non-subagentDetails aria-disabled) are non-blocking and consistent with the existing deferred accessibility items — suitable for a follow-up.
This review was generated by QoderWork AI
yiliang114
approved these changes
Aug 19, 2026
qqqys
added a commit
to qqqys/qwen-code
that referenced
this pull request
Aug 19, 2026
Resolve the ChatPane composer conflict with QwenLM#9351: keep upstream's approval-owns-the-footer wrapper and take the Goal composer-status stack (queued prompts plus the Goal strip) inside it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
|
Released in v0.21.15. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Reworks the tool-approval and ask-user-question dialogs so they no longer cover the message list: while either dialog is pending, the composer (and the split-pane status area) drops out of layout and the dialog takes its slot as an in-flow bottom sheet, with the width aligned to the chat content width. The agent-launch dialog now defaults its focus to the one-shot allow instead of the reject button, and an agent row whose launch approval is still pending stays compact and is not openable.
Also fixes a false failure: the background-agent reconciliation probes
/subagents/:callIdfor every active background agent, and while the launch approval is unanswered that session cannot exist yet — the repeated 404s crossed the missing-agent grace and marked the agent failed while the permission dialog was still up. Agents with an unresolved permission request are now skipped until the approval resolves.Why it's needed
A pending approval left the input area live and floated over the conversation, hiding messages and inviting a stray keystroke; the dialog width also disagreed with the message body. Meanwhile a subagent call under approval showed a red error icon purely because the reconciliation could not find a session that legitimately does not exist yet.
Reviewer Test Plan
How to verify
/subagentsprobe must not fire (or must not mark the agent failed); after approval it resumes and a genuinely missing session still fails after the two-miss grace.Evidence (Before & After)
Before: the floating dialog covered the message list with the composer still live, and an agent under launch approval showed a red error icon. After: the dialog is an in-flow bottom sheet, the composer is hidden while it is up, and the pending agent stays pending.
Tested on
Environment (optional)
Local dev via
npm run dev:daemon(vite dev server), unit suites via vitest.Risk & Scope
Linked Issues
None.
中文说明
本 PR 重构了权限弹窗与 AskUserQuestion 弹窗的展示方式，使其不再遮挡消息列表：任一弹窗挂起时，输入区（含分屏状态区）退出布局，弹窗以流内底部面板占据其位置，宽度与消息主体对齐。agent 启动弹窗的默认焦点从「拒绝」改为「允许一次」；启动审批尚未通过时，agent 行保持紧凑且不可点击展开。
同时修复一个误报：后台 agent 对账会对每个活跃后台 agent 探测
/subagents/:callId，而启动审批尚未通过时该会话本就不存在 —— 连续 404 越过缺失宽限后被标记为失败，在权限弹窗仍挂起时就显示了红色错误图标。现在带未决权限请求的 agent 会被跳过，直到审批解析。验证方式：主视图触发权限/提问弹窗时输入区隐藏、弹窗贴底不遮挡、宽度与消息一致，解析后输入区原样恢复（草稿保留）；分屏下 pane 的状态与输入区同样隐藏；ask 模式调用 agent 时，审批挂起期间行保持 pending、无红图标、不可点击，弹窗默认焦点为「允许一次」；后台 agent 审批挂起期间不触发
/subagents探测，批准后恢复轮询，真正缺失的会话仍按两次宽限转失败。