Skip to content

fix(serve): stop cdp-mcp-command reading process.env directly - #6562

Merged
yiliang114 merged 1 commit into
QwenLM:mainfrom
chinesepowered:fix/serve-cdp-env-guard
Jul 9, 2026
Merged

fix(serve): stop cdp-mcp-command reading process.env directly#6562
yiliang114 merged 1 commit into
QwenLM:mainfrom
chinesepowered:fix/serve-cdp-env-guard

Conversation

@chinesepowered

@chinesepowered chinesepowered commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Problem

packages/cli/src/serve/process-env-guard.test.ts is currently failing on main. The guard asserts that workspace-scoped serve/acp-bridge code never reads process.env directly (outside an explicit allowlist of boundary files). packages/cli/src/serve/cdp-mcp-command.ts — added in #6472 — reads process.env directly in two spots and was never added to the allowlist, so the guard flags it:

AssertionError: expected [ 'packages/cli/src/serve/cdp-mcp-command.ts' ] to deeply equal []

This turns the Test CI check red for every PR branched off current main.

Fix

Finish the dependency-injection the file was already set up for, so it reads no process.env at all:

  • resolveCdpMcpCommand(env) — drop the = process.env default; take env explicitly.
  • isBrowserAutomationMcpAvailable(opts, env) — take env explicitly and use it for both the QWEN_SERVE_ACP_HTTP check and the resolveCdpMcpCommand call.

env is supplied by the three callers, all of which are already in the guard's allowlist and legitimately hold process.env:

  • acp-http/index.ts
  • serve/run-qwen-serve.ts
  • serve/server/serve-features.ts

Behavior

Unchanged — every process.env lookup resolves to exactly the same value as before; it's just threaded through a parameter. No allowlist entry needed, because cdp-mcp-command.ts no longer touches process.env.

Related issue

Fixes #6554.

cdp-mcp-command.ts read process.env directly — via a `= process.env` default on resolveCdpMcpCommand and a direct read in isBrowserAutomationMcpAvailable — which trips the serve process.env guard test (process-env-guard.test.ts) and fails CI on main.

Thread env through both helpers instead: they now take env explicitly, supplied by the already-allowlisted boundary callers (acp-http, run-qwen-serve, serve-features). Behavior is unchanged.
@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template: PR body uses custom headings (Problem, Fix, Behavior) instead of the template's required headings — notably missing Reviewer Test Plan, Risk & Scope, and Linked Issues. The substance is excellent though, so noting this for future PRs rather than blocking.

Problem: Observed and verified ✅ — process-env-guard.test.ts is failing on main, flagging cdp-mcp-command.ts for direct process.env reads. I reproduced the failure locally.

Direction: Clear fix — completing the dependency injection that #6472 started. No scope or direction concerns.

Size: Not applicable (no core module paths touched).

