From a1f8f0740a0faf3cfcd9332a389ca52199285751 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 22 Aug 2026 12:45:25 +0000 Subject: [PATCH] fix(settings): declare the settings ordering edge and make the pre-bind read audible (#10250) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `SettingsServicePlugin` binds its data engine from a `kernel:ready` hook registered in its `start()`. `plugin-email`, `service-sms` and `service-storage` each read a settings namespace from a `kernel:ready` hook registered in THEIR `start()` — and none of them declared any dependency on `com.objectstack.service.settings`, so their position relative to the bind was pure `kernel.use()` order. A reader that started first read the in-memory fallback, which is empty at boot, and received the manifest DEFAULTS with `locked: false` and no diagnostic while the operator's saved row sat unread in `sys_setting`. - each of the three declares `optionalDependencies: ['com.objectstack.service.settings']` — soft, order-if-present (ADR-0116), so the kernel orders the bind ahead of the read in every host, and a kernel with no settings service still boots them unchanged. - `SettingsService` reports one operator-actionable `warn` per namespace when a read lands inside a declared-but-pending bind window. Not a refusal: an in-window read of a setting with no persisted row must answer the manifest default. Silent after `bindEngine`, on a kernel with no `objectql` (`settleWithoutEngine`), for a directly constructed `SettingsService`, and for a read satisfied by an `OS_*` override. - a new pin derives the slate boundary from what the pin is for: every always-on entry that is not one of the services others bind into at `kernel:ready` must be mounted after all of them. The previous assertion covered `slice(0, 6)` while `sms` — a settings reader — sits at index 6. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx --- .changeset/settings-prebind-read-ordering.md | 53 +++ .../serve-settings-ordering.pin.test.ts | 258 ++++++++++ .../plugins/plugin-email/src/email-plugin.ts | 27 ++ .../src/settings-prebind-read-warning.test.ts | 445 ++++++++++++++++++ .../service-settings/src/settings-service.ts | 76 +++ .../src/settings-service.types.ts | 8 + .../services/service-sms/src/sms-plugin.ts | 26 + .../src/storage-service-plugin.ts | 40 +- 8 files changed, 928 insertions(+), 5 deletions(-) create mode 100644 .changeset/settings-prebind-read-ordering.md create mode 100644 packages/cli/src/commands/serve-settings-ordering.pin.test.ts create mode 100644 packages/services/service-settings/src/settings-prebind-read-warning.test.ts diff --git a/.changeset/settings-prebind-read-ordering.md b/.changeset/settings-prebind-read-ordering.md new file mode 100644 index 0000000000..7e1bec1815 --- /dev/null +++ b/.changeset/settings-prebind-read-ordering.md @@ -0,0 +1,53 @@ +--- +"@objectstack/plugin-email": patch +"@objectstack/service-sms": patch +"@objectstack/service-storage": patch +"@objectstack/service-settings": patch +--- + +Make the settings ordering contract **declared and enforced**, and make the +residual pre-bind READ audible (#10250). + +`SettingsServicePlugin` binds its data engine from a `kernel:ready` hook +registered in its `start()`. Three shipped plugins read a settings namespace +from a `kernel:ready` hook registered in *their* `start()` — `plugin-email` +(`mail`: SMTP/provider/from-address), `service-sms` (`sms`: provider +credentials and the daily cost ceiling) and `service-storage` (`storage`: +backend and credentials). Hooks fire in registration order, so a reader that +started before the settings plugin read `SettingsService`'s in-memory fallback, +which is empty at boot: the caller received the manifest **default** with +`source: 'default'` and `locked: false`, no diagnostic anywhere, while the +operator's saved row sat unread in `sys_setting`. + +Nothing constrained that order. None of the three declared any dependency on +`com.objectstack.service.settings`, so their position was pure `kernel.use()` +order. It was correct under `os serve` only because the always-on slate happens +to list `settings` first — and `serve` *prepends* an app's declared `requires`, +so an ordinary `requires: ['email']` produced email-before-settings and bypassed +that; cloud's per-tenant runtime mounts the slate from its own wiring. + +Three changes, one contract: + +- **Declared order.** Each of the three plugins now declares + `optionalDependencies: ['com.objectstack.service.settings']`. The kernel + resolves both the init and the start phase from that graph + (`resolvePluginOrder`, ADR-0116), so the bind is ordered ahead of the read + wherever the plugin is composed, in any host. Soft, not hard: a kernel with + no settings service still boots these plugins unchanged. +- **The residual is audible.** A settings read issued while a bind is + *declared but pending* now emits one operator-actionable `warn` per namespace + naming the repair. Deliberately not a refusal — an in-window read of a + setting with genuinely no persisted row must answer the manifest default, and + refusing would turn a correct startup sequence into an error. It stays silent + in every case that is not the window: after `bindEngine`, on a kernel with no + `objectql` at all (`settleWithoutEngine`), for a directly constructed + `SettingsService`, and for a read satisfied by an `OS_*` env override. +- **The slate pin now derives its boundary.** The foundational-prefix + assertion covered `slice(0, 6)` while `sms` — a settings reader — sits at + index 6, one past the end. The new pin + (`packages/cli/src/commands/serve-settings-ordering.pin.test.ts`) states the + rule instead of the count: every always-on entry that is not one of the + services others bind into at `kernel:ready` must be mounted after all of + them. An entry added tomorrow is covered wherever it lands. + +No behaviour changes for a deployment whose order was already correct. diff --git a/packages/cli/src/commands/serve-settings-ordering.pin.test.ts b/packages/cli/src/commands/serve-settings-ordering.pin.test.ts new file mode 100644 index 0000000000..1320254473 --- /dev/null +++ b/packages/cli/src/commands/serve-settings-ordering.pin.test.ts @@ -0,0 +1,258 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The settings ORDERING contract (#10250) — declared, not incidental. + * + * ## What went wrong + * + * `SettingsServicePlugin` binds its data engine from a `kernel:ready` hook + * registered in its `start()`. Three shipped plugins read a settings namespace + * from a `kernel:ready` hook registered in THEIR `start()`: + * + * `plugin-email` → `getNamespace('mail')` — SMTP / provider / from-address + * `service-sms` → `getNamespace('sms')` — provider credentials, cost ceiling + * `service-storage`→ `getNamespace('storage')` — backend + credentials + * + * Hooks fire in registration order, so whichever plugin STARTS first registers + * the earlier hook. A reader that started before the settings plugin therefore + * read `SettingsService`'s in-memory fallback — empty at boot — and received the + * manifest DEFAULTS with `locked: false` and no diagnostic, while the operator's + * saved row sat unread in `sys_setting`. + * + * Until this change **nothing constrained that order**. None of the three + * declared any dependency on `com.objectstack.service.settings`, so their + * position was pure `kernel.use()` order. It happened to be right under + * `os serve` only because the always-on slate lists `settings` before them — + * and `serve` PREPENDS an app's declared `requires`, so an ordinary + * `requires: ['email']` produced email-before-settings and bypassed that + * entirely. Cloud's objectos-runtime mounts the slate from its own wiring, + * which is why a CLI-only repair was rejected. + * + * ## Why this file lives in `@objectstack/cli` + * + * It needs the REAL plugin classes and the REAL slate in one place. `cli` is the + * only package that depends on all of them, and it is the runtime that actually + * appends `PLATFORM_ALWAYS_ON_CAPABILITIES` to an app's `requires` + * (`Serve.ALWAYS_ON_CAPABILITIES` is a re-export of it). + * + * ⚠️ **Resolution: these imports reach `dist/`, deliberately.** Every specifier + * below is a bare workspace package listed in + * `KNOWN_UNALIASED_TEST_IMPORTS['@objectstack/cli']` + * (`scripts/check-test-source-alias.mjs`), and `packages/cli/vitest.config.ts` + * aliases only `@objectstack/service-cache` and `create-objectstack/created-summary`. + * That registry is ⛔ SHRINK-ONLY and aliasing a fourth dependency to source + * would pull its whole import surface into this package's resolution domain for + * all ~137 test files (the #7378 shape). So this file reads BUILT plugin + * classes: a change to any of the three manifests needs + * `pnpm --filter build` before this file can see it, and an ablation of + * one of them is only believable if the rebuild is proved to have reached + * `dist/`. + * + * ## What each case pins, and why it cannot pass vacuously + * + * 1. the three shipped readers each DECLARE the edge; + * 2. the declaration MOVES resolution order — proved by deleting it from a live + * instance and watching the order revert, not by asserting the key exists + * (ADR-0049: a declaration nothing reads is the defect, not the fix); + * 3. the slate keeps every non-foundational entry after the services that get + * bound into during `kernel:ready` — a boundary DERIVED from what the pin is + * for, so an eleventh entry added tomorrow is covered rather than one past + * the end of a hard-coded slice. + */ + +import { describe, it, expect } from 'vitest'; +import { resolvePluginOrder } from '@objectstack/core'; +import type { OrderablePlugin } from '@objectstack/core'; +import { PLATFORM_ALWAYS_ON_CAPABILITIES } from '@objectstack/spec/kernel'; +import { EmailServicePlugin } from '@objectstack/plugin-email'; +import { SmsServicePlugin } from '@objectstack/service-sms'; +import { StorageServicePlugin } from '@objectstack/service-storage'; +import { SettingsServicePlugin } from '@objectstack/service-settings'; + +const SETTINGS_PLUGIN = 'com.objectstack.service.settings'; + +/** + * The always-on capability tokens whose provider plugin reads a settings + * namespace from its own `kernel:ready` hook, paired with that plugin. + * + * Enumerated rather than discovered because the discovery would have to import + * all ten always-on providers (see the resolution note above). Case 4 is what + * keeps the enumeration honest in the direction that matters: a settings-reading + * plugin that FORGETS the declaration drops out of this file's derived set, and + * case 1's floor turns red. + */ +const SETTINGS_READING_ALWAYS_ON: ReadonlyArray<{ token: string; plugin: () => OrderablePlugin }> = [ + { token: 'email', plugin: () => new EmailServicePlugin() as unknown as OrderablePlugin }, + { token: 'sms', plugin: () => new SmsServicePlugin() as unknown as OrderablePlugin }, + { token: 'storage', plugin: () => new StorageServicePlugin() as unknown as OrderablePlugin }, +]; + +/** + * `EmailServicePlugin` declares `com.objectstack.engine.objectql` a HARD + * dependency, so every registry below has to contain it or `resolvePluginOrder` + * throws before it can order anything. Name-only: this module orders plugins by + * their declarations and never runs a lifecycle. + */ +const engineStub = (): OrderablePlugin => ({ name: 'com.objectstack.engine.objectql' }); + +const newSettingsPlugin = (): OrderablePlugin => + new SettingsServicePlugin({ + registerRoutes: false, manifests: [], actionHandlers: {}, + }) as unknown as OrderablePlugin; + +/** Registry in the given insertion order — `resolvePluginOrder` preserves it + * for plugins with no edges between them, which is what makes the hostile + * order below hostile. */ +const registryOf = (...plugins: OrderablePlugin[]) => + new Map(plugins.map((p) => [p.name, p])); + +/** + * The always-on entries that other entries BIND INTO during `kernel:ready`, in + * the order they must be mounted. This is what the foundational prefix was + * always for — the spec-side pin's own comment says so: *"Order matters at mount + * time: settings/queue/job must precede the services that bind to them during + * their own `kernel:ready` phase."* + * + * Stating the prefix as a ROLE rather than as a count is the whole point of + * case 5. `PLATFORM_ALWAYS_ON_CAPABILITIES.slice(0, 6)` bundled the four + * bind-TARGETS together with two of their readers (`email`, `storage`) and + * stopped one short of the third (`sms`, at index 6) — an off-by-one that reads + * as correct at a glance and left `sms`'s position held by nothing. + */ +const BIND_TARGETS = ['queue', 'job', 'cache', 'settings'] as const; + +const slateIndex = (token: string) => PLATFORM_ALWAYS_ON_CAPABILITIES.indexOf(token); + +describe('settings-reading plugins declare the ordering edge (ADR-0116)', () => { + it('1. all three shipped always-on readers declare `com.objectstack.service.settings`', () => { + // The floor. An empty or shrunken derived set would make cases 2-3 pass + // over nothing, so the count is asserted before anything is derived from it. + expect(SETTINGS_READING_ALWAYS_ON).toHaveLength(3); + expect(SETTINGS_READING_ALWAYS_ON.map((r) => r.token).sort()).toEqual( + ['email', 'sms', 'storage'], + ); + + for (const { token, plugin } of SETTINGS_READING_ALWAYS_ON) { + const declared = (plugin() as { optionalDependencies?: string[] }).optionalDependencies ?? []; + expect(declared, `${token} must declare the settings ordering edge`).toContain( + SETTINGS_PLUGIN, + ); + } + }); + + it('2. the declaration MOVES resolution order — settings inits/starts first even when used last', () => { + // `ObjectKernel.bootstrap` and `LiteKernel.bootstrap` both iterate the + // SAME `resolvePluginOrder` output for Phase 1 (init) and Phase 2 (start), + // so this is the order the `kernel:ready` hooks get registered in. + for (const { token, plugin } of SETTINGS_READING_ALWAYS_ON) { + const reader = plugin(); + // Hostile insertion order: the READER first, the settings plugin last. + const registry = registryOf(engineStub(), reader, newSettingsPlugin()); + const ordered = resolvePluginOrder(registry).map((p) => p.name); + expect( + ordered.indexOf(SETTINGS_PLUGIN), + `${token}: settings must resolve ahead of the reader`, + ).toBeLessThan(ordered.indexOf(reader.name)); + } + }); + + it('3. …and the declaration is what does it — forget it and the order reverts', () => { + // The ADR-0049 half. Case 2 alone would still pass if `resolvePluginOrder` + // happened to hoist by some other rule; this proves the DECLARATION is the + // cause by removing it from a live instance and re-resolving. + for (const { token, plugin } of SETTINGS_READING_ALWAYS_ON) { + const reader = plugin(); + // Storage declares TWO optional edges; drop only the settings one so the + // objectql edge it shares with the other readers is not what changes. + const kept = ((reader as { optionalDependencies?: string[] }).optionalDependencies ?? []) + .filter((d) => d !== SETTINGS_PLUGIN); + (reader as { optionalDependencies?: string[] }).optionalDependencies = kept; + + const registry = registryOf(engineStub(), reader, newSettingsPlugin()); + const ordered = resolvePluginOrder(registry).map((p) => p.name); + // Insertion order is preserved for plugins with no edges between them — + // so with the declaration gone the reader is back in front, which is the + // defect this card was filed about. + expect( + ordered.indexOf(reader.name), + `${token}: without the declaration the reader must come back first — ` + + 'if this passes, case 2 was not measuring the declaration', + ).toBeLessThan(ordered.indexOf(SETTINGS_PLUGIN)); + } + }); + + it('4. the edge is SOFT — a kernel with no settings plugin still resolves', () => { + // `optionalDependencies` is order-if-present. A hard dependency here would + // refuse to boot every metadata-only / lean kernel that composes email or + // storage without a settings service. + for (const { token, plugin } of SETTINGS_READING_ALWAYS_ON) { + const reader = plugin(); + const registry = registryOf(engineStub(), reader); + expect( + () => resolvePluginOrder(registry), + `${token}: the settings edge must not be hard`, + ).not.toThrow(); + expect(resolvePluginOrder(registry).map((p) => p.name)).toContain(reader.name); + } + }); +}); + +describe('the always-on slate mounts bind targets before everything else', () => { + it('5. every non-foundational entry — `sms` included — comes after ALL bind targets', () => { + // THE DERIVED BOUNDARY. Not "the first six": the rule is that the services + // other entries bind into during `kernel:ready` are mounted first, and + // everything else grows after them. An eleventh always-on entry added + // tomorrow is covered by this the moment it is added, wherever it goes — + // and one inserted BEFORE `settings` turns this red. + for (const target of BIND_TARGETS) { + expect(slateIndex(target), `${target} must be on the slate`).toBeGreaterThanOrEqual(0); + } + const lastTarget = Math.max(...BIND_TARGETS.map(slateIndex)); + + const tail = PLATFORM_ALWAYS_ON_CAPABILITIES.filter( + (c) => !(BIND_TARGETS as readonly string[]).includes(c), + ); + // Non-vacuity: the tail is what this case is about, so an empty one is a + // pass over nothing. + expect(tail.length).toBeGreaterThan(0); + for (const token of tail) { + expect( + slateIndex(token), + `${token} is mounted before a service it may bind into at kernel:ready`, + ).toBeGreaterThan(lastTarget); + } + }); + + it('6. `sms` specifically — the entry the old `slice(0, 6)` pin stopped one short of', () => { + // Kept as its own case because the off-by-one is the thing that reads as + // correct at a glance. `sms` is at index 6; the pinned prefix was + // `slice(0, 6)`, i.e. indices 0-5. + expect(slateIndex('sms')).toBeGreaterThan(slateIndex('settings')); + expect(slateIndex('sms')).toBeGreaterThanOrEqual(6); + }); + + it('7. every declared settings reader on the slate is mounted after `settings`', () => { + const settingsAt = slateIndex('settings'); + expect(settingsAt).toBeGreaterThanOrEqual(0); + for (const { token } of SETTINGS_READING_ALWAYS_ON) { + expect(slateIndex(token), `${token} must be on the slate`).toBeGreaterThanOrEqual(0); + expect(slateIndex(token), `${token} must be mounted after settings`).toBeGreaterThan( + settingsAt, + ); + } + }); + + it('8. the invariant is falsifiable — a hostile slate is reported, not shrugged off', () => { + // The positive control for case 5's derivation. Run the SAME predicate over + // a slate with `settings` moved to the end: it must name every reader that + // now precedes it. A predicate that reported nothing here would report + // nothing on a real regression either. + const hostile = ['email', 'sms', 'storage', 'queue', 'job', 'cache', 'settings']; + const at = (t: string) => hostile.indexOf(t); + const lastTarget = Math.max(...BIND_TARGETS.map(at)); + const violations = hostile.filter( + (c) => !(BIND_TARGETS as readonly string[]).includes(c) && at(c) < lastTarget, + ); + expect(violations).toEqual(['email', 'sms', 'storage']); + }); +}); diff --git a/packages/plugins/plugin-email/src/email-plugin.ts b/packages/plugins/plugin-email/src/email-plugin.ts index 5773c297bf..11b7ac288f 100644 --- a/packages/plugins/plugin-email/src/email-plugin.ts +++ b/packages/plugins/plugin-email/src/email-plugin.ts @@ -266,6 +266,33 @@ export class EmailServicePlugin implements Plugin { version = '1.0.0'; type = 'standard'; dependencies = ['com.objectstack.engine.objectql']; + /** + * Order-if-present on the settings service (ADR-0116, #10250). + * + * `start()` registers a `kernel:ready` hook that reads the `mail` namespace + * (SMTP host/port, provider, from-address). `SettingsServicePlugin` binds its + * data engine from ITS `kernel:ready` hook, registered in ITS `start()` — so + * whichever of the two plugins starts first registers the earlier hook, and + * the earlier hook runs first. Start order is the topological order over this + * declaration (`resolvePluginOrder`, used for BOTH phases in + * `ObjectKernel.bootstrap`), so declaring the edge is what puts the bind + * ahead of the read. + * + * Without it the ordering was incidental: `serve` happens to append the + * always-on slate with `settings` ahead of `email`, but an app that declares + * `requires: ['email']` gets email FIRST (declared tokens are prepended, the + * slate only appended), and cloud's objectos-runtime mounts from its own + * wiring. In that window `getNamespace('mail')` reads the in-memory fallback + * and answers with the manifest DEFAULTS while the operator's saved SMTP row + * sits unread in `sys_setting` — silently, on a transport that then delivers + * nowhere. + * + * SOFT, not hard: a kernel with no settings service must still boot this + * plugin (the transport is buildable from env/options alone). ADR-0049 — + * declared is enforced: `serve-settings-ordering.pin.test.ts` boots a kernel + * with the readers registered BEFORE settings and proves the order moves. + */ + optionalDependencies = ['com.objectstack.service.settings']; private readonly options: EmailServicePluginOptions; private service?: EmailService; diff --git a/packages/services/service-settings/src/settings-prebind-read-warning.test.ts b/packages/services/service-settings/src/settings-prebind-read-warning.test.ts new file mode 100644 index 0000000000..eb9e850864 --- /dev/null +++ b/packages/services/service-settings/src/settings-prebind-read-warning.test.ts @@ -0,0 +1,445 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The pre-bind window, READ half (#10250) — **a read that answers with the + * manifest default while a persisted `sys_setting` row sits unread.** + * + * ## The defect + * + * `SettingsService.loadRows` picks its store on `if (this.engine)`, exactly as + * `upsertRow` does, and the engine is bound in one place: `SettingsServicePlugin` + * registers a `kernel:ready` hook from its `start()` and calls `bindEngine` + * inside it. A `get()` / `getNamespace()` issued before that hook runs takes the + * `this.memory` branch, which at boot holds nothing — so the caller receives the + * specifier's declared `default`, with `source: 'default'` and `locked: false`, + * and no diagnostic of any kind, while the operator's saved row is never read. + * + * The write half (#10159 / PR #10251) could REFUSE, because an in-window write + * has no correct outcome. A read does: a setting with genuinely no persisted row + * must answer the manifest default, and doing so at boot is ordinary. So the + * fix here is not a refusal — it is that the residual stops being silent. + * + * ## What is load-bearing in this file, and which half it is + * + * Not the warning. An implementation that warned on EVERY read would pass a + * warn-only suite and turn every boot of every deployment noisy — the classic + * way a diagnostic gets added, ignored, and then removed. The load-bearing half + * is therefore the SILENCE: four separate populations that read the in-memory + * fallback and must stay quiet, each of which is a real shipped shape rather + * than a variation on one. + * + * 1. an ordinary, correctly-ordered boot (the reader starts AFTER settings) + * 2. every read after `bindEngine` + * 3. a kernel with no `objectql` at all (`settleWithoutEngine`) + * 4. a directly constructed `SettingsService` (no declared pending bind) + * + * Measured: making the report unconditional keeps case 5 (the warning) green and + * turns 1-4 red. Neutering the report keeps 1-4 green and turns 5 red. Neither + * mutation can pass this file. + * + * ## Resolution + * + * Every subject here is imported RELATIVELY (`./settings-service.js`, + * `./settings-service-plugin.js`), so vitest resolves them to the sibling `src` + * files in this same package — not to `dist/`. There is no build step between an + * edit to `settings-service.ts` and a verdict from this file, and an ablation of + * it needs no rebuild to be believed. + */ + +import { describe, expect, it, vi } from 'vitest'; +import { LiteKernel } from '@objectstack/core'; +import type { Plugin, PluginContext } from '@objectstack/core'; +import { ObjectQL } from '@objectstack/objectql'; +import { SysSecret, SysSetting } from '@objectstack/platform-objects/system'; +import type { SettingsManifest } from '@objectstack/spec/system'; +import { SettingsService } from './settings-service.js'; +import { SettingsServicePlugin, wrapEngineAsSettingsEngine } from './settings-service-plugin.js'; + +const OWNER_PACKAGE = 'com.objectstack.test.settings-prebind-read-warning'; + +/** + * One global key with a declared default and one env-overridable key. The + * default is what an in-window read wrongly hands back, so it is the value the + * persisted row must differ from. + */ +const probeManifest: SettingsManifest = { + namespace: 'read_probe', + version: 1, + label: 'Read probe', + scope: 'global', + specifiers: [ + { type: 'text', key: 'provider', label: 'Provider', required: false, default: 'log' }, + ], +}; + +/** The exact line an in-window read must produce. Asserted, not paraphrased. */ +const EXPECTED_WARNING = + "[SettingsService] Pre-bind READ of namespace 'read_probe': the data engine is declared " + + 'but not yet bound, so this read was answered from the in-memory fallback and the ' + + 'manifest defaults — any persisted `sys_setting` row was NOT consulted. Declare ' + + "optionalDependencies: ['com.objectstack.service.settings'] on the reading plugin so it " + + 'starts after the settings engine binds (earliest safe phase: kernel:bootstrapped).'; + +// --------------------------------------------------------------------------- +// A driver over plain Maps — the same shape `settings-engine-bind-window.test.ts` +// uses, and for the same reason: the real `ObjectQL` sits on top of it, so what +// these cases measure is the real engine's row state rather than a fake's +// bookkeeping. +// --------------------------------------------------------------------------- + +function makeMemoryDriver() { + const store = new Map>>(); + let nextId = 0; + const copy = (r: Record) => ({ ...r }); + const rowsOf = (object: string) => { + let s = store.get(object); + if (!s) { s = new Map(); store.set(object, s); } + return s; + }; + const matches = (row: Record, where: any): boolean => { + if (!where || typeof where !== 'object') return true; + return Object.entries(where).every(([k, v]) => { + if (k.startsWith('$')) throw new Error(`fake driver: unsupported operator ${k}`); + return (row[k] ?? null) === (v ?? null); + }); + }; + const driver: any = { + name: 'memory', version: '0.0.0', supports: {} as any, + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, + async execute() { return null; }, + async find(o: string, ast: any) { + return [...rowsOf(o).values()].filter((r) => matches(r, ast?.where)).map(copy); + }, + async findOne(o: string, ast: any) { + for (const r of rowsOf(o).values()) if (matches(r, ast?.where)) return copy(r); + return null; + }, + async create(o: string, data: Record) { + nextId += 1; + const id = (data.id as string) ?? `row_${nextId}`; + const row = { ...data, id }; + rowsOf(o).set(id, row); + return copy(row); + }, + async update(o: string, id: string, data: Record) { + const s = rowsOf(o); + const cur = s.get(id); + if (!cur) return null; + const next = { ...cur, ...data, id }; + s.set(id, next); + return copy(next); + }, + async upsert(o: string, data: Record) { + const id = data.id as string | undefined; + return id && rowsOf(o).has(id) ? this.update(o, id, data) : this.create(o, data); + }, + async delete(o: string, id: string) { return rowsOf(o).delete(id); }, + async count(o: string, ast: any) { return (await this.find(o, ast)).length; }, + async bulkCreate(o: string, rows: Record[]) { + return Promise.all(rows.map((r) => this.create(o, r))); + }, + async bulkUpdate() { return []; }, + async bulkDelete() {}, + async updateMany(o: string, ast: any, data: Record) { + const rows = await this.find(o, ast); + const s = rowsOf(o); + for (const r of rows) s.set(r.id as string, { ...s.get(r.id as string), ...data, id: r.id }); + return rows.length; + }, + async deleteMany(o: string, ast: any) { + const rows = await this.find(o, ast); + for (const r of rows) rowsOf(o).delete(r.id as string); + return rows.length; + }, + async syncSchema() {}, async dropTable() {}, + async beginTransaction() { return { commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, async rollback() {}, + }; + return { driver, rowsOf }; +} + +/** Publishes `objectql` from `init()`, where the real ObjectQLPlugin does. */ +class EnginePlugin implements Plugin { + name = 'com.objectstack.engine.objectql'; + version = '0.0.0'; + type = 'standard' as const; + providesServices = ['objectql']; + constructor(private readonly engine: ObjectQL) {} + init = async (ctx: PluginContext) => { + ctx.registerService('objectql', this.engine); + }; +} + +/** + * A reader in the shape the census measured: it registers its `kernel:ready` + * hook from `start()` and reads a settings namespace from inside it. Whether + * that read lands inside or outside the window is decided purely by whether + * this plugin starts before or after `SettingsServicePlugin` — which is the + * whole point. + */ +class SettingsReadingPlugin implements Plugin { + name = 'com.objectstack.test.settings-reader'; + version = '0.0.0'; + type = 'standard' as const; + /** Set when the declaration under test is wanted (the correctly-ordered leg). */ + optionalDependencies?: string[]; + /** `resolved:` — what the in-hook read actually saw. */ + readAtReady?: string; + engineBoundAtReady?: boolean; + + constructor(opts: { declareSettingsOrder?: boolean } = {}) { + if (opts.declareSettingsOrder) { + this.optionalDependencies = ['com.objectstack.service.settings']; + } + } + + /** Nothing to register — but the kernel calls `init` on every plugin. */ + init = async () => {}; + + start = async (ctx: PluginContext) => { + ctx.hook('kernel:ready', async () => { + let svc: SettingsService | undefined; + try { svc = ctx.getService('settings'); } catch { /* absent */ } + if (!svc) return; + // Reaching into the private field on purpose: "was the engine bound at + // this instant" is the fact the window is defined by, and there is no + // public spelling of it. + this.engineBoundAtReady = Boolean((svc as unknown as { engine?: unknown }).engine); + const r = await svc.get('read_probe', 'provider'); + this.readAtReady = `resolved:${JSON.stringify(r.value)}`; + }); + }; +} + +/** + * Boot a real kernel with a real `SettingsServicePlugin`, a seeded persisted + * row, and one reader. + * + * `readerFirst` is the ONLY thing that differs between the noisy leg and the + * quiet leg: it flips `kernel.use()` order. Everything else — the manifest, the + * seeded row, the plugin options, the logger — is identical, so a difference in + * the observed behaviour can only come from ordering. + */ +async function bootKernel(opts: { + readerFirst: boolean; + declareSettingsOrder?: boolean; + withEngine?: boolean; + /** Row seeded into `sys_setting` before boot — the value an in-window read misses. */ + seedPersisted?: string; +}) { + const withEngine = opts.withEngine !== false; + const { driver, rowsOf } = makeMemoryDriver(); + const engine = new ObjectQL(); + engine.registerDriver(driver, true); + await engine.init(); + for (const o of [SysSetting, SysSecret]) engine.registry.registerObject(o as any, OWNER_PACKAGE); + + if (opts.seedPersisted !== undefined) { + rowsOf('sys_setting').set('seed_1', { + id: 'seed_1', + namespace: 'read_probe', + key: 'provider', + scope: 'global', + user_id: null, + value: opts.seedPersisted, + value_enc: null, + encrypted: false, + locked: false, + locked_reason: null, + updated_at: new Date().toISOString(), + updated_by: null, + }); + } + + const reader = new SettingsReadingPlugin({ declareSettingsOrder: opts.declareSettingsOrder }); + const settings = new SettingsServicePlugin({ + registerRoutes: false, + manifests: [probeManifest], + actionHandlers: {}, + }); + + const kernel = new LiteKernel({ logger: { level: 'error' } as never }); + // `SettingsServicePlugin.init` stores `ctx.logger`, and `createContext()` + // captures the kernel's OWN logger instance by reference — so the spy has to + // replace the METHOD on that instance rather than swap the field, which the + // already-built context would never see. Same object, so this is what the + // service really calls. + const kernelLogger = (kernel as unknown as { + logger: { warn: (message: string, meta?: unknown) => void }; + }).logger; + const warn = vi.spyOn(kernelLogger, 'warn').mockImplementation(() => {}); + + if (withEngine) kernel.use(new EnginePlugin(engine)); + if (opts.readerFirst) { kernel.use(reader); kernel.use(settings); } + else { kernel.use(settings); kernel.use(reader); } + await kernel.bootstrap(); + + return { + kernel, + reader, + warn, + svc: kernel.getService('settings'), + preBindWarnings: () => warn.mock.calls.map((c) => c[0]).filter((m) => + String(m).includes('Pre-bind READ')), + }; +} + +// --------------------------------------------------------------------------- +// 5. The window is AUDIBLE — the one case that must be noisy +// --------------------------------------------------------------------------- + +describe('a settings read inside the pre-bind window warns', () => { + it('warns with the operator-actionable line, and the read really did miss the persisted row', async () => { + const { reader, preBindWarnings } = await bootKernel({ + readerFirst: true, + seedPersisted: 'twilio', + }); + + // The window, measured on the reader itself. + expect(reader.engineBoundAtReady).toBe(false); + // THE DEFECT, still observable: the read answered with the manifest default + // (`log`) while `sys_setting` held `twilio`. The fix does NOT change this — + // that is deliberate (#10159's fix left reads open on purpose) — so this + // assertion is the reason the warning has to exist at all. + expect(reader.readAtReady).toBe('resolved:"log"'); + + // THE DIRECTION PIN: the residual is audible. Asserted as the WHOLE line, + // not "a warn happened" — a diagnostic whose text nobody pins is one nobody + // can act on, and the repair instruction is the part that makes it useful. + const warnings = preBindWarnings(); + expect(warnings).toHaveLength(1); + expect(warnings[0]).toBe(EXPECTED_WARNING); + expect(warnings[0]).toContain("optionalDependencies: ['com.objectstack.service.settings']"); + expect(warnings[0]).toContain('kernel:bootstrapped'); + }); + + it('reports once per namespace, not once per key', async () => { + // `getNamespace()` resolves every specifier through `get()`, so a naive + // report would emit one identical line per declared key. One actionable + // line survives being read; a dozen get filtered. + const svc = new SettingsService({ env: {}, engineBindPending: true, logger: { warn: vi.fn() } }); + const spy = (svc as unknown as { logger: { warn: ReturnType } }).logger.warn; + svc.registerManifest({ + ...probeManifest, + specifiers: [ + { type: 'text', key: 'provider', label: 'P', required: false, default: 'log' }, + { type: 'text', key: 'region', label: 'R', required: false, default: 'us' }, + { type: 'text', key: 'sender', label: 'S', required: false, default: 'os' }, + ], + }); + await svc.getNamespace('read_probe'); + await svc.getNamespace('read_probe'); + expect(spy.mock.calls.filter((c) => String(c[0]).includes('Pre-bind READ'))).toHaveLength(1); + }); +}); + +// --------------------------------------------------------------------------- +// 1-4. STILL QUIET — the load-bearing half. Each is a distinct shipped shape. +// --------------------------------------------------------------------------- + +describe('a correctly-ordered boot stays silent', () => { + it('1. the reader declaring the settings order reads the PERSISTED row and warns nothing', async () => { + // Registered FIRST via `kernel.use()`, exactly like the noisy case — the + // declaration is the only difference, and it is what moves the order. + const { reader, preBindWarnings } = await bootKernel({ + readerFirst: true, + declareSettingsOrder: true, + seedPersisted: 'twilio', + }); + + expect(reader.engineBoundAtReady).toBe(true); + // The whole point of the ordering fix, stated as a value: the operator's + // saved row, not the manifest default. + expect(reader.readAtReady).toBe('resolved:"twilio"'); + expect(preBindWarnings()).toEqual([]); + }); + + it('1b. plain slate order (settings mounted first) is silent too', async () => { + // The incidental ordering the always-on slate provides today. It must not + // become noisy just because a declaration now exists. + const { reader, preBindWarnings } = await bootKernel({ + readerFirst: false, + seedPersisted: 'twilio', + }); + expect(reader.engineBoundAtReady).toBe(true); + expect(reader.readAtReady).toBe('resolved:"twilio"'); + expect(preBindWarnings()).toEqual([]); + }); + + it('2. reads after bind — the ordinary steady state — warn nothing', async () => { + const { svc, warn } = await bootKernel({ readerFirst: false, seedPersisted: 'twilio' }); + warn.mockClear(); + for (let i = 0; i < 5; i++) await svc.get('read_probe', 'provider'); + expect(warn.mock.calls.filter((c) => String(c[0]).includes('Pre-bind READ'))).toEqual([]); + }); + + it('3. a kernel with NO objectql settles the window — the memory fallback IS the store', async () => { + // `SettingsServicePlugin` declares `objectql` OPTIONAL and degrades on + // purpose. On such a kernel no engine is ever coming, so reading the + // fallback is correct rather than premature, and warning would make every + // lean/embedded boot noisy forever. + const { svc, preBindWarnings } = await bootKernel({ readerFirst: false, withEngine: false }); + await svc.get('read_probe', 'provider'); + await svc.getNamespace('read_probe'); + expect(preBindWarnings()).toEqual([]); + }); + + it('4. a directly constructed SettingsService declares no pending bind and stays quiet', async () => { + // "unit tests, bootstrap, control-plane mock" — the documented second + // reading of the in-memory fallback. Only `SettingsServicePlugin` sets + // `engineBindPending`, so the report never arms here. + const warn = vi.fn<(message: string) => void>(); + const svc = new SettingsService({ env: {}, logger: { warn } }); + svc.registerManifest(probeManifest); + await svc.set('read_probe', 'provider', 'memory-is-the-store'); + expect((await svc.get('read_probe', 'provider')).value).toBe('memory-is-the-store'); + expect(warn.mock.calls.filter((c) => String(c[0]).includes('Pre-bind READ'))).toEqual([]); + }); + + it('5. an in-window read satisfied by an OS_* env override is correct, and silent', async () => { + // Env outranks every persisted scope, so this answer is right whether or + // not the engine is bound — `get()` returns before ever reaching + // `loadRows`. Reporting it would train operators to ignore the line. + const warn = vi.fn<(message: string) => void>(); + const svc = new SettingsService({ + env: { OS_READ_PROBE_PROVIDER: 'from-env' }, + engineBindPending: true, + logger: { warn }, + }); + svc.registerManifest(probeManifest); + const r = await svc.get('read_probe', 'provider'); + expect(r.value).toBe('from-env'); + expect(r.source).toBe('env'); + expect(warn.mock.calls.filter((c) => String(c[0]).includes('Pre-bind READ'))).toEqual([]); + }); +}); + +// --------------------------------------------------------------------------- +// The window CLOSES — a service that warned once stops warning after bind +// --------------------------------------------------------------------------- + +describe('the report is scoped to the window, not to engine-less-ness', () => { + it('warns before bindEngine and never again after it', async () => { + const { driver } = makeMemoryDriver(); + const engine = new ObjectQL(); + engine.registerDriver(driver, true); + await engine.init(); + for (const o of [SysSetting, SysSecret]) engine.registry.registerObject(o as any, OWNER_PACKAGE); + + const warn = vi.fn<(message: string) => void>(); + const svc = new SettingsService({ env: {}, engineBindPending: true, logger: { warn } }); + svc.registerManifest(probeManifest); + + await svc.get('read_probe', 'provider'); + expect(warn.mock.calls.filter((c) => String(c[0]).includes('Pre-bind READ'))).toHaveLength(1); + + svc.bindEngine(wrapEngineAsSettingsEngine(engine as never)); + warn.mockClear(); + // A DIFFERENT namespace, so the per-namespace dedupe cannot be what makes + // this silent — only the closed window can. + svc.registerManifest({ ...probeManifest, namespace: 'read_probe_2' }); + await svc.get('read_probe_2', 'provider'); + await svc.get('read_probe', 'provider'); + expect(warn.mock.calls.filter((c) => String(c[0]).includes('Pre-bind READ'))).toEqual([]); + }); +}); diff --git a/packages/services/service-settings/src/settings-service.ts b/packages/services/service-settings/src/settings-service.ts index ff35b55c7d..c05abaa800 100644 --- a/packages/services/service-settings/src/settings-service.ts +++ b/packages/services/service-settings/src/settings-service.ts @@ -548,6 +548,16 @@ export class SettingsService { * otherwise repeat one operator-actionable line per attempt. */ private readonly reportedCryptoRefusals = new Set(); + /** + * Namespaces whose pre-bind READ has already been reported (#10250). Deduped + * for the same reason the two sets above are, and keyed by NAMESPACE rather + * than by key: `getNamespace()` resolves every specifier in the namespace + * through `get()`, so one in-window `getNamespace('mail')` reaches + * {@link loadRows} once per declared key. One operator-actionable line per + * namespace per window is the report; a dozen identical ones is noise that + * gets filtered. + */ + private readonly reportedPreBindReads = new Set(); /** In-memory fallback when no engine is wired. */ private readonly memory: SettingsRow[] = []; /** @@ -655,6 +665,63 @@ export class SettingsService { throw new SettingsEngineNotBoundError(namespace, keys); } + /** + * Report a READ issued inside the pre-bind window (#10250) — the read half of + * the same window {@link assertEngineBound} refuses writes in. + * + * ## Why this reports instead of refusing + * + * The write half could refuse because a write in the window has no correct + * outcome: nothing durable exists to write to. A READ does. An in-window read + * of a setting that genuinely has no persisted row must answer the manifest + * `default`, and that is an ordinary, common thing for a boot-time reader to + * do. Refusing it would turn a correct startup sequence into an error — which + * is why #10159's fix deliberately left reads open. + * + * What is wrong is not the answer, it is that the answer was produced WITHOUT + * CONSULTING the store. In the window {@link loadRows} takes its `this.memory` + * branch, which at boot holds nothing, so the caller receives the declared + * `default` with `source: 'default'` and `locked: false` — an assertion about + * persisted state made without reading persisted state — while a real + * `sys_setting` row with a different value sits unread. Nothing distinguishes + * that from "no row exists", at any level, which is what made this quieter + * than the write half it was found beside. + * + * So the residual is made AUDIBLE rather than fatal, and the line names the + * repair rather than the symptom: the reader declares + * `optionalDependencies: ['com.objectstack.service.settings']` and the kernel + * orders it after the bind (ADR-0116). The three shipped always-on readers + * (`plugin-email`, `service-sms`, `service-storage`) declare exactly that as + * of this change, so on a correct boot this reporter never fires — see + * `settings-prebind-read-warning.test.ts`, whose load-bearing half is the + * SILENCE on an ordinary boot. + * + * ## When it stays quiet, and why each case is not the window + * + * - engine bound — `loadRows` never reaches here. + * - `settleWithoutEngine()` — no engine is EVER coming; the in-memory + * fallback IS this deployment's store, so a + * read of it is correct, not premature. + * - no declared pending bind — a directly constructed `SettingsService` + * ("unit tests, bootstrap, control-plane + * mock"). Only `SettingsServicePlugin` sets + * `engineBindPending`. + */ + private reportPreBindRead(namespace: string): void { + if (!this.engineBindPending || this.engine) return; + if (this.reportedPreBindReads.has(namespace)) return; + this.reportedPreBindReads.add(namespace); + const message = + `[SettingsService] Pre-bind READ of namespace '${namespace}': the data engine is ` + + 'declared but not yet bound, so this read was answered from the in-memory fallback ' + + 'and the manifest defaults — any persisted `sys_setting` row was NOT consulted. ' + + "Declare optionalDependencies: ['com.objectstack.service.settings'] on the reading " + + 'plugin so it starts after the settings engine binds (earliest safe phase: ' + + 'kernel:bootstrapped).'; + if (this.logger?.warn) this.logger.warn(message); + else console.warn(message); + } + /** * Cascade priority ranks for lock comparisons (lower = higher * precedence). env r.namespace === namespace && diff --git a/packages/services/service-settings/src/settings-service.types.ts b/packages/services/service-settings/src/settings-service.types.ts index be4a618aed..f7b481be0f 100644 --- a/packages/services/service-settings/src/settings-service.types.ts +++ b/packages/services/service-settings/src/settings-service.types.ts @@ -292,6 +292,14 @@ export type SettingsActionHandler = (input: { */ export interface SettingsDiagnosticsLogger { error?: (message: string) => void; + /** + * `warn` carries the diagnostics that are NOT refusals — today, the pre-bind + * READ report (#10250). Same one-argument shape, and for the same reasons the + * header above gives for `error`: `Logger.warn(message, meta?)`, `ctx.logger`, + * `console.warn` and a one-line spy all stay assignable, and no `meta` object + * is passed that the redactor could silently empty. + */ + warn?: (message: string) => void; } export interface SettingsServiceOptions { diff --git a/packages/services/service-sms/src/sms-plugin.ts b/packages/services/service-sms/src/sms-plugin.ts index 498a07313d..968d0e574f 100644 --- a/packages/services/service-sms/src/sms-plugin.ts +++ b/packages/services/service-sms/src/sms-plugin.ts @@ -94,6 +94,32 @@ export class SmsServicePlugin implements Plugin { * kernel name this plugin when a consumer requires one before it inits. */ providesServices = ['sms']; + /** + * Order-if-present on the settings service (ADR-0116, #10250). + * + * `start()` registers a `kernel:ready` hook that reads the `sms` namespace — + * provider credentials AND the daily cost ceiling. `SettingsServicePlugin` + * binds its data engine from ITS `kernel:ready` hook, registered in ITS + * `start()`, so the plugin that starts first registers the earlier hook and + * the earlier hook runs first. Start order is the topological order over this + * declaration (`resolvePluginOrder`, used for BOTH phases in + * `ObjectKernel.bootstrap`), so declaring the edge is what puts the bind + * ahead of the read. + * + * `sms` is the entry this card was filed about: it sits at index 6 of + * `PLATFORM_ALWAYS_ON_CAPABILITIES`, one past the pinned `slice(0, 6)`, so + * before this declaration its correct position relative to `settings` was + * held by nothing at all. In the window `getNamespace('sms')` answers from + * the manifest defaults — a `log` provider that reports every OTP "sent" and + * a default cost ceiling — while the operator's saved credentials sit unread + * in `sys_setting`. + * + * SOFT, not hard: a kernel with no settings service must still boot this + * plugin (the transport is buildable from options alone). ADR-0049 — + * declared is enforced: `serve-settings-ordering.pin.test.ts` boots a kernel + * with the readers registered BEFORE settings and proves the order moves. + */ + optionalDependencies = ['com.objectstack.service.settings']; version = '1.0.0'; type = 'standard' as const; diff --git a/packages/services/service-storage/src/storage-service-plugin.ts b/packages/services/service-storage/src/storage-service-plugin.ts index 4d2e6e69e1..62ff91c409 100644 --- a/packages/services/service-storage/src/storage-service-plugin.ts +++ b/packages/services/service-storage/src/storage-service-plugin.ts @@ -128,12 +128,42 @@ export class StorageServicePlugin implements Plugin { */ providesServices = ['storage', 'file-storage']; /** - * init() registers sys_file / sys_upload_session / sys_attachment through - * the `manifest` service ObjectQLPlugin provides — order-if-present so the - * registration is deterministic (ADR-0116, #4471). Soft, not hard: without - * an engine the plugin degrades on purpose (storage service still up). + * `com.objectstack.engine.objectql` — init() registers sys_file / + * sys_upload_session / sys_attachment through the `manifest` service + * ObjectQLPlugin provides — order-if-present so the registration is + * deterministic (ADR-0116, #4471). Soft, not hard: without an engine the + * plugin degrades on purpose (storage service still up). + * + * `com.objectstack.service.settings` — order-if-present (ADR-0116, #10250). + * + * `start()` registers a `kernel:ready` hook that reads the `storage` + * namespace (backend selection + credentials). `SettingsServicePlugin` binds + * its data engine from ITS `kernel:ready` hook, registered in ITS `start()`, + * so the plugin that starts first registers the earlier hook and the earlier + * hook runs first. Start order is the topological order over these + * declarations (`resolvePluginOrder`, used for BOTH phases in + * `ObjectKernel.bootstrap`), so declaring the edge is what puts the bind + * ahead of the read. + * + * Without it the ordering was incidental: the always-on slate happens to put + * `settings` ahead of `storage`, but an app declaring `requires: ['storage']` + * gets storage FIRST (declared tokens are prepended, the slate only + * appended), and cloud's objectos-runtime mounts from its own wiring. In that + * window `getNamespace('storage')` reads the in-memory fallback and answers + * with the manifest DEFAULTS — so the deployment keeps its constructor-built + * local adapter while the operator's saved S3 credentials sit unread in + * `sys_setting`. + * + * SOFT, not hard, exactly like the objectql edge above: without a settings + * service this plugin degrades on purpose (`bindToSettings` is optional and + * the constructor-built adapter stands). ADR-0049 — declared is enforced: + * `serve-settings-ordering.pin.test.ts` boots a kernel with the readers + * registered BEFORE settings and proves the order moves. */ - optionalDependencies = ['com.objectstack.engine.objectql']; + optionalDependencies = [ + 'com.objectstack.engine.objectql', + 'com.objectstack.service.settings', + ]; version = '1.0.0'; type = 'standard';