Round-5 fold-in. Four findings from the deepseek-v4-pro review on
PR #4305 — all real, three are sister fixes for the same security
classes that #4305 already closed at adjacent surfaces.

#1 (deviceFlow.ts) — `pollTimedOut` race correctness. The flag was
set unconditionally inside the timer callback. If the provider
settled the wrapper at 29.9s, `finally` would call
`clearScheduled(pollTimer)` — but if the timer callback was already
queued for execution before the clear landed (a real possibility
in Node's event-loop ordering, even if not always observed in
practice), this branch could still run and incorrectly mark
`pollTimedOut`. Move the flag assignment to the catch block where
the settled cause is unambiguous via `instanceof
DeviceFlowPollTimeoutError`. New test pins the negative: provider
beats the timeout → no spurious `lost_late_poll_after_timeout`
audit even after ticking 2× the ceiling.

#2 (deviceFlow.ts) — late-rejection observer interpolated raw
`lateErr.name` into the audit hint without sanitization. Same
attacker-controlled vector closed at the provider layer for
`err.name` in round-4. Route through `sanitizeForStderr`.

#3 (deviceFlow.ts) — late-success observer interpolated
`latePollResult.kind` directly into the audit template. While the
typed shape is `'pending' | 'slow_down' | 'success' | 'error'`, a
non-conforming provider could return an arbitrary string. Same
log-injection vector. Route through `sanitizeForStderr`.

#4 (qwenDeviceFlowProvider.ts → deviceFlow.ts) —
`sanitizeForStderr` only stripped ASCII C0/C1 + DEL; bypass via
Unicode lookalikes:
  - U+2028/U+2029: LINE/PARAGRAPH SEPARATOR (newline-equivalent in
    most Unicode-aware terminals — most direct log-forging vector)
  - U+200B–U+200F: zero-width chars + LRM/RLM
  - U+202A–U+202E: bidirectional override controls
  - U+FEFF: BOM / ZWNBSP

A malicious IdP returning `slow_down
[serve] FAKE` in
`oauthError` would otherwise still forge log lines.

Architectural change: `sanitizeForStderr` was previously private to
`qwenDeviceFlowProvider.ts`. To address #2/#3, the registry layer
needs to call it too. Lifted into `deviceFlow.ts` (the foundation
module) and re-imported from the provider. Single source of truth;
the regex is now a module-level constant compiled once with explicit
`\uXXXX` escapes (via `String.raw` so the source is greppable, not
literal-Unicode-laden).

Tests:
- `does NOT attach late-poll observer when the provider beats the
  timeout` — N1 race regression
- `sanitizes hostile latePollResult.kind in late-observer audit` — N3
- `sanitizes hostile lateErr.name in late-rejection observer audit` — N2
- `sanitizes Unicode lookalike controls (U+2028 LINE SEPARATOR,
  bidi, ZWNBSP) in oauthError` — N4

cli serve 706/706 (was 702, +4 — all new round-5 tests); sdk
421/421; typecheck clean; eslint --max-warnings 0 clean on touched
files.

Refs: #4175, #4255, #4291, #4305

🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)

…threads)

Round-6 fold-in. Five findings split between maintainability,
security hardening, and a real defensive bug.

#1 (qwenDeviceFlowProvider.test.ts) — gpt-5.5: round-5 #4 test
embedded U+2028 / U+200E / U+FEFF as literal characters in source.
Invisible in GitHub diffs / most editors; the negative
`not.toContain('')` looked like an empty-string check. Rewrote
the payload + assertions to use named `\uXXXX`-bound constants.
Also added a companion test exercising U+2066–U+2069 (round-6 #5
below).

#2 (deviceFlow.ts) — qwen-latest: the late-poll observer's
`void tracked.then(...)` was missing a terminal `.catch(() => {})`.
A synchronous throw inside either handler (e.g., a misbehaving
`audit.record`: backpressure, malformed payload, sink out-of-disk)
would reject the derived promise unhandled. On Node 22's default
`--unhandled-rejections=throw`, that crashes the daemon. Added the
terminal `.catch(() => {})` matching the persist-tracker pattern.
New test injects a poison audit sink that throws specifically on
the `lost_late_poll_after_timeout` call; asserts `flushAsync()`
resolves cleanly.

#3 (deviceFlow.ts) — qwen-latest: the `case 'error'` audit-record
hint interpolated `rawProviderError` (raw `err.message`) without
`sanitizeForStderr`. Per ES2019+ `JSON.stringify` no longer escapes
U+2028/U+2029 — those would still forge log lines downstream
through file/stdout audit sinks. Apply the same sanitizer used on
every other provider-controlled audit path. New test pins a hostile
provider message containing U+2028 + ANSI escape and asserts
neither survives.

#4 (deviceFlow.ts) — qwen-latest: the round-5 #1 comment claimed
"`DeviceFlowPollTimeoutError` isn't exported as a public DeviceFlow
contract", but it IS `export class` (the test file constructs it
directly for fixtures). With `pollTimedOut = true` keyed solely on
`instanceof`, a future provider that imports + throws the class
would spoof the registry's "I caused the timeout" signal —
attaching a phantom late-poll observer.

Fix: introduce a runtime brand `_isRegistryTimeout: boolean` on the
class (default `false`) plus an internal-only
`makeRegistryPollTimeoutError(ms)` helper that sets the brand to
`true`. The brand is set ONLY at the registry's race-timer
construction site. Both gates updated:
  - `if (err instanceof X && err._isRegistryTimeout === true)` in
    the catch (for `pollTimedOut`)
  - `if (lateErr instanceof X && lateErr._isRegistryTimeout === true)`
    in the late-rejection self-filter

A provider-thrown brand-false instance now flows through the
generic provider-throw audit path — correctly auditing the misuse
rather than silently swallowing it. Repurposed the original "no
double-audit when registry's own DeviceFlowPollTimeoutError is
late-rejected" test (which was actually exercising the brand-false
path) into the inverted assertion: brand-false provider throw IS
audited as a real failure. Removed the orphaned old assertion; the
brand-true happy path is implicitly covered by the hanging-provider
test (which exercises the registry-built timeout end-to-end).

#5 (deviceFlow.ts) — qwen-latest: `sanitizeForStderr` regex covered
U+202A–U+202E (bidi embedding/override) but missed U+2066–U+2069
(LRI/RLI/FSI/PDI). These are the primary CVE-2021-42574
("Trojan Source") attack vectors — a hostile IdP swapping U+2066
for U+202D achieves the same visual reordering and would have
bypassed the round-5 filter entirely. Extended the regex range and
JSDoc; new test exercises U+2066/U+2068/U+2069 in `oauthError` and
asserts none survive while substantive ASCII parts remain.

cli serve 713/713 (was 710, +3 round-6 tests + the round-5 #4
rewrite + the round-6 #5 companion); typecheck clean across all 4
workspaces; eslint --max-warnings 0 clean on touched files.

Refs: #4175, #4255, #4291, #4305

🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)

… test

PR #4312 review (Copilot): the round-6 #3 test (sanitizes
rawProviderError) regressed back to embedding a literal U+2028
character in source via `const U_2028 = ' '`. That's the same
maintainability anti-pattern round-6 #1 was fixing in the sister
test. Internal-consistency fix: switch to the explicit `
`
escape so the constant is greppable and reviewable in GitHub diffs.

Refs: #4291, #4305, #4312

🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)

* fix(serve): address qwen-latest review on merged #4291 (7 threads)

Seven post-merge findings from the qwen-latest review on #4291,
all real. Most are tightening fixes for issues introduced by the
earlier rounds of #4291 — the same security / DRY / observability
classes the original review surfaced, applied to surfaces that
weren't covered initially.

#1 (deviceFlow.ts:1179) — late-poll observer closure retained the
entire entry by reference (deviceCode/pkceVerifier BrandedSecrets +
cancelController) for the lifetime of the daemon if `provider.poll()`
never settled. Memory leak + indefinite secret retention. Destructure
the four fields the closure actually needs (deviceFlowId, providerId,
initiatorClientId, audit sink) so the entry is GC-eligible the
moment runPollTick returns.

#2 (server.ts) — `callerIsInitiator` was duplicated verbatim across
three locations: GET handler, toDeviceFlowStartResponseBody,
toDeviceFlowStateBody. The exact bug class #4291 was fixing was
"POST and GET diverged on the same redaction policy" — duplicating
the gate recreated the preconditions for divergence. Extracted to
shared `callerIsDeviceFlowInitiator(view, callerClientId)` helper
with the consolidated threat-model JSDoc. All three sites now call
the helper.

#3 (deviceFlow.ts:1110) — timeout callback constructed two separate
`DeviceFlowPollTimeoutError` instances (one for `signal.reason`, one
for the wrapper rejection). Each capture its own V8 stack trace,
and `signal.reason.stack` would diverge from the caught rejection's
stack — confusing for operators inspecting both. Build the sentinel
ONCE per timer fire and pass the same instance to both sites.

#4 (qwenDeviceFlowProvider.ts:273) — `Error.name` is a freely
assignable string property; a hostile fetch wrapper could set
`e.name = 'X\n[serve] FAKE LINE\x1b[31m'` to inject log lines or
ANSI sequences via the same vector we already closed for `oauthError`.
The non-OAuth catch path interpolated `${err.name}` raw. Apply the
same `sanitizeForStderr()` helper.

#5 (deviceFlow.ts:1551) — on the timeout path, `rawProviderError`
is undefined (deliberately, to skip the misleading
`provider.poll() threw (raw): ...` audit template), but that left
the audit hint field omitted entirely. Operators reading the
durable audit trail saw `errorKind: 'upstream_error'` with no signal
whether it was a hung IdP or a generic provider failure. Use
`result.hint` (which already carries the timeout-specific
`provider.poll() timed out after Nms; check IdP connectivity` text
built in the catch) so the audit matches the SSE event.

#6 (server.ts) — the `QWEN_SERVE_DEBUG` env-var check was inlined
in the GET route handler, duplicating the `isServeDebugMode()`
helper from `./debugMode.js` that workspaceAgents and
workspaceMemory already use. The inline copy also had a dead `?? ''`
fallback (the value is guaranteed truthy at that point per the
preceding check). Use the canonical helper.

#7 (deviceFlow.ts:1217) — late-rejection observer interpolated the
raw `lateErr.message` into the audit hint (truncated to 256 bytes,
but RFC 8628 `device_code` values fit comfortably in 256 bytes).
The provider's catch already uses the `name + length` redaction
pattern to prevent WAF-echoed `device_code`/PKCE leaks; the
registry layer was undoing that hardening because the same failure
settled late. Apply the same `name + length` pattern at the late-
rejection site.

Tests:
- Existing late-rejection test reseeded with a `device-code-secret-*`
  substring inside the long detail; hard-negative-asserts the seeded
  secret is absent from the audit + asserts the new
  `Error (message N bytes; raw suppressed)` shape.
- Existing poll-timeout test now also asserts: hint IS defined on
  the audit (not omitted), hint contains `'timed out after'` /
  `'check IdP connectivity'`, and `signal.reason instanceof
  DeviceFlowPollTimeoutError` (proves the single sentinel is
  shared between abort and reject).
- New `sanitizes control characters in attacker-controlled
  err.name` test in qwenDeviceFlowProvider.test.ts pins the round-4
  #4 fix with a hostile `e.name` containing `\n` + `\x1b[31m...`.

cli serve 702/702 (was 686, +16 — additional tests imported via
the acp-bridge package lift on main); sdk 421/421; typecheck clean
across all 4 workspaces; eslint --max-warnings 0 clean on touched
files.

Refs: #4175, #4255, #4291

🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)

* fix(serve): address deepseek-v4-pro review on #4305 (4 threads)

Round-5 fold-in. Four findings from the deepseek-v4-pro review on
PR #4305 — all real, three are sister fixes for the same security
classes that #4305 already closed at adjacent surfaces.

#1 (deviceFlow.ts) — `pollTimedOut` race correctness. The flag was
set unconditionally inside the timer callback. If the provider
settled the wrapper at 29.9s, `finally` would call
`clearScheduled(pollTimer)` — but if the timer callback was already
queued for execution before the clear landed (a real possibility
in Node's event-loop ordering, even if not always observed in
practice), this branch could still run and incorrectly mark
`pollTimedOut`. Move the flag assignment to the catch block where
the settled cause is unambiguous via `instanceof
DeviceFlowPollTimeoutError`. New test pins the negative: provider
beats the timeout → no spurious `lost_late_poll_after_timeout`
audit even after ticking 2× the ceiling.

#2 (deviceFlow.ts) — late-rejection observer interpolated raw
`lateErr.name` into the audit hint without sanitization. Same
attacker-controlled vector closed at the provider layer for
`err.name` in round-4. Route through `sanitizeForStderr`.

#3 (deviceFlow.ts) — late-success observer interpolated
`latePollResult.kind` directly into the audit template. While the
typed shape is `'pending' | 'slow_down' | 'success' | 'error'`, a
non-conforming provider could return an arbitrary string. Same
log-injection vector. Route through `sanitizeForStderr`.

#4 (qwenDeviceFlowProvider.ts → deviceFlow.ts) —
`sanitizeForStderr` only stripped ASCII C0/C1 + DEL; bypass via
Unicode lookalikes:
  - U+2028/U+2029: LINE/PARAGRAPH SEPARATOR (newline-equivalent in
    most Unicode-aware terminals — most direct log-forging vector)
  - U+200B–U+200F: zero-width chars + LRM/RLM
  - U+202A–U+202E: bidirectional override controls
  - U+FEFF: BOM / ZWNBSP

A malicious IdP returning `slow_down
[serve] FAKE` in
`oauthError` would otherwise still forge log lines.

Architectural change: `sanitizeForStderr` was previously private to
`qwenDeviceFlowProvider.ts`. To address #2/#3, the registry layer
needs to call it too. Lifted into `deviceFlow.ts` (the foundation
module) and re-imported from the provider. Single source of truth;
the regex is now a module-level constant compiled once with explicit
`\uXXXX` escapes (via `String.raw` so the source is greppable, not
literal-Unicode-laden).

Tests:
- `does NOT attach late-poll observer when the provider beats the
  timeout` — N1 race regression
- `sanitizes hostile latePollResult.kind in late-observer audit` — N3
- `sanitizes hostile lateErr.name in late-rejection observer audit` — N2
- `sanitizes Unicode lookalike controls (U+2028 LINE SEPARATOR,
  bidi, ZWNBSP) in oauthError` — N4

cli serve 706/706 (was 702, +4 — all new round-5 tests); sdk
421/421; typecheck clean; eslint --max-warnings 0 clean on touched
files.

Refs: #4175, #4255, #4291, #4305

🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)

* fix(serve): address gpt-5.5 + qwen-latest review on #4305 round-5 (5 threads)

Round-6 fold-in. Five findings split between maintainability,
security hardening, and a real defensive bug.

#1 (qwenDeviceFlowProvider.test.ts) — gpt-5.5: round-5 #4 test
embedded U+2028 / U+200E / U+FEFF as literal characters in source.
Invisible in GitHub diffs / most editors; the negative
`not.toContain('')` looked like an empty-string check. Rewrote
the payload + assertions to use named `\uXXXX`-bound constants.
Also added a companion test exercising U+2066–U+2069 (round-6 #5
below).

#2 (deviceFlow.ts) — qwen-latest: the late-poll observer's
`void tracked.then(...)` was missing a terminal `.catch(() => {})`.
A synchronous throw inside either handler (e.g., a misbehaving
`audit.record`: backpressure, malformed payload, sink out-of-disk)
would reject the derived promise unhandled. On Node 22's default
`--unhandled-rejections=throw`, that crashes the daemon. Added the
terminal `.catch(() => {})` matching the persist-tracker pattern.
New test injects a poison audit sink that throws specifically on
the `lost_late_poll_after_timeout` call; asserts `flushAsync()`
resolves cleanly.

#3 (deviceFlow.ts) — qwen-latest: the `case 'error'` audit-record
hint interpolated `rawProviderError` (raw `err.message`) without
`sanitizeForStderr`. Per ES2019+ `JSON.stringify` no longer escapes
U+2028/U+2029 — those would still forge log lines downstream
through file/stdout audit sinks. Apply the same sanitizer used on
every other provider-controlled audit path. New test pins a hostile
provider message containing U+2028 + ANSI escape and asserts
neither survives.

#4 (deviceFlow.ts) — qwen-latest: the round-5 #1 comment claimed
"`DeviceFlowPollTimeoutError` isn't exported as a public DeviceFlow
contract", but it IS `export class` (the test file constructs it
directly for fixtures). With `pollTimedOut = true` keyed solely on
`instanceof`, a future provider that imports + throws the class
would spoof the registry's "I caused the timeout" signal —
attaching a phantom late-poll observer.

Fix: introduce a runtime brand `_isRegistryTimeout: boolean` on the
class (default `false`) plus an internal-only
`makeRegistryPollTimeoutError(ms)` helper that sets the brand to
`true`. The brand is set ONLY at the registry's race-timer
construction site. Both gates updated:
  - `if (err instanceof X && err._isRegistryTimeout === true)` in
    the catch (for `pollTimedOut`)
  - `if (lateErr instanceof X && lateErr._isRegistryTimeout === true)`
    in the late-rejection self-filter

A provider-thrown brand-false instance now flows through the
generic provider-throw audit path — correctly auditing the misuse
rather than silently swallowing it. Repurposed the original "no
double-audit when registry's own DeviceFlowPollTimeoutError is
late-rejected" test (which was actually exercising the brand-false
path) into the inverted assertion: brand-false provider throw IS
audited as a real failure. Removed the orphaned old assertion; the
brand-true happy path is implicitly covered by the hanging-provider
test (which exercises the registry-built timeout end-to-end).

#5 (deviceFlow.ts) — qwen-latest: `sanitizeForStderr` regex covered
U+202A–U+202E (bidi embedding/override) but missed U+2066–U+2069
(LRI/RLI/FSI/PDI). These are the primary CVE-2021-42574
("Trojan Source") attack vectors — a hostile IdP swapping U+2066
for U+202D achieves the same visual reordering and would have
bypassed the round-5 filter entirely. Extended the regex range and
JSDoc; new test exercises U+2066/U+2068/U+2069 in `oauthError` and
asserts none survive while substantive ASCII parts remain.

cli serve 713/713 (was 710, +3 round-6 tests + the round-5 #4
rewrite + the round-6 #5 companion); typecheck clean across all 4
workspaces; eslint --max-warnings 0 clean on touched files.

Refs: #4175, #4255, #4291, #4305

🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)

* fix(serve): replace literal U+2028 with explicit 
 escape in round-6 #3 test

PR #4312 review (Copilot): the round-6 #3 test (sanitizes
rawProviderError) regressed back to embedding a literal U+2028
character in source via `const U_2028 = ' '`. That's the same
maintainability anti-pattern round-6 #1 was fixing in the sister
test. Internal-consistency fix: switch to the explicit `
`
escape so the constant is greppable and reviewable in GitHub diffs.

Refs: #4291, #4305, #4312

🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)

* fix(serve): post-merge fixes for #4291 review (7 threads) (#4305)

* fix(serve): address qwen-latest review on merged #4291 (7 threads)

Seven post-merge findings from the qwen-latest review on #4291,
all real. Most are tightening fixes for issues introduced by the
earlier rounds of #4291 — the same security / DRY / observability
classes the original review surfaced, applied to surfaces that
weren't covered initially.

#1 (deviceFlow.ts:1179) — late-poll observer closure retained the
entire entry by reference (deviceCode/pkceVerifier BrandedSecrets +
cancelController) for the lifetime of the daemon if `provider.poll()`
never settled. Memory leak + indefinite secret retention. Destructure
the four fields the closure actually needs (deviceFlowId, providerId,
initiatorClientId, audit sink) so the entry is GC-eligible the
moment runPollTick returns.

#2 (server.ts) — `callerIsInitiator` was duplicated verbatim across
three locations: GET handler, toDeviceFlowStartResponseBody,
toDeviceFlowStateBody. The exact bug class #4291 was fixing was
"POST and GET diverged on the same redaction policy" — duplicating
the gate recreated the preconditions for divergence. Extracted to
shared `callerIsDeviceFlowInitiator(view, callerClientId)` helper
with the consolidated threat-model JSDoc. All three sites now call
the helper.

#3 (deviceFlow.ts:1110) — timeout callback constructed two separate
`DeviceFlowPollTimeoutError` instances (one for `signal.reason`, one
for the wrapper rejection). Each capture its own V8 stack trace,
and `signal.reason.stack` would diverge from the caught rejection's
stack — confusing for operators inspecting both. Build the sentinel
ONCE per timer fire and pass the same instance to both sites.

#4 (qwenDeviceFlowProvider.ts:273) — `Error.name` is a freely
assignable string property; a hostile fetch wrapper could set
`e.name = 'X\n[serve] FAKE LINE\x1b[31m'` to inject log lines or
ANSI sequences via the same vector we already closed for `oauthError`.
The non-OAuth catch path interpolated `${err.name}` raw. Apply the
same `sanitizeForStderr()` helper.

#5 (deviceFlow.ts:1551) — on the timeout path, `rawProviderError`
is undefined (deliberately, to skip the misleading
`provider.poll() threw (raw): ...` audit template), but that left
the audit hint field omitted entirely. Operators reading the
durable audit trail saw `errorKind: 'upstream_error'` with no signal
whether it was a hung IdP or a generic provider failure. Use
`result.hint` (which already carries the timeout-specific
`provider.poll() timed out after Nms; check IdP connectivity` text
built in the catch) so the audit matches the SSE event.

#6 (server.ts) — the `QWEN_SERVE_DEBUG` env-var check was inlined
in the GET route handler, duplicating the `isServeDebugMode()`
helper from `./debugMode.js` that workspaceAgents and
workspaceMemory already use. The inline copy also had a dead `?? ''`
fallback (the value is guaranteed truthy at that point per the
preceding check). Use the canonical helper.

#7 (deviceFlow.ts:1217) — late-rejection observer interpolated the
raw `lateErr.message` into the audit hint (truncated to 256 bytes,
but RFC 8628 `device_code` values fit comfortably in 256 bytes).
The provider's catch already uses the `name + length` redaction
pattern to prevent WAF-echoed `device_code`/PKCE leaks; the
registry layer was undoing that hardening because the same failure
settled late. Apply the same `name + length` pattern at the late-
rejection site.

Tests:
- Existing late-rejection test reseeded with a `device-code-secret-*`
  substring inside the long detail; hard-negative-asserts the seeded
  secret is absent from the audit + asserts the new
  `Error (message N bytes; raw suppressed)` shape.
- Existing poll-timeout test now also asserts: hint IS defined on
  the audit (not omitted), hint contains `'timed out after'` /
  `'check IdP connectivity'`, and `signal.reason instanceof
  DeviceFlowPollTimeoutError` (proves the single sentinel is
  shared between abort and reject).
- New `sanitizes control characters in attacker-controlled
  err.name` test in qwenDeviceFlowProvider.test.ts pins the round-4
  #4 fix with a hostile `e.name` containing `\n` + `\x1b[31m...`.

cli serve 702/702 (was 686, +16 — additional tests imported via
the acp-bridge package lift on main); sdk 421/421; typecheck clean
across all 4 workspaces; eslint --max-warnings 0 clean on touched
files.

Refs: #4175, #4255, #4291

🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)

* fix(serve): address deepseek-v4-pro review on #4305 (4 threads)

Round-5 fold-in. Four findings from the deepseek-v4-pro review on
PR #4305 — all real, three are sister fixes for the same security
classes that #4305 already closed at adjacent surfaces.

#1 (deviceFlow.ts) — `pollTimedOut` race correctness. The flag was
set unconditionally inside the timer callback. If the provider
settled the wrapper at 29.9s, `finally` would call
`clearScheduled(pollTimer)` — but if the timer callback was already
queued for execution before the clear landed (a real possibility
in Node's event-loop ordering, even if not always observed in
practice), this branch could still run and incorrectly mark
`pollTimedOut`. Move the flag assignment to the catch block where
the settled cause is unambiguous via `instanceof
DeviceFlowPollTimeoutError`. New test pins the negative: provider
beats the timeout → no spurious `lost_late_poll_after_timeout`
audit even after ticking 2× the ceiling.

#2 (deviceFlow.ts) — late-rejection observer interpolated raw
`lateErr.name` into the audit hint without sanitization. Same
attacker-controlled vector closed at the provider layer for
`err.name` in round-4. Route through `sanitizeForStderr`.

#3 (deviceFlow.ts) — late-success observer interpolated
`latePollResult.kind` directly into the audit template. While the
typed shape is `'pending' | 'slow_down' | 'success' | 'error'`, a
non-conforming provider could return an arbitrary string. Same
log-injection vector. Route through `sanitizeForStderr`.

#4 (qwenDeviceFlowProvider.ts → deviceFlow.ts) —
`sanitizeForStderr` only stripped ASCII C0/C1 + DEL; bypass via
Unicode lookalikes:
  - U+2028/U+2029: LINE/PARAGRAPH SEPARATOR (newline-equivalent in
    most Unicode-aware terminals — most direct log-forging vector)
  - U+200B–U+200F: zero-width chars + LRM/RLM
  - U+202A–U+202E: bidirectional override controls
  - U+FEFF: BOM / ZWNBSP

A malicious IdP returning `slow_down
[serve] FAKE` in
`oauthError` would otherwise still forge log lines.

Architectural change: `sanitizeForStderr` was previously private to
`qwenDeviceFlowProvider.ts`. To address #2/#3, the registry layer
needs to call it too. Lifted into `deviceFlow.ts` (the foundation
module) and re-imported from the provider. Single source of truth;
the regex is now a module-level constant compiled once with explicit
`\uXXXX` escapes (via `String.raw` so the source is greppable, not
literal-Unicode-laden).

Tests:
- `does NOT attach late-poll observer when the provider beats the
  timeout` — N1 race regression
- `sanitizes hostile latePollResult.kind in late-observer audit` — N3
- `sanitizes hostile lateErr.name in late-rejection observer audit` — N2
- `sanitizes Unicode lookalike controls (U+2028 LINE SEPARATOR,
  bidi, ZWNBSP) in oauthError` — N4

cli serve 706/706 (was 702, +4 — all new round-5 tests); sdk
421/421; typecheck clean; eslint --max-warnings 0 clean on touched
files.

Refs: #4175, #4255, #4291, #4305

🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)

* fix(serve): address gpt-5.5 + qwen-latest review on #4305 round-5 (5 threads)

Round-6 fold-in. Five findings split between maintainability,
security hardening, and a real defensive bug.

#1 (qwenDeviceFlowProvider.test.ts) — gpt-5.5: round-5 #4 test
embedded U+2028 / U+200E / U+FEFF as literal characters in source.
Invisible in GitHub diffs / most editors; the negative
`not.toContain('')` looked like an empty-string check. Rewrote
the payload + assertions to use named `\uXXXX`-bound constants.
Also added a companion test exercising U+2066–U+2069 (round-6 #5
below).

#2 (deviceFlow.ts) — qwen-latest: the late-poll observer's
`void tracked.then(...)` was missing a terminal `.catch(() => {})`.
A synchronous throw inside either handler (e.g., a misbehaving
`audit.record`: backpressure, malformed payload, sink out-of-disk)
would reject the derived promise unhandled. On Node 22's default
`--unhandled-rejections=throw`, that crashes the daemon. Added the
terminal `.catch(() => {})` matching the persist-tracker pattern.
New test injects a poison audit sink that throws specifically on
the `lost_late_poll_after_timeout` call; asserts `flushAsync()`
resolves cleanly.

#3 (deviceFlow.ts) — qwen-latest: the `case 'error'` audit-record
hint interpolated `rawProviderError` (raw `err.message`) without
`sanitizeForStderr`. Per ES2019+ `JSON.stringify` no longer escapes
U+2028/U+2029 — those would still forge log lines downstream
through file/stdout audit sinks. Apply the same sanitizer used on
every other provider-controlled audit path. New test pins a hostile
provider message containing U+2028 + ANSI escape and asserts
neither survives.

#4 (deviceFlow.ts) — qwen-latest: the round-5 #1 comment claimed
"`DeviceFlowPollTimeoutError` isn't exported as a public DeviceFlow
contract", but it IS `export class` (the test file constructs it
directly for fixtures). With `pollTimedOut = true` keyed solely on
`instanceof`, a future provider that imports + throws the class
would spoof the registry's "I caused the timeout" signal —
attaching a phantom late-poll observer.

Fix: introduce a runtime brand `_isRegistryTimeout: boolean` on the
class (default `false`) plus an internal-only
`makeRegistryPollTimeoutError(ms)` helper that sets the brand to
`true`. The brand is set ONLY at the registry's race-timer
construction site. Both gates updated:
  - `if (err instanceof X && err._isRegistryTimeout === true)` in
    the catch (for `pollTimedOut`)
  - `if (lateErr instanceof X && lateErr._isRegistryTimeout === true)`
    in the late-rejection self-filter

A provider-thrown brand-false instance now flows through the
generic provider-throw audit path — correctly auditing the misuse
rather than silently swallowing it. Repurposed the original "no
double-audit when registry's own DeviceFlowPollTimeoutError is
late-rejected" test (which was actually exercising the brand-false
path) into the inverted assertion: brand-false provider throw IS
audited as a real failure. Removed the orphaned old assertion; the
brand-true happy path is implicitly covered by the hanging-provider
test (which exercises the registry-built timeout end-to-end).

#5 (deviceFlow.ts) — qwen-latest: `sanitizeForStderr` regex covered
U+202A–U+202E (bidi embedding/override) but missed U+2066–U+2069
(LRI/RLI/FSI/PDI). These are the primary CVE-2021-42574
("Trojan Source") attack vectors — a hostile IdP swapping U+2066
for U+202D achieves the same visual reordering and would have
bypassed the round-5 filter entirely. Extended the regex range and
JSDoc; new test exercises U+2066/U+2068/U+2069 in `oauthError` and
asserts none survive while substantive ASCII parts remain.

cli serve 713/713 (was 710, +3 round-6 tests + the round-5 #4
rewrite + the round-6 #5 companion); typecheck clean across all 4
workspaces; eslint --max-warnings 0 clean on touched files.

Refs: #4175, #4255, #4291, #4305

🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)

* fix(serve): replace literal U+2028 with explicit 
 escape in round-6 #3 test

PR #4312 review (Copilot): the round-6 #3 test (sanitizes
rawProviderError) regressed back to embedding a literal U+2028
character in source via `const U_2028 = ' '`. That's the same
maintainability anti-pattern round-6 #1 was fixing in the sister
test. Internal-consistency fix: switch to the explicit `
`
escape so the constant is greppable and reviewable in GitHub diffs.

Refs: #4291, #4305, #4312

🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)

* fix(serve): post-merge P2 corrections from Codex review on #4282 (#4297)

* fix(serve): post-merge P2 corrections from Codex review on #4282

Follow-up to PR #4282 (Wave 4 PR 17) addressing four P2 issues
flagged by Codex's `/review` after the squash-merge to main:

P2-1 — Read the workspace context filename for init
  `qwen serve` parent never goes through `loadCliConfig`, so the
  process-global `getCurrentGeminiMdFilename()` stays on the default
  `QWEN.md` even when the workspace configures
  `context.fileName: 'AGENTS.md'`. `runQwenServe` now snapshots the
  workspace's merged setting at boot and forwards via
  `BridgeOptions.contextFilename`, so init writes the same file the
  ACP child reads.

P2-2 — Restart MCP servers with a fresh disabledTools snapshot
  `Config.disabledTools` was frozen at construction time;
  `setWorkspaceToolEnabled` only updated settings.json. The
  documented "toggle + restart" workflow re-registered just-disabled
  tools because rediscovery still saw the bootstrap snapshot. Added
  `Config.setDisabledTools()` plus a re-read at the ACP restart
  handler so `discoverMcpToolsForServer` honors the latest set.

P2-3 — Match the SDK timeout to the daemon's restart budget
  Bridge waits up to 300s for stdio MCP discovery; SDK helper used
  the client-wide 30s default and aborted valid slow restarts.
  Added a per-call `timeoutMs` plumbed through `fetchWithTimeout`,
  defaulting `restartMcpServer` to 5 minutes.

P2-4 — Reject symlinked parent directories before init writes
  `lstat(target)` only checked the final component; a symlinked
  parent (e.g. `docs -> /tmp` with `context.fileName:
  'docs/QWEN.md'`) would let `writeFile` follow the link and create
  / truncate outside `boundWorkspace`. Added
  `canonicalizeExistingAncestor` (walks up through ENOENT to the
  deepest extant ancestor, then `realpath`s) and verifies the
  canonical parent stays within the canonical workspace.

5 new tests (4 bridge / 2 SDK):
- contextFilename snapshot honored
- parent-symlink escape rejected
- nested real subdir accepted
- restartMcpServer survives 1.2s response with 1s default timeout
- restartMcpServer honors a 50ms caller override

Typecheck clean across cli / sdk-typescript / core.
1604/1604 unit tests pass.

* fix(serve): fold-in 1 — address 16:32:44-round review on #4282

Follow-up addressing the 8 unresolved review threads opened on PR
shipping in this same #4297; addresses correctness gaps + missing
test coverage that would otherwise let regressions ride into main.

Behavior fix:
- broadcastWorkspaceEvent gains a `skipSessionId` parameter; when
  `setSessionApprovalMode` runs with `persist:true`, the broadcast
  skips the requesting session so it doesn't receive the same
  `approval_mode_changed` event twice (once via session-scoped
  publish + once via broadcast). The SDK reducer's
  `approvalModeChangedCount` now increments by 1, not 2, on the
  requesting client (peers still see 1 via the broadcast).
  Addresses #3260501134.

Observability + posture:
- broadcastWorkspaceEvent now mirrors PR 16's publishWorkspaceEvent
  member: per-entry success/failure accounting + an "ALL buses
  dropped" stderr elevation. The previous local helper silently
  swallowed every publish failure. Addresses #3260501126.
- WorkspaceInitPathEscapeError + WorkspaceInitSymlinkError typed
  classes for the two boundary guards in initWorkspace, mapped to
  HTTP 400 by sendBridgeError. Previous generic `Error` fell
  through to the 500 handler, telling operators "daemon broken"
  when the actual fix was workspace-config correction. Addresses
  #3260501161.

Public surface symmetry:
- Re-export McpServerNotFoundError, McpServerRestartFailedError,
  WorkspaceInitPathEscapeError, WorkspaceInitSymlinkError from the
  serve barrel. External embeds matching these via `instanceof`
  no longer need deep imports. Addresses #3260501163.

Test coverage:
- restartMcpServer bridge tests (5): success + event broadcast,
  soft-skip + refused event, McpServerNotFoundError translation,
  McpServerRestartFailedError translation, originator clientId
  stamping. Addresses #3260501141.
- sendBridgeError mapping tests (4): McpServerNotFoundError → 404,
  McpServerRestartFailedError → 502, WorkspaceInitPathEscapeError
  → 400, WorkspaceInitSymlinkError → 400. Addresses #3260501148.
- initWorkspace boundary guard tests (2 added): symlink-at-target
  rejected, contextFilename '../outside.md' rejected. Addresses
  #3260501157.
- TrustGateError tests assert the typed class via `.toThrow(TrustGateError)`,
  not just message text. Addresses #3260501165.

