fix(versioner): publish with npm via OIDC #6
Conversation
Switch @dot/versioner to pack with pnpm and publish via an OIDC-capable npm CLI. Update the release workflow and .npmrc to drop token-based auth.
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
Main concern: the workflow currently checks out ref: master, which can publish a different commit than the one that triggered the run—this is a release integrity problem. In versioner.ts, tarball selection is under-specified (first match wins) and should enforce exactly one artifact to avoid publishing the wrong file. Consider using fetch-depth: 0 for tag/history-dependent release tooling and explicitly pinning the npm registry during npm publish for safer CI behavior.
Summary of changes
What changed
GitHub Actions release workflow
- Added top-level
permissions: contents: readand job-levelpermissions: contents: write+id-token: writeto enable OIDC-based publishing. - Modernized actions:
actions/checkout@v1→actions/checkout@v4withfetch-tags: trueandfetch-depth: 100.actions/setup-node@v1→actions/setup-node@v4and Node18→20.
- Removed the custom “Checkout Master” shell step and instead checks out
ref: masterdirectly. - Added an OIDC Preflight step that fails early if
ACTIONS_ID_TOKEN_REQUEST_URL/TOKENare missing. - Removed
NPM_TOKEN/NODE_AUTH_TOKENfrom the publish step env.
npm configuration
- Removed token-based auth line (
//registry.npmjs.org/:_authToken=${NPM_TOKEN}) and added a comment indicating CI publishing uses OIDC Trusted Publisher.
@dot/versioner publish mechanics
- Replaced
pnpm publishwith:pnpm pack --pack-destination <tempdir>pnpm dlx [email protected] publish ... <tarball>
- Automatically adds
--provenancewhen running in GitHub Actions OIDC environment. - Ensures temp pack directory cleanup via
finally.
packages/versioner/src/versioner.ts
Outdated
| await execa('pnpm', ['pack', '--pack-destination', packDir], { cwd, stdio: 'inherit' }); | ||
|
|
||
| const tarballs = readdirSync(packDir).filter((file) => file.endsWith('.tgz')); | ||
| const [tarball] = tarballs; | ||
| if (!tarball) throw new Error(`Could not find packed tarball in: ${packDir}`); | ||
|
|
||
| const tarballPath = join(packDir, tarball); |
There was a problem hiding this comment.
readdirSync(packDir).filter(...); const [tarball] = tarballs; silently picks the first .tgz if multiple exist. That can publish the wrong artifact (e.g., leftover files if packDir is reused unexpectedly, or if pnpm pack ever emits more than one tarball). You should validate that exactly one tarball is present and fail otherwise.
Also consider sorting to make the selection deterministic if you intentionally support multiple outputs later.
Suggestion
Tighten the tarball discovery to be explicit and deterministic:
const tarballs = readdirSync(packDir)
.filter((file) => file.endsWith('.tgz'))
.sort();
if (tarballs.length !== 1) {
throw new Error(
`Expected exactly 1 packed tarball in ${packDir}, found ${tarballs.length}: ${tarballs.join(', ')}`
);
}
const tarballPath = join(packDir, tarballs[0]);Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
There was a problem hiding this comment.
Expand this to see my work.
- Previously documented the maintainer-approved changes to
publishinpackages/versioner/src/versioner.ts, includingDEFAULT_NPM_REGISTRY,normalizeRegistry,isValidRegistry, stricter tarball error messaging, and the updated.github/workflows/release.ymlOIDC/npm setup for PR #6. - Updated
packages/versioner/src/versioner.tssopublishderives and normalizes the registry internally fromregistryOverrideorDEFAULT_NPM_REGISTRY, always passes--registryexplicitly topnpm dlx, and tightened CLI validation and error messages around the--registryargument. - Enhanced tarball selection in
publishto require exactly one.tgzfile and throw a detailed error includingpackDir,cwd, and the discovered tarball list when the count is not 1. - Adjusted the call site to
await publish(cwd);afterpull()so it relies on the internal registry resolution logic, then confirmed all merge conflict markers were removed frompackages/versioner/src/versioner.ts. - Completed an interactive rebase preserving
fix(repo,versioner): harden npm publishwhile skipping the redundantfix(versioner): validate registry argcommit once its logic was folded into the resolvedpublishimplementation. - Verified the post-rebase history with
git log --oneline --graph -n 10(showingd145af1 fix(repo,versioner): harden npm publishonfix/versioner/npm-oidc-publish) and confirmed the expectednormalizeRegistry,--registryusage, and improved tarball error are present inpackages/versioner/src/versioner.ts. - Inspected
.github/workflows/release.ymlto ensure the OIDC-based npm publish job (withactions/setup-node@v4,id-token: write,fetch-depth: 0, andpnpmbuild/install steps) remains aligned with the hardenedpublishbehavior inversioner. - Rebased
fix/versioner/npm-oidc-publishonto remote commit3ce093e, resolving the conflict inpackages/versioner/src/versioner.tsto keep both the{grey Registry:}log.infoline and inline.replace(/\/+$/, '')behavior alongside the stricter registry validation and tarball checks, then pushed updated commit85a0689. - Ran
pnpm -r build && pnpm lint && AWS_REGION=us-east-1 pnpm -r test --if-presentsuccessfully, with all workspace builds and tests passing and only the knownno-param-reassignwarning inpackages/cdk/src/methods/security.ts, confirming the latestversionerchanges are clean. - Requested fresh review and assignment on PR #6 via
gh pr edit 6 --add-assignee shellscape --add-reviewer shellscape --add-reviewer CharlieHelps, re-ranCI=1 pnpm install --frozen-lockfileto validate workspacepreparehooks (includingpackages/versionerbuild) in a CI-like environment, and confirmed withgit statusthat the working tree is clean and only behindorigin/fix/versioner/npm-oidc-publishby one remote commit (no local changes).
There was a problem hiding this comment.
Applied the requested changes on PR #6.
Changes
@dot/versionernow sorts.tgzartifacts frompnpm packand fails unless exactly one tarball is produced (to avoid publishing the wrong artifact).@dot/versionerpublishsupports--registry(defaults tohttps://registry.npmjs.org), logs the chosen registry, and rejects empty/whitespace values..github/workflows/release.ymlnow checks out the triggering ref (noref: master) and usesfetch-depth: 0for tag/history-dependent release logic.
Verification
# Install
$ CI=1 pnpm install --frozen-lockfile
# Build
$ pnpm -r build
# Lint (1 warning)
$ pnpm lint
# Tests
$ AWS_REGION=us-east-1 pnpm -r test --if-present- Ava: 65 tests passed (3 packages)
charliecreates
bot
force-pushed
the
fix/versioner/npm-oidc-publish
branch
from
a44265e to
e96045b
Compare
There was a problem hiding this comment.
The workflow currently checks out ref: master, which can cause publishing a commit that is not the one that triggered the run—this is a release integrity risk. The checkout depth is capped at 100, which is brittle for tag/history-based release tooling and should be fetch-depth: 0. In versioner.ts, tarball discovery picks the first .tgz and should enforce exactly one artifact to avoid publishing the wrong package.
Summary of changes
Summary
This PR updates the release pipeline and @dot/versioner publishing flow to use npm Trusted Publisher via GitHub OIDC instead of token-based auth.
GitHub Actions (.github/workflows/release.yml)
- Adds workflow/job
permissions(contents+id-token) needed for OIDC. - Modernizes actions:
actions/checkout@v4,actions/setup-node@v4, and bumps Node18 → 20. - Adds PNPM setup earlier in the job.
- Adds an OIDC preflight step to fail early when OIDC env vars are missing.
- Removes
NPM_TOKEN/NODE_AUTH_TOKENfrom the publish step.
npm configuration (.npmrc)
- Removes token-based auth configuration and documents that CI publishing uses OIDC.
Versioner publish implementation (packages/versioner/src/versioner.ts)
- Replaces
pnpm publishwithpnpm packto a temp directory andpnpm dlx [email protected] publish. - Adds
--provenancewhen OIDC env vars are present. - Adds
--registrysupport with a default ofhttps://registry.npmjs.org/and logs the chosen registry. - Ensures temp pack directory cleanup via
finally.
- Fail if pnpm pack emits != 1 tarball. - Add --registry option (default: https://registry.npmjs.org/) and log it. - Release workflow: fetch full git history and publish the triggering SHA.
packages/versioner/src/versioner.ts
Outdated
| if (argv.registry != null && typeof argv.registry !== 'string') { | ||
| throw new TypeError( | ||
| `--registry must be a string (e.g. "${DEFAULT_NPM_REGISTRY}"), received ${typeof argv.registry}: ${String( | ||
| argv.registry | ||
| )}` | ||
| ); | ||
| } | ||
|
|
||
| const registryOverride = typeof argv.registry === 'string' ? argv.registry.trim() : null; | ||
| if (registryOverride != null && registryOverride.length === 0) { | ||
| throw new TypeError(`--registry must be a non-empty string (e.g. "${DEFAULT_NPM_REGISTRY}")`); | ||
| } |
There was a problem hiding this comment.
@CharlieHelps are you checking the type of argv.registry here twice unnecessarily? this is very sloppy
There was a problem hiding this comment.
Investigating this PR comment about the duplicated argv.registry type checks and I’m likely to propose a cleaner single-check version that keeps the same safety guarantees.
Updated: Jan 11, 20:31 UTC
I’m working and can’t be interrupted or see replies—feel free to start another task with me while I finish.
There was a problem hiding this comment.
Expand this to see my work.
- Clarified the reviewer’s concern about the duplicate
argv.registrychecks inpackages/versioner/src/versioner.tsso the response can focus on simplifying validation without changing behavior. - Opened the surrounding
publishlogic inpackages/versioner/src/versioner.tsto pinpoint whereargv.registrywas being both type-checked and then re-checked via a conditional expression. - Introduced a single
const registryOverrideRaw = argv.registry;inpublishand updated the type guard toif (registryOverrideRaw != null && typeof registryOverrideRaw !== 'string')so the type check happens once on a stable value. - Adjusted the
TypeErrormessage to reporttypeof registryOverrideRawandregistryOverrideRawinstead of reading fromargv.registryagain, keeping the error consistent with the new local variable. - Replaced
const registryOverride = typeof argv.registry === 'string' ? argv.registry.trim() : null;withconst registryOverride = registryOverrideRaw == null ? null : registryOverrideRaw.trim();to avoid re-checking the type while preserving trimming behavior. - Re-verified the updated block via
git diffto confirm that the only changes topublishare the newregistryOverrideRawvariable, the consolidated type check, and the adjusted error message/trim logic. - Committed the refinement as
fix(versioner): simplify --registry validationon branchfix/versioner/npm-oidc-publish, which updates onlypackages/versioner/src/versioner.tswith a small, focused diff and passed the pre-commitlint-staged/eslint --fix --cachechecks. - Ran
pnpm lintsuccessfully, with the only reported issue being an existingno-param-reassignwarning inpackages/cdk/src/methods/security.tsand no new problems introduced by theversionerchanges. - Executed
AWS_REGION=us-east-1 pnpm -r test --if-present, confirming all existing package tests (includingpackages/log,packages/access, andpackages/config) pass despite pre-existingclient.sendTypeScript errors inpackages/config’ssrc/aws.tsduring its pretest build step. - Pushed the updated branch
fix/versioner/npm-oidc-publishto origin so PR #6 now includes the cleaned-upargv.registryhandling and is ready for the reviewer to re-check.
There was a problem hiding this comment.
Good catch — yep, that was redundant. I refactored the --registry validation to read argv.registry once (registryOverrideRaw), type-check it once, then trim/validate without a second typeof check. Fix is in 60b10d0.
Changes
- Deduped
argv.registryhandling inpublish()by cachingargv.registryand reusing it for both the type guard and trimming. - Updated the
TypeErrormessage to reference the cached value (so the code path reads cleanly without re-checkingargv.registry).
Verification
$ pnpm install --frozen-lockfile
$ pnpm -r build
# tsc: 3 errors in @dot/config, 4 errors in @dot/cdk (preexisting; build script exits 0)
$ pnpm lint
# ESLint: 1 warning (no-param-reassign) in packages/cdk/src/methods/security.ts (preexisting)
$ AWS_REGION=us-east-1 pnpm -r test --if-present
# Ava: 65 tests passed (3 packages)
There was a problem hiding this comment.
The publish flow is materially safer (strict tarball selection + explicit --registry), but the --registry normalization is unnecessarily duplicated and should be simplified for maintainability. Also, provenance is currently optional based on env detection; if OIDC/provenance is a policy requirement for CI, publish() should fail fast (or at least warn) when OIDC vars are absent to avoid silent non-provenanced releases.
Summary of changes
What changed
GitHub Actions release workflow (.github/workflows/release.yml)
- Added top-level
permissions: contents: readand job-levelpermissionsincludingid-token: writeto enable OIDC. - Modernized actions (
actions/checkout@v4,actions/setup-node@v4) and bumped Node18 → 20. - Switched checkout to a full history/tag fetch via
fetch-depth: 0+fetch-tags: true. - Moved PNPM setup earlier in the job.
- Added an OIDC Preflight bash step to fail early if OIDC env vars are missing.
- Removed token-based auth env vars (
NPM_TOKEN/NODE_AUTH_TOKEN) from the publish step.
npm config (.npmrc)
- Removed token-based auth line and documented that CI publishing uses GitHub OIDC / npm Trusted Publisher.
@dot/versioner publishing (packages/versioner/src/versioner.ts)
- Replaced
pnpm publishwithpnpm packto a temp directory andpnpm dlx [email protected] publish. - Added
--registrysupport (defaulthttps://registry.npmjs.org), normalizes trailing slashes, and logs the chosen registry. - Hardened artifact selection by requiring exactly one packed
.tgztarball. - Added conditional
--provenancewhen OIDC env vars are present. - Ensured temp directory cleanup via
finally+rmSync(..., { recursive: true, force: true }).
shellscape
changed the title
There was a problem hiding this comment.
The main issue is incorrect/underspecified --registry validation in publish(): using truthiness (argv.registry && ... and argv.registry || ...) can silently bypass validation and/or fall back to the default registry, contradicting the PR’s stated behavior. Additionally, --provenance is optional based on env detection; given the workflow enforces OIDC, publish() should fail fast (or at least warn) in CI when OIDC vars are missing to avoid silent non-provenanced releases.
Summary of changes
Summary of changes
GitHub Actions release workflow (.github/workflows/release.yml)
- Added top-level workflow
permissions: contents: readand job-levelpermissions: contents: write+id-token: writeto enable GitHub OIDC. - Updated checkout to
actions/checkout@v4withfetch-depth: 0andfetch-tags: true(full history + tags). - Updated Node setup to
actions/setup-node@v4and bumped Node from18→20. - Reordered PNPM setup earlier in the job.
- Added an OIDC Preflight step to fail early if
ACTIONS_ID_TOKEN_REQUEST_URL/TOKENaren’t present. - Removed token-based env vars (
NPM_TOKEN/NODE_AUTH_TOKEN) from the publish step.
npm configuration (.npmrc)
- Removed token-based auth config line and documented that CI publishing uses OIDC Trusted Publisher, while local publishing uses
~/.npmrc.
Versioner publishing (packages/versioner/src/versioner.ts)
- Switched from
pnpm publishtopnpm packinto a temp dir +pnpm dlx [email protected] publish. - Added
--registrysupport with defaulthttps://registry.npmjs.organd included the registry in publish logs. - Hardened packed artifact selection by requiring exactly one
.tgztarball. - Added conditional
--provenancebased on presence of GitHub OIDC env vars. - Ensured temp pack directory cleanup via
finally+rmSync(..., { recursive: true, force: true }).
This PR contains a:
Are tests included?
Breaking Changes?
Description
@dot/versionerto publish using an OIDC-capablenpmCLI (pnpm pack+pnpm dlx [email protected] publish --provenance) instead ofpnpm publish.--registrysupport (defaulting tohttps://registry.npmjs.org) and logs the publish target registry.pnpm packoutput by requiring exactly one.tgztarball (fail fast if0or>1)..github/workflows/release.yml+.npmrcto removeNPM_TOKEN/NODE_AUTH_TOKENusage and requestid-token: write, and fixes release integrity by checking out the triggering ref (noref: master) withfetch-depth: 0.Refs shellscape/jsx-email#395.
Verification
$ pnpm install --frozen-lockfile $ pnpm -r build $ pnpm lint $ AWS_REGION=us-east-1 pnpm -r test --if-presenttarballs.length === 1per PR review request to avoid publishing the wrong artifact.--registrywarnings - this PR intentionally forces deterministic publishing to the npm registry (unless overridden) per reviewer direction.