Approach: Minimal and focused — removes the process.env default from resolveCdpMcpCommand, threads env explicitly through isBrowserAutomationMcpAvailable, updates the 3 callers (all already in the guard's allowlist). No scope creep.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板: PR 正文使用了自定义标题(ProblemFixBehavior),缺少模板要求的 Reviewer Test PlanRisk & ScopeLinked Issues 等部分。不过内容质量很高,仅作提醒,不阻塞。

问题: 已观察并验证 ✅ — process-env-guard.test.tsmain 上失败,标记了 cdp-mcp-command.ts 的直接 process.env 读取。本地已复现。

方向: 清晰的修复 — 完成 #6472 开始的依赖注入工作。无范围或方向问题。

规模: 不适用(未触及核心模块路径)。

方案: 最小化且聚焦 — 移除 resolveCdpMcpCommandprocess.env 默认值,将 env 显式传入 isBrowserAutomationMcpAvailable,更新 3 个调用方(均已在 guard 白名单中)。无范围蔓延。

进入代码审查 🔍

Qwen Code · qwen3.7-max

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Code Review

Clean, correct implementation. resolveCdpMcpCommand(env) drops the = process.env default; isBrowserAutomationMcpAvailable(opts, env) takes env explicitly for both the QWEN_SERVE_ACP_HTTP check and the resolveCdpMcpCommand call. All three callers (acp-http/index.ts, run-qwen-serve.ts, serve-features.ts) are already in the guard's allowlist — no new allowlist entries needed. The test in run-qwen-serve.test.ts is updated to match the new signature (passing {} as env). No issues found.

Before/After Testing

Before (main branch — guard test fails)

 ❯ src/serve/process-env-guard.test.ts (1 test | 1 failed) 20ms
   × serve process.env guard > keeps workspace-scoped serve and acp-bridge code off direct process.env reads 19ms
     → expected [ Array(1) ] to deeply equal []

 FAIL  src/serve/process-env-guard.test.ts > serve process.env guard > keeps workspace-scoped serve and acp-bridge code off direct process.env reads
AssertionError: expected [ Array(1) ] to deeply equal []

- Expected
+ Received

- []
+ [
+   "packages/cli/src/serve/cdp-mcp-command.ts",
+ ]

 Test Files  1 failed (1)
      Tests  1 failed (1)

After (this PR — guard test passes, full suite green)

 ✓ src/serve/process-env-guard.test.ts (1 test) 12ms

 Test Files  1 passed (1)
      Tests  1 passed (1)

Full run-qwen-serve.test.ts suite (152 tests) with the PR applied:

 ✓ src/serve/run-qwen-serve.test.ts (152 tests) 8152ms

 Test Files  1 passed (1)
      Tests  152 passed (152)
中文说明

代码审查

实现干净、正确。resolveCdpMcpCommand(env) 移除了 = process.env 默认值;isBrowserAutomationMcpAvailable(opts, env) 显式接收 env 参数。所有三个调用方均已在 guard 白名单中,无需新增白名单条目。测试已更新以匹配新签名。未发现问题。

前后对比测试

  • 修复前: guard 测试失败,标记 cdp-mcp-command.ts
  • 修复后: guard 测试通过,完整 serve 测试套件(152 个测试)全部通过

Qwen Code · qwen3.7-max

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

This is a clean, minimal CI fix. The guard test was broken on main by #6472 leaving two process.env reads in cdp-mcp-command.ts, and this PR finishes the dependency injection that file was already set up for. The diff is exactly what I'd have written — remove the default, thread env through, update callers. No surprises in the test results: guard goes red→green, 152 serve tests stay green.

One note for the author: the PR body didn't follow the repo's PR template (custom headings instead of the required sections). Not blocking here since the description quality is high, but worth using the template for future contributions.

Approving. ✅

中文说明

这是一个干净、最小化的 CI 修复。guard 测试因 #6472cdp-mcp-command.ts 中遗留了两个 process.env 读取而在 main 上失败,本 PR 完成了该文件已有的依赖注入设置。diff 正是我会写的方案 — 移除默认值、传入 env、更新调用方。测试结果无意外:guard 从红变绿,152 个 serve 测试保持绿色。

提醒作者:PR 正文未遵循仓库的 PR 模板(使用了自定义标题而非必需部分)。此处不阻塞,因为描述质量很高,但建议未来贡献时使用模板。

批准 ✅

Qwen Code · qwen3.7-max

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, looks ready to ship. ✅

@qwen-code-ci-bot qwen-code-ci-bot added category/cli Command line interface and interaction type/bug Something isn't working as expected labels Jul 9, 2026
@yiliang114
yiliang114 enabled auto-merge July 9, 2026 03:26

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed — no blockers. Suggestion-level recommendations are in the Suggestion summary comment below.

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Suggestions — commit faecee7

  • packages/cli/src/serve/run-qwen-serve.test.ts:1519 — The only unit test for isBrowserAutomationMcpAvailable passes {} as env with token: 'secret-token', which short-circuits before any env-dependent logic runs. Consider adding positive-path and QWEN_SERVE_ACP_HTTP=0 negative-path tests that exercise the new env parameter directly.

— qwen3.7-max via Qwen Code /review

@yiliang114 yiliang114 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the env-threading change and the related serve/acp-http call paths. No issues found.

Local verification on faecee7:

  • npm -w packages/cli run lint
  • npm run build
  • npm -w packages/cli run typecheck
  • npm -w packages/cli exec vitest run src/serve/process-env-guard.test.ts
  • npm -w packages/cli exec -- vitest run src/serve/run-qwen-serve.test.ts -t "normalizes browser MCP env flag|auto-enables only the CDP tunnel|advertises browser automation MCP|does not advertise browser automation MCP|does not enable browser automation MCP"
  • npm -w packages/cli exec -- vitest run src/serve/server.test.ts -t "advertises browser automation MCP only when the CDP adapter can connect"
  • npm -w packages/cli exec -- vitest run src/serve/acp-http/transport.test.ts -t "does not register chrome-devtools MCP|treats a whitespace-only CDP MCP command|dynamically registers chrome-devtools MCP|passes a custom CDP MCP command"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category/cli Command line interface and interaction type/bug Something isn't working as expected

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Release Failed for v0.19.8-nightly.20260709.e3a247f99 on 2026-07-09

3 participants