fix(ci): update the Qwen binary used by ECS runners - #8000
Merged
Conversation
Collaborator
yiliang114
enabled auto-merge
July 29, 2026 03:55
Collaborator
|
Thanks for the quick fix! Template looks good ✓ Problem: observed, not theoretical. The Singapore runner ( Direction: aligned. This is the project's own runner-update workflow, and the fix targets exactly the reported failure mode. No auth/sandbox/model/telemetry/public-contract surface is touched; the passwordless Size: not applicable — CI workflow plus its test, 12 additions / 16 deletions across 2 files. No core paths. Approach: the scope is right and minimal. Unsetting Risk: no elevated risk signals — neither changed file matches the revert-correlated paths. Moving on to code review. 🔍 中文说明感谢这个及时的修复！ 模板完整 ✓ 问题：是已观测到的问题，而非理论性的。新加坡 runner（ 方向：对齐。这是项目自身的 runner 更新 workflow，修复正好针对所报告的失败模式。未触及 auth/sandbox/model/telemetry/公共契约；其依赖的免密 规模：不适用——CI workflow 及其测试，2 个文件，12 行新增 / 16 行删除。未触及核心路径。 方案：范围恰当且最小化。在 风险：无升级风险信号——两个改动文件均未命中与 revert 相关的路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Collaborator
Code reviewReading just the title and the "why", my own fix would have been: stop carrying the runner user's custom npm prefix into the
The companion test is updated to assert the new workflow content rather than the old prefix-preserving block, so it stays a meaningful pin. No correctness, security, or regression concerns; nothing over-abstracted or out of scope. CI evidenceGreen on the reviewed commit — the Linux unit suite (which runs the updated
The runtime claim (the runner now resolves the system binary) is not something the CLI sandboxed lanes can settle — 中文说明代码审查只看标题和"为什么需要"，我自己的修复方案会是：不要再把 runner 用户的自定义 npm prefix 带进
配套测试已更新为断言新的 workflow 内容，而非旧的 prefix 保留逻辑，因此仍是有意义的约束。无正确性、安全或回归问题；无过度抽象或越界改动。 CI 证据在被审 commit 上为绿色——Linux 单元测试套件（运行更新后的 运行时声明（runner 现在解析到系统二进制）不是 CLI 沙箱通道能验证的—— — Qwen Code · qwen3.8-max-preview Reviewed at |
Collaborator
|
Confidence: 5/5 — clean across every stage; a tight, well-evidenced fix that does exactly one thing. This is the kind of PR that is easy to merge. The problem was real and observed (the Singapore runner verifying the stale binary), the fix is the direct one — stop forcing the install into the user's custom npm prefix — and the verify step now prints the resolved path so the next time something like this happens it diagnoses itself. The The PR is already merged (with maintainer approvals on this exact commit), so there is nothing left to gate — I am not adding a redundant approval on a shipped PR. Leaving this review as the record. ✅ 中文说明置信度：5/5——各阶段均干净；一个紧凑、证据充分、只做一件事的修复。 这是一个很容易合入的 PR。问题是真实且已观测到的（新加坡 runner 校验到旧版本二进制），修复也是直接的——不再强行把安装指向用户自定义 npm prefix——并且校验步骤现在会打印解析到的路径，下次再出现类似情况时即可自我诊断。 该 PR 已合入（在此 commit 上已有维护者批准），因此没有需要再把关的内容——我不会对一个已发布的 PR 追加多余的批准。仅留下此审查作为记录。✅ — Qwen Code · qwen3.8-max-preview Reviewed at |
chiga0
previously approved these changes
Jul 29, 2026
chiga0
left a comment
chiga0
left a comment
Collaborator
There was a problem hiding this comment.
Review — APPROVE
Reviewed at HEAD 6abb384. No Critical or Major issues. This is a clean, well-targeted CI fix that correctly addresses the root cause and adds a couple of security hardening measures along the way.
What I verified
Root cause is precisely targeted. The old code deliberately carried the runner user's prefix into the privileged install via sudo env "NPM_CONFIG_PREFIX=${global_prefix}" npm install -g, which on the Singapore host landed the package outside the runner service's PATH. The new sudo env -u NPM_CONFIG_PREFIX npm install -g unsets that variable so npm falls back to root's system-wide prefix — exactly the location the service resolves. The env -u is defensive regardless of sudoers env_keep/env_reset config (a no-op if sudo already stripped it, a fix if it didn't).
Security hardening, not just a fix. Two changes are strict improvements over the prior code: --registry=https://registry.npmjs.org pins the source (the old code honored whatever registry the env/.npmrc specified, a spoofing surface), and cd "${RUNNER_TEMP:?}" runs the global install from a neutral directory so the repo checkout's local .npmrc/package.json can't influence it. The :? guard fails fast if RUNNER_TEMP is unset, and the subshell keeps the cd from leaking into later steps.
Trust boundary of VERSION is safe. Although inputs.version / client_payload.version are operator-controlled, the value that reaches the install command is steps.version.outputs.version, which is the stdout of npm view "${specifier}" version — a validated published semver (the step exits 1 if npm view yields nothing). It's also double-quoted in the package spec, so no word-splitting or command injection. The workflow already scopes itself with permissions: contents: read and the github.repository == 'QwenLM/qwen-code' guard.
The verify step is a strong safety net. command -v qwen + "${qwen_path}" --version + test "${actual}" = "${VERSION}" means that even if the install landed somewhere unexpected, the workflow fails loudly rather than silently "succeeding" with the stale binary — which is precisely the original symptom. Logging qwen path: gives operators the visibility that was missing before. Under set -e, a missing qwen aborts at the command -v assignment.
Cross-Validation
No prior reviews or inline comments on this PR — this is the first review, so there is nothing to reconcile.
Minor observations (non-blocking)
- The fix assumes unsetting the
NPM_CONFIG_PREFIXenv var is sufficient — i.e., there's noprefix=key in root's npmrc (or in the runner user's~/.npmrcif sudo preservesHOME). This is empirically validated on the Singapore host per the PR description, and the verify step would catch any residual mismatch, so it's not a concern — just noting the assumption. - The change moves from "sudo only when the prefix isn't writable" to "always sudo." That's intentional and appropriate for a dedicated self-hosted runner whose service resolves the system prefix; it relies on the same passwordless sudo the updater already used.
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. |
Collaborator
Review —
|
| run | SG job |
|---|---|
| main 30418809177 | changed 13 packages → qwen version: 0.21.0 → exit 1 |
| this branch 30420458647 | qwen path: /usr/bin/qwen → qwen version: 0.21.1 ✅ |
Both matrix jobs are green on this branch (/usr/bin/qwen on SG, /usr/local/bin/qwen on 64c), and prettier --check passes on the file. One blocker below, then smaller items.
🔴 Blocker — the workflow guard test is not updated, test:ci will fail
scripts/tests/update-ecs-runner-qwen-workflow.test.js (added alongside #7689) pins the exact four strings this PR deletes. This PR touches only one file, so the test is left asserting the old shell.
Verified by running the real suite at this PR's head (6abb384):
$ npx vitest run --config ./scripts/tests/vitest.config.ts update-ecs-runner-qwen-workflow
❯ scripts/tests/update-ecs-runner-qwen-workflow.test.js:17:22
17| expect(workflow).toContain('global_prefix="$(npm prefix -g)"');
Test Files 1 failed (1)
All four assertions fail at head and all four pass on main. This is reached in CI via npm run test:ci → npm run test:scripts.
Suggest updating it to guard the new invariant rather than deleting the coverage:
it('installs into the system npm prefix used by the runner service', () => {
expect(workflow).toContain('sudo env -u NPM_CONFIG_PREFIX npm install -g');
// Regression guard: never re-introduce the runner user's prefix (#7689).
expect(workflow).not.toContain('NPM_CONFIG_PREFIX=${global_prefix}');
});🟠 The new path logging is absent on exactly the failure it's meant to diagnose
The echo "qwen path: …" runs after the binary is invoked, and command -v prints nothing when it fails — so a missing/broken qwen produces a completely silent step. A/B on the two forms with the binary absent:
=== main form === bash: qwen_absent: command not found exit=127
=== PR form === (no output at all) exit=1
So relative to main this is a small diagnostic regression, not an improvement. Moving one line fixes it:
set -euo pipefail
qwen_path="$(command -v qwen)" || {
echo "::error::qwen is not on the runner service PATH after install."
exit 1
}
echo "qwen path: ${qwen_path}"
actual="$("${qwen_path}" --version)"
echo "qwen version: ${actual}"
test "${actual}" = "${VERSION}"🟡 env -u NPM_CONFIG_PREFIX only unsets one of the two spellings
npm matches npm_config_* case-insensitively, so a lowercase npm_config_prefix survives the unset:
$ npm_config_prefix=/tmp/lower NPM_CONFIG_PREFIX=/tmp/UPPER env -u NPM_CONFIG_PREFIX npm prefix -g
/tmp/lower
Unlikely to be set on these hosts (and sudo's default env_reset already strips the uppercase one, so this flag is belt-and-braces either way) — but completing it costs one token: sudo env -u NPM_CONFIG_PREFIX -u npm_config_prefix npm install -g ….
🟡 Third flip of the same knob — worth recording the invariant
#7513 (always sudo → system prefix) → #7689 (preserve the runner user's prefix) → this PR (back to system prefix). The green run genuinely validates both hosts, but on ecs-qwen-runner-sg-22 / -64c-16, while the failing main run used sg-16 / 64c-1 — different members of the same pools, so the fix is proven for the code path, not for every machine behind the labels.
Two cheap things that would stop the next flip:
- The comment explains what not to do; it would help more to state the invariant it depends on, e.g. "root's npm prefix bin dir must be what the runner service PATH resolves first (
/usr/binon SG,/usr/local/binon 64c)". If a pool member is provisioned with a user-prefix-first PATH, this reverts to the fix(ci): update qwen in the runner's active npm prefix #7689 failure mode and the::error::above is what tells you so. - The PR body says "Not validated / out of scope: live runner execution is dispatched from this branch after PR creation" — it was validated. Please link run 30420458647 in the body so the next person doesn't have to rediscover it.
🟢 Nits / notes
cd "${RUNNER_TEMP:?}"is a good catch and worth keeping: this workflow has noactions/checkout, so the default cwd is the persistent self-hosted workspace, which can still hold a stale checkout whose.npmrcnpm would read. Pairing that escape with an explicit--registryis consistent withqwen-code-pr-review.yml:467andqwen-triage.yml:516.- The subshell
( … )aroundcd+ install is redundant — it's the last command in the step. - Minor inconsistency: the
Resolve versionstep still runsnpm viewfrom the workspace cwd without the registry pin, so version resolution and install can read different registry config. Same treatment there would make the two steps agree. - Security, FYI only: on 64c,
main's writable-prefix branch installed as the unprivileged runner user; the install now runs as root on both hosts. The published package declares no lifecycle scripts, so this is low risk, but transitive install scripts now execute as root on 64c too.
Verdict: the fix itself is right and demonstrated end-to-end. Blocking only on the stale guard test; the verify-step ordering is a one-line follow-up worth folding into the same push.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline. Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/update-ecs-runner-qwen.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
中文说明
已审查——无阻断问题。 建议见行内评论。 未检查（工具限制，非阻断）：the executable-script lint — .github/workflows/update-ecs-runner-qwen.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
— qwen3.8-max-preview via Qwen Code /review
Comment on lines
+17
to
+18
| expect(workflow).toContain('cd "${RUNNER_TEMP:?}"'); | ||
| expect(workflow).toContain('sudo env -u NPM_CONFIG_PREFIX npm install -g'); |
Collaborator
There was a problem hiding this comment.
[Suggestion] The updated guard test pins only the prefix-removal behaviour. The --registry=https://registry.npmjs.org pin this PR newly adds to the install command has no assertion, so removing it would not fail the test.
Failure scenario: a future edit drops --registry=https://registry.npmjs.org (e.g. switching to an internal mirror, or an accidental refactor); the test still passes because sudo env -u NPM_CONFIG_PREFIX npm install -g is a substring of the command regardless of the registry flag. The global install then resolves from whatever registry root's npm config points at — the supply-chain outcome the pin was added to prevent.
Suggested change
| expect(workflow).toContain('cd "${RUNNER_TEMP:?}"'); | |
| expect(workflow).toContain('sudo env -u NPM_CONFIG_PREFIX npm install -g'); | |
| expect(workflow).toContain('cd "${RUNNER_TEMP:?}"'); | |
| expect(workflow).toContain('sudo env -u NPM_CONFIG_PREFIX npm install -g'); | |
| expect(workflow).toContain('--registry=https://registry.npmjs.org'); |
中文说明
更新后的守护测试仅固定了「移除 prefix」这一行为。本 PR 新增加到安装命令中的 --registry=https://registry.npmjs.org 固定项没有对应断言，因此删除它不会让测试失败。
失败场景：未来某次修改删掉了 --registry=https://registry.npmjs.org（例如改用内部镜像，或重构时误删），测试仍会通过——因为无论是否带 registry 参数，sudo env -u NPM_CONFIG_PREFIX npm install -g 都是该命令的子串。于是全局安装会回退到 root 的 npm 配置所指向的任意 registry——正是此次固定 registry 所要避免的供应链风险。
— qwen3.8-max-preview via Qwen Code /review
Collaborator
Author
|
@qwen-code /triage |
wenshao
approved these changes
Jul 29, 2026
Collaborator
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. |
Collaborator
|
Released in v0.21.2. |
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
5 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
Installs the requested Qwen release into the system-wide npm location used by the self-hosted runner service, and logs the resolved executable during verification.
Why it's needed
The updater previously preserved a runner user's custom npm prefix while running sudo. On the Singapore host that installed the new package outside the service PATH, so verification continued to run the previous Qwen version.
Reviewer Test Plan
How to verify
Dispatch the ECS runner update workflow with a published version. Both matrix jobs should report the requested version, and the Singapore job should print the system qwen path.
Evidence (Before & After)
Before: the Singapore update job installed 0.21.1 but verification resolved 0.21.0. After: the workflow installs using root's system npm prefix and verifies the executable on the runner service PATH.
Tested on
Environment (optional)
GitHub Actions self-hosted Linux runner workflow.
Risk & Scope
Linked Issues
N/A
中文说明
此 PR 的内容
将指定的 Qwen 版本安装到自托管 runner 服务实际使用的系统级 npm 位置，并在校验阶段输出实际解析到的可执行文件路径。
背景
原更新流程在执行 sudo 时保留了 runner 用户的自定义 npm prefix。新加坡宿主机因此把新包安装到了服务 PATH 之外的位置，校验仍执行旧版本 Qwen。
验证方式
手动触发 ECS runner 更新工作流并指定一个已发布版本。两个矩阵任务都应输出目标版本，新加坡任务还应输出系统 qwen 路径。
证据
修复前：新加坡任务安装了 0.21.1，但校验解析到 0.21.0。修复后：工作流使用 root 的系统 npm prefix 安装，并校验 runner 服务 PATH 中的可执行文件。
测试环境
Linux：已通过 Prettier YAML 检查；macOS 和 Windows：不适用。
风险与范围
主要风险是依赖更新工作流原本已有的免密 sudo 能力。分支创建后将实际触发一次 runner 更新验证；无破坏性变更或迁移要求。