diff --git a/.changeset/conversion-notice-channel.md b/.changeset/conversion-notice-channel.md new file mode 100644 index 0000000000..255d135357 --- /dev/null +++ b/.changeset/conversion-notice-channel.md @@ -0,0 +1,37 @@ +--- +"@objectstack/spec": minor +"@objectstack/cli": patch +--- + +fix(spec,cli): conversion deprecation notices reach the author, not just `os validate` (#3855) + +The ADR-0087 D2 conversion layer rewrites an old-shape key to its canonical +spelling at load and emits a structured `ConversionNotice` for each rewrite. The +conversion being silent about *fixing* the shape is the point — zero consumer +action. Being silent about having **had** to is not: the notice is the one signal +that says *this spelling retires in protocol N, and your metadata stops loading +then*. + +Two of the three surfaces that run the conversion pass discarded every notice: + +| Surface | Before | After | +|---|---|---| +| `os validate` | passed a sink, printed them | unchanged | +| `os build` / `os compile` | **passed no sink — notices discarded** | prints them, and includes a `conversions` array in `--json` under the same key `os validate --json` uses | +| `defineStack` | **passed no sink — notices discarded** | warns on the console, once per distinct conversion site | + +This is the #3782 parity class one layer down: not "does this command run the +gate" but "does it listen to what the gate says". Five conversions are live +today (protocol 11 and 15), so an author on any of those shapes was told by one +command and not the other two — and `defineStack` is where that author actually +is, since it runs inside their own config module. + +`defineStack` surfaces notices in **both** strict and non-strict mode: the +conversion happens on the shared `normalizeStackInput` call before the strict +branch, and `strict: false` does not make the old shape any less retiring. + +A new assertion in `validate-build-gate-parity.test.ts` fails if either command +calls `normalizeStackInput` without a sink, so the gap cannot silently reopen. + +No behaviour change for a stack already on canonical shapes: nothing converts, +so nothing warns. diff --git a/packages/cli/src/commands/compile.ts b/packages/cli/src/commands/compile.ts index 16521a4875..fb1426d40a 100644 --- a/packages/cli/src/commands/compile.ts +++ b/packages/cli/src/commands/compile.ts @@ -5,7 +5,7 @@ import path from 'path'; import fs from 'fs'; import chalk from 'chalk'; import { ZodError } from 'zod'; -import { ObjectStackDefinitionSchema, normalizeStackInput, lintDeprecatedAliases } from '@objectstack/spec'; +import { ObjectStackDefinitionSchema, normalizeStackInput, lintDeprecatedAliases, type ConversionNotice } from '@objectstack/spec'; import { loadConfig } from '../utils/config.js'; import { lowerCallables } from '../utils/lower-callables.js'; import { validateStackExpressions } from '@objectstack/lint'; @@ -92,8 +92,28 @@ export default class Compile extends Command { } // 2. Normalize map-formatted stack definition. + // The ADR-0087 D2 conversion layer runs here (inside normalizeStackInput). + // Each rewrite emits a structured deprecation notice, and this command + // used to drop every one of them: `os validate` passed a sink and + // surfaced them, `os build` passed none. That is the #3782 parity class + // — the two surfaces disagreeing about what an author is told — and it + // bites harder than it reads, because the notice is the ONLY warning an + // old-shape author gets before the conversion retires and their metadata + // stops loading. Five conversions are live today (protocol 11 and 15), + // so the gap is real, not hypothetical. if (!flags.json) printStep('Normalizing stack definition...'); - const normalized = normalizeStackInput(config as Record); + const conversionNotices: ConversionNotice[] = []; + const normalized = normalizeStackInput(config as Record, { + onConversionNotice: (n) => conversionNotices.push(n), + }); + if (conversionNotices.length > 0 && !flags.json) { + console.log(''); + for (const n of conversionNotices) { + printWarning( + `${n.path}: '${n.from}' → '${n.to}' (converted at load; conversion '${n.conversionId}', retires in protocol ${n.retiresIn})`, + ); + } + } // 2a. [#3743] PRE-PARSE authoring lint. Everything in the post-parse lint // block (3d and below) reads `result.data`, which is the wrong side of @@ -791,6 +811,9 @@ export default class Compile extends Command { runtimeModule: runtimeBundle?.outputFileName ?? null, runtimeModuleSize: runtimeBundle?.size ?? 0, warnings: widgetWarnings, + // Same key `os validate --json` uses, so a CI consumer reads one shape + // from either command rather than learning two. + conversions: conversionNotices, specVersionGap: specGap, stats, duration: timer.elapsed(), diff --git a/packages/cli/test/validate-build-gate-parity.test.ts b/packages/cli/test/validate-build-gate-parity.test.ts index 5df516e042..1f35a17c7c 100644 --- a/packages/cli/test/validate-build-gate-parity.test.ts +++ b/packages/cli/test/validate-build-gate-parity.test.ts @@ -77,4 +77,30 @@ describe('os validate is the read-only superset of os build (#3782)', () => { expect(validateGates.has(gate), `validate.ts must call ${gate}`).toBe(true); } }); + + /** + * The same drift, one layer down and easier to miss: not "does this command + * run the gate" but "does it LISTEN to what the gate says". The ADR-0087 D2 + * conversion pass runs inside `normalizeStackInput` on both commands, so both + * always converted — but only `os validate` passed an `onConversionNotice` + * sink, so `os build` silently discarded every deprecation notice. A notice + * is the one warning an old-shape author gets before the conversion retires + * and their metadata stops loading, and five conversions are live today. + * + * Source-level for the same reason as the gate check above: it fails when the + * sink is dropped, which is the moment it is cheap to fix. + */ + it('both commands pass a conversion-notice sink to normalizeStackInput', () => { + for (const file of ['compile.ts', 'validate.ts']) { + const src = readFileSync(join(COMMANDS_DIR, file), 'utf8'); + const call = src.match(/normalizeStackInput\([\s\S]{0,400}?\)\s*;/); + expect(call, `${file} must call normalizeStackInput`).not.toBeNull(); + expect( + call![0].includes('onConversionNotice'), + `${file} calls normalizeStackInput without an onConversionNotice sink, so every ADR-0087 ` + + `D2 deprecation notice it raises is discarded. Pass a sink and surface the notices ` + + `(mirror the other command).`, + ).toBe(true); + } + }); }); diff --git a/packages/spec/src/stack.test.ts b/packages/spec/src/stack.test.ts index 145cc10831..504b8937a7 100644 --- a/packages/spec/src/stack.test.ts +++ b/packages/spec/src/stack.test.ts @@ -1468,3 +1468,78 @@ describe('defineStack — deprecated alias warnings (#3743)', () => { expect('conditionalRequired' in field).toBe(false); }); }); + +// ── ADR-0087 D2 conversion notices reach the author ───────────────────────── +// +// A conversion is deliberately silent about FIXING the shape — zero consumer +// action is the point. It is not supposed to be silent about having had to: +// the notice carries "retires in protocol N", after which the old spelling +// stops loading. `defineStack` passed no sink, so the author who wrote the old +// shape heard nothing unless they happened to run `os validate` (`os build` +// was deaf too). Five conversions are live today, so this was a real gap. +describe('defineStack — ADR-0087 D2 conversion notices', () => { + const manifest = { id: 'p', version: '1.0.0', type: 'app' as const, name: 'P', namespace: 'demo' }; + const obj = { name: 'demo_task', label: 'Task', fields: { title: { type: 'text' as const } } }; + + // A protocol-11 flow callout node type — `webhook` converts to `http`. + // + // The warn-once key is (conversionId, path, from, to) and the notice path is + // INDEX-based (`flows[i].nodes[j].type`) — the flow's name never enters it. + // Since the dedupe set is process-scoped by design, every case below has to + // occupy its own node index or it silently reuses an earlier case's key and + // asserts nothing. + const legacyFlowAt = (index: number) => ({ + name: `conv_demo_${index}`, + nodes: [ + ...Array.from({ length: index }, (_, i) => ({ id: `n_pad_${i}`, type: 'http' })), + { id: 'n_call', type: 'webhook' }, + ], + }); + // Non-strict throughout: the assertion is about the conversion sink alone, + // not about satisfying every downstream cross-reference check. The sink is + // wired on the single `normalizeStackInput` call that runs BEFORE the strict + // branch, so both modes go through it. + const define = (index: number) => + defineStack({ manifest, objects: [obj], flows: [legacyFlowAt(index)] }, { strict: false }); + + let warn: ReturnType; + beforeEach(() => { + warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + }); + afterEach(() => { + warn.mockRestore(); + }); + + it('warns when a conversion rewrites an old shape, naming the retirement major', () => { + define(0); + + expect(warn).toHaveBeenCalledTimes(1); + const msg = warn.mock.calls[0][0] as string; + expect(msg).toContain(`'webhook' → 'http'`); + expect(msg).toContain('flow-node-http-callout-rename'); + expect(msg).toContain('retires in protocol'); + }); + + it('nags once per distinct conversion site', () => { + define(1); + define(1); + define(1); + expect(warn).toHaveBeenCalledTimes(1); + }); + + it('reports a second, distinct site separately', () => { + // Two different authored occurrences, each needing its own fix — dedupe + // must not swallow the second. + define(2); + define(3); + expect(warn).toHaveBeenCalledTimes(2); + }); + + it('stays quiet for a stack that is already canonical', () => { + defineStack( + { manifest, objects: [obj], flows: [{ name: 'conv_canonical', nodes: [{ id: 'n1', type: 'http' }] }] }, + { strict: false }, + ); + expect(warn).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/spec/src/stack.zod.ts b/packages/spec/src/stack.zod.ts index f9f58fe73c..f094d5551b 100644 --- a/packages/spec/src/stack.zod.ts +++ b/packages/spec/src/stack.zod.ts @@ -11,6 +11,7 @@ import { hasPlatformObjectPrefix } from './system/constants/platform-object-name import { objectStackErrorMap, formatZodError } from './shared/error-map.zod'; import { normalizeStackInput, type MetadataCollectionInput, type MapSupportedField } from './shared/metadata-collection.zod'; import { lintDeprecatedAliases, formatDeprecatedAliasFinding } from './shared/deprecated-aliases'; +import type { ConversionNotice } from './conversions/types.js'; // Data Protocol import { ObjectSchema, ObjectExtensionSchema } from './data/object.zod'; @@ -1068,6 +1069,33 @@ function warnDeprecatedAliases(normalized: Record): void { } } +/** Conversion notices already reported this process — same warn-once reason. */ +const warnedConversionNotices = new Set(); + +/** + * Surface the ADR-0087 D2 conversion notices raised while normalizing. + * + * A conversion is deliberately silent about *fixing* the shape — zero consumer + * action is the point — but it is not supposed to be silent about having HAD to. + * The notice is the one signal that says "this spelling retires in protocol N, + * and your metadata stops loading then", and `defineStack` is where the author + * who wrote the old shape actually is. Until now it passed no sink, so that + * author heard nothing unless they happened to run `os validate` — the same gap + * this change closes in `os build`. + * + * Advisory and warn-once: the conversion already produced a correct stack. + */ +function warnConversionNotice(notice: ConversionNotice): void { + const key = `${notice.conversionId} ${notice.path} ${notice.from} ${notice.to}`; + if (warnedConversionNotices.has(key)) return; + warnedConversionNotices.add(key); + console.warn( + `defineStack: ${notice.path}: '${notice.from}' → '${notice.to}' (converted at load; ` + + `conversion '${notice.conversionId}', retires in protocol ${notice.retiresIn}). ` + + `Update the source to the canonical shape — the conversion stops running then.`, + ); +} + export function defineStack( config: ObjectStackDefinitionInput, options?: DefineStackOptions, @@ -1075,8 +1103,14 @@ export function defineStack( // Default to strict=true for safety (validate by default) const strict = options?.strict !== false; - // Normalize map-formatted collections to arrays (key → name injection) - const normalized = normalizeStackInput(config as Record); + // Normalize map-formatted collections to arrays (key → name injection), and + // surface every ADR-0087 D2 conversion the pass had to apply. Unlike the alias + // warning below this runs in BOTH modes: a conversion happens whether or not + // we go on to parse, so `strict: false` does not make the old shape any less + // retiring. + const normalized = normalizeStackInput(config as Record, { + onConversionNotice: warnConversionNotice, + }); if (!strict) { // Non-strict mode: skip validation (advanced use cases only).