test(acp-bridge): cover BridgeFileSystem injection seam + extract shared writeStderrLine (#4319 wenshao review)

Folds in wenshao review on #4319:

1. **[Critical]** zero test coverage for the F1 step 5 `BridgeFileSystem`
   delegation branches in `BridgeClient.writeTextFile` /
   `BridgeClient.readTextFile` and the factory's
   `opts.fileSystem` → constructor positional-arg forwarding.

   New `packages/acp-bridge/src/bridgeClient.test.ts` adds 6 tests
   covering:
   - writeTextFile delegates to injected fileSystem.writeText (inline
     proxy fully bypassed; `fakeFs.writeText` called with the original
     params; `readText` mock not invoked)
   - writeTextFile invalid-path call succeeds purely via the mock
     when fileSystem is injected (proof that the inline `fs.realpath`
     path doesn't run)
   - readTextFile delegates to injected fileSystem.readText
   - readTextFile propagates injection errors to the caller
   - inline-fallback regression guard: write actually hits disk via
     the inline proxy when fileSystem is omitted (real tmp file
     round-trip)
   - same for read

   Why these matter: the 7-arg `BridgeClient` constructor places
   `fileSystem` at the tail as optional. A reordering — or dropping
   the arg from `bridge.ts` factory's `new BridgeClient(..., opts.fileSystem)`
   call — would silently bypass the adapter in production and the
   inline `fs.writeFile` raw-path would run with no audit / trust /
   TOCTOU coverage. The delegation tests would catch that because
   the mock fileSystem would never be invoked.

2. **[Suggestion]** `writeStderrLine` was defined identically in
   `bridge.ts:117` and `bridgeClient.ts:30` (22 call sites across the
   two files). Both consumers live in the SAME `@qwen-code/acp-bridge`
   package, so the original "no reverse-dep on cli" justification
   doesn't apply within the package. Extracted to
   `packages/acp-bridge/src/internal/stderrLine.ts` — a single source
   of truth that future behavior changes (timestamp prefix, log
   level, structured field) can edit once. `internal/` subpath is
   intentionally not in `package.json`'s `exports`, keeping the
   helper package-private. `spawnChannel.ts` deliberately does NOT
   consume it (its stderr writes use `process.stderr.write(prefix +
   line + '\n')` directly because each line carries its own
   `[serve pid=… cwd=…]` line prefix).

- 6/6 new BridgeFileSystem-seam tests pass
- 50/50 acp-bridge total (44 existing + 6 new)
- 174/174 cli httpAcpBridge tests pass (no regression from refactor)
- typecheck + eslint clean

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

packages/acp-bridge/src/bridge.ts

@@ -67,6 +67,7 @@ import type {
 } from './bridgeTypes.js';
 import type { BridgeOptions } from './bridgeOptions.js';
 import { defaultSpawnChannelFactory } from './spawnChannel.js';
+import { writeStderrLine } from './internal/stderrLine.js';
 import {
   BridgeClient,
   MAX_RESOLVED_PERMISSION_RECORDS,
@@ -107,17 +108,6 @@ import {
  * route handlers don't need to change.
  */
 
-/**
- * Inline `writeStderrLine` (lifted from `cli/src/utils/stdioHelpers.ts` in
- * #4175 F1) so acp-bridge has no reverse dependency on `cli`. Behavior is
- * byte-identical to the cli helper: writes the message to stderr followed
- * by a newline, avoiding a double newline if the message already ends with
- * one.
- */
-function writeStderrLine(message: string): void {
-  process.stderr.write(message.endsWith('\n') ? message : `${message}\n`);
-}
-
 interface ChannelInfo {
   channel: AcpChannel;
   connection: ClientSideConnection;

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

@@ -0,0 +1,185 @@
+/**
+ * @license
+ * Copyright 2025 Qwen Team
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+/**
+ * Unit tests for the `BridgeFileSystem` injection seam introduced in
+ * #4175 PR F1 step 5. The wider 174-test `httpAcpBridge.test.ts` suite
+ * exercises BridgeClient end-to-end via the lifted factory, but none
+ * of those tests wire `fileSystem` — they all exercise the inline
+ * `fs.writeFile` / `fs.readFile` proxy. These tests close that gap
+ * (wenshao #4319 Critical fold-in): they directly assert that
+ *
+ *   1. when `fileSystem` is provided, both `writeTextFile` and
+ *      `readTextFile` delegate every call to it (and the inline
+ *      proxy is fully bypassed — no `fs.writeFile` syscall);
+ *   2. when `fileSystem` is omitted, the inline proxy runs and
+ *      reads / writes real disk (sanity check that the fallback
+ *      path the 14-arg constructor's positional slot opt-outs to
+ *      still works).
+ *
+ * Regression guard: the constructor takes 7 positional args; the
+ * 7th (`fileSystem`) is optional and at the tail. A subtle re-
+ * ordering (or dropping the arg from `bridge.ts:773` factory's
+ * `new BridgeClient(..., opts.fileSystem)` call) would silently
+ * bypass the adapter in production. Test #1 + #2 catch that
+ * because the mock fileSystem would never be called.
+ */
+
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
+import { promises as fsp } from 'node:fs';
+import * as os from 'node:os';
+import * as path from 'node:path';
+import type {
+  ReadTextFileRequest,
+  ReadTextFileResponse,
+  WriteTextFileRequest,
+  WriteTextFileResponse,
+} from '@agentclientprotocol/sdk';
+import { BridgeClient } from './bridgeClient.js';
+import type { BridgeFileSystem } from './bridgeFileSystem.js';
+
+/**
+ * Minimal-stub constructor for a `BridgeClient` whose only purpose is
+ * to exercise `writeTextFile` / `readTextFile`. The 6 callback args
+ * before `fileSystem` are filled with thrower-defaults so any test
+ * that accidentally hits the permission path (instead of the fs path)
+ * fails loudly instead of silently.
+ */
+function makeClient(fileSystem?: BridgeFileSystem): BridgeClient {
+  const noPermissionFlow = () => {
+    throw new Error('test: permission flow should not run in fs-path tests');
+  };
+  return new BridgeClient(
+    noPermissionFlow as never, // resolveEntry
+    noPermissionFlow as never, // resolvePendingRestoreEvents
+    noPermissionFlow, // registerPending
+    noPermissionFlow, // rollbackPending
+    0, // permissionTimeoutMs (disabled)
+    Infinity, // maxPendingPerSession (disabled)
+    fileSystem,
+  );
+}
+
+describe('BridgeClient — BridgeFileSystem injection seam (F1 step 5)', () => {
+  describe('writeTextFile', () => {
+    it('delegates to the injected fileSystem.writeText, bypassing the inline fs proxy', async () => {
+      const writeText = vi
+        .fn<(p: WriteTextFileRequest) => Promise<WriteTextFileResponse>>()
+        .mockResolvedValue({});
+      const readText =
+        vi.fn<(p: ReadTextFileRequest) => Promise<ReadTextFileResponse>>();
+      const fakeFs: BridgeFileSystem = { writeText, readText };
+
+      const client = makeClient(fakeFs);
+      const params: WriteTextFileRequest = {
+        path: '/this/path/never/touches/disk',
+        content: 'injected-content',
+        sessionId: 'sess:test',
+      };
+
+      const response = await client.writeTextFile(params);
+
+      expect(response).toEqual({});
+      expect(writeText).toHaveBeenCalledTimes(1);
+      expect(writeText).toHaveBeenCalledWith(params);
+      expect(readText).not.toHaveBeenCalled();
+    });
+
+    it('does NOT touch real fs when delegating — invalid path never reaches fs.realpath', async () => {
+      const writeText = vi
+        .fn<(p: WriteTextFileRequest) => Promise<WriteTextFileResponse>>()
+        .mockResolvedValue({});
+      const fakeFs: BridgeFileSystem = {
+        writeText,
+        readText: vi.fn(),
+      };
+      const client = makeClient(fakeFs);
+
+      // A path the inline proxy would choke on (no parent, no read
+      // permission, no existing target). Delegation skips realpath,
+      // so the call succeeds purely on the mock's resolve.
+      await client.writeTextFile({
+        path: '/proc/no-such-file',
+        content: '',
+        sessionId: 'sess:test',
+      });
+
+      expect(writeText).toHaveBeenCalled();
+    });
+  });
+
+  describe('readTextFile', () => {
+    it('delegates to the injected fileSystem.readText, bypassing the inline fs proxy', async () => {
+      const writeText =
+        vi.fn<(p: WriteTextFileRequest) => Promise<WriteTextFileResponse>>();
+      const readText = vi
+        .fn<(p: ReadTextFileRequest) => Promise<ReadTextFileResponse>>()
+        .mockResolvedValue({ content: 'injected-content' });
+      const fakeFs: BridgeFileSystem = { writeText, readText };
+
+      const client = makeClient(fakeFs);
+      const params: ReadTextFileRequest = {
+        path: '/this/path/never/touches/disk',
+        sessionId: 'sess:test',
+      };
+
+      const response = await client.readTextFile(params);
+
+      expect(response).toEqual({ content: 'injected-content' });
+      expect(readText).toHaveBeenCalledTimes(1);
+      expect(readText).toHaveBeenCalledWith(params);
+      expect(writeText).not.toHaveBeenCalled();
+    });
+
+    it('propagates fileSystem.readText errors to the caller', async () => {
+      const readText = vi.fn(async (): Promise<ReadTextFileResponse> => {
+        throw new Error('adapter-rejected');
+      });
+      const client = makeClient({ writeText: vi.fn(), readText });
+
+      await expect(
+        client.readTextFile({ path: '/x', sessionId: 'sess:test' }),
+      ).rejects.toThrow('adapter-rejected');
+    });
+  });
+
+  describe('inline fallback when fileSystem is omitted (regression guard)', () => {
+    let tmpDir: string;
+    beforeEach(async () => {
+      tmpDir = await fsp.mkdtemp(path.join(os.tmpdir(), 'bridgeclient-test-'));
+    });
+    afterEach(async () => {
+      await fsp.rm(tmpDir, { recursive: true, force: true });
+    });
+
+    it('writeTextFile actually writes to disk through the inline proxy', async () => {
+      const client = makeClient(/* no fileSystem */);
+      const target = path.join(tmpDir, 'inline.txt');
+
+      await client.writeTextFile({
+        path: target,
+        content: 'inline-content',
+        sessionId: 'sess:test',
+      });
+
+      const onDisk = await fsp.readFile(target, 'utf8');
+      expect(onDisk).toBe('inline-content');
+    });
+
+    it('readTextFile actually reads from disk through the inline proxy', async () => {
+      const client = makeClient(/* no fileSystem */);
+      const target = path.join(tmpDir, 'src.txt');
+      await fsp.writeFile(target, 'on-disk-content', 'utf8');
+
+      const response = await client.readTextFile({
+        path: target,
+        sessionId: 'sess:test',
+      });
+
+      expect(response.content).toBe('on-disk-content');
+    });
+  });
+});

packages/acp-bridge/src/bridgeClient.ts

@@ -19,17 +19,7 @@ import type {
 } from '@agentclientprotocol/sdk';
 import type { BridgeEvent, EventBus } from './eventBus.js';
 import type { BridgeFileSystem } from './bridgeFileSystem.js';
-
-/**
- * Inline `writeStderrLine` (lifted from `cli/src/utils/stdioHelpers.ts` in
- * #4175 F1) so acp-bridge has no reverse dependency on `cli`. Behavior is
- * byte-identical to the cli helper: writes the message to stderr followed
- * by a newline, avoiding a double newline if the message already ends with
- * one.
- */
-function writeStderrLine(message: string): void {
-  process.stderr.write(message.endsWith('\n') ? message : `${message}\n`);
-}
+import { writeStderrLine } from './internal/stderrLine.js';
 
 /**
  * Bounded duplicate-vote cache. Stores only requestId/sessionId/outcome, so

packages/acp-bridge/src/internal/stderrLine.ts

@@ -0,0 +1,30 @@
+/**
+ * @license
+ * Copyright 2025 Qwen Team
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+/**
+ * Shared `writeStderrLine` helper for `bridge.ts` + `bridgeClient.ts`.
+ *
+ * Originally inlined per-file in #4175 F1 steps 1-3 to keep the lifted
+ * modules free of any reverse import on `cli/src/utils/stdioHelpers.ts`.
+ * Wenshao review (#4319) noted that both consumers now live in the
+ * **same** `@qwen-code/acp-bridge` package — the cross-package
+ * justification no longer applies, and a future behavior change
+ * (timestamp prefix, log level, structured field) would require
+ * touching two identical copies. Extracted here so both `bridge.ts`
+ * and `bridgeClient.ts` import from a single source of truth.
+ *
+ * Not part of the package's public API — `internal/` subpath is
+ * excluded from `exports` in `package.json`. `spawnChannel.ts`
+ * deliberately does NOT consume this (its stderr writes carry their
+ * own `[serve pid=… cwd=…]` line prefix and use raw
+ * `process.stderr.write` for that reason).
+ *
+ * Byte-identical to the original `cli/src/utils/stdioHelpers.ts`
+ * implementation.
+ */
+export function writeStderrLine(message: string): void {
+  process.stderr.write(message.endsWith('\n') ? message : `${message}\n`);
+}