Skip to content

service-storage success bodies are three shapes, none carrying success: true — the other half of #3675 #3689

Description

@os-zhuang

Found while fixing the error envelope in #3675 (PR #3687), and deliberately left out of it because — unlike the error fix and unlike #3636's success fix — this one is not additive.

The drift

storage-routes.ts returns three different success shapes, none of which carries the success flag BaseResponseSchema declares and ObjectStackClient.unwrapResponse keys on:

Route(s) Body
/upload/presigned, /upload/complete, /upload/chunked, …/chunk/:i, …/complete, …/progress { data: {…} }
GET /files/:fileId/url { url } — bare, no data
PUT /_local/raw/:token { ok: true, key }

Meanwhile storage.zod.ts declares every one of these as BaseResponseSchema.extend({ data: … }), i.e. { success: boolean, data: {…} }. PresignedUrlResponse and friends are z.inferred from those schemas and used as the SDK's declared return types, so the declaration says success: boolean and the wire says nothing.

Why it is not currently breaking

Checked before filing: the storage SDK methods return res.json() raw — they do not call unwrapResponse — so they hand back exactly what the server sent and nothing throws. TypeScript cannot catch it because res.json() is any. The one method that reads into the body, getDownloadUrl, does (await res.json()).url and matches the bare { url } shape, so it works too.

In other words this is a live declared ≠ actual gap that happens to be harmless because the SDK never relies on the declaration. That is the same posture i18n was in before #3636 — right up until something did rely on it.

Why it is not additive

Adding success: true to the { data } routes is additive and safe (callers destructure .data; adding a sibling key changes nothing). The other two are not:

  • { url }{ success: true, data: { url } } moves the payload and breaks getDownloadUrl plus objectui's body?.url ?? body?.data?.url reader.
  • { ok: true, key } → likewise, and ok overlaps in meaning with success, so it is a naming decision as much as a shape one.

So it needs a deliberate pass with the consumers in hand, not a find-replace.

What needs deciding

  1. Normalize all three to { success: true, data } and update getDownloadUrl + the objectui reader. Correct per the contract; needs the console change to ship tolerantly (read both) so the two repos are not coupled by merge order — the same approach objectui#2869 used for the error path.
  2. Add success: true only where it is additive, leaving { url } and { ok, key } as declared exceptions. Cheap and safe, but the storage.zod.ts schemas would still be lying about two routes.
  3. Fix the schemas instead — declare what the routes actually return. Honest, but gives up the shared envelope for this surface.

Recommendation: option 1, since packages/spec is the source of truth and the SDK's declared return types are generated from it. Worth doing together with whatever addresses the code vocabulary split noted below.

Related, not the same

Guard note

error-envelope.conformance.test.ts (added in #3687) covers the error path only. Extending it to the success path is straightforward — same harness, same imported BaseResponseSchema — and should land with whichever option is chosen, otherwise the new shape has nothing holding it in place.

As with #3675: the route ledgers cannot catch this. They audit which routes exist and whether the SDK can address them, not what comes back.

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions