Skip to content

fix(cli): os generate refuses to write TypeScript that does not parse - #16724

Merged
os-project-manager merged 1 commit into
mainfrom
claude/issue-16541-generate-identifier-refusal
Sep 8, 2026
Merged

fix(cli): os generate refuses to write TypeScript that does not parse#16724
os-project-manager merged 1 commit into
mainfrom
claude/issue-16541-generate-identifier-refusal

Conversation

@claude

@claude claude Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Fixes #16541

os generate TYPE NAME ran no name validation of any kind — no validateProjectName, no sanitiser — so the name went into a binding position untouched. os generate object foo.bar reported success and left two files that are not TypeScript:

src/objects/foo.bar.object.ts   const foo.bar: Data.ServiceObject = {
src/objects/index.ts            export { default as foo.bar } from './foo.bar.object';

The author learns about it at the next tsc, in a file the scaffolder had just told them it created.

What lands

Both emissions — the scaffold file and the barrel re-export line — are now rendered once, at the single point where the derived identifier is finished, and handed to TypeScript's own parser before anything is written. If either does not parse, the command prints the compiler's own diagnostics per file and exits 1 without touching the filesystem.

  • One check covers all 14 emission sites across all 7 generators plus the barrel, and a generator added later inherits it — nothing is restated per site.
  • --dry-run refuses too. A preview that renders un-parseable TypeScript under exit 0 is the same defect in preview form: the author copies it, or a script trusts the status.
  • The write path now reuses the very bytes the check accepted. It used to call generator.generate(name) a second time, which would have made "the bytes checked" and "the bytes written" two different things.

The parser is reached through ts-morph's ts re-export — already a CLI runtime dependency, and the call src/utils/detect-free-identifiers.ts already makes for the same reason. It is imported lazily inside the check, so only a generate invocation pays for the compiler.

The ruling this obeys

sanitizeIdentifier is not ported over from #15892. Triage, verbatim:

不许把 sanitizeIdentifier#15892 平移过来。 那会用放宽容忍度的方式,悄悄回答一个尚未裁定的接受性问题

and the line it elevated from the card to a hard constraint:

a legal-looking identifier derived from a name that should have been refused is the worse of the two failures.

What lands is the half that needs no adjudication, verbatim:

本卡的交付物是那条不需要裁定的:os generate 不得在派生标识符无法作为合法 TS 标识符解析时 exit 0。

The criterion could be built without touching the acceptance question, so no stop-and-report was needed: "do the bytes parse" is answered by the compiler, not by a charset opinion. Still open, and still the maintainer's: whether os generate gets an acceptance gate, a sanitiser, or both — and if a gate, create's npm charset or something narrower. Nothing here narrows that choice; a sanitiser can be added on top of this refusal later without moving it.

Clause-②: no — with the evidence

This diff narrows os generate back toward a contract it already violates, and widens no accepted set:

Measured on this tree — the dispatch's three assumptions

  1. One chokepoint exists.runMetadataGeneration is it: every generator's generate(name) and the barrel's export line are both derived there. Site count re-measured and it matches the card: toCamelCase(name) appears at 14 template sites across 7 generators (object / view / action / flow / dashboard / app / skill), 2 per generator, plus 3 references in the barrel rewrite.
  2. The barrel is fed by the same derived identifier. ✅ Confirmed — and it is the bare camel for every type, while the scaffold binds a per-type suffix (Views, Action, Flow, Dashboard, App, Skill). The two emissions therefore disagree about the same name, which is precisely why the check reads the actual bytes rather than judging one identifier.
  3. The three toCamelCase copies are not the same function. ✅ Re-verified by symbol, not by the card's line numbers (init.ts's has since moved from 720 to 736): generate.ts and init.ts both fold /[-_]([a-z])/; create.ts no longer has a toCamelCase at all — os create plugin foo.bar emits export const foo.barPlugin — an npm-legal name that renders un-parseable TypeScript #15892 replaced it with sanitizeIdentifier.

One correction to the card's framing, measured: init.ts is not a third open door. It feeds toCamelCase the output of sanitizeNamespace(projectName), which is already constrained to ^[a-z][a-z0-9_]{1,19}$, so its looser fold cannot produce an illegal identifier. os generate was the only unguarded one.

Evidence — the defect, and the refusal, on this branch

The refusal was ablated (revert generate.ts to the merge base, prove the mutation reached disk, measure, restore, prove the restore by blob hash). Pre-fix leg, driven:

PRE-FIX EXIT = 0
PRE-FIX FILES:  /src/objects/foo.bar.object.ts   /src/objects/index.ts
PRE-FIX BYTES:  const foo.bar: Data.ServiceObject = {
                export { default as foo.bar } from './foo.bar.object';
RESTORE: blob matches HEAD   (git diff HEAD empty)

Post-fix, same command:

✗ Refusing to generate — the TypeScript this would write does not parse

  Name:       foo.bar
  Identifier: foo.bar

  src/objects/foo.bar.object.ts
    ',' expected.
  src/objects/index.ts
    ',' expected.

exit 1, nothing written. ',' expected. is the compiler's own diagnostic — the same message the #15892 instrument reports on the pre-fix os create bytes, so a zero from this harness is a reading rather than an inert one.

Why asking the compiler is not the same as writing a rule. Measured, driven:

command verdict why a hand-written rule gets it wrong
os g object class exit 1 const class: is not a declaration
os g view class exit 0 that generator emits const classViews: — legal
os g object order-line exit 0 control: orderLine, both files, unchanged

A character-class rule passes all three (every character is a letter). A reserved-word rule refuses the first two. Only the bytes answer per emission position — and the same instrument also catches a name carrying a quote or a comment terminator, which breaks the emitted file without touching the identifier at all.

Tests

  • test/generate-emission-parses.test.ts (unit tier) — runs the derived GENERATOR_SCAFFOLD_TARGETS roster through the shipped check, not a second copy of it. Carries the control (order-line still emits orderLine, zero failures, every generator), the canary (foo.bar fails at both emissions, every generator), and the discriminator rows above.
  • test/generate-refuses-unparseable-name.test.ts (integration tier) — a real child process, because process.exitCode inside a vitest worker is not an exit status and the reported defect is an exit code. Asserts exit 1, both files named, the compiler's own diagnostic quoted, ⛔ no fooBar anywhere in the output (the silent-sanitiser outcome), nothing on disk, --dry-run refused, and a control that writes both files and re-parses them.

⚠️ Deliberate naming, declared: the spawn pin is not named .e2e, so the merge queue runs it. vitest-tiers.ts names this exact combination as sanctioned ("a file that spawns the CLI without the name is queue (name) AND integration (behaviour)"), and nothing was renamed to make the two cuts agree. The defect pinned here is a command reporting success while writing broken files; that belongs in the run that gates the queue, not in the one that reports the next morning.

Verification

All on 38aa6ed2d, the branch head.

run result
pnpm --filter '@objectstack/cli^...' build (dependency closure) VERDICT command-exit 0
vitest --project unit — the 10 queue-tier files that reach generate.ts, incl. the new pin 10 files, 193 passed / 6 expected-fail
vitest --project integration — the new spawn pin 1 file, 11 passed
vitest --project integrationgenerate-string-family-width.pin.test.ts (kernel tier) 1 file, 28 passed
vitest test/vitest-tiers-partition.test.ts 22 passed
OS_TEST_TIERS=nightlygenerate-agent-retired.e2e, generate-skill.e2e, scaffold-emission-policy.e2e 3 files, 32 passed
pnpm --filter @objectstack/cli typecheck tsc --noEmit clean; check:test-typecheck OK, ledger unmoved
pnpm lint (repo-scoped, eslint . --no-inline-config) exit 0 — 6340 files, 0 errors, 0 warnings

Gates derived from the actual diff with node scripts/pm/dispatch-gates.mjs --commands, each run with its exit code captured before any pipe, then reconciled: ✓ dispatch-gates --ran: 59 derived famil(ies) accounted for — 59 run, 0 NOT-MEASURED. 58 green.

NOT MEASURED (1): pnpm check:dual-build-cjs-loads — exit 3, the gate's own stated reason: "PREREQUISITE NOT MET — this gate reads built output, and some package has no dist/ … Run pnpm build first. ⛔ This is NOT a pass: nothing was measured." It needs a whole-repo build; declared to CI. Three other gates first answered exit 3 / 1 for the same class of reason (check:i18n, check:i18n-coverage, check:i18n-walk-parity), and were re-run green after building the closure each names.

Five roster gates whose baselines sit under directories this diff touches were run beyond the derivation, since their silence is not evidence there: check:scaffold-emission-policy, check-changeset-fixed, check:authz-resolver, check:error-code-casing, check:filter-alias-parity — all green.

验收备注

Noted while measuring, ⛔ not filed and ⛔ not fixed here:

  • os g view class is accepted, and the barrel it writes is export { default as class } from './class.view';. That parses (a reserved word is legal as a module export alias), so this PR does not refuse it — but it is awkward to consume, and it is exactly the kind of case the open acceptance decision should rule on. Recorded here as evidence for that decision, not as a defect of this change.
  • os generate types derives INTERFACE_NAME + Record from the config's object names (generateTypesFromConfig) without a schema parse first. The spec constrains an object name to ^[a-z_][a-z0-9_]*$, so the door is gated upstream and this was not driven to a repro — hence not filed. The succeeding author is the one who settles the acceptance question above: it is the same command family and the same file.

Neither is a packages/spec change, neither widens an accepted set, and neither is touched by this diff.


Generated by Claude Code

`generate.ts` ran no name validation at all, so a name that is legal as a
name but not as an identifier was interpolated straight into a binding
position and written out under exit 0 — `const foo.bar: Data.ServiceObject
= {` plus a matching barrel re-export line: two files that are not
TypeScript, from a command that reported success.

Both emissions are now rendered once, at the single point where the derived
identifier is finished, and handed to TypeScript's own parser (reached
through `ts-morph`'s re-export, as `detect-free-identifiers.ts` already
does) before anything is written. On a parse failure the command prints the
compiler's own diagnostics per file and exits 1 without touching the
filesystem, `--dry-run` included. One check covers all 14 emission sites
across all 7 generators plus the barrel.

The criterion is parseability, not a charset: nothing is rewritten, and no
name that already produced parseable output is refused. Which names this
command should accept, and whether it should normalise them, stays an open
decision.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015QE8qk46e5CHJxyQEUjbf8
@github-actions github-actions Bot added the size/l label Sep 8, 2026
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

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

19 hand-written doc(s) name something this change touched — list omitted above 15 rows. Re-derive on the tree named below: node scripts/docs-audit/affected-docs.mjs --json be92d468043c1b9e100e6e0f2c2fc9216a5aa850.

1 release-owned page(s) also affected — read-only, see AGENTS.md Documentation Guardrails.

What this run could not see
  • 2 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 be92d468043c1b9e100e6e0f2c2fc9216a5aa850packageMentionDocs.

Which tree this was computed on

This run read content/docs from 48ae8c28e92ef330998c6af6edb31b8ce25cb9e3 — the merge of head 38aa6ed2db0db0d23bfbd693f365596ba453ef75 into base be92d468043c1b9e100e6e0f2c2fc9216a5aa850, 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 48ae8c28e92ef330998c6af6edb31b8ce25cb9e3 && git checkout 48ae8c28e92ef330998c6af6edb31b8ce25cb9e3
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin be92d468043c1b9e100e6e0f2c2fc9216a5aa850 38aa6ed2db0db0d23bfbd693f365596ba453ef75 && git checkout -B drift-repro be92d468043c1b9e100e6e0f2c2fc9216a5aa850 && git merge --no-ff 38aa6ed2db0db0d23bfbd693f365596ba453ef75

node scripts/docs-audit/affected-docs.mjs --json be92d468043c1b9e100e6e0f2c2fc9216a5aa850

⚠️ 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 be92d468043c1b9e100e6e0f2c2fc9216a5aa850 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@github-actions github-actions Bot added documentation Improvements or additions to documentation tests tooling labels Sep 8, 2026
@os-project-manager
os-project-manager marked this pull request as ready for review September 8, 2026 02:25
@os-project-manager
os-project-manager added this pull request to the merge queue Sep 8, 2026
Merged via the queue into main with commit 1ea349f Sep 8, 2026
35 checks passed
@os-project-manager
os-project-manager deleted the claude/issue-16541-generate-identifier-refusal branch September 8, 2026 02:52
os-justin pushed a commit that referenced this pull request Sep 10, 2026
`os generate <type> <name>` accepted any name at all until #16724 taught it
to refuse names whose emitted TypeScript does not parse. It now also refuses,
ahead of that check and ahead of every derivation, any name the object-`name`
declaration in `@objectstack/spec` rejects — maintainer ruling, decision batch
#82, option A: a gate, no sanitiser, no third charset.

The judge is the schema itself (`ObjectSchema.shape.name`), so the charset is
asked rather than transcribed, and the refusal quotes the schema's own message
so the rule the author is shown is the rule that judged them.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DapQyvYrFb1MxSYe7BL2nt
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/l tests tooling

Projects

None yet

2 participants