-
Notifications
You must be signed in to change notification settings - Fork 3.1k
fix(ci): scope workflow-size ratchet to the PR that grew the file #9931
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
35eeeb3
0b56a20
3044242
9dabee0
c855178
c4557cd
8ad2326
8b3b24e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,7 +31,42 @@ GROWTH_ALLOWANCE="${WORKFLOW_SIZE_GROWTH_ALLOWANCE:-4096}" | |
| # the slack for the next unreviewed 25 KB. | ||
| SLACK_BYTES=20000 | ||
|
|
||
| # The ratchet compares the worktree against a checked-in baseline, so a | ||
| # workflow that grew on main without the same-PR baseline bump leaves every | ||
| # OTHER open PR failing a gate on a file it never touched (red-walled the | ||
| # queue twice in two weeks: #9747, #9822). When the caller passes the PR's | ||
| # base commit in WORKFLOW_SIZE_BASE_SHA, the missing-entry and growth | ||
| # branches below hard-fail only if the PR actually changed the file; a | ||
| # byte-identical copy means the staleness is main-side drift and earns a | ||
| # warning instead. An unresolvable base (local run, fetch failure) falls | ||
| # back to the strict failure — the ratchet fails closed, never open. One | ||
| # residual window stays by design: if main edits the same workflow again | ||
| # after the PR branched, the comparison against the new base sees the PR's | ||
| # older copy as different and fails closed until that PR rebases — | ||
| # self-healing, and still fail-closed, so it is left alone rather than | ||
| # wiring the PR's changed-files list into a gate that today needs no API | ||
| # call. | ||
| BASE_SHA="${WORKFLOW_SIZE_BASE_SHA:-}" | ||
| # Returns 0 when the worktree copy of $1 is byte-identical to the base | ||
| # commit, 1 when it differs (or no base was given), and 2 when the base | ||
| # cannot be resolved — the callers add a diagnostic on 2, because a | ||
| # transient fetch failure and genuine PR growth need opposite remedies. | ||
| file_matches_base() { | ||
| local file="$1" | ||
| [[ -n "${BASE_SHA}" ]] || return 1 | ||
| if ! git rev-parse --verify --quiet "${BASE_SHA}^{commit}" >/dev/null && | ||
| ! git fetch --depth=1 --quiet origin "${BASE_SHA}"; then | ||
| return 2 | ||
| fi | ||
| git show "${BASE_SHA}:${file}" 2>/dev/null | cmp -s - "${file}" | ||
| } | ||
|
|
||
| unresolvable_base_note() { | ||
| echo "::warning::base ${BASE_SHA} could not be resolved (git fetch failed?) — failing strict; if this PR did not touch ${1}, re-run the job." | ||
| } | ||
|
|
||
| status=0 | ||
| warned_stale=0 | ||
| declare -A baseline=() | ||
| if [[ -r "${BASELINE_FILE}" ]]; then | ||
| # The || clause keeps an unterminated final line, which read reports as a | ||
|
|
@@ -72,17 +107,41 @@ for file in .github/workflows/*.yml .github/workflows/*.yaml; do | |
|
|
||
| base="${baseline[${file##*/}]:-}" | ||
| if [[ -z "${base}" ]]; then | ||
| echo "::error file=${file}::${file} has no entry in ${BASELINE_FILE}. Add '${size} ${file##*/}' so its growth is tracked." | ||
| status=1 | ||
| file_matches_base "${file}" | ||
| match=$? | ||
| if ((match == 0)); 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." | ||
| warned_stale=1 | ||
| else | ||
| echo "::error file=${file}::${file} has no entry in ${BASELINE_FILE}. Add '${size} ${file##*/}' so its growth is tracked." | ||
| status=1 | ||
| if ((match == 2)); then | ||
| unresolvable_base_note "${file}" | ||
| fi | ||
| fi | ||
| 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 | ||
| file_matches_base "${file}" | ||
| match=$? | ||
| if ((match == 0)); 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." | ||
| warned_stale=1 | ||
| 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." | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] When 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: 中文说明当 — qwen3.8-max via Qwen Code /review (v0.22.0) |
||
| status=1 | ||
| if ((match == 2)); then | ||
| unresolvable_base_note "${file}" | ||
| fi | ||
| fi | ||
| elif ((size + SLACK_BYTES < base)); then | ||
| echo "::warning file=${file}::${file} is ${size} bytes, $((base - size)) under its recorded ${base} — lower the entry in ${BASELINE_FILE} so the slack is not banked." | ||
| fi | ||
| done | ||
|
|
||
| if ((status == 0)); then | ||
| echo "✅ every workflow file is under the ${GATE_BYTES}-byte gate and within ${GROWTH_ALLOWANCE} bytes of its recorded baseline" | ||
| if ((warned_stale)); then | ||
| echo "✅ every workflow file is under the ${GATE_BYTES}-byte gate (stale-baseline warnings above — update ${BASELINE_FILE} on main)" | ||
| else | ||
| echo "✅ every workflow file is under the ${GATE_BYTES}-byte gate and within ${GROWTH_ALLOWANCE} bytes of its recorded baseline" | ||
| fi | ||
| fi | ||
| exit "${status}" | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -51,6 +51,12 @@ env: | |
| # new helper test can't be added to one path and silently dropped from the | ||
| # other. | ||
| HELPER_TESTS: '.github/scripts/pr-safety-precheck.test.mjs .github/scripts/cap-release-notes.test.mjs .github/scripts/ci/classify-profile.test.mjs .github/scripts/ci/classify-pr-profile.test.mjs .github/scripts/upsert-bot-comment.test.mjs .github/scripts/ci/main-failure-signature.test.mjs .github/scripts/classify-release-notes.test.mjs .github/scripts/dsw-swe-verified/make-manifest.test.mjs .github/scripts/dsw-swe-verified/make-terminal-bench-manifest.test.mjs .github/scripts/resolve-sandbox-image.test.mjs .github/scripts/web-shell-visuals-publish.test.mjs .github/scripts/web-shell-visuals-compose.test.mjs .github/scripts/serve-ab-diff.test.mjs .github/scripts/serve-ab-drive.test.mjs .github/scripts/qwen-triage-workflow.test.mjs .github/scripts/assign-issue-owner.test.mjs .github/scripts/auto-minimize-spam.test.mjs .github/scripts/ci-runner-routing.test.mjs' | ||
| # The growth ratchet and its vitest mirror compare each workflow against | ||
| # the PR's base commit to tell "this PR grew the file" apart from "the | ||
| # 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 }}' | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The env wiring added here makes the 中文说明此处新增的环境变量接线使 — qwen3.8-max via Qwen Code /review (v0.22.0) |
||
|
|
||
| jobs: | ||
| classify_pr: | ||
|
|
@@ -261,10 +267,13 @@ jobs: | |
| uses: 'actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10' # v6.0.3 | ||
| with: | ||
| ref: "${{ github.event.inputs.branch_ref || (github.event_name == 'pull_request' && format('refs/pull/{0}/head', github.event.pull_request.number)) || (github.event_name == 'merge_group' && github.event.merge_group.head_sha) || github.ref }}" | ||
| # Shallow: nothing here walks git history (the verify guard below checks | ||
| # head.sha == HEAD, schema/tests touch only the working tree). On the | ||
| # in-repo ECS runner a full-history clone is the heaviest transfer and | ||
| # chokes the squid egress proxy, flaking checkout. depth 1 is enough. | ||
| # 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). On the in-repo ECS runner a full-history clone is the | ||
| # heaviest transfer and chokes the squid egress proxy, flaking | ||
| # checkout. depth 1 is enough. | ||
| fetch-depth: 1 | ||
|
|
||
| # Guard against a stale checkout (e.g. a caching egress proxy serving an old | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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 assertingbytes <= recorded + allowanceagainst the checkout's own tree with no base-SHA awareness. The mirror has noskipIfand runs viatest:scriptsinsidenpm run test:cion every full-profile lane, andmerge_groupevents always classifyfull, 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 intotest:ci. The drift is not hypothetical: before this PR's bump,origin/mainhadci.ymlat 73850 bytes against the 69782 entry — 28 bytes inside the allowance.Make the two enforcement points agree about the drift case: either give the mirror the same base-scoped leniency (wire
WORKFLOW_SIZE_BASE_SHAinto thetestjob's env and pass/skip the allowance assertion when the file is byte-identical to the base, reusingfile_matches_basesemantics), or — if the mirror is deliberately kept strict as the final enforcer — amend this warning text and theqwen-autofix.mdparagraph to say full CI still fails until the one-line baseline-bump PR lands on main. (Onlygithub_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)