fix(cli,service-storage): stop re-pointing the storage adapter every boot — and mean it when it warns (#4096)

.changeset/storage-adapter-swap-verdict.md

@@ -0,0 +1,58 @@
+---
+'@objectstack/service-storage': patch
+'@objectstack/cli': patch
+---
+
+**The storage adapter stops being rebuilt and re-pointed on every boot, and the
+"files may be unreachable" warning stops firing at a healthy server (#4096).**
+
+Every `os dev` / `os serve` boot printed:
+
+```
+WARN StorageServicePlugin: storage adapter swapped (LocalStorageAdapter →
+LocalStorageAdapter). Existing files were NOT migrated and may be unreachable
+through the new adapter.
+```
+
+The warning was telling the truth. `serve` constructed the plugin with
+`{ driver: 'local', root }` — and `StorageServicePluginOptions` declares
+neither key. Both were dropped silently, so the plugin applied its own
+`./storage` default, `OS_STORAGE_ROOT` changed nothing, and uploads landed in a
+directory nobody named. The `storage` settings namespace then corrected the root
+on its first read (its manifest default is `./.objectstack/data/uploads`),
+genuinely moving the backing store — every boot, forever.
+
+Three fixes, because there were three defects:
+
+- **`serve` now passes options the plugin reads** — `{ adapter: 'local',
+  local: { rootDir } }`. `OS_STORAGE_ROOT` takes effect, and local uploads land
+  under `.objectstack/data/uploads` from the first byte instead of `./storage`.
+  Extracted as `resolveStorageCapabilityArg` so the option SHAPE is pinned by
+  tests: a mismatch like this type-checks fine and does nothing at runtime.
+- **A swap is skipped when nothing changed.** The plugin records what the
+  running adapter points at and compares resolved configurations, instead of
+  rebuilding whenever the settings namespace held any value at all — which is
+  every boot once that namespace has persisted its own defaults.
+- **The warning now means what it says.** It fires when the BACKING STORE moved
+  (kind change, different root, different bucket/region/endpoint), not merely
+  when the adapter object was replaced. A credential rotation swaps the adapter
+  so the new key takes effect and logs at info: same bucket, nothing stranded.
+  A swap from a caller that resolved no target still warns — ignorance must not
+  silence it.
+
+Path spellings are normalised, so the platform writing the same default two ways
+(`./.objectstack/data/uploads` in the settings manifest,
+`.objectstack/data/uploads` in the CLI) is no longer read as a migration between
+a directory and itself.
+
+Verified on `examples/app-todo`: the boot-diagnostics block went from four
+warnings to three, with the storage line gone and `./storage` no longer created.
+19 unit cases cover the target resolver and the swap/warn split (including the
+refusals), 4 plugin-level cases pin what a boot does and says, and 7 pin the CLI
+option shape.
+
+`config.storage` authored with the `driver`/`root` dialect is still forwarded
+verbatim and still not read by the plugin — the same mismatch one layer up.
+Correcting it means deciding whether the plugin accepts that dialect or the
+config schema is wrong, so it is filed rather than papered over with a lenient
+alias here (AGENTS.md Prime Directive #12).

packages/cli/src/commands/serve-storage-capability.test.ts

@@ -0,0 +1,81 @@
+// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
+
+/**
+ * framework#4096 — what `StorageServicePlugin` is actually constructed with.
+ *
+ * The fallback used to be `{ driver: 'local', root }`, and
+ * `StorageServicePluginOptions` declares neither key. Both were dropped
+ * silently, so the plugin applied its own `./storage` default,
+ * `OS_STORAGE_ROOT` changed nothing, and uploads landed somewhere the operator
+ * never named. The `storage` settings namespace then corrected the root on its
+ * first read — its manifest default is `./.objectstack/data/uploads` — which
+ * swapped the adapter and warned "existing files were NOT migrated" on every
+ * boot of a healthy server.
+ *
+ * The warning was telling the truth; the configuration was wrong. These pin the
+ * option SHAPE, because a shape mismatch is exactly the failure a passing type
+ * check does not catch when the receiving interface has no index signature and
+ * the value is built as a plain object literal.
+ */
+
+import { describe, it, expect } from 'vitest';
+import { resolveStorageCapabilityArg } from './serve.js';
+
+describe('resolveStorageCapabilityArg', () => {
+  it('builds options StorageServicePlugin actually reads', () => {
+    // `adapter` + `local.rootDir` — NOT `driver` + `root`.
+    expect(resolveStorageCapabilityArg(undefined).options).toEqual({
+      adapter: 'local',
+      local: { rootDir: '.objectstack/data/uploads' },
+    });
+  });
+
+  it('never emits the keys the plugin ignores', () => {
+    // The regression guard proper: `{driver, root}` type-checks fine as an
+    // argument and does nothing at runtime.
+    const { options } = resolveStorageCapabilityArg(undefined);
+    expect(options).not.toHaveProperty('driver');
+    expect(options).not.toHaveProperty('root');
+  });
+
+  it('honours OS_STORAGE_ROOT, which the old shape discarded', () => {
+    const { options, localRoot } = resolveStorageCapabilityArg(undefined, '/srv/uploads');
+    expect(options).toEqual({ adapter: 'local', local: { rootDir: '/srv/uploads' } });
+    expect(localRoot).toBe('/srv/uploads');
+  });
+
+  it('ignores a blank or whitespace-only env root', () => {
+    for (const blank of ['', '   ']) {
+      expect(resolveStorageCapabilityArg(undefined, blank).options).toEqual({
+        adapter: 'local',
+        local: { rootDir: '.objectstack/data/uploads' },
+      });
+    }
+  });
+
+  it('reports the local root so only the fallback triggers the production warning', () => {
+    // A host that configured its own backend must not be told it is on local disk.
+    expect(resolveStorageCapabilityArg(undefined).localRoot).toBe('.objectstack/data/uploads');
+    expect(resolveStorageCapabilityArg({ adapter: 's3', s3: { bucket: 'b', region: 'r' } }).localRoot)
+      .toBeUndefined();
+  });
+
+  it('forwards a host-configured storage block verbatim', () => {
+    const cfg = { adapter: 's3', s3: { bucket: 'b', region: 'r' } };
+    expect(resolveStorageCapabilityArg(cfg).options).toBe(cfg);
+    // The `driver` dialect is still forwarded untouched — the plugin does not
+    // read it either, but rewriting it here would fossilize the wrong contract
+    // rather than fix it. Tracked separately.
+    const legacy = { driver: 's3', bucket: 'b' };
+    expect(resolveStorageCapabilityArg(legacy).options).toBe(legacy);
+  });
+
+  it('falls back when the block names no backend at all', () => {
+    // `config.storage = { presignedTtl: 60 }` configures no backend, so the
+    // local default still applies rather than being replaced by a partial block.
+    expect(resolveStorageCapabilityArg({ presignedTtl: 60 }).options).toEqual({
+      adapter: 'local',
+      local: { rootDir: '.objectstack/data/uploads' },
+    });
+  });
+});

packages/cli/src/commands/serve.ts

@@ -2190,17 +2190,15 @@ export default class Serve extends Command {
             // In production mode we emit a single loud warning so the
             // operator knows to point storage at S3 / GCS / Azure before
             // shipping (data on a single pod is volatile / non-replicated).
-            const cfgStorage = (config as any).storage;
-            if (cfgStorage && (cfgStorage.driver || cfgStorage.adapter)) {
-              arg = cfgStorage;
-            } else {
-              const root = process.env.OS_STORAGE_ROOT || '.objectstack/data/uploads';
-              arg = { driver: 'local', root };
-              if (!isDev) {
-                console.warn(chalk.yellow(
-                  `  ⚠ StorageServicePlugin using local driver (${root}) — switch to S3/GCS/Azure for production (set config.storage or OS_STORAGE_*).`,
-                ));
-              }
+            const storageArg = resolveStorageCapabilityArg(
+              (config as any).storage,
+              process.env.OS_STORAGE_ROOT,
+            );
+            arg = storageArg.options;
+            if (storageArg.localRoot && !isDev) {
+              console.warn(chalk.yellow(
+                `  ⚠ StorageServicePlugin using local driver (${storageArg.localRoot}) — switch to S3/GCS/Azure for production (set config.storage or OS_STORAGE_*).`,
+              ));
             }
           }
           await kernel.use(arg !== undefined ? new Ctor(arg) : new Ctor());
@@ -2649,6 +2647,48 @@ export default class Serve extends Command {
   }
 }
 
+/**
+ * Constructor options for `StorageServicePlugin`, plus the local root to name in
+ * the production warning (absent when the host configured a backend itself).
+ */
+export interface StorageCapabilityArg {
+  options: Record<string, unknown>;
+  localRoot?: string;
+}
+
+/**
+ * Resolve what `StorageServicePlugin` is constructed with (#4096).
+ *
+ * Storage is in the default capability slate, so a host that configures nothing
+ * still gets local disk under `.objectstack/data/uploads/` and avatars /
+ * attachments / report files work out of the box.
+ *
+ * The fallback used to be `{ driver: 'local', root }` — neither of which
+ * `StorageServicePluginOptions` declares. Both were dropped on the floor, so the
+ * plugin applied its OWN default (`./storage`), `OS_STORAGE_ROOT` changed
+ * nothing, and uploads landed somewhere the operator never named. The `storage`
+ * settings namespace then corrected the root on its first read (its manifest
+ * default IS `./.objectstack/data/uploads`), which swapped the adapter and
+ * warned about stranded files — on every boot of a healthy server.
+ *
+ * A caller-supplied `config.storage` is still forwarded verbatim, including the
+ * `driver`/`root` dialect, which the plugin does not read either. That is the
+ * same mismatch one layer up and is tracked separately: correcting it means
+ * deciding whether the plugin accepts that dialect or the config schema is
+ * wrong, and a lenient alias here would fossilize the wrong contract
+ * (AGENTS.md Prime Directive #12).
+ */
+export function resolveStorageCapabilityArg(
+  cfgStorage: any,
+  envRoot?: string,
+): StorageCapabilityArg {
+  if (cfgStorage && (cfgStorage.driver || cfgStorage.adapter)) {
+    return { options: cfgStorage };
+  }
+  const rootDir = envRoot?.trim() || '.objectstack/data/uploads';
+  return { options: { adapter: 'local', local: { rootDir } }, localRoot: rootDir };
+}
+
 /**
  * Best-effort driver introspection.
  *

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

@@ -18,8 +18,16 @@ import { SwappableStorageService } from './swappable-storage-service';
 function makeCtx() {
   const services = new Map<string, any>();
   const hooks: Array<() => Promise<void> | void> = [];
+  // Captured so tests can assert on what a boot SAID, not just what it built —
+  // #4096 is entirely about a warning that should not have been emitted.
+  const logs: { info: string[]; warn: string[]; error: string[] } = { info: [], warn: [], error: [] };
   const ctx: any = {
-    logger: { info: () => {}, warn: () => {}, error: () => {} },
+    logger: {
+      info: (m: string) => { logs.info.push(String(m)); },
+      warn: (m: string) => { logs.warn.push(String(m)); },
+      error: (m: string) => { logs.error.push(String(m)); },
+    },
+    _logs: logs,
     registerService: (name: string, svc: any) => { services.set(name, svc); },
     getService: <T>(name: string): T => {
       const s = services.get(name);
@@ -121,6 +129,103 @@ describe('StorageServicePlugin: settings live-wire', () => {
     expect(proxy.getInner()).toBe(before);
   });
 
+  // ── #4096: the swap decision, and what it says out loud ──────────────────
+  //
+  // `hasAny` is true on every boot once the settings service has persisted its
+  // own defaults, so this used to rebuild and swap the adapter unconditionally
+  // and warn that "existing files were NOT migrated" — about a swap from an
+  // adapter to an identically-configured one, on a healthy server, forever.
+
+  it('neither swaps nor warns when persisted settings match the running adapter', async () => {
+    const dir = await fs.mkdtemp(join(tmpdir(), 'oss-same-'));
+    const plugin = new StorageServicePlugin({
+      adapter: 'local',
+      local: { rootDir: dir },
+      registerRoutes: false,
+    });
+    const ctx = makeCtx();
+    // Exactly what the settings namespace holds after it persists its defaults:
+    // values present (so `hasAny` is true) and identical to what is running.
+    ctx.registerService('settings', makeFakeSettings({ adapter: 'local', local_root: dir }));
+
+    await plugin.init(ctx);
+    await plugin.start(ctx);
+    const proxy = ctx.getService('file-storage') as SwappableStorageService;
+    const before = proxy.getInner();
+
+    await ctx._flushReady();
+
+    expect(proxy.getInner()).toBe(before);
+    expect(ctx._logs.warn.join('\n')).not.toContain('adapter swapped');
+  });
+
+  it('treats an implicit local root and its explicit default as the same store', async () => {
+    // The real boot shape: the host leaves `local.rootDir` unset while the
+    // settings namespace persists the schema default, so the two spellings of
+    // one location must not read as a move.
+    const plugin = new StorageServicePlugin({ adapter: 'local', registerRoutes: false });
+    const ctx = makeCtx();
+    ctx.registerService('settings', makeFakeSettings({ adapter: 'local', local_root: './storage' }));
+
+    await plugin.init(ctx);
+    await plugin.start(ctx);
+    const proxy = ctx.getService('file-storage') as SwappableStorageService;
+    const before = proxy.getInner();
+
+    await ctx._flushReady();
+
+    expect(proxy.getInner()).toBe(before);
+    expect(ctx._logs.warn.join('\n')).not.toContain('adapter swapped');
+  });
+
+  it('still warns when the backing store really moves', async () => {
+    // The guard has to survive the fix: Local → another root strands whatever
+    // the old one held, and that is the whole point of the message.
+    const dirA = await fs.mkdtemp(join(tmpdir(), 'oss-move-a-'));
+    const dirB = await fs.mkdtemp(join(tmpdir(), 'oss-move-b-'));
+    const plugin = new StorageServicePlugin({
+      adapter: 'local',
+      local: { rootDir: dirA },
+      registerRoutes: false,
+    });
+    const ctx = makeCtx();
+    ctx.registerService('settings', makeFakeSettings({ adapter: 'local', local_root: dirB }));
+
+    await plugin.init(ctx);
+    await plugin.start(ctx);
+    await ctx._flushReady();
+
+    const warned = ctx._logs.warn.join('\n');
+    expect(warned).toContain('adapter swapped');
+    expect(warned).toContain('were NOT migrated');
+  });
+
+  it('warns again on a later real move, having stayed quiet for the no-op', async () => {
+    // The verdict is per-swap state, so a quiet boot must not disarm the next
+    // genuine migration.
+    const dir = await fs.mkdtemp(join(tmpdir(), 'oss-seq-'));
+    const moved = await fs.mkdtemp(join(tmpdir(), 'oss-seq-moved-'));
+    const plugin = new StorageServicePlugin({
+      adapter: 'local',
+      local: { rootDir: dir },
+      registerRoutes: false,
+    });
+    const ctx = makeCtx();
+    const settings = makeFakeSettings({ adapter: 'local', local_root: dir });
+    ctx.registerService('settings', settings);
+
+    await plugin.init(ctx);
+    await plugin.start(ctx);
+    await ctx._flushReady();
+    expect(ctx._logs.warn.join('\n')).not.toContain('adapter swapped');
+
+    settings.values = { adapter: 'local', local_root: moved };
+    settings._emit('storage');
+    await new Promise((r) => setTimeout(r, 20));
+
+    expect(ctx._logs.warn.join('\n')).toContain('adapter swapped');
+  });
+
   it('registers a working storage/test action handler that round-trips a probe blob', async () => {
     const dir = await fs.mkdtemp(join(tmpdir(), 'oss-probe-'));
     const plugin = new StorageServicePlugin({