diff --git a/.changeset/publish-failure-reads-error-message.md b/.changeset/publish-failure-reads-error-message.md new file mode 100644 index 0000000000..fae411509c --- /dev/null +++ b/.changeset/publish-failure-reads-error-message.md @@ -0,0 +1,42 @@ +--- +"@objectstack/cli": patch +--- + +`os package publish` now prints the reason a publish was refused instead of the +literal `[object Object]` (#10763). + +Both request helpers in `package/publish.ts` built their failure text the same +way: + +```ts +const errMsg = parsed?.error ?? response.statusText ?? `HTTP ${response.status}`; +return { ok: false, status: response.status, body: parsed, error: String(errMsg) }; +``` + +In the declared envelope `error` is an **object** — `{ code, message }` — so +`String(errMsg)` stringified the object. The `??` chain never reached +`statusText`, because an object is not nullish; there was no useful fallback to +reach. Every failed publish printed the same seven characters no matter what the +control plane had refused, at all three call sites: package registration, +version publish, and the icon upload. + +Both sites now read through a new `readErrorMessage` in +`packages/cli/src/utils/response-envelope.ts`, which returns the declared +envelope's `error.message`, degrades to `error.code` when a refusal carries no +message, and falls back to a non-blank `statusText` and then the status line. A +blank `statusText` counts as absent — HTTP/2 carries no reason phrase, and the +old `??` chain kept the empty string and printed nothing after the status code. + +The reader also accepts the flat `error: ''` shape, deliberately and +temporarily. That is a **measured** property of these routes rather than an +assumption: `/api/v1/cloud/**` is served by the sibling `cloud` repo, and the +closest first-hand reader of that same `service-cloud` family — objectui's +`readApiError` — records that it answers failures in both shapes while cloud#944 +converts it. A strict envelope-only read (the `readEnvelope` landed by #10675 +for the in-repo `/api/v1/datasources/**` routes) would have replaced today's +live flat dialect with a different unreadable failure, so it is not reused here; +the reasoning, and the condition under which the flat branch is deleted, are +recorded on the function. + +No request the CLI sends changes, and the server sends exactly what it sent +before — this is only how a failure is read and shown. diff --git a/packages/cli/src/commands/package/publish.ts b/packages/cli/src/commands/package/publish.ts index 29e29c4696..c1be9ba559 100644 --- a/packages/cli/src/commands/package/publish.ts +++ b/packages/cli/src/commands/package/publish.ts @@ -23,6 +23,7 @@ import { resolve as resolvePath, basename, dirname, isAbsolute } from 'node:path import { Args, Command, Flags } from '@oclif/core'; import { printHeader, printKV, printSuccess, printError, printStep } from '../../utils/format.js'; import { DEFAULT_CLOUD_URL, tryReadCloudConfig } from '../../utils/cloud-config.js'; +import { readErrorMessage } from '../../utils/response-envelope.js'; const MANIFEST_ID_RE = /^[a-z0-9][a-z0-9._-]{0,254}$/i; @@ -643,8 +644,7 @@ export default class PackagePublish extends Command { let parsed: any = null; try { parsed = await response.json(); } catch { /* empty/non-json */ } if (!response.ok) { - const errMsg = parsed?.error ?? response.statusText ?? `HTTP ${response.status}`; - return { ok: false, status: response.status, body: parsed, error: String(errMsg) }; + return { ok: false, status: response.status, body: parsed, error: readErrorMessage(parsed, response) }; } return { ok: true, status: response.status, body: parsed }; } catch (err: any) { @@ -688,8 +688,7 @@ export default class PackagePublish extends Command { let parsed: any = null; try { parsed = await response.json(); } catch { /* empty/non-json */ } if (!response.ok) { - const errMsg = parsed?.error ?? response.statusText ?? `HTTP ${response.status}`; - return { ok: false, status: response.status, body: parsed, error: String(errMsg) }; + return { ok: false, status: response.status, body: parsed, error: readErrorMessage(parsed, response) }; } return { ok: true, status: response.status, body: parsed }; } catch (err: any) { diff --git a/packages/cli/src/utils/response-envelope.test.ts b/packages/cli/src/utils/response-envelope.test.ts index 9748cc6390..1e871a3211 100644 --- a/packages/cli/src/utils/response-envelope.test.ts +++ b/packages/cli/src/utils/response-envelope.test.ts @@ -14,7 +14,7 @@ import { describe, expect, it } from 'vitest'; import { sendError, sendOk } from '@objectstack/types'; import { serverBody } from './__tests__/server-body.js'; -import { readEnvelope, readEnvelopeFrom } from './response-envelope.js'; +import { readEnvelope, readEnvelopeFrom, readErrorMessage } from './response-envelope.js'; describe('readEnvelope', () => { it('returns the payload nested under `data` for a `sendOk` body', () => { @@ -87,3 +87,75 @@ describe('readEnvelopeFrom', () => { expect((read as { message: string }).message).toContain('HTTP 404'); }); }); + +/** + * `readErrorMessage` — the PRINTABLE-message reader (#10763). + * + * Two dialects are covered because the control plane really does emit two: the + * declared envelope, and the flat `error: ''` its `fail()` helper + * still writes while cloud#944 converts it. The declared arm is built with + * `sendError`, the one writer, for the reason the file header gives. The flat + * arm has to be a literal — its writer lives in the closed `cloud` repo and + * cannot be imported — so it is written here exactly as objectui's `readApiError` + * records it, and that transcription is the thing to re-check if it ever drifts. + */ +describe('readErrorMessage', () => { + const res = (status: number, statusText = 'Bad Request') => ({ status, statusText }); + + it('reads `error.message` — the FIELD — out of the declared envelope', () => { + const body = serverBody((r) => + sendError(r, 422, 'PACKAGE_PUBLISH_FAILED', 'Version 1.2.0 already exists for com.acme.crm.'), + ); + + expect(readErrorMessage(body, res(422))).toBe('Version 1.2.0 already exists for com.acme.crm.'); + }); + + it('reads the control plane’s flat `fail()` dialect, which is still live (cloud#944)', () => { + const body = { success: false, error: 'Publisher is not verified.' }; + + expect(readErrorMessage(body, res(403))).toBe('Publisher is not verified.'); + }); + + /** + * The defect this card is named for. `String(parsed?.error)` over the + * declared envelope produced this literal, and `??` never reached the + * `statusText` fallback because an object is not nullish. + */ + it('NEVER renders an error object as text, in any shape', () => { + const bodies: unknown[] = [ + serverBody((r) => sendError(r, 400, 'VALIDATION_ERROR', 'Manifest is invalid.')), + { success: false, error: { code: 'FORBIDDEN' } }, + { success: false, error: {} }, + { success: false, error: [] }, + { success: false, error: { message: 42 } }, + { success: false, error: { message: ' ' } }, + ]; + + for (const body of bodies) { + const read = readErrorMessage(body, res(400)); + expect(typeof read, JSON.stringify(body)).toBe('string'); + expect(read, JSON.stringify(body)).not.toContain('[object'); + } + }); + + it('falls back to the code when the envelope refuses without a message', () => { + expect(readErrorMessage({ success: false, error: { code: 'PACKAGE_PUBLISH_FAILED' } }, res(422))) + .toBe('PACKAGE_PUBLISH_FAILED'); + }); + + it('falls back to `statusText`, then to the status line, when the body carries no text', () => { + expect(readErrorMessage(null, res(502, 'Bad Gateway'))).toBe('Bad Gateway'); + expect(readErrorMessage({ success: false }, res(502, 'Bad Gateway'))).toBe('Bad Gateway'); + expect(readErrorMessage('not json at all', res(502, 'Bad Gateway'))).toBe('Bad Gateway'); + }); + + /** + * HTTP/2 carries no reason phrase, so `fetch` reports `statusText` as ''. The + * original chain used `??`, which keeps an empty string and printed nothing + * after the status code — the second half of "there is no useful fallback". + */ + it('treats a blank `statusText` as absent rather than printing nothing', () => { + expect(readErrorMessage(null, { status: 500, statusText: '' })).toBe('HTTP 500'); + expect(readErrorMessage(null, { status: 500 })).toBe('HTTP 500'); + }); +}); diff --git a/packages/cli/src/utils/response-envelope.ts b/packages/cli/src/utils/response-envelope.ts index 54cd1d1c9d..16351e8557 100644 --- a/packages/cli/src/utils/response-envelope.ts +++ b/packages/cli/src/utils/response-envelope.ts @@ -113,3 +113,95 @@ export async function readEnvelopeFrom(res: EnvelopeSource): Promise(body, res.status); } + +/** + * Read a PRINTABLE failure message out of a response body the CLI has already + * decided is a failure. + * + * ## Why this is tolerant when {@link readEnvelope} above is strict + * + * They do different jobs, and the strictness follows the job rather than the + * file. `readEnvelope` decides **whether** a request succeeded and hands back + * its payload; tolerating an off-spec body there would let a payload be read as + * data, which is how a second de-facto contract grows (Prime Directive #12) and + * why that reader refuses the legacy flat shape outright. + * + * This function decides **nothing**. The caller has already seen + * `!response.ok`; the only remaining question is which bytes to show a human. + * The failure is reported either way — the choice is between the server's own + * sentence and a placeholder. + * + * ## What the control plane actually sends (measured, not assumed) + * + * The `/api/v1/cloud/**` publish routes are served by the sibling `cloud` repo, + * which this repo's dispatcher explicitly refuses — so no in-repo ledger can + * vouch for them (`docs/audits/2026-07-dispatcher-client-route-coverage.md` + * §10). The measurement comes from the closest first-hand reader of that same + * `service-cloud` family, `readApiError` in objectui's + * `packages/app-shell/src/console/marketplace/marketplaceApi.ts`, which records + * that those routes answer failures in TWO shapes and are mid-conversion from + * one to the other (cloud#944): + * + * { success: false, error: 'a sentence' } today, via the cloud's `fail()` + * { success: false, error: { code, message } } the declared envelope + * + * So BOTH arms are live. That is what rules out reusing `readEnvelope` here: + * against the dialect the control plane still emits today it would discard a + * real explanation and print "not the declared envelope" instead — trading one + * unreadable failure for another. + * + * ## This accommodation is bounded, and it is the consumer's only option + * + * The flat dialect is a PRODUCER defect, tracked and being converted at the + * producer (cloud#944) — it is not a shape this repo can fix, and not one it is + * hiding. When that conversion lands, the `fail()` branch below is deletable on + * its own, and nothing else here changes. + * + * Deliberately NOT accepted: a top-level `body.message`. objectui's reader + * tolerates one because a few OTHER routes in that family put text there; no + * publish route was measured doing it, and inventing a third dialect to read is + * the accretion #12 forbids. + * + * ## Why it can never return `[object Object]` + * + * Every branch either yields a checked non-empty string or falls through. The + * defect under repair was `String(parsed?.error)` over an `error` that is an + * OBJECT: `??` never fell through to `statusText`, because an object is not + * nullish, so the fallback chain was unreachable and the operator got + * `[object Object]` instead of the reason the publish was refused. + * + * `statusText` is treated as absent when blank for the same reason the original + * chain failed: HTTP/2 carries no reason phrase, so `??` would have kept an + * empty string and printed nothing at all. + */ +export function readErrorMessage( + body: unknown, + res: { status: number; statusText?: string }, +): string { + return errorTextFrom(body) ?? blankToUndefined(res.statusText) ?? `HTTP ${res.status}`; +} + +/** A string is usable as a message only when it is actually a non-blank string. */ +function blankToUndefined(value: unknown): string | undefined { + return typeof value === 'string' && value.trim().length > 0 ? value : undefined; +} + +/** + * The server's own text, in whichever of the two measured dialects it arrived — + * or `undefined` when the body carries none, so the caller can fall back. + */ +function errorTextFrom(body: unknown): string | undefined { + if (typeof body !== 'object' || body === null) return undefined; + const error = (body as { error?: unknown }).error; + + if (typeof error === 'object' && error !== null) { + // The declared envelope: `message` is a FIELD of `error`, never the object + // itself. `code` is the last resort that keeps a refusal naming SOMETHING + // machine-readable rather than degrading to a bare status line. + const declared = error as { code?: unknown; message?: unknown }; + return blankToUndefined(declared.message) ?? blankToUndefined(declared.code); + } + + // The control plane's `fail()` dialect (cloud#944): `error` IS the sentence. + return blankToUndefined(error); +} diff --git a/packages/cli/test/package-publish-error-envelope.test.ts b/packages/cli/test/package-publish-error-envelope.test.ts new file mode 100644 index 0000000000..44909e730e --- /dev/null +++ b/packages/cli/test/package-publish-error-envelope.test.ts @@ -0,0 +1,218 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * `os package publish` renders a failed publish as the SERVER's own sentence, + * never as the literal `[object Object]` (#10763). + * + * ## The defect + * + * Both request helpers built their failure text with + * `String(parsed?.error ?? response.statusText ?? ...)`. In the declared + * envelope `error` is an OBJECT — `{ code, message }` — so `String(...)` + * stringified the object, and `??` never reached `statusText` because an object + * is not nullish. Every failed publish printed the same seven characters + * regardless of what the control plane had refused and why. + * + * ## Why the reader is tolerant, and why that is a measurement rather than a guess + * + * The `/api/v1/cloud/**` routes are served by the sibling `cloud` repo, so no + * in-repo suite can drive the real producer. The measurement that picked the + * read shape is objectui's `readApiError` + * (`packages/app-shell/src/console/marketplace/marketplaceApi.ts`): the same + * `service-cloud` family answers failures in BOTH the declared envelope and a + * flat `error: ''` written by its `fail()` helper, mid-conversion + * under cloud#944. Both arms are therefore driven below. A strict envelope-only + * read would have turned today's live flat dialect into a different unreadable + * failure, which is why #10675's reader is not reused here — see the docblock + * on `readErrorMessage`. + * + * ## Why the command is driven rather than the helper alone + * + * The helper has its own unit coverage. These cases exist because the defect + * was at the CALL SITE — the value handed to `printError` — and all three sites + * (`postJson` twice, `postBinary` once) have to be shown reaching the operator + * with the server's text, including the icon upload, which is the only + * `postBinary` caller in the command. + */ + +import { describe, it, expect, afterEach, beforeEach, vi } from 'vitest'; +import { mkdtemp, rm, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { sendError } from '@objectstack/types'; +import { serverBody } from '../src/utils/__tests__/server-body.js'; +import PackagePublish from '../src/commands/package/publish.js'; + +/** A failure body written by the server's OWN writer — the declared envelope. */ +const declared = (status: number, code: any, message: string) => + serverBody((r) => sendError(r, status, code, message)); + +/** + * The control plane's still-live flat dialect (cloud#944). Written as a literal + * because its writer lives in the closed `cloud` repo; this transcription is + * what to re-check if that dialect ever changes. + */ +const flat = (message: string) => ({ success: false, error: message }); + +/** Smallest valid PNG, so `--icon-file` reaches the binary upload step. */ +const PNG_1X1 = Buffer.from( + 'iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mP8z8BQDwAEhQGAhKmMIQAAAABJRU5ErkJggg==', + 'base64', +); + +type Call = { url: string }; + +describe('os package publish — a failed publish shows the server’s reason', () => { + let dir = ''; + const prevEnv = { url: process.env.OS_CLOUD_URL, key: process.env.OS_CLOUD_API_KEY }; + const prevCwd = process.cwd(); + let output: string[] = []; + + beforeEach(() => { + output = []; + for (const channel of ['error', 'log', 'warn'] as const) { + vi.spyOn(console, channel).mockImplementation((...args: unknown[]) => { + output.push(args.map(String).join(' ')); + }); + } + }); + + afterEach(async () => { + process.chdir(prevCwd); + vi.unstubAllGlobals(); + vi.restoreAllMocks(); + process.env.OS_CLOUD_URL = prevEnv.url; + process.env.OS_CLOUD_API_KEY = prevEnv.key; + if (dir) await rm(dir, { recursive: true, force: true }); + dir = ''; + }); + + async function artifact(): Promise { + dir = await mkdtemp(join(tmpdir(), 'package-publish-err-')); + const path = join(dir, 'objectstack.json'); + await writeFile( + path, + JSON.stringify({ + manifest: { id: 'com.acme.crm', name: 'Acme CRM', version: '1.2.0' }, + objects: [], + }), + ); + process.env.OS_CLOUD_URL = 'http://cloud.test'; + process.env.OS_CLOUD_API_KEY = 'tok_123'; + return path; + } + + /** + * Stub the cloud so exactly one step fails with `body`, and every earlier + * step succeeds — so each case isolates one call site. + */ + function stubCloud(failOn: 'packages' | 'icon' | 'versions', status: number, body: unknown): Call[] { + const calls: Call[] = []; + vi.stubGlobal('fetch', vi.fn(async (url: string) => { + calls.push({ url }); + const step = url.endsWith('/icon') ? 'icon' : url.endsWith('/versions') ? 'versions' : 'packages'; + if (step === failOn) { + // `statusText` is '' exactly as an HTTP/2 response reports it, so no + // case can pass on a reason phrase the real transport would not supply. + return { ok: false, status, statusText: '', json: async () => body } as any; + } + const data = step === 'versions' + ? { id: 'ver_1', version: '1.2.0', listing_status: 'draft' } + : step === 'icon' + ? { icon_url: '/icons/com.acme.crm.png' } + : { id: 'pkg_1', created: true, visibility: 'org' }; + return { ok: true, status: 200, statusText: 'OK', json: async () => ({ success: true, data }) } as any; + })); + return calls; + } + + async function runExpectingExit1(args: string[]): Promise { + let exitCode: number | undefined; + try { + await PackagePublish.run(args); + } catch (err: any) { + exitCode = err?.oclif?.exit ?? err?.exitCode; + } + expect(exitCode).toBe(1); + } + + it('prints `error.message` from the declared envelope when registering the package fails', async () => { + const path = await artifact(); + const calls = stubCloud( + 'packages', + 422, + declared(422, 'PACKAGE_PUBLISH_FAILED', 'Namespace "crm" is reserved by another publisher.'), + ); + + await runExpectingExit1([path]); + + const printed = output.join('\n'); + expect(calls[0].url).toBe('http://cloud.test/api/v1/cloud/packages'); + expect(printed).toContain('Namespace "crm" is reserved by another publisher.'); + expect(printed).not.toContain('[object'); + // The status still reaches the operator alongside the reason. + expect(printed).toContain('422'); + }); + + it('prints the flat `fail()` sentence the control plane still emits (cloud#944)', async () => { + const path = await artifact(); + stubCloud('packages', 403, flat('Publisher is not verified.')); + + await runExpectingExit1([path]); + + const printed = output.join('\n'); + expect(printed).toContain('Publisher is not verified.'); + expect(printed).not.toContain('[object'); + }); + + it('prints the server’s reason when the VERSION publish fails — the second `postJson` site', async () => { + const path = await artifact(); + const calls = stubCloud( + 'versions', + 422, + declared(422, 'PACKAGE_PUBLISH_FAILED', 'Version 1.2.0 already exists for com.acme.crm.'), + ); + + await runExpectingExit1([path]); + + const printed = output.join('\n'); + expect(calls.map((c) => c.url)).toEqual([ + 'http://cloud.test/api/v1/cloud/packages', + 'http://cloud.test/api/v1/cloud/packages/pkg_1/versions', + ]); + expect(printed).toContain('Version 1.2.0 already exists for com.acme.crm.'); + expect(printed).not.toContain('[object'); + }); + + it('prints the server’s reason when the ICON upload fails — the `postBinary` site', async () => { + const path = await artifact(); + const iconPath = join(dir, 'icon.png'); + await writeFile(iconPath, PNG_1X1); + const calls = stubCloud('icon', 413, declared(413, 'VALIDATION_ERROR', 'Icon exceeds the 512 KB limit.')); + + await runExpectingExit1([path, '--icon-file', iconPath]); + + const printed = output.join('\n'); + expect(calls.some((c) => c.url.endsWith('/icon'))).toBe(true); + expect(printed).toContain('Icon exceeds the 512 KB limit.'); + expect(printed).not.toContain('[object'); + }); + + /** + * Reverse verification. With no readable text anywhere in the body AND no + * reason phrase (HTTP/2), the operator must still get the status line rather + * than an empty tail — the half of the old chain that `??` also broke, since + * an empty `statusText` is not nullish either. + */ + it('falls back to the status line when the body carries no readable text', async () => { + const path = await artifact(); + stubCloud('packages', 500, { success: false }); + + await runExpectingExit1([path]); + + const printed = output.join('\n'); + expect(printed).toContain('HTTP 500'); + expect(printed).not.toContain('[object'); + expect(printed).not.toContain('undefined'); + }); +});