fix(ci): narrow serve-ab's self-hosted wipe to the A/B checkout dirs - #9228
fix(ci): narrow serve-ab's self-hosted wipe to the A/B checkout dirs#9228qwen-code-dev-bot wants to merge 3 commits into
Conversation
'Wipe stale workspace before checkout' deleted the whole shared workspace including the root .git, forcing the next job on that runner (e.g. a fetch-depth: 0 review job) to re-download the full ~900 MB of history from github.com. On the ECS pool's slow link that stalls checkouts for 20+ minutes and the fetches drop mid-pack often enough to read as hung runners (2026-08-15: 20 orphaned tmp_pack files, ~6 GB, across 10 runners; one checkout re-downloaded 890 MB in 19m45s). serve-ab only builds inside its own head/ and base/ checkouts and never reads the workspace root, so removing just those two dirs keeps the anti-bleed guarantee without destroying the shared object store. The ci-runner-routing pin now asserts the narrow scope and fails on a whole-workspace wipe regression.
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
@qwen-code-dev-bot the change itself looks sensible, but the PR body doesn't follow the PR template — none of the required sections are present:
What this PR does/Why it's needed(currently free-formWhat changes/Why)Reviewer Test PlanwithHow to verify,Evidence (Before & After), and theTested onmatrix (currently a free-formVerificationsection)Risk & Scope(currentlyNot in this PR)Linked Issues- the Chinese translation in a
<details>block
Your recent PRs (#9162, #9082) followed the template — please reformat this body into the same shape. The existing content carries over almost verbatim: the pool measurements and the 19m45s re-fetch data belong under How to verify, and the Not in this PR notes fit Risk & Scope. Once the body uses the template, re-running triage (@qwen-code /triage) will pick it up and continue to the code review.
中文说明
改动本身看起来合理，但 PR 描述没有遵循 PR 模板——所有必需章节都缺失：
What this PR does/Why it's needed（目前是自由格式的What changes/Why）Reviewer Test Plan，含How to verify、Evidence (Before & After)和Tested on矩阵（目前是自由格式的Verification）Risk & Scope（目前是Not in this PR）Linked Issues<details>中的中文翻译
你最近的 PR（#9162、#9082）都使用了模板——请把本 PR 的描述整理成相同格式。现有内容基本可以原样迁移：ECS 池的测量数据和 19 分 45 秒重新拉取的记录放在 How to verify 下，Not in this PR 的内容放进 Risk & Scope。描述符合模板后，重新运行 triage（@qwen-code /triage）即可继续代码审查。
— Qwen Code · qwen3.8-max
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. |
🩺 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 |
|
@qwen-code /takeover |
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。移除 |
qwen-code-ci-bot
left a comment
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/serve-ab.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
— qwen3.8-max via Qwen Code /review (v0.21.12)
| assert.match( | ||
| wipe.run, | ||
| /rm -rf "\$\{GITHUB_WORKSPACE:\?\}\/head" "\$\{GITHUB_WORKSPACE:\?\}\/base"/, | ||
| ); |
There was a problem hiding this comment.
[Suggestion] The wipe assertion hard-codes the directory names head/base instead of deriving them from the two actions/checkout steps' path: values, so drift between the wipe targets and the actual checkout directories passes green. — Failure scenario: a follow-up PR renames a checkout path (path: 'head' → 'pr-head') or adds a third checkout dir; the wipe then deletes two directories that no longer exist (rm -rf on missing paths exits 0 silently) while stale files accumulate in the real checkout directories on the persistent ECS pool — the next PR's A/B build picks up leftover code and posts a silently wrong A/B diff, the exact failure this step exists to prevent, while this suite stays green.
Witness (probe, real node --test run):
BASELINE: # pass 6 / # fail 0
MUTANT (path: 'head'→'pr-head', 'base'→'pr-base',
rm line untouched): # pass 6 / # fail 0 (mutant survives)
A derived-invariant probe flips both ways: clean tree exit 0; mutant exit 1 (checkout path 'pr-head' wiped: false).
| assert.match( | |
| wipe.run, | |
| /rm -rf "\$\{GITHUB_WORKSPACE:\?\}\/head" "\$\{GITHUB_WORKSPACE:\?\}\/base"/, | |
| ); | |
| // Derive the wipe targets from the checkout steps so the pin cannot | |
| // drift from the paths the checkouts actually use. | |
| const checkoutPaths = serveAbDoc.jobs.ab.steps | |
| .filter( | |
| (s) => | |
| String(s.uses || '').startsWith('actions/checkout') && | |
| s.with && | |
| s.with.path, | |
| ) | |
| .map((s) => s.with.path); | |
| assert.ok(checkoutPaths.length >= 2, 'expected at least two checkout paths'); | |
| for (const p of checkoutPaths) { | |
| const target = '"${GITHUB_WORKSPACE:?}/' + p + '"'; | |
| assert.ok( | |
| wipe.run.includes(target), | |
| 'checkout path ' + p + ' must be wiped before checkout', | |
| ); | |
| } |
— qwen3.8-max via Qwen Code /review (v0.21.12)
| // the narrow scope so it cannot regress. | ||
| assert.doesNotMatch(wipe.run, /-mindepth 1 -maxdepth 1 -exec rm -rf/); |
There was a problem hiding this comment.
[Suggestion] The negative assertion rejects only the literal old find … -mindepth 1 … command; a differently-worded whole-workspace wipe added alongside the pinned line passes both assertions, although the comment claims the narrow scope "cannot regress". — Failure scenario: a future disk-pressure/cleanup PR appends rm -rf "$GITHUB_WORKSPACE"/* to this step; wipe.run still matches the narrow-rm regex and not the -mindepth 1… regex, so the test stays green while the root .git is destroyed again — reintroducing the full-history re-fetch / hung-runner pathology this PR fixes.
Witness (probe, real node --test run):
MUTANT A (appended rm -rf "$GITHUB_WORKSPACE"/* after the pinned rm): # pass 6 / # fail 0
MUTANT B (narrow rm replaced with the exact old find form): # pass 5 / # fail 1
The realistic revert is caught (Mutant B), so the hole is confined to appended/re-worded variants; counting rm invocations closes it.
| // the narrow scope so it cannot regress. | |
| assert.doesNotMatch(wipe.run, /-mindepth 1 -maxdepth 1 -exec rm -rf/); | |
| // the narrow scope so it cannot regress. | |
| assert.doesNotMatch(wipe.run, /-mindepth 1 -maxdepth 1 -exec rm -rf/); | |
| assert.equal( | |
| (wipe.run.match(/\brm\b/g) ?? []).length, | |
| 1, | |
| 'wipe must contain exactly one rm invocation', | |
| ); |
— qwen3.8-max via Qwen Code /review (v0.21.12)
| assert.match( | ||
| wipe.run, | ||
| /rm -rf "\$\{GITHUB_WORKSPACE:\?\}\/head" "\$\{GITHUB_WORKSPACE:\?\}\/base"/, | ||
| ); |
There was a problem hiding this comment.
[Suggestion] The positive wipe pin is an unanchored substring match over the whole step script, so it matches the rm command even when it is a shell comment or the argument of a no-op — the guard cannot distinguish an executed wipe from a disabled one. — Failure scenario: a maintainer iterating on the hung-runner/disk-pressure pathology comments out the wipe (# rm -rf "${GITHUB_WORKSPACE:?}/head" …) or neuters it (echo rm -rf …); both assertions still pass. With the wipe disabled, the next serve-ab job checks out into the previous PR's head/: actions/checkout resets tracked files, but the previous PR's untracked build artifacts (packages/*/dist, node_modules) survive into the next npm run build and daemon drive, silently corrupting the posted A/B diff.
Witness (probe, real node --test run):
comment mutant: # pass 6 / # fail 0
echo mutant: # pass 6 / # fail 0
fix flip (comment-filtered, line-anchored pin): comment → # fail 1, echo → # fail 1,
unmutated workflow still # pass 6
This targets the same assert.match block as the hard-coded-names comment above — combine the two fixes. Filter comment lines and require the exact rm as an executed line:
const executedLines = wipe.run
.split('\n')
.map((l) => l.trim())
.filter((l) => l !== '' && !l.startsWith('#'));
assert.ok(
executedLines.includes(
'rm -rf "${GITHUB_WORKSPACE:?}/head" "${GITHUB_WORKSPACE:?}/base"',
),
'the wipe rm must be an executed (non-comment) line',
);— qwen3.8-max via Qwen Code /review (v0.21.12)
Address review suggestions: the wipe targets are now derived from the actions/checkout steps, and the wipe must be exactly one executed (non-comment) rm line covering exactly those paths. Renamed checkout paths, appended whole-workspace wipes, and commented-out or echo'd rms now all fail the suite, while the reverted find-form still does.
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/100 轮）。改动内容与我反驳保留之处如下： Autofix review round — PR #9228One commit: Feedback points and dispositions1. [CHANGES_REQUESTED] PR body does not follow the PR template — NOT ADDRESSED (outside this flow's authority)The request is legitimate: the body uses free-form sections instead of the template's 2. [rc:3789629589] Suggestion — wipe targets hard-coded instead of derived from the checkout steps — RESOLVEDClaim reproduced first (probe on the pre-fix code): renaming 3. [rc:3789629594] Suggestion — negative assertion only rejects the literal old
|
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
[Critical] R2-11: The PR body does not follow the PR template — the open triage CHANGES_REQUESTED (review 4943936576) still stands at the reviewed commit. The body uses free-form sections (## Why, ## What changes, ## Verification, ## Not in this PR) and none of the template's required sections are present: What this PR does, Why it's needed, Reviewer Test Plan (How to verify, Evidence (Before & After), Tested on matrix), Risk & Scope, Linked Issues, plus the Chinese
Details
translation. The autofix loop has already declared it cannot reformat the body (no GitHub write access in that mode), so this blocker persists until a maintainer reformats the body; the existing content carries over almost verbatim (pool measurements and the 19m45s re-fetch data underHow to verify, the Not in this PR notes under Risk & Scope).
— qwen3.8-max via Qwen Code /review (v0.21.12)
| .filter((l) => l !== '' && !l.startsWith('#') && /\brm\b/.test(l)); | ||
| assert.deepEqual(rmLines, [expectedRm]); |
There was a problem hiding this comment.
[Suggestion] The wipe pin only sees lines containing the word rm: rmLines is filtered with /\brm\b/ before assert.deepEqual(rmLines, [expectedRm]), so an executed wipe line that avoids the rm token is invisible to the guard whose comment claims the narrow scope "cannot regress". — Failure scenario: the wipe's run block gains a second executed line such as find "${GITHUB_WORKSPACE:?}" -mindepth 1 -delete alongside the pinned rm line → the line is dropped from rmLines and deepEqual still passes → a whole-workspace wipe (the exact "destroys the shared root .git → hung runner" regression this pin exists to prevent) ships with a green test.
Witness (probe): appended find "${GITHUB_WORKSPACE:?}" -mindepth 1 -delete after the pinned rm line → suite still pass 6 / fail 0; replacing the filter with the all-executed-lines pin below → the same mutated workflow fails (fail 1).
Note: the R2-4/R2-6/R2-7/R2-10 threads (and R2-2) touch this same assertion block and their fixes interact — they are best applied as one rework: filter to all non-empty, non-comment lines, pin the whole set exactly, and compare the rm command's targets as a sorted set.
const executedLines = wipe.run
.split('\n')
.map((l) => l.trim())
.filter((l) => l !== '' && !l.startsWith('#'));
assert.deepEqual(executedLines, ['set -uo pipefail', expectedRm]);— qwen3.8-max via Qwen Code /review (v0.21.12)
| const expectedRm = | ||
| 'rm -rf ' + | ||
| checkoutPaths.map((p) => '"${GITHUB_WORKSPACE:?}/' + p + '"').join(' '); |
There was a problem hiding this comment.
[Suggestion] expectedRm joins checkoutPaths in YAML document order, coupling the pin to the semantically-meaningless argument order of the hand-written rm line; the assertions this commit removed tolerated a step reorder, the new pin does not. — Failure scenario: a maintainer reorders the "Checkout PR head" and "Checkout the merge-base" steps in serve-ab.yml (e.g. base first for readability) and leaves the wipe command untouched → checkoutPaths becomes ['base', 'head'] and deepEqual fails against the unchanged, still-correct rm line — a spurious CI failure that reads like a scope regression.
Witness (probe): moved 'Checkout the merge-base' before 'Checkout PR head' (wipe untouched) → test fails, actual rm -rf "${GITHUB_WORKSPACE:?}/head" "${GITHUB_WORKSPACE:?}/base" vs expected rm -rf "${GITHUB_WORKSPACE:?}/base" "${GITHUB_WORKSPACE:?}/head"; an A/B arm restoring the deleted assertions verbatim passes 6/6 on the same reordered YAML.
Keep the exactly-one-rm-line pin but compare target sets:
assert.equal(rmLines.length, 1, 'exactly one executed rm line');
const actualTargets = rmLines[0].replace(/^rm -rf\s+/, '').split(' ');
const expectedTargets = checkoutPaths.map(
(p) => `"${GITHUB_WORKSPACE:?}/${p}"`,
);
assert.deepEqual([...actualTargets].sort(), expectedTargets.sort());— qwen3.8-max via Qwen Code /review (v0.21.12)
| assert.ok( | ||
| checkoutPaths.length >= 2, | ||
| 'expected at least two checkout paths', | ||
| ); |
There was a problem hiding this comment.
[Suggestion] The derivation filter silently drops any actions/checkout step lacking with.path; checkoutPaths.length >= 2 checks a lower bound, not that every checkout is covered by the wipe pin. — Failure scenario: a future edit adds a third checkout to jobs.ab without with.path (which checks out into the workspace root itself) → checkoutPaths still derives only head/base, the pin stays green, but stale files from a previous PR in that checkout's location are never wiped on the reused self-hosted runner — the cross-PR A/B-diff bleed this test exists to prevent, now invisible. (A path-bearing third checkout, by contrast, already fails today.)
Witness (probe): added a third actions/checkout step with with: but no path → suite pass 6 / fail 0; checkoutPaths still ['head', 'base'].
| assert.ok( | |
| checkoutPaths.length >= 2, | |
| 'expected at least two checkout paths', | |
| ); | |
| assert.ok( | |
| checkoutPaths.length >= 2, | |
| 'expected at least two checkout paths', | |
| ); | |
| assert.equal( | |
| checkoutPaths.length, | |
| serveAbDoc.jobs.ab.steps.filter((s) => | |
| String(s.uses || '').startsWith('actions/checkout'), | |
| ).length, | |
| 'every checkout must declare a with.path the wipe can target', | |
| ); |
— qwen3.8-max via Qwen Code /review (v0.21.12)
| const rmLines = wipe.run | ||
| .split('\n') | ||
| .map((l) => l.trim()) | ||
| .filter((l) => l !== '' && !l.startsWith('#') && /\brm\b/.test(l)); | ||
| assert.deepEqual(rmLines, [expectedRm]); |
There was a problem hiding this comment.
[Suggestion] The pin proves the rm line is present AS TEXT, not that it executes as a wipe: control-flow or repositioning on lines lacking the rm token makes the pinned line inert while rmLines stays [expectedRm], contradicting the comment's claim of pinning "exactly one executed … rm line". — Failure scenario: any of (a) an early-exit guard line inserted before the rm line, (b) a trailing \ on the preceding executed line swallowing the rm line into echo's arguments, (c) the rm text moved into a never-executed heredoc — keeps the test green while the wipe deletes nothing on the persistent self-hosted pool, so the previous PR's head/base checkouts bleed into the next PR's A/B build.
Witness (probe): all three shapes → suite pass 6 / fail 0 while executing the real wipe script leaves both dirs intact (shape (b): stdout wiping rm -rf /tmp/wipe-probe-…/head …/base, head_exists=true base_exists=true); with the all-executed-lines pin: pristine 6/0, each shape 5/1.
Same fix as the R2-1 thread (pin the whole executed-line set — deepEqual(executedLines, ['set -uo pipefail', expectedRm])); each inert-line shape adds or alters a line outside the pinned set. Distinct defect from R2-1: the wipe NOT happening green, vs an extra wipe shipping green.
— qwen3.8-max via Qwen Code /review (v0.21.12)
| const checkoutPaths = serveAbDoc.jobs.ab.steps | ||
| .filter( |
There was a problem hiding this comment.
[Suggestion] The pin validates the wipe step's content (name, if, run text) but never its POSITION: nothing asserts the wipe precedes the checkout steps (as this test's own title, "before checking out PR code", claims) or follows 'Restore workspace ownership', on which it depends — the wipe runs without sudo precisely because ownership-restore runs first. — Failure scenario: a future serve-ab.yml edit moves 'Wipe stale workspace before checkout' below the checkouts (plausible while debugging the ownership interplay) → the wipe deletes the freshly checked-out head//base/ and every self-hosted serve-ab run fails in the build steps; or moves 'Restore workspace ownership' below the wipe → the sudo-less rm hits root-owned leftovers it cannot remove. Both keep this guard green because it never compares step indices.
Witness (probe): moved the wipe below both checkouts (parsed order: wipe index 4, checkouts 1 and 3) → suite pass 6 / fail 0; moved ownership-restore below the wipe → pass 6 / fail 0; with the ordering assertions below → fail 1 on both variants, 6/0 pristine.
Add at the end of this test:
const idx = (n) =>
serveAbDoc.jobs.ab.steps.findIndex((s) => s.name === n);
assert.ok(
idx('Restore workspace ownership') <
idx('Wipe stale workspace before checkout'),
);
assert.ok(
idx('Wipe stale workspace before checkout') < idx('Checkout PR head'),
);— qwen3.8-max via Qwen Code /review (v0.21.12)
| .split('\n') | ||
| .map((l) => l.trim()) |
There was a problem hiding this comment.
[Suggestion] Single-line/exact-string coupling: the pin requires the rm command to occupy exactly one physical line matching expectedRm byte-for-byte, so a continuation-wrapped or annotated rendering of the identical command fails spuriously. Distinct from the R2-2 thread (argument-order coupling) — that fix would not repair this. — Failure scenario: wrapping the rm line with backslash continuations (an ordinary reformat of a long line) makes rmLines ['rm -rf \\'] and the test fails with the wipe semantics unchanged; appending a trailing inline # keep the shared root .git fails likewise because the # filter only handles full-line comments — a confusing red CI on an innocent formatting edit.
Witness (probe): continuation-wrapped rm line → test fails (+ 'rm -rf "${GITHUB_WORKSPACE:?}/head" \\' vs the expected full line) while executing the real script still wipes both dirs (head_exists=false base_exists=false); the trailing-comment variant fails the same way.
Normalize before comparing — join backslash-continuations and strip trailing # comments, then compare the tokenized command (or its sorted target set) — and compose this with the whole-executed-line-set pin suggested in the R2-1/R2-4 threads, which would itself fail on a continuation wrap.
— qwen3.8-max via Qwen Code /review (v0.21.12)
There was a problem hiding this comment.
Declined — intentional strictness, not an oversight. This round's rework pins the wipe step's entire executed-line set (see the R2-1 thread), and that pin is deliberately exact: any change to the wipe script — a backslash continuation wrap or a trailing inline comment on the rm line included — must update the pin in the same diff. That friction is the point: an edit to this rm gets a reviewer's eyes on the workflow change and the pin update together. The failure mode is safe: an innocent reformat produces red CI (fixed by re-pinning in the same PR), never a green CI hiding a widened wipe. Adding continuation-joining plus trailing-comment stripping to the guard is shell lexing inside a regression test — complexity for the very scenario the guard exists to catch. Note the strictness is pre-existing: the prior byte-exact pin also failed on a continuation wrap (your witness confirms), so the rework introduces no new friction here.
中文说明
拒绝——这是有意的严格性，并非疏忽。本轮重构将 wipe 步骤的全部执行行集合作为固定对象（见 R2-1 线程），且该固定有意做到精确：wipe 脚本的任何改动——包括续行反斜杠换行或 rm 行尾的行内注释——都必须在同一 diff 中同步更新固定。这种摩擦正是目的所在：对该 rm 的修改会让审阅者同时看到工作流改动与固定更新。失败模式也是安全的：无辜的重排只会导致红色 CI（在同一 PR 中重新固定即可修复），绝不会出现放宽 wipe 却绿灯通过。在守卫中加入续行合并与行尾注释剥离，等于在回归测试里做 shell 词法解析——是为守卫本就要拦截的场景增加复杂度。另外，此严格性并非新增：此前的逐字节固定同样会在续行换行时失败（你的见证也证实了这一点），因此本轮重构没有引入新的摩擦。
| const rmLines = wipe.run | ||
| .split('\n') | ||
| .map((l) => l.trim()) | ||
| .filter((l) => l !== '' && !l.startsWith('#') && /\brm\b/.test(l)); |
There was a problem hiding this comment.
[Suggestion] The /\brm\b/ filter classifies ANY line merely mentioning the word rm as an rm command, so a harmless non-wipe line added to the step fails the pin — pass/fail hinges on a lexical coincidence unrelated to wipe scope (the false-positive mirror of the R2-1 thread, and not closed by its fix). — Failure scenario: adding one observability line echo "rm stale head/ + base/ checkout dirs" to the wipe step while leaving the actual wipe command byte-identical fails the test; the identical edit without the word 'rm' (echo "removing stale dirs") passes — same intent, opposite verdict.
Witness (probe): the echo-with-'rm' variant → deepEqual fails (two rm-matching lines); the identical edit without 'rm' → pass 6 / fail 0; with the command-position filter below → 6/6, and a broad-wipe replacement still fails.
Anchor the filter to command position instead of word occurrence:
.filter(
(l) =>
l !== '' &&
!l.startsWith('#') &&
/^(sudo\s+|env\s+\S+\s+|command\s+)*rm\b/.test(l),
)Tradeoff measured by the verifier: the anchored filter no longer flags an rm-bearing line in non-command position (find … -exec rm, xargs rm) added alongside the intact narrow rm; those shapes are covered if the whole-executed-line-set pin from the R2-1/R2-4 threads is adopted.
— qwen3.8-max via Qwen Code /review (v0.21.12)
There was a problem hiding this comment.
Declined as superseded by design. The /\brm\b/ word filter is gone entirely: the pin now compares the wipe step's whole executed-line set, so pass/fail no longer hinges on the lexical coincidence of the word rm — the filter defect you identified is removed. What we intentionally do not adopt is the tolerance your fix asks for: your scenario (add echo "rm stale head/ + base/ checkout dirs", expect green) must stay red, because the sibling finding R2-9 requires exactly that — a line after the rm can mask a failing rm (the script runs without set -e), so any added line, harmless or not, fails the pin until a human consciously re-pins it. Your own tradeoff note agrees: the anchored filter misses the find -exec rm / xargs rm shapes, and those are covered only by the whole-executed-line-set pin adopted here. Both echo variants now get the same verdict (both red), so the inconsistent-verdict complaint is closed as well.
中文说明
拒绝——设计上已被取代。/\brm\b/ 词过滤已被完全移除：固定现在比较 wipe 步骤的全部执行行集合，因此通过与否不再取决于 rm 一词的词法巧合——你指出的过滤缺陷已经消除。我们有意不采纳的是你所要求的容忍度：你的场景（新增 echo "rm stale head/ + base/ checkout dirs" 并期望绿灯）必须保持红色，因为姊妹发现 R2-9 恰好要求如此——rm 之后的行可能掩盖失败的 rm（脚本未启用 set -e），所以任何新增行，无论是否无害，都必须让固定失败，直到人工有意识地重新固定。你自己的权衡备注也认同这一点：锚定过滤会漏掉 find -exec rm / xargs rm 形态，而这些只有本轮采纳的整体执行行集固定才能覆盖。两种 echo 变体现在得到相同判定（均为红色），判定不一致的问题也一并消除。
| .split('\n') | ||
| .map((l) => l.trim()) | ||
| .filter((l) => l !== '' && !l.startsWith('#') && /\brm\b/.test(l)); | ||
| assert.deepEqual(rmLines, [expectedRm]); |
There was a problem hiding this comment.
[Suggestion] The pin inspects only the wipe step's run text and if condition; it never asserts the step lacks continue-on-error: true, so a wipe that FAILS at runtime no longer blocks the job and this guard stays green. — Failure scenario: a future edit adds continue-on-error: true to the wipe step — plausible in this very file, where the base checkout already carries it and the comments preach best-effort degradation. Later, when the rm fails (e.g. root-owned leftovers after an ownership-restore regression — the situation the R2-5 thread describes, which today fails the job loudly), the step turns yellow, the job proceeds, and stale head/base files bleed into the next PR's A/B diff with nothing red in CI.
Witness (probe): added continue-on-error: true to the wipe step → suite pass 6 / fail 0; with the assertion below → fail 1 on the mutated workflow, 6/0 pristine.
| assert.deepEqual(rmLines, [expectedRm]); | |
| assert.deepEqual(rmLines, [expectedRm]); | |
| assert.notEqual( | |
| wipe['continue-on-error'], | |
| true, | |
| 'a failed wipe must fail the job, not silently bleed into the next PR', | |
| ); |
— qwen3.8-max via Qwen Code /review (v0.21.12)
| const rmLines = wipe.run | ||
| .split('\n') |
There was a problem hiding this comment.
[Suggestion] The pin extracts only rm-token lines and never pins that the rm line is the step's LAST command (or that set -e is present): the wipe script runs set -uo pipefail WITHOUT -e (serve-ab.yml), so the script's exit status is the last command's, and a trailing line after the rm masks a failing rm. — Failure scenario: a maintainer appends one harmless trailing line after the rm line — e.g. echo "::notice::wipe complete", a common Actions idiom. Later, rm -rf fails at runtime — realistic on this pool, where the best-effort ownership-restore can leave root-owned leftovers and the sudo-less rm gets Permission denied. The trailing echo masks the failure (exit 0), the job continues on an un-wiped workspace, and stale head/base files bleed into the next PR's A/B builds — pin and job both green. Sibling of the R2-8 thread but distinct: it requires no step-level flag, and R2-8's assertion does not close it.
Witness (probe): appended echo "::notice::wipe complete" after the rm line → suite pass 6 / fail 0; with a last-executed-line assert → fail 1 on the mutated workflow, 6/0 pristine. Bash runs under set -uo pipefail: failing rm as last command → exit=1; with the trailing echo → exit=0; with -e → exit=1.
Pin the rm line as the final executed command:
assert.equal(
executed.at(-1),
expectedRm,
'a failed rm must fail the step',
);(subsumed as a side effect if the whole-executed-line-set pin from the R2-1/R2-4 threads is adopted).
— qwen3.8-max via Qwen Code /review (v0.21.12)
|
🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 2/100 轮）。改动内容与我反驳保留之处如下： Implemented 7 of the 9 inline suggestions as one rework of the serve-ab wipe pin; declined 2 with reasons (replies on their threads). The Critical (R2-11, PR body template) is not fixable from this mode — escalated at the end. What changed
Witness matrix — mutation probes applied to the real
Declined (replies posted on the threads)
Escalated to a maintainer — R2-11 (Critical): the PR body still does not follow the PR templateConfirmed at the reviewed commit: the body uses free-form sections ( ConflictNo conflict with VerificationCommands actually run this round:
中文说明9 条行内建议中实现了 7 条，合并为对 serve-ab wipe 固定（pin）的一次整体重构；其余 2 条附理由拒绝（已在对应线程回复）。Critical（R2-11，PR 正文模板）在本模式下无法修复——已在文末升级给维护者。 改动内容
见证矩阵——对真实
已拒绝（已在对应线程回复）
升级给维护者 —— R2-11（Critical）：PR 正文仍未遵循 PR 模板在被评审的提交上确认：正文使用自由章节（ 冲突与 验证本轮实际运行的命令：
🦷 Gate advisory — this round resolves a Critical/Request-changes finding with test-only changes (machine-measured): the bite check cannot verify a test-side fix, so the resolution rests on the round summary alone. · 本轮以纯测试改动解决 Critical/Request-changes 反馈（门自动测量）：bite 检查无法验证测试侧修复，该解决仅以轮次摘要为凭。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
Why
On the self-hosted ECS pool, the
Wipe stale workspace before checkoutstep deleted the entire shared workspace — including the root.git(~900 MB of history). The next job on that runner then had to re-download the full history from github.com:review-prjobs check out withfetch-depth: 0, so they pay the whole ~900 MB.Measured on 2026-08-15 across the pool:
.gitrecreated since Aug 11 (i.e. wiped + re-cloned).tmp_pack_*files (~6 GB) across 10 runners = fetches that died mid-download.fetch-depth: 0checkout re-downloaded 890 MB in 19m45s, dying once mid-pack (421 MBtmp_packleft behind).What changes
serve-abonly ever builds inside its ownhead/andbase/checkouts — no step reads the workspace root. So the wipe now removes exactly those two directories:rm -rf "${GITHUB_WORKSPACE:?}/head" "${GITHUB_WORKSPACE:?}/base"This keeps the anti-bleed guarantee (one PR's stale
head//base/can't contaminate the next A/B diff) without destroying the shared.gitthat every other job on the runner depends on. The${GITHUB_WORKSPACE:?}guard matches the defensive style used inqwen-triage.yml.The
ci-runner-routing.test.mjspin is updated accordingly: it now asserts the narrow scope and explicitly fails if the whole-workspace wipe regresses.Verification
node --test .github/scripts/ci-runner-routing.test.mjs— 6/6 pass locally.head,base) are exactly the twopath:targets of this job's checkouts;Restore workspace ownershipruns first, so leftovers of any ownership are removable.Not in this PR
qwen-triage.yml's before/after wipes of external-PR code deliberately remove a possibly-planted.git(deny-by-default security boundary) and are left untouched. Mitigating their re-fetch cost needs a different mechanism (e.g. a local object mirror or routing wipe-jobs to dedicated runners) and separate review.