Hey @chiga0, thanks for the PR — the ACP/REST parity work looks substantial.

Template check: the PR body is missing all required headings from the PR template. Specifically:

• ## What this PR does — the changes are described, but not under this heading
• ## Why it's needed — motivation is absent (what user problem does full parity solve? who consumes these 29 endpoints?)
• ## Reviewer Test Plan — no How to verify, no Before & After, no Tested on table
• ## Risk & Scope — not present (important for a +935-line PR touching auth, files, and session management)
• ## Linked Issues — #4782 is referenced inline but not in the required section
• 中文说明 — not present

Could you restructure the body to match the template? It helps reviewers (and future git log readers) understand the why and how to verify without reading the diff.

Quick direction note (since we're here anyway): this targets daemon_mode_b_main rather than main, so the scope makes sense as part of the larger daemon-mode effort. The per-method dispatch pattern in dispatch.ts is consistent with what's already there. No directional concerns from my side.

Once the template is filled in, happy to continue with the code review. 🙏

— Qwen Code · qwen3.7-max

Independent Review at HEAD 8674565519

PR: feat(serve): ACP/REST parity — 29 new _qwen/* methods + production hardening — +999/-15, 5 files, 1 commit

Key finding: 3 claimed fixes not present at HEAD

I verified the code at HEAD 8674565519 against wenshao's findings and your replies. Three items you replied "Fixed" to are still present in the current commit:

These fixes exist in #4773's intermediate commits (48c49aa, 4b7a15a) but were not carried into #4827's single squashed commit.

Fixes that ARE present at HEAD (verified)

wenshao findings still open (not addressed)

Priority action items

Must fix before next review round:

1. cmd → rawCmd in session/shell (line ~1057/1061) — ReferenceError blocks entire endpoint
2. providerId cast: providerId as DeviceFlowProviderId or validate against known values
3. description default: change undefined → '' to match SubagentConfig type

Should fix:
4. Add comment on sessions/delete ownership bypass (matches REST POST /sessions/delete)
5. Add BuiltinAgentRegistry.isBuiltinAgent(name) guard to agents/create — without this, ACP clients can create undeletable shadow agents

Verified by a clean npm run build from daemon_mode_b_main (which rebuilds @qwen-code/acp-bridge, so the long-disputed "stale dist" type errors — the 6 missing bridge methods, providerId, cmd/rawCmd — do not reproduce at this HEAD). Exactly one type error remains, and it is real and build-breaking (inline). The other three comments are REST-parity divergences not raised on earlier commits.

— claude-opus-4-8 via Qwen Code /qreview

R6 (commit 0324310f) — 2 Criticals from R5 still unresolved

Comparing R5 commit 7ade0754 → R6 commit 0324310f: the code at the two blocker sites is byte-identical. Neither Critical was addressed.

Critical 1 — tsc TS2345: missing level in SubagentConfig (line 1747)

packages/cli/src/serve/acpHttp/dispatch.ts(1747,13): error TS2345:
Argument of type '{ name: string; description: string; systemPrompt: string; tools: string[] | undefined; model: string | undefined; }'
is not assignable to parameter of type 'SubagentConfig'.
  Property 'level' is missing in type ... but required in type 'SubagentConfig'.

SubagentManager.createSubagent(config, options) expects config: SubagentConfig which requires level: SubagentLevel. The handler passes { level } in the second argument but not the first. Fix — add level into the config object literal (or spread { ...config, level }):

   await this.agentManager.createSubagent(
     {
       name,
+      level,
       description: ...,
       systemPrompt: ...,
       tools: ...,
       model: ...,
     },
-    { level },
   );

(See R5 inline comment at line 1747 for the same finding; the R5 reply thread confirms no fix was applied.)

Critical 2 — agents/create bypasses REST parseAgentConfig validation (line 1731)

The REST route enforces: name regex AGENT_TYPE_PATTERN, length bounds (2–64), BuiltinAgentRegistry.isBuiltinAgent(name) shadowing check, MAX_SYSTEM_PROMPT_BYTES cap, and parseStringArray for tools. The ACP handler only checks !name.trim() and forwards unvalidated fields.

Author reply in R1 deferred to follow-up #4782 ("extracting parseAgentConfig requires refactoring the 1300-line workspaceAgents.ts"). That's a reasonable plan, but no guard has been added in any subsequent push — the bypass remains as-is.

Minimum viable fix before merge: at least call BuiltinAgentRegistry.isBuiltinAgent(name) and reject shadowing, since that's a security invariant (not a validation nicety). The rest can stay as a #4782 follow-up.

All other R4 findings resolved at this HEAD

The cmd undefined (R2), providerId type (R2), description type (R3), shell .trim(), sessions/delete ownership, file caps, and device_flow gating issues are all addressed in earlier rounds. ESLint is clean on all 5 changed files; only the single tsc error above blocks build.

Verdict

Request changes (posted as Comment because the 2 Critical inline comments from R5 already exist at the same commit_id and re-posting would create duplicate annotations on the Files-changed tab). Address the level TS2345 and the builtin-shadowing guard, then re-request review.

— qwen3.7-max via Qwen Code /review

整体实现扎实，error propagation 和 protocol-level guards 做得很好。主要问题集中在 agents/create 与 update 的字段大小限制不一致（安全漏洞）、file/write 大小检查顺序、以及 dispatch.ts 膨胀需要拆分。

做得好的地方

• toRpcError 扩展后的结构化 data.errorKind 设计很好，客户端可以程序化处理不同错误类型
• auth/status 的 projection（不泄露 userCode/verificationUri）安全意识到位
• sessions/delete 的 100 cap + dedup + Promise.allSettled 处理稳健
• Content-Type 415 / Accept 406 / batch 501 的协议级防护是正确做法
• session/shell 的审计日志记录（截断长命令）

测试覆盖缺口

• Agent CRUD（list/get/create/update/delete）5 个方法零测试
• File 方法只测了 "not configured" 和参数缺失，缺 positive-path 测试
• sessions/delete 的 dedup（new Set()）逻辑没有测试
• memory/write 的 1MB 限制没有测试
• agents/update 的 256KB / tools 256-entry 限制没有测试

建议（非 blocking）

• dispatch.ts 已膨胀到 2319 行，建议拆分 Wave 1+2 handlers 到独立文件（dispatchFiles.ts、dispatchAuth.ts、dispatchAgents.ts）
• 重复 40+ 次的 if (id !== undefined) conn.sendConn(error(...)) 模式建议抽 helper 方法
• sessions/delete 用了复数命名，其他 session 方法都是单数（session/recap），命名不一致

No high-confidence issues found. Typecheck, lint, and tests all pass. The PR delivers 29 new ACP dispatch methods with consistent patterns and solid production hardening (error data propagation, session/delete dedup+cap, auth projection, BTW validation). Minor low-confidence parity gaps identified in terminal review (untrimmed shell command, tools validation, assertMutableLevel for extension agents, FILE_ERROR path disclosure) — none block merge. ✅ — qwen3.7-max via Qwen Code /review

Re-review at HEAD e39dad5

All Critical findings from the previous review round have been addressed in this update:

• sessions/delete: now correctly filters to closedIds before calling removeSessions; errors from both closeSession and removeSessions are logged to stderr
• file/list: capped at 2000 entries with truncated flag
• file/glob: capped at 5000 (configurable up to 50000) with truncated flag
• file/write: enforces 10MB content size limit
• device_flow/get: gates userCode/verificationUri on initiatorClientId match
• Constructor params: fsFactory and deviceFlowRegistry threaded through MountAcpHttpOptions
• toRpcError: properly propagates data field with errorKind for structured error handling
• HTTP protocol compliance: Content-Type 415, batch 501, Accept 406, split missing/unknown connection-id (400/404)

New tests provide good coverage of the 29 new methods, protocol compliance, and error paths.

One remaining suggestion (not blocking): the agents/create ACP handler bypasses parseAgentConfig transport-level validation (name regex/length, description/systemPrompt byte limits) that the REST path enforces. The core SubagentValidator provides a safety net for name pattern and emptiness checks, but error messages are generic and the description/systemPrompt byte limits from MAX_DESCRIPTION_BYTES (256KB) are not enforced at the ACP transport layer. Consider extracting the shared validation into a transport-agnostic function as previously discussed.

— qwen3-coder via Qwen Code /review

Review Summary

Verdict: Comment — No critical issues. 29 new ACP dispatch methods with solid parameter validation and security gating. 2 suggestions for validation consistency.

Deterministic analysis: TypeCheck ✅ (all errors pre-existing) | ESLint ✅ 0 errors

— qwen3.7-max via Qwen Code /review

No blocking issues. Comprehensive ACP/REST parity implementation with solid validation and security gating. LGTM! ✅ — qwen3.7-max via Qwen Code /review

LGTM✅~~