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

[doudouOUC]

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 / ReadTextFileRequest → WorkspaceFileSystem.resolve → writeTextOverwrite / 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.ts — fsFactory 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.ts — closeSession + 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.writeText → wfs.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

• F1 PR: feat(acp-bridge): F1 — acp-bridge package self-sufficiency (#4175 mechanical lift + BridgeFileSystem seam) #4319 (merged 2026-05-19, commit 981bc7c7e)
• Tracker: proposal(serve): Mode B feature-priority roadmap toward v0.16 production-ready #4175 Wave 5 — F1 follow-up batch (this PR is the "fs adapter + channelInfo + write mode preservation" deliverable)
• Closes bug(acp-bridge): closeSession + killSession use module-scoped channelInfo instead of channelInfoForEntry(entry) #4325
• Copilot review thread resolved: discussion_r3268275710

🤖 Generated with Qwen Code

[github-actions]

📋 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

• Excellent documentation: Both the adapter and the bug(acp-bridge): closeSession + killSession use module-scoped channelInfo instead of channelInfoForEntry(entry) #4325 fix include detailed inline comments explaining the "why" behind the changes, including race condition scenarios and historical context.
• Strong test coverage: The new bridgeFileSystemAdapter.test.ts provides 10 comprehensive tests covering happy paths, trust gate enforcement, boundary rejection, line/limit windowing, and audit context wiring.
• Consistent patterns: The bug(acp-bridge): closeSession + killSession use module-scoped channelInfo instead of channelInfoForEntry(entry) #4325 fix correctly aligns closeSession and killSession with the pattern already used in setSessionApprovalMode and requestSessionStatus.
• Unified audit stream: Both HTTP fs routes and ACP fs now share the same fsFactory instance, ensuring consistent audit logging across surfaces.
• Backward compatibility preserved: Optional BridgeOptions.fileSystem and channelInfoForEntry returning identical results in the common case mean no breaking changes for existing consumers.

🎯 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]

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.

[wenshao]

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

[wenshao]

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/passwd → symlink_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.

[wenshao]

[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

[wenshao]

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

[wenshao]

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