Also updates the existing fold-in 4 S2 broadcast test to reflect
the new no-duplicate semantics on the requesting session.

Typecheck clean across cli / sdk-typescript / core.
1615/1615 unit tests pass.

* fix(serve): fold-in 2 — copilot + wenshao review on #4297

Round-2 reviewer adoption on the same PR:

Critical fixes:
- `restartMcpServer` JSDoc documents `timeoutMs: 0` as "disable the
  timeout entirely", but the `> 0` guard in `fetchWithTimeout`
  rejected `0` and silently fell back to the 30s client default.
  Loosened the guard to `>= 0` so `0` flows through to the
  no-timeout branch via the existing truthiness check; NaN /
  negative inputs still coerce to the client default. Addresses
  duplicate reports from copilot (#3260577538) and wenshao
  (#3260661833).
- TS2322 in the slow-fetch test stub: `resolveResponse` was typed
  against `import('undici-types').Response` but assigned a
  `(v: Response) => void`. Re-typed against the global `Response`
  throughout. Caught only by tsc runs that include the test
  files. Addresses #3260663072.

Test fidelity:
- Slow-fetch stub now observes `init.signal` and rejects on abort,
  so a regression that drops the per-call `timeoutMs` override
  will reliably fail the test instead of resolving after the
  timer fired (false-negative coverage). Addresses #3260577600.
- New test pinning the `timeoutMs: 0` semantics: 1ms client
  default + a stub that resolves after 50ms. Without the `>= 0`
  fix, the call would abort at 1ms; with it, the explicit
  `0` disables the timer and the call completes.

Bug fixes:
- `runQwenServe.contextFilenameForInit` previously called
  `String(arr[0])` on the array branch, producing a literal
  `"[object Object]"` filename for hand-edited bad data. Now
  validates each element with `typeof === 'string'` and falls
  back to `undefined` (so the bridge uses its
  `getCurrentGeminiMdFilename()` default) when no string is
  found. Addresses #3260577641.

Documentation drift:
- `Config.getDisabledTools()` JSDoc rewritten to describe the
  mutable-via-`setDisabledTools()` semantics introduced by P2-2,
  and the "registration-time only / no retroactive unregister"
  contract that pairs with it. Old comment claimed the set was
  frozen at construction. Addresses #3260577677.

Observability:
- `acpAgent` MCP-restart `loadSettings` failure now surfaces a
  stderr line naming the server + the underlying error, instead
  of silently swallowing it. The documented "toggle + restart"
  workflow used to break with zero diagnostic when settings.json
  was corrupted or unreadable. Addresses #3260663303.

Code organization:
- Moved `canonicalizeExistingAncestor` after `describeStatKind` so
  the latter's JSDoc is no longer orphaned (TypeScript only
  associates the last `/** ... */` block before a declaration).
  Addresses #3260668618.

Typecheck clean across cli / sdk-typescript / core.
1616/1616 unit tests pass.

* fix(serve): fold-in 3 — read merged scope on MCP restart refresh

Critical bug from wenshao review (#3260725526) on PR #4297:
the P2-2 acpAgent re-read narrowed `Config.disabledTools` to
`SettingScope.Workspace` alone, dropping User / System scope
entries. The bootstrap Config received `merged.tools?.disabled`
(union of all scopes), so user-level / system-level disables
worked at boot — but the first `mcp restart` would replace the
in-memory set with the workspace scope alone, silently re-enabling
any tool that was disabled at a higher scope but absent from the
workspace file.

The asymmetry vs. the persist-write path is deliberate and
documented:
- Reads (here): merged — match the bootstrap Config snapshot,
  preserve user/system policy.
- Writes (`runQwenServe.persistDisabledTools`): workspace scope —
  don't bake higher-scope entries into the workspace file
  (per-#4282 fold-in 1 H2 fix).

Two paths look alike but answer different questions.

Typecheck clean across cli / sdk-typescript / core.
1616/1616 unit tests pass.

* fix(test): fold-in 4 — wire timeoutMs:0 stub to init.signal

Critical follow-up from wenshao (#3260810242) on PR #4297:
the new `timeoutMs: 0` regression test (added in fold-in 2)
inherited the same flaw it was meant to prevent — the slow-fetch
stub didn't observe `init.signal`, so a regression that ignored
the `0` override would fire the AbortController at the 1ms client
default but the stub would keep the promise pending. The 50ms
`resolveResponse` would win, the test would still pass, and the
documented "0 disables timeout" contract would be unprotected.

Mirrored the listener pattern already used by the two sibling
tests in fold-in 2 — `init.signal.addEventListener('abort', () =>
reject(...))`. Now a regression that re-rejects `0` triggers the
abort, the stub rejects, the test fails.

8/8 restartMcpServer SDK tests pass; SDK typecheck clean.

* fix(serve): fold-in 5 — TOCTOU + setDisabledTools coverage

Two new critical reviews from wenshao on PR #4297:

C1 — TOCTOU between lstat and writeFile (#3260836305):
The `lstat(target)` symlink check and the subsequent `writeFile`
were two separate syscalls, leaving a race window where a local
attacker with workspace write access could substitute a symlink
between them. With `force: true`, `writeFile` would follow the
link and truncate an external target.

The `action === 'created'` path now uses `fs.open(target, 'wx')`
(O_WRONLY|O_CREAT|O_EXCL), which atomically refuses any
pre-existing inode (regular file, dir, OR symlink) at the target
path. EEXIST after the absence check most plausibly means a
race-created symlink, so we throw `WorkspaceInitSymlinkError(kind:
'target')` — same typed class the route maps to 400.

The `force: true` overwrite path retains the existing TOCTOU as a
documented limitation; closing it requires `O_NOFOLLOW`-aware open
which the post-PR18 `WorkspaceFileSystem` migration will provide.

C2 — P2-2 zero test coverage (#3260836302):
The `setDisabledTools` runtime sync was the only Wave-4 P2 fix
without a dedicated test. Added 5 Config-level tests:
- Initializes from `disabledTools` ConfigParameters
- Defaults to empty set when omitted
- `setDisabledTools` replaces the live snapshot
- Defensive copy: caller-set mutations don't leak into the live snapshot
- Accepts an empty set (clears live snapshot)

Plus a TOCTOU regression test in httpAcpBridge.test.ts that
spies fs.lstat / fs.readFile to simulate the race window:
pre-creates a symlink, makes lstat lie about it, asserts the
'wx' open catches the racing inode and throws the typed
`WorkspaceInitSymlinkError(kind: 'target')`.

1622/1622 unit tests pass; typecheck clean across cli /
sdk-typescript / core.

* fix(serve): fold-in 6 — count actual skips in broadcast alarm

DeepSeek review on #4297 (#3261079572):
`broadcastWorkspaceEvent` unconditionally subtracted 1 from the
`eligible` recipient count whenever `skipSessionId` was set, even
when the id matched zero live sessions (caller mistake, stale id,
or the matching session was just torn down between resolution and
broadcast). In a single-session workspace that's the difference
between `eligible = 0` (alarm suppressed) and `eligible = 1`
(alarm fires when the publish failed) — silently losing the
all-dropped breadcrumb the telemetry was meant to surface.

Today's call sites pass real session ids so the bug doesn't
manifest in practice, but the defensive shape is small: track
`skippedCount` inside the loop and subtract that, so the alarm
condition is self-consistent regardless of how the caller mis-uses
the param.

162/162 bridge tests pass; CLI typecheck clean.

* fix(serve): fold-in 7 — close overwrite TOCTOU, harden boot + diagnostics

Round-7 review on PR #4297. Three critical fixes + one suggestion
test, plus a regression test for the overwrite TOCTOU close.

C1 — force:true overwrite TOCTOU (#3262615446):
The fold-in 5 fix only closed the `'created'` action via 'wx';
the `'overwrote'` branch still used plain `fs.writeFile`, so a
local writer could swap the verified regular file to a symlink
between the lstat/readFile checks and the write and have the
forced overwrite truncate an external target. Switched to
`fs.open(target, O_WRONLY | O_TRUNC | O_NOFOLLOW)` — `O_NOFOLLOW`
makes open() fail with ELOOP on a symlink at the final component
even under race. ELOOP / ENOENT (race-deleted) translate to
`WorkspaceInitSymlinkError(kind: 'target')` so the route still
maps to a structured 400 instead of a generic 500.

C2 — settings.json corrupt blocks daemon boot (#3262625091):
`loadSettings(boundWorkspace)` at boot had no try/catch — a
corrupted, malformed, or temporarily unreadable settings file
threw synchronously and prevented daemon startup. Pre-PR this
never happened because settings were read lazily inside request
handlers. Wrapped in try/catch with stderr fallback so the daemon
keeps booting (with the bridge's default context filename) when
the file is broken.

C3 — malformed `tools.disabled` clears policy silently (#3262625101):
When `merged.tools?.disabled` is present but not an array
(boolean / string / object from a hand-edited settings.json), the
ternary `Array.isArray(...) ? ... : []` substituted an empty list
without firing the surrounding catch block. After an MCP restart
every disabled tool would silently re-register. Added an explicit
`!Array.isArray && !== undefined` check that stderr-logs the
malformed type before clearing — operators see the
misconfiguration instead of a stealth re-enable.

S1 — contextFilename extraction tested (#3262690842):
Lifted the inline `firstStringInArray` + branching into an
exported `extractContextFilename(value: unknown)` helper and
added `runQwenServe.test.ts` with 5 tests covering the four
branches the suggestion called out: non-empty string, array with
strings, array with no strings, non-string non-array.

Plus a TOCTOU regression test for the overwrite path that
verifies `O_NOFOLLOW` returns `WorkspaceInitSymlinkError(kind:
'target')` when the file is race-substituted with a symlink
behind the lstat/readFile mocks.

S2 (acpAgent restart-handler integration test #3262690845) is
deferred — Config-level coverage of `setDisabledTools` already
locks the load-bearing surface (5 tests in fold-in 5), and
adding a full acpAgent integration test requires heavy ext-method
plumbing. The new C3 stderr diagnostic plus existing tests give
us the regression signal we need without that scaffolding.

1627/1627 unit tests pass; typecheck clean across cli /
sdk-typescript / core / acp-bridge.

* fix(serve): fold-in 8 — split ELOOP / ENOENT diagnostic in overwrite path

qwen-latest review on PR #4297 (#3262861754):
The fold-in 7 ELOOP/ENOENT branch shared one error message that
said "swapped to a symlink." That's accurate for ELOOP (genuine
O_NOFOLLOW rejection — likely an attack race) but misleading for
ENOENT in the overwrite path: there `readFile` just succeeded
proving the file existed, so ENOENT means the file was DELETED
between the content check and the open — a benign race with a
concurrent writer (git checkout, editor save, lockfile rename),
NOT a symlink swap. An operator seeing the symlink language for
a benign delete would `ls -la`, see no symlink, and waste time
hunting an attack that didn't happen.

Split into two messages:
- ELOOP: "swapped to a symlink between the content check and the
  overwrite — refusing to follow it"
- ENOENT: "deleted between the content check and the overwrite
  (likely a concurrent writer) — refusing to recreate blindly"

Both still surface as `WorkspaceInitSymlinkError(kind: 'target')`
so the route maps to a structured 400; the class doubles as the
workspace-init race-condition bucket with kind='target' meaning
"target inode misbehaved at write time" generally.

Updated the existing fold-in 7 TOCTOU test to assert the ELOOP
message specifically, and added a new ENOENT race-delete test
that mocks lstat/readFile to land on the overwrote action against
a non-existent path — verifies the message says "deleted" and
NOT "swapped to a symlink."

170/170 bridge tests pass; CLI typecheck clean.

* fix(serve): fold-in 9 — route MCP restart through registry cleanup wrapper

gpt-5.5 critical review on PR #4297 (#3263088414):

The fold-in 5 P2-2 fix refreshed `Config.disabledTools` from merged
settings, but then called `manager.discoverMcpToolsForServer()`
directly — bypassing the `ToolRegistry.discoverToolsForServer`
wrapper that PURGES the server's existing `DiscoveredMCPTool`
entries (and `revealedDeferred` markers) plus its prompts before
rediscovery. Without the cleanup, `registerTool` only consulted
the refreshed `disabledTools` set for NEWLY-discovered tools —
entries already in the registry from the prior MCP boot kept
serving requests. Net effect: toggle-disable-then-restart
silently left the disabled tool live, breaking the documented
"toggle + restart" workflow that P2-2 was meant to fix.

Routed through `toolRegistry.discoverToolsForServer(serverName)`
which:
1. Removes existing `DiscoveredMCPTool` entries for this server
2. Drops their `revealedDeferred` reveal state
3. Removes the server's prompts via `removePromptsByServer`
4. THEN delegates to `manager.discoverMcpToolsForServer` for the
   actual reconnect + rediscover

The pre-discovery budget / in-flight checks still go through the
`manager` reference (which is the same object the registry
wrapper would forward to) — so soft-skip semantics for
`budget_would_exceed`, `in_flight`, `disabled` are preserved.

CLI typecheck clean; 403/403 server + bridge tests pass.

* fix(serve): fold-in 10 — qwen-latest 05:45-round review on #4297

5 review threads from qwen-latest's late round on PR #4297 (now closed
in favor of #4313 against `daemon_mode_b_main`). 1 critical + 4
suggestions, all adopted.

C1 — extractContextFilename / getCurrentGeminiMdFilename divergence
(#3263954685): with `context.fileName: ['  ', 'AGENTS.md']`, the
daemon parent's `extractContextFilename` (which skips empty entries)
wrote `AGENTS.md`, but the ACP child's `getCurrentGeminiMdFilename`
(which returned `arr[0]` unconditionally) read `''`. The init'd file
was orphaned. Aligned `getCurrentGeminiMdFilename` to skip empty
entries with the same semantics, falling back to
`DEFAULT_CONTEXT_FILENAME` when all entries are empty.

S2 — WorkspaceInitSymlinkError reused for non-symlink races
(#3263954690): the EEXIST race-create and ENOENT race-delete cases
were surfacing as `code: 'workspace_init_symlink'`, misleading
operators into hunting symlink attacks for benign concurrent-
modification windows. Split into a sibling `WorkspaceInitRaceError`
class (`kind: 'eexist' | 'enoent'`, HTTP code
`workspace_init_race`). The genuine symlink class stays for ELOOP,
lstat-detected target symlinks, and parent-realpath escapes.

S3 — fsConstants.O_NOFOLLOW defensive `?? 0` (#3263954697): matches
the existing codebase convention in
`core/src/utils/{sessionStorageUtils,gitDiff}.ts` and
`cli/src/ui/utils/customBanner.ts`. Functionally a no-op (JS
bitwise coerces undefined to 0) but consistent.

S5 — Parent-directory TOCTOU still open (#3263954707): O_NOFOLLOW
only protects the final path component; a local writer could swap
a real parent dir for a symlink between
`canonicalizeExistingAncestor` and `fs.open`. Added
`verifyParentWithinWorkspace` post-open helper that re-realpaths
`path.dirname(target)` and refuses with
`WorkspaceInitSymlinkError(kind: 'parent')` if the parent moved.
On the create path (where we just opened with `'wx'`), the failure
also unlinks the file we just made best-effort. Residual race
window narrowed from "between pre-check and open" to "between
post-open realpath and writeFile" — sub-millisecond, documented as
accepted Stage-1 trust posture.

S4 — broadcastWorkspaceEvent vs publishWorkspaceEvent stale comment
(#3263954688): the "now removed" comment was inaccurate (5 call
sites still use the closure). Replaced with an accurate
description of why both coexist (factory closure can't `this`-call
proxy member; closure also takes `skipSessionId` for persisted
approval-mode mirror) and a TODO marker for future helper extraction.

Two existing tests updated to assert the new `WorkspaceInitRaceError`
class for EEXIST / ENOENT scenarios (the symlink-class assertions
are preserved for ELOOP / lstat / parent cases).

1759/1759 unit tests pass; typecheck clean across all 4 packages.

* feat(acp-bridge): F1 — acp-bridge package self-sufficiency (#4175 mechanical lift + BridgeFileSystem seam) (#4319)

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

First mechanical lift of #4175 F1 (acp-bridge package self-sufficiency).
Moves the production spawn factory + its `killChild` helper +
`SCRUBBED_CHILD_ENV_KEYS` denylist + `KILL_HARD_DEADLINE_MS` constant
from `cli/src/serve/httpAcpBridge.ts` (~283 lines) to
`@qwen-code/acp-bridge/spawnChannel`. This unblocks
`channels/base/AcpBridge.ts` and `vscode-ide-companion`'s
acpConnection from each reimplementing the child lifecycle — they can
now consume the same primitive.

Backward compatible: `cli/src/serve/httpAcpBridge.ts` imports the
lifted factory and re-exports it, so existing references in
`cli/src/serve/index.ts:90` and the factory's own internal usage
(`opts.channelFactory ?? defaultSpawnChannelFactory`) keep resolving.
Bridge tests that mock `defaultSpawnChannelFactory` via
`BridgeOptions.channelFactory` are unaffected.

Side cleanups: drops `spawn` / `ChildProcess` / `Readable` / `Writable`
/ `ndJsonStream` / `MissingCliEntryError` imports from
httpAcpBridge.ts (all only used by the lifted spawn factory).

- 44/44 acp-bridge tests pass
- 174/174 cli httpAcpBridge tests pass
- typecheck clean across acp-bridge + cli

🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)

* refactor(acp-bridge): lift BridgeClient + permission types to acp-bridge/bridgeClient (#4175 F1 step 2)

Second mechanical lift of #4175 F1 (acp-bridge package self-sufficiency).
Moves `BridgeClient` class (~700 LOC) + `PendingPermission` interface +
`PermissionResolutionRecord` interface + `MAX_RESOLVED_PERMISSION_RECORDS`
constant + early-event capacity constants + `describeStatKind` and
`sliceLineRange` helpers from `cli/src/serve/httpAcpBridge.ts` to
`@qwen-code/acp-bridge/bridgeClient`.

Design choice for SessionEntry boundary: introduce a minimal
`BridgeClientSessionEntry` interface in bridgeClient.ts with only the
four fields BridgeClient actually reads from the factory's richer
`SessionEntry` (`sessionId`, `events`, `pendingPermissionIds`,
`activePromptOriginatorClientId`). The factory's `SessionEntry`
structurally satisfies it — TypeScript's structural typing enforces
the match at the `resolveEntry` callback signature, so no explicit
conversion is required and the bridge package stays free of daemon-host
session-bookkeeping types.

Cross-package writeStderrLine handling: inline the 3-line helper in
bridgeClient.ts (mirrors the spawnChannel.ts pattern from F1 step 1)
so acp-bridge has no reverse dependency on `cli/src/utils/stdioHelpers`.

httpAcpBridge.ts shrinks from 4406 LOC to 3647 LOC (-759 lines).
Removed ACP SDK imports that only BridgeClient consumed: `Client`,
`RequestPermissionRequest`, `WriteTextFileRequest`,
`WriteTextFileResponse`, `ReadTextFileRequest`, `ReadTextFileResponse`,
`SessionNotification`. Kept the ones the factory still uses
(`CancelNotification`, `PromptRequest`, `RequestPermissionResponse`,
`SetSessionModelRequest`, `SetSessionModelResponse`).

Backward compatible: httpAcpBridge.ts re-exports `BridgeClient`,
`BridgeClientSessionEntry`, `PendingPermission`,
`PermissionResolutionRecord`, and `MAX_RESOLVED_PERMISSION_RECORDS` so
the `ChannelInfo.client: BridgeClient` field declaration below + any
embedder reaching into these types keep resolving.

- 44/44 acp-bridge tests pass
- 174/174 cli httpAcpBridge tests pass
- 229/229 cli server tests pass
- typecheck clean across acp-bridge + cli

🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)

* refactor(acp-bridge): lift createHttpAcpBridge factory to acp-bridge/bridge (#4175 F1 step 3)

Third + final mechanical lift of #4175 F1 (acp-bridge package
self-sufficiency). Moves the `createHttpAcpBridge` factory closure
(~3000 LOC) + `ChannelInfo` + `SessionEntry` interfaces + factory-only
helpers (`canonicalizeExistingAncestor`, `verifyParentWithinWorkspace`,
`withTimeout`, `isServeDebugLoggingEnabled`, `writeServeDebugLine`,
`hasControlCharacter`) + factory constants (`DEFAULT_INIT_TIMEOUT_MS`,
`MCP_RESTART_TIMEOUT_MS`, `DEFAULT_MAX_SESSIONS`, `MAX_EVENT_RING_SIZE`,
`DEFAULT_PERMISSION_TIMEOUT_MS`, `DEFAULT_MAX_PENDING_PER_SESSION`,
`MAX_DISPLAY_NAME_LENGTH`) from `cli/src/serve/httpAcpBridge.ts` to
`@qwen-code/acp-bridge/bridge`.

`cli/src/serve/httpAcpBridge.ts` shrinks from 3647 LOC to 97 LOC — a
pure re-export shim that preserves every existing relative import
path (`./httpAcpBridge.js`) so `server.ts`, `runQwenServe.ts`,
`workspaceAgents.ts`, `workspaceMemory.ts`, `index.ts`, plus the bridge
test suite, keep resolving without any call-site changes.

The new `bridge.ts` reuses what was already in acp-bridge (errors,
types, options, status helpers, channel types, event bus, workspace
paths) via local relative imports — no reverse dependency on `cli`.
`writeStderrLine` is inlined at the top of `bridge.ts` (same pattern as
`spawnChannel.ts` + `bridgeClient.ts` from F1 steps 1-2) so the
package self-contained promise holds.

Cumulative F1 impact across the 3 mechanical lift steps:
- httpAcpBridge.ts: 4682 LOC → 97 LOC (-4585 lines; the original file
  was 98% bridge core, 2% backward-compat re-exports)
- 3 new files in acp-bridge: spawnChannel.ts (~270 LOC), bridgeClient.ts
  (~745 LOC), bridge.ts (~3515 LOC)
- All daemon-host concerns (env snapshot, daemon preflight cells)
  remain in `cli/src/serve/daemonStatusProvider.ts` and reach the
  bridge through the `BridgeOptions.statusProvider` seam frozen by
  PR 22b/2.

- 735/735 cli serve tests pass across 17 files
- 174/174 cli httpAcpBridge tests pass
- 44/44 acp-bridge tests pass
- typecheck clean across acp-bridge + cli

`packages/cli/src/serve/httpAcpBridge.test.ts` (~6600 LOC) is
intentionally NOT moved in this commit — it currently imports
`createHttpAcpBridge` / `defaultSpawnChannelFactory` / `BridgeClient`
via the cli shim and keeps passing without changes. Moving it to
`acp-bridge/src/bridge.test.ts` is a follow-up worth tracking
separately so the production-code lift can land + be reviewed cleanly.

The `BridgeFileSystem` injection seam (originally bundled into F1 as
the 22b' scope) is also deferred to a follow-up so the mechanical lift
stays mechanical — design + implementation of the fs injection is its
own discussion.

🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)

* feat(acp-bridge): add BridgeFileSystem injection seam (#4175 F1 step 5, 22b' scope)

Adds the `BridgeFileSystem` injection seam originally scoped as #4175
22b'. When a `BridgeFileSystem` is wired through
`BridgeOptions.fileSystem`, `BridgeClient.readTextFile` and
`BridgeClient.writeTextFile` delegate to it instead of running their
inline `fs.realpath` / `fs.writeFile` / `fs.readFile` proxy.

This unblocks production `qwen serve` plumbing PR 18's
`WorkspaceFileSystem` (TOCTOU guards, symlink-substitution checks,
trust gate, `.gitignore`, audit hooks) into the ACP fs methods —
closing the `ws.ts:613` follow-up thread that has been tracked since
PR 18 landed. The serve-side adapter that wraps `WorkspaceFileSystem`
+ the `runQwenServe` wiring are intentionally split into the
immediate-follow-up so this PR stays focused on the seam design.

Backward compatible: `fileSystem` is optional on `BridgeOptions`.
Tests, Mode A in-process consumers, channels (`packages/channels/base/
AcpBridge.ts`), and the VSCode IDE companion all keep working
unchanged — they omit the field and `BridgeClient` falls through to
the inline proxy that has been the Stage 1 default since #3889.

API:
- `BridgeFileSystem.readText(params: ReadTextFileRequest):
  Promise<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…

…BX9_p) (#4557)

* fix(serve): post-merge fixes for #4291 review (7 threads) (#4305)

* fix(serve): address qwen-latest review on merged #4291 (7 threads)

Seven post-merge findings from the qwen-latest review on #4291,
all real. Most are tightening fixes for issues introduced by the
earlier rounds of #4291 — the same security / DRY / observability
classes the original review surfaced, applied to surfaces that
weren't covered initially.

#1 (deviceFlow.ts:1179) — late-poll observer closure retained the
entire entry by reference (deviceCode/pkceVerifier BrandedSecrets +
cancelController) for the lifetime of the daemon if `provider.poll()`
never settled. Memory leak + indefinite secret retention. Destructure
the four fields the closure actually needs (deviceFlowId, providerId,
initiatorClientId, audit sink) so the entry is GC-eligible the
moment runPollTick returns.

#2 (server.ts) — `callerIsInitiator` was duplicated verbatim across
three locations: GET handler, toDeviceFlowStartResponseBody,
toDeviceFlowStateBody. The exact bug class #4291 was fixing was
"POST and GET diverged on the same redaction policy" — duplicating
the gate recreated the preconditions for divergence. Extracted to
shared `callerIsDeviceFlowInitiator(view, callerClientId)` helper
with the consolidated threat-model JSDoc. All three sites now call
the helper.

#3 (deviceFlow.ts:1110) — timeout callback constructed two separate
`DeviceFlowPollTimeoutError` instances (one for `signal.reason`, one
for the wrapper rejection). Each capture its own V8 stack trace,
and `signal.reason.stack` would diverge from the caught rejection's
stack — confusing for operators inspecting both. Build the sentinel
ONCE per timer fire and pass the same instance to both sites.

#4 (qwenDeviceFlowProvider.ts:273) — `Error.name` is a freely
assignable string property; a hostile fetch wrapper could set
`e.name = 'X\n[serve] FAKE LINE\x1b[31m'` to inject log lines or
ANSI sequences via the same vector we already closed for `oauthError`.
The non-OAuth catch path interpolated `${err.name}` raw. Apply the
same `sanitizeForStderr()` helper.

#5 (deviceFlow.ts:1551) — on the timeout path, `rawProviderError`
is undefined (deliberately, to skip the misleading
`provider.poll() threw (raw): ...` audit template), but that left
the audit hint field omitted entirely. Operators reading the
durable audit trail saw `errorKind: 'upstream_error'` with no signal
whether it was a hung IdP or a generic provider failure. Use
`result.hint` (which already carries the timeout-specific
`provider.poll() timed out after Nms; check IdP connectivity` text
built in the catch) so the audit matches the SSE event.

#6 (server.ts) — the `QWEN_SERVE_DEBUG` env-var check was inlined
in the GET route handler, duplicating the `isServeDebugMode()`
helper from `./debugMode.js` that workspaceAgents and
workspaceMemory already use. The inline copy also had a dead `?? ''`
fallback (the value is guaranteed truthy at that point per the
preceding check). Use the canonical helper.

#7 (deviceFlow.ts:1217) — late-rejection observer interpolated the
raw `lateErr.message` into the audit hint (truncated to 256 bytes,
but RFC 8628 `device_code` values fit comfortably in 256 bytes).
The provider's catch already uses the `name + length` redaction
pattern to prevent WAF-echoed `device_code`/PKCE leaks; the
registry layer was undoing that hardening because the same failure
settled late. Apply the same `name + length` pattern at the late-
rejection site.

Tests:
- Existing late-rejection test reseeded with a `device-code-secret-*`
  substring inside the long detail; hard-negative-asserts the seeded
  secret is absent from the audit + asserts the new
  `Error (message N bytes; raw suppressed)` shape.
- Existing poll-timeout test now also asserts: hint IS defined on
  the audit (not omitted), hint contains `'timed out after'` /
  `'check IdP connectivity'`, and `signal.reason instanceof
  DeviceFlowPollTimeoutError` (proves the single sentinel is
  shared between abort and reject).
- New `sanitizes control characters in attacker-controlled
  err.name` test in qwenDeviceFlowProvider.test.ts pins the round-4
  #4 fix with a hostile `e.name` containing `\n` + `\x1b[31m...`.

cli serve 702/702 (was 686, +16 — additional tests imported via
the acp-bridge package lift on main); sdk 421/421; typecheck clean
across all 4 workspaces; eslint --max-warnings 0 clean on touched
files.

Refs: #4175, #4255, #4291

🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)

* fix(serve): address deepseek-v4-pro review on #4305 (4 threads)

Round-5 fold-in. Four findings from the deepseek-v4-pro review on
PR #4305 — all real, three are sister fixes for the same security
classes that #4305 already closed at adjacent surfaces.

#1 (deviceFlow.ts) — `pollTimedOut` race correctness. The flag was
set unconditionally inside the timer callback. If the provider
settled the wrapper at 29.9s, `finally` would call
`clearScheduled(pollTimer)` — but if the timer callback was already
queued for execution before the clear landed (a real possibility
in Node's event-loop ordering, even if not always observed in
practice), this branch could still run and incorrectly mark
`pollTimedOut`. Move the flag assignment to the catch block where
the settled cause is unambiguous via `instanceof
DeviceFlowPollTimeoutError`. New test pins the negative: provider
beats the timeout → no spurious `lost_late_poll_after_timeout`
audit even after ticking 2× the ceiling.

#2 (deviceFlow.ts) — late-rejection observer interpolated raw
`lateErr.name` into the audit hint without sanitization. Same
attacker-controlled vector closed at the provider layer for
`err.name` in round-4. Route through `sanitizeForStderr`.

#3 (deviceFlow.ts) — late-success observer interpolated
`latePollResult.kind` directly into the audit template. While the
typed shape is `'pending' | 'slow_down' | 'success' | 'error'`, a
non-conforming provider could return an arbitrary string. Same
log-injection vector. Route through `sanitizeForStderr`.

#4 (qwenDeviceFlowProvider.ts → deviceFlow.ts) —
`sanitizeForStderr` only stripped ASCII C0/C1 + DEL; bypass via
Unicode lookalikes:
  - U+2028/U+2029: LINE/PARAGRAPH SEPARATOR (newline-equivalent in
    most Unicode-aware terminals — most direct log-forging vector)
  - U+200B–U+200F: zero-width chars + LRM/RLM
  - U+202A–U+202E: bidirectional override controls
  - U+FEFF: BOM / ZWNBSP

A malicious IdP returning `slow_down
[serve] FAKE` in
`oauthError` would otherwise still forge log lines.

Architectural change: `sanitizeForStderr` was previously private to
`qwenDeviceFlowProvider.ts`. To address #2/#3, the registry layer
needs to call it too. Lifted into `deviceFlow.ts` (the foundation
module) and re-imported from the provider. Single source of truth;
the regex is now a module-level constant compiled once with explicit
`\uXXXX` escapes (via `String.raw` so the source is greppable, not
literal-Unicode-laden).

Tests:
- `does NOT attach late-poll observer when the provider beats the
  timeout` — N1 race regression
- `sanitizes hostile latePollResult.kind in late-observer audit` — N3
- `sanitizes hostile lateErr.name in late-rejection observer audit` — N2
- `sanitizes Unicode lookalike controls (U+2028 LINE SEPARATOR,
  bidi, ZWNBSP) in oauthError` — N4

cli serve 706/706 (was 702, +4 — all new round-5 tests); sdk
421/421; typecheck clean; eslint --max-warnings 0 clean on touched
files.

Refs: #4175, #4255, #4291, #4305

🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)

* fix(serve): address gpt-5.5 + qwen-latest review on #4305 round-5 (5 threads)

Round-6 fold-in. Five findings split between maintainability,
security hardening, and a real defensive bug.

#1 (qwenDeviceFlowProvider.test.ts) — gpt-5.5: round-5 #4 test
embedded U+2028 / U+200E / U+FEFF as literal characters in source.
Invisible in GitHub diffs / most editors; the negative
`not.toContain('')` looked like an empty-string check. Rewrote
the payload + assertions to use named `\uXXXX`-bound constants.
Also added a companion test exercising U+2066–U+2069 (round-6 #5
below).

#2 (deviceFlow.ts) — qwen-latest: the late-poll observer's
`void tracked.then(...)` was missing a terminal `.catch(() => {})`.
A synchronous throw inside either handler (e.g., a misbehaving
`audit.record`: backpressure, malformed payload, sink out-of-disk)
would reject the derived promise unhandled. On Node 22's default
`--unhandled-rejections=throw`, that crashes the daemon. Added the
terminal `.catch(() => {})` matching the persist-tracker pattern.
New test injects a poison audit sink that throws specifically on
the `lost_late_poll_after_timeout` call; asserts `flushAsync()`
resolves cleanly.

#3 (deviceFlow.ts) — qwen-latest: the `case 'error'` audit-record
hint interpolated `rawProviderError` (raw `err.message`) without
`sanitizeForStderr`. Per ES2019+ `JSON.stringify` no longer escapes
U+2028/U+2029 — those would still forge log lines downstream
through file/stdout audit sinks. Apply the same sanitizer used on
every other provider-controlled audit path. New test pins a hostile
provider message containing U+2028 + ANSI escape and asserts
neither survives.

#4 (deviceFlow.ts) — qwen-latest: the round-5 #1 comment claimed
"`DeviceFlowPollTimeoutError` isn't exported as a public DeviceFlow
contract", but it IS `export class` (the test file constructs it
directly for fixtures). With `pollTimedOut = true` keyed solely on
`instanceof`, a future provider that imports + throws the class
would spoof the registry's "I caused the timeout" signal —
attaching a phantom late-poll observer.

Fix: introduce a runtime brand `_isRegistryTimeout: boolean` on the
class (default `false`) plus an internal-only
`makeRegistryPollTimeoutError(ms)` helper that sets the brand to
`true`. The brand is set ONLY at the registry's race-timer
construction site. Both gates updated:
  - `if (err instanceof X && err._isRegistryTimeout === true)` in
    the catch (for `pollTimedOut`)
  - `if (lateErr instanceof X && lateErr._isRegistryTimeout === true)`
    in the late-rejection self-filter

A provider-thrown brand-false instance now flows through the
generic provider-throw audit path — correctly auditing the misuse
rather than silently swallowing it. Repurposed the original "no
double-audit when registry's own DeviceFlowPollTimeoutError is
late-rejected" test (which was actually exercising the brand-false
path) into the inverted assertion: brand-false provider throw IS
audited as a real failure. Removed the orphaned old assertion; the
brand-true happy path is implicitly covered by the hanging-provider
test (which exercises the registry-built timeout end-to-end).

#5 (deviceFlow.ts) — qwen-latest: `sanitizeForStderr` regex covered
U+202A–U+202E (bidi embedding/override) but missed U+2066–U+2069
(LRI/RLI/FSI/PDI). These are the primary CVE-2021-42574
("Trojan Source") attack vectors — a hostile IdP swapping U+2066
for U+202D achieves the same visual reordering and would have
bypassed the round-5 filter entirely. Extended the regex range and
JSDoc; new test exercises U+2066/U+2068/U+2069 in `oauthError` and
asserts none survive while substantive ASCII parts remain.

cli serve 713/713 (was 710, +3 round-6 tests + the round-5 #4
rewrite + the round-6 #5 companion); typecheck clean across all 4
workspaces; eslint --max-warnings 0 clean on touched files.

Refs: #4175, #4255, #4291, #4305

🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)

* fix(serve): replace literal U+2028 with explicit 
 escape in round-6 #3 test

PR #4312 review (Copilot): the round-6 #3 test (sanitizes
rawProviderError) regressed back to embedding a literal U+2028
character in source via `const U_2028 = ' '`. That's the same
maintainability anti-pattern round-6 #1 was fixing in the sister
test. Internal-consistency fix: switch to the explicit `
`
escape so the constant is greppable and reviewable in GitHub diffs.

Refs: #4291, #4305, #4312

🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)

* fix(serve): post-merge P2 corrections from Codex review on #4282 (#4297)

* fix(serve): post-merge P2 corrections from Codex review on #4282

Follow-up to PR #4282 (Wave 4 PR 17) addressing four P2 issues
flagged by Codex's `/review` after the squash-merge to main:

P2-1 — Read the workspace context filename for init
  `qwen serve` parent never goes through `loadCliConfig`, so the
  process-global `getCurrentGeminiMdFilename()` stays on the default
  `QWEN.md` even when the workspace configures
  `context.fileName: 'AGENTS.md'`. `runQwenServe` now snapshots the
  workspace's merged setting at boot and forwards via
  `BridgeOptions.contextFilename`, so init writes the same file the
  ACP child reads.

P2-2 — Restart MCP servers with a fresh disabledTools snapshot
  `Config.disabledTools` was frozen at construction time;
  `setWorkspaceToolEnabled` only updated settings.json. The
  documented "toggle + restart" workflow re-registered just-disabled
  tools because rediscovery still saw the bootstrap snapshot. Added
  `Config.setDisabledTools()` plus a re-read at the ACP restart
  handler so `discoverMcpToolsForServer` honors the latest set.

P2-3 — Match the SDK timeout to the daemon's restart budget
  Bridge waits up to 300s for stdio MCP discovery; SDK helper used
  the client-wide 30s default and aborted valid slow restarts.
  Added a per-call `timeoutMs` plumbed through `fetchWithTimeout`,
  defaulting `restartMcpServer` to 5 minutes.

P2-4 — Reject symlinked parent directories before init writes
  `lstat(target)` only checked the final component; a symlinked
  parent (e.g. `docs -> /tmp` with `context.fileName:
  'docs/QWEN.md'`) would let `writeFile` follow the link and create
  / truncate outside `boundWorkspace`. Added
  `canonicalizeExistingAncestor` (walks up through ENOENT to the
  deepest extant ancestor, then `realpath`s) and verifies the
  canonical parent stays within the canonical workspace.

5 new tests (4 bridge / 2 SDK):
- contextFilename snapshot honored
- parent-symlink escape rejected
- nested real subdir accepted
- restartMcpServer survives 1.2s response with 1s default timeout
- restartMcpServer honors a 50ms caller override

Typecheck clean across cli / sdk-typescript / core.
1604/1604 unit tests pass.

* fix(serve): fold-in 1 — address 16:32:44-round review on #4282

Follow-up addressing the 8 unresolved review threads opened on PR
shipping in this same #4297; addresses correctness gaps + missing
test coverage that would otherwise let regressions ride into main.

Behavior fix:
- broadcastWorkspaceEvent gains a `skipSessionId` parameter; when
  `setSessionApprovalMode` runs with `persist:true`, the broadcast
  skips the requesting session so it doesn't receive the same
  `approval_mode_changed` event twice (once via session-scoped
  publish + once via broadcast). The SDK reducer's
  `approvalModeChangedCount` now increments by 1, not 2, on the
  requesting client (peers still see 1 via the broadcast).
  Addresses #3260501134.

Observability + posture:
- broadcastWorkspaceEvent now mirrors PR 16's publishWorkspaceEvent
  member: per-entry success/failure accounting + an "ALL buses
  dropped" stderr elevation. The previous local helper silently
  swallowed every publish failure. Addresses #3260501126.
- WorkspaceInitPathEscapeError + WorkspaceInitSymlinkError typed
  classes for the two boundary guards in initWorkspace, mapped to
  HTTP 400 by sendBridgeError. Previous generic `Error` fell
  through to the 500 handler, telling operators "daemon broken"
  when the actual fix was workspace-config correction. Addresses
  #3260501161.

Public surface symmetry:
- Re-export McpServerNotFoundError, McpServerRestartFailedError,
  WorkspaceInitPathEscapeError, WorkspaceInitSymlinkError from the
  serve barrel. External embeds matching these via `instanceof`
  no longer need deep imports. Addresses #3260501163.

Test coverage:
- restartMcpServer bridge tests (5): success + event broadcast,
  soft-skip + refused event, McpServerNotFoundError translation,
  McpServerRestartFailedError translation, originator clientId
  stamping. Addresses #3260501141.
- sendBridgeError mapping tests (4): McpServerNotFoundError → 404,
  McpServerRestartFailedError → 502, WorkspaceInitPathEscapeError
  → 400, WorkspaceInitSymlinkError → 400. Addresses #3260501148.
- initWorkspace boundary guard tests (2 added): symlink-at-target
  rejected, contextFilename '../outside.md' rejected. Addresses
  #3260501157.
- TrustGateError tests assert the typed class via `.toThrow(TrustGateError)`,
  not just message text. Addresses #3260501165.

Also updates the existing fold-in 4 S2 broadcast test to reflect
the new no-duplicate semantics on the requesting session.

Typecheck clean across cli / sdk-typescript / core.
1615/1615 unit tests pass.

* fix(serve): fold-in 2 — copilot + wenshao review on #4297

Round-2 reviewer adoption on the same PR:

Critical fixes:
- `restartMcpServer` JSDoc documents `timeoutMs: 0` as "disable the
  timeout entirely", but the `> 0` guard in `fetchWithTimeout`
  rejected `0` and silently fell back to the 30s client default.
  Loosened the guard to `>= 0` so `0` flows through to the
  no-timeout branch via the existing truthiness check; NaN /
  negative inputs still coerce to the client default. Addresses
  duplicate reports from copilot (#3260577538) and wenshao
  (#3260661833).
- TS2322 in the slow-fetch test stub: `resolveResponse` was typed
  against `import('undici-types').Response` but assigned a
  `(v: Response) => void`. Re-typed against the global `Response`
  throughout. Caught only by tsc runs that include the test
  files. Addresses #3260663072.

Test fidelity:
- Slow-fetch stub now observes `init.signal` and rejects on abort,
  so a regression that drops the per-call `timeoutMs` override
  will reliably fail the test instead of resolving after the
  timer fired (false-negative coverage). Addresses #3260577600.
- New test pinning the `timeoutMs: 0` semantics: 1ms client
  default + a stub that resolves after 50ms. Without the `>= 0`
  fix, the call would abort at 1ms; with it, the explicit
  `0` disables the timer and the call completes.

Bug fixes:
- `runQwenServe.contextFilenameForInit` previously called
  `String(arr[0])` on the array branch, producing a literal
  `"[object Object]"` filename for hand-edited bad data. Now
  validates each element with `typeof === 'string'` and falls
  back to `undefined` (so the bridge uses its
  `getCurrentGeminiMdFilename()` default) when no string is
  found. Addresses #3260577641.

Documentation drift:
- `Config.getDisabledTools()` JSDoc rewritten to describe the
  mutable-via-`setDisabledTools()` semantics introduced by P2-2,
  and the "registration-time only / no retroactive unregister"
  contract that pairs with it. Old comment claimed the set was
  frozen at construction. Addresses #3260577677.

Observability:
- `acpAgent` MCP-restart `loadSettings` failure now surfaces a
  stderr line naming the server + the underlying error, instead
  of silently swallowing it. The documented "toggle + restart"
  workflow used to break with zero diagnostic when settings.json
  was corrupted or unreadable. Addresses #3260663303.

Code organization:
- Moved `canonicalizeExistingAncestor` after `describeStatKind` so
  the latter's JSDoc is no longer orphaned (TypeScript only
  associates the last `/** ... */` block before a declaration).
  Addresses #3260668618.

Typecheck clean across cli / sdk-typescript / core.
1616/1616 unit tests pass.

* fix(serve): fold-in 3 — read merged scope on MCP restart refresh

Critical bug from wenshao review (#3260725526) on PR #4297:
the P2-2 acpAgent re-read narrowed `Config.disabledTools` to
`SettingScope.Workspace` alone, dropping User / System scope
entries. The bootstrap Config received `merged.tools?.disabled`
(union of all scopes), so user-level / system-level disables
worked at boot — but the first `mcp restart` would replace the
in-memory set with the workspace scope alone, silently re-enabling
any tool that was disabled at a higher scope but absent from the
workspace file.

The asymmetry vs. the persist-write path is deliberate and
documented:
- Reads (here): merged — match the bootstrap Config snapshot,
  preserve user/system policy.
- Writes (`runQwenServe.persistDisabledTools`): workspace scope —
  don't bake higher-scope entries into the workspace file
  (per-#4282 fold-in 1 H2 fix).

Two paths look alike but answer different questions.

Typecheck clean across cli / sdk-typescript / core.
1616/1616 unit tests pass.

* fix(test): fold-in 4 — wire timeoutMs:0 stub to init.signal

Critical follow-up from wenshao (#3260810242) on PR #4297:
the new `timeoutMs: 0` regression test (added in fold-in 2)
inherited the same flaw it was meant to prevent — the slow-fetch
stub didn't observe `init.signal`, so a regression that ignored
the `0` override would fire the AbortController at the 1ms client
default but the stub would keep the promise pending. The 50ms
`resolveResponse` would win, the test would still pass, and the
documented "0 disables timeout" contract would be unprotected.

Mirrored the listener pattern already used by the two sibling
tests in fold-in 2 — `init.signal.addEventListener('abort', () =>
reject(...))`. Now a regression that re-rejects `0` triggers the
abort, the stub rejects, the test fails.

8/8 restartMcpServer SDK tests pass; SDK typecheck clean.

* fix(serve): fold-in 5 — TOCTOU + setDisabledTools coverage

Two new critical reviews from wenshao on PR #4297:

C1 — TOCTOU between lstat and writeFile (#3260836305):
The `lstat(target)` symlink check and the subsequent `writeFile`
were two separate syscalls, leaving a race window where a local
attacker with workspace write access could substitute a symlink
between them. With `force: true`, `writeFile` would follow the
link and truncate an external target.

The `action === 'created'` path now uses `fs.open(target, 'wx')`
(O_WRONLY|O_CREAT|O_EXCL), which atomically refuses any
pre-existing inode (regular file, dir, OR symlink) at the target
path. EEXIST after the absence check most plausibly means a
race-created symlink, so we throw `WorkspaceInitSymlinkError(kind:
'target')` — same typed class the route maps to 400.

The `force: true` overwrite path retains the existing TOCTOU as a
documented limitation; closing it requires `O_NOFOLLOW`-aware open
which the post-PR18 `WorkspaceFileSystem` migration will provide.

C2 — P2-2 zero test coverage (#3260836302):
The `setDisabledTools` runtime sync was the only Wave-4 P2 fix
without a dedicated test. Added 5 Config-level tests:
- Initializes from `disabledTools` ConfigParameters
- Defaults to empty set when omitted
- `setDisabledTools` replaces the live snapshot
- Defensive copy: caller-set mutations don't leak into the live snapshot
- Accepts an empty set (clears live snapshot)

Plus a TOCTOU regression test in httpAcpBridge.test.ts that
spies fs.lstat / fs.readFile to simulate the race window:
pre-creates a symlink, makes lstat lie about it, asserts the
'wx' open catches the racing inode and throws the typed
`WorkspaceInitSymlinkError(kind: 'target')`.

1622/1622 unit tests pass; typecheck clean across cli /
sdk-typescript / core.

* fix(serve): fold-in 6 — count actual skips in broadcast alarm

DeepSeek review on #4297 (#3261079572):
`broadcastWorkspaceEvent` unconditionally subtracted 1 from the
`eligible` recipient count whenever `skipSessionId` was set, even
when the id matched zero live sessions (caller mistake, stale id,
or the matching session was just torn down between resolution and
broadcast). In a single-session workspace that's the difference
between `eligible = 0` (alarm suppressed) and `eligible = 1`
(alarm fires when the publish failed) — silently losing the
all-dropped breadcrumb the telemetry was meant to surface.

Today's call sites pass real session ids so the bug doesn't
manifest in practice, but the defensive shape is small: track
`skippedCount` inside the loop and subtract that, so the alarm
condition is self-consistent regardless of how the caller mis-uses
the param.

162/162 bridge tests pass; CLI typecheck clean.

* fix(serve): fold-in 7 — close overwrite TOCTOU, harden boot + diagnostics

Round-7 review on PR #4297. Three critical fixes + one suggestion
test, plus a regression test for the overwrite TOCTOU close.

C1 — force:true overwrite TOCTOU (#3262615446):
The fold-in 5 fix only closed the `'created'` action via 'wx';
the `'overwrote'` branch still used plain `fs.writeFile`, so a
local writer could swap the verified regular file to a symlink
between the lstat/readFile checks and the write and have the
forced overwrite truncate an external target. Switched to
`fs.open(target, O_WRONLY | O_TRUNC | O_NOFOLLOW)` — `O_NOFOLLOW`
makes open() fail with ELOOP on a symlink at the final component
even under race. ELOOP / ENOENT (race-deleted) translate to
`WorkspaceInitSymlinkError(kind: 'target')` so the route still
maps to a structured 400 instead of a generic 500.

C2 — settings.json corrupt blocks daemon boot (#3262625091):
`loadSettings(boundWorkspace)` at boot had no try/catch — a
corrupted, malformed, or temporarily unreadable settings file
threw synchronously and prevented daemon startup. Pre-PR this
never happened because settings were read lazily inside request
handlers. Wrapped in try/catch with stderr fallback so the daemon
keeps booting (with the bridge's default context filename) when
the file is broken.

C3 — malformed `tools.disabled` clears policy silently (#3262625101):
When `merged.tools?.disabled` is present but not an array
(boolean / string / object from a hand-edited settings.json), the
ternary `Array.isArray(...) ? ... : []` substituted an empty list
without firing the surrounding catch block. After an MCP restart
every disabled tool would silently re-register. Added an explicit
`!Array.isArray && !== undefined` check that stderr-logs the
malformed type before clearing — operators see the
misconfiguration instead of a stealth re-enable.

S1 — contextFilename extraction tested (#3262690842):
Lifted the inline `firstStringInArray` + branching into an
exported `extractContextFilename(value: unknown)` helper and
added `runQwenServe.test.ts` with 5 tests covering the four
branches the suggestion called out: non-empty string, array with
strings, array with no strings, non-string non-array.

Plus a TOCTOU regression test for the overwrite path that
verifies `O_NOFOLLOW` returns `WorkspaceInitSymlinkError(kind:
'target')` when the file is race-substituted with a symlink
behind the lstat/readFile mocks.

S2 (acpAgent restart-handler integration test #3262690845) is
deferred — Config-level coverage of `setDisabledTools` already
locks the load-bearing surface (5 tests in fold-in 5), and
adding a full acpAgent integration test requires heavy ext-method
plumbing. The new C3 stderr diagnostic plus existing tests give
us the regression signal we need without that scaffolding.

1627/1627 unit tests pass; typecheck clean across cli /
sdk-typescript / core / acp-bridge.

* fix(serve): fold-in 8 — split ELOOP / ENOENT diagnostic in overwrite path

qwen-latest review on PR #4297 (#3262861754):
The fold-in 7 ELOOP/ENOENT branch shared one error message that
said "swapped to a symlink." That's accurate for ELOOP (genuine
O_NOFOLLOW rejection — likely an attack race) but misleading for
ENOENT in the overwrite path: there `readFile` just succeeded
proving the file existed, so ENOENT means the file was DELETED
between the content check and the open — a benign race with a
concurrent writer (git checkout, editor save, lockfile rename),
NOT a symlink swap. An operator seeing the symlink language for
a benign delete would `ls -la`, see no symlink, and waste time
hunting an attack that didn't happen.

Split into two messages:
- ELOOP: "swapped to a symlink between the content check and the
  overwrite — refusing to follow it"
- ENOENT: "deleted between the content check and the overwrite
  (likely a concurrent writer) — refusing to recreate blindly"

Both still surface as `WorkspaceInitSymlinkError(kind: 'target')`
so the route maps to a structured 400; the class doubles as the
workspace-init race-condition bucket with kind='target' meaning
"target inode misbehaved at write time" generally.

Updated the existing fold-in 7 TOCTOU test to assert the ELOOP
message specifically, and added a new ENOENT race-delete test
that mocks lstat/readFile to land on the overwrote action against
a non-existent path — verifies the message says "deleted" and
NOT "swapped to a symlink."

170/170 bridge tests pass; CLI typecheck clean.

* fix(serve): fold-in 9 — route MCP restart through registry cleanup wrapper

gpt-5.5 critical review on PR #4297 (#3263088414):

The fold-in 5 P2-2 fix refreshed `Config.disabledTools` from merged
settings, but then called `manager.discoverMcpToolsForServer()`
directly — bypassing the `ToolRegistry.discoverToolsForServer`
wrapper that PURGES the server's existing `DiscoveredMCPTool`
entries (and `revealedDeferred` markers) plus its prompts before
rediscovery. Without the cleanup, `registerTool` only consulted
the refreshed `disabledTools` set for NEWLY-discovered tools —
entries already in the registry from the prior MCP boot kept
serving requests. Net effect: toggle-disable-then-restart
silently left the disabled tool live, breaking the documented
"toggle + restart" workflow that P2-2 was meant to fix.

Routed through `toolRegistry.discoverToolsForServer(serverName)`
which:
1. Removes existing `DiscoveredMCPTool` entries for this server
2. Drops their `revealedDeferred` reveal state
3. Removes the server's prompts via `removePromptsByServer`
4. THEN delegates to `manager.discoverMcpToolsForServer` for the
   actual reconnect + rediscover

The pre-discovery budget / in-flight checks still go through the
`manager` reference (which is the same object the registry
wrapper would forward to) — so soft-skip semantics for
`budget_would_exceed`, `in_flight`, `disabled` are preserved.

CLI typecheck clean; 403/403 server + bridge tests pass.

* fix(serve): fold-in 10 — qwen-latest 05:45-round review on #4297

5 review threads from qwen-latest's late round on PR #4297 (now closed
in favor of #4313 against `daemon_mode_b_main`). 1 critical + 4
suggestions, all adopted.

C1 — extractContextFilename / getCurrentGeminiMdFilename divergence
(#3263954685): with `context.fileName: ['  ', 'AGENTS.md']`, the
daemon parent's `extractContextFilename` (which skips empty entries)
wrote `AGENTS.md`, but the ACP child's `getCurrentGeminiMdFilename`
(which returned `arr[0]` unconditionally) read `''`. The init'd file
was orphaned. Aligned `getCurrentGeminiMdFilename` to skip empty
entries with the same semantics, falling back to
`DEFAULT_CONTEXT_FILENAME` when all entries are empty.

S2 — WorkspaceInitSymlinkError reused for non-symlink races
(#3263954690): the EEXIST race-create and ENOENT race-delete cases
were surfacing as `code: 'workspace_init_symlink'`, misleading
operators into hunting symlink attacks for benign concurrent-
modification windows. Split into a sibling `WorkspaceInitRaceError`
class (`kind: 'eexist' | 'enoent'`, HTTP code
`workspace_init_race`). The genuine symlink class stays for ELOOP,
lstat-detected target symlinks, and parent-realpath escapes.

S3 — fsConstants.O_NOFOLLOW defensive `?? 0` (#3263954697): matches
the existing codebase convention in
`core/src/utils/{sessionStorageUtils,gitDiff}.ts` and
`cli/src/ui/utils/customBanner.ts`. Functionally a no-op (JS
bitwise coerces undefined to 0) but consistent.

S5 — Parent-directory TOCTOU still open (#3263954707): O_NOFOLLOW
only protects the final path component; a local writer could swap
a real parent dir for a symlink between
`canonicalizeExistingAncestor` and `fs.open`. Added
`verifyParentWithinWorkspace` post-open helper that re-realpaths
`path.dirname(target)` and refuses with
`WorkspaceInitSymlinkError(kind: 'parent')` if the parent moved.
On the create path (where we just opened with `'wx'`), the failure
also unlinks the file we just made best-effort. Residual race
window narrowed from "between pre-check and open" to "between
post-open realpath and writeFile" — sub-millisecond, documented as
accepted Stage-1 trust posture.

S4 — broadcastWorkspaceEvent vs publishWorkspaceEvent stale comment
(#3263954688): the "now removed" comment was inaccurate (5 call
sites still use the closure). Replaced with an accurate
description of why both coexist (factory closure can't `this`-call
proxy member; closure also takes `skipSessionId` for persisted
approval-mode mirror) and a TODO marker for future helper extraction.

Two existing tests updated to assert the new `WorkspaceInitRaceError`
class for EEXIST / ENOENT scenarios (the symlink-class assertions
are preserved for ELOOP / lstat / parent cases).

1759/1759 unit tests pass; typecheck clean across all 4 packages.

* feat(acp-bridge): F1 — acp-bridge package self-sufficiency (#4175 mechanical lift + BridgeFileSystem seam) (#4319)

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

First mechanical lift of #4175 F1 (acp-bridge package self-sufficiency).
Moves the production spawn factory + its `killChild` helper +
`SCRUBBED_CHILD_ENV_KEYS` denylist + `KILL_HARD_DEADLINE_MS` constant
from `cli/src/serve/httpAcpBridge.ts` (~283 lines) to
`@qwen-code/acp-bridge/spawnChannel`. This unblocks
`channels/base/AcpBridge.ts` and `vscode-ide-companion`'s
acpConnection from each reimplementing the child lifecycle — they can
now consume the same primitive.

Backward compatible: `cli/src/serve/httpAcpBridge.ts` imports the
lifted factory and re-exports it, so existing references in
`cli/src/serve/index.ts:90` and the factory's own internal usage
(`opts.channelFactory ?? defaultSpawnChannelFactory`) keep resolving.
Bridge tests that mock `defaultSpawnChannelFactory` via
`BridgeOptions.channelFactory` are unaffected.

Side cleanups: drops `spawn` / `ChildProcess` / `Readable` / `Writable`
/ `ndJsonStream` / `MissingCliEntryError` imports from
httpAcpBridge.ts (all only used by the lifted spawn factory).

- 44/44 acp-bridge tests pass
- 174/174 cli httpAcpBridge tests pass
- typecheck clean across acp-bridge + cli

🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)

* refactor(acp-bridge): lift BridgeClient + permission types to acp-bridge/bridgeClient (#4175 F1 step 2)

Second mechanical lift of #4175 F1 (acp-bridge package self-sufficiency).
Moves `BridgeClient` class (~700 LOC) + `PendingPermission` interface +
`PermissionResolutionRecord` interface + `MAX_RESOLVED_PERMISSION_RECORDS`
constant + early-event capacity constants + `describeStatKind` and
`sliceLineRange` helpers from `cli/src/serve/httpAcpBridge.ts` to
`@qwen-code/acp-bridge/bridgeClient`.

Design choice for SessionEntry boundary: introduce a minimal
`BridgeClientSessionEntry` interface in bridgeClient.ts with only the
four fields BridgeClient actually reads from the factory's richer
`SessionEntry` (`sessionId`, `events`, `pendingPermissionIds`,
`activePromptOriginatorClientId`). The factory's `SessionEntry`
structurally satisfies it — TypeScript's structural typing enforces
the match at the `resolveEntry` callback signature, so no explicit
conversion is required and the bridge package stays free of daemon-host
session-bookkeeping types.

Cross-package writeStderrLine handling: inline the 3-line helper in
bridgeClient.ts (mirrors the spawnChannel.ts pattern from F1 step 1)
so acp-bridge has no reverse dependency on `cli/src/utils/stdioHelpers`.

httpAcpBridge.ts shrinks from 4406 LOC to 3647 LOC (-759 lines).
Removed ACP SDK imports that only BridgeClient consumed: `Client`,
`RequestPermissionRequest`, `WriteTextFileRequest`,
`WriteTextFileResponse`, `ReadTextFileRequest`, `ReadTextFileResponse`,
`SessionNotification`. Kept the ones the factory still uses
(`CancelNotification`, `PromptRequest`, `RequestPermissionResponse`,
`SetSessionModelRequest`, `SetSessionModelResponse`).

Backward compatible: httpAcpBridge.ts re-exports `BridgeClient`,
`BridgeClientSessionEntry`, `PendingPermission`,
`PermissionResolutionRecord`, and `MAX_RESOLVED_PERMISSION_RECORDS` so
the `ChannelInfo.client: BridgeClient` field declaration below + any
embedder reaching into these types keep resolving.

- 44/44 acp-bridge tests pass
- 174/174 cli httpAcpBridge tests pass
- 229/229 cli server tests pass
- typecheck clean across acp-bridge + cli

🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)

* refactor(acp-bridge): lift createHttpAcpBridge factory to acp-bridge/bridge (#4175 F1 step 3)

Third + final mechanical lift of #4175 F1 (acp-bridge package
self-sufficiency). Moves the `createHttpAcpBridge` factory closure
(~3000 LOC) + `ChannelInfo` + `SessionEntry` interfaces + factory-only
helpers (`canonicalizeExistingAncestor`, `verifyParentWithinWorkspace`,
`withTimeout`, `isServeDebugLoggingEnabled`, `writeServeDebugLine`,
`hasControlCharacter`) + factory constants (`DEFAULT_INIT_TIMEOUT_MS`,
`MCP_RESTART_TIMEOUT_MS`, `DEFAULT_MAX_SESSIONS`, `MAX_EVENT_RING_SIZE`,
`DEFAULT_PERMISSION_TIMEOUT_MS`, `DEFAULT_MAX_PENDING_PER_SESSION`,
`MAX_DISPLAY_NAME_LENGTH`) from `cli/src/serve/httpAcpBridge.ts` to
`@qwen-code/acp-bridge/bridge`.

`cli/src/serve/httpAcpBridge.ts` shrinks from 3647 LOC to 97 LOC — a
pure re-export shim that preserves every existing relative import
path (`./httpAcpBridge.js`) so `server.ts`, `runQwenServe.ts`,
`workspaceAgents.ts`, `workspaceMemory.ts`, `index.ts`, plus the bridge
test suite, keep resolving without any call-site changes.

The new `bridge.ts` reuses what was already in acp-bridge (errors,
types, options, status helpers, channel types, event bus, workspace
paths) via local relative imports — no reverse dependency on `cli`.
`writeStderrLine` is inlined at the top of `bridge.ts` (same pattern as
`spawnChannel.ts` + `bridgeClient.ts` from F1 steps 1-2) so the
package self-contained promise holds.

Cumulative F1 impact across the 3 mechanical lift steps:
- httpAcpBridge.ts: 4682 LOC → 97 LOC (-4585 lines; the original file
  was 98% bridge core, 2% backward-compat re-exports)
- 3 new files in acp-bridge: spawnChannel.ts (~270 LOC), bridgeClient.ts
  (~745 LOC), bridge.ts (~3515 LOC)
- All daemon-host concerns (env snapshot, daemon preflight cells)
  remain in `cli/src/serve/daemonStatusProvider.ts` and reach the
  bridge through the `BridgeOptions.statusProvider` seam frozen by
  PR 22b/2.

- 735/735 cli serve tests pass across 17 files
- 174/174 cli httpAcpBridge tests pass
- 44/44 acp-bridge tests pass
- typecheck clean across acp-bridge + cli

`packages/cli/src/serve/httpAcpBridge.test.ts` (~6600 LOC) is
intentionally NOT moved in this commit — it currently imports
`createHttpAcpBridge` / `defaultSpawnChannelFactory` / `BridgeClient`
via the cli shim and keeps passing without changes. Moving it to
`acp-bridge/src/bridge.test.ts` is a follow-up worth tracking
separately so the production-code lift can land + be reviewed cleanly.

The `BridgeFileSystem` injection seam (originally bundled into F1 as
the 22b' scope) is also deferred to a follow-up so the mechanical lift
stays mechanical — design + implementation of the fs injection is its
own discussion.

🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)

* feat(acp-bridge): add BridgeFileSystem injection seam (#4175 F1 step 5, 22b' scope)

Adds the `BridgeFileSystem` injection seam originally scoped as #4175
22b'. When a `BridgeFileSystem` is wired through
`BridgeOptions.fileSystem`, `BridgeClient.readTextFile` and
`BridgeClient.writeTextFile` delegate to it instead of running their
inline `fs.realpath` / `fs.writeFile` / `fs.readFile` proxy.

This unblocks production `qwen serve` plumbing PR 18's
`WorkspaceFileSystem` (TOCTOU guards, symlink-substitution checks,
trust gate, `.gitignore`, audit hooks) into the ACP fs methods —
closing the `ws.ts:613` follow-up thread that has been tracked since
PR 18 landed. The serve-side adapter that wraps `WorkspaceFileSystem`
+ the `runQwenServe` wiring are intentionally split into the
immediate-follow-up so this PR stays focused on the seam design.

Backward compatible: `fileSystem` is optional on `BridgeOptions`.
Tests, Mode A in-process consumers, channels (`packages/channels/base/
AcpBridge.ts`), and the VSCode IDE companion all keep working
unchanged — they omit the field and `BridgeClient` falls through to
the inline proxy that has been the Stage 1 default since #3889.

API:
- `BridgeFileSystem.readText(params: ReadTextFileRequest):
  Promise<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 (origina…

… approval-mode serialization, catch-up indicator) (#4510)

* fix(serve): post-merge fixes for #4291 review (7 threads) (#4305)

* fix(serve): address qwen-latest review on merged #4291 (7 threads)

Seven post-merge findings from the qwen-latest review on #4291,
all real. Most are tightening fixes for issues introduced by the
earlier rounds of #4291 — the same security / DRY / observability
classes the original review surfaced, applied to surfaces that
weren't covered initially.

#1 (deviceFlow.ts:1179) — late-poll observer closure retained the
entire entry by reference (deviceCode/pkceVerifier BrandedSecrets +
cancelController) for the lifetime of the daemon if `provider.poll()`
never settled. Memory leak + indefinite secret retention. Destructure
the four fields the closure actually needs (deviceFlowId, providerId,
initiatorClientId, audit sink) so the entry is GC-eligible the
moment runPollTick returns.

#2 (server.ts) — `callerIsInitiator` was duplicated verbatim across
three locations: GET handler, toDeviceFlowStartResponseBody,
toDeviceFlowStateBody. The exact bug class #4291 was fixing was
"POST and GET diverged on the same redaction policy" — duplicating
the gate recreated the preconditions for divergence. Extracted to
shared `callerIsDeviceFlowInitiator(view, callerClientId)` helper
with the consolidated threat-model JSDoc. All three sites now call
the helper.

#3 (deviceFlow.ts:1110) — timeout callback constructed two separate
`DeviceFlowPollTimeoutError` instances (one for `signal.reason`, one
for the wrapper rejection). Each capture its own V8 stack trace,
and `signal.reason.stack` would diverge from the caught rejection's
stack — confusing for operators inspecting both. Build the sentinel
ONCE per timer fire and pass the same instance to both sites.

#4 (qwenDeviceFlowProvider.ts:273) — `Error.name` is a freely
assignable string property; a hostile fetch wrapper could set
`e.name = 'X\n[serve] FAKE LINE\x1b[31m'` to inject log lines or
ANSI sequences via the same vector we already closed for `oauthError`.
The non-OAuth catch path interpolated `${err.name}` raw. Apply the
same `sanitizeForStderr()` helper.

#5 (deviceFlow.ts:1551) — on the timeout path, `rawProviderError`
is undefined (deliberately, to skip the misleading
`provider.poll() threw (raw): ...` audit template), but that left
the audit hint field omitted entirely. Operators reading the
durable audit trail saw `errorKind: 'upstream_error'` with no signal
whether it was a hung IdP or a generic provider failure. Use
`result.hint` (which already carries the timeout-specific
`provider.poll() timed out after Nms; check IdP connectivity` text
built in the catch) so the audit matches the SSE event.

#6 (server.ts) — the `QWEN_SERVE_DEBUG` env-var check was inlined
in the GET route handler, duplicating the `isServeDebugMode()`
helper from `./debugMode.js` that workspaceAgents and
workspaceMemory already use. The inline copy also had a dead `?? ''`
fallback (the value is guaranteed truthy at that point per the
preceding check). Use the canonical helper.

#7 (deviceFlow.ts:1217) — late-rejection observer interpolated the
raw `lateErr.message` into the audit hint (truncated to 256 bytes,
but RFC 8628 `device_code` values fit comfortably in 256 bytes).
The provider's catch already uses the `name + length` redaction
pattern to prevent WAF-echoed `device_code`/PKCE leaks; the
registry layer was undoing that hardening because the same failure
settled late. Apply the same `name + length` pattern at the late-
rejection site.

Tests:
- Existing late-rejection test reseeded with a `device-code-secret-*`
  substring inside the long detail; hard-negative-asserts the seeded
  secret is absent from the audit + asserts the new
  `Error (message N bytes; raw suppressed)` shape.
- Existing poll-timeout test now also asserts: hint IS defined on
  the audit (not omitted), hint contains `'timed out after'` /
  `'check IdP connectivity'`, and `signal.reason instanceof
  DeviceFlowPollTimeoutError` (proves the single sentinel is
  shared between abort and reject).
- New `sanitizes control characters in attacker-controlled
  err.name` test in qwenDeviceFlowProvider.test.ts pins the round-4
  #4 fix with a hostile `e.name` containing `\n` + `\x1b[31m...`.

cli serve 702/702 (was 686, +16 — additional tests imported via
the acp-bridge package lift on main); sdk 421/421; typecheck clean
across all 4 workspaces; eslint --max-warnings 0 clean on touched
files.

Refs: #4175, #4255, #4291

🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)

* fix(serve): address deepseek-v4-pro review on #4305 (4 threads)

Round-5 fold-in. Four findings from the deepseek-v4-pro review on
PR #4305 — all real, three are sister fixes for the same security
classes that #4305 already closed at adjacent surfaces.

#1 (deviceFlow.ts) — `pollTimedOut` race correctness. The flag was
set unconditionally inside the timer callback. If the provider
settled the wrapper at 29.9s, `finally` would call
`clearScheduled(pollTimer)` — but if the timer callback was already
queued for execution before the clear landed (a real possibility
in Node's event-loop ordering, even if not always observed in
practice), this branch could still run and incorrectly mark
`pollTimedOut`. Move the flag assignment to the catch block where
the settled cause is unambiguous via `instanceof
DeviceFlowPollTimeoutError`. New test pins the negative: provider
beats the timeout → no spurious `lost_late_poll_after_timeout`
audit even after ticking 2× the ceiling.

#2 (deviceFlow.ts) — late-rejection observer interpolated raw
`lateErr.name` into the audit hint without sanitization. Same
attacker-controlled vector closed at the provider layer for
`err.name` in round-4. Route through `sanitizeForStderr`.

#3 (deviceFlow.ts) — late-success observer interpolated
`latePollResult.kind` directly into the audit template. While the
typed shape is `'pending' | 'slow_down' | 'success' | 'error'`, a
non-conforming provider could return an arbitrary string. Same
log-injection vector. Route through `sanitizeForStderr`.

#4 (qwenDeviceFlowProvider.ts → deviceFlow.ts) —
`sanitizeForStderr` only stripped ASCII C0/C1 + DEL; bypass via
Unicode lookalikes:
  - U+2028/U+2029: LINE/PARAGRAPH SEPARATOR (newline-equivalent in
    most Unicode-aware terminals — most direct log-forging vector)
  - U+200B–U+200F: zero-width chars + LRM/RLM
  - U+202A–U+202E: bidirectional override controls
  - U+FEFF: BOM / ZWNBSP

A malicious IdP returning `slow_down
[serve] FAKE` in
`oauthError` would otherwise still forge log lines.

Architectural change: `sanitizeForStderr` was previously private to
`qwenDeviceFlowProvider.ts`. To address #2/#3, the registry layer
needs to call it too. Lifted into `deviceFlow.ts` (the foundation
module) and re-imported from the provider. Single source of truth;
the regex is now a module-level constant compiled once with explicit
`\uXXXX` escapes (via `String.raw` so the source is greppable, not
literal-Unicode-laden).

Tests:
- `does NOT attach late-poll observer when the provider beats the
  timeout` — N1 race regression
- `sanitizes hostile latePollResult.kind in late-observer audit` — N3
- `sanitizes hostile lateErr.name in late-rejection observer audit` — N2
- `sanitizes Unicode lookalike controls (U+2028 LINE SEPARATOR,
  bidi, ZWNBSP) in oauthError` — N4

cli serve 706/706 (was 702, +4 — all new round-5 tests); sdk
421/421; typecheck clean; eslint --max-warnings 0 clean on touched
files.

Refs: #4175, #4255, #4291, #4305

🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)

* fix(serve): address gpt-5.5 + qwen-latest review on #4305 round-5 (5 threads)

Round-6 fold-in. Five findings split between maintainability,
security hardening, and a real defensive bug.

#1 (qwenDeviceFlowProvider.test.ts) — gpt-5.5: round-5 #4 test
embedded U+2028 / U+200E / U+FEFF as literal characters in source.
Invisible in GitHub diffs / most editors; the negative
`not.toContain('')` looked like an empty-string check. Rewrote
the payload + assertions to use named `\uXXXX`-bound constants.
Also added a companion test exercising U+2066–U+2069 (round-6 #5
below).

#2 (deviceFlow.ts) — qwen-latest: the late-poll observer's
`void tracked.then(...)` was missing a terminal `.catch(() => {})`.
A synchronous throw inside either handler (e.g., a misbehaving
`audit.record`: backpressure, malformed payload, sink out-of-disk)
would reject the derived promise unhandled. On Node 22's default
`--unhandled-rejections=throw`, that crashes the daemon. Added the
terminal `.catch(() => {})` matching the persist-tracker pattern.
New test injects a poison audit sink that throws specifically on
the `lost_late_poll_after_timeout` call; asserts `flushAsync()`
resolves cleanly.

#3 (deviceFlow.ts) — qwen-latest: the `case 'error'` audit-record
hint interpolated `rawProviderError` (raw `err.message`) without
`sanitizeForStderr`. Per ES2019+ `JSON.stringify` no longer escapes
U+2028/U+2029 — those would still forge log lines downstream
through file/stdout audit sinks. Apply the same sanitizer used on
every other provider-controlled audit path. New test pins a hostile
provider message containing U+2028 + ANSI escape and asserts
neither survives.

#4 (deviceFlow.ts) — qwen-latest: the round-5 #1 comment claimed
"`DeviceFlowPollTimeoutError` isn't exported as a public DeviceFlow
contract", but it IS `export class` (the test file constructs it
directly for fixtures). With `pollTimedOut = true` keyed solely on
`instanceof`, a future provider that imports + throws the class
would spoof the registry's "I caused the timeout" signal —
attaching a phantom late-poll observer.

Fix: introduce a runtime brand `_isRegistryTimeout: boolean` on the
class (default `false`) plus an internal-only
`makeRegistryPollTimeoutError(ms)` helper that sets the brand to
`true`. The brand is set ONLY at the registry's race-timer
construction site. Both gates updated:
  - `if (err instanceof X && err._isRegistryTimeout === true)` in
    the catch (for `pollTimedOut`)
  - `if (lateErr instanceof X && lateErr._isRegistryTimeout === true)`
    in the late-rejection self-filter

A provider-thrown brand-false instance now flows through the
generic provider-throw audit path — correctly auditing the misuse
rather than silently swallowing it. Repurposed the original "no
double-audit when registry's own DeviceFlowPollTimeoutError is
late-rejected" test (which was actually exercising the brand-false
path) into the inverted assertion: brand-false provider throw IS
audited as a real failure. Removed the orphaned old assertion; the
brand-true happy path is implicitly covered by the hanging-provider
test (which exercises the registry-built timeout end-to-end).

#5 (deviceFlow.ts) — qwen-latest: `sanitizeForStderr` regex covered
U+202A–U+202E (bidi embedding/override) but missed U+2066–U+2069
(LRI/RLI/FSI/PDI). These are the primary CVE-2021-42574
("Trojan Source") attack vectors — a hostile IdP swapping U+2066
for U+202D achieves the same visual reordering and would have
bypassed the round-5 filter entirely. Extended the regex range and
JSDoc; new test exercises U+2066/U+2068/U+2069 in `oauthError` and
asserts none survive while substantive ASCII parts remain.

cli serve 713/713 (was 710, +3 round-6 tests + the round-5 #4
rewrite + the round-6 #5 companion); typecheck clean across all 4
workspaces; eslint --max-warnings 0 clean on touched files.

Refs: #4175, #4255, #4291, #4305

🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)

* fix(serve): replace literal U+2028 with explicit 
 escape in round-6 #3 test

PR #4312 review (Copilot): the round-6 #3 test (sanitizes
rawProviderError) regressed back to embedding a literal U+2028
character in source via `const U_2028 = ' '`. That's the same
maintainability anti-pattern round-6 #1 was fixing in the sister
test. Internal-consistency fix: switch to the explicit `
`
escape so the constant is greppable and reviewable in GitHub diffs.

Refs: #4291, #4305, #4312

🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)

* fix(serve): post-merge P2 corrections from Codex review on #4282 (#4297)

* fix(serve): post-merge P2 corrections from Codex review on #4282

Follow-up to PR #4282 (Wave 4 PR 17) addressing four P2 issues
flagged by Codex's `/review` after the squash-merge to main:

P2-1 — Read the workspace context filename for init
  `qwen serve` parent never goes through `loadCliConfig`, so the
  process-global `getCurrentGeminiMdFilename()` stays on the default
  `QWEN.md` even when the workspace configures
  `context.fileName: 'AGENTS.md'`. `runQwenServe` now snapshots the
  workspace's merged setting at boot and forwards via
  `BridgeOptions.contextFilename`, so init writes the same file the
  ACP child reads.

P2-2 — Restart MCP servers with a fresh disabledTools snapshot
  `Config.disabledTools` was frozen at construction time;
  `setWorkspaceToolEnabled` only updated settings.json. The
  documented "toggle + restart" workflow re-registered just-disabled
  tools because rediscovery still saw the bootstrap snapshot. Added
  `Config.setDisabledTools()` plus a re-read at the ACP restart
  handler so `discoverMcpToolsForServer` honors the latest set.

P2-3 — Match the SDK timeout to the daemon's restart budget
  Bridge waits up to 300s for stdio MCP discovery; SDK helper used
  the client-wide 30s default and aborted valid slow restarts.
  Added a per-call `timeoutMs` plumbed through `fetchWithTimeout`,
  defaulting `restartMcpServer` to 5 minutes.

P2-4 — Reject symlinked parent directories before init writes
  `lstat(target)` only checked the final component; a symlinked
  parent (e.g. `docs -> /tmp` with `context.fileName:
  'docs/QWEN.md'`) would let `writeFile` follow the link and create
  / truncate outside `boundWorkspace`. Added
  `canonicalizeExistingAncestor` (walks up through ENOENT to the
  deepest extant ancestor, then `realpath`s) and verifies the
  canonical parent stays within the canonical workspace.

5 new tests (4 bridge / 2 SDK):
- contextFilename snapshot honored
- parent-symlink escape rejected
- nested real subdir accepted
- restartMcpServer survives 1.2s response with 1s default timeout
- restartMcpServer honors a 50ms caller override

Typecheck clean across cli / sdk-typescript / core.
1604/1604 unit tests pass.

* fix(serve): fold-in 1 — address 16:32:44-round review on #4282

Follow-up addressing the 8 unresolved review threads opened on PR
shipping in this same #4297; addresses correctness gaps + missing
test coverage that would otherwise let regressions ride into main.

Behavior fix:
- broadcastWorkspaceEvent gains a `skipSessionId` parameter; when
  `setSessionApprovalMode` runs with `persist:true`, the broadcast
  skips the requesting session so it doesn't receive the same
  `approval_mode_changed` event twice (once via session-scoped
  publish + once via broadcast). The SDK reducer's
  `approvalModeChangedCount` now increments by 1, not 2, on the
  requesting client (peers still see 1 via the broadcast).
  Addresses #3260501134.

Observability + posture:
- broadcastWorkspaceEvent now mirrors PR 16's publishWorkspaceEvent
  member: per-entry success/failure accounting + an "ALL buses
  dropped" stderr elevation. The previous local helper silently
  swallowed every publish failure. Addresses #3260501126.
- WorkspaceInitPathEscapeError + WorkspaceInitSymlinkError typed
  classes for the two boundary guards in initWorkspace, mapped to
  HTTP 400 by sendBridgeError. Previous generic `Error` fell
  through to the 500 handler, telling operators "daemon broken"
  when the actual fix was workspace-config correction. Addresses
  #3260501161.

Public surface symmetry:
- Re-export McpServerNotFoundError, McpServerRestartFailedError,
  WorkspaceInitPathEscapeError, WorkspaceInitSymlinkError from the
  serve barrel. External embeds matching these via `instanceof`
  no longer need deep imports. Addresses #3260501163.

Test coverage:
- restartMcpServer bridge tests (5): success + event broadcast,
  soft-skip + refused event, McpServerNotFoundError translation,
  McpServerRestartFailedError translation, originator clientId
  stamping. Addresses #3260501141.
- sendBridgeError mapping tests (4): McpServerNotFoundError → 404,
  McpServerRestartFailedError → 502, WorkspaceInitPathEscapeError
  → 400, WorkspaceInitSymlinkError → 400. Addresses #3260501148.
- initWorkspace boundary guard tests (2 added): symlink-at-target
  rejected, contextFilename '../outside.md' rejected. Addresses
  #3260501157.
- TrustGateError tests assert the typed class via `.toThrow(TrustGateError)`,
  not just message text. Addresses #3260501165.

Also updates the existing fold-in 4 S2 broadcast test to reflect
the new no-duplicate semantics on the requesting session.

Typecheck clean across cli / sdk-typescript / core.
1615/1615 unit tests pass.

* fix(serve): fold-in 2 — copilot + wenshao review on #4297

Round-2 reviewer adoption on the same PR:

Critical fixes:
- `restartMcpServer` JSDoc documents `timeoutMs: 0` as "disable the
  timeout entirely", but the `> 0` guard in `fetchWithTimeout`
  rejected `0` and silently fell back to the 30s client default.
  Loosened the guard to `>= 0` so `0` flows through to the
  no-timeout branch via the existing truthiness check; NaN /
  negative inputs still coerce to the client default. Addresses
  duplicate reports from copilot (#3260577538) and wenshao
  (#3260661833).
- TS2322 in the slow-fetch test stub: `resolveResponse` was typed
  against `import('undici-types').Response` but assigned a
  `(v: Response) => void`. Re-typed against the global `Response`
  throughout. Caught only by tsc runs that include the test
  files. Addresses #3260663072.

Test fidelity:
- Slow-fetch stub now observes `init.signal` and rejects on abort,
  so a regression that drops the per-call `timeoutMs` override
  will reliably fail the test instead of resolving after the
  timer fired (false-negative coverage). Addresses #3260577600.
- New test pinning the `timeoutMs: 0` semantics: 1ms client
  default + a stub that resolves after 50ms. Without the `>= 0`
  fix, the call would abort at 1ms; with it, the explicit
  `0` disables the timer and the call completes.

Bug fixes:
- `runQwenServe.contextFilenameForInit` previously called
  `String(arr[0])` on the array branch, producing a literal
  `"[object Object]"` filename for hand-edited bad data. Now
  validates each element with `typeof === 'string'` and falls
  back to `undefined` (so the bridge uses its
  `getCurrentGeminiMdFilename()` default) when no string is
  found. Addresses #3260577641.

Documentation drift:
- `Config.getDisabledTools()` JSDoc rewritten to describe the
  mutable-via-`setDisabledTools()` semantics introduced by P2-2,
  and the "registration-time only / no retroactive unregister"
  contract that pairs with it. Old comment claimed the set was
  frozen at construction. Addresses #3260577677.

Observability:
- `acpAgent` MCP-restart `loadSettings` failure now surfaces a
  stderr line naming the server + the underlying error, instead
  of silently swallowing it. The documented "toggle + restart"
  workflow used to break with zero diagnostic when settings.json
  was corrupted or unreadable. Addresses #3260663303.

Code organization:
- Moved `canonicalizeExistingAncestor` after `describeStatKind` so
  the latter's JSDoc is no longer orphaned (TypeScript only
  associates the last `/** ... */` block before a declaration).
  Addresses #3260668618.

Typecheck clean across cli / sdk-typescript / core.
1616/1616 unit tests pass.

* fix(serve): fold-in 3 — read merged scope on MCP restart refresh

Critical bug from wenshao review (#3260725526) on PR #4297:
the P2-2 acpAgent re-read narrowed `Config.disabledTools` to
`SettingScope.Workspace` alone, dropping User / System scope
entries. The bootstrap Config received `merged.tools?.disabled`
(union of all scopes), so user-level / system-level disables
worked at boot — but the first `mcp restart` would replace the
in-memory set with the workspace scope alone, silently re-enabling
any tool that was disabled at a higher scope but absent from the
workspace file.

The asymmetry vs. the persist-write path is deliberate and
documented:
- Reads (here): merged — match the bootstrap Config snapshot,
  preserve user/system policy.
- Writes (`runQwenServe.persistDisabledTools`): workspace scope —
  don't bake higher-scope entries into the workspace file
  (per-#4282 fold-in 1 H2 fix).

Two paths look alike but answer different questions.

Typecheck clean across cli / sdk-typescript / core.
1616/1616 unit tests pass.

* fix(test): fold-in 4 — wire timeoutMs:0 stub to init.signal

Critical follow-up from wenshao (#3260810242) on PR #4297:
the new `timeoutMs: 0` regression test (added in fold-in 2)
inherited the same flaw it was meant to prevent — the slow-fetch
stub didn't observe `init.signal`, so a regression that ignored
the `0` override would fire the AbortController at the 1ms client
default but the stub would keep the promise pending. The 50ms
`resolveResponse` would win, the test would still pass, and the
documented "0 disables timeout" contract would be unprotected.

Mirrored the listener pattern already used by the two sibling
tests in fold-in 2 — `init.signal.addEventListener('abort', () =>
reject(...))`. Now a regression that re-rejects `0` triggers the
abort, the stub rejects, the test fails.

8/8 restartMcpServer SDK tests pass; SDK typecheck clean.

* fix(serve): fold-in 5 — TOCTOU + setDisabledTools coverage

Two new critical reviews from wenshao on PR #4297:

C1 — TOCTOU between lstat and writeFile (#3260836305):
The `lstat(target)` symlink check and the subsequent `writeFile`
were two separate syscalls, leaving a race window where a local
attacker with workspace write access could substitute a symlink
between them. With `force: true`, `writeFile` would follow the
link and truncate an external target.

The `action === 'created'` path now uses `fs.open(target, 'wx')`
(O_WRONLY|O_CREAT|O_EXCL), which atomically refuses any
pre-existing inode (regular file, dir, OR symlink) at the target
path. EEXIST after the absence check most plausibly means a
race-created symlink, so we throw `WorkspaceInitSymlinkError(kind:
'target')` — same typed class the route maps to 400.

The `force: true` overwrite path retains the existing TOCTOU as a
documented limitation; closing it requires `O_NOFOLLOW`-aware open
which the post-PR18 `WorkspaceFileSystem` migration will provide.

C2 — P2-2 zero test coverage (#3260836302):
The `setDisabledTools` runtime sync was the only Wave-4 P2 fix
without a dedicated test. Added 5 Config-level tests:
- Initializes from `disabledTools` ConfigParameters
- Defaults to empty set when omitted
- `setDisabledTools` replaces the live snapshot
- Defensive copy: caller-set mutations don't leak into the live snapshot
- Accepts an empty set (clears live snapshot)

Plus a TOCTOU regression test in httpAcpBridge.test.ts that
spies fs.lstat / fs.readFile to simulate the race window:
pre-creates a symlink, makes lstat lie about it, asserts the
'wx' open catches the racing inode and throws the typed
`WorkspaceInitSymlinkError(kind: 'target')`.

1622/1622 unit tests pass; typecheck clean across cli /
sdk-typescript / core.

* fix(serve): fold-in 6 — count actual skips in broadcast alarm

DeepSeek review on #4297 (#3261079572):
`broadcastWorkspaceEvent` unconditionally subtracted 1 from the
`eligible` recipient count whenever `skipSessionId` was set, even
when the id matched zero live sessions (caller mistake, stale id,
or the matching session was just torn down between resolution and
broadcast). In a single-session workspace that's the difference
between `eligible = 0` (alarm suppressed) and `eligible = 1`
(alarm fires when the publish failed) — silently losing the
all-dropped breadcrumb the telemetry was meant to surface.

Today's call sites pass real session ids so the bug doesn't
manifest in practice, but the defensive shape is small: track
`skippedCount` inside the loop and subtract that, so the alarm
condition is self-consistent regardless of how the caller mis-uses
the param.

162/162 bridge tests pass; CLI typecheck clean.

* fix(serve): fold-in 7 — close overwrite TOCTOU, harden boot + diagnostics

Round-7 review on PR #4297. Three critical fixes + one suggestion
test, plus a regression test for the overwrite TOCTOU close.

C1 — force:true overwrite TOCTOU (#3262615446):
The fold-in 5 fix only closed the `'created'` action via 'wx';
the `'overwrote'` branch still used plain `fs.writeFile`, so a
local writer could swap the verified regular file to a symlink
between the lstat/readFile checks and the write and have the
forced overwrite truncate an external target. Switched to
`fs.open(target, O_WRONLY | O_TRUNC | O_NOFOLLOW)` — `O_NOFOLLOW`
makes open() fail with ELOOP on a symlink at the final component
even under race. ELOOP / ENOENT (race-deleted) translate to
`WorkspaceInitSymlinkError(kind: 'target')` so the route still
maps to a structured 400 instead of a generic 500.

C2 — settings.json corrupt blocks daemon boot (#3262625091):
`loadSettings(boundWorkspace)` at boot had no try/catch — a
corrupted, malformed, or temporarily unreadable settings file
threw synchronously and prevented daemon startup. Pre-PR this
never happened because settings were read lazily inside request
handlers. Wrapped in try/catch with stderr fallback so the daemon
keeps booting (with the bridge's default context filename) when
the file is broken.

C3 — malformed `tools.disabled` clears policy silently (#3262625101):
When `merged.tools?.disabled` is present but not an array
(boolean / string / object from a hand-edited settings.json), the
ternary `Array.isArray(...) ? ... : []` substituted an empty list
without firing the surrounding catch block. After an MCP restart
every disabled tool would silently re-register. Added an explicit
`!Array.isArray && !== undefined` check that stderr-logs the
malformed type before clearing — operators see the
misconfiguration instead of a stealth re-enable.

S1 — contextFilename extraction tested (#3262690842):
Lifted the inline `firstStringInArray` + branching into an
exported `extractContextFilename(value: unknown)` helper and
added `runQwenServe.test.ts` with 5 tests covering the four
branches the suggestion called out: non-empty string, array with
strings, array with no strings, non-string non-array.

Plus a TOCTOU regression test for the overwrite path that
verifies `O_NOFOLLOW` returns `WorkspaceInitSymlinkError(kind:
'target')` when the file is race-substituted with a symlink
behind the lstat/readFile mocks.

S2 (acpAgent restart-handler integration test #3262690845) is
deferred — Config-level coverage of `setDisabledTools` already
locks the load-bearing surface (5 tests in fold-in 5), and
adding a full acpAgent integration test requires heavy ext-method
plumbing. The new C3 stderr diagnostic plus existing tests give
us the regression signal we need without that scaffolding.

1627/1627 unit tests pass; typecheck clean across cli /
sdk-typescript / core / acp-bridge.

* fix(serve): fold-in 8 — split ELOOP / ENOENT diagnostic in overwrite path

qwen-latest review on PR #4297 (#3262861754):
The fold-in 7 ELOOP/ENOENT branch shared one error message that
said "swapped to a symlink." That's accurate for ELOOP (genuine
O_NOFOLLOW rejection — likely an attack race) but misleading for
ENOENT in the overwrite path: there `readFile` just succeeded
proving the file existed, so ENOENT means the file was DELETED
between the content check and the open — a benign race with a
concurrent writer (git checkout, editor save, lockfile rename),
NOT a symlink swap. An operator seeing the symlink language for
a benign delete would `ls -la`, see no symlink, and waste time
hunting an attack that didn't happen.

Split into two messages:
- ELOOP: "swapped to a symlink between the content check and the
  overwrite — refusing to follow it"
- ENOENT: "deleted between the content check and the overwrite
  (likely a concurrent writer) — refusing to recreate blindly"

Both still surface as `WorkspaceInitSymlinkError(kind: 'target')`
so the route maps to a structured 400; the class doubles as the
workspace-init race-condition bucket with kind='target' meaning
"target inode misbehaved at write time" generally.

Updated the existing fold-in 7 TOCTOU test to assert the ELOOP
message specifically, and added a new ENOENT race-delete test
that mocks lstat/readFile to land on the overwrote action against
a non-existent path — verifies the message says "deleted" and
NOT "swapped to a symlink."

170/170 bridge tests pass; CLI typecheck clean.

* fix(serve): fold-in 9 — route MCP restart through registry cleanup wrapper

gpt-5.5 critical review on PR #4297 (#3263088414):

The fold-in 5 P2-2 fix refreshed `Config.disabledTools` from merged
settings, but then called `manager.discoverMcpToolsForServer()`
directly — bypassing the `ToolRegistry.discoverToolsForServer`
wrapper that PURGES the server's existing `DiscoveredMCPTool`
entries (and `revealedDeferred` markers) plus its prompts before
rediscovery. Without the cleanup, `registerTool` only consulted
the refreshed `disabledTools` set for NEWLY-discovered tools —
entries already in the registry from the prior MCP boot kept
serving requests. Net effect: toggle-disable-then-restart
silently left the disabled tool live, breaking the documented
"toggle + restart" workflow that P2-2 was meant to fix.

Routed through `toolRegistry.discoverToolsForServer(serverName)`
which:
1. Removes existing `DiscoveredMCPTool` entries for this server
2. Drops their `revealedDeferred` reveal state
3. Removes the server's prompts via `removePromptsByServer`
4. THEN delegates to `manager.discoverMcpToolsForServer` for the
   actual reconnect + rediscover

The pre-discovery budget / in-flight checks still go through the
`manager` reference (which is the same object the registry
wrapper would forward to) — so soft-skip semantics for
`budget_would_exceed`, `in_flight`, `disabled` are preserved.

CLI typecheck clean; 403/403 server + bridge tests pass.

* fix(serve): fold-in 10 — qwen-latest 05:45-round review on #4297

5 review threads from qwen-latest's late round on PR #4297 (now closed
in favor of #4313 against `daemon_mode_b_main`). 1 critical + 4
suggestions, all adopted.

C1 — extractContextFilename / getCurrentGeminiMdFilename divergence
(#3263954685): with `context.fileName: ['  ', 'AGENTS.md']`, the
daemon parent's `extractContextFilename` (which skips empty entries)
wrote `AGENTS.md`, but the ACP child's `getCurrentGeminiMdFilename`
(which returned `arr[0]` unconditionally) read `''`. The init'd file
was orphaned. Aligned `getCurrentGeminiMdFilename` to skip empty
entries with the same semantics, falling back to
`DEFAULT_CONTEXT_FILENAME` when all entries are empty.

S2 — WorkspaceInitSymlinkError reused for non-symlink races
(#3263954690): the EEXIST race-create and ENOENT race-delete cases
were surfacing as `code: 'workspace_init_symlink'`, misleading
operators into hunting symlink attacks for benign concurrent-
modification windows. Split into a sibling `WorkspaceInitRaceError`
class (`kind: 'eexist' | 'enoent'`, HTTP code
`workspace_init_race`). The genuine symlink class stays for ELOOP,
lstat-detected target symlinks, and parent-realpath escapes.

S3 — fsConstants.O_NOFOLLOW defensive `?? 0` (#3263954697): matches
the existing codebase convention in
`core/src/utils/{sessionStorageUtils,gitDiff}.ts` and
`cli/src/ui/utils/customBanner.ts`. Functionally a no-op (JS
bitwise coerces undefined to 0) but consistent.

S5 — Parent-directory TOCTOU still open (#3263954707): O_NOFOLLOW
only protects the final path component; a local writer could swap
a real parent dir for a symlink between
`canonicalizeExistingAncestor` and `fs.open`. Added
`verifyParentWithinWorkspace` post-open helper that re-realpaths
`path.dirname(target)` and refuses with
`WorkspaceInitSymlinkError(kind: 'parent')` if the parent moved.
On the create path (where we just opened with `'wx'`), the failure
also unlinks the file we just made best-effort. Residual race
window narrowed from "between pre-check and open" to "between
post-open realpath and writeFile" — sub-millisecond, documented as
accepted Stage-1 trust posture.

S4 — broadcastWorkspaceEvent vs publishWorkspaceEvent stale comment
(#3263954688): the "now removed" comment was inaccurate (5 call
sites still use the closure). Replaced with an accurate
description of why both coexist (factory closure can't `this`-call
proxy member; closure also takes `skipSessionId` for persisted
approval-mode mirror) and a TODO marker for future helper extraction.

Two existing tests updated to assert the new `WorkspaceInitRaceError`
class for EEXIST / ENOENT scenarios (the symlink-class assertions
are preserved for ELOOP / lstat / parent cases).

1759/1759 unit tests pass; typecheck clean across all 4 packages.

* feat(acp-bridge): F1 — acp-bridge package self-sufficiency (#4175 mechanical lift + BridgeFileSystem seam) (#4319)

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

First mechanical lift of #4175 F1 (acp-bridge package self-sufficiency).
Moves the production spawn factory + its `killChild` helper +
`SCRUBBED_CHILD_ENV_KEYS` denylist + `KILL_HARD_DEADLINE_MS` constant
from `cli/src/serve/httpAcpBridge.ts` (~283 lines) to
`@qwen-code/acp-bridge/spawnChannel`. This unblocks
`channels/base/AcpBridge.ts` and `vscode-ide-companion`'s
acpConnection from each reimplementing the child lifecycle — they can
now consume the same primitive.

Backward compatible: `cli/src/serve/httpAcpBridge.ts` imports the
lifted factory and re-exports it, so existing references in
`cli/src/serve/index.ts:90` and the factory's own internal usage
(`opts.channelFactory ?? defaultSpawnChannelFactory`) keep resolving.
Bridge tests that mock `defaultSpawnChannelFactory` via
`BridgeOptions.channelFactory` are unaffected.

Side cleanups: drops `spawn` / `ChildProcess` / `Readable` / `Writable`
/ `ndJsonStream` / `MissingCliEntryError` imports from
httpAcpBridge.ts (all only used by the lifted spawn factory).

- 44/44 acp-bridge tests pass
- 174/174 cli httpAcpBridge tests pass
- typecheck clean across acp-bridge + cli

🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)

* refactor(acp-bridge): lift BridgeClient + permission types to acp-bridge/bridgeClient (#4175 F1 step 2)

Second mechanical lift of #4175 F1 (acp-bridge package self-sufficiency).
Moves `BridgeClient` class (~700 LOC) + `PendingPermission` interface +
`PermissionResolutionRecord` interface + `MAX_RESOLVED_PERMISSION_RECORDS`
constant + early-event capacity constants + `describeStatKind` and
`sliceLineRange` helpers from `cli/src/serve/httpAcpBridge.ts` to
`@qwen-code/acp-bridge/bridgeClient`.

Design choice for SessionEntry boundary: introduce a minimal
`BridgeClientSessionEntry` interface in bridgeClient.ts with only the
four fields BridgeClient actually reads from the factory's richer
`SessionEntry` (`sessionId`, `events`, `pendingPermissionIds`,
`activePromptOriginatorClientId`). The factory's `SessionEntry`
structurally satisfies it — TypeScript's structural typing enforces
the match at the `resolveEntry` callback signature, so no explicit
conversion is required and the bridge package stays free of daemon-host
session-bookkeeping types.

Cross-package writeStderrLine handling: inline the 3-line helper in
bridgeClient.ts (mirrors the spawnChannel.ts pattern from F1 step 1)
so acp-bridge has no reverse dependency on `cli/src/utils/stdioHelpers`.

httpAcpBridge.ts shrinks from 4406 LOC to 3647 LOC (-759 lines).
Removed ACP SDK imports that only BridgeClient consumed: `Client`,
`RequestPermissionRequest`, `WriteTextFileRequest`,
`WriteTextFileResponse`, `ReadTextFileRequest`, `ReadTextFileResponse`,
`SessionNotification`. Kept the ones the factory still uses
(`CancelNotification`, `PromptRequest`, `RequestPermissionResponse`,
`SetSessionModelRequest`, `SetSessionModelResponse`).

Backward compatible: httpAcpBridge.ts re-exports `BridgeClient`,
`BridgeClientSessionEntry`, `PendingPermission`,
`PermissionResolutionRecord`, and `MAX_RESOLVED_PERMISSION_RECORDS` so
the `ChannelInfo.client: BridgeClient` field declaration below + any
embedder reaching into these types keep resolving.

- 44/44 acp-bridge tests pass
- 174/174 cli httpAcpBridge tests pass
- 229/229 cli server tests pass
- typecheck clean across acp-bridge + cli

🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)

* refactor(acp-bridge): lift createHttpAcpBridge factory to acp-bridge/bridge (#4175 F1 step 3)

Third + final mechanical lift of #4175 F1 (acp-bridge package
self-sufficiency). Moves the `createHttpAcpBridge` factory closure
(~3000 LOC) + `ChannelInfo` + `SessionEntry` interfaces + factory-only
helpers (`canonicalizeExistingAncestor`, `verifyParentWithinWorkspace`,
`withTimeout`, `isServeDebugLoggingEnabled`, `writeServeDebugLine`,
`hasControlCharacter`) + factory constants (`DEFAULT_INIT_TIMEOUT_MS`,
`MCP_RESTART_TIMEOUT_MS`, `DEFAULT_MAX_SESSIONS`, `MAX_EVENT_RING_SIZE`,
`DEFAULT_PERMISSION_TIMEOUT_MS`, `DEFAULT_MAX_PENDING_PER_SESSION`,
`MAX_DISPLAY_NAME_LENGTH`) from `cli/src/serve/httpAcpBridge.ts` to
`@qwen-code/acp-bridge/bridge`.

`cli/src/serve/httpAcpBridge.ts` shrinks from 3647 LOC to 97 LOC — a
pure re-export shim that preserves every existing relative import
path (`./httpAcpBridge.js`) so `server.ts`, `runQwenServe.ts`,
`workspaceAgents.ts`, `workspaceMemory.ts`, `index.ts`, plus the bridge
test suite, keep resolving without any call-site changes.

The new `bridge.ts` reuses what was already in acp-bridge (errors,
types, options, status helpers, channel types, event bus, workspace
paths) via local relative imports — no reverse dependency on `cli`.
`writeStderrLine` is inlined at the top of `bridge.ts` (same pattern as
`spawnChannel.ts` + `bridgeClient.ts` from F1 steps 1-2) so the
package self-contained promise holds.

Cumulative F1 impact across the 3 mechanical lift steps:
- httpAcpBridge.ts: 4682 LOC → 97 LOC (-4585 lines; the original file
  was 98% bridge core, 2% backward-compat re-exports)
- 3 new files in acp-bridge: spawnChannel.ts (~270 LOC), bridgeClient.ts
  (~745 LOC), bridge.ts (~3515 LOC)
- All daemon-host concerns (env snapshot, daemon preflight cells)
  remain in `cli/src/serve/daemonStatusProvider.ts` and reach the
  bridge through the `BridgeOptions.statusProvider` seam frozen by
  PR 22b/2.

- 735/735 cli serve tests pass across 17 files
- 174/174 cli httpAcpBridge tests pass
- 44/44 acp-bridge tests pass
- typecheck clean across acp-bridge + cli

`packages/cli/src/serve/httpAcpBridge.test.ts` (~6600 LOC) is
intentionally NOT moved in this commit — it currently imports
`createHttpAcpBridge` / `defaultSpawnChannelFactory` / `BridgeClient`
via the cli shim and keeps passing without changes. Moving it to
`acp-bridge/src/bridge.test.ts` is a follow-up worth tracking
separately so the production-code lift can land + be reviewed cleanly.

The `BridgeFileSystem` injection seam (originally bundled into F1 as
the 22b' scope) is also deferred to a follow-up so the mechanical lift
stays mechanical — design + implementation of the fs injection is its
own discussion.

🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)

* feat(acp-bridge): add BridgeFileSystem injection seam (#4175 F1 step 5, 22b' scope)

Adds the `BridgeFileSystem` injection seam originally scoped as #4175
22b'. When a `BridgeFileSystem` is wired through
`BridgeOptions.fileSystem`, `BridgeClient.readTextFile` and
`BridgeClient.writeTextFile` delegate to it instead of running their
inline `fs.realpath` / `fs.writeFile` / `fs.readFile` proxy.

This unblocks production `qwen serve` plumbing PR 18's
`WorkspaceFileSystem` (TOCTOU guards, symlink-substitution checks,
trust gate, `.gitignore`, audit hooks) into the ACP fs methods —
closing the `ws.ts:613` follow-up thread that has been tracked since
PR 18 landed. The serve-side adapter that wraps `WorkspaceFileSystem`
+ the `runQwenServe` wiring are intentionally split into the
immediate-follow-up so this PR stays focused on the seam design.

Backward compatible: `fileSystem` is optional on `BridgeOptions`.
Tests, Mode A in-process consumers, channels (`packages/channels/base/
AcpBridge.ts`), and the VSCode IDE companion all keep working
unchanged — they omit the field and `BridgeClient` falls through to
the inline proxy that has been the Stage 1 default since #3889.

API:
- `BridgeFileSystem.readText(params: ReadTextFileRequest):
  Promise<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 prox…