feat(cli): include monitors in /tasks + add interactive-mode hint#3801
Merged
Conversation
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.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the /tasks slash command output to include monitor tasks (in addition to agents and background shells) and adds an interactive-mode-only hint directing users to the richer Ctrl+T Background tasks dialog.
Changes:
- Thread
getMonitorRegistry().getAll()into/tasksand merge monitor entries into the existing startTime-sorted list. - Extend formatting helpers (
statusLabel,taskLabel,taskId,taskOutputPath) to support a newmonitorkind. - Add unit tests for monitor formatting, hint gating by execution mode, and merged ordering across all three kinds.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/cli/src/ui/commands/tasksCommand.ts | Add monitor entries to /tasks, format monitor status with event counts, and prepend an interactive-only Ctrl+T hint. |
| packages/cli/src/ui/commands/tasksCommand.test.ts | Add test coverage for monitor rendering, singular/plural event wording, hint gating, and 3-kind ordering. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
May 3, 2026
wenshao
commented
May 3, 2026
wenshao
commented
May 3, 2026
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
…ests 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).
Collaborator
Author
End-to-end tmux verification — 1 working, 2 issues foundTest setup# tsx-based dev mode (npm run dev) bypasses the TypeScript build errors
# in this PR while still exercising the actual TS source on disk.
tmux new-session -d ... "HOME=/tmp/test-home npm run dev -- -y -i '<spawn-monitor-prompt>'"
# … wait for monitor to start …
# Type /tasks + EnterWorking ✓
Issue 1 —
|
| Issue | Severity | Fix scope |
|---|---|---|
tsc build fails on missing exitCode / error fields, runtime shows exit ? placeholder always |
Blocker | Add fields to MonitorEntry + populate in MonitorRegistry.complete/fail, OR simplify statusLabel |
Hint text says Ctrl+T but that key isn't bound to the dialog |
UX | Update string |
wenshao
commented
May 3, 2026
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
Re-review of the latest commit (5f6286a) confirms all prior findings are resolved:
- tsc: 0 type errors (the 7
MonitorEntryerrors from 796bd3a are gone after the origin/main merge brought inexitCode?/error?fields from #3791) - eslint: clean
- Exhaustive switch:
taskLabel,statusLabel,taskOutputPathnow useswitch+neverdefault - i18n: Ctrl+T hint string wrapped in
t() - Test coverage: added cancelled monitor + ACP hint suppression tests
No new issues found in the incremental diff. The build passes and the implementation is solid.
— glm-5.1 via Qwen Code /review
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.
Collaborator
Author
|
Tmux verification fixes — 965e02c: Issue 1 ( Issue 2 (
Thanks for the tmux verification — the bot reviewers ( |
Collaborator
Author
Round 2 verification — both fixes confirmedAfter commits $ npm run build # tsc --build
# clean ✓
Couldn't capture the LGTM 👍 |
wenshao
commented
May 3, 2026
wenshao
commented
May 3, 2026
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
…th-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.
wenshao
commented
May 3, 2026
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.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
wenshao
commented
May 3, 2026
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
No issues found. LGTM! ✅
Downgraded from Approve to Comment: self-PR cannot be approved by its own author.
— deepseek-v4-pro via Qwen Code /review
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
wenshao
commented
May 3, 2026
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: self-PR cannot be approved by its own author. — gpt-5.5 via Qwen Code /review
doudouOUC
reviewed
May 3, 2026
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
PR #3801 Review
Overall Assessment
This is a well-structured PR that fixes a real bug (monitors silently missing from /tasks output) and adds a useful UX hint. The code quality is high, with thorough exhaustive switches, proper ANSI sanitization, and good test coverage (7 new test cases). The PR description and design notes are exceptionally detailed.
Issues Found
1. [Low] C0/C1 control byte gap in sanitization
File: tasksCommand.ts (lines 228-240 in the new file)
escapeAnsiCtrlCodes only handles ESC-initiated ANSI sequences (CSI/OSC/SGR). Isolated C0 control bytes like BEL (0x07, audible bell), BS (0x08, cursor backspace), or VT (0x0B) in entry.description, entry.error, or entry.command pass through to the terminal. While the PR documents this clearly and it's a pre-existing exposure for shell entries, this PR extends the attack surface to monitor entries.
Suggestion: Consider using stripVTControlCharacters from node:util (already imported in textUtils.ts) before the ANSI escape-code sanitizer, or use the existing stripUnsafeCharacters utility which handles both. The current approach is defense-in-depth for ESC sequences but leaves other control bytes untouched.
2. [Low] Inconsistent exhaustiveness pattern in inner status switches
File: tasksCommand.ts (lines 31-97)
The outer switch (entry.kind) correctly uses a never-typed default for exhaustive checking. However, the inner switch (entry.status) blocks use a default that duplicates the running case:
// monitor case
case 'running':
return `running (${events})`;
default:
return `running (${events})`; // duplicates 'running'Since MonitorStatus = 'running' | 'completed' | 'failed' | 'cancelled' and all four are handled, the default is unreachable. The same applies to the shell inner switch. For the agent case the default sits with 'running' which has the same issue.
Suggestion: For maximum safety, the inner switches could also use the never-typed exhaustive pattern, or remove the default entirely and let TypeScript catch unhandled cases at compile time. This would give the same compile-time guarantee that the outer switch provides if a 5th status is ever added to MonitorStatus.
3. [Nit] executionMode is optional on CommandContext
executionMode is typed as 'interactive' | 'non_interactive' | 'acp' | undefined. The PR's strict === 'interactive' check handles undefined correctly (no hint shown), which is the safe default. Worth noting: if executionMode is ever undefined in a truly interactive context, the hint would be silently suppressed. The PR's design notes call this out as intentional ("an unknown / future mode value defaults to not showing the hint"), so this is by design. No action needed.
4. [Nit] Hint string length in narrow terminals
The hint text is 155 characters:
Tip: focus the Background tasks pill in the footer (use ↓ from an empty composer) and press Enter for the interactive dialog with detail view + live updates.
In terminals narrower than ~80 columns, this will wrap across multiple lines and could feel visually heavy at the top of the output. Not a bug, but worth considering whether a shorter form or a two-line split would improve readability.
5. [Nit] Test beforeEach default changed from interactive to non_interactive
The beforeEach now explicitly sets executionMode: 'non_interactive', overriding createMockCommandContext's default of 'interactive'. This is correct for the new hint-suppression tests, but it changes the execution context of the 3 pre-existing tests. Those tests don't assert on the hint so behavior is unchanged, but a brief inline comment would help reviewers understand why the default was changed.
Positive Observations
-
Exhaustive
never-typed defaults on all four helper functions (statusLabel,taskLabel,taskId,taskOutputPath) ensure a compile error if a 4thTaskEntrykind is added. Excellent defensive typing. -
ANSI sanitization at the output boundary rather than per-field is a clean design. It covers all current and future fields with one call and avoids per-site sanitization sprawl.
-
Test coverage is thorough: all monitor statuses (running, completed-natural, completed-auto-stop, failed, cancelled), singular/plural eventCount, interactive/non_interactive/acp hint gating, ANSI escape sanitization, and cross-kind merge ordering.
-
The
entry.errorvsentry.exitCodedecision for auto-stop monitors is well-reasoned: when a monitor auto-stops, the reason ("Max events reached", "Idle timeout") is more informative than the exit code. Good UX choice. -
Scope discipline — leaving
task-stop.ts:110andshell.ts:865stale references for a separate PR keeps the diff reviewable.
Summary
The PR is in good shape. The primary concern (issue #1) is the incomplete sanitization of control characters — it's a gap that existed before this PR but is widened by adding monitor entries. The exhaustiveness inconsistency (issue #2) is minor. Everything else is clean, well-tested, and well-documented.
review from gpt5.5
…veness 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).
Collaborator
Author
|
@doudouOUC thanks for the thorough review. Addressed in 677b6e5: Issue 1 — C0/C1 control byte gap ✅ Issue 2 — Inner status switches inconsistent ✅ Issue 5 — beforeEach default change ✅ Issues 3 + 4 — no action:
Tests: 10 / 10 pass, lint + tsc clean. PR description (Design notes section) and inline comments updated to reflect the new Note on review attribution: I see this came from |
…uild 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.
doudouOUC
reviewed
May 3, 2026
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
Follow-up Review (post 677b6e5)
The latest commit addresses all three actionable items from the previous review. Changes look correct.
Issues Resolved
Issue 1 (C0/C1 control byte gap) — Switched from escapeAnsiCtrlCodes to stripUnsafeCharacters, which handles ANSI sequences (via strip-ansi), VT control sequences (via node:util.stripVTControlCharacters), and bare C0/C1 control bytes in a single pass while preserving TAB/CR/LF. The sanitization test now covers both ANSI sequences (\x1b[2J, \x1b[31m) and bare control bytes (\x07 BEL, \x08 BS), and verifies surrounding printable text + newlines survive. ✓
Issue 2 (Inner switch exhaustiveness) — All three inner switch (entry.status) blocks (agent / shell / monitor) now have never-typed default arms with throw, matching the outer switch (entry.kind) pattern. Adding a new status to any of the three entry types will now produce a compile error here. ✓
Issue 5 (beforeEach comment) — Clear 5-line inline comment explaining why the test overrides createMockCommandContext's default to 'non_interactive'. ✓
One Minor Observation (No Action Needed)
stripUnsafeCharacters preserves DEL (0x7F) — its doc comment says "handled functionally by applyOperations, not a display issue", which is true in its original editor-input context. In the /tasks terminal-output context, DEL is not processed by applyOperations and would display as ^? if present. Practically irrelevant (DEL in a monitor description/error is essentially impossible), but worth noting the function wasn't originally designed for this use case. If this ever becomes a concern, a one-line filter addition in stripUnsafeCharacters would close it.
Behavioral Note
The switch from escapeAnsiCtrlCodes (which escaped ANSI sequences into visible \u001b[...] form) to stripUnsafeCharacters (which strips them entirely) means users no longer see what escape sequences were attempted — they just see the clean text. This is a reasonable trade-off for a listing view where readability matters more than forensic detail.
Verdict
LGTM. No remaining blockers.
review from gpt5.5
doudouOUC
approved these changes
May 3, 2026
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
Review Summary
Well-structured PR — fixes a real bug (monitors missing from /tasks) and adds a useful interactive-mode hint. Code quality is high throughout.
What's Good
- Exhaustive
never-typed defaults on all helper functions (statusLabel,taskLabel,taskId,taskOutputPath) — both outerswitch (entry.kind)and innerswitch (entry.status)— ensure compile errors if a new kind or status is ever added. stripUnsafeCharactersat the output boundary handles ANSI sequences, VT control sequences, and bare C0/C1 control bytes in one pass. Clean design that covers all current and future entry fields without per-site sanitization.- Thorough test coverage: all monitor statuses, singular/plural eventCount, interactive/non_interactive/acp hint gating, ANSI + bare control byte sanitization, and cross-kind merge ordering.
- Strict
=== 'interactive'for the hint — correctly handlesundefinedand unknown future modes by defaulting to no hint. - Good scope discipline — stale
/tasksreferences intask-stop.tsandshell.tsleft for a separate PR.
Minor Observations (No Action Needed)
stripUnsafeCharacterspreserves DEL (0x7F) since it was designed for editor-input contexts. In/tasksterminal output DEL would render as^?, but it's practically impossible to encounter in entry fields.- The switch from escaping ANSI (visible
\u001b[...]) to stripping (silent removal) is a reasonable trade-off for a listing view.
LGTM.
review from gpt5.5
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
) * 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
…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
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 closure for Issue #3634. Two coupled changes to the
/tasksslash command:getMonitorRegistry()pull and threads monitor through every helper (statusLabel/taskLabel/taskId/taskOutputPath)./tasksbecomes the long-form fallback for non-TTY consumers rather than the primary surface. Show a single-line hint at the top of the output whenexecutionMode === 'interactive'; keep the bare list fornon_interactive/acp.Size: +217 / -13 across 2 files, 1 commit.
Decision: which of the three options from #3634
Issue #3634's Phase B left this open as "delete / keep as fallback / add deprecation hint — pick one." This PR picks "keep + add hint, scoped to interactive mode" — a hybrid of options 2 and 3.
/tasksis the only waynon_interactive(-pflag),acp(IDE bridges like Zed), and SDK consumers can inspect background-task state — they have no TTY for the dialog. Deleting would silently break those flows./tasksstays load-bearing for non-TTY callers indefinitely. Reframed as a surface redirect: in interactive mode where users have a richer alternative, point them at it; in non-TTY modes the command is the canonical surface so the hint would be noise.The result is the same line count cost as a pure deprecation note but the framing accurately matches the long-term intent.
Before / After
Before (any mode):
— monitor entries omitted; no hint about Ctrl+T.
After (interactive):
After (non_interactive / ACP): same content, hint suppressed.
What changed
tasksCommand.ts:MonitorTaskEntry = MonitorEntry & { kind: 'monitor' }to the union.statusLabelmonitor branch:running (N events),completed (exit X, N events)for natural exit,completed (Max events reached, N events)for auto-stop,failed: <reason> (N events),cancelled (N events).taskLabelreturnsentry.descriptionfor monitor (mirrors what the dialog uses).taskIdswitches across all three kinds with an exhaustive default — flips to a TS error if a 4th kind ever lands.taskOutputPathreturnsundefinedfor monitor (events stream viatask_notification, no on-disk file).actionpulls fromgetMonitorRegistry()and merges into the existingstartTimesort.Tip: Ctrl+T opens the interactive Background tasks dialog with detail view + live updates.prepended whencontext.executionMode === 'interactive'.tasksCommand.test.ts(4 new cases): monitor entry formatting across 4 status combinations + asserts nooutput:line for monitors; singular1 eventvs pluralN events(with substring guard); interactive-mode hint gating; cross-kind merge order.Design notes
A few decisions that aren't obvious from the diff:
executionMode === 'interactive'(not!== 'non_interactive'). The defensive choice — an unknown / future mode value defaults to not showing the hint, which is better than misfiring (a hint pointing at Ctrl+T in a context where Ctrl+T doesn't work would be actively confusing). Three known modes today; widen the check explicitly if a 4th interactive-like mode is added.statusLabelincludeseventCountbut notdroppedLines.eventCountis the primary signal of monitor activity at a glance — every monitor user wants to know "is it producing events?".droppedLinesonly matters when the back-pressure path activates (rare); surfacing it in the always-on row would noisify the common case. The dialog's detail view (feat(cli): wire Monitor entries into combined Background tasks dialog #3791) does showdroppedLines > 0because users opened the detail surface specifically to see fault context.errorstring instead ofexitCode. When a monitor auto-stops (Max events reached,Idle timeout), the exit code is uninformative — what the user actually wants is the reason.errorcarries it (per feat(cli): wire Monitor entries into combined Background tasks dialog #3791'sMonitorEntry.errorfield). Natural exit andfailedpaths fall back toexitCodesince that's the load-bearing signal there./tasksreferences untouched.task-stop.ts:110andshell.ts:865still say "visible via /tasks once the process drains" / "Use the /tasks command to list and inspect…". Those wording tweaks (mention Ctrl+T as the interactive option) are a separate concern and a separate small PR — bundling them here would make the diff harder to review and they don't depend on the changes here.stripUnsafeCharacters(textUtils.ts) — one pass that removes ANSI escape sequences (CSI / OSC / SGR), VT control sequences, and isolated C0 (BEL / BS / FF / VT) + C1 control bytes, while preserving TAB / CR / LF that are needed for line breaks. One call covers every kind including any future one and avoids per-field sanitization sprawl. Earlier rev usedescapeAnsiCtrlCodes(only handles ESC sequences), but @doudouOUC's review caught that bare BEL / BS / VT bytes still passed through; switched to the more comprehensive helper.中文版
Issue #3634 的 Phase B 收尾。
/tasks命令两处耦合改动：getMonitorRegistry()调用，把 monitor 串进每个 helper。/tasks变成非 TTY 消费者的长格式 fallback，不再是主入口。executionMode === 'interactive'时在输出顶部加单行提示；non_interactive / ACP 不显示。三选一决策：Issue #3634 把 Phase B 这一项留作"删 / 留作 fallback / 加 deprecation hint 三选一"。本 PR 选**"留 + 仅 interactive 加 hint"**的混合。
/tasks是 non_interactive / ACP / SDK 唯一能看到后台任务状态的方式，删了断这些路径。设计说明（diff 里看不出来的几条）：
executionMode === 'interactive'而不是!== 'non_interactive'— 防御未来新增 mode：未知模式默认不显示 hint，比误显示安全。statusLabel显示eventCount但不显示droppedLines— eventCount 是 glance 信号（每个 monitor 用户都想知道"有事件吗"）；droppedLines 只在 back-pressure 触发时才有意义（罕见），主榜显示会噪声化。详情视图（feat(cli): wire Monitor entries into combined Background tasks dialog #3791）会在droppedLines > 0时显示，因为用户那时是主动看故障上下文。error字符串而不是exitCode— auto-stop 时 exit code 无信息量，用户真正想知道的是为什么停。error字段承载这个信息（feat(cli): wire Monitor entries into combined Background tasks dialog #3791 引入）；natural exit 和failed路径仍用exitCode因为那才是关键信号。/tasks引用没改：task-stop.ts:110和shell.ts:865还在说"visible via /tasks once the process drains" / "Use the /tasks command..."。那是独立小 PR 的事，捆进来反而难审，且互相不依赖。lines.join('\n')外包一层stripUnsafeCharacters（textUtils.ts） — 一次 pass 移除 ANSI 转义序列（CSI / OSC / SGR）、VT 控制序列、孤立 C0（BEL / BS / FF / VT）+ C1 控制字节，同时保留 TAB / CR / LF（行分隔需要）。一次调用覆盖所有 kind 包括未来新加的，避免每字段消毒散点。早期版本用escapeAnsiCtrlCodes（只处理 ESC 序列），@doudouOUC review 抓到 bare BEL / BS / VT 仍能穿透，换成这个更全面的 helper。Test plan
vitest run tasksCommand.test.ts: 7 / 7 pass (3 existing + 4 new)/tasksin interactive — expect hint + 3 entries with correct kind formatting; run with-pnon-interactive — expect 3 entries, no hint.Related
/tasksup to it)/tasksnow points at it from the hint)/tasksreferences intask-stop.ts:110+shell.ts:865to mention Ctrl+T as the interactive option (intentionally out of scope here).