fix: 7 wenshao/copilot review fold-ins on #4360 (1 Critical + 6 Suggestion)

Adopts all 7 review threads from the first wenshao + Copilot review
round on PR #4360. All technical fixes (no judgment calls).

**[Critical] BridgeTimeoutError constructor blocks tsc** (wenshao
PRRT_kwDOPB-92c6DfcRI)

`server.test.ts:4670` called `new BridgeTimeoutError('initialize
timed out')` but the constructor signature is `(label: string,
timeoutMs: number)` — TS2554 blocked `tsc --noEmit` and `npm run
build`. Fixed to `new BridgeTimeoutError('initialize', 5000)` per
suggested fix; resulting message `"HttpAcpBridge initialize timed
out after 5000ms"` still satisfies the existing
`.toContain('timed out')` assertion.

**[Suggestion] Copilot JSDoc package name** (Copilot
PRRT_kwDOPB-92c6De-Sm, ToolCallEmitter.ts:210)

JSDoc referenced `@qwen-code/core/mcp-tool` but the actual package
is `@qwen-code/qwen-code-core` with the file at
`packages/core/src/tools/mcp-tool.ts`. Updated the reference.

**[Suggestion] Copilot errorKind type widening** (Copilot
PRRT_kwDOPB-92c6De-Ro, events.ts:244)

`DaemonStreamErrorData.errorKind` was typed as `string` and the
JSDoc said "7-value" closed enum — but `DAEMON_ERROR_KINDS` actually
has 8 values, and `SERVE_ERROR_KINDS` (daemon-side) has 9 (adds
`stat_failed`). Typed as `DaemonErrorKind | (string & {})` for
forward-compat: SDK consumers get IDE autocomplete on the known 8
kinds while still accepting future daemon-side additions (like
`stat_failed`) without a type error. Updated JSDoc to accurately
list 8 current values + call out the forward-compat widening.

Side observation (NOT in scope of this PR): `DAEMON_ERROR_KINDS`
(SDK) lacks `stat_failed` that exists in `SERVE_ERROR_KINDS`
(daemon). That's a separate drift fix.

**[Suggestion] TERMINAL wording misleading** (wenshao
PRRT_kwDOPB-92c6Dj-JL, eventBus.ts:369)

Comment called `state_resync_required` a "TERMINAL synthetic frame"
but it's emitted FIRST (before replay) and the stream stays OPEN.
Genuine terminals like `client_evicted` close the stream after the
frame. Rewrote the comment per suggestion: "id-less synthetic
frame... Unlike `client_evicted`, the stream stays OPEN" — so an
oncall reading the source at 3am gets the right mental model.

**[Suggestion] `_meta` merge dead code + stale reference** (wenshao
PRRT_kwDOPB-92c6Dj-JF, server.ts:2569)

The `existingMeta` merge reads `event._meta` at BridgeEvent top
level, but ToolCallEmitter's `_meta` lives nested inside
`event.data._meta` (publish path goes through `events.publish({type:
'session_update', data: params})`). In production `existingMeta` is
always undefined — the merge is a forward-compat escape hatch, not
an active merge. Also the comment referenced
`extractServerTimestamp` (sdk-typescript) which grep confirms
doesn't exist yet (it's planned in chiga0 PR #4353).

Rewrote the comment block to (1) acknowledge no current producer
sets `_meta` at the top level — it's a forward-compat hook for
future envelope-level metadata; (2) drop the stale
`extractServerTimestamp` reference and instead note that chiga0
PR #4353 plans the 3-location probe. Code shape unchanged
(forward-compat spread stays).

**[Suggestion] session_closed + client_evicted passthrough tests**
(wenshao PRRT_kwDOPB-92c6Dj-JW, daemonEvents.test.ts:2284)

`RESYNC_PASSTHROUGH_TYPES` has 5 members but only `session_died`
and `stream_error` had passthrough tests. Added two missing tests:
`session_closed` and `client_evicted` while awaitingResync.
Critical because if a future refactor accidentally drops either
from the set, a consumer in resync limbo would silently swallow
the terminal signal and the UI would hang on "loading resync
state…".

**[Suggestion] readTextFile non-FsError passthrough test** (wenshao
PRRT_kwDOPB-92c6Dj-JX, bridgeClient.test.ts:251)

The non-FsError pass-through test only covered `writeTextFile`.
Added a symmetric `readTextFile` test — the two `try/catch` blocks
in `bridgeClient.ts` are independent, so test parity guards against
divergent refactors (e.g. someone adding wrapping on one side but
not the other).

