From 98ea42fb62a3d6487cd1246d2831024aa0c479b5 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 15 Aug 2026 06:32:32 +0000 Subject: [PATCH 1/2] fix(metadata-protocol): declare saveMetaItem's missing-item refusal (400 INVALID_REQUEST) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `saveMetaItem`'s opening guard was the one refusal in the method carrying neither `code` nor `status`, so `clientFacingFailureText` withheld its sentence and `handleRouteError` — with no status to read — served it as `500 INTERNAL_ERROR`. Measured end to end against a live server before changing anything: the guard IS reachable from the wire as an authoring refusal. `PUT /api/v1/meta/:type/:name` unwraps the `{ item }` / `{ metadata }` envelope shapes before calling, so `{"item": null}` and `{"metadata": null}` arrive as `item: null` and land on it — both answered 500 before this change. A missing, empty or literal-`null` body does NOT reach it: the route folds those to `{}`, which is truthy, and the per-type Zod parse refuses them with 422 INVALID_METADATA. `INVALID_REQUEST`/400, matching `rollbackMetaItem`'s structurally identical opening guard, rather than the `INVALID_METADATA`/422 the sibling refusals use — those describe a body that exists and failed the per-type parse, and each carries structured `issues`, which a missing body has none of. Both codes are already registered to this package in the ADR-0112 ledger; no new code is minted. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XeQRiAa7vYRVX5Fog7Zby8 --- .../protocol.save-meta-missing-item.test.ts | 148 ++++++++++++++++++ packages/metadata-protocol/src/protocol.ts | 41 ++++- packages/objectql/src/protocol-meta.test.ts | 17 +- 3 files changed, 201 insertions(+), 5 deletions(-) create mode 100644 packages/metadata-protocol/src/protocol.save-meta-missing-item.test.ts diff --git a/packages/metadata-protocol/src/protocol.save-meta-missing-item.test.ts b/packages/metadata-protocol/src/protocol.save-meta-missing-item.test.ts new file mode 100644 index 0000000000..cab1e13243 --- /dev/null +++ b/packages/metadata-protocol/src/protocol.save-meta-missing-item.test.ts @@ -0,0 +1,148 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #8818 — `saveMetaItem`'s opening guard was the ONE refusal in the method + * that declared no ADR-0112 envelope, so consumers applying the rule withheld + * its sentence and the REST boundary served it as a server fault. + * + * ## What was measured, end to end, before the fix + * + * The card was filed as an observation with an explicitly unverified premise + * ("read from source, no victim measured"), so the premise was probed against + * a real server (`pnpm dev:crm -- --fresh`, `PUT /api/v1/meta/view/:name`, + * authenticated as the seeded platform admin) before a line was changed: + * + * | request body | reaches this guard? | answered (before) | + * |---|---|---| + * | *(no body at all)* | no | `422 INVALID_METADATA` | + * | `null` | no | `422 INVALID_METADATA` | + * | `{}` | no | `422 INVALID_METADATA` | + * | `{"item": null}` | **YES** | **`500 INTERNAL_ERROR`** | + * | `{"metadata": null}` | **YES** | **`500 INTERNAL_ERROR`** | + * + * So the honest outcomes the card itself listed — "unreachable, leave it + * alone" and "a programming-error guard, not an authoring refusal" — are both + * FALSE, and the reason is precise: `PUT /meta/:type/:name` unwraps the + * `{ item }` / `{ metadata }` envelope shapes before calling, so an + * explicitly-null envelope arrives as `item: null` and lands here, while a + * missing/empty/`null` BODY folds to `{}` (truthy) and is refused downstream + * by the per-type Zod parse. The reachable population is caller-authored JSON + * only; every internal caller passes a concrete document. + * + * The cost was also LARGER than filed. The card predicted the sentence would + * degrade to a consumer's generic fallback; what the wire actually did was + * answer **500 `INTERNAL_ERROR`** — because `handleRouteError` has no status + * to read and defaults to a server fault. A 500 does not merely tell the + * author less, it tells them something FALSE: that the server broke and the + * request is worth retrying, when it can never succeed unchanged. + * + * ## Why the `clientFacingFailureText` assertion is the point of this file + * + * A test asserting only `code`/`status` on the thrown error would pass without + * demonstrating the thing the card is about. `declaresClientRefusal` is a + * POSITIVE list keyed on a 4xx `status`, so the assertion that matters is that + * the sentence now SURVIVES the rule rather than being replaced by the + * caller's fallback — and that assertion is only evidence next to its control: + * the same helper, handed the bare `Error` this guard used to throw, still + * withholds. Both directions are asserted below; drop the control and the pin + * would stay green against a `clientFacingFailureText` that had stopped + * withholding anything at all. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { ObjectStackProtocolImplementation, clientFacingFailureText } from './protocol.js'; + +/** + * A protocol over an engine whose every verb is a tripwire: this guard is the + * FIRST statement of `saveMetaItem`, so a refusal that touched the engine at + * all would mean the check had moved behind something with side effects. + */ +function makeProtocol() { + const findOne = vi.fn(async () => null); + const find = vi.fn(async () => [] as unknown[]); + const engine = { + registry: { getObject: () => undefined }, + findOne, + find, + }; + return { p: new ObjectStackProtocolImplementation(engine as any), findOne, find }; +} + +/** The refusal a rejected request produced, plus proof the engine was untouched. */ +async function refusalFor(request: Record) { + const { p, findOne, find } = makeProtocol(); + let answered: unknown; + try { + answered = await (p as any).saveMetaItem(request); + } catch (e) { + expect(findOne, 'the engine was reached before the refusal').not.toHaveBeenCalled(); + expect(find, 'the engine was reached before the refusal').not.toHaveBeenCalled(); + return e as Error & { code?: string; status?: number }; + } + throw new Error( + `${JSON.stringify(request)} was ACCEPTED (answered ${JSON.stringify(answered)}) instead of refused`, + ); +} + +describe('#8818 — a save with no item declares the ADR-0112 envelope', () => { + // The three spellings that reach this guard. `item: null` is the one the + // REST route actually produces (from `{"item": null}` / `{"metadata": + // null}`); the other two are the same condition reached through the SDK + // and the protocol interface, where `item` is an optional parameter. + it.each<[string, Record]>([ + ['an explicitly null item (what the wire produces)', { type: 'app', name: 'a', item: null }], + ['an absent item', { type: 'app', name: 'a' }], + ['an explicitly undefined item', { type: 'app', name: 'a', item: undefined }], + ])('refuses %s with 400 INVALID_REQUEST', async (_label, request) => { + const err = await refusalFor(request); + + // The envelope, not merely the throw. A bare `toThrow()` here would be + // permanently green: the UNFIXED guard threw too — that was the whole + // defect — so the throw carries no information and only the + // declaration does. + expect(err.code).toBe('INVALID_REQUEST'); + expect(err.status).toBe(400); + }); + + it('names the remedy in the message, so the refusal is self-correcting', async () => { + const err = await refusalFor({ type: 'view', name: 'my_view', item: null }); + + expect(err.message).toContain("requires an 'item' body"); + // The address the author got wrong is echoed back to them. + expect(err.message).toContain('view/my_view'); + }); + + it('does not refuse a request that DOES carry an item', async () => { + // What a refusal is cheapest to break. Green in both directions on its + // own — it is a guard, not evidence — but an over-broad guard (say, one + // testing `'item' in request`) would turn it red. + const { p } = makeProtocol(); + await expect( + (p as any).saveMetaItem({ type: 'app', name: 'a', item: { name: 'a' } }), + ).rejects.not.toMatchObject({ code: 'INVALID_REQUEST' }); + }); +}); + +describe('#8818 — the refusal now SURVIVES the ADR-0112 disclosure rule', () => { + it('is quoted back to the author instead of degrading to the fallback', async () => { + const err = await refusalFor({ type: 'app', name: 'test_app', item: null }); + + const shown = clientFacingFailureText(err, 'save failed'); + + // THE POINT OF THE CARD: a consumer applying the #8086/#8136 rule now + // shows the producer's own sentence. + expect(shown).not.toBe('save failed'); + expect(shown).toBe(err.message); + expect(shown).toContain("requires an 'item' body"); + }); + + it('CONTROL — the bare Error this guard used to throw is still withheld', () => { + // Byte-for-byte what `origin/main` threw at this site. Without this + // control the assertion above would also pass against a + // `clientFacingFailureText` that had stopped withholding ANYTHING, + // which is the regression that would silently re-open #8086. + const undeclared = new Error('Item data is required'); + + expect(clientFacingFailureText(undeclared, 'save failed')).toBe('save failed'); + }); +}); diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index 444f64d992..627e9d3b1e 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -11368,8 +11368,47 @@ export class ObjectStackProtocolImplementation implements } async saveMetaItem(request: { type: string, name: string, item?: any, organizationId?: string, parentVersion?: string | null, actor?: string, force?: boolean, mode?: 'draft' | 'publish', packageId?: string | null, source?: string }) { + // [#8818] The ADR-0112 envelope this refusal always owed. Every OTHER + // refusal in this method declares `code` AND `status` + // (`NOT_OVERRIDABLE`/403, `NOT_CREATABLE`/403, `ITEM_LOCKED`/403, + // `OBJECT_OVERLAY_PACKAGE_MISMATCH`/422, the org-scope and + // destructive-change refusals, the parent-version conflict/409); this + // one declared neither, and an undeclared throw is withheld by + // {@link clientFacingFailureText} (a positive list keyed on 4xx + // `status`) and rendered `500 INTERNAL_ERROR` by `handleRouteError` — + // a SERVER FAULT for what is purely the caller's mistake, telling the + // author less than the producer knew and inviting a pointless retry. + // + // MEASURED reachable from the wire — this is an AUTHORING refusal, not + // a programming-error guard. `PUT /api/v1/meta/:type/:name` unwraps the + // `{ item }` / `{ metadata }` envelope shapes before calling here, so + // `{"item": null}` and `{"metadata": null}` arrive as `item: null` and + // land exactly here (measured end to end against a live server: both + // answered `500 INTERNAL_ERROR` before this change). ⚠️ A missing, + // empty or literal-`null` BODY does NOT reach this guard: the route + // folds it to `{}`, which is truthy, and the per-type Zod parse below + // refuses it with `422 INVALID_METADATA` — so this guard's whole + // reachable population is the explicitly-null envelope. + // + // `INVALID_REQUEST`/400 rather than the `INVALID_METADATA`/422 the + // sibling refusals use, and NEITHER mints a code: both are already + // registered to this package in the ADR-0112 ledger (D3). The 422 + // sites all describe a body that EXISTS and failed the per-type parse, + // and every one carries structured `issues`; here there is no body to + // validate and no issues to report, so a 422 would misdescribe the + // failure and break that convention. The structural twin is + // {@link rollbackMetaItem}'s own opening guard — same class, same + // position, a malformed REQUEST ENVELOPE rather than an off-spec + // document — which is `[invalid_request]`/400. if (!request.item) { - throw new Error('Item data is required'); + const err: any = new Error( + `[invalid_request] saveMetaItem requires an 'item' body for '${request.type}/${request.name}'. ` + + `Send the metadata document as the request body, or wrap it as {"item": {...}} / {"metadata": {...}}. ` + + `An explicitly null item is refused rather than persisted as an empty document.`, + ); + err.code = 'INVALID_REQUEST'; + err.status = 400; + throw err; } // #4432 — CANONICAL TYPE KEY. See {@link canonicalMetaType}. request = canonicalizeMetaRequestType(request); diff --git a/packages/objectql/src/protocol-meta.test.ts b/packages/objectql/src/protocol-meta.test.ts index 55e4f804de..525ecc0947 100644 --- a/packages/objectql/src/protocol-meta.test.ts +++ b/packages/objectql/src/protocol-meta.test.ts @@ -225,10 +225,19 @@ describe('ObjectStackProtocolImplementation - Metadata Persistence', () => { }); describe('saveMetaItem', () => { - it('should throw when item data is missing', async () => { - await expect( - protocol.saveMetaItem({ type: 'app', name: 'test_app' }) - ).rejects.toThrow('Item data is required'); + // [#8818] WAS `rejects.toThrow('Item data is required')` — a bare + // message match that stayed green while the refusal declared no + // ADR-0112 envelope at all, so `clientFacingFailureText` withheld the + // sentence and the REST boundary served `500 INTERNAL_ERROR`. The + // envelope is the contract; assert it. + it('refuses a missing item with the ADR-0112 envelope (400 INVALID_REQUEST)', async () => { + const err: any = await protocol.saveMetaItem({ type: 'app', name: 'test_app' }) + .then(() => { throw new Error('saveMetaItem ACCEPTED a request with no item'); }) + .catch((e: any) => e); + + expect(err.code).toBe('INVALID_REQUEST'); + expect(err.status).toBe(400); + expect(err.message).toContain("requires an 'item' body"); }); it('writes the saved body through to the SchemaRegistry for non-object types (#4521)', async () => { From 2c3492204061c9f265fcb81e56023700a52d889b Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 15 Aug 2026 06:36:40 +0000 Subject: [PATCH 2/2] chore(changeset): saveMetaItem missing-item refusal declares 400 INVALID_REQUEST Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XeQRiAa7vYRVX5Fog7Zby8 --- .changeset/hungry-donkeys-shout.md | 9 +++++++++ 1 file changed, 9 insertions(+) create mode 100644 .changeset/hungry-donkeys-shout.md diff --git a/.changeset/hungry-donkeys-shout.md b/.changeset/hungry-donkeys-shout.md new file mode 100644 index 0000000000..a8445b7e18 --- /dev/null +++ b/.changeset/hungry-donkeys-shout.md @@ -0,0 +1,9 @@ +--- +"@objectstack/metadata-protocol": patch +--- + +Declare `saveMetaItem`'s missing-item refusal as a real ADR-0112 envelope: `400` / `INVALID_REQUEST`, was an undeclared throw served as `500 INTERNAL_ERROR`. + +`PUT /api/v1/meta/:type/:name` unwraps the `{ item }` / `{ metadata }` envelope shapes before calling the protocol, so a caller sending `{"item": null}` or `{"metadata": null}` reached a guard that declared neither `code` nor `status` — the only refusal in the method that did not. With no status to read, the REST boundary defaulted to a server fault, so an authoring mistake was reported as `500 INTERNAL_ERROR` and the guard's own sentence was withheld by the ADR-0112 disclosure rule and replaced with a generic fallback. Callers now receive `400` with the refusal quoted and the remedy named. + +Unchanged: a missing, empty or literal-`null` request body never reached this guard and still answers `422 INVALID_METADATA` from the per-type schema parse. No new error code is introduced — `INVALID_REQUEST` is already registered to this package in the ADR-0112 ledger, and is what the structurally identical opening guard in `rollbackMetaItem` already uses.