fix(ci): narrow serve-ab's self-hosted wipe to the A/B checkout dirs - #9228
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
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 12 scenario(s). — Qwen Code · serve A/B |
|
@qwen-code /takeover |
qwen-code-dev-bot
added
the
autofix/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
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
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/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
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.
[Critical] R2-11 The PR body does not follow the PR template — still stands at the reviewed commit: free-form sections (## Why, ## What changes, ## Verification, ## Not in this PR); none of the template's required sections are present (What this PR does, Why it's needed, Reviewer Test Plan with How to verify / Evidence (Before & After) / Tested on matrix, Risk & Scope, Linked Issues, plus the Chinese
Details
translation). The open triage CHANGES_REQUESTED (review 4943936576) persists; the autofix loop has 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)
| assert.ok( | ||
| stepIndex('Restore workspace ownership') < wipeIndex, | ||
| 'the wipe depends on ownership-restore running first', | ||
| ); |
There was a problem hiding this comment.
[Suggestion] R3-1: The ordering assertion is vacuously true when the 'Restore workspace ownership' step is missing — stepIndex is findIndex, which returns -1 when the step is deleted or renamed, and -1 < wipeIndex always holds (wipeIndex is asserted !== -1 above). The test already applies the existence-check pattern to the wipe step itself (assert.ok(wipeIndex !== -1, …)) but not to this lookup. — Failure scenario: a future PR deletes or renames the 'Restore workspace ownership' step in serve-ab.yml → the test stays green, but on an ECS runner whose workspace holds root-owned leftovers the sudo-less rm -rf then fails with permission errors and serve-ab jobs fail on every reuse — the exact regression this assertion was written to pin. Witness (probe): with the step deleted, node --test .github/scripts/ci-runner-routing.test.mjs still reports # pass 6 / # fail 0; with the step deleted and the suggested fix applied → not ok … error: 'ownership-restore step must exist', # pass 5 / # fail 1.
| assert.ok( | |
| stepIndex('Restore workspace ownership') < wipeIndex, | |
| 'the wipe depends on ownership-restore running first', | |
| ); | |
| const ownershipIndex = stepIndex('Restore workspace ownership'); | |
| assert.ok( | |
| ownershipIndex !== -1, | |
| 'the wipe depends on the ownership-restore step existing', | |
| ); | |
| assert.ok( | |
| ownershipIndex < wipeIndex, | |
| 'the wipe depends on ownership-restore running first', | |
| ); |
— qwen3.8-max via Qwen Code /review (v0.21.12)
| # runner to re-fetch 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. | ||
| rm -rf "${GITHUB_WORKSPACE:?}/head" "${GITHUB_WORKSPACE:?}/base" |
There was a problem hiding this comment.
[Suggestion] R3-2: The step name (serve-ab.yml:74) — 'Wipe stale workspace before checkout' — now overstates what the step does: after this change it deliberately preserves everything except head/ and base/, which is the point of the PR. The added comment above this line says 'Only these two directories are removed', this PR's own test title says 'wipes only the reused A/B checkout dirs…', and sibling steps are named after their exact target (ci.yml: 'Clean stale .qwen before checkout'). — Failure scenario: a maintainer reading the step name in the Actions UI or logs assumes the entire workspace is cleaned between runs (e.g. that the shared root .git is destroyed or root-level leftovers are wiped — the exact opposite of the new behaviour) while debugging disk growth or cross-PR bleed on the ECS pool. Suggested fix: rename the step (e.g. 'Wipe stale A/B checkout dirs before checkout') and update the name pin at ci-runner-routing.test.mjs:194 in the same commit.
— qwen3.8-max via Qwen Code /review (v0.21.12)
|
🔀 Base updated: red check(s) [Test (ubuntu-latest, Node 22.x)] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base：红色检查 [Test (ubuntu-latest, Node 22.x)] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main，CI 将重新运行。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
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.
[Critical] R2-11 The PR body does not follow the PR template — still stands at the reviewed commit. The body uses free-form sections (## Why, ## What changes, ## Verification, ## Not in this PR); 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 open triage CHANGES_REQUESTED (review 4943936576) persists; the autofix loop has declared it cannot reformat the body (no GitHub write access in that mode), so this blocker persists until the body is reformatted — 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)
| # Only these two directories are removed: wiping the whole workspace | ||
| # also destroys the shared root .git, forcing the next job on this |
There was a problem hiding this comment.
[Suggestion] R4-2: Narrowing the wipe deliberately preserves the shared root .git across runs that execute untrusted PR code, and removes the incidental scrub hop that destroyed prior runs' planted workspace-root git exec vectors — the threat class the pool's own hygiene code documents (qwen-triage.yml 'Clean stale agent state' enumerates core.{hooksPath,fsmonitor,pager,editor,sshCommand}, filter.*.{clean,smudge,process}, include/includeIf, hooks, aliases and sweeps symlinked hooks; ci.yml guards only its pre-checkout git calls with GIT_SAFE and has no config-sanitize step). An approved fork PR routed to this pool runs serve-ab with lifecycle scripts enabled (no --ignore-scripts) after ownership-restore makes the workspace writable, so its npm ci can plant exec vectors into the preserved $GITHUB_WORKSPACE/.git; the plant now survives any number of serve-ab hops and can fire in a later job that runs git at the workspace root (e.g. another PR's ci.yml checkout, whose post-checkout hooks/fsmonitor/smudge run without the GIT_SAFE overrides, in a checks:write / pull-requests:write context). Bounded at Suggestion: the direct plant→next-job path already existed pre-diff whenever no serve-ab run intervened (the wipe ran at run start, never as post-run cleanup), and an attacker able to edit runs-on: could equally delete the wipe — so this removes one incidental defense-in-depth scrub rather than opening a new hole; the comment above presents the narrowing as purely a reliability win without naming this trade-off.
Suggested fix: keep the .git but strip the exec-vector class per the pool's existing playbook — e.g. a best-effort post-build step running qwen-triage's allowlist sweep on the root .git (git config --local --name-only --list | grep -ivE '<allowlist>' | xargs -r -n1 git config --local --unset-all plus deleting non-sample hooks). If the trade-off is accepted as-is, state that in this comment instead.
— qwen3.8-max via Qwen Code /review (v0.21.12)
| // removal may be added, and nothing may follow the rm — the script | ||
| // runs without `set -e`, so a trailing command would mask a failing | ||
| // rm. |
There was a problem hiding this comment.
[Suggestion] R4-3: This comment asserts a false runtime fact: GitHub Actions invokes bash steps as bash --noprofile --norc -e -o pipefail {0} — verified against actions/runner ScriptHandlerHelpers.cs (["bash"] = "--noprofile --norc -e -o pipefail {0}") and ADR 0277; serve-ab.yml sets defaults: run: shell: 'bash' with no step-level override, and the script's own set -uo pipefail adds -u without clearing -e. So -e IS in effect for the whole step — a trailing command cannot mask a failing rm. — Failure scenario: a future maintainer who knows Actions bash steps fail-fast recognizes the rationale as wrong and relaxes the whole executed.length === 2 pin, silently losing its genuinely load-bearing other half (no extra removals may be added — the guard against reintroducing the whole-workspace wipe). Suggested fix: reword to the true rationale.
| // removal may be added, and nothing may follow the rm — the script | |
| // runs without `set -e`, so a trailing command would mask a failing | |
| // rm. | |
| // removal may be added, and nothing may follow the rm. The runner | |
| // already invokes this step with -eo pipefail, but pin the executed | |
| // script anyway so any change forces a deliberate test update. |
— qwen3.8-max via Qwen Code /review (v0.21.12)
| const checkouts = steps.filter((s) => | ||
| String(s.uses || '').startsWith('actions/checkout'), | ||
| ); |
There was a problem hiding this comment.
[Suggestion] R4-4: The pin derives the wipe's protected set only from top-level steps whose uses starts with actions/checkout, so a checkout performed by another mechanism — a repo-local composite action (an established in-repo convention: 13 uses across ci.yml and windows-runner-smoke.yml) or a run: git clone step — silently leaves both the pin's coverage set and the actual wipe behind it. The checkoutPaths.length >= 2 guard catches migrating an existing checkout away from actions/checkout, not adding a new non-actions/checkout one. — Failure scenario: a maintainer adds a third checkout after the wipe (e.g. fetching a shared tooling repo into tools/ via run: git clone); every assertion still passes, the suite stays 6/6 green, but tools/ is never wiped — one PR's contents bleed into the next serve-ab run on the persistent ECS pool, the exact bleed this test exists to prevent. Witness (probe): BASE # pass 6 / # fail 0 · arm B (post-wipe run: git clone … tools): # pass 6 / # fail 0 — tools/ never wiped · arm A (same op via actions/checkout): fail 1 — 'the rm must target exactly the checkout dirs' · flip (probe fix + arm B): fail 1 — 'post-wipe git clone target tools is not covered by the wipe'. Suggested fix: assert positively that no post-wipe step creates workspace content the wipe does not cover — e.g. fail if any post-wipe run step clones into a directory absent from checkoutPaths, or allowlist post-wipe uses to known-safe actions (actions/checkout, actions/setup-node, actions/upload-artifact).
— qwen3.8-max via Qwen Code /review (v0.21.12)
#9228) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
🤖 Addressed the latest review feedback (round 3/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 3/100 轮）。改动内容与我反驳保留之处如下： Implemented all 5 inline suggestions as one batch; the Critical (R2-11, PR body template) remains escalated to a maintainer — it needs a GitHub write this mode does not have. What changed
Probe notesAll mutations were applied to scratch copies under Still open — R2-11 (Critical): PR body does not follow the PR templateCarried over from the round-2 escalation, unchanged: the body uses free-form sections ( ConflictNo conflict with VerificationCommands actually run this round:
中文说明以一次批量改动实现了全部 5 条行内建议；Critical（R2-11，PR 正文模板）继续升级给维护者——它需要本模式不具备的 GitHub 写入权限。 改动内容
探针说明所有变异都施加在 仍未解决 —— R2-11（Critical）：PR 正文未遵循 PR 模板沿用第 2 轮的升级，状态不变：正文使用自由章节（ 冲突与 验证本轮实际运行的命令：
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/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 4)": none — no check was cut short by the tool ceiling.; "agent 2": none.** (Tool calls used: 6.).
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.
[Critical] R2-11: The PR body does not follow the PR template — still stands at the reviewed commit e7bb585. The body uses free-form sections (## Why, ## What changes, ## Verification, ## Not in this PR); 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 open triage CHANGES_REQUESTED (review 4943936576) persists; the autofix loop has declared it cannot reformat the body (no GitHub write access in that mode), so this blocker persists until the body is reformatted — the existing content carries over almost verbatim (the pool measurements and the 19m45s re-fetch data belong under How to verify, the Not in this PR notes under Risk & Scope).
— qwen3.8-max via Qwen Code /review (v0.21.12)
| .replace(/^(\$\{GITHUB_WORKSPACE[^}]*\}|\$GITHUB_WORKSPACE)\//, ''); | ||
| const dest = join(base, target); |
There was a problem hiding this comment.
[Suggestion] R5-1: The post-wipe git clone coverage scan miscomputes absolute clone targets: after stripping the ${GITHUB_WORKSPACE}/ prefix, the remainder is workspace-root-relative, but it is still joined onto the step's working-directory — so a step with working-directory set can get an outside-the-wipe target blessed as covered. — Failure scenario: a future post-wipe step with working-directory: 'head' and run: git clone <url> "${GITHUB_WORKSPACE}/other" computes dest = join('head', 'other') = head/other, passes the coverage assert, yet the actual clone lands in $GITHUB_WORKSPACE/other, which the narrowed wipe does not remove — cross-PR bleed on the persistent pool with the pin green. The identical clone without working-directory correctly fails, so the guard's verdict flips on an irrelevant syntactic detail (both build steps in this very workflow already use working-directory, so that shape is the house style a future step would copy).
Witness (probe at e7bb585):
mutant (working-directory: 'head' + clone to "${GITHUB_WORKSPACE}/other") → # pass 6 / # fail 0
control (same clone, no working-directory) → error: 'post-wipe git clone target other is not covered by the wipe'
with fix applied → mutant flips to fail 1; pristine stays 6/6
Suggested fix — treat a workspace-prefixed target as absolute and skip the join:
const raw = line.trim().split(/\s+/).at(-1).replace(/^["']|["']$/g, '');
const wsPrefix = /^\$(?:\{GITHUB_WORKSPACE[^}]*\}|GITHUB_WORKSPACE)\//;
const dest = wsPrefix.test(raw) ? raw.replace(wsPrefix, '') : join(base, raw);— qwen3.8-max via Qwen Code /review (v0.21.12)
| const target = line | ||
| .trim() | ||
| .split(/\s+/) | ||
| .at(-1) |
There was a problem hiding this comment.
[Suggestion] R5-2: The clone-target extraction takes the LAST whitespace token of the whole line, so any compound command whose trailing token lands inside a checkout path passes the scan while the clone itself targets an unwiped dir; # comment lines containing git clone also false-positive the scan. — Failure scenario: appending a post-wipe step run: git clone --depth 1 https://example.com/x.git vendor && cp vendor/manifest.json head/manifest.json leaves the suite green, yet vendor/ is outside head//base/, survives the wipe, and bleeds into the next run on the persistent ECS pool — the exact regression this pin exists to catch. Conversely, a safe step whose run only says # defensive note: never git clone into tmp fails the suite spuriously ('post-wipe git clone target tmp is not covered').
Witness (probe at e7bb585):
compound mutant → # pass 6 / # fail 0 (clone into unwiped vendor/)
comment-only step → error: 'post-wipe git clone target tmp is not covered by the wipe'
with fix applied → both flip; pristine stays 6/6
Suggested fix — split each line on shell operators (&&, ||, ;, |) and extract the target from the segment containing git clone (or reject any git clone line containing an operator so clone steps stay one-command-per-line), and skip # comment lines as the wipe-script filter above already does.
— qwen3.8-max via Qwen Code /review (v0.21.12)
| const run = String(step.run || ''); | ||
| if (!run.includes('git clone')) continue; |
There was a problem hiding this comment.
[Suggestion] R5-3 (class finding — coverage model): The pin's coverage model enumerates recognized step shapes: the wipe-target oracle filters uses on actions/checkout (lines 222-223), and this scan matches only the literal substring git clone in run: scripts — while the comment above states the invariant generally ("any later step that materializes fresh workspace content must land inside them"). Any unrecognized materializer is simultaneously not wiped and not flagged, and because the expected side of the rm deepEqual derives from the same filtered list, both sides drift together and stay green. — Failure scenario: git -C "$GITHUB_WORKSPACE" clone u vendor — .includes('git clone') is false, the step is skipped here, the clone lands outside the wiped dirs and survives into the next run: the bleed recurs with the pin green. A composite/wrapper-action checkout (uses: './.github/actions/checkout-fork' with with.path: 'vendor') is invisible to both the derived rm targets and this scan; uses: steps are skipped unconditionally, and gh repo clone, curl | tar, download-artifact with a workspace-relative path are likewise invisible. This is the same family as R4-4, whose literal-git clone entrance this scan closed — the surface cannot be closed entrance by entrance. (No current step materializes content outside head//base/, so nothing ships broken today; the risk is future edits.)
Witness (probes at e7bb585):
'git -C "$GITHUB_WORKSPACE" clone u vendor'.includes('git clone') → false
that step appended → # pass 6 / # fail 0
composite-checkout mutant (with.path: 'vendor') → # pass 6 / # fail 0 (deepEqual green)
`${{`-guarded with.path coverage fix → composite mutant fails 1; pristine 6/6
Suggested fix — close the class at the layer that owns it: an exclusion-based wipe that keeps the root .git (find "$GITHUB_WORKSPACE" -mindepth 1 -maxdepth 1 ! -name '.git' -exec rm -rf {} +, plus a keep-list if other ECS-pool jobs persist root-level state), or an allowlist of post-wipe steps. Incremental options: assert that no post-wipe uses: step's with.path lands outside the checkout paths, guarded on ${{-expressed paths (the unguarded one-liner false-positives the existing upload-artifact step — probe-verified with the guard); narrow the comment to say only literal git clone run-steps are machine-checked; strip # comments as the adjacent parser does.
— qwen3.8-max via Qwen Code /review (v0.21.12)
| // already invokes this step with -eo pipefail, but pin the executed | ||
| // script anyway so any change forces a deliberate test update. | ||
| const executed = wipe.run |
There was a problem hiding this comment.
[Suggestion] R5-4 (class finding — execution semantics): The pin reads only the wipe step's run text, if, and boolean continue-on-error; it never constrains the other keys and levels that determine whether, how, and under what environment the pinned script executes — so this comment's claim ("pin the wipe step's entire executed script… any change forces a deliberate test update") is under-delivered. — Failure scenarios, each probe-verified green at e7bb585 while the wipe is disabled or masked: (1) step-level shell: 'true {0}' — the runner never executes the pinned rm, and stale head//base/ bleed into the next PR's checkouts; (2) workflow-level defaults: run: shell: 'true {0}' — same outcome, and a step-level-only wipe.shell assertion stays blind to it (wipe.shell remains undefined); (3) continue-on-error: "${{ runner.environment == 'self-hosted' }}" — a string under node:assert/strict, so it passes notEqual(…, true), and the runner evaluates it true exactly on the self-hosted pool, masking a wipe failure there; (4) env: BASH_ENV: <file> (step or workflow level) defining rm() { :; } — bash sources BASH_ENV before the script even under the runner's bash --noprofile --norc -eo pipefail, the function shadows /bin/rm, and the wipe exits 0 with both dirs surviving.
Witness (probes at e7bb585):
shell: 'true {0}' (step level) → # pass 6 / # fail 0
defaults.run.shell: 'true {0}' → # pass 6 / # fail 0 (step-only shell fix stays blind)
continue-on-error expression form → # pass 6 / # fail 0
BASH_ENV probe: `rm is a function`, wipe exit=0, head/base survive; suite green at step & workflow env level
chained shell resolution + absence asserts flip every mutant; pristine stays 6/6
Suggested fix — pin the whole execution chain:
const shell =
wipe.shell ?? serveAbDoc.jobs.ab.defaults?.run?.shell ?? serveAbDoc.defaults?.run?.shell;
assert.ok(shell === undefined || shell === 'bash', 'the wipe must run under the default bash wrapper');
assert.ok(!('continue-on-error' in wipe), 'a failed wipe must fail the job, not bleed into the next PR');
for (const envMap of [wipe.env, serveAbDoc.jobs.ab.env, serveAbDoc.env]) {
assert.ok(!envMap || envMap.BASH_ENV === undefined, 'BASH_ENV can shadow the pinned rm');
}(an env: no-GITHUB_WORKSPACE pin would additionally cover the defense-in-depth thread, though the runner re-writes GITHUB_WORKSPACE over user env for run: steps per actions/runner source)
— qwen3.8-max via Qwen Code /review (v0.21.12)
#9228) Replace the head/base-only rm with the exclusion-based find suggested by the R5-3 review: everything is wiped except the shared root .git, which closes the recurring materializer-coverage class (R4-4/R5-1/R5-2/R5-3) at the layer that owns it — anything landed outside .git is removed by the next run's wipe, no matter how it was materialized. This removes a strict subset of what main's whole-workspace wipe removes, so it cannot regress base behavior, while keeping the .git whose destruction forced ~900 MB re-fetches on the ECS pool's slow link. The pin now also locks the execution chain (shell wrapper, no continue-on-error, no BASH_ENV at step/job/workflow level) per R5-4, and the obsolete clone-coverage scan is deleted. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
🤖 Addressed the latest review feedback (round 4/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 4/100 轮）。改动内容与我反驳保留之处如下： 🤖 Addressed the latest review feedback (round 4/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 4/100 轮）。改动内容与我反驳保留之处如下： Implemented all 4 inline suggestions as one subtractive rework — the exclusion-based wipe R5-3 itself suggests as the root-cause fix — which also dissolves R5-1 and R5-2. The Critical (R2-11, PR body template) remains escalated; this round attaches a paste-ready template-compliant body to make it a one-paste job for a maintainer. What changedOne commit: Feedback points and dispositionsR5-3 (class finding — coverage model) + R5-1 + R5-2 — RESOLVED by one root-cause changeAll claims reproduced first (probes at pre-round HEAD e7bb585, same verdicts as the findings' witnesses):
Instead of patching the coverage scan (R5-1/R5-2's suggested fixes) or stacking the incremental guards (R5-3's incremental options), this round implements R5-3's primary suggested fix — the exclusion-based wipe: the step now runs That makes the defective coverage scan the findings critique obsolete: it is deleted, along with the derived-target machinery (~30 lines removed). R5-1's and R5-2's mutants are no longer defects — their clone targets are wiped by the next run regardless of Live-execution witness of the pinned command under the runner's R5-4 (class finding — execution semantics) — RESOLVEDAll four mutants reproduced first at pre-round HEAD (each pass 6/0 while the wipe is disabled or masked): step-level Post-fix probe matrix
Bite check for the verification gate: the changed test run against the pre-round branch fails 5/6 (renamed step + changed command), so this round's tests bite the pre-round branch. (The Critical resolved this round is the PR body, not code, and the inline findings are Suggestions, so no bite-rejection applies either way.) R2-11 (Critical) — PR body does not follow the PR template — ESCALATED with a paste-ready body attachedRe-verified this round at the current workflow: the address-review path still has no PR-body write path (no VerificationCommands actually run this round:
Appendix — paste-ready PR title and body for R2-11PR title (also in run artifact pr-title.txt)PR body (also in run artifact pr-body.md — fill the two MAINTAINER placeholders from the current body before saving)## What this PR does
The `Serve A/B` workflow builds the PR base and head on the persistent self-hosted ECS pool and reuses one workspace across runs, so a pre-checkout wipe removes leftovers that could otherwise bleed from one PR into the next. This PR keeps that wipe but stops it from destroying the shared root `.git`: the wipe now removes every top-level workspace entry except `.git`, and a regression test pins the wipe's exact command, its execution semantics (bash wrapper, no `continue-on-error`, no `BASH_ENV` at step/job/workflow level), and its ordering against the ownership-restore and checkout steps.
## Why it's needed
The previous whole-workspace wipe deleted the shared root `.git`, forcing the next job on the same runner to re-fetch 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.
<!\-\- MAINTAINER: please paste the original `## Verification` pool-measurement paragraphs (including the 19m45s re-fetch data) here — this autofix mode cannot read the original PR body. -->
## Reviewer Test Plan
### How to verify
- Run `node --test .github/scripts/ci-runner-routing.test.mjs` (part of ci.yml's `HELPER_TESTS`): it pins the wipe step's exact executed script and execution chain, the `if` guard, and the step ordering (ownership-restore < wipe < both checkouts).
- Mutation check a reviewer can apply: any edit to the wipe step — the command itself, a `shell:` override at step/job/workflow level, a `continue-on-error`, a `BASH_ENV` entry, reordering past a checkout, or an extra trailing command — must fail that test and force a deliberate test update.
- On a self-hosted runner, the root `.git` survives a serve-ab run and the next checkout reuses it (no full-history re-fetch); every other top-level workspace entry is removed before checkout, so post-wipe steps may materialize content anywhere without bleeding into the next run.
<!\-\- MAINTAINER: please paste any remaining original `## Verification` content (pool measurements, 19m45s re-fetch data) here. -->
### Evidence (Before & After)
N/A (workflow internals, not user-visible). The pinned wipe command was additionally live-executed against a synthetic workspace (`head/`, `base/`, `vendor/nested`, a root file, a dotfile, `.git/`) under the runner's `bash --noprofile --norc -e -o pipefail` wrapper: everything except `.git` was removed, exit 0.
### Tested on
| OS | Status |
| :--------: | :----: |
| 🍏 macOS | ⚠️ not tested |
| 🪟 Windows | ⚠️ not tested |
| 🐧 Linux | ✅ tested |
### Environment (optional)
Node 22 `node --test` helper suite + mutation probes; no CLI/sandbox run needed (workflow-only change).
## Risk & Scope
- Main risk or tradeoff: keeping the root `.git` also keeps any git-level exec vectors (hooks, fsmonitor, filters) an earlier untrusted run planted in it, which the old whole-workspace wipe destroyed as a side effect. The plant-to-next-job path exists with or without this wipe (it runs at run start, never as post-run cleanup), so this removes one incidental scrub rather than opening a new hole — documented in the workflow comment.
- Not validated / out of scope: the wipe removes every top-level entry except `.git`, exactly as `main`'s whole-workspace wipe does today, so other root-level state persisted by other ECS-pool jobs is treated exactly as before; no keep-list beyond `.git` was identified as necessary. <!\-\- MAINTAINER: please carry over the original `## Not in this PR` notes here. -->
- Breaking changes / migration notes: none.
## Linked Issues
N/A — no linked issue known to this autofix run (replace with the issue reference if one exists).
<details>
<summary>中文说明</summary>
（见 run artifact pr-body.md 中的完整中文翻译 / see the run artifact pr-body.md for the full Chinese translation）
</details>中文说明以一次减法重构实现了全部 4 条行内建议——采用 R5-3 自己提出的根因修复（基于排除项的 wipe）——同时也消解了 R5-1 与 R5-2。Critical（R2-11，PR 正文模板）继续升级给维护者；本轮附上一份可直接粘贴的模板合规正文，使维护者只需粘贴一次。 改动内容一个提交： 反馈点与处置R5-3（类发现——覆盖模型）+ R5-1 + R5-2 —— 以同一个根因改动解决所有声明均先复现（在 round 前 HEAD e7bb585 上的探针，判定与发现中的见证完全一致）：
本轮没有修补覆盖扫描（R5-1/R5-2 的建议修复），也没有叠加增量守卫（R5-3 的增量选项），而是实现了 R5-3 的首选建议修复——基于排除项的 wipe：该步骤现在执行 这使各发现所批评的有缺陷的覆盖扫描变得多余：它被删除，连同派生目标机制（约 30 行移除）。R5-1 与 R5-2 的变异不再是缺陷——无论 固定命令在 runner 的 R5-4（类发现——执行语义）—— 已解决四个变异均先在 round 前 HEAD 复现（wipe 被禁用或掩盖的情况下各自通过 6/0）：步骤级 修复后探针矩阵
供验证门参考的咬合检查：改动后的测试对 round 前分支运行为 5/6 失败（重命名的步骤名 + 变更的命令），因此本轮测试咬住 round 前分支。（本轮解决的 Critical 是 PR 正文而非代码，行内发现均为 Suggestion，因此咬合拒绝条款本就不适用。） R2-11（Critical）—— PR 正文未遵循 PR 模板 —— 升级给维护者，附可直接粘贴的正文本轮在当前工作流上再次核实：address-review 路径仍然没有 PR 正文写入路径（address 流程中没有任何 验证本轮实际运行的命令：
附录 —— R2-11 可直接粘贴的 PR 标题与正文（PR 标题与正文草稿见上方英文附录；完整双语版本在 run artifact 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/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
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.
[Critical] R5-5: The PR body does not follow the PR template — still stands at the reviewed commit 900221e. The body uses free-form sections (## Why, ## What changes, ## Verification, ## Not in this PR); 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 open triage CHANGES_REQUESTED (review 4943936576) persists; the autofix loop has declared it cannot reformat the body (no GitHub write access in that mode), so this blocker persists until a maintainer reformats the body. The body content is additionally stale: it still describes the superseded head/base-only wipe ("the wipe now removes exactly those two directories") while the shipped code wipes everything except the root .git (see R6-1) — the reformat must not carry that stale content forward verbatim.— qwen3.8-max via Qwen Code /review (v0.21.12)
| # plant-to-next-job path exists with or without this wipe (it runs | ||
| # at run start, never as post-run cleanup), so this removes one | ||
| # incidental scrub rather than opening a new hole. | ||
| find "$GITHUB_WORKSPACE" -mindepth 1 -maxdepth 1 ! -name '.git' -exec rm -rf {} + |
There was a problem hiding this comment.
[Suggestion] R6-1: The PR title ("narrow serve-ab's self-hosted wipe to the A/B checkout dirs") and body still describe the superseded head/base-only implementation — "the wipe now removes exactly those two directories: rm -rf "${GITHUB_WORKSPACE:?}/head" "${GITHUB_WORKSPACE:?}/base"" — while the shipped code wipes every top-level workspace entry except the root .git: the command pinned here. — Failure scenario: the R2-11 remedy is a maintainer body reformat where "the existing content carries over almost verbatim"; following that path bakes the stale description into the merged record — title and body would state a two-directory wipe while the shipped behaviour removes everything except .git (a materially different scope carrying the deliberately documented security trade-off). Anyone auditing runner wipe behaviour — or verifying the body's Verification bullets — checks the wrong command. Suggested fix: retitle to match the shipped change (e.g. the HEAD commit's own wording, fix(ci): exclude the shared root .git from serve-ab's self-hosted wipe) and rewrite the What changes / Verification sections to describe the except-.git find form and the structural test pin, before the R2-11 reformat carries the stale content forward.
— qwen3.8-max via Qwen Code /review (v0.21.12)
There was a problem hiding this comment.
Agreed — the live title and body describe the superseded two-directory wipe, while the shipped code (re-verified this round at 900221ea) is the except-.git find form pinned here. This finding cannot be resolved from this mode, for the same reason as the Critical (R6-2, previously R5-5/R2-11): the address-review flow has no GitHub write access, and qwen-autofix.yml's address path contains no title/body edit step and consumes no title/body artifact (re-verified this round). This round's no-change report comment attaches a paste-ready replacement implementing exactly the suggested fix — the title becomes the HEAD commit's own wording, and the body sections are rewritten around the except-.git find form and the structural test pin — also saved as the run artifacts pr-title.txt / pr-body.md, so the maintainer reformat carries the shipped description forward, not the stale one. The thread stays open until the live title/body are actually updated.
中文说明
认同——线上标题与正文描述的是已被取代的两目录清理，而已发布的代码（本轮在 900221ea 上重新核实）是此处固定的排除 .git 的 find 形式。本发现无法在本模式中解决，原因与 Critical（R6-2，即此前的 R5-5/R2-11）相同：address-review 流程没有 GitHub 写入权限，且 qwen-autofix.yml 的 address 路径中既没有标题/正文编辑步骤，也不消费任何标题/正文产物（本轮已重新核实）。本轮的"无改动"报告评论附上了一份可直接粘贴的替换稿，实现的正是本发现建议的修复——标题改用 HEAD 提交自身的措辞，正文各章节围绕排除 .git 的 find 形式与结构化测试固定重写——同时保存为 run artifact pr-title.txt / pr-body.md，使维护者重排时带入的是已发布行为的描述，而非过时描述。在线上标题/正文实际更新之前，本线程保持打开。
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix review round — PR #9228 — no code changes this roundBoth remaining findings target the PR's GitHub-side title and body, which this address-review flow cannot edit: it has no GitHub write access, and the address-review path of Feedback points and dispositionsLint note (actionlint embedded-shell source mapping) — NOTED, no actionThe review itself marks this as a tool limitation, not a blocker: actionlint cannot source-map the shell embedded in the workflow, so that fragment was not linted. The workflow YAML itself lints clean ( R6-2 (Critical) — PR body does not follow the PR template and carries stale content — AGREED, ESCALATED (maintainer action required)Agreed on both counts. The live body uses free-form sections ( R6-1 (Suggestion, rc:3790666519) — title and body describe the superseded implementation — AGREED, ESCALATED (same maintainer action), thread reply postedVerified at the shipped code: the wipe is What a maintainer needs to do (one paste)
Verification
Appendix — paste-ready PR title and body (for the maintainer reformat)Proposed PR title: Proposed PR body (fill the ## What this PR does
The `Serve A/B` workflow runs on the persistent self-hosted ECS pool and reuses one workspace across runs, so a pre-checkout wipe removes leftovers that could otherwise bleed from one PR into the next. This PR keeps that wipe but stops it from destroying the shared root `.git`: the wipe now removes every top-level workspace entry except `.git` — the pre-PR whole-workspace command plus exactly one `! -name '.git'` predicate — the step is renamed to say so, and the structural test pins the wipe step's entire executed script, its execution semantics (default bash wrapper, no `continue-on-error`, no `BASH_ENV` at step/job/workflow level), and its ordering (ownership-restore before wipe, wipe before both checkouts).
## Why it's needed
The previous whole-workspace wipe also deleted the shared root `.git`, forcing the next job on the same runner to re-fetch 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.
<!\-\- MAINTAINER: please paste the original `## Verification` pool-measurement paragraphs (including the 19m45s re-fetch data) here — this autofix mode cannot read the original PR body. Delete this comment when done. -->
## Reviewer Test Plan
### How to verify
- Run `node --test .github/scripts/ci-runner-routing.test.mjs` (part of ci.yml's `HELPER_TESTS`): it pins the wipe step's exact two executed lines (`set -uo pipefail` + the except-`.git` `find`), the `if: runner.environment == 'self-hosted'` guard, the resolved shell, the absence of `continue-on-error` and of `BASH_ENV` at all three levels, and the step ordering (ownership-restore < wipe < both checkouts).
- Mutation checks a reviewer can apply: any edit to the wipe step — dropping `! -name '.git'`, a `shell:` override at step/job/workflow level, a `continue-on-error`, a `BASH_ENV` entry, moving the step below a checkout, or an extra trailing command — must fail that test and force a deliberate test update.
- On a self-hosted runner: the root `.git` survives a serve-ab run (the next job reuses it instead of re-fetching the full history), and every other top-level workspace entry is removed before checkout, so whatever a run materializes outside `.git` cannot bleed into the next run.
<!\-\- MAINTAINER: please paste any remaining original `## Verification` content (pool measurements, 19m45s re-fetch data) here. Delete this comment when done. -->
### Evidence (Before & After)
N/A (workflow internals, not user-visible). The pinned wipe command was additionally live-executed against a synthetic workspace (`head/`, `base/`, `vendor/nested`, a root file, a dotfile, `.git/`) under the runner's `bash --noprofile --norc -e -o pipefail` wrapper: everything except `.git` was removed, exit 0.
### Tested on
| OS | Status |
| :--------: | :----: |
| 🍏 macOS | ⚠️ not tested |
| 🪟 Windows | ⚠️ not tested |
| 🐧 Linux | ✅ tested |
### Environment (optional)
Node 22 `node --test` helper suite + mutation probes; no CLI/sandbox run needed (workflow-only change).
## Risk & Scope
- Main risk or tradeoff: keeping the root `.git` also keeps any git-level exec vectors (hooks, fsmonitor, filters) an earlier untrusted run planted in it, which the old whole-workspace wipe destroyed as a side effect. The plant-to-next-job path exists with or without this wipe (it runs at run start, never as post-run cleanup), so this removes one incidental scrub rather than opening a new hole — documented in the workflow comment.
- Not validated / out of scope: the wipe removes every top-level entry except `.git`, a strict subset of what `main`'s whole-workspace wipe removes, so root-level state persisted by other ECS-pool jobs is treated exactly as before; no keep-list beyond `.git` was identified as necessary. <!\-\- MAINTAINER: please carry over the original `## Not in this PR` notes here. Delete this comment when done. -->
- Breaking changes / migration notes: none.
## Linked Issues
N/A — this PR is not linked from a separate issue; the `(#9228)` references in its commits are this PR itself.
<details>
<summary>中文说明</summary>
## 本 PR 做了什么
`Serve A/B` 工作流运行在持久化的自托管 ECS 池上，跨运行复用同一个工作区，因此 checkout 前的清理（wipe）会删除上一次运行的残留，避免其串入下一个 PR。本 PR 保留该清理，但不再销毁共享的根 `.git`：清理现在删除工作区根下除 `.git` 外的所有顶层条目——即本 PR 之前的全工作区命令恰好加上一个 `! -name '.git'` 谓词——步骤名相应更新，并且结构化测试固定（pin）了清理步骤的完整执行脚本、执行语义（默认 bash 包装器、无 `continue-on-error`、step/job/workflow 三级均无 `BASH_ENV`）以及步骤顺序（ownership-restore 先于 wipe，wipe 先于两个 checkout）。
## 为什么需要
此前的全工作区清理会连共享根 `.git` 一起删除，迫使同一 runner 上的下一个作业从 github.com 重新拉取约 900 MB 的完整历史。在 ECS 池的慢速链路上，这会使 checkout 停滞 20 分钟以上，且拉取经常在中途断掉，表现如同 runner 挂起。
<!\-\- MAINTAINER：请将原 `## Verification` 中的 ECS 池测量段落（含 19 分 45 秒重新拉取数据）粘贴到此处——本 autofix 模式无法读取原始 PR 正文。完成后删除本注释。 -->
## 审阅者测试计划
### 如何验证
- 运行 `node --test .github/scripts/ci-runner-routing.test.mjs`（ci.yml 的 `HELPER_TESTS` 之一）：它固定了清理步骤恰好两条执行行（`set -uo pipefail` + 排除 `.git` 的 `find`）、`if: runner.environment == 'self-hosted'` 守卫、解析后的 shell、`continue-on-error` 与三级 `BASH_ENV` 的缺失，以及步骤顺序（ownership-restore < wipe < 两个 checkout）。
- 审阅者可施加的变异检查：对清理步骤的任何改动——去掉 `! -name '.git'`、在 step/job/workflow 任一级加 `shell:` 覆盖、加 `continue-on-error`、加 `BASH_ENV` 条目、把步骤移到 checkout 之后、或追加任何后续命令——都必须使该测试失败，从而强制走一次有意的测试更新。
- 在自托管 runner 上：serve-ab 运行后根 `.git` 保留（下一个作业直接复用，无需重新拉取完整历史），且 checkout 前工作区根的其余所有顶层条目均被删除，因此运行期间在 `.git` 之外生成的任何内容都不会串入下一次运行。
<!\-\- MAINTAINER：请将原 `## Verification` 的其余内容（池测量数据、19 分 45 秒重新拉取数据）粘贴到此处。完成后删除本注释。 -->
### 证据（前后对比）
N/A（工作流内部改动，用户不可见）。此外，固定的清理命令已在合成工作区（`head/`、`base/`、`vendor/nested`、一个根级文件、一个点文件、`.git/`）上、于 runner 的 `bash --noprofile --norc -e -o pipefail` 包装器下真实执行：除 `.git` 外全部删除，退出码 0。
### 测试环境
| OS | 状态 |
| :--------: | :----: |
| 🍏 macOS | ⚠️ 未测试 |
| 🪟 Windows | ⚠️ 未测试 |
| 🐧 Linux | ✅ 已测试 |
### 环境（可选）
Node 22 `node --test` 辅助测试套件 + 变异探针；无需 CLI/沙箱运行（纯工作流改动）。
## 风险与范围
- 主要风险与权衡：保留根 `.git` 意味着此前不受信任的运行植入其中的 git 级执行载体（hooks、fsmonitor、filters）也会随之保留——旧的全工作区清理会顺带销毁它们。植入到下一个作业的路径无论有没有本清理都存在（它发生在运行开始时，从不是运行后清理），因此这只是移除了一个附带清除，而非打开新的口子——已在工作流注释中记录。
- 未验证 / 超出范围：清理删除除 `.git` 外的所有顶层条目，是 `main` 全工作区清理删除范围的严格子集，因此其他 ECS 池作业持久化在根级的状态与之前完全一致；未发现需要 `.git` 之外的保留名单。<!\-\- MAINTAINER：请将原 `## Not in this PR` 的备注迁移到此处。完成后删除本注释。 -->
- 破坏性变更 / 迁移说明：无。
## 关联 Issue
N/A —— 本 PR 没有单独关联的 issue；提交信息中的 `(#9228)` 即本 PR 自身。
</details>中文说明Autofix 审查轮次 — PR #9228 — 本轮无代码改动剩余的两个发现都指向 PR 在 GitHub 侧的标题与正文，而本 address-review 流程无法编辑它们：它没有 GitHub 写入权限，且 反馈点及处置Lint 备注（actionlint 嵌入式 shell 源码映射）—— 已知悉，无需处理审查本身将其标注为工具限制而非阻塞项：actionlint 无法对工作流中嵌入的 shell 做源码映射，因此该片段未被 lint。工作流 YAML 本身 lint 干净（早前的 round 在同一步骤文本下运行 R6-2（Critical）—— PR 正文未遵循 PR 模板且内容已过时 —— 认同，升级（需要维护者操作）两点均认同。线上正文使用自由格式章节（ R6-1（Suggestion，rc:3790666519）—— 标题与正文描述的是被取代的实现 —— 认同，升级（同一维护者操作），已在对应线程回复在已发布代码上核实：清理命令是 维护者需要做什么（一次粘贴）
验证
附录 —— 可直接粘贴的 PR 标题与正文（供维护者重排）（与上文英文附录完全相同：建议 PR 标题为 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
|
🔀 Base updated: red check(s) [Test (ubuntu-latest, Node 22.x)] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base：红色检查 [Test (ubuntu-latest, Node 22.x)] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main，CI 将重新运行。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
3 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- cross-copy allowlist parity pin for the serve-ab scrub (agent 3a this round) — already reported as R8-5 (comment 3792902589); the author deferred it pending the maintainer decision on the R9-1 thread, which is still open
- exec fixture for the -type d keep-predicate negative branch — a symlink/gitfile .git (agent 5 this round) — already recorded in the round-14 deferral list (serve-ab-workflow.test.js:131) and re-confirmed through round 18
- pinning working-directory/GIT_DIR in the execution-context audit next to BASH_ENV/PATH/GITHUB_WORKSPACE (agent 6b this round) — already recorded in the round-14/17 deferral lists (ci-runner-routing.test.mjs:227)
Not reviewed: issue-fidelity — closing-issue metadata unavailable (host gh < 2.72.0); fidelity judged from the PR description and thread instead.
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.
[Critical] R8-2 (carried from round 8; R2-11/R5-5/R6-2/R7-2 lineage): the live PR body does not follow the PR template (free-form sections ## Why / ## What changes / ## Verification / ## Not in this PR; none of the required sections present: What this PR does / Why it's needed / Reviewer Test Plan with How to verify, Evidence (Before & After), Tested on matrix / Risk & Scope / Linked Issues / Chinese
Details
translation) and is materially stale: '## What changes' still quotes the superseded two-directory rm (rm -rf "${GITHUB_WORKSPACE:?}/head" "${GITHUB_WORKSPACE:?}/base", "the wipe now removes exactly those two directories") while the shipped code keeps the shared root .git, defangs it, and scrubs its config — the design the open R9-1/R8-9 maintainer decision is about. The autofix loop cannot reformat the body (no GitHub write access in that mode); a maintainer must reformat it describing the shipped keep-and-defang behavior. Witness: live body fetched at head 4fd6a7a this round; deterministic test-plan gate: "the PR description has no Test Plan section".— qwen3.8-max via Qwen Code /review (v0.22.0)
| # mirroring qwen-triage.yml's config-sanitize. | ||
| find "$WS" -mindepth 1 -maxdepth 1 ! \( -name '.git' -type d \) -exec rm -rf {} + | ||
| rm -rf "$WS/.git/hooks" "$WS/.git/info/attributes" | ||
| { git config --local --name-only --list 2>/dev/null || true; } | { grep -ivE '^(core\.(repositoryformatversion|bare|filemode|symlinks|ignorecase|precomposeunicode|logallrefupdates|worktree|hidedotfiles|protecthfs|protectntfs)|remote\.|branch\.|extensions\.|gc\.|pack\.|fetch\.|index\.|safe\.|submodule\.[^.]+\.(url|active|branch))' || true; } | while IFS= read -r key; do git config --local --unset-all "$key" 2>/dev/null || true; done |
There was a problem hiding this comment.
[Critical] R9-1 (class finding; supersedes R8-1): still stands at the reviewed commit 4fd6a7aa — the kept .git is attacker-writable state handed to later, more privileged pool jobs, and this scrub sanitizes it entrance by entrance over a surface that cannot be enumerated. The scrub line is byte-identical to rounds 8–18, and this round re-probed four entrances against the verbatim wipe tail with real git 2.43, against a discriminating control (a plain planted core.hooksPath in .git/config IS scrubbed and never fires, so each survivor genuinely bypassed the scrub):
extensions.worktreeConfig(allowlisted) +.git/config.worktree—git config --localnever lists or unsets that file; a plantedcore.hooksPathsurvives and FIRED at the next job's checkout;- a planted
.git/config.lockmakes every--unset-alldie swallowed by|| true— the scrub exits 0 having unset nothing, and the planted hook FIRED; - a planted
core.repositoryformatversion = 999(allowlisted) kills the scrub's own--list(exit 128, swallowed), self-shielding co-planted exec keys and wedging the next job's checkout; core.worktreeis deliberately kept by the allowlist — through actions/checkout's reuse path a planted value destroyed a file OUTSIDE the workspace.
The fifth entrance claimed in earlier rounds (planted .git/commondir redirect) did not reproduce on git 2.43.0 this round and is not re-asserted. The class stands: rounds 7–9 each found live entrances, and this round confirmed four of them again — the surface (git's config namespace, the git-dir's files, and the repo's shape) cannot be closed by an entrance-by-entrance sweep.
Failure scenario: a pool job running untrusted PR code (this workflow's own npm ci on the head checkout runs as the runner user) plants any of the above in $GITHUB_WORKSPACE/.git; the next wipe keeps all of it and logs success (exit 0); the next root-checkout consumer on the ecs-qwen pool (ci.yml Test, qwen-triage.yml — pull_request_target with write token, qwen-autofix.yml, qwen-code-pr-review.yml) reuses the repo and executes attacker code or destroys victim files outside the workspace.
Witness (probe, git 2.43, verbatim wipe tail, this round):
config.worktree: --local list shows hookspath? 0 hit(s) → wipe exit 0 →
config.worktree exists: yes → next-job checkout exit 0, hook marker: FIRED
config.lock: wipe exit 0 → after: --local core.hooksPath: <attacker path>,
hook marker: FIRED
control: plain core.hooksPath in .git/config → scrubbed, hook marker: no
core.worktree: reuse-path git clean -ffdx exit 0 → victim.txt outside WS: DESTROYED
Structural close, per the pending maintainer decision (triage defer comment 5311584017): (A) revert to the full wipe — closes every entrance because nothing is kept; or (B) keep the .git only behind the repo's hardened preamble (rm -f of commondir/shallow/config.worktree/config.lock + --unset-all extensions.worktreeConfig before the sweep, per resanitize-git-config.sh) AND a fail-closed post-state audit that exits 1 when non-allowlisted state remains. Immediate hardening alone is insufficient (each piece verified: none closes the class alone). Update the pinned executed-line set in ci-runner-routing.test.mjs with whichever lands.
— qwen3.8-max via Qwen Code /review (v0.22.0)
There was a problem hiding this comment.
Stands at this head, and deliberately left unresolved — same escalation as the round-18 thread: the maintainer decision (triage defer comment 5311584017) between (A) reverting to the full wipe and (B) keeping the .git only behind the hardened preamble + fail-closed post-state audit. The four re-probed entrances are acknowledged (including the dropped commondir claim, which this thread correctly does not re-assert); the class argument — that git's config namespace, git-dir files, and repo shape cannot be swept entrance by entrance — is exactly why this is a design decision and not an actionable fix: the probes verify no single hardening piece closes the class. No code change this round (Critical-only mode). Maintainer question: (A) or (B)? If (B), this PR would grow to the structural close named in the finding; if (A), the wipe reverts and the keep-.git tests are removed with it. Thread stays open until answered.
中文说明
在该 head 上依然成立，并被有意保持未解决——与第 18 轮线程相同的升级：等待维护者在 (A) 恢复完全清除与 (B) 仅在硬化前置 + 失败即关闭的事后状态审计之后保留 .git 之间作出决策（triage 延后评论 5311584017）。四个被重新探测的入口已知悉（包括被放弃的 commondir 论断，本线程正确地未再主张）；类别论点——git 配置命名空间、git-dir 文件与仓库形态无法被逐入口清扫——正是为什么这是设计决策而非可执行修复：探测已验证任何单一硬化部件都无法闭合该类别。本轮无代码改动（仅处理 Critical 模式）。维护者问题：(A) 还是 (B)？若选 (B)，本 PR 将扩展到发现中所述的结构化收口；若选 (A)，清除逻辑回退，保留 .git 的相关测试一并移除。在得到答复前线程保持开放。
| # scrubbed to the same allowlist as qwen-triage.yml's config- | ||
| # sanitize (git's exec knobs are an open-ended class; an allowlist | ||
| # closes the class instead of denylisting knobs one by one). | ||
| # Objects and refs — the reason the .git is kept — are untouched, |
There was a problem hiding this comment.
[Critical] R8-9 (class finding, folds R8-10): still stands at the reviewed commit 4fd6a7aa — "untouched" here means "trusted": the kept .git's objects and refs are attacker-writable, and downstream pool jobs consume them with no integrity check. The wipe tail is byte-identical to rounds 8–18 and touches only hooks/info/attributes and config keys; refs/ and objects/ survive by design. The refs/replace channel was re-probed THIS round with real git, adjudicating the in-round dispute — git checkout --force <good-sha>, the exact command pinned actions/checkout df4cb1c runs, DOES materialize the replaced tree.
Failure scenario: a malicious PR author (same-repo or write-access fork — the pool's admitted population) plants git replace <head-sha> <evil> during a serve-ab job; nothing on the pool ever removes it (grep for refs/replace / --no-replace across .github/: 0 matches); a later pool job root-checking-out that SHA — qwen-code-pr-review.yml::review-pr (issue_comment-triggered, write token), or a SHA-pinned checkout in another workflow — builds/executes attacker content disguised as the pinned SHA while rev-parse HEAD still prints the clean SHA.
Witness (probe, git 2.43, this round):
replace refs after wipe: 0695977e… (intact, by design)
git fetch + git checkout --force <good-sha> (verbatim actions/checkout command):
worktree file.txt content: ATTACKER PAYLOAD
rev-parse HEAD: 0695977e… (the clean SHA)
flip: git replace -d + re-checkout → file.txt content: benign content v1
The round-8 pack-index channel is subsumed by the demonstrated wholesale preservation of .git/objects. The surface (objects + refs of a kept attacker-writable repo) cannot be closed entrance by entrance — per the same pending maintainer decision as R9-1: (A) revert to the full wipe; or (B) treat objects+refs as untrusted before reuse — delete refs/replace (loose + packed-refs filtering), sweep non-essential ref namespaces, gate reuse on git fsck --strict with rm -rf of the kept .git on failure (accepting the one-time re-fetch cost), complemented by GIT_NO_REPLACE_OBJECTS=1 for SHA-pinned consumers.
— qwen3.8-max via Qwen Code /review (v0.22.0)
There was a problem hiding this comment.
Stands at this head, and deliberately left unresolved — same escalation as the round-18 thread: closes only via the pending maintainer decision shared with R9-1 (triage defer comment 5311584017). The round-19 re-probe adjudicating the replace-ref dispute is acknowledged: git checkout --force <sha>, the verbatim pinned actions/checkout command, materializes the replaced tree while rev-parse HEAD prints the clean SHA, and refs/replace survives the wipe by design. No code change this round: the fix shape ((A) full wipe, or (B) replace-ref deletion + ref-namespace sweep + git fsck --strict reuse gate with rm -rf fallback + consumer-side GIT_NO_REPLACE_OBJECTS=1) is the maintainer's choice. Maintainer question: (A) or (B)? Thread stays open until answered.
中文说明
在该 head 上依然成立，并被有意保持未解决——与第 18 轮线程相同的升级：只能通过与 R9-1 共享的待决维护者决策来闭合（triage 延后评论 5311584017）。第 19 轮对 replace 引用争议的重新探测裁定已知悉：git checkout --force <sha>（即固定 actions/checkout 的原命令）会实体化被替换的树，而 rev-parse HEAD 仍输出干净的 SHA，且 refs/replace 按设计在清除后存活。本轮无代码改动：修复形态（(A) 完全清除，或 (B) replace 引用删除 + 引用命名空间清扫 + git fsck --strict 复用门禁并在失败时 rm -rf 回退 + 消费者侧 GIT_NO_REPLACE_OBJECTS=1）由维护者选择。维护者问题：(A) 还是 (B)？在得到答复前线程保持开放。
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix round 20 — PR #9228: no code change this roundOutcome: no action. Every remaining Critical finding is either escalated for a Mode context
Findings and dispositionsR9-1 — kept
|
|
@qwen-code /triage |
|
Sandboxed verification: ❌ not passed — findings reported (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: 176 passed · 0 failed · 176 total Flakiness gate: ✅ 2 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：176 通过 · 0 失败 · 176 总计 抖动门：✅ 2 changed test file(s) x 5 identical rounds, no divergence Verification reportVerdict: 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：176 通过 · 0 失败 · 176 总计 中文摘要
Previous-finding status (follow-up round)The previous round verified head
Scope
Central claim + A/B tableThe wipe
Head keeps a real Test pins (secondary claim 1) — mutation matrix ( Targeted gates ( Corrections
FindingsF1 (carried, stands) —
|
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix review round — no action taken (PR #9228)This round has no actionable feedback, so no code changes were made and no commit was created:
The deferred non-Critical items stay open for human follow-up; nothing was resolved, declined, or escalated this round. 中文说明Autofix 评审轮次 — 未采取任何操作（PR #9228）本轮没有可处理的反馈，因此未做任何代码改动，也未创建任何提交：
被延迟的非 Critical 条目保持开放，留待人工跟进；本轮没有解决、拒绝或升级任何条目。 Deferred non-Critical feedbackCritical-only mode is active: 5 change-producing rounds are complete. The workflow excluded the non-Critical feedback below from this round's actionable sections; the items remain open for human follow-up. Maintainer feedback is deferred only after its author has used 2 regular feedback batches in this window's Critical-only tail; authors at that budget, if any, are named below. (
中文说明已进入仅处理 Critical 的模式：已完成 5 个产生改动的轮次。本轮可执行区域已排除下方非 Critical 反馈；这些条目保持开放，留待人工跟进。维护者反馈仅在其本人于本窗口 Critical-only 阶段已使用 2 批常规反馈预算后才会延后；达到预算的作者（如有）在下方点名。（评论 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
|
Triage re-run completed without a new review. The bot already has a review of its own on 机器人在 The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
6 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- cross-copy allowlist parity pin for the serve-ab scrub (agents 3a/5 this round) — already reported as R8-5 (comment 3792902589); the author deferred it pending the maintainer decision on the R9-1 thread, which is still open
- working-directory/GIT_DIR pin in the execution-context audit (agent 5/6c and reverse-audit rounds 1/3 this round) — already recorded in the round-14 and round-17 deferral lists (ci-runner-routing.test.mjs:227)
- exec fixture for the -type d keep-predicate negative branch — a symlink/gitfile .git (agent 5 this round) — already recorded in the round-14 deferral list (serve-ab-workflow.test.js:131)
- scrub breadcrumb for hung-runner forensics (agent 6b this round) — already recorded in the round-13 deferral list (serve-ab.yml:99)
- git -C "$WS" for CWD-independence of the scrub (agent 6c this round) — already recorded in the round-17 deferral list (serve-ab.yml:206)
- narrowing the remote..uploadpack/receivepack allowlist arm (agent 6a this round) — already recorded in the round-13 deferral list and folded into the re-posted R9-1 entrance (4) this round
Not reviewed: reverse audit — stopped before round 6 by the review time budget.
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.
Deferred under the convergence posture (round 20, not a blocker) — recorded, not requested in this round:
scripts/tests/serve-ab-workflow.test.js:42 — [probe] runWipe cwd pin lets the scrub walk up to the host repo's local config when TMPDIR sits inside a git work tree
[Critical] R8-2 (carried from round 8; R2-11/R5-5/R6-2/R7-2 lineage): the live PR body does not follow the PR template (free-form sections ## Why / ## What changes / ## Verification / ## Not in this PR; none of the required sections present: What this PR does / Why it's needed / Reviewer Test Plan with How to verify, Evidence (Before & After), Tested on matrix / Risk & Scope / Linked Issues / Chinese
Details
translation) and is materially stale: '## What changes' still quotes the superseded two-directory rm (rm -rf "${GITHUB_WORKSPACE:?}/head" "${GITHUB_WORKSPACE:?}/base", "the wipe now removes exactly those two directories") while the shipped code keeps the shared root .git, defangs it, and scrubs its config — the design the open R9-1/R8-9 maintainer decision is about. '## Verification' also cites pins that no longer match the shipped tests. The open triage CHANGES_REQUESTED (review 4943936576) persists; the autofix loop cannot reformat the body (no GitHub write access in that mode), so a maintainer must reformat it describing the shipped keep-and-defang behavior (a paste-ready template-conforming draft was posted in round 11; it must be re-derived if the pending R9-1/R8-9 decision changes the design). Merging on the current description would put a materially different change into the history than the body documents. Witness: live body fetched at the reviewed commit this round via gh pr view 9228 --json body (free-form headings as listed; '## What changes' quoting the superseded two-directory rm) vs reviewed tree serve-ab.yml:204find "$WS" -mindepth 1 -maxdepth 1 ! \( -name '.git' -type d \) -exec rm -rf {} + — the two do not describe the same change; deterministic test-plan gate: "the PR description has no Test Plan section".
— qwen3.8-max via Qwen Code /review (v0.22.0)
| # mirroring qwen-triage.yml's config-sanitize. | ||
| find "$WS" -mindepth 1 -maxdepth 1 ! \( -name '.git' -type d \) -exec rm -rf {} + | ||
| rm -rf "$WS/.git/hooks" "$WS/.git/info/attributes" | ||
| { git config --local --name-only --list 2>/dev/null || true; } | { grep -ivE '^(core\.(repositoryformatversion|bare|filemode|symlinks|ignorecase|precomposeunicode|logallrefupdates|worktree|hidedotfiles|protecthfs|protectntfs)|remote\.|branch\.|extensions\.|gc\.|pack\.|fetch\.|index\.|safe\.|submodule\.[^.]+\.(url|active|branch))' || true; } | while IFS= read -r key; do git config --local --unset-all "$key" 2>/dev/null || true; done |
There was a problem hiding this comment.
[Critical] R9-1 (class finding; supersedes R8-1): still stands at the reviewed commit 51777f43 — the kept .git is attacker-writable state handed to later, more privileged pool jobs, and this scrub sanitizes it entrance by entrance over a surface that cannot be enumerated — git's config namespace, the git-dir's files, and the repo's shape. The scrub line is byte-identical to rounds 8–19 (this round merged main only). Five entrances were re-proven at this commit this round, each against the verbatim scrub: (1) allowlisted extensions.worktreeConfig + a planted .git/config.worktree carrying core.hooksPath — git config --local neither lists nor unsets config.worktree keys; after the scrub exits 0 the planted hook fires in a consumer checkout (flip: rm config.worktree + unset extensions.worktreeConfig — hook does not fire); (2) a planted .git/config.lock makes every --unset-all die with "could not lock config file", swallowed by || true — the scrub exits 0 having unset nothing and a consumer checkout fires the planted hook (flip: lock removed — 0/4 attack keys survive); (3) a planted .git/commondir kills the sweep wholesale (fatal: --local can only be used inside a git repository), scrub exit 0, planted keys survive; (4) the blanket remote\. allowlist arm keeps remote.<name>.uploadpack/receivepack/proxy exec knobs; (5) core.worktree is deliberately kept by the allowlist's core\. arm — a consumer git checkout --force <sha> then materializes the commit's files at the planted path OUTSIDE the workspace while the repo directory stays empty (flip: drop worktree from the keep arm — files land in the workspace). The failure scenario is unchanged: a pool job running untrusted PR code (serve-ab's own npm ci/build runs as the runner user after the wipe) plants any combination in $GITHUB_WORKSPACE/.git; the next wipe keeps all of it and logs success; the next root-checkout consumer on the ecs-qwen pool — ci.yml (zero git-state sanitization, checks:write/pull-requests:write, incl. its push lane), qwen-triage.yml's pull_request_target triage job with CI_BOT_PAT, qwen-autofix.yml with CI_DEV_BOT_PAT, qwen-code-pr-review.yml — reuses the kept repo (actions/checkout's origin-URL-match reuse deletes only refs/heads/*) and executes attacker code with that job's token/secrets. The siblings this step claims to mirror (qwen-triage.yml:2704-2713, qwen-autofix.yml:668-673, resanitize-git-config.sh) carry the rm/unset preamble for this exact "Verified" bypass; this copy does not.
Witness (probes, this round, verbatim scrub extracted from this line, all with flips):
E1 config.worktree: scrub exit 0, core.hooksPath survives at --worktree scope,
consumer checkout -> HOOK-FIRED; flip -> hook did NOT fire
E2 config.lock: wipe exit 0, 4/4 attack keys survive, HOOK-FIRED; flip -> 0/4 survive
E3 commondir: fatal: --local can only be used inside a git repository, scrub exit 0, keys survive
E4 remote.*: uploadpack/receivepack/proxy all survive the scrub
E5 core.worktree: checkout --force materializes files ONLY at the planted outside path;
flip (drop worktree from keep arm) -> files land in the workspace
Still escalated for the maintainer decision (triage defer comment 5311584017) — (A) revert to the full wipe, or (B) keep the .git only behind the hardened preamble (rm -f of commondir/shallow/config.worktree/config.lock + unset extensions.worktreeConfig before the sweep; narrow remote\. to remote\..+\.(url|fetch|pushurl); drop worktree from the core\. arm; fail-loud post-state audit) plus a fail-closed integrity pass. Update the pinned executed-line set in ci-runner-routing.test.mjs with any change.
— qwen3.8-max via Qwen Code /review (v0.22.0)
There was a problem hiding this comment.
Disposition: escalated to a maintainer decision — thread deliberately left unresolved; not settled unilaterally.
Verified at the reviewed commit: HEAD is 51777f43a2 and git diff 4fd6a7aa..HEAD over the three PR files is empty — the scrub is byte-identical to the head this round's probes re-proved the five bypasses on, so the finding stands by code identity; no new probe was needed to confirm it this round.
The pending decision is the one recorded in triage defer comment 5311584017 (deferred to @wensao): (A) revert to the full wipe, or (B) keep the .git only behind the hardened preamble (remove commondir/shallow/config.worktree/config.lock + unset extensions.worktreeConfig before the sweep; narrow remote\. to remote\..+\.(url|fetch|pushurl); drop worktree from the core\. arm; fail-loud post-state audit) plus a fail-closed integrity pass. Both directions are contested scope — A abandons this PR's purpose, B adds substantial security machinery — and the triage record shows the maintainer's Aug-16 position ("still recommend merging") reads as accepting the residual risk, so this loop implements neither without an answer. No code change this round; the decision is not preempted.
Question for the maintainer: (A) revert to the full wipe, (B) keep-and-harden behind the preamble above, or (C) record a risk acceptance for the current design and merge as-is?
中文说明
处置：已升级至维护者决策——线程有意保持未解决，不单方面定夺。
已在被评审提交核实：HEAD 为 51777f43a2，三个 PR 文件的 git diff 4fd6a7aa..HEAD 为空——scrub 与本轮探针重新证明五个绕过所针对的代码逐字节一致，因此该发现依据代码同一性成立，本轮无需重新探测。
待定决策即 triage 延迟评论 5311584017（移交 @wensao）所记录者：(A) 恢复全量清理，或 (B) 仅在强化前置步骤后保留 .git（清扫前删除 commondir/shallow/config.worktree/config.lock 并取消 extensions.worktreeConfig；将 remote\. 收窄为 remote\..+\.(url|fetch|pushurl)；从 core\. 分支移除 worktree；失败即报错的状态后审计），并加失败即关闭的完整性检查。两个方向均属有争议的范围选择——A 放弃本 PR 主旨，B 增加大量安全机制——且 triage 记录显示维护者 8 月 16 日立场（"仍然建议合并"）倾向于接受残余风险，故本循环在未获答复前不实施任一方向。本轮无代码改动；未抢占决策。
给维护者的问题：(A) 恢复全量清理，(B) 按上述前置步骤保留并加固，还是 (C) 书面记录对当前设计的风险接受并按现状合并？
| # scrubbed to the same allowlist as qwen-triage.yml's config- | ||
| # sanitize (git's exec knobs are an open-ended class; an allowlist | ||
| # closes the class instead of denylisting knobs one by one). | ||
| # Objects and refs — the reason the .git is kept — are untouched, |
There was a problem hiding this comment.
[Critical] R8-9 (class finding, folds R8-10): still stands at the reviewed commit 51777f43 — "untouched" here means "trusted": the kept .git's objects and refs are attacker-writable, and downstream pool jobs consume them with no integrity check. The wipe tail is byte-identical to rounds 8–19 (this round merged main only — git diff 4fd6a7aa..HEAD over the three PR files is empty); it touches only hooks/info/attributes and config keys, while refs/ and objects/ survive by design, and nothing anywhere in .github removes refs/replace, gates reuse on git fsck, or sets GIT_NO_REPLACE_OBJECTS (grep at HEAD: zero matches). Both round-8 content-substitution channels were re-proven end-to-end this round with flips: (1) refs/replace — a planted refs/replace/<sha> survives the scrub; git checkout --force <pinned-sha> materializes attacker content while rev-parse HEAD still prints the clean pinned SHA (flip: GIT_NO_REPLACE_OBJECTS=1 → clean); actions/checkout v6.0.3 (pinned df4cb1c0) has zero replace handling and its reuse path deletes only refs/heads/*, so ci.yml's verify-checkout-head guard — which compares rev-parse HEAD — is blind by construction; (2) pack-index forgery — one 4-byte idx offset edit makes git serve attacker content for a known blob SHA on the normal read path with no re-hash (forged cat-file → EVIL, forced checkout → EVIL; flip: restore idx → clean); the forged-loose-object variant flips the same way. The surface (objects + refs of a kept attacker-writable repo — including objects/info/alternates, which also survives the wipe) cannot be enumerated entrance by entrance; it closes only structurally. Failure scenario: a malicious PR author (same-repo or write-access fork — the pool's admitted population) plants git replace <head-sha> <evil> and/or a forged idx entry during a serve-ab job; nothing on the pool ever removes it; a later pool job root-checking-out that SHA — ci.yml's root checkout (zero sanitization) or qwen-autofix.yml's "Checkout trusted base" (base_sha is PR-author-known) whose later steps carry CI_DEV_BOT_PAT — builds/executes attacker content disguised as the pinned SHA; the kept store holds full history, so replace refs and forged objects can target any blob/tree SHA future trees reuse (package.json, CI scripts executed by npm ci).
Witness (probes, this round, with flips):
refs/replace: post-scrub refs/replace/<sha> present; rev-parse HEAD = clean pinned SHA;
git checkout --force <sha> -> attacker file content
FLIP GIT_NO_REPLACE_OBJECTS=1 -> clean content
pack-idx: 4-byte offset edit -> cat-file blob <sha> = EVIL, checkout = EVIL
FLIP restore idx -> clean
loose-object forgery -> EVIL; FLIP -> clean
Same pending maintainer decision as R9-1 (triage defer comment 5311584017): treat objects+refs as untrusted before the kept repo is handed to the next job — delete refs/replace (loose + packed-refs), sweep non-essential ref namespaces, gate reuse on git fsck --strict with rm -rf of the kept .git on failure, plus consumer-side GIT_NO_REPLACE_OBJECTS=1 for SHA-pinned checkouts — or revert to the full wipe.
— qwen3.8-max via Qwen Code /review (v0.22.0)
There was a problem hiding this comment.
Disposition: escalated — same pending maintainer decision as the R9-1 thread; deliberately left unresolved.
Verified at the reviewed commit: HEAD is 51777f43a2 and git diff 4fd6a7aa..HEAD over the three PR files is empty — the wipe tail is byte-identical to the head this round's probes re-proved the refs/replace and pack-index channels on, so the finding stands by code identity. It closes only structurally (objects and refs of the kept attacker-writable repo cannot be sanitized entrance by entrance), and the choice is the same contested one recorded in triage defer comment 5311584017: revert to the full wipe, or keep the .git and treat objects+refs as untrusted before handoff (delete refs/replace loose + packed, sweep non-essential ref namespaces, gate reuse on git fsck --strict with rm -rf of the kept .git on failure, plus consumer-side GIT_NO_REPLACE_OBJECTS=1 for SHA-pinned checkouts). The consumer-side arm lives in other workflows outside this PR's footprint, which this loop may not modify; if the maintainer chooses keep-and-harden, that part becomes tracked follow-up work. No code change this round; the decision is not preempted.
Question for the maintainer: same as the R9-1 thread — (A) revert to the full wipe, (B) keep-and-harden with the integrity pass above, or (C) record a risk acceptance and merge as-is?
中文说明
处置：已升级——与 R9-1 线程等待同一维护者决策；有意保持未解决。
已在被评审提交核实：HEAD 为 51777f43a2，三个 PR 文件的 git diff 4fd6a7aa..HEAD 为空——wipe 尾部与本轮探针重新证明 refs/replace 与 pack-index 通道所针对的代码逐字节一致，因此该发现依据代码同一性成立。它只能结构性关闭（保留的、攻击者可写的仓库的 objects 与 refs 无法逐入口净化），而选择与 triage 延迟评论 5311584017 所记录者为同一个有争议的决策：恢复全量清理，或保留 .git 并在移交前将 objects+refs 视为不可信（删除松散与 packed 的 refs/replace、清扫非必要 ref 命名空间、以 git fsck --strict 把关复用并在失败时 rm -rf 保留的 .git，外加消费端在 SHA 锚定检出时使用 GIT_NO_REPLACE_OBJECTS=1）。消费端一环位于本 PR 足迹之外的其他工作流中，本循环不得修改；若维护者选择保留并加固，该部分将成为被跟踪的后续工作。本轮无代码改动；未抢占决策。
给维护者的问题：与 R9-1 线程相同——(A) 恢复全量清理，(B) 按上述完整性检查保留并加固，还是 (C) 书面记录风险接受并按现状合并？
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix review round — PR #9228: no action (all actionable Criticals are with a maintainer)Critical-only mode is active in this window. This round's actionable feedback carries three Critical findings — R9-1, R8-9, and R8-2 — plus a review whose six Suggestion-level items are confirmed already reported and not repeated. No failed checks, no still-red checks, no base conflict. No code changes this round: every actionable item is either already escalated for a maintainer decision or requires a maintainer action this loop cannot perform, and implementing one side unilaterally would preempt the pending decision. Evidence base verified locally this round
DispositionsR9-1 (inline rc:3840851642) — escalated, thread left openConfig-scrub bypasses on the kept R8-9 (inline rc:3840851644) — escalated, same pending decision, thread left openObjects/refs of the kept R8-2 (review rv:5004710324) — maintainer action required, cannot be done in this modeThe live PR body/title still describe the superseded round-1 two-directory wipe and carry none of the template headings. Address-review mode has no GitHub write that can edit a PR body (the workflow consumes a prepared body only on the issue-fix/PR-creation path), so this remains a maintainer-only action, exactly as triage defer comment 5311584017 records. The round-11 paste-ready template-conforming draft is already in the thread; if the pending R9-1/R8-9 decision changes the design, the body must be re-derived from it. Non-Critical items — audit record, untouchedThe deferred non-Critical section is an audit record, not work. The six Suggestion-level items are confirmed already reported and sit in their recorded states: the allowlist-parity pin is deferred pending the R9-1 decision; the others remain on the round-13/14/17 deferral lists. Nothing new to act on this round. Open question for the maintainer (rides along until answered)For the kept- 中文说明Autofix 评审轮次 — PR #9228：无需改动（所有可处理的 Critical 均已移交维护者）本窗口处于仅处理 Critical 的模式。本轮可处理反馈包含三个 Critical 发现——R9-1、R8-9、R8-2——以及一份评审，其中六个 Suggestion 级条目已确认此前报告过、不再重复。无失败检查、无持续红色检查、无基分支冲突。本轮无代码改动：每个可处理条目要么已升级等待维护者决策，要么需要本循环无法执行的维护者操作；单方面实施任一方向都会抢占待定决策。 本轮本地核实的证据基础
处置R9-1（行内 rc:3840851642）——已升级，线程保持开放保留的 R8-9（行内 rc:3840851644）——已升级，同一待定决策，线程保持开放保留的 R8-2（评审 rv:5004710324）——需维护者操作，本模式无法完成线上 PR 正文/标题仍描述被取代的第 1 轮"双目录"清理，且缺少全部模板标题。address-review 模式没有任何可以编辑 PR 正文的 GitHub 写操作（工作流仅在 issue 修复/创建 PR 路径上消费准备好的正文），因此这仍是只能由维护者完成的操作，与 triage 延迟评论 5311584017 的记录一致。第 11 轮已在线程中给出可直接粘贴的模板合规草稿；若待定的 R9-1/R8-9 决策改变设计，正文需从该草稿重新推导。 非 Critical 条目——审计记录，不处理"延迟的非 Critical 反馈"一节是审计记录，不是工作。六个 Suggestion 级条目已确认此前报告过，并处于各自记录的状态：允许清单对等锚定因等待 R9-1 决策而延迟；其余仍在第 13/14/17 轮延迟清单上。本轮没有新的可处理内容。 给维护者的未决问题（在得到答复前随每轮带出）关于保留 Deferred non-Critical feedbackCritical-only mode is active: 5 change-producing rounds are complete. The workflow excluded the non-Critical feedback below from this round's actionable sections; the items remain open for human follow-up. Maintainer feedback is deferred only after its author has used 2 regular feedback batches in this window's Critical-only tail; authors at that budget, if any, are named below. ( 中文说明已进入仅处理 Critical 的模式：已完成 5 个产生改动的轮次。本轮可执行区域已排除下方非 Critical 反馈；这些条目保持开放，留待人工跟进。维护者反馈仅在其本人于本窗口 Critical-only 阶段已使用 2 批常规反馈预算后才会延后；达到预算的作者（如有）在下方点名。（评论 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
Local real-environment re-verification at
|
| # | finding | severity (prev) | status at head 51777f43 |
|---|---|---|---|
| F1 | .git/config.worktree exec vectors survive the scrub (worktreeConfig split-config bypass); planted post-checkout hook fires on the next job's checkout |
concrete security finding | stands, re-measured. The two fix lines (present in qwen-triage.yml 2711–2712 at this head) are still absent from the wipe; cell H re-measures the bypass and the E2E hook fire end-to-end; cell I re-runs the oracle; cells J/J2/J3 re-verify the measured fix. |
| N1 | after the guard heals a symlinked workspace root, the config scrub operates on the link's target repo | low | stands, re-measured. Cell D vs cell K: the outside-workspace write is still introduced by this PR's tail, not the shared guard. |
| note | submodule.<name>.url stays allowlisted |
note | stands by design — allowlist regex re-asserted byte-identical to both qwen-triage config-sanitize sites. |
| — | PR body still describes the v1 rm -rf head/ base/ approach |
correction | stands — body unchanged. |
| — | "find -H fixes the symlinked-root sweep" |
superseded | superseded by design (unchanged) — mechanism is the guard's heal + realpath -m canonicalization; re-proven (cells C, D). |
Central claim + A/B table — wipe run: blocks extracted verbatim via YAML parse (raw/wipe-base.sh, raw/wipe-head.sh; extraction asserts guard-equal / tail-differs: 39 shared executed guard lines, base tail = whole-workspace find, head tail = the 3 narrowed lines). Each script ran under bash --noprofile --norc -eo pipefail with GIT_CONFIG_GLOBAL/SYSTEM=/dev/null against fresh fixtures (shared root repo with a commit, 9 plumbing keys, 14 hostile exec-vector keys, hooks/+info/attributes, head/, base/, tmp_pack_*, dotfiles):
| cell | script | workspace shape | .git after |
oracle result |
|---|---|---|---|---|
| A | base | real dir | destroyed | predicted base failure → assertion passed |
| B | head | real dir | kept; HEAD + objects intact; 14 hostile keys unset; 9 plumbing keys kept; hooks + info/attributes gone; exactly .git remains |
central claim holds |
| C | head | non-canonical $RWS/./repo |
kept + scrubbed | canonicalization reaches wipe + scrub |
| E | head | fresh runner, no .git |
n/a | exit 0, clean stderr |
| F | head | .git is a symlink |
symlink removed; outside repo untouched | only a REAL dir survives |
| G | head | .git is a gitfile |
gitfile removed; target untouched | same |
| D | head | root is a symlink | healed to empty real dir; target repo scrubbed (N1) | heal path; scrub-cwd observation |
| K | base | root is a symlink | healed | contrast: target untouched — N1 introduced by the PR's tail |
Mutation matrix — the same 13 single-point mutants as the previous round, re-applied through YAML-object mutation and run against both committed suites in scratch trees: all 13 killed (M01/M02/M03/M09/M10/M12/M13 by both suites; M04/M05/M06/M07/M08/M11 by the routing pin only — attribution unchanged). Vacuity quotes: M01's vitest failure is expected [] to deeply equal [ '.git' ] (the kept-.git behavior itself); M13's is expected [ …(3) ] to have a length of 1 but got 3 (the || true contract) — adopting F1's fix forces a deliberate pin update, as the pins' design documents. Controls: unmutated head green/green; base YAML kills both new suites; head YAML kills both old suites; base YAML + old tests green/green.
F1 (carried, stands) — suggested fix, re-measured at this head (insert between the defang rm and the allowlist pipeline; mirrors qwen-triage's hardened site):
rm -f "$(git rev-parse --git-path config.worktree 2>/dev/null || echo /nonexistent)" 2>/dev/null || true
git config --local --unset-all extensions.worktreeConfig 2>/dev/null || trueThree measured results (cells J/J2/J3): (1) hostile+split fixture clean — config.worktree deleted, extension unset, E2E checkout no longer fires the planted hook; (2) benign fixture byte-identical (.git/config sha256-equal pre/post, exit 0, clean stderr); (3) fresh-runner no-.git shape stays exit 0.
Not covered — uncalibrated replay (no real ECS-pool wipe-step job log retrievable; harnesses execute the verbatim extracted scripts in the same container class); per-commit attribution (aggregate three-file diff fully verified; intermediate designs covered by the Aug 16/17/23 rounds at their heads); actions/checkout v6 reuse behavior on the kept repo (offline; the E2E reproduces the wire shape, not the action itself); yamllint (scripts/lint.js --setup exits 1 in-container; actionlint + both suites' yaml parsing cover the structural gate); the body's ECS-pool measurements (motivation, no API); runner chdir on a dangling root symlink (inferred fail-closed). Environmental note, A/A-styled: npm ci's prepare build was OOM-killed by the container memory limit (tsc exit 137) — the PR touches no TypeScript and no gate consumes built packages; deps reinstalled clean with --ignore-scripts.
Methodology — macOS host; all execution inside a credential-free node:22-bookworm container (node 22.23.2, git 2.39.5, GNU realpath 9.1). Base = worktree at 747dbf0 (PR baseRefOid), head = worktree at 51777f43 (PR headRefOid), both verified with git cat-file. Scripts extracted with the repo's yaml parser (22 structural assertions incl. allowlist parity and the F1-fix-absent precondition); A/B harness 138 assertions (raw/ab-run1.log); mutation matrix 29 assertions (raw/mutate-run1.log); gates 21 assertions with planted-violation liveness probes (raw/gates-run4.log) — an initially vacuous shellcheck probe (binary absent, exit 127 counted as "reported") was caught and fixed before any green was cited. The serve-ab vitest suite was driven as a non-root verifier user so the skipIf(uid===0) chmod-fixture test executes (root: 20 passed | 1 skipped; verifier: 21/21). Evidence captures via the repo's scripts/verify-capture.mjs. Full report, harnesses, and raw logs: tmp/pr9228-verify-20260824-131001/.
Evidence images
— wenshao · local verification round
…e scrub (#9228) The kept-.git wipe tail had two measured holes: extensions.worktreeConfig activates .git/config.worktree, a second local file that git config --local neither lists nor unsets, so a planted core.hooksPath survived the allowlist sweep; and after the heal unlinks a symlinked workspace root the step's CWD still is the link's target, so the CWD-discovered scrub wrote outside the workspace. Add qwen-triage's hardened defang pair (delete config.worktree, unset the extension) and anchor every git call to $WS/.git. Both findings reproduced locally before the fix; both suites pin the new lines and are mutation-tested.
|
🤖 Addressed the latest review feedback (round 9/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 9/100 轮）。改动内容与我反驳保留之处如下： Autofix review round — PR #9228 (address-review)Round input: one issue-level comment — the maintainer's local re-verification Feedback triage
Changes
Mutation probes (every added guard has its own witness)
Verification
Not run (binaries unavailable on this runner without network installs): Commit: 中文说明Autofix 审查轮次 — PR #9228（address-review）本轮输入：一条 issue 级评论 —— 维护者在 head 反馈分类
变更内容
变异探针（每个新增守卫都有自己的见证）
验证
未执行（本 runner 无法在不联网安装的情况下获得这些二进制）：actionlint / 提交： Deferred non-Critical feedbackCritical-only mode is active: 5 change-producing rounds are complete. The workflow excluded the non-Critical feedback below from this round's actionable sections; the items remain open for human follow-up. Maintainer feedback is deferred only after its author has used 2 regular feedback batches in this window's Critical-only tail; authors at that budget, if any, are named below. ( 中文说明已进入仅处理 Critical 的模式：已完成 5 个产生改动的轮次。本轮可执行区域已排除下方非 Critical 反馈；这些条目保持开放，留待人工跟进。维护者反馈仅在其本人于本窗口 Critical-only 阶段已使用 2 批常规反馈预算后才会延后；达到预算的作者（如有）在下方点名。（评论 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/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
3 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- cross-copy allowlist parity pin for the serve-ab scrub (agent 3a this round) — already reported as R8-5 (comment 3792902589); the author deferred it pending the maintainer decision on the R9-1 thread, which is still open
- exec witness for the -type d keep-predicate negative branch — a symlink/gitfile .git (agent 5 this round) — already recorded in the round-12 and round-14 deferral lists (serve-ab-workflow.test.js:116/131)
- scrub breadcrumb logging for hung-runner forensics (agent 6b this round) — already recorded in the round-13 deferral list (serve-ab.yml:99)
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.
[Critical] R8-2 (carried from round 8; R2-11/R5-5/R6-2/R7-2 lineage): the live PR body still does not follow the PR template — free-form sections (## Why / ## What changes / ## Verification / ## Not in this PR); none of the template's required sections present (What this PR does / Why it's needed / Reviewer Test Plan with How to verify, Evidence (Before & After), Tested on matrix / Risk & Scope / Linked Issues / Chinese
Details
translation — deterministic check at this head: 0 of 9 present) — and is materially stale: '## What changes' still quotes the superseded two-directory rm (rm -rf "${GITHUB_WORKSPACE:?}/head" "${GITHUB_WORKSPACE:?}/base", 'the wipe now removes exactly those two directories') while the shipped code keeps the shared root .git, defangs it, and scrubs its config — the design the open R9-1/R8-9 maintainer decision is about. Round 21 additionally rewrote the exit-contract assertion (not.toContain('|| true') → exactly three identified || true lines) without the body declaring the rewrite. The open triage CHANGES_REQUESTED (review 4943936576) persists; the autofix loop cannot reformat the body (no GitHub write access in that mode), so a maintainer must reformat it describing the shipped keep-and-defang behavior (a paste-ready template-conforming draft was posted in round 11; it must be re-derived if the pending R9-1/R8-9 decision changes the design). Merging on the current description would put a materially different change into the history than the body documents. Witness: live body fetched this round via gh pr view 9228 --json body (headings as listed; stale quote present; deterministic test-plan gate: 'the PR description has no Test Plan section').
— qwen3.8-max via Qwen Code /review (v0.22.0)
| rm -rf "$WS/.git/hooks" "$WS/.git/info/attributes" | ||
| rm -f "$(git --git-dir="$WS/.git" rev-parse --git-path config.worktree 2>/dev/null || echo /nonexistent)" 2>/dev/null || true | ||
| git --git-dir="$WS/.git" config --local --unset-all extensions.worktreeConfig 2>/dev/null || true | ||
| { git --git-dir="$WS/.git" config --local --name-only --list 2>/dev/null || true; } | { grep -ivE '^(core\.(repositoryformatversion|bare|filemode|symlinks|ignorecase|precomposeunicode|logallrefupdates|worktree|hidedotfiles|protecthfs|protectntfs)|remote\.|branch\.|extensions\.|gc\.|pack\.|fetch\.|index\.|safe\.|submodule\.[^.]+\.(url|active|branch))' || true; } | while IFS= read -r key; do git --git-dir="$WS/.git" config --local --unset-all "$key" 2>/dev/null || true; done |
There was a problem hiding this comment.
[Critical] R9-1: still stands at the reviewed commit 9b5b1348 — the kept .git is attacker-writable state handed to later, more privileged pool jobs, and this scrub sanitizes it entrance by entrance over a surface that cannot be enumerated. This round's commit closes the worktreeConfig entrance (the maintainer-measured F1 defang pair — probe-verified closed this round, benign on clean and no-.git shapes) and anchors every git call to $WS/.git (N1), but the class remains open, and two entrances demonstrated this round defeat the round's own containment claim:
commondirentrance — a planted$WS/.git/commondir(kept by the wipe) redirects every anchoredconfig --localcall to a repo outside the workspace: the sweep wrote the outside repo's config while exec keys planted in the real$WS/.git/configsurvived, and hooks resolved viaGIT_COMMON_DIRpast the hooks-dir deletion and fired on a consumer checkout — the exact write-outside-workspace the new anchoring comment and thenever scrubs a repo outside a healed workspacetest assert is impossible.resanitize-git-config.shremovescommondir/shallowfirst for exactly this reason; this tail omits that half.*.lockentrance — a plantedconfig.lock, or one left stale byconcurrency: cancel-in-progress+ SIGKILL/OOM mid git-write, makes every--unset-alldie with "could not lock config file", swallowed by|| true: the scrub exits 0 having unset nothing, and stalerefs/**/*.lock/index.lockadditionally wedge every later job's checkout until manual cleanup on the runner.
Prior-round entrances also remain untouched: allowlisted core.worktree; the allowlisted repositoryformatversion + extensions.* self-shielding pair; the broad remote\. arm.
Failure scenario: a prior pool job (runs contributor code via npm ci) plants $WS/.git/commondir pointing at a decoy gitdir carrying hooks/post-checkout — or simply leaves a config.lock after a cancelled run; the wipe keeps everything under .git, the anchored scrub scrubs the wrong repo or silently no-ops, and the next root-checkout consumer on the pool (ci.yml:240, qwen-code-pr-review.yml:546 — both carrying write tokens) fires the planted hook or wedges on the stale lock.
Witness (probe this round, verbatim five-line tail under bash -eo pipefail, with flips):
commondir arm: TAIL EXIT CODE: 0; decoy hooks/post-checkout still executable: YES;
real-config planted keys STILL PRESENT; sweep scrubbed the DECOY config;
consumer checkout: >>> HOOK FIRED
flip (no commondir): planted keys unset; hook did not fire (contained)
lock arm: 'error: could not lock config file ... File exists'; TAIL EXIT CODE: 0;
alias.pwned survived; >>> HOOK FIRED; flip (no lock): keys gone, contained
operational arm: refs/heads/master.lock + index.lock survive the tail; next-job
update-ref: fatal: cannot lock ref ... Another git process seems to be running (exit 128)
F1-closure arm: config.worktree deleted, extension unset, hooksPath unresolvable,
hook did not fire
Suggested fix (spans the workflow and the pinned test, so regular block): adopt the hardened preamble the repo's own resanitize-git-config.sh carries before its sweep —
rm -f "$WS/.git/commondir" "$WS/.git/shallow" "$WS/.git/config.lock"
find "$WS/.git" -name '*.lock' -deleteahead of the defang pair, then add a post-state audit that fails the wipe when any non-allowlisted key remains, so a future defeat is loud instead of silent; update the pinned executed-line set in ci-runner-routing.test.mjs deliberately in the same change. Structural close (the pending maintainer decision on this thread): gate the kept .git on a fail-closed integrity pass, or stop retaining it across untrusted-code runs.
— qwen3.8-max via Qwen Code /review (v0.22.0)
There was a problem hiding this comment.
No code change this round, and this thread stays open deliberately: R9-1 remains escalated to the maintainer decision requested since round 9. The round-22 probes re-confirm the class claim — the arm added in round 21 was defeated by the commondir / *.lock / symlinked-config entrances — so this round adds no further arms: more entrances on the contested design would grow the diff without closing the class. The decision this thread waits on:
- (A) revert the wipe to the full wipe, including the shared root
.git, or - (B) keep the
.gitonly behind a fail-closed integrity pass (remove the redirect/lock plumbingcommondir/shallow/config.lock/gitdirand every*.lock,rm -rf .git/worktrees, dropcore.worktreefrom the allowlist, narrowremote\., reject a symlinked.git/config, and fail the wipe when any non-allowlisted state remains — plus the R8-9 gate on objects/refs).
A maintainer answering (A) or (B) here unblocks the next round.
中文说明
本轮无代码改动，本线程有意保持开放：R9-1 仍处于自 round 9 起请求的维护者决策等待中。round-22 的探针再次证实了类级指控 —— round 21 新增的防线被 commondir / *.lock / 符号链接 config 入口击穿 —— 因此本轮不再添加任何新防线：在有争议的设计上继续加入口只会增大 diff，并不能关闭这个类。本线程等待的决策：
- (A) 将 wipe 回退为包含共享根
.git在内的完整清除，或 - (B) 仅在失败即关闭（fail-closed）的完整性检查之后保留
.git（移除重定向/锁管道commondir/shallow/config.lock/gitdir及所有*.lock，rm -rf .git/worktrees，将core.worktree移出白名单，收窄remote\.，拒绝符号链接的.git/config，并在存在任何非白名单状态时使 wipe 失败 —— 外加 R8-9 对 objects/refs 的门禁）。
维护者在此回答 (A) 或 (B) 即可解锁下一轮。
| # the repo from the CWD: after the heal above unlinks a symlinked | ||
| # workspace root, the step's CWD still is the link's target, and | ||
| # CWD discovery would scrub a repo outside the workspace. | ||
| find "$WS" -mindepth 1 -maxdepth 1 ! \( -name '.git' -type d \) -exec rm -rf {} + |
There was a problem hiding this comment.
[Critical] R8-9 (class finding, folds R8-10): still stands at the reviewed commit 9b5b1348 — "untouched" here means "trusted": the kept .git's objects and refs are attacker-writable, and downstream pool jobs consume them with no integrity check. This round's commit (worktreeConfig defang pair + --git-dir anchoring) touches neither refs/ nor objects/ — by design, they are why the .git is kept — and no refs/replace removal, git fsck gate, or GIT_NO_REPLACE_OBJECTS exists anywhere under .github/ (tree-wide grep at HEAD: none found). Both round-8 content-substitution channels therefore remain open: (1) refs/replace silently rewrites object resolution — a later SHA-pinned root checkout materializes attacker content while rev-parse HEAD still prints the original SHA; (2) pack-index forgery serves attacker blob content under a pinned SHA through the full consumer sequence. The surface (objects + refs of a kept attacker-writable repo) cannot be enumerated entrance by entrance — it closes only structurally.
Failure scenario: a malicious PR author (same-repo or write-access fork — the pool's admitted population) plants git replace <head-sha> <evil> and/or a forged idx entry during a serve-ab job; nothing on the pool ever removes it; a later pool job root-checking-out that SHA — another workflow on the same PR, or the autofix "Checkout trusted base" which later stages a PAT push — builds/executes attacker content disguised as the pinned SHA.
Witness: round-8 probes (quoted on this thread): refs/replace: after verbatim wipe: ALIVE, checkout materialized the PWNED content while rev-parse HEAD printed the original GOOD sha; idx forgery: FULL CONSUMER SEQUENCE MATERIALIZED ATTACKER CONTENT with the verify-checkout-head guard passing on the forged store. This round: grep -rnE 'no-replace-objects|GIT_NO_REPLACE_OBJECTS|refs/replace|git fsck' .github/ scripts/ at HEAD returned nothing, and the diff touches neither .git/objects/ nor .git/refs/ handling.
Suggested fix (spans the workflow and the pinned test, so regular block): treat objects+refs as untrusted before the kept repo is handed to the next job — delete refs/replace (loose + packed-refs filtering), sweep non-essential ref namespaces (refs are re-established by fetch; objects are what make keeping .git worthwhile), and gate reuse on git fsck --strict with rm -rf of the kept .git on failure — accepting the one-time re-fetch cost; complement with GIT_NO_REPLACE_OBJECTS=1 for SHA-pinned consumer checkouts. If that cannot be done soundly, revert to the full wipe for the .git as well (option A of the pending maintainer decision shared with the R9-1 thread).
— qwen3.8-max via Qwen Code /review (v0.22.0)
There was a problem hiding this comment.
No code change this round, and this thread stays open deliberately: R8-9 closes only via the same pending maintainer decision as R9-1 — (A) the full wipe (the kept objects/refs go away with it), or (B) a fail-closed integrity gate on the kept .git (refs/replace removal from loose refs and packed-refs, git fsck --strict, and rm -rf of the kept .git on failure). The round-22 probes re-confirm both content-substitution channels are open at this head, and the class cannot be closed entrance by entrance — adding scrub arms here would be wasted work under decision (A) and incomplete under decision (B). A maintainer answering on the R9-1 thread (shared decision) unblocks both findings.
中文说明
本轮无代码改动，本线程有意保持开放：R8-9 只能通过与 R9-1 相同的、悬而未决的维护者决策关闭 —— (A) 完整清除（保留的 objects/refs 一并消失），或 (B) 对保留的 .git 施加失败即关闭的完整性门禁（从松散 refs 与 packed-refs 中移除 refs/replace、git fsck --strict、失败时 rm -rf 保留的 .git）。round-22 的探针再次证实两条内容替换通道在当前 head 上均为开放状态，且该类无法逐入口关闭 —— 在此添加清扫防线在决策为 (A) 时是无效劳动，在决策为 (B) 时也不完整。维护者在 R9-1 线程上作答（同一决策）即可同时解锁两条发现。
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
4 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- cross-copy allowlist parity pin for the serve-ab scrub (agents 3a/6c this round) — already reported as R8-5 (comment 3792902589); the author deferred it pending the maintainer decision on the R9-1 thread, which is still open
- exec witness for the -type d keep-predicate negative branch — a symlink/gitfile .git (agent 5 this round) — already recorded in the round-12 and round-14 deferral lists (serve-ab-workflow.test.js:116/131)
- scrub breadcrumb logging for hung-runner forensics (agent 6b this round) — already recorded in the round-13 deferral list (serve-ab.yml:99)
- defang rm -rf following a symlinked .git/info to delete an external attributes file (agent 6a this round) — already recorded in the round-13 deferral list (serve-ab.yml:143), probe-verified, impact constrained
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.
[Critical] R8-2 (carried from round 8; R2-11/R5-5/R6-2/R7-2 lineage): still stands at the reviewed commit 8306207 — the live PR body still does not follow the PR template (free-form sections ## Why / ## What changes / ## Verification / ## Not in this PR; deterministic heading inventory at this head: 0 of the 9 required template sections present — What this PR does / Why it's needed / Reviewer Test Plan with How to verify, Evidence (Before & After), Tested on matrix / Risk & Scope / Linked Issues / Chinese
Details
translation) and is materially stale: '## What changes' still quotes the superseded two-directory rm (rm -rf "${GITHUB_WORKSPACE:?}/head" "${GITHUB_WORKSPACE:?}/base", 'the wipe now removes exactly those two directories') while the shipped code keeps the shared root .git, defangs it, and scrubs its config — the design the open R9-1/R8-9 maintainer decision is about. The open triage CHANGES_REQUESTED (review 4943936576) persists; the autofix loop cannot reformat the body (no GitHub write access in that mode), so a maintainer must reformat it describing the shipped keep-and-defang behavior (a paste-ready template-conforming draft was posted in round 11; it must be re-derived if the pending R9-1/R8-9 decision changes the design). Merging on the current description would put a materially different change into the history than the body documents. Witness: live body fetched at head 8306207 this round (heading inventory as listed; stale quote present; deterministic test-plan gate: 'the PR description has no Test Plan section').— qwen3.8-max via Qwen Code /review (v0.22.0)
| rm -rf "$WS/.git/hooks" "$WS/.git/info/attributes" | ||
| rm -f "$(git --git-dir="$WS/.git" rev-parse --git-path config.worktree 2>/dev/null || echo /nonexistent)" 2>/dev/null || true | ||
| git --git-dir="$WS/.git" config --local --unset-all extensions.worktreeConfig 2>/dev/null || true | ||
| { git --git-dir="$WS/.git" config --local --name-only --list 2>/dev/null || true; } | { grep -ivE '^(core\.(repositoryformatversion|bare|filemode|symlinks|ignorecase|precomposeunicode|logallrefupdates|worktree|hidedotfiles|protecthfs|protectntfs)|remote\.|branch\.|extensions\.|gc\.|pack\.|fetch\.|index\.|safe\.|submodule\.[^.]+\.(url|active|branch))' || true; } | while IFS= read -r key; do git --git-dir="$WS/.git" config --local --unset-all "$key" 2>/dev/null || true; done |
There was a problem hiding this comment.
[Critical] R9-1 (class finding; supersedes R8-1 — its config.worktree/commondir entrances fold in as evidence): still stands at the reviewed commit 83062077 — the kept .git is attacker-writable state handed to later, more privileged pool jobs, and this scrub sanitizes it entrance by entrance over a surface that cannot be enumerated — git's config namespace, the git-dir's files, and the repo's shape. The wipe tail is byte-identical to rounds 8–21 (this head is a merge of main only), and the round-9 defang of extensions.worktreeConfig/config.worktree closed one entrance while the class stands. Re-probed this round against the verbatim script (git 2.43.0, extracted at this commit, under the job's bash -eo pipefail): a planted .git/config.lock swallows every --unset-all (wipe exit 0, planted exec keys survive); allowlisted core.worktree makes the pinned actions/checkout reuse run git clean -ffdx / git reset --hard against the planted worktree, deleting files outside the workspace; a planted .git/commondir redirects hook resolution and this sweep onto an attacker gitdir outside the workspace, and the next consumer checkout fires the planted hook through the verbatim five-line defang tail; and a .git/config replaced by a symlink to a parseable file outside the workspace makes this sweep's own --unset-all writes follow the link and rewrite the outside file (probe: victim emptied, 4 keys → 0, symlink survived, wipe exit 0). A planted allowlisted core.repositoryformatversion=999 additionally blinds the sweep's own --list (exit 128 swallowed) and self-shields co-planted keys. Escalated to a maintainer decision since round 9 — (A) revert to the full wipe, or (B) keep the .git only behind a fail-closed integrity pass (rm -f the redirect/lock plumbing commondir/shallow/config.lock/gitdir, rm -rf .git/worktrees, drop worktree from the allowlist, narrow remote., enumerate via git config --file, reject a symlinked .git/config with test -L, and fail the wipe when any is present); this thread is deliberately left open until that decision lands.
— qwen3.8-max via Qwen Code /review (v0.22.0)
There was a problem hiding this comment.
No code change this round, and this thread stays open deliberately: R9-1 remains escalated to the maintainer decision requested since round 9. The round-22 probes re-confirm the class claim — the arm added in round 21 was defeated by the commondir / *.lock / symlinked-config entrances — so this round adds no further arms: more entrances on the contested design would grow the diff without closing the class. The decision this thread waits on:
- (A) revert the wipe to the full wipe, including the shared root
.git, or - (B) keep the
.gitonly behind a fail-closed integrity pass (remove the redirect/lock plumbingcommondir/shallow/config.lock/gitdirand every*.lock,rm -rf .git/worktrees, dropcore.worktreefrom the allowlist, narrowremote\., reject a symlinked.git/config, and fail the wipe when any non-allowlisted state remains — plus the R8-9 gate on objects/refs).
A maintainer answering (A) or (B) here unblocks the next round.
中文说明
本轮无代码改动，本线程有意保持开放：R9-1 仍处于自 round 9 起请求的维护者决策等待中。round-22 的探针再次证实了类级指控 —— round 21 新增的防线被 commondir / *.lock / 符号链接 config 入口击穿 —— 因此本轮不再添加任何新防线：在有争议的设计上继续加入口只会增大 diff，并不能关闭这个类。本线程等待的决策：
- (A) 将 wipe 回退为包含共享根
.git在内的完整清除，或 - (B) 仅在失败即关闭（fail-closed）的完整性检查之后保留
.git（移除重定向/锁管道commondir/shallow/config.lock/gitdir及所有*.lock，rm -rf .git/worktrees，将core.worktree移出白名单，收窄remote\.，拒绝符号链接的.git/config，并在存在任何非白名单状态时使 wipe 失败 —— 外加 R8-9 对 objects/refs 的门禁）。
维护者在此回答 (A) 或 (B) 即可解锁下一轮。
| # the repo from the CWD: after the heal above unlinks a symlinked | ||
| # workspace root, the step's CWD still is the link's target, and | ||
| # CWD discovery would scrub a repo outside the workspace. | ||
| find "$WS" -mindepth 1 -maxdepth 1 ! \( -name '.git' -type d \) -exec rm -rf {} + |
There was a problem hiding this comment.
[Critical] R8-9 (class finding, folds R8-10): still stands at the reviewed commit 83062077 — "untouched" here means "trusted": the kept .git's objects and refs are attacker-writable, and downstream pool jobs consume them with no integrity check. The wipe tail is byte-identical to rounds 8–21 (this head is a merge of main only) and touches only hooks/info/attributes/config keys; refs/ and objects/ survive by design (the step's own comment: "Objects and refs — the reason the .git is kept — are untouched"). Both content-substitution channels re-probed this round against the verbatim script: (1) refs/replace — a planted refs/replace/ survives the wipe and silently rewrites object resolution, so a SHA-pinned root checkout materializes attacker content while rev-parse HEAD still prints the original SHA (probe: consumer HEAD reported the pinned sha while the checked-out file read "EVIL PAYLOAD — attacker code"; deleting the replace ref flips it back); actions/checkout v6.0.3 reuse verified against the pinned df4cb1c0 source — reuse on .git presence + origin-URL match, deletes only refs/heads/* and conflicting refs/remotes/origin/*, zero refs/replace handling; (2) pack-index forgery — git serves packed blobs by the name written in the idx without re-hashing on the checkout read path, so a forged idx entry makes attacker file content resolve under a pinned SHA through the full consumer sequence (probe: checkout of a commit naming blob 66cf1462 served "EVIL PAYLOAD VIA FORGED IDX", exit 0). Nothing under .github/ removes refs/replace (grep: zero hits), and the same pool carries PAT-bearing root-checkout consumers (qwen-autofix.yml "Checkout trusted base"). Closes only via the same pending maintainer decision as R9-1: revert to the full wipe, or a fail-closed integrity gate (refs/replace removal + git fsck --strict with rm -rf .git on failure).
— qwen3.8-max via Qwen Code /review (v0.22.0)
There was a problem hiding this comment.
No code change this round, and this thread stays open deliberately: R8-9 closes only via the same pending maintainer decision as R9-1 — (A) the full wipe (the kept objects/refs go away with it), or (B) a fail-closed integrity gate on the kept .git (refs/replace removal from loose refs and packed-refs, git fsck --strict, and rm -rf of the kept .git on failure). The round-22 probes re-confirm both content-substitution channels are open at this head, and the class cannot be closed entrance by entrance — adding scrub arms here would be wasted work under decision (A) and incomplete under decision (B). A maintainer answering on the R9-1 thread (shared decision) unblocks both findings.
中文说明
本轮无代码改动，本线程有意保持开放：R8-9 只能通过与 R9-1 相同的、悬而未决的维护者决策关闭 —— (A) 完整清除（保留的 objects/refs 一并消失），或 (B) 对保留的 .git 施加失败即关闭的完整性门禁（从松散 refs 与 packed-refs 中移除 refs/replace、git fsck --strict、失败时 rm -rf 保留的 .git）。round-22 的探针再次证实两条内容替换通道在当前 head 上均为开放状态，且该类无法逐入口关闭 —— 在此添加清扫防线在决策为 (A) 时是无效劳动，在决策为 (B) 时也不完整。维护者在 R9-1 线程上作答（同一决策）即可同时解锁两条发现。
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Round outcome: no changes — every Critical finding is blocked on a pending maintainer decision/actionThis round evaluated two review bodies and four inline Critical findings (the round-21 pair re-anchored at R8-2 — PR body does not follow the template and is materially stale (rv:5006447725, rv:5007139125)Disposition: escalated — needs a maintainer's GitHub write. The finding is real. The reviewer's probe fetched the live body at both heads and counted 0 of the 9 template-required sections present, and the staleness claim is corroborated locally: the body still quotes the superseded two-directory This round cannot fix it: editing the PR body is a GitHub write. This mode holds no GitHub credentials, and the address-review flow consumes no file that edits the PR body — Maintainer action needed: reformat the PR body into the template describing the shipped behavior (the round-11 paste-ready draft must be re-derived if the R9-1/R8-9 decision changes the design). R9-1 — the kept
|
Local real-environment re-verification at
|
| # | finding | severity (prev) | status at head 83062077 |
|---|---|---|---|
| F1 | .git/config.worktree exec vectors survive the scrub; planted post-checkout hook fires on the next job's checkout |
concrete security finding | fixed — serve-ab.yml lines 216–217 (mirroring qwen-triage.yml 2711–2712, plus --git-dir anchoring); cell H re-measures the bypass and the E2E hook fire end-to-end; M13 confirms removing the pair re-arms the hook; both suites pin the lines. |
| N1 | after the heal unlinks a symlinked root, the config scrub operates on the link's target repo | low | fixed — every scrub git call anchored to $WS/.git; cell D shows the target untouched (cell K: base arm identical); M05 confirms dropping the anchoring reintroduces the outside write; both suites pin it. |
| note | submodule.<name>.url stays allowlisted |
note | stands by design — allowlist re-asserted byte-identical to both qwen-triage config-sanitize sites. |
| — | PR body still describes the v1 rm -rf head/ base/ approach |
correction | stands — body unchanged; cosmetic. |
| — | "find -H fixes the symlinked-root sweep" |
superseded | unchanged by design — mechanism is the guard's heal + realpath -m; re-proven (cells C, D, K). |
Central claim + A/B table — wipe run: blocks extracted verbatim via the repo's yaml parser (raw/wipe-base.sh, raw/wipe-head.sh; extraction asserts guard-equal 39 shared executed lines / tail-differs), executed under bash --noprofile --norc -eo pipefail with hermetic GIT_CONFIG_*=/dev/null:
| cell | script | workspace shape | .git after |
oracle result |
|---|---|---|---|---|
| A | base | real shared repo | destroyed | predicted base failure → assertion passed |
| B | head | real shared repo | kept + usable | HEAD identical; objects intact; fsck clean; local clone succeeds at same HEAD; 14/14 hostile keys unset; 9/9 plumbing keys intact; hooks + info/attributes gone |
| H | head | split-config bypass (F1) | kept, defanged | config.worktree deleted, extension unset, E2E checkout fires no planted hook (positive control fired pre-wipe) |
| D | head | root is a symlink (N1) | healed to empty dir | outside target repo untouched — anchored scrub |
| K | base | root is a symlink | healed | target untouched (contrast: no scrub tail) |
| F | head | .git is a symlink |
removed | outside repo untouched incl. split config |
| G | head | .git is a gitfile |
removed | target untouched |
| C | head | $RWS/./repo spelling |
kept + scrubbed | canonicalization reaches wipe + scrub |
| E | head | no .git |
n/a | exit 0, clean output |
| J | head | benign repo | kept | .git/config sha256-identical pre/post |
| guard | both | 12 hostile paths + rm recorder |
n/a | 24/24 refusals, zero rm invocations |
Mutation matrix — 13 single-point mutants through the parsed YAML object in hardlink scratch trees (unlink-before-write so no in-place write reaches the pristine tree; a pristine-tree guard re-asserts cleanliness): M01 (find→whole-workspace), M02 (hooks rm), M03/M04 (each F1 half), M05 (all anchoring dropped), M06 (defang call un-anchored), M09 (rename), M12 (|| true on the find), M13 (both F1 lines) — all killed; M01/M02/M03/M04/M05/M06/M09/M12/M13 by both suites, M07 (allowlist widened to core.hooksPath)/M10 (-type d dropped)/M11 (tail swap) by the routing pin only; M08 (comment-only) survives by design. Controls: unmutated head green/green; base-YAML-in-head-tree red/red (expected -1 to be greater than or equal to 0); 3× repeat, no divergence. Behavior probes independent of the suites: M13 → planted hook fires again (F1 load-bearing); M05 → outside-repo key unset through CWD (N1 load-bearing).
Gates — routing 6/6 (node --test); vitest as root 22 passed + 1 by-design skip; as non-root verifier 23 passed (23) with 0 skips; bash -n both verbatim scripts; shellcheck 0.9.0 -s bash clean on both arms (liveness: planted violation reported); actionlint 1.7.12 green under the repo contract (-pyflakes= -shellcheck= + the five ignores).
Not covered — uncalibrated replay (no real ECS-pool wipe-step log retrievable; harnesses execute the verbatim scripts in the CI verify container class; the workflow run API would calibrate it); actions/checkout v6 reuse behavior on the kept repo (offline; the E2E proves clone/checkout usability, i.e. the wire shape, not the action); per-commit attribution (aggregate three-file diff fully verified; intermediates covered by prior rounds); yamllint (scripts/lint.js --setup has no linux/arm64 mapping); arm64 container vs the CI lane's x64 (same bookworm userland); npm ci --ignore-scripts (no gate here consumes built packages).
Methodology — macOS host; all execution inside a credential-free node:22-bookworm container (node 22.23.2, git 2.39.5, GNU realpath 9.1, bash 5.2.15); only the artifact dir and npm cache bind-mounted. Base 6a21c43, head 83062077, both verified with git cat-file. A/B 80 assertions (raw/ab-run1.log), mutations 17 (raw/mutate-run1.log), gates 9 (raw/gates-run4.log); evidence via the repo's scripts/verify-capture.mjs. Full report, harnesses, and raw logs: tmp/pr9228-verify-20260824-200108/.
Evidence images
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix round summary — no changes needed (PR #9228)No actionable feedback arrived for this round, so no commits were made. The branch remains at Feedback triage
Disposition of the one residual noteThe verification report's status table still lists one note — "PR body still describes the v1 中文说明Autofix 轮次总结 —— 无需改动（PR #9228）本轮没有收到任何可执行的反馈，因此未产生任何提交。分支保持在 反馈分类处理
唯一遗留备注的处理验证报告的状态表中仍列有一条备注 —— "PR 正文仍在描述 v1 的 Deferred non-Critical feedbackCritical-only mode is active: 5 change-producing rounds are complete. The workflow excluded the non-Critical feedback below from this round's actionable sections; the items remain open for human follow-up. Maintainer feedback is deferred only after its author has used 2 regular feedback batches in this window's Critical-only tail; authors at that budget, if any, are named below. ( 中文说明已进入仅处理 Critical 的模式：已完成 5 个产生改动的轮次。本轮可执行区域已排除下方非 Critical 反馈；这些条目保持开放，留待人工跟进。维护者反馈仅在其本人于本窗口 Critical-only 阶段已使用 2 批常规反馈预算后才会延后；达到预算的作者（如有）在下方点名。（评论 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
|
@qwen-code /triage |
|
Sandboxed verification: ❌ not passed — findings reported (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: 206 passed · 0 failed · 206 total Flakiness gate: ✅ 2 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：206 通过 · 0 失败 · 206 总计 抖动门：✅ 2 changed test file(s) x 5 identical rounds, no divergence Verification reportPR 9228 — sandboxed verification (round 3, follow-up)Verdict: 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：206 通过 · 0 失败 · 206 总计 中文摘要
Previous-finding status (follow-up round)The previous round verified head
Scope
Central claim + A/B tableThe wipe
Head keeps a real Test pins (secondary claim 1) — mutation matrix (
Targeted gates ( Corrections
FindingsF2 (new, low) — a DIRECTORY-shaped
|
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix round — no action needed (PR #9228)This round found no actionable feedback:
No code changes, commits, or comment replies were made this round. 中文说明Autofix 轮次 — 无需处理（PR #9228）本轮没有可执行的反馈：
本轮未进行任何代码改动、提交或评论回复。 Deferred non-Critical feedbackCritical-only mode is active: 5 change-producing rounds are complete. The workflow excluded the non-Critical feedback below from this round's actionable sections; the items remain open for human follow-up. Maintainer feedback is deferred only after its author has used 2 regular feedback batches in this window's Critical-only tail; authors at that budget, if any, are named below. (
中文说明已进入仅处理 Critical 的模式：已完成 5 个产生改动的轮次。本轮可执行区域已排除下方非 Critical 反馈；这些条目保持开放，留待人工跟进。维护者反馈仅在其本人于本窗口 Critical-only 阶段已使用 2 批常规反馈预算后才会延后；达到预算的作者（如有）在下方点名。（评论 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM — the delta since the last pass is exactly the fix the maintainer verification prescribed (worktreeConfig defang + scrub anchoring, both load-bearing-pinned), CI is fully green at this head with both test suites executing the real script, and the maintainer merge-ready verdict already stands on this commit. The residual round-22 findings (R9-1 entrance class, R8-9 refs/objects trust) stand as accepted risk per the maintainer approval; the stale PR body is tracked as a non-blocking follow-up in the Stage 3 comment. ✅
|
Released in v0.22.2. |
4 participants
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.