ci: collapse PR checks into Ubuntu gate#5767
Merged
Merged
Conversation
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. |
The Ubuntu gate kept failing on the self-hosted runner: the PR checkout used github.ref (refs/pull/N/merge), which GitHub rebuilds asynchronously and can serve stale for minutes after a push, so the verify guard saw a tree without the PR head. The retry/refresh machinery added to work around this could not help — its direct GitHub fetch fallback times out on the self-hosted squid proxy. Check out refs/pull/N/head instead (immutable, published the instant the branch is pushed) for pull_request events in both the test and test_platforms jobs, and drop the 6-step retry/verify/refresh block. A single sanity guard stays to fail loud if the head is missing. Non-PR events keep github.ref; the merge queue validates the merged result.
Collaborator
|
Thanks for the PR! Template looks good ✓ On direction: this is a well-scoped CI optimization — collapsing three PR jobs into one Ubuntu gate to reduce runner queue time is directly aligned with keeping CI fast for contributors. The motivation (fork PRs can't use ECS, so every extra job competes for hosted capacity) is concrete and the fix is narrow. Resolves #5766 as the first step, with follow-ups explicitly deferred. Aligned. On approach: the scope feels right for the stated goal. The diff is focused — ci.yml consolidation, one integration test proxy fix, and updated guard tests. The checkout strategy change from merge ref ( One observation: with lint, tests, and no-AK smoke now serial in one job, a flaky lint step blocks the entire Ubuntu gate. The PR notes this as an accepted trade-off, and cross-platform signal still runs in parallel via Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 方向：这是一个范围合理的 CI 优化——把三个 PR job 合并到一个 Ubuntu gate 里，减少 runner 排队时间，与维护快速 CI 的目标直接对齐。动机具体（fork PR 不能跑 ECS，每多一个 job 都抢 hosted 容量），修复范围窄。作为 #5766 的第一步，后续跟进明确推迟。对齐。 方案：范围与目标匹配。diff 聚焦——ci.yml 合并、一处集成测试 proxy 修复、守护测试更新。checkout 策略从 merge ref 改为不可变的 head ref 是务实的取舍：避免自建 runner 过期缓存问题，但 required check 验证的是 PR head 而非合并结果。PR 已说明这一点，merge queue 仍然验证合并结果。 一个观察：lint、tests、no-AK smoke 现在在同一个 job 里顺序跑，lint 偶发失败会阻塞整个 Ubuntu gate。PR 将此作为可接受的取舍，跨平台信号仍然通过 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Contributor
There was a problem hiding this comment.
Pull request overview
Collapses PR CI execution into a single required Ubuntu gate job while keeping macOS/Windows unit test coverage as a separate non-required matrix, reducing PR job fan-out and duplicated setup work.
Changes:
- Consolidates lockfile/desktop isolation checks, lint/tooling, unit tests, coverage upload, and the PR-only no-AK integration smoke suite into the required Ubuntu
testjob. - Removes standalone
Lintandintegration_no_akjobs; introducestest_platformsfor macOS/Windows unit tests. - Updates CI wiring assertions and hardens integration tests against proxy env leakage (local loopback traffic).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
.github/workflows/ci.yml |
Reworks the workflow to make the Ubuntu job the primary required gate and moves previously separate PR checks into it; reshapes macOS/Windows runs into test_platforms. |
scripts/tests/no-ak-integration-ci.test.js |
Updates the CI wiring test to enforce “single no-AK invocation” and that it runs only inside the Ubuntu gate, plus verifies PR-head checkout behavior. |
integration-tests/cli/qwen-serve-streaming.test.ts |
Filters proxy env vars and sets NO_PROXY/QWEN_HOME to keep loopback integration tests stable in proxied CI environments. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+244
to
+246
| - name: 'Publish Test Report (for non-forks)' | ||
| if: |- | ||
| ${{ always() && needs.classify_pr.outputs.skip_ci != 'true' && steps.unit_tests.outcome != 'skipped' && (github.event.pull_request.head.repo.full_name == github.repository) }} |
Collaborator
Code ReviewThe workflow consolidation is clean. I reviewed the diff against my independent proposal (merge lint + no-AK into test job, remove standalone jobs, keep macOS/Windows as non-required matrix) and the PR matches or exceeds it — the additional checkout hardening (PR head ref + No correctness bugs or security issues found. The proxy env filtering in The guard tests in Test Resultsactionlint on ci.ymlno-AK integration guard tests (3/3 passed)no-AK integration smoke tests (36/36 passed)Real-Scenario TestingN/A — workflow-only change, no user-visible behavior to capture via tmux. The three test suites above (actionlint, guard tests, integration smoke) cover the changed code paths. 中文说明代码审查Workflow 合并干净。我将 diff 与独立方案（把 lint + no-AK 合并进 test job，移除独立 job，保留 macOS/Windows 为非 required matrix）对比，PR 达到或超过预期——额外的 checkout 加固（PR head ref + 未发现正确性 bug 或安全问题。
测试结果
真实场景测试N/A——纯 workflow 变更，无用户可见行为可通过 tmux 捕获。上述三个测试套件（actionlint、守护测试、集成冒烟）覆盖了变更的代码路径。 — Qwen Code · qwen3.7-max |
Collaborator
|
This PR does exactly what it says and does it well. Three CI jobs become one, the diff is focused, and every test suite passes. The checkout hardening (switching from the flaky merge ref to the immutable PR head ref, with a The proxy env fix in the streaming test is a small but important detail: the no-AK smoke now runs on ECS where system-wide proxy settings could silently break the fake server, and the fix is surgical (filter + explicit The serial-execution trade-off (lint → tests → no-AK in one job) is acknowledged and acceptable — a lint failure short-circuits fast, and cross-platform signal still runs in parallel. The guard tests are solid and will catch drift if someone later re-adds a standalone no-AK job or reintroduces the merge-ref machinery. Approving. ✅ 中文说明这个 PR 做到了它所承诺的一切。三个 CI job 合并为一个，diff 聚焦，所有测试套件通过。checkout 加固（从容易出问题的 merge ref 切换到不可变的 PR head ref，加上 streaming 测试中的 proxy 环境变量修复虽小但重要：no-AK smoke 现在跑在 ECS 上，系统级 proxy 设置可能悄悄破坏 fake server，修复精准（过滤 + 显式 顺序执行的取舍（lint → tests → no-AK 在一个 job 里）已说明且可接受——lint 失败快速短路，跨平台信号仍然并行运行。守护测试扎实，能在将来有人重新加回独立 no-AK job 或恢复 merge-ref 机制时捕获漂移。 批准。✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
approved these changes
Jun 23, 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. ✅
pomelo-nwu
approved these changes
Jun 23, 2026
| NO_COLOR: true | ||
| run: 'npm run test:ci' | ||
|
|
||
| - name: 'Run no-AK integration smoke tests' |
Collaborator
There was a problem hiding this comment.
[Suggestion] The no-AK integration step has no always() condition. If npm run test:ci fails, GitHub Actions skips this step silently — integration tests are dropped with no log output or annotation. The old integration_no_ak was a separate job, so it ran (and reported) independently regardless of unit test outcomes.
Suggested change
| - name: 'Run no-AK integration smoke tests' | |
| - name: 'Run no-AK integration smoke tests' | |
| if: "${{ always() && needs.classify_pr.outputs.skip_ci != 'true' && github.event_name == 'pull_request' && steps.unit_tests.outcome != 'skipped' }}" |
Using always() with steps.unit_tests.outcome != 'skipped' ensures integration runs even when unit tests fail, while still respecting the skip_ci and PR-only guards.
— qwen3.7-max via Qwen Code /review
| name: 'coverage-reports-22.x-ubuntu-latest' | ||
| path: 'packages/*/coverage' | ||
|
|
||
| test_platforms: |
Collaborator
There was a problem hiding this comment.
[Suggestion] The test_platforms job lost dorny/test-reporter and fork artifact upload steps that existed in the old per-platform matrix. macOS/Windows test failures will now only appear in raw logs — no JUnit annotations on the PR and no downloadable artifacts for fork PRs. This makes platform-specific failures harder to diagnose.
Consider adding a minimal dorny/test-reporter step here (matching the one in the test job), or explicitly document this as an intentional trade-off in a comment.
— qwen3.7-max via Qwen Code /review
| uses: 'actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd' # v6.0.2 | ||
| with: | ||
| ref: '${{ github.event.inputs.branch_ref || github.ref }}' | ||
| ref: "${{ github.event.inputs.branch_ref || (github.event_name == 'pull_request' && format('refs/pull/{0}/head', github.event.pull_request.number)) || github.ref }}" |
Collaborator
There was a problem hiding this comment.
[Suggestion] Switching to refs/pull/N/head validates the PR branch in isolation, not the merge result. The documented fallback (integration_cli on merge_group) is noted as inactive until merge queue is enabled on main (line ~420). Until then, required checks pass on a commit that is not what actually gets merged — a stale-base PR can go green and break main on merge.
Consider either (a) enabling merge queue before this lands, (b) adding a lightweight git merge --no-commit origin/main step after checkout to validate the merge result, or (c) adding a # WARNING comment here making the gap explicit for future maintainers.
— qwen3.7-max via Qwen Code /review
| stdio: ['ignore', 'pipe', 'pipe'], | ||
| env: { | ||
| ...process.env, | ||
| ...Object.fromEntries( |
Collaborator
There was a problem hiding this comment.
[Suggestion] This proxy-stripping fix is applied only to this test file. Other daemon-spawning tests (_daemon-harness.ts line ~122 uses env: { ...process.env, ...opts.env } unfiltered; qwen-serve-routes.test.ts also passes process.env through) would hit the same proxy-related flakes on runners with HTTP_PROXY/HTTPS_PROXY set.
Consider extracting this into a shared helper (e.g., integration-tests/_helpers.ts exporting a cleanEnv()) or centralizing it in _daemon-harness.ts's spawnDaemon() so all daemon subprocesses get consistent proxy isolation.
— 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
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
Collapses the default PR CI path into the existing required Ubuntu check,
Test (ubuntu-latest, Node 22.x). The Ubuntu gate now installs dependencies once and then runs the shared validation steps that previously lived in separate PR jobs: lockfile and desktop isolation checks, lint/tooling checks, unit tests, coverage upload, and (onpull_requestevents) the no-AK integration smoke suite.The standalone
Lintjob and theIntegration Tests (No-AK Smoke)job are removed — both now run as steps inside the Ubuntu gate. macOS and Windows still run on every PR, but as a separate, non-requiredtest_platformsmatrix rather than as legs of the requiredTestcheck.The Ubuntu gate's checkout is also hardened against the self-hosted runner serving a stale cache: it isolates the checkout's Git config and verifies that the checked-out tree actually contains the PR head before running anything, so a stale merge ref can no longer run pre-merge code.
Why it's needed
Fork PRs cannot safely use the ECS self-hosted runner pool, so each extra PR job competes for GitHub-hosted runner capacity. Recent CI runs showed long queue waits before jobs even started. This P0 shrinks the default PR fan-out by folding the separate
Lintand no-AK smoke jobs into the single Ubuntu gate, while keeping macOS/Windows cross-platform coverage and preserving the required Ubuntu merge-ref gate.This is the first, narrow step from #5766 and the broader CI optimization tracked in #5027. Follow-ups for CodeQL trigger changes, diff-only lint, and broader fake-server conversion stay out of this PR.
Reviewer Test Plan
How to verify
Open
.github/workflows/ci.ymland confirm:Test (ubuntu-latest, Node 22.x), now produced by the consolidatedtestjob rather than a matrix leg.Lintandintegration_no_akjobs are gone; their steps now run inside the Ubuntu gate.pull_request(github.event_name == 'pull_request'), and there is exactly onenpm run test:integration:no-ak:sandbox:noneinvocation in the workflow.test_platformsmatrix on every PR.scripts/tests/no-ak-integration-ci.test.jsenforces the no-AK wiring (single invocation, Ubuntu-gate placement, no model secrets), andactionlintpasses on the workflow.Evidence (Before & After)
N/A, workflow-only change.
Tested on
Environment (optional)
Local validation in a macOS worktree:
Note:
npm run test:integration:no-ak:sandbox:nonemust run outside the restricted sandbox because it starts listeners on127.0.0.1and writes the test QWEN home.Risk & Scope
test_platformsruns the unit suite on macOS/Windows in parallel. The no-AK smoke now executes inside the Ubuntu gate, which routes to the ECS self-hosted runner for in-repo PRs — watch the first runs for proxy /127.0.0.1listener issues.Test (ubuntu-latest, Node 22.x)is a required check; the removedLint/ no-AK / per-platform check names were never required.Linked Issues
Resolves #5766
Related: #5027, #5219, #5642, #5725, #5732, #5636, #5729
中文说明
这个 PR 做了什么
把默认 PR CI 收敛到现有 required Ubuntu check：
Test (ubuntu-latest, Node 22.x)。Ubuntu gate 现在只安装一次依赖，然后顺序运行原来分散在多个 PR job 里的共享检查：lockfile、desktop isolation、lint/tooling checks、unit tests、coverage upload，以及（仅在pull_request事件上）no-AK integration smoke。独立的
Lintjob 和Integration Tests (No-AK Smoke)job 被移除，二者现在作为 step 跑在 Ubuntu gate 内。macOS 和 Windows 仍然在每个 PR 上跑，但改成一个独立的、非 required 的test_platformsmatrix，而不再是 requiredTestcheck 的 matrix leg。Ubuntu gate 的 checkout 还做了加固，避免自建 runner 用过期缓存检出旧代码：隔离 checkout 的 Git 配置，并在跑任何步骤前校验检出的树确实包含 PR head，这样过期的 merge ref 不会再静默跑到 pre-merge 的代码。
为什么需要
fork PR 不能安全地跑到 ECS self-hosted runner，所以每多一个 PR job 都会抢 GitHub-hosted runner 容量。最近的 CI case 显示大量时间花在 job 开始前的 runner 排队上。这个 P0 通过把独立的
Lint和 no-AK smoke job 折进单个 Ubuntu gate 来收窄默认 PR fan-out，同时保留 macOS/Windows 跨平台覆盖，并保留 required Ubuntu merge-ref gate。这是 #5766 的第一步窄范围修复，也属于 #5027 追踪的整体 CI 优化。CodeQL 触发策略、diff-only lint、release integration fake server 化都留到后续 PR。
Reviewer Test Plan
如何验证
打开
.github/workflows/ci.yml确认：Test (ubuntu-latest, Node 22.x)，现在由合并后的testjob 产出，而不是 matrix leg。Lint和integration_no_akjob 已移除，其 step 现在跑在 Ubuntu gate 内。pull_request（github.event_name == 'pull_request'）上运行，且整个 workflow 里只有一处npm run test:integration:no-ak:sandbox:none。test_platformsmatrix 在每个 PR 上运行。scripts/tests/no-ak-integration-ci.test.js守护 no-AK 的接线（单次调用、放在 Ubuntu gate、无 model secrets），actionlint在该 workflow 上通过。证据
N/A，workflow-only change。
本地验证
在 macOS worktree 中运行：
npm run test:integration:no-ak:sandbox:none必须脱离受限 sandbox 运行，因为它会在127.0.0.1上监听并写测试 QWEN home。风险和范围
test_platforms在 macOS/Windows 上并行跑 unit suite。no-AK smoke 现在跑在 Ubuntu gate 内，对 in-repo PR 会落到 ECS self-hosted runner——头几次运行要盯一下代理 /127.0.0.1监听是否正常。Test (ubuntu-latest, Node 22.x)是 required check；被移除的Lint/ no-AK / 各平台 check 名称从来都不是 required。