fix(service-storage): emit the declared success envelope on all eight routes (#3689)

.changeset/service-storage-success-envelope.md

@@ -0,0 +1,65 @@
+---
+"@objectstack/spec": patch
+"@objectstack/service-storage": patch
+"@objectstack/client": patch
+---
+
+fix(service-storage): emit the declared success envelope on all eight routes (#3689)
+
+#3675 moved the **error** bodies of the autonomously-mounted `/api/v1/storage/*`
+routes into the declared `{ success: false, error: { code, message } }`
+envelope and deliberately stopped there: unlike the errors, the success bodies
+were not an additive fix. They were three shapes, none of them carrying the
+`success` flag `BaseResponseSchema` declares and
+`ObjectStackClient.unwrapResponse` keys on —
+
+| Route(s) | Was | Now |
+|---|---|---|
+| the six upload routes (`/upload/presigned`, `/upload/complete`, `/upload/chunked`, `…/chunk/:i`, `…/complete`, `…/progress`) | `{ data: {…} }` | `{ success: true, data: {…} }` |
+| `GET /files/:fileId/url` | `{ url }` | `{ success: true, data: { url } }` |
+| `PUT /_local/raw/:token` | `{ ok: true, key }` | `{ success: true, data: { key } }` |
+
+— while `storage.zod.ts` declared every one of them as
+`BaseResponseSchema.extend({ data })`, and `PresignedUrlResponse` and friends
+are `z.infer`red from those schemas and published as the SDK's return types.
+The declaration said `success: boolean`; the wire said nothing. It broke
+nothing only because the storage SDK methods returned `res.json()` raw —
+`any`, so TypeScript could not see the gap and nothing relied on the
+declaration. That is the posture i18n was in before #3636, right up until
+something did rely on it.
+
+**The payload moved on two routes, and that is the breaking part.** A direct
+HTTP caller reading `body.url` from `GET /files/:fileId/url` must now read
+`body.data.url`; one reading `body.ok`/`body.key` from the local adapter's
+`PUT /_local/raw/:token` loopback must read `body.success`/`body.data.key`.
+`ok` is dropped rather than kept beside `success` — it was a second, private
+word for the same thing. The six upload routes are additive: callers already
+destructure `.data`, and a new sibling key changes nothing.
+
+Every in-repo consumer was fixed first, so the two repos are not coupled by
+merge order:
+
+- `client.storage.getDownloadUrl()` now reads through `unwrapResponse`, the
+ SDK's one standard envelope seam — which strips the envelope when present
+ and returns the body untouched when not, so a client either side of this
+ server change resolves the same URL. The other storage methods hand back the
+ whole envelope by design and were already correct.
+- The console's two attachment openers (`RecordAttachmentsPanel`,
+ `ApprovalsInboxPage`) already read `body?.url ?? body?.data?.url`; objectui
+ gains tests pinning that tolerance as deliberate.
+
+Two schemas that were missing are now declared — `FileDownloadUrlResponse` and
+`RawUploadResponse` — and `getDownloadUrl` joins `StorageApiContracts`, which
+it had never been in. That absence is how its shape drifted outside the
+envelope unnoticed. The two `_local/raw/:token` routes stay out of the
+registry on purpose: they are the local adapter's own presign loopback,
+ledgered `server-only` and addressed as an opaque signed URL rather than as an
+API.
+
+`success-envelope.conformance.test.ts` holds the new shape in place the way
+`error-envelope.conformance.test.ts` holds the error one: every route is
+driven and its body parsed against the **declared schema** it answers to — not
+a restatement — the retired shapes are asserted dead, and the module source is
+scanned so a new route cannot bypass the `sendOk` helper. As with #3675, the
+route ledgers cannot catch this class of drift: they audit which routes exist
+and whether the SDK can address them, not what comes back.

content/docs/references/api/storage.mdx

@@ -20,8 +20,8 @@ rather than proxying bytes through the API server.
 ## TypeScript Usage
 
 ```typescript
-import { CompleteChunkedUploadRequest, CompleteChunkedUploadResponse, CompleteUploadRequest, FileTypeValidation, FileUploadResponse, GetPresignedUrlRequest, InitiateChunkedUploadRequest, InitiateChunkedUploadResponse, PresignedUrlResponse, UploadChunkRequest, UploadChunkResponse, UploadProgress } from '@objectstack/spec/api';
-import type { CompleteChunkedUploadRequest, CompleteChunkedUploadResponse, CompleteUploadRequest, FileTypeValidation, FileUploadResponse, GetPresignedUrlRequest, InitiateChunkedUploadRequest, InitiateChunkedUploadResponse, PresignedUrlResponse, UploadChunkRequest, UploadChunkResponse, UploadProgress } from '@objectstack/spec/api';
+import { CompleteChunkedUploadRequest, CompleteChunkedUploadResponse, CompleteUploadRequest, FileDownloadUrlResponse, FileTypeValidation, FileUploadResponse, GetPresignedUrlRequest, InitiateChunkedUploadRequest, InitiateChunkedUploadResponse, PresignedUrlResponse, RawUploadResponse, UploadChunkRequest, UploadChunkResponse, UploadProgress } from '@objectstack/spec/api';
+import type { CompleteChunkedUploadRequest, CompleteChunkedUploadResponse, CompleteUploadRequest, FileDownloadUrlResponse, FileTypeValidation, FileUploadResponse, GetPresignedUrlRequest, InitiateChunkedUploadRequest, InitiateChunkedUploadResponse, PresignedUrlResponse, RawUploadResponse, UploadChunkRequest, UploadChunkResponse, UploadProgress } from '@objectstack/spec/api';
 
 // Validate data
 const result = CompleteChunkedUploadRequest.parse(data);
@@ -65,6 +65,20 @@ const result = CompleteChunkedUploadRequest.parse(data);
 | **eTag** | `string` | optional | S3 ETag verification |
 
 
+---
+
+## FileDownloadUrlResponse
+
+### Properties
+
+| Property | Type | Required | Description |
+| :--- | :--- | :--- | :--- |
+| **success** | `boolean` | ✅ | Operation success status |
+| **error** | `{ code: string; message: string; category?: string; details?: any; … }` | optional | Error details if success is false |
+| **meta** | `{ timestamp: string; duration?: number; requestId?: string; traceId?: string }` | optional | Response metadata |
+| **data** | `{ url: string }` | ✅ | |
+
+
 ---
 
 ## FileTypeValidation
@@ -154,6 +168,20 @@ const result = CompleteChunkedUploadRequest.parse(data);
 | **data** | `{ uploadUrl: string; downloadUrl?: string; fileId: string; method: Enum<'PUT' \| 'POST'>; … }` | ✅ | |
 
 
+---
+
+## RawUploadResponse
+
+### Properties
+
+| Property | Type | Required | Description |
+| :--- | :--- | :--- | :--- |
+| **success** | `boolean` | ✅ | Operation success status |
+| **error** | `{ code: string; message: string; category?: string; details?: any; … }` | optional | Error details if success is false |
+| **meta** | `{ timestamp: string; duration?: number; requestId?: string; traceId?: string }` | optional | Response metadata |
+| **data** | `{ key: string }` | ✅ | |
+
+
 ---
 
 ## UploadChunkRequest

packages/client/src/index.ts

@@ -2443,11 +2443,23 @@ export class ObjectStackClient {
  return completeJson;
  },
 
+ /**
+ * Resolve a committed file to a short-lived signed download URL.
+ *
+ * Read through `unwrapResponse` rather than off the raw body: the route
+ * answers the declared `{ success: true, data: { url } }` envelope as of
+ * #3689, and this SDK ships as its own npm package against servers it was
+ * not built with. `unwrapResponse` strips the envelope when it is there
+ * and hands back the body untouched when it is not, so a client on either
+ * side of that server upgrade resolves the same URL. That is the SDK's one
+ * standard envelope seam — every other enveloped method already goes
+ * through it — not a fallback grown for this route.
+ */
  getDownloadUrl: async (fileId: string): Promise<string> => {
  const route = this.getRoute('storage');
  const res = await this.fetch(`${this.baseUrl}${route}/files/${fileId}/url`);
- const data = await res.json();
- return data.url;
+ const { url } = await this.unwrapResponse<{ url: string }>(res);
+ return url;
  },
 
  /**

packages/client/src/storage-wire-dialect.test.ts

@@ -0,0 +1,138 @@
+// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
+
+/**
+ * Storage wire-dialect proof (#3689) — the SDK's storage methods against the
+ * envelope `service-storage` actually emits, and against the one it emitted
+ * before.
+ *
+ * The gap this pins: `storage.zod.ts` declared every storage response as
+ * `BaseResponseSchema.extend({ data })`, `PresignedUrlResponse` and friends are
+ * `z.infer`red from those schemas and published as these methods' return types
+ * — and the methods returned `res.json()` raw. `res.json()` is `any`, so the
+ * declaration could say `success: boolean` while the wire said nothing at all
+ * and TypeScript would never know. `getDownloadUrl` was the one method that
+ * read INTO a body, and it read `data.url` off a bare `{ url }`.
+ *
+ * #3689 moved the wire onto the declaration. This SDK ships as its own npm
+ * package, so it meets servers on both sides of that change: the enveloped and
+ * the bare body are both asserted here, and `unwrapResponse` — the client's one
+ * standard envelope seam, not a fallback grown for this route — is what spans
+ * them.
+ */
+
+import { describe, it, expect, vi } from 'vitest';
+import { ObjectStackClient } from './index';
+
+function clientReturning(body: any) {
+ const fetchMock = vi.fn().mockResolvedValue({
+ ok: true,
+ status: 200,
+ statusText: 'OK',
+ json: async () => body,
+ headers: new Headers(),
+ });
+ const client = new ObjectStackClient({ baseUrl: 'http://localhost:3000', fetch: fetchMock });
+ return { client, fetchMock };
+}
+
+describe('storage.getDownloadUrl reads the signed URL off either dialect (#3689)', () => {
+ it('resolves from the declared { success: true, data: { url } } envelope', async () => {
+ const { client, fetchMock } = clientReturning({
+ success: true,
+ data: { url: '/api/v1/storage/_local/raw/eyJrIjoi.c2ln' },
+ });
+
+ await expect(client.storage.getDownloadUrl('f1')).resolves.toBe(
+ '/api/v1/storage/_local/raw/eyJrIjoi.c2ln',
+ );
+ expect(fetchMock.mock.calls[0][0]).toContain('/api/v1/storage/files/f1/url');
+ });
+
+ it('still resolves from the bare { url } an older server answers', async () => {
+ // Not a tolerated dialect going forward — a version-skew allowance. The
+ // client is published separately from the server it talks to, so a build
+ // predating the #3689 rollout must keep resolving downloads.
+ const { client } = clientReturning({ url: 'https://bucket.s3.amazonaws.com/user/f1.png?sig=abc' });
+
+ await expect(client.storage.getDownloadUrl('f1')).resolves.toBe(
+ 'https://bucket.s3.amazonaws.com/user/f1.png?sig=abc',
+ );
+ });
+
+ it('resolves an absolute S3-style URL out of the envelope unchanged', async () => {
+ const { client } = clientReturning({
+ success: true,
+ data: { url: 'https://bucket.s3.amazonaws.com/user/f1.png?X-Amz-Signature=abc' },
+ });
+
+ await expect(client.storage.getDownloadUrl('f1')).resolves.toBe(
+ 'https://bucket.s3.amazonaws.com/user/f1.png?X-Amz-Signature=abc',
+ );
+ });
+});
+
+describe('the enveloped storage responses match their declared return types (#3689)', () => {
+ /**
+ * These methods hand back the whole envelope by design — their declared
+ * return types ARE `BaseResponseSchema.extend({ data })`. Before #3689 the
+ * `success` half of that declaration was fiction. Asserting it here is what
+ * makes the published type honest, since `res.json()` erases to `any` and
+ * the compiler cannot.
+ */
+ it('getPresignedUrl carries success alongside data', async () => {
+ const { client } = clientReturning({
+ success: true,
+ data: {
+ uploadUrl: '/api/v1/storage/_local/raw/tok',
+ method: 'PUT',
+ fileId: 'f1',
+ expiresIn: 3600,
+ downloadUrl: '/api/v1/storage/files/f1/url',
+ },
+ });
+
+ const res = await client.storage.getPresignedUrl({
+ filename: 'a.png',
+ mimeType: 'image/png',
+ size: 10,
+ scope: 'user',
+ });
+ expect(res.success).toBe(true);
+ expect(res.data.fileId).toBe('f1');
+ });
+
+ it('initChunkedUpload carries success alongside data', async () => {
+ const { client } = clientReturning({
+ success: true,
+ data: {
+ uploadId: 'up1',
+ resumeToken: 'tok',
+ fileId: 'f1',
+ totalChunks: 1,
+ chunkSize: 5242880,
+ expiresAt: '2026-01-01T00:00:00.000Z',
+ },
+ });
+
+ const res = await client.storage.initChunkedUpload({
+ filename: 'big.bin',
+ mimeType: 'application/octet-stream',
+ totalSize: 100,
+ chunkSize: 5242880,
+ scope: 'user',
+ });
+ expect(res.success).toBe(true);
+ expect(res.data.uploadId).toBe('up1');
+ });
+
+ it('uploadPart carries success alongside data', async () => {
+ const { client } = clientReturning({
+ success: true,
+ data: { chunkIndex: 0, eTag: '"abc"', bytesReceived: 100 },
+ });
+
+ const res = await client.storage.uploadPart('up1', 0, 'tok', Buffer.from('x'));
+ expect(res.success).toBe(true);
+ expect(res.data.eTag).toBe('"abc"');
+ });
+});

packages/qa/dogfood/test/attachments-permission-matrix.dogfood.test.ts

@@ -327,10 +327,14 @@ describe('attachments permission matrix (#2755)', () => {
  expect(denied.status).toBe(403);
  expect(((await denied.json()) as any).error?.code).toBe('ATTACHMENT_DOWNLOAD_DENIED');
 
- // The owner (admin) → 200 with a signed URL.
+ // The owner (admin) → 200 with a signed URL, in the declared
+ // `{ success: true, data: { url } }` envelope (#3689 — this route answered
+ // a bare `{ url }` until then).
  const owner = await stack.apiAs(adminTok, 'GET', `/storage/files/${adminFile}/url`);
  expect(owner.status).toBe(200);
- expect(((await owner.json()) as any).url).toBeTruthy();
+ const ownerBody = (await owner.json()) as any;
+ expect(ownerBody.success).toBe(true);
+ expect(ownerBody.data?.url).toBeTruthy();
 
  // Parent-inherited read: a file on the PUBLIC att_case record is
  // downloadable by any member who can read that record — even a

packages/services/service-storage/src/storage-route-ledger.ts

@@ -79,13 +79,13 @@ export const STORAGE_ROUTE_LEDGER: readonly StorageRouteLedgerEntry[] = [
 
  // ── download ──────────────────────────────────────────────────────────────
  { route: 'GET /api/v1/storage/files/:fileId/url', family: 'download', disposition: 'sdk', client: 'storage.getDownloadUrl',
- note: 'JSON { url } — the authorization-gated signed-URL resolve the dispatcher ledger points at (#3584)' },
+ note: 'JSON { success: true, data: { url } } — the authorization-gated signed-URL resolve the dispatcher ledger points at (#3584); the bare { url } it used to answer moved into the declared envelope in #3689' },
  { route: 'GET /api/v1/storage/files/:fileId', family: 'download', disposition: 'server-only',
  note: 'stable 302 to the same signed URL. This is the value objectql stamps into file/image field payloads (engine.ts buildFileValue), followed verbatim by <img src>/<a href> — a browser URL, not an SDK call. The SDK resolves via the /url sibling above.' },
 
  // ── local-driver loopback (LocalStorageAdapter presign targets) ───────────
  { route: 'PUT /api/v1/storage/_local/raw/:token', family: 'local-driver', disposition: 'server-only',
- note: 'the uploadUrl LocalStorageAdapter mints for its own presign tokens. storage.upload does PUT it, but opaquely — via fetchImpl on whatever uploadUrl came back, exactly as it would an S3 presigned URL. HMAC-token-authorized, adapter-internal, never a named SDK method.' },
+ note: 'the uploadUrl LocalStorageAdapter mints for its own presign tokens. storage.upload does PUT it, but opaquely — via fetchImpl on whatever uploadUrl came back, exactly as it would an S3 presigned URL. HMAC-token-authorized, adapter-internal, never a named SDK method. Answers { success: true, data: { key } }; the { ok: true, key } it used to answer retired in #3689, `ok` being a private second word for `success`.' },
  { route: 'GET /api/v1/storage/_local/raw/:token', family: 'local-driver', disposition: 'server-only',
  note: 'ditto for download: the target of getPresignedDownload/getSignedUrl on the local adapter, handed to the browser as an opaque signed link.' },
 ];

packages/services/service-storage/src/storage-routes.test.ts

@@ -241,7 +241,7 @@ describe('Storage REST Routes', () => {
  const urlRes = createMockRes();
  await urlHandler(urlReq, urlRes);
  expect(urlRes._status).toBe(200);
- expect(urlRes._json.url).toContain('/_local/raw/');
+ expect(urlRes._json.data.url).toContain('/_local/raw/');
  });
 
  it('should 404 for non-committed file', async () => {
@@ -308,7 +308,7 @@ describe('Storage REST Routes', () => {
  await commit(s, { id: 'a3' });
  const res = await hit(server, '/api/v1/storage/files/:fileId/url', 'a3');
  expect(res._status).toBe(200);
- expect(res._json.url).toContain('/_local/raw/');
+ expect(res._json.data.url).toContain('/_local/raw/');
  expect(authorizeFileRead).toHaveBeenCalledOnce();
  });
 
@@ -421,7 +421,11 @@ describe('Storage REST Routes', () => {
  const putRes = createMockRes();
  await putHandler(putReq, putRes);
  expect(putRes._status).toBe(200);
- expect(putRes._json.ok).toBe(true);
+ // `{ ok: true, key }` until #3689 — `ok` was a private second word for
+ // the `success` the declared envelope already carries.
+ expect(putRes._json.success).toBe(true);
+ expect(putRes._json.ok).toBeUndefined();
+ expect(putRes._json.data.key).toBe('rawtest/file.bin');
 
  // Verify file was written
  const downloaded = await adapter.download('rawtest/file.bin');