From 6890d694b53d4bb52086fe79f10f737dbe38b839 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 15:33:36 +0000 Subject: [PATCH] fix(plugin-auth,plugin-webhooks): retire a dead degrade branch and an implicit transitive dependency (#4187) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two concrete findings from the ADR-0116 consumer-side audit, plus the authoring rule that would have prevented both. plugin-auth claimed a fallback it did not have: init() ran `const dataEngine = ctx.getService('data'); if (!dataEngine) { warn(... "auth will use in-memory storage") }`. That branch could never execute — getService THROWS for an unregistered service rather than returning undefined, and this plugin declares a hard dependency on ObjectQL (which registers `data` unconditionally), so a kernel without the engine fails earlier still with "Dependency ... not found". The branch is removed and the real contract declared as requiresServices: ['data', 'manifest'], which also replaces a trailing `// manifest service required` comment with the machine-checked form of the same claim. AuthManager keeps its own optional dataEngine guards — it is usable outside the plugin. plugin-webhook-outbox was protected only transitively: it resolves `manifest` in init() with no fallback while depending on messaging, which in turn depends on ObjectQL, the actual provider. That would have broken silently the day messaging stopped depending on the engine, and surfaced as a crash inside an unrelated plugin's init. It now declares requiresServices: ['manifest'] directly. Neither change alters ordering or boot outcomes on any current composition — both plugins were already ordered correctly. What changes is what a broken composition says, and that the guarantees are checked rather than inherited. Docs: anatomy.mdx gains the three ADR-0116 fields and the decision rule for resolving a service inside init(), including the two traps behind these fixes. The api-registry example declares the contract on all seven of its plugins instead of relying on kernel.use() order. Refs #4187 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_014DQuJBNpwStpJvo3owBw2B --- .changeset/adr-0116-followups.md | 42 ++++++++++++ content/docs/plugins/anatomy.mdx | 64 +++++++++++++++++++ .../core/examples/api-registry-example.ts | 18 ++++++ .../plugins/plugin-auth/src/auth-plugin.ts | 31 +++++++-- .../src/webhook-outbox-plugin.ts | 10 +++ 5 files changed, 159 insertions(+), 6 deletions(-) create mode 100644 .changeset/adr-0116-followups.md diff --git a/.changeset/adr-0116-followups.md b/.changeset/adr-0116-followups.md new file mode 100644 index 0000000000..7e0189584a --- /dev/null +++ b/.changeset/adr-0116-followups.md @@ -0,0 +1,42 @@ +--- +"@objectstack/plugin-auth": patch +"@objectstack/plugin-webhooks": patch +--- + +fix(plugin-auth,plugin-webhooks): retire a dead degrade branch and an implicit transitive dependency (ADR-0116 follow-ups, #4187) + +Two concrete findings from the ADR-0116 consumer-side audit, plus the +authoring rule that would have prevented both. + +**`plugin-auth` claimed a fallback it did not have.** `init()` ran +`const dataEngine = ctx.getService('data'); if (!dataEngine) { warn('No data +engine service found - auth will use in-memory storage') }`. That branch could +never execute: `getService` **throws** for an unregistered service rather than +returning `undefined`, and this plugin declares a hard dependency on ObjectQL +(which registers `data` unconditionally), so a kernel without the engine fails +even earlier with `Dependency … not found`. The branch is removed and the real +contract is declared — `requiresServices: ['data', 'manifest']` — which also +replaces a trailing `// manifest service required` comment with the +machine-checked form of the same claim. `AuthManager` keeps its own optional +`dataEngine` guards: it is usable outside the plugin. + +**`plugin-webhook-outbox` was protected only transitively.** It resolves +`manifest` in `init()` with no fallback while depending on +`com.objectstack.service.messaging`, which in turn depends on ObjectQL, the +actual provider. That works today and would have broken silently the day +messaging stopped depending on the engine — surfacing as a crash inside an +unrelated plugin's init. It now declares `requiresServices: ['manifest']` +directly. + +Neither change alters ordering or boot outcomes on any current composition: +both plugins were already ordered correctly. What changes is what a broken +composition *says*, and that the guarantees are now checked rather than +inherited. + +Docs: `content/docs/plugins/anatomy.mdx` gains the three ADR-0116 fields and +the decision rule for resolving a service inside `init()` (hard dependency vs +`optionalDependencies` + `requiresServices`), including the two traps behind +these fixes — don't rely on a transitive provider, and don't write an +`if (!svc)` fallback after a bare `getService`. The api-registry example +declares the contract on all seven of its plugins instead of relying on +`kernel.use()` order. diff --git a/content/docs/plugins/anatomy.mdx b/content/docs/plugins/anatomy.mdx index 7ea9469a29..a7525c3c03 100644 --- a/content/docs/plugins/anatomy.mdx +++ b/content/docs/plugins/anatomy.mdx @@ -131,9 +131,32 @@ export interface Plugin { * Dependencies (Optional) * List of other plugin names that this plugin depends on. * The kernel ensures these plugins are initialized before this one. + * A name that is not composed on the kernel fails the boot. */ dependencies?: string[]; + /** + * Soft dependencies (Optional) — order-if-present, ADR-0116. + * Hoisted ahead exactly like `dependencies` when composed, silently + * skipped when absent. For plugins that degrade without the dependency + * but must never initialize before it. + */ + optionalDependencies?: string[]; + + /** + * Services this plugin resolves SYNCHRONOUSLY during init() (ADR-0116). + * Validated before Phase 1 and again immediately before this plugin's + * init, so a misordered composition fails with a named error instead of + * crashing inside your init. + */ + requiresServices?: string[]; + + /** + * Services this plugin's init() registers UNCONDITIONALLY (ADR-0116). + * Never declare an option-gated registration. + */ + providesServices?: string[]; + /** * Init Phase (REQUIRED) * Called when the kernel is initializing. Use this to: @@ -242,6 +265,47 @@ Plugins follow a strict three-phase lifecycle managed by the kernel: 2. **start()** — Called after *all* plugins have initialized. Start servers, connect to databases, or execute main logic. 3. **destroy()** — Called during shutdown, in reverse order. Clean up connections, timers, and resources. +### Resolving a service inside `init()` — declare it + +`kernel.use()` **registration order is not a contract**. Init order comes from +the dependency graph, so "it works because I registered it later" is luck, and +it is the recipe behind two production bugs (a server that booted with no +tables, then [#4085](https://github.com/objectstack-ai/objectstack/issues/4085)). +Anything you resolve in `start()` needs no declaration — Phase 1 completes +before any `start()` runs. For `init()`, pick one of two: + +| Your plugin… | Declare | +| :--- | :--- | +| **cannot work** without the provider | `dependencies: ['']` — plus `requiresServices` to say which service, and why | +| **degrades** without it, but must init after it when present | `optionalDependencies: ['']` + `requiresServices: ['']` | + +Resolving a service in `init()` while declaring **neither** is the bug shape: +it works until someone composes the stack in a different order, and then fails +inside your `init()` with an error that names the service but not the cause. + +```ts +class MyPlugin implements Plugin { + // Hard: the engine is always composed with this plugin. + dependencies = ['com.objectstack.engine.objectql']; + requiresServices = ['manifest']; // what the dependency is FOR + + async init(ctx: PluginContext) { + ctx.getService<{ register(m: any): void }>('manifest').register(mySchema); + } +} +``` + +Two rules worth stating explicitly, because both have already caused bugs: + +- **Declare the requirement directly, not transitively.** Depending on a plugin + that *happens* to depend on the real provider works until that plugin's own + dependencies change, and the breakage then surfaces in your `init()`. +- **A service you probe behind `try`/`catch` is not a requirement.** Put only + hard, no-fallback needs in `requiresServices` — and if you write a fallback + branch, make sure it can actually run: `getService` **throws** for a missing + service, it never returns `undefined`, so `if (!svc) { /* degrade */ }` after + a bare `getService` is dead code advertising a mode you do not have. + ## Next Steps - **[Plugin System](/docs/plugins)** — Module overview: architecture, configuration, and built-in plugins diff --git a/packages/core/examples/api-registry-example.ts b/packages/core/examples/api-registry-example.ts index 2bbf732af3..83baa95561 100644 --- a/packages/core/examples/api-registry-example.ts +++ b/packages/core/examples/api-registry-example.ts @@ -24,6 +24,12 @@ async function example1_BasicApiRegistration() { const customerPlugin: Plugin = { name: 'customer-plugin', version: '1.0.0', + // init() resolves `api-registry` synchronously with no fallback, so the + // ordering requirement is DECLARED rather than left to the `kernel.use()` + // order above (ADR-0116). Registration order is not a contract — the + // kernel orders from this graph. + dependencies: ['com.objectstack.core.api-registry'], + requiresServices: ['api-registry'], init: async (ctx) => { const registry = ctx.getService('api-registry'); @@ -163,6 +169,8 @@ async function example2_MultiPluginDiscovery() { // Data Plugin - REST APIs const dataPlugin: Plugin = { name: 'data-plugin', + dependencies: ['com.objectstack.core.api-registry'], + requiresServices: ['api-registry'], init: async (ctx) => { const registry = ctx.getService('api-registry'); @@ -212,6 +220,8 @@ async function example2_MultiPluginDiscovery() { // Analytics Plugin - Beta API const analyticsPlugin: Plugin = { name: 'analytics-plugin', + dependencies: ['com.objectstack.core.api-registry'], + requiresServices: ['api-registry'], init: async (ctx) => { const registry = ctx.getService('api-registry'); @@ -282,6 +292,8 @@ async function example3_ConflictResolution() { // Core Plugin - High priority const corePlugin: Plugin = { name: 'core-plugin', + dependencies: ['com.objectstack.core.api-registry'], + requiresServices: ['api-registry'], init: async (ctx) => { const registry = ctx.getService('api-registry'); @@ -310,6 +322,8 @@ async function example3_ConflictResolution() { // Custom Plugin - Medium priority const customPlugin: Plugin = { name: 'custom-plugin', + dependencies: ['com.objectstack.core.api-registry'], + requiresServices: ['api-registry'], init: async (ctx) => { const registry = ctx.getService('api-registry'); @@ -361,6 +375,8 @@ async function example4_CustomProtocol() { const websocketPlugin: Plugin = { name: 'websocket-plugin', + dependencies: ['com.objectstack.core.api-registry'], + requiresServices: ['api-registry'], init: async (ctx) => { const registry = ctx.getService('api-registry'); @@ -431,6 +447,8 @@ async function example5_DynamicSchemas() { const dynamicPlugin: Plugin = { name: 'dynamic-plugin', + dependencies: ['com.objectstack.core.api-registry'], + requiresServices: ['api-registry'], init: async (ctx) => { const registry = ctx.getService('api-registry'); diff --git a/packages/plugins/plugin-auth/src/auth-plugin.ts b/packages/plugins/plugin-auth/src/auth-plugin.ts index 8d034ab7d3..a34867707b 100644 --- a/packages/plugins/plugin-auth/src/auth-plugin.ts +++ b/packages/plugins/plugin-auth/src/auth-plugin.ts @@ -144,8 +144,19 @@ export class AuthPlugin implements Plugin { providesServices = ['auth', 'tenancy']; type = 'standard'; version = '1.0.0'; - dependencies: string[] = ['com.objectstack.engine.objectql']; // manifest service required - + dependencies: string[] = ['com.objectstack.engine.objectql']; + /** + * What that dependency is FOR, stated so the kernel can check it (#4187). + * `init()` resolves both synchronously with no fallback: `data` builds the + * AuthManager config, `manifest` registers the auth objects. ObjectQL + * registers both unconditionally and the hard dependency above hoists it + * ahead, so this never changes whether the boot succeeds — it changes what + * a broken composition SAYS, and replaces the trailing `// manifest service + * required` comment with the machine-checked form of the same claim. + */ + requiresServices = ['data', 'manifest']; + + private options: AuthPluginOptions; private authManager: AuthManager | null = null; /** ADR-0093 D4 — the tenancy service registered in init(); reused at kernel:ready. */ @@ -199,11 +210,19 @@ export class AuthPlugin implements Plugin { throw new Error('AuthPlugin: secret is required'); } - // Get data engine service for database operations + // Get data engine service for database operations. This plugin declares a + // hard dependency on ObjectQL (which registers `data` unconditionally), so + // the service is always present here. + // + // There used to be an `if (!dataEngine) warn('…auth will use in-memory + // storage')` guard under this line. It could never fire and the fallback it + // named did not exist: `getService` THROWS for an unregistered service, it + // does not return undefined, and the hard dependency means a kernel without + // the engine fails earlier still with "Dependency … not found". A branch + // that advertises a tolerance the composition forbids is worse than no + // branch — it reads as a supported degraded mode (#4187). AuthManager keeps + // its own `dataEngine?` guards because it is usable outside this plugin. const dataEngine = ctx.getService('data'); - if (!dataEngine) { - ctx.logger.warn('No data engine service found - auth will use in-memory storage'); - } const authConfig: AuthManagerOptions & AuthPluginOptions = { ...this.options, diff --git a/packages/plugins/plugin-webhooks/src/webhook-outbox-plugin.ts b/packages/plugins/plugin-webhooks/src/webhook-outbox-plugin.ts index bca364208b..929aba428d 100644 --- a/packages/plugins/plugin-webhooks/src/webhook-outbox-plugin.ts +++ b/packages/plugins/plugin-webhooks/src/webhook-outbox-plugin.ts @@ -62,6 +62,16 @@ export class WebhookOutboxPlugin implements Plugin { version = '2.0.0'; type = 'standard' as const; dependencies = ['com.objectstack.service.messaging']; + /** + * `init()` registers this plugin's schema through `manifest` with no + * fallback. Until #4187 that was safe only TRANSITIVELY — messaging happens + * to depend on ObjectQL, which provides `manifest` — so the guarantee would + * have evaporated silently the day messaging stopped depending on the + * engine, and the failure would have surfaced as an unrelated plugin's + * init crash. Declaring the requirement directly makes the kernel check it + * regardless of what messaging depends on. + */ + requiresServices = ['manifest']; private autoEnqueuer: AutoEnqueuer | undefined; /** Engine the provenance hook was bound to, so `dispose()` can unbind it. */