docs(core): point background-shell + monitor guidance at both /tasks and the dialog#3808
Merged
Conversation
…alog 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.
Contributor
There was a problem hiding this comment.
Pull request overview
Updates core-facing strings and internal comments to reference both /tasks (text listing) and the interactive Background tasks dialog as valid inspection surfaces for background shells/monitors, aligning documentation with the newer dialog UI.
Changes:
- Update LLM-facing tool result strings to mention both
/tasksand the Background tasks dialog. - Refresh related internal comments/docstrings to avoid implying
/tasksis the only consumer surface. - Clarify MonitorRegistry commentary to distinguish
/taskslistings vs dialog detail view behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/core/src/tools/task-stop.ts | Updates cancellation messaging/commentary to reference both /tasks and the Background tasks dialog. |
| packages/core/src/tools/shell.ts | Updates the post-spawn background shell guidance string to include both inspection surfaces. |
| packages/core/src/services/monitorRegistry.ts | Clarifies comment wording to distinguish dialog reopens vs /tasks listings. |
| packages/core/src/services/backgroundShellRegistry.ts | Updates module/JSDoc to name both the dialog and /tasks as consumers. |
💡 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.
[Suggestion] task-stop.ts:127 — Monitor cancellation llmContent lacks inspection guidance. The shell cancellation path (line 111) tells the model where to check final status ("via /tasks (text) or the interactive Background tasks dialog"), but the monitor path just says Cancellation requested for monitor "...". without any hint. Since monitors are equally visible in both surfaces, consider adding the same guidance:
llmContent:
`Cancellation requested for monitor "${taskId}". ` +
`Final status will be visible via /tasks (text) or the interactive Background tasks dialog once the process drains.\n` +
`Command: ${monitorEntry.command}`,[Suggestion] monitor.ts:593 — Monitor startup llmContent does not mention /tasks or the Background tasks dialog as inspection surfaces, while shell startup (shell.ts:865) includes a detailed "To inspect: ..." paragraph. Both monitors and shells are background tasks visible in the same UI surfaces. Consider adding inspection guidance after the auto-stop sentence:
`The monitor auto-stops after ${maxEvents} events or ${idleTimeoutMs}ms of silence.\n` +
`To inspect: /tasks (text listing, works in any mode) or the interactive Background tasks dialog (focus the footer pill, then Enter).`,
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. |
…rdering 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.
Collaborator
Author
Review feedback addressed (commit 76b5929)
581 单测通过；lint + typecheck 干净。纯文档/字符串改动。 |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 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 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.
Collaborator
Author
|
Round 2 grammar nits addressed (commit 24cd5d7):
|
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
'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'.
Collaborator
Author
|
Round 3 grammar nit (commit 19538dc): |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Collaborator
Author
|
Round 4 wording polish (commit 5249601):
87 个直接相关单测通过；lint + typecheck 干净。 |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 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, GitHub does not allow approving your own PR. — gpt-5.5-0424-global via Qwen Code /review
doudouOUC
reviewed
May 4, 2026
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
PR #3808 Review
Overall this is a well-scoped, well-documented PR that correctly updates LLM-facing strings and internal comments to reference both /tasks and the interactive Background tasks dialog. CI green, no behavior change, good commit hygiene. Below are the findings.
[Medium] Monitor startup llmContent lacks inspection guidance
File: packages/core/src/tools/monitor.ts:588-596
The shell startup message (shell.ts:865) now includes a detailed "To inspect: /tasks (text) or the interactive Background tasks dialog (focus the footer Background tasks pill, then Enter — detail view + live updates)." paragraph, but the monitor startup message says only:
Events will be delivered as notifications. The monitor auto-stops after N events or Nms of silence.
No mention of /tasks or the dialog. Both monitors and shells appear in the same Background tasks dialog — the LLM deserves the same awareness for both. Without this, the model has no way to guide a user toward inspecting a running monitor through the dialog.
Suggestion: Add inspection guidance after the auto-stop sentence:
`The monitor auto-stops after ${maxEvents} events or ${idleTimeoutMs}ms of silence.\n` +
`To inspect: /tasks (text) or the interactive Background tasks dialog (focus the footer Background tasks pill, then Enter).`,[Medium] Monitor cancellation llmContent lacks inspection guidance
File: packages/core/src/tools/task-stop.ts:124-129
The shell cancellation path (line 111) tells the model where to check final status:
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;
But the monitor cancellation path says only:
Cancellation requested for monitor "${taskId}".
Command: ${monitorEntry.command}
No inspection guidance at all. Since monitors are equally visible in both surfaces after cancellation, the model should know where to direct the user.
Suggestion: Add the same inspection guidance:
llmContent:
`Cancellation requested for monitor "${taskId}". ` +
`Final status will be visible via /tasks (text) or the interactive Background tasks dialog once the process drains.\n` +
`Command: ${monitorEntry.command}`,[Low] 5-commit wording churn — squash merge recommended
The PR went through 5 commits of wording polish (round 4 wording polish, grammar fix, grammar polish, etc.), all responding to Copilot/self-review nits on the same strings. The final result is good, but the commit history is noisy for a docs-only PR. Consider squash-merging to keep main history clean.
[Info] No test coverage for exact LLM message strings
None of the core tests assert on the exact string content of the LLM-facing messages being updated. This is a pre-existing gap, not introduced by this PR. If exact message format matters for model behavior, snapshot tests would be appropriate as a follow-up.
Verdict
The PR achieves its stated goal. The primary concern is the inconsistency with monitors — the PR updates shell-facing strings to mention both inspection surfaces but leaves monitor-facing strings with no inspection guidance at all, even though monitors are visible in the same UI. This should either be addressed in this PR or tracked as an explicit follow-up issue. Everything else is clean.
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.
Collaborator
Author
|
@doudouOUC thanks for the review — both Medium findings addressed in commit d9fdde2:
Now both shells and monitors mirror the same The (Low) commit-churn observation is a maintainer call (squash on merge would be reasonable); the (Info) snapshot-test gap is pre-existing and out of scope for this PR. 78 monitor + task-stop tests pass; lint + typecheck clean. |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
doudouOUC
reviewed
May 4, 2026
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
Updated review after the new commits adding monitor inspection guidance.
The two Medium findings from my previous review have been addressed:
- ✅ Monitor startup
llmContentnow includes inspection guidance (monitor.ts:597) - ✅ Monitor cancellation
llmContentnow includes inspection guidance (task-stop.ts:129)
One new issue found — see inline comment on task-stop.ts:129.
…og 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.
Collaborator
Author
Round 5 review feedback addressed (commit 7035ad7)
附带把 |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Collaborator
Author
|
Round 6 (copilot — scope/title/size): Fair catch. The PR did expand scope after @doudouOUC's review pointed out the shell-vs-monitor asymmetry, and the meta hadn't kept up. Updated:
Reverting the monitor changes wasn't an option — they were addressing a real symmetry gap doudouOUC flagged. Updating the meta is the right call. |
doudouOUC
approved these changes
May 4, 2026
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
Code Review Summary
Reviewed all changes across 7 commits (+24/-12, 6 files). All previous findings resolved.
Changes Reviewed
| File | Change | Assessment |
|---|---|---|
backgroundShellRegistry.ts |
Module docstring + JSDoc: add dialog as consumer alongside /tasks |
✅ Clear, consistent |
monitorRegistry.ts |
Rewrite comment: clarify chat-history notification vs persisted entry.error |
✅ Much clearer than previous "not visible from /tasks dialog reopens" |
monitor.ts |
Startup llmContent: add inspection guidance line |
✅ Consistent with shell.ts |
shell.ts |
Startup llmContent: replace old /tasks-only guidance with both surfaces |
✅ Good |
task-stop.ts |
Shell cancel: add dialog surface; Monitor cancel: restructure with accurate phrasing | ✅ See below |
task-stop.test.ts |
Update assertion to match new monitor cancel message | ✅ Correct |
Previous Findings — All Resolved
- [Medium → Fixed] Monitor startup
llmContentlacked inspection guidance — now added atmonitor.ts:597, consistent withshell.ts:865. - [Medium → Fixed] Monitor cancellation used inaccurate "once the process drains" — now reads
Monitor "${taskId}" cancelled. Status is visible via /tasks (text) or the interactive Background tasks dialog ...with a clear code comment explaining the semantic difference (synchronous settle vs async drain). - [Low → Noted] 7-commit wording churn — acceptable if squash-merged.
- [Info] No snapshot tests for LLM message strings — pre-existing gap, out of scope.
Nit (non-blocking)
The PR description still says "+10 / −8 across 4 files" but the actual diff is +24 / −12 across 6 files (monitor startup/cancel guidance + test update were added). Updating the description before merge would be nice but not required.
CI
The macos-latest / 22.x failure is in AuthDialog.test.tsx — unrelated to this PR. All other platforms pass, lint + typecheck clean.
Verdict
All medium findings addressed, test updated, no regressions. Approving.
TaimoorSiddiquiOfficial
pushed a commit
to TaimoorSiddiquiOfficial/HopCode
that referenced
this pull request
May 4, 2026
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
…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
Follow-up to PR #3801, fulfilling the "separate small PR" commitment in its description. The model-facing strings (
shell.tsafter spawning a background shell,task-stop.tsafter requesting cancel, plus the parallel monitor strings inmonitor.tsandtask-stop.ts) and several related comments referenced only/tasksas 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 (TTY vs headless / SDK / ACP).Size: +24 / −12 across 6 files. Pure docs / string update — no behavior change.
Scope
Started as 2 LLM-facing strings (
task-stop.ts:110,shell.ts:865— the original commitment in #3801) plus 4 internal comments / docstrings I caught while runninggrep -rn '/tasks' packages/core/src. After @doudouOUC's review on this PR pointed out the symmetry gap, also extended to:monitor.ts:587-598— Monitor-startedllmContentmirrors the same/tasks+ dialog inspection guidance asshell.ts:865.task-stop.ts:121-130(monitor-cancel branch) — adds final-status guidance, parallel to the shell-cancel branch.task-stop.test.ts— assertion updated to match the new "Monitor "..." cancelled" prefix.This brings shells and monitors to the same level of LLM awareness about both inspection surfaces.
Intentionally NOT changed
Two
/tasksreferences in the same area are correct as-is:agent-transcript.ts:52— references the path<projectDir>/tasks/<sessionId>/<kind>-<id>.jsonl, not the slash command. Different concept that happens to share the word.shell.ts:839— the comment already readslet /tasks (and any future UI) ..., so it's already framed as one of multiple consumers. No update needed.Before / After (final form after review)
shell.ts:865(LLM message afteris_background: truespawn)task-stop.ts:111(LLM message aftertask_stopon a shell)monitor.ts:597(LLM message after Monitor tool spawns)task-stop.ts:127(LLM message aftertask_stopon a monitor)task-stop.ts:95(comment)monitorRegistry.ts:195-200(comment — fixes a real misnomer)The old wording said
"/tasks dialog"which conflated two distinct surfaces —/tasksis the slash-command text dump, the Background tasks dialog is the interactive overlay. Fixed by naming them separately and tightening the explanation.backgroundShellRegistry.ts:7-12(module docstring) and:31(shellIdJSDoc)Design notes
focus the footer Background tasks pill, then Enterto disambiguate from other footer pills (e.g. status indicators).monitorRegistry.cancel()settles synchronously while shell cancel is async. The shell branch keeps "once the process drains" since that's accurate for shells.Self-correction
The original commitment in #3801 said the follow-up should "mention Ctrl+T as the interactive option". That was wrong —
Ctrl+Tis bound to the MCP tool descriptions toggle, not the Background tasks dialog. The actual opener is↓from an empty composer to focus the footer Background tasks pill, thenEnter. This PR uses the correct framing.Test plan
tsc --buildclean/tasksand the dialog; cancel a background shell viatask_stop→ same. Same formonitortool spawn and cancel.Related
/tasksrepositioning — committed to this PR as scope-discipline carve-out, also the source of the now-correctedCtrl+Tframing)