Repository navigation
Merge v4-beta to v4 (2026-10-20) - #220
Conversation
…ishing MILAB-7081: npm trusted publishing (OIDC) for node-simple-pnpm
…e-docker-simple-fast-pnpm, node-simple, java-gradle (#219) * MILAB-7081: trusted publishing for the remaining shared node workflows npm no longer accepts 2FA-bypass tokens for direct publish. These workflows already publish from GitHub-hosted runners with id-token: write, so they need no split: - node-matrix-pnpm, node-docker-simple-fast-pnpm: pnpm -r publish via changesets/action is replaced by pack-unpublished + publish-tarballs in the same job. node-docker-simple-fast-pnpm keeps changesets/action for the version PR and publishes only when no changesets are pending. - node-matrix, node-simple, java-gradle (blocks/node/build-and-publish): new actions/node/prepare-trusted-publishing before `npm publish`: npm 11.5.1+ and, in public repositories, a package.json `repository` naming the repository (npm auto-enables provenance; the registry rejects it otherwise). The token stays as the fallback npm uses if OIDC fails. - npm-trust-setup.sh: npm prompts for 2FA only on a terminal; on EOTP the list call is re-run on /dev/tty so the browser approval appears. * MILAB-7081: address independent review of the shared-workflow change - pack refuses packages registry.npmjs.org does not have yet, before any script runs: without a token their first version needs a manual publish (npm only accepts a trusted publisher for an existing package). Avoids a partial release that keeps failing at upload. - prepare-trusted-publishing: switch to Node 24 when the job's Node is older than 22.14 (npm documents 22.14+ for trusted publishing); leave package.json alone for registries other than npmjs (publishConfig.registry wins), via a tested prepareRepository. - README: manifest.jsonl documented as a public contract (pframes-rs writes it); new-package rule.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (/\bE404\b/.test(result.stderr + result.stdout)) { | ||
| return false; | ||
| return 'new-package'; |
There was a problem hiding this comment.
versionState treats every E404 from npm view name@version version as a missing package. npm also returns E404 when the package exists but that exact version does not. The new gate in packWorkspace therefore rejects normal npmjs releases with instructions to publish the first version by hand.
After a version lookup returns E404, query the package without the version before returning new-package. Add a test that distinguishes these two failures.
Prompt To Fix With AI
This is a comment left during a code review.
Path: actions/node/npm-split-publish/src/registry.ts
Line: 59-60
Comment:
**Normal version bumps get rejected**
`versionState` treats every `E404` from `npm view name@version version` as a missing package. npm also returns `E404` when the package exists but that exact version does not. The new gate in `packWorkspace` therefore rejects normal npmjs releases with instructions to publish the first version by hand.
After a version lookup returns `E404`, query the package without the version before returning `new-package`. Add a test that distinguishes these two failures.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| const manifest = JSON.parse(fs.readFileSync(path.join(options.pkgDir, 'package.json'), 'utf8')) as PackageJson; | ||
| const registry = manifest.publishConfig?.registry || options.registry || 'https://registry.npmjs.org/'; | ||
| let host = ''; | ||
| try { | ||
| host = new URL(registry).hostname; | ||
| } catch { | ||
| /* not a URL: treated as another registry */ | ||
| } | ||
| if (host !== 'registry.npmjs.org') return 'other-registry'; |
There was a problem hiding this comment.
prepareRepository ignores scope settings in .npmrc. If a caller uses npmrc-config to send a package to npmjs while leaving registry-url at its GitHub Packages default, this function returns other-registry. It never adds the missing repository field, so the following npmjs upload can still fail the provenance check this action was added to fix.
Resolve the package's registry from npm's effective settings, as resolveRegistry already does.
Prompt To Fix With AI
This is a comment left during a code review.
Path: actions/node/npm-split-publish/src/repository.ts
Line: 116-124
Comment:
**npmjs preparation gets skipped**
`prepareRepository` ignores scope settings in `.npmrc`. If a caller uses `npmrc-config` to send a package to npmjs while leaving `registry-url` at its GitHub Packages default, this function returns `other-registry`. It never adds the missing `repository` field, so the following npmjs upload can still fail the provenance check this action was added to fix.
Resolve the package's registry from npm's effective settings, as `resolveRegistry` already does.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| - name: Node for trusted publishing | ||
| if: steps.node.outputs.too-old == 'true' | ||
| uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: ${{ inputs.node-version }} |
There was a problem hiding this comment.
This action can switch Node from the caller's chosen version to Node 24 after dependencies have been installed and the package has been built. Callers then run npm publish lifecycle scripts or publish-script-name against those same dependencies. If a script loads a native module built for Node 20, it will fail under Node 24. This also affects GitHub Packages uploads, which do not need npm trusted publishing.
Choose the publishing Node version before installing dependencies, or keep the caller's runtime for non-npmjs uploads.
Prompt To Fix With AI
This is a comment left during a code review.
Path: actions/node/prepare-trusted-publishing/action.yaml
Line: 53-57
Comment:
**Publish scripts change runtime**
This action can switch Node from the caller's chosen version to Node 24 after dependencies have been installed and the package has been built. Callers then run `npm publish` lifecycle scripts or `publish-script-name` against those same dependencies. If a script loads a native module built for Node 20, it will fail under Node 24. This also affects GitHub Packages uploads, which do not need npm trusted publishing.
Choose the publishing Node version before installing dependencies, or keep the caller's runtime for non-npmjs uploads.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| - name: Publish npm packages | ||
| if: steps.npm-pack.outputs.count != '' && steps.npm-pack.outputs.count != '0' | ||
| uses: milaboratory/github-ci/actions/node/publish-tarballs@v4 | ||
| with: | ||
| publish: pnpm -r publish --no-git-checks --access public | ||
| dir: ${{ steps.npm-pack.outputs.dir }} | ||
| access-public: ${{ inputs.publish-to-public }} | ||
| npm-auth-token: ${{ secrets.GITHUB_TOKEN }} |
There was a problem hiding this comment.
Replacing pnpm -r publish with publish-tarballs removes custom-registry support here and in node-docker-simple-fast-pnpm.yaml. If a caller supplies a custom registry through npmrc-config, packing still accepts it, but npmrcFor rejects every host except npmjs and GitHub Packages. The release fails after its prepublish scripts have already run.
Preserve that upload path, or reject the unsupported configuration before packing and document the changed input contract.
Prompt To Fix With AI
This is a comment left during a code review.
Path: .github/workflows/node-matrix-pnpm.yaml
Line: 945-951
Comment:
**Custom registries stop working**
Replacing `pnpm -r publish` with `publish-tarballs` removes custom-registry support here and in `node-docker-simple-fast-pnpm.yaml`. If a caller supplies a custom registry through `npmrc-config`, packing still accepts it, but `npmrcFor` rejects every host except npmjs and GitHub Packages. The release fails after its prepublish scripts have already run.
Preserve that upload path, or reject the unsupported configuration before packing and document the changed input contract.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| if [ "${rc}" -ne 0 ] && grep -q 'EOTP' "${err}" && authenticate "${pkg}"; then | ||
| # Approved in the browser; the 2FA skip window covers the retry. | ||
| rc=0 | ||
| out="$(npm trust list "${pkg}" --json 2>"${err}")" || rc=$? |
There was a problem hiding this comment.
The new EOTP recovery depends on a terminal call succeeding and a second captured call returning usable JSON. The existing test covers listing failure, but not that successful recovery sequence. A broken retry could therefore pass the tests and leave operators unable to finish setup.
Make terminal access replaceable in tests, or use a test terminal. Check that the retry preserves returned configurations and failure status.
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/npm-trust-setup.sh
Line: 88-91
Comment:
**Trust recovery lacks coverage**
The new `EOTP` recovery depends on a terminal call succeeding and a second captured call returning usable JSON. The existing test covers listing failure, but not that successful recovery sequence. A broken retry could therefore pass the tests and leave operators unable to finish setup.
Make terminal access replaceable in tests, or use a test terminal. Check that the retry preserves returned configurations and failure status.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Merge v4-beta into v4
Fix the release-blocking lookup, registry, and Node runtime changes before merging.
Fix with agent prompt
Summary
This PR moves two pnpm workflows to separate packing and uploading steps. It also adds preparation for direct npm trusted publishing and browser approval recovery to the trust setup script.
Important touched terms:
VersionState: New type describing whether a package version is published, new, or belongs to a missing package.packWorkspacenow uses it to stop first-time npmjs uploads.PrepareResult: New type describing whether preparation skipped another registry or private repository, added metadata, or found matching metadata.RepositoryState: Existing type describing missing, matching, or mismatched repository metadata. The newensureRepositoryalso uses it.PackageJson: Existing type for package fields used by publishing. The new preparation code reads itsrepositoryandpublishConfigfields; its shape is unchanged.ManifestEntry: Existing type for one packed package and its upload settings. The README now documents its shared file format; its shape is unchanged.Diagram
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Release ready] --> B{Publishing path} B -->|pnpm workflows| C[Check package versions] C --> D[Run prepublish scripts and pack] D --> E[Write manifest and tarballs] E --> F[Upload tarballs with npm] B -->|Direct npm workflows| G[Prepare Node and npm] G --> H[Check or add repository metadata] H --> I[Run npm publish or custom script]Reviews (1) · Last reviewed commit: "MILAB-7081: trusted publishing for node-..." · Reviewed by Greptile