From 1016233cb77cd8afe5ba256d1be328cf63c20a32 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 21 Aug 2026 19:34:27 +0000 Subject: [PATCH] fix(cli): os package publish prints the server's reason, not [object Object] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both request helpers in package/publish.ts built their failure text with String(parsed?.error ?? response.statusText ?? `HTTP ${status}`). In the declared envelope `error` is an OBJECT — { code, message } — so String() stringified it, and the ?? chain never reached statusText because an object is not nullish. All three call sites (package registration, version publish, icon upload) printed the same literal regardless of what the control plane refused. Both sites now read through a new readErrorMessage in packages/cli/src/utils/response-envelope.ts: the declared envelope's error.message, degrading to error.code, then a non-blank statusText, then the status line. A blank statusText counts as absent — HTTP/2 carries no reason phrase, and the old ?? chain kept the empty string. The reader also accepts the flat `error: ''` dialect. That is measured, not assumed: /api/v1/cloud/** is served by the sibling cloud repo, and objectui's readApiError records that the same service-cloud family answers failures in both shapes while cloud#944 converts it. A strict envelope-only read (#10675's readEnvelope, measured against 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 two sibling readings are deliberately untouched: plugin/publish.ts already reads correctly on both measured arms, and package/install.ts targets the runtime's /api/v1/marketplace/install-local, a different route family. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019bmVFqoQPq63zhKrxdYG1r --- .../publish-failure-reads-error-message.md | 42 ++++ packages/cli/src/commands/package/publish.ts | 7 +- .../cli/src/utils/response-envelope.test.ts | 74 +++++- packages/cli/src/utils/response-envelope.ts | 92 ++++++++ .../package-publish-error-envelope.test.ts | 218 ++++++++++++++++++ 5 files changed, 428 insertions(+), 5 deletions(-) create mode 100644 .changeset/publish-failure-reads-error-message.md create mode 100644 packages/cli/test/package-publish-error-envelope.test.ts 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'); + }); +});