From 215e0dbf53b279b9af2386315d9dd715b504f676 Mon Sep 17 00:00:00 2001 From: wenshao Date: Wed, 13 May 2026 17:36:58 +0800 Subject: [PATCH 01/16] =?UTF-8?q?refactor(serve):=201=20daemon=20=3D=201?= =?UTF-8?q?=20workspace=20(#3803=20=C2=A702)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Stage 1 shipped with M-workspaces-per-daemon routing (`byWorkspaceChannel` Map keyed by request `cwd`). The §02 architectural revision in `docs/comparison/qwen-code-daemon-design/02-architectural-decisions.md` narrows the bridge to 1 daemon = 1 workspace × N sessions: each daemon binds to one canonical workspace path at boot; `POST /session` with a mismatched `cwd` returns 400 `workspace_mismatch`. Multi-workspace deployments run multiple daemon processes (one per workspace, supervised externally — systemd / docker-compose / k8s / `qwen-coordinator`). Bridge state collapses from maps to single optional slots: - `byWorkspaceChannel: Map` → `channelInfo?: ChannelInfo` - `inFlightChannelSpawns: Map` → `inFlightChannelSpawn?: Promise` - `byWorkspace: Map` → `defaultEntry?: SessionEntry` - `liveChannels: Set` → not needed; `channelInfo` is the live reference, cleared only by `channel.exited` (preserves the tanzhenxin BkUyD invariant that `killAllSync` finds a target mid-SIGTERM-grace) `BridgeOptions.boundWorkspace` becomes required. `WorkspaceMismatchError` is thrown from `spawnOrAttach` when the request's canonical cwd doesn't match the bound path, translated to 400 `workspace_mismatch` (with both paths in the body) by the route layer. `CapabilitiesEnvelope.workspaceCwd` surfaces the bound path so clients pre-flight check + omit `cwd` from `POST /session` (it falls back to the bound workspace). A new `--workspace ` CLI flag lets operators override `process.cwd()` at boot. The previous `--http-bridge` / `--multi-workspace` opt-in was never shipped; nothing changes for default users running `qwen serve` in their project directory. Removed code path: ~150 LOC of multi-workspace map machinery in `httpAcpBridge.ts` plus the test cases that exercised it. Test surgery: - New `makeBridge()` helper in `httpAcpBridge.test.ts` injects `boundWorkspace: WS_A` by default; tests that need a different bind (the mismatch test) pass it explicitly. - `does NOT reuse across workspaces` → `rejects cross-workspace requests with WorkspaceMismatchError` (the new semantics under §02). - `shutdown kills every live channel` retargeted to single-channel multi-session shutdown. - `killAllSync force-kills channels even after shutdown cleared byWorkspaceChannel (BkUyD)` retargeted to single-channel: the invariant is the same (channel reference must outlive eager shutdown clearing), the surface is just smaller. - `listWorkspaceSessions` cross-workspace assertion now expects empty for the un-bound path. - `--max-sessions` cap test uses two thread-scope sessions on `WS_A` instead of WS_A + WS_B. Closes #3803 §02. --- docs/developers/qwen-serve-protocol.md | 20 +- docs/users/qwen-serve.md | 44 +- packages/cli/src/commands/serve.ts | 11 + packages/cli/src/serve/httpAcpBridge.test.ts | 240 +++++----- packages/cli/src/serve/httpAcpBridge.ts | 438 ++++++++++--------- packages/cli/src/serve/runQwenServe.ts | 23 +- packages/cli/src/serve/server.test.ts | 67 ++- packages/cli/src/serve/server.ts | 40 +- packages/cli/src/serve/types.ts | 29 ++ 9 files changed, 566 insertions(+), 346 deletions(-) diff --git a/docs/developers/qwen-serve-protocol.md b/docs/developers/qwen-serve-protocol.md index 4b73183e169..583c2ac8814 100644 --- a/docs/developers/qwen-serve-protocol.md +++ b/docs/developers/qwen-serve-protocol.md @@ -42,6 +42,19 @@ with status `400`. with status `404`. +`WorkspaceMismatchError` for a `POST /session` whose `cwd` doesn't canonicalize to the daemon's bound workspace (#3803 §02 — 1 daemon = 1 workspace) returns `400` with: + +```json +{ + "error": "Workspace mismatch: daemon is bound to \"…\" but request asked for \"…\". …", + "code": "workspace_mismatch", + "boundWorkspace": "/path/the/daemon/binds", + "requestedWorkspace": "/path/in/the/request" +} +``` + +Use this to detect mismatch pre-flight: read `workspaceCwd` off `/capabilities` and omit `cwd` from `POST /session` (it falls back to the bound workspace), or route the request to a daemon bound to `requestedWorkspace`. + `POST /session` past the daemon's `--max-sessions` cap returns `503` with a `Retry-After: 5` header and: ```json @@ -93,7 +106,8 @@ Pass `?deep=1` (also accepts `?deep=true` or bare `?deep`) for a probe that expo "v": 1, "mode": "http-bridge", "features": ["health", "capabilities", "..."], - "modelServices": [] + "modelServices": [], + "workspaceCwd": "/canonical/path/to/workspace" } ``` @@ -101,6 +115,8 @@ Stable contract: when `v` increments the frame layout has changed in a backwards > **`modelServices` is always `[]` in Stage 1.** The agent uses its single default model service and doesn't enumerate it over the wire. Stage 2 will populate this from registered model adapters so SDK clients can build service-pickers; until then, do NOT rely on this field being non-empty. +> **`workspaceCwd`** is the canonical absolute path this daemon binds to (#3803 §02 — 1 daemon = 1 workspace). Use it to (a) detect mismatch before posting `/session` and (b) omit `cwd` on `POST /session` (the route falls back to this path). Multi-workspace deployments expose multiple daemons on different ports, each with its own `workspaceCwd`. + ### `POST /session` Spawn a new agent or attach to an existing one (under `sessionScope: 'single'`, the default). @@ -116,7 +132,7 @@ Request: | Field | Required | Notes | | ---------------- | -------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | -| `cwd` | yes | Absolute path. Relative paths return `400`. Workspace paths are canonicalized via `realpathSync.native` (with a resolve-only fallback for non-existent paths) so case-insensitive filesystems don't fork sessions per spelling. | +| `cwd` | no | Absolute path matching the daemon's bound workspace. If omitted, the route falls back to `boundWorkspace` (read it off `/capabilities.workspaceCwd`). A mismatched non-empty `cwd` returns `400 workspace_mismatch` (#3803 §02 — 1 daemon = 1 workspace). Workspace paths are canonicalized via `realpathSync.native` (with a resolve-only fallback for non-existent paths) so case-insensitive filesystems don't reject sessions per spelling. | | `modelServiceId` | no | Selects which configured _model service_ the agent will route through (the back-end provider — Alibaba ModelStudio, OpenRouter, etc). If omitted the agent uses its default. If the workspace already has a session, this calls `setSessionModel` on the existing one and broadcasts `model_switched`. Distinct from `modelId` on `POST /session/:id/model`, which selects the model **within** an already-bound service. The `modelServices` array on `/capabilities` is reserved for advertising configured services; in Stage 1 it is always `[]` (the agent's default service is used and not enumerated over HTTP). | Response: diff --git a/docs/users/qwen-serve.md b/docs/users/qwen-serve.md index 66c1b8acc21..1e2f7a1e75d 100644 --- a/docs/users/qwen-serve.md +++ b/docs/users/qwen-serve.md @@ -8,9 +8,10 @@ Run Qwen Code as a local HTTP daemon so multiple clients (IDE plugins, web UIs, ## What it gives you -- **One agent process, many clients** — under the default `sessionScope: 'single'`, every client connecting to the same workspace shares one ACP session. Live cross-client collaboration on the same conversation, the same file diffs, the same permission prompts. +- **One agent process, many clients** — under the default `sessionScope: 'single'`, every client connecting to the daemon shares one ACP session. Live cross-client collaboration on the same conversation, the same file diffs, the same permission prompts. - **Reconnect-safe streaming** — SSE with `Last-Event-ID` reconnect lets a client drop and pick up exactly where it left off (within the ring's replay window). - **First-responder permissions** — when the agent asks for permission to run a tool, every connected client sees the request; whichever client answers first wins. +- **One daemon, one workspace** — each `qwen serve` process binds to exactly one workspace at boot (per [#3803](https://github.com/QwenLM/qwen-code/issues/3803) §02). Multi-workspace deployments run one daemon per workspace on separate ports (or behind an orchestrator). ## Quickstart @@ -19,11 +20,11 @@ Run Qwen Code as a local HTTP daemon so multiple clients (IDE plugins, web UIs, ```bash cd your-project/ qwen serve -# → qwen serve listening on http://127.0.0.1:4170 (mode=http-bridge) +# → qwen serve listening on http://127.0.0.1:4170 (mode=http-bridge, workspace=/path/to/your-project) # → qwen serve: bearer auth disabled (loopback default). Set QWEN_SERVER_TOKEN to enable. ``` -The default bind is `127.0.0.1:4170`. Bearer auth is **off** on loopback so local development "just works". +The default bind is `127.0.0.1:4170`. Bearer auth is **off** on loopback so local development "just works". The daemon binds to the current working directory; use `--workspace /path/to/dir` to override. ### 2. Sanity-check it @@ -32,19 +33,23 @@ curl http://127.0.0.1:4170/health # → {"status":"ok"} curl http://127.0.0.1:4170/capabilities -# → {"v":1,"mode":"http-bridge","features":["health","capabilities","session_create",...]} +# → {"v":1,"mode":"http-bridge","features":["health","capabilities","session_create",...],"workspaceCwd":"/path/to/your-project"} ``` +The `workspaceCwd` field surfaces the bound workspace so clients can pre-flight check + omit `cwd` on `POST /session`. + ### 3. Open a session ```bash curl -X POST http://127.0.0.1:4170/session \ -H 'Content-Type: application/json' \ - -d '{"cwd":"'"$PWD"'"}' + -d '{}' # → {"sessionId":"","workspaceCwd":"…","attached":false} ``` -A second client posting to `/session` with the same `cwd` gets `"attached": true` — they're now sharing the agent. +`cwd` may be omitted — the route falls back to the daemon's bound workspace. Posting a `cwd` that doesn't match the bound workspace returns `400 workspace_mismatch` (the daemon is bound to exactly one workspace; start a separate daemon for a different one). + +A second client posting to `/session` (any matching `cwd` or none) gets `"attached": true` — they're now sharing the agent. ### 4. Subscribe to the event stream (in another terminal first) @@ -102,14 +107,15 @@ The token comparison is constant-time (SHA-256 + `crypto.timingSafeEqual`); 401 ## CLI flags -| Flag | Default | Purpose | -| ----------------------- | ----------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `--port ` | `4170` | TCP port. `0` = OS-assigned ephemeral port. | -| `--hostname ` | `127.0.0.1` | Bind interface. Anything beyond loopback requires a token. | -| `--token ` | — | Bearer token. Falls back to `QWEN_SERVER_TOKEN` env var (with leading/trailing whitespace stripped — handy for `$(cat token.txt)`). | -| `--max-sessions ` | `20` | Cap on concurrent live sessions. New `POST /session` requests that would spawn a fresh child return `503` (with `Retry-After: 5`) when the cap is hit; attaches to existing sessions are NOT counted. Set to `0` to disable. Sized for single-user / small-team usage; raise it if your deployment has the RAM/FD headroom (~30–50 MB per session). | -| `--max-connections ` | `256` | Listener-level TCP connection cap (`server.maxConnections`). Bounds raw socket count irrespective of session count — slow / phantom SSE clients get rejected at accept time once full. Raise alongside `--max-sessions` if your deployment expects many SSE subscribers per session. | -| `--http-bridge` | `true` | Stage 1 mode: per-session `qwen --acp` child process. Stage 2 native in-process becomes available later. | +| Flag | Default | Purpose | +| ----------------------- | --------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `--port ` | `4170` | TCP port. `0` = OS-assigned ephemeral port. | +| `--hostname ` | `127.0.0.1` | Bind interface. Anything beyond loopback requires a token. | +| `--token ` | — | Bearer token. Falls back to `QWEN_SERVER_TOKEN` env var (with leading/trailing whitespace stripped — handy for `$(cat token.txt)`). | +| `--max-sessions ` | `20` | Cap on concurrent live sessions. New `POST /session` requests that would spawn a fresh child return `503` (with `Retry-After: 5`) when the cap is hit; attaches to existing sessions are NOT counted. Set to `0` to disable. Sized for single-user / small-team usage; raise it if your deployment has the RAM/FD headroom (~30–50 MB per session). | +| `--workspace ` | `process.cwd()` | Absolute workspace path this daemon binds to (per [#3803](https://github.com/QwenLM/qwen-code/issues/3803) §02 — 1 daemon = 1 workspace). `POST /session` requests with a mismatched `cwd` return `400 workspace_mismatch`. For multi-workspace deployments, run one `qwen serve` per workspace on separate ports. | +| `--max-connections ` | `256` | Listener-level TCP connection cap (`server.maxConnections`). Bounds raw socket count irrespective of session count — slow / phantom SSE clients get rejected at accept time once full. Raise alongside `--max-sessions` if your deployment expects many SSE subscribers per session. | +| `--http-bridge` | `true` | Stage 1 mode: per-session `qwen --acp` child process. Stage 2 native in-process becomes available later. | > **Sizing the load knobs.** `--max-sessions` is the **new-child** cap. > Three other layers also limit load — when sizing for a high-concurrency @@ -157,13 +163,15 @@ The token comparison is constant-time (SHA-256 + `crypto.timingSafeEqual`); 401 > swallow RSTs may want to lower `server.keepAliveTimeout` via a > reverse proxy or accept periodic daemon restarts. -## Multi-session & remote deployment +## Multi-session & multi-workspace deployment + +Per [#3803](https://github.com/QwenLM/qwen-code/issues/3803) §02, each `qwen serve` process binds to **one workspace** at boot. Within that workspace it multiplexes N sessions onto a single `qwen --acp` child via the agent's native session map — sessions share the child's process / OAuth state / file-read cache / hierarchy-memory parse. -A single `qwen serve` process can manage sessions for any workspace path passed via `cwd` on `POST /session` — under the default `sessionScope: 'single'` it keeps one ACP session per canonicalized workspace, sharing it across every client that posts the same `cwd`. So one daemon will happily host sessions for many workspaces at once. +To host **multiple workspaces** (one user, several repos; or several users on the same host), run **multiple daemon processes** — one per workspace, each on its own port, supervised by systemd / docker-compose / k8s / a `qwen-coordinator` reference orchestrator. The trade-off is intentional: one workspace per child means `loadSettings(cwd)` / OAuth / MCP server scope stay aligned with the bound directory and don't drift across requests. > **Subscribe BEFORE posting `modelServiceId` on attach.** When a client `POST /session` with a `modelServiceId` and the workspace already has a session running a different model, the daemon issues an internal `setSessionModel` call — failures are NOT propagated as an HTTP error (the session stays operational on its current model). The visible failure signal is a `model_switch_failed` event on the session's SSE stream. If you call `POST /session` and only THEN open `GET /session/:id/events`, you'll miss the failure event and silently keep talking to the wrong model. Open the SSE stream first, or pass `Last-Event-ID: 0` on subscribe to replay the ring's oldest available event. -To handle multiple **users** (each with their own quota, audit log, sandbox) or to scale beyond one process's reach (cold-start budget, FD count, RSS), you spawn multiple daemon instances behind an external orchestrator. That orchestrator (multi-tenancy / OIDC / Quota / Audit / k8s) is **out of scope** for the qwen-code project — see issue [#3803](https://github.com/QwenLM/qwen-code/issues/3803) "External Reference Architecture" for the design pointers. +To handle multiple **users** (each with their own quota, audit log, sandbox) or to scale beyond one process's reach (cold-start budget, FD count, RSS), spawn one daemon per workspace per user behind an external orchestrator. That orchestrator (multi-tenancy / OIDC / Quota / Audit / k8s) is **out of scope** for the qwen-code project — see issue [#3803](https://github.com/QwenLM/qwen-code/issues/3803) "External Reference Architecture" for the design pointers. ## Durability model @@ -255,7 +263,7 @@ Concrete cost at N=5 sessions on the same workspace: | Auto-memory learned facts | shared | one knowledge base per child | | Cold start | first only | <200 ms after first session | -The bridge keeps **one channel per workspace** (cross-workspace sharing is intentionally not done — different workspaces have different settings/auth scope, and `acpAgent.ts:601` reloads settings per newSession `cwd`, which would interfere). The channel stays alive while at least one session is live; the last `killSession` (or a channel-level crash) kills the child. +The bridge keeps **one channel per daemon** (one daemon per workspace, per §02). The channel stays alive while at least one session is live; the last `killSession` (or a channel-level crash) kills the child. **MCP server children** are still per-session today — each session's config can specify different servers, so they're independently spawned. Stage 1.5 follow-up: refcount MCP server children by `(workspace, config-hash)` so identical configs share. Not in scope for this PR. diff --git a/packages/cli/src/commands/serve.ts b/packages/cli/src/commands/serve.ts index 6240f970907..d2a2d6bf8bb 100644 --- a/packages/cli/src/commands/serve.ts +++ b/packages/cli/src/commands/serve.ts @@ -30,6 +30,7 @@ interface ServeArgs { token?: string; 'max-sessions': number; 'max-connections': number; + workspace?: string; // Read from the kebab-case key only — the camelCase mirror that yargs // synthesizes is convenient for handlers but type-confusing here. The // handler reads `argv['http-bridge']` directly. @@ -66,6 +67,15 @@ export const serveCommand: CommandModule = { 'Cap on concurrent live sessions. New spawn requests beyond this return 503; ' + 'attach to existing sessions still works. Set to 0 to disable.', }) + .option('workspace', { + type: 'string', + description: + 'Absolute workspace path this daemon binds to. ' + + 'POST /session requests with a mismatched cwd return 400 workspace_mismatch. ' + + 'Defaults to process.cwd() when omitted. ' + + 'For multi-workspace deployments, run one `qwen serve` per workspace ' + + 'on separate ports (or behind an external orchestrator).', + }) .option('max-connections', { type: 'number', default: 256, @@ -112,6 +122,7 @@ export const serveCommand: CommandModule = { mode: 'http-bridge', maxSessions: argv['max-sessions'], maxConnections: argv['max-connections'], + workspace: argv.workspace, }); } catch (err) { writeStderrLine( diff --git a/packages/cli/src/serve/httpAcpBridge.test.ts b/packages/cli/src/serve/httpAcpBridge.test.ts index 51ba8b0fe35..b4e3e8f44b5 100644 --- a/packages/cli/src/serve/httpAcpBridge.test.ts +++ b/packages/cli/src/serve/httpAcpBridge.test.ts @@ -36,8 +36,11 @@ import { createHttpAcpBridge, InvalidPermissionOptionError, SessionNotFoundError, + WorkspaceMismatchError, type AcpChannel, + type BridgeOptions, type ChannelFactory, + type HttpAcpBridge, } from './httpAcpBridge.js'; import type { BridgeEvent } from './eventBus.js'; @@ -51,6 +54,17 @@ const WS_A = path.resolve(path.sep, 'work', 'a'); const WS_B = path.resolve(path.sep, 'work', 'b'); const SESS_A = `sess:${WS_A}`; +/** + * Convenience wrapper: `createHttpAcpBridge` now requires `boundWorkspace` + * (per #3803 §02 — 1 daemon = 1 workspace). Tests that only ever talk to + * `WS_A` would otherwise repeat `boundWorkspace: WS_A` everywhere; this + * helper defaults it. Tests that need a different bind path (e.g. the + * mismatch test) pass `boundWorkspace` explicitly. + */ +function makeBridge(opts: Partial = {}): HttpAcpBridge { + return createHttpAcpBridge({ boundWorkspace: WS_A, ...opts }); +} + interface FakeAgentOpts { /** What the fake agent returns from `newSession`. */ sessionIdPrefix?: string; @@ -211,7 +225,7 @@ describe('createHttpAcpBridge', () => { handles.push(h); return h.channel; }; - const bridge = createHttpAcpBridge({ channelFactory: factory }); + const bridge = makeBridge({ channelFactory: factory }); const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); expect(session.sessionId).toBe(SESS_A); @@ -232,7 +246,7 @@ describe('createHttpAcpBridge', () => { handles.push(h); return h.channel; }; - const bridge = createHttpAcpBridge({ channelFactory: factory }); + const bridge = makeBridge({ channelFactory: factory }); const first = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); const second = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); @@ -246,23 +260,38 @@ describe('createHttpAcpBridge', () => { await bridge.shutdown(); }); - it('does NOT reuse across workspaces', async () => { + it('rejects cross-workspace requests with WorkspaceMismatchError (#3803 §02)', async () => { + // Per #3803 §02 (1 daemon = 1 workspace), `spawnOrAttach` calls + // whose canonical `workspaceCwd` doesn't match `boundWorkspace` + // throw `WorkspaceMismatchError`. The server route translates + // this to a 400 with `code: 'workspace_mismatch'` so clients can + // route to (or spawn) a daemon for the other workspace. const handles: ChannelHandle[] = []; const factory: ChannelFactory = async () => { const h = makeChannel(); handles.push(h); return h.channel; }; - const bridge = createHttpAcpBridge({ channelFactory: factory }); + const bridge = makeBridge({ channelFactory: factory }); const a = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); - const b = await bridge.spawnOrAttach({ workspaceCwd: WS_B }); + expect(a.sessionId).toBe(SESS_A); - expect(a.sessionId).not.toBe(b.sessionId); - expect(a.attached).toBe(false); - expect(b.attached).toBe(false); - expect(handles).toHaveLength(2); - expect(bridge.sessionCount).toBe(2); + // Cross-workspace POST throws before touching the channel. + await expect( + bridge.spawnOrAttach({ workspaceCwd: WS_B }), + ).rejects.toBeInstanceOf(WorkspaceMismatchError); + // The error carries both paths for the route's 400 body. + const err = await bridge + .spawnOrAttach({ workspaceCwd: WS_B }) + .catch((e: WorkspaceMismatchError) => e); + expect(err).toBeInstanceOf(WorkspaceMismatchError); + expect((err as WorkspaceMismatchError).bound).toBe(WS_A); + expect((err as WorkspaceMismatchError).requested).toBe(WS_B); + + // Only the original WS_A spawn succeeded — no channel spawned for WS_B. + expect(handles).toHaveLength(1); + expect(bridge.sessionCount).toBe(1); await bridge.shutdown(); }); @@ -274,7 +303,7 @@ describe('createHttpAcpBridge', () => { handles.push(h); return h.channel; }; - const bridge = createHttpAcpBridge({ + const bridge = makeBridge({ sessionScope: 'thread', channelFactory: factory, }); @@ -296,7 +325,7 @@ describe('createHttpAcpBridge', () => { }); it('rejects relative workspace paths', async () => { - const bridge = createHttpAcpBridge({ + const bridge = makeBridge({ channelFactory: async () => { throw new Error('factory should not be called'); }, @@ -313,7 +342,7 @@ describe('createHttpAcpBridge', () => { handles.push(h); return h.channel; }; - const bridge = createHttpAcpBridge({ channelFactory: factory }); + const bridge = makeBridge({ channelFactory: factory }); const a = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); const aNoisy = await bridge.spawnOrAttach({ workspaceCwd: '/work/./a' }); @@ -334,7 +363,7 @@ describe('createHttpAcpBridge', () => { handles.push(h); return h.channel; }; - const bridge = createHttpAcpBridge({ channelFactory: factory }); + const bridge = makeBridge({ channelFactory: factory }); // ACP SDK rewrites unhandled exceptions to a JSON-RPC Internal error // object (code -32603); the original message text is intentionally not @@ -355,7 +384,7 @@ describe('createHttpAcpBridge', () => { handles.push(h); return h.channel; }; - const bridge = createHttpAcpBridge({ + const bridge = makeBridge({ channelFactory: factory, initializeTimeoutMs: 50, }); @@ -367,59 +396,64 @@ describe('createHttpAcpBridge', () => { expect(bridge.sessionCount).toBe(0); }); - it('shutdown kills every live channel', async () => { + it('shutdown kills the live channel and its multiplexed sessions', async () => { + // Stage 1.5 multi-session under single-workspace mode (#3803 §02): + // a daemon hosts one channel with N sessions multiplexed on it. + // Shutdown kills that one channel and tears down every multiplexed + // session. const handles: ChannelHandle[] = []; const factory: ChannelFactory = async () => { - const h = makeChannel(); + const h = makeChannel({ sessionIdPrefix: 's' }); handles.push(h); return h.channel; }; - const bridge = createHttpAcpBridge({ channelFactory: factory }); + const bridge = makeBridge({ + sessionScope: 'thread', + channelFactory: factory, + }); await bridge.spawnOrAttach({ workspaceCwd: WS_A }); - await bridge.spawnOrAttach({ workspaceCwd: WS_B }); + await bridge.spawnOrAttach({ workspaceCwd: WS_A }); expect(bridge.sessionCount).toBe(2); + expect(handles).toHaveLength(1); // one channel multiplexing two sessions await bridge.shutdown(); - expect(handles.every((h) => h.killed)).toBe(true); + expect(handles[0]?.killed).toBe(true); expect(bridge.sessionCount).toBe(0); }); - it('killAllSync force-kills channels even after shutdown cleared byWorkspaceChannel (BkUyD)', async () => { - // tanzhenxin BkUyD regression: shutdown clears - // `byWorkspaceChannel` BEFORE awaiting per-child SIGTERM grace. - // If the operator double-Ctrl+C's during that window, - // killAllSync MUST still see the in-flight-being-killed - // channels. Pre-fix: killAllSync iterated `byWorkspaceChannel` - // and silently no-op'd; children orphaned. Fix: separate - // `liveChannels` set, only emptied on channel.exited. - const killSyncInvoked: number[] = []; - let nextChannelTag = 0; + it('killAllSync force-kills the live channel mid-shutdown (BkUyD)', async () => { + // tanzhenxin BkUyD regression: pre-fix, `shutdown()` cleared the + // live-channel reference BEFORE awaiting the child's SIGTERM + // grace. A mid-drain double-Ctrl+C invoked `killAllSync`, found + // nothing to force-kill, and `process.exit(1)` orphaned the + // child. Under #3803 §02 the bridge has at most one channel, but + // the invariant is the same: `channelInfo` MUST stay set until + // `channel.exited` fires (OS-level reap), not be eagerly cleared + // by `shutdown()`. + const killSyncInvoked: string[] = []; const factory: ChannelFactory = async () => { - const tag = nextChannelTag++; - const h = makeChannel({ sessionIdPrefix: `s${tag}` }); + const h = makeChannel({ sessionIdPrefix: 's' }); const realKillSync = h.channel.killSync; - // Spy on killSync calls so we can assert the force-kill path - // actually fired for every live channel. h.channel = { ...h.channel, kill: () => // Never resolve — simulates a stuck SIGTERM grace window. new Promise(() => {}), killSync: () => { - killSyncInvoked.push(tag); + killSyncInvoked.push('called'); realKillSync(); }, }; return h.channel; }; - const bridge = createHttpAcpBridge({ channelFactory: factory }); + const bridge = makeBridge({ channelFactory: factory }); await bridge.spawnOrAttach({ workspaceCwd: WS_A }); - await bridge.spawnOrAttach({ workspaceCwd: WS_B }); // Kick off shutdown — its `channel.kill()` will hang on the - // never-resolving Promise above, so `byWorkspaceChannel` clears - // but the awaits never finish. This is the mid-drain state. + // never-resolving Promise above, so the entry maps clear but + // the channel-kill await never finishes. This is the mid-drain + // state. const shutdownPromise = bridge.shutdown(); // Yield twice so shutdown's sync prefix runs (clear maps, // publish session_died, start awaits). @@ -429,9 +463,10 @@ describe('createHttpAcpBridge', () => { // Operator double-Ctrl+C arrives now. bridge.killAllSync(); - // Both channels' killSync was invoked. Pre-fix this would have - // been an empty array. - expect(killSyncInvoked).toHaveLength(2); + // The channel's killSync fired. Pre-fix this would have been an + // empty array because `channelInfo` was cleared in shutdown's + // sync prefix. + expect(killSyncInvoked).toHaveLength(1); // Cleanup: the never-resolving kill keeps shutdownPromise // pending forever. Don't await it (would hang the test). The @@ -449,7 +484,7 @@ describe('createHttpAcpBridge', () => { handles.push(h); return h.channel; }; - const bridge = createHttpAcpBridge({ channelFactory: factory }); + const bridge = makeBridge({ channelFactory: factory }); const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); const result = await bridge.sendPrompt(session.sessionId, { @@ -469,7 +504,7 @@ describe('createHttpAcpBridge', () => { handles.push(h); return h.channel; }; - const bridge = createHttpAcpBridge({ channelFactory: factory }); + const bridge = makeBridge({ channelFactory: factory }); const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); await bridge.sendPrompt(session.sessionId, { @@ -506,7 +541,7 @@ describe('createHttpAcpBridge', () => { handles.push(h); return h.channel; }; - const bridge = createHttpAcpBridge({ channelFactory: factory }); + const bridge = makeBridge({ channelFactory: factory }); const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); const p1 = bridge.sendPrompt(session.sessionId, { @@ -551,7 +586,7 @@ describe('createHttpAcpBridge', () => { handles.push(h); return h.channel; }; - const bridge = createHttpAcpBridge({ channelFactory: factory }); + const bridge = makeBridge({ channelFactory: factory }); const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); const failed = await bridge @@ -575,7 +610,7 @@ describe('createHttpAcpBridge', () => { }); it('throws SessionNotFoundError for unknown session ids', async () => { - const bridge = createHttpAcpBridge({ + const bridge = makeBridge({ channelFactory: async () => { throw new Error('factory should not be called'); }, @@ -597,7 +632,7 @@ describe('createHttpAcpBridge', () => { handles.push(h); return h.channel; }; - const bridge = createHttpAcpBridge({ channelFactory: factory }); + const bridge = makeBridge({ channelFactory: factory }); const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); await bridge.cancelSession(session.sessionId); @@ -612,7 +647,7 @@ describe('createHttpAcpBridge', () => { }); it('throws SessionNotFoundError for unknown session ids', async () => { - const bridge = createHttpAcpBridge({ + const bridge = makeBridge({ channelFactory: async () => { throw new Error('factory should not be called'); }, @@ -660,7 +695,7 @@ describe('createHttpAcpBridge', () => { }, }; }; - const bridge = createHttpAcpBridge({ channelFactory: factory }); + const bridge = makeBridge({ channelFactory: factory }); const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); return { bridge, session, conn: capturedConn!, handles }; } @@ -848,7 +883,7 @@ describe('createHttpAcpBridge', () => { }); it('respondToPermission returns false for unknown requestId', async () => { - const bridge = createHttpAcpBridge({ + const bridge = makeBridge({ channelFactory: async () => makeChannel().channel, }); const accepted = bridge.respondToPermission('does-not-exist', { @@ -977,7 +1012,7 @@ describe('createHttpAcpBridge', () => { killSync: () => {}, }; }; - const bridge = createHttpAcpBridge({ channelFactory: factory }); + const bridge = makeBridge({ channelFactory: factory }); const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); // Kick off sendPrompt — agent will issue a permission request @@ -1055,7 +1090,7 @@ describe('createHttpAcpBridge', () => { killSync: () => {}, }; }; - const bridge = createHttpAcpBridge({ channelFactory: factory }); + const bridge = makeBridge({ channelFactory: factory }); return { bridge, setModelCalls }; } @@ -1160,7 +1195,7 @@ describe('createHttpAcpBridge', () => { handles.push(h); return h.channel; }; - const bridge = createHttpAcpBridge({ channelFactory: factory }); + const bridge = makeBridge({ channelFactory: factory }); const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); expect(bridge.sessionCount).toBe(1); @@ -1204,7 +1239,7 @@ describe('createHttpAcpBridge', () => { handles.push(h); return h.channel; }; - const bridge = createHttpAcpBridge({ channelFactory: factory }); + const bridge = makeBridge({ channelFactory: factory }); const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); // No subscribers; planned shutdown removes the entry first, THEN @@ -1252,7 +1287,7 @@ describe('createHttpAcpBridge', () => { killSync: () => {}, }; }; - const bridge = createHttpAcpBridge({ channelFactory: factory }); + const bridge = makeBridge({ channelFactory: factory }); const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A, modelServiceId: 'first', @@ -1332,7 +1367,7 @@ describe('createHttpAcpBridge', () => { killSync: () => {}, }; }; - const bridge = createHttpAcpBridge({ channelFactory: factory }); + const bridge = makeBridge({ channelFactory: factory }); // First call spawns the session AND applies model "A". await bridge.spawnOrAttach({ workspaceCwd: WS_A, @@ -1406,7 +1441,7 @@ describe('createHttpAcpBridge', () => { it('applies modelServiceId on attach via unstable_setSessionModel', async () => { const { factory, setModelCalls } = setupRecording(); - const bridge = createHttpAcpBridge({ channelFactory: factory }); + const bridge = makeBridge({ channelFactory: factory }); // First call spawns; second call attaches with a DIFFERENT model. const first = await bridge.spawnOrAttach({ @@ -1431,7 +1466,7 @@ describe('createHttpAcpBridge', () => { it('attach without modelServiceId does NOT issue setSessionModel', async () => { const { factory, setModelCalls } = setupRecording(); - const bridge = createHttpAcpBridge({ channelFactory: factory }); + const bridge = makeBridge({ channelFactory: factory }); await bridge.spawnOrAttach({ workspaceCwd: WS_A, @@ -1503,7 +1538,7 @@ describe('createHttpAcpBridge', () => { killSync: () => resolveExited!(), }; }; - const bridge = createHttpAcpBridge({ channelFactory: factory }); + const bridge = makeBridge({ channelFactory: factory }); const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); const promptResult = bridge.sendPrompt(session.sessionId, { @@ -1522,17 +1557,17 @@ describe('createHttpAcpBridge', () => { describe('opts validation', () => { it('rejects an invalid sessionScope', () => { expect(() => - createHttpAcpBridge({ + makeBridge({ sessionScope: 'bogus' as unknown as 'single', }), ).toThrow(/Invalid sessionScope/); }); it('rejects a non-positive initializeTimeoutMs', () => { - expect(() => createHttpAcpBridge({ initializeTimeoutMs: 0 })).toThrow( + expect(() => makeBridge({ initializeTimeoutMs: 0 })).toThrow( /initializeTimeoutMs/, ); - expect(() => createHttpAcpBridge({ initializeTimeoutMs: -1 })).toThrow( + expect(() => makeBridge({ initializeTimeoutMs: -1 })).toThrow( /initializeTimeoutMs/, ); }); @@ -1541,17 +1576,13 @@ describe('createHttpAcpBridge', () => { // A typo / parse error in CLI / config that yields NaN must // NOT silently disable the daemon's resource cap. We fail // boot loud instead of serving unbounded. - expect(() => createHttpAcpBridge({ maxSessions: NaN })).toThrow( + expect(() => makeBridge({ maxSessions: NaN })).toThrow( /maxSessions: NaN/, ); - expect(() => createHttpAcpBridge({ maxSessions: -5 })).toThrow( - /maxSessions: -5/, - ); + expect(() => makeBridge({ maxSessions: -5 })).toThrow(/maxSessions: -5/); // Explicit zero or Infinity remain valid "unlimited" sentinels. - expect(() => createHttpAcpBridge({ maxSessions: 0 })).not.toThrow(); - expect(() => - createHttpAcpBridge({ maxSessions: Infinity }), - ).not.toThrow(); + expect(() => makeBridge({ maxSessions: 0 })).not.toThrow(); + expect(() => makeBridge({ maxSessions: Infinity })).not.toThrow(); }); }); @@ -1565,7 +1596,7 @@ describe('createHttpAcpBridge', () => { await new Promise((r) => setTimeout(r, 10)); return makeChannel().channel; }; - const bridge = createHttpAcpBridge({ channelFactory: factory }); + const bridge = makeBridge({ channelFactory: factory }); const [a, b] = await Promise.all([ bridge.spawnOrAttach({ workspaceCwd: WS_A }), @@ -1595,7 +1626,7 @@ describe('createHttpAcpBridge', () => { } return makeChannel().channel; }; - const bridge = createHttpAcpBridge({ channelFactory: factory }); + const bridge = makeBridge({ channelFactory: factory }); await expect( bridge.spawnOrAttach({ workspaceCwd: WS_A }), @@ -1636,7 +1667,7 @@ describe('createHttpAcpBridge', () => { killSync: () => {}, }; }; - const bridge = createHttpAcpBridge({ channelFactory: factory }); + const bridge = makeBridge({ channelFactory: factory }); const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); return { bridge, session, conn: capturedConn! }; } @@ -1955,7 +1986,7 @@ describe('createHttpAcpBridge', () => { }); describe('listWorkspaceSessions', () => { - it('returns sessions matching the canonical workspace cwd', async () => { + it('returns sessions matching the bound workspace cwd', async () => { let n = 0; const factory: ChannelFactory = async () => { // Distinct sessionIdPrefix per spawn so two thread-scope sessions @@ -1964,22 +1995,24 @@ describe('createHttpAcpBridge', () => { const h = makeChannel({ sessionIdPrefix: `s${n++}` }); return h.channel; }; - const bridge = createHttpAcpBridge({ + const bridge = makeBridge({ sessionScope: 'thread', channelFactory: factory, }); const a1 = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); const a2 = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); - await bridge.spawnOrAttach({ workspaceCwd: WS_B }); const aList = bridge.listWorkspaceSessions(WS_A); expect(aList).toHaveLength(2); expect(aList.map((s) => s.sessionId).sort()).toEqual( [a1.sessionId, a2.sessionId].sort(), ); + // Querying a different workspace returns an empty list (the + // bridge only hosts `boundWorkspace` per #3803 §02; a UI asking + // for sessions in some other path is correct to see "none"). const bList = bridge.listWorkspaceSessions(WS_B); - expect(bList).toHaveLength(1); + expect(bList).toEqual([]); const idleList = bridge.listWorkspaceSessions('/work/c'); expect(idleList).toEqual([]); @@ -1988,7 +2021,7 @@ describe('createHttpAcpBridge', () => { it('canonicalizes the lookup path', async () => { const factory: ChannelFactory = async () => makeChannel().channel; - const bridge = createHttpAcpBridge({ channelFactory: factory }); + const bridge = makeBridge({ channelFactory: factory }); await bridge.spawnOrAttach({ workspaceCwd: WS_A }); const list = bridge.listWorkspaceSessions('/work/./a'); @@ -1999,7 +2032,7 @@ describe('createHttpAcpBridge', () => { }); it('returns empty for relative paths instead of throwing', async () => { - const bridge = createHttpAcpBridge({ + const bridge = makeBridge({ channelFactory: async () => { throw new Error('factory should not be called'); }, @@ -2046,7 +2079,7 @@ describe('createHttpAcpBridge', () => { killSync: () => {}, }; }; - const bridge = createHttpAcpBridge({ channelFactory: factory }); + const bridge = makeBridge({ channelFactory: factory }); const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); return { bridge, session, setModelCalls }; } @@ -2085,7 +2118,7 @@ describe('createHttpAcpBridge', () => { }); it('throws SessionNotFoundError for unknown session ids', async () => { - const bridge = createHttpAcpBridge({ + const bridge = makeBridge({ channelFactory: async () => { throw new Error('factory should not be called'); }, @@ -2101,7 +2134,7 @@ describe('createHttpAcpBridge', () => { describe('subscribeEvents', () => { it('throws SessionNotFoundError for unknown session ids', () => { - const bridge = createHttpAcpBridge({ + const bridge = makeBridge({ channelFactory: async () => { throw new Error('factory should not be called'); }, @@ -2132,7 +2165,7 @@ describe('createHttpAcpBridge', () => { killSync: () => {}, }; }; - const bridge = createHttpAcpBridge({ channelFactory: factory }); + const bridge = makeBridge({ channelFactory: factory }); const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); const abort = new AbortController(); @@ -2163,7 +2196,7 @@ describe('createHttpAcpBridge', () => { it('shutdown closes live event subscriptions', async () => { const factory: ChannelFactory = async () => makeChannel().channel; - const bridge = createHttpAcpBridge({ channelFactory: factory }); + const bridge = makeBridge({ channelFactory: factory }); const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); const abort = new AbortController(); @@ -2195,8 +2228,10 @@ describe('createHttpAcpBridge', () => { describe('maxSessions cap (chiga0 Rec 3)', () => { it('refuses NEW spawns past the cap with SessionLimitExceededError', async () => { - const factory: ChannelFactory = async () => makeChannel().channel; - const bridge = createHttpAcpBridge({ + let n = 0; + const factory: ChannelFactory = async () => + makeChannel({ sessionIdPrefix: `s${n++}` }).channel; + const bridge = makeBridge({ channelFactory: factory, maxSessions: 2, // `thread` so each call is a fresh session, not an attach. @@ -2205,7 +2240,7 @@ describe('createHttpAcpBridge', () => { // First two spawns succeed. await bridge.spawnOrAttach({ workspaceCwd: WS_A }); - await bridge.spawnOrAttach({ workspaceCwd: WS_B }); + await bridge.spawnOrAttach({ workspaceCwd: WS_A }); expect(bridge.sessionCount).toBe(2); // Third hits the cap. @@ -2223,7 +2258,7 @@ describe('createHttpAcpBridge', () => { it('attach to an existing session under single scope is NOT counted toward the cap', async () => { const factory: ChannelFactory = async () => makeChannel().channel; - const bridge = createHttpAcpBridge({ + const bridge = makeBridge({ channelFactory: factory, maxSessions: 1, sessionScope: 'single', @@ -2240,12 +2275,11 @@ describe('createHttpAcpBridge', () => { expect(b.sessionId).toBe(a.sessionId); expect(bridge.sessionCount).toBe(1); - // But a NEW workspace (would need a fresh spawn) is rejected. + // A cross-workspace request rejects with WorkspaceMismatchError + // (#3803 §02) — the bridge is bound to one workspace. await expect( bridge.spawnOrAttach({ workspaceCwd: WS_B }), - ).rejects.toMatchObject({ - name: 'SessionLimitExceededError', - }); + ).rejects.toBeInstanceOf(WorkspaceMismatchError); await bridge.shutdown(); }); @@ -2256,7 +2290,7 @@ describe('createHttpAcpBridge', () => { // for the same workspace and gets attached:true. Without the // race guard, A's reaper would tear down B's session. const factory: ChannelFactory = async () => makeChannel().channel; - const bridge = createHttpAcpBridge({ + const bridge = makeBridge({ channelFactory: factory, sessionScope: 'single', }); @@ -2288,7 +2322,7 @@ describe('createHttpAcpBridge', () => { }); return makeChannel().channel; }; - const bridge = createHttpAcpBridge({ + const bridge = makeBridge({ channelFactory: slowFactory, sessionScope: 'single', }); @@ -2321,7 +2355,7 @@ describe('createHttpAcpBridge', () => { // when the spawn owner ALSO indicated they want it (via the // killSession-bail tombstone). const factory: ChannelFactory = async () => makeChannel().channel; - const bridge = createHttpAcpBridge({ + const bridge = makeBridge({ channelFactory: factory, sessionScope: 'single', }); @@ -2345,7 +2379,7 @@ describe('createHttpAcpBridge', () => { // set during the spawn-owner bail, B's detach now completes // the deferred reap. const factory: ChannelFactory = async () => makeChannel().channel; - const bridge = createHttpAcpBridge({ + const bridge = makeBridge({ channelFactory: factory, sessionScope: 'single', }); @@ -2369,7 +2403,7 @@ describe('createHttpAcpBridge', () => { // Counterpart: when client C is actively subscribed, detach // from a transient B must NOT reap C's session. const factory: ChannelFactory = async () => makeChannel().channel; - const bridge = createHttpAcpBridge({ + const bridge = makeBridge({ channelFactory: factory, sessionScope: 'single', }); @@ -2400,7 +2434,7 @@ describe('createHttpAcpBridge', () => { // negative path so a future change can't accidentally make // it always-skip. const factory: ChannelFactory = async () => makeChannel().channel; - const bridge = createHttpAcpBridge({ + const bridge = makeBridge({ channelFactory: factory, sessionScope: 'single', }); @@ -2420,7 +2454,7 @@ describe('createHttpAcpBridge', () => { let n = 0; const factory: ChannelFactory = async () => makeChannel({ sessionIdPrefix: `s${n++}` }).channel; - const bridge = createHttpAcpBridge({ + const bridge = makeBridge({ channelFactory: factory, maxSessions: 0, sessionScope: 'thread', @@ -2449,7 +2483,7 @@ describe('createHttpAcpBridge', () => { factoryCalls++; return makeChannel({ sessionIdPrefix: `s${factoryCalls}` }).channel; }; - const bridge = createHttpAcpBridge({ + const bridge = makeBridge({ channelFactory: factory, maxSessions: 0, sessionScope: 'thread', @@ -2478,7 +2512,7 @@ describe('createHttpAcpBridge', () => { handles.push(h); return h.channel; }; - const bridge = createHttpAcpBridge({ + const bridge = makeBridge({ channelFactory: factory, sessionScope: 'thread', }); @@ -2512,7 +2546,7 @@ describe('createHttpAcpBridge', () => { handles.push(h); return h.channel; }; - const bridge = createHttpAcpBridge({ + const bridge = makeBridge({ channelFactory: factory, sessionScope: 'thread', }); diff --git a/packages/cli/src/serve/httpAcpBridge.ts b/packages/cli/src/serve/httpAcpBridge.ts index 10b0a773731..777cf1210f5 100644 --- a/packages/cli/src/serve/httpAcpBridge.ts +++ b/packages/cli/src/serve/httpAcpBridge.ts @@ -40,12 +40,18 @@ import type { /** * Stage 1 HTTP→ACP bridge. * - * Per design §08 (Roadmap, Stage 1) and the issue body's Caveat: - * - One `qwen --acp` child PER WORKSPACE; multiple sessions on the same - * workspace multiplex onto that child via `connection.newSession()` - * (the agent's native `sessions: Map` — see - * `acp-integration/acpAgent.ts:194`). Sessions share the child's - * process / OAuth state / `FileReadCache` / hierarchy-memory parse. + * Per #3803 §02 (architectural revision) and design §08 (Roadmap, Stage 1): + * - **1 daemon = 1 workspace**: every bridge instance is bound to a + * single canonical workspace path at construction + * (`BridgeOptions.boundWorkspace`). All `spawnOrAttach` calls must + * target that workspace; cross-workspace requests throw + * `WorkspaceMismatchError`. Multi-workspace deployments use multiple + * daemon processes (one per workspace, supervised externally). + * - One `qwen --acp` child total; multiple sessions multiplex onto it + * via `connection.newSession()` (the agent's native + * `sessions: Map` — see `acp-integration/acpAgent.ts:194`). + * Sessions share the child's process / OAuth state / `FileReadCache` / + * hierarchy-memory parse. * - HTTP request bodies are forwarded as ACP NDJSON over the child's stdin. * - Child stdout NDJSON notifications publish onto each session's * `EventBus`; HTTP SSE subscribers (`GET /session/:id/events`) drain @@ -54,10 +60,6 @@ import type { * bridge (FIFO; honors ACP's "one active prompt per session" invariant). * Different sessions on the same channel can prompt concurrently — * the ACP layer demultiplexes by sessionId. - * - Cross-workspace channel sharing is intentionally NOT done. Different - * workspaces have different `loadSettings(cwd)` state; one child would - * step on the previous workspace's settings. One channel per workspace - * is the safe scope. * * Stage 2 replaces the spawn step with an in-process call into core's * ACP-equivalent API. The `HttpAcpBridge` interface stays the same so HTTP @@ -244,6 +246,32 @@ export class SessionLimitExceededError extends Error { } } +/** + * Thrown by `spawnOrAttach` when the requested `workspaceCwd` doesn't + * canonicalize to the daemon's bound workspace. Per #3803 §02 every + * bridge instance is bound to exactly one workspace; cross-workspace + * requests are rejected at the daemon boundary. The server route + * translates this to a 400 response with `code: 'workspace_mismatch'` + * and both paths in the body so clients can fall through to spawning + * their own daemon / routing to a different one via an orchestrator. + */ +export class WorkspaceMismatchError extends Error { + readonly bound: string; + readonly requested: string; + constructor(bound: string, requested: string) { + super( + `Workspace mismatch: daemon is bound to "${bound}" but ` + + `request asked for "${requested}". Each \`qwen serve\` ` + + `daemon binds to exactly one workspace; start a separate ` + + `daemon for "${requested}" (or route the request to one ` + + `via an orchestrator).`, + ); + this.name = 'WorkspaceMismatchError'; + this.bound = bound; + this.requested = requested; + } +} + /** * One ACP NDJSON channel to a single agent. Tests inject a fake by replacing * the channel factory; production uses `defaultSpawnChannelFactory`. @@ -265,8 +293,8 @@ export interface AcpChannel { * Resolves when the channel has terminated for any reason — planned * (`kill()` called) OR unexpected (child process crashed, stream closed). * The bridge subscribes to this so a SessionEntry whose underlying - * channel dies between requests is removed from `byWorkspace`/`byId` - * instead of lingering as a stuck session. + * channel dies between requests is removed from `byId` / + * `defaultEntry` instead of lingering as a stuck session. * * Resolves to `{ exitCode, signalCode }` when the spawn factory can * capture them (the standard `child.on('exit', code, signal)` path), @@ -348,25 +376,32 @@ export interface BridgeOptions { * cap. */ maxPendingPermissionsPerSession?: number; + /** + * Absolute path this daemon is bound to (per #3803 §02: 1 daemon = + * 1 workspace). `spawnOrAttach` calls whose `workspaceCwd` doesn't + * canonicalize to this same value throw `WorkspaceMismatchError` + * (route → 400 with code `workspace_mismatch`). + * + * The bound path is canonicalized once at boot time so subsequent + * request comparisons are canonical-vs-canonical (mixed casing / + * symlink resolution / etc. already accounted for). + */ + boundWorkspace: string; } /** - * One `qwen --acp` child + the ACP connection on top of it, shared by - * all SessionEntries whose workspace maps to this channel. Stage 1.5 - * multi-session work (per LaZzyMan / tanzhenxin reviews) leverages - * the agent's native `sessions: Map` (see - * `acp-integration/acpAgent.ts:194`) so multiple `newSession()` calls - * on one channel get separate session ids while sharing the child's - * process / OAuth / file-cache / hierarchy-memory parse. + * The single `qwen --acp` child + the ACP connection on top of it, + * shared by every SessionEntry in this daemon. Per #3803 §02 the + * bridge is bound to one workspace at construction, so there is at + * most one channel alive at any moment. Multiple sessions multiplex + * onto it via the agent's native `sessions: Map` + * (see `acp-integration/acpAgent.ts:194`), each `newSession()` call + * returning a distinct id while sharing the child's process / OAuth / + * file-cache / hierarchy-memory parse. * - * Lifetime: created on first `spawnOrAttach` for a workspace, kept - * alive while `sessionIds.size > 0`, and killed by `killSession` when - * the last entry leaves OR by `channel.exited` when the child dies. - * Cross-workspace channel sharing is intentionally NOT done in this - * bridge — `acpAgent.ts:601 (this.settings = loadSettings(cwd))` - * replaces the cached settings on each newSession call, so different - * workspaces in one child would step on each other's settings. One - * channel per workspace is the safe scope for Stage 1.5. + * Lifetime: created on first `spawnOrAttach`, kept alive while + * `sessionIds.size > 0`, and killed by `killSession` when the last + * entry leaves OR by `channel.exited` when the child dies. */ interface ChannelInfo { channel: AcpChannel; @@ -378,8 +413,8 @@ interface ChannelInfo { * Live session ids multiplexed on this channel. Updated when * `doSpawn` registers a new session and when `killSession` / * `channel.exited` removes one. When the set drops to empty AND no - * session is mid-attach, the channel is killed and removed from - * `byWorkspaceChannel`. + * session is mid-attach, the channel is killed and `channelInfo` + * is cleared. */ sessionIds: Set; } @@ -879,7 +914,7 @@ const DEFAULT_PERMISSION_TIMEOUT_MS = 5 * 60 * 1000; // `BridgeOptions.maxPendingPermissionsPerSession`. const DEFAULT_MAX_PENDING_PER_SESSION = 64; -export function createHttpAcpBridge(opts: BridgeOptions = {}): HttpAcpBridge { +export function createHttpAcpBridge(opts: BridgeOptions): HttpAcpBridge { const sessionScope = opts.sessionScope ?? 'single'; // `undefined` → default 20 (intentionally tight per #3803 N≈50 cliff). // `0` → explicitly unlimited (operator opt-out). @@ -936,38 +971,35 @@ export function createHttpAcpBridge(opts: BridgeOptions = {}): HttpAcpBridge { maxPendingRaw > 0 && Number.isFinite(maxPendingRaw) ? maxPendingRaw : Infinity; - - // Single-scope reuse keyed by canonical workspace path. Tracks the - // SessionEntry that a same-workspace attach should re-use. With - // Stage 1.5 multi-session per channel, this points at the FIRST - // session created for the workspace under `single` scope; under - // `thread` scope additional sessions on the same workspace don't - // overwrite this entry. - const byWorkspace = new Map(); - // Stage 1.5 multi-session: one channel per workspace, N sessions - // multiplex on it via `connection.newSession({cwd, mcpServers})`. - // `byWorkspaceChannel.get(workspaceKey)` returns the shared channel - // for spawn-vs-reuse decisions in `doSpawn`. Channel is kept alive - // while `sessionIds.size > 0`; the last `killSession` (or the - // `channel.exited` cleanup) drops the entry from this map. - const byWorkspaceChannel = new Map(); - // tanzhenxin BkUyD: source of truth for "channels with potentially- - // alive child processes" — independent of `byWorkspaceChannel`, - // which `shutdown()` clears BEFORE awaiting per-child SIGTERM- - // grace kills. `killAllSync()` (the double-Ctrl+C force-exit - // path) iterates THIS set so a mid-shutdown second signal still - // sees the children that haven't yet finished their SIGTERM grace. - // Only removed when `channel.exited` fires (the OS-level "really - // dead" signal). The earlier design iterated `byWorkspaceChannel` - // and silently no-op'd during the shutdown await window. - const liveChannels = new Set(); - // Coalesces concurrent channel-spawn requests for the same workspace - // (regardless of sessionScope). Without this, two parallel callers - // would both `channelFactory(workspaceKey)` and one of the - // spawned children would never make it into `byWorkspaceChannel`, - // becoming a permanent orphan. Cleared in the `finally` of the - // creator regardless of outcome. - const inFlightChannelSpawns = new Map>(); + // #3803 §02: canonicalize the bound path once at boot so subsequent + // `spawnOrAttach` comparisons are canonical-vs-canonical (mixed + // casing / symlink resolution / etc. already accounted for). + if (!path.isAbsolute(opts.boundWorkspace)) { + throw new TypeError( + `Invalid boundWorkspace: "${opts.boundWorkspace}". Must be an ` + + `absolute path.`, + ); + } + const boundWorkspace = canonicalizeWorkspace(opts.boundWorkspace); + + // #3803 §02 single-workspace model: the bridge hosts AT MOST one + // channel and one default attach-target entry. Multi-session + // multiplexing happens through `channelInfo.sessionIds`; the + // `defaultEntry` slot is the FIRST session created (the one a + // same-workspace attach under `single` scope reuses). Thread-scope + // sessions add to `byId` but don't displace `defaultEntry`. + let defaultEntry: SessionEntry | undefined; + let channelInfo: ChannelInfo | undefined; + // tanzhenxin BkUyD: the channel reference is cleared ONLY when + // `channel.exited` fires (OS-level "really dead" signal), NOT + // eagerly during `shutdown()`. The double-Ctrl+C force-exit path + // (`killAllSync`) needs the reference until the OS has reaped the + // child — otherwise a second signal mid-SIGTERM-grace finds + // nothing to force-kill and `process.exit(1)` orphans the child. + // Coalesces a concurrent second `ensureChannel()` call onto the + // first one's spawn so we never create two children for the same + // daemon. Cleared in the `finally` of the creator. + let inFlightChannelSpawn: Promise | undefined; const byId = new Map(); // Daemon-wide pending permission table; requestIds are UUIDs so collisions // across sessions are infeasible in practice. @@ -981,11 +1013,13 @@ export function createHttpAcpBridge(opts: BridgeOptions = {}): HttpAcpBridge { // (b) `server.close` rejecting new connections, during which a // late-arriving `POST /session` slips a fresh child past cleanup. let shuttingDown = false; - // Coalesces concurrent `spawnOrAttach` calls for the same workspace under - // single-scope. Without this, two parallel callers would both pass the - // `byWorkspace.get` check, both spawn, and one entry would be orphaned - // (in `byId` but not in `byWorkspace`) — violating the - // "at most one session per workspace" invariant. + // Coalesces concurrent `spawnOrAttach` calls under single-scope and + // tracks in-progress thread-scope spawns for shutdown to await. + // Single-scope uses the workspaceKey as the dedup key (at most one + // entry; concurrent callers pass the `defaultEntry` check together + // and coalesce here). Thread-scope uses `workspaceKey#uuid` so + // simultaneous calls don't collide while still being awaitable from + // `shutdown()`. const inFlightSpawns = new Map>(); const registerPending = (p: PendingPermission) => { @@ -1033,28 +1067,23 @@ export function createHttpAcpBridge(opts: BridgeOptions = {}): HttpAcpBridge { }; /** - * Get-or-create the shared `qwen --acp` channel for a workspace. - * Stage 1.5 multi-session: one channel hosts N sessions via - * `connection.newSession()`. Concurrent callers coalesce through - * `inFlightChannelSpawns` so we never spawn two children for one - * workspace. The returned `ChannelInfo` is shared — caller adds - * their session id to `sessionIds` and uses `info.connection.newSession()`. + * Get-or-create the daemon's single `qwen --acp` channel (#3803 §02). + * N sessions multiplex onto it via `connection.newSession()`. + * Concurrent callers coalesce through `inFlightChannelSpawn` so we + * never spawn two children. The returned `ChannelInfo` is shared — + * the caller adds their session id to `sessionIds` and uses + * `info.connection.newSession()`. * * Wires up the one-and-only `channel.exited` cleanup on first - * creation so the late-arriving event tears down ALL sessions on - * the channel (vs. the previous 1-session-per-channel design where - * each entry registered its own listener). + * creation so the late-arriving event tears down ALL multiplexed + * sessions. */ - async function getOrCreateChannel( - workspaceKey: string, - ): Promise { - const existing = byWorkspaceChannel.get(workspaceKey); - if (existing) return existing; - const inFlight = inFlightChannelSpawns.get(workspaceKey); - if (inFlight) return await inFlight; + async function ensureChannel(): Promise { + if (channelInfo) return channelInfo; + if (inFlightChannelSpawn) return await inFlightChannelSpawn; const promise = (async () => { - const channel = await channelFactory(workspaceKey); + const channel = await channelFactory(boundWorkspace); const client = new BridgeClient( // BfFut: ACP today carries a sessionId on every per-session // notification / request, so the no-sessionId branch is @@ -1064,12 +1093,11 @@ export function createHttpAcpBridge(opts: BridgeOptions = {}): HttpAcpBridge { // instead of throwing. Surface that ambiguity loudly. (sessionId) => { if (sessionId) return byId.get(sessionId); - const info = byWorkspaceChannel.get(workspaceKey); - if (info && info.sessionIds.size > 1) { + if (channelInfo && channelInfo.sessionIds.size > 1) { throw new Error( 'BridgeClient: ACP call without sessionId on a ' + 'multi-session channel cannot be routed — workspace=' + - workspaceKey, + boundWorkspace, ); } return undefined; @@ -1112,27 +1140,26 @@ export function createHttpAcpBridge(opts: BridgeOptions = {}): HttpAcpBridge { channel, connection, client, - workspaceCwd: workspaceKey, + workspaceCwd: boundWorkspace, sessionIds: new Set(), }; - byWorkspaceChannel.set(workspaceKey, info); - liveChannels.add(info); + channelInfo = info; // One-time channel.exited cleanup. The child dying takes ALL // multiplexed sessions with it — iterate `sessionIds` (snapshot // first to be safe against concurrent killSession during // iteration), publish `session_died` on each session's bus, - // remove from byId / byWorkspace / pending tables. + // remove from byId / defaultEntry / pending tables. + // + // tanzhenxin BkUyD: this is also where `channelInfo` is cleared. + // The async kill paths (`killSession` reap, `shutdown()` await, + // `doSpawn`'s newSession-failure tear-down) all leave + // `channelInfo` set until the OS has reaped the child, so the + // double-Ctrl+C `killAllSync` force-kill path still has a + // reference to fire SIGKILL against during the SIGTERM grace + // window. void channel.exited.then((exitInfo) => { - // tanzhenxin BkUyD: drop from `liveChannels` ONLY when the - // OS process is actually gone. Async kill paths - // (`killSession` reap, `shutdown()` await) remove from - // `byWorkspaceChannel` early but the child's SIGTERM grace - // can still be in-flight; the force-kill path needs the - // entry until `channel.exited` fires here. - liveChannels.delete(info); - const stillOurs = byWorkspaceChannel.get(workspaceKey) === info; - if (stillOurs) byWorkspaceChannel.delete(workspaceKey); + if (channelInfo === info) channelInfo = undefined; const sessions = Array.from(info.sessionIds); info.sessionIds.clear(); for (const sid of sessions) { @@ -1154,9 +1181,7 @@ export function createHttpAcpBridge(opts: BridgeOptions = {}): HttpAcpBridge { /* bus already closed */ } byId.delete(sid); - if (byWorkspace.get(sessEntry.workspaceCwd) === sessEntry) { - byWorkspace.delete(sessEntry.workspaceCwd); - } + if (defaultEntry === sessEntry) defaultEntry = undefined; sessEntry.events.close(); } }); @@ -1164,23 +1189,21 @@ export function createHttpAcpBridge(opts: BridgeOptions = {}): HttpAcpBridge { return info; })(); - inFlightChannelSpawns.set(workspaceKey, promise); + inFlightChannelSpawn = promise; try { return await promise; } finally { - inFlightChannelSpawns.delete(workspaceKey); + inFlightChannelSpawn = undefined; } } - async function doSpawn( - workspaceKey: string, - modelServiceId?: string, - ): Promise { - // Stage 1.5 multi-session: get-or-create the channel for this - // workspace, then call `connection.newSession()` on it. Sessions - // share the child's process / OAuth / file-cache / hierarchy- - // memory parse via the agent's `sessions: Map` - // (see `acp-integration/acpAgent.ts:194`). + async function doSpawn(modelServiceId?: string): Promise { + // #3803 §02: get-or-create the daemon's single channel, then call + // `connection.newSession()` on it. Sessions share the child's + // process / OAuth / file-cache / hierarchy-memory parse via the + // agent's `sessions: Map` (see + // `acp-integration/acpAgent.ts:194`). + // // newSession on an established channel can fail (auth, config, // etc.) without the channel dying. We DON'T kill the channel on // newSession failure when OTHER sessions are still using it — @@ -1188,17 +1211,17 @@ export function createHttpAcpBridge(opts: BridgeOptions = {}): HttpAcpBridge { // // BkwQA: when the failed newSession was the channel's ONLY // attempt (sessionIds.size === 0), the empty channel must NOT - // linger — it would sit in `byWorkspaceChannel` invisible to + // linger — it would stay set as `channelInfo` invisible to // `sessionCount` / `maxSessions` (both backed by `byId`), and // repeated failing creates would still find this channel via - // `getOrCreateChannel`, never spawning a fresh one. Tear down - // the empty channel so the next attempt gets a clean spawn. - const channelInfo = await getOrCreateChannel(workspaceKey); + // `ensureChannel`, never spawning a fresh one. Tear down the + // empty channel so the next attempt gets a clean spawn. + const ci = await ensureChannel(); let newSessionResp: { sessionId: string }; try { newSessionResp = await withTimeout( - channelInfo.connection.newSession({ - cwd: workspaceKey, + ci.connection.newSession({ + cwd: boundWorkspace, mcpServers: [], }), initTimeoutMs, @@ -1208,11 +1231,9 @@ export function createHttpAcpBridge(opts: BridgeOptions = {}): HttpAcpBridge { // Only reap when this newSession was the channel's first/only // attempt — a populated channel keeps running for its other // live sessions. - if (channelInfo.sessionIds.size === 0) { - if (byWorkspaceChannel.get(workspaceKey) === channelInfo) { - byWorkspaceChannel.delete(workspaceKey); - } - await channelInfo.channel.kill().catch(() => { + if (ci.sessionIds.size === 0) { + if (channelInfo === ci) channelInfo = undefined; + await ci.channel.kill().catch(() => { /* best-effort — channel.exited handler still runs */ }); } @@ -1228,9 +1249,9 @@ export function createHttpAcpBridge(opts: BridgeOptions = {}): HttpAcpBridge { const entry: SessionEntry = { sessionId: newSessionResp.sessionId, - workspaceCwd: workspaceKey, - channel: channelInfo.channel, - connection: channelInfo.connection, + workspaceCwd: boundWorkspace, + channel: ci.channel, + connection: ci.connection, events: new EventBus(), promptQueue: Promise.resolve(), modelChangeQueue: Promise.resolve(), @@ -1238,14 +1259,12 @@ export function createHttpAcpBridge(opts: BridgeOptions = {}): HttpAcpBridge { attachCount: 0, spawnOwnerWantedKill: false, }; - channelInfo.sessionIds.add(entry.sessionId); + ci.sessionIds.add(entry.sessionId); byId.set(entry.sessionId, entry); - // `byWorkspace` is the single-scope attach lookup — only the - // FIRST session for a workspace wins this slot. Subsequent - // thread-scope sessions don't overwrite it. - if (!byWorkspace.has(workspaceKey)) { - byWorkspace.set(workspaceKey, entry); - } + // `defaultEntry` is the single-scope attach target — only the + // FIRST session wins this slot. Subsequent thread-scope sessions + // don't overwrite it. + if (!defaultEntry) defaultEntry = entry; // ACP `newSession` doesn't take a model id; honor the caller's // `modelServiceId` via `unstable_setSessionModel`. See @@ -1435,9 +1454,15 @@ export function createHttpAcpBridge(opts: BridgeOptions = {}): HttpAcpBridge { ); } const workspaceKey = canonicalizeWorkspace(req.workspaceCwd); + // #3803 §02: reject cross-workspace requests at the daemon + // boundary. The route layer catches `WorkspaceMismatchError` + // and translates to 400 with `code: 'workspace_mismatch'`. + if (workspaceKey !== boundWorkspace) { + throw new WorkspaceMismatchError(boundWorkspace, workspaceKey); + } if (sessionScope === 'single') { - const existing = byWorkspace.get(workspaceKey); + const existing = defaultEntry; if (existing) { // BRSCi: bump attach counter BEFORE any await so the // spawn-owner's disconnect reaper (server.ts: @@ -1540,7 +1565,7 @@ export function createHttpAcpBridge(opts: BridgeOptions = {}): HttpAcpBridge { throw new SessionLimitExceededError(maxSessions); } - const promise = doSpawn(workspaceKey, req.modelServiceId); + const promise = doSpawn(req.modelServiceId); // Track in-flight spawns regardless of scope. Under `single` // this also serves the coalescing path above (a parallel // `spawnOrAttach` finds the entry and waits for the same @@ -1850,18 +1875,16 @@ export function createHttpAcpBridge(opts: BridgeOptions = {}): HttpAcpBridge { entry.spawnOwnerWantedKill = true; return; } - // Remove from the maps eagerly so concurrent `spawnOrAttach` + // Remove from the state eagerly so concurrent `spawnOrAttach` // can't reattach to a session we're tearing down. - if (byWorkspace.get(entry.workspaceCwd) === entry) { - byWorkspace.delete(entry.workspaceCwd); - } + if (defaultEntry === entry) defaultEntry = undefined; byId.delete(sessionId); - // Stage 1.5 multi-session: detach from the channel. The channel - // dies only when its LAST session leaves — other sessions on - // the same channel keep running. - const channelInfo = byWorkspaceChannel.get(entry.workspaceCwd); - if (channelInfo && channelInfo.channel === entry.channel) { - channelInfo.sessionIds.delete(sessionId); + // Detach from the channel. The channel dies only when its LAST + // session leaves — other sessions on the same channel keep + // running. + const ci = channelInfo; + if (ci && ci.channel === entry.channel) { + ci.sessionIds.delete(sessionId); } // Resolve any still-pending permission as cancelled (matches the // shutdown path) so callers awaiting requestPermission unwind. @@ -1889,10 +1912,11 @@ export function createHttpAcpBridge(opts: BridgeOptions = {}): HttpAcpBridge { // 20) so memory is capped. FIXME(stage-1.5): if ACP grows a // `closeSession` notification, send it here so the agent can // drop the entry from its map immediately rather than at - // channel exit. - if (channelInfo && channelInfo.sessionIds.size === 0) { - byWorkspaceChannel.delete(entry.workspaceCwd); - await channelInfo.channel.kill().catch(() => { + // channel exit. (`channelInfo` itself is cleared by the + // `channel.exited` handler once the OS reaps the child — + // tanzhenxin BkUyD invariant.) + if (ci && ci.sessionIds.size === 0) { + await ci.channel.kill().catch(() => { // Best-effort kill — channel may already be dead. }); } @@ -1933,26 +1957,19 @@ export function createHttpAcpBridge(opts: BridgeOptions = {}): HttpAcpBridge { }, killAllSync() { - // Bd1y6: synchronous best-effort SIGKILL on every live channel. + // Bd1y6: synchronous best-effort SIGKILL on the live channel. // Set `shuttingDown` so any racing async path fails fast. - // tanzhenxin BkUyD fix: iterate `liveChannels` (the OS-level - // source of truth) NOT `byWorkspaceChannel`. The latter is - // cleared by `shutdown()` BEFORE awaiting per-child SIGTERM - // grace; if the operator double-Ctrl+C's during that window, - // iterating `byWorkspaceChannel` would find nothing and - // `process.exit(1)` would orphan children still inside their - // SIGTERM grace. `liveChannels` only loses an entry when - // `channel.exited` fires (OS exit), so the force-kill path - // catches every still-alive child regardless of where the - // graceful drain is. + // tanzhenxin BkUyD: read `channelInfo` directly — it's only + // cleared by `channel.exited` (OS-level reap), NOT eagerly by + // `shutdown()`, so a mid-SIGTERM-grace double-signal still + // finds the live reference and force-kills. shuttingDown = true; - const channels = Array.from(liveChannels); - byWorkspaceChannel.clear(); - byWorkspace.clear(); + const ci = channelInfo; + defaultEntry = undefined; byId.clear(); - for (const info of channels) { + if (ci) { try { - info.channel.killSync(); + ci.channel.killSync(); } catch { /* best-effort — already-dead child / pid race */ } @@ -1966,11 +1983,12 @@ export function createHttpAcpBridge(opts: BridgeOptions = {}): HttpAcpBridge { // spawning a child this teardown won't see. shuttingDown = true; const entries = Array.from(byId.values()); - // Snapshot channels too — Stage 1.5 multi-session means N - // sessions may share one channel; we tear down channels - // (which transitively takes all their sessions), not entries - // one-by-one. - const channelInfos = Array.from(byWorkspaceChannel.values()); + // Snapshot the live channel (if any) — `channelInfo` itself is + // intentionally NOT cleared here; the `channel.exited` handler + // clears it once the OS has reaped the child. That preserves + // the BkUyD invariant: a double-Ctrl+C arriving mid-SIGTERM- + // grace can still find the channel via `killAllSync`. + const ci = channelInfo; // Resolve every still-pending permission as cancelled before clearing // the maps so callers awaiting `requestPermission` unwind cleanly. for (const e of entries) { @@ -1979,9 +1997,8 @@ export function createHttpAcpBridge(opts: BridgeOptions = {}): HttpAcpBridge { resolvePending(id, { outcome: { outcome: 'cancelled' } }); } } - byWorkspace.clear(); + defaultEntry = undefined; byId.clear(); - byWorkspaceChannel.clear(); pendingPermissions.clear(); // Publish a terminal `session_died` BEFORE closing each bus so SSE // subscribers can distinguish "daemon shut down" from a transient @@ -2001,16 +2018,15 @@ export function createHttpAcpBridge(opts: BridgeOptions = {}): HttpAcpBridge { } e.events.close(); } - // Wait for in-flight channel spawns + session spawns. The - // snapshots above only see what's already registered; a doSpawn - // past `newSession()` but pre-`byId.set` is missed, as is a - // `getOrCreateChannel` past `channelFactory()` but pre- - // `byWorkspaceChannel.set`. The late-shutdown re-checks at - // doSpawn/getOrCreateChannel catch both — but without these - // awaits, `bridge.shutdown()` would resolve before they - // finish, and the orphan stderr error from a half-built - // child would fire AFTER the daemon claimed graceful - // shutdown (log-confusing). + // Wait for in-flight channel + session spawns. The snapshot + // above only sees what's already registered; a doSpawn past + // `newSession()` but pre-`byId.set` is missed, as is an + // `ensureChannel` past `channelFactory()` but pre-`channelInfo + // = info`. The late-shutdown re-checks at doSpawn/ensureChannel + // catch both — but without these awaits, `bridge.shutdown()` + // would resolve before they finish, and the orphan stderr + // error from a half-built child would fire AFTER the daemon + // claimed graceful shutdown (log-confusing). const inFlightSessionAwaits = Array.from(inFlightSpawns.values()).map( (p): Promise => p.then( @@ -2018,22 +2034,16 @@ export function createHttpAcpBridge(opts: BridgeOptions = {}): HttpAcpBridge { () => undefined, ), ); - const inFlightChannelAwaits = Array.from( - inFlightChannelSpawns.values(), - ).map( - (p): Promise => - p.then( + const inFlightChannelAwait: Promise = inFlightChannelSpawn + ? inFlightChannelSpawn.then( () => undefined, () => undefined, - ), - ); + ) + : Promise.resolve(); await Promise.all([ - // Kill each unique channel once. With multi-session per - // channel, the same channel object can be referenced by - // multiple entries; `channelInfos` is the deduplicated set. - ...channelInfos.map((ci) => ci.channel.kill().catch(() => {})), + ci ? ci.channel.kill().catch(() => {}) : Promise.resolve(), ...inFlightSessionAwaits, - ...inFlightChannelAwaits, + inFlightChannelAwait, ]); }, }; @@ -2090,12 +2100,14 @@ function sliceLineRange( } /** - * Canonicalize a workspace path so two callers referring to the same - * directory get the same `byWorkspace` key. `path.resolve` alone collapses - * `..` and `.` segments and absolutizes, but on case-insensitive filesystems - * (macOS APFS, Windows NTFS) `/Work/A` and `/work/a` are the same directory - * yet `resolve` returns them verbatim — two `byWorkspace` entries form for - * one physical workspace and `sessionScope: 'single'` silently degrades to + * Canonicalize a workspace path so the boot-time bound path and every + * request's `workspaceCwd` collapse to the same key. `path.resolve` + * alone normalizes `..` and `.` segments and absolutizes, but on + * case-insensitive filesystems (macOS APFS, Windows NTFS) `/Work/A` + * and `/work/a` are the same directory yet `resolve` returns them + * verbatim — without normalization the `boundWorkspace` check would + * reject every request that spelled the path with different casing + * and `sessionScope: 'single'` re-attach would silently degrade to * "one per spelling". * * `realpathSync.native` (when the path exists) walks symlinks and returns @@ -2108,17 +2120,17 @@ function sliceLineRange( * * NOTE: This is a **cross-module contract** (BX9_q) — `config.ts`, * `settings.ts`, `sandbox.ts`, and this file all need to canonicalize - * the same way for `sessionScope: 'single'` re-attach to work - * correctly across paths. The contract: use `realpathSync.native` on - * the resolved absolute path; fall back to `path.resolve` only when - * the path doesn't exist yet. If a future change breaks this - * alignment (e.g. one module starts lowercasing on Windows but this - * one doesn't), `byWorkspace.get()` lookup misses for the same - * physical directory → duplicate sessions silently spawn, and - * `sessionScope: 'single'` degrades to "one per spelling" with no - * error. There's no test that pins the alignment; the integration - * suite would catch a divergence only if it tested the specific - * casing / symlink path the affected module changed. + * the same way for the bound-workspace check + `sessionScope: + * 'single'` re-attach to work correctly across paths. The contract: + * use `realpathSync.native` on the resolved absolute path; fall back + * to `path.resolve` only when the path doesn't exist yet. If a future + * change breaks this alignment (e.g. one module starts lowercasing on + * Windows but this one doesn't), the canonicalized request path + * won't match the canonicalized bound path → every request returns + * `workspace_mismatch` even though the human-readable paths look + * equivalent. There's no test that pins the alignment; the + * integration suite would catch a divergence only if it tested the + * specific casing / symlink path the affected module changed. * * Stage 2 in-process (#3803 §10) collapses the bridge into core, * removing the bridge-side path resolution entirely. Stage 1.5 diff --git a/packages/cli/src/serve/runQwenServe.ts b/packages/cli/src/serve/runQwenServe.ts index 13bdb7841fc..3c8227e0649 100644 --- a/packages/cli/src/serve/runQwenServe.ts +++ b/packages/cli/src/serve/runQwenServe.ts @@ -5,6 +5,7 @@ */ import { type Server } from 'node:http'; +import * as path from 'node:path'; import { writeStderrLine, writeStdoutLine } from '../utils/stdioHelpers.js'; import { createHttpAcpBridge, type HttpAcpBridge } from './httpAcpBridge.js'; import { isLoopbackBind } from './loopbackBinds.js'; @@ -100,8 +101,23 @@ export async function runQwenServe( ); } + // Resolve the bound workspace per #3803 §02 (1 daemon = 1 workspace). + // Explicit `--workspace` wins; otherwise default to process.cwd(). + // `POST /session` with a mismatched `cwd` is rejected by the bridge + // with `WorkspaceMismatchError`. Multi-workspace deployments use + // multiple daemon processes, not intra-daemon routing. + const boundWorkspace = opts.workspace ?? process.cwd(); + if (!path.isAbsolute(boundWorkspace)) { + throw new Error( + `Invalid --workspace "${boundWorkspace}": must be an absolute path.`, + ); + } const bridge = - deps.bridge ?? createHttpAcpBridge({ maxSessions: opts.maxSessions }); + deps.bridge ?? + createHttpAcpBridge({ + maxSessions: opts.maxSessions, + boundWorkspace, + }); let actualPort = opts.port; const app = createServeApp(opts, () => actualPort, { bridge }); @@ -188,7 +204,10 @@ export async function runQwenServe( const addr = server.address(); actualPort = typeof addr === 'object' && addr ? addr.port : opts.port; const url = `http://${formatHostForUrl(opts.hostname)}:${actualPort}`; - writeStdoutLine(`qwen serve listening on ${url} (mode=${opts.mode})`); + writeStdoutLine( + `qwen serve listening on ${url} (mode=${opts.mode}, ` + + `workspace=${boundWorkspace})`, + ); if (!token) { writeStderrLine( `qwen serve: bearer auth disabled (loopback default). Set ${QWEN_SERVER_TOKEN_ENV} to enable.`, diff --git a/packages/cli/src/serve/server.test.ts b/packages/cli/src/serve/server.test.ts index c9ec984b3d8..f51322f1eaa 100644 --- a/packages/cli/src/serve/server.test.ts +++ b/packages/cli/src/serve/server.test.ts @@ -20,6 +20,7 @@ import { InvalidPermissionOptionError, SessionLimitExceededError, SessionNotFoundError, + WorkspaceMismatchError, type BridgeSession, type BridgeSessionSummary, type BridgeSpawnRequest, @@ -199,6 +200,24 @@ describe('createServeApp', () => { expect(res.body.features).toEqual([...STAGE1_FEATURES]); expect(res.body.modelServices).toEqual([]); }); + + it('reports the bound workspace (#3803 §02)', async () => { + const app = createServeApp({ ...baseOpts, workspace: '/work/bound' }); + const res = await request(app) + .get('/capabilities') + .set('Host', `127.0.0.1:${baseOpts.port}`); + expect(res.status).toBe(200); + expect(res.body.workspaceCwd).toBe('/work/bound'); + }); + + it('falls back to process.cwd() when --workspace is omitted', async () => { + const app = createServeApp(baseOpts); + const res = await request(app) + .get('/capabilities') + .set('Host', `127.0.0.1:${baseOpts.port}`); + expect(res.status).toBe(200); + expect(res.body.workspaceCwd).toBe(process.cwd()); + }); }); describe('host allowlist (loopback bind)', () => { @@ -277,20 +296,31 @@ describe('createServeApp', () => { }); describe('POST /session', () => { - it('400 when cwd is missing', async () => { + it('200 when cwd is omitted (falls back to bound workspace, #3803 §02)', async () => { + // 1 daemon = 1 workspace: the daemon binds to + // `opts.workspace ?? process.cwd()` at boot, so clients may + // omit `cwd` and the route falls back to the bound path. const bridge = fakeBridge(); - const app = createServeApp(baseOpts, undefined, { bridge }); + const app = createServeApp( + { ...baseOpts, workspace: '/work/bound' }, + undefined, + { bridge }, + ); const res = await request(app) .post('/session') .set('Host', `127.0.0.1:${baseOpts.port}`) .send({}); - expect(res.status).toBe(400); - expect(bridge.calls).toHaveLength(0); + expect(res.status).toBe(200); + expect(bridge.calls[0]?.workspaceCwd).toBe('/work/bound'); }); it('400 when cwd is relative', async () => { const bridge = fakeBridge(); - const app = createServeApp(baseOpts, undefined, { bridge }); + const app = createServeApp( + { ...baseOpts, workspace: '/work/bound' }, + undefined, + { bridge }, + ); const res = await request(app) .post('/session') .set('Host', `127.0.0.1:${baseOpts.port}`) @@ -299,6 +329,33 @@ describe('createServeApp', () => { expect(bridge.calls).toHaveLength(0); }); + it('400 workspace_mismatch when bridge rejects cross-workspace cwd (#3803 §02)', async () => { + // Single-workspace mode: bridge throws WorkspaceMismatchError + // when the route forwards a non-bound cwd. Route translates + // to 400 with code `workspace_mismatch` + both paths in the + // body so orchestrator-aware clients can route correctly. + const bridge = fakeBridge({ + spawnImpl: async (req) => { + throw new WorkspaceMismatchError('/work/bound', req.workspaceCwd); + }, + }); + const app = createServeApp( + { ...baseOpts, workspace: '/work/bound' }, + undefined, + { bridge }, + ); + const res = await request(app) + .post('/session') + .set('Host', `127.0.0.1:${baseOpts.port}`) + .send({ cwd: '/work/different' }); + expect(res.status).toBe(400); + expect(res.body).toMatchObject({ + code: 'workspace_mismatch', + boundWorkspace: '/work/bound', + requestedWorkspace: '/work/different', + }); + }); + it('200 with the BridgeSession shape on success', async () => { const bridge = fakeBridge(); const app = createServeApp(baseOpts, undefined, { bridge }); diff --git a/packages/cli/src/serve/server.ts b/packages/cli/src/serve/server.ts index 456b2ffe8aa..98323be0334 100644 --- a/packages/cli/src/serve/server.ts +++ b/packages/cli/src/serve/server.ts @@ -15,6 +15,7 @@ import { InvalidPermissionOptionError, SessionLimitExceededError, SessionNotFoundError, + WorkspaceMismatchError, type HttpAcpBridge, } from './httpAcpBridge.js'; import { SubscriberLimitExceededError, type BridgeEvent } from './eventBus.js'; @@ -61,8 +62,17 @@ export function createServeApp( // cap they configured via `ServeOptions`. Previously the default // bridge silently fell back to `DEFAULT_MAX_SESSIONS` (20) and // only the `runQwenServe` path piped the option through. + // Workspace binding mirrors `runQwenServe`: per #3803 §02 the + // daemon is bound to exactly one workspace (`opts.workspace` or + // `process.cwd()`). `POST /session` with a mismatched cwd is + // rejected with 400 `workspace_mismatch`. + const boundWorkspace = opts.workspace ?? process.cwd(); const bridge = - deps.bridge ?? createHttpAcpBridge({ maxSessions: opts.maxSessions }); + deps.bridge ?? + createHttpAcpBridge({ + maxSessions: opts.maxSessions, + boundWorkspace, + }); // Order matters: rejection guards (CORS / Host allowlist / bearer auth) // run BEFORE the JSON body parser. Otherwise an unauthenticated POST @@ -142,14 +152,23 @@ export function createServeApp( mode: opts.mode, features: [...STAGE1_FEATURES], modelServices: [], + // #3803 §02: surface the bound workspace so clients can detect + // mismatch pre-flight and omit `cwd` on `POST /session`. + workspaceCwd: boundWorkspace, }; res.status(200).json(envelope); }); app.post('/session', async (req, res) => { const body = safeBody(req); - const cwd = typeof body['cwd'] === 'string' ? (body['cwd'] as string) : ''; - if (!cwd || !path.isAbsolute(cwd)) { + // #3803 §02: 1 daemon = 1 workspace. `cwd` may be omitted — the + // route falls back to the daemon's bound workspace. A non-empty + // `cwd` that doesn't match the bound path is rejected downstream + // by the bridge with WorkspaceMismatchError (translated to 400 + // `workspace_mismatch` in `sendBridgeError`). + let cwd = typeof body['cwd'] === 'string' ? (body['cwd'] as string) : ''; + if (!cwd) cwd = boundWorkspace; + if (!path.isAbsolute(cwd)) { res .status(400) .json({ error: '`cwd` is required and must be an absolute path' }); @@ -801,6 +820,21 @@ function sendBridgeError( res.status(404).json({ error: err.message, sessionId: err.sessionId }); return; } + if (err instanceof WorkspaceMismatchError) { + // #3803 §02 single-workspace mode: the daemon binds to one + // workspace at boot; cross-workspace POSTs are rejected here. + // 400 (not 404 — the daemon is "fine", the client just picked + // the wrong daemon for their workspace). Body includes both + // paths so orchestrator-aware clients can route to the right + // daemon / spawn a new one. + res.status(400).json({ + error: err.message, + code: 'workspace_mismatch', + boundWorkspace: err.bound, + requestedWorkspace: err.requested, + }); + return; + } if (err instanceof SessionLimitExceededError) { // 503 Service Unavailable + `Retry-After` is the canonical // "we'd serve you, but we're full right now" shape. The hint diff --git a/packages/cli/src/serve/types.ts b/packages/cli/src/serve/types.ts index 11a4b8e9c76..f7de01856bc 100644 --- a/packages/cli/src/serve/types.ts +++ b/packages/cli/src/serve/types.ts @@ -56,6 +56,26 @@ export interface ServeOptions { * (default cap 64) plus short-lived REST calls. */ maxConnections?: number; + /** + * Absolute workspace path this daemon binds to. Per #3803 §02 the + * daemon is **1 daemon = 1 workspace × N sessions**: one bound + * workspace at boot, sessions multiplexed on the single + * `qwen --acp` child via `connection.newSession()`. + * + * `POST /session` calls whose `cwd` doesn't canonicalize to this + * path are rejected with `400 workspace_mismatch`. Clients may + * also omit `cwd` — the route falls back to this bound path. + * + * Multi-workspace deployments use **multiple daemon processes** + * (one per workspace, each on its own port), supervised by + * systemd / docker-compose / k8s / `qwen-coordinator` reference + * orchestrator. There is no intra-daemon multi-workspace mode + * (the previous Stage 1 `byWorkspaceChannel` routing layer was + * removed in the §02 design revision). + * + * Defaults to `process.cwd()` when omitted. + */ + workspace?: string; } /** @@ -77,6 +97,15 @@ export interface CapabilitiesEnvelope { * this field being non-empty. */ modelServices: string[]; + /** + * Absolute workspace path this daemon is bound to (per #3803 §02: + * `1 daemon = 1 workspace`). Clients use this to: + * - Detect mismatch before posting `/session` (vs. waiting for + * 400 workspace_mismatch from the bridge). + * - Omit `cwd` on `POST /session` — the route falls back to this + * path when the body has no `cwd` field. + */ + workspaceCwd: string; } export const CAPABILITIES_SCHEMA_VERSION = 1 as const; From 31520279e5ea48cfc5111db4491b9c01e1d137dd Mon Sep 17 00:00:00 2001 From: wenshao Date: Wed, 13 May 2026 19:04:08 +0800 Subject: [PATCH 02/16] =?UTF-8?q?fix(serve):=20address=20review=20findings?= =?UTF-8?q?=20on=20the=20=C2=A702=20refactor?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two correctness fixes + four doc/test polish items surfaced by the multi-agent review of #4113: 1. `killSession` → `spawnOrAttach` race (Critical). After killing the last session, `channel.kill()` runs through a 5s SIGTERM grace before SIGKILL. During that window a concurrent `spawnOrAttach` used to hit `ensureChannel`, find `channelInfo` still set, and reuse the dying transport — either landing the caller with a sessionId that 404s on every follow-up once `channel.exited` fires, or hanging until the newSession timeout. Fix: add an `isDying: boolean` flag on `ChannelInfo`, set synchronously by `killSession` / `doSpawn`-newSession-failure / `shutdown` BEFORE awaiting `channel.kill()`. `ensureChannel` treats a dying channel as absent and spawns a fresh one. The tanzhenxin BkUyD invariant ("`channelInfo` reference must outlive the kill-await for `killAllSync` mid-grace") is preserved — we set `isDying` but don't clear `channelInfo` until the OS reaps the child via `channel.exited`. A regression test in `httpAcpBridge.test.ts` pins the invariant: a never-resolving `kill()` keeps the SIGTERM grace open while a concurrent spawn verifies the factory was called twice (two distinct handles). 2. `boundWorkspace` canonicalization divergence (Critical). `server.ts` and `runQwenServe.ts` each computed `opts.workspace ?? process.cwd()` independently. The bridge canonicalized that string via `realpathSync.native` (resolving symlinks, case-folding on case-insensitive filesystems); the callers retained the raw form. On macOS HFS+ / APFS or any symlinked path, `/capabilities.workspaceCwd` advertised one spelling while the bridge enforced against another — clients echoing the advertised path back saw `POST /session` succeed but the response carry a different `workspaceCwd`. Fix: export `canonicalizeWorkspace` from `httpAcpBridge.ts` and call it once in `runQwenServe` (after the existence check) and once in `createServeApp`. Both paths land on the same canonical form; the bridge's own re-canonicalize is now a no-op (idempotent). 3. Reject `--workspace` pointing at non-existent directories at boot (Suggestion). `canonicalizeWorkspace`'s ENOENT fallback to `path.resolve` previously let the daemon boot pointed at a path that didn't exist; every `POST /session` then spawned a `qwen --acp` child with that cwd and the agent failed with an opaque ENOENT. Now `runQwenServe` `statSync`s the bound path at boot and rejects "directory does not exist" / "not a directory" with a clear message. 4. Stale docstrings (Nice to have). `types.ts` `ServeMode` JSDoc said "one `qwen --acp` child PER WORKSPACE" — directly contradicted the new `workspace` field's doc in the same file. `commands/serve.ts` `--http-bridge` description said "per workspace" — directly contradicted the `--workspace` flag's help in the same yargs builder. Both updated to "per daemon (the daemon binds to ONE workspace at boot)". 5. Stale `byWorkspace` comment references (Nice to have). `server.ts:188` ("orphaned in byId / byWorkspace") and `httpAcpBridge.test.ts:1210` ("still in byId/byWorkspace at the moment of crash") referenced the removed Map. Updated to `defaultEntry`. 6. `/capabilities` curl example in the Authentication section of `docs/users/qwen-serve.md` was missing the new `workspaceCwd` field — the Quickstart's curl example was updated but the parallel one in the auth section was not. Synced. Tests added: - `killSession marks the channel dying so concurrent spawnOrAttach gets a fresh channel` — pins fix (1). - `--workspace flows end-to-end and surfaces on /capabilities` — exercises the runQwenServe → server.ts → bridge plumbing that no prior test covered. - `rejects --workspace pointing at a non-existent directory` and `rejects --workspace pointing at a regular file` — pin fix (3). - `rejects relative --workspace at boot` — covers the absoluteness check that exists but was untested. Net: +238 / -24 across 8 files. All 149 serve tests pass. --- docs/users/qwen-serve.md | 2 +- packages/cli/src/commands/serve.ts | 8 +-- packages/cli/src/serve/httpAcpBridge.test.ts | 50 +++++++++++++- packages/cli/src/serve/httpAcpBridge.ts | 47 +++++++++++-- packages/cli/src/serve/runQwenServe.ts | 46 ++++++++++-- packages/cli/src/serve/server.test.ts | 73 ++++++++++++++++++++ packages/cli/src/serve/server.ts | 17 ++++- packages/cli/src/serve/types.ts | 17 ++--- 8 files changed, 236 insertions(+), 24 deletions(-) diff --git a/docs/users/qwen-serve.md b/docs/users/qwen-serve.md index 1e2f7a1e75d..f398f0c048f 100644 --- a/docs/users/qwen-serve.md +++ b/docs/users/qwen-serve.md @@ -99,7 +99,7 @@ Clients then send `Authorization: Bearer $QWEN_SERVER_TOKEN` on every request. ` ```bash curl -H "Authorization: Bearer $QWEN_SERVER_TOKEN" http://your-host:4170/capabilities -# → {"v":1,"mode":"http-bridge","features":[...],"modelServices":[]} +# → {"v":1,"mode":"http-bridge","features":[...],"modelServices":[],"workspaceCwd":"/path/to/your-project"} # Wrong token → 401 ``` diff --git a/packages/cli/src/commands/serve.ts b/packages/cli/src/commands/serve.ts index d2a2d6bf8bb..20262145cce 100644 --- a/packages/cli/src/commands/serve.ts +++ b/packages/cli/src/commands/serve.ts @@ -88,10 +88,10 @@ export const serveCommand: CommandModule = { type: 'boolean', default: true, description: - 'Stage 1 mode: one `qwen --acp` child per workspace behind the HTTP routes, ' + - "with multiple sessions multiplexed onto each child via the agent's native " + - '`newSession()`. Stage 2 native in-process mode is not yet implemented; ' + - 'this flag will become opt-in then.', + 'Stage 1 mode: one `qwen --acp` child per daemon (the daemon binds to ' + + 'one workspace at boot, multiplexing N sessions onto that child via ' + + "the agent's native `newSession()`). Stage 2 native in-process mode " + + 'is not yet implemented; this flag will become opt-in then.', }) as unknown as Argv, handler: async (argv) => { if (!argv['http-bridge']) { diff --git a/packages/cli/src/serve/httpAcpBridge.test.ts b/packages/cli/src/serve/httpAcpBridge.test.ts index b4e3e8f44b5..35e97c5d510 100644 --- a/packages/cli/src/serve/httpAcpBridge.test.ts +++ b/packages/cli/src/serve/httpAcpBridge.test.ts @@ -474,6 +474,54 @@ describe('createHttpAcpBridge', () => { void shutdownPromise; }); + it('killSession marks the channel dying so concurrent spawnOrAttach gets a fresh channel', async () => { + // After the last session is killed, `channel.kill()` runs through + // its SIGTERM grace window before SIGKILL — up to 10s in the real + // factory. During that window a concurrent `spawnOrAttach` MUST + // get a FRESH channel, never the dying one. Pre-fix: `channelInfo` + // stayed set with no `isDying` flag, so `ensureChannel` returned + // the dying channel and `newSession()` either succeeded onto a + // transport about to close (landing a sessionId that 404s on the + // next request when `channel.exited` fires) or hung until the + // newSession timeout. Fix: `killSession` sets `isDying = true` + // synchronously before `await ci.channel.kill()`; `ensureChannel` + // skips dying channels and spawns a fresh one. + const handles: ChannelHandle[] = []; + const factory: ChannelFactory = async () => { + const h = makeChannel({ sessionIdPrefix: `s${handles.length}` }); + // Make kill() hang forever so the SIGTERM grace window stays + // open for the test (simulates a slow-to-exit child). + h.channel = { ...h.channel, kill: () => new Promise(() => {}) }; + handles.push(h); + return h.channel; + }; + const bridge = makeBridge({ channelFactory: factory }); + const first = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + expect(handles).toHaveLength(1); + + // Kick off killSession (the only session leaving triggers the + // channel teardown). The kill() Promise never resolves, so the + // method's await hangs — we fire-and-forget. + const killPromise = bridge.killSession(first.sessionId); + // Yield once so killSession's sync prefix runs (it marks + // `isDying = true` synchronously before `await ci.channel.kill()`). + await new Promise((r) => setImmediate(r)); + + // A new spawn MUST get a FRESH channel, not reuse the dying one. + const second = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + expect(handles).toHaveLength(2); + expect(second.sessionId).not.toBe(first.sessionId); + // The second session is on the fresh channel (handles[1]), not + // multiplexed onto the dying one (handles[0]). + expect(handles[1]?.agent.newSessionCalls).toHaveLength(1); + + // Cleanup: both channels' kill() never resolves (factory above + // overrides it). Don't await killSession or shutdown — same + // pattern as the BkUyD test above. The test runner GCs the + // dangling promises when this `it` returns. + void killPromise; + }); + describe('sendPrompt', () => { it('forwards a prompt and returns the agent response', async () => { const handles: ChannelHandle[] = []; @@ -1207,7 +1255,7 @@ describe('createHttpAcpBridge', () => { }); // Simulate a child crash (channel.exited resolves but we never called - // kill() — entry is still in byId/byWorkspace at the moment of crash). + // kill() — entry is still in byId / defaultEntry at the moment of crash). handles[0]?.crash(); // Drain the bus — first frame is `session_died`. diff --git a/packages/cli/src/serve/httpAcpBridge.ts b/packages/cli/src/serve/httpAcpBridge.ts index 777cf1210f5..d601ba6ecbc 100644 --- a/packages/cli/src/serve/httpAcpBridge.ts +++ b/packages/cli/src/serve/httpAcpBridge.ts @@ -417,6 +417,19 @@ interface ChannelInfo { * is cleared. */ sessionIds: Set; + /** + * Set synchronously by `killSession` / `doSpawn`-newSession-failure + * BEFORE awaiting `channel.kill()`. `ensureChannel` treats a dying + * channel as absent and spawns a fresh one. Without this flag a + * concurrent `spawnOrAttach` arriving during the SIGTERM grace + * window (up to 10s) would attach to a transport about to close, + * landing the caller with a sessionId that 404s on every follow-up + * request. We don't clear `channelInfo` eagerly because the + * tanzhenxin BkUyD invariant requires `killAllSync` to still find + * the channel mid-grace for force-kill — `isDying` is the + * "available-for-new-spawns" half of the two-bit state. + */ + isDying: boolean; } interface SessionEntry { @@ -1079,7 +1092,11 @@ export function createHttpAcpBridge(opts: BridgeOptions): HttpAcpBridge { * sessions. */ async function ensureChannel(): Promise { - if (channelInfo) return channelInfo; + // Skip a channel that's marked dying — its underlying transport is + // mid-SIGTERM-or-already-dead and `connection.newSession()` on it + // would either hang or land the caller with a sessionId that + // immediately 404s on every follow-up. + if (channelInfo && !channelInfo.isDying) return channelInfo; if (inFlightChannelSpawn) return await inFlightChannelSpawn; const promise = (async () => { @@ -1142,6 +1159,7 @@ export function createHttpAcpBridge(opts: BridgeOptions): HttpAcpBridge { client, workspaceCwd: boundWorkspace, sessionIds: new Set(), + isDying: false, }; channelInfo = info; @@ -1232,7 +1250,13 @@ export function createHttpAcpBridge(opts: BridgeOptions): HttpAcpBridge { // attempt — a populated channel keeps running for its other // live sessions. if (ci.sessionIds.size === 0) { - if (channelInfo === ci) channelInfo = undefined; + // Mark dying SYNCHRONOUSLY so a concurrent `spawnOrAttach` + // calling `ensureChannel()` between this point and the + // `channel.exited` cleanup spawns a fresh channel instead of + // attaching to the one we're about to tear down. `channelInfo` + // stays set until OS reap so `killAllSync` mid-SIGTERM still + // finds a target (tanzhenxin BkUyD invariant). + ci.isDying = true; await ci.channel.kill().catch(() => { /* best-effort — channel.exited handler still runs */ }); @@ -1916,6 +1940,15 @@ export function createHttpAcpBridge(opts: BridgeOptions): HttpAcpBridge { // `channel.exited` handler once the OS reaps the child — // tanzhenxin BkUyD invariant.) if (ci && ci.sessionIds.size === 0) { + // Mark dying SYNCHRONOUSLY before the await so a concurrent + // `spawnOrAttach` arriving during the SIGTERM grace window + // doesn't attach to a transport we're tearing down — without + // this it would land the caller with a sessionId that 404s on + // every follow-up once `channel.exited` fires (the equivalent + // of the pre-PR eager `byWorkspaceChannel.delete()` from the + // Stage 1 routing era). `channelInfo` stays set until OS reap + // so `killAllSync` still finds a target (BkUyD). + ci.isDying = true; await ci.channel.kill().catch(() => { // Best-effort kill — channel may already be dead. }); @@ -1987,8 +2020,14 @@ export function createHttpAcpBridge(opts: BridgeOptions): HttpAcpBridge { // intentionally NOT cleared here; the `channel.exited` handler // clears it once the OS has reaped the child. That preserves // the BkUyD invariant: a double-Ctrl+C arriving mid-SIGTERM- - // grace can still find the channel via `killAllSync`. + // grace can still find the channel via `killAllSync`. Setting + // `isDying` makes the dying channel invisible to any racing + // `ensureChannel` call — but `shuttingDown` already blocks new + // `spawnOrAttach` upstream, so this is mostly belt-and-suspenders + // (a direct internal `ensureChannel` past the gate would still + // see the dying state and not attach). const ci = channelInfo; + if (ci) ci.isDying = true; // Resolve every still-pending permission as cancelled before clearing // the maps so callers awaiting `requestPermission` unwind cleanly. for (const e of entries) { @@ -2141,7 +2180,7 @@ function sliceLineRange( * call sites to converge. Until then, *any* change to how those * modules resolve workspace paths needs a matching change here. */ -function canonicalizeWorkspace(p: string): string { +export function canonicalizeWorkspace(p: string): string { const resolved = path.resolve(p); try { // FIXME(stage-2): switch to `fs.promises.realpath` once the diff --git a/packages/cli/src/serve/runQwenServe.ts b/packages/cli/src/serve/runQwenServe.ts index 3c8227e0649..f2092dd7502 100644 --- a/packages/cli/src/serve/runQwenServe.ts +++ b/packages/cli/src/serve/runQwenServe.ts @@ -4,10 +4,15 @@ * SPDX-License-Identifier: Apache-2.0 */ +import * as fs from 'node:fs'; import { type Server } from 'node:http'; import * as path from 'node:path'; import { writeStderrLine, writeStdoutLine } from '../utils/stdioHelpers.js'; -import { createHttpAcpBridge, type HttpAcpBridge } from './httpAcpBridge.js'; +import { + canonicalizeWorkspace, + createHttpAcpBridge, + type HttpAcpBridge, +} from './httpAcpBridge.js'; import { isLoopbackBind } from './loopbackBinds.js'; import { createServeApp } from './server.js'; import type { ServeOptions } from './types.js'; @@ -106,12 +111,45 @@ export async function runQwenServe( // `POST /session` with a mismatched `cwd` is rejected by the bridge // with `WorkspaceMismatchError`. Multi-workspace deployments use // multiple daemon processes, not intra-daemon routing. - const boundWorkspace = opts.workspace ?? process.cwd(); - if (!path.isAbsolute(boundWorkspace)) { + // + // Boot-loud validation: absolute path, exists, is a directory. + // Without the stat() check, `canonicalizeWorkspace`'s ENOENT fallback + // to `path.resolve` would let the daemon boot pointed at a + // non-existent directory; every `POST /session` would then spawn a + // `qwen --acp` child with that cwd and the agent would fail with an + // opaque ENOENT — operator pain we can avoid by failing at boot. + const rawWorkspace = opts.workspace ?? process.cwd(); + if (!path.isAbsolute(rawWorkspace)) { throw new Error( - `Invalid --workspace "${boundWorkspace}": must be an absolute path.`, + `Invalid --workspace "${rawWorkspace}": must be an absolute path.`, ); } + try { + const stats = fs.statSync(rawWorkspace); + if (!stats.isDirectory()) { + throw new Error( + `Invalid --workspace "${rawWorkspace}": exists but is not a directory.`, + ); + } + } catch (err) { + if (err && typeof err === 'object' && 'code' in err) { + const code = (err as { code?: unknown }).code; + if (code === 'ENOENT') { + throw new Error( + `Invalid --workspace "${rawWorkspace}": directory does not exist.`, + ); + } + } + throw err; + } + // Canonicalize ONCE here so `/capabilities` and the POST /session + // fallback (both via server.ts) AND the bridge agree on the same + // path. Without this, server.ts and the bridge each compute + // `boundWorkspace` independently; on symlinks or case-insensitive + // filesystems the bridge's `realpathSync.native` form diverges from + // server.ts's raw `opts.workspace` and clients see one path on + // `/capabilities` but another on `POST /session` responses. + const boundWorkspace = canonicalizeWorkspace(rawWorkspace); const bridge = deps.bridge ?? createHttpAcpBridge({ diff --git a/packages/cli/src/serve/server.test.ts b/packages/cli/src/serve/server.test.ts index f51322f1eaa..03bd75b105c 100644 --- a/packages/cli/src/serve/server.test.ts +++ b/packages/cli/src/serve/server.test.ts @@ -1157,6 +1157,79 @@ describe('runQwenServe', () => { ).rejects.toThrow(/Invalid --hostname/); }); + it('--workspace flows end-to-end and surfaces on /capabilities (#3803 §02)', async () => { + // Use process.cwd() so the boot-time existence check passes — any + // real absolute directory works. The bridge canonicalizes this + // once at boot; `/capabilities.workspaceCwd` returns the canonical + // form, NOT the raw input. Tests inject a fake bridge here so we + // verify the route layer's canonicalization (not the bridge's), + // making this a true E2E that doesn't require a real `qwen --acp` + // child. + const bridge = fakeBridge(); + handle = await runQwenServe( + { + hostname: '127.0.0.1', + port: 0, + mode: 'http-bridge', + workspace: process.cwd(), + }, + { bridge }, + ); + const port = (handle.server.address() as { port: number }).port; + const caps = await ( + await fetch(`http://127.0.0.1:${port}/capabilities`) + ).json(); + // Canonical form per `canonicalizeWorkspace` — realpath of cwd + // (handles symlinks like `/var` → `/private/var` on macOS). + const expected = await import('node:fs').then((m) => + m.realpathSync.native(process.cwd()), + ); + expect(caps.workspaceCwd).toBe(expected); + }); + + it('rejects --workspace pointing at a non-existent directory (BkUyD followup — boot-loud over opaque ENOENT)', async () => { + // Without the boot-time stat check, `canonicalizeWorkspace`'s + // ENOENT fallback to `path.resolve` would let the daemon boot + // pointed at a non-existent directory; every `POST /session` + // would then spawn a `qwen --acp` child with that cwd and the + // agent would fail with an opaque ENOENT. + await expect( + runQwenServe({ + hostname: '127.0.0.1', + port: 0, + mode: 'http-bridge', + workspace: `/tmp/qwen-serve-no-such-path-${Date.now()}-${Math.random().toString(36).slice(2)}`, + }), + ).rejects.toThrow(/directory does not exist/); + }); + + it('rejects --workspace pointing at a regular file', async () => { + // Pointing the daemon at a file (vs. a directory) is operator error + // — the agent would fail at child-spawn time with ENOTDIR. Catch + // it at boot for a clearer error message. + await expect( + runQwenServe({ + hostname: '127.0.0.1', + port: 0, + mode: 'http-bridge', + // `import.meta.url` resolves to this test file's URL on Node; + // strip `file://` to get an absolute path that's known to exist. + workspace: new URL(import.meta.url).pathname, + }), + ).rejects.toThrow(/exists but is not a directory/); + }); + + it('rejects relative --workspace at boot', async () => { + await expect( + runQwenServe({ + hostname: '127.0.0.1', + port: 0, + mode: 'http-bridge', + workspace: 'relative/path', + }), + ).rejects.toThrow(/must be an absolute path/); + }); + it('drains the bridge before closing the listener', async () => { const bridge = fakeBridge(); handle = await runQwenServe( diff --git a/packages/cli/src/serve/server.ts b/packages/cli/src/serve/server.ts index 98323be0334..7841766186d 100644 --- a/packages/cli/src/serve/server.ts +++ b/packages/cli/src/serve/server.ts @@ -11,6 +11,7 @@ import { writeStderrLine } from '../utils/stdioHelpers.js'; import { bearerAuth, denyBrowserOriginCors, hostAllowlist } from './auth.js'; import { isLoopbackBind } from './loopbackBinds.js'; import { + canonicalizeWorkspace, createHttpAcpBridge, InvalidPermissionOptionError, SessionLimitExceededError, @@ -62,11 +63,23 @@ export function createServeApp( // cap they configured via `ServeOptions`. Previously the default // bridge silently fell back to `DEFAULT_MAX_SESSIONS` (20) and // only the `runQwenServe` path piped the option through. + // // Workspace binding mirrors `runQwenServe`: per #3803 §02 the // daemon is bound to exactly one workspace (`opts.workspace` or // `process.cwd()`). `POST /session` with a mismatched cwd is // rejected with 400 `workspace_mismatch`. - const boundWorkspace = opts.workspace ?? process.cwd(); + // + // Canonicalize here so the value advertised on `/capabilities`, + // used for the `POST /session` cwd fallback, AND passed into the + // bridge are all the SAME canonical form. Without this step the + // bridge re-canonicalizes via `realpathSync.native` and on symlinks + // / case-insensitive filesystems the bridge's stored form diverges + // from the value `/capabilities` advertises — clients echoing the + // advertised path back end up with a response whose `workspaceCwd` + // differs from what they sent. `canonicalizeWorkspace` is + // idempotent so `runQwenServe`'s pre-canonicalization here is a + // no-op when the caller already did the work. + const boundWorkspace = canonicalizeWorkspace(opts.workspace ?? process.cwd()); const bridge = deps.bridge ?? createHttpAcpBridge({ @@ -185,7 +198,7 @@ export function createServeApp( }); // Client may have disconnected during the 1–3s spawn window. If // so, the response can't be delivered. The session is otherwise - // orphaned (in `byId` / `byWorkspace` with no client knowing the + // orphaned (in `byId` / `defaultEntry` with no client knowing the // id), and under churn this leaks one child per aborted request. // // Detect "can we still write the response?" via `res.writable`, diff --git a/packages/cli/src/serve/types.ts b/packages/cli/src/serve/types.ts index f7de01856bc..12bacdd5345 100644 --- a/packages/cli/src/serve/types.ts +++ b/packages/cli/src/serve/types.ts @@ -7,14 +7,15 @@ /** * Stage 1 daemon mode shape. * - * `http-bridge` (Stage 1): one `qwen --acp` child PER WORKSPACE, with - * multiple sessions multiplexed onto that child via the agent's native - * `connection.newSession()` (see `acp-integration/acpAgent.ts:194`). - * Sessions on the same workspace share the child's process / OAuth / - * file-cache / hierarchy-memory parse. The daemon pipes ACP NDJSON over - * HTTP/SSE. Same-session multi-client requests serialize through the - * bridge's per-session FIFO; cross-session requests on the same channel - * can run concurrently (the ACP layer demultiplexes by sessionId). + * `http-bridge` (Stage 1): per #3803 §02, one `qwen --acp` child per + * daemon (the daemon binds to ONE workspace at boot). Multiple + * sessions multiplex onto that child via the agent's native + * `connection.newSession()` (see `acp-integration/acpAgent.ts:194`), + * sharing the child's process / OAuth / file-cache / hierarchy-memory + * parse. The daemon pipes ACP NDJSON over HTTP/SSE. Same-session + * multi-client requests serialize through the bridge's per-session + * FIFO; cross-session requests on the same channel can run + * concurrently (the ACP layer demultiplexes by sessionId). * `native` (Stage 2+): in-process multi-session, AsyncLocalStorage; not yet * implemented. */ From 2cd0ff8c5abbbe89955b45a00901cb8c18a3c9e5 Mon Sep 17 00:00:00 2001 From: wenshao Date: Wed, 13 May 2026 19:26:05 +0800 Subject: [PATCH 03/16] fix(serve): BkUyD overwrite race + Windows-fragile test + doSpawn-failure coverage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-2 review of #4113 caught three follow-up issues introduced by or left open after round-1's fixes: 1. **BkUyD invariant overwrite race (Critical).** Round-1's `isDying` flag lets `ensureChannel` skip a dying channel and spawn a fresh one. When the fresh spawn completes, `channelInfo = info` overwrote the dying channel's reference — leaving NO global pointer to it. `killAllSync()` then iterated only `channelInfo` (the fresh one) and missed the dying child entirely. A double-Ctrl+C arriving mid-SIGTERM-grace would call `process.exit(1)` before the dying child's per-channel SIGKILL escalation timer fired, orphaning the child. Restore a `aliveChannels: Set` (parallel to the original Stage 1 design, but justified by single-workspace too). Entries added in `ensureChannel`, removed by each channel's `channel.exited` handler. `killAllSync` iterates the SET, not the single attach-target slot. `shutdown` does the same — snapshots every alive channel and kills each, not just the current `channelInfo`. New regression test pins the invariant: spawn → killSession (channel marked dying, kill hangs) → spawnOrAttach (fresh channel overwrites `channelInfo`) → `killAllSync` — expect BOTH channels' `killSync` to fire. Pre-fix only the fresh one would have fired. 2. **Windows-fragile test path.** The new `rejects --workspace pointing at a regular file` test used `new URL(import.meta.url).pathname` to get a path to the test file. On Windows that returns `/C:/path/...` (leading slash); `fs.statSync` then resolves it as path-from-current-drive-root, fails with ENOENT, and the test sees the "does not exist" error message instead of the expected "not a directory" branch. CI runs `windows-latest`. Fix: `fileURLToPath(import.meta.url)` from `node:url`. 3. **doSpawn newSession-failure isDying path was untested.** The round-1 fix added `ci.isDying = true` to both `killSession` AND `doSpawn`'s newSession-failure catch, but only the killSession path had a regression test. Added a parallel one for the doSpawn path: thread-scope bridge with a `newSessionImpl` that throws on the first call → captures the rejection without awaiting it (the bridge's `await ci.channel.kill()` hangs in the test), yields enough cycles for the `isDying = true` sync prefix to settle, then confirms (a) the next `spawnOrAttach` produces a fresh channel and (b) `killAllSync` finds both channels in `aliveChannels`. Also added a `newSessionImpl` option to the test FakeAgent — the existing `initializeThrows` hook covered handshake-time failures, but post-init `newSession` rejections (auth, bad config, mid-init crashes) had no test affordance. All 151 serve tests pass. --- packages/cli/src/serve/httpAcpBridge.test.ts | 160 +++++++++++++++++++ packages/cli/src/serve/httpAcpBridge.ts | 106 +++++++----- packages/cli/src/serve/server.test.ts | 11 +- 3 files changed, 236 insertions(+), 41 deletions(-) diff --git a/packages/cli/src/serve/httpAcpBridge.test.ts b/packages/cli/src/serve/httpAcpBridge.test.ts index 35e97c5d510..ac6161d350f 100644 --- a/packages/cli/src/serve/httpAcpBridge.test.ts +++ b/packages/cli/src/serve/httpAcpBridge.test.ts @@ -80,6 +80,16 @@ interface FakeAgentOpts { p: PromptRequest, self: FakeAgent, ) => Promise | PromptResponse; + /** + * Custom `newSession` handler. Default returns a synthesized id (see + * `newSession` below). Used by tests that need to exercise the + * doSpawn newSession-failure path (e.g. throwing to cover the + * `isDying`-mark-then-kill cleanup). + */ + newSessionImpl?: ( + p: NewSessionRequest, + self: FakeAgent, + ) => Promise | NewSessionResponse; } class FakeAgent implements Agent { @@ -103,6 +113,9 @@ class FakeAgent implements Agent { async newSession(p: NewSessionRequest): Promise { this.newSessionCalls.push(p); + if (this.opts.newSessionImpl) { + return this.opts.newSessionImpl(p, this); + } const prefix = this.opts.sessionIdPrefix ?? 'sess'; // Stage 1.5 multi-session: one FakeAgent can host multiple // sessions (same as the real ACP agent), so each newSession call @@ -522,6 +535,153 @@ describe('createHttpAcpBridge', () => { void killPromise; }); + it('doSpawn newSession-failure marks the empty channel dying so the next spawn gets a fresh one', async () => { + // Parallel to "killSession marks the channel dying" above, but + // covers the OTHER `isDying = true` site: `doSpawn`'s + // `connection.newSession()` rejection path. When the channel's + // first/only `newSession` fails (auth, bad config, agent crash + // during init), the bridge marks the empty channel dying and + // kicks off `channel.kill()`. The kill awaits a SIGTERM grace, + // and during that window the next `spawnOrAttach` retry MUST + // get a FRESH channel — not reuse the one whose newSession just + // failed (which would re-issue newSession to a transport about + // to close, almost certainly hanging or failing identically). + // Pre-fix the equivalent code eagerly cleared `channelInfo` so + // the BkUyD invariant was violated; the round-2 fix uses + // `isDying` + `aliveChannels` instead. + let factoryCount = 0; + const killSyncCalls: string[] = []; + const factory: ChannelFactory = async () => { + const tag = `c${factoryCount++}`; + // First channel's newSession rejects; subsequent channels succeed. + const firstChannel = factoryCount === 1; + const h = makeChannel({ + sessionIdPrefix: tag, + newSessionImpl: firstChannel + ? () => { + throw new Error('agent refused newSession (test)'); + } + : undefined, + }); + const realKillSync = h.channel.killSync; + h.channel = { + ...h.channel, + // Hang kill() so the SIGTERM grace stays open for the + // duration of the test. We don't await spawnOrAttach's + // rejection (which would block on the kill) — instead we + // catch it via .catch() and yield enough cycles for the + // sync prefix (`isDying = true`) to settle. + kill: () => new Promise(() => {}), + killSync: () => { + killSyncCalls.push(tag); + realKillSync(); + }, + }; + return h.channel; + }; + // Thread scope so calls don't coalesce via `inFlightSpawns` — + // the second spawn must not wait on the first one's hanging + // doSpawn. Without thread scope the single-scope coalescing + // would make `spawnOrAttach` call 2 await call 1's in-flight + // promise (still pending on the never-resolving kill). + const bridge = makeBridge({ + channelFactory: factory, + sessionScope: 'thread', + }); + + // First spawn: newSession on c0 fails. `doSpawn`'s catch runs + // `ci.isDying = true` synchronously, then `await ci.channel.kill()` + // (hangs in this test). The original error never propagates + // because the kill never resolves — so we DON'T await the + // rejection. Capture it for cleanup. + let firstErr: unknown; + const firstAttempt = bridge + .spawnOrAttach({ workspaceCwd: WS_A }) + .catch((err) => { + firstErr = err; + }); + + // Yield enough times for `ensureChannel`'s spawn to complete, + // newSession to reject, and doSpawn's catch sync prefix + // (`ci.isDying = true`) to run before the kill-await hangs. + for (let i = 0; i < 5; i++) { + await new Promise((r) => setImmediate(r)); + } + expect(factoryCount).toBe(1); + + // Second attempt: `ensureChannel` finds c0 with `isDying: true`, + // skips it, spawns a fresh c1. Pre-fix the equivalent code + // (eagerly clearing `channelInfo`) made this work via a + // different mechanism that violated BkUyD; the current fix uses + // `isDying` + `aliveChannels` for both correctness AND BkUyD. + const second = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + expect(factoryCount).toBe(2); + expect(second.attached).toBe(false); + + // Both channels live in `aliveChannels` (c0 is dying but its + // `channel.exited` hasn't fired; c1 is freshly attached). + // `killAllSync` MUST find both. + bridge.killAllSync(); + expect(killSyncCalls.sort()).toEqual(['c0', 'c1']); + + // Cleanup: firstAttempt is pending forever (kill never resolves). + // Touch firstErr to satisfy linters about the variable. + void firstAttempt; + void firstErr; + }); + + it('killAllSync force-kills BOTH the dying channel AND the fresh attach-target (BkUyD overwrite race)', async () => { + // The killSession → spawnOrAttach race opens a window where two + // channels are simultaneously "alive" from the daemon's + // perspective: the dying one (sessionIds.size === 0, in + // SIGTERM grace) and the fresh one (just spawned to serve the new + // request). Pre-fix `killAllSync()` iterated only `channelInfo` + // (the fresh one), missing the dying channel and orphaning its + // child when `process.exit(1)` fired before its SIGTERM + // escalation timer. Fix: separate `aliveChannels: Set` + // that `killAllSync` iterates, only cleared by each channel's + // `channel.exited` (the OS-reap signal). + const killSyncCalls: string[] = []; + let factoryCount = 0; + const factory: ChannelFactory = async () => { + const tag = `c${factoryCount++}`; + const h = makeChannel({ sessionIdPrefix: tag }); + const realKillSync = h.channel.killSync; + h.channel = { + ...h.channel, + // kill() hangs forever so the dying channel stays in + // SIGTERM grace for the duration of the test. + kill: () => new Promise(() => {}), + killSync: () => { + killSyncCalls.push(tag); + realKillSync(); + }, + }; + return h.channel; + }; + const bridge = makeBridge({ channelFactory: factory }); + const first = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + + // Trigger the overwrite race: kill the only session → channel + // marked dying, kill awaits a never-resolving Promise; then + // spawn a new session → fresh channel, `channelInfo` reassigned. + const killPromise = bridge.killSession(first.sessionId); + await new Promise((r) => setImmediate(r)); + await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + + // Both channels are alive from the OS's perspective. A + // double-Ctrl+C arrives. + bridge.killAllSync(); + + // BOTH channels received killSync. Pre-fix only `c1` (the fresh + // one in `channelInfo`) would have fired — `c0` was dying in + // unreachable state and would have orphaned its child. + expect(killSyncCalls.sort()).toEqual(['c0', 'c1']); + + // Cleanup: dangling never-resolving promises GC'd by the runner. + void killPromise; + }); + describe('sendPrompt', () => { it('forwards a prompt and returns the agent response', async () => { const handles: ChannelHandle[] = []; diff --git a/packages/cli/src/serve/httpAcpBridge.ts b/packages/cli/src/serve/httpAcpBridge.ts index d601ba6ecbc..1d5bb20da54 100644 --- a/packages/cli/src/serve/httpAcpBridge.ts +++ b/packages/cli/src/serve/httpAcpBridge.ts @@ -996,19 +996,35 @@ export function createHttpAcpBridge(opts: BridgeOptions): HttpAcpBridge { const boundWorkspace = canonicalizeWorkspace(opts.boundWorkspace); // #3803 §02 single-workspace model: the bridge hosts AT MOST one - // channel and one default attach-target entry. Multi-session - // multiplexing happens through `channelInfo.sessionIds`; the - // `defaultEntry` slot is the FIRST session created (the one a + // ATTACH-AVAILABLE channel and one default attach-target entry. + // Multi-session multiplexing happens through `channelInfo.sessionIds`; + // the `defaultEntry` slot is the FIRST session created (the one a // same-workspace attach under `single` scope reuses). Thread-scope // sessions add to `byId` but don't displace `defaultEntry`. let defaultEntry: SessionEntry | undefined; + // `channelInfo` is the SINGLE attach-available channel. Cleared when + // `killSession` marks the last session leaving (via `isDying`), + // when `doSpawn`'s newSession fails on an empty channel, or when + // `channel.exited` fires for the channel currently in this slot. + // Race-aware code paths (`ensureChannel`, `killAllSync`) gate on + // `isDying` rather than presence — see ChannelInfo.isDying for the + // BkUyD invariant. let channelInfo: ChannelInfo | undefined; - // tanzhenxin BkUyD: the channel reference is cleared ONLY when - // `channel.exited` fires (OS-level "really dead" signal), NOT - // eagerly during `shutdown()`. The double-Ctrl+C force-exit path - // (`killAllSync`) needs the reference until the OS has reaped the - // child — otherwise a second signal mid-SIGTERM-grace finds - // nothing to force-kill and `process.exit(1)` orphans the child. + // tanzhenxin BkUyD: superset of `channelInfo` covering channels + // that are dying but not yet OS-reaped. `killSession` / + // `doSpawn`-newSession-failure / `shutdown` mark a channel as + // `isDying` and start its async kill; meanwhile a concurrent + // `spawnOrAttach` can spawn a FRESH channel and reassign + // `channelInfo`. Without this set, the dying channel becomes + // unreachable — a double-Ctrl+C arriving mid-grace would call + // `killAllSync()`, find only the fresh channel in `channelInfo`, + // force-kill it, and `process.exit(1)` would orphan the dying one + // whose SIGTERM hadn't yet completed. The set is the OS-level + // "still alive" source of truth: entries are added when a channel + // is created and removed when its `channel.exited` resolves. + // `killAllSync` iterates THIS set to fire SIGKILL on every alive + // child regardless of whether it's still the attach target. + const aliveChannels = new Set(); // Coalesces a concurrent second `ensureChannel()` call onto the // first one's spawn so we never create two children for the same // daemon. Cleared in the `finally` of the creator. @@ -1162,6 +1178,7 @@ export function createHttpAcpBridge(opts: BridgeOptions): HttpAcpBridge { isDying: false, }; channelInfo = info; + aliveChannels.add(info); // One-time channel.exited cleanup. The child dying takes ALL // multiplexed sessions with it — iterate `sessionIds` (snapshot @@ -1169,14 +1186,17 @@ export function createHttpAcpBridge(opts: BridgeOptions): HttpAcpBridge { // iteration), publish `session_died` on each session's bus, // remove from byId / defaultEntry / pending tables. // - // tanzhenxin BkUyD: this is also where `channelInfo` is cleared. - // The async kill paths (`killSession` reap, `shutdown()` await, - // `doSpawn`'s newSession-failure tear-down) all leave - // `channelInfo` set until the OS has reaped the child, so the - // double-Ctrl+C `killAllSync` force-kill path still has a - // reference to fire SIGKILL against during the SIGTERM grace - // window. + // tanzhenxin BkUyD: drop from `aliveChannels` ONLY when the OS + // process is actually gone. Async kill paths (`killSession` + // reap, `shutdown()` await, `doSpawn`'s newSession-failure + // tear-down) mark `isDying = true` but leave the entry in + // `aliveChannels` until this handler fires, so the double-Ctrl+C + // `killAllSync` force-kill path still has a reference to fire + // SIGKILL against during the SIGTERM grace window — even if a + // concurrent `spawnOrAttach` has already reassigned + // `channelInfo` to a fresh channel. void channel.exited.then((exitInfo) => { + aliveChannels.delete(info); if (channelInfo === info) channelInfo = undefined; const sessions = Array.from(info.sessionIds); info.sessionIds.clear(); @@ -1990,19 +2010,27 @@ export function createHttpAcpBridge(opts: BridgeOptions): HttpAcpBridge { }, killAllSync() { - // Bd1y6: synchronous best-effort SIGKILL on the live channel. - // Set `shuttingDown` so any racing async path fails fast. - // tanzhenxin BkUyD: read `channelInfo` directly — it's only - // cleared by `channel.exited` (OS-level reap), NOT eagerly by - // `shutdown()`, so a mid-SIGTERM-grace double-signal still - // finds the live reference and force-kills. + // Bd1y6: synchronous best-effort SIGKILL on EVERY alive channel + // (typically 1, but during a `killSession`-then-`spawnOrAttach` + // overlap there can be 2 — the dying one in `aliveChannels` + // plus a fresh attach-target in `channelInfo`). Set + // `shuttingDown` so any racing async path fails fast. + // + // tanzhenxin BkUyD: iterate `aliveChannels` (the OS-level "still + // alive" source of truth) — `channelInfo` only points at the + // CURRENT attach target, missing any dying channel whose + // `channel.exited` hasn't fired yet. Pre-fix this iterated + // `channelInfo` alone; if a fresh spawn had already overwritten + // it during the prior channel's SIGTERM grace, the dying child + // received SIGTERM but no SIGKILL escalation when `process.exit(1)` + // fires and was orphaned. shuttingDown = true; - const ci = channelInfo; + const channels = Array.from(aliveChannels); defaultEntry = undefined; byId.clear(); - if (ci) { + for (const info of channels) { try { - ci.channel.killSync(); + info.channel.killSync(); } catch { /* best-effort — already-dead child / pid race */ } @@ -2016,18 +2044,20 @@ export function createHttpAcpBridge(opts: BridgeOptions): HttpAcpBridge { // spawning a child this teardown won't see. shuttingDown = true; const entries = Array.from(byId.values()); - // Snapshot the live channel (if any) — `channelInfo` itself is - // intentionally NOT cleared here; the `channel.exited` handler - // clears it once the OS has reaped the child. That preserves - // the BkUyD invariant: a double-Ctrl+C arriving mid-SIGTERM- - // grace can still find the channel via `killAllSync`. Setting - // `isDying` makes the dying channel invisible to any racing - // `ensureChannel` call — but `shuttingDown` already blocks new - // `spawnOrAttach` upstream, so this is mostly belt-and-suspenders - // (a direct internal `ensureChannel` past the gate would still - // see the dying state and not attach). - const ci = channelInfo; - if (ci) ci.isDying = true; + // Snapshot every alive channel (typically 1; up to 2 during a + // `killSession`-then-`spawnOrAttach` overlap) — entries are + // intentionally NOT removed from `aliveChannels` here; their + // `channel.exited` handlers clear them once the OS has reaped + // each child. That preserves the BkUyD invariant: a + // double-Ctrl+C arriving mid-SIGTERM-grace can still find every + // alive channel via `killAllSync`. Marking each `isDying` makes + // them invisible to any racing `ensureChannel` call — but + // `shuttingDown` already blocks new `spawnOrAttach` upstream, + // so this is mostly belt-and-suspenders (a direct internal + // `ensureChannel` past the gate would still see the dying + // state and not attach). + const channels = Array.from(aliveChannels); + for (const ci of channels) ci.isDying = true; // Resolve every still-pending permission as cancelled before clearing // the maps so callers awaiting `requestPermission` unwind cleanly. for (const e of entries) { @@ -2080,7 +2110,7 @@ export function createHttpAcpBridge(opts: BridgeOptions): HttpAcpBridge { ) : Promise.resolve(); await Promise.all([ - ci ? ci.channel.kill().catch(() => {}) : Promise.resolve(), + ...channels.map((ci) => ci.channel.kill().catch(() => {})), ...inFlightSessionAwaits, inFlightChannelAwait, ]); diff --git a/packages/cli/src/serve/server.test.ts b/packages/cli/src/serve/server.test.ts index 03bd75b105c..7ae04ad2445 100644 --- a/packages/cli/src/serve/server.test.ts +++ b/packages/cli/src/serve/server.test.ts @@ -4,6 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ +import { fileURLToPath } from 'node:url'; import { describe, it, expect, afterEach, vi } from 'vitest'; import request from 'supertest'; import { createServeApp } from './server.js'; @@ -1207,14 +1208,18 @@ describe('runQwenServe', () => { // Pointing the daemon at a file (vs. a directory) is operator error // — the agent would fail at child-spawn time with ENOTDIR. Catch // it at boot for a clearer error message. + // + // `fileURLToPath` (not `new URL(...).pathname`) — on Windows the + // latter returns `/C:/path/...` with a leading slash, which + // `statSync` resolves as path-from-current-drive-root and the + // test would then see ENOENT instead of the expected + // "not a directory" branch. await expect( runQwenServe({ hostname: '127.0.0.1', port: 0, mode: 'http-bridge', - // `import.meta.url` resolves to this test file's URL on Node; - // strip `file://` to get an absolute path that's known to exist. - workspace: new URL(import.meta.url).pathname, + workspace: fileURLToPath(import.meta.url), }), ).rejects.toThrow(/exists but is not a directory/); }); From cdaa760f5c8180d0c811ad02ce55252c590402c0 Mon Sep 17 00:00:00 2001 From: wenshao Date: Wed, 13 May 2026 19:37:33 +0800 Subject: [PATCH 04/16] =?UTF-8?q?docs(serve):=20update=20daemon-client-qui?= =?UTF-8?q?ckstart=20for=20=C2=A702=20single-workspace?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-3 review caught that the SDK example doc was the only one of the three serve-related docs that the §02 refactor didn't touch. Updated: - Boot log example now shows the `, workspace=/path/to/your-project` suffix that `runQwenServe` emits after the §02 changes. - The "Hello daemon" example now reads `caps.workspaceCwd` off `/capabilities` and passes it back as `workspaceCwd` on session creation — illustrating the documented pre-flight pattern, not a hand-written literal that may not match the daemon's actual bind. - Shared-session example makes the prerequisite explicit: the daemon must be bound to `/work/repo` (via `--workspace` or `cd`); under §02 two clients can only share a session if they're both hitting a daemon already bound to that workspace. - New "Workspace mismatch" section shows how to handle the `400 workspace_mismatch` error class: catching `DaemonHttpError`, branching on `body.code`, surfacing `boundWorkspace` / `requestedWorkspace` for the operator. This is a new error class SDK consumers' error handlers should branch on. No code changes; docs only. --- .../examples/daemon-client-quickstart.md | 50 +++++++++++++++++-- 1 file changed, 45 insertions(+), 5 deletions(-) diff --git a/docs/developers/examples/daemon-client-quickstart.md b/docs/developers/examples/daemon-client-quickstart.md index a72069aca47..35a7dd036ad 100644 --- a/docs/developers/examples/daemon-client-quickstart.md +++ b/docs/developers/examples/daemon-client-quickstart.md @@ -9,9 +9,11 @@ In one terminal: ```bash cd your-project/ qwen serve --port 4170 -# → qwen serve listening on http://127.0.0.1:4170 (mode=http-bridge) +# → qwen serve listening on http://127.0.0.1:4170 (mode=http-bridge, workspace=/path/to/your-project) ``` +Per [#3803](https://github.com/QwenLM/qwen-code/issues/3803) §02 each daemon binds to one workspace at boot (the current `cwd`, or override with `--workspace /path/to/dir`). The daemon's bound path is advertised on `/capabilities.workspaceCwd` so clients can pre-flight check + omit `cwd` from `POST /session`. + In another: ```bash @@ -28,13 +30,19 @@ const client = new DaemonClient({ // token: process.env.QWEN_SERVER_TOKEN, // required for non-loopback binds }); -// 1. Confirm we can reach the daemon and gate UI on its features. +// 1. Confirm we can reach the daemon, gate UI on its features, and +// read back the daemon's bound workspace (#3803 §02). const caps = await client.capabilities(); console.log('Daemon features:', caps.features); +console.log('Daemon workspace:', caps.workspaceCwd); // canonical bound path -// 2. Spawn-or-attach a session for the current workspace. +// 2. Spawn-or-attach a session. The `workspaceCwd` MUST match the +// daemon's bound workspace (`caps.workspaceCwd`) — pass it +// explicitly, OR omit it and the route falls back to the bound +// path. A mismatched `workspaceCwd` returns +// `400 workspace_mismatch` (see "Workspace mismatch" below). const session = await client.createOrAttachSession({ - workspaceCwd: process.cwd(), + workspaceCwd: caps.workspaceCwd, }); console.log(`session=${session.sessionId} attached=${session.attached}`); @@ -139,9 +147,12 @@ case 'permission_request': { ## Shared-session collaboration -Two clients pointed at the same daemon and `cwd` end up on the same session: +Two clients pointed at the **same daemon** end up on the same session. Per #3803 §02 each daemon is bound to ONE workspace at boot, so the daemon launched as `qwen serve --workspace /work/repo` (or `cd /work/repo && qwen serve`) is what both clients connect to: ```ts +// Daemon was launched as `qwen serve --workspace /work/repo` so +// `caps.workspaceCwd === '/work/repo'` for both clients. + // Client A (e.g. an IDE plugin) const a = await clientA.createOrAttachSession({ workspaceCwd: '/work/repo' }); console.log(a.attached); // false — A spawned the agent @@ -154,6 +165,35 @@ console.log(a.sessionId === b.sessionId); // true Both clients see the same `session_update` / `permission_request` stream. Either can send a prompt; they FIFO-queue per the agent's "one active prompt per session" guarantee. +## Workspace mismatch + +If `workspaceCwd` doesn't match the daemon's bound workspace, `createOrAttachSession` rejects with `DaemonHttpError` carrying status `400` and a structured body: + +```ts +import { DaemonHttpError } from '@qwen-code/sdk'; + +try { + await client.createOrAttachSession({ workspaceCwd: '/some/other/project' }); +} catch (err) { + if (err instanceof DaemonHttpError && err.status === 400) { + const body = err.body as { + code?: string; + boundWorkspace?: string; + requestedWorkspace?: string; + }; + if (body.code === 'workspace_mismatch') { + console.error( + `This daemon is bound to ${body.boundWorkspace}, ` + + `not ${body.requestedWorkspace}. Start a separate daemon ` + + `for that workspace, or route to the right one.`, + ); + } + } +} +``` + +Multi-workspace deployments run one daemon per workspace on separate ports — there's no intra-daemon routing under §02. An orchestrator (or the user's launcher) picks the right daemon based on the project the client wants to talk to. + ## Authentication When the daemon was started with a token (any non-loopback bind requires one): From 5e309a90bb720510a8d895eae4e1c3d933bc76e8 Mon Sep 17 00:00:00 2001 From: wenshao Date: Wed, 13 May 2026 19:49:45 +0800 Subject: [PATCH 05/16] =?UTF-8?q?feat(sdk,test):=20align=20SDK=20types=20+?= =?UTF-8?q?=20integration=20tests=20with=20=C2=A702=20single-workspace?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-4 review caught one type-drift gap + a set of integration-test assumptions that the §02 refactor invalidated. **SDK type drift.** `DaemonCapabilities` in `packages/sdk-typescript/src/daemon/types.ts` was the SDK-side mirror of `CapabilitiesEnvelope` on the daemon side. The §02 PR added `workspaceCwd: string` to the daemon envelope (and the round-3 doc example reads `caps.workspaceCwd` off the SDK client) but the SDK type wasn't updated. A TypeScript consumer copying the doc snippet verbatim would hit `TS2339 'workspaceCwd' does not exist on type 'DaemonCapabilities'`. The wire field is present so JS consumers wouldn't notice — but the SDK is marketed as a TypeScript quickstart, so this is a real onboarding break. Fix: add `workspaceCwd: string` to `DaemonCapabilities` (parallel to `DaemonSession.workspaceCwd` which is already there). The SDK unit test for `client.capabilities()` was updated to put the new field in the mocked response. **Integration tests.** `qwen-serve-routes.test.ts` spawns a real `qwen serve` daemon in `beforeAll`. Three breakages exposed: 1. The daemon was launched without `--workspace`, so it inherited the test runner's `cwd`. Tests then POST `workspaceCwd: REPO_ROOT` assuming the daemon is bound to the repo root — true when run via `npm test` from the repo, brittle from IDEs / launchers that have a different `cwd`. Added `'--workspace', REPO_ROOT` to the spawn args so the bound workspace is deterministic regardless of where the test runner is launched. 2. The `bad modelServiceId` test used `cwd: '/tmp'`. Under §02 this would now return 400 workspace_mismatch before the session was spawned. Switched to `REPO_ROOT` and softened the `attached` assertion (REPO_ROOT may already have a session from earlier tests in the suite under sessionScope:single). 3. Added three new integration tests pinning the §02 surface end-to-end through a real daemon process: - `rejects cross-workspace cwd with 400 workspace_mismatch` — posts `/tmp` and asserts the full structured error body (`code`, `boundWorkspace`, `requestedWorkspace`). - `omits cwd → falls back to bound workspace` — posts an empty body and asserts the response's `workspaceCwd` matches REPO_ROOT (verifies the runQwenServe → createServeApp → bridge fallback plumbing). - `GET /capabilities surfaces workspaceCwd` — asserts the new SDK type field is populated correctly off the wire. All 422 unit tests pass (cli serve + sdk). Integration tests typecheck clean. --- .../cli/qwen-serve-routes.test.ts | 79 +++++++++++++++++-- packages/sdk-typescript/src/daemon/types.ts | 11 +++ .../test/unit/DaemonClient.test.ts | 4 + 3 files changed, 87 insertions(+), 7 deletions(-) diff --git a/integration-tests/cli/qwen-serve-routes.test.ts b/integration-tests/cli/qwen-serve-routes.test.ts index 295cdb1c7bd..c5dfb0c790e 100644 --- a/integration-tests/cli/qwen-serve-routes.test.ts +++ b/integration-tests/cli/qwen-serve-routes.test.ts @@ -60,6 +60,14 @@ beforeAll(async () => { TOKEN, '--hostname', '127.0.0.1', + // Per #3803 §02 (1 daemon = 1 workspace), pin the bound + // workspace so test assertions that POST `workspaceCwd: + // REPO_ROOT` succeed regardless of where the test runner + // happens to be cwd'd. Without this the daemon would inherit + // the test runner's cwd, which is brittle across CI / local + // / IDE-launcher environments. + '--workspace', + REPO_ROOT, ], { stdio: ['ignore', 'pipe', 'pipe'] }, ); @@ -221,20 +229,77 @@ describe('qwen serve — POST /session validation + concurrent coalescing', () = // Tearing the session down on model-switch failure would force // the caller into a 500 with no way to recover. The // `model_switch_failed` SSE event is the visible failure signal. - const cwd = '/tmp'; + // + // Use REPO_ROOT (the daemon's bound workspace) — under #3803 §02 + // any other cwd would return 400 workspace_mismatch before the + // session is even spawned. + const cwd = REPO_ROOT; const session = await client.createOrAttachSession({ workspaceCwd: cwd, modelServiceId: 'definitely-not-a-real-model', }); expect(session.sessionId).toBeTypeOf('string'); - expect(session.attached).toBe(false); + // `attached` may be true or false depending on whether earlier + // tests in this file already created a REPO_ROOT session. The + // shape of the response is what matters here (sessionId present, + // listWorkspaceSessions sees it). + expect(typeof session.attached).toBe('boolean'); const sessions = await client.listWorkspaceSessions(cwd); - expect(sessions).toHaveLength(1); - expect(sessions[0]?.sessionId).toBe(session.sessionId); + expect(sessions.some((s) => s.sessionId === session.sessionId)).toBe(true); // No teardown — Stage 1 has no DELETE /session route, and the - // session persists in `byId` until daemon shutdown. The other - // tests in this file use unique workspace cwds so the surviving - // session here doesn't interfere. + // session persists in `byId` until daemon shutdown. + }); + + it('rejects cross-workspace cwd with 400 workspace_mismatch (#3803 §02)', async () => { + // The daemon is bound to REPO_ROOT (via `--workspace` in beforeAll). + // A POST /session with `cwd: '/tmp'` (or any other absolute path + // that doesn't canonicalize to REPO_ROOT) must reject with 400 + // `workspace_mismatch`, carrying both paths in the body so an + // orchestrator-aware client can spawn / route to the right + // daemon. + const res = await fetch(`${base}/session`, { + method: 'POST', + headers: { + Authorization: `Bearer ${TOKEN}`, + 'content-type': 'application/json', + }, + body: JSON.stringify({ cwd: '/tmp' }), + }); + expect(res.status).toBe(400); + const body = (await res.json()) as { + code?: string; + boundWorkspace?: string; + requestedWorkspace?: string; + }; + expect(body.code).toBe('workspace_mismatch'); + expect(body.boundWorkspace).toBe(REPO_ROOT); + expect(body.requestedWorkspace).toBe('/tmp'); + }); + + it('omits cwd → falls back to bound workspace (#3803 §02)', async () => { + // The route accepts an empty body and falls back to the daemon's + // bound workspace. Asserting this end-to-end through a real + // daemon process verifies the runQwenServe → createServeApp → + // bridge plumbing for the fallback path. + const res = await fetch(`${base}/session`, { + method: 'POST', + headers: { + Authorization: `Bearer ${TOKEN}`, + 'content-type': 'application/json', + }, + body: JSON.stringify({}), + }); + expect(res.status).toBe(200); + const session = (await res.json()) as { + sessionId?: string; + workspaceCwd?: string; + }; + expect(session.workspaceCwd).toBe(REPO_ROOT); + }); + + it('GET /capabilities surfaces workspaceCwd (#3803 §02)', async () => { + const caps = await client.capabilities(); + expect(caps.workspaceCwd).toBe(REPO_ROOT); }); }); diff --git a/packages/sdk-typescript/src/daemon/types.ts b/packages/sdk-typescript/src/daemon/types.ts index 29d49c0da82..0d7166b20ba 100644 --- a/packages/sdk-typescript/src/daemon/types.ts +++ b/packages/sdk-typescript/src/daemon/types.ts @@ -25,6 +25,17 @@ export interface DaemonCapabilities { */ features: string[]; modelServices: string[]; + /** + * Absolute canonical workspace path this daemon is bound to + * (per #3803 §02: 1 daemon = 1 workspace). Clients use this to + * (a) detect mismatch before posting `/session` (vs. waiting for + * a 400 `workspace_mismatch` response), and (b) omit `cwd` on + * `POST /session` — the route falls back to this path when the + * body has no `cwd` field. Multi-workspace deployments expose + * multiple daemons on different ports, each advertising its own + * `workspaceCwd`. + */ + workspaceCwd: string; } /** Returned from `POST /session`. */ diff --git a/packages/sdk-typescript/test/unit/DaemonClient.test.ts b/packages/sdk-typescript/test/unit/DaemonClient.test.ts index 27c9aadb14f..bdc22315973 100644 --- a/packages/sdk-typescript/test/unit/DaemonClient.test.ts +++ b/packages/sdk-typescript/test/unit/DaemonClient.test.ts @@ -96,11 +96,15 @@ describe('DaemonClient', () => { mode: 'http-bridge' as const, features: ['health', 'capabilities'], modelServices: [], + workspaceCwd: '/work/bound', }; const { fetch } = recordingFetch(() => jsonResponse(200, envelope)); const client = new DaemonClient({ baseUrl: 'http://daemon', fetch }); const caps = await client.capabilities(); expect(caps).toEqual(envelope); + // #3803 §02: clients use `workspaceCwd` to pre-flight check + + // omit `cwd` from `POST /session` (route falls back). + expect(caps.workspaceCwd).toBe('/work/bound'); }); }); From 4d41fcee76d8d9af9d64b363c60c036cf6d2fcd8 Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Wed, 13 May 2026 22:44:40 +0800 Subject: [PATCH 06/16] fix(serve): address /review feedback from gpt-5.5 + deepseek-v4-pro MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Process the 7 inline /review comments on PR #4113: - C1+C3 (SDK): make `DaemonCapabilities.workspaceCwd` and `CreateSessionRequest.workspaceCwd` optional in the SDK types. `workspaceCwd` is an additive field on the v=1 envelope per #3803 §02; the protocol's "bump v only on incompatible changes" stance is honored by leaving the field optional at the type level. `DaemonClient.createOrAttachSession` now omits `cwd` from the body when `workspaceCwd` isn't passed, matching the PR description's "SDK accepts bound path or none". Adds a unit test pinning the empty-body shape. - C2 (docs/users/qwen-serve.md): the `--http-bridge` row described the pre-§02 per-session model; updated to reflect one child per daemon with N sessions multiplexed via ACP `newSession()`. - C4 (server.ts): `WorkspaceMismatchError` was silently 400'ing without a stderr breadcrumb, leaving operators blind to cross-workspace routing drift. Mirrors the SessionLimitExceeded /InvalidPermissionOption observability pattern. - C5 (server.test.ts): the `/capabilities` fallback test compared `res.body.workspaceCwd` against raw `process.cwd()`; on macOS default tmpdir flows (`/var/folders/...` → `/private/var/...`) the canonicalize-once route value diverges. Use `realpathSync.native(process.cwd())` to match the route's canonicalization. - C6 (server.ts): the cwd-not-absolute error said "cwd is required and must be an absolute path" but cwd is now optional under §02. Tightened wording to "must be an absolute path when provided". - C7 (runQwenServe.ts): the `statSync` catch only wrapped ENOENT with a friendly diagnostic; EACCES / EPERM (typical for SIP-protected dirs on macOS or root-owned paths the daemon's UID can't traverse) re-threw as raw `SystemError`. Wrap both codes with a `--workspace`-context message so the boot failure points at the flag the operator set. Docs: quickstart shows the explicit-pass-or-omit options side by side; protocol reference notes `workspaceCwd` is additive to v=1. --- .../examples/daemon-client-quickstart.md | 14 +++++++++----- docs/developers/qwen-serve-protocol.md | 2 +- docs/users/qwen-serve.md | 2 +- packages/cli/src/serve/runQwenServe.ts | 14 ++++++++++++++ packages/cli/src/serve/server.test.ts | 9 ++++++++- packages/cli/src/serve/server.ts | 15 ++++++++++++++- .../sdk-typescript/src/daemon/DaemonClient.ts | 18 ++++++++++++++++-- packages/sdk-typescript/src/daemon/types.ts | 9 ++++++++- .../test/unit/DaemonClient.test.ts | 19 +++++++++++++++++++ 9 files changed, 90 insertions(+), 12 deletions(-) diff --git a/docs/developers/examples/daemon-client-quickstart.md b/docs/developers/examples/daemon-client-quickstart.md index 35a7dd036ad..16d1dd864ce 100644 --- a/docs/developers/examples/daemon-client-quickstart.md +++ b/docs/developers/examples/daemon-client-quickstart.md @@ -36,11 +36,15 @@ const caps = await client.capabilities(); console.log('Daemon features:', caps.features); console.log('Daemon workspace:', caps.workspaceCwd); // canonical bound path -// 2. Spawn-or-attach a session. The `workspaceCwd` MUST match the -// daemon's bound workspace (`caps.workspaceCwd`) — pass it -// explicitly, OR omit it and the route falls back to the bound -// path. A mismatched `workspaceCwd` returns -// `400 workspace_mismatch` (see "Workspace mismatch" below). +// 2. Spawn-or-attach a session. Two equally-valid shapes: +// (a) pass `workspaceCwd: caps.workspaceCwd` to be explicit, or +// (b) omit `workspaceCwd` entirely — the SDK then sends no `cwd` +// field and the daemon route falls back to its bound +// workspace. The (b) shape is concise but assumes you trust +// `caps.workspaceCwd` to be whatever you intended. +// A non-empty `workspaceCwd` that doesn't canonicalize to the +// daemon's bound path yields `400 workspace_mismatch` (see +// "Workspace mismatch" below). const session = await client.createOrAttachSession({ workspaceCwd: caps.workspaceCwd, }); diff --git a/docs/developers/qwen-serve-protocol.md b/docs/developers/qwen-serve-protocol.md index 583c2ac8814..74224ab1cfc 100644 --- a/docs/developers/qwen-serve-protocol.md +++ b/docs/developers/qwen-serve-protocol.md @@ -115,7 +115,7 @@ Stable contract: when `v` increments the frame layout has changed in a backwards > **`modelServices` is always `[]` in Stage 1.** The agent uses its single default model service and doesn't enumerate it over the wire. Stage 2 will populate this from registered model adapters so SDK clients can build service-pickers; until then, do NOT rely on this field being non-empty. -> **`workspaceCwd`** is the canonical absolute path this daemon binds to (#3803 §02 — 1 daemon = 1 workspace). Use it to (a) detect mismatch before posting `/session` and (b) omit `cwd` on `POST /session` (the route falls back to this path). Multi-workspace deployments expose multiple daemons on different ports, each with its own `workspaceCwd`. +> **`workspaceCwd`** is the canonical absolute path this daemon binds to (#3803 §02 — 1 daemon = 1 workspace). Use it to (a) detect mismatch before posting `/session` and (b) omit `cwd` on `POST /session` (the route falls back to this path). Multi-workspace deployments expose multiple daemons on different ports, each with its own `workspaceCwd`. Additive to v=1: pre-§02 v=1 daemons omit the field — clients that target older builds should null-check before consuming it. ### `POST /session` diff --git a/docs/users/qwen-serve.md b/docs/users/qwen-serve.md index f398f0c048f..1c7ef153218 100644 --- a/docs/users/qwen-serve.md +++ b/docs/users/qwen-serve.md @@ -115,7 +115,7 @@ The token comparison is constant-time (SHA-256 + `crypto.timingSafeEqual`); 401 | `--max-sessions ` | `20` | Cap on concurrent live sessions. New `POST /session` requests that would spawn a fresh child return `503` (with `Retry-After: 5`) when the cap is hit; attaches to existing sessions are NOT counted. Set to `0` to disable. Sized for single-user / small-team usage; raise it if your deployment has the RAM/FD headroom (~30–50 MB per session). | | `--workspace ` | `process.cwd()` | Absolute workspace path this daemon binds to (per [#3803](https://github.com/QwenLM/qwen-code/issues/3803) §02 — 1 daemon = 1 workspace). `POST /session` requests with a mismatched `cwd` return `400 workspace_mismatch`. For multi-workspace deployments, run one `qwen serve` per workspace on separate ports. | | `--max-connections ` | `256` | Listener-level TCP connection cap (`server.maxConnections`). Bounds raw socket count irrespective of session count — slow / phantom SSE clients get rejected at accept time once full. Raise alongside `--max-sessions` if your deployment expects many SSE subscribers per session. | -| `--http-bridge` | `true` | Stage 1 mode: per-session `qwen --acp` child process. Stage 2 native in-process becomes available later. | +| `--http-bridge` | `true` | Stage 1 mode: one `qwen --acp` child per daemon (bound to one workspace at boot, per [#3803](https://github.com/QwenLM/qwen-code/issues/3803) §02); N sessions multiplex onto that child via ACP `newSession()`. Stage 2 native in-process becomes available later. | > **Sizing the load knobs.** `--max-sessions` is the **new-child** cap. > Three other layers also limit load — when sizing for a high-concurrency diff --git a/packages/cli/src/serve/runQwenServe.ts b/packages/cli/src/serve/runQwenServe.ts index f2092dd7502..fc3e7ed54e3 100644 --- a/packages/cli/src/serve/runQwenServe.ts +++ b/packages/cli/src/serve/runQwenServe.ts @@ -139,6 +139,20 @@ export async function runQwenServe( `Invalid --workspace "${rawWorkspace}": directory does not exist.`, ); } + // EACCES / EPERM: the path exists but the current user can't + // stat it (typical for SIP-protected paths on macOS, root-owned + // dirs the daemon's user can't traverse, etc.). The raw Node + // SystemError has the path AND the syscall but no operator- + // facing breadcrumb that this came from `--workspace`. Wrap + // both codes so the boot failure points at the flag the + // operator actually set. + if (code === 'EACCES' || code === 'EPERM') { + throw new Error( + `Invalid --workspace "${rawWorkspace}": permission denied ` + + `(${String(code)}). The path exists but cannot be stat'd ` + + `by the current user.`, + ); + } } throw err; } diff --git a/packages/cli/src/serve/server.test.ts b/packages/cli/src/serve/server.test.ts index 7ae04ad2445..1351a9f7b0d 100644 --- a/packages/cli/src/serve/server.test.ts +++ b/packages/cli/src/serve/server.test.ts @@ -4,6 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ +import { realpathSync } from 'node:fs'; import { fileURLToPath } from 'node:url'; import { describe, it, expect, afterEach, vi } from 'vitest'; import request from 'supertest'; @@ -217,7 +218,13 @@ describe('createServeApp', () => { .get('/capabilities') .set('Host', `127.0.0.1:${baseOpts.port}`); expect(res.status).toBe(200); - expect(res.body.workspaceCwd).toBe(process.cwd()); + // `createServeApp` runs `canonicalizeWorkspace` on + // `process.cwd()`, which collapses symlinks via + // `realpathSync.native`. On macOS the default tmpdir is + // `/var/folders/...` whose canonical form is + // `/private/var/folders/...`; a raw `process.cwd()` assertion + // would diverge there. Use the same realpath the route does. + expect(res.body.workspaceCwd).toBe(realpathSync.native(process.cwd())); }); }); diff --git a/packages/cli/src/serve/server.ts b/packages/cli/src/serve/server.ts index 7841766186d..64bf5b28c12 100644 --- a/packages/cli/src/serve/server.ts +++ b/packages/cli/src/serve/server.ts @@ -184,7 +184,7 @@ export function createServeApp( if (!path.isAbsolute(cwd)) { res .status(400) - .json({ error: '`cwd` is required and must be an absolute path' }); + .json({ error: '`cwd` must be an absolute path when provided' }); return; } const modelServiceId = @@ -840,6 +840,19 @@ function sendBridgeError( // the wrong daemon for their workspace). Body includes both // paths so orchestrator-aware clients can route to the right // daemon / spawn a new one. + // + // Operator log line: unlike SessionNotFoundError (per-session + // 404 with rich URL context), workspace_mismatch indicates an + // orchestration / deployment drift (operator booted with the + // wrong workspace, or client is routing to the wrong daemon). + // Without a breadcrumb the daemon's log looks healthy while + // every client request silently 400s. Limited to authenticated + // requests by the upstream bearer-token gate, so probing-DoS + // log noise stays bounded. + writeStderrLine( + `qwen serve: workspace_mismatch (POST /session): ` + + `daemon bound to "${err.bound}", rejected "${err.requested}"`, + ); res.status(400).json({ error: err.message, code: 'workspace_mismatch', diff --git a/packages/sdk-typescript/src/daemon/DaemonClient.ts b/packages/sdk-typescript/src/daemon/DaemonClient.ts index 3983c63af9e..d07051cee5b 100644 --- a/packages/sdk-typescript/src/daemon/DaemonClient.ts +++ b/packages/sdk-typescript/src/daemon/DaemonClient.ts @@ -88,7 +88,16 @@ export class DaemonHttpError extends Error { } export interface CreateSessionRequest { - workspaceCwd: string; + /** + * Workspace path the daemon must be bound to (per #3803 §02). When + * omitted, the SDK sends no `cwd` field and the daemon route falls + * back to its boot-time `boundWorkspace`. Pass `caps.workspaceCwd` + * to be explicit, or omit it for the daemon-knows-best path. A + * non-empty `workspaceCwd` that doesn't canonicalize to the + * daemon's bound path yields a `400 workspace_mismatch` + * `DaemonHttpError`. + */ + workspaceCwd?: string; modelServiceId?: string; } @@ -250,13 +259,18 @@ export class DaemonClient { async createOrAttachSession( req: CreateSessionRequest, ): Promise { + // Per #3803 §02: omitting `cwd` lets the daemon fall back to its + // bound workspace. Send `cwd` ONLY when the caller passed + // `workspaceCwd` — sending `cwd: undefined` would JSON.stringify + // it out anyway, but conditional spread keeps the on-wire body + // shape obvious to anyone tracing the request. return await this.fetchWithTimeout( `${this.baseUrl}/session`, { method: 'POST', headers: this.headers({ 'Content-Type': 'application/json' }), body: JSON.stringify({ - cwd: req.workspaceCwd, + ...(req.workspaceCwd ? { cwd: req.workspaceCwd } : {}), ...(req.modelServiceId ? { modelServiceId: req.modelServiceId } : {}), }), }, diff --git a/packages/sdk-typescript/src/daemon/types.ts b/packages/sdk-typescript/src/daemon/types.ts index 0d7166b20ba..865aa0b5be1 100644 --- a/packages/sdk-typescript/src/daemon/types.ts +++ b/packages/sdk-typescript/src/daemon/types.ts @@ -34,8 +34,15 @@ export interface DaemonCapabilities { * body has no `cwd` field. Multi-workspace deployments expose * multiple daemons on different ports, each advertising its own * `workspaceCwd`. + * + * Optional at the type level because the field is an additive + * extension to v=1 envelopes (added by #3803 §02). Daemons + * predating §02 still announce `v: 1` but omit this field; the + * protocol's "bump v only on incompatible frame changes" stance + * (see `qwen-serve-protocol.md`) makes additive optionality the + * correct shape. All post-§02 daemons populate it. */ - workspaceCwd: string; + workspaceCwd?: string; } /** Returned from `POST /session`. */ diff --git a/packages/sdk-typescript/test/unit/DaemonClient.test.ts b/packages/sdk-typescript/test/unit/DaemonClient.test.ts index bdc22315973..06f74621329 100644 --- a/packages/sdk-typescript/test/unit/DaemonClient.test.ts +++ b/packages/sdk-typescript/test/unit/DaemonClient.test.ts @@ -151,6 +151,25 @@ describe('DaemonClient', () => { expect(JSON.parse(calls[0]!.body!)).toEqual({ cwd: '/work/a' }); }); + it('omits cwd when workspaceCwd is not provided (#3803 §02)', async () => { + // Per #3803 §02 the daemon route falls back to its bound + // workspace when `cwd` is absent. The SDK must therefore NOT + // send `cwd: undefined` (or any other placeholder) when the + // caller omits `workspaceCwd` — otherwise the wire body + // carries an explicit empty field that wouldn't ever match + // the fallback semantics on a future stricter daemon. + const { fetch, calls } = recordingFetch(() => + jsonResponse(200, { + sessionId: 's-1', + workspaceCwd: '/work/bound', + attached: false, + }), + ); + const client = new DaemonClient({ baseUrl: 'http://daemon', fetch }); + await client.createOrAttachSession({}); + expect(JSON.parse(calls[0]!.body!)).toEqual({}); + }); + it('forwards modelServiceId when supplied', async () => { const { fetch, calls } = recordingFetch(() => jsonResponse(200, { From adcb3f983650dd5032c2278d18553ca727645810 Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Wed, 13 May 2026 23:44:08 +0800 Subject: [PATCH 07/16] fix(serve/test): make /work/bound literals Windows-portable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Windows CI failed on this PR's two new tests because returns (drive-relative absolute), so the route's canonicalize step diverged from the hardcoded literal. Mirror the WS_A/WS_B pattern already used in httpAcpBridge.test.ts: define WS_BOUND / WS_DIFFERENT via `path.resolve(path.sep, …)` and use the constants everywhere. The 400 workspace_mismatch test would still have passed (mock controls both throw + assertion) but I aligned it for consistency. Failures from CI run 25806528710: expected 'D:\work\bound' to be '/work/bound' (Object.is) Affected tests: - createServeApp > GET /capabilities > reports the bound workspace - createServeApp > POST /session > 200 when cwd is omitted --- packages/cli/src/serve/server.test.ts | 31 ++++++++++++++++++--------- 1 file changed, 21 insertions(+), 10 deletions(-) diff --git a/packages/cli/src/serve/server.test.ts b/packages/cli/src/serve/server.test.ts index 1351a9f7b0d..8e877ed1b6c 100644 --- a/packages/cli/src/serve/server.test.ts +++ b/packages/cli/src/serve/server.test.ts @@ -5,6 +5,7 @@ */ import { realpathSync } from 'node:fs'; +import * as path from 'node:path'; import { fileURLToPath } from 'node:url'; import { describe, it, expect, afterEach, vi } from 'vitest'; import request from 'supertest'; @@ -41,6 +42,16 @@ const baseOpts: ServeOptions = { mode: 'http-bridge', }; +// Workspace fixtures must round-trip through `path.resolve` so the +// expected values match the canonicalized form the route produces on +// every platform. On Windows `path.resolve('/work/bound')` returns +// `D:\work\bound` (drive-relative absolute), so hardcoding `/work/bound` +// as a literal makes the test fail on Windows CI even though the code +// is correct. Mirror the pattern used by httpAcpBridge.test.ts (WS_A / +// WS_B). +const WS_BOUND = path.resolve(path.sep, 'work', 'bound'); +const WS_DIFFERENT = path.resolve(path.sep, 'work', 'different'); + interface FakeBridgeOpts { spawnImpl?: (req: BridgeSpawnRequest) => Promise; promptImpl?: ( @@ -204,12 +215,12 @@ describe('createServeApp', () => { }); it('reports the bound workspace (#3803 §02)', async () => { - const app = createServeApp({ ...baseOpts, workspace: '/work/bound' }); + const app = createServeApp({ ...baseOpts, workspace: WS_BOUND }); const res = await request(app) .get('/capabilities') .set('Host', `127.0.0.1:${baseOpts.port}`); expect(res.status).toBe(200); - expect(res.body.workspaceCwd).toBe('/work/bound'); + expect(res.body.workspaceCwd).toBe(WS_BOUND); }); it('falls back to process.cwd() when --workspace is omitted', async () => { @@ -310,7 +321,7 @@ describe('createServeApp', () => { // omit `cwd` and the route falls back to the bound path. const bridge = fakeBridge(); const app = createServeApp( - { ...baseOpts, workspace: '/work/bound' }, + { ...baseOpts, workspace: WS_BOUND }, undefined, { bridge }, ); @@ -319,13 +330,13 @@ describe('createServeApp', () => { .set('Host', `127.0.0.1:${baseOpts.port}`) .send({}); expect(res.status).toBe(200); - expect(bridge.calls[0]?.workspaceCwd).toBe('/work/bound'); + expect(bridge.calls[0]?.workspaceCwd).toBe(WS_BOUND); }); it('400 when cwd is relative', async () => { const bridge = fakeBridge(); const app = createServeApp( - { ...baseOpts, workspace: '/work/bound' }, + { ...baseOpts, workspace: WS_BOUND }, undefined, { bridge }, ); @@ -344,23 +355,23 @@ describe('createServeApp', () => { // body so orchestrator-aware clients can route correctly. const bridge = fakeBridge({ spawnImpl: async (req) => { - throw new WorkspaceMismatchError('/work/bound', req.workspaceCwd); + throw new WorkspaceMismatchError(WS_BOUND, req.workspaceCwd); }, }); const app = createServeApp( - { ...baseOpts, workspace: '/work/bound' }, + { ...baseOpts, workspace: WS_BOUND }, undefined, { bridge }, ); const res = await request(app) .post('/session') .set('Host', `127.0.0.1:${baseOpts.port}`) - .send({ cwd: '/work/different' }); + .send({ cwd: WS_DIFFERENT }); expect(res.status).toBe(400); expect(res.body).toMatchObject({ code: 'workspace_mismatch', - boundWorkspace: '/work/bound', - requestedWorkspace: '/work/different', + boundWorkspace: WS_BOUND, + requestedWorkspace: WS_DIFFERENT, }); }); From c922a4e862098e7edf705af22db06a53ca96a9dd Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Thu, 14 May 2026 07:41:41 +0800 Subject: [PATCH 08/16] fix(serve): address second /review round (gpt-5.5 + deepseek-v4-pro) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four new inline findings from the latest /review pass: - N1 (integration-tests/cli/qwen-serve-routes.test.ts) — Critical: the `workspace_mismatch` assertion compared `requestedWorkspace` against the literal `'/tmp'`, but the bridge canonicalizes via `realpathSync.native` and on macOS `/tmp` is a symlink to `/private/tmp`. Compare against `realpathSync.native('/tmp')` so the assertion is portable. - N2 (packages/cli/src/serve/types.ts): `CapabilitiesEnvelope.workspaceCwd: string` (server side) diverged from the SDK's `DaemonCapabilities.workspaceCwd?: string`. Made the server type optional too — matches the SDK, matches the protocol doc's "additive to v=1" framing, doesn't change runtime emission (the post-§02 server still always populates the field). - N3 + N4 (packages/cli/src/serve/server.ts + sdk-typescript/.../DaemonClient.ts): the route's `cwd` validation treated every non-string body value (`null`, `123`, `{}`, `[]`) the same as omitted, silently falling back to `boundWorkspace`. That hid client/orchestrator serialization bugs as "session attached to wrong workspace". Now the route uses `'cwd' in body` to detect presence and rejects presence-but-not-a-string with `400 'cwd must be a string absolute path when provided'`. Empty string still hits the existing `path.isAbsolute` branch ("must be an absolute path when provided"), so an SDK caller passing `workspaceCwd: ''` no longer silently lands in the daemon's bound workspace. SDK side: reverted my conditional spread to `cwd: req.workspaceCwd` unconditional. `JSON.stringify` strips `undefined` automatically (so omitted `workspaceCwd` becomes "no `cwd` key" on the wire, as before), but empty-string is now forwarded verbatim and the server's 400 surfaces the bug instead of the SDK swallowing it. Added a unit test pinning the empty-string-forwarded shape. Server tests: - `400 when cwd is present but not a string` covers null / number / object / array via a sub-loop. - `400 when cwd is the empty string` pins the isAbsolute path. bridge: 73/73; server: 80/80 (was 78, +2 new); SDK: 40/40 (was 39, +1 empty-string test). tsc clean for SDK and PR-touched CLI files. --- .../cli/qwen-serve-routes.test.ts | 8 ++- packages/cli/src/serve/server.test.ts | 52 +++++++++++++++++++ packages/cli/src/serve/server.ts | 30 ++++++++--- packages/cli/src/serve/types.ts | 10 +++- .../sdk-typescript/src/daemon/DaemonClient.ts | 17 ++++-- .../test/unit/DaemonClient.test.ts | 25 +++++++-- 6 files changed, 123 insertions(+), 19 deletions(-) diff --git a/integration-tests/cli/qwen-serve-routes.test.ts b/integration-tests/cli/qwen-serve-routes.test.ts index c5dfb0c790e..bef03a6e6c0 100644 --- a/integration-tests/cli/qwen-serve-routes.test.ts +++ b/integration-tests/cli/qwen-serve-routes.test.ts @@ -20,6 +20,7 @@ * in `qwen-serve-streaming.test.ts` and skip when no auth is set. */ import { spawn, type ChildProcess } from 'node:child_process'; +import { realpathSync } from 'node:fs'; import * as path from 'node:path'; import { fileURLToPath } from 'node:url'; import { afterAll, beforeAll, describe, expect, it } from 'vitest'; @@ -273,7 +274,12 @@ describe('qwen serve — POST /session validation + concurrent coalescing', () = }; expect(body.code).toBe('workspace_mismatch'); expect(body.boundWorkspace).toBe(REPO_ROOT); - expect(body.requestedWorkspace).toBe('/tmp'); + // The bridge canonicalizes the requested cwd via `realpathSync.native` + // so the response carries the on-disk canonical form, NOT the literal + // we POSTed. On macOS `/tmp` is a symlink to `/private/tmp`, so the + // hardcoded `/tmp` literal would diverge there. Resolve the same way + // the bridge does to keep the assertion portable. + expect(body.requestedWorkspace).toBe(realpathSync.native('/tmp')); }); it('omits cwd → falls back to bound workspace (#3803 §02)', async () => { diff --git a/packages/cli/src/serve/server.test.ts b/packages/cli/src/serve/server.test.ts index 8e877ed1b6c..4f7f2f68ede 100644 --- a/packages/cli/src/serve/server.test.ts +++ b/packages/cli/src/serve/server.test.ts @@ -348,6 +348,58 @@ describe('createServeApp', () => { expect(bridge.calls).toHaveLength(0); }); + it('400 when cwd is present but not a string (#3803 §02 — distinguishes omitted vs malformed)', async () => { + // Three non-string shapes a buggy client / orchestrator could + // serialize for the `cwd` field: `null`, a number, an object. + // Pre-fix the route treated all three the same as "omitted" and + // fell back to `boundWorkspace`, silently masking client bugs. + // Now the route distinguishes "absent" (legitimate §02 fallback) + // from "present but malformed" (client-side bug → 400 + actionable + // error message). Empty string still falls through to the + // `path.isAbsolute` check (and 400s there with the + // "absolute path when provided" message). + const malformed: unknown[] = [null, 123, { foo: 'bar' }, []]; + for (const cwd of malformed) { + const bridge = fakeBridge(); + const app = createServeApp( + { ...baseOpts, workspace: WS_BOUND }, + undefined, + { bridge }, + ); + const res = await request(app) + .post('/session') + .set('Host', `127.0.0.1:${baseOpts.port}`) + .send({ cwd }); + expect(res.status).toBe(400); + expect(res.body.error).toMatch(/must be a string absolute path/); + // Bridge must NOT be touched — silent fallback regressions + // would otherwise let the malformed input hit `spawnOrAttach`. + expect(bridge.calls).toHaveLength(0); + } + }); + + it('400 when cwd is the empty string', async () => { + // Empty string is technically a string so the type-check above + // lets it through; `path.isAbsolute('')` is false so the + // "must be an absolute path when provided" branch catches it. + // Important: the `'cwd' in body` presence test means an empty + // string is NOT treated as omitted (which would fall back to + // boundWorkspace) — empty-string is the strongest "client + // explicitly passed nothing useful" signal we have. + const bridge = fakeBridge(); + const app = createServeApp( + { ...baseOpts, workspace: WS_BOUND }, + undefined, + { bridge }, + ); + const res = await request(app) + .post('/session') + .set('Host', `127.0.0.1:${baseOpts.port}`) + .send({ cwd: '' }); + expect(res.status).toBe(400); + expect(bridge.calls).toHaveLength(0); + }); + it('400 workspace_mismatch when bridge rejects cross-workspace cwd (#3803 §02)', async () => { // Single-workspace mode: bridge throws WorkspaceMismatchError // when the route forwards a non-bound cwd. Route translates diff --git a/packages/cli/src/serve/server.ts b/packages/cli/src/serve/server.ts index 64bf5b28c12..08b9726f1a6 100644 --- a/packages/cli/src/serve/server.ts +++ b/packages/cli/src/serve/server.ts @@ -174,13 +174,29 @@ export function createServeApp( app.post('/session', async (req, res) => { const body = safeBody(req); - // #3803 §02: 1 daemon = 1 workspace. `cwd` may be omitted — the - // route falls back to the daemon's bound workspace. A non-empty - // `cwd` that doesn't match the bound path is rejected downstream - // by the bridge with WorkspaceMismatchError (translated to 400 - // `workspace_mismatch` in `sendBridgeError`). - let cwd = typeof body['cwd'] === 'string' ? (body['cwd'] as string) : ''; - if (!cwd) cwd = boundWorkspace; + // #3803 §02: 1 daemon = 1 workspace. Three input shapes: + // - `cwd` ABSENT from body → fall back to the daemon's bound + // workspace (the §02 documented shape — clients pre-flight + // `caps.workspaceCwd` and may then omit `cwd`). + // - `cwd` PRESENT but not a string → 400 malformed. A + // client/orchestrator serialization bug (`cwd: null`, + // `cwd: 123`, `cwd: {}`) must not silently bind a session + // to the daemon's workspace; surface the bug instead. + // - `cwd` PRESENT as a string → fall through to the + // `path.isAbsolute` check (empty string and relative both + // fail there with "must be an absolute path when provided"). + // + // `safeBody` returns an `Object.create(null)` map, so + // `'cwd' in body` reflects exactly "did the client send the + // key?" without prototype-chain confusion. + const hasCwd = 'cwd' in body; + if (hasCwd && typeof body['cwd'] !== 'string') { + res + .status(400) + .json({ error: '`cwd` must be a string absolute path when provided' }); + return; + } + let cwd = hasCwd ? (body['cwd'] as string) : boundWorkspace; if (!path.isAbsolute(cwd)) { res .status(400) diff --git a/packages/cli/src/serve/types.ts b/packages/cli/src/serve/types.ts index 12bacdd5345..19ccdf4c3e9 100644 --- a/packages/cli/src/serve/types.ts +++ b/packages/cli/src/serve/types.ts @@ -105,8 +105,16 @@ export interface CapabilitiesEnvelope { * 400 workspace_mismatch from the bridge). * - Omit `cwd` on `POST /session` — the route falls back to this * path when the body has no `cwd` field. + * + * Optional at the type level (matches the SDK's `DaemonCapabilities` + * type) because the field is an additive extension of the v=1 + * envelope introduced by #3803 §02. Daemons predating §02 still + * announce `v: 1` and omit this field; the protocol's "bump v only + * on incompatible frame changes" stance (see `qwen-serve-protocol.md` + * "Additive to v=1" note) makes additive optionality the correct + * shape. The post-§02 server code here always populates it. */ - workspaceCwd: string; + workspaceCwd?: string; } export const CAPABILITIES_SCHEMA_VERSION = 1 as const; diff --git a/packages/sdk-typescript/src/daemon/DaemonClient.ts b/packages/sdk-typescript/src/daemon/DaemonClient.ts index d07051cee5b..c96ef245642 100644 --- a/packages/sdk-typescript/src/daemon/DaemonClient.ts +++ b/packages/sdk-typescript/src/daemon/DaemonClient.ts @@ -260,17 +260,24 @@ export class DaemonClient { req: CreateSessionRequest, ): Promise { // Per #3803 §02: omitting `cwd` lets the daemon fall back to its - // bound workspace. Send `cwd` ONLY when the caller passed - // `workspaceCwd` — sending `cwd: undefined` would JSON.stringify - // it out anyway, but conditional spread keeps the on-wire body - // shape obvious to anyone tracing the request. + // bound workspace. JSON.stringify strips `undefined` values, so + // `cwd: undefined` becomes "no `cwd` key" on the wire — and the + // server then takes the documented fallback path. + // + // Send EVERY defined `workspaceCwd` value through as-is, including + // the empty string. A truthy guard would silently swallow + // `workspaceCwd: ""` (a likely client-side bug) and let the server + // fall back instead of returning a clear 400 for the malformed + // input. The SDK should be a transparent layer here: passing the + // caller's value verbatim lets the server's validation surface + // bugs that would otherwise hide as "wrong workspace bound". return await this.fetchWithTimeout( `${this.baseUrl}/session`, { method: 'POST', headers: this.headers({ 'Content-Type': 'application/json' }), body: JSON.stringify({ - ...(req.workspaceCwd ? { cwd: req.workspaceCwd } : {}), + cwd: req.workspaceCwd, ...(req.modelServiceId ? { modelServiceId: req.modelServiceId } : {}), }), }, diff --git a/packages/sdk-typescript/test/unit/DaemonClient.test.ts b/packages/sdk-typescript/test/unit/DaemonClient.test.ts index 06f74621329..320a07dbc3e 100644 --- a/packages/sdk-typescript/test/unit/DaemonClient.test.ts +++ b/packages/sdk-typescript/test/unit/DaemonClient.test.ts @@ -153,11 +153,10 @@ describe('DaemonClient', () => { it('omits cwd when workspaceCwd is not provided (#3803 §02)', async () => { // Per #3803 §02 the daemon route falls back to its bound - // workspace when `cwd` is absent. The SDK must therefore NOT - // send `cwd: undefined` (or any other placeholder) when the - // caller omits `workspaceCwd` — otherwise the wire body - // carries an explicit empty field that wouldn't ever match - // the fallback semantics on a future stricter daemon. + // workspace when `cwd` is absent. The SDK relies on + // JSON.stringify stripping `undefined` values, so an + // omitted `workspaceCwd` ends up as "no `cwd` key" on the + // wire — exactly the fallback shape the server expects. const { fetch, calls } = recordingFetch(() => jsonResponse(200, { sessionId: 's-1', @@ -170,6 +169,22 @@ describe('DaemonClient', () => { expect(JSON.parse(calls[0]!.body!)).toEqual({}); }); + it('forwards empty-string workspaceCwd verbatim so the server can 400 it', async () => { + // `workspaceCwd: ""` is a likely client-side bug shape. A + // truthy-guard SDK would silently drop the field and let the + // server's fallback bind the session — masking the bug. We + // forward it verbatim so the server's + // `cwd must be an absolute path when provided` 400 surfaces. + const { fetch, calls } = recordingFetch(() => + jsonResponse(400, { error: 'bad cwd' }), + ); + const client = new DaemonClient({ baseUrl: 'http://daemon', fetch }); + await expect( + client.createOrAttachSession({ workspaceCwd: '' }), + ).rejects.toMatchObject({ status: 400 }); + expect(JSON.parse(calls[0]!.body!)).toEqual({ cwd: '' }); + }); + it('forwards modelServiceId when supplied', async () => { const { fetch, calls } = recordingFetch(() => jsonResponse(200, { From 1c7f5f069a8787d22a80c083fb00572687c7fa1e Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Thu, 14 May 2026 07:50:33 +0800 Subject: [PATCH 09/16] fix(serve): use const cwd in POST /session (prefer-const lint) CI lint failed with packages/cli/src/serve/server.ts:199:9 prefer-const: 'cwd' is never reassigned. The wave-4 rewrite split the original 'let cwd; if (!cwd) cwd = boundWorkspace' into a single ternary, which removes the only mutation path; the variable should be const accordingly. --- packages/cli/src/serve/server.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/cli/src/serve/server.ts b/packages/cli/src/serve/server.ts index 08b9726f1a6..09e657f51e0 100644 --- a/packages/cli/src/serve/server.ts +++ b/packages/cli/src/serve/server.ts @@ -196,7 +196,7 @@ export function createServeApp( .json({ error: '`cwd` must be a string absolute path when provided' }); return; } - let cwd = hasCwd ? (body['cwd'] as string) : boundWorkspace; + const cwd = hasCwd ? (body['cwd'] as string) : boundWorkspace; if (!path.isAbsolute(cwd)) { res .status(400) From 67a937a57cd7ffa15bda87a4d6fbcc8e0b0fa930 Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Thu, 14 May 2026 09:17:00 +0800 Subject: [PATCH 10/16] fix(serve): address third /review round (gpt-5.5 + glm-5.1 + deepseek-v4-pro) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five new inline findings; M1 was already resolved in 1c7f5f069. - M2 (httpAcpBridge.ts): drop the dead `ChannelInfo.workspaceCwd` field. Pre-§02 it was the routing key for `byWorkspaceChannel.get`; after the §02 collapse all reads target `SessionEntry.workspaceCwd` and `ChannelInfo.workspaceCwd` was only written, never read. Per- channel storage also suggests variance the "1 daemon = 1 workspace" model forbids. Removing the field encodes the single-workspace invariant in the type itself; left a stub comment so future readers don't reintroduce it. - M3 (httpAcpBridge.ts): fast-path `canonicalizeWorkspace` when `req.workspaceCwd === boundWorkspace`. The §02 recommended client flow is `caps.workspaceCwd` → POST `cwd: caps.workspaceCwd`, and the omit-cwd route in server.ts synthesizes the same equality. Both hit the equality check and skip the sync `realpathSync.native` syscall. Non-equal inputs fall through to the full canonicalize (clients sending `/work/./bound`, mixed casing on case-insensitive FS, symlink aliases) so correctness is unchanged. - M4 (httpAcpBridge.ts): operator stderr breadcrumb in the `channel.exited` handler. An agent crash (OOM / segfault) used to be silent on the daemon side — the child-stderr forwarder caught whatever the child wrote before dying (often nothing on SIGKILL/segfault), and SSE subscribers saw `session_died` frames but operators reading `qwen serve`'s own output had no signal that the agent process was gone. Log code+signal+affected-session-count so the line is the canonical "agent disappeared" indicator. - M5 (server.ts): documentation-only. The reviewer wanted `createServeApp` to validate `opts.workspace` exists + is a directory (currently only `runQwenServe` does). Trade-off: doing that breaks 4 existing tests which pass synthetic `/work/bound` on purpose to exercise route-layer behavior without a real directory. Deferred the helper extraction; added a JSDoc note pinning the contract so future entry points binding `createServeApp` to user input know to replicate the validation. - M6 (runQwenServe.ts): pass the already-canonical `boundWorkspace` into `createServeApp` via `opts.workspace`. `canonicalizeWorkspace` is idempotent so the server-side recanonicalize is a no-op today, but if a future refactor ever makes it non-idempotent the values the route advertises on `/capabilities` and the bridge enforces would diverge — landing clients in a "/capabilities says X, POST /session/X returns workspace_mismatch" contradiction. Removes the drift risk. bridge: 73/73; server: 80/80; tsc clean for PR-touched files. --- packages/cli/src/serve/httpAcpBridge.ts | 36 ++++++++++++++++++++++--- packages/cli/src/serve/runQwenServe.ts | 14 +++++++++- packages/cli/src/serve/server.ts | 16 +++++++++++ 3 files changed, 62 insertions(+), 4 deletions(-) diff --git a/packages/cli/src/serve/httpAcpBridge.ts b/packages/cli/src/serve/httpAcpBridge.ts index 1d5bb20da54..6b8f98470b7 100644 --- a/packages/cli/src/serve/httpAcpBridge.ts +++ b/packages/cli/src/serve/httpAcpBridge.ts @@ -408,7 +408,13 @@ interface ChannelInfo { connection: ClientSideConnection; /** Shared BridgeClient — its methods route ACP params by sessionId. */ client: BridgeClient; - workspaceCwd: string; + // Note: pre-§02 a `workspaceCwd: string` field lived here so the + // `byWorkspaceChannel.get(entry.workspaceCwd)` lookup could route + // multi-workspace requests. Under "1 daemon = 1 workspace" the + // module-scope `boundWorkspace` is the single source of truth and + // every channel inherits it. Per-channel storage would suggest + // variance the model doesn't allow; dropping it makes the + // single-workspace invariant visible at the type level. /** * Live session ids multiplexed on this channel. Updated when * `doSpawn` registers a new session and when `killSession` / @@ -1173,7 +1179,6 @@ export function createHttpAcpBridge(opts: BridgeOptions): HttpAcpBridge { channel, connection, client, - workspaceCwd: boundWorkspace, sessionIds: new Set(), isDying: false, }; @@ -1200,6 +1205,19 @@ export function createHttpAcpBridge(opts: BridgeOptions): HttpAcpBridge { if (channelInfo === info) channelInfo = undefined; const sessions = Array.from(info.sessionIds); info.sessionIds.clear(); + // Operator breadcrumb on the daemon side. Without it, an + // agent crash (OOM / segfault) is invisible from the daemon + // log: each affected SSE subscriber sees a `session_died` + // frame and disconnects, the daemon's child-stderr forwarder + // emits whatever the child wrote before dying (often + // nothing on a SIGKILL / segfault), and operators can't tell + // from `qwen serve`'s own output that the agent process is + // gone. Log the OS-level signal/exitCode + how many sessions + // were affected so the line is the canonical "agent + // disappeared" indicator. + writeStderrLine( + `qwen serve: channel exited (code=${exitInfo?.exitCode ?? 'none'}, signal=${exitInfo?.signalCode ?? 'none'}, ${sessions.length} session(s) torn down)`, + ); for (const sid of sessions) { const sessEntry = byId.get(sid); if (!sessEntry) continue; @@ -1497,7 +1515,19 @@ export function createHttpAcpBridge(opts: BridgeOptions): HttpAcpBridge { `workspaceCwd must be an absolute path; got "${req.workspaceCwd}"`, ); } - const workspaceKey = canonicalizeWorkspace(req.workspaceCwd); + // Fast-path the common §02 case: clients pre-flight `caps.workspaceCwd` + // and post back the exact same string, so the equality check + // saves a `realpathSync.native` syscall per spawnOrAttach. The + // omit-cwd path in `server.ts` also synthesizes `cwd = + // boundWorkspace` before calling here, so it hits this branch + // too. Falls through to the full canonicalize when the client + // sent a non-canonical alias (`/work/./bound`, mixed casing on + // case-insensitive FS, a symlinked aliased path, …) — that + // still needs the realpath to compare correctly. + const workspaceKey = + req.workspaceCwd === boundWorkspace + ? boundWorkspace + : canonicalizeWorkspace(req.workspaceCwd); // #3803 §02: reject cross-workspace requests at the daemon // boundary. The route layer catches `WorkspaceMismatchError` // and translates to 400 with `code: 'workspace_mismatch'`. diff --git a/packages/cli/src/serve/runQwenServe.ts b/packages/cli/src/serve/runQwenServe.ts index fc3e7ed54e3..f09ec5e8459 100644 --- a/packages/cli/src/serve/runQwenServe.ts +++ b/packages/cli/src/serve/runQwenServe.ts @@ -171,7 +171,19 @@ export async function runQwenServe( boundWorkspace, }); let actualPort = opts.port; - const app = createServeApp(opts, () => actualPort, { bridge }); + // Pass the already-canonical `boundWorkspace` into `createServeApp` + // via `opts.workspace`. `canonicalizeWorkspace` is idempotent so + // server.ts's own canonicalize is a no-op, but if some future + // refactor ever makes it non-idempotent (caching, async I/O, + // etc.) the value the route advertises on `/capabilities` and the + // value the bridge enforces would diverge — landing clients in a + // "/capabilities says X, POST /session/X 400s with workspace_mismatch" + // contradiction. Pre-canonicalizing here removes that drift risk. + const app = createServeApp( + { ...opts, workspace: boundWorkspace }, + () => actualPort, + { bridge }, + ); // Node's `app.listen()` wants the unbracketed IPv6 literal (`::1`) but // operators conventionally type `[::1]` (or copy/paste from URLs that diff --git a/packages/cli/src/serve/server.ts b/packages/cli/src/serve/server.ts index 09e657f51e0..1357f72d1c5 100644 --- a/packages/cli/src/serve/server.ts +++ b/packages/cli/src/serve/server.ts @@ -51,6 +51,22 @@ export interface ServeAppDeps { * - `POST /session/:id/model` * - `GET /session/:id/events` (SSE) * - `POST /permission/:requestId` + * + * **Workspace validation contract.** `createServeApp` itself does NOT + * verify that `opts.workspace` exists or is a directory — it + * canonicalizes via `canonicalizeWorkspace`, which falls back to + * `path.resolve` on ENOENT so the app boots even against a missing + * path. `runQwenServe` is the production entry point and DOES + * perform the `fs.statSync` + `isDirectory()` boot-loud check before + * calling this function. Tests inject synthetic paths (`/work/bound` + * etc.) on purpose: they want to exercise the route layer's + * canonicalization and `workspace_mismatch` translation without + * needing a real directory on disk. If a future entry point binds + * `createServeApp` directly to user input, it MUST replicate the + * `runQwenServe` validation (or call into a shared helper if one is + * extracted) — otherwise a non-existent `--workspace` would boot + * a "healthy"-looking daemon whose every spawn fails with cryptic + * child-process ENOENT. */ export function createServeApp( opts: ServeOptions, From 19650b51d4782edc3af092c3d1161a6325c5d709 Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Thu, 14 May 2026 16:47:22 +0800 Subject: [PATCH 11/16] fix(serve,sdk): address fourth /review round (deepseek-v4-pro x2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two new inline findings: - O1 (server.ts): the POST /session route uses `'cwd' in body` against `safeBody`'s `Object.create(null)` output to distinguish "client omitted cwd" from "client sent cwd". The semantics quietly couple to `safeBody`'s literal strip list (`__proto__/constructor/prototype`). If a future maintainer adds a user-facing key (e.g. `cwd`) to that strip list, the route's presence-check would silently flip to "absent → fallback", masking the bug as "wrong workspace bound." Extracted `PROTOTYPE_POLLUTION_KEYS: ReadonlySet` as a named module-scope constant; safeBody uses `.has()` on it (behavior unchanged); the route's comment now cross-references the const so the coupling is documented at both ends. The const's JSDoc spells out what to do if the strip set ever has to grow into user-key territory. - O2 (sdk-typescript): `DaemonCapabilities.workspaceCwd` is `string | undefined` (additive to v=1; pre-§02 daemons omit). SDK consumers that pass it into a `string` context get a TS strict error or, against an old daemon, a runtime `Cannot read properties of undefined`. Added a `requireWorkspaceCwd` helper + `DaemonCapabilityMissingError` so consumers can opt into an actionable `DaemonCapabilities.workspaceCwd is missing — introduced in #3803 §02 …` error instead. Exported both from `@qwen-code/sdk`'s top-level module + the `daemon/` sub-module. Unit tests cover populated, missing, and empty-string inputs. bridge: 73/73; server: 80/80; SDK DaemonClient: 43/43 (was 40, +3 new requireWorkspaceCwd cases). tsc clean for SDK and PR-touched CLI files. --- packages/cli/src/serve/server.ts | 44 ++++++++++---- packages/sdk-typescript/src/daemon/index.ts | 4 ++ packages/sdk-typescript/src/daemon/types.ts | 52 +++++++++++++++++ packages/sdk-typescript/src/index.ts | 2 + .../test/unit/DaemonClient.test.ts | 58 +++++++++++++++++++ 5 files changed, 149 insertions(+), 11 deletions(-) diff --git a/packages/cli/src/serve/server.ts b/packages/cli/src/serve/server.ts index 1357f72d1c5..1cc926bc031 100644 --- a/packages/cli/src/serve/server.ts +++ b/packages/cli/src/serve/server.ts @@ -204,7 +204,10 @@ export function createServeApp( // // `safeBody` returns an `Object.create(null)` map, so // `'cwd' in body` reflects exactly "did the client send the - // key?" without prototype-chain confusion. + // key?" without prototype-chain confusion. The presence-check + // is safe as long as `PROTOTYPE_POLLUTION_KEYS` doesn't grow to + // include `cwd` — see the cross-reference in the const's JSDoc + // for what to do if that invariant ever has to break. const hasCwd = 'cwd' in body; if (hasCwd && typeof body['cwd'] !== 'string') { res @@ -744,19 +747,40 @@ export function createServeApp( return app; } +/** + * Keys stripped by `safeBody` to defend against prototype-pollution + * — see BZ9uv/va/vs/wD/Bd1zz. Routes downstream of `safeBody` spread + * the filtered result into objects passed to the bridge / ACP SDK; + * without this scrub a client could set + * `{"__proto__": {"polluted": true}}` and pollute + * `Object.prototype` via downstream spreads. + * + * **Cross-reference for route maintainers:** the POST `/session` + * route distinguishes "absent" from "present" via `'cwd' in body` + * against `safeBody`'s output. The semantics rely on this set NOT + * overlapping with user-payload keys. If you ever add a key here + * that a route's presence-check cares about (highly unlikely — this + * set is the JS prototype-attack triple, plus a route would have + * to deliberately name a property after one of these), the + * presence-check needs to move to the pre-`safeBody` `req.body` + * (with its own pollution guard) or `safeBody` needs to return a + * separate "raw-keys" set alongside the filtered object. + */ +const PROTOTYPE_POLLUTION_KEYS: ReadonlySet = new Set([ + '__proto__', + 'constructor', + 'prototype', +]); + /** * Coerce `req.body` into a safe `Record` for route * handlers. Replaces the 5-site copy-pasted preamble * `typeof req.body === 'object' && req.body !== null ? ... : {}` * (Bd10m). * - * Also strips prototype-pollution keys (`__proto__`, `constructor`, - * `prototype`) before returning — see BZ9uv/va/vs/wD/Bd1zz. Routes - * downstream of this helper spread the result into objects passed to - * the bridge / ACP SDK; without this scrub, a client could set - * `{"__proto__": {"polluted": true}}` and pollute `Object.prototype`. - * Uses an `Object.create(null)` target so the returned object itself - * has no prototype either, blocking second-order spread-into-default- + * Strips the `PROTOTYPE_POLLUTION_KEYS` set before returning. Uses an + * `Object.create(null)` target so the returned object itself has no + * prototype either, blocking second-order spread-into-default- * prototype attacks. */ function safeBody(req: import('express').Request): Record { @@ -766,9 +790,7 @@ function safeBody(req: import('express').Request): Record { } const out = Object.create(null) as Record; for (const [key, value] of Object.entries(raw as Record)) { - if (key === '__proto__' || key === 'constructor' || key === 'prototype') { - continue; - } + if (PROTOTYPE_POLLUTION_KEYS.has(key)) continue; out[key] = value; } return out; diff --git a/packages/sdk-typescript/src/daemon/index.ts b/packages/sdk-typescript/src/daemon/index.ts index c463fed9280..68dab9fd332 100644 --- a/packages/sdk-typescript/src/daemon/index.ts +++ b/packages/sdk-typescript/src/daemon/index.ts @@ -13,6 +13,10 @@ export { type SubscribeOptions, } from './DaemonClient.js'; export { parseSseStream, SseFramingError } from './sse.js'; +export { + DaemonCapabilityMissingError, + requireWorkspaceCwd, +} from './types.js'; export type { DaemonCapabilities, DaemonEvent, diff --git a/packages/sdk-typescript/src/daemon/types.ts b/packages/sdk-typescript/src/daemon/types.ts index 865aa0b5be1..d5ce430d3d0 100644 --- a/packages/sdk-typescript/src/daemon/types.ts +++ b/packages/sdk-typescript/src/daemon/types.ts @@ -41,10 +41,62 @@ export interface DaemonCapabilities { * protocol's "bump v only on incompatible frame changes" stance * (see `qwen-serve-protocol.md`) makes additive optionality the * correct shape. All post-§02 daemons populate it. + * + * **SDK consumers**: if you need the value as a non-undefined + * `string` (e.g. to call `.startsWith()` or pass into a function + * typed `string`), use the `requireWorkspaceCwd` helper from this + * module — it throws `DaemonCapabilityMissingError` with an + * actionable "this daemon predates §02" message instead of + * letting the call site hit a cryptic + * "Cannot read properties of undefined". */ workspaceCwd?: string; } +/** + * Thrown by `requireWorkspaceCwd` (and any future + * `requireCapability` helpers) when the daemon's + * `/capabilities` envelope is missing a field the caller needs. + * Carries the field name so handlers can branch on it. + */ +export class DaemonCapabilityMissingError extends Error { + readonly capability: string; + constructor(capability: string, hint: string) { + super( + `DaemonCapabilities.${capability} is missing — ${hint}. The daemon ` + + `you are connected to likely predates the feature that added ` + + `this field; upgrade the daemon or fall back to a different ` + + `code path that doesn't require it.`, + ); + this.name = 'DaemonCapabilityMissingError'; + this.capability = capability; + } +} + +/** + * Assert that `caps.workspaceCwd` is populated (i.e. the daemon was + * built post-§02) and return it as a non-undefined `string`. Throws + * `DaemonCapabilityMissingError` otherwise so the call site gets an + * actionable error rather than a downstream + * `Cannot read properties of undefined`. + * + * Use this when you need the value as a guaranteed `string` — + * e.g. to render in UI, log, compare with `.startsWith()`, or pass + * into a function typed `string`. If your code is fine with the + * value being absent (e.g. you fall back to `POST /session` without + * `workspaceCwd` and let the daemon choose), just read + * `caps.workspaceCwd` directly. + */ +export function requireWorkspaceCwd(caps: DaemonCapabilities): string { + if (typeof caps.workspaceCwd !== 'string' || caps.workspaceCwd.length === 0) { + throw new DaemonCapabilityMissingError( + 'workspaceCwd', + 'introduced in #3803 §02 (1 daemon = 1 workspace)', + ); + } + return caps.workspaceCwd; +} + /** Returned from `POST /session`. */ export interface DaemonSession { sessionId: string; diff --git a/packages/sdk-typescript/src/index.ts b/packages/sdk-typescript/src/index.ts index 64d3214f108..2dc5a2e8fea 100644 --- a/packages/sdk-typescript/src/index.ts +++ b/packages/sdk-typescript/src/index.ts @@ -5,9 +5,11 @@ export { SdkLogger } from './utils/logger.js'; // Daemon HTTP client (talks to `qwen serve`; see GitHub issue #3803) export { + DaemonCapabilityMissingError, DaemonClient, DaemonHttpError, parseSseStream, + requireWorkspaceCwd, SseFramingError, type CreateSessionRequest, type DaemonCapabilities, diff --git a/packages/sdk-typescript/test/unit/DaemonClient.test.ts b/packages/sdk-typescript/test/unit/DaemonClient.test.ts index 320a07dbc3e..e39c93d1ba3 100644 --- a/packages/sdk-typescript/test/unit/DaemonClient.test.ts +++ b/packages/sdk-typescript/test/unit/DaemonClient.test.ts @@ -11,6 +11,11 @@ import { abortTimeout, composeAbortSignals, } from '../../src/daemon/DaemonClient.js'; +import { + DaemonCapabilityMissingError, + requireWorkspaceCwd, +} from '../../src/daemon/types.js'; +import type { DaemonCapabilities } from '../../src/daemon/types.js'; function jsonResponse(status: number, body: unknown): Response { return new Response(JSON.stringify(body), { @@ -673,4 +678,57 @@ describe('DaemonClient', () => { expect(elapsed).toBeLessThan(2000); }); }); + + describe('requireWorkspaceCwd', () => { + // Helper: build a `DaemonCapabilities`-shaped envelope without + // having to spell out the unrelated fields on every call. + const caps = (overrides: Partial): DaemonCapabilities => + ({ + v: 1, + mode: 'http-bridge', + features: [], + modelServices: [], + ...overrides, + }) as DaemonCapabilities; + + it('returns the workspaceCwd when populated', () => { + expect(requireWorkspaceCwd(caps({ workspaceCwd: '/work/bound' }))).toBe( + '/work/bound', + ); + }); + + it('throws DaemonCapabilityMissingError when the field is undefined (pre-§02 daemon)', () => { + // Pre-§02 daemons emit v=1 envelopes without `workspaceCwd`. + // The helper exists so SDK consumers get an actionable error + // instead of a downstream `Cannot read properties of undefined`. + expect(() => requireWorkspaceCwd(caps({}))).toThrow( + DaemonCapabilityMissingError, + ); + const err = (() => { + try { + requireWorkspaceCwd(caps({})); + return null; + } catch (e) { + return e; + } + })(); + expect(err).toBeInstanceOf(DaemonCapabilityMissingError); + expect((err as DaemonCapabilityMissingError).capability).toBe( + 'workspaceCwd', + ); + expect((err as DaemonCapabilityMissingError).message).toMatch( + /predates the feature|workspaceCwd/, + ); + }); + + it('treats empty-string as missing (defensive)', () => { + // A daemon that erroneously sends `workspaceCwd: ""` would + // otherwise satisfy `typeof === 'string'` while still being + // useless to consumers. Treat it like a missing field so the + // call site lands in the same error branch. + expect(() => requireWorkspaceCwd(caps({ workspaceCwd: '' }))).toThrow( + DaemonCapabilityMissingError, + ); + }); + }); }); From 3e544f384ffc273490eafeffcd9dafc30c8612db Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Thu, 14 May 2026 20:31:18 +0800 Subject: [PATCH 12/16] fix(serve): address tanzhenxin REQUEST_CHANGES (cold-spawn + streaming-test bind) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings from the CHANGES_REQUESTED review on PR #4113. - T1 (integration-tests/cli/qwen-serve-streaming.test.ts) — high severity: the daemon spawn in `beforeAll` did not pass `--workspace REPO_ROOT`, so under §02 the daemon bound to whatever cwd the test runner was invoked from. Every later `createOrAttachSession({ workspaceCwd: REPO_ROOT })` then 400'd with `workspace_mismatch`, and the entire file — child-crash recovery, multi-client first-responder permission, Last-Event-ID resume — silently no-op'd once `SKIP_LLM_TESTS` was unset. The sibling `qwen-serve-routes.test.ts` got the same fix earlier in this PR; this file was missed in that pass. Added the flag with a comment pointing at the rationale so the omission can't recur. - T2 (packages/cli/src/serve/httpAcpBridge.ts) — medium severity: cold-spawn window orphans the agent child on double-Ctrl+C. The `qwen --acp` child exists from the moment `channelFactory` spawns it, but pre-fix the bridge only added the channel to `aliveChannels` AFTER `connection.initialize()` returned. During the up-to-`initTimeoutMs` (default 10s) handshake window `aliveChannels` was empty, and a double-Ctrl+C in that window played out as: first SIGINT entered `shutdown()` and awaited the in-flight spawn; second SIGINT called `killAllSync()` against an empty set; `process.exit(1)` orphaned the child. Same class of bug the BkUyD invariant set out to close — the post-init overwrite race was covered, the pre-init handshake window wasn't. Fix: move `info` creation + `aliveChannels.add(info)` + the `channel.exited` handler registration BEFORE the `initialize` await. Init-failure / late-shutdown / child-crash-during-handshake all converge on the same cleanup path: mark `isDying = true`, `await channel.kill()`, let the exited handler `aliveChannels .delete(info)` once the OS reaps the process. `channelInfo` (the attach target) is still assigned LAST so `ensureChannel`'s fast-path never returns a still-handshaking channel. Regression test: `killAllSync force-kills the channel during the initialize handshake` uses a bespoke factory whose agent's `initialize` never resolves and asserts `killAllSync` fires killSync against the channel during the handshake window. Pre-fix the test would observe an empty `killSyncCalls` array. bridge: 74/74 (was 73, +1 cold-spawn test); server: 80/80; tsc clean for PR-touched files. --- .../cli/qwen-serve-streaming.test.ts | 11 ++ packages/cli/src/serve/httpAcpBridge.test.ts | 125 ++++++++++++++++++ packages/cli/src/serve/httpAcpBridge.ts | 92 +++++++++---- 3 files changed, 203 insertions(+), 25 deletions(-) diff --git a/integration-tests/cli/qwen-serve-streaming.test.ts b/integration-tests/cli/qwen-serve-streaming.test.ts index 08369bc2309..afc25f6d9dc 100644 --- a/integration-tests/cli/qwen-serve-streaming.test.ts +++ b/integration-tests/cli/qwen-serve-streaming.test.ts @@ -71,6 +71,17 @@ beforeAll(async () => { TOKEN, '--hostname', '127.0.0.1', + // Per #3803 §02 (1 daemon = 1 workspace), pin the bound + // workspace so every `createOrAttachSession({ workspaceCwd: + // REPO_ROOT })` below matches. Without this the daemon inherits + // the test runner's cwd (CI / IDE-launcher / direct vitest + // invocations all differ) and every session create returns + // 400 workspace_mismatch — the SSE / permission / Last-Event-ID + // tests below would all silently 404 once `SKIP_LLM_TESTS` is + // unset. Same fix the sibling routes test received earlier in + // this PR — missed in this file in the original §02 pass. + '--workspace', + REPO_ROOT, ], { stdio: ['ignore', 'pipe', 'pipe'] }, ); diff --git a/packages/cli/src/serve/httpAcpBridge.test.ts b/packages/cli/src/serve/httpAcpBridge.test.ts index ac6161d350f..48b72a0d924 100644 --- a/packages/cli/src/serve/httpAcpBridge.test.ts +++ b/packages/cli/src/serve/httpAcpBridge.test.ts @@ -487,6 +487,131 @@ describe('createHttpAcpBridge', () => { void shutdownPromise; }); + it('killAllSync force-kills the channel during the initialize handshake (tanzhenxin cold-spawn-window)', async () => { + // tanzhenxin cold-spawn-window finding: the agent child exists + // from the moment `channelFactory(boundWorkspace)` returns, but + // pre-fix `aliveChannels.add(info)` ran only AFTER the + // `initialize` handshake completed (up to `initTimeoutMs`, + // default 10s). A double-Ctrl+C in that handshake window played + // out as: first SIGINT entered `shutdown()` and awaited the + // in-flight spawn; second SIGINT called `killAllSync()` against + // an empty `aliveChannels` (the channel hadn't been added yet) + // and `process.exit(1)` orphaned the child. The fix moves the + // add + the `channel.exited` handler registration BEFORE the + // `initialize` await; this test pins that the channel is + // reachable via `killAllSync` during the handshake. + const killSyncCalls: string[] = []; + const factory: ChannelFactory = async () => { + // Bespoke agent whose `initialize` never resolves — that's the + // handshake-hanging window the finding is about. A real agent + // can spend up to `initTimeoutMs` ms here before the bridge's + // `withTimeout` aborts it. + const ab = new TransformStream(); + const ba = new TransformStream(); + const clientStream = ndJsonStream(ab.writable, ba.readable); + const agentStream = ndJsonStream(ba.writable, ab.readable); + let resolveExited: + | (( + info?: + | { + exitCode: number | null; + signalCode: NodeJS.Signals | null; + } + | undefined, + ) => void) + | undefined; + const exited = new Promise< + | { exitCode: number | null; signalCode: NodeJS.Signals | null } + | undefined + >((r) => { + resolveExited = r; + }); + const stuckAgent: Agent = { + async initialize() { + // Hang forever — the bridge's `withTimeout` would normally + // bound this, but the test asserts behavior DURING the + // handshake, so we let it sit until killAllSync resolves + // `exited` and tears the channel down externally. + return new Promise(() => {}); + }, + async newSession() { + throw new Error('newSession should not be reached'); + }, + async loadSession() { + throw new Error('loadSession should not be reached'); + }, + async authenticate() { + throw new Error('authenticate should not be reached'); + }, + async prompt() { + throw new Error('prompt should not be reached'); + }, + async cancel() { + /* no-op */ + }, + async setSessionMode() { + throw new Error('setSessionMode should not be reached'); + }, + async setSessionConfigOption() { + throw new Error('setSessionConfigOption should not be reached'); + }, + }; + new AgentSideConnection(() => stuckAgent, agentStream); + return { + stream: clientStream, + exited, + kill: async () => { + resolveExited!(undefined); + }, + killSync: () => { + killSyncCalls.push('called'); + resolveExited!(undefined); + }, + }; + }; + const bridge = makeBridge({ + channelFactory: factory, + // Bump initializeTimeoutMs so it doesn't race with the + // killAllSync we fire below. We're NOT testing the timeout + // path — we're testing the cold-spawn window before it. + initializeTimeoutMs: 30_000, + }); + + // Kick off a spawn — `initialize` hangs forever in this fake, + // so the spawn promise never resolves naturally. Don't await + // (would block the test); `.catch` keeps the rejection from + // being unhandled when killAllSync eventually tears things down. + const spawnPromise = bridge + .spawnOrAttach({ workspaceCwd: WS_A }) + .catch(() => undefined); + + // Yield enough microtasks for `channelFactory` to return AND the + // bridge's `info` creation + `aliveChannels.add(info)` + the + // `channel.exited` handler registration to all run BEFORE the + // bridge enters `await initialize`. Pre-fix the alive-set add + // sat AFTER initialize, so any number of yields here would still + // find an empty set when killAllSync fires below. + for (let i = 0; i < 5; i++) { + await new Promise((r) => setImmediate(r)); + } + + // Operator double-Ctrl+C arrives during the handshake window. + bridge.killAllSync(); + + // Post-fix expectation: channel was added to `aliveChannels` + // BEFORE the `initialize` await, so killAllSync iterates a set + // containing it and fires killSync. Pre-fix this array would + // have been empty — and `process.exit(1)` after this would have + // orphaned the agent child. + expect(killSyncCalls).toEqual(['called']); + + // Cleanup: spawnPromise resolves on its own once killSync's + // `resolveExited` propagates through the bridge's + // `channel.exited` handler and the IIFE's catch reaps the half- + // initialized channel. + void spawnPromise; + }); + it('killSession marks the channel dying so concurrent spawnOrAttach gets a fresh channel', async () => { // After the last session is killed, `channel.kill()` runs through // its SIGTERM grace window before SIGKILL — up to 10s in the real diff --git a/packages/cli/src/serve/httpAcpBridge.ts b/packages/cli/src/serve/httpAcpBridge.ts index 6b8f98470b7..265182edda5 100644 --- a/packages/cli/src/serve/httpAcpBridge.ts +++ b/packages/cli/src/serve/httpAcpBridge.ts @@ -1151,30 +1151,21 @@ export function createHttpAcpBridge(opts: BridgeOptions): HttpAcpBridge { ); const connection = new ClientSideConnection(() => client, channel.stream); - try { - await withTimeout( - connection.initialize({ - protocolVersion: PROTOCOL_VERSION, - clientCapabilities: { - fs: { readTextFile: true, writeTextFile: true }, - }, - clientInfo: { name: 'qwen-serve-bridge', version: '0' }, - }), - initTimeoutMs, - 'initialize', - ); - } catch (err) { - await channel.kill().catch(() => {}); - throw err; - } - - // Late-shutdown re-check: if shutdown flipped during `initialize`, - // tear this channel down rather than leak past `process.exit(0)`. - if (shuttingDown) { - await channel.kill().catch(() => {}); - throw new Error('HttpAcpBridge is shutting down'); - } - + // tanzhenxin cold-spawn-window finding: the agent child exists + // from the moment `channelFactory(boundWorkspace)` returns, but + // pre-fix `aliveChannels.add(info)` ran only AFTER the + // `initialize` handshake completed (up to `initTimeoutMs`, default + // 10s). A double-Ctrl+C in that handshake window played out as: + // first SIGINT entered `shutdown()` and awaited the in-flight + // spawn; second SIGINT called `killAllSync()` against an empty + // `aliveChannels` (the channel hadn't been added yet) and + // `process.exit(1)` orphaned the child. Add to `aliveChannels` + // BEFORE the handshake await, and register the `channel.exited` + // handler immediately afterward so init-failure / child-crash / + // late-shutdown all clean up through the standard exit path + // instead of needing bespoke catches. `channelInfo` (the attach + // target) stays set only AFTER initialize succeeds — see further + // down — so callers don't attach to a still-handshaking channel. const info: ChannelInfo = { channel, connection, @@ -1182,7 +1173,6 @@ export function createHttpAcpBridge(opts: BridgeOptions): HttpAcpBridge { sessionIds: new Set(), isDying: false, }; - channelInfo = info; aliveChannels.add(info); // One-time channel.exited cleanup. The child dying takes ALL @@ -1191,6 +1181,14 @@ export function createHttpAcpBridge(opts: BridgeOptions): HttpAcpBridge { // iteration), publish `session_died` on each session's bus, // remove from byId / defaultEntry / pending tables. // + // Registered BEFORE the `initialize` await (tanzhenxin + // cold-spawn-window fix above) so init-failure / child-crash / + // late-shutdown all converge here. During handshake + // `sessionIds` is empty — the loop below no-ops, the stderr + // line still fires to tell operators "agent process gone + // during init", and `aliveChannels.delete(info)` clears the + // entry through the normal exit path. + // // tanzhenxin BkUyD: drop from `aliveChannels` ONLY when the OS // process is actually gone. Async kill paths (`killSession` // reap, `shutdown()` await, `doSpawn`'s newSession-failure @@ -1242,6 +1240,50 @@ export function createHttpAcpBridge(opts: BridgeOptions): HttpAcpBridge { } }); + // Initialize handshake. The channel is already in + // `aliveChannels` and the `channel.exited` handler above is + // registered, so failure paths (init throw, timeout, late + // shutdown) only need to mark dying + kill — the handler does + // the alive-set cleanup when the OS reaps the child. + try { + await withTimeout( + connection.initialize({ + protocolVersion: PROTOCOL_VERSION, + clientCapabilities: { + fs: { readTextFile: true, writeTextFile: true }, + }, + clientInfo: { name: 'qwen-serve-bridge', version: '0' }, + }), + initTimeoutMs, + 'initialize', + ); + } catch (err) { + // Mark dying so concurrent `ensureChannel` callers spawn + // fresh instead of awaiting this in-flight promise's + // `channelInfo` (which we never assigned). The kill below + // triggers `channel.exited` → handler runs `aliveChannels + // .delete(info)` → entry leaves the alive set after OS reap. + info.isDying = true; + await channel.kill().catch(() => {}); + throw err; + } + + // Late-shutdown re-check: if shutdown flipped during the + // handshake, tear this channel down rather than leak past + // `process.exit(0)`. Same cleanup pattern as the init-failure + // path: mark dying + kill, let the exited handler reap. + if (shuttingDown) { + info.isDying = true; + await channel.kill().catch(() => {}); + throw new Error('HttpAcpBridge is shutting down'); + } + + // Handshake succeeded — now publish the channel as the + // attach-available slot. `channelInfo` is assigned LAST so + // `ensureChannel`'s fast-path (`if (channelInfo && !.isDying)`) + // never returns a still-handshaking channel to a concurrent + // caller. + channelInfo = info; return info; })(); From b34a28bfca837a32f87fe79982739de1224dca6d Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Fri, 15 May 2026 00:46:08 +0800 Subject: [PATCH 13/16] fix(serve): address third /review round (gpt-5.5 + glm-5.1 + deepseek-v4-pro) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Eight new inline findings; six applied, two deferred-with-reply. - P1 (httpAcpBridge.ts init-failure isDying comment): my comment overstated what `info.isDying` accomplishes on the init-failure path — concurrent `ensureChannel()` callers don't bypass via `isDying`, they coalesce on `inFlightChannelSpawn` and observe the same rejection. Reworded to describe the actual cross-path invariant marker. - P2 (server.ts workspace_mismatch log injection): doudouOUC flagged log injection via `err.requested` (user-controlled). `path.resolve` + `realpathSync.native` preserve control chars in path segments, so a body `{"cwd": "/legit/path\nqwen serve: FAKE LOG"}` would emit two valid-looking daemon log lines on stderr — weaponizing line-based log shippers (Splunk / Loki / journald → SIEM). `JSON.stringify` both `err.bound` and `err.requested` in the log line escapes control chars + quotes the values, making any injection attempt visible-as-quoted-noise rather than forged-line. Bound is operator-controlled and inherently safe but quoted symmetrically for readability. The defense-in-depth alternative (reject control chars in canonicalizeWorkspace) is deferred — this single log site was the actionable interpolation; future workspace-path-into-stderr / -JSON / -templated-SQL flows can pick up the rejection if they ship. - P3 (httpAcpBridge.test.ts): refactor the cross-workspace WorkspaceMismatchError test to a single `.catch((e) => e)` capture rather than firing the rejection twice (once for the `rejects .toBeInstanceOf` matcher, once for the field assertions). Logic unchanged. - P4 (httpAcpBridge.ts channel.exited log): the `qwen serve: channel exited (...)` line fired on every channel exit including planned shutdown — alarming for operators who Ctrl+C'd a healthy daemon. Guarded with `if (!shuttingDown)` so the planned-shutdown case (operator already saw `received SIGINT, draining...`) stays silent. The killSession path (last session leaves, daemon stays up — no top-level context line) still logs, since the line is the only signal that the cleanup actually ran. - P5 (httpAcpBridge.ts): light trim of the "pre-fix" narrative voice in two comment blocks (cold-spawn ensureChannel layout + BkUyD killAllSync aliveChannels iteration). Kept the invariant explanations — those carry maintenance value — dropped the "pre-fix the code did X" framing that's review-context not future-reader context. - P6 (server.ts + runQwenServe.ts): `createServeApp` now accepts a pre-canonicalized `deps.boundWorkspace` to skip its own `canonicalizeWorkspace` syscall when the caller (runQwenServe) already did the work. Replaces my earlier `{...opts, workspace: boundWorkspace}` opts-mutation hack — cleaner separation of concerns + drops one `realpathSync.native` per boot. Direct callers (tests, embeds) that omit `deps.boundWorkspace` still get the in-body canonicalize path. - P8 (httpAcpBridge.ts): defensive `aliveChannels.size > 2` warning. The set is intentionally multi-entry to cover the killSession-then-spawnOrAttach overlap window (size 2 is legitimate). Anything higher implies a `channel.exited` handler never fired for a prior channel — a real leak we'd otherwise catch only as gradually-growing RSS. The warning surfaces it the moment it happens. - P7 (CreateSessionRequest.workspaceCwd optional): deferred with reply rationale. Making the field optional is the §02 design ("SDK accepts bound path or none"); the JSDoc already explains the omit-vs-explicit choice; Stage 1 has no shipping SDK consumers so there's no breakage to call out in a changelog file. No code change. bridge: 74/74 (cross-workspace test refactor + behavioral assertions unchanged); server: 80/80; SDK 43/43. tsc clean for PR-touched files. --- packages/cli/src/serve/httpAcpBridge.test.ts | 8 +- packages/cli/src/serve/httpAcpBridge.ts | 103 ++++++++++++------- packages/cli/src/serve/runQwenServe.ts | 22 ++-- packages/cli/src/serve/server.ts | 56 +++++++--- 4 files changed, 122 insertions(+), 67 deletions(-) diff --git a/packages/cli/src/serve/httpAcpBridge.test.ts b/packages/cli/src/serve/httpAcpBridge.test.ts index 48b72a0d924..e08fe4bba8a 100644 --- a/packages/cli/src/serve/httpAcpBridge.test.ts +++ b/packages/cli/src/serve/httpAcpBridge.test.ts @@ -291,13 +291,11 @@ describe('createHttpAcpBridge', () => { expect(a.sessionId).toBe(SESS_A); // Cross-workspace POST throws before touching the channel. - await expect( - bridge.spawnOrAttach({ workspaceCwd: WS_B }), - ).rejects.toBeInstanceOf(WorkspaceMismatchError); - // The error carries both paths for the route's 400 body. + // Single `.catch` capture — assert instance + carried fields off + // the same caught value rather than firing the rejection twice. const err = await bridge .spawnOrAttach({ workspaceCwd: WS_B }) - .catch((e: WorkspaceMismatchError) => e); + .catch((e: unknown) => e); expect(err).toBeInstanceOf(WorkspaceMismatchError); expect((err as WorkspaceMismatchError).bound).toBe(WS_A); expect((err as WorkspaceMismatchError).requested).toBe(WS_B); diff --git a/packages/cli/src/serve/httpAcpBridge.ts b/packages/cli/src/serve/httpAcpBridge.ts index 265182edda5..c35e0802acd 100644 --- a/packages/cli/src/serve/httpAcpBridge.ts +++ b/packages/cli/src/serve/httpAcpBridge.ts @@ -1151,21 +1151,17 @@ export function createHttpAcpBridge(opts: BridgeOptions): HttpAcpBridge { ); const connection = new ClientSideConnection(() => client, channel.stream); - // tanzhenxin cold-spawn-window finding: the agent child exists - // from the moment `channelFactory(boundWorkspace)` returns, but - // pre-fix `aliveChannels.add(info)` ran only AFTER the - // `initialize` handshake completed (up to `initTimeoutMs`, default - // 10s). A double-Ctrl+C in that handshake window played out as: - // first SIGINT entered `shutdown()` and awaited the in-flight - // spawn; second SIGINT called `killAllSync()` against an empty - // `aliveChannels` (the channel hadn't been added yet) and - // `process.exit(1)` orphaned the child. Add to `aliveChannels` - // BEFORE the handshake await, and register the `channel.exited` - // handler immediately afterward so init-failure / child-crash / - // late-shutdown all clean up through the standard exit path - // instead of needing bespoke catches. `channelInfo` (the attach - // target) stays set only AFTER initialize succeeds — see further - // down — so callers don't attach to a still-handshaking channel. + // Add to `aliveChannels` + register the `channel.exited` handler + // BEFORE the `initialize` handshake (tanzhenxin cold-spawn-window + // finding): the agent child exists from the moment + // `channelFactory(boundWorkspace)` returns, so a `killAllSync()` + // during the handshake window (up to `initTimeoutMs`, default + // 10s) must find it to avoid orphaning on `process.exit(1)`. + // Init-failure / child-crash / late-shutdown all converge on + // the same cleanup path via the handler below. + // `channelInfo` (the attach target) is assigned only AFTER + // initialize succeeds so callers don't attach to a still- + // handshaking channel. const info: ChannelInfo = { channel, connection, @@ -1174,6 +1170,24 @@ export function createHttpAcpBridge(opts: BridgeOptions): HttpAcpBridge { isDying: false, }; aliveChannels.add(info); + // Belt-and-suspenders leak detection. The set is intentionally + // multi-entry to cover the `killSession`-then-`spawnOrAttach` + // overlap window (size 2 is legitimate: one dying + one fresh + // attach-target). Anything higher implies a `channel.exited` + // handler never fired for some prior channel — a real leak we'd + // otherwise notice only as gradually-growing RSS over hours. + // The warning surfaces it the moment it happens. Threshold is + // 2 because that's the design ceiling; bumping it requires + // updating both this guard and the comments around + // `aliveChannels` declaration. + if (aliveChannels.size > 2) { + writeStderrLine( + `qwen serve: WARNING aliveChannels.size=${aliveChannels.size} ` + + `(expected 1, max 2 during killSession-then-spawnOrAttach ` + + `overlap) — possible channel leak; check that prior channels' ` + + `channel.exited fired and the handler ran cleanup.`, + ); + } // One-time channel.exited cleanup. The child dying takes ALL // multiplexed sessions with it — iterate `sessionIds` (snapshot @@ -1203,19 +1217,27 @@ export function createHttpAcpBridge(opts: BridgeOptions): HttpAcpBridge { if (channelInfo === info) channelInfo = undefined; const sessions = Array.from(info.sessionIds); info.sessionIds.clear(); - // Operator breadcrumb on the daemon side. Without it, an - // agent crash (OOM / segfault) is invisible from the daemon - // log: each affected SSE subscriber sees a `session_died` - // frame and disconnects, the daemon's child-stderr forwarder - // emits whatever the child wrote before dying (often - // nothing on a SIGKILL / segfault), and operators can't tell - // from `qwen serve`'s own output that the agent process is - // gone. Log the OS-level signal/exitCode + how many sessions - // were affected so the line is the canonical "agent - // disappeared" indicator. - writeStderrLine( - `qwen serve: channel exited (code=${exitInfo?.exitCode ?? 'none'}, signal=${exitInfo?.signalCode ?? 'none'}, ${sessions.length} session(s) torn down)`, - ); + // Operator breadcrumb for UNEXPECTED channel exits. Without + // this an agent crash (OOM / segfault) is invisible from the + // daemon log: each affected SSE subscriber sees a + // `session_died` frame and disconnects, the daemon's + // child-stderr forwarder emits whatever the child wrote before + // dying (often nothing on a SIGKILL / segfault), and operators + // can't tell from `qwen serve`'s own output that the agent + // process is gone. + // + // Suppressed during `shuttingDown` because the operator + // already saw "received SIGINT, draining..." from + // `runQwenServe`'s signal handler. The standalone + // killSession case (last session leaves, channel torn down + // but daemon stays up) still logs — there's no upstream + // context line in that flow, and the message confirms the + // cleanup actually ran. + if (!shuttingDown) { + writeStderrLine( + `qwen serve: channel exited (code=${exitInfo?.exitCode ?? 'none'}, signal=${exitInfo?.signalCode ?? 'none'}, ${sessions.length} session(s) torn down)`, + ); + } for (const sid of sessions) { const sessEntry = byId.get(sid); if (!sessEntry) continue; @@ -1258,11 +1280,17 @@ export function createHttpAcpBridge(opts: BridgeOptions): HttpAcpBridge { 'initialize', ); } catch (err) { - // Mark dying so concurrent `ensureChannel` callers spawn - // fresh instead of awaiting this in-flight promise's - // `channelInfo` (which we never assigned). The kill below - // triggers `channel.exited` → handler runs `aliveChannels - // .delete(info)` → entry leaves the alive set after OS reap. + // Mark the half-initialized channel as dying/unavailable, then + // kill it. Coalesced callers (`inFlightChannelSpawn` branch in + // `ensureChannel`) observe the same rejection on this promise + // and propagate it to their callers; the `inFlightSpawns` + // tracker is cleared in `spawnOrAttach`'s finally so a follow- + // up call retries cleanly. The `channel.exited` handler + // registered earlier removes `info` from `aliveChannels` once + // the OS reaps the child. `isDying` here is the cross-path + // invariant marker (matches `killSession` / `doSpawn`- + // newSession-failure / `shutdown`): "any channel in + // `aliveChannels` with `isDying === true` is mid-teardown." info.isDying = true; await channel.kill().catch(() => {}); throw err; @@ -2091,11 +2119,10 @@ export function createHttpAcpBridge(opts: BridgeOptions): HttpAcpBridge { // tanzhenxin BkUyD: iterate `aliveChannels` (the OS-level "still // alive" source of truth) — `channelInfo` only points at the // CURRENT attach target, missing any dying channel whose - // `channel.exited` hasn't fired yet. Pre-fix this iterated - // `channelInfo` alone; if a fresh spawn had already overwritten - // it during the prior channel's SIGTERM grace, the dying child - // received SIGTERM but no SIGKILL escalation when `process.exit(1)` - // fires and was orphaned. + // `channel.exited` hasn't fired yet. Without this, a fresh + // spawn overwriting `channelInfo` during the prior channel's + // SIGTERM grace would leave the dying child without SIGKILL + // escalation when `process.exit(1)` fires. shuttingDown = true; const channels = Array.from(aliveChannels); defaultEntry = undefined; diff --git a/packages/cli/src/serve/runQwenServe.ts b/packages/cli/src/serve/runQwenServe.ts index f09ec5e8459..250fb28d9f3 100644 --- a/packages/cli/src/serve/runQwenServe.ts +++ b/packages/cli/src/serve/runQwenServe.ts @@ -172,18 +172,16 @@ export async function runQwenServe( }); let actualPort = opts.port; // Pass the already-canonical `boundWorkspace` into `createServeApp` - // via `opts.workspace`. `canonicalizeWorkspace` is idempotent so - // server.ts's own canonicalize is a no-op, but if some future - // refactor ever makes it non-idempotent (caching, async I/O, - // etc.) the value the route advertises on `/capabilities` and the - // value the bridge enforces would diverge — landing clients in a - // "/capabilities says X, POST /session/X 400s with workspace_mismatch" - // contradiction. Pre-canonicalizing here removes that drift risk. - const app = createServeApp( - { ...opts, workspace: boundWorkspace }, - () => actualPort, - { bridge }, - ); + // via `deps.boundWorkspace`. That field is the pre-canonicalized + // fast-path: createServeApp skips its own `canonicalizeWorkspace` + // call (which would issue a redundant `realpathSync.native` + // syscall — idempotent but unnecessary I/O at boot). Direct + // callers of createServeApp (tests / embeds) omit it and the + // server canonicalizes itself. + const app = createServeApp(opts, () => actualPort, { + bridge, + boundWorkspace, + }); // Node's `app.listen()` wants the unbracketed IPv6 literal (`::1`) but // operators conventionally type `[::1]` (or copy/paste from URLs that diff --git a/packages/cli/src/serve/server.ts b/packages/cli/src/serve/server.ts index 1cc926bc031..9777c8fd496 100644 --- a/packages/cli/src/serve/server.ts +++ b/packages/cli/src/serve/server.ts @@ -30,6 +30,19 @@ import { export interface ServeAppDeps { /** Bridge instance; tests inject a fake. Defaults to a fresh real one. */ bridge?: HttpAcpBridge; + /** + * Pre-canonicalized workspace path. When supplied, `createServeApp` + * skips its own `canonicalizeWorkspace` call (which would issue a + * redundant `realpathSync.native` syscall — idempotent, but a hot + * boot-time stat we can avoid). `runQwenServe` passes this after + * its own boot-time canonicalize so the value used by + * `/capabilities`, the `POST /session` cwd fallback, and the + * bridge are all the SAME canonical form. Callers that haven't + * canonicalized yet (tests, direct embeds) omit this and + * `createServeApp` falls back to canonicalizing `opts.workspace ?? + * process.cwd()` itself. + */ + boundWorkspace?: string; } /** @@ -85,17 +98,22 @@ export function createServeApp( // `process.cwd()`). `POST /session` with a mismatched cwd is // rejected with 400 `workspace_mismatch`. // - // Canonicalize here so the value advertised on `/capabilities`, - // used for the `POST /session` cwd fallback, AND passed into the - // bridge are all the SAME canonical form. Without this step the - // bridge re-canonicalizes via `realpathSync.native` and on symlinks - // / case-insensitive filesystems the bridge's stored form diverges - // from the value `/capabilities` advertises — clients echoing the - // advertised path back end up with a response whose `workspaceCwd` - // differs from what they sent. `canonicalizeWorkspace` is - // idempotent so `runQwenServe`'s pre-canonicalization here is a - // no-op when the caller already did the work. - const boundWorkspace = canonicalizeWorkspace(opts.workspace ?? process.cwd()); + // The value advertised on `/capabilities`, used for the `POST + // /session` cwd fallback, AND passed into the bridge must be the + // SAME canonical form — otherwise the bridge's + // `realpathSync.native` would diverge from what `/capabilities` + // shows on symlinks / case-insensitive filesystems, and clients + // echoing the advertised path back would see a response whose + // `workspaceCwd` differs from what they sent. + // + // `deps.boundWorkspace` is the pre-canonicalized fast-path — + // `runQwenServe` passes it after its own boot-time + // `canonicalizeWorkspace`, so we skip the redundant + // `realpathSync.native` here. When omitted (tests, direct embeds) + // we canonicalize ourselves. + const boundWorkspace = + deps.boundWorkspace ?? + canonicalizeWorkspace(opts.workspace ?? process.cwd()); const bridge = deps.bridge ?? createHttpAcpBridge({ @@ -903,9 +921,23 @@ function sendBridgeError( // every client request silently 400s. Limited to authenticated // requests by the upstream bearer-token gate, so probing-DoS // log noise stays bounded. + // SECURITY: `err.requested` is derived from the request body + // (`req.workspaceCwd` → `canonicalizeWorkspace` → here). `path.resolve` + // + `realpathSync.native` both preserve control characters inside + // path segments — they only normalize separators / `..` / `.` and + // walk symlinks. A body like `{"cwd": "/legit/path\nqwen serve: + // FAKE LOG LINE"}` would otherwise emit two valid-looking daemon + // log lines, weaponizing line-based log shippers (Splunk / Loki / + // journald → SIEM). `JSON.stringify` escapes control chars and + // wraps in quotes so any injection attempt surfaces as + // visible-as-quoted-noise rather than forged-line. `err.bound` is + // safe (canonicalized at boot from operator-controlled + // `--workspace` / `process.cwd()`) but quoted symmetrically for + // readability. writeStderrLine( `qwen serve: workspace_mismatch (POST /session): ` + - `daemon bound to "${err.bound}", rejected "${err.requested}"`, + `daemon bound to ${JSON.stringify(err.bound)}, ` + + `rejected ${JSON.stringify(err.requested)}`, ); res.status(400).json({ error: err.message, From 0c6e963cd24cecdaff49966adb03a24666f20e5f Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Fri, 15 May 2026 08:08:44 +0800 Subject: [PATCH 14/16] fix(serve): apply auto-fixes from /review (#4113) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - canonicalizeWorkspace: narrow catch to ENOENT only, propagate other filesystem errors - listWorkspaceSessions: add fast-path string equality to avoid realpathSync on every poll - GET /workspace/:id/sessions: return 400 workspace_mismatch for cross-workspace queries - SessionNotFoundError: accept optional extra message; clarify agent-crash-on-spawn case - requireWorkspaceCwd: distinguish empty-string (post-§02 bug) from absent (pre-§02 daemon) --- packages/cli/src/serve/httpAcpBridge.ts | 42 ++++++++++++++++----- packages/cli/src/serve/server.ts | 12 ++++++ packages/sdk-typescript/src/daemon/types.ts | 4 +- 3 files changed, 47 insertions(+), 11 deletions(-) diff --git a/packages/cli/src/serve/httpAcpBridge.ts b/packages/cli/src/serve/httpAcpBridge.ts index c35e0802acd..7f3c41a3edc 100644 --- a/packages/cli/src/serve/httpAcpBridge.ts +++ b/packages/cli/src/serve/httpAcpBridge.ts @@ -223,8 +223,8 @@ export interface HttpAcpBridge { */ export class SessionNotFoundError extends Error { readonly sessionId: string; - constructor(sessionId: string) { - super(`No session with id "${sessionId}"`); + constructor(sessionId: string, extra?: string) { + super(`No session with id "${sessionId}"` + (extra ? `. ${extra}` : '')); this.name = 'SessionNotFoundError'; this.sessionId = sessionId; } @@ -1680,12 +1680,14 @@ export function createHttpAcpBridge(opts: BridgeOptions): HttpAcpBridge { // continuation running (e.g. channel.exited firing during // a crash spawn, or a direct bridge.killSession call from // outside the route handler). In those cases byId.get() - // returned undefined; we'd otherwise return - // `{ attached: true, sessionId: }` and every - // subsequent prompt/cancel call would 404. Fail loud - // instead so the caller can retry into a fresh spawn. + // returned undefined. Fail loud with a descriptive error + // so the caller can distinguish "immediate agent death" + // from a stale sessionId and retry into a fresh spawn. if (!attachedEntry) { - throw new SessionNotFoundError(session.sessionId); + throw new SessionNotFoundError( + session.sessionId, + 'the agent child likely crashed during initialization — retry to spawn a new session', + ); } if (req.modelServiceId) { // Same swallow as above — we picked up an in-flight @@ -1897,7 +1899,16 @@ export function createHttpAcpBridge(opts: BridgeOptions): HttpAcpBridge { listWorkspaceSessions(workspaceCwd) { if (!path.isAbsolute(workspaceCwd)) return []; - const key = canonicalizeWorkspace(workspaceCwd); + // fast-path: under §02 single-workspace, string equality + // with boundWorkspace avoids a realpathSync syscall on + // every poll. If the literal doesn't match, canonicalize + // to handle symlink aliases; if that still doesn't match, + // this daemon doesn't own the workspace. + const key = + workspaceCwd === boundWorkspace + ? boundWorkspace + : canonicalizeWorkspace(workspaceCwd); + if (key !== boundWorkspace) return []; const out: BridgeSessionSummary[] = []; for (const entry of byId.values()) { if (entry.workspaceCwd === key) { @@ -2321,8 +2332,19 @@ export function canonicalizeWorkspace(p: string): string { // entire bridge-side path resolution anyway, but if Stage 2 // ever lands without that change, switch to the async version. return realpathSync.native(resolved); - } catch { - return resolved; + } catch (err) { + // Only fall back to path.resolve for ENOENT (path doesn't exist + // yet). Other filesystem errors (EACCES, EIO, ELOOP) should + // propagate — swallowing them would hide transient I/O failures + // behind misleading workspace_mismatch rejections. + if ( + err && + typeof err === 'object' && + (err as { code?: unknown }).code === 'ENOENT' + ) { + return resolved; + } + throw err; } } diff --git a/packages/cli/src/serve/server.ts b/packages/cli/src/serve/server.ts index 9777c8fd496..d5ede8df894 100644 --- a/packages/cli/src/serve/server.ts +++ b/packages/cli/src/serve/server.ts @@ -434,6 +434,18 @@ export function createServeApp( .json({ error: '`:id` must decode to an absolute workspace path' }); return; } + // #3803 §02: reject cross-workspace queries so orchestrators + // don't mistake "no sessions here" for "workspace is idle". + const key = canonicalizeWorkspace(workspaceCwd); + if (key !== boundWorkspace) { + res.status(400).json({ + error: `Workspace mismatch: daemon is bound to "${boundWorkspace}"`, + code: 'workspace_mismatch', + boundWorkspace, + requestedWorkspace: key, + }); + return; + } const sessions = bridge.listWorkspaceSessions(workspaceCwd); res.status(200).json({ sessions }); }); diff --git a/packages/sdk-typescript/src/daemon/types.ts b/packages/sdk-typescript/src/daemon/types.ts index d5ce430d3d0..7114b0f6670 100644 --- a/packages/sdk-typescript/src/daemon/types.ts +++ b/packages/sdk-typescript/src/daemon/types.ts @@ -91,7 +91,9 @@ export function requireWorkspaceCwd(caps: DaemonCapabilities): string { if (typeof caps.workspaceCwd !== 'string' || caps.workspaceCwd.length === 0) { throw new DaemonCapabilityMissingError( 'workspaceCwd', - 'introduced in #3803 §02 (1 daemon = 1 workspace)', + caps.workspaceCwd === '' + ? 'daemon returned an empty workspaceCwd (post-§02 daemon with a bug)' + : 'daemon predates #3803 §02 (1 daemon = 1 workspace); upgrade it', ); } return caps.workspaceCwd; From d87c9fc0e84a12c12afc07e8125c572300b4c907 Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Fri, 15 May 2026 09:17:27 +0800 Subject: [PATCH 15/16] fix(serve/test): bind workspace explicitly in GET /workspace tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Wave-5 commit 0c6e963cd ("apply auto-fixes from /review (#4113)") added a 400 workspace_mismatch reject path to GET /workspace/:id/sessions for cross-workspace queries, but the existing two happy-path tests queried `/work/a` / `/work/idle` against an unbound daemon (which falls back to `process.cwd()`). Both turned to 400 in CI. Bind the daemon to WS_BOUND in both happy-path tests and query the same path. Add a third regression test that pins the §02 cross-workspace rejection contract — `code: workspace_mismatch`, both paths in the body, bridge.listCalls untouched (no silent fallback regression). Brings server.test.ts from 80 → 82 tests, all passing. --- packages/cli/src/serve/server.test.ts | 79 ++++++++++++++++++++++++--- 1 file changed, 72 insertions(+), 7 deletions(-) diff --git a/packages/cli/src/serve/server.test.ts b/packages/cli/src/serve/server.test.ts index 4f7f2f68ede..1c916d271b9 100644 --- a/packages/cli/src/serve/server.test.ts +++ b/packages/cli/src/serve/server.test.ts @@ -400,6 +400,36 @@ describe('createServeApp', () => { expect(bridge.calls).toHaveLength(0); }); + it('400 when cwd exceeds MAX_WORKSPACE_PATH_LENGTH (memory amplification guard)', async () => { + // Real filesystem paths fit well under PATH_MAX (4096 on Linux). + // A multi-MB `cwd` is either a malformed client or a memory- + // amplification attempt — `WorkspaceMismatchError` interpolates + // `requested` into `.message` twice, `sendBridgeError` writes it + // to stderr, and `res.json` echoes it again, so a ~10 MB body + // (right under express.json's 10 MB cap) would amplify to + // ~60 MB/request × maxConnections. The route caps the input + // before any of those echoes. + const bridge = fakeBridge(); + const app = createServeApp( + { ...baseOpts, workspace: WS_BOUND }, + undefined, + { bridge }, + ); + // Build an absolute path of MAX+1 chars. `path.isAbsolute` + // sees the leading `/` and the length cap fires before the + // isAbsolute branch — verifying both invariants in one go. + const longCwd = `/${'a'.repeat(4096)}`; + const res = await request(app) + .post('/session') + .set('Host', `127.0.0.1:${baseOpts.port}`) + .send({ cwd: longCwd }); + expect(res.status).toBe(400); + expect(res.body.error).toMatch(/exceeds the 4096-character limit/); + // Bridge must NOT be touched — silent fallback or pass-through + // would defeat the cap. + expect(bridge.calls).toHaveLength(0); + }); + it('400 workspace_mismatch when bridge rejects cross-workspace cwd (#3803 §02)', async () => { // Single-workspace mode: bridge throws WorkspaceMismatchError // when the route forwards a non-bound cwd. Route translates @@ -631,31 +661,66 @@ describe('createServeApp', () => { describe('GET /workspace/:id/sessions', () => { it('returns the list returned by the bridge', async () => { + // #3803 §02 (commit 0c6e963cd): the route now rejects + // cross-workspace queries with 400 workspace_mismatch (so + // orchestrators don't mistake "no sessions here" for + // "workspace is idle"). Bind the daemon to the same workspace + // we'll query so the happy path runs. const bridge = fakeBridge({ listImpl: () => [ - { sessionId: 's-1', workspaceCwd: '/work/a' }, - { sessionId: 's-2', workspaceCwd: '/work/a' }, + { sessionId: 's-1', workspaceCwd: WS_BOUND }, + { sessionId: 's-2', workspaceCwd: WS_BOUND }, ], }); - const app = createServeApp(baseOpts, undefined, { bridge }); + const app = createServeApp( + { ...baseOpts, workspace: WS_BOUND }, + undefined, + { bridge, boundWorkspace: WS_BOUND }, + ); const res = await request(app) - .get(`/workspace/${encodeURIComponent('/work/a')}/sessions`) + .get(`/workspace/${encodeURIComponent(WS_BOUND)}/sessions`) .set('Host', `127.0.0.1:${baseOpts.port}`); expect(res.status).toBe(200); expect(res.body.sessions).toHaveLength(2); - expect(bridge.listCalls).toEqual(['/work/a']); + expect(bridge.listCalls).toEqual([WS_BOUND]); }); it('returns an empty array when no sessions exist for the workspace', async () => { const bridge = fakeBridge(); - const app = createServeApp(baseOpts, undefined, { bridge }); + const app = createServeApp( + { ...baseOpts, workspace: WS_BOUND }, + undefined, + { bridge, boundWorkspace: WS_BOUND }, + ); const res = await request(app) - .get(`/workspace/${encodeURIComponent('/work/idle')}/sessions`) + .get(`/workspace/${encodeURIComponent(WS_BOUND)}/sessions`) .set('Host', `127.0.0.1:${baseOpts.port}`); expect(res.status).toBe(200); expect(res.body).toEqual({ sessions: [] }); }); + it('400 workspace_mismatch when querying a cross-workspace path (#3803 §02)', async () => { + // Pin the §02 cross-workspace rejection: querying any path + // that doesn't canonicalize to the bound workspace gets a 400 + // with `code: 'workspace_mismatch'` and both paths in the + // body — so an orchestrator-aware client can route to / spawn + // the right daemon. The bridge MUST NOT be touched (a silent + // fallback would defeat the whole purpose of §02). + const bridge = fakeBridge(); + const app = createServeApp( + { ...baseOpts, workspace: WS_BOUND }, + undefined, + { bridge, boundWorkspace: WS_BOUND }, + ); + const res = await request(app) + .get(`/workspace/${encodeURIComponent(WS_DIFFERENT)}/sessions`) + .set('Host', `127.0.0.1:${baseOpts.port}`); + expect(res.status).toBe(400); + expect(res.body.code).toBe('workspace_mismatch'); + expect(res.body.boundWorkspace).toBe(WS_BOUND); + expect(bridge.listCalls).toHaveLength(0); + }); + it('400 when :id does not decode to an absolute path', async () => { const bridge = fakeBridge(); const app = createServeApp(baseOpts, undefined, { bridge }); From 594846dba8ea8e2f9ab35455ed60624339a4c860 Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Fri, 15 May 2026 09:19:10 +0800 Subject: [PATCH 16/16] fix(serve,sdk): address fourth /review round (deepseek-v4-pro x2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six new inline findings; five applied, one defer-with-reply. - Q1 (httpAcpBridge.ts + server.ts + tests): cwd length amplification through WorkspaceMismatchError. The error constructor interpolates `requested` into `.message` TWICE; `sendBridgeError` echoes it on stderr (now JSON.stringify-wrapped); `res.json` echoes it again — a ~10 MB `cwd` body (right under express.json's 10 MB cap) would amplify to ~60 MB per request × maxConnections (default 256). On loopback-default-no-token deployments this is pre-auth. Added `MAX_WORKSPACE_PATH_LENGTH = 4096` (Linux PATH_MAX); route rejects oversized `cwd` with a 400 BEFORE the bridge is touched, and the `WorkspaceMismatchError` constructor truncates `requested` as defense-in-depth for non-route callers (tests, embeds, future entry points that throw the error directly). Three new tests pin the route 400, the constructor truncation, and the normal-path passthrough. - Q2 + Q5 (httpAcpBridge.ts docs): the `channelInfo` declaration comment + `ChannelInfo.sessionIds` JSDoc + `ChannelInfo.isDying` JSDoc all overstated when `channelInfo` is cleared. Post-§02 the BkUyD invariant is "ONLY `channel.exited` clears `channelInfo`" — teardown initiators (killSession last-session-leaving, doSpawn-newSession-failure, ensureChannel init-failure/late- shutdown, shutdown) set `isDying = true` but LEAVE `channelInfo` pointing at the dying channel until OS reap, so `killAllSync` can still reach it through `aliveChannels`. A future maintainer reading the old phrasing might "fix" killSession to also clear `channelInfo` and silently break the double-Ctrl+C force-kill path. Rewrote all three sites to describe the actual invariant + enumerate the 5 isDying set-sites + spell out the BkUyD rationale in one place (the `isDying` JSDoc) that other comments point at. - Q3 (runQwenServe.ts): the "listening on …" boot summary goes to stdout but every other operational diagnostic (bearer auth, the workspace_mismatch breadcrumb, channel-exited, bridge errors) goes to stderr. Operators capturing only stderr (systemd / docker / k8s default) miss the `workspace=` indicator, which is the single piece of information they need most when triaging §02 migration issues. Added a `qwen serve: bound to workspace "X"` stderr line alongside the stdout one — keeps stdout untouched (integration tests + scripts parse it) while making the breadcrumb visible to stderr-only log shippers. `JSON.stringify` the boundWorkspace value (operator-controlled but cheap defense-in-depth against any future flow that lands a control char in the path). - Q4 (integration-tests/tsconfig.json): the `paths` entry resolved `@qwen-code/sdk` to the SDK's built `dist/` directory; `dist/` is gitignored and stale dist (no `npm run build` first) yields TS2339 errors on the integration tests' imports of new SDK fields. Pointed `paths` at SDK source instead — `tsc -p integration-tests/tsconfig.json` no longer requires a prior rebuild. The vitest config's runtime alias still resolves to `dist/index.mjs` so the actual test execution exercises the published-bundle shape; this paths entry only affects type resolution. - Q6 (httpAcpBridge.ts): `createHttpAcpBridge` constructor called `canonicalizeWorkspace(opts.boundWorkspace)` even when the caller (`runQwenServe`) had already canonicalized and threaded the same value through `deps.boundWorkspace` into `createServeApp`. Two independent `realpathSync.native` calls can theoretically diverge on NFS-transient / mid-rename filesystems, landing the bridge with a canonical form different from what `/capabilities` advertises and from `createServeApp`'s view. Dropped the bridge's re-canonicalize; kept `path.isAbsolute` (structural, not a syscall); documented the caller contract on `BridgeOptions .boundWorkspace` ("MUST be pre-canonicalized; tests/embeds call `canonicalizeWorkspace` first"). Tests use `path.resolve(path.sep, ...)` which is already canonical-or- fallback for non-existent paths, so no test changes needed. bridge: 76/76 (was 74, +2 WorkspaceMismatchError truncation tests); server: 82/82 (was 80, +2 length cap + the auto-applied helper). tsc clean for SDK, CLI PR-touched files, and integration-tests' qwen-serve-*. --- integration-tests/tsconfig.json | 3 +- packages/cli/src/serve/httpAcpBridge.test.ts | 35 +++++ packages/cli/src/serve/httpAcpBridge.ts | 144 ++++++++++++++----- packages/cli/src/serve/runQwenServe.ts | 15 ++ packages/cli/src/serve/server.ts | 20 +++ 5 files changed, 182 insertions(+), 35 deletions(-) diff --git a/integration-tests/tsconfig.json b/integration-tests/tsconfig.json index 0cd24f82f59..8e94434c0c3 100644 --- a/integration-tests/tsconfig.json +++ b/integration-tests/tsconfig.json @@ -5,7 +5,8 @@ "allowJs": true, "baseUrl": ".", "paths": { - "@qwen-code/sdk": ["../packages/sdk-typescript/dist/index.d.ts"] + "//": "Resolve types from SDK source rather than dist so `tsc -p` here does not require a fresh `npm run build` of the SDK package before checking integration tests. The runtime vitest alias (`integration-tests/vitest.config.ts`) still points at the built `dist/index.mjs` to exercise the published-bundle shape; this paths entry only affects type resolution.", + "@qwen-code/sdk": ["../packages/sdk-typescript/src/index.ts"] } }, "include": ["**/*.ts"], diff --git a/packages/cli/src/serve/httpAcpBridge.test.ts b/packages/cli/src/serve/httpAcpBridge.test.ts index e08fe4bba8a..6036137567e 100644 --- a/packages/cli/src/serve/httpAcpBridge.test.ts +++ b/packages/cli/src/serve/httpAcpBridge.test.ts @@ -35,6 +35,7 @@ import type { import { createHttpAcpBridge, InvalidPermissionOptionError, + MAX_WORKSPACE_PATH_LENGTH, SessionNotFoundError, WorkspaceMismatchError, type AcpChannel, @@ -307,6 +308,40 @@ describe('createHttpAcpBridge', () => { await bridge.shutdown(); }); + it('WorkspaceMismatchError truncates oversized `requested` to MAX_WORKSPACE_PATH_LENGTH (defense-in-depth)', () => { + // The route-level cap in `server.ts` rejects oversized `cwd` + // bodies before reaching the bridge, but `WorkspaceMismatchError` + // can be constructed directly by other callers (tests, embeds, + // future entry points) or by passing pre-validated paths that + // somehow grew. The constructor interpolates `requested` into + // `.message` twice + downstream code echoes it on stderr + + // `res.json` — without truncation a 10 MB string amplifies + // ~6× per request. The truncation here is the cross-caller + // belt-and-suspenders defense. + const oversized = '/' + 'a'.repeat(MAX_WORKSPACE_PATH_LENGTH * 2); + const err = new WorkspaceMismatchError('/work/bound', oversized); + expect(err.requested.length).toBeLessThanOrEqual( + MAX_WORKSPACE_PATH_LENGTH + 32, // truncation marker overhead + ); + expect(err.requested.endsWith('…[truncated]')).toBe(true); + // `.message` interpolates `requested` twice; both go through the + // truncated form, so the message is bounded too. + expect(err.message.length).toBeLessThan( + MAX_WORKSPACE_PATH_LENGTH * 2 + 1024, + ); + // Bound is operator-controlled — not truncated. + expect(err.bound).toBe('/work/bound'); + }); + + it('WorkspaceMismatchError passes through `requested` shorter than MAX_WORKSPACE_PATH_LENGTH untouched', () => { + // Common case: legitimate `requested` paths (PATH_MAX is 4096 on + // Linux, 1024 on macOS) should not be modified. + const normal = '/work/different'; + const err = new WorkspaceMismatchError('/work/bound', normal); + expect(err.requested).toBe(normal); + expect(err.requested.endsWith('…[truncated]')).toBe(false); + }); + it('creates fresh session per call under sessionScope:thread (Stage 1.5 multi-session: shares channel)', async () => { const handles: ChannelHandle[] = []; const factory: ChannelFactory = async () => { diff --git a/packages/cli/src/serve/httpAcpBridge.ts b/packages/cli/src/serve/httpAcpBridge.ts index 7f3c41a3edc..af751d3a3f5 100644 --- a/packages/cli/src/serve/httpAcpBridge.ts +++ b/packages/cli/src/serve/httpAcpBridge.ts @@ -259,19 +259,46 @@ export class WorkspaceMismatchError extends Error { readonly bound: string; readonly requested: string; constructor(bound: string, requested: string) { + // Truncate `requested` to PATH_MAX so a malicious or buggy client + // can't amplify a multi-MB `cwd` body through this error. The + // constructor interpolates `requested` into `.message` TWICE, the + // route's `sendBridgeError` echoes it in stderr (now JSON.stringify + // -wrapped per the log-injection fix), and `res.json` echoes it in + // the 400 body — without truncation a ~10 MB cwd (right under the + // `express.json({limit: '10mb'})` cap) becomes ~20 MB message + + // ~10 MB stderr + ~30 MB JSON response per request, × + // `maxConnections` (default 256). The route also caps `cwd.length` + // at this same limit upstream (POST /session); this is + // defense-in-depth for non-HTTP callers (tests, embeds, future + // entry points that throw the error directly). + const safeRequested = + requested.length > MAX_WORKSPACE_PATH_LENGTH + ? `${requested.slice(0, MAX_WORKSPACE_PATH_LENGTH)}…[truncated]` + : requested; super( `Workspace mismatch: daemon is bound to "${bound}" but ` + - `request asked for "${requested}". Each \`qwen serve\` ` + + `request asked for "${safeRequested}". Each \`qwen serve\` ` + `daemon binds to exactly one workspace; start a separate ` + - `daemon for "${requested}" (or route the request to one ` + + `daemon for "${safeRequested}" (or route the request to one ` + `via an orchestrator).`, ); this.name = 'WorkspaceMismatchError'; this.bound = bound; - this.requested = requested; + this.requested = safeRequested; } } +/** + * PATH_MAX on Linux is 4096; macOS / BSD is 1024. We use the Linux + * value as a generous ceiling — anything bigger is either a + * malformed client request (memory amplification attack against the + * 400 / stderr / error-message echo paths) or a synthetic test + * input. The route's POST /session pre-check rejects bodies past + * this; `WorkspaceMismatchError` truncates for any caller that + * skips the pre-check. + */ +export const MAX_WORKSPACE_PATH_LENGTH = 4096; + /** * One ACP NDJSON channel to a single agent. Tests inject a fake by replacing * the channel factory; production uses `defaultSpawnChannelFactory`. @@ -377,14 +404,24 @@ export interface BridgeOptions { */ maxPendingPermissionsPerSession?: number; /** - * Absolute path this daemon is bound to (per #3803 §02: 1 daemon = - * 1 workspace). `spawnOrAttach` calls whose `workspaceCwd` doesn't - * canonicalize to this same value throw `WorkspaceMismatchError` - * (route → 400 with code `workspace_mismatch`). + * Absolute, **already-canonical** path this daemon is bound to (per + * #3803 §02: 1 daemon = 1 workspace). `spawnOrAttach` calls whose + * `workspaceCwd` doesn't canonicalize to this same value throw + * `WorkspaceMismatchError` (route → 400 with code `workspace_mismatch`). * - * The bound path is canonicalized once at boot time so subsequent - * request comparisons are canonical-vs-canonical (mixed casing / - * symlink resolution / etc. already accounted for). + * **Caller contract**: pass the result of + * `canonicalizeWorkspace(path)`. `runQwenServe` does this at boot + * and threads the same canonical value into the bridge AND + * `createServeApp` (via `deps.boundWorkspace`) so all three — + * `/capabilities.workspaceCwd`, the `POST /session` cwd fallback, + * and this bridge's mismatch check — share one canonical form. The + * constructor only checks `path.isAbsolute`; it does NOT + * re-canonicalize (a redundant `realpathSync.native` could + * theoretically diverge from the runQwenServe canonicalize on + * NFS-transient / mid-rename filesystems, landing the bridge with + * one canonical form while `/capabilities` advertises another). + * Direct embeds / tests calling `createHttpAcpBridge` themselves + * MUST canonicalize before passing. */ boundWorkspace: string; } @@ -418,22 +455,39 @@ interface ChannelInfo { /** * Live session ids multiplexed on this channel. Updated when * `doSpawn` registers a new session and when `killSession` / - * `channel.exited` removes one. When the set drops to empty AND no - * session is mid-attach, the channel is killed and `channelInfo` - * is cleared. + * `channel.exited` removes one. When the set drops to empty under + * `killSession`, the channel is marked `isDying = true` and its + * `channel.kill()` is awaited; `channelInfo` itself is left + * pointing at the dying channel until `channel.exited` fires (see + * BkUyD invariant on `isDying` below). */ sessionIds: Set; /** - * Set synchronously by `killSession` / `doSpawn`-newSession-failure - * BEFORE awaiting `channel.kill()`. `ensureChannel` treats a dying - * channel as absent and spawns a fresh one. Without this flag a - * concurrent `spawnOrAttach` arriving during the SIGTERM grace - * window (up to 10s) would attach to a transport about to close, - * landing the caller with a sessionId that 404s on every follow-up - * request. We don't clear `channelInfo` eagerly because the - * tanzhenxin BkUyD invariant requires `killAllSync` to still find - * the channel mid-grace for force-kill — `isDying` is the - * "available-for-new-spawns" half of the two-bit state. + * MUST be set to `true` synchronously by any teardown path BEFORE + * awaiting `channel.kill()`. `ensureChannel` treats a dying channel + * as absent and spawns a fresh one — without this flag a concurrent + * `spawnOrAttach` arriving during the SIGTERM grace window (up to + * 10s) would attach to a transport about to close, landing the + * caller with a sessionId that 404s on every follow-up request. + * + * **Set-sites (5)** — any new teardown path MUST call into one of + * these or replicate the pattern: + * + * 1. `ensureChannel`: `initialize`-failure catch. + * 2. `ensureChannel`: late-shutdown re-check (shuttingDown flipped + * during handshake). + * 3. `doSpawn`: newSession-failure on an empty channel + * (sessionIds.size === 0). + * 4. `killSession`: last session leaving (sessionIds.size === 0 + * after the delete). + * 5. `shutdown`: bulk-mark every entry in `aliveChannels`. + * + * **BkUyD invariant (why we don't clear `channelInfo` here)**: + * `killAllSync` must still find the channel during the SIGTERM + * grace window to fire SIGKILL on `process.exit(1)`. `aliveChannels` + * holds the dying entry until `channel.exited` fires (OS-level + * reap); `isDying` is the "available-for-new-spawns" half of the + * two-bit (alive, dying) state. */ isDying: boolean; } @@ -990,16 +1044,31 @@ export function createHttpAcpBridge(opts: BridgeOptions): HttpAcpBridge { maxPendingRaw > 0 && Number.isFinite(maxPendingRaw) ? maxPendingRaw : Infinity; - // #3803 §02: canonicalize the bound path once at boot so subsequent - // `spawnOrAttach` comparisons are canonical-vs-canonical (mixed - // casing / symlink resolution / etc. already accounted for). + // #3803 §02: the bound path is the canonical form `spawnOrAttach` + // compares incoming `workspaceCwd` against. The caller MUST pass an + // already-canonical value (via `canonicalizeWorkspace`). `runQwenServe` + // does this at boot and threads the same value into both + // `createHttpAcpBridge` and `createServeApp` (via + // `deps.boundWorkspace`); direct embeds / tests that construct the + // bridge themselves must call `canonicalizeWorkspace` first. + // + // Pre-fix the bridge re-canonicalized defensively here. The fix + // (deepseek-v4-pro review) drops the redundant `realpathSync.native`: + // (a) on case-insensitive / symlinked filesystems two independent + // `realpathSync.native` calls could theoretically disagree if the FS + // mutates between them (NFS transient, operator rename), landing + // the bridge with one canonical form while `runQwenServe` advertises + // another and `/capabilities` clients see `workspace_mismatch` on + // every POST; (b) it's a syscall removed from the boot path. The + // `path.isAbsolute` guard stays — it's a structural input check, not + // a syscall. if (!path.isAbsolute(opts.boundWorkspace)) { throw new TypeError( `Invalid boundWorkspace: "${opts.boundWorkspace}". Must be an ` + `absolute path.`, ); } - const boundWorkspace = canonicalizeWorkspace(opts.boundWorkspace); + const boundWorkspace = opts.boundWorkspace; // #3803 §02 single-workspace model: the bridge hosts AT MOST one // ATTACH-AVAILABLE channel and one default attach-target entry. @@ -1008,13 +1077,20 @@ export function createHttpAcpBridge(opts: BridgeOptions): HttpAcpBridge { // same-workspace attach under `single` scope reuses). Thread-scope // sessions add to `byId` but don't displace `defaultEntry`. let defaultEntry: SessionEntry | undefined; - // `channelInfo` is the SINGLE attach-available channel. Cleared when - // `killSession` marks the last session leaving (via `isDying`), - // when `doSpawn`'s newSession fails on an empty channel, or when - // `channel.exited` fires for the channel currently in this slot. - // Race-aware code paths (`ensureChannel`, `killAllSync`) gate on - // `isDying` rather than presence — see ChannelInfo.isDying for the - // BkUyD invariant. + // `channelInfo` is the SINGLE attach-available channel. Cleared + // ONLY by the `channel.exited` handler (see below) when the OS + // reaps the underlying child process. Teardown initiators + // (`killSession` last-session-leaving, `doSpawn`-newSession-failure + // on an empty channel, `ensureChannel` init-failure / + // late-shutdown, `shutdown`) set `isDying = true` but LEAVE + // `channelInfo` pointing at the dying channel until OS reap — that + // asymmetry IS the BkUyD invariant. It lets `killAllSync` reach a + // mid-SIGTERM-grace channel through `aliveChannels` while a + // concurrent `spawnOrAttach` can already start spawning a fresh + // replacement (which overwrites `channelInfo` when its + // handshake completes). Race-aware code paths (`ensureChannel`, + // `killAllSync`) gate on `isDying` rather than presence; see + // `ChannelInfo.isDying` for the per-set-site rationale. let channelInfo: ChannelInfo | undefined; // tanzhenxin BkUyD: superset of `channelInfo` covering channels // that are dying but not yet OS-reaped. `killSession` / diff --git a/packages/cli/src/serve/runQwenServe.ts b/packages/cli/src/serve/runQwenServe.ts index 250fb28d9f3..80c2d2e6a23 100644 --- a/packages/cli/src/serve/runQwenServe.ts +++ b/packages/cli/src/serve/runQwenServe.ts @@ -270,6 +270,21 @@ export async function runQwenServe( `qwen serve listening on ${url} (mode=${opts.mode}, ` + `workspace=${boundWorkspace})`, ); + // Operator log on stderr too (systemd/docker/k8s default + // captures only stderr for service diagnostics, and the + // workspace= breadcrumb is the single piece of information + // operators need most when triaging §02 migration issues — + // "did the daemon bind to the right workspace?"). The stdout + // line above stays put so integration tests + scripts that + // parse stdout for the listening URL keep working; + // `JSON.stringify(boundWorkspace)` quotes the value + // symmetrically with the workspace_mismatch log (defends + // against control-char log injection if `boundWorkspace` + // somehow contained one — operator-controlled today, but + // cheap defense-in-depth). + writeStderrLine( + `qwen serve: bound to workspace ${JSON.stringify(boundWorkspace)}`, + ); if (!token) { writeStderrLine( `qwen serve: bearer auth disabled (loopback default). Set ${QWEN_SERVER_TOKEN_ENV} to enable.`, diff --git a/packages/cli/src/serve/server.ts b/packages/cli/src/serve/server.ts index d5ede8df894..39cb996bde3 100644 --- a/packages/cli/src/serve/server.ts +++ b/packages/cli/src/serve/server.ts @@ -14,6 +14,7 @@ import { canonicalizeWorkspace, createHttpAcpBridge, InvalidPermissionOptionError, + MAX_WORKSPACE_PATH_LENGTH, SessionLimitExceededError, SessionNotFoundError, WorkspaceMismatchError, @@ -233,6 +234,25 @@ export function createServeApp( .json({ error: '`cwd` must be a string absolute path when provided' }); return; } + // Length cap BEFORE assignment so a multi-MB `cwd` body can't + // amplify through downstream interpolations + // (`WorkspaceMismatchError`'s `.message` echoes `requested` twice; + // `sendBridgeError` writes it to stderr; `res.json` echoes it + // again). On the loopback-default-no-token deployment shape this + // is pre-auth, so a 10 MB cwd body — right under + // `express.json({limit: '10mb'})` — would otherwise cost + // ~60 MB per request × `maxConnections` (default 256). The + // `MAX_WORKSPACE_PATH_LENGTH` constant matches Linux's PATH_MAX + // (4096); legitimate filesystem paths fit well under it. The + // `WorkspaceMismatchError` constructor also truncates as a + // belt-and-suspenders defense for non-route callers (tests, + // embeds, future entry points that throw the error directly). + if (hasCwd && (body['cwd'] as string).length > MAX_WORKSPACE_PATH_LENGTH) { + res.status(400).json({ + error: `\`cwd\` exceeds the ${MAX_WORKSPACE_PATH_LENGTH}-character limit`, + }); + return; + } const cwd = hasCwd ? (body['cwd'] as string) : boundWorkspace; if (!path.isAbsolute(cwd)) { res