Repository navigation
ci: let the consumer SBOM outlast the registry it waits on - #716
Conversation
Every release-triggered supply-chain run has failed since bestax-migrate 2.3.2, each time on the one matrix leg whose version the release names. The decision logic was right every time: the step resolved `@allxsmith/eslint-plugin-bestax@1.0.1` and `expect=1.0.1` correctly, then `npm install` returned ETARGET three times over half a minute and the leg gave up. Two things were wrong with the retry, and the step's own comment already predicted the failure it could not survive. `--prefer-online`, because npm caches the packument it fetched on the way in. Every attempt could re-read the same pre-publish document out of `~/.npm` and fail identically, which is what the logs show: the same ETARGET at each interval, no new registry read. Sleeping between attempts that all consult one cached document only proves the cache agrees with itself. A deadline instead of a fixed attempt count, because the thing being waited on is not this job. The `release` event fires when the GitHub release is created, and the version becomes installable when the registry finishes serving it. Nothing here can hurry that, so the budget has to be generous rather than clever. The old one spent well under a minute. The legs no release names are unaffected: they resolve `latest`, which always had something to resolve, so they still succeed on the first attempt and never reach the deadline.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reachedNext included review available in 18 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository: allxsmith/bestax/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: allxsmith/bestax/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughThe consumer SBOM workflow now calls a dedicated Node.js script for npm installation. The script uses duration-based retries, online package resolution, validated arguments, distinct exit codes, and injected test dependencies. Changesnpm install retry flow
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~30 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant consumer-sbom
participant npm-install-retry.mjs
participant npm
consumer-sbom->>npm-install-retry.mjs: pass package spec and consumer directory
npm-install-retry.mjs->>npm: install with --prefer-online and --ignore-scripts
npm-->>npm-install-retry.mjs: return installation status
npm-install-retry.mjs->>npm: retry within the configured budget
npm-install-retry.mjs-->>consumer-sbom: return exit code
Merge Risk: ⚪ Minimal · up to The consumer SBOM workflow now uses bounded online npm retries, with coverage for failure and logging paths; it is ready to merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description provides a detailed and relevant explanation of the failure, implementation, scope, testing, and follow-up issue. However, it does not use the required template sections or provide the required package selection, related-issue fields, type-of-change selection, checklist responses, or screenshots/additional-context headings. Resolution Reformat the description using the repository template. Add the affected package or explicitly select Other, complete the Related Issue(s) and Type of Change sections, mark each applicable Checklist item, and include the Screenshots / Demos and Additional Context sections with Not applicable where appropriate. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Preview DeploymentPreview URL: https://6f785e5d.bestax.pages.dev |
|
deep-review: fresh — please read ee6a9e6 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Deep review — 0 blocking · 3 advisory
| No. | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🔵 Advisory | Robustness | A non-propagation npm install failure (a version that never published, a registry outage, a broken tarball) now burns the full 600 s budget on that leg instead of failing in ~31 s. Accepted cost — fail-fast: false keeps the siblings independent, and the job sets no timeout-minutes, so the 360-minute default is nowhere near — but a genuinely broken released leg now reports red ~10 minutes later than it used to. |
.github/workflows/supply-chain.yml:410 |
| 2 | 🔵 Advisory | Security | Out of this diff, surfaced while tracing the failure class: the install list in verify-provenance names four packages. @allxsmith/eslint-plugin-bestax is published (dist-tags.latest = 1.0.1) and is in the consumer-SBOM matrix of this same file, so it gets an SBOM but its attestation is never checked — exactly the drift the comment on that step already warns about. Worth its own issue. |
.github/workflows/supply-chain.yml:1043 |
| 3 | 🔵 Advisory | Robustness | Point-in-time evidence in the new comment — "The budget was well under a minute when every release-triggered run began failing at this step" — move to the issue and link it. | .github/workflows/supply-chain.yml:408 |
Overall: The change is sound, and the diagnosis holds up against the logs rather than only against the diff. I confirmed the failure shape in job 106176103562 (@allxsmith/eslint-plugin-bestax@1.0.1): three identical ETARGET / No matching version found inside 31 s (00:23:47 to 00:24:18), then failed three times. The cache diagnosis is right — without --prefer-online npm fetches with cache: default, which honours the max-age on the packument, so all three attempts inside that window could be answered from ~/.npm with no second registry read; --prefer-online is documented as forcing staleness checks even for fresh package data, which is what the comment claims and no more. The loop is correct under the default bash -e -o pipefail: the until condition is exempt from errexit, attempt=$(( attempt + 1 )) is an assignment and so exits 0 (not (( attempt++ )), which would have killed the step on the first pass), and the if yields 0 when the test is false. deadline=$(( SECONDS + 600 )) is captured once, SECONDS is a bash builtin, and the step sets no shell: override. The riskiest part is unavoidable: the syft path beyond this install runs only on release, schedule and workflow_dispatch, so the next release is the only real proof. Send the human to row 2 first — a live gap, unrelated to this PR.
Residual risk:
- The other four matrix legs — refuted. They resolve
latest, which always resolves. I checked the failing-job list on five runs across the window (35547552745,35548900387,35510390330,35052264412,35547462874): in each one, exactly the leg whose version the release names failed, and only that leg. --prefer-onlineguarantees a revalidation request, not a fresh document — the CDN in front of the registry can answer the conditional request with304while the edge still holds the pre-publish packument. Open, but covered by the other half of the change: the 600 s deadline is what actually outlasts the CDN, and the comment says "revalidate against the registry" rather than claiming freshness, so it does not overstate its mechanism.- Tarball not yet served even though the packument is — refuted as a new risk. The
fetch-retriesbuilt into npm handle the transient case, and a hard failure falls out ofnpm installinto this loop, which now has the budget to outlast it. Strictly better than three attempts. - A partial tree from a failed attempt poisoning a later one — refuted.
ETARGETaborts during ideal-tree resolution, before reify writes the lockfile, and the--expectread-back inconsumer-sbom-meta.mjs stampstill fails the leg if the installed version is not the one the release names. - The sibling registry consumer in this file,
verify-provenance— refuted as this failure class. It installs by bare name with no version, so it has nothing to wait on; the one time it failed (2026-09-11) is outside this window and is a different job. The roster gap is row 2, not a propagation race. - Trust boundary — nothing on the exposure axis. No new trigger, no
permissions:change, no new identity, no newallowed-endpointsentry (registry.npmjs.org:443was already allow-listed and is still the only host this step reaches), and nothing new leaves the job. Middle row, workflow logic only. - Gates —
pnpm check:conformanceis green across all 22 rules includingfragile-prose; the added lines are whitespace-clean (no tabs, no trailing space, block-scalar indentation intact); nothing in the tree referenced the removedfailed three timesstring. The--prefer-onlineprecedent already exists here, invisual-regression.ymlvia commit1f2e15d.
🏄 Total mellow set here. The old retry was paddling hard against its own cache and wondering why the wave never came — now it just waits out the registry with a real deadline and a flag that actually re-asks. Clean lines, no drop-ins, good to go.
The sentence explaining why the budget grew carried its reason but not its evidence. Every other comment in this step hands a reader an issue number for the history (#412, #526, #530); this one paraphrased a run list instead. The claim itself stays. It is past tense and stays true: the budget was well under a minute, and the release-triggered runs did begin failing here. What it was missing is the hop.
|
Round 1: one row already fixed elsewhere, one refuted, one that had a real improvement in it. Row 3 — partially taken. The sentence stays, because it is past tense and stays true: the budget was well under a minute, and runs did begin failing at this step. That does not decay the way a count or a run id does, and it is the rationale for the change rather than a measurement of the current tree. What was genuinely missing is the hop to the evidence, which is how the rest of this file works (#412, #526, #530 all sit in comments right here). It now reads Row 1 — refuting the actionable half, accepting the cost. The premise is that a non-propagation failure now waits out the full budget, and for a "version that never published" that is true but unavoidable: it is the same error. A version that will never exist returns exactly what a not-yet-propagated one returns: That is byte-identical in shape to the For the failures that are distinguishable — a registry outage, a connection reset — waiting is the correct response rather than a cost, since those recover. npm's own So the ten-minute delay on a genuinely dead release is real and I am accepting it, not designing it away. The reason it is affordable: Row 2 — already fixed, in #717. That roster gap is hunk 5 of #706, which #709 auto-closed while the hunk was still outstanding. #706 is reopened and #717 closes it. It was deferred rather than missed: Two notes on the verification in the overall, both of which I agree with and want to underline rather than let pass as flattery. The |
|
deep-review: fresh — please read 7d60f84 |
Preview DeploymentPreview URL: https://78e4de72.bestax.pages.dev |
|
deep-review: fresh — please read 7d60f84 |
|
deep-review: fresh — please read 7d60f84 |
There was a problem hiding this comment.
Deep review — 0 blocking · 3 advisory
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🔵 Advisory | Robustness | A failure that retrying can never fix — a version that never published, a broken tarball, a registry outage — now costs the full 600 s on that leg instead of ~31 s. sign-sbom and attach-sbom both needs: this job, and needs waits for completion, not success, so a release's SBOMs now attach up to ten minutes later when any one leg is stuck. Accepted cost of trading a count for a deadline; fail-fast: false keeps the siblings independent and no timeout-minutes is anywhere near. |
.github/workflows/supply-chain.yml:410 |
| 2 | 🔵 Advisory | Correctness | Out of this diff, found while enumerating the same failure class in this file: verify-provenance installs its roster by bare name — no version, no --prefer-online, no retry — on the same release trigger. Right after a publish that is exactly the "CDN-cached packument resolves the PREVIOUS version" hazard the consumer-sbom comment names above the Decide which version to install step, and here it is silent: the job verifies the previous release's attestation and goes green without ever checking the tarball the release is attached to. verify-attestation.mjs budgets [5000, 10000] ms for this same propagation lag — half the budget this PR just declared too small. Worth its own issue. |
.github/workflows/supply-chain.yml:1042 |
| 3 | 🔵 Advisory | Coverage | The retry policy stays untested YAML. The repo's other registry-race handler went the other way — verify-attestation.mjs + lib/fetch-retry.mjs, extracted under rule 9 with a node --test sibling, and its header records that it replaced a shell loop spending "three attempts for exactly this". This loop's first version was wrong across every release run in the window with nothing able to catch it. |
.github/workflows/supply-chain.yml:409 |
Overall: The change is sound, and the diagnosis survives the logs rather than only the diff. I pulled job 106176103562 myself: spec=@allxsmith/eslint-plugin-bestax@1.0.1 / expect=1.0.1 resolved correctly, then three identical ETARGET / No matching version found between 00:23:48 and 00:24:18 — 31 s — and failed three times. I also read the failing-job list on 35548900387, 35547388242 and 35547552745: in each, exactly the leg whose version the release names failed, and only that leg, so the latest legs really are untouched by this path. The cache half of the diagnosis holds up structurally — the runner starts with an empty ~/.npm, attempt 1 populates it, and attempts 2 and 3 land inside the packument's max-age, so no second registry read was guaranteed; --prefer-online forces the staleness check that makes a retry mean anything. The loop is correct under the default bash -e: until's condition is exempt from errexit, attempt=$(( attempt + 1 )) is an assignment and so exits 0 (unlike (( attempt++ )), which would have killed the step on the first pass), a false if with no else yields 0, and deadline is captured once from bash's own SECONDS. I also confirmed the release ordering the whole fix depends on: in every release.config.js the pnpm-publish plugins sit before @semantic-release/github, so the GitHub release really is created after npm publish returns — this is propagation lag, not a pipeline that publishes after it announces. Send the human to row 2 first: it is a live silent pass in this same file, and unrelated to this PR.
Residual risk:
- The other four matrix legs — refuted, on the run data rather than on the argument: only the release-named leg ever entered this path, in every run I checked.
--prefer-onlinebuys a revalidation request, not a fresh document — open, and correctly not claimed otherwise. A CDN can answer the conditional request from the same pre-publish edge copy. The deadline is the half of the change that actually outlasts that, and the comment says "revalidate against the registry" rather than claiming freshness, so it does not overstate its mechanism.- The budget is a guess, not a measurement — open by construction, and named as such in the comment ("generous rather than clever"). No run has ever observed how long this propagation actually takes, because every one gave up at 31 s. The next release is the only instrument.
- A hang rather than a failure — open, and unchanged by this diff. The deadline is only read between attempts, so an
npm installthat never returns is bounded by the job's default timeout, not by the budget. Pre-existing; the three-attempt loop had the same hole. - A partial tree poisoning a later attempt — refuted.
ETARGETaborts during ideal-tree resolution, before reify writes a lockfile, and the--expectread-back inconsumer-sbom-meta.mjs stampstill fails the leg if the installed version is not the one the release names. - The
--prefer-onlineprecedent — a correction for the record, because my own earlier pass on this PR cited it wrongly.1f2e15ddid add the flag tovisual-regression.yml, but it is not there today:3460241fremoved it when that workflow moved to pnpm. It was never reverted for misbehaving, so the precedent still supports the change — it just no longer exists in the tree. - Trust boundary — nothing on the exposure axis. No new trigger, no
permissions:change, no new identity, no newallowed-endpointsentry (registry.npmjs.org:443was already listed and is still the only host this step reaches), and nothing new leaves the job. Middle row, workflow logic only; I1 and I2 are untouched, as the PR body says. - Gates —
Build and Testis green on the head commit, which is what runscheck:conformanceincludingfragile-prose. Nothing in the tree referenced the removedfailed three timesstring. One PR-body imprecision for the maintainer, not worth a row of its own: "the leg gave up about half a minute after the publish" measures 31 s from the step starting, not from the publish — the release event had already fired before the step, and the publish before that, so the real window this failed inside is longer than the body implies. That only makes the new budget look more justified, not less. - The previous pass's advisory on point-in-time evidence — settled by the head commit, which added the
(#716)hop that "evidence goes on the issue and is linked" asks for. The surviving clause is past tense and true, so re-litigating it would be reversing an applied suggestion.
🏄 Total mellow set here. The old retry was paddling hard against its own cache and wondering why the wave never showed — now it re-asks the registry for real and waits the swell out on an honest deadline instead of a countdown nobody measured. Clean lines, no drop-ins, good to go.
Rule 9 of .github/CLAUDE.md puts logic worth testing outside YAML, and this logic earned it the hard way: the shell loop was wrong in two ways at once and stayed wrong on every release-triggered run for over a week, because nothing existed that could fail when it was wrong. That is not incidental to this job. consumer-sbom runs on release, schedule and workflow_dispatch, so no PR event reaches it, and the pinned-install path fires only on a real release. A test sibling is not one more check here, it is the only check. scripts/npm-install-retry.mjs holds the policy with every outside dependency injected, so the budget, the stopping condition and the log can be driven without a registry, a subprocess or a real wait. The assertions are about what the policy spends and when it stops rather than wall-clock time, and one of them fails for any policy that counts attempts instead of reading a clock, which is the regression that started this. Writing them found a real wart: a budget shorter than one interval slept a full interval anyway, to make an attempt whose result the loop had already decided to discard. So a leg that waiting could never fix reported its failure an interval later than it knew it. The stopping condition now refuses a wait that would land past the deadline. Two things the extraction buys beyond testability. The spec reaches npm as an argument array rather than a shell word, so nothing gives it a second reading. And the exhaustion message runs through forLog, which is what that helper's own header asks for: the message is printed as an ::error:: workflow command, a workflow command ends at a newline, and the spec arrives from argv.
|
Round 2: one taken as written, one filed as its own issue, one accepted. Row 3 — taken. This was the strongest finding in either round, and the argument is the same one I used in #709 to justify The policy now lives in Writing them found a real wart, which is the best argument for the finding that I can offer. A budget shorter than one sleep interval slept a full interval anyway, to make an attempt whose result the loop had already decided to discard: a leg that waiting can never fix reported its failure an interval later than it knew. The stopping condition now refuses a wait that would land past the deadline. That was invisible in YAML and fell out of the first test case that set a small budget. Two things the extraction bought beyond testability. The spec reaches npm as an argument array rather than a shell word. And the exhaustion message goes through End to end, as the workflow actually calls it: run from the consumer directory with Row 2 — confirmed and filed as #719. I checked the claim rather than taking it: Worth naming why this one is worse than the bug this PR fixes: #716 was loud, because asking for an exact version and not getting it is an error. Asking for The issue also records the asymmetry you flagged, that It stays out of this PR because it is a behaviour change to the provenance gate, and this PR's job is to stop every release-triggered run going red. Row 1 — accepted, including the part I had not considered. The Head |
|
deep-review: fresh — please read 6fa599f |
|
deep-review: fresh — please read 6fa599f |
|
deep-review: fresh — please read b4384db |
Preview DeploymentPreview URL: https://a7088400.bestax.pages.dev |
There was a problem hiding this comment.
Deep review — 0 blocking · 5 advisory
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🔵 Advisory | Correctness | The focus commit's replacement sentence points at the wrong message. It sits above the success log line, whose text is resolved after ${waited}s and ${attempt} attempts — no budget in it; only the exhaustion message at :325 prints (${waited}s of ${budgetSeconds}s, …). Clause to delete so the remainder is true: the comparison the message makes —. The comparison itself is sound; a reader just has to bring DEFAULT_BUDGET_SECONDS to it on the success path. |
scripts/npm-install-retry.mjs:118 |
| 2 | 🔵 Advisory | Robustness | Carried from all nine earlier passes and re-verified this one: a failure no wait can fix now costs the full 900 s on that leg instead of ~33 s, and sign-sbom (needs: [sbom, consumer-sbom], line 684) plus attach-sbom (line 886) gate on completion, so a release's signatures and assets attach up to 15 minutes later. Bounded — fail-fast: false, no timeout-minutes on the job. Accepted cost of trading a count for a deadline. |
.github/workflows/supply-chain.yml:406 |
| 3 | 🔵 Advisory | Robustness | The budget remains a point estimate against a distribution that has already shifted once, which the commit series itself says. 900 s clears the worst case measured on this PR (bestax-migrate@2.3.4, 732 s) by 168 s, and that measurement is a lower bound on what a runner sees through a CDN edge. The new waited line is the right instrument for the next resize, but it lives only in the run log, so the window is GitHub's log retention. |
scripts/npm-install-retry.mjs:55 |
| 4 | 🔵 Advisory | Coverage | Two production expressions no case executes: the entry guard's process.exitCode = await main() (:336) and the ?? spawnSync fallback in main's installer wiring (:311). Both are deliberate — driving either means spawning a subprocess, which the suite's header bans — and both are backstopped downstream (see residual risk). ?? spawnSync is also redundant with runNpmInstall's own spawn = spawnSync default. Nothing gates coverage on scripts/*.test.mjs either (no --experimental-test-coverage, no threshold), so "the sibling is the only check" is unmeasured. |
scripts/npm-install-retry.mjs:336 |
| 5 | 🔵 Advisory | Correctness | For the maintainer, not the fix agent — the PR body still says "a ten-minute budget costs no time". The budget became 900 s (fifteen minutes) in b785bab8. Neither this review nor the fix agent can edit a PR body. |
PR body, Where the retry policy lives |
Overall: Sound, and the focus commit is a clean close on the one thread it set out to close — I traced both figures it pins against the injected clock rather than taking the message for them (budget 50 / interval 20 → sleeps at 0, 20 000, 40 000 ms, guard trips at attempt 3, waited = 40), so waited: elapsed() → waited: 0 and dropping ${waited}s of from the exhaustion message both die now; my one open thread from the previous pass is replied to and resolved. The prose correction is the right call too: the figure genuinely bounds nothing, because it starts when the script starts rather than at the publish. The riskiest part is unchanged and is not code — it is the 900 s number itself, fitted to a nine-release sample whose worst case it clears by 168 s. Focus there first, since a release is the only instrument that will ever exercise it.
Residual risk:
- Propagation exceeding 900 s still reds the release — open. Nothing in the diff can refute this; it is the residual the budget is. The margin over the worst case measured on this PR is 168 s, and that measurement understates what a runner sees through a CDN edge.
--prefer-onlinedefeats the local cache but not a stale CDN edge — refuted as a new failure mode. It forces the staleness check, which is what the identical-ETARGET-at-every-interval logs showed was missing; edge staleness is only curable by waiting, and waiting is now what the policy does. The flag cannot be silently dropped again:every attempt carries --prefer-online, and the spec comes lastdeep-equals the whole argv off an injectedspawn, andINSTALL_ARGSis separately asserted as data, so a rewritten call site is still caught.--ignore-scriptsrides the same two assertions.- A swallowed exit code going silently green — refuted, three independent backstops.
mainreturns 1 on exhaustion intoprocess.exitCode; the step is a plainrun:underbash -ewith the node call neither last, piped, nor inside anif; a failed install writes nopackage-lock.json, so thecptwo lines down fails; andconsumer-sbom-meta.mjs stamp --expectre-reads the installed version against the release's. - An
npm installthat hangs instead of failing — open, pre-existing, unchanged. The deadline is read only between attempts (stated in the module at:89), andconsumer-sbomsets notimeout-minutes, so the bound is the 360-minute job default. Equally true of the shell loop this replaces. - The
latestlegs now force-revalidate too — refuted as a regression. Areleaseevent can make a non-released leg resolve a newerlatestthan the cached packument would have given it, which is exactly what a consumer SBOM is defined as describing;EXPECTis empty on those legs, so nothing asserts a version and thestamppath is unchanged. - Rule 9's bootstrap gap — refuted.
supply-chain.ymldeclares onlyrelease/schedule/workflow_dispatch, and the checkout takes the event ref, so the first run to readnpm-install-retry.mjsis the first release after merge, by which time it is in the tree the tag points at. - Trust boundary — unchanged, and the PR body's I1/I2 claim checks out. No new trigger, no
permissions:edit, no identity, no checkout of PR code;registry.npmjs.org:443was already inallowed-endpoints(line 172) and the diff adds no host;consumer-sbomholds no model-auth token and runs no model. Nothing new leaves the job.forLogisJSON.stringify(String(...)), so the per-attempt retry line and thecwdin the throw are both newline-neutralised before reaching stdout. - On this pass's own verification: script execution was unavailable in this session and the
Build and Testcheck forb4384db8was stillin_progresswhen this was posted, so the two new assertions were verified by tracing the injected clock by hand rather than by running them. The same suite is green atc19b7613(job106604323833, withthe last wait never lands past the deadlineandan exhausted budget exits 1 and says where to lookboth passing), and the head commit only adds assertions to those two cases.
🏄 Eleven commits chasing one number down the beach and it finally sits still — the figure says what it is, the exhausted leg reports what it actually spent, and the last free-surfing mutant got wiped out. All that's left is a stale minute-count in the PR body and a budget that's a judgement call, not a bug. Send it.
The sentence sat above the success log line and described it as printing the spend against the budget. It prints the spend alone. Only the exhaustion message prints both, and the comment now says which is which. Fifth correction to this comment block, against one defect in the policy it describes, so the pattern is worth naming in the history: the prose around this change has been considerably less reliable than the change. Also drops a redundant fallback. main passed `spawn ?? spawnSync` into runNpmInstall, which already defaults the same parameter to the same value. Two fallbacks for one thing only make a reader work out which one wins.
|
Round 11: three taken, two carried, and I think this is the point to stop. Row 1 — taken, fifth correction to this comment block. The sentence sat above the success line and credited it with printing the spend against the budget. It prints the spend alone; the exhaustion message is the one that prints both. Fixed by deletion again, with the distinction named. Row 4 — taken, the redundancy half. On the two genuinely uncovered expressions: both stay uncovered on purpose. Driving Row 5 — taken. The body said "a ten-minute budget costs no time" after the budget became 900 s. Rephrased so it cannot decay with the number again. I could edit it, so it is done rather than handed over. Rows 2 and 3 — carried, both honest limits rather than defects. The 900 s point estimate against a distribution that shifted once, and the stuck-leg delay now being 900 s rather than the 600 s I first accepted it at. Why I am calling it here rather than asking for round twelve. The last four passes found no defect in the retry policy. Every finding was coverage of something I had just added, or a sentence I had written wrong: five prose corrections against one real policy defect across the whole review. That is a useful ratio to know and a bad one to keep paying for, because the marginal round is now mostly auditing my own descriptions. Against that, the bug is still live. So: policy mutation-verified from eight directions, 27 cases, Head |
Preview DeploymentPreview URL: https://9cdb47d6.bestax.pages.dev |
|
deep-review: fresh — please read bb02bef |
There was a problem hiding this comment.
Deep review — 0 blocking · 4 advisory
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🔵 Advisory | Correctness | The waited instrument has a third skew the comment does not name, and it is on the upper side: elapsed() is read after await run(spec) returns, so a successful attempt's own resolve/reify time is inside the figure. On the bestax-migrate leg that is a 98-package closure. The comment names the start-side skew (script start, not publish) and the one-interval granularity; this one inflates rather than truncates. Safe direction — it biases the next resize larger — but it is the instrument #716 built for exactly that resize. Nothing to change in the file: the honest fix is a qualifier, and adding one is the move the house rule forbids. |
scripts/npm-install-retry.mjs:125 |
| 2 | 🔵 Advisory | Robustness | Carried from all ten earlier passes and re-verified here rather than restated: a failure no wait can fix costs the full 900 s on that leg instead of ~31 s, and both sign-sbom (needs: [sbom, consumer-sbom]) and attach-sbom (needs: [sbom, consumer-sbom, sign-sbom]) gate on completion, not success — so a release's signatures and assets attach up to fifteen minutes later. Bounded by fail-fast: false and by nothing here being able to fail a release. Accepted cost of trading a count for a deadline. |
.github/workflows/supply-chain.yml:406 |
| 3 | 🔵 Advisory | Coverage | Two production expressions the suite still does not execute, both deliberately: the entry guard process.exitCode = await main(), and runNpmInstall's spawn = spawnSync default — driving either means spawning a real subprocess, which the suite header bans. bb02bef6 improved this rather than worsening it by deleting the duplicate ?? spawnSync in main, so there is now exactly one fallback and undefined still reaches the default parameter correctly. Coverage on scripts/ remains ungated (no threshold, no --experimental-test-coverage), which is #724. |
scripts/npm-install-retry.mjs:336 |
| 4 | 🔵 Advisory | Robustness | Point-in-time evidence and one stale attribution in the added prose — one row for the lot, per the house rule: "on every release-triggered run for over a week" (twice), "usually under a couple of minutes but was once an order of magnitude worse", and # New in #530: this job runs scripts from scripts/ — the install policy, …, which now attributes the install policy to #530 when it arrives with #716. The (#716) hop that the rule actually asks for is already there in both places, so this is the residue, not the omission. |
.github/workflows/supply-chain.yml:221 |
Overall: Sound, and the evidence holds up under independent checking rather than on the PR body's word — every one of the last ten release-triggered supply-chain runs failed, each on exactly the leg its tag names (run 35547552745 → Consumer SBOM (@allxsmith/eslint-plugin-bestax); run 35682756557 → the bestax-bulma leg), while the last five schedule runs are all green, and the failing install step ran 00:23:47 → 00:24:18 — thirty-one seconds, which is the 10+20 sleep budget plus three fast attempts and nothing else. The riskiest part is still not code: it is the 900 s number, fitted to a nine-release sample whose worst case it clears by 168 s, against a distribution the commit series itself says has already shifted once. Focus there first, because a real release is the only instrument that will ever exercise it. The focus commit bb02bef6 is clean on both halves — the success line genuinely prints the spend without the budget, only the exhaustion message prints both, and dropping spawn ?? spawnSync is behaviour-preserving because passing undefined is what triggers runNpmInstall's own default.
Residual risk:
- Propagation exceeding 900 s still reds the release — open. This is the residual the budget is; nothing in the diff can refute it. The margin over the worst case measured on this PR is 168 s, and that measurement is a lower bound on what a runner sees through a CDN edge.
- A 15-minute wait sits under harden-runner's periodic DNS refresh instead of one window — open, new with this change, low likelihood. Rule 10 of
.github/CLAUDE.mdrecords that the agent re-resolves allow-listed hosts every 30 s and on failure only logsfailed to insert new ipaddress in firewall. The loop this replaces finished inside a single refresh window; this one can sit under the firewall for the whole budget, so aregistry.npmjs.orgIP rotation with a failed refresh would fail every remaining attempt and be reported by the exhaustion message as propagation. The diagnosis is at least in the agent log, and the endpoint is unchanged — no allowlist widens. --prefer-onlinedefeats the local cache but not a stale CDN edge — refuted as a new failure mode. It forces the staleness check that the identical-ETARGET-at-every-interval logs showed was missing; edge staleness is only curable by waiting, and waiting is what the policy now does. The flag cannot be silently dropped again: the argv is deep-equalled off an injectedspawnandINSTALL_ARGSis separately asserted as data, so a rewritten call site is still caught.--ignore-scriptsrides the same two assertions, which matters more than the retry does.- A failed install going silently green — refuted, four independent backstops.
mainreturns 1 intoprocess.exitCode; the step is a plainrun:underbash -ewith thenodecall neither last, piped, nor inside anif; a failed install writes nopackage-lock.json, so thecptwo lines down fails; andconsumer-sbom-meta.mjs stamp --expectre-reads the installed version against the release's. - An
npm installthat hangs rather than failing — open, pre-existing, unchanged. The deadline is read only between attempts, stated in the module itself, andconsumer-sbomsets notimeout-minutes, so the bound is the 360-minute job default. Equally true of the shell loop. - Release bursts holding runners longer — open, bounded.
supply-chain.ymldeclares noconcurrency:group, and semantic-release fires releases in bursts (five runs inside four minutes on 2026-09-21, 00:19–00:23). Each burst can now hold one long-lived job per run for up to fifteen minutes where it previously held one for thirty-one seconds. Bounded: only the pinned leg can reach the deadline, the siblings resolvelateston the first attempt, and nothing here can fail a release. - Rule 9's bootstrap gap — refuted.
supply-chain.ymldeclares onlyrelease/schedule/workflow_dispatch, and the checkout takes the event ref deliberately, so the first run to readnpm-install-retry.mjsis the first release after merge, by which time it is in the tree the tag points at. - Trust boundary — unchanged, and the PR body's I1/I2 claim checks out against the default branch's
.github/CLAUDE.md. No new trigger, nopermissions:edit, no new identity, no checkout of PR code, no new third-party action.registry.npmjs.org:443was already inallowed-endpointsand the diff adds no host, so nothing widens who can trigger the job, what it can reach, or what leaves it.consumer-sbomholds no model-auth token and runs no model. Both places the spec is printed go throughforLog(JSON.stringify(String(…))), so a crafted spec cannot terminate a workflow command, and the spec reaches npm as an argv element rather than a shell word. - On this pass's own verification:
nodeandpnpmexecution were unavailable in this session, so the suite was read rather than run — butBuild and Testis green atbb02bef6, and rootpnpm testisturbo run test && node --test "scripts/*.test.mjs", so the new file ran there. I additionally hand-traced the two arithmetic cases against the injected clock (budget 50 / interval 20 → sleeps at 0 and 20 000 ms, guard trips at attempt 3,waited = 40; budget 600 / interval 20 → 31 attempts) and both match their assertions.
🏄 Twelve commits deep and the swell finally cleaned up — the retry actually re-asks the registry, the clock does the stopping, and the last comment that was claiming a wave it never caught got trimmed back to the truth. Only thing still floating out there is the 900 you can't test from shore. Paddle it out.
Deleting the stale count from this comment left the sentence attributing the install policy to #530, which introduced the other two scripts. The policy arrives here. Each is credited to what added it now.
|
Round 12: one actionable, and it was mine. Row 4 — the attribution, fixed. When I deleted the stale count from that comment in an earlier round, I left the sentence crediting the install policy to #530, which introduced the other two scripts. The policy arrives with this PR. Each is credited to what added it now. That one only surfaces from reading the sentence against the history rather than against the code, so thanks for doing that. The point-in-time phrases in the same row I am leaving: the Row 1 — agreed, and agreed about not fixing it. A third skew in the same figure, this one inflating: Rows 2 and 3 — carried, and both accurate. The stuck-leg delay at 900s rather than the 600s I first accepted it at, and the two deliberately uncovered expressions. Worth acknowledging your note that Head On the evidence you re-derived independently: thirty-one seconds being the |
|
deep-review: fresh — please read 81407fd |
Preview DeploymentPreview URL: https://ebba862d.bestax.pages.dev |
There was a problem hiding this comment.
Deep review — 0 blocking · 4 advisory
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🔵 Advisory | Security | Two of the ::error::-bound strings this module can emit interpolate argv raw — ${key} needs a value and unknown flag --${name} — while their neighbour a line above uses forLog(key). Unreachable from the workflow (the step hardcodes --spec/--dir, so even-index argv is always a literal and only values can be crafted), so there is no failure scenario today; noted only because the suite’s own case is named "the spec is neutralised in both places it is printed", and these are the two places a hand-run argv could still forge a workflow command. |
scripts/npm-install-retry.mjs:215 |
| 2 | 🔵 Advisory | Robustness | Carried and re-verified rather than restated: a failure no wait can fix costs the full 900 s on that leg instead of ~31 s, and both sign-sbom and attach-sbom gate on consumer-sbom completing, not succeeding — so a release’s signatures and assets attach up to fifteen minutes later. Bounded by fail-fast: false and by nothing here being able to fail a release. |
.github/workflows/supply-chain.yml:406 |
| 3 | 🔵 Advisory | Coverage | Measured this pass rather than inferred: node --test --experimental-test-coverage reports 99.41 % lines, 95.83 % branches, 100 % funcs, with only the entry guard unexecuted. The one residual uncovered branch is runNpmInstall’s spawn = spawnSync default, deliberately unreachable from a suite that bans real subprocesses. Coverage on scripts/ remains ungated (#724). |
scripts/npm-install-retry.mjs:336 |
| 4 | 🔵 Advisory | Robustness | Point-in-time evidence in the added prose — one row for the lot, per the house rule: "on every release-triggered run for over a week" (twice) and "usually under a couple of minutes but was once an order of magnitude worse". The (#716) hop the rule asks for is present in both places and resolves — the nine-release table is in the round-7 comment here — so this is residue, not an omission. The focus commit’s re-attribution checks out: the job does now call three scripts (consumer-sbom-meta, npm-install-retry, check-consumer-sbom), and the install policy is genuinely not one of #530’s two. |
scripts/npm-install-retry.mjs:39 |
Overall: Sound, and this pass could execute rather than read. The suite runs green (27/27) and the failure it was written against is real in exactly the shape claimed: job 106603059044 on run 35682756557 shows the step starting at 03:20:21.98, ETARGET / No matching version found for @allxsmith/bestax-bulma@5.16.6 three times, and the leg dead at 03:20:54.55 — thirty-three seconds. The cache diagnosis holds up in the timings too: attempt 1 took ~1.95 s and attempts 2 and 3 ~0.29 s each, which is a ~/.npm packument hit rather than a registry round trip, so --prefer-online is addressing the thing that actually happened. The riskiest part is still not code — it is the 900 s constant, fitted to nine observations whose worst is 732 s — and the one thing a human should weigh is whether that outlier was tail behaviour or an incident, because the answer decides whether every doomed leg should now wait fifteen minutes.
Residual risk:
- Propagation exceeding 900 s still reds the release — open. This is the residual the budget is; the diff cannot refute it. The self-measuring
resolved after Ns and N attemptsline makes the next resize readable off the runs instead of the registry API, which is the right answer to a constant nothing here can fit. --prefer-onlinedefeats the local cache but not a stale CDN edge — refuted as a new failure mode. The identicalETARGETat each interval plus the 1.95 s to 0.29 s timing collapse is the local-cache signature, and that is what the flag closes; edge staleness is only curable by waiting, which the deadline now does. The flag cannot be silently dropped again:INSTALL_ARGSis deep-equalled off an injectedspawnand asserted separately as data, and I confirmed each half fails independently of the other.- A silently-green failed install — refuted, four backstops, each checked.
mainreturns 1 intoprocess.exitCode; thenodecall sits underbash -e, neither last nor piped nor inside anif; a failed install writes nopackage-lock.json, so thecptwo lines down fails; andconsumer-sbom-meta.mjs stamp --expectre-reads the installed version. - An empty
$SPECquietly installing nothing — refuted, and improved by this change.npm install --ignore-scripts ""would have installed the consumer manifest’s (empty) deps and exited 0;parseArgsnow rejects it with--spec is requiredand exit 2. - An
npm installthat hangs rather than failing — open, pre-existing, unchanged. The deadline is read only between attempts (stated in the module), andconsumer-sbomsets notimeout-minutes, so the bound is the 360-minute job default. Worst case is deadline plus one attempt, not deadline times attempts. - A 15-minute wait spanning harden-runner’s 30 s DNS refresh — open, new with this change, low likelihood. Rule 10 records that a failed re-resolve only logs; a
registry.npmjs.orgrotation mid-budget would be reported by the exhaustion message as propagation. No allowlist widens, and the diagnosis is at least in the agent log. - Rule 9’s bootstrap gap — refuted. The workflow declares only
release/schedule/workflow_dispatch, the checkout takes the event ref deliberately, and the first run to readnpm-install-retry.mjsis the first release after merge, by which time the tag’s tree carries it. The newforLogimport is side-effect-free —consumer-sbom-meta.mjskeeps its CLI behind animport.meta.urlguard — so nothing extra runs at module load. - The same race in a sibling job — open, out of scope, already filed.
verify-provenanceinstalls its roster by bare name on the samereleasetrigger (#719). Not in this diff. - Trust boundary — unchanged, verified against the default branch’s
.github/CLAUDE.md. No new trigger, nopermissions:edit, no new identity, no new action, no checkout of PR code;registry.npmjs.org:443was already allow-listed and the diff adds no host.consumer-sbomholds no model-auth token and runs no model. The spec reaches npm as an argv element rather than a shell word, which is strictly narrower than the quoted$SPECit replaces. - Repo gates — run, not assumed.
node --test scripts/npm-install-retry.test.mjs27/27;check-conformance.mjs22/22 includingfragile-prose;prettier --checkandeslintclean on all three changed files.
🏄 Thirteen commits and the set finally lined up — the loop re-asks the registry for real, the clock calls the session, and the last comment that was giving #530 credit for someone else’s wave got handed back. Nothing left out there but that 900 you can only read from the lineup. Send it.
|
Round 13: nothing actionable. Refuting row 1 and carrying the rest, so Row 1 — refuted, on trust class rather than on reachability. Your own note that it is unreachable is correct, but the reason not to change it is stronger than "unreachable today", because unreachable-today is exactly the argument that ages badly. These two strings interpolate a flag key, and keys and values come from different places. The workflow passes Adding On the test name: "in both places it is printed" is scoped to the spec, which is printed in exactly two places (the retry line and the exhaustion message) and is neutralised in both. It does not claim every argv-bearing string is neutralised, so the name is accurate for what the case covers. Row 3 — no action, and the number is a good result worth recording. 99.41% lines, 95.83% branches, 100% funcs, with only the entry guard unexecuted and the one residual branch being Row 4 — accepted as residue, no action. You confirmed the Row 2 — carried, accepted, unchanged for the eleventh time. So this is the convergence point, and this time the head I am claiming it for is the head that was reviewed. |
The budget stopped being a guess. Across the releases preceding this change, the gap between a release going up and the registry writing the new version into the packument is usually under a couple of minutes, and was once an order of magnitude worse. The previous value cleared the typical case and would have expired short of the worst one, which is the false red this whole change exists to prevent. The figures and how they were taken are on #716, because measurements belong on the issue rather than in a comment that nothing re-measures. It is set to clear the worst observed case with margin, on the reasoning that the two mistakes are not symmetric: a generous budget spends extra minutes on a release that was already broken, and a short one reds a release that was fine. The default sleep was the last function nothing could call. As an inline default, dropping its argument left every case green, because every other case injects its own sleep, while turning a few dozen packument reads into a few thousand inside the same budget. Exported and timed now. A retryable spawn failure threw its errno away. stdio inherit carries npm's own diagnosis when npm runs, but a spawn that never started produced nothing to inherit, so EAGAIN gave a column of bare retry lines under a propagation diagnosis that does not fit it. It says the code out loud. And a usage-error case let main fall through to console.log, so two ::error:: workflow commands reached stdout during pnpm test. On a runner those are filed as failure annotations against a green job. Captured and asserted here; the same leak in four other script suites is #723.
|
🎉 This PR is included in version 5.16.7 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 4.2.10 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 2.3.8 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.2.7 🎉 The release is available on: Your semantic-release bot 📦🚀 |
1 participant
Every
release-triggeredsupply-chainrun has failed sincebestax-migrate@2.3.2, each time on the one matrix leg whose version the release names. The scheduled runs pass, which is why this stayed quiet.What was actually wrong
Not the decision logic. On the eslint-plugin release the step resolved
spec=@allxsmith/eslint-plugin-bestax@1.0.1andexpect=1.0.1correctly, then everynpm installreturnedETARGET / No matching version foundand the leg gave up about half a minute after the publish. The step's own comment already named this hazard ("asking for an exact version seconds afternpm publishcan 404 while the packument propagates"); what it had was a retry too small to survive it.Two separate defects:
npm was not being re-asked. The logs show the identical
ETARGETat each interval. npm caches the packument it fetched on the way in, so each attempt could re-read the same pre-publish document out of~/.npm. Sleeping between attempts that all consult one cached document only proves the cache agrees with itself, so--prefer-onlineis what makes a retry mean anything here.The budget was measured against the wrong clock. The
releaseevent fires when the GitHub release is created; the version becomes installable when the registry finishes serving it. Nothing in this job can hurry that, so a fixed attempt count is the wrong shape. It is now a deadline.Scope
Only the released leg ever reached this path. The legs no release names resolve
latest, which always had something to resolve, so they succeed on the first attempt and never approach the deadline.Per
.github/CLAUDE.mdMiddle row: workflow logic. I1 is untouched, since this job holds no model-auth token and runs no model. I2 is untouched: no new trigger, no change to
permissions:, no new identity, and nothing new leaves the job. The only egress isregistry.npmjs.org, already allow-listed, so no allowlist widens. The change is confined to one step's retry policy.Where the retry policy lives
It started as a shell loop in the step. It is now
scripts/npm-install-retry.mjswith anode --testsibling, per rule 9 of.github/CLAUDE.md, and the reason is specific to this job rather than general tidiness:consumer-sbomruns onrelease,scheduleandworkflow_dispatch, so no PR event reaches it, and the pinned path fires only on a real release. A test sibling is not one more check here, it is the only check. The version this replaces had none, which is how it stayed broken on every release-triggered run for over a week.Every outside dependency is injected — runner, clock, sleep, log — so the suite reaches neither the network nor a subprocess, and the budget costs no time however large it is set.
Verification
--prefer-onlinefrom the exported list, dropping it at the call site, replacing the deadline with an attempt count, removing the overshoot guard, removingforLogfrom the retry line, making every spawn error fatal, and accepting a zero budget.--dir, it exits 0, installs the pinned version, and leavespackage-lock.jsonwhere thecpon the next line expects it.--prefer-onlineconfirmed a real npm config key (npm config get prefer-online), not an accepted-and-ignored flag.ETARGET / No matching version foundas one that has not propagated, so no fail-fast heuristic can separate them. That is why this waits rather than guessing, and why the exhaustion message names the release step.pnpm allgreen; conformance 22/22 includingfragile-prose; prettier and eslint clean; the rootnode --test "scripts/*.test.mjs"glob picks the new file up with no wiring.The syft path beyond the install still only runs on
release/schedule/workflow_dispatch, so the next release is still the only instrument for the propagation budget itself.Found while reviewing this, filed separately
#719 —
verify-provenanceinstalls its roster by bare name on the samereleasetrigger, with no pin and no read-back, so a stale packument means it verifies the previous release's attestation and goes green. Worse than the bug this PR fixes, because asking for an exact version and failing is loud whilelatestalways resolves to something. Out of scope here: it is a behaviour change to the provenance gate, and this PR's job is to stop every release-triggered run going red.Summary by CodeRabbit
Bug Fixes
Tests