diff --git a/.changeset/olive-crabs-shave.md b/.changeset/olive-crabs-shave.md new file mode 100644 index 0000000000..6d6476f633 --- /dev/null +++ b/.changeset/olive-crabs-shave.md @@ -0,0 +1,20 @@ +--- +'@object-ui/app-shell': patch +--- + +Every envelope reader on the package surfaces now renders a producer-marked +`error.userMessage`, and the Studio package list opens the failure body at all. + +`fetchPackages` — the `/api/v1/packages` read behind the Studio switcher, the writability +courtesy gate, the namespace lookup and the builder landing page — answered a refusal with +`HTTP ` and never opened the body, so `message`, `code` and `userMessage` were +discarded together: a 403 whose envelope said `Reading packages requires the studio.access +or setup.access capability.` reached the author as four characters. `apiJson` on the +package admin page read `error.message` and never the mark, and rendered no code. +`duplicatePackage` read `error.message` alone. + +All three now ask one shared rule (`readEnvelopeFailureText`): a producer-marked +`userMessage` outranks the diagnostic `message` at any status — presence of the field is +the producer's marking, and a consumer that sees it renders it verbatim — with `error.code` +appended to whichever prose won. An unmarked refusal is unchanged and still renders its +diagnostic; a body carrying no prose still falls back to each reader's own status text. diff --git a/packages/app-shell/src/utils/apiErrorEnvelope.test.ts b/packages/app-shell/src/utils/apiErrorEnvelope.test.ts new file mode 100644 index 0000000000..9210e2ef5f --- /dev/null +++ b/packages/app-shell/src/utils/apiErrorEnvelope.test.ts @@ -0,0 +1,161 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * objectui#7959 — the RULE, pinned where it is defined. + * + * Three readers on the package surfaces held three independent implementations + * of "read the ADR-0112 failure envelope", and they had already drifted into + * three different answers for the same body: + * + * - `fetchPackages` (`views/studio-design/packages-io.ts`) never opened the + * body at all — `message`, `code` and `userMessage` were dropped together; + * - `apiJson` (`views/metadata-admin/PackagesPage.tsx`) read `error.message` + * and never `error.userMessage`, and appended no code; + * - `fetchFullPackage` (`StudioDesignSurface.tsx`) read `message` + `code`, + * and was taught the mark separately in objectui#7938. + * + * This file pins the extracted rule itself, so a fourth consumer inherits a + * pinned rule rather than a fourth reading of the envelope. The CALL SITES are + * pinned separately — `packages-io.envelopeUserMessage.test.ts` and + * `PackagesPage.envelopeUserMessage.test.tsx` — because "the rule is right" and + * "this reader actually asks it" are two different claims, and only the second + * one is what the author sees. + */ + +import { describe, expect, it } from 'vitest'; +import { readEnvelopeFailureText } from './apiErrorEnvelope'; + +/** The generic sentence the 5xx prose withhold substitutes (`INTERNAL_ERROR_MESSAGE`). */ +const GENERIC = 'Internal server error'; +/** A producer's marked text: what `userMessage` carries, written for the person. */ +const MARKED = 'Publishing is temporarily unavailable. Nothing was changed.'; +/** The card's measured sample — a refusal that names the capability to grant. */ +const CAPABILITY = 'Reading packages requires the `studio.access` or `setup.access` capability.'; + +/** A body exactly as `sendError` writes it: `{ success: false, error: { … } }`. */ +const envelope = (error: Record) => ({ success: false, error }); + +describe('readEnvelopeFailureText — the four combinations (#7959)', () => { + /** + * ⭐ The overwhelmingly common case, and the one this pin exists to protect: + * "prefer `userMessage`" must never be implemented as "read `userMessage` + * INSTEAD", which would blank every unmarked refusal the platform serves + * today. Byte-identical to the pre-extraction diagnostic read. + */ + it('§1 `message` only — the unmarked refusal renders the diagnostic', () => { + expect(readEnvelopeFailureText(envelope({ code: 'FORBIDDEN', message: CAPABILITY }))).toBe( + `${CAPABILITY} (FORBIDDEN)`, + ); + }); + + it('§2 `userMessage` only — the mark is the only prose on the body', () => { + expect(readEnvelopeFailureText(envelope({ code: 'SERVICE_UNAVAILABLE', userMessage: MARKED }))).toBe( + `${MARKED} (SERVICE_UNAVAILABLE)`, + ); + }); + + /** + * The live 5xx case: the door substituted the generic sentence into + * `message` and the mark rode through untouched. The mark DISPLACES the + * diagnostic — it is not appended to it. + */ + it('§3 both — the mark wins and the generic sentence does not also appear', () => { + const shown = readEnvelopeFailureText( + envelope({ code: 'INTERNAL_ERROR', message: GENERIC, userMessage: MARKED }), + ); + expect(shown).toBe(`${MARKED} (INTERNAL_ERROR)`); + expect(shown).not.toContain(GENERIC); + }); + + it('§4 neither — no prose means no answer, and the caller states its own fallback', () => { + expect(readEnvelopeFailureText(envelope({ code: 'SERVICE_UNAVAILABLE' }))).toBeNull(); + }); +}); + +describe('readEnvelopeFailureText — how `code` composes', () => { + it('is appended to whichever prose won — the mark', () => { + expect(readEnvelopeFailureText(envelope({ code: 'CONFLICT', message: GENERIC, userMessage: MARKED }))).toBe( + `${MARKED} (CONFLICT)`, + ); + }); + + it('is appended to whichever prose won — the diagnostic', () => { + expect(readEnvelopeFailureText(envelope({ code: 'CONFLICT', message: CAPABILITY }))).toBe( + `${CAPABILITY} (CONFLICT)`, + ); + }); + + it('a marked body with NO code renders the bare sentence', () => { + expect(readEnvelopeFailureText(envelope({ message: GENERIC, userMessage: MARKED }))).toBe(MARKED); + }); + + /** + * ⛔ A code NEVER rescues a prose-less body. A machine code is not a + * sentence to show a person, so this stays `null` and the caller falls back + * to naming the status. + */ + it('a code with no prose is still no prose', () => { + expect(readEnvelopeFailureText(envelope({ code: 'SERVICE_UNAVAILABLE' }))).toBeNull(); + }); + + it('a non-string code is not a code — the prose renders bare', () => { + expect(readEnvelopeFailureText(envelope({ code: 500, message: CAPABILITY }))).toBe(CAPABILITY); + }); +}); + +describe('readEnvelopeFailureText — what is not a mark', () => { + it('a non-string `userMessage` falls through to the diagnostic', () => { + expect(readEnvelopeFailureText(envelope({ code: 'INTERNAL_ERROR', message: GENERIC, userMessage: 42 }))).toBe( + `${GENERIC} (INTERNAL_ERROR)`, + ); + }); + + it('an empty-string `userMessage` falls through to the diagnostic', () => { + // The producer's `declaredUserMessage` already applies the non-empty-string + // rule, so an empty mark should never ship — this reader does not depend on + // the producer having applied it. + expect(readEnvelopeFailureText(envelope({ code: 'INTERNAL_ERROR', message: GENERIC, userMessage: '' }))).toBe( + `${GENERIC} (INTERNAL_ERROR)`, + ); + }); + + it('a non-string `message` is not prose either', () => { + expect(readEnvelopeFailureText(envelope({ code: 'INTERNAL_ERROR', message: { nested: 'x' } }))).toBeNull(); + }); + + it('an empty-string mark AND an empty-string diagnostic leave nothing to show', () => { + expect(readEnvelopeFailureText(envelope({ code: 'INTERNAL_ERROR', message: '', userMessage: '' }))).toBeNull(); + }); +}); + +describe('readEnvelopeFailureText — bodies that are not this envelope', () => { + it.each([ + ['null (an unparseable body)', null], + ['undefined', undefined], + ['no `error` key at all', { success: false }], + ['a bare-string `error` (an older runtime shape — the caller keeps that rung)', { error: 'boom' }], + ['an array', [1, 2, 3]], + ['a string', 'boom'], + ['a number', 502], + ])('%s → null', (_label, payload) => { + expect(readEnvelopeFailureText(payload)).toBeNull(); + }); +}); + +describe('readEnvelopeFailureText — structurally cannot be scoped to a status band', () => { + /** + * ⛔ The producing door applies NO status condition to the marked channel — + * "a marked text is the producer's deliberate statement to the caller at any + * status" — so a consumer honouring the mark in one band only would + * re-create, on the reading end, the divergence that door refused to create + * on the writing end (ruled on objectui#7938). + * + * This asserts that as a property of the SIGNATURE rather than as a behaviour + * sampled at two statuses: the rule takes one parameter, the body, and the + * status is not among its inputs. A future "only in the 5xx band" variant + * cannot be written without changing the arity this line pins. + */ + it('takes the body and nothing else — the status is not an input to the rule', () => { + expect(readEnvelopeFailureText.length).toBe(1); + }); +}); diff --git a/packages/app-shell/src/utils/apiErrorEnvelope.ts b/packages/app-shell/src/utils/apiErrorEnvelope.ts new file mode 100644 index 0000000000..a5b6070e8f --- /dev/null +++ b/packages/app-shell/src/utils/apiErrorEnvelope.ts @@ -0,0 +1,116 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * ONE read of the platform's ADR-0112 failure envelope — the prose a person + * should be shown when a request was refused. + * + * ## The rule this implements, and whose rule it is + * + * The envelope writer (`sendError`, `@objectstack/types` `response-envelope.ts`) + * nests the refusal under `error`: + * + * res.status(status).json({ success: false, error: { code, message, ...extra } }); + * + * and `extra` is where the producer's declared channels ride. Two of the fields + * under `error` are prose, and they are NOT two spellings of one thing: + * + * - `message` — the DIAGNOSTIC. Always present, written for whoever is + * debugging. In the 5xx band the door substitutes the generic + * `Internal server error` into it (see below), so it is the text that can be + * withheld. + * - `userMessage` — the text a producer marked, AT THROW TIME, as addressed to + * the END USER (#9934). Its presence IS the marking, and the envelope + * writer's own words are the rule this module implements: "a consumer that + * sees the field renders it verbatim and keeps its generic substitution + * (#3821) for everything unmarked". + * + * So the mark OUTRANKS the diagnostic. That order is the contract's, not a + * preference — a reader that ignores `userMessage` is not degrading gracefully, + * it is discarding the one sentence a producer deliberately wrote for the + * person now reading the screen. + * + * ## Measured, not assumed: both doors that serve these routes emit the field + * + * `GET /api/v1/packages` and its lifecycle siblings are served by two doors, + * and each spreads the marked channel onto the wire beside `details` / + * `declaredCode`: + * + * - `sendThrownError` (`@objectstack/rest` `package-routes.ts`): + * `...(thrown.userMessage !== undefined ? { userMessage: thrown.userMessage } : {})` + * - `errorFromThrown` (`@objectstack/runtime` `http-dispatcher.ts`), which that + * same door's note calls "byte for byte the dispatcher twin's expression … + * which serves this same path and has emitted the channel since #9934". + * + * The framework pins the pair wire-side in `package-door-user-message.test.ts`. + * + * ## Why the 5xx band is where the loss became visible + * + * The producing door withholds the producer's PROSE above: + * + * const message = thrown.status >= 500 && looksLikeInternalErrorLeak(thrown.message) + * ? INTERNAL_ERROR_MESSAGE + * : thrown.message; + * + * The withhold rewrites a LOCAL `message` const, and `looksLikeInternalErrorLeak` + * is only ever handed `thrown.message` — so `userMessage` is never an input to + * it and rides through a sanitised 500 untouched. A reader that only knew about + * `message` therefore showed the author the GENERIC sentence on exactly the + * bodies that were carrying a specific one. Nothing invalid was displayed, + * which is what made the loss quiet. + * + * ⛔ NOT scoped to 5xx, deliberately. The producing door applies no status + * condition to this channel — "a marked text is the producer's deliberate + * statement to the caller at any status" — so a consumer that honoured the mark + * in one band only would re-create, on the READING end, precisely the + * divergence that door refused to create on the WRITING end. + * + * ⛔ Not a tolerant alias ladder either. These are two DECLARED fields with + * different meanings. An unmarked refusal carries no `userMessage` at all (the + * producer's `declaredUserMessage` already applied its non-empty-string rule), + * so the overwhelmingly common case falls straight through to `message` with + * byte-identical output. + * + * ## Why this returns `null` instead of a fallback + * + * The three readers that share this rule do NOT share a fallback: the + * `packages-io` readers say `HTTP ` and `PackagesPage`'s `apiJson` says + * `Request failed ()`, and `apiJson` additionally keeps two legacy rungs + * (a bare-string `error`, a top-level `message`) for the shapes older runtimes + * send. Folding a fallback in here would have forced a `fallback` parameter and + * a `legacyRungs` flag — three different things pressed into one signature, + * which is harder to read than the copies it replaces. So the shared part is + * exactly the part that IS shared: envelope in, the person's prose out, or + * `null` when this body carried no prose at all. Each caller keeps its own + * fallback, on its own line, where a reader can see it. + * + * @param payload The parsed response body, or `null` when it could not be + * parsed. Typed `unknown` because every caller obtains it differently + * (`res.json().catch(() => null)`, `res.text()` + `JSON.parse`) and none of + * them can promise a shape. + * @returns The prose to show, with `error.code` appended in parentheses when + * the envelope declared one; `null` when the body carried no prose, in which + * case the caller states its own fallback. A code NEVER rescues a + * prose-less body: a machine code alone is not a sentence to show a person. + */ +export function readEnvelopeFailureText(payload: unknown): string | null { + const error = (payload as { error?: unknown } | null | undefined)?.error; + // A bare-string `error` (an older runtime's shape) is not this envelope. It + // is left to the caller that still knows about it — see the note above. + if (!error || typeof error !== 'object') return null; + const { code, message, userMessage } = error as { + code?: unknown; + message?: unknown; + userMessage?: unknown; + }; + // A typed `string` check, not a truthiness one, and it is load-bearing twice: + // a non-string mark is not a mark (it is a producer bug, and falling through + // to the diagnostic is the honest answer), and an empty-string mark is not + // one either — `declaredUserMessage` already applies that rule producer-side, + // and this reader does not depend on the producer having applied it. + const marked = typeof userMessage === 'string' ? userMessage : ''; + const diagnostic = typeof message === 'string' ? message : ''; + const prose = marked || diagnostic; + if (!prose) return null; + const declaredCode = typeof code === 'string' ? code : ''; + return declaredCode ? `${prose} (${declaredCode})` : prose; +} diff --git a/packages/app-shell/src/views/metadata-admin/PackagesPage.envelopeUserMessage.test.tsx b/packages/app-shell/src/views/metadata-admin/PackagesPage.envelopeUserMessage.test.tsx new file mode 100644 index 0000000000..1940c9db52 --- /dev/null +++ b/packages/app-shell/src/views/metadata-admin/PackagesPage.envelopeUserMessage.test.tsx @@ -0,0 +1,180 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * objectui#7959 — `apiJson` reads the producer-marked `error.userMessage`. + * + * This page's shared reader had the ladder `error.message || error || message + * || 'Request failed (n)'`. It read the diagnostic and stopped: a + * producer-marked `error.userMessage` arrived on the wire and had nowhere to + * appear, and `error.code` was never rendered at all. It is the third + * independent implementation of "read the ADR-0112 failure envelope" on the + * package surfaces — the drift that motivated extracting one rule + * (`utils/apiErrorEnvelope.ts`, pinned in `apiErrorEnvelope.test.ts`). + * + * Driven through the PAGE rather than by calling `apiJson` directly, because + * `apiJson` is module-private and because what the card is about is what the + * person reads: the list load calls it, and the error banner renders + * `e.message` verbatim. + * + * ## ⚠️ Two rungs stay, and they are NOT the envelope + * + * A bare-string `error` and a top-level `message` are older runtimes' shapes, + * live for this page's lifecycle routes and for no other consumer of the rule. + * They stay HERE, below the shared read — folding them into the shared helper + * would hand every other consumer a tolerant dialect it never asked for. §6 + * pins that they still work. + */ + +import * as React from 'react'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { cleanup, render, screen, waitFor } from '@testing-library/react'; +import { MemoryRouter } from 'react-router-dom'; + +import { PackagesPage } from './PackagesPage'; + +/** The generic sentence the 5xx prose withhold substitutes (`INTERNAL_ERROR_MESSAGE`). */ +const GENERIC = 'Internal server error'; +/** A producer's marked text: what `userMessage` carries, written for the person. */ +const MARKED = 'Publishing is temporarily unavailable. Nothing was changed.'; +/** The card's measured sample — a refusal that names the capability to grant. */ +const CAPABILITY = 'Reading packages requires the `studio.access` or `setup.access` capability.'; + +/** + * `apiJson` reads the body as TEXT and `JSON.parse`s it (not `res.json()`), so + * the stub has to answer `text` — mirroring `PackageFormDialog.test.tsx`. + */ +function stubBody(status: number, body: unknown) { + vi.stubGlobal( + 'fetch', + vi.fn(async () => ({ + ok: status >= 200 && status < 300, + status, + text: async () => JSON.stringify(body), + })) as unknown as typeof fetch, + ); +} + +/** A failure exactly as `sendError` writes it. */ +const envelope = (error: Record) => ({ success: false, error }); + +/** What the page's error banner ends up showing the person. */ +async function bannerFor(status: number, body: unknown): Promise { + stubBody(status, body); + render( + + + , + ); + const banner = await screen.findByTestId('packages-load-error'); + await waitFor(() => expect(banner.textContent).toBeTruthy()); + return banner.textContent ?? ''; +} + +beforeEach(() => { + vi.restoreAllMocks(); +}); + +afterEach(() => { + cleanup(); + vi.unstubAllGlobals(); +}); + +describe('PackagesPage / apiJson — the four combinations (#7959)', () => { + /** + * ⭐ GREEN with the fix reverted, and load-bearing for exactly that reason. + * This reader already rendered `error.message`, so the unmarked refusal — the + * overwhelmingly common case — must come through byte for byte. It is the pin + * that stops "prefer `userMessage`" from being implemented as "read + * `userMessage` INSTEAD", which would have blanked every refusal this page + * serves today. + * + * Deliberately code-less: the code append is new behaviour and would make + * this case red on revert, costing the guard its role. §5 covers the code. + */ + it('§1 `message` only, no code — the unmarked refusal, byte for byte as before', async () => { + expect(await bannerFor(403, envelope({ message: CAPABILITY }))).toBe(CAPABILITY); + }); + + it('§2 `userMessage` only — the marked sentence instead of `Request failed (503)`', async () => { + const shown = await bannerFor(503, envelope({ code: 'SERVICE_UNAVAILABLE', userMessage: MARKED })); + expect(shown).toBe(`${MARKED} (SERVICE_UNAVAILABLE)`); + expect(shown).not.toContain('Request failed'); + }); + + it('§3 both — the mark displaces the generic substitution', async () => { + const shown = await bannerFor(500, envelope({ code: 'INTERNAL_ERROR', message: GENERIC, userMessage: MARKED })); + expect(shown).toBe(`${MARKED} (INTERNAL_ERROR)`); + expect(shown).not.toContain(GENERIC); + }); + + it('§4 neither — this page names the status its own way, unchanged', async () => { + expect(await bannerFor(503, envelope({ code: 'SERVICE_UNAVAILABLE' }))).toBe('Request failed (503)'); + }); +}); + +describe('PackagesPage / apiJson — how `code` composes', () => { + it('§5 the code is appended to whichever prose won — the diagnostic', async () => { + expect(await bannerFor(403, envelope({ code: 'FORBIDDEN', message: CAPABILITY }))).toBe( + `${CAPABILITY} (FORBIDDEN)`, + ); + }); + + it('§5 the code is appended to whichever prose won — the mark', async () => { + expect(await bannerFor(409, envelope({ code: 'RESOURCE_CONFLICT', message: GENERIC, userMessage: MARKED }))).toBe( + `${MARKED} (RESOURCE_CONFLICT)`, + ); + }); + + it('§5 a marked body with no code renders the bare sentence', async () => { + expect(await bannerFor(503, envelope({ message: GENERIC, userMessage: MARKED }))).toBe(MARKED); + }); + + it('§5 a non-string mark is not a mark — it falls through to the diagnostic', async () => { + expect(await bannerFor(500, envelope({ code: 'INTERNAL_ERROR', message: GENERIC, userMessage: 42 }))).toBe( + `${GENERIC} (INTERNAL_ERROR)`, + ); + }); + + it('§5 an empty-string mark is not a mark either', async () => { + expect(await bannerFor(500, envelope({ code: 'INTERNAL_ERROR', message: GENERIC, userMessage: '' }))).toBe( + `${GENERIC} (INTERNAL_ERROR)`, + ); + }); +}); + +describe('PackagesPage / apiJson — the legacy rungs below the envelope (GREEN with the fix reverted)', () => { + it('§6 a bare-string `error` still reaches the banner', async () => { + expect(await bannerFor(500, { success: false, error: 'metadata service unavailable' })).toBe( + 'metadata service unavailable', + ); + }); + + it('§6 a top-level `message` still reaches the banner', async () => { + expect(await bannerFor(500, { success: false, message: 'metadata service unavailable' })).toBe( + 'metadata service unavailable', + ); + }); + + it('§6 a body with no readable prose anywhere still names the status', async () => { + expect(await bannerFor(500, { success: false })).toBe('Request failed (500)'); + }); +}); + +/** + * ⚠️ Its own block, and NOT under the green-when-reverted heading above: this + * one asserts the mark, so it goes RED on revert like §2/§3/§5. The forward + * control caught it living under that heading, which would have read as a + * claim the run does not support. + */ +describe('PackagesPage / apiJson — the 200 that declares failure', () => { + /** + * ⭐ The failure trigger is `!res.ok || payload.success === false`, so a 200 + * that declares failure is a refusal too — and it is the arm a fix that + * reached only for `!res.ok` would silently leave behind. + */ + it('§7 a 200 that declares `success: false` is still a refusal, and now carries the mark', async () => { + expect(await bannerFor(200, envelope({ code: 'INTERNAL_ERROR', message: GENERIC, userMessage: MARKED }))).toBe( + `${MARKED} (INTERNAL_ERROR)`, + ); + }); +}); diff --git a/packages/app-shell/src/views/metadata-admin/PackagesPage.tsx b/packages/app-shell/src/views/metadata-admin/PackagesPage.tsx index 3bacf20b56..8205733132 100644 --- a/packages/app-shell/src/views/metadata-admin/PackagesPage.tsx +++ b/packages/app-shell/src/views/metadata-admin/PackagesPage.tsx @@ -63,6 +63,7 @@ import { import { useMetadataLocale, t, tFormat } from './i18n.js'; import { PackageFormDialog } from './PackageFormDialog.js'; import { errorCodeIs } from '@object-ui/types'; +import { readEnvelopeFailureText } from '../../utils/apiErrorEnvelope.js'; /* -------------------------------------------------------------------------- */ /* Types + API */ @@ -127,8 +128,20 @@ async function apiJson(path: string, init?: RequestInit): Promise { const text = await res.text(); const payload = text ? JSON.parse(text) : null; if (!res.ok || payload?.success === false) { + // The ADR-0112 envelope first, by the ONE shared rule — a producer-marked + // `error.userMessage` outranks the diagnostic `error.message`, and + // `error.code` rides along behind whichever won. This helper used to read + // `error.message` and stop, so a marked sentence arrived on the wire (both + // doors serving these package routes emit the channel) and had nowhere to + // appear. See {@link readEnvelopeFailureText}. + // + // The two rungs BELOW it stay, and stay here rather than moving into the + // shared reader: they are not the ADR-0112 envelope. A bare-string `error` + // and a top-level `message` are older runtimes' shapes, live for this page + // and not for the Studio readers, and folding them in would have handed + // every other consumer of the rule a tolerant dialect it never asked for. const msg = - payload?.error?.message || + readEnvelopeFailureText(payload) || payload?.error || payload?.message || `Request failed (${res.status})`; @@ -915,7 +928,10 @@ export function PackagesPage() { ) : error ? (
- {error} + {/* A stable handle for the load-failure pins (objectui#7959): the + words in here are the server's, so a test that located this + banner BY those words could not assert what is absent from it. */} + {error}
) : filtered.length === 0 ? (
diff --git a/packages/app-shell/src/views/studio-design/packages-io.envelopeUserMessage.test.ts b/packages/app-shell/src/views/studio-design/packages-io.envelopeUserMessage.test.ts new file mode 100644 index 0000000000..39d1804fb9 --- /dev/null +++ b/packages/app-shell/src/views/studio-design/packages-io.envelopeUserMessage.test.ts @@ -0,0 +1,283 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * objectui#7959 — the `packages-io` readers open the failure envelope. + * + * ## What was lost, measured + * + * `fetchPackages` answered a refusal with `throw new Error(\`HTTP ${status}\`)`. + * It never opened the body, so `message`, `code` and `userMessage` were + * discarded together — a strictly larger loss than the sibling + * `fetchFullPackage` lookup's (objectui#7938), which at least read `message` + * and `code`. + * + * That matters because every caller of this read reports the words: the Studio + * switcher, the writability courtesy gate and the namespace lookup all render + * `formatMetadataError(e)` — which returns `err.message` — onto the shared + * `studio-package-list` sonner id (objectui#7368's posture), and the builder + * landing page puts the same string in its error banner. So the plumbing to + * display a sentence was already there and already wired; what was missing was + * a sentence to put in it. A 403 whose envelope named the capability to grant + * reached the author as the four characters `HTTP 403`. + * + * `duplicatePackage`, a hundred lines below it in the same module, read + * `error.message` alone — a third reading of the same envelope, dropping the + * mark and the code. Both now ask one shared rule + * (`utils/apiErrorEnvelope.ts`), pinned on its own in `apiErrorEnvelope.test.ts`. + * + * ## Both doors serving these routes emit the marked channel + * + * Not assumed — read from the producers. `sendThrownError` + * (`@objectstack/rest` `package-routes.ts`) spreads + * `...(thrown.userMessage !== undefined ? { userMessage: thrown.userMessage } : {})` + * beside `details`/`declaredCode`, and its own note calls that expression + * "byte for byte the dispatcher twin's" — `errorFromThrown` + * (`@objectstack/runtime` `http-dispatcher.ts`), which serves the lifecycle + * routes including `/duplicate` and has emitted the channel since #9934. + * `sendError` nests the whole object under `error`, which is the path these + * pins drive. The framework pins the wire side in + * `packages/rest/src/package-door-user-message.test.ts`. + * + * ## ⚠️ Where this file's §1 differs from objectui#7938's §1 — read before reverting + * + * objectui#7938's `message`-only pin is GREEN with its fix reverted, because + * that reader already rendered `error.message`; its §1 is the guard that stops + * "prefer `userMessage`" from sliding into "read `userMessage` INSTEAD". + * + * Here it CANNOT be green when reverted, and the difference is the defect, not + * a weaker pin: the pre-fix `fetchPackages` opened no body at all, so the + * unmarked refusal was as lost as the marked one. §1 below is therefore a + * forward assertion of the same guarantee (an unmarked refusal renders its + * diagnostic, not the status) and goes RED on revert along with §2 and §3. The + * "still green when reverted" role is held here by §7 (an unreadable body still + * names the status) and §8 (a successful read is untouched) — without them, a + * "fix" that reported on every read, or that broke the success path by + * consuming the body twice, would satisfy every assertion above. + * + * The `apiJson` twin (`PackagesPage.envelopeUserMessage.test.tsx` §1) DOES hold + * the green-when-reverted guard, because that reader did already render + * `error.message`. + */ + +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { duplicatePackage, fetchPackages } from './packages-io'; + +/** The generic sentence the 5xx prose withhold substitutes (`INTERNAL_ERROR_MESSAGE`). */ +const GENERIC = 'Internal server error'; +/** A producer's marked text: what `userMessage` carries, written for the author. */ +const MARKED = 'Publishing is temporarily unavailable. Nothing was changed.'; +/** + * The card's measured sample. A 403 that names the very capability to grant — + * and the reader was reporting it as `HTTP 403`. + */ +const CAPABILITY = 'Reading packages requires the `studio.access` or `setup.access` capability.'; + +/** A failure exactly as `sendError` writes it — `{ code, message, ...extra }` under `error`. */ +function failure(status: number, error: Record): Response { + return { + ok: false, + status, + json: async () => ({ success: false, error }), + } as unknown as Response; +} + +/** The success shape: `sendOk` wraps the handler's `{ packages, total }`. */ +function listOk(packages: unknown[]): Response { + return { + ok: true, + status: 200, + json: async () => ({ success: true, data: { packages, total: packages.length } }), + } as unknown as Response; +} + +function stubFetch(response: Response) { + const fetchMock = vi.fn().mockResolvedValue(response); + vi.stubGlobal('fetch', fetchMock); + return fetchMock; +} + +/** What `formatMetadataError` would hand the toast — i.e. what the author reads. */ +async function reportedFor(status: number, error: Record): Promise { + stubFetch(failure(status, error)); + return await fetchPackages().then( + () => { + throw new Error('fetchPackages resolved on a refusal'); + }, + (e: unknown) => (e as Error).message, + ); +} + +beforeEach(() => { + vi.restoreAllMocks(); +}); + +afterEach(() => { + vi.unstubAllGlobals(); +}); + +describe('fetchPackages — the four combinations (#7959)', () => { + /** + * ⚠️ RED when the fix is reverted, unlike objectui#7938's §1 — see the file + * docblock. The pre-fix reader opened no body, so the unmarked refusal was + * lost too. The guarantee asserted is the same one: an unmarked refusal + * renders its diagnostic and is NOT displaced by anything. + */ + it('§1 `message` only — the unmarked refusal reaches the author with its code', async () => { + expect(await reportedFor(403, { code: 'FORBIDDEN', message: CAPABILITY })).toBe( + `${CAPABILITY} (FORBIDDEN)`, + ); + }); + + it('§2 `userMessage` only — the marked sentence, not the bare status', async () => { + const shown = await reportedFor(503, { code: 'SERVICE_UNAVAILABLE', userMessage: MARKED }); + expect(shown).toBe(`${MARKED} (SERVICE_UNAVAILABLE)`); + expect(shown).not.toBe('HTTP 503'); + }); + + it('§3 both — the mark displaces the generic substitution, it is not appended to it', async () => { + const shown = await reportedFor(500, { code: 'INTERNAL_ERROR', message: GENERIC, userMessage: MARKED }); + expect(shown).toBe(`${MARKED} (INTERNAL_ERROR)`); + expect(shown).not.toContain(GENERIC); + }); + + it('§4 neither — the status is still the honest answer', async () => { + expect(await reportedFor(503, { code: 'SERVICE_UNAVAILABLE' })).toBe('HTTP 503'); + }); +}); + +describe('fetchPackages — `message` and `code` arrive too, not only the mark (#7959)', () => { + /** + * ⭐ This reader's loss was not one optional field: the body was never opened. + * So the fix has to be shown delivering the WHOLE envelope, and these two + * cases would both be satisfied by a fix that read only `userMessage`. + */ + it('the diagnostic sentence now reaches the author at all', async () => { + expect(await reportedFor(403, { code: 'FORBIDDEN', message: CAPABILITY })).toContain(CAPABILITY); + }); + + it('the machine code now reaches the author at all', async () => { + expect(await reportedFor(409, { code: 'RESOURCE_CONFLICT', message: 'version 1.2.0 already published' })).toBe( + 'version 1.2.0 already published (RESOURCE_CONFLICT)', + ); + }); + + it('the card\'s measured 403 sample is no longer reported as four characters', async () => { + const shown = await reportedFor(403, { code: 'FORBIDDEN', message: CAPABILITY }); + expect(shown).not.toBe('HTTP 403'); + expect(shown).toContain('studio.access'); + expect(shown).toContain('setup.access'); + }); +}); + +describe('fetchPackages — how `code` composes, and what is not a mark', () => { + it('a marked body with NO code renders the bare sentence', async () => { + expect(await reportedFor(503, { message: GENERIC, userMessage: MARKED })).toBe(MARKED); + }); + + it('a code with no prose is still just the status — a code is not a sentence', async () => { + expect(await reportedFor(503, { code: 'SERVICE_UNAVAILABLE' })).toBe('HTTP 503'); + }); + + it('a non-string `userMessage` is not a mark — it falls through to `message`', async () => { + expect(await reportedFor(500, { code: 'INTERNAL_ERROR', message: GENERIC, userMessage: 42 })).toBe( + `${GENERIC} (INTERNAL_ERROR)`, + ); + }); + + it('an empty-string `userMessage` is not a mark either', async () => { + expect(await reportedFor(500, { code: 'INTERNAL_ERROR', message: GENERIC, userMessage: '' })).toBe( + `${GENERIC} (INTERNAL_ERROR)`, + ); + }); + + /** + * ⛔ NOT scoped to 5xx. The producing door applies no status condition to the + * marked channel, so a consumer that honoured it in one band only would + * re-create on the reading end the divergence that door refused to create on + * the writing end (ruled on objectui#7938). + */ + it('prefers the mark in the 4xx band too, where `message` was never withheld', async () => { + expect( + await reportedFor(409, { + code: 'RESOURCE_CONFLICT', + message: 'version 1.2.0 already published for app.b2r4', + userMessage: 'That version is already published. Bump the version and retry.', + }), + ).toBe('That version is already published. Bump the version and retry. (RESOURCE_CONFLICT)'); + }); +}); + +describe('fetchPackages — negative controls (GREEN with the fix reverted)', () => { + /** + * ⭐ §7. A proxy's HTML 502 has no envelope to read. The status is then the + * only honest thing to say, and it is what `packageListErrorPosture` / + * `manageSnapshotRefresh` already pin downstream. + */ + it('§7 an unparseable body still names the status', async () => { + stubFetch({ + ok: false, + status: 502, + json: async () => { + throw new SyntaxError("Unexpected token '<', \"\"... is not valid JSON"); + }, + } as unknown as Response); + + await expect(fetchPackages()).rejects.toThrow('HTTP 502'); + }); + + /** + * ⭐ §8. The success path is untouched — and specifically the body is still + * read exactly once on it. A fix that moved the `res.json()` call, or that + * consumed the body on both arms, would break this while satisfying every + * failure assertion above. + */ + it('§8 a successful read still parses the list and reports nothing', async () => { + stubFetch( + listOk([ + { manifest: { id: 'app.b2r4', name: 'Leave', scope: 'project' }, writable: true }, + { manifest: { id: 'app.kernel', scope: 'system' } }, + ]), + ); + + const list = await fetchPackages(); + expect(list).toEqual([{ id: 'app.b2r4', name: 'Leave', writable: true, namespace: 'b2r4' }]); + }); +}); + +/** + * The second reader in this module. It is here rather than in a card of its own + * because leaving a hand-rolled copy of the rule a hundred lines below the + * import is precisely the drift the extraction exists to stop — and because + * `/packages/:id/duplicate` is served by the dispatcher door, the twin that has + * emitted the marked channel since #9934. + * + * `packages-io.duplicateEnvelope.test.ts` keeps pinning what this reader is + * mainly about — the OPERATION's verdict inside a 200 — and is unaffected. + */ +describe('duplicatePackage — the same envelope, the same rule (#7959)', () => { + it('renders a producer-marked sentence that the old `error.message` read dropped', async () => { + stubFetch(failure(503, { code: 'SERVICE_UNAVAILABLE', message: GENERIC, userMessage: MARKED })); + + await expect(duplicatePackage('a.b.c', 'a.b.d')).rejects.toThrow(`${MARKED} (SERVICE_UNAVAILABLE)`); + }); + + it('still renders the diagnostic when nothing was marked — now with its code', async () => { + stubFetch(failure(403, { code: 'PERMISSION_DENIED', message: 'Permission denied: manage_metadata is required' })); + + await expect(duplicatePackage('a.b.c', 'a.b.d')).rejects.toThrow( + 'Permission denied: manage_metadata is required (PERMISSION_DENIED)', + ); + }); + + it('still falls back to the status when the body is unreadable', async () => { + stubFetch({ + ok: false, + status: 502, + json: async () => { + throw new SyntaxError('Unexpected token < in JSON at position 0'); + }, + } as unknown as Response); + + await expect(duplicatePackage('a.b.c', 'a.b.d')).rejects.toThrow('HTTP 502'); + }); +}); diff --git a/packages/app-shell/src/views/studio-design/packages-io.ts b/packages/app-shell/src/views/studio-design/packages-io.ts index 3707462776..5fbb599a76 100644 --- a/packages/app-shell/src/views/studio-design/packages-io.ts +++ b/packages/app-shell/src/views/studio-design/packages-io.ts @@ -28,6 +28,7 @@ */ import { deriveNamespaceFromPackageId, validateObjectNamespacePrefix } from '@objectstack/spec/kernel'; +import { readEnvelopeFailureText } from '../../utils/apiErrorEnvelope.js'; export interface PkgEntry { id: string; @@ -102,13 +103,42 @@ export function parsePackages(payload: unknown): PkgEntry[] { return out; } +/** + * The package list every Studio surface reads — the switcher, the writability + * courtesy gate, the namespace lookup and the builder landing page. + * + * ## The failure arm used to discard the whole answer + * + * It was `if (!res.ok) throw new Error(\`HTTP ${res.status}\`)`: the body was + * never opened at all, so `message`, `code` and `userMessage` were dropped + * together and the status became the entire report. Every caller here reports + * through `formatMetadataError`, which renders `err.message` — so a 403 whose + * envelope said `Reading packages requires the \`studio.access\` or + * \`setup.access\` capability.` reached the author as the four characters + * `HTTP 403`, and the one sentence that named the capability to grant was + * discarded by the reader that was holding it. + * + * This is a strictly larger loss than the sibling `fetchFullPackage` lookup's + * (objectui#7938), which at least read `message` and `code`. Both now read the + * same envelope by the same rule — see {@link readEnvelopeFailureText} for the + * rule and for why the mark outranks the diagnostic at every status. + * + * ⛔ The fallback stays `HTTP `, unchanged and deliberately so: it is + * what a body-less or unparseable failure (a proxy's HTML 502) still says, and + * `StudioDesignSurface.packageListErrorPosture` / `manageSnapshotRefresh` pin + * that the three-state switcher and the refresh report keep working when the + * wire answers with nothing readable. + */ export async function fetchPackages(): Promise { const res = await fetch('/api/v1/packages', { credentials: 'include', headers: { Accept: 'application/json' }, cache: 'no-store', }); - if (!res.ok) throw new Error(`HTTP ${res.status}`); + if (!res.ok) { + const payload = await res.json().catch(() => null); + throw new Error(readEnvelopeFailureText(payload) ?? `HTTP ${res.status}`); + } return parsePackages(await res.json()); } @@ -189,8 +219,15 @@ export async function duplicatePackage(sourceId: string, targetId: string, targe if (!res.ok) { // The error envelope IS top-level (`{ success: false, error }`) — no `data` // to unwrap on this arm. - const message = (payload?.error as { message?: string } | undefined)?.message; - throw new Error(message || `HTTP ${res.status}`); + // + // Read by the SAME rule as `fetchPackages` above, and not by a second + // hand-rolled ladder: this arm read `error.message` alone, so a + // producer-marked `error.userMessage` — which the dispatcher door serving + // this very route has emitted since #9934 — had nowhere to appear, and + // `error.code` was dropped too. One definition of the rule, in + // {@link readEnvelopeFailureText}; leaving a copy of it a hundred lines + // below the import is exactly the drift this extraction exists to stop. + throw new Error(readEnvelopeFailureText(payload) ?? `HTTP ${res.status}`); } // Unwrap FIRST, then read the operation's flag. The `?? payload` arm mirrors // the commit-revert helper: it would classify a hypothetical bare (unwrapped)