chore(ci): drop floating npm@latest from publish; assert the Trusted-Publishing npm floor - #81
Merged
Merged
Conversation
…Publishing npm floor
publish.yml ran `npm install -g npm@latest` inside the repo's highest-trust job
— OIDC Trusted Publishing, `id-token: write`, the `npm-publish` environment —
so whatever npm shipped that day executed the release.
Two facts made the fix "delete", not "pin".
1. The install is unnecessary. Its own comment gives the only reason
("Trusted Publishing requires npm >= 11.5.1"), but the digest-pinned
setup-node with node-version: 24 resolves to the latest released v24.x, and
that release — v24.19.0 as of 2026-08-10 — bundles npm 11.17.0, verified
against the tag itself rather than the branch head. npm's own docs confirm
the floor: "Trusted publishing requires npm CLI version 11.5.1 or later and
Node version 22.14.0 or higher." The publish job can run zero publish-time
tool downloads.
2. The floating step had already crossed a major, silently. The registry
`latest` dist-tag is npm 12.0.2 today, so every release since npm 12 shipped
published with a major the workflow was never written against — the live
demonstration of why @latest rots in a release path.
The step is replaced by an "Assert npm floor" step: pure node stdlib, no
packages, no network, a three-part integer semver compare that exits non-zero
naming both versions. The floor is now enforced instead of installed, and the
release job downloads no third-party tool at all.
Fail-closed rests on the PARSE, not the shell. `set -euo pipefail` does NOT
abort on a command substitution used as an argument (reproduced:
`bash -c 'set -euo pipefail; echo "[$(nonexistent)]"; echo REACHED'` prints
both and exits 0), so a failing `npm --version` arrives as "" and exits 1 as an
unparseable version.
Also adds .dev/floor/check-run-pins.mjs so this cannot silently regress.
check-action-pins scans `uses:` refs only; a `run:` line pulling @latest was
outside its contract. The new gate classifies every package spec in a
recognised install/exec invocation across workflows and local composite
actions, requiring an exact semver — enum reasons floating-version /
unpinned-package / unpinnable-version / unreadable-file. It is wired with no
workflow change: floor.yml's existing `node --test ".dev/**/*.test.mjs"` glob
collects its live repo-consistency test on every PR.
Proof of record, since publish.yml only fires on `release: published` and no
gate in this repo can execute it: the assert program was extracted from the
shipped publish.yml bytes and run against 11.5.0 (exit 1), 11.5.1 (exit 0,
inclusive), 11.17.0 (0), 12.0.2 (0), "" (1) and "garbage" (1); the whole step
body also executes clean against real npm. Execution proof is the next release.
Named residual, not fixed here: node-version: 24 floats within the major, so
the bundled npm floats across Node-vetted 11.x releases. The assert catches any
dip below the floor; pinning node exactly is the node-version-policy follow-up
(third mention — it has earned its own ticket).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 50 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe publish workflow now validates the installed npm version instead of installing ChangesPublish npm floor enforcement
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ReleaseEvent
participant PublishWorkflow
participant Npm
ReleaseEvent->>PublishWorkflow: start published-release job
PublishWorkflow->>Npm: run npm --version
Npm-->>PublishWorkflow: return installed version
PublishWorkflow->>PublishWorkflow: enforce npm >= 11.5.1
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
.dev/floor/check-run-pins.test.mjs (1)
46-48: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winScope cleanup to the directories this run created.
cleanupdeletes everypharn-runpins-*directory in the sharedtmpdir(). If two runs of this file overlap on one machine, for example a CI matrix or a local watch run, one run deletes the other run's scratch directories while they are still in use. Track the created paths and remove only those.♻️ Proposed refactor
+const CREATED = []; function scratch() { - return mkdtempSync(join(tmpdir(), "pharn-runpins-")); + const dir = mkdtempSync(join(tmpdir(), "pharn-runpins-")); + CREATED.push(dir); + return dir; }test("cleanup", () => { - for (const d of readdirSync(tmpdir())) { - if (d.startsWith("pharn-runpins-")) rmSync(join(tmpdir(), d), { recursive: true, force: true }); - } + for (const d of CREATED) rmSync(d, { recursive: true, force: true }); });
readdirSyncstays in use for the live workflow recount at line 358.Also applies to: 428-432
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.dev/floor/check-run-pins.test.mjs around lines 46 - 48, Update the scratch-directory lifecycle around scratch() and cleanup so each run tracks the paths returned by mkdtempSync, and cleanup removes only those tracked directories instead of scanning and deleting every pharn-runpins-* entry under tmpdir(). Preserve the existing readdirSync usage for live workflow recounting.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.dev/features/publish-npm-floor-assert/GRILL.md:
- Line 148: Update the sentence in GRILL.md so the issue references `#15` and `#79`
are wrapped in inline code spans, preventing markdownlint from interpreting the
line as an ATX heading while preserving the existing wording.
In @.dev/features/publish-npm-floor-assert/PLAN.md:
- Around line 45-88: The plan’s “literal diff” and related intent sections are
stale; update them to match the shipped workflow and checker. In Part 1, copy
the current publish.yml comment block wording, including the check-run-pins/R1
and fail-closed-parse paragraphs and revised ASSERTS sentence; change the
output-field reference from spec to ref; and revise the residual list to include
R1–R6 plus the YAML-fidelity bound, using the names emitted by
check-run-pins.mjs.
In @.dev/features/publish-npm-floor-assert/SHIP.md:
- Around line 37-45: Update the post-GATE-2 disposition in SHIP.md to list the
floor-tests repair as the third repaired REVIEW finding, then classify the
PLAN.md determinism correction separately as a GRILL finding. Keep the existing
workflow-comment and R5 entries, and align the wording with REVIEW.md’s P1/P2/P3
status.
In @.dev/features/publish-npm-floor-assert/VERIFY.md:
- Around line 65-67: Update the workflow validation test around the existing
presence-gate check to parse publish.yml structurally, then verify the npm-floor
assertion is within the release job, includes the 11.5.1 comparison, and appears
before the npm publish step. Replace the current independent regex checks with
these ordering and job-scope validations.
In @.dev/floor/check-run-pins.mjs:
- Around line 310-342: Update the per-file scanning flow around parseLine to
join shell continuation lines before parsing, removing the trailing backslash
and preserving the first physical line number for any findings or diagnostics.
Ensure commands such as a continued npm install are passed to parseLine as one
command so later package arguments are classified, and document or report the
continuation residual if the scanner remains strictly line-based.
In @.dev/floor/check-run-pins.test.mjs:
- Around line 317-328: Update the symlinked action.yml test to assert the
sibling gate’s expected status directly, rather than deriving it from
sibling.violations. Preserve the existing assertions for mine and the comparison
of violation lists so the sibling verdict is explicitly pinned.
---
Nitpick comments:
In @.dev/floor/check-run-pins.test.mjs:
- Around line 46-48: Update the scratch-directory lifecycle around scratch() and
cleanup so each run tracks the paths returned by mkdtempSync, and cleanup
removes only those tracked directories instead of scanning and deleting every
pharn-runpins-* entry under tmpdir(). Preserve the existing readdirSync usage
for live workflow recounting.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 776e5cbb-bee3-4031-8f2a-da49f676dfbf
📒 Files selected for processing (14)
.dev/features/publish-npm-floor-assert/GRILL.md.dev/features/publish-npm-floor-assert/PLAN.md.dev/features/publish-npm-floor-assert/REGRESSION.md.dev/features/publish-npm-floor-assert/REVIEW.md.dev/features/publish-npm-floor-assert/SHIP.md.dev/features/publish-npm-floor-assert/VERIFY.md.dev/features/publish-npm-floor-assert/regression-report.json.dev/features/publish-npm-floor-assert/verify-report.json.dev/floor/check-run-pins.mjs.dev/floor/check-run-pins.test.mjs.github/workflows/publish.yml.pharn/pharn-dev-verify/results.json.pharn/writes-scope.jsondocs/RELEASING.md
Backslash-continued run commands are merged before parseLine so package args on the next line classify correctly, and the symlink parity test pins the sibling gate exit code directly. Co-authored-by: Cursor <cursoragent@cursor.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The finding
publish.ymlrannpm install -g npm@latestinside the repo's highest-trust job — OIDC Trusted Publishing,id-token: write, thenpm-publishenvironment. Whatever npm shipped that day executed the release.Two facts made the fix delete, not pin:
1. The install is unnecessary. Its own comment gives the only reason — "Trusted Publishing requires npm >= 11.5.1" — but the digest-pinned
setup-nodewithnode-version: 24resolves to the latest releasedv24.x, and that release (v24.19.0, checked 2026-08-10) bundles npm 11.17.0. Verified against the tag itself, not the branch head. npm's own docs confirm the floor number rather than us inheriting the workflow's claim:Node 24 clears both halves. The publish job can run zero publish-time tool downloads.
2. The floating step had already crossed a major, silently. The registry
latestdist-tag is npm 12.0.2 today — so every release since npm 12 shipped published with a major the workflow was never written against. Same shape as #79's movedv6tag: a live demonstration of why@latestrots in a release path.Repo scan: this was the only floating tool pull in any workflow.
gitleaks'curlis version- and SHA-256-pinned;npm ciis lockfile-pinned.runs-on: ubuntu-lateststill floats — platform-level, out of axis, observed.The fix
Part 1 — delete + enforce. The step is replaced by an
Assert npm floorstep: pure node stdlib, no packages, no network, a three-part integer semver compare that exits non-zero naming both versions. The floor is now enforced instead of installed, and the release job downloads no third-party tool at all.Fail-closed rests on the parse, not the shell.
set -euo pipefaildoes not abort on a command substitution used as an argument — reproduced:So a failing
npm --versionarrives as""and exits 1 as an unparseable version. The step comment says exactly this rather than crediting the shell.Part 2 — the enforcement gate (
.dev/floor/check-run-pins.mjs).check-action-pinsscansuses:refs only; arun:line pulling@latestwas outside its contract. The new gate classifies every package spec in a recognised install/exec invocation across workflows and local composite actions, requiring an exact semver. Enum reasons:floating-version/unpinned-package/unpinnable-version/unreadable-file. Verdict isviolations.length > 0— an integer test, so no decision reads the untrusted spec it copies verbatim.Wired with no workflow change:
floor.yml's existingnode --test ".dev/**/*.test.mjs"glob collects its live repo-consistency test on every PR.Named residuals, in the file's own header: non-package-manager pulls (
curl | sh, andnpm run <script>executing a package.json script), the walker duplicated fromcheck-action-pins(backstopped by a same-files[]cross-check test), shell indirection, value-separated flags, and a symlink asymmetry inherited from the sibling gate (pinned by a test asserting both gates agree).Proof of record
publish.ymlfires only onrelease: published, so no gate in this repo can execute it. The assert program was therefore extracted from the shippedpublish.ymlbytes and run against fake inputs:npm --version11.5.0npm floor NOT met: found 11.5.0, need >= 11.5.111.5.1npm floor OK— inclusive boundary11.17.0npm floor OK12.0.2npm floor OK""npm floor: unparseable version ""garbagenpm floor: unparseable version "garbage"The whole step body also executes clean against real npm (
npm floor OK: npm 11.12.1 >= 11.5.1). Actions parsing this workflow on the PR is the syntax proof.41 tests ship with the gate, including a positive control that reads the live
publish.yml, mutates one line back tonpm install -g npm@latest, and asserts it is caught — because this repo's live numbers arechecked: 0, andexit 0is also what a checker returns when it finds nothing to inspect.The gate also asserts the assert still exists: deleting
- name: Assert npm floorfails the suite, so the guarantee is protected against reversion and deletion.For the releaser — verification at the next release
Execution proof of record is the next release. In that run, look for the
Assert npm floorstep green, printingnpm floor OK: npm <version> >= 11.5.1. Note the version it reports: that is the npm actually bundled with the node the runner installed. If it ever fails, the message names both the found version and the floor — the fix is a pinned exactnpm install -g npm@<version>plus the assert, never@latest.Not in this PR
node-version: 24floats within the major, so the bundled npm floats across Node-vetted 11.x releases. The assert catches any dip below the floor. Pinning node exactly is the node-version-policy follow-up — third mention (M3, CI-matrix, here); it has earned its own ticket..dev/floor/README.mdopens "The floor is three files" over a three-row table while the directory holds 40+ checkers. Pre-existing (last touched in chore: add PHARN scaffolding and tighten write scope #15; chore(ci): pin floor.yml actions by digest; add a floor gate for action pins #79 added a checker without updating it) and flagged rather than auto-fixed, perCONSTITUTION.md.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests