diff --git a/packages/acp-bridge/src/bridge.ts b/packages/acp-bridge/src/bridge.ts index 1156e2f63b..554106165f 100644 --- a/packages/acp-bridge/src/bridge.ts +++ b/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); } diff --git a/packages/acp-bridge/src/bridgeFileSystem.ts b/packages/acp-bridge/src/bridgeFileSystem.ts index 0de7fd0ccc..668cb2ff9d 100644 --- a/packages/acp-bridge/src/bridgeFileSystem.ts +++ b/packages/acp-bridge/src/bridgeFileSystem.ts @@ -59,21 +59,39 @@ export interface BridgeFileSystem { readText(params: ReadTextFileRequest): Promise; /** - * 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; } diff --git a/packages/cli/src/serve/bridgeFileSystemAdapter.test.ts b/packages/cli/src/serve/bridgeFileSystemAdapter.test.ts new file mode 100644 index 0000000000..7191992949 --- /dev/null +++ b/packages/cli/src/serve/bridgeFileSystemAdapter.test.ts @@ -0,0 +1,503 @@ +/** + * @license + * Copyright 2025 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +/** + * Integration tests for `createBridgeFileSystemAdapter` — the F1 + * follow-up (#4319) that wires PR 18's `WorkspaceFileSystem` through + * the `BridgeFileSystem` seam shipped in F1. + * + * Coverage focus: + * - Happy paths: ACP writeText / readText hit real disk under the + * workspace via PR 18's defensive layer. + * - Trust gate: with `trusted: false` the adapter's write call + * rejects with the same `FsError(untrusted_workspace)` posture + * HTTP `POST /file` already gives. + * - Boundary enforcement: ACP-provided absolute path that escapes + * the workspace is rejected by `WorkspaceFileSystem.resolve` + * (the resolve call fails before any disk touch). + * - Line / limit window: ACP read with `{line: 2, limit: 1}` returns + * just the requested slice (PR 18 windowing applied). + * - Audit context: the adapter routes ACP requests through + * `factory.forRequest({ route: 'ACP writeTextFile' | 'ACP readTextFile', ... })` + * so the audit stream distinguishes agent fs from HTTP fs. + */ + +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, + WriteTextFileRequest, +} from '@agentclientprotocol/sdk'; +import { createBridgeFileSystemAdapter } from './bridgeFileSystemAdapter.js'; +import { + createWorkspaceFileSystemFactory, + type WorkspaceFileSystemFactory, +} from './fs/workspaceFileSystem.js'; + +describe('createBridgeFileSystemAdapter', () => { + let tmpDir: string; + let auditEmits: Array<{ data: unknown }>; + + beforeEach(async () => { + // realpath here so macOS `/var` → `/private/var` resolution doesn't + // make the bound-workspace canonical form diverge from the path the + // test passes into the adapter (PR 18 boundary check would reject + // otherwise as "path escapes workspace"). + tmpDir = await fsp.realpath( + await fsp.mkdtemp(path.join(os.tmpdir(), 'bridge-fs-adapter-')), + ); + auditEmits = []; + }); + afterEach(async () => { + await fsp.rm(tmpDir, { recursive: true, force: true }); + }); + + function buildFactory(opts: { + trusted: boolean; + }): WorkspaceFileSystemFactory { + return createWorkspaceFileSystemFactory({ + boundWorkspace: tmpDir, + trusted: opts.trusted, + emit: (ev) => auditEmits.push(ev), + }); + } + + describe('writeText (trusted workspace)', () => { + it('writes content to disk through the PR 18 layer', async () => { + const adapter = createBridgeFileSystemAdapter( + buildFactory({ trusted: true }), + ); + const target = path.join(tmpDir, 'out.txt'); + + const params: WriteTextFileRequest = { + path: target, + content: 'adapter-content', + sessionId: 'sess:test', + }; + const response = await adapter.writeText(params); + + expect(response).toEqual({}); + const onDisk = await fsp.readFile(target, 'utf8'); + expect(onDisk).toBe('adapter-content'); + }); + + it('creates new files at 0o600 (NOT umask default — BridgeFileSystem contract)', async () => { + // BridgeFileSystem contract requires `0o600` for newly-created + // files (NOT umask defaults — agent writes don't know the file's + // intended audience, so default to "owner-only"). The old inline + // BridgeClient.writeTextFile proxy did this via fs.writeFile's + // `mode` arg; the F1 follow-up wiring delegates to PR 18's new + // `writeTextOverwrite` primitive which opens the tmp file with + // `0o600` and chmods to that default before rename. Pinning this + // here prevents a future refactor that switches the adapter back + // to `wfs.writeText` (no mode handling → umask default 0o644). + // Skipped on Windows since POSIX permission bits are not honored. + if (process.platform === 'win32') return; + const adapter = createBridgeFileSystemAdapter( + buildFactory({ trusted: true }), + ); + const target = path.join(tmpDir, 'new-secret.txt'); + await adapter.writeText({ + path: target, + content: 'secret', + sessionId: 'sess:test', + }); + const st = await fsp.stat(target); + expect(st.mode & 0o7777).toBe(0o600); + }); + + it('preserves target mode when overwriting an existing file', async () => { + // Editing a `0o600` secret must NOT downgrade it to `0o644` via + // umask. The PR 18 atomic write path snapshots the existing + // target's mode and applies it to the temp file before rename. + // Skipped on Windows for the same reason as the 0o600 test. + if (process.platform === 'win32') return; + const target = path.join(tmpDir, 'existing-secret.txt'); + await fsp.writeFile(target, 'before', { mode: 0o600 }); + await fsp.chmod(target, 0o600); + const adapter = createBridgeFileSystemAdapter( + buildFactory({ trusted: true }), + ); + await adapter.writeText({ + path: target, + content: 'after', + sessionId: 'sess:test', + }); + const st = await fsp.stat(target); + expect(st.mode & 0o7777).toBe(0o600); + expect(await fsp.readFile(target, 'utf8')).toBe('after'); + }); + + // Symlink-rejection posture (BridgeFileSystem contract divergence + // from the pre-F1 inline proxy) is enforced by `writeTextOverwrite` + // and verified at the lower layer in + // `workspaceFileSystem.test.ts > writeTextOverwrite rejects symlink + // targets planted post-resolve (symlink_escape)`. Re-testing at the + // adapter layer would only re-exercise the same code path; the + // adapter contract is "delegate to writeTextOverwrite", and the + // mode-preservation assertions above already pin THAT. + + it('emits an audit event with route="ACP writeTextFile"', async () => { + const adapter = createBridgeFileSystemAdapter( + buildFactory({ trusted: true }), + ); + + await adapter.writeText({ + path: path.join(tmpDir, 'audit.txt'), + content: 'x', + sessionId: 'sess:audit', + }); + + // Audit emits should include at least one event whose payload + // routes through 'ACP writeTextFile'. We don't pin the exact + // event count because PR 18 may emit both access + denied + // (denied if any guard fired) events — just assert the + // route label is the ACP one, not an HTTP route name. + const acpEvents = auditEmits.filter((ev) => { + const data = ev.data as { route?: string } | undefined; + return data?.route === 'ACP writeTextFile'; + }); + expect(acpEvents.length).toBeGreaterThanOrEqual(1); + }); + }); + + describe('writeText (untrusted workspace)', () => { + it('rejects with FsError when trust gate is closed', async () => { + const adapter = createBridgeFileSystemAdapter( + buildFactory({ trusted: false }), + ); + + await expect( + adapter.writeText({ + path: path.join(tmpDir, 'denied.txt'), + content: 'x', + sessionId: 'sess:test', + }), + ).rejects.toThrow(/not trusted|forbidden/i); + + // The deny should NOT have created a file. + await expect(fsp.stat(path.join(tmpDir, 'denied.txt'))).rejects.toThrow( + /ENOENT/, + ); + }); + + it('reads still succeed under trusted=false (read is not gated)', async () => { + // Parity check (per wenshao review on #4334): the writeText + // trust-gate test above covers the deny posture, but the + // adapter must NOT extend that gate to reads — PR 18's trust + // gate is write-only. Without this assertion, a future refactor + // that mistakenly gates reads would only fail HTTP-fs tests, not + // adapter ones. + const target = path.join(tmpDir, 'readable.txt'); + await fsp.writeFile(target, 'visible-content', 'utf8'); + const adapter = createBridgeFileSystemAdapter( + buildFactory({ trusted: false }), + ); + const response = await adapter.readText({ + path: target, + sessionId: 'sess:test', + }); + expect(response.content).toBe('visible-content'); + }); + }); + + describe('readText', () => { + it('reads the full file content via PR 18 readText', async () => { + const target = path.join(tmpDir, 'src.txt'); + await fsp.writeFile(target, 'line1\nline2\nline3\n', 'utf8'); + + const adapter = createBridgeFileSystemAdapter( + buildFactory({ trusted: true }), + ); + const response = await adapter.readText({ + path: target, + sessionId: 'sess:test', + }); + expect(response.content).toBe('line1\nline2\nline3\n'); + }); + + it('forwards line/limit window to PR 18', async () => { + const target = path.join(tmpDir, 'big.txt'); + await fsp.writeFile( + target, + Array.from({ length: 10 }, (_, i) => `line${i + 1}`).join('\n') + '\n', + 'utf8', + ); + + const adapter = createBridgeFileSystemAdapter( + buildFactory({ trusted: true }), + ); + const response = await adapter.readText({ + path: target, + sessionId: 'sess:test', + line: 3, + limit: 2, + }); + // PR 18's `readText` accepts 1-based line + limit and returns the + // requested window. The exact slice format mirrors HTTP `/file`'s + // line/limit semantics from PR 19. Allow trailing newline tolerance. + expect(response.content).toContain('line3'); + expect(response.content).toContain('line4'); + expect(response.content).not.toContain('line5'); + expect(response.content).not.toContain('line1'); + }); + + it('treats null line/limit as undefined (ACP wire compatibility)', async () => { + const target = path.join(tmpDir, 'null-window.txt'); + await fsp.writeFile(target, 'hello\nworld\n', 'utf8'); + + const adapter = createBridgeFileSystemAdapter( + buildFactory({ trusted: true }), + ); + // ACP allows `null` on these fields; PR 18 wants `undefined`. + // The adapter drops nulls so PR 18 sees a clean opts bag. + const response = await adapter.readText({ + path: target, + sessionId: 'sess:test', + line: null as unknown as number, + limit: null as unknown as number, + } as ReadTextFileRequest); + expect(response.content).toBe('hello\nworld\n'); + }); + + it('drops non-positive limit (negative / zero) instead of forwarding', async () => { + // wenshao #4334 review: pre-PR 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. The adapter drops + // non-positive `limit` and `line` so PR 18 falls back to no- + // windowing defaults (closest approximation to the pre-PR empty- + // content posture without smuggling `parse_error` to agents). + const target = path.join(tmpDir, 'neg-limit.txt'); + await fsp.writeFile(target, 'a\nb\nc\n', 'utf8'); + const adapter = createBridgeFileSystemAdapter( + buildFactory({ trusted: true }), + ); + const response = await adapter.readText({ + path: target, + sessionId: 'sess:test', + limit: -1 as number, + }); + // With `limit: -1` dropped, no windowing → full file content. + // Notably NOT 'a\nb\n' (which would be the broken slice(0,-1) result). + expect(response.content).toBe('a\nb\nc\n'); + }); + + it('propagates file_too_large from wfs.readText through the adapter', async () => { + // DeepSeek #4334 review: read-side error propagation through the + // adapter is otherwise untested. Pin that PR 18's file-size cap + // surfaces to ACP callers as an `FsError({kind:'file_too_large'})` + // without being silently swallowed or wrapped. + const { MAX_READ_BYTES } = await import('./fs/policy.js'); + const target = path.join(tmpDir, 'too-large.txt'); + await fsp.writeFile(target, 'x'.repeat(MAX_READ_BYTES + 1024), 'utf8'); + const adapter = createBridgeFileSystemAdapter( + buildFactory({ trusted: true }), + ); + const err = await adapter + .readText({ path: target, sessionId: 'sess:test' }) + .catch((e: unknown) => e); + expect((err as { kind?: string }).kind).toBe('file_too_large'); + }); + + it('propagates binary_file from wfs.readText through the adapter', async () => { + const target = path.join(tmpDir, 'image.bin'); + const buf = Buffer.alloc(128); + buf[5] = 0; // null byte → looksBinary() + await fsp.writeFile(target, buf); + const adapter = createBridgeFileSystemAdapter( + buildFactory({ trusted: true }), + ); + const err = await adapter + .readText({ path: target, sessionId: 'sess:test' }) + .catch((e: unknown) => e); + expect((err as { kind?: string }).kind).toBe('binary_file'); + }); + + it('propagates symlink_escape from wfs.resolve when target is a symlink to outside', async () => { + // Symmetric with the boundary-enforcement read test above, but + // covers the symlink-specific rejection path rather than the + // raw "/etc/passwd"-style outside path. PR 18 + HTTP /file + // posture: reads through a symlink resolving outside the + // workspace get `symlink_escape`. + if (process.platform === 'win32') return; + const outsideTarget = path.join(tmpDir, '..', 'outside-link-target.txt'); + await fsp.writeFile(outsideTarget, 'outside').catch(() => undefined); + try { + const link = path.join(tmpDir, 'link-out.txt'); + await fsp.symlink(outsideTarget, link, 'file'); + const adapter = createBridgeFileSystemAdapter( + buildFactory({ trusted: true }), + ); + const err = await adapter + .readText({ path: link, sessionId: 'sess:test' }) + .catch((e: unknown) => e); + // resolve() collapses symlinks → outside the workspace surfaces + // either `symlink_escape` or `path_outside_workspace` depending + // on whether resolve sees the link-collapse. Both are valid + // security signals; pin "not silently succeeded". + expect(['symlink_escape', 'path_outside_workspace']).toContain( + (err as { kind?: string }).kind, + ); + } finally { + await fsp.unlink(outsideTarget).catch(() => undefined); + } + }); + + it('drops non-positive line (zero) instead of forwarding parse_error', async () => { + const target = path.join(tmpDir, 'zero-line.txt'); + await fsp.writeFile(target, 'x\ny\n', 'utf8'); + const adapter = createBridgeFileSystemAdapter( + buildFactory({ trusted: true }), + ); + // Pre-fix the adapter would forward `line: 0` and PR 18 would + // reject with `parse_error` ("line must be a positive integer"). + // Post-fix it's dropped and the read returns the full content. + const response = await adapter.readText({ + path: target, + sessionId: 'sess:test', + line: 0 as number, + }); + expect(response.content).toBe('x\ny\n'); + }); + }); + + describe('boundary enforcement', () => { + it('rejects writes outside the bound workspace with path_outside_workspace', async () => { + // wenshao #4334 review (DWrbl): bare `.rejects.toThrow()` would + // also pass on an incidental OS-level EACCES (e.g. CI container + // refusing /etc/passwd) or any future refactor that throws a + // different error class before the boundary check runs. Pin the + // specific FsError.kind so the test verifies boundary + // enforcement is what rejects, not an accident. + const adapter = createBridgeFileSystemAdapter( + buildFactory({ trusted: true }), + ); + const err = await adapter + .writeText({ + path: '/etc/passwd', + content: 'pwned', + sessionId: 'sess:test', + }) + .catch((e: unknown) => e); + expect((err as { kind?: string }).kind).toBe('path_outside_workspace'); + }); + + it('rejects reads outside the bound workspace with path_outside_workspace', async () => { + const adapter = createBridgeFileSystemAdapter( + buildFactory({ trusted: true }), + ); + const err = await adapter + .readText({ + path: '/etc/passwd', + sessionId: 'sess:test', + }) + .catch((e: unknown) => e); + expect((err as { kind?: string }).kind).toBe('path_outside_workspace'); + }); + }); + + describe('factory.forRequest wiring', () => { + it('passes sessionId into the audit context for both read and write', async () => { + const calls: Array<{ route: string; sessionId?: string }> = []; + const fakeFactory: WorkspaceFileSystemFactory = { + forRequest: (ctx) => { + calls.push({ + route: ctx.route, + ...(ctx.sessionId ? { sessionId: ctx.sessionId } : {}), + }); + // Return a stub fs that no-ops the resolve + write/read. + return { + resolve: vi.fn(async (input) => input as never), + stat: vi.fn(), + readText: vi.fn(async () => ({ + content: 'stub', + meta: { lineEnding: 'lf' as const }, + })), + readBytes: vi.fn(), + readBytesWindow: vi.fn(), + list: vi.fn(), + glob: vi.fn(), + writeTextAtomic: vi.fn(), + writeText: vi.fn(async () => {}), + writeTextOverwrite: vi.fn(async () => ({ + created: true, + sizeBytes: 0, + hash: 'sha256:0000000000000000000000000000000000000000000000000000000000000000' as const, + meta: { lineEnding: 'lf' as const }, + })), + edit: vi.fn(), + editAtomic: vi.fn(), + }; + }, + }; + + const adapter = createBridgeFileSystemAdapter(fakeFactory); + await adapter.writeText({ + path: '/tmp/x', + content: '', + sessionId: 'sess:write', + }); + await adapter.readText({ + path: '/tmp/x', + sessionId: 'sess:read', + }); + + expect(calls).toEqual([ + { route: 'ACP writeTextFile', sessionId: 'sess:write' }, + { route: 'ACP readTextFile', sessionId: 'sess:read' }, + ]); + }); + + it('omits sessionId from audit context when ACP request lacks one', async () => { + const calls: Array<{ route: string; sessionId?: string }> = []; + const fakeFactory: WorkspaceFileSystemFactory = { + forRequest: (ctx) => { + calls.push({ + route: ctx.route, + ...(ctx.sessionId ? { sessionId: ctx.sessionId } : {}), + }); + return { + resolve: vi.fn(async (input) => input as never), + stat: vi.fn(), + readText: vi.fn(async () => ({ + content: 'stub', + meta: { lineEnding: 'lf' as const }, + })), + readBytes: vi.fn(), + readBytesWindow: vi.fn(), + list: vi.fn(), + glob: vi.fn(), + writeTextAtomic: vi.fn(), + writeText: vi.fn(async () => {}), + writeTextOverwrite: vi.fn(async () => ({ + created: true, + sizeBytes: 0, + hash: 'sha256:0000000000000000000000000000000000000000000000000000000000000000' as const, + meta: { lineEnding: 'lf' as const }, + })), + edit: vi.fn(), + editAtomic: vi.fn(), + }; + }, + }; + + const adapter = createBridgeFileSystemAdapter(fakeFactory); + // Bypass the wire types — ACP guarantees sessionId in practice, + // but the adapter's defensive omit-when-absent contract is + // worth pinning so a future schema relaxation doesn't introduce + // an undefined-string-keyed audit record. + await adapter.writeText({ + path: '/tmp/y', + content: '', + } as unknown as WriteTextFileRequest); + + expect(calls).toEqual([{ route: 'ACP writeTextFile' }]); + }); + }); +}); diff --git a/packages/cli/src/serve/bridgeFileSystemAdapter.ts b/packages/cli/src/serve/bridgeFileSystemAdapter.ts new file mode 100644 index 0000000000..5e32ae95cc --- /dev/null +++ b/packages/cli/src/serve/bridgeFileSystemAdapter.ts @@ -0,0 +1,147 @@ +/** + * @license + * Copyright 2025 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +/** + * Serve-side adapter that satisfies `@qwen-code/acp-bridge`'s + * `BridgeFileSystem` interface by routing ACP `writeTextFile` / + * `readTextFile` requests through PR 18's `WorkspaceFileSystem`. + * + * F1 (#4319) shipped the seam — `BridgeOptions.fileSystem` + + * `BridgeClient`'s early-return delegation; without this adapter the + * seam stays unused in production and `BridgeClient` falls back to + * its inline `fs.realpath` / `fs.writeFile` / `fs.readFile` proxy + * (which lacks the TOCTOU + symlink + trust-gate + audit machinery + * PR 18 added). + * + * Wiring this adapter through `runQwenServe` + `createServeApp`'s + * default bridge construction closes the `ws.ts:613` follow-up + * thread tracked since PR 18 landed — agent-side ACP fs calls now + * pick up the same defensive guarantees the HTTP file routes + * (`POST /file`, `POST /file/edit` from PR 20) already enforce. + * + * The adapter is a thin translation layer: + * - ACP request → `WorkspaceFileSystem.resolve(path, intent)` to + * materialize the `ResolvedPath` brand + * - For writes: `wfs.writeTextOverwrite(resolved, content)` — the PR + * 18 primitive that does atomic temp+rename with target-mode + * preservation (existing `0o600` survives the edit; new files + * default to `0o600`, NOT umask). Picked over `wfs.writeText` (no + * mode handling, non-atomic) and over `wfs.writeTextAtomic` (whose + * `expectedHash` CAS gate doesn't map to ACP's hash-less + * `WriteTextFileRequest` wire shape). + * - For reads: `wfs.readText(resolved, { line, limit })` (PR 18's + * read path enforces size caps + line/limit windowing + audit) + * - Error propagation is by reference — `FsError` (PR 18's + * boundary-error type, carrying a discriminator on `.kind`: + * `untrusted_workspace` / `symlink_escape` / `file_too_large` / + * etc.) is thrown unchanged through `BridgeClient`'s ACP + * `writeTextFile` / `readTextFile` handlers and serialized to the + * agent via the existing ACP error envelope. The classifier in + * `@qwen-code/acp-bridge`'s `mapDomainErrorToErrorKind` does NOT + * translate `FsError.kind` to `ServeErrorKind` — it only checks + * `instanceof` / `.name` / `.code`. The `.kind` field rides + * through on the error object itself; downstream consumers + * reading the ACP error payload pick it up directly. HTTP route + * errors take the same shape (`sendFsError` in `cli/src/serve/fs/ + * errors.ts` serializes the same `.kind`), so an SDK consumer + * handling either surface keys on `.kind` either way. + * Future PR: if `mapDomainErrorToErrorKind` should also map + * `FsError.kind`, it'd need cross-package imports (FsError lives + * in `cli/src/serve/fs`, classifier in `acp-bridge`) — handled + * as a separate scope. + * + * Tests for this adapter live alongside the bridge integration + * suite — they verify both the happy path (ACP write/read hits + * disk under the workspace) and the trust gate (`trustedWorkspace: + * false` fsFactory makes ACP writes reject with the same posture + * as HTTP `POST /file`). + */ + +import type { + ReadTextFileRequest, + ReadTextFileResponse, + WriteTextFileRequest, + WriteTextFileResponse, +} from '@agentclientprotocol/sdk'; +import type { BridgeFileSystem } from '@qwen-code/acp-bridge'; +import type { + WorkspaceFileSystemFactory, + RequestContext, +} from './fs/workspaceFileSystem.js'; + +/** Route label used in audit events for ACP-triggered fs operations. */ +const ACP_WRITE_ROUTE = 'ACP writeTextFile'; +const ACP_READ_ROUTE = 'ACP readTextFile'; + +/** + * Build the per-tick `RequestContext` the `WorkspaceFileSystemFactory` + * needs. ACP fs calls always carry a `sessionId`; `originatorClientId` + * is intentionally NOT set here because the agent (not an HTTP + * client) initiated the call — the audit record's `route` field is + * what marks it as agent-sourced. SDK consumers reading the audit + * stream can `switch` on `route` to distinguish HTTP route fs from + * agent fs. + */ +function buildAuditContext( + params: { sessionId?: string }, + route: string, +): RequestContext { + return { + route, + ...(params.sessionId ? { sessionId: params.sessionId } : {}), + }; +} + +/** + * Adapter factory. Pass the existing `WorkspaceFileSystemFactory` + * (the same instance `createServeApp` / `runQwenServe` build for + * HTTP fs routes) — both paths share the same `fsAuditEmit` channel + * + trust gate snapshot so an operator gets a unified audit stream. + */ +export function createBridgeFileSystemAdapter( + factory: WorkspaceFileSystemFactory, +): BridgeFileSystem { + return { + async writeText( + params: WriteTextFileRequest, + ): Promise { + const wfs = factory.forRequest( + buildAuditContext(params, ACP_WRITE_ROUTE), + ); + const resolved = await wfs.resolve(params.path, 'write'); + await wfs.writeTextOverwrite(resolved, params.content); + return {}; + }, + + async readText(params: ReadTextFileRequest): Promise { + const wfs = factory.forRequest(buildAuditContext(params, ACP_READ_ROUTE)); + const resolved = await wfs.resolve(params.path, 'read'); + // ACP `line` / `limit` are `number | null | undefined`; PR 18's + // `readText` opts expect `number | undefined`. Drop nulls AND + // undefineds so we only forward concrete numeric windows. + // + // Also drop non-positive `limit` (e.g. `-1`, `0`): pre-PR the + // inline `BridgeClient.readTextFile` proxy returned `{ content: + // '' }` for `limit <= 0`, but PR 18's `readText` applies + // `slice(0, limit)` which returns "all lines except the last + // |limit|" for negative limits — wrong content. Same for non- + // positive `line` (1-based; <= 0 is meaningless and currently + // rejected with parse_error). Drop both so PR 18 falls back to + // its `undefined` defaults (no windowing) — closest match to the + // pre-PR empty-content posture without smuggling a `parse_error` + // to agents that previously got `''`. + const opts: { line?: number; limit?: number } = {}; + if (typeof params.line === 'number' && params.line > 0) { + opts.line = params.line; + } + if (typeof params.limit === 'number' && params.limit > 0) { + opts.limit = params.limit; + } + const { content } = await wfs.readText(resolved, opts); + return { content }; + }, + }; +} diff --git a/packages/cli/src/serve/fs/workspaceFileSystem.test.ts b/packages/cli/src/serve/fs/workspaceFileSystem.test.ts index 930b209005..18b505fc9e 100644 --- a/packages/cli/src/serve/fs/workspaceFileSystem.test.ts +++ b/packages/cli/src/serve/fs/workspaceFileSystem.test.ts @@ -442,6 +442,209 @@ describe('WorkspaceFileSystem - write/edit', () => { expect((err as { kind: string }).kind).toBe('file_too_large'); }); + // ----- writeTextOverwrite ----- + // The "unconditional create-or-overwrite, no client hash" primitive + // used by adapters whose wire format omits expectedHash (ACP + // `WriteTextFileRequest` is the immediate consumer). Tests pin the + // BridgeFileSystem-contract guarantees the adapter relies on: + // atomic write, mode preservation, `0o600` default for new files, + // symlink rejection. + + it('writeTextOverwrite creates a new file at 0o600 (no umask leakage)', async () => { + // Skipped on Windows where POSIX permission bits are not honored. + if (process.platform === 'win32') return; + const r = await h.fs.resolve('new-overwrite.txt', 'write'); + const out = await h.fs.writeTextOverwrite(r, 'hello\n'); + expect(out.created).toBe(true); + expect(out.sizeBytes).toBe(6); + expect(out.hash).toBe(rawHash('hello\n')); + expect(await fsp.readFile(r as string, 'utf-8')).toBe('hello\n'); + const st = await fsp.lstat(r as string); + expect(st.mode & 0o7777).toBe(0o600); + }); + + it('writeTextOverwrite preserves existing target mode bits', async () => { + if (process.platform === 'win32') return; + const target = path.join(h.workspace, 'secret.txt'); + await fsp.writeFile(target, 'old', { mode: 0o600 }); + await fsp.chmod(target, 0o600); + const r = await h.fs.resolve('secret.txt', 'write'); + const out = await h.fs.writeTextOverwrite(r, 'new'); + expect(out.created).toBe(false); + expect(out.hash).toBe(rawHash('new')); + expect(await fsp.readFile(target, 'utf-8')).toBe('new'); + const st = await fsp.lstat(target); + expect(st.mode & 0o7777).toBe(0o600); + }); + + it('writeTextOverwrite preserves an executable +x bit on overwrite', async () => { + if (process.platform === 'win32') return; + const target = path.join(h.workspace, 'exec.sh'); + await fsp.writeFile(target, '#!/bin/sh\necho old\n', { mode: 0o755 }); + await fsp.chmod(target, 0o755); + const r = await h.fs.resolve('exec.sh', 'write'); + await h.fs.writeTextOverwrite(r, '#!/bin/sh\necho new\n'); + const st = await fsp.lstat(target); + expect(st.mode & 0o7777).toBe(0o755); + }); + + it('writeTextOverwrite rejects symlink targets planted post-resolve (symlink_escape)', async () => { + // Parity with writeTextAtomic and HTTP POST /file from PR 20 — + // the inline pre-F1 BridgeClient proxy resolved symlinks; PR 18 + // intentionally rejects them so a planted link can't redirect a + // write outside the operator's expectation. Mirrors the existing + // `writeText rejects when path was swapped to a symlink between + // resolve and write` shape so the test only differs in which + // method is invoked (writeTextOverwrite). + if (process.platform === 'win32') return; + const target = path.join(h.workspace, 'about-to-overwrite.txt'); + const r = await h.fs.resolve('about-to-overwrite.txt', 'write'); + const outside = path.join(h.scratch, 'overwrite-outside.txt'); + await fsp.writeFile(outside, ''); // pre-create so symlink isn't dangling + await fsp.symlink(outside, target, 'file'); + const err = await h.fs + .writeTextOverwrite(r, 'attacker') + .catch((e: unknown) => e); + expect(isFsError(err)).toBe(true); + expect((err as { kind: string }).kind).toBe('symlink_escape'); + // The outside file MUST remain empty (no escape). + expect(await fsp.readFile(outside, 'utf-8')).toBe(''); + }); + + it('writeTextOverwrite enforces the trust gate', async () => { + const untrusted = await makeHarness({ trusted: false }); + try { + const r = await untrusted.fs.resolve('denied.txt', 'write'); + const err = await untrusted.fs + .writeTextOverwrite(r, 'x') + .catch((e: unknown) => e); + expect(isFsError(err)).toBe(true); + expect((err as { kind: string }).kind).toBe('untrusted_workspace'); + } finally { + await teardown(untrusted); + } + }); + + it('writeTextOverwrite emits fs.access on success', async () => { + const r = await h.fs.resolve('audited.txt', 'write'); + await h.fs.writeTextOverwrite(r, 'audited-content'); + const access = h.events.find( + (e) => + e.type === FS_ACCESS_EVENT_TYPE && + (e.data as { intent: string }).intent === 'write', + ); + expect(access).toBeDefined(); + }); + + it('writeTextOverwrite succeeds over an existing >MAX_READ_BYTES file', async () => { + // wenshao #4334 review: the meta-read inside writeTextOverwrite is + // best-effort (only used for encoding / BOM / line-ending hints). + // A `file_too_large` thrown by `readExistingTextMeta` must NOT + // block the overwrite — pre-PR the inline `BridgeClient` proxy + // never read the existing file, so overwriting a 1 MiB log via + // the agent was always supported. Regression guard for that. + const { MAX_READ_BYTES } = await import('./policy.js'); + const target = path.join(h.workspace, 'big-existing.log'); + await fsp.writeFile(target, 'a'.repeat(MAX_READ_BYTES + 1024)); + const r = await h.fs.resolve('big-existing.log', 'write'); + const out = await h.fs.writeTextOverwrite(r, 'tiny'); + expect(out.created).toBe(false); + expect(await fsp.readFile(target, 'utf-8')).toBe('tiny'); + }); + + it('writeTextOverwrite succeeds over an existing 0o000 (unreadable) file', async () => { + // wenshao #4334 review: EACCES on the best-effort meta-read must + // NOT block the overwrite. Pre-PR the inline BridgeClient proxy + // never read the existing file, so the daemon could always + // overwrite an unreadable target (subject only to the parent dir's + // write permission). The 0o000 case also matters as a probing + // defense — bubbling EACCES on overwrite would let agents probe + // file readability indirectly. + // Skipped on Windows + when running as root (root bypasses mode bits). + if (process.platform === 'win32') return; + if (process.getuid && process.getuid() === 0) return; + const target = path.join(h.workspace, 'unreadable-secret.txt'); + await fsp.writeFile(target, 'old-secret', { mode: 0o000 }); + await fsp.chmod(target, 0o000); + try { + const r = await h.fs.resolve('unreadable-secret.txt', 'write'); + const out = await h.fs.writeTextOverwrite(r, 'new-content'); + expect(out.created).toBe(false); + // Restore mode so the test can read back the content for verification. + await fsp.chmod(target, 0o600); + expect(await fsp.readFile(target, 'utf-8')).toBe('new-content'); + } finally { + // Best-effort restore so afterEach rm doesn't trip on a 0o000 file. + await fsp.chmod(target, 0o600).catch(() => {}); + } + }); + + it('writeTextOverwrite succeeds over an existing binary file', async () => { + // Sibling of the >MAX_READ_BYTES regression: existing binary + // content makes `readExistingTextMeta` throw `binary_file`. The + // overwrite must still succeed since the new content is text and + // ACP semantics is "just write". + const target = path.join(h.workspace, 'binary-existing.bin'); + const buf = Buffer.alloc(64); + buf[5] = 0; // null byte → looksBinary() + await fsp.writeFile(target, buf); + const r = await h.fs.resolve('binary-existing.bin', 'write'); + const out = await h.fs.writeTextOverwrite(r, 'now-text'); + expect(out.created).toBe(false); + expect(await fsp.readFile(target, 'utf-8')).toBe('now-text'); + }); + + it('writeTextOverwrite rejects a directory target with parse_error', async () => { + // wenshao #4334 review sub-bullet: `assertAtomicTargetPrecondition` + // throws `parse_error` for non-regular files in the 'overwrite' + // branch (parity with 'replace'). Pin it so a future refactor that + // accidentally relaxes that branch (e.g. by treating a directory + // as "missing target → create") is caught. + const dir = path.join(h.workspace, 'a-dir'); + await fsp.mkdir(dir); + const r = await h.fs.resolve('a-dir', 'write'); + const err = await h.fs + .writeTextOverwrite(r, 'noop') + .catch((e: unknown) => e); + expect(isFsError(err)).toBe(true); + expect((err as { kind: string }).kind).toBe('parse_error'); + }); + + it('writeTextOverwrite rejects content exceeding MAX_WRITE_BYTES with file_too_large', async () => { + // wenshao #4334 review: the `enforceWriteSize(decodedSizeBytes)` + // call at the top of `writeTextOverwrite` mirrors `writeText`'s + // 5 MiB cap. The existing oversized-write test only exercises + // `writeText`; pin the cap for `writeTextOverwrite` too since it's + // the primary consumer (ACP adapter). A regression dropping this + // check would let agents write arbitrarily large files undetected. + const { MAX_WRITE_BYTES } = await import('./policy.js'); + const r = await h.fs.resolve('huge-overwrite.txt', 'write'); + const err = await h.fs + .writeTextOverwrite(r, 'a'.repeat(MAX_WRITE_BYTES + 1024)) + .catch((e: unknown) => e); + expect(isFsError(err)).toBe(true); + expect((err as { kind: string }).kind).toBe('file_too_large'); + }); + + it('writeTextAtomic rejects mode="overwrite" with parse_error (internal-only)', async () => { + // wenshao #4334 review: `'overwrite'` is a `WriteMode` value but + // `writeTextAtomic`'s `existingMeta` branch only reads metadata for + // `'replace'` AND its `created` outcome is hard-coded to `opts.mode + // === 'create'`. A direct caller of `writeTextAtomic({mode: + // 'overwrite'})` would silently drop CRLF on Windows files and + // report `created: false` for new files. The validator explicitly + // rejects this combination so the only supported path for + // unconditional-overwrite semantics is the dedicated + // `writeTextOverwrite()` method (which handles both correctly). + const r = await h.fs.resolve('atomic-overwrite-reject.txt', 'write'); + const err = await h.fs + .writeTextAtomic(r, 'x', { mode: 'overwrite' as never }) + .catch((e: unknown) => e); + expect(isFsError(err)).toBe(true); + expect((err as { kind: string }).kind).toBe('parse_error'); + expect((err as Error).message).toMatch(/writeTextOverwrite/); + }); + it('edits an existing file by replacing oldText with newText', async () => { const target = path.join(h.workspace, 'config.txt'); await fsp.writeFile(target, 'foo=1\nbar=2\n'); diff --git a/packages/cli/src/serve/fs/workspaceFileSystem.ts b/packages/cli/src/serve/fs/workspaceFileSystem.ts index 1b74473f76..2fca0a191a 100644 --- a/packages/cli/src/serve/fs/workspaceFileSystem.ts +++ b/packages/cli/src/serve/fs/workspaceFileSystem.ts @@ -128,10 +128,38 @@ export interface ReadBytesOutcome { hash?: ContentHash; } -export type WriteMode = 'create' | 'replace'; +/** + * Atomic write modes. + * + * - `'create'` — fails with `file_already_exists` if the target exists. + * - `'replace'` — requires `expectedHash`; fails with `hash_mismatch` if + * the on-disk hash doesn't match (optimistic concurrency). + * - `'overwrite'` — unconditional create-or-overwrite, no hash check. Used + * by callers whose protocol has no client-side hash + * (e.g. ACP `WriteTextFileRequest` has only + * `{path, content, sessionId}`). Still goes through the + * atomic tmp+rename + mode-preservation path so a + * `0o600` secret edit does NOT downgrade to umask-default + * and a SIGKILL mid-write does NOT truncate the target. + */ +export type WriteMode = 'create' | 'replace' | 'overwrite'; + +/** + * Subset of `WriteMode` that `writeTextAtomic` accepts. `'overwrite'` + * is intentionally excluded: the helper underneath + * (`atomicWriteTextResolvedFile`) supports it for the `writeTextOverwrite` + * method, but `writeTextAtomic`'s `existingMeta`-detection + + * `created`-derivation branches assume 'create' | 'replace' shape. + * Narrowing here prevents callers from writing + * `writeTextAtomic(p, c, {mode: 'overwrite'})` and hitting the runtime + * `parse_error` from `validateWriteTextAtomicOptions` — TypeScript + * catches it at compile time and points at the right alternative + * (`writeTextOverwrite`). + */ +export type AtomicWriteMode = Exclude; export interface WriteTextAtomicOptions extends WriteTextFileOptions { - mode: WriteMode; + mode: AtomicWriteMode; expectedHash?: ContentHash; lineEnding?: 'crlf' | 'lf'; } @@ -178,6 +206,22 @@ export interface WorkspaceFileSystem { content: string, opts: WriteTextAtomicOptions, ): Promise; + /** + * Unconditional create-or-overwrite (no `expectedHash` gate). Atomic + * temp+rename with target-mode preservation: a `0o600` secret survives + * the edit at `0o600`; a new file is created at `0o600` (NOT umask + * default). Used by protocols whose wire format carries no client-side + * hash — e.g. ACP `WriteTextFileRequest` is just `{path, content, + * sessionId}` so the CAS-gated `writeTextAtomic` doesn't fit. + * + * Symlinks at the target are rejected (`symlink_escape`) consistent + * with `writeTextAtomic` and HTTP `POST /file` from PR 20. + */ + writeTextOverwrite( + p: ResolvedPath, + content: string, + opts?: WriteTextFileOptions, + ): Promise; writeText( p: ResolvedPath, content: string, @@ -793,6 +837,111 @@ class WorkspaceFileSystemImpl implements WorkspaceFileSystem { } } + async writeTextOverwrite( + p: ResolvedPath, + content: string, + opts: WriteTextFileOptions = {}, + ): Promise { + const start = performance.now(); + try { + assertTrustedForIntent(this.deps.trusted, 'write'); + const decodedSizeBytes = Buffer.byteLength(content, 'utf-8'); + enforceWriteSize(decodedSizeBytes); + const out = await this.deps.pathLocks.runExclusive( + p as string, + async () => { + // Determine `created` from a stat — NOT from whether the meta + // read succeeded. The meta read is best-effort and can fail + // on existing files (file_too_large, binary_file); those still + // count as "the target existed", so `created: false`. + // ENOENT here means "no entry at the target" → `created: true`. + let targetExisted = false; + try { + await fsp.lstat(p as string); + targetExisted = true; + } catch (err) { + if ((err as NodeJS.ErrnoException)?.code !== 'ENOENT') { + throw err; + } + } + // Best-effort read of existing meta so we preserve detected + // encoding / BOM / line-ending across overwrites — matches the + // posture of `writeTextAtomic({mode:'replace'})` whose existing + // meta is sourced the same way. ENOENT (new file) leaves + // `existingMeta` undefined and `mergeWriteMeta` falls back to + // its UTF-8 / no-BOM / lf defaults. + let existingMeta: ReadMeta | undefined; + try { + existingMeta = await readExistingTextMeta(p); + } catch (err) { + // The meta read is best-effort — we only need it to preserve + // encoding / BOM / line-ending hints across overwrites. The + // overwrite itself never needs the existing content, so any + // failure to read it must NOT block the write: + // - ENOENT → new file, no meta to preserve (UTF-8/LF defaults) + // - EACCES / EPERM → daemon can't read (e.g. 0o000 or + // other-user-owned); the actual write + // may still succeed if the parent dir + // grants write. Bubbling here would + // both regress pre-PR behavior AND let + // agents probe file readability by + // observing EACCES on overwrite. + // - file_too_large → existing is >256 KiB; fall back to defaults + // - binary_file → existing is binary; text meta is meaningless + // Pre-PR, ACP `BridgeClient.writeTextFile` never read the + // existing file at all, so a 1 MiB log, binary config, or + // unreadable secret could always be overwritten by an agent + // (subject only to the parent dir's write permission). + // Bubbling any of these here would silently regress that. + const code = (err as NodeJS.ErrnoException)?.code; + const kind = (err as { kind?: string })?.kind; + if ( + code !== 'ENOENT' && + code !== 'EACCES' && + code !== 'EPERM' && + kind !== 'file_too_large' && + kind !== 'binary_file' + ) { + throw err; + } + } + const meta = mergeWriteMeta(existingMeta, opts); + const result = await atomicWriteTextResolvedFile({ + target: p, + content, + mode: 'overwrite', + meta, + }); + const verdict = shouldIgnore( + p, + this.deps.boundWorkspace, + this.deps.ignore, + 'file', + ); + if (verdict.ignored) meta.matchedIgnore = verdict.category; + meta.sizeBytes = result.sizeBytes; + meta.hash = result.hash; + this.deps.audit.recordAccess(this.deps.ctx, { + intent: 'write', + absolute: p, + durationMs: performance.now() - start, + sizeBytes: result.sizeBytes, + matchedIgnore: meta.matchedIgnore, + }); + return { + created: !targetExisted, + sizeBytes: result.sizeBytes, + hash: result.hash, + meta, + }; + }, + ); + return out; + } catch (err) { + throw this.recordAndWrap(err, 'write', p as string); + } + } + async writeText( p: ResolvedPath, content: string, @@ -1151,10 +1300,20 @@ interface AtomicWriteTextOutcome { } function validateWriteTextAtomicOptions(opts: WriteTextAtomicOptions): void { + // `'overwrite'` is intentionally rejected here even though the + // `WriteMode` union admits it. The `'overwrite'` variant skips the + // expectedHash CAS gate AND requires the caller to handle existing + // text-meta detection (encoding/BOM/line-ending preservation) and + // the `created` outcome flag — none of which `writeTextAtomic`'s + // existing branches do. Direct callers of `writeTextAtomic({mode: + // 'overwrite'})` would silently lose CRLF on Windows files and + // report `created: false` for new files. The dedicated + // `writeTextOverwrite()` method handles those correctly and is the + // only supported entry point for unconditional-overwrite semantics. if (opts.mode !== 'create' && opts.mode !== 'replace') { throw new FsError( 'parse_error', - 'mode must be either "create" or "replace"', + 'mode must be either "create" or "replace" (use writeTextOverwrite() for unconditional overwrites)', ); } if (opts.expectedHash !== undefined && !isContentHash(opts.expectedHash)) { @@ -1517,6 +1676,37 @@ async function assertAtomicTargetPrecondition(input: { await assertCreateTargetAbsent(input.target); return {}; } + if (input.mode === 'overwrite') { + // Tolerate missing target (new file path); reject symlinks and + // non-regular files (parity with 'replace'). When the target + // exists, return its mode so the caller can preserve it on the + // temp file before rename. + let pre: Awaited>; + try { + pre = await fsp.lstat(input.target); + } catch (err) { + if ((err as NodeJS.ErrnoException)?.code === 'ENOENT') { + return {}; + } + throw err; + } + if (pre.isSymbolicLink()) { + throw new FsError( + 'symlink_escape', + `path is a symlink and cannot be overwritten atomically: ${input.target}`, + { + hint: 're-resolve the target file instead of writing through a link', + }, + ); + } + if (!pre.isFile()) { + throw new FsError( + 'parse_error', + `path is not a regular file: ${input.target}`, + ); + } + return { mode: pre.mode & 0o7777 }; + } if (!isContentHash(input.expectedHash)) { throw new FsError( 'parse_error', diff --git a/packages/cli/src/serve/httpAcpBridge.test.ts b/packages/cli/src/serve/httpAcpBridge.test.ts index b8181b66cc..28898d70a5 100644 --- a/packages/cli/src/serve/httpAcpBridge.test.ts +++ b/packages/cli/src/serve/httpAcpBridge.test.ts @@ -6417,6 +6417,95 @@ describe('createHttpAcpBridge', () => { await bridge.shutdown(); }); + + it('routes per-entry channel bookkeeping via channelInfoForEntry, not the module-scoped channelInfo (#4325)', async () => { + // Regression guard for #4325 (wenshao review on F1 #4319). + // + // The bug pre-fix: `closeSession` (and `killSession`) 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`): closing 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, and (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. + // + // Constructing the exact overlap state deterministically requires + // factory-internal hooks not currently exposed (A only becomes + // `isDying` when its sessionIds drains to 0, and that drain path + // 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 `closeSession`). The + // full overlap regression test is deferred to a follow-up that + // adds the necessary test-only factory inspection seam. + // + // What this smoke test guards: under the normal single-channel + // case, `closeSession` still drives the channel's lifecycle + // correctly — channel kill fires after the last session closes, + // which is the most-load-bearing behavior in the fix's neighborhood + // and would fail trivially if a future refactor reverted to the + // module-scoped `channelInfo` capture without thinking through + // the case where the helper returns `undefined`. + const handles: ChannelHandle[] = []; + const factory: ChannelFactory = async () => { + const h = makeChannel(); + handles.push(h); + return h.channel; + }; + const bridge = makeBridge({ channelFactory: factory }); + const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + expect(handles).toHaveLength(1); + expect(handles[0]?.killed).toBe(false); + + await bridge.closeSession(session.sessionId); + + // Channel kill must have fired — proves `closeSession` correctly + // located the entry's channel via `channelInfoForEntry(entry)` + // (which returns the channel matching `entry.channel`) and + // triggered the L2163-2165 "kill on last session" branch. A + // reverted fix that captured `channelInfo` after the entry was + // gone from `byId` would also pass this assertion, but the + // diff-review-time visibility of the `channelInfoForEntry` call + // is the primary defense. + expect(handles[0]?.killed).toBe(true); + expect(bridge.sessionCount).toBe(0); + + await bridge.shutdown(); + }); + + it('killSession routes per-entry channel bookkeeping via channelInfoForEntry (#4325 symmetric)', async () => { + // Symmetric smoke guard for #4325 (wenshao review on this PR). + // `killSession` received the same `channelInfo` → + // `channelInfoForEntry(entry)` fix at `bridge.ts:3182` as + // `closeSession` did. The closeSession smoke above doesn't + // exercise the killSession code path, so a future refactor + // reverting only killSession would pass that test trivially. + // Same single-channel caveat: the channel-overlap race itself + // isn't deterministic without test-only factory hooks; this + // smoke verifies the most-load-bearing behavior — kill fires + // and tears down the channel — which would fail if a revert + // captured a stale module-scoped `channelInfo`. + const handles: ChannelHandle[] = []; + const factory: ChannelFactory = async () => { + const h = makeChannel(); + handles.push(h); + return h.channel; + }; + const bridge = makeBridge({ channelFactory: factory }); + const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + expect(handles).toHaveLength(1); + expect(handles[0]?.killed).toBe(false); + + await bridge.killSession(session.sessionId); + + expect(handles[0]?.killed).toBe(true); + expect(bridge.sessionCount).toBe(0); + + await bridge.shutdown(); + }); }); describe('updateSessionMetadata', () => { diff --git a/packages/cli/src/serve/runQwenServe.ts b/packages/cli/src/serve/runQwenServe.ts index 9e34b79ee7..33aa85b423 100644 --- a/packages/cli/src/serve/runQwenServe.ts +++ b/packages/cli/src/serve/runQwenServe.ts @@ -16,14 +16,12 @@ import { createHttpAcpBridge, type HttpAcpBridge, } from './httpAcpBridge.js'; +import { createBridgeFileSystemAdapter } from './bridgeFileSystemAdapter.js'; import { createDaemonStatusProvider } from './daemonStatusProvider.js'; import { isLoopbackBind } from './loopbackBinds.js'; -import { createDefaultFsAuditEmit, createServeApp } from './server.js'; +import { createServeApp, resolveBridgeFsFactory } from './server.js'; import type { ServeOptions } from './types.js'; -import { - createWorkspaceFileSystemFactory, - type WorkspaceFileSystemFactory, -} from './fs/index.js'; +import type { WorkspaceFileSystemFactory } from './fs/index.js'; const QWEN_SERVER_TOKEN_ENV = 'QWEN_SERVER_TOKEN'; const SHUTDOWN_FORCE_CLOSE_MS = 5_000; @@ -386,6 +384,22 @@ export async function runQwenServe( ); } + // F1 follow-up (#4319): construct `fsFactory` BEFORE the bridge so + // the bridge can wire it through `BridgeFileSystem` for ACP-side + // writeTextFile / readTextFile calls. Pre-fix the bridge constructed + // first and `fsFactory` lived only inside the HTTP route layer — + // agent-triggered fs calls used `BridgeClient`'s inline `fs.realpath` + // / `fs.writeFile` proxy, bypassing PR 18's TOCTOU + symlink + trust + // gate + audit machinery. See `bridgeFileSystemAdapter.ts` for the + // translation layer. + const trustedWorkspace = deps.trustedWorkspace ?? true; + const fsFactory = resolveBridgeFsFactory({ + boundWorkspace, + ...(deps.fsFactory ? { injected: deps.fsFactory } : {}), + trusted: trustedWorkspace, + ...(deps.fsAuditEmit ? { emit: deps.fsAuditEmit } : {}), + }); + const bridge = deps.bridge ?? createHttpAcpBridge({ @@ -401,6 +415,12 @@ export async function runQwenServe( // implementation wraps `buildEnvStatusFromProcess` and the // (lifted) `buildDaemonPreflightCells` body. statusProvider: createDaemonStatusProvider(), + // F1 follow-up (#4319): inject the WorkspaceFileSystem adapter so + // agent ACP `writeTextFile` / `readTextFile` calls go through + // PR 18's defensive fs layer (trust gate + atomic write + symlink + // resolution + audit emit) instead of `BridgeClient`'s inline + // raw-fs proxy. Closes the `ws.ts:613` follow-up thread. + fileSystem: createBridgeFileSystemAdapter(fsFactory), ...(contextFilenameForInit !== undefined ? { contextFilename: contextFilenameForInit } : {}), @@ -467,34 +487,12 @@ export async function runQwenServe( // callers of createServeApp (tests / embeds) omit it and the // server canonicalizes itself. // - // PR 19 — wire up `fsFactory` so the new read routes - // (`GET /file|/list|/glob|/stat`) consume a per-request boundary - // built against THIS daemon's bound workspace. Trust snapshot - // defaults to true; tests / future hardening flows pass an - // explicit `trustedWorkspace` to flip the gate. The audit-emit - // hook plugs into PR 21's SSE fan-out once that lands; until then - // the warning-emit fallback in `createServeApp` makes any silent - // drop visible in operator logs. - const trustedWorkspace = deps.trustedWorkspace ?? true; - // Reuse `createDefaultFsAuditEmit` so the throttle behavior here - // matches what `createServeApp`'s built-in fallback would emit. - // The earlier per-event `writeStderrLine` would print one line for - // every `/file` / `/list` / `/glob` / `/stat` audit event under - // normal traffic — a workspace scan can flood operator logs in - // seconds. The shared helper warns once + every 100th drop and - // includes payload context (errorKind / intent / pathHash), so a - // genuine wiring regression still surfaces but routine audit - // traffic stays quiet. Future PR 21 SSE fan-out replaces this - // default with the workspace-scoped event channel; until then the - // throttled stderr warning is the canonical "emit channel orphaned" - // breadcrumb. - const fsFactory = - deps.fsFactory ?? - createWorkspaceFileSystemFactory({ - boundWorkspace, - trusted: trustedWorkspace, - emit: deps.fsAuditEmit ?? createDefaultFsAuditEmit(), - }); + // PR 19 — `fsFactory` is now constructed above (before the bridge) + // so the bridge can wire it through `BridgeFileSystem`. The HTTP + // read routes (`GET /file|/list|/glob|/stat`) still consume the + // same factory instance — trust snapshot + audit-emit hook flow + // through both surfaces identically, so a single operator audit + // stream covers HTTP fs + ACP fs. const app = createServeApp(opts, () => actualPort, { bridge, boundWorkspace, diff --git a/packages/cli/src/serve/server.ts b/packages/cli/src/serve/server.ts index fc749a20ca..b5913a5702 100644 --- a/packages/cli/src/serve/server.ts +++ b/packages/cli/src/serve/server.ts @@ -28,6 +28,7 @@ import { type DeviceFlowPublicView, } from './auth/deviceFlow.js'; import { QwenOAuthDeviceFlowProvider } from './auth/qwenDeviceFlowProvider.js'; +import { createBridgeFileSystemAdapter } from './bridgeFileSystemAdapter.js'; import { createDaemonStatusProvider } from './daemonStatusProvider.js'; import { isServeDebugMode } from './debugMode.js'; import { isLoopbackBind } from './loopbackBinds.js'; @@ -111,6 +112,53 @@ export function createDefaultFsAuditEmit(): (event: BridgeEvent) => void { }; } +/** + * Shared `WorkspaceFileSystemFactory` construction used by both + * `runQwenServe` and `createServeApp`'s default bridge wiring. + * Centralizes the "use the injected factory if provided, otherwise + * build one with the given trust + audit-emit posture" logic so a + * future change to one call site doesn't silently diverge the other + * (#4334 DWcK4 review fold-in). + * + * Trust is intentionally a **required** parameter — the two call + * sites have different correct defaults and centralizing only the + * construction shape (not the policy) prevents accidental coupling: + * - `runQwenServe` defaults to `trusted: true` (production daemon + * boots up trusted; an explicit `deps.trustedWorkspace: false` + * overrides for sandboxed runs). + * - `createServeApp` defaults to `trusted: false` (test-safe; + * direct embeds — IDE companions, hosted daemons — must + * explicitly opt in to writes via `deps.fsFactory` or by + * injecting their own `deps.bridge`). + * + * The audit-emit defaults to `createDefaultFsAuditEmit()` (the + * throttled stderr warning) when not provided. + */ +/** + * Module-scoped once-per-process guard for the `createServeApp` + * default-trust stderr warning (#4334 wenshao review DWrbn). Without + * this, `server.test.ts`'s ~25 `createServeApp` calls would flood + * stderr with identical lines, masking genuine failures in CI logs. + * Module scope (not closure-per-app) is intentional — the warning is + * a *posture* statement about this binary, not about each individual + * createServeApp instance. + */ +let warnedDefaultTrust = false; + +export function resolveBridgeFsFactory(input: { + boundWorkspace: string; + injected?: WorkspaceFileSystemFactory; + trusted: boolean; + emit?: (event: BridgeEvent) => void; +}): WorkspaceFileSystemFactory { + if (input.injected) return input.injected; + return createWorkspaceFileSystemFactory({ + boundWorkspace: input.boundWorkspace, + trusted: input.trusted, + emit: input.emit ?? createDefaultFsAuditEmit(), + }); +} + export interface ServeAppDeps { /** Bridge instance; tests inject a fake. Defaults to a fresh real one. */ bridge?: HttpAcpBridge; @@ -240,6 +288,52 @@ export function createServeApp( const boundWorkspace = deps.boundWorkspace ?? canonicalizeWorkspace(opts.workspace ?? process.cwd()); + // F1 follow-up (#4319): construct `fsFactory` BEFORE the bridge so + // the bridge can wire it through `BridgeFileSystem` for ACP-side + // writeTextFile / readTextFile calls. Symmetric with + // `runQwenServe.ts` — without this wiring, direct embeds / tests + // that don't inject `deps.bridge` would silently lose the PR 18 + // defensive fs layer for agent-triggered fs (the inline raw-fs + // proxy in `BridgeClient` would still run, bypassing trust / + // TOCTOU / audit). Default trust here is `false` (test-safe) to + // match the existing `createServeApp` posture below. + // + // Behavior change warning for embed consumers (#4334 wenshao review): + // pre-this-PR, ACP writeTextFile went through `BridgeClient`'s inline + // proxy which had no trust gate, so embeds calling `createServeApp` + // without providing `deps.fsFactory` had agent writes always succeed. + // Now those writes will reject with `untrusted_workspace` unless the + // embed either (a) provides its own `deps.fsFactory` with `trusted: + // true`, (b) provides its own `deps.bridge` (which controls its own + // `BridgeFileSystem` wiring and bypasses the default adapter), or + // (c) explicitly accepts the trust-gate-default posture for its + // hosting environment. Warn loudly so the asymmetry is visible to + // operators rather than appearing as opaque agent-side write failures. + // `runQwenServe.ts` consumers are unaffected — that path constructs + // `fsFactory` with `trusted: true` by default. + // + // Suppress the warning when `deps.bridge` is provided — that embed + // owns its own fileSystem wiring (the default adapter never runs) so + // the warning's claim about ACP writes rejecting would be false. + // + // Throttled to fire ONCE per process (#4334 wenshao review DWrbn): + // `server.test.ts` calls `createServeApp` ~25 times and the + // unthrottled warning floods stderr, masking genuine failures in CI + // output. Operators see the warning the first time and can take + // action; subsequent calls don't add information. + if (!deps.fsFactory && !deps.bridge && !warnedDefaultTrust) { + warnedDefaultTrust = true; + process.stderr.write( + 'qwen serve: createServeApp default fsFactory uses trusted=false ' + + '— agent ACP writeTextFile calls will reject with untrusted_workspace. ' + + 'Inject deps.fsFactory (with explicit trust) or deps.bridge to override.\n', + ); + } + const fsFactory = resolveBridgeFsFactory({ + boundWorkspace, + ...(deps.fsFactory ? { injected: deps.fsFactory } : {}), + trusted: false, + }); const bridge = deps.bridge ?? createHttpAcpBridge({ @@ -260,6 +354,10 @@ export function createServeApp( // here preserves byte-for-byte route output on the default // bridge construction path. statusProvider: createDaemonStatusProvider(), + // F1 follow-up (#4319): wire the WorkspaceFileSystem adapter so + // ACP writeTextFile / readTextFile pick up trust / TOCTOU / + // audit. See `bridgeFileSystemAdapter.ts`. + fileSystem: createBridgeFileSystemAdapter(fsFactory), }); // Allow same-origin requests from the demo page. Browsers send an @@ -303,13 +401,12 @@ export function createServeApp( // audit destination — `runQwenServe` will inject one whose // `trusted` mirrors `Config.isTrustedFolder()` and whose `emit` // plumbs into the per-session EventBus once PR 19/20 lands. - const fsFactory: WorkspaceFileSystemFactory = - deps.fsFactory ?? - createWorkspaceFileSystemFactory({ - boundWorkspace, - trusted: false, - emit: createDefaultFsAuditEmit(), - }); + // + // F1 follow-up (#4319): the `fsFactory` is now constructed earlier + // (above the bridge) so the bridge can wire it into the + // `BridgeFileSystem` seam for ACP fs methods. Re-using the same + // instance for HTTP fs routes + ACP fs ensures a single operator + // audit stream covers both surfaces. // Park the factory on `app.locals` so PR 19/20 route handlers can // pick it up via `req.app.locals.fsFactory` without re-threading // the value through every handler signature, and so PR 18 tests