Skip to content

feat(serve): F1 follow-up — BridgeFileSystem wiring + #4325 channelInfo fix#4334

Merged
doudouOUC merged 7 commits into
daemon_mode_b_mainfrom
feat/f1-followup-fs-adapter-test-split-channelinfo
May 20, 2026
Merged

feat(serve): F1 follow-up — BridgeFileSystem wiring + #4325 channelInfo fix#4334
doudouOUC merged 7 commits into
daemon_mode_b_mainfrom
feat/f1-followup-fs-adapter-test-split-channelinfo

Conversation

@doudouOUC

@doudouOUC doudouOUC commented May 19, 2026

Copy link
Copy Markdown
Collaborator

Summary

Three F1 (#4319) follow-ups, batched into one PR against daemon_mode_b_main:

  1. BridgeFileSystem wiring (9560dfc28) — closes ws.ts:613 TOCTOU thread. ACP writeTextFile / readTextFile now go through PR 18's WorkspaceFileSystem (trust gate + symlink resolution + atomic temp-file write + line/limit windowing + unified audit emit) instead of BridgeClient's inline raw-fs proxy. F1 shipped the BridgeFileSystem seam in BridgeClient; this commit wires the default createServeApp / runQwenServe paths to actually inject the adapter so the seam is live in production.
  2. bug(acp-bridge): closeSession + killSession use module-scoped channelInfo instead of channelInfoForEntry(entry) #4325 channelInfo overlap fix (db8972b81) — closeSession + killSession now use channelInfoForEntry(entry) instead of the module-scoped channelInfo, matching the pattern already in setSessionApprovalMode + requestSessionStatus. Eliminates the latent bookkeeping mismatch during channel-overlap windows.
  3. Mode preservation + atomic write for ACP writes (881133407) — addresses the Copilot review finding (discussion) on commit 1's adapter. Pre-fix, ACP writes routed through wfs.writeText had no mode handling, so new files got umask-default (0o644) instead of the BridgeFileSystem contract's 0o600, and existing target mode wasn't preserved (a 0o600 secret edit would downgrade to umask-default). Fix introduces a new reusable PR 18 primitive WorkspaceFileSystem.writeTextOverwrite — unconditional create-or-overwrite with mode preservation + 0o600 default + atomic temp+rename, via a new WriteMode = 'overwrite' variant that tolerates ENOENT (new file) and skips the expectedHash CAS gate (which doesn't fit ACP's hash-less wire). Adapter switched to it; contract docstring updated to reflect production posture (write-then-rename atomicity, mode preservation, 0o600 default, symlink rejection — a divergence from pre-F1 inline-proxy semantics, now consistent with HTTP POST /file from PR 20).

Files

New (fs adapter, commit 1):

  • packages/cli/src/serve/bridgeFileSystemAdapter.ts (~110 LOC) — thin translation from ACP WriteTextFileRequest / ReadTextFileRequestWorkspaceFileSystem.resolvewriteTextOverwrite / readText. Drops ACP-wire null line/limit (PR 18 wants undefined). Routes labeled 'ACP writeTextFile' / 'ACP readTextFile' so audit consumers can distinguish agent fs from HTTP fs.
  • packages/cli/src/serve/bridgeFileSystemAdapter.test.ts (12 tests after commit 3 added mode-preservation assertions) — happy paths, trust-gate deny, boundary rejection (writes + reads), line/limit window, null→undefined normalization, factory.forRequest audit-context wiring, new-file 0o600 default + existing-target mode preservation through the adapter (commit 3, skipped on Windows).

Modified (fs adapter, commit 1):

  • packages/cli/src/serve/runQwenServe.ts + packages/cli/src/serve/server.tsfsFactory constructed BEFORE the bridge default; bridge gets fileSystem: createBridgeFileSystemAdapter(fsFactory). Same factory instance feeds HTTP fs routes + ACP fs → single operator audit stream covers both.

Modified (#4325 fix, commit 2):

  • packages/acp-bridge/src/bridge.tscloseSession + killSession use channelInfoForEntry(entry).
  • packages/cli/src/serve/httpAcpBridge.test.ts — smoke regression test for channelInfoForEntry routing.

Modified (writeTextOverwrite, commit 3):

  • packages/cli/src/serve/fs/workspaceFileSystem.ts — new writeTextOverwrite public method (~75 LOC); WriteMode extended with 'overwrite'; validateWriteTextAtomicOptions accepts new mode without expectedHash; assertAtomicTargetPrecondition gains 'overwrite' branch that tolerates ENOENT and returns existing target mode for preservation.
  • packages/cli/src/serve/fs/workspaceFileSystem.test.ts — +6 tests for writeTextOverwrite (new file 0o600, preserve mode bits, preserve +x, symlink rejection, trust gate, audit emit).
  • packages/cli/src/serve/bridgeFileSystemAdapter.ts — adapter swapped from wfs.writeTextwfs.writeTextOverwrite. Docstring updated to explain primitive choice (writeText has no mode handling, writeTextAtomic requires expectedHash that ACP can't provide).
  • packages/acp-bridge/src/bridgeFileSystem.ts — contract docstring rewritten to reflect production posture: write-then-rename atomicity, target mode preservation, 0o600 default for new files, symlink rejection (divergence from pre-F1 inline-proxy realpath/readlink follow, now matches PR 18 + HTTP POST /file from PR 20).

Test plan

  • npx vitest run packages/cli/src/serve/bridgeFileSystemAdapter.test.ts packages/cli/src/serve/fs/workspaceFileSystem.test.ts — 81/81 pass on touched files
  • npx vitest run packages/cli/src/serve/ — 18 files, 754/754 pass
  • npx vitest run (packages/acp-bridge/) — 5 files, 62/62 pass
  • Pre-commit hook (prettier --write + eslint --fix --max-warnings 0) clean on all staged files
  • CI green across Linux / macOS / Windows
  • Smoke: qwen serve boot, ACP child agent writeTextFile (new file → 0o600 on disk; overwriting an existing 0o600 file → still 0o600; symlinked target rejected with symlink_escape) + readTextFile + boundary rejection observable via /workspace/audit/events

Backward compat

  • BridgeOptions.fileSystem was already optional in F1 (seam-only). Embeds that don't pass it keep using BridgeClient's inline raw-fs proxy — this PR only changes the default createServeApp + runQwenServe wiring.
  • channelInfoForEntry(entry) returns the same record as the module-scoped channelInfo when there's no channel overlap (the common case), so single-channel callers see identical behavior; the fix only affects overlap windows where prior behavior was the latent bug.
  • WorkspaceFileSystem.writeTextOverwrite is a new method on the interface; pre-existing in-tree consumers of WorkspaceFileSystem are unaffected. Out-of-tree TS consumers implementing the interface manually would need to add the method (no known out-of-tree consumers — @qwen-code/acp-bridge is not yet npm-published, and PR 18 lives in cli/src/serve/ not in a published package).
  • WriteMode union widened from 'create' | 'replace''create' | 'replace' | 'overwrite'. The new value is opt-in via writeTextAtomic({mode: 'overwrite', ...}) or the new writeTextOverwrite method; existing callers using 'create' / 'replace' see no behavior change.
  • Behavior divergence from pre-F1 ACP fs: agent writes through symlinks now reject with symlink_escape (consistent with HTTP POST /file since PR 20). Pre-F1 inline BridgeClient.writeTextFile resolved symlinks via realpath / readlink and wrote through to the target. Agents that previously relied on writing through symlinked dotfiles will need to address the resolved path directly. Documented in bridgeFileSystem.ts contract docstring.

Refs

🤖 Generated with Qwen Code

doudouOUC added 2 commits May 20, 2026 00:48
#4325)

Folds in the deferred fix from F1 (#4319) for #4325. Pre-fix both
methods captured `const ci = channelInfo` — the module-scoped CURRENT
attach target — rather than `channelInfoForEntry(entry)`. The two
diverge during the channel-overlap window (A dying, B freshly spawned
as `channelInfo`), where closing or killing a session whose
`entry.channel = A` would:

  1. Skip `A.sessionIds.delete()` because `B.channel !== A.channel`,
     leaving A's `sessionIds` set pinned past the close;
  2. Call `markSessionClosed` on **B**'s client instead of **A**'s,
     evaluating B's kill condition with stale assumptions about its
     session count — potentially killing B unnecessarily and forcing
     a third spawn cascade.

Other session methods in the same factory (`setSessionApprovalMode`
at ~L2609, `requestSessionStatus` at ~L1245) already use the
`channelInfoForEntry(entry)` helper; this brings `closeSession` and
`killSession` in line with that pattern.

Net change: 2 lines (one in each method) replaced; surrounding
comment blocks updated to document the channel-overlap rationale +
the matching sibling-method consistency argument.

## Why the smoke test rather than a full overlap regression

The exact bug-triggering state is hard to construct deterministically
under the current factory architecture:

  - A only flips `isDying = true` when its `sessionIds` drains to 0
  - The drain path (`killSession` or `closeSession` on the last
    session) also removes the session from `byId` synchronously
  - So by the time `channelInfo` could move to B, every session that
    was on A is gone from `byId` and thus unreachable to a subsequent
    `closeSession`

A faithful overlap regression test requires a test-only factory
inspection seam (manual `channelInfo` override, or a hook into
`aliveChannels` mutation). Adding that seam is non-trivial and
expands the bridge's public surface — out of F1-followup scope.

What this commit ships:

  - The 2-line fix itself (matches the sibling-method pattern; the
    correctness argument is structural, not race-empirical)
  - A smoke regression test at `httpAcpBridge.test.ts` exercising
    `closeSession` on the normal single-channel case and asserting
    the kill-on-last-session cascade fires correctly — would fail
    trivially if a future refactor reverted to module-scoped
    `channelInfo` capture without thinking through the
    `channelInfoForEntry → undefined` case
  - Inline comments at both fix sites + on the new test documenting
    why the full overlap repro is deferred

A follow-up issue can track adding the factory inspection seam +
the deterministic overlap regression test if anyone needs the
empirical guard rather than the structural one.

- 175/175 cli httpAcpBridge tests pass (174 existing + 1 new #4325 smoke)
- 62/62 acp-bridge tests pass (no regression)
- typecheck + eslint clean
- Closes #4325

🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
…follow-up #4319)

Closes the ws.ts:613 TOCTOU thread that PR 18 (`WorkspaceFileSystem`)
flagged and that F1 (#4319) deliberately left to a follow-up by
shipping only the `BridgeFileSystem` injection seam in `BridgeClient`.

Pre-fix, ACP `writeTextFile` / `readTextFile` calls landed in
`BridgeClient`'s inline `fs.realpath` / `fs.writeFile` / `fs.readFile`
proxy, bypassing PR 18's defensive layer (trust gate, symlink
resolution, atomic temp-file write, line/limit windowing, audit emit).
HTTP `POST /file` / `GET /file` already routed through that layer —
agent fs and HTTP fs diverged in posture.

Changes
- New `bridgeFileSystemAdapter.ts` (~110 LOC): thin translation from
  ACP `WriteTextFileRequest` / `ReadTextFileRequest` to
  `WorkspaceFileSystem.resolve` → `writeText` / `readText`. Drops
  ACP-wire `null` line/limit (PR 18 wants `undefined`). Routes labeled
  `'ACP writeTextFile'` / `'ACP readTextFile'` so the unified audit
  stream can distinguish agent fs from HTTP fs at the consumer side.
- `runQwenServe.ts` + `server.ts`: construct `fsFactory` BEFORE the
  bridge default and pass `fileSystem: createBridgeFileSystemAdapter(fsFactory)`
  into `BridgeOptions`. Same factory instance feeds both HTTP fs
  routes and ACP fs → single operator audit stream covers both.
- New `bridgeFileSystemAdapter.test.ts` (10 tests, all pass): happy
  paths (trusted write + read), trust-gate deny, boundary rejection
  (writes + reads outside workspace), line/limit window, null→undefined
  normalization, `factory.forRequest` audit-context wiring (sessionId
  forwarded, omitted when ACP request lacks one).

Backward compatibility
- `BridgeOptions.fileSystem` was already optional in F1 (seam-only);
  embeds that don't pass it (or that pre-date this commit) keep using
  `BridgeClient`'s inline raw-fs proxy as before. This commit only
  changes the *default* `createServeApp` + `runQwenServe` wiring.

Verification
- `vitest run src/serve/`: 18 files, 746/746 tests pass (includes
  the 10 new adapter tests + the 175-test `httpAcpBridge.test.ts` that
  exercises the seam through `BridgeOptions.fileSystem`).
Copilot AI review requested due to automatic review settings May 19, 2026 17:20
@github-actions

Copy link
Copy Markdown
Contributor

📋 Review Summary

This PR delivers two F1 (#4319) follow-ups: (1) wiring the BridgeFileSystem adapter to close a TOCTOU thread in ws.ts:613, and (2) fixing the #4325 channelInfo overlap bug. The implementation is well-documented, thoroughly tested, and maintains backward compatibility. Overall assessment is positive—the changes close important security and correctness gaps with minimal risk.

🔍 General Feedback

🎯 Specific Feedback

🟢 Medium

  • File: packages/cli/src/serve/bridgeFileSystemAdapter.ts:85-92 - The readText method's null/undefined normalization is correct but could benefit from a small helper function to avoid repetition if similar patterns appear elsewhere. Consider extracting:
    function normalizeWindowParams(
      line: number | null | undefined,
      limit: number | null | undefined,
    ): { line?: number; limit?: number } {
      const opts: { line?: number; limit?: number } = {};
      if (typeof line === 'number') opts.line = line;
      if (typeof limit === 'number') opts.limit = limit;
      return opts;
    }

🔵 Low

  • File: packages/cli/src/serve/runQwenServe.ts:386-402 - The comment about constructing fsFactory BEFORE the bridge is excellent, but consider adding a brief code comment at the fsFactory declaration line itself (not just the block comment) for quicker scanning:

    // Construct fsFactory before bridge so ACP fs calls use PR 18's defensive layer
    const trustedWorkspace = deps.trustedWorkspace ?? true;
    const fsFactory = ...
  • File: packages/cli/src/serve/server.ts:241-258 - Similar to above, the symmetric change in createServeApp could use a one-line inline comment at the fsFactory declaration for consistency with runQwenServe.ts.

  • File: packages/cli/src/serve/httpAcpBridge.test.ts:6420-6475 - The regression test comment acknowledges that the full overlap state isn't deterministically testable without factory inspection seams. Consider filing a follow-up issue (#????) to add those test-only hooks so the complete race condition scenario can be verified, rather than deferring to an unspecified future PR.

✅ Highlights

  • Security win: The BridgeFileSystem wiring closes the TOCTOU vulnerability where agent-side ACP fs calls bypassed PR 18's trust gate, symlink resolution, atomic writes, and audit logging. This is a significant security improvement.
  • Correctness fix: The bug(acp-bridge): closeSession + killSession use module-scoped channelInfo instead of channelInfoForEntry(entry) #4325 channelInfoForEntry fix prevents a subtle but impactful bookkeeping mismatch during channel-overlap windows that could leave session sets pinned and incorrectly kill freshly-spawned channels.
  • Thoughtful audit design: Using route labels ('ACP writeTextFile' / 'ACP readTextFile') to distinguish agent-sourced fs from HTTP-sourced fs in the audit stream is a clean pattern that enables operators to trace both surfaces uniformly.
  • Defensive type handling: The adapter's explicit typeof checks for line/limit normalization (dropping both nulls and undefineds) show good attention to type boundary conditions between ACP schema and PR 18's expectations.
  • Test quality: The test file includes thoughtful edge cases like macOS /var/private/var symlink resolution in the setup, and the audit context tests verify both sessionId presence and absence scenarios.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR follows up on Mode B serve F1 by (1) wiring the BridgeFileSystem seam so ACP writeTextFile/readTextFile go through the serve WorkspaceFileSystem boundary (trust gate, boundary/symlink checks, audit emission), and (2) fixing channel bookkeeping during overlap windows by using channelInfoForEntry(entry) in closeSession/killSession.

Changes:

  • Inject a serve-side BridgeFileSystem adapter into the default createServeApp and runQwenServe bridge construction paths.
  • Add adapter + tests to route ACP fs operations through WorkspaceFileSystemFactory.forRequest(...) with ACP-specific audit route labels.
  • Fix @qwen-code/acp-bridge channel bookkeeping to use channelInfoForEntry(entry) in session close/kill, plus a smoke regression test.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
packages/cli/src/serve/server.ts Constructs fsFactory before the bridge and injects createBridgeFileSystemAdapter(fsFactory) into default bridge creation.
packages/cli/src/serve/runQwenServe.ts Builds fsFactory earlier and injects the adapter into the bridge, ensuring ACP fs shares the same trust/audit pipeline as HTTP fs routes.
packages/cli/src/serve/bridgeFileSystemAdapter.ts New adapter implementing BridgeFileSystem by translating ACP read/write requests to WorkspaceFileSystem.resolve + readText/writeText.
packages/cli/src/serve/bridgeFileSystemAdapter.test.ts New integration tests covering adapter happy paths, trust gate behavior, boundary rejection, line/limit windowing, and audit-context wiring.
packages/acp-bridge/src/bridge.ts Fixes #4325 by using channelInfoForEntry(entry) in closeSession and killSession.
packages/cli/src/serve/httpAcpBridge.test.ts Adds a smoke regression test guarding the channel bookkeeping neighborhood (#4325).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread packages/cli/src/serve/bridgeFileSystemAdapter.ts Outdated
…Copilot review)

Adopts Copilot's finding on PR #4334 (security-relevant):
#4334 (comment)

Pre-fix, the adapter routed ACP writeTextFile through
`WorkspaceFileSystem.writeText` which has no mode handling — new files
got umask-default (typically 0o644) and existing-target mode wasn't
preserved. The `BridgeFileSystem` contract requires 0o600 for new
files (NOT umask) and target mode preservation (a 0o600 secret edit
must stay 0o600). The old inline `BridgeClient.writeTextFile` proxy
did this; the adapter regressed it.

Fix: add a new `writeTextOverwrite` primitive to PR 18's
`WorkspaceFileSystem` (Approach B from the design discussion — picked
over CAS-in-adapter because the "unconditional create-or-overwrite
with mode preservation" semantic will recur in F4 TUI/IDE adapters
and future webhook integrations; cleaner to land it as a reusable PR
18 primitive now than retrofit later).

Implementation
- `WorkspaceFileSystem.writeTextOverwrite(p, content, opts?)` —
  unconditional create-or-overwrite, no expectedHash gate. Reuses the
  existing `atomicWriteTextResolvedFile` infrastructure via a new
  `WriteMode = 'overwrite'` variant: tolerates missing target (returns
  empty mode → 0o600 default), rejects symlinks (`symlink_escape`),
  preserves existing mode bits (`chmod` to `targetState.mode ?? 0o600`
  in line 1450). Path-locked the whole window; emits the same
  `fs.access` audit as `writeText` / `writeTextAtomic`.
- `assertAtomicTargetPrecondition` gains an `'overwrite'` branch that
  stats the target, returns its mode for preservation, and tolerates
  ENOENT (new file path); rejects symlinks / non-regular files in
  parity with `'replace'`.
- `validateWriteTextAtomicOptions` accepts `'overwrite'` mode WITHOUT
  expectedHash — that's the whole point of the new primitive (callers
  whose wire format has no client-side hash, like ACP).
- `atomicWriteTextResolvedFile`'s rename branch handles `'overwrite'`
  automatically (falls through to `renameWithRetryLocal` like
  `'replace'`; rename both clobbers existing and creates new).

Adapter switch
- `bridgeFileSystemAdapter.ts:96` — `wfs.writeText(resolved, content)`
  → `wfs.writeTextOverwrite(resolved, content)`. Updated docstring
  explains why this primitive over `writeText` (no mode) or
  `writeTextAtomic` (CAS gate doesn't fit ACP's hash-less wire).

Contract update
- `bridgeFileSystem.ts:61-93` — `writeText` doc now reflects the
  production posture: write-then-rename atomicity, target mode
  preservation, 0o600 default for new files, **symlink rejection**.
  The pre-F1 inline proxy resolved symlinks and wrote through to the
  target; PR 18 + HTTP `POST /file` (PR 20) reject them. The adapter
  now matches that posture, so ACP fs and HTTP fs behave identically
  — a divergence from pre-F1 ACP semantics, called out explicitly.

Tests (+10, 81 passing on touched files)
- workspaceFileSystem.test.ts: writeTextOverwrite creates new file at
  0o600, preserves existing target mode (0o600 secret stays 0o600),
  preserves +x executable bit, rejects post-resolve symlink swap with
  symlink_escape, enforces trust gate, emits fs.access.
- bridgeFileSystemAdapter.test.ts: through-adapter assertions that
  new files land at 0o600 and existing 0o600 secrets stay 0o600 after
  agent overwrite. Skipped on Windows (POSIX permission bits not
  honored). Symlink rejection is covered at the lower workspaceFileSystem
  layer to avoid duplicating the post-resolve-swap setup.
@doudouOUC doudouOUC requested a review from wenshao May 19, 2026 23:46

@wenshao wenshao left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No review findings. Downgraded from Approve to Comment: no passing CI checks were found for this PR head (only skipped/no applicable checks reported). — gpt-5.5 via Qwen Code /review

Comment thread packages/cli/src/serve/fs/workspaceFileSystem.ts Outdated
Comment thread packages/cli/src/serve/fs/workspaceFileSystem.ts
Comment thread packages/cli/src/serve/httpAcpBridge.test.ts
Comment thread packages/cli/src/serve/server.ts Outdated

@wenshao wenshao left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve — local real-world verification on PR head 88113340.

Local test results (fresh worktree, tmux, real fs)

Item Result
npm ci exit 0
npx vitest run packages/cli/src/serve/bridgeFileSystemAdapter.test.ts packages/cli/src/serve/fs/workspaceFileSystem.test.ts 81/81 pass (matches PR test plan)
npx vitest run packages/cli/src/serve/ 18 files, 754/754 pass (matches PR test plan)
npx vitest run in packages/acp-bridge/ 5 files, 62/62 pass (matches PR test plan)
npm run typecheck exit 0
npm run lint:ci (--max-warnings 0) exit 0
npm run bundle exit 0

Boot smoke (covers the test plan's unchecked smoke item, HTTP surface)

Booted qwen serve --port 47334 --require-auth against a tmpdir workspace and exercised the HTTP fs route that shares the same fsFactory as the new ACP adapter:

  • POST /file/write (new file) → on-disk mode -rw------- (0o600 contract holds)
  • GET /file → round-trip content matches
  • POST /file/write with path: "../escape.txt"path_outside_workspace (400)
  • POST /file/write against a symlink to /etc/passwdsymlink_escape (400)

The ACP-side writeTextOverwrite shares this same factory + WorkspaceFileSystem defenses; its 0o600-default, mode-preservation, and symlink-rejection invariants are covered by bridgeFileSystemAdapter.test.ts and workspaceFileSystem.test.ts with real-fs harnesses (mkdtemp + chmod + symlink), not mocks.

Note on the prior CHANGES_REQUESTED

My earlier CHANGES_REQUESTED on this same head cited only "no passing CI checks were found". That gate is now satisfied by the local run above — not a code finding. Treating this approval as superseding that request.

Note on serve log diagnostic

The daemon emits fs audit emit is the default no-op when runQwenServe is invoked by the CLI without an injected fsAuditEmit. This is pre-existing PR 18/19 behavior — the emit: line in this PR's diff is moved as-is (no semantic change). Not a regression introduced by this PR. The PR's "single audit stream" claim holds for embedders that wire deps.fsAuditEmit, because both the bridge adapter and the HTTP routes are handed the same fsFactory instance.

…tion)

All four threads from the wenshao review round on PR #4334 (Qwen
`/review`) — adopted as-suggested with the fixes outlined below.

**[Critical] writeTextOverwrite blocks on large/binary existing files**
(`workspaceFileSystem.ts:849` thread r3270664710)

`readExistingTextMeta(p)` reads the existing file just for encoding /
BOM / line-ending hints (best-effort meta). My earlier catch only
swallowed `ENOENT`, so `file_too_large` (>256 KiB) and `binary_file`
errors propagated and **blocked the overwrite entirely**. Pre-PR ACP
`BridgeClient.writeTextFile` never read the existing file at all —
an agent overwriting a 1 MiB log or binary config would have always
succeeded. Bubbling those classified errors regressed that.

Fix: catch ENOENT + file_too_large + binary_file; leave `existingMeta`
undefined and let `mergeWriteMeta` fall back to UTF-8/no-BOM/LF
defaults. New tests cover both scenarios.

Side fix uncovered while writing the tests: `created` was derived from
`existingMeta === undefined` which is wrong after this catch widening
— a binary or too-large existing file would now report `created: true`.
Replaced with an explicit `lstat` to detect target existence
independently of meta-read success.

**[Critical] writeTextAtomic({mode:'overwrite'}) is unsupported**
(`workspaceFileSystem.ts:146` thread r3270664723)

`WriteMode` was widened to include `'overwrite'` and
`validateWriteTextAtomicOptions` accepted it — but
`writeTextAtomic`'s `existingMeta` branch only reads meta for
`mode === 'replace'` AND `created: opts.mode === 'create'` is
hard-coded so `'overwrite'` always reports `created: false` even for
new files. Direct callers of `writeTextAtomic({mode: 'overwrite'})`
would silently lose CRLF on Windows files and misreport new-file
creation. The dedicated `writeTextOverwrite()` method handles both
correctly and is the only supported entry point for
unconditional-overwrite semantics.

Fix (option b from the reviewer): reject `'overwrite'` in
`validateWriteTextAtomicOptions` with a `parse_error` that names the
correct method. The `WriteMode` union still admits `'overwrite'`
internally (so `atomicWriteTextResolvedFile` + `assertAtomicTarget
Precondition`'s 'overwrite' branch compile), but no external caller
can reach those code paths via `writeTextAtomic`. The error message
points to `writeTextOverwrite()` so misuse surfaces an actionable hint.

**[Suggestion] killSession #4325 fix missing symmetric regression test**
(`httpAcpBridge.test.ts:6421` thread r3270664724)

The earlier #4325 fix touched both `closeSession` AND `killSession`
(both `const ci = channelInfo` → `const ci = channelInfoForEntry(entry)`)
but the smoke test only exercises closeSession. A future refactor
reverting `killSession` alone would pass all existing tests.

Fix: add a symmetric `killSession` smoke test mirroring the closeSession
shape — single-channel kill → assert handle.killed + sessionCount = 0.
Same overlap-race caveat documented inline. Future deterministic
overlap test still deferred to the same follow-up that adds factory
inspection seams.

**[Suggestion] createServeApp default `trusted: false` silently rejects
agent writes for embeds** (`server.ts:257` thread r3270664727)

`createServeApp` constructs its default `fsFactory` with
`trusted: false` (test-safe posture), and now wires it into the
bridge via `createBridgeFileSystemAdapter(fsFactory)`. Pre-PR ACP
`writeTextFile` went through the inline raw-fs proxy which had no
trust gate. Any embed using `createServeApp` without providing
`deps.fsFactory` or `deps.bridge` will now have ALL agent writes
silently reject with `untrusted_workspace`. `runQwenServe` consumers
are unaffected (defaults `trusted: true`), but IDE companions /
hosted daemons calling `createServeApp` directly are at risk.

Fix: emit a stderr startup warning when `deps.fsFactory` is not
provided, explicitly naming the asymmetry and the three opt-out
paths (provide fsFactory, provide bridge, or accept the gate). Visible
to operators so the trust-gate-default isn't an opaque "writes
silently fail" mystery in production.

Additional test gaps closed (sub-bullet from r3270664724):
- adapter-level `readText` trust-gate parity check — verifies that
  `trusted: false` does NOT extend to reads (PR 18's trust gate is
  write-only). A future refactor mistakenly gating reads would only
  fail HTTP-fs tests, not adapter ones.
- `writeTextOverwrite` non-regular-file rejection — pins the
  `parse_error` posture for directory targets so a relaxation in
  `assertAtomicTargetPrecondition`'s 'overwrite' branch is caught.

Verification
- `npx vitest run packages/cli/src/serve/` — 18 files, 760/760 pass
  (+6 new tests over the previous 754)
- `cd packages/acp-bridge && npx vitest run` — 5 files, 62/62 pass
- Pre-commit (`prettier --write` + `eslint --fix --max-warnings 0`)
  clean on all 5 staged files
@doudouOUC doudouOUC requested review from chiga0 and yiliang114 May 20, 2026 02:29

@wenshao wenshao left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical] mapDomainErrorToErrorKind (in packages/acp-bridge/src/status.ts) does not match FsError — it has .name = 'FsError', .kind, .status but no .code property. Every adapter-originated error (symlink_escape, untrusted_workspace, path_outside_workspace, file_too_large, etc.) returns undefined from the classifier, leaving structured monitoring without an errorKind. (Out of PR diff scope — flagging for awareness; consider adding an FsError branch in a follow-up.)

— qwen-latest-series-invite-beta-v34 via Qwen Code /review

Comment thread packages/cli/src/serve/bridgeFileSystemAdapter.ts
Comment thread packages/cli/src/serve/fs/workspaceFileSystem.ts
Comment thread packages/cli/src/serve/fs/workspaceFileSystem.ts
Comment thread packages/cli/src/serve/bridgeFileSystemAdapter.ts Outdated
Comment thread packages/cli/src/serve/server.ts Outdated
Adopts 4 of 7 wenshao review threads on PR #4334. The remaining 3
(1 Critical + 2 placeholder) are surfaced separately for user
judgment — the Critical's suggested fix doesn't work as-is and needs
a design call; the 2 placeholders look like reviewer-tool tests
("JSDoc test." / "test").

**[Critical] EACCES/EPERM blocks overwrite** (r3270921396, ws.ts:877)

The earlier r3270664710 fix widened the meta-read catch to swallow
ENOENT + file_too_large + binary_file. wenshao caught that EACCES /
EPERM also need to be swallowed — a file the daemon can't read
(0o000, other-user-owned) would abort the overwrite, contradicting
the "best-effort meta read" comment. Also opens an agent-side probe:
an attacker could detect file readability by observing EACCES on
overwrite attempts.

Fix: extend the catch to also swallow EACCES + EPERM. Comment block
expanded to spell out the full set (ENOENT / EACCES / EPERM /
file_too_large / binary_file) and the probing-defense rationale.

Test: `writeTextOverwrite succeeds over an existing 0o000 (unreadable)
file` — pins the posture so a regression here is caught. Skipped on
Windows + when running as root (root bypasses POSIX mode bits).

**[Suggestion] Negative `limit` produces wrong content**
(r3270921401, bridgeFileSystemAdapter.ts:112)

Pre-PR the inline `BridgeClient.readTextFile` returned `{ content:
'' }` for `limit <= 0`. PR 18's `readText` applies `slice(0, limit)`,
which for `limit: -1` returns "all lines except the last" — wrong
content. Same hazard for non-positive `line` (PR 18 rejects with
`parse_error` for `line < 1`, smuggling a 4xx-shaped error to agents
that previously got `''`).

Fix: tighten the adapter's `typeof === 'number'` guard to also
require `> 0`. Comment expanded to call out the divergence and why
"drop and let PR 18 default to no-windowing" is the closest match to
pre-PR empty-content posture without leaking parse_error.

Tests: `drops non-positive limit (negative / zero) instead of
forwarding` + `drops non-positive line (zero) instead of forwarding
parse_error`.

**[Suggestion] Warning fires when deps.bridge is provided**
(r3270921402, server.ts:266)

Earlier r3270664727 fix added a startup stderr warning when
`deps.fsFactory` is not provided. wenshao caught that the warning
also fires when `deps.bridge` IS provided — but in that case the
embed owns its own fileSystem wiring (the default adapter never
runs), so the warning's claim about ACP writes rejecting is false.

Fix: narrow guard to `!deps.fsFactory && !deps.bridge`. Comment
expanded to explain why bridge-injection suppresses the warning.

**[Suggestion] No oversized-payload test for writeTextOverwrite**
(r3270921399, ws.ts:835)

`writeTextOverwrite` calls `enforceWriteSize(decodedSizeBytes)`
mirroring `writeText`'s 5 MiB cap, but the existing oversized-write
test only exercises `writeText`. A regression dropping the check on
the new method would let agents (the primary consumer) write
arbitrarily large files undetected.

Test: `writeTextOverwrite rejects content exceeding MAX_WRITE_BYTES
with file_too_large`.

Verification
- `npx vitest run packages/cli/src/serve/` — 18 files, 764/764 pass
  (+4 new tests over the previous 760)
- Pre-commit (`prettier --write` + `eslint --fix --max-warnings 0`)
  clean on all 5 staged files
Comment thread packages/cli/src/serve/server.ts
Comment thread packages/cli/src/serve/fs/workspaceFileSystem.ts
Comment thread packages/cli/src/serve/bridgeFileSystemAdapter.ts
Comment thread packages/cli/src/serve/bridgeFileSystemAdapter.ts Outdated
Comment thread packages/cli/src/serve/httpAcpBridge.test.ts
Comment thread packages/cli/src/serve/fs/workspaceFileSystem.ts
Comment thread packages/cli/src/serve/bridgeFileSystemAdapter.test.ts Outdated
Comment thread packages/cli/src/serve/server.ts Outdated
Comment thread packages/acp-bridge/src/bridge.ts
… + 3 Suggestion)

Adopts 4 of 5 new threads from the DeepSeek-v4-pro review round on
PR #4334 (Qwen `/review`). The 5th (DWcK8) is a duplicate of a test
already in commit 9f73b83 — declined separately with a pointer.

**[Critical] Trust-default asymmetry between runQwenServe ↔ createServeApp**
(r3270978579, server.ts DWcK4)

`runQwenServe.ts` defaults `trusted: true` (production daemon),
`server.ts` defaults `trusted: false` (test-safe). The asymmetry is
intentional but lives in two places — a future maintainer can break
the alignment without any compile-time signal. The earlier stderr
warning (commit e185409) covers the embed-omits-fsFactory case but
NOT a regression in the runQwenServe → createServeApp pass-through.

Fix: extract `resolveBridgeFsFactory(input)` helper in `server.ts`
(exported alongside `createDefaultFsAuditEmit`). Both call sites use
it. Trust stays a REQUIRED parameter — the policy difference is
preserved at the call sites, but the construction shape (build vs
inject + audit-emit default) is centralized. Defense-in-depth, not
behavior change.

**[Suggestion] adapter JSDoc claim about `mapDomainErrorToErrorKind`
is misleading** (r3270978595, DWcLB)

The docstring at `bridgeFileSystemAdapter.ts:38` says "the bridge's
existing `mapDomainErrorToErrorKind` classifier downstream picks up
`FsError` codes". This is false: `mapDomainErrorToErrorKind` in
`acp-bridge/src/status.ts` checks `instanceof` / `.name` / `.code`
(Node errno names), but has NO branch reading `err.kind` (FsError's
discriminator: `untrusted_workspace` / `symlink_escape` / etc.).
Errors still propagate (the `.kind` field rides through on the
thrown FsError object itself), but a future maintainer debugging
error classification during an incident would chase the wrong code
path.

Fix: rewrite the docstring to describe the actual flow — `FsError`
is thrown unchanged through BridgeClient's ACP handlers; downstream
consumers reading the ACP error payload key on `.kind` directly. The
HTTP `sendFsError` serializes the same `.kind`, so SDK consumers see
the same shape from either surface. Adding a real `instanceof
FsError` branch to `mapDomainErrorToErrorKind` would need cross-
package imports (FsError lives in `cli/src/serve/fs`, classifier in
`acp-bridge`) — explicitly deferred to a separate PR.

**[Suggestion] adapter readText error propagation untested**
(r3270978593, DWcK_)

Read-side errors from `wfs.readText` (`file_too_large`,
`binary_file`, `symlink_escape`) propagate untested through the
adapter — the existing tests cover trust-gate (already write-only),
line/limit forwarding, null/non-positive guards, and boundary, but
not the `FsError` classes themselves. A regression silently
swallowing or wrapping them would only fail HTTP-fs tests.

Fix: add 3 adapter tests pinning `file_too_large` / `binary_file` /
`symlink_escape` propagation surface as-is via the adapter's
re-thrown error.

**[Suggestion] channelInfoForEntry HAZARD comments on bridge.ts fix
sites** (r3270978598, DWcLD)

The regression test for the `#4325` fix
(`httpAcpBridge.test.ts:6421`) is single-channel smoke only — its
own comment acknowledges "a reverted fix that captured `channelInfo`
after the entry was gone from `byId` would also pass this assertion".
The actual overlap-race state isn't deterministically constructable
without factory-internal hooks. Until the deterministic test lands,
the only defense against accidental revert is code-review visibility.

Fix: add `HAZARD(#4325)` comments at both `closeSession` and
`killSession` fix sites in `acp-bridge/src/bridge.ts`, explicitly
flagging that the existing smoke test would not catch a revert and
that the `channelInfoForEntry(entry)` call must NOT be refactored
away without first landing the deterministic overlap test.

Verification
- `npx vitest run packages/cli/src/serve/` — 18 files, 767/767 pass
  (+3 new adapter tests; the prior 760→767 includes runs of multi-
   tick fold-ins on the same branch).
- `cd packages/acp-bridge && npx vitest run` — 5 files, 62/62 pass
- Pre-commit (`prettier --write` + `eslint --fix --max-warnings 0`)
  clean on all 5 staged files

@wenshao wenshao left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No high-confidence issues found. The previous review's 4 findings (EACCES/EPERM swallow, writeTextOverwrite size cap test, non-positive line/limit filtering, stderr warning suppression for deps.bridge) were all correctly addressed in commit e185409. Build passes, all 266 tests green. LGTM! ✅ — qwen-latest-series-invite-beta-v34 via Qwen Code /review

Adopts the second round of DeepSeek-v4-pro suggestions on PR #4334.
All 4 are small, targeted improvements without controversy.

**DWrbe — WriteMode admits 'overwrite' at compile time** (r3271063030)

`WriteTextAtomicOptions.mode` was typed as `WriteMode` (which
includes 'overwrite'), but `validateWriteTextAtomicOptions` throws
`parse_error` for that value. The runtime error catches misuse but
TypeScript happily lets the call through.

Fix: introduce `AtomicWriteMode = Exclude<WriteMode, 'overwrite'>`
public type and narrow `WriteTextAtomicOptions.mode` to it. Runtime
validator stays as defense-in-depth.

**DWrbl — boundary tests use bare .rejects.toThrow()** (r3271063040)

Both boundary-enforcement adapter tests asserted "throws" without
pinning the FsError kind. Incidental OS errors (CI container EACCES
on /etc/passwd) or future pre-check additions could pass these
tests trivially while masking that boundary enforcement isn't
firing.

Fix: assert `.kind === 'path_outside_workspace'` for both sides.

**DWrbn — trust warning floods stderr in tests** (r3271063045)

The startup warning fires on every createServeApp call. server.test.ts
calls createServeApp ~25 times, masking genuine failures.

Fix: module-scoped once-per-process guard `warnedDefaultTrust`. Module
scope (not per-app closure) because the warning is a posture statement
about this binary, not per-instance.

**DWrbr — channelInfoForEntry undefined is silent** (r3271063052)

closeSession / killSession's cleanup branches short-circuit silently
when channelInfoForEntry returns undefined (entry's channel torn
down). The "closing session" log fires but the skipped-cleanup fact
is invisible, making zombie-channel debugging harder.

Fix: emit stderr diagnostic naming session id + which method
short-circuited + likely cause. Sibling methods like
requestSessionStatus throw SessionNotFoundError; close/kill are
idempotent so we log instead.

Verification: serve 767/767, acp-bridge 62/62, pre-commit clean.

@wenshao wenshao left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found. LGTM! ✅ — qwen-latest-series-invite-beta-v34 via Qwen Code /review

@doudouOUC doudouOUC merged commit dfa8ca4 into daemon_mode_b_main May 20, 2026
10 checks passed
@doudouOUC doudouOUC deleted the feat/f1-followup-fs-adapter-test-split-channelinfo branch May 20, 2026 05:10
doudouOUC added a commit that referenced this pull request May 20, 2026
Mixed batch: bridge-test backfill from wenshao's APPROVED review
plus 4 DeepSeek/v4-pro suggestions and the 3 typecheck/test
blockers DeepSeek named in CHANGES_REQUESTED #4325674833.

**Pre-merge blockers (DeepSeek #4325674833 body)**

- `server.test.ts:529` `FakeBridge` — added the F3-required
  `permissionPolicy: 'first-responder' as const`. Tests don't
  exercise mediation; the literal pins the pre-F3 default so
  existing assertions stay shape-compatible.

- `server.test.ts:3994` `WorkspaceFileSystemFactory.forRequest()`
  mock — added the missing `writeTextOverwrite` method that PR
  #4334 introduced on `WorkspaceFileSystem` after this branch
  forked.

- 4 vote-context test failures from `fromLoopback` plumbing —
  updated the four `expect(...).toEqual(...)` assertions in
  `POST /session/:id/permission/:requestId` and
  `POST /permission/:requestId` to include `fromLoopback: true`
  on the captured context. The supertest peer is `127.0.0.1`,
  so `detectFromLoopback(req)` correctly stamps the field; the
  pre-F3 expected shape was stale.

**Inline suggestions adopted**

- **3271420267** (wenshao APPROVED, security-critical) — added
  bridge-level test `rejects cancel sentinel injection via
  {selected,'__cancelled__'}` in `httpAcpBridge.test.ts`. Without
  it, a future refactor could silently remove the wire-injection
  guard that closes the policy-bypass attack surface introduced
  in Round 5 (#3271185588). Required `npm run build
  --workspace=packages/acp-bridge` to refresh `dist/` before
  vitest picked up the F3 bridge.ts changes; documented for
  future contributors editing F3 acp-bridge code.

- **3271627444** (DeepSeek) — `request()` JSDoc rewritten to
  drop "Promise contract — never rejects" without qualification.
  The `CancelSentinelCollisionError` synchronous throw is real
  and intentional (a never-settling Promise alongside a thrown
  error is worse than fail-fast), but callers must be aware of
  it. Updated the contract doc to call out the sync-throw
  exception explicitly and documented that async callers get
  the throw via their own Promise machinery.

- **3271627446** (DeepSeek) — fixed "Bounded LRU" comment on
  `MAX_RESOLVED_PERMISSION_RECORDS` to "Bounded FIFO" since
  `rememberResolved` uses `resolvedOrder.shift()` (drop oldest).
  Mirrors the parallel `PermissionAuditRing` correction in
  commit b0242dd.

- **3271627457** (DeepSeek) — added stderr breadcrumbs to all 3
  forbidden-vote sites (voteDesignated / voteConsensus /
  voteLocalOnly). Audit ring is in-memory only (no v1 query
  route), SSE events are transient — operators tailing daemon
  stderr previously had zero indication of permission rejections.
  New `writeForbiddenStderr` helper centralizes the formatting
  + try/catch defensive posture (mirrors the timeout breadcrumb
  pattern from Round 4).

- **3271627459** (DeepSeek) — added a `TODO(forward-compat)`
  comment at `voteConsensus`'s rejection site documenting the
  `designated_mismatch` reason-code overload. The same wire
  string covers two distinct semantic cases: "voter is not the
  prompt originator" (designated policy) and "voter not in
  consensus votersAtIssue snapshot" (consensus). Splitting them
  into distinct codes is deferred to a future PR once an SDK
  consumer needs to disambiguate.

🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
doudouOUC added a commit that referenced this pull request May 20, 2026
…4335)

* feat(acp-bridge): F3 — multi-client permission coordination (#4175) [rebased onto F1]

Squashed F3 implementation rebased from origin/main onto
daemon_mode_b_main (post-F1 #4319). F1 lifted the bridge core to
@qwen-code/acp-bridge package; F3's edits to the pre-F1
httpAcpBridge.ts BridgeClient class + factory were ported to the
new file locations:

  - BridgeClient.requestPermission rewrite → bridgeClient.ts
  - Factory mediator construction / pendingPermissions deletion /
    cancelPendingForSession refactor / respondTo*Permission
    rewrites / pendingPermissionCount + permissionPolicy getters /
    teardown sites (closeSession, killSession, shutdown drain)
    → bridge.ts
  - Error class re-exports → cli/src/serve/httpAcpBridge.ts shim
    (added CancelSentinelCollisionError, PermissionForbiddenError,
    PermissionPolicyNotImplementedError to the F1 re-export block)

This commit folds 13 logical F3 commits + 4 review fold-ins (Copilot
inline comments + 3 final-pass agent reviews) into a single
post-rebase squash. The full review trail is in
.claude/plans/fluttering-coalescing-kettle*.md (worktree-local).

Strategies (4): first-responder (default, byte-for-byte preserved),
designated, consensus (default N=floor(M/2)+1), local-only.

New SSE events: permission_partial_vote, permission_forbidden.
Capability tag: permission_mediation (always-on with build-supported
modes list); active policy at /capabilities.policy.permission.

Settings: policy.permissionStrategy enum + policy.consensusQuorum
number, both requiresRestart: true (F3 v1 reads at boot).

3 new typed errors: PermissionForbiddenError → 403,
PermissionPolicyNotImplementedError → 501 (forward-compat for future
policy literals), CancelSentinelCollisionError → 500 (agent / daemon
contract violation).

Hardness invariants: N1 synchronous-register, N2 cleanup ordering,
N3 originatorClientId stamping, O5 cancel sentinel pre-publish
collision check, O8 pre-F3 permission_resolved wire shape preserved.

Tests: 35 mediator unit + 10 audit ring + 56 SDK reducer + 6
bridgeClient + 3 bridge integration. Pre-existing
httpAcpBridge.test.ts cross-session-vote suite passes byte-for-byte.

Issue: #4175 (F3)

* fix(f3): build/capability fixes from Copilot review (#4335)

- packages/sdk-typescript/src/daemon/index.ts: re-export the four F3
  permission event types (`DaemonPermissionForbiddenData/Event`,
  `DaemonPermissionPartialVoteData/Event`) so the public package barrel
  at `src/index.ts` (which forwards them via `from './daemon/index.js'`)
  resolves at build time. Without this fix `npm run build
  --workspace=packages/sdk-typescript` failed with TS2305/TS2724;
  vitest passed only because it resolves TS source via tsx and
  bypasses tsc compilation. Reported in PR #4335 review comments
  3270615836 / 3270622302 (wenshao via Qwen Code /review).

- packages/cli/src/serve/server.test.ts: append `'permission_mediation'`
  to `EXPECTED_STAGE1_FEATURES` and adjust `EXPECTED_REGISTERED_FEATURES`
  reordering so the test fixture matches the registry's actual order
  (`...workspace_mcp_restart, require_auth, auth_device_flow,
  permission_mediation`). Without this fix four `serve capability
  registry` tests asserted via `.toEqual` against a stale list.

- docs/developers/qwen-serve-protocol.md: swap `permission_mediation`
  and `auth_device_flow` in the documented capability list so the order
  mirrors `SERVE_CAPABILITY_REGISTRY` declaration order.

- packages/vscode-ide-companion/schemas/settings.schema.json: regenerate
  the IDE-companion JSON schema with the new `policy` section (was
  pending from Commit 5 of the F3 series; checked in here so the
  IDE companion sees the same `permissionStrategy` / `consensusQuorum`
  shape that the CLI accepts).

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

* fix(f3): wire production audit ring + restore timeout stderr (#4335)

Wenshao review #4335 surfaced two related Critical findings:

1. **Audit publisher silently no-op in production** (3270622298). The
   `bridgeOptions.ts:305` JSDoc claimed "the bridge allocates an
   internal `PermissionAuditRing`" but the actual fallback at
   `bridge.ts:543` is `createNoOpPermissionAuditPublisher()`, and
   `runQwenServe.ts` never wired one. All 5 audit record types
   (`requested`, `voted`, `forbidden`, `resolved`, `timeout`) were
   silently discarded — the forensic audit trail the F3 plan
   committed to ("ring 留给后续 PR 加查询接口") never existed in
   any deployed daemon.

2. **Timeout breadcrumb lost** (3270622304). Pre-F3 wrote
   `"timed out after Xms"` to daemon stderr on every permission
   timeout. F3 removed that direct write and delegated to
   `audit.recordTimeout()`, but the audit publisher is the no-op
   fallback in production (see #1). Operators tailing daemon
   stderr could no longer observe permission timeouts.

Fixes:

- `runQwenServe.ts` allocates a `PermissionAuditRing` (default cap 512)
  + `createPermissionAuditPublisher` and passes the publisher via
  `BridgeOptions.permissionAudit`. The ring is held in the daemon
  host's closure for the lifetime of the daemon — a future
  `GET /workspace/permission/audit` route (out of F3 v1 scope) can
  lift it out for query without further bridge changes.

- `permissionMediator.ts` writes the stderr breadcrumb directly from
  the timer callback, before forwarding to the (potentially no-op)
  audit publisher. Wrapped in try/catch because `process.stderr.write`
  can synchronously throw on EPIPE — losing observability is
  preferable to crashing the timer queue.

- `bridgeOptions.ts` JSDoc rewritten to match reality: the bridge
  falls back to a no-op publisher; production wiring lives in
  `runQwenServe.ts`; the stderr breadcrumb is in the mediator
  (independent of the publisher).

- New unit test `writes a stderr breadcrumb when the timer fires`
  spies on `process.stderr.write` and asserts the breadcrumb format
  contains the requestId, sessionId, and the timeout duration so
  future refactors can't silently drop the line again.

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

* fix(f3): drop dead helper + propagate originator to F3 view state (#4335)

Two small follow-ups from wenshao review #4335:

- **`bridge.ts:672-682` — dead `_resolutionToAcpResponse` helper**
  (3270622309). Defined and immediately suppressed with `void`. The
  identical `resolutionToAcpResponse` lives at `bridgeClient.ts:41`
  and is the one actually used by `BridgeClient.requestPermission`
  — the bridge-factory copy was a stranded leftover from the lift
  out of inline closures into the mediator pattern. Removed
  declaration, `void` statement, and the now-unused
  `RequestPermissionResponse` (`@agentclientprotocol/sdk`) and
  `PermissionResolution` (`./permission.js`) imports.

- **SDK reducer `mergeOriginator` for F3 events** (3270622311). The
  mediator stamps `originatorClientId` (= prompt originator per
  N3) on the `permission_partial_vote` / `permission_forbidden`
  envelope, but the reducer cases used `next.push({ ...event.data })`
  which only copies `data` fields. SDK consumers reading
  `permissionVoteProgress[reqId]` / `forbiddenVotes[i]` could not
  determine which client's prompt was targeted by the partial-vote
  progress / forbidden vote — same gap PR #4282 fixed for
  approval-mode / tool-toggle / workspace-init / mcp-restart.

  Applied the existing `mergeOriginator` helper to both reducer
  cases. Added `originatorClientId?: string` to both Data
  interfaces with JSDoc explaining the propagation contract
  (preserve any pre-existing `data.originatorClientId`; otherwise
  stamp from the envelope; for forbidden votes the field is
  distinct from `data.clientId` which carries the rejected voter).

  Three new reducer tests:
  1. `permission_partial_vote` propagates envelope originator into
     `permissionVoteProgress`.
  2. `permission_forbidden` propagates envelope originator into
     `forbiddenVotes`, distinct from `data.clientId`.
  3. `mergeOriginator` preserves any pre-existing
     `data.originatorClientId` over the envelope value.

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

* fix(f3): wenshao Round 4 — defensive stderr, audit accuracy, orphan cleanup (#4335)

Four findings from wenshao review #4324937255 — the Critical one
masked an actual hang scenario; the other three are observability /
correctness fixes that round out F3 v1.

**[Critical] safeEmit / safeAudit stderr breadcrumb wraps** (3271041461).
Both helpers wrote `process.stderr.write` inside their `catch` block
WITHOUT a nested `try/catch`. If stderr itself synchronously throws
(EPIPE during daemon shutdown), the exception escapes the "safe"
wrapper. In `resolveEntry`'s cleanup ladder
(`safeEmit → rememberResolved → safeAudit → pending.resolve`), an
escaping safeEmit exception aborts before `pending.resolve(resolution)`
runs — the request was already deleted from `this.pending` (no
double-resolve guard), so the agent's awaiting Promise never
settles. `requestPermission` hangs until the timeout fires. The
timer callback already wraps its breadcrumb in `try/catch` for the
same reason — applied the matching pattern to safeEmit + safeAudit.

**[Suggestion] Idempotent re-vote audit shows attempted optionId,
not the original** (3271041464). When `client_A` originally voted
for `proceed_once` and later attempts `proceed_always`, the tally
silently keeps `proceed_once` (idempotent) but the audit ring
recorded `optionId: proceed_always`. An operator reading the ring
would see a vote for proceed_always that never counted toward
quorum. Look up the originally-voted option from the tally and
substitute it into the audit record. Added regression test
asserting the audit reflects tally state.

**[Suggestion] SDK reducer leaks `permissionVoteProgress` on
mid-permission reconnect** (3271041465). When an SDK client
reconnects and misses `permission_request`, then receives
`permission_partial_vote` (stored in `permissionVoteProgress`),
then receives `permission_resolved` — the early-return path on
unmatched `requestId` did NOT clear `permissionVoteProgress`. The
orphan progress entry persisted until session end. Both
`permission_resolved` and `permission_already_resolved` reducer
cases now unconditionally clear any orphan entry on the unmatched
path. Two new reducer tests cover the recovery contract; the
misleading "the next `permission_resolved` will clear both"
comment on `permission_partial_vote` is corrected.

**[Suggestion] Document votersAtIssue snapshot timing window**
(3271041469). The snapshot fires synchronously after
`entry.events.publish`, with no event-loop yield between, so a NEW
HTTP client cannot register between publish and snapshot. But an
SSE-only subscriber (no `X-Qwen-Client-Id` registered yet) that
connected BEFORE publish is invisible to the snapshot — `consensus`
silently rejects its later vote as `forbidden`. Documented the
window in `votersForSession` JSDoc; future PRs surfacing
`eligibleVoters[]` on `permission_request.data` should source it
from the same snapshot for consistency. No code change — the
narrow window is acceptable for F3 v1, and the structural fix
(snapshot at publish time) requires bridge-level refactor.

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

* fix(f3): wenshao Round 5 — sentinel injection guard, observability, /8 loopback (#4335)

Four findings from wenshao review #4325130053. The Critical one is
a real security gap; the others are observability + correctness
hardening.

**[Critical] Cancel sentinel injection bypass** (3271185588). The
mediator's `vote()` recognizes `CANCEL_VOTE_SENTINEL` BEFORE
validating the option against `allowedOptionIds`, so a wire client
sending `{outcome:'selected', optionId:'__cancelled__'}` would
short-circuit ALL policy dispatch (designated originator check,
consensus quorum, local-only loopback gate). The mediator's JSDoc
documented the precondition ("callers MUST NOT forward an
incoming vote.optionId === CANCEL_VOTE_SENTINEL from a wire
client") but the precondition was never enforced — the bridge's
`respondToSessionPermission` mapped the wire optionId straight
through. Added an explicit `InvalidPermissionOptionError` throw
when the wire payload is `{selected, CANCEL_VOTE_SENTINEL}`. The
collision-defense at request issue time
(`CancelSentinelCollisionError`) already prevents agents from
advertising the sentinel as a legitimate option; this closes the
remaining vector.

**[Suggestion] Silent quorum cap + M=0 hang observability**
(3271185594). Two related diagnostic gaps in the consensus
policy:
- When `policy.consensusQuorum` exceeds `votersAtIssue.size`, the
  cap fires silently. Operators investigating "why did consensus
  resolve at N=2 when I configured 5?" had no breadcrumb.
- When `policy === 'consensus'` and `votersAtIssue.size === 0`,
  every vote rejects as `forbidden: designated_mismatch` because
  the empty snapshot can never match any voter clientId. The
  request hangs until `permissionTimeoutMs` with no
  diagnostic signal.

Added stderr breadcrumbs at both points: cap-applied (once per
request via a `consensusQuorumCapNoted` flag on `MediatorPending`)
and at issue time when consensus M=0. No semantic change — the
cap and the timeout-only resolution behavior are intentional per
the F3 plan; the breadcrumbs just make them debuggable.

**[Suggestion] detectFromLoopback misses 127.0.0.0/8** (3271185597).
Per RFC 1122 the entire `127.0.0.0/8` block is loopback. The
exact-match Set of three literals (`127.0.0.1`, `::1`,
`::ffff:127.0.0.1`) silently fail-CLOSED on legitimate
`127.0.0.2` / `127.0.1.1` / `::ffff:127.0.0.2` peers, causing
unexpected `remote_not_allowed` rejections under `local-only`
policy. Switched to a prefix test so the entire `/8` and its
dual-stack mirror are accepted. Direction stays fail-CLOSED for
unrecognized address shapes.

**[Suggestion] VSCode JSON schema integer/min validation**
(3271185604). `runQwenServe.ts` validates
`Number.isInteger(consensusQuorum) && >= 1`, but the generated
`settings.schema.json` declared `"type": "number"` so VSCode's
inline JSON Schema validation accepted `0` / `-1` / `1.5` and
the user only learned the value was invalid on the next daemon
restart. Added `jsonSchemaOverride: {type:'integer', minimum:1}`
to the `consensusQuorum` settings entry and regenerated the
schema. IDE editors now flag invalid values immediately.

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

* fix(f3): Round 6 — wenshao APPROVED + DeepSeek follow-ups (#4335)

Mixed batch: bridge-test backfill from wenshao's APPROVED review
plus 4 DeepSeek/v4-pro suggestions and the 3 typecheck/test
blockers DeepSeek named in CHANGES_REQUESTED #4325674833.

**Pre-merge blockers (DeepSeek #4325674833 body)**

- `server.test.ts:529` `FakeBridge` — added the F3-required
  `permissionPolicy: 'first-responder' as const`. Tests don't
  exercise mediation; the literal pins the pre-F3 default so
  existing assertions stay shape-compatible.

- `server.test.ts:3994` `WorkspaceFileSystemFactory.forRequest()`
  mock — added the missing `writeTextOverwrite` method that PR
  #4334 introduced on `WorkspaceFileSystem` after this branch
  forked.

- 4 vote-context test failures from `fromLoopback` plumbing —
  updated the four `expect(...).toEqual(...)` assertions in
  `POST /session/:id/permission/:requestId` and
  `POST /permission/:requestId` to include `fromLoopback: true`
  on the captured context. The supertest peer is `127.0.0.1`,
  so `detectFromLoopback(req)` correctly stamps the field; the
  pre-F3 expected shape was stale.

**Inline suggestions adopted**

- **3271420267** (wenshao APPROVED, security-critical) — added
  bridge-level test `rejects cancel sentinel injection via
  {selected,'__cancelled__'}` in `httpAcpBridge.test.ts`. Without
  it, a future refactor could silently remove the wire-injection
  guard that closes the policy-bypass attack surface introduced
  in Round 5 (#3271185588). Required `npm run build
  --workspace=packages/acp-bridge` to refresh `dist/` before
  vitest picked up the F3 bridge.ts changes; documented for
  future contributors editing F3 acp-bridge code.

- **3271627444** (DeepSeek) — `request()` JSDoc rewritten to
  drop "Promise contract — never rejects" without qualification.
  The `CancelSentinelCollisionError` synchronous throw is real
  and intentional (a never-settling Promise alongside a thrown
  error is worse than fail-fast), but callers must be aware of
  it. Updated the contract doc to call out the sync-throw
  exception explicitly and documented that async callers get
  the throw via their own Promise machinery.

- **3271627446** (DeepSeek) — fixed "Bounded LRU" comment on
  `MAX_RESOLVED_PERMISSION_RECORDS` to "Bounded FIFO" since
  `rememberResolved` uses `resolvedOrder.shift()` (drop oldest).
  Mirrors the parallel `PermissionAuditRing` correction in
  commit b0242dd.

- **3271627457** (DeepSeek) — added stderr breadcrumbs to all 3
  forbidden-vote sites (voteDesignated / voteConsensus /
  voteLocalOnly). Audit ring is in-memory only (no v1 query
  route), SSE events are transient — operators tailing daemon
  stderr previously had zero indication of permission rejections.
  New `writeForbiddenStderr` helper centralizes the formatting
  + try/catch defensive posture (mirrors the timeout breadcrumb
  pattern from Round 4).

- **3271627459** (DeepSeek) — added a `TODO(forward-compat)`
  comment at `voteConsensus`'s rejection site documenting the
  `designated_mismatch` reason-code overload. The same wire
  string covers two distinct semantic cases: "voter is not the
  prompt originator" (designated policy) and "voter not in
  consensus votersAtIssue snapshot" (consensus). Splitting them
  into distinct codes is deferred to a future PR once an SDK
  consumer needs to disambiguate.

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

* fix(f3): Round 7 — error precedence + 7 hardening fixes from wenshao (#4335)

8 findings from wenshao Round 7. The Critical one closes a session-
existence information leak; 6 Suggestions improve observability,
type safety, and test coverage; 1 documents the cancel-sentinel
escape hatch in the local-only setting description.

**[Critical] Error precedence regression in respondToSessionPermission**
(3271978329). When `peekSessionFor(requestId)` returned `undefined`
(timed out / LRU-evicted / never registered), the cross-session
guard at line 2033 didn't fire (`!== undefined` skips it), so
execution fell through to `resolveTrustedClientId` which throws
`InvalidClientIdError` (HTTP 400) when the caller's clientId
isn't registered. Pre-F3 returned `false` (HTTP 404) for unknown
requestIds regardless of clientId validity. Without the explicit
guard, a probe with a fabricated clientId could distinguish
"session exists with these registered clients" (400) from "no
such request" (404). Added an explicit `actualSessionId ===
undefined → return false` short-circuit BEFORE the clientId
validation. The defensive `unknown_request` switch case below
becomes unreachable in practice; left in place for defense-in-depth.

**[Suggestion] Cancel sentinel cross-policy escape hatch under
`local-only`** (3271978336). Documented in `voteLocalOnly` JSDoc
and the settings description that a remote voter can ABORT a
pending permission via `{outcome:'cancelled'}` even though they
cannot RESOLVE one. The F3 plan calls this out as intentional
(cross-policy cancel for consistency with first-responder /
designated / consensus); operators wanting strict-cancel-too need
a dedicated loopback-bound daemon. Doc-only — semantic change
deferred.

**[Suggestion] CapabilitiesEnvelope.policy.permission widens
silently** (3271978342). Replaced the inlined string-literal
union with `import type { PermissionPolicy } from
'@qwen-code/acp-bridge'`. Adding a 5th policy upstream would now
trigger a compile error here instead of silently accepting the
narrower set.

**[Suggestion] M=2 unanimity surprise** (3271978356). Default
quorum `floor(M/2)+1` requires unanimity for even M (M=2 →
quorum=2; both voters must agree). An operator picking
`consensus` with two clients expecting "majority of 2 = 1" gets
unanimity instead — a split vote silently hangs until
`permissionTimeoutMs`. Added stderr breadcrumb at issue time
when the default formula yields unanimity (M ≥ 2 and floor(M/2)+1
== M). Mirrors the existing M=0 / cap-applied breadcrumbs added
in Round 5. Formula stays unchanged (true majority for all M is
mutually exclusive with M=1 → quorum=1). Description in the
settings schema also calls out the M=2 case explicitly.

**[Suggestion] Cancel sentinel adversarial test gap**
(3271978359). The existing "resolves cancelled regardless of
policy" test used the originator under designated and a
votersAtIssue voter under consensus — those would be ACCEPTED by
the policies even without the sentinel bypass. Added two
adversarial tests that pin the cross-policy escape hatch:
non-originator voter under designated and not-in-snapshot voter
under consensus.

**[Suggestion] BridgeClient pre-publish collision test gap**
(3271978365). `bridgeClient.requestPermission` throws
`CancelSentinelCollisionError` BEFORE publishing the SSE
`permission_request` to prevent orphan events (the mediator-level
collision check in `mediator.request` happens too late if publish
goes first). Added test asserting the throw + asserting publish
was NOT called + asserting `pendingPermissionIds` was NOT
incremented.

**[Suggestion] Settings descriptions missing security caveats**
(3271978370). Added explicit caveats to `permissionStrategy`
description: (a) `designated` notes that client identity is
self-declared with no proof-of-possession (impersonation by
observing originatorClientId on SSE frames is possible); (b)
`local-only` notes the cancel-sentinel cross-policy escape hatch.
Schema regenerated to `vscode-ide-companion/schemas/settings.schema.json`.

**[Suggestion] Boot validation error class** (3271978374). Replaced
`err.message.includes('invalid policy.')` substring matching with
a dedicated `InvalidPolicyConfigError` class checked via
`instanceof`. A future reworded validation message would have
silently downgraded operator misconfiguration to "fall back to
defaults" under the previous fragile match.

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

* fix(f3): Round 8 — close legacy clientId oracle + 5 hardening fixes (#4335)

6 follow-up findings from wenshao Round 8 review #4326742064 (state:
COMMENTED — not blocking but addresses leftover risk surfaces).

**[Suggestion] Legacy `respondToPermission` info leak** (3272493777).
Round 7 closed the cross-session client-registration oracle on the
session-scoped vote route, but the legacy workspace-level route
(`POST /permission/<requestId>`) still called
`resolveAnyTrustedClientId` on unknown-requestId paths, throwing
`InvalidClientIdError` (400) for unregistered clientIds and
returning false (404) for registered ones — the same oracle. The
PR #4231 reasoning ("preserve security boundary") was inverted:
the 400-vs-404 distinction WAS the leak. Removed the call,
deleted the now-unused `resolveAnyTrustedClientId` helper, and
updated the previously-leak-asserting test (`rejects unknown
permission votes with unregistered client ids`) to assert the
new uniform `false` behavior across all 3 input shapes
(unregistered / registered / no-clientId).

**[Suggestion] Error-precedence regression test gap +
observability inconsistency** (3272493792). Two parts:
- Added regression test `returns false (not InvalidClientIdError)
  when session exists but requestId is unknown and clientId is
  unregistered` to lock the Round-7 fix against future refactors.
- Promoted the error-precedence guard's stderr line from
  debug-gated `writeServeDebugLine` to unconditional
  `writeStderrLine`, matching the `writeForbiddenStderr` posture
  in the mediator. Operators tailing stderr at 3 AM no longer
  need `QWEN_SERVE_DEBUG=1` to see unexpected 404s on the
  permission endpoint.

**[Suggestion] Settings description "UNANIMITY for even M" was
factually wrong** (3272493795). `floor(M/2)+1` equals M only when
M=2; for M=4 it gives 3 (supermajority), M=6 gives 4 (~67%).
The mediator's own unanimity warning correctly fires only when
M=2. Settings description now reads "UNANIMITY for M=2 (quorum=2,
both must agree) and supermajority for larger even M (M=4 →
quorum=3; M=6 → quorum=4)". VSCode JSON schema regenerated.

**[Suggestion] runQwenServe.ts inline policy unions** (3272493805).
Same drift-protection rationale as the types.ts fix in Round 7.
Imported `PermissionPolicy` from `@qwen-code/acp-bridge`,
replaced 3 inline unions: the `let` declaration, the `as` cast,
and the `VALID_PERMISSION_POLICIES` Set construction. Used a
typed-array + Set<string> pattern (drift caught at array
construction; runtime Set keeps `.has(string)` ergonomics).

**[Suggestion] InvalidPolicyConfigError discrimination needs
positive tests** (3272493818). Extracted the inline
`policyConfig`-validation logic into an exported
`validatePolicyConfig(policyConfig, onWarning?)` helper and
exported `InvalidPolicyConfigError` itself. Added 7 unit tests
covering: empty config, all 4 valid literals, invalid literal
throws (with class identity check + message regex), 4
non-positive-integer quorum cases throw, valid combination
returns, mismatch (consensusQuorum + non-consensus strategy)
emits warning without throwing, no-warning happy path, and
error messages name the failed field. The boot path in
`runQwenServe` now delegates to the helper (one call site,
DRY).

**[Suggestion] Unanimity breadcrumb spammed per-request**
(3272493829). The Round-7 unanimity stderr line fires inside the
synchronous Promise executor of every `request()` call, which
for a 2-client consensus session is EVERY permission request (M=2
unanimity is the normal operating mode, not a rare edge). Added
`unanimityBreadcrumbEmitted` boolean to the mediator class
(per-mediator dedup, parallel to `consensusQuorumCapNoted` on
`MediatorPending`). One emit per daemon lifetime — visible at
boot, silent thereafter. Comment also corrects the "for even M"
generalization to "for M=2" specifically, matching the actual
condition (`floor(M/2)+1 === M` only for M=1 and M=2).

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

* fix(f3): Round 9 — terminal-event forbidden cleanup + 7 hardening fixes (#4335)

8 follow-up findings from wenshao Round 9 (4 separate review
records: 4326832742 / 4326833568 / 4326844430 / 4326851074, the
last one a non-blocking comment review). 1 Critical + 7 Suggestions.

**[Critical] Terminal events leaked forbiddenVotes history**
(3272576003). `session_died` / `session_closed` / `client_evicted`
/ `stream_error` reducer cases cleared `pendingPermissions` and
`permissionVoteProgress` but not `forbiddenVotes` /
`forbiddenVoteCount`. Adapters reading view state for a dead
session would render stale rejection data. All 4 cases now
zero out the rejection ring + counter. Parameterized regression
test asserts the cleanup contract.

**[Suggestion] safeAudit JSDoc was orphaned over
writeForbiddenStderr** (3272567323). Two consecutive JSDoc
blocks were stacked back-to-back but the method definitions
followed in the opposite order, so IDE hover and API doc
generation showed `safeAudit`'s docs as `writeForbiddenStderr`'s.
Reordered method definitions so each JSDoc precedes its actual
method.

**[Suggestion] writeForbiddenStderr had no test coverage**
(3272568031). Added a 3-path test (designated / consensus /
local-only) that spies on `process.stderr.write` and asserts each
breadcrumb contains the expected reason fragment plus the
requestId + sessionId for grep-ability. Pins the format so a
future refactor can't silently drop the line.

**[Suggestion] resolveEntry numbered list contradicted code**
(3272581553). The N2-invariant cleanup ladder docstring bundled
"delete from pending + write to resolved" into step 2 ahead of
the SSE emit, but the actual code defers `rememberResolved`
until AFTER `safeEmit` (the I5 inline comment on line 1103
correctly explains this). Split step 2 into two halves around
the emit so the spec faithfully describes the ordering invariant.

**[Suggestion] Dead exports in bridgeClient.ts** (3272581548).
`MAX_RESOLVED_PERMISSION_RECORDS`, `PendingPermission`, and
`PermissionResolutionRecord` were defined and exported but no
longer referenced — the mediator owns the same state under
different names (`permissionMediator.ts:77` / `:319`). The
JSDoc still pointed at deleted closures (`registerPending`,
`resolvedPermissions` map). Removed all three definitions and
the matching re-exports in `cli/src/serve/httpAcpBridge.ts`.

**[Suggestion] detectFromLoopback prefix-match had no direct test**
(3272581557). Supertest in the broader server.test.ts suite
always connects from `127.0.0.1`, so the Round-5 prefix-match
fix for `127.x`-beyond-`.0.0.1`, `::1`, `::ffff:127.*`, and the
fail-closed branches had no coverage. Exported the helper from
`server.ts` (loosened parameter type to a minimal shape so tests
don't need to spin up Express) and added an `it.each` table
covering the variants the fix targets, plus an explicit "does
NOT consult X-Forwarded-For" assertion as a security pin.

**[Suggestion] Validate-policies set is a 4th hardcoded copy**
(3272581563). The policy literals already exist in 3 places —
`PermissionPolicy` type, `SERVE_CAPABILITY_REGISTRY.permission_
mediation.modes`, and `settingsSchema.ts` enum options.
`validatePolicyConfig` now derives its valid-set from
`SERVE_CAPABILITY_REGISTRY.permission_mediation.modes` (single
runtime source of truth). Adding a 5th policy upstream lands in
one place; a future drift between the registry and the type
union would still surface at the `as PermissionPolicy` cast.

**[Suggestion] BridgeClient over-coupled to MultiClientPermissionMediator**
(3272581569). `BridgeClient` only ever calls `mediator.request()`
but its field was typed as the concrete class, forcing every
test stub to fake all 6 mediator members. Narrowed the field type
to `Pick<PermissionMediator, 'request'>` (the frozen interface
from `permission.ts`); the bridge factory still passes the full
`MultiClientPermissionMediator` instance via structural typing.
Test stubs simplified from 6 placeholder members to 1.

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

* fix(f3): Round 10 — wenshao APPROVED + 3 final polish (#4335)

wenshao APPROVED the PR (review 4327485978: "No issues found in
the latest Round 9 changes... LGTM ✅") with 3 minor follow-up
suggestions in a separate COMMENTED review (4327443147). All
adopted; the 4th suggestion (3273077262) was already addressed
in Round 9.

**[Suggestion] Symmetric stderr breadcrumb on legacy
respondToPermission** (3273077256). The session-scoped sibling
already writes an unconditional `writeStderrLine` on its
`actualSessionId === undefined` rejection path (Round 8 /
3272493792); the legacy `POST /permission/<id>` route returned
`false` silently after the Round-8 oracle removal, leaving an
observability gap. Added matching `writeStderrLine`. Operators
tailing stderr at 3 AM now see legacy-route 404s without needing
QWEN_SERVE_DEBUG=1.

**[Suggestion] consensusQuorum contract mismatch** (3273077270).
The warning text told the operator "the override will be
ignored" but the function still propagated `permissionConsensusQuorum`
to BridgeOptions. The downstream mediator only reads it under
the consensus policy, so behavior was correct — but the public
contract contradicted itself. Adopt option (a): drop the value
to `undefined` when the strategy is not 'consensus' so the
returned struct matches what the warning promises. Updated the
existing `validatePolicyConfig` test to assert the new contract.

**[Suggestion] Stderr-breadcrumb assertion missing from
error-precedence regression test** (3273077272). The Round-8
test pinned the return-value behavior (`false`) but not the
unconditional-stderr promotion that was the primary behavioral
change of that hunk. Added `vi.spyOn(process.stderr, 'write')`
+ assertions for both "rejected permission vote" and the literal
requestId in the test. A future refactor that drops or downgrades
the log line is now caught.

**[Suggestion] _validPolicies underscore-prefix misleading**
(3273077262 — already addressed). Round 9's commit 6793b89
replaced the literal `_validPolicies` array with a single Set
derived from `SERVE_CAPABILITY_REGISTRY.permission_mediation.modes`
(per separate suggestion 3272581563). The underscore-prefixed
identifier is gone in current HEAD; replied via PR comment
pointing wenshao at the existing fix.

🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
doudouOUC added a commit that referenced this pull request May 20, 2026
…x round 2 fold-in)

Adopts Codex review round 2 P2 finding on PR #4360 — fold-in to the
F4 prereq scope per user's "a" decision.

**Problem**: When the `BridgeFileSystem` adapter (introduced in
#4334 fs adapter wiring) throws a structured `FsError` (e.g.
`kind: 'untrusted_workspace'` / `kind: 'symlink_escape'` / `kind:
'file_too_large'`), the `@agentclientprotocol/sdk` default RPC
error serialization only sends `error.message` as JSON-RPC -32603
"Internal error". The structured `kind` / `status` / `hint` fields
on FsError are stripped on the way to the agent.

Downstream impact: SDK consumers receiving the ACP error payload
lose the typed discriminator and have to regex-match the human-
readable message to dispatch UI (auth retry vs file picker vs
proxy hint). This silently regresses what the FsError-typed
contract was supposed to provide.

**Fix**: At the bridge boundary (`BridgeClient.writeTextFile` and
`BridgeClient.readTextFile`), catch errors from `this.fileSystem.
writeText/readText` calls. Duck-type check for FsError shape
(`err.name === 'FsError'` + `typeof err.kind === 'string'`); when
matched, rethrow as ACP `RequestError(-32603, message, {errorKind,
hint, status})`. The agent's RPC client now receives `data.
errorKind` and can branch on the closed-enum kind.

Cross-package note: FsError lives in `cli/src/serve/fs/errors.ts`
and acp-bridge can't `import { FsError }` from cli (dependency
inversion). Same duck-typing pattern that `mapDomainErrorToErrorKind`
(status.ts) already applies to `TrustGateError` / `SkillError` for
the same cross-package bundling reason — `instanceof` would fail
across package boundaries when bundlers don't dedupe.

**Code shape**

```typescript
function isFsErrorShape(err: unknown): err is FsErrorShape {
  return (
    err instanceof Error &&
    err.name === 'FsError' &&
    typeof (err as { kind?: unknown }).kind === 'string'
  );
}

function preserveFsErrorOverAcp(err: unknown): never {
  if (isFsErrorShape(err)) {
    throw new RequestError(-32603, err.message, {
      errorKind: err.kind,
      ...(err.hint !== undefined ? { hint: err.hint } : {}),
      ...(err.status !== undefined ? { status: err.status } : {}),
    });
  }
  throw err;
}
```

Applied at both `if (this.fileSystem) { ... }` blocks (writeTextFile
+ readTextFile) — wrapped the adapter call in try/catch +
`preserveFsErrorOverAcp(err)`. Non-FsError errors are rethrown
unchanged (default ACP serialization is fine for unstructured
errors; only the structured shape needs preservation).

JSON-RPC code stays at -32603 (internal error) rather than mapping
FsError.kind → JSON-RPC code. Rationale: the JSON-RPC standard
defines only a handful of code values (-32700/-32600/-32601/-32602/
-32603 + a reserved range for application errors), and mapping
~10 FsError kinds to that narrow space is lossy. Instead the
structured `data.errorKind` carries the semantic information SDK
consumers need; JSON-RPC code remains the generic "an error happened"
signal.

**Tests** (+5 in `bridgeClient.test.ts`)

- writeTextFile FsError → ACP RequestError with errorKind in data
- readTextFile FsError preserving symlink_escape kind (no hint
  field present → not stamped, spread guard works)
- non-FsError pass-through (plain Error stays plain Error, no
  RequestError wrap)
- hint field preservation when present
- defensive: error with `kind` field but wrong `name` does NOT get
  wrapped (e.g. PermissionForbiddenError happens to have a kind
  field internally — must NOT be confused for FsError)

Verification: 113/113 acp-bridge tests pass (+5 new FsError-
preservation tests). Full serve suite shows pre-existing F3-related
failures unrelated to this change (verified in isolation).
doudouOUC added a commit that referenced this pull request May 21, 2026
…amp / provenance / errorKind / state_resync_required) (#4360)

* feat(serve): stamp serverTimestamp / tool provenance / errorKind on daemon events (#4175 F4 prereq)

Adopts chiga0's three P0 SDK-side blockers from #4175 comment #19 —
the SDK side already consumes these fields (PR #4353), but daemon
hadn't stamped them yet, leaving the corresponding UI affordances
inert. All three stampings are purely additive on the wire and don't
require any SDK type changes (SDK already has forward-compat field
slots).

**#19.1 — `_meta.serverTimestamp` on every SSE frame** (`server.ts`
`formatSseFrame()`)

Stamped at the SSE write boundary (NOT EventBus.publish) so the
in-memory `BridgeEvent` type stays unchanged and internal consumers
don't see `_meta`. Pre-existing `_meta` keys (e.g. tool_call's
`_meta.toolName`) are preserved via spread merge. SDK reads via the
3-location probe in `extractServerTimestamp` (chiga0's PR #4353);
we pick `_meta.serverTimestamp` (Anthropic convention) so top-level
event type stays unpolluted.

Why this matters: pre-fix, multi-client UIs showing "X minutes ago"
or sorting transcript blocks by emit time used each client's local
clock — drifts of tens of seconds to minutes across browsers/tabs/
mobile produced visibly inconsistent timestamps.

**#19.2 — `tool_call` `provenance` + `serverId` on every emitter
event** (`ToolCallEmitter.ts`)

New static `ToolCallEmitter.resolveToolProvenance(toolName,
subagentMeta)` returns `{ provenance: 'builtin' | 'mcp' | 'subagent';
serverId? }`. Resolution rules (per user-confirmed design decision
from issue comment): subagent takes precedence (set when
subagentMeta is present); `mcp__<server>__<tool>` naming heuristic
classifies MCP tools with serverId; everything else is builtin.

Stamped on `emitStart` AND `emitResult` AND `emitError` (all three
emit paths) so a reconnecting client receiving a `tool_call_update`
frame from the replay ring (without the original `tool_call` start
event) can still derive the provenance. Provenance is stable per
tool, so stamping on every event is redundant — but the marginal
serialization cost is tiny and reconnect correctness wins.

Chose the naming heuristic (not ToolRegistry lookup) per user
confirmation: matches the SDK's own fallback (chiga0 PR #4353), no
new ctx-dep on emit hot path, no signature changes.

**#19.3 — `errorKind` on `stream_error`** (`server.ts` line ~1955)

Stamped via `mapDomainErrorToErrorKind(err)` — the 7-value classifier
already lives in `@qwen-code/acp-bridge/status.ts` since #4319. When
the classifier returns `undefined` (generic Error etc.) the field is
omitted — strictly additive. SDK consumers handle "errorKind absent"
as before (fall back to rendering `error` text).

NOT stamped on `session_died` because the 3 emit sites in `acp-bridge/
bridge.ts` don't have a classifiable `err` in scope:
- `channel_closed` carries only exitCode/signalCode (no error)
- `killed` is user-initiated (no domain error)
- `daemon_shutdown` is operator-initiated (no domain error)

A follow-up could thread channel-spawn errors through to the
session_died emit site to enable `errorKind: 'init_timeout'` /
`missing_binary` classification — left for a separate PR to avoid
mixing protocol stamping with lifecycle plumbing.

Verification
- `npx vitest run packages/cli/src/serve/server.test.ts -t "serverTimestamp|stream_error|errorKind"` — 5 pass
- `npx vitest run packages/cli/src/acp-integration/session/emitters/ToolCallEmitter.test.ts` — 46 pass (+ 11 new tests for resolveToolProvenance + provenance stamping on all 3 emit paths)
- `npx vitest run packages/cli/src/acp-integration/session/HistoryReplayer.test.ts` — 17 pass
- TypeScript clean on touched regions; pre-existing F3 (#4335 merge) errors elsewhere are unrelated.

Existing test updates
- 15 `_meta: { toolName: 'X' }` assertions in ToolCallEmitter.test.ts updated to include `provenance: 'builtin'` (defensive — catches accidental drift if a future refactor stops stamping). 2 strict-equality assertions in HistoryReplayer.test.ts similarly updated. The first SSE-frame test in server.test.ts switched from `toEqual` to `toMatchObject` since `_meta.serverTimestamp` makes exact equality brittle; a dedicated test pins the new field's shape.

* feat(serve+sdk): detect SSE ring eviction on resume, expose state_resync_required (#4175 F4 prereq)

Closes the multi-client SSE reducer divergence bug Ilya0527 raised in
#4175 comment #15. Pre-fix scenario:

1. Consumer's SSE stream drops; client buffers `Last-Event-ID: N`.
2. Network reconnects long enough later that events `[N+1, ringHead-1]`
   were evicted from the daemon's per-session ring.
3. Daemon's `subscribe({lastEventId: N})` silently replays only the
   surviving suffix.
4. Consumer's SDK reducer keeps applying deltas as if the stream was
   contiguous. Its state has now drifted from the daemon's truth —
   no terminal signal, no warning. The `SessionState` reducer's
   "same event stream in → same state out" purity guarantee is
   broken.

The bug's blast radius is exactly when multi-client matters: F4
brings up the TUI / IDE / web client adapters that share session
state, so divergence becomes visibly inconsistent across clients.

**Daemon side** (`packages/acp-bridge/src/eventBus.ts`)

In `subscribe()`'s replay path, detect ring eviction by comparing
the ring's earliest id against `lastEventId + 1`. When a gap exists,
force-push a synthetic terminal `state_resync_required` frame BEFORE
the surviving replay events:

```
{ v: 1, type: 'state_resync_required',
  data: { reason: 'ring_evicted',
          lastDeliveredId: N,
          earliestAvailableId: M } }
```

Per user-confirmed design (issue comment discussion): the frame has
NO `id` (mirrors the `client_evicted` synthetic terminal pattern so
it doesn't burn a slot in the per-session monotonic sequence). Replay
continues after the resync frame — the SDK reducer auto-skips
subsequent deltas (see below) but the frames stay on the wire so
adapters have the option to compute a "what you missed" diff later.

**SDK side** (`packages/sdk-typescript/src/daemon/events.ts`)

Adds:
- `'state_resync_required'` to `DAEMON_EVENT_TYPES` union
- `DaemonStateResyncRequiredData` + `DaemonStateResyncRequiredEvent`
- `isStateResyncRequiredData` predicate
- `DaemonStreamLifecycleEvent` union widened
- Reducer state fields: `awaitingResync: boolean`,
  `resyncRequiredCount: number`, `lastResyncRequired?`
- Reducer case for `state_resync_required` — sets the flag, increments
  count, records data
- **Top-of-reducer gate**: when `awaitingResync === true`, all non-
  terminal events are auto-skipped (still advance `lastEventId`).
  Terminal lifecycle events (`session_died` / `session_closed` /
  `client_evicted` / `stream_error`) STILL apply — critical end-of-
  stream signals don't depend on prior state being current.
- Re-exported `DaemonStateResyncRequiredData` / Event from
  `daemon/index.ts` and `src/index.ts` (matches surface posture of
  sibling lifecycle types).

Consumer recovery contract: when `state.awaitingResync === true`,
call `loadSession` (out of band) to fetch the daemon's canonical
session snapshot, then reconstruct view state via
`createDaemonSessionViewState({...seed from loaded state})`. The
fresh state defaults `awaitingResync: false` so the seed implicitly
clears the flag.

**Side fix** (`stream_error` errorKind)

`DaemonStreamErrorData.errorKind?: string` typed for the optional
classification field that Commit 1 (`14637cd79`) added daemon-side.
Strictly additive — old daemons omit the field, SDK falls back to
rendering `error` text.

Verification
- `packages/acp-bridge`: 6 files, 108/108 pass (+5 new resync-detection
  tests; 1 existing "default ring size 8000" test updated to acknowledge
  the synthetic resync frame at the head of its replay batch).
- `packages/sdk-typescript`: 13 files, 451/451 pass (+8 new reducer
  resync tests covering set/skip/terminal-passthrough/recovery/
  repeated-resync/malformed-payload).
- TypeScript clean across both packages on touched regions.

* fix(acp-bridge): preserve FsError structure over ACP wire (#4360 Codex round 2 fold-in)

Adopts Codex review round 2 P2 finding on PR #4360 — fold-in to the
F4 prereq scope per user's "a" decision.

**Problem**: When the `BridgeFileSystem` adapter (introduced in
#4334 fs adapter wiring) throws a structured `FsError` (e.g.
`kind: 'untrusted_workspace'` / `kind: 'symlink_escape'` / `kind:
'file_too_large'`), the `@agentclientprotocol/sdk` default RPC
error serialization only sends `error.message` as JSON-RPC -32603
"Internal error". The structured `kind` / `status` / `hint` fields
on FsError are stripped on the way to the agent.

Downstream impact: SDK consumers receiving the ACP error payload
lose the typed discriminator and have to regex-match the human-
readable message to dispatch UI (auth retry vs file picker vs
proxy hint). This silently regresses what the FsError-typed
contract was supposed to provide.

**Fix**: At the bridge boundary (`BridgeClient.writeTextFile` and
`BridgeClient.readTextFile`), catch errors from `this.fileSystem.
writeText/readText` calls. Duck-type check for FsError shape
(`err.name === 'FsError'` + `typeof err.kind === 'string'`); when
matched, rethrow as ACP `RequestError(-32603, message, {errorKind,
hint, status})`. The agent's RPC client now receives `data.
errorKind` and can branch on the closed-enum kind.

Cross-package note: FsError lives in `cli/src/serve/fs/errors.ts`
and acp-bridge can't `import { FsError }` from cli (dependency
inversion). Same duck-typing pattern that `mapDomainErrorToErrorKind`
(status.ts) already applies to `TrustGateError` / `SkillError` for
the same cross-package bundling reason — `instanceof` would fail
across package boundaries when bundlers don't dedupe.

**Code shape**

```typescript
function isFsErrorShape(err: unknown): err is FsErrorShape {
  return (
    err instanceof Error &&
    err.name === 'FsError' &&
    typeof (err as { kind?: unknown }).kind === 'string'
  );
}

function preserveFsErrorOverAcp(err: unknown): never {
  if (isFsErrorShape(err)) {
    throw new RequestError(-32603, err.message, {
      errorKind: err.kind,
      ...(err.hint !== undefined ? { hint: err.hint } : {}),
      ...(err.status !== undefined ? { status: err.status } : {}),
    });
  }
  throw err;
}
```

Applied at both `if (this.fileSystem) { ... }` blocks (writeTextFile
+ readTextFile) — wrapped the adapter call in try/catch +
`preserveFsErrorOverAcp(err)`. Non-FsError errors are rethrown
unchanged (default ACP serialization is fine for unstructured
errors; only the structured shape needs preservation).

JSON-RPC code stays at -32603 (internal error) rather than mapping
FsError.kind → JSON-RPC code. Rationale: the JSON-RPC standard
defines only a handful of code values (-32700/-32600/-32601/-32602/
-32603 + a reserved range for application errors), and mapping
~10 FsError kinds to that narrow space is lossy. Instead the
structured `data.errorKind` carries the semantic information SDK
consumers need; JSON-RPC code remains the generic "an error happened"
signal.

**Tests** (+5 in `bridgeClient.test.ts`)

- writeTextFile FsError → ACP RequestError with errorKind in data
- readTextFile FsError preserving symlink_escape kind (no hint
  field present → not stamped, spread guard works)
- non-FsError pass-through (plain Error stays plain Error, no
  RequestError wrap)
- hint field preservation when present
- defensive: error with `kind` field but wrong `name` does NOT get
  wrapped (e.g. PermissionForbiddenError happens to have a kind
  field internally — must NOT be confused for FsError)

Verification: 113/113 acp-bridge tests pass (+5 new FsError-
preservation tests). Full serve suite shows pre-existing F3-related
failures unrelated to this change (verified in isolation).

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

* fix: 3 wenshao observability fold-ins on #4360 (all Suggestion)

Adopts all 3 threads from wenshao's second review round on PR #4360.
All Suggestion-level — daemon-side observability + 1 missing SDK
reducer test.

**[Suggestion] SSE ring eviction silently emits state_resync_required**
(PRRT_kwDOPB-92c6Dp_Uk, eventBus.ts:394)

Pre-fix: when a consumer reconnects past the ring boundary, the
daemon emits `state_resync_required` with zero stderr breadcrumb.
A 3am oncall chasing "my UI is frozen with stale state" couldn't
grep daemon logs to distinguish (a) ring undersized, (b) client
reconnecting too slowly, (c) network partition causing repeated
reconnects.

Fix: detect `next.value.type === 'state_resync_required'` in the
SSE handler's iter loop in `server.ts` and emit a `writeStderrLine`
with the gap details (`lastEventId`, `earliestInRing`, computed
`gap` count, `reason`). Logged at the route boundary rather than
inside `EventBus.subscribe` to keep the bus implementation pure +
concentrate daemon-side observability in the route handler that
already logs socket errors + heartbeats.

**[Suggestion] Bridge iterator throw forwarded to client but not
logged daemon-side** (PRRT_kwDOPB-92c6Dp_Uo, server.ts:1956)

Pre-fix inconsistency: the adjacent `res.on('error', ...)` handler
at line ~1925 logs SSE socket errors with `writeStderrLine`, but
the bridge-iterator-catch block at line ~1940-1965 sends a
`stream_error` SSE frame to the client AND swallows the error
daemon-side. When the bridge iterator throws (subprocess crash,
channel protocol error, unhandled rejection), distinguishing
"subprocess OOM-killed" from "protocol bug" required attaching a
debugger.

Fix: mirror the adjacent handler's pattern — add `writeStderrLine`
before the `stream_error` SSE frame send, including the classified
`errorKind` (when available) in brackets so operators can grep for
`[init_timeout]` / `[missing_binary]` etc.

**[Suggestion] No SDK reducer test verifying stream_error.errorKind
flowthrough** (PRRT_kwDOPB-92c6Dp_Uq, daemonEvents.test.ts:2331)

The daemon-side wire format is tested in `server.test.ts`
(`parsed.data.errorKind === 'init_timeout'`) and
`DaemonStreamErrorData` now declares `errorKind?`, but the SDK
reducer test suite never fed a `stream_error` event with
`errorKind` and asserted `state.streamError?.errorKind`. A future
refactor stripping `errorKind` from the reducer's data assignment
(e.g. spreading only `{error}`) would silently regress without
test signal.

Fix: added `captures errorKind on stream_error in view state` test
exercising the full pipeline — reducer receives stream_error with
errorKind, view state's `streamError.errorKind` matches.

Verification
- `packages/sdk-typescript`: 76/76 daemonEvents tests pass (+1
  new flowthrough test).
- `packages/cli/src/serve/server.test.ts`: 6 targeted serverTimestamp
  / stream_error / errorKind tests pass — server.ts changes are
  observability-only (no behavior change to wire format).
- Pre-existing F3 (#4335 merge) test failures elsewhere are
  unrelated to this PR's changes.

* test(serve): 2 wenshao observability fold-ins on #4360 (stderr log coverage)

Adopts both threads from wenshao's third review round on PR #4360.
Both Suggestion-level — pin the daemon-side stderr log artifacts that
commit `dce2fed0f` introduced. Pre-fix: the EventBus-level
state_resync_required emission was tested in eventBus.test.ts, and
the SSE wire shape was tested in server.test.ts, but the actual
operator-facing artifacts (the stderr log lines themselves) had no
test coverage. A regression swapping operands in the `gap`
arithmetic, dropping the sessionId from the log, or breaking the
`[errorKind]` suffix would ship silently and only surface when an
operator went grepping during an incident.

**[Suggestion] SSE ring eviction stderr log untested**
(PRRT_kwDOPB-92c6Dqtlb, server.ts:1948)

Added 2 tests:
- `writes a daemon-side stderr log on SSE ring eviction` — yields
  a `state_resync_required` frame from a fake bridge, spies on
  `process.stderr.write`, asserts the captured log contains
  `session sess-A` + `lastEventId=5` + `earliestInRing=12` +
  `gap=6 events` (pins the arithmetic) + `reason=ring_evicted` +
  `loadSession` (the recovery hint).
- `falls back to "?" placeholders when state_resync_required data is
  partial` — yields a frame with empty `data: {}`, asserts every
  `?? '?'` branch fires (lastEventId=? / earliestInRing=? /
  gap=? events / reason=?). Defensive against future daemon schema
  changes that drop one of these fields.

**[Suggestion] Bridge iterator error stderr log untested**
(PRRT_kwDOPB-92c6Dqtlh, server.ts:1993)

Added 2 tests:
- `writes a daemon-side stderr log on bridge iterator error` — fake
  bridge throws plain `Error('agent died')` mid-stream, captures
  stderr, asserts the log contains `session sess-A` + `agent died`,
  and **no** `[…]` suffix (plain Error → `mapDomainErrorToErrorKind`
  returns undefined → no suffix).
- `includes [errorKind] suffix in bridge iterator error log when
  classified` — fake bridge throws `BridgeTimeoutError('initialize',
  5000)`, asserts the log contains `[init_timeout]`. Pins the
  classified-vs-unclassified branch of the conditional suffix
  template.

All 4 tests use `vi.spyOn(process.stderr, 'write').mockReturnValue(
true)` + filter `mock.calls` for the relevant log prefix — same
pattern as the existing `mcp-client-manager.test.ts` stderr-spy
tests in core, plus `startupProfiler.test.ts` in cli.

Verification: 7/7 targeted observability tests pass. Pre-existing
F3 (#4335 merge) test failures elsewhere are unrelated to this
PR's changes.
doudouOUC added a commit that referenced this pull request May 23, 2026
…to acp-bridge (#4445)

* refactor(acp-bridge): rename httpAcpBridge.test.ts -> bridge.test.ts (git mv)

Pure file rename; zero content change. Follow-up commits will:
- extract FakeAgent + makeChannel + makeBridge into testUtils.ts
- split 4 daemon-host integration tests back to cli/daemonStatusProvider.test.ts

Part of #4175 F1 test split (deferred from #4334).

* refactor(acp-bridge): extract testUtils + split daemon-host tests to cli (#4175 F1)

Net mechanical extraction following commit 2aff1a4 (pure git mv of
httpAcpBridge.test.ts -> bridge.test.ts). After this commit
`@qwen-code/acp-bridge` owns the bulk of the lifted bridge test
suite, and cli keeps only the 4 daemon-host integration tests that
need to wire `createDaemonStatusProvider()`.

Changes:

1. New `packages/acp-bridge/src/internal/testUtils.ts` (~280 LOC):
   FakeAgent, FakeAgentOpts, ChannelHandle, makeChannel, makeBridge
   (no statusProvider default — acp-bridge tests exercise the
   no-provider fallback path), WS_A/WS_B/SESS_A constants. Marked
   @internal; lives under `internal/` matching the existing
   `stderrLine.ts` package-private convention. Exposed via new
   `./internal/testUtils` subpath in package.json exports.

2. `packages/acp-bridge/src/bridge.test.ts` shrinks from 6861 ->
   ~6400 LOC: fixtures replaced with named imports from
   `./internal/testUtils.js`; cross-package import
   `from './daemonStatusProvider.js'` removed (4 daemon-host tests
   moved out); ACP SDK + bridgeErrors / workspacePaths / bridge /
   channel / bridgeTypes imports split into multiple statements
   reflecting actual post-F1 provenance.

3. New `packages/cli/src/serve/daemonStatusProvider.test.ts`
   (~240 LOC, 4 tests): wires real `createDaemonStatusProvider()`
   through a cli-side `makeBridge` wrapper to assert end-to-end
   daemon env / preflight cells. Imports
   `createHttpAcpBridge` via the `./httpAcpBridge.js` re-export
   shim — doubles as a shim surface smoke check.

Verification:
- acp-bridge: 291/291 tests pass (177 in bridge.test.ts).
- cli: daemonStatusProvider.test.ts 4/4 pass; full cli suite 6742/6767
  green (16 pre-existing failures in AuthDialog / memoryDiagnostics /
  useAtCompletion — all on `daemon_mode_b_main` baseline, last
  modified by commits predating this branch).
- Tests counts pre-split: 181 in httpAcpBridge.test.ts;
  post-split: 177 in bridge.test.ts + 4 in daemonStatusProvider.test.ts
  = 181 (parity preserved).

Part of #4175 F1 test split (deferred from #4334).

* refactor(acp-bridge): self-review round 1 — vitest alias + doc/comment polish

Five code-reviewer findings folded in on top of e97282f:

S1 [Suggestion] — Test-utils ships to npm + cli reads stale dist.
  Added `packages/cli/vitest.config.ts:resolve.alias` mapping
  `@qwen-code/acp-bridge/internal/testUtils` → the .ts source. The
  package subpath export is RETAINED (required for TypeScript
  `nodenext` to resolve types — it won't fall back to tsconfig
  paths once exports rejects a subpath). Dual-channel approach
  documented in the testUtils JSDoc, including the alpha-stage 0.0.1
  tradeoff that the file still ships in dist (stripInternal /
  .npmignore deferred).

S2 [Suggestion] — Stale wording "two tests" in narrative comment.
  bridge.test.ts split-marker now correctly says "4 fallback tests"
  (no-provider × 2 surfaces + throwing-provider × 2 surfaces).

S3 [Suggestion] — "Shim smoke check" only half-applied.
  daemonStatusProvider.test.ts now routes `BridgeOptions` and
  `HttpAcpBridge` types through `./httpAcpBridge.js` shim too
  (alongside `createHttpAcpBridge`), so the entire factory surface
  the cli tests rely on flows through the F1 re-export shim.

N1 [Nit] — Asymmetric split-marker phrasing.
  Both markers now describe the 4 moved tests by surface
  (env real / preflight idle / preflight merged-live /
  preflight extMethod-throws) rather than "1 of" + "3 more".

N2 [Nit] — testUtils "the suite" ambiguity.
  makeChannel JSDoc now references `bridge.test.ts` explicitly
  instead of "the suite" (which was unambiguous pre-split when
  helpers + 10 createInMemoryChannel sites lived in the same file).

Verification: 291/291 acp-bridge tests pass; 4/4 cli daemon
integration tests pass; tsc clean on both packages (pre-existing
server.ts errors on baseline unchanged); eslint --max-warnings 0
clean on all 4 touched files.

* docs(cli): self-review round 2 — fix stale vitest.config.ts alias comment

Round 2 reviewer caught a 3-way contradiction in the round 1 docs:
- vitest.config.ts said: alias replaces the export, internal/* stays
  unpublished (matches stderrLine convention).
- package.json: subpath export IS declared.
- testUtils.ts JSDoc: both channels intentionally retained,
  testUtils ships in dist.

Round 1 explicitly chose to retain the export because TS `nodenext`
won't fall back to tsconfig `paths` once `exports` rejects a
subpath; the alias only serves to short-circuit *runtime* resolution
so cli reads src/ not dist/. Rewriting the vitest.config.ts comment
to reflect that dual-channel reality (and pointing readers at
testUtils.ts for the full rationale).

* fix(acp-bridge): #4445 round 3 fold-in — 4 of 7 reviewer threads adopted

PR #4445 review pass — 4 adopt + 3 decline (declines replied
inline; not folded here):

ADOPTED:

T1 [copilot daemonStatusProvider.test.ts:136 — bridge.shutdown
   missing]: added `await bridge.shutdown()` to test 2 (preflight
   idle). Three of four tests already shut down; symmetry +
   future-proof if `createHttpAcpBridge` gains background work
   even when no channel was spawned.

T5 [wenshao testUtils.ts:92 — makeBridge naming collision]: cli-
   side helper renamed `makeBridge` -> `makeBridgeWithDaemonStatusProvider`
   (4 call sites in daemonStatusProvider.test.ts), JSDoc updated to
   reference the wenshao thread. testUtils.makeBridge stays as the
   canonical name used by ~100 tests in bridge.test.ts. A future
   contributor can no longer pick the wrong helper by accident.

T6 [wenshao testUtils.ts:32 — JSDoc mis-claims @internal tag matches
   stderrLine.ts convention]: fixed wording. stderrLine.ts uses prose
   only; @internal is an additional package-private signal, not a
   convention match. Also restructured the npm-leak paragraph to
   describe the new .npmignore-via-files-negation enforcement (T7).

T7 [wenshao package.json:70 — testUtils ships to npm]: switched
   `files: ["dist"]` -> `files: ["dist", "!dist/internal/testUtils.*",
   "!dist/**/*.test.*"]`. Wenshao's suggested `"test"` exports
   condition wasn't viable: vitest sets `vitest` not `test`, and
   gating on `vitest` would hide types from the cli's tsc compile.
   The negation-pattern files-field excludes the built testUtils
   from the publish surface while keeping the subpath export entry
   that TypeScript `nodenext` needs to resolve types. Verified via
   `npm pack --dry-run`: dist/internal/stderrLine.* still ships
   (production internal helper); dist/internal/testUtils.* +
   dist/**/*.test.* are excluded.

DECLINED (replied on PR threads, not folded here):

T2/T3 [copilot — `handles` array unused in tests 3/4]: bookkeeping
   matches the pre-split bridge.test.ts verbatim; cleanup is scope
   creep on this rename PR.

T4 [copilot — testUtils eager-imports createHttpAcpBridge,
   cross-copy identity risk]: cli daemonStatusProvider.test.ts uses
   its OWN local `makeBridgeWithDaemonStatusProvider` and never
   imports testUtils.makeBridge — the cross-copy concern isn't
   triggered. Premature abstraction on a test-only fixture.

Verification: 291/291 acp-bridge tests pass; 4/4 cli daemon tests
pass; tsc clean both packages; eslint --max-warnings 0 clean on
2 touched .ts files; `npm pack --dry-run` confirms publish-surface
exclusions.
doudouOUC added a commit that referenced this pull request May 27, 2026
…to acp-bridge (#4445)

* refactor(acp-bridge): rename httpAcpBridge.test.ts -> bridge.test.ts (git mv)

Pure file rename; zero content change. Follow-up commits will:
- extract FakeAgent + makeChannel + makeBridge into testUtils.ts
- split 4 daemon-host integration tests back to cli/daemonStatusProvider.test.ts

Part of #4175 F1 test split (deferred from #4334).

* refactor(acp-bridge): extract testUtils + split daemon-host tests to cli (#4175 F1)

Net mechanical extraction following commit 2aff1a4 (pure git mv of
httpAcpBridge.test.ts -> bridge.test.ts). After this commit
`@qwen-code/acp-bridge` owns the bulk of the lifted bridge test
suite, and cli keeps only the 4 daemon-host integration tests that
need to wire `createDaemonStatusProvider()`.

Changes:

1. New `packages/acp-bridge/src/internal/testUtils.ts` (~280 LOC):
   FakeAgent, FakeAgentOpts, ChannelHandle, makeChannel, makeBridge
   (no statusProvider default — acp-bridge tests exercise the
   no-provider fallback path), WS_A/WS_B/SESS_A constants. Marked
   @internal; lives under `internal/` matching the existing
   `stderrLine.ts` package-private convention. Exposed via new
   `./internal/testUtils` subpath in package.json exports.

2. `packages/acp-bridge/src/bridge.test.ts` shrinks from 6861 ->
   ~6400 LOC: fixtures replaced with named imports from
   `./internal/testUtils.js`; cross-package import
   `from './daemonStatusProvider.js'` removed (4 daemon-host tests
   moved out); ACP SDK + bridgeErrors / workspacePaths / bridge /
   channel / bridgeTypes imports split into multiple statements
   reflecting actual post-F1 provenance.

3. New `packages/cli/src/serve/daemonStatusProvider.test.ts`
   (~240 LOC, 4 tests): wires real `createDaemonStatusProvider()`
   through a cli-side `makeBridge` wrapper to assert end-to-end
   daemon env / preflight cells. Imports
   `createHttpAcpBridge` via the `./httpAcpBridge.js` re-export
   shim — doubles as a shim surface smoke check.

Verification:
- acp-bridge: 291/291 tests pass (177 in bridge.test.ts).
- cli: daemonStatusProvider.test.ts 4/4 pass; full cli suite 6742/6767
  green (16 pre-existing failures in AuthDialog / memoryDiagnostics /
  useAtCompletion — all on `daemon_mode_b_main` baseline, last
  modified by commits predating this branch).
- Tests counts pre-split: 181 in httpAcpBridge.test.ts;
  post-split: 177 in bridge.test.ts + 4 in daemonStatusProvider.test.ts
  = 181 (parity preserved).

Part of #4175 F1 test split (deferred from #4334).

* refactor(acp-bridge): self-review round 1 — vitest alias + doc/comment polish

Five code-reviewer findings folded in on top of e97282f:

S1 [Suggestion] — Test-utils ships to npm + cli reads stale dist.
  Added `packages/cli/vitest.config.ts:resolve.alias` mapping
  `@qwen-code/acp-bridge/internal/testUtils` → the .ts source. The
  package subpath export is RETAINED (required for TypeScript
  `nodenext` to resolve types — it won't fall back to tsconfig
  paths once exports rejects a subpath). Dual-channel approach
  documented in the testUtils JSDoc, including the alpha-stage 0.0.1
  tradeoff that the file still ships in dist (stripInternal /
  .npmignore deferred).

S2 [Suggestion] — Stale wording "two tests" in narrative comment.
  bridge.test.ts split-marker now correctly says "4 fallback tests"
  (no-provider × 2 surfaces + throwing-provider × 2 surfaces).

S3 [Suggestion] — "Shim smoke check" only half-applied.
  daemonStatusProvider.test.ts now routes `BridgeOptions` and
  `HttpAcpBridge` types through `./httpAcpBridge.js` shim too
  (alongside `createHttpAcpBridge`), so the entire factory surface
  the cli tests rely on flows through the F1 re-export shim.

N1 [Nit] — Asymmetric split-marker phrasing.
  Both markers now describe the 4 moved tests by surface
  (env real / preflight idle / preflight merged-live /
  preflight extMethod-throws) rather than "1 of" + "3 more".

N2 [Nit] — testUtils "the suite" ambiguity.
  makeChannel JSDoc now references `bridge.test.ts` explicitly
  instead of "the suite" (which was unambiguous pre-split when
  helpers + 10 createInMemoryChannel sites lived in the same file).

Verification: 291/291 acp-bridge tests pass; 4/4 cli daemon
integration tests pass; tsc clean on both packages (pre-existing
server.ts errors on baseline unchanged); eslint --max-warnings 0
clean on all 4 touched files.

* docs(cli): self-review round 2 — fix stale vitest.config.ts alias comment

Round 2 reviewer caught a 3-way contradiction in the round 1 docs:
- vitest.config.ts said: alias replaces the export, internal/* stays
  unpublished (matches stderrLine convention).
- package.json: subpath export IS declared.
- testUtils.ts JSDoc: both channels intentionally retained,
  testUtils ships in dist.

Round 1 explicitly chose to retain the export because TS `nodenext`
won't fall back to tsconfig `paths` once `exports` rejects a
subpath; the alias only serves to short-circuit *runtime* resolution
so cli reads src/ not dist/. Rewriting the vitest.config.ts comment
to reflect that dual-channel reality (and pointing readers at
testUtils.ts for the full rationale).

* fix(acp-bridge): #4445 round 3 fold-in — 4 of 7 reviewer threads adopted

PR #4445 review pass — 4 adopt + 3 decline (declines replied
inline; not folded here):

ADOPTED:

T1 [copilot daemonStatusProvider.test.ts:136 — bridge.shutdown
   missing]: added `await bridge.shutdown()` to test 2 (preflight
   idle). Three of four tests already shut down; symmetry +
   future-proof if `createHttpAcpBridge` gains background work
   even when no channel was spawned.

T5 [wenshao testUtils.ts:92 — makeBridge naming collision]: cli-
   side helper renamed `makeBridge` -> `makeBridgeWithDaemonStatusProvider`
   (4 call sites in daemonStatusProvider.test.ts), JSDoc updated to
   reference the wenshao thread. testUtils.makeBridge stays as the
   canonical name used by ~100 tests in bridge.test.ts. A future
   contributor can no longer pick the wrong helper by accident.

T6 [wenshao testUtils.ts:32 — JSDoc mis-claims @internal tag matches
   stderrLine.ts convention]: fixed wording. stderrLine.ts uses prose
   only; @internal is an additional package-private signal, not a
   convention match. Also restructured the npm-leak paragraph to
   describe the new .npmignore-via-files-negation enforcement (T7).

T7 [wenshao package.json:70 — testUtils ships to npm]: switched
   `files: ["dist"]` -> `files: ["dist", "!dist/internal/testUtils.*",
   "!dist/**/*.test.*"]`. Wenshao's suggested `"test"` exports
   condition wasn't viable: vitest sets `vitest` not `test`, and
   gating on `vitest` would hide types from the cli's tsc compile.
   The negation-pattern files-field excludes the built testUtils
   from the publish surface while keeping the subpath export entry
   that TypeScript `nodenext` needs to resolve types. Verified via
   `npm pack --dry-run`: dist/internal/stderrLine.* still ships
   (production internal helper); dist/internal/testUtils.* +
   dist/**/*.test.* are excluded.

DECLINED (replied on PR threads, not folded here):

T2/T3 [copilot — `handles` array unused in tests 3/4]: bookkeeping
   matches the pre-split bridge.test.ts verbatim; cleanup is scope
   creep on this rename PR.

T4 [copilot — testUtils eager-imports createHttpAcpBridge,
   cross-copy identity risk]: cli daemonStatusProvider.test.ts uses
   its OWN local `makeBridgeWithDaemonStatusProvider` and never
   imports testUtils.makeBridge — the cross-copy concern isn't
   triggered. Premature abstraction on a test-only fixture.

Verification: 291/291 acp-bridge tests pass; 4/4 cli daemon tests
pass; tsc clean both packages; eslint --max-warnings 0 clean on
2 touched .ts files; `npm pack --dry-run` confirms publish-surface
exclusions.
doudouOUC added a commit that referenced this pull request Jun 11, 2026
* perf(core): F2 cleanup PR A — R9/W11/W12/R10 (post-merge follow-ups) (#4411)

* refactor(core): F2 PR A R9 — McpClientManager options-object ctor

R9 (filed as F2 follow-up from #4336 review): 7 positional ctor args
collapse to (config, toolRegistry, options?: McpClientManagerOptions).
The trailing 5 (eventEmitter, sendSdkMcpMessage, healthConfig,
budgetConfig, pool) become named fields on `McpClientManagerOptions`.
Test factory `mkManager(overrides?)` introduced at the top of
`mcp-client-manager.test.ts` so each of the prior 80 inline
constructions becomes a single line naming only the field(s) the test
overrides; the 4 `undefined` sentinels each test threaded through to
reach the trailing `pool` arg are gone.

Net: 113 LOC removed (test) + 35 LOC added (src exposes interface +
mkManager factory + tool-registry call site update). Behavior
unchanged — same field assignments, same downgrade-enforce-without-
budget breadcrumb, same budget event wiring.

Filed bucket: F2 perf / cleanup PR A (R9 + W11 + W12 + R10/R23 T7),
see issue #4175 item 7 "F2 post-merge cleanup PRs". This is the first
of the 4 fixes in PR A; W11/W12/R10 follow as separate commits.

Test sweep: 84/84 mcp-client-manager.test.ts pass; typecheck clean.

* refactor(core): F2 PR A W11 — extract attachPooledSession + rollbackReservationOnSpawnFailure

W11 (filed as F2 follow-up from #4336 review): two private helpers
on `McpTransportPool` to eliminate inline duplication in `acquire()`:

  - `attachPooledSession(entry, id, serverName, cfg, sessionId,
    toolReg, promptReg)`: builds `SessionMcpView` + `entry.attach`
    with the standard pool release callback. Used by both the
    fast-path attach (existing entry) and the post-spawn attach
    (after `await inFlight`). NOT used by `createUnpooledConnection`
    — its release callback runs `entry.forceShutdown('manual')` +
    `indexDetach` directly (no pool refcount accounting since
    unpooled entries are per-session).

  - `rollbackReservationOnSpawnFailure(reservationResult, serverName)`:
    R24 T17 contract — only release the budget slot if THIS acquire
    actually reserved a new slot (`'reserved'`); `'already_held'`
    skips because the sibling owns it. Used by both the unpooled
    catch and the pooled spawn-in-flight catch.

Race-window invariants (W10 / W77 / W90 / W111 / W125 / R24 T17)
stay at the call sites because they describe the SURROUNDING
ordering, not the helpers themselves. Helpers are documented to
defer those decisions back to callers.

Behavior unchanged. Filed bucket: F2 perf cleanup PR A (R9 done /
W11 this commit / W12 + R10 to follow).

Test sweep: 28/28 mcp-transport-pool.test.ts pass; typecheck clean.

* refactor(core): F2 PR A W12 — SessionMcpView precompute filter Sets

W12 (filed as F2 follow-up from #4336 review): `applyTools` /
`applyPrompts` precompute `excludeSet` + `includeSet` once per pass
instead of scanning `cfg.includeTools` / `cfg.excludeTools` arrays
inside every per-tool iteration.

Pre-fix the per-tool predicate (`passesSessionFilter`) walked both
arrays for every snapshot entry → O(M × N) per `applyTools` call.
With M tools × N filter entries, typical M=5-20 / N=2-5 case
finishes in microseconds either way; the win is data-structure
correctness and code clarity, not perceived perf.

`passesSessionFilter` / `passesSessionPromptFilter` (the array-
based predicates) stay exported and unchanged for unit tests + any
caller wanting to test a single name without paying Set construction.
The bulk path uses two new private helpers `compileNameFilter` +
`compiledFilterAccepts` whose Sets live on the `applyTools` /
`applyPrompts` stack frame.

Same semantics: `excludeTools` is direct-equality match (no parens
strip — pre-F2 behavior preserved); `includeTools` strips the first
`(...)` suffix so `toolName(args)` matches `toolName`.

Filed bucket: F2 perf cleanup PR A (R9 + W11 done / W12 this commit
/ R10 to follow).

Test sweep: 13/13 session-mcp-view.test.ts pass; typecheck clean.

* perf(core): F2 PR A R10 / R23 T7 — pid-descendants ps snapshot + pgrep fallback

R10 / R23 T7 (filed as F2 follow-up from #4336 review): the Linux
/ macOS pid-descendant enumeration moves from per-pid `pgrep -P
<pid>` BFS (one subprocess fork per node visited) to a single
`ps -A -o pid=,ppid=` snapshot followed by an in-memory tree walk
over `Map<ppid, pid[]>`. Windows analog: single `Get-CimInstance
Win32_Process | ConvertTo-Csv` snapshot of all `(ProcessId,
ParentProcessId)` rows replaces per-pid
`Get-CimInstance -Filter "ParentProcessId=$p"` BFS.

Two motivations:
  1. **Fork count**: typical `npx → tool` / `uvx → tool` wrapper
     trees are 2-3 levels deep with B=1-3 children per node →
     pre-fix BFS forked ~5-10 subprocesses per pool-shutdown call.
     Post-fix: exactly 1 fork regardless of tree depth.
  2. **Snapshot consistency**: pre-fix BFS walked the table level
     by level; a child that forked between two adjacent BFS levels
     could be missed (we'd see the child but query its
     descendants AFTER the new fork). The snapshot path captures
     the table at one instant; new descendants forked after the
     snapshot are tolerated by the existing ESRCH-tolerant
     SIGTERM loop.

Caveats:
  - `ps -A -o pid=,ppid=` is POSIX standard (macOS / Linux /
    *BSD), but BusyBox `ps` <v1.28 (2018) doesn't support `-o`.
    Distroless containers may not have `ps` at all. To preserve
    behavior on those edge platforms, the legacy per-pid `pgrep`
    BFS is retained as a fallback (`listDescendantPidsUnixPgrepFallback`).
    Same retention on Windows for the per-pid filter path.
  - Snapshot path uses `maxBuffer: 8MB` to cover ~250k-process
    pathological hosts. Default 1MB would clip at ~30k processes.
  - `MAX_DESCENDANTS = 256` / `MAX_DEPTH = 8` caps preserved on
    both snapshot + fallback paths.
  - Snapshot scans the entire host process table (not just the
    target subtree). On the typical 200-500 process developer
    machine this parses in <10ms; the win over BFS is real but
    not order-of-magnitude — ~2x improvement, not 100x. PR A's
    motivation framing is "fork hygiene + consistency", not raw
    perf.

Empty-result detection: snapshot path tracks `parsedRows`. If the
ps/CIM tool runs successfully but produces 0 parseable rows
(BusyBox without `-o` echoing usage, AppLocker truncating CIM
output, etc.), we throw — the outer catch falls back to the
per-pid path. A genuine "root has no children" case parses many
rows and just returns empty from the walk. So the
"no-children-found" semantics are preserved across both paths.

Test gate update: pre-fix `integration: spawn-and-enumerate` test
skipped on `CI === '1'` because pgrep wasn't available on
minimal CI runners. Post-fix `ps -A` is universally available on
non-distroless Linux/macOS — only the Windows skip remains.
6/6 pid-descendants tests pass including the now-active
integration spawn test.

Design doc (`docs/design/f2-mcp-transport-pool.md` §6.4 + the F2
follow-up table at lines 82-85) updated to reflect the snapshot
+ fallback shape, and to mark W11 / W12 / R9 / R10 as ✅ Done in
PR A with the per-fix commit refs.

This commit completes F2 cleanup PR A. Filed bucket order:
R9 (commit 0cb1eaa27) → W11 (commit 2d546efca) → W12 (commit
a4a855ab3) → R10 (this commit). Issue #4175 item 7 "F2 post-
merge cleanup PRs": PR A done; PR B (W93 + W133-a + W134) and
PR C (W133-c SDK breaking) to follow as separate clusters.

Test sweep: 287/287 F2 + cli pass; ESLint clean; typecheck clean
(core + cli). Integration test on macOS local runs the new
snapshot path successfully.

* refactor(core): F2 PR A R2 — wenshao followup (visited set + dedup predicate)

Two Suggestions from wenshao's first PR #4411 review pass (07:15Z),
both small and worth folding before merge:

PR-A-R2 #1 (pid-descendants.ts:309 — walkDescendants visited set):
  `walkDescendants`'s BFS lacked a `visited` set. If the snapshot
  captures a PID-reuse cycle — rare but possible on busy hosts with
  rapid pid churn between `ps -A`'s start and parse, where Linux
  wraparound can show a freed pid in a different parent's children
  list creating an A→B / B→A cycle — pre-fix BFS would revisit nodes
  and fill the MAX_DESCENDANTS=256 quota with duplicate entries,
  starving legitimate descendants. Pre-PR-A the per-pid `pgrep` BFS
  had the same theoretical issue but was less exposed (each
  `pgrep -P pid` call returns only DIRECT children; snapshot captures
  the whole tree at once, making cycles instantly visible).

  Fix: 3-LOC `Set<number>` add. `root` seeded into `visited` so a
  malformed snapshot listing root as a descendant of its own child
  doesn't re-enqueue root either.

PR-A-R2 #2 (session-mcp-view.ts:117 — predicate dedup):
  After W12, the exported `passesSessionFilter` /
  `passesSessionPromptFilter` still called `passesNameFilter` (the
  pre-W12 array-based implementation), while `applyTools` /
  `applyPrompts` used `compiledFilterAccepts(compileNameFilter(...))`.
  Two parallel implementations of the same predicate — future change
  to one without the other would silently diverge:
    - the exported function's tests (passesSessionFilter unit tests)
      would still pass
    - the production filter path in applyTools/applyPrompts would
      behave differently

  Reviewer also noted `passesSessionPromptFilter` had zero callers
  in production code or tests after W12 — `applyPrompts` no longer
  references it. Kept the export rather than deleting it (matches
  the `passesSessionFilter` shape for symmetry + the F3 audit-path
  comment block earmarks both as the replay predicates), but routed
  both through `compiledFilterAccepts(compileNameFilter(...))` so
  there is a single source of truth. Set construction is per-call
  for these exports (negligible for unit-test / one-off probes);
  the bulk paths in `applyTools` / `applyPrompts` still construct
  ONE filter per pass via the original W12 code path.

`passesNameFilter` (the standalone array-based helper) deleted —
its only callers were the two exports, which now use the compiled
path. Public-API surface unchanged: the two exported functions
keep their signatures and semantics.

Test sweep: 19/19 pid-descendants + session-mcp-view tests pass;
typecheck + ESLint clean.

Continues commit chain: f05917071 (R9) → 20d2f1b90 (W11) →
6cf18f641 (W12) → 2a41c6fae (R10) → this (R2 followups).

* fix(core): F2 PR A R3 T3 — Windows CSV delimiter locale fix

`ConvertTo-Csv -NoTypeInformation` honors the system locale's list
separator on PowerShell 5.1. On German / French / Dutch / Italian /
... locales the separator is `;` not `,`, so the regex
`^"(\d+)","(\d+)"$` in `snapshotProcessTreeWin` never matched →
`parsedRows === 0` → snapshot threw → fell back to the per-pid CIM
filter path with ~0.5-1s extra PowerShell startup latency per
descendant on every pool shutdown.

Fix: 1-LOC `-Delimiter ","` on `ConvertTo-Csv`. Forces comma
regardless of locale or PowerShell version. PowerShell 7+ defaults
to comma already; 5.1 (the Windows-bundled version most users have
without explicit upgrade) honored locale. The explicit delimiter
makes both consistent.

Skipped wenshao's companion Suggestion T4 (test coverage for
walkDescendants MAX_DESCENDANTS / MAX_DEPTH caps) as F2 hardening
follow-up — the caps are simple 2-line guards exercisable by
inspection; ~50 LOC of mock infrastructure isn't commensurate
with the regression risk on currently-stable defensive code,
and (per the issue #4175 follow-up bucket) we keep dedicated
test-coverage work out of perf-cleanup PRs.

Continues commit chain: f05917071 (R9) → 20d2f1b90 (W11) →
6cf18f641 (W12) → 2a41c6fae (R10) → ced5d62b0 (R2) → this (R3 T3).

Test sweep: 6/6 pid-descendants tests pass; typecheck + ESLint clean.

* refactor(acp-bridge): F1 test split — lift bridge.test.ts (6861 LOC) to acp-bridge (#4445)

* refactor(acp-bridge): rename httpAcpBridge.test.ts -> bridge.test.ts (git mv)

Pure file rename; zero content change. Follow-up commits will:
- extract FakeAgent + makeChannel + makeBridge into testUtils.ts
- split 4 daemon-host integration tests back to cli/daemonStatusProvider.test.ts

Part of #4175 F1 test split (deferred from #4334).

* refactor(acp-bridge): extract testUtils + split daemon-host tests to cli (#4175 F1)

Net mechanical extraction following commit 2aff1a4d1 (pure git mv of
httpAcpBridge.test.ts -> bridge.test.ts). After this commit
`@qwen-code/acp-bridge` owns the bulk of the lifted bridge test
suite, and cli keeps only the 4 daemon-host integration tests that
need to wire `createDaemonStatusProvider()`.

Changes:

1. New `packages/acp-bridge/src/internal/testUtils.ts` (~280 LOC):
   FakeAgent, FakeAgentOpts, ChannelHandle, makeChannel, makeBridge
   (no statusProvider default — acp-bridge tests exercise the
   no-provider fallback path), WS_A/WS_B/SESS_A constants. Marked
   @internal; lives under `internal/` matching the existing
   `stderrLine.ts` package-private convention. Exposed via new
   `./internal/testUtils` subpath in package.json exports.

2. `packages/acp-bridge/src/bridge.test.ts` shrinks from 6861 ->
   ~6400 LOC: fixtures replaced with named imports from
   `./internal/testUtils.js`; cross-package import
   `from './daemonStatusProvider.js'` removed (4 daemon-host tests
   moved out); ACP SDK + bridgeErrors / workspacePaths / bridge /
   channel / bridgeTypes imports split into multiple statements
   reflecting actual post-F1 provenance.

3. New `packages/cli/src/serve/daemonStatusProvider.test.ts`
   (~240 LOC, 4 tests): wires real `createDaemonStatusProvider()`
   through a cli-side `makeBridge` wrapper to assert end-to-end
   daemon env / preflight cells. Imports
   `createHttpAcpBridge` via the `./httpAcpBridge.js` re-export
   shim — doubles as a shim surface smoke check.

Verification:
- acp-bridge: 291/291 tests pass (177 in bridge.test.ts).
- cli: daemonStatusProvider.test.ts 4/4 pass; full cli suite 6742/6767
  green (16 pre-existing failures in AuthDialog / memoryDiagnostics /
  useAtCompletion — all on `daemon_mode_b_main` baseline, last
  modified by commits predating this branch).
- Tests counts pre-split: 181 in httpAcpBridge.test.ts;
  post-split: 177 in bridge.test.ts + 4 in daemonStatusProvider.test.ts
  = 181 (parity preserved).

Part of #4175 F1 test split (deferred from #4334).

* refactor(acp-bridge): self-review round 1 — vitest alias + doc/comment polish

Five code-reviewer findings folded in on top of e97282f30:

S1 [Suggestion] — Test-utils ships to npm + cli reads stale dist.
  Added `packages/cli/vitest.config.ts:resolve.alias` mapping
  `@qwen-code/acp-bridge/internal/testUtils` → the .ts source. The
  package subpath export is RETAINED (required for TypeScript
  `nodenext` to resolve types — it won't fall back to tsconfig
  paths once exports rejects a subpath). Dual-channel approach
  documented in the testUtils JSDoc, including the alpha-stage 0.0.1
  tradeoff that the file still ships in dist (stripInternal /
  .npmignore deferred).

S2 [Suggestion] — Stale wording "two tests" in narrative comment.
  bridge.test.ts split-marker now correctly says "4 fallback tests"
  (no-provider × 2 surfaces + throwing-provider × 2 surfaces).

S3 [Suggestion] — "Shim smoke check" only half-applied.
  daemonStatusProvider.test.ts now routes `BridgeOptions` and
  `HttpAcpBridge` types through `./httpAcpBridge.js` shim too
  (alongside `createHttpAcpBridge`), so the entire factory surface
  the cli tests rely on flows through the F1 re-export shim.

N1 [Nit] — Asymmetric split-marker phrasing.
  Both markers now describe the 4 moved tests by surface
  (env real / preflight idle / preflight merged-live /
  preflight extMethod-throws) rather than "1 of" + "3 more".

N2 [Nit] — testUtils "the suite" ambiguity.
  makeChannel JSDoc now references `bridge.test.ts` explicitly
  instead of "the suite" (which was unambiguous pre-split when
  helpers + 10 createInMemoryChannel sites lived in the same file).

Verification: 291/291 acp-bridge tests pass; 4/4 cli daemon
integration tests pass; tsc clean on both packages (pre-existing
server.ts errors on baseline unchanged); eslint --max-warnings 0
clean on all 4 touched files.

* docs(cli): self-review round 2 — fix stale vitest.config.ts alias comment

Round 2 reviewer caught a 3-way contradiction in the round 1 docs:
- vitest.config.ts said: alias replaces the export, internal/* stays
  unpublished (matches stderrLine convention).
- package.json: subpath export IS declared.
- testUtils.ts JSDoc: both channels intentionally retained,
  testUtils ships in dist.

Round 1 explicitly chose to retain the export because TS `nodenext`
won't fall back to tsconfig `paths` once `exports` rejects a
subpath; the alias only serves to short-circuit *runtime* resolution
so cli reads src/ not dist/. Rewriting the vitest.config.ts comment
to reflect that dual-channel reality (and pointing readers at
testUtils.ts for the full rationale).

* fix(acp-bridge): #4445 round 3 fold-in — 4 of 7 reviewer threads adopted

PR #4445 review pass — 4 adopt + 3 decline (declines replied
inline; not folded here):

ADOPTED:

T1 [copilot daemonStatusProvider.test.ts:136 — bridge.shutdown
   missing]: added `await bridge.shutdown()` to test 2 (preflight
   idle). Three of four tests already shut down; symmetry +
   future-proof if `createHttpAcpBridge` gains background work
   even when no channel was spawned.

T5 [wenshao testUtils.ts:92 — makeBridge naming collision]: cli-
   side helper renamed `makeBridge` -> `makeBridgeWithDaemonStatusProvider`
   (4 call sites in daemonStatusProvider.test.ts), JSDoc updated to
   reference the wenshao thread. testUtils.makeBridge stays as the
   canonical name used by ~100 tests in bridge.test.ts. A future
   contributor can no longer pick the wrong helper by accident.

T6 [wenshao testUtils.ts:32 — JSDoc mis-claims @internal tag matches
   stderrLine.ts convention]: fixed wording. stderrLine.ts uses prose
   only; @internal is an additional package-private signal, not a
   convention match. Also restructured the npm-leak paragraph to
   describe the new .npmignore-via-files-negation enforcement (T7).

T7 [wenshao package.json:70 — testUtils ships to npm]: switched
   `files: ["dist"]` -> `files: ["dist", "!dist/internal/testUtils.*",
   "!dist/**/*.test.*"]`. Wenshao's suggested `"test"` exports
   condition wasn't viable: vitest sets `vitest` not `test`, and
   gating on `vitest` would hide types from the cli's tsc compile.
   The negation-pattern files-field excludes the built testUtils
   from the publish surface while keeping the subpath export entry
   that TypeScript `nodenext` needs to resolve types. Verified via
   `npm pack --dry-run`: dist/internal/stderrLine.* still ships
   (production internal helper); dist/internal/testUtils.* +
   dist/**/*.test.* are excluded.

DECLINED (replied on PR threads, not folded here):

T2/T3 [copilot — `handles` array unused in tests 3/4]: bookkeeping
   matches the pre-split bridge.test.ts verbatim; cleanup is scope
   creep on this rename PR.

T4 [copilot — testUtils eager-imports createHttpAcpBridge,
   cross-copy identity risk]: cli daemonStatusProvider.test.ts uses
   its OWN local `makeBridgeWithDaemonStatusProvider` and never
   imports testUtils.makeBridge — the cross-copy concern isn't
   triggered. Premature abstraction on a test-only fixture.

Verification: 291/291 acp-bridge tests pass; 4/4 cli daemon tests
pass; tsc clean both packages; eslint --max-warnings 0 clean on
2 touched .ts files; `npm pack --dry-run` confirms publish-surface
exclusions.

* fix(core): F2 cleanup PR B — self-heal observability (W133-a + W134) (#4460)

* fix(core): F2 cleanup PR B — self-heal observability (W133-a + W134)

W93 declined as already satisfied by W1 fix in #4336 commit 6
(spawnEntry's catch already calls forceShutdown which runs the full
cleanup table — listener removal, timer clear, subscriber detach,
sweep+disconnect, onClosed eviction). Source-verified non-repro.

W133-a: McpClient.onerror now captures the error in a private
`lastTransportError` field (reset at each connect()); the W120
silent-drop block at mcp-pool-entry.ts:346 reads it via the new
`getLastTransportError()` getter and appends `: <error.message>` to
the lastError string on the emitted 'failed' event. Preserves the
literal "silent transport drop" prefix invariant for log-grep
backward compat — pre-fix marker stays a substring.

W134: sweepAndDisconnect now returns SweepResult instead of void —
{ pidSweepError?, disconnectError?, descendantsFound?,
descendantsSignaled? }. The silent-drop fire-and-forget caller chains
to inspect the result and emits a structured warn log when either
pid-sweep threw OR sigtermPids partially signaled (signaled < found)
— surfaces orphan-process pressure without inflating PR scope (no
new SSE event or SDK reducer state; deferred to W134-followup if
maintainers want metrics).

forceShutdown / doRestart sweep callers ignore the return value (JS
implicit-void at await sites preserves behavior).

4 new tests in mcp-transport-pool.test.ts covering W133-a happy path
+ fallback (no prior onerror) + W134 pidSweepError + W134
partial-signal failure modes. Module-mocks pid-descendants.js for
controllable sweep behavior, and debugLogger.js to observe warn
calls (production logger is session-gated and a no-op in tests).
Singleton-stub debugLogger mock so production module-load
`createDebugLogger('McpPool:Entry')` and the test's retrieval get
the same vi.fn instances.

Verification:
- tsc clean: packages/core, packages/cli (server.ts pre-existing
  errors unchanged)
- F2 transport-pool: 32/32 pass (28 pre-existing + 4 new)
- mcp-client: 46/46 pass
- eslint --max-warnings 0 clean on 3 touched files

Part of #4175 #4336 follow-up bucket.

* fix(core): #4460 round 1 fold-in — 4 copilot doc/comment threads adopted

T1 [copilot mcp-pool-entry.ts:116 — stale line ref in SweepResult JSDoc]:
  replaced `mcp-pool-entry.ts:383` with stable method-anchor reference
  to the W120 silent-drop block inside `statusChangeListener`. Line
  numbers drift on every edit; method names don't.

T2 [copilot mcp-pool-entry.ts:453 — `?? 0` ambiguous in warn payload]:
  silent-drop warn log now prints `descendantsFound=unknown` and
  `descendantsSignaled=unknown` when the values are undefined (only
  reachable in the pidSweepError branch — sweep threw before
  assignment). Operators triaging the warn can now distinguish
  "sweep succeeded but found 0 descendants" from "sweep itself
  threw, count is genuinely unmeasured". Locked in via a new
  assertion in the W134 pidSweepError test.

T3 [copilot mcp-client.ts:116 — brittle line refs in lastTransportError
  JSDoc]: replaced `mcp-pool-entry.ts:346` and `mcp-client.ts:130`
  with stable method/block names (the `statusChangeListener` silent-
  drop block; the `client.onerror` arrow inside connect()). Same
  fix applied to the parallel comment in mcp-transport-pool.test.ts:730
  for consistency.

T4 [copilot mcp-transport-pool.test.ts:797 — singleton-stub mock comment
  contradictory]: rewrote the comment to unambiguously describe what
  the mock DOES (factory body runs once; inner arrow returns the same
  object on every call) instead of the prior hypothetical phrasing
  ("Returning a fresh object would have...") which read as a
  description of current behavior at first glance.

All 4 are doc/comment fixes — zero behavior change apart from the
T2 string format ('unknown' instead of '0'). Verified:
- 32/32 mcp-transport-pool.test.ts pass
- tsc clean on packages/core
- eslint --max-warnings 0 clean on 3 touched files

* fix(core): #4460 round 2 fold-in — remove dead SweepResult.disconnectError field

T5 [wenshao mcp-pool-entry.ts:134 — `disconnectError` is dead data]:
  glm-5.1 review caught that the field was populated when
  `client.disconnect()` threw (line 844) but no consumer ever read
  it — the silent-drop `.then()` handler gated only on
  `pidSweepError` and partial-signal; `forceShutdown` and `doRestart`
  ignore the return; no test asserted on it.

Removed the field from `SweepResult` and the assignment in the
disconnect catch. The pre-existing `debugLogger.error(`client.disconnect
failed for ...`)` inside `sweepAndDisconnect` already gives operators
the signal — adding it to the outer silent-drop warn would have been
duplicate noise. If a future consumer needs to gate logic on disconnect
failures, re-add the field + reader at that point.

Verification: 32/32 mcp-transport-pool.test.ts pass; tsc + eslint
clean on the touched file.

* feat(sdk/daemon-ui): unified completeness follow-up to #4328 (#4353)

* feat(sdk/daemon-ui): expand event coverage to 28+ daemon event types (PR-A)

Closes the "12+ daemon events fall through to debug" gap surfaced in the PR
the daemon currently emits (Stage 1 + Wave 3-4), so renderers stop having
to peek at `rawEvent.data` for known event categories.

Session-meta:
- session.metadata.changed (from session_metadata_updated)
- session.approval_mode.changed (from approval_mode_changed)
- session.available_commands (from available_commands_update; upgraded
  from a status-text fallback to a typed event carrying the command list)

Workspace state (Wave 3-4):
- workspace.memory.changed
- workspace.agent.changed
- workspace.tool.toggled
- workspace.initialized
- workspace.mcp.budget_warning
- workspace.mcp.child_refused
- workspace.mcp.server_restarted
- workspace.mcp.server_restart_refused

Auth device-flow (Wave 4 OAuth, RFC 8628):
- auth.device_flow.started
- auth.device_flow.throttled
- auth.device_flow.authorized
- auth.device_flow.failed (carries DaemonAuthDeviceFlowSdkErrorKind)
- auth.device_flow.cancelled

- `DaemonUiErrorEvent.errorKind?: DaemonErrorKind` — closed-enum error
  category propagated from daemon's typed-error taxonomy. Renderers can
  branch on errorKind for "retry auth" vs "check file path" affordances
  instead of regex-matching `text`.
- `DaemonUiToolUpdateEvent.provenance?: DaemonUiToolProvenance` +
  `.serverId?` — closed enum ('builtin' | 'mcp' | 'subagent' | 'unknown').
  Falls back to the `mcp__<server>__<tool>` naming heuristic when the
  daemon doesn't stamp provenance explicitly. Unblocks UI namespace
  dispatch without string-matching toolName.

Session-meta / workspace / auth events do NOT push transcript blocks.
They are intentional sidechannel observations: `lastEventId` advances
(monotonic invariant preserved), but the chat-stream transcript stays
focused on user/assistant/tool/shell/permission content. Renderers
consume them via selectors (introduced in follow-up PRs).

All new event types produce short structured lines in
`daemonUiEventToTerminalText` for tail-style debug consumers. Web/IDE
renderers should consume the typed events directly via subscription.

40/40 tests pass. New tests verify:
- All 16 new event types normalize correctly
- Malformed payloads fall back to debug without leaking raw data
  (`secret` field never appears in fallback text)
- MCP tool provenance heuristic (`mcp__github__create_issue` →
  provenance='mcp', serverId='github')
- errorKind propagation on session_died / stream_error
- Reducer is no-op on new event types; lastEventId still advances

This is PR-A of the unified-renderer-layer follow-up series:
- PR-A (this commit) — event coverage + closed-enum schema
- PR-B — server-side timestamps + ordering refactor
- PR-C — multimodal content + tool preview taxonomy
- PR-D — render contract (toMarkdown / toHtml / toPlainText) + adapter
  conformance test framework
- PR-E — reducer state machine (subagent / progress / current tool /
  cancellation propagation)

See https://github.com/QwenLM/qwen-code/pull/4328#issuecomment-4494179724
for the full proposal.

Generated with AI

Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>

* feat(sdk/daemon-ui): server timestamps + event-id-based ordering (PR-B)

Closes the "时间定义不标准" gap surfaced in the PR #4328 review:
- Client-side `Date.now()` drifts across clients
- No daemon-authoritative timestamp propagated to UI
- Out-of-order replay events get fresher `state.now` than originals,
  breaking `createdAt` ordering

- `DaemonUiEventBase.serverTimestamp?: number` — daemon-authoritative
  wall-clock timestamp extracted from envelope.
- `DaemonTranscriptBlockBase.serverTimestamp?: number` + `clientReceivedAt: number`.
- `createdAt` preserved as `@deprecated` alias for `clientReceivedAt`
  (backward compat for code written before this PR).

`extractServerTimestamp` looks at three candidate envelope locations:

1. `event.serverTimestamp` (preferred when daemon adds it)
2. `event._meta.serverTimestamp` (Anthropic-style metadata convention)
3. `event.data._meta.serverTimestamp` (sessionUpdate nested location)

The SDK is ready to consume serverTimestamp WHEN daemon emits it, without
requiring a coordinated SDK release. Undefined when daemon doesn't emit
(current state) — graceful degradation to client-clock ordering.

`selectTranscriptBlocksOrderedByEventId(state)` — returns blocks sorted by:

1. `eventId` (daemon-monotonic SSE cursor) — primary key
2. `serverTimestamp` (daemon wall clock) — fallback for synthetic frames
3. `clientReceivedAt` (local clock) — last resort

Use this when displaying long sessions where event id 5 may arrive AFTER
event id 7 (typical in SSE replay-after-reconnect).

`formatBlockTimestamp(block, opts)` — formats the most authoritative
timestamp on a block using `Intl.DateTimeFormat`. Prefers
`serverTimestamp` over `clientReceivedAt` for cross-client consistency.
Accepts locale / timeZone / dateStyle / timeStyle.

Daemon needs to stamp `_meta.serverTimestamp` on every SSE envelope. This
SDK PR is ready to consume it the moment the daemon ships the field; no
coordination needed.

- serverTimestamp extraction from all three envelope locations
- Defaults undefined when envelope has none
- `selectTranscriptBlocksOrderedByEventId` sorts mixed-arrival events by
  eventId (replay scenario)
- `formatBlockTimestamp` prefers serverTimestamp; returns localized string

PR-B of the unified follow-up to PR #4328 (PR-A + PR-B + PR-C + PR-D +
PR-E in one branch).

Generated with AI

Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>

* feat(sdk/daemon-ui): reducer state machine — currentTool / approvalMode / cancellation propagation (PR-E)

Closes the "reducer state machine 设计缺漏" gap surfaced in the PR #4328 review:
- No `currentTool` — UI scans `blocks[]` to find the running tool
- No mirrored approval mode — UI walks events to badge "plan"/"yolo"
- Cancellation does not propagate — in-flight tool blocks stuck at
  'in_progress' forever when the parent prompt is cancelled

## State additions (sidechannel, no transcript blocks)

`DaemonTranscriptSidechannelState`:
- `currentToolCallId?: string` — toolCallId of the in-flight tool
- `approvalMode?: string` — mirrored from session.approval_mode.changed
- `toolProgress: Record<string, { ratio?, step? }>` — per-tool progress
  shape (daemon-side emission of `tool.progress` events pending)

## Reducer behavior

### `tool.update` events

`IN_FLIGHT_TOOL_STATUSES` = { pending, confirming, running, in_progress }
`TERMINAL_TOOL_STATUSES` = { completed, success, failed, error, canceled, cancelled }

- Tool enters in-flight: set `currentToolCallId = event.toolCallId`
- Tool enters terminal: clear `currentToolCallId` if it matches
- Unknown status (forward-compat): leave pointer untouched

This avoids the failure mode where a future daemon-emitted status like
`'paused'` would silently mark unknown states as either in-flight or
terminal incorrectly.

### `session.approval_mode.changed`

Mirror `event.next` onto `state.approvalMode`. Renderers can render a
mode badge ("plan" / "default" / "auto-edit" / "yolo") with a single
selector call, no event-stream walking.

### `assistant.done` with `reason === 'cancelled'`

`propagateCancellationToInFlightTools` walks every tool block whose
status is still in-flight and force-sets it to 'cancelled'. The daemon
does not guarantee terminal `tool_call_update` for every in-flight tool
when the parent prompt is cancelled, so this propagation prevents UI
spinners from spinning forever.

`currentToolCallId` is also cleared in the same call.

Non-cancellation `assistant.done` (e.g., `reason: 'end_turn'`) does NOT
propagate — in-flight tools remain in-flight until the daemon emits
their terminal update naturally.

## Selectors

- `selectCurrentTool(state)` — returns the running tool block, or undefined
- `selectApprovalMode(state)` — returns the mirrored approval mode
- `selectToolProgress(state, toolCallId)` — per-tool progress query

All exported from `@qwen-code/sdk/daemon`.

## Scope deliberately deferred

Subagent nesting (`parentBlockId` / `delegationId` / `DaemonSubagentTranscriptBlock`)
is NOT in this PR. The shape needs design discussion (how to project nested
events; whether to bake delegation tracking into transcript or sidechannel).
PR-D / PR-F follow-up.

## Test coverage (51/51 pass)

- currentToolCallId set on enter, cleared on terminal
- approvalMode mirrors changes
- Cancellation marks in-flight tools 'cancelled', leaves completed alone
- Unknown status does NOT clear currentToolCallId (forward-compat)
- Non-cancellation `assistant.done` does NOT propagate

## Roadmap

PR-E of the unified follow-up to PR #4328 (PR-A + PR-B + PR-E in this
branch; PR-C / PR-D pending).

Generated with AI

Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>

* feat(sdk/daemon-ui): tool preview taxonomy + multimodal content extraction (PR-C)

Closes two related gaps surfaced in the PR #4328 review:
- `DaemonToolPreview` had only 4 kinds — UI fell back to `key_value` /
  `generic` for tools that deserved structured display
- `getTextContent` silently dropped non-text content (image / audio /
  resource), so multimodal conversations vanished from the UI

`DaemonToolPreview` extends from 4 to 8 variants:

- `file_diff` — `{ path, oldText?, newText?, patch? }` — file edit tools
  (Anthropic-style `oldText/newText`, aider-style `patch`, write-style
  `newText` alone)
- `file_read` — `{ path, range?: [start, end] }` — file read tools, with
  range extracted from `lineRange` tuple OR `offset/limit` pair
- `web_fetch` — `{ url, method? }` — HTTP fetch tools (requires URL
  with scheme to avoid false positives on relative paths)
- `mcp_invocation` — `{ serverId, toolName, argsSummary? }` — MCP server
  tool calls, identified via `mcp__<server>__<tool>` naming convention
  (same heuristic as PR-A `DaemonUiToolUpdateEvent.provenance`)

Detector order matters — MCP wins first (most specific), then file_diff,
file_read, web_fetch, then the existing command / key_value fallbacks.

New helper `extractContentPart(value): DaemonUiContentPart | undefined`
returns a discriminated union:

```ts
type DaemonUiContentPart =
  | { kind: 'text'; text: string }
  | { kind: 'image'; mediaType: string; source: { url?, data? } }
  | { kind: 'audio'; mediaType: string; source: { url?, data? } }
  | { kind: 'resource'; uri: string; mediaType?, description? };
```

The existing `getTextContent` is preserved for backward compat. Renderers
that need to surface non-text content (web UI thumbnails, IDE attachment
chips) now have a typed shape to consume.

- Wiring `extractContentPart` into the normalizer / reducer so text
  blocks accumulate `parts: DaemonUiContentPart[]` alongside `text`
  (additive shape change requires render contract coordination — PR-D).
- 5 additional tool preview kinds (image_generation / code_block /
  tabular / subagent_delegation / search) — useful but not urgent;
  current 8 kinds cover the typical agent flows.

- file_diff detection from Anthropic / aider / write shapes
- file_read with lineRange tuple AND offset+limit pair
- web_fetch with method, REJECTS relative paths (no scheme)
- mcp_invocation with serverId + toolName extraction
- Detector priority: MCP wins over file_diff on conflicting shapes
- extractContentPart for text / image (url) / audio (data) / resource
- Unknown content type returns undefined (skip rather than synthesize)
- Image without source returns undefined (defensive)

PR-C of the unified follow-up to PR #4328 (PR-A + PR-B + PR-E + PR-C in
this branch; PR-D render contract pending).

Generated with AI

Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>

* feat(sdk/daemon-ui): render contract — markdown / HTML / plain text helpers (PR-D)

Closes the "render 契约只覆盖 terminal" gap surfaced in the PR #4328 review:

> PR ships `daemonUiEventToTerminalText` for terminal. Web/IDE/channel
> adapters each roll their own projection. No shared contract → adapter
> divergence is inevitable.

## New helpers

```ts
daemonBlockToMarkdown(block, opts?): string  // GFM-compatible
daemonBlockToHtml(block, opts?): string      // conservatively escaped HTML
daemonBlockToPlainText(block, opts?): string // for copy-paste / logs
daemonToolPreviewToMarkdown(preview, opts?): string
```

All three respect the same `kind` discrimination so adapters can switch
between them without touching call sites.

## Per-kind projection

For each `DaemonTranscriptBlock['kind']`:

- `user` / `assistant` / `thought` — plain text with role labels
- `tool` — header with toolName + structured preview + status badge
- `shell` — fenced code block, stream-discriminated (stdout vs stderr)
- `permission` — title + options list + resolved/pending indicator
- `status` / `debug` / `error` — semantic class / role (error → role=alert)

For each `DaemonToolPreview['kind']`:

- `ask_user_question` — question + options as bullet list
- `command` — fenced bash with optional cwd comment
- `file_diff` — unified diff in fenced code block (oldText/newText OR patch)
- `file_read` — `path (lines N-M)` line
- `web_fetch` — `METHOD url` line
- `mcp_invocation` — `serverId::toolName` with args summary
- `key_value` — bullet list
- `generic` — emphasized summary

## Security

- Default HTML sanitizer escapes `<`, `>`, `&`, `"`, `'` and FIRST strips
  ANSI/control sequences via `sanitizeTerminalText` (defense against
  agent-emitted escape codes in HTML output).
- Custom sanitizer hook for consumers wanting markdown→HTML pipelines
  (markdown-it + DOMPurify, etc.).
- `sanitizeUrls` option strips token-like query params (`token=`, `key=`,
  `x-amz-`, etc.) from URLs in `web_fetch` previews.
- `maxFieldLength` truncation defaults 8192, prevents pathological
  rendering on huge content.

## Adapter conformance (out of scope for this commit)

The conformance test framework (fixture corpus + `runAdapterConformanceSuite`)
mentioned in PR-D scope is deferred to a follow-up. The render helpers
here are the precondition — once stable, the conformance framework can
use them as the reference projection.

## Test coverage (77/77 pass)

- All 9 block kinds render in markdown (verified for user/assistant/tool/
  shell/permission/error specifically)
- file_diff renders as unified diff with old/new lines
- mcp_invocation renders as `server::tool` format
- HTML escapes XSS (`<script>` → `&lt;script&gt;`)
- HTML strips terminal escape sequences before escaping
- Error blocks emit `role="alert"` for screen readers
- plain text drops markdown delimiters
- maxFieldLength truncates with ellipsis
- sanitizeUrls strips token query params
- Custom sanitizer hook works

## Roadmap

PR-D of the unified follow-up to PR #4328 — completes the 5-PR series
(A: event coverage, B: time schema, E: state machine, C: tool preview +
content extraction, D: render contract).

Generated with AI

Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>

* feat(sdk/daemon-ui): 5 additional tool preview kinds — taxonomy complete (PR-F)

Closes the "5 additional preview kinds" item in PR #4353's TODO §A
(SDK-only work).

## New preview kinds (8 → 13)

- `code_block` — `{ language?, code, origin? }` — REPL / formatter /
  generator output, fenced as `\`\`\`<language>` in markdown
- `search` — `{ query, resultCount?, top? }` — grep / ripgrep / find /
  glob results with up to 5 top hits
- `tabular` — `{ columns, rows, totalRows? }` — structured table output
  (50-row cap with `totalRows` truncation indicator); supports both
  `columns: string[] + rows: unknown[][]` explicit shape and legacy
  `data: Array<Record<>>` shape (auto-infers columns from first row)
- `image_generation` — `{ prompt, thumbnailUrl?, model? }` — dall-e /
  diffusion / imagen / flux / sora style tools
- `subagent_delegation` — `{ agentName, task, parentDelegationId? }` —
  Anthropic-style Task tool and similar sub-agent dispatchers

## Detector priority

Order matters — most specific wins. New detectors slot in between
`mcp_invocation` and `file_diff`:

```
mcp_invocation > subagent_delegation > search > image_generation
  > file_diff > file_read > web_fetch > code_block > tabular
  > command > key_value > generic
```

Rationale: subagent / search / image generation are most discriminable
(distinct toolName patterns); file ops next; code_block / tabular last
because their shapes (`code:`, `columns:`) can appear in other tools.

## Render projections

Both `daemonToolPreviewToMarkdown` and the plain-text rendering paths
extended with cases for all 5 new kinds:

- code_block: fenced markdown code block with language tag
- search: bold header + GFM bullet list of top results
- tabular: GFM pipe table with header / separator / body / truncation hint
- image_generation: bold header + blockquoted prompt + embedded markdown
  image (URL sanitization respected via `sanitizeUrls` opt)
- subagent_delegation: bold delegate-arrow header + blockquoted task +
  optional parent delegation reference

## Test coverage (91/91 pass, +14 new)

- Each detector with positive case
- Detector priority verified: subagent_delegation wins over file_diff
  when toolName='Task' has both subagent + file-edit fields
- Tabular row cap (50) + totalRows stamping for truncated data
- Legacy data: Array<Record<>> auto-column inference
- Each render projection with structural assertions (markdown table
  format, image embed, bullet lists)

## Roadmap

PR-F of the unified follow-up to PR #4328. Brings the preview taxonomy
to 13 kinds covering: file ops (3), web (1), code/data (2), media (1),
agent control (2 — ask_user_question + subagent_delegation), MCP (1),
search (1), generic fallbacks (2).

Generated with AI

Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>

* feat(sdk/daemon-ui): adapter conformance framework + fixture corpus (PR-G)

Closes the "Adapter conformance test framework" item in PR #4353's TODO §A.
Lets any daemon-ui adapter (TUI / web / IDE / channel / mobile) validate
that it projects a fixed corpus of daemon SSE event streams to the same
semantic shape — catches projection drift before it reaches users.

## API surface

```ts
interface DaemonUiAdapterUnderTest {
  reduce(events: readonly DaemonUiEvent[]): unknown;
  renderToText(state: unknown): string;
}

interface DaemonUiConformanceFixture {
  name: string;
  description: string;
  envelopes: DaemonEvent[];           // raw daemon envelopes
  expectedContains: string[];          // phrases the rendered text MUST contain
  expectedAbsent?: string[];           // phrases that MUST NOT appear
  normalizeOptions?: { ... };          // forward-compat normalize opts
}

runAdapterConformanceSuite(adapter, opts?): ConformanceSuiteResult
DAEMON_UI_CONFORMANCE_FIXTURES: ReadonlyArray<DaemonUiConformanceFixture>
```

## Design

**Format-agnostic assertion**: adapters can render to ANSI / HTML /
markdown / JSX — the framework only inspects plain text via
`renderToText`. Catches semantic divergence (missing user message,
wrong tool status, leaked secret) without forcing identical formatting.

**Embedded fixture corpus** (no fs reads — works in browser bundle):
- `simple-chat` — user/assistant streaming flow
- `tool-call-lifecycle` — running → completed transition
- `file-edit-diff` — file_diff preview surfacing
- `mcp-invocation` — MCP serverId/toolName extraction via heuristic
- `permission-lifecycle` — request + resolved with outcome
- `mcp-budget-warning` — Wave 3 event (adapter must observe but rendering
  is its choice)
- `cancellation-propagates` — tool block status flows
- `malformed-payload-redaction` — uses `includeRawEvent: true` to verify
  even a debug-mode adapter doesn't leak `token: secret-do-not-leak`
- `auth-device-flow-success` — Wave 4 OAuth events
- `available-commands-typed-event` — PR-A upgrade from status text

Per-fixture `expectedContains` and `expectedAbsent` describe the
content contract independently of format.

## Suite result

```ts
{
  passed: number,
  failed: ConformanceFailure[],   // each carries missing + leaked + excerpt
  total: number,
}
```

**Does not throw** — caller asserts on `result.failed` so adapter test
suites can produce per-fixture diagnostics rather than a single opaque
exception.

## Filter options

`only` / `skip` allow targeted runs during adapter development:

```ts
runAdapterConformanceSuite(myAdapter, { only: ['simple-chat'] });
runAdapterConformanceSuite(myAdapter, { skip: ['cancellation-propagates'] });
```

## Test coverage (97/97 pass, +6 new)

- SDK reference adapter (reducer + markdown render) passes all fixtures
- SDK reference adapter (reducer + plainText render) also passes
- Buggy adapter (empty string output) fails every fixture with non-empty
  `expectedContains`
- Buggy adapter (raw event dump via JSON.stringify) caught by redaction
  fixture's `expectedAbsent`
- `only` filter narrows to a single fixture
- `skip` filter excludes named fixtures from the corpus

## Usage from adapter authors

```ts
// In your adapter's test file
import { runAdapterConformanceSuite } from '@qwen-code/sdk/daemon';
import { reduceForTui, renderTuiState } from './my-tui-adapter';

it('TUI adapter conforms to daemon UI corpus', () => {
  const result = runAdapterConformanceSuite({
    reduce: reduceForTui,
    renderToText: renderTuiState,
  });
  expect(result.failed).toEqual([]);
});
```

## Roadmap

PR-G of the unified follow-up to PR #4328. The corpus is intentionally
small (10 fixtures) but extensible — adapter authors can submit new
fixtures via additions to `DAEMON_UI_CONFORMANCE_FIXTURES` to lock in
regression coverage for edge cases their adapter encountered.

Generated with AI

Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>

* feat(webui+sdk/daemon-ui): wire transcriptAdapter to SDK render contract (PR-H)

Closes the "WebUI transcriptAdapter migration" item in PR #4353's TODO §A.
Validates the PR-D render contract end-to-end on the real WebUI consumer.

`daemonTranscriptToUnifiedMessages(blocks, options?)` gains a new options
parameter:

```ts
interface DaemonTranscriptAdapterOptions {
  useMarkdown?: boolean;                  // default: false
  enrichToolDetailsWithPreview?: boolean; // default: false
}
```

Defaults preserve legacy behavior — existing callers see no change.

For `user` / `assistant` / `thought` blocks, content is projected via
SDK's `daemonBlockToMarkdown` instead of raw sanitized text. The WebUI's
markdown renderer (markdown-it) then gets:

- `**You**\n\n<content>` for user blocks (bold "You" label)
- Raw text for assistant blocks (markdown formatting in agent output
  passes through cleanly)
- `> *thought:* <text>` blockquote for thought blocks

For `tool` blocks, `rawOutput` is replaced with `daemonToolPreviewToMarkdown(block.preview)`.
This lets WebUI surfaces without per-preview-kind React components still
display:

- `file_diff` as a fenced unified diff
- `mcp_invocation` as `server::tool` with args summary
- `tabular` as GFM pipe table
- `search` as bullet list with match count
- `image_generation` as embedded markdown image
- `subagent_delegation` as delegate arrow + task quote

Renderers with per-kind components should leave this opt-out.

`packages/sdk-typescript/src/daemon/index.ts` was missing exports for
PR-D / PR-F / PR-G / PR-B / PR-E surface — WebUI's `@qwen-code/sdk/daemon`
import path uses the daemon root, not the ui/ sub-index. Added 15+
re-exports so consumers don't need to use the longer
`@qwen-code/sdk/daemon/ui/index.js` path.

Now exported from `@qwen-code/sdk/daemon` root:
- `daemonBlockToMarkdown` / `daemonBlockToHtml` / `daemonBlockToPlainText`
- `daemonToolPreviewToMarkdown`
- `extractContentPart` + `DaemonUiContentPart` type
- `formatBlockTimestamp` + `selectTranscriptBlocksOrderedByEventId`
- `selectCurrentTool` / `selectApprovalMode` / `selectToolProgress`
- `runAdapterConformanceSuite` + `DAEMON_UI_CONFORMANCE_FIXTURES`
- All associated types

`webui/src/daemon/transcriptAdapter.test.ts` mock blocks updated to include
`clientReceivedAt` (required field added in PR-B). Mechanical change —
every `createdAt: N` test fixture gets a matching `clientReceivedAt: N`.

- WebUI `npm run typecheck` — clean
- SDK `npm run typecheck` — clean
- SDK `vitest run test/unit/daemonUi.test.ts` — 97/97 pass
- WebUI transcriptAdapter test fixtures typecheck against updated
  DaemonTranscriptBlockBase schema

PR-H of the unified follow-up to PR #4328. Closes the WebUI migration
gap in TODO §A.

Generated with AI

Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>

* docs(daemon-ui): add developer guide + migration cookbook (PR-I)

Closes the final "Documentation" item in PR #4353's TODO §A. Brings the
unified daemon UI surface to ~95% SDK-side completion.

## Files added

- `docs/developers/daemon-ui/README.md` — full API reference
  - Three-layer model (normalizer → reducer → render helpers)
  - Quick start with idiomatic event-loop pattern
  - Event taxonomy (28+ types categorized: chat-stream / session-meta /
    workspace / auth device-flow)
  - Render contract cookbook (markdown / HTML / plainText)
  - Tool preview taxonomy (13 kinds with use cases)
  - State selectors (currentTool / approvalMode / toolProgress / ordering)
  - Cancellation propagation explanation
  - Time semantics (eventId > serverTimestamp > clientReceivedAt
    precedence)
  - Adapter conformance usage
  - ErrorKind dispatch pattern
  - Tool provenance dispatch pattern
  - Forward-compat principles

- `docs/developers/daemon-ui/MIGRATION.md` — adapter author migration
  cookbook
  - Step-by-step recommended adoption order (9 steps, value-ranked)
  - Before/after code examples for each step
  - Backward-compat checklist (everything is additive — no breaking
    changes)
  - Cross-references to PR-A through PR-H commits

## Roadmap

PR-I of the unified follow-up to PR #4328. Documentation-only — no
code changes; no tests affected.

Generated with AI

Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>

* fix(daemon-ui): address review feedback

* fix(daemon-ui): address review hardening feedback

* fix(daemon-ui): handle resync-required events

* feat(sdk/daemon-ui): consume daemon-side subagent nesting context (PR-K)

Closes the SDK-side gap for §B1 in PR #4353's TODO list. PR-E originally
deferred subagent nesting because daemon-side parent-context wasn't yet
stamped on tool_call events. After the rebase onto current
daemon_mode_b_main, source verification confirms the daemon now emits
`tool_call._meta.parentToolCallId` + `tool_call._meta.subagentType` via
`SubAgentTracker.getSubagentMeta()` (core), so the SDK side is unblocked.

## Schema additions (additive, forward-compat-safe)

`DaemonUiToolUpdateEvent`:
  - parentToolCallId?: string  — toolCallId of the parent Task / delegation
  - subagentType?: string      — sub-agent type label (e.g. 'code-reviewer')

`DaemonToolTranscriptBlock`:
  - parentToolCallId?: string  — mirror of event field
  - subagentType?: string      — mirror of event field
  - parentBlockId?: string     — pre-resolved by reducer when parent already
                                 in state, so renderers don't re-correlate

## Normalizer wiring

`normalizeToolUpdate` checks both top-level and `_meta` for parentToolCallId
+ subagentType (fallback chain mirrors how provenance/serverId are read).
Top-level tool calls without sub-agent context omit the fields cleanly.

## Reducer behavior

- New tool block: resolves `parentBlockId` from `toolBlockByCallId` at
  create time. Out-of-order arrival (child before parent) leaves
  `parentBlockId` undefined — selectors fall back to `parentToolCallId`
  lookup.
- Existing tool block update: adopts parent context if not yet
  correlated, never overwrites established correlation (handles the
  flow where SubAgentTracker activates after the initial tool_call).

## New public selectors

- selectSubagentChildBlocks(state, parentToolCallId): returns the
  array of tool blocks invoked inside a given parent delegation
- isSubagentChildBlock(block): type guard for "this tool block came
  from a sub-agent"

Both exported from @qwen-code/sdk/daemon root + ui/index.

## Forward-compat properties

- Top-level tool calls (no sub-agent) work identically as before
- Trimmed parent blocks: child fallback to undefined parentBlockId
- Daemon emits both fields together; SDK reads independently to tolerate
  partial future stamping

## Test coverage (129/129 pass, +5 new tests)

- Extract parentToolCallId + subagentType from `_meta`
- Top-level tool calls have undefined parent fields (forward-compat)
- Reducer correlates parentBlockId at create time
- Reducer adopts parent context on later update (out-of-order arrival)
- isSubagentChildBlock discriminator

## Roadmap

PR-K of the unified follow-up to PR #4353. Closes §B1 (subagent nesting)
in the TODO declaration; daemon-side already shipped on
`daemon_mode_b_main` via SubAgentTracker (core).

Remaining TODO §B / §D items still depend on further daemon/Core work:
- §B2 `tool.progress` event type (daemon emit pending)
- §D MessageEmitter multimodal echo + HistoryReplayer inlineData/fileData
  (core change pending)

Generated with AI

Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>

* fix(daemon-ui): PR-K self-review hardening — back-fill / trim / self-ref / docs

Multi-round self-review of PR-K (d8375fe46) surfaced two real bugs, a
few defensive gaps, and missing docs/fixture coverage. All addressed
in one commit.

## Bugs fixed

### Bug 1 — `parentBlockId` never back-filled for out-of-order arrival

Original PR-K resolved `parentBlockId` only at child create time, which
broke this flow:

  1. Child arrives WITH parent stamp → block created with
     `parentToolCallId` set, `parentBlockId` undefined (parent not in
     state yet)
  2. Parent arrives later → block created, `toolBlockByCallId` indexed
  3. Subsequent child updates: existing-block branch only ran the
     back-fill inside `!existing.parentToolCallId`, which is false (we
     already adopted the stamp in step 1). `parentBlockId` stayed
     undefined forever.

Fix: separate the two correlations.
  - existing-block update: independently back-fill `parentBlockId`
    whenever `parentToolCallId` is set and `parentBlockId` is missing
  - new-block create: scan existing children whose `parentToolCallId`
    matches the new block's `toolCallId` and back-fill their
    `parentBlockId`. Cheap O(n) over current blocks.

### Bug 2 — dangling `parentBlockId` after trim

`trimTranscriptState` reset `toolBlockByCallId[id]` to the trimmed
sentinel for evicted blocks but did NOT walk surviving children to
null their `parentBlockId` references. Renderers walking
`blockIndexById.get(parentBlockId)` would get undefined, with no
"why" signal.

Fix: post-trim, walk remaining tool blocks; if `parentBlockId`
references an id not in `keptIds`, null it. `parentToolCallId` stays
(survives trimming so selector-keyed queries still work).

## Defensive hardening

- **Self-reference guard** (normalizer): drop
  `parentToolCallId === toolCallId` before it reaches the reducer.
  Daemon should never emit this, but defending costs nothing.
- **Selector docstring**: clarify `selectSubagentChildBlocks` returns
  **direct** children only; document cycle / depth-cap responsibility
  for renderers walking up the chain.
- **Cosmetic**: remove redundant `as DaemonToolTranscriptBlock` cast
  in `isSubagentChildBlock` (TypeScript already narrows after
  `block.kind === 'tool'` on the discriminated union).
- **Alphabetical**: move `isSubagentChildBlock` re-export to correct
  position in both `daemon/index.ts` and `daemon/ui/index.ts`.

## Docs + conformance gaps closed

- `README.md` — new "Sub-agent nesting (PR-K)" section with full
  reducer behavior, out-of-order handling note, recursive walk example,
  cycle-defense note.
- `MIGRATION.md` — new step 8a with before/after for nested rendering.
- `conformance.ts` — new `subagent-nesting` fixture covering parent +
  nested child via `tool_call._meta`. Markdown-safe phrases chosen
  (markdown escapes `-` so titles cannot be substring-matched as-is).

## Test coverage (+5 tests, 134/134 pass)

- Self-reference dropped in normalizer
- Back-fill on out-of-order parent arrival (child first, parent after)
- Back-fill on later child update when parent now exists
- Dangling `parentBlockId` nulled after parent trimmed
- New `subagent-nesting` conformance fixture passes SDK reference adapter

## Side-effect verification

Verified no regressions:
- Cancellation propagation still cancels parent + children together
  (iterates `toolBlockByCallId`, which includes both)
- Render contract unchanged (`daemonBlockToMarkdown` etc. project per
  block, no nested awareness required)
- No serializer to update
- `selectTranscriptBlocksOrderedByEventId` unaffected (parent-agnostic)

Generated with AI

Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>

* fix(daemon-ui): permission block trim contract — wenshao review

Addresses both items from wenshao's review on PR #4353:

## Critical — resolvePermissionBlock missing TRIMMED guard

The sibling `upsertPermissionBlock` (transcript.ts:544) correctly returns
early when `existingId === TRIMMED_PERMISSION_BLOCK_ID`, but
`resolvePermissionBlock` (transcript.ts:581) had no such guard. When
`maxBlocks` trimming evicted a pending permission request, a subsequent
`permission.resolved` event would:

1. Fail the `getWritableBlockById` lookup (sentinel is not a real block id)
2. Fall through and create a brand-new orphan resolution block

This wasted a block slot, accelerated further trimming, and silently
broke the trimmed-block contract that the request-side guard establishes.

Fix: mirror the request-side guard. Read the index entry up front,
return early on the sentinel.

## Suggestion — permissionBlockByRequestId grows unboundedly

`trimTranscriptState` writes `TRIMMED_PERMISSION_BLOCK_ID` for evicted
permission requests but never deletes those entries. Unlike the tool
side (which calls `pruneTrimmedToolIndexes` post-trim), the permission
index grew without bound in long sessions.

Fix: add `pruneTrimmedPermissionIndexes` analogous to the tool-side
helper. Caps the sentinel set at `maxBlocks` entries; older entries are
deleted (any later resolution event still drops cleanly via the new
Critical guard).

## Tests

- Updated existing `keeps orphan permission resolutions visible after
  request trimming` test to encode the corrected contract (drops silently
  instead of creating an orphan). Test rename: "drops resolution for
  trimmed permission requests (wenshao Critical)".
- New `Suggestion: pruneTrimmedPermissionIndexes caps the trimmed
  sentinel set` test verifies the cap.

Total: 136/136 tests pass, SDK + WebUI typecheck green.

## Side-effect verification

- `upsertPermissionBlock` already had the equivalent guard — no
  asymmetry remains.
- `pruneTrimmedPermissionIndexes` only touches entries holding the
  sentinel; live permission blocks are unaffected.
- Selectors over `state.blocks` (e.g. `selectPendingPermissionBlocks`)
  iterate the block array, not the index — unaffected by cap.

Generated with AI

Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>

* fix(daemon-ui): address wenshao + doudouOUC inline reviews (2026-05-23)

Addresses the 13 inline review comments from wenshao (6) and doudouOUC
(7, one overlap) on the 2026-05-23 review round.

## Critical / Important

### sanitizeUrls not threaded through HTML preview path (doudouOUC)

`daemonBlockToHtml` for tool blocks called `daemonToolPreviewToPlainText`
which didn't accept `opts` — when callers set `sanitizeUrls: true`, the
markdown path stripped auth tokens but the HTML path leaked them into
the DOM. Now: helper accepts opts, threads through `web_fetch.url` and
`image_generation.thumbnailUrl`.

### enrichToolDetailsWithPreview overwrote rawOutput (doudouOUC)

The webui adapter replaced structured `rawOutput` with a markdown
summary string when `enrichDetails: true`. Downstream `ToolCallData`
consumers may branch on the shape (object vs string) and break. Plus
the actual tool output was silently dropped.

Fix: keep `rawOutput` verbatim, surface markdown via a new optional
`previewMarkdown` field added to `ToolCallData`.

### transcriptBlockToTerminalText zero test coverage (wenshao)

Added 12 tests covering each `switch` branch (user / assistant / thought
/ tool / shell stdout+stderr / permission unresolved+resolved / status /
debug / error) plus the unknown-kind degradation path. Verified
`assertNever` returns a graceful error line (does NOT throw) — wenshao's
reviewer was slightly wrong on the throw claim but coverage gap was
real.

### selectTranscriptBlocksOrderedByEventId no memoization (wenshao)

Selector was called from React `useSyncExternalStore` and re-sorted on
every dispatch — including sidechannel-only events that don't touch
blocks. Added WeakMap cache keyed on `state.blocks` reference; the
reducer preserves the same array reference for non-block-mutating
events, so the cache hits across renders.

### selectSubagentChildBlocks O(n) per call (wenshao)

Naive `state.blocks.filter()` was O(n) per call; rendering a tree with
m parents made it O(n*m). Built a memoized reverse index keyed on
`state.blocks` reference (WeakMap of parentToolCallId →
DaemonToolTranscriptBlock[]). Each lookup now O(1) after first call.

### Test file TS errors at root tsc (wenshao)

Fixed multiple TS errors in `daemonUi.test.ts` flagged by root
`tsc --noEmit`:
- Added `DaemonTranscriptState` + `DaemonUiEvent` imports
- `block.content` access via `as Array<Record<string, unknown>>` cast
- `delete` on globalThis property via narrower interface cast
- `debug?.text` via `DaemonUiEvent & { text: string }` narrowing (Extract on
  union with `'status' | 'debug'` literal would resolve to never)
- 6 occurrences of index-signature access via bracket notation
- `raw: null` added to 3 `DaemonUiPermissionOption` literals (required field)
- Explicit type annotations on conformance-suite `renderToText` params

Note: `webui/src/daemon/transcriptAdapter.test.ts` shows residual
"clientReceivedAt does not exist" errors at root tsc, but this is
environmental — the resolution trace shows `@qwen-code/sdk/daemon`
crossing into a sibling worktree's stale dist via shared workspace
node_modules. In a single-worktree CI checkout this resolves cleanly.

## Suggestions (cleanups)

### Hoist asDaemonErrorKind double-eval (doudouOUC)

`session_died` + `stream_error` cases each computed `asDaemonErrorKind`
twice in the conditional spread (predicate + value). Hoisted to const,
no functional change.

### renderToolHeader bypassed opts (doudouOUC)

Forwarded `opts` so `maxFieldLength` is honored for tool title /
toolName / toolKind.

### isSensitiveKey duplicates (doudouOUC)

Removed duplicate `endsWith('accesskey')` / `endsWith('secretkey')`
checks and the redundant exact-match `privatekey` (already covered by
`endsWith`).

### propagateCancellationToInFlightTools iterated trimmed (wenshao)

Filter `TRIMMED_TOOL_BLOCK_ID` sentinels up front. Avoids redundant
index dereferences in long sessions with many historical tools.

### toolProgress shallow clone (doudouOUC + wenshao)

`cloneTranscriptState` outer `...state` spread shared inner
`{ ratio?, step? }` references between snapshots. Once `tool.progress`
event handlers start mutating in place, the prior snapshot would leak.
Deep-clone the inner records now (cost bounded by in-flight tools,
small).

### isDeviceFlowErrorKind closed set (wenshao + doudouOUC)

Both reviewers suggested strict validation. We INTENTIONALLY kept
lenient pass-through — the public type
`DaemonAuthDeviceFlowSdkErrorKind` explicitly includes `(string & {})`
as a forward-compat escape hatch (existing test `keeps future
auth_device_flow_failed errorKind values observable` enforces this).
Now expose `KNOWN_DEVICE_FLOW_ERROR_KINDS` as documentation and
explain the design in the JSDoc.

## Validation

| | |
|---|---|
| SDK tests | 148/148 pass (+12 terminal coverage + assorted hardening) |
| SDK typecheck | clean |
| WebUI typecheck | clean |

## Side-effect verification

- WeakMap memos invalidate correctly: reducer creates a fresh
  `state.blocks` reference only on block-mutating events. Sidechannel
  events reuse t…
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants