From 4481b402fb940f5a21d363bcc232809c2e053ae8 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 8 Sep 2026 12:29:50 +0000 Subject: [PATCH] fix(cli): the i18n provenance companion accompanies a module, and names its sections from their payloads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `os i18n extract --source-hashes` narrowed the provenance table to "the sections this run commits" and built that list from two literals. The half deciding WHICH modules were emitted already read the emitted set; the half naming them pushed 'objects' / 'metadataForms'. Two consequences, one cause: - With no module emitted the list is empty, narrowToCommittedSections returns {}, and {} is truthy at the emit gate — so the run wrote a zero-record companion with no bundle module beside it. Because --check compares the companion by bytes like any other emitted file, that orphan once committed is a file the gate demands forever. - Under kind: 'stack' the module holds every group the stack authors while the caller named one of them — correct only for as long as no other section can reach the table. translationModuleSections() now sits beside translationModulePayload and is switched on the same kind, so what a module holds and which sections it commits are one decision. The emit gate returns undefined for an empty set. No provenance record moves in this repository: the tables only ever carry the two sections collectFilledFromHashes walks, so 'objects' was the right name for both stack sub-tree modes — correct by coincidence, now by construction. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015QE8qk46e5CHJxyQEUjbf8 --- ...-extract-companion-accompanies-a-module.md | 14 + packages/cli/src/commands/i18n/extract.ts | 40 ++- packages/cli/src/utils/i18n-extract.ts | 46 ++++ .../i18n-extract-companion-orphan.test.ts | 258 ++++++++++++++++++ .../test/i18n-extract-source-hashes.test.ts | 53 ++++ 5 files changed, 406 insertions(+), 5 deletions(-) create mode 100644 .changeset/i18n-extract-companion-accompanies-a-module.md create mode 100644 packages/cli/test/i18n-extract-companion-orphan.test.ts diff --git a/.changeset/i18n-extract-companion-accompanies-a-module.md b/.changeset/i18n-extract-companion-accompanies-a-module.md new file mode 100644 index 0000000000..a7998a45ec --- /dev/null +++ b/.changeset/i18n-extract-companion-accompanies-a-module.md @@ -0,0 +1,14 @@ +--- +"@objectstack/cli": patch +--- + +`os i18n extract --source-hashes` no longer writes a provenance companion with no bundle module beside it, and names the sections it commits from the payloads those modules hold instead of from two literals. + +The command narrows the provenance table to "the sections this run commits" before writing `.source-hashes.generated.ts`. The half that decided WHICH modules were emitted already read the emitted set; the half that named them pushed the string `'objects'` or `'metadataForms'`. + +- **A zero-record orphan is no longer written.** With no module emitted for a locale — a stack whose only surface is apps, under the default `--objects-only` with `--no-metadata-forms` — the committed-section list is empty, `narrowToCommittedSections` returns `{}`, and `{}` is truthy at the emit gate. The run therefore wrote one file holding an empty table, describing nothing, with no bundle module beside it for it to be about. Because `--check` compares the companion by bytes like any other emitted file, that orphan once committed is a file the gate demands forever: deleting it made `--check` report `missing` and exit 1. Such a run now writes nothing, and reports `Generated 0 file(s)`. +- **The section list is derived.** `translationModuleSections(bundle, kind)` sits beside `translationModulePayload` and is switched on the same `kind`, so what a module holds and which sections it commits are one decision rather than two. Under `kind: 'stack'` the module holds every group the stack authors and the caller now names all of them; a group added later needs no edit, and a further aggregate kind fails to compile at that one site rather than silently committing its own name as a section. + +**No provenance record changes in this repository, and none is restored.** The generated tables only ever carry the two sections `collectFilledFromHashes` walks (`objects` and `metadataForms`), so `'objects'` was the right name for both stack sub-tree modes — the old list was correct by coincidence, not by construction. In particular an `apps.*` record is not restored by this change: no such record is built, so none was being filtered out. + +**Already committed an empty companion?** Nothing needs doing and nothing is deleted. `--check` compares only the files a run writes and reports `missing` / `stale` over that set, so a leftover empty companion is in neither category — it is tolerated where it sits, and is inert to the next extract, which reads it back as an empty record set exactly as it would read its absence. Delete it at your convenience. diff --git a/packages/cli/src/commands/i18n/extract.ts b/packages/cli/src/commands/i18n/extract.ts index 997160d6b2..068a1fc0de 100644 --- a/packages/cli/src/commands/i18n/extract.ts +++ b/packages/cli/src/commands/i18n/extract.ts @@ -24,6 +24,7 @@ import { parseSourceHashModule, narrowToCommittedSections, translationModulePayload, + translationModuleSections, countTranslationLeaves, type FillStrategy, type TranslationModuleKind, @@ -448,15 +449,44 @@ export default class I18nExtract extends Command { * bundle files — {@link emittedModules} — never by a second rule. A set that commits both — * `platform-objects` is the one today — keeps every record it had. The * narrowing itself is `narrowToCommittedSections`, a pure function in the - * extractor's utils so it can be pinned without driving oclif; this layer - * contributes only the two booleans it alone knows. + * extractor's utils so it can be pinned without driving oclif. + * + * ⭐ And the sections are read off the PAYLOADS those modules hold + * (`translationModuleSections`), not written here as literals. This layer + * used to push `'objects'` and `'metadataForms'` — the emitted-module half + * already read `emittedModules`, but what it pushed was a hand-copied + * name, so under `kind: 'stack'` it named one of the several groups the + * module actually commits. Nothing in this repository's provenance tables + * is filtered by that mismatch today, because the tables only ever carry + * the two GENERATED sections (`GENERATED_SECTIONS` in + * `@objectstack/platform-objects`), and `'objects'` is the right name for + * both stack kinds — the list was correct by COINCIDENCE, not by + * construction, and a third generated section would have broken it + * silently. It is now derived. + * + * ⭐ Returning `undefined` when nothing is committed is the second half, + * and it is a file-set decision rather than a narrowing one: + * `narrowToCommittedSections` returns `{}` for an empty section set, `{}` + * is truthy at the emit site, and the run therefore wrote a zero-record + * companion with NO bundle module beside it for it to be about. `--check` + * compares the companion by bytes like any other emitted file, so that + * orphan, once committed, is a file the gate demands forever. */ const committedSourceHashes = (locale: string): Record | undefined => { const table = result.sourceHashes[locale]; if (!table) return undefined; - const committed: string[] = []; - if (emittedModules(locale).some((m) => m.kind !== 'metadataForms')) committed.push('objects'); - if (emittedModules(locale).some((m) => m.kind === 'metadataForms')) committed.push('metadataForms'); + const committed = new Set(); + for (const mod of emittedModules(locale)) { + for (const section of translationModuleSections(result.bundles[locale], mod.kind)) { + committed.add(section); + } + } + // No module is committed for this locale, so there is nothing beside a + // companion for it to be ABOUT — and an orphan is worse than nothing: + // `--check` compares by bytes against the emitted list, so a zero-record + // companion written once is a file the gate demands forever. `{}` is + // truthy, so returning the narrowed table here wrote exactly that. + if (committed.size === 0) return undefined; return narrowToCommittedSections(table, committed); }; diff --git a/packages/cli/src/utils/i18n-extract.ts b/packages/cli/src/utils/i18n-extract.ts index 2bb6acd745..824a392941 100644 --- a/packages/cli/src/utils/i18n-extract.ts +++ b/packages/cli/src/utils/i18n-extract.ts @@ -1904,6 +1904,52 @@ export function translationModulePayload( : stackAuthoredSubtree(data); } +/** + * The `TranslationData` SECTIONS a module of the given kind COVERS — the first + * dotted segment its leaves carry, which is the identity + * {@link narrowToCommittedSections} narrows a provenance table by. + * + * Beside {@link translationModulePayload} and switched on the same `kind`, so + * "what a module holds" and "which sections it commits" are ONE decision. The + * caller was two literals — `committed.push('objects')` and + * `committed.push('metadataForms')`, chosen by which modules were emitted — and + * a literal cannot follow the payload: under `kind: 'stack'` the module holds + * every group the stack authors (`objects`, `apps`, `dashboards`, ...) while the + * caller named exactly one of them. Replacing that list with a longer list of + * literals only moves the day it goes wrong to the next group added. + * + * ⚠️ The sections are NOT simply the payload's own top-level keys, and reading + * them off it would be WRONG for two of the three kinds. `'objects'` and + * `'metadataForms'` select the sub-tree ROOTED AT one section, so those keys are + * object and form names (`kpi_metric`) — deriving the section list from them + * would commit `['kpi_metric']` and narrow away every `objects.*` record, which + * is the one path this repository's single `--source-hashes` config is on. + * `'stack'` selects a `TranslationData`-shaped subtree, so THERE the top-level + * keys are sections — every group the stack authors, today's and any added + * later, with nothing here to update. + * + * The switch is exhaustive on purpose: a fourth kind that selects one section + * needs no edit (`[kind]` already names it), and a fourth AGGREGATE kind fails + * to compile here rather than silently committing its own name as a section. + */ +export function translationModuleSections( + data: TranslationData, + kind: TranslationModuleKind, +): string[] { + switch (kind) { + case 'stack': + return Object.keys(stackAuthoredSubtree(data)); + case 'objects': + case 'metadataForms': + // The selector's own name IS the section it roots at. + return [kind]; + default: { + const exhaustive: never = kind; + return exhaustive; + } + } +} + /** * String leaves under a nested translation tree. * diff --git a/packages/cli/test/i18n-extract-companion-orphan.test.ts b/packages/cli/test/i18n-extract-companion-orphan.test.ts new file mode 100644 index 0000000000..8df9fd1645 --- /dev/null +++ b/packages/cli/test/i18n-extract-companion-orphan.test.ts @@ -0,0 +1,258 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The provenance companion accompanies the bundle modules beside it, and its + * section list is read off their payloads (#16242). + * + * ## What was wrong + * + * `os i18n extract --source-hashes` narrowed the provenance table to "the + * sections this run commits", and built that list from two LITERALS: + * + * if (emittedModules(locale).some((m) => m.kind !== 'metadataForms')) committed.push('objects'); + * if (emittedModules(locale).some((m) => m.kind === 'metadataForms')) committed.push('metadataForms'); + * + * The "which modules were emitted" half already read the emitted set; what it + * PUSHED was a hand-copied name. Two consequences, one cause: + * + * 1. under `kind: 'stack'` the module holds every group the stack authors + * (`objects`, `apps`, `dashboards`, ...) while the caller named one of + * them — a list that is correct only for as long as no other section can + * appear in the table; + * 2. with NO module emitted the list is empty, `narrowToCommittedSections` + * returns `{}`, and `{}` is truthy at the emit site — so the run wrote a + * zero-record companion with no bundle module beside it for it to be + * about, and `--check` compares the companion by bytes like any other + * emitted file, so that orphan once committed is a file the gate demands + * forever. + * + * ## Why these cases drive the real CLI + * + * The defect is about WHICH FILES EXIST, decided apart from the modules they + * accompany. A mirror of the emit rule — the shape + * `i18n-extract-emitted-files.test.ts` uses deliberately — cannot see it: it + * re-implements the rule, so it agrees with the rule by construction and would + * have written the orphan too. Every case here therefore reads the real + * directory the real command wrote. + * + * ⚠️ Symptom 1 as the card diagnosed it — an `apps.*` provenance record being + * FILTERED OUT — does not occur, and the third case below is the measurement + * that says so rather than a pin of a repair. The provenance table is built by + * `collectFilledFromHashes`, which walks `GENERATED_SECTIONS` + * (`['objects', 'metadataForms']`) in `@objectstack/platform-objects`, so an + * `apps.*` record never enters the table to be dropped. What the derived list + * buys is that the caller's statement stops being true by coincidence. + * + * ## Tier, and why the name carries no `.e2e` + * + * This file spawns the real CLI, so the behavioural predicate in + * `../vitest-tiers.ts` puts it in the `integration` PROJECT. The `.e2e` NAME + * would additionally move it into the `OS_TEST_TIERS=nightly` POPULATION — + * the two cuts are independent, and the name is the one that decides the RUN. + * A `--check` semantics guard belongs in the queue's run, so it is named like + * its siblings `i18n-extract-check-json.test.ts` and + * `i18n-extract-check-dry-run.test.ts`, which spawn the CLI under the same + * cut, and unlike `i18n-extract-key-count.e2e.test.ts`, which is nightly. + * + * ## Fixture placement + * + * As in `i18n-extract-key-count.e2e.test.ts`: the stack config goes under this + * package's git-ignored `tmp/` so that `bundle-require`'s bundled module + * resolves the bare `@objectstack/spec` specifier out of this package's real + * `node_modules`, and the `--out` root goes in the system temp dir. `afterAll` + * removes only this suite's own `mkdtemp` directories — several suites share + * that root and run concurrently. + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { execFileSync } from 'node:child_process'; +import { mkdirSync, mkdtempSync, rmSync, writeFileSync, readdirSync, readFileSync, existsSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join, resolve } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { childEnv } from './helpers/serve-process.js'; + +const HERE = resolve(fileURLToPath(import.meta.url), '..'); +const CLI = resolve(HERE, '../bin/run-dev.js'); +const TSX = resolve(HERE, '../../../node_modules/.bin/tsx'); +const CLI_PACKAGE_ROOT = resolve(HERE, '..'); + +/** One object (2 leaves) + one app (1 leaf); the registry baseline rides along. */ +const STACK_CONFIG = [ + "import { defineStack } from '@objectstack/spec';", + '', + 'export default defineStack({', + " i18n: { defaultLocale: 'zh-CN', supportedLocales: ['zh-CN'] },", + " objects: [{ name: 'kpi_metric', label: 'Metric', fields: { name: { type: 'text', label: 'Name' } } }],", + " apps: [{ name: 'kpi', label: 'KPI Console' }],", + '});', + '', +].join('\n'); + +/** + * No `objects` at all. Under the default `--objects-only` the stack module's + * sub-tree is empty, so no module is written — the input class that produced + * the orphan. + */ +const APPS_ONLY_CONFIG = [ + "import { defineStack } from '@objectstack/spec';", + '', + 'export default defineStack({', + " i18n: { defaultLocale: 'zh-CN', supportedLocales: ['zh-CN'] },", + " apps: [{ name: 'kpi', label: 'KPI Console' }],", + '});', + '', +].join('\n'); + +let fixtureRoot: string; +let outRoot: string; +let CONFIG: string; +let APPS_ONLY: string; + +beforeAll(() => { + const sharedRoot = join(CLI_PACKAGE_ROOT, 'tmp'); + mkdirSync(sharedRoot, { recursive: true }); + fixtureRoot = mkdtempSync(join(sharedRoot, 'os-i18n-16242-fixture-')); + CONFIG = join(fixtureRoot, 'stack.config.ts'); + APPS_ONLY = join(fixtureRoot, 'apps-only.config.ts'); + writeFileSync(CONFIG, STACK_CONFIG, 'utf8'); + writeFileSync(APPS_ONLY, APPS_ONLY_CONFIG, 'utf8'); + outRoot = mkdtempSync(join(tmpdir(), 'os-i18n-16242-')); +}, 300_000); + +afterAll(() => { + rmSync(fixtureRoot, { recursive: true, force: true }); + rmSync(outRoot, { recursive: true, force: true }); +}); + +/** stdout with SGR sequences removed — chalk is off through a pipe, belt and braces. */ +function plain(text: string): string { + // The escape byte is SPELLED, never embedded: a raw control byte in a source + // file renders as nothing and is findable by neither spelling. + return text.replace(/\u001b\[[0-9;]*m/g, ''); +} + +function runExtract( + name: string, + config: string, + flags: readonly string[], +): { stdout: string; dir: string; files: string[] } { + const dir = join(outRoot, name); + const stdout = execFileSync( + TSX, + [CLI, 'i18n', 'extract', config, '--locales=ja-JP', '--source-hashes', '--fill=default', ...flags, `--out=${dir}`], + { encoding: 'utf8', env: childEnv(), timeout: 180_000 }, + ); + return { stdout: plain(stdout), dir, files: existsSync(dir) ? readdirSync(dir).sort() : [] }; +} + +/** The dotted keys of a rendered `.source-hashes.generated.ts`. */ +function companionKeys(dir: string, locale = 'ja-JP'): string[] { + const src = readFileSync(join(dir, `${locale}.source-hashes.generated.ts`), 'utf8'); + return [...src.matchAll(/^ {2}"([^"]+)":/gm)].map((m) => m[1]).sort(); +} + +/** Leaves of an emitted bundle module, counted off its bytes. */ +function leavesOnDisk(dir: string, file: string): number { + const src = readFileSync(join(dir, file), 'utf8'); + return [...src.matchAll(/^[ \t]+(?:"[^"]*"|[A-Za-z_$][\w$]*): "/gm)].length; +} + +describe('os i18n extract — the provenance companion accompanies a module (#16242)', () => { + /** + * Symptom 2. Falsifier: restoring the literal list makes this run write + * exactly one file, `ja-JP.source-hashes.generated.ts`, holding an empty + * table — measured on `41cbc54fc5` before the repair. + */ + it('writes no companion at all when the run commits no bundle module', () => { + const run = runExtract('orphan', APPS_ONLY, ['--no-metadata-forms']); + + expect(run.files).toEqual([]); + expect(run.stdout).toContain('Generated 0 file(s)'); + // Specifically not the zero-record file: a companion with no module beside + // it describes nothing that exists. + expect(run.stdout).not.toContain('source-hashes.generated.ts'); + }); + + /** + * The settled `--check` semantics for a repository that already committed one + * of those orphans (#16242 acceptance item 4). `compareCommitted` iterates the + * EMITTED list and reports `missing` / `stale` over it; a file on disk that + * this run does not write is in neither category, so the leftover is + * tolerated, not demanded and not deleted. Pinned because it is a decision, + * not an accident — and because the alternative reading (delete it, or fail on + * it) would be a silently breaking change for those repositories. + * + * ⭐ The seeded bytes are deliberately a STUB rather than a faithful copy of + * what the old command rendered. The property being measured is that this + * file is not in the compared set AT ALL — a case seeded with bytes the run + * would itself produce could not tell "not compared" apart from "compared and + * equal". Ablating the emit gate reddens this case for that reason: the old + * command writes a companion, compares it against the stub, and reports it + * out of date. + */ + it('tolerates an already-committed empty companion — neither demanded nor deleted', () => { + const dir = join(outRoot, 'legacy-orphan'); + mkdirSync(dir, { recursive: true }); + const legacy = join(dir, 'ja-JP.source-hashes.generated.ts'); + writeFileSync( + legacy, + "export const jaJPGeneratedSourceHashes: Readonly> = {\n};\n", + 'utf8', + ); + + const stdout = plain( + execFileSync( + TSX, + [ + CLI, 'i18n', 'extract', APPS_ONLY, '--locales=ja-JP', '--source-hashes', + '--fill=default', '--no-metadata-forms', '--check', `--out=${dir}`, + ], + { encoding: 'utf8', env: childEnv(), timeout: 180_000 }, + ), + ); + + // Exit 0 — `execFileSync` would have thrown on the drift exit. + expect(stdout).toContain('in sync with the schema'); + expect(existsSync(legacy)).toBe(true); + }); + + /** + * The live path, and the case that catches the WRONG derivation. Reading the + * section list off the payload's own top-level keys would commit + * `['kpi_metric']` here — the object's name, not the section — and narrow + * every `objects.*` record away, leaving a zero-record companion beside a + * 2-leaf module. That is the one path this repository's single + * `--source-hashes` config is on. + */ + it('keeps its objects records under the default --objects-only', () => { + const run = runExtract('objects-only', CONFIG, ['--no-metadata-forms']); + + expect(run.files).toEqual(['ja-JP.objects.generated.ts', 'ja-JP.source-hashes.generated.ts', 'zh-CN.objects.generated.ts']); + expect(companionKeys(run.dir)).toEqual([ + 'objects.kpi_metric.fields.name.label', + 'objects.kpi_metric.label', + ]); + }); + + /** + * The same table survives the multi-section stack module, where the caller now + * commits `objects` AND `apps` instead of naming one literal. + * + * ⚠️ Two records, not three. `apps.kpi.label` IS a leaf of the emitted module + * and it has no provenance record — but it is not narrowed away, it is never + * recorded: `collectFilledFromHashes` walks `GENERATED_SECTIONS`, and `apps` + * is a HAND_AUTHORED section judged by a different predicate against a + * different, hand-maintained file. So this case pins the measurement, not a + * repair, and its number would not move if the section list were reverted. + */ + it('keeps every objects record when the stack module commits several sections', () => { + const run = runExtract('stack', CONFIG, ['--no-objects-only', '--no-metadata-forms']); + + expect(leavesOnDisk(run.dir, 'ja-JP.objects.generated.ts')).toBe(3); + expect(companionKeys(run.dir)).toEqual([ + 'objects.kpi_metric.fields.name.label', + 'objects.kpi_metric.label', + ]); + }); +}, 900_000); diff --git a/packages/cli/test/i18n-extract-source-hashes.test.ts b/packages/cli/test/i18n-extract-source-hashes.test.ts index 4e256d239f..2538848cfc 100644 --- a/packages/cli/test/i18n-extract-source-hashes.test.ts +++ b/packages/cli/test/i18n-extract-source-hashes.test.ts @@ -16,6 +16,8 @@ import { renderSourceHashModule, parseSourceHashModule, narrowToCommittedSections, + translationModulePayload, + translationModuleSections, } from '../src/utils/i18n-extract.js'; const stack = (help: string) => ({ @@ -144,3 +146,54 @@ describe('the provenance table is narrowed to the committed sections (#12559)', expect(narrowToCommittedSections(table, [])).toEqual({}); }); }); + +// --------------------------------------------------------------------------- +// The committed-section list is DERIVED from the module payloads (#16242) +// --------------------------------------------------------------------------- +// +// The command used to name the sections it commits with two literals — +// `committed.push('objects')` and `committed.push('metadataForms')` — chosen by +// which modules were emitted. The emitted-module half was already derived; the +// NAME was hand-copied, so under `kind: 'stack'` the caller named one of the +// several sections the module it was describing actually holds. +// +// ⚠️ These cases pin the mapping, and the mapping is NOT "the payload's own +// top-level keys". For `'objects'` and `'metadataForms'` the payload is the +// sub-tree ROOTED AT one section, so its keys are object and form names; only +// `'stack'` selects a `TranslationData`-shaped subtree whose keys are sections. +// Deriving from the keys in all three cases is the plausible wrong repair, and +// the first case below is what catches it: it would commit `['account']` and +// narrow every `objects.*` record away — on the one path this repository's +// single `--source-hashes` config is on. + +describe('the sections a module commits are read off its payload (#16242)', () => { + const result = extractTranslations( + { objects: [{ name: 'account', label: 'Account', fields: { name: { label: 'Name' } } }] }, + { defaultLocale: 'en', locales: ['zh-CN'], fill: 'default' }, + ); + const bundle = result.bundles['zh-CN']; + + it('names the section a single-section module roots at, never its payload keys', () => { + expect(translationModuleSections(bundle, 'objects')).toEqual(['objects']); + expect(translationModuleSections(bundle, 'metadataForms')).toEqual(['metadataForms']); + // The falsifier, spelled out: the payload's own keys are the object's name. + expect(Object.keys(translationModulePayload(bundle, 'objects'))).toEqual(['account']); + }); + + it('names every group a stack module holds, so a new group needs no edit here', () => { + const sections = translationModuleSections(bundle, 'stack'); + expect(sections).toContain('objects'); + // The registry baseline is the one thing a stack module never holds. + expect(sections).not.toContain('metadataForms'); + // Derived, not enumerated: a group the bundle grows appears by itself. + const grown = { ...bundle, apps: { kpi: { label: 'KPI Console' } } }; + expect(translationModuleSections(grown, 'stack')).toContain('apps'); + }); + + it('feeds narrowToCommittedSections a set that keeps the records it describes', () => { + const table = result.sourceHashes['zh-CN']; + const narrowed = narrowToCommittedSections(table, translationModuleSections(bundle, 'objects')); + expect(Object.keys(narrowed).length).toBeGreaterThan(0); + for (const path of Object.keys(narrowed)) expect(path.startsWith('objects.')).toBe(true); + }); +});