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

packages/acp-bridge/src/bridge.ts

@@ -2114,7 +2114,46 @@ export function createHttpAcpBridge(opts: BridgeOptions): HttpAcpBridge {
             : ''),
       );
       if (defaultEntry === entry) defaultEntry = undefined;
-      const ci = channelInfo;
+      // #4325 fix: resolve the channel via `channelInfoForEntry(entry)`
+      // (search `aliveChannels` for the entry's actual channel) instead
+      // of the module-scoped `channelInfo` (the CURRENT attach target).
+      // The two diverge during the channel-overlap window — A dying,
+      // B freshly spawned as `channelInfo` — where capturing
+      // `channelInfo` would (1) skip the `sessionIds.delete()` since
+      // `B.channel !== entry.channel`, leaving A's `sessionIds` set
+      // pinned past the close, and (2) call `markSessionClosed` on
+      // **B**'s client instead of **A**'s, evaluating B's kill
+      // condition with stale assumptions about its session count.
+      // Other session methods in this file already use the helper
+      // (`setSessionApprovalMode`, `requestSessionStatus`); this
+      // brings closeSession in line.
+      //
+      // HAZARD(#4325): the regression test for this fix
+      // (`httpAcpBridge.test.ts` ~L6421) is single-channel smoke ONLY —
+      // a revert that reintroduces the module-scoped `channelInfo`
+      // capture WILL NOT fail any existing test, because the
+      // overlap-race state isn't deterministically constructable
+      // without factory-internal hooks. Code-review-time visibility
+      // of the `channelInfoForEntry(entry)` call here is the primary
+      // defense against accidental revert. Don't refactor away the
+      // helper call without first landing the deterministic overlap
+      // test (deferred follow-up).
+      const ci = channelInfoForEntry(entry);
+      if (!ci) {
+        // Diagnostic visibility (#4334 wenshao review DWrbr): when the
+        // entry's channel has already been torn down out-of-band, the
+        // cleanup branches below all short-circuit silently. The
+        // "closing session" log at the top of this method fires
+        // regardless, so the close *call* is visible — but the fact
+        // that channel-side bookkeeping was skipped is not. Sibling
+        // methods (e.g. `requestSessionStatus`) surface this as
+        // `SessionNotFoundError`; `closeSession` is intentionally
+        // idempotent so we just log instead of throwing.
+        writeStderrLine(
+          `qwen serve: closeSession channelInfoForEntry returned undefined ` +
+            `for session ${JSON.stringify(sessionId)} — channel cleanup skipped (entry's channel already torn down)`,
+        );
+      }
       if (ci && ci.channel === entry.channel) {
         ci.sessionIds.delete(sessionId);
       }
@@ -3159,7 +3198,29 @@ export function createHttpAcpBridge(opts: BridgeOptions): HttpAcpBridge {
       // Detach from the channel. The channel dies only when its LAST
       // session leaves — other sessions on the same channel keep
       // running.
-      const ci = channelInfo;
+      //
+      // #4325 fix: same channel-overlap fix as in `closeSession` above.
+      // `channelInfoForEntry(entry)` returns the entry's actual
+      // channel rather than the module-scoped `channelInfo` (current
+      // attach target), preventing the "kill operates on the freshly-
+      // spawned channel B instead of the dying channel A" cascade
+      // during the overlap window.
+      //
+      // HAZARD(#4325): see the matching block in `closeSession`. The
+      // regression test for this fix is single-channel smoke and
+      // would NOT fail if this line reverts to `channelInfo`. Keep
+      // `channelInfoForEntry(entry)` until the deterministic overlap
+      // test lands.
+      const ci = channelInfoForEntry(entry);
+      if (!ci) {
+        // Same diagnostic as `closeSession` (#4334 wenshao review
+        // DWrbr) — when the entry's channel is already gone, the
+        // cleanup below short-circuits silently; surface that.
+        writeStderrLine(
+          `qwen serve: killSession channelInfoForEntry returned undefined ` +
+            `for session ${JSON.stringify(sessionId)} — channel cleanup skipped (entry's channel already torn down)`,
+        );
+      }
       if (ci && ci.channel === entry.channel) {
         ci.sessionIds.delete(sessionId);
       }

packages/acp-bridge/src/bridgeFileSystem.ts

@@ -59,21 +59,39 @@ export interface BridgeFileSystem {
   readText(params: ReadTextFileRequest): Promise<ReadTextFileResponse>;
 
   /**
-   * Atomically replace `params.path` with `params.content`. Adapter
-   * MUST preserve mode/owner where possible, resolve symlinks
-   * before write, and reject paths outside the bound workspace.
-   * Returns the ACP-shaped empty response on success.
+   * Atomically replace `params.path` with `params.content`. Returns
+   * the ACP-shaped empty response on success; throws an `FsError`
+   * (classified downstream by `mapDomainErrorToErrorKind`) on
+   * boundary, trust, or I/O failure.
    *
-   * Adapter MUST replicate the inline proxy's defenses:
-   *   - Write-then-rename atomicity (avoid truncation on
-   *     SIGKILL / OOM mid-write).
-   *   - Dangling-symlink → write through to the symlink's intended
-   *     target (don't replace the symlink with a regular file).
-   *   - Preserve target mode bits + owner/group where the daemon
-   *     has permission.
-   *   - Default to `0o600` for newly-created files (NOT umask
-   *     defaults — the inline `mode` arg bypasses umask for
-   *     atomicity of secret writes).
+   * Adapter MUST provide:
+   *   - **Write-then-rename atomicity** — a SIGKILL / OOM mid-write
+   *     does NOT leave the target truncated.
+   *   - **Target mode preservation** — editing a `0o600` secret
+   *     keeps it at `0o600`; an executable `+x` bit is retained.
+   *   - **`0o600` default for new files** — NOT umask defaults (the
+   *     write syscall's `mode` arg bypasses umask). This is the
+   *     security posture for agent-driven writes where the agent's
+   *     intent about the file's audience is unknown.
+   *   - **Symlink rejection** — paths whose target is a symlink
+   *     surface `symlink_escape`. This is a **divergence from the
+   *     pre-F1 inline `BridgeClient.writeTextFile` proxy** which
+   *     resolved symlinks and wrote through to their target;
+   *     production now matches the more conservative PR 18 +
+   *     HTTP `POST /file` posture (PR 20). Agents that previously
+   *     relied on writing through symlinked dotfiles will need
+   *     to address the resolved path directly.
+   *   - **Workspace boundary enforcement** — paths outside the
+   *     bound workspace surface `path_outside_workspace`.
+   *
+   * Owner/group preservation is best-effort and platform-dependent
+   * (POSIX `chown` requires root for cross-user changes; Windows
+   * lacks the concept entirely). The contract does NOT require it.
+   *
+   * The serve-side adapter satisfies this via
+   * `WorkspaceFileSystem.writeTextOverwrite` — the PR 18 primitive
+   * that does atomic tmp+rename with mode preservation + `0o600`
+   * default + symlink reject inside a per-path lock.
    */
   writeText(params: WriteTextFileRequest): Promise<WriteTextFileResponse>;
 }