Comfy-Org/github-workflows

Fork 0

mirror of https://github.com/Comfy-Org/github-workflows.git synced 2026-11-05 00:17:37 +00:00

[PR #218] [MERGED] ci(pr-size): narrow the size job's fetch and short-circuit a bypassed run (BE-8940) #225

New issue

Closed

opened 2026-10-26 22:37:32 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:37:32 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/218
Author: @mattmillerai
Created: 8/24/2026
Status: ✅ Merged
Merged: 8/24/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-8940-pr-size-fetch

📝 Commits (3)

a1dca0a ci(pr-size): narrow the size job's fetch and short-circuit a bypassed run (BE-8940)
82dfea5 ci(pr-size): trust the merge base before counting from it (BE-8940)
30753da ci(pr-size): scope the concurrency group to the reusable and close the publish race (BE-8940)

📊 Changes

2 files changed (+327 additions, -8 deletions)

View changed files

📝 .github/workflows/pr-size.yml (+284 -8)
📝 docs/callers/pr-size.md (+43 -0)

📄 Description

ELI-5

The PR-size check was downloading the entire consumer repo — every branch, every tag — just to compare two commits. On a big monorepo that download alone used up the job's whole 5-minute allowance, so the check got killed before it ever counted anything and showed up as a red X on an otherwise-green PR. Now it downloads just the two commits it actually needs plus enough history to line them up. Separately, if a PR already carries the "this is allowed to be big" label, the job now skips all the expensive work instead of doing it and then throwing the answer away.

What changed

1. The fetch is narrowed (this is the half that produces red checks). The PR-head checkout drops from fetch-depth: 0 to fetch-depth: 1, and a new step fetches exactly what the counting tool asks git for — the head commit, the base commit, and enough history for git merge-base to resolve. That is the full set: merge-base, the base...head three-dot diff, check-attr --source <base> against .gitattributes, and show <ref>:<path> for the generated-marker scan. None of it needs every branch and every tag.

The fetch starts at --depth=100, then walks a bounded ladder of --deepen rungs (400 / 1600 / 6400) only while the merge base is still unreachable and the repo is still shallow, then falls back to complete history for those two commits only. A rung that errors is not fatal — it breaks to the fallback, because a complete answer is one fetch away and a rung failing must never redden a consumer's check. If the merge base is still unresolvable after complete history, the step fails loudly rather than letting the tool report could not resolve merge base three steps later.

2. persist-credentials: false is preserved on the PR-head checkout. No change, and no request to change it. That is why the new fetch carries its own credential: the job's own read-only GITHUB_TOKEN, injected via GIT_CONFIG_COUNT/GIT_CONFIG_KEY_0/GIT_CONFIG_VALUE_0 so it applies to that step's git processes and nothing else — never written to .git/config, and (unlike git -c) never in argv. The base64 form is ::add-mask::-ed, since it is not the literal token the runner already masks.

A partial clone (filter: blob:none / tree:0) is deliberately NOT used. With no persisted credential there is nothing left to lazily fetch the missing blobs with, and the consumer repos are private — it would appear to work on a public repo and fail on every private one. timeout-minutes is also left at 5 on purpose: raising it would convert 5-minute failures into 6-minute successes and leave the steady-state cost in place.

3. A bypass-labelled run short-circuits. Both checkouts, Set up Go, Build check-pr-size and Check PR size are gated on env.PR_SIZE_BYPASS != 'true', and one cheap step writes a "Bypassed via <label> ✅" report instead. It writes pr-size-report.md and both flag files (pr-size-over-cap, pr-size-tests-decisive), because the comment job bails out on an empty report artifact and leaves any existing sticky comment untouched — skip that and a PR that was red and then got bypass-labelled keeps a stale red comment forever, strictly worse than paying for the full run.

Require a pinned workflows_ref stays unconditional: a caller that forgot the pin is misconfigured whether or not this particular PR happens to carry the label, and the guard is a shell test that costs nothing. Record over-cap flag is the one existing step gated off the bypassed path, so a skipped steps.check's empty outputs cannot overwrite the flags the bypass step just wrote.

Known tradeoff, stated rather than engineered around: the bypassed report loses the real line count (no more "1500 counted / 1000 cap"), because nothing counted the lines. Recovering it would mean paying for the checkout and build the short-circuit exists to skip. The report says so and points at removing the label; docs/callers/pr-size.md carries the same note.

Verification

Counts are unchanged — that was measured, not assumed. I built the tool from this branch and ran it twice per case: once in a full clone (the fetch-depth: 0 object set) and once in a repo built by replaying actions/checkout @ fetch-depth: 1 followed by this PR's new fetch step verbatim. Reports were compared byte-for-byte.

Case (this repo)	counted	full-clone vs narrowed
PR #217	254	identical
PR #213	641	identical
PR #212	173	identical
old head vs today's tip (base 209 commits ahead of the fork point)	0	identical

The ladder resolves a genuinely distant fork point. Against a real GitHub remote on a large public repo, an old PR head (≈1500 PRs back) versus today's default-branch tip: --depth=100 then one --deepen=400 rung, 2s, merge base matching the full clone exactly.

Cost. Same large public repo, same two commits, from my workstation (not a runner, so treat as a ratio not an absolute): actions/checkout's fetch-depth: 0 refspec — +refs/heads/*:… +refs/tags/*:… — took 18s and produced a 100M .git; the narrowed sequence took 7s and 18M. On this repo the narrowed fetch leaves a 2.9M .git versus a 5.7M full clone.

The bypass transition — the one the short-circuit can silently break — was exercised by replaying the comment job's upsert script against a stubbed gh, with the exact artifact the new step writes: (a) PR with an existing red sticky comment → PATCHed to the bypassed ✅ report, no new comment; (b) PR with no comment → "nothing to post" (no spam); (c) regression guard, empty report artifact → existing comment left untouched.

Suites. cd scripts/check-pr-size && gofmt -l . && go vet ./... && go test ./... — clean. python3 -m unittest discover -s .github/workflow-pins/tests -p 'test_*.py' — 216 passed; python3 .github/workflow-pins/check_workflow_pins.py — OK, 11 workflows, none with a default, every ref checkout guarded.

Judgment calls, and what I could not verify

Acceptance on the actual affected consumer is unverified. The failing run this came from is on a private consumer repo I have no access to, so I could not re-run it, re-read its job log, or measure the job's wall-clock there. Everything above is a like-for-like reproduction on public repos. The private run URL and repo name are deliberately not reproduced here (public repo).
go test ./... fails on a workstation whose init.defaultBranch is main — TestResolveMergeBaseWithAdvancedBaseBranch does git init then checkout -b main. Pre-existing and environmental (CI's git init defaults to master); the suite is green with GIT_CONFIG_GLOBAL pinning init.defaultBranch=master. No Go file is touched by this PR.
check_agents_md.py --root . fails on main too — AGENTS.md is 305 lines against a 200-line ceiling. Untouched here and out of scope.
The half I am not fixing, counted: three other reusable workflows in this repo still use fetch-depth: 0 — cursor-review.yml (3 uses), groom.yml (3), refresh-reviewers.yml (1); 7 occurrences total. Not all are defects — groom.yml and refresh-reviewers.yml do whole-repo and git-history analysis where full history is the point — but nobody has checked which. That audit is explicitly out of scope here and is not fixed by this PR.
No feature flag. This is CI plumbing with no user-visible product surface.
No caller-fleet change needed. Merging trips bump-pr-size-callers.yml's path filter and the fleet rolls on its own; consumers carry only a thin caller.

Refs BE-8940 — kept as a reference rather than a closing keyword: the ticket's first acceptance criterion is about wall-clock on a monorepo-sized consumer, and that consumer is a private repo this run had no access to, so the criterion is argued from a public-repo reproduction rather than measured where it was reported.

Review round 1 — what the panel changed

Five review threads, all in the new fetch step; every one is addressed in code rather than argued away.

A shallow merge base can be WRONG, not just missing (the high finding). Grafted boundary commits look parentless, so while the true fork point is still hidden git merge-base returns an older common ancestor that some other path does reach — a long-lived branch rooted before the fork point and merged into the base branch recently is enough. The tool would then count every base-branch change in between against a small PR. Reproduced before fixing: on a purpose-built repo (fork point 100 commits in, base 200 commits past it, a 10-commit branch rooted 40 commits before the fork and merged near the tip), the first ladder resolved to that older ancestor. have_merge_base now accepts an answer only once every shallow boundary the walk can reach is itself an ancestor of that answer — all the graft hides is then older than the merge base, so no better common ancestor can be in it. Unprovable ⇒ deepen, worst case to the complete-history fallback.

Runs are serialized per PR. (Both details in this paragraph were revised in round 2 below — the group name and the comment gate.) A workflow-level concurrency group keyed on the PR number with cancel-in-progress: true. The bypassed path finishes in seconds while a counted run takes minutes, so labelling a PR whose run is still in flight let the slow run finish LAST and overwrite the ✅ bypass report with the stale red verdict. The comment job already bails on a cancelled pr-size, so a superseded run publishes nothing. Same shape as pr-area-label.yml; docs/callers/pr-size.md states the consumer-visible behaviour.

Three smaller ones. The injected auth header is scoped to http.${GITHUB_SERVER_URL}/.extraheader — the key actions/checkout writes — so it cannot ride along to a redirect on another host. The mandatory first rung retries once instead of exiting the step under set -e on a transient 5xx. A failed deepen rung continues to the next depth instead of breaking straight into the most expensive fetch in the step.

Cost of the merge-base fix, measured on three built repos: the false-positive case resolves to the true merge base after one extra --deepen=400 rung (no unshallow); a normal PR (fork 3 back, 300-commit base) still resolves at --depth=100, 100 commits, repo still shallow — fast path unchanged; a distant fork (250 back of 600) resolves in two fetches, no unshallow.

Review round 2 — what the panel changed

Six threads. Four taken in code, two answered with evidence.

The concurrency group is namespaced, and the caller constraints are documented (high). pr-size-<n> is exactly the name a caller reaches for, and ci-groom.yml already records what happens next: the caller holds the group while its reusable uses: job waits to acquire the same one, and the run hangs to timeout. It is now pr-size-reusable-<n>. It stays at workflow level rather than moving to jobs.pr-size.concurrency, because job scope only ever cancels a pr-size job that is still running — and the losing case is the other one, a counted run whose job already finished red before a bypass run superseded it, leaving nothing to cancel and its comment job free to overwrite the newer ✅. docs/callers/pr-size.md now carries both consumer-facing constraints in the ci-groom.yml register: do not declare this group yourself, and call this from a dedicated workflow file rather than one job of a larger ci.yml, since cancellation is run-scoped.

The publish race was only half closed (high). Round 1 relied on always() && needs.pr-size.result != 'cancelled', but always() runs on a cancelled run by design and the needs test only sees cancellation that lands while pr-size is still going. A run superseded after its pr-size job had already finished still PATCHed its stale report over the newer verdict — the likelier ordering, given the short-circuit is the thing that finishes first. Now !cancelled(), the spelling cursor-review.yml already uses for this hazard.

The trustworthiness check is one pass. rev-list BASE HEAD --not mb intersected with the shallow set replaces three merge-base --is-ancestor calls per boundary line. Fuzzed against the old form over 60 shallow states (six repo shapes × ten fetch depths): zero disagreements. The stated motivation did not hold up and is reported as such below.

A fetch failure no longer masquerades as a merge-base failure. Each rung records its error — cleared on success, so it means "the last fetch failed" rather than "some fetch failed once" — and a permanently unfetchable base SHA (force-pushed and GC'd, a pruned fork ref, a token without read access) now says so. A failed rung still prints to plain stderr, not ::warning::, so a transient rung the ladder recovers from does not annotate a passing check.

Not taken: relaxing the merge-base check to one side. The proposal was to accept when all HEAD-reachable boundaries are ancestors of mb or all BASE-reachable ones are. Run against round 1's own repro, the one-side OR re-admits the original false positive — it accepts the wrong ancestor c60 where the both-sides check deepens once and resolves the true fork point. It accepts because there are no HEAD-reachable boundaries there at all, so that half is vacuously true. The soundness argument has a gap: merge-base also misses a better ancestor that is present in the graph but unreachable from one side, which is exactly this case, and only the BASE-side half catches it. Both halves are load-bearing.

Not taken: keying the group on head SHA + bypass-label state. The underlying waste is real — an unrelated auto-labeler preempts an in-flight counted run — but putting bypass state in the key lands the bypass run and the counted run in different groups, so the bypass run stops cancelling the counted one and the round-1 race reopens. Fixed at the caller layer instead: docs/callers/pr-size.md shows an optional job-level if: that skips label events other than the bypass label, since GitHub cannot filter labeled triggers by name.

Measurements behind round 2.

Fixture (built, then run through the real step)	`--deepen` rungs	outcome
round 1's false-positive shape	2 fetches, no unshallow	true merge base
300-commit base, 10 merged 5-commit branches	2 fetches, no unshallow	correct
250-commit base, one 150-commit long-lived branch	2 fetches, no unshallow	correct
3320-commit base, 3000 pre-fork, 20 merged branches	2 fetches, no unshallow (still shallow, 500 objects)	correct
6440-commit base, 40 × 150-commit branches merged into the last 40 commits	2 fetches, no unshallow	correct
normal PR (fork 3 back, 300-commit base)	1 fetch, still shallow, 100 objects	fast path unchanged

The objection that the both-sides check makes --unshallow the normal outcome on merge-heavy history did not reproduce on any of them, including the fixture built specifically to strand a boundary inside every merged branch. The per-boundary cost premise did not either: 5 calls took 0.055s (loop) vs 0.053s (rev-list) at depth 100, and 0.218s vs 0.221s after --deepen=400, with the boundary count going 2 → 1 rather than accumulating. The rewrite is kept for the bounded worst case and the smaller code, not for a measured speedup.

Provenance

Authored by: agent-work loop
Verified: round 2 — python3 -c yaml.safe_load on the edited workflow OK; check_workflow_pins.py OK (11 workflows, 0 exempt) and its 216 unit tests pass; unittest suites for cursor-review / agents-md-integrity / groom / workflow-pins all OK; shellcheck -x clean on the bump-callers scripts and on the merge-base step extracted verbatim from the workflow; the extracted step driven end-to-end against 6 built fixtures — correct merge base in all 6, plus a negative case (unfetchable base SHA) asserting the new error blames the fetch; old vs new have_merge_base fuzzed over 60 shallow states with 0 disagreements; go test ./... ok with init.defaultBranch=master. Round 1 — as recorded above; unchanged.
Deviations: acceptance criterion 1 (job wall-clock on a monorepo-sized consumer) not measured on the affected consumer — it is a private repo this run cannot read; substituted a like-for-like measurement on a large public repo. Round 2 — two review findings were answered with evidence rather than code (the one-side merge-base relaxation, which reproduces the original false positive; and the concurrency-key change, which would reopen the round-1 publish race); the label-event waste behind the second is addressed at the caller layer in the docs rather than in the reusable. actionlint reports two SC2016 info notes on the bypass-report step's markdown backticks — introduced earlier in this PR, not a gate (nothing in this repo's CI runs actionlint) and left as-is. check_agents_md.py --root . still fails on AGENTS.md being 305 lines against a 200-line ceiling — pre-existing on main, untouched, out of scope.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/github-workflows/pull/218 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/24/2026 **Status:** ✅ Merged **Merged:** 8/24/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-8940-pr-size-fetch` --- ### 📝 Commits (3) - [`a1dca0a`](https://github.com/Comfy-Org/github-workflows/commit/a1dca0a7db96470e796e33d8bbcd101bcabca5e7) ci(pr-size): narrow the size job's fetch and short-circuit a bypassed run (BE-8940) - [`82dfea5`](https://github.com/Comfy-Org/github-workflows/commit/82dfea5ebc775a518119567b5f4e4a1a8cd62aaf) ci(pr-size): trust the merge base before counting from it (BE-8940) - [`30753da`](https://github.com/Comfy-Org/github-workflows/commit/30753dad5d4193d6366b2a5667c5d246ea8efe5c) ci(pr-size): scope the concurrency group to the reusable and close the publish race (BE-8940) ### 📊 Changes **2 files changed** (+327 additions, -8 deletions) <details> <summary>View changed files</summary> 📝 `.github/workflows/pr-size.yml` (+284 -8) 📝 `docs/callers/pr-size.md` (+43 -0) </details> ### 📄 Description ## ELI-5 The PR-size check was downloading the *entire* consumer repo — every branch, every tag — just to compare two commits. On a big monorepo that download alone used up the job's whole 5-minute allowance, so the check got killed before it ever counted anything and showed up as a red X on an otherwise-green PR. Now it downloads just the two commits it actually needs plus enough history to line them up. Separately, if a PR already carries the "this is allowed to be big" label, the job now skips all the expensive work instead of doing it and then throwing the answer away. ## What changed **1. The fetch is narrowed (this is the half that produces red checks).** The PR-head checkout drops from `fetch-depth: 0` to `fetch-depth: 1`, and a new step fetches exactly what the counting tool asks git for — the head commit, the base commit, and enough history for `git merge-base` to resolve. That is the full set: `merge-base`, the `base...head` three-dot diff, `check-attr --source <base>` against `.gitattributes`, and `show <ref>:<path>` for the generated-marker scan. None of it needs every branch and every tag. The fetch starts at `--depth=100`, then walks a bounded ladder of `--deepen` rungs (400 / 1600 / 6400) only while the merge base is still unreachable and the repo is still shallow, then falls back to complete history *for those two commits only*. A rung that errors is not fatal — it breaks to the fallback, because a complete answer is one fetch away and a rung failing must never redden a consumer's check. If the merge base is still unresolvable after complete history, the step fails loudly rather than letting the tool report `could not resolve merge base` three steps later. **2. `persist-credentials: false` is preserved on the PR-head checkout.** No change, and no request to change it. That is why the new fetch carries its own credential: the job's own read-only `GITHUB_TOKEN`, injected via `GIT_CONFIG_COUNT`/`GIT_CONFIG_KEY_0`/`GIT_CONFIG_VALUE_0` so it applies to that step's git processes and nothing else — never written to `.git/config`, and (unlike `git -c`) never in argv. The base64 form is `::add-mask::`-ed, since it is not the literal token the runner already masks. **A partial clone (`filter: blob:none` / `tree:0`) is deliberately NOT used.** With no persisted credential there is nothing left to lazily fetch the missing blobs with, and the consumer repos are private — it would appear to work on a public repo and fail on every private one. `timeout-minutes` is also left at 5 on purpose: raising it would convert 5-minute failures into 6-minute successes and leave the steady-state cost in place. **3. A bypass-labelled run short-circuits.** Both checkouts, `Set up Go`, `Build check-pr-size` and `Check PR size` are gated on `env.PR_SIZE_BYPASS != 'true'`, and one cheap step writes a "Bypassed via `<label>` ✅" report instead. It writes `pr-size-report.md` **and** both flag files (`pr-size-over-cap`, `pr-size-tests-decisive`), because the `comment` job bails out on an empty report artifact and leaves any existing sticky comment untouched — skip that and a PR that was red and then got bypass-labelled keeps a stale red comment forever, strictly worse than paying for the full run. `Require a pinned workflows_ref` stays **unconditional**: a caller that forgot the pin is misconfigured whether or not this particular PR happens to carry the label, and the guard is a shell test that costs nothing. `Record over-cap flag` is the one existing step gated off the bypassed path, so a skipped `steps.check`'s empty outputs cannot overwrite the flags the bypass step just wrote. **Known tradeoff, stated rather than engineered around:** the bypassed report loses the real line count (no more "1500 counted / 1000 cap"), because nothing counted the lines. Recovering it would mean paying for the checkout and build the short-circuit exists to skip. The report says so and points at removing the label; `docs/callers/pr-size.md` carries the same note. ## Verification **Counts are unchanged — that was measured, not assumed.** I built the tool from this branch and ran it twice per case: once in a full clone (the `fetch-depth: 0` object set) and once in a repo built by replaying `actions/checkout @ fetch-depth: 1` followed by this PR's new fetch step verbatim. Reports were compared byte-for-byte. | Case (this repo) | counted | full-clone vs narrowed | |---|---|---| | PR #217 | 254 | identical | | PR #213 | 641 | identical | | PR #212 | 173 | identical | | old head vs today's tip (base 209 commits ahead of the fork point) | 0 | identical | **The ladder resolves a genuinely distant fork point.** Against a real GitHub remote on a large public repo, an old PR head (≈1500 PRs back) versus today's default-branch tip: `--depth=100` then one `--deepen=400` rung, 2s, merge base matching the full clone exactly. **Cost.** Same large public repo, same two commits, from my workstation (not a runner, so treat as a ratio not an absolute): `actions/checkout`'s `fetch-depth: 0` refspec — `+refs/heads/*:… +refs/tags/*:…` — took 18s and produced a 100M `.git`; the narrowed sequence took 7s and 18M. On this repo the narrowed fetch leaves a 2.9M `.git` versus a 5.7M full clone. **The bypass transition — the one the short-circuit can silently break — was exercised** by replaying the `comment` job's upsert script against a stubbed `gh`, with the exact artifact the new step writes: (a) PR with an existing **red** sticky comment → `PATCH`ed to the bypassed ✅ report, no new comment; (b) PR with no comment → "nothing to post" (no spam); (c) regression guard, empty report artifact → existing comment left untouched. **Suites.** `cd scripts/check-pr-size && gofmt -l . && go vet ./... && go test ./...` — clean. `python3 -m unittest discover -s .github/workflow-pins/tests -p 'test_*.py'` — 216 passed; `python3 .github/workflow-pins/check_workflow_pins.py` — OK, 11 workflows, none with a default, every ref checkout guarded. ## Judgment calls, and what I could not verify - **Acceptance on the actual affected consumer is unverified.** The failing run this came from is on a private consumer repo I have no access to, so I could not re-run it, re-read its job log, or measure the job's wall-clock there. Everything above is a like-for-like reproduction on public repos. The private run URL and repo name are deliberately not reproduced here (public repo). - **`go test ./...` fails on a workstation whose `init.defaultBranch` is `main`** — `TestResolveMergeBaseWithAdvancedBaseBranch` does `git init` then `checkout -b main`. Pre-existing and environmental (CI's `git init` defaults to `master`); the suite is green with `GIT_CONFIG_GLOBAL` pinning `init.defaultBranch=master`. No Go file is touched by this PR. - **`check_agents_md.py --root .` fails on `main` too** — `AGENTS.md` is 305 lines against a 200-line ceiling. Untouched here and out of scope. - **The half I am not fixing, counted:** three *other* reusable workflows in this repo still use `fetch-depth: 0` — `cursor-review.yml` (3 uses), `groom.yml` (3), `refresh-reviewers.yml` (1); 7 occurrences total. Not all are defects — `groom.yml` and `refresh-reviewers.yml` do whole-repo and git-history analysis where full history is the point — but nobody has checked which. That audit is explicitly out of scope here and is not fixed by this PR. - **No feature flag.** This is CI plumbing with no user-visible product surface. - **No caller-fleet change needed.** Merging trips `bump-pr-size-callers.yml`'s path filter and the fleet rolls on its own; consumers carry only a thin caller. Refs BE-8940 — kept as a reference rather than a closing keyword: the ticket's first acceptance criterion is about wall-clock on a monorepo-sized consumer, and that consumer is a private repo this run had no access to, so the criterion is argued from a public-repo reproduction rather than measured where it was reported. ## Review round 1 — what the panel changed Five review threads, all in the new fetch step; every one is addressed in code rather than argued away. **A shallow merge base can be WRONG, not just missing (the high finding).** Grafted boundary commits look parentless, so while the true fork point is still hidden `git merge-base` returns an older common ancestor that some other path does reach — a long-lived branch rooted before the fork point and merged into the base branch recently is enough. The tool would then count every base-branch change in between against a small PR. Reproduced before fixing: on a purpose-built repo (fork point 100 commits in, base 200 commits past it, a 10-commit branch rooted 40 commits *before* the fork and merged near the tip), the first ladder resolved to that older ancestor. `have_merge_base` now accepts an answer only once **every shallow boundary the walk can reach is itself an ancestor of that answer** — all the graft hides is then older than the merge base, so no better common ancestor can be in it. Unprovable ⇒ deepen, worst case to the complete-history fallback. **Runs are serialized per PR.** *(Both details in this paragraph were revised in round 2 below — the group name and the `comment` gate.)* A workflow-level `concurrency` group keyed on the PR number with `cancel-in-progress: true`. The bypassed path finishes in seconds while a counted run takes minutes, so labelling a PR whose run is still in flight let the slow run finish LAST and overwrite the ✅ bypass report with the stale red verdict. The `comment` job already bails on a cancelled `pr-size`, so a superseded run publishes nothing. Same shape as `pr-area-label.yml`; `docs/callers/pr-size.md` states the consumer-visible behaviour. **Three smaller ones.** The injected auth header is scoped to `http.${GITHUB_SERVER_URL}/.extraheader` — the key `actions/checkout` writes — so it cannot ride along to a redirect on another host. The mandatory first rung retries once instead of exiting the step under `set -e` on a transient 5xx. A failed deepen rung `continue`s to the next depth instead of `break`ing straight into the most expensive fetch in the step. **Cost of the merge-base fix, measured on three built repos:** the false-positive case resolves to the true merge base after one extra `--deepen=400` rung (no unshallow); a normal PR (fork 3 back, 300-commit base) still resolves at `--depth=100`, 100 commits, repo still shallow — fast path unchanged; a distant fork (250 back of 600) resolves in two fetches, no unshallow. ## Review round 2 — what the panel changed Six threads. Four taken in code, two answered with evidence. **The concurrency group is namespaced, and the caller constraints are documented (high).** `pr-size-<n>` is exactly the name a caller reaches for, and `ci-groom.yml` already records what happens next: the caller holds the group while its reusable `uses:` job waits to acquire the same one, and the run hangs to timeout. It is now `pr-size-reusable-<n>`. It stays at **workflow** level rather than moving to `jobs.pr-size.concurrency`, because job scope only ever cancels a `pr-size` job that is still running — and the losing case is the other one, a counted run whose job already finished red before a bypass run superseded it, leaving nothing to cancel and its `comment` job free to overwrite the newer ✅. `docs/callers/pr-size.md` now carries both consumer-facing constraints in the `ci-groom.yml` register: do not declare this group yourself, and call this from a dedicated workflow file rather than one job of a larger `ci.yml`, since cancellation is run-scoped. **The publish race was only half closed (high).** Round 1 relied on `always() && needs.pr-size.result != 'cancelled'`, but `always()` runs on a cancelled run by design and the `needs` test only sees cancellation that lands while `pr-size` is *still going*. A run superseded after its `pr-size` job had already finished still PATCHed its stale report over the newer verdict — the likelier ordering, given the short-circuit is the thing that finishes first. Now `!cancelled()`, the spelling `cursor-review.yml` already uses for this hazard. **The trustworthiness check is one pass.** `rev-list BASE HEAD --not mb` intersected with the shallow set replaces three `merge-base --is-ancestor` calls per boundary line. Fuzzed against the old form over 60 shallow states (six repo shapes × ten fetch depths): zero disagreements. The stated motivation did not hold up and is reported as such below. **A fetch failure no longer masquerades as a merge-base failure.** Each rung records its error — cleared on success, so it means "the *last* fetch failed" rather than "some fetch failed once" — and a permanently unfetchable base SHA (force-pushed and GC'd, a pruned fork ref, a token without read access) now says so. A failed rung still prints to plain stderr, not `::warning::`, so a transient rung the ladder recovers from does not annotate a passing check. **Not taken: relaxing the merge-base check to one side.** The proposal was to accept when all HEAD-reachable boundaries are ancestors of `mb` **or** all BASE-reachable ones are. Run against round 1's own repro, the one-side OR **re-admits the original false positive** — it accepts the wrong ancestor `c60` where the both-sides check deepens once and resolves the true fork point. It accepts because there are no HEAD-reachable boundaries there at all, so that half is vacuously true. The soundness argument has a gap: `merge-base` also misses a better ancestor that is *present in the graph but unreachable from one side*, which is exactly this case, and only the BASE-side half catches it. Both halves are load-bearing. **Not taken: keying the group on head SHA + bypass-label state.** The underlying waste is real — an unrelated auto-labeler preempts an in-flight counted run — but putting bypass state in the key lands the bypass run and the counted run in *different* groups, so the bypass run stops cancelling the counted one and the round-1 race reopens. Fixed at the caller layer instead: `docs/callers/pr-size.md` shows an optional job-level `if:` that skips label events other than the bypass label, since GitHub cannot filter `labeled` triggers by name. **Measurements behind round 2.** | Fixture (built, then run through the real step) | `--deepen` rungs | outcome | |---|---|---| | round 1's false-positive shape | 2 fetches, no unshallow | true merge base | | 300-commit base, 10 merged 5-commit branches | 2 fetches, no unshallow | correct | | 250-commit base, one 150-commit long-lived branch | 2 fetches, no unshallow | correct | | 3320-commit base, 3000 pre-fork, 20 merged branches | 2 fetches, no unshallow (still shallow, 500 objects) | correct | | 6440-commit base, 40 × 150-commit branches merged into the last 40 commits | 2 fetches, no unshallow | correct | | normal PR (fork 3 back, 300-commit base) | 1 fetch, still shallow, 100 objects | fast path unchanged | The objection that the both-sides check makes `--unshallow` the normal outcome on merge-heavy history did not reproduce on any of them, including the fixture built specifically to strand a boundary inside every merged branch. The per-boundary cost premise did not either: 5 calls took 0.055s (loop) vs 0.053s (rev-list) at depth 100, and 0.218s vs 0.221s after `--deepen=400`, with the boundary count going 2 → 1 rather than accumulating. The rewrite is kept for the bounded worst case and the smaller code, not for a measured speedup. ## Provenance - **Authored by:** agent-work loop - **Verified:** round 2 — `python3 -c yaml.safe_load` on the edited workflow OK; `check_workflow_pins.py` OK (11 workflows, 0 exempt) and its 216 unit tests pass; `unittest` suites for cursor-review / agents-md-integrity / groom / workflow-pins all OK; `shellcheck -x` clean on the bump-callers scripts **and** on the merge-base step extracted verbatim from the workflow; the extracted step driven end-to-end against 6 built fixtures — correct merge base in all 6, plus a negative case (unfetchable base SHA) asserting the new error blames the fetch; old vs new `have_merge_base` fuzzed over 60 shallow states with 0 disagreements; `go test ./...` ok with `init.defaultBranch=master`. Round 1 — as recorded above; unchanged. - **Deviations:** acceptance criterion 1 (job wall-clock on a monorepo-sized consumer) not measured on the affected consumer — it is a private repo this run cannot read; substituted a like-for-like measurement on a large public repo. Round 2 — two review findings were answered with evidence rather than code (the one-side merge-base relaxation, which reproduces the original false positive; and the concurrency-key change, which would reopen the round-1 publish race); the label-event waste behind the second is addressed at the caller layer in the docs rather than in the reusable. `actionlint` reports two SC2016 *info* notes on the bypass-report step's markdown backticks — introduced earlier in this PR, not a gate (nothing in this repo's CI runs actionlint) and left as-is. `check_agents_md.py --root .` still fails on `AGENTS.md` being 305 lines against a 200-line ceiling — pre-existing on `main`, untouched, out of scope. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:37:32 +00:00