-
Notifications
You must be signed in to change notification settings - Fork 71
Add npm publish preflight to releases #1619
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,72 @@ | ||
| import { describe, expect, test } from 'bun:test'; | ||
|
|
||
| import { | ||
| describeNpmAccessFailure, | ||
| describeNpmPublishFailure, | ||
| partitionPublishArgs, | ||
| } from '../changeset-publish-utils'; | ||
|
|
||
| describe('describeNpmAccessFailure', () => { | ||
| test('explains invalid npm token errors', () => { | ||
| const message = describeNpmAccessFailure({ | ||
| output: `npm ERR! code E401 | ||
| npm ERR! Unable to authenticate, your authentication token seems to be invalid.`, | ||
| packageName: '@lucid-agents/core', | ||
| scope: '@lucid-agents', | ||
| }); | ||
|
|
||
| expect(message).toContain('NPM_TOKEN is missing or invalid'); | ||
| expect(message).toContain('@lucid-agents/core'); | ||
| }); | ||
|
|
||
| test('explains missing scope permissions', () => { | ||
| const message = describeNpmAccessFailure({ | ||
| output: `npm ERR! code E404 | ||
| npm ERR! 404 Not Found - GET https://registry.npmjs.org/-/package/@lucid-agents%2fcore/collaborators?format=cli`, | ||
| packageName: '@lucid-agents/core', | ||
| scope: '@lucid-agents', | ||
| }); | ||
|
|
||
| expect(message).toContain('lacks collaborator or publish access'); | ||
| expect(message).toContain('@lucid-agents'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('describeNpmPublishFailure', () => { | ||
| test('turns publish put 404s into a permission hint', () => { | ||
| const message = describeNpmPublishFailure({ | ||
| output: `npm ERR! code E404 | ||
| npm ERR! 404 Not Found - PUT https://registry.npmjs.org/@lucid-agents%2fanalytics - Not found | ||
| npm ERR! 404 '@lucid-agents/analytics@0.3.3' is not in this registry.`, | ||
| scope: '@lucid-agents', | ||
| }); | ||
|
|
||
| expect(message).toContain('likely a permissions problem'); | ||
| expect(message).toContain('@lucid-agents'); | ||
| }); | ||
|
|
||
| test('returns nothing for unrelated publish failures', () => { | ||
| const message = describeNpmPublishFailure({ | ||
| output: 'npm ERR! code E500\nnpm ERR! Internal server error', | ||
| scope: '@lucid-agents', | ||
| }); | ||
|
|
||
| expect(message).toBeUndefined(); | ||
| }); | ||
| }); | ||
|
|
||
| describe('partitionPublishArgs', () => { | ||
| test('extracts the preflight-only flag', () => { | ||
| const result = partitionPublishArgs(['--preflight-only', '--tag', 'next']); | ||
|
|
||
| expect(result.preflightOnly).toBe(true); | ||
| expect(result.passthroughArgs).toEqual(['--tag', 'next']); | ||
| }); | ||
|
|
||
| test('passes through regular changeset publish args unchanged', () => { | ||
| const result = partitionPublishArgs(['--tag', 'beta']); | ||
|
|
||
| expect(result.preflightOnly).toBe(false); | ||
| expect(result.passthroughArgs).toEqual(['--tag', 'beta']); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,88 @@ | ||
| type NpmFailureContext = { | ||
| output: string; | ||
| packageName?: string; | ||
| scope: string; | ||
| }; | ||
|
|
||
| export function getPackageScope(packageName: string): string | undefined { | ||
| if (!packageName.startsWith("@")) return undefined; | ||
| const slashIndex = packageName.indexOf("/"); | ||
| if (slashIndex <= 1) return undefined; | ||
| return packageName.slice(0, slashIndex); | ||
| } | ||
|
|
||
| export function describeNpmAccessFailure({ | ||
| output, | ||
| packageName, | ||
| scope, | ||
| }: NpmFailureContext): string | undefined { | ||
| if (/\bE401\b/i.test(output) || /Unable to authenticate/i.test(output)) { | ||
| return [ | ||
| `NPM_TOKEN is missing or invalid.`, | ||
| packageName | ||
| ? `npm access preflight failed for ${packageName}.` | ||
| : "npm access preflight failed.", | ||
| "Use an npm automation token with publish access to this scope.", | ||
| ].join(" "); | ||
| } | ||
|
|
||
| if ( | ||
| /\bE403\b/i.test(output) || | ||
| /\bE404\b/i.test(output) || | ||
| /collaborators/i.test(output) | ||
| ) { | ||
| return [ | ||
| `NPM_TOKEN authenticates, but it lacks collaborator or publish access for ${scope}.`, | ||
| packageName | ||
| ? `The preflight probe against ${packageName} was rejected by npm.` | ||
| : "The publish access probe was rejected by npm.", | ||
| `Use a token from an npm owner or package collaborator that can publish ${scope} packages.`, | ||
| ].join(" "); | ||
| } | ||
|
|
||
| return undefined; | ||
| } | ||
|
|
||
| export function describeNpmPublishFailure({ | ||
| output, | ||
| scope, | ||
| }: NpmFailureContext): string | undefined { | ||
| if ( | ||
| /\bE401\b/i.test(output) || | ||
| /Unable to authenticate/i.test(output) || | ||
| /ENEEDAUTH/i.test(output) | ||
| ) { | ||
| return `npm rejected the publish because NPM_TOKEN is missing or invalid for ${scope}.`; | ||
| } | ||
|
|
||
| if ( | ||
| /\bE404\b/i.test(output) && | ||
| /Not Found - PUT https:\/\/registry\.npmjs\.org\//i.test(output) | ||
| ) { | ||
| return [ | ||
| `npm returned a PUT 404 while publishing ${scope} packages.`, | ||
| `For scoped packages that already exist on npm, this is likely a permissions problem with NPM_TOKEN rather than missing package metadata.`, | ||
| `Verify that the token belongs to an npm owner or collaborator with publish access to ${scope}.`, | ||
| ].join(" "); | ||
| } | ||
|
|
||
| return undefined; | ||
| } | ||
|
|
||
| export function partitionPublishArgs(argv: string[]): { | ||
| preflightOnly: boolean; | ||
| passthroughArgs: string[]; | ||
| } { | ||
| const passthroughArgs: string[] = []; | ||
| let preflightOnly = false; | ||
|
|
||
| for (const arg of argv) { | ||
| if (arg === "--preflight-only") { | ||
| preflightOnly = true; | ||
| continue; | ||
| } | ||
| passthroughArgs.push(arg); | ||
| } | ||
|
|
||
| return { preflightOnly, passthroughArgs }; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,13 @@ import { existsSync } from "node:fs"; | |
| import path from "node:path"; | ||
| import { fileURLToPath } from "node:url"; | ||
|
|
||
| import { | ||
| describeNpmAccessFailure, | ||
| describeNpmPublishFailure, | ||
| getPackageScope, | ||
| partitionPublishArgs, | ||
| } from "./changeset-publish-utils"; | ||
|
|
||
| type DependencyBlocks = | ||
| | "dependencies" | ||
| | "devDependencies" | ||
|
|
@@ -27,6 +34,11 @@ type Backup = { | |
| contents: string; | ||
| }; | ||
|
|
||
| type ExecResult = { | ||
| code: number; | ||
| output: string; | ||
| }; | ||
|
|
||
| const __filename = fileURLToPath(import.meta.url); | ||
| const __dirname = path.dirname(__filename); | ||
| const repoRoot = path.resolve(__dirname, ".."); | ||
|
|
@@ -168,7 +180,76 @@ function restoreBackups(backups: Backup[]) { | |
| } | ||
| } | ||
|
|
||
| async function verifyNpmPublishAccess() { | ||
| if (process.env.LUCID_SKIP_NPM_PUBLISH_PREFLIGHT === "1") { | ||
| console.log("Skipping npm publish preflight via LUCID_SKIP_NPM_PUBLISH_PREFLIGHT=1"); | ||
| return; | ||
| } | ||
|
|
||
| const publicPackages = packages | ||
| .filter((pkg) => !pkg.manifest.private && pkg.manifest.name) | ||
| .sort((a, b) => (a.manifest.name ?? "").localeCompare(b.manifest.name ?? "")); | ||
| const scopes = new Set<string>(); | ||
|
|
||
| for (const pkg of publicPackages) { | ||
| const scope = pkg.manifest.name ? getPackageScope(pkg.manifest.name) : undefined; | ||
| if (scope) scopes.add(scope); | ||
| } | ||
|
|
||
| if (!scopes.size) return; | ||
|
|
||
| const auth = await exec(["npm", "whoami"], { allowFailure: true }); | ||
| if (auth.code !== 0) { | ||
| const scope = scopes.values().next().value ?? "the configured npm scope"; | ||
| const message = | ||
| describeNpmAccessFailure({ output: auth.output, scope }) ?? | ||
| `npm publish preflight failed before publishing ${scope} packages.`; | ||
| throw new Error(message); | ||
| } | ||
|
|
||
| for (const scope of scopes) { | ||
| const probe = await findPublishedPackageForScope(publicPackages, scope); | ||
| if (!probe) { | ||
| console.warn( | ||
| `Skipping npm collaborator preflight for ${scope}: no existing published package found to probe.` | ||
| ); | ||
| continue; | ||
| } | ||
|
|
||
| const access = await exec( | ||
| ["npm", "access", "list", "collaborators", probe, "--json"], | ||
| { allowFailure: true } | ||
| ); | ||
| if (access.code !== 0) { | ||
| const message = | ||
| describeNpmAccessFailure({ | ||
| output: access.output, | ||
| packageName: probe, | ||
| scope, | ||
| }) ?? | ||
| `npm publish preflight failed while checking collaborator access for ${probe}.`; | ||
| throw new Error(message); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| async function findPublishedPackageForScope( | ||
| candidates: PackageInfo[], | ||
| scope: string | ||
| ): Promise<string | undefined> { | ||
| for (const pkg of candidates) { | ||
| const name = pkg.manifest.name; | ||
| if (!name || getPackageScope(name) !== scope) continue; | ||
| const view = await exec(["npm", "view", name, "version", "--json"], { | ||
| allowFailure: true, | ||
| }); | ||
| if (view.code === 0) return name; | ||
| } | ||
| return undefined; | ||
| } | ||
|
|
||
| async function runPublish() { | ||
| const parsedArgs = partitionPublishArgs(process.argv.slice(2)); | ||
| const backups: Backup[] = []; | ||
| const sanitisedPackages: string[] = []; | ||
|
|
||
|
|
@@ -191,8 +272,28 @@ async function runPublish() { | |
| } | ||
|
|
||
| try { | ||
| const extraArgs = process.argv.slice(2); | ||
| await exec(["bun", "x", "changeset", "publish", ...extraArgs]); | ||
| await verifyNpmPublishAccess(); | ||
| if (parsedArgs.preflightOnly) { | ||
| console.log("npm publish preflight succeeded."); | ||
| return; | ||
|
Comment on lines
251
to
+278
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
More importantly, if Consider skipping the sanitization block when async function runPublish() {
const parsedArgs = partitionPublishArgs(process.argv.slice(2));
const backups: Backup[] = [];
const sanitisedPackages: string[] = [];
if (!parsedArgs.preflightOnly) {
for (const pkg of packages) {
// … sanitisation …
}
}
// …
}Prompt To Fix With AIThis is a comment left during a code review.
Path: scripts/changeset-publish.ts
Line: 251-278
Comment:
**Package sanitization runs unnecessarily during `--preflight-only`**
`runPublish` writes (and then restores) modified `package.json` files before calling `verifyNpmPublishAccess()`. When `--preflight-only` is passed, this disk I/O is entirely unnecessary — the sanitization step exists to prepare manifests for publishing, not to check credentials.
More importantly, if `sanitiseManifest` throws (e.g. an unresolved workspace dependency), the preflight step will fail with a manifest error instead of an npm auth error, which is confusing.
Consider skipping the sanitization block when `parsedArgs.preflightOnly` is true:
```typescript
async function runPublish() {
const parsedArgs = partitionPublishArgs(process.argv.slice(2));
const backups: Backup[] = [];
const sanitisedPackages: string[] = [];
if (!parsedArgs.preflightOnly) {
for (const pkg of packages) {
// … sanitisation …
}
}
// …
}
```
How can I resolve this? If you propose a fix, please make it concise. |
||
| } | ||
|
|
||
| const extraArgs = parsedArgs.passthroughArgs; | ||
| const publish = await exec(["bun", "x", "changeset", "publish", ...extraArgs], { | ||
| allowFailure: true, | ||
| }); | ||
| if (publish.code !== 0) { | ||
| const scope = packages | ||
| .map((pkg) => pkg.manifest.name) | ||
| .find((name): name is string => Boolean(name)) | ||
| ?.match(/^@[^/]+/)?.[0]; | ||
| const message = | ||
| scope && describeNpmPublishFailure({ output: publish.output, scope }); | ||
| if (message) { | ||
| throw new Error(`${message}\n\nbun x changeset publish exited with code ${publish.code}`); | ||
| } | ||
| throw new Error(`bun x changeset publish exited with code ${publish.code}`); | ||
| } | ||
| } finally { | ||
| if (backups.length) { | ||
| restoreBackups(backups); | ||
|
|
@@ -203,17 +304,32 @@ async function runPublish() { | |
| } | ||
| } | ||
|
|
||
| async function exec(argv: string[]) { | ||
| async function exec( | ||
| argv: string[], | ||
| opts: { allowFailure?: boolean } = {} | ||
| ): Promise<ExecResult> { | ||
| const proc = Bun.spawn(argv, { | ||
| cwd: repoRoot, | ||
| stdin: "inherit", | ||
| stdout: "inherit", | ||
| stderr: "inherit", | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
| const code = await proc.exited; | ||
| if (code !== 0) { | ||
|
|
||
| const [stdout, stderr, code] = await Promise.all([ | ||
| proc.stdout ? new Response(proc.stdout).text() : "", | ||
| proc.stderr ? new Response(proc.stderr).text() : "", | ||
| proc.exited, | ||
| ]); | ||
|
|
||
| if (stdout) process.stdout.write(stdout); | ||
| if (stderr) process.stderr.write(stderr); | ||
|
|
||
| const output = [stdout, stderr].filter(Boolean).join("\n"); | ||
| if (code !== 0 && !opts.allowFailure) { | ||
| throw new Error(`${argv.join(" ")} exited with code ${code}`); | ||
| } | ||
|
|
||
| return { code, output }; | ||
| } | ||
|
Comment on lines
+307
to
333
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The If the publish stalls, hangs, or is killed by the 30-minute job timeout, there will be zero diagnostic output in the log. Consider streaming output in real time while still capturing it for error reporting, e.g. via a Prompt To Fix With AIThis is a comment left during a code review.
Path: scripts/changeset-publish.ts
Line: 307-333
Comment:
**Output buffering breaks real-time CI streaming**
The `exec` function was changed from `stdout: "inherit"` / `stderr: "inherit"` to `stdout: "pipe"` / `stderr: "pipe"`, with output flushed only after the subprocess exits. For quick commands like `npm whoami` this is harmless, but `bun x changeset publish` publishes every package in the monorepo sequentially — potentially running for several minutes — and CI logs will show nothing until the entire command completes.
If the publish stalls, hangs, or is killed by the 30-minute job timeout, there will be zero diagnostic output in the log. Consider streaming output in real time while still capturing it for error reporting, e.g. via a `PassThrough` tee or by writing to the log in chunks as each line arrives.
How can I resolve this? If you propose a fix, please make it concise. |
||
|
|
||
| await runPublish().catch((err) => { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
masterUnlike
release.ymlwhere the preflight is gated bysteps.metadata.outputs.should_publish == 'true', this step has no condition. It will run on every push tomastereven when there are no pending changesets and thechangesets/actionstep would not publish anything. This adds two npm API calls (npm whoami+npm access list collaborators) on every merge tomaster.More practically, repositories forked without
NPM_TOKENconfigured will see this step fail on every push, which is noisy. Consider adding a guard or at least documenting the intentional unconditional behaviour.Prompt To Fix With AI