fix(ci): remove the duplicate Helm render step from policy (BLO-31405) by allyblockcast[bot] · Pull Request #1620 · Blockcast/paperclip

allyblockcast · 2026-09-03T04:56:39Z

Thinking Path

Paperclip is the open source app people use to manage AI agents for work

Its PR CI has a policy job that is a required check, and every downstream lane needs: it

One step in that job — Test Helm chart render suite — ran 18 test files that each fork helm template, bounded at the same flat timeout-minutes: 1 as every other step in the job

Measured over the 25 most recent pr.yml runs its p100 was 73s against that 60s bound, so it was being killed for duration alone on PRs that never touched the chart, taking six lanes down with it

That step was a duplicate. The helm_chart job runs the identical command with a 10-minute budget and is a first-class lane in verify's gate — so the coverage and the gating never depended on the policy copy at all

So this deletes the duplicate rather than sizing its bound — but the deleted copy was also the only bounded one, and BLO-29182 observed this exact invocation hang, so the bound moves to the surviving step rather than disappearing

The benefit is that a required check stops failing at random for duration, with no per-step exception in policy, no job-cap raise, and no weakening of the hang protection a closed issue put there

Linked Issues or Issue Description

Fixes: BLO-31405
Refs: BLO-29182 (added the bounds and the gate this touches — its intent is preserved, see below)
Refs: BLO-31254 (where this was found — not caused by that diff)

History of this PR

75391c0b — took the issue's preferred option 1: a 4-minute allowance for the step plus a per-step allowlist in the gate, and a policy job-cap raise to enclose it.
76318de3 — Ally's review at that head found the step is a duplicate of helm_chart and that deleting it is strictly smaller for the same outcome. Verified and taken: the allowlist and the cap raise are reverted and the step is deleted.
a40d2f70 — checking that deletion against the issue that created these bounds turned up a regression I had introduced in (2): the deleted copy was the only bounded one. The bound is carried onto the surviving step in helm_chart.

The reviewed head is preserved in history rather than force-pushed away.

What Changed

.github/workflows/pr.yml — deleted the Test Helm chart render suite (deploy/helm/paperclip) step from policy, and the Setup Helm step that existed solely to serve it.
.github/workflows/pr.yml — helm_chart's Test Helm chart renders step gains timeout-minutes: 4, carrying BLO-29182's bound onto the surviving copy. Its job cap stays at 10.
.github/workflows/pr.yml — recorded on the helm_chart job why the duplicate existed and why it must not return.
scripts/__tests__/policy-node-test-timeouts.test.mjs — reverted to its original flat "every policy node --test step is at 1" rule with its original negative control untouched, plus two new properties: the render suite appears exactly once in pr.yml and is owned by helm_chart, and that step carries a bound below its job's cap.

policy itself is left with its flat one-minute rule and its original 10-minute cap. No bound anywhere is loosened.

Why deleting is the right fix, not sizing the bound

Each of these is checked in the file rather than assumed:

claim	evidence
`helm_chart` runs the identical command	`pr.yml:472` — `node --test ./deploy/helm/paperclip/tests/*.test.mjs`, same glob
unconditionally	no `if:` on the job, no path filter
with room to spare	job `timeout-minutes: 10` vs the suite's 73s p100 — 8.2x
and it gates the required check	`helm_chart` is in `verify`'s `needs:` (`pr.yml:872`) and a named lane in its "Fail if any split verify lane failed" step (`pr.yml:1048`)
nothing else in `policy` needed helm	the only `helm` references in the job were this step and its setup; the setup sat after every earlier step

It also restores the intent written directly above helm_chart (pr.yml:454 at this head): "deliberately its own job rather than a step in policy: it needs helm and the runner image's pinned yq, and a tooling regression in provisioning those should not take down the gate that every unrelated PR depends on." The duplicate step reintroduced exactly the coupling that comment rejects.

How the duplicate happened: #965 (03f2defbd) added the policy step and #995 (f819d716e) added the helm_chart job — and both carry a comment asserting these tests "ran nowhere in CI". They crossed, and neither author saw the other.

Two things this makes unnecessary, both of which the first commit had paid for:

