diff --git a/.changeset/18978-aggregate-surface-scope.md b/.changeset/18978-aggregate-surface-scope.md new file mode 100644 index 0000000000..df4d584158 --- /dev/null +++ b/.changeset/18978-aggregate-surface-scope.md @@ -0,0 +1,50 @@ +--- +'@objectstack/spec': minor +--- + +fix(spec): `spec-changes.json`'s aggregate export diff declares the release pair it really spans (#18978) + +Clause-②: yes (widening) — one new OPTIONAL key on a published artifact (`aggregate.surfaceScope`) +and one new optional field on `SpecChangesSchema`. Nothing is renamed, retired or reshaped: the +schema still ACCEPTS a record without it, every existing key keeps its spelling and meaning, and +`perMajor` and the `release` section are byte-identical. Contract-review tier. + +`aggregate.added` / `aggregate.removed` are not registry-derived. A release-time api-surface diff +fills them by comparing the artifact being published against the previously **published** one, so +they span **one release** — while the record they sit in is keyed by protocol major (`from: 10, +to: 17`) and every entry carries only `since: 17` / `removedIn: 17`, with +`perMajor[16 → 17].added` at `0` beside it. Nothing in the file distinguished one minor's slice +from the whole major-boundary delta. + +Measured on the published `@objectstack/spec@17.4.0` Release asset: `aggregate.added` = **225**, +`aggregate.removed` = **51**, every entry `since`/`removedIn` = 17 — and set-identical to a +recomputed `17.3.0 → 17.4.0` diff of the two tarballs' own `api-surface/` snapshots. It was the +minor's delta wearing a major's label. + +**What ships now.** A record whose export arrays are non-empty carries the version pair they were +diffed between: + +```bash +jq '.aggregate | {from, to, surfaceScope, added: (.added | length), removed: (.removed | length)}' \ + node_modules/@objectstack/spec/spec-changes.json +``` + +- `surfaceScope: { fromVersion, toVersion }` present ⇒ `added`/`removed` span exactly that + published-version pair. ⛔ They are **not** the `from` → `to` major delta, and never were. +- `surfaceScope` absent ⇒ the record carries no export diff at all and `added`/`removed` are + empty. ⛔ Read that as "this record does not say", never as "nothing was added between `from` + and `to`" — the same rule the `release` section already states for itself. +- `from` / `to` still answer the major-boundary question for `converted` / `migrated`, which are + registry-derived and unaffected. + +**Refused at the producer and at the publish gate, in both directions.** The generator reads the +previous version off the previous artifact's own `package.json`, omits the arrays loudly when it +cannot read one, and refuses outright to write a non-empty unlabelled array. +`scripts/check-release-spec-changes.mjs` — which until now checked the `release` section and not +the aggregate — recomputes the aggregate's claim from the two tarballs and refuses an absent, +mislabelled or untrue scope. Its self-test roster grows from 15 batteries to 23. + +**Nothing previously honest moved.** The committed registry-only projection and every `perMajor` +record carry no new key at all; the committed `spec-changes.json` changes on its `$comment` line +and nowhere else. The published schema is deliberately not narrowed — every manifest published so +far carries an unscoped diff and must keep parsing. diff --git a/content/docs/upgrading.mdx b/content/docs/upgrading.mdx index bf18219095..59f4220ba0 100644 --- a/content/docs/upgrading.mdx +++ b/content/docs/upgrading.mdx @@ -335,8 +335,27 @@ jq '.release | {fromVersion, toVersion, named — `"./ai: AgentSchema (const)"`, the entry point followed by the export and its kind. `converted` and `migrated` are the ADR-0087 conversions and semantic migrations first registered in that release. The same file's -`aggregate` and `perMajor` records are unchanged and still answer the -major-boundary question. +`perMajor` records are unchanged and still answer the major-boundary question, +and so does `aggregate` — for its `converted` and `migrated`, which are derived +from the ADR-0087 registries across the whole `from` → `to` range. + +⛔ **But not for `aggregate.added` / `aggregate.removed`.** Those come from the +same one-release export diff as the section above, not from the major range the +record is keyed by, so when they are filled the `aggregate` record carries a +`surfaceScope` naming the exact pair they span: + +```bash +jq '.aggregate | {from, to, surfaceScope, + added: (.added | length), removed: (.removed | length)}' \ + node_modules/@objectstack/spec/spec-changes.json +``` + +`surfaceScope` absent means that record claims no export diff at all and its +`added` / `removed` are empty — the registry-only shape. ⛔ Read that as "this +record does not say", never as "nothing was added between `from` and `to`", +which is the same rule the `release` section states for itself below. A release +whose `aggregate` arrays disagree with the two published tarballs, or carry no +`surfaceScope`, does not publish. The `os` CLI reads the same section, so a CI job does not have to know the file exists: diff --git a/packages/spec/api-surface-declarations/root.txt b/packages/spec/api-surface-declarations/root.txt index f9464b07d8..27a726614a 100644 --- a/packages/spec/api-surface-declarations/root.txt +++ b/packages/spec/api-surface-declarations/root.txt @@ -44929,6 +44929,10 @@ declare const SpecChangesSchema: z.ZodObject<{ removedIn: z.ZodNumber; replacement: z.ZodOptional; }, z.core.$strip>>; + surfaceScope: z.ZodOptional>; }, z.core.$strip>; // ── SpecConverted (type) ── @@ -45036,6 +45040,8 @@ type StoredConversionOptions = Omit; interface SurfaceDiff { added?: SpecSurfaceAdd[]; removed?: SpecSurfaceRemove[]; + /** The published-version pair `added`/`removed` were diffed between. */ + scope?: SpecSurfaceScope; } // ── TemplateExpressionInputSchema (const) ── diff --git a/packages/spec/scripts/build-spec-changes.ts b/packages/spec/scripts/build-spec-changes.ts index 12c4135338..2ab626b181 100644 --- a/packages/spec/scripts/build-spec-changes.ts +++ b/packages/spec/scripts/build-spec-changes.ts @@ -18,10 +18,19 @@ * Release-time surface join: `--previous-surface ` diffs the current * committed export surface against a previously *published* one (both ship in * the npm artifact from protocol 15 on) and fills the `added[]`/`removed[]` - * arrays of the aggregate record, attributed to the current major. The Release - * workflow runs this against the last published spec tarball and attaches the - * result to the GitHub Release; the committed copy keeps `added`/`removed` - * empty (registry-derived content only) so it stays deterministic. + * arrays of the aggregate record. The Release workflow runs this against the + * last published spec tarball and attaches the result to the GitHub Release; the + * committed copy keeps `added`/`removed` empty (registry-derived content only) + * so it stays deterministic. + * + * ⚠️ That diff is ONE RELEASE wide while the aggregate record is keyed by + * protocol MAJOR (`from: 10, to: 17`), so the arrays ship with + * `surfaceScope: { fromVersion, toVersion }` naming the pair they really span. + * Without it a consumer read one minor's 225-export slice as the whole 10 → 17 + * delta — with `perMajor[16 → 17].added` sitting at `0` beside it and no field + * distinguishing the two. The previous version is read off the previous + * artifact's own `package.json`; when it cannot be read the arrays are OMITTED, + * loudly, and a non-empty unlabelled array is refused outright. * * `` is whichever shape that published tarball carried: the `api-surface/` * directory from #5837 on, or the single `api-surface.json` before it. Reading @@ -68,9 +77,11 @@ import { composeSpecChanges, SpecChangesSchema, SpecReleaseChangesSchema, + surfaceScopeProblem, type SpecReleaseChanges, type SpecSurfaceAdd, type SpecSurfaceRemove, + type SpecSurfaceScope, } from '../src/migrations/spec-changes'; import { API_SURFACE_DIR_NAME, readApiSurfaceFrom } from './lib/sharded-artifacts'; @@ -104,6 +115,39 @@ const PREV_SURFACE = PREV_PACKAGE ? process.argv[prevSurfaceIdx + 1] : undefined; +/** + * The `version` of the unpacked published tarball an export snapshot came out + * of, or `null` when the snapshot's path does not sit inside one. + * + * `--previous-package` points at the `package/` root, so the manifest is right + * there; `--previous-surface` points at the snapshot itself (`api-surface/` or + * `api-surface.json`), whose parent is that same root in every shape the release + * lane has ever produced. Read, never transcribed — the same discipline + * {@link previousRelease} already applies to the registry ids. + */ +function publishedVersionAt(pkgDir: string): string | null { + const pkgPath = resolve(pkgDir, 'package.json'); + if (!existsSync(pkgPath)) return null; + const version = (JSON.parse(readFileSync(pkgPath, 'utf8')) as { version?: string }).version; + return typeof version === 'string' && version.length > 0 ? version : null; +} + +/** + * The version pair the aggregate's `added`/`removed` really span, or `null` when + * the previous release's version cannot be read off the inputs. + * + * ⛔ `null` is not "omit the label" — the caller then omits the ARRAYS, loudly. + * An unlabelled export diff under a major-keyed record is the defect this whole + * field exists to end, so producing it would be worse than producing nothing. + */ +function surfaceScope(): SpecSurfaceScope | null { + const root = PREV_PACKAGE ?? (PREV_SURFACE ? resolve(PREV_SURFACE, '..') : undefined); + if (!root) return null; + const fromVersion = publishedVersionAt(root); + if (!fromVersion) return null; + return { fromVersion, toVersion: THIS_VERSION }; +} + /** Flatten an export surface ({ entry: ["name (kind)", …] }) into one set. */ function flattenSurface(path: string): Set { const doc = readApiSurfaceFrom(path); @@ -186,7 +230,23 @@ function buildReleaseSection(current: ReturnType): Sp } function build(): string { - const surfaceDiff = PREV_SURFACE ? diffSurfaces(PREV_SURFACE) : {}; + // The export diff spans ONE RELEASE, so it ships only with the version pair + // that says so. No readable previous version ⇒ no arrays, and the reason is + // printed: an unlabelled slice under the MAJOR-keyed aggregate record is read + // as the whole major-boundary delta, which is strictly worse than an empty + // one — the same call `buildReleaseSection` makes for the same reason. + const scope = surfaceScope(); + let surfaceDiff: ReturnType | { scope?: SpecSurfaceScope } = {}; + if (PREV_SURFACE && scope) { + surfaceDiff = { ...diffSurfaces(PREV_SURFACE), scope }; + } else if (PREV_SURFACE) { + console.error( + `No aggregate export diff: the previous artifact at ${PREV_PACKAGE ?? PREV_SURFACE} carries no ` + + 'readable package.json, so the version pair the diff spans cannot be read. Omitting ' + + '`added`/`removed` — an unlabelled one-release slice under the major-keyed aggregate record ' + + 'reads as the whole from → to delta.', + ); + } // Per-major records compose (ADR-0087 D4): any tool can fold them into a // single from→to view. The aggregate is that fold, precomputed. @@ -197,6 +257,11 @@ function build(): string { const aggregate = SpecChangesSchema.parse( composeSpecChanges(MIGRATION_SUPPORT_FLOOR, PROTOCOL_MAJOR, surfaceDiff), ); + const problem = surfaceScopeProblem(aggregate); + if (problem) { + console.error(`Refusing to write ${SNAPSHOT}: ${problem}`); + process.exit(1); + } const release = buildReleaseSection(aggregate); const doc = { @@ -204,6 +269,12 @@ function build(): string { 'GENERATED (ADR-0087 D4) — do not edit. Regenerate with: pnpm --filter @objectstack/spec gen:spec-changes. ' + 'A projection of the D2 conversion table + D3 migration chain; the upgrade guide and the MCP spec_changes ' + 'tool derive from this same data. ' + + 'A record\'s `added`/`removed` are NOT at its `from` → `to` MAJOR resolution: they come from an ' + + 'api-surface diff against the previously PUBLISHED artifact, so they span ONE RELEASE. When they are ' + + 'non-empty the record carries `surfaceScope: { fromVersion, toVersion }` naming exactly that pair, and a ' + + 'release whose arrays disagree with the two tarballs — or carry no `surfaceScope` — does not publish. ' + + 'Absent `surfaceScope` means the record carries no export diff at all (`added`/`removed` empty), never ' + + '"nothing was added between from and to". ' + 'When a `release` section is present, its four ADR-0087 D4 arrays report what that release ADDED: ' + '`added`/`removed` are the export-surface diff of the two published tarballs, and `converted`/`migrated` ' + 'are the D2/D3 ids FIRST REGISTERED in it. An id that LEFT the published chain between the two releases ' + diff --git a/packages/spec/spec-changes.json b/packages/spec/spec-changes.json index 9dbc98682b..43601bf31f 100644 --- a/packages/spec/spec-changes.json +++ b/packages/spec/spec-changes.json @@ -1,5 +1,5 @@ { - "$comment": "GENERATED (ADR-0087 D4) — do not edit. Regenerate with: pnpm --filter @objectstack/spec gen:spec-changes. A projection of the D2 conversion table + D3 migration chain; the upgrade guide and the MCP spec_changes tool derive from this same data. When a `release` section is present, its four ADR-0087 D4 arrays report what that release ADDED: `added`/`removed` are the export-surface diff of the two published tarballs, and `converted`/`migrated` are the D2/D3 ids FIRST REGISTERED in it. An id that LEFT the published chain between the two releases is reported in none of them — `converted: []` means \"this release registered none\", never \"none was withdrawn\"; a withdrawal is visible only by comparing two published manifests.", + "$comment": "GENERATED (ADR-0087 D4) — do not edit. Regenerate with: pnpm --filter @objectstack/spec gen:spec-changes. A projection of the D2 conversion table + D3 migration chain; the upgrade guide and the MCP spec_changes tool derive from this same data. A record's `added`/`removed` are NOT at its `from` → `to` MAJOR resolution: they come from an api-surface diff against the previously PUBLISHED artifact, so they span ONE RELEASE. When they are non-empty the record carries `surfaceScope: { fromVersion, toVersion }` naming exactly that pair, and a release whose arrays disagree with the two tarballs — or carry no `surfaceScope` — does not publish. Absent `surfaceScope` means the record carries no export diff at all (`added`/`removed` empty), never \"nothing was added between from and to\". When a `release` section is present, its four ADR-0087 D4 arrays report what that release ADDED: `added`/`removed` are the export-surface diff of the two published tarballs, and `converted`/`migrated` are the D2/D3 ids FIRST REGISTERED in it. An id that LEFT the published chain between the two releases is reported in none of them — `converted: []` means \"this release registered none\", never \"none was withdrawn\"; a withdrawal is visible only by comparing two published manifests.", "protocolVersion": "17.0.0", "supportFloor": 10, "migrateCommand": "objectstack migrate meta --from (N >= 10)", diff --git a/packages/spec/src/migrations/spec-changes-surface-scope.test.ts b/packages/spec/src/migrations/spec-changes-surface-scope.test.ts new file mode 100644 index 0000000000..124949134d --- /dev/null +++ b/packages/spec/src/migrations/spec-changes-surface-scope.test.ts @@ -0,0 +1,144 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The aggregate record's export arrays must say which range they cover (#18978). + * + * `added`/`removed` are not registry-derived: a release-time api-surface diff + * fills them by comparing the artifact being published against the previously + * PUBLISHED one, so they span ONE RELEASE. Under a record keyed `from: 10, + * to: 17` that was indistinguishable from the whole major-boundary delta — and + * the `@objectstack/spec@17.4.0` Release asset really carried 225 added / 51 + * removed, every entry `since: 17`, while `perMajor[16 → 17].added` sat at `0`. + * + * The three things pinned here, in the order they matter: + * + * 1. a scoped diff carries the version pair, and the pair is what a consumer + * reads to tell a minor's slice from a major's; + * 2. an UNSCOPED non-empty diff is refused by {@link surfaceScopeProblem} — and + * is still ACCEPTED by {@link SpecChangesSchema}, deliberately, because a + * previously published manifest carries exactly that shape and a schema that + * refused it would narrow what an already-shipped artifact parses as; + * 3. the records that are honest today stay byte-identical: a `perMajor` record + * and the committed registry-only aggregate carry no new key at all. + */ + +import { describe, expect, it } from 'vitest'; + +import { PROTOCOL_MAJOR } from '../kernel/protocol-version.js'; +import { MIGRATION_SUPPORT_FLOOR } from './registry.js'; +import { + composeReleaseChanges, + composeSpecChanges, + SpecChangesSchema, + SpecSurfaceScopeSchema, + surfaceScopeProblem, +} from './spec-changes.js'; + +/** One release's worth of export diff, the shape the publish lane supplies. */ +const ONE_RELEASE_SLICE = { + added: [ + { surface: './api: AnalyticsEndpoint (const)', since: PROTOCOL_MAJOR }, + { surface: './api: ApiEndpoint (const)', since: PROTOCOL_MAJOR }, + ], + removed: [{ surface: './integration: ConnectorErrorCategory (type)', removedIn: PROTOCOL_MAJOR }], +}; +const SCOPE = { fromVersion: '17.3.0', toVersion: '17.4.0' }; + +describe('aggregate export arrays declare the range they really cover (#18978)', () => { + it('carries the published-version pair the diff was taken between', () => { + const aggregate = composeSpecChanges(MIGRATION_SUPPORT_FLOOR, PROTOCOL_MAJOR, { + ...ONE_RELEASE_SLICE, + scope: SCOPE, + }); + + // The record is still keyed by MAJOR — that half is unchanged — and the + // arrays now name the release pair, so the two resolutions are separable. + expect(aggregate.from).toBe(MIGRATION_SUPPORT_FLOOR); + expect(aggregate.to).toBe(PROTOCOL_MAJOR); + expect(aggregate.surfaceScope).toEqual(SCOPE); + expect(SpecSurfaceScopeSchema.safeParse(aggregate.surfaceScope).success).toBe(true); + + const parsed = SpecChangesSchema.safeParse(aggregate); + expect(parsed.success).toBe(true); + expect(parsed.success && parsed.data.surfaceScope).toEqual(SCOPE); + }); + + it('refuses a non-empty export diff that names no range', () => { + const unscoped = composeSpecChanges(MIGRATION_SUPPORT_FLOOR, PROTOCOL_MAJOR, ONE_RELEASE_SLICE); + expect(unscoped.surfaceScope).toBeUndefined(); + + const problem = surfaceScopeProblem(unscoped); + expect(problem).not.toBeNull(); + // The refusal must name both counts and the range it would be misread as — + // a bare "missing surfaceScope" leaves the reader to rediscover why. + expect(problem).toContain('2 added'); + expect(problem).toContain('1 removed'); + expect(problem).toContain(`${MIGRATION_SUPPORT_FLOOR} → ${PROTOCOL_MAJOR}`); + expect(problem).toContain('surfaceScope'); + }); + + it('is refused by the producer and still ACCEPTED by the schema', () => { + // Not a contradiction, and the one thing that keeps this additive: every + // manifest published before this field exists carries an unscoped diff, so + // the published schema must keep parsing it. The refusal lives at the + // producer and at the publish gate, never in the accept set. + const unscoped = composeSpecChanges(MIGRATION_SUPPORT_FLOOR, PROTOCOL_MAJOR, ONE_RELEASE_SLICE); + expect(surfaceScopeProblem(unscoped)).not.toBeNull(); + expect(SpecChangesSchema.safeParse(unscoped).success).toBe(true); + }); + + it('passes a scoped diff and an empty one alike', () => { + expect( + surfaceScopeProblem( + composeSpecChanges(MIGRATION_SUPPORT_FLOOR, PROTOCOL_MAJOR, { ...ONE_RELEASE_SLICE, scope: SCOPE }), + ), + ).toBeNull(); + expect(surfaceScopeProblem(composeSpecChanges(MIGRATION_SUPPORT_FLOOR, PROTOCOL_MAJOR))).toBeNull(); + }); +}); + +describe('preserved truth — the records that are honest today are untouched', () => { + it('a registry-only record carries no surfaceScope KEY at all', () => { + const aggregate = composeSpecChanges(MIGRATION_SUPPORT_FLOOR, PROTOCOL_MAJOR); + // `in`, not `=== undefined`: the committed artifact is compared byte-for-byte + // by `check:spec-changes`, so a present-but-null key would be a diff. + expect('surfaceScope' in aggregate).toBe(false); + expect(Object.keys(aggregate).sort()).toEqual(['added', 'converted', 'from', 'migrated', 'removed', 'to']); + expect(aggregate.added).toEqual([]); + expect(aggregate.removed).toEqual([]); + }); + + it('every perMajor record keeps its exact shape and its counts', () => { + for (let major = MIGRATION_SUPPORT_FLOOR + 1; major <= PROTOCOL_MAJOR; major++) { + const record = composeSpecChanges(major - 1, major); + expect('surfaceScope' in record).toBe(false); + expect(record.added).toEqual([]); + expect(record.removed).toEqual([]); + expect(SpecChangesSchema.safeParse(record).success).toBe(true); + } + }); + + it('the per-release section is unchanged — same six keys, no scope on it', () => { + const current = composeSpecChanges(MIGRATION_SUPPORT_FLOOR, PROTOCOL_MAJOR); + const section = composeReleaseChanges( + '17.3.0', + '17.4.0', + current, + { conversionIds: current.converted.map((c) => c.conversionId), migrationIds: [] }, + { added: ['./api: ApiEndpoint (const)'], removed: [] }, + ); + expect(Object.keys(section).sort()).toEqual([ + 'added', + 'converted', + 'fromVersion', + 'migrated', + 'removed', + 'toVersion', + ]); + // Its attribution already lives on the section, which is why its entries + // are bare `{ surface }` — the aggregate now says the same thing its own way. + expect(section.added).toEqual([{ surface: './api: ApiEndpoint (const)' }]); + expect(section.fromVersion).toBe('17.3.0'); + expect(section.toVersion).toBe('17.4.0'); + }); +}); diff --git a/packages/spec/src/migrations/spec-changes.ts b/packages/spec/src/migrations/spec-changes.ts index 06f0e5d376..2e046d0845 100644 --- a/packages/spec/src/migrations/spec-changes.ts +++ b/packages/spec/src/migrations/spec-changes.ts @@ -25,6 +25,13 @@ * consumer gets one aggregate answer instead of N documents to reconcile. * {@link composeSpecChanges} is that fold, computed from the registries; the * `added`/`removed` arrays are supplied by the release-time api-surface diff. + * + * ⚠️ **Those two arrays are NOT at the record's `from` → `to` resolution**, and + * that is what {@link SpecSurfaceScopeSchema} exists to say out loud: the diff + * that fills them compares this artifact against the previously PUBLISHED one, + * so they span one release, not the major range the record is keyed by. A record + * whose arrays are non-empty carries `surfaceScope` naming that version pair, + * and {@link surfaceScopeProblem} is what refuses one that does not. */ import { z } from 'zod'; @@ -48,6 +55,47 @@ export const SpecSurfaceRemoveSchema = z }) .describe('A removed public export.'); +/** + * The published-version pair a record's `added`/`removed` arrays were actually + * diffed between (ADR-0087 D4). + * + * ## Why the arrays need this, and why a major on each entry was not enough + * + * `added`/`removed` are not registry-derived: they are supplied by a release-time + * api-surface diff of the artifact being published against the previously + * PUBLISHED one. That diff is **one release wide**. Under an aggregate record + * keyed `from: 10, to: 17` the arrays therefore looked like the whole + * major-boundary delta, while every entry carried only `since: 17` / + * `removedIn: 17` — true of the entry (it did arrive in major 17) and false of + * the array (major 17's earlier minors are not in it), with + * `perMajor[16 → 17].added/removed` sitting at `0`/`0` beside it. A consumer had + * no field to tell the two apart, which is the one thing a machine-readable + * surface may not do. + * + * So the scope is declared once, on the record, rather than repeated on 400 + * entries — the same choice {@link SpecReleaseChangesSchema} already makes for + * the same reason. Present means "these arrays span exactly this version pair"; + * absent means the record carries no export diff at all (the committed, + * registry-only projection, and every `perMajor` record). + * + * ⛔ It is deliberately NOT enforced by {@link SpecChangesSchema}: a previously + * published manifest carries unscoped arrays, and a schema that refused those + * would narrow what an already-shipped artifact parses as. The producer + * ({@link surfaceScopeProblem}, called by `scripts/build-spec-changes.ts`) and + * the publish gate (`scripts/check-release-spec-changes.mjs`) are where it is + * refused. + */ +export const SpecSurfaceScopeSchema = z + .object({ + fromVersion: z + .string() + .describe('The previously published @objectstack/spec version the export diff started at.'), + toVersion: z + .string() + .describe('The @objectstack/spec version this artifact ships — where every entry arrived or left.'), + }) + .describe('The published-version pair an export-surface diff was computed between.'); + /** A losslessly converted surface (from the D2 conversion table). */ export const SpecConvertedSchema = z .object({ @@ -129,6 +177,11 @@ export const SpecChangesSchema = z converted: z.array(SpecConvertedSchema), migrated: z.array(SpecMigratedSchema), removed: z.array(SpecSurfaceRemoveSchema), + surfaceScope: SpecSurfaceScopeSchema.optional().describe( + 'The published-version pair `added`/`removed` were diffed between. Absent exactly when ' + + 'this record carries no export diff — ⛔ `added`/`removed` are then empty and say nothing ' + + 'about the `from` → `to` range, and a non-empty array without this key is refused at publish.', + ), }) .describe('ADR-0087 D4 machine-readable change manifest for a protocol version pair.'); @@ -137,6 +190,7 @@ export type SpecSurfaceRemove = z.infer; export type SpecConverted = z.infer; export type SpecMigrated = z.infer; export type SpecChanges = z.infer; +export type SpecSurfaceScope = z.infer; export type SpecReleaseSurface = z.infer; export type SpecReleaseChanges = z.infer; @@ -144,6 +198,31 @@ export type SpecReleaseChanges = z.infer; export interface SurfaceDiff { added?: SpecSurfaceAdd[]; removed?: SpecSurfaceRemove[]; + /** The published-version pair `added`/`removed` were diffed between. */ + scope?: SpecSurfaceScope; +} + +/** + * Why a record's `added`/`removed` arrays cannot be published as they stand, or + * `null` when they can. + * + * The one refusable shape is a non-empty export diff with no + * {@link SpecSurfaceScopeSchema}: the arrays then sit under a MAJOR-keyed + * `from` → `to` record carrying no statement of the range they really cover, so + * a consumer reads one release's slice as the whole major-boundary delta. This + * is a producer-side and publish-side assertion on purpose — see + * {@link SpecSurfaceScopeSchema} for why {@link SpecChangesSchema} does not + * refuse it. + */ +export function surfaceScopeProblem(record: SpecChanges): string | null { + const entries = record.added.length + record.removed.length; + if (entries === 0 || record.surfaceScope) return null; + return ( + `the ${record.from} → ${record.to} record carries ${record.added.length} added and ` + + `${record.removed.length} removed export(s) with no \`surfaceScope\`. Those arrays come from a ` + + 'ONE-RELEASE api-surface diff, so without the version pair they read as the whole ' + + `${record.from} → ${record.to} delta — which they are not.` + ); } /** @@ -188,6 +267,10 @@ export function composeSpecChanges( converted, migrated, removed: surfaceDiff.removed ?? [], + // Spread, never `scope: undefined`: a record with no export diff must carry + // no key at all, so the committed registry-only projection and every + // `perMajor` record serialise exactly as they did before this field existed. + ...(surfaceDiff.scope ? { surfaceScope: surfaceDiff.scope } : {}), }; } diff --git a/scripts/check-release-spec-changes.mjs b/scripts/check-release-spec-changes.mjs index 530185453f..cbd1aae7f5 100755 --- a/scripts/check-release-spec-changes.mjs +++ b/scripts/check-release-spec-changes.mjs @@ -42,6 +42,20 @@ * plus the one command that regenerates the section. A release is held only by * a failure whose remedy is printed with it. * + * ## Both export claims in the artifact, not just the section's + * + * `aggregate.added`/`removed` are filled by the same one-release api-surface + * diff, under a record keyed by protocol MAJOR — and until #18978 this gate did + * not look at them at all. Published unlabelled, one minor's slice reads as the + * whole major-boundary delta: the `@objectstack/spec@17.4.0` Release asset + * carried 225 added / 51 removed, every entry `since: 17`, beside + * `perMajor[16 → 17].added: 0`. So the aggregate's claim is recomputed from the + * same two tarballs and must carry a `surfaceScope` naming the version pair it + * really spans. An aggregate that claims NOTHING (empty arrays, no scope — the + * committed registry-only projection) is left alone: this gate refuses wrong + * claims, and turning "must not lie" into "must speak" is a publish requirement + * rather than a refusal. + * * ## The one thing it deliberately does NOT require * * A previous tarball that ships no `api-surface` snapshot (before protocol 15) @@ -49,7 +63,9 @@ * the section loudly in that case rather than emitting an empty one, and this * gate derives the same condition from the same artifacts and accepts the * absence — but it REFUSES a section that is present when it could not have - * been computed, which is the shape that would lie. + * been computed, which is the shape that would lie. The aggregate half needs + * strictly less (a snapshot and a version, never the previous manifest), so it + * is still checked on the shape where no section is owed. */ import fs from 'node:fs'; @@ -148,26 +164,133 @@ function listNames(names) { * tarballs, in BOTH directions — a section that omits a real removal and one * that invents a removal are different defects and read differently. */ -function compareArray(label, claimed, actual, problems) { +function compareArray(path, claimed, actual, problems) { + const kind = path.slice(path.indexOf('.') + 1); const claimedSet = new Set(claimed); const actualSet = new Set(actual); const invented = [...claimedSet].filter((n) => !actualSet.has(n)).sort(); const missed = [...actualSet].filter((n) => !claimedSet.has(n)).sort(); if (invented.length > 0) { problems.push( - `release.${label}: ${invented.length} export(s) the section CLAIMS but the two tarballs do not show as ${label}:`, + `${path}: ${invented.length} export(s) the artifact CLAIMS but the two tarballs do not show as ${kind}:`, ...listNames(invented), ); } if (missed.length > 0) { problems.push( - `release.${label}: ${missed.length} export(s) the two tarballs show as ${label} and the section OMITS:`, + `${path}: ${missed.length} export(s) the two tarballs show as ${kind} and the artifact OMITS:`, ...listNames(missed), ); } return invented.length === 0 && missed.length === 0; } +/** + * The aggregate record's own export-surface claim, checked against the same two + * tarballs — the half `release.*` was gated for and this one was not (#18978). + * + * `aggregate.added`/`removed` are filled by a ONE-RELEASE api-surface diff while + * the record is keyed by protocol MAJOR (`from: 10, to: 17`). Published + * unlabelled, one minor's slice reads as the whole major-boundary delta — and it + * did: the `@objectstack/spec@17.4.0` Release asset carried 225 added / 51 + * removed, every entry `since: 17`, beside `perMajor[16 → 17].added: 0`. So two + * things are refused here, in both directions: arrays that disagree with the two + * tarballs, and arrays that carry no `surfaceScope` naming the version pair they + * really span. + * + * ⚠️ An aggregate diff is computable from strictly less than a release section: + * it needs the previous tarball's export snapshot and version, and NOT its + * `spec-changes.json`. So this runs on the pre-#2897 shape too, where the + * release section is legitimately absent. + */ +/** One line naming what the aggregate record claims about the export surface. */ +function aggregateSummary(aggregate) { + const added = (aggregate?.added ?? []).length; + const removed = (aggregate?.removed ?? []).length; + const scope = aggregate?.surfaceScope; + if (added + removed === 0 && !scope) { + return 'aggregate claims no export diff (registry-only projection).'; + } + return ( + `aggregate export diff ${scope ? `${scope.fromVersion} → ${scope.toVersion}` : '(UNSCOPED)'} ` + + `verified: ${added} added, ${removed} removed.` + ); +} + +function verifyAggregateSurface(ctx, problems) { + const { aggregate, previousSurface, previousVersion, publishedSurface, publishedVersion } = ctx; + if (!aggregate) { + problems.push( + `the artifact's ${MANIFEST} has no aggregate record — ADR-0087 D4 requires it, and its export ` + + 'claim cannot be checked.', + ); + return false; + } + const claimedAdded = (aggregate.added ?? []).map((e) => e.surface); + const claimedRemoved = (aggregate.removed ?? []).map((e) => e.surface); + const scope = aggregate.surfaceScope; + const claims = claimedAdded.length + claimedRemoved.length; + + // ⛔ Deliberately NOT checked: an aggregate that makes no export claim at all. + // Empty arrays with no `surfaceScope` is the committed registry-only + // projection — honest, because it claims nothing — so requiring the published + // artifact to FILL them would be a new publish requirement rather than a + // refusal of a wrong claim, and that call is not this gate's to make. What is + // refused below is a claim that is unlabelled, mislabelled or untrue. + if (claims === 0 && !scope) return true; + + // Nothing to diff against ⇒ nothing may be claimed. Same call as the release + // section's: a claim that could not have been derived is the shape that lies. + if (!previousSurface || !previousVersion || !publishedSurface) { + const missing = [ + previousSurface ? null : 'the previous tarball ships no api-surface snapshot', + previousVersion ? null : 'the previous tarball ships no readable package.json', + publishedSurface ? null : 'the artifact about to publish ships no api-surface snapshot', + ].filter(Boolean); + if (claims > 0 || scope) { + problems.push( + `aggregate: the record claims ${claimedAdded.length} added / ${claimedRemoved.length} removed ` + + `export(s)${scope ? ' and a surfaceScope' : ''}, but ${missing.join(' and ')} — so no export ` + + 'diff could have been computed. A claim that could not be derived is exactly what this gate refuses.', + ); + return false; + } + return true; + } + + let ok = true; + if (claims > 0 && !scope) { + ok = false; + problems.push( + `aggregate.surfaceScope is absent while aggregate.added/removed carry ${claims} export(s). Those ` + + `arrays are a ONE-RELEASE diff, so under the ${aggregate.from} → ${aggregate.to} record they read ` + + `as the whole major-boundary delta. Expected { fromVersion: ${JSON.stringify(previousVersion)}, ` + + `toVersion: ${JSON.stringify(publishedVersion)} }.`, + ); + } + if (scope) { + if (scope.fromVersion !== previousVersion) { + ok = false; + problems.push( + `aggregate.surfaceScope.fromVersion is ${JSON.stringify(scope.fromVersion)} but the previous tarball ` + + `is ${JSON.stringify(previousVersion)} — the export diff was taken against a different release.`, + ); + } + if (scope.toVersion !== publishedVersion) { + ok = false; + problems.push( + `aggregate.surfaceScope.toVersion is ${JSON.stringify(scope.toVersion)} but this artifact is ` + + `${JSON.stringify(publishedVersion)} — the scope describes a release this tarball is not.`, + ); + } + } + const actualAdded = [...publishedSurface].filter((n) => !previousSurface.has(n)); + const actualRemoved = [...previousSurface].filter((n) => !publishedSurface.has(n)); + ok = compareArray('aggregate.added', claimedAdded, actualAdded, problems) && ok; + ok = compareArray('aggregate.removed', claimedRemoved, actualRemoved, problems) && ok; + return ok; +} + /** * The whole verdict for one release pair. * @@ -204,6 +327,18 @@ export function verifyRelease(previousDir, publishedDir) { const computable = Boolean(previousSurface && previousIds && previousVersion); const section = publishedManifest.release; + // Read once, used by both halves: the release section needs a previous + // `spec-changes.json` and the aggregate's export claim does not, so the two + // are checked against the same snapshots but gated on different inputs. + const publishedSurface = readSurface(publishedDir); + const aggregateCtx = { + aggregate: publishedManifest.aggregate, + previousSurface, + previousVersion, + publishedSurface, + publishedVersion, + }; + if (!computable) { // The one legitimate absence. Naming which input is missing keeps "we could // not compute it" distinguishable from "nothing changed". @@ -213,15 +348,18 @@ export function verifyRelease(previousDir, publishedDir) { previousVersion ? null : 'a readable package.json', ].filter(Boolean); if (section) { - return { - ok: false, - problems: [ - `the artifact carries a release section, but the previous tarball ships ${missing.join(' and ')} — ` + - 'so no delta could have been computed from it. A section that could not be derived is exactly the ' + - 'wrong-data case this gate exists to refuse.', - ], - summary: null, - }; + problems.push( + `the artifact carries a release section, but the previous tarball ships ${missing.join(' and ')} — ` + + 'so no delta could have been computed from it. A section that could not be derived is exactly the ' + + 'wrong-data case this gate exists to refuse.', + ); + } + // The aggregate's export claim survives an absent previous `spec-changes.json`, + // so it is still checked on the shape where no release section is owed. + const aggregateOk = verifyAggregateSurface(aggregateCtx, problems); + if (section || !aggregateOk) { + problems.push(REGENERATE_HINT); + return { ok: false, problems, summary: null }; } return { ok: true, @@ -263,7 +401,6 @@ export function verifyRelease(previousDir, publishedDir) { ); } - const publishedSurface = readSurface(publishedDir); if (!publishedSurface) { return { ok: false, @@ -280,8 +417,11 @@ export function verifyRelease(previousDir, publishedDir) { const actualRemoved = [...previousSurface].filter((n) => !publishedSurface.has(n)); const claimedAdded = (section.added ?? []).map((e) => e.surface); const claimedRemoved = (section.removed ?? []).map((e) => e.surface); - ok = compareArray('added', claimedAdded, actualAdded, problems) && ok; - ok = compareArray('removed', claimedRemoved, actualRemoved, problems) && ok; + ok = compareArray('release.added', claimedAdded, actualAdded, problems) && ok; + ok = compareArray('release.removed', claimedRemoved, actualRemoved, problems) && ok; + + // The aggregate record's own export claim, against the same two snapshots. + ok = verifyAggregateSurface(aggregateCtx, problems) && ok; // The registry half: entries NEW in this release are the ids the published // projection carries and the previous one did not. @@ -294,14 +434,14 @@ export function verifyRelease(previousDir, publishedDir) { const priorMigrations = new Set(previousIds.migrationIds); ok = compareArray( - 'converted', + 'release.converted', (section.converted ?? []).map((c) => c.conversionId), publishedIds.conversionIds.filter((id) => !priorConversions.has(id)), problems, ) && ok; ok = compareArray( - 'migrated', + 'release.migrated', (section.migrated ?? []).map((m) => m.migrationId), publishedIds.migrationIds.filter((id) => !priorMigrations.has(id)), problems, @@ -316,7 +456,8 @@ export function verifyRelease(previousDir, publishedDir) { summary: ok ? `release ${section.fromVersion} → ${section.toVersion} verified against both tarballs: ` + `${claimedAdded.length} added, ${claimedRemoved.length} removed, ` + - `${(section.converted ?? []).length} converted, ${(section.migrated ?? []).length} migrated.` + `${(section.converted ?? []).length} converted, ${(section.migrated ?? []).length} migrated. ` + + aggregateSummary(publishedManifest.aggregate) : null, }; } @@ -339,8 +480,16 @@ const SELF_TEST_BATTERIES = Object.freeze({ 'R9 — the published artifact ships no spec-changes.json → RED': 1, 'R10 — the published artifact ships no api-surface → RED': 1, 'R11 — an empty export snapshot is not a silent pass → RED': 1, + 'an unscoped EMPTY aggregate is the registry-only projection → GREEN': 1, + 'a scoped aggregate matching both tarballs → GREEN': 1, + 'R12 — a filled aggregate export diff with NO surfaceScope → RED': 1, + 'R13 — aggregate.surfaceScope.fromVersion naming another release → RED': 1, + 'R14 — aggregate.surfaceScope.toVersion disagreeing with the artifact → RED': 1, + 'R15 — an export the aggregate invents in added → RED, naming it': 1, + 'R16 — a real removal the scoped aggregate omits → RED, naming it': 1, + 'R17 — an aggregate export claim the previous tarball could not produce → RED': 1, }); -const SELF_TEST_BATTERY_FLOOR = 15; +const SELF_TEST_BATTERY_FLOOR = 23; function writeTree(root, files) { for (const [rel, content] of Object.entries(files)) { @@ -374,7 +523,7 @@ function selfTest() { }; const shard = (entry, exports) => ({ description: 'test shard', entry, exports }); - const manifest = (extra = {}) => ({ + const manifest = (extra = {}, aggregateExtra = {}) => ({ protocolVersion: '17.0.0', supportFloor: 10, aggregate: { @@ -384,10 +533,18 @@ function selfTest() { converted: [{ surface: 's', to: 't', conversionId: 'conv-old', toMajor: 17 }], migrated: [{ surface: 's', replacement: 'r', migrationId: 'mig-old', toMajor: 17, rationale: 'why' }], removed: [], + ...aggregateExtra, }, perMajor: [], ...extra, }); + /** The aggregate export claim that IS true of PREV → NEXT below. */ + const aggregateSurface = (extra = {}) => ({ + added: [{ surface: './ai: NewThing (const)', since: 17 }], + removed: [{ surface: './ai: OldThing (const)', removedIn: 17 }], + surfaceScope: { fromVersion: '17.3.0', toVersion: '17.4.0' }, + ...extra, + }); const release = (extra = {}) => ({ fromVersion: '17.3.0', toVersion: '17.4.0', @@ -403,10 +560,10 @@ function selfTest() { 'api-surface/ai.json': shard('./ai', ['Kept (const)', 'OldThing (const)']), 'spec-changes.json': manifest(), }; - const NEXT = (releaseSection = release(), extra = {}) => ({ + const NEXT = (releaseSection = release(), extra = {}, aggregateExtra = {}) => ({ 'package.json': { name: '@objectstack/spec', version: '17.4.0' }, 'api-surface/ai.json': shard('./ai', ['Kept (const)', 'NewThing (const)']), - 'spec-changes.json': manifest(releaseSection === null ? {} : { release: releaseSection }), + 'spec-changes.json': manifest(releaseSection === null ? {} : { release: releaseSection }, aggregateExtra), ...extra, }); @@ -584,6 +741,83 @@ function selfTest() { './ai: Kept (const)', ); + // ── The aggregate record's own export claim (#18978) ────────────────── + // The preserved-truth control comes FIRST: every battery above runs against + // an aggregate with empty, unscoped arrays, so if this half refused that + // shape they would all have gone red and the roster would read as a rewrite + // of the gate rather than an addition to it. + check( + 'an unscoped EMPTY aggregate is the registry-only projection → GREEN', + { prev: PREV, next: NEXT() }, + true, + 'aggregate claims no export diff', + ); + + check( + 'a scoped aggregate matching both tarballs → GREEN', + { prev: PREV, next: NEXT(release(), {}, aggregateSurface()) }, + true, + 'aggregate export diff 17.3.0 → 17.4.0 verified: 1 added, 1 removed', + ); + + check( + 'R12 — a filled aggregate export diff with NO surfaceScope → RED', + { prev: PREV, next: NEXT(release(), {}, aggregateSurface({ surfaceScope: undefined })) }, + false, + 'aggregate.surfaceScope is absent', + ); + + check( + 'R13 — aggregate.surfaceScope.fromVersion naming another release → RED', + { + prev: PREV, + next: NEXT(release(), {}, aggregateSurface({ surfaceScope: { fromVersion: '17.2.0', toVersion: '17.4.0' } })), + }, + false, + 'taken against a different release', + ); + + check( + 'R14 — aggregate.surfaceScope.toVersion disagreeing with the artifact → RED', + { + prev: PREV, + next: NEXT(release(), {}, aggregateSurface({ surfaceScope: { fromVersion: '17.3.0', toVersion: '17.9.0' } })), + }, + false, + 'describes a release this tarball is not', + ); + + check( + 'R15 — an export the aggregate invents in added → RED, naming it', + { + prev: PREV, + next: NEXT( + release(), + {}, + aggregateSurface({ added: [{ surface: './ai: Phantom (const)', since: 17 }] }), + ), + }, + false, + './ai: Phantom (const)', + ); + + check( + 'R16 — a real removal the scoped aggregate omits → RED, naming it', + { prev: PREV, next: NEXT(release(), {}, aggregateSurface({ removed: [] })) }, + false, + './ai: OldThing (const)', + ); + + check( + 'R17 — an aggregate export claim the previous tarball could not produce → RED', + { + prev: { 'package.json': { name: '@objectstack/spec', version: '17.3.0' }, 'spec-changes.json': manifest() }, + next: NEXT(null, {}, aggregateSurface()), + }, + false, + 'no export diff could have been computed', + ); + // ── Floor: what ran must be what is declared ────────────────────────── const floorFailure = (message) => { console.error(`✗ self-test floor: ${message}`); @@ -667,7 +901,10 @@ function main() { const verdict = verifyRelease(previous, published); if (!verdict.ok) { - console.error('✗ the per-release section of spec-changes.json disagrees with the two tarballs (ADR-0087 D4).'); + console.error( + "✗ spec-changes.json's export claims disagree with the two tarballs (ADR-0087 D4) — the per-release " + + 'section, the aggregate record, or both. Each line below names which.', + ); console.error(' A wrong change file is worse than none — a consumer gates its upgrade on this data.\n'); for (const line of verdict.problems) console.error(` ${line}`); process.exit(1);