diff --git a/.changeset/olive-spiders-refuse.md b/.changeset/olive-spiders-refuse.md new file mode 100644 index 0000000000..cf9e7d95c0 --- /dev/null +++ b/.changeset/olive-spiders-refuse.md @@ -0,0 +1,13 @@ +--- +"@objectstack/cli": minor +--- + +**BREAKING** `os create ` now refuses a project name that npm refuses, and refuses it before it writes anything. + +`os create plugin "My App"` used to exit 0 having written `./plugin-My App/`, carrying a manifest that read `name: "@objectstack/plugin-My App"`. Nothing failed at scaffold time, so the invalid name surfaced later at `npm publish`, in the terminal of whoever ran it next. `os init` has always refused that same input before touching the disk. The rule set is now shared between the two scaffolders rather than restated in one of them, so they answer the same way. + +`os create` also refuses a name whose composed scoped package name exceeds npm's 214-character ceiling. `@objectstack/plugin-` spends 20 of those characters before the name begins, so a name that `os init` accepts can still compose to one npm rejects; that check sits next to the composition rather than in the shared rule set. + +A scripted invocation that passed an invalid name now exits 1 with the reason on stderr, where it previously exited 0 and produced a project that could not be published. + + diff --git a/packages/cli/src/commands/create.ts b/packages/cli/src/commands/create.ts index 3beda2c13a..9189d2f740 100644 --- a/packages/cli/src/commands/create.ts +++ b/packages/cli/src/commands/create.ts @@ -82,9 +82,11 @@ import path from 'path'; import { PROTOCOL_MAJOR } from '@objectstack/spec/kernel'; import { getCliVersion, + NPM_PACKAGE_NAME_MAX_LENGTH, renderPnpmWorkspaceYaml, sanitizeNamespace, SCAFFOLD_PNPM_RANGE, + validateProjectName, } from './init.js'; /** @@ -165,6 +167,52 @@ function defineTemplate(t: Omit): CreateTemplate { }; } +/** + * The scoped package name a scaffold is about to write, READ BACK off the + * rendered manifest rather than recomposed here. + * + * Recomposing it would be a second copy of `@objectstack/plugin-${name}` that + * nothing keeps in step with the renderer — the same restatement that let this + * command's emitted name drift away from what `os init` enforces. Reading the + * rendered object measures the string that actually lands on disk, and a + * template added later is covered without being told to declare anything. + * + * `null` when the template emits no `package.json`, or emits one without a + * string `name`: there is then no package name to judge, which is not the same + * as judging one and finding it fine. + */ +export function emittedPackageName( + template: CreateTemplate, + placement: ScaffoldPlacement, + name: string, +): string | null { + const render = template.filesFor(placement)['package.json']; + if (!render) return null; + const manifest = render(name) as { name?: unknown } | null | undefined; + return typeof manifest?.name === 'string' ? manifest.name : null; +} + +/** + * The one rule `os create` needs and `os init` cannot. + * + * `init`'s argument IS the package name, so measuring the argument is the same + * measurement. `create` composes its argument into a SCOPED name, and npm's + * 214-character ceiling counts the scope: `@objectstack/plugin-` spends 20 of + * them before the user's first character. A 200-character name is therefore + * legal for `init` (measured: accepted) and illegal for `create` (measured: + * emits a 220-character name npm refuses) — which is why the shared validator + * is shared and this check is not. + */ +export function validateEmittedPackageName(packageName: string): string | null { + const over = packageName.length - NPM_PACKAGE_NAME_MAX_LENGTH; + if (over <= 0) return null; + return ( + `The package name this would emit is ${packageName.length} characters; npm's limit is ` + + `${NPM_PACKAGE_NAME_MAX_LENGTH}. Shorten the project name by at least ${over} character` + + `${over === 1 ? '' : 's'}.` + ); +} + function toCamelCase(str: string): string { return str.replace(/-([a-z])/g, (g) => g[1].toUpperCase()); } @@ -464,12 +512,39 @@ export default class Create extends Command { console.log(chalk.dim(`Usage: objectstack create ${args.type} `)); process.exit(1); } - + + // ⛔ BEFORE the first write, which is the whole property — a refusal that + // arrives after `mkdirSync` has fixed the message and not the defect. + // + // This command used to validate nothing it emitted, so `os create plugin + // "My App"` exited 0 having written `./plugin-My App/` with a manifest + // reading `name: "@objectstack/plugin-My App"` — a name npm refuses — + // while `os init "My App"` refused the same input and wrote nothing. The + // rule set is `init`'s, imported rather than restated: the two scaffolders + // already share four symbols, and the one they did not share is the one + // they disagreed on. + const nameError = validateProjectName(args.name); + if (nameError) { + console.error(chalk.red(`\n❌ ${nameError}`)); + console.log(chalk.dim(` Usage: objectstack create ${args.type} `)); + process.exit(1); + } + const template = templates[args.type as keyof typeof templates]; const cwd = process.cwd(); const placement: ScaffoldPlacement = flags['in-repo'] ? 'in-repo' : DEFAULT_PLACEMENT; const projectDirName = template.dirName(args.name); + // The check `init` cannot need, on the string `init` never composes. Also + // before any write, and read off the rendered manifest so it measures what + // would land rather than a second copy of how it is built. + const willEmit = emittedPackageName(template, placement, args.name); + const packageNameError = willEmit ? validateEmittedPackageName(willEmit) : null; + if (packageNameError) { + console.error(chalk.red(`\n❌ ${packageNameError}`)); + process.exit(1); + } + // Refuse `--in-repo` outside a workspace rather than emit the one thing // this command is no longer allowed to emit: a project that cannot install. if (placement === 'in-repo' && !fs.existsSync(path.join(cwd, 'pnpm-workspace.yaml'))) { diff --git a/packages/cli/src/commands/init.ts b/packages/cli/src/commands/init.ts index 26565625c7..d0e83bb76e 100644 --- a/packages/cli/src/commands/init.ts +++ b/packages/cli/src/commands/init.ts @@ -676,13 +676,34 @@ export function detectPackageManager(env: NodeJS.ProcessEnv = process.env): 'npm return 'npm'; } +/** + * npm's hard ceiling on a package name, the scope included. + * + * Exported because `os create` does NOT validate the same string this file + * does: it composes its argument into a scoped name + * (`@objectstack/plugin-`) and has to measure the COMPOSED string + * against this limit. Restating the number over there is exactly how the two + * scaffolders came to disagree in the first place. + */ +export const NPM_PACKAGE_NAME_MAX_LENGTH = 214; + /** * Validate that `name` is a usable npm package name AND a safe directory * segment. Mirrors the subset of rules used by `npm init`/`create-vite`. + * + * Exported for `os create`, which took none of this and therefore accepted + * names npm refuses — `os create plugin "My App"` wrote `./plugin-My App/` + * carrying `name: "@objectstack/plugin-My App"`, while `os init "My App"` + * refused the same input before touching the disk. The rule set is shared + * rather than copied so a rule added here reaches both scaffolders; the one + * check `create` needs and `init` cannot (the length of the composed scoped + * name) lives next to the composition, in `create.ts`. */ -function validateProjectName(name: string): string | null { +export function validateProjectName(name: string): string | null { if (!name) return 'Project name is required'; - if (name.length > 214) return 'Project name must be ≤ 214 characters'; + if (name.length > NPM_PACKAGE_NAME_MAX_LENGTH) { + return `Project name must be ≤ ${NPM_PACKAGE_NAME_MAX_LENGTH} characters`; + } if (/[A-Z]/.test(name)) return 'Project name must be lowercase'; if (!/^[a-z0-9][a-z0-9._-]*$/.test(name)) { return 'Project name must start with a lowercase letter or digit and contain only [a-z0-9._-]'; diff --git a/packages/cli/test/create-refuses-invalid-project-name.e2e.test.ts b/packages/cli/test/create-refuses-invalid-project-name.e2e.test.ts new file mode 100644 index 0000000000..dba7d96ca8 --- /dev/null +++ b/packages/cli/test/create-refuses-invalid-project-name.e2e.test.ts @@ -0,0 +1,218 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * PIN — `os create` refuses a project name npm refuses, BEFORE it writes. + * + * ## The defect + * + * Measured on `origin/main` e75a9040b02, driving the published entry: + * + * ``` + * $ os create plugin "My App" + * exit 0 — wrote ./plugin-My App/, manifest name "@objectstack/plugin-My App" + * $ os init "My App" + * exit 2 — "Project name must be lowercase", wrote NOTHING + * ``` + * + * Both spellings are npm-invalid. One scaffolder refused before touching the + * disk; the other emitted a directory and an unpublishable manifest with every + * gate green, so the failure was deferred to `npm publish` in the terminal of + * whoever ran it next. + * + * ## Why the refusal, and not just the message, is what is asserted + * + * `os init` refuses BEFORE the first write. A repair that refuses AFTER + * `mkdirSync` has fixed the message and not the defect — the invalid directory + * still lands. So every refusal case here asserts the directory is ABSENT, and + * `accepts a valid name` is the positive control for that predicate: it runs + * the same `scaffoldDir()` check against the same shape of temp directory and + * finds the directory PRESENT. An absence assertion whose predicate cannot + * fail is not evidence. + * + * ## Why the two commands do NOT refuse identically + * + * `os init`'s argument IS the package name. `os create`'s argument is composed + * into a SCOPED one (`@objectstack/plugin-`), and npm's 214-character + * ceiling counts the scope — so a name that is legal for `init` can compose to + * one npm refuses. `refuses a name only the composed length catches` pins that + * asymmetry from both ends: the shared validator passes the name (asserted + * directly), `create` refuses it, and `init` still accepts it. ⛔ Moving that + * length rule into the shared validator would break `init` for a name npm + * accepts; this test is what says so. + * + * Spawned through `bin/run-dev.js` + tsx, so the suite does not depend on + * `packages/cli/dist` having been built — `@objectstack/cli#test` depends on + * `^build` only (the reason `invocation-loudness.e2e.test.ts` spawns that way). + */ + +import { describe, it, expect } from 'vitest'; +import { execFile } from 'node:child_process'; +import { existsSync, mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join, resolve } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { childEnv } from './helpers/serve-process.js'; +import { + emittedPackageName, + templates, + validateEmittedPackageName, + DEFAULT_PLACEMENT, + type ScaffoldPlacement, +} from '../src/commands/create.js'; +import { NPM_PACKAGE_NAME_MAX_LENGTH, validateProjectName } from '../src/commands/init.js'; + +const HERE = resolve(fileURLToPath(import.meta.url), '..'); +const CLI = resolve(HERE, '../bin/run-dev.js'); +const TSX = resolve(HERE, '../../../node_modules/.bin/tsx'); + +/** oclif + tsx cold start with every command module loaded; ~2-10 s when healthy. */ +const RUN_TIMEOUT_MS = 180_000; + +/** The card's input: a capital and a space, both npm-invalid. */ +const INVALID_NAME = 'My App'; +const VALID_NAME = 'my-app'; + +/** + * A name the SHARED validator accepts and the composed one cannot: exactly at + * `init`'s ceiling, so `@objectstack/plugin-` pushes it past the same ceiling. + * Derived from the constant rather than written as a number, so a change to the + * limit moves this case with it. + */ +const COMPOSED_TOO_LONG = 'a'.repeat(NPM_PACKAGE_NAME_MAX_LENGTH); + +interface Run { + code: number; + stdout: string; + stderr: string; +} + +function runCli(args: string[], cwd: string): Promise { + return new Promise((resolvePromise) => { + execFile( + TSX, + [CLI, ...args], + { cwd, maxBuffer: 8 * 1024 * 1024, env: childEnv({ NO_COLOR: '1' }) }, + (err, stdout, stderr) => { + resolvePromise({ + // `err.code` is the real exit status; null/undefined means the child + // was signalled — a different failure, never reported as 0. + code: err + ? typeof (err as { code?: unknown }).code === 'number' + ? (err as unknown as { code: number }).code + : 1 + : 0, + stdout: String(stdout), + stderr: String(stderr), + }); + }, + ); + }); +} + +/** A fresh empty directory to scaffold into, removed by the caller. */ +function workspace(): string { + return mkdtempSync(join(tmpdir(), 'os-create-name-')); +} + +/** The path `os create plugin ` writes, present or not. */ +function scaffoldDir(cwd: string, name: string): string { + return join(cwd, templates.plugin.dirName(name)); +} + +describe('os create: a name npm refuses is refused before anything is written', () => { + it( + 'refuses the card\'s input and writes NOTHING', + async () => { + const cwd = workspace(); + try { + const run = await runCli(['create', 'plugin', INVALID_NAME], cwd); + + expect(run.code).not.toBe(0); + // The message is the SHARED validator's own return value, not a second + // copy of it written here — that identity is what stops the two + // scaffolders drifting apart again. + expect(validateProjectName(INVALID_NAME)).not.toBeNull(); + expect(run.stderr).toContain(validateProjectName(INVALID_NAME)!); + + // The load-bearing half: refused BEFORE the first write. + expect(existsSync(scaffoldDir(cwd, INVALID_NAME))).toBe(false); + } finally { + rmSync(cwd, { recursive: true, force: true }); + } + }, + RUN_TIMEOUT_MS, + ); + + it( + 'accepts a valid name — the positive control for the absence check above', + async () => { + const cwd = workspace(); + try { + const run = await runCli(['create', 'plugin', VALID_NAME], cwd); + + expect(run.code).toBe(0); + // Same predicate, same shape of directory, opposite verdict: the + // absence assertion above is capable of failing. + expect(existsSync(scaffoldDir(cwd, VALID_NAME))).toBe(true); + } finally { + rmSync(cwd, { recursive: true, force: true }); + } + }, + RUN_TIMEOUT_MS, + ); + + it( + 'refuses a name only the COMPOSED length catches, which `os init` must keep accepting', + async () => { + const createCwd = workspace(); + const initCwd = workspace(); + try { + // The shared validator passes it — so whatever refuses it below is the + // composed-name rule and nothing else. + expect(validateProjectName(COMPOSED_TOO_LONG)).toBeNull(); + + const created = await runCli(['create', 'plugin', COMPOSED_TOO_LONG], createCwd); + expect(created.code).not.toBe(0); + expect(created.stderr).toContain(String(NPM_PACKAGE_NAME_MAX_LENGTH)); + expect(existsSync(scaffoldDir(createCwd, COMPOSED_TOO_LONG))).toBe(false); + + // ⛔ The asymmetry is correct, not a second defect: `init`'s argument is + // the package name, so npm's ceiling is already measured against it. + const inited = await runCli(['init', COMPOSED_TOO_LONG], initCwd); + expect(inited.code).toBe(0); + } finally { + rmSync(createCwd, { recursive: true, force: true }); + rmSync(initCwd, { recursive: true, force: true }); + } + }, + RUN_TIMEOUT_MS, + ); +}); + +describe('os create: the composed package name is judged for every template', () => { + const PLACEMENTS: ScaffoldPlacement[] = ['standalone', 'in-repo']; + + // Derived from the template map, never a list of `plugin` and `example`: a + // third template must arrive already covered. + for (const key of Object.keys(templates)) { + for (const placement of PLACEMENTS) { + it(`${key} / ${placement}: the emitted name is readable and judged`, () => { + const emitted = emittedPackageName(templates[key], placement, VALID_NAME); + expect(typeof emitted).toBe('string'); + expect(emitted).toContain(VALID_NAME); + expect(validateEmittedPackageName(emitted!)).toBeNull(); + + const overlong = emittedPackageName(templates[key], placement, COMPOSED_TOO_LONG); + expect(overlong!.length).toBeGreaterThan(NPM_PACKAGE_NAME_MAX_LENGTH); + expect(validateEmittedPackageName(overlong!)).toContain( + String(NPM_PACKAGE_NAME_MAX_LENGTH), + ); + }); + } + } + + it('reads the name off the DEFAULT placement the same way the command does', () => { + const emitted = emittedPackageName(templates.plugin, DEFAULT_PLACEMENT, VALID_NAME); + expect(emitted).toBe(`@objectstack/plugin-${VALID_NAME}`); + }); +});