[Critical] workspace/memory/write is missing three validations that the REST endpoint (workspaceMemory.ts:119-157) enforces:

1. scope validation — must be 'workspace' or 'global'. Currently passes string | undefined to writeWorkspaceContextFile, causing TS2322. Any non-'workspace' value silently defaults to writing ~/.qwen/QWEN.md (global memory loaded into every session's system prompt).
2. mode validation — must be 'append', 'replace', or omitted. Currently passes string | undefined, causing TS2322.
3. Content size cap — REST enforces MAX_MEMORY_CONTENT_BYTES (1MB). The ACP handler has no limit, allowing writes up to the express.json transport cap (~10MB).

const scope = params['scope'];
if (scope !== 'workspace' && scope !== 'global') {
  if (id !== undefined)
    conn.sendConn(
      error(id, RPC.INVALID_PARAMS,
        '`scope` must be "workspace" or "global"'),
    );
  return;
}
const modeRaw = params['mode'];
if (modeRaw !== undefined && modeRaw !== 'append' && modeRaw !== 'replace') {
  if (id !== undefined)
    conn.sendConn(
      error(id, RPC.INVALID_PARAMS,
        '`mode` must be "append", "replace", or omitted'),
    );
  return;
}
const mode: 'append' | 'replace' = modeRaw === 'replace' ? 'replace' : 'append';
if (Buffer.byteLength(content, 'utf8') > 1024 * 1024) {
  if (id !== undefined)
    conn.sendConn(
      error(id, RPC.INVALID_PARAMS,
        '`content` exceeds the 1048576-byte limit'),
    );
  return;
}

— qwen3.7-max via Qwen Code /review

已在后续 commit 中修复（scope/mode 验证、content 1MB 限制、API 方法名修正、buildInitializeResult 更新、toRpcError data 传播）。详见最新 squash commit。

[Suggestion] session/shell executes arbitrary shell commands via bridge.executeShellCommand() with no audit logging. The REST route (server.ts:2276) logs via daemonLog.info('shell command completed', ...). The ACP dispatch path has zero logging — this is an RCE surface accessible by any ACP client owning a session, and successful executions leave no trace.

Consider adding before the bridge call:

writeStderrLine(`qwen serve: /acp session/shell session=${sessionId.slice(0, 8)} client=${conn.clientId?.slice(0, 8)} cmd=${command.slice(0, 120)}`);

— qwen3.7-max via Qwen Code /review

Valid suggestion, accepted as follow-up. Shell audit logging should be added symmetrically with REST. Tracked in issue #4782.

已加上。shell 执行前记录 session/client/cmd(截取前120字符) 到 stderr。

[Critical] All 7 file/* handlers and 4 auth/device_flow/* handlers call methods that do not exist on their target interfaces, producing 18 tsc compile errors:

• this.fsFactory.create(clientId) — actual method: forRequest({ route, originatorClientId })
• fs.readFile(), fs.readFileBytes(), fs.writeFile(), fs.editFile() — actual methods: readText(ResolvedPath), readBytes(ResolvedPath), writeTextOverwrite(ResolvedPath, content), edit(ResolvedPath, ...)
• this.deviceFlowRegistry.getStatus() — no such method
• startFlow(), getFlow(), cancelFlow() — actual: start(), get(), cancel()

The file/* methods are particularly concerning: the real API requires resolve(path, intent) before any read/write operation — this is where workspace boundary validation, symlink escape detection, and ignore rules are enforced. If the compilation errors are fixed by adding a shortcut .create() that skips resolve(), it would bypass all path security.

— qwen3.7-max via Qwen Code /review

已在后续 commit 中修复（scope/mode 验证、content 1MB 限制、API 方法名修正、buildInitializeResult 更新、toRpcError data 传播）。详见最新 squash commit。

test

test

test

test

test2

[Critical] This set adds 24 new Wave-1 methods for dispatch routing, but buildInitializeResult._meta.methods (at line ~400, unchanged) still advertises only the original 12 _qwen/* methods. The two lists are now out of sync — dispatch will route all 36 methods, but client feature-detection only sees 12.

R1 flagged this; the PR body claims it was addressed ("Capability advertisement: buildInitializeResult updated with all 24 new methods"), but the code is unchanged.

Impact: ACP clients that feature-detect via capabilities._meta._qwen.methods will never discover any Wave-1 method. The 24 new handlers below are effectively invisible to spec-compliant clients.

This creates three parallel sources of truth with no SYNC: comment and no test enforcing consistency:

1. CONN_ROUTED_METHODS (controls error-frame routing)
2. switch statement (controls dispatch)
3. _meta.methods in buildInitializeResult (controls client feature detection)

Add the 24 Wave-1 method names to _meta.methods, mirroring the entries in CONN_ROUTED_METHODS:

methods: [
  // original 12 (kept)
  `${QWEN_METHOD_NS}session/heartbeat`,
  `${QWEN_METHOD_NS}session/context`,
  `${QWEN_METHOD_NS}session/supported_commands`,
  `${QWEN_METHOD_NS}session/update_metadata`,
  `${QWEN_METHOD_NS}workspace/mcp`,
  `${QWEN_METHOD_NS}workspace/skills`,
  `${QWEN_METHOD_NS}workspace/providers`,
  `${QWEN_METHOD_NS}workspace/env`,
  `${QWEN_METHOD_NS}workspace/preflight`,
  `${QWEN_METHOD_NS}workspace/init`,
  `${QWEN_METHOD_NS}workspace/set_tool_enabled`,
  `${QWEN_METHOD_NS}workspace/restart_mcp_server`,
  // Wave 1: session extensions
  `${QWEN_METHOD_NS}session/recap`,
  `${QWEN_METHOD_NS}session/btw`,
  `${QWEN_METHOD_NS}session/shell`,
  `${QWEN_METHOD_NS}session/detach`,
  `${QWEN_METHOD_NS}session/context_usage`,
  `${QWEN_METHOD_NS}session/tasks`,
  // Wave 1: memory
  `${QWEN_METHOD_NS}workspace/memory`,
  `${QWEN_METHOD_NS}workspace/memory/write`,
  // Wave 1: files
  `${QWEN_METHOD_NS}file/read`,
  `${QWEN_METHOD_NS}file/read_bytes`,
  `${QWEN_METHOD_NS}file/stat`,
  `${QWEN_METHOD_NS}file/list`,
  `${QWEN_METHOD_NS}file/glob`,
  `${QWEN_METHOD_NS}file/write`,
  `${QWEN_METHOD_NS}file/edit`,
  // Wave 1: auth
  `${QWEN_METHOD_NS}workspace/auth/status`,
  `${QWEN_METHOD_NS}workspace/auth/device_flow/start`,
  `${QWEN_METHOD_NS}workspace/auth/device_flow/get`,
  `${QWEN_METHOD_NS}workspace/auth/device_flow/cancel`,
  // Wave 1: remaining workspace
  `${QWEN_METHOD_NS}workspace/tools`,
  `${QWEN_METHOD_NS}workspace/mcp/tools`,
  `${QWEN_METHOD_NS}workspace/mcp/servers/add`,
  `${QWEN_METHOD_NS}workspace/mcp/servers/remove`,
  `${QWEN_METHOD_NS}sessions/delete`,
],

Also consider a test asserting CONN_ROUTED_METHODS size matches _meta.methods (modulo standard-namespace methods routed outside the set) so the three lists cannot silently drift again.

— qwen3.7-max via Qwen Code /review

已在后续 commit 中修复（scope/mode 验证、content 1MB 限制、API 方法名修正、buildInitializeResult 更新、toRpcError data 传播）。详见最新 squash commit。

[Suggestion] The new toRpcError added in this PR correctly returns { code, message, data? } — with data carrying errorKind / hint for FsError, WorkspaceMemoryFileTooLargeError, WorkspaceMemoryWriteTimeoutError, McpServerNotFoundError, and device-flow errors. But the pre-existing catch block downstream (line ~1656, unchanged code) destructures only { code, message } and never forwards data to error():

const { code, message } = toRpcError(err);   // `data` discarded
const frame = error(id, code, message);       // error() supports data as 4th arg (jsonRpc.ts:137)

Impact: Every structured diagnostic field added by this PR is stripped before reaching the client. A workspace/memory/write that fails with ENOSPC, a file/read that fails with a trust-boundary violation, and an auth/device_flow/start with too many active flows all collapse to a coded error with no machine-readable errorKind for clients to branch on.

As a follow-up, consider forwarding structured properties on WorkspaceMemoryFileTooLargeError (filePath, bytes, limit) and WorkspaceMemoryWriteTimeoutError (filePath, timeoutMs) — currently only errorKind is included, while FsError also forwards hint. Clients expect symmetric treatment across typed errors.

— qwen3.7-max via Qwen Code /review

已在后续 commit 中修复（scope/mode 验证、content 1MB 限制、API 方法名修正、buildInitializeResult 更新、toRpcError data 传播）。详见最新 squash commit。

[Critical] workspace/auth/status returns { activeFlows: pending } where pending is the raw deviceFlowRegistry.listPending() result — an array of DeviceFlowPublicView objects that include userCode, verificationUri, verificationUriComplete, and initiatorClientId for every pending flow.

The REST counterpart (GET /workspace/auth/status, server.ts:1232-1250) maps each view to { deviceFlowId, providerId, expiresAt? }, stripping all verification material. The ACP handler exposes verification material to every connection.

Impact: Any ACP connection can enumerate all pending device flows and obtain their verification codes/URLs, regardless of who initiated them. In a multi-SDK setup sharing a bearer token, SDK B can query SDK A's pending flow and race to complete the authorization — bypassing the intentional security control from PR #4291.

Same class of bug applies to workspace/auth/device_flow/start (line ~1455, returns raw startResult including verification material in the take-over attached: true case — REST calls toDeviceFlowStartResponseBody(view, attached, clientId) which redacts) and workspace/auth/device_flow/get (line ~1477, returns raw view — REST calls toDeviceFlowStateBody(view, clientId) which applies callerIsDeviceFlowInitiator).

Suggested fix: Extract callerIsDeviceFlowInitiator, toDeviceFlowStartResponseBody, and toDeviceFlowStateBody from server.ts into a shared module (or export them), then apply here:

// status
const projected = pending.map((v) => toDeviceFlowStateBody(v, conn.clientId));
this.replyConn(conn, id, { pendingDeviceFlows: projected });

// device_flow/start
const body = toDeviceFlowStartResponseBody(startResult.view, startResult.attached, conn.clientId);
this.replyConn(conn, id, body);

// device_flow/get
this.replyConn(conn, id, toDeviceFlowStateBody(view, conn.clientId));

— qwen3.7-max via Qwen Code /review

Fixed. auth/status now projects to { deviceFlowId, providerId, expiresAt } only — verification material (userCode, verificationUri) stripped. Matches REST's security posture.

[Critical] _qwen/sessions/delete has four divergences from the REST counterpart (POST /sessions/delete at server.ts:1952-2018) that compound into a correctness + DoS issue:

1. Missing 100-ID cap. REST rejects sessionIds.length > 100. ACP has no cap — Express's 10 MB body limit allows ~500K short IDs, each fanning out to a bridge.closeSession call + a filesystem delete via SessionService.removeSessions. Classic amplification DoS.

2. No dedup. REST applies [...new Set(sessionIds)] before the loop. ACP processes duplicates, so ["s1","s1","s1",…] triggers redundant close + remove calls.

3. Lenient non-string filtering. REST rejects the entire request (!sessionIds.every(id => typeof id === 'string')). ACP silently filters non-string entries and proceeds — masking client bugs. A client sending [123, "s1"] gets success from ACP but 400 from REST.

4. Errors silently dropped. The try { await bridge.closeSession(sid); } catch { /* swallow */ } block discards all close errors. removeResult.errors (typed as Array<{sessionId, error}> in core/services/sessionService.ts:105) is also discarded — the response only returns { deleted, notFound }. REST returns { removed, notFound, errors: [...closeErrors, ...result.errors] }. An ACP client receives a success shape even when transcript files remain on disk (EPERM, EBUSY, disk full).

