feat(vscode): surface ACP background notifications#4358
Merged
Conversation
Contributor
📋 Review SummaryThis PR implements background notification handling for VSCode ACP mode, enabling background task completions and model follow-ups to be properly displayed as discrete chat messages. The implementation is well-structured with comprehensive test coverage, proper serialization of notification draining, and thoughtful handling of edge cases. The changes are additive and non-breaking. 🔍 General Feedback
🎯 Specific Feedback🟡 High
🟢 Medium
🔵 Low
✅ Highlights
|
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
requested changes
May 20, 2026
wenshao
requested changes
May 20, 2026
DragonnZhang
enabled auto-merge (squash)
May 21, 2026 05:49
The hardening commit (42a537c) added Session#dispose() (called by QwenAgent on cancel/end paths) and made the Session constructor register notification callbacks on three Config registries. The existing test mocks weren't updated, breaking 26 tests on ubuntu/macOS CI: - acpAgent.test.ts (19) and acpAgent.worktree.test.ts (3) hit "session.dispose is not a function" - Session.worktree.test.ts (4) hit "this.config.getBackgroundTaskRegistry is not a function" Add dispose: vi.fn() to all Session mockImplementation blocks and provide no-op getBackgroundTaskRegistry / getMonitorRegistry / getBackgroundShellRegistry stubs in Session.worktree.test.ts's mockConfig. No production changes.
Collaborator
Author
|
Pushed The hardening commit
This commit is test-only — adds Validated locally:
(The one |
wenshao
requested changes
May 21, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Collaborator
Author
|
Pushed Changes:
Validation:
Both remaining review threads have been replied to and resolved. CI is running on the new commit. |
wenshao
requested changes
May 21, 2026
wenshao
requested changes
May 22, 2026
wenshao
reviewed
May 23, 2026
…output - Pass entry.error through stripDisplayControlChars before XML escaping to prevent ANSI escape sequences and terminal control codes in shell error messages from leaking into task-notification XML - Strip Unicode bidirectional override characters (U+202A-U+202E and U+2066-U+2069) in stripDisplayControlChars to prevent malicious shell output from reordering surrounding text in terminals and IDEs Addresses review feedback from PR #4358.
DragonnZhang
commented
May 25, 2026
wenshao
previously approved these changes
May 25, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Second-opinion review with qwen3.7-max at commit 3828bf11b. Build clean, 214/214 tests pass, CI green 30/30. No new high-confidence issues found — the prior 5 review rounds have thoroughly covered this PR's critical surface area, and all prior Critical findings have been addressed across commits 42a537c34, c157d58c1, 2a11323f8, and 3828bf11b. Five open comments from prior rounds (dispose race, tool-call security, code duplication, test gaps, queue eviction logging) remain for the author to address. LGTM. ✅ — qwen3.7-max via Qwen Code /review
When the background notification queue reaches its maximum capacity (MAX_NOTIFICATION_QUEUE = 20), oldest items are evicted to make room for new ones. Previously this happened silently with no debug logging, making it difficult to diagnose missing background completion notices. Now logs a warning when items are evicted, including the task ID and kind (agent/monitor/shell) to aid in debugging production issues.
wenshao
requested changes
May 25, 2026
dispose() was synchronous: it aborted the notification abort controller,
reset notificationProcessing, and returned immediately. But an in-flight
#drainNotificationQueue's finally block calls void this.#drainCronQueue()
after dispose() returns. The cron queue was not cleared and the drain
methods had no disposed-state check, so #drainCronQueue could start
processing cron items on a session whose registries had already been
unregistered. After session replacement (e.g. /clear or session reload),
orphaned cron prompts would execute on the old session — phantom API
calls, potential rate limiting, model responses with no visible consumer.
- Add a `disposed` flag set by dispose().
- Clear cronQueue in dispose() alongside notificationQueue.
- Early-return from #drainCronQueue and #drainNotificationQueue when
disposed, so the finally-block chain from either drain cannot revive
the other on a disposed session.
Adds two unit tests under describe('dispose'):
- queues cleared, disposed flag set, callbacks unregistered;
- dispose() is idempotent (no throw, no surprise re-registration).
…tion Previously, when the CLI emitted a `background_notification` (or `background_notification_response`) `agent_message_chunk`, the webview provider persisted it via `conversationStore.addMessage(getCurrentConversationId(), message)`. `getCurrentConversationId()` returns whichever conversation is currently active in the panel — not the conversation that owns the ACP session that produced the notification. If the user switched conversations between triggering a background task in conversation A and the notification being delivered (e.g. a long- running tool turn returning after the user moved to conversation B), the notification — whose content was generated from conversation A's full chat history — was persisted into conversation B's message store. That is a cross-conversation data integrity / leakage issue: A's code snippets, proprietary prompts, and tool outputs would surface in B's persisted history on subsequent loads. This change forwards the originating ACP session id on the message: - Add `sessionId?: string` to `ChatMessage` so discrete messages can carry their originating session id end-to-end. - In `QwenSessionUpdateHandler.handleSessionUpdate`, populate `sessionId` from the incoming `SessionNotification.sessionId` for any `agent_message_chunk` that is emitted as a discrete message (background notifications and other qwenDiscreteMessage updates). - In `WebViewProvider.onMessage`, prefer `message.sessionId` over `conversationStore.getCurrentConversationId()` when persisting background notifications. The conversation store is keyed by the ACP session id (see `SessionMessageHandler.updateCurrentConversationId`), so `message.sessionId` is the correct key. Tests: - `qwenSessionUpdateHandler.test.ts`: existing discrete-message test now asserts `sessionId` is forwarded; added two new tests that explicitly cover (a) sessionId pass-through and (b) defensive omission when the notification has no sessionId. - All existing `WebViewProvider` and `useWebViewMessages` tests still pass; the active-conversation fallback path is unchanged when no sessionId is present on the message. Validation: - `cd packages/vscode-ide-companion && npx vitest run \ src/services/qwenSessionUpdateHandler.test.ts \ src/webview/providers/WebViewProvider.test.ts \ src/webview/hooks/useWebViewMessages.test.tsx` → 74/74 pass - `npm run typecheck` → clean across cli/core/sdk/webui - `npx eslint` on all changed files → clean Addresses PR #4358 review thread `PRRT_kwDOPB-92c6Ej6IP`.
`BackgroundShellRegistry.stripDisplayControlChars` (private) stripped C0/C1 controls AND Unicode bidi overrides (U+202A-U+202E, U+2066-U+2069). The same-named local function in `monitorRegistry.ts` stripped only C0/C1 — no bidi stripping. Both registries push display text into the same Session notification queue via `#enqueueBackgroundNotification`, so monitor event lines containing bidi controls slipped through unsanitized while shell notifications were properly cleaned, leaving a Trojan-Source-style spoofing path through the monitor surface. Extract a single `stripDisplayControlChars` to `packages/core/src/utils/terminalSafe.ts`, document the threat model (CVE-2021-42574) and the exact stripped code-point ranges, and have both registries import it. The shell path is unchanged; the monitor path now also strips LRE/RLE/PDF/LRO/RLO and LRI/RLI/FSI/PDI. Add `terminalSafe.test.ts` (11 tests) locking in the behavior: - TAB and printable text preserved - C0 (except TAB), C1, DEL handling - bidi U+202A-U+202E and U+2066-U+2069 stripped - adjacent code points (U+2029, U+202F, U+2065, U+206A) preserved - canonical Trojan-Source `/* … } if (isAdmin) … */` payload neutralized - idempotency All pre-existing `backgroundShellRegistry.test.ts` (32) and `monitorRegistry.test.ts` (53) tests still pass; workspace `tsc --noEmit` and ESLint are clean. Addresses review feedback on PR #4358.
… session replacement - Test dispose() aborts active notificationAbortController and nulls it - Test dispose() prevents post-dispose notification queue drain - Test rewindToTurn rejects when notificationProcessing is true - Test rewindToTurn rejects when notificationAbortController is active - Test restoreHistory rejects when notificationProcessing is true - Test restoreHistory rejects when notificationAbortController is active - Test loadSession disposes existing session on same-ID replacement
Fix TS2339 error where lastSessionMock type was missing the dispose property, causing CI build failures across all platforms.
wenshao
requested changes
May 26, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Additional findings (cannot be mapped to a specific diff line):
[Suggestion] monitorRegistry.ts running-event XML lacks bidi stripping (defense-in-depth)
monitorRegistry.ts:516 builds <result>${escapeXml(eventLine)}</result> without stripDisplayControlChars, while the terminal notification path at line 564 and backgroundShellRegistry.ts both apply stripDisplayControlChars before escapeXml. The ACP session callback filters status: 'running', but sub-agent callbacks in agent.ts:726 and background-agent-resume.ts:682 receive ALL monitor notifications including running events — bidi override characters (U+202A–U+202E, U+2066–U+2069) in a running-event <result> reach sub-agent models unstripped.
`<result>${escapeXml(stripDisplayControlChars(eventLine))}</result>`,
[Suggestion] Test coverage gaps for cross-conversation persistence and shell notification emission
Two high-value code paths lack test coverage:
-
WebViewProvider.onMessageconversation persistence (WebViewProvider.ts:252-283) — the cross-conversation data leak fix routes background notification messages throughconversationStore.addMessageusingmessage.sessionId. The mockonMessage = vi.fn()in the test file does not capture the callback, so no test exercises:message.sessionIdpresent (should use it), absent (should fall back togetCurrentConversationId()),conversationIdnull (should skip), oraddMessagerejection. -
BackgroundShellRegistryfail/cancel notification emission (backgroundShellRegistry.ts:207,217) —emitTerminalNotification()is only tested forcomplete(line 67 of the test file). Thefail()path's XML payload (<result>Error: ${escapeXml(stripDisplayControlChars(entry.error))}</result>) — the security sanitization fix from review round 6 — is entirely untested.
[Nice to have] responseText += quadratic string concatenation in notification streaming
Session.ts:1783 accumulates notification response text via responseText += part.text in the streaming loop. Unlike the normal prompt and cron paths which emit each chunk individually, the notification path buffers all text to produce a single discrete message. For long responses, each += creates a new string (JS string immutability), yielding O(n²). Accumulate into an array and join('') once instead.
— qwen3.7-max via Qwen Code /review
…ispose - Remove backgroundTask/qwenDiscreteMessage/source from REWRITE_META_EXCLUDED_KEYS so rewritten messages retain routing metadata needed by qwenSessionUpdateHandler for discrete-message classification and conversation persistence. - Add cron subsystem cleanup (abort controller, processing flag, completion promise) to Session.dispose() to prevent in-flight cron turns from continuing after session replacement. - Update tests to match new behavior.
wenshao
reviewed
May 26, 2026
Collaborator
🔍 Maintainer Verification Report — PR #4358Branch: 1. Build & Static Analysis
2. Unit Tests (348 total)
3. tmux Real-User E2E TestRan the built bundle (
Key E2E observations:
4. Code Quality Review NotesPositive signals:
Pre-existing issue (not introduced by this PR):
5. Summary
Recommendation: ✅ Ready to merge All verification checks pass. The core feature (background notification drain + model follow-up) works as designed in real-user testing. Session lifecycle management (dispose, notification guards, cross-conversation isolation) is robust. |
LaZzyMan
reviewed
May 26, 2026
LaZzyMan
left a comment
LaZzyMan
left a comment
Collaborator
There was a problem hiding this comment.
Review
This routes background agent / monitor / shell completions into discrete VSCode chat messages via a serialized notification queue on the ACP session, runs each notification as its own mini-turn so the model can follow up, and propagates an end-of-turn source so the per-conversation idle toast is suppressed for synthetic notification turns. The cli-side queue draining, abort wiring, and dispose() hygiene are sound; the terminalSafe.ts extraction is a real refactor that also extends the bidi (Trojan Source) defense to the shell-registry notification surface. The webview-side persistence routes the message to the conversation that owns the work via the forwarded ACP session id. One residual gap on the webview-display side.
1. Background notification renders in the active chat panel regardless of origin conversation (severity: medium · confidence: very high)
The web view provider correctly persists a background-notification message to the originating conversation's store via the forwarded session id, but the persistence branch only handles storage — the message dispatch to the visible web view right after it is unconditional. So if the user triggers a long-running background task in conversation A, switches to conversation B, then the task completes, the message is persisted to A's history (the intended behavior) but also displayed in B's open transcript. Gating the dispatch on the originating session id matching the active conversation — or having the web view drop foreign-conversation messages on receive — closes the leak.
Verdict
COMMENT — Serialization, abort wiring, dispose hygiene, terminal-safety extraction, and discrete-message metadata are all done well. The one substantive bug is local to the webview provider and small to fix; nothing blocking.
wenshao
approved these changes
May 26, 2026
Collaborator
PR #4358 本地真实测试报告
结论PASS — CLI 在
方法写了一份最小 ACP JSON-RPC 客户端 (
真实运行的协议时序（Run 2 关键节选）
Run 1 — Cancel pathdrain in-flight 时 正是 Run 2 摘要{
"discreteMessages": [
{ "source": "background_notification",
"backgroundTask": { "taskId": "bg_55785af3", "status": "completed", "kind": "shell" } },
{ "source": "background_notification",
"backgroundTask": { "taskId": "bg_88590491", "status": "completed", "kind": "shell" } }
],
"endTurnNotifications": [
{ "reason": "end_turn", "source": "background_notification" }
],
"toolCallCount": 4,
"sessionUpdateCount": 69,
"errors": []
}给作者 / Reviewer 的几条小观察（不阻塞合并）
没有覆盖到的部分（坦白）
复现 driver.mjs展开查看 ACP 客户端驱动器（200 行）// driver.mjs — minimal ACP client for verifying PR 4358 wire behavior.
// Usage: node driver.mjs <qwen-cli-entry-js> "<prompt>" --cwd <dir> --log <path>
import { spawn } from 'node:child_process';
import { createInterface } from 'node:readline';
import { writeFileSync, appendFileSync } from 'node:fs';
import path from 'node:path';
const [cliEntry, userPrompt, ...rest] = process.argv.slice(2);
let cwd = process.cwd();
let logPath = './driver.log';
for (let i = 0; i < rest.length; i++) {
if (rest[i] === '--cwd') cwd = rest[++i];
else if (rest[i] === '--log') logPath = rest[++i];
}
if (!cliEntry || !userPrompt) {
console.error('Usage: node driver.mjs <cli-entry> <prompt> [--cwd <dir>] [--log <path>]');
process.exit(2);
}
writeFileSync(logPath, '');
const log = (line) => appendFileSync(logPath, line + '\n');
const child = spawn(process.execPath, [cliEntry, '--acp', '--approval-mode=yolo'], {
cwd,
stdio: ['pipe', 'pipe', 'pipe'],
env: { ...process.env, NO_COLOR: '1', FORCE_COLOR: '0', TERM: 'dumb' },
});
child.stderr.on('data', (c) => log('[stderr] ' + c.toString('utf8').trimEnd()));
let nextId = 1;
const pending = new Map();
const findings = {
discreteMessages: [],
endTurnNotifications: [],
toolCalls: [],
sessionUpdates: [],
errors: [],
};
const summaryPath = path.join(path.dirname(logPath), 'driver-summary.json');
const emit = (o) => { const s = JSON.stringify(o); log('[->cli] ' + s); child.stdin.write(s + '\n'); };
const request = (method, params) => {
const id = nextId++;
return new Promise((resolve, reject) => {
pending.set(id, { resolve, reject });
emit({ jsonrpc: '2.0', id, method, params });
});
};
const respond = (id, result, error) =>
emit(error ? { jsonrpc: '2.0', id, error } : { jsonrpc: '2.0', id, result });
createInterface({ input: child.stdout, crlfDelay: Infinity }).on('line', async (line) => {
if (!line.trim()) return;
log('[<-cli] ' + line);
const msg = JSON.parse(line);
if (msg.id !== undefined && (msg.result !== undefined || msg.error !== undefined)) {
const entry = pending.get(msg.id);
if (entry) { pending.delete(msg.id); msg.error ? entry.reject(msg.error) : entry.resolve(msg.result); }
return;
}
if (msg.id !== undefined && msg.method) {
// agent → us request
try {
let result = null;
const { default: fs } = await import('node:fs/promises');
switch (msg.method) {
case 'fs/read_text_file':
result = { content: await fs.readFile(msg.params.path, 'utf8') };
break;
case 'fs/write_text_file':
await fs.mkdir(path.dirname(msg.params.path), { recursive: true });
await fs.writeFile(msg.params.path, msg.params.content, 'utf8');
break;
case 'session/request_permission': {
const opts = msg.params.options || [];
const allow = opts.find((o) => /allow|approve|once/i.test(o.optionId || '')) || opts[0];
result = { outcome: { outcome: 'selected', optionId: allow?.optionId || 'allow' } };
break;
}
case 'terminal/create': case 'terminal/output':
case 'terminal/wait_for_exit': case 'terminal/kill': case 'terminal/release':
result = {};
break;
default:
respond(msg.id, null, { code: -32601, message: 'unhandled ' + msg.method });
return;
}
respond(msg.id, result);
} catch (e) {
respond(msg.id, null, { code: -32000, message: e.message });
}
return;
}
// agent → us notification
if (msg.method === 'session/update') {
findings.sessionUpdates.push(msg.params);
const meta = msg.params?.update?._meta;
const text = msg.params?.update?.content?.text;
if (meta?.qwenDiscreteMessage === true) {
findings.discreteMessages.push({
source: meta.source, text, backgroundTask: meta.backgroundTask,
});
log(`[driver] *** DISCRETE: ${meta.source} bg=${JSON.stringify(meta.backgroundTask)}`);
}
const u = msg.params?.update?.sessionUpdate;
if (u === 'tool_call' || u === 'tool_call_update') findings.toolCalls.push(msg.params.update);
} else if (msg.method === '_qwencode/end_turn') {
findings.endTurnNotifications.push(msg.params);
log(`[driver] *** END_TURN: ${JSON.stringify(msg.params)}`);
}
});
(async () => {
await request('initialize', {
protocolVersion: 1,
clientCapabilities: { fs: { readTextFile: true, writeTextFile: true }, terminal: false },
});
const sess = await request('session/new', { cwd, mcpServers: [] });
try {
await request('session/prompt', {
sessionId: sess.sessionId,
prompt: [{ type: 'text', text: userPrompt }],
});
} catch (err) {
findings.errors.push({ phase: 'session/prompt', error: err });
}
// wait up to 120s for the background notification end_turn
const deadline = Date.now() + 120000;
while (Date.now() < deadline) {
if (findings.endTurnNotifications.some(
(p) => p?.source === 'background_notification' && p?.reason !== 'cancelled'
)) break;
await new Promise((r) => setTimeout(r, 500));
}
writeFileSync(summaryPath, JSON.stringify(findings, null, 2));
child.kill('SIGTERM');
setTimeout(() => process.exit(0), 100);
})().catch((e) => { log('[driver] err: ' + e.stack); child.kill('SIGTERM'); });复现命令git fetch origin pull/4358/head:pr-4358
git worktree add /tmp/pr4358 pr-4358
cd /tmp/pr4358 && npm install && npm run build
mkdir -p /tmp/pr4358-verify/workdir
# 把上面那段 driver.mjs 保存到 /tmp/pr4358-verify/driver.mjs，然后:
node /tmp/pr4358-verify/driver.mjs /tmp/pr4358/packages/cli/dist/index.js \
'Use the run_shell_command tool to run "echo HELLO_BG; sleep 1; echo DONE_BG" with is_background set to true. Just kick it off and end your turn.' \
--cwd /tmp/pr4358-verify/workdir --log /tmp/pr4358-verify/run.log &
# 看摘要
until grep -q '"reason":"end_turn".*"source":"background_notification"' /tmp/pr4358-verify/run.log; do sleep 2; done
jq '{discreteMessages, endTurnNotifications, toolCallCount, errors}' /tmp/pr4358-verify/driver-summary.json |
wenshao
approved these changes
May 26, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
R9 review at 000072f. The R8 Critical (REWRITE_META_EXCLUDED_KEYS stripping routing metadata) and Suggestion (cron dispose cleanup) are both correctly addressed. 236 tests pass across 8 suites, CI green 18/18. 6 low-confidence Suggestions for human review (monitorRegistry.ts:516 incomplete bidi fix, dispose() notificationCompletion asymmetry, staleness guard semantic change, dispose() not awaiting in-flight work, drain early-return logging, end-turn error log level). No blocking issues. — qwen3.7-max via Qwen Code /review
Collaborator
|
Thanks for the thorough work on this, @DragonnZhang — the change looks solid and CI is green. 🎉 The branch has fallen behind Thanks! 🙏 |
Resolve backgroundShellRegistry conflict in favor of main's richer shell notification implementation (output-tail, command truncation, pid/cwd/ exitCode meta); drop the branch's superseded notification format and its two now-obsolete unit tests, which are covered by main's notifications suite.
- qwenSessionUpdateHandler: do not treat the rewritten summary (_meta.rewritten) as a discrete message; the original chunk already carries qwenDiscreteMessage, so persisting both double-stored each background-notification response. Add a 'rewritten' flag to SessionUpdateMeta and guard on it. - MessageRewriteMiddleware: document why REWRITE_META_EXCLUDED_KEYS is an empty set (routing metadata must survive rewrites) and keep it as an extension point. - Session.dispose(): null notificationCompletion symmetrically with cronCompletion to avoid leaving a stale promise reference.
tanzhenxin
approved these changes
Jun 8, 2026
tanzhenxin
left a comment
tanzhenxin
left a comment
Collaborator
There was a problem hiding this comment.
Approving — thanks for the thorough turnaround, @DragonnZhang. 🎉
The conflict resolution is clean: you adopted main's emitNotification and dropped the duplicate registry implementation, while keeping the ACP routing wired in Session.ts against main's callback signature — no duplicate symbols, no double-firing. All three review threads are addressed as well:
- double-persist —
rewrittenflag added toSessionUpdateMetaand guarded inqwenSessionUpdateHandler, so the rewritten summary no longer gets stored a second time 👍 - excluded-keys — the empty
REWRITE_META_EXCLUDED_KEYSset now documents why it must stay empty - dispose() —
notificationCompletionnulled symmetrically withcronCompletion
CI is green and shell/agent/monitor completions still route into the discrete-message queue (covered by tests). LGTM 🚀
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
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
_qwencode/end_turnsource propagation, and avoiding idle toasts for background notification turns.Validation
task-notification, sends it to the model, and emits the model response as a discrete assistant message.Waiting for your inputidle toast for that synthetic turn.npm run typecheckpassed;npm run buildpassed. Build still reports an existing warning inpackages/vscode-ide-companion/src/utils/editorGroupUtils.tsabout a missing curly brace.Session.test.ts74/74,qwenSessionUpdateHandler.test.ts17/17,acpConnection.test.ts16/16,WebViewProvider.test.ts43/43.Scope / Risk
_qwencode/end_turnsource metadata is additive and local to the VSCode ACP integration.Testing Matrix
Testing matrix notes:
Linked Issues / Bugs
Background sub-agent task:
Background monitor task:
