diff --git a/docs/design/daemon-local-text-reads.md b/docs/design/daemon-local-text-reads.md new file mode 100644 index 0000000000..4ce78edf45 --- /dev/null +++ b/docs/design/daemon-local-text-reads.md @@ -0,0 +1,88 @@ +# Daemon local text reads + +## Decision + +`BridgeOptions.delegateReadTextFileToClient` defaults to `true`, preserving +generic ACP, IDE, remote, and virtual-filesystem behavior. Same-host `qwen +serve` runtimes set it to `false`, so the ACP initialize capability is +`{ readTextFile: false, writeTextFile: true }` and the child uses its regular +CLI filesystem service for text reads. Caller-injected bridges remain under +the caller's control. + +## Behavior + +Direct external text `read_file` calls use the normal CLI permission flow: +their default is `ask`, approval allows the read, and rejection prevents tool +execution. Allow rules and automatic approval modes behave as in the CLI. +Non-text `read_file` paths were already read locally by the child and are +unchanged. + +Because the capability applies to `FileSystemService.readTextFile`, shared +text pre-reads used by write, edit, notebook, sed, and artifact operations also +move to the regular CLI filesystem service. This intentionally accepts the +CLI's read-side limits and behavior instead of WFS's 256 KiB returned-output +and full-snapshot cap, 8 MiB large-text scan cap, read audit, symlink rejection, +and read-side TOCTOU protections. Direct `read_file` still applies the core line +and output limits, subject to their existing configuration. + +This document is the single owner of that tradeoff list. Other documents +reference it rather than restating the limits, so tuning one of them does not +leave stale copies behind. + +### What this does not fix + +Reads become child-local; final ACP text writes stay delegated. The reported +failure in #8618 therefore still reproduces for the `write_file`, `replace`, +and `notebook_edit` family, only later in the sequence: the pre-read now +succeeds locally, the diff renders, the user approves, and the delegated write +is then refused by the workspace filesystem because the target is outside the +workspace. The model can still fall back to shell at that point. Moving writes +child-local as well would give up the trust gate, symlink rejection, TOCTOU +protection, atomic temp-and-rename with mode preservation, and the write audit, +which is a materially larger concession than the read change; it is deliberately +out of scope here and tracked separately. + +### Pre-approval exposure in the daemon + +A confirmation payload is built by reading the file, so an edit or write +confirmation for an out-of-workspace path now carries that file's content in +its diff. The daemon fans that payload out to every attached SSE subscriber +before the approval decision exists. In the interactive CLI the same diff is +seen only by the person at the terminal. This follows from treating +authenticated daemon clients as one security principal, and is called out here +because that framing is easy to read past. + +HTTP filesystem routes such as `/glob` and `/list` remain workspace-scoped. +Agent `glob`, `ls`, `grep`, and other discovery-tool behavior is unchanged by +this capability. Final ACP `writeTextFile` content writes stay delegated through +`WorkspaceFileSystem`, retaining workspace, trust, symlink, atomic-write, and +audit enforcement. This does not imply that every agent write or helper +operation goes through WFS. + +## Resource and audit boundaries + +A child-local text read does not emit WFS `fs.access`; direct external +`read_file` retains its permission audit and core file-operation telemetry. +Same-host reads run under the daemon user's OS identity. `qwen serve` assumes +one machine, one UID, and one security principal; it is not an OS sandbox. + +## Compatibility + +Only the default embedded daemon bridge and primary, static-secondary, and +dynamic `qwen serve` workspace runtimes disable read delegation. The WFS +adapter keeps its read implementation so an unexpected or +capability-violating delegated read still reaches the workspace boundary and +fails closed for external paths. + +That "fails closed" is bounded, not absolute. `AcpFileSystemService` has a +second, pre-existing bypass: when a delegated read is refused with +`path_outside_workspace` or `symlink_escape`, it retries the read locally if +the path's realpath sits under one of its managed read roots. Those roots +include `/tmp` unconditionally on POSIX, plus anything named by +`QWEN_ACP_LOCAL_READ_ROOTS`. So the boundary is fail-closed only for paths +outside those roots. The daemon neutralizes the env-supplied half by setting +`QWEN_ACP_LOCAL_READ_ROOTS` empty for the child. + +With the capability off, that retry path is unreachable in the daemon anyway — +the capability check returns before the delegated call is attempted — so it +now guards only generic ACP hosts that keep delegation enabled. diff --git a/docs/design/serve-large-text-range-consistency.md b/docs/design/serve-large-text-range-consistency.md index 0d2323d5c0..325852c594 100644 --- a/docs/design/serve-large-text-range-consistency.md +++ b/docs/design/serve-large-text-range-consistency.md @@ -82,7 +82,14 @@ reaching this boundary: - `GET /file` - ACP HTTP `_qwen/file/read` -- the injected ACP `readTextFile` adapter + +The injected ACP `readTextFile` adapter is no longer a production consumer: +same-host daemon runtimes advertise `readTextFile: false`, so agent text reads +are served by the child's regular CLI filesystem service and never reach this +boundary. The adapter's read path is kept as a fail-closed guard for an +unexpected or capability-violating delegated read, but the concurrent-append, +truncation, and symlink-replacement guarantees verified below no longer apply +to any agent read. See [daemon local text reads](./daemon-local-text-reads.md). Windowless reads used by workspace setup retain the existing 256 KiB full-snapshot refusal. diff --git a/docs/developers/daemon/03-acp-bridge.md b/docs/developers/daemon/03-acp-bridge.md index b33decd574..ea2270fa58 100644 --- a/docs/developers/daemon/03-acp-bridge.md +++ b/docs/developers/daemon/03-acp-bridge.md @@ -15,7 +15,7 @@ Each active `WorkspaceRuntime` owns one `HttpAcpBridge` instance. Production att - Per-session FIFO for `setSessionModel` calls so concurrent attaches with different models do not race the agent. - Per-session `EventBus` that drives `GET /session/:id/events` (see [`10-event-bus.md`](./10-event-bus.md)). - Permission flow: `BridgeClient.requestPermission` → `MultiClientPermissionMediator.request` → fan-out → vote collection → ACP response (see [`04-permission-mediation.md`](./04-permission-mediation.md)). -- File I/O: `BridgeFileSystem` adapter for ACP `readTextFile` / `writeTextFile` calls (see [`07-workspace-filesystem.md`](./07-workspace-filesystem.md)). +- File I/O: `BridgeFileSystem` adapter for ACP reads and writes; same-host daemon runtimes advertise `readTextFile: false` so normal text reads stay in the child while final text writes remain delegated (see [`07-workspace-filesystem.md`](./07-workspace-filesystem.md)). - extMethod RPCs for workspace-level status (`/workspace/mcp`, `/workspace/skills`, `/workspace/providers`), MCP restart, and the optional private managed Tool Guard callback. - Lifecycle: graceful `shutdown()` with `KILL_HARD_DEADLINE_MS` (10s) per channel; synchronous `killAllSync()` for second-signal force-exit. @@ -209,6 +209,7 @@ sequenceDiagram | `persistApprovalMode`, `persistDisabledTools` | — | Settings-write hooks for the Wave 4 mutation routes. | | `contextFilename` | from `settings.json`'s `context.fileName` | Overrides `getCurrentGeminiMdFilename`. | | `statusProvider` | (none) | Daemon-host preflight cells (`DaemonStatusProvider`). | +| `delegateReadTextFileToClient` | `true` | Set `false` only for same-host runtimes so every child `FileSystemService.readTextFile` consumer uses the regular CLI filesystem service. | | `fileSystem` | (none) | `BridgeFileSystem` adapter for ACP `readTextFile` / `writeTextFile`. | | `permissionPolicy` | from `settings.json`'s `policy.permissionStrategy` | One of `first-responder` / `designated` / `consensus` / `local-only`. | | `permissionConsensusQuorum` | from `settings.json` | N for consensus policy. | diff --git a/docs/developers/daemon/07-workspace-filesystem.md b/docs/developers/daemon/07-workspace-filesystem.md index a0f6f3a270..dadefb216f 100644 --- a/docs/developers/daemon/07-workspace-filesystem.md +++ b/docs/developers/daemon/07-workspace-filesystem.md @@ -2,7 +2,7 @@ ## Overview -The daemon never lets HTTP routes or ACP-side agent calls touch the host filesystem directly. Every read, write, list, glob, and stat goes through the `WorkspaceFileSystem` boundary (`packages/cli/src/serve/fs/`), which provides: +Daemon HTTP file routes and delegated ACP `readTextFile` / `writeTextFile` calls go through the `WorkspaceFileSystem` boundary (`packages/cli/src/serve/fs/`), which provides: - **Path resolution** — canonicalize paths and reject anything escaping the bound workspace, including via symlinks. - **Trust gating** — refuse writes when the workspace is not trusted (`untrusted_workspace`). @@ -11,7 +11,16 @@ The daemon never lets HTTP routes or ACP-side agent calls touch the host filesys - **Audit** — every access / denial emits a structured event for `PermissionAuditRing` / monitoring. - **Typed errors** — closed `FsErrorKind` union mapped to HTTP statuses. -The HTTP file routes (`GET /file`, `GET /file/bytes`, `POST /file/write`, `POST /file/edit`, `GET /list`, `GET /glob`, `GET /stat`) and the ACP-side `BridgeFileSystem` adapter (so agent-driven `readTextFile` / `writeTextFile` calls get the same gates) both go through this boundary. +The HTTP file routes (`GET /file`, `GET /file/bytes`, `POST /file/write`, `POST /file/edit`, `GET /list`, `GET /glob`, `GET /stat`) use this boundary. In the production daemon, ACP calls that remain delegated reach WFS through the injected bridge adapter; generic bridge callers use WFS only when they inject such an adapter. Production same-host `qwen serve` runtimes advertise `readTextFile: false`, so all child `FileSystemService.readTextFile` consumers use the regular CLI filesystem service; final ACP `writeTextFile` content writes remain delegated through WFS. + +That text-read capability slice covers direct `read_file` plus the shared pre-reads used by write, edit, notebook, sed, and artifact operations: + +- It intentionally accepts regular CLI read behavior rather than the WFS read-side guarantees. [The design doc](../../design/daemon-local-text-reads.md) owns the exact list of what is given up. +- The same doc records why #8618 still reproduces for the write and edit family even after this change, and the bounded sense in which the retained adapter read path "fails closed". +- Direct external `read_file` keeps the normal CLI permission rules and core file-operation telemetry. +- HTTP filesystem routes remain workspace-scoped, and agent discovery-tool behavior is unchanged by this capability. +- Auxiliary actions such as parent-directory creation and shell commands are separate existing paths, not covered by this boundary. +- `qwen serve` assumes a same-machine, same-UID security principal and is not an OS sandbox. ## Responsibilities @@ -66,7 +75,7 @@ interface BridgeFileSystem { } ``` -This is the injection point for ACP `readTextFile` / `writeTextFile`. Bridge tests and Mode A embedded callers can omit it on `BridgeOptions`; `BridgeClient` falls back to its inline `fs.readFile` / `fs.writeFile` proxy (preserves pre-F1 behavior). Production `qwen serve` wires `BridgeFileSystem` through `createBridgeFileSystemAdapter(fsFactory)` (`packages/cli/src/serve/bridge-file-system-adapter.ts`) so agent-side ACP writes pick up the same TOCTOU, symlink, trust-gate, and audit gates the HTTP routes use. +This is the injection point for ACP `readTextFile` / `writeTextFile`. Bridge tests and Mode A embedded callers can omit it on `BridgeOptions`; `BridgeClient` falls back to its inline `fs.readFile` / `fs.writeFile` proxy (preserves pre-F1 behavior). Production `qwen serve` wires `BridgeFileSystem` through `createBridgeFileSystemAdapter(fsFactory)` (`packages/cli/src/serve/bridge-file-system-adapter.ts`) and sets `delegateReadTextFileToClient: false`. Capability-compliant children therefore read text locally and delegate final ACP text writes. The adapter retains its read implementation so unexpected or capability-violating delegated reads still encounter WFS's workspace boundary. Two defensive properties the adapter MUST preserve (because the inline proxy is fully bypassed when the adapter is injected): diff --git a/docs/users/qwen-serve.md b/docs/users/qwen-serve.md index 54bd53ff23..d45b5de96a 100644 --- a/docs/users/qwen-serve.md +++ b/docs/users/qwen-serve.md @@ -557,6 +557,7 @@ provider decision with their normal tool policy and isolation boundary. - **CORS denies any browser Origin by default** — returns `403` JSON. Pass **`--allow-origin `** (repeatable, T2.4 #4514) to opt specific browser origins through. Each value is either the literal `*` (any origin — boot refuses if no bearer token is configured; `--require-auth` on loopback is recommended for full hardening since `/health` and `/demo` remain pre-auth on loopback by default) or a canonical URL origin (`://[:]`, no trailing slash / path / userinfo). Matched origins receive proper CORS response headers (`Access-Control-Allow-Origin: `, `Vary: Origin`, plus standard methods / headers / max-age and exposed `Retry-After`); unmatched origins still get a 403 with the same envelope as the default wall. `caps.features.allow_origin` is advertised conditionally so SDK / webui clients can pre-flight whether the daemon honors cross-origin hits before issuing them. Example: `qwen serve --allow-origin http://localhost:3000 --allow-origin http://localhost:5173`. Loopback self-origin hits (e.g. the `/demo` page) are unaffected — a separate Origin-strip shim handles them regardless of `--allow-origin`. **Browser webuis without `--allow-origin` configured** still fall back to the same Stage 1 options as before: package as a native shell (Electron/Tauri) so no `Origin` header is sent, or front the daemon with a same-origin reverse proxy. - **Chrome extension browser automation is separate from framing.** `qwen serve --allow-origin chrome-extension://` lets the extension frame the Web Shell and connect to the daemon. Console/network/screenshot/click tools require an external CDP MCP adapter command: `QWEN_CDP_MCP_COMMAND=/path/to/cdp-mcp-adapter qwen serve --allow-origin chrome-extension://`. The main CLI package does not bundle a browser automation adapter; clients can check `caps.features.includes('browser_automation_mcp')` before presenting those tools as available. - **A spawned `qwen --acp` child receives its owning runtime's effective environment.** The daemon freezes a process-env base, applies that workspace's settings/env-file overlay to a runtime-local snapshot, and never writes the overlay back to `process.env`; same-named keys in another runtime do not cross over. `QWEN_SERVER_TOKEN` is scrubbed before spawn because the agent does not need the daemon bearer. Base credentials such as `OPENAI_API_KEY`, `ANTHROPIC_API_KEY`, `QWEN_*`, and `DASHSCOPE_API_KEY` otherwise pass through unless the runtime overlay changes them. **This is intentional, not a sandbox.** The agent runs as the same UID with shell-tool access, so anything in `~/.bashrc`, `~/.aws/credentials`, or `~/.npmrc` is reachable by prompt injection regardless. Environment isolation between runtimes is not an operating-system security boundary; do not run `qwen serve` under an identity that has credentials you would not trust the agent with. +- **Agent text reads are child-local and follow the regular CLI permission rules, not the workspace filesystem boundary.** Direct `read_file` can reach host text paths outside every registered workspace: external paths default to confirmation, and allow rules or approval modes may approve them automatically. Approved reads use the configurable CLI output limits rather than the workspace filesystem's returned-output, full-snapshot, and large-text scan caps. This applies to every shared text-read consumer, so the pre-reads performed by write, edit, notebook, sed, and artifact operations lose those caps together with the workspace filesystem's read audit, symlink rejection, and read-side TOCTOU protections — see [the design doc](../design/daemon-local-text-reads.md) for the exact list. Because a confirmation payload is built by reading the file, an out-of-workspace diff is fanned out to **every** attached SSE subscriber before anyone approves it — in the interactive CLI that content is seen only by the person at the terminal. Treat authenticated daemon clients as the same security principal. HTTP filesystem routes remain workspace-scoped and still refuse these paths, agent discovery-tool behavior is unchanged, and final ACP `writeTextFile` content writes continue through the workspace filesystem. - **Per-subscriber bounded SSE queues** — a slow client that overflows its queue gets a `client_evicted` terminal frame and is closed; one stuck consumer can't pin the daemon. - **Per-session prompt admission cap** — defaults to 5 accepted-but-unsettled prompts per session. A buggy client cannot enqueue unbounded prompt promises or temporary SSE waits for one session. - **Graceful shutdown** — SIGINT/SIGTERM drain the agent children before closing the listener (10s deadline per child). diff --git a/integration-tests/cli/qwen-serve-streaming.test.ts b/integration-tests/cli/qwen-serve-streaming.test.ts index 4592b0e6f5..6e0c68a997 100644 --- a/integration-tests/cli/qwen-serve-streaming.test.ts +++ b/integration-tests/cli/qwen-serve-streaming.test.ts @@ -9,7 +9,7 @@ * * These tests fire real daemon prompts and observe the resulting SSE stream, * but the model side is backed by a local OpenAI-compatible fake server so - * the suite can run without API keys. They cover three flows that unit tests + * the suite can run without API keys. They cover five flows that unit tests * can't fully exercise: * * 1. Real `qwen --acp` child crash → daemon publishes `session_died`, @@ -24,14 +24,26 @@ * 4. An admitted prompt keeps running with no SSE subscriber while the Todo * Stop Guard performs its bounded continuations; a later subscriber * replays each discrete status event. + * 5. A same-host ACP child reads text outside the workspace only after the + * daemon permission request is approved, and never returns the content + * after rejection. * */ import { spawn, execSync, type ChildProcess } from 'node:child_process'; -import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { + accessSync, + constants, + mkdirSync, + mkdtempSync, + realpathSync, + rmSync, + writeFileSync, +} from 'node:fs'; import { tmpdir } from 'node:os'; import * as path from 'node:path'; import { fileURLToPath } from 'node:url'; import { afterAll, beforeAll, describe, expect, it } from 'vitest'; +import { isPathWithinRoot } from '@qwen-code/qwen-code-core'; import { DaemonClient, parseSseStream } from '@qwen-code/sdk'; import type { DaemonEvent, DaemonSessionSummary } from '@qwen-code/sdk'; import { @@ -41,6 +53,7 @@ import { } from '../fake-openai-server.js'; const __dirname = path.dirname(fileURLToPath(import.meta.url)); +const REPO_ROOT = path.resolve(__dirname, '../..'); // Match the rest of the integration suite: prefer `TEST_CLI_PATH` // from `globalSetup.ts` (root `dist/cli.js` bundle), fall back to // the per-package output for direct vitest invocations. See the same @@ -71,14 +84,69 @@ const SKIP = ); const describePOSIX = SKIP ? describe.skip : describe; +// The base only has to sit outside both the workspace and the `/tmp` local-read +// root, so the test reads a genuinely external path. The real `$HOME` is +// excluded deliberately: cleanup lives in `afterAll`, so a Ctrl-C, `--bail`, or +// CI timeout leaks the fixture dir. `/var/tmp` leaks the same way — the leak is +// relocated somewhere harmless, not eliminated. +function findExternalReadBase(): string | undefined { + if (SKIP) return undefined; + const candidates = [ + // Escape hatch for images where /var/tmp is absent or read-only. + process.env['QWEN_TEST_EXTERNAL_READ_BASE'], + '/var/tmp', + ].filter((value): value is string => Boolean(value)); + // Carry each rejection reason into the diagnostics below. A bare `catch {}` + // here cannot tell "no /var/tmp on this image" (expected) from a bug in this + // function (not expected), and the latter reads as a green skip. + const rejections: string[] = []; + for (const candidate of candidates) { + try { + const resolved = realpathSync(candidate); + accessSync(resolved, constants.W_OK); + if ( + isPathWithinRoot(resolved, realpathSync('/tmp')) || + isPathWithinRoot(resolved, realpathSync(REPO_ROOT)) + ) { + rejections.push(`${candidate}: inside the /tmp read root or the repo`); + continue; + } + return resolved; + } catch (error) { + rejections.push(`${candidate}: ${error}`); + } + } + // Skipping is acceptable on a developer box, but on CI a silently disabled + // security regression test is indistinguishable from a passing one. Fail + // loudly instead and let the operator point QWEN_TEST_EXTERNAL_READ_BASE at + // a writable directory outside both the workspace and the /tmp read root. + const diagnostics = `no usable external-read fixture base (${rejections.join('; ')})`; + if (process.env['CI']) { + throw new Error( + `${diagnostics}. Set QWEN_TEST_EXTERNAL_READ_BASE to a writable ` + + 'directory outside the repo and outside /tmp.', + ); + } + console.warn( + `[qwen-serve-streaming] skipping external read tests: ${diagnostics}`, + ); + return undefined; +} + +const externalReadBase = findExternalReadBase(); + let daemon: ChildProcess; let port = 0; let base = ''; let client: DaemonClient; let fakeServer: FakeOpenAIServer; let homeDir = ''; +let externalReadDir = ''; let workspaceDir = ''; let pendingWritePath = ''; +let pendingReadPath = ''; +let pendingReadMarker = ''; +let pendingReadSentinel = ''; beforeAll(async () => { if (SKIP) return; @@ -119,9 +187,45 @@ beforeAll(async () => { }; } + if ( + pendingReadPath && + pendingReadMarker && + messages.includes(pendingReadMarker) + ) { + if (!hasToolResult) { + return { + toolCalls: [ + fakeToolCall('read_file', { + file_path: pendingReadPath, + }), + ], + }; + } + + return { + content: messages.includes(pendingReadSentinel) + ? `external read observed: ${pendingReadSentinel}` + : 'external read content not observed', + }; + } + return { content: 'fake response complete' }; }); homeDir = mkdtempSync(path.join(tmpdir(), 'qwen-serve-streaming-home-')); + if (externalReadBase) { + let candidateDir = ''; + try { + candidateDir = mkdtempSync( + path.join(externalReadBase, '.qwen-serve-external-read-'), + ); + externalReadDir = realpathSync(candidateDir); + } catch { + if (candidateDir) { + rmSync(candidateDir, { recursive: true, force: true }); + } + externalReadDir = ''; + } + } const qwenHome = path.join(homeDir, '.qwen'); mkdirSync(qwenHome, { recursive: true }); writeFileSync( @@ -168,6 +272,7 @@ beforeAll(async () => { ), HOME: homeDir, QWEN_HOME: path.join(homeDir, '.qwen'), + QWEN_ACP_LOCAL_READ_ROOTS: '', NO_PROXY: '127.0.0.1,localhost', no_proxy: '127.0.0.1,localhost', OPENAI_API_KEY: 'fake-key', @@ -215,6 +320,9 @@ afterAll(async () => { if (homeDir) { rmSync(homeDir, { recursive: true, force: true }); } + if (externalReadDir) { + rmSync(externalReadDir, { recursive: true, force: true }); + } if (workspaceDir) { rmSync(workspaceDir, { recursive: true, force: true }); } @@ -452,6 +560,159 @@ describePOSIX('qwen serve — multi-client first-responder permission', () => { }, 90_000); }); +describePOSIX('qwen serve — same-host external text reads', () => { + async function runExternalRead( + decision: 'allow_once' | 'reject_once', + ): Promise { + const suffix = `${decision}-${Date.now()}`; + const marker = `external-read-${suffix}`; + const sentinel = `external-read-sentinel-${suffix}`; + const externalPath = path.join(externalReadDir, 'outside-workspace.txt'); + writeFileSync(externalPath, sentinel); + pendingReadPath = externalPath; + pendingReadMarker = marker; + pendingReadSentinel = sentinel; + + const session = await client.createOrAttachSession({ + // The daemon is bound to `workspaceDir` by `beforeAll`, so any other + // value is rejected with 400 Workspace mismatch. The read under test is + // external because `externalReadDir` sits outside this workspace, not + // because the session claims a wider one. + workspaceCwd: workspaceDir, + sessionScope: 'thread', + }); + await client.setSessionApprovalMode(session.sessionId, 'default'); + + const events: DaemonEvent[] = []; + const ac = new AbortController(); + let promptId: string | undefined; + const subscriber = (async () => { + try { + for await (const event of sseFrames(session.sessionId, { + signal: ac.signal, + })) { + events.push(event); + const data = event.data as { promptId?: string } | undefined; + if (event.type === 'turn_complete' && data?.promptId === promptId) { + break; + } + } + } catch { + /* aborted */ + } + })(); + const findReadPermission = () => + events.find((event) => { + if (event.type !== 'permission_request') return false; + const data = event.data as { + toolCall?: { + rawInput?: { file_path?: string }; + _meta?: { toolName?: string }; + }; + }; + return ( + data.toolCall?._meta?.toolName === 'read_file' && + data.toolCall.rawInput?.file_path === externalPath + ); + }); + + const requestStart = fakeServer.requests.length; + try { + await new Promise((resolve) => setTimeout(resolve, 200)); + const accepted = await client.promptNonBlocking(session.sessionId, { + prompt: [{ type: 'text', text: marker }], + }); + expect('promptId' in accepted).toBe(true); + if (!('promptId' in accepted)) return; + promptId = accepted.promptId; + + await expect.poll(findReadPermission, { timeout: 30_000 }).toBeDefined(); + const permission = findReadPermission(); + const permissionData = permission!.data as { + requestId: string; + options: Array<{ optionId: string; kind: string }>; + }; + const optionId = permissionData.options.find( + (option) => option.kind === decision, + )?.optionId; + expect(optionId).toBeDefined(); + expect( + await client.respondToPermission(permissionData.requestId, { + outcome: { outcome: 'selected', optionId: optionId! }, + }), + ).toBe(true); + + await expect + .poll( + () => + events.some((event) => { + const data = event.data as { promptId?: string } | undefined; + return ( + event.type === 'turn_complete' && data?.promptId === promptId + ); + }), + { timeout: 30_000 }, + ) + .toBe(true); + + const modelRequests = fakeServer.requests + .slice(requestStart) + .map((request) => JSON.stringify(request.body['messages'] ?? [])) + .filter((messages) => messages.includes(marker)); + + const serializedEvents = JSON.stringify(events); + if (decision === 'allow_once') { + expect(modelRequests.length).toBeGreaterThanOrEqual(2); + expect( + modelRequests.some((messages) => messages.includes(sentinel)), + ).toBe(true); + expect(serializedEvents).toContain( + `external read observed: ${sentinel}`, + ); + } else { + expect(modelRequests).toHaveLength(1); + expect( + modelRequests.every((messages) => !messages.includes(sentinel)), + ).toBe(true); + expect( + events.some((event) => { + if (event.type !== 'session_update') return false; + const data = event.data as { + update?: { sessionUpdate?: string; status?: string }; + }; + return ( + data.update?.sessionUpdate === 'tool_call_update' && + data.update.status === 'failed' + ); + }), + ).toBe(true); + // The failed `tool_call_update` above and the sentinel absence below + // carry the whole meaning. Asserting the user-facing rejection copy + // would fail on a wording change or a non-English locale for reasons + // unrelated to the capability under test. + expect(serializedEvents).not.toContain(sentinel); + } + } finally { + await client.cancel(session.sessionId).catch(() => undefined); + ac.abort(); + await subscriber; + await client.closeSession(session.sessionId).catch(() => undefined); + pendingReadPath = ''; + pendingReadMarker = ''; + pendingReadSentinel = ''; + rmSync(externalPath, { force: true }); + } + } + + it('returns approved content and withholds rejected content', async (ctx) => { + if (!externalReadDir) { + ctx.skip('no writable fixture root outside the workspace and /tmp'); + } + await runExternalRead('allow_once'); + await runExternalRead('reject_once'); + }, 150_000); +}); + describePOSIX('qwen serve — Last-Event-ID resume', () => { it('reconnect with Last-Event-ID:N yields events with id > N', async () => { const session = await client.createOrAttachSession({ diff --git a/packages/acp-bridge/README.md b/packages/acp-bridge/README.md index c278e7aa41..9b090df529 100644 --- a/packages/acp-bridge/README.md +++ b/packages/acp-bridge/README.md @@ -62,6 +62,9 @@ Lift history (#4175 Mode B daemon roadmap): injection seam for daemon-host env / preflight cells (production impl in `cli/src/serve/daemon-status-provider.ts`) and the F1 `BridgeFileSystem` injection seam for the ACP fs proxy. + `delegateReadTextFileToClient` defaults to `true`; same-host daemon callers + may set it to `false` so child text reads use the regular CLI filesystem + service while final ACP text writes remain delegated. - `spawnChannel` (F1) — `defaultSpawnChannelFactory` + `killChild` + `SCRUBBED_CHILD_ENV_KEYS` denylist + `scrubChildEnv` pure env-policy helper (exported for adapter reuse + unit-test access; isolates the @@ -90,9 +93,10 @@ Lift history (#4175 Mode B daemon roadmap): ACP fs proxy. When wired through `BridgeOptions.fileSystem`, `BridgeClient.readTextFile` / `BridgeClient.writeTextFile` delegate to it instead of the inline `fs.realpath` / - `fs.writeFile` / `fs.readFile` proxy. Production `qwen serve` - follow-up wraps PR 18's `WorkspaceFileSystem` here so writes - get TOCTOU + symlink + trust-gate + audit guarantees. + `fs.writeFile` / `fs.readFile` proxy. Production `qwen serve` injects + `WorkspaceFileSystem` for final ACP `writeTextFile` content writes and for + defensive handling of unexpected or capability-violating delegated reads; + normal same-host text reads stay in the child. ## Imports — root vs subpaths diff --git a/packages/acp-bridge/src/bridge-file-capabilities.test.ts b/packages/acp-bridge/src/bridge-file-capabilities.test.ts new file mode 100644 index 0000000000..a3dbc336eb --- /dev/null +++ b/packages/acp-bridge/src/bridge-file-capabilities.test.ts @@ -0,0 +1,47 @@ +/** + * @license + * Copyright 2025 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { afterEach, describe, expect, it } from 'vitest'; +import { makeBridge, makeChannel } from './internal/testUtils.js'; +import type { AcpSessionBridge } from './bridgeTypes.js'; + +describe('ACP bridge file capabilities', () => { + let bridge: AcpSessionBridge | undefined; + + afterEach(async () => { + await bridge?.shutdown(); + bridge = undefined; + }); + + it('delegates reads to the ACP client by default', async () => { + const handle = makeChannel(); + bridge = makeBridge({ channelFactory: async () => handle.channel }); + + await bridge.preheat(); + + expect(handle.agent.initializeCalls).toHaveLength(1); + expect(handle.agent.initializeCalls[0]!.clientCapabilities!.fs).toEqual({ + readTextFile: true, + writeTextFile: true, + }); + }); + + it('can keep text reads in a same-host ACP child', async () => { + const handle = makeChannel(); + bridge = makeBridge({ + channelFactory: async () => handle.channel, + delegateReadTextFileToClient: false, + }); + + await bridge.preheat(); + + expect(handle.agent.initializeCalls).toHaveLength(1); + expect(handle.agent.initializeCalls[0]!.clientCapabilities!.fs).toEqual({ + readTextFile: false, + writeTextFile: true, + }); + }); +}); diff --git a/packages/acp-bridge/src/bridge.ts b/packages/acp-bridge/src/bridge.ts index 63bf942936..14e84d2f53 100644 --- a/packages/acp-bridge/src/bridge.ts +++ b/packages/acp-bridge/src/bridge.ts @@ -1394,6 +1394,11 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { let liveTaskToolRequestHandler: LiveTaskToolRequestHandler | undefined; let liveSpeakToUserHandler: LiveSpeakToUserHandler | undefined; const defaultSessionScope = opts.sessionScope ?? 'single'; + // Resolved once beside the other option defaults: this default is + // load-bearing for every non-daemon consumer, and reading `?? true` inline + // would let a second `initialize` site drift away from it. + const delegateReadTextFileToClient = + opts.delegateReadTextFileToClient ?? true; // `undefined` → default 32 (intentionally tight to avoid resource cliffs). // `0` → explicitly unlimited (operator opt-out). // `Infinity` → unlimited (programmatic opt-out — accepted as a @@ -2542,7 +2547,10 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { [PRIVATE_PARENT_CAPABILITY_META_KEY]: privateParentCapability, }, clientCapabilities: { - fs: { readTextFile: true, writeTextFile: true }, + fs: { + readTextFile: delegateReadTextFileToClient, + writeTextFile: true, + }, }, clientInfo: { name: 'qwen-serve-bridge', version: '0' }, }), diff --git a/packages/acp-bridge/src/bridgeOptions.ts b/packages/acp-bridge/src/bridgeOptions.ts index 0d1eee25ef..99e67f7d49 100644 --- a/packages/acp-bridge/src/bridgeOptions.ts +++ b/packages/acp-bridge/src/bridgeOptions.ts @@ -373,19 +373,23 @@ export interface BridgeOptions { telemetry?: BridgeTelemetry; /** - * Optional fs injection seam. When provided, `BridgeClient.readTextFile` and - * `BridgeClient.writeTextFile` delegate every ACP fs call to this - * implementation instead of using BridgeClient's inline - * `fs.realpath` / `fs.writeFile` / `fs.readFile` proxy. + * Whether ACP text reads are delegated to the client filesystem service. + * Defaults to true for generic ACP, IDE, remote, and virtual-filesystem + * compatibility. Same-host runtimes may set false so the child uses its + * regular CLI filesystem service for every `FileSystemService.readTextFile` + * consumer. Final ACP text writes remain delegated independently. + */ + delegateReadTextFileToClient?: boolean; + + /** + * Optional fs injection seam. When provided, the enabled + * `BridgeClient.readTextFile` / `BridgeClient.writeTextFile` callbacks + * delegate ACP fs calls to this implementation instead of using + * BridgeClient's inline `fs.realpath` / `fs.writeFile` / `fs.readFile` + * proxy. * - * The immediate F1 follow-up will land a serve-side adapter that - * wraps its `WorkspaceFileSystem` and a `runQwenServe` wiring - * patch so production `qwen serve` writes pick up its TOCTOU + - * symlink-substitution + trust-gate + `.gitignore` + audit - * machinery — closing the follow-up thread about - * `BridgeClient`'s inline fs proxy bypassing `WorkspaceFileSystem` - * (originally raised in code review). Until that lands, BridgeClient's inline - * proxy continues to handle writes (current behavior preserved). + * Production `qwen serve` injects a `WorkspaceFileSystem` adapter for final + * text writes and for defensive handling of unexpected delegated reads. * * When omitted (tests, Mode A in-process consumers, channels / * IDE companion using the bridge directly), BridgeClient's inline diff --git a/packages/cli/src/acp-integration/service/filesystem.test.ts b/packages/cli/src/acp-integration/service/filesystem.test.ts index ba31dc6de5..5aa630cf52 100644 --- a/packages/cli/src/acp-integration/service/filesystem.test.ts +++ b/packages/cli/src/acp-integration/service/filesystem.test.ts @@ -26,6 +26,7 @@ import type { FileSystemService } from '@qwen-code/qwen-code-core'; import { AcpFileSystemService } from './filesystem.js'; import type { AgentSideConnection } from '@agentclientprotocol/sdk'; import { promises as fs } from 'node:fs'; +import type { Stats } from 'node:fs'; import { realpath as fsRealpath } from 'node:fs/promises'; import os from 'node:os'; import path from 'node:path'; @@ -965,9 +966,13 @@ describe('AcpFileSystemService', () => { }); }); - it('uses fallback when readTextFile capability is disabled', async () => { + // Split from the write case below on purpose: this half is the one that + // protects the capability's core behavior, so deleting "the write test" + // later must not silently drop read coverage with it. + it('routes reads to the local fallback when readTextFile capability is disabled', async () => { const client = { readTextFile: vi.fn(), + writeTextFile: vi.fn().mockResolvedValue(undefined), } as unknown as AgentSideConnection; const fallback = createFallback(); @@ -987,22 +992,64 @@ describe('AcpFileSystemService', () => { ); const signal = new AbortController().signal; + const stats = {} as Stats; const result = await svc.readTextFile({ path: '/some/file.txt', line: 0, + limit: 7, maxOutputBytes: 2048, signal, + stats, + _meta: { request: 'same-host' }, }); expect(result).toEqual(fallbackResponse); expect(fallback.readTextFile).toHaveBeenCalledWith({ path: '/some/file.txt', line: 0, + limit: 7, maxOutputBytes: 2048, signal, + stats, + _meta: { request: 'same-host' }, }); expect(client.readTextFile).not.toHaveBeenCalled(); }); + + it('keeps writes delegated when readTextFile capability is disabled', async () => { + const client = { + readTextFile: vi.fn(), + writeTextFile: vi.fn().mockResolvedValue(undefined), + } as unknown as AgentSideConnection; + + const fallback = createFallback(); + const svc = new AcpFileSystemService( + client, + 'session-3', + { readTextFile: false, writeTextFile: true }, + fallback, + ); + + // A defined `_meta` round trip: `toEqual` ignores undefined-valued + // properties, so asserting `{ _meta: undefined }` also passes for `{}` + // and would not catch the field being dropped. `bom: true` additionally + // pins the BOM prepend on the delegated content. + const meta = { bom: true }; + const writeResult = await svc.writeTextFile({ + path: '/some/file.txt', + content: 'updated content', + _meta: meta, + }); + + expect(writeResult).toEqual({ _meta: meta }); + expect(client.writeTextFile).toHaveBeenCalledWith({ + path: '/some/file.txt', + content: '\uFEFFupdated content', + sessionId: 'session-3', + _meta: meta, + }); + expect(fallback.writeTextFile).not.toHaveBeenCalled(); + }); }); describe('writeTextFile', () => { diff --git a/packages/cli/src/acp-integration/service/filesystem.ts b/packages/cli/src/acp-integration/service/filesystem.ts index bcc010f716..2744131835 100644 --- a/packages/cli/src/acp-integration/service/filesystem.ts +++ b/packages/cli/src/acp-integration/service/filesystem.ts @@ -145,6 +145,10 @@ export class AcpFileSystemService implements FileSystemService { return this.fallback.readTextFile(params); } + // Everything below — including the localReadRoots retry in the catch — is + // unreachable under `qwen serve`, which advertises this capability as + // false. It guards only generic ACP hosts that keep delegation on. Do not + // read the retry as a live backstop for daemon reads. let response: ReadTextFileResponse; try { response = await this.connection.readTextFile( diff --git a/packages/cli/src/serve/bridge-file-system-adapter.ts b/packages/cli/src/serve/bridge-file-system-adapter.ts index 584826b042..0bc48cba03 100644 --- a/packages/cli/src/serve/bridge-file-system-adapter.ts +++ b/packages/cli/src/serve/bridge-file-system-adapter.ts @@ -6,10 +6,11 @@ /** * Serve-side adapter that satisfies `@qwen-code/acp-bridge`'s - * `BridgeFileSystem` interface by routing ACP `writeTextFile` / - * `readTextFile` requests through the `WorkspaceFileSystem`. Agent-side - * ACP fs calls pick up the same defensive guarantees the HTTP file - * routes already enforce. + * `BridgeFileSystem` interface by routing delegated ACP `writeTextFile` / + * `readTextFile` requests through the `WorkspaceFileSystem`. Production + * `qwen serve` keeps text reads in the same-host child and delegates final ACP + * `writeTextFile` content writes through this adapter. The read path remains a + * fail-closed boundary for unexpected or capability-violating delegated reads. * * The adapter is a thin translation layer: * - ACP request → `WorkspaceFileSystem.resolve(path, intent)` to @@ -87,8 +88,8 @@ function buildAuditContext( /** * 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. + * HTTP fs routes) — delegated operations share the same `fsAuditEmit` channel + * + trust gate snapshot. */ export function createBridgeFileSystemAdapter( factory: WorkspaceFileSystemFactory, diff --git a/packages/cli/src/serve/run-qwen-serve.test.ts b/packages/cli/src/serve/run-qwen-serve.test.ts index 47986a5fff..41ac3c204a 100644 --- a/packages/cli/src/serve/run-qwen-serve.test.ts +++ b/packages/cli/src/serve/run-qwen-serve.test.ts @@ -1257,6 +1257,11 @@ describe('runQwenServe telemetry validation', () => { await closing; } expect(createBridge).toHaveBeenCalledTimes(2); + for (const [options] of createBridge.mock.calls) { + expect(options).toMatchObject({ + delegateReadTextFileToClient: false, + }); + } for (const result of createBridge.mock.results) { expect(result.value.shutdown).toHaveBeenCalledWith({ reason: 'daemon_shutdown', @@ -1485,6 +1490,11 @@ describe('runQwenServe telemetry validation', () => { }); expect(readded.status).toBe(201); expect(createBridge).toHaveBeenCalledTimes(3); + for (const [options] of createBridge.mock.calls) { + expect(options).toMatchObject({ + delegateReadTextFileToClient: false, + }); + } let releaseRemoval!: (count: number) => void; removeByIds.mockImplementationOnce( () => diff --git a/packages/cli/src/serve/run-qwen-serve.ts b/packages/cli/src/serve/run-qwen-serve.ts index ae3c849719..de36145482 100644 --- a/packages/cli/src/serve/run-qwen-serve.ts +++ b/packages/cli/src/serve/run-qwen-serve.ts @@ -3934,6 +3934,7 @@ async function runQwenServeImpl( : {}), permissionAudit: permissionAuditPublisher, statusProvider, + delegateReadTextFileToClient: false, fileSystem: createBridgeFileSystemAdapter(fsFactory), persistApprovalMode: (workspace, mode) => withSettingsLock(workspace, async () => { @@ -4334,6 +4335,7 @@ async function runQwenServeImpl( : {}), permissionAudit: permissionAuditPublisher, statusProvider: secondaryStatusProvider, + delegateReadTextFileToClient: false, fileSystem: createBridgeFileSystemAdapter(secondaryBridgeFsFactory), persistApprovalMode: (workspace, mode) => withSettingsLock(workspace, async () => { @@ -4884,6 +4886,7 @@ async function runQwenServeImpl( statusProvider: runtime.createDaemonStatusProvider({ env: wsEnv.effectiveEnv, }), + delegateReadTextFileToClient: false, fileSystem: createBridgeFileSystemAdapter(wsFsFactory), persistApprovalMode: (workspace, mode) => withSettingsLock(workspace, async () => { diff --git a/packages/cli/src/serve/server-default-bridge-wiring.test.ts b/packages/cli/src/serve/server-default-bridge-wiring.test.ts index cbd464df86..c5eac70377 100644 --- a/packages/cli/src/serve/server-default-bridge-wiring.test.ts +++ b/packages/cli/src/serve/server-default-bridge-wiring.test.ts @@ -53,6 +53,7 @@ describe('createServeApp default bridge wiring', () => { it('wires the internally-created bridge lifecycle into the workspace registry', async () => { let sessionLifecycle: BridgeOptions['sessionLifecycle']; + let bridgeOptions: BridgeOptions | undefined; const liveSessionIds = new Set(); const bridge = makeBridge(0, liveSessionIds); vi.doMock('./acp-session-bridge.js', async () => { @@ -62,6 +63,7 @@ describe('createServeApp default bridge wiring', () => { return { ...actual, createAcpSessionBridge: vi.fn((opts: BridgeOptions) => { + bridgeOptions = opts; sessionLifecycle = opts.sessionLifecycle; return bridge; }), @@ -80,6 +82,9 @@ describe('createServeApp default bridge wiring', () => { const locals = app.locals as { workspaceRegistry?: WorkspaceRegistry }; expect(sessionLifecycle).toBeDefined(); + expect(bridgeOptions).toMatchObject({ + delegateReadTextFileToClient: false, + }); liveSessionIds.add('session-indexed'); sessionLifecycle!({ type: 'registered', diff --git a/packages/cli/src/serve/server.ts b/packages/cli/src/serve/server.ts index c817a8461e..6f403aac5d 100644 --- a/packages/cli/src/serve/server.ts +++ b/packages/cli/src/serve/server.ts @@ -977,8 +977,9 @@ export function createServeApp( // Wire the production status provider so direct embeds / tests // that don't inject `deps.bridge` get daemon env + preflight cells. statusProvider, - // Wire the WorkspaceFileSystem adapter so ACP writeTextFile / - // readTextFile pick up trust / TOCTOU / audit. + delegateReadTextFileToClient: false, + // Final ACP text writes remain delegated through WorkspaceFileSystem. + // Unexpected delegated reads still fail closed at the WFS boundary. fileSystem: createBridgeFileSystemAdapter(fsFactory), // Reverse tool channel: answer the child's `client_mcp/message` // ext-method by reaching the WS connection that hosts the named server.