Repository navigation
Merge v4-beta to v4 (2026-10-20, #221) - #222
Merged
Merged
Conversation
…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.
npm 10+ answers 'npm view name@version' with E404 both when the version is missing and when the package is missing, so pack treated every pending version of an existing package as a package new to npmjs and refused the release (runenv-python-3 on GitHub-hosted runners). On E404, look up the package itself.
…sion MILAB-7081: tell a new version from a new package with npm 10+
| return 'new-package'; | ||
| } | ||
| throw new CommandError(['npm', ...args].join(' '), result); | ||
| throw new CommandError(['npm', ...packageArgs].join(' '), pkg); |
There was a problem hiding this comment.
Second lookup errors lack tests
The new error branch has no test. registryRunner returns E500 on the first lookup for packages in failing, so the existing error test never reaches this branch. A later change could treat a registry outage or access error as “package missing” without failing the tests.
Add a case where the version lookup returns E404 and the package lookup returns E500 or E403. Assert that versionState throws with the second command and its error.
Prompt To Fix With AI
This is a comment left during a code review.
Path: actions/node/npm-split-publish/src/registry.ts
Line: 72
Comment:
**Second lookup errors lack tests**
The new error branch has no test. `registryRunner` returns `E500` on the first lookup for packages in `failing`, so the existing error test never reaches this branch. A later change could treat a registry outage or access error as “package missing” without failing the tests.
Add a case where the version lookup returns `E404` and the package lookup returns `E500` or `E403`. Assert that `versionState` throws with the second command and its error.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
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
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.
Merge v4-beta into v4: #221 (tell a new version from a new package with npm 10+).
The tree is v4-beta with the @v4-beta -> @v4 self-ref rewrite only. The plain merge-beta.sh merge duplicated hunks (a second prepare step in node-simple.yaml, a second isPublished in registry.ts) because #219 reached v4-beta as a squash and v4 as an amended commit; the merge commit's tree was replaced with v4-beta's to drop them.
The PR appears safe to merge, with a non-blocking test gap around errors from the second lookup.
Fix with agent prompt
Summary
This PR fixes how
versionStatetells a new version from a new package when npm returnsE404for both.Important touched terms:
VersionState— The three possible states:published,new-version, andnew-package. Its definition is unchanged; the new lookup improves which state is returned.Runner— A function that runs a command and returns aRunResult. Its type is unchanged;versionStatenow uses it for a second lookup afterE404.RunResult— A command’s exit code, standard output, and error output. Its shape is unchanged; the test helpers now model npm 10+ and legacy responses.Diagram
%%{init: {'theme': 'neutral'}}%% flowchart TD A["Look up name@version"] --> B{"Result"} B -->|"Version found"| C["published"] B -->|"Success, no output"| D["new-version"] B -->|"E404"| E["Look up package name"] B -->|"Other error"| F["Throw CommandError"] E --> G{"Result"} G -->|"Name found"| D G -->|"E404 or success with no output"| H["new-package"] G -->|"Other error"| FReviews (1) · Last reviewed commit: "Merge v4-beta into v4" · Reviewed by Greptile