From 876c4ef14950c31c909f4195c0e2bae114c88d64 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 00:52:45 +0000 Subject: [PATCH 1/2] fix(runtime,cli,types): share the __search companion schema decision across serve/dev and migrate boots (#3955) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit os migrate plan/apply boots through createStandaloneStack, which never derived the locale-gated pinyin decision (ADR-0098 D1) from the compiled artifact's i18n config. Its SchemaRegistry therefore provisioned no __search companion columns, and detectManagedDrift() reported every live companion column of a dev-created database as a destructive orphan (drop_column) — with --allow-destructive as the printed remediation, which would have dropped live feature columns. - @objectstack/types: add collectConfiguredLocales + stampSearchPinyinEnabled, the single resolve-and-stamp seam for OS_SEARCH_PINYIN_ENABLED. Explicit env still wins; only a positive locale-derived decision is stamped; env writes go through globalThis (no Node type dependency, edge-safe). - @objectstack/runtime: createStandaloneStack stamps the decision from the artifact's i18n BEFORE any plugin constructs a SchemaRegistry, and surfaces i18n on its result like requires/objects/manifest so the CLI artifact-serve merge sees the same config keys. - @objectstack/cli: serve/dev stamps through the same shared helper (behaviour unchanged; second implementation removed). - ADR-0098: record that BOTH config-seeing boot paths must stamp. A fresh CLI-created database is now born with the same __search columns the dev runtime provisions, so the two paths converge in both directions. Regression coverage: types env unit tests; runtime standalone-stack boot tests (zh artifact stamps + surfaces i18n, non-zh leaves env untouched, explicit false survives); CLI integration test seeding a dev-shaped SQLite DB (with __search) plus a zh-CN artifact, asserting the migrate boot's managed metadata carries __search and drift reports no __search entries and zero destructive entries. Closes #3955 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01FcphmGzBuWwF8SaYPSCtCw --- .changeset/migrate-search-companion-parity.md | 31 ++++++ .../0098-pinyin-search-companion-column.md | 20 ++-- packages/cli/src/commands/serve.ts | 28 ++--- .../utils/schema-migrate.integration.test.ts | 100 ++++++++++++++++++ packages/runtime/src/standalone-stack.test.ts | 61 +++++++++++ packages/runtime/src/standalone-stack.ts | 27 ++++- packages/types/src/env.test.ts | 49 +++++++++ packages/types/src/env.ts | 61 ++++++++++- 8 files changed, 348 insertions(+), 29 deletions(-) create mode 100644 .changeset/migrate-search-companion-parity.md diff --git a/.changeset/migrate-search-companion-parity.md b/.changeset/migrate-search-companion-parity.md new file mode 100644 index 0000000000..6e44384cb0 --- /dev/null +++ b/.changeset/migrate-search-companion-parity.md @@ -0,0 +1,31 @@ +--- +"@objectstack/types": patch +"@objectstack/runtime": patch +"@objectstack/cli": patch +--- + +fix(runtime,cli,types): `os migrate` and the dev runtime now share one `__search` companion schema view (#3955) + +On a zh-locale deployment the dev runtime provisions the hidden `__search` +pinyin companion column (ADR-0098) on every eligible object, but the +`os migrate plan`/`apply` boot went through `createStandaloneStack`, which +never derived the locale-gated pinyin decision from the compiled artifact. +Its metadata therefore lacked every companion column, and `migrate plan` +reported each live `__search` column of a dev-created database as a +destructive orphan — with `--allow-destructive` as the printed remediation, +which would have dropped live feature columns. + +- `@objectstack/types`: new `collectConfiguredLocales(i18n)` and + `stampSearchPinyinEnabled(i18n)` — the single resolve-and-stamp helper for + `OS_SEARCH_PINYIN_ENABLED`. An explicit env value still wins; only a + positive locale-derived decision is stamped. +- `@objectstack/runtime`: `createStandaloneStack` stamps the decision from + the artifact's `i18n` before any plugin constructs a `SchemaRegistry`, and + surfaces `i18n` on its result like `requires`/`objects`/`manifest`. +- `@objectstack/cli`: the `serve`/`dev` boot now stamps through the same + shared helper (behaviour unchanged), so create/serve and plan/apply cannot + compute different schema views of the same source tree. + +A fresh CLI-created database is now also born with the same `__search` +columns the dev runtime would provision, instead of acquiring them on the +next dev boot. diff --git a/docs/adr/0098-pinyin-search-companion-column.md b/docs/adr/0098-pinyin-search-companion-column.md index 65af56c943..353c7f43c6 100644 --- a/docs/adr/0098-pinyin-search-companion-column.md +++ b/docs/adr/0098-pinyin-search-companion-column.md @@ -31,13 +31,19 @@ time**. 1. **Locale-gated platform switch, no field metadata.** `OS_SEARCH_PINYIN_ENABLED` (resolved by `resolveSearchPinyinEnabled()` in - `@objectstack/types`) gates the feature end-to-end. When unset, the CLI - boot path derives the default from the stack's configured locales (any - `zh-*` → on) and stamps the decision back into the env var so every - consumer — the per-engine `SchemaRegistry` and the plugin gate — reads the - same single decision. No field-level `pinyin` marker exists, so there is - no declared-but-unenforced dead metadata (ADR-0049) and no half-state - where a field "pretends" to support pinyin. + `@objectstack/types`) gates the feature end-to-end. When unset, every boot + path that sees the stack config derives the default from its configured + locales (any `zh-*` → on) and stamps the decision back into the env var + (shared `stampSearchPinyinEnabled()` helper) so every consumer — the + per-engine `SchemaRegistry` and the plugin gate — reads the same single + decision. There are exactly two such paths: the CLI `serve`/`dev` boot + (from `objectstack.config.ts`) and `createStandaloneStack` (from the + compiled artifact's `i18n` — `os migrate plan`/`apply`, embedders). A path + that resolved without stamping would compute a schema view without the + companion columns; that is how `os migrate` once flagged live `__search` + columns as destructive orphans (#3955). No field-level `pinyin` marker + exists, so there is no declared-but-unenforced dead metadata (ADR-0049) + and no half-state where a field "pretends" to support pinyin. 2. **Materialization set ≠ search set: one column per object.** Only the ADR-0079 display/name field (`resolveDisplayField`) feeds the hidden diff --git a/packages/cli/src/commands/serve.ts b/packages/cli/src/commands/serve.ts index 2b0fd5c10d..58fa4b8345 100644 --- a/packages/cli/src/commands/serve.ts +++ b/packages/cli/src/commands/serve.ts @@ -9,7 +9,7 @@ import { bundleRequire } from 'bundle-require'; import { loadConfig, BUNDLE_REQUIRE_EXTERNALS } from '../utils/config.js'; import { isHostConfig, shouldBootWithLibrary } from '../utils/plugin-detection.js'; import { resolveDriverType, resolveStorageDefinition, UnsupportedDriverError } from '../utils/storage-driver.js'; -import { readEnvWithDeprecation, resolveMultiOrgEnabled, resolveTenancyPosture, resolveAllowDegradedTenancy, isMcpServerEnabled, resolveSearchPinyinEnabled, isModuleNotFoundError } from '@objectstack/types'; +import { readEnvWithDeprecation, resolveMultiOrgEnabled, resolveTenancyPosture, resolveAllowDegradedTenancy, isMcpServerEnabled, stampSearchPinyinEnabled, isModuleNotFoundError } from '@objectstack/types'; import { PLATFORM_CAPABILITY_TOKENS } from '@objectstack/spec/kernel'; import { missingProviderMessage } from '../utils/capability-preflight.js'; import { resolveObjectStackHome } from '@objectstack/runtime'; @@ -726,22 +726,16 @@ export default class Serve extends Command { } // Pinyin search recall (#2486): locale-gated platform capability. When // `OS_SEARCH_PINYIN_ENABLED` is unset, the default derives from the - // stack's configured locales (any `zh-*` → on). This is the ONE place - // that sees the stack config, so the resolved decision is stamped back - // into the env var — every later consumer (each engine's SchemaRegistry - // provisioning the `__search` companion column, the plugin's own gate) - // reads the same answer via the no-arg `resolveSearchPinyinEnabled()`. - { - const i18nCfg = (config as any).i18n ?? {}; - const configuredLocales = [ - i18nCfg.defaultLocale, - i18nCfg.fallbackLocale, - ...(Array.isArray(i18nCfg.supportedLocales) ? i18nCfg.supportedLocales : []), - ].filter((l: unknown): l is string => typeof l === 'string'); - if (resolveSearchPinyinEnabled({ locales: configuredLocales })) { - process.env.OS_SEARCH_PINYIN_ENABLED = 'true'; - if (!requires.includes('pinyin-search')) requires.push('pinyin-search'); - } + // stack's configured locales (any `zh-*` → on), and the resolved + // decision is stamped back into the env var — every later consumer + // (each engine's SchemaRegistry provisioning the `__search` companion + // column, the plugin's own gate) reads the same answer via the no-arg + // `resolveSearchPinyinEnabled()`. The shared `stampSearchPinyinEnabled` + // helper is also what `createStandaloneStack` stamps from the compiled + // artifact, so serve/dev and `os migrate plan`/`apply` cannot compute + // different schema views of the same source tree (#3955). + if (stampSearchPinyinEnabled((config as any).i18n)) { + if (!requires.includes('pinyin-search')) requires.push('pinyin-search'); } // Default capability slate — every preset except `minimal` gets the // foundational services (queue + job + cache + settings + email + diff --git a/packages/cli/src/utils/schema-migrate.integration.test.ts b/packages/cli/src/utils/schema-migrate.integration.test.ts index ccd7dfb4e2..ec1d92386d 100644 --- a/packages/cli/src/utils/schema-migrate.integration.test.ts +++ b/packages/cli/src/utils/schema-migrate.integration.test.ts @@ -117,3 +117,103 @@ describe('bootSchemaStack + migrate engine (integration)', () => { } }, 30_000); }); + +/** + * #3955 — `os migrate` and the dev runtime must compute ONE schema view. + * + * A zh-locale deployment's dev runtime provisions the hidden `__search` + * pinyin companion column (ADR-0098) on every eligible object. The migrate + * boot goes through `createStandaloneStack`, which — before the fix — never + * derived the locale-gated pinyin decision from the artifact, so its metadata + * lacked every companion column and `os migrate plan` reported each live + * `__search` column as a destructive orphan, with `--allow-destructive` as + * the printed remediation. Following that advice would have dropped live + * feature columns. + */ +describe('bootSchemaStack — dev-provisioned __search companions are not orphans (#3955)', () => { + let dir: string; + let dbFile: string; + const savedEnv: Record = {}; + + beforeAll(async () => { + dir = mkdtempSync(join(tmpdir(), 'os-mig-pinyin-')); + mkdirSync(join(dir, 'dist'), { recursive: true }); + mkdirSync(join(dir, 'data'), { recursive: true }); + dbFile = join(dir, 'data', 'app.db'); + + // Compiled-artifact stand-in for a Chinese deployment: the i18n block is + // exactly what `os build` compiles out of `objectstack.config.ts`. + writeFileSync( + join(dir, 'dist', 'objectstack.json'), + JSON.stringify({ + id: 'mig_pinyin_smoke', + name: 'Migrate Pinyin Smoke', + i18n: { defaultLocale: 'en', supportedLocales: ['en', 'zh-CN'], fallbackLocale: 'en' }, + objects: [ + { + name: 'mig_person', + fields: { + name: { type: 'text', required: true }, + email: { type: 'text' }, + }, + }, + ], + }), + ); + + // Seed the database the way a dev-runtime first boot leaves it: the + // object's columns PLUS the provisioned `__search` companion. + const seed = new SqlDriver({ client: 'better-sqlite3', connection: { filename: dbFile }, useNullAsDefault: true }); + const k = (seed as any).knex; + await k.schema.createTable('mig_person', (t: any) => { + t.string('id').primary(); + t.timestamp('created_at'); + t.timestamp('updated_at'); + t.string('name').notNullable(); + t.string('email'); + t.string('__search'); + }); + await k('mig_person').insert({ id: '1', name: '张伟', email: 'zw@example.com', __search: 'zhangwei zw' }); + await k.destroy(); + + savedEnv.OS_ARTIFACT_PATH = process.env.OS_ARTIFACT_PATH; + savedEnv.NODE_ENV = process.env.NODE_ENV; + savedEnv.OS_SEARCH_PINYIN_ENABLED = process.env.OS_SEARCH_PINYIN_ENABLED; + process.env.OS_ARTIFACT_PATH = join(dir, 'dist', 'objectstack.json'); + process.env.NODE_ENV = 'production'; + // The defect precondition: nothing external decided pinyin — the boot must + // derive it from the artifact's locales, exactly like serve/dev does. + delete process.env.OS_SEARCH_PINYIN_ENABLED; + }); + + afterAll(() => { + process.env.OS_ARTIFACT_PATH = savedEnv.OS_ARTIFACT_PATH; + process.env.NODE_ENV = savedEnv.NODE_ENV; + if (savedEnv.OS_SEARCH_PINYIN_ENABLED === undefined) delete process.env.OS_SEARCH_PINYIN_ENABLED; + else process.env.OS_SEARCH_PINYIN_ENABLED = savedEnv.OS_SEARCH_PINYIN_ENABLED; + try { rmSync(dir, { recursive: true, force: true }); } catch { /* ignore */ } + }); + + it('the migrate boot provisions the companion in metadata, so plan reports no __search drift', async () => { + const stack = await bootSchemaStack({ databaseUrl: `file:${dbFile}` }); + try { + expect(stack.driver).toBeTruthy(); + + // The fix, observed at the seam: the artifact's zh-CN locale was stamped + // into the pinyin decision, so the migrate boot's OWN metadata carries + // the companion column — the same schema view the dev runtime serves. + const managed = (stack.driver as any).managedObjectFields.get('mig_person'); + expect(managed, 'mig_person must be metadata-managed').toBeDefined(); + expect(Object.keys(managed)).toContain('__search'); + + // And the defect, gone: no drift on __search — before the fix this was a + // destructive drop_column "orphaned" finding pointing at --allow-destructive. + const drift = await stack.driver!.detectManagedDrift(); + const searchDrift = drift.filter((d) => d.column === '__search'); + expect(searchDrift).toEqual([]); + expect(drift.filter((d) => d.category === 'destructive')).toEqual([]); + } finally { + await stack.shutdown(); + } + }, 30_000); +}); diff --git a/packages/runtime/src/standalone-stack.test.ts b/packages/runtime/src/standalone-stack.test.ts index b72635d15a..7f0f71ff29 100644 --- a/packages/runtime/src/standalone-stack.test.ts +++ b/packages/runtime/src/standalone-stack.test.ts @@ -121,6 +121,67 @@ describe('createStandaloneStack — surfaces app RBAC from the artifact (ADR-005 }, BOOT_TIMEOUT); }); +// #3955 — the standalone boot must share the serve/dev boot's locale-gated +// pinyin decision. `os migrate plan`/`apply` boot through this factory with the +// compiled artifact as the ONLY config in sight; before the stamp below, that +// boot resolved `resolveSearchPinyinEnabled()` env-first-only → off, computed a +// schema view WITHOUT the `__search` companion columns the dev runtime +// provisions, and reported every live companion column of a dev-created +// database as a destructive orphan (`drop_column`). +describe('createStandaloneStack — stamps the locale-derived pinyin decision from the artifact (#3955)', () => { + const originalEnv = process.env.OS_SEARCH_PINYIN_ENABLED; + let dir: string; + + beforeAll(() => { + dir = mkdtempSync(join(tmpdir(), 'os-standalone-pinyin-')); + }); + afterAll(() => { + try { rmSync(dir, { recursive: true, force: true }); } catch { /* noop */ } + if (originalEnv === undefined) delete process.env.OS_SEARCH_PINYIN_ENABLED; + else process.env.OS_SEARCH_PINYIN_ENABLED = originalEnv; + }); + + function writeArtifact(name: string, i18n?: unknown): string { + const p = join(dir, name); + writeFileSync(p, JSON.stringify({ ...ARTIFACT, ...(i18n ? { i18n } : {}) }), 'utf-8'); + return p; + } + + it('a zh-* locale in the artifact stamps OS_SEARCH_PINYIN_ENABLED before plugins boot, and i18n is surfaced', async () => { + delete process.env.OS_SEARCH_PINYIN_ENABLED; + const i18n = { defaultLocale: 'en', supportedLocales: ['en', 'zh-CN'], fallbackLocale: 'en' }; + const result = await createStandaloneStack({ + artifactPath: writeArtifact('zh.objectstack.json', i18n), + databaseUrl: 'memory://standalone-pinyin-zh', + }); + // The stamp is what each engine's SchemaRegistry (constructed later, at + // kernel start, without config access) reads to decide whether to + // provision the `__search` companion column. + expect(process.env.OS_SEARCH_PINYIN_ENABLED).toBe('true'); + // And the config-shaped result carries i18n like requires/objects/manifest, + // so the CLI artifact-serve merge sees the same stack config keys. + expect(result.i18n).toEqual(i18n); + }, BOOT_TIMEOUT); + + it('a non-Chinese artifact leaves the env untouched (companion stays off)', async () => { + delete process.env.OS_SEARCH_PINYIN_ENABLED; + await createStandaloneStack({ + artifactPath: writeArtifact('en.objectstack.json', { defaultLocale: 'en', supportedLocales: ['en'] }), + databaseUrl: 'memory://standalone-pinyin-en', + }); + expect(process.env.OS_SEARCH_PINYIN_ENABLED).toBeUndefined(); + }, BOOT_TIMEOUT); + + it('an explicit OS_SEARCH_PINYIN_ENABLED=false survives a zh-* artifact (operator override wins)', async () => { + process.env.OS_SEARCH_PINYIN_ENABLED = 'false'; + await createStandaloneStack({ + artifactPath: writeArtifact('zh-override.objectstack.json', { supportedLocales: ['zh-CN'] }), + databaseUrl: 'memory://standalone-pinyin-override', + }); + expect(process.env.OS_SEARCH_PINYIN_ENABLED).toBe('false'); + }, BOOT_TIMEOUT); +}); + // ADR-0062 D1 (#3826) — the standalone `default` datasource is a DECLARATION. // The stack no longer constructs a driver: it translates the database URL into // a `{ driver, config }` definition carried by `DefaultDatasourcePlugin`, which diff --git a/packages/runtime/src/standalone-stack.ts b/packages/runtime/src/standalone-stack.ts index a4d4f2b3d6..ee2ac06d27 100644 --- a/packages/runtime/src/standalone-stack.ts +++ b/packages/runtime/src/standalone-stack.ts @@ -33,7 +33,7 @@ import { resolve as resolvePath } from 'node:path'; import { mkdirSync } from 'node:fs'; import { homedir } from 'node:os'; import { z } from 'zod'; -import { readEnvWithDeprecation } from '@objectstack/types'; +import { readEnvWithDeprecation, stampSearchPinyinEnabled } from '@objectstack/types'; import { loadArtifactBundle, isHttpUrl } from './load-artifact-bundle.js'; /** @@ -108,6 +108,16 @@ export interface StandaloneStackResult { requires?: string[]; objects?: any[]; manifest?: any; + /** + * The stack's `i18n` config as compiled into the artifact. Surfaced so a + * caller wrapping this result as a `defineStack()`-shaped config (the CLI + * artifact-serve path) drives the SAME locale-gated decisions the + * config-load path drives — notably the pinyin-search default + * (`stampSearchPinyinEnabled`, #3955). The boot itself already stamps the + * decision; this keeps the surfaced config shape complete for consumers + * that re-derive it. + */ + i18n?: any; /** * App-declared RBAC metadata, surfaced so the CLI (`serve`/`dev`/`start`) * can wire it without a host `objectstack.config.ts`. The `serve` command @@ -283,6 +293,18 @@ export async function createStandaloneStack(config?: StandaloneStackConfig): Pro unwrapEnvelope: true, }); + // Locale-gated pinyin search (#2486 / #3955): the compiled artifact carries + // the stack's `i18n` config, and it is the ONLY config this boot ever sees — + // `os migrate plan`/`apply` and embedders never load `objectstack.config.ts`. + // Resolve the same locale-derived decision the CLI serve boot resolves and + // stamp it into `OS_SEARCH_PINYIN_ENABLED` BEFORE any plugin constructs a + // SchemaRegistry, so this boot provisions the same `__search` companion + // columns as the dev runtime. Without the stamp, `os migrate` diffed a + // dev-created database against a schema view missing every companion column + // and flagged the live columns as destructive orphans (#3955). No artifact → + // no locales → the env-first resolver decides, same as before. + stampSearchPinyinEnabled(artifactBundle?.i18n); + const plugins: any[] = [ // MUST precede ObjectQLPlugin: its start() connects the default driver // through the datasource connection service, and ObjectQLPlugin.start() @@ -331,6 +353,8 @@ export async function createStandaloneStack(config?: StandaloneStackConfig): Pro Array.isArray(artifactBundle?.permissions) ? artifactBundle.permissions : undefined; const positions: any[] | undefined = Array.isArray(artifactBundle?.positions) ? artifactBundle.positions : undefined; + const i18n: any | undefined = + artifactBundle?.i18n && typeof artifactBundle.i18n === 'object' ? artifactBundle.i18n : undefined; return { plugins, @@ -343,5 +367,6 @@ export async function createStandaloneStack(config?: StandaloneStackConfig): Pro ...(manifest ? { manifest } : {}), ...(permissions ? { permissions } : {}), ...(positions ? { positions } : {}), + ...(i18n ? { i18n } : {}), }; } diff --git a/packages/types/src/env.test.ts b/packages/types/src/env.test.ts index a0c4590822..66c7e9ad7f 100644 --- a/packages/types/src/env.test.ts +++ b/packages/types/src/env.test.ts @@ -3,12 +3,14 @@ import { afterEach, describe, expect, it, vi } from 'vitest'; import { _resetEnvDeprecationWarnings, + collectConfiguredLocales, readEnvWithDeprecation, resolveAllowDegradedTenancy, resolveSearchPinyinEnabled, resolveSandboxTimeoutMs, isMcpServerEnabled, resolveMcpStdioAutoStart, + stampSearchPinyinEnabled, } from './env.js'; describe('readEnvWithDeprecation', () => { @@ -176,6 +178,53 @@ describe('resolveSearchPinyinEnabled (#2486)', () => { }); }); +describe('collectConfiguredLocales / stampSearchPinyinEnabled (#3955)', () => { + const original = process.env.OS_SEARCH_PINYIN_ENABLED; + afterEach(() => { + if (original === undefined) delete process.env.OS_SEARCH_PINYIN_ENABLED; + else process.env.OS_SEARCH_PINYIN_ENABLED = original; + }); + + it('collects defaultLocale, fallbackLocale and supportedLocales; garbage collapses to []', () => { + expect( + collectConfiguredLocales({ defaultLocale: 'en', fallbackLocale: 'en', supportedLocales: ['en', 'zh-CN'] }), + ).toEqual(['en', 'en', 'en', 'zh-CN']); + expect(collectConfiguredLocales({ supportedLocales: ['en', 42, null] })).toEqual(['en']); + expect(collectConfiguredLocales(undefined)).toEqual([]); + expect(collectConfiguredLocales(null)).toEqual([]); + expect(collectConfiguredLocales('zh-CN')).toEqual([]); + }); + + it('stamps OS_SEARCH_PINYIN_ENABLED=true when a zh-* locale is configured and env is unset', () => { + delete process.env.OS_SEARCH_PINYIN_ENABLED; + expect(stampSearchPinyinEnabled({ defaultLocale: 'en', supportedLocales: ['en', 'zh-CN'] })).toBe(true); + expect(process.env.OS_SEARCH_PINYIN_ENABLED).toBe('true'); + // Downstream no-arg consumers (per-engine SchemaRegistry, the plugin + // gate) now read the same decision — the whole point of the stamp. + expect(resolveSearchPinyinEnabled()).toBe(true); + }); + + it('leaves the env untouched (and returns false) for a non-Chinese config', () => { + delete process.env.OS_SEARCH_PINYIN_ENABLED; + expect(stampSearchPinyinEnabled({ defaultLocale: 'en', supportedLocales: ['en', 'ja-JP'] })).toBe(false); + expect(process.env.OS_SEARCH_PINYIN_ENABLED).toBeUndefined(); + expect(stampSearchPinyinEnabled(undefined)).toBe(false); + expect(process.env.OS_SEARCH_PINYIN_ENABLED).toBeUndefined(); + }); + + it('an explicit env opt-out beats the locale-derived default and is not overwritten', () => { + process.env.OS_SEARCH_PINYIN_ENABLED = 'false'; + expect(stampSearchPinyinEnabled({ supportedLocales: ['zh-CN'] })).toBe(false); + expect(process.env.OS_SEARCH_PINYIN_ENABLED).toBe('false'); + }); + + it('an explicit env opt-in wins even with no zh locale configured', () => { + process.env.OS_SEARCH_PINYIN_ENABLED = 'true'; + expect(stampSearchPinyinEnabled({ supportedLocales: ['en'] })).toBe(true); + expect(process.env.OS_SEARCH_PINYIN_ENABLED).toBe('true'); + }); +}); + describe('MCP switches — HTTP surface vs stdio auto-start are decoupled (#3167)', () => { const origServer = process.env.OS_MCP_SERVER_ENABLED; const origServerLegacy = process.env.MCP_SERVER_ENABLED; diff --git a/packages/types/src/env.ts b/packages/types/src/env.ts index 9b810fcd5e..be3633cc2d 100644 --- a/packages/types/src/env.ts +++ b/packages/types/src/env.ts @@ -297,10 +297,12 @@ export function resolveOrgLimit(): number | undefined { * 3. No env var and no `zh-*` locale → off. OSS / non-Chinese deployments * never load `pinyin-pro` and pay zero compute cost. * - * Hosts that know the stack's i18n config (the CLI `serve` boot path) resolve - * once with locales and stamp the decision back into the env, so downstream - * consumers constructed without config access (per-engine SchemaRegistry) - * read the same answer via the no-arg form. + * Hosts that know the stack's i18n config — the CLI `serve` boot path AND the + * standalone artifact boot (`createStandaloneStack`, which `os migrate` + * plan/apply and embedders go through) — resolve once with locales and stamp + * the decision back into the env via {@link stampSearchPinyinEnabled}, so + * downstream consumers constructed without config access (per-engine + * SchemaRegistry) read the same answer via the no-arg form (#3955). */ export function resolveSearchPinyinEnabled(opts?: { locales?: readonly string[] }): boolean { const raw = readEnvWithDeprecation('OS_SEARCH_PINYIN_ENABLED', [], { silent: true }); @@ -310,6 +312,57 @@ export function resolveSearchPinyinEnabled(opts?: { locales?: readonly string[] return (opts?.locales ?? []).some((l) => /^zh([-_]|$)/i.test(String(l ?? '').trim())); } +/** + * The locales a stack's `i18n` config declares — `defaultLocale`, + * `fallbackLocale`, then `supportedLocales`. Accepts the config loosely typed + * (`unknown`) so any boot path can pass whatever its stack config or compiled + * artifact carries without importing spec schemas; non-string entries and a + * non-object config collapse to `[]`. + */ +export function collectConfiguredLocales(i18n: unknown): string[] { + const cfg = (i18n && typeof i18n === 'object' ? i18n : {}) as { + defaultLocale?: unknown; + fallbackLocale?: unknown; + supportedLocales?: unknown; + }; + return [ + cfg.defaultLocale, + cfg.fallbackLocale, + ...(Array.isArray(cfg.supportedLocales) ? cfg.supportedLocales : []), + ].filter((l): l is string => typeof l === 'string'); +} + +/** + * Resolve the pinyin-search decision from a stack's `i18n` config and stamp a + * positive result back into `OS_SEARCH_PINYIN_ENABLED` (#2486, #3955). + * + * Every boot path that SEES the stack config must stamp, because consumers + * constructed later without config access (each engine's `SchemaRegistry` + * provisioning the `__search` companion column, the `plugin-pinyin-search` + * gate) read the decision through the no-arg + * {@link resolveSearchPinyinEnabled}. A boot path that skips the stamp + * computes a schema view WITHOUT the companion columns — which is how + * `os migrate` came to flag the dev runtime's live `__search` columns as + * destructive orphans (#3955). Call sites: the CLI `serve`/`dev` boot + * (`objectstack.config.ts`) and `createStandaloneStack` (compiled artifact — + * `os migrate plan`/`apply`, embedders). + * + * An explicit `OS_SEARCH_PINYIN_ENABLED` always wins — the resolver reads it + * before consulting locales, so the stamp only materializes the + * locale-derived default. Only a positive decision is written: "unset" and + * "off" read identically through the no-arg resolver, and leaving the var + * untouched keeps a later boot free to re-derive from ITS config. + */ +export function stampSearchPinyinEnabled(i18n: unknown): boolean { + const enabled = resolveSearchPinyinEnabled({ locales: collectConfiguredLocales(i18n) }); + // Write through `globalThis` like `readEnvWithDeprecation` reads — this + // package has no Node type dependency (edge-safe); no env object → no stamp. + const env = (globalThis as { process?: { env?: Record } }) + .process?.env; + if (enabled && env) env.OS_SEARCH_PINYIN_ENABLED = 'true'; + return enabled; +} + /** * SINGLE decision point for a sandbox script-runner DEFAULT (ms), resolved from * the environment (framework#3259 / ADR-0102). From c5be0824f82d87438ec65c20a9624e3c938b681b Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 01:31:07 +0000 Subject: [PATCH 2/2] =?UTF-8?q?fix(driver-sql):=20a=20declared=20unique=20?= =?UTF-8?q?index=20is=20not=20legacy=20debt=20=E2=80=94=20index=20drift=20?= =?UTF-8?q?no=20longer=20ping-pongs=20(#3955)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The second half of #3955, reproduced against HotCRM's real metadata shape. crm_contact declares BOTH a tenant-scoped field-level unique and an object-level single-column unique index on the same column: email: Field.email({ unique: true }), indexes: [{ fields: ['email'], unique: true }], The declared index materializes under buildIndexName as uniq__, which is also one of the two spellings legacyUniqueIndexNames hunts for when retiring pre-#3696 platform-wide uniques. The detector therefore read an index the current metadata declares as legacy debt and proposed replacing it with the tenant composite the same sync had already created. The plan never converged: apply drops the declared index, the next plan reports it missing and recreates it, and the one after that calls it legacy again — an unbounded drop/create cycle on a live unique index, each round presented as a "safe" change. The issue's guess (create path emits legacy indexes that drift wants to replace) had it backwards: creation is correct, detection is wrong. legacyUniqueReplacements now takes declaredIndexes and filters their normalized names out of the legacy candidate set, using the same normalizeDeclaredIndex the create path uses so the two cannot disagree about what a declared index is called. Genuinely legacy indexes are still retired, including the knex-spelled
__unique when only the uniq_ spelling is declared. Regression coverage in sql-driver-index-drift.test.ts: fresh table builds both indexes and reports zero drift; three plan/apply rounds stay clean (the ping-pong); a legacy index nobody declares is still retired; the knex spelling is still retired when the buildIndexName spelling is declared. All four fail without the filter. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01FcphmGzBuWwF8SaYPSCtCw --- .../declared-unique-index-not-legacy.md | 31 +++++++ .../plugins/driver-sql/src/schema-drift.ts | 26 +++++- .../src/sql-driver-index-drift.test.ts | 92 +++++++++++++++++++ packages/plugins/driver-sql/src/sql-driver.ts | 4 +- 4 files changed, 150 insertions(+), 3 deletions(-) create mode 100644 .changeset/declared-unique-index-not-legacy.md diff --git a/.changeset/declared-unique-index-not-legacy.md b/.changeset/declared-unique-index-not-legacy.md new file mode 100644 index 0000000000..a074022f56 --- /dev/null +++ b/.changeset/declared-unique-index-not-legacy.md @@ -0,0 +1,31 @@ +--- +"@objectstack/driver-sql": patch +--- + +fix(driver-sql): a currently-declared unique index is never legacy debt — index drift no longer ping-pongs (#3955) + +An object may declare both a tenant-scoped field-level `unique: true` and an +object-level single-column unique index on the same column: + +```ts +email: Field.email({ unique: true }), +indexes: [{ fields: ['email'], unique: true }], +``` + +The declared index materializes under `buildIndexName` as +`uniq_
_` — which is also one of the two spellings +`legacyUniqueIndexNames` looks for when hunting pre-#3696 platform-wide +uniques. The detector therefore read an index the current metadata declares +as legacy debt and proposed replacing it with the tenant composite (which +the same sync had already created). + +The resulting plan never converged: `apply` dropped the declared index, the +next `plan` reported it missing and recreated it, and the one after that +called it legacy again — an unbounded drop/create cycle on a live unique +index, every round rendered as a "safe" change. + +`legacyUniqueReplacements` now takes the object's `declaredIndexes` and +filters their normalized names out of the legacy candidate set, so an index +metadata declares today is never mistaken for debt. Genuinely legacy indexes +are still retired, including the knex-spelled `
__unique` when +only the `uniq_…` spelling is declared. diff --git a/packages/plugins/driver-sql/src/schema-drift.ts b/packages/plugins/driver-sql/src/schema-drift.ts index 05cdca7399..b0742a2585 100644 --- a/packages/plugins/driver-sql/src/schema-drift.ts +++ b/packages/plugins/driver-sql/src/schema-drift.ts @@ -465,28 +465,50 @@ export interface LegacyUniqueReplacement { * names to look for and the composite that replaces them. `unique: 'global'` * fields are excluded — their single-column index is the declared intent now, * not legacy debt. + * + * `declaredIndexes` are excluded the same way, and for the same reason (#3955). + * An object may declare a single-column unique index alongside a tenant-scoped + * field-level `unique: true` — `email: { unique: true }` plus + * `indexes: [{ fields: ['email'], unique: true }]`. That declared index + * materializes under {@link buildIndexName}, which is *also* one of the two + * spellings {@link legacyUniqueIndexNames} looks for, so without this filter the + * detector reads an index metadata declares TODAY as pre-#3696 debt and proposes + * dropping it. The plan then never converges: apply drops the declared index, + * the next plan reports it missing and recreates it, and the one after that + * calls it legacy again — an unbounded drop/create cycle on a live unique index. + * An index the current metadata declares is by definition not legacy. */ export function legacyUniqueReplacements(args: { table: string; fields: Record; tenantField: string | null; physicalColumns: Set; + declaredIndexes?: Array<{ name?: string; fields?: string[]; unique?: boolean | 'global' }>; }): LegacyUniqueReplacement[] { - const { table, fields, tenantField, physicalColumns } = args; + const { table, fields, tenantField, physicalColumns, declaredIndexes } = args; if (!tenantField) return []; // Nothing was ever mis-scoped on a tenant-less table. // Without a physical tenant column there is no composite to replace the // legacy index with, and dropping it unreplaced would remove the constraint // outright rather than relax it. Leave it alone. if (!physicalColumns.has(tenantField)) return []; + // Normalized through the same helper the create path uses, so "what the + // declared index is named" is answered once, not guessed at twice. + const declaredNames = new Set( + (Array.isArray(declaredIndexes) ? declaredIndexes : []) + .map((idx) => normalizeDeclaredIndex(table, idx)?.name) + .filter((n): n is string => typeof n === 'string'), + ); const out: LegacyUniqueReplacement[] = []; for (const [name, field] of Object.entries(fields ?? {})) { if (!isUniqueDeclared(field?.unique)) continue; if (isGlobalUnique(field.unique)) continue; if (name === tenantField || !physicalColumns.has(name)) continue; + const legacyNames = legacyUniqueIndexNames(table, name).filter((n) => !declaredNames.has(n)); + if (legacyNames.length === 0) continue; const columns = [tenantField, name]; out.push({ column: name, - legacyNames: legacyUniqueIndexNames(table, name), + legacyNames, replacement: { name: buildIndexName(table, columns, true), columns, unique: true }, }); } diff --git a/packages/plugins/driver-sql/src/sql-driver-index-drift.test.ts b/packages/plugins/driver-sql/src/sql-driver-index-drift.test.ts index b3feec1191..6e323e95f9 100644 --- a/packages/plugins/driver-sql/src/sql-driver-index-drift.test.ts +++ b/packages/plugins/driver-sql/src/sql-driver-index-drift.test.ts @@ -153,6 +153,98 @@ describe('SqlDriver index drift (#3728)', () => { }); }); + // ── #3955: a DECLARED single-column unique is not legacy debt ───────────── + // + // An object may declare both a tenant-scoped field-level `unique: true` and + // an object-level single-column unique index on the same column (HotCRM's + // `crm_contact` does exactly this: `email: { unique: true }` plus + // `indexes: [{ fields: ['email'], unique: true }]`). The declared index + // materializes under `buildIndexName` — `uniq_
_` — which is also + // one of the two spellings the legacy detector looks for. It was therefore + // reported as pre-#3696 debt to be dropped, and the plan never converged. + describe('a currently-declared single-column unique index is never called legacy (#3955)', () => { + const contactMeta = [ + { + name: 'hp_contact', + fields: { + organization_id: { type: 'string' }, + email: { type: 'string', unique: true }, + last_name: { type: 'string' }, + }, + indexes: [ + { fields: ['email'], unique: true }, + { fields: ['last_name'] }, + ], + }, + ]; + + it('builds both indexes and then reports NO drift on a fresh table', async () => { + const driver = makeDriver(); + await driver.initObjects(contactMeta); + + // Both are current intent: the tenant composite from the field-level + // `unique: true`, and the verbatim declared global unique. + const uniques = await uniqueIndexColumns('hp_contact'); + expect(uniques['uniq_hp_contact_organization_id_email']).toEqual(['organization_id', 'email']); + expect(uniques['uniq_hp_contact_email']).toEqual(['email']); + + // Before the fix this reported `replace_unique_index` — proposing to drop + // the index the very same sync had just created from metadata. + expect(await driver.detectManagedDrift()).toHaveLength(0); + }); + + it('does not ping-pong: applying the plan converges instead of recreating work', async () => { + const driver = makeDriver(); + await driver.initObjects(contactMeta); + + // Round 1: nothing to do. Round 2 (the old loop's second half) likewise — + // the declared index is still there, so nothing reports it missing. + for (let round = 0; round < 3; round++) { + const drift = await driver.detectManagedDrift(); + expect(drift, `round ${round + 1} should be clean`).toHaveLength(0); + await driver.applyMigrationEntries(drift, { allowDestructive: false }); + } + + const uniques = await uniqueIndexColumns('hp_contact'); + expect(Object.keys(uniques).sort()).toEqual([ + 'uniq_hp_contact_email', + 'uniq_hp_contact_organization_id_email', + ]); + }); + + it('still retires a genuinely legacy index when metadata declares no such index', async () => { + // The #3728 behaviour must survive the filter: the same `uniq_
_` + // spelling IS legacy when nothing in metadata declares it. + const driver = makeDriver(); + await seedLegacyGlobalUnique('uniq_product_code'); + await driver.initObjects(productMeta); // no declared `indexes[]` + + const drift = await driver.detectManagedDrift(); + const entry = drift.find((d) => d.op.type === 'replace_unique_index'); + expect(entry, 'a legacy index nobody declares must still be retired').toBeDefined(); + expect((entry!.op as any).dropIndexNames).toEqual(['uniq_product_code']); + }); + + it('retires the knex-spelled legacy index even when the buildIndexName spelling is declared', async () => { + // Only the declared spelling is exempt. A pre-#3696 `
__unique` + // left over from the old createColumn path is still debt. + const driver = makeDriver(); + await knexInstance.schema.createTable('hp_contact', (t: any) => { + t.string('id').primary(); + t.string('organization_id'); + t.string('email'); + t.string('last_name'); + }); + await knexInstance.raw('CREATE UNIQUE INDEX hp_contact_email_unique ON hp_contact (email)'); + await driver.initObjects(contactMeta); + + const drift = await driver.detectManagedDrift(); + const entry = drift.find((d) => d.op.type === 'replace_unique_index'); + expect(entry).toBeDefined(); + expect((entry!.op as any).dropIndexNames).toEqual(['hp_contact_email_unique']); + }); + }); + // ── Applying it through `os migrate apply` ──────────────────────────────── describe('applyMigrationEntries', () => { diff --git a/packages/plugins/driver-sql/src/sql-driver.ts b/packages/plugins/driver-sql/src/sql-driver.ts index 2c8f93d246..6eb157dd49 100644 --- a/packages/plugins/driver-sql/src/sql-driver.ts +++ b/packages/plugins/driver-sql/src/sql-driver.ts @@ -2940,7 +2940,9 @@ export class SqlDriver implements IDataDriver { return diffManagedIndexes({ table: tableName, expected: expectedIndexes({ table: tableName, fields, tenantField, declaredIndexes, physicalColumns }), - legacy: legacyUniqueReplacements({ table: tableName, fields, tenantField, physicalColumns }), + // `declaredIndexes` goes to BOTH: it is what the table should have, and + // therefore also what must never be mistaken for legacy debt (#3955). + legacy: legacyUniqueReplacements({ table: tableName, fields, tenantField, physicalColumns, declaredIndexes }), physical: await this.introspectIndexes(tableName), }); }