diff --git a/.changeset/stack-storage-not-an-authoring-key.md b/.changeset/stack-storage-not-an-authoring-key.md new file mode 100644 index 0000000000..0a30a760c4 --- /dev/null +++ b/.changeset/stack-storage-not-an-authoring-key.md @@ -0,0 +1,71 @@ +--- +'@objectstack/spec': minor +'@objectstack/cli': patch +--- + +**`config.storage` is not a stack key, and an undeclared top-level key now says +so instead of vanishing (#4167).** + +`os serve` read `config.storage` and forwarded it to `StorageServicePlugin`. +It could almost never arrive: `ObjectStackDefinitionSchema` does not declare +`storage`, and is not `.strict()`, so `defineStack` — which every documented +authoring path and every compiled artifact goes through — strips the key before +`serve` runs. The one combination that reached the branch (a bare-object config +on the config-boot path) then carried the `driver`/`root` spelling the plugin +does not read either, so it did nothing there too. + +The result was one authoring key that worked on a single unreachable-in-practice +path and disappeared silently everywhere else. A host writing +`storage: { driver: 's3', … }` believed it had configured S3 and got local disk. + +- **`serve` no longer reads it.** `resolveStorageCapabilityArg` takes only the + env root; the production warning stops naming `config.storage` and names the + two channels that work — `OS_STORAGE_*` and Setup → Settings, the latter being + the one with proper credential handling. +- **The undeclared-key lint now covers the stack's own top-level keys.** New + `lintUnknownStackKeys(rawStack, stackSchema)`, wired into `defineStack`, + `os validate` and `os compile` beside the existing walker. `storage` gets a + prescriptive entry naming both channels and why a stack definition is the + wrong home for a credential — it would commit it to git and to any published + artifact. An ordinary misspelling still gets the edit-distance suggestion + (`datasource` → `datasources`). +- **`os migrate files-to-references` shares the resolver.** It built + `{ driver: 'local', root }` — the same dead keys — so its adapter used + `./storage` while the server writes under `.objectstack/data/uploads` since + #4096. That command reconciles what records claim against what storage holds, + so a disagreeing root reconciled against the wrong tree. + +**`onEnable` is exempt, and the exemption has one owner.** `onEnable` is a +function, so `ObjectStackDefinitionSchema` cannot declare it and +`dist/objectstack.json` cannot carry it — but it is not lost: `AppPlugin` calls +it off the authored bundle, and the artifact-boot path grafts it back (#4095). +"Not declared" and "dropped at load" are different claims, and this is the +surface where they come apart. New `STACK_RUNTIME_MEMBERS` in `@objectstack/spec` +names the members the runtime honours off the bundle; the lint treats them as +declared, and the CLI's `GRAFTABLE_RUNTIME_MEMBERS` is now **derived** from it +rather than restating it, so the list that decides what gets grafted and the +list that decides what the lint stays quiet about cannot drift. `onDisable` is +deliberately not on it — nothing calls it, so a value written there really does +go nowhere and the lint should say so. + +Additive: `lintUnknownAuthoringKeys` keeps its signature. The new pass is a +separate export rather than a fold into that walker for two reasons. The walker +iterates metadata COLLECTIONS, so a stack whose only mistake is at the envelope +level — no objects, no pages, nothing to iterate — walks clean; and the stack +schema has to be INJECTED, because `stack.zod.ts` imports the lint module and +importing back would close a cycle. A separate export keeps that requirement +visible: a call site either asks for the coverage or does not, and its absence +shows up in a diff. An optional parameter would be the same silent-loss shape +this rule family exists to report. It follows the walker's posture rule — only a +schema that STRIPS unknown keys is linted, so if the stack schema ever graduates +to `.strict()` the parse takes over and this goes quiet. + +Verified end to end: authoring `storage:` through `defineStack` warns at load, +and `os compile` reports it for configs that skip `defineStack`. + +Nothing is being taken away that worked. `storage` was never in the schema, is +not documented anywhere, and has no consumer in `objectstack-ai/cloud` (checked). +Whether the platform should eventually grow a real in-stack storage declaration +is a separate question — if so it should follow `datasources`, which solves +credentials by referencing `sys_secret` rather than inlining them, and that +deserves an ADR rather than a resurrected undeclared key. diff --git a/content/docs/deployment/validating-metadata.mdx b/content/docs/deployment/validating-metadata.mdx index 67788fe173..932de3dbcd 100644 --- a/content/docs/deployment/validating-metadata.mdx +++ b/content/docs/deployment/validating-metadata.mdx @@ -248,6 +248,26 @@ the parse itself rejects loudly, with the schema's own guidance. Because the lint reads each schema's real unknown-key posture, it can never disagree with the parse. +It also covers the stack's **own top-level keys** — the envelope those +collections sit in, and the level where the silence is hardest to spot, because +an undeclared key there reads as configuration that took effect rather than as +a typo: + +```ts +export default defineStack({ + storage: { adapter: 's3', s3: { bucket: 'app-files' } }, + // ↑ not a stack key → dropped at load; the app keeps writing to local disk + objects: [...], +}); +``` + +``` +stack.storage: 'storage' is not a declared stack key, so its value is dropped at + load — the file-storage backend is a deployment concern, not an application + declaration. Configure it with the OS_STORAGE_* environment variables, or + per-deployment in Setup → Settings → Storage. +``` + This is **advisory** — the stack still loads. Strict rejection is where these schemas are headed (ADR-0049 enforce-or-remove), but these are the protocol's most-authored surfaces, so the tightening is scheduled on what this check finds @@ -273,7 +293,7 @@ time, so an author sees them without running the CLI at all. | View references — form targets, view-key collisions (#2554) | ✓ | ✓ | | Flow authoring anti-patterns (#1874) | ✓ | ✓ | | Liveness author-warnings | ✓ | ✓ | -| Undeclared authoring keys, every metadata collection (#3786) | ✓ | ✓ | +| Undeclared authoring keys — every metadata collection (#3786) and the stack's own top-level keys (#4167) | ✓ | ✓ | | Emits `dist/objectstack.json` | — | ✓ | So `os validate` is the fast inner-loop check (no artifact); `os build` is what diff --git a/packages/cli/src/commands/compile.ts b/packages/cli/src/commands/compile.ts index a85a1285fc..6e82d11734 100644 --- a/packages/cli/src/commands/compile.ts +++ b/packages/cli/src/commands/compile.ts @@ -9,6 +9,7 @@ import { ObjectStackDefinitionSchema, normalizeStackInput, lintUnknownAuthoringKeys, + lintUnknownStackKeys, formatUnknownAuthoringKey, type ConversionNotice, } from '@objectstack/spec'; @@ -263,7 +264,10 @@ export default class Compile extends Command { // authored through it; this covers the ones that skip it (a plain // object default-export, `strict: false`) and would otherwise emit an // artifact with the key quietly gone. Advisory, never fatal. - const unknownKeyFindings = lintUnknownAuthoringKeys(normalized as Record); + const unknownKeyFindings = [ + ...lintUnknownStackKeys(normalized as Record, ObjectStackDefinitionSchema), + ...lintUnknownAuthoringKeys(normalized as Record), + ]; if (unknownKeyFindings.length > 0 && !flags.json) { printWarning(`Undeclared authoring keys (${unknownKeyFindings.length}) — dropped at load (#3786)`); for (const f of unknownKeyFindings.slice(0, 50)) { diff --git a/packages/cli/src/commands/migrate/files-to-references.ts b/packages/cli/src/commands/migrate/files-to-references.ts index d6ace7f552..09113c745c 100644 --- a/packages/cli/src/commands/migrate/files-to-references.ts +++ b/packages/cli/src/commands/migrate/files-to-references.ts @@ -17,7 +17,7 @@ import { import { bootSchemaStack } from '../../utils/schema-migrate.js'; import { OCCUPANCY_HINT, probeMigrationTarget } from '../../utils/migrate-occupancy-gate.js'; import { describeOccupancy } from '../../utils/sqlite-occupancy.js'; -import { loadConfig } from '../../utils/config.js'; +import { resolveStorageCapabilityArg } from '../serve.js'; async function confirm(question: string): Promise { if (!process.stdin.isTTY) return false; // non-interactive → require --yes @@ -36,8 +36,15 @@ async function confirm(question: string): Promise { * Settings first: the storage plugin re-resolves its adapter from persisted * settings when a settings service is present, which is how an S3-configured * deployment's backfill uploads land in S3 rather than on this machine. - * Storage config mirrors `os serve`'s resolution (config.storage, else the - * local default) so the CLI materialises bytes exactly where the server would. + * Storage config goes through the SAME resolver `os serve` uses + * (`resolveStorageCapabilityArg`), so the CLI materialises bytes exactly where + * the server would. It did not, and that mattered here more than anywhere: this + * command reconciles what records claim against what storage actually holds, so + * a root that disagrees with the server's reconciles against the wrong tree. + * It built `{ driver: 'local', root }` — keys `StorageServicePluginOptions` does + * not declare — so the adapter silently used the plugin's own `./storage` + * default while the server (since framework#4096) writes under + * `.objectstack/data/uploads`. */ async function buildDataMigrationPlugins(): Promise { const plugins: unknown[] = []; @@ -48,19 +55,8 @@ async function buildDataMigrationPlugins(): Promise { // optional — without it, constructor/env-driven storage config still applies } const { StorageServicePlugin } = await import('@objectstack/service-storage'); - let arg: Record | undefined; - try { - const { config } = await loadConfig(); - const cfgStorage = (config as any)?.storage; - if (cfgStorage && (cfgStorage.driver || cfgStorage.adapter)) arg = cfgStorage; - } catch { - // artifact-only project (no objectstack.config.ts) — use the default below - } - if (arg === undefined) { - const root = process.env.OS_STORAGE_ROOT || '.objectstack/data/uploads'; - arg = { driver: 'local', root }; - } - plugins.push(new StorageServicePlugin({ ...(arg as any), registerRoutes: false })); + const { options } = resolveStorageCapabilityArg(process.env.OS_STORAGE_ROOT); + plugins.push(new StorageServicePlugin({ ...options, registerRoutes: false })); return plugins; } diff --git a/packages/cli/src/commands/serve-storage-capability.test.ts b/packages/cli/src/commands/serve-storage-capability.test.ts index 16f65594b5..57a5e55b4c 100644 --- a/packages/cli/src/commands/serve-storage-capability.test.ts +++ b/packages/cli/src/commands/serve-storage-capability.test.ts @@ -16,6 +16,13 @@ * option SHAPE, because a shape mismatch is exactly the failure a passing type * check does not catch when the receiving interface has no index signature and * the value is built as a plain object literal. + * + * framework#4167 then removed the `config.storage` branch this helper also had. + * `storage` was never a declared stack key, so `defineStack` — which every + * documented authoring path and every compiled artifact goes through — stripped + * it long before `serve` ran. Reading it here made one authoring key work on the + * single path that skips `defineStack` and vanish on all the others. Authors who + * write it are now told, by `lintUnknownAuthoringKeys`. */ import { describe, it, expect } from 'vitest'; @@ -24,7 +31,7 @@ import { resolveStorageCapabilityArg } from './serve.js'; describe('resolveStorageCapabilityArg', () => { it('builds options StorageServicePlugin actually reads', () => { // `adapter` + `local.rootDir` — NOT `driver` + `root`. - expect(resolveStorageCapabilityArg(undefined).options).toEqual({ + expect(resolveStorageCapabilityArg().options).toEqual({ adapter: 'local', local: { rootDir: '.objectstack/data/uploads' }, }); @@ -33,49 +40,40 @@ describe('resolveStorageCapabilityArg', () => { it('never emits the keys the plugin ignores', () => { // The regression guard proper: `{driver, root}` type-checks fine as an // argument and does nothing at runtime. - const { options } = resolveStorageCapabilityArg(undefined); + const { options } = resolveStorageCapabilityArg(); expect(options).not.toHaveProperty('driver'); expect(options).not.toHaveProperty('root'); }); it('honours OS_STORAGE_ROOT, which the old shape discarded', () => { - const { options, localRoot } = resolveStorageCapabilityArg(undefined, '/srv/uploads'); + const { options, localRoot } = resolveStorageCapabilityArg('/srv/uploads'); expect(options).toEqual({ adapter: 'local', local: { rootDir: '/srv/uploads' } }); expect(localRoot).toBe('/srv/uploads'); }); it('ignores a blank or whitespace-only env root', () => { for (const blank of ['', ' ']) { - expect(resolveStorageCapabilityArg(undefined, blank).options).toEqual({ + expect(resolveStorageCapabilityArg(blank).options).toEqual({ adapter: 'local', local: { rootDir: '.objectstack/data/uploads' }, }); } }); - it('reports the local root so only the fallback triggers the production warning', () => { - // A host that configured its own backend must not be told it is on local disk. - expect(resolveStorageCapabilityArg(undefined).localRoot).toBe('.objectstack/data/uploads'); - expect(resolveStorageCapabilityArg({ adapter: 's3', s3: { bucket: 'b', region: 'r' } }).localRoot) - .toBeUndefined(); + it('reports the local root the production warning names', () => { + // The warning quotes this, so it has to be the directory actually in use — + // it previously named a root the adapter was not using. + expect(resolveStorageCapabilityArg().localRoot).toBe('.objectstack/data/uploads'); + expect(resolveStorageCapabilityArg('/srv/x').localRoot).toBe('/srv/x'); }); - it('forwards a host-configured storage block verbatim', () => { - const cfg = { adapter: 's3', s3: { bucket: 'b', region: 'r' } }; - expect(resolveStorageCapabilityArg(cfg).options).toBe(cfg); - // The `driver` dialect is still forwarded untouched — the plugin does not - // read it either, but rewriting it here would fossilize the wrong contract - // rather than fix it. Tracked separately. - const legacy = { driver: 's3', bucket: 'b' }; - expect(resolveStorageCapabilityArg(legacy).options).toBe(legacy); + it('does not read config.storage at all — it was never a stack key (#4167)', () => { + // `ObjectStackDefinitionSchema` does not declare `storage`, and is not + // `.strict()`, so `defineStack` strips it before serve could see it. Reading + // it here made the same authoring key work on the one path that skips + // `defineStack` and vanish on every documented one. The signature no longer + // accepts it; `lintUnknownAuthoringKeys` now tells the author instead. + expect(resolveStorageCapabilityArg.length).toBe(1); }); - it('falls back when the block names no backend at all', () => { - // `config.storage = { presignedTtl: 60 }` configures no backend, so the - // local default still applies rather than being replaced by a partial block. - expect(resolveStorageCapabilityArg({ presignedTtl: 60 }).options).toEqual({ - adapter: 'local', - local: { rootDir: '.objectstack/data/uploads' }, - }); - }); }); diff --git a/packages/cli/src/commands/serve.ts b/packages/cli/src/commands/serve.ts index 3626220928..ca37774327 100644 --- a/packages/cli/src/commands/serve.ts +++ b/packages/cli/src/commands/serve.ts @@ -2184,14 +2184,13 @@ export default class Serve extends Command { // In production mode we emit a single loud warning so the // operator knows to point storage at S3 / GCS / Azure before // shipping (data on a single pod is volatile / non-replicated). - const storageArg = resolveStorageCapabilityArg( - (config as any).storage, - process.env.OS_STORAGE_ROOT, - ); + const storageArg = resolveStorageCapabilityArg(process.env.OS_STORAGE_ROOT); arg = storageArg.options; if (storageArg.localRoot && !isDev) { + // Names only the channels that actually work — `config.storage` + // was in this sentence and was never read (framework#4167). console.warn(chalk.yellow( - ` ⚠ StorageServicePlugin using local driver (${storageArg.localRoot}) — switch to S3/GCS/Azure for production (set config.storage or OS_STORAGE_*).`, + ` ⚠ StorageServicePlugin using local driver (${storageArg.localRoot}) — switch to S3/GCS/Azure for production (set OS_STORAGE_* or configure storage in Setup → Settings).`, )); } } @@ -2665,20 +2664,23 @@ export interface StorageCapabilityArg { * default IS `./.objectstack/data/uploads`), which swapped the adapter and * warned about stranded files — on every boot of a healthy server. * - * A caller-supplied `config.storage` is still forwarded verbatim, including the - * `driver`/`root` dialect, which the plugin does not read either. That is the - * same mismatch one layer up and is tracked separately: correcting it means - * deciding whether the plugin accepts that dialect or the config schema is - * wrong, and a lenient alias here would fossilize the wrong contract - * (AGENTS.md Prime Directive #12). + * `config.storage` is deliberately NOT read (framework#4167). It was never a + * stack key: `ObjectStackDefinitionSchema` does not declare it, and the schema + * is not `.strict()`, so `defineStack` — which every documented authoring path + * and every compiled artifact goes through — strips it before `serve` could + * ever see it. It arrived here only from a bare-object config on the + * config-boot path, i.e. one unreachable-in-practice combination, where it then + * ALSO carried the `driver`/`root` spelling the plugin does not read. Honouring + * it on that one path meant the same authoring key worked in one place and + * vanished in every other, which is worse than not having it. + * + * The storage backend is a deployment concern with two real channels: the + * `OS_STORAGE_*` env vars (below) and the `storage` settings namespace, which + * is also the one with proper credential handling. Authors who write `storage:` + * anyway now get told so — `lintUnknownStackKeys` reports undeclared top-level + * keys, and `STACK_KEY_GUIDANCE` names both channels. */ -export function resolveStorageCapabilityArg( - cfgStorage: any, - envRoot?: string, -): StorageCapabilityArg { - if (cfgStorage && (cfgStorage.driver || cfgStorage.adapter)) { - return { options: cfgStorage }; - } +export function resolveStorageCapabilityArg(envRoot?: string): StorageCapabilityArg { const rootDir = envRoot?.trim() || '.objectstack/data/uploads'; return { options: { adapter: 'local', local: { rootDir } }, localRoot: rootDir }; } diff --git a/packages/cli/src/commands/validate.ts b/packages/cli/src/commands/validate.ts index 1add504da4..ec47e4d7c3 100644 --- a/packages/cli/src/commands/validate.ts +++ b/packages/cli/src/commands/validate.ts @@ -10,6 +10,7 @@ import { ObjectStackDefinitionSchema, normalizeStackInput, lintUnknownAuthoringKeys, + lintUnknownStackKeys, formatUnknownAuthoringKey, type ConversionNotice, } from '@objectstack/spec'; @@ -97,8 +98,10 @@ export default class Validate extends Command { // key the author actually wrote. Computed here rather than down in the // warnings section so the `--json` path reports it too — the // "computed, then discarded" shape this file already had to fix once. - const unknownKeyWarnings = lintUnknownAuthoringKeys(normalized as Record) - .map(formatUnknownAuthoringKey); + const unknownKeyWarnings = [ + ...lintUnknownStackKeys(normalized as Record, ObjectStackDefinitionSchema), + ...lintUnknownAuthoringKeys(normalized as Record), + ].map(formatUnknownAuthoringKey); const result = ObjectStackDefinitionSchema.safeParse(normalized); if (!result.success) { diff --git a/packages/cli/src/utils/graft-runtime-hooks.ts b/packages/cli/src/utils/graft-runtime-hooks.ts index 2237c4d1e6..09b4299766 100644 --- a/packages/cli/src/utils/graft-runtime-hooks.ts +++ b/packages/cli/src/utils/graft-runtime-hooks.ts @@ -28,6 +28,8 @@ // invisible for so long. // --------------------------------------------------------------------------- +import { STACK_RUNTIME_MEMBERS } from '@objectstack/spec'; + /** * Bundle members `AppPlugin` executes, and which therefore cannot survive a * trip through JSON: @@ -40,8 +42,16 @@ * `onDisable` is deliberately absent: it is declared in `packages/spec` but no * kernel, runtime or service ever calls it, so grafting it would wire a hook * nothing runs. + * + * **Derived, not restated** (framework#4167). The same list decides what the + * undeclared-top-level-key lint stays quiet about: these members are not + * declared by `ObjectStackDefinitionSchema`, but they are honoured off the + * authored bundle, so reporting them as "dropped at load" would be false. Two + * hand-written copies could disagree, and the disagreement would be silent in + * exactly the direction that lint exists to catch — so the protocol owns the + * list and this re-exports it. */ -export const GRAFTABLE_RUNTIME_MEMBERS = ['onEnable', 'functions'] as const; +export const GRAFTABLE_RUNTIME_MEMBERS = STACK_RUNTIME_MEMBERS; export type GraftableRuntimeMember = (typeof GRAFTABLE_RUNTIME_MEMBERS)[number]; diff --git a/packages/spec/api-surface.json b/packages/spec/api-surface.json index 46a786dc84..2cf00c2564 100644 --- a/packages/spec/api-surface.json +++ b/packages/spec/api-surface.json @@ -91,6 +91,8 @@ "PredicateInput (type)", "PredicateInputSchema (const)", "PredicateSchema (const)", + "STACK_KEY_GUIDANCE (const)", + "STACK_RUNTIME_MEMBERS (const)", "SemanticMigration (interface)", "Skill (type)", "SpecChanges (type)", @@ -157,6 +159,7 @@ "isAggregatedViewContainer (function)", "isKnownPlatformCapability (function)", "lintUnknownAuthoringKeys (function)", + "lintUnknownStackKeys (function)", "listLintableAuthoringCollections (function)", "mapMembershipRole (function)", "normalizeMetadataCollection (function)", @@ -501,6 +504,8 @@ "SQLiteDataTypeMappingDefaults (const)", "SSLConfig (type)", "SSLConfigSchema (const)", + "STACK_KEY_GUIDANCE (const)", + "STACK_RUNTIME_MEMBERS (const)", "STRING_VALUE_TYPES (const)", "STRUCTURED_JSON_TYPES (const)", "Scalar (type)", @@ -1862,6 +1867,7 @@ "isConsumerInstallable (function)", "isKnownPlatformCapability (function)", "lintUnknownAuthoringKeys (function)", + "lintUnknownStackKeys (function)", "listLintableAuthoringCollections (function)", "listMetadataCreateSeedTypes (function)", "listMetadataTypeSchemaTypes (function)", diff --git a/packages/spec/src/data/authoring-key-lint.ts b/packages/spec/src/data/authoring-key-lint.ts index e8676bc455..3f9f73f979 100644 --- a/packages/spec/src/data/authoring-key-lint.ts +++ b/packages/spec/src/data/authoring-key-lint.ts @@ -135,6 +135,59 @@ export const OBJECT_KEY_GUIDANCE: Readonly< tableName: { why: 'removed — the table name always equals the object `name`.' }, }); +/** + * Semantic near-misses on the stack's own TOP-LEVEL keys + * (`ObjectStackDefinitionSchema`). + * + * Same silent-drop mechanism as the two tables above, one level up. The walker + * covers every metadata COLLECTION; this covers the envelope those collections + * sit in, which is where the silence is easiest to miss — an undeclared + * top-level key reads as configuration that took effect. + * + * `storage` is the worked example (#4167): `os serve` honoured it only on the + * one boot path that skips `defineStack`, so the same key configured a backend + * in one place and vanished in every other. A stack asking for S3 could + * silently get local disk. + */ +export const STACK_KEY_GUIDANCE: Readonly< + Record +> = Object.freeze({ + storage: { + why: 'the file-storage backend is a deployment concern, not an application declaration. ' + + 'Configure it with the OS_STORAGE_* environment variables, or per-deployment in Setup → Settings → Storage ' + + '(which also holds credentials — a stack definition would commit them to git and to any published artifact).', + }, +}); + +/** + * Authored TOP-LEVEL members the **runtime executes off the bundle**, which the + * stack schema therefore does not — and cannot — declare. + * + * `onEnable` is a function. It cannot survive `ObjectStackDefinitionSchema` + * (which does not declare it) and it cannot survive `dist/objectstack.json` + * (JSON has no functions), yet it is not lost: `AppPlugin` reads it straight + * off the authored bundle and calls it at `start()`, and on the artifact-boot + * path the CLI grafts it back (#4095). It is the documented place to register + * action handlers, and `examples/app-todo` and `examples/app-showcase` both + * ship it. + * + * So the lint must stay SILENT here. "Not declared" and "dropped at load" are + * different claims, and this is the one surface where they come apart — telling + * an author their working `onEnable` is being discarded would be a confident + * lie about the pattern we ship in our own examples. + * + * `functions` is listed for the same reason (a name → handler map the runtime + * resolves string-named hooks against); the schema happens to declare it too, + * so it self-excludes. `onDisable` is deliberately ABSENT: it is declared in + * the protocol but no kernel, runtime or service ever calls it, so a value + * written there really does go nowhere and the lint should say so. + * + * Single source of truth — the CLI's `GRAFTABLE_RUNTIME_MEMBERS` is derived + * from this, so the list that decides what gets grafted and the list that + * decides what the lint stays quiet about cannot drift apart. + */ +export const STACK_RUNTIME_MEMBERS = Object.freeze(['onEnable', 'functions'] as const); + /** A plain object — the only shape an authored metadata item can take. */ export function isPlainRecord(v: unknown): v is Record { return !!v && typeof v === 'object' && !Array.isArray(v); diff --git a/packages/spec/src/index.ts b/packages/spec/src/index.ts index 35765bd56d..51c25d3199 100644 --- a/packages/spec/src/index.ts +++ b/packages/spec/src/index.ts @@ -98,6 +98,7 @@ export { defineObjectExtension } from './data/object.zod'; // tables and finding shape stay in data/ (frontend-safe). export { lintUnknownAuthoringKeys, + lintUnknownStackKeys, listLintableAuthoringCollections, } from './kernel/metadata-authoring-lint'; export type { LintableAuthoringCollection } from './kernel/metadata-authoring-lint'; @@ -105,6 +106,8 @@ export { formatUnknownAuthoringKey, FIELD_KEY_GUIDANCE, OBJECT_KEY_GUIDANCE, + STACK_KEY_GUIDANCE, + STACK_RUNTIME_MEMBERS, } from './data/authoring-key-lint'; export type { UnknownAuthoringKeyFinding, diff --git a/packages/spec/src/kernel/metadata-authoring-lint.test.ts b/packages/spec/src/kernel/metadata-authoring-lint.test.ts index ecadf24256..180971dd19 100644 --- a/packages/spec/src/kernel/metadata-authoring-lint.test.ts +++ b/packages/spec/src/kernel/metadata-authoring-lint.test.ts @@ -11,12 +11,16 @@ */ import { describe, it, expect } from 'vitest'; +import { z } from 'zod'; import { lintUnknownAuthoringKeys, + lintUnknownStackKeys, listLintableAuthoringCollections, } from './metadata-authoring-lint'; +import { STACK_KEY_GUIDANCE, STACK_RUNTIME_MEMBERS } from '../data/authoring-key-lint'; import { PLURAL_TO_SINGULAR } from '../shared/metadata-collection.zod'; +import { ObjectStackDefinitionSchema } from '../stack.zod'; import { getMetadataTypeSchema } from './metadata-type-schemas'; const lintables = listLintableAuthoringCollections(); @@ -135,3 +139,108 @@ describe('the #4148 behaviours survive the generalization', () => { expect(findings).toEqual([]); }); }); + +describe('top-level stack keys (#4167)', () => { + const lint = (raw: unknown) => lintUnknownStackKeys(raw, ObjectStackDefinitionSchema); + + it('covers ground the collection walker cannot reach', () => { + // The complementarity proof, and the reason this is a second pass rather + // than a tidier fold into the walker: the walker iterates COLLECTIONS, so a + // stack whose only mistake is at the envelope level — no objects, no pages, + // nothing to iterate — walks clean and reports nothing. + const raw = { storage: { adapter: 's3', s3: { bucket: 'app-files' } } }; + expect(lintUnknownAuthoringKeys(raw)).toEqual([]); + expect(lint(raw)).toHaveLength(1); + }); + + it('reports the worked example with its guidance and no suggestion', () => { + // `storage` is 5 edits from `sharingRules`; "did you mean sharingRules?" + // would be confident nonsense pointed at a real key. A `why` must win. + const [finding, ...rest] = lint({ storage: { adapter: 's3' } }); + expect(rest).toEqual([]); + expect(finding).toMatchObject({ path: 'stack.storage', surface: 'stack', key: 'storage' }); + expect(finding.suggestion).toBeUndefined(); + // The prescription has to name where the setting DOES live, or the author + // learns only that they were wrong — the #4167 complaint in miniature. + expect(finding.guidance).toContain('OS_STORAGE_'); + }); + + it('falls back to edit distance for a near-miss on a declared key', () => { + const [finding] = lint({ datasource: [{ name: 'db' }] }); + expect(finding).toMatchObject({ path: 'stack.datasource', suggestion: 'datasources' }); + }); + + it('is silent on a clean stack, and on the packaging channel', () => { + expect(lint({ objects: [], pages: [], manifest: { name: 'app' }, _packageId: 'p' })).toEqual([]); + }); + + it('stays silent on the runtime members the schema cannot declare', () => { + // The regression that shipped in review: `onEnable` is a function, so the + // schema does not declare it — but `AppPlugin` calls it off the authored + // bundle, and `examples/app-todo` and `examples/app-showcase` both ship it. + // The first version of this lint told both of them their working handler + // registration was "dropped at load". + expect(lint({ onEnable: () => {}, functions: { doThing: () => {} } })).toEqual([]); + }); + + it('still reports `onDisable`, which really does go nowhere', () => { + // The distinction the exclusion list has to preserve: `onDisable` is + // declared in the protocol but no kernel, runtime or service calls it, so + // a value written there IS lost and the author should hear about it. + const [finding, ...rest] = lint({ onDisable: () => {} }); + expect(rest).toEqual([]); + expect(finding).toMatchObject({ path: 'stack.onDisable', key: 'onDisable' }); + }); + + it('agrees with the injected schema posture instead of asserting its own', () => { + // Guarding the day `ObjectStackDefinitionSchema` graduates to `.strict()` + // (ADR-0049 / #4001): the parse becomes loud, and this lint must go quiet + // rather than become a second, possibly disagreeing voice. + const declared = { objects: z.array(z.unknown()).optional() }; + expect(lintUnknownStackKeys({ storage: {} }, z.strictObject(declared))).toEqual([]); + expect(lintUnknownStackKeys({ storage: {} }, z.looseObject(declared))).toEqual([]); + expect(lintUnknownStackKeys({ storage: {} }, z.object(declared))).toHaveLength(1); + }); + + it('survives malformed input rather than throwing', () => { + for (const junk of [undefined, null, 42, 'x', [], {}]) { + expect(() => lint(junk)).not.toThrow(); + expect(lint(junk)).toEqual([]); + } + expect(lintUnknownStackKeys({ storage: {} }, z.string())).toEqual([]); + expect(lintUnknownStackKeys({ storage: {} }, undefined)).toEqual([]); + }); +}); + +describe('STACK_KEY_GUIDANCE does not rot', () => { + const declared = new Set(Object.keys(ObjectStackDefinitionSchema.shape)); + + it('names no key the stack schema declares itself', () => { + // If a "not a stack key" key were ever added to the schema, the entry would + // be actively wrong — telling an author to delete something that now works. + for (const key of Object.keys(STACK_KEY_GUIDANCE)) { + expect(declared, `STACK_KEY_GUIDANCE has an entry for the LIVE key '${key}'`).not.toContain(key); + } + }); + + it('no runtime member is silently excluded for a key the schema declares', () => { + // `functions` legitimately appears in both lists. But if a member ever + // exists ONLY here while the schema also declares it and the runtime has + // stopped reading it, the exclusion is dead weight hiding a real finding. + // Pin the one that carries the exclusion's whole weight instead of trusting + // the list's shape: `onEnable` must be excluded AND undeclared. + expect(STACK_RUNTIME_MEMBERS).toContain('onEnable'); + expect(declared, 'onEnable became a declared key — the exclusion is now dead').not.toContain('onEnable'); + expect(STACK_RUNTIME_MEMBERS, 'onDisable is honoured nowhere; excluding it would hide a real drop') + .not.toContain('onDisable'); + }); + + it('every entry carries a rename target that exists, or a reason', () => { + expect(Object.keys(STACK_KEY_GUIDANCE).length).toBeGreaterThan(0); + for (const [key, hint] of Object.entries(STACK_KEY_GUIDANCE)) { + expect(hint.to ?? hint.why, `STACK_KEY_GUIDANCE.${key} needs a 'to' or a 'why'`).toBeTruthy(); + if (hint.to) expect(declared, `STACK_KEY_GUIDANCE.${key} → '${hint.to}'`).toContain(hint.to); + if (hint.why) expect(hint.why.length, `STACK_KEY_GUIDANCE.${key} reason too short`).toBeGreaterThan(30); + } + }); +}); diff --git a/packages/spec/src/kernel/metadata-authoring-lint.ts b/packages/spec/src/kernel/metadata-authoring-lint.ts index 9d1e7cd8f4..136800bdc6 100644 --- a/packages/spec/src/kernel/metadata-authoring-lint.ts +++ b/packages/spec/src/kernel/metadata-authoring-lint.ts @@ -53,6 +53,8 @@ import { isPlainRecord, FIELD_KEY_GUIDANCE, OBJECT_KEY_GUIDANCE, + STACK_KEY_GUIDANCE, + STACK_RUNTIME_MEMBERS, type UnknownAuthoringKeyFinding, } from '../data/authoring-key-lint'; import { FieldSchema } from '../data/field.zod'; @@ -218,3 +220,49 @@ export function lintUnknownAuthoringKeys(rawStack: unknown): UnknownAuthoringKey } return out; } + +/** + * Report every TOP-LEVEL key an authored stack sets that + * `ObjectStackDefinitionSchema` does not declare (#4167). + * + * The walker above covers every metadata COLLECTION; this covers the envelope + * those collections sit in. Same silence, one level up — and the level where it + * is hardest to notice, because an undeclared top-level key reads as + * configuration that took effect rather than as a typo. `storage` is the worked + * example: `os serve` honoured it only on the one boot path that skips + * `defineStack`, so a stack asking for S3 could silently get local disk. + * + * Separate from {@link lintUnknownAuthoringKeys} rather than folded into it + * because the stack schema has to be INJECTED: `stack.zod.ts` imports this + * module, so importing `ObjectStackDefinitionSchema` back would close a cycle. + * A separate export keeps that requirement visible — a call site either asks + * for this coverage or does not, and its absence shows up in a diff. An + * optional parameter on the walker would be the same silent-loss shape this + * whole rule family exists to report. + * + * @param rawStack The authored stack, after `normalizeStackInput` and before + * `ObjectStackDefinitionSchema.parse`. + * @param stackSchema `ObjectStackDefinitionSchema`, injected — see above. + */ +export function lintUnknownStackKeys( + rawStack: unknown, + stackSchema: unknown, +): UnknownAuthoringKeyFinding[] { + if (!isPlainRecord(rawStack)) return []; + + // Same posture rule the walker applies per collection: only a schema that + // STRIPS unknown keys has a silence worth reporting. If the stack schema is + // ever made strict, the parse rejects loudly on its own and this must go + // quiet rather than become a second, possibly disagreeing voice. + const posture = keyPosture(stackSchema); + if (!posture || posture.mode !== 'strip' || posture.keys.size === 0) return []; + + // The runtime members are honoured OFF the authored bundle, so the parse + // dropping them costs nothing — treating them as declared is what keeps the + // lint from calling a working `onEnable` discarded. See STACK_RUNTIME_MEMBERS. + const declared = new Set([...posture.keys, ...STACK_RUNTIME_MEMBERS]); + + const out: UnknownAuthoringKeyFinding[] = []; + lintAuthoredRecordKeys(rawStack, declared, STACK_KEY_GUIDANCE, 'stack', 'stack', out); + return out; +} diff --git a/packages/spec/src/stack.zod.ts b/packages/spec/src/stack.zod.ts index 89014a1a9a..9b85b2e2f2 100644 --- a/packages/spec/src/stack.zod.ts +++ b/packages/spec/src/stack.zod.ts @@ -12,7 +12,7 @@ import { objectStackErrorMap, formatZodError } from './shared/error-map.zod'; import { normalizeStackInput, type MetadataCollectionInput, type MapSupportedField } from './shared/metadata-collection.zod'; import type { ConversionNotice } from './conversions/types.js'; import { formatUnknownAuthoringKey } from './data/authoring-key-lint'; -import { lintUnknownAuthoringKeys } from './kernel/metadata-authoring-lint'; +import { lintUnknownAuthoringKeys, lintUnknownStackKeys } from './kernel/metadata-authoring-lint'; // Data Protocol import { ObjectSchema, ObjectExtensionSchema } from './data/object.zod'; @@ -1107,7 +1107,13 @@ const warnedUnknownAuthoringKeys = new Set(); * author deserves to hear about it either way. */ function warnUnknownAuthoringKeys(raw: unknown): void { - for (const finding of lintUnknownAuthoringKeys(raw)) { + const findings = [ + // Top level first: an undeclared envelope key is the one that reads as + // configuration that took effect (#4167). + ...lintUnknownStackKeys(raw, ObjectStackDefinitionSchema), + ...lintUnknownAuthoringKeys(raw), + ]; + for (const finding of findings) { if (warnedUnknownAuthoringKeys.has(finding.path)) continue; warnedUnknownAuthoringKeys.add(finding.path); console.warn(`defineStack: ${formatUnknownAuthoringKey(finding)}`);