Skip to content

fix(cli): refuse a project name npm rejects in os create, before any write - #15893

Merged
os-litant merged 3 commits into
mainfrom
claude/issue-15816-create-validates-project-name
Sep 5, 2026
Merged

fix(cli): refuse a project name npm rejects in os create, before any write#15893
os-litant merged 3 commits into
mainfrom
claude/issue-15816-create-validates-project-name

Conversation

@os-litant

Copy link
Copy Markdown
Collaborator

Fixes #15816

os create plugin "My App" exited 0 having written ./plugin-My App/, carrying a manifest that read name: "@objectstack/plugin-My App". os init "My App" refused the same input and wrote nothing. create validated nothing it emitted.

Measured before the repair, on the published entry

Driven as node packages/cli/bin/run.js, NO_COLOR=1, streams captured separately, exit code read before any pipe. Tree: origin/main e75a9040b02.

input os create plugin NAME os init NAME
my-app (valid control) exit 0, writes exit 0, writes
My App exit 0, writes exit 2, nothing
foo/bar exit 0, writes ./plugin-foo/bar/ exit 2, nothing
foo.bar exit 0, writes exit 0, writes
. exit 0, writes ./plugin-./ exit 2, nothing
214 x a exit 0, emits a 234-char package name exit 0 (correct: the name itself is at the cap)

os create's refusal surface, measured rather than read off the imports, was: unknown type, missing name, --in-repo outside a workspace, target directory already exists. Nothing about the name's content. -foo is refused by oclif's flag parser on both commands, which is not validation.

