… mechanical lift + BridgeFileSystem seam) (QwenLM#4319)

* refactor(acp-bridge): lift defaultSpawnChannelFactory to acp-bridge/spawnChannel (QwenLM#4175 F1 step 1)

First mechanical lift of QwenLM#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 (QwenLM#4175 F1 step 2)

Second mechanical lift of QwenLM#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 (QwenLM#4175 F1 step 3)

Third + final mechanical lift of QwenLM#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 (QwenLM#4175 F1 step 5, 22b' scope)

Adds the `BridgeFileSystem` injection seam originally scoped as QwenLM#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 QwenLM#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 QwenLM#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 QwenLM#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 (QwenLM#4319 wenshao review)

Folds in wenshao review on QwenLM#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 (QwenLM#4319 wenshao round 2)

Folds in wenshao review on QwenLM#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 QwenLM#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 (QwenLM#4319 wenshao round 3)

Folds in 2 of 3 wenshao Suggestions from QwenLM#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 QwenLM#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 QwenLM#4297 review (3 issues)

Folds 3 unresolved review comments from the post-merge thread on QwenLM#4297
(wenshao via qwen-latest agent) into F1 (QwenLM#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, QwenLM#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 QwenLM#4297 (wenshao via qwen-latest agent), post-
  merge, currently unresolvable on QwenLM#4297 itself because that PR is
  already MERGED.
- Other 2 QwenLM#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 (QwenLM#4319)

Folds in 2 P2 findings from a Codex review run on `git diff main...HEAD`
of F1 PR QwenLM#4319. Both are pre-existing in code merged into
`daemon_mode_b_main` before F1 was created (QwenLM#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 QwenLM#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 `QwenLM#4282 PR 17` code, not F1-introduced
regressions. Normally we'd track as separate follow-up issues (mirror
of the QwenLM#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 QwenLM#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 (QwenLM#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 QwenLM#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 (QwenLM#4319)

Folds in 1 of 3 wenshao Suggestions on F1 PR QwenLM#4319 round 5; 2 declined
with tracking issues opened (QwenLM#4329, QwenLM#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 QwenLM#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 QwenLM#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 QwenLM#4319 round 5 (closes QwenLM#4329)

Folds in wenshao Suggestion from QwenLM#4319 round 5 (originally declined as
out-of-scope, opened as QwenLM#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 QwenLM#4329
- F1 PR QwenLM#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)

## Critical #1 — 401/403 reconnect storm + transcript wipe

`DaemonSessionProvider`'s reconnect loop kept retrying `createOrAttach` on
401/403 even with `autoReconnect: true`. Each cycle:
  - hit the daemon with the same bad token → 401 again
  - cleared the session handle
  - the next successful attempt (if token magically recovered) would
    receive a different sessionId, triggering the `store.reset()` branch
    at line 143 and wiping the user's transcript
  - no terminal "auth failed" state surfaced to the user

Fix: split `TERMINAL_SESSION_HTTP_STATUSES` into `AUTH_FAILURE_HTTP_STATUSES`
(401, 403) and the rest (404, 410). On auth failure, return from the
reconnect loop unconditionally regardless of the `autoReconnect` flag —
these are credential failures, not transient. The user must update
credentials; daemon spam must stop.

`extractHttpStatus` helper factored out of `isTerminalSessionHttpError` to
share between the two predicates.

## Critical #2 — rawInput / rawOutput leaking secrets to UI

`normalizer.normalizeToolUpdate` forwarded `rawInput` / `rawOutput`
verbatim onto `DaemonUiToolUpdateEvent` → `DaemonToolTranscriptBlock`. The
`details` projection was redacted via `stringifyRedactedJson` /
`redactSensitiveFields`, but the underlying `rawInput` / `rawOutput`
fields were unredacted. Any UI component that read those fields directly
(ShellToolCall, WriteToolCall, JSON debug panels) leaked the raw values
to the DOM.

Example: `{ command: 'curl', apiKey: 'sk-prod-...' }` had `apiKey`
redacted in `details` but exposed verbatim on `rawInput`.

Fix: apply `redactSensitiveFields` to both `rawInput` and `rawOutput`
ONCE at the normalizer boundary, then reuse the redacted shape for the
`details` projection. Downstream is uniformly safe; no double traversal.

## Tests (49/49 pass)

- SDK `daemonUi.test.ts` (36 tests, +1) — new test `redacts sensitive
  fields in tool.update rawInput and rawOutput at normalizer boundary`
  verifies full-event string scan finds zero secret values + structural
  keys preserved with values `'[redacted]'`.
- WebUI `DaemonSessionProvider.test.tsx` (13 tests, +2) — new tests
  `breaks out of the reconnect loop on 401 / 403 auth failures even when
  autoReconnect is true` and `still reconnects on 404 / 410
  session-not-found errors when autoReconnect is true` lock in the
  asymmetry: auth failure → 1 attempt only; session-not-found → retries
  until success.

## Out of scope (declined / deferred — see PR review reply)

- CRIT #3 `withActionTimeout` test coverage gap → behavior correct,
  test-only follow-up (avoids PR bloat)
- Suggestions QwenLM#4-7 → 4 nice-to-haves, deferred to keep PR focused on
  production-correctness fixes

Generated with AI

Co-authored-by: Claude Opus 4.7 <[email protected]>

…#4340)

* fix(review): harden SKILL.md against weak-model rule skipping

Weak models often skip parts of the long /review prompt and fall back
to familiar defaults — `gh pr checkout` instead of the worktree flow,
or running the autofix prompt even when the user passed `--comment`
(which means "only post inline comments, don't mutate code").

Three reinforcements, all in SKILL.md (no CLI changes):

- Promote the two most commonly violated rules to the top of the
  "Critical rules" list: worktree is mandatory for PR reviews, and
  `--comment` skips Step 8 entirely.
- Add an inline blockquote at the top of the Step 1 PR branch that
  names the specific forbidden commands (`gh pr checkout`,
  `git checkout`, `git switch`, `git pull`, `git reset --hard`).
- Add an explicit skip block at the top of Step 8 listing the three
  conditions that bypass autofix — `--comment`, cross-repo lightweight
  mode, or no fixable findings — so a weak model doesn't have to
  infer them from scattered earlier text.

* fix(review): address /review comments on rule scope + Step 8 dedup

Follow-up to the initial harden pass, addressing the inline review
comments on PR QwenLM#4340.

Rule #1 (worktree mandatory):
- Scope it to **same-repo PR reviews** so cross-repo PRs running in
  lightweight mode (no matching local remote, no worktree) don't read
  as a contradiction.
- Replace "Your very first action" with "After argument parsing and
  remote detection, the first command that touches code state" — the
  literal "very first" was wrong since `--comment` parsing and
  URL/remote disambiguation legitimately run before `fetch-pr`.
- Align the forbidden-command list with the Step 1 blockquote (add
  `git pull` and `git reset --hard`) so a weak model that only reads
  the Critical rules section sees the same five commands as a model
  that reaches the blockquote at the point of use.
- Add an explicit "cross-repo PRs use lightweight mode" parenthetical
  so the same model knows where to look for the alternative path.

Step 8 skip block:
- Drop the redundant third bullet ("no Critical or Suggestion findings
  with concrete, applicable fixes") — it was both logically equivalent
  to the "Otherwise" clause below and used a different qualifier
  ("concrete, applicable" vs "clear, unambiguous"), risking a weak
  model treating them as two distinct thresholds.
- "ANY of the following" → "EITHER" since only two bullets remain.
- Fold the no-findings case into the Otherwise clause as a no-op note.