fix(daemon): cross-client sync follow-up cleanup (epoch-reset resync, approval-mode serialization, catch-up indicator)#4510
Merged
Conversation
* 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 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.
What this does
Independent fixes and cleanup over the merged cross-client real-time sync base. Each item is self-contained.
Correctness
Last-Event-IDpast the bus high-water — which only happens after a daemon restart rebuilt the EventBus — the bus now emitsstate_resync_required{reason:'epoch_reset'}before replay. Previously the empty post-restart ring made the existing ring-evicted check a no-op, so the consumer got a barereplay_complete{replayedCount:0}— a false "you're caught up" while its reducer still held stale data from the dead epoch. Epoch reset also replays the whole fresh ring, so the new low-id events aren't filtered out by the stale cursor.setSessionApprovalModenow runs through a per-session FIFO (mirroring the existingmodelChangeQueue). Two concurrent approval-mode changes can no longer interleave their ACP roundtrips and publish anapproval_mode_changedthat disagrees with the mode the child actually settled on.Cross-client UX
DaemonSessionProviderconsumes thereplay_completesentinel to exposeconnection.catchingUp, giving clients a deterministic "replaying history → live" transition instead of a spinner-timeout guess. Armed only on resume (the daemon emitsreplay_completeonly forLast-Event-IDsubscriptions).prompt_cancelledis now broadcast at most once per active prompt, so a client that POSTs/canceland then drops its socket no longer produces two cancel frames for the same turn.prompt_cancelled{reason:'forward_failed'}so peers see the turn end instead of waiting forever on the echoed input.Wire / hardening
replay_completenow carrieslastReplayedEventId(canonical) alongside a deprecatedlastEventIdalias; the SDK prefers the new name. Removes the naming collision with the SSE protocol'sLast-Event-IDin raw traces. Additive and backward-compatible.Array.isArrayguard on the echo path, and documented the deliberate_metaplacement.Benefits
Tests
acp-bridge 302 pass (new epoch-reset / approval-mode-serialization / cancel-dedup / forward-failed cases), SDK daemon 259 pass, webui daemon 30 pass (new catch-up case). Typecheck and lint clean.