diff --git a/.changeset/flow-function-declared-effect.md b/.changeset/flow-function-declared-effect.md new file mode 100644 index 0000000000..ef1463ce5a --- /dev/null +++ b/.changeset/flow-function-declared-effect.md @@ -0,0 +1,56 @@ +--- +"@objectstack/spec": minor +"@objectstack/objectql": minor +"@objectstack/runtime": minor +"@objectstack/service-automation": minor +"@objectstack/cli": minor +--- + +feat(automation): a `script` node's purity contract is declared, and a function that writes can say so (#4396) + +The `script` executor's contract — *the named function returns a value; data I/O +stays on the flow graph* — existed only as a comment inside the executor, while +#4354's run summary depended on it. That summary reports no record metrics for a +`script` step precisely because a pure function's writes are downstream +`create_record` / `update_record` nodes counting themselves. A function that +wrote anyway made its run report `selected: 30, acted: 0` — indistinguishable +from the broken sweep the counters exist to detect, recorded permanently on +`sys_automation_run`. + +**The rule is now visible.** `ActionDescriptor` carries +`handlerContract: 'none' | 'pure'`, and the `script` descriptor publishes +`'pure'`, so the action catalog, the designer palette and the reference docs +state the rule an author has to follow instead of an executor holding it +privately. + +**And a legitimate writer can opt out honestly.** A `defineStack({ functions })` +entry may declare what it does, in either shape: + +```ts +defineStack({ + functions: { + scoreLead: (ctx) => ({ score: 42 }), // pure — the default + syncBilling: { handler: syncBilling, effect: 'writes' }, // declared writer + }, +}); +``` + +A step calling a declared writer reports `unmeasuredEffect`, so the run's +`unmeasured` tally keeps the broken-sweep query +(`selected > 0 AND acted = 0 AND unmeasured = 0`) off that flow — and only that +flow. Marking *every* `script` step unmeasured was rejected: it would blind the +detector on every flow that calls any function in order to cover the few that +break the rule. + +Nothing here is retired or renamed: a bare `functions: { fn }` entry is +unchanged and means `effect: 'pure'`. The declaration is carried end to end — +`ObjectQL.registerFunction` accepts `{ packageId, effect }` alongside the +existing `packageId` string and exposes `resolveFunctionEntry(name)`, +`objectstack build` lowers a declared entry without dropping it, and the +artifact loader re-attaches the module's callable to the declaration the JSON +carried. + +**Also fixed:** `bindHooksToEngine` returned before registering a bundle's +functions when the stack declared no hooks, so a flow-only app's +`defineStack({ functions })` reached the engine as nothing and every `script` +node calling one failed with "no function named 'x' is registered". diff --git a/content/docs/automation/flows.mdx b/content/docs/automation/flows.mdx index faede6a16f..0db5623b40 100644 --- a/content/docs/automation/flows.mdx +++ b/content/docs/automation/flows.mdx @@ -245,6 +245,44 @@ function fails the step loudly rather than passing silently. } ``` + + +It takes its `inputs`, **returns** a value, and a later **declarative** node +persists that value. Data I/O stays on the flow graph — the function itself does +no writes, and the runtime hands it nothing to write with (`ctx` carries +`input` / `variables` / `automation` / `logger`, no data engine). The `script` +action publishes this as `handlerContract: 'pure'` on its +[action descriptor](/docs/references/automation/node-executor). + +The rule is load-bearing, not stylistic. [Run summaries](#run-summaries) report +what a run did to the data, and a `script` step reports **no** record metrics +precisely because of it: every write a pure function causes is a downstream +`create_record` / `update_record` that counts itself. A function that writes +behind the platform's back makes its run claim it wrote nothing — the durable +`sys_automation_run` row then reads exactly like a broken sweep, permanently. + +If a function genuinely must write (an upstream billing system, a legacy API), +**declare it** so the run stays honest: + +```typescript +defineStack({ + functions: { + scoreLead: (ctx) => ({ score: 42 }), // pure — the default + syncBilling: { handler: syncBilling, effect: 'writes' }, // declared writer + }, +}); +``` + +A step that calls a declared writer is counted as an effect the platform cannot +measure (`unmeasured`), never as zero — so the broken-sweep query +`selected > 0 AND acted = 0 AND unmeasured = 0` stops firing on that flow, and +keeps working on every other flow that calls a function. Declaring changes what +is *reported*, not what is *allowed*: an undeclared writer is still counted as +having written nothing, and no runtime check can catch it — a function is +ordinary host code and may close over a client at module scope. + + + **Screen (flat fields):** The default shape. Each field is collected as a **bare flow variable**, so a @@ -623,7 +661,15 @@ instead: | `http`, mutating method, rejected / timed out | `unmeasured` — a 500 can arrive after the write landed | | `http`, `durable: true` | `acted: 1` — the outbox row is a real, durable effect | | `connector_action` | `unmeasured` | -| `script` | nothing — a registered function is **contractually pure**: data I/O stays on the flow graph, so every write it causes is a downstream node that counts itself | +| `script`, function declared pure (the default) | nothing — a registered function is **contractually pure**: data I/O stays on the flow graph, so every write it causes is a downstream node that counts itself | +| `script`, function declared `effect: 'writes'` | `unmeasured` — the function said it writes where the platform cannot see, so the run says the count is incomplete | + +The `script` row is a contract, not a measurement: nothing stops a registered +function from writing, so an **undeclared** writer still makes its run report +`acted: 0`. That is why the declaration exists and why it is worth using — see +[the purity callout](#node-examples). It is also why the reverse fix was +rejected: marking *every* `script` step `unmeasured` would blind the detector on +every flow that calls any function, to cover the few that break the rule. `unmeasured` propagates through `subflow` and `map` roll-ups, so a parent whose child dispatched an uncountable effect knows its own `acted` is incomplete. diff --git a/content/docs/references/automation/flow-function.mdx b/content/docs/references/automation/flow-function.mdx new file mode 100644 index 0000000000..40e4f938bd --- /dev/null +++ b/content/docs/references/automation/flow-function.mdx @@ -0,0 +1,109 @@ +--- +title: Flow Function +description: Flow Function protocol schemas +--- + +{/* ⚠️ AUTO-GENERATED — DO NOT EDIT. Run build-docs.ts to regenerate. Hand-written docs live in the module folders under content/docs/. */} + +@module automation/flow-function + +The contract for a **named handler function a `script` node invokes** — + +contributed by `defineStack(\{ functions \})` and resolved by name at execute + +time (#1870). + +## The rule, and why it lives here instead of in a comment + +A flow function is a PURE compute step: it receives its mapped `input`, + +RETURNS a value, and the node's `outputVariable` exposes that value as a flow + +variable so a later DECLARATIVE node persists it (`update_record fields: \{ + +ai_category: '\{aiResult.ai_category\}' \}`). Data I/O stays on the flow graph. + +That is not style advice. #4354's per-run summary reports what a run did to + +the data, and the `script` node reports NO record metrics *because* of this + +rule: every write a script causes is a downstream `create_record` / + +`update_record` that counts itself, so "this node touched no records" is the + +accurate answer rather than a guess. A function that writes anyway makes its + +run under-report — `selected: 30, acted: 0` on a run that wrote 30 invoices, + +which reads exactly like the broken sweep #4354 exists to detect, and the + +durable `sys_automation_run` row says so permanently. + +Until #4396 that rule lived ONLY in a comment inside the executor, so neither + +an author, a lint, nor the runtime could see the contract the summary was + +relying on. It is now declared in two halves: + +1. `ActionDescriptor.handlerContract` — `script` publishes `'pure'`, so the + +action catalog and the designer palette carry the rule an author reads. + +2. `FlowFunctionEffectSchema` — a function that legitimately writes + +DECLARES it, and its step then reports `unmeasuredEffect`, so the run + +says "cannot count" instead of claiming it wrote nothing. + +## What is deliberately not here + +A blanket `unmeasuredEffect` on every `script` step (the escape hatch #4354 + +gave `connector_action`) was rejected: it would suppress the broken-sweep + +signal on every flow that calls any function, in order to accommodate the + +flows that break the rule — paying for a rule-breaker with everyone else's + +signal, and fossilizing the violation as supported behaviour. + +Nor is this *enforcement*. The runtime hands a function no data reach — + +`FlowFunctionContext` in `@objectstack/service-automation` carries + +`input` / `variables` / `automation` / `logger` and no engine handle — but a + +function is ordinary host code and can close over a client at module scope. + +What the declaration buys is that the honest case is now *expressible*, and + +the platform's own counters stop being wrong for it. + + +**Source:** `packages/spec/src/automation/flow-function.zod.ts` + + +## TypeScript Usage + +```typescript +import { FlowFunctionEffect } from '@objectstack/spec/automation'; +import type { FlowFunctionEffect } from '@objectstack/spec/automation'; + +// Validate data +const result = FlowFunctionEffect.parse(data); +``` + +--- + +## FlowFunctionEffect + +What a script-node function does to data: 'pure' (computes and returns — the contract) or 'writes' (performs uncountable writes/effects, reported as unmeasured) + +### Allowed Values + +* `pure` +* `writes` + + +--- + diff --git a/content/docs/references/automation/index.mdx b/content/docs/references/automation/index.mdx index 11d43f78da..fe6cef7bf8 100644 --- a/content/docs/references/automation/index.mdx +++ b/content/docs/references/automation/index.mdx @@ -13,6 +13,7 @@ This section contains all protocol schemas for the automation layer of ObjectSta + diff --git a/content/docs/references/automation/meta.json b/content/docs/references/automation/meta.json index 9a54bd03d2..30a557be1d 100644 --- a/content/docs/references/automation/meta.json +++ b/content/docs/references/automation/meta.json @@ -21,6 +21,7 @@ "job", "---More---", "builtin-node-config", + "flow-function", "io-node-config", "schemaless-node-config" ] diff --git a/content/docs/references/automation/node-executor.mdx b/content/docs/references/automation/node-executor.mdx index d4b5cb40d7..e6e35ff1b8 100644 --- a/content/docs/references/automation/node-executor.mdx +++ b/content/docs/references/automation/node-executor.mdx @@ -76,6 +76,7 @@ Canonical cross-paradigm action/node descriptor (ADR-0018) | **supportsRetry** | `boolean` | ✅ | Supports retry on failure | | **needsOutbox** | `boolean` | ✅ | Dispatch via service-messaging outbox (retry/idempotency/dead-letter) | | **isAsync** | `boolean` | ✅ | Suspends the flow awaiting an external reply | +| **handlerContract** | `Enum<'none' \| 'pure'>` | ✅ | Effect contract for author-supplied code this action invokes: 'none' (invokes none) or 'pure' (must not write — it returns a value and the flow graph persists it) | | **resumeAuthority** | `Enum<'any' \| 'service'>` | ✅ | Who may resume a run this node suspended: 'any' (the generic resume route) or 'service' (only the owning service, e.g. approvals) | | **maturity** | `Enum<'ga' \| 'beta' \| 'reserved'>` | ✅ | Runtime maturity: ga (shipped), beta, or reserved (contract only — designers grey this out) | | **source** | `Enum<'builtin' \| 'plugin'>` | ✅ | builtin = platform baseline; plugin = third-party contributed | diff --git a/content/docs/references/automation/schemaless-node-config.mdx b/content/docs/references/automation/schemaless-node-config.mdx index 32c7b4ceef..ab971fe805 100644 --- a/content/docs/references/automation/schemaless-node-config.mdx +++ b/content/docs/references/automation/schemaless-node-config.mdx @@ -145,7 +145,7 @@ const result = DecisionCondition.parse(data); | Property | Type | Required | Description | | :--- | :--- | :--- | :--- | | **actionType** | `string` | optional | How this step runs: a built-in side effect ('email' \| 'slack'), the 'invoke_function' marker, or shorthand for a registered-function name | -| **function** | `string` | optional | Registered function to call (defineStack(`{ functions }`)); takes precedence over actionType | +| **function** | `string` | optional | Registered function to call (defineStack(`{ functions }`)); takes precedence over actionType. Contractually pure — it returns a value a later declarative node persists | | **inputs** | `Record` | optional | Inputs passed to the function (values interpolate `{token}` templates) | | **outputVariable** | `string` | optional | Flow variable the function's return value is bound to | | **template** | `string` | optional | Built-in side effects only: message template id | diff --git a/docs/audits/2026-07-unknown-key-strictness-ledger.md b/docs/audits/2026-07-unknown-key-strictness-ledger.md index 2654d2afb5..12b9463af4 100644 --- a/docs/audits/2026-07-unknown-key-strictness-ledger.md +++ b/docs/audits/2026-07-unknown-key-strictness-ledger.md @@ -165,7 +165,7 @@ tightening (the #4001 "sharing-rule lesson": candidates, not verdicts). | `external-catalog.zod.ts` | 4 | wire (p) | | | `field-value.zod.ts` / `seed.zod.ts` / `validation.zod.ts` | 1 ea | mixed (p) | | -### `automation/` — 98 sites +### `automation/` — 99 sites | File | Sites | Class | Note | |---|---|---|---| @@ -182,6 +182,7 @@ tightening (the #4001 "sharing-rule lesson": candidates, not verdicts). | `builtin-node-config.zod.ts` | 8 | authorable | Same family (#4045): the CRUD quartet, `screen`, `map`. Written from what the executors read rather than from the descriptors' `configSchema` literals, and reconciled bidirectionally by `builtin-node-form-zod-ledger.test.ts` — so unlike most rows here, this one already has a drift check of its own. Same candidacy note as `io-node-config` | | `schemaless-node-config.zod.ts` | 4 | authorable | Same family, third panel (#4278): `script` / `subflow` / `decision` (+ the decision branch item) — the descriptor-schemaless nodes whose form lives in objectui's hand-written table. Written from the executors; the drift check is objectui's `flow-node-config.spec-reconciliation` test (cross-repo, via the published exports). Contract exports only — nothing parses node config with them yet, so strictness candidacy follows `io-node-config` | | `webhook.zod.ts` | 1 | authorable (p) | spec-only (#3461) | +| `flow-function.zod.ts` | 1 | authorable | `FlowFunctionDeclarationSchema` (#4396) — the `{ handler, effect }` form of a `defineStack({ functions })` entry. Authored, but note what an undeclared key here would be: a sibling of a **live function**, not data. `defineStack`'s union already rejects a record whose `handler` is not callable, and the boot-path reader is the hand-written `normalizeFlowFunctionEntry` rather than a `.parse()` (re-validating a live handler every boot buys nothing), so strictness would bind at authoring only. Candidate on the same verify-first rule as its `*-node-config` neighbours | ### `security/` — 20 sites diff --git a/packages/cli/src/utils/build-runtime.ts b/packages/cli/src/utils/build-runtime.ts index ceda474b61..3b969ce3ba 100644 --- a/packages/cli/src/utils/build-runtime.ts +++ b/packages/cli/src/utils/build-runtime.ts @@ -76,10 +76,13 @@ function collect(cfg) { } } } - // Top-level functions map + // Top-level functions map — a value is the handler, or a declaration + // record stating its effect (\`{ handler, effect: 'writes' }\`). if (cfg.functions && !Array.isArray(cfg.functions) && typeof cfg.functions === 'object') { for (const [k, v] of Object.entries(cfg.functions)) { - if (typeof v === 'function' && REFS.has(k)) out[k] = v; + if (!REFS.has(k)) continue; + if (typeof v === 'function') out[k] = v; + else if (v && typeof v === 'object' && typeof v.handler === 'function') out[k] = v.handler; } } // Top-level functions array diff --git a/packages/cli/src/utils/lower-callables.test.ts b/packages/cli/src/utils/lower-callables.test.ts index f6ec974a6b..2d637bcd38 100644 --- a/packages/cli/src/utils/lower-callables.test.ts +++ b/packages/cli/src/utils/lower-callables.test.ts @@ -87,3 +87,40 @@ describe('lowerCallables — only `target` binds a handler (#3855)', () => { expect(result.functions[ref]()).toBe('preferred'); }); }); + +// ── #4396: a function's DECLARATION survives lowering ─────────────────────── +// +// `functions: { syncBilling: { handler, effect: 'writes' } }` is how a function +// that legitimately writes keeps its run's metrics honest. Lowering is the +// first place a built artifact could lose that: the map branch bound only bare +// callables, so a declared entry was dropped entirely — the function then went +// missing from `objectstack.json` AND from the runtime bundle, and every +// `script` node calling it failed on a built deployment while working from +// source. +describe('lowerCallables — declared `functions` entries (#4396)', () => { + const functionsOf = (result: { lowered: Record }) => + (result.lowered as { functions: Record }).functions; + + it('lowers a bare handler to its ref, unchanged', () => { + const result = lowerCallables({ functions: { scoreLead: () => 'scored' } }); + expect(functionsOf(result).scoreLead).toBe('scoreLead'); + expect((result.functions.scoreLead as () => string)()).toBe('scored'); + }); + + it('lowers a declared entry and keeps what it declared', () => { + const result = lowerCallables({ + functions: { syncBilling: { handler: () => 'synced', effect: 'writes' } }, + }); + expect(functionsOf(result).syncBilling).toEqual({ handler: 'syncBilling', effect: 'writes' }); + expect((result.functions.syncBilling as () => string)()).toBe('synced'); + expect(result.count).toBe(1); + }); + + it('keeps `effect` on the array form too', () => { + const result = lowerCallables({ + functions: [{ name: 'syncBilling', handler: () => 'synced', effect: 'writes' }], + }); + const [entry] = functionsOf(result) as unknown as Array>; + expect(entry).toEqual({ name: 'syncBilling', handler: 'syncBilling', effect: 'writes' }); + }); +}); diff --git a/packages/cli/src/utils/lower-callables.ts b/packages/cli/src/utils/lower-callables.ts index 3c084d6264..b9b8a91f1d 100644 --- a/packages/cli/src/utils/lower-callables.ts +++ b/packages/cli/src/utils/lower-callables.ts @@ -148,13 +148,23 @@ export function lowerCallables(input: Record): LoweringResult { }); (lowered as Record).functions = arr; } else if (isPlainObject(fnsField)) { - const out: Record = {}; + const out: Record = {}; for (const [key, value] of Object.entries(fnsField)) { if (typeof value === 'function') { const ref = uniqueName(key, taken); taken.add(ref); functions[ref] = value as AnyFn; out[ref] = ref; + } else if (isPlainObject(value) && typeof value.handler === 'function') { + // A DECLARED entry (`{ handler, effect: 'writes' }`, #4396). Lower the + // callable exactly like the bare form and keep the declaration beside + // it, so what the function said about itself survives into the + // artifact — dropping it here would silently un-declare the function + // on every built deployment while it kept working from source. + const ref = uniqueName(key, taken); + taken.add(ref); + functions[ref] = value.handler as AnyFn; + out[ref] = { ...value, handler: ref }; } } // Preserve any pre-existing string entries (legacy bundles). diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 1851170138..34f243233d 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -25,6 +25,7 @@ import { isDataMigrationFlagVerified, } from '@objectstack/spec/system'; import { ExecutionContext, ExecutionContextInput, ExecutionContextSchema } from '@objectstack/spec/kernel'; +import type { FlowFunctionEffect } from '@objectstack/spec/automation'; import { IDataDriver, IDataEngine, @@ -433,9 +434,33 @@ export interface HookEntry { } /** Function registry entry — see `registerFunction`. */ -interface FunctionEntry { +export interface FunctionEntry { handler: HookHandler; packageId?: string; + /** + * What this function does to data, as DECLARED at registration (#4396). + * Only a `script`-node caller reads it: a flow function is contractually + * pure, and the run summary counts on that, so a function that legitimately + * writes declares `'writes'` and its step is reported as an effect the + * platform cannot count rather than as none. Absent ⇒ `'pure'`. + * + * Carried on the registry entry rather than beside it because this IS part of + * the registration — one registry, and what a caller needs to know about a + * function travels with the function. + */ + effect?: FlowFunctionEffect; +} + +/** + * Declarations that may accompany a `registerFunction` call. The bare + * `packageId` string is still accepted in that position (every existing caller + * passes one), so this widens the signature without a migration. + */ +export interface FunctionRegistrationOptions { + /** Owning package — the unit `unregisterFunctionsByPackage` removes. */ + packageId?: string; + /** Declared data effect (#4396); omit for the pure default. */ + effect?: FlowFunctionEffect; } /** @@ -757,14 +782,27 @@ export class ObjectQL implements IObjectQLEngine { /** * Register a named function handler that can later be referenced by - * string from a `Hook.handler` field. This is the JSON-safe form of + * string from a `Hook.handler` field, an `Action.target`, or a flow + * `script` node's `config.function`. This is the JSON-safe form of * handler binding — declarative metadata persisted to disk or shipped * over the wire only carries the name. + * + * The third parameter accepts either the owning `packageId` (its original + * shape, unchanged for every existing caller) or a + * {@link FunctionRegistrationOptions} record that also carries what the + * function DECLARES about itself — today its data `effect` (#4396). */ - registerFunction(name: string, handler: HookHandler, packageId?: string): void { + registerFunction( + name: string, + handler: HookHandler, + packageIdOrOptions?: string | FunctionRegistrationOptions, + ): void { if (!name || typeof handler !== 'function') return; - this.functions.set(name, { handler, packageId }); - this.logger.debug('Registered function', { name, packageId }); + const opts: FunctionRegistrationOptions = + typeof packageIdOrOptions === 'string' ? { packageId: packageIdOrOptions } : (packageIdOrOptions ?? {}); + const { packageId, effect } = opts; + this.functions.set(name, { handler, packageId, ...(effect ? { effect } : {}) }); + this.logger.debug('Registered function', { name, packageId, effect }); } /** Look up a registered function by name. */ @@ -772,6 +810,18 @@ export class ObjectQL implements IObjectQLEngine { return this.functions.get(name)?.handler; } + /** + * Look up a registered function's FULL entry — the handler plus whatever it + * declared about itself (#4396). `resolveFunction` above answers "can I call + * it"; a caller that must also report what the call did (the automation + * engine's `script` node, feeding the #4354 run summary) needs the + * declaration, and reading it off the same registry keeps the two from + * drifting. + */ + resolveFunctionEntry(name: string): Readonly | undefined { + return this.functions.get(name); + } + /** Remove all functions registered under a given `packageId`. */ unregisterFunctionsByPackage(packageId: string): number { if (!packageId) return 0; diff --git a/packages/objectql/src/function-registry-effect.test.ts b/packages/objectql/src/function-registry-effect.test.ts new file mode 100644 index 0000000000..4d1545a156 --- /dev/null +++ b/packages/objectql/src/function-registry-effect.test.ts @@ -0,0 +1,136 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The function registry carries what a function DECLARED about itself (#4396). + * + * One registry serves hooks, actions and flow `script` nodes, so the + * declaration has to travel WITH the handler: the automation engine reads it + * back through this registry to report whether a run's `acted` count covers + * everything the run did. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { ObjectQL } from './engine.js'; +import { bindHooksToEngine } from './hook-binder.js'; +import type { Hook, HookContext } from '@objectstack/spec/data'; + +describe('ObjectQL.registerFunction — declarations (#4396)', () => { + it('keeps the packageId-string form working unchanged', () => { + const engine = new ObjectQL(); + const fn = () => {}; + engine.registerFunction('scoreLead', fn, 'app:demo'); + expect(engine.resolveFunction('scoreLead')).toBe(fn); + expect(engine.resolveFunctionEntry('scoreLead')).toEqual({ handler: fn, packageId: 'app:demo' }); + }); + + it('records a declared effect and hands it back on the entry', () => { + const engine = new ObjectQL(); + const fn = () => {}; + engine.registerFunction('syncBilling', fn, { packageId: 'app:demo', effect: 'writes' }); + expect(engine.resolveFunction('syncBilling')).toBe(fn); + expect(engine.resolveFunctionEntry('syncBilling')).toEqual({ + handler: fn, + packageId: 'app:demo', + effect: 'writes', + }); + }); + + it('leaves the effect absent when nothing was declared — absent reads as pure', () => { + const engine = new ObjectQL(); + engine.registerFunction('scoreLead', () => {}, { packageId: 'app:demo' }); + expect(engine.resolveFunctionEntry('scoreLead')?.effect).toBeUndefined(); + }); + + it('still unregisters by package with the options form', () => { + const engine = new ObjectQL(); + engine.registerFunction('syncBilling', () => {}, { packageId: 'app:demo', effect: 'writes' }); + expect(engine.unregisterFunctionsByPackage('app:demo')).toBe(1); + expect(engine.resolveFunctionEntry('syncBilling')).toBeUndefined(); + }); + + it('answers undefined for an unknown name', () => { + expect(new ObjectQL().resolveFunctionEntry('nope')).toBeUndefined(); + }); +}); + +describe('bindHooksToEngine — function entries (#4396)', () => { + it('registers a declared entry with both its handler and its effect', () => { + const engine = new ObjectQL(); + const syncBilling = () => {}; + bindHooksToEngine(engine, [], { + packageId: 'app:demo', + functions: { syncBilling: { handler: syncBilling, effect: 'writes' } }, + }); + expect(engine.resolveFunctionEntry('syncBilling')).toEqual({ + handler: syncBilling, + packageId: 'app:demo', + effect: 'writes', + }); + }); + + it('resolves a string-named hook handler through a declared entry', async () => { + const engine = new ObjectQL(); + const calls: string[] = []; + const hook: Hook = { + name: 'h1', + object: ['account'], + events: ['beforeInsert'], + handler: 'syncBilling', + } as Hook; + + const result = bindHooksToEngine(engine, [hook], { + packageId: 'app:demo', + functions: { syncBilling: { handler: () => { calls.push('ran'); }, effect: 'writes' } }, + }); + + expect(result.errors).toEqual([]); + expect(result.registered).toBe(1); + await engine.triggerHooks('beforeInsert', { + object: 'account', + event: 'beforeInsert', + input: { data: {} }, + } as HookContext); + expect(calls).toEqual(['ran']); + }); + + it('warns and keeps the run measurable when an effect spelling is unreadable', () => { + const engine = new ObjectQL(); + const warn = vi.fn(); + bindHooksToEngine(engine, [], { + packageId: 'app:demo', + // 'write' is not a member — read as an uncountable write rather than as + // the pure default, which would silently under-report the run. + functions: { syncBilling: { handler: () => {}, effect: 'write' } as never }, + logger: { debug: vi.fn(), info: vi.fn(), warn, error: vi.fn() }, + }); + expect(engine.resolveFunctionEntry('syncBilling')?.effect).toBe('writes'); + expect(warn).toHaveBeenCalledWith( + expect.stringContaining('unrecognized function effect'), + expect.objectContaining({ name: 'syncBilling', effect: 'write' }), + ); + }); +}); + +/** + * `functions` is not a hook accessory (found while wiring #4396). + * + * The binder bailed out on an empty `hooks` array BEFORE registering the + * bundle's functions, so a stack that declared functions and no hooks — the + * shape a flow-only app has — registered none of them, and every `script` node + * calling one failed with "no function named 'x' is registered". + */ +describe('bindHooksToEngine — functions without hooks', () => { + it('registers the bundle functions even when the stack declares no hooks', () => { + const engine = new ObjectQL(); + const scoreLead = () => {}; + bindHooksToEngine(engine, [], { packageId: 'app:demo', functions: { scoreLead } }); + expect(engine.resolveFunction('scoreLead')).toBe(scoreLead); + }); + + it('registers them for an undefined hooks list too', () => { + const engine = new ObjectQL(); + const scoreLead = () => {}; + bindHooksToEngine(engine, undefined, { packageId: 'app:demo', functions: { scoreLead } }); + expect(engine.resolveFunction('scoreLead')).toBe(scoreLead); + }); +}); diff --git a/packages/objectql/src/hook-binder.ts b/packages/objectql/src/hook-binder.ts index 6026723006..535cead1d4 100644 --- a/packages/objectql/src/hook-binder.ts +++ b/packages/objectql/src/hook-binder.ts @@ -22,6 +22,7 @@ */ import type { Hook } from '@objectstack/spec/data'; +import { normalizeFlowFunctionEntry, type FlowFunctionEntry } from '@objectstack/spec/automation'; import type { ObjectQL, HookHandler } from './engine.js'; import { wrapDeclarativeHook } from './hook-wrappers.js'; import type { HookMetricsRecorder } from './hook-metrics.js'; @@ -34,8 +35,14 @@ export interface BindHooksOptions { * Optional name → function map for resolving string `handler` references. * Typically supplied by `defineStack({ functions })` and merged with any * functions previously registered on the engine. + * + * A value may be the handler itself or a declaration record stating what the + * function does (`{ handler, effect: 'writes' }`, #4396) — the same two + * spellings `defineStack({ functions })` accepts. The declaration is stored + * on the registry entry, where the `script` node reads it to report the run's + * metrics honestly; hook binding itself only ever needs the handler. */ - functions?: Record; + functions?: Record; /** * Optional factory that converts a metadata-only `Hook.body` (L1 expression @@ -104,6 +111,45 @@ export function bindHooksToEngine( const logger = opts.logger ?? noopLogger; const result: BindHooksResult = { registered: 0, skipped: 0, errors: [] }; + // Pre-load any inline functions supplied via `bundle.functions` so + // string-handler resolution works. An entry may declare its effect (#4396); + // that declaration is registered WITH the handler, so a later `script`-node + // caller reads both off the one registry. + // + // BEFORE the no-hooks bail-out below, deliberately: `functions` is not a + // hook accessory. It also feeds `Action.target` string refs and a flow + // `script` node's `config.function`, so a stack that declares functions and + // no hooks — the shape #4396's example has — registered NOTHING and every + // such `script` node failed with "no function named 'x' is registered", + // naming the one thing the author had actually done. + if (opts.functions && typeof (engine as any).registerFunction === 'function') { + for (const [name, entry] of Object.entries(opts.functions)) { + const fn = normalizeFlowFunctionEntry(entry); + if (!fn) { + logger.warn('[hook-binder] skipping function entry with no callable handler', { name }); + continue; + } + if (fn.unrecognizedEffect !== undefined) { + logger.warn('[hook-binder] unrecognized function effect — counted as an uncountable write', { + name, + effect: fn.unrecognizedEffect, + expected: "'pure' | 'writes'", + }); + } + try { + (engine as any).registerFunction(name, fn.handler, { + packageId: opts.packageId, + effect: fn.effect, + }); + } catch (err: any) { + logger.warn('[hook-binder] failed to register function', { + name, + error: err?.message, + }); + } + } + } + if (!Array.isArray(hooks) || hooks.length === 0) { return result; } @@ -121,21 +167,6 @@ export function bindHooksToEngine( } } - // Pre-load any inline functions supplied via `bundle.functions` so - // string-handler resolution works. - if (opts.functions && typeof (engine as any).registerFunction === 'function') { - for (const [name, fn] of Object.entries(opts.functions)) { - try { - (engine as any).registerFunction(name, fn, opts.packageId); - } catch (err: any) { - logger.warn('[hook-binder] failed to register function', { - name, - error: err?.message, - }); - } - } - } - for (const hook of hooks) { try { const resolved = resolveHandler(engine, hook, opts); @@ -276,9 +307,10 @@ function resolveHandler( if (typeof h === 'function') return h as HookHandler; if (typeof h === 'string' && h.length > 0) { // Try the per-bundle map first (hot path during initial bind), - // then fall back to whatever the engine already knows. - const fromBundle = opts.functions?.[h]; - if (typeof fromBundle === 'function') return fromBundle; + // then fall back to whatever the engine already knows. A declaration + // record resolves to its handler — a hook cares only about the callable. + const fromBundle = normalizeFlowFunctionEntry(opts.functions?.[h]); + if (fromBundle) return fromBundle.handler as HookHandler; if (typeof (engine as any).resolveFunction === 'function') { const fn = (engine as any).resolveFunction(h); if (typeof fn === 'function') return fn as HookHandler; diff --git a/packages/qa/dogfood/test/fixtures/flow-function-effect-fixture.ts b/packages/qa/dogfood/test/fixtures/flow-function-effect-fixture.ts new file mode 100644 index 0000000000..bda1b73a31 --- /dev/null +++ b/packages/qa/dogfood/test/fixtures/flow-function-effect-fixture.ts @@ -0,0 +1,78 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// Flow-function EFFECT fixture (#4396) — the declaration a run's metrics depend on. +// +// A `script` node's function is contractually pure: it returns a value and a +// later declarative node persists it. #4354's run summary counts on that — a +// `script` step reports no record metrics because every write a pure function +// causes is a downstream node counting itself. A function that writes anyway +// makes its run report `acted: 0`, which is exactly what a dead sweep looks +// like. +// +// So a function that legitimately writes DECLARES it, and this fixture is the +// end-to-end proof that the declaration survives every seam between the author +// and the counter: `defineStack({ functions })` → AppPlugin's bundle collection +// → the ObjectQL function registry → the automation service's resolver bridge → +// the `script` executor → the run summary on the trigger response. +// +// Two flows, identical but for the function they call, so the difference in the +// reported summary can only come from the declaration. + +import { defineStack } from '@objectstack/spec'; +import { ObjectSchema, Field } from '@objectstack/spec/data'; + +/** One object, so the sweep flow has something real to select. */ +export const FxInvoice = ObjectSchema.create({ + name: 'fxn_invoice', + // [ADR-0090 D1] grandfather stamp: the gate under test is run METRICS, not + // owner-sharing. + sharingModel: 'public_read_write', + label: 'Invoice', + pluralLabel: 'Invoices', + fields: { + name: Field.text({ label: 'Name', required: true }), + status: Field.text({ label: 'Status' }), + }, +}); + +/** start → get_record → script → end. `fn` is the only thing that varies. */ +const sweepFlow = (name: string, fn: string) => ({ + name, + label: name, + type: 'autolaunched', + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { + id: 'select', + type: 'get_record', + label: 'Select invoices', + config: { objectName: 'fxn_invoice', filter: { status: 'open' }, outputVariable: 'invoices' }, + }, + { id: 'run', type: 'script', label: 'Run', config: { function: fn, inputs: { count: '{invoices}' } } }, + { id: 'end', type: 'end', label: 'End' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'select' }, + { id: 'e2', source: 'select', target: 'run' }, + { id: 'e3', source: 'run', target: 'end' }, + ], +}); + +export const flowFunctionEffectStack = defineStack({ + manifest: { + id: 'com.dogfood.flow_function_effect', + namespace: 'fxn', + version: '0.0.0', + type: 'app', + name: 'Flow Function Effect Fixture', + description: "Proves a flow function's declared effect reaches the run summary (#4396).", + }, + objects: [FxInvoice], + flows: [sweepFlow('fxn_pure_sweep', 'scoreInvoices'), sweepFlow('fxn_writing_sweep', 'syncBilling')], + functions: { + // Undeclared ⇒ pure, the contract's default and the short spelling of it. + scoreInvoices: () => ({ scored: true }), + // Declared: writes somewhere the platform cannot see or count. + syncBilling: { handler: () => ({ ok: true }), effect: 'writes' }, + }, +}); diff --git a/packages/qa/dogfood/test/flow-function-effect.dogfood.test.ts b/packages/qa/dogfood/test/flow-function-effect.dogfood.test.ts new file mode 100644 index 0000000000..92508caed7 --- /dev/null +++ b/packages/qa/dogfood/test/flow-function-effect.dogfood.test.ts @@ -0,0 +1,80 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// FLOW FUNCTION EFFECT proof (#4396), through the real HTTP + automation stack. +// +// The claim under test is a counting claim, and it spans four packages: an +// author writes `functions: { syncBilling: { handler, effect: 'writes' } }` in +// `defineStack`; AppPlugin collects it; ObjectQL's function registry stores it +// beside the handler; the automation service bridges it into the engine; and +// the `script` executor turns it into `unmeasuredEffect` on the step, which +// #4354's summary folds into the run's `unmeasured` tally. +// +// Any one of those seams can drop the declaration silently — the run still +// succeeds, it just reports `acted: 0, unmeasured: 0` on a run that wrote, which +// is indistinguishable from a dead sweep and is the whole defect #4396 is +// about. Only an end-to-end assertion can tell the difference, so this boots the +// app, triggers both flows over HTTP, and reads the summary the run reports. + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { bootStack, type VerifyStack } from '@objectstack/verify'; +import { flowFunctionEffectStack } from './fixtures/flow-function-effect-fixture.js'; + +interface RunSummary { + selected: number; + acted: number; + unmeasured?: number; + nodes: Array<{ nodeId: string; unmeasured?: number; acted?: number; selected?: number }>; +} + +describe('objectstack verify FLOW: a function\'s declared effect reaches the run summary (#4396)', () => { + let stack: VerifyStack; + let token: string; + + beforeAll(async () => { + stack = await bootStack(flowFunctionEffectStack, { automation: true }); + token = await stack.signIn(); + // Two open invoices, so `selected > 0` — the first half of the + // broken-sweep signal the `unmeasured` tally has to keep honest. + for (const name of ['inv-1', 'inv-2']) { + const res = await stack.apiAs(token, 'POST', '/data/fxn_invoice', { name, status: 'open' }); + expect(res.status, `seed ${name} failed: ${res.status} ${await res.clone().text()}`).toBeLessThan(300); + } + }, 60_000); + + afterAll(async () => { + await stack?.stop(); + }); + + async function runSummaryOf(flow: string): Promise { + const res = await stack.apiAs(token, 'POST', `/automation/${flow}/trigger`, {}); + expect(res.status, `trigger ${flow} failed: ${res.status} ${await res.clone().text()}`).toBeLessThan(300); + const body = (await res.json()) as { data?: { success?: boolean; error?: string; summary?: RunSummary } }; + // A `script` node whose function never registered fails LOUDLY (#1870), so + // this assertion is also the proof that `defineStack({ functions })` + // reached the engine at all. + expect(body.data?.success, `run failed: ${body.data?.error}`).toBe(true); + expect(body.data?.summary, 'run reported no summary').toBeTruthy(); + return body.data!.summary!; + } + + it('a PURE function leaves the run fully measured — `acted: 0` means zero', async () => { + const summary = await runSummaryOf('fxn_pure_sweep'); + expect(summary.selected).toBeGreaterThan(0); + expect(summary.acted).toBe(0); + // Nothing uncountable happened, so the broken-sweep query + // (`selected > 0 AND acted = 0 AND unmeasured = 0`) is free to fire on this + // run — which is correct: a pure function that writes nothing IS a sweep + // that did nothing. + expect(summary.unmeasured).toBe(0); + expect(summary.nodes.find((n) => n.nodeId === 'run')?.unmeasured).toBeUndefined(); + }); + + it("a DECLARED writer's run says the count is incomplete instead of claiming zero", async () => { + const summary = await runSummaryOf('fxn_writing_sweep'); + expect(summary.selected).toBeGreaterThan(0); + expect(summary.acted).toBe(0); + // The one bit that must survive all four seams. + expect(summary.unmeasured).toBe(1); + expect(summary.nodes.find((n) => n.nodeId === 'run')?.unmeasured).toBe(1); + }); +}); diff --git a/packages/runtime/src/app-plugin.ts b/packages/runtime/src/app-plugin.ts index 95a7a34359..a0a4a7eadc 100644 --- a/packages/runtime/src/app-plugin.ts +++ b/packages/runtime/src/app-plugin.ts @@ -8,6 +8,7 @@ import { recordSeedOutcome } from './seed-summary.js'; import { mergeSeedDatasets, readSeedDatasets, registerSeedReplayerOnce } from './seed-datasets.js'; import { loadDisabledPackageIds } from './package-state-store.js'; import type { IJobService, IMetadataService, IObjectQLEngine, II18nService } from '@objectstack/spec/contracts'; +import { normalizeFlowFunctionEntry, type NormalizedFlowFunction } from '@objectstack/spec/automation'; import { readServiceSelfInfo } from '@objectstack/spec/api'; import { QuickJSScriptRunner } from './sandbox/quickjs-runner.js'; import { hookBodyRunnerFactory, actionBodyRunnerFactory } from './sandbox/body-runner.js'; @@ -638,7 +639,19 @@ export class AppPlugin implements Plugin { // cases — both will coexist on the engine. try { const hooks = collectBundleHooks(this.bundle); - const functions = collectBundleFunctions(this.bundle); + // Entries, not bare handlers: each function's declared `effect` + // (#4396) rides along to the registry, where a `script` node reads + // it to report what its run actually did. + const functions = collectBundleFunctionEntries(this.bundle); + for (const [name, fn] of Object.entries(functions)) { + if (fn.unrecognizedEffect === undefined) continue; + ctx.logger.warn('[AppPlugin] unrecognized function effect — counted as an uncountable write', { + appId, + name, + effect: fn.unrecognizedEffect, + expected: "'pure' | 'writes'", + }); + } if (hooks.length > 0 || Object.keys(functions).length > 0) { if (typeof ql.bindHooks === 'function') { ql.bindHooks(hooks, { @@ -1384,27 +1397,33 @@ export function collectBundleActions( } /** - * Collect a name → handler map from `bundle.functions`. Accepted shapes: + * Collect a name → {@link NormalizedFlowFunction} map from `bundle.functions`, + * keeping each entry's DECLARATION (its data `effect`, #4396) attached to the + * handler. Accepted shapes: * - * - `{ functions: { foo: fn, bar: fn } }` ← preferred map form - * - `{ functions: [{ name: 'foo', handler: fn }] }` ← array of records + * - `{ functions: { foo: fn, bar: fn } }` ← preferred map form + * - `{ functions: { foo: { handler: fn, effect: 'writes' } } }` ← declared map entry + * - `{ functions: [{ name: 'foo', handler: fn, effect: … }] }` ← array of records * - * String-named hook handlers (`Hook.handler: 'foo'`) are resolved against - * this map (and the engine's persistent function registry). + * The declaration matters at exactly one consumer — a `script` node reporting + * what its run did to the data — but it has to survive the collection step to + * get there, and dropping it here is how the shape would silently become + * unsupported. */ -export function collectBundleFunctions(bundle: any): Record any> { - const out: Record any> = {}; +export function collectBundleFunctionEntries(bundle: any): Record { + const out: Record = {}; const merge = (src: any) => { if (!src) return; if (Array.isArray(src)) { for (const item of src) { - if (item && typeof item.name === 'string' && typeof item.handler === 'function') { - out[item.name] = item.handler; - } + if (!item || typeof item.name !== 'string') continue; + const fn = normalizeFlowFunctionEntry(item); + if (fn) out[item.name] = fn; } } else if (typeof src === 'object') { - for (const [name, fn] of Object.entries(src)) { - if (typeof fn === 'function') out[name] = fn as any; + for (const [name, entry] of Object.entries(src)) { + const fn = normalizeFlowFunctionEntry(entry); + if (fn) out[name] = fn; } } }; @@ -1412,3 +1431,17 @@ export function collectBundleFunctions(bundle: any): Record any> { + const out: Record any> = {}; + for (const [name, fn] of Object.entries(collectBundleFunctionEntries(bundle))) { + out[name] = fn.handler as (ctx: any) => any; + } + return out; +} diff --git a/packages/runtime/src/artifact-function-declarations.test.ts b/packages/runtime/src/artifact-function-declarations.test.ts new file mode 100644 index 0000000000..141e369c70 --- /dev/null +++ b/packages/runtime/src/artifact-function-declarations.test.ts @@ -0,0 +1,71 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * A function's DECLARATION survives the built-artifact path (#4396). + * + * `objectstack build` splits every callable in two: the JSON artifact keeps a + * serialisable `functions` entry, the sibling `objectstack-runtime.*.mjs` keeps + * the callable. The merge that puts them back together used to be a plain + * overwrite, which kept the handler and dropped everything the entry declared — + * so `effect: 'writes'` worked from source and silently vanished on every built + * deployment, taking the run's honest `unmeasured` count with it. + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { mergeRuntimeModule } from './load-artifact-bundle.js'; +import { collectBundleFunctionEntries } from './app-plugin.js'; + +let dir: string; +let artifactPath: string; + +beforeAll(() => { + dir = mkdtempSync(join(tmpdir(), 'os-artifact-fns-')); + artifactPath = join(dir, 'objectstack.json'); + writeFileSync( + join(dir, 'objectstack-runtime.mjs'), + 'export const functions = {\n' + + ' scoreLead: () => ({ score: 1 }),\n' + + ' syncBilling: () => ({ ok: true }),\n' + + '};\n', + ); +}); + +afterAll(() => { + rmSync(dir, { recursive: true, force: true }); +}); + +describe('mergeRuntimeModule — declared functions', () => { + it('re-attaches the module callable to the declaration the JSON carried', async () => { + const bundle: any = { + runtimeModule: './objectstack-runtime.mjs', + // What `lowerCallables` emits: a ref string for a bare handler, a + // declaration record for a declared one. + functions: { + scoreLead: 'scoreLead', + syncBilling: { handler: 'syncBilling', effect: 'writes' }, + }, + }; + + await mergeRuntimeModule(bundle, artifactPath); + + expect(typeof bundle.functions.scoreLead).toBe('function'); + expect(typeof bundle.functions.syncBilling.handler).toBe('function'); + expect(bundle.functions.syncBilling.effect).toBe('writes'); + + // ...and the collector downstream reads both back. + const entries = collectBundleFunctionEntries(bundle); + expect(entries.scoreLead.effect).toBe('pure'); + expect(entries.syncBilling.effect).toBe('writes'); + expect((entries.syncBilling.handler as () => unknown)()).toEqual({ ok: true }); + }); + + it('leaves a bundle with no runtimeModule alone', async () => { + const handler = () => ({ ok: true }); + const bundle: any = { functions: { syncBilling: { handler, effect: 'writes' } } }; + await mergeRuntimeModule(bundle, artifactPath); + expect(bundle.functions.syncBilling).toEqual({ handler, effect: 'writes' }); + }); +}); diff --git a/packages/runtime/src/bundle-function-entries.test.ts b/packages/runtime/src/bundle-function-entries.test.ts new file mode 100644 index 0000000000..0b238a4bc1 --- /dev/null +++ b/packages/runtime/src/bundle-function-entries.test.ts @@ -0,0 +1,79 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * `bundle.functions` collection keeps each entry's DECLARATION (#4396). + * + * The declaration matters at one consumer — a `script` node reporting what its + * run did to the data — but the collector is the first place it can be lost, + * and losing it here is indistinguishable from never having supported the + * shape: the run then reports the writes as nothing. + */ + +import { describe, it, expect } from 'vitest'; +import { collectBundleFunctions, collectBundleFunctionEntries } from './app-plugin'; + +describe('collectBundleFunctionEntries (#4396)', () => { + it('reads a bare handler as the pure default', () => { + const scoreLead = () => ({ score: 1 }); + const entries = collectBundleFunctionEntries({ functions: { scoreLead } }); + expect(entries.scoreLead).toEqual({ handler: scoreLead, effect: 'pure' }); + }); + + it('keeps a declared writer’s effect off the map form', () => { + const syncBilling = () => ({ ok: true }); + const entries = collectBundleFunctionEntries({ + functions: { syncBilling: { handler: syncBilling, effect: 'writes' } }, + }); + expect(entries.syncBilling).toEqual({ handler: syncBilling, effect: 'writes' }); + }); + + it('keeps it off the array form too', () => { + const syncBilling = () => ({ ok: true }); + const entries = collectBundleFunctionEntries({ + functions: [{ name: 'syncBilling', handler: syncBilling, effect: 'writes' }], + }); + expect(entries.syncBilling).toEqual({ handler: syncBilling, effect: 'writes' }); + }); + + it('merges manifest.functions alongside bundle.functions', () => { + const a = () => 1; + const b = () => 2; + const entries = collectBundleFunctionEntries({ + functions: { a }, + manifest: { functions: { b: { handler: b, effect: 'writes' } } }, + }); + expect(Object.keys(entries).sort()).toEqual(['a', 'b']); + expect(entries.b.effect).toBe('writes'); + }); + + it('drops entries with no callable, and flags an unreadable effect as a write', () => { + const syncBilling = () => ({ ok: true }); + const entries = collectBundleFunctionEntries({ + functions: { + broken: { effect: 'writes' }, + typo: { handler: syncBilling, effect: 'write' }, + }, + }); + expect(entries.broken).toBeUndefined(); + expect(entries.typo).toEqual({ + handler: syncBilling, + effect: 'writes', + unrecognizedEffect: 'write', + }); + }); +}); + +describe('collectBundleFunctions', () => { + it('still answers with plain callables — hooks and jobs need nothing else', () => { + const scoreLead = () => ({ score: 1 }); + const syncBilling = () => ({ ok: true }); + const functions = collectBundleFunctions({ + functions: { scoreLead, syncBilling: { handler: syncBilling, effect: 'writes' } }, + }); + expect(functions.scoreLead).toBe(scoreLead); + // The declared form resolves to its handler rather than being dropped: + // before #4396 a `{ handler, effect }` entry was silently skipped here, + // which would have made a string-named hook unresolvable. + expect(functions.syncBilling).toBe(syncBilling); + }); +}); diff --git a/packages/runtime/src/index.ts b/packages/runtime/src/index.ts index 59b9336f5d..880d3e080d 100644 --- a/packages/runtime/src/index.ts +++ b/packages/runtime/src/index.ts @@ -19,7 +19,7 @@ export type { DefaultHostConfigOptions, DefaultHostConfigResult } from './defaul export { DriverPlugin } from './driver-plugin.js'; export { DefaultDatasourcePlugin } from './default-datasource-plugin.js'; export type { DefaultDatasourceDefinition, DefaultDatasourcePluginOptions } from './default-datasource-plugin.js'; -export { AppPlugin, collectBundleHooks, collectBundleFunctions, collectBundleActions } from './app-plugin.js'; +export { AppPlugin, collectBundleHooks, collectBundleFunctions, collectBundleFunctionEntries, collectBundleActions } from './app-plugin.js'; export { SeedLoaderService } from './seed-loader.js'; // Boot-summary seed outcome accumulator (#3415/#3430) — the single writer // contract shared by AppPlugin and the marketplace rehydrate/heal path. diff --git a/packages/runtime/src/load-artifact-bundle.ts b/packages/runtime/src/load-artifact-bundle.ts index c5ff425ef1..eb318b5ed4 100644 --- a/packages/runtime/src/load-artifact-bundle.ts +++ b/packages/runtime/src/load-artifact-bundle.ts @@ -145,7 +145,19 @@ export async function mergeRuntimeModule(bundle: any, artifactAbsPath: string, t const existing = (bundle.functions && typeof bundle.functions === 'object' && !Array.isArray(bundle.functions)) ? bundle.functions as Record : {}; - bundle.functions = { ...existing, ...fns }; + // The module supplies the CALLABLE; the JSON supplies what the function + // DECLARED about itself (`{ handler: '', effect: 'writes' }`, + // #4396). A plain overwrite would keep the first and drop the second, + // silently un-declaring every writer on the artifact path. + const merged: Record = { ...existing }; + for (const [name, fn] of Object.entries(fns as Record)) { + const declared = merged[name]; + merged[name] = + typeof fn === 'function' && declared && typeof declared === 'object' && !Array.isArray(declared) + ? { ...(declared as Record), handler: fn } + : fn; + } + bundle.functions = merged; } catch (err: any) { // eslint-disable-next-line no-console console.warn(`${tag} runtime module load FAILED: path='${moduleAbsPath}' error=${err?.message ?? err}`); diff --git a/packages/runtime/vitest.config.ts b/packages/runtime/vitest.config.ts index 5eb4520ac4..9ce1b9ab6a 100644 --- a/packages/runtime/vitest.config.ts +++ b/packages/runtime/vitest.config.ts @@ -15,6 +15,9 @@ export default defineConfig({ '@objectstack/rest': path.resolve(__dirname, '../rest/src/index.ts'), '@objectstack/spec/ai': path.resolve(__dirname, '../spec/src/ai/index.ts'), '@objectstack/spec/api': path.resolve(__dirname, '../spec/src/api/index.ts'), + // `AppPlugin` reads a bundle function's declared effect off this + // namespace (#4396). + '@objectstack/spec/automation': path.resolve(__dirname, '../spec/src/automation/index.ts'), '@objectstack/spec/contracts': path.resolve(__dirname, '../spec/src/contracts/index.ts'), '@objectstack/spec/data': path.resolve(__dirname, '../spec/src/data/index.ts'), // Reached via `@objectstack/platform-objects` (sys-user.object.ts), which diff --git a/packages/services/service-automation/src/builtin/screen-nodes.test.ts b/packages/services/service-automation/src/builtin/screen-nodes.test.ts index c856742282..6b2652f95c 100644 --- a/packages/services/service-automation/src/builtin/screen-nodes.test.ts +++ b/packages/services/service-automation/src/builtin/screen-nodes.test.ts @@ -304,3 +304,132 @@ describe('screen node — the field wire payload (#3528)', () => { expect(paused.screen!.fields[1].visibleWhen).toBe('tier == "gold" && "{recordId}" != ""'); }); }); + +/** + * #4396 — a `script` function is contractually pure, and the run summary counts + * on it: the node reports NO record metrics because every write a pure function + * causes is a downstream declarative node counting itself. A function that + * legitimately writes DECLARES it, and only then does its step report an effect + * the platform cannot count. + */ +describe('script node — the purity contract and its declared exception (#4396)', () => { + let engine: AutomationEngine; + + beforeEach(() => { + engine = new AutomationEngine(createTestLogger()); + registerScreenNodes(engine, createCtx()); + }); + + it("publishes the contract on the action descriptor, where an author can read it", () => { + const script = engine.getActionDescriptors().find((d) => d.type === 'script'); + expect(script?.handlerContract).toBe('pure'); + }); + + it('reports NO metrics for a pure function — absent is the accurate answer, not a guess', async () => { + engine.setFunctionResolver(() => () => ({ ai_category: 'billing' })); + engine.registerFlow('script_flow', scriptFlow({ function: 'triage', outputVariable: 'ai' })); + + const r = await engine.execute('script_flow', {} as any); + expect(r.success).toBe(true); + expect(r.summary?.unmeasured).toBe(0); + const node = r.summary?.nodes.find((n) => n.nodeId === 'run'); + expect(node).toMatchObject({ runs: 1, status: 'success' }); + expect(node?.selected).toBeUndefined(); + expect(node?.acted).toBeUndefined(); + }); + + it("counts a declared writer's step as an effect it cannot measure", async () => { + // The under-report this closes: without the declaration the run below + // says `acted: 0` — indistinguishable from a sweep that did nothing. + engine.setFunctionResolver((name) => + name === 'syncBilling' ? { handler: () => ({ ok: true }), effect: 'writes' } : undefined); + engine.registerFlow('script_flow', scriptFlow({ function: 'syncBilling' })); + + const r = await engine.execute('script_flow', {} as any); + expect(r.success).toBe(true); + expect(r.summary?.acted).toBe(0); + // ...but NOT "measured as zero": the third answer keeps the + // broken-sweep query (`acted = 0 AND unmeasured = 0`) off this run. + expect(r.summary?.unmeasured).toBe(1); + expect(r.summary?.nodes.find((n) => n.nodeId === 'run')?.unmeasured).toBe(1); + }); + + it('still reports the effect when a declared writer throws — it may have written first', async () => { + engine.setFunctionResolver(() => ({ + handler: () => { throw new Error('upstream 500'); }, + effect: 'writes', + })); + engine.registerFlow('script_flow', scriptFlow({ function: 'syncBilling' })); + + const r = await engine.execute('script_flow', {} as any); + expect(r.success).toBe(false); + expect(r.summary?.unmeasured).toBe(1); + }); + + it('leaves every OTHER flow measurable — declaring is per function, not per node type', async () => { + // The rejected fix was a blanket `unmeasuredEffect` on every script + // step, which would have blinded the detector on every flow that calls + // any function. A pure call in the same run must still count as + // measured. + engine.setFunctionResolver((name) => + name === 'syncBilling' + ? { handler: () => ({ ok: true }), effect: 'writes' } + : () => ({ score: 1 })); + engine.registerFlow('mixed', { + name: 'mixed', label: 'Mixed', type: 'autolaunched', + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { id: 'pure', type: 'script', label: 'Pure', config: { function: 'scoreLead' } }, + { id: 'writer', type: 'script', label: 'Writer', config: { function: 'syncBilling' } }, + { id: 'end', type: 'end', label: 'End' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'pure' }, + { id: 'e2', source: 'pure', target: 'writer' }, + { id: 'e3', source: 'writer', target: 'end' }, + ], + } as any); + + const r = await engine.execute('mixed', {} as any); + expect(r.success).toBe(true); + expect(r.summary?.unmeasured).toBe(1); + expect(r.summary?.nodes.find((n) => n.nodeId === 'pure')?.unmeasured).toBeUndefined(); + expect(r.summary?.nodes.find((n) => n.nodeId === 'writer')?.unmeasured).toBe(1); + }); + + it('reads a bare handler as the pure default — the short spelling of the contract', async () => { + const calls: unknown[] = []; + engine.setFunctionResolver(() => (c: any) => { calls.push(c.input); return 1; }); + engine.registerFlow('script_flow', scriptFlow({ function: 'scoreLead', inputs: { r: 'x' } })); + + const r = await engine.execute('script_flow', {} as any); + expect(r.success).toBe(true); + expect(calls).toEqual([{ r: 'x' }]); + expect(r.summary?.unmeasured).toBe(0); + }); +}); + +/** + * The one part of the purity contract the runtime CAN hold: a flow function is + * handed nothing to write with. Not enforcement — a function may close over a + * client at module scope — but the day someone adds `ctx.api` here, the + * contract stops being about author discipline and starts being a lie, so the + * context's shape is pinned rather than left incidental (#4396, issue option 3). + */ +describe('FlowFunctionContext carries no data reach (#4396)', () => { + it('hands the function input / variables / automation / logger and nothing else', async () => { + const engine = new AutomationEngine(createTestLogger()); + registerScreenNodes(engine, createCtx()); + + let seen: Record | undefined; + engine.setFunctionResolver(() => (c: any) => { seen = c; return 1; }); + engine.registerFlow('script_flow', scriptFlow({ function: 'inspect' })); + await engine.execute('script_flow', {} as any); + + expect(Object.keys(seen ?? {}).sort()).toEqual(['automation', 'input', 'logger', 'variables']); + // `automation` is the trigger/run context (record, identity, params) — + // provenance, not a handle on the data engine. + expect(seen!.automation).not.toHaveProperty('api'); + expect(seen!.automation).not.toHaveProperty('objectql'); + }); +}); diff --git a/packages/services/service-automation/src/builtin/screen-nodes.ts b/packages/services/service-automation/src/builtin/screen-nodes.ts index e6d572cd00..25b1003f26 100644 --- a/packages/services/service-automation/src/builtin/screen-nodes.ts +++ b/packages/services/service-automation/src/builtin/screen-nodes.ts @@ -27,6 +27,10 @@ import { parseNodeConfig } from './parse-config.js'; * which the host bridges to `bundle.functions` / `defineStack({ functions })`. * A target that resolves to neither fails the step LOUDLY rather than the old * silent "no-op handler" success, so an unwired callable can't quietly skip. + * The named function is contractually PURE — it returns a value and the flow + * graph persists it — which the descriptor publishes as + * `handlerContract: 'pure'` and a writing function opts out of by declaring + * `effect: 'writes'` where it is registered (#4396). */ /** @@ -204,6 +208,11 @@ export function registerScreenNodes(engine: AutomationEngine, ctx: PluginContext type: 'script', version: '1.0.0', name: 'Script', description: 'Run a custom script action.', icon: 'code', category: 'logic', source: 'builtin', + // #4396 — the purity rule this executor relies on, published where an + // author, a designer and the action catalog can read it. `category: + // 'logic'` says nothing about it, so until this field the contract + // existed only as a comment inside the call below. + handlerContract: 'pure', }), async execute(node, variables, context) { const cfg = (node.config ?? {}) as Record; @@ -259,8 +268,8 @@ export function registerScreenNodes(engine: AutomationEngine, ctx: PluginContext }; } - const handler = engine.resolveFunction(target); - if (!handler) { + const registration = engine.resolveFunction(target); + if (!registration) { return { success: false, error: @@ -276,27 +285,42 @@ export function registerScreenNodes(engine: AutomationEngine, ctx: PluginContext const input = interpolate(cfg.inputs ?? {}, variables, context) as Record; const outputVariable = typeof cfg.outputVariable === 'string' && cfg.outputVariable.trim() ? cfg.outputVariable.trim() : undefined; + // Pure-function pattern: the function RETURNS its result; `outputVariable` + // exposes it as a flow variable so a later declarative node persists it + // (e.g. `update_record fields: { ai_category: '{aiResult.ai_category}' }`). + // Data I/O stays on the flow graph — the function itself does no writes. + // The descriptor above publishes that as `handlerContract: 'pure'`, and + // `FlowFunctionContext` hands the function no data engine to write with. + // + // #4354 — which is why a pure function's step reports NO metrics: by that + // contract every write it causes is a downstream `update_record` counting + // itself, so "absent" (this kind of node touches no records) is the + // accurate answer, not a guess. + // + // #4396 — and a function that legitimately writes DECLARES it + // (`{ handler, effect: 'writes' }`), which is what turns this step's + // silence into an honest "cannot count". Declaring stays per FUNCTION: + // a blanket `unmeasuredEffect` on every script step would suppress the + // broken-sweep signal on every flow that calls any function, to cover + // the few that write. An UNDECLARED writer still under-reports — no + // runtime check can catch code that closed over a client at module + // scope, so the rule is published rather than policed. + const unmeasured = registration.effect === 'writes'; try { - const result = await handler({ input, variables, automation: context, logger: ctx.logger }); - // Pure-function pattern: the function RETURNS its result; `outputVariable` - // exposes it as a flow variable so a later declarative node persists it - // (e.g. `update_record fields: { ai_category: '{aiResult.ai_category}' }`). - // Data I/O stays on the flow graph — the function itself does no writes. - // - // #4354 — which is why this node reports NO metrics, deliberately: by - // that contract every write it causes is a downstream `update_record` - // counting itself, so "absent" (this kind of node touches no records) - // is the accurate answer, not a guess. Nothing ENFORCES the purity - // though, so a function that writes behind the platform's back makes - // its run under-report. Filed as #4396 rather than accommodated: a blanket - // `unmeasuredEffect` here would suppress the broken-sweep signal on - // every flow that calls any function, to cover a contract violation. + const result = await registration.handler({ input, variables, automation: context, logger: ctx.logger }); if (outputVariable) variables.set(outputVariable, result); - return { success: true, output: { function: target, result } }; + return { + success: true, + output: { function: target, result }, + ...(unmeasured ? { metrics: { unmeasuredEffect: true } } : {}), + }; } catch (err) { return { success: false, error: `script function '${target}' (node '${node.id}') failed: ${(err as Error).message}`, + // A declared writer that threw may have written before it did, so + // the run still cannot claim it counted everything. + ...(unmeasured ? { metrics: { unmeasuredEffect: true } } : {}), }; } }, diff --git a/packages/services/service-automation/src/engine.ts b/packages/services/service-automation/src/engine.ts index 92ce98838b..7f805c97d8 100644 --- a/packages/services/service-automation/src/engine.ts +++ b/packages/services/service-automation/src/engine.ts @@ -6,6 +6,7 @@ import type { ActionDescriptor, ExecutionStepMetrics, ExecutionStepSkipReason, + FlowFunctionEffect, FlowRunSummary, } from '@objectstack/spec/automation'; import type { AutomationContext, AutomationResult, ResumeSignal, IAutomationService, ScreenSpec } from '@objectstack/spec/contracts'; @@ -315,6 +316,14 @@ export interface RegisteredConnector { * (#1870). Mirrors {@link ConnectorActionContext} but carries the node's mapped * `input` so the function reads its arguments without reaching into the raw * variable map. The function's return value becomes the node output. + * + * Note what is NOT here: any handle on the data engine. A flow function is a + * pure compute step (`ActionDescriptor.handlerContract: 'pure'`, #4396) — it + * returns a value and a later declarative node persists it — so the context + * gives it nothing to write with. That is as far as the runtime can go: a + * function is ordinary host code and may close over a client at module scope, + * which is why one that legitimately writes DECLARES it + * (`FlowFunctionEffect`) instead of being detected. */ export interface FlowFunctionContext { /** Inputs mapped from the node's `config.inputs` (already in scope). */ @@ -334,14 +343,37 @@ export interface FlowFunctionContext { */ export type FlowFunctionHandler = (ctx: FlowFunctionContext) => unknown | Promise; +/** + * A resolved function: the callable plus what it DECLARED about itself at + * registration (#4396). + * + * The effect is what keeps the run summary honest for the one case the purity + * contract does not cover. A `script` step normally reports no record metrics — + * accurate, because a pure function's writes are downstream declarative nodes + * that count themselves — but a function declared `'writes'` gets its step + * marked `unmeasuredEffect`, so the run says "an effect I cannot count" + * rather than claiming it wrote nothing. + */ +export interface FlowFunctionRegistration { + readonly handler: FlowFunctionHandler; + /** Declared data effect. Absent ⇒ `'pure'`, the contract's default. */ + readonly effect?: FlowFunctionEffect; +} + /** * Resolves a function name to its handler. Injected by the host (the automation - * plugin bridges it to ObjectQL's `resolveFunction`, fed by `bundle.functions`), + * plugin bridges it to ObjectQL's function registry, fed by `bundle.functions`), * so the engine stays decoupled from any specific function registry. Returns * `undefined` for an unknown name, letting the `script` node fail the step * loudly instead of silently no-op'ing (#1870). + * + * A resolver may return the bare handler — which IS the declaration + * `effect: 'pure'`, written the short way — or a {@link FlowFunctionRegistration} + * carrying what the function declared. */ -export type FlowFunctionResolver = (name: string) => FlowFunctionHandler | undefined; +export type FlowFunctionResolver = ( + name: string, +) => FlowFunctionHandler | FlowFunctionRegistration | undefined; /** * Resolves the schema of the object a flow's conditions bind against — its field @@ -1396,12 +1428,18 @@ export class AutomationEngine implements IAutomationService { } /** - * Resolve a named function for a `script` node. Returns `undefined` when no - * resolver is wired or the name is unregistered — the node then fails the - * step with a clear error rather than silently no-op'ing. + * Resolve a named function for a `script` node, normalized to + * {@link FlowFunctionRegistration} so the caller reads the handler and its + * declared effect the same way whichever shape the host's resolver returned. + * Returns `undefined` when no resolver is wired or the name is unregistered + * — the node then fails the step with a clear error rather than silently + * no-op'ing. */ - resolveFunction(name: string): FlowFunctionHandler | undefined { - return this.functionResolver?.(name) ?? undefined; + resolveFunction(name: string): FlowFunctionRegistration | undefined { + const resolved = this.functionResolver?.(name); + if (!resolved) return undefined; + if (typeof resolved === 'function') return { handler: resolved }; + return typeof resolved.handler === 'function' ? resolved : undefined; } /** Get all registered connector names. */ diff --git a/packages/services/service-automation/src/plugin.ts b/packages/services/service-automation/src/plugin.ts index ecf987ae56..bfb8990794 100644 --- a/packages/services/service-automation/src/plugin.ts +++ b/packages/services/service-automation/src/plugin.ts @@ -3,6 +3,7 @@ import type { Plugin, PluginContext } from '@objectstack/core'; import { resolveUserAuthzGrants } from '@objectstack/core'; import type { IJobService } from '@objectstack/spec/contracts'; +import type { FlowFunctionEffect } from '@objectstack/spec/automation'; import type { Connector, ConnectorInstanceAuth, @@ -477,13 +478,24 @@ export class AutomationServicePlugin implements Plugin { try { const fnRegistry = ctx.getService<{ resolveFunction?: (name: string) => ((c: unknown) => unknown) | undefined; + // #4396 — the same lookup, with what the function declared about + // itself. Optional: an older/foreign `objectql` service exposes + // only the handler, and every such function is then read as the + // contract's default (pure), exactly as before this existed. + resolveFunctionEntry?: (name: string) => { + handler?: (c: unknown) => unknown; + effect?: FlowFunctionEffect; + } | undefined; }>('objectql'); if (fnRegistry && typeof fnRegistry.resolveFunction === 'function') { this.engine.setFunctionResolver((name) => { - const fn = fnRegistry.resolveFunction!(name); - return typeof fn === 'function' - ? (fnCtx) => (fn as (c: unknown) => unknown)(fnCtx) - : undefined; + const entry = fnRegistry.resolveFunctionEntry?.(name); + const fn = entry?.handler ?? fnRegistry.resolveFunction!(name); + if (typeof fn !== 'function') return undefined; + return { + handler: (fnCtx) => (fn as (c: unknown) => unknown)(fnCtx), + ...(entry?.effect ? { effect: entry.effect } : {}), + }; }); ctx.logger.debug('[Automation] script-node function registry bridged to objectql.resolveFunction'); } diff --git a/packages/spec/api-surface.json b/packages/spec/api-surface.json index dd28ce4b22..6b97500628 100644 --- a/packages/spec/api-surface.json +++ b/packages/spec/api-surface.json @@ -2087,6 +2087,7 @@ "CreateRecordConfig (type)", "CreateRecordConfigParsed (type)", "CreateRecordConfigSchema (const)", + "DEFAULT_FLOW_FUNCTION_EFFECT (const)", "DEPRECATED_APPROVER_TYPES (const)", "DataDestinationConfig (type)", "DataDestinationConfigSchema (const)", @@ -2149,6 +2150,14 @@ "FlowEdge (type)", "FlowEdgeParsed (type)", "FlowEdgeSchema (const)", + "FlowFunctionCallable (type)", + "FlowFunctionDeclaration (type)", + "FlowFunctionDeclarationInput (type)", + "FlowFunctionDeclarationSchema (const)", + "FlowFunctionEffect (type)", + "FlowFunctionEffectSchema (const)", + "FlowFunctionEntry (type)", + "FlowFunctionEntrySchema (const)", "FlowGraph (interface)", "FlowNode (type)", "FlowNodeAction (const)", @@ -2193,6 +2202,7 @@ "NON_AUTHORABLE_APPROVER_TYPES (const)", "NodeExecutorDescriptor (type)", "NodeExecutorDescriptorSchema (const)", + "NormalizedFlowFunction (interface)", "NotifyConfig (type)", "NotifyConfigParsed (type)", "NotifyConfigSchema (const)", @@ -2283,8 +2293,10 @@ "flowForm (const)", "getApprovalNodeConfigJsonSchema (function)", "importBpmnToConstructs (function)", + "isFlowFunctionEffect (function)", "normalizeControlFlowRegions (function)", "normalizeDecisionOutputs (function)", + "normalizeFlowFunctionEntry (function)", "resolveFlowNodeExpressions (function)", "validateControlFlow (function)" ], diff --git a/packages/spec/authorable-surface.json b/packages/spec/authorable-surface.json index 07e77a1d6d..db6b7a2f1f 100644 --- a/packages/spec/authorable-surface.json +++ b/packages/spec/authorable-surface.json @@ -2128,6 +2128,7 @@ "automation/ActionDescriptor:configSchema", "automation/ActionDescriptor:deprecated", "automation/ActionDescriptor:description", + "automation/ActionDescriptor:handlerContract", "automation/ActionDescriptor:icon", "automation/ActionDescriptor:isAsync", "automation/ActionDescriptor:maturity", diff --git a/packages/spec/json-schema.manifest.json b/packages/spec/json-schema.manifest.json index f0061d0498..fb2d896eaa 100644 --- a/packages/spec/json-schema.manifest.json +++ b/packages/spec/json-schema.manifest.json @@ -554,6 +554,7 @@ "automation/ExecutionStepSkipReason", "automation/Flow", "automation/FlowEdge", + "automation/FlowFunctionEffect", "automation/FlowNode", "automation/FlowNodeAction", "automation/FlowRegion", diff --git a/packages/spec/src/automation/flow-function.test.ts b/packages/spec/src/automation/flow-function.test.ts new file mode 100644 index 0000000000..613b9accdd --- /dev/null +++ b/packages/spec/src/automation/flow-function.test.ts @@ -0,0 +1,106 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect } from 'vitest'; +import { + DEFAULT_FLOW_FUNCTION_EFFECT, + FlowFunctionDeclarationSchema, + FlowFunctionEffectSchema, + FlowFunctionEntrySchema, + isFlowFunctionEffect, + normalizeFlowFunctionEntry, +} from './flow-function.zod'; +import { defineStack } from '../stack.zod'; + +describe('FlowFunctionEffectSchema (#4396)', () => { + it('declares exactly the two effects the runtime acts on', () => { + expect(FlowFunctionEffectSchema.options).toEqual(['pure', 'writes']); + }); + + it('rejects a near-miss spelling instead of reading it as the pure default', () => { + // 'write' singular would otherwise silently mean "wrote nothing" — the + // under-report this whole declaration exists to stop. + expect(FlowFunctionEffectSchema.safeParse('write').success).toBe(false); + expect(isFlowFunctionEffect('write')).toBe(false); + }); + + it('assumes purity when nothing is declared', () => { + expect(DEFAULT_FLOW_FUNCTION_EFFECT).toBe('pure'); + expect(FlowFunctionDeclarationSchema.parse({ handler: () => 1 }).effect).toBe('pure'); + }); +}); + +describe('normalizeFlowFunctionEntry', () => { + it('reads a bare handler as the pure declaration, written the short way', () => { + const fn = () => 1; + expect(normalizeFlowFunctionEntry(fn)).toEqual({ handler: fn, effect: 'pure' }); + }); + + it('carries a declared effect through', () => { + const fn = () => 1; + expect(normalizeFlowFunctionEntry({ handler: fn, effect: 'writes' })) + .toEqual({ handler: fn, effect: 'writes' }); + }); + + it('drops an entry with no callable — there is nothing to invoke', () => { + expect(normalizeFlowFunctionEntry(undefined)).toBeUndefined(); + expect(normalizeFlowFunctionEntry({ effect: 'writes' })).toBeUndefined(); + expect(normalizeFlowFunctionEntry('nope')).toBeUndefined(); + }); + + it('reads an UNRECOGNIZED effect as an uncountable write, and says which value it was', () => { + // Reachable only off the `defineStack` parse (a hand-built bundle). The safe + // direction is "cannot say this ran clean", never "it wrote nothing" — + // and the raw value is surfaced so the typo gets fixed. + const fn = () => 1; + expect(normalizeFlowFunctionEntry({ handler: fn, effect: 'write' })).toEqual({ + handler: fn, + effect: 'writes', + unrecognizedEffect: 'write', + }); + }); +}); + +describe('FlowFunctionEntrySchema', () => { + it('accepts both spellings of one entry', () => { + expect(FlowFunctionEntrySchema.safeParse(() => 1).success).toBe(true); + expect(FlowFunctionEntrySchema.safeParse({ handler: () => 1, effect: 'writes' }).success).toBe(true); + }); + + it('rejects a declaration whose handler is not callable', () => { + expect(FlowFunctionEntrySchema.safeParse({ handler: 'scoreLead' }).success).toBe(false); + }); +}); + +describe('defineStack({ functions }) — the authoring surface (#4396)', () => { + const base = { + manifest: { id: 'com.example.demo', name: 'demo', version: '1.0.0', type: 'app' as const }, + }; + + it('accepts a bare handler and a declared writer side by side', () => { + const stack = defineStack({ + ...base, + functions: { + scoreLead: () => ({ score: 1 }), + syncBilling: { handler: () => ({ ok: true }), effect: 'writes' }, + }, + }); + const functions = stack.functions as Record; + expect(typeof functions.scoreLead).toBe('function'); + expect(normalizeFlowFunctionEntry(functions.syncBilling)?.effect).toBe('writes'); + }); + + it('accepts `effect` on the array form too', () => { + const stack = defineStack({ + ...base, + functions: [{ name: 'syncBilling', handler: () => ({ ok: true }), effect: 'writes' }], + }); + expect((stack.functions as Array<{ effect?: string }>)[0].effect).toBe('writes'); + }); + + it('rejects an effect the runtime has no meaning for, at authoring time', () => { + expect(() => defineStack({ + ...base, + functions: { syncBilling: { handler: () => ({ ok: true }), effect: 'sometimes' } }, + } as never)).toThrow(); + }); +}); diff --git a/packages/spec/src/automation/flow-function.zod.ts b/packages/spec/src/automation/flow-function.zod.ts new file mode 100644 index 0000000000..a5ce3ab843 --- /dev/null +++ b/packages/spec/src/automation/flow-function.zod.ts @@ -0,0 +1,168 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * @module automation/flow-function + * + * The contract for a **named handler function a `script` node invokes** — + * contributed by `defineStack({ functions })` and resolved by name at execute + * time (#1870). + * + * ## The rule, and why it lives here instead of in a comment + * + * A flow function is a PURE compute step: it receives its mapped `input`, + * RETURNS a value, and the node's `outputVariable` exposes that value as a flow + * variable so a later DECLARATIVE node persists it (`update_record fields: { + * ai_category: '{aiResult.ai_category}' }`). Data I/O stays on the flow graph. + * + * That is not style advice. #4354's per-run summary reports what a run did to + * the data, and the `script` node reports NO record metrics *because* of this + * rule: every write a script causes is a downstream `create_record` / + * `update_record` that counts itself, so "this node touched no records" is the + * accurate answer rather than a guess. A function that writes anyway makes its + * run under-report — `selected: 30, acted: 0` on a run that wrote 30 invoices, + * which reads exactly like the broken sweep #4354 exists to detect, and the + * durable `sys_automation_run` row says so permanently. + * + * Until #4396 that rule lived ONLY in a comment inside the executor, so neither + * an author, a lint, nor the runtime could see the contract the summary was + * relying on. It is now declared in two halves: + * + * 1. `ActionDescriptor.handlerContract` — `script` publishes `'pure'`, so the + * action catalog and the designer palette carry the rule an author reads. + * 2. {@link FlowFunctionEffectSchema} — a function that legitimately writes + * DECLARES it, and its step then reports `unmeasuredEffect`, so the run + * says "cannot count" instead of claiming it wrote nothing. + * + * ## What is deliberately not here + * + * A blanket `unmeasuredEffect` on every `script` step (the escape hatch #4354 + * gave `connector_action`) was rejected: it would suppress the broken-sweep + * signal on every flow that calls any function, in order to accommodate the + * flows that break the rule — paying for a rule-breaker with everyone else's + * signal, and fossilizing the violation as supported behaviour. + * + * Nor is this *enforcement*. The runtime hands a function no data reach — + * {@link FlowFunctionContext} in `@objectstack/service-automation` carries + * `input` / `variables` / `automation` / `logger` and no engine handle — but a + * function is ordinary host code and can close over a client at module scope. + * What the declaration buys is that the honest case is now *expressible*, and + * the platform's own counters stop being wrong for it. + */ + +import { z } from 'zod'; +import { lazySchema } from '../shared/lazy-schema'; + +/** + * What a `script`-node function does to data (#4396). + * + * - `'pure'` (the default, and the contract) — it computes and returns. The + * step reports no record metrics, which is exact: the writes it leads to are + * downstream declarative nodes that count themselves. + * - `'writes'` — it performs writes, or dispatches effects, that the platform + * cannot see or count. The step reports + * `ExecutionStepMetrics.unmeasuredEffect`, so the run's `unmeasured` tally + * keeps the broken-sweep query + * (`selected > 0 AND acted = 0 AND unmeasured = 0`) off it — and only off + * the flows that actually call such a function. + * + * There is deliberately no `'reads'` member: an under-reported `selected` can + * only make the broken-sweep alert quieter, never wrong, so declaring it would + * buy nothing the runtime acts on. + */ +export const FlowFunctionEffectSchema = lazySchema(() => z.enum([ + 'pure', + 'writes', +]).describe("What a script-node function does to data: 'pure' (computes and returns — the contract) or 'writes' (performs uncountable writes/effects, reported as unmeasured)")); + +export type FlowFunctionEffect = z.infer; + +/** + * The effect a function is assumed to have when it declares none — the + * contract's default, and the reason a bare `functions: { fn }` entry stays + * exactly as cheap to write as it was. + */ +export const DEFAULT_FLOW_FUNCTION_EFFECT: FlowFunctionEffect = 'pure'; + +/** + * The **declared** form of a `defineStack({ functions })` entry: the handler + * plus what it does. + * + * ```ts + * defineStack({ + * functions: { + * scoreLead: (ctx) => ({ score: 42 }), // pure (default) + * syncBilling: { handler: syncBilling, effect: 'writes' }, // declared writer + * }, + * }); + * ``` + * + * Both spellings are first-class; the bare-function form IS the declaration + * `effect: 'pure'`, written the short way. Declaring `'writes'` does not grant + * a function anything — it changes what the platform *reports* about the runs + * that call it, which is the whole point: an undeclared writer is counted as + * having written nothing. + */ +export const FlowFunctionDeclarationSchema = lazySchema(() => z.object({ + handler: z.function().describe('The function invoked by name (a `script` node, a string-named Hook/Action handler)'), + effect: FlowFunctionEffectSchema.default(DEFAULT_FLOW_FUNCTION_EFFECT) + .describe("What the function does to data — omit for the pure default"), +}).describe('A named handler function plus its declared effect (#4396)')); + +export type FlowFunctionDeclaration = z.infer; +export type FlowFunctionDeclarationInput = z.input; + +/** + * One entry of the `functions` map as authors may write it: the handler alone + * (pure), or a {@link FlowFunctionDeclarationSchema} that states its effect. + */ +export const FlowFunctionEntrySchema = lazySchema(() => z.union([ + z.function(), + FlowFunctionDeclarationSchema, +]).describe('A named handler function, or a declaration record stating its effect')); + +export type FlowFunctionEntry = z.infer; + +/** Any callable registerable under a name (a hook body, a flow function). */ +export type FlowFunctionCallable = (...args: never[]) => unknown; + +/** A `functions` entry read back in one shape, whichever way it was written. */ +export interface NormalizedFlowFunction { + readonly handler: FlowFunctionCallable; + readonly effect: FlowFunctionEffect; + /** + * Set to the raw value when `effect` was present but not a member of + * {@link FlowFunctionEffectSchema} — a typo (`'write'`) on a path that + * skipped `defineStack`'s parse. `effect` then reads `'writes'`, because a + * declaration the platform cannot read means "cannot say this ran clean", + * never "it wrote nothing"; the caller surfaces the raw value so the typo is + * fixable rather than permanently costing the run its measurement. + */ + readonly unrecognizedEffect?: unknown; +} + +/** True for a member of {@link FlowFunctionEffectSchema}. */ +export function isFlowFunctionEffect(value: unknown): value is FlowFunctionEffect { + return value === 'pure' || value === 'writes'; +} + +/** + * Read a `functions` map entry — the bare handler or the declaration record — + * into one normalized shape. Returns `undefined` when the entry carries no + * callable at all (the value every collector already drops). + * + * Deliberately hand-written rather than a `FlowFunctionEntrySchema.parse()`: + * the entry holds a live function, and the collectors that call this run on the + * boot path where re-parsing every handler buys nothing. + */ +export function normalizeFlowFunctionEntry(entry: unknown): NormalizedFlowFunction | undefined { + if (typeof entry === 'function') { + return { handler: entry as FlowFunctionCallable, effect: DEFAULT_FLOW_FUNCTION_EFFECT }; + } + if (!entry || typeof entry !== 'object') return undefined; + const record = entry as { handler?: unknown; effect?: unknown }; + if (typeof record.handler !== 'function') return undefined; + const handler = record.handler as FlowFunctionCallable; + if (record.effect === undefined) return { handler, effect: DEFAULT_FLOW_FUNCTION_EFFECT }; + if (isFlowFunctionEffect(record.effect)) return { handler, effect: record.effect }; + return { handler, effect: 'writes', unrecognizedEffect: record.effect }; +} diff --git a/packages/spec/src/automation/index.ts b/packages/spec/src/automation/index.ts index df4f6a8f3a..7abfa49c33 100644 --- a/packages/spec/src/automation/index.ts +++ b/packages/spec/src/automation/index.ts @@ -7,6 +7,7 @@ export * from './control-flow.zod'; export * from './io-node-config.zod'; export * from './builtin-node-config.zod'; export * from './schemaless-node-config.zod'; +export * from './flow-function.zod'; export { flowForm } from './flow.form'; export * from './execution.zod'; export * from './webhook.zod'; diff --git a/packages/spec/src/automation/node-executor.test.ts b/packages/spec/src/automation/node-executor.test.ts index 4c32187503..2c4694b6c4 100644 --- a/packages/spec/src/automation/node-executor.test.ts +++ b/packages/spec/src/automation/node-executor.test.ts @@ -274,3 +274,23 @@ describe('ActionDescriptorSchema.resumeAuthority', () => { expect(() => ActionDescriptorSchema.parse({ ...base, resumeAuthority: 'admin' })).toThrow(); }); }); + +// --------------------------------------------------------------------------- +// ActionDescriptorSchema — handlerContract (#4396) +// --------------------------------------------------------------------------- +describe('ActionDescriptorSchema.handlerContract', () => { + const base = { type: 'demo', version: '1.0.0', name: 'Demo' }; + + it("defaults to 'none' — most actions invoke no author-supplied code", () => { + expect(defineActionDescriptor(base).handlerContract).toBe('none'); + }); + + it("accepts 'pure' for an action whose author code must not write (script)", () => { + const desc = defineActionDescriptor({ ...base, type: 'script', handlerContract: 'pure' }); + expect(desc.handlerContract).toBe('pure'); + }); + + it('rejects an unknown contract rather than publishing a rule nothing defines', () => { + expect(() => ActionDescriptorSchema.parse({ ...base, handlerContract: 'sandboxed' })).toThrow(); + }); +}); diff --git a/packages/spec/src/automation/node-executor.zod.ts b/packages/spec/src/automation/node-executor.zod.ts index 49273e47c7..5d495a9541 100644 --- a/packages/spec/src/automation/node-executor.zod.ts +++ b/packages/spec/src/automation/node-executor.zod.ts @@ -291,6 +291,34 @@ export const ActionDescriptorSchema = lazySchema(() => z.object({ isAsync: z.boolean().default(false) .describe('Suspends the flow awaiting an external reply'), + /** + * The effect contract this action places on the AUTHOR-SUPPLIED code it + * invokes (#4396). + * + * - `'none'` (default) — the action invokes no author code, so there is no + * such contract to state. Nearly every action. + * - `'pure'` — it does, and that code must not perform data I/O: it takes + * its inputs, RETURNS a value, and a later DECLARATIVE node persists the + * result. `script` is the built-in that declares this, and the rule is + * load-bearing rather than stylistic — the node reports no record metrics + * precisely because every write it causes is a downstream `create_record` + * / `update_record` counting itself (#4354). A function that writes anyway + * makes its run under-report. + * + * It is declared HERE because the alternative — the state this field was + * added to leave — was a rule visible only inside the executor's own source: + * `category: 'logic'` said nothing about it, so no author, lint or designer + * could read the contract the run summary was relying on. A function that + * legitimately writes says so on its own registration + * (`FlowFunctionEffectSchema`, `defineStack({ functions })`) and its step is + * then counted as `unmeasuredEffect` rather than as nothing. + * + * Declaration, not enforcement: author code can close over a data client at + * module scope, and no descriptor field stops that. + */ + handlerContract: z.enum(['none', 'pure']).default('none') + .describe("Effect contract for author-supplied code this action invokes: 'none' (invokes none) or 'pure' (must not write — it returns a value and the flow graph persists it)"), + /** * WHO may resume a run this node suspended (#3801). The generic resume * route (`POST /automation/:name/runs/:runId/resume`) validates machine diff --git a/packages/spec/src/automation/schemaless-node-config.zod.ts b/packages/spec/src/automation/schemaless-node-config.zod.ts index df18fa189d..084d7d4e0e 100644 --- a/packages/spec/src/automation/schemaless-node-config.zod.ts +++ b/packages/spec/src/automation/schemaless-node-config.zod.ts @@ -97,14 +97,27 @@ export const SCRIPT_INVOKE_FUNCTION_ACTION_TYPE = 'invoke_function'; * 4. Anything left in `actionType` (except the * {@link SCRIPT_INVOKE_FUNCTION_ACTION_TYPE} marker) is shorthand for a * function name; a name that resolves to nothing fails the step LOUDLY. + * + * The invoked function is contractually PURE — it returns its result and the + * flow graph persists it (`FlowFunctionEffectSchema`, #4396). The descriptor + * publishes that as `handlerContract: 'pure'`, and it is what lets the node + * report no record metrics without guessing. */ export const ScriptConfigSchema = lazySchema(() => z.object({ /** Built-in side-effect id, the `invoke_function` marker, or (shorthand) a registered-function name. */ actionType: z.string().optional() .describe("How this step runs: a built-in side effect ('email' | 'slack'), the 'invoke_function' marker, or shorthand for a registered-function name"), - /** Registered function to call (`defineStack({ functions })`) — always wins over `actionType`. */ + /** + * Registered function to call (`defineStack({ functions })`) — always wins + * over `actionType`. + * + * Contractually pure: it takes `inputs`, RETURNS a value, and does no data + * I/O of its own. A function that legitimately writes declares + * `effect: 'writes'` where it is registered, so the run reports an effect it + * cannot count instead of reporting none (#4396). + */ function: z.string().optional() - .describe('Registered function to call (defineStack({ functions })); takes precedence over actionType'), + .describe('Registered function to call (defineStack({ functions })); takes precedence over actionType. Contractually pure — it returns a value a later declarative node persists'), /** Inputs passed to the function; values interpolate `{token}` templates against the live flow variables. */ inputs: z.record(z.string(), z.unknown()).optional() .describe('Inputs passed to the function (values interpolate {token} templates)'), diff --git a/packages/spec/src/contracts/objectql-engine.ts b/packages/spec/src/contracts/objectql-engine.ts index 9afb16d403..b5470e8655 100644 --- a/packages/spec/src/contracts/objectql-engine.ts +++ b/packages/spec/src/contracts/objectql-engine.ts @@ -47,6 +47,7 @@ import type { IDataEngine } from './data-engine'; import type { IDataDriver } from './data-driver'; +import type { FlowFunctionEffect, FlowFunctionEntry } from '../automation/flow-function.zod'; /** * The engine's schema-registry view — the eight members reached through the @@ -111,7 +112,16 @@ export interface IObjectQLEngine extends IDataEngine { options?: { object?: string | string[]; priority?: number; packageId?: string }, ): void; unregisterHooksByPackage(packageId: string): number; - registerFunction(name: string, handler: (context: any) => Promise | void, packageId?: string): void; + /** + * The third parameter is the owning `packageId`, or a record that also + * carries what the function DECLARES about itself (#4396) — today its data + * `effect`, which a `script` node reads back to report its run honestly. + */ + registerFunction( + name: string, + handler: (context: any) => Promise | void, + packageIdOrOptions?: string | { packageId?: string; effect?: FlowFunctionEffect }, + ): void; registerMiddleware( fn: (opCtx: any, next: () => Promise) => Promise, options?: { object?: string }, @@ -121,7 +131,8 @@ export interface IObjectQLEngine extends IDataEngine { hooks: unknown[] | undefined, opts?: { packageId?: string; - functions?: Record Promise | void>; + /** Handlers, or declaration records stating each function's effect (#4396). */ + functions?: Record; bodyRunner?: unknown; strict?: boolean; warnLegacyHandler?: boolean; diff --git a/packages/spec/src/stack.zod.ts b/packages/spec/src/stack.zod.ts index c9cb34a99e..9f9270631a 100644 --- a/packages/spec/src/stack.zod.ts +++ b/packages/spec/src/stack.zod.ts @@ -30,6 +30,7 @@ import { ThemeSchema } from './ui/theme.zod'; // Automation Protocol import { FlowSchema } from './automation/flow.zod'; +import { FlowFunctionEntrySchema, FlowFunctionEffectSchema } from './automation/flow-function.zod'; import { JobSchema } from './system/job.zod'; // Security Protocol @@ -333,23 +334,36 @@ export const ObjectStackDefinitionSchema = lazySchema(() => z.object({ /** * Named handler functions for declarative metadata that references * handlers by string name (`Hook.handler: 'my_handler'`, - * `Action.target: 'my_handler'`). Two accepted shapes: + * `Action.target: 'my_handler'`, a `script` node's `config.function`). + * Two accepted shapes: * * - Map form (preferred): `{ my_handler: (ctx) => {...} }` * - Array form: `[{ name: 'my_handler', handler: (ctx) => {...} }]` * - * Functions live in code only; they are not serialized into project - * artifacts. The `AppPlugin` registers them on the engine before - * binding hooks so `string` handlers resolve at startup. + * Either shape may state what the function DOES instead of just naming it + * (#4396) — `{ my_handler: { handler, effect: 'writes' } }`, or `effect` on + * an array entry. A `script` node's function is contractually pure (it + * returns a value; the flow graph persists it), and the run summary counts on + * that: an undeclared function is counted as having written nothing. A + * function that legitimately writes declares `effect: 'writes'` and its step + * is reported as an effect the platform cannot count, rather than as none. + * See `FlowFunctionEffectSchema` in `@objectstack/spec/automation`. + * + * The CALLABLE lives in code only — `objectstack build` lowers it to a + * handler ref and carries the function itself in the sibling runtime module + * (what it declared rides along in the artifact and is re-attached on load). + * The `AppPlugin` registers them on the engine before binding hooks so + * `string` handlers resolve at startup. */ functions: z.union([ - z.record(z.string(), z.function()), + z.record(z.string(), FlowFunctionEntrySchema), z.array(z.object({ name: z.string(), handler: z.function(), packageId: z.string().optional(), + effect: FlowFunctionEffectSchema.optional(), })), - ]).optional().describe('Named handler functions referenced by hooks/actions'), + ]).optional().describe('Named handler functions referenced by hooks/actions/script nodes (optionally declaring their effect)'), mappings: z.array(MappingSchema).optional().describe('Data Import/Export Mappings'), analyticsCubes: z.array(CubeSchema).optional().describe('Analytics Semantic Layer Cubes'), diff --git a/skills/objectstack-automation/SKILL.md b/skills/objectstack-automation/SKILL.md index 3660efd888..868fbbd8b2 100644 --- a/skills/objectstack-automation/SKILL.md +++ b/skills/objectstack-automation/SKILL.md @@ -885,6 +885,21 @@ them right the first time: If you genuinely need data-lifecycle **side effects** (read/write other records), that's an L2 **hook** (objectstack-data) — hooks get `ctx.api`; flow functions don't. + The rule is load-bearing: a `script` step reports **no** record metrics in the + run summary *because* every write a pure function causes is a downstream node + that counts itself. A function that writes anyway makes its run report + `acted: 0` — indistinguishable from a sweep that silently did nothing. When a + function must write somewhere the platform cannot see (an upstream billing + API), **declare it** so the run stays honest — its step is then counted as an + effect that cannot be measured, never as zero: + + ```ts + defineStack({ functions: { + 'helpdesk.aiTriageStub': (ctx) => ({ ai_category: 'other' }), // pure — the default + 'billing.sync': { handler: syncBilling, effect: 'writes' }, // declared writer + } }); + ``` + 10. **Conditions are bare CEL — only the stdlib is callable.** `now()`, `today()`, `daysFromNow(n)`, `daysAgo(n)`, `daysBetween(a, b)`, `isBlank(v)`, `coalesce(a, b)`, `abs/round/min/max`, `upper/lower/contains/matches`, plus CEL