diff --git a/packages/acp-bridge/src/bridge.ts b/packages/acp-bridge/src/bridge.ts index bbd004ec56..6623448562 100644 --- a/packages/acp-bridge/src/bridge.ts +++ b/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; diff --git a/packages/acp-bridge/src/bridgeClient.test.ts b/packages/acp-bridge/src/bridgeClient.test.ts new file mode 100644 index 0000000000..24256eac49 --- /dev/null +++ b/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>() + .mockResolvedValue({}); + const readText = + vi.fn<(p: ReadTextFileRequest) => Promise>(); + 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>() + .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>(); + const readText = vi + .fn<(p: ReadTextFileRequest) => Promise>() + .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 => { + 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'); + }); + }); +}); diff --git a/packages/acp-bridge/src/bridgeClient.ts b/packages/acp-bridge/src/bridgeClient.ts index 83dd856adb..65ee406e61 100644 --- a/packages/acp-bridge/src/bridgeClient.ts +++ b/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 diff --git a/packages/acp-bridge/src/internal/stderrLine.ts b/packages/acp-bridge/src/internal/stderrLine.ts new file mode 100644 index 0000000000..1930a4d19d --- /dev/null +++ b/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`); +}