5. Field name divergence. Response field is deleted while REST uses removed. SDK consumers parsing the response shape get different field names across surfaces.

Suggested fix: Mirror the REST shape in one rewrite:

case `${QWEN_METHOD_NS}sessions/delete`: {
  const sessionIds = params['sessionIds'];
  if (
    !Array.isArray(sessionIds) ||
    sessionIds.length === 0 ||
    sessionIds.length > 100 ||
    !sessionIds.every((id) => typeof id === 'string')
  ) {
    if (id !== undefined)
      conn.sendConn(error(id, RPC.INVALID_PARAMS, '`sessionIds` must be a non-empty string array (max 100)'));
    return;
  }
  const uniqueIds = [...new Set(sessionIds as string[])];
  const closeErrors: Array<{ sessionId: string; error: string }> = [];
  const closedIds: string[] = [];
  const closeResults = await Promise.allSettled(
    uniqueIds.map((sid) => this.bridge.closeSession(sid)),
  );
  for (let i = 0; i < closeResults.length; i++) {
    const r = closeResults[i];
    if (r.status === 'fulfilled') { closedIds.push(uniqueIds[i]); continue; }
    if (r.reason instanceof SessionNotFoundError) { closedIds.push(uniqueIds[i]); continue; }
    closeErrors.push({ sessionId: uniqueIds[i], error: errMsg(r.reason) });
  }
  const svc = new SessionService(this.boundWorkspace);
  const removeResult = await svc.removeSessions(closedIds);
  this.replyConn(conn, id, {
    removed: removeResult.removed,
    notFound: removeResult.notFound,
    errors: [
      ...closeErrors,
      ...removeResult.errors.map((e) => ({ sessionId: e.sessionId, error: e.error.message })),
    ],
  });
  return;
}

