From 38aa6ed2db0db0d23bfbd693f365596ba453ef75 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 8 Sep 2026 01:22:01 +0000 Subject: [PATCH] fix(cli): `os generate` refuses to write TypeScript that does not parse MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 Claude-Session: https://claude.ai/code/session_015QE8qk46e5CHJxyQEUjbf8 --- .../generate-refuses-unparseable-emission.md | 12 + packages/cli/src/commands/generate.ts | 87 +++++++- .../cli/src/utils/emitted-source-parses.ts | 127 +++++++++++ .../cli/test/generate-emission-parses.test.ts | 196 ++++++++++++++++ .../generate-refuses-unparseable-name.test.ts | 209 ++++++++++++++++++ 5 files changed, 625 insertions(+), 6 deletions(-) create mode 100644 .changeset/generate-refuses-unparseable-emission.md create mode 100644 packages/cli/src/utils/emitted-source-parses.ts create mode 100644 packages/cli/test/generate-emission-parses.test.ts create mode 100644 packages/cli/test/generate-refuses-unparseable-name.test.ts diff --git a/.changeset/generate-refuses-unparseable-emission.md b/.changeset/generate-refuses-unparseable-emission.md new file mode 100644 index 0000000000..a9cd48c1f3 --- /dev/null +++ b/.changeset/generate-refuses-unparseable-emission.md @@ -0,0 +1,12 @@ +--- +"@objectstack/cli": patch +--- + +`os generate ` no longer exits 0 after writing TypeScript the compiler cannot parse. + +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. + +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. + +- **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. +- **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. diff --git a/packages/cli/src/commands/generate.ts b/packages/cli/src/commands/generate.ts index 4186c4592c..fe8d04fd50 100644 --- a/packages/cli/src/commands/generate.ts +++ b/packages/cli/src/commands/generate.ts @@ -20,6 +20,7 @@ import type { FieldType } from '@objectstack/spec/data'; import { isTenancyDisabled, isUniqueDeclared } from '@objectstack/spec/data'; import { printHeader, printSuccess, printError, printInfo, printStep, createTimer, CLI_ALIAS } from '../utils/format.js'; import { metadataFileName } from '../utils/metadata-file-name.js'; +import { findEmissionParseFailures } from '../utils/emitted-source-parses.js'; // ─── Metadata Type Templates ──────────────────────────────────────── @@ -699,12 +700,87 @@ async function runMetadataGeneration(type: string, name: string, flags: { dir?: console.log(` ${chalk.dim('File:')} ${chalk.white(path.join(dir, fileName))}`); console.log(''); + // Both emissions are rendered ONCE, here, and every branch below reuses + // them: the scaffold file, and the barrel re-export line. They are the two + // files one name reaches (#16541), and rendering them at the single point + // where the name has finished being derived is what lets one refusal cover + // all 14 emission sites across all 7 generators instead of 14 patches. + const content = generator.generate(name); + const exportLine = `export { default as ${toCamelCase(name)} } from '${moduleSpecifier}';`; + + // ⛔ REFUSE rather than rewrite (#16541). + // + // This command 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 line: two files that are + // not TypeScript, from a command that reported success. + // + // The criterion is PARSEABILITY, not a charset. `findEmissionParseFailures` + // asks the compiler about the bytes above and about nothing else, which is + // why it also covers what a rule about identifier characters would miss — + // a reserved word is illegal as a `const` binding and legal as an + // `export { default as … }` alias, and `${toCamelCase(name)}Views` parses + // for a name that bare `${toCamelCase(name)}` refuses. + // + // Which names this command should ACCEPT — and whether it should normalise + // the ones it does, the way `os create` derives its identifier since + // #15892 — is an OPEN decision. Sanitising here would answer it by quietly + // widening tolerance, and a legal-looking identifier derived from a name + // that should have been refused is the worse of the two failures. So + // nothing is rewritten, acceptance is unchanged for every name that already + // produced parseable output, and the refusal is loud. + // + // Placed AHEAD of the dry-run branch on purpose: a preview that prints + // un-parseable TypeScript and exits 0 is the same defect in preview form. + const parseFailures = await findEmissionParseFailures([ + { label: path.join(dir, fileName), source: content }, + { label: path.join(dir, 'index.ts'), source: exportLine }, + ]); + if (parseFailures.length > 0) { + printError('Refusing to generate — the TypeScript this would write does not parse'); + console.log(''); + console.log(` ${chalk.dim('Name:')} ${chalk.white(name)}`); + console.log(` ${chalk.dim('Identifier:')} ${chalk.white(toCamelCase(name))}`); + console.log(''); + for (const failure of parseFailures) { + console.log(` ${chalk.white(failure.label)}`); + for (const diagnostic of failure.diagnostics) { + console.log(chalk.dim(` ${diagnostic}`)); + } + } + console.log(''); + console.log(chalk.dim( + ` \`${CLI_ALIAS} g\` derives a TypeScript identifier from the name you give it, and`, + )); + console.log(chalk.dim( + ' this one is not something the compiler can parse — so what is listed above', + )); + console.log(chalk.dim( + ' would be written broken. Nothing was written.', + )); + console.log(''); + console.log(chalk.dim( + ' It refuses instead of rewriting your name into a legal-looking identifier,', + )); + console.log(chalk.dim( + ' which would decide in silence which names this command accepts. Pick a name', + )); + console.log(chalk.dim( + ` that survives as an identifier — \`${CLI_ALIAS} g ${type} order_line\` and`, + )); + console.log(chalk.dim( + ` \`${CLI_ALIAS} g ${type} order-line\` both work, and both fold to \`orderLine\`.`, + )); + console.log(''); + process.exit(1); + } + if (flags.dryRun) { printInfo('Dry run — no files written'); console.log(''); console.log(chalk.dim(' Content:')); console.log(chalk.dim(' ' + '-'.repeat(38))); - const content = generator.generate(name); for (const line of content.split('\n')) { console.log(chalk.dim(` ${line}`)); } @@ -725,8 +801,9 @@ async function runMetadataGeneration(type: string, name: string, flags: { dir?: fs.mkdirSync(fullDir, { recursive: true }); } - // Write file - const content = generator.generate(name); + // Write file — the same `content` the parse check above accepted, ⛔ not + // a re-render: a second call to `generator.generate` would make the + // bytes that were checked and the bytes that land two different things. fs.writeFileSync(filePath, content); printSuccess(`Created ${path.join(dir, fileName)}`); @@ -734,7 +811,6 @@ async function runMetadataGeneration(type: string, name: string, flags: { dir?: const indexPath = path.join(process.cwd(), dir, 'index.ts'); if (fs.existsSync(indexPath)) { const indexContent = fs.readFileSync(indexPath, 'utf-8'); - const exportLine = `export { default as ${toCamelCase(name)} } from '${moduleSpecifier}';`; if (!indexContent.includes(toCamelCase(name))) { fs.appendFileSync(indexPath, exportLine + '\n'); @@ -742,8 +818,7 @@ async function runMetadataGeneration(type: string, name: string, flags: { dir?: } } else { // Create barrel index - const exportLine = `export { default as ${toCamelCase(name)} } from '${moduleSpecifier}';\n`; - fs.writeFileSync(indexPath, exportLine); + fs.writeFileSync(indexPath, exportLine + '\n'); printSuccess(`Created ${dir}/index.ts`); } diff --git a/packages/cli/src/utils/emitted-source-parses.ts b/packages/cli/src/utils/emitted-source-parses.ts new file mode 100644 index 0000000000..a47cf374d6 --- /dev/null +++ b/packages/cli/src/utils/emitted-source-parses.ts @@ -0,0 +1,127 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Does the TypeScript a scaffolder is ABOUT TO WRITE actually parse? (#16541) + * + * ## The defect this exists to end + * + * `os generate ` derives a code identifier from `name` by + * camel-casing it, and ran NO validation of any kind on the result. So a name + * that is legal as a *name* but not as an *identifier* was interpolated + * straight into a binding position: + * + * os generate object foo.bar exit 0 + * src/objects/foo.bar.object.ts -> const foo.bar: Data.ServiceObject = { + * src/objects/index.ts -> export { default as foo.bar } from './foo.bar.object'; + * + * Two files, neither of them TypeScript, and a command that reported success. + * The next `tsc` is where the author finds out. + * + * ## ⛔ What this is NOT + * + * It is NOT a sanitiser and it is NOT a charset. It answers exactly one + * question — *do the bytes we are about to write parse?* — and the answer + * comes from the compiler rather than from an opinion about which characters + * are tasteful. Which names `os generate` should ACCEPT, and whether it should + * normalise the ones it does, is an open decision (#16541): deriving a + * legal-looking identifier from a name that should have been refused is the + * worse of the two failures, so this refuses loudly and rewrites nothing. + * + * ## Why the instrument is TypeScript's own parser + * + * "Does it look like an identifier" is the judgement that produced the defect + * in the first place, and every restatement of the grammar is a fresh chance + * to get it wrong: `${camel}Views` is fine for a name that `const ${camel}` + * refuses (`class`), a reserved word is legal as an `export { default as … }` + * alias and illegal as a `const` binding, and a name carrying a quote or a + * comment terminator breaks the emission without touching the identifier at + * all. Asking `ts` about the ACTUAL EMITTED BYTES answers all of those at once + * and restates none of them — the same reasoning, and the same instrument, + * that `create-plugin-identifier-parses.test.ts` (#15892) uses to pin the + * sibling door. + * + * Syntactic diagnostics only: `noLib` and `noResolve` keep the verdict about + * the grammar of these bytes. A scaffold references types it cannot resolve in + * a temp directory by design, and a resolution-aware verdict would refuse + * every name. + * + * ## Why `ts` arrives through a lazy import + * + * `ts-morph` is already a CLI runtime dependency and re-exports the full + * TypeScript compiler namespace, so we use its `ts` rather than adding a + * direct `typescript` dependency — the same call `detect-free-identifiers.ts` + * makes, and for the same reason. It is imported *inside* the check rather + * than at module top because the compiler is a heavy load and `os generate` + * has no other use for it: a command that is refused pays for the parser, a + * command that never reaches here does not. + */ + +import type { ts as TS } from 'ts-morph'; + +/** One file a command is about to write, and the bytes it would contain. */ +export interface EmittedSource { + /** Path as the author will see it, e.g. `src/objects/foo.bar.object.ts`. */ + label: string; + /** The exact bytes that would be written. */ + source: string; +} + +/** An emission the compiler cannot parse, with the compiler's own reasons. */ +export interface EmissionParseFailure { + label: string; + /** TypeScript's syntactic diagnostics, flattened to text, in source order. */ + diagnostics: string[]; +} + +/** + * The syntactic diagnostics TypeScript reports for `source`, flattened to text. + * + * Exported so a pin can reach the instrument the command actually uses instead + * of a second copy of it that could drift green. + */ +export function syntacticDiagnostics(ts: typeof TS, source: string): string[] { + const fileName = 'emitted.ts'; + const sourceFile = ts.createSourceFile( + fileName, + source, + ts.ScriptTarget.Latest, + true, + ts.ScriptKind.TS, + ); + const host: TS.CompilerHost = { + getSourceFile: (requested) => (requested === fileName ? sourceFile : undefined), + getDefaultLibFileName: () => 'lib.d.ts', + writeFile: () => {}, + getCurrentDirectory: () => '/', + getCanonicalFileName: (f) => f, + useCaseSensitiveFileNames: () => true, + getNewLine: () => '\n', + fileExists: (f) => f === fileName, + readFile: (f) => (f === fileName ? source : undefined), + }; + const program = ts.createProgram( + [fileName], + { noLib: true, noResolve: true, target: ts.ScriptTarget.Latest }, + host, + ); + return program + .getSyntacticDiagnostics(sourceFile) + .map((d) => ts.flattenDiagnosticMessageText(d.messageText, ' ')); +} + +/** + * Every emission in `emissions` the compiler refuses to parse, in the order + * given. An empty array means all of them parse — ⛔ never that the check was + * skipped: `emissions` is built from the same values the command writes. + */ +export async function findEmissionParseFailures( + emissions: readonly EmittedSource[], +): Promise { + const { ts } = await import('ts-morph'); + const failures: EmissionParseFailure[] = []; + for (const { label, source } of emissions) { + const diagnostics = syntacticDiagnostics(ts, source); + if (diagnostics.length > 0) failures.push({ label, diagnostics }); + } + return failures; +} diff --git a/packages/cli/test/generate-emission-parses.test.ts b/packages/cli/test/generate-emission-parses.test.ts new file mode 100644 index 0000000000..8dd89873af --- /dev/null +++ b/packages/cli/test/generate-emission-parses.test.ts @@ -0,0 +1,196 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * PIN (#16541) — what `os generate ` would write must PARSE, and + * the check that decides it is the one the command actually runs. + * + * ## The defect + * + * `generate.ts` ran NO name validation at all — no `validateProjectName`, no + * sanitiser — so its accepted set was strictly wider than `os create`'s, and + * the name went into a binding position untouched: + * + * os generate object foo.bar exit 0 + * src/objects/foo.bar.object.ts -> const foo.bar: Data.ServiceObject = { + * src/objects/index.ts -> export { default as foo.bar } from './foo.bar.object'; + * + * One name, TWO broken files, and a command that reported success. The blast + * radius is 14 emission sites across 7 generators plus the barrel the command + * rewrites, which is why the refusal lives at the single point where the name + * has finished being derived rather than at each site. + * + * ## ⛔ What this pin deliberately does NOT assert + * + * It does not assert any charset, and it does not assert a MAPPING from a name + * to a repaired identifier. `os create`'s sibling pin + * (`create-plugin-identifier-parses.test.ts`, #15892) does assert a mapping, + * because a maintainer ruling gave it one. No such ruling exists for + * `os generate`, whose starting point (no gate at all) differs from + * `create`'s (an npm-charset gate) — so the only property pinned here is the + * one that needs no adjudication: the command must not exit 0 having written + * TypeScript the compiler cannot parse. A future ruling may ADD a sanitiser or + * a gate on top; it must not turn this file green by making the refusal quiet. + * + * ## Why the roster is derived + * + * `GENERATOR_SCAFFOLD_TARGETS` is built from `GENERATORS` itself, so a + * generator added tomorrow is measured by this file on the day it lands rather + * than the day someone remembers to extend a list. The barrel line is rebuilt + * from `metadataFileName` for the same reason. + * + * ## The controls, and why there are three + * + * A parse check that resolves nothing reports zero diagnostics and reads + * exactly like a pass, so a green here is only worth something if the same + * instrument can be made to fail: + * + * - CONTROL — `order-line` must produce ZERO failures for every generator, + * and must still emit `orderLine`. This half is what a fix that narrowed + * acceptance too far would break, and a parse-only assertion would not + * notice. + * - CANARY — `foo.bar`, the card's measured name, must produce a failure for + * every generator AND for the barrel line. This is the reading that proves + * the harness is wired to real bytes. + * - DISCRIMINATOR — `class`. A rule written about identifier CHARACTERS + * passes it (every character is a letter) and a rule written about + * reserved words refuses it everywhere. Neither is right: what the emitted + * bytes do with it differs per generator and per emission position, and + * this row records the compiler's answer rather than an opinion. + */ + +import { describe, expect, it } from 'vitest'; +import { GENERATOR_SCAFFOLD_TARGETS } from '../src/commands/generate.js'; +import { metadataFileName } from '../src/utils/metadata-file-name.js'; +import { findEmissionParseFailures } from '../src/utils/emitted-source-parses.js'; + +/** + * `toSnakeCase` as `generate.ts` spells it. Restated rather than exported + * because exporting it would widen the command module's public surface for a + * test's convenience; it is three characters of regex and it is pinned by the + * filename assertion below, which fails if the two ever disagree. + */ +function toSnakeCase(str: string): string { + return str.replace(/[-]/g, '_').replace(/[A-Z]/g, (c) => `_${c.toLowerCase()}`).replace(/^_/, ''); +} + +/** `toCamelCase` as `generate.ts` spells it — hyphen AND underscore. */ +function toCamelCase(str: string): string { + return str.replace(/[-_]([a-z])/g, (_, c: string) => c.toUpperCase()); +} + +/** + * The two emissions one `os generate ` produces, built the way + * the command builds them. + */ +function emissionsFor(type: string, generate: (name: string) => string, name: string) { + const fileName = metadataFileName(type, toSnakeCase(name)); + if (fileName === null) throw new Error(`no file naming convention for type: ${type}`); + const moduleSpecifier = `./${fileName.replace(/\.ts$/, '')}`; + return { + fileName, + scaffold: { label: fileName, source: generate(name) }, + barrel: { + label: 'index.ts', + source: `export { default as ${toCamelCase(name)} } from '${moduleSpecifier}';`, + }, + }; +} + +const ROSTER = GENERATOR_SCAFFOLD_TARGETS.map((t) => ({ type: t.type, generate: t.generate })); + +describe('[#16541] the roster this pin runs over is derived from GENERATORS', () => { + it('covers the seven scaffolding subcommands, and any added later', () => { + // Not a frozen list: the assertion is that the derived roster is non-empty + // and that today's known types are IN it, so a new generator arrives + // measured rather than unmeasured. + const types = ROSTER.map((t) => t.type); + for (const known of ['object', 'view', 'action', 'flow', 'dashboard', 'app', 'skill']) { + expect(types).toContain(known); + } + expect(types.length).toBeGreaterThanOrEqual(7); + }); +}); + +describe('[#16541] CONTROL — an ordinary name still emits, and still parses', () => { + it.each(ROSTER)('$type emits parseable TypeScript for `order-line`', async ({ type, generate }) => { + const { scaffold, barrel } = emissionsFor(type, generate, 'order-line'); + expect(await findEmissionParseFailures([scaffold, barrel])).toEqual([]); + }); + + it.each(ROSTER)('$type still derives `orderLine` — acceptance and output are unmoved', ({ type, generate }) => { + const { scaffold, barrel } = emissionsFor(type, generate, 'order-line'); + expect(scaffold.source).toContain('orderLine'); + expect(barrel.source).toContain('export { default as orderLine }'); + }); + + it.each(ROSTER)('$type leaves the written filename as the registry derives it', ({ type, generate }) => { + const { fileName } = emissionsFor(type, generate, 'order-line'); + expect(fileName).toContain('order_line'); + }); +}); + +describe('[#16541] CANARY — the card`s measured name is refused, at both emissions', () => { + it.each(ROSTER)('$type refuses `foo.bar`', async ({ type, generate }) => { + const { scaffold, barrel } = emissionsFor(type, generate, 'foo.bar'); + const failures = await findEmissionParseFailures([scaffold, barrel]); + // Both files, not one: a barrel-only or scaffold-only reading would leave + // half the defect standing. + expect(failures.map((f) => f.label)).toEqual([scaffold.label, 'index.ts']); + for (const failure of failures) { + expect(failure.diagnostics.length, `${type} ${failure.label}`).toBeGreaterThan(0); + } + }); + + it('reports the exact pre-fix bytes the card measured as broken', async () => { + const object = ROSTER.find((t) => t.type === 'object'); + if (!object) throw new Error('the `object` generator is gone'); + const { scaffold } = emissionsFor('object', object.generate, 'foo.bar'); + expect(scaffold.source).toContain('const foo.bar: Data.ServiceObject = {'); + const failures = await findEmissionParseFailures([scaffold]); + expect(failures).toHaveLength(1); + expect(failures[0].diagnostics.length).toBeGreaterThan(0); + }); +}); + +describe('[#16541] DISCRIMINATOR — the verdict comes from the compiler, not from a charset', () => { + /** + * `class` is every-character-legal and reserved. What the emitted bytes do + * with it is measured here rather than asserted from the grammar: the + * verdicts below are the ones `ts` returned for these exact emissions, and a + * refusal rule written as a character class or as a reserved-word list would + * disagree with at least one of them. + */ + it('`class` is refused where it lands in a `const` binding', async () => { + const object = ROSTER.find((t) => t.type === 'object'); + if (!object) throw new Error('the `object` generator is gone'); + const { scaffold } = emissionsFor('object', object.generate, 'class'); + expect(scaffold.source).toContain('const class:'); + expect(await findEmissionParseFailures([scaffold])).not.toEqual([]); + }); + + it('`class` is accepted where the generator appends a suffix to it', async () => { + const view = ROSTER.find((t) => t.type === 'view'); + if (!view) throw new Error('the `view` generator is gone'); + const { scaffold } = emissionsFor('view', view.generate, 'class'); + expect(scaffold.source).toContain('const classViews:'); + expect(await findEmissionParseFailures([scaffold])).toEqual([]); + }); +}); + +describe('[#16541] the check answers about BYTES, so it also sees breakage off the identifier', () => { + /** + * A rule about identifier characters would pass both of these: neither name + * damages the identifier. They damage the string literal and the doc comment + * the same name is ALSO interpolated into, and the result is the same defect + * — `exit 0` on a file that is not TypeScript. + */ + it.each([ + { name: "a'b", why: 'a quote closes the emitted string literal early' }, + { name: 'a*/b', why: 'a comment terminator closes the emitted doc comment early' }, + ])('refuses `$name` ($why)', async ({ name }) => { + const object = ROSTER.find((t) => t.type === 'object'); + if (!object) throw new Error('the `object` generator is gone'); + const { scaffold } = emissionsFor('object', object.generate, name); + expect(await findEmissionParseFailures([scaffold])).not.toEqual([]); + }); +}); diff --git a/packages/cli/test/generate-refuses-unparseable-name.test.ts b/packages/cli/test/generate-refuses-unparseable-name.test.ts new file mode 100644 index 0000000000..2209ff9036 --- /dev/null +++ b/packages/cli/test/generate-refuses-unparseable-name.test.ts @@ -0,0 +1,209 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * PIN (#16541) — `os generate` must not EXIT 0 on a name whose emission does + * not parse, and must write nothing when it refuses. + * + * ## Why a child process, and why this file exists next to the unit pin + * + * `generate-emission-parses.test.ts` measures the check: given a name, do the + * bytes parse. It cannot measure the half this card is actually about — that + * the COMMAND consults it, before the writes, on every branch. The reported + * defect is an exit code (`os generate object foo.bar` -> exit 0 with two + * broken files on disk), and `process.exitCode` set inside a vitest worker is + * not an exit status: a CI script judges this command by `$?`. So the + * assertions here are on a real child process and on stdout, for the same two + * reasons `invocation-loudness.e2e.test.ts` documents at length — these + * commands print through `utils/format.ts`, whose `printError` writes to + * stdout. Spawned through `bin/run-dev.js` + tsx so the suite does not depend + * on `packages/cli/dist` having been built. + * + * ## ⛔ Why this file is NOT named `.e2e` + * + * The two cuts in `vitest-tiers.ts` are orthogonal and deliberately disagree: + * the BEHAVIOUR predicate puts this file in the `integration` project (it + * spawns), while the NAME decides which RUN collects it — `*.e2e.test.ts` is + * nightly, everything else is the queue's. The defect pinned here is a + * command that reports success while writing broken files, so it is pinned in + * the run that gates the merge queue rather than in the one that reports the + * next morning. The tiers module names this exact combination as sanctioned + * ("a file that spawns the CLI without the name is queue (name) AND + * `integration` (behaviour)"), and ⛔ nothing is renamed to make the two cuts + * agree. + * + * ## The control is load-bearing + * + * A refusal that fires on everything would satisfy every assertion about + * `foo.bar` and would be a worse command than the broken one. `order-line` + * runs the whole path — writes the scaffold, writes the barrel — and both of + * its files are re-read and re-parsed here, so "still works" is a reading + * rather than an exit code. + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { execFile } from 'node:child_process'; +import { existsSync, mkdtempSync, readFileSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join, resolve } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import ts from 'typescript'; +import { childEnv } from './helpers/serve-process.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 = 240_000; + +interface Run { + code: number; + stdout: string; + stderr: string; +} + +function runTsx(args: string[], cwd: string): Promise { + return new Promise((resolvePromise) => { + execFile( + TSX, + 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), + }); + }, + ); + }); +} + +/** Syntactic diagnostics only — the instrument #15892 introduced, unchanged. */ +function parseErrors(source: string): string[] { + const fileName = 'probe.ts'; + const sourceFile = ts.createSourceFile(fileName, source, ts.ScriptTarget.Latest, true, ts.ScriptKind.TS); + const host: ts.CompilerHost = { + getSourceFile: (requested) => (requested === fileName ? sourceFile : undefined), + getDefaultLibFileName: () => 'lib.d.ts', + writeFile: () => {}, + getCurrentDirectory: () => '/', + getCanonicalFileName: (f) => f, + useCaseSensitiveFileNames: () => true, + getNewLine: () => '\n', + fileExists: (f) => f === fileName, + readFile: (f) => (f === fileName ? source : undefined), + }; + const program = ts.createProgram([fileName], { noLib: true, noResolve: true, target: ts.ScriptTarget.Latest }, host); + return program.getSyntacticDiagnostics(sourceFile).map((d) => ts.flattenDiagnosticMessageText(d.messageText, ' ')); +} + +let refusedDir: string; +let controlDir: string; +let dryRunDir: string; + +let refused: Run; +let refusedAgain: Run; +let control: Run; +let dryRun: Run; + +beforeAll(async () => { + refusedDir = mkdtempSync(join(tmpdir(), 'os-g-refuse-')); + controlDir = mkdtempSync(join(tmpdir(), 'os-g-control-')); + dryRunDir = mkdtempSync(join(tmpdir(), 'os-g-dryrun-')); + + // Sequential on purpose: cold tsx starts, each loading every command module, + // in a container several agents share. + refused = await runTsx([CLI, 'generate', 'object', 'foo.bar'], refusedDir); + // A second generator, to show the refusal is not one patched call site. + refusedAgain = await runTsx([CLI, 'generate', 'flow', 'foo.bar'], refusedDir); + dryRun = await runTsx([CLI, 'generate', 'object', 'foo.bar', '--dry-run'], dryRunDir); + control = await runTsx([CLI, 'generate', 'object', 'order-line'], controlDir); +}, RUN_TIMEOUT_MS); + +afterAll(() => { + for (const dir of [refusedDir, controlDir, dryRunDir]) { + rmSync(dir, { recursive: true, force: true }); + } +}); + +describe('[#16541] `os generate object foo.bar` refuses instead of exiting 0', () => { + it('exits non-zero — the reported defect was exit 0', () => { + expect(refused.code).not.toBe(0); + expect(refused.code).toBe(1); + }); + + it('says it is refusing, and says the emission does not parse', () => { + expect(refused.stdout).toContain('Refusing to generate'); + expect(refused.stdout).toContain('does not parse'); + }); + + it('names BOTH files the name would have corrupted', () => { + expect(refused.stdout).toContain('foo.bar.object.ts'); + expect(refused.stdout).toContain('index.ts'); + }); + + it('quotes the compiler`s own diagnostic rather than a restatement of it', () => { + // The message TypeScript emits for a property access in a binding + // position. Asserted because a hand-written "invalid name" line would pass + // every other assertion in this block. + expect(refused.stdout).toContain("',' expected."); + }); + + it('⛔ does not rewrite the name into a legal-looking identifier', () => { + // The silent-sanitiser outcome this card exists to refuse: `fooBar` must + // not appear anywhere in the output, and nothing may be reported created. + expect(refused.stdout).not.toContain('fooBar'); + expect(refused.stdout).not.toContain('Created'); + }); + + it('writes nothing — no scaffold, no barrel, no directory', () => { + expect(existsSync(join(refusedDir, 'src', 'objects', 'foo.bar.object.ts'))).toBe(false); + expect(existsSync(join(refusedDir, 'src', 'objects', 'index.ts'))).toBe(false); + expect(existsSync(join(refusedDir, 'src'))).toBe(false); + }); +}); + +describe('[#16541] the refusal is one chokepoint, not one patched generator', () => { + it('`os generate flow foo.bar` is refused the same way', () => { + expect(refusedAgain.code).toBe(1); + expect(refusedAgain.stdout).toContain('Refusing to generate'); + expect(existsSync(join(refusedDir, 'src', 'flows'))).toBe(false); + }); +}); + +describe('[#16541] `--dry-run` refuses too', () => { + it('does not print un-parseable TypeScript under exit 0', () => { + // A preview that renders the broken file and exits 0 is the same defect in + // preview form — the author copies it, or a script trusts the status. + expect(dryRun.code).toBe(1); + expect(dryRun.stdout).toContain('Refusing to generate'); + expect(dryRun.stdout).not.toContain('const foo.bar'); + }); +}); + +describe('[#16541] CONTROL — an ordinary name is untouched by this change', () => { + it('still exits 0 and reports both writes', () => { + expect(control.code).toBe(0); + expect(control.stdout).toContain('Created src/objects/order_line.object.ts'); + expect(control.stdout).toContain('Created src/objects/index.ts'); + }); + + it('writes a scaffold that parses, still binding `orderLine`', () => { + const scaffold = readFileSync(join(controlDir, 'src', 'objects', 'order_line.object.ts'), 'utf8'); + expect(parseErrors(scaffold)).toEqual([]); + expect(scaffold).toContain('const orderLine: Data.ServiceObject = {'); + }); + + it('writes a barrel that parses, still re-exporting `orderLine`', () => { + const barrel = readFileSync(join(controlDir, 'src', 'objects', 'index.ts'), 'utf8'); + expect(parseErrors(barrel)).toEqual([]); + expect(barrel).toBe("export { default as orderLine } from './order_line.object';\n"); + }); +});