fix(bg-shell): keep agent on duty until session-started bg jobs finish (#167)#168
Merged
Conversation
Previously the right-click 'Archive' menu was a soft-hide toggle — it flipped session.archived in globalState, the entry disappeared from the sidebar, and that was it. Users reported that the label did not match what happened: nothing was actually archived anywhere they could grep, commit, or share. This change makes Archive do what it says: 1. src/chat/archive-export.js (new): renders a SessionStore record to Markdown — YAML frontmatter with sessionId/model/timestamps, role- tagged sections (User / Assistant), and <details>-wrapped reasoning so it does not drown out the conversation. Picks the destination via workspaceFolders (single root -> use it; multi-root -> workspace folder pick, biased toward the session's original ws; no folder open -> showSaveDialog). Sanitises titles for filesystem hostility, appends -1/-2 on name collision, and verifies the resolved path stays under the chosen root before writing (defence in depth). 2. SessionStore.archive: on first archive of a visible session, export to .deepcopilot/archives/yyyyMMdd-HHmmss-<title>.md, then perform the original soft-hide. Clicking again on an archived session still un-hides it (toggle preserved; the file on disk is not touched). Export failures surface through showErrorMessage but do not block the hide, so the gesture always advances UI state. 3. Bottom-right toast on success with Open File / Reveal in Explorer actions. Path is shown workspace-relative when possible. 4. i18n: 9 new keys for the toast, dialog labels, and rendered role headers. EN and ZH both populated. Security (per copilot-instructions.md red-line #3): writes are gated by path.relative containment check + path.resolve normalisation before any fs.writeFile call; no user input is interpolated into a shell command. Closes #165.
Four corrections from Copilot's PR review on #166: 1. archive-export.js renderSessionMarkdown: drop the global `replace(/\\n{3,}/g, '\\n\\n')` pass. It would collapse blank lines inside fenced code blocks / tool output, mutating verbatim message content. Each section already pushes its own controlled trailing blank line, so the global normalisation was both redundant and harmful. Issue raised by Copilot inline review. 2. archive-export.js exportSessionToMarkdown: showSaveDialog's `defaultUri` was `vscode.Uri.file(fileName)` — `Uri.file()` requires an absolute path. On Windows that resolves to a drive root, on POSIX to `/`, which is a confusing and possibly unwritable default. Anchor it at `os.homedir() + fileName` so the dialog opens somewhere the user expects. 3. session-store.js archive(): only set `archived = true` when a file was actually written. Previously, both `exportSessionToMarkdown` throwing AND returning null (user cancelled the save dialog) would still hide the session from the sidebar even though nothing was archived — leaving the UI inconsistent with disk state. New contract: - returns absolute path → hide + show toast - returns null (cancel) → keep visible, no toast - throws (fs error) → keep visible, show error message 4. archive-export.js _uniquePath: replace the fs.access pre-check with `fs.open(candidate, 'wx')` (exclusive create). Closes the TOCTOU window where two concurrent archive clicks in the same second could both pass the existence check and one would overwrite the other. The placeholder zero-byte file is then overwritten by the subsequent fs.writeFile. Also added a comment to `_safeTitle` explaining the regex classes (reserved-on-Windows set + C0 control codes + leading-dot strip), addressing a readability note from the AI reviewer. Build verified locally; no behavioural change outside the four fixes.
- _writeUnique: write through the exclusive handle so a failed write no longer leaves a zero-byte placeholder behind (closes Copilot comment on _uniquePath placeholder leak). - Replace ad-hoc workspace-relative path logic in _notifyArchived with the multi-root + longest-prefix-aware findContainingFolder() helper from src/utils/paths.js. - Move the 'archive path escapes workspace' error string into i18n (archiveErrEscape) so ZH users no longer see an English assertion in the failure toast. - Drop the self-referential 'PR #166 review' rationale comments left over from the previous round; keep only the substantive 'why'.
- archive-export: sanitize session.title for Markdown `#` heading (collapse newlines/control chars) - archive-export: distinguish 'user cancelled folder picker' (sentinel) from 'no workspace open' (null) so cancel no longer falls back to showSaveDialog - session-store: wrap Open File / Reveal in Explorer handlers in try/catch and surface failures as a non-fatal toast (archiveOpenFailed i18n key)
- _safeTitle: strip trailing space/dot so Win32 path normalisation can't silently mutate the on-disk filename or defeat the collision counter - _frontmatter: always quote string scalars so titles/models that look like YAML keywords (true/null/2026-05-26/123) aren't coerced into bool/date/null/number - _notifyArchived: drop redundant Promise.resolve() wrapper around showInformationMessage (it already returns a thenable); attach rejection handler as the second .then() arg
Round 4 review (#166): - _pickWorkspaceRoot: drop sessionWs shortcut. session.ws is always folder[0] so the shortcut silently bypassed the picker in multi-root workspaces. - _frontmatter: add provider (from config), promptTokens / completionTokens / totalTokens (from session.totals).
#167) Track jobIds started by run_shell_bg within the current run in run._sessionStartedBgJobs, and refuse to take the BG_WAIT_SKIPPED_MODEL_DONE early exit while any of them is still alive. Previously the model could defuse the wait loop by emitting a single closing sentence (e.g. 'I'll tell you when it's done'), leaving the just-spawned background task orphaned with no one listening for its end event. Also restate the run_shell_bg tool hint honestly so the model understands it cannot end the turn via a verbal promise while a bg job is live. Refs #167
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
|
|
||
| # scratch files used by AI review triage | ||
| .tmp-*.json |
There was a problem hiding this comment.
新增的 .tmp-*.json 文件可能会导致敏感数据泄露，建议在版本控制中忽略此类临时文件。此外，确保这些临时文件不会被错误地上传到生产环境中。请考虑使用更安全的文件命名规则或在文档中明确说明这些文件的用途和处理方式。
| if (payload.jobId) run._sessionStartedBgJobs.delete(payload.jobId); | ||
| }; | ||
| onBgJobEnded(_bgJobEndHandler); | ||
|
|
There was a problem hiding this comment.
在这一部分代码中，增加了对背景作业的跟踪和管理，但需要注意以下几点：
- 安全性：确保
run._sessionStartedBgJobs中的 jobId 不会被外部输入操控，避免潜在的命令注入风险。建议对 jobId 进行验证。 - 异常处理：在处理
payload时，建议增加对payload.jobId的存在性检查，避免空指针异常。 - 内存管理：虽然有清理机制，但需要确保在高并发情况下不会导致内存泄漏，建议定期检查和清理无效的 jobId。
| }); | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
在这一部分代码中，增加了对背景作业的状态检查，但存在以下问题：
- 安全性：确保
myBgJobs()返回的作业列表不包含敏感信息，避免信息泄露。 - 异常处理：在
Logger.info调用前，建议检查jobs和sessionStartedJobs的内容，确保它们不为空，以避免潜在的异常。 - 性能：在高并发情况下，
myBgJobs()的调用可能会影响性能，建议考虑缓存机制或优化查询逻辑。
| return uri.fsPath; | ||
| } | ||
|
|
||
| module.exports = { exportSessionToMarkdown, renderSessionMarkdown }; |
There was a problem hiding this comment.
- 安全性：在
_writeUnique函数中，虽然使用了fs.open(..., 'wx')来防止竞争条件，但在高并发情况下，仍然可能会出现文件名冲突的情况。建议增加更强的锁机制或使用数据库来管理文件名。 - 异常处理：在
exportSessionToMarkdown函数中，虽然有对文件系统错误的处理，但在某些情况下（如fs.mkdir或fs.writeFile），可能会抛出未处理的异常，建议使用try-catch块来捕获这些异常并进行适当处理。 - 代码风格：整体代码风格较为一致，但在某些地方（如注释）可以进一步优化，使其更简洁明了。
| const { t, tf } = require('../utils/i18n'); | ||
|
|
||
| // ─── Orphan tool_calls sanitizer ─────────────────────────────────────────── | ||
| // Removes ANY incomplete assistant{tool_calls} group from a message array, |
There was a problem hiding this comment.
在引入 tf 函数时，请确保它的来源和用途是安全的，避免潜在的命令注入风险。建议检查 ../utils/i18n 中的实现，确认 tf 的输入不会被恶意操控。
| }; | ||
| if (tcId) { | ||
| ctx.onStreamDelta = (delta) => { | ||
| if (!delta) return; |
There was a problem hiding this comment.
在这段代码中，ctx.registerBgJob 函数的实现存在潜在的空指针异常风险。虽然在函数开头进行了 if (!jobId || !run) return; 的检查，但在 run._sessionStartedBgJobs 的赋值操作中，如果 run 为 null，将会导致异常。建议在访问 run._sessionStartedBgJobs 之前，确保 run 不为 null。此外，建议对 jobId 的有效性进行更严格的检查，以防止无效值的传入。
| try { ctx.registerBgJob && ctx.registerBgJob(jobId); } catch {} | ||
| } | ||
|
|
||
| // ── Early-failure capture: many commands fail within ~1–2 s (missing |
There was a problem hiding this comment.
在这里，虽然使用了 try-catch 来捕获可能的异常，但没有对异常进行处理或记录，这可能导致潜在的问题被忽略。建议至少记录异常信息，以便后续排查。同时，ctx.registerBgJob 的存在性检查虽然有效，但如果 ctx 为 null 或 undefined，仍然可能导致空指针异常。建议在调用前增加对 ctx 的有效性检查。
| `Do NOT tell the user "I'll notify you when it's done" in this branch — there is no automatic notification.`, | ||
| ].join('\n'), | ||
| `To cancel: ask the user to close the terminal named "${jobId}".`, | ||
| ].join('\n'), |
There was a problem hiding this comment.
在这里，提示信息的内容需要注意，尤其是关于用户交互的部分。建议明确告知用户在特定情况下不应使用某些命令，避免用户误操作。同时，提示信息中提到的 'agent-loop' 和 'system-reminder' 可能会引起用户的困惑，建议使用更清晰的术语或提供额外的上下文信息。此外，确保这些提示信息不会引入安全隐患，例如泄露内部实现细节。
| archiveThoughtsLabel: 'Reasoning', | ||
| }; | ||
|
|
||
| const ZH = { |
There was a problem hiding this comment.
新增的归档相关信息中，archiveErrEscape 提到的 "解析后的路径超出了工作区" 可能导致路径穿越漏洞。建议在处理路径时使用安全库（如 path.resolve）来确保路径在工作区内。此外，archiveFailed 和 archiveOpenFailed 中的 {msg} 可能会泄露内部错误信息，建议对错误信息进行适当的处理和过滤，以避免信息泄露。
| archiveThoughtsLabel: '思维链', | ||
| }; | ||
|
|
||
| function t(key) { |
There was a problem hiding this comment.
同样，archiveErrEscape 的中文翻译也存在路径穿越的风险。建议在路径处理时确保安全性。对于 archiveFailed 和 archiveOpenFailed 的错误信息，需注意避免泄露敏感信息，建议进行适当的处理。
There was a problem hiding this comment.
Pull request overview
This PR primarily targets issue #167 by preventing the agent loop from ending a turn early when a background shell job started in the current run is still active, ensuring the eventual bg-job end event is still observed and injected back to the model. However, it also introduces a session archive → Markdown export feature (issue #165), which expands scope beyond the PR title/description.
Changes:
- Track bg jobs started within the current agent run and block
BG_WAIT_SKIPPED_MODEL_DONEearly-exit while any of those jobs are still active. - Allow
run_shell_bgto register newly-started jobIds into the current run viactx.registerBgJob(jobId). - Add session archive-to-Markdown export plumbing (new exporter module, SessionStore flow, i18n strings) and a
.gitignoreentry.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/chat/agent-loop.js | Adds run-scoped tracking to prevent early-exit while run-started bg jobs are still running. |
| src/chat/tool-executor.js | Injects ctx.registerBgJob(jobId) so tools can register bg jobs to the current run. |
| src/tools/bg-shell.js | Registers bg jobs with agent-loop context and updates hint text to reflect actual behavior. |
| src/chat/session-store.js | Changes “Archive” from soft-hide to Markdown export + hide; adds post-save notification. |
| src/chat/archive-export.js | New module to render a session to Markdown and write to a workspace path safely. |
| src/utils/i18n.js | Adds EN/ZH strings for archive/export UX. |
| .gitignore | Ignores AI triage scratch JSON files. |
结论：需修改
Comment on lines
119
to
125
| if (usedSI) { | ||
| addActiveBgJob(jobId, ctx.sessionId || null); | ||
| // Issue #167: tell agent-loop this jobId was started in THIS run, so | ||
| // its BG_WAIT_SKIPPED_MODEL_DONE early-exit guard refuses to end the | ||
| // turn while the freshly spawned job is still alive. | ||
| try { ctx.registerBgJob && ctx.registerBgJob(jobId); } catch {} | ||
| } |
| const fs = require('fs/promises'); | ||
| const { t } = require('../utils/i18n'); | ||
|
|
||
| const ARCHIVE_SUBDIR = '.deepcopilot/archives'; |
Comment on lines
+371
to
+373
| * New behaviour: render the session to Markdown and write it under | ||
| * <workspace>/.deepcopilot/archives/yyyyMMdd-HHmmss-<title>.md | ||
| * Then perform the original soft-hide so the session leaves the sidebar. |
Comment on lines
+363
to
+370
| /** | ||
| * "Archive" a session — issue #165. | ||
| * | ||
| * Original behaviour (pre-#165) was a soft-hide toggle: flip `archived`, | ||
| * disappear from the sidebar list, data stays in globalState. Users | ||
| * reported that this didn't match the menu label's promise: nothing was | ||
| * actually *archived* anywhere they could see, find, or grep. | ||
| * |
Comment on lines
+424
to
+426
| * Show the bottom-right toast with "Open" / "Reveal in Explorer" buttons. | ||
| * Path display is workspace-relative when possible so users see | ||
| * ".deepcopilot/archives/20260526-….md" |
| // Issue #167: tell agent-loop this jobId was started in THIS run, so | ||
| // its BG_WAIT_SKIPPED_MODEL_DONE early-exit guard refuses to end the | ||
| // turn while the freshly spawned job is still alive. | ||
| try { ctx.registerBgJob && ctx.registerBgJob(jobId); } catch {} |
1. Register jobId in run-scoped _sessionStartedBgJobs BEFORE publishing to addActiveBgJob, closing a race where an extremely short-lived command could fire bg-job-end between the two calls, leaving a stale entry in the run set. 2. Stop swallowing ctx.registerBgJob errors. Guard with typeof check and log via Logger.info (Logger has no warn today) so regressions surface in debug-logs instead of silently losing the keep-alive signal. 3. Rename archive directory from .deepcopilot/archives to .deep-copilot/archives to match the rest of the codebase's workspace-artifact convention (plans, memory, logs). Updates docstrings and toast example path accordingly.
…-copilot/DeepCopilot into fix/bg-shell-keep-alive-167
Comment on lines
+23
to
+26
| // PR #168 review: align with the rest of the codebase's workspace-artifact | ||
| // convention (`.deep-copilot/plans`, `.deep-copilot/memory.md`, | ||
| // `.deep-copilot/logs`). Previously this lived under `.deepcopilot/archives`, | ||
| // which produced an inconsistent second hidden directory in user workspaces. |
| // convention (`.deep-copilot/plans`, `.deep-copilot/memory.md`, | ||
| // `.deep-copilot/logs`). Previously this lived under `.deepcopilot/archives`, | ||
| // which produced an inconsistent second hidden directory in user workspaces. | ||
| const ARCHIVE_SUBDIR = '.deep-copilot/archives'; |
Comment on lines
+120
to
+123
| // Issue #167 follow-up (PR #168 review): register the jobId in the | ||
| // run-scoped set BEFORE publishing to the cross-session active-job | ||
| // registry. If the order were reversed, an extremely short-lived | ||
| // command could fire its `bg-job-end` event between addActiveBgJob() |
Collaborator
Author
|
The race comment is a false positive on this commit — the order in the current code is |
Comment on lines
+131
to
+134
| if (typeof ctx.registerBgJob === 'function') { | ||
| try { | ||
| ctx.registerBgJob(jobId); | ||
| } catch (e) { |
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.
概述
修复 #167 报告的核心问题——
run_shell_bg启动的长任务在 agent-loop 的BG_WAIT_SKIPPED_MODEL_DONE分支被模型“一句安抚话”提前结束本轮，导致最终结束事件无人监听、用户永远收不到反馈。
本 PR 落地 issue 中 阶段 1（必做止血） 的全部改动，不动 UI、不改 schema、
不引入新工具。
修改内容
src/chat/agent-loop.jsrun._sessionStartedBgJobs: Set<jobId>，只收纳本 run内通过
run_shell_bg启动的 jobId。_bgJobEndHandler收到结束事件时把对应 jobId 从集合里清除，集合大小有界。BG_WAIT_SKIPPED_MODEL_DONE判据：模型说收尾话且无 pending 事件 且_sessionStartedBgJobs与当前 active bg jobs 没有交集时才允许 break。dev server / watcher 这类上一轮启动的长任务行为不变；本轮新启动的
bg 任务会被强制值守。
sessionStartedJobs便于回归验证。src/chat/tool-executor.jsctx加registerBgJob(jobId)回调，写入当前 run 的_sessionStartedBgJobs。run为空时（一次性脚本/测试场景）no-op。src/tools/bg-shell.jsaddActiveBgJob之前先调用ctx.registerBgJob(jobId)，把 jobId同步登记到 agent-loop（注册顺序很关键，详见下文“评审反馈处理”）。
status: 'running'分支的 hint 文案改为对系统行为如实描述：明确告诉模型“你不能用‘说完再走’的方式提前结束 turn；loop 会保持你值守，结束时
会以
<system-reminder>再次喂给你”。SI 不可用分支同样补一句“不要承诺‘完成后通知你’，没有自动通知”。
顺手改进（user-visible）
<workspace>/.deepcopilot/archives→<workspace>/.deep-copilot/archives。与代码库其它产物（
.deep-copilot/plans、.deep-copilot/memory.md、.deep-copilot/logs）约定对齐，避免在用户工作区里出现第二个隐藏目录。本 PR 顺路改了
archive-export.js的
ARCHIVE_SUBDIR常量、session-store.js的两处 docstring 路径示例；toast显示路径与
findContainingFolder解析逻辑无需改动。归档功能本身由 feat(session): archive action exports session to Markdown (#165) #166 引入，已合入 main。
评审反馈处理
addActiveBgJob()与
ctx.registerBgJob()之间完成；若顺序倒置，end-handler 会对一个尚未 add 的set 执行 delete，并在随后的 register 中留下 stale 条目。已把
registerBgJob挪到
addActiveBgJob之前，关闭这个窗口。try {} catch {}静默丢失注册失败信号。现以
typeof === 'function'守卫，失败走Logger.info('BG_JOB_REGISTER_FAILED', ...)入 debug-log（Logger 目前只暴露.info，tag 自身携带 severity 关键字便于 grep）。验收用例（U1，本 PR 范围）
在 SI 可用的终端跑：
然后让模型回一句话即结束工具调用。
BG_WAIT_SKIPPED_MODEL_DONE，run 立即结束，30s 后无任何BG_JOB_END_INJECTED。BG_WAIT_SKIPPED_MODEL_DONE（或仅在 jobId 与sessionStartedJobs无交集时记录）；~30s 后看到BG_JOB_END_INJECTED，模型收到 system-reminder 并给出最终回复。
未在本 PR 中处理（留作后续 issue）
pendingBgEvents持久化，跨 VS Code 重启投递。兼容性 / 风险
.deepcopilot/archives/下的旧文件保留原位，不做迁移。
预期的“值守”行为，不是 regression。
验证
npm run build：成功。npm run lint：0 errors（既有 indent/quote 风格 warning 与本 PR 无关，未触发任何新错误）。
Closes #167