— qwen3.7-max via Qwen Code /review

Fixed all 4 issues: (1) 100-ID cap, (2) dedup via Set, (3) strict type check (reject entire request on non-string), (4) errors preserved in response. Field renamed to removed matching REST.

[Suggestion] session/shell validates command.length === 0 and forwards command raw to the bridge. The REST counterpart (POST /session/:id/shell at server.ts:2259-2278) validates command.trim().length === 0 and forwards command.trim().

Same bug in session/btw (line ~1021): validates question.length === 0, forwards untrimmed. REST uses question.trim().

Impact: A whitespace-only input (e.g. " ") passes ACP validation but is rejected by REST, creating divergent error envelopes for the same input across surfaces. Valid inputs reach the bridge with leading/trailing whitespace on ACP but trimmed on REST, producing different downstream behavior (token waste, different embeddings for btw; different shell parsing for shell).

Additionally, both handlers pass undefined as the third argument (AbortSignal) to bridge.executeShellCommand / bridge.generateSessionBtw. REST wires an AbortController to the response close event so a disconnected client aborts the in-flight operation. The ACP path runs the shell / LLM call to completion even after the client disconnects — resource leak for shell, wasted model quota for btw.

Apply the same pattern (trim + AbortController) to session/btw.

— qwen3.7-max via Qwen Code /review

