Gate browser automation MCP on external adapter - #6472
Conversation
|
Thanks for the PR! Template looks good ✓ Problem: This is a packaging/deployment improvement, not a user-facing bug. The motivation is credible: the main npm package previously bundled a browser automation MCP implementation and its transitive browser-control dependencies, which can trigger security scanner false-positives. The PR body documents Direction: Aligned. Making browser automation MCP opt-in via an explicit external adapter command is a clean separation of concerns — the Chrome extension can still frame the Web Shell and use the CDP tunnel without implying that browser automation tools are always available. The capability flag split ( Size: Core path only touches Approach: The scope is reasonable for the stated goal. Two observations:
Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ 问题: 这是一个打包/部署改进,不是用户可见的 bug。动机可信:主 npm 包之前打包了 browser automation MCP 实现及其传递的浏览器控制依赖,可能触发安全扫描误报。PR 正文记录了 方向: 对齐。通过显式外部 adapter 命令使 browser automation MCP 变成 opt-in,是干净的关注点分离。capability flag 拆分( 规模: 核心路径仅触及 方案: 范围对目标合理。两点观察:
进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code ReviewThe implementation is clean and follows the project's patterns well. New Capability registration — follows the Package preparation — the shift from urlValidator test URL swap — replacing Chrome extension sidepanel CSS — the full color scheme overhaul (warm charcoal → graphite + Qwen purple) is cosmetic and tangential to the PR goal, but minor. Test ResultsAll PR-relevant tests pass: Real-Scenario Testing (capabilities endpoint)Without
|
ReflectionThis PR does what it says: it separates the "CDP tunnel exists" signal from "browser automation MCP tools are available," and it makes the npm package cleaner by not bundling browser automation dependencies. The real-scenario test confirms the capability flag behavior matches the documentation — The code is straightforward. The new The one soft concern from Stage 1 stands: the sidepanel CSS color overhaul is a cosmetic drive-by that doesn't belong in this PR. It's minor enough not to block, but splitting cosmetic UI changes into their own PR keeps the diff focused and easier to revert. The manual Chrome extension E2E with a real external adapter remains untested — the PR acknowledges this. For the code that ships in this PR (capability flags + package guards), the coverage is adequate. The actual adapter integration is exercised by whatever adapter the operator provides, not by this package. Overall: clean implementation, solves a real deployment concern, tests verify the behavior. Ready to ship. 中文说明总结PR 实现了承诺:将"CDP 隧道存在"信号与"browser automation MCP 工具可用"分离,并通过不打包浏览器自动化依赖使 npm 包更干净。真实场景测试确认 capability flag 行为与文档一致。 代码简洁直接。新的 唯一的软关注点:sidepanel CSS 配色全面更新是一个与 PR 目标无关的装饰性改动。足够小不构成阻塞,但将装饰性 UI 变更拆到单独 PR 可以让 diff 更聚焦、更容易回退。 真实外部 adapter 下的完整 Chrome 扩展 E2E 仍未测试——PR 已承认。对于本 PR 交付的代码(capability flags + 包守卫),覆盖足够。 总体:实现干净,解决真实部署问题,测试验证行为。可以合并。 — Qwen Code · qwen3.7-max |
|
本轮自动处理了当前 review threads:
验证:新增 focused test 通过;相关文件 Prettier、 |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
The capability split between cdp_tunnel_over_ws and browser_automation_mcp correctly models the deployment reality, and the package scanning guards are a solid defense-in-depth measure. Build passes, all 1041 tests green.
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
— qwen3.7-max via Qwen Code /review
Suggestions — commit
|
| File | Issue | Suggested fix |
|---|---|---|
scripts/prepare-package.js |
collectFiles calls fs.statSync on every file, then assertPreparedPackageSize calls fs.statSync again on the same files — double stat syscalls |
Have collectFiles store [path, size] pairs so assertPreparedPackageSize sums sizes without re-statting |
scripts/prepare-package.js:332-342 |
No test verifies that the scanner intentionally skips binary files — a forbidden literal in .node/.png ships undetected |
Add a test with a non-matching extension containing a forbidden literal, asserting preparePackage succeeds |
scripts/prepare-package.js:171 |
verifyNativeAudioCapturePackage implies read-only but starts with fs.rmSync(addonDest, { recursive: true, force: true }) — destructive operation hidden behind a "verify" name |
Rename to cleanAndVerifyNativeAudioCapturePackage or split into cleanStaleAudioCaptureArtifacts() + verifyNativeAudioCapturePackage() |
packages/cli/src/serve/acp-http/index.ts:112-120 |
QWEN_CDP_MCP_COMMAND passed as single command string with no argument splitting — /path/to/adapter --debug fails with confusing ENOENT |
Document bare-path requirement or split on first whitespace to separate command from leading args |
packages/cli/src/serve/run-qwen-serve.test.ts:1125-1143 |
readBrowserMcpFeatureFlagsForEnv saves/restores QWEN_SERVE_CDP_TUNNEL_OVER_WS and QWEN_CDP_MCP_COMMAND but not QWEN_SERVE_ACP_HTTP — integration test silently depends on it being unset |
Add QWEN_SERVE_ACP_HTTP to the save/restore list |
— qwen3.7-max via Qwen Code /review
Suggestions — commit
|
| File | Issue | Suggested fix |
|---|---|---|
scripts/prepare-package.js:345-357 |
listTextPackageFiles uses Dirent.isFile() which returns false for symlinks, while collectFiles (size check) and npm pack both follow symlinks via fs.statSync. A symlinked file containing a forbidden literal evades the scanner but still gets packaged. |
Use fs.statSync(entryPath).isFile() (follows symlinks) instead of entry.isFile(), or add an entry.isSymbolicLink() branch with a visited-set to prevent cycles. |
packages/cli/src/serve/run-qwen-serve.test.ts:1116-1175 |
readBrowserMcpFeatureFlagsForEnv saves/restores three env vars but not QWEN_SERVE_ACP_HTTP, which isBrowserAutomationMcpAvailable reads directly. A leaked QWEN_SERVE_ACP_HTTP='0' from a preceding test causes a silent false-negative. |
Add QWEN_SERVE_ACP_HTTP to the save/restore block and explicitly delete process.env['QWEN_SERVE_ACP_HTTP'] in positive tests. |
scripts/tests/package-assets.test.js:222,341,375,453,497 |
Multiple tests assert bundledDependencies is undefined with names like "omits bundledDependencies when audio-capture artifacts are missing." Since writeDistPackageJson now never emits bundledDependencies, these assertions are tautological and test names describe removed behavior. |
Update test names to "does not emit bundledDependencies" and add a comment: // Field removed in PR #6472; audio-capture now resolved via optionalDependencies. |
packages/cli/src/serve/cdp-mcp-command.ts:17-27 |
isBrowserAutomationMcpAvailable mixes frozen opts (cdpTunnelOverWs, token) with live process.env reads (QWEN_SERVE_ACP_HTTP, QWEN_CDP_MCP_COMMAND). The inconsistency makes the function harder to test and creates an undocumented coupling. |
Either resolve all inputs from opts at boot, or accept an optional env parameter and thread it through. At minimum, add a comment explaining the deliberate choice. |
packages/cli/src/serve/cdp-mcp-command.ts:22 |
Token-based suppression of browser_automation_mcp is silent — no stderr message when the !opts.token check fires. The acp-http/index.ts log covers a different path. |
Add a writeStderrLine when opts.token is truthy and QWEN_CDP_MCP_COMMAND is set, explaining bearer-token mode disables browser automation. |
scripts/prepare-package.js:331-344 |
assertNoSensitivePackageScanLiterals walks all of dist/ rather than only files npm would package. Stale dist/node_modules/ from prior builds could trigger false positives. |
Scope the scan to collectPreparedPackageFiles output, or exclude node_modules from listTextPackageFiles. |
scripts/prepare-package.js:334 |
contents.toLowerCase() creates a full copy of each file in memory. For large bundled JS files this doubles peak memory per file. |
Use case-insensitive regex (/literal/i) instead of lowercasing the entire contents. |
packages/cli/src/serve/cdp-mcp-command.ts:19 |
The !opts.token check lacks an inline comment explaining the WebSocket limitation (browsers cannot set Authorization headers on WS upgrades). A future maintainer could misinterpret it as MCP adapter auth. |
Add: // Browsers cannot set Authorization headers on WebSocket upgrades, so a configured token makes /cdp unreachable. |
— qwen3.7-max via Qwen Code /review
|
Closeout update:
|
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestion-level recommendations are in the Suggestion summary comment below.
wenshao
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestion-level recommendations are in the Suggestion summary comment below.
wenshao
left a comment
There was a problem hiding this comment.
No issues found. LGTM.
— GPT-5 via Qwen Code /review
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
What this PR does
This PR keeps the Chrome extension and
qwen serveCDP tunnel path available while making browser automation MCP opt-in through an explicit external adapter command. The serve capability list now separates the raw CDP tunnel from browser automation MCP availability, so clients can tell the difference between "the extension can bridge CDP" and "the daemon has a browser automation adapter to register".It also tightens the publish packaging path so the main npm package does not bundle an external browser automation MCP implementation or its transitive browser-control dependencies. The package preparation step now removes stale bundled native audio artifacts, avoids bundled dependencies in the generated package metadata, and fails if known scanner-sensitive browser adapter literals or the unpacked package-size budget regress.
The Chrome extension onboarding copy and side panel styling are updated to match that model:
qwen serve --allow-origin ...is enough for the panel and CDP tunnel framing, while browser automation tools requireQWEN_CDP_MCP_COMMAND=/path/to/adapter.Why it's needed
The previous "bundle the DevTools MCP server" direction made the browser tools easier to demo but caused the main Qwen Code package to carry code that security scanners can flag. Keeping that adapter outside the main package gives us a cleaner compliance boundary while preserving a path for power users and reviewers to test the same browser automation behavior explicitly.
This also avoids overstating the default Chrome extension capability. A default extension connection can frame the web shell and expose the CDP tunnel, but it should not advertise browser automation MCP tools unless the local daemon actually has an adapter command to run.
Reviewer Test Plan
How to verify
Run
qwen serve --allow-origin chrome-extension://<extension-id>withoutQWEN_CDP_MCP_COMMAND, connect the Chrome extension, and confirm the panel can reach the local daemon while serve capabilities exposecdp_tunnel_over_wsbut notbrowser_automation_mcp.Run the same serve command with
QWEN_CDP_MCP_COMMAND=/path/to/adapter, connect the extension, and confirm the capability list includesbrowser_automation_mcpand the daemon attempts browser automation MCP registration only after an active CDP bridge is available.Prepare the publish package and confirm the generated main package has no
dist/node_modules, no bundled dependencies, no scanner-sensitive browser adapter literals, and stays under the 80MiB unpacked-size budget.Evidence (Before & After)
Automated local evidence:
npm run buildpassed; focused serve/ACP/core tests passed; package preparation andnpm pack ./distpassed withbundled: [],size=22,676,092, andunpackedSize=83,006,699;rg -n "chrome-devtools-mcp|puppeteer-core|oastify\.com|webhook\.site|ngrok\.io|ngrok-free\.app" distreturned no matches.Manual Chrome extension before/after evidence is not attached in this draft; the remaining reviewer check is a real browser run with an external adapter command.
Tested on
Environment (optional)
macOS local worktree, Node.js/npm workspace build, focused Vitest runs, and local
npm packusingNPM_CONFIG_CACHE=/tmp/npm-cachebecause the default user npm cache contains root-owned files.Risk & Scope
qwen serve --allow-origin chrome-extension://<id>remains sufficient for the extension panel and CDP tunnel framing, but browser automation tools requireQWEN_CDP_MCP_COMMAND=/path/to/adapter.Linked Issues
Related to #5777 and #6085.
中文说明
What this PR does
这个 PR 保留 Chrome 扩展和
qwen serve的 CDP tunnel 路径,但把 browser automation MCP 改成必须通过显式外部 adapter command 才启用。serve capability 现在会区分原始 CDP tunnel 和 browser automation MCP 是否可用,因此客户端可以知道“扩展能桥接 CDP”和“daemon 真的有可注册的浏览器自动化 adapter”不是同一件事。同时,这个 PR 收紧发布打包路径,避免主 npm 包打进外部 browser automation MCP 实现或其传递的浏览器控制依赖。package preparation 现在会清理陈旧的 bundled native audio artifacts,生成的 package metadata 不再带 bundled dependencies,并在扫描敏感 browser adapter literal 或 unpacked package-size budget 回退时失败。
Chrome 扩展 onboarding 文案和 side panel 色系也按这个模型做了调整:
qwen serve --allow-origin ...足够用于 panel 和 CDP tunnel framing;如果要 browser automation tools,则需要QWEN_CDP_MCP_COMMAND=/path/to/adapter。Why it's needed
之前“把 DevTools MCP server 打进包里”的方向确实更容易 demo browser tools,但会让 Qwen Code 主包携带容易被安全扫描命中的代码。把 adapter 留在主包外,可以让合规边界更干净,同时仍保留 power user 和 reviewer 显式测试同一套 browser automation 行为的路径。
这也避免默认 Chrome 扩展能力被过度声明。默认扩展连接可以 frame web shell 并暴露 CDP tunnel,但除非本地 daemon 真的有 adapter command 可运行,否则不应该宣称自己具备 browser automation MCP tools。
Reviewer Test Plan
How to verify
不设置
QWEN_CDP_MCP_COMMAND,运行qwen serve --allow-origin chrome-extension://<extension-id>,连接 Chrome 扩展,确认 panel 能连到本地 daemon,同时 serve capabilities 里有cdp_tunnel_over_ws,但没有browser_automation_mcp。设置
QWEN_CDP_MCP_COMMAND=/path/to/adapter后用同样方式启动 serve 并连接扩展,确认 capability list 出现browser_automation_mcp,并且 daemon 只会在 active CDP bridge 可用后尝试注册 browser automation MCP。运行发布包准备流程,确认生成的主包没有
dist/node_modules、没有 bundled dependencies、没有 scanner-sensitive browser adapter literals,并且 unpacked size 仍低于 80MiB budget。Evidence (Before & After)
本地自动化证据:
npm run build通过;focused serve/ACP/core tests 通过;package preparation 和npm pack ./dist通过,结果里bundled: [],size=22,676,092,unpackedSize=83,006,699;rg -n "chrome-devtools-mcp|puppeteer-core|oastify\.com|webhook\.site|ngrok\.io|ngrok-free\.app" dist没有命中。这个 draft 里没有附真实 Chrome 扩展 before/after 证据;剩余 reviewer check 是带外部 adapter command 的真实浏览器运行。
Tested on
Environment (optional)
macOS 本地 worktree,Node.js/npm workspace build,focused Vitest runs,以及本地
npm pack。由于默认用户 npm cache 里存在 root-owned 文件,pack 时使用了NPM_CONFIG_CACHE=/tmp/npm-cache。Risk & Scope
qwen serve --allow-origin chrome-extension://<id>仍足够用于 extension panel 和 CDP tunnel framing,但 browser automation tools 需要QWEN_CDP_MCP_COMMAND=/path/to/adapter。Linked Issues
关联 #5777 和 #6085。