Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions .changeset/loud-pausing-resume-authority.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
---
'@objectstack/spec': patch
---

automation: `ActionDescriptor.resumeAuthority` no longer defaults to `'any'` — an
omission is now a distinct, reportable fact (#5561, from #3823)

The #3801 resume gate keys on the suspended node, so it covers a pausing node type
exactly when that type's author declared who may resume it. The schema default made
that impossible to check: Zod filled the key inside `defineActionDescriptor`, so
"the author chose `'any'`" and "the author never considered it" produced
byte-identical descriptors. #3823 is what the erasure cost — ADR-0044 pointed an
approval's revise edge at a generic `wait`, `wait` is legitimately `'any'`, and the
pause standing in a service-owned position inherited a fail-open value nobody chose.

The field is now optional with no default, and absent means absent. Two seams read
it: `AutomationEngine.registerNodeExecutor` warns once per node type when a
`supportsPause` descriptor omits it, and the new `check:resume-authority-declared`
gate fails CI on an omission in this repo's own executors.

**Not a behaviour change.** The engine already resolved the value with `?? 'any'`,
so an undeclared pausing type is still raw-resumable exactly as before — loudly now
instead of silently. Nothing needs migrating: an executor that declared
`resumeAuthority` keeps its value, and one that omitted it keeps today's semantics
and gains a warning telling it to state its intent. Making omission mean
*fail-closed* is a breaking change still tracked on #5561 for a version window that
allows it; it is now a one-expression change rather than a schema migration.
20 changes: 20 additions & 0 deletions .changeset/warn-undeclared-resume-authority.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
---
'@objectstack/service-automation': patch
---

automation: a pausing node type that never declares `resumeAuthority` is now named
at registration, and the four pausing built-ins declare theirs (#5561)

`registerNodeExecutor` warns once per node type (per engine instance) when a
descriptor declares `supportsPause: true` and omits `resumeAuthority` — the state in
which the #3801 resume gate silently treats every pause that type creates as
raw-resumable through the generic resume route. The line names the two legal values
and says that declaring `'any'` explicitly silences it and changes no behaviour, so
a node whose pause really is open to the route is not pushed toward `'service'` to
quieten a log.

`screen`, `wait`, `subflow` and `map` now declare `resumeAuthority: 'any'`
explicitly. Each was already correct on its own terms — it was inheriting the value
rather than stating it — so the warning names nothing on a stock boot today and only
catches future omissions. Authority resolution is unchanged: `resolveResumeAuthority`
still resolves an absent value to `'any'`.
18 changes: 18 additions & 0 deletions .github/workflows/lint.yml
Original file line number Diff line number Diff line change
Expand Up @@ -381,6 +381,24 @@ jobs:
- name: Engine test-double contract gate
run: pnpm check:engine-double-contract

# Resume-authority declaration gate (#5561, from #3823). The #3801 resume
# gate keys on the SUSPENDED NODE, so it covers a pausing node type exactly
# when that type's author remembered to declare `resumeAuthority`. #3823 is
# what forgetting costs: ADR-0044 pointed a revise edge at a generic `wait`,
# `wait` is legitimately 'any', and a pause standing in a service-owned
# position inherited that value with nobody choosing it — an unaudited
# resubmit plus a destroyed remote run. Until #5561 removed the schema
# default the omission was not even observable (Zod filled the key, so it
# parsed byte-identically to an explicit 'any'); now a pausing descriptor
# that never states its authority cannot merge. Shipped sources only —
# fixtures deliberately construct the omission to test the engine's
# registration warning. Static AST, no build needed, so it belongs in this
# job. Runs its own --self-test first: the detector can be broken while
# every descriptor is fine, and a scan that stops matching would report OK
# while reading nothing (#4868's family).
- name: Resume-authority declaration gate
run: pnpm check:resume-authority-declared

typecheck:
name: TypeScript Type Check
runs-on: ubuntu-latest
Expand Down
2 changes: 1 addition & 1 deletion content/docs/references/automation/node-executor.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ Canonical cross-paradigm action/node descriptor (ADR-0018)
| **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) |
| **resumeAuthority** | `Enum<'any' \| 'service'>` | optional | Who may resume a run this node suspended: 'any' (the generic resume route) or 'service' (only the owning service, e.g. approvals). Deliberately has no default — an omission is a distinct, reportable fact, and a pausing node type that omits it is warned about at registration (#5561) |
| **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 |
| **deprecated** | `boolean` | ✅ | Deprecated alias kept for back-compat |
Expand Down
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@
"check:type-check-coverage": "node scripts/check-type-check-coverage.mjs --self-test && node scripts/check-type-check-coverage.mjs",
"check:driver-conformance": "node scripts/check-driver-conformance.mjs --self-test && node scripts/check-driver-conformance.mjs",
"check:engine-double-contract": "node scripts/check-engine-double-contract.mjs --self-test && node scripts/check-engine-double-contract.mjs",
"check:resume-authority-declared": "node scripts/check-resume-authority-declared.mjs --self-test && node scripts/check-resume-authority-declared.mjs",
"check:stall-guard": "node scripts/run-with-stall-guard.mjs --self-test"
},
"keywords": [
Expand Down
5 changes: 5 additions & 0 deletions packages/services/service-automation/src/builtin/map-node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,11 @@ export function registerMapNode(engine: AutomationEngine, ctx: PluginContext): v
// Each item's subflow may pause, so the map suspends and resumes per item.
supportsPause: true,
isAsync: true,
// As with `subflow`, `'any'` here is not the authority that applies: the
// #3801 gate follows the `map:` correlation to the in-flight item's child
// run and judges that node instead — judging the loop rather than the item
// is precisely the hole #3853 closed. Stated rather than inherited (#5561).
resumeAuthority: 'any',
// Structured config form for the flow designer (ADR-0018). Mirrors the
// objectui hardcoded `map` field group field-for-field, so the online
// (schema-driven) form matches the offline one (objectui #2670 Phase 3 /
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,11 @@ export function registerScreenNodes(engine: AutomationEngine, ctx: PluginContext
icon: 'window', category: 'human', source: 'builtin',
// Human-input nodes suspend the flow awaiting input.
supportsPause: true, isAsync: true,
// The generic resume route IS this node's intended door: the flow-runner
// collects the inputs and hands them back as the continuation, so there
// is no service decision to route around (#3801). Stated rather than
// inherited from a default — an omission is now a reported fact (#5561).
resumeAuthority: 'any',
// Designer form (ADR-0018, #3304) — mirrors objectui's hardcoded `screen`
// field group: flat input list OR an object form, plus title/description.
// `visibleWhen` is bare CEL (xExpression), `defaults` a free-form keyValue
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,12 @@ export function registerSubflowNode(engine: AutomationEngine, ctx: PluginContext
// A child that suspends (approval/screen/wait) suspends this node too —
// the parent run pauses here and resumes when the child completes.
supportsPause: true,
// `'any'` on this node is not the authority that ends up applying: the
// #3801 gate follows the `subflow:` correlation down to the CHILD and
// judges the node the signal actually lands on (#3853), so a parent parked
// above a pending approval is still refused. Stated rather than inherited
// so the omission-warning's silence here is a decision, not a gap (#5561).
resumeAuthority: 'any',
}),
async execute(node, variables, context) {
// #4343 — the contract is parsed before anything runs, the same seam the
Expand Down
6 changes: 6 additions & 0 deletions packages/services/service-automation/src/builtin/wait-node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,12 @@ export function registerWaitNode(engine: AutomationEngine, ctx: PluginContext):
// Durable pause — the run suspends and resumes later (timer/signal).
supportsPause: true,
isAsync: true,
// An external producer is *meant* to resume a signal wait, so the generic
// route is the door (#3801). Stated rather than inherited from a default:
// #3823 is what inheriting it costs — ADR-0044 pointed a revise edge at a
// generic `wait`, and the pause in that service-owned position took this
// value without anyone choosing it (#5561).
resumeAuthority: 'any',
}),
async execute(node, variables, _context) {
// `waitEventConfig` is the whole contract (`FlowNodeSchema`, flow.zod.ts).
Expand Down
68 changes: 67 additions & 1 deletion packages/services/service-automation/src/engine.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1138,6 +1138,15 @@ export class AutomationEngine implements IAutomationService {
* and a module-level flag would report only whichever engine ran first.
*/
private nodeTypeSealOmissionWarned = false;
/**
* Node types already named by {@link warnIfResumeAuthorityUndeclared}
* (#5561). Per **instance** and per **type**, for the same reason
* {@link nodeTypeSealOmissionWarned} is per instance: a hot-reload or a
* multi-tenant host re-registers the same executor repeatedly, and one
* omission must read as one finding rather than as a log that grows with
* uptime.
*/
private readonly resumeAuthorityOmissionWarned = new Set<string>();
private triggers = new Map<string, FlowTrigger>();
/**
* Flows currently wired to a trigger, keyed by flow name → the trigger
Expand Down Expand Up @@ -1334,11 +1343,60 @@ export class AutomationEngine implements IAutomationService {
);
}
this.actionDescriptors.set(descriptorType, executor.descriptor);
this.warnIfResumeAuthorityUndeclared(executor.descriptor);
}

this.logger.info(`Node executor registered: ${executor.type}`);
}

/**
* Name a pausing node type that never declared WHO may resume the pauses it
* creates (#5561, the tracking item ADR-0044's amendment deferred).
*
* `resumeAuthority` carries no schema default precisely so this warning can
* exist: with `.default('any')` an omission parsed into a descriptor
* byte-identical to an author's explicit `'any'`, so the fact was gone
* before the engine ever saw the object. Absent now means absent, and a
* pausing type that leaves it absent is fail-open by omission rather than
* by decision — #3823 is what that costs (a revise pause standing in a
* service-owned position inherited `wait`'s legitimate `'any'`, and a raw
* resume walked past an unrecorded decision).
*
* **What it asserts, and why that is safe here.** Only the static fact that
* THIS descriptor omits the key — a property of the object being registered,
* fixed at authoring time, which no later registration can contradict. It
* reads no registry and draws no conclusion from anything being absent from
* one, so it is not the shape AGENTS.md "Startup registry reads" forbids and
* needs no seal flag (contrast {@link warnIfNodeTypeVocabularyNeverSealed},
* which reports a missing CALL for the same reason). Whether the omission
* *matters* at run time is deliberately not judged: the engine still
* resolves absent to `'any'` ({@link resolveResumeAuthority}), so nothing
* about today's behaviour changes.
*
* **Blind spot, stated up front:** the trigger is `supportsPause`, itself a
* declaration no execution path enforces (#5703) — a run pauses because
* `execute()` returned `suspend: true`. An executor that suspends while
* leaving `supportsPause` false is therefore fail-open AND silent here.
* `check:resume-authority-declared` catches this repo's own executors at
* authoring time; #5703 tracks the runtime half.
*/
private warnIfResumeAuthorityUndeclared(descriptor: ActionDescriptor): void {
if (descriptor.supportsPause !== true) return;
if (descriptor.resumeAuthority !== undefined) return;
if (this.resumeAuthorityOmissionWarned.has(descriptor.type)) return;
this.resumeAuthorityOmissionWarned.add(descriptor.type);
this.logger.warn(
`[automation] node type '${descriptor.type}' declares supportsPause but never declares ` +
`resumeAuthority, so the #3801 resume gate treats every pause it creates as raw-resumable ` +
`through the generic route (POST /automation/:name/runs/:runId/resume) — fail-open by omission ` +
`rather than by decision, which is how #3823 walked past an unrecorded approval decision. ` +
`Declare it on the descriptor: 'any' if that route IS the intended door (a screen's collected ` +
`inputs, a signal wait's external producer), or 'service' if resuming is the tail of a decision ` +
`some service must authorize and record first. Declaring 'any' explicitly silences this and ` +
`changes no behaviour. Reported once per node type per engine.`,
);
}

/**
* Register a **deprecated alias** of a canonical node type (ADR-0018 M3).
*
Expand Down Expand Up @@ -2704,8 +2762,16 @@ export class AutomationEngine implements IAutomationService {
* snapshotting at alias-registration time) also keeps it correct whichever
* order the two register in. No alias of a pausing type exists today; this
* keeps it from becoming a hole the day one does.
*
* The `?? 'any'` is load-bearing in a second way since #5561: with no schema
* default on `resumeAuthority`, an undeclared descriptor arrives with the key
* absent and this is the one place that resolves it. It resolves fail-OPEN,
* exactly as the removed default did — step one of #5561 changed nothing
* here, it only made the omission audible at registration. Flipping this
* fallback to `'service'` is the breaking half still tracked on #5561, and
* it is this single expression.
*/
private resolveResumeAuthority(nodeType: string): ActionDescriptor['resumeAuthority'] {
private resolveResumeAuthority(nodeType: string): NonNullable<ActionDescriptor['resumeAuthority']> {
let descriptor = this.actionDescriptors.get(nodeType);
for (let hop = 0; descriptor?.aliasOf && hop < AutomationEngine.MAX_ALIAS_HOPS; hop++) {
const canonical = this.actionDescriptors.get(descriptor.aliasOf);
Expand Down
Loading
Loading