fix: auto-detect publish target; loud failure for unpublishable roots#49
Conversation
Closes two related disasters seen in lacy's release stream: * LAC-2055 — single-project mode published the root package.json of a monorepo (no bin field), breaking npx for several versions in a row. * LAC-2090 — multi-mode produced a tag + GitHub release for v1.8.16 then silently skipped npm publish because hasNpm went stale after a branch switch. Unifying principle: npm.cwd is the source of truth for the package being published. Everything publish-related now reads from there, and shipx refuses to publish a structurally unpublishable package without a config fix or an explicit opt-out. Changes: * src/detect.ts — isUnpublishablePackage + detectPublishTarget. Conservative: only picks a subpackage when the root is clearly unpublishable (private, workspaces field, or no entry-point fields). Returns ambiguous when 2+ candidates exist instead of guessing. * src/config.ts — loadConfig runs detection when the user didn't set npm.cwd or npm.targets. When detection picks a subpackage, also defaults packageJsonPaths and versionSource to it so the bumped version matches the published one. Records the reason on ResolvedConfig so the cli can log it. * src/steps/preflight.ts — publish-side checks (entry points, files, npm auth) now read from npm.cwd, not packageJsonPaths[0]. New checkPackagePublishable refuses to proceed when any npm target is structurally unpublishable. * src/multi.ts — re-evaluate project.hasNpm after a branch switch (LAC-2090 fix). Warn loudly for every prepared project that is about to skip npm publish — silent skips after producing a tag are the worst failure mode. * src/steps/npm.ts — verifyPublishedArtifact runs after each successful publish (single + multi + retry paths), npm view's the version, and warns when the registry artifact is missing bin/main fields the local package has. Best-effort, never fails the release. * Tests for detect, loadConfig auto-detect, packageJsonPaths defaulting, and user-override precedence. After this change, a repo structured like lacy/ (root is private, single publishable subpackage under packages/) works with zero shipx config.
There was a problem hiding this comment.
Code Review
This pull request implements automatic detection of publishable subpackages in monorepos when the root package is unpublishable (e.g., private or a workspace root). It introduces a new detection utility, enhances preflight checks to prevent shipping unpublishable targets, and adds post-publish verification to ensure registry artifacts match local entry points. The review feedback correctly identifies two instances in src/multi.ts where manual publishability checks should be replaced with the centralized isUnpublishablePackage function to ensure consistency and capture all failure modes, such as missing entry points.
| for (const target of config.npm.targets) { | ||
| try { | ||
| const pkg = readJson(resolve(target.cwd, "package.json")); | ||
| const isPublishable = pkg.private !== true && !pkg.workspaces; |
There was a problem hiding this comment.
The publishability check here is inconsistent with the isUnpublishablePackage function defined in src/detect.ts. Specifically, it misses the check for entry points (bin, main, exports, module). This could lead to the same "no bin field" disaster the PR aims to prevent, as multi mode might incorrectly identify a package as publishable.
Since isUnpublishablePackage is already defined as the source of truth for this logic, it should be imported and used here to ensure consistency across the codebase.
| const isPublishable = pkg.private !== true && !pkg.workspaces; | |
| const isPublishable = !isUnpublishablePackage(pkg); |
There was a problem hiding this comment.
Done in 741343a — switched to isUnpublishablePackage(pkg) === null so the no-entry-points case is covered. Added a focused unit test (reevaluateProjectPublishability > target with no bin/main/exports/module: hasNpm becomes false) to pin the regression.
| if (pkg.private === true) reasons.push(`${target.cwd}: private`); | ||
| else if (pkg.workspaces) reasons.push(`${target.cwd}: workspace root`); |
There was a problem hiding this comment.
This manual check for skip reasons is incomplete and duplicates logic that should be centralized. It misses the "no entry point" reason, which is one of the primary failure modes this PR addresses.
Consider using isUnpublishablePackage to retrieve the specific reason for why a target is considered unpublishable.
| if (pkg.private === true) reasons.push(`${target.cwd}: private`); | |
| else if (pkg.workspaces) reasons.push(`${target.cwd}: workspace root`); | |
| const reason = isUnpublishablePackage(pkg); | |
| if (reason) reasons.push(`${target.cwd}: ${reason}`); |
There was a problem hiding this comment.
Done in 741343a — now uses isUnpublishablePackage(pkg) and reports the returned reason verbatim, so private / workspace-root / no-entry-points all surface consistently.
) Both spots in multi.ts that previously inlined a partial publishability check now call isUnpublishablePackage from detect.ts, so private, workspace-root, AND missing-entry-points are all caught consistently. - reevaluateProjectPublishability: a sub-package with no bin/main/exports was previously treated as publishable, defeating the original fix. Now it falls into the skip path with a clear reason. - Phase-2 skip-reason walk: now reports the reason verbatim from isUnpublishablePackage instead of a hand-rolled subset. Plus a focused unit test for reevaluateProjectPublishability that pins the entry-points regression case. Both raised by Gemini Code Assist on PR #49.
Summary
Closes two related disasters seen in lacy's release stream:
Unifying principle: `npm.cwd` is the source of truth for the package being published. Everything publish-related now reads from there, and shipx refuses to publish a structurally unpublishable package without a config fix or an explicit opt-out.
Changes
After this change, a repo structured like lacy (root is private, single publishable subpackage under `packages/`) works with zero shipx config.
Test plan
Notes