Thanks for the PR!

Template looks good ✓

On direction: directly aligned — issue #5677 tracks remote daemon and ACP parity gaps for slash-command-only workflows, and this PR closes the permission rules read/write gap for remote clients. CHANGELOG: no direct reference but the daemon/ACP parity area is clearly within scope.

On approach: scope feels right. The extraction of permission logic into permission-settings.ts enables clean reuse between the ACP ext methods and the new REST route. The SDK helpers (get/set/add/remove) with honest TOCTOU caveats are a natural addition. The 773-line route test file is thorough — it covers the live-child path, the persist-fallback path, malformed rule preservation, and error cases. No unrelated changes or drive-by refactors spotted.

Moving on to code review. 🔍

— Qwen Code · qwen3.7-max

Code Review

Independent proposal before reading the diff: extract the permission rule types/helpers from acpAgent.ts into a shared module, add GET /workspace/permissions and POST /workspace/permissions REST routes mirroring the existing workspace-settings route pattern, register a workspace_permissions capability gated on persistSettingAvailable, and add typed SDK client methods.

The PR follows this approach exactly. The extraction into permission-settings.ts is clean — readPermissionRuleSet, normalizePermissionRules, buildPermissionSettings are reused by both the ACP ext methods and the new REST route. The normalizePermissionRules function correctly handles the malformed-rule preservation requirement (existing malformed rules round-trip, new malformed rules are rejected). The REST route properly prefers a live ACP child for writes and falls back to daemon-side persistence when no child is running.

No critical blockers found. No AGENTS.md violations.

Minor observation (not a blocker): the persistSetting type in ServeAppDeps was broadened from Promise<void> to Promise<void | LoadedSettings> to support the permissions route's response optimization. The existing settings route wraps it to discard the return — a clean compatibility approach.

Unit Tests

$ cd packages/cli && npx vitest run src/serve/routes/workspace-permissions.test.ts
 ✓ src/serve/routes/workspace-permissions.test.ts (18 tests) 150ms
 Test Files  1 passed (1)
      Tests  18 passed (18)

$ cd packages/cli && npx vitest run src/acp-integration/acpAgent.test.ts
 ✓ src/acp-integration/acpAgent.test.ts (139 tests) 6474ms
 Test Files  1 passed (1)
      Tests  139 passed (139)

$ cd packages/cli && npx vitest run src/serve/server.test.ts
 ✓ src/serve/server.test.ts (513 tests) 10526ms
 Test Files  1 passed (1)
      Tests  513 passed (513)

$ cd packages/sdk-typescript && npx vitest run test/unit/DaemonClient.test.ts
 ✓ test/unit/DaemonClient.test.ts (175 tests) 1828ms
 Test Files  1 passed (1)
      Tests  175 passed (175)

Build & Typecheck

$ npm run build
✖ 15 problems (0 errors, 15 warnings)  [pre-existing vscode-ide-companion lint warnings]

$ npm run typecheck
> tsc --noEmit  [all packages pass]

Tmux Testing

N/A — this is a daemon/ACP/SDK API change with no TUI or visual UI surface, as noted in the PR's Reviewer Test Plan.

— Qwen Code · qwen3.7-max

Stepping back: this PR does exactly what I'd do if tasked with closing the permission rules parity gap. The extraction of permission-settings.ts from the 65-line inline block in acpAgent.ts is the right move — it enables the REST route to share validation and response shaping without duplicating logic. The REST route's dual-path write strategy (live ACP child first, persist fallback second) is well-thought-out and matches how the daemon already handles settings mutations.

The SDK helpers are a natural addition. The honest JSDoc caveat about TOCTOU on add/remove is the right call — no point pretending a read-modify-write is atomic when it isn't.

All 845 relevant tests pass. Build is clean. Typecheck is clean. The diff is focused — every changed file contributes to the stated goal, no drive-by refactors, no scope creep.

The persistSetting return-type broadening in ServeAppDeps is a minor interface change, but it's handled cleanly: the existing settings route wraps it to discard the return, and the new permissions route uses it to avoid a redundant loadSettings call after persisting.

LGTM, ready to ship. ✅

— Qwen Code · qwen3.7-max

✅ Local runtime verification (real daemon, not just unit tests)

I built the real esbuild bundle from this PR's head (695aa4c) and exercised the daemon end‑to‑end — a live qwen serve driven over HTTP with curl and the TypeScript SDK — in addition to the unit/integration suites.

Environment

• Bundle: node esbuild.config.js → dist/cli.js (≈6.3 MB, API‑only). Confirmed the new route code is actually compiled in (string markers permission_update_failed, invalid_rule_type, workspace_permissions, Malformed permission rule).
• Daemon: qwen serve --hostname 127.0.0.1 --port 4178 --workspace <tmp> with QWEN_SERVER_TOKEN, isolated QWEN_HOME, running in tmux. qwenCodeVersion=0.19.0, mode=http-bridge.
• Node v22.

Results

Live examples:

POST /workspace/permissions {scope:workspace, ruleType:deny,
     rules:[" Read(.env) ","Read(.env)","Bash(rm *)"]}
  → 200, workspace.deny = ["Read(.env)","Bash(rm *)"]      # trimmed + deduped
  settings.json → {"permissions":{"deny":["Read(.env)","Bash(rm *)"]}}

# user (global) scope also persists to ~/.qwen/settings.json  ✅
# with a live ACP child, the write is routed through the child (preferred path)  ✅

⚠️ One robustness issue confirmed at runtime

A read‑modify‑write cycle is blocked by a pre‑existing malformed rule. GET returns rule strings verbatim (readPermissionRuleSet keeps them), but every write re‑validates the entire list and returns 400 invalid_rule on the first malformed entry. So a client that reads → edits → writes — including this PR's own addWorkspacePermissionRule / removeWorkspacePermissionRule — cannot add or remove any rule once a malformed entry exists (e.g. unbalanced parens, which the pre‑PR setRules and the interactive /permissions flow accepted without parseRule validation).

Reproduced live against the daemon:

# settings.json already contains  allow: ["Bash(git log)", "Bash(rm "]   ("Bash(rm " is malformed)
GET  /workspace/permissions  → workspace.allow = ["Bash(git log)","Bash(rm "]   # returned verbatim
POST /workspace/permissions {scope:workspace, ruleType:allow,
     rules:["Bash(git log)","Bash(rm ","Bash(ls)"]}          # adding a VALID rule "Bash(ls)"
  → 400 {"error":"Malformed permission rule: Bash(rm","code":"invalid_rule"}
GET  → "Bash(ls)" was NOT persisted

And through the SDK helper directly (live daemon):

addWorkspacePermissionRule("workspace","allow","Bash(pwd)")  → THROWS invalid_rule   (blocked)
addWorkspacePermissionRule("workspace","ask",  "Bash(pwd)")  → OK                     (clean list — control)

Suggested fix: validate only the incoming delta, or drop invalid pre‑existing rules with a warning (as core's parseRules already does) rather than rejecting the whole request. Adding an SDK test for the actual append/replace path (the current add/remove tests only assert the no‑op branch) would catch this.

Verdict

The feature works as designed — capability gating, auth, validation, persistence, and both the fallback and live‑ACP‑child write paths are all correct. The round‑trip behavior above is the one robustness gap worth addressing (or explicitly accepting as out of scope) before merge.

POST /workspace/permissions {scope:workspace, ruleType:deny,
     rules:[" Read(.env) ","Read(.env)","Bash(rm *)"]}
  → 200, workspace.deny = ["Read(.env)","Bash(rm *)"]      # 已 trim + 去重
  settings.json → {"permissions":{"deny":["Read(.env)","Bash(rm *)"]}}

# user（全局）scope 同样落盘到 ~/.qwen/settings.json  ✅
# 当存在 live ACP child 时，写入会走 child（优先路径）  ✅

# settings.json 中已有  allow: ["Bash(git log)", "Bash(rm "]   （"Bash(rm " 非法）
GET  /workspace/permissions  → workspace.allow = ["Bash(git log)","Bash(rm "]   # 原样返回
POST /workspace/permissions {scope:workspace, ruleType:allow,
     rules:["Bash(git log)","Bash(rm ","Bash(ls)"]}          # 新增一条合法规则 "Bash(ls)"
  → 400 {"error":"Malformed permission rule: Bash(rm","code":"invalid_rule"}
GET  → "Bash(ls)" 未被持久化

addWorkspacePermissionRule("workspace","allow","Bash(pwd)")  → 抛出 invalid_rule   （被阻断）
addWorkspacePermissionRule("workspace","ask",  "Bash(pwd)")  → OK                   （干净列表——对照组）

🔬 Local build & verification report — PR #5743

Verified at PR head 639834f · macOS · Node v22.22.2 · fresh npm ci in an isolated git worktree (not a symlinked node_modules).

TL;DR

🔴 One blocking issue: the PR's own Reviewer Test Plan fails. acpAgent.test.ts › qwen/permissions/setRules rejects malformed permission rules fails because normalizePermissionRules() silently skips malformed rules (continue) instead of rejecting them. Everything else — route tests, SDK tests, build, typecheck, lint, and the real-daemon happy path — is green. A one-line fix makes all 657 CLI tests pass (verified).

What I ran (exactly the Reviewer Test Plan, plus a real-daemon e2e)

1) The failing test (will turn CI red)

FAIL  src/acp-integration/acpAgent.test.ts
  > qwen/permissions/setRules rejects malformed permission rules
  AssertionError: promise resolved "{ v: 1, user: { … } }" instead of rejecting
    .rejects.toThrowError(/Malformed permission rule/)   ← never thrown
    expect(settings.setValue).not.toHaveBeenCalled()     ← setValue IS called with []

Root cause — packages/cli/src/config/permission-settings.ts:

const rule = item.trim();
if (parseRule(rule).invalid) {
  continue;          // ← silently skips. The test expects a throw + no setValue().
}

The string "Malformed permission rule" appears only in the test, nowhere in the source — implementation and test have diverged. This test is new in this PR, and the CI Test (macos/ubuntu/windows) jobs are still pending, so they will go red on this.

2) Real-daemon e2e — the user-facing impact

I booted the actual built daemon (node packages/cli/dist/index.js serve --port 0 --token … --workspace …) and exercised the HTTP API directly:

• ✅ GET /capabilities advertises workspace_permissions
• ✅ GET /workspace/permissions returns correct scoped + merged + trust state
• ✅ POST trims & dedups — [" ShellTool(git status) ", "ShellTool(git status)"] → ["ShellTool(git status)"], written to settings.json
• ✅ POST {"scope":"system"} → 400 invalid_scope
• 🔴 POST {"scope":"user","ruleType":"deny","rules":["ShellTool(git status"]} (malformed) → HTTP 200, and the rule is silently dropped (deny stays []).

So a client — including one persisting a security-sensitive deny rule — gets a 200 OK while its rule silently disappears, with no error to act on. The REST route and the ACP method share the same normalizePermissionRules, so both swallow malformed rules.

3) Suggested fix (verified — 657/657 pass)

Reject malformed rules instead of skipping them:

if (parseRule(rule).invalid) {
  throw new PermissionRulesValidationError(
    `Malformed permission rule: ${rule}`,
    'invalid_rules',
  );
}

With this change all 657 CLI tests pass with no regressions: the REST route already maps PermissionRulesValidationError → 400 invalid_rules, and the ACP handler maps it → RequestError.invalidParams, so REST and ACP reject malformed rules consistently.

Alternatively, if silent-drop is intended, then acpAgent.test.ts must be updated to assert the 200/skip semantics — but that keeps the footgun of silently discarding a deny rule behind a 200.

Everything else looks solid

Clean refactor (extracting permission-settings.ts out of acpAgent.ts), good test coverage, documented non-atomic read-modify-write (TOCTOU) on the SDK add/remove helpers, and correct trust handling (untrusted workspace rules excluded from merged). Once the malformed-rule handling is reconciled, this is good to merge.

# isolated worktree at PR head + clean install
git worktree add --detach /tmp/pr5743 639834fcf6de4e97ca46d00ca923c7a7782b5e46
cd /tmp/pr5743 && npm ci

# Reviewer Test Plan
cd packages/cli && npx vitest run \
  src/serve/routes/workspace-permissions.test.ts \
  src/acp-integration/acpAgent.test.ts \
  src/serve/server.test.ts          # → 1 failed / 656 passed
cd ../sdk-typescript && npx vitest run test/unit/DaemonClient.test.ts  # → 171 passed
cd ../.. && npm run build && npm run typecheck   # → both pass

# real daemon
HOME=/tmp/h QWEN_HOME=/tmp/h/.qwen OPENAI_API_KEY=fake OPENAI_BASE_URL=http://127.0.0.1:9/v1 \
  OPENAI_MODEL=fake QWEN_MODEL=fake \
  node packages/cli/dist/index.js serve --port 0 --token T --hostname 127.0.0.1 --workspace /tmp/ws
# then: curl -H 'Authorization: Bearer T' http://127.0.0.1:<port>/workspace/permissions
#       curl -X POST … -d '{"scope":"user","ruleType":"deny","rules":["ShellTool(git status"]}'  → 200, rule dropped

FAIL  src/acp-integration/acpAgent.test.ts
  > qwen/permissions/setRules rejects malformed permission rules
  AssertionError: promise 被 resolve 了，而不是 reject
    .rejects.toThrowError(/Malformed permission rule/)   ← 从未抛出
    expect(settings.setValue).not.toHaveBeenCalled()     ← setValue 实际被以 [] 调用了

const rule = item.trim();
if (parseRule(rule).invalid) {
  continue;          // ← 静默跳过；但测试期望「抛错 + 不调用 setValue」。
}

if (parseRule(rule).invalid) {
  throw new PermissionRulesValidationError(
    `Malformed permission rule: ${rule}`,
    'invalid_rules',
  );
}

_{Verification performed locally by the maintainer against a real build; CI checks were still pending at the time of writing.}

Qwen Code review did not complete successfully: Qwen review timed out after 85 minutes. See workflow logs.

@qwen-code /triage

🔬 Local runtime verification — real qwen serve daemon (PR #5743 @ bddf771)

I built the real bundle from this PR's head and drove a live daemon end-to-end — raw HTTP (fetch + curl) and the built @qwen-code/sdk DaemonClient — on top of the unit suites.

TL;DR

The feature is solid: capability gating, auth, validation, persistence, trim/dedup, and the malformed-rule round-trip that was the earlier blocker are all green (17/19 automated E2E checks). But the real daemon surfaced one bug the unit tests mask:

🔴 Submitting a brand-new malformed rule returns 500 permission_update_failed instead of the intended 400 invalid_rules, and the actionable Malformed permission rule: … message is dropped. Root cause: getInvalidParamsMessage requires err instanceof Error, but the live ACP child returns the JSON-RPC error as a plain {code:-32602, message} object, so the guard never matches. A one-line fix restores the 400 + message — verified, all 19/19 green.

Environment

• Head bddf771fb836b31a972c0768f95a2cd36876b071, Linux, Node v22.22.2, fresh npm ci + npm run bundle in an isolated git worktree (real dist/cli.js + dist/chunks/serve-*.js; markers workspace/permissions, permission_update_failed, registerWorkspacePermissionsRoutes confirmed compiled in).
• qwen serve --hostname 127.0.0.1 --port 41743 --workspace <tmp> --no-web with QWEN_SERVER_TOKEN and an isolated QWEN_HOME, in tmux. Driven over HTTP and via new DaemonClient({ baseUrl, token }) from the built SDK index.mjs.

What works (verified live)

The two earlier blockers (read-modify-write through add/remove, and the PR's own acpAgent malformed-rule test) are genuinely resolved at this head. 👍

🔴 The one bug: new malformed rule → 500 instead of 400 (message lost)

Repro (pristine PR code), no session running:

POST /workspace/permissions {"scope":"workspace","ruleType":"allow","rules":["Bash(git *"]}
→ HTTP 500  {"error":"Failed to update permission rules","code":"permission_update_failed"}

The daemon writes the workspace through the live ACP child (the -32602 below comes from the child, proving that path is the active one — the 400-correct persist-fallback only runs when no child is reachable). Daemon stderr shows the message is already gone:

qwen serve: POST /workspace/permissions ACP error (key=permissions.allow, scope=workspace, …): [object Object]

Instrumented the caught error to get its exact shape:

typeof=object  isErr=false  ctor=Object  code=-32602
json={"code":-32602,"message":"Invalid params: Malformed permission rule: Bash(git *"}

So the bridge delivers a plain object that does carry code:-32602 and the real message — but workspace-permissions.ts:25-31 gates on err instanceof Error:

if (code === -32602 && err instanceof Error) return err.message; // plain object ⇒ false ⇒ message dropped

Because the object is not an Error instance, getInvalidParamsMessage returns undefined, the handler falls through to the generic 500 permission_update_failed, and String(err) logs [object Object].

Why the unit test misses it — workspace-permissions.test.ts:497 ("POST maps live ACP invalid params to invalid_rules") mocks the child error as Object.assign(new Error(...), { code: -32602 }), i.e. a real Error instance, which passes the instanceof guard. The real cross-process JSON-RPC error is a plain object, so the test is green while the daemon is not.

Impact

1. A client sending a malformed rule gets a 5xx (server fault / “retry”) for what is a 4xx client-input error — breaks client error-handling and retry logic.
2. The actionable Malformed permission rule: <rule> message is replaced by a generic string — the client can’t tell the user which rule is bad.
3. The daemon logs [object Object], hampering debugging.
4. Deterministic and on the primary (live-child) write path — not an edge case.

Verified fix (one line) — drop the instanceof Error requirement and read message off the object:

 function getInvalidParamsMessage(err: unknown): string | undefined {
   if (err && typeof err === 'object' && 'code' in err) {
     const code = (err as { code?: unknown }).code;
-    if (code === -32602 && err instanceof Error) return err.message;
+    const message = (err as { message?: unknown }).message;
+    if (code === -32602 && typeof message === 'string') return message;
   }
   return undefined;
 }

With this change the same request returns the intended result and the full E2E matrix is 19/19:

POST … {"rules":["Bash(git *"]}
→ HTTP 400  {"error":"Invalid params: Malformed permission rule: Bash(git *","code":"invalid_rules"}

(Optional polish: the Invalid params: prefix from RequestError.invalidParams leaks into the HTTP body — worth stripping for a clean client message. A regression test should mock the child error as a plain {code:-32602, message} object, not an Error, so it actually covers this path.)

Verdict

Everything except the malformed-new-rule error mapping is verified working on a real daemon, including the round-trip that was the prior blocker. I’d apply the one-line fix above (and tighten the test) before merge; nothing else blocking.

POST /workspace/permissions {"scope":"workspace","ruleType":"allow","rules":["Bash(git *"]}
→ HTTP 500  {"error":"Failed to update permission rules","code":"permission_update_failed"}

qwen serve: POST /workspace/permissions ACP error (…): [object Object]

typeof=object  isErr=false  ctor=Object  code=-32602
json={"code":-32602,"message":"Invalid params: Malformed permission rule: Bash(git *"}

if (code === -32602 && err instanceof Error) return err.message; // 普通对象 ⇒ false ⇒ message 被丢弃

 function getInvalidParamsMessage(err: unknown): string | undefined {
   if (err && typeof err === 'object' && 'code' in err) {
     const code = (err as { code?: unknown }).code;
-    if (code === -32602 && err instanceof Error) return err.message;
+    const message = (err as { message?: unknown }).message;
+    if (code === -32602 && typeof message === 'string') return message;
   }
   return undefined;
 }

POST … {"rules":["Bash(git *"]}
→ HTTP 400  {"error":"Invalid params: Malformed permission rule: Bash(git *","code":"invalid_rules"}

Qwen Code review did not complete successfully: Qwen review timed out after 85 minutes. See workflow logs.

@qwen-code /triage

✅ Local build + real-daemon verification — PR #5743

Verified at commit 333c2424 (fix(cli): report workspace permission write backend) on macOS (Darwin 25.5.0), Node v22.22.2 / npm 10.9.7. I built the whole workspace from source and exercised the new /workspace/permissions API against a real qwen serve daemon (the built binary, not mocks).

Results

Real-daemon E2E (built packages/cli/dist/index.js serve)

Spawned the actual daemon bound to throwaway workspaces and drove the HTTP API end-to-end:

• Capability — /capabilities advertises workspace_permissions.
• GET — returns { v:1, user, workspace, merged, isTrusted }.
• POST allow — response and on-disk .qwen/settings.json both updated; merged reflects the rule.
• Per-list replace — POST deny does not clobber the stored allow list.
• appliedVia (new in 333c2424) — a write with no live ACP child correctly reports appliedVia: "persist-fallback".
• Validation — scope!=workspace → invalid_scope; bad ruleType → invalid_rule_type; non-array / malformed rule → invalid_rules (all HTTP 400).
• Atomic reject — a rejected malformed POST leaves the stored rules intact.
• Malformed-rule preservation — a pre-existing malformed rule (Bash(rm ) round-trips successfully (preserved as the trimmed Bash(rm), while a brand-new malformed rule is still rejected with invalid_rules.

Notes (confirmed behavior, not defects)

• Write routes require a bearer token even on loopback. GET is auth-free on loopback, but POST /workspace/permissions returns 401 token_required unless the daemon is started with --token / QWEN_SERVER_TOKEN. This is the intended mutate-guard — worth surfacing in the SDK/docs so remote clients know writes need a token.
• Rules are normalized (trimmed) on round-trip, matching the route test's own expectation (Bash(rm → Bash(rm).

Verdict

Build, typecheck, the full test suites, the mutation check, and real end-to-end daemon behavior all pass at 333c2424. From a build-and-behavior standpoint this looks ready to merge. (This is a functional verification, not a design/security code review.) The PR currently shows BLOCKED — that appears to be merge-queue / CI / approval gating rather than a code problem.

— verified with claude-opus-4-8 · local build + real-daemon E2E (Node v22.22.2, macOS)