refactor(service-storage): 摘除 Local/S3 适配器的 list(prefix) 实现与测试 (#5541)

.changeset/storage-adapter-list-implementations-removed.md

@@ -0,0 +1,58 @@
+---
+"@objectstack/service-storage": patch
+---
+
+refactor(service-storage): drop `list(prefix)` from the local and S3 adapters — the implementation half of the #5540 contract retirement (#5541)
+
+`IStorageService.list?(prefix)` was removed from the contract in `@objectstack/spec` 5.x
+(#5540, ADR-0049 enforce-or-remove; analysis #5266). This removes what it left behind:
+the two shipped adapters' own implementations, the tests that pinned them, and the
+`'list'` label in each adapter's metrics vocabulary.
+
+**Nothing in this repository ever called them.** The only in-repo call site was the
+`SwappableStorageService` pass-through, deleted with the contract member in #5540. After
+that deletion the surviving references were the two adapter methods and their own tests —
+four sites, all inside `@objectstack/service-storage`, all of them producers. REST, the
+CLI, the storage routes, the attachment/file-reference lifecycles and the backfill
+tooling never called `list` on either adapter, on the swappable proxy, or on the
+`file-storage` service. #5172 came closest and walked away: it planned to reclaim email
+attachments by listing `EMAIL_ATTACHMENT_KEY_PREFIX`, found the local adapter could not
+see one level down, and switched to queue-driven deferred work instead.
+
+**What the two implementations actually did**, which is why aligning them was rejected:
+
+| Adapter | Answered `list('a')` with |
+| --- | --- |
+| `LocalStorageAdapter` | one level of `readdir` — a nested key `a/b/c` was invisible, you got `a/b` — and every subdirectory `stat` succeeded on was returned as if it were a file, so `size` was a directory inode and `download()` could not fetch it |
+| `S3StorageAdapter` | a recursive `ListObjectsV2` that read neither `IsTruncated` nor `ContinuationToken`, so past 1000 objects the "all files under this prefix" you got was the first page, indistinguishable from a complete answer |
+
+One contract method, two dialects, both silently incomplete, no signal on either.
+
+**Migration.** Callers holding the contract type were already migrated by #5540 — the
+member is gone from `IStorageService`, so `storage.list(...)` stops type-checking there.
+This release also removes the method from the **concrete** classes, so a caller holding a
+`LocalStorageAdapter` or `S3StorageAdapter` directly loses it too:
+
+| Wrote | Write instead |
+| --- | --- |
+| `new LocalStorageAdapter(...).list('attachments/task/')` | query the records you wrote — `sys_file` / file-reference rows carry the storage key and page deterministically through ObjectQL |
+| `new S3StorageAdapter(...).list(prefix)` | same; for a genuine bucket sweep, call `ListObjectsV2` through the AWS SDK yourself and handle `ContinuationToken`, which is the part the adapter never did |
+| a custom adapter of your own with `list?(prefix)` | nothing breaks — an extra method on a class is not a type error; delete it whenever it suits you |
+
+Querying your own records is not a workaround for the missing method. It is the only form
+that was ever correct on both backends and past 1000 objects: the bucket was never the
+system of record for "which files exist" — the rows are.
+
+**If enumeration ever comes back, it comes back cursor-shaped.** Not this signature. A
+prefix listing that cannot paginate is the wrong shape to inherit, so a future
+first-party need returns `list(prefix, { cursor, limit })` — 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.
+
+Patch rather than major: the contract break was #5540's and shipped there. `tsc` cannot
+see this one — a class may carry members its interface does not declare, which is exactly
+why the #5540 changeset told adapter authors that leaving an implementation in place
+still compiles — so the absence is held by a runtime pin,
+`storage-adapter-list-retirement.test.ts`, instead.

packages/services/service-storage/src/local-storage-adapter.metrics.test.ts

@@ -29,31 +29,23 @@ describe('LocalStorageAdapter instrumentation', () => {
         expect(durations[0]).toBeGreaterThanOrEqual(0);
     });
 
-    it('records ok for get / head / list when the object is present', async () => {
+    it('records ok for get / head when the object is present', async () => {
         const metrics = new InMemoryMetricsRegistry();
         const storage = new LocalStorageAdapter({ rootDir, metrics });
         await storage.upload('a/b.txt', Buffer.from('x'));
         await storage.download('a/b.txt');
         await storage.exists('a/b.txt');
         await storage.getInfo('a/b.txt');
-        await storage.list('a');
 
         expect(metrics.totalCounter(SEMCONV.storageOperationsTotal, { adapter: 'local', op: 'get', result: 'ok' })).toBe(1);
         expect(metrics.totalCounter(SEMCONV.storageOperationsTotal, { adapter: 'local', op: 'head', result: 'ok' })).toBe(2);
-        expect(metrics.totalCounter(SEMCONV.storageOperationsTotal, { adapter: 'local', op: 'list', result: 'ok' })).toBe(1);
     });
 
-    it('list() does not double-count head per entry', async () => {
-        const metrics = new InMemoryMetricsRegistry();
-        const storage = new LocalStorageAdapter({ rootDir, metrics });
-        await storage.upload('p/a.txt', Buffer.from('x'));
-        await storage.upload('p/b.txt', Buffer.from('y'));
-        metrics.reset();
-        await storage.list('p');
-        // Exactly one list operation; no head operations from inner stats.
-        expect(metrics.totalCounter(SEMCONV.storageOperationsTotal, { adapter: 'local', op: 'list' })).toBe(1);
-        expect(metrics.totalCounter(SEMCONV.storageOperationsTotal, { adapter: 'local', op: 'head' })).toBe(0);
-    });
+    // The `list() does not double-count head per entry` case was deleted with the
+    // method it exercised (#5541). It pinned an internal detail of the removed
+    // implementation (inline `stat` instead of `getInfo`), so with `list` gone it
+    // could only have stayed green by asserting nothing. `op: 'list'` is no longer
+    // in the adapter's `track()` vocabulary either, so no sample can carry it.
 
     it('records errors_total{errorClass} on path-traversal rejection', async () => {
         const metrics = new InMemoryMetricsRegistry();

packages/services/service-storage/src/local-storage-adapter.test.ts

@@ -32,7 +32,8 @@ describe('LocalStorageAdapter', () => {
     expect(typeof storage.exists).toBe('function');
     expect(typeof storage.getInfo).toBe('function');
     // `list` is deliberately absent: IStorageService no longer declares it
-    // (#5540). The adapter's own `list` implementation goes in #5541.
+    // (#5540) and the adapter no longer implements it (#5541). The absence is
+    // pinned in `storage-adapter-list-retirement.test.ts`.
   });
 
   it('should upload and download a file', async () => {
@@ -74,21 +75,12 @@ describe('LocalStorageAdapter', () => {
     expect(info.lastModified).toBeInstanceOf(Date);
   });
 
-  it('should list files in a directory', async () => {
-    await createTempDir();
-    await adapter.upload('docs/a.txt', Buffer.from('a'));
-    await adapter.upload('docs/b.txt', Buffer.from('bb'));
-    const files = await adapter.list('docs');
-    expect(files).toHaveLength(2);
-    const keys = files.map(f => f.key).sort();
-    expect(keys).toEqual(['docs/a.txt', 'docs/b.txt']);
-  });
-
-  it('should return empty array when listing non-existent directory', async () => {
-    await createTempDir();
-    const files = await adapter.list('nonexistent');
-    expect(files).toEqual([]);
-  });
+  // The `should list files in a directory` and `should return empty array when
+  // listing non-existent directory` cases were deleted with the method they
+  // exercised (#5541). They pinned exactly the single-level, directories-as-files
+  // behaviour the retirement removed — `docs/a.txt` + `docs/b.txt` are both one
+  // level down, which is the only depth that implementation could see — so
+  // keeping them green would have meant keeping the method.
 
   it('should reject path traversal', async () => {
     await createTempDir();

packages/services/service-storage/src/local-storage-adapter.ts

@@ -87,7 +87,7 @@ export class LocalStorageAdapter implements IStorageService {
    * Wrap a storage operation with metrics instrumentation. Never swallows
    * the underlying error; instrumentation failures are silently ignored.
    */
-  private async track<T>(op: 'put' | 'get' | 'delete' | 'head' | 'list', fn: () => Promise<T>): Promise<T> {
+  private async track<T>(op: 'put' | 'get' | 'delete' | 'head', fn: () => Promise<T>): Promise<T> {
     const started = Date.now();
     const baseLabels = { adapter: 'local', op } as const;
     try {
@@ -189,29 +189,17 @@ export class LocalStorageAdapter implements IStorageService {
     });
   }
 
-  async list(prefix: string): Promise<StorageFileInfo[]> {
-    return this.track('list', async () => {
-      const dirPath = this.resolvePath(prefix);
-      try {
-        const entries = await fs.readdir(dirPath);
-        const results: StorageFileInfo[] = [];
-        for (const entry of entries) {
-          if (entry.startsWith('.')) continue;
-          const fullKey = prefix ? `${prefix}/${entry}` : entry;
-          try {
-            // Inline stat to avoid double-counting `head` operations.
-            const stat = await fs.stat(this.resolvePath(fullKey));
-            results.push({ key: fullKey, size: stat.size, lastModified: stat.mtime });
-          } catch {
-            /* skip */
-          }
-        }
-        return results;
-      } catch {
-        return [];
-      }
-    });
-  }
+  // `list(prefix)` is gone (#5541), following its removal from IStorageService
+  // (#5540, ADR-0049 enforce-or-remove; analysis #5266). This implementation was
+  // a single-level `readdir` that reported subdirectories as files, so it and the
+  // S3 adapter's recursive-but-truncated one answered the same call differently
+  // and neither said so. Nothing in the repo called either. Enumerate the records
+  // you wrote (`sys_file` / file references, paginated through ObjectQL) instead
+  // of the bucket; if a first-party caller ever needs real bucket enumeration it
+  // returns cursor-shaped — `list(prefix, { cursor, limit })` — with
+  // adapter-conformance cases proving both backends agree before it ships.
+  // Absence is pinned in `storage-adapter-list-retirement.test.ts`: an excess
+  // method on a class is not a type error, so tsc cannot hold this line.
 
   // ---------------------------------------------------------------------------
   // Presigned URL helpers

packages/services/service-storage/src/s3-storage-adapter.ts

@@ -72,7 +72,7 @@ export class S3StorageAdapter implements IStorageService {
    * Records ok/error counters, a duration histogram, and an error counter
    * keyed by error class on failure. Never swallows the underlying error.
    */
-  private async track<T>(op: 'put' | 'get' | 'delete' | 'head' | 'list', fn: () => Promise<T>): Promise<T> {
+  private async track<T>(op: 'put' | 'get' | 'delete' | 'head', fn: () => Promise<T>): Promise<T> {
     const started = Date.now();
     const baseLabels = { adapter: 's3', op } as const;
     try {
@@ -211,19 +211,19 @@ export class S3StorageAdapter implements IStorageService {
     });
   }
 
-  async list(prefix: string): Promise<StorageFileInfo[]> {
-    return this.track('list', async () => {
-      const client = await this.getClient();
-      const s3 = await this.s3Mod();
-      const cmd = new s3.ListObjectsV2Command({ Bucket: this.bucket, Prefix: prefix });
-      const res = await client.send(cmd);
-      return (res.Contents ?? []).map((item: any) => ({
-        key: item.Key,
-        size: item.Size ?? 0,
-        lastModified: item.LastModified ?? new Date(),
-      }));
-    });
-  }
+  // `list(prefix)` is gone (#5541), following its removal from IStorageService
+  // (#5540, ADR-0049 enforce-or-remove; analysis #5266). This implementation
+  // issued one `ListObjectsV2` and read neither `IsTruncated` nor
+  // `ContinuationToken`, so past 1000 objects it returned the first page with
+  // nothing to distinguish it from a complete answer — while the local adapter
+  // answered the same call one level deep. Nothing in the repo called either.
+  // Enumerate the records you wrote (`sys_file` / file references, paginated
+  // through ObjectQL) instead of the bucket; if a first-party caller ever needs
+  // real bucket enumeration it returns cursor-shaped —
+  // `list(prefix, { cursor, limit })` — with adapter-conformance cases proving
+  // both backends agree before it ships. Absence is pinned in
+  // `storage-adapter-list-retirement.test.ts`: an excess method on a class is
+  // not a type error, so tsc cannot hold this line.
 
   // ---------------------------------------------------------------------------
   // Presigned URLs

packages/services/service-storage/src/storage-adapter-list-retirement.test.ts

@@ -0,0 +1,82 @@
+// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
+
+/**
+ * Retirement pin — `list(prefix)` on the shipped storage adapters.
+ *
+ * `IStorageService.list?(prefix)` was removed from the contract in #5540
+ * (ADR-0049 enforce-or-remove; analysis #5266), and the two shipped adapters'
+ * own implementations were removed in #5541. This file keeps the retired
+ * surface retired.
+ *
+ * **Why a runtime pin and not `@ts-expect-error`.** On the *contract* side tsc
+ * is the enforced channel and `packages/spec/src/contracts/storage-service.test.ts`
+ * already carries both directives — reading `storage.list` is a type error, and
+ * an object literal typed `IStorageService` that declares `list` is an excess
+ * property. Neither reaches an adapter: a **class** that `implements` an
+ * interface is only checked for the members the interface requires, so a class
+ * may carry any number of extra methods without a single type error. That is
+ * exactly what the retirement changeset promises adapter authors ("an
+ * implementation left in place still compiles"), and it is also why tsc cannot
+ * notice these two coming back. The pin has to read the shape at runtime.
+ *
+ * `SwappableStorageService` deliberately gets no pin here: it forwards to an
+ * `inner` typed as `IStorageService`, so a re-added `list` passthrough fails to
+ * compile — which is how #5540 found it in the first place. tsc holds that line
+ * already; duplicating it here would pin nothing new.
+ *
+ * If prefix enumeration ever comes back 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. Restoring the old single-argument
+ * shape to satisfy this file is the one fix that is not a fix.
+ */
+
+import { describe, it, expect } from 'vitest';
+import { LocalStorageAdapter } from './local-storage-adapter';
+import { S3StorageAdapter } from './s3-storage-adapter';
+
+/** Every method name reachable on an instance, own + prototype chain. */
+function reachableMethodNames(instance: object): string[] {
+  const names = new Set<string>();
+  for (
+    let cursor: object | null = instance;
+    cursor && cursor !== Object.prototype;
+    cursor = Object.getPrototypeOf(cursor)
+  ) {
+    for (const name of Object.getOwnPropertyNames(cursor)) names.add(name);
+  }
+  return [...names];
+}
+
+describe('storage adapters no longer implement list(prefix) (#5540 / #5541)', () => {
+  it('LocalStorageAdapter exposes no list member', () => {
+    const adapter = new LocalStorageAdapter({ rootDir: '/tmp/os-storage-pin-not-created' });
+
+    expect(reachableMethodNames(adapter)).not.toContain('list');
+    expect('list' in adapter).toBe(false);
+    expect((adapter as unknown as Record<string, unknown>).list).toBeUndefined();
+  });
+
+  it('S3StorageAdapter exposes no list member', () => {
+    // The constructor only records options; the AWS SDK is imported lazily on
+    // first use, so this never touches the network or the peer dependency.
+    const adapter = new S3StorageAdapter({ bucket: 'pin-bucket', region: 'us-east-1' });
+
+    expect(reachableMethodNames(adapter)).not.toContain('list');
+    expect('list' in adapter).toBe(false);
+    expect((adapter as unknown as Record<string, unknown>).list).toBeUndefined();
+  });
+
+  it('still exposes the per-key contract members the retirement kept', () => {
+    // Guards the pin against the mirror failure: a pin that passes because the
+    // adapter has no methods at all would be green and meaningless.
+    const local = new LocalStorageAdapter({ rootDir: '/tmp/os-storage-pin-not-created' });
+    const s3 = new S3StorageAdapter({ bucket: 'pin-bucket', region: 'us-east-1' });
+
+    for (const adapter of [local, s3]) {
+      for (const member of ['upload', 'download', 'delete', 'exists', 'getInfo'] as const) {
+        expect(typeof (adapter as unknown as Record<string, unknown>)[member]).toBe('function');
+      }
+    }
+  });
+});

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

@@ -24,11 +24,9 @@ class FakeAdapter implements IStorageService {
     if (!b) throw new Error('not found');
     return { key, size: b.length, lastModified: new Date(), contentType: 'application/octet-stream' };
   }
-  async list(prefix: string): Promise<StorageFileInfo[]> {
-    return Array.from(this.store.keys())
-      .filter((k) => k.startsWith(prefix))
-      .map((k) => ({ key: k, size: this.store.get(k)!.length, lastModified: new Date() }));
-  }
+  // No `list(prefix)`: the contract dropped it in #5540 and the shipped adapters
+  // dropped their implementations in #5541, so a fake that still advertised one
+  // would model a surface no real adapter has.
 }
 
 /** Adapter that omits the optional methods to exercise the proxy's