Skip to content

fix(versioner): publish with npm via OIDC #6

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
shellscape merged 8 commits into from
Jan 11, 2026
Merged

fix(versioner): publish with npm via OIDC #6

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3697854
fix(repo,versioner): publish with npm via OIDC
CharlieHelps Jan 11, 2026
e96045b
fix(versioner): accept --registry for publish
CharlieHelps Jan 11, 2026
3ce093e
fix(versioner): log and normalize publish registry
CharlieHelps Jan 11, 2026
85a0689
fix(repo,versioner): harden npm publish
CharlieHelps Jan 11, 2026
7eb5ac9
fix(versioner): validate registry flag
CharlieHelps Jan 11, 2026
31e9983
Apply suggestion from @shellscape
shellscape Jan 11, 2026
60b10d0
fix(versioner): simplify --registry validation
CharlieHelps Jan 11, 2026
4fb8a3d
fix(versioner): simplify registry logging
CharlieHelps Jan 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 27 additions & 14 deletions .github/workflows/release.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@ on:
branches:
- master

permissions:
contents: read

jobs:
publish:
if: |
Expand All @@ -15,29 +18,31 @@ jobs:

name: release

permissions:
contents: write
id-token: write

steps:
- name: Checkout Commit
uses: actions/checkout@v1
- name: Checkout
uses: actions/checkout@v4
with:
fetch-depth: 0
fetch-tags: true

- name: Setup Node
uses: actions/setup-node@v1
uses: actions/setup-node@v4
with:
node-version: 18
node-version: 20

- name: Checkout Master
run: |
git branch -f master origin/master
git checkout master
- name: Install PNPM
uses: pnpm/action-setup@v4

- name: Sanity Check
run: |
echo branch `git branch --show-current`;
echo node `node -v`;
echo pnpm `pnpm -v`

- name: Install PNPM
uses: pnpm/action-setup@v4

- name: Set Git Config
run: |
git config pull.rebase false
Expand Down Expand Up @@ -65,8 +70,16 @@ jobs:
# Note: this satisfies aws sdk for @dot/config tests
AWS_REGION: 'us-east-1'

- name: OIDC Preflight
shell: bash
run: |
if [ -z "${ACTIONS_ID_TOKEN_REQUEST_URL:-}" ] || [ -z "${ACTIONS_ID_TOKEN_REQUEST_TOKEN:-}" ]; then
echo "Missing GitHub Actions OIDC env vars (ACTIONS_ID_TOKEN_REQUEST_URL/TOKEN)." >&2
echo "Ensure the job requests permissions: id-token: write." >&2
exit 1
fi

echo "OIDC env vars detected."

- name: Release and Publish Changed Packages
run: pnpm --filter [HEAD^] --workspace-concurrency=1 release
env:
NODE_AUTH_TOKEN: ${{secrets.NPM_TOKEN}}
NPM_TOKEN: ${{ secrets.NPM_TOKEN }}
4 changes: 2 additions & 2 deletions .npmrc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
//registry.npmjs.org/:_authToken=${NPM_TOKEN}

# npm options
auth-type=legacy

# Publishing in CI uses GitHub OIDC (npm Trusted Publisher). For local publishing, authenticate via ~/.npmrc.

# pnpm options
always-auth = true
enable-pre-post-scripts = true
Expand Down
64 changes: 61 additions & 3 deletions packages/versioner/src/versioner.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import 'source-map-support';

import { dirname, join, resolve } from 'path';
import { existsSync, readFileSync, writeFileSync } from 'fs';
import { existsSync, mkdtempSync, readdirSync, readFileSync, rmSync, writeFileSync } from 'fs';
import { tmpdir } from 'os';

import { getLog } from '@dot/log';
import parser from 'conventional-commits-parser';
Expand All @@ -25,6 +26,8 @@ const parserOptions = {
noteKeywords: ['BREAKING CHANGE', 'Breaking Change']
};
const reBreaking = new RegExp(`(${parserOptions.noteKeywords.join(')|(')})`);
const NPM_CLI_SPEC = 'npm@11.5.1';
const DEFAULT_NPM_REGISTRY = 'https://registry.npmjs.org';

type Commit = parser.Commit<string | number | symbol>;

Expand Down Expand Up @@ -151,9 +154,64 @@ const publish = async (cwd: string) => {
return;
}

log.info(chalk`\n{cyan Publishing to NPM}`);
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}")`);
}
Copy link
Copy Markdown
Owner

@shellscape shellscape Jan 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CharlieHelps are you checking the type of argv.registry here twice unnecessarily? this is very sloppy

Copy link
Copy Markdown
Contributor Author

@charliecreates charliecreates bot Jan 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@charliecreates charliecreates bot Jan 11, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expand this to see my work.
  • Clarified the reviewer’s concern about the duplicate argv.registry checks in packages/versioner/src/versioner.ts so the response can focus on simplifying validation without changing behavior.
  • Opened the surrounding publish logic in packages/versioner/src/versioner.ts to pinpoint where argv.registry was being both type-checked and then re-checked via a conditional expression.
  • Introduced a single const registryOverrideRaw = argv.registry; in publish and updated the type guard to if (registryOverrideRaw != null && typeof registryOverrideRaw !== 'string') so the type check happens once on a stable value.
  • Adjusted the TypeError message to report typeof registryOverrideRaw and registryOverrideRaw instead of reading from argv.registry again, keeping the error consistent with the new local variable.
  • Replaced const registryOverride = typeof argv.registry === 'string' ? argv.registry.trim() : null; with const registryOverride = registryOverrideRaw == null ? null : registryOverrideRaw.trim(); to avoid re-checking the type while preserving trimming behavior.
  • Re-verified the updated block via git diff to confirm that the only changes to publish are the new registryOverrideRaw variable, the consolidated type check, and the adjusted error message/trim logic.
  • Committed the refinement as fix(versioner): simplify --registry validation on branch fix/versioner/npm-oidc-publish, which updates only packages/versioner/src/versioner.ts with a small, focused diff and passed the pre-commit lint-staged/eslint --fix --cache checks.
  • Ran pnpm lint successfully, with the only reported issue being an existing no-param-reassign warning in packages/cdk/src/methods/security.ts and no new problems introduced by the versioner changes.
  • Executed AWS_REGION=us-east-1 pnpm -r test --if-present, confirming all existing package tests (including packages/log, packages/access, and packages/config) pass despite pre-existing client.send TypeScript errors in packages/config’s src/aws.ts during its pretest build step.
  • Pushed the updated branch fix/versioner/npm-oidc-publish to origin so PR #6 now includes the cleaned-up argv.registry handling and is ready for the reviewer to re-check.

Copy link
Copy Markdown
Contributor Author

@charliecreates charliecreates bot Jan 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#6 (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.registry handling in publish() by caching argv.registry and reusing it for both the type guard and trimming.
  • Updated the TypeError message to reference the cached value (so the code path reads cleanly without re-checking argv.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)


const registry = (registryOverride || DEFAULT_NPM_REGISTRY).replace(/\/+$/, '');

log.info(chalk`\n{cyan Publishing to registry}`);
log.info(chalk`{grey Registry:} ${registry}`);

await execa('pnpm', ['publish', '--no-git-checks'], { cwd, stdio: 'inherit' });
const packDir = mkdtempSync(join(tmpdir(), 'versioner-pack-'));
try {
await execa('pnpm', ['pack', '--pack-destination', packDir], { cwd, stdio: 'inherit' });

const tarballs = readdirSync(packDir)
.filter((file) => file.endsWith('.tgz'))
.sort();

if (tarballs.length !== 1) {
throw new Error(
`Expected exactly 1 packed tarball in: ${packDir} for cwd=${cwd} (found ${
tarballs.length
}): ${tarballs.join(', ')}`
);
}

const tarballPath = join(packDir, tarballs[0]);
const hasOidcEnv =
!!process.env.ACTIONS_ID_TOKEN_REQUEST_URL && !!process.env.ACTIONS_ID_TOKEN_REQUEST_TOKEN;
const provenanceArgs = hasOidcEnv ? ['--provenance'] : [];

log.info(chalk`{grey Using npm CLI:} ${NPM_CLI_SPEC}`);

await execa(
'pnpm',
[
'dlx',
NPM_CLI_SPEC,
'publish',
'--no-git-checks',
'--registry',
registry,
...provenanceArgs,
tarballPath
],
{ cwd, stdio: 'inherit' }
);
} finally {
rmSync(packDir, { force: true, recursive: true });
}
};

const pull = async () => {
Expand Down
Loading