fix(ci): scope workflow-size ratchet to the PR that grew the file - #9931
Conversation
) A workflow that grew on main without a same-PR baseline bump red-walled every other open PR's CI (recurred twice in two weeks). Given the PR's base commit, the growth branch now downgrades to a warning when the PR's copy of the file is byte-identical to the base; a PR that actually changes the file still fails closed, as does any unresolvable base. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
⏳ Approval still deferred — 1 PR CI workflow run(s) still in progress for ⏳ 审批仍在延迟中 —— |
|
@qwen-code /takeover |
|
Thanks for the PR! Template looks good ✓ — all required sections present, including the bilingual summary. Problem: observed, not theoretical. Issue #9904 documents a recurring failure mode — a PR that grows a workflow without bumping Direction: aligned. This is the gate failing on its own terms — the ratchet exists to make growth visible in review, not to punish unrelated PRs for main-side drift. Scoping the hard failure to the PR that actually changed the file preserves the ratchet's intent (the PR that grows a workflow still has to bump the baseline) while containing the blast radius. CHANGELOG: no direct reference, but this is internal CI infrastructure with no changelog surface. Size: no core module paths touched. ~48 production lines (31 gate script, 5 ci.yml wiring, 2 baseline, 10 docs) vs ~169 lines of tests. Small and focused. Approach: the scope feels right. Two alternatives come to mind — wiring the PR's changed-files list into the gate (adds an API dependency to a gate that today needs none), and auto-bumping the baseline (breaks the gate's read-only stance). The byte-comparison against the PR's base commit is the simplest of the three and fails closed on every unresolved path (no base SHA, fetch failure, file absent at base). The documented residual window — main re-edits the same workflow after the PR branched, failing closed until that PR rebases — is the right tradeoff: self-healing, and never fails open. The same-PR baseline bump is exactly what the ratchet demands: I measured Risk: no elevated risk signals — none of the changed files match the high-risk paths from the revert-history analysis. Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ —— 所有必填部分齐全,包含双语摘要。 问题:已观测到,不是理论问题。Issue #9904 记录了这一反复出现的失败模式——某个 PR 改大了 workflow 却没同步更新 方向:对齐。这正是门自身定位内的失败——棘轮的存在是为了让增长在评审中可见,而不是让无关 PR 为 main 侧的漂移买单。把硬失败限定在真正修改了文件的 PR 上,既保留了棘轮的初衷(改大文件的 PR 仍必须更新基线),又限定了爆炸半径。 规模:未触及核心模块路径。生产逻辑约 48 行(门脚本 31、ci.yml 接线 5、基线 2、文档 10),测试约 169 行。小而聚焦。 方案:范围合理。有两个显而易见的替代方案——把 PR 的 changed-files 列表接入门(给今天零 API 依赖的门引入 API 依赖)、自动上调基线(破坏门的只读立场)。与 PR base 提交逐字节比较是三者中最简的,且在所有无法解析的路径上失败关闭(无 base SHA、fetch 失败、base 中不存在该文件)。文档化的残余窗口——PR 分支后 main 再次编辑同一 workflow、失败关闭直到该 PR rebase——是正确的取舍:自愈,且绝不失败放行。同 PR 内的基线上调正是棘轮的要求:我通过 API 实测了本 PR head 上的 风险:无升级风险信号——改动文件均未命中 revert 历史分析中的高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewIndependent proposal first, before reading the diff: scope the ratchet's hard failure to files the PR actually changed — compare the grown file against the PR's base commit (needs only the base SHA from the event payload plus a depth-1 fetch), downgrade to a warning when byte-identical, fail closed whenever the base cannot be resolved, and keep the absolute ceiling unconditional. The PR does exactly this; I did not find a simpler path it missed. No blockers found. What I verified along the way:
Testing evidence (the PR's own CI, read via API — no PR code executed)CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。 No failures on the reviewed commit so far. The unit-test lane that carries this PR's 182-test suite ( Not verified: runtime behavior of the gate on a genuinely stale-baseline main — no such staleness exists right now, so there is no live red-wall to observe; the hermetic fixtures stand in for it, and they execute the real script. 中文说明代码审查:先独立提出方案再读 diff。我的独立方案是——把棘轮的硬失败限定在 PR 实际修改的文件上:用事件负载里的 base SHA 加一次 depth-1 fetch,与该文件在 base 处的副本逐字节比较,相同则降级为告警,任何无法解析的情形失败关闭,绝对上限保持无条件。本 PR 与此完全一致,我没有找到它遗漏的更简路径。 未发现阻断问题。核实过程:
测试证据:通过 API 读取本 PR 自身的 CI(未执行任何 PR 代码)。截至当前无失败;承载 182 个测试套件的 未验证:真实陈旧基线下的运行时行为——当前 main 上不存在陈旧基线,无实际红墙可观测;由隔离夹具代为覆盖,且夹具执行的是真实脚本。 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 5/5 — clean across every stage: observed recurring problem, minimal fail-closed fix, execution-grade tests. Stepping back: this is what the gate should feel like when it works. My independent proposal and the PR's approach converged on the same answer — base-commit byte-comparison was the simplest of the three paths on the table, and the implementation carries no extra weight. Every edit in the diff is load-bearing: the script change is the fix, the If I had to maintain this in six months, the part I'd thank the author for is the fail-closed posture. There are four ways the base comparison can fail to resolve, and all four land on the strict pre-fix behavior — the change can only ever remove false red-walls, never let real growth slip past. And the warning text names the one-line fix, which turns "someone else forgot to bump the baseline" from a queue-blocking mystery into a five-minute PR. The only reservation is by design, not defect: a PR branched before a later main-side edit to the same workflow still fails closed until it rebases. That is documented in the script, self-heals, and was the explicit alternative to wiring an API dependency into the gate — I agree with the call. CI is still in flight on the reviewed commit (the Linux unit lane running, the Windows shell lane running, security checks queued), so no approval is posted in this run — approval is deferred until CI lands green on the reviewed commit, and the finalize job performs it. 中文说明Confidence: 5/5 —— 各阶段均干净:问题已观测且反复发生、修复最小且失败关闭、测试为执行级。 回顾整体:我的独立方案与 PR 的做法收敛到同一答案——与 base 提交逐字节比较是三个候选中最简的,实现上没有多余的分量。diff 中每一处改动都不可或缺:脚本改动即修复本身, 六个月后维护这段代码时最值得肯定的是失败关闭的姿态:基线比较有四种无法解析的情形,全部回落到修复前的严格行为——本改动只会消除误染红,绝不会让真实的增长溜过去。告警文案还直接写明了一行修复方案,把"别人忘了更新基线"从阻塞全队列的谜团变成五分钟的 PR。 唯一的保留项是设计使然而非缺陷:在 main 之后又编辑了同一 workflow 的情况下分支较早的 PR 仍会失败关闭、直到 rebase。这一点在脚本中有文档、可自愈,且是相对"给门引入 API 依赖"的明确取舍——我认同这个决定。 评审提交上的 CI 仍在进行(Linux 单元车道与 Windows shell 车道运行中,安全检查排队中),本次不直接批准——等 CI 在该提交上全绿后由 finalize 任务执行批准。 — Qwen Code · qwen3.8-max Reviewed at |
|
@qwen-code /takeover |
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. Remove the 中文说明🤝 已接管:autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突,直到移除标签或达到轮次上限。移除 |
doudouOUC
left a comment
There was a problem hiding this comment.
Review Summary
Verdict: Comment — no blockers found. The PR is clean, focused, and well-tested.
What I verified
11 review dimensions across 5 files (308 diff lines, 99 source lines):
| Dimension | Result |
|---|---|
| Line-by-line correctness | ✅ No findings |
| Removed-behavior audit | ✅ No findings |
| Security | ✅ No findings |
| Reuse & duplication | ✅ No findings |
| Altitude & abstraction | ✅ No findings |
| Consistency & clarity | ✅ No findings |
| Performance | ✅ No findings |
| Test coverage | ✅ 1 Nice-to-have (see below) |
| Attacker mindset | ✅ No findings |
| 3AM oncall mindset | ✅ No findings |
| 6-month maintainer | ✅ No findings |
Key properties verified
- Fail-closed on all error paths: empty
BASE_SHA, unresolvable base commit, fetch failure, file absent at base — all return 1 (strict::error). The only downgrade path (file byte-identical to base →::warning) requires a resolved base and a byte-identical file. - No injection vectors:
BASE_SHAcomes fromgithub.event.pull_request.base.sha(GitHub-provided, 40-char hex), not author-controllable. All shell variables are quoted. - Test hermeticity: env vars stripped from
process.env,GIT_CONFIG_NOSYSTEM+ emptyGIT_CONFIG_GLOBALprevent developer config leakage,file://URL forces true depth-1 clone semantics. - Residual window documented: unrebased PR after main-side drift fails closed until rebase — self-healing, documented in the script.
Nice-to-have (not posted as inline)
The describe.skipIf condition at scripts/tests/workflow-size.test.js:150 merges gitAvailable into the skip condition, causing pre-existing tests (which don't use git) to also be skipped on a system without git. Splitting into two describe blocks would preserve coverage for the strict-path tests on git-less systems.
Build & test
No build/test required — the PR changes CI infrastructure files outside any workspace (.github/scripts/, .github/workflows/, scripts/tests/).
— Qwen Code · qwen3.8-max
doudouOUC
left a comment
There was a problem hiding this comment.
Head drift detected: this two-stage review was initiated against commit 35eeeb3969eca4c64413c266001993f7b4b5a9c2, but the PR head is now 0b56a20eea0c38c8b3216cace34607693397a969. The review comment already posted by the bot reflects the earlier commit and may not cover the latest changes.
|
🔄 Takeover re-armed: the round counter starts a fresh window (previous rounds no longer count toward the cap); management continues. 中文说明🔄 已重新武装:轮次计数开启新窗口(此前轮次不再计入上限),托管继续。 |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: the executable-script lint — .github/scripts/check-workflow-size.sh: shellcheck is not installed.
Not explored to full depth (tool budget reached): "agent 1a": install monorepo dependencies and run scripts/tests/workflow-size.test.js under vitest itself — node_modules is absent in the worktree and installing would dr….
Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/ci.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
中文说明
仅完成部分审查,审查缺口已披露。
未审查:the executable-script lint — .github/scripts/check-workflow-size.sh: shellcheck is not installed。
未探索到全部深度(达到工具调用预算):"agent 1a":install monorepo dependencies and run scripts/tests/workflow-size.test.js under vitest itself — node_modules is absent in the worktree and installing would dr…。
未检查(工具限制,非阻断):the executable-script lint — .github/workflows/ci.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| # Lets the growth ratchet tell "this PR grew the workflow" apart | ||
| # from "the baseline went stale on main" so one author's missing | ||
| # baseline bump cannot red-wall every unrelated open PR (#9904). | ||
| WORKFLOW_SIZE_BASE_SHA: "${{ github.event.pull_request.base.sha || github.event.merge_group.base_sha }}" |
There was a problem hiding this comment.
[Critical] The double-quoted value on this line violates this repository's yamllint config (.yamllint.yml sets quoted-strings: {quote-type: 'single', required: true}) and prettier (singleQuote: true). Measured at the reviewed commit: node scripts/lint.js --yamllint exits 1 with the single error .github/workflows/ci.yml:324:35 [quoted-strings] string value is not quoted with single quotes, while the merge-base copy of ci.yml lints clean. This PR classifies as the full CI profile, so the test job's yamllint step (no continue-on-error) hard-fails on it — the PR's own required CI is red on exactly this line, the same red-wall class of failure this PR exists to remove. The currently failing Test (ubuntu-latest, Node 22.x) check on this PR is consistent with this measurement.
witness:
$ node scripts/lint.js --yamllint -> exit 1
.github/workflows/ci.yml:324:35 [quoted-strings] string value is not quoted with single quotes
(merge-base ci.yml: clean)
| WORKFLOW_SIZE_BASE_SHA: "${{ github.event.pull_request.base.sha || github.event.merge_group.base_sha }}" | |
| WORKFLOW_SIZE_BASE_SHA: '${{ github.event.pull_request.base.sha || github.event.merge_group.base_sha }}' |
中文说明
这一行的双引号值违反了本仓库的 yamllint 配置(.yamllint.yml 设置了 quoted-strings: {quote-type: 'single', required: true})和 prettier(singleQuote: true)。在受审提交上实测:node scripts/lint.js --yamllint 以退出码 1 失败,唯一错误为 .github/workflows/ci.yml:324:35 [quoted-strings] string value is not quoted with single quotes,而 merge-base 上的 ci.yml 通过检查。本 PR 被分类为 full CI 配置,因此 test 任务的 yamllint 步骤(没有 continue-on-error)会在此硬失败——本 PR 自己的必需 CI 恰好在这一行上变红,这正是本 PR 要消除的那类红墙。本 PR 当前失败的 Test (ubuntu-latest, Node 22.x) 检查与该测量一致。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| echo "::error file=${file}::${file} grew to ${size} bytes, $((size - base)) over its recorded ${base} (allowance ${GROWTH_ALLOWANCE}). Move prose into a sibling .md and long steps into .github/scripts/ — or, if the growth is real, update ${BASELINE_FILE} in this PR and say why." | ||
| status=1 | ||
| if file_matches_base "${file}"; then | ||
| echo "::warning file=${file}::${file} is ${size} bytes, $((size - base)) over its recorded ${base}, but the file is unchanged from this PR's base — the baseline went stale on main, not in this PR. Bump ${BASELINE_FILE} on main (a one-line PR saying why); unrelated PRs are not blocked." |
There was a problem hiding this comment.
[Critical] The claim added here — "unrelated PRs are not blocked" — is false as long as the vitest mirror in scripts/tests/workflow-size.test.js (the '%s is within its baseline allowance' block near line 110) keeps asserting bytes <= recorded + allowance against the checkout's own tree with no base-SHA awareness. The mirror has no skipIf and runs via test:scripts inside npm run test:ci on every full-profile lane, and merge_group events always classify full, so the merge-queue lanes run it too. The next time a baseline goes stale on main — the exact #9904 condition this PR exists to fix — the shell gate will warn, but the mirror still fails the run: the red wall is only relocated from the gate step into test:ci. The drift is not hypothetical: before this PR's bump, origin/main had ci.yml at 73850 bytes against the 69782 entry — 28 bytes inside the allowance.
witness (scratch-tree probe, drift state simulated: baseline 69782, ci.yml 74203, base SHA resolvable, unrelated-PR shape):
BASE-side (this gate): ::warning ... ci.yml is 74203 bytes, 4421 over its recorded 69782 ... unrelated PRs are not blocked. gate-exit=0
PR-side (vitest mirror): FAIL scripts/tests/workflow-size.test.js > .github/workflows/ci.yml is within its baseline allowance
AssertionError: expected 74203 to be less than or equal to 73878
Tests 1 failed | 181 passed; vitest-exit=1
Make the two enforcement points agree about the drift case: either give the mirror the same base-scoped leniency (wire WORKFLOW_SIZE_BASE_SHA into the test job's env and pass/skip the allowance assertion when the file is byte-identical to the base, reusing file_matches_base semantics), or — if the mirror is deliberately kept strict as the final enforcer — amend this warning text and the qwen-autofix.md paragraph to say full CI still fails until the one-line baseline-bump PR lands on main. (Only github_ci_only/docs-only PRs escape, because they skip vitest.)
中文说明
此处新增的说法——"unrelated PRs are not blocked"(无关 PR 不会被阻塞)——并不成立:只要 scripts/tests/workflow-size.test.js 中的 vitest 镜像(约第 110 行的 '%s is within its baseline allowance' 代码块)仍然在没有任何 base SHA 感知的情况下,对检出树自身断言 bytes <= recorded + allowance。该镜像没有 skipIf,并经由 npm run test:ci 里的 test:scripts 在每个 full 配置车道上运行;而 merge_group 事件总是被分类为 full,因此合并队列车道同样会运行它。下一次 main 上的基线变陈旧时——也就是本 PR 要修复的 #9904 情形本身——shell 门只会告警,但镜像仍会让整个测试运行失败:红墙只是从门步骤搬进了 test:ci。这种漂移并非假设:在本 PR 上调基线之前,origin/main 上 ci.yml 为 73850 字节,而基线条目是 69782——距允许增量仅差 28 字节。
请让两个执行点对漂移情形保持一致:要么给镜像同样的 base 范围宽容(把 WORKFLOW_SIZE_BASE_SHA 接线进 test 任务的环境变量,当文件与 base 逐字节相同时让允许增量断言通过/跳过,复用 file_matches_base 语义);要么——如果刻意让镜像保持严格、作为最终执行者——就修改此处的告警文案与 qwen-autofix.md 的段落,说明完整 CI 仍会失败,直到那一行基线修复 PR 合入 main。(只有 github_ci_only/纯文档 PR 能幸免,因为它们不跑 vitest。)
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const dir = mkdtempSync(join(tmpdir(), 'workflow-size-gate-fetch-')); | ||
| try { | ||
| const env = { ...process.env }; | ||
| delete env.WORKFLOW_SIZE_BASE_SHA; |
There was a problem hiding this comment.
[Suggestion] The hermetic-env construction — { ...process.env }, the three WORKFLOW_SIZE_* deletes, the empty fixture-gitconfig write, and the GIT_CONFIG_NOSYSTEM/GIT_CONFIG_GLOBAL assignment — appears twice in this same PR: inside runGate's commitBase branch (lines 175–193) and again here. The fixture bodies legitimately differ (this test needs the base genuinely unfetched), but the env-scrub block is a pure paste candidate for a small helper. The scrub list is load-bearing — your own comment says a WORKFLOW_SIZE_* leak from the developer's shell must not change what the strict-path fixtures assert — so if the gate gains a fourth WORKFLOW_SIZE_* knob, both copies must be edited in sync, and missing one produces no loud error: a leaked WORKFLOW_SIZE_BASE_SHA turns the fail-closed fixture into the warning path, a leaked WORKFLOW_SIZE_GROWTH_ALLOWANCE turns a one-byte-over failure green.
const hermeticGateEnv = (dir) => {
const env = { ...process.env };
delete env.WORKFLOW_SIZE_BASE_SHA;
delete env.WORKFLOW_SIZE_GATE_BYTES;
delete env.WORKFLOW_SIZE_GROWTH_ALLOWANCE;
const gitconfigPath = join(dir, 'fixture-gitconfig');
writeFileSync(gitconfigPath, '');
return Object.assign(env, {
GIT_CONFIG_NOSYSTEM: '1',
GIT_CONFIG_GLOBAL: gitconfigPath,
});
};
// and start both fixtures with: const env = hermeticGateEnv(dir);中文说明
密封环境的构造——{ ...process.env }、三个 WORKFLOW_SIZE_* 的 delete、空 fixture-gitconfig 的写入,以及 GIT_CONFIG_NOSYSTEM/GIT_CONFIG_GLOBAL 的赋值——在同一个 PR 里出现了两次:一次在 runGate 的 commitBase 分支(175–193 行),一次在这里。两个夹具的主体确实不同(本测试需要 base 真正未被 fetch),但环境清洗这一段完全可以抽成一个小工具函数。这份清洗清单是承重结构——你们自己的注释也写明,开发者 shell 里泄漏进来的 WORKFLOW_SIZE_* 不得改变严格路径夹具的断言——因此一旦门新增第四个 WORKFLOW_SIZE_* 开关,两处副本必须同步修改,而漏改不会有任何响亮的报错:泄漏的 WORKFLOW_SIZE_BASE_SHA 会把失败关闭夹具变成告警路径,泄漏的 WORKFLOW_SIZE_GROWTH_ALLOWANCE 会让超出 1 字节的失败变绿。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| @@ -75,8 +98,12 @@ for file in .github/workflows/*.yml .github/workflows/*.yaml; do | |||
| echo "::error file=${file}::${file} has no entry in ${BASELINE_FILE}. Add '${size} ${file##*/}' so its growth is tracked." | |||
There was a problem hiding this comment.
[Suggestion] The PR-scope downgrade is applied to the growth-past-allowance arm below but not to this twin arm, which has the same main-side-drift red-wall shape the PR exists to remove. An entry-less workflow reaches main only via a path that bypasses the gate (admin/bypass merge, misclassification, or a gate outage), so the state is low-frequency — but when it occurs, every open PR that never touched the file hard-fails on a bookkeeping fix its author cannot perform, and the added docs overstate coverage for it ("the ratchet's blast radius is scoped to the PR that earned it"; script comment: "the growth branch below hard-fails only if the PR actually changed the file").
witness (probe, fixture D): orphan.yml present in the base commit with no baseline entry,
PR file byte-identical to base, WORKFLOW_SIZE_BASE_SHA=<base>
-> D-exit=1
::error file=.github/workflows/orphan.yml::.github/workflows/orphan.yml has no entry in
.github/workflows/.size-baseline. Add '5000 orphan.yml' so its growth is tracked.
Wrap this arm the same way (the absolute GATE_BYTES ceiling stays deliberately excluded — it guards the near-GitHub-limit emergency, and its loud blast radius is the point):
if [[ -z "${base}" ]]; then
if file_matches_base "${file}"; then
echo "::warning file=${file}::${file} has no entry in ${BASELINE_FILE}, but the file is unchanged from this PR's base — add '${size} ${file##*/}' on main so its growth is tracked; unrelated PRs are not blocked."
else
echo "::error file=${file}::${file} has no entry in ${BASELINE_FILE}. Add '${size} ${file##*/}' so its growth is tracked."
status=1
fi中文说明
PR 范围的降级只应用到了下面那个"超出允许增量"的分支,而没有应用到它的孪生分支——后者同样具备本 PR 要消除的"main 侧漂移红墙"形态。没有基线条目的 workflow 只会经由绕过门的路径进入 main(admin/绕过合并、分类错误或门故障),所以该状态出现频率低——但一旦发生,每个从未碰过该文件的开放 PR 都会在一个它们作者无法执行的簿记修复上硬失败,而且新增文档对覆盖面的描述过头了("the ratchet's blast radius is scoped to the PR that earned it";脚本注释:"the growth branch below hard-fails only if the PR actually changed the file")。
请用同样的方式包住这个分支(绝对上限 GATE_BYTES 刻意排除在外——它守护的是逼近 GitHub 上限的紧急情况,其响亮的爆炸半径正是要点)。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // used to red-wall every OTHER open PR. A PR whose copy of the file is | ||
| // byte-identical to its base did not cause the drift and must only see a | ||
| // warning; the hard failure belongs to the PR that changes the file. | ||
| it('warns instead of failing when the PR did not touch the file', () => { |
There was a problem hiding this comment.
[Suggestion] file_matches_base has three input classes — identical to base, differing from base, and absent from the base commit (git show fails → empty pipe into cmp) — and the new tests cover only the first two. The current behavior for the absent case is correct (probe-verified: a PR adding a brand-new over-allowance workflow still hard-fails, exit 1), but it rests on cmp happening to see empty-vs-nonempty and is pinned by no test — every fixture here commits the file into the base (dirtyFiles only ever overwrites small.yml, which exists in base). A future simplification treating a failed git show as "nothing to compare, treat as unchanged" (early return 0) would downgrade a file the PR clearly introduced from hard-fail to warning on exactly the PR that owns it — the ratchet's core invariant, unguarded. One fixture closes it: commitBase: true with a base that contains only a different workflow, then add the grown file post-commit via dirtyFiles with a baseline entry far below size minus allowance, and assert status 1 plus grew to ... bytes.
witness (probe, fixture C): base commit lacks small2.yml; worktree adds it at 5000 bytes
with baseline entry '100 small2.yml'; BASE_SHA=base -> C-exit=1, '::error ... grew to 5000 bytes'
(correct today, unpinned by any test)
中文说明
file_matches_base 有三类输入——与 base 相同、与 base 不同、以及文件在 base 提交中不存在(git show 失败 → 空管道流入 cmp)——新增测试只覆盖了前两类。不存在这一情形的当前行为是正确的(探针验证:PR 新增一个超出允许增量的全新 workflow 仍会硬失败,退出码 1),但它依赖 cmp 恰好看到"空对非空",没有任何测试钉住——此处所有夹具都把文件提交进了 base(dirtyFiles 只会覆写 small.yml,而它在 base 中存在)。未来若有简化把 git show 失败当作"没有可比内容,视为未变更"(提前 return 0),就会在恰恰引入了该文件的 PR 上,把硬失败降级为告警——棘轮的核心不变量将无人守护。补一个夹具即可闭环:commitBase: true,base 中只包含另一个 workflow,提交后用 dirtyFiles 新增超量文件,基线条目记为远小于"体积减允许增量"的值,断言退出码 1 与 grew to ... bytes。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| if file_matches_base "${file}"; then | ||
| echo "::warning file=${file}::${file} is ${size} bytes, $((size - base)) over its recorded ${base}, but the file is unchanged from this PR's base — the baseline went stale on main, not in this PR. Bump ${BASELINE_FILE} on main (a one-line PR saying why); unrelated PRs are not blocked." | ||
| else | ||
| echo "::error file=${file}::${file} grew to ${size} bytes, $((size - base)) over its recorded ${base} (allowance ${GROWTH_ALLOWANCE}). Move prose into a sibling .md and long steps into .github/scripts/ — or, if the growth is real, update ${BASELINE_FILE} in this PR and say why." |
There was a problem hiding this comment.
[Suggestion] When WORKFLOW_SIZE_BASE_SHA is set but unresolvable — e.g. a transient git fetch --depth=1 failure — the annotation emitted is byte-identical to genuine PR growth (probe-verified: diff of the two ::error outputs is empty), and the two causes have opposite remedies: re-run the job vs. bump the baseline. This diff adds the network round-trip to a gate that previously had none, on a lane whose adjacent checkout-step comment documents a flaky egress proxy. The fail-closed behavior is right and stays; only the diagnosis needs to improve, otherwise an unrelated PR hit by an infra blip gets told the growth is theirs — re-creating the red wall this PR removes, with a misleading remedy.
witness (probe, size held at 5000):
fixture A (genuine growth) vs fixture B (file committed at 5000, never touched,
WORKFLOW_SIZE_BASE_SHA=0x40 unresolvable)
diff <(grep '::error' A/out.txt) <(grep '::error' B/out.txt) -> no difference (BYTE-IDENTICAL)
Distinguish "base unresolvable" from "content differs" — e.g. return a distinct status for the resolution-failure path — and emit one extra line in that case: echo "::warning::base ${BASE_SHA} could not be resolved (git fetch failed?) — failing strict; if this PR did not touch ${file}, re-run the job."
中文说明
当 WORKFLOW_SIZE_BASE_SHA 已设置但无法解析时——例如 git fetch --depth=1 瞬时失败——发出的标注与 PR 真实增长逐字节相同(探针验证:两个 ::error 输出的 diff 为空),而两种原因的补救方式恰恰相反:重跑任务 vs 上调基线。本 diff 给一个此前没有任何网络调用的门新增了一次网络往返,且所在车道的相邻 checkout 步骤注释明确记录了出口代理不稳定。失败关闭的行为是正确的、应保留;需要改进的只是诊断——否则一个被基础设施抖动击中的无关 PR 会被告知"增长是你造成的",以误导性的补救方式重新筑起本 PR 要拆除的红墙。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| elif ((size > base + GROWTH_ALLOWANCE)); then | ||
| echo "::error file=${file}::${file} grew to ${size} bytes, $((size - base)) over its recorded ${base} (allowance ${GROWTH_ALLOWANCE}). Move prose into a sibling .md and long steps into .github/scripts/ — or, if the growth is real, update ${BASELINE_FILE} in this PR and say why." | ||
| status=1 | ||
| if file_matches_base "${file}"; then |
There was a problem hiding this comment.
[Suggestion] The warning branch leaves status at 0, so the script's final success banner — ✅ every workflow file is under the ... gate and within 4096 bytes of its recorded baseline — now prints for the very run that emitted the stale-baseline warning. Before this diff, status==0 implied every file was within allowance (the only non-failing warning was the shrink one, within allowance by definition), so the banner's claim always held when printed. Now, on the exact #9904 scenario, the log shows ::warning ... 4421 over its recorded baseline followed by the ✅ banner asserting every file is within 4096 bytes — directly contradicting the warning in the same log.
witness (probe, drift run on the real tree):
::warning ... ci.yml is 74203 bytes, 4421 over its recorded 69782 ...
✅ every workflow file is under the 470000-byte gate and within 4096 bytes of its recorded baseline
gate-exit=0
Track the downgrade (e.g. warned_stale=1 in the warning branch) and qualify the banner when it is set, e.g. echo "✅ every workflow file is under the ${GATE_BYTES}-byte gate${warned_stale:+ (stale-baseline warnings above — bump ${BASELINE_FILE} on main)}".
中文说明
告警分支让 status 保持为 0,于是脚本最后的成功横幅——✅ every workflow file is under the ... gate and within 4096 bytes of its recorded baseline——会在恰恰发出了陈旧基线告警的那次运行里打印。在本 diff 之前,status==0 蕴含每个文件都在允许增量之内(唯一不失败的告警是收缩告警,按定义就在允许增量之内),所以横幅的说法在打印时总是成立。现在,在 #9904 的确切场景下,日志先显示 ::warning ... 4421 over its recorded baseline,紧接着 ✅ 横幅又声称每个文件都在 4096 字节之内——同一条日志里自相矛盾。
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
🤖 AutoFix could not reach the model — [API Error: 500 <500> InternalError.Algo: An error occurred in model serving, error message is: [Receive batching backend response failed!] (attempt 1/100) — it will retry on the next scan. What I found before stopping: See the Qwen Autofix agent step logs for model/tool output. 中文说明🤖 AutoFix 无法连接模型 —— [API Error: 500 <500> InternalError.Algo: An error occurred in model serving, error message is: [Receive batching backend response failed!](第 1/100 次尝试)—— 将在下次扫描时重试。 Run log: https://github.com/QwenLM/qwen-code/actions/runs/32740992248 🧠 Handled by Qwen Code · model/模型 |
The repo's yamllint config requires single-quoted strings; the value contains no single quotes, so the double-quoted form failed the quoted-strings rule in CI. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
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. |
…de (#9931) Address review round 1: - Single-quote the WORKFLOW_SIZE_BASE_SHA value; the double quotes violated yamllint's quoted-strings rule and prettier's singleQuote, hard-failing this PR's own Test lane. - Wire the base SHA into every `npm run test:ci` step and grant the vitest mirror the same stale-baseline leniency as the shell gate; the mirror is the only enforcer on the merge-queue lanes, and without this the red wall just moves from the gate into test:ci. - Apply the downgrade to the missing-entry arm too (same red-wall shape as a stale size). - Return a distinct status for an unresolvable base and add a diagnostic, so a transient fetch failure is not annotated like genuine PR growth. - Qualify the success banner when stale-baseline warnings fired. - Deduplicate the hermetic-env construction, pin the brand-new-file class, and split the git fixtures into their own describe so the strict-path tests still run on git-less runners.
…ttps://github.com/QwenLM/qwen-code into fix/workflow-size-gate-stale-baseline-blast-radius
|
🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on: · 已处理最新评审反馈(第 2/100 轮)。改动内容与我反驳保留之处如下: Review round summary — PR #9931Commit: No base conflict ( Feedback dispositionsCritical
Suggestions
Maintainer review (rv:5007885933)
Informational / disclosed gaps
Failed checks
Mutation probes (all restored to green afterwards)
VerificationCommands actually run and their results:
Not run: integration tests (the change is CI-infrastructure-only and never exercised through the bundled CLI); 中文说明审查轮次总结 — PR #9931提交: 无 base 冲突( 反馈处置Critical
Suggestion
维护者审查 (rv:5007885933)
信息性 / 已披露缺口
失败检查
变异探针(全部恢复至绿色)
验证实际执行的命令及结果:
未运行:集成测试(本改动仅涉及 CI 基础设施,从不经由打包后的 CLI 执行); 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
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): "agent 4": did not empirically time git fetch failure against an unreachable origin; the ~minute-per-attempt cost in finding 1 is from git's default transport timeouts b….
Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/ci.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
中文说明
未探索到全部深度(达到工具调用预算):"agent 4":did not empirically time git fetch failure against an unreachable origin; the ~minute-per-attempt cost in finding 1 is from git's default transport timeouts b…。
未检查(工具限制,非阻断):the executable-script lint — .github/workflows/ci.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // Stale-baseline leniency (#9904), mirroring the shell gate: overage on | ||
| // a file byte-identical to the PR's base is main-side drift, not this | ||
| // PR's growth. Without this the gate warns but the mirror still fails | ||
| // the run, relocating the red wall into `npm run test:ci`. |
There was a problem hiding this comment.
[Critical] The stale-baseline leniency added here was applied only to the allowance test it belongs to. The mirror's sibling test '%s has a baseline entry' (line 156) still asserts expect(baseline.has(workflowName(file))).toBe(true) unconditionally, with no fileMatchesBase escape — while the shell gate now downgrades BOTH arms (over-allowance and missing-entry) to a warning when the file is byte-identical to the PR's base, telling the user "unrelated PRs are not blocked". So a workflow file that reaches main without a .size-baseline entry — the fixture comment in this same PR names the causes: bypass merge, misclassification, gate outage — red-walls every later unrelated PR: the bash gate step passes with that warning, and in the same CI run every lane running npm run test:ci fails hard on the entry assertion, including the merge-group-only test_macos/test_windows lanes where the bash gate never runs at all. That is the exact #9904 red wall this PR exists to remove, surviving for the missing-entry class, relocated into test:ci; only github_ci_only/docs-only PRs escape, because they skip vitest.
Witness — scratch-tree probe with the drift state simulated (codeql.yml entry removed from .size-baseline, file byte-identical to the base commit):
BASE-side (bash gate): ::warning ... unrelated PRs are not blocked. GATE_EXIT=0
PR-side (vitest mirror): FAIL ... codeql.yml has a baseline entry
AssertionError: expected false to be true
Tests 1 failed | 50 passed
Applying the fix below flips the probe to Tests 51 passed | 0 failed. Give the entry test the same base-scoped leniency (the fix targets the sibling test above this comment, line 156):
it.each(workflowFiles)('%s has a baseline entry', (file) => {
if (baseline.has(workflowName(file))) return;
expect(
fileMatchesBase(file),
`${file} has no entry in .size-baseline and differs from the PR's base`,
).toBe(true);
});plus a mirror fixture for the missing-entry drift outcome. If strictness there is intentional instead, the gate's missing-entry warning text "unrelated PRs are not blocked" needs to say full CI still fails until the entry lands.
中文说明
此处新增的陈旧基线宽容只应用到了它所属的允许增量断言。镜像的姊妹测试 '%s has a baseline entry'(第 156 行)仍然无条件断言 expect(baseline.has(workflowName(file))).toBe(true),没有 fileMatchesBase 逃生通道——而 shell 门现在已经在文件与 PR base 逐字节相同时,对缺条目和超允许增量两个分支都降级为告警,并告诉用户"unrelated PRs are not blocked"(无关 PR 不会被阻塞)。于是,一个没有 .size-baseline 条目就进入 main 的 workflow 文件(本 PR 自己的夹具注释列出了成因:bypass 合并、误分类、门故障)会让此后每个无关 PR 撞红墙:bash 门步骤带着上述告警通过,而同一次 CI 运行中每个执行 npm run test:ci 的车道都会在条目断言上硬失败——包括从不运行 bash 门的 merge-group 전용 test_macos/test_windows 车道。这正是本 PR 要消除的 #9904 红墙,它在缺条目这一类上存活了下来,只是搬进了 test:ci;只有 github_ci_only/纯文档 PR 能幸免,因为它们不跑 vitest。
见证——在临时树中模拟漂移状态(从 .size-baseline 删除 codeql.yml 条目、文件与 base 提交逐字节相同)的探针:
BASE 侧(bash 门): ::warning ... unrelated PRs are not blocked. GATE_EXIT=0
PR 侧(vitest 镜像): FAIL ... codeql.yml has a baseline entry
AssertionError: expected false to be true
Tests 1 failed | 50 passed
应用下方修复后,探针翻转为 Tests 51 passed | 0 failed。请给条目测试同样的 base 范围宽容(修复目标是本条评论上方的姊妹测试,第 156 行),并补充一个缺条目漂移情形的镜像夹具。如果那里刻意保持严格,则门的缺条目告警文案"unrelated PRs are not blocked"需要改为说明:完整 CI 仍会失败,直到条目补上。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| 49610 cd-cua-driver.yml | ||
| 2076 cd-mobile-mcp.yml | ||
| 69782 ci.yml | ||
| 74203 ci.yml |
There was a problem hiding this comment.
[Suggestion] This entry is already 622 bytes stale against this PR's own HEAD: it records 74203, but the committed ci.yml at the reviewed commit measures 74825 bytes. The bump was exact when made; the follow-up commit that wired WORKFLOW_SIZE_BASE_SHA into the macOS/Windows lanes grew ci.yml by 622 bytes without re-bumping. Nothing fails today — 622 < the 4096 allowance — but the PR dedicated to baseline truthfulness ships its own file's recorded size wrong: the post-bump growth accrues as unreviewed headroom inside the allowance, and every future PR editing ci.yml trips the ratchet 622 bytes early — the same recorded-vs-actual drift this PR exists to police, shipped inside itself.
| 74203 ci.yml | |
| 74825 ci.yml |
中文说明
这一条目相对本 PR 自己的 HEAD 已经陈旧了 622 字节:记录值为 74203,而受审提交上入库的 ci.yml 实测为 74825 字节。这次上调在做出时是精确的;随后把 WORKFLOW_SIZE_BASE_SHA 接线进 macOS/Windows 车道的提交让 ci.yml 又长了 622 字节,却没有再次上调条目。今天不会有任何东西失败——622 小于 4096 的允许增量——但一个以基线真实性为使命的 PR,却把自己文件的记录体积带着错发出去:上调之后的增长作为未经审查的余量累积在允许增量之内,此后每个编辑 ci.yml 的 PR 都会提前 622 字节触发棘轮——这正是本 PR 要治理的"记录值与实际值漂移",却出现在它自己身上。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| expect( | ||
| fileMatchesBase(file), | ||
| `${file} is ${bytes - recorded} bytes over its recorded ${recorded} and differs from the PR's base`, | ||
| ).toBe(true); |
There was a problem hiding this comment.
[Suggestion] The mirror collapses "base unresolvable" into the same false as "file differs" (if (!resolved) return false in fileMatchesBase), and this failure message asserts the latter — while both git calls use stdio: 'ignore', so a fetch failure leaves no trace in the log. The shell half of this same diff deliberately separates the two: file_matches_base returns 2 and the callers emit unresolvable_base_note, because "a transient fetch failure and genuine PR growth need opposite remedies". During a stale-baseline window on the depth-1 ubuntu lane — the checkout comment already documents the squid egress proxy flaking — a transient git fetch --depth=1 failure makes this assertion fail with a message blaming the PR's growth: the oncall diffs the file against the base (which succeeds on retry), finds it byte-identical, and contradicts the message before discovering the remedy was just re-run the job. Have fileMatchesBase surface the unresolvable case distinctly (e.g. a sentinel or throw) and widen the message, e.g. ...and differs from the PR's base — or the base could not be resolved (transient fetch failure: re-run the job), and drop stdio: 'ignore' so the fetch stderr reaches the log.
中文说明
镜像把"base 不可解析"折叠进了与"文件不同"相同的 false(fileMatchesBase 中的 if (!resolved) return false),而这条失败消息断言的是后者——且两处 git 调用都用了 stdio: 'ignore',fetch 失败在日志中不留任何痕迹。同一 diff 的 shell 一侧刻意区分了两者:file_matches_base 返回 2,调用方输出 unresolvable_base_note,因为"瞬时 fetch 失败与真实的 PR 增长需要相反的补救"。在陈旧基线窗口内的 depth-1 ubuntu 车道上——checkout 注释已记录 squid 出口代理会抽风——一次瞬时的 git fetch --depth=1 失败会让这条断言带着"归咎于 PR 增长"的消息变红:值班人把文件与 base 对比(重试即成功),发现逐字节相同,与消息矛盾,最后才发现补救只是重跑任务。请让 fileMatchesBase 显式暴露不可解析情形(如哨兵值或抛错)并扩宽消息,例如 ...and differs from the PR's base — or the base could not be resolved (transient fetch failure: re-run the job),并去掉 stdio: 'ignore' 让 fetch 的 stderr 进入日志。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| } | ||
| }); | ||
|
|
||
| describe('the vitest mirror of the leniency (fileMatchesBase)', () => { |
There was a problem hiding this comment.
[Suggestion] The only unit fixtures for fileMatchesBase are nested inside describe.skipIf(!gitAvailable) inside the bash-gated describe.skipIf(process.platform === 'win32' || !bashSupportsAssocArrays) block — so they never run on the Windows lane, nor on macOS whenever PATH bash lacks declare -A: precisely the merge-group-only lanes where this mirror is the ONLY ratchet enforcer. The helper spawns only git, so the inherited bash gate is broader than its requirements. And no test on any lane passes a backslash path to it: on POSIX join() emits only /, so the file.split(/[\\/]/).join('/') normalization in the git show pathspec is never exercised with a backslash anywhere — a mutation removing it survives every test that runs. A future edit that breaks that normalization stays green on the ubuntu fixtures; at the next stale-baseline drift git show 'SHA:.github\\workflows\\ci.yml' fails on the test_windows merge-queue lane, fileMatchesBase returns false, and the #9904 red wall relocates onto a required merge-queue check — first observed during a real incident. Hoist this describe out of the bash-gated block and gate it on gitAvailable alone, plus a case passing a win32.join(WORKFLOW_DIR, 'small.yml') path so the normalization is pinned on every lane.
中文说明
fileMatchesBase 的唯一单元夹具嵌套在以 bash 为门槛的 describe.skipIf(process.platform === 'win32' || !bashSupportsAssocArrays) 块内的 describe.skipIf(!gitAvailable) 里——因此它们在 Windows 车道上从不运行,在 PATH bash 缺少 declare -A 的 macOS 上也不运行:而这恰恰是该镜像作为唯一棘轮执行者的 merge-group 전용车道。该助手只 spawn git,继承来的 bash 门槛超出了它的真实需求。且任何车道上都没有测试传给它反斜杠路径:POSIX 上 join() 只产生 /,所以 git show pathspec 中的 file.split(/[\\/]/).join('/') 归一化从未在任何地方用反斜杠输入验证过——删掉它的变异能在所有运行的测试中存活。未来某次破坏该归一化的改动在 ubuntu 夹具上会保持绿色;下一次陈旧基线漂移时,git show 'SHA:.github\\workflows\\ci.yml' 会在 test_windows 合并队列车道上失败,fileMatchesBase 返回 false,#9904 红墙就搬到了一个必需的合并队列检查上——而且要到真实事故时才首次被发现。请把这个 describe 提出 bash 门槛块、只以 gitAvailable 为门槛,并补一个传入 win32.join(WORKFLOW_DIR, 'small.yml') 路径的用例,让归一化在每个车道上都被钉住。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| spawnSync('git', ['fetch', '--depth=1', '--quiet', 'origin', baseSha], { | ||
| stdio: 'ignore', | ||
| }).status === 0; |
There was a problem hiding this comment.
[Suggestion] This fetch arm is exercised by no test: the mirror fixture repo commits its base locally (rev-parse always succeeds) and has no origin remote, and in the unresolvable-base fixture the fetch fails anyway — so deleting this entire block is a surviving mutation across all mirror tests (probe-verified: with the arm removed, Tests 6 passed | 186 skipped — every mirror test stays green). Every production lane checks out at depth 1, so the base commit is never locally present and this arm is the production path; with it broken, fileMatchesBase returns false for every stale-baseline PR and the pre-fix red wall silently returns while the suite stays green. The shell side's identical arm has a dedicated shallow-clone fixture annotated "Removing the fetch line from the script must turn this test red"; add the mirror's counterpart — bare origin with uploadpack.allowAnySHA1InWant, depth-1 clone via file:// URL, base commit behind the tip, asserting fileMatchesBase(...) returns true only because the fetch succeeded.
中文说明
这条 fetch 分支没有任何测试覆盖:镜像夹具仓库把 base 提交在本地(rev-parse 总是成功),也没有 origin 远端;而在不可解析 base 的夹具里 fetch 本来就会失败——因此删掉整段代码块这一变异在所有镜像测试中都能存活(已用探针验证:删掉该分支后 Tests 6 passed | 186 skipped,所有镜像测试仍为绿色)。每个生产车道都以 depth 1 检出,base 提交从不在本地,这条分支就是生产路径;它一旦损坏,fileMatchesBase 会对每个陈旧基线 PR 返回 false,修复前的红墙会悄悄回来,而测试套件依旧绿色。shell 一侧的相同分支有专门的浅克隆夹具,注释写明"删掉脚本中的 fetch 行必须让此测试变红";请为镜像补上对应夹具——带 uploadpack.allowAnySHA1InWant 的 bare origin、经 file:// URL 的 depth-1 克隆、base 提交落后于 tip,断言 fileMatchesBase(...) 仅在 fetch 成功时返回 true。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| # Lets the growth ratchet tell "this PR grew the workflow" apart | ||
| # from "the baseline went stale on main" so one author's missing | ||
| # baseline bump cannot red-wall every unrelated open PR (#9904). | ||
| WORKFLOW_SIZE_BASE_SHA: '${{ github.event.pull_request.base.sha || github.event.merge_group.base_sha }}' |
There was a problem hiding this comment.
[Suggestion] This workflow-scoped input is hand-wired at four step-level env: blocks (here and at the three Run tests and generate reports steps), guarded by a tripwire test whose regex matches only steps literally named 'Run tests and generate reports' with toHaveLength(3) — ci.yml already has a workflow-level env: block whose stated purpose is exactly this kind of shared input (the HELPER_TESTS comment: "so a new helper test can't be added to one path and silently dropped from the other"). A future lane that runs npm run test:ci under a differently named step never matches the tripwire, toHaveLength(3) stays green, and at the next stale-baseline episode — the #9904 condition this PR exists to fix, which hit twice in two weeks — WORKFLOW_SIZE_BASE_SHA is unset on that lane, fileMatchesBase returns false, and the mirror hard-fails: the exact red wall this PR removes, relocated onto the new lane where no test pointed at the missing wiring. Either hoist the value to the workflow-level env: block once and delete the four step-level copies (it is GitHub-set metadata, harmless to every other step), or change the tripwire to match any step whose run: contains npm run test:ci, regardless of name.
中文说明
这个 workflow 级输入被手工接线在四个步骤级 env: 块中(此处与三个 Run tests and generate reports 步骤),由一个正则只匹配字面名为 'Run tests and generate reports' 的步骤、并断言 toHaveLength(3) 的钉桩测试守护——而 ci.yml 本就有一个 workflow 级 env: 块,其声明的用途正是这类共享输入(HELPER_TESTS 注释:"这样新增的助手测试就不会被加进一条路径却从另一条路径悄悄漏掉")。未来某个以不同步骤名运行 npm run test:ci 的车道永远不会匹配该钉桩,toHaveLength(3) 保持绿色;而下一次陈旧基线事件——正是本 PR 要修复、两周内发生过两次的 #9904 情形——时,该车道上 WORKFLOW_SIZE_BASE_SHA 未设置,fileMatchesBase 返回 false,镜像硬失败:本 PR 要消除的红墙原样搬到了这个没有任何测试指向其缺接线的新车道上。要么把该值一次性提升到 workflow 级 env: 块并删除四处步骤级副本(它是 GitHub 设置的元数据,对其他步骤无害),要么把钉桩改为匹配任何 run: 含 npm run test:ci 的步骤,不论名字。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const fileMatchesBase = (file) => { | ||
| const baseSha = (process.env.WORKFLOW_SIZE_BASE_SHA ?? '').trim(); | ||
| if (!baseSha) return false; |
There was a problem hiding this comment.
[Suggestion] One predicate shipped as two independent implementations — this JS fileMatchesBase and the bash file_matches_base() (.github/scripts/check-workflow-size.sh) — each pinned by its own fixture suite, with no test running both against the same repo state; and they already differ: bash returns a third outcome with a dedicated unresolvable_base_note diagnostic, while JS folds that case into false behind a "differs from the PR's base" message. A future edit to the leniency logic (fetch retry, a new event arm, a simplification) that lands in only one copy makes the PR lanes (bash gate) and the merge-queue/macOS/Windows lanes (this mirror, via npm run test:ci) disagree — one warns while the other hard-fails — recreating exactly the #9904 red wall on lanes where the bash diagnostic is never visible, with nothing in the suite turning that disagreement red. The cross-runtime split itself is structurally forced (the mirror must stay portable JS, the gate node-free), so the ask is a parity test, not a shared mechanism: in one committed-git mirror fixture also run .github/scripts/check-workflow-size.sh over the same repo state and assert both sides reach the same verdict.
中文说明
同一个谓词以两个独立实现交付——这个 JS fileMatchesBase 与 bash 的 file_matches_base()(.github/scripts/check-workflow-size.sh)——各自由自己的夹具套件钉住,却没有任何测试让两者在同一仓库状态上对跑;而且它们已经有差异:bash 对 base 不可解析返回第三种结果并附专门的 unresolvable_base_note 诊断,JS 则把该情形折叠进 false,消息还断言"与 PR 的 base 不同"。未来对宽容逻辑的任一编辑(fetch 重试、新事件分支、某次简化)只要只落进其中一份副本,就会让 PR 车道(bash 门)与合并队列/macOS/Windows 车道(本镜像,经 npm run test:ci)相互矛盾——一边告警、另一边硬失败——在 bash 诊断根本不可见的车道上重新制造出 #9904 红墙,且套件中没有任何东西会让这种分歧变红。跨运行时拆分本身是结构所迫(镜像必须保持可移植 JS、门必须无 node),因此要的是一个对等测试而非共享机制:在某个已提交 git 的镜像夹具中,对同一仓库状态也运行一次 .github/scripts/check-workflow-size.sh,断言两侧得出相同结论。
— qwen3.8-max via Qwen Code /review (v0.22.0)
- Apply the #9904 stale-baseline leniency to the mirror's entry test, not only the allowance test: a workflow that reached main without a baseline entry used to warn in the bash gate but hard-fail every unrelated PR in `npm run test:ci`, relocating the exact red wall this PR removes. - Distinguish an unresolvable base from a changed file: fileMatchesBase now throws with "re-run the job" guidance instead of folding into false behind a message blaming the PR's growth, and the fetch stderr reaches the log. - Hoist WORKFLOW_SIZE_BASE_SHA to the workflow-level env block so every lane inherits it; delete the four hand-wired step-level copies and point the tripwire test at the single source. - Bump the ci.yml baseline entry to the file's true post-hoist size. - Harden the mirror's witnesses: fixtures hoisted out of the bash-gated block so they run on the merge-group Windows/macOS lanes, a backslash-path case pinning the pathspec normalization on every lane, a shallow-clone fixture pinning the mirror's fetch arm (the production path), and a parity fixture running the bash gate and the JS mirror on the same repo state.
|
🤖 Addressed the latest review feedback (round 3/100). What changed, and what I pushed back on: · 已处理最新评审反馈(第 3/100 轮)。改动内容与我反驳保留之处如下: Autofix review round — PR #9931All 7 inline findings addressed in one commit ( Dispositions
Mutation probes (each new guard witnessed)
Verification
中文说明Autofix 评审轮次 — PR #9931全部 7 条内联发现已在一个提交中处理完毕( 逐条处置
变异探针(每个新守卫都有见证)
验证
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
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/ci.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
scripts/tests/workflow-size.test.js:180 — [probe] allowance failure message renders NaN/undefined for entry-less filesscripts/tests/workflow-size.test.js:267 — [review] git-spawn runner pasted five times with drifted assertion strictness
Convergence: round 3 posted 4 inline comment(s), 4 of them reported for the first time; the previous round posted 7 (7 new). Findings keep coming back to the same files: scripts/tests/workflow-size.test.js (findings in round 2; 3 more now); .github/workflows/ci.yml (findings in round 2; 1 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. No Critical finding is open on this round, so merging and moving the remaining Suggestion threads to a follow-up issue is available as an ending — a merged pull request cannot diverge further. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
已审查——无阻断问题。 建议见行内评论。
未检查(工具限制,非阻断):the executable-script lint — .github/workflows/ci.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
收敛姿态下延后(第 3 轮,非阻断)——已记录,本轮不要求修改:共 2 条(原文未翻译,列表见上方英文部分)。
收敛情况:第 3 轮发布了 4 条行内评论,其中 4 条是首次提出;上一轮发布了 7 条(其中 7 条首次提出)。发现反复回到同一批文件:scripts/tests/workflow-size.test.js(第 2 轮已出过发现,本轮又有 3 条);.github/workflows/ci.yml(第 2 轮已出过发现,本轮又有 1 条)。一个不断再生兄弟发现的簇,通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因,或把独立的簇拆成单独的 PR,通常比逐条修复更快结束循环。本轮没有未决的 Critical,因此"合入后把剩余 Suggestion 线程转到后续 issue"是一个可选的结束方式——已合入的 PR 不会继续发散。(仅为观察——本轮评审未因此扣留任何内容。)
— qwen3.8-max via Qwen Code /review (v0.22.0)
| # baseline went stale on main" (#9904). Wired once here so every lane | ||
| # inherits it — the gate step and every `npm run test:ci` step, whatever | ||
| # it is named — instead of each step hand-wiring a copy. | ||
| WORKFLOW_SIZE_BASE_SHA: '${{ github.event.pull_request.base.sha || github.event.merge_group.base_sha }}' |
There was a problem hiding this comment.
[Suggestion] The env wiring added here makes the test job's checkout comment (lines ~270–273: "Shallow: nothing here walks git history (the verify guard below checks head.sha == HEAD, schema/tests touch only the working tree)") false: the size-gate step in this same job now runs git rev-parse / git fetch --depth=1 origin <sha> / git show against this checkout once WORKFLOW_SIZE_BASE_SHA is set, and the vitest mirror in npm run test:ci does the same. The inventory matters because it justifies the checkout tuning: a maintainer hardening this checkout — e.g. setting persist-credentials: false — would make both fetches fail, the ratchet would fail closed, and the stale-baseline red wall this PR removes would return on every stale-baseline PR with nothing in the checkout visibly consuming git to point at the cause. The "depth 1 is enough" conclusion itself still holds; only the stated rationale is stale. Suggested rewrite of the parenthetical:
# Shallow: nothing here walks git history except on demand (the verify guard
# below checks head.sha == HEAD; the size gate and its vitest mirror fetch
# the PR's base commit at depth 1 when the baseline went stale; everything
# else touches only the working tree).
中文说明
此处新增的环境变量接线使 test 任务的检出注释(约第 270–273 行:"Shallow: nothing here walks git history(下方的验证守卫检查 head.sha == HEAD,schema/测试只碰工作树)")不再成立:设置 WORKFLOW_SIZE_BASE_SHA 后,同一任务中的体积门步骤会对这个检出运行 git rev-parse / git fetch --depth=1 origin <sha> / git show,npm run test:ci 中的 vitest 镜像也会做同样的事。这份"清单"之所以重要,是因为它为检出调优提供了理由:一位维护者若据此加固检出——例如设置 persist-credentials: false——会让两个 fetch 都失败、棘轮失败关闭,本 PR 要消除的陈旧基线红墙会在每个基线陈旧的 PR 上重现,而检出中却看不到任何明显消费 git 的地方。"depth 1 足够"的结论本身仍然成立,只是写出的理由已经过时。建议把括号内的说明改写为:
# Shallow: nothing here walks git history except on demand (the verify guard
# below checks head.sha == HEAD; the size gate and its vitest mirror fetch
# the PR's base commit at depth 1 when the baseline went stale; everything
# else touches only the working tree).
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // including the merge-queue lanes where the mirror is the only ratchet | ||
| // enforcer — without each step hand-wiring a copy a future lane could | ||
| // forget. | ||
| const workflowEnv = ciWorkflow.match(/^env:[\s\S]*?\njobs:/m)?.[0]; |
There was a problem hiding this comment.
[Suggestion] The only test pinning the new ci.yml env expression uses three substring checks, which cannot discriminate the expression's operator or an added fallback. A one-line mutation || → && evaluates to the empty string on both pull_request and merge_group events (Actions && returns the first falsy operand), so the gate and mirror would fail closed on every stale-baseline run — re-creating the exact #9904 red wall this PR removes — while all three toContain assertions stay green; a mutation appending || github.sha likewise passes this test and would make workflow_dispatch runs resolve the base to the checked-out commit itself, failing the ratchet open on that lane. Both mutants were probed in a scratch tree: each stays green against the shipped assertions, while an anchored full-expression regex rejects them. Pin the whole line instead of its substrings, e.g.:
expect(workflowEnv).toMatch(
/^\s*WORKFLOW_SIZE_BASE_SHA: '\$\{\{ github\.event\.pull_request\.base\.sha \|\| github\.event\.merge_group\.base_sha \}\}'$/m,
);中文说明
钉住新 ci.yml 表达式的唯一测试使用了三个子串检查,无法区分表达式中的运算符或额外追加的回退项。单行变异 || → && 在 pull_request 与 merge_group 两种事件上都求值为空字符串(Actions 的 && 返回第一个假值操作数),于是门与镜像会在每次基线陈旧时失败关闭——重新制造出本 PR 要消除的 #9904 红墙——而三个 toContain 断言依旧全绿;追加 || github.sha 的变异同样能通过本测试,并会让 workflow_dispatch 运行把 base 解析为检出的提交本身,使棘轮在该车道上失败放行。两个变异体都已在临时树中实测:在现有断言下均保持绿色,而锚定整行的完整表达式正则能拒绝它们。请钉住整行而非子串,例如:
expect(workflowEnv).toMatch(
/^\s*WORKFLOW_SIZE_BASE_SHA: '\$\{\{ github\.event\.pull_request\.base\.sha \|\| github\.event\.merge_group\.base_sha \}\}'$/m,
);— qwen3.8-max via Qwen Code /review (v0.22.0)
| fileMatchesBase(file), | ||
| `${file} has no entry in .size-baseline and differs from the PR's base`, | ||
| ).toBe(true); |
There was a problem hiding this comment.
[Suggestion] The mirror's failure messages drop the remediation text their sibling the bash gate emits ("Add 'NNN name.yml' so its growth is tracked." / "update .size-baseline in this PR and say why"), and on the merge-queue macOS/Windows lanes — which never run the bash gate — the mirror's assertion text is the only diagnostic. A merge-queue lane that goes red because a workflow reached main without a baseline entry and this PR also touches the file leaves the author reconstructing the remediation unaided; the allowance-arm message further down has the same gap. Append the gate's remediation to this message:
| fileMatchesBase(file), | |
| `${file} has no entry in .size-baseline and differs from the PR's base`, | |
| ).toBe(true); | |
| fileMatchesBase(file), | |
| `${file} has no entry in .size-baseline and differs from the PR's base — add its byte size to .size-baseline in this PR so its growth is tracked`, | |
| ).toBe(true); |
中文说明
镜像的失败消息丢掉了其姊妹 bash 门会输出的修复指引("Add 'NNN name.yml' so its growth is tracked." / "update .size-baseline in this PR and say why"),而在从不运行 bash 门的合并队列 macOS/Windows 车道上,镜像的断言文本是唯一的诊断信息。当某个合并队列车道因为一个 workflow 没有基线条目就进入 main、且本 PR 又修改了该文件而变红时,作者只能独自重建修复方法;下方的允许增量分支消息也有同样的缺口。请给此消息补上与门一致的修复指引:
| fileMatchesBase(file), | |
| `${file} has no entry in .size-baseline and differs from the PR's base`, | |
| ).toBe(true); | |
| fileMatchesBase(file), | |
| `${file} has no entry in .size-baseline and differs from the PR's base — add its byte size to .size-baseline in this PR so its growth is tracked`, | |
| ).toBe(true); |
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const env = hermeticGateEnv(fetchDir); | ||
| const bare = join(fetchDir, 'origin.git'); |
There was a problem hiding this comment.
[Suggestion] The ~40-line shallow-clone fetch fixture (bare init → symbolic-ref → uploadpack.allowAnySHA1InWant → two-commit seed → push → depth-1 clone) now exists twice in this file — here for the vitest mirror and at lines ~544–588 for the bash gate — with only the seeded file contents differing, and the copies already drifted: the gate copy spells the clone URL file://${bare} while this copy uses pathToFileURL(bare).href (only the latter is Windows-lane-safe). The drift proves independent maintenance: any future fixture fix (git-version behaviour of allowAnySHA1InWant, default-branch handling, URL spelling) must be applied in both places, and missing one silently weakens one runtime's fetch-arm test. The parity describe guards the two predicates against divergence; nothing guards the two fixture builders. Extract one helper parameterised on the seeded files — e.g. seedShallowClone({ root, env, seedFiles }) returning { checkout, baseSha } — building the clone URL once via pathToFileURL; each test keeps its own precondition pin and subject invocation.
中文说明
约 40 行的浅克隆 fetch 夹具(bare init → symbolic-ref → uploadpack.allowAnySHA1InWant → 两次提交的种子 → push → depth-1 克隆)现在在本文件中存在两份——此处供 vitest 镜像使用,另一份在约第 544–588 行供 bash 门使用——仅种子文件内容不同,且两份已经漂移:门的副本把克隆 URL 写作 file://${bare},而本副本使用 pathToFileURL(bare).href(只有后者在 Windows 车道上安全)。漂移证明了两份代码在独立维护:未来任何夹具修复(allowAnySHA1InWant 的 git 版本行为、默认分支处理、URL 拼写)都必须同时应用到两处,漏掉任何一处都会悄悄削弱其中一种运行时的 fetch 分支测试。parity describe 防止两个谓词彼此漂移,但没有任何东西防止两个夹具构造器漂移。请抽取一个以种子文件为参数的公共助手——例如 seedShallowClone({ root, env, seedFiles }) 返回 { checkout, baseSha }——并在一处通过 pathToFileURL 构造克隆 URL;每个测试保留自己的前置钉桩与被测对象调用。
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
🤖 Addressed the latest review feedback (round 4/100). What changed, and what I pushed back on: · 已处理最新评审反馈(第 4/100 轮)。改动内容与我反驳保留之处如下: Autofix review round summary — PR #9931 (round 3)All four inline Suggestion findings are resolved in code in commit Findings and dispositions[rc:3847154691] ci.yml:59 — stale shallow-checkout comment — ResolvedVerified the claim: with [rc:3847154694] workflow-size.test.js:73 — env expression pinned only by substrings — ResolvedReplaced the three [rc:3847154700] workflow-size.test.js:167 — mirror failure messages drop the gate's remediation — ResolvedBoth mirror assertion messages now carry the remediation their bash-gate sibling emits: the missing-entry arm says to add the file's byte size to [rc:3847154713] workflow-size.test.js:702 — shallow-clone fixture duplicated and drifted — ResolvedExtracted one Recorded by the reviewer but not requested this roundThe review body deferred two items under its convergence posture: the line-180 NaN/undefined rendering (addressed above because it sits in the very message rc:3847154700 edits) and the line-267 git-spawn runner pasted five times (partially absorbed by the shared helper; the three remaining copies serve different fixture contexts and were left as the reviewer directed). Conflict notesNone — VerificationCommands actually run this round (runner: linux, git 2.39.5):
中文说明Autofix 审查轮次总结 — PR #9931(第 3 轮)四条行内 Suggestion 发现已全部在提交 发现与处置[rc:3847154691] ci.yml:59 — 过时的浅克隆检出注释 — 已解决已核实该说法:在 workflow 层级接好 [rc:3847154694] workflow-size.test.js:73 — env 表达式仅被子串钉住 — 已解决将三个 [rc:3847154700] workflow-size.test.js:167 — 镜像的失败消息丢掉了门的修复指引 — 已解决镜像的两条断言消息现在都带上了其 bash 门姊妹会输出的修复指引:缺少条目分支提示把该文件的字节数加入本 PR 的 [rc:3847154713] workflow-size.test.js:702 — 浅克隆重复且已漂移 — 已解决抽取了一个 审查者记录但本轮未要求修改的项审查正文按收敛姿态延后了两项:180 行的 NaN/undefined 渲染(因其恰好位于 rc:3847154700 所编辑的消息中,上面已一并处理)和 267 行粘贴五份的 git-spawn 运行器(已被共享助手部分吸收;剩余三份服务于不同的夹具上下文,按审查者的指示保留)。 冲突说明无 — 验证本轮实际运行的命令(运行环境:linux,git 2.39.5):
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
There was a problem hiding this comment.
No issues found. LGTM! ✅
Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/ci.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
中文说明
未发现问题。LGTM!✅
未检查(工具限制,非阻断):the executable-script lint — .github/workflows/ci.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
— qwen3.8-max via Qwen Code /review (v0.22.0)
yiliang114
left a comment
There was a problem hiding this comment.
Approved. Verified at head 8b3b24e: yamllint single-quote fix in ci.yml, and both mirror tests (baseline entry + allowance) now carry the fileMatchesBase stale-baseline escape, closing all three earlier Criticals; bot re-approved after round-3. Remaining open threads are Suggestion-level.
One conflict, resolved as the union of both sides: ci.yml's HELPER_TESTS keeps this branch's ci/classify-platform-sensitivity.test.mjs entry and gains main's WORKFLOW_SIZE_BASE_SHA env block (#9931/#9904) beneath it. Also bump ci.yml's .size-baseline entry 74315 -> 81137 in the same commit: main's workflow-size ratchet arrived while this branch was open, and this PR's growth of ci.yml is real — the schedule trigger, the classify_platform job, and the revived lane wiring. Without the bump the ratchet's vitest mirror fails `npm run test:scripts` on every lane and the shell gate fails the ubuntu gate step.
What this PR does
The workflow-size gate's growth ratchet compares every workflow against the checked-in size baseline. This change downgrades the ratchet's hard failure to a non-blocking warning whenever the PR's copy of the grown file is byte-identical to the PR's base commit — i.e. when the PR itself did not touch the file. The base commit reaches the gate through
WORKFLOW_SIZE_BASE_SHA, wired in the CI workflow from the pull-request and merge-group event payloads. Any base that cannot be resolved (local run, fetch failure) keeps the strict failure, so the gate fails closed, never open.Why it's needed
Fixes #9904. The baseline is a checked-in file, so a PR that grows a workflow without bumping the baseline in the same PR leaves the baseline stale on main — and from then on every other open PR fails the gate on a file it never touched, within ~20 seconds of CI starting and before any test runs. That happened twice in about two weeks (#9747, #9822), and each time the whole queue stayed red until a one-line baseline-fix PR landed. This scopes the blast radius to the PR that actually grew the file while keeping the staleness visible: unrelated PRs now get a warning that names exactly the one-line fix.
Reviewer Test Plan
How to verify
git fetch --depth=1 origin <sha>(bare repo + depth-1 clone overfile://).WORKFLOW_SIZE_BASE_SHAwiring from the CI workflow, turns the corresponding tests red.Evidence (Before & After)
N/A (CI gate logic, no user-visible UI).
Tested on
macOS: bash 5.3 (Homebrew) for the execution fixtures; under the system bash 3.2 they skip via the existing capability probe, unchanged. Windows/Linux lanes are covered by CI.
Environment (optional)
Unit tests only:
npx vitest run --config ./scripts/tests/vitest.config.ts scripts/tests/workflow-size.test.js, plus the patched script run directly against the repository.Risk & Scope
workflow_dispatch) keep the exact pre-fix behavior.Linked Issues
Fixes #9904
中文说明
这个 PR 做了什么
workflow-size 门的增长棘轮会把每个 workflow 与入库的体积基线比较。本改动在「该 PR 里这个变大文件与其 base 提交逐字节相同」(即 PR 本身没碰过该文件)时,把棘轮的硬失败降级为非阻断告警。PR 的 base 提交通过
WORKFLOW_SIZE_BASE_SHA传入,由 CI workflow 从 pull-request 与 merge-group 事件负载接线。任何无法解析的 base(本地运行、fetch 失败)保持严格失败——门只失败关闭,绝不失败放行。为什么需要
修复 #9904。基线是入库文件,一个改大了 workflow 却没在同 PR 更新基线的 PR 会让 main 上的基线保持陈旧——此后每个其他开放 PR 都会在约 20 秒内、跑任何测试之前,就在一个自己从未碰过的文件上挂掉这个门。约两周内发生了两次(#9747、#9822),每次整个队列都红着,直到一个一行基线修复 PR 合入。本改动把爆炸半径限定在真正改大文件的 PR 上,同时保持陈旧状态可见:无关 PR 现在收到一条告警,里面直接写明那行修复该怎么做。
评审者测试计划
如何验证
git fetch --depth=1 origin <sha>获取:bare 仓库 +file://depth-1 克隆)。WORKFLOW_SIZE_BASE_SHA接线,对应测试都会变红。证据(前后对比)
N/A(CI 门逻辑,无用户可见界面)。
测试环境
macOS:执行夹具用 bash 5.3(Homebrew);在系统 bash 3.2 下由既有的能力探针跳过,行为不变。Windows/Linux 车道由 CI 覆盖。
环境(可选)
仅单元测试:
npx vitest run --config ./scripts/tests/vitest.config.ts scripts/tests/workflow-size.test.js,另将打过补丁的脚本直接在本仓库上运行。风险与范围
workflow_dispatch)保持与修复前完全一致的行为。关联 Issue
Fixes #9904