From eefd63310d72e926f28870187663ff2bcff65b4b Mon Sep 17 00:00:00 2001 From: Lacy Morrow Date: Sun, 24 May 2026 13:56:43 -0400 Subject: [PATCH 1/2] fix: auto-detect publish target; loud failure for unpublishable roots MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/cli.ts | 4 ++ src/config.test.ts | 97 ++++++++++++++++++++++++++++- src/config.ts | 56 +++++++++++++++-- src/detect.test.ts | 135 +++++++++++++++++++++++++++++++++++++++++ src/detect.ts | 119 ++++++++++++++++++++++++++++++++++++ src/multi.test.ts | 2 +- src/multi.ts | 106 +++++++++++++++++++++++++------- src/steps/bump.test.ts | 2 +- src/steps/git.test.ts | 2 +- src/steps/npm.ts | 64 +++++++++++++++++++ src/steps/preflight.ts | 60 +++++++++++++++--- src/types.ts | 6 ++ 12 files changed, 615 insertions(+), 38 deletions(-) create mode 100644 src/detect.test.ts create mode 100644 src/detect.ts diff --git a/src/cli.ts b/src/cli.ts index 121addf..991b071 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -248,6 +248,10 @@ async function main(argv: string[] = process.argv.slice(2)): Promise { p.log.info(pc.dim("Dry-run mode — no changes will be made")); } + if (config.npm.autoDetectedReason) { + p.log.info(config.npm.autoDetectedReason); + } + const hookCtx = () => ({ config, version: newVersion, tag: gitTag, changelog, isBeta }); let newVersion = ""; let gitTag = ""; diff --git a/src/config.test.ts b/src/config.test.ts index 4f9c233..cf01760 100644 --- a/src/config.test.ts +++ b/src/config.test.ts @@ -1,5 +1,9 @@ -import { describe, expect, test } from "bun:test"; -import { normalizeFlags } from "./config.ts"; +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import { execSync } from "node:child_process"; +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { resolve } from "node:path"; +import { loadConfig, normalizeFlags } from "./config.ts"; describe("normalizeFlags (M2)", () => { test("undefined → empty array", () => { @@ -38,3 +42,92 @@ describe("normalizeFlags (M2)", () => { expect(normalizeFlags(["", "--no-verify", ""])).toEqual(["--no-verify"]); }); }); + +describe("loadConfig npm.cwd auto-detection", () => { + let root: string; + + beforeEach(() => { + root = mkdtempSync(resolve(tmpdir(), "shipx-cfg-")); + // Mark as a git repo so any downstream consumers behave; not strictly + // required for loadConfig but keeps it close to real-world use. + execSync("git init -q", { cwd: root }); + }); + + afterEach(() => { + rmSync(root, { recursive: true, force: true }); + }); + + function writePkg(dir: string, pkg: object): void { + mkdirSync(dir, { recursive: true }); + writeFileSync(resolve(dir, "package.json"), JSON.stringify(pkg)); + } + + test("publishable root: npm.cwd defaults to root, no auto-detect reason", async () => { + writePkg(root, { name: "foo", bin: "cli.js" }); + const cfg = await loadConfig(root); + expect(cfg.npm.cwd).toBe(root); + expect(cfg.npm.autoDetectedReason).toBe(""); + }); + + test('"private": true root: auto-detects single publishable subpackage', async () => { + writePkg(root, { name: "lacy", private: true }); + writePkg(resolve(root, "packages/lacy"), { name: "lacy", bin: { lacy: "index.mjs" } }); + + const cfg = await loadConfig(root); + expect(cfg.npm.cwd).toBe(resolve(root, "packages/lacy")); + expect(cfg.npm.autoDetectedReason).toMatch(/packages\/lacy/); + expect(cfg.npm.targets[0].cwd).toBe(resolve(root, "packages/lacy")); + }); + + test("auto-detection also points packageJsonPaths + versionSource at the detected subpackage", async () => { + writePkg(root, { name: "lacy", private: true }); + writePkg(resolve(root, "packages/lacy"), { name: "lacy", bin: "x.js" }); + + const cfg = await loadConfig(root); + expect(cfg.versionSource).toBe("packages/lacy/package.json"); + // Subpackage first so preflight (which reads paths[0]) sees the right + // bin/files; root second so it still gets a version bump in lockstep. + expect(cfg.packageJsonPaths).toEqual([ + "packages/lacy/package.json", + "package.json", + ]); + }); + + test("user-set packageJsonPaths is not overridden by auto-detection", async () => { + writePkg(root, { name: "lacy", private: true }); + writePkg(resolve(root, "packages/lacy"), { name: "lacy", bin: "x.js" }); + writeFileSync( + resolve(root, ".shipxrc.json"), + JSON.stringify({ packageJsonPaths: ["custom.json"] }), + ); + + const cfg = await loadConfig(root); + expect(cfg.packageJsonPaths).toEqual(["custom.json"]); + // npm.cwd still auto-detects independently + expect(cfg.npm.cwd).toBe(resolve(root, "packages/lacy")); + }); + + test("ambiguous root: falls back to root and records the ambiguity", async () => { + writePkg(root, { name: "monorepo", private: true }); + writePkg(resolve(root, "packages/a"), { name: "a", main: "a.js" }); + writePkg(resolve(root, "packages/b"), { name: "b", main: "b.js" }); + + const cfg = await loadConfig(root); + expect(cfg.npm.cwd).toBe(root); + expect(cfg.npm.autoDetectedReason).toMatch(/set npm.cwd or npm.targets explicitly/); + }); + + test("user-set npm.cwd overrides detection and is resolved against root", async () => { + writePkg(root, { name: "lacy", private: true }); + writePkg(resolve(root, "packages/lacy"), { name: "lacy", bin: "x.js" }); + writePkg(resolve(root, "packages/other"), { name: "other", bin: "y.js" }); + writeFileSync( + resolve(root, ".shipxrc.json"), + JSON.stringify({ npm: { cwd: "packages/other" } }), + ); + + const cfg = await loadConfig(root); + expect(cfg.npm.cwd).toBe(resolve(root, "packages/other")); + expect(cfg.npm.autoDetectedReason).toBe(""); + }); +}); diff --git a/src/config.ts b/src/config.ts index a8aae6d..bda4506 100644 --- a/src/config.ts +++ b/src/config.ts @@ -1,5 +1,6 @@ import { existsSync, readFileSync } from "node:fs"; import { resolve } from "node:path"; +import { detectPublishTarget } from "./detect.ts"; import type { ResolvedConfig, ShipConfig } from "./types.ts"; import { detectDefaultBranch, exec, readJson } from "./utils.ts"; @@ -53,6 +54,7 @@ const DEFAULTS: Omit = { cwd: "", access: "public", targets: [], + autoDetectedReason: "", }, homebrew: { tapPath: "", @@ -90,6 +92,7 @@ function mergeConfig(base: Omit, user: ShipConfig): Omit cwd: user.npm?.cwd ?? base.npm.cwd, access: user.npm?.access ?? base.npm.access, targets: [], // resolved in loadConfig after cwd is finalized + autoDetectedReason: "", }, homebrew: { ...base.homebrew, @@ -132,12 +135,57 @@ export async function loadConfig(root: string): Promise { const merged = mergeConfig(DEFAULTS, userConfig); - if (!merged.packageJsonPaths.length && existsSync(pkgPath)) { - merged.packageJsonPaths = ["package.json"]; - } + const userSetCwd = typeof userConfig.npm?.cwd === "string" && userConfig.npm.cwd.length > 0; + const userSetTargets = (userConfig.npm?.targets?.length ?? 0) > 0; + const userSetPackageJsonPaths = (userConfig.packageJsonPaths?.length ?? 0) > 0; + const userSetVersionSource = typeof userConfig.versionSource === "string" && userConfig.versionSource.length > 0; + // Try auto-detection only when the user gave us no publish-target hint + // (no npm.cwd, no npm.targets). The detector is conservative: it only + // picks a subpackage when the root package.json is clearly unpublishable + // (private, workspace root, or no entry points). + let detectedRelativePath: string | null = null; if (!merged.npm.cwd) { - merged.npm.cwd = root; + if (!userSetCwd && !userSetTargets) { + const detected = detectPublishTarget(root); + if (detected.target) { + merged.npm.cwd = detected.target.cwd; + merged.npm.autoDetectedReason = `auto-detected npm.cwd → ${detected.target.relativePath} (${detected.reason})`; + detectedRelativePath = detected.target.relativePath; + } else if (detected.ambiguousCandidates && detected.ambiguousCandidates.length > 0) { + // Ambiguous: don't pick one silently. Fall back to root and + // record the ambiguity so preflight can warn loudly. + merged.npm.cwd = root; + merged.npm.autoDetectedReason = `${detected.reason}. Candidates: ${detected.ambiguousCandidates.map((c) => c.relativePath).join(", ")}`; + } else { + merged.npm.cwd = root; + } + } else { + merged.npm.cwd = root; + } + } else { + merged.npm.cwd = resolve(root, merged.npm.cwd); + } + + // When auto-detection picked a subpackage as npm.cwd, that same package's + // version field is what the registry will see — so default packageJsonPaths + // and versionSource to it too, unless the user already configured them. + // Without this, shipx would bump root/package.json and publish the + // subpackage with a stale version. + if (detectedRelativePath) { + const subPath = `${detectedRelativePath}/package.json`; + if (!userSetPackageJsonPaths) { + merged.packageJsonPaths = existsSync(pkgPath) + ? [subPath, "package.json"] + : [subPath]; + } + if (!userSetVersionSource) { + merged.versionSource = subPath; + } + } + + if (!merged.packageJsonPaths.length && existsSync(pkgPath)) { + merged.packageJsonPaths = ["package.json"]; } const userTargets = userConfig.npm?.targets; diff --git a/src/detect.test.ts b/src/detect.test.ts new file mode 100644 index 0000000..7835cd1 --- /dev/null +++ b/src/detect.test.ts @@ -0,0 +1,135 @@ +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { resolve } from "node:path"; +import { detectPublishTarget, isUnpublishablePackage } from "./detect.ts"; + +describe("isUnpublishablePackage", () => { + test('returns reason for "private": true', () => { + expect(isUnpublishablePackage({ private: true, bin: "x.js" })).toMatch(/private/); + }); + + test('returns reason for "workspaces" field', () => { + expect(isUnpublishablePackage({ workspaces: ["packages/*"], bin: "x.js" })).toMatch(/workspace/); + }); + + test("returns reason when no entry-point fields are present", () => { + expect(isUnpublishablePackage({ name: "foo" })).toMatch(/entry point/); + }); + + test("returns null when package has bin", () => { + expect(isUnpublishablePackage({ bin: "x.js" })).toBeNull(); + }); + + test("returns null when package has main", () => { + expect(isUnpublishablePackage({ main: "index.js" })).toBeNull(); + }); + + test("returns null when package has exports", () => { + expect(isUnpublishablePackage({ exports: { ".": "./index.js" } })).toBeNull(); + }); + + test("returns null when package has module", () => { + expect(isUnpublishablePackage({ module: "index.mjs" })).toBeNull(); + }); + + test("private takes precedence over having a bin", () => { + expect(isUnpublishablePackage({ private: true, bin: "x.js" })).toMatch(/private/); + }); +}); + +describe("detectPublishTarget", () => { + let root: string; + + beforeEach(() => { + root = mkdtempSync(resolve(tmpdir(), "shipx-detect-")); + }); + + afterEach(() => { + rmSync(root, { recursive: true, force: true }); + }); + + function writePkg(dir: string, pkg: object): void { + mkdirSync(dir, { recursive: true }); + writeFileSync(resolve(dir, "package.json"), JSON.stringify(pkg)); + } + + test("returns null when no root package.json", () => { + const result = detectPublishTarget(root); + expect(result.target).toBeNull(); + expect(result.reason).toMatch(/no root package.json/); + }); + + test("returns null when root is publishable", () => { + writePkg(root, { name: "foo", bin: "cli.js" }); + const result = detectPublishTarget(root); + expect(result.target).toBeNull(); + expect(result.reason).toMatch(/publishable/); + }); + + test('detects single subpackage when root has "private": true', () => { + writePkg(root, { name: "lacy", private: true }); + writePkg(resolve(root, "packages/lacy"), { + name: "lacy", + bin: { lacy: "index.mjs" }, + }); + + const result = detectPublishTarget(root); + expect(result.target).not.toBeNull(); + expect(result.target?.relativePath).toBe("packages/lacy"); + expect(result.target?.cwd).toBe(resolve(root, "packages/lacy")); + }); + + test('detects single subpackage when root has "workspaces"', () => { + writePkg(root, { name: "monorepo", workspaces: ["packages/*"] }); + writePkg(resolve(root, "packages/core"), { name: "core", main: "index.js" }); + + const result = detectPublishTarget(root); + expect(result.target?.relativePath).toBe("packages/core"); + }); + + test("detects subpackage when root has no entry points", () => { + writePkg(root, { name: "monorepo" }); + writePkg(resolve(root, "apps/cli"), { name: "cli", bin: "cli.js" }); + + const result = detectPublishTarget(root); + expect(result.target?.relativePath).toBe("apps/cli"); + }); + + test("returns ambiguous when multiple publishable subpackages exist", () => { + writePkg(root, { name: "monorepo", private: true }); + writePkg(resolve(root, "packages/a"), { name: "a", main: "a.js" }); + writePkg(resolve(root, "packages/b"), { name: "b", main: "b.js" }); + + const result = detectPublishTarget(root); + expect(result.target).toBeNull(); + expect(result.ambiguousCandidates).toHaveLength(2); + expect(result.reason).toMatch(/set npm.cwd or npm.targets explicitly/); + }); + + test("skips unpublishable subpackages when searching", () => { + writePkg(root, { name: "monorepo", private: true }); + writePkg(resolve(root, "packages/published"), { name: "published", bin: "x.js" }); + writePkg(resolve(root, "packages/private"), { name: "private", private: true, bin: "y.js" }); + writePkg(resolve(root, "packages/empty"), { name: "empty" }); + + const result = detectPublishTarget(root); + expect(result.target?.relativePath).toBe("packages/published"); + }); + + test("returns null when root is unpublishable and no subpackages found", () => { + writePkg(root, { name: "monorepo", private: true }); + + const result = detectPublishTarget(root); + expect(result.target).toBeNull(); + expect(result.reason).toMatch(/no publishable subpackage found/); + }); + + test("searches apps/ in addition to packages/", () => { + writePkg(root, { name: "monorepo", workspaces: ["apps/*"] }); + writePkg(resolve(root, "apps/web"), { name: "web", main: "index.js" }); + + const result = detectPublishTarget(root); + expect(result.target?.relativePath).toBe("apps/web"); + }); +}); diff --git a/src/detect.ts b/src/detect.ts new file mode 100644 index 0000000..df35e9e --- /dev/null +++ b/src/detect.ts @@ -0,0 +1,119 @@ +import { existsSync, readdirSync, statSync } from "node:fs"; +import { resolve } from "node:path"; +import { readJson } from "./utils.ts"; + +const SEARCH_DIRS = ["packages", "apps"]; + +/** + * Returns a reason string if the package is not directly publishable to npm, + * or null if it looks publishable. + * + * Unpublishable signals (any one is sufficient): + * - `"private": true` + * - `"workspaces"` field (monorepo root) + * - No entry-point field (`bin`, `main`, `exports`, `module`) + */ +export function isUnpublishablePackage(pkg: Record): string | null { + if (pkg.private === true) return 'marked "private": true'; + if (pkg.workspaces) return 'workspace root (has "workspaces" field)'; + const hasEntry = pkg.bin || pkg.main || pkg.exports || pkg.module; + if (!hasEntry) return "no bin/main/exports/module entry point"; + return null; +} + +export interface PublishTargetCandidate { + /** Absolute path */ + cwd: string; + /** Path relative to the root that triggered detection, for display */ + relativePath: string; + /** Why this candidate was picked, for logging */ + reason: string; +} + +interface DetectResult { + target: PublishTargetCandidate | null; + /** When target is null, why detection didn't fire (or did but failed) */ + reason: string; + /** When detection found multiple candidates, list them so callers can guide the user */ + ambiguousCandidates?: PublishTargetCandidate[]; +} + +/** + * If `root/package.json` is unpublishable, scan well-known subdirs + * (`packages/*`, `apps/*`) for exactly one publishable subpackage. + * + * Returns: + * - `target` set if root is unpublishable AND exactly one candidate found. + * - `target` null + `ambiguousCandidates` populated if 2+ candidates found. + * - `target` null otherwise (root is publishable, or no candidates found). + */ +export function detectPublishTarget(root: string): DetectResult { + const rootPkgPath = resolve(root, "package.json"); + if (!existsSync(rootPkgPath)) { + return { target: null, reason: "no root package.json" }; + } + + let rootPkg: Record; + try { + rootPkg = readJson(rootPkgPath); + } catch { + return { target: null, reason: "root package.json unreadable" }; + } + + const rootUnpublishable = isUnpublishablePackage(rootPkg); + if (!rootUnpublishable) { + return { target: null, reason: "root looks publishable" }; + } + + const candidates: PublishTargetCandidate[] = []; + + for (const searchDir of SEARCH_DIRS) { + const dirPath = resolve(root, searchDir); + if (!existsSync(dirPath)) continue; + let entries: string[]; + try { + entries = readdirSync(dirPath); + } catch { + continue; + } + + for (const entry of entries) { + if (entry.startsWith(".")) continue; + const subDir = resolve(dirPath, entry); + try { + if (!statSync(subDir).isDirectory()) continue; + } catch { + continue; + } + const subPkgPath = resolve(subDir, "package.json"); + if (!existsSync(subPkgPath)) continue; + let subPkg: Record; + try { + subPkg = readJson(subPkgPath); + } catch { + continue; + } + if (isUnpublishablePackage(subPkg)) continue; + candidates.push({ + cwd: subDir, + relativePath: `${searchDir}/${entry}`, + reason: `root is ${rootUnpublishable}; ${searchDir}/${entry} is the only publishable subpackage`, + }); + } + } + + if (candidates.length === 1) { + return { target: candidates[0], reason: candidates[0].reason }; + } + if (candidates.length === 0) { + return { + target: null, + reason: `root is ${rootUnpublishable} and no publishable subpackage found under ${SEARCH_DIRS.join("/, ")}/`, + }; + } + return { + target: null, + reason: `root is ${rootUnpublishable} and ${candidates.length} publishable subpackages found — set npm.cwd or npm.targets explicitly`, + ambiguousCandidates: candidates, + }; +} diff --git a/src/multi.test.ts b/src/multi.test.ts index 75fc6e8..450e6f0 100644 --- a/src/multi.test.ts +++ b/src/multi.test.ts @@ -55,7 +55,7 @@ function makeFakeConfig(name: string): ResolvedConfig { commitMessage: "release: {tag}", commitFlags: ["--no-verify"], pushFlags: ["--no-verify"], }, github: { draft: false, assets: [] }, - npm: { cwd: `/tmp/fake/${name}`, access: "public", targets: [{ cwd: `/tmp/fake/${name}`, access: "public" }] }, + npm: { cwd: `/tmp/fake/${name}`, access: "public", targets: [{ cwd: `/tmp/fake/${name}`, access: "public" }], autoDetectedReason: "" }, homebrew: { tapPath: "", formulaFile: "", repoSlug: "", commitMessage: "", binaryAssets: {} }, hooks: {}, }; diff --git a/src/multi.ts b/src/multi.ts index 942a0ab..c23056c 100644 --- a/src/multi.ts +++ b/src/multi.ts @@ -184,6 +184,50 @@ interface PreparedProject { isBeta: boolean; } +/** + * Re-read the current state of every npm publish target and update + * `project.hasNpm` / `project.private` / `project.name` accordingly. + * + * Discover reads the working tree once at scan time. After that, the user can: + * - switch the project's branch (changing package.json content), or + * - have configured `npm.cwd` / `npm.targets` pointing at a subpackage that + * wasn't visible at discover time. + * + * Both make `project.hasNpm` stale. We re-derive from whatever package.json + * files `npm publish` will actually read at the moment of decision, so a + * project never gets a tag + GitHub release without an npm publish purely + * because state went stale (see LAC-2090). + */ +function reevaluateProjectPublishability( + project: DiscoveredProject, + config: ResolvedConfig, +): void { + if (!config.steps.npm) { + project.hasNpm = false; + return; + } + + let anyPublishable = false; + for (const target of config.npm.targets) { + try { + const pkg = readJson(resolve(target.cwd, "package.json")); + const isPublishable = pkg.private !== true && !pkg.workspaces; + if (isPublishable) { + anyPublishable = true; + // Prefer the published-package name over the root dir name for + // registry-version reconciliation later. + if (typeof pkg.name === "string") project.name = pkg.name; + if (target.cwd === project.path) project.private = false; + } else if (target.cwd === project.path) { + project.private = pkg.private === true; + } + } catch { + // unreadable target — treat as not publishable + } + } + project.hasNpm = anyPublishable; +} + function parseFlag(argv: string[], flag: string): string | undefined { const idx = argv.indexOf(flag); if (idx === -1 || idx === argv.length - 1) return undefined; @@ -356,28 +400,10 @@ export async function multiMain(argv: string[]): Promise { if (isAnyBranch) config.anyBranch = true; if (customTag) config.tag = customTag; - // Re-derive npm publishability from loaded config — config targets are - // authoritative over discover-time detection. When config explicitly - // points npm.cwd at a subdirectory, that sub-package determines hasNpm. - if (config.steps.npm) { - const hasCustomTarget = config.npm.targets.some( - (t) => t.cwd !== project.path, - ); - if (hasCustomTarget) { - project.hasNpm = config.npm.targets.some((t) => { - try { - const pkg = readJson(resolve(t.cwd, "package.json")); - const isPublishable = pkg.private !== true && !pkg.workspaces; - if (isPublishable && typeof pkg.name === "string") { - project.name = pkg.name; - } - return isPublishable; - } catch { - return false; - } - }); - } - } + // Re-derive npm publishability from the loaded config (config targets + // are authoritative over discover-time detection: explicit npm.cwd or + // auto-detected subpackage may differ from the project root). + reevaluateProjectPublishability(project, config); if (!isBeta && !isAnyBranch && project.branch !== config.git.releaseBranch) { const canSwitch = branchExists(project.path, config.git.releaseBranch); @@ -407,6 +433,19 @@ export async function multiMain(argv: string[]): Promise { p.log.error(`Failed to switch branch: ${errorText(err)}`); continue; } + + // Branch switch changes package.json content — re-evaluate + // publishability so the new branch's `private` / entry-points + // drive Phase 2 (not the stale pre-switch working tree). Without + // this, a tag + GitHub release can be produced while npm publish + // silently skips. See LAC-2090. + const wasNpm = project.hasNpm; + reevaluateProjectPublishability(project, config); + if (wasNpm !== project.hasNpm) { + p.log.info( + `${project.dirName}: npm publishability changed after branch switch — ${pc.yellow(String(wasNpm))} → ${pc.green(String(project.hasNpm))}`, + ); + } } } @@ -556,6 +595,29 @@ export async function multiMain(argv: string[]): Promise { // Phase 2: Batch npm publish const npmProjects = prepared.filter((pp) => pp.config.steps.npm && pp.project.hasNpm); + // Surface any prepared project that is about to skip npm publish despite + // having steps.npm=true. A tag + GitHub release without an npm publish is + // almost always a bug or a misconfiguration — never let it happen silently + // (see LAC-2090). + const skippedFromNpm = prepared.filter((pp) => pp.config.steps.npm && !pp.project.hasNpm); + for (const pp of skippedFromNpm) { + const reasons: string[] = []; + for (const target of pp.config.npm.targets) { + try { + const pkg = readJson(resolve(target.cwd, "package.json")); + if (pkg.private === true) reasons.push(`${target.cwd}: private`); + else if (pkg.workspaces) reasons.push(`${target.cwd}: workspace root`); + } catch { + reasons.push(`${target.cwd}: package.json unreadable`); + } + } + const reasonStr = reasons.length ? ` (${reasons.join("; ")})` : ""; + p.log.warn( + `${pc.yellow(pp.project.dirName)} was tagged and pushed but will NOT be published to npm${reasonStr}.\n` + + ` If this is unexpected, check ${pc.cyan("npm.cwd")} in your shipx config or the package.json on this branch.`, + ); + } + if (npmProjects.length) { if (isDryRun) { p.log.step(pc.bold(`Phase 2: npm publish (${npmProjects.length} package${npmProjects.length === 1 ? "" : "s"})`)); diff --git a/src/steps/bump.test.ts b/src/steps/bump.test.ts index 5cfda37..2f7f4f0 100644 --- a/src/steps/bump.test.ts +++ b/src/steps/bump.test.ts @@ -26,7 +26,7 @@ function makeConfig(root: string, overrides: Partial = {}): Reso commitMessage: "release: {tag}", commitFlags: [], pushFlags: [], }, github: { draft: false, assets: [] }, - npm: { cwd: root, access: "public", targets: [{ cwd: root, access: "public" }] }, + npm: { cwd: root, access: "public", targets: [{ cwd: root, access: "public" }], autoDetectedReason: "" }, homebrew: { tapPath: "", formulaFile: "", repoSlug: "", commitMessage: "", binaryAssets: {} }, hooks: {}, ...overrides, diff --git a/src/steps/git.test.ts b/src/steps/git.test.ts index df706b6..fa99260 100644 --- a/src/steps/git.test.ts +++ b/src/steps/git.test.ts @@ -46,7 +46,7 @@ function makeConfig(root: string): ResolvedConfig { pushFlags: ["--no-verify"], }, github: { draft: false, assets: [] }, - npm: { cwd: root, access: "public", targets: [{ cwd: root, access: "public" }] }, + npm: { cwd: root, access: "public", targets: [{ cwd: root, access: "public" }], autoDetectedReason: "" }, homebrew: { tapPath: "", formulaFile: "", repoSlug: "", commitMessage: "", binaryAssets: {} }, hooks: {}, }; diff --git a/src/steps/npm.ts b/src/steps/npm.ts index 2d0f929..2175240 100644 --- a/src/steps/npm.ts +++ b/src/steps/npm.ts @@ -76,6 +76,64 @@ function targetDisplayName(target: NpmTarget): string { return target.cwd; } +/** + * After a successful `npm publish`, ask the registry what it actually has + * for `@` and warn if the published artifact is missing + * entry points that exist locally — the lacy-style "no bin field on npm" + * disaster (LAC-2055). Best-effort: warns but never fails the release, + * since the registry can lag and offline environments shouldn't break. + */ +export function verifyPublishedArtifact(cwd: string): void { + let localPkg: Record; + try { + localPkg = readJson(resolve(cwd, "package.json")); + } catch { + return; + } + if (typeof localPkg.name !== "string" || typeof localPkg.version !== "string") return; + const name = localPkg.name; + const version = localPkg.version; + + let raw: string; + try { + raw = exec("npm", ["view", `${name}@${version}`, "bin", "main", "--json"], { cwd }); + } catch { + // Registry lag / network / package not found — don't fail the release. + p.log.warn( + `Could not verify ${pc.cyan(`${name}@${version}`)} on the registry (npm view failed).\n` + + ` Manually verify with ${pc.green(`npm view ${name}@${version}`)} once propagation completes.`, + ); + return; + } + + let view: Record; + try { + view = JSON.parse(raw.trim() || "{}"); + } catch { + return; + } + + const mismatches: string[] = []; + + const hasLocalBin = Boolean(localPkg.bin); + const hasRegistryBin = Boolean(view.bin); + if (hasLocalBin && !hasRegistryBin) { + mismatches.push(`local has "bin" but registry version has none`); + } + + if (typeof localPkg.main === "string" && typeof view.main !== "string") { + mismatches.push(`local has "main": "${localPkg.main}" but registry version has none`); + } + + if (mismatches.length) { + p.log.warn( + `Published ${pc.cyan(`${name}@${version}`)} doesn't match the local package:\n` + + mismatches.map((m) => ` ${pc.yellow("●")} ${m}`).join("\n") + "\n" + + ` This usually means a different package.json was published than expected — verify the package on npm before relying on it.`, + ); + } +} + async function publishMultipleTargets( targets: NpmTarget[], isBeta: boolean, @@ -150,6 +208,7 @@ async function publishMultipleTargets( if (tryWebPublish(args, target.cwd)) { p.log.success(` Published ${pc.green(displayName)}`); results.set(target, { name: displayName, success: true }); + verifyPublishedArtifact(target.cwd); } else { p.log.error(` Failed to publish ${displayName}`); results.set(target, { name: displayName, success: false }); @@ -167,6 +226,7 @@ async function publishMultipleTargets( exec("npm", attemptArgs, { cwd: target.cwd }); spinner.stop(pc.green(`Published ${displayName}`)); results.set(target, { name: displayName, success: true }); + verifyPublishedArtifact(target.cwd); } catch (err) { spinner.stop(pc.red(`Failed to publish ${displayName}`)); p.log.message(pc.dim(errorText(err))); @@ -251,6 +311,7 @@ export async function publishNpm( p.log.info("Publishing to npm with browser authentication…"); if (tryWebPublish(baseArgs, cwd)) { p.log.success(pc.green(`Published to npm${isBeta ? " (beta)" : ""}`)); + verifyPublishedArtifact(cwd); return true; } p.log.error("npm publish with web auth failed"); @@ -264,6 +325,7 @@ export async function publishNpm( try { exec("npm", attemptArgs, { cwd }); spinner.stop(pc.green(`Published to npm${isBeta ? " (beta)" : ""}`)); + verifyPublishedArtifact(cwd); return true; } catch (err) { spinner.stop(pc.yellow("npm publish failed")); @@ -309,6 +371,7 @@ export async function publishNpm( p.log.info("Opening browser for authentication…"); if (tryWebPublish(baseArgs, cwd)) { p.log.success(pc.green("Published to npm")); + verifyPublishedArtifact(cwd); return true; } p.log.error("npm publish with web auth failed"); @@ -333,6 +396,7 @@ export async function publishNpm( try { exec("npm", retryArgs, { cwd }); retrySpinner.stop(pc.green("Published to npm")); + verifyPublishedArtifact(cwd); return true; } catch (err) { retrySpinner.stop(pc.red("npm publish failed")); diff --git a/src/steps/preflight.ts b/src/steps/preflight.ts index 04d3f94..4ee7d3a 100644 --- a/src/steps/preflight.ts +++ b/src/steps/preflight.ts @@ -2,9 +2,21 @@ import * as p from "@clack/prompts"; import pc from "picocolors"; import { existsSync } from "node:fs"; import { resolve } from "node:path"; +import { isUnpublishablePackage } from "../detect.ts"; import type { ResolvedConfig } from "../types.ts"; import { exec, isRepoArchived, readJson } from "../utils.ts"; +/** + * Path to the package.json that will actually be published — read from + * `npm.cwd`, which is the directory `npm publish` runs in. This may differ + * from `packageJsonPaths[0]` (which is the version-bump source); using the + * wrong one means preflight validates a different package than the one + * heading to the registry. + */ +function publishPkgPath(config: ResolvedConfig): string { + return resolve(config.npm.cwd, "package.json"); +} + function checkCleanTree(config: ResolvedConfig): void { let status: string; try { @@ -127,7 +139,7 @@ export function resolveNpmRegistry( function checkNpmAuth(config: ResolvedConfig): void { if (!config.steps.npm) return; - const pkgPath = resolve(config.root, config.packageJsonPaths[0] ?? "package.json"); + const pkgPath = publishPkgPath(config); let registry: string | undefined; if (existsSync(pkgPath)) { registry = resolveNpmRegistry(readJson(pkgPath), config.npm.cwd); @@ -152,23 +164,23 @@ function checkNpmAuth(config: ResolvedConfig): void { function checkPackageEntryPoints(config: ResolvedConfig): void { if (!config.steps.npm) return; - const pkgPath = resolve(config.root, config.packageJsonPaths[0] ?? "package.json"); + const pkgPath = publishPkgPath(config); if (!existsSync(pkgPath)) return; const pkg = readJson(pkgPath); const missing: string[] = []; if (typeof pkg.main === "string") { - const mainPath = resolve(config.root, pkg.main); + const mainPath = resolve(config.npm.cwd, pkg.main); if (!existsSync(mainPath)) missing.push(`main: ${pkg.main}`); } if (typeof pkg.bin === "string") { - const binPath = resolve(config.root, pkg.bin); + const binPath = resolve(config.npm.cwd, pkg.bin); if (!existsSync(binPath)) missing.push(`bin: ${pkg.bin}`); } else if (pkg.bin && typeof pkg.bin === "object") { for (const [name, binFile] of Object.entries(pkg.bin as Record)) { - const binPath = resolve(config.root, binFile); + const binPath = resolve(config.npm.cwd, binFile); if (!existsSync(binPath)) missing.push(`bin.${name}: ${binFile}`); } } @@ -184,12 +196,12 @@ function checkPackageEntryPoints(config: ResolvedConfig): void { function checkPackageFiles(config: ResolvedConfig): void { if (!config.steps.npm) return; - const pkgPath = resolve(config.root, config.packageJsonPaths[0] ?? "package.json"); + const pkgPath = publishPkgPath(config); if (!existsSync(pkgPath)) return; const pkg = readJson(pkgPath); const hasFiles = Array.isArray(pkg.files); - const hasNpmIgnore = existsSync(resolve(config.root, ".npmignore")); + const hasNpmIgnore = existsSync(resolve(config.npm.cwd, ".npmignore")); if (!hasFiles && !hasNpmIgnore) { p.log.warn( @@ -200,6 +212,39 @@ function checkPackageFiles(config: ResolvedConfig): void { } } +/** + * Refuses to proceed when the package about to be published is structurally + * unpublishable (private, workspace root, or missing entry points). + * + * This is the main backstop against the lacy-style disaster where shipx + * silently shipped a root package with no `bin` field. Detection is the + * preferred fix; this is the loud-failure guard for cases where detection + * didn't fire or was overridden. + */ +function checkPackagePublishable(config: ResolvedConfig): void { + if (!config.steps.npm) return; + + for (const target of config.npm.targets) { + const pkgPath = resolve(target.cwd, "package.json"); + if (!existsSync(pkgPath)) { + p.log.error( + `npm target ${pc.cyan(target.cwd)} has no package.json — refusing to publish.`, + ); + process.exit(1); + } + const pkg = readJson(pkgPath); + const reason = isUnpublishablePackage(pkg); + if (reason) { + p.log.error( + `Package at ${pc.cyan(target.cwd)} is not publishable: ${reason}.\n` + + ` Either configure ${pc.green("npm.cwd")} / ${pc.green("npm.targets")} to point at the right subpackage,\n` + + ` or disable npm publish with ${pc.green("steps.npm: false")} in your shipx config.`, + ); + process.exit(1); + } + } +} + export function runPreflight(config: ResolvedConfig, isBeta: boolean): string { const spinner = p.spinner(); spinner.start("Running preflight checks"); @@ -228,6 +273,7 @@ export function runPreflight(config: ResolvedConfig, isBeta: boolean): string { spinner.stop("Preflight OK"); + checkPackagePublishable(config); checkNpmAuth(config); checkPackageEntryPoints(config); checkPackageFiles(config); diff --git a/src/types.ts b/src/types.ts index 7366704..525e785 100644 --- a/src/types.ts +++ b/src/types.ts @@ -179,6 +179,12 @@ export interface ResolvedConfig extends Required>; hooks: Hooks; From 741343a07e80f74c5666c77a50ef7d8ed8c0a7a5 Mon Sep 17 00:00:00 2001 From: Lacy Morrow Date: Sun, 24 May 2026 15:40:18 -0400 Subject: [PATCH 2/2] review: use isUnpublishablePackage as the single source of truth (PR #49) 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. --- src/multi.test.ts | 92 +++++++++++++++++++++++++++++++++++++++++++++-- src/multi.ts | 17 ++++++--- 2 files changed, 103 insertions(+), 6 deletions(-) diff --git a/src/multi.test.ts b/src/multi.test.ts index 450e6f0..53320f3 100644 --- a/src/multi.test.ts +++ b/src/multi.test.ts @@ -1,5 +1,9 @@ -import { describe, expect, test } from "bun:test"; -import { buildNpmAuthOptions, batchPublishNpm } from "./multi.ts"; +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { resolve } from "node:path"; +import { buildNpmAuthOptions, batchPublishNpm, reevaluateProjectPublishability } from "./multi.ts"; +import type { DiscoveredProject } from "./discover.ts"; import type { ResolvedConfig } from "./types.ts"; // ── Auth option selection ────────────────────────────────────────────── @@ -162,3 +166,87 @@ describe("batchPublishNpm — OTP per-package", () => { ]); }); }); + +describe("reevaluateProjectPublishability", () => { + let root: string; + + beforeEach(() => { + root = mkdtempSync(resolve(tmpdir(), "shipx-reeval-")); + }); + + afterEach(() => { + rmSync(root, { recursive: true, force: true }); + }); + + function writePkg(dir: string, pkg: object): void { + mkdirSync(dir, { recursive: true }); + writeFileSync(resolve(dir, "package.json"), JSON.stringify(pkg)); + } + + function makeProject(path: string): DiscoveredProject { + return { + name: "x", + dirName: "x", + path, + version: "1.0.0", + private: false, + hasNpm: false, + changeCount: 0, + lastTag: "", + branch: "main", + dirty: false, + archived: false, + }; + } + + function makeConfig(cwd: string, npmEnabled = true): ResolvedConfig { + const cfg = makeFakeConfig("reeval"); + cfg.steps.npm = npmEnabled; + cfg.npm.targets = [{ cwd, access: "public" }]; + cfg.npm.cwd = cwd; + return cfg; + } + + test("publishable target: hasNpm becomes true", () => { + writePkg(root, { name: "pkg", bin: "x.js" }); + const project = makeProject(root); + reevaluateProjectPublishability(project, makeConfig(root)); + expect(project.hasNpm).toBe(true); + expect(project.name).toBe("pkg"); + }); + + test('private target: hasNpm becomes false', () => { + writePkg(root, { name: "pkg", bin: "x.js", private: true }); + const project = makeProject(root); + reevaluateProjectPublishability(project, makeConfig(root)); + expect(project.hasNpm).toBe(false); + expect(project.private).toBe(true); + }); + + // Regression: the previous inline `pkg.private !== true && !pkg.workspaces` + // check would have marked this publishable, missing the entry-points gap + // that caused the original npx-lacy disaster. Reported on PR #49 by Gemini. + test("target with no bin/main/exports/module: hasNpm becomes false", () => { + writePkg(root, { name: "pkg" }); + const project = makeProject(root); + project.hasNpm = true; // simulate stale "publishable" state + reevaluateProjectPublishability(project, makeConfig(root)); + expect(project.hasNpm).toBe(false); + }); + + test("steps.npm disabled: hasNpm is forced false regardless of pkg", () => { + writePkg(root, { name: "pkg", bin: "x.js" }); + const project = makeProject(root); + project.hasNpm = true; + reevaluateProjectPublishability(project, makeConfig(root, false)); + expect(project.hasNpm).toBe(false); + }); + + test("unreadable target: hasNpm becomes false, does not throw", () => { + // No package.json written. + const project = makeProject(root); + project.hasNpm = true; + expect(() => reevaluateProjectPublishability(project, makeConfig(root))).not.toThrow(); + expect(project.hasNpm).toBe(false); + }); +}); diff --git a/src/multi.ts b/src/multi.ts index c23056c..7cc405c 100644 --- a/src/multi.ts +++ b/src/multi.ts @@ -3,6 +3,7 @@ import pc from "picocolors"; import { resolve } from "node:path"; import { loadConfig } from "./config.ts"; import { runHook } from "./hooks.ts"; +import { isUnpublishablePackage } from "./detect.ts"; import { discoverProjects, type DiscoveredProject, type DiscoverResult } from "./discover.ts"; import { computeIgnoredAfterSelection, loadIgnored, saveIgnored } from "./ignore.ts"; import { bumpCargoWorkspaces } from "./steps/cargo.ts"; @@ -198,7 +199,7 @@ interface PreparedProject { * project never gets a tag + GitHub release without an npm publish purely * because state went stale (see LAC-2090). */ -function reevaluateProjectPublishability( +export function reevaluateProjectPublishability( project: DiscoveredProject, config: ResolvedConfig, ): void { @@ -211,7 +212,11 @@ function reevaluateProjectPublishability( for (const target of config.npm.targets) { try { const pkg = readJson(resolve(target.cwd, "package.json")); - const isPublishable = pkg.private !== true && !pkg.workspaces; + // Single source of truth: isUnpublishablePackage covers private, + // workspaces, AND the no-entry-points case. Inlining the check + // here previously missed the entry-points reason (caught by + // Gemini on PR #49). + const isPublishable = isUnpublishablePackage(pkg) === null; if (isPublishable) { anyPublishable = true; // Prefer the published-package name over the root dir name for @@ -605,8 +610,12 @@ export async function multiMain(argv: string[]): Promise { for (const target of pp.config.npm.targets) { try { const pkg = readJson(resolve(target.cwd, "package.json")); - if (pkg.private === true) reasons.push(`${target.cwd}: private`); - else if (pkg.workspaces) reasons.push(`${target.cwd}: workspace root`); + // Delegate to isUnpublishablePackage so the reason string stays + // in sync with detection (covers private, workspaces, AND + // missing entry points — Gemini caught the entry-points gap on + // PR #49). + const reason = isUnpublishablePackage(pkg); + if (reason) reasons.push(`${target.cwd}: ${reason}`); } catch { reasons.push(`${target.cwd}: package.json unreadable`); }