From 1644f27c83e5d3f58f709cb38c3b9cebf946aca1 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 18 Aug 2026 06:56:05 +0000 Subject: [PATCH] fix(cli): os migrate meta can open the retired-key sources it exists to rewrite (#9418) The codemod refused its own input class. A retired authorable key is a `retiredKey()` tombstone -- `z.never()` carrying the upgrade prescription -- so the current schema does not strip it, it REJECTS it. And a real `objectstack.config.ts` runs that schema itself: `os init` scaffolds `export default defineStack({ ... })`, larger projects spread define* helpers across per-artifact modules, and every one of them is a `Schema.parse()`. The rejection therefore fired while the config module was being EVALUATED, inside the load, before the command reached its first conversion -- so it exited 1 having rewritten nothing, printing the very prescription that sends authors there (144 occurrences across 39 files under packages/spec/src). There was no CLI-side validation step to reorder: the gate lives in the loaded module. So `loadConfig()` gains an opt-in `authoredSource` mode, set by `os migrate meta` alone, that replaces each `@objectstack/spec` entrypoint the config imports -- root and subpaths -- with a shim re-exporting the real module and wrapping its define* helpers as try-real-then-authored. A source that loads today loads identically; a source the current schema refuses reaches the chain exactly as authored, with the swallowed verdict announced on stderr. Validation is not skipped but moved after the conversion: the migrated stack is still parsed and reported through `schemaValid`. `--stored` was probed and is not affected -- it never reads the config file. Co-Authored-By: Claude --- .../migrate-meta-reads-retired-key-sources.md | 58 +++++++ packages/cli/src/commands/migrate/meta.ts | 17 +- packages/cli/src/utils/config.ts | 161 +++++++++++++++++- packages/cli/test/migrate-meta.e2e.test.ts | 129 +++++++++++++- 4 files changed, 361 insertions(+), 4 deletions(-) create mode 100644 .changeset/migrate-meta-reads-retired-key-sources.md diff --git a/.changeset/migrate-meta-reads-retired-key-sources.md b/.changeset/migrate-meta-reads-retired-key-sources.md new file mode 100644 index 0000000000..4d5753d6e7 --- /dev/null +++ b/.changeset/migrate-meta-reads-retired-key-sources.md @@ -0,0 +1,58 @@ +--- +"@objectstack/cli": patch +--- + +fix(cli): `os migrate meta --from N` can finally open the retired-key sources it exists to rewrite (#9418) + +The codemod refused its own input class. A retired authorable key is a +`retiredKey()` tombstone — `z.never()` carrying the upgrade prescription — so the +current schema does not strip it, it **rejects** it. And a real +`objectstack.config.ts` runs that schema itself: `os init` scaffolds +`export default defineStack({ … })`, larger projects spread `defineView` / +`defineAgent` / `defineFlow` across per-artifact modules, and every one of those +`define*` helpers is a `Schema.parse()`. The rejection therefore fired while the +config module was being **evaluated**, inside the load, before `os migrate meta` +reached its first conversion — the command exited 1 having rewritten nothing. + +The message it printed was the instruction that sent the author there. The +sentence "Run `os migrate meta --from ` to rewrite existing sources +automatically." ships **144 times across 39 files** under `packages/spec/src`, so the v17 upgrade path closed +a loop on itself: hit a retired key, get told to run the codemod, watch the +codemod refuse **because of** the retired key. + +**The fix is a tolerant load for that one command.** There was no CLI-side +validation step to reorder — the gate lives in the loaded module — so +`loadConfig()` gains an opt-in `authoredSource` mode that replaces each +`@objectstack/spec` entrypoint the config imports (the root **and** the subpaths +the example apps author through, `@objectstack/spec/ui`, `/ai`, `/data`, …) with +a generated shim. The shim re-exports the real module and wraps its `define*` +helpers as try-real-then-authored: the real helper runs first, and only when the +current schema refuses the artifact is it handed on **exactly as authored**, with +the swallowed verdict announced on stderr. + +Three properties keep this a restoration rather than a widening of what the +command accepts: + +- **A source that loads today loads identically** — the real helper still runs, + so its defaults and transforms still apply (`defineForm` still moves + `schemaId` into `data`, `defineStack` still merges actions into objects). Only + the sources that are refused today take the new path. +- **Validation is moved after the conversion, not skipped.** The command still + parses the **migrated** stack through `ObjectStackDefinitionSchema` and reports + `schemaValid`, so a source broken for reasons the chain cannot fix is still + reported as broken — after the codemod has done the part it can. +- **Every other command still hears the tombstone.** `os build`, `os validate` + and `os serve` keep the default strict load: the rejection is their upgrade + channel, and only the codemod is entitled to read past it. Pinned both ways. + +`os migrate meta --stored` was probed and is **not** affected: it never reads +`objectstack.config.ts` at all — it boots from the compiled artifact and replays +the chain over `sys_metadata` rows, and it already exits 0 in a project whose +config carries a retired key. The defect was the authored-source arm alone. + +The regression proof is shaped like a real project rather than like a test — the +retired keys are authored through `defineStack` **and** through helpers imported +from a spec subpath, which is where a tolerance scoped to `defineStack` alone +would still have refused. The suite that shipped alongside the defect could not +have caught it: its fixture is a bare `export default { … }` object literal, and +a bare literal is validated by nobody at load. diff --git a/packages/cli/src/commands/migrate/meta.ts b/packages/cli/src/commands/migrate/meta.ts index ef0e52d384..529ab0ff39 100644 --- a/packages/cli/src/commands/migrate/meta.ts +++ b/packages/cli/src/commands/migrate/meta.ts @@ -290,7 +290,22 @@ export default class MigrateMeta extends Command { try { if (!flags.json) printStep('Loading configuration…'); - const { config, absolutePath } = await loadConfig(args.config); + // `authoredSource`: read the config as the author WROTE it, not as the + // current schema would have it (#9418). A retired key is a `retiredKey()` + // tombstone — the schema rejects it rather than stripping it — and a real + // config runs that schema itself: `os init` scaffolds + // `export default defineStack({ … })`, and every `define*` helper is a + // `Schema.parse()`. So the refusal used to happen while the config module + // was being EVALUATED, inside the load, before this command reached its + // first conversion — leaving the codemod unable to open the one input + // class it exists for, while the message it printed was the prescription + // telling the author to run it. + // + // This is where "convert before validating" has to land, because the CLI + // has no validation step of its own to move: the load is tolerant, and + // the schema verdict is taken below on the MIGRATED stack instead + // (`schemaValid`), which is the stack the author is being asked to adopt. + const { config, absolutePath } = await loadConfig(args.config, { authoredSource: true }); // Map→array normalization ONLY (convert:false): the chain must replay the // conversions itself against the raw authored source so each rewrite is diff --git a/packages/cli/src/utils/config.ts b/packages/cli/src/utils/config.ts index d0c122ced6..c6b77708c4 100644 --- a/packages/cli/src/utils/config.ts +++ b/packages/cli/src/utils/config.ts @@ -2,8 +2,11 @@ import path from 'path'; import fs from 'fs'; +import { createRequire } from 'node:module'; +import { pathToFileURL } from 'node:url'; import chalk from 'chalk'; import { bundleRequire } from 'bundle-require'; +import type { Plugin } from 'esbuild'; import { printError } from './format.js'; export interface LoadedConfig { @@ -72,17 +75,173 @@ export function resolveConfigPath(source?: string): string { process.exit(1); } +/** + * Every `@objectstack/spec` entrypoint an authored config can reach the + * `define*` helpers through — the root and every subpath export. Real projects + * use both: the example apps import `defineView`/`defineApp` from + * `@objectstack/spec/ui` and `defineHook`/`defineDatasource` from + * `@objectstack/spec/data`, so a shim that knew only the root package would + * cover the smaller half of the authored surface. + */ +const SPEC_MODULE_RE = /^@objectstack\/spec(?:\/[\w./-]+)?$/; + +/** esbuild namespace the authored-source shim modules live in. */ +const AUTHORED_SOURCE_NAMESPACE = 'objectstack-authored-source'; + +/** `defineStack`, `defineView`, … — the authoring helpers, by naming convention. */ +const DEFINE_HELPER_RE = /^define[A-Z]/; + +/** + * The `define*` helpers a given `@objectstack/spec` entrypoint exports, read + * from the copy **the config itself would import** (resolved from the config's + * own directory, not the CLI's). + * + * Returns `[]` — i.e. "shim nothing" — when the entrypoint cannot be resolved + * or imported. That is the safe direction: an unshimmed load is exactly + * today's behaviour, so a project the enumeration cannot read is no worse off + * than before. + */ +async function defineHelpersOf(specifier: string, requireFromConfig: NodeRequire): Promise { + try { + const resolved = requireFromConfig.resolve(specifier); + const ns = (await import(pathToFileURL(resolved).href)) as Record; + return Object.keys(ns).filter((k) => DEFINE_HELPER_RE.test(k) && typeof ns[k] === 'function'); + } catch { + return []; + } +} + +/** + * Load an authored config **as authored**, for the one consumer whose input is + * a source the CURRENT schema is expected to refuse: the `os migrate meta` + * codemod (#9418). + * + * ## The defect this exists to close + * + * A retired authorable key is a `retiredKey()` tombstone — `z.never()` carrying + * the upgrade prescription — so the current schema does not strip it, it + * REJECTS it. Every `define*` helper in `@objectstack/spec` is a + * `Schema.parse(config)`, and a real `objectstack.config.ts` calls them: `os + * init` scaffolds `export default defineStack({ … })`, and larger projects + * spread `defineView` / `defineAgent` / `defineFlow` across per-artifact + * modules. So the rejection happens while the config MODULE is being evaluated, + * inside `bundleRequire` — before `os migrate meta` has run a line of its own. + * + * The CLI never had a validation step to reorder: the gate lives in the loaded + * module. That made the codemod refuse the only input class it exists for, and + * the refusal it printed was the prescription telling the author to run it — + * `Run \`os migrate meta --from \`…` ships 144 times across 39 files under + * `packages/spec/src`, so the upgrade path closed a loop on itself. + * + * ## What the shim does + * + * Each `@objectstack/spec` entrypoint the config imports is replaced by a + * generated module that re-exports the real one and wraps its `define*` + * helpers as **try-real-then-authored**: + * + * ```js + * export const defineView = (...authored) => { + * try { return realDefineView(...authored); } catch { return authored[0]; } + * }; + * ``` + * + * The narrowness is the point, and it is what keeps this a restoration rather + * than a widening of what the command accepts: + * + * - **A source that loads today loads identically.** The real helper runs, so + * its defaults and transforms still apply (`defineForm` moves `schemaId` + * into `data`, `defineStack` merges actions into objects, …). Nothing about + * the existing happy path is re-decided. + * - **A source the current schema refuses reaches the chain as authored** — + * which is precisely the codemod's input. `defineX(config: z.input)` means the authored argument is by construction a shape + * `XSchema` accepts, so handing it on unparsed yields a well-formed + * authoring tree rather than an ad-hoc one. + * - **Validation is not skipped, it is moved after the conversion.** The + * command still parses the MIGRATED stack through + * `ObjectStackDefinitionSchema` and reports `schemaValid`, so a source that + * is broken for reasons the chain cannot fix is still reported as broken — + * just after the codemod has done the part it can. + * + * A swallowed verdict is announced on **stderr** rather than dropped: the + * author deserves to know an artifact bypassed the parse, and stderr keeps a + * `--json` run's stdout a single parseable document. + * + * ⚠️ Deliberately NOT the default for `loadConfig()`. Every other command — + * `os build`, `os validate`, `os serve` — must keep hearing the rejection: the + * tombstone IS their upgrade channel. Only the codemod is entitled to read + * past it. + */ +function authoredSourcePlugin(configPath: string): Plugin { + const requireFromConfig = createRequire(configPath); + return { + name: 'objectstack:authored-source', + setup(build) { + build.onResolve({ filter: SPEC_MODULE_RE }, (args) => { + // The shim re-exports the SAME specifier it stands in for. Left to + // resolve normally that import would land back here and shim itself + // forever, so inside the namespace the specifier is handed straight to + // the runtime — which is also what keeps `__real` the project's own + // copy of spec rather than the CLI's. + if (args.namespace === AUTHORED_SOURCE_NAMESPACE) { + return { path: args.path, external: true }; + } + return { path: args.path, namespace: AUTHORED_SOURCE_NAMESPACE }; + }); + + build.onLoad({ filter: /.*/, namespace: AUTHORED_SOURCE_NAMESPACE }, async (args) => { + const helpers = await defineHelpersOf(args.path, requireFromConfig); + const spec = JSON.stringify(args.path); + const lines = [ + `import * as __real from ${spec};`, + `export * from ${spec};`, + ]; + for (const name of helpers) { + lines.push( + `export const ${name} = (...authored) => {`, + ` try {`, + ` return __real.${name}(...authored);`, + ` } catch (error) {`, + ` console.warn(`, + ` '[authored-source] ' + ${JSON.stringify(name)} + '(): the current schema refuses this '`, + ` + 'artifact, so it is handed to the migration chain exactly as authored. '`, + ` + ((error && error.message) || String(error)),`, + ` );`, + ` return authored[0];`, + ` }`, + `};`, + ); + } + return { contents: lines.join('\n'), loader: 'js' }; + }); + }, + }; +} + +export interface LoadConfigOptions { + /** + * Read the config as AUTHORED rather than as the current schema would have + * it — see {@link authoredSourcePlugin}. Set by `os migrate meta` only. + * + * @default false + */ + authoredSource?: boolean; +} + /** * Load and bundle a config file using bundle-require. * Returns the resolved config object and load time. */ -export async function loadConfig(source?: string): Promise { +export async function loadConfig(source?: string, options?: LoadConfigOptions): Promise { const absolutePath = resolveConfigPath(source); const start = Date.now(); const { mod } = await bundleRequire({ filepath: absolutePath, external: BUNDLE_REQUIRE_EXTERNALS, + ...(options?.authoredSource + ? { esbuildOptions: { plugins: [authoredSourcePlugin(absolutePath)] } } + : {}), }); const baseConfig = mod.default || mod; diff --git a/packages/cli/test/migrate-meta.e2e.test.ts b/packages/cli/test/migrate-meta.e2e.test.ts index 734bca67fe..67d66cc106 100644 --- a/packages/cli/test/migrate-meta.e2e.test.ts +++ b/packages/cli/test/migrate-meta.e2e.test.ts @@ -17,9 +17,10 @@ import { describe, it, expect, beforeAll, afterAll } from 'vitest'; import { execFile } from 'node:child_process'; import { promisify } from 'node:util'; -import { mkdtempSync, rmSync, writeFileSync, readFileSync } from 'node:fs'; +import { mkdtempSync, mkdirSync, rmSync, symlinkSync, unlinkSync, writeFileSync, readFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; -import { join, resolve } from 'node:path'; +import { dirname, join, resolve } from 'node:path'; +import { createRequire } from 'node:module'; import { fileURLToPath } from 'node:url'; import { ObjectStackDefinitionSchema } from '@objectstack/spec'; @@ -317,3 +318,127 @@ describe('os migrate meta --from 16 (e2e over the real CLI)', () => { expect(failed, 'expected a non-zero exit below the floor').toBe(true); }, 120_000); }); + +/** + * #9418 — the input class the codemod EXISTS for: a source carrying a RETIRED + * key, migrated end-to-end. + * + * The suite above deliberately does not reach this. Its fixture is a bare + * `export default { … }` object literal, and a bare literal is validated by + * nobody at load — so it passed on a build where `os migrate meta` could not + * open a single real project. A real `objectstack.config.ts` runs the CURRENT + * schema itself: `os init` scaffolds `export default defineStack({ … })`, every + * `define*` helper is a `Schema.parse()`, and a retired key is a `retiredKey()` + * tombstone — `z.never()` carrying the upgrade prescription — so the parse + * REJECTS it rather than stripping it. The refusal therefore happened while the + * config module was being evaluated inside the load, before the command reached + * its first conversion, and the message it printed was the very prescription + * telling the author to run this command. + * + * So the fixture here is deliberately shaped like a real project and not like a + * test: `defineStack` at the root, per-artifact helpers imported from a spec + * SUBPATH (`@objectstack/spec/ai`, the spelling the example apps use), and the + * retired keys authored inside them. A `defineStack`-only tolerance passes the + * first and fails the last two. + * + * The second test is the other half of the pin, and it is the one that keeps + * this a restoration rather than a widening: `os validate` on the SAME fixture + * must still refuse. The tolerance belongs to the codemod alone — for every + * other command the tombstone is the upgrade channel. + */ +describe('os migrate meta over a source carrying a retired key (#9418)', () => { + /** + * Resolved through node_modules rather than by walking up from this file: + * `packages/cli` already depends on `@objectstack/spec`, so the dependency is + * one turbo already knows about, and a package specifier is not a + * cross-package source read. + */ + const SPEC_PACKAGE_ROOT = dirname(createRequire(import.meta.url).resolve('@objectstack/spec/package.json')); + + const RETIRED_KEY_CONFIG = ` +import { defineStack } from '@objectstack/spec'; +import { defineAgent, defineSkill } from '@objectstack/spec/ai'; + +export default defineStack({ + manifest: { id: 'retired_key_e2e', name: 'Retired Key E2E', version: '1.0.0', type: 'app', namespace: 'rk' }, + objects: [{ + name: 'rk_ticket', + label: 'Ticket', + fields: { title: { type: 'text', label: 'Title' } }, + }], + agents: [defineAgent({ + name: 'rk_agent', + label: 'Agent', + role: 'Helper', + instructions: 'help', + knowledge: { sources: ['faq'] }, // RETIRED in protocol 17 — the whole point + })], + skills: [defineSkill({ + name: 'rk_skill', + label: 'Skill', + tools: ['query_records'], + triggerPhrases: ['do the thing'], // RETIRED in protocol 17 + })], +}); +`; + + let rkDir: string; + let specLink: string; + + beforeAll(() => { + rkDir = mkdtempSync(join(tmpdir(), 'os-migrate-meta-retired-')); + mkdirSync(join(rkDir, 'node_modules', '@objectstack'), { recursive: true }); + specLink = join(rkDir, 'node_modules', '@objectstack', 'spec'); + symlinkSync(SPEC_PACKAGE_ROOT, specLink, 'dir'); + writeFileSync(join(rkDir, 'objectstack.config.ts'), RETIRED_KEY_CONFIG); + }); + + afterAll(() => { + // Unlinked BEFORE the recursive remove, and named explicitly: this symlink + // points at the real `packages/spec` in the checkout, and the one thing + // that must never be ambiguous in a cleanup is whether it can follow it. + try { unlinkSync(specLink); } catch { /* already gone */ } + try { rmSync(rkDir, { recursive: true, force: true }); } catch { /* ignore */ } + }); + + it('loads it, converts it, and reports the migrated stack schema-valid', async () => { + const stdout = await runMeta(['--from', '16', '--json', '--out', join(rkDir, 'migrated.json')], rkDir); + const parsed = JSON.parse(stdout); + + const appliedAt = new Map( + parsed.applied.map((a: any) => [a.conversionId, a.path] as [string, string]), + ); + // Authored through `defineStack`'s own inline literal… + expect(parsed.from).toBe(16); + // …and through helpers imported from a spec subpath, which is where a + // tolerance scoped to `defineStack` alone would still have refused. + expect(appliedAt.get('agent-knowledge-removed')).toBe('agents[0].knowledge'); + expect(appliedAt.get('skill-trigger-phrases-removed')).toBe('skills[0].triggerPhrases'); + expect(parsed.schemaValid).toBe(true); + + const snap = JSON.parse(readFileSync(join(rkDir, 'migrated.json'), 'utf-8')); + expect(snap.agents[0].knowledge).toBeUndefined(); + expect(snap.skills[0].triggerPhrases).toBeUndefined(); + // The rest of the artifact survives the tolerant load intact — the codemod + // removes what retired, not what it could not parse. + expect(snap.agents[0].name).toBe('rk_agent'); + expect(snap.skills[0].tools).toEqual(['query_records']); + }, 180_000); + + it('still refuses the same source everywhere else — `os validate` keeps the prescription', async () => { + let refused = false; + try { + await execFileP(TSX, [CLI, 'validate'], { + cwd: rkDir, + maxBuffer: 16 * 1024 * 1024, + env: { ...process.env, NO_COLOR: '1' }, + }); + } catch (e: any) { + refused = true; + const output = `${String(e.stdout ?? '')}${String(e.stderr ?? '')}`; + expect(output).toMatch(/`agent\.knowledge` was removed/); + expect(output).toMatch(/os migrate meta --from 16/); + } + expect(refused, '`os validate` must still reject a retired key').toBe(true); + }, 180_000); +});