fix(ci): back-port the checkout-heal wipe guard to the triage and serve-ab wipes - #9277
Conversation
…ve-ab wipes The "empty the workspace, keep the directory" idiom exists in three copies; only the review workflow's copy received the #9220 hardening (canonicalization, trailing-slash strip, RUNNER_WORKSPACE allowlist). Measured on main for #9265, the two triage guards let non-canonical spellings of the guarded roots through (/home/, /home/., //usr, /root/, /var/ all reached the rm), and serve-ab's wipe had no guard at all — even `/home` or an empty string arrived at `find … -exec rm -rf`. Port the reference guard to all three sites, keeping each site's exit contract: triage fails loud both before and after external code, serve-ab stays bare under the job's `-eo pipefail` so an unclearable workspace fails before either checkout builds on top of the leftovers. Pin each ported copy with its own tests: bad-path batteries under an rm recorder (the destructive primitive cannot fire under any edit), an allowlist-escaping `..` case gated on a GNU-realpath host probe (the lesson from 90fa6bb), a realpath-absent trailing-slash RUNNER_WORKSPACE case, and text pins on the ported layers. Every pin was mutation-verified red against a deletion of the layer it guards.
|
Re-run on the current head. Thanks for the PR!
Moving on to code review. 🔍 中文说明基于当前 head 的 re-run。感谢贡献!
进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
🩺 serve daemon A/BBuilt the PR base vs this PR head ✅ No response changes against the PR base across 4 scenario(s). — Qwen Code · serve A/B |
Code reviewThe port holds up on the current head, and the blocker from the previous pass is fixed. All three copies carry the reference guard's layers — canonicalize with One non-blocking observation: with the fail-closed realpath leg in place, the Testing evidence (this PR's own CI, fetched via API — no PR code executed here)The previously red
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。 Real-scenario (tmux) testing: N/A — CI-only change with no user-visible surface, and this is an unattended run. 中文说明代码审查:移植在当前 head 上站得住,上一轮的阻断项已修复。三份副本都带上了参考 guard 的各层—— 一个非阻断观察:在 realpath 失败即拒绝的分支就位后, 测试证据:引用 PR 自身 CI 的真实 check 名与结论(上方表格),未执行任何 PR 代码。此前变红的 ubuntu Test 门禁已绿;Serve A/B 在真实 ecs-qwen self-hosted runner 上运行,新 serve-ab guard 在本 PR 的 CI 中实际执行并通过。triage 副本的执行用例在 CI runner 上以 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — clean across every stage; the previous blocker is fixed and verified green, and the only leftovers are two non-blocking notes named below. Stepping back: my independent proposal for #9265 is exactly what landed — port the reference layers into the three weaker copies, keep each site's exit contract, pin each copy with its own mutation-resistant tests. The follow-up commits since the last triage pass closed every gap I raised: the routing-test pin is updated (the ubuntu gate is green again and the new vitest suites actually ran), the Windows exclusion narrowed back to just the new file, and the fallback hardening (fail-closed realpath, 中文说明置信度:4/5 —— 各阶段均干净;上一轮的阻断项已修复并验证变绿,只剩两条下述非阻断意见。 回头看:我对 #9265 的独立方案与最终落地完全一致——把参考实现的各层移植到三份较弱的副本,保留各站点退出契约,为每份副本配上抗变异的钉桩测试。上次 triage 之后的后续提交闭合了我提出的所有缺口:routing 测试钉桩已更新(ubuntu 门禁重新变绿,新 vitest 套件真正跑起来了)、Windows 排除收窄到只含新文件、fallback 加固(realpath 失败即拒绝、 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
@yiliang114 One blocker before this can merge: the serve-ab wipe has a second pin you didn't update — .github/scripts/ci-runner-routing.test.mjs (wipes the reused workspace before checking out PR code) still expects find "$GITHUB_WORKSPACE" -mindepth 1 -maxdepth 1 -exec rm -rf, so this commit fails the ubuntu gate's node --test helper step — before the unit suite runs, which is why the new vitest suites never executed on CI. One-line fix: point the pin at the new find "$WS" … form (details in my notes above). Everything else — the port, the exit contracts, the per-copy pinning tests — looks right to me; get CI green and this is in good shape.
|
Updated the PR body to the current template and pushed a test-only fix for the Serve A/B wipe smoke pin. Verified: |
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. |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "agent 1c": none — but note the grep_search tool appeared to skip .github/workflows entirely, so all workflow-side consumer sweeps were redone with fixed-string shell grep..
Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/qwen-triage.yml: actionlint embedded-shell source mapping is not yet supported — not linted; the executable-script lint — .github/workflows/serve-ab.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
中文说明
仅完成部分审查,审查缺口已披露。 建议见行内评论。
未审查:build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally。
未探索到全部深度(达到工具调用预算):"agent 1c":none — but note the grep_search tool appeared to skip .github/workflows entirely, so all workflow-side consumer sweeps were redone with fixed-string shell grep.。
未检查(工具限制,非阻断):the executable-script lint — .github/workflows/qwen-triage.yml: actionlint embedded-shell source mapping is not yet supported — not linted; the executable-script lint — .github/workflows/serve-ab.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
— qwen3.8-max via Qwen Code /review (v0.21.12)
|
Closeout from resolve-pr-comments: fixed the wipe guard fallback path by refusing |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI (merge-queue-only lane) and its suite did not run locally; the changed vitest.config.ts win32 exclusion is exercised only on that lane.
Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/qwen-triage.yml: actionlint embedded-shell source mapping is not yet supported — not linted; the executable-script lint — .github/workflows/serve-ab.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
中文说明
仅完成部分审查,审查缺口已披露。 建议见行内评论。
未审查:build-and-test — Test (windows-latest, Node 22.x) was skipped in CI (merge-queue-only lane) and its suite did not run locally; the changed vitest.config.ts win32 exclusion is exercised only on that lane。
未检查(工具限制,非阻断):the executable-script lint — .github/workflows/qwen-triage.yml: actionlint embedded-shell source mapping is not yet supported — not linted; the executable-script lint — .github/workflows/serve-ab.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
— qwen3.8-max via Qwen Code /review (v0.21.12)
|
Closeout from resolve-pr-comments: changed the copied wipe guards to fail closed when realpath cannot canonicalize the workspace paths, and updated the focused fallback tests. Verified: focused serve-ab/qwen-triage vitest for realpath-absent and allowlist-escape cases, Prettier, git diff --check. Declined the broader late-round guard-family/test-matrix expansions.\n\n |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- shared guard-test fixture/helper extraction across the four workflow suites — already reported in round 1 (comment 3792740505, scripts/tests/serve-ab-workflow.test.js:106) and explicitly declined by the author as out of this PR's closeout s…
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI (merge-queue-only lane) and its darwin-host run of the changed suites did not run locally (approximated by a BSD-realpath stub probe — see R3-1).
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI (merge-queue-only lane) and its suite did not run locally; the changed vitest.config.ts win32 exclusion is exercised only on that lane.
Not explored to full depth (tool budget reached): "agent 6b": trace actions/runner source to confirm RUNNER_WORKSPACE materialization and container path translation (literal-string searches of JobExtension.cs, ExecutionCon…; "agent 6b": full local run of scripts/tests/qwen-triage-workflow.test.js on Linux (Linux-green status inferred from the serve-ab suite's 8/8 run plus the targeted realpath-….
Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/qwen-triage.yml: actionlint embedded-shell source mapping is not yet supported — not linted; the executable-script lint — .github/workflows/serve-ab.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
中文说明
仅完成部分审查,审查缺口已披露。
本轮确认的 1 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。
未审查:build-and-test — Test (macos-latest, Node 22.x) was skipped in CI (merge-queue-only lane) and its darwin-host run of the changed suites did not run locally (approximated by a BSD-realpath stub probe — see R3-1)。
未审查:build-and-test — Test (windows-latest, Node 22.x) was skipped in CI (merge-queue-only lane) and its suite did not run locally; the changed vitest.config.ts win32 exclusion is exercised only on that lane。
未探索到全部深度(达到工具调用预算):"agent 6b":trace actions/runner source to confirm RUNNER_WORKSPACE materialization and container path translation (literal-string searches of JobExtension.cs, ExecutionCon…;"agent 6b":full local run of scripts/tests/qwen-triage-workflow.test.js on Linux (Linux-green status inferred from the serve-ab suite's 8/8 run plus the targeted realpath-…。
未检查(工具限制,非阻断):the executable-script lint — .github/workflows/qwen-triage.yml: actionlint embedded-shell source mapping is not yet supported — not linted; the executable-script lint — .github/workflows/serve-ab.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
— qwen3.8-max via Qwen Code /review (v0.21.13)
|
Closeout update:
|
|
@qwen-code /review --timeout=360 |
|
Qwen Code review request accepted. Review is queued in workflow run. |
|
@qwen-code /review --timeout=360 |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI (merge-queue-only lane) and its BSD-realpath run of the changed suites did not run locally (the skipIf gate's skip branch was approximated by probe semantics, not executed).
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI (merge-queue-only lane) and its suite did not run locally; the changed vitest.config.ts win32 exclusion is exercised only on that lane.
Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/qwen-triage.yml: actionlint embedded-shell source mapping is not yet supported — not linted; the executable-script lint — .github/workflows/serve-ab.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
Deferred under the convergence posture (round 4, not a blocker) — recorded, not requested in this round:
.github/workflows/serve-ab.yml:99 — [probe] R4-1: trailing-slash strip loops are dead under fail-closed realpath and their named tests pin the fail-closed exit — loop-deletion mutants ship every suite green while the reference suite catches….github/workflows/serve-ab.yml:101 — [probe] R4-2: the WS-side .. case arm is unreachable at runtime and unpinned by every suite — deleting all three arms (9 lines) ships every suite green
中文说明
仅完成部分审查,审查缺口已披露。 建议见行内评论。
未审查:build-and-test — Test (macos-latest, Node 22.x) was skipped in CI (merge-queue-only lane) and its BSD-realpath run of the changed suites did not run locally (the skipIf gate's skip branch was approximated by probe semantics, not executed)。
未审查:build-and-test — Test (windows-latest, Node 22.x) was skipped in CI (merge-queue-only lane) and its suite did not run locally; the changed vitest.config.ts win32 exclusion is exercised only on that lane。
未检查(工具限制,非阻断):the executable-script lint — .github/workflows/qwen-triage.yml: actionlint embedded-shell source mapping is not yet supported — not linted; the executable-script lint — .github/workflows/serve-ab.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
收敛姿态下延后(第 4 轮,非阻断)——已记录,本轮不要求修改:共 2 条(原文未翻译,列表见上方英文部分)。
— qwen3.8-max via Qwen Code /review (v0.21.13)
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI (merge-queue-only lane); its darwin-host run of the changed suites is approximated only by the hasGnuRealpath skipIf gating plus the round-3 BSD-realpath stub probe.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI (merge-queue-only lane); the changed vitest.config.ts win32 exclusion is exercised only on that lane.
Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/qwen-triage.yml: actionlint embedded-shell source mapping is not yet supported — not linted; the executable-script lint — .github/workflows/serve-ab.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
中文说明
仅完成部分审查,审查缺口已披露。 建议见行内评论。
未审查:build-and-test — Test (macos-latest, Node 22.x) was skipped in CI (merge-queue-only lane); its darwin-host run of the changed suites is approximated only by the hasGnuRealpath skipIf gating plus the round-3 BSD-realpath stub probe。
未审查:build-and-test — Test (windows-latest, Node 22.x) was skipped in CI (merge-queue-only lane); the changed vitest.config.ts win32 exclusion is exercised only on that lane。
未检查(工具限制,非阻断):the executable-script lint — .github/workflows/qwen-triage.yml: actionlint embedded-shell source mapping is not yet supported — not linted; the executable-script lint — .github/workflows/serve-ab.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
— qwen3.8-max via Qwen Code /review (v0.21.13)
- run the rewritten pre-run sweep battery under -e -o pipefail so a failing sweep can no longer report success (bare bash -c masked it) - pin the RWS '..' refusal and degenerate-root refusal text in all copies, and add RUNNER_WORKSPACE='/' exec cases to both copy suites - exercise both pre-run and post-run copies in the realpath-absent refusal test - replace the '..' escape vector with a symlink escape that only the realpath line can refuse, and correct the mutant-outcome comments - add the serve-ab wipe-before-checkouts ordering pin from the sister suite and a happy-path RWS canonicalization pin
|
Closeout from resolve-pr-comments (a241a3d, test-only):
Verified: both workflow test suites green (150/150), Prettier, |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
2 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- shared guard-test fixture/helper extraction across the four workflow suites — already reported in round 1 (comment 3792740505, scripts/tests/serve-ab-workflow.test.js:131) and explicitly declined by the author as out of this PR's closeout s…
- guard-family drift between the three backported copies and the reference guard (R2-3, .github/workflows/qwen-triage.yml:2579) — still stands but explicitly declined by the author across rounds 2-4 as a follow-up outside this backport
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI (merge-queue-only lane); its darwin-host run of the changed suites is approximated only by the hasGnuRealpath skipIf gating plus the round-3 BSD-realpath stub probe.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI (merge-queue-only lane); the changed vitest.config.ts win32 exclusion is exercised only on that lane.
Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/qwen-triage.yml: actionlint embedded-shell source mapping is not yet supported — not linted; the executable-script lint — .github/workflows/serve-ab.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
Deferred under the convergence posture (round 5, not a blocker) — recorded, not requested in this round:
.github/workflows/serve-ab.yml:101 (+3 locations) — [probe] WS-side '..' refusal arm has zero pinning in any suite — deleting all three arms ships every suite green (anchored on code unchanged since the previous round; re-surfacing of round…
中文说明
仅完成部分审查,审查缺口已披露。 建议见行内评论。
本轮确认的 2 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。
未审查:build-and-test — Test (macos-latest, Node 22.x) was skipped in CI (merge-queue-only lane); its darwin-host run of the changed suites is approximated only by the hasGnuRealpath skipIf gating plus the round-3 BSD-realpath stub probe。
未审查:build-and-test — Test (windows-latest, Node 22.x) was skipped in CI (merge-queue-only lane); the changed vitest.config.ts win32 exclusion is exercised only on that lane。
未检查(工具限制,非阻断):the executable-script lint — .github/workflows/qwen-triage.yml: actionlint embedded-shell source mapping is not yet supported — not linted; the executable-script lint — .github/workflows/serve-ab.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
收敛姿态下延后(第 5 轮,非阻断)——已记录,本轮不要求修改:共 1 条(原文未翻译,列表见上方英文部分)。
— qwen3.8-max via Qwen Code /review (v0.21.13)
The symlink-escape comments claimed that with the WS realpath line deleted, find reaches rm through the link target. GNU find's default -P mode does not descend symlink operands: the mutant passes every guard, wipes nothing, and exits 0, so only the non-zero-status assertion catches it — the rm-log assertion passes vacuously. Reword both twin comments (R5-1).
|
Patrol closeout (R5-1, both locations): verified by probe — GNU find's default |
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 372 passed · 0 failed · 372 total 中文 — 判定:✅ 通过 · 可合入(agent 判定)沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码(与 base 构建 A/B 对照、无 mock harness 断言、定向门禁)。仅作为评审证据,不构成评审、批准或 CI 检查。 脚本断言:372 通过 · 0 失败 · 372 总计 Verification reportPR 9277 deep verificationVerdict: 中文摘要
ScopeCentral claim — the three full-workspace wipe blocks (qwen-triage pre-run, qwen-triage post-run, serve-ab pre-checkout) now canonicalize, strip trailing slashes, refuse Secondary claims — (1) legitimate wipes still work and the accept side holds for the real runner env; (2) the new/updated suites pin every decision-making layer of every copy (green at head, red under mutation). A sibling sweep of every Central claim A/BMethod:
Witness: Exit contracts preserved: triage refusals exit 1 with Corrections
FindingsNone blocking. The two non-blocking observations above are recorded under Corrections. Everything the PR claims to close is closed, and the closed set is exactly the set of full-workspace wipes in the repo. Not covered
MethodologyEnvironment: the CI verify container itself ( Evidence imagesHarness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
Union-resolve the serve-ab wipe conflict with #9277: keep the ported checkout-heal path guard (canonicalize, strip, denylist, RUNNER_WORKSPACE allowlist) verbatim, then apply this PR's narrowed wipe — keep a real root .git, defang hooks/info/attributes, scrub local config to the qwen-triage.yml allowlist — against the guarded $WS. The guard's realpath canonicalization subsumes the PR's `find -H` (a symlinked root resolves before the allowlist match), so the find takes main's plain shape. Adapt the pins: ci-runner-routing keeps the structural checks and pins the guard marker plus the kept-.git tail line by line; the #9277 suite gets the new step name, a scoped bare-script pin (only the config scrub may carry `|| true`), a CWD pinned to the fixture so the scrub never touches the host checkout, and a happy path that proves the kept .git survives defanged.
Merge origin/main. #9277 landed while this PR was in review and back-ported the checkout-heal guard (fail-closed realpath canonicalization, `..` arms) to the same triage and serve-ab wipes this PR ports the heal chain to. Compose both: keep the heal layer for symlinked/non-directory workspaces, adopt the fail-closed canonicalization and `..` arms on top of it, and preserve each site's exit contract. Pin the composed behavior with both suites' batteries merged, and fix the review's R4-1/R4-2: canonicalize the fixture roots at creation (the lockFixture pattern) and gate the GNU-realpath-dependent assertions on the host probe, so the suites stay green on BSD userlands whose tmpdir is a symlink.
|
Released in v0.21.14. |




What this PR does
Ports the guarded workspace-wipe idiom from the review workflow checkout-heal path to the two triage wipes and the Serve A/B pre-checkout wipe. The change keeps the existing failure contracts: triage wipes still fail loud, and Serve A/B still fails the job if a reused self-hosted workspace cannot be cleared before checkout.
Why it's needed
Fixes #9265. The older copies only checked the raw workspace string or had no guard, so non-canonical root spellings could reach
rm -rfin a mangled runner environment. This is defense-in-depth for runner safety; the shared-script consolidation option remains out of scope for this PR.Reviewer Test Plan
How to verify
Review that the three workflow wipe blocks canonicalize the workspace, strip trailing slashes, deny known roots, require the path to live under
RUNNER_WORKSPACE, and preserve each workflow's existing fail-open or fail-loud behavior. The new and updated workflow tests should fail if one copied guard drifts from the others.Evidence (Before & After)
N/A — CI-only workflow hardening.
Tested on
Environment (optional)
Local evidence from the PR branch:
scripts/tests/serve-ab-workflow.test.js,scripts/tests/qwen-triage-workflow.test.js, script suite with known baseline failures, and Prettier. Automation follow-up also verifiednode --test .github/scripts/ci-runner-routing.test.mjsafter updating the routing smoke pin.Risk & Scope
Linked Issues
Fixes #9265.
中文说明
What this PR does
把 review workflow checkout-heal 路径里的受保护 workspace wipe 写法移植到两个 triage wipe 和 Serve A/B 的 checkout 前 wipe。改动保留现有失败语义:triage wipe 仍然失败即报错,Serve A/B 在无法清理复用的 self-hosted workspace 时仍然直接失败,避免在残留文件上构建。
Why it's needed
修复 #9265。旧副本只检查原始 workspace 字符串,或者完全没有 guard,因此在 runner 环境被污染时,非规范化的根目录写法可能进入
rm -rf。这是 runner 安全的防御性加固;抽取共享脚本不在本 PR 范围内。Reviewer Test Plan
How to verify
确认三个 workflow wipe 块都会规范化 workspace、剥离尾部斜杠、拒绝已知根目录、要求路径位于
RUNNER_WORKSPACE下,并保留各 workflow 既有的 fail-open 或 fail-loud 行为。新增和更新的 workflow 测试应能在任一 guard 副本漂移时失败。Evidence (Before & After)
N/A — 仅 CI workflow 加固,无用户可见界面变化。
Tested on
Environment (optional)
PR 分支已有本地证据:
scripts/tests/serve-ab-workflow.test.js、scripts/tests/qwen-triage-workflow.test.js、带已知 baseline failures 的 scripts suite、Prettier。本轮自动化补充验证了node --test .github/scripts/ci-runner-routing.test.mjs,并同步了 routing smoke pin。Risk & Scope
Linked Issues
Fixes #9265.