Verification
- `packages/acp-bridge`: 6 files, 114/114 pass (+1 new readTextFile
  non-FsError test).
- `packages/sdk-typescript`: 75/75 pass on daemonEvents.test.ts
  (+2 new session_closed / client_evicted passthrough tests).
- `packages/cli/src/serve/server.test.ts`: 248 tests pass on
  touched cases (5 SSE / serverTimestamp / stream_error tests).
  Pre-existing F3 (#4335 merge) test failures unrelated to this
  PR's changes — verified by stash-test-restore on clean tree.
- TypeScript clean on touched regions; `BridgeTimeoutError`
  2-arg fix unblocks `tsc --noEmit` for the test file.

Author: doudouOUC

packages/acp-bridge/src/bridgeClient.test.ts

@@ -272,6 +272,27 @@ describe('BridgeClient — BridgeFileSystem injection seam (F1 step 5)', () => {
       expect(err.data).toBeUndefined();
     });
 
+    it('readTextFile passes non-FsError errors through unchanged (wenshao #4360 review)', async () => {
+      // Symmetric guard for the read-side `preserveFsErrorOverAcp`
+      // call. The write- and read-side catch blocks are independent
+      // try/catch wrappers in `bridgeClient.ts`; if a future refactor
+      // diverges them (e.g. adds Error-wrapping to one but not the
+      // other), this test catches the read-side regression.
+      const readText = vi.fn(async (): Promise<ReadTextFileResponse> => {
+        throw new Error('generic read failure');
+      });
+      const client = makeClient({ writeText: vi.fn(), readText });
+
+      const err = (await client
+        .readTextFile({ path: '/x', sessionId: 'sess:test' })
+        .catch((e) => e)) as Error & { code?: number; data?: unknown };
+
+      expect(err.name).toBe('Error');
+      expect(err.message).toBe('generic read failure');
+      expect(err.code).toBeUndefined();
+      expect(err.data).toBeUndefined();
+    });
+
     it('preserves hint field when present on the FsError', async () => {
       const writeText = vi.fn(async (): Promise<WriteTextFileResponse> => {
         throw makeFsError(

packages/acp-bridge/src/eventBus.ts

@@ -366,21 +366,26 @@ export class EventBus {
       // caught up!") even though the SDK reducer's state was now
       // diverged from the daemon's truth.
       //
-      // Emit `state_resync_required` as a TERMINAL synthetic frame
-      // (no `id` — same pattern as `client_evicted` so it doesn't
-      // burn a slot in the per-session monotonic sequence other
-      // subscribers observe). The SDK reducer treats this as "your
-      // state is stale; call loadSession before applying any further
-      // deltas" — see `awaitingResync` flag in the SDK reducer.
+      // Emit `state_resync_required` as an id-less synthetic frame
+      // (no `id` — same no-burn pattern as `client_evicted`, so it
+      // doesn't occupy a slot in the per-session monotonic sequence
+      // other subscribers observe). **Unlike `client_evicted`, the
+      // stream stays OPEN after this frame** — the resync frame is
+      // emitted FIRST (before replay), and replay + live frames
+      // continue flowing afterward. The SDK reducer treats this as
+      // "your state is stale; call loadSession before applying any
+      // further deltas" — see `awaitingResync` flag in the SDK
+      // reducer. Wenshao review (#4360) corrected the prior wording
+      // that called this "TERMINAL" — that's misleading for oncall;
+      // `client_evicted` is genuinely terminal (closes stream),
+      // `state_resync_required` is recovery-oriented (keeps stream
+      // open).
       //
       // Replay continues after the resync frame (per design): the
       // SDK reducer will auto-skip delta application until
       // loadSession clears the flag, but the frames stay on the
       // wire so SDK has the option to compute a "what you missed"
-      // diff later. This is network-friendly (no extra reconnect)
-      // and matches the existing `client_evicted` semantics
-      // (terminal-on-close, but consumer can still drain queued
-      // frames first).
+      // diff later. This is network-friendly (no extra reconnect).
       const earliestInRing = this.ring[0]?.id;
       if (
         earliestInRing !== undefined &&

packages/cli/src/acp-integration/session/emitters/ToolCallEmitter.ts

@@ -205,9 +205,11 @@ export class ToolCallEmitter extends BaseEmitter {
    *   - `subagentMeta` present → `'subagent'` (a Task tool / Codex
    *     subagent / etc. wrapping its own tool calls)
    *   - toolName matches `mcp__<server>__<tool>` → `'mcp'` with
-   *     `serverId: <server>`. Naming convention from `@qwen-code/core/
-   *     mcp-tool` — mirrors the SDK's same heuristic fallback so SDK
-   *     consumers stay consistent with daemon classification.
+   *     `serverId: <server>`. Naming convention from
+   *     `packages/core/src/tools/mcp-tool.ts` in the
+   *     `@qwen-code/qwen-code-core` package — mirrors the SDK's same
+   *     heuristic fallback so SDK consumers stay consistent with
+   *     daemon classification.
    *   - everything else → `'builtin'`
    *
    * Static + pure so it can be unit-tested without an emitter

packages/cli/src/serve/server.test.ts

@@ -4667,7 +4667,11 @@ describe('GET /session/:id/events (SSE)', () => {
     const bridge = fakeBridge({
       async *subscribeImpl(_sessionId, _opts) {
         yield { id: 1, v: 1, type: 'session_update', data: 'first' };
-        throw new BridgeTimeoutError('initialize timed out');
+        // `BridgeTimeoutError(label, timeoutMs)` — 2 positional args
+        // (wenshao #4360 review). The resulting message is
+        // `"HttpAcpBridge initialize timed out after 5000ms"` which
+        // satisfies the `.toContain('timed out')` assertion below.
+        throw new BridgeTimeoutError('initialize', 5000);
       },
     });
     handle = await runQwenServe(

packages/cli/src/serve/server.ts

@@ -2559,13 +2559,28 @@ function formatSseFrame(event: BridgeEvent | OmitId<BridgeEvent>): string {
   // ordering and "X minutes ago" UIs use the server's clock rather than
   // each client's drifting local clock. Stamped at the wire boundary
   // (NOT at EventBus.publish) so the in-memory `BridgeEvent` type stays
-  // unchanged and internal consumers don't see `_meta`. Pre-existing
-  // `_meta` keys on the event (e.g. tool_call carries `_meta.toolName` /
-  // `_meta.timestamp` from ToolCallEmitter) are preserved by spreading
-  // first. SDK consumers read via the 3-location probe in
-  // `extractServerTimestamp` (sdk-typescript) — `_meta.serverTimestamp`
-  // is one of the supported locations; the other two stay free for
-  // future use.
+  // unchanged and internal consumers don't see `_meta`.
+  //
+  // **Where `_meta` lives**: this code merges with `event._meta` at the
+  // BridgeEvent top level. **No current producer in the daemon sets
+  // `_meta` at the top level** — wenshao #4360 review noted that
+  // ToolCallEmitter's `_meta` lives nested at `event.data._meta` (the
+  // ACP `session/update` payload's own metadata), and `bridgeClient.ts`
+  // publishes via `events.publish({ type: 'session_update', data:
+  // params })` so the emitter's `_meta` stays inside `data`. The
+  // top-level merge here is a **forward-compat escape hatch** — if a
+  // future emitter ever wants to stamp envelope-level metadata, it can
+  // do so via `_meta` and this spread preserves it. Today
+  // `existingMeta` is always `undefined` in production and the spread
+  // is a no-op merge with `{}`.
+  //
+  // SDK consumer plan: a 3-location probe (event.serverTimestamp /
+  // event._meta.serverTimestamp / event.data._meta.serverTimestamp)
+  // is planned in chiga0's separate PR #4353 (not yet merged into
+  // `daemon_mode_b_main`). When that ships, SDK readers find this
+  // field at the `_meta.serverTimestamp` location. Until then this
+  // stamp is daemon-side only; SDK consumers can read it through an
+  // `as any` cast or wait for #4353.
   const existingMeta = (event as { _meta?: Record<string, unknown> })._meta;
   const stamped = {
     ...event,

packages/sdk-typescript/src/daemon/events.ts

@@ -6,6 +6,7 @@
 
 import type {
   DaemonEvent,
+  DaemonErrorKind,
   DaemonMcpTransport,
   PermissionOutcome,
 } from './types.js';
@@ -232,16 +233,23 @@ export interface DaemonStreamErrorData {
   error: string;
   /**
    * #4175 F4 prereq (chiga0 issue #19 P0). Classified error kind from
-   * the daemon's `mapDomainErrorToErrorKind` — one of the 7-value
-   * closed enum (`init_timeout` / `protocol_error` / `missing_binary`
-   * / `auth_env_error` / `blocked_egress` / `missing_file` /
-   * `parse_error`). Absent for unclassified errors — the daemon omits
-   * the field rather than stamping a meaningless value. UI consumers
-   * key on this for typed retry / remediation rendering (retry on
-   * init_timeout vs install on missing_binary, etc.) instead of
-   * regex-matching the `error` string.
+   * the daemon's `mapDomainErrorToErrorKind` — typed as the closed
+   * `DaemonErrorKind` enum (currently 8 values: `missing_binary` /
+   * `blocked_egress` / `auth_env_error` / `init_timeout` /
+   * `protocol_error` / `missing_file` / `parse_error` /
+   * `budget_exhausted`) with a `(string & {})` widening for forward-
+   * compat. A future daemon may emit additional kinds (e.g.
+   * `stat_failed` already exists on the daemon's `SERVE_ERROR_KINDS`
+   * but is not yet mirrored on this SDK constant) — the union shape
+   * preserves IDE autocomplete on the known values while accepting
+   * forward-compat strings without a type error. Absent for
+   * unclassified errors — the daemon omits the field rather than
+   * stamping a meaningless value. UI consumers key on this for typed
+   * retry / remediation rendering (retry on init_timeout vs install
+   * on missing_binary, etc.) instead of regex-matching the `error`
+   * string.
    */
-  errorKind?: string;
+  errorKind?: DaemonErrorKind | (string & {});
   [key: string]: unknown;
 }
 

packages/sdk-typescript/test/unit/daemonEvents.test.ts

@@ -2334,6 +2334,66 @@ describe('PR 21 — auth device-flow events', () => {
       expect(errored.streamError).toEqual({ error: 'transport gone' });
     });
 
+    it('still applies session_closed while awaitingResync (wenshao #4360 review)', () => {
+      // session_closed is in RESYNC_PASSTHROUGH_TYPES alongside
+      // session_died — terminal session lifecycle signals must still
+      // surface even when the consumer is in resync limbo. Otherwise
+      // a session that closes during resync would silently keep
+      // `alive: true` in view state and the UI would render "loading
+      // resync state…" indefinitely.
+      const afterResync = reduceDaemonSessionEvent(
+        createDaemonSessionViewState(),
+        {
+          v: 1,
+          type: 'state_resync_required',
+          data: {
+            reason: 'ring_evicted',
+            lastDeliveredId: 5,
+            earliestAvailableId: 12,
+          },
+        },
+      );
+      const closed = reduceDaemonSessionEvent(afterResync, {
+        id: 15,
+        v: 1,
+        type: 'session_closed',
+        data: { sessionId: 's-1', reason: 'client_initiated' },
+      });
+      expect(closed.alive).toBe(false);
+      expect(closed.terminalEvent?.type).toBe('session_closed');
+      // awaitingResync stays set (consumer never recovered) — the
+      // terminal event takes precedence for UI rendering but the
+      // resync flag remains as observability state.
+      expect(closed.awaitingResync).toBe(true);
+    });
+
+    it('still applies client_evicted while awaitingResync (wenshao #4360 review)', () => {
+      // client_evicted is the 5th member of RESYNC_PASSTHROUGH_TYPES.
+      // It happens when the subscriber's queue overflows (the daemon
+      // closes the stream after force-pushing the synthetic frame).
+      // Even in resync limbo, the SDK must see the eviction so the
+      // adapter can stop pretending the stream is alive.
+      const afterResync = reduceDaemonSessionEvent(
+        createDaemonSessionViewState(),
+        {
+          v: 1,
+          type: 'state_resync_required',
+          data: {
+            reason: 'ring_evicted',
+            lastDeliveredId: 5,
+            earliestAvailableId: 12,
+          },
+        },
+      );
+      const evicted = reduceDaemonSessionEvent(afterResync, {
+        v: 1,
+        type: 'client_evicted',
+        data: { reason: 'queue_overflow', droppedAfter: 17 },
+      });
+      expect(evicted.alive).toBe(false);
+      expect(evicted.terminalEvent?.type).toBe('client_evicted');
+    });
+
     it('reseeding view state via createDaemonSessionViewState clears awaitingResync (consumer recovery)', () => {
       // Consumer recovery path: after observing awaitingResync, call
       // loadSession (out of band) and reconstruct view state. The