From 12a435b0f1b4bd6997d2a9ac87d41d9de0d4f6b2 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 04:04:33 +0000 Subject: [PATCH 1/2] =?UTF-8?q?fix(metadata):=20capabilities.write=20binds?= =?UTF-8?q?=20save()=20too=20=E2=80=94=20a=20writable=20datasource=20loade?= =?UTF-8?q?r=20must=20implement=20both=20halves=20(#5654)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `registerLoader()`'s #5276 gate required `delete()` for a `datasource:` loader declaring `capabilities.write: true`, and stopped there. `register()`'s persistence loop read `loader.save &&` FIRST, so the same declaration with no `save()` method was silently skipped — no warn, no error — while the registry write, list-cache invalidation, `created`/`updated` event and watcher notification all proceeded, telling the caller the write had landed. The item read back for the life of the process and was gone at the next restart. The gate (renamed `assertWritableLoaderContract`) now requires both methods and rejects with one message naming which are missing, the consequence, and both repairs. `register()`'s `save` short-circuit survives as annotated defensive code whose unreachability is guaranteed by `registerLoader()` being the sole writer of the loader map — mirroring `unregister()`'s `delete` guard. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01V7WetGmnfoXNn8cLieKKmx --- .changeset/metadata-loader-save-contract.md | 50 +++ ...ata-manager-loader-delete-contract.test.ts | 8 + ...adata-manager-loader-save-contract.test.ts | 350 ++++++++++++++++++ packages/metadata/src/metadata-manager.ts | 138 +++++-- 4 files changed, 508 insertions(+), 38 deletions(-) create mode 100644 .changeset/metadata-loader-save-contract.md create mode 100644 packages/metadata/src/metadata-manager-loader-save-contract.test.ts diff --git a/.changeset/metadata-loader-save-contract.md b/.changeset/metadata-loader-save-contract.md new file mode 100644 index 0000000000..3ad8c16c9c --- /dev/null +++ b/.changeset/metadata-loader-save-contract.md @@ -0,0 +1,50 @@ +--- +"@objectstack/metadata": patch +--- + +fix(metadata): `capabilities.write` now also binds `save()` — a writable datasource loader must implement both halves of the write (#5654) + +#5276 (shipped in v17.0.0-rc) made `capabilities.write` binding on `delete()`: +a loader declaring `protocol: 'datasource:'` with `capabilities.write: true` +and no `delete()` is refused at registration, because `unregister()` used to +skip it silently and announce the deletion anyway. The gate stopped there, so +**one declaration was binding at one end of an item's life and decorative at +the other**. + +`MetadataManager.register()` had the identical hole one direction over. Its +persistence loop read `loader.save &&` first, so a `datasource:` loader +declaring `capabilities.write: true` **without** a `save()` method was +**silently skipped** — no warn, no error. `register()` then wrote the in-memory +registry, invalidated the list cache, announced `created`/`updated` and notified +watchers, so the caller (Studio/Setup, REST PUT, the CLI, a package publish) was +told the write succeeded. The item read back correctly for the life of the +process and was **gone at the next restart**, with nothing to retry it — a +durability degradation that leaves the system looking entirely healthy. + +`registerLoader()`'s gate (renamed `assertWritableLoaderContract`) now requires +**both** `save()` and `delete()` for that combination, and rejects with one +message naming which method is missing, the consequence, and both repairs. +`registerLoader()` is the sole writer of the loader map — the constructor's +`config.loaders` funnel through it — so the combination can no longer reach the +runtime and lose a write there. The `save` short-circuit inside `register()` +survives as defensive code whose unreachability is now guaranteed by +construction, exactly like `unregister()`'s. + +**Does this affect you?** Only if you register a custom metadata loader that +declares `protocol: 'datasource:'` with `capabilities.write: true`. If it does +and has no `save()`, registration now throws where it previously succeeded and +quietly discarded your writes. Two ways to fix it, both stated in the error: + +1. implement + `async save(type: string, name: string, data: any, options?: MetadataSaveOptions): Promise` + on the loader, persisting the item into its store (`DatabaseLoader` in this + package is the reference implementation); or +2. if the loader is genuinely read-only, declare `capabilities.write: false` — a + read-only `datasource:` loader registers without complaint and is never + written to in the first place. + +Loaders on the other protocols (`file:`, `memory:`, `http:`, `s3:`) are +unaffected: `MetadataManager` never persists to them at runtime, so they may +declare `capabilities.write` without a `save()`/`delete()` exactly as before. +The one `datasource:` loader shipped in this package, `DatabaseLoader`, has +always implemented both and is unchanged. diff --git a/packages/metadata/src/metadata-manager-loader-delete-contract.test.ts b/packages/metadata/src/metadata-manager-loader-delete-contract.test.ts index e409c3ff66..2e7d90afb3 100644 --- a/packages/metadata/src/metadata-manager-loader-delete-contract.test.ts +++ b/packages/metadata/src/metadata-manager-loader-delete-contract.test.ts @@ -35,6 +35,14 @@ * register without a `delete`, because the manager never writes to them; * 5. `DatabaseLoader`, the repo's only real `datasource:` loader, passes the * gate unchanged. + * + * [#5654] The gate this file pins has since been widened — it is + * `assertWritableLoaderContract` now, and `capabilities.write` requires `save()` + * as well, because `register()` had the identical silent skip one direction + * over. Everything below still holds verbatim: these loaders all implement + * `save`, so `delete` is the only thing missing and the message is unchanged. + * The `save` half is pinned next door in + * `metadata-manager-loader-save-contract.test.ts`. */ import { describe, it, expect, vi, beforeEach } from 'vitest'; diff --git a/packages/metadata/src/metadata-manager-loader-save-contract.test.ts b/packages/metadata/src/metadata-manager-loader-save-contract.test.ts new file mode 100644 index 0000000000..e609397756 --- /dev/null +++ b/packages/metadata/src/metadata-manager-loader-save-contract.test.ts @@ -0,0 +1,350 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #5654 — the `save` half of the same gate #5276 built for `delete`. + * + * `register()` persists into every writable `datasource:` loader, and it read + * `loader.save &&` FIRST: a loader declaring `protocol: 'datasource:'` with + * `capabilities.write: true` but implementing no `save()` was **silently + * skipped** — no warn, no error, nothing. `register()` then wrote the in-memory + * registry, invalidated the list cache, announced `created`/`updated` and + * notified watchers, so the caller was told the write succeeded. The item read + * back correctly for the life of the process and was gone at the next restart, + * with nothing to retry it. Durability degradation of the exact shape AGENTS.md + * → "Degradation log levels" says must not even be a `warn`. + * + * #5276 (PR #5652) closed the mirror-image hole on `delete` and deliberately + * stopped there, which left ONE declaration binding at one end of an item's + * life and decorative at the other: + * + * - delete side: declared = enforced (registration throws); + * - save side: declared ≠ enforced (registration passes, the write is lost). + * + * The fix is the same cure one direction over — `registerLoader()`'s gate, + * renamed `assertWritableLoaderContract`, now requires BOTH methods and says so + * in one message. `register()`'s `save` short-circuit survives as defensive + * code whose unreachability is guaranteed by construction, exactly like + * `unregister()`'s. + * + * What these tests pin: + * 1. the rejection of a `save`-less writable datasource loader, on both entry + * points (constructor config and the direct `registerLoader()` call), + * including that nothing is half-registered; + * 2. the message is actionable — it names the loader, the consequence, and + * both repairs — and is the ONE text `buildWritableLoaderMissingMethodsMessage` + * builds, quoted rather than paraphrased; + * 3. a loader missing BOTH methods is rejected once, naming both; + * 4. the positive case is untouched and really durable: a loader with both + * methods registers, `register()` reaches its store, and the row is still + * there for a fresh manager over the same store — the "restart" the silent + * skip used to lose; + * 5. the gate's scope is exactly the combination `register()` acts on — a + * read-only `datasource:` loader and every non-`datasource:` protocol + * register without a `save`, because the manager never persists to them; + * 6. `DatabaseLoader`, the repo's only real `datasource:` loader, passes the + * widened gate unchanged. + */ + +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import type { + MetadataLoadResult, + MetadataLoaderContract, + MetadataSaveResult, + MetadataStats, +} from '@objectstack/spec/system'; +import type { IDataDriver } from '@objectstack/spec/contracts'; +import { MetadataManager, buildWritableLoaderMissingMethodsMessage } from './metadata-manager.js'; +import { DatabaseLoader } from './loaders/database-loader.js'; +import type { MetadataLoader } from './loaders/loader-interface.js'; + +const logger = vi.hoisted(() => ({ + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + debug: vi.fn(), +})); + +vi.mock('@objectstack/core', () => ({ + createLogger: () => logger, +})); + +type Protocol = MetadataLoaderContract['protocol']; + +/** + * A loader whose contract is dictated per test and whose `save`/`delete` are + * present or absent on demand — the axes the gate reads, and nothing else. The + * backing store is shared by reference so a second manager can be pointed at it + * to play "restart". + */ +function makeLoader(opts: { + name: string; + protocol: Protocol; + write: boolean; + withSave: boolean; + withDelete: boolean; + store?: Map; +}): MetadataLoader & { + store: Map; + saveCalls: Array<[string, string]>; + deleteCalls: Array<[string, string]>; +} { + const saveCalls: Array<[string, string]> = []; + const deleteCalls: Array<[string, string]> = []; + const store = opts.store ?? new Map(); + const key = (type: string, name: string) => `${type}/${name}`; + + const loader: MetadataLoader & { + store: Map; + saveCalls: Array<[string, string]>; + deleteCalls: Array<[string, string]>; + } = { + contract: { + name: opts.name, + protocol: opts.protocol, + capabilities: { read: true, write: opts.write, watch: false, list: true }, + }, + store, + saveCalls, + deleteCalls, + async load(type: string, name: string): Promise { + const data = store.get(key(type, name)); + return data === undefined ? { data: null } : { data }; + }, + async loadMany(): Promise { + return Array.from(store.values()) as T[]; + }, + async exists(type: string, name: string): Promise { + return store.has(key(type, name)); + }, + async stat(): Promise { + return null; + }, + async list(): Promise { + return []; + }, + }; + + if (opts.withSave) { + loader.save = async (type: string, name: string, data: unknown): Promise => { + saveCalls.push([type, name]); + store.set(key(type, name), data); + return { success: true }; + }; + } + + if (opts.withDelete) { + loader.delete = async (type: string, name: string): Promise => { + deleteCalls.push([type, name]); + store.delete(key(type, name)); + }; + } + + return loader; +} + +/** Read the manager's private loader map — the thing registration writes. */ +const registeredLoaderNames = (mgr: MetadataManager): string[] => + Array.from((mgr as unknown as { loaders: Map }).loaders.keys()); + +const messageOf = (run: () => unknown): string => { + try { + run(); + } catch (error) { + return error instanceof Error ? error.message : String(error); + } + throw new Error('expected the call to throw, but it returned normally'); +}; + +beforeEach(() => { + logger.info.mockClear(); + logger.warn.mockClear(); + logger.error.mockClear(); + logger.debug.mockClear(); +}); + +describe('a `datasource:` loader that declares `capabilities.write` MUST implement `save()`', () => { + const unsavable = () => + makeLoader({ + name: 'half_writable_store', + protocol: 'datasource:', + write: true, + withSave: false, + withDelete: true, + }); + + it('registerLoader() throws rather than accepting a loader it can never persist into', () => { + const mgr = new MetadataManager({ formats: ['json'], loaders: [] }); + + expect(() => mgr.registerLoader(unsavable())).toThrow(/half_writable_store/); + }); + + it('…and nothing is half-registered — the rejected loader is not in the map', () => { + const mgr = new MetadataManager({ formats: ['json'], loaders: [] }); + + expect(() => mgr.registerLoader(unsavable())).toThrow(); + expect(registeredLoaderNames(mgr)).not.toContain('half_writable_store'); + }); + + it('the constructor rejects it too — `config.loaders` is not a back door', () => { + expect( + () => new MetadataManager({ formats: ['json'], loaders: [unsavable()] }), + ).toThrow(/half_writable_store/); + }); + + it('the message names the loader, the consequence, and BOTH repairs', () => { + const mgr = new MetadataManager({ formats: ['json'], loaders: [] }); + const message = messageOf(() => mgr.registerLoader(unsavable())); + + // Which loader, and what it declared. + expect(message).toContain('half_writable_store'); + expect(message).toContain("protocol: 'datasource:'"); + expect(message).toContain('capabilities.write: true'); + expect(message).toContain('implements no `save()` method'); + // The consequence: the write is announced, never lands, and vanishes at + // restart while everything keeps looking healthy. + expect(message).toContain('`register()`'); + expect(message).toContain('`created`/`updated`'); + expect(message).toContain('gone at the next restart'); + // Repair A — implement it. Repair B — stop declaring the capability. + expect(message).toContain('save(type: string, name: string, data: any'); + expect(message).toContain('capabilities.write: false'); + // …and it is the one text the builder owns, not a paraphrase of it. + expect(message).toBe(buildWritableLoaderMissingMethodsMessage('half_writable_store', ['save'])); + }); + + it('a loader missing BOTH methods is rejected once, naming both', () => { + const mgr = new MetadataManager({ formats: ['json'], loaders: [] }); + const neither = makeLoader({ + name: 'inert_store', + protocol: 'datasource:', + write: true, + withSave: false, + withDelete: false, + }); + + const message = messageOf(() => mgr.registerLoader(neither)); + + expect(message).toContain('implements neither a `save()` nor a `delete()` method'); + expect(message).toContain('save(type: string, name: string, data: any'); + expect(message).toContain('delete(type: string, name: string)'); + expect(message).toBe( + buildWritableLoaderMissingMethodsMessage('inert_store', ['save', 'delete']), + ); + }); + + it('the `delete`-only rejection #5276 shipped still reads exactly as it did', () => { + const mgr = new MetadataManager({ formats: ['json'], loaders: [] }); + const undeletable = makeLoader({ + name: 'append_only_store', + protocol: 'datasource:', + write: true, + withSave: true, + withDelete: false, + }); + + const message = messageOf(() => mgr.registerLoader(undeletable)); + + expect(message).toContain('implements no `delete()` method'); + expect(message).toContain('`unregister()`'); + expect(message).toContain('`deleted`'); + expect(message).not.toContain('`save()`'); + }); +}); + +describe('the positive case is untouched — and the write is really durable', () => { + it('a loader with both methods registers, is written to, and survives a restart', async () => { + const store = new Map(); + const writable = makeLoader({ + name: 'writable_store', + protocol: 'datasource:', + write: true, + withSave: true, + withDelete: true, + store, + }); + + const mgr = new MetadataManager({ formats: ['json'], loaders: [writable] }); + expect(registeredLoaderNames(mgr)).toContain('writable_store'); + + await mgr.register('object', 'account', { name: 'account' }); + expect(writable.saveCalls).toEqual([['object', 'account']]); + + // The restart the silent skip used to lose: a fresh manager holding an + // empty registry, reading the same store. + const afterRestart = new MetadataManager({ + formats: ['json'], + loaders: [ + makeLoader({ + name: 'writable_store', + protocol: 'datasource:', + write: true, + withSave: true, + withDelete: true, + store, + }), + ], + }); + expect(await afterRestart.get('object', 'account')).toEqual({ name: 'account' }); + + // …and the other half of the contract still works through the same gate. + await mgr.unregister('object', 'account'); + expect(writable.deleteCalls).toEqual([['object', 'account']]); + expect(store.has('object/account')).toBe(false); + }); +}); + +describe('the gate covers exactly the combination `register()` acts on', () => { + it('a read-only `datasource:` loader needs no `save` — nothing ever writes to it', async () => { + const readOnly = makeLoader({ + name: 'reporting_replica', + protocol: 'datasource:', + write: false, + withSave: false, + withDelete: false, + }); + + const mgr = new MetadataManager({ formats: ['json'], loaders: [readOnly] }); + expect(registeredLoaderNames(mgr)).toContain('reporting_replica'); + + await expect(mgr.register('object', 'account', { name: 'account' })).resolves.toBeUndefined(); + expect(readOnly.saveCalls).toEqual([]); + }); + + it.each(['file:', 'memory:', 'http:', 's3:'])( + 'a `%s` loader may declare write without a `save` — the manager never persists there', + (protocol) => { + const loader = makeLoader({ + name: `loader_${protocol.replace(':', '')}`, + protocol, + write: true, + withSave: false, + withDelete: false, + }); + + const mgr = new MetadataManager({ formats: ['json'], loaders: [] }); + expect(() => mgr.registerLoader(loader)).not.toThrow(); + expect(registeredLoaderNames(mgr)).toContain(loader.contract.name); + }, + ); +}); + +describe('regression — the real `datasource:` loader is unaffected', () => { + /** + * `DatabaseLoader` declares `datasource:` + `capabilities.write` and has + * implemented both `save()` and `delete()` all along; the widened gate must + * be a no-op for it. The driver is a stub because registration touches no + * storage — construction and the contract are the whole surface here. + */ + it('DatabaseLoader registers under the widened gate', () => { + const loader = new DatabaseLoader({ driver: {} as IDataDriver }); + + expect(loader.contract.protocol).toBe('datasource:'); + expect(loader.contract.capabilities.write).toBe(true); + expect(typeof loader.save).toBe('function'); + expect(typeof loader.delete).toBe('function'); + + const mgr = new MetadataManager({ formats: ['json'], loaders: [] }); + expect(() => mgr.registerLoader(loader)).not.toThrow(); + expect(registeredLoaderNames(mgr)).toContain('database'); + }); +}); diff --git a/packages/metadata/src/metadata-manager.ts b/packages/metadata/src/metadata-manager.ts index 3a82a53b8b..7e70e8b2f7 100644 --- a/packages/metadata/src/metadata-manager.ts +++ b/packages/metadata/src/metadata-manager.ts @@ -79,27 +79,73 @@ import type { ApiEndpointMatch } from '@objectstack/spec/contracts'; export type WatchCallback = (event: MetadataWatchEvent) => void | Promise; /** - * [#5276] The registration gate's message for a loader that declares it can be - * written to but cannot be deleted from. + * [#5654] The two methods `capabilities.write` promises on a `datasource:` + * loader, in the order an item meets them: written by + * {@link MetadataManager.register}, taken back out by + * {@link MetadataManager.unregister}. Both are `?` on {@link MetadataLoader} + * because the other protocols legitimately have neither. + */ +export type WritableLoaderMethod = 'save' | 'delete'; + +const WRITABLE_LOADER_METHODS: readonly WritableLoaderMethod[] = ['save', 'delete']; + +/** How the message names each method, and what the author has to write. */ +const WRITABLE_LOADER_METHOD_SIGNATURE: Record = { + save: 'save(type: string, name: string, data: any, options?: MetadataSaveOptions): Promise', + delete: 'delete(type: string, name: string): Promise', +}; + +/** + * [#5276, #5654] The registration gate's message for a loader that declares it + * can be written to but cannot actually carry out one (or both) halves of that + * write. * * Built here rather than inline so the gate and its tests quote one text, in the * shape AGENTS.md → "Degradation log levels" asks of a loud failure: the * **consequence** (concretely, and that the system keeps looking healthy) and * the **fix** (both ways out, so the author does not have to guess which one * their loader wants). + * + * `missing` is the subset of {@link WRITABLE_LOADER_METHODS} the loader does not + * implement; each one contributes its own consequence sentence, because the two + * failures are durability failures in opposite directions — a `save`-less loader + * loses the row that was never written, a `delete`-less one keeps the row that + * was supposed to go. */ -export function buildWritableLoaderMissingDeleteMessage(loaderName: string): string { +export function buildWritableLoaderMissingMethodsMessage( + loaderName: string, + missing: readonly WritableLoaderMethod[], +): string { + const missingPhrase = + missing.length === 2 + ? 'implements neither a `save()` nor a `delete()` method' + : `implements no \`${missing[0]}()\` method`; + + const consequences = missing.map((method) => + method === 'save' + ? 'Registered as-is, every write would be a silent lie: `register()` skips a loader that cannot save, then ' + + 'writes the in-memory registry, invalidates the list cache, announces a `created`/`updated` event and ' + + 'notifies watchers, so the caller (Studio/Setup, REST PUT, the CLI, a package publish) is told the write ' + + `succeeded while nothing ever reaches \`${loaderName}\` — the item reads back correctly for the life of ` + + 'this process and is gone at the next restart, with nothing to retry it. ' + : 'Registered as-is, every deletion would be a silent lie: `unregister()` skips a loader that cannot delete, ' + + 'then drops the registry entry, invalidates the list cache and announces a `deleted` event, so the caller ' + + '(Studio/Setup, REST DELETE, the CLI, a package teardown) is told the delete succeeded while the row stays ' + + `in \`${loaderName}\` and is read straight back out by the very next \`list()\`/\`get()\` — across ` + + 'restarts, with nothing to retry it. ', + ); + + const repair = missing + .map((method) => `\`${WRITABLE_LOADER_METHOD_SIGNATURE[method]}\``) + .join(' and '); + return ( `[MetadataManager] Refusing to register metadata loader \`${loaderName}\`: it declares ` + - "`protocol: 'datasource:'` with `capabilities.write: true` but implements no `delete()` method. " + + `\`protocol: 'datasource:'\` with \`capabilities.write: true\` but ${missingPhrase}. ` + 'A write-capable datasource loader is written to AND deleted from — `register()` persists every item into it, ' + 'and `unregister()` has to take those rows back out again. ' + - 'Registered as-is, every deletion would be a silent lie: `unregister()` skips a loader that cannot delete, then ' + - 'drops the registry entry, invalidates the list cache and announces a `deleted` event, so the caller ' + - '(Studio/Setup, REST DELETE, the CLI, a package teardown) is told the delete succeeded while the row stays in ' + - `\`${loaderName}\` and is read straight back out by the very next \`list()\`/\`get()\` — across restarts, with ` + - 'nothing to retry it. ' + - `Fix: either implement \`delete(type: string, name: string): Promise\` on \`${loaderName}\` ` + + consequences.join('') + + `Fix: either implement ${repair} on \`${loaderName}\` ` + '(`DatabaseLoader` in this package is the reference implementation), or, if the loader is genuinely read-only, ' + "declare `capabilities.write: false` — a read-only `datasource:` loader registers without complaint and is " + 'never written to in the first place.' @@ -107,30 +153,36 @@ export function buildWritableLoaderMissingDeleteMessage(loaderName: string): str } /** - * [#5276] Registration gate: a `datasource:` loader that declares - * `capabilities.write` MUST implement `delete()`. + * [#5276, #5654] Registration gate: a `datasource:` loader that declares + * `capabilities.write` MUST implement **both** `save()` and `delete()`. * - * `capabilities.write` used to mean two different things at the two ends of an - * item's life — "persist into me" to {@link MetadataManager.register}, and - * nothing at all to {@link MetadataManager.unregister}, which duck-typed - * `delete` at the call site and **silently skipped** a loader that had none - * before announcing the deletion anyway. That is the declared ≠ enforced shape - * (Prime Directive #10), and the cure it prescribes is to enforce the - * declaration, not to tolerate the gap: the loader is rejected at registration, - * where the author is standing, instead of losing a row at delete time in a - * deployment nobody is watching. + * `capabilities.write` used to mean three different things at the three places + * it is read — "persist into me" to {@link MetadataManager.register}, which + * duck-typed `save` and **silently skipped** a loader that had none; nothing at + * all to {@link MetadataManager.unregister}, which did the same with `delete`; + * and, on the contract, a flat promise that the store can be written. That is + * the declared ≠ enforced shape (Prime Directive #10), and the cure it + * prescribes is to enforce the declaration, not to tolerate the gap: the loader + * is rejected at registration, where the author is standing, instead of losing a + * row at write or delete time in a deployment nobody is watching. * - * Scope is deliberately exactly the combination `unregister()` acts on. Other - * protocols (`file:`, `memory:`, `http:`, `s3:`) are never written to by the - * manager at runtime — `register()` filters on `datasource:` too — so they have - * no deletion of their own to take back and are not gated. A `datasource:` - * loader with `capabilities.write: false` is likewise untouched by both paths. + * #5276 closed the `delete` half; #5654 closed the `save` half, which was the + * same shape one direction over — and leaving it open meant one declaration was + * binding at one end of an item's life and decorative at the other. + * + * Scope is deliberately exactly the combination `register()`/`unregister()` act + * on. Other protocols (`file:`, `memory:`, `http:`, `s3:`) are never written to + * by the manager at runtime — both loops filter on `datasource:` too — so they + * have neither a write nor a deletion of their own to lose and are not gated. A + * `datasource:` loader with `capabilities.write: false` is likewise untouched by + * both paths. */ -function assertWritableLoaderCanDelete(loader: MetadataLoader): void { +function assertWritableLoaderContract(loader: MetadataLoader): void { const { name, protocol, capabilities } = loader.contract; if (protocol !== 'datasource:' || capabilities.write !== true) return; - if (typeof loader.delete === 'function') return; - throw new Error(buildWritableLoaderMissingDeleteMessage(name)); + const missing = WRITABLE_LOADER_METHODS.filter((method) => typeof loader[method] !== 'function'); + if (missing.length === 0) return; + throw new Error(buildWritableLoaderMissingMethodsMessage(name, missing)); } /** @@ -600,14 +652,15 @@ export class MetadataManager implements IMetadataService { /** * Register a new metadata loader (data source) * - * [#5276] Rejects — loudly, before the loader is stored — a `datasource:` - * loader that declares `capabilities.write` without implementing `delete()`. - * This is the **only** way into `this.loaders` (the constructor's - * `config.loaders` come through here too), which is what lets every later - * delete-capability guard be defensive rather than load-bearing. + * [#5276, #5654] Rejects — loudly, before the loader is stored — a + * `datasource:` loader that declares `capabilities.write` without + * implementing `save()` **and** `delete()`. This is the **only** way into + * `this.loaders` (the constructor's `config.loaders` come through here too), + * which is what lets every later write-capability guard be defensive rather + * than load-bearing. */ registerLoader(loader: MetadataLoader) { - assertWritableLoaderCanDelete(loader); + assertWritableLoaderContract(loader); this.loaders.set(loader.contract.name, loader); this.logger.info(`Registered metadata loader: ${loader.contract.name} (${loader.contract.protocol})`); } @@ -662,9 +715,18 @@ export class MetadataManager implements IMetadataService { // FilesystemLoader is read-only at runtime — writing to it can crash in // read-only environments (e.g. serverless, containerized deployments). for (const loader of this.loaders.values()) { - if (loader.save && loader.contract.protocol === 'datasource:' && loader.contract.capabilities.write) { - await loader.save(type, name, data); - } + if (loader.contract.protocol !== 'datasource:' || !loader.contract.capabilities.write) continue; + // [#5654] Defensive only — unreachable for a registered loader, and read + // in this order on purpose: the protocol/capability test is the policy, + // the method test is the type narrowing. This exact combination + // (`datasource:` + `capabilities.write`, no `save`) is rejected by + // `registerLoader()`, the sole writer of `this.loaders`, so reaching this + // `continue` would mean a loader entered the map without passing the + // gate. Kept because the alternative is a TypeError on the line below, + // and because `save?` stays optional on the interface for the protocols + // the gate does not cover. Mirrors the same guard in `unregister()`. + if (typeof loader.save !== 'function') continue; + await loader.save(type, name, data); } // Publish metadata.{type}.created / .updated event to realtime service. From 630ff4080f760a8bb32cfbb956e90c0cea9d95d1 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 04:26:21 +0000 Subject: [PATCH 2/2] docs(metadata): state the writable-loader contract on `save?` too, where the author is standing (#5654) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `delete?` carried #5276's note that it is mandatory for a `datasource:` loader declaring `capabilities.write`; `save?` carried nothing, so after #5654 widened the gate the asymmetry pointed the wrong way — the interface documented one half of a contract that binds both. `save?` now states the same thing in its own terms (silent skip in `register()`, the write announced but never landed, gone at the next restart), and `delete?`'s note points at the widened gate by name (`assertWritableLoaderContract`, #5276 + #5654) instead of describing a delete-only one. Comments only — no signature change, no runtime effect. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01V7WetGmnfoXNn8cLieKKmx --- .../metadata/src/loaders/loader-interface.ts | 32 +++++++++++++++++-- 1 file changed, 29 insertions(+), 3 deletions(-) diff --git a/packages/metadata/src/loaders/loader-interface.ts b/packages/metadata/src/loaders/loader-interface.ts index cbfa2592e0..470bd8505c 100644 --- a/packages/metadata/src/loaders/loader-interface.ts +++ b/packages/metadata/src/loaders/loader-interface.ts @@ -73,7 +73,27 @@ export interface MetadataLoader { list(type: string): Promise; /** - * Save metadata item + * Save metadata item into this loader's store. + * + * [#5654] Optional on the interface, **mandatory for a `datasource:` loader + * that declares `capabilities.write`** — `MetadataManager.registerLoader()` + * refuses to register such a loader when this method is missing, so the + * combination "declared writable, cannot persist" never reaches the runtime. + * + * The reason it is enforced at registration rather than tolerated at the write + * site: `MetadataManager.register()` persists into every writable + * `datasource:` loader, and it used to read `loader.save &&` first — a loader + * that declares it can be written to but has no `save()` made every write a + * silent lie. `register()` would skip it, then write the in-memory registry, + * invalidate the list cache, announce a `created`/`updated` event and notify + * watchers, so the caller is told the write succeeded; the item reads back + * correctly for the life of the process and is **gone at the next restart**, + * with nothing to retry it. + * + * Loaders on the other protocols (`file:`, `memory:`, `http:`, `s3:`) are not + * gated: `MetadataManager` never persists to them at runtime — `register()` + * filters on `datasource:` — so a missing `save()` there loses nothing. + * * @param type The metadata type * @param name The item name * @param data The data to save @@ -89,8 +109,8 @@ export interface MetadataLoader { /** * Delete a metadata item from this loader's store. * - * [#5276] Optional on the interface, **mandatory for a `datasource:` loader - * that declares `capabilities.write`** — `MetadataManager.registerLoader()` + * [#5276, #5654] Optional on the interface, **mandatory for a `datasource:` + * loader that declares `capabilities.write`** — `MetadataManager.registerLoader()` * refuses to register such a loader when this method is missing, so the * combination "declared writable, cannot delete" never reaches the runtime. * @@ -105,6 +125,12 @@ export interface MetadataLoader { * therefore means *both* directions of the write, on both ends of the item's * life — declared = enforced. * + * One gate covers both halves: `assertWritableLoaderContract` in + * `metadata-manager.ts` requires `save()` **and** `delete()` for this + * combination and names whichever is missing. #5276 built it for `delete`; + * #5654 widened it to `save`, which had the identical silent skip in + * `register()` — see the note on `save?` above. + * * Loaders on the other protocols (`file:`, `memory:`, `http:`, `s3:`) are not * gated: `MetadataManager` never writes to them at runtime, so it never has a * deletion of its own to take back.