fix(service-storage): emit the declared success envelope on all eight routes (#3689) - #3837
Merged
Merged
Conversation
… routes (#3689) The other half of #3675. That PR 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 carrying the `success` flag `BaseResponseSchema` declares and `ObjectStackClient.unwrapResponse` keys on — the six upload routes { 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.inferred 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 the compiler 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. The payload MOVES on the last two, which is why this was filed separately rather than smuggled into #3675. Every in-repo consumer was fixed first, so the 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 — and the console's two attachment openers already read both `url` shapes (objectui follow-up pins that). `ok` is dropped rather than kept beside `success`: it was a second, private word for the same thing. 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 `_local/raw/:token` pair stays out of the registry on purpose: it is 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 its error twin holds the other: 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 single `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. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RmnSQC8KxxsoZQe3oDEHjG
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
github-actions
Bot
added
documentation
Contributor
📓 Docs Drift CheckThis PR changes 4 package(s): 111 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
|
os-zhuang
marked this pull request as ready for review
July 28, 2026 09:16
This was referenced Jul 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #3689 — the other half of #3675.
Takes option 1 from the issue (normalize all three shapes), which is what the issue recommends and what
packages/specbeing the source of truth requires: the SDK's declared return types are inferred from these schemas, so leaving two routes as "declared exceptions" (option 2) or rewriting the schemas to match the drift (option 3) both keep a surface where the contract and the wire disagree.The drift
/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 } }storage.zod.tsdeclared every one of these asBaseResponseSchema.extend({ data }), andPresignedUrlResponseand friends arez.inferred from those schemas and published as the SDK's return types. The declaration saidsuccess: boolean; the wire said nothing. It broke nothing only because the storage SDK methods returnedres.json()raw —any, so the compiler 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.Why it needed its own PR
The payload moves on the last two, so this is not the find-replace the six upload routes would have been. Every in-repo consumer was fixed first, so the two repos are not coupled by merge order — the same approach the error path took:
client.storage.getDownloadUrl()now reads throughunwrapResponse, the SDK's one standard envelope seam — it 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. Deliberately not a bespoke??grown for this route (PD Add comprehensive test suite for Zod schema validation #12); every other enveloped SDK method already goes through it. The remaining storage methods hand back the whole envelope by design and were already correct.RecordAttachmentsPanel,ApprovalsInboxPage) already readbody?.url ?? body?.data?.url. objectui#2913 pins that as deliberate rather than incidental — it is tests and comments only, nothing to fix there.attachments-permission-matrix.dogfood.test.tsread the bare.urland is updated.okis dropped, not kept besidesuccess— it was a second, private word for the same thing, on a route mounted under/api/v1/storagelike any other. Nothing reads that body: the SDK and the console both PUT there opaquely, exactly as they would an S3 presigned URL, and check only the status.Two schemas that never existed
FileDownloadUrlResponseandRawUploadResponseare now declared, andgetDownloadUrljoinsStorageApiContracts— it had never been in the registry despite being ledgereddisposition: 'sdk', and that absence is how its shape drifted outside the envelope unnoticed. The_local/raw/:tokenpair stays out of the registry on purpose: it is the local adapter's own presign loopback, ledgeredserver-onlyand addressed as an opaque signed URL rather than as an API, so it gets a declared shape without being promoted to a client contract.The guard
success-envelope.conformance.test.tsholds the new shape in place the wayerror-envelope.conformance.test.tsholds the error one — same harness, same imported spec, three directions:url/ok/key), so a revert cannot pass quietly;.json(call sites, onesuccess: true, onesuccess: false, nook: true— so a new route cannot bypass the singlesendOkhelper. Without (3) the suite would only ever cover the routes that existed the day it was written.Mutation-checked: reverting one route to
res.json({ url })fails 3 of its tests.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.
Not in scope
The two items the issue flags as related, not the same are untouched and still open questions: the dispatcher putting the HTTP status number in
error.code, and theStandardErrorCodesnake_case-vs-SCREAMING_SNAKE vocabulary split. Both are spec decisions, not envelope shape.Verification
service-storage222 ✅ (incl. the new 10-case suite) ·client179 ✅ ·spec6752 ✅ ·runtime665 ✅check:docs,check:api-surface,check:spec-changes,check:upgrade-guide✅ —storage.mdx,api-surface.jsonand the schema manifest regenerated and committed🤖 Generated with Claude Code
https://claude.ai/code/session_01RmnSQC8KxxsoZQe3oDEHjG
Generated by Claude Code