diff --git a/.changeset/runner-setters-first-wins.md b/.changeset/runner-setters-first-wins.md new file mode 100644 index 0000000000..27bcc86eae --- /dev/null +++ b/.changeset/runner-setters-first-wins.md @@ -0,0 +1,32 @@ +--- +"@objectstack/objectql": minor +"@objectstack/runtime": patch +--- + +feat(objectql,runtime): the default-runner setters are first-wins, and the private-field probes that used to enforce that are gone (#4251) + +`setDefaultBodyRunner` / `setDefaultActionRunner` now enforce their own +documented contract — "the runtime layer sets this once per engine" — by +keeping the first runner and returning `false` for any later call. Public +accessors `getDefaultBodyRunner()` / `getDefaultActionRunner()` join them, and +the fields become real `private` members instead of `(this as any)` attachments. + +Before this, the invariant lived in the CALLERS: AppPlugin probed the engine's +private `_defaultBodyRunner` / `_defaultActionRunner` fields through `any` to +avoid clobbering another AppPlugin's runner on a shared kernel — an invariant +owned by every caller and enforced by none, and a private reach that a field +rename would have broken silently (the guard reads `undefined`, every AppPlugin +reinstalls). The engine's own `bindHooks` fallback and ObjectQLPlugin's +authored-action re-sync read the same fields the same way. All three read the +public accessors now; the only remaining `_default*` mentions in the repo are +comments and test doubles. + +Caller audit before the semantics change: every setter call site either owns a +fresh engine (the sandbox and hook-binder tests) or wants exactly +keep-the-first (AppPlugin) — nobody replaces a runner on a live engine. Return +type `void` → `boolean` is additive; AppPlugin uses it to keep its "Installed +default … runner" log truthful (skipped when the engine kept an earlier one). + +Pinned in hook-binder tests: second install refused end-to-end (the first +runner is the one that executes) and the accessors expose exactly what was +kept. diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index f747fb56bb..42f2dbaa1c 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -690,8 +690,8 @@ export class ObjectQL implements IDataEngine { metrics?: any; }): void { const merged = { ...(opts ?? {}), logger: this.logger } as any; - if (!merged.bodyRunner && (this as any)._defaultBodyRunner) { - merged.bodyRunner = (this as any)._defaultBodyRunner; + if (!merged.bodyRunner && this._defaultBodyRunner) { + merged.bodyRunner = this._defaultBodyRunner; } if (merged.strict === undefined && (this as any)._strictHookBinding) { merged.strict = true; @@ -705,14 +705,41 @@ export class ObjectQL implements IDataEngine { bindHooksToEngine(this, hooks, merged); } + /** Default hook body-runner — see {@link setDefaultBodyRunner}. */ + private _defaultBodyRunner?: any; + /** Default action body-runner factory — see {@link setDefaultActionRunner}. */ + private _defaultActionRunner?: (actionDef: any) => ((ctx: any) => Promise) | undefined; + /** * Install a default body-runner used when `bindHooks` is called without * an explicit one. The runtime layer sets this once on each per-project * engine so every binding path (template seed, metadata sync, AppPlugin) * can execute hook `body.source` consistently. + * + * FIRST-WINS (#4251): "set once per engine" is this method's own contract, + * so the method enforces it — a second call is ignored and returns `false`. + * Callers used to implement the guard themselves by probing the private + * `_defaultBodyRunner` field through `any` (multiple AppPlugin instances on + * one kernel must not clobber each other's runner), which meant the + * invariant lived in every caller and belonged to none. Nobody replaces a + * runner on a live engine: every setter call site either owns a fresh + * engine or wants exactly this keep-the-first behaviour. + * + * @returns `true` when this call installed the runner, `false` when one was + * already present (kept unchanged). */ - setDefaultBodyRunner(runner: any): void { - (this as any)._defaultBodyRunner = runner; + setDefaultBodyRunner(runner: any): boolean { + if (this._defaultBodyRunner) { + this.logger.debug('Default body runner already installed — keeping the first'); + return false; + } + this._defaultBodyRunner = runner; + return true; + } + + /** The installed default body-runner, if any — the public read the first-wins guard implies. */ + getDefaultBodyRunner(): any { + return this._defaultBodyRunner; } /** @@ -724,9 +751,25 @@ export class ObjectQL implements IDataEngine { * `body` into an executable `registerAction` handler. The factory returns * `undefined` for actions it cannot run (no `body`, invalid shape), which * callers must treat as "skip", not an error. + * + * FIRST-WINS (#4251) — same contract and rationale as + * {@link setDefaultBodyRunner}. + * + * @returns `true` when this call installed the runner, `false` when one was + * already present (kept unchanged). */ - setDefaultActionRunner(runner: (actionDef: any) => ((ctx: any) => Promise) | undefined): void { - (this as any)._defaultActionRunner = runner; + setDefaultActionRunner(runner: (actionDef: any) => ((ctx: any) => Promise) | undefined): boolean { + if (this._defaultActionRunner) { + this.logger.debug('Default action runner already installed — keeping the first'); + return false; + } + this._defaultActionRunner = runner; + return true; + } + + /** The installed default action-runner factory, if any. */ + getDefaultActionRunner(): ((actionDef: any) => ((ctx: any) => Promise) | undefined) | undefined { + return this._defaultActionRunner; } /** diff --git a/packages/objectql/src/hook-binder.test.ts b/packages/objectql/src/hook-binder.test.ts index 65c37fc6aa..e54fae46f7 100644 --- a/packages/objectql/src/hook-binder.test.ts +++ b/packages/objectql/src/hook-binder.test.ts @@ -227,6 +227,35 @@ describe('bindHooksToEngine', () => { expect(calls).toEqual(['body-default-runner']); }); + it('keeps the FIRST default body runner — the setter is first-wins (#4251)', async () => { + const engine = makeEngine(); + const calls: string[] = []; + expect(engine.setDefaultBodyRunner(() => async () => { calls.push('first'); })).toBe(true); + // A second install (another AppPlugin sharing the kernel) is refused — + // the invariant used to be enforced by callers probing the private + // `_defaultBodyRunner` field; it is the engine's own now. + expect(engine.setDefaultBodyRunner(() => async () => { calls.push('second'); })).toBe(false); + const hook: Hook = { + name: 'body-first-wins', object: 'account', events: ['beforeInsert'], priority: 100, + body: { language: 'js', source: 'ctx.input.x = 1;' }, + } as unknown as Hook; + engine.bindHooks([hook], { packageId: 'app:first-wins' }); + await engine.triggerHooks('beforeInsert', makeCtx()); + // Execution proves the first runner stayed installed. + expect(calls).toEqual(['first']); + }); + + it('keeps the FIRST default action runner and exposes both via public accessors (#4251)', () => { + const engine = makeEngine(); + const first = () => undefined; + const second = () => undefined; + expect(engine.setDefaultActionRunner(first)).toBe(true); + expect(engine.setDefaultActionRunner(second)).toBe(false); + // The accessors are the public read the old private-field probes become. + expect(engine.getDefaultActionRunner()).toBe(first); + expect(engine.getDefaultBodyRunner()).toBeUndefined(); + }); + it('explicit bodyRunner wins over the engine default', async () => { const engine = makeEngine(); const calls: string[] = []; diff --git a/packages/objectql/src/hook-binder.ts b/packages/objectql/src/hook-binder.ts index e778e20079..6026723006 100644 --- a/packages/objectql/src/hook-binder.ts +++ b/packages/objectql/src/hook-binder.ts @@ -259,7 +259,9 @@ function resolveHandler( if (body && typeof body === 'object') { let runner = opts.bodyRunner; if (typeof runner !== 'function') { - const fallback = (engine as any)?._defaultBodyRunner; + // [#4251] The public accessor — this read used to reach the private + // `_defaultBodyRunner` field through `as any`. + const fallback = engine?.getDefaultBodyRunner?.(); if (typeof fallback === 'function') runner = fallback; } if (typeof runner !== 'function') { diff --git a/packages/objectql/src/plugin-authored-actions.test.ts b/packages/objectql/src/plugin-authored-actions.test.ts index 8cdfaf6b2a..0d15f5dff3 100644 --- a/packages/objectql/src/plugin-authored-actions.test.ts +++ b/packages/objectql/src/plugin-authored-actions.test.ts @@ -30,8 +30,13 @@ function makeQlMock(overrides: AnyRecord = {}) { getArtifactItem: vi.fn(() => undefined), }, // Default action runner as the runtime installs it: body → handler. + // The plugin reads it through `getDefaultActionRunner()` (#4251); the + // field stays so tests can swap or clear the runner directly. _defaultActionRunner: vi.fn((action: AnyRecord) => action?.body ? async () => `ran:${action.name}` : undefined), + getDefaultActionRunner(): unknown { + return (this as AnyRecord)._defaultActionRunner; + }, ...overrides, }; } diff --git a/packages/objectql/src/plugin.ts b/packages/objectql/src/plugin.ts index 115ec392ff..18a9b8808d 100644 --- a/packages/objectql/src/plugin.ts +++ b/packages/objectql/src/plugin.ts @@ -1665,7 +1665,12 @@ export class ObjectQLPlugin implements Plugin { // deleting the last authored action must unregister it. ql.removeActionsByPackage('metadata-service'); - const runner: any = ql._defaultActionRunner; + // [#4251] The public accessor — this read used to reach the private + // `_defaultActionRunner` field directly. Probed because `ql` can be a + // test double that predates the accessor. + const runner: any = typeof ql.getDefaultActionRunner === 'function' + ? ql.getDefaultActionRunner() + : undefined; let registered = 0; let skippedNoHandler = 0; for (const action of bindable) { diff --git a/packages/runtime/src/app-plugin.test.ts b/packages/runtime/src/app-plugin.test.ts index 93f12c5b62..3a60ee5706 100644 --- a/packages/runtime/src/app-plugin.test.ts +++ b/packages/runtime/src/app-plugin.test.ts @@ -436,11 +436,19 @@ describe('AppPlugin', () => { expect(mockManifest.register).not.toHaveBeenCalled(); }); - it('does not replace an already-installed default runner', async () => { - mockQL._defaultBodyRunner = () => undefined; + it('delegates keep-the-first to the engine — the setter is called unconditionally (#4251)', async () => { + // The caller-side guard (probing the engine's private + // `_defaultBodyRunner` field) is gone; the ENGINE's first-wins + // setter owns the invariant, pinned in objectql's hook-binder + // tests. This side only skips the "Installed" log when the engine + // answers `false`. + mockQL.setDefaultBodyRunner = vi.fn(() => false); // engine: already installed const plugin = new AppPlugin({ id: 'com.test.app' }); await plugin.init(mockContext); - expect(mockQL.setDefaultBodyRunner).not.toHaveBeenCalled(); + expect(mockQL.setDefaultBodyRunner).toHaveBeenCalledTimes(1); + expect(vi.mocked(mockContext.logger.info).mock.calls.map((c) => c[0])).not.toContain( + '[AppPlugin] Installed default hook body runner (runtime-authored hooks can execute)', + ); }); it('honours the OS_DISABLE_AUTHORED_HOOKS=1 opt-out', async () => { diff --git a/packages/runtime/src/app-plugin.ts b/packages/runtime/src/app-plugin.ts index 5893b6283f..58045b11d6 100644 --- a/packages/runtime/src/app-plugin.ts +++ b/packages/runtime/src/app-plugin.ts @@ -55,26 +55,22 @@ interface AppEngineSurface { registerAction(objectKey: string, name: string, handler: unknown, packageId: string): void; registerDriver(driver: unknown): void; setDatasourceMapping(rules: unknown): void; - setDefaultBodyRunner(runner: unknown): void; + /** + * FIRST-WINS setters (#4251): the engine keeps the first runner and + * returns whether this call installed it. The idempotence guard used to + * live HERE, as a probe of the engine's private `_defaultBodyRunner` / + * `_defaultActionRunner` fields through this surface — an invariant owned + * by every caller and enforced by none. It is the engine's now. + * `boolean | void` because pre-first-wins engines (and bare `vi.fn()` + * doubles) return undefined — treated as installed. + */ + setDefaultBodyRunner(runner: unknown): boolean | void; setDefaultActionRunner( runner: (actionDef: any) => ((ctx: any) => Promise) | undefined, - ): void; + ): boolean | void; setHookMetricsRecorder(recorder: unknown): void; getHookMetricsRecorder(): any; readonly registry?: { setInitialDisabledPackageIds?: (ids: Iterable) => void }; - /** - * The idempotence guards for the two runners above, read directly. - * - * These are NOT part of any declared API: the engine attaches them with - * `(this as any)._defaultBodyRunner = runner` and publishes no reader, so - * "has another AppPlugin already installed one?" has no answer except - * reaching for the field. Declared here to keep that reach visible instead - * of laundering it through `any` — the fix is a public accessor on the - * engine beside `getHookMetricsRecorder`, which already exists for exactly - * this question about the metrics recorder. Filed on #4251. - */ - _defaultBodyRunner?: unknown; - _defaultActionRunner?: unknown; } /** The engine as AppPlugin uses it: the data contract plus those seams. */ @@ -326,13 +322,17 @@ export class AppPlugin implements Plugin { return; // no engine on this kernel — nothing to wire } if (!ql || typeof ql.setDefaultBodyRunner !== 'function') return; - if (ql._defaultBodyRunner) return; // another AppPlugin already installed one - ql.setDefaultBodyRunner(hookBodyRunnerFactory(new QuickJSScriptRunner(), { + // [#4251] The setter is first-wins — the engine keeps the first runner + // when several AppPlugins share one kernel; this caller no longer + // probes the engine's private `_defaultBodyRunner` field to find out. + const installed = ql.setDefaultBodyRunner(hookBodyRunnerFactory(new QuickJSScriptRunner(), { ql, logger: ctx.logger, appId: 'runtime-authored', })); - ctx.logger.info('[AppPlugin] Installed default hook body runner (runtime-authored hooks can execute)'); + if (installed !== false) { + ctx.logger.info('[AppPlugin] Installed default hook body runner (runtime-authored hooks can execute)'); + } } /** @@ -364,13 +364,15 @@ export class AppPlugin implements Plugin { return; // no engine on this kernel — nothing to wire } if (!ql || typeof ql.setDefaultActionRunner !== 'function') return; - if (ql._defaultActionRunner) return; // another AppPlugin already installed one - ql.setDefaultActionRunner(actionBodyRunnerFactory(new QuickJSScriptRunner(), { + // [#4251] First-wins setter — same as the hook body runner above. + const installed = ql.setDefaultActionRunner(actionBodyRunnerFactory(new QuickJSScriptRunner(), { ql, logger: ctx.logger, appId: 'runtime-authored', })); - ctx.logger.info('[AppPlugin] Installed default action body runner (runtime-authored actions can execute)'); + if (installed !== false) { + ctx.logger.info('[AppPlugin] Installed default action body runner (runtime-authored actions can execute)'); + } } /**