fix(ci): cover release integration regressions - #5994
Merged
Conversation
yiliang114
force-pushed
the
codex/release-docker-client-mcp-fix
branch
from
June 29, 2026 07:45
253b4aa to
cbba9c0
Compare
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
yiliang114
changed the title
yiliang114
marked this pull request as ready for review
June 29, 2026 09:06
qwen-code-ci-bot
requested changes
Jun 29, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Hey @yiliang114 — quick template issue before we dive into the review.
The PR body is missing a few required headings from the PR template:
## What this PR does— currently## Summary## Why it's needed— currently## Root Cause## Risk & Scope— missing entirely## Linked Issues— missing entirely
The content is all there (great verification section!), just needs the heading names aligned and the two missing sections added. Once that's fixed I'll continue the review.
中文说明
@yiliang114 — 模板格式小问题：PR 正文缺少几个必需标题。## Summary 应改为 ## What this PR does，## Root Cause 应改为 ## Why it's needed，另外缺少 ## Risk & Scope 和 ## Linked Issues 两个段落。内容本身很完整，调整标题后继续审查。
— Qwen Code · qwen3.7-max
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: CI still running.
— qwen3.7-max via Qwen Code /review
Collaborator
Author
|
@qwen-code /triage |
Collaborator
Code reviewIndependent proposal (before reading the diff). Given the problem description:
That's essentially what the PR does. The diff matches or exceeds my proposal — it also correctly removes the No issues found. All changes are necessary and minimal.
Test resultsThis is a CI/test-only change with no user-facing TUI impact. Terminal test output is the evidence. Regression test (new)Chrome extension build (updated scripts)Build succeeds without POSIX env prefix. The Integration test — QWEN_SANDBOX=0 (both tests should run)Both tests run and pass when sandbox is explicitly disabled. Integration test — QWEN_SANDBOX=docker (prompted-model test should skip)Skip logic works correctly: the prompted-model test (test 2) is skipped under 中文说明代码审查独立方案（读 diff 之前）。 根据问题描述：(1) 从 npm 脚本中移除 POSIX diff 与我的方案一致甚至更好——还正确地去掉了 未发现问题。 所有改动都是必要且最小化的。 测试结果这是纯 CI/测试修改，无用户可见的 TUI 变化。终端测试输出即为证据。
— Qwen Code · qwen3.7-max |
Collaborator
|
This is a clean, focused CI fix. The diff is small (3 files, +38/-3), every change is necessary, and the test results confirm it works as described. The two problems are real and well-diagnosed: POSIX env-prefix breaks Windows The Independent proposal matched the PR's approach. No simpler path exists. Approving. ✅ 中文说明这是一个干净、聚焦的 CI 修复。diff 很小（3 个文件，+38/-3），每处改动都是必要的，测试结果确认了行为与描述一致。 两个问题是真实且诊断准确的：POSIX 环境变量前缀在 Windows build 脚本中移除 独立方案与 PR 方案一致。不存在更简路径。批准。✅ — Qwen Code · qwen3.7-max |
Collaborator
|
Thanks for the PR! Template looks good ✓ On direction: CI stability fix for two release pipeline regressions — Windows On approach: Three focused changes across three files, each minimal for the stated goal:
Scope is tight. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 方向：修复 release CI 的两个回归问题——Windows 方案：三个文件各做一处聚焦的改动，范围紧凑：
范围紧凑。进入代码审查 🔍 — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
previously approved these changes
Jun 29, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
yiliang114
enabled auto-merge
June 29, 2026 09:53
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: CI still running.
— qwen3.7-max via Qwen Code /review
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Jun 29, 2026
wenshao
previously approved these changes
Jun 29, 2026
wenshao
reviewed
Jun 29, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed the three CI-regression fixes. The chrome-extension env-var removal is safe (all consumers — esbuild.background.config.js:19, sync-extension.js:26, dev-watch.js:14 — default to dist/extension). Three points below; the sandbox-skip one is the most actionable.
中文
审阅了三个 CI 回归修复。chrome-extension 去掉 EXTENSION_OUT_DIR= 是安全的（所有消费方都默认 dist/extension）。下面三点，sandbox 跳过那条最值得处理。
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: CI still running.
— qwen3.7-max via Qwen Code /review
wenshao
approved these changes
Jun 29, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed all 3 files across correctness, security, build/test, and undirected audits — no blocking issues.
What I verified
packages/chrome-extension/package.json— removing theEXTENSION_OUT_DIR=dist/extensionPOSIX env-prefixes is behavior-preserving (all three consumers already default todist/extension). Droppingnpm run clean &&is also safe: the build path still wipesdist/extensionviasync-extension.js(syncAll({ clean: !isWatch })) before esbuild writes into it. I rannpm run build --workspace=packages/chrome-extensionlocally and it passes.integration-tests/cli/qwen-serve-client-mcp.test.ts— the conditional skip targets only the prompt-driven (host-loopback) test; the Docker-safe discovery path keeps running, so the reverse client-MCP registration path stays covered. Bonus: the heuristic also excludes'0', making it slightly more precise than the sibling sandbox-gated tests.scripts/tests/chrome-extension-package.test.js— the new regression guard passes against the fixed script and correctly tolerates the portablecross-envform.
LGTM ✅
中文
从正确性、安全、构建/测试以及多角度审计审查了全部 3 个文件，没有发现阻塞性问题。
已核验
packages/chrome-extension/package.json— 移除EXTENSION_OUT_DIR=dist/extension这种 POSIX 环境变量前缀不改变行为（三个消费方本来就默认dist/extension）。去掉npm run clean &&也安全：构建路径仍会通过sync-extension.js（syncAll({ clean: !isWatch })）在 esbuild 写入前清空dist/extension。我本地跑了npm run build --workspace=packages/chrome-extension，通过。integration-tests/cli/qwen-serve-client-mcp.test.ts— 条件跳过只针对依赖 host-loopback 的 prompt-driven 测试；Docker 下安全的 discovery 路径仍会运行，reverse client-MCP 注册路径仍被覆盖。另外该启发式还排除了'0'，比其它 sandbox 门控的兄弟测试略更精确。scripts/tests/chrome-extension-package.test.js— 新增的回归守卫对修复后的脚本通过，并能正确放行可移植的cross-env写法。
LGTM ✅
— claude-opus-4-8 via Claude Code /qreview
Collaborator
Author
|
@qwen-code /triage |
yiliang114
enabled auto-merge
June 29, 2026 11:43
qwen-code-ci-bot
approved these changes
Jun 29, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
| "build:bg": "node scripts/sync-extension.js && node config/esbuild.background.config.js", | ||
| "build:bg:watch": "node scripts/sync-extension.js && node config/esbuild.background.config.js --watch", | ||
| "build": "EXTENSION_OUT_DIR=dist/extension npm run clean && EXTENSION_OUT_DIR=dist/extension node scripts/sync-extension.js && EXTENSION_OUT_DIR=dist/extension node config/esbuild.background.config.js --production", | ||
| "build": "node scripts/sync-extension.js && node config/esbuild.background.config.js --production", |
Collaborator
There was a problem hiding this comment.
[Suggestion] The build script silently drops npm run clean alongside the POSIX env-var fix. clean.sh has broader scope (removes native-host/dist/, chrome-extension.zip, .extension-id files, log files) than sync-extension.js's internal dist/extension/ cleaning. If intentional, consider documenting this in the PR description; if not, restore it:
Suggested change
| "build": "node scripts/sync-extension.js && node config/esbuild.background.config.js --production", | |
| "build": "npm run clean && node scripts/sync-extension.js && node config/esbuild.background.config.js --production", |
— qwen3.7-max via Qwen Code /review
| ); | ||
|
|
||
| expect(packageJson.scripts.build).not.toMatch( | ||
| /(?:^|\s&&\s)[A-Za-z_][A-Za-z0-9_]*=/, |
Collaborator
There was a problem hiding this comment.
[Suggestion] The regression guard only asserts scripts.build, but scripts.dev also had its POSIX env-var removed in this PR. A future contributor could reintroduce the pattern in dev undetected. Consider iterating over all scripts:
const posixEnvAssignment = /(?:^|\s&&\s)[A-Za-z_][A-Za-z0-9_]*=/;
for (const [name, script] of Object.entries(packageJson.scripts)) {
expect(script, `scripts.${name}`).not.toMatch(posixEnvAssignment);
}— qwen3.7-max via Qwen Code /review
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
This PR fixes two release CI regressions. The Chrome extension build lifecycle now uses the package's existing Node-based defaults instead of POSIX inline environment assignment, and the client-hosted MCP integration coverage now separates the Docker-safe discovery path from the prompt-driven fake-model path. The discovery test still runs under Docker, while the prompt-driven fake-model scenario is skipped only when a container sandbox is actually active.
It also adds a regression guard for the extension build script so the Windows-incompatible POSIX environment-prefix form does not come back.
Why it's needed
The Windows CI failure was caused by a POSIX inline environment assignment in an npm lifecycle script, which Windows
cmd.exedoes not understand during dependency installation.The Release Docker integration failure was caused by a test setup mismatch: the model-side fake server listens on host
127.0.0.1, but the sandboxed ACP child runs inside a container where that address points back to the container rather than the host. That made the prompt-driven test wait until timeout even though the client-hosted MCP discovery path itself can run in Docker.Reviewer Test Plan
How to verify
Reviewers should confirm that dependency installation and extension packaging no longer rely on POSIX shell environment assignment on Windows, that Release Docker CLI integration no longer times out in the prompt-driven client-MCP fake-model scenario, and that the Docker-safe discovery path still executes and discovers the client-hosted tool.
QWEN_SANDBOX=0should be treated as sandbox disabled and should still run the prompt-driven test.Evidence (Before & After)
Before: Windows CI failed during the extension build lifecycle because
EXTENSION_OUT_DIR=...was interpreted bycmd.exeas an unknown command. Release Docker integration timed out in the prompt-driven client-MCP fake-model test because the sandboxed child could not reach the host-loopback fake model server.After:
npx vitest run --config scripts/tests/vitest.config.ts scripts/tests/chrome-extension-package.test.jspassed.QWEN_SANDBOX=0 DEBUG_CLIENT_MCP=1 npx vitest run --root ./integration-tests cli/qwen-serve-client-mcp.test.tspassed with 2 tests run.DOCKER_HOST=unix:///Users/jinjing/.colima/default/docker.sock QWEN_SANDBOX=docker DEBUG_CLIENT_MCP=1 npx vitest run --root ./integration-tests cli/qwen-serve-client-mcp.test.tspassed with 1 passed / 1 skipped.npm run build --workspace=packages/chrome-extensionpassed.git diff --checkpassed.N/A for screenshots or recordings because this is CI/test coverage only.
Tested on
Environment (optional)
macOS host with Colima Docker, Node 22, and
DOCKER_HOST=unix:///Users/jinjing/.colima/default/docker.sockfor Docker sandbox verification.Risk & Scope
Linked Issues
Related CI runs:
中文说明
这个 PR 做了什么
这个 PR 修复两个 release CI 回归问题。Chrome extension 的构建生命周期现在使用包内已有的 Node 默认值，不再依赖 POSIX inline 环境变量赋值；client-hosted MCP 集成测试现在把 Docker 下安全可跑的 discovery 路径和依赖 fake model 的 prompt-driven 路径拆开。discovery 测试仍然会在 Docker 下运行；只有容器 sandbox 确实启用时，才跳过依赖 host loopback 的 prompt-driven fake-model 场景。
同时增加了一个 package script 回归测试，避免 Windows 不兼容的 POSIX env-prefix 写法之后又被加回来。
为什么需要
Windows CI 失败是因为 npm lifecycle script 里用了 POSIX inline 环境变量赋值，Windows
cmd.exe在依赖安装期间无法识别这种写法。Release Docker integration 失败是测试环境不匹配导致的：model 侧 fake server 监听 host 上的
127.0.0.1，但 sandboxed ACP child 运行在容器里，容器内的这个地址指向容器自身而不是 host。因此 prompt-driven 测试会一直等到超时，虽然 client-hosted MCP discovery 路径本身可以在 Docker 下运行。Reviewer Test Plan
如何验证
Reviewer 应确认依赖安装和 extension packaging 不再依赖 Windows 不支持的 POSIX shell 环境变量写法，确认 Release Docker CLI integration 不再卡在 prompt-driven client-MCP fake-model 场景，并确认 Docker 下安全的 discovery 路径仍会实际执行并发现 client-hosted tool。
QWEN_SANDBOX=0应被视为 sandbox disabled，并且仍应运行 prompt-driven 测试。Before/After 证据
Before：Windows CI 在 extension build lifecycle 阶段失败，因为
EXTENSION_OUT_DIR=...被cmd.exe当成未知命令。Release Docker integration 在 prompt-driven client-MCP fake-model 测试中超时，因为 sandboxed child 无法访问 host-loopback fake model server。After：
npx vitest run --config scripts/tests/vitest.config.ts scripts/tests/chrome-extension-package.test.js通过。QWEN_SANDBOX=0 DEBUG_CLIENT_MCP=1 npx vitest run --root ./integration-tests cli/qwen-serve-client-mcp.test.ts通过，2 个测试都实际运行。DOCKER_HOST=unix:///Users/jinjing/.colima/default/docker.sock QWEN_SANDBOX=docker DEBUG_CLIENT_MCP=1 npx vitest run --root ./integration-tests cli/qwen-serve-client-mcp.test.ts通过，结果为 1 passed / 1 skipped。npm run build --workspace=packages/chrome-extension通过。git diff --check通过。截图或录屏不适用，因为这是 CI/test coverage 修复。
Tested on
Environment
本地使用 macOS host、Colima Docker、Node 22，并通过
DOCKER_HOST=unix:///Users/jinjing/.colima/default/docker.sock执行 Docker sandbox 验证。风险与范围
关联问题
相关 CI runs：