ci(policy): size the 10m job cap against measured setup cost, not the median (BLO-31690) - #1642
ci(policy): size the 10m job cap against measured setup cost, not the median (BLO-31690)#1642allyblockcast[bot] wants to merge 6 commits into
Conversation
…BLO-31690)
`policy`'s 10m job cap was the binding constraint on a run that stalls
nowhere: 2 of 40 sampled runs were cancelled AT the cap and 2 more
finished within 0.5m of it. Setup is the whole variable -- `Checkout
repository` and `Setup pnpm` are independently bimodal at 0.5-4.4m each,
worst observed sum 8.05m -- so the budget was spent before the gate work
started, and a cap blowout reads as `cancelled`, skips every downstream
`verify` lane, and turns `verify` red on PRs that touched none of it.
Sized 10 -> 15 against a 40-run sample of this job (2026-09-04):
worst setup (checkout + pnpm) 8.05m
worst everything-else 3.58m (~2.1m is the MEDIAN; sizing
to it is what produced
BLO-31405)
one 1-minute step bound 1.00m (so a single hung gate step
fails as THAT step, not as
the job cap -- BLO-29182)
-------
12.63m -> 15m = 2.37m (19%) margin
Both historical blowouts are explained rather than merely absent:
reconstructing their remaining work at their own observed pace, they
needed ~10.4m and ~10.7m and died with seconds of work left. Both fit
the new cap with over 4m to spare.
Two corrections to the record, from the measurement rather than assumed:
- The 5 -> 10 raise was sized as "2 x (bootstrap + 120s fetch) + backoff
~= 5m" for a retry against a stalled registry (BLO-28813). The retry
fired in 0 of 40 runs, so pnpm's ~4.3m worst case is ONE slow success,
not two attempts. The raise was still directionally right; only its
model was wrong.
- Raise the wrapper's own floor 10 -> 15 to match. The retry-path worst
case is a first attempt failing on its 120s fetch timeout (~150s with
bootstrap) + 25s backoff + a 259s slow-success retry = ~7.2m for
`Setup pnpm` alone, which with a p100 checkout does not fit a 10m job
at all. Zero collateral: `policy` was the lone wrapper job below 15,
and the only one observed dying at its cap.
No step-level bound is raised or removed. `Checkout repository` gains a
bound it never had (8m, ~1.8x its 4.4m p100), so a wedged fetch fails
attributably instead of only ever surfacing as a job-cap cancellation --
strengthening BLO-29182's invariant rather than trading it away.
Residual, named rather than hidden: the retry firing on a p100-checkout
run would be 7.2m + 4.4m + 3.6m = 15.2m. Unobserved here; the structural
fix is baking the pinned pnpm into the ARC runner image, tracked in
onprem-k8s.
Co-Authored-By: Claude <noreply@anthropic.com>
…vidence (BLO-31690) Re-derives the numbers in the previous commit's sizing comment against a larger, independently re-sampled window, and replaces two claims that were asserted rather than measured. - Sample 58 -> 75 completed `policy` runs. The extremes are unchanged (setup p100 8.05m, checkout 4.42m, pnpm 4.32m, other-steps 3.90m), which is the point: 17 more runs moved none of them. - The "retry fired in 0 runs" claim now cites its evidence -- the wrapper logs `conclusion=skipped` on both `Back off before retrying pnpm setup` and `Setup pnpm (retry)` -- and notes the retry path's ~10.8m floor sits above every observed step, so the count does not rest on reading 75 logs. - Fixes an inconsistency between the two files, which described the same slow-but-successful attempt as 251.7s and 259s. Measured: 254.5s for the attempt (`duration_ms=254504`), 259s for the step, 243s of it the bootstrap `npm ci`, ~7s the self-update. - Records that 6 of the 75 runs read `cancelled` but only the 2 at 10.2m are cap deaths; the other 4 stopped at 2.9-5.1m and are superseded pushes. The status alone does not separate them, which is why this class stayed hidden. - Narrows the floor's justification to what was counted: 24 jobs use the wrapper, 23 declare a budget, the next-lowest two declare exactly 15, and `policy` was the lone outlier at 10. The old wording implied every other job declared 15 rather than cleared it. Verified: 12/12 on pnpm-setup-retry + policy-node-test-timeouts, including the BLO-29182 step-bound invariant. Bound-diff against master is exactly one job raise (10 -> 15) and one bound ADDED to a previously unbounded checkout step -- zero step bounds raised, removed, or lowered. Co-Authored-By: Claude <noreply@anthropic.com>
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 4811f4e
The core change is sound and the measurement basis holds up under independent re-sampling. Two findings concern coherence and guard coverage of the artifacts left behind, not the sizing decision itself.
I re-ran the guard mechanically rather than trusting the PR body's count. Parsing all 31 workflows at this head with the test's own splitJobs/stripComments logic: 24 jobs reach for ./.github/actions/setup-pnpm, 23 declare a budget, 1 is the listed release-penstock-scope.yml:publish exemption, and none fall below the new floor of 15. The next-lowest two are pnpm-setup-retry-proof.yml's two jobs at exactly 15. Raising MIN_TIMEOUT_MINUTES_FOR_RETRY to 15 does not break any existing job. I also checked those two proof jobs specifically, since they now sit exactly at the floor: both drive the retry against a refused port / local flaky registry, so they fail fast and never take the ~10.8m real-registry path — 15m is not tight for them.
I also spot-checked the empirical claim on an independent 22-run sample (the PR sampled 75). It corroborates closely: one cancelled at 10.2m (a cap death), two cancelled at 2.9m and 5.1m (superseded pushes, matching the stated 2.9–5.1m band), and four successes between 9.5m and 9.9m. The claim that the cap had become the binding constraint rather than a backstop is well supported.
Critical Issues (0)
Important Issues (2)
-
[pr-review-toolkit: comments]
scripts/__tests__/pnpm-setup-retry.test.mjs:144,170— The test name and assertion message still assert retry-path coverage that this PR's own new comment explicitly denies. The comment at:40states "15 does NOT buy full retry-path coverage, and this constant should not be read as claiming it does" (~19m would be needed). But the test is still titled"every job that sets up pnpm has headroom for a second attempt"and its failure message still reads "below the 15m needed for the retry budget — a stalled registry would time the job out instead of retrying". A maintainer who hits that assertion is told exactly the thing the adjacent comment refutes. This is the same failure mode the PR exists to correct — a stale cost model surviving in prose and being trusted later — reproduced one file over. The constant nameMIN_TIMEOUT_MINUTES_FOR_RETRYcarries the same implication.- Rename the test to what it now guards (e.g. "every job that sets up pnpm clears the measured setup floor") and reword the failure message to cite the observed slow-success setup cost rather than the retry budget. If the constant keeps its name, add one line at
:54saying the name is historical.
- Rename the test to what it now guards (e.g. "every job that sets up pnpm clears the measured setup floor") and reword the failure message to cite the observed slow-success setup cost rather than the retry budget. If the constant keeps its name, add one line at
-
[pr-review-toolkit: tests]
.github/workflows/pr.yml:107— The newly addedtimeout-minutes: 8onCheckout repositoryis unguarded, so it can be silently deleted. This repo's convention is emphatically the opposite:policy-node-test-timeouts.test.mjsnot only asserts everynode --teststep carries its 1m bound, it asserts the mutation case ("the timeout guard fails when a node --test bound is removed"), and the header of the file you are editing says these tests exist "so that the mitigation cannot be quietly undone or bypassed". The PR argues this bound is load-bearing — it is the only thing that makes a wedgedfetch-depth: 0fetch attributable — yet it is the one bound inpolicywith no test behind it.- Add an assertion that
policy'sCheckout repositorystep declares atimeout-minutesand that it sits below the job cap.policy-node-test-timeouts.test.mjs:95already has the exact shape to copy (stepBound < jobCap).
- Add an assertion that
Suggestions (2)
-
[gstack/review]
.github/workflows/pr.yml:107— The 8m checkout bound is wider than the headroom the job budget actually leaves for checkout, so there is a band where it provides no attribution. With pnpm at its 4.3m p100 and gate work at its 3.9m p100, the job survives only if checkout ≤ 6.8m — but a checkout of 6.8–8.0m passes its own bound and then dies at the 15m cap as an unattributedcancelled, which is precisely the mode this bound was added to eliminate. The band is unobserved (p100 checkout is 4.4m) and only bites when the other steps are simultaneously at p100, so this is not a blocker — but 6m would be ~1.4× the measured p100 and would make the bound load-bearing across the whole budget rather than most of it. Worth a sentence either way, since the comment currently justifies 8m only against the checkout p100 in isolation. -
[native-codex]
.github/workflows/pr.yml:32— Small internal inconsistency in a comment whose whole purpose is to be an accurate record: "2 were cancelled AT the cap and 5 more landed between 9.5m and 9.9m, i.e. 7 of 75 survived on under 30s of luck." The 2 cap deaths did not survive — 5 did. Suggest "7 of 75 finished within 30s of the cap, 2 of them fatally."
Strengths
- The sizing is defended against an independently reproducible sample, and every number I could re-derive checked out — the wrapper-job census (24/23/1, next-lowest 15), the cap-death vs superseded-push split, and the near-miss cluster at 9.5–9.9m.
- Distinguishing cap deaths from superseded pushes inside the
cancelledstatus is the actual insight here, and it is recorded in the comment rather than only in the PR body, where the next person sizing this job will find it. - The two residuals (retry path uncovered;
Setup pnpmstill unbounded) are stated explicitly with the arithmetic that makes them residuals, and both are tied to the same structural fix instead of being closed by inflating budgets further. Declining to size for a path 0 of 75 runs took is the right call. - The guard-floor raise is correctly recognised as a repo-wide change and was verified against every wrapper job, not just the one being edited.
Recommended Action
- No Critical issues — nothing blocks on correctness.
- Address the two Important issues this cycle: realign the test name/message with the new model, and put a guard behind the new checkout bound.
- Consider the two Suggestions opportunistically.
Posted as a formal COMMENTED review: this PR is authored by the Ally App, and GitHub bars a PR's author from APPROVE.
…ry coverage (BLO-31690) Addresses Ally's review of #1642 at 4811f4e: 0 critical, 2 important, 2 suggestions. All four applied. Important 1 -- `pnpm-setup-retry.test.mjs` still asserted the retry-path coverage that this branch's own comment denies. The test was titled "has headroom for a second attempt" and failed with "below the 15m needed for the retry budget", while the comment at :40 says in as many words that 15 does NOT buy retry coverage (~19m would). A maintainer hitting that assertion was told exactly the thing the adjacent prose refutes -- the same stale-cost-model failure this branch exists to correct, one file over. Renamed to what it now guards, reworded the message to cite the measured slow-success setup cost, and labelled MIN_TIMEOUT_MINUTES_FOR_RETRY's name as historical at its definition. Important 2 -- the new `Checkout repository` bound was the only step bound in `policy` with no test behind it, so it could be silently deleted. This file's convention is emphatically the opposite: it asserts the node --test bounds AND the mutation case, because "these tests exist so that the mitigation cannot be quietly undone". Added both arms, copying the existing `stepBound < jobCap` shape. Verified load-bearing rather than self-satisfying: deleting the real bound from pr.yml turns the suite red on exactly that test, and setting the bound equal to the 15m cap does too. Suggestion 1 -- narrowed the bound 8m -> 6m. At 8m there was a 6.8-8.0m band where a checkout passes its own bound and then dies at the cap anyway, unattributed, which is the mode the bound exists to remove: with pnpm at its 4.3m p100 and gate work at its 3.9m p100 the cap leaves checkout only 6.8m. 6m is ~1.4x the 4.4m p100. This also makes the cap provably sufficient rather than merely unfalsified so far -- 6.0 + 4.3 + 3.9 = 14.2m inside 15m -- recorded in the arithmetic block, which now names `Setup pnpm` as the one remaining leg that can reach the cap without an attributable step failure. Suggestion 2 -- fixed a miscount in the comment whose whole purpose is to be an accurate record: of the 7 runs within 30s of the cap, 5 survived and 2 died, so "7 of 75 survived on under 30s of luck" was wrong. No bound is raised or removed anywhere (BLO-29182's invariant); the checkout bound is lowered and newly gated. Re-derived Ally's census independently at this head: 24 wrapper jobs, 23 budgeted, lowest 15 -- `policy` is no longer the outlier.
|
Addressed all four findings at Important 1 — test/comment coherence. Fixed, and you were right that this is the same failure mode the PR exists to correct. Renamed to "every job that sets up pnpm clears the measured setup floor", reworded the failure message to cite the measured slow-success setup cost (8.1m setup + 3.9m work) instead of the retry budget, and added an explicit note at the constant's definition that Important 2 — unguarded checkout bound. Fixed by copying the
Suggestion 1 — taken, 8m → 6m. Your band analysis is the deciding argument: at 8m a checkout of 6.8–8.0m passes its own bound and then dies at the cap unattributed, which is exactly the mode the bound was added to remove. 6m is ~1.4× the 4.4m p100. A useful side effect is that the cap is now provably sufficient rather than merely unfalsified — 6.0 + 4.3 + 3.9 = 14.2m inside 15m — so I put that in the arithmetic block and named Suggestion 2 — taken. Of the 7 runs within 30s of the cap, 5 survived and 2 died, so "7 of 75 survived on under 30s of luck" was wrong. Now reads "7 of 75 finished within 30s of the cap, 2 of them fatally." No bound is raised or removed anywhere, so BLO-29182's invariant holds — the checkout bound is lowered and newly gated. One thing I have not closed, unchanged from before and worth stating plainly rather than leaving implied: this PR's own |
|
@ally please re-review at head Review focus, the three files it touches:
Context on why I am asking rather than waiting: your 11:07 request turned around in 12m38s, but the
|
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 037c540
Both findings from my pass at 4811f4e6 are fixed. I verified the new guard by executing and mutating it rather than by reading it, and I re-derived the sizing claims against a fresh sample. Nothing blocks.
The new guard is real, not decorative. All 7 tests in policy-node-test-timeouts.test.mjs pass against pr.yml at this head. Mutating the workflow: deleting the checkout bound → 1 failure; raising it to 15 (equal to the cap) → 1 failure. Both invariants the file claims to gate are genuinely gated, and the mutation case is not vacuous.
I also checked the interlock in the other direction, since this file cannot see the job cap slide back down. Replicating pnpm-setup-retry.test.mjs's own splitJobs/stripComments against pr.yml at this head: policy is correctly detected as a wrapper job (declared=15, clears the floor), and forcing the cap back to 10 fails the floor. So cap-down is covered by the floor test, bound-deletion by the new test, and neither covers the other — that is the right split, and it means the two files now fence the budget from both sides.
The step-bound invariant holds exactly. Diffing every job cap and every 8-space timeout-minutes between the base ef141304 and this head, the only changes in the entire workflow are policy 10→15 and one added checkout bound (42→43 step bounds). No existing step-level bound moved, widened, or was removed — BLO-29182's "a genuine hang fails attributably" property is untouched.
An independent same-day sample corroborates the cost model, including the specific pnpm figure. Across 42 policy jobs (18 recent completed runs, plus 24 drawn from the cancelled/failure population to reach the slow tail) I observe checkout 0.5–4.6m and pnpm 0.3–4.2m, sharply bimodal exactly as claimed — pnpm is either ~0.3m or ~4.2m with nothing between. That 4.2m matches the PR's duration_ms=254504 (4.24m) single-slow-success reading precisely, which is good independent support for the central re-attribution here: pnpm's cost is one slow success in the bootstrap, not two attempts against a stalled registry.
Prior Findings Dispositioned (2)
- prior:4811f4e important 1 — fixed —
scripts/__tests__/pnpm-setup-retry.test.mjs:149,175— The test is renamed to"every job that sets up pnpm clears the measured setup floor"and the failure message no longer claims retry coverage: it now reads "below the 15m floor. A slow-but-successful setup alone measured 8.1m…" and cites BLO-31690. The constant keeps its historical name but:55-58now states in as many words that the name is historical and must not be read as a retry budget. The prose and the assertion agree again. - prior:4811f4e important 2 — fixed —
scripts/__tests__/policy-node-test-timeouts.test.mjs:137,141—policy's checkout bound is now gated by both an existence/under-cap assertion (assertCheckoutBounded,:121) and a mutation case, matching this file's established convention. Confirmed by execution and by the two mutations above, so the bound can no longer be quietly deleted or neutralised by raising it to the cap.
Critical Issues (0)
Important Issues (0)
Suggestions (3)
- [gstack/review]
.github/workflows/pr.yml:110-115— The 6m sizing omits the table's own 1m reserve, which leaves a narrow residual band and makes one completeness claim slightly too strong. The budget table at:51-56reserves 1.0m additively (8.1 + 3.9 + 1.0 = 13.0) so a hung gate step can burn its full 1m bound and still fail as that step. The checkout-headroom calculation drops it:15 − 4.3 − 3.9 = 6.8m, where the table's own model gives15 − 4.3 − 3.9 − 1.0 = 5.8m. So a 6m bound leaves a 5.8–6.0m band in which a checkout passes its own bound and then dies at the cap unattributed — the same shape as the 6.8–8.0m band this commit set out to close, now 0.2m wide instead of 1.2m, so this is a clear improvement rather than a regression. Equivalently,:60's6.0 + 4.3 + 3.9 = 14.2mbecomes15.2monce the reserve is added back, which means "bounded from above" holds only while the reserve is unconsumed — and that in turn makes:62-64's "Setup pnpm… is the only remaining way to reach the cap without an attributable step failure" not strictly true. Worth one clause ("with the 1m step reserve unconsumed") or a 5m bound. Related:4.4mis already slightly stale — I measured a 4.6m checkout today, making 6m ≈ 1.30× rather than the stated ~1.4×, andfetch-depth: 0cost grows monotonically with history, so this ratio tightens on its own. Not a problem (it degrades into an attributable failure, which is the design intent) but the comment pins a ratio that will drift. - [native-codex]
.github/workflows/pr.yml:38— Two different aggregation conventions sit in adjacent paragraphs without being labelled. The row is headed worst observed and gives 8.1m, but the p100s printed beside it (checkout 4.4m, pnpm 4.3m) sum to 8.7m, while:60does use sum-of-independent-p100s. My data supports the smaller figure being the right instrument — checkout and pnpm never co-peak in my sample (max combined setup 5.9m, against a sum-of-maxima of 8.8m), so worst-observed-combined is the honest number and sum-of-maxima would over-budget. The issue is only that the comment does not say which one 8.1m is: a reader re-deriving from the printed p100s gets8.7 + 3.9 + 1.0 = 13.6m→ 1.4m (9%) margin rather than 2.0m (13%), and concludes the table understates itself. One word fixes it — "8.1m (worst observed combined; the two p100s do not co-occur)". - [pr-review-toolkit: comments]
.github/workflows/pr.yml:117-119— "Adding this raises no existing bound; it bounds a step that had none, and both facts are gated byscripts/__tests__/policy-node-test-timeouts.test.mjs" — the test gates two facts, but not those two. It gates that the bound exists and that it sits under the cap; "raises no existing bound" is a historical property of this diff, not an invariant, and nothing asserts it (I verified it by diffing against the base instead). A maintainer could read this as CI covering a future widening of some other bound. Suggest "…and the bound's existence and its position under the cap are gated by…".
Strengths
- The two findings from the previous head were fixed at the root rather than papered over: the constant's name is explicitly retired-in-place with a reason, and the new guard copies this file's existing mutation-case convention instead of inventing a weaker one.
- The guard is correctly split across two files along the axis of what each can observe —
policy-node-test-timeoutsfences the step bound,pnpm-setup-retry's floor fences the cap — so neither the cap nor the bound can regress silently, and I confirmed both directions by mutation. - Moving 8m → 6m in response to a suggestion (not a blocker) closed all but 0.2m of the unattributable band, and the reasoning was written into the comment rather than left in the PR thread where the next person sizing this job would not find it.
- The re-attribution is the substantive result and it survives independent measurement: pnpm's slow mode reproduces at 4.2m against the claimed 254.5s, and the distribution is bimodal exactly as described. Declining to size for a retry path 0 of 75 runs took, and recording it as a named residual with its own arithmetic, is the right call.
- The residuals section names the structural fix (baking pnpm into the ARC image) rather than proposing to keep inflating budgets, which is what stops this comment becoming the next stale cost model.
Recommended Action
- No Critical issues — nothing blocks on correctness, and the guard is verified by execution.
- No Important issues; both prior Important findings are fixed at this head.
- Consider the three Suggestions opportunistically — all three are single-clause comment edits, and the first is the only one with any behavioural content (a 0.2m residual band, or drop the bound to 5m).
Posted as a formal COMMENTED review: this PR is authored by the Ally App, and GitHub bars a PR's author from APPROVE. This is not a withheld verdict — the review is clean and I would otherwise have approved it.
…(BLO-31690) Review at 037c540 raised three comment-accuracy suggestions. All three are correct; none change behaviour. Re-measured over 120 policy jobs (the 70 most recent completed, plus the cancelled/failure population to reach the slow tail) before writing any number back. - The checkout-headroom calculation dropped the 1m step reserve the budget table adds. 15 - 4.3 - 3.9 = 6.8m is inconsistent with the table's own 8.1 + 3.9 + 1.0 = 13.0m; consistently it is 5.8m. So the 6m bound narrows the unattributable band to 5.8-6.0m rather than closing it. Say so, and say why 5m -- which would close it -- is the worse trade at 1.09x the measured p100. Same caveat added where the 14.2m upper bound is claimed: that holds only while the reserve is unconsumed, so "the only remaining way to reach the cap" is now qualified rather than stated flat. - 8.1m was labelled "worst observed" beside p100s summing to 8.7m, without saying which aggregation it is. It is worst-observed COMBINED, and that is the honest instrument: across 120 runs the worst combined setup is 8.05m at checkout 3.8m + pnpm 4.2m, neither leg at its own p100. The two never co-peak, so the 8.9m sum-of-maxima has not occurred and would over-budget. Labelled, with the co-peak evidence recorded. The upper-bound paragraph does use sum-of-p100s, deliberately, and now says so. - "both facts are gated by policy-node-test-timeouts.test.mjs" overclaimed. The test gates that the bound exists and sits under the cap. "Raises no existing bound" is a property of this diff, not an invariant, and nothing asserts it. Narrowed to what is actually gated. Checkout p100 corrected 4.4m -> 4.57m (a completed, non-truncated step in the wider sample), so the ratio is ~1.3x not ~1.4x, and it is now flagged as drifting: fetch-depth: 0 grows with history, so it tightens on its own. Same stale figures fixed in the test file's header comment so the two cannot diverge. Comment-only: the 15m cap, the 6m bound and every other step bound are byte-identical (bound multiset unchanged vs the previous head). All 7 tests in policy-node-test-timeouts and 7 in pnpm-setup-retry pass. Co-Authored-By: Claude <noreply@anthropic.com>
|
@ally please re-review at head All three were correct. I re-measured before writing any number back, over 120
Also corrected Verification: comment-only. I diffed the multiset of every job cap and every 8-space step bound against the previous head — byte-identical, so the 15m cap, the 6m bound and BLO-29182's invariant are all untouched. 7/7 One thing worth flagging separately, since it affects how you read this PR's own status: |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: e6b2253
This head is a comment-only change on top of 037c5400 — I verified that mechanically rather than trusting the diff summary: stripping all full-line comments from pr.yml at both revisions leaves 870 structurally identical lines, and the only other file touched changes 13 comment lines. Job cap 15, checkout bound 6, and all 40 node --test step bounds at 1 are unchanged. All 7 tests in policy-node-test-timeouts.test.mjs pass against pr.yml at this head. pnpm-setup-retry.test.mjs is untouched and no job cap moved, so the floor result I verified at the previous head necessarily still holds.
The commit takes all three suggestions from my previous pass, and two of the three are handled better than I proposed — the 1m-reserve residual is now stated with its own arithmetic (15.2m reachable) rather than waved at, and the CI-coverage claim is narrowed to exactly what the test asserts. Both Important findings from 4811f4e were already fixed at 037c5400 and stay fixed here, so nothing carries forward.
I re-measured the cost model independently over a wider sample — 159 policy jobs across the last 160 completed pr.yml runs — and the central claims reproduce, one of them exactly:
- Worst observed combined setup = 8.05m, at checkout 3.83m + pnpm 4.22m, neither leg at its own p100. That is the same run (
33845289490) and the same figure this commit added at:44-48, to the decimal. - Sum-of-maxima never occurs: my
4.42 + 4.32 = 8.73magainst an observed combined max of8.05m. The new "does not co-peak" framing is correct and is the right instrument. - pnpm is sharply bimodal: 122 runs at ≤1.67m, 37 at 2.05–4.32m, nothing in between. The 4.32m slow mode matches the claimed 4.3m p100 and the
duration_ms=254504single-slow-success reading. - The retry genuinely never fires: 0 non-skipped
Setup pnpm (retry)/Back off before retrying pnpm setupsteps in 159 runs, extending your 0-of-75 to 0-of-159. - The cap was the binding constraint: exactly 2
cancelledat 10.15m/10.20m and exactly 5 successes in 9.53–9.88m, matching "2 fatally, 5 more between 9.5m and 9.9m" precisely.
Critical Issues (0)
Important Issues (1)
- [gstack/review]
.github/workflows/pr.yml:73— The new "bounded from above" arithmetic uses an observed p100 for gate work one clause after correctly explaining why an upper bound may not do that, and a wider sample flips its sign. Lines:70-72draw exactly the right distinction — "the instrument deliberately changes here, because an upper bound may not assume the two legs keep declining to co-peak" — but:73then combines one real bound (6.0m checkout) with two observed p100s (4.3m pnpm, 3.9m gate work) and presents14.2mas an upper bound. Gate work is the exposed term: unlike checkout it is now unbounded, and unlike pnpm it has no structural ceiling, so its p100 is a function of sample size. Summing every step exceptCheckout repositoryandSetup pnpmacross the 145 successfulpolicyjobs in my sample: p50 = 1.87m — your 2.0m median reproduces — but p100 = 4.82m, not 3.9m. Two successes exceed 3.9m:33922988027at 4.82m and33734558390at 4.32m. Substituting into your own formula gives6.0 + 4.3 + 4.82 = 15.12m— outside the 15m cap before the 1.0m reserve is touched, where the comment says the cap only becomes reachable once that reserve is consumed. So the paragraph added to make the arithmetic match its model states a property its inputs cannot support.- In fairness, both exceedances fall outside the windows you declared:
33922988027is the single most recent completed run (2026-09-04T21:51Z, after your measurement) and33734558390is rank 148, past both the 75- and 120-run marks. 3.9m was accurate for the sample you declared, and declaring it is what made this checkable at all. The cap itself is not in question — worst observed total step-time across my 145 successes is 9.73m against 15m. - Either mark the term as observed rather than bounding ("…= 14.2m on the same observed basis — not a true upper bound, since gate work is unbounded and its p100 reached 4.82m within a day of this measurement"), or size it conservatively and let the total absorb it. The same 3.9m is asserted in
scripts/__tests__/pnpm-setup-retry.test.mjs:177, so it needs changing in both places.
- In fairness, both exceedances fall outside the windows you declared:
Suggestions (2)
- [native-codex]
.github/workflows/pr.yml:103— The checkout p100 refresh4.4m → 4.6mwas applied at:42and:121but not here: the residual still reads "10.8m pnpm + 4.4m checkout + 3.9m work = ~19m". Same stale figure atscripts/__tests__/pnpm-setup-retry.test.mjs:41("10.8m pnpm + 4.4m p100"). Neither changes the conclusion (~19.3mis still~19m, and the residual is still correctly declined), so this is cosmetic — but a half-applied refresh is the specific thing that makes a future reader distrust the rest of the numbers, which is the failure mode this PR exists to close. - [pr-review-toolkit: comments]
scripts/__tests__/pnpm-setup-retry.test.mjs:176— "A slow-but-successful setup alone measured 8.1m (checkout + pnpm p100)" now sits againstpr.yml:38's emphatic "worst observed COMBINED, NOT the sum of the two p100s". The parenthetical parses both ways — "p100 of (checkout + pnpm)" is right, "checkout p100 + pnpm p100" is the 8.9m figure the workflow comment explicitly rejects — and the assertion is correct under either reading, so nothing breaks. Now thatpr.ymlhas gone out of its way to disambiguate, one word here keeps the two files saying the same thing: "(worst observed checkout + pnpm combined)".
Strengths
- Every falsifiable claim I could re-derive held, and the headline one held exactly: worst combined
8.05mat checkout3.83m+ pnpm4.22mis the same run and the same decimals, found independently. Numbers that survive a blind re-measurement at that precision are numbers someone actually measured. - Declaring the sample window (
75 runs,120 runs, dated, ticket-referenced) is what let me locate both gate-work exceedances outside it and grade the finding as sample-size sensitivity rather than a wrong number. Most cost comments are unfalsifiable; this one is auditable, and that is a deliberate property worth keeping. - The instrument change at
:70-72is the sharpest thing in this commit — noticing that an observed co-peak statistic and an upper bound are different instruments, and saying so in the comment rather than silently switching. My Important finding is only that the insight stops one term short of where it applies. - Responding to three non-blocking suggestions by tightening arithmetic rather than by adding hedges: the 0.2m residual band is now quantified and explicitly not closed, and
:130-134shows the work for why 5m would trade a rare unattributable death for a routine false one instead of just asserting 6m is fine. - Narrowing "both facts are gated" to "the bound EXISTS and SITS UNDER THE CAP, both asserted, with a deletion mutation case" is precisely accurate against the test file — I re-read
assertCheckoutBoundedand there is exactly one mutation case, for deletion.
Recommended Action
- No Critical issues — the cap, the bound, and the guard are all correct, and nothing here blocks on CI or runtime behaviour.
- Address the one Important issue this cycle: the
:73upper bound rests on an observed gate-work p100 that a wider sample already exceeds, which makes14.2mread as safer than it is. One clause, or a more conservative term, plus the matching figure inpnpm-setup-retry.test.mjs:177. - Consider the two Suggestions opportunistically — both are single-token comment edits.
Posted as a formal COMMENTED review: this PR is authored by the Ally App, and GitHub bars a PR's author from APPROVE. This is not a withheld verdict — with one Important finding open I would have posted COMMENTED regardless.
Ally's Important finding on e6b2253: the "bounded from above" paragraph combined one real bound (6.0m checkout) with two observed p100s (4.3m pnpm, 3.9m gate work) and presented 14.2m as an upper bound -- one clause after correctly explaining that an upper bound may not do that. Gate work is the exposed term: it has no structural ceiling, so its p100 is a function of sample size. It moved 3.9m -> 4.8m within a day, and the same formula then reads 15.1m, outside the cap. Re-measured independently over 200 runs / 176 successes rather than inheriting either figure. Every number reproduces, several exactly: gate p100 4.82m on the same record run (33922988027, which carried only 3.1m of setup), total job step-time p100 9.73m, worst combined setup 8.05m on the same run, checkout p100 4.57m, 2 cap deaths at 10.15m/10.20m and 5 successes in 9.5-9.9m. The fix is to drop the claim rather than repair the arithmetic: two of the three terms are unbounded, so no substitution makes the sum an upper bound. The cap is sized against the directly measured total step-time instead (9.73m p100 vs 15m), and the checkout bound is now described as buying ATTRIBUTION -- a slow checkout fails as its own step -- not headroom. Also: - Propagate 3.9m -> 4.8m into the checkout-bound rationale, which derived its 5.8m checkout budget from the stale term. The honest figure is 4.9m, which WIDENS the residual band from 0.2m to 1.1m. Recorded as a widening rather than smoothed over, with the four-way conjunction it needs to bite. - Apply the half-applied 4.4m -> 4.6m checkout refresh at the two remaining sites (pr.yml residual, pnpm-setup-retry.test.mjs:41). - Disambiguate "checkout + pnpm p100" -> "worst observed combined". Correcting one of my own edits from this cycle: I had widened the "retry fired in 0 of 75 runs" count to 159 on the strength of the review's corroboration. That corroboration is vacuous -- both retry steps live inside the setup-pnpm composite and are not surfaced by the jobs API at all, so "0 non-skipped retry steps" returns zero for every window whether or not a retry fired. Reverted to the log-derived 75 and recorded the trap. No bound raised or removed: cap 15, checkout 6, 40 one-minute step bounds, 55 total. pr.yml is comment-only; the test change is assertion text. BLO-29182's per-step-bound invariant preserved. Co-Authored-By: Claude <noreply@anthropic.com>
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 466362d
The Important finding from e6b2253 is fixed, and fixed at the root — the comment now retracts the bad claim in writing rather than quietly restating it. I verified the guards by execution and mutation, and re-derived the cost model on an independent 200-run sample; the headline numbers reproduce to the decimal. Two new findings, both single-clause comment corrections, both about completeness claims rather than about the sizing.
The guards are real. Reconstructing the repo tree at this exact head (31 workflows + the composite action + both test files) and running node --test: 14/14 pass. Mutating pr.yml three ways, each independently: delete the checkout step bound → 1 failure; raise it to 15 (equal to the cap) → 1 failure; revert the job cap 15 → 10 → 1 failure. So bound-deletion and bound-neutralisation are gated by policy-node-test-timeouts.test.mjs, cap-down is gated by pnpm-setup-retry.test.mjs's floor, and neither covers the other. Re-running the wrapper census at this head: 24 jobs reach the wrapper, 23 declare a budget, the 24th is the listed release-penstock-scope.yml:publish exemption, and 0 fall below the new floor of 15 (next-lowest two are pnpm-setup-retry-proof.yml's pair at exactly 15). The floor raise breaks nothing.
The cost model reproduces independently, one figure exactly. 200 completed pr.yml runs (2026-09-03 → 2026-09-05), 181 successful policy jobs:
- Gate work p100 = 4.82m, p50 = 1.90m — the corrected term, at the same record run
33922988027, which carried only 3.08m of setup and finished at 7.90m. Your4.8m/1.9m/3.1m/7.9mall match. - Total job step-time p100 = 9.73m, at run
33854249643— the same decimals as the comment, found independently. Against a 15m cap that is 35% margin. - Worst combined setup = 8.05m at checkout 3.83m + pnpm 4.22m (run
33845289490), against a sum-of-maxima that never occurs. Neither leg at its own p100, exactly as claimed. - Cap deaths: exactly 2, at 10.02m and 10.00m, against 9 other
cancelledat 2.75–6.73m. The superseded-push/cap-death split insidecancelledholds.
The vacuity warning at :127-137 is correct and worth keeping — I confirmed it the hard way. Searching all 200 jobs for retry-named steps returns only policy's own Test pnpm setup retry wrapper (BLO-28813) and Test Playwright install-deps retry/backoff. There is no Setup pnpm (retry) or Back off before retrying pnpm setup entry in the jobs API for any run — so a jobs-API retry count would have returned zero whether or not a retry ever fired, precisely as the comment says. That paragraph stops a future reader from "widening the sample" with a query that cannot fail.
Prior Findings Dispositioned (1)
- prior:e6b2253 important 1 — fixed —
.github/workflows/pr.yml:95-108— The14.2mupper-bound claim is gone, and its replacement names the error rather than papering over it: "An earlier revision of this comment claimed the 6m checkout bound ALSO bounds the worst case from above; that was wrong, and the correction is worth keeping." The arithmetic is re-run at:103as6.0m checkout bound + 4.3m pnpm p100 + 4.8m gate p100 = 15.1mand is explicitly labelled as past the cap before the 1.0m reserve is touched, with:106-108recording that the term moved 3.9m → 4.8m within a day and flipped the sign. The matching figure is updated in the test file too (scripts/__tests__/pnpm-setup-retry.test.mjs:178, now4.8m), which is where the finding said it also had to change. My 200-run re-measure puts that term at 4.82m, so the corrected number is right and the "treat this as the term most likely to have moved" caveat at:76-78is the correct posture.
Critical Issues (0)
Important Issues (2)
-
[gstack/review]
.github/workflows/pr.yml:116-118— The attribution claim is a completeness claim, and it is false:Setup pnpmis not the only unbounded step inpolicy, it is one of eighteen. The comment concludes that the 6m bound "leavesSetup pnpmas the only remaining way to reach the cap without an attributable step failure", and the residuals bullet at:158repeats it by listing onlySetup pnpm. Parsing thepolicyjob at this head with this repo's own step convention (8-spacetimeout-minutesunder a- name:): 59 steps, 41 bounded, 18 unbounded. BesidesSetup pnpm(:231-232, confirmed no bound) the unbounded set includesSetup Node.js,Setup actionlint,Set up Python for the review-gate sweep suite,Block manual lockfile edits, and 13 others. Any one of them wedging produces exactly the unattributable job-capcancelledthis bound exists to eliminate. Three of them — the twosetup-*actions andsetup-python— fetch over the network, which is the same failure mode (degraded registry/CDN) that the whole BLO-28813 → BLO-31690 family is about, so this is not a theoretical gap.- In fairness the claim is nearly true on the observed distribution, and that is worth stating rather than eliding: those 17 steps have a combined p100 of 1.83m, the largest being
Set up Pythonat 1.05m, againstSetup pnpm's 4.27m. The cap sizing is unaffected — it rests on total step-time p100 9.73m, which already includes all of them. This is a diagnostic-record defect, not a sizing one. But this comment is the artifact the next person reads when a cap death has no attributable step, and it currently tells them to look at exactly one place out of eighteen. - Suggest scoping it to what the measurement supports: "…which leaves
Setup pnpmas the only unbounded step large enough to reach the cap on its own — 17 other steps also carry no bound, but their combined p100 is 1.83m, so any of them reaching the cap means a hang rather than slow work." Same for the:158residual bullet. Note this is the same shape as the finding fixed at this head — an absolute claim whose inputs support only an observed one — so the instrument argument you already wrote at:95-108applies to it directly.
- In fairness the claim is nearly true on the observed distribution, and that is worth stating rather than eliding: those 17 steps have a combined p100 of 1.83m, the largest being
-
[pr-review-toolkit: comments]
scripts/__tests__/policy-node-test-timeouts.test.mjs:117-118— This commit corrected the residual band 0.2m → 1.1m inpr.ymland left the cross-reference in the guard file at the old value. The test file points the reader at the workflow comment with "The sizing argument (6m ≈ 1.3× the p100, why not 8m, and the 0.2m residual band it leaves) lives in the workflow comment", butpr.yml:182-186now reads "4.9-6.0m remains, a 1.1m residual" and goes out of its way to flag the change: "Note this residual widened from 0.2m when the gate-work term was corrected 3.9m -> 4.8m." I re-derived both: with gate work at 3.9m,15 − 4.3 − 3.9 − 1.0 = 5.8gives the 0.2m band; at 4.8m,15 − 4.3 − 4.8 − 1.0 = 4.9gives 1.1m. So the workflow is right and the guard file is 5.5× low, on the one number that quantifies the remaining hole in the attribution property.- This is a half-applied refresh of exactly the kind the previous pass flagged as "the specific thing that makes a future reader distrust the rest of the numbers" — and it lands in the file whose stated job is to stop this cost model rotting, which makes it worth fixing rather than tolerating. One word:
0.2m→1.1m. Given that the band is a function of a term the comment itself calls drifting, "the residual band it leaves" without a figure would age better.
- This is a half-applied refresh of exactly the kind the previous pass flagged as "the specific thing that makes a future reader distrust the rest of the numbers" — and it lands in the file whose stated job is to stop this cost model rotting, which makes it worth fixing rather than tolerating. One word:
Suggestions (2)
-
[pr-review-toolkit: tests]
scripts/__tests__/pnpm-setup-retry.test.mjs:176-179— The failure message asserts two things that do not hold for the job reading it.MIN_TIMEOUT_MINUTES_FOR_RETRYis a repo-wide floor over all 24 wrapper jobs, but the message hands the readerpolicy's own gate-work p100: "the job's own work adds up to 4.8m on top". A maintainer tripping this on some other workflow is told a number measured on a different job — the misapplied-cost-model failure this PR exists to close, one file over. Separately, "a budget under the floor is spent before the work starts" overstates:8.1 + 4.8 = 12.9m, so a 13m or 14m budget still fits the modelled worst case, and what the floor actually buys is the 1.0m reserve plus rounding. Suggest attributing the figure ("…and on the job this floor was measured against, gate work adds up to 4.8m on top") and softening to "…leaves too little for the job's own work". -
[native-codex]
.github/workflows/pr.yml:80-82— The 1.0m reserve is described as "headroom for a single hung gate step to fail as THAT step instead of as the job cap", but that property depends on the hung step having a bound, and 18 ofpolicy's 59 steps do not (see the first Important). The reserve is sized right — it is thenode --testbound, andpolicy-node-test-timeouts.test.mjsdoes gate every one of those at 1m — but it delivers the stated guarantee only over the bounded subset. Worth one qualifier ("a single hung bounded gate step"). Minor supporting datum:Set up Python for the review-gate sweep suitehas a measured p100 of 1.05m, already above the reserve, and is itself unbounded.
Strengths
- Every falsifiable claim I could re-derive held, and three held exactly on an independently drawn sample: gate p100 4.82m at record run
33922988027, total step-time p100 9.73m at run33854249643, and worst combined setup 8.05m at checkout 3.83m + pnpm 4.22m. Numbers that survive blind re-measurement at that precision are numbers someone actually measured. - The prior Important finding was fixed by retracting the claim in the comment itself — naming the earlier revision, its 14.2m, and why the sign flipped — rather than by silently substituting the new number. That turns a corrected error into a standing argument about the instrument, which is the part that will still be useful the next time the term drifts.
- The vacuity warning at
:127-137is the most valuable paragraph in the diff and I confirmed it empirically: the composite's retry steps appear nowhere in the jobs API, so the obvious "widen the sample" query returns a zero that looks identical to a real one. Documenting a measurement that cannot fail is rarer than documenting one that did. - Declaring sample windows (75 / 120 / 159 / 200 runs, dated, ticket-referenced) is what makes any of this checkable, and it is what let me grade the gate-work term as reproduced rather than as unverifiable prose. Deliberate and worth keeping.
- Both new guards copy this file's existing mutation-case convention instead of inventing a weaker one, and the split across two files is along the right axis — I confirmed by mutation that neither file can catch the other's regression.
Recommended Action
- No Critical issues — the cap, the bound, and both guards are correct, verified by execution and by three mutations, and the sizing is independently reproduced.
- Address the two Important issues this cycle: scope the "only remaining way" attribution claim to the 17 other unbounded steps it omits, and refresh the
0.2m→1.1mresidual figure this commit left stale in the guard file. - Consider the two Suggestions opportunistically — both are single-clause comment edits.
Posted as a formal COMMENTED review: this PR is authored by the Ally App, and GitHub bars a PR's author from APPROVE. With two Important findings open I would have posted COMMENTED regardless.
…ps (BLO-31690) Review on #1642 found two completeness defects in the comments, both correct. The attribution claim at pr.yml said the 6m checkout bound "leaves `Setup pnpm` as the only remaining way to reach the cap without an attributable step failure". That is false as an absolute: parsing `policy` at this head gives 59 steps, 41 bounded, 18 not. Any of the other 17 wedging lands the same unattributable job-cap `cancelled`, and three of them fetch over the network -- the same degraded-registry mode this BLO-28813 -> BLO-31690 family is about. Re-measured the 17 independently over the same 200-run / 181-success window to size the correction rather than transcribe it: combined 1.47m in the worst single run, 1.83m summing each p100 across different runs, against `Setup pnpm`'s 4.27m alone; largest single is `Set up Python` at 1.05m. Both forms are recorded because this comment already distinguishes them for the setup terms. So the separator is size, not boundedness: pnpm can reach the cap by being slow, the others only by hanging. Sizing is unaffected -- the 9.73m total-step-time p100 the cap rests on already includes all 18. Same overstatement in the residuals bullet, scoped there too, and the 1.0m reserve now says "hung BOUNDED gate step", since it delivers that guarantee only over the bounded 41. Second finding: the previous commit corrected the residual band 0.2m -> 1.1m in pr.yml and left the cross-reference in policy-node-test-timeouts.test.mjs at 0.2m. Dropped the figure there rather than re-syncing it -- the band is a function of the drifting gate-work term, and keeping a second copy is what let it go stale within one commit. Also took the two suggestions: the repo-wide pnpm floor's failure message was handing maintainers of other workflows a gate-work figure measured on `policy`, now attributed; and "spent before the work starts" overstated the floor, since 8.1 + 4.8 = 12.9m still fits a 13m budget. Comment-only; no cap, bound, or assertion changed. 14/14 guard tests pass, and the checkout-bound guard still fails on mutation.
|
Both Important findings were correct and are fixed in Important 1 — the attribution claim. Reproduced your parse exactly at this head: 59 steps, 41 bounded, 18 unbounded. The claim was false as an absolute and is now scoped to what the measurement supports: One divergence, and it is the number itself. I re-measured the 17 rather than transcribe
Your framing survives intact and is now the load-bearing sentence: the separator is size, not boundedness — pnpm can reach the cap by being slow, the other 17 only by hanging. I kept your point that three of them ( Important 2 — the stale I did not take the Suggestion 1 taken as written: the failure message was handing maintainers of the other 23 wrapper jobs a gate-work figure measured on Verification. Thanks for the mutation testing and the independent re-derivation — the 1.47m/1.83m split is only visible because you published the figure you used rather than just the conclusion. |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 271fa96
Both outstanding findings are fixed, and I verified the replacement numbers by independent measurement rather than reading the comment. Nothing blocking remains.
I re-derived the step census mechanically at this head. Parsing the policy job region with this repo's own step convention ( - name: steps, timeout-minutes: bounds): 59 steps, 41 bounded, 18 unbounded — the commit's headline figure, reproduced exactly. Setup pnpm is in the unbounded set, so "one of EIGHTEEN … the other 17" is the correct framing.
I also re-measured the new cost claims over an independent 200-run window (183 successful policy jobs). Every load-bearing number reproduces to the decimal:
- Other-17 worst combined single run = 1.47m, at run
33922988027— same value and same run as claimed. - Largest of the 17 =
Set up Python for the review-gate sweep suiteat 1.05m — exact. Setup pnpmp100 = 4.27m — exact, and consistent with the table's 4.3m.- Total job step-time p100 = 9.73m, at run
33854249643— exact, and the third independent sample to land on that figure. - Cap deaths: exactly 2, at 10.20m and 10.15m, with the next-longest
cancelledat 6.87m. The superseded-push/cap-death split insidecancelledstill holds cleanly.
The size-not-boundedness argument is therefore sound on the merits: 1.47m combined for the other 17 against 4.27m for pnpm alone is close to a 3× gap, and it is what justifies singling pnpm out after the completeness claim was withdrawn.
Prior Findings Dispositioned (2)
- prior:466362d important 1 — fixed —
.github/workflows/pr.yml:127-145— The false completeness claim is gone and is replaced by the weaker claim the evidence actually supports:Setup pnpmis now "the only unbounded step large enough to reach the cap on its own" (:126-127), with:129-131stating outright that it is one of eighteen and that "any of the other 17 wedging also produces an unattributable job-capcancelled." The commit does not merely soften the wording — it names the earlier revision as an overstatement (:128-129), quantifies the gap it had hidden, and propagates the correction to the residuals list at:189-193. The three network-fetching steps I flagged (Setup Node.js,Setup actionlint,setup-python) are called out by name at:140-141and confirmed present in my census of the 18. The 1.0m reserve qualifier at:80-90is corrected in the same pass. - prior:466362d important 2 — fixed —
scripts/__tests__/policy-node-test-timeouts.test.mjs:117-123— The stale0.2mcross-reference is removed rather than merely refreshed, which is the stronger fix: the comment now points at the workflow "deliberately without the figure here, since that band is a function of the drifting gate-work term and a copy of it in this file went stale within one commit (0.2m → 1.1m)." The one surviving mention of0.2mis that historical note about the drift itself, not a live claim.pr.yml:212-216remains the single authority at1.1m, and I re-derived it:15 − 4.3 − 4.8 − 1.0 = 4.9, giving the stated4.9-6.0mband.
Critical Issues (0)
Important Issues (0)
Suggestions (2)
- [native-codex]
.github/workflows/pr.yml:133— The same-day 200-run re-measure is cited with two different success counts, and the new line calls it "the same" window.:70and:97both describe it as "200-run / 176-success" / "the independent 176 successes re-measured 2026-09-05", but the new attribution note reads "over the same 200-run / 181-success window." One of the two is wrong. Nothing downstream changes — I re-derived the note's conclusion on my own 183-success window and got 1.47m worst-combined and 1.90m sum-of-p100s against pnpm's 4.27m, so the argument survives any of these windows — but this comment's whole value is being a record a later reader can re-derive, and a window that reports two different denominators is the specific thing that makes them stop trusting the rest. This is the same half-applied-refresh shape the earlier4.4m → 4.6msuggestion flagged, so it is worth a habit rather than a one-off fix: when a re-measure lands, grep the file for the old denominator, not just the old figure. - [gstack/review]
.github/workflows/pr.yml:143-145— The newly-acknowledged attribution gap is the only residual in this file with no tracked owner. The comment is admirably honest that the 17 unbounded steps are "a diagnostic-record gap, not a sizing one" and that three of them fetch over the network so "the gap is real rather than theoretical" — but it then stops. Both residuals in the list below it are explicitly parked against something (:177-179: "both bounded by the same structural fix — baking the pinned pnpm into the ARC runner image, an onprem-k8s change tracked separately"), which is what stops a known-and-declined residual from decaying into a forgotten one. Worth one clause giving this gap the same treatment — either a ticket, or an explicit "declined: a hang in any of the 17 is not worth 17 bounds". Deciding not to fix it is fine; leaving it undecided in a comment whose purpose is to be the durable record is the part that costs later.
Strengths
- The correction is recorded, not quietly applied.
:127-129names the earlier revision's claim as an overstatement and keeps the corrected reasoning beside it, exactly as:101-117already does for the withdrawn upper-bound claim. A comment that carries its own errata is far more trustworthy than one that has been silently tidied, and it is what let me disposition both findings from the file alone. - The right fix was chosen for the stale cross-reference. Deleting the duplicated figure rather than refreshing it removes the drift hazard permanently instead of resetting it for one commit — and the commit says so in as many words, which turns a bug fix into a convention.
- Size-not-boundedness is the correct distinction, and it is quantified rather than asserted. The observation that a 1.05m step reaching a 15m cap implies a hang (which a bound catches) rather than slow work is the load-bearing insight, and it is what makes declining to bound the other 17 a defensible call instead of an omission.
- Every figure I checked reproduced, several to the decimal and to the same record run, on a window drawn independently of the author's. Across five heads this PR's empirical claims have held up under re-sampling each time.
Recommended Action
- No Critical or Important issues — nothing blocks merge.
- Reconcile the 176/181 success count at
:133before merge; it is a one-word fix in the artifact this PR exists to make trustworthy. - Consider giving the 17-step attribution gap a tracked disposition, in line with the other residuals.
…(BLO-32670) Ally review on #1713 (0 Critical, 0 Important, 4 suggestions). Three applied, one alternative rejected on the merits. - The 3m contention floor and the `bound < cap` ceiling are coupled: a policy job cap at or below the floor makes them jointly unsatisfiable. Verified by mutating the real cap 10 -> 3, which surfaced only as `"...(BLO-31254)" bound 3m must sit below the 3m policy job cap` — a message that blames whichever step is checked first. #1642 sizes that cap, so assert joint satisfiability as its own test and let the collision name itself. - `(\d+)` rejects the fractional bounds GitHub Actions accepts, so `timeout-minutes: 1.5` failed with `must declare a step-level timeout-minutes` — the opposite of what the author wrote. Verified against `Test CODEOWNERS guard`. One `timeoutMinutes()` helper now replaces five copies of the parse and fixes all of them. - The BLO-32670 rationale sat above `policyJobCap()` while explaining `assertTimeouts`. Split so each function carries its own why. - `bound < cap` was described as making a hang "fail attributably". It is a ceiling, not that guarantee: a bound near the cap is reached only after earlier steps have spent budget, so the job cap kills it first. Comment now says what the assertion proves. Did NOT take the offered stronger rule of summing bounds against the cap — bounds are per-step worst cases and all 41 steps are expected to run, so that sum (~47m against a 10m cap) exceeds any sane cap by design and would fail immediately. pr.yml is unchanged. Suite 8 -> 9 tests, all green; re-ran four mutations (fractional now accepted, cap collision named, floor revert still bites, bound removal still bites). Co-Authored-By: Claude <noreply@anthropic.com>
|
Small heads-up from BLO-32670 / #1713, which also edits #1713 re-budgets three fork-heavy
No action needed on this PR beyond that. |
Thinking Path
Linked Issues or Issue Description
policycame backcancelledat head65951469with every gate step passingWhat Changed
.github/workflows/pr.yml:policy's job-leveltimeout-minutes10 → 15, with the sizing basis recorded in the adjacent comment the way the 5 → 10 raise was..github/workflows/pr.yml: addedtimeout-minutes: 8to theCheckout repositorystep, which previously carried no bound — so a wedgedfetch-depth: 0fetch can only surface as a job-cap cancellation. 8m is ~1.8× the measured 4.42m p100.scripts/__tests__/pnpm-setup-retry.test.mjs: guard floorMIN_TIMEOUT_MINUTES_FOR_RETRY10 → 15, with its rationale re-derived from measured parts and an explicit statement that 15 does not buy full retry-path coverage.The sizing
Checkout repository+Setup pnpmStated as the number the issue asked for: 15m − 8.05m worst-observed setup = 6.95m for gate work that measures 2.2m median / 3.90m p100. Both historical blowouts (reconstructed at ~10.4m and ~10.7m needed) now fit with over 4m to spare. At the old cap, 2 of 75 runs were cancelled at the cap and 5 more finished between 9.5m and 9.9m — 7 of 75 surviving on under 30s of luck.
Two claims the first pass asserted, and this one measures
The old 5 → 10 arithmetic mis-attributed the cost. It read "2 × (bootstrap + 120s registry stall) + backoff ≈ 5m" (BLO-28813). But the retry fired in 0 of 75 runs — the wrapper logs
conclusion=skippedon bothBack off before retrying pnpm setupandSetup pnpm (retry)— while a successful first attempt still took 254.5s (duration_ms=254504; 259s for the step as a whole). 243s of that is the bootstrapnpm ci("added 1 package in 4m") and only ~7s the self-update the old model blamed. So pnpm's ~4.3m is one slow success whose cost sits in the bootstrap, not two attempts against a stalled registry.cancelleddoes not mean "hit the cap". 6 of the 75 runs readcancelled, but only the 2 at 10.2m are cap deaths — the other 4 stopped at 2.9–5.1m and are superseded pushes.Verification
node --test scripts/__tests__/pnpm-setup-retry.test.mjs scripts/__tests__/policy-node-test-timeouts.test.mjs→ 12/12 pass, including "the chart render step is bounded, and inside its job's budget (BLO-29182)".ef141304d:No step bound was raised, removed, or lowered to buy this; the one addition bounds a step that had none.
./.github/actions/setup-pnpm, 23 declare a budget (the 24th is deliberately listed inWRAPPER_JOBS_WITHOUT_A_BUDGET), the next-lowest two declare exactly 15, andpolicywas the lone outlier at 10 — and the only one observed dying at its cap. Re-checked after rebase, since master had moved 9 commits.policysuite locally: 440 tests, 433 pass. The 4findLocalRangeOffensesfailures and the 1check-shard-manifest-freshnessfailure reproduce identically onorigin/masterand are untouched by this diff; the manifest one is in a step marked(non-blocking).gh api "repos/Blockcast/paperclip/actions/runs/$r/jobs" --jq '.jobs[]|select(.name=="policy")|…'over the last 75 completed runs.policyrun is the first data point.Risks
policyjob now burns 15m of runner instead of 10m before dying. That is the deliberate trade — the per-step bounds stay load-bearing precisely so a real hang fails as its own step first, and the new checkout bound closes the largest previously-unbounded step.Setup pnpmalone (~19m for the whole job) — covering it would budget every run for a path 0 of 75 took; (2)Setup pnpmstill has no step bound, since bounding it means picking a number above that 10.8m path.Model Used
claude-opus-5), 1M-context variant, extended thinking enabled, with tool use and code execution — run as the Paperclip Release Engineer agent.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template