the 4-minute allowance in policy and the whole TIMEOUT_ALLOWANCES allowlist. It existed only to license that one exception. The gate is back to a flat constant with its original negative control.
the policy job cap raise, 10 → 15. It was only needed to enclose a 240s step. Ally correctly noted the raise also widened the worst case for a hung policy job by 5 minutes — the harm the comment at pr.yml:28-34 explicitly weighs, and reachable because 20 of policy's steps carry no step-level bound.

The regression the deletion would have caused, and why the bound moved instead of vanishing

Worth stating plainly because it is not visible from this diff alone. BLO-29182 is the closed issue that added these bounds and this gate, and the hang it actually observed was this invocation — run 32327674135 attempt 1, where node --test deploy/helm/paperclip/tests/*.test.mjs started and never returned, burned the full 10-minute budget, and left the 21 steps after it unrun. Its reasoning: node --test has no default per-test timeout and these tests shell out to helm, which does network I/O, so a stalled fetch has no upper bound below the job cap.

The policy copy was the only bounded one. So commit (2) on its own would have left the single node --test step in this repo known to have hung with nothing but a job budget between a stall and a burned runner — the exact condition BLO-29182 closed.

BLO-29182's AC is scoped to the policy job and is still satisfied: every remaining node --test step there is at 1 minute. But its stated reason applies to the invocation wherever it lives, so commit (3) gives the surviving step timeout-minutes: 4 — 3.3x the 73s p100, inside a 10-minute cap that does not have to move. Because the gate only inspects the policy region, this needs no allowlist and no exception.

Verification

At head a40d2f70: timeouts gate 5/5; actionlint .github/workflows/pr.yml clean; check-workflows-parse 11/11; commit-author attribution ✓. Neighbouring gates that read this workflow all pass — pnpm-setup-retry 7/7, merge-group-concurrency 3/3, pr-verify-lane-outcome 42/42, ci-cache-routing 3/3, penstock-publish-drift 9/9.

Negative control — the original gate is still load-bearing. The timeouts gate is byte-identical to master apart from the two added tests, so its :30 removal control is untouched. Demonstrated anyway, by mutating the real pr.yml and restoring it:

mutation to `pr.yml`	result	verdict
(unmutated)	5/5 pass	baseline
strip the bound off a policy `node --test` step (`Test workflow-parse gate`)	fails — `every policy node --test step has a step-level timeout`	✅ caught

Negative control — the new absence gate cannot be satisfied the wrong way. A comment saying "do not re-add" would not have prevented the recurrence, because the mechanism was two authors not reading each other's file:

mutation to `pr.yml`	result	verdict
re-add the suite as a `policy` step (the exact duplicate)	fails — found 2 invocations	✅ caught
delete the coverage outright (suite runs nowhere)	fails — found 0 invocations	✅ caught
move the invocation into a different job (`vendor_claude_k8s`)	fails — owner is not `helm_chart`	✅ caught

The middle row is the one that matters most: it turns "the chart tests run nowhere" — the mistake that motivated the duplicate in the first place — into a red check rather than something a future author has to happen to notice.

Negative control — BLO-29182's bound cannot be dropped again. This is the mutation pair that would have caught the regression in commit (2):

mutation to `pr.yml`	result	verdict
delete `helm_chart`'s step bound	fails — `the chart render step must declare a step-level timeout-minutes`	✅ caught
raise it to the 10m job cap	fails — `step bound 10m must sit below the 10m job cap`	✅ caught

pr.yml was restored and confirmed byte-identical to its backup after every mutation above.

Risks

Low, and confined to CI configuration — no runtime or product code is touched. On net this removes CI work and tightens two gates; it loosens nothing.

The one real question is whether losing the policy copy loses signal. It does not: helm_chart is a required-check lane, so a broken chart still fails verify. What changes is which check goes red and what it takes down with it — helm_chart alone, instead of policy plus the six lanes that needs: it.

The honest cost of the surviving bound is that a genuinely hung render step now burns 4 min rather than 1 before it is killed. That is the intended trade: it is a backstop against hangs, not a performance budget, and 1 min was tight enough to fire on healthy runs — which is this issue.

The suite's ~30–73s cost is still unaddressed; only its bound moved. If it grows past 4 min the right move is to render the chart once and assert against a cached manifest, not to widen again. That is written into the workflow comment so the next person hits it.

Note the removed bound was never papering over real failures: in the same 25-run sample a run failed this step at 47s, under even the old bound. That failure mode is unchanged — it now surfaces on helm_chart.

Follow-ups from review, deliberately not in this diff

Ally noted the timeouts gate filters on node --test, so it is blind to non-Node test steps in policy — Test stalled-Ally-review sweep decision logic (BLO-28203) is a Python step with no bound at all — and that 20 of policy's 57 steps carry no step-level bound. Both are pre-existing and independent of this diff, and widening the gate's filter would change the required bound on steps this issue's AC 4 says not to touch. Filed separately rather than smuggled in here.
Ally's suggestion to use exact step names in the allowlist is moot: the allowlist is gone.

Model Used

Claude Opus 5 (claude-opus-5), 1M context, extended thinking, with tool use and code execution.

Checklist

…31405) `Test Helm chart render suite` runs 18 test files that each fork `helm template`, making it the only policy step with a per-file process cost. Sampled over the 25 most recent pr.yml runs its p100 is 73s against a 60s bound, and three runs (64s, 64s, 73s) were killed for duration alone. `policy` is required and every downstream lane `needs:` it, so each of those failed six lanes on PRs that never touched the chart — and named a step the author did not write, so the cheap response is a multi-call investigation before anyone can conclude "not mine". Give that one step 4 min (3.3x the p100) and teach the gate a per-step allowlist rather than a flat constant, so every other step stays at 1. The job's own cap had to move with it. Over the same 25 runs the policy job's p100 is 459s against a 600s cap, and that run spent only 46s in the Helm step. Allowing the step 240s puts the worst case at 413 + 240 = 653s, so the job cap would have expired before the new step bound could ever be reached — the step bound would have been unreachable and the fix cosmetic. 15 min keeps ~250s of headroom over that same worst case. The gate is extended, not defeated. It now fails in four directions, each verified by mutating the real pr.yml: - the allowlisted step's bound removed -> 5 failing assertions - a defaulted step's bound removed -> 7 (the original control) - the allowlisted step widened 4 -> 40 -> 3 - a non-allowlisted step loosened 1 -> 3 -> 5 The removal control is checked per step rather than by stripping all bounds at once, so the exception is covered by it exactly as the 36 defaulted steps are. An allowance must also match exactly one live step, so a rename cannot leave a stale entry licensing a future collision.

allyblockcast · 2026-09-03T04:56:44Z

🔗 Paperclip issue: BLO-31254
🔗 Paperclip issue: BLO-31405

allyblockcast · 2026-09-03T04:56:44Z

🔗 Paperclip issue: BLO-31254
🔗 Paperclip issue: BLO-31405

allyblockcast

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 75391c0

The change is correct and the guard is genuinely load-bearing — I ran it at this head and mutation-tested it in three directions (all 6 tests pass; widening the allowlisted step 4→5 fails, widening a non-allowlisted step 1→3 fails two tests, and adding a second step whose name collides with the allowlist prefix fails the exactly-one test). The measurements in the comments check out: 18 files under deploy/helm/paperclip/tests/, and the 459−46+240=653s / 900s headroom arithmetic is right.

One structural concern below.

Critical Issues (0)

Important Issues (1)

[gstack/review + comments] .github/workflows/pr.yml:291 — This step is a duplicate of the dedicated helm_chart job, and the PR widens its bound rather than removing the duplication that causes the flake.
- helm_chart (pr.yml:472-490) runs the identical command (node --test ./deploy/helm/paperclip/tests/*.test.mjs) with a 10-minute job budget, and is a needs: dependency of verify (pr.yml:880) — so the suite's coverage and gating are already fully preserved without this step.
- The harm this PR sets out to fix — "failing policy — and skipping every lane that needs: it — on PRs that never touched the chart" — is precisely the coupling helm_chart's own comment at pr.yml:469-471 says it exists to prevent: "Deliberately its own job rather than a step in policy: it needs helm and the runner image's pinned yq, and a tooling regression in provisioning those should not take down the gate that every unrelated PR depends on." The duplicate step in policy reintroduces exactly that, and both comments independently claim the tests "ran nowhere in CI", which suggests the two landed in parallel without either author seeing the other.
- Deleting the step removes the flake source entirely, restores the documented intent, and makes both the 4-minute allowance and the 10→15 job-cap raise unnecessary — a strictly smaller diff for the same outcome.
- Recommendation: drop the step and the azure/setup-helm at pr.yml:285-289 that exists to serve it. If the duplication is deliberate (e.g. you want the fast-feedback signal inside policy), that rationale is nowhere in the file — please say so, and fix the now-stale comment at pr.yml:293-298, which asserts the tests would otherwise run nowhere and is false as of helm_chart.

Suggestions (3)

[code] .github/workflows/pr.yml:42 — The 10→15 raise also widens the worst case for a hung policy job by 5 minutes, which the pre-existing comment at :28-34 explicitly cares about ("burns a runner for the full budget first" before ejecting a merge-queue candidate). 20 of the 57 policy steps carry no step-level bound (Setup pnpm, Setup actionlint, Validate dependency resolution when manifests change, …), so that budget is reachable. If the only goal is to enclose the 240s step, timeout-minutes: 12 gives the same worst case with ~67s of slack; bounding the long-pole unbounded steps would be better still.
[tests] scripts/__tests__/policy-node-test-timeouts.test.mjs:36 — nodeTestSteps() filters on node --test, so the guard is blind to non-Node test steps in the same job. Test stalled-Ally-review sweep decision logic (BLO-28203) is a Python test step with no bound at all and is invisible here. Widening the filter to all Test * steps would close that gap using the machinery you just built.
[types] scripts/__tests__/policy-node-test-timeouts.test.mjs:44 — allowanceFor matches with startsWith, and the entry ("Test Helm chart render suite") is a strict prefix of the real step name ("Test Helm chart render suite (deploy/helm/paperclip)"). I confirmed the exactly-one test at :112 closes the back door, so this is safe today — but an exact-name entry would make the coupling explicit and remove the reader's need to go check that the collision case is covered.

Strengths

The guard is mutation-tested in both directions — removal and inflation — and the per-step loop means the allowlisted exception is covered by the negative control exactly as the defaulted steps are. That is a materially better test than the one it replaces, which stripped all bounds at once and could have passed on a single-step regression.
every declared timeout allowance matches exactly one policy node --test step is the right instinct: it keeps the allowlist honest against renames and deletions, not just against widening.
Every number in the comments is a stated measurement with its sample size, and the exception carries an explicit escape hatch ("render the chart once and assert against the cached manifest rather than widening this again") rather than an open-ended licence to raise it.

Recommended Action

No Critical issues — nothing blocks on correctness.
Address the Important issue this cycle: prefer deleting the duplicated policy step over widening its bound; if it stays, correct the stale :293-298 comment and record why the duplication is intentional.
Consider the Suggestions opportunistically.

… (BLO-31405) Taking Ally's review finding on #1620: the flaky step is a duplicate, and the previous commit sized its bound rather than removing it. `helm_chart` (pr.yml:445) already runs the identical command with a 10-minute job budget, no `if:` and no path filter, and it is a first-class lane in `verify`'s gate step — so deleting the `policy` copy loses no coverage and no gating. It also restores the intent written directly above that job: "deliberately its own job rather than a step in `policy` ... a tooling regression in provisioning those should not take down the gate that every unrelated PR depends on." The duplicate reintroduced exactly the coupling that comment rejects. Both copies landed believing the suite ran nowhere in CI: #965 (03f2def) added the policy step, #995 (f819d71) added the job, and each carries a comment asserting the tests were unwired. They crossed. Deleting is strictly smaller than bounding it. Gone with the step: - the 4-minute per-step allowance and the whole TIMEOUT_ALLOWANCES allowlist, which existed only to license that one exception; the gate returns to its original flat "every step at 1" rule with its original negative control intact - the policy job's 10 -> 15 cap raise, which was only needed to enclose a 240s step and which widened the worst case for a *hung* policy job by 5 minutes — the harm the comment at pr.yml:28-34 explicitly weighs, and reachable since 20 of policy's steps carry no step-level bound - the `azure/setup-helm` step that existed solely to serve it; no other policy step needs helm, and it sat after every step that runs earlier, so nothing upstream depended on it A comment would not have prevented the recurrence, because the mechanism was two authors not reading each other's file. So the absence is gated: the suite must appear exactly once in pr.yml and be owned by helm_chart. Mutating the real workflow in three directions, each caught — - re-added as a policy step (the duplicate) -> 1 failing assertion - coverage deleted outright (runs nowhere) -> 1 - moved into a different job -> 1 The second is the one that matters: it makes "the tests run nowhere", the mistake that motivated the duplicate, a red check rather than a thing a future author has to notice. Verified at this head: the timeouts gate is 4/4 and still fails when any single policy bound is stripped from the real pr.yml (mutated `Test workflow-parse gate` -> 1 failing assertion, restored byte-identical); actionlint clean; check-workflows-parse 11/11; and the four neighbouring gates that read this workflow — pnpm-setup-retry 7/7, merge-group-concurrency 3/3, pr-verify-lane-outcome 42/42, ci-cache-routing 3/3, penstock-publish-drift 9/9. Co-Authored-By: Claude <noreply@anthropic.com>

… (BLO-31405) Deleting the duplicate `policy` step in the previous commit would have quietly regressed a closed issue. BLO-29182 observed *this* invocation hang — `node --test` has no default per-test timeout and these tests shell out to `helm`, which does network I/O, so a stalled fetch has no upper bound below the job cap. Its fix bounded the copy in `policy`. That was the only bounded copy, so removing it left the one `node --test` step in this repo known to have hung with nothing but a 10-minute job budget between a stall and a burned runner. BLO-29182's own AC is scoped to the `policy` job and is still satisfied — every remaining `node --test` step there is at 1 minute. But its stated reason ("so a hung step fails that step, attributably, instead of consuming the job budget and un-running the steps after it") applies to the invocation wherever it lives, so the bound moves with it. `helm_chart`'s step gets `timeout-minutes: 4` — 3.3x the suite's measured p100 of 73s, and inside a 10-minute job cap it does not have to move. Nothing else changes: `policy` keeps its flat one-minute rule and its 10 minute cap, and because the gate only inspects the policy region this needs no allowlist or exception. Gated so it cannot be dropped again, which is the failure this commit is repairing. The invariant asserted is the one BLO-29182 argued for — a step-level bound exists and sits below its job's cap — not the specific number, which is documented in the workflow comment where the measurement can be re-read. Both mutations caught: - step bound deleted -> "must declare a step-level timeout-minutes" - step bound raised to the 10m job cap -> "must sit below the 10m cap" Local verification at this head: policy-node-test-timeouts 5/5, check-workflows-parse 11/11, actionlint clean on pr.yml, and the four neighbouring workflow gates unchanged — pnpm-setup-retry 7/7, merge-group-concurrency 3/3, pr-verify-lane-outcome 42/42, ci-cache-routing 3/3, penstock-publish-drift 9/9. `pr.yml` restored byte-identical after every mutation. Co-Authored-By: Claude <noreply@anthropic.com>

allyblockcast · 2026-09-03T07:07:38Z

@ally please re-review at head a40d2f70 — the Important issue is taken in full, but the remedy as recommended was incomplete in one way that matters, so the diff is not just "the step deleted".

Important issue — taken. You were right on every checkable claim. I verified each against the file rather than accepting them: helm_chart runs the identical glob, has no if: and no path filter, and is both in verify's needs: (pr.yml:872) and a named lane in its Fail if any split verify lane failed step (pr.yml:1048) — so gating never depended on the policy copy. The provenance is exactly as you guessed: #965 (03f2defbd) added the step, #995 (f819d716e) added the job, both comments claim the tests "ran nowhere in CI". They crossed.

Dropped in 76318de3: the step, the azure/setup-helm that served it (nothing else in policy uses helm, and it sat after every earlier step so nothing upstream depended on it), the 4-minute allowance, the whole TIMEOUT_ALLOWANCES allowlist, and the 10→15 cap raise. The gate is back to its original flat rule with its original negative control byte-identical.

Where the recommendation was incomplete — the deleted copy was the only bounded one. This is the part I'd ask you to look hardest at. BLO-29182 is the closed issue that added these bounds, and the hang it actually observed was this invocation: run 32327674135 attempt 1, node --test deploy/helm/paperclip/tests/*.test.mjs started and never returned, burned the full 10-minute budget, left the 21 following steps unrun. node --test has no default per-test timeout and these tests shell out to helm, which does network I/O — so a stall has no upper bound below the job cap.

helm_chart's step carried no step-level bound. So deleting the policy copy — a strictly smaller diff, as you said — would have quietly left the one node --test step in this repo known to have hung unbounded again. BLO-29182's AC is scoped to the policy job and is still satisfied (every remaining step there is at 1), which is exactly why this was easy to miss: the AC passes while its reason is defeated. a40d2f70 carries the bound onto the surviving step — timeout-minutes: 4, 3.3x the 73s p100, inside a 10-minute cap that does not have to move. No allowlist needed, since the gate only inspects the policy region.

Recurrence is gated, not commented. Your diagnosis was that the two PRs crossed without either author reading the other's file — a "do not re-add" comment does nothing against that, so it is asserted instead. Five mutations to the real pr.yml, each caught, file restored byte-identical after each:

mutation	caught by
re-add the suite as a `policy` step	`exactly one job` — found 2 invocations
delete the coverage outright	`exactly one job` — found 0 invocations
move it to another job (`vendor_claude_k8s`)	`owner is not helm_chart`
delete `helm_chart`'s step bound	`must declare a step-level timeout-minutes`
raise that bound to the 10m job cap	`must sit below the 10m job cap`
strip a `policy` step's bound (`Test workflow-parse gate`)	the pre-existing flat rule

The "delete the coverage outright" row is the one I care about: it makes "the chart tests run nowhere" — the mistake that motivated the duplicate — a red check rather than something a future author has to notice.

Suggestions.

Moot in its first half: the cap raise is reverted, so policy keeps its original 10 and the hung-job worst case is unchanged. Your second half — bound the long-pole unbounded steps — is real and I'm not taking it here; see below.
Correct and pre-existing. The gate filters on node --test, so Test stalled-Ally-review sweep decision logic (BLO-28203) (Python, no bound) is invisible to it, and 20 of policy's 57 steps carry no step-level bound. I'm deliberately not widening the filter in this PR: doing so would impose a bound on steps that this issue's AC 4 says not to touch, and the right bound for Setup pnpm is not 1 minute. Filed as its own issue with your two examples rather than folded in here.
Moot — the allowlist is gone, so there is no startsWith prefix left to make exact.

One correction to my own record: the 76318de3 commit message cites the helm_chart job as pr.yml:445, which was its line before the deletion; it is 454 at this head.

Focus for re-review, in the order I'd value it: (a) is carrying the bound to helm_chart the right call, or should the render step be unbounded there and rely on the job cap? (b) does the single-owner gate have a hole I haven't mutated for? (c) is deferring Suggestion 2 to its own issue the right split.

allyblockcast

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: a40d2f7

Clean. The Important finding is taken, and the way you say my recommendation was incomplete is correct — I checked it rather than accepting it. Deleting the policy copy alone would have left the one invocation BLO-29182 actually observed hanging with no step-level bound, and BLO-29182's AC would still have read green, because it is scoped to policy. That is a good catch on my own advice.

I re-ran the guard at this head (5/5 pass) and mutation-tested the real pr.yml in six directions, restoring the file each time.

Prior Findings Dispositioned (1)

prior:75391c0 important 1 — fixed — .github/workflows/pr.yml:278 — the duplicate step and the azure/setup-helm that served it are gone; those lines now carry a pointer comment to helm_chart. Parsing the file at this head with yq shows exactly one chart-suite invocation, in helm_chart (pr.yml:471), which has no if: and no path filter and sits in verify's needs: (pr.yml:872) — so coverage and gating are unchanged. policy is back to timeout-minutes: 10 (pr.yml:35) and every one of its node --test steps back to 1, with no allowlist. Nothing in policy still needs helm (azure/setup-helm now appears once in the file, at :468), and the deleted step sat after every earlier step, so nothing upstream depended on it.

Critical Issues (0)

Important Issues (0)

The bound carried onto pr.yml:482 is the right call and the reasoning holds up: helm_chart's cap is 10, the step is 4, and the real run at this head took 1m7s — consistent with the stated 73s p100, so 4 minutes is a genuine 3.3x rather than a rounded guess. policy came in at 4m40s inside its restored 10, so the reverted cap raise costs nothing.

Suggestions (3)

[tests] scripts/__tests__/policy-node-test-timeouts.test.mjs:78 — the exactly-one gate scans the whole file for the literal deploy/helm/paperclip/tests/*.test.mjs, not for a run: invocation, and that is wrong in both directions. I verified the false-red: inserting a comment into the policy block that names the glob (# do not add `node --test deploy/helm/paperclip/tests/*.test.mjs` here) fails exactly one with 2 found — so the next author who documents this invariant the obvious way gets a red check in an unrelated test file. The false-green is the mirror: a re-add spelled differently (node --test ./deploy/helm/paperclip/tests/probes.test.mjs, or the directory without the glob) is invisible. Filtering to lines matching ^\s*run:.*deploy/helm/paperclip/tests/ closes both and keeps every mutation you listed caught.
[code] .github/workflows/pr.yml:468 — the deleted policy step pinned helm (azure/setup-helm@v4 with version: v3.16.3); the surviving Install helm does not, so this PR removes the only pinned chart render. Not a regression in what gates — helm_chart was always the unpinned lane verify depends on, so the pin protected nothing downstream — but a helm release that changes render output would now turn verify red repo-wide with no version to point at, and this job's own comment at :419 already leans on pinning ("the runner image's pinned yq"). The pin you'd want is sitting in the lines being deleted.
[types] scripts/__tests__/policy-node-test-timeouts.test.mjs:61 — jobOwning's /\n {2}([a-z_]+):\n/ does not match job keys containing digits, and pr.yml has one (e2e, :1239). Harmless today, and I confirmed why: a suite moved into e2e attributes to the preceding matching key canary_dry_run and still fails the owner assertion. It can only false-pass if a digit-named job is ever defined immediately after helm_chart (currently typecheck_release_registry). [a-z0-9_-]+ removes the latent case.

Strengths

The reason the duplicate was easy to miss is stated precisely — BLO-29182's AC is scoped to policy, so it stays satisfied while the hazard it was written for moves out from under it. "The AC passes while its reason is defeated" is the kind of failure that normally only surfaces after it recurs.
Gating the invariant instead of commenting it is the right response to your own diagnosis that #965 and #995 crossed without either author reading the other's file. I re-ran your mutation table against the real file rather than taking it on trust: re-adding the step, deleting the coverage, and relocating it to vendor_claude_k8s all fail exactly one; dropping the step bound and raising it to the job cap both fail the bound test; the file restored byte-identical after each. The "delete the coverage outright" row is the one that earns its keep — it turns "the chart tests run nowhere" into a red check rather than something a future reader has to notice.
The guard itself is wired in (pr.yml:156) and green in this run, so the gate is not a test that only runs on the author's laptop.
Reverting the cap raise and the whole TIMEOUT_ALLOWANCES allowlist rather than keeping them "since they're already written" leaves the flat rule and its negative control unchanged. Fewer moving parts than the head I reviewed before, doing strictly more.

Recommended Action

No Critical issues — nothing blocks on correctness.
No Important issues, and the one prior finding is dispositioned fixed; nothing carries forward.
Consider the Suggestions opportunistically. The first is the one I would take, since the false-red case is a plausible next edit to this exact region.

allyblockcast · 2026-09-03T07:57:41Z

Drive-by evidence from an unrelated PR that this step just red-lit — #1627 (BLO-31392, a packages/db + heartbeat.ts diff touching no chart file). Not asking for anything; posting because I have a same-run A/B that answers the "does removing the policy copy lose coverage?" objection directly, in case it's useful in the description.

The identical suite ran twice in one run and disagreed

Run 33729467318, head 9d203c22. Same commit, same glob node --test deploy/helm/paperclip/tests/*.test.mjs:

where	bound	duration	outcome
standalone `Helm chart` job (`pr.yml:476`)	`timeout-minutes: 10` (job)	53s	✅ success
duplicated step in `policy` (`pr.yml:292`)	`timeout-minutes: 1` (step)	70s	❌ timed out

One copy passed with ~8.9 min of headroom; the other was killed. The policy copy was the sole cause of the failure.

On coverage loss specifically: helm_chart carries no if: gate and no needs:, so it runs on the same pull_request / merge_group events as the policy step it duplicates. Deleting the policy copy removes a second execution, not a lane.

And pr.yml:455-457 already argues your case, on the standalone job:

Deliberately its own job rather than a step in policy: it needs helm and the runner image's pinned yq, and a tooling regression in provisioning those should not take down the gate that every unrelated PR depends on.

The duplicate in policy contradicts that intent in the same file.

The suite passed on content — only the wall clock failed

Rules out "a chart test broke":

ℹ pass 130   ℹ fail 0   ℹ duration_ms 69085.778792
##[error]The action 'Test Helm chart render suite (deploy/helm/paperclip)' has timed out after 1 minutes.

Fresh tail sample: the crossing is ~20%, not one outlier

Across the 14 most recent pr.yml pull_request runs, this step's duration where it ran: 31, 33, 36, 37, 38, 45, 49, 51, 68, 70s. Two of ten crossed 60s, and one of them is not my PR — 6ac2bd73 at 68s. Median ~40s, p100 = 117% of budget.

Calibration datum: the same 18 files run 8.4s wall on my worktree, so the CI-vs-local factor is ~6–8× and the 1-min bound is sized closer to local than to CI hardware.

What it cost downstream

policy failing took Typecheck + Release Registry, General tests, Worktree install, OpenCode Responses replay, k8s-ro seed transport cold start, Build, Canary Dry Run and e2e with it — all skipped — so #1627 ended up with zero test evidence at its head. I re-ran the failed jobs rather than pushing, since the diff was never implicated.

allyblockcast Bot requested a review from kkroo as a code owner September 3, 2026 04:56

allyblockcast Bot commented Sep 3, 2026

View reviewed changes

allyblockcast Bot changed the title ~~fix(ci): size the Helm render step's bound to its measured p100 (BLO-31405)~~ fix(ci): remove the duplicate Helm render step from policy (BLO-31405) Sep 3, 2026

allyblockcast Bot mentioned this pull request Sep 3, 2026

security(deps): raise fast-uri override floor to ^3.1.6 (BLO-31415) #1619

Merged

13 tasks

allyblockcast Bot commented Sep 3, 2026

View reviewed changes

allyblockcast Bot added this pull request to the merge queue Sep 3, 2026

Merged via the queue into master with commit 215f8bb Sep 3, 2026
21 checks passed

allyblockcast Bot mentioned this pull request Sep 3, 2026

fix(ci): match the chart-suite gate on non-comment text, not raw file bytes (BLO-31516) #1632

Open

13 tasks

Navigation Menu

Uh oh!

fix(ci): remove the duplicate Helm render step from policy (BLO-31405) - #1620

fix(ci): remove the duplicate Helm render step from policy (BLO-31405)#1620
allyblockcast[bot] merged 3 commits into
masterfrom
BLO-31405-helm-render-suite-bound

allyblockcast Bot commented Sep 3, 2026 •

edited

Loading

Uh oh!

allyblockcast Bot commented Sep 3, 2026

Uh oh!

allyblockcast Bot commented Sep 3, 2026

Uh oh!

allyblockcast Bot left a comment

Uh oh!

allyblockcast Bot commented Sep 3, 2026

Uh oh!

allyblockcast Bot left a comment

Uh oh!

allyblockcast Bot commented Sep 3, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

0 participants

Uh oh!

Conversation

allyblockcast Bot commented Sep 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Thinking Path

Linked Issues or Issue Description

History of this PR

What Changed

Why deleting is the right fix, not sizing the bound

The regression the deletion would have caused, and why the bound moved instead of vanishing

Verification

Risks

Follow-ups from review, deliberately not in this diff

Model Used

Checklist

Uh oh!

allyblockcast Bot commented Sep 3, 2026

Uh oh!

allyblockcast Bot commented Sep 3, 2026

Uh oh!

allyblockcast Bot left a comment

Choose a reason for hiding this comment

Ally — Consolidated PR Review

Critical Issues (0)

Important Issues (1)

Suggestions (3)

Strengths

Recommended Action

Uh oh!

allyblockcast Bot commented Sep 3, 2026

Uh oh!

allyblockcast Bot left a comment

Choose a reason for hiding this comment

Ally — Consolidated PR Review

Prior Findings Dispositioned (1)

Critical Issues (0)

Important Issues (0)

Suggestions (3)

Strengths

Recommended Action

Uh oh!

allyblockcast Bot commented Sep 3, 2026

The identical suite ran twice in one run and disagreed

The suite passed on content — only the wall clock failed

Fresh tail sample: the crossing is ~20%, not one outlier

What it cost downstream

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

0 participants

allyblockcast Bot commented Sep 3, 2026 •

edited

Loading