From 014e198b64297a4d508f42d29bb467a0b8dea5cc Mon Sep 17 00:00:00 2001 From: ChiGao Date: Wed, 27 May 2026 16:24:59 +0800 Subject: [PATCH 01/36] feat(daemon): ACP Streamable HTTP transport at /acp [RFD #721] (#4472) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(serve): post-merge fixes for #4291 review (7 threads) (#4305) * fix(serve): address qwen-latest review on merged #4291 (7 threads) Seven post-merge findings from the qwen-latest review on #4291, all real. Most are tightening fixes for issues introduced by the earlier rounds of #4291 — the same security / DRY / observability classes the original review surfaced, applied to surfaces that weren't covered initially. #1 (deviceFlow.ts:1179) — late-poll observer closure retained the entire entry by reference (deviceCode/pkceVerifier BrandedSecrets + cancelController) for the lifetime of the daemon if `provider.poll()` never settled. Memory leak + indefinite secret retention. Destructure the four fields the closure actually needs (deviceFlowId, providerId, initiatorClientId, audit sink) so the entry is GC-eligible the moment runPollTick returns. #2 (server.ts) — `callerIsInitiator` was duplicated verbatim across three locations: GET handler, toDeviceFlowStartResponseBody, toDeviceFlowStateBody. The exact bug class #4291 was fixing was "POST and GET diverged on the same redaction policy" — duplicating the gate recreated the preconditions for divergence. Extracted to shared `callerIsDeviceFlowInitiator(view, callerClientId)` helper with the consolidated threat-model JSDoc. All three sites now call the helper. #3 (deviceFlow.ts:1110) — timeout callback constructed two separate `DeviceFlowPollTimeoutError` instances (one for `signal.reason`, one for the wrapper rejection). Each capture its own V8 stack trace, and `signal.reason.stack` would diverge from the caught rejection's stack — confusing for operators inspecting both. Build the sentinel ONCE per timer fire and pass the same instance to both sites. #4 (qwenDeviceFlowProvider.ts:273) — `Error.name` is a freely assignable string property; a hostile fetch wrapper could set `e.name = 'X\n[serve] FAKE LINE\x1b[31m'` to inject log lines or ANSI sequences via the same vector we already closed for `oauthError`. The non-OAuth catch path interpolated `${err.name}` raw. Apply the same `sanitizeForStderr()` helper. #5 (deviceFlow.ts:1551) — on the timeout path, `rawProviderError` is undefined (deliberately, to skip the misleading `provider.poll() threw (raw): ...` audit template), but that left the audit hint field omitted entirely. Operators reading the durable audit trail saw `errorKind: 'upstream_error'` with no signal whether it was a hung IdP or a generic provider failure. Use `result.hint` (which already carries the timeout-specific `provider.poll() timed out after Nms; check IdP connectivity` text built in the catch) so the audit matches the SSE event. #6 (server.ts) — the `QWEN_SERVE_DEBUG` env-var check was inlined in the GET route handler, duplicating the `isServeDebugMode()` helper from `./debugMode.js` that workspaceAgents and workspaceMemory already use. The inline copy also had a dead `?? ''` fallback (the value is guaranteed truthy at that point per the preceding check). Use the canonical helper. #7 (deviceFlow.ts:1217) — late-rejection observer interpolated the raw `lateErr.message` into the audit hint (truncated to 256 bytes, but RFC 8628 `device_code` values fit comfortably in 256 bytes). The provider's catch already uses the `name + length` redaction pattern to prevent WAF-echoed `device_code`/PKCE leaks; the registry layer was undoing that hardening because the same failure settled late. Apply the same `name + length` pattern at the late- rejection site. Tests: - Existing late-rejection test reseeded with a `device-code-secret-*` substring inside the long detail; hard-negative-asserts the seeded secret is absent from the audit + asserts the new `Error (message N bytes; raw suppressed)` shape. - Existing poll-timeout test now also asserts: hint IS defined on the audit (not omitted), hint contains `'timed out after'` / `'check IdP connectivity'`, and `signal.reason instanceof DeviceFlowPollTimeoutError` (proves the single sentinel is shared between abort and reject). - New `sanitizes control characters in attacker-controlled err.name` test in qwenDeviceFlowProvider.test.ts pins the round-4 #4 fix with a hostile `e.name` containing `\n` + `\x1b[31m...`. cli serve 702/702 (was 686, +16 — additional tests imported via the acp-bridge package lift on main); sdk 421/421; typecheck clean across all 4 workspaces; eslint --max-warnings 0 clean on touched files. Refs: #4175, #4255, #4291 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * fix(serve): address deepseek-v4-pro review on #4305 (4 threads) Round-5 fold-in. Four findings from the deepseek-v4-pro review on PR #4305 — all real, three are sister fixes for the same security classes that #4305 already closed at adjacent surfaces. #1 (deviceFlow.ts) — `pollTimedOut` race correctness. The flag was set unconditionally inside the timer callback. If the provider settled the wrapper at 29.9s, `finally` would call `clearScheduled(pollTimer)` — but if the timer callback was already queued for execution before the clear landed (a real possibility in Node's event-loop ordering, even if not always observed in practice), this branch could still run and incorrectly mark `pollTimedOut`. Move the flag assignment to the catch block where the settled cause is unambiguous via `instanceof DeviceFlowPollTimeoutError`. New test pins the negative: provider beats the timeout → no spurious `lost_late_poll_after_timeout` audit even after ticking 2× the ceiling. #2 (deviceFlow.ts) — late-rejection observer interpolated raw `lateErr.name` into the audit hint without sanitization. Same attacker-controlled vector closed at the provider layer for `err.name` in round-4. Route through `sanitizeForStderr`. #3 (deviceFlow.ts) — late-success observer interpolated `latePollResult.kind` directly into the audit template. While the typed shape is `'pending' | 'slow_down' | 'success' | 'error'`, a non-conforming provider could return an arbitrary string. Same log-injection vector. Route through `sanitizeForStderr`. #4 (qwenDeviceFlowProvider.ts → deviceFlow.ts) — `sanitizeForStderr` only stripped ASCII C0/C1 + DEL; bypass via Unicode lookalikes: - U+2028/U+2029: LINE/PARAGRAPH SEPARATOR (newline-equivalent in most Unicode-aware terminals — most direct log-forging vector) - U+200B–U+200F: zero-width chars + LRM/RLM - U+202A–U+202E: bidirectional override controls - U+FEFF: BOM / ZWNBSP A malicious IdP returning `slow_down
[serve] FAKE` in `oauthError` would otherwise still forge log lines. Architectural change: `sanitizeForStderr` was previously private to `qwenDeviceFlowProvider.ts`. To address #2/#3, the registry layer needs to call it too. Lifted into `deviceFlow.ts` (the foundation module) and re-imported from the provider. Single source of truth; the regex is now a module-level constant compiled once with explicit `\uXXXX` escapes (via `String.raw` so the source is greppable, not literal-Unicode-laden). Tests: - `does NOT attach late-poll observer when the provider beats the timeout` — N1 race regression - `sanitizes hostile latePollResult.kind in late-observer audit` — N3 - `sanitizes hostile lateErr.name in late-rejection observer audit` — N2 - `sanitizes Unicode lookalike controls (U+2028 LINE SEPARATOR, bidi, ZWNBSP) in oauthError` — N4 cli serve 706/706 (was 702, +4 — all new round-5 tests); sdk 421/421; typecheck clean; eslint --max-warnings 0 clean on touched files. Refs: #4175, #4255, #4291, #4305 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * fix(serve): address gpt-5.5 + qwen-latest review on #4305 round-5 (5 threads) Round-6 fold-in. Five findings split between maintainability, security hardening, and a real defensive bug. #1 (qwenDeviceFlowProvider.test.ts) — gpt-5.5: round-5 #4 test embedded U+2028 / U+200E / U+FEFF as literal characters in source. Invisible in GitHub diffs / most editors; the negative `not.toContain('')` looked like an empty-string check. Rewrote the payload + assertions to use named `\uXXXX`-bound constants. Also added a companion test exercising U+2066–U+2069 (round-6 #5 below). #2 (deviceFlow.ts) — qwen-latest: the late-poll observer's `void tracked.then(...)` was missing a terminal `.catch(() => {})`. A synchronous throw inside either handler (e.g., a misbehaving `audit.record`: backpressure, malformed payload, sink out-of-disk) would reject the derived promise unhandled. On Node 22's default `--unhandled-rejections=throw`, that crashes the daemon. Added the terminal `.catch(() => {})` matching the persist-tracker pattern. New test injects a poison audit sink that throws specifically on the `lost_late_poll_after_timeout` call; asserts `flushAsync()` resolves cleanly. #3 (deviceFlow.ts) — qwen-latest: the `case 'error'` audit-record hint interpolated `rawProviderError` (raw `err.message`) without `sanitizeForStderr`. Per ES2019+ `JSON.stringify` no longer escapes U+2028/U+2029 — those would still forge log lines downstream through file/stdout audit sinks. Apply the same sanitizer used on every other provider-controlled audit path. New test pins a hostile provider message containing U+2028 + ANSI escape and asserts neither survives. #4 (deviceFlow.ts) — qwen-latest: the round-5 #1 comment claimed "`DeviceFlowPollTimeoutError` isn't exported as a public DeviceFlow contract", but it IS `export class` (the test file constructs it directly for fixtures). With `pollTimedOut = true` keyed solely on `instanceof`, a future provider that imports + throws the class would spoof the registry's "I caused the timeout" signal — attaching a phantom late-poll observer. Fix: introduce a runtime brand `_isRegistryTimeout: boolean` on the class (default `false`) plus an internal-only `makeRegistryPollTimeoutError(ms)` helper that sets the brand to `true`. The brand is set ONLY at the registry's race-timer construction site. Both gates updated: - `if (err instanceof X && err._isRegistryTimeout === true)` in the catch (for `pollTimedOut`) - `if (lateErr instanceof X && lateErr._isRegistryTimeout === true)` in the late-rejection self-filter A provider-thrown brand-false instance now flows through the generic provider-throw audit path — correctly auditing the misuse rather than silently swallowing it. Repurposed the original "no double-audit when registry's own DeviceFlowPollTimeoutError is late-rejected" test (which was actually exercising the brand-false path) into the inverted assertion: brand-false provider throw IS audited as a real failure. Removed the orphaned old assertion; the brand-true happy path is implicitly covered by the hanging-provider test (which exercises the registry-built timeout end-to-end). #5 (deviceFlow.ts) — qwen-latest: `sanitizeForStderr` regex covered U+202A–U+202E (bidi embedding/override) but missed U+2066–U+2069 (LRI/RLI/FSI/PDI). These are the primary CVE-2021-42574 ("Trojan Source") attack vectors — a hostile IdP swapping U+2066 for U+202D achieves the same visual reordering and would have bypassed the round-5 filter entirely. Extended the regex range and JSDoc; new test exercises U+2066/U+2068/U+2069 in `oauthError` and asserts none survive while substantive ASCII parts remain. cli serve 713/713 (was 710, +3 round-6 tests + the round-5 #4 rewrite + the round-6 #5 companion); typecheck clean across all 4 workspaces; eslint --max-warnings 0 clean on touched files. Refs: #4175, #4255, #4291, #4305 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * fix(serve): replace literal U+2028 with explicit 
 escape in round-6 #3 test PR #4312 review (Copilot): the round-6 #3 test (sanitizes rawProviderError) regressed back to embedding a literal U+2028 character in source via `const U_2028 = ' '`. That's the same maintainability anti-pattern round-6 #1 was fixing in the sister test. Internal-consistency fix: switch to the explicit `
` escape so the constant is greppable and reviewable in GitHub diffs. Refs: #4291, #4305, #4312 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * fix(serve): post-merge P2 corrections from Codex review on #4282 (#4297) * fix(serve): post-merge P2 corrections from Codex review on #4282 Follow-up to PR #4282 (Wave 4 PR 17) addressing four P2 issues flagged by Codex's `/review` after the squash-merge to main: P2-1 — Read the workspace context filename for init `qwen serve` parent never goes through `loadCliConfig`, so the process-global `getCurrentGeminiMdFilename()` stays on the default `QWEN.md` even when the workspace configures `context.fileName: 'AGENTS.md'`. `runQwenServe` now snapshots the workspace's merged setting at boot and forwards via `BridgeOptions.contextFilename`, so init writes the same file the ACP child reads. P2-2 — Restart MCP servers with a fresh disabledTools snapshot `Config.disabledTools` was frozen at construction time; `setWorkspaceToolEnabled` only updated settings.json. The documented "toggle + restart" workflow re-registered just-disabled tools because rediscovery still saw the bootstrap snapshot. Added `Config.setDisabledTools()` plus a re-read at the ACP restart handler so `discoverMcpToolsForServer` honors the latest set. P2-3 — Match the SDK timeout to the daemon's restart budget Bridge waits up to 300s for stdio MCP discovery; SDK helper used the client-wide 30s default and aborted valid slow restarts. Added a per-call `timeoutMs` plumbed through `fetchWithTimeout`, defaulting `restartMcpServer` to 5 minutes. P2-4 — Reject symlinked parent directories before init writes `lstat(target)` only checked the final component; a symlinked parent (e.g. `docs -> /tmp` with `context.fileName: 'docs/QWEN.md'`) would let `writeFile` follow the link and create / truncate outside `boundWorkspace`. Added `canonicalizeExistingAncestor` (walks up through ENOENT to the deepest extant ancestor, then `realpath`s) and verifies the canonical parent stays within the canonical workspace. 5 new tests (4 bridge / 2 SDK): - contextFilename snapshot honored - parent-symlink escape rejected - nested real subdir accepted - restartMcpServer survives 1.2s response with 1s default timeout - restartMcpServer honors a 50ms caller override Typecheck clean across cli / sdk-typescript / core. 1604/1604 unit tests pass. * fix(serve): fold-in 1 — address 16:32:44-round review on #4282 Follow-up addressing the 8 unresolved review threads opened on PR shipping in this same #4297; addresses correctness gaps + missing test coverage that would otherwise let regressions ride into main. Behavior fix: - broadcastWorkspaceEvent gains a `skipSessionId` parameter; when `setSessionApprovalMode` runs with `persist:true`, the broadcast skips the requesting session so it doesn't receive the same `approval_mode_changed` event twice (once via session-scoped publish + once via broadcast). The SDK reducer's `approvalModeChangedCount` now increments by 1, not 2, on the requesting client (peers still see 1 via the broadcast). Addresses #3260501134. Observability + posture: - broadcastWorkspaceEvent now mirrors PR 16's publishWorkspaceEvent member: per-entry success/failure accounting + an "ALL buses dropped" stderr elevation. The previous local helper silently swallowed every publish failure. Addresses #3260501126. - WorkspaceInitPathEscapeError + WorkspaceInitSymlinkError typed classes for the two boundary guards in initWorkspace, mapped to HTTP 400 by sendBridgeError. Previous generic `Error` fell through to the 500 handler, telling operators "daemon broken" when the actual fix was workspace-config correction. Addresses #3260501161. Public surface symmetry: - Re-export McpServerNotFoundError, McpServerRestartFailedError, WorkspaceInitPathEscapeError, WorkspaceInitSymlinkError from the serve barrel. External embeds matching these via `instanceof` no longer need deep imports. Addresses #3260501163. Test coverage: - restartMcpServer bridge tests (5): success + event broadcast, soft-skip + refused event, McpServerNotFoundError translation, McpServerRestartFailedError translation, originator clientId stamping. Addresses #3260501141. - sendBridgeError mapping tests (4): McpServerNotFoundError → 404, McpServerRestartFailedError → 502, WorkspaceInitPathEscapeError → 400, WorkspaceInitSymlinkError → 400. Addresses #3260501148. - initWorkspace boundary guard tests (2 added): symlink-at-target rejected, contextFilename '../outside.md' rejected. Addresses #3260501157. - TrustGateError tests assert the typed class via `.toThrow(TrustGateError)`, not just message text. Addresses #3260501165. Also updates the existing fold-in 4 S2 broadcast test to reflect the new no-duplicate semantics on the requesting session. Typecheck clean across cli / sdk-typescript / core. 1615/1615 unit tests pass. * fix(serve): fold-in 2 — copilot + wenshao review on #4297 Round-2 reviewer adoption on the same PR: Critical fixes: - `restartMcpServer` JSDoc documents `timeoutMs: 0` as "disable the timeout entirely", but the `> 0` guard in `fetchWithTimeout` rejected `0` and silently fell back to the 30s client default. Loosened the guard to `>= 0` so `0` flows through to the no-timeout branch via the existing truthiness check; NaN / negative inputs still coerce to the client default. Addresses duplicate reports from copilot (#3260577538) and wenshao (#3260661833). - TS2322 in the slow-fetch test stub: `resolveResponse` was typed against `import('undici-types').Response` but assigned a `(v: Response) => void`. Re-typed against the global `Response` throughout. Caught only by tsc runs that include the test files. Addresses #3260663072. Test fidelity: - Slow-fetch stub now observes `init.signal` and rejects on abort, so a regression that drops the per-call `timeoutMs` override will reliably fail the test instead of resolving after the timer fired (false-negative coverage). Addresses #3260577600. - New test pinning the `timeoutMs: 0` semantics: 1ms client default + a stub that resolves after 50ms. Without the `>= 0` fix, the call would abort at 1ms; with it, the explicit `0` disables the timer and the call completes. Bug fixes: - `runQwenServe.contextFilenameForInit` previously called `String(arr[0])` on the array branch, producing a literal `"[object Object]"` filename for hand-edited bad data. Now validates each element with `typeof === 'string'` and falls back to `undefined` (so the bridge uses its `getCurrentGeminiMdFilename()` default) when no string is found. Addresses #3260577641. Documentation drift: - `Config.getDisabledTools()` JSDoc rewritten to describe the mutable-via-`setDisabledTools()` semantics introduced by P2-2, and the "registration-time only / no retroactive unregister" contract that pairs with it. Old comment claimed the set was frozen at construction. Addresses #3260577677. Observability: - `acpAgent` MCP-restart `loadSettings` failure now surfaces a stderr line naming the server + the underlying error, instead of silently swallowing it. The documented "toggle + restart" workflow used to break with zero diagnostic when settings.json was corrupted or unreadable. Addresses #3260663303. Code organization: - Moved `canonicalizeExistingAncestor` after `describeStatKind` so the latter's JSDoc is no longer orphaned (TypeScript only associates the last `/** ... */` block before a declaration). Addresses #3260668618. Typecheck clean across cli / sdk-typescript / core. 1616/1616 unit tests pass. * fix(serve): fold-in 3 — read merged scope on MCP restart refresh Critical bug from wenshao review (#3260725526) on PR #4297: the P2-2 acpAgent re-read narrowed `Config.disabledTools` to `SettingScope.Workspace` alone, dropping User / System scope entries. The bootstrap Config received `merged.tools?.disabled` (union of all scopes), so user-level / system-level disables worked at boot — but the first `mcp restart` would replace the in-memory set with the workspace scope alone, silently re-enabling any tool that was disabled at a higher scope but absent from the workspace file. The asymmetry vs. the persist-write path is deliberate and documented: - Reads (here): merged — match the bootstrap Config snapshot, preserve user/system policy. - Writes (`runQwenServe.persistDisabledTools`): workspace scope — don't bake higher-scope entries into the workspace file (per-#4282 fold-in 1 H2 fix). Two paths look alike but answer different questions. Typecheck clean across cli / sdk-typescript / core. 1616/1616 unit tests pass. * fix(test): fold-in 4 — wire timeoutMs:0 stub to init.signal Critical follow-up from wenshao (#3260810242) on PR #4297: the new `timeoutMs: 0` regression test (added in fold-in 2) inherited the same flaw it was meant to prevent — the slow-fetch stub didn't observe `init.signal`, so a regression that ignored the `0` override would fire the AbortController at the 1ms client default but the stub would keep the promise pending. The 50ms `resolveResponse` would win, the test would still pass, and the documented "0 disables timeout" contract would be unprotected. Mirrored the listener pattern already used by the two sibling tests in fold-in 2 — `init.signal.addEventListener('abort', () => reject(...))`. Now a regression that re-rejects `0` triggers the abort, the stub rejects, the test fails. 8/8 restartMcpServer SDK tests pass; SDK typecheck clean. * fix(serve): fold-in 5 — TOCTOU + setDisabledTools coverage Two new critical reviews from wenshao on PR #4297: C1 — TOCTOU between lstat and writeFile (#3260836305): The `lstat(target)` symlink check and the subsequent `writeFile` were two separate syscalls, leaving a race window where a local attacker with workspace write access could substitute a symlink between them. With `force: true`, `writeFile` would follow the link and truncate an external target. The `action === 'created'` path now uses `fs.open(target, 'wx')` (O_WRONLY|O_CREAT|O_EXCL), which atomically refuses any pre-existing inode (regular file, dir, OR symlink) at the target path. EEXIST after the absence check most plausibly means a race-created symlink, so we throw `WorkspaceInitSymlinkError(kind: 'target')` — same typed class the route maps to 400. The `force: true` overwrite path retains the existing TOCTOU as a documented limitation; closing it requires `O_NOFOLLOW`-aware open which the post-PR18 `WorkspaceFileSystem` migration will provide. C2 — P2-2 zero test coverage (#3260836302): The `setDisabledTools` runtime sync was the only Wave-4 P2 fix without a dedicated test. Added 5 Config-level tests: - Initializes from `disabledTools` ConfigParameters - Defaults to empty set when omitted - `setDisabledTools` replaces the live snapshot - Defensive copy: caller-set mutations don't leak into the live snapshot - Accepts an empty set (clears live snapshot) Plus a TOCTOU regression test in httpAcpBridge.test.ts that spies fs.lstat / fs.readFile to simulate the race window: pre-creates a symlink, makes lstat lie about it, asserts the 'wx' open catches the racing inode and throws the typed `WorkspaceInitSymlinkError(kind: 'target')`. 1622/1622 unit tests pass; typecheck clean across cli / sdk-typescript / core. * fix(serve): fold-in 6 — count actual skips in broadcast alarm DeepSeek review on #4297 (#3261079572): `broadcastWorkspaceEvent` unconditionally subtracted 1 from the `eligible` recipient count whenever `skipSessionId` was set, even when the id matched zero live sessions (caller mistake, stale id, or the matching session was just torn down between resolution and broadcast). In a single-session workspace that's the difference between `eligible = 0` (alarm suppressed) and `eligible = 1` (alarm fires when the publish failed) — silently losing the all-dropped breadcrumb the telemetry was meant to surface. Today's call sites pass real session ids so the bug doesn't manifest in practice, but the defensive shape is small: track `skippedCount` inside the loop and subtract that, so the alarm condition is self-consistent regardless of how the caller mis-uses the param. 162/162 bridge tests pass; CLI typecheck clean. * fix(serve): fold-in 7 — close overwrite TOCTOU, harden boot + diagnostics Round-7 review on PR #4297. Three critical fixes + one suggestion test, plus a regression test for the overwrite TOCTOU close. C1 — force:true overwrite TOCTOU (#3262615446): The fold-in 5 fix only closed the `'created'` action via 'wx'; the `'overwrote'` branch still used plain `fs.writeFile`, so a local writer could swap the verified regular file to a symlink between the lstat/readFile checks and the write and have the forced overwrite truncate an external target. Switched to `fs.open(target, O_WRONLY | O_TRUNC | O_NOFOLLOW)` — `O_NOFOLLOW` makes open() fail with ELOOP on a symlink at the final component even under race. ELOOP / ENOENT (race-deleted) translate to `WorkspaceInitSymlinkError(kind: 'target')` so the route still maps to a structured 400 instead of a generic 500. C2 — settings.json corrupt blocks daemon boot (#3262625091): `loadSettings(boundWorkspace)` at boot had no try/catch — a corrupted, malformed, or temporarily unreadable settings file threw synchronously and prevented daemon startup. Pre-PR this never happened because settings were read lazily inside request handlers. Wrapped in try/catch with stderr fallback so the daemon keeps booting (with the bridge's default context filename) when the file is broken. C3 — malformed `tools.disabled` clears policy silently (#3262625101): When `merged.tools?.disabled` is present but not an array (boolean / string / object from a hand-edited settings.json), the ternary `Array.isArray(...) ? ... : []` substituted an empty list without firing the surrounding catch block. After an MCP restart every disabled tool would silently re-register. Added an explicit `!Array.isArray && !== undefined` check that stderr-logs the malformed type before clearing — operators see the misconfiguration instead of a stealth re-enable. S1 — contextFilename extraction tested (#3262690842): Lifted the inline `firstStringInArray` + branching into an exported `extractContextFilename(value: unknown)` helper and added `runQwenServe.test.ts` with 5 tests covering the four branches the suggestion called out: non-empty string, array with strings, array with no strings, non-string non-array. Plus a TOCTOU regression test for the overwrite path that verifies `O_NOFOLLOW` returns `WorkspaceInitSymlinkError(kind: 'target')` when the file is race-substituted with a symlink behind the lstat/readFile mocks. S2 (acpAgent restart-handler integration test #3262690845) is deferred — Config-level coverage of `setDisabledTools` already locks the load-bearing surface (5 tests in fold-in 5), and adding a full acpAgent integration test requires heavy ext-method plumbing. The new C3 stderr diagnostic plus existing tests give us the regression signal we need without that scaffolding. 1627/1627 unit tests pass; typecheck clean across cli / sdk-typescript / core / acp-bridge. * fix(serve): fold-in 8 — split ELOOP / ENOENT diagnostic in overwrite path qwen-latest review on PR #4297 (#3262861754): The fold-in 7 ELOOP/ENOENT branch shared one error message that said "swapped to a symlink." That's accurate for ELOOP (genuine O_NOFOLLOW rejection — likely an attack race) but misleading for ENOENT in the overwrite path: there `readFile` just succeeded proving the file existed, so ENOENT means the file was DELETED between the content check and the open — a benign race with a concurrent writer (git checkout, editor save, lockfile rename), NOT a symlink swap. An operator seeing the symlink language for a benign delete would `ls -la`, see no symlink, and waste time hunting an attack that didn't happen. Split into two messages: - ELOOP: "swapped to a symlink between the content check and the overwrite — refusing to follow it" - ENOENT: "deleted between the content check and the overwrite (likely a concurrent writer) — refusing to recreate blindly" Both still surface as `WorkspaceInitSymlinkError(kind: 'target')` so the route maps to a structured 400; the class doubles as the workspace-init race-condition bucket with kind='target' meaning "target inode misbehaved at write time" generally. Updated the existing fold-in 7 TOCTOU test to assert the ELOOP message specifically, and added a new ENOENT race-delete test that mocks lstat/readFile to land on the overwrote action against a non-existent path — verifies the message says "deleted" and NOT "swapped to a symlink." 170/170 bridge tests pass; CLI typecheck clean. * fix(serve): fold-in 9 — route MCP restart through registry cleanup wrapper gpt-5.5 critical review on PR #4297 (#3263088414): The fold-in 5 P2-2 fix refreshed `Config.disabledTools` from merged settings, but then called `manager.discoverMcpToolsForServer()` directly — bypassing the `ToolRegistry.discoverToolsForServer` wrapper that PURGES the server's existing `DiscoveredMCPTool` entries (and `revealedDeferred` markers) plus its prompts before rediscovery. Without the cleanup, `registerTool` only consulted the refreshed `disabledTools` set for NEWLY-discovered tools — entries already in the registry from the prior MCP boot kept serving requests. Net effect: toggle-disable-then-restart silently left the disabled tool live, breaking the documented "toggle + restart" workflow that P2-2 was meant to fix. Routed through `toolRegistry.discoverToolsForServer(serverName)` which: 1. Removes existing `DiscoveredMCPTool` entries for this server 2. Drops their `revealedDeferred` reveal state 3. Removes the server's prompts via `removePromptsByServer` 4. THEN delegates to `manager.discoverMcpToolsForServer` for the actual reconnect + rediscover The pre-discovery budget / in-flight checks still go through the `manager` reference (which is the same object the registry wrapper would forward to) — so soft-skip semantics for `budget_would_exceed`, `in_flight`, `disabled` are preserved. CLI typecheck clean; 403/403 server + bridge tests pass. * fix(serve): fold-in 10 — qwen-latest 05:45-round review on #4297 5 review threads from qwen-latest's late round on PR #4297 (now closed in favor of #4313 against `daemon_mode_b_main`). 1 critical + 4 suggestions, all adopted. C1 — extractContextFilename / getCurrentGeminiMdFilename divergence (#3263954685): with `context.fileName: [' ', 'AGENTS.md']`, the daemon parent's `extractContextFilename` (which skips empty entries) wrote `AGENTS.md`, but the ACP child's `getCurrentGeminiMdFilename` (which returned `arr[0]` unconditionally) read `''`. The init'd file was orphaned. Aligned `getCurrentGeminiMdFilename` to skip empty entries with the same semantics, falling back to `DEFAULT_CONTEXT_FILENAME` when all entries are empty. S2 — WorkspaceInitSymlinkError reused for non-symlink races (#3263954690): the EEXIST race-create and ENOENT race-delete cases were surfacing as `code: 'workspace_init_symlink'`, misleading operators into hunting symlink attacks for benign concurrent- modification windows. Split into a sibling `WorkspaceInitRaceError` class (`kind: 'eexist' | 'enoent'`, HTTP code `workspace_init_race`). The genuine symlink class stays for ELOOP, lstat-detected target symlinks, and parent-realpath escapes. S3 — fsConstants.O_NOFOLLOW defensive `?? 0` (#3263954697): matches the existing codebase convention in `core/src/utils/{sessionStorageUtils,gitDiff}.ts` and `cli/src/ui/utils/customBanner.ts`. Functionally a no-op (JS bitwise coerces undefined to 0) but consistent. S5 — Parent-directory TOCTOU still open (#3263954707): O_NOFOLLOW only protects the final path component; a local writer could swap a real parent dir for a symlink between `canonicalizeExistingAncestor` and `fs.open`. Added `verifyParentWithinWorkspace` post-open helper that re-realpaths `path.dirname(target)` and refuses with `WorkspaceInitSymlinkError(kind: 'parent')` if the parent moved. On the create path (where we just opened with `'wx'`), the failure also unlinks the file we just made best-effort. Residual race window narrowed from "between pre-check and open" to "between post-open realpath and writeFile" — sub-millisecond, documented as accepted Stage-1 trust posture. S4 — broadcastWorkspaceEvent vs publishWorkspaceEvent stale comment (#3263954688): the "now removed" comment was inaccurate (5 call sites still use the closure). Replaced with an accurate description of why both coexist (factory closure can't `this`-call proxy member; closure also takes `skipSessionId` for persisted approval-mode mirror) and a TODO marker for future helper extraction. Two existing tests updated to assert the new `WorkspaceInitRaceError` class for EEXIST / ENOENT scenarios (the symlink-class assertions are preserved for ELOOP / lstat / parent cases). 1759/1759 unit tests pass; typecheck clean across all 4 packages. * feat(acp-bridge): F1 — acp-bridge package self-sufficiency (#4175 mechanical lift + BridgeFileSystem seam) (#4319) * refactor(acp-bridge): lift defaultSpawnChannelFactory to acp-bridge/spawnChannel (#4175 F1 step 1) First mechanical lift of #4175 F1 (acp-bridge package self-sufficiency). Moves the production spawn factory + its `killChild` helper + `SCRUBBED_CHILD_ENV_KEYS` denylist + `KILL_HARD_DEADLINE_MS` constant from `cli/src/serve/httpAcpBridge.ts` (~283 lines) to `@qwen-code/acp-bridge/spawnChannel`. This unblocks `channels/base/AcpBridge.ts` and `vscode-ide-companion`'s acpConnection from each reimplementing the child lifecycle — they can now consume the same primitive. Backward compatible: `cli/src/serve/httpAcpBridge.ts` imports the lifted factory and re-exports it, so existing references in `cli/src/serve/index.ts:90` and the factory's own internal usage (`opts.channelFactory ?? defaultSpawnChannelFactory`) keep resolving. Bridge tests that mock `defaultSpawnChannelFactory` via `BridgeOptions.channelFactory` are unaffected. Side cleanups: drops `spawn` / `ChildProcess` / `Readable` / `Writable` / `ndJsonStream` / `MissingCliEntryError` imports from httpAcpBridge.ts (all only used by the lifted spawn factory). - 44/44 acp-bridge tests pass - 174/174 cli httpAcpBridge tests pass - typecheck clean across acp-bridge + cli 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * refactor(acp-bridge): lift BridgeClient + permission types to acp-bridge/bridgeClient (#4175 F1 step 2) Second mechanical lift of #4175 F1 (acp-bridge package self-sufficiency). Moves `BridgeClient` class (~700 LOC) + `PendingPermission` interface + `PermissionResolutionRecord` interface + `MAX_RESOLVED_PERMISSION_RECORDS` constant + early-event capacity constants + `describeStatKind` and `sliceLineRange` helpers from `cli/src/serve/httpAcpBridge.ts` to `@qwen-code/acp-bridge/bridgeClient`. Design choice for SessionEntry boundary: introduce a minimal `BridgeClientSessionEntry` interface in bridgeClient.ts with only the four fields BridgeClient actually reads from the factory's richer `SessionEntry` (`sessionId`, `events`, `pendingPermissionIds`, `activePromptOriginatorClientId`). The factory's `SessionEntry` structurally satisfies it — TypeScript's structural typing enforces the match at the `resolveEntry` callback signature, so no explicit conversion is required and the bridge package stays free of daemon-host session-bookkeeping types. Cross-package writeStderrLine handling: inline the 3-line helper in bridgeClient.ts (mirrors the spawnChannel.ts pattern from F1 step 1) so acp-bridge has no reverse dependency on `cli/src/utils/stdioHelpers`. httpAcpBridge.ts shrinks from 4406 LOC to 3647 LOC (-759 lines). Removed ACP SDK imports that only BridgeClient consumed: `Client`, `RequestPermissionRequest`, `WriteTextFileRequest`, `WriteTextFileResponse`, `ReadTextFileRequest`, `ReadTextFileResponse`, `SessionNotification`. Kept the ones the factory still uses (`CancelNotification`, `PromptRequest`, `RequestPermissionResponse`, `SetSessionModelRequest`, `SetSessionModelResponse`). Backward compatible: httpAcpBridge.ts re-exports `BridgeClient`, `BridgeClientSessionEntry`, `PendingPermission`, `PermissionResolutionRecord`, and `MAX_RESOLVED_PERMISSION_RECORDS` so the `ChannelInfo.client: BridgeClient` field declaration below + any embedder reaching into these types keep resolving. - 44/44 acp-bridge tests pass - 174/174 cli httpAcpBridge tests pass - 229/229 cli server tests pass - typecheck clean across acp-bridge + cli 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * refactor(acp-bridge): lift createHttpAcpBridge factory to acp-bridge/bridge (#4175 F1 step 3) Third + final mechanical lift of #4175 F1 (acp-bridge package self-sufficiency). Moves the `createHttpAcpBridge` factory closure (~3000 LOC) + `ChannelInfo` + `SessionEntry` interfaces + factory-only helpers (`canonicalizeExistingAncestor`, `verifyParentWithinWorkspace`, `withTimeout`, `isServeDebugLoggingEnabled`, `writeServeDebugLine`, `hasControlCharacter`) + factory constants (`DEFAULT_INIT_TIMEOUT_MS`, `MCP_RESTART_TIMEOUT_MS`, `DEFAULT_MAX_SESSIONS`, `MAX_EVENT_RING_SIZE`, `DEFAULT_PERMISSION_TIMEOUT_MS`, `DEFAULT_MAX_PENDING_PER_SESSION`, `MAX_DISPLAY_NAME_LENGTH`) from `cli/src/serve/httpAcpBridge.ts` to `@qwen-code/acp-bridge/bridge`. `cli/src/serve/httpAcpBridge.ts` shrinks from 3647 LOC to 97 LOC — a pure re-export shim that preserves every existing relative import path (`./httpAcpBridge.js`) so `server.ts`, `runQwenServe.ts`, `workspaceAgents.ts`, `workspaceMemory.ts`, `index.ts`, plus the bridge test suite, keep resolving without any call-site changes. The new `bridge.ts` reuses what was already in acp-bridge (errors, types, options, status helpers, channel types, event bus, workspace paths) via local relative imports — no reverse dependency on `cli`. `writeStderrLine` is inlined at the top of `bridge.ts` (same pattern as `spawnChannel.ts` + `bridgeClient.ts` from F1 steps 1-2) so the package self-contained promise holds. Cumulative F1 impact across the 3 mechanical lift steps: - httpAcpBridge.ts: 4682 LOC → 97 LOC (-4585 lines; the original file was 98% bridge core, 2% backward-compat re-exports) - 3 new files in acp-bridge: spawnChannel.ts (~270 LOC), bridgeClient.ts (~745 LOC), bridge.ts (~3515 LOC) - All daemon-host concerns (env snapshot, daemon preflight cells) remain in `cli/src/serve/daemonStatusProvider.ts` and reach the bridge through the `BridgeOptions.statusProvider` seam frozen by PR 22b/2. - 735/735 cli serve tests pass across 17 files - 174/174 cli httpAcpBridge tests pass - 44/44 acp-bridge tests pass - typecheck clean across acp-bridge + cli `packages/cli/src/serve/httpAcpBridge.test.ts` (~6600 LOC) is intentionally NOT moved in this commit — it currently imports `createHttpAcpBridge` / `defaultSpawnChannelFactory` / `BridgeClient` via the cli shim and keeps passing without changes. Moving it to `acp-bridge/src/bridge.test.ts` is a follow-up worth tracking separately so the production-code lift can land + be reviewed cleanly. The `BridgeFileSystem` injection seam (originally bundled into F1 as the 22b' scope) is also deferred to a follow-up so the mechanical lift stays mechanical — design + implementation of the fs injection is its own discussion. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * feat(acp-bridge): add BridgeFileSystem injection seam (#4175 F1 step 5, 22b' scope) Adds the `BridgeFileSystem` injection seam originally scoped as #4175 22b'. When a `BridgeFileSystem` is wired through `BridgeOptions.fileSystem`, `BridgeClient.readTextFile` and `BridgeClient.writeTextFile` delegate to it instead of running their inline `fs.realpath` / `fs.writeFile` / `fs.readFile` proxy. This unblocks production `qwen serve` plumbing PR 18's `WorkspaceFileSystem` (TOCTOU guards, symlink-substitution checks, trust gate, `.gitignore`, audit hooks) into the ACP fs methods — closing the `ws.ts:613` follow-up thread that has been tracked since PR 18 landed. The serve-side adapter that wraps `WorkspaceFileSystem` + the `runQwenServe` wiring are intentionally split into the immediate-follow-up so this PR stays focused on the seam design. Backward compatible: `fileSystem` is optional on `BridgeOptions`. Tests, Mode A in-process consumers, channels (`packages/channels/base/ AcpBridge.ts`), and the VSCode IDE companion all keep working unchanged — they omit the field and `BridgeClient` falls through to the inline proxy that has been the Stage 1 default since #3889. API: - `BridgeFileSystem.readText(params: ReadTextFileRequest): Promise` - `BridgeFileSystem.writeText(params: WriteTextFileRequest): Promise` The interface mirrors ACP SDK request/response types directly so the adapter does the minimum amount of translation (`{ path, content }` ↔ `WorkspaceFileSystem`'s `ResolvedPath` brand types + options bag). - 735/735 cli serve tests pass (inline fallback path preserved) - 44/44 acp-bridge tests pass - typecheck + eslint clean 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * docs(acp-bridge): catch README + stale source comments up to F1 lift Self-review fold-in: post-F1 the package README still said "PR 22a" and listed `BridgeClient` / `createHttpAcpBridge` / `defaultSpawnChannelFactory` under "What's not here yet" — both contradicted by this PR. Updated: - README lift-history table now shows PR 22a / 22b/1 / 22b/2 as merged and F1 (this PR) as the slice that closes the bridge core + adds `BridgeFileSystem`. F3 PR 24 row aligned to the feature-cohesive plan. - "What's here today" now documents `spawnChannel`, `bridgeClient`, `bridge`, `bridgeFileSystem` modules. - "What's not here yet" section removed (its 2 bullets are both resolved by F1). - Subpath import list updated to enumerate all 14 subpaths. - Backward-compat section updated to call out the 97-line shim and the 6 consuming files that still import via `./httpAcpBridge.js`. Source-comment line-number drift: - `channel.ts:12` no longer claims `defaultSpawnChannelFactory` is "still in cli/src/serve/httpAcpBridge.ts" — points to the lifted location. - `permission.ts:33` + `permission.ts:45` no longer reference `httpAcpBridge.ts:1096-1106` / `httpAcpBridge.ts:1003` (file is now 97 lines after F1). Updated to point at the structurally- equivalent locations inside the lifted `bridgeClient.ts`. - `permission.ts:7` no longer says first-responder still lives in `cli/src/serve/httpAcpBridge.ts` — points at the bridgeClient.ts location. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * docs(acp-bridge): adopt 3 Copilot review comments on F1 doc accuracy Folds in 3 of 4 Copilot inline comments from #4319 review: 1. `bridgeClient.ts` writeTextFile preserveMode comment said "fall through to umask defaults" for new files, but the code passes `mode: preserveMode?.mode ?? 0o600` to `fs.writeFile`. Updated the "BkwQW" comment + the inner catch-block comment to clarify that new files actually get the `0o600` default applied at writeFile time (NOT umask defaults — the explicit `mode` arg bypasses umask for atomicity per the `Blehd` comment block). 2. `bridgeFileSystem.ts` JSDoc referenced `cli/src/serve/bridgeFileSystemAdapter.ts` as if the file exists, but it's deferred to the immediate F1 follow-up PR. Reworded as "the immediate follow-up PR will land a serve-side adapter" so reviewers don't grep for a non-existent file. 3. `bridgeOptions.ts` `fileSystem` field JSDoc had the same wording issue ("Production `qwen serve` wires this to..."). Same fix — now says "The immediate F1 follow-up will land a serve-side adapter" so the deferred state is obvious. Declined from this review round: - Copilot inline #1 (`spawnChannel.ts:155` stderr forwarder drops empty lines): pre-existing behavior since #3889. F1 lifted verbatim — not a regression introduced here. Out of scope for a lift PR. - github-actions bot summary: most items are pre-existing notes (TOCTOU residual race, SCRUBBED_CHILD_ENV_KEYS allowlist concern, sliceLineRange benchmark threshold) on code the F1 lift moved verbatim. One ("httpAcpBridge.ts still has ~3700 LOC") is a false positive — the file is 97 LOC after F1. Others are cosmetic refactors (extract FIXME to tracking issue, ARCHITECTURE_DECISIONS doc system, deprecation timeline) that aren't worth churning the lift PR over. - 44/44 acp-bridge tests pass - typecheck clean 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * docs(acp-bridge): tighten BridgeFileSystem contract + re-export type from shim Self-review + code-reviewer agent fold-in, two changes: 1. `cli/src/serve/httpAcpBridge.ts` shim now re-exports `BridgeFileSystem` from `@qwen-code/acp-bridge/bridgeFileSystem` so the immediate F1 follow-up adapter (in `cli/src/serve/`) can import it via the established `./httpAcpBridge.js` path like every other daemon-side bridge import does. Without this the adapter would need to deep-import from acp-bridge while every other serve file goes through the shim — inconsistent. 2. `BridgeFileSystem.readText` + `writeText` JSDoc now spells out the two defensive gates the inline proxy carried (non-regular- file rejection + 100 MiB buffered-size cap for reads; write-then-rename atomicity + dangling-symlink walk-through + mode preservation + `0o600` new-file default for writes). When a `BridgeFileSystem` is injected, the inline path is FULLY bypassed — without the contract spelled out, a future adapter author could silently drop the `/dev/zero` / 500 MB log RSS defenses the inline path established. Note on F1 CI: this PR targets `daemon_mode_b_main` but the `.github/workflows/ci.yml` `pull_request` trigger is scoped to `branches: main / release/**`, so the main CI workflow (Lint / Test on Linux/macOS/Windows / CodeQL) does NOT run on this PR. This is a by-design side effect of the new feature-cohesive branching strategy — `daemon_mode_b_main → main` periodic merges will trigger the full CI matrix, providing safety net coverage before any F-series work lands on `main`. Locally verified: - 174/174 cli httpAcpBridge tests pass - 44/44 acp-bridge tests pass - 735/735 cli serve tests pass - typecheck clean across acp-bridge + cli 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * test(acp-bridge): cover BridgeFileSystem injection seam + extract shared writeStderrLine (#4319 wenshao review) Folds in wenshao review on #4319: 1. **[Critical]** zero test coverage for the F1 step 5 `BridgeFileSystem` delegation branches in `BridgeClient.writeTextFile` / `BridgeClient.readTextFile` and the factory's `opts.fileSystem` → constructor positional-arg forwarding. New `packages/acp-bridge/src/bridgeClient.test.ts` adds 6 tests covering: - writeTextFile delegates to injected fileSystem.writeText (inline proxy fully bypassed; `fakeFs.writeText` called with the original params; `readText` mock not invoked) - writeTextFile invalid-path call succeeds purely via the mock when fileSystem is injected (proof that the inline `fs.realpath` path doesn't run) - readTextFile delegates to injected fileSystem.readText - readTextFile propagates injection errors to the caller - inline-fallback regression guard: write actually hits disk via the inline proxy when fileSystem is omitted (real tmp file round-trip) - same for read Why these matter: the 7-arg `BridgeClient` constructor places `fileSystem` at the tail as optional. A reordering — or dropping the arg from `bridge.ts` factory's `new BridgeClient(..., opts.fileSystem)` call — would silently bypass the adapter in production and the inline `fs.writeFile` raw-path would run with no audit / trust / TOCTOU coverage. The delegation tests would catch that because the mock fileSystem would never be invoked. 2. **[Suggestion]** `writeStderrLine` was defined identically in `bridge.ts:117` and `bridgeClient.ts:30` (22 call sites across the two files). Both consumers live in the SAME `@qwen-code/acp-bridge` package, so the original "no reverse-dep on cli" justification doesn't apply within the package. Extracted to `packages/acp-bridge/src/internal/stderrLine.ts` — a single source of truth that future behavior changes (timestamp prefix, log level, structured field) can edit once. `internal/` subpath is intentionally not in `package.json`'s `exports`, keeping the helper package-private. `spawnChannel.ts` deliberately does NOT consume it (its stderr writes use `process.stderr.write(prefix + line + '\n')` directly because each line carries its own `[serve pid=… cwd=…]` line prefix). - 6/6 new BridgeFileSystem-seam tests pass - 50/50 acp-bridge total (44 existing + 6 new) - 174/174 cli httpAcpBridge tests pass (no regression from refactor) - typecheck + eslint clean 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * test(acp-bridge): cover defaultSpawnChannelFactory env scrubbing + fix bridge.ts comment refs (#4319 wenshao round 2) Folds in wenshao review on #4319 round 2 — 1 Critical + 2 Suggestions: 1. **[Critical] spawnChannel.ts has 0 unit tests, security-critical paths untested.** Now that `defaultSpawnChannelFactory` is a public export of `@qwen-code/acp-bridge`, channels + IDE consumers can't rely on cli-package integration tests for env-scrubbing guarantees. Refactored the inline env-scrubbing logic into a pure exported helper `scrubChildEnv(source, scrubbed, overrides)`. Behavior is byte-identical to the pre-extraction inline implementation; the factory body now reads: const childEnv = scrubChildEnv( process.env, SCRUBBED_CHILD_ENV_KEYS, childEnvOverrides); Added `packages/acp-bridge/src/spawnChannel.test.ts` with 12 tests covering: - shallow-clone (no aliasing into live process.env) - QWEN_SERVER_TOKEN stripping - non-scrubbed vars pass through - override-add a new key - override-replace an existing key - override with undefined deletes the key (PR 14 fix #4247 wenshao R5) - override CANNOT re-introduce a scrubbed key (defense in depth) - override CANNOT undo the scrub by setting undefined for a scrubbed key - override-apply-after-scrub ordering invariant - empty overrides equals no overrides - multi-key scrub for forward-compat (the WARNING comment on SCRUBBED_CHILD_ENV_KEYS anticipates a future sandboxed-agent mode expanding the denylist; this verifies the loop already handles that) The killChild SIGTERM→SIGKILL escalation + STDERR_LINE_CAP_CHARS truncation are NOT covered yet — they require either real child processes or extensive node:child_process mocking; both are orthogonal to the env-scrubbing security guarantees wenshao explicitly called out, and can land as a follow-up if anyone wants the full surface tested. 2. **[Suggestion] bridge.ts comments referenced a "consolidated re- export block earlier in this file" that doesn't exist in acp-bridge (only in the cli shim).** Fixed both occurrences (~line 292, ~line 310) to point at the actual local import + the package barrel re-export. 3. **[Suggestion] bridge.ts canonicalizeWorkspace re-export comment referenced `./fs/paths.ts`.** Updated to mention the full lift chain: extracted to `cli/src/serve/fs/paths.ts` in PR 18, then lifted here to `./workspacePaths.ts` in PR 22b/1. - 12/12 new spawn env-scrub tests pass - 62/62 acp-bridge total (50 existing + 12 new spawn) - 174/174 cli httpAcpBridge tests still pass (the factory's inline env-scrubbing refactor preserves byte-identical behavior) - typecheck + eslint clean 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * docs(acp-bridge): fix 14-arg→7-arg typo in test docstring + simplify canonicalizeWorkspace re-export doc (#4319 wenshao round 3) Folds in 2 of 3 wenshao Suggestions from #4319 round 3: 1. `bridgeClient.test.ts:20` JSDoc said "the 14-arg constructor's positional slot" — typo I introduced when writing the test in `fbc92bccf`. The same docstring correctly says "the constructor takes 7 positional args" at line 25. Updated to "7-arg". 2. `bridge.ts:3461` `canonicalizeWorkspace` re-export JSDoc no longer references the historical `cli/src/serve/fs/paths.ts` location. Reads cleaner as a present-tense pointer to `./workspacePaths.ts` (where the implementation actually lives now post-PR 22b/1). Git history covers the lift chain; the docstring should describe current state. DECLINED + tracked separately: - **[Critical]** `closeSession` + `killSession` use module-scoped `channelInfo` instead of `channelInfoForEntry(entry)` — channel- overlap edge case can kill the wrong channel. Wenshao explicitly notes "pre-existing bug preserved by the lift" — F1's mechanical- lift scope shouldn't carry behavior fixes, and the fix needs a channel-overlap regression test to land safely. Tracked as #4325. - 62/62 acp-bridge tests pass (no regression from doc tweaks) - typecheck + eslint clean 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * docs(acp-bridge): polish from second-pass self-review (cross-platform test + package metadata + dead tombstones) Five small adoptions from a second-pass code-reviewer agent review on F1 (no new external comments — pre-emptive cleanup before reviewer returns): 1. **`bridge.ts:290-313`** — deleted two standalone "InvalidPermission OptionError / WorkspaceInit* / McpServer* lifted to bridgeErrors" tombstone comments. Pre-22b they were load-bearing (explained why the class wasn't `class`-defined inline at that file location). Post-F1 the symbols are imported at the top of the file and the comments sit between unrelated code (`writeServeDebugLine` / `MAX_DISPLAY_NAME_LENGTH` / `DEFAULT_INIT_TIMEOUT_MS`) with no anchor. Dead doc — removed. 2. **`README.md`** — `spawnChannel` entry now lists `scrubChildEnv` alongside `defaultSpawnChannelFactory` + `killChild` + `SCRUBBED_CHILD_ENV_KEYS`. Channels / VSCode IDE consume the package barrel so the helper should be visible in the inventory. 3. **`package.json:description`** — refreshed from the PR 22a wording ("EventBus, AcpChannel, in-memory channel, PermissionMediator interface") to include F1 additions (`createHttpAcpBridge` / `BridgeClient` / `defaultSpawnChannelFactory` / `BridgeFileSystem`). Visible on `npm view`-style tooling + IDE hover so worth keeping current. 4. **`bridgeClient.test.ts:92-115`** — swapped `/proc/no-such-file` for `/this/dir/never/exists/file.txt` and reworded the comment. `/proc/` is Linux-only; on macOS / Windows the inline proxy's dangling-symlink fallback would write through to a path under root rather than failing. Test passed regardless (mock assertion, not real disk) but the comment overstated portability. 5. **`spawnChannel.test.ts:36`** — added a comment block explaining why the test deliberately hand-rolls the SCRUBBED set instead of importing the production `SCRUBBED_CHILD_ENV_KEYS`. The decoupling is intentional (pure-function parameterized test + forward-guard for future denylist expansion) but a naive reader would think it's an oversight. - 62/62 acp-bridge tests pass - 174/174 cli httpAcpBridge.test.ts pass - typecheck + eslint + pre-commit hooks clean 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * fix(acp-bridge): bridge.ts security fold-in from #4297 review (3 issues) Folds 3 unresolved review comments from the post-merge thread on #4297 (wenshao via qwen-latest agent) into F1 (#4319). All 3 touch `acp-bridge/src/bridge.ts` — the same file F1 already moves the lifted factory into — so consolidating here saves opening a separate follow-up PR and keeps the security narrative in one reviewable commit. The 2 cross-package fixes (`core/src/memory/const.ts` test gap + `cli/src/serve/runQwenServe.ts` malformed-context fallback) will land as their own small PRs after F1 merges. #### Fix 1 (wenshao Critical, #4297 thread): `fs.unlink(target)` arbitrary-file-deletion primitive in `verifyParentWithinWorkspace` 'create'-cleanup After `fs.open(target, 'wx')` creates the empty file at the real parent, an attacker with local workspace write access can swap the parent directory for a symlink (`docs/` → `/etc`). The cleanup's `fs.unlink(target)` re-resolves the TEXTUAL path through the attacker's freshly-planted parent symlink, deleting whatever file exists at the external location. Fix: drop the `fs.unlink(target)` line. The 0-byte file at the pre-race location is harmless (0 bytes, inside the workspace we'd already verified) — leaving it over deleting an arbitrary external file is the right safety trade. Comment block explains the reasoning so future maintainers don't re-introduce the unlink. #### Fix 2 (wenshao Critical): `O_TRUNC` arbitrary-file-truncation primitive in workspace-init 'overwrite' branch `O_TRUNC` causes the kernel to truncate the file to zero bytes AT `open(2)` SYSCALL TIME — strictly before `verifyParentWithinWorkspace` runs. A parent-symlink TOCTOU race between `canonicalizeExistingAncestor` and this `open()` zeros the file at the attacker-redirected location (arbitrary-file-truncation primitive against any file the daemon UID can open). The pre-fix code's own comment on `verifyParentWithinWorkspace` acknowledged this as "Acceptable residual posture for the Stage-1 trust model"; wenshao pushed back that arbitrary-file-zeroing exceeds the Stage-1 trust budget. Fix: drop `O_TRUNC` from the open flags. Truncation moves to AFTER `verifyParentWithinWorkspace` succeeds, via `fh.truncate(0)` on the fd we already hold. fd-based truncate does NOT re-resolve the path — an attacker swapping the parent symlink after we open can't redirect the truncation. #### Fix 3 (wenshao Suggestion): `canonicalizeExistingAncestor` missing `ELOOP` catch Circular symlinks in the parent path (`a -> b`, `b -> a`) cause `fs.realpath` to fail with `ELOOP`. Without catching it, the error propagates as an unstructured HTTP 500 instead of the typed `WorkspaceInitSymlinkError` (HTTP 400) the route handler expects from the workspace-init race-detection family. Fix: add `'ELOOP'` to the caught error codes alongside `'ENOENT'` and `'ENOTDIR'`. Walking up the parent chain when ELOOP hits at a sub-component preserves the existing "walk to the deepest extant ancestor" contract — the deepest realpath-able ancestor still dictates the canonical prefix. #### Why no new tests in this commit - Fix 1 is a single-line removal: any regression that re-adds the unlink would be caught by reviewing the diff; existing 174-test `httpAcpBridge.test.ts` integration suite confirms the create-path still works (file is created + closed correctly; only the attacker-cleanup branch changes). - Fix 2 is a structural move (truncate from open-time to post-verify); the existing overwrite-init integration tests confirm the end-to-end behavior is unchanged (file ends up empty after init). Adding a TOCTOU race regression test requires controlled filesystem-race simulation that exceeds reasonable test infra scope for this PR. - Fix 3 is a one-word addition to an error code list; the `canonicalizeExistingAncestor` helper is module-private and the integration test for circular-symlink → typed 400 would require exporting it OR setting up a real circular-symlink workspace. Both routes widen scope beyond the security fix itself; the high-level behavior is verifiable by the existing route-error- mapping test pattern + diff review. A follow-up PR can add the integration tests once the security fix itself has shipped; the immediate priority is closing the arbitrary-file-deletion + arbitrary-file-truncation primitives. - 62/62 acp-bridge tests pass - 174/174 cli httpAcpBridge.test.ts pass - typecheck + eslint clean #### Refs - Original review on #4297 (wenshao via qwen-latest agent), post- merge, currently unresolvable on #4297 itself because that PR is already MERGED. - Other 2 #4297 review threads (`const.ts` test coverage, `runQwenServe.ts` malformed-context observability) target files outside F1's scope and will land as separate follow-up PRs. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * fix: post-merge Codex P2 fold-in — MCP restart disabled-tools normalization + SDK timeout headroom (#4319) Folds in 2 P2 findings from a Codex review run on `git diff main...HEAD` of F1 PR #4319. Both are pre-existing in code merged into `daemon_mode_b_main` before F1 was created (#4282 PR 17), but they're tiny tactical fixes (~25 LOC + 1 LOC) on the same integration branch the same reviewer (wenshao) already engages with, so folding into F1 saves an extra follow-up PR cycle. #### Fix 1: normalize disabled tool names during MCP restart refresh `packages/cli/src/acp-integration/acpAgent.ts:1563-1566` The bootstrap path in `cli/src/config/config.ts:1426-1434` applies a 4-step normalization to `tools.disabled`: 1. typeof string filter 2. .trim() 3. drop empty after trim 4. dedupe via Set The MCP-restart refresh path only did step 1, then stored the raw strings. `ToolRegistry` checks disabled tools with EXACT `Set.has(tool.name)`, so a tool disabled at boot as `' Foo '` (or `'Foo\n'`) is no longer matched after `restartMcpServer` and gets silently re-registered. This contradicts the documented "toggle + restart" workflow that #4282 PR 17 advertised. Fix: mirror the bootstrap normalization verbatim before `setDisabledTools`. Adds 6 lines + a 7-line comment pointing at the bootstrap reference for future maintainers. #### Fix 2: add headroom to MCP restart SDK timeout `packages/sdk-typescript/src/daemon/DaemonClient.ts:102` The SDK's `MCP_RESTART_DEFAULT_TIMEOUT_MS` was EXACTLY 300_000ms, the same ceiling the daemon's own `MCP_RESTART_TIMEOUT_MS` uses for the upper bound on a single MCP rediscovery. For restarts that finish (or fail with a typed `McpServerRestartFailedError` JSON envelope) near 300s, the client `AbortSignal` could fire BEFORE the daemon had finished serializing + transmitting the response, yielding a client `TimeoutError` even though the daemon was still within its own budget. Fix: bump to 330_000ms (10% / 30s headroom over the daemon ceiling). Comment updated to call out the race + the rationale for the specific headroom value. Callers needing tighter caps still pass their own `timeoutMs` to `restartMcpServer`. #### Why folded into F1 vs separate follow-up PRs These are post-merge findings on `#4282 PR 17` code, not F1-introduced regressions. Normally we'd track as separate follow-up issues (mirror of the #4325 / `channelInfo` decline). But: - Both fixes are TINY (~25 LOC + ~2 LOC including comment); the bridge security fold-in commit `7bd66c6e8` set the precedent of folding in small same-branch issues when the cost-benefit favors closing them immediately. - Same reviewer (wenshao via qwen-latest agent) — won't be confused by the scope expansion; in fact the original PR 17 commenter is also the one who'd review the follow-up issue's fix. - Both fixes target `daemon_mode_b_main`-only paths (MCP restart route added by PR 17 lives on the integration branch). - Saves opening 2 trivial follow-up issues that would just sit until someone picks them up. #### Verification - sdk-typescript: 424/424 tests pass (no test hardcoded the old 300_000 default — only the constant declaration itself referenced it) - cli acp-integration: 282/282 tests pass (no test exercised the exact whitespace-bearing disabled-tools scenario, so no test changes were strictly required; a regression test would belong in a separate test-coverage PR alongside the const.ts test gap from the #4297 unresolved-comment thread) - typecheck clean across cli + sdk-typescript 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * docs(acp-bridge): wenshao review round 4 — 3 Suggestion fold-ins (#4319) 1. **bridge.ts:2270 stale line refs in `publishWorkspaceEvent` JSDoc** — comment said `permission_resolved at line 1717` (actual: line 682) and `broadcastWorkspaceEvent closure at ~line 2127` (actual: line 1281). Line numbers drifted across the lift commits. Replaced both with function-name refs (`in resolvePending`, `declared above in this factory body`) that survive future edits. 2. **`ws.ts:613` opaque references in bridgeFileSystem.ts:20 + bridgeOptions.ts:267** — no `ws.ts` file exists in the repo; the ref came from an internal review thread on PR 18 that future readers can't locate. Replaced with a self-contained description ("post-PR-18 follow-up thread about BridgeClient's inline fs proxy bypassing WorkspaceFileSystem (originally raised in #4250 review)") plus a cross-reference to the FIXME(stage-1.5, chiga0 finding 4) already lifted into this package. 3. **bridge.ts:3503 duplicate `canonicalizeWorkspace` re-export** — `index.ts:11` already does `export * from './workspacePaths.js'` which exposes `canonicalizeWorkspace` through the package barrel. The bridge.ts re-export was a leftover from the lift that just duplicated the symbol at the barrel level (`bridge.ts` then re- exports it again via `index.ts`'s `export * from './bridge.js'`). Removed; `canonicalizeWorkspace` stays available via the package barrel + the `@qwen-code/acp-bridge/workspacePaths` subpath, which is what the cli shim already imports from. - 62/62 acp-bridge tests pass - 174/174 cli httpAcpBridge tests pass - typecheck + eslint clean 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * fix(acp-bridge): wenshao round 5 — killChild deadline log + stale line-ref cleanup (#4319) Folds in 1 of 3 wenshao Suggestions on F1 PR #4319 round 5; 2 declined with tracking issues opened (#4329, #4330). **Adopted:** `spawnChannel.ts:323` — `killChild` hard deadline now emits a stderr warning before abandoning a stuck child. Pre-fix the `setTimeout(KILL_HARD_DEADLINE_MS)` silently resolved the promise, letting `bridge.shutdown()` claim graceful shutdown while a `qwen --acp` zombie still held FDs / memory / locks. Under systemd/k8s supervision this lets the daemon respawn race the orphan for the same workspace. New warning is a single line on the daemon's stderr (`qwen serve: killChild hard deadline (10000ms) reached; child pid=... still alive (uninterruptible sleep?) — abandoning. Operator should check for zombie qwen --acp processes...`) so monitoring/log aggregators catch the zombie signal. **Partial adopt:** `acpAgent.ts:1564` — replaced the hard-coded `cli/src/config/config.ts:1426-1434` line-number cross- reference (will drift when config.ts is edited) with a content-anchor pointer ("search for `disabledTools` array population around the `tools.disabled` settings read"). Same class of stale-line-ref cleanup F1 already did across `bridge.ts` / `permission.ts` / `bridgeClient.test.ts`. **Declined** for F1 scope, both with tracking issues: - `acpAgent.ts:1564` — extract a shared `normalizeDisabledToolList()` helper for the boot path + restart path so future enhancements (case-folding, Unicode normalization, plugin-name aliasing) only edit one site. Tracked as #4329. - `DaemonClient.ts:112` — enforce SDK/server MCP-restart timeout coupling so a future bump on either side doesn't silently re-introduce the race that `b78de2719` fixed. Tracked as #4330 (shared constant vs cross-package integration test vs startup assertion — three options enumerated). Both extractions have real merit but are structural refactors that sit outside F1's "mechanical lift + targeted security/doc fixes" scope. Folding either would add new shared-utility / shared-package plumbing the lift PR explicitly avoids. - 62/62 acp-bridge tests pass - 174/174 cli httpAcpBridge tests pass - typecheck + eslint clean 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * refactor(cli): extract normalizeDisabledToolList helper — fold-in for wenshao #4319 round 5 (closes #4329) Folds in wenshao Suggestion from #4319 round 5 (originally declined as out-of-scope, opened as #4329 for follow-up tracking). User pushed back that the helper is small enough + same package as the duplicate sites, so doing it inline rather than as a separate follow-up PR closes the review thread completely. ## Change New file `packages/cli/src/config/normalizeDisabledTools.ts`: ```typescript export function normalizeDisabledToolList(raw: unknown): string[] ``` 4-step normalization (`typeof string` filter + `.trim()` + drop empty + dedupe preserving first-occurrence order). Non-array `raw` short- circuits to `[]` so callers can pass arbitrary settings-shaped input without `Array.isArray` boilerplate. Replaces two byte-identical inline implementations: - `packages/cli/src/config/config.ts:1426-1434` (bootstrap path) — was 9 lines of inline trim+dedupe loop. - `packages/cli/src/acp-integration/acpAgent.ts:1571-1591` (MCP restart refresh path) — was 10 lines + an `Array.isArray` gate + 20 lines of explanatory comment about why it had to mirror the bootstrap path. Both call sites now just call `normalizeDisabledToolList(raw)`. ## Why it matters `ToolRegistry.has(tool.name)` is an exact-string match. A hand-edited `tools.disabled: [' Foo ', '', 'Foo']` settings entry must produce `Set(['Foo'])` at boot AND after every `restartMcpServer` — otherwise the boot-disabled tool gets silently re-registered after the next MCP restart (the bug Codex P2 originally caught in `b78de2719`). Sharing the helper makes future enhancements (Unicode normalization, plugin- name aliasing, case-folding decisions) edit exactly one site. ## Tests New `packages/cli/src/config/normalizeDisabledTools.test.ts` (16 tests) covering: - non-array short-circuit (undefined, null, object, number, string, bool) - typeof-string filter (drops mid-array non-strings without aborting) - trim + empty-skip (whitespace-only entries dropped) - dedupe (exact match, whitespace variants collapse to first occurrence, case NOT folded) - boot/restart parity scenarios (the BkwQW class the helper was written to prevent) - order preservation across trim + dedupe ## Refs - Closes #4329 - F1 PR #4319, originally tracked the helper extraction as deferred (commit `5f6b55e80` round 5 reply); now folded in here. - Original duplicate introduction was `b78de2719` (Codex P2 fold-in for MCP restart normalization). 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * feat(serve): F1 follow-up — BridgeFileSystem wiring + #4325 channelInfo fix (#4334) * fix(acp-bridge): use channelInfoForEntry in closeSession + killSession (#4325) Folds in the deferred fix from F1 (#4319) for #4325. Pre-fix both methods captured `const ci = channelInfo` — the module-scoped CURRENT attach target — rather than `channelInfoForEntry(entry)`. The two diverge during the channel-overlap window (A dying, B freshly spawned as `channelInfo`), where closing or killing a session whose `entry.channel = A` would: 1. Skip `A.sessionIds.delete()` because `B.channel !== A.channel`, leaving A's `sessionIds` set pinned past the close; 2. Call `markSessionClosed` on **B**'s client instead of **A**'s, evaluating B's kill condition with stale assumptions about its session count — potentially killing B unnecessarily and forcing a third spawn cascade. Other session methods in the same factory (`setSessionApprovalMode` at ~L2609, `requestSessionStatus` at ~L1245) already use the `channelInfoForEntry(entry)` helper; this brings `closeSession` and `killSession` in line with that pattern. Net change: 2 lines (one in each method) replaced; surrounding comment blocks updated to document the channel-overlap rationale + the matching sibling-method consistency argument. ## Why the smoke test rather than a full overlap regression The exact bug-triggering state is hard to construct deterministically under the current factory architecture: - A only flips `isDying = true` when its `sessionIds` drains to 0 - The drain path (`killSession` or `closeSession` on the last session) also removes the session from `byId` synchronously - So by the time `channelInfo` could move to B, every session that was on A is gone from `byId` and thus unreachable to a subsequent `closeSession` A faithful overlap regression test requires a test-only factory inspection seam (manual `channelInfo` override, or a hook into `aliveChannels` mutation). Adding that seam is non-trivial and expands the bridge's public surface — out of F1-followup scope. What this commit ships: - The 2-line fix itself (matches the sibling-method pattern; the correctness argument is structural, not race-empirical) - A smoke regression test at `httpAcpBridge.test.ts` exercising `closeSession` on the normal single-channel case and asserting the kill-on-last-session cascade fires correctly — would fail trivially if a future refactor reverted to module-scoped `channelInfo` capture without thinking through the `channelInfoForEntry → undefined` case - Inline comments at both fix sites + on the new test documenting why the full overlap repro is deferred A follow-up issue can track adding the factory inspection seam + the deterministic overlap regression test if anyone needs the empirical guard rather than the structural one. - 175/175 cli httpAcpBridge tests pass (174 existing + 1 new #4325 smoke) - 62/62 acp-bridge tests pass (no regression) - typecheck + eslint clean - Closes #4325 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * feat(serve): wire WorkspaceFileSystem into BridgeFileSystem seam (F1 follow-up #4319) Closes the ws.ts:613 TOCTOU thread that PR 18 (`WorkspaceFileSystem`) flagged and that F1 (#4319) deliberately left to a follow-up by shipping only the `BridgeFileSystem` injection seam in `BridgeClient`. Pre-fix, ACP `writeTextFile` / `readTextFile` calls landed in `BridgeClient`'s inline `fs.realpath` / `fs.writeFile` / `fs.readFile` proxy, bypassing PR 18's defensive layer (trust gate, symlink resolution, atomic temp-file write, line/limit windowing, audit emit). HTTP `POST /file` / `GET /file` already routed through that layer — agent fs and HTTP fs diverged in posture. Changes - New `bridgeFileSystemAdapter.ts` (~110 LOC): thin translation from ACP `WriteTextFileRequest` / `ReadTextFileRequest` to `WorkspaceFileSystem.resolve` → `writeText` / `readText`. Drops ACP-wire `null` line/limit (PR 18 wants `undefined`). Routes labeled `'ACP writeTextFile'` / `'ACP readTextFile'` so the unified audit stream can distinguish agent fs from HTTP fs at the consumer side. - `runQwenServe.ts` + `server.ts`: construct `fsFactory` BEFORE the bridge default and pass `fileSystem: createBridgeFileSystemAdapter(fsFactory)` into `BridgeOptions`. Same factory instance feeds both HTTP fs routes and ACP fs → single operator audit stream covers both. - New `bridgeFileSystemAdapter.test.ts` (10 tests, all pass): happy paths (trusted write + read), trust-gate deny, boundary rejection (writes + reads outside workspace), line/limit window, null→undefined normalization, `factory.forRequest` audit-context wiring (sessionId forwarded, omitted when ACP request lacks one). Backward compatibility - `BridgeOptions.fileSystem` was already optional in F1 (seam-only); embeds that don't pass it (or that pre-date this commit) keep using `BridgeClient`'s inline raw-fs proxy as before. This commit only changes the *default* `createServeApp` + `runQwenServe` wiring. Verification - `vitest run src/serve/`: 18 files, 746/746 tests pass (includes the 10 new adapter tests + the 175-test `httpAcpBridge.test.ts` that exercises the seam through `BridgeOptions.fileSystem`). * fix(serve): preserve mode + atomic write for ACP writeTextFile (#4334 Copilot review) Adopts Copilot's finding on PR #4334 (security-relevant): https://github.com/QwenLM/qwen-code/pull/4334#discussion_r3268275710 Pre-fix, the adapter routed ACP writeTextFile through `WorkspaceFileSystem.writeText` which has no mode handling — new files got umask-default (typically 0o644) and existing-target mode wasn't preserved. The `BridgeFileSystem` contract requires 0o600 for new files (NOT umask) and target mode preservation (a 0o600 secret edit must stay 0o600). The old inline `BridgeClient.writeTextFile` proxy did this; the adapter regressed it. Fix: add a new `writeTextOverwrite` primitive to PR 18's `WorkspaceFileSystem` (Approach B from the design discussion — picked over CAS-in-adapter because the "unconditional create-or-overwrite with mode preservation" semantic will recur in F4 TUI/IDE adapters and future webhook integrations; cleaner to land it as a reusable PR 18 primitive now than retrofit later). Implementation - `WorkspaceFileSystem.writeTextOverwrite(p, content, opts?)` — unconditional create-or-overwrite, no expectedHash gate. Reuses the existing `atomicWriteTextResolvedFile` infrastructure via a new `WriteMode = 'overwrite'` variant: tolerates missing target (returns empty mode → 0o600 default), rejects symlinks (`symlink_escape`), preserves existing mode bits (`chmod` to `targetState.mode ?? 0o600` in line 1450). Path-locked the whole window; emits the same `fs.access` audit as `writeText` / `writeTextAtomic`. - `assertAtomicTargetPrecondition` gains an `'overwrite'` branch that stats the target, returns its mode for preservation, and tolerates ENOENT (new file path); rejects symlinks / non-regular files in parity with `'replace'`. - `validateWriteTextAtomicOptions` accepts `'overwrite'` mode WITHOUT expectedHash — that's the whole point of the new primitive (callers whose wire format has no client-side hash, like ACP). - `atomicWriteTextResolvedFile`'s rename branch handles `'overwrite'` automatically (falls through to `renameWithRetryLocal` like `'replace'`; rename both clobbers existing and creates new). Adapter switch - `bridgeFileSystemAdapter.ts:96` — `wfs.writeText(resolved, content)` → `wfs.writeTextOverwrite(resolved, content)`. Updated docstring explains why this primitive over `writeText` (no mode) or `writeTextAtomic` (CAS gate doesn't fit ACP's hash-less wire). Contract update - `bridgeFileSystem.ts:61-93` — `writeText` doc now reflects the production posture: write-then-rename atomicity, target mode preservation, 0o600 default for new files, **symlink rejection**. The pre-F1 inline proxy resolved symlinks and wrote through to the target; PR 18 + HTTP `POST /file` (PR 20) reject them. The adapter now matches that posture, so ACP fs and HTTP fs behave identically — a divergence from pre-F1 ACP semantics, called out explicitly. Tests (+10, 81 passing on touched files) - workspaceFileSystem.test.ts: writeTextOverwrite creates new file at 0o600, preserves existing target mode (0o600 secret stays 0o600), preserves +x executable bit, rejects post-resolve symlink swap with symlink_escape, enforces trust gate, emits fs.access. - bridgeFileSystemAdapter.test.ts: through-adapter assertions that new files land at 0o600 and existing 0o600 secrets stay 0o600 after agent overwrite. Skipped on Windows (POSIX permission bits not honored). Symlink rejection is covered at the lower workspaceFileSystem layer to avoid duplicating the post-resolve-swap setup. * fix(serve): 4 wenshao review fold-ins on #4334 (2 Critical + 2 Suggestion) All four threads from the wenshao review round on PR #4334 (Qwen `/review`) — adopted as-suggested with the fixes outlined below. **[Critical] writeTextOverwrite blocks on large/binary existing files** (`workspaceFileSystem.ts:849` thread r3270664710) `readExistingTextMeta(p)` reads the existing file just for encoding / BOM / line-ending hints (best-effort meta). My earlier catch only swallowed `ENOENT`, so `file_too_large` (>256 KiB) and `binary_file` errors propagated and **blocked the overwrite entirely**. Pre-PR ACP `BridgeClient.writeTextFile` never read the existing file at all — an agent overwriting a 1 MiB log or binary config would have always succeeded. Bubbling those classified errors regressed that. Fix: catch ENOENT + file_too_large + binary_file; leave `existingMeta` undefined and let `mergeWriteMeta` fall back to UTF-8/no-BOM/LF defaults. New tests cover both scenarios. Side fix uncovered while writing the tests: `created` was derived from `existingMeta === undefined` which is wrong after this catch widening — a binary or too-large existing file would now report `created: true`. Replaced with an explicit `lstat` to detect target existence independently of meta-read success. **[Critical] writeTextAtomic({mode:'overwrite'}) is unsupported** (`workspaceFileSystem.ts:146` thread r3270664723) `WriteMode` was widened to include `'overwrite'` and `validateWriteTextAtomicOptions` accepted it — but `writeTextAtomic`'s `existingMeta` branch only reads meta for `mode === 'replace'` AND `created: opts.mode === 'create'` is hard-coded so `'overwrite'` always reports `created: false` even for new files. Direct callers of `writeTextAtomic({mode: 'overwrite'})` would silently lose CRLF on Windows files and misreport new-file creation. The dedicated `writeTextOverwrite()` method handles both correctly and is the only supported entry point for unconditional-overwrite semantics. Fix (option b from the reviewer): reject `'overwrite'` in `validateWriteTextAtomicOptions` with a `parse_error` that names the correct method. The `WriteMode` union still admits `'overwrite'` internally (so `atomicWriteTextResolvedFile` + `assertAtomicTarget Precondition`'s 'overwrite' branch compile), but no external caller can reach those code paths via `writeTextAtomic`. The error message points to `writeTextOverwrite()` so misuse surfaces an actionable hint. **[Suggestion] killSession #4325 fix missing symmetric regression test** (`httpAcpBridge.test.ts:6421` thread r3270664724) The earlier #4325 fix touched both `closeSession` AND `killSession` (both `const ci = channelInfo` → `const ci = channelInfoForEntry(entry)`) but the smoke test only exercises closeSession. A future refactor reverting `killSession` alone would pass all existing tests. Fix: add a symmetric `killSession` smoke test mirroring the closeSession shape — single-channel kill → assert handle.killed + sessionCount = 0. Same overlap-race caveat documented inline. Future deterministic overlap test still deferred to the same follow-up that adds factory inspection seams. **[Suggestion] createServeApp default `trusted: false` silently rejects agent writes for embeds** (`server.ts:257` thread r3270664727) `createServeApp` constructs its default `fsFactory` with `trusted: false` (test-safe posture), and now wires it into the bridge via `createBridgeFileSystemAdapter(fsFactory)`. Pre-PR ACP `writeTextFile` went through the inline raw-fs proxy which had no trust gate. Any embed using `createServeApp` without providing `deps.fsFactory` or `deps.bridge` will now have ALL agent writes silently reject with `untrusted_workspace`. `runQwenServe` consumers are unaffected (defaults `trusted: true`), but IDE companions / hosted daemons calling `createServeApp` directly are at risk. Fix: emit a stderr startup warning when `deps.fsFactory` is not provided, explicitly naming the asymmetry and the three opt-out paths (provide fsFactory, provide bridge, or accept the gate). Visible to operators so the trust-gate-default isn't an opaque "writes silently fail" mystery in production. Additional test gaps closed (sub-bullet from r3270664724): - adapter-level `readText` trust-gate parity check — verifies that `trusted: false` does NOT extend to reads (PR 18's trust gate is write-only). A future refactor mistakenly gating reads would only fail HTTP-fs tests, not adapter ones. - `writeTextOverwrite` non-regular-file rejection — pins the `parse_error` posture for directory targets so a relaxation in `assertAtomicTargetPrecondition`'s 'overwrite' branch is caught. Verification - `npx vitest run packages/cli/src/serve/` — 18 files, 760/760 pass (+6 new tests over the previous 754) - `cd packages/acp-bridge && npx vitest run` — 5 files, 62/62 pass - Pre-commit (`prettier --write` + `eslint --fix --max-warnings 0`) clean on all 5 staged files * fix(serve): 4 more wenshao fold-ins on #4334 (1 Critical + 3 Suggestion) Adopts 4 of 7 wenshao review threads on PR #4334. The remaining 3 (1 Critical + 2 placeholder) are surfaced separately for user judgment — the Critical's suggested fix doesn't work as-is and needs a design call; the 2 placeholders look like reviewer-tool tests ("JSDoc test." / "test"). **[Critical] EACCES/EPERM blocks overwrite** (r3270921396, ws.ts:877) The earlier r3270664710 fix widened the meta-read catch to swallow ENOENT + file_too_large + binary_file. wenshao caught that EACCES / EPERM also need to be swallowed — a file the daemon can't read (0o000, other-user-owned) would abort the overwrite, contradicting the "best-effort meta read" comment. Also opens an agent-side probe: an attacker could detect file readability by observing EACCES on overwrite attempts. Fix: extend the catch to also swallow EACCES + EPERM. Comment block expanded to spell out the full set (ENOENT / EACCES / EPERM / file_too_large / binary_file) and the probing-defense rationale. Test: `writeTextOverwrite succeeds over an existing 0o000 (unreadable) file` — pins the posture so a regression here is caught. Skipped on Windows + when running as root (root bypasses POSIX mode bits). **[Suggestion] Negative `limit` produces wrong content** (r3270921401, bridgeFileSystemAdapter.ts:112) Pre-PR the inline `BridgeClient.readTextFile` returned `{ content: '' }` for `limit <= 0`. PR 18's `readText` applies `slice(0, limit)`, which for `limit: -1` returns "all lines except the last" — wrong content. Same hazard for non-positive `line` (PR 18 rejects with `parse_error` for `line < 1`, smuggling a 4xx-shaped error to agents that previously got `''`). Fix: tighten the adapter's `typeof === 'number'` guard to also require `> 0`. Comment expanded to call out the divergence and why "drop and let PR 18 default to no-windowing" is the closest match to pre-PR empty-content posture without leaking parse_error. Tests: `drops non-positive limit (negative / zero) instead of forwarding` + `drops non-positive line (zero) instead of forwarding parse_error`. **[Suggestion] Warning fires when deps.bridge is provided** (r3270921402, server.ts:266) Earlier r3270664727 fix added a startup stderr warning when `deps.fsFactory` is not provided. wenshao caught that the warning also fires when `deps.bridge` IS provided — but in that case the embed owns its own fileSystem wiring (the default adapter never runs), so the warning's claim about ACP writes rejecting is false. Fix: narrow guard to `!deps.fsFactory && !deps.bridge`. Comment expanded to explain why bridge-injection suppresses the warning. **[Suggestion] No oversized-payload test for writeTextOverwrite** (r3270921399, ws.ts:835) `writeTextOverwrite` calls `enforceWriteSize(decodedSizeBytes)` mirroring `writeText`'s 5 MiB cap, but the existing oversized-write test only exercises `writeText`. A regression dropping the check on the new method would let agents (the primary consumer) write arbitrarily large files undetected. Test: `writeTextOverwrite rejects content exceeding MAX_WRITE_BYTES with file_too_large`. Verification - `npx vitest run packages/cli/src/serve/` — 18 files, 764/764 pass (+4 new tests over the previous 760) - Pre-commit (`prettier --write` + `eslint --fix --max-warnings 0`) clean on all 5 staged files * fix(serve): 4 wenshao/deepseek fold-ins on #4334 (1 Critical refactor + 3 Suggestion) Adopts 4 of 5 new threads from the DeepSeek-v4-pro review round on PR #4334 (Qwen `/review`). The 5th (DWcK8) is a duplicate of a test already in commit 9f73b83d7 — declined separately with a pointer. **[Critical] Trust-default asymmetry between runQwenServe ↔ createServeApp** (r3270978579, server.ts DWcK4) `runQwenServe.ts` defaults `trusted: true` (production daemon), `server.ts` defaults `trusted: false` (test-safe). The asymmetry is intentional but lives in two places — a future maintainer can break the alignment without any compile-time signal. The earlier stderr warning (commit e185409d5) covers the embed-omits-fsFactory case but NOT a regression in the runQwenServe → createServeApp pass-through. Fix: extract `resolveBridgeFsFactory(input)` helper in `server.ts` (exported alongside `createDefaultFsAuditEmit`). Both call sites use it. Trust stays a REQUIRED parameter — the policy difference is preserved at the call sites, but the construction shape (build vs inject + audit-emit default) is centralized. Defense-in-depth, not behavior change. **[Suggestion] adapter JSDoc claim about `mapDomainErrorToErrorKind` is misleading** (r3270978595, DWcLB) The docstring at `bridgeFileSystemAdapter.ts:38` says "the bridge's existing `mapDomainErrorToErrorKind` classifier downstream picks up `FsError` codes". This is false: `mapDomainErrorToErrorKind` in `acp-bridge/src/status.ts` checks `instanceof` / `.name` / `.code` (Node errno names), but has NO branch reading `err.kind` (FsError's discriminator: `untrusted_workspace` / `symlink_escape` / etc.). Errors still propagate (the `.kind` field rides through on the thrown FsError object itself), but a future maintainer debugging error classification during an incident would chase the wrong code path. Fix: rewrite the docstring to describe the actual flow — `FsError` is thrown unchanged through BridgeClient's ACP handlers; downstream consumers reading the ACP error payload key on `.kind` directly. The HTTP `sendFsError` serializes the same `.kind`, so SDK consumers see the same shape from either surface. Adding a real `instanceof FsError` branch to `mapDomainErrorToErrorKind` would need cross- package imports (FsError lives in `cli/src/serve/fs`, classifier in `acp-bridge`) — explicitly deferred to a separate PR. **[Suggestion] adapter readText error propagation untested** (r3270978593, DWcK_) Read-side errors from `wfs.readText` (`file_too_large`, `binary_file`, `symlink_escape`) propagate untested through the adapter — the existing tests cover trust-gate (already write-only), line/limit forwarding, null/non-positive guards, and boundary, but not the `FsError` classes themselves. A regression silently swallowing or wrapping them would only fail HTTP-fs tests. Fix: add 3 adapter tests pinning `file_too_large` / `binary_file` / `symlink_escape` propagation surface as-is via the adapter's re-thrown error. **[Suggestion] channelInfoForEntry HAZARD comments on bridge.ts fix sites** (r3270978598, DWcLD) The regression test for the `#4325` fix (`httpAcpBridge.test.ts:6421`) is single-channel smoke only — its own comment acknowledges "a reverted fix that captured `channelInfo` after the entry was gone from `byId` would also pass this assertion". The actual overlap-race state isn't deterministically constructable without factory-internal hooks. Until the deterministic test lands, the only defense against accidental revert is code-review visibility. Fix: add `HAZARD(#4325)` comments at both `closeSession` and `killSession` fix sites in `acp-bridge/src/bridge.ts`, explicitly flagging that the existing smoke test would not catch a revert and that the `channelInfoForEntry(entry)` call must NOT be refactored away without first landing the deterministic overlap test. Verification - `npx vitest run packages/cli/src/serve/` — 18 files, 767/767 pass (+3 new adapter tests; the prior 760→767 includes runs of multi- tick fold-ins on the same branch). - `cd packages/acp-bridge && npx vitest run` — 5 files, 62/62 pass - Pre-commit (`prettier --write` + `eslint --fix --max-warnings 0`) clean on all 5 staged files * fix(serve): 4 more wenshao fold-ins on #4334 (DWrbe/DWrbl/DWrbn/DWrbr) Adopts the second round of DeepSeek-v4-pro suggestions on PR #4334. All 4 are small, targeted improvements without controversy. **DWrbe — WriteMode admits 'overwrite' at compile time** (r3271063030) `WriteTextAtomicOptions.mode` was typed as `WriteMode` (which includes 'overwrite'), but `validateWriteTextAtomicOptions` throws `parse_error` for that value. The runtime error catches misuse but TypeScript happily lets the call through. Fix: introduce `AtomicWriteMode = Exclude` public type and narrow `WriteTextAtomicOptions.mode` to it. Runtime validator stays as defense-in-depth. **DWrbl — boundary tests use bare .rejects.toThrow()** (r3271063040) Both boundary-enforcement adapter tests asserted "throws" without pinning the FsError kind. Incidental OS errors (CI container EACCES on /etc/passwd) or future pre-check additions could pass these tests trivially while masking that boundary enforcement isn't firing. Fix: assert `.kind === 'path_outside_workspace'` for both sides. **DWrbn — trust warning floods stderr in tests** (r3271063045) The startup warning fires on every createServeApp call. server.test.ts calls createServeApp ~25 times, masking genuine failures. Fix: module-scoped once-per-process guard `warnedDefaultTrust`. Module scope (not per-app closure) because the warning is a posture statement about this binary, not per-instance. **DWrbr — channelInfoForEntry undefined is silent** (r3271063052) closeSession / killSession's cleanup branches short-circuit silently when channelInfoForEntry returns undefined (entry's channel torn down). The "closing session" log fires but the skipped-cleanup fact is invisible, making zombie-channel debugging harder. Fix: emit stderr diagnostic naming session id + which method short-circuited + likely cause. Sibling methods like requestSessionStatus throw SessionNotFoundError; close/kill are idempotent so we log instead. Verification: serve 767/767, acp-bridge 62/62, pre-commit clean. * feat(acp-bridge): F3 — multi-client permission coordination (#4175) (#4335) * feat(acp-bridge): F3 — multi-client permission coordination (#4175) [rebased onto F1] Squashed F3 implementation rebased from origin/main onto daemon_mode_b_main (post-F1 #4319). F1 lifted the bridge core to @qwen-code/acp-bridge package; F3's edits to the pre-F1 httpAcpBridge.ts BridgeClient class + factory were ported to the new file locations: - BridgeClient.requestPermission rewrite → bridgeClient.ts - Factory mediator construction / pendingPermissions deletion / cancelPendingForSession refactor / respondTo*Permission rewrites / pendingPermissionCount + permissionPolicy getters / teardown sites (closeSession, killSession, shutdown drain) → bridge.ts - Error class re-exports → cli/src/serve/httpAcpBridge.ts shim (added CancelSentinelCollisionError, PermissionForbiddenError, PermissionPolicyNotImplementedError to the F1 re-export block) This commit folds 13 logical F3 commits + 4 review fold-ins (Copilot inline comments + 3 final-pass agent reviews) into a single post-rebase squash. The full review trail is in .claude/plans/fluttering-coalescing-kettle*.md (worktree-local). Strategies (4): first-responder (default, byte-for-byte preserved), designated, consensus (default N=floor(M/2)+1), local-only. New SSE events: permission_partial_vote, permission_forbidden. Capability tag: permission_mediation (always-on with build-supported modes list); active policy at /capabilities.policy.permission. Settings: policy.permissionStrategy enum + policy.consensusQuorum number, both requiresRestart: true (F3 v1 reads at boot). 3 new typed errors: PermissionForbiddenError → 403, PermissionPolicyNotImplementedError → 501 (forward-compat for future policy literals), CancelSentinelCollisionError → 500 (agent / daemon contract violation). Hardness invariants: N1 synchronous-register, N2 cleanup ordering, N3 originatorClientId stamping, O5 cancel sentinel pre-publish collision check, O8 pre-F3 permission_resolved wire shape preserved. Tests: 35 mediator unit + 10 audit ring + 56 SDK reducer + 6 bridgeClient + 3 bridge integration. Pre-existing httpAcpBridge.test.ts cross-session-vote suite passes byte-for-byte. Issue: #4175 (F3) * fix(f3): build/capability fixes from Copilot review (#4335) - packages/sdk-typescript/src/daemon/index.ts: re-export the four F3 permission event types (`DaemonPermissionForbiddenData/Event`, `DaemonPermissionPartialVoteData/Event`) so the public package barrel at `src/index.ts` (which forwards them via `from './daemon/index.js'`) resolves at build time. Without this fix `npm run build --workspace=packages/sdk-typescript` failed with TS2305/TS2724; vitest passed only because it resolves TS source via tsx and bypasses tsc compilation. Reported in PR #4335 review comments 3270615836 / 3270622302 (wenshao via Qwen Code /review). - packages/cli/src/serve/server.test.ts: append `'permission_mediation'` to `EXPECTED_STAGE1_FEATURES` and adjust `EXPECTED_REGISTERED_FEATURES` reordering so the test fixture matches the registry's actual order (`...workspace_mcp_restart, require_auth, auth_device_flow, permission_mediation`). Without this fix four `serve capability registry` tests asserted via `.toEqual` against a stale list. - docs/developers/qwen-serve-protocol.md: swap `permission_mediation` and `auth_device_flow` in the documented capability list so the order mirrors `SERVE_CAPABILITY_REGISTRY` declaration order. - packages/vscode-ide-companion/schemas/settings.schema.json: regenerate the IDE-companion JSON schema with the new `policy` section (was pending from Commit 5 of the F3 series; checked in here so the IDE companion sees the same `permissionStrategy` / `consensusQuorum` shape that the CLI accepts). 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * fix(f3): wire production audit ring + restore timeout stderr (#4335) Wenshao review #4335 surfaced two related Critical findings: 1. **Audit publisher silently no-op in production** (3270622298). The `bridgeOptions.ts:305` JSDoc claimed "the bridge allocates an internal `PermissionAuditRing`" but the actual fallback at `bridge.ts:543` is `createNoOpPermissionAuditPublisher()`, and `runQwenServe.ts` never wired one. All 5 audit record types (`requested`, `voted`, `forbidden`, `resolved`, `timeout`) were silently discarded — the forensic audit trail the F3 plan committed to ("ring 留给后续 PR 加查询接口") never existed in any deployed daemon. 2. **Timeout breadcrumb lost** (3270622304). Pre-F3 wrote `"timed out after Xms"` to daemon stderr on every permission timeout. F3 removed that direct write and delegated to `audit.recordTimeout()`, but the audit publisher is the no-op fallback in production (see #1). Operators tailing daemon stderr could no longer observe permission timeouts. Fixes: - `runQwenServe.ts` allocates a `PermissionAuditRing` (default cap 512) + `createPermissionAuditPublisher` and passes the publisher via `BridgeOptions.permissionAudit`. The ring is held in the daemon host's closure for the lifetime of the daemon — a future `GET /workspace/permission/audit` route (out of F3 v1 scope) can lift it out for query without further bridge changes. - `permissionMediator.ts` writes the stderr breadcrumb directly from the timer callback, before forwarding to the (potentially no-op) audit publisher. Wrapped in try/catch because `process.stderr.write` can synchronously throw on EPIPE — losing observability is preferable to crashing the timer queue. - `bridgeOptions.ts` JSDoc rewritten to match reality: the bridge falls back to a no-op publisher; production wiring lives in `runQwenServe.ts`; the stderr breadcrumb is in the mediator (independent of the publisher). - New unit test `writes a stderr breadcrumb when the timer fires` spies on `process.stderr.write` and asserts the breadcrumb format contains the requestId, sessionId, and the timeout duration so future refactors can't silently drop the line again. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * fix(f3): drop dead helper + propagate originator to F3 view state (#4335) Two small follow-ups from wenshao review #4335: - **`bridge.ts:672-682` — dead `_resolutionToAcpResponse` helper** (3270622309). Defined and immediately suppressed with `void`. The identical `resolutionToAcpResponse` lives at `bridgeClient.ts:41` and is the one actually used by `BridgeClient.requestPermission` — the bridge-factory copy was a stranded leftover from the lift out of inline closures into the mediator pattern. Removed declaration, `void` statement, and the now-unused `RequestPermissionResponse` (`@agentclientprotocol/sdk`) and `PermissionResolution` (`./permission.js`) imports. - **SDK reducer `mergeOriginator` for F3 events** (3270622311). The mediator stamps `originatorClientId` (= prompt originator per N3) on the `permission_partial_vote` / `permission_forbidden` envelope, but the reducer cases used `next.push({ ...event.data })` which only copies `data` fields. SDK consumers reading `permissionVoteProgress[reqId]` / `forbiddenVotes[i]` could not determine which client's prompt was targeted by the partial-vote progress / forbidden vote — same gap PR #4282 fixed for approval-mode / tool-toggle / workspace-init / mcp-restart. Applied the existing `mergeOriginator` helper to both reducer cases. Added `originatorClientId?: string` to both Data interfaces with JSDoc explaining the propagation contract (preserve any pre-existing `data.originatorClientId`; otherwise stamp from the envelope; for forbidden votes the field is distinct from `data.clientId` which carries the rejected voter). Three new reducer tests: 1. `permission_partial_vote` propagates envelope originator into `permissionVoteProgress`. 2. `permission_forbidden` propagates envelope originator into `forbiddenVotes`, distinct from `data.clientId`. 3. `mergeOriginator` preserves any pre-existing `data.originatorClientId` over the envelope value. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * fix(f3): wenshao Round 4 — defensive stderr, audit accuracy, orphan cleanup (#4335) Four findings from wenshao review #4324937255 — the Critical one masked an actual hang scenario; the other three are observability / correctness fixes that round out F3 v1. **[Critical] safeEmit / safeAudit stderr breadcrumb wraps** (3271041461). Both helpers wrote `process.stderr.write` inside their `catch` block WITHOUT a nested `try/catch`. If stderr itself synchronously throws (EPIPE during daemon shutdown), the exception escapes the "safe" wrapper. In `resolveEntry`'s cleanup ladder (`safeEmit → rememberResolved → safeAudit → pending.resolve`), an escaping safeEmit exception aborts before `pending.resolve(resolution)` runs — the request was already deleted from `this.pending` (no double-resolve guard), so the agent's awaiting Promise never settles. `requestPermission` hangs until the timeout fires. The timer callback already wraps its breadcrumb in `try/catch` for the same reason — applied the matching pattern to safeEmit + safeAudit. **[Suggestion] Idempotent re-vote audit shows attempted optionId, not the original** (3271041464). When `client_A` originally voted for `proceed_once` and later attempts `proceed_always`, the tally silently keeps `proceed_once` (idempotent) but the audit ring recorded `optionId: proceed_always`. An operator reading the ring would see a vote for proceed_always that never counted toward quorum. Look up the originally-voted option from the tally and substitute it into the audit record. Added regression test asserting the audit reflects tally state. **[Suggestion] SDK reducer leaks `permissionVoteProgress` on mid-permission reconnect** (3271041465). When an SDK client reconnects and misses `permission_request`, then receives `permission_partial_vote` (stored in `permissionVoteProgress`), then receives `permission_resolved` — the early-return path on unmatched `requestId` did NOT clear `permissionVoteProgress`. The orphan progress entry persisted until session end. Both `permission_resolved` and `permission_already_resolved` reducer cases now unconditionally clear any orphan entry on the unmatched path. Two new reducer tests cover the recovery contract; the misleading "the next `permission_resolved` will clear both" comment on `permission_partial_vote` is corrected. **[Suggestion] Document votersAtIssue snapshot timing window** (3271041469). The snapshot fires synchronously after `entry.events.publish`, with no event-loop yield between, so a NEW HTTP client cannot register between publish and snapshot. But an SSE-only subscriber (no `X-Qwen-Client-Id` registered yet) that connected BEFORE publish is invisible to the snapshot — `consensus` silently rejects its later vote as `forbidden`. Documented the window in `votersForSession` JSDoc; future PRs surfacing `eligibleVoters[]` on `permission_request.data` should source it from the same snapshot for consistency. No code change — the narrow window is acceptable for F3 v1, and the structural fix (snapshot at publish time) requires bridge-level refactor. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * fix(f3): wenshao Round 5 — sentinel injection guard, observability, /8 loopback (#4335) Four findings from wenshao review #4325130053. The Critical one is a real security gap; the others are observability + correctness hardening. **[Critical] Cancel sentinel injection bypass** (3271185588). The mediator's `vote()` recognizes `CANCEL_VOTE_SENTINEL` BEFORE validating the option against `allowedOptionIds`, so a wire client sending `{outcome:'selected', optionId:'__cancelled__'}` would short-circuit ALL policy dispatch (designated originator check, consensus quorum, local-only loopback gate). The mediator's JSDoc documented the precondition ("callers MUST NOT forward an incoming vote.optionId === CANCEL_VOTE_SENTINEL from a wire client") but the precondition was never enforced — the bridge's `respondToSessionPermission` mapped the wire optionId straight through. Added an explicit `InvalidPermissionOptionError` throw when the wire payload is `{selected, CANCEL_VOTE_SENTINEL}`. The collision-defense at request issue time (`CancelSentinelCollisionError`) already prevents agents from advertising the sentinel as a legitimate option; this closes the remaining vector. **[Suggestion] Silent quorum cap + M=0 hang observability** (3271185594). Two related diagnostic gaps in the consensus policy: - When `policy.consensusQuorum` exceeds `votersAtIssue.size`, the cap fires silently. Operators investigating "why did consensus resolve at N=2 when I configured 5?" had no breadcrumb. - When `policy === 'consensus'` and `votersAtIssue.size === 0`, every vote rejects as `forbidden: designated_mismatch` because the empty snapshot can never match any voter clientId. The request hangs until `permissionTimeoutMs` with no diagnostic signal. Added stderr breadcrumbs at both points: cap-applied (once per request via a `consensusQuorumCapNoted` flag on `MediatorPending`) and at issue time when consensus M=0. No semantic change — the cap and the timeout-only resolution behavior are intentional per the F3 plan; the breadcrumbs just make them debuggable. **[Suggestion] detectFromLoopback misses 127.0.0.0/8** (3271185597). Per RFC 1122 the entire `127.0.0.0/8` block is loopback. The exact-match Set of three literals (`127.0.0.1`, `::1`, `::ffff:127.0.0.1`) silently fail-CLOSED on legitimate `127.0.0.2` / `127.0.1.1` / `::ffff:127.0.0.2` peers, causing unexpected `remote_not_allowed` rejections under `local-only` policy. Switched to a prefix test so the entire `/8` and its dual-stack mirror are accepted. Direction stays fail-CLOSED for unrecognized address shapes. **[Suggestion] VSCode JSON schema integer/min validation** (3271185604). `runQwenServe.ts` validates `Number.isInteger(consensusQuorum) && >= 1`, but the generated `settings.schema.json` declared `"type": "number"` so VSCode's inline JSON Schema validation accepted `0` / `-1` / `1.5` and the user only learned the value was invalid on the next daemon restart. Added `jsonSchemaOverride: {type:'integer', minimum:1}` to the `consensusQuorum` settings entry and regenerated the schema. IDE editors now flag invalid values immediately. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * fix(f3): Round 6 — wenshao APPROVED + DeepSeek follow-ups (#4335) Mixed batch: bridge-test backfill from wenshao's APPROVED review plus 4 DeepSeek/v4-pro suggestions and the 3 typecheck/test blockers DeepSeek named in CHANGES_REQUESTED #4325674833. **Pre-merge blockers (DeepSeek #4325674833 body)** - `server.test.ts:529` `FakeBridge` — added the F3-required `permissionPolicy: 'first-responder' as const`. Tests don't exercise mediation; the literal pins the pre-F3 default so existing assertions stay shape-compatible. - `server.test.ts:3994` `WorkspaceFileSystemFactory.forRequest()` mock — added the missing `writeTextOverwrite` method that PR #4334 introduced on `WorkspaceFileSystem` after this branch forked. - 4 vote-context test failures from `fromLoopback` plumbing — updated the four `expect(...).toEqual(...)` assertions in `POST /session/:id/permission/:requestId` and `POST /permission/:requestId` to include `fromLoopback: true` on the captured context. The supertest peer is `127.0.0.1`, so `detectFromLoopback(req)` correctly stamps the field; the pre-F3 expected shape was stale. **Inline suggestions adopted** - **3271420267** (wenshao APPROVED, security-critical) — added bridge-level test `rejects cancel sentinel injection via {selected,'__cancelled__'}` in `httpAcpBridge.test.ts`. Without it, a future refactor could silently remove the wire-injection guard that closes the policy-bypass attack surface introduced in Round 5 (#3271185588). Required `npm run build --workspace=packages/acp-bridge` to refresh `dist/` before vitest picked up the F3 bridge.ts changes; documented for future contributors editing F3 acp-bridge code. - **3271627444** (DeepSeek) — `request()` JSDoc rewritten to drop "Promise contract — never rejects" without qualification. The `CancelSentinelCollisionError` synchronous throw is real and intentional (a never-settling Promise alongside a thrown error is worse than fail-fast), but callers must be aware of it. Updated the contract doc to call out the sync-throw exception explicitly and documented that async callers get the throw via their own Promise machinery. - **3271627446** (DeepSeek) — fixed "Bounded LRU" comment on `MAX_RESOLVED_PERMISSION_RECORDS` to "Bounded FIFO" since `rememberResolved` uses `resolvedOrder.shift()` (drop oldest). Mirrors the parallel `PermissionAuditRing` correction in commit b0242ddec. - **3271627457** (DeepSeek) — added stderr breadcrumbs to all 3 forbidden-vote sites (voteDesignated / voteConsensus / voteLocalOnly). Audit ring is in-memory only (no v1 query route), SSE events are transient — operators tailing daemon stderr previously had zero indication of permission rejections. New `writeForbiddenStderr` helper centralizes the formatting + try/catch defensive posture (mirrors the timeout breadcrumb pattern from Round 4). - **3271627459** (DeepSeek) — added a `TODO(forward-compat)` comment at `voteConsensus`'s rejection site documenting the `designated_mismatch` reason-code overload. The same wire string covers two distinct semantic cases: "voter is not the prompt originator" (designated policy) and "voter not in consensus votersAtIssue snapshot" (consensus). Splitting them into distinct codes is deferred to a future PR once an SDK consumer needs to disambiguate. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * fix(f3): Round 7 — error precedence + 7 hardening fixes from wenshao (#4335) 8 findings from wenshao Round 7. The Critical one closes a session- existence information leak; 6 Suggestions improve observability, type safety, and test coverage; 1 documents the cancel-sentinel escape hatch in the local-only setting description. **[Critical] Error precedence regression in respondToSessionPermission** (3271978329). When `peekSessionFor(requestId)` returned `undefined` (timed out / LRU-evicted / never registered), the cross-session guard at line 2033 didn't fire (`!== undefined` skips it), so execution fell through to `resolveTrustedClientId` which throws `InvalidClientIdError` (HTTP 400) when the caller's clientId isn't registered. Pre-F3 returned `false` (HTTP 404) for unknown requestIds regardless of clientId validity. Without the explicit guard, a probe with a fabricated clientId could distinguish "session exists with these registered clients" (400) from "no such request" (404). Added an explicit `actualSessionId === undefined → return false` short-circuit BEFORE the clientId validation. The defensive `unknown_request` switch case below becomes unreachable in practice; left in place for defense-in-depth. **[Suggestion] Cancel sentinel cross-policy escape hatch under `local-only`** (3271978336). Documented in `voteLocalOnly` JSDoc and the settings description that a remote voter can ABORT a pending permission via `{outcome:'cancelled'}` even though they cannot RESOLVE one. The F3 plan calls this out as intentional (cross-policy cancel for consistency with first-responder / designated / consensus); operators wanting strict-cancel-too need a dedicated loopback-bound daemon. Doc-only — semantic change deferred. **[Suggestion] CapabilitiesEnvelope.policy.permission widens silently** (3271978342). Replaced the inlined string-literal union with `import type { PermissionPolicy } from '@qwen-code/acp-bridge'`. Adding a 5th policy upstream would now trigger a compile error here instead of silently accepting the narrower set. **[Suggestion] M=2 unanimity surprise** (3271978356). Default quorum `floor(M/2)+1` requires unanimity for even M (M=2 → quorum=2; both voters must agree). An operator picking `consensus` with two clients expecting "majority of 2 = 1" gets unanimity instead — a split vote silently hangs until `permissionTimeoutMs`. Added stderr breadcrumb at issue time when the default formula yields unanimity (M ≥ 2 and floor(M/2)+1 == M). Mirrors the existing M=0 / cap-applied breadcrumbs added in Round 5. Formula stays unchanged (true majority for all M is mutually exclusive with M=1 → quorum=1). Description in the settings schema also calls out the M=2 case explicitly. **[Suggestion] Cancel sentinel adversarial test gap** (3271978359). The existing "resolves cancelled regardless of policy" test used the originator under designated and a votersAtIssue voter under consensus — those would be ACCEPTED by the policies even without the sentinel bypass. Added two adversarial tests that pin the cross-policy escape hatch: non-originator voter under designated and not-in-snapshot voter under consensus. **[Suggestion] BridgeClient pre-publish collision test gap** (3271978365). `bridgeClient.requestPermission` throws `CancelSentinelCollisionError` BEFORE publishing the SSE `permission_request` to prevent orphan events (the mediator-level collision check in `mediator.request` happens too late if publish goes first). Added test asserting the throw + asserting publish was NOT called + asserting `pendingPermissionIds` was NOT incremented. **[Suggestion] Settings descriptions missing security caveats** (3271978370). Added explicit caveats to `permissionStrategy` description: (a) `designated` notes that client identity is self-declared with no proof-of-possession (impersonation by observing originatorClientId on SSE frames is possible); (b) `local-only` notes the cancel-sentinel cross-policy escape hatch. Schema regenerated to `vscode-ide-companion/schemas/settings.schema.json`. **[Suggestion] Boot validation error class** (3271978374). Replaced `err.message.includes('invalid policy.')` substring matching with a dedicated `InvalidPolicyConfigError` class checked via `instanceof`. A future reworded validation message would have silently downgraded operator misconfiguration to "fall back to defaults" under the previous fragile match. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * fix(f3): Round 8 — close legacy clientId oracle + 5 hardening fixes (#4335) 6 follow-up findings from wenshao Round 8 review #4326742064 (state: COMMENTED — not blocking but addresses leftover risk surfaces). **[Suggestion] Legacy `respondToPermission` info leak** (3272493777). Round 7 closed the cross-session client-registration oracle on the session-scoped vote route, but the legacy workspace-level route (`POST /permission/`) still called `resolveAnyTrustedClientId` on unknown-requestId paths, throwing `InvalidClientIdError` (400) for unregistered clientIds and returning false (404) for registered ones — the same oracle. The PR #4231 reasoning ("preserve security boundary") was inverted: the 400-vs-404 distinction WAS the leak. Removed the call, deleted the now-unused `resolveAnyTrustedClientId` helper, and updated the previously-leak-asserting test (`rejects unknown permission votes with unregistered client ids`) to assert the new uniform `false` behavior across all 3 input shapes (unregistered / registered / no-clientId). **[Suggestion] Error-precedence regression test gap + observability inconsistency** (3272493792). Two parts: - Added regression test `returns false (not InvalidClientIdError) when session exists but requestId is unknown and clientId is unregistered` to lock the Round-7 fix against future refactors. - Promoted the error-precedence guard's stderr line from debug-gated `writeServeDebugLine` to unconditional `writeStderrLine`, matching the `writeForbiddenStderr` posture in the mediator. Operators tailing stderr at 3 AM no longer need `QWEN_SERVE_DEBUG=1` to see unexpected 404s on the permission endpoint. **[Suggestion] Settings description "UNANIMITY for even M" was factually wrong** (3272493795). `floor(M/2)+1` equals M only when M=2; for M=4 it gives 3 (supermajority), M=6 gives 4 (~67%). The mediator's own unanimity warning correctly fires only when M=2. Settings description now reads "UNANIMITY for M=2 (quorum=2, both must agree) and supermajority for larger even M (M=4 → quorum=3; M=6 → quorum=4)". VSCode JSON schema regenerated. **[Suggestion] runQwenServe.ts inline policy unions** (3272493805). Same drift-protection rationale as the types.ts fix in Round 7. Imported `PermissionPolicy` from `@qwen-code/acp-bridge`, replaced 3 inline unions: the `let` declaration, the `as` cast, and the `VALID_PERMISSION_POLICIES` Set construction. Used a typed-array + Set pattern (drift caught at array construction; runtime Set keeps `.has(string)` ergonomics). **[Suggestion] InvalidPolicyConfigError discrimination needs positive tests** (3272493818). Extracted the inline `policyConfig`-validation logic into an exported `validatePolicyConfig(policyConfig, onWarning?)` helper and exported `InvalidPolicyConfigError` itself. Added 7 unit tests covering: empty config, all 4 valid literals, invalid literal throws (with class identity check + message regex), 4 non-positive-integer quorum cases throw, valid combination returns, mismatch (consensusQuorum + non-consensus strategy) emits warning without throwing, no-warning happy path, and error messages name the failed field. The boot path in `runQwenServe` now delegates to the helper (one call site, DRY). **[Suggestion] Unanimity breadcrumb spammed per-request** (3272493829). The Round-7 unanimity stderr line fires inside the synchronous Promise executor of every `request()` call, which for a 2-client consensus session is EVERY permission request (M=2 unanimity is the normal operating mode, not a rare edge). Added `unanimityBreadcrumbEmitted` boolean to the mediator class (per-mediator dedup, parallel to `consensusQuorumCapNoted` on `MediatorPending`). One emit per daemon lifetime — visible at boot, silent thereafter. Comment also corrects the "for even M" generalization to "for M=2" specifically, matching the actual condition (`floor(M/2)+1 === M` only for M=1 and M=2). 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * fix(f3): Round 9 — terminal-event forbidden cleanup + 7 hardening fixes (#4335) 8 follow-up findings from wenshao Round 9 (4 separate review records: 4326832742 / 4326833568 / 4326844430 / 4326851074, the last one a non-blocking comment review). 1 Critical + 7 Suggestions. **[Critical] Terminal events leaked forbiddenVotes history** (3272576003). `session_died` / `session_closed` / `client_evicted` / `stream_error` reducer cases cleared `pendingPermissions` and `permissionVoteProgress` but not `forbiddenVotes` / `forbiddenVoteCount`. Adapters reading view state for a dead session would render stale rejection data. All 4 cases now zero out the rejection ring + counter. Parameterized regression test asserts the cleanup contract. **[Suggestion] safeAudit JSDoc was orphaned over writeForbiddenStderr** (3272567323). Two consecutive JSDoc blocks were stacked back-to-back but the method definitions followed in the opposite order, so IDE hover and API doc generation showed `safeAudit`'s docs as `writeForbiddenStderr`'s. Reordered method definitions so each JSDoc precedes its actual method. **[Suggestion] writeForbiddenStderr had no test coverage** (3272568031). Added a 3-path test (designated / consensus / local-only) that spies on `process.stderr.write` and asserts each breadcrumb contains the expected reason fragment plus the requestId + sessionId for grep-ability. Pins the format so a future refactor can't silently drop the line. **[Suggestion] resolveEntry numbered list contradicted code** (3272581553). The N2-invariant cleanup ladder docstring bundled "delete from pending + write to resolved" into step 2 ahead of the SSE emit, but the actual code defers `rememberResolved` until AFTER `safeEmit` (the I5 inline comment on line 1103 correctly explains this). Split step 2 into two halves around the emit so the spec faithfully describes the ordering invariant. **[Suggestion] Dead exports in bridgeClient.ts** (3272581548). `MAX_RESOLVED_PERMISSION_RECORDS`, `PendingPermission`, and `PermissionResolutionRecord` were defined and exported but no longer referenced — the mediator owns the same state under different names (`permissionMediator.ts:77` / `:319`). The JSDoc still pointed at deleted closures (`registerPending`, `resolvedPermissions` map). Removed all three definitions and the matching re-exports in `cli/src/serve/httpAcpBridge.ts`. **[Suggestion] detectFromLoopback prefix-match had no direct test** (3272581557). Supertest in the broader server.test.ts suite always connects from `127.0.0.1`, so the Round-5 prefix-match fix for `127.x`-beyond-`.0.0.1`, `::1`, `::ffff:127.*`, and the fail-closed branches had no coverage. Exported the helper from `server.ts` (loosened parameter type to a minimal shape so tests don't need to spin up Express) and added an `it.each` table covering the variants the fix targets, plus an explicit "does NOT consult X-Forwarded-For" assertion as a security pin. **[Suggestion] Validate-policies set is a 4th hardcoded copy** (3272581563). The policy literals already exist in 3 places — `PermissionPolicy` type, `SERVE_CAPABILITY_REGISTRY.permission_ mediation.modes`, and `settingsSchema.ts` enum options. `validatePolicyConfig` now derives its valid-set from `SERVE_CAPABILITY_REGISTRY.permission_mediation.modes` (single runtime source of truth). Adding a 5th policy upstream lands in one place; a future drift between the registry and the type union would still surface at the `as PermissionPolicy` cast. **[Suggestion] BridgeClient over-coupled to MultiClientPermissionMediator** (3272581569). `BridgeClient` only ever calls `mediator.request()` but its field was typed as the concrete class, forcing every test stub to fake all 6 mediator members. Narrowed the field type to `Pick` (the frozen interface from `permission.ts`); the bridge factory still passes the full `MultiClientPermissionMediator` instance via structural typing. Test stubs simplified from 6 placeholder members to 1. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * fix(f3): Round 10 — wenshao APPROVED + 3 final polish (#4335) wenshao APPROVED the PR (review 4327485978: "No issues found in the latest Round 9 changes... LGTM ✅") with 3 minor follow-up suggestions in a separate COMMENTED review (4327443147). All adopted; the 4th suggestion (3273077262) was already addressed in Round 9. **[Suggestion] Symmetric stderr breadcrumb on legacy respondToPermission** (3273077256). The session-scoped sibling already writes an unconditional `writeStderrLine` on its `actualSessionId === undefined` rejection path (Round 8 / 3272493792); the legacy `POST /permission/` route returned `false` silently after the Round-8 oracle removal, leaving an observability gap. Added matching `writeStderrLine`. Operators tailing stderr at 3 AM now see legacy-route 404s without needing QWEN_SERVE_DEBUG=1. **[Suggestion] consensusQuorum contract mismatch** (3273077270). The warning text told the operator "the override will be ignored" but the function still propagated `permissionConsensusQuorum` to BridgeOptions. The downstream mediator only reads it under the consensus policy, so behavior was correct — but the public contract contradicted itself. Adopt option (a): drop the value to `undefined` when the strategy is not 'consensus' so the returned struct matches what the warning promises. Updated the existing `validatePolicyConfig` test to assert the new contract. **[Suggestion] Stderr-breadcrumb assertion missing from error-precedence regression test** (3273077272). The Round-8 test pinned the return-value behavior (`false`) but not the unconditional-stderr promotion that was the primary behavioral change of that hunk. Added `vi.spyOn(process.stderr, 'write')` + assertions for both "rejected permission vote" and the literal requestId in the test. A future refactor that drops or downgrades the log line is now caught. **[Suggestion] _validPolicies underscore-prefix misleading** (3273077262 — already addressed). Round 9's commit 6793b89b9 replaced the literal `_validPolicies` array with a single Set derived from `SERVE_CAPABILITY_REGISTRY.permission_mediation.modes` (per separate suggestion 3272581563). The underscore-prefixed identifier is gone in current HEAD; replied via PR comment pointing wenshao at the existing fix. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * feat(serve+sdk): F4 prereq — daemon protocol completion (serverTimestamp / provenance / errorKind / state_resync_required) (#4360) * feat(serve): stamp serverTimestamp / tool provenance / errorKind on daemon events (#4175 F4 prereq) Adopts chiga0's three P0 SDK-side blockers from #4175 comment #19 — the SDK side already consumes these fields (PR #4353), but daemon hadn't stamped them yet, leaving the corresponding UI affordances inert. All three stampings are purely additive on the wire and don't require any SDK type changes (SDK already has forward-compat field slots). **#19.1 — `_meta.serverTimestamp` on every SSE frame** (`server.ts` `formatSseFrame()`) Stamped at the SSE write boundary (NOT EventBus.publish) so the in-memory `BridgeEvent` type stays unchanged and internal consumers don't see `_meta`. Pre-existing `_meta` keys (e.g. tool_call's `_meta.toolName`) are preserved via spread merge. SDK reads via the 3-location probe in `extractServerTimestamp` (chiga0's PR #4353); we pick `_meta.serverTimestamp` (Anthropic convention) so top-level event type stays unpolluted. Why this matters: pre-fix, multi-client UIs showing "X minutes ago" or sorting transcript blocks by emit time used each client's local clock — drifts of tens of seconds to minutes across browsers/tabs/ mobile produced visibly inconsistent timestamps. **#19.2 — `tool_call` `provenance` + `serverId` on every emitter event** (`ToolCallEmitter.ts`) New static `ToolCallEmitter.resolveToolProvenance(toolName, subagentMeta)` returns `{ provenance: 'builtin' | 'mcp' | 'subagent'; serverId? }`. Resolution rules (per user-confirmed design decision from issue comment): subagent takes precedence (set when subagentMeta is present); `mcp____` naming heuristic classifies MCP tools with serverId; everything else is builtin. Stamped on `emitStart` AND `emitResult` AND `emitError` (all three emit paths) so a reconnecting client receiving a `tool_call_update` frame from the replay ring (without the original `tool_call` start event) can still derive the provenance. Provenance is stable per tool, so stamping on every event is redundant — but the marginal serialization cost is tiny and reconnect correctness wins. Chose the naming heuristic (not ToolRegistry lookup) per user confirmation: matches the SDK's own fallback (chiga0 PR #4353), no new ctx-dep on emit hot path, no signature changes. **#19.3 — `errorKind` on `stream_error`** (`server.ts` line ~1955) Stamped via `mapDomainErrorToErrorKind(err)` — the 7-value classifier already lives in `@qwen-code/acp-bridge/status.ts` since #4319. When the classifier returns `undefined` (generic Error etc.) the field is omitted — strictly additive. SDK consumers handle "errorKind absent" as before (fall back to rendering `error` text). NOT stamped on `session_died` because the 3 emit sites in `acp-bridge/ bridge.ts` don't have a classifiable `err` in scope: - `channel_closed` carries only exitCode/signalCode (no error) - `killed` is user-initiated (no domain error) - `daemon_shutdown` is operator-initiated (no domain error) A follow-up could thread channel-spawn errors through to the session_died emit site to enable `errorKind: 'init_timeout'` / `missing_binary` classification — left for a separate PR to avoid mixing protocol stamping with lifecycle plumbing. Verification - `npx vitest run packages/cli/src/serve/server.test.ts -t "serverTimestamp|stream_error|errorKind"` — 5 pass - `npx vitest run packages/cli/src/acp-integration/session/emitters/ToolCallEmitter.test.ts` — 46 pass (+ 11 new tests for resolveToolProvenance + provenance stamping on all 3 emit paths) - `npx vitest run packages/cli/src/acp-integration/session/HistoryReplayer.test.ts` — 17 pass - TypeScript clean on touched regions; pre-existing F3 (#4335 merge) errors elsewhere are unrelated. Existing test updates - 15 `_meta: { toolName: 'X' }` assertions in ToolCallEmitter.test.ts updated to include `provenance: 'builtin'` (defensive — catches accidental drift if a future refactor stops stamping). 2 strict-equality assertions in HistoryReplayer.test.ts similarly updated. The first SSE-frame test in server.test.ts switched from `toEqual` to `toMatchObject` since `_meta.serverTimestamp` makes exact equality brittle; a dedicated test pins the new field's shape. * feat(serve+sdk): detect SSE ring eviction on resume, expose state_resync_required (#4175 F4 prereq) Closes the multi-client SSE reducer divergence bug Ilya0527 raised in #4175 comment #15. Pre-fix scenario: 1. Consumer's SSE stream drops; client buffers `Last-Event-ID: N`. 2. Network reconnects long enough later that events `[N+1, ringHead-1]` were evicted from the daemon's per-session ring. 3. Daemon's `subscribe({lastEventId: N})` silently replays only the surviving suffix. 4. Consumer's SDK reducer keeps applying deltas as if the stream was contiguous. Its state has now drifted from the daemon's truth — no terminal signal, no warning. The `SessionState` reducer's "same event stream in → same state out" purity guarantee is broken. The bug's blast radius is exactly when multi-client matters: F4 brings up the TUI / IDE / web client adapters that share session state, so divergence becomes visibly inconsistent across clients. **Daemon side** (`packages/acp-bridge/src/eventBus.ts`) In `subscribe()`'s replay path, detect ring eviction by comparing the ring's earliest id against `lastEventId + 1`. When a gap exists, force-push a synthetic terminal `state_resync_required` frame BEFORE the surviving replay events: ``` { v: 1, type: 'state_resync_required', data: { reason: 'ring_evicted', lastDeliveredId: N, earliestAvailableId: M } } ``` Per user-confirmed design (issue comment discussion): the frame has NO `id` (mirrors the `client_evicted` synthetic terminal pattern so it doesn't burn a slot in the per-session monotonic sequence). Replay continues after the resync frame — the SDK reducer auto-skips subsequent deltas (see below) but the frames stay on the wire so adapters have the option to compute a "what you missed" diff later. **SDK side** (`packages/sdk-typescript/src/daemon/events.ts`) Adds: - `'state_resync_required'` to `DAEMON_EVENT_TYPES` union - `DaemonStateResyncRequiredData` + `DaemonStateResyncRequiredEvent` - `isStateResyncRequiredData` predicate - `DaemonStreamLifecycleEvent` union widened - Reducer state fields: `awaitingResync: boolean`, `resyncRequiredCount: number`, `lastResyncRequired?` - Reducer case for `state_resync_required` — sets the flag, increments count, records data - **Top-of-reducer gate**: when `awaitingResync === true`, all non- terminal events are auto-skipped (still advance `lastEventId`). Terminal lifecycle events (`session_died` / `session_closed` / `client_evicted` / `stream_error`) STILL apply — critical end-of- stream signals don't depend on prior state being current. - Re-exported `DaemonStateResyncRequiredData` / Event from `daemon/index.ts` and `src/index.ts` (matches surface posture of sibling lifecycle types). Consumer recovery contract: when `state.awaitingResync === true`, call `loadSession` (out of band) to fetch the daemon's canonical session snapshot, then reconstruct view state via `createDaemonSessionViewState({...seed from loaded state})`. The fresh state defaults `awaitingResync: false` so the seed implicitly clears the flag. **Side fix** (`stream_error` errorKind) `DaemonStreamErrorData.errorKind?: string` typed for the optional classification field that Commit 1 (`14637cd79`) added daemon-side. Strictly additive — old daemons omit the field, SDK falls back to rendering `error` text. Verification - `packages/acp-bridge`: 6 files, 108/108 pass (+5 new resync-detection tests; 1 existing "default ring size 8000" test updated to acknowledge the synthetic resync frame at the head of its replay batch). - `packages/sdk-typescript`: 13 files, 451/451 pass (+8 new reducer resync tests covering set/skip/terminal-passthrough/recovery/ repeated-resync/malformed-payload). - TypeScript clean across both packages on touched regions. * fix(acp-bridge): preserve FsError structure over ACP wire (#4360 Codex round 2 fold-in) Adopts Codex review round 2 P2 finding on PR #4360 — fold-in to the F4 prereq scope per user's "a" decision. **Problem**: When the `BridgeFileSystem` adapter (introduced in #4334 fs adapter wiring) throws a structured `FsError` (e.g. `kind: 'untrusted_workspace'` / `kind: 'symlink_escape'` / `kind: 'file_too_large'`), the `@agentclientprotocol/sdk` default RPC error serialization only sends `error.message` as JSON-RPC -32603 "Internal error". The structured `kind` / `status` / `hint` fields on FsError are stripped on the way to the agent. Downstream impact: SDK consumers receiving the ACP error payload lose the typed discriminator and have to regex-match the human- readable message to dispatch UI (auth retry vs file picker vs proxy hint). This silently regresses what the FsError-typed contract was supposed to provide. **Fix**: At the bridge boundary (`BridgeClient.writeTextFile` and `BridgeClient.readTextFile`), catch errors from `this.fileSystem. writeText/readText` calls. Duck-type check for FsError shape (`err.name === 'FsError'` + `typeof err.kind === 'string'`); when matched, rethrow as ACP `RequestError(-32603, message, {errorKind, hint, status})`. The agent's RPC client now receives `data. errorKind` and can branch on the closed-enum kind. Cross-package note: FsError lives in `cli/src/serve/fs/errors.ts` and acp-bridge can't `import { FsError }` from cli (dependency inversion). Same duck-typing pattern that `mapDomainErrorToErrorKind` (status.ts) already applies to `TrustGateError` / `SkillError` for the same cross-package bundling reason — `instanceof` would fail across package boundaries when bundlers don't dedupe. **Code shape** ```typescript function isFsErrorShape(err: unknown): err is FsErrorShape { return ( err instanceof Error && err.name === 'FsError' && typeof (err as { kind?: unknown }).kind === 'string' ); } function preserveFsErrorOverAcp(err: unknown): never { if (isFsErrorShape(err)) { throw new RequestError(-32603, err.message, { errorKind: err.kind, ...(err.hint !== undefined ? { hint: err.hint } : {}), ...(err.status !== undefined ? { status: err.status } : {}), }); } throw err; } ``` Applied at both `if (this.fileSystem) { ... }` blocks (writeTextFile + readTextFile) — wrapped the adapter call in try/catch + `preserveFsErrorOverAcp(err)`. Non-FsError errors are rethrown unchanged (default ACP serialization is fine for unstructured errors; only the structured shape needs preservation). JSON-RPC code stays at -32603 (internal error) rather than mapping FsError.kind → JSON-RPC code. Rationale: the JSON-RPC standard defines only a handful of code values (-32700/-32600/-32601/-32602/ -32603 + a reserved range for application errors), and mapping ~10 FsError kinds to that narrow space is lossy. Instead the structured `data.errorKind` carries the semantic information SDK consumers need; JSON-RPC code remains the generic "an error happened" signal. **Tests** (+5 in `bridgeClient.test.ts`) - writeTextFile FsError → ACP RequestError with errorKind in data - readTextFile FsError preserving symlink_escape kind (no hint field present → not stamped, spread guard works) - non-FsError pass-through (plain Error stays plain Error, no RequestError wrap) - hint field preservation when present - defensive: error with `kind` field but wrong `name` does NOT get wrapped (e.g. PermissionForbiddenError happens to have a kind field internally — must NOT be confused for FsError) Verification: 113/113 acp-bridge tests pass (+5 new FsError- preservation tests). Full serve suite shows pre-existing F3-related failures unrelated to this change (verified in isolation). * fix: 7 wenshao/copilot review fold-ins on #4360 (1 Critical + 6 Suggestion) Adopts all 7 review threads from the first wenshao + Copilot review round on PR #4360. All technical fixes (no judgment calls). **[Critical] BridgeTimeoutError constructor blocks tsc** (wenshao PRRT_kwDOPB-92c6DfcRI) `server.test.ts:4670` called `new BridgeTimeoutError('initialize timed out')` but the constructor signature is `(label: string, timeoutMs: number)` — TS2554 blocked `tsc --noEmit` and `npm run build`. Fixed to `new BridgeTimeoutError('initialize', 5000)` per suggested fix; resulting message `"HttpAcpBridge initialize timed out after 5000ms"` still satisfies the existing `.toContain('timed out')` assertion. **[Suggestion] Copilot JSDoc package name** (Copilot PRRT_kwDOPB-92c6De-Sm, ToolCallEmitter.ts:210) JSDoc referenced `@qwen-code/core/mcp-tool` but the actual package is `@qwen-code/qwen-code-core` with the file at `packages/core/src/tools/mcp-tool.ts`. Updated the reference. **[Suggestion] Copilot errorKind type widening** (Copilot PRRT_kwDOPB-92c6De-Ro, events.ts:244) `DaemonStreamErrorData.errorKind` was typed as `string` and the JSDoc said "7-value" closed enum — but `DAEMON_ERROR_KINDS` actually has 8 values, and `SERVE_ERROR_KINDS` (daemon-side) has 9 (adds `stat_failed`). Typed as `DaemonErrorKind | (string & {})` for forward-compat: SDK consumers get IDE autocomplete on the known 8 kinds while still accepting future daemon-side additions (like `stat_failed`) without a type error. Updated JSDoc to accurately list 8 current values + call out the forward-compat widening. Side observation (NOT in scope of this PR): `DAEMON_ERROR_KINDS` (SDK) lacks `stat_failed` that exists in `SERVE_ERROR_KINDS` (daemon). That's a separate drift fix. **[Suggestion] TERMINAL wording misleading** (wenshao PRRT_kwDOPB-92c6Dj-JL, eventBus.ts:369) Comment called `state_resync_required` a "TERMINAL synthetic frame" but it's emitted FIRST (before replay) and the stream stays OPEN. Genuine terminals like `client_evicted` close the stream after the frame. Rewrote the comment per suggestion: "id-less synthetic frame... Unlike `client_evicted`, the stream stays OPEN" — so an oncall reading the source at 3am gets the right mental model. **[Suggestion] `_meta` merge dead code + stale reference** (wenshao PRRT_kwDOPB-92c6Dj-JF, server.ts:2569) The `existingMeta` merge reads `event._meta` at BridgeEvent top level, but ToolCallEmitter's `_meta` lives nested inside `event.data._meta` (publish path goes through `events.publish({type: 'session_update', data: params})`). In production `existingMeta` is always undefined — the merge is a forward-compat escape hatch, not an active merge. Also the comment referenced `extractServerTimestamp` (sdk-typescript) which grep confirms doesn't exist yet (it's planned in chiga0 PR #4353). Rewrote the comment block to (1) acknowledge no current producer sets `_meta` at the top level — it's a forward-compat hook for future envelope-level metadata; (2) drop the stale `extractServerTimestamp` reference and instead note that chiga0 PR #4353 plans the 3-location probe. Code shape unchanged (forward-compat spread stays). **[Suggestion] session_closed + client_evicted passthrough tests** (wenshao PRRT_kwDOPB-92c6Dj-JW, daemonEvents.test.ts:2284) `RESYNC_PASSTHROUGH_TYPES` has 5 members but only `session_died` and `stream_error` had passthrough tests. Added two missing tests: `session_closed` and `client_evicted` while awaitingResync. Critical because if a future refactor accidentally drops either from the set, a consumer in resync limbo would silently swallow the terminal signal and the UI would hang on "loading resync state…". **[Suggestion] readTextFile non-FsError passthrough test** (wenshao PRRT_kwDOPB-92c6Dj-JX, bridgeClient.test.ts:251) The non-FsError pass-through test only covered `writeTextFile`. Added a symmetric `readTextFile` test — the two `try/catch` blocks in `bridgeClient.ts` are independent, so test parity guards against divergent refactors (e.g. someone adding wrapping on one side but not the other). Verification - `packages/acp-bridge`: 6 files, 114/114 pass (+1 new readTextFile non-FsError test). - `packages/sdk-typescript`: 75/75 pass on daemonEvents.test.ts (+2 new session_closed / client_evicted passthrough tests). - `packages/cli/src/serve/server.test.ts`: 248 tests pass on touched cases (5 SSE / serverTimestamp / stream_error tests). Pre-existing F3 (#4335 merge) test failures unrelated to this PR's changes — verified by stash-test-restore on clean tree. - TypeScript clean on touched regions; `BridgeTimeoutError` 2-arg fix unblocks `tsc --noEmit` for the test file. * fix: 3 wenshao observability fold-ins on #4360 (all Suggestion) Adopts all 3 threads from wenshao's second review round on PR #4360. All Suggestion-level — daemon-side observability + 1 missing SDK reducer test. **[Suggestion] SSE ring eviction silently emits state_resync_required** (PRRT_kwDOPB-92c6Dp_Uk, eventBus.ts:394) Pre-fix: when a consumer reconnects past the ring boundary, the daemon emits `state_resync_required` with zero stderr breadcrumb. A 3am oncall chasing "my UI is frozen with stale state" couldn't grep daemon logs to distinguish (a) ring undersized, (b) client reconnecting too slowly, (c) network partition causing repeated reconnects. Fix: detect `next.value.type === 'state_resync_required'` in the SSE handler's iter loop in `server.ts` and emit a `writeStderrLine` with the gap details (`lastEventId`, `earliestInRing`, computed `gap` count, `reason`). Logged at the route boundary rather than inside `EventBus.subscribe` to keep the bus implementation pure + concentrate daemon-side observability in the route handler that already logs socket errors + heartbeats. **[Suggestion] Bridge iterator throw forwarded to client but not logged daemon-side** (PRRT_kwDOPB-92c6Dp_Uo, server.ts:1956) Pre-fix inconsistency: the adjacent `res.on('error', ...)` handler at line ~1925 logs SSE socket errors with `writeStderrLine`, but the bridge-iterator-catch block at line ~1940-1965 sends a `stream_error` SSE frame to the client AND swallows the error daemon-side. When the bridge iterator throws (subprocess crash, channel protocol error, unhandled rejection), distinguishing "subprocess OOM-killed" from "protocol bug" required attaching a debugger. Fix: mirror the adjacent handler's pattern — add `writeStderrLine` before the `stream_error` SSE frame send, including the classified `errorKind` (when available) in brackets so operators can grep for `[init_timeout]` / `[missing_binary]` etc. **[Suggestion] No SDK reducer test verifying stream_error.errorKind flowthrough** (PRRT_kwDOPB-92c6Dp_Uq, daemonEvents.test.ts:2331) The daemon-side wire format is tested in `server.test.ts` (`parsed.data.errorKind === 'init_timeout'`) and `DaemonStreamErrorData` now declares `errorKind?`, but the SDK reducer test suite never fed a `stream_error` event with `errorKind` and asserted `state.streamError?.errorKind`. A future refactor stripping `errorKind` from the reducer's data assignment (e.g. spreading only `{error}`) would silently regress without test signal. Fix: added `captures errorKind on stream_error in view state` test exercising the full pipeline — reducer receives stream_error with errorKind, view state's `streamError.errorKind` matches. Verification - `packages/sdk-typescript`: 76/76 daemonEvents tests pass (+1 new flowthrough test). - `packages/cli/src/serve/server.test.ts`: 6 targeted serverTimestamp / stream_error / errorKind tests pass — server.ts changes are observability-only (no behavior change to wire format). - Pre-existing F3 (#4335 merge) test failures elsewhere are unrelated to this PR's changes. * test(serve): 2 wenshao observability fold-ins on #4360 (stderr log coverage) Adopts both threads from wenshao's third review round on PR #4360. Both Suggestion-level — pin the daemon-side stderr log artifacts that commit `dce2fed0f` introduced. Pre-fix: the EventBus-level state_resync_required emission was tested in eventBus.test.ts, and the SSE wire shape was tested in server.test.ts, but the actual operator-facing artifacts (the stderr log lines themselves) had no test coverage. A regression swapping operands in the `gap` arithmetic, dropping the sessionId from the log, or breaking the `[errorKind]` suffix would ship silently and only surface when an operator went grepping during an incident. **[Suggestion] SSE ring eviction stderr log untested** (PRRT_kwDOPB-92c6Dqtlb, server.ts:1948) Added 2 tests: - `writes a daemon-side stderr log on SSE ring eviction` — yields a `state_resync_required` frame from a fake bridge, spies on `process.stderr.write`, asserts the captured log contains `session sess-A` + `lastEventId=5` + `earliestInRing=12` + `gap=6 events` (pins the arithmetic) + `reason=ring_evicted` + `loadSession` (the recovery hint). - `falls back to "?" placeholders when state_resync_required data is partial` — yields a frame with empty `data: {}`, asserts every `?? '?'` branch fires (lastEventId=? / earliestInRing=? / gap=? events / reason=?). Defensive against future daemon schema changes that drop one of these fields. **[Suggestion] Bridge iterator error stderr log untested** (PRRT_kwDOPB-92c6Dqtlh, server.ts:1993) Added 2 tests: - `writes a daemon-side stderr log on bridge iterator error` — fake bridge throws plain `Error('agent died')` mid-stream, captures stderr, asserts the log contains `session sess-A` + `agent died`, and **no** `[…]` suffix (plain Error → `mapDomainErrorToErrorKind` returns undefined → no suffix). - `includes [errorKind] suffix in bridge iterator error log when classified` — fake bridge throws `BridgeTimeoutError('initialize', 5000)`, asserts the log contains `[init_timeout]`. Pins the classified-vs-unclassified branch of the conditional suffix template. All 4 tests use `vi.spyOn(process.stderr, 'write').mockReturnValue( true)` + filter `mock.calls` for the relevant log prefix — same pattern as the existing `mcp-client-manager.test.ts` stderr-spy tests in core, plus `startupProfiler.test.ts` in cli. Verification: 7/7 targeted observability tests pass. Pre-existing F3 (#4335 merge) test failures elsewhere are unrelated to this PR's changes. * feat(serve): shared MCP transport pool [F2] (#4336) * docs(serve): F2 MCP transport pool design (v2.1) Design document for F2 shared MCP transport pool — workspace-scoped pool that replaces today's per-session McpClient spawning so N sessions in one workspace share one process per unique server config. v2.1 folds in 12 review corrections on top of v2: - single-PR delivery per #4175 branching strategy (commit-by-commit review) - sessionToEntries reverse index for O(refs) releaseSession - ?entryIndex= selective restart route - spawn-failure slot leak fix - in-flight tool call during reconnect semantics (MCPCallInterruptedError) - /mcp disable triggers SessionMcpView re-apply - entryIndex exposure instead of raw fingerprint (avoid token-rotation side-channel) - reconnect backoff spec (stdio 5s x3, HTTP exponential 1/2/4/8/16s x5) - canonicalOAuth normalization - legacyInProcessAcquire renamed to createUnpooledConnection - drainAll(opts?) signature with timeoutMs - locked SDK reducer field names (no public API rename) - extension uninstall orphan entries deferred to MAX_IDLE_MS natural reap Refs: #3803, #4175 F2 Generated with Qwen Code * docs(serve): fix V21-10 changelog row wording Replace-all regression from prior commit: both sides of the rename arrow ended up as createUnpooledConnection. Restore the meaning (old name was descriptive, not a literal symbol). Generated with Qwen Code * refactor(core): split McpClient.discover into pure tool/prompt list (#4175 F2 commit 1) Foundation for the F2 shared MCP transport pool. Splits the existing side-effecting discovery API into a pure version that returns a {tools, prompts} snapshot, so the upcoming pool (#4175 F2 commit 2) can let a single shared McpClient produce one snapshot and have N per-session SessionMcpView instances each register a filtered copy into their own ToolRegistry / PromptRegistry. Changes: - Extract listMcpPrompts(serverName, mcpClient) — pure version of discoverPrompts that returns DiscoveredMCPPrompt[] (with serverName and bound invoke) WITHOUT touching any PromptRegistry. - Refactor discoverPrompts(name, client, registry) to wrap listMcpPrompts + register; preserves historical Promise return type (strips serverName / invoke from returned plain Prompt objects so existing callsites are unaffected). - Add McpClient.discoverAndReturn(cliConfig) — pure method returning {tools, prompts}. Same error semantics as discover(): flips status to DISCONNECTED on any failure and re-throws; "No prompts or tools found on the server." sentinel preserved so wrapping managers / pools can distinguish "server up but empty" from "server down". - Refactor McpClient.discover(cliConfig) to delegate: calls discoverAndReturn then explicitly registers BOTH tools and prompts into the per-instance registries. Pre-F2 prompts were registered as a side effect inside discoverPrompts; post-F2-1 registration happens in discover() after the pure call returns. Observable side effects identical (both registries populated by end of call); the order flip (tools first, then prompts vs. prompts first as side effect, then tools) has no observable race because discover() is awaited as a unit by connectAndDiscover and the two registries are independent maps. - Remove dead private methods McpClient.discoverTools and McpClient.discoverPrompts that delegated to the exported functions. Tests: - 7 new tests covering discoverAndReturn (snapshot purity, no registration, no-prompts-or-tools rejection with DISCONNECTED status flip, unconnected-state guard) and listMcpPrompts (enriched return type with invoke, no-prompts-capability fallback, protocol error swallow). - 1 new backward-compat test asserting discoverPrompts wrapper still registers prompts AND strips enrichment fields from return value. - 1 forward-defense assertion: the no-prompts-or-tools throw path verifies registries were strictly untouched, catching future regressions in commits 2-6 that might register a partial batch before the guard fires. Backward compatibility: - McpClient.discover() signature and side-effect contract unchanged for all standalone qwen callers + existing tests (44/44 pass). - discoverPrompts() exported signature unchanged. - No new public exports from packages other than listMcpPrompts + McpClient.discoverAndReturn (additive). - All 36 pre-existing tests in mcp-client.test.ts pass; all 71 tests in mcp-client-manager.test.ts pass. - packages/core typecheck clean; lint clean on touched files. Refs: #3803, #4175 F2; design doc docs/design/f2-mcp-transport-pool.md §7 Generated with Qwen Code * feat(core): McpTransportPool + SessionMcpView (#4175 F2 commit 2) Core implementation of the F2 shared MCP transport pool. Workspace- scoped pool that lets N ACP sessions share one MCP client per unique (serverName, fingerprint) tuple instead of each session spawning its own MCP child process. New files: - mcp-pool-events.ts: PoolEvent discriminated union, PoolEntryState enum, MCPCallInterruptedError class (§13.4), type guards. - mcp-pool-key.ts: fingerprint() with sorted canonical form for stable hashing across env-key permutations; canonicalOAuth() collapses {enabled:false}/undefined/null/{} to null (V21-9); mcpTransportOf() classification; isPoolable() opt-in gate; POOLED_TRANSPORTS_DEFAULT = {stdio, websocket} (V21 C8); connectionIdOf / parseConnectionId. - session-mcp-view.ts: per-session, per-server projection of the pool's snapshot into a session's own ToolRegistry + PromptRegistry. passesSessionFilter() preserves pre-F2 include/exclude semantics. applyTools clones each tool via withTrust() so per-session trust never cross-contaminates the shared snapshot (V21 C7). teardown() drops all this view's registrations. - mcp-pool-entry.ts: PoolEntry class with refcount, drain state machine (spawning -> active <-> draining -> closed | failed), generation counter for stale-handler guard (§7.3), snapshot replay on attach (§7.2 / V21 C4), restart() with in-flight coalescing (§13.2), forceShutdown() with idempotency, MAX_IDLE_MS hard cap that survives drain/attach flap. defaultPoolEntryOptions() returns transport-keyed defaults (stdio: 5s fixed x3, http: 1/2/4/8/16s exponential x5 per §6.6). - mcp-transport-pool.ts: top-level McpTransportPool class. - acquire(name, cfg, sid, toolReg, promptReg): pool lookup, spawnInFlight dedup for concurrent acquires, slot reservation released on spawn failure (V21-4), sessionToEntries reverse index for O(refs) releaseSession (V21-2). - release(id, sid) / releaseSession(sid). - restartByName(name, {entryIndex?}): V21-3 selective restart via opaque entryIndex; returns RestartResult[]. - getSnapshot(): includes entryCount + entrySummary (with opaque entryIndex, NOT raw fingerprint per V21-7) for the pool-aware status route in commit 5. - aggregateStatusByName(): "any-CONNECTED wins" across multi-entry name collisions (§8.1). - drainAll({force?, timeoutMs?}): wall-clock bounded graceful shutdown for QwenAgent.close (§17 + V21-11). - createUnpooledConnection(): SDK MCP + HTTP-no-opt-in path constructs a per-session McpClient and uses the legacy discover() (which writes to session registries directly). - poisonedToolRegistry/PromptRegistry: stub passed to pool's own McpClient instances; throws on any registration to catch regressions where a pool path accidentally fell back to side-effecting discover() instead of discoverAndReturn(). Changes: - mcp-tool.ts: added DiscoveredMCPTool.withTrust(trust) clone method (analogue of asFullyQualifiedTool but only updates trust; returns this when trust unchanged to skip allocation in the common case). Tests (40 new): - mcp-pool-key.test.ts (18 tests): fingerprint stability across env permutations, divergence on auth byte changes, exclusion of per-session filters from key, canonicalOAuth collapse, transport classification, isPoolable gate, connectionId round-trip with :: in server names. - session-mcp-view.test.ts (11 tests): filter semantics, trust copy invariant (snapshot tool NOT mutated), allocation pin when trust unchanged, include/exclude precedence, prompt fan-out, updateConfig + re-apply, idempotent teardown. - mcp-transport-pool.test.ts (11 tests): 3-session sharing with 1 spawn, credential isolation via env divergence, drain timer cancellation by re-attach, drain timer expiry, spawnInFlight dedup of 5 concurrent acquires, reverse-index releaseSession, restartByName + entryIndex selectivity, subprocessCount in snapshot, drainAll teardown. No integration with daemon yet (acpAgent / Config / ToolRegistry wiring lands in commit 4). Pool currently constructible in isolation; existing standalone qwen + per-session McpClient path untouched and all 71 mcp-client-manager + 44 mcp-client tests pass unchanged. Refs: #3803, #4175 F2; design doc docs/design/f2-mcp-transport-pool.md §4 architecture, §5 fingerprint, §6 lifecycle, §7 SessionMcpView Generated with Qwen Code * feat(core): cross-platform pid sweep + commit-2 review fixes (#4175 F2 commit 3) Two adjacent concerns in one commit: 1. Cross-platform descendant pid sweep (new file pid-descendants.ts) 2. Two P1 bug fixes folded back from commit-2 self-review == Pid descendant enumeration == `listDescendantPids(rootPid)` walks the process tree below the MCP child's root pid and returns all descendant pids in BFS order. `sigtermPids(pids)` sends SIGTERM tolerantly (ESRCH swallowed). Both are platform-aware: - Linux/macOS: `pgrep -P ` recursion (pgrep exit code 1 means no children, NOT an error — special-cased) - Windows: PowerShell `Get-CimInstance Win32_Process` filtered by `ParentProcessId` (CIM replaces deprecated wmic on Win10 21H1+) Bounded by `QUERY_TIMEOUT_MS=2000`, `MAX_DESCENDANTS=256`, `MAX_DEPTH=8` so a runaway process tree can't stall daemon shutdown. Graceful degradation: tool missing or timeout returns `[]` and logs warn; OS will eventually reap the orphans (Linux init / Windows job objects). `PoolEntry.forceShutdown` now calls `getTransportPid()` → `listDescendantPids` → `sigtermPids` BEFORE `client.disconnect()`. Closes the leaked-wrapper-process gap that pre-F2 per-session McpClient teardown also had — wrappers like `npx`, `uvx`, `pnpm dlx` spawn the actual server as a grandchild; killing only the wrapper leaves the real server hanging. New `McpClient.getTransportPid()` public getter that introspects `StdioClientTransport.pid` (returns undefined for non-stdio transports + already-exited children). Optional-chained call site in PoolEntry tolerates older mock McpClient stubs in tests. == P1 fixes folded back from commit-2 review == P1 #1: PooledConnection.release() was a documented no-op that leaked refs until releaseSession bulk-cleanup. Wired `PooledConnectionImpl.releaseCallback` to the pool-supplied `pool.release(id, sessionId)`. Pool's `acquire` (both fast-path existing-entry and post-spawn paths) passes the callback through `PoolEntry.attach`'s new `opts.release` parameter. P1 #2: createUnpooledConnection double-teardown. Path: client.discover() registers tools/prompts into session registries → entry.markActive([], []) → entry.attach(sid, view) which synchronously called view.applyTools([]) → removeMcpToolsByServer(serverName) wiping the registrations discover() just made. Fix: PoolEntry.attach now accepts `opts.skipReplay?: boolean`. createUnpooledConnection passes `skipReplay: true` AND a release callback that calls forceShutdown directly (per-session lifetime, no pool refcount). Existing pool paths pass `release` but NOT `skipReplay`, preserving snapshot replay for the late-attach race. Tests (6 new on pid-descendants.test.ts): - input validation (non-positive, NaN, no-children) - sigtermPids empty input + ESRCH tolerance - integration: spawn shell that spawns node grandchild, verify listDescendantPids finds at least one descendant (POSIX-only, CI-skip gated) Verification: - 161/161 MCP-related tests pass (44 mcp-client + 71 mcp-client-manager + 18 mcp-pool-key + 11 session-mcp-view + 11 mcp-transport-pool + 6 pid-descendants) - packages/core typecheck clean - lint clean on touched files Not included (deferred to later commits): - Health monitor / auto-reconnect inside PoolEntry. Existing per-server reconnect logic lives in McpClientManager (consecutiveFailures + isReconnecting + reconnectDelayMs); pool doesn't yet have its own monitor. PoolEntry.restart() works for manual restart; future commit will plumb `client.onerror` → pool's reconnect path with §6.6 backoff strategy. Refs: #3803, #4175 F2; design doc §6.4 pid sweep, §6.5/§6.6 spawn failure + reconnect backoff, §7.2 snapshot replay Generated with Qwen Code * feat(serve): wire McpTransportPool into QwenAgent daemon mode (#4175 F2 commit 4) Daemon-mode integration of the F2 shared MCP transport pool. Sessions running in the same workspace now share one MCP transport per unique server config, instead of each session spawning its own child process. Touches: - packages/core/src/config/config.ts: setMcpTransportPool / getMcpTransportPool. Pool reference stored on Config so ToolRegistry's nested McpClientManager construction can pick it up at config.initialize() time. Forward-declared via inline `import('...').McpTransportPool` to avoid a circular import between config.ts and tools/. - packages/core/src/tools/tool-registry.ts: forwards config.getMcpTransportPool() into the McpClientManager ctor. When undefined, manager keeps its pre-F2 behavior (71/71 existing manager tests pass unchanged). - packages/core/src/tools/mcp-client-manager.ts: new optional `pool?` ctor param + new `discoverAllMcpToolsViaPool` branch in discoverAllMcpTools. Gated on pool presence so standalone qwen is unaffected. Pool path: * Iterates servers with disable check * Calls pool.acquire(name, cfg, sessionId, toolReg, promptReg) * Tracks returned PooledConnection in `pooledConnections` map * On disconnectServer: pooled.release() + map delete * On stop(): releaseAllPooledConnections + existing flow SDK MCP servers stay on the legacy path inside the pool itself (createUnpooledConnection); manager doesn't need a parallel SDK code path. - packages/cli/src/acp-integration/acpAgent.ts: QwenAgent.mcpPool field, eager construction in ctor (V21-13 Q6 resolved). Reads options from env vars set by runQwenServe: * QWEN_SERVE_NO_MCP_POOL=1 → kill switch (mcpPool stays undefined; sessions fall back to per-session spawn) * QWEN_SERVE_MCP_POOL_TRANSPORTS=stdio,websocket,http,sse → operator opt-in for HTTP/SSE pooling (V21 C8); default keeps stdio + websocket only * QWEN_SERVE_MCP_POOL_DRAIN_MS=N → drain grace override (default 30s; bounded [1s, 10min]) newSessionConfig calls config.setMcpTransportPool(this.mcpPool) BEFORE config.initialize() so the ToolRegistry that initialize constructs picks up the pool reference. New `shutdownMcpPool(timeoutMs)` method called from the SIGTERM/SIGINT handler in runAcpAgent before runExitCleanup so the pool's descendant pid sweep (commit 3) catches npx/uvx wrapper grandchildren. - packages/core/src/index.ts: barrel exports for the pool primitives (McpTransportPool, POOLED_TRANSPORTS_DEFAULT, types, helpers). - packages/core/src/tools/mcp-pool-key.ts: dedupe — removed local McpTransportKind / mcpTransportOf definitions and re-export from mcp-client-manager.ts (avoids name collision in the index.ts barrel). Tests: - mcp-client-manager.test.ts: 2 new tests * "routes discovery through the pool when one is injected" — asserts pool.acquire called with (name, cfg, sessionId, toolReg, promptReg); inverse invariant that McpClient is NOT constructed by the manager when pool present (catches a regression where the pool branch silently bypasses). * "falls back to per-session McpClient spawn when no pool injected" — explicit backward-compat assertion. - All 73/73 mcp-client-manager tests pass (71 existing + 2 new) - All 161/161 MCP-related tests pass (44 + 73 + 18 + 11 + 11 + 6 — incremented manager count) - packages/core typecheck clean - packages/cli typecheck: pool-related imports resolve; pre-existing serve/status.ts + @google/genai issues unrelated to F2 unchanged Backward compatibility: - Standalone qwen (non-daemon): QwenAgent not constructed; pool not constructed; behavior identical to pre-F2 - QWEN_SERVE_NO_MCP_POOL=1: kill switch falls back to per-session spawn even in daemon mode - ACP child invoked with no pool env vars: defaults activate (pool on, stdio+websocket transports, 30s drain) - Existing McpClientManager construction sites (ToolRegistry, test fixtures with the older 1-6 arg signatures) unchanged because new pool param is optional and trailing - McpTransportKind / mcpTransportOf still exported from the same module path consumers used pre-F2 Not included (deferred to commits 5-6): - Pool-aware GET /workspace/mcp snapshot (commit 5) — buildWorkspaceMcpStatus still reads from bootstrap session's manager; pool snapshot integration via QwenAgent extMethod is next commit - Pool-aware POST /workspace/mcp/:server/restart route with ?entryIndex= (commit 5) - Budget guardrails graduation to workspace scope (commit 6) — pool currently has no `--mcp-client-budget` integration, so per-session budget enforcement still applies in pool mode (each session's manager state machine is independent). PR 14b push events still fire per session. Refs: #3803, #4175 F2; design doc §2 current state, §10 per-session injection, §17 shutdown ordering Generated with Qwen Code * fix(serve): repair acpAgent imports clobbered by pre-commit auto-format (#4175 F2 commit 4 follow-up) The pre-commit eslint --fix in the previous commit (3dcdddf19) merged the value imports into the type-only import block, which yielded `import type { ... type McpTransportKind, ... }` — TypeScript rejects nested `type` modifier inside `import type`. Restore the original two-block layout: value imports for runtime symbols (McpTransportPool, POOLED_TRANSPORTS_DEFAULT, etc.) and a separate `import type { ... }` for types only (McpTransportKind, ApprovalMode, Config, ConversationRecord, DeviceAuthorizationData). Pre-existing unrelated issues (ServeMcpTransport / @google/genai in cli/) are not addressed here. Generated with Qwen Code * fix(core): SDK MCP servers must stay on legacy path in pool mode (#4175 F2 commit 4 follow-up 2) Self-review found a regression: pool mode would route SDK MCP servers through pool.acquire which delegates to createUnpooledConnection. createUnpooledConnection constructs an McpClient with the pool's `sendSdkMcpMessage` callback — but the pool was constructed in QwenAgent ctor with no callback, so SDK MCP server tool calls would fail in daemon mode. Fix: discoverAllMcpToolsViaPool checks isSdkMcpServerConfig per server and routes SDK servers to the legacy discoverMcpToolsForServer path which preserves the per-session sendSdkMcpMessage wiring from McpClientManager's ctor. Non-SDK servers continue through pool.acquire. Bypass is per-server, not per-manager, so a workspace mixing SDK and non-SDK servers gets both pool-shared transports for the non-SDK ones AND working SDK MCP for the rest. Generated with Qwen Code * fix(core): wenshao review fold-ins — 7 critical races + lifecycle gaps + 4 suggestions (#4175 F2 PR #4336) Folds in @wenshao's first review pass on PR #4336. 7 critical bugs in pool lifecycle / race handling, 4 smaller suggestion fixes. Each issue keyed by its label in the PR comment thread for back-reference. == Critical fixes == C1 (acpAgent.ts:269) — Normal IDE close path missing pool drain. `await connection.closed` returned without calling `shutdownMcpPool`, leaking shared MCP entries (subprocess + wrappers) until OS reaped them — a real regression vs pre-F2 where each session's manager torn down its own clients on disconnect. Mirror SIGTERM handler's pool drain on the normal-close branch too. C2 (mcp-pool-entry.ts:291 area) — `attach()` ref ordering broke max-idle hard cap. Pre-fix, `attach` added the ref before calling `cancelDrainTimer`, so the `refs.size > 0` check inside cancelDrainTimer was always true and the maxIdle timer + firstIdleAt got reset on every attach — completely defeating its purpose (per design §6.3: "started at first idle and NEVER reset"). Fix: cancelDrainTimer now only cancels the drain grace timer; maxIdle survives the entire entry lifetime, cleared only by forceShutdown. C3 (mcp-pool-entry.ts:401) — `doRestart()` zombie state on reconnect failure. Pre-fix, a thrown `client.connect()` / `client.discoverAndReturn()` propagated up but left the entry with `localStatus = CONNECTED`, `state = 'active'`, stale snapshot — pool snapshot lies, subsequent acquires reuse the broken entry. Fix: try/catch wraps connect + discover; on failure transitions to terminal `'failed'` state, sets DISCONNECTED status, emits `failed` event, detaches subscribers via SessionMcpView.teardown, calls onClosed so pool drops the entry from its map. C4 (mcp-pool-entry.ts:361) — `forceShutdown`/`attach` race creates zombie connections. Pre-fix, `state = 'closed'` was assigned AFTER two async yields (`await listDescendantPids`, `await client.disconnect()`). During those yields, a concurrent `acquire` calling `attach` only rejected `'closed'`/`'failed'` states — got a handle to an entry mid-teardown. Fix: flip state to `'closed'` synchronously at the top of forceShutdown, before any await. Concurrent attach now sees 'closed' immediately and rejects. C5 (mcp-transport-pool.ts:399) — `drainAll` race with in-flight spawns. Pre-fix, after Promise.race resolved, `entries.clear()` + `spawnInFlight.clear()` ran synchronously. But in-flight spawn promises continued executing and called `entries.set(id, entry)` AFTER the clear — orphan entries leaking subprocesses past pool shutdown. Fix: introduce `draining` mutex flag (acquire rejects when set), and `await Promise.allSettled` on in-flight spawns BEFORE taking the entry snapshot. Spawn completion before clear is now ordered correctly. C6 (mcp-pool-entry.ts:155) — PoolEntry ignored transport- level errors. Pre-fix, McpClient.onerror writes DISCONNECTED to the global `serverStatuses` map on transport drop, but PoolEntry's `localStatus` stayed CONNECTED — pool's `aggregateStatusByName` then read the stale localStatus and "any-CONNECTED-wins" overwrote the correct DISCONNECTED back into the global map. Fix: PoolEntry registers a module-level status change listener filtered by serverName, mirrors the GLOBAL value into localStatus on every change. `suppressNextStatusEcho` flag guards against listener loops when the entry's own updateGlobalStatus writes to the global map. Listener detached on forceShutdown / failed-state transition. Sub-fix in spawnEntry: order is now `entries.set(id, entry)` BEFORE `entry.markActive(...)`. Pre-fix, markActive ran updateGlobalStatus before entries.set, so aggregateStatusByName couldn't find the just-spawned entry, returned DISCONNECTED, wrote that to the global map, the new status listener echoed it back as `localStatus = DISCONNECTED` — defeating the CONNECTED state markActive had just set. Reorder + idempotent `entries.delete(id)` in catch covers the race. C7 (mcp-client-manager.ts:966) — `discoverAllMcpToolsIncremental` bypassed pool. The pool gate in `discoverAllMcpTools` correctly routed the bulk path through `discoverAllMcpToolsViaPool`, but `discoverAllMcpToolsIncremental` (called from `Config.startMcpDiscoveryInBackground` during boot's default progressive mode) had no such guard — silently reverting to per-session McpClient spawning during the exact path most daemon sessions take. Fix: same `if (this.pool) return discoverAllMcpToolsViaPool(cliConfig)` gate at the top of discoverAllMcpToolsIncremental. == Suggestions == S1 (session-mcp-view.ts:38) — Docstring claimed both includeTools and excludeTools support `()` parens form, but only includeTools strips parens. excludeTools uses direct equality (matches pre-F2 `mcp-client.ts:isEnabled` history). Doc fixed to reflect actual behavior. S2 (pid-descendants.ts:166) — `sigtermPids` docstring claimed it used `taskkill /F` on Windows, but the implementation always calls `process.kill(pid, 'SIGTERM')` regardless of platform. On Windows, Node polyfills SIGTERM to TerminateProcess (similar effect, no shell-out needed). Doc fixed; implementation unchanged. S3 (session-mcp-view.ts:110) — Debug log contained literal "N" instead of `${count}` interpolation. Operators enabling debug logging saw a meaningless placeholder. Track actual `registered` count and interpolate. S4 (mcp-transport-pool.ts:545) — `createUnpooledConnection` passed `() => MCPServerStatus.CONNECTED` as the status aggregator callback. After forceShutdown, this would write CONNECTED to the global serverStatuses map even though the transport was dead. Fix: aggregator now delegates to `client.getStatus()` so the global map reflects the actual McpClient state. == Verification == - 163/163 MCP-related tests pass (44 + 71 + 18 + 11 + 11 + 6 + 2) - packages/core typecheck clean - All fixes folded into the commit-where-the-bug-lived (commit 2 / commit 3 / commit 4) via fix-up commit on top — preserves bisectability of the buggy state for future forensics Refs: PR #4336 review by @wenshao (commit 4 round 1) Generated with Qwen Code * feat(serve): pool-aware status + restart routes (#4175 F2 commit 5) Wire the F2 transport pool into the daemon's `GET /workspace/mcp` and `POST /workspace/mcp/:server/restart` surfaces, plus advertise two new conditional capability tags. Status route enrichment (`buildWorkspaceMcpStatus`): - pool snapshot taken once outside the per-server loop (avoids N walks) - per-server cells gain `entryCount` + `entrySummary` (V21-7 opaque `entryIndex`, NOT raw fingerprint) when the pool holds at least one matching entry - pool snapshot failure is a stderr-loud non-fatal — the legacy budget-accounting cells still render Restart route routing (`workspaceMcpRestart` ext method): - new `?entryIndex=N` query param (or `*` / omitted) on `/workspace/mcp/:server/restart` — bounded non-negative integer or the literal `*`; bad inputs return `400 invalid_entry_index` - ACP child routes through `pool.restartByName(name, {entryIndex})` when the pool holds entries; falls back to the legacy `discoverToolsForServer` path otherwise (`--no-mcp-pool` daemons, unpooled HTTP/SSE/SDK transports, or names that drained out) - legacy single-entry response shape `{restarted, durationMs}` preserved; multi-entry responses use the new `{entries: RestartResult[]}` shape — clients gated on the `mcp_pool_restart` capability tag are the only senders of `entryIndex` - pool-mode hard restart failure fans out one `mcp_server_restart_refused` event per failed entry with `reason: 'restart_failed'` (additive enum value) plus `details` carrying the underlying error text; soft-skip pre-flight checks (`disabled` / `in_flight` / `budget_would_exceed`) still run BEFORE the pool branch Capability advertisement: - `mcp_workspace_pool` + `mcp_pool_restart` both gated on a new `mcpPoolActive` toggle in `AdvertiseFeatureToggles` - conditional predicate is default-OFF (matches `require_auth` pattern); server.ts call site flips to default-ON via `opts.mcpPoolActive !== false`, so a daemon booted without the kill switch advertises both tags by default - `runQwenServe.ts` infers `mcpPoolActive: false` when the parent process has `QWEN_SERVE_NO_MCP_POOL=1` so the envelope tracks the ACP child's actual feature set SDK type extensions (additive only): - `ServeWorkspaceMcpServerStatus.entryCount` + `entrySummary` - `DaemonMcpServerRestartedData.entryIndex?` - `DaemonMcpServerRestartRefusedData.{reason: 'restart_failed', entryIndex?, details?}` - `MCP_RESTART_REFUSED_REASONS` widened to include `restart_failed` Tests: - `EXPECTED_REGISTERED_FEATURES` gains the two pool tags; conditional- features drift test asserts `mcpPoolActive` predicate behavior - `daemonEvents.test.ts` exercises the new `restart_failed` reason through the reducer 163 F2 tests + 62 acp-bridge tests + 46 daemon events tests pass. * fix(serve): self-review fold-ins for F2 commit 5 — capability test + SDK doc Two findings from the code-reviewer pass on `edeb0a5cf`: R1 (critical): the `/capabilities` v1-envelope test was asserting `features` against `getAdvertisedServeFeatures()` (no toggles → both new pool tags filtered out by the default-OFF predicate), but the actual response uses `mcpPoolActive: opts.mcpPoolActive !== false` (default-ON at the call site). Anchored the assertion against the same toggle the route uses, plus added a separate test that explicitly boots with `mcpPoolActive: false` and verifies both pool tags drop out (mirrors the `QWEN_SERVE_NO_MCP_POOL=1` kill-switch path). R3 (doc clarity): the `restart_failed` reason's jsdoc claimed old SDK reducers "see the new value as `unknown` (TS structural widening) and surface it generically rather than crashing." That described the type system but mis-stated the runtime: `isMcpServerRestartRefusedData` calls `MCP_RESTART_REFUSED_REASONS.has(...)` and returns false for unknown reasons, so `parseDaemonEvent` silently DROPS the event. New text explains the closed-set predicate + how the additive-protocol contract still holds (pre-PR SDKs gate on `mcp_pool_restart` before sending `entryIndex`, so they shouldn't be observing pool-mode multi-entry restarts). * fix(core): wenshao R1-R8 review fold-ins for F2 commit 5 Eight findings from wenshao's review of commit 5; six adopted as real bug fixes / encapsulation wins, two with partial / declined replies. R1 (critical): `maxIdleTimer` force-closed actively-used pool entries. The C2 fix intentionally let the timer survive attach/detach flap, but the fire-action didn't re-check `refs.size`. A session that re-attached inside the 30s drain grace and stayed busy for 4+ minutes would lose the entry permanently when `maxIdleTimer` (started at the earlier detach) fired. Now: if active refs exist at fire time, log + reset `firstIdleAt` so the next idle window gets a fresh hard cap. R2 (critical): incremental discovery released ALL pooled connections then re-acquired everything. Pre-fix every progressive-mode boot pass or `/mcp refresh` produced a brief window with zero MCP tools registered AND bounced every entry's drain timer. Now: diff `pooledConnections` against the desired (name, fingerprint) set and release only stale entries; survivors stay attached, no tool registry churn. SDK MCP servers still re-run via the legacy path (idempotent re-call). R3 (correctness): `doRestart` updated `toolsSnapshot`/`promptsSnapshot` and emitted typed events but no `SessionMcpView` instance subscribed to that event stream — so session ToolRegistry instances kept stale pre-restart registrations. Latent until commit 5 landed the restart HTTP route; now a real correctness bug. Iterate `subscribers` directly after snapshot update so views actually pick up the new tools/prompts. R4 (cosmetic→correctness): `getSnapshot()` counted websocket toward `subprocessCount`, but websocket transports dial a (potentially remote) server and don't spawn a local OS child — inflated the operator-facing capacity-planning metric. Restricted to `stdio` only. R5 (defense-in-depth): the Windows `Get-CimInstance` PowerShell script interpolated `${pid}` directly into the `-Filter` string. The entry-point integer guard makes injection impossible today, but binding the pid to a `$p` variable up front makes the integer-only contract robust against future relaxations of the guard. R6 (encapsulation): `PoolEntry.cfg` was readonly-public, exposing secrets (env API keys, header auth tokens, OAuth fields) to anyone holding an entry reference. Made private; added `transportKind` getter for the only external reader (subprocessCount classification in `getSnapshot`). R7 (partial): removed five PoolEvent type guards, the `Prompt` re-export, and `PoolEntryConnectionStatus` — all premature public API with zero callers in source or tests. Kept `MCPCallInterruptedError` because design §13.4 declares it as the user-facing contract for the V21-5 in-flight call interruption follow-up; removing it would lose the invariant carrier. R8 (cleanup): SIGTERM handler and IDE-initiated close path had identical `if (agentInstance) { try { await shutdownMcpPool(8_000) } catch ... }` blocks. Extracted into `drainPoolBeforeExit(label)` so both paths share the timeout + log labels and future drain-semantic changes happen in one place. R9 / R10 deferred: the McpClientManager 7th-arg sentinel pattern (R9) and per-PID-per-level pgrep cost (R10) work correctly today; both are refactoring/perf optimizations for a later cleanup PR rather than F2 correctness blockers. Tests: - All 163 F2 tests pass; all 73 mcp-client-manager tests pass - No new tests added; the existing R3 fix was caught only because commit 5's restart route activated the latent path. Adding a unit test for the snapshot fan-out would require wiring a mock SessionMcpView; deferred to commit 6's test harness expansion. * feat(serve): graduate MCP budget guardrails to workspace scope (#4175 F2 commit 6) Move slot reservation + 75% hysteresis + refused-batch coalescing from per-session McpClientManager copies onto a single workspace-scoped controller owned by the pool. 4 sessions × budget=2 now caps the workspace at 2, not 8. Core class (`packages/core/src/tools/mcp-workspace-budget.ts`): - New `WorkspaceMcpBudget` mirrors the manager's state machine (`tryReserve` / `release` / `recordRefusal` / hysteresis at `MCP_BUDGET_WARN_FRACTION`/`MCP_BUDGET_REARM_FRACTION` / bulk-pass coalescing) but is constructed once per workspace. - Reservation key is server NAME (matches PR 14 v1 contract; two pool entries with same name but divergent fingerprints share one slot). - `recordRefusal` flushes inline as a length-1 batch when called out-of-bulk-pass; bulk passes accumulate and `endBulkPass` does the coalesced emit (mirrors `McpClientManager.refuseAndLog → emitRefusedBatchIfAny`). Pool integration (`mcp-transport-pool.ts`): - New optional `budget?: WorkspaceMcpBudget` ctor option + `getBudget()` accessor for snapshot builders. - `acquire()` calls `tryReserve` pre-spawn; `'refused'` returns `BudgetExhaustedError` after `recordRefusal`. Spawn-failure path rolls back the slot (V21-4) when no sibling entry holds the name. - Entry close callback releases the slot if no other entry shares the same `serverName` (multi-fingerprint preservation). Manager integration (`mcp-client-manager.ts`): - `discoverAllMcpToolsViaPool` brackets the pass with `beginBulkPass`/`endBulkPass` so per-server BudgetExhaustedError refusals coalesce into ONE `refused_batch` event at end of pass. - `BudgetExhaustedError` from pool is logged at debug (deliberate refusal, not a failure); other errors stay at `error`. Daemon wiring (`acpAgent.ts`): - `QwenAgent` ctor reads `QWEN_SERVE_MCP_CLIENT_BUDGET` / `QWEN_SERVE_MCP_BUDGET_MODE` env vars (same path as per-session manager) and constructs `WorkspaceMcpBudget` when budget > 0, passes it to the pool. - `broadcastBudgetEvent(event)` fans workspace-scoped events to every attached session via per-sid `extNotification`s on the shared connection — replaces N per-session callbacks with one pool callback fanning out N times. - `newSessionConfig` skips the per-session `setMcpBudgetEventCallback` wiring when the workspace budget is active (prevents double-firing). - `buildWorkspaceMcpStatus` reads pool budget when active, marks the cell `scope: 'workspace'`. Per-session fallback unchanged. - `buildBudgetCells` accepts optional `scope` parameter; pre-F2 daemons / `--no-mcp-pool` keep `'session'` for back-compat. SDK additive surface (`sdk-typescript/src/daemon/events.ts`): - `DaemonMcpBudgetWarningData.scope?: 'workspace' | 'session'` - `DaemonMcpChildRefusedBatchData.scope?: 'workspace' | 'session'` - New helper `isWorkspaceScopedBudgetEvent(data)` for SDK consumers branching on scope. Type predicates unchanged (scope is optional). - Reducer counters (`mcpBudgetWarningCount` / `mcpChildRefusedBatchCount`) increment regardless of scope per V21-12 — workspace events fan to all sessions so counters move in lockstep. Tests: - 17 new `WorkspaceMcpBudget` tests covering tryReserve, release, hysteresis state machine, refused-batch coalescing, getters - 3 new pool integration tests covering acquire-refused-on-cap, slot release on entry close, slot rollback on spawn failure - All 163 pre-existing F2 tests pass; 229 total core+SDK tests Total: 1 new core class, ~600 LOC production + ~270 LOC tests. * fix(core): self-review fold-ins for F2 commit 6 — slot release race + iter safety Three findings from the code-reviewer pass on `ef2974b85`; one real race fix + two clarity/defensive improvements. R1 (race, important — 86): close-callback released the budget slot prematurely when a same-name in-flight spawn was still running. The sibling check inspected only `this.entries`, missing entries that hadn't yet completed `markActive`. Sequence: entry A for 'srvA' finishes spawn → registers in `entries`. Entry B (different fingerprint, same name) starts spawning. Entry A drains; close- callback finds no siblings in `entries` (B not yet registered) → releases the slot. B finishes; slot is unreserved while B occupies capacity. A subsequent acquire for a third name slips past the cap. Fix: new `hasNameSibling(name)` helper checks BOTH `this.entries` and `this.spawnInFlight.keys` (form `${name}::${fingerprint}`, so a `startsWith(`${name}::`)` test isolates same-name in-flight spawns). Used by the close-callback AND the spawn-failure rollback. Order of catch/finally chained on the spawn promise is also fixed: `finally` removes from `spawnInFlight` BEFORE the `catch` runs the rollback, so `hasNameSibling` sees the post-cleanup state. Pre-fix the catch ran first while the in-flight entry was still in the Map — masked the rollback's release decision. New test: `preserves slot when entry closes during a same-name in-flight spawn (R1 race fix)` exercises exactly this sequence. R2 (docs): SDK reducer counter docstrings updated to call out the N× workspace fan-out multiplier explicitly. A workspace-scoped `mcp_budget_warning` event fires once at the budget but produces N reducer increments across N attached sessions on the daemon's connection. Pre-fix the docstring didn't mention this and consumers aggregating `mcpBudgetWarningCount` across sessions would double-count silently. Now both `mcpBudgetWarningCount` and `mcpChildRefusedBatchCount` docstrings have a "workspace-scope multiplier" paragraph pointing consumers at `isWorkspaceScopedBudgetEvent` for branching. R3 (defense): `broadcastBudgetEvent` snapshots `this.sessions.keys` into `Array.from(...)` BEFORE the per-id async fan-out so a concurrent `killSession` (which mutates `this.sessions` synchronously inside its handler) can't corrupt the iterator. No known reproducer in the current code paths but cheap defensive hardening — matches the same pattern used by the bridge's `broadcastWorkspaceEvent`. R2 of the original review (V21-12 reducer scope-blindness) is by- design per design §11.4: SDK consumers wanting a deduplicated "workspace events fired" tally use `lastMcpBudgetWarning?.scope` to gate. The docstring fix (above) closes the documentation gap that made this contract invisible. Tests: 151 pool + workspace-budget + manager + SDK events tests pass (3 new pool integration tests including the R1 regression). Lint clean. * fix(core): wenshao W1-W15 review fold-ins for F2 commits 5+6 Twelve real fixes (7 critical + 5 minor) + 3 declined-with-reply. W1 (critical): pool spawn-failure leaked `statusChangeListener` — catch only ran `entries.delete` + `client.disconnect`, never `forceShutdown` (the sole removal path). Each failure leaked one listener permanently. Fix: call `entry.forceShutdown('manual')` before disconnect; wrap in try/catch since the entry never reached `active`. W2 (critical): `statusChangeListener` corrupted sibling entries' `localStatus` for multi-fingerprint name collisions. Module-level `serverStatuses` is shared across all entries with the same `serverName`; entry A's transport error wrote DISCONNECTED, B's listener fired with that status, and the `if (status !== this.localStatus)` guard didn't catch it because B was CONNECTED. Fix: cross-check `this.client.getStatus() !== status` (per-entry truth) before mirroring — sibling writes are now ignored. W3 (critical): `doRestart()` skipped the `listDescendantPids` + `sigtermPids` sweep that `forceShutdown` performs. For stdio MCP servers wrapped by `npx`/`uvx`/`pnpm dlx`, every restart-via-HTTP left the actual server grandchild as an orphan. Fix: mirror the sweep BEFORE `client.disconnect`; per-pid failures tolerated. W4 (critical): `doRestart()` didn't `cancelDrainTimer` or transition `'draining' → 'active'`. An entry in drain grace whose restart arrived would yield to the drain timer mid-disconnect, get force-closed, then `client.connect` would spawn an orphan that the pool no longer tracks. Fix: cancel drain + transition state at the top of `doRestart`. W5 (critical): `McpClientManager.pooledConnections` held dead handles after a pool entry transitioned to `'failed'` (entry removed from `pool.entries`, manager never learned). Subsequent discovery passes saw `pooledConnections.has(name)` and skipped re-acquiring → server's tools permanently lost for the session until full `stop` + rediscovery. Fix: subscribe to entry events on `acquire`; evict on `'failed'` (idempotent via `get(name) === conn` guard). W6 (critical): `discoverAllMcpToolsViaPool` was not re-entrant. Two concurrent passes (full + incremental, or two incrementals) could both see `pooledConnections.has(name) === false` before either called `.set()` → second `.set` overwrote first → conn1 leaked forever. Fix: per-manager `discoveryInFlight` mutex; second caller awaits the same promise. W14 (critical): `createUnpooledConnection`'s catch path had the same `statusChangeListener` leak as W1 (different code path, same root cause — only `forceShutdown` removes the listener). Fix: same mirror in the unpooled catch. W9 (minor): `parsePoolDrainMs` accepted `'30000ms'` / `'30000abc'` silently via `Number.parseInt` truncation. Fix: strict `^\d+$` regex; reject with stderr warning + default fallback. W10 (minor): pool's `acquire` called `indexAttach(sessionId, id)` BEFORE `entry.attach()`. If `attach` threw (e.g., entry transitioned to `closed`/`failed` between the existence check and the call), the reverse index retained a stale mapping. Fix: index AFTER `attach` succeeds (both fast path + in-flight path). W13 (doc): `subprocessCount` JSDoc still claimed `stdio + websocket` after R4 restricted it to stdio in commit 5. Fix: doc updated. W15 (defensive): bridge's pool-mode response handler cast `response as PoolEntries` and iterated `response.entries` without runtime shape validation. A buggy/out-of-sync ACP child returning a malformed shape would crash the route with TypeError. Fix: `Array.isArray` check + per-entry shape guard; malformed entries skipped with stderr warning. W7 (test gaps, partial): added regression test `serializes concurrent discovery passes via mutex` for W6. Other coverage gaps (drain mutex, spawnEntry failure, restart failure, createUnpooledConnection) are deferred — better addressed via a focused test-coverage commit after F2 series merges. Declined (with reply on PR): - W8 (`maxReconnectAttempts`/`reconnectStrategy` unused) — health monitor reconnect is a deferred F2 follow-up per design §6.6; the fields stay as forward-compat placeholders. - W11 (duplicate fast-path/in-flight-path attach blocks) — accepted refactor opportunity; not blocking F2 series merge. - W12 (passesSessionFilter O(M×N)) — micro-perf optimization; measurable only with hundreds of tools / large filter lists. Tests: 231 F2/SDK tests pass (1 new mutex regression test); 62 acp-bridge tests pass. Lint clean. * docs(serve): F2 design v2.2 — record PR #4336 32-fold-in review history The PR cycle on #4336 surfaced 32 review fold-ins across 3 wenshao review batches plus 2 self-review batches. Each fold-in is recorded in v2.2 changelog with site / what was wrong / fold-in commit ref so a future contributor reading the design doc + git log can trace every behavior nudge back to its review trigger. Highlight critical fixes that landed mid-PR: - C1 (IDE-close path missed pool drain — leaked entries until OS reaped) - C3 (doRestart reconnect failure left zombie state) - C5 (drainAll mid-spawn race) - C6 (statusChangeListener missing serverName filter) - WR1 (maxIdleTimer fire-action ignored active refs) - WR2 (release-all-then-acquire-all left zero-tools window) - WR3 (doRestart skipped subscriber fan-out) - 6R1 (slot-release race during same-name in-flight spawn) - W2 (sibling-fingerprint statusChangeListener corruption) - W3 (doRestart skipped descendant pid sweep — orphan grandchildren) - W4 (doRestart drain-timer race orphaned new subprocess) - W5 (manager held dead handles after entry 'failed') - W6 (discoverAllMcpToolsViaPool not re-entrant — leaked conn1) Plus 5 declined-with-reply items (W7/W8/W11/W12/R9/R10) filed as F2 follow-ups for a future cleanup PR. * fix(core): wenshao W21-W25 review fold-ins for F2 commit 6 — critical bugs round 4 Three critical bugs + one parsing divergence + one test gap, four adopted as fixes. Round 4 of cumulative wenshao review on F2 PR #4336; all earlier rounds (C1-C7+S1-S4, R1-R10, W1-W15) already shipped in `ae0b296c4` / `72399f109` / `4a3c5cd90`. W21 (critical): `hasNameSibling` used `id.startsWith(\`${name}::\`)` on `spawnInFlight` keys, which produces false positives when a sibling name BEGINS with `${name}::` — server names CAN contain `::` per `mcp-pool-key.test.ts:258`, and `connectionIdOf` is just string concatenation with zero sanitization. Sequence: configure servers `"ext"` and `"ext::github"`, spawn for `"ext"` fails → rollback finds `"ext::github::"` in spawnInFlight, returns `true` (false positive) → slot for `"ext"` never released → permanent leak until daemon restart. Fix: use `parseConnectionId` (which uses `lastIndexOf('::')`) to extract the exact serverName and compare via equality. Malformed ids skip via try/catch so a stray bad key doesn't crash the rollback path. W24 (parsing divergence): `createWorkspaceMcpBudget` used `Number.parseInt(rawBudget, 10)` while `McpClientManager.readBudgetFromEnv` uses `Number(rawBudget)` + `Number.isInteger`. Same env var produced 100× enforcement difference for `"1e2"` (pool: 1, manager: 100) and divergent acceptance for `"2.5"` / `"0x10"`. Fix: switch to `Number(...)` + explicit `Number.isInteger` guard so pool and manager honor identical env values. W25 (critical, gpt-5.5): pool-mode `spawnEntry` awaited `client.connect()` + `client.discoverAndReturn()` directly with no timeout. A hung stdio/websocket server's connect/discover left `spawnInFlight` unresolved forever — every same-id acquirer waited indefinitely AND the budget slot was never rolled back because the catch never ran. Fix: new `runWithTimeout` wrapper + new `discoveryTimeoutFor(cfg)` helper mirroring `McpClientManager.discoveryTimeoutFor` (stdio 30s, remote 5s, per-server `discoveryTimeoutMs` override clamped to [100ms, 300s]). On timeout the existing W1 catch runs `entry.forceShutdown('manual')` + `client.disconnect()` (which races to close the transport ahead of any silent tool registration) AND the W6 budget rollback releases the slot. W23 (test gap): added `swallows BudgetExhaustedError from pool.acquire and logs at debug` to mcp-client-manager.test.ts. Wires a fake pool whose `acquire` throws `BudgetExhaustedError` for one server, asserts the discovery completes (Promise.all resolves), only the non-refused server lands in `pooledConnections`, and `beginBulkPass`/`endBulkPass` fire exactly once each. W22 (test gap, deferred): five integration paths in acpAgent.ts remain untested (`createWorkspaceMcpBudget`, `broadcastBudgetEvent`, snapshot builder workspace branch, `skipPerSessionBudgetCallback` guard, `buildBudgetCells` scope param). The cli package's vitest config requires a workspace setup not available in this branch; adding tests for these paths produces files that pass locally but might break in CI. Filed as F2 follow-up rather than blocking merge — same pattern as W7 commit-6 partial-adopt. Tests: 186 F2 + workspace-budget + manager tests pass (1 new W23 regression). Lint clean. * fix(core): wenshao W31-W40 review fold-ins for F2 commits 5+6 — round 5 Two more critical doRestart races + DRY refactor + 3 test gaps. W33 duplicate of already-fixed W21 (no action). W31 (critical): `doRestart` cancelled `drainTimer` (W4 fix) but NOT `maxIdleTimer`. Same orphan-process race as W4, different timer: when the entry was draining (refs=0, both timers running), the maxIdleTimer's fire-action checked `refs.size > 0` and force-shut down the entry mid-restart → `doRestart` resumed and spawned an orphan that the pool no longer tracked. Fix: cancel BOTH timers + reset `firstIdleAt` at top of `doRestart` so a future detach starts a fresh idle window. W32 (critical): `doRestart` failure catch skipped descendant pid sweep. When `client.connect()` partially spawned a stdio wrapper before `discoverAndReturn()` failed, the wrapper's grandchildren (npx / uvx workers, real MCP server) survived as orphans. Every failed restart leaked one+ orphan process. Fix: call `sweepAndDisconnect('restart_failed')` in the failure catch so the NEW transport's grandchildren are SIGTERM'd before the entry transitions to `'failed'`. W34 (improvement): generation guard alone didn't catch concurrent `forceShutdown`. If `forceShutdown` ran during any of `doRestart`'s awaits (e.g., `drainAll` mid-restart on shutdown), the entry was in `'closed'` state but `doRestart` resumed and wrote CONNECTED + emitted `reconnected` on a pool-evicted zombie entry. Fix: state guard `if (this.state === 'closed' || this.state === 'failed')` after the generation guard; drop the snapshot silently. W35 (observability): `doRestart` logged pid-sweep + disconnect failures at `debug` level while `forceShutdown`'s identical operations used `warn` and `error`. In production (debug off) a restart that failed to sweep grandchildren was completely invisible — operators debugging memory climb saw "successful restarts" with no error trail. Fix: unified into the new `sweepAndDisconnect` helper with `warn` for sweep failures, `error` for disconnect failures. W36 (doc): `restartByName` JSDoc said `Promise.allSettled` but the implementation uses `Promise.all` with per-entry try/catch (rejections never escape). Doc updated to match. W37 (DRY): pid sweep + disconnect was duplicated nearly verbatim across three sites — `forceShutdown`, `doRestart` pre-call, and (after W32) the failure catch. Extracted shared `sweepAndDisconnect(reason)` private helper. Future changes to either step now happen in one place. W38 (coverage): no test exercised `discoverAllMcpToolsIncremental` with a pool — the C7 commit 5 fix added the gate but only `discoverAllMcpTools` had pool-routing coverage. Added regression test mirroring the existing pool test but calling `discoverAllMcpToolsIncremental`. W39 (coverage): no test exercised `disconnectServer`'s pool-mode branch (release pooled connection + delete from `pooledConnections`). Added test wiring fake pool, populating via discovery, asserting `release()` called on disconnect. W40 (coverage): existing `restartByName` test only asserted `results[0].restarted === true` — never verified that the R3 fix's post-restart subscriber fan-out actually delivered the new snapshot to attached views. Added assertion: post-restart `removeMcpToolsByServer` call count > pre-restart count (one extra call from the fan-out's `view.applyTools` invocation). W33 was reviewer noticing the same `hasNameSibling` startsWith prefix collision already fixed by W21 in `3fb453220` — replied with the commit reference, no action needed. Tests: 189 F2 + workspace-budget + manager tests pass (3 new W38 / W39 / W40 regressions). Lint clean. * fix(core): wenshao W41-W46 review fold-ins for F2 commits 5+6 — round 6 Six review findings — 4 real critical bugs, 1 false positive (already correct), 1 coverage gap deferred. The bugs are tightly clustered around the doRestart + spawnEntry timeout / state-guard surface. W41 (false positive): reviewer claimed `entryCount` / `entrySummary` not on `ServeWorkspaceMcpServerStatus`. Verified — they ARE declared in `packages/acp-bridge/src/status.ts` (added in commit 5). Both core and cli typecheck pass cleanly. No change. W42 (critical, build break): TS2367 at `mcp-pool-entry.ts:639`. The `if (this.state === 'closed' || this.state === 'failed')` state guard added in W34 fold-in passes runtime correctness but TS's control-flow analysis narrows `this.state` along the non-throwing path of the prior `try { connect; discover } catch` (catch sets state='failed' then throws), eliminating `'closed'`/`'failed'` from the reachable union. Build hard-failed. Fix: read `this.state` into a `currentState` local with explicit `as PoolEntryState` cast to re-widen the type. The runtime guard is required (concurrent forceShutdown CAN mutate state across awaits). W43 (critical, race): `runWithTimeout` in `spawnEntry` had `entries.set(id, entry)` + `entry.markActive(...)` INSIDE the timeout-wrapped IIFE. When timeout fired, the catch block deleted the entry and forceShutdown'd it, but the IIFE kept running. If connect/discover settled later, the IIFE's late `entries.set` re-inserted the deleted entry and `markActive` set `state='active'` + `localStatus=CONNECTED` on a transport already disconnected by forceShutdown → zombie entry. Fix: move `entries.set` + `markActive` OUT of the IIFE into the post-await success path. Mirrors `McpClientManager.runWithDiscoveryTimeout`'s `timedOut` flag pattern. W44 (critical, hang): `doRestart` had no wall-clock timeout matching W25's `spawnEntry` fix. A hung MCP server during a restart blocked `restartInFlight` indefinitely; because `restart()` coalesces concurrent callers onto the same promise, every subsequent restart attempt also hung forever and the HTTP route handler never returned. Fix: wrap connect+discover in `runWithTimeout` using the same `discoveryTimeoutFor` resolution. W45 (critical, leak): generation guard + state guard in `doRestart` returned silently without sweeping the new transport spawn. `client.connect()` had already spawned npx/uvx wrapper + MCP grandchild; the OLD transport was disconnected pre-attempt via `sweepAndDisconnect('restart')`, so the new spawn would leak as net-new orphans on both supersede paths. Fix: both guards now call `await this.sweepAndDisconnect('restart_superseded')` before returning. W46 (coverage, deferred): 5 untested new paths flagged. The existing W38/W39/W40 tests (commit `ee3e60af3`) cover incremental discovery + disconnectServer + restart fan-out. The remaining gaps (maxIdleTimer cancellation in doRestart, state guard, sweepAndDisconnect('restart_failed'), runWithTimeout in spawnEntry, hasNameSibling parseConnectionId) need integration tests with fake timers + hung-mock connect — substantially more test infrastructure than the partial-adopt budget for this round. Filing as F2 follow-up. Refactor: `runWithTimeout` + `discoveryTimeoutFor` extracted from mcp-transport-pool.ts into new `mcp-discovery-timeout.ts` so `PoolEntry.doRestart` (W44) can share the primitives without a cross-module value import (which would create a runtime cycle between mcp-pool-entry → mcp-transport-pool). Tests: 189 F2 tests pass; typecheck clean (`npx tsc --noEmit` returns 0 errors). Lint clean. * fix(core): wenshao W51 + W52 review fold-ins for F2 commit 6 — round 7 Two suggestions, both adopted. W52 (semantic): doRestart's generation guard + state guard returned void with debug-level logging. `restart()` resolved successfully → `restartByName` reported `{restarted: true}` to the HTTP API caller even when the restart was effectively aborted. Operators saw "restart succeeded" while sessions silently lost the server. Fix: both guards now `throw new Error(...)` AFTER calling `sweepAndDisconnect('restart_superseded')` (W45 cleanup still happens). `restartByName`'s try/catch translates the throw into `{restarted: false, reason: }` on the HTTP response — the caller now sees an accurate per-entry result. W51 (coverage): added `mcp-discovery-timeout.test.ts` with 14 tests covering both shared primitives. Pre-fix the new `mcp-discovery-timeout.ts` module had ZERO unit tests despite both `spawnEntry` (W25) AND `doRestart` (W44) depending on it for correctness (timeout bounds, clamping, timer cleanup). Tests pin: `discoveryTimeoutFor` stdio default (30s) / remote defaults (httpUrl / url / tcp → 5s) / per-server override clamping to [100ms, 300s] / NaN+Infinity fall through; `runWithTimeout` task resolve-before-timer / timer-before-task / task rejection / clearTimeout on both settlement paths. Tests: 203 F2 tests pass (14 new in mcp-discovery-timeout.test.ts). Typecheck clean. Lint clean. * fix(core): wenshao W61-W76 review fold-ins for F2 commits 5+6 — round 8 Sixteen review findings — 11 adopted as fixes (6 critical bugs + 5 suggestions/improvements), 5 declined-with-reply. W62 (critical, hang): `createUnpooledConnection` had no timeout matching W25/W44. SDK MCP / non-pooled HTTP servers could block `acquire` indefinitely. Fix: wrap connect+discover in `runWithTimeout` using `discoveryTimeoutFor(cfg)`. W63 (critical, race + leak): `drainAll` had three bugs in one block: (1) returned a live `errors` array reference that background `shutdownPromises` could keep mutating; (2) never cleared the timeout timer when `Promise.all` won the race; (3) `forced` count went retroactively negative when late settles pushed into `drained` after the snapshot. Fix: capture lengths synchronously after the race, return `[...errors]` copy, and explicitly `clearTimeout` on both race outcomes. Clamp `forced` to non-negative. W65 (critical, bypass): workspace budget enforcement was bypassed for unpooled HTTP/SSE/SDK-MCP connections — `--mcp-client-budget=2` let 3 HTTP MCP servers connect without refusal. Fix: move the `tryReserve` check BEFORE the `isPoolable` early-return so it applies to both pooled-spawn and unpooled paths. Unpooled entries' close-callback now releases the slot via the same `hasNameSibling`-guarded pattern pooled entries use. W66 (correctness): `applyPrompts` registered ALL prompts unconditionally, ignoring the per-session `excludeTools` / `includeTools` filter that `applyTools` honored. A session restricting tools still received every prompt + the prompt's bound `invoke` closure reaching the same shared `Client` state/credentials as more-trusted siblings. Fix: new `passesSessionPromptFilter` helper applied to each prompt by name. Reuses `excludeTools`/`includeTools` config keys. W68 (defense-in-depth): `restartByName` lacked the `draining` mutex check `acquire()` has. A concurrent restart during `drainAll()` could spawn a fresh subprocess via `client.connect()` that wasn't in drainAll's entry snapshot. Fix: `if (this.draining) return [];` early-out. W69 (correctness): `forceShutdown` set `localStatus = DISCONNECTED` AFTER `await this.sweepAndDisconnect`. During the async yield, `getSnapshot()` still saw `localStatus === CONNECTED` for an entry mid-teardown. Fix: set `localStatus` synchronously alongside `state` at the top of the method (sibling of the C4 fix). W70 (defensive): `emit()` delegated to `EventEmitter.emit` directly, so a synchronous throw from one session's listener would crash the emit call and skip remaining listeners — in `forceShutdown` this meant one buggy listener prevented subprocess cleanup, budget slot release, and entry eviction for ALL sessions sharing the entry. Fix: iterate listeners with per-listener try/catch + debug log on failure. W67 (premature API): `MCPCallInterruptedError` + `onEntryEvent` were exported with zero callers. Removed `onEntryEvent` (was public, no F4 consumer shipping in this PR); `MCPCallInterruptedError` stays per design §13.4 contract for the V21-5 in-flight call interruption follow-up. Re-introduce `onEntryEvent` alongside its first F4 consumer. W72 (correctness, gpt-5.5): pool-mode discovery only updated `McpClientManager.discoveryState` (manager-local), leaving the module-global `mcpDiscoveryState` at `NOT_STARTED`. `GET /workspace/mcp` + MCP preflight cell read the global → reported `not_started` while pool discovery was running or already complete. Fix: new exported `setMCPDiscoveryState(...)` from mcp-client.ts; pool path writes the global at IN_PROGRESS / COMPLETED transitions. W73 (critical, gpt-5.5): `drainAll`'s `Promise.allSettled([...spawnInFlight])` wait was unbounded — a spawn with a large `discoveryTimeoutMs` override could block daemon shutdown for the full discovery timeout BEFORE the 8-10s drain budget began. Fix: race the in-flight wait against the same `timeoutMs` deadline; if it doesn't settle, proceed with whatever entries are visible. W75 (memory leak, gpt-5.5): the `'failed'` event listener wired in `discoverAllMcpToolsViaPool` was anonymous arrow → only removed on `conn.release()`. The `'failed'` branch deleted from `pooledConnections` but never released/unsubscribed; listener stayed attached, pinning manager/connection refs in its closure. Fix: named listener that calls `conn.off('event', ...)` on 'failed' before deleting from the map. Declined with reply (filed as F4 / scope follow-ups): - W61 / W71 (releaseSession wiring on per-session close): the ACP channel has no per-session close notification, so sessions are append-only in `acpAgent.this.sessions` for the daemon's lifetime. Adding session-end hooks needs F4-level lifecycle work; pool entries currently drain en-masse via `drainAll` on daemon shutdown. Filing as F4 follow-up. - W64 (cross-session DoS via restart): per-session ownership checks would change the workspace permission model — currently all authenticated workspace clients are equal (PR 17 contract); adding ownership for restart specifically would be inconsistent with the rest of the workspace mutation surface. Defer to a workspace-policy PR. - W74 (`discoveryTimeoutFor` duplication with manager): refactor to share single source-of-truth touches `McpClientManager` internals; risk of regression in legacy mode. The duplication is acknowledged in the file's own header comment ("Mirrors `McpClientManager.discoveryTimeoutFor` exactly"). Defer. - W76 (entryIndex route tests): cli package's vitest setup requires workspace-linked deps not available locally; same partial-adopt pattern as W22. Tests: 203 F2/SDK tests pass (no new tests this round — fixes only). Typecheck clean. Lint clean. * fix(core): address MCP pool review feedback Co-authored-by: Qwen-Coder * fix(core): gpt-5.5 W77 — cancel in-flight unpooled acquire on session release W77 (gpt-5.5 via Qwen Code /review): `createUnpooledConnection` stored the `unpooled-*` entry in `this.entries` before awaiting `client.connect()` / `client.discover()`, but only called `indexAttach(sessionId, id)` after `entry.attach()` succeeded. If `closeStoredSession()` invoked `releaseSession(sessionId)` during the connect/discover window, `sessionToEntries[sessionId]` was empty — so the in-flight unpooled transport kept spawning and `attach()` later registered tools/prompts into a session that had already been closed. The race is latent today (per-session releaseSession wiring is W61/W71, deferred to F4) but would become live the moment that hook lands. Fix: - `mcp-pool-entry.ts`: add public `isTerminated()` probe and guard `markActive()` against terminal state. Pre-fix, a concurrent `forceShutdown` flipping state→'closed' would be undone by markActive's unconditional `state='active'` assignment, resurrecting a torn-down entry. - `mcp-transport-pool.ts` `createUnpooledConnection`: * call `indexAttach(sessionId, id)` synchronously right after `entries.set(id, entry)`, BEFORE the connect/discover await. * post-await: extend the discard guard with `entry.isTerminated()` to detect a concurrent `releaseSession`→`forceShutdown` that landed during the await, and call `view.teardown()` to roll back the side-effects of the legacy u… * feat(daemon): add shared UI transcript layer (#4328) * feat(daemon): add shared UI transcript layer * fix(daemon): address ui review feedback * test(daemon): cover raw event diagnostics option * fix(daemon): address latest ui review * fix(daemon): cover reconnect and status edge cases * fix(daemon): guard prompt busy cleanup * fix(daemon): handle trimmed tool updates * fix(daemon): cap transcript text blocks * fix(daemon): dedupe trimmed tool diagnostics * fix(daemon): harden webui transcript edge cases * fix(daemon): preserve webui daemon events * fix(daemon): address latest ui review comments * fix(daemon): close latest ui review nits * fix(daemon): harden ui review edges * fix(daemon-ui): address wenshao 2 Critical findings (#4328 review) ## Critical #1 — 401/403 reconnect storm + transcript wipe `DaemonSessionProvider`'s reconnect loop kept retrying `createOrAttach` on 401/403 even with `autoReconnect: true`. Each cycle: - hit the daemon with the same bad token → 401 again - cleared the session handle - the next successful attempt (if token magically recovered) would receive a different sessionId, triggering the `store.reset()` branch at line 143 and wiping the user's transcript - no terminal "auth failed" state surfaced to the user Fix: split `TERMINAL_SESSION_HTTP_STATUSES` into `AUTH_FAILURE_HTTP_STATUSES` (401, 403) and the rest (404, 410). On auth failure, return from the reconnect loop unconditionally regardless of the `autoReconnect` flag — these are credential failures, not transient. The user must update credentials; daemon spam must stop. `extractHttpStatus` helper factored out of `isTerminalSessionHttpError` to share between the two predicates. ## Critical #2 — rawInput / rawOutput leaking secrets to UI `normalizer.normalizeToolUpdate` forwarded `rawInput` / `rawOutput` verbatim onto `DaemonUiToolUpdateEvent` → `DaemonToolTranscriptBlock`. The `details` projection was redacted via `stringifyRedactedJson` / `redactSensitiveFields`, but the underlying `rawInput` / `rawOutput` fields were unredacted. Any UI component that read those fields directly (ShellToolCall, WriteToolCall, JSON debug panels) leaked the raw values to the DOM. Example: `{ command: 'curl', apiKey: 'sk-prod-...' }` had `apiKey` redacted in `details` but exposed verbatim on `rawInput`. Fix: apply `redactSensitiveFields` to both `rawInput` and `rawOutput` ONCE at the normalizer boundary, then reuse the redacted shape for the `details` projection. Downstream is uniformly safe; no double traversal. ## Tests (49/49 pass) - SDK `daemonUi.test.ts` (36 tests, +1) — new test `redacts sensitive fields in tool.update rawInput and rawOutput at normalizer boundary` verifies full-event string scan finds zero secret values + structural keys preserved with values `'[redacted]'`. - WebUI `DaemonSessionProvider.test.tsx` (13 tests, +2) — new tests `breaks out of the reconnect loop on 401 / 403 auth failures even when autoReconnect is true` and `still reconnects on 404 / 410 session-not-found errors when autoReconnect is true` lock in the asymmetry: auth failure → 1 attempt only; session-not-found → retries until success. ## Out of scope (declined / deferred — see PR review reply) - CRIT #3 `withActionTimeout` test coverage gap → behavior correct, test-only follow-up (avoids PR bloat) - Suggestions #4-7 → 4 nice-to-haves, deferred to keep PR focused on production-correctness fixes Generated with AI Co-authored-by: Claude Opus 4.7 * fix(daemon-ui): redact tool details in web transcript * fix(daemon-ui): close review gaps in transcript safety --------- Co-authored-by: 秦奇 Co-authored-by: Claude Opus 4.7 * perf(core): F2 cleanup PR A — R9/W11/W12/R10 (post-merge follow-ups) (#4411) * refactor(core): F2 PR A R9 — McpClientManager options-object ctor R9 (filed as F2 follow-up from #4336 review): 7 positional ctor args collapse to (config, toolRegistry, options?: McpClientManagerOptions). The trailing 5 (eventEmitter, sendSdkMcpMessage, healthConfig, budgetConfig, pool) become named fields on `McpClientManagerOptions`. Test factory `mkManager(overrides?)` introduced at the top of `mcp-client-manager.test.ts` so each of the prior 80 inline constructions becomes a single line naming only the field(s) the test overrides; the 4 `undefined` sentinels each test threaded through to reach the trailing `pool` arg are gone. Net: 113 LOC removed (test) + 35 LOC added (src exposes interface + mkManager factory + tool-registry call site update). Behavior unchanged — same field assignments, same downgrade-enforce-without- budget breadcrumb, same budget event wiring. Filed bucket: F2 perf / cleanup PR A (R9 + W11 + W12 + R10/R23 T7), see issue #4175 item 7 "F2 post-merge cleanup PRs". This is the first of the 4 fixes in PR A; W11/W12/R10 follow as separate commits. Test sweep: 84/84 mcp-client-manager.test.ts pass; typecheck clean. * refactor(core): F2 PR A W11 — extract attachPooledSession + rollbackReservationOnSpawnFailure W11 (filed as F2 follow-up from #4336 review): two private helpers on `McpTransportPool` to eliminate inline duplication in `acquire()`: - `attachPooledSession(entry, id, serverName, cfg, sessionId, toolReg, promptReg)`: builds `SessionMcpView` + `entry.attach` with the standard pool release callback. Used by both the fast-path attach (existing entry) and the post-spawn attach (after `await inFlight`). NOT used by `createUnpooledConnection` — its release callback runs `entry.forceShutdown('manual')` + `indexDetach` directly (no pool refcount accounting since unpooled entries are per-session). - `rollbackReservationOnSpawnFailure(reservationResult, serverName)`: R24 T17 contract — only release the budget slot if THIS acquire actually reserved a new slot (`'reserved'`); `'already_held'` skips because the sibling owns it. Used by both the unpooled catch and the pooled spawn-in-flight catch. Race-window invariants (W10 / W77 / W90 / W111 / W125 / R24 T17) stay at the call sites because they describe the SURROUNDING ordering, not the helpers themselves. Helpers are documented to defer those decisions back to callers. Behavior unchanged. Filed bucket: F2 perf cleanup PR A (R9 done / W11 this commit / W12 + R10 to follow). Test sweep: 28/28 mcp-transport-pool.test.ts pass; typecheck clean. * refactor(core): F2 PR A W12 — SessionMcpView precompute filter Sets W12 (filed as F2 follow-up from #4336 review): `applyTools` / `applyPrompts` precompute `excludeSet` + `includeSet` once per pass instead of scanning `cfg.includeTools` / `cfg.excludeTools` arrays inside every per-tool iteration. Pre-fix the per-tool predicate (`passesSessionFilter`) walked both arrays for every snapshot entry → O(M × N) per `applyTools` call. With M tools × N filter entries, typical M=5-20 / N=2-5 case finishes in microseconds either way; the win is data-structure correctness and code clarity, not perceived perf. `passesSessionFilter` / `passesSessionPromptFilter` (the array- based predicates) stay exported and unchanged for unit tests + any caller wanting to test a single name without paying Set construction. The bulk path uses two new private helpers `compileNameFilter` + `compiledFilterAccepts` whose Sets live on the `applyTools` / `applyPrompts` stack frame. Same semantics: `excludeTools` is direct-equality match (no parens strip — pre-F2 behavior preserved); `includeTools` strips the first `(...)` suffix so `toolName(args)` matches `toolName`. Filed bucket: F2 perf cleanup PR A (R9 + W11 done / W12 this commit / R10 to follow). Test sweep: 13/13 session-mcp-view.test.ts pass; typecheck clean. * perf(core): F2 PR A R10 / R23 T7 — pid-descendants ps snapshot + pgrep fallback R10 / R23 T7 (filed as F2 follow-up from #4336 review): the Linux / macOS pid-descendant enumeration moves from per-pid `pgrep -P ` BFS (one subprocess fork per node visited) to a single `ps -A -o pid=,ppid=` snapshot followed by an in-memory tree walk over `Map`. Windows analog: single `Get-CimInstance Win32_Process | ConvertTo-Csv` snapshot of all `(ProcessId, ParentProcessId)` rows replaces per-pid `Get-CimInstance -Filter "ParentProcessId=$p"` BFS. Two motivations: 1. **Fork count**: typical `npx → tool` / `uvx → tool` wrapper trees are 2-3 levels deep with B=1-3 children per node → pre-fix BFS forked ~5-10 subprocesses per pool-shutdown call. Post-fix: exactly 1 fork regardless of tree depth. 2. **Snapshot consistency**: pre-fix BFS walked the table level by level; a child that forked between two adjacent BFS levels could be missed (we'd see the child but query its descendants AFTER the new fork). The snapshot path captures the table at one instant; new descendants forked after the snapshot are tolerated by the existing ESRCH-tolerant SIGTERM loop. Caveats: - `ps -A -o pid=,ppid=` is POSIX standard (macOS / Linux / *BSD), but BusyBox `ps` ` add. `root` seeded into `visited` so a malformed snapshot listing root as a descendant of its own child doesn't re-enqueue root either. PR-A-R2 #2 (session-mcp-view.ts:117 — predicate dedup): After W12, the exported `passesSessionFilter` / `passesSessionPromptFilter` still called `passesNameFilter` (the pre-W12 array-based implementation), while `applyTools` / `applyPrompts` used `compiledFilterAccepts(compileNameFilter(...))`. Two parallel implementations of the same predicate — future change to one without the other would silently diverge: - the exported function's tests (passesSessionFilter unit tests) would still pass - the production filter path in applyTools/applyPrompts would behave differently Reviewer also noted `passesSessionPromptFilter` had zero callers in production code or tests after W12 — `applyPrompts` no longer references it. Kept the export rather than deleting it (matches the `passesSessionFilter` shape for symmetry + the F3 audit-path comment block earmarks both as the replay predicates), but routed both through `compiledFilterAccepts(compileNameFilter(...))` so there is a single source of truth. Set construction is per-call for these exports (negligible for unit-test / one-off probes); the bulk paths in `applyTools` / `applyPrompts` still construct ONE filter per pass via the original W12 code path. `passesNameFilter` (the standalone array-based helper) deleted — its only callers were the two exports, which now use the compiled path. Public-API surface unchanged: the two exported functions keep their signatures and semantics. Test sweep: 19/19 pid-descendants + session-mcp-view tests pass; typecheck + ESLint clean. Continues commit chain: f05917071 (R9) → 20d2f1b90 (W11) → 6cf18f641 (W12) → 2a41c6fae (R10) → this (R2 followups). * fix(core): F2 PR A R3 T3 — Windows CSV delimiter locale fix `ConvertTo-Csv -NoTypeInformation` honors the system locale's list separator on PowerShell 5.1. On German / French / Dutch / Italian / ... locales the separator is `;` not `,`, so the regex `^"(\d+)","(\d+)"$` in `snapshotProcessTreeWin` never matched → `parsedRows === 0` → snapshot threw → fell back to the per-pid CIM filter path with ~0.5-1s extra PowerShell startup latency per descendant on every pool shutdown. Fix: 1-LOC `-Delimiter ","` on `ConvertTo-Csv`. Forces comma regardless of locale or PowerShell version. PowerShell 7+ defaults to comma already; 5.1 (the Windows-bundled version most users have without explicit upgrade) honored locale. The explicit delimiter makes both consistent. Skipped wenshao's companion Suggestion T4 (test coverage for walkDescendants MAX_DESCENDANTS / MAX_DEPTH caps) as F2 hardening follow-up — the caps are simple 2-line guards exercisable by inspection; ~50 LOC of mock infrastructure isn't commensurate with the regression risk on currently-stable defensive code, and (per the issue #4175 follow-up bucket) we keep dedicated test-coverage work out of perf-cleanup PRs. Continues commit chain: f05917071 (R9) → 20d2f1b90 (W11) → 6cf18f641 (W12) → 2a41c6fae (R10) → ced5d62b0 (R2) → this (R3 T3). Test sweep: 6/6 pid-descendants tests pass; typecheck + ESLint clean. * refactor(acp-bridge): F1 test split — lift bridge.test.ts (6861 LOC) to acp-bridge (#4445) * refactor(acp-bridge): rename httpAcpBridge.test.ts -> bridge.test.ts (git mv) Pure file rename; zero content change. Follow-up commits will: - extract FakeAgent + makeChannel + makeBridge into testUtils.ts - split 4 daemon-host integration tests back to cli/daemonStatusProvider.test.ts Part of #4175 F1 test split (deferred from #4334). * refactor(acp-bridge): extract testUtils + split daemon-host tests to cli (#4175 F1) Net mechanical extraction following commit 2aff1a4d1 (pure git mv of httpAcpBridge.test.ts -> bridge.test.ts). After this commit `@qwen-code/acp-bridge` owns the bulk of the lifted bridge test suite, and cli keeps only the 4 daemon-host integration tests that need to wire `createDaemonStatusProvider()`. Changes: 1. New `packages/acp-bridge/src/internal/testUtils.ts` (~280 LOC): FakeAgent, FakeAgentOpts, ChannelHandle, makeChannel, makeBridge (no statusProvider default — acp-bridge tests exercise the no-provider fallback path), WS_A/WS_B/SESS_A constants. Marked @internal; lives under `internal/` matching the existing `stderrLine.ts` package-private convention. Exposed via new `./internal/testUtils` subpath in package.json exports. 2. `packages/acp-bridge/src/bridge.test.ts` shrinks from 6861 -> ~6400 LOC: fixtures replaced with named imports from `./internal/testUtils.js`; cross-package import `from './daemonStatusProvider.js'` removed (4 daemon-host tests moved out); ACP SDK + bridgeErrors / workspacePaths / bridge / channel / bridgeTypes imports split into multiple statements reflecting actual post-F1 provenance. 3. New `packages/cli/src/serve/daemonStatusProvider.test.ts` (~240 LOC, 4 tests): wires real `createDaemonStatusProvider()` through a cli-side `makeBridge` wrapper to assert end-to-end daemon env / preflight cells. Imports `createHttpAcpBridge` via the `./httpAcpBridge.js` re-export shim — doubles as a shim surface smoke check. Verification: - acp-bridge: 291/291 tests pass (177 in bridge.test.ts). - cli: daemonStatusProvider.test.ts 4/4 pass; full cli suite 6742/6767 green (16 pre-existing failures in AuthDialog / memoryDiagnostics / useAtCompletion — all on `daemon_mode_b_main` baseline, last modified by commits predating this branch). - Tests counts pre-split: 181 in httpAcpBridge.test.ts; post-split: 177 in bridge.test.ts + 4 in daemonStatusProvider.test.ts = 181 (parity preserved). Part of #4175 F1 test split (deferred from #4334). * refactor(acp-bridge): self-review round 1 — vitest alias + doc/comment polish Five code-reviewer findings folded in on top of e97282f30: S1 [Suggestion] — Test-utils ships to npm + cli reads stale dist. Added `packages/cli/vitest.config.ts:resolve.alias` mapping `@qwen-code/acp-bridge/internal/testUtils` → the .ts source. The package subpath export is RETAINED (required for TypeScript `nodenext` to resolve types — it won't fall back to tsconfig paths once exports rejects a subpath). Dual-channel approach documented in the testUtils JSDoc, including the alpha-stage 0.0.1 tradeoff that the file still ships in dist (stripInternal / .npmignore deferred). S2 [Suggestion] — Stale wording "two tests" in narrative comment. bridge.test.ts split-marker now correctly says "4 fallback tests" (no-provider × 2 surfaces + throwing-provider × 2 surfaces). S3 [Suggestion] — "Shim smoke check" only half-applied. daemonStatusProvider.test.ts now routes `BridgeOptions` and `HttpAcpBridge` types through `./httpAcpBridge.js` shim too (alongside `createHttpAcpBridge`), so the entire factory surface the cli tests rely on flows through the F1 re-export shim. N1 [Nit] — Asymmetric split-marker phrasing. Both markers now describe the 4 moved tests by surface (env real / preflight idle / preflight merged-live / preflight extMethod-throws) rather than "1 of" + "3 more". N2 [Nit] — testUtils "the suite" ambiguity. makeChannel JSDoc now references `bridge.test.ts` explicitly instead of "the suite" (which was unambiguous pre-split when helpers + 10 createInMemoryChannel sites lived in the same file). Verification: 291/291 acp-bridge tests pass; 4/4 cli daemon integration tests pass; tsc clean on both packages (pre-existing server.ts errors on baseline unchanged); eslint --max-warnings 0 clean on all 4 touched files. * docs(cli): self-review round 2 — fix stale vitest.config.ts alias comment Round 2 reviewer caught a 3-way contradiction in the round 1 docs: - vitest.config.ts said: alias replaces the export, internal/* stays unpublished (matches stderrLine convention). - package.json: subpath export IS declared. - testUtils.ts JSDoc: both channels intentionally retained, testUtils ships in dist. Round 1 explicitly chose to retain the export because TS `nodenext` won't fall back to tsconfig `paths` once `exports` rejects a subpath; the alias only serves to short-circuit *runtime* resolution so cli reads src/ not dist/. Rewriting the vitest.config.ts comment to reflect that dual-channel reality (and pointing readers at testUtils.ts for the full rationale). * fix(acp-bridge): #4445 round 3 fold-in — 4 of 7 reviewer threads adopted PR #4445 review pass — 4 adopt + 3 decline (declines replied inline; not folded here): ADOPTED: T1 [copilot daemonStatusProvider.test.ts:136 — bridge.shutdown missing]: added `await bridge.shutdown()` to test 2 (preflight idle). Three of four tests already shut down; symmetry + future-proof if `createHttpAcpBridge` gains background work even when no channel was spawned. T5 [wenshao testUtils.ts:92 — makeBridge naming collision]: cli- side helper renamed `makeBridge` -> `makeBridgeWithDaemonStatusProvider` (4 call sites in daemonStatusProvider.test.ts), JSDoc updated to reference the wenshao thread. testUtils.makeBridge stays as the canonical name used by ~100 tests in bridge.test.ts. A future contributor can no longer pick the wrong helper by accident. T6 [wenshao testUtils.ts:32 — JSDoc mis-claims @internal tag matches stderrLine.ts convention]: fixed wording. stderrLine.ts uses prose only; @internal is an additional package-private signal, not a convention match. Also restructured the npm-leak paragraph to describe the new .npmignore-via-files-negation enforcement (T7). T7 [wenshao package.json:70 — testUtils ships to npm]: switched `files: ["dist"]` -> `files: ["dist", "!dist/internal/testUtils.*", "!dist/**/*.test.*"]`. Wenshao's suggested `"test"` exports condition wasn't viable: vitest sets `vitest` not `test`, and gating on `vitest` would hide types from the cli's tsc compile. The negation-pattern files-field excludes the built testUtils from the publish surface while keeping the subpath export entry that TypeScript `nodenext` needs to resolve types. Verified via `npm pack --dry-run`: dist/internal/stderrLine.* still ships (production internal helper); dist/internal/testUtils.* + dist/**/*.test.* are excluded. DECLINED (replied on PR threads, not folded here): T2/T3 [copilot — `handles` array unused in tests 3/4]: bookkeeping matches the pre-split bridge.test.ts verbatim; cleanup is scope creep on this rename PR. T4 [copilot — testUtils eager-imports createHttpAcpBridge, cross-copy identity risk]: cli daemonStatusProvider.test.ts uses its OWN local `makeBridgeWithDaemonStatusProvider` and never imports testUtils.makeBridge — the cross-copy concern isn't triggered. Premature abstraction on a test-only fixture. Verification: 291/291 acp-bridge tests pass; 4/4 cli daemon tests pass; tsc clean both packages; eslint --max-warnings 0 clean on 2 touched .ts files; `npm pack --dry-run` confirms publish-surface exclusions. * fix(core): F2 cleanup PR B — self-heal observability (W133-a + W134) (#4460) * fix(core): F2 cleanup PR B — self-heal observability (W133-a + W134) W93 declined as already satisfied by W1 fix in #4336 commit 6 (spawnEntry's catch already calls forceShutdown which runs the full cleanup table — listener removal, timer clear, subscriber detach, sweep+disconnect, onClosed eviction). Source-verified non-repro. W133-a: McpClient.onerror now captures the error in a private `lastTransportError` field (reset at each connect()); the W120 silent-drop block at mcp-pool-entry.ts:346 reads it via the new `getLastTransportError()` getter and appends `: ` to the lastError string on the emitted 'failed' event. Preserves the literal "silent transport drop" prefix invariant for log-grep backward compat — pre-fix marker stays a substring. W134: sweepAndDisconnect now returns SweepResult instead of void — { pidSweepError?, disconnectError?, descendantsFound?, descendantsSignaled? }. The silent-drop fire-and-forget caller chains to inspect the result and emits a structured warn log when either pid-sweep threw OR sigtermPids partially signaled (signaled < found) — surfaces orphan-process pressure without inflating PR scope (no new SSE event or SDK reducer state; deferred to W134-followup if maintainers want metrics). forceShutdown / doRestart sweep callers ignore the return value (JS implicit-void at await sites preserves behavior). 4 new tests in mcp-transport-pool.test.ts covering W133-a happy path + fallback (no prior onerror) + W134 pidSweepError + W134 partial-signal failure modes. Module-mocks pid-descendants.js for controllable sweep behavior, and debugLogger.js to observe warn calls (production logger is session-gated and a no-op in tests). Singleton-stub debugLogger mock so production module-load `createDebugLogger('McpPool:Entry')` and the test's retrieval get the same vi.fn instances. Verification: - tsc clean: packages/core, packages/cli (server.ts pre-existing errors unchanged) - F2 transport-pool: 32/32 pass (28 pre-existing + 4 new) - mcp-client: 46/46 pass - eslint --max-warnings 0 clean on 3 touched files Part of #4175 #4336 follow-up bucket. * fix(core): #4460 round 1 fold-in — 4 copilot doc/comment threads adopted T1 [copilot mcp-pool-entry.ts:116 — stale line ref in SweepResult JSDoc]: replaced `mcp-pool-entry.ts:383` with stable method-anchor reference to the W120 silent-drop block inside `statusChangeListener`. Line numbers drift on every edit; method names don't. T2 [copilot mcp-pool-entry.ts:453 — `?? 0` ambiguous in warn payload]: silent-drop warn log now prints `descendantsFound=unknown` and `descendantsSignaled=unknown` when the values are undefined (only reachable in the pidSweepError branch — sweep threw before assignment). Operators triaging the warn can now distinguish "sweep succeeded but found 0 descendants" from "sweep itself threw, count is genuinely unmeasured". Locked in via a new assertion in the W134 pidSweepError test. T3 [copilot mcp-client.ts:116 — brittle line refs in lastTransportError JSDoc]: replaced `mcp-pool-entry.ts:346` and `mcp-client.ts:130` with stable method/block names (the `statusChangeListener` silent- drop block; the `client.onerror` arrow inside connect()). Same fix applied to the parallel comment in mcp-transport-pool.test.ts:730 for consistency. T4 [copilot mcp-transport-pool.test.ts:797 — singleton-stub mock comment contradictory]: rewrote the comment to unambiguously describe what the mock DOES (factory body runs once; inner arrow returns the same object on every call) instead of the prior hypothetical phrasing ("Returning a fresh object would have...") which read as a description of current behavior at first glance. All 4 are doc/comment fixes — zero behavior change apart from the T2 string format ('unknown' instead of '0'). Verified: - 32/32 mcp-transport-pool.test.ts pass - tsc clean on packages/core - eslint --max-warnings 0 clean on 3 touched files * fix(core): #4460 round 2 fold-in — remove dead SweepResult.disconnectError field T5 [wenshao mcp-pool-entry.ts:134 — `disconnectError` is dead data]: glm-5.1 review caught that the field was populated when `client.disconnect()` threw (line 844) but no consumer ever read it — the silent-drop `.then()` handler gated only on `pidSweepError` and partial-signal; `forceShutdown` and `doRestart` ignore the return; no test asserted on it. Removed the field from `SweepResult` and the assignment in the disconnect catch. The pre-existing `debugLogger.error(`client.disconnect failed for ...`)` inside `sweepAndDisconnect` already gives operators the signal — adding it to the outer silent-drop warn would have been duplicate noise. If a future consumer needs to gate logic on disconnect failures, re-add the field + reader at that point. Verification: 32/32 mcp-transport-pool.test.ts pass; tsc + eslint clean on the touched file. * feat(sdk/daemon-ui): unified completeness follow-up to #4328 (#4353) * feat(sdk/daemon-ui): expand event coverage to 28+ daemon event types (PR-A) Closes the "12+ daemon events fall through to debug" gap surfaced in the PR the daemon currently emits (Stage 1 + Wave 3-4), so renderers stop having to peek at `rawEvent.data` for known event categories. Session-meta: - session.metadata.changed (from session_metadata_updated) - session.approval_mode.changed (from approval_mode_changed) - session.available_commands (from available_commands_update; upgraded from a status-text fallback to a typed event carrying the command list) Workspace state (Wave 3-4): - workspace.memory.changed - workspace.agent.changed - workspace.tool.toggled - workspace.initialized - workspace.mcp.budget_warning - workspace.mcp.child_refused - workspace.mcp.server_restarted - workspace.mcp.server_restart_refused Auth device-flow (Wave 4 OAuth, RFC 8628): - auth.device_flow.started - auth.device_flow.throttled - auth.device_flow.authorized - auth.device_flow.failed (carries DaemonAuthDeviceFlowSdkErrorKind) - auth.device_flow.cancelled - `DaemonUiErrorEvent.errorKind?: DaemonErrorKind` — closed-enum error category propagated from daemon's typed-error taxonomy. Renderers can branch on errorKind for "retry auth" vs "check file path" affordances instead of regex-matching `text`. - `DaemonUiToolUpdateEvent.provenance?: DaemonUiToolProvenance` + `.serverId?` — closed enum ('builtin' | 'mcp' | 'subagent' | 'unknown'). Falls back to the `mcp____` naming heuristic when the daemon doesn't stamp provenance explicitly. Unblocks UI namespace dispatch without string-matching toolName. Session-meta / workspace / auth events do NOT push transcript blocks. They are intentional sidechannel observations: `lastEventId` advances (monotonic invariant preserved), but the chat-stream transcript stays focused on user/assistant/tool/shell/permission content. Renderers consume them via selectors (introduced in follow-up PRs). All new event types produce short structured lines in `daemonUiEventToTerminalText` for tail-style debug consumers. Web/IDE renderers should consume the typed events directly via subscription. 40/40 tests pass. New tests verify: - All 16 new event types normalize correctly - Malformed payloads fall back to debug without leaking raw data (`secret` field never appears in fallback text) - MCP tool provenance heuristic (`mcp__github__create_issue` → provenance='mcp', serverId='github') - errorKind propagation on session_died / stream_error - Reducer is no-op on new event types; lastEventId still advances This is PR-A of the unified-renderer-layer follow-up series: - PR-A (this commit) — event coverage + closed-enum schema - PR-B — server-side timestamps + ordering refactor - PR-C — multimodal content + tool preview taxonomy - PR-D — render contract (toMarkdown / toHtml / toPlainText) + adapter conformance test framework - PR-E — reducer state machine (subagent / progress / current tool / cancellation propagation) See https://github.com/QwenLM/qwen-code/pull/4328#issuecomment-4494179724 for the full proposal. Generated with AI Co-authored-by: Claude Opus 4.7 * feat(sdk/daemon-ui): server timestamps + event-id-based ordering (PR-B) Closes the "时间定义不标准" gap surfaced in the PR #4328 review: - Client-side `Date.now()` drifts across clients - No daemon-authoritative timestamp propagated to UI - Out-of-order replay events get fresher `state.now` than originals, breaking `createdAt` ordering - `DaemonUiEventBase.serverTimestamp?: number` — daemon-authoritative wall-clock timestamp extracted from envelope. - `DaemonTranscriptBlockBase.serverTimestamp?: number` + `clientReceivedAt: number`. - `createdAt` preserved as `@deprecated` alias for `clientReceivedAt` (backward compat for code written before this PR). `extractServerTimestamp` looks at three candidate envelope locations: 1. `event.serverTimestamp` (preferred when daemon adds it) 2. `event._meta.serverTimestamp` (Anthropic-style metadata convention) 3. `event.data._meta.serverTimestamp` (sessionUpdate nested location) The SDK is ready to consume serverTimestamp WHEN daemon emits it, without requiring a coordinated SDK release. Undefined when daemon doesn't emit (current state) — graceful degradation to client-clock ordering. `selectTranscriptBlocksOrderedByEventId(state)` — returns blocks sorted by: 1. `eventId` (daemon-monotonic SSE cursor) — primary key 2. `serverTimestamp` (daemon wall clock) — fallback for synthetic frames 3. `clientReceivedAt` (local clock) — last resort Use this when displaying long sessions where event id 5 may arrive AFTER event id 7 (typical in SSE replay-after-reconnect). `formatBlockTimestamp(block, opts)` — formats the most authoritative timestamp on a block using `Intl.DateTimeFormat`. Prefers `serverTimestamp` over `clientReceivedAt` for cross-client consistency. Accepts locale / timeZone / dateStyle / timeStyle. Daemon needs to stamp `_meta.serverTimestamp` on every SSE envelope. This SDK PR is ready to consume it the moment the daemon ships the field; no coordination needed. - serverTimestamp extraction from all three envelope locations - Defaults undefined when envelope has none - `selectTranscriptBlocksOrderedByEventId` sorts mixed-arrival events by eventId (replay scenario) - `formatBlockTimestamp` prefers serverTimestamp; returns localized string PR-B of the unified follow-up to PR #4328 (PR-A + PR-B + PR-C + PR-D + PR-E in one branch). Generated with AI Co-authored-by: Claude Opus 4.7 * feat(sdk/daemon-ui): reducer state machine — currentTool / approvalMode / cancellation propagation (PR-E) Closes the "reducer state machine 设计缺漏" gap surfaced in the PR #4328 review: - No `currentTool` — UI scans `blocks[]` to find the running tool - No mirrored approval mode — UI walks events to badge "plan"/"yolo" - Cancellation does not propagate — in-flight tool blocks stuck at 'in_progress' forever when the parent prompt is cancelled ## State additions (sidechannel, no transcript blocks) `DaemonTranscriptSidechannelState`: - `currentToolCallId?: string` — toolCallId of the in-flight tool - `approvalMode?: string` — mirrored from session.approval_mode.changed - `toolProgress: Record` — per-tool progress shape (daemon-side emission of `tool.progress` events pending) ## Reducer behavior ### `tool.update` events `IN_FLIGHT_TOOL_STATUSES` = { pending, confirming, running, in_progress } `TERMINAL_TOOL_STATUSES` = { completed, success, failed, error, canceled, cancelled } - Tool enters in-flight: set `currentToolCallId = event.toolCallId` - Tool enters terminal: clear `currentToolCallId` if it matches - Unknown status (forward-compat): leave pointer untouched This avoids the failure mode where a future daemon-emitted status like `'paused'` would silently mark unknown states as either in-flight or terminal incorrectly. ### `session.approval_mode.changed` Mirror `event.next` onto `state.approvalMode`. Renderers can render a mode badge ("plan" / "default" / "auto-edit" / "yolo") with a single selector call, no event-stream walking. ### `assistant.done` with `reason === 'cancelled'` `propagateCancellationToInFlightTools` walks every tool block whose status is still in-flight and force-sets it to 'cancelled'. The daemon does not guarantee terminal `tool_call_update` for every in-flight tool when the parent prompt is cancelled, so this propagation prevents UI spinners from spinning forever. `currentToolCallId` is also cleared in the same call. Non-cancellation `assistant.done` (e.g., `reason: 'end_turn'`) does NOT propagate — in-flight tools remain in-flight until the daemon emits their terminal update naturally. ## Selectors - `selectCurrentTool(state)` — returns the running tool block, or undefined - `selectApprovalMode(state)` — returns the mirrored approval mode - `selectToolProgress(state, toolCallId)` — per-tool progress query All exported from `@qwen-code/sdk/daemon`. ## Scope deliberately deferred Subagent nesting (`parentBlockId` / `delegationId` / `DaemonSubagentTranscriptBlock`) is NOT in this PR. The shape needs design discussion (how to project nested events; whether to bake delegation tracking into transcript or sidechannel). PR-D / PR-F follow-up. ## Test coverage (51/51 pass) - currentToolCallId set on enter, cleared on terminal - approvalMode mirrors changes - Cancellation marks in-flight tools 'cancelled', leaves completed alone - Unknown status does NOT clear currentToolCallId (forward-compat) - Non-cancellation `assistant.done` does NOT propagate ## Roadmap PR-E of the unified follow-up to PR #4328 (PR-A + PR-B + PR-E in this branch; PR-C / PR-D pending). Generated with AI Co-authored-by: Claude Opus 4.7 * feat(sdk/daemon-ui): tool preview taxonomy + multimodal content extraction (PR-C) Closes two related gaps surfaced in the PR #4328 review: - `DaemonToolPreview` had only 4 kinds — UI fell back to `key_value` / `generic` for tools that deserved structured display - `getTextContent` silently dropped non-text content (image / audio / resource), so multimodal conversations vanished from the UI `DaemonToolPreview` extends from 4 to 8 variants: - `file_diff` — `{ path, oldText?, newText?, patch? }` — file edit tools (Anthropic-style `oldText/newText`, aider-style `patch`, write-style `newText` alone) - `file_read` — `{ path, range?: [start, end] }` — file read tools, with range extracted from `lineRange` tuple OR `offset/limit` pair - `web_fetch` — `{ url, method? }` — HTTP fetch tools (requires URL with scheme to avoid false positives on relative paths) - `mcp_invocation` — `{ serverId, toolName, argsSummary? }` — MCP server tool calls, identified via `mcp____` naming convention (same heuristic as PR-A `DaemonUiToolUpdateEvent.provenance`) Detector order matters — MCP wins first (most specific), then file_diff, file_read, web_fetch, then the existing command / key_value fallbacks. New helper `extractContentPart(value): DaemonUiContentPart | undefined` returns a discriminated union: ```ts type DaemonUiContentPart = | { kind: 'text'; text: string } | { kind: 'image'; mediaType: string; source: { url?, data? } } | { kind: 'audio'; mediaType: string; source: { url?, data? } } | { kind: 'resource'; uri: string; mediaType?, description? }; ``` The existing `getTextContent` is preserved for backward compat. Renderers that need to surface non-text content (web UI thumbnails, IDE attachment chips) now have a typed shape to consume. - Wiring `extractContentPart` into the normalizer / reducer so text blocks accumulate `parts: DaemonUiContentPart[]` alongside `text` (additive shape change requires render contract coordination — PR-D). - 5 additional tool preview kinds (image_generation / code_block / tabular / subagent_delegation / search) — useful but not urgent; current 8 kinds cover the typical agent flows. - file_diff detection from Anthropic / aider / write shapes - file_read with lineRange tuple AND offset+limit pair - web_fetch with method, REJECTS relative paths (no scheme) - mcp_invocation with serverId + toolName extraction - Detector priority: MCP wins over file_diff on conflicting shapes - extractContentPart for text / image (url) / audio (data) / resource - Unknown content type returns undefined (skip rather than synthesize) - Image without source returns undefined (defensive) PR-C of the unified follow-up to PR #4328 (PR-A + PR-B + PR-E + PR-C in this branch; PR-D render contract pending). Generated with AI Co-authored-by: Claude Opus 4.7 * feat(sdk/daemon-ui): render contract — markdown / HTML / plain text helpers (PR-D) Closes the "render 契约只覆盖 terminal" gap surfaced in the PR #4328 review: > PR ships `daemonUiEventToTerminalText` for terminal. Web/IDE/channel > adapters each roll their own projection. No shared contract → adapter > divergence is inevitable. ## New helpers ```ts daemonBlockToMarkdown(block, opts?): string // GFM-compatible daemonBlockToHtml(block, opts?): string // conservatively escaped HTML daemonBlockToPlainText(block, opts?): string // for copy-paste / logs daemonToolPreviewToMarkdown(preview, opts?): string ``` All three respect the same `kind` discrimination so adapters can switch between them without touching call sites. ## Per-kind projection For each `DaemonTranscriptBlock['kind']`: - `user` / `assistant` / `thought` — plain text with role labels - `tool` — header with toolName + structured preview + status badge - `shell` — fenced code block, stream-discriminated (stdout vs stderr) - `permission` — title + options list + resolved/pending indicator - `status` / `debug` / `error` — semantic class / role (error → role=alert) For each `DaemonToolPreview['kind']`: - `ask_user_question` — question + options as bullet list - `command` — fenced bash with optional cwd comment - `file_diff` — unified diff in fenced code block (oldText/newText OR patch) - `file_read` — `path (lines N-M)` line - `web_fetch` — `METHOD url` line - `mcp_invocation` — `serverId::toolName` with args summary - `key_value` — bullet list - `generic` — emphasized summary ## Security - Default HTML sanitizer escapes `<`, `>`, `&`, `"`, `'` and FIRST strips ANSI/control sequences via `sanitizeTerminalText` (defense against agent-emitted escape codes in HTML output). - Custom sanitizer hook for consumers wanting markdown→HTML pipelines (markdown-it + DOMPurify, etc.). - `sanitizeUrls` option strips token-like query params (`token=`, `key=`, `x-amz-`, etc.) from URLs in `web_fetch` previews. - `maxFieldLength` truncation defaults 8192, prevents pathological rendering on huge content. ## Adapter conformance (out of scope for this commit) The conformance test framework (fixture corpus + `runAdapterConformanceSuite`) mentioned in PR-D scope is deferred to a follow-up. The render helpers here are the precondition — once stable, the conformance framework can use them as the reference projection. ## Test coverage (77/77 pass) - All 9 block kinds render in markdown (verified for user/assistant/tool/ shell/permission/error specifically) - file_diff renders as unified diff with old/new lines - mcp_invocation renders as `server::tool` format - HTML escapes XSS (`` pass the protocol check. Modern browsers don't execute ``, but the comment claimed "never legitimate in ``" which slightly over-claimed the protection. Tighten the data: branch to require an `image/` MIME prefix. Verified by a new test that covers: https (allow), data:image/png (allow), data:text/html (reject → '#'), javascript: (reject → '#'). Generated with AI Co-authored-by: Claude Opus 4.7 * fix(daemon-ui): wenshao + doudouOUC R4 review batch Walks 6 wenshao items (delivered as 8 review submissions — 2 CHANGES_REQUESTED + 6 individual COMMENTED — but 6 distinct concerns) and 3 doudouOUC R4 nits. All 9 real issues addressed; no false-positives this round. ## Real Criticals ### awaitingResync recovery API (wenshao R4) `store.reset()` requires session-id change semantics — wrong shape for "same-session reconnect with SSE replay" recovery. Added explicit `store.clearAwaitingResync()` API. Latch is still set on receipt of `session.state_resync_required` (intentional one-way during replay window); consumers now have a clean path to clear after the replay stream drains. ### normalizeAuthDeviceFlowCancelled test coverage (wenshao R4) Coverage gap surfaced — happy path (valid deviceFlowId) and malformed fallback to debug both untested. Added 2 tests. ## Real Suggestions ### sanitizeUrl: AWS / Azure / GCP credential patterns The previous regex caught `x-amz-` and `x-goog-` headers + generic `signature` / `sig`, but missed: - `AWSAccessKeyId` (S3 presigned) - Azure SAS short codes (`sv` / `se` / `sr` / `sp` / `st` / `spr` / `sip` / `ss` / `srt` / `sig` / `skoid` / etc.) - GCP signed-URL `GoogleAccessId` + `Expires` (paired with credentials in signed URL contexts) Widened regex to include `aws|google|expires` prefixes + added explicit Azure-SAS Set check. ### detectFileDiff: `content` alias disambiguated `{ path, content }` was being classified as `file_diff` regardless of tool semantics — but the same shape is common for file_read assertions or search queries. Since detectFileDiff runs BEFORE detectFileRead in the detector chain, this caused mis-classification. Fix: restrict bare `content` to require either (a) write-intent tool name (write/create/edit/replace/save/update) OR (b) co-occurrence with `oldText`. Explicit `newText` / `new_text` / etc. still pass through unconditionally. Required adding `opts` to the `detectFileDiff` signature (callers already pass opts to siblings). ### detectFileRead: 0-based offset → 1-based range Type doc says `range: [startLine, endLine]` is 1-based inclusive. The offset+limit conversion produced 0-based output ([0, 9] for offset=0/limit=10), which displayed as "lines 0-9" — line 0 doesn't exist in 1-based. Convert at the detector: `[offset+1, offset+limit]`. Updated the matching test (which had encoded the 0-based bug as expected behavior). ### formatMissedRange — guard inverted / single-event ranges The naive `lastDeliveredId+1 .. earliestAvailableId-1` formula produced: - `gap === 0`: "missed 6-5" (inverted) - `gap === 1`: "missed 6-6" (single event shown as range) Added `formatMissedRange()` helper with explicit branches: - `last < first` → "no events lost (resync requested without gap)" - `last === first` → "missed 1 daemon event (id N)" - `last > first` → "missed daemon events X-Y" Applied in both `transcript.ts` (status block message) and `terminal.ts` (ANSI projection) — same formula was duplicated. ## doudouOUC R4 nits ### README errorKind list outdated Replaced `expired / transport / server / internal` with pointer to `KNOWN_DEVICE_FLOW_ERROR_KINDS` exported constant — canonical list auto-stays-in-sync. ### README "10 scenarios" stale Was 10, became 11 with subagent-nesting. Removed the count and let the corpus be derived at runtime via `DAEMON_UI_CONFORMANCE_FIXTURES.length`. ### selectTranscriptBlocks danger post lazy-COW With state.blocks now shared across sidechannel snapshots, a misbehaving consumer doing `(state.blocks as DaemonTranscriptBlock[]).sort()` would poison every snapshot sharing the reference. Freeze the blocks array at the dispatch boundary in `reduceDaemonTranscriptEvents`. Internal reducer mutation goes through `takeBlocksOwnership` which copies before mutating, so the frozen reference is never modified in place. ## Validation | | | |---|---| | SDK tests | **162/162** | | WebUI tests | **9/9** | | SDK typecheck | clean | | WebUI typecheck | clean | Generated with AI Co-authored-by: Claude Opus 4.7 * fix(daemon-ui): wenshao R5 review batch — Critical OAuth fragment leak + 10 more Walks 13 inline items from wenshao's 16:46-17:28 reviews. 11 fixed, 1 deduped (lint-no-console flagged in both reviews), 1 reverted/push-back (multi-part deny re-flags the same design-intent territory as R2 #4). ## Critical fixes ### sanitizeUrl: OAuth #fragment leak `sanitizeUrl` cleared query params and Basic Auth userinfo, but `u.toString()` preserved `u.hash`. OAuth 2.0 implicit grant puts `access_token=...` directly in the fragment (e.g., `https://app/#access_token=gho_xxx&token_type=bearer`); some Azure SAS variants similarly. Now `u.hash = ''` before serialize. For rendered output (markdown / HTML / plaintext), the fragment is client- state-only and dropping it removes the entire fragment-side leak surface. ### ESLint no-console on awaitingResync diagnostic Project lint forbids bare `console.*`. Added `eslint-disable-next-line no-console -- intentional diagnostic` per wenshao's suggestion. Behavior unchanged. ### normalizeAuthDeviceFlowCancelled test coverage (still missing post-R4) R4 added tests for one of the five device-flow normalizers; the `cancelled` variant was still uncovered. Added happy + malformed-payload tests. ## Behavior fixes ### Plaintext sanitizeTerminalText parity `daemonBlockToPlainText` + `daemonToolPreviewToPlainText` previously returned ANSI/bidi-control text verbatim, while markdown and HTML paths sanitized via `sanitizeTerminalText`. A daemon emitting bidi overrides survived clean to plaintext output — contradicting the "copy-paste / logs" JSDoc intent. Now routes every text field through `clean()` = `cap(sanitizeTerminalText(raw))`. ### blockquote helper applied to image_generation + subagent_delegation R3 added the helper for thought/debug/error but missed two preview markdown sites (`> ${text(preview.prompt)}` for image_generation, `> ${text(preview.task)}` for subagent_delegation). Multi-line prompts / tasks now stay inside the blockquote. ### Default unrecognized-event branch: single debug block Was emitting `status + debug` (2 blocks) per unknown event type. In long sessions where the daemon adds new types an older SDK doesn't recognize, this doubled block-consumption rate and accelerated `maxBlocks` trimming of real content. Now emit a single `debug` block that prefixes the event-type for adapters that want to pattern-match. ### writeIntent regex underscore-boundary aware R4's `content` alias gate-check used `\b` word boundaries, but `\b` doesn't match between `write` and `_` in `write_file` (both `\w`). Fixed to `(?:^|[_-])verb(?:$|[_-])` which catches the canonical `write_file` naming AND still rejects `prewrite_check`. Verb list extended per wenshao's suggestion (`overwrite`/`modify`/`patch`/`generate`). ### useDaemonPendingPermissions over-subscription Hook used `useDaemonTranscriptState()` which fires on every daemon event (text deltas, tool updates, sidechannel). Switched to `useDaemonTranscriptBlocks()` which only invalidates when the blocks array reference changes — block-mutating dispatches only, thanks to lazy COW. Same selector semantics, ~10x fewer renders in chat-heavy sessions. ### Conformance suite: try/catch adapter JSDoc promised "does not throw" but the loop wrapped adapter calls without try/catch. Buggy adapters aborted the whole suite instead of producing a structured `ConformanceFailure`. Now wrap; on throw, capture the error message in `renderedExcerpt: "[adapter threw: ...]"` and continue. ## Type / Quality fixes ### DaemonTranscriptState.blocks typed readonly Runtime contract is frozen (lazy-COW poison defense), but the type was mutable — consumers got runtime `TypeError` for in-place mutation instead of compile errors. Now `readonly DaemonTranscriptBlock[]` so mutation is caught at the type level. ### formatMissedRange exported / deduplicated Helper was duplicated inline between transcript.ts (full phrasing) and terminal.ts (terser phrasing). Exported from transcript.ts and reused in terminal.ts to prevent future drift. ## Push-back (false-positive — see reply) ### classifySelectedPermissionOption multi-part deny (`selected:deny:access_violation`) Re-flags the same `selected:X` design intent rejected in R2 #4. The caller comment explicitly states a selected option resolves the prompt even when the option id contains `deny`/`cancel`. The existing test `cancelled-substring-permission` (payload `selected:abort`, expected `completed`) codifies this. Daemon expresses true user-cancellation via the `cancelled` PRIMARY token, not `selected:cancel`. Not changing; reply directs to the same R2 #4 reasoning. ## Tests added (+10) - normalizeAuthDeviceFlowCancelled happy + malformed - sanitizeUrl OAuth fragment access_token rejected - sanitizeUrl AWS/GCP/Azure SAS credential params stripped - formatMissedRange no-gap / single-event / multi-event - detectFileDiff content alias rejected for read-like tools - detectFileDiff content alias accepted for write-like tools - writeIntent word boundaries (prewrite_check NOT matched) - conformance captures adapter throw - unrecognized event → single debug block - store.clearAwaitingResync clears latch ## Validation | | | |---|---| | SDK tests | **172/172** (was 162, +10) | | WebUI tests | **9/9** | | SDK typecheck | clean | | WebUI typecheck | clean | Generated with AI Co-authored-by: Claude Opus 4.7 * fix(daemon-ui): wenshao R6 — recovery flow chicken-and-egg + pending pointer Three Criticals from R6 review (4351217188) all pointing at real bugs introduced by R4/R5 work — not false positives. Fixes plus regression tests. ## Critical 1 — same-session reconnect never clears the latch When the daemon emitted `state_resync_required`, the reducer set `awaitingResync = true`. The webui provider dispatched `assistant.done { reason: 'reconnected' }` after re-attaching SSE but never called `store.clearAwaitingResync()`. Result: events flowed in on the fresh stream but every one got dropped by the `applyDaemonTranscriptEvent` passthrough guard. Transcript appeared permanently frozen with no diagnostic clue (the `console.warn` fired on each drop, but the user wouldn't necessarily check DevTools). Fix: in `DaemonSessionProvider.tsx`, after dispatching the synthetic `reconnected` `assistant.done`, check `awaitingResync` and clear it BEFORE the new SSE event loop starts. ## Critical 2 — updateCurrentToolPointer breaks on undefined status In `upsertToolBlock`, a new tool block is created with `status: event.status ?? 'pending'`. But `updateCurrentToolPointer` was called with raw `event.status` — when undefined, the function's own `if (status === undefined) return;` guard short-circuited without ever pointing at the new (visually-pending) block. Result: `selectCurrentTool` returned `undefined` for daemon events that omitted the explicit `status` field, while the block sat at "pending" in the UI — invisible to the current-tool selector. Fix: pass the EFFECTIVE status (`event.status ?? 'pending'`) so the pointer logic mirrors the actual stored status. ## Critical 3 — clearAwaitingResync flow chicken-and-egg The earlier (R4) JSDoc documented the recovery flow as: "re-subscribe with `Last-Event-ID: 0`, then call clearAwaitingResync after replay drains." But while the latch is true, EVERY non-passthrough event is dropped at `applyDaemonTranscriptEvent`. So during the replay drain, zero events made it into state, and clearing the latch afterward did nothing — transcript permanently empty. Correct flow: clear FIRST, then stream events. Updated JSDoc on both `types.ts` interface and `store.ts` impl to document this clearly. Added a regression test (`clearAwaitingResync AFTER dispatching events: events ARE dropped`) that pins the correct flow in code. ## Regression tests (+3) - `undefined status` creates pending block AND sets currentToolCallId - clear-then-dispatch ✓ events flow - dispatch-then-clear ✗ events dropped (correct flow documentation) ## Validation | | | |---|---| | SDK tests | **175/175** (was 172, +3) | | WebUI tests | **9/9** | | SDK typecheck | clean | | WebUI typecheck | clean | ## Note on doudouOUC heads-up #4469 (main → daemon_mode_b_main sync, 45 commits since 2026-05-19) will land soon. doudouOUC's note says rebase should be smooth (no daemon-ui surface conflicts). Will rebase on the cron's next pass after #4469 merges. Generated with AI Co-authored-by: Claude Opus 4.7 * fix(daemon-ui): wenshao R7 — escapeMarkdownText covers `<` + details URL sanitization Two items from wenshao R7 (one inline Suggestion + one Verification-PASS finding). Both gate-checked as real; fixed. ## escapeMarkdownText: add `<` to escape set Markdown rendered through markdown-it with `html: true` would previously pass through raw `` / `` pass the protocol check. Modern browsers don't execute ``, but the comment claimed "never legitimate in ``" which slightly over-claimed the protection. Tighten the data: branch to require an `image/` MIME prefix. Verified by a new test that covers: https (allow), data:image/png (allow), data:text/html (reject → '#'), javascript: (reject → '#'). Generated with AI Co-authored-by: Claude Opus 4.7 * fix(daemon-ui): wenshao + doudouOUC R4 review batch Walks 6 wenshao items (delivered as 8 review submissions — 2 CHANGES_REQUESTED + 6 individual COMMENTED — but 6 distinct concerns) and 3 doudouOUC R4 nits. All 9 real issues addressed; no false-positives this round. ## Real Criticals ### awaitingResync recovery API (wenshao R4) `store.reset()` requires session-id change semantics — wrong shape for "same-session reconnect with SSE replay" recovery. Added explicit `store.clearAwaitingResync()` API. Latch is still set on receipt of `session.state_resync_required` (intentional one-way during replay window); consumers now have a clean path to clear after the replay stream drains. ### normalizeAuthDeviceFlowCancelled test coverage (wenshao R4) Coverage gap surfaced — happy path (valid deviceFlowId) and malformed fallback to debug both untested. Added 2 tests. ## Real Suggestions ### sanitizeUrl: AWS / Azure / GCP credential patterns The previous regex caught `x-amz-` and `x-goog-` headers + generic `signature` / `sig`, but missed: - `AWSAccessKeyId` (S3 presigned) - Azure SAS short codes (`sv` / `se` / `sr` / `sp` / `st` / `spr` / `sip` / `ss` / `srt` / `sig` / `skoid` / etc.) - GCP signed-URL `GoogleAccessId` + `Expires` (paired with credentials in signed URL contexts) Widened regex to include `aws|google|expires` prefixes + added explicit Azure-SAS Set check. ### detectFileDiff: `content` alias disambiguated `{ path, content }` was being classified as `file_diff` regardless of tool semantics — but the same shape is common for file_read assertions or search queries. Since detectFileDiff runs BEFORE detectFileRead in the detector chain, this caused mis-classification. Fix: restrict bare `content` to require either (a) write-intent tool name (write/create/edit/replace/save/update) OR (b) co-occurrence with `oldText`. Explicit `newText` / `new_text` / etc. still pass through unconditionally. Required adding `opts` to the `detectFileDiff` signature (callers already pass opts to siblings). ### detectFileRead: 0-based offset → 1-based range Type doc says `range: [startLine, endLine]` is 1-based inclusive. The offset+limit conversion produced 0-based output ([0, 9] for offset=0/limit=10), which displayed as "lines 0-9" — line 0 doesn't exist in 1-based. Convert at the detector: `[offset+1, offset+limit]`. Updated the matching test (which had encoded the 0-based bug as expected behavior). ### formatMissedRange — guard inverted / single-event ranges The naive `lastDeliveredId+1 .. earliestAvailableId-1` formula produced: - `gap === 0`: "missed 6-5" (inverted) - `gap === 1`: "missed 6-6" (single event shown as range) Added `formatMissedRange()` helper with explicit branches: - `last < first` → "no events lost (resync requested without gap)" - `last === first` → "missed 1 daemon event (id N)" - `last > first` → "missed daemon events X-Y" Applied in both `transcript.ts` (status block message) and `terminal.ts` (ANSI projection) — same formula was duplicated. ## doudouOUC R4 nits ### README errorKind list outdated Replaced `expired / transport / server / internal` with pointer to `KNOWN_DEVICE_FLOW_ERROR_KINDS` exported constant — canonical list auto-stays-in-sync. ### README "10 scenarios" stale Was 10, became 11 with subagent-nesting. Removed the count and let the corpus be derived at runtime via `DAEMON_UI_CONFORMANCE_FIXTURES.length`. ### selectTranscriptBlocks danger post lazy-COW With state.blocks now shared across sidechannel snapshots, a misbehaving consumer doing `(state.blocks as DaemonTranscriptBlock[]).sort()` would poison every snapshot sharing the reference. Freeze the blocks array at the dispatch boundary in `reduceDaemonTranscriptEvents`. Internal reducer mutation goes through `takeBlocksOwnership` which copies before mutating, so the frozen reference is never modified in place. ## Validation | | | |---|---| | SDK tests | **162/162** | | WebUI tests | **9/9** | | SDK typecheck | clean | | WebUI typecheck | clean | Generated with AI Co-authored-by: Claude Opus 4.7 * fix(daemon-ui): wenshao R5 review batch — Critical OAuth fragment leak + 10 more Walks 13 inline items from wenshao's 16:46-17:28 reviews. 11 fixed, 1 deduped (lint-no-console flagged in both reviews), 1 reverted/push-back (multi-part deny re-flags the same design-intent territory as R2 #4). ## Critical fixes ### sanitizeUrl: OAuth #fragment leak `sanitizeUrl` cleared query params and Basic Auth userinfo, but `u.toString()` preserved `u.hash`. OAuth 2.0 implicit grant puts `access_token=...` directly in the fragment (e.g., `https://app/#access_token=gho_xxx&token_type=bearer`); some Azure SAS variants similarly. Now `u.hash = ''` before serialize. For rendered output (markdown / HTML / plaintext), the fragment is client- state-only and dropping it removes the entire fragment-side leak surface. ### ESLint no-console on awaitingResync diagnostic Project lint forbids bare `console.*`. Added `eslint-disable-next-line no-console -- intentional diagnostic` per wenshao's suggestion. Behavior unchanged. ### normalizeAuthDeviceFlowCancelled test coverage (still missing post-R4) R4 added tests for one of the five device-flow normalizers; the `cancelled` variant was still uncovered. Added happy + malformed-payload tests. ## Behavior fixes ### Plaintext sanitizeTerminalText parity `daemonBlockToPlainText` + `daemonToolPreviewToPlainText` previously returned ANSI/bidi-control text verbatim, while markdown and HTML paths sanitized via `sanitizeTerminalText`. A daemon emitting bidi overrides survived clean to plaintext output — contradicting the "copy-paste / logs" JSDoc intent. Now routes every text field through `clean()` = `cap(sanitizeTerminalText(raw))`. ### blockquote helper applied to image_generation + subagent_delegation R3 added the helper for thought/debug/error but missed two preview markdown sites (`> ${text(preview.prompt)}` for image_generation, `> ${text(preview.task)}` for subagent_delegation). Multi-line prompts / tasks now stay inside the blockquote. ### Default unrecognized-event branch: single debug block Was emitting `status + debug` (2 blocks) per unknown event type. In long sessions where the daemon adds new types an older SDK doesn't recognize, this doubled block-consumption rate and accelerated `maxBlocks` trimming of real content. Now emit a single `debug` block that prefixes the event-type for adapters that want to pattern-match. ### writeIntent regex underscore-boundary aware R4's `content` alias gate-check used `\b` word boundaries, but `\b` doesn't match between `write` and `_` in `write_file` (both `\w`). Fixed to `(?:^|[_-])verb(?:$|[_-])` which catches the canonical `write_file` naming AND still rejects `prewrite_check`. Verb list extended per wenshao's suggestion (`overwrite`/`modify`/`patch`/`generate`). ### useDaemonPendingPermissions over-subscription Hook used `useDaemonTranscriptState()` which fires on every daemon event (text deltas, tool updates, sidechannel). Switched to `useDaemonTranscriptBlocks()` which only invalidates when the blocks array reference changes — block-mutating dispatches only, thanks to lazy COW. Same selector semantics, ~10x fewer renders in chat-heavy sessions. ### Conformance suite: try/catch adapter JSDoc promised "does not throw" but the loop wrapped adapter calls without try/catch. Buggy adapters aborted the whole suite instead of producing a structured `ConformanceFailure`. Now wrap; on throw, capture the error message in `renderedExcerpt: "[adapter threw: ...]"` and continue. ## Type / Quality fixes ### DaemonTranscriptState.blocks typed readonly Runtime contract is frozen (lazy-COW poison defense), but the type was mutable — consumers got runtime `TypeError` for in-place mutation instead of compile errors. Now `readonly DaemonTranscriptBlock[]` so mutation is caught at the type level. ### formatMissedRange exported / deduplicated Helper was duplicated inline between transcript.ts (full phrasing) and terminal.ts (terser phrasing). Exported from transcript.ts and reused in terminal.ts to prevent future drift. ## Push-back (false-positive — see reply) ### classifySelectedPermissionOption multi-part deny (`selected:deny:access_violation`) Re-flags the same `selected:X` design intent rejected in R2 #4. The caller comment explicitly states a selected option resolves the prompt even when the option id contains `deny`/`cancel`. The existing test `cancelled-substring-permission` (payload `selected:abort`, expected `completed`) codifies this. Daemon expresses true user-cancellation via the `cancelled` PRIMARY token, not `selected:cancel`. Not changing; reply directs to the same R2 #4 reasoning. ## Tests added (+10) - normalizeAuthDeviceFlowCancelled happy + malformed - sanitizeUrl OAuth fragment access_token rejected - sanitizeUrl AWS/GCP/Azure SAS credential params stripped - formatMissedRange no-gap / single-event / multi-event - detectFileDiff content alias rejected for read-like tools - detectFileDiff content alias accepted for write-like tools - writeIntent word boundaries (prewrite_check NOT matched) - conformance captures adapter throw - unrecognized event → single debug block - store.clearAwaitingResync clears latch ## Validation | | | |---|---| | SDK tests | **172/172** (was 162, +10) | | WebUI tests | **9/9** | | SDK typecheck | clean | | WebUI typecheck | clean | Generated with AI Co-authored-by: Claude Opus 4.7 * fix(daemon-ui): wenshao R6 — recovery flow chicken-and-egg + pending pointer Three Criticals from R6 review (4351217188) all pointing at real bugs introduced by R4/R5 work — not false positives. Fixes plus regression tests. ## Critical 1 — same-session reconnect never clears the latch When the daemon emitted `state_resync_required`, the reducer set `awaitingResync = true`. The webui provider dispatched `assistant.done { reason: 'reconnected' }` after re-attaching SSE but never called `store.clearAwaitingResync()`. Result: events flowed in on the fresh stream but every one got dropped by the `applyDaemonTranscriptEvent` passthrough guard. Transcript appeared permanently frozen with no diagnostic clue (the `console.warn` fired on each drop, but the user wouldn't necessarily check DevTools). Fix: in `DaemonSessionProvider.tsx`, after dispatching the synthetic `reconnected` `assistant.done`, check `awaitingResync` and clear it BEFORE the new SSE event loop starts. ## Critical 2 — updateCurrentToolPointer breaks on undefined status In `upsertToolBlock`, a new tool block is created with `status: event.status ?? 'pending'`. But `updateCurrentToolPointer` was called with raw `event.status` — when undefined, the function's own `if (status === undefined) return;` guard short-circuited without ever pointing at the new (visually-pending) block. Result: `selectCurrentTool` returned `undefined` for daemon events that omitted the explicit `status` field, while the block sat at "pending" in the UI — invisible to the current-tool selector. Fix: pass the EFFECTIVE status (`event.status ?? 'pending'`) so the pointer logic mirrors the actual stored status. ## Critical 3 — clearAwaitingResync flow chicken-and-egg The earlier (R4) JSDoc documented the recovery flow as: "re-subscribe with `Last-Event-ID: 0`, then call clearAwaitingResync after replay drains." But while the latch is true, EVERY non-passthrough event is dropped at `applyDaemonTranscriptEvent`. So during the replay drain, zero events made it into state, and clearing the latch afterward did nothing — transcript permanently empty. Correct flow: clear FIRST, then stream events. Updated JSDoc on both `types.ts` interface and `store.ts` impl to document this clearly. Added a regression test (`clearAwaitingResync AFTER dispatching events: events ARE dropped`) that pins the correct flow in code. ## Regression tests (+3) - `undefined status` creates pending block AND sets currentToolCallId - clear-then-dispatch ✓ events flow - dispatch-then-clear ✗ events dropped (correct flow documentation) ## Validation | | | |---|---| | SDK tests | **175/175** (was 172, +3) | | WebUI tests | **9/9** | | SDK typecheck | clean | | WebUI typecheck | clean | ## Note on doudouOUC heads-up #4469 (main → daemon_mode_b_main sync, 45 commits since 2026-05-19) will land soon. doudouOUC's note says rebase should be smooth (no daemon-ui surface conflicts). Will rebase on the cron's next pass after #4469 merges. Generated with AI Co-authored-by: Claude Opus 4.7 * fix(daemon-ui): wenshao R7 — escapeMarkdownText covers `<` + details URL sanitization Two items from wenshao R7 (one inline Suggestion + one Verification-PASS finding). Both gate-checked as real; fixed. ## escapeMarkdownText: add `<` to escape set Markdown rendered through markdown-it with `html: true` would previously pass through raw `` / `` pass the protocol check. Modern browsers don't execute ``, but the comment claimed "never legitimate in ``" which slightly over-claimed the protection. Tighten the data: branch to require an `image/` MIME prefix. Verified by a new test that covers: https (allow), data:image/png (allow), data:text/html (reject → '#'), javascript: (reject → '#'). Generated with AI Co-authored-by: Claude Opus 4.7 * fix(daemon-ui): wenshao + doudouOUC R4 review batch Walks 6 wenshao items (delivered as 8 review submissions — 2 CHANGES_REQUESTED + 6 individual COMMENTED — but 6 distinct concerns) and 3 doudouOUC R4 nits. All 9 real issues addressed; no false-positives this round. ## Real Criticals ### awaitingResync recovery API (wenshao R4) `store.reset()` requires session-id change semantics — wrong shape for "same-session reconnect with SSE replay" recovery. Added explicit `store.clearAwaitingResync()` API. Latch is still set on receipt of `session.state_resync_required` (intentional one-way during replay window); consumers now have a clean path to clear after the replay stream drains. ### normalizeAuthDeviceFlowCancelled test coverage (wenshao R4) Coverage gap surfaced — happy path (valid deviceFlowId) and malformed fallback to debug both untested. Added 2 tests. ## Real Suggestions ### sanitizeUrl: AWS / Azure / GCP credential patterns The previous regex caught `x-amz-` and `x-goog-` headers + generic `signature` / `sig`, but missed: - `AWSAccessKeyId` (S3 presigned) - Azure SAS short codes (`sv` / `se` / `sr` / `sp` / `st` / `spr` / `sip` / `ss` / `srt` / `sig` / `skoid` / etc.) - GCP signed-URL `GoogleAccessId` + `Expires` (paired with credentials in signed URL contexts) Widened regex to include `aws|google|expires` prefixes + added explicit Azure-SAS Set check. ### detectFileDiff: `content` alias disambiguated `{ path, content }` was being classified as `file_diff` regardless of tool semantics — but the same shape is common for file_read assertions or search queries. Since detectFileDiff runs BEFORE detectFileRead in the detector chain, this caused mis-classification. Fix: restrict bare `content` to require either (a) write-intent tool name (write/create/edit/replace/save/update) OR (b) co-occurrence with `oldText`. Explicit `newText` / `new_text` / etc. still pass through unconditionally. Required adding `opts` to the `detectFileDiff` signature (callers already pass opts to siblings). ### detectFileRead: 0-based offset → 1-based range Type doc says `range: [startLine, endLine]` is 1-based inclusive. The offset+limit conversion produced 0-based output ([0, 9] for offset=0/limit=10), which displayed as "lines 0-9" — line 0 doesn't exist in 1-based. Convert at the detector: `[offset+1, offset+limit]`. Updated the matching test (which had encoded the 0-based bug as expected behavior). ### formatMissedRange — guard inverted / single-event ranges The naive `lastDeliveredId+1 .. earliestAvailableId-1` formula produced: - `gap === 0`: "missed 6-5" (inverted) - `gap === 1`: "missed 6-6" (single event shown as range) Added `formatMissedRange()` helper with explicit branches: - `last < first` → "no events lost (resync requested without gap)" - `last === first` → "missed 1 daemon event (id N)" - `last > first` → "missed daemon events X-Y" Applied in both `transcript.ts` (status block message) and `terminal.ts` (ANSI projection) — same formula was duplicated. ## doudouOUC R4 nits ### README errorKind list outdated Replaced `expired / transport / server / internal` with pointer to `KNOWN_DEVICE_FLOW_ERROR_KINDS` exported constant — canonical list auto-stays-in-sync. ### README "10 scenarios" stale Was 10, became 11 with subagent-nesting. Removed the count and let the corpus be derived at runtime via `DAEMON_UI_CONFORMANCE_FIXTURES.length`. ### selectTranscriptBlocks danger post lazy-COW With state.blocks now shared across sidechannel snapshots, a misbehaving consumer doing `(state.blocks as DaemonTranscriptBlock[]).sort()` would poison every snapshot sharing the reference. Freeze the blocks array at the dispatch boundary in `reduceDaemonTranscriptEvents`. Internal reducer mutation goes through `takeBlocksOwnership` which copies before mutating, so the frozen reference is never modified in place. ## Validation | | | |---|---| | SDK tests | **162/162** | | WebUI tests | **9/9** | | SDK typecheck | clean | | WebUI typecheck | clean | Generated with AI Co-authored-by: Claude Opus 4.7 * fix(daemon-ui): wenshao R5 review batch — Critical OAuth fragment leak + 10 more Walks 13 inline items from wenshao's 16:46-17:28 reviews. 11 fixed, 1 deduped (lint-no-console flagged in both reviews), 1 reverted/push-back (multi-part deny re-flags the same design-intent territory as R2 #4). ## Critical fixes ### sanitizeUrl: OAuth #fragment leak `sanitizeUrl` cleared query params and Basic Auth userinfo, but `u.toString()` preserved `u.hash`. OAuth 2.0 implicit grant puts `access_token=...` directly in the fragment (e.g., `https://app/#access_token=gho_xxx&token_type=bearer`); some Azure SAS variants similarly. Now `u.hash = ''` before serialize. For rendered output (markdown / HTML / plaintext), the fragment is client- state-only and dropping it removes the entire fragment-side leak surface. ### ESLint no-console on awaitingResync diagnostic Project lint forbids bare `console.*`. Added `eslint-disable-next-line no-console -- intentional diagnostic` per wenshao's suggestion. Behavior unchanged. ### normalizeAuthDeviceFlowCancelled test coverage (still missing post-R4) R4 added tests for one of the five device-flow normalizers; the `cancelled` variant was still uncovered. Added happy + malformed-payload tests. ## Behavior fixes ### Plaintext sanitizeTerminalText parity `daemonBlockToPlainText` + `daemonToolPreviewToPlainText` previously returned ANSI/bidi-control text verbatim, while markdown and HTML paths sanitized via `sanitizeTerminalText`. A daemon emitting bidi overrides survived clean to plaintext output — contradicting the "copy-paste / logs" JSDoc intent. Now routes every text field through `clean()` = `cap(sanitizeTerminalText(raw))`. ### blockquote helper applied to image_generation + subagent_delegation R3 added the helper for thought/debug/error but missed two preview markdown sites (`> ${text(preview.prompt)}` for image_generation, `> ${text(preview.task)}` for subagent_delegation). Multi-line prompts / tasks now stay inside the blockquote. ### Default unrecognized-event branch: single debug block Was emitting `status + debug` (2 blocks) per unknown event type. In long sessions where the daemon adds new types an older SDK doesn't recognize, this doubled block-consumption rate and accelerated `maxBlocks` trimming of real content. Now emit a single `debug` block that prefixes the event-type for adapters that want to pattern-match. ### writeIntent regex underscore-boundary aware R4's `content` alias gate-check used `\b` word boundaries, but `\b` doesn't match between `write` and `_` in `write_file` (both `\w`). Fixed to `(?:^|[_-])verb(?:$|[_-])` which catches the canonical `write_file` naming AND still rejects `prewrite_check`. Verb list extended per wenshao's suggestion (`overwrite`/`modify`/`patch`/`generate`). ### useDaemonPendingPermissions over-subscription Hook used `useDaemonTranscriptState()` which fires on every daemon event (text deltas, tool updates, sidechannel). Switched to `useDaemonTranscriptBlocks()` which only invalidates when the blocks array reference changes — block-mutating dispatches only, thanks to lazy COW. Same selector semantics, ~10x fewer renders in chat-heavy sessions. ### Conformance suite: try/catch adapter JSDoc promised "does not throw" but the loop wrapped adapter calls without try/catch. Buggy adapters aborted the whole suite instead of producing a structured `ConformanceFailure`. Now wrap; on throw, capture the error message in `renderedExcerpt: "[adapter threw: ...]"` and continue. ## Type / Quality fixes ### DaemonTranscriptState.blocks typed readonly Runtime contract is frozen (lazy-COW poison defense), but the type was mutable — consumers got runtime `TypeError` for in-place mutation instead of compile errors. Now `readonly DaemonTranscriptBlock[]` so mutation is caught at the type level. ### formatMissedRange exported / deduplicated Helper was duplicated inline between transcript.ts (full phrasing) and terminal.ts (terser phrasing). Exported from transcript.ts and reused in terminal.ts to prevent future drift. ## Push-back (false-positive — see reply) ### classifySelectedPermissionOption multi-part deny (`selected:deny:access_violation`) Re-flags the same `selected:X` design intent rejected in R2 #4. The caller comment explicitly states a selected option resolves the prompt even when the option id contains `deny`/`cancel`. The existing test `cancelled-substring-permission` (payload `selected:abort`, expected `completed`) codifies this. Daemon expresses true user-cancellation via the `cancelled` PRIMARY token, not `selected:cancel`. Not changing; reply directs to the same R2 #4 reasoning. ## Tests added (+10) - normalizeAuthDeviceFlowCancelled happy + malformed - sanitizeUrl OAuth fragment access_token rejected - sanitizeUrl AWS/GCP/Azure SAS credential params stripped - formatMissedRange no-gap / single-event / multi-event - detectFileDiff content alias rejected for read-like tools - detectFileDiff content alias accepted for write-like tools - writeIntent word boundaries (prewrite_check NOT matched) - conformance captures adapter throw - unrecognized event → single debug block - store.clearAwaitingResync clears latch ## Validation | | | |---|---| | SDK tests | **172/172** (was 162, +10) | | WebUI tests | **9/9** | | SDK typecheck | clean | | WebUI typecheck | clean | Generated with AI Co-authored-by: Claude Opus 4.7 * fix(daemon-ui): wenshao R6 — recovery flow chicken-and-egg + pending pointer Three Criticals from R6 review (4351217188) all pointing at real bugs introduced by R4/R5 work — not false positives. Fixes plus regression tests. ## Critical 1 — same-session reconnect never clears the latch When the daemon emitted `state_resync_required`, the reducer set `awaitingResync = true`. The webui provider dispatched `assistant.done { reason: 'reconnected' }` after re-attaching SSE but never called `store.clearAwaitingResync()`. Result: events flowed in on the fresh stream but every one got dropped by the `applyDaemonTranscriptEvent` passthrough guard. Transcript appeared permanently frozen with no diagnostic clue (the `console.warn` fired on each drop, but the user wouldn't necessarily check DevTools). Fix: in `DaemonSessionProvider.tsx`, after dispatching the synthetic `reconnected` `assistant.done`, check `awaitingResync` and clear it BEFORE the new SSE event loop starts. ## Critical 2 — updateCurrentToolPointer breaks on undefined status In `upsertToolBlock`, a new tool block is created with `status: event.status ?? 'pending'`. But `updateCurrentToolPointer` was called with raw `event.status` — when undefined, the function's own `if (status === undefined) return;` guard short-circuited without ever pointing at the new (visually-pending) block. Result: `selectCurrentTool` returned `undefined` for daemon events that omitted the explicit `status` field, while the block sat at "pending" in the UI — invisible to the current-tool selector. Fix: pass the EFFECTIVE status (`event.status ?? 'pending'`) so the pointer logic mirrors the actual stored status. ## Critical 3 — clearAwaitingResync flow chicken-and-egg The earlier (R4) JSDoc documented the recovery flow as: "re-subscribe with `Last-Event-ID: 0`, then call clearAwaitingResync after replay drains." But while the latch is true, EVERY non-passthrough event is dropped at `applyDaemonTranscriptEvent`. So during the replay drain, zero events made it into state, and clearing the latch afterward did nothing — transcript permanently empty. Correct flow: clear FIRST, then stream events. Updated JSDoc on both `types.ts` interface and `store.ts` impl to document this clearly. Added a regression test (`clearAwaitingResync AFTER dispatching events: events ARE dropped`) that pins the correct flow in code. ## Regression tests (+3) - `undefined status` creates pending block AND sets currentToolCallId - clear-then-dispatch ✓ events flow - dispatch-then-clear ✗ events dropped (correct flow documentation) ## Validation | | | |---|---| | SDK tests | **175/175** (was 172, +3) | | WebUI tests | **9/9** | | SDK typecheck | clean | | WebUI typecheck | clean | ## Note on doudouOUC heads-up #4469 (main → daemon_mode_b_main sync, 45 commits since 2026-05-19) will land soon. doudouOUC's note says rebase should be smooth (no daemon-ui surface conflicts). Will rebase on the cron's next pass after #4469 merges. Generated with AI Co-authored-by: Claude Opus 4.7 * fix(daemon-ui): wenshao R7 — escapeMarkdownText covers `<` + details URL sanitization Two items from wenshao R7 (one inline Suggestion + one Verification-PASS finding). Both gate-checked as real; fixed. ## escapeMarkdownText: add `<` to escape set Markdown rendered through markdown-it with `html: true` would previously pass through raw `` / `