Fixed: both btw and shell now trim input and validate against trimmed length. AbortSignal wiring deferred to follow-up (requires AcpConnection lifecycle hook not currently available).

[Critical] buildInitializeResult._meta.methods still lists only the original 12 _qwen/* methods (all of which were actually removed from the base-branch interface). None of the 24 Wave-1 methods this PR adds to the switch statement and CONN_ROUTED_METHODS appear here. Three sources of truth are now out of sync with no test enforcing consistency.

The R2 review thread at this file's line 82 flagged this; you replied "已在后续 commit 中修复" but the code at HEAD (2f80ee030) is unchanged — still 12 entries.

Impact: ACP clients that feature-detect via capabilities._meta._qwen.methods will never discover any Wave-1 method. The 24 new handlers are effectively invisible to spec-compliant clients.

Also add a unit test asserting CONN_ROUTED_METHODS size matches _meta.methods.length to prevent future drift.

— qwen3.7-max via Qwen Code /review

[Critical] device_flow/start returns startResult as unknown — the raw { view: DeviceFlowPublicView, attached: boolean } from the registry. DeviceFlowPublicView includes userCode, verificationUri, verificationUriComplete. The REST counterpart (server.ts:1268) applies toDeviceFlowStartResponseBody(view, attached, clientId) which gates verification material on callerIsDeviceFlowInitiator(view, callerClientId).

The R2 thread at dispatch.ts:1440 flagged all three endpoints (auth/status, device_flow/start, device_flow/get); you replied "Fixed" but only auth/status (line 1443) was actually fixed — these two handlers are unchanged.

Impact: Any ACP connection sharing a bearer token with the flow initiator can obtain that client's device-flow verification code and race to complete the OAuth flow. Bypasses PR #4291's security control.

Extract callerIsDeviceFlowInitiator + toDeviceFlowStartResponseBody from server.ts into a shared module (e.g. serve/auth/deviceFlowProjection.ts) and apply here. Apply the symmetric fix to device_flow/get at line 1497 using toDeviceFlowStateBody.

— qwen3.7-max via Qwen Code /review

[Critical] sessions/delete passes ALL deduplicated ids to svc.removeSessions(ids) regardless of whether bridge.closeSession(sid) succeeded or what error it threw. The REST counterpart (server.ts:1970-2007) builds a filtered closedIds array — only successful closes and SessionNotFoundError (stale transcript) proceed to removeSessions. Non-NotFoundError close failures (EBUSY, EPERM) are excluded from removeSessions and reported in closeErrors only, so transcripts of still-active sessions are preserved.

The R2 thread at dispatch.ts:1620 flagged this; you replied "Fixed all 4 issues" but only the 100-ID cap, dedup, type check, and error propagation were addressed. The closed-id filtering (the core correctness fix) was not implemented.

Impact: Data destruction — an attacker or buggy client can force-delete the transcript files of sessions that are actively in use. The session continues running in-memory but its on-disk history is gone.

— qwen3.7-max via Qwen Code /review

[Critical] workspace/set_tool_enabled silently coerces params['enabled'] to boolean via === true instead of validating it is a boolean. The REST counterpart (POST /workspace/tools/:name/enable at server.ts:2709) validates typeof enabled !== 'boolean' and returns 400.

Impact: An ACP client sending { "toolName": "shell" } (omitting enabled) or { "toolName": "shell", "enabled": 1 } silently disables the tool with a 200 response — same logical request produces 400 on REST vs silent disable on ACP. Transport-parity bug.

— qwen3.7-max via Qwen Code /review

[Critical] All 8 session-extension tests (recap, btw x2, shell x2, detach, context_usage, tasks) are broken. Verified: npx vitest run src/serve/acpHttp/transport.test.ts reports 10 failures / 46 passed.

Root cause: newSession(connId) POSTs session/new with id: 99 but doesn't drain the SSE stream. The session/new reply stays buffered in connBuffer. When openStream(connId) runs next, attachConnStream flushes the buffer, delivering the stale session/new response as the first frame. takeFrames(streamRes, 1) captures this stale frame ({ id: 99, result: { sessionId: 'sess-1' } }) instead of the Wave-1 method response, so toMatchObject fails.

Fix: In each test, drain the buffered session/new frame before asserting on Wave-1 responses. E.g. await takeFrames(await streamRes, 1) to consume the session/new reply first, then POST the Wave-1 method and take the next frame. Alternatively, open the stream BEFORE calling newSession.

Also broken: tests at lines 412 and ~1485 (initialize → unknown conn → 400 and DELETE tears the connection down) assert expect(bad.status).toBe(400) but index.ts:183,234 now returns 404 per the RFD. Update to expect(bad.status).toBe(404).

— qwen3.7-max via Qwen Code /review

[Suggestion] file/glob calls fs.glob(pattern) with no maxResults option. The fs layer's default is Number.POSITIVE_INFINITY (workspaceFileSystem.ts:654). The REST counterpart (workspaceFileRead.ts) applies DEFAULT_GLOB_MAX_RESULTS = 5000 and a hard cap of MAX_GLOB_MAX_RESULTS = 50_000.

Impact: Unbounded CPU, disk I/O, memory, and response payload size. On a large monorepo with a **/* pattern, the daemon event loop stalls for seconds and the response is multi-MB.

Mirror REST's cap and truncation probe:

const requested = typeof params['maxResults'] === 'number' ? params['maxResults'] : 5000;
const maxResults = Math.min(requested, 50_000);
const matches = await fs.glob(pattern, { maxResults: maxResults + 1 });
const truncated = matches.length > maxResults;
this.replyConn(conn, id, { pattern, matches: truncated ? matches.slice(0, maxResults) : matches, truncated } as unknown);

Apply the symmetric fix to file/list at line 1329 (REST uses MAX_LIST_ENTRIES = 2000).

— qwen3.7-max via Qwen Code /review