From 521b6fbeac4653ea1fee971f148e5ec32f9b63ff Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 13:30:22 +0000 Subject: [PATCH 1/2] refactor(spec)!: retire IStorageService.list(prefix) (#5540) One contract method, two adapter dialects, both silently incomplete, and no caller. ADR-0049 enforce-or-remove; maintainer ruling 2026-08-05 on #5266. Same disposition as IDataDriver.findStream (#4484): a TS/API contract that code IMPLEMENTS and nothing ever .parse()s, so there is no tombstone and no D2 source rewrite -- tsc is the channel and it reports at the call site. The retirement is registered as the ADR-0087 D3 semantic entry `storage-service-list-retired` in the protocol-17 chain step. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_011M7UwH25Unfi73UHim7ajY --- .changeset/storage-service-list-retired.md | 78 +++++++++++++++++++ .../docs/kernel/contracts/storage-service.mdx | 41 +++++----- .../runtime-services/storage-service.mdx | 11 ++- docs/protocol-upgrade-guide.md | 5 ++ packages/spec/spec-changes.json | 14 ++++ .../src/contracts/storage-service.test.ts | 52 +++++++++---- .../spec/src/contracts/storage-service.ts | 22 ++++-- packages/spec/src/migrations/registry.ts | 63 +++++++++++++++ 8 files changed, 243 insertions(+), 43 deletions(-) create mode 100644 .changeset/storage-service-list-retired.md diff --git a/.changeset/storage-service-list-retired.md b/.changeset/storage-service-list-retired.md new file mode 100644 index 0000000000..af9e34611d --- /dev/null +++ b/.changeset/storage-service-list-retired.md @@ -0,0 +1,78 @@ +--- +"@objectstack/spec": major +--- + +refactor(spec)!: retire `IStorageService.list(prefix)` — one contract method, two adapter dialects, both silently incomplete, and no caller (#5540, ADR-0049 enforce-or-remove) + +`list?(prefix)` was an optional method on the storage contract, documented as + +> List files in a directory/prefix + +and the two shipped adapters answered the same call with two different meanings. +Neither told you. + +**The local adapter listed one level and counted directories as files.** +`LocalStorageAdapter.list` was a plain `readdir` over the prefix directory, so a +nested key `a/b/c` was invisible under `list('a')` — you got `a/b` — and every +subdirectory that `stat` succeeded on was pushed into the result as if it were a +file, producing a `StorageFileInfo` whose `size` is a directory inode and which +`download()` cannot fetch. + +**The S3 adapter recursed and stopped at 1000.** `S3StorageAdapter.list` issued +one `ListObjectsV2` with the prefix — which matches the whole key, so it is +recursive, not one level — and read neither `IsTruncated` nor +`ContinuationToken`. Past 1000 objects, the "all files under this prefix" a +caller received was the first page, with nothing to distinguish it from a +complete answer. + +So the same call was one-level-plus-junk on one deployment and +recursive-but-truncated on the other, and the first feature that genuinely +needed to enumerate a prefix — backup, orphan sweep, migration audit — would +have got two different wrong answers and no error on either. + +**Nothing called it.** The only call site in the repository was the +`SwappableStorageService` pass-through, which rejects anyway when the active +adapter has no `list`. REST, the CLI and the storage routes never called it. +#5172 came closest: it planned to reclaim email-attachment content by listing +`EMAIL_ATTACHMENT_KEY_PREFIX`, discovered the local adapter could not see one +level down, and switched to queue-driven deferred work — the divergence cost a +design, and the method still had no consumer afterwards. + +**Migration.** + +| Wrote | Write instead | +| --- | --- | +| `await storage.list('attachments/task/')` | query the records you wrote — `sys_file` / file-reference rows carry the storage key and page deterministically through ObjectQL | +| `list?(prefix) { … }` on your own adapter | delete the method (see below) | +| `if (typeof storage.list === 'function')` capability probe | delete the branch; the contract has no `list` to probe for | + +Querying your own records is not a workaround for the missing method — it is the +only form that was ever correct across both backends and past 1000 objects. The +bucket was never the system of record for "which files exist"; the rows are. + +**Adapter authors: nothing breaks on you.** An implementation left in place still +compiles — an extra method is not an error on a class — it is simply unreachable +through the contract, so deleting it is cleanup you can do whenever. The break is +on the **caller** side: `storage.list(...)` no longer type-checks. That includes a +*proxy* typed against `IStorageService` that forwards to `inner.list`; the one in +`@objectstack/service-storage` is removed with the adapters in #5541. + +**No tombstone, deliberately.** `IStorageService` is a contract that code +*implements*; nothing anywhere runs a storage adapter through a `.parse()`, so a +`retiredKey()` prescription would have no one to reach. The channel that can +carry it is `tsc`, and `tsc` reports it where it is actionable — at the call +site. This is the same disposition, for the same reason, as +`IDataDriver.findStream` (#4484). The retirement is registered as the +`storage-service-list-retired` semantic entry in the protocol-17 chain step +(ADR-0087 D3), so `spec-changes.json`, the generated upgrade guide and the +`spec_changes` MCP tool all carry it. There is no `os migrate meta` step: an +adapter is code, never stack metadata, so the chain has no source to rewrite. + +**No replacement, on purpose.** A prefix listing that cannot paginate is the +wrong signature to inherit. If a first-party caller ever needs real bucket +enumeration it comes back cursor-shaped — `list(prefix, { cursor, limit })` +returning a page plus a continuation token — with adapter-conformance cases +(nested keys, directory entries, more than 1000 objects) proving both backends +agree before either ships. Maintainer ruling 2026-08-05 on #5266 chose this over +aligning the two adapters, which would have grown a conformance surface nobody +walks. diff --git a/content/docs/kernel/contracts/storage-service.mdx b/content/docs/kernel/contracts/storage-service.mdx index 4c16edf9a0..ce69874759 100644 --- a/content/docs/kernel/contracts/storage-service.mdx +++ b/content/docs/kernel/contracts/storage-service.mdx @@ -25,9 +25,6 @@ export interface IStorageService { exists(key: string): Promise; getInfo(key: string): Promise; - // Listing (optional — adapter may not implement) - list?(prefix: string): Promise; - // Signed URL (optional) getSignedUrl?(key: string, expiresIn: number, options?: PresignedDownloadOptions): Promise; @@ -55,8 +52,8 @@ export interface IStorageService { ``` -Only `upload`, `download`, `delete`, `exists`, and `getInfo` are required. Listing, -signed/presigned URLs, and chunked upload are optional — call them only after checking +Only `upload`, `download`, `delete`, `exists`, and `getInfo` are required. +Signed/presigned URLs and chunked upload are optional — call them only after checking the method exists on the active adapter (the local and S3 adapters implement them). @@ -172,22 +169,22 @@ export interface StorageUploadOptions { ## Listing Files -### list - -Lists files under a key prefix. This method is optional — verify the active -adapter implements it before calling. It takes a single `prefix` argument -(no pagination options) and returns `StorageFileInfo[]`. - -```typescript -// List all attachments for a task -const files = await storageService.list?.( - 'attachments/tasks/tsk_01HQ4A7B/' -) ?? []; - -for (const file of files) { - console.log(`${file.key} (${file.size} bytes)`); -} -``` + +**`list(prefix)` was removed** in `@objectstack/spec` 5.x — the contract has no +prefix-enumeration method. It was declared but never consumed, and the two shipped +adapters answered it differently while both silently returned an incomplete answer: +the local adapter listed a single level and reported directories as files, the S3 +adapter recursed and stopped at 1000 objects without reading `IsTruncated` / +`ContinuationToken`. + +There is no drop-in replacement, deliberately — a prefix listing that cannot paginate +is the wrong shape to keep. If you were enumerating a prefix, track the keys you wrote +(the `sys_file` / file-reference records already do this, and are queryable through +ObjectQL with real pagination) instead of asking the bucket. When a first-party caller +genuinely needs bucket enumeration, it returns cursor-shaped — +`list(prefix, { cursor, limit })` — with adapter-conformance cases proving both +backends agree. + --- @@ -273,7 +270,7 @@ const key = await storageService.completeChunkedUpload?.(uploadId, [ ## StorageFileInfo -The return type for file metadata (`getInfo`, `list`). +The return type for file metadata (`getInfo`). {/* os:check */} ```typescript diff --git a/content/docs/kernel/runtime-services/storage-service.mdx b/content/docs/kernel/runtime-services/storage-service.mdx index 0a78a2ce25..6c71dfb783 100644 --- a/content/docs/kernel/runtime-services/storage-service.mdx +++ b/content/docs/kernel/runtime-services/storage-service.mdx @@ -16,7 +16,6 @@ services.storage.download(key: string): Promise services.storage.delete(key: string): Promise services.storage.exists(key: string): Promise services.storage.getInfo(key: string): Promise -services.storage.list?(prefix: string): Promise services.storage.getSignedUrl?(key: string, expiresIn: number, options?: PresignedDownloadOptions): Promise ``` @@ -31,6 +30,16 @@ services.storage.completeChunkedUpload?(uploadId: string, parts: Array<{ partNum services.storage.abortChunkedUpload?(uploadId: string): Promise ``` +## Removed + +`services.storage.list?(prefix)` was removed in `@objectstack/spec` 5.x — the contract +has no prefix-enumeration method. It had no consumer, and the two shipped adapters gave +the same call two different, silently-incomplete answers (local: one level, directories +reported as files; S3: recursive, truncated at 1000 objects). Query the file records you +wrote rather than the bucket; a future enumeration returns cursor-shaped +(`list(prefix, { cursor, limit })`). See the +[contract reference](/docs/kernel/contracts/storage-service). + ## Typical Errors Storage methods reject with a plain `Error` carrying a descriptive message — the diff --git a/docs/protocol-upgrade-guide.md b/docs/protocol-upgrade-guide.md index 1e29960521..ca9e0d30bf 100644 --- a/docs/protocol-upgrade-guide.md +++ b/docs/protocol-upgrade-guide.md @@ -208,6 +208,8 @@ The same is true of the protocol-17 retirement that closes this list, and the pa The last enforce-or-remove entry of this step is on the RUNTIME context rather than on anything authorable: `HookContext.session.roles` (#5050). It was declared in `data/hook.zod.ts`, read by exactly two consumers — the approvals record lock and the delegation write guard, each opening with `session.roles?.includes('admin')` — and produced by nobody on the hook path: ObjectQL's `buildSession()` writes the session field by field (`userId`, `organizationId`, `accessToken`, `isSystem`, `actor`, the skip flags) and has no `roles` write, here or in `cloud`, whose hook consumers read `hookContext?.session?.userId` and nothing else (an ACTION body's `ctx.session` is a different untyped object that does carry one, tracked apart). So both branches were dead on every real engine path: an authorization decision in shape only, and — worse for a reader — a SECOND admin dialect competing with the one ADR-0095 D3 sanctions. #4839 (PR #5049) deleted the two readers on the maintainer's ruling; this step removes the declaration that outlived them, which is what ADR-0049 asks for once a key has neither end. Nothing observable changes: a key nobody wrote and nothing read cannot alter a single decision. It is tombstoned rather than deleted because `HookContextSchema` is deliberately NOT `.strict()` (strictness there would make an engine-internal enrichment a breaking change for anyone parsing a context they were handed, as `provenance` was in #3712), so a plain delete would strip the key in silence — the #3733 / ADR-0104 failure this whole pass exists to end. There is NO conversion and no source rewrite: a HookContext is built per operation by the engine and never stored, so no `sys_metadata` row, example or template can carry the key — the `openApi31` / `activationEvents` shape, one semantic TODO for hook authors. The live vocabulary is untouched and deliberately elsewhere: gate on `session.userId` / `session.isSystem` in the hook, and judge PRIVILEGE through the security service, which reads capability grants (`permissions`), placements (`positions`) and the derived posture off the execution context. +The same enforce-or-remove reading reaches the storage contract: `IStorageService.list(prefix)` is removed (#5540, analysis #5266). It had no consumer — the only in-repo call site was a proxy pass-through — and the two shipped adapters answered it with two different, silently incomplete semantics: the local adapter listed a single level and reported directories as files, the S3 adapter recursed and stopped at 1000 objects without reading `IsTruncated` / `ContinuationToken`. Enumerating a prefix without a cursor is the wrong signature to inherit, so nothing replaces it in place: query the file records you wrote, and let a real caller bring back a cursor-shaped `list(prefix, { cursor, limit })` with adapter-conformance cases behind it. Same shape and same disposition as the `findStream` retirement above — a TS/API contract, no stored source, no tombstone, tsc at the call site. + Finally it retires the two inert `IndexSchema` keys, `indexes[].type` and `indexes[].partial` (#5248, #4943). Neither ever had a DDL consumer: `SqlDriver.syncDeclaredIndexes` creates declared indexes through knex's `table.index()` / `table.unique()`, and the drift differ's `DeclaredIndexInput` carries only `name`/`fields`/`unique`/`nullSafeColumns` — so an authored `type` selected no access method and an authored `partial` produced a FULL index with its predicate discarded. `partial` was the more damaging of the two because it read as a correctness control: the platform's own `sys_metadata` declared it for overlay uniqueness, and what the declaration alone materialized was an unrestricted unique index (the active-row scoping is delivered by a runtime migration, `metadata-protocol`'s `ensureOverlayIndex`, not by the key). `type` was the louder: its `.default('btree')` put an inert knob into every parse output, so it read as live configuration — the ADR-0078 no-silently-inert shape. Remove was chosen over enforce (maintainer ruling, 2026-08-06): enforcing needs per-dialect algorithm mapping (`gin`/`gist` Postgres-only, `fulltext` MySQL-family), raw-SQL `CREATE INDEX … WHERE` on the dialects that have partial indexes at all (MySQL does not), and a redesign of how `isSyncReproducibleIndex` excludes partial indexes from incremental sync — design cost for a capability nothing has asked for. Both are lossless deletes: no DDL changes, because no DDL ever depended on them. Drift detection is untouched — the `partial` flag it consumes is parsed back out of the database's OWN `CREATE INDEX` DDL and never came from this key. ### Mechanical (applied for you) @@ -342,6 +344,9 @@ Finally it retires the two inert `IndexSchema` keys, `indexes[].type` and `index - **`action-session-roles-to-positions`** — `ui.actionSession.roles` → ui.actionSession.positions (an action body reads `ctx.session.positions`) - Why not automatic: The MIRROR-IMAGE neighbour of the entry above, and the reason both are in this step: the hook `ctx.session` carried `roles` declared-and-never-produced (removed outright, #5050), while the ACTION body's `ctx.session` carries it produced-and-really-populated. `buildActionSession()` (`packages/runtime/src/action-execution.ts`) copies `ExecutionContext.positions` into a key spelled `roles` — the ADR-0090 D3 vocabulary handed to the author under the one spelling that ADR bans — so a body author met two different answers to one key name on one platform: rejected in a hook, live and full of values in an action. #5613 ruled contract-first (maintainer, 2026-08-06: "C skeleton + A semantics"): phase 1 (#5697) declared the previously undeclared shape as `ActionSessionSchema`, and phase 2 renames the key. `positions` is now the canonical key on that schema and `roles` a deprecated alias of it (#5779); the producer emits both for one deprecation window (#5613 runtime half), after which `roles` is removed on the path the v11 session-alias removal already walked (#3280 deprecated → #3290 removed). Why this is a D3 semantic TODO and not a D2 conversion, on two independent grounds: FIRST, there is no source to convert — an action `ctx.session` is constructed per dispatch and never persisted, so no `sys_metadata` row, example or template can carry the key — the `openApi31` (#4579) / `activationEvents` (#4657) / `hook-context-session-roles-retired` (#5050) shape. SECOND, the only place the key is ever SPELLED is inside an action body: author-written JS/TS, or a sandboxed script whose `ScriptContext.session` is still `unknown`. A declarative transform cannot safely rewrite an identifier inside free-form code — exactly the reason the ADR-0090 wave delegated `current_user.roles` to the author at step 13 (`cel-current-user-roles-to-positions`) instead of substituting text. Note what is deliberately NOT done here: the alias is not tombstoned. A `retiredKey()` REJECTS the key, and a deprecation window exists precisely so the old spelling keeps working while its readers move — tombstoning during the window would be the removal it is meant to defer. The tombstone (or the plain deletion the authorable-surface ratchet adjudicates) belongs to the release that closes the window. Until then this entry IS the channel: `spec-changes.json` and the generated upgrade guide are how a reader learns the rename before the removal reaches them. ADR-0090 D3, ADR-0087, #5613 / #5779. - Done when: No action body reads `ctx.session.roles`; every such read is `ctx.session.positions` and observes the same array (the rename is a rename — the VALUE is `ExecutionContext.positions` on both sides, which the runtime pin `action-session-shape-contract.test.ts` asserts independently of the key name). Privilege is NOT re-derived from either spelling: a read that was `roles.includes('admin')` as an access check is rewritten to ask the security service (capability grants / placements / derived posture, ADR-0095), never renamed to `positions.includes('admin')` — renaming that read migrates the defect rather than the code. Verify against a real dispatch, not a fixture: invoke an action as a caller holding positions and assert the body observed them under the canonical key. During the window both keys are present and equal, so a reader can be migrated and verified before the alias is removed; after it, `roles` is absent and a body still reading it sees `undefined` — which is why the read must be moved inside the window rather than at its close. +- **`storage-service-list-retired`** — `contracts.IStorageService.list` → no replacement — track the keys you wrote (sys_file / file-reference records, queryable through ObjectQL with real pagination) instead of enumerating the bucket + - Why not automatic: `list(prefix)` was an OPTIONAL contract method documented as "List files in a directory/prefix", and the two shipped adapters answered the same call with two different semantics — both of them silently incomplete. `LocalStorageAdapter.list` was a single-level `readdir`, so a nested key `a/b/c` was invisible under `list('a')` (only `a/b` came back), and a subdirectory that `stat` succeeded on was pushed into the result as a file, yielding a `StorageFileInfo` whose `size` is a directory inode and which cannot be downloaded at all. `S3StorageAdapter.list` was RECURSIVE (`ListObjectsV2` matches the whole key) and read neither `IsTruncated` nor `ContinuationToken`, so past 1000 objects the "all files" a caller received was the first page, with no signal. One contract method, two dialects, both quietly incomplete — and the first feature that genuinely needed to enumerate a prefix (backup, orphan sweep, migration audit) would have got two different answers on two deployments without an error on either. #5172 was nearly that feature: it planned to drive attachment reclamation off `list(EMAIL_ATTACHMENT_KEY_PREFIX)`, found the local adapter could not see one level down, and switched to queue-driven deferred work instead. Nothing consumed it afterwards: the only in-repo call site was the `SwappableStorageService` pass-through (which itself rejects when the active adapter has no `list`), and REST, CLI and the storage routes never called it. Remove was chosen over align-and-tighten (maintainer ruling, 2026-08-05, #5266): aligning would grow a conformance surface nobody walks, while a prefix listing that cannot paginate is the wrong signature to inherit — when a real caller needs enumeration it returns cursor-shaped, `list(prefix, { cursor, limit })`, with adapter-conformance cases (nested keys, directory entries, >1000 objects) proving both backends agree. This is a TS/API contract surface — a storage adapter is CODE, never stack metadata — so there is no source for the chain to rewrite, and deliberately no schema tombstone: nothing ever ran an adapter through a `.parse()`, so a prescription there would reach no one. The enforced channel is tsc, and it reports at the call site. Same disposition, and the same reason, as `data-driver-find-stream-retired` (#4484). ADR-0049 / ADR-0087, #5540 (analysis #5266). + - Done when: No code calls `storage.list(...)` on the `file-storage` service or on any `IStorageService` value. Code that needed "which files are under this prefix" reads the records it wrote — `sys_file` / file-reference rows carry the storage key and page deterministically through ObjectQL — rather than asking the bucket, which is also the only form that stays correct past 1000 objects and across both adapters. An adapter that still IMPLEMENTS `list` keeps compiling (an extra method is not an error on a class) and is simply unreachable through the contract, so deleting it is cleanup that can follow. The break is on the CALLER side: `storage.list(...)` no longer type-checks, and a PROXY typed against `IStorageService` that forwards to `inner.list` is exactly such a caller — the one in `@objectstack/service-storage` goes with the adapters (#5541). --- diff --git a/packages/spec/spec-changes.json b/packages/spec/spec-changes.json index 61f3604072..f8c137492f 100644 --- a/packages/spec/spec-changes.json +++ b/packages/spec/spec-changes.json @@ -592,6 +592,13 @@ "migrationId": "action-session-roles-to-positions", "toMajor": 17, "rationale": "The MIRROR-IMAGE neighbour of the entry above, and the reason both are in this step: the hook `ctx.session` carried `roles` declared-and-never-produced (removed outright, #5050), while the ACTION body's `ctx.session` carries it produced-and-really-populated. `buildActionSession()` (`packages/runtime/src/action-execution.ts`) copies `ExecutionContext.positions` into a key spelled `roles` — the ADR-0090 D3 vocabulary handed to the author under the one spelling that ADR bans — so a body author met two different answers to one key name on one platform: rejected in a hook, live and full of values in an action. #5613 ruled contract-first (maintainer, 2026-08-06: \"C skeleton + A semantics\"): phase 1 (#5697) declared the previously undeclared shape as `ActionSessionSchema`, and phase 2 renames the key. `positions` is now the canonical key on that schema and `roles` a deprecated alias of it (#5779); the producer emits both for one deprecation window (#5613 runtime half), after which `roles` is removed on the path the v11 session-alias removal already walked (#3280 deprecated → #3290 removed). Why this is a D3 semantic TODO and not a D2 conversion, on two independent grounds: FIRST, there is no source to convert — an action `ctx.session` is constructed per dispatch and never persisted, so no `sys_metadata` row, example or template can carry the key — the `openApi31` (#4579) / `activationEvents` (#4657) / `hook-context-session-roles-retired` (#5050) shape. SECOND, the only place the key is ever SPELLED is inside an action body: author-written JS/TS, or a sandboxed script whose `ScriptContext.session` is still `unknown`. A declarative transform cannot safely rewrite an identifier inside free-form code — exactly the reason the ADR-0090 wave delegated `current_user.roles` to the author at step 13 (`cel-current-user-roles-to-positions`) instead of substituting text. Note what is deliberately NOT done here: the alias is not tombstoned. A `retiredKey()` REJECTS the key, and a deprecation window exists precisely so the old spelling keeps working while its readers move — tombstoning during the window would be the removal it is meant to defer. The tombstone (or the plain deletion the authorable-surface ratchet adjudicates) belongs to the release that closes the window. Until then this entry IS the channel: `spec-changes.json` and the generated upgrade guide are how a reader learns the rename before the removal reaches them. ADR-0090 D3, ADR-0087, #5613 / #5779." + }, + { + "surface": "contracts.IStorageService.list", + "replacement": "no replacement — track the keys you wrote (sys_file / file-reference records, queryable through ObjectQL with real pagination) instead of enumerating the bucket", + "migrationId": "storage-service-list-retired", + "toMajor": 17, + "rationale": "`list(prefix)` was an OPTIONAL contract method documented as \"List files in a directory/prefix\", and the two shipped adapters answered the same call with two different semantics — both of them silently incomplete. `LocalStorageAdapter.list` was a single-level `readdir`, so a nested key `a/b/c` was invisible under `list('a')` (only `a/b` came back), and a subdirectory that `stat` succeeded on was pushed into the result as a file, yielding a `StorageFileInfo` whose `size` is a directory inode and which cannot be downloaded at all. `S3StorageAdapter.list` was RECURSIVE (`ListObjectsV2` matches the whole key) and read neither `IsTruncated` nor `ContinuationToken`, so past 1000 objects the \"all files\" a caller received was the first page, with no signal. One contract method, two dialects, both quietly incomplete — and the first feature that genuinely needed to enumerate a prefix (backup, orphan sweep, migration audit) would have got two different answers on two deployments without an error on either. #5172 was nearly that feature: it planned to drive attachment reclamation off `list(EMAIL_ATTACHMENT_KEY_PREFIX)`, found the local adapter could not see one level down, and switched to queue-driven deferred work instead. Nothing consumed it afterwards: the only in-repo call site was the `SwappableStorageService` pass-through (which itself rejects when the active adapter has no `list`), and REST, CLI and the storage routes never called it. Remove was chosen over align-and-tighten (maintainer ruling, 2026-08-05, #5266): aligning would grow a conformance surface nobody walks, while a prefix listing that cannot paginate is the wrong signature to inherit — when a real caller needs enumeration it returns cursor-shaped, `list(prefix, { cursor, limit })`, with adapter-conformance cases (nested keys, directory entries, >1000 objects) proving both backends agree. This is a TS/API contract surface — a storage adapter is CODE, never stack metadata — so there is no source for the chain to rewrite, and deliberately no schema tombstone: nothing ever ran an adapter through a `.parse()`, so a prescription there would reach no one. The enforced channel is tsc, and it reports at the call site. Same disposition, and the same reason, as `data-driver-find-stream-retired` (#4484). ADR-0049 / ADR-0087, #5540 (analysis #5266)." } ], "removed": [] @@ -1243,6 +1250,13 @@ "migrationId": "action-session-roles-to-positions", "toMajor": 17, "rationale": "The MIRROR-IMAGE neighbour of the entry above, and the reason both are in this step: the hook `ctx.session` carried `roles` declared-and-never-produced (removed outright, #5050), while the ACTION body's `ctx.session` carries it produced-and-really-populated. `buildActionSession()` (`packages/runtime/src/action-execution.ts`) copies `ExecutionContext.positions` into a key spelled `roles` — the ADR-0090 D3 vocabulary handed to the author under the one spelling that ADR bans — so a body author met two different answers to one key name on one platform: rejected in a hook, live and full of values in an action. #5613 ruled contract-first (maintainer, 2026-08-06: \"C skeleton + A semantics\"): phase 1 (#5697) declared the previously undeclared shape as `ActionSessionSchema`, and phase 2 renames the key. `positions` is now the canonical key on that schema and `roles` a deprecated alias of it (#5779); the producer emits both for one deprecation window (#5613 runtime half), after which `roles` is removed on the path the v11 session-alias removal already walked (#3280 deprecated → #3290 removed). Why this is a D3 semantic TODO and not a D2 conversion, on two independent grounds: FIRST, there is no source to convert — an action `ctx.session` is constructed per dispatch and never persisted, so no `sys_metadata` row, example or template can carry the key — the `openApi31` (#4579) / `activationEvents` (#4657) / `hook-context-session-roles-retired` (#5050) shape. SECOND, the only place the key is ever SPELLED is inside an action body: author-written JS/TS, or a sandboxed script whose `ScriptContext.session` is still `unknown`. A declarative transform cannot safely rewrite an identifier inside free-form code — exactly the reason the ADR-0090 wave delegated `current_user.roles` to the author at step 13 (`cel-current-user-roles-to-positions`) instead of substituting text. Note what is deliberately NOT done here: the alias is not tombstoned. A `retiredKey()` REJECTS the key, and a deprecation window exists precisely so the old spelling keeps working while its readers move — tombstoning during the window would be the removal it is meant to defer. The tombstone (or the plain deletion the authorable-surface ratchet adjudicates) belongs to the release that closes the window. Until then this entry IS the channel: `spec-changes.json` and the generated upgrade guide are how a reader learns the rename before the removal reaches them. ADR-0090 D3, ADR-0087, #5613 / #5779." + }, + { + "surface": "contracts.IStorageService.list", + "replacement": "no replacement — track the keys you wrote (sys_file / file-reference records, queryable through ObjectQL with real pagination) instead of enumerating the bucket", + "migrationId": "storage-service-list-retired", + "toMajor": 17, + "rationale": "`list(prefix)` was an OPTIONAL contract method documented as \"List files in a directory/prefix\", and the two shipped adapters answered the same call with two different semantics — both of them silently incomplete. `LocalStorageAdapter.list` was a single-level `readdir`, so a nested key `a/b/c` was invisible under `list('a')` (only `a/b` came back), and a subdirectory that `stat` succeeded on was pushed into the result as a file, yielding a `StorageFileInfo` whose `size` is a directory inode and which cannot be downloaded at all. `S3StorageAdapter.list` was RECURSIVE (`ListObjectsV2` matches the whole key) and read neither `IsTruncated` nor `ContinuationToken`, so past 1000 objects the \"all files\" a caller received was the first page, with no signal. One contract method, two dialects, both quietly incomplete — and the first feature that genuinely needed to enumerate a prefix (backup, orphan sweep, migration audit) would have got two different answers on two deployments without an error on either. #5172 was nearly that feature: it planned to drive attachment reclamation off `list(EMAIL_ATTACHMENT_KEY_PREFIX)`, found the local adapter could not see one level down, and switched to queue-driven deferred work instead. Nothing consumed it afterwards: the only in-repo call site was the `SwappableStorageService` pass-through (which itself rejects when the active adapter has no `list`), and REST, CLI and the storage routes never called it. Remove was chosen over align-and-tighten (maintainer ruling, 2026-08-05, #5266): aligning would grow a conformance surface nobody walks, while a prefix listing that cannot paginate is the wrong signature to inherit — when a real caller needs enumeration it returns cursor-shaped, `list(prefix, { cursor, limit })`, with adapter-conformance cases (nested keys, directory entries, >1000 objects) proving both backends agree. This is a TS/API contract surface — a storage adapter is CODE, never stack metadata — so there is no source for the chain to rewrite, and deliberately no schema tombstone: nothing ever ran an adapter through a `.parse()`, so a prescription there would reach no one. The enforced channel is tsc, and it reports at the call site. Same disposition, and the same reason, as `data-driver-find-stream-retired` (#4484). ADR-0049 / ADR-0087, #5540 (analysis #5266)." } ], "removed": [] diff --git a/packages/spec/src/contracts/storage-service.test.ts b/packages/spec/src/contracts/storage-service.test.ts index e5fcf7c51f..5474ed48bc 100644 --- a/packages/spec/src/contracts/storage-service.test.ts +++ b/packages/spec/src/contracts/storage-service.test.ts @@ -29,11 +29,9 @@ describe('Storage Service Contract', () => { delete: async () => {}, exists: async () => false, getInfo: async (key) => ({ key, size: 0, lastModified: new Date() }), - list: async (_prefix) => [], getSignedUrl: async (_key, _expiresIn) => 'https://example.com/signed', }; - expect(storage.list).toBeDefined(); expect(storage.getSignedUrl).toBeDefined(); }); @@ -110,26 +108,52 @@ describe('Storage Service Contract', () => { expect(info.metadata?.uploadedBy).toBe('user-1'); }); - it('should list files by prefix', async () => { + // --------------------------------------------------------------------- + // Retirement pin — `list?(prefix)` removed in #5540 (ADR-0049 + // enforce-or-remove; the two-dialect analysis is #5266). + // + // `IStorageService` is a pure TypeScript contract: nothing parses it, so + // this retirement has no `retiredKey()` tombstone and no parse-time + // prescription to assert (spec-property-retirement §2, "nothing parses it" + // route). tsc is the only channel the removal has — and `tsconfig.test.json` + // puts this file in front of tsc (#5286), so the two `@ts-expect-error` + // directives below are real checks that go red the day the member returns, + // not phantom ones. Restoring `list?()` to the interface turns both into + // "unused '@ts-expect-error' directive". + // + // The test these replaced exercised the removed member itself; keeping it + // green would have meant keeping the member. + // --------------------------------------------------------------------- + it('no longer declares list(prefix) — reading it is a type error', () => { const storage: IStorageService = { upload: async () => {}, download: async () => Buffer.from(''), delete: async () => {}, exists: async () => true, getInfo: async (key) => ({ key, size: 0, lastModified: new Date() }), - list: async (prefix) => { - const allFiles: StorageFileInfo[] = [ - { key: 'docs/a.txt', size: 100, lastModified: new Date() }, - { key: 'docs/b.txt', size: 200, lastModified: new Date() }, - { key: 'images/c.png', size: 300, lastModified: new Date() }, - ]; - return allFiles.filter((f) => f.key.startsWith(prefix)); - }, }; - const docs = await storage.list!('docs/'); - expect(docs).toHaveLength(2); - expect(docs[0].key).toBe('docs/a.txt'); + // @ts-expect-error — `list` was removed from IStorageService (#5540). + // Prefix enumeration returns cursor-shaped when a caller needs it: + // `list(prefix, { cursor, limit })`. It is not on the contract today. + const retired = storage.list; + + expect(retired).toBeUndefined(); + }); + + it('no longer accepts an implementation that declares list(prefix)', () => { + const storage: IStorageService = { + upload: async () => {}, + download: async () => Buffer.from(''), + delete: async () => {}, + exists: async () => true, + getInfo: async (key) => ({ key, size: 0, lastModified: new Date() }), + // @ts-expect-error — excess property: the contract has no `list` member, + // so an adapter can no longer advertise one through it (#5540). + list: async (_prefix: string): Promise => [], + }; + + expect(typeof storage.getInfo).toBe('function'); }); it('should generate signed URLs', async () => { diff --git a/packages/spec/src/contracts/storage-service.ts b/packages/spec/src/contracts/storage-service.ts index 526860572d..eac0434180 100644 --- a/packages/spec/src/contracts/storage-service.ts +++ b/packages/spec/src/contracts/storage-service.ts @@ -123,12 +123,22 @@ export interface IStorageService { */ getInfo(key: string): Promise; - /** - * List files in a directory/prefix - * @param prefix - Key prefix to list - * @returns Array of file info objects - */ - list?(prefix: string): Promise; + // `list?(prefix: string): Promise` was REMOVED in + // @objectstack/spec 5.x (#5540, ADR-0049 enforce-or-remove; analysis in + // #5266). It had no consumer — the only in-repo call site was a proxy + // pass-through — and the two shipped adapters answered the same call with + // two different semantics, both silently incomplete: the local adapter + // listed one level and reported directories as files, the S3 adapter + // recursed and truncated at 1000 objects without reading + // `IsTruncated`/`ContinuationToken`. One contract method, two dialects, + // no signal. + // + // There is no replacement, deliberately: a prefix enumeration that cannot + // paginate is the wrong shape to inherit. When a real caller needs one, it + // comes back cursor-shaped — `list(prefix, { cursor, limit })` returning a + // page plus a continuation token — with adapter-conformance cases (nested + // keys, directory entries, >1000 objects) proving both backends agree. + // Until then the storage contract only exposes per-key operations. /** * Generate a pre-signed URL for temporary access diff --git a/packages/spec/src/migrations/registry.ts b/packages/spec/src/migrations/registry.ts index 2484edeeba..a00937710e 100644 --- a/packages/spec/src/migrations/registry.ts +++ b/packages/spec/src/migrations/registry.ts @@ -992,6 +992,18 @@ const step17: MigrationStep = { + '`session.isSystem` in the hook, and judge PRIVILEGE through the security service, which ' + 'reads capability grants (`permissions`), placements (`positions`) and the derived posture ' + 'off the execution context.\n\n' + + 'The same enforce-or-remove reading reaches the storage contract: ' + + '`IStorageService.list(prefix)` is removed (#5540, analysis #5266). It had no ' + + 'consumer — the only in-repo call site was a proxy pass-through — and the two shipped ' + + 'adapters answered it with two different, silently incomplete semantics: the local ' + + 'adapter listed a single level and reported directories as files, the S3 adapter ' + + 'recursed and stopped at 1000 objects without reading `IsTruncated` / ' + + '`ContinuationToken`. Enumerating a prefix without a cursor is the wrong signature to ' + + 'inherit, so nothing replaces it in place: query the file records you wrote, and let a ' + + 'real caller bring back a cursor-shaped `list(prefix, { cursor, limit })` with ' + + 'adapter-conformance cases behind it. Same shape and same disposition as the ' + + '`findStream` retirement above — a TS/API contract, no stored source, no tombstone, ' + + 'tsc at the call site.\n\n' + 'Finally it retires the two inert `IndexSchema` keys, `indexes[].type` and ' + '`indexes[].partial` (#5248, #4943). Neither ever had a DDL consumer: ' + '`SqlDriver.syncDeclaredIndexes` creates declared indexes through knex\'s `table.index()` / ' @@ -1955,6 +1967,57 @@ const step17: MigrationStep = { + 'and a body still reading it sees `undefined` — which is why the read must be moved ' + 'inside the window rather than at its close.', }, + { + id: 'storage-service-list-retired', + surface: 'contracts.IStorageService.list', + replacement: + 'no replacement — track the keys you wrote (sys_file / file-reference records, ' + + 'queryable through ObjectQL with real pagination) instead of enumerating the bucket', + reason: + '`list(prefix)` was an OPTIONAL contract method documented as "List files in a ' + + 'directory/prefix", and the two shipped adapters answered the same call with two ' + + 'different semantics — both of them silently incomplete. `LocalStorageAdapter.list` ' + + 'was a single-level `readdir`, so a nested key `a/b/c` was invisible under ' + + '`list(\'a\')` (only `a/b` came back), and a subdirectory that `stat` succeeded on ' + + 'was pushed into the result as a file, yielding a `StorageFileInfo` whose `size` is ' + + 'a directory inode and which cannot be downloaded at all. `S3StorageAdapter.list` ' + + 'was RECURSIVE (`ListObjectsV2` matches the whole key) and read neither ' + + '`IsTruncated` nor `ContinuationToken`, so past 1000 objects the "all files" a ' + + 'caller received was the first page, with no signal. One contract method, two ' + + 'dialects, both quietly incomplete — and the first feature that genuinely needed to ' + + 'enumerate a prefix (backup, orphan sweep, migration audit) would have got two ' + + 'different answers on two deployments without an error on either. #5172 was nearly ' + + 'that feature: it planned to drive attachment reclamation off ' + + '`list(EMAIL_ATTACHMENT_KEY_PREFIX)`, found the local adapter could not see one ' + + 'level down, and switched to queue-driven deferred work instead. Nothing consumed ' + + 'it afterwards: the only in-repo call site was the `SwappableStorageService` ' + + 'pass-through (which itself rejects when the active adapter has no `list`), and ' + + 'REST, CLI and the storage routes never called it. Remove was chosen over ' + + 'align-and-tighten (maintainer ruling, 2026-08-05, #5266): aligning would grow a ' + + 'conformance surface nobody walks, while a prefix listing that cannot paginate is ' + + 'the wrong signature to inherit — when a real caller needs enumeration it returns ' + + 'cursor-shaped, `list(prefix, { cursor, limit })`, with adapter-conformance cases ' + + '(nested keys, directory entries, >1000 objects) proving both backends agree. This ' + + 'is a TS/API contract surface — a storage adapter is CODE, never stack metadata — ' + + 'so there is no source for the chain to rewrite, and deliberately no schema ' + + 'tombstone: nothing ever ran an adapter through a `.parse()`, so a prescription ' + + 'there would reach no one. The enforced channel is tsc, and it reports at the call ' + + 'site. Same disposition, and the same reason, as ' + + '`data-driver-find-stream-retired` (#4484). ADR-0049 / ADR-0087, #5540 ' + + '(analysis #5266).', + acceptanceCriteria: + 'No code calls `storage.list(...)` on the `file-storage` service or on any ' + + '`IStorageService` value. Code that needed "which files are under this prefix" ' + + 'reads the records it wrote — `sys_file` / file-reference rows carry the storage ' + + 'key and page deterministically through ObjectQL — rather than asking the bucket, ' + + 'which is also the only form that stays correct past 1000 objects and across both ' + + 'adapters. An adapter that still IMPLEMENTS `list` keeps compiling (an extra ' + + 'method is not an error on a class) and is simply unreachable through the ' + + 'contract, so deleting it is cleanup that can follow. The break is on the CALLER ' + + 'side: `storage.list(...)` no longer type-checks, and a PROXY typed against ' + + '`IStorageService` that forwards to `inner.list` is exactly such a caller — the ' + + 'one in `@objectstack/service-storage` goes with the adapters (#5541).', + }, ], }; From 2421898abc6c202fe489ffc0b9e623fec2ab7d12 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 14:45:21 +0000 Subject: [PATCH 2/2] refactor(service-storage): drop the SwappableStorageService.list passthrough (#5540) The contract member it forwarded to is gone, so `this.inner.list` no longer type-checks. That break is CI-visible through tsup's DTS step (which runs tsc), not through a `typecheck` script -- `@objectstack/service-storage#build` failed and took Build Core / Test Core / Dogfood with it. PM ruling on #5540: land the contract removal and its only caller's deletion atomically, so `main` is never red. Scope is the proxy passthrough plus its two test sites only; the adapters' own `list` implementations stay for #5541. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_011M7UwH25Unfi73UHim7ajY --- .../src/local-storage-adapter.test.ts | 3 ++- .../src/swappable-storage-service.test.ts | 13 +++---------- .../src/swappable-storage-service.ts | 10 ++++------ 3 files changed, 9 insertions(+), 17 deletions(-) diff --git a/packages/services/service-storage/src/local-storage-adapter.test.ts b/packages/services/service-storage/src/local-storage-adapter.test.ts index 184aece397..ef6678665b 100644 --- a/packages/services/service-storage/src/local-storage-adapter.test.ts +++ b/packages/services/service-storage/src/local-storage-adapter.test.ts @@ -31,7 +31,8 @@ describe('LocalStorageAdapter', () => { expect(typeof storage.delete).toBe('function'); expect(typeof storage.exists).toBe('function'); expect(typeof storage.getInfo).toBe('function'); - expect(typeof storage.list).toBe('function'); + // `list` is deliberately absent: IStorageService no longer declares it + // (#5540). The adapter's own `list` implementation goes in #5541. }); it('should upload and download a file', async () => { diff --git a/packages/services/service-storage/src/swappable-storage-service.test.ts b/packages/services/service-storage/src/swappable-storage-service.test.ts index 20d18336b8..cf3d52c776 100644 --- a/packages/services/service-storage/src/swappable-storage-service.test.ts +++ b/packages/services/service-storage/src/swappable-storage-service.test.ts @@ -83,19 +83,12 @@ describe('SwappableStorageService', () => { it('rejects optional methods when the active adapter omits them', async () => { const proxy = new SwappableStorageService(new MinimalAdapter()); - await expect(proxy.list('p')).rejects.toThrow(/does not support list/); await expect(proxy.getSignedUrl('k', 60)).rejects.toThrow(/does not support getSignedUrl/); await expect(proxy.getPresignedUpload('k', 60)).rejects.toThrow(/does not support getPresignedUpload/); await expect(proxy.initiateChunkedUpload('k')).rejects.toThrow(/does not support initiateChunkedUpload/); }); - it('forwards list() to the active adapter when supported', async () => { - const a = new FakeAdapter('A'); - await a.upload('p/1', Buffer.from('1')); - await a.upload('p/2', Buffer.from('22')); - await a.upload('q/3', Buffer.from('333')); - const proxy = new SwappableStorageService(a); - const out = await proxy.list('p/'); - expect(out.map((i) => i.key).sort()).toEqual(['p/1', 'p/2']); - }); + // The `forwards list() to the active adapter when supported` case was + // deleted with the proxy method it exercised (#5540): IStorageService no + // longer declares `list`, so there is nothing for the proxy to forward. }); diff --git a/packages/services/service-storage/src/swappable-storage-service.ts b/packages/services/service-storage/src/swappable-storage-service.ts index f4f1af7d0a..5690ead032 100644 --- a/packages/services/service-storage/src/swappable-storage-service.ts +++ b/packages/services/service-storage/src/swappable-storage-service.ts @@ -71,12 +71,10 @@ export class SwappableStorageService implements IStorageService { return this.inner.getInfo(key); } - list(prefix: string): Promise { - if (typeof this.inner.list !== 'function') { - return Promise.reject(new Error('Active storage adapter does not support list()')); - } - return this.inner.list(prefix); - } + // `list(prefix)` was removed from IStorageService in #5540 (ADR-0049 + // enforce-or-remove; analysis #5266), so there is no contract member left to + // forward to. The adapters' own `list` implementations are retired + // separately in #5541. getSignedUrl(key: string, expiresIn: number, options?: PresignedDownloadOptions): Promise { if (typeof this.inner.getSignedUrl !== 'function') {