feat(cli): wire background shells into combined Background tasks dialog#3720
Merged
Conversation
Phase B follow-up #2: surface managed background shells in the same overlay that already shows local subagents, so users get one unified view instead of having to remember /tasks for shells. - BackgroundShellRegistry: add setRegisterCallback/setStatusChangeCallback and requestCancel(id), mirroring BackgroundTaskRegistry's contract. register() also fires statusChange so subscribers see the lifecycle start, not just transitions. - useBackgroundTaskView: subscribe to both registries, merge entries by startTime, attach a `kind` discriminator (DialogEntry union) so renderers can dispatch on agent vs shell. - BackgroundTasksPill: group running counts by kind ("2 shells, 1 local agent"); when all entries are terminal, collapse to "N task(s) done". - BackgroundTasksDialog: replace per-kind section header with a single "Background tasks" header; ListBody renders shell rows as "[shell] <command>"; DetailBody dispatches to AgentDetailBody (the original) or a new ShellDetailBody (cwd / output file / pid / exit). - Context cancelSelected switches by kind: agents go through cancel(), shells through requestCancel() — only aborts, lets the spawn settle path record the real terminal state (mirrors task_stop in #3687). Tests: 8 pill cases (singular/plural per kind, mixed, terminal-only), 4 dialog cases (auto-fallback on running→terminal, cancel flow, already-terminal stays in detail, selectedIndex clamp); shell registry gains 5 callback tests + 3 requestCancel tests.
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. |
wenshao
commented
Apr 29, 2026
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
[Suggestion] packages/cli/src/ui/components/background-view/BackgroundTasksDialog.tsx still renders the non-empty list section header as Local agents, even though entries can now contain both agents and shells and the displayed count includes both kinds. When shell rows are present, this mislabels the unified Background tasks list. Please rename that non-empty section header to a combined label such as Background tasks, matching the empty-state branch and the PR's unified model.
— gpt-5.5 via Qwen Code /review
useBackgroundTaskView shallow-copies agent entries into DialogEntry so each entry can carry a `kind` discriminator. The copy detaches `recentActivities` from the registry: BackgroundTaskRegistry.appendActivity mutates `entry.recentActivities = next` on the registry object and emits `activityChange`, but the dialog's activity callback only bumps a local counter — so the snapshot's `recentActivities` reference goes stale and the Progress block keeps rendering the old array until the next status-driven refresh. Resolve `selectedEntry` against the registry on each render when the selected entry is an agent, with `activityTick` as a useMemo dep so it recomputes on every activity callback. Snapshot remains the source of truth for the list (no churn on the pill / AppContainer); only the detail body re-reads live. Also rename the non-empty list section header from "Local agents" to "Background tasks" to match the empty-state branch and the unified multi-kind contents.
Collaborator
Author
|
Both review notes addressed in 08e75f2:
Tests still pass (4 dialog + 8 pill + 22 shell-registry); lint / typecheck clean. |
Collaborator
Author
|
No issues found in the latest incremental review. LGTM! ✅ — gpt-5.5 via Qwen Code /review |
tanzhenxin
approved these changes
Apr 29, 2026
tanzhenxin
left a comment
Collaborator
There was a problem hiding this comment.
Review
Cleanly extends the existing combined Background tasks dialog to surface managed background shells alongside subagents, mirroring the established BackgroundTaskRegistry callback contract on the shell side. The architecture is sound and the trade-offs are documented honestly.
One small follow-up: [shell] ${entry.command} has no newline normalization or width clamp, so multi-line commands (heredocs, multi-statement scripts) can break the one-row-per-entry windowing math. The agent path already truncates via buildBackgroundEntryLabel; same treatment for shells would bring them to parity.
Verdict
APPROVE
5 tasks
6 tasks
wenshao
added a commit
that referenced
this pull request
May 3, 2026
…#3791) * feat(cli): wire Monitor entries into combined Background tasks dialog Phase C mirror follow-up of Issue #3634, structurally a clean repeat of #3720 but for Monitor (third consumer of the kind framework). Core (MonitorRegistry): - Add `setStatusChangeCallback` mirroring `BackgroundShellRegistry` / `BackgroundTaskRegistry`. Fires synchronously inside `register()` (so subscribers see lifecycle start) and inside `settle()` (so subscribers see every running → terminal transition: complete / fail / cancel / emitEvent's auto-stop at maxEvents). - Subscriber failures are caught and logged but do not poison the registry — same defensive contract as the other two registries. CLI: - `useBackgroundTaskView` subscribes to all three registries (agent + shell + monitor) and merges by `startTime`. `DialogEntry` union extended with `(MonitorEntry & { kind: 'monitor' })`. `entryId` switches over the three kinds and returns the right id field. - `BackgroundTasksPill.getPillLabel` adds monitor to its KIND_NAMES table; grouping order is shell → agent → monitor (monitor last because it tends to be the longest-lived, least urgent to glance at). - `BackgroundTasksDialog`: - `rowLabel` returns `[monitor] <description>` for monitor rows. - New `MonitorDetailBody` showing command / status / pid / event count / dropped lines. No Progress block (monitors don't fire activity callbacks per-event). - DetailBody dispatcher gains the monitor branch. - `BackgroundTaskViewContext.cancelSelected` routes monitor cancels via `monitorRegistry.cancel(monitorId)`. Monitor's cancel is synchronous (settle + abort happen inside the registry), matching the same path task_stop already uses. Tests: 6 new core tests (registry callbacks fire on register / each terminal transition / not on emitEvent / on auto-stop / clear stops notifications / subscriber-failure isolation), 4 new pill tests (singular / plural / 3-kind grouping / running-only filter), dialog mock extended with `getMonitorRegistry`. * fix(cli): add exhaustive default arms to DialogEntry switches ESLint's `default-case` rule requires every switch to have a default arm even when TypeScript can prove the union is exhaustive. Add `default: { const _exhaustive: never = entry; throw ... }` to the five switches added in this PR — same pattern keeps both the runtime guard and the compile-time exhaustiveness check. * fix(core): fire statusChange in MonitorRegistry.reset() The newly-added `setStatusChangeCallback` subscriber misses `reset()`, so a `/clear` or session reset leaves stale monitor rows visible in the combined Background tasks dialog until an unrelated register/settle event happens. Both BackgroundShellRegistry and BackgroundTaskRegistry already fire statusChange on their reset paths — Monitor was the outlier. Fix: fire `statusChange()` (no arg) after `monitors.clear()`, with an early return when the registry is already empty so we don't notify on a no-op reset. Two new tests cover both branches. * fix(cli,core): address PR 3791 review feedback Four review threads from /review's second pass on top of f26b700: 1. **MonitorDetailBody live counters were stale.** `eventCount` and `droppedLines` came from the `useBackgroundTaskView` snapshot, which only refreshes on `statusChange`, and `emitEvent` deliberately doesn't fire `statusChange` (the per-event churn would burn the pill / AppContainer). Fix: extend the existing `selectedEntry` useMemo to re-resolve monitors from `monitorRegistry.get()` on each `activityTick`, mirroring the agent path. The pre-existing 1s wall-clock interval already drives the recompute while the entry is running, so no new callback wiring is needed. 2. **Settle reasons weren't persisted on `MonitorEntry`.** `fail()`, `complete(exitCode)`, max-events auto-stop, and idle-timeout all passed their reason strings only to the chat-history terminal notification — opening the dialog detail view after the monitor died showed the bare status with no clue why. Add `exitCode?` and `error?` fields (mirrors `BackgroundShellEntry`); populate them before `settle()` in each path; render them in `MonitorDetailBody` with appropriate colour (red for `failed`, warning for auto-stopped `completed`). 3. **`monitorCancel` mock had no test asserting it.** Add a dedicated test that opens detail on a monitor entry, presses `x`, and verifies `monitorRegistry.cancel(monitorId)` was called and the agent registry's cancel was NOT called. Pins the kind switch in `cancelSelected` so a regression flipping the monitor branch to anything else (e.g. `requestCancel`) would fail loudly. 4. **`MonitorStatusChangeCallback` docstring was wrong.** It claimed the callback fires on running → terminal transitions, but `register()` (nothing → running) and `reset()` (mass clear, fired with no entry) also fire it. Rewrite the docstring to enumerate the actual fire sites and explicitly note that `emitEvent` is excluded (per-event churn). * docs(cli,core): tighten MonitorEntry.error and rowLabel comments Two doc-precision fixes from copilot's PR 3791 review pass: - `MonitorEntry.error` enumerated max-events as the only auto-stop reason that populates the field, but `idle-timeout` also writes it (was added in the same earlier commit). Rewrote to list both current reasons and explicitly note: any future auto-stop reason should populate this field too. Also clarified that `cancelled` and natural-exit `completed` paths intentionally don't. - `rowLabel`'s shell-branch comment claimed long commands are "truncated by the row renderer's MaxSizedBox", but ListBody renders rows with plain `<Text>` (no MaxSizedBox / truncation helper). Long text wraps. Rewrote to describe actual behaviour and note the intentional decision to leave it wrapping (the dialog is what users open to see context — truncating defeats the purpose). * test(cli): cover MonitorDetailBody render branches + useBackgroundTaskView Two coverage gaps from /review's third pass on PR 3791: - New file `useBackgroundTaskView.test.ts` (6 cases) directly exercises the merge logic with a stub config: empty when config is null, merges three registries, sorts by startTime, tags `kind`, subscribes on mount, refreshes when any registry fires statusChange, clears all three subscriptions on unmount. - New `MonitorDetailBody render branches` describe block in `BackgroundTasksDialog.test.tsx` (8 cases) covers the conditional pieces my last commit added: title + Command, pid show/hide, eventCount singular vs plural, droppedLines show/hide, exitCode display, Error block (failed) vs Stopped because (auto-stop), and the all-undefined no-block case.
4 tasks
wenshao
added a commit
that referenced
this pull request
May 3, 2026
) * feat(cli): include monitors in /tasks + add interactive-mode hint Phase B closure for Issue #3634. Two coupled changes to /tasks: 1. **Bug fix — include monitors.** The command was last touched before #3684 / #3791 landed, so it merged only agent + shell entries while monitors silently disappeared from the headless / non-interactive / ACP listing path. Add a third registry pull from `getMonitorRegistry()` and wire monitor through statusLabel / taskLabel / taskId / taskOutputPath. Status line includes eventCount (`running (N events)`, `completed (exit 0, N events)`, `completed (Max events reached, N events)` for auto-stop) and pid where defined. 2. **Soft deprecation hint, scoped to interactive mode only.** Once the richer Ctrl+T dialog (#3488 + #3720 + #3791) is available, the text dump is the long-form fallback rather than the primary surface. Show `Tip: Ctrl+T opens the interactive Background tasks dialog with detail view + live updates.` at the top of the output when `executionMode === 'interactive'`. Headless / ACP get the bare list — they have no dialog to point at and the hint would just clutter. Description string also clarified to call out the modal split. Kept on all three executionModes (no deletion) — `/tasks` is the only way headless / ACP / SDK consumers can inspect background-task state. Tests: 4 new cases in tasksCommand.test.ts cover monitor entry formatting (running with pid, natural completion with exitCode, auto-stop with error string, failed), the singular `1 event` form, the interactive-mode hint gating, and the cross-kind merge order. * fix(cli): address PR 3801 review — exhaustive switch + i18n + extra tests Three actionable Suggestions from /review's pass: - `taskLabel` rewritten as a `switch` with a `never`-typed `default` arm, matching the structural-safety pattern already used by `taskId`. Adding a 4th DialogEntry kind in the future will now flip both helpers to compile errors instead of letting `taskLabel` silently fall through to `entry.description` (which the new kind may not have). - Hint string wrapped in `t()` for i18n consistency with the rest of the file. The literal stays as the i18n key default, so today's output is unchanged. - Tests: cover `cancelled` monitor status (was the only one without an inline assertion) and explicit `acp` execution mode hint suppression (pins the suppression rationale so a future regression flipping the check to `!== 'non_interactive'` would fail loudly). * fix(cli): correct /tasks dialog-open hint — Ctrl+T was wrong Tmux verification on PR #3801 caught that the hint string says "Ctrl+T opens the interactive Background tasks dialog" but Ctrl+T is actually bound to the MCP tool descriptions toggle (ContextSummaryDisplay.tsx lines 110-115). The dialog opens via Down arrow on an empty composer (focuses the footer pill) followed by Enter (InputPrompt.tsx 947-968). Same misattribution slipped into PR #3791's first description and was caught + fixed there before merge — this PR carried the wrong wording forward in code. Updates four sites: - The hint string itself: "Tip: press ↓ from an empty composer then Enter to open the interactive Background tasks dialog with detail view + live updates." - The slash-command description: "interactive UI is Ctrl+T" → "interactive dialog opens via the footer pill" - Two inline comments referencing Ctrl+T as the dialog opener - The interactive-mode hint test now pins on `↓` + `Enter` and asserts `not.toContain('Ctrl+T')` so a regression to the wrong wording fails loudly. * fix(cli): address PR 3801 review — exhaustive switch consistency + path-agnostic hint Four Suggestions from the latest /review pass: - `statusLabel` rewritten as a single top-level switch with a `never`-typed default, matching `taskLabel` / `taskId` / `taskOutputPath`. The previous `if`/`if`/fallthrough form would silently apply monitor formatting to a future 4th kind. - `taskOutputPath` gained the same exhaustive default — was the only per-kind helper still relying on implicit fallthrough; would silently omit a 4th-kind output path while the adjacent helpers flip to compile errors. - Hint wording de-specifies the exact keystroke count: `'Tip: focus the Background tasks pill in the footer (use ↓ from an empty composer) and press Enter ...'`. Previous "press ↓ then Enter" phrasing was wrong when the Arena agent tab bar is present — `InputPrompt`'s focus chain routes Down through the tab bar first, so a single Down lands there, not on the bg pill. - Test pin tightened: `[mon_fail] failed: spawn ENOENT (0 events)` is now a full-string assertion instead of a prefix match, so a regression that drops the `(N events)` suffix from monitor's failed branch fails loudly. * fix(cli): sanitize ANSI escape sequences in /tasks output deepseek's review pass flagged that monitor description / error fields are user / process-supplied strings rendered directly to the terminal. A maliciously-crafted tool description or spawn error containing raw ANSI control sequences (clear-screen, cursor-move, colour) would otherwise reach stdout verbatim and corrupt display. Same risk applies to agent error / description and shell error / command — all already-existing renderers with the same exposure that this PR didn't introduce but inherits. So instead of per-field sprinkling, wrap the joined output once with `escapeAnsiCtrlCodes` (no-op when no control chars present, so cost is zero in the common case). One line change in the renderer covers every kind including any future one. Test pins the behaviour: a monitor entry with `\x1b[2J` / `\x1b[31m...` content produces output with no raw ESC bytes and visible escaped `�[...]` sequences. * docs(cli): tighten escapeAnsiCtrlCodes comments to match actual scope Two doc-precision Suggestions from copilot's pass on 0840e32: - Source comment claimed `escapeAnsiCtrlCodes` is "a no-op when no control chars" but it's narrower than that — it only handles sequences matched by `ansi-regex` (CSI / OSC / SGR — anything starting with ESC). Isolated C0/C1 control bytes like BEL, BS, VT pass through untouched. Updated the comment to enumerate the actual scope and call out that `node:util`'s `stripVTControlCharacters` would be needed if those become a concern. - Test comment had a literal raw ESC byte (octal 033) embedded in the source — visually showed `^[[...]` in editors that render ESC, but was a real ESC byte in the file rather than the escaped `�` form the sanitizer produces. Rewrote with a literal `�` text description so what the comment shows matches what the assertions check for. * fix(cli): broaden /tasks sanitization + tighten inner switch exhaustiveness Addresses 3 of 5 items from doudouOUC's PR 3801 review: - **Issue 1 (Low) — C0/C1 control byte gap**: switched from `escapeAnsiCtrlCodes` (only handles ESC-initiated ANSI sequences) to `stripUnsafeCharacters` (one-pass strip of ANSI + VT + C0/C1, with TAB/CR/LF preserved). The pre-existing exposure to bare BEL / BS / FF / VT bytes via shell entry strings is now closed for all three kinds. Test rewritten to cover both ANSI sequences AND bare control bytes (BEL, BS), and pins that surrounding printable text and line breaks survive. - **Issue 2 (Low) — inner status switches inconsistent**: the three inner `switch (entry.status)` blocks (agent / shell / monitor) used `case 'running': default: return 'running'` (or duplicated bodies). All three now have explicit `running` cases plus a `never`-typed default that throws — matches the outer `switch (entry.kind)` pattern and means a future status added to any of `BackgroundTaskEntry` / `BackgroundShellEntry` / `MonitorStatus` flips to a compile error here instead of silently returning `'running'`. - **Issue 5 (Nit) — beforeEach default change**: added an inline comment explaining why the test default overrides `createMockCommandContext`'s `'interactive'` default (`'non_interactive'` lets the hint-suppression assertions work without each test rebinding context). Issues 3 and 4 from the review are nits with no action needed (3 is already documented as intentional; 4 is a UX call about hint length that's better handled by user feedback than guess-tweaking). * fix(cli): bind status to local before exhaustive switch — fixes tsc build CI's `tsc --build` (full mode, vs `--noEmit` locally) caught that `switch (entry.status)` followed by a `never`-typed default reading `entry.status` doesn't compile. After the case arms exhaust the discriminated union, TS narrows `entry` itself to `never`, so the `.status` access in the default arm becomes "Property 'status' does not exist on type 'never'" + the resulting `any` value can't be assigned to `never`. Fix: bind `entry.status` to a local `status` const before the inner switch. The local stays typed as the per-kind status union and narrows correctly to `never` at the default arm — `const _exhaustive: never = status` is then `never = never`, valid. Standard exhaustive-switch-on-discriminator pattern; doesn't change runtime behavior or test surface, just gets past TS narrowing on the nested case.
5 tasks
wenshao
added a commit
that referenced
this pull request
May 4, 2026
…and the dialog (#3808) * docs(core): point background-shell guidance at both /tasks and the dialog Follow-up to PR #3801, fulfilling the "separate small PR" commitment in its description. The two model-facing strings (`shell.ts` after spawning a background shell, `task-stop.ts` after requesting cancel) referenced only `/tasks` as the inspection path, predating the interactive Background tasks dialog landing at #3488 / #3720 / #3791. Now that the dialog handles all three kinds (agent / shell / monitor), both surfaces should be visible to the LLM so it can suggest the right one based on the user's mode. Updates: - `shell.ts:865` (LLM message after `is_background: true` spawn) now surfaces both `/tasks` (text, any mode) AND the interactive dialog (footer pill + Enter, with detail view + live updates). Output file guidance retained. - `task-stop.ts:110` (LLM message after `task_stop` on a shell) same pattern: both surfaces named. - `task-stop.ts:95` comment updated to enumerate all observation paths (including the dialog). - `monitorRegistry.ts:197` comment fixed — said "/tasks dialog" which conflated two distinct surfaces. Split to "Background tasks dialog reopens or `/tasks` listings". - `backgroundShellRegistry.ts:10` (module docstring) and `:31` (shellId doc) now mention all three consumers (agent, dialog, slash command). No behavior change — pure documentation/string update. Tests untouched (none asserted on these exact strings); build + lint + 152-test core suite all clean. * docs(core): address PR 3808 review — 'captured output' + consistent ordering Three review nits: 1. (LoqU — copilot) `shell.ts:865` said the output file holds "raw content", but `shellExecutionService` runs each chunk through stripAnsi and skips non-string/binary chunks before writing. Reword to "captured output" so callers don't expect a byte-for-byte stream. 2. (LqKr — wenshao) The PR mentioned both surfaces in two different orders depending on the file: `backgroundShellRegistry.ts` listed the dialog first, while `task-stop.ts` and `shell.ts` listed `/tasks` first. Unify on the LLM-facing order — `/tasks` first, then the interactive Background tasks dialog — across all four sites. Also flips the line-31 docstring on the `shellId` field for the same reason. 3. (LqKt — wenshao, flagged for awareness only) Trim the redundant keystroke detail in shell.ts:865 to match `task-stop.ts:111`'s shorter "(footer pill + Enter)" form. Saves ~7 tokens per background shell launch in `llmContent` while still naming both surfaces. The PR description's rationale (LLM should know both surfaces exist so it can suggest the right one for the user's mode) is preserved — only the operational verbosity is trimmed. 581 tests pass; lint + typecheck clean. Pure docs / string update. * docs(core): grammar polish on PR 3808 strings Two more wording nits from copilot review: - backgroundShellRegistry.ts:10 — change "metadata the agent…need to query" to "metadata that the agent…use to query". The original phrasing reads as if the metadata itself is performing the query. - shell.ts:865 — change "Read the output file directly for the captured output." to "Read the output file directly to view the captured output." — clearer instruction to the model/user. Pure wording, no behavior change. * docs(core): grammar fix on PR 3808 monitor comment 'not visible from later Background tasks dialog reopens' read as if 'reopens' was a noun. Reword to 'not visible after reopening the Background tasks dialog or from /tasks listings'. * docs(core): round 4 wording polish on PR 3808 Four more nits from copilot: - shell.ts:865 + task-stop.ts:96,111: "footer pill + Enter" was ambiguous now that the footer renders multiple pills (background tasks vs other status indicators). Disambiguate to "focus the footer Background tasks pill, then Enter". - monitorRegistry.ts:198: re-tweak my round-3 phrasing — "after reopening the Background tasks dialog or from /tasks listings" → "in later Background tasks dialog reopens or /tasks listings". Reads as "from those surfaces" rather than "after reopening", which the reviewer found ungrammatical. - backgroundShellRegistry.ts:10,31: clarify "/tasks" as the slash command, since the codebase also uses "<projectDir>/tasks/..." on-disk paths in agent-transcript contexts. Pure wording, no behavior change. 87 affected tests pass. * docs(core): mirror /tasks + dialog guidance to monitor llmContent paths Address @doudouOUC review on PR #3808 — two Medium findings: this PR updated shell-facing strings to mention both inspection surfaces but left the parallel monitor strings without any inspection guidance, even though monitors render in the same /tasks output and the same Background tasks dialog. Restore symmetry: - monitor.ts:587-598 — append the same "/tasks (text) or the interactive Background tasks dialog (focus the footer Background tasks pill, then Enter — detail view + live updates)" sentence to the Monitor-started llmContent, mirroring shell.ts:865. - task-stop.ts:125-131 — the monitor cancellation llmContent had no guidance at all. Add the same "Final status will be visible via /tasks (text) or the interactive Background tasks dialog (focus the footer Background tasks pill, then Enter) once the process drains" line that already existed for shells at task-stop.ts:111. The (Low) commit-churn observation is a maintainer call (squash on merge); the (Info) snapshot-test gap is pre-existing and not in scope. 78 monitor + task-stop tests pass; lint + typecheck clean. * docs(core): drop drain phrasing for monitor cancel + restructure dialog comment Address PR #3808 review round 5 (doudouOUC + copilot × 2): 1. (XNoH copilot, XSBu doudouOUC — Medium) The monitor cancellation message inherited "once the process drains" from the shell branch, but `monitorRegistry.cancel()` settles synchronously — when the tool returns, the entry is already `cancelled`, not waiting on a child process. The drain qualifier is accurate for shells (which use `requestCancel()` + the AbortController and settle when the real process exits) but misleading for monitors. Reword the monitor branch in `task-stop.ts:121-130` to drop the drain phrasing and add an explanatory comment about the sync vs. async difference so future maintainers don't replicate the wording from the shell branch by reflex. 2. (XNod copilot — wording, third revision on the same comment) Restructure rather than re-litigate the preposition. The "reopens" noun framing has gone through three rounds of churn (`from later... reopens` → `after reopening...` → `in later... reopens` → and now back to `from`). Sidestep the loop by making the comment a proper sentence about WHAT the surfaces actually read: the persisted `entry.error` is the source of truth; the chat-history notification is a separate, ephemeral side channel. Avoids the noun-form "reopens" entirely. Updated test assertion to match the new "Monitor \"...\" cancelled" prefix. 51 tests pass; lint + typecheck clean.
DragonnZhang
pushed a commit
that referenced
this pull request
May 8, 2026
…#3791) * feat(cli): wire Monitor entries into combined Background tasks dialog Phase C mirror follow-up of Issue #3634, structurally a clean repeat of #3720 but for Monitor (third consumer of the kind framework). Core (MonitorRegistry): - Add `setStatusChangeCallback` mirroring `BackgroundShellRegistry` / `BackgroundTaskRegistry`. Fires synchronously inside `register()` (so subscribers see lifecycle start) and inside `settle()` (so subscribers see every running → terminal transition: complete / fail / cancel / emitEvent's auto-stop at maxEvents). - Subscriber failures are caught and logged but do not poison the registry — same defensive contract as the other two registries. CLI: - `useBackgroundTaskView` subscribes to all three registries (agent + shell + monitor) and merges by `startTime`. `DialogEntry` union extended with `(MonitorEntry & { kind: 'monitor' })`. `entryId` switches over the three kinds and returns the right id field. - `BackgroundTasksPill.getPillLabel` adds monitor to its KIND_NAMES table; grouping order is shell → agent → monitor (monitor last because it tends to be the longest-lived, least urgent to glance at). - `BackgroundTasksDialog`: - `rowLabel` returns `[monitor] <description>` for monitor rows. - New `MonitorDetailBody` showing command / status / pid / event count / dropped lines. No Progress block (monitors don't fire activity callbacks per-event). - DetailBody dispatcher gains the monitor branch. - `BackgroundTaskViewContext.cancelSelected` routes monitor cancels via `monitorRegistry.cancel(monitorId)`. Monitor's cancel is synchronous (settle + abort happen inside the registry), matching the same path task_stop already uses. Tests: 6 new core tests (registry callbacks fire on register / each terminal transition / not on emitEvent / on auto-stop / clear stops notifications / subscriber-failure isolation), 4 new pill tests (singular / plural / 3-kind grouping / running-only filter), dialog mock extended with `getMonitorRegistry`. * fix(cli): add exhaustive default arms to DialogEntry switches ESLint's `default-case` rule requires every switch to have a default arm even when TypeScript can prove the union is exhaustive. Add `default: { const _exhaustive: never = entry; throw ... }` to the five switches added in this PR — same pattern keeps both the runtime guard and the compile-time exhaustiveness check. * fix(core): fire statusChange in MonitorRegistry.reset() The newly-added `setStatusChangeCallback` subscriber misses `reset()`, so a `/clear` or session reset leaves stale monitor rows visible in the combined Background tasks dialog until an unrelated register/settle event happens. Both BackgroundShellRegistry and BackgroundTaskRegistry already fire statusChange on their reset paths — Monitor was the outlier. Fix: fire `statusChange()` (no arg) after `monitors.clear()`, with an early return when the registry is already empty so we don't notify on a no-op reset. Two new tests cover both branches. * fix(cli,core): address PR 3791 review feedback Four review threads from /review's second pass on top of f26b700: 1. **MonitorDetailBody live counters were stale.** `eventCount` and `droppedLines` came from the `useBackgroundTaskView` snapshot, which only refreshes on `statusChange`, and `emitEvent` deliberately doesn't fire `statusChange` (the per-event churn would burn the pill / AppContainer). Fix: extend the existing `selectedEntry` useMemo to re-resolve monitors from `monitorRegistry.get()` on each `activityTick`, mirroring the agent path. The pre-existing 1s wall-clock interval already drives the recompute while the entry is running, so no new callback wiring is needed. 2. **Settle reasons weren't persisted on `MonitorEntry`.** `fail()`, `complete(exitCode)`, max-events auto-stop, and idle-timeout all passed their reason strings only to the chat-history terminal notification — opening the dialog detail view after the monitor died showed the bare status with no clue why. Add `exitCode?` and `error?` fields (mirrors `BackgroundShellEntry`); populate them before `settle()` in each path; render them in `MonitorDetailBody` with appropriate colour (red for `failed`, warning for auto-stopped `completed`). 3. **`monitorCancel` mock had no test asserting it.** Add a dedicated test that opens detail on a monitor entry, presses `x`, and verifies `monitorRegistry.cancel(monitorId)` was called and the agent registry's cancel was NOT called. Pins the kind switch in `cancelSelected` so a regression flipping the monitor branch to anything else (e.g. `requestCancel`) would fail loudly. 4. **`MonitorStatusChangeCallback` docstring was wrong.** It claimed the callback fires on running → terminal transitions, but `register()` (nothing → running) and `reset()` (mass clear, fired with no entry) also fire it. Rewrite the docstring to enumerate the actual fire sites and explicitly note that `emitEvent` is excluded (per-event churn). * docs(cli,core): tighten MonitorEntry.error and rowLabel comments Two doc-precision fixes from copilot's PR 3791 review pass: - `MonitorEntry.error` enumerated max-events as the only auto-stop reason that populates the field, but `idle-timeout` also writes it (was added in the same earlier commit). Rewrote to list both current reasons and explicitly note: any future auto-stop reason should populate this field too. Also clarified that `cancelled` and natural-exit `completed` paths intentionally don't. - `rowLabel`'s shell-branch comment claimed long commands are "truncated by the row renderer's MaxSizedBox", but ListBody renders rows with plain `<Text>` (no MaxSizedBox / truncation helper). Long text wraps. Rewrote to describe actual behaviour and note the intentional decision to leave it wrapping (the dialog is what users open to see context — truncating defeats the purpose). * test(cli): cover MonitorDetailBody render branches + useBackgroundTaskView Two coverage gaps from /review's third pass on PR 3791: - New file `useBackgroundTaskView.test.ts` (6 cases) directly exercises the merge logic with a stub config: empty when config is null, merges three registries, sorts by startTime, tags `kind`, subscribes on mount, refreshes when any registry fires statusChange, clears all three subscriptions on unmount. - New `MonitorDetailBody render branches` describe block in `BackgroundTasksDialog.test.tsx` (8 cases) covers the conditional pieces my last commit added: title + Command, pid show/hide, eventCount singular vs plural, droppedLines show/hide, exitCode display, Error block (failed) vs Stopped because (auto-stop), and the all-undefined no-block case.
DragonnZhang
pushed a commit
that referenced
this pull request
May 8, 2026
) * feat(cli): include monitors in /tasks + add interactive-mode hint Phase B closure for Issue #3634. Two coupled changes to /tasks: 1. **Bug fix — include monitors.** The command was last touched before #3684 / #3791 landed, so it merged only agent + shell entries while monitors silently disappeared from the headless / non-interactive / ACP listing path. Add a third registry pull from `getMonitorRegistry()` and wire monitor through statusLabel / taskLabel / taskId / taskOutputPath. Status line includes eventCount (`running (N events)`, `completed (exit 0, N events)`, `completed (Max events reached, N events)` for auto-stop) and pid where defined. 2. **Soft deprecation hint, scoped to interactive mode only.** Once the richer Ctrl+T dialog (#3488 + #3720 + #3791) is available, the text dump is the long-form fallback rather than the primary surface. Show `Tip: Ctrl+T opens the interactive Background tasks dialog with detail view + live updates.` at the top of the output when `executionMode === 'interactive'`. Headless / ACP get the bare list — they have no dialog to point at and the hint would just clutter. Description string also clarified to call out the modal split. Kept on all three executionModes (no deletion) — `/tasks` is the only way headless / ACP / SDK consumers can inspect background-task state. Tests: 4 new cases in tasksCommand.test.ts cover monitor entry formatting (running with pid, natural completion with exitCode, auto-stop with error string, failed), the singular `1 event` form, the interactive-mode hint gating, and the cross-kind merge order. * fix(cli): address PR 3801 review — exhaustive switch + i18n + extra tests Three actionable Suggestions from /review's pass: - `taskLabel` rewritten as a `switch` with a `never`-typed `default` arm, matching the structural-safety pattern already used by `taskId`. Adding a 4th DialogEntry kind in the future will now flip both helpers to compile errors instead of letting `taskLabel` silently fall through to `entry.description` (which the new kind may not have). - Hint string wrapped in `t()` for i18n consistency with the rest of the file. The literal stays as the i18n key default, so today's output is unchanged. - Tests: cover `cancelled` monitor status (was the only one without an inline assertion) and explicit `acp` execution mode hint suppression (pins the suppression rationale so a future regression flipping the check to `!== 'non_interactive'` would fail loudly). * fix(cli): correct /tasks dialog-open hint — Ctrl+T was wrong Tmux verification on PR #3801 caught that the hint string says "Ctrl+T opens the interactive Background tasks dialog" but Ctrl+T is actually bound to the MCP tool descriptions toggle (ContextSummaryDisplay.tsx lines 110-115). The dialog opens via Down arrow on an empty composer (focuses the footer pill) followed by Enter (InputPrompt.tsx 947-968). Same misattribution slipped into PR #3791's first description and was caught + fixed there before merge — this PR carried the wrong wording forward in code. Updates four sites: - The hint string itself: "Tip: press ↓ from an empty composer then Enter to open the interactive Background tasks dialog with detail view + live updates." - The slash-command description: "interactive UI is Ctrl+T" → "interactive dialog opens via the footer pill" - Two inline comments referencing Ctrl+T as the dialog opener - The interactive-mode hint test now pins on `↓` + `Enter` and asserts `not.toContain('Ctrl+T')` so a regression to the wrong wording fails loudly. * fix(cli): address PR 3801 review — exhaustive switch consistency + path-agnostic hint Four Suggestions from the latest /review pass: - `statusLabel` rewritten as a single top-level switch with a `never`-typed default, matching `taskLabel` / `taskId` / `taskOutputPath`. The previous `if`/`if`/fallthrough form would silently apply monitor formatting to a future 4th kind. - `taskOutputPath` gained the same exhaustive default — was the only per-kind helper still relying on implicit fallthrough; would silently omit a 4th-kind output path while the adjacent helpers flip to compile errors. - Hint wording de-specifies the exact keystroke count: `'Tip: focus the Background tasks pill in the footer (use ↓ from an empty composer) and press Enter ...'`. Previous "press ↓ then Enter" phrasing was wrong when the Arena agent tab bar is present — `InputPrompt`'s focus chain routes Down through the tab bar first, so a single Down lands there, not on the bg pill. - Test pin tightened: `[mon_fail] failed: spawn ENOENT (0 events)` is now a full-string assertion instead of a prefix match, so a regression that drops the `(N events)` suffix from monitor's failed branch fails loudly. * fix(cli): sanitize ANSI escape sequences in /tasks output deepseek's review pass flagged that monitor description / error fields are user / process-supplied strings rendered directly to the terminal. A maliciously-crafted tool description or spawn error containing raw ANSI control sequences (clear-screen, cursor-move, colour) would otherwise reach stdout verbatim and corrupt display. Same risk applies to agent error / description and shell error / command — all already-existing renderers with the same exposure that this PR didn't introduce but inherits. So instead of per-field sprinkling, wrap the joined output once with `escapeAnsiCtrlCodes` (no-op when no control chars present, so cost is zero in the common case). One line change in the renderer covers every kind including any future one. Test pins the behaviour: a monitor entry with `\x1b[2J` / `\x1b[31m...` content produces output with no raw ESC bytes and visible escaped `�[...]` sequences. * docs(cli): tighten escapeAnsiCtrlCodes comments to match actual scope Two doc-precision Suggestions from copilot's pass on 0840e32: - Source comment claimed `escapeAnsiCtrlCodes` is "a no-op when no control chars" but it's narrower than that — it only handles sequences matched by `ansi-regex` (CSI / OSC / SGR — anything starting with ESC). Isolated C0/C1 control bytes like BEL, BS, VT pass through untouched. Updated the comment to enumerate the actual scope and call out that `node:util`'s `stripVTControlCharacters` would be needed if those become a concern. - Test comment had a literal raw ESC byte (octal 033) embedded in the source — visually showed `^[[...]` in editors that render ESC, but was a real ESC byte in the file rather than the escaped `�` form the sanitizer produces. Rewrote with a literal `�` text description so what the comment shows matches what the assertions check for. * fix(cli): broaden /tasks sanitization + tighten inner switch exhaustiveness Addresses 3 of 5 items from doudouOUC's PR 3801 review: - **Issue 1 (Low) — C0/C1 control byte gap**: switched from `escapeAnsiCtrlCodes` (only handles ESC-initiated ANSI sequences) to `stripUnsafeCharacters` (one-pass strip of ANSI + VT + C0/C1, with TAB/CR/LF preserved). The pre-existing exposure to bare BEL / BS / FF / VT bytes via shell entry strings is now closed for all three kinds. Test rewritten to cover both ANSI sequences AND bare control bytes (BEL, BS), and pins that surrounding printable text and line breaks survive. - **Issue 2 (Low) — inner status switches inconsistent**: the three inner `switch (entry.status)` blocks (agent / shell / monitor) used `case 'running': default: return 'running'` (or duplicated bodies). All three now have explicit `running` cases plus a `never`-typed default that throws — matches the outer `switch (entry.kind)` pattern and means a future status added to any of `BackgroundTaskEntry` / `BackgroundShellEntry` / `MonitorStatus` flips to a compile error here instead of silently returning `'running'`. - **Issue 5 (Nit) — beforeEach default change**: added an inline comment explaining why the test default overrides `createMockCommandContext`'s `'interactive'` default (`'non_interactive'` lets the hint-suppression assertions work without each test rebinding context). Issues 3 and 4 from the review are nits with no action needed (3 is already documented as intentional; 4 is a UX call about hint length that's better handled by user feedback than guess-tweaking). * fix(cli): bind status to local before exhaustive switch — fixes tsc build CI's `tsc --build` (full mode, vs `--noEmit` locally) caught that `switch (entry.status)` followed by a `never`-typed default reading `entry.status` doesn't compile. After the case arms exhaust the discriminated union, TS narrows `entry` itself to `never`, so the `.status` access in the default arm becomes "Property 'status' does not exist on type 'never'" + the resulting `any` value can't be assigned to `never`. Fix: bind `entry.status` to a local `status` const before the inner switch. The local stays typed as the per-kind status union and narrows correctly to `never` at the default arm — `const _exhaustive: never = status` is then `never = never`, valid. Standard exhaustive-switch-on-discriminator pattern; doesn't change runtime behavior or test surface, just gets past TS narrowing on the nested case.
DragonnZhang
pushed a commit
that referenced
this pull request
May 8, 2026
…and the dialog (#3808) * docs(core): point background-shell guidance at both /tasks and the dialog Follow-up to PR #3801, fulfilling the "separate small PR" commitment in its description. The two model-facing strings (`shell.ts` after spawning a background shell, `task-stop.ts` after requesting cancel) referenced only `/tasks` as the inspection path, predating the interactive Background tasks dialog landing at #3488 / #3720 / #3791. Now that the dialog handles all three kinds (agent / shell / monitor), both surfaces should be visible to the LLM so it can suggest the right one based on the user's mode. Updates: - `shell.ts:865` (LLM message after `is_background: true` spawn) now surfaces both `/tasks` (text, any mode) AND the interactive dialog (footer pill + Enter, with detail view + live updates). Output file guidance retained. - `task-stop.ts:110` (LLM message after `task_stop` on a shell) same pattern: both surfaces named. - `task-stop.ts:95` comment updated to enumerate all observation paths (including the dialog). - `monitorRegistry.ts:197` comment fixed — said "/tasks dialog" which conflated two distinct surfaces. Split to "Background tasks dialog reopens or `/tasks` listings". - `backgroundShellRegistry.ts:10` (module docstring) and `:31` (shellId doc) now mention all three consumers (agent, dialog, slash command). No behavior change — pure documentation/string update. Tests untouched (none asserted on these exact strings); build + lint + 152-test core suite all clean. * docs(core): address PR 3808 review — 'captured output' + consistent ordering Three review nits: 1. (LoqU — copilot) `shell.ts:865` said the output file holds "raw content", but `shellExecutionService` runs each chunk through stripAnsi and skips non-string/binary chunks before writing. Reword to "captured output" so callers don't expect a byte-for-byte stream. 2. (LqKr — wenshao) The PR mentioned both surfaces in two different orders depending on the file: `backgroundShellRegistry.ts` listed the dialog first, while `task-stop.ts` and `shell.ts` listed `/tasks` first. Unify on the LLM-facing order — `/tasks` first, then the interactive Background tasks dialog — across all four sites. Also flips the line-31 docstring on the `shellId` field for the same reason. 3. (LqKt — wenshao, flagged for awareness only) Trim the redundant keystroke detail in shell.ts:865 to match `task-stop.ts:111`'s shorter "(footer pill + Enter)" form. Saves ~7 tokens per background shell launch in `llmContent` while still naming both surfaces. The PR description's rationale (LLM should know both surfaces exist so it can suggest the right one for the user's mode) is preserved — only the operational verbosity is trimmed. 581 tests pass; lint + typecheck clean. Pure docs / string update. * docs(core): grammar polish on PR 3808 strings Two more wording nits from copilot review: - backgroundShellRegistry.ts:10 — change "metadata the agent…need to query" to "metadata that the agent…use to query". The original phrasing reads as if the metadata itself is performing the query. - shell.ts:865 — change "Read the output file directly for the captured output." to "Read the output file directly to view the captured output." — clearer instruction to the model/user. Pure wording, no behavior change. * docs(core): grammar fix on PR 3808 monitor comment 'not visible from later Background tasks dialog reopens' read as if 'reopens' was a noun. Reword to 'not visible after reopening the Background tasks dialog or from /tasks listings'. * docs(core): round 4 wording polish on PR 3808 Four more nits from copilot: - shell.ts:865 + task-stop.ts:96,111: "footer pill + Enter" was ambiguous now that the footer renders multiple pills (background tasks vs other status indicators). Disambiguate to "focus the footer Background tasks pill, then Enter". - monitorRegistry.ts:198: re-tweak my round-3 phrasing — "after reopening the Background tasks dialog or from /tasks listings" → "in later Background tasks dialog reopens or /tasks listings". Reads as "from those surfaces" rather than "after reopening", which the reviewer found ungrammatical. - backgroundShellRegistry.ts:10,31: clarify "/tasks" as the slash command, since the codebase also uses "<projectDir>/tasks/..." on-disk paths in agent-transcript contexts. Pure wording, no behavior change. 87 affected tests pass. * docs(core): mirror /tasks + dialog guidance to monitor llmContent paths Address @doudouOUC review on PR #3808 — two Medium findings: this PR updated shell-facing strings to mention both inspection surfaces but left the parallel monitor strings without any inspection guidance, even though monitors render in the same /tasks output and the same Background tasks dialog. Restore symmetry: - monitor.ts:587-598 — append the same "/tasks (text) or the interactive Background tasks dialog (focus the footer Background tasks pill, then Enter — detail view + live updates)" sentence to the Monitor-started llmContent, mirroring shell.ts:865. - task-stop.ts:125-131 — the monitor cancellation llmContent had no guidance at all. Add the same "Final status will be visible via /tasks (text) or the interactive Background tasks dialog (focus the footer Background tasks pill, then Enter) once the process drains" line that already existed for shells at task-stop.ts:111. The (Low) commit-churn observation is a maintainer call (squash on merge); the (Info) snapshot-test gap is pre-existing and not in scope. 78 monitor + task-stop tests pass; lint + typecheck clean. * docs(core): drop drain phrasing for monitor cancel + restructure dialog comment Address PR #3808 review round 5 (doudouOUC + copilot × 2): 1. (XNoH copilot, XSBu doudouOUC — Medium) The monitor cancellation message inherited "once the process drains" from the shell branch, but `monitorRegistry.cancel()` settles synchronously — when the tool returns, the entry is already `cancelled`, not waiting on a child process. The drain qualifier is accurate for shells (which use `requestCancel()` + the AbortController and settle when the real process exits) but misleading for monitors. Reword the monitor branch in `task-stop.ts:121-130` to drop the drain phrasing and add an explanatory comment about the sync vs. async difference so future maintainers don't replicate the wording from the shell branch by reflex. 2. (XNod copilot — wording, third revision on the same comment) Restructure rather than re-litigate the preposition. The "reopens" noun framing has gone through three rounds of churn (`from later... reopens` → `after reopening...` → `in later... reopens` → and now back to `from`). Sidestep the loop by making the comment a proper sentence about WHAT the surfaces actually read: the persisted `entry.error` is the source of truth; the chat-history notification is a separate, ephemeral side channel. Avoids the noun-form "reopens" entirely. Updated test assertion to match the new "Monitor \"...\" cancelled" prefix. 51 tests pass; lint + typecheck clean.
xaelistic
pushed a commit
to xaelistic/qwen-code
that referenced
this pull request
Jun 7, 2026
…og (QwenLM#3720) * feat(cli): wire background shells into combined Background tasks dialog Phase B follow-up QwenLM#2: surface managed background shells in the same overlay that already shows local subagents, so users get one unified view instead of having to remember /tasks for shells. - BackgroundShellRegistry: add setRegisterCallback/setStatusChangeCallback and requestCancel(id), mirroring BackgroundTaskRegistry's contract. register() also fires statusChange so subscribers see the lifecycle start, not just transitions. - useBackgroundTaskView: subscribe to both registries, merge entries by startTime, attach a `kind` discriminator (DialogEntry union) so renderers can dispatch on agent vs shell. - BackgroundTasksPill: group running counts by kind ("2 shells, 1 local agent"); when all entries are terminal, collapse to "N task(s) done". - BackgroundTasksDialog: replace per-kind section header with a single "Background tasks" header; ListBody renders shell rows as "[shell] <command>"; DetailBody dispatches to AgentDetailBody (the original) or a new ShellDetailBody (cwd / output file / pid / exit). - Context cancelSelected switches by kind: agents go through cancel(), shells through requestCancel() — only aborts, lets the spawn settle path record the real terminal state (mirrors task_stop in QwenLM#3687). Tests: 8 pill cases (singular/plural per kind, mixed, terminal-only), 4 dialog cases (auto-fallback on running→terminal, cancel flow, already-terminal stays in detail, selectedIndex clamp); shell registry gains 5 callback tests + 3 requestCancel tests. * fix(cli): refresh detail-body agent fields between status changes useBackgroundTaskView shallow-copies agent entries into DialogEntry so each entry can carry a `kind` discriminator. The copy detaches `recentActivities` from the registry: BackgroundTaskRegistry.appendActivity mutates `entry.recentActivities = next` on the registry object and emits `activityChange`, but the dialog's activity callback only bumps a local counter — so the snapshot's `recentActivities` reference goes stale and the Progress block keeps rendering the old array until the next status-driven refresh. Resolve `selectedEntry` against the registry on each render when the selected entry is an agent, with `activityTick` as a useMemo dep so it recomputes on every activity callback. Snapshot remains the source of truth for the list (no churn on the pill / AppContainer); only the detail body re-reads live. Also rename the non-empty list section header from "Local agents" to "Background tasks" to match the empty-state branch and the unified multi-kind contents. --------- Co-authored-by: wenshao <wenshao@U-K7F6PQY3-2157.local>
xaelistic
pushed a commit
to xaelistic/qwen-code
that referenced
this pull request
Jun 7, 2026
…QwenLM#3791) * feat(cli): wire Monitor entries into combined Background tasks dialog Phase C mirror follow-up of Issue QwenLM#3634, structurally a clean repeat of QwenLM#3720 but for Monitor (third consumer of the kind framework). Core (MonitorRegistry): - Add `setStatusChangeCallback` mirroring `BackgroundShellRegistry` / `BackgroundTaskRegistry`. Fires synchronously inside `register()` (so subscribers see lifecycle start) and inside `settle()` (so subscribers see every running → terminal transition: complete / fail / cancel / emitEvent's auto-stop at maxEvents). - Subscriber failures are caught and logged but do not poison the registry — same defensive contract as the other two registries. CLI: - `useBackgroundTaskView` subscribes to all three registries (agent + shell + monitor) and merges by `startTime`. `DialogEntry` union extended with `(MonitorEntry & { kind: 'monitor' })`. `entryId` switches over the three kinds and returns the right id field. - `BackgroundTasksPill.getPillLabel` adds monitor to its KIND_NAMES table; grouping order is shell → agent → monitor (monitor last because it tends to be the longest-lived, least urgent to glance at). - `BackgroundTasksDialog`: - `rowLabel` returns `[monitor] <description>` for monitor rows. - New `MonitorDetailBody` showing command / status / pid / event count / dropped lines. No Progress block (monitors don't fire activity callbacks per-event). - DetailBody dispatcher gains the monitor branch. - `BackgroundTaskViewContext.cancelSelected` routes monitor cancels via `monitorRegistry.cancel(monitorId)`. Monitor's cancel is synchronous (settle + abort happen inside the registry), matching the same path task_stop already uses. Tests: 6 new core tests (registry callbacks fire on register / each terminal transition / not on emitEvent / on auto-stop / clear stops notifications / subscriber-failure isolation), 4 new pill tests (singular / plural / 3-kind grouping / running-only filter), dialog mock extended with `getMonitorRegistry`. * fix(cli): add exhaustive default arms to DialogEntry switches ESLint's `default-case` rule requires every switch to have a default arm even when TypeScript can prove the union is exhaustive. Add `default: { const _exhaustive: never = entry; throw ... }` to the five switches added in this PR — same pattern keeps both the runtime guard and the compile-time exhaustiveness check. * fix(core): fire statusChange in MonitorRegistry.reset() The newly-added `setStatusChangeCallback` subscriber misses `reset()`, so a `/clear` or session reset leaves stale monitor rows visible in the combined Background tasks dialog until an unrelated register/settle event happens. Both BackgroundShellRegistry and BackgroundTaskRegistry already fire statusChange on their reset paths — Monitor was the outlier. Fix: fire `statusChange()` (no arg) after `monitors.clear()`, with an early return when the registry is already empty so we don't notify on a no-op reset. Two new tests cover both branches. * fix(cli,core): address PR 3791 review feedback Four review threads from /review's second pass on top of 918250f: 1. **MonitorDetailBody live counters were stale.** `eventCount` and `droppedLines` came from the `useBackgroundTaskView` snapshot, which only refreshes on `statusChange`, and `emitEvent` deliberately doesn't fire `statusChange` (the per-event churn would burn the pill / AppContainer). Fix: extend the existing `selectedEntry` useMemo to re-resolve monitors from `monitorRegistry.get()` on each `activityTick`, mirroring the agent path. The pre-existing 1s wall-clock interval already drives the recompute while the entry is running, so no new callback wiring is needed. 2. **Settle reasons weren't persisted on `MonitorEntry`.** `fail()`, `complete(exitCode)`, max-events auto-stop, and idle-timeout all passed their reason strings only to the chat-history terminal notification — opening the dialog detail view after the monitor died showed the bare status with no clue why. Add `exitCode?` and `error?` fields (mirrors `BackgroundShellEntry`); populate them before `settle()` in each path; render them in `MonitorDetailBody` with appropriate colour (red for `failed`, warning for auto-stopped `completed`). 3. **`monitorCancel` mock had no test asserting it.** Add a dedicated test that opens detail on a monitor entry, presses `x`, and verifies `monitorRegistry.cancel(monitorId)` was called and the agent registry's cancel was NOT called. Pins the kind switch in `cancelSelected` so a regression flipping the monitor branch to anything else (e.g. `requestCancel`) would fail loudly. 4. **`MonitorStatusChangeCallback` docstring was wrong.** It claimed the callback fires on running → terminal transitions, but `register()` (nothing → running) and `reset()` (mass clear, fired with no entry) also fire it. Rewrite the docstring to enumerate the actual fire sites and explicitly note that `emitEvent` is excluded (per-event churn). * docs(cli,core): tighten MonitorEntry.error and rowLabel comments Two doc-precision fixes from copilot's PR 3791 review pass: - `MonitorEntry.error` enumerated max-events as the only auto-stop reason that populates the field, but `idle-timeout` also writes it (was added in the same earlier commit). Rewrote to list both current reasons and explicitly note: any future auto-stop reason should populate this field too. Also clarified that `cancelled` and natural-exit `completed` paths intentionally don't. - `rowLabel`'s shell-branch comment claimed long commands are "truncated by the row renderer's MaxSizedBox", but ListBody renders rows with plain `<Text>` (no MaxSizedBox / truncation helper). Long text wraps. Rewrote to describe actual behaviour and note the intentional decision to leave it wrapping (the dialog is what users open to see context — truncating defeats the purpose). * test(cli): cover MonitorDetailBody render branches + useBackgroundTaskView Two coverage gaps from /review's third pass on PR 3791: - New file `useBackgroundTaskView.test.ts` (6 cases) directly exercises the merge logic with a stub config: empty when config is null, merges three registries, sorts by startTime, tags `kind`, subscribes on mount, refreshes when any registry fires statusChange, clears all three subscriptions on unmount. - New `MonitorDetailBody render branches` describe block in `BackgroundTasksDialog.test.tsx` (8 cases) covers the conditional pieces my last commit added: title + Command, pid show/hide, eventCount singular vs plural, droppedLines show/hide, exitCode display, Error block (failed) vs Stopped because (auto-stop), and the all-undefined no-block case.
xaelistic
pushed a commit
to xaelistic/qwen-code
that referenced
this pull request
Jun 7, 2026
…enLM#3801) * feat(cli): include monitors in /tasks + add interactive-mode hint Phase B closure for Issue QwenLM#3634. Two coupled changes to /tasks: 1. **Bug fix — include monitors.** The command was last touched before QwenLM#3684 / QwenLM#3791 landed, so it merged only agent + shell entries while monitors silently disappeared from the headless / non-interactive / ACP listing path. Add a third registry pull from `getMonitorRegistry()` and wire monitor through statusLabel / taskLabel / taskId / taskOutputPath. Status line includes eventCount (`running (N events)`, `completed (exit 0, N events)`, `completed (Max events reached, N events)` for auto-stop) and pid where defined. 2. **Soft deprecation hint, scoped to interactive mode only.** Once the richer Ctrl+T dialog (QwenLM#3488 + QwenLM#3720 + QwenLM#3791) is available, the text dump is the long-form fallback rather than the primary surface. Show `Tip: Ctrl+T opens the interactive Background tasks dialog with detail view + live updates.` at the top of the output when `executionMode === 'interactive'`. Headless / ACP get the bare list — they have no dialog to point at and the hint would just clutter. Description string also clarified to call out the modal split. Kept on all three executionModes (no deletion) — `/tasks` is the only way headless / ACP / SDK consumers can inspect background-task state. Tests: 4 new cases in tasksCommand.test.ts cover monitor entry formatting (running with pid, natural completion with exitCode, auto-stop with error string, failed), the singular `1 event` form, the interactive-mode hint gating, and the cross-kind merge order. * fix(cli): address PR 3801 review — exhaustive switch + i18n + extra tests Three actionable Suggestions from /review's pass: - `taskLabel` rewritten as a `switch` with a `never`-typed `default` arm, matching the structural-safety pattern already used by `taskId`. Adding a 4th DialogEntry kind in the future will now flip both helpers to compile errors instead of letting `taskLabel` silently fall through to `entry.description` (which the new kind may not have). - Hint string wrapped in `t()` for i18n consistency with the rest of the file. The literal stays as the i18n key default, so today's output is unchanged. - Tests: cover `cancelled` monitor status (was the only one without an inline assertion) and explicit `acp` execution mode hint suppression (pins the suppression rationale so a future regression flipping the check to `!== 'non_interactive'` would fail loudly). * fix(cli): correct /tasks dialog-open hint — Ctrl+T was wrong Tmux verification on PR QwenLM#3801 caught that the hint string says "Ctrl+T opens the interactive Background tasks dialog" but Ctrl+T is actually bound to the MCP tool descriptions toggle (ContextSummaryDisplay.tsx lines 110-115). The dialog opens via Down arrow on an empty composer (focuses the footer pill) followed by Enter (InputPrompt.tsx 947-968). Same misattribution slipped into PR QwenLM#3791's first description and was caught + fixed there before merge — this PR carried the wrong wording forward in code. Updates four sites: - The hint string itself: "Tip: press ↓ from an empty composer then Enter to open the interactive Background tasks dialog with detail view + live updates." - The slash-command description: "interactive UI is Ctrl+T" → "interactive dialog opens via the footer pill" - Two inline comments referencing Ctrl+T as the dialog opener - The interactive-mode hint test now pins on `↓` + `Enter` and asserts `not.toContain('Ctrl+T')` so a regression to the wrong wording fails loudly. * fix(cli): address PR 3801 review — exhaustive switch consistency + path-agnostic hint Four Suggestions from the latest /review pass: - `statusLabel` rewritten as a single top-level switch with a `never`-typed default, matching `taskLabel` / `taskId` / `taskOutputPath`. The previous `if`/`if`/fallthrough form would silently apply monitor formatting to a future 4th kind. - `taskOutputPath` gained the same exhaustive default — was the only per-kind helper still relying on implicit fallthrough; would silently omit a 4th-kind output path while the adjacent helpers flip to compile errors. - Hint wording de-specifies the exact keystroke count: `'Tip: focus the Background tasks pill in the footer (use ↓ from an empty composer) and press Enter ...'`. Previous "press ↓ then Enter" phrasing was wrong when the Arena agent tab bar is present — `InputPrompt`'s focus chain routes Down through the tab bar first, so a single Down lands there, not on the bg pill. - Test pin tightened: `[mon_fail] failed: spawn ENOENT (0 events)` is now a full-string assertion instead of a prefix match, so a regression that drops the `(N events)` suffix from monitor's failed branch fails loudly. * fix(cli): sanitize ANSI escape sequences in /tasks output deepseek's review pass flagged that monitor description / error fields are user / process-supplied strings rendered directly to the terminal. A maliciously-crafted tool description or spawn error containing raw ANSI control sequences (clear-screen, cursor-move, colour) would otherwise reach stdout verbatim and corrupt display. Same risk applies to agent error / description and shell error / command — all already-existing renderers with the same exposure that this PR didn't introduce but inherits. So instead of per-field sprinkling, wrap the joined output once with `escapeAnsiCtrlCodes` (no-op when no control chars present, so cost is zero in the common case). One line change in the renderer covers every kind including any future one. Test pins the behaviour: a monitor entry with `\x1b[2J` / `\x1b[31m...` content produces output with no raw ESC bytes and visible escaped `�[...]` sequences. * docs(cli): tighten escapeAnsiCtrlCodes comments to match actual scope Two doc-precision Suggestions from copilot's pass on d392747a4: - Source comment claimed `escapeAnsiCtrlCodes` is "a no-op when no control chars" but it's narrower than that — it only handles sequences matched by `ansi-regex` (CSI / OSC / SGR — anything starting with ESC). Isolated C0/C1 control bytes like BEL, BS, VT pass through untouched. Updated the comment to enumerate the actual scope and call out that `node:util`'s `stripVTControlCharacters` would be needed if those become a concern. - Test comment had a literal raw ESC byte (octal 033) embedded in the source — visually showed `^[[...]` in editors that render ESC, but was a real ESC byte in the file rather than the escaped `�` form the sanitizer produces. Rewrote with a literal `�` text description so what the comment shows matches what the assertions check for. * fix(cli): broaden /tasks sanitization + tighten inner switch exhaustiveness Addresses 3 of 5 items from doudouOUC's PR 3801 review: - **Issue 1 (Low) — C0/C1 control byte gap**: switched from `escapeAnsiCtrlCodes` (only handles ESC-initiated ANSI sequences) to `stripUnsafeCharacters` (one-pass strip of ANSI + VT + C0/C1, with TAB/CR/LF preserved). The pre-existing exposure to bare BEL / BS / FF / VT bytes via shell entry strings is now closed for all three kinds. Test rewritten to cover both ANSI sequences AND bare control bytes (BEL, BS), and pins that surrounding printable text and line breaks survive. - **Issue 2 (Low) — inner status switches inconsistent**: the three inner `switch (entry.status)` blocks (agent / shell / monitor) used `case 'running': default: return 'running'` (or duplicated bodies). All three now have explicit `running` cases plus a `never`-typed default that throws — matches the outer `switch (entry.kind)` pattern and means a future status added to any of `BackgroundTaskEntry` / `BackgroundShellEntry` / `MonitorStatus` flips to a compile error here instead of silently returning `'running'`. - **Issue 5 (Nit) — beforeEach default change**: added an inline comment explaining why the test default overrides `createMockCommandContext`'s `'interactive'` default (`'non_interactive'` lets the hint-suppression assertions work without each test rebinding context). Issues 3 and 4 from the review are nits with no action needed (3 is already documented as intentional; 4 is a UX call about hint length that's better handled by user feedback than guess-tweaking). * fix(cli): bind status to local before exhaustive switch — fixes tsc build CI's `tsc --build` (full mode, vs `--noEmit` locally) caught that `switch (entry.status)` followed by a `never`-typed default reading `entry.status` doesn't compile. After the case arms exhaust the discriminated union, TS narrows `entry` itself to `never`, so the `.status` access in the default arm becomes "Property 'status' does not exist on type 'never'" + the resulting `any` value can't be assigned to `never`. Fix: bind `entry.status` to a local `status` const before the inner switch. The local stays typed as the per-kind status union and narrows correctly to `never` at the default arm — `const _exhaustive: never = status` is then `never = never`, valid. Standard exhaustive-switch-on-discriminator pattern; doesn't change runtime behavior or test surface, just gets past TS narrowing on the nested case.
xaelistic
pushed a commit
to xaelistic/qwen-code
that referenced
this pull request
Jun 7, 2026
…and the dialog (QwenLM#3808) * docs(core): point background-shell guidance at both /tasks and the dialog Follow-up to PR QwenLM#3801, fulfilling the "separate small PR" commitment in its description. The two model-facing strings (`shell.ts` after spawning a background shell, `task-stop.ts` after requesting cancel) referenced only `/tasks` as the inspection path, predating the interactive Background tasks dialog landing at QwenLM#3488 / QwenLM#3720 / QwenLM#3791. Now that the dialog handles all three kinds (agent / shell / monitor), both surfaces should be visible to the LLM so it can suggest the right one based on the user's mode. Updates: - `shell.ts:865` (LLM message after `is_background: true` spawn) now surfaces both `/tasks` (text, any mode) AND the interactive dialog (footer pill + Enter, with detail view + live updates). Output file guidance retained. - `task-stop.ts:110` (LLM message after `task_stop` on a shell) same pattern: both surfaces named. - `task-stop.ts:95` comment updated to enumerate all observation paths (including the dialog). - `monitorRegistry.ts:197` comment fixed — said "/tasks dialog" which conflated two distinct surfaces. Split to "Background tasks dialog reopens or `/tasks` listings". - `backgroundShellRegistry.ts:10` (module docstring) and `:31` (shellId doc) now mention all three consumers (agent, dialog, slash command). No behavior change — pure documentation/string update. Tests untouched (none asserted on these exact strings); build + lint + 152-test core suite all clean. * docs(core): address PR 3808 review — 'captured output' + consistent ordering Three review nits: 1. (LoqU — copilot) `shell.ts:865` said the output file holds "raw content", but `shellExecutionService` runs each chunk through stripAnsi and skips non-string/binary chunks before writing. Reword to "captured output" so callers don't expect a byte-for-byte stream. 2. (LqKr — wenshao) The PR mentioned both surfaces in two different orders depending on the file: `backgroundShellRegistry.ts` listed the dialog first, while `task-stop.ts` and `shell.ts` listed `/tasks` first. Unify on the LLM-facing order — `/tasks` first, then the interactive Background tasks dialog — across all four sites. Also flips the line-31 docstring on the `shellId` field for the same reason. 3. (LqKt — wenshao, flagged for awareness only) Trim the redundant keystroke detail in shell.ts:865 to match `task-stop.ts:111`'s shorter "(footer pill + Enter)" form. Saves ~7 tokens per background shell launch in `llmContent` while still naming both surfaces. The PR description's rationale (LLM should know both surfaces exist so it can suggest the right one for the user's mode) is preserved — only the operational verbosity is trimmed. 581 tests pass; lint + typecheck clean. Pure docs / string update. * docs(core): grammar polish on PR 3808 strings Two more wording nits from copilot review: - backgroundShellRegistry.ts:10 — change "metadata the agent…need to query" to "metadata that the agent…use to query". The original phrasing reads as if the metadata itself is performing the query. - shell.ts:865 — change "Read the output file directly for the captured output." to "Read the output file directly to view the captured output." — clearer instruction to the model/user. Pure wording, no behavior change. * docs(core): grammar fix on PR 3808 monitor comment 'not visible from later Background tasks dialog reopens' read as if 'reopens' was a noun. Reword to 'not visible after reopening the Background tasks dialog or from /tasks listings'. * docs(core): round 4 wording polish on PR 3808 Four more nits from copilot: - shell.ts:865 + task-stop.ts:96,111: "footer pill + Enter" was ambiguous now that the footer renders multiple pills (background tasks vs other status indicators). Disambiguate to "focus the footer Background tasks pill, then Enter". - monitorRegistry.ts:198: re-tweak my round-3 phrasing — "after reopening the Background tasks dialog or from /tasks listings" → "in later Background tasks dialog reopens or /tasks listings". Reads as "from those surfaces" rather than "after reopening", which the reviewer found ungrammatical. - backgroundShellRegistry.ts:10,31: clarify "/tasks" as the slash command, since the codebase also uses "<projectDir>/tasks/..." on-disk paths in agent-transcript contexts. Pure wording, no behavior change. 87 affected tests pass. * docs(core): mirror /tasks + dialog guidance to monitor llmContent paths Address @doudouOUC review on PR QwenLM#3808 — two Medium findings: this PR updated shell-facing strings to mention both inspection surfaces but left the parallel monitor strings without any inspection guidance, even though monitors render in the same /tasks output and the same Background tasks dialog. Restore symmetry: - monitor.ts:587-598 — append the same "/tasks (text) or the interactive Background tasks dialog (focus the footer Background tasks pill, then Enter — detail view + live updates)" sentence to the Monitor-started llmContent, mirroring shell.ts:865. - task-stop.ts:125-131 — the monitor cancellation llmContent had no guidance at all. Add the same "Final status will be visible via /tasks (text) or the interactive Background tasks dialog (focus the footer Background tasks pill, then Enter) once the process drains" line that already existed for shells at task-stop.ts:111. The (Low) commit-churn observation is a maintainer call (squash on merge); the (Info) snapshot-test gap is pre-existing and not in scope. 78 monitor + task-stop tests pass; lint + typecheck clean. * docs(core): drop drain phrasing for monitor cancel + restructure dialog comment Address PR QwenLM#3808 review round 5 (doudouOUC + copilot × 2): 1. (XNoH copilot, XSBu doudouOUC — Medium) The monitor cancellation message inherited "once the process drains" from the shell branch, but `monitorRegistry.cancel()` settles synchronously — when the tool returns, the entry is already `cancelled`, not waiting on a child process. The drain qualifier is accurate for shells (which use `requestCancel()` + the AbortController and settle when the real process exits) but misleading for monitors. Reword the monitor branch in `task-stop.ts:121-130` to drop the drain phrasing and add an explanatory comment about the sync vs. async difference so future maintainers don't replicate the wording from the shell branch by reflex. 2. (XNod copilot — wording, third revision on the same comment) Restructure rather than re-litigate the preposition. The "reopens" noun framing has gone through three rounds of churn (`from later... reopens` → `after reopening...` → `in later... reopens` → and now back to `from`). Sidestep the loop by making the comment a proper sentence about WHAT the surfaces actually read: the persisted `entry.error` is the source of truth; the chat-history notification is a separate, ephemeral side channel. Avoids the noun-form "reopens" entirely. Updated test assertion to match the new "Monitor \"...\" cancelled" prefix. 51 tests pass; lint + typecheck clean.
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
2 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.
Summary
Phase B follow-up #2 of Issue #3634. Surfaces the managed background-shell pool (#3642) in the same combined Background tasks overlay that already shows local subagents (#3488), so users get one unified view across both task kinds instead of having to remember
/tasksfor shells specifically.After this change, the pill, the list overlay, the detail overlay, and the cancel keybinding all work uniformly across agents and shells — driven by the same registry-callback subscription pattern that the agent side already uses.
Size: +500 / −100 across 8 files, 2 commits.
Before / After
Before
1 local agentonly (shells invisible).Ctrl+Toverlay: agent rows only./tasksslash command.x): agent only.After
2 shells, 1 local agentwhile running;3 tasks donewhen all terminal.Ctrl+Toverlay: singleBackground taskssection with both kinds, agents prefixed naturally and shells prefixed[shell] <command>.x): goes throughrequestCancel()for shells — only aborts, the spawn settle path records the real terminal state (mirrorstask_stopin feat(core): wire background shells into the task_stop tool #3687).What changed
Commit
314692e7f— main wiring:BackgroundShellRegistry(core): addsetRegisterCallback/setStatusChangeCallback/requestCancel(id), mirroringBackgroundTaskRegistry's contract.register()also firesstatusChangeso subscribers observe the lifecycle start, not just transitions.useBackgroundTaskView(cli): subscribe to both registries, merge entries bystartTime, attach akinddiscriminator (DialogEntryunion:agent|shell) so renderers can dispatch.BackgroundTasksPill: group running counts by kind ("2 shells, 1 local agent"); when all terminal, collapse to "N task(s) done". Shells listed first (matches Claude Code's pill convention).BackgroundTasksDialog: singleBackground tasksheader; ListBody renders shells as[shell] <command>; DetailBody dispatches toAgentDetailBody(original) or newShellDetailBody.cancelSelected: switches bykind— agents throughcancel(), shells throughrequestCancel().Commit
08e75f2d8— review-driven follow-up:selectedEntryre-resolves the agent from the registry on each render (inBackgroundTasksDialog.tsxvia auseMemowhose deps include anactivityTickcounter that thesetActivityChangeCallbackbumps). The shallow-copy inuseBackgroundTaskViewcopiesrecentActivitiesby reference, butBackgroundTaskRegistry.appendActivityreassignsentry.recentActivities = nexton the registry object — so the snapshot's reference detaches after the first activity and the Progress block goes stale. The fix keeps the snapshot as the source of truth for the list (so the pill and AppContainer don't churn on tool traffic), and only the detail body re-reads live.ListBodyrenderedLocal agentseven thoughentriesnow contains both kinds. Renamed toBackground tasksto match the empty-state branch.Design notes
The
DialogEntryshape is(BackgroundTaskEntry & { kind: 'agent' }) | (BackgroundShellEntry & { kind: 'shell' })— i.e., the underlying entry's fields are spread inline, then akinddiscriminator is added. Considered a wrapper shape ({ kind: 'agent', agent: BackgroundTaskEntry } | { kind: 'shell', shell: BackgroundShellEntry }) but rejected: every consumer (rowLabel,entryId,elapsedFor,cancelSelected,terminalStatusPresentation, plus all the renderers) would need to switch fromentry.startTimetoentry.agent.startTimeetc., and the renames cascade through several files. The spread keeps the renderer code mechanical.The trade-off is identity loss for nested mutable references like
recentActivities— addressed surgically in commit08e75f2d8via the live re-resolve inselectedEntry, scoped to the detail body so the optimization for the pill / AppContainer (no churn on tool traffic) is preserved.中文版
Phase B 后续任务 #2（Issue #3634 跟踪）。把 #3642 引入的后台 shell 池接入 #3488 的组合后台任务面板，统一两种后台任务（agent / shell）的展示与控制，不再需要记
/tasks。体量：+500 / −100，8 个文件，2 个 commit。
改动覆盖：
Commit
314692e7f（主接入）：BackgroundShellRegistry加上setRegisterCallback/setStatusChangeCallback/requestCancel(id)，对齐BackgroundTaskRegistry。register()同时触发statusChange，订阅者能看到生命周期起点。useBackgroundTaskView同时订阅两个 registry，按startTime合并，给每条目加kind判别字段（DialogEntry联合类型）。[shell] <command>；详情视图按 kind 派发，shell 展示 cwd / 输出文件 / pid / 退出码。x）：agent 走cancel()，shell 走requestCancel()（只 abort，settle 路径记录真实终态，沿用 feat(core): wire background shells into the task_stop tool #3687 在task_stop上的策略）。Commit
08e75f2d8（review 反馈）：selectedEntry在 detail mode 下从 registry 重新取（BackgroundTasksDialog.tsx的useMemo，deps 含activityTick，由setActivityChangeCallback触发刷新）。useBackgroundTaskView里的 shallow-copy 把recentActivities按引用复制，但BackgroundTaskRegistry.appendActivity会entry.recentActivities = next重新赋值 — snapshot 拿到的引用第一次 activity 后就脱节，Progress block 会渲染过期数据。修法保留 snapshot 作为列表来源（pill / AppContainer 不被 tool-call 流冲刷），只让详情视图走 live。ListBody还是Local agents，但entries现在含两种 kind。改成Background tasks跟 empty-state 对齐。设计说明
DialogEntry是(BackgroundTaskEntry & { kind: 'agent' }) | (BackgroundShellEntry & { kind: 'shell' })— 把底层 entry 的字段 spread 进来后加kind判别。考虑过 wrapper 形态（{ kind: 'agent', agent: BackgroundTaskEntry } | ...），否决：所有消费者（rowLabel、entryId、elapsedFor、cancelSelected、terminalStatusPresentation+ 所有渲染器）都要从entry.startTime改成entry.agent.startTime，跨多个文件级联改动。Spread 形态让渲染层代码保持机械。代价是
recentActivities这种嵌套可变引用的 identity 丢失 — 在08e75f2d8里精准修掉：selectedEntry走 live re-resolve，只针对 detail body，pill / AppContainer 的 "tool-call 不冲刷" 优化保留。Test plan
vitest runcore: 253 files / 6250 pass + 2 skippedvitest runcli: 304 files / 4701 pass + 7 skipped (含本 PR 新增 / 修改的：8 个 pill 用例 + 4 个 dialog 用例 + 5 + 3 个 shell-registry 用例)Ctrl+T打开 dialog；行带[shell]前缀；Enter 进 shell 详情；x取消；shell drain 后 pill 折叠成 "task done"。Related
/tasks)task_stopintegration; same cancel-vs-exit race policy is reused here)PR 3720 tmux interactive E2E test report
Branch:
feat/shell-in-dialogBinary tested:
node dist/cli.jsBuild command:
npm run build && npm run bundleResult: Build/bundle passed. Bundle assets copied to
dist/.Environment
/Users/wenshao/git/qwen-code-x7/tmp/qwen-bg-shell-e2e-pr3720qwen-pr3720-bg-shelltmux new-session -d -s qwen-pr3720-bg-shell -x 200 -y 50 \ 'cd /tmp/qwen-bg-shell-e2e-pr3720 && node /Users/wenshao/git/qwen-code-x7/dist/cli.js --approval-mode yolo'Commands actually run
Observations
Build / bundle
PASS
npm run build && npm run bundlecompleted successfully with exit code 0. The build emitted existing lint warnings in the VS Code companion package, but no errors.Background shell appears in footer pill
PASS
After launching a background shell, the footer showed:
This confirms the pill counts running shell entries.
Background shell appears in combined Background tasks overlay
PASS
Opening the overlay through the footer pill showed:
With two shell entries, the same combined overlay showed:
Shell row shows
[shell]prefixPASS
Both shell rows in the Background tasks dialog rendered with the
[shell]prefix.Entering shell detail shows cwd / output / pid / exit code
PASS
While the first shell was running, detail view showed:
After it finished, reopening detail showed terminal state and exit code:
This confirms cwd, output file path, pid, and completed exit code are visible.
xcancels a shellPASS
A second long-running shell was launched and selected in the Background tasks list. Before cancellation, the dialog hint showed
x stopfor the running selected shell:After sending
x, the selected shell stopped. Reopening its detail showed:The list hint also no longer showed
x stopfor that terminal entry.Shell finish collapses pill to task done
PASS
After the first shell completed, closing the dialog showed:
After both shell entries were terminal, footer showed:
This confirms running shell counts collapse to the terminal “task(s) done” form.
Ctrl+T opens Background tasks dialog
BLOCKED / NOT VERIFIED
I attempted:
The dialog did not open in the captured tmux frame. I could reliably open the Background tasks dialog via the footer pill route (
Down, thenEnter), but could not verify Ctrl+T opening the Background tasks dialog through tmux.Important caveat: current code/docs still show
Ctrl+Tbound toTOGGLE_TOOL_DESCRIPTIONS/ MCP tool description toggling, andtmux send-keyshas known limitations for key-combo verification. Therefore I am marking the Ctrl+T-specific item as blocked/not verified from this tmux run rather than failed.Notes
This did not block shell registration, overlay rendering, detail inspection, cancellation, or pill-state verification.
node dist/cli.js.Summary
[shell]prefixxstops/cancels running shelltask(s) done