From 586d7257ddc130facbee7b511e2b4b9345ce30dd Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 27 Aug 2026 12:56:56 +0000 Subject: [PATCH] fix(app-shell): unwrap the dispatcher envelope before reading duplicatePackage's verdict `duplicatePackage()` read `success` at the top level of the response body. That is the runtime dispatcher's envelope (`deps.success(result)` answers `{ success: true, data }`), so on every HTTP 200 the flag it read was `true` by construction. The operation's own verdict lives one level down in `data` and is a three-state, computed server-side as `failed.length === 0 && copied.length > 0`. Two outcomes therefore answered 200 with `envelope.success: true` while the duplicate had not succeeded, and both were reported to the Studio author as a complete success: - partial: some items failed to copy, with `failed[].error` the only place the reason is ever stated; - empty: nothing was copied at all (e.g. an all-env-wide source package under a session that resolves no active organization). Unwrap `data` first, then read the operation flag -- the order `revertCommit` already uses for the sibling commit-revert route in `preview/commitHistory.ts`. A false verdict now rejects with the copied/failed counts plus each `failed[].error` (first five by name, then a `+N more` tail); a generic `HTTP nnn` is not sufficient for the partial arm. The non-2xx arm is unchanged. Both reachable false states are pinned; a suite covering only `res.ok === false` would re-create the defect, because the transport arm was never the broken one. Refs objectui#6593. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CRJge11jso9TpXRWFt1Z49 --- .../6593-duplicate-package-envelope-unwrap.md | 33 +++ .../packages-io.duplicateEnvelope.test.ts | 203 ++++++++++++++++++ .../src/views/studio-design/packages-io.ts | 81 ++++++- 3 files changed, 312 insertions(+), 5 deletions(-) create mode 100644 .changeset/6593-duplicate-package-envelope-unwrap.md create mode 100644 packages/app-shell/src/views/studio-design/packages-io.duplicateEnvelope.test.ts diff --git a/.changeset/6593-duplicate-package-envelope-unwrap.md b/.changeset/6593-duplicate-package-envelope-unwrap.md new file mode 100644 index 0000000000..3dec6496ad --- /dev/null +++ b/.changeset/6593-duplicate-package-envelope-unwrap.md @@ -0,0 +1,33 @@ +--- +'@object-ui/app-shell': patch +--- + +Studio's "duplicate base" now reports a partial or empty duplicate as a failure instead of +a complete success (objectui#6593). + +`duplicatePackage()` read `success` at the **top level** of the response body. That is the +runtime dispatcher's envelope — `deps.success(result)` answers +`{ success: true, data }` — so on every HTTP 200 the flag it read was `true` by +construction. The operation's own verdict lives one level down in `data` and is a real +three-state: the server computes it as `failed.length === 0 && copied.length > 0`. + +Two outcomes therefore answered 200 with `envelope.success: true` while the duplicate had +not succeeded, and both were shown to the author as "created", followed by a navigation +into the new base: + +- **partial** — some items failed to copy. `failed[]` carries a per-item `error` string + that is the only place the reason is ever stated, and none of it was read. +- **empty** — nothing was copied at all, e.g. an all-env-wide source package under a + session that resolves no active organization (`copiedCount: 0`, `failedCount: 0`). + +`duplicatePackage()` now unwraps `data` before reading the operation flag, and rejects with +a message built from what actually happened: the copied/failed counts, plus each +`failed[].error` (the first five by name, then a `+N more` tail). A generic `HTTP nnn` +message is deliberately not sufficient for the partial arm — it is the half an author needs +to act on. The non-2xx arm is unchanged and still surfaces the error envelope's message. + +The unwrap-then-read order is the one `revertCommit` already uses for the sibling +commit-revert route in `preview/commitHistory.ts`; this is one consumer converging on that, +not a new convention. The route's underlying contract absence (it publishes no response +schema, so reading the wrong `success` typechecks perfectly) stays upstream in +objectstack#12038. diff --git a/packages/app-shell/src/views/studio-design/packages-io.duplicateEnvelope.test.ts b/packages/app-shell/src/views/studio-design/packages-io.duplicateEnvelope.test.ts new file mode 100644 index 0000000000..03fb3ea25b --- /dev/null +++ b/packages/app-shell/src/views/studio-design/packages-io.duplicateEnvelope.test.ts @@ -0,0 +1,203 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * `duplicatePackage` must read the OPERATION's verdict, not the ENVELOPE's. + * + * `POST /packages/:id/duplicate` is served only by the runtime dispatcher, and + * that door always wraps: `deps.success(result)` answers + * `{ status: 200, body: { success: true, data } }`. So the top-level `success` + * is `true` by construction on every 200, and reading it classifies every 200 + * as a complete success. The operation's own verdict lives one level down in + * `data`, computed server-side as `failed.length === 0 && copied.length > 0`. + * + * That leaves TWO reachable outcomes which answer HTTP 200 while the duplicate + * did not succeed, and this file pins BOTH — a suite that only covered + * `res.ok === false` would re-create the defect, because the transport arm was + * never the broken one: + * + * 1. PARTIAL — `failed.length > 0`. The per-item `error` strings in `failed[]` + * are the only place the reason is ever stated, so a generic `HTTP nnn` + * message is explicitly NOT sufficient here; that is asserted, not implied. + * 2. EMPTY — `copied.length === 0` with nothing named as failed, e.g. an + * all-env-wide source package under a session that resolves no active + * organization. Measured upstream as `{ success: false, copiedCount: 0, + * failedCount: 0 }`. + * + * The pattern being converged on is the sibling commit-revert helper in + * `preview/commitHistory.ts` (`revertCommit`), which already unwraps `data` + * before reading the flag. This is one consumer catching up to it, not a new + * convention. + */ + +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { duplicatePackage } from './packages-io'; + +/** The dispatcher's success envelope: the operation result under `data`. */ +function envelope(data: unknown, status = 200) { + return { ok: status >= 200 && status < 300, status, json: async () => ({ success: true, data }) } as unknown as Response; +} + +/** The dispatcher's error envelope — top-level, with no `data` to unwrap. */ +function errorEnvelope(status: number, message: string) { + return { + ok: false, + status, + json: async () => ({ success: false, error: { message, code: 'PERMISSION_DENIED', status } }), + } as unknown as Response; +} + +function stubFetch(response: Response) { + const fetchMock = vi.fn().mockResolvedValue(response); + vi.stubGlobal('fetch', fetchMock); + return fetchMock; +} + +beforeEach(() => { + vi.restoreAllMocks(); +}); + +afterEach(() => { + vi.unstubAllGlobals(); +}); + +describe('duplicatePackage — the operation verdict lives under `data`', () => { + it('rejects a PARTIAL duplicate and names the counts and every per-item error', async () => { + stubFetch( + envelope({ + success: false, + copiedCount: 7, + failedCount: 2, + targetPackageId: 'com.example.leave-copy', + copied: [], + failed: [ + { type: 'object', name: 'leave_request', error: 'copy failed: duplicate key' }, + { type: 'view', name: 'leave_list', error: 'the source item does not convert' }, + ], + }), + ); + + const err = await duplicatePackage('com.example.leave', 'com.example.leave-copy').then( + () => null, + (e: unknown) => e as Error, + ); + + expect(err).toBeInstanceOf(Error); + // The counts… + expect(err?.message).toContain('7'); + expect(err?.message).toContain('2'); + // …and the per-item reasons, which exist nowhere else in the response. + expect(err?.message).toContain('object/leave_request'); + expect(err?.message).toContain('copy failed: duplicate key'); + expect(err?.message).toContain('view/leave_list'); + expect(err?.message).toContain('the source item does not convert'); + // A generic transport message is NOT sufficient for this arm — it is the + // half the fix exists to deliver. + expect(err?.message).not.toMatch(/HTTP \d/); + }); + + it('summarizes the tail rather than pasting an unbounded failure list', async () => { + stubFetch( + envelope({ + success: false, + copiedCount: 0, + failedCount: 7, + failed: Array.from({ length: 7 }, (_, i) => ({ + type: 'object', + name: `obj_${i}`, + error: `copy failed ${i}`, + })), + }), + ); + + const err = await duplicatePackage('a.b.c', 'a.b.d').catch((e: unknown) => e as Error); + + expect(err?.message).toContain('object/obj_0'); + expect(err?.message).toContain('object/obj_4'); + expect(err?.message).not.toContain('object/obj_5'); + expect(err?.message).toContain('+2 more'); + }); + + it('rejects an EMPTY duplicate — 200, nothing copied, nothing named as failed', async () => { + stubFetch( + envelope({ + success: false, + copiedCount: 0, + failedCount: 0, + targetPackageId: 'com.example.leave-copy', + copied: [], + failed: [], + }), + ); + + const err = await duplicatePackage('com.example.leave', 'com.example.leave-copy').then( + () => null, + (e: unknown) => e as Error, + ); + + expect(err).toBeInstanceOf(Error); + expect(err?.message).toMatch(/nothing was copied/i); + // The empty arm has no `failed[]` to quote, so the message must still say + // something the author can act on instead of falling through to a status. + expect(err?.message).not.toMatch(/HTTP \d/); + }); + + it('does NOT let the envelope alone stand in for the operation verdict', async () => { + // The exact shape the defect read as success: `success: true` at the top + // level, `success: false` one level down. + const fetchMock = stubFetch( + envelope({ success: false, copiedCount: 0, failedCount: 1, failed: [{ type: 'object', name: 'x', error: 'nope' }] }), + ); + + await expect(duplicatePackage('a.b.c', 'a.b.d')).rejects.toThrow(); + expect(fetchMock).toHaveBeenCalledTimes(1); + }); + + it('resolves when the operation itself succeeded', async () => { + stubFetch( + envelope({ + success: true, + copiedCount: 9, + failedCount: 0, + targetPackageId: 'com.example.leave-copy', + copied: [{ type: 'object', name: 'leave_request' }], + failed: [], + }), + ); + + await expect(duplicatePackage('com.example.leave', 'com.example.leave-copy')).resolves.toBeUndefined(); + }); + + it('still surfaces the error envelope message on a non-2xx', async () => { + stubFetch(errorEnvelope(403, 'Permission denied: manage_metadata is required')); + + await expect(duplicatePackage('a.b.c', 'a.b.d')).rejects.toThrow( + 'Permission denied: manage_metadata is required', + ); + }); + + it('falls back to the status when a non-2xx carries no readable body', 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'); + }); + + it('posts the target id and name to the duplicate route', async () => { + const fetchMock = stubFetch(envelope({ success: true, copiedCount: 1, failedCount: 0, copied: [{}], failed: [] })); + + await duplicatePackage('com.example.leave', 'com.example.leave-copy', 'Leave (copy)'); + + expect(fetchMock).toHaveBeenCalledWith( + '/api/v1/packages/com.example.leave/duplicate', + expect.objectContaining({ + method: 'POST', + body: JSON.stringify({ targetPackageId: 'com.example.leave-copy', targetName: 'Leave (copy)' }), + }), + ); + }); +}); 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 4f70bbf466..ccf2908258 100644 --- a/packages/app-shell/src/views/studio-design/packages-io.ts +++ b/packages/app-shell/src/views/studio-design/packages-io.ts @@ -54,11 +54,71 @@ export async function fetchPackages(): Promise { return parsePackages(await res.json()); } +/** + * The duplicate operation's OWN result, which travels inside the dispatcher + * envelope's `data` — not at the top level. + * + * `POST /packages/:id/duplicate` is served only by the runtime dispatcher, and + * that door always wraps (`deps.success(result)` → `{ success: true, data }`). + * So a top-level `success` read is reading the ENVELOPE, whose value is `true` + * by construction on a 200. The operation's verdict is one level down, and it + * is a real three-state: the server computes it as + * `failed.length === 0 && copied.length > 0`, which leaves TWO reachable + * outcomes that answer HTTP 200 with `data.success === false`: + * + * 1. PARTIAL — some items failed to copy. `failed[]` carries a per-item + * `error` string and is the ONLY place the reason is ever stated; a + * generic `HTTP nnn` here tells the author nothing. + * 2. EMPTY — nothing was copied at all (e.g. a source package whose rows are + * outside this caller's scope). `failed[]` is empty in this arm — nothing + * is named as having failed — so the counts are the only signal. + * + * Those two are exhaustive for `success === false`: with no failures AND at + * least one copy the server would have said `true`. + */ +interface DuplicateOutcome { + success?: boolean; + copiedCount?: number; + failedCount?: number; + failed?: Array<{ type?: string; name?: string; error?: string }>; +} + +/** Per-item failures named in full before the message summarizes the rest. */ +const DUPLICATE_FAILURE_DETAIL_LIMIT = 5; + +/** + * Turn a false operation verdict into something the Studio author can act on. + * The counts say how far the copy got; `failed[].error` says why each item did + * not make it. Both arms are reachable on a 200 — see {@link DuplicateOutcome}. + */ +function describeDuplicateFailure(outcome: DuplicateOutcome): string { + const failed = Array.isArray(outcome.failed) ? outcome.failed : []; + const failedCount = typeof outcome.failedCount === 'number' ? outcome.failedCount : failed.length; + const copiedCount = typeof outcome.copiedCount === 'number' ? outcome.copiedCount : 0; + if (failedCount > 0) { + const shown = failed + .slice(0, DUPLICATE_FAILURE_DETAIL_LIMIT) + .map((f) => `${f.type || 'item'}/${f.name || '?'}: ${f.error || 'copy failed'}`); + const rest = failed.length - shown.length; + const detail = shown.length ? ` ${shown.join('; ')}${rest > 0 ? `; +${rest} more` : ''}` : ''; + return `Partial duplicate: ${copiedCount} item(s) copied, ${failedCount} failed.${detail}`; + } + return ( + 'Nothing was copied: the duplicate is empty (0 items copied, 0 reported as failed). ' + + "The source package may have no active items, or its rows may be outside this session's scope." + ); +} + /** * Duplicate a package into a NEW writable base (ADR-0070 D4 — the Airtable * "duplicate base" gesture; POST /packages/:id/duplicate). This is how a * read-only code package becomes a customizable starting point: objects are * re-namespaced and intra-package references rewritten server-side. + * + * Rejects on a transport/error-envelope failure AND on a false operation + * verdict, which a 200 can carry. Unwrapping before reading that verdict is + * the same order `revertCommit` uses for the sibling commit-revert route in + * `preview/commitHistory.ts`. */ export async function duplicatePackage(sourceId: string, targetId: string, targetName?: string): Promise { const res = await fetch(`/api/v1/packages/${encodeURIComponent(sourceId)}/duplicate`, { @@ -67,11 +127,22 @@ export async function duplicatePackage(sourceId: string, targetId: string, targe headers: { 'Content-Type': 'application/json', Accept: 'application/json' }, body: JSON.stringify({ targetPackageId: targetId, ...(targetName ? { targetName } : {}) }), }); - const payload = (await res.json().catch(() => null)) as - | { success?: boolean; error?: { message?: string } } - | null; - if (!res.ok || payload?.success === false) { - throw new Error(payload?.error?.message || `HTTP ${res.status}`); + const payload = (await res.json().catch(() => null)) as Record | null; + 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}`); + } + // Unwrap FIRST, then read the operation's flag. The `?? payload` arm mirrors + // the commit-revert helper: it would classify a hypothetical bare (unwrapped) + // answer instead of reading it as success. Against today's single wrapping + // surface only the `data` arm is live. + const inner = ((payload?.data as Record | undefined) ?? payload ?? undefined) as + | DuplicateOutcome + | undefined; + if (inner?.success === false) { + throw new Error(describeDuplicateFailure(inner)); } }