Two things the card asserts that the measurement does not support, both reported on the card:

  1. The card's own re-check block does not reproduce it. name is a positional argument; --name is a nonexistent flag and exits 2 on today's tree. Anyone re-checking with the literal block would see a refusal and conclude the defect was already fixed.
  2. The cost does not wait for npm publish. The plugin template renders export const ${toCamelCase(name)}Plugin, so My App emits export const My AppPlugin: Plugin = {. Parsed with TypeScript's own parser: 2 syntactic diagnostics for My App, 0 for my-app on the same template. The emitted project does not compile, let alone publish.

The shape, and why it is not simply "call the fifth symbol"

validateProjectName()'s full contract is five rules, not the one message the card quotes: required, <= 214 characters, lowercase, ^[a-z0-9][a-z0-9._-]*$, and no path separators.

The two commands do not validate the same string. os init's argument is the package name. os create composes its argument into a directory segment and a scoped package name, and npm's 214-character ceiling counts the scope — @objectstack/plugin- spends 20 characters before the user's first one. So a 214-character name is legal for init (measured: accepted) and composes to a 234-character name npm refuses (measured: emitted).

⇒ The validator is shared, and create carries one strictly additional check that init cannot need:

  • validateProjectName() and the length ceiling are now exported from init.ts and called by create.ts before its first mkdirSync. Imported rather than restated: the two scaffolders already shared four symbols, and the one they did not share is the one they disagreed on.
  • The composed-length check reads the package name back off the rendered manifest rather than recomposing @objectstack/plugin-${name} a second time. That measures the string that would actually land, covers a template added later without it declaring anything, and leaves the emitted scope literals untouched — os create plugin names the scaffolded package @objectstack/plugin-NAME — a scope the developer it is scaffolded for cannot publish to #15530 owns those.

sanitizeNamespace() was already on this path and does not narrow the defect: it is called once, inside the example template's config renderer. Measured on os create example "My App" — the config reads namespace: 'my_app' while the same run writes directory My App and manifest @example/My App. It normalises one field and leaves the two that reach npm alone.

Refusal is chosen over normalisation, per the triage seat's request that this be decided explicitly: the card's whole argument is that two commands answer the same input differently, and silently rewriting the user's name would restore the asymmetry pointing the other way.

After

My App now exits 1 with Project name must be lowercase — the shared validator's own return value — and the directory is absent. The absence predicate is the same one that reported CONTROL-OK: directory 'plugin-My App' EXISTS before the repair, so it is a check capable of failing. my-app still scaffolds; foo.bar still scaffolds (see findings below).

Tests

packages/cli/test/create-refuses-invalid-project-name.e2e.test.ts, driven end to end through bin/run-dev.js + tsx, which is how this package's e2e suites already spawn (@objectstack/cli#test depends on ^build only, so dist/ may legitimately be absent).

Ablations, each with the direction predicted in writing first, the mutation proven on disk by removed-text and injected-marker counts, and the restore proven by blob equality against the HEAD blob plus an empty git diff HEAD:

  • Removing the shared validator call: predicted 1 red — refuses the card's input and writes NOTHING. Measured exactly that: 1 failed, 7 passed. The valid-name control stayed green (the guard never fired for it) and the composed-length case stayed green (a different guard catches it). It went red while dist/ still carried the fix, which proves the pin resolves through src/ and the reading is not void.
  • Removing the composed-length check: predicted 1 red — refuses a name only the COMPOSED length catches. Measured exactly that: 1 failed, 7 passed.

Each guard is therefore separately load-bearing.

Gates: the union derived with node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack on the final tree, asserted against the tool's own Reconciliation line — 56 families (45 path-matched, 4 change-KIND, 7 always-runs), harvested with --commands so neither invocation spelling is dropped. All 56 exit 0, each captured before any pipe, run at f43c6e5bf18. pnpm --filter @objectstack/cli typecheck green, and the new test file is confirmed inside tsconfig.test.json's program (--listFiles, 1 hit, 0 errors attributed to it) so that green actually covers it.

Clause 2 (contract review) — declared per limb

  • Mechanical / path limb: NO. The diff adds no key to a published payload and touches no packages/spec/src/** path. The four symbols newly exported from init.ts / create.ts are not reachable through this package's exports map, which resolves only dist/index.js (re-exporting the command classes), dist/utils/console.js and dist/hook-body.js.
  • Non-mechanizable conformance limb: YES. This turns an input a published CLI accepted into one it refuses — an accept-set narrowing on a shipped face, which check-changeset-no-major.mjs names in as many words as the thing that grades major at GA. Graded yes rather than talked down because the diff is small.

needs:contract-review is applied to the card and this PR together.

The changeset declares it BREAKING and carries the ADR-0087 disposition not-required (no-migration-prescription); the gate accepts it and prints the reason. The level is minor because the launch window forbids major and carries breaking-ness in the banner plus the disposition instead.

Out of scope, filed rather than folded in

  • Filed as os create plugin foo.bar emits export const foo.barPlugin — an npm-legal name that renders un-parseable TypeScript #15892: foo.bar is an npm-legal name that still emits broken TypeScript (export const foo.barPlugin, 1 syntactic diagnostic against 0 for my-app on the same template). Same root cause, different failure surface. It survives this PR by design — validateProjectName() admits . because npm does — and the repair is a design choice (reject dots, sanitise the identifier, or stop deriving one) rather than a mechanical one.
  • The card's re-check block uses --name, which the command does not accept.

#15530 is untouched: the emitted scope is not changed, and no scope literal moves.


Generated by Claude Code

…y write

`os create plugin "My App"` exited 0 having written `./plugin-My App/` with a
manifest reading `name: "@objectstack/plugin-My App"`, while `os init "My App"`
refused the same input and wrote nothing. `create` validated nothing it emitted.

The rule set is `init`'s, imported rather than restated — the two scaffolders
already shared four symbols, and the one they did not share is the one they
disagreed on. `validateProjectName()` and npm's length ceiling are now exported
from `init.ts` and called by `create.ts` before its first `mkdirSync`.

The commands do not refuse identically, because they do not validate the same
string: `init`'s argument IS the package name, while `create` composes its
argument into a scoped one and npm's 214-character ceiling counts the scope. A
214-character name is therefore legal for `init` and composes to a 234-character
name npm refuses. That one check lives next to the composition, and reads the
name back off the RENDERED manifest so it measures the string that would land
rather than a second copy of how it is built.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D47qPfEWVPmhguWgBZCi5N
@github-actions github-actions Bot added size/m documentation Improvements or additions to documentation tests tooling labels Sep 5, 2026
@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/cli, touching 7 documentable anchor(s).

12 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/deployment/cli.mdx (via package.json (literal, a string literal in emittedPackageName), os create (command, read off packages/cli/src/commands/create.ts), os init (command, read off packages/cli/src/commands/init.ts))
  • content/docs/deployment/self-hosting.mdx (via package.json (literal, a string literal in emittedPackageName))
  • content/docs/deployment/tenancy-modes.mdx (via package.json (literal, a string literal in emittedPackageName))
  • content/docs/deployment/troubleshooting.mdx (via package.json (literal, a string literal in emittedPackageName))
  • content/docs/getting-started/examples.mdx (via package.json (literal, a string literal in emittedPackageName), os init (command, read off packages/cli/src/commands/init.ts))
  • content/docs/getting-started/your-first-project.mdx (via package.json (literal, a string literal in emittedPackageName), os init (command, read off packages/cli/src/commands/init.ts))
  • content/docs/plugins/development.mdx (via package.json (literal, a string literal in emittedPackageName))
  • content/docs/plugins/index.mdx (via package.json (literal, a string literal in emittedPackageName), os create (command, read off packages/cli/src/commands/create.ts))
  • content/docs/protocol/kernel/index.mdx (via package.json (literal, a string literal in emittedPackageName), os create (command, read off packages/cli/src/commands/create.ts))
  • content/docs/protocol/kernel/plugin-spec.mdx (via package.json (literal, a string literal in emittedPackageName), os create (command, read off packages/cli/src/commands/create.ts))
  • content/docs/protocol/objectql/schema.mdx (via package.json (literal, a string literal in emittedPackageName))
  • content/docs/upgrading.mdx (via package.json (literal, a string literal in emittedPackageName))

1 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/v17.mdx (via package.json (literal, a string literal in emittedPackageName), os init (command, read off packages/cli/src/commands/init.ts))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • 1 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 61 of 219 client-bound route-ledger rows — the other 158 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 158: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 22 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 4f379125e31e47bc0fe403fde930292639de5b3cpackageMentionDocs.

Which tree this was computed on

This run read content/docs from 615a78cf0768a910a15f5fbc1574715800fde486 — the merge of head f43c6e5bf18be0e1c7a5fb88d34d7eb03cb8cb26 into base 4f379125e31e47bc0fe403fde930292639de5b3c, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 615a78cf0768a910a15f5fbc1574715800fde486 && git checkout 615a78cf0768a910a15f5fbc1574715800fde486
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 4f379125e31e47bc0fe403fde930292639de5b3c f43c6e5bf18be0e1c7a5fb88d34d7eb03cb8cb26 && git checkout -B drift-repro 4f379125e31e47bc0fe403fde930292639de5b3c && git merge --no-ff f43c6e5bf18be0e1c7a5fb88d34d7eb03cb8cb26

node scripts/docs-audit/affected-docs.mjs --json 4f379125e31e47bc0fe403fde930292639de5b3c

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs 4f379125e31e47bc0fe403fde930292639de5b3c → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@os-litant os-litant left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Contract review (clause ②) — PASS

Reviewed at head f43c6e5bf18 from a detached worktree cut at that commit (merge-base with origin/main: b398ad258b9). Posted as a COMMENT because GitHub refuses APPROVE on a same-account PR (os-litant on both sides); the independence pair is the branch versus the seat's session, which cannot collide:

Implemented-by: claude/issue-15816-create-validates-project-name
Reviewed-by: session_01D47qPfEWVPmhguWgBZCi5N

Governed surface: none of the four delivered files (.changeset/olive-spiders-refuse.md, packages/cli/src/commands/{create,init}.ts, packages/cli/test/create-refuses-invalid-project-name.e2e.test.ts) matches docs/adr/**, .claude/**, skills/**, AGENTS.md, CLAUDE.md, or content/docs/releases/. Label removal is the seat's stroke, not this review's — needs:contract-review stays on both carriers until the seat clears it.

Tier fuse

Read from my own transcript (…/subagents/agent-a29a7ddce3c594434.jsonl, meta description "Contract review PR 15893"), not the dispatching session's: all 9 type:"assistant" records carry the harness stamp equal to CONTRACT_REVIEW_TIER at scripts/pm/dispatch-gates.mjs:8659. At tier.

Clause ② — per limb, from the delivered diff

Mechanical / path limb: NO — holds, verified at the published artifact. Pulled @objectstack/cli@17.3.0 from the registry (dist-tags.latest) and ran Node's own resolver against the extracted tarball:

specifier result role
@objectstack/cli resolves → dist/index.js positive control
@objectstack/cli/console resolves → dist/utils/console.js positive control
@objectstack/cli/dist/commands/init.js ERR_PACKAGE_PATH_NOT_EXPORTED where validateProjectName / NPM_PACKAGE_NAME_MAX_LENGTH live
@objectstack/cli/dist/commands/create.js ERR_PACKAGE_PATH_NOT_EXPORTED where emittedPackageName / validateEmittedPackageName live

dist/index.js and dist/index.d.ts re-export only default as <X>Command per command module — no export *, so nothing named in commands/* surfaces through .. One correction to the PR's wording: the published 17.3.0 exports map has two entries (., ./console); ./hook-body (and ./package.json) exist only in the source map at HEAD. Neither reaches commands/*, so the verdict is unchanged. No new key on a published payload (the manifest emitted for an accepted name is produced by an untouched renderer — measured @objectstack/plugin-my-app), no packages/spec/src/** path.

Non-mechanizable conformance limb: YES — holds. Derived judgments, each measured by hand on the head tree through bin/run-dev.js (exit code captured before any pipe; "entries" = files written into a fresh temp cwd):

  • create plugin "My App" → exit 1, 0 entries, stderr Project name must be lowercase (the shared validator's own string). Previously exit 0 + written (PR body table, consistent with the card).
  • create plugin my-app → exit 0, wrote plugin-my-app/, manifest @objectstack/plugin-my-app — positive control for the absence predicate.
  • create plugin <214×a> → exit 1, 0 entries, …234 characters; npm's limit is 214. Shorten the project name by at least 20 characters.
  • create plugin foo/bar → exit 1, 0 entries (path-split case closed).
  • init <214×a> → exit 0, wrote — init's accept-set is unchanged; the rule text at 214 is byte-identical after the constant extraction.

That is an accept-set narrowing on a shipped face, before the first write (validateProjectName at create.ts:526 and the composed check at :542 both precede the --in-repo guard, existsSync, and the first mkdirSync at :587). **BREAKING** is the honest grade; agreed with the dev's "graded yes rather than talked down".

The changeset — the two things adjudicated hardest

Level minor: correct. The launch-window convention is ruled, not tribal: #14043 (closed, completed) was landed by #14227 into the header of scripts/check-changeset-no-major.mjs — breaking changes on the lockstep 17.x fixed group ship as minor with a mandatory **BREAKING** banner and an ADR-0087 disposition, never major, until GA. The guard is armed and doing work, not cited as cover: no .changeset/pre.json (so the RC exemption is not in effect), --base origin/main exits 0 on the committed diff, and --self-test passes 116 assertions including the pre/exit switch in both directions. The 2026-08-30 addendum's counter-example (#8140/#11925/#12034/#12104) is minor without the banner and without the token; this changeset carries both, so it is the shape the addendum prescribes, not the one it condemns.

Disposition not-required (no-migration-prescription): correct — by elimination and by meaning, not because the gate went green. The ADR-0087 ledger serves objectstack migrate meta; D8 states this category "cannot tell a prescription for a metadata upgrader from a prescription for a source-code consumer, and only the first is the ledger's business." The author-visible action here — pick a name npm accepts — is neither a metadata rewrite nor a source rewrite; its channel is the refusal on stderr at invocation, and the body carries no FROM→TO or table prescription (the gate's statement-against-statement check passes on the committed diff, working tree 0 / untracked 0, so the instrument trap the dev flagged does not apply to my run). The other five: registered — nothing for the ledger to carry, an entry would be fabricated; unpublished — refused, @objectstack/cli publishes (private unset, 17.3.0 on npm); already-registered — no id exists; runtime-interface-only — requires naming an exported TS declaration that is the subject, and the subject here is behaviour; type-surface-only — requires an erased→concrete type narrowing, none present. The residual honest category is the one claimed. Registration gate: --base origin/main exit 0 (1 declared-breaking changeset(s), each carrying an ADR-0087 disposition), --self-test 325 assertions green. The only change to that gate between merge-base and origin/main (the STALE TREE warning names it) is a one-line comment respelling in #15876 — immaterial.

Shape

Composed-length instrument: right. emittedPackageName() reads template.filesFor(placement)['package.json'] and invokes the renderer, which is a pure object-literal builder for both templates (create.ts:231, :354 — no I/O, and the placement-specific spreads never touch name). Reading the rendered object measures the string that lands, covers example (@example/<name>, a different scope length) with no second constant, and covers a template added later. The null branch (no manifest / non-string name) is not a silent skip in practice: the per-template × per-placement test derives its cases from the templates map and asserts a string comes back, so a renderer that changes shape fails the pin rather than bypassing the check. No init.tscreate.ts import cycle was introduced.

Refusal over normalisation: right. It matches init, matches the triage seat's stated lean, avoids the third behaviour the triage comment warned against, and is the reversible choice — a refusal can later be relaxed into normalisation without breaking anyone who got through; normalisation, once shipped, cannot be withdrawn without a second narrowing. One watch item for the seat, not a blocker: the triage comment asked that this follow #15530's eventual ruling on the emitted scope; #15530 is with the maintainer and untouched here (the diff touches @objectstack/plugin- only inside comments — no scope literal moved). If that ruling goes to normalisation, the two fields of one manifest will carry two philosophies and this card should be revisited then.

Non-blocking observation: create refuses with exit 1 (its existing refusal convention, process.exit(1)) while init refuses with exit 2 (oclif this.error). The changeset states "exits 1" accurately; scripted callers testing != 0 are unaffected.

Tests and the single failure

  • New pin run in the worktree at head: exit 0, 8/8 passed (86 s). CI at the head merge commit 615a78cf076: Test Core 6/6 shards positively attested (read from the aggregate job log), all 66 check runs success or skipped, none failed.
  • test/lint-eval-json-unscorable-stack.e2e.test.ts: discrimination is sound. Confirmed all three legs independently — not among the 4 delivered files; references none of validateProjectName / emittedPackageName / validateEmittedPackageName / NPM_PACKAGE_NAME_MAX_LENGTH / commands/create / commands/init and spawns only lint --eval --json; and it passes 13/13, exit 0 in 114 s run alone at head in my worktree (load ~5). create.ts has no top-level side effects, so the only shared path with that test is oclif module load, and CI on a clean runner passed every shard.

Gate union

Derived in the worktree with dispatch-gates.mjs --changed and asserted against the tool's own Reconciliation line: 56 families (--commands prints 56; the matched bullet block is 45, which the tool itself warns is not the total). Re-ran the changeset-bearing families myself: check-changeset-no-major (+ self-test), check-adr-0087-registration (+ self-test), check-empty-changeset --base origin/main, check-changeset-fixed — all exit 0. The remaining families are taken from the dev's report (all 56 exit 0 at f43c6e5bf18) and corroborated by CI's "Lint & Repo Gates" success; I did not independently re-run them.

Controls and their axes

  • Resolver: positive (././console resolve) vs negative (deep paths refused) — discriminates "the exports map is actually consulted" from "everything resolves".
  • Write-absence: my-app writes a directory under the same predicate that reports 0 entries for My App — discriminates "refused before write" from "predicate cannot fail".
  • Label read-half: the card's labels read through two API paths (issue get and get_labels) agree on the same non-empty 6-label set including needs:contract-review; the PR's 5 labels (incl. the four restored auto-labels and needs:contract-review) came back non-empty through the structured PR read — discriminates a real read from the href-regex false empty the dev reported.
  • Gate arming: both changeset gates' self-tests green — discriminates an armed green from a NOT MEASURED green.

NOT MEASURED, by name

  • node scripts/pm/check-clause2-carriers.mjs --pair 15893exit 3, PREREQUISITE NOT MET (token refused with HTTP 403 through the proxy). Replaced by the two-path by-hand read above; the seat's pre-landing check ② still owes the mechanical run.
  • The 6 workflow-valued families (check-shard-attestation ×3, check-test-completeness ×2, check-cross-package-test-inputs --union-into) — not runnable locally by the tool's own declaration; CI's aggregate Test Core job carries the shard attestation (6/6).
  • Runtime import('@objectstack/cli') from the scratch tarball failed with ERR_MODULE_NOT_FOUND (its dependencies are not installed there); reachability rests on the resolver measurement plus the static read of dist/index.js, not on a runtime import.
  • Per-file presence of the new test in a CI shard log was not read (aggregate attestation only); my local run of the file substitutes.
  • The 50 gate families outside the four I re-ran are taken on the dev's report + CI, as stated.

Hand-written docs listed by the drift check (content/docs/deployment/cli.mdx etc.) document os create without stating a name rule, so nothing there is falsified by this narrowing.


Generated by Claude Code

@os-litant
os-litant marked this pull request as ready for review September 5, 2026 14:27
@os-litant
os-litant enabled auto-merge September 5, 2026 14:28
@os-litant
os-litant added this pull request to the merge queue Sep 5, 2026
Merged via the queue into main with commit cee3961 Sep 5, 2026
71 checks passed
@os-litant
os-litant deleted the claude/issue-15816-create-validates-project-name branch September 5, 2026 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/m tests tooling

Projects

None yet

2 participants