* 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.

…hanical 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<ReadTextFileResponse>`
- `BridgeFileSystem.writeText(params: WriteTextFileRequest):
  Promise<WriteTextFileResponse>`

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)

…fo 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):
#4334 (comment)

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 9f73b83 — 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 e185409) 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<WriteMode, 'overwrite'>`
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.