Skip to content

Commit e7be87f

Browse files
committed
fix(runtime,hono,plugin-dev): retire the /storage dispatcher bridge — it never spoke the storage contract (#4087)
`POST /api/v1/storage/upload` and `GET /api/v1/storage/file/:id` bridged to the `file-storage` slot against a service shape that does not exist: - upload called `upload(key, data, options?)` as `upload(file, { request })` — the parsed file in the `key` slot, `{ request }` in `data`. A TypeError on every implementation in the repo (S3, local, swappable, plugin-dev's memory one), not a near-miss. - download branched on `{ url | redirect | stream | mimeType }` while `download(key)` resolves a Buffer, so every branch fell through and the route answered a JSON-serialized Buffer. Nothing shadowed them — service-storage mounts `/storage/upload/presigned`, not `/storage/upload`, so both stayed mounted and reachable. What hid the defect is that they had no caller: no SDK method builds those URLs, only the docs pointed at them. The tests that covered them mocked the shape the handler wanted rather than the shape `IStorageService` declares, so the suite was green on two routes that answered 500. `/api/v1/storage` is `@objectstack/service-storage`'s surface — the presigned / chunked / signed-URL protocol `@objectstack/client` actually speaks. Removed: the `/storage` domain, `HttpDispatcher.handleStorage()`, the dispatcher-plugin mounts, the hono adapter's `/storage/*` wildcard (which claimed the whole subtree for the two dead routes), and both route-ledger rows. Without the package the path now has no handler — the same answer every uninstalled capability gives. Two follow-ons keep declared === enforced (Prime Directive #10): - discovery advertises `routes.storage` only when the slot's occupant actually mounts HTTP handlers (`handlerReady`, the predicate #4000 gave analytics); - plugin-dev's in-memory file-storage self-declares `handlerReady: false` — the retired bridge was the only thing that ever routed HTTP to it. It still works for in-process callers, it is simply no longer advertised as an HTTP surface. Docs: `plugin-endpoints` now lists the real protocol with its SDK column, and the #3563 audit records the premise #3584's storage disposition rested on. Closes #4087 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015T5CeitwV1jXLvsL1A84tw
1 parent 707dae1 commit e7be87f

17 files changed

Lines changed: 301 additions & 226 deletions
Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
---
2+
"@objectstack/runtime": minor
3+
"@objectstack/hono": minor
4+
"@objectstack/plugin-dev": patch
5+
---
6+
7+
fix(runtime,hono,plugin-dev): retire the dispatcher's `/storage` bridge — it never spoke the storage contract (#4087)
8+
9+
`POST /api/v1/storage/upload` and `GET /api/v1/storage/file/:id` were a
10+
dispatcher-side bridge to the `file-storage` service slot, written against a
11+
service shape that does not exist:
12+
13+
- **Upload** called the contract's `upload(key, data, options?)` as
14+
`upload(file, { request })` — the parsed file object landed in the `key`
15+
slot and `{ request }` in `data`. That is a `TypeError` against every
16+
implementation in the repo (`S3StorageAdapter`, `LocalStorageAdapter`,
17+
`SwappableStorageService`, plugin-dev's in-memory one), not a
18+
near-miss: `Buffer.from({}) → ERR_INVALID_ARG_TYPE`, or an object used as
19+
an S3 object key / `path.join` segment.
20+
- **Download** branched on `result.url` / `result.redirect` / `result.stream`
21+
/ `result.mimeType` while the contract's `download(key)` resolves a
22+
`Buffer`, so every branch fell through and the route answered a
23+
JSON-serialized Buffer.
24+
25+
Both routes are removed, along with `HttpDispatcher.handleStorage()`, the
26+
`/storage` domain registration, the dispatcher-plugin mounts and the two route
27+
ledger rows.
28+
29+
**Migration.** There is nothing to migrate off in practice — neither route
30+
could complete a request. (They were reachable: `service-storage` mounts
31+
`/storage/upload/presigned`, not `/storage/upload`, so nothing shadowed them.
32+
They simply had no caller — no SDK method builds those URLs.)
33+
`/api/v1/storage` is `@objectstack/service-storage`'s surface and always was
34+
the working one:
35+
36+
- Upload — FROM `POST /api/v1/storage/upload` TO the presigned protocol
37+
(`POST /storage/upload/presigned` → direct `PUT` to the returned URL →
38+
`POST /storage/upload/complete`), or `client.storage.upload(file)`, which
39+
runs all three steps.
40+
- Download — FROM `GET /api/v1/storage/file/:id` TO
41+
`GET /storage/files/:fileId/url` (`client.storage.getDownloadUrl(fileId)`)
42+
for a signed URL, or `GET /storage/files/:fileId` for a stable browser URL
43+
that 302s to it.
44+
45+
Install `@objectstack/service-storage` to get those routes; without it
46+
`/api/v1/storage` now has no handler, which is the same answer every other
47+
uninstalled capability gives.
48+
49+
Two follow-on corrections keep `declared === enforced`:
50+
51+
- `@objectstack/hono` no longer mounts `app.all('<prefix>/storage/*')`. That
52+
wildcard claimed the whole `/storage` subtree for the two dead routes, so
53+
every other path under it — service-storage's protocol above all — got the
54+
bridge's own 404 rather than falling through. Storage is ordinary catch-all
55+
traffic now.
56+
- Discovery advertises `routes.storage` only when the `file-storage` slot's
57+
occupant actually mounts HTTP handlers (`handlerReady`, the predicate #4000
58+
gave analytics), and plugin-dev's in-memory implementation now self-declares
59+
`handlerReady: false` — with the bridge gone, nothing routes HTTP to it. It
60+
keeps working for in-process callers; it is simply no longer advertised as a
61+
reachable HTTP capability.

content/docs/api/plugin-endpoints.mdx

Lines changed: 37 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -144,10 +144,43 @@ protocol `ai.chatStream` parses and owns message state for you.
144144

145145
### File Storage (`/storage`) — Plugin Required
146146

147-
| Method | Endpoint | Description |
148-
|:-------|:---------|:------------|
149-
| POST | `/storage/upload` | Upload a file |
150-
| GET | `/storage/file/:id` | Download a file |
147+
Provided by `@objectstack/service-storage`, which registers these routes on the
148+
host HTTP server. Uploads are a three-step protocol — ask for a presigned
149+
target, send the bytes straight to it, then commit — so the bytes never proxy
150+
through the API server, and the same shape works for S3, local disk, or any
151+
other adapter.
152+
153+
| Method | Endpoint | SDK | Description |
154+
|:-------|:---------|:----|:------------|
155+
| POST | `/storage/upload/presigned` | `storage.getPresignedUrl` | Step 1 — mint an upload target for a new `sys_file` row |
156+
| POST | `/storage/upload/complete` | `storage.upload` | Step 3 — commit the uploaded bytes |
157+
| POST | `/storage/upload/chunked` | `storage.initChunkedUpload` | Open a chunked / resumable session |
158+
| PUT | `/storage/upload/chunked/:uploadId/chunk/:chunkIndex` | `storage.uploadPart` | Send one chunk |
159+
| POST | `/storage/upload/chunked/:uploadId/complete` | `storage.completeChunkedUpload` | Assemble the parts |
160+
| GET | `/storage/upload/chunked/:uploadId/progress` | `storage.resumeUpload` | Read session progress (first step of a resume) |
161+
| GET | `/storage/files/:fileId/url` | `storage.getDownloadUrl` | Resolve a short-lived signed download URL |
162+
| GET | `/storage/files/:fileId` || Stable browser URL; 302s to the same signed URL (what file fields carry) |
163+
164+
`storage.upload(file)` runs steps 1–3 for you, including the direct-to-storage
165+
`PUT` in step 2.
166+
167+
<Callout type="warn">
168+
**This table used to list two routes that never worked (#4087).** `POST
169+
/storage/upload` and `GET /storage/file/:id` were a dispatcher-side bridge to
170+
the `file-storage` service, written against a service shape that does not
171+
exist: it called `upload(key, data, options?)` as `upload(file, { request })`
172+
a `TypeError` against every implementation in the repo — and read the `Buffer`
173+
that `download(key)` resolves as if it were a `{ url | stream | mimeType }`
174+
descriptor. They stayed mounted and reachable — nothing shadowed them — so
175+
anyone who followed this table got a 500; everyone who followed the SDK used
176+
the protocol above and never touched them.
177+
178+
Both routes are retired. `/storage` is service-storage's surface — install that
179+
package to get it. Discovery advertises the route only when the occupant of the
180+
`file-storage` slot actually mounts HTTP handlers, so an in-memory dev
181+
implementation now reports `handlerReady: false` and no `routes.storage`
182+
instead of pointing at a path with nothing behind it.
183+
</Callout>
151184

152185
---
153186

docs/audits/2026-07-dispatcher-client-route-coverage.md

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,37 @@ had zero call sites in `objectstack` and `objectui`.
111111
dispatcher's two plain routes remain a low-level redirect/stream compat
112112
surface, deliberately outside the SDK.
113113

114+
### Amendment (#4087) — the storage half rested on its own bad premise
115+
116+
The #3584 pass corrected one premise in this table and kept another it never
117+
checked: that the dispatcher's two storage routes were *a working low-level
118+
compat surface*. End-to-end verification found they were not, and never had
119+
been. `POST /storage/upload` called `upload(key, data, options?)` as
120+
`upload(file, { request })` — the file object landing in the `key` slot and
121+
`{ request }` in `data` — which is a `TypeError` against every implementation
122+
in the repo (`s3-storage-adapter`, `local-storage-adapter`,
123+
`swappable-storage-service`, plugin-dev's in-memory one). `GET
124+
/storage/file/:id` had the mirror-image defect one layer quieter: it branched
125+
on `result.url` / `result.redirect` / `result.stream` / `result.mimeType`
126+
while the contract's `download(key)` resolves a `Buffer`, so every branch fell
127+
through to "serialize the Buffer as JSON".
128+
129+
Neither could serve a request. What kept that hidden was not shadowing —
130+
service-storage mounts `/storage/upload/presigned`, not `/storage/upload`, so
131+
both bridge routes stayed mounted and reachable in a full stack — but the
132+
absence of any caller: no SDK method, no console call — only the
133+
`content/docs/api/plugin-endpoints` table advertising them to readers. The
134+
tests that covered them mocked the shape the handler wanted rather than the
135+
shape `IStorageService` declares, so the suite stayed green on a pair of routes
136+
that answered 500.
137+
138+
**Both routes are retired** (#4087) — the `/storage` domain, its
139+
dispatcher-plugin mounts, the hono adapter's `/storage/*` wildcard (which had
140+
been claiming the whole subtree for them) and the two ledger rows are gone.
141+
Read the two storage rows in §4 as retired rather than as the `server-only`
142+
disposition #3584 gave them: the surface belongs entirely to `service-storage`,
143+
enumerated in `packages/services/service-storage/src/storage-route-ledger.ts`.
144+
114145
## 5. Client-internal findings
115146

116147
- **`trigger` vs `execute`** hit different URLs for the same intent

packages/adapters/hono/src/__mocks__/runtime.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ export class HttpDispatcher {
77
handleAuth = vi.fn().mockResolvedValue({ handled: true, response: { status: 200, body: { ok: true } } });
88
handleMetadata = vi.fn().mockResolvedValue({ handled: true, response: { status: 200, body: { objects: [] } } });
99
handleData = vi.fn().mockResolvedValue({ handled: true, response: { status: 200, body: { records: [] } } });
10-
handleStorage = vi.fn().mockResolvedValue({ handled: true, response: { status: 200, body: {} } });
1110

1211
constructor(_kernel: any) {}
1312
}

packages/adapters/hono/src/hono.test.ts

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@ const mockDispatcher = {
88
getDiscoveryInfo: vi.fn().mockReturnValue({ version: '1.0', endpoints: [] }),
99
handleAuth: vi.fn().mockResolvedValue({ handled: true, response: { body: { ok: true }, status: 200 } }),
1010
handleGraphQL: vi.fn().mockResolvedValue({ data: {} }),
11-
handleStorage: vi.fn().mockResolvedValue({ handled: true, response: { body: {}, status: 200 } }),
1211
dispatch: vi.fn().mockResolvedValue({ handled: true, response: { body: { success: true }, status: 200 } }),
1312
};
1413

@@ -486,6 +485,29 @@ describe('createHonoApp', () => {
486485
'/api',
487486
);
488487
});
488+
489+
// [#4087] The retired `/storage/*` mount was a WILDCARD: it claimed the
490+
// whole subtree for two dispatcher routes, so service-storage's protocol
491+
// (`/storage/upload/presigned`, `/storage/files/:id/url`, …) — registered
492+
// under the same prefix on the same server — could never be reached
493+
// through this app. Storage is ordinary catch-all traffic now; nothing
494+
// here parses formData or shadows the subtree.
495+
it('POST /api/storage/upload/presigned reaches the catch-all, not a storage mount', async () => {
496+
const res = await app.request('/api/storage/upload/presigned', {
497+
method: 'POST',
498+
body: JSON.stringify({ filename: 'a.txt' }),
499+
headers: { 'Content-Type': 'application/json' },
500+
});
501+
expect(res.status).toBe(200);
502+
expect(mockDispatcher.dispatch).toHaveBeenCalledWith(
503+
'POST',
504+
'/storage/upload/presigned',
505+
{ filename: 'a.txt' },
506+
expect.any(Object),
507+
expect.objectContaining({ request: expect.anything() }),
508+
'/api',
509+
);
510+
});
489511
});
490512

491513
describe('Error Handling', () => {

packages/adapters/hono/src/index.ts

Lines changed: 14 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -96,8 +96,8 @@ export function objectStackMiddleware(kernel: ObjectKernel) {
9696
/**
9797
* Creates a full-featured Hono app with all ObjectStack route dispatchers.
9898
*
99-
* Only routes that need framework-specific handling (auth service, storage
100-
* formData, GraphQL raw result, discovery wrapper) are registered explicitly.
99+
* Only routes that need framework-specific handling (auth service, GraphQL
100+
* raw result, discovery wrapper) are registered explicitly.
101101
* All other routes (meta, data, packages, analytics, automation, i18n, ui,
102102
* openapi, custom endpoints, and any future routes) are handled by a
103103
* catch-all that delegates to `HttpDispatcher.dispatch()`.
@@ -347,24 +347,18 @@ export function createHonoApp(options: ObjectStackHonoOptions): Hono {
347347
}
348348
});
349349

350-
// --- Storage (needs formData parsing) ---
351-
app.all(`${prefix}/storage/*`, async (c) => {
352-
try {
353-
const subPath = c.req.path.substring(`${prefix}/storage`.length);
354-
const method = c.req.method;
355-
356-
let file: any = undefined;
357-
if (method === 'POST' && subPath === '/upload') {
358-
const formData = await c.req.formData();
359-
file = formData.get('file');
360-
}
361-
362-
const result = await dispatcher.handleStorage(subPath, method, file, { request: c.req.raw });
363-
return toResponse(c, result);
364-
} catch (err: any) {
365-
return errorJson(c, err.message || 'Internal Server Error', err.statusCode || 500);
366-
}
367-
});
350+
// --- Storage: deliberately NOT mounted (#4087) ---
351+
// This used to be `app.all(prefix + '/storage/*')` feeding
352+
// `dispatcher.handleStorage`. Two things were wrong with it. The handler it
353+
// called spoke a storage contract that does not exist (it passed the parsed
354+
// file as the `key` argument of `upload(key, data, options?)`, and read the
355+
// Buffer that `download(key)` resolves as a `{ url | stream }` descriptor) —
356+
// retired with this change. And the wildcard was wider than the two routes
357+
// it served: it claimed the WHOLE `/storage` subtree, so every other path
358+
// under it — the presigned / chunked / signed-URL protocol `service-storage`
359+
// registers, above all — was answered by the bridge's own 404 instead of
360+
// falling through to anything else this app might mount. Storage is ordinary
361+
// catch-all traffic now, like every other domain.
368362

369363
// ─── Catch-all: delegate to dispatcher.dispatch() ─────────────────────────
370364
// Handles meta, data, packages, analytics, automation, i18n, ui, openapi,

packages/plugins/plugin-dev/src/dev-plugin.test.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -275,7 +275,11 @@ describe('DevPlugin', () => {
275275

276276
// No HTTP/WS surface is mounted for these, so no handler can be ready —
277277
// independent of the implementation being real (ADR-0076 D12, realtime).
278-
for (const name of ['cache', 'queue', 'job', 'realtime']) {
278+
// `file-storage` joined them in #4087: the dispatcher's `/storage` bridge
279+
// was the only thing that ever routed HTTP to this slot, and it was
280+
// retired — `@objectstack/service-storage` owns that surface and mounts
281+
// its own routes against its own adapters, never this implementation.
282+
for (const name of ['cache', 'queue', 'job', 'realtime', 'file-storage']) {
279283
expect(readServiceSelfInfo(registeredServices.get(name))?.handlerReady, `${name} handlerReady`).toBe(false);
280284
}
281285

packages/plugins/plugin-dev/src/dev-plugin.ts

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -295,14 +295,18 @@ const DEV_STUB_FACTORIES: Record<string, () => Record<string, any>> = {
295295
*
296296
* `handlerReady` answers a different question: does an HTTP handler genuinely
297297
* serve this? It stays `false` for every `stub`, and also for `degraded`
298-
* services with no HTTP surface at all (`realtime` — the D12 precedent).
298+
* services with no HTTP surface at all (`realtime` — the D12 precedent, joined
299+
* by `file-storage` in #4087: the dispatcher's `/storage` bridge — the one
300+
* thing that ever routed HTTP to this slot — was retired, and the surface it
301+
* claimed belongs to `@objectstack/service-storage`, which mounts its own
302+
* routes and does not use this implementation).
299303
*
300304
* Entries are only needed for plugin-dev's OWN fakes. The wrapped kernel
301305
* fallbacks (`cache` / `queue` / `job` / `i18n`) already carry their own
302306
* `__serviceInfo`, and {@link applySelfInfo} never overwrites one.
303307
*/
304308
const DEV_STUB_SELF_INFO: Record<string, { status: 'stub' | 'degraded'; handlerReady?: boolean; message: string }> = {
305-
'file-storage': { status: 'degraded', message: 'Dev in-memory file storage — really stores and returns bytes, but nothing survives a restart. Register a storage plugin for durable files.' },
309+
'file-storage': { status: 'degraded', handlerReady: false, message: 'Dev in-memory file storage — really stores and returns bytes to in-process callers, but nothing survives a restart and no HTTP surface is mounted. Register @objectstack/service-storage for durable files and the upload/download protocol.' },
306310
'search': { status: 'degraded', message: 'Dev in-memory index — real substring matching over indexed documents, no ranking, analyzers, or persistence. Register a search plugin for the real engine.' },
307311
'metadata': { status: 'degraded', message: 'Dev in-memory metadata registry — real reads and writes, no persistence. Register MetadataPlugin for a persisted registry.' },
308312
'workflow': { status: 'degraded', message: 'Dev in-memory workflow state — transitions are recorded and read back, but NOTHING is validated: every transition is accepted and availableTransitions is always empty.' },

packages/runtime/src/dispatcher-plugin.error-envelope.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
* #3867 — the dispatcher-plugin error exit.
55
*
66
* `errorResponseBase` is the single error exit for EVERY route this plugin
7-
* mounts (`/analytics`, `/packages`, `/i18n`, `/storage`, `/automation`,
7+
* mounts (`/analytics`, `/packages`, `/i18n`, `/automation`,
88
* `/auth`, `/notifications`, `/mcp`, …): each handler catches and calls it
99
* rather than re-throwing. Two defects lived there, and neither is visible
1010
* until a real error actually reaches it:

0 commit comments

Comments
 (0)