Skip to content

Commit 1ea349f

Browse files
claude[bot]claude
claude[bot]
and
claude
authored
fix(cli): os generate refuses to write TypeScript that does not parse (#16724)
`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. Claude-Session: https://claude.ai/code/session_015QE8qk46e5CHJxyQEUjbf8 Co-authored-by: Claude <noreply@anthropic.com>
1 parent 1b25ced commit 1ea349f

5 files changed

Lines changed: 625 additions & 6 deletions

File tree

‎.changeset/generate-refuses-unparseable-emission.md‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
---
2+
"@objectstack/cli": patch
3+
---
4+
5+
`os generate <type> <name>` no longer exits 0 after writing TypeScript the compiler cannot parse.
6+
7+
The command 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 broken files behind: `const foo.bar: Data.ServiceObject = {` in `src/objects/foo.bar.object.ts`, and a matching `export { default as foo.bar } from './foo.bar.object';` appended to the barrel `src/objects/index.ts`. The author learned about it at the next `tsc`, in a file the scaffolder had just told them it created.
8+
9+
Both emissions 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 for each affected file and exits 1 without touching the filesystem — including under `--dry-run`, where a preview of un-parseable output under exit 0 is the same defect in preview form. One check covers all 14 emission sites across all 7 generators (`object`, `view`, `action`, `flow`, `dashboard`, `app`, `skill`) plus the barrel, and a generator added later inherits it.
10+
11+
- **The criterion is parseability, not a charset.** Nothing is rewritten and no name that already produced parseable output is refused: the accepted set moves only by the names whose emission was already broken. Which names `os generate` should accept — and whether it should normalise the ones it does, the way `os create` derives its identifier — is a separate, open decision. Deriving a legal-looking identifier from a name that should have been refused is the worse of the two failures, so this refuses loudly rather than answering that question by widening tolerance.
12+
- **Asking the compiler is what makes the check correct per emission position.** A rule about identifier characters, or about reserved words, gets this wrong in both directions: `os generate object class` is refused (`const class:` is not a declaration) while `os generate view class` is accepted (that generator emits `const classViews:`), and a name carrying a quote or a comment terminator breaks the emitted file without touching the identifier at all.

‎packages/cli/src/commands/generate.ts‎

Lines changed: 81 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import type { FieldType } from '@objectstack/spec/data';
2020
import { isTenancyDisabled, isUniqueDeclared } from '@objectstack/spec/data';
2121
import { printHeader, printSuccess, printError, printInfo, printStep, createTimer, CLI_ALIAS } from '../utils/format.js';
2222
import { metadataFileName } from '../utils/metadata-file-name.js';
23+
import { findEmissionParseFailures } from '../utils/emitted-source-parses.js';
2324

2425
// ─── Metadata Type Templates ────────────────────────────────────────
2526

@@ -699,12 +700,87 @@ async function runMetadataGeneration(type: string, name: string, flags: { dir?:
699700
console.log(` ${chalk.dim('File:')} ${chalk.white(path.join(dir, fileName))}`);
700701
console.log('');
701702

703+
// Both emissions are rendered ONCE, here, and every branch below reuses
704+
// them: the scaffold file, and the barrel re-export line. They are the two
705+
// files one name reaches (#16541), and rendering them at the single point
706+
// where the name has finished being derived is what lets one refusal cover
707+
// all 14 emission sites across all 7 generators instead of 14 patches.
708+
const content = generator.generate(name);
709+
const exportLine = `export { default as ${toCamelCase(name)} } from '${moduleSpecifier}';`;
710+
711+
// ⛔ REFUSE rather than rewrite (#16541).
712+
//
713+
// This command ran no name validation at all, so a name that is legal as a
714+
// NAME but not as an IDENTIFIER was interpolated straight into a binding
715+
// position and written out under `exit 0` — `const foo.bar:
716+
// Data.ServiceObject = {`, plus a matching barrel line: two files that are
717+
// not TypeScript, from a command that reported success.
718+
//
719+
// The criterion is PARSEABILITY, not a charset. `findEmissionParseFailures`
720+
// asks the compiler about the bytes above and about nothing else, which is
721+
// why it also covers what a rule about identifier characters would miss —
722+
// a reserved word is illegal as a `const` binding and legal as an
723+
// `export { default as … }` alias, and `${toCamelCase(name)}Views` parses
724+
// for a name that bare `${toCamelCase(name)}` refuses.
725+
//
726+
// Which names this command should ACCEPT — and whether it should normalise
727+
// the ones it does, the way `os create` derives its identifier since
728+
// #15892 — is an OPEN decision. Sanitising here would answer it by quietly
729+
// widening tolerance, and a legal-looking identifier derived from a name
730+
// that should have been refused is the worse of the two failures. So
731+
// nothing is rewritten, acceptance is unchanged for every name that already
732+
// produced parseable output, and the refusal is loud.
733+
//
734+
// Placed AHEAD of the dry-run branch on purpose: a preview that prints
735+
// un-parseable TypeScript and exits 0 is the same defect in preview form.
736+
const parseFailures = await findEmissionParseFailures([
737+
{ label: path.join(dir, fileName), source: content },
738+
{ label: path.join(dir, 'index.ts'), source: exportLine },
739+
]);
740+
if (parseFailures.length > 0) {
741+
printError('Refusing to generate — the TypeScript this would write does not parse');
742+
console.log('');
743+
console.log(` ${chalk.dim('Name:')} ${chalk.white(name)}`);
744+
console.log(` ${chalk.dim('Identifier:')} ${chalk.white(toCamelCase(name))}`);
745+
console.log('');
746+
for (const failure of parseFailures) {
747+
console.log(` ${chalk.white(failure.label)}`);
748+
for (const diagnostic of failure.diagnostics) {
749+
console.log(chalk.dim(` ${diagnostic}`));
750+
}
751+
}
752+
console.log('');
753+
console.log(chalk.dim(
754+
` \`${CLI_ALIAS} g\` derives a TypeScript identifier from the name you give it, and`,
755+
));
756+
console.log(chalk.dim(
757+
' this one is not something the compiler can parse — so what is listed above',
758+
));
759+
console.log(chalk.dim(
760+
' would be written broken. Nothing was written.',
761+
));
762+
console.log('');
763+
console.log(chalk.dim(
764+
' It refuses instead of rewriting your name into a legal-looking identifier,',
765+
));
766+
console.log(chalk.dim(
767+
' which would decide in silence which names this command accepts. Pick a name',
768+
));
769+
console.log(chalk.dim(
770+
` that survives as an identifier — \`${CLI_ALIAS} g ${type} order_line\` and`,
771+
));
772+
console.log(chalk.dim(
773+
` \`${CLI_ALIAS} g ${type} order-line\` both work, and both fold to \`orderLine\`.`,
774+
));
775+
console.log('');
776+
process.exit(1);
777+
}
778+
702779
if (flags.dryRun) {
703780
printInfo('Dry run — no files written');
704781
console.log('');
705782
console.log(chalk.dim(' Content:'));
706783
console.log(chalk.dim(' ' + '-'.repeat(38)));
707-
const content = generator.generate(name);
708784
for (const line of content.split('\n')) {
709785
console.log(chalk.dim(` ${line}`));
710786
}
@@ -725,25 +801,24 @@ async function runMetadataGeneration(type: string, name: string, flags: { dir?:
725801
fs.mkdirSync(fullDir, { recursive: true });
726802
}
727803

728-
// Write file
729-
const content = generator.generate(name);
804+
// Write file — the same `content` the parse check above accepted, ⛔ not
805+
// a re-render: a second call to `generator.generate` would make the
806+
// bytes that were checked and the bytes that land two different things.
730807
fs.writeFileSync(filePath, content);
731808
printSuccess(`Created ${path.join(dir, fileName)}`);
732809

733810
// Check for barrel index
734811
const indexPath = path.join(process.cwd(), dir, 'index.ts');
735812
if (fs.existsSync(indexPath)) {
736813
const indexContent = fs.readFileSync(indexPath, 'utf-8');
737-
const exportLine = `export { default as ${toCamelCase(name)} } from '${moduleSpecifier}';`;
738814

739815
if (!indexContent.includes(toCamelCase(name))) {
740816
fs.appendFileSync(indexPath, exportLine + '\n');
741817
printSuccess(`Updated ${dir}/index.ts with export`);
742818
}
743819
} else {
744820
// Create barrel index
745-
const exportLine = `export { default as ${toCamelCase(name)} } from '${moduleSpecifier}';\n`;
746-
fs.writeFileSync(indexPath, exportLine);
821+
fs.writeFileSync(indexPath, exportLine + '\n');
747822
printSuccess(`Created ${dir}/index.ts`);
748823
}
749824

‎packages/cli/src/utils/emitted-source-parses.ts‎

Lines changed: 127 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,127 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* Does the TypeScript a scaffolder is ABOUT TO WRITE actually parse? (#16541)
5+
*
6+
* ## The defect this exists to end
7+
*
8+
* `os generate <type> <name>` derives a code identifier from `name` by
9+
* camel-casing it, and ran NO validation of any kind on the result. So a name
10+
* that is legal as a *name* but not as an *identifier* was interpolated
11+
* straight into a binding position:
12+
*
13+
* os generate object foo.bar exit 0
14+
* src/objects/foo.bar.object.ts -> const foo.bar: Data.ServiceObject = {
15+
* src/objects/index.ts -> export { default as foo.bar } from './foo.bar.object';
16+
*
17+
* Two files, neither of them TypeScript, and a command that reported success.
18+
* The next `tsc` is where the author finds out.
19+
*
20+
* ## ⛔ What this is NOT
21+
*
22+
* It is NOT a sanitiser and it is NOT a charset. It answers exactly one
23+
* question — *do the bytes we are about to write parse?* — and the answer
24+
* comes from the compiler rather than from an opinion about which characters
25+
* are tasteful. Which names `os generate` should ACCEPT, and whether it should
26+
* normalise the ones it does, is an open decision (#16541): deriving a
27+
* legal-looking identifier from a name that should have been refused is the
28+
* worse of the two failures, so this refuses loudly and rewrites nothing.
29+
*
30+
* ## Why the instrument is TypeScript's own parser
31+
*
32+
* "Does it look like an identifier" is the judgement that produced the defect
33+
* in the first place, and every restatement of the grammar is a fresh chance
34+
* to get it wrong: `${camel}Views` is fine for a name that `const ${camel}`
35+
* refuses (`class`), a reserved word is legal as an `export { default as … }`
36+
* alias and illegal as a `const` binding, and a name carrying a quote or a
37+
* comment terminator breaks the emission without touching the identifier at
38+
* all. Asking `ts` about the ACTUAL EMITTED BYTES answers all of those at once
39+
* and restates none of them — the same reasoning, and the same instrument,
40+
* that `create-plugin-identifier-parses.test.ts` (#15892) uses to pin the
41+
* sibling door.
42+
*
43+
* Syntactic diagnostics only: `noLib` and `noResolve` keep the verdict about
44+
* the grammar of these bytes. A scaffold references types it cannot resolve in
45+
* a temp directory by design, and a resolution-aware verdict would refuse
46+
* every name.
47+
*
48+
* ## Why `ts` arrives through a lazy import
49+
*
50+
* `ts-morph` is already a CLI runtime dependency and re-exports the full
51+
* TypeScript compiler namespace, so we use its `ts` rather than adding a
52+
* direct `typescript` dependency — the same call `detect-free-identifiers.ts`
53+
* makes, and for the same reason. It is imported *inside* the check rather
54+
* than at module top because the compiler is a heavy load and `os generate`
55+
* has no other use for it: a command that is refused pays for the parser, a
56+
* command that never reaches here does not.
57+
*/
58+
59+
import type { ts as TS } from 'ts-morph';
60+
61+
/** One file a command is about to write, and the bytes it would contain. */
62+
export interface EmittedSource {
63+
/** Path as the author will see it, e.g. `src/objects/foo.bar.object.ts`. */
64+
label: string;
65+
/** The exact bytes that would be written. */
66+
source: string;
67+
}
68+
69+
/** An emission the compiler cannot parse, with the compiler's own reasons. */
70+
export interface EmissionParseFailure {
71+
label: string;
72+
/** TypeScript's syntactic diagnostics, flattened to text, in source order. */
73+
diagnostics: string[];
74+
}
75+
76+
/**
77+
* The syntactic diagnostics TypeScript reports for `source`, flattened to text.
78+
*
79+
* Exported so a pin can reach the instrument the command actually uses instead
80+
* of a second copy of it that could drift green.
81+
*/
82+
export function syntacticDiagnostics(ts: typeof TS, source: string): string[] {
83+
const fileName = 'emitted.ts';
84+
const sourceFile = ts.createSourceFile(
85+
fileName,
86+
source,
87+
ts.ScriptTarget.Latest,
88+
true,
89+
ts.ScriptKind.TS,
90+
);
91+
const host: TS.CompilerHost = {
92+
getSourceFile: (requested) => (requested === fileName ? sourceFile : undefined),
93+
getDefaultLibFileName: () => 'lib.d.ts',
94+
writeFile: () => {},
95+
getCurrentDirectory: () => '/',
96+
getCanonicalFileName: (f) => f,
97+
useCaseSensitiveFileNames: () => true,
98+
getNewLine: () => '\n',
99+
fileExists: (f) => f === fileName,
100+
readFile: (f) => (f === fileName ? source : undefined),
101+
};
102+
const program = ts.createProgram(
103+
[fileName],
104+
{ noLib: true, noResolve: true, target: ts.ScriptTarget.Latest },
105+
host,
106+
);
107+
return program
108+
.getSyntacticDiagnostics(sourceFile)
109+
.map((d) => ts.flattenDiagnosticMessageText(d.messageText, ' '));
110+
}
111+
112+
/**
113+
* Every emission in `emissions` the compiler refuses to parse, in the order
114+
* given. An empty array means all of them parse — ⛔ never that the check was
115+
* skipped: `emissions` is built from the same values the command writes.
116+
*/
117+
export async function findEmissionParseFailures(
118+
emissions: readonly EmittedSource[],
119+
): Promise<EmissionParseFailure[]> {
120+
const { ts } = await import('ts-morph');
121+
const failures: EmissionParseFailure[] = [];
122+
for (const { label, source } of emissions) {
123+
const diagnostics = syntacticDiagnostics(ts, source);
124+
if (diagnostics.length > 0) failures.push({ label, diagnostics });
125+
}
126+
return failures;
127+
}

0 commit comments

Comments
 (0)