From bb9b508305c7f4418e5aa50af038bbd1a991c609 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 28 Jul 2026 13:19:14 +0000 Subject: [PATCH] fix(spec,cli): conversion deprecation notices reach the author, not just `os validate` (#3855) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The ADR-0087 D2 conversion layer rewrites an old-shape key to canonical at load and emits a structured `ConversionNotice` per rewrite. 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 pass discarded every notice: os validate — passed a sink, printed them. os build — passed NO sink. Every notice dropped. defineStack — passed NO sink. Every notice dropped. That 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 one 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. `os build` now prints the notices and includes a `conversions` array in `--json` under the same key `os validate --json` already uses, so a CI consumer reads one shape from either command. `defineStack` warns once per distinct conversion site, 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 — verified non-vacuous by stripping the sink and watching it go red. The `defineStack` tests document a sharp edge worth knowing: the notice path is index-based (`flows[i].nodes[j].type`) and the warn-once set is process-scoped, so fixtures must occupy distinct node indices or they silently reuse an earlier case's dedupe key and assert nothing. Groundwork for #3855: once the three deprecated aliases graduate into the conversion layer, this is their only warning channel. No behaviour change for a stack already on canonical shapes — nothing converts, so nothing warns (the showcase's no-startup-warnings test still passes). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JqC9ckaLbmhmytt65Gxi96 --- .changeset/conversion-notice-channel.md | 37 +++++++++ packages/cli/src/commands/compile.ts | 27 ++++++- .../test/validate-build-gate-parity.test.ts | 26 +++++++ packages/spec/src/stack.test.ts | 75 +++++++++++++++++++ packages/spec/src/stack.zod.ts | 38 +++++++++- 5 files changed, 199 insertions(+), 4 deletions(-) create mode 100644 .changeset/conversion-notice-channel.md 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).