fix(service-storage,service-sms): converge the authored-value criterion on ResolvedSettingValue.source (#14135)

A schema default (source: 'default') is not a decision anyone made. The
storage swap gate now requires an authored adapter-relevant key before
settings may override the constructor-built adapter; the sms downgrade to
LogSmsTransport now requires an operator-authored provider selection.
Same criterion, same reason as EmailServicePlugin. The sms daily_quota
reader keeps its declared by-value exception.


Claude-Session: https://claude.ai/code/session_016ZC5rNQj3WEet5HAmmAkMs

Co-authored-by: Claude <noreply@anthropic.com>

Author: claude[bot]

.changeset/settings-authored-source-convergence.md

@@ -0,0 +1,29 @@
+---
+"@objectstack/service-storage": patch
+"@objectstack/service-sms": patch
+---
+
+fix(service-storage,service-sms): a schema default is not a configuration — converge the "authored value" criterion on `ResolvedSettingValue.source` (#5536)
+
+Both settings-bound plugins decided "has anyone configured this namespace?" by
+value presence, but the manifest defaults are non-empty on every boot, so an
+unopened settings page read as configuration:
+
+- **service-storage**: the swap gate now requires an adapter-relevant key
+  (`adapter`, `local_root`, `s3_*` — exactly the inputs `resolveStorageTarget`
+  reads) whose `source` is not `'default'` before settings may override the
+  constructor-built adapter. Previously the schema defaults
+  (`adapter: 'local'`, `local_root: './.objectstack/data/uploads'`) could
+  silently move a deployment's declared backing store — and an authored save
+  that touched only, say, the upload limit could open the same door.
+- **service-sms**: the downgrade to `LogSmsTransport` now requires an
+  operator-authored `provider: 'log'` (`source !== 'default'`). Previously
+  the manifest default `'log'` — a value nobody selected — switched off the
+  transport the deployment declared via constructor options on every boot.
+
+Same criterion, same reason as `EmailServicePlugin` (the in-repo precedent):
+the manifest default (`source: 'default'`) is not a decision anyone made.
+Admin-saved rows and env overrides behave exactly as before. A snapshot with
+no `source` at all reads as `'default'` — the conservative side keeps the
+deployment-declared adapter/transport. The sms `daily_quota` reader keeps its
+declared #2814 exception and still binds by value, never by source.

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

@@ -9,19 +9,34 @@ import { AliyunSmsTransport, TwilioSmsTransport } from './transports/index.js';
  * Lightweight fake PluginContext (service registry + kernel:ready hooks +
  * a fake settings service) — mirrors the messaging plugin's test harness.
  */
-function fakeCtx(opts: { settingsValues?: Record<string, unknown> } = {}) {
+function fakeCtx(opts: {
+  settingsValues?: Record<string, unknown>;
+  /**
+   * Per-key `ResolvedSettingValue.source`. The real service resolves every
+   * key to `{ value, source, … }` — a fake that dropped `source` would model
+   * the exact defect #5536 fixed. Default `'global'` = an admin-saved row,
+   * which is what every pre-existing test here meant; `'default'` pins the
+   * schema-default shape and `null` omits the field (a non-conforming
+   * snapshot).
+   */
+  settingsSources?: Record<string, string | null>;
+} = {}) {
   const services = new Map<string, unknown>();
   const readyHooks: Array<() => Promise<void> | void> = [];
   const actions = new Map<string, (input: any) => Promise<any>>();
   const subscriptions: Array<{ ns: string; fn: () => void }> = [];
   let values = opts.settingsValues;
+  const sources = opts.settingsSources;
 
   if (values !== undefined) {
     services.set('settings', {
       async getNamespace(ns: string) {
         if (ns !== 'sms') throw new Error('unknown namespace');
-        const wrapped: Record<string, { value: unknown }> = {};
-        for (const [k, v] of Object.entries(values ?? {})) wrapped[k] = { value: v };
+        const wrapped: Record<string, { value: unknown; source?: string }> = {};
+        for (const [k, v] of Object.entries(values ?? {})) {
+          const src = sources && k in sources ? sources[k] : 'global';
+          wrapped[k] = src === null ? { value: v } : { value: v, source: src };
+        }
         return { values: wrapped };
       },
       subscribe(ns: string, fn: () => void) { subscriptions.push({ ns, fn }); return () => {}; },
@@ -154,6 +169,93 @@ describe('SmsServicePlugin', () => {
   });
 });
 
+describe('SmsServicePlugin — authored-value criterion (#5536)', () => {
+  // The two criteria — old "values.provider === 'log'" and new
+  // "source !== 'default'" — AGREE whenever an operator actually saved the
+  // row, so these tests sit deliberately on the disagreement: the manifest
+  // default `'log'`, a non-empty value nobody authored.
+  const twilioOpts = {
+    provider: 'twilio' as const,
+    providerOptions: { accountSid: 'AC1', authToken: 't', from: '+15005550006' },
+  };
+
+  it('keeps the constructor-declared transport when provider is only the schema default', async () => {
+    // THE card: `provider: 'log'` is non-empty, so the old criterion read it
+    // as an operator selection and downgraded the deployment's declared
+    // transport to LogSmsTransport on every boot of an unopened settings
+    // page; `source: 'default'` says nobody made that decision.
+    const harness = fakeCtx({
+      settingsValues: { provider: 'log', daily_quota: 0 },
+      settingsSources: { provider: 'default', daily_quota: 'default' },
+    });
+    const plugin = new SmsServicePlugin(twilioOpts);
+    await plugin.init(harness.ctx);
+    await plugin.start(harness.ctx);
+    await harness.fireReady();
+
+    const svc = harness.services.get('sms') as SmsService;
+    expect(svc.options.transport).toBeInstanceOf(TwilioSmsTransport);
+    expect(svc.isConfigured()).toBe(true);
+  });
+
+  it('still downgrades when an operator actually saved provider=log (control)', async () => {
+    // An admin-authored row: both criteria answer "configured", so this pins
+    // byte-identical behaviour — a declared control, not fix evidence.
+    const harness = fakeCtx({
+      settingsValues: { provider: 'log' },
+      settingsSources: { provider: 'global' },
+    });
+    const plugin = new SmsServicePlugin(twilioOpts);
+    await plugin.init(harness.ctx);
+    await plugin.start(harness.ctx);
+    await harness.fireReady();
+
+    const svc = harness.services.get('sms') as SmsService;
+    expect(svc.options.transport).toBeInstanceOf(LogSmsTransport);
+    expect(svc.isConfigured()).toBe(false);
+  });
+
+  it('a snapshot with no source at all keeps the declared transport (reverse control)', async () => {
+    // A non-conforming snapshot (no `source` anywhere) must land on the
+    // conservative side, and the conservative side here KEEPS the transport
+    // the deployment declared delivering: the other side's failure mode is
+    // silently routing every send to the log transport on unattributable
+    // authorship. An operator can still downgrade explicitly — a real save
+    // always carries `source`.
+    const harness = fakeCtx({
+      settingsValues: { provider: 'log' },
+      settingsSources: { provider: null },
+    });
+    const plugin = new SmsServicePlugin(twilioOpts);
+    await plugin.init(harness.ctx);
+    await plugin.start(harness.ctx);
+    await harness.fireReady();
+
+    const svc = harness.services.get('sms') as SmsService;
+    expect(svc.options.transport).toBeInstanceOf(TwilioSmsTransport);
+  });
+
+  it('daily_quota still binds by VALUE at source=default — the #2814 declination stands', async () => {
+    // `sms-plugin.ts` deliberately declines to read `source` for the daily
+    // cost ceiling (its comment cites #5536 by number): env-locked,
+    // admin-saved and defaulted quotas are the same instruction to that
+    // reader. Pinned so the authored-value criterion never creeps into the
+    // quota path.
+    const harness = fakeCtx({
+      settingsValues: { provider: 'log', daily_quota: 1 },
+      settingsSources: { provider: 'default', daily_quota: 'default' },
+    });
+    const plugin = new SmsServicePlugin();
+    await plugin.init(harness.ctx);
+    await plugin.start(harness.ctx);
+    await harness.fireReady();
+
+    const svc = harness.services.get('sms') as SmsService;
+    expect((await svc.send({ to: '+8613800000001', body: 'a' })).status).toBe('sent');
+    expect((await svc.send({ to: '+8613800000002', body: 'b' })).status).toBe('failed');
+  });
+});
+
 describe('SmsServicePlugin — daily quota binding (#2814)', () => {
   it('applies sms.daily_quota from settings at kernel:ready', async () => {
     const harness = fakeCtx({ settingsValues: { provider: 'log', daily_quota: 2 } });

packages/services/service-sms/src/sms-plugin.ts

@@ -226,8 +226,10 @@ export class SmsServicePlugin implements Plugin {
           try {
             const payload = await settings.getNamespace('sms');
             const values: Record<string, unknown> = {};
+            const sources: Record<string, string> = {};
             for (const [k, v] of Object.entries(payload.values as Record<string, any>)) {
               values[k] = v?.value;
+              if (v?.source) sources[k] = String(v.source);
             }
             // #2814 — the daily cost ceiling binds for EVERY composition,
             // including a host-injected transport: "how much may this
@@ -239,7 +241,7 @@ export class SmsServicePlugin implements Plugin {
             this.dailyQuota?.setQuota(values.daily_quota);
             // A host-injected transport, by contrast, IS authoritative — the
             // settings form only manages the provider-tag path.
-            if (!this.options.transport) this.applySmsSettings(values, ctx);
+            if (!this.options.transport) this.applySmsSettings(values, sources, ctx);
           } catch (err: any) {
             ctx.logger.warn('SmsServicePlugin: failed to apply sms settings: ' + (err?.message ?? err));
           }
@@ -313,7 +315,11 @@ export class SmsServicePlugin implements Plugin {
    * on the running SmsService. Incomplete credentials keep the previous
    * transport (with a warning) so a half-saved form can't break delivery.
    */
-  private applySmsSettings(values: Record<string, unknown>, ctx: PluginContext): void {
+  private applySmsSettings(
+    values: Record<string, unknown>,
+    sources: Record<string, string>,
+    ctx: PluginContext,
+  ): void {
     if (!this.service) return;
     const resolved = providerFromSettings(values);
     if (resolved.missing) {
@@ -323,9 +329,19 @@ export class SmsServicePlugin implements Plugin {
       return;
     }
     if (resolved.provider === 'log') {
-      // Downgrade to the dev transport only when the operator explicitly
-      // selected `log`; an unset namespace keeps the constructor opts.
-      if (values.provider === 'log') {
+      // [#5536] Downgrade to the dev transport only when an operator (or an
+      // env override) actually AUTHORED the `log` selection —
+      // `values.provider === 'log'` alone cannot tell that apart from the
+      // manifest default, which resolves to `'log'` on every boot whether or
+      // not anyone ever opened the settings page. Same criterion, same
+      // reason as EmailServicePlugin: the manifest default
+      // (`source: 'default'`) is not a decision anyone made, and treating it
+      // as one would let a settings page nobody opened silently switch off
+      // the transport the deployment declared via constructor options. A
+      // missing `source` (a non-conforming snapshot) reads as 'default': the
+      // conservative side keeps the deployment's declared transport
+      // delivering.
+      if (values.provider === 'log' && (sources.provider ?? 'default') !== 'default') {
         this.service.setTransport(new LogSmsTransport(ctx.logger), false);
         ctx.logger.info('SmsServicePlugin: sms settings applied (provider=log; SMS will NOT be sent).');
       }

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

@@ -43,16 +43,29 @@ function makeCtx() {
   return ctx;
 }
 
-function makeFakeSettings(initialValues: Record<string, any>) {
+// The real service resolves every key to a full `ResolvedSettingValue`
+// (`{ value, source, … }`) — a fake that dropped `source` would model the
+// exact defect #5536 fixed. Default `'global'` = an admin-saved row, which is
+// what every pre-existing test here meant; the `sources` overrides pin the
+// schema-default (`'default'`) and missing-source (`null` → key omitted)
+// shapes per key.
+function makeFakeSettings(
+  initialValues: Record<string, any>,
+  initialSources?: Record<string, string | null>,
+) {
   let values = { ...initialValues };
+  const sources = initialSources;
   const subs: Array<(ns: string) => void> = [];
   const actions = new Map<string, (input: any) => Promise<any>>();
   return {
     set values(v: Record<string, any>) { values = v; },
     get values() { return values; },
     createClient: (_ns: string) => ({}),
     getNamespace: async (_ns: string) => ({
-      values: Object.fromEntries(Object.entries(values).map(([k, v]) => [k, { value: v }])),
+      values: Object.fromEntries(Object.entries(values).map(([k, v]) => {
+        const src = sources && k in sources ? sources[k] : 'global';
+        return [k, src === null ? { value: v } : { value: v, source: src }];
+      })),
     }),
     subscribe: (ns: string, fn: () => void) => { subs.push((n) => { if (n === ns) fn(); }); },
     registerAction: (ns: string, id: string, fn: (input: any) => Promise<any>) => {
@@ -101,6 +114,9 @@ describe('StorageServicePlugin: settings live-wire', () => {
     expect(alias).toBe(canonical);
   });
 
+  // #5536 control: an admin-saved row (`source: 'global'`, the fake's
+  // default) is "configured" under BOTH the old value-presence criterion and
+  // the new authored-source one — behaviour must stay byte-identical.
   it('swaps the inner adapter when storage settings change', async () => {
     const dirA = await fs.mkdtemp(join(tmpdir(), 'oss-a-'));
     const dirB = await fs.mkdtemp(join(tmpdir(), 'oss-b-'));
@@ -150,12 +166,101 @@ describe('StorageServicePlugin: settings live-wire', () => {
     expect(proxy.getInner()).toBe(before);
   });
 
+  // ── #5536: only an AUTHORED value may move the backing store ─────────────
+  //
+  // The two criteria — old "any value non-empty" and new
+  // "source !== 'default'" — AGREE whenever an admin actually saved a value,
+  // so the tests below sit deliberately on the disagreement: a non-empty
+  // value whose source says nobody authored it.
+
+  it('keeps the constructor adapter when every value is a schema default (#5536)', async () => {
+    const dirA = await fs.mkdtemp(join(tmpdir(), 'oss-default-'));
+    const plugin = new StorageServicePlugin({
+      adapter: 'local',
+      local: { rootDir: dirA },
+      registerRoutes: false,
+    });
+    const ctx = makeCtx();
+    // Non-empty values (the manifest defaults) that nobody ever authored:
+    // the old "value present" criterion read this as configured and swapped
+    // the deployment's adapter to the schema default; `source: 'default'`
+    // says no one made that decision.
+    const settings = makeFakeSettings(
+      { adapter: 'local', local_root: './.objectstack/data/uploads', max_upload_mb: 100 },
+      { adapter: 'default', local_root: 'default', max_upload_mb: 'default' },
+    );
+    ctx.registerService('settings', settings);
+
+    await plugin.init(ctx);
+    await plugin.start(ctx);
+    const proxy = ctx.getService('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('an authored save that touched no adapter key does not open the swap gate (#5536)', async () => {
+    // The criterion is per-decision, exactly as plugin-email applies it per
+    // key: an admin who saved only the upload limit did not author the
+    // schema-default adapter, so the constructor's store stays.
+    const dirA = await fs.mkdtemp(join(tmpdir(), 'oss-limits-'));
+    const plugin = new StorageServicePlugin({
+      adapter: 'local',
+      local: { rootDir: dirA },
+      registerRoutes: false,
+    });
+    const ctx = makeCtx();
+    const settings = makeFakeSettings(
+      { adapter: 'local', local_root: './.objectstack/data/uploads', max_upload_mb: 50 },
+      { adapter: 'default', local_root: 'default', max_upload_mb: 'global' },
+    );
+    ctx.registerService('settings', settings);
+
+    await plugin.init(ctx);
+    await plugin.start(ctx);
+    const proxy = ctx.getService('storage') as SwappableStorageService;
+    const before = proxy.getInner();
+    await ctx._flushReady();
+    expect(proxy.getInner()).toBe(before);
+  });
+
+  it('a snapshot with no source at all lands on the no-swap side (#5536 reverse control)', async () => {
+    // An un-upgraded / non-conforming settings implementation that omits
+    // `source` must land on the conservative side, and the conservative side
+    // here is the one that does NOT move the backing store on unattributable
+    // authorship: files stay reachable through the adapter the deployment
+    // declared, and a real admin save (which always carries `source`) can
+    // still trigger the swap via the subscription.
+    const dirA = await fs.mkdtemp(join(tmpdir(), 'oss-nosrc-a-'));
+    const dirB = await fs.mkdtemp(join(tmpdir(), 'oss-nosrc-b-'));
+    const plugin = new StorageServicePlugin({
+      adapter: 'local',
+      local: { rootDir: dirA },
+      registerRoutes: false,
+    });
+    const ctx = makeCtx();
+    const settings = makeFakeSettings(
+      { adapter: 'local', local_root: dirB },
+      { adapter: null, local_root: null },
+    );
+    ctx.registerService('settings', settings);
+
+    await plugin.init(ctx);
+    await plugin.start(ctx);
+    const proxy = ctx.getService('storage') as SwappableStorageService;
+    const before = proxy.getInner();
+    await ctx._flushReady();
+    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.
+  // The old value-presence gate (now the #5536 authored gate) was true on
+  // every boot once an admin save had persisted the namespace, 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-'));