ci(release): fail closed when npm pack reports no tarball; pin the privileged npm upgrade - #57
ci(release): fail closed when npm pack reports no tarball; pin the privileged npm upgrade#57yakimoto wants to merge 2 commits into
Conversation
The e2e-smoke gate could pass without ever exercising the tarball it exists to
exercise:
TARBALL="$(npm pack --silent | tail -n1)"
TARBALL="$PWD/$TARBALL"
npm install --no-save "$TARBALL"
Some npm versions suppress the emitted filename on stdout under --silent. When
that happens TARBALL is empty, `$PWD/` is a DIRECTORY, and `npm install`
happily installs the workspace directory instead. Every assertion after it
then passes -- against the working tree, not against the packed artifact. The
gate goes green having proved nothing about what would actually ship.
That is the same fail-open shape as a swallowed error: the check does not
fail, it stops being a check.
Replaced with `npm pack --json`, whose filename field is not
verbosity-dependent, plus two assertions -- the value ends in .tgz, and the
file exists on disk.
Verified both directions rather than just the happy path:
happy path PACK OK -> wave-av-adk-1.0.2.tgz exit 0
empty TARBALL ::error::npm pack --json reported '', not
a .tgz filename exit 1
A guard that has never been observed rejecting anything is not yet a guard.
Credit where it is due: a peer found this in the mcp-server copy of the same
workflow (bb1bf17) and fixed it there. I had written the --silent version and
copied it across repos, so this propagates their fix to the two that still
carry it.
Also pins the privileged publish job's npm upgrade: `npm install -g npm@latest`
-> `npm@11.19.0`. That job holds `id-token: write`; it should not execute
whatever npm publishes next. adk and mcp-server were pinned by a peer already,
which left this file the only unpinned one of the three.
|
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_ded500cc-84b0-4edb-92d8-022ea736f018) |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 7 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
|
@coderabbitai review Context for the review: same fail-open fix as wave-av/adk#69 — |
|
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 39 minutes. |
The gate that proves the CLI can do trusted publishing passed for a version
string it could not parse at all.
const s = 'banana'.split('.').map(Number) // [NaN]
s[i] > min[i] // false
s[i] < min[i] // false
Both comparisons are false against NaN, so neither branch was taken, the loop
ran out, and node exited 0. Measured on the shipped expression:
11.19.0 -> 0 11.5.1 -> 0 11.5.0 -> 1 9.9.9 -> 1
banana -> 0 <-- reported "capable" having compared nothing
11 -> 0 <-- s[1], s[2] undefined -> NaN -> same fall-through
Reachable whenever `npm --version` yields anything but a bare x.y.z: a shim or
proxy printing a banner, npm emitting a second stdout line so NPM_VER is
multi-line, a leading segment carrying a suffix. The step then goes on to
publish on a CLI that may have no trusted-publishing support, which is the one
thing it exists to rule out.
Now the shape is validated before anything is compared, and an unparseable
version is a hard failure rather than a silent pass. The threshold is untouched:
every well-formed version keeps its previous verdict, boundary 11.5.1 included.
11.19.0 -> 0 11.5.1 -> 0 12.0.0 -> 0 11.19.0-pre.0 -> 0
11.5.0 -> 1 9.9.9 -> 1 10.9.9 -> 1
banana -> 1 "" -> 1 "11" -> 1 1e400.0.0 -> 1
"11.5.1\nextra-line" -> 1
The version also stops being spliced into the program text and is passed
through the environment instead. It is the output of an external command, so
reading it from process.env removes the shell-quoting surface rather than
narrowing it.
actionlint clean (SC2016 suppressed with the reason: the single quotes hold a JS
program, and ${...} inside it are template literals, not shell expansions).
YAML parses.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_6378dec6-bcfc-4a8f-8309-d07e7a38793e) |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
ApprovabilityVerdict: Approved 14b94f2 CI workflow hardening that makes release validation fail-closed instead of silently passing. Changes are limited to You can customize Macroscope's approvability policy. Learn more. |
1 participant
The e2e-smoke gate could pass without ever exercising the tarball it exists to
exercise:
TARBALL="$(npm pack --silent | tail -n1)"
TARBALL="$PWD/$TARBALL"
npm install --no-save "$TARBALL"
Some npm versions suppress the emitted filename on stdout under --silent. When
that happens TARBALL is empty,
$PWD/is a DIRECTORY, andnpm installhappily installs the workspace directory instead. Every assertion after it
then passes -- against the working tree, not against the packed artifact. The
gate goes green having proved nothing about what would actually ship.
That is the same fail-open shape as a swallowed error: the check does not
fail, it stops being a check.
Replaced with
npm pack --json, whose filename field is notverbosity-dependent, plus two assertions -- the value ends in .tgz, and the
file exists on disk.
Verified both directions rather than just the happy path:
happy path PACK OK -> wave-av-adk-1.0.2.tgz exit 0
empty TARBALL ::error::npm pack --json reported '', not
a .tgz filename exit 1
A guard that has never been observed rejecting anything is not yet a guard.
Credit where it is due: a peer found this in the mcp-server copy of the same
workflow (bb1bf17) and fixed it there. I had written the --silent version and
copied it across repos, so this propagates their fix to the two that still
carry it.
Also pins the privileged publish job's npm upgrade:
npm install -g npm@latest->
npm@11.19.0. That job holdsid-token: write; it should not executewhatever npm publishes next. adk and mcp-server were pinned by a peer already,
which left this file the only unpinned one of the three.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is enabled.Note
Low Risk
Changes only affect CI release gates; they tighten validation and pin a known npm CLI without altering application runtime code or publish semantics beyond safer pre-publish checks.
Overview
Release workflow (
release.yml) fixes two ways the verify/publish gates could pass without actually validating what ships.The e2e-smoke step no longer relies on
npm pack --silent(empty stdout on some npm versions could makenpm installuse the workspace directory). It usesnpm pack --json, parses the tarball filename, requires a.tgzsuffix, and checks the file exists before install/import smoke tests.The publish job pins the global npm upgrade to
npm@11.19.0instead of@lateston a job withid-token: write, and replaces the inline version check with an env-driven script that fails closed whennpm --versionis not parseable asx.y.z(the old>/<loop could treat NaN as success).Reviewed by Cursor Bugbot for commit 14b94f2. Configure here.
Summary by cubic
Harden the release workflow: the e2e-smoke gate installs the real tarball using
npm pack --jsonand fails closed if no.tgzexists; the publish job pinsnpm@11.19.0and strictly validates the CLI version before trusted publishing.Bug Fixes
npm pack --json, parse the filename, assert it ends with.tgz, and confirm the file exists to avoid installing the workspace directory.npm --versionvia env: require a parseablex.y.zwith integers and fail if unparseable or< 11.5.1so the trusted-publishing check cannot pass silently.Dependencies
npmin the trusted-publishing job tonpm@11.19.0, avoiding unreviewed upgrades on a job withid-token: write.Written for commit 14b94f2. Summary will update on new commits.
Note
Fail closed on missing tarball and pin npm version in release CI
npm pack --silent | tail -n1withnpm pack --jsonand a Node one-liner to extract the tarball filename reliably; validates the.tgzextension and file existence, emitting::error::and exiting 1 on failure.npm@11.19.0instead oflatest, and replaces the inline version check with a stricter Node script that parses the version via regex and fails closed if it is unparsable or below 11.5.1.Macroscope summarized 14b94f2.