diff --git a/.changeset/resume-authority-fail-closed-by-omission.md b/.changeset/resume-authority-fail-closed-by-omission.md new file mode 100644 index 0000000000..38fedc2bc1 --- /dev/null +++ b/.changeset/resume-authority-fail-closed-by-omission.md @@ -0,0 +1,75 @@ +--- +'@objectstack/service-automation': major +'@objectstack/spec': major +'@objectstack/runtime': patch +--- + +feat(automation)!: 未声明 `resumeAuthority` 的暂停节点改为 fail-closed —— 通用 resume 路由从「默认开门」变成「显式 `'any'` 才开门」(#5561 第二步) + + + +**BREAKING**(仅影响注册了暂停型节点、且描述符未声明 `resumeAuthority` 的执行器 —— +本仓内为零)。`AutomationEngine.resolveResumeAuthority` 对缺省值的解析由 `'any'` 翻成 +`'service'`:一个从未声明「谁可以续跑它产生的暂停」的节点类型,其暂停在通用路由 +`POST /automation/:name/runs/:runId/resume` 上被拒绝(`PERMISSION_DENIED` / 403), +直到它的描述符把话说出来。通用 resume 门从此是描述符**主动 opt-in** 的一扇门,不是每个 +暂停节点**继承**来的默认。 + +这是 ADR-0044 2026-07-28 修正案里「记录但刻意不在此建造」的第一项,分两步落地。 +第一步(#5561 / PR #5725,非 breaking)把 `ActionDescriptorSchema.resumeAuthority` +的 Zod `.default('any')` 摘成 `.optional()`。那个默认值的问题不只是取值不对,而是它 +**抹掉了事实**:`defineActionDescriptor` 在任何消费者看到对象之前就把 key 填上了,于是 +「作者选了 `'any'`」和「作者从没考虑过」parse 出逐字节相同的描述符,遗漏根本无法被观测。 +默认值摘掉之后「缺省」才重新可见,注册告警与 `check:resume-authority-declared` CI 门也 +才写得出来。第二步就是本次改动:让缺省真正意味着 fail-closed。 + +### 为什么往「拒绝」这个方向猜 + +两种猜错的代价不对称,这就是全部理由。猜 `'any'`,会让一次 resume 走过一个**没有任何 +记录的决策**,而且悄无声息 —— #3823 就是这么发生的:ADR-0044 把审批的 `revise` 边指向 +了通用 `wait`,`wait` 本身声明 `'any'` 完全正确,而站在「服务持有」位置上的那个暂停 +继承了一个没人选过的 fail-open 值;实测代价是一次未经审计的重新提交,外加一个被销毁的 +远程 run。猜 `'service'`,则是返回一次拒绝,并把修好它的那一行原样交回作者手里。 +两种错误里只有一种能被犯错的人自己发现。 + +### 迁移:`resumeAuthority` 未声明 → 显式声明(一行) + +只有**注册暂停型节点的插件作者**需要动手,处方是在描述符上加一行: + +```ts +// FROM —— 依赖旧默认值,暂停可被通用路由续跑 +defineActionDescriptor({ + type: 'my_pause', version: '1.0.0', name: 'My Pause', + supportsPause: true, +}); + +// TO —— 通用路由确实是这个暂停的正门时(screen 式收集输入、signal wait 式外部生产者) +defineActionDescriptor({ + type: 'my_pause', version: '1.0.0', name: 'My Pause', + supportsPause: true, resumeAuthority: 'any', +}); + +// TO —— 续跑是「某个服务必须先授权并记录的决策」的尾巴时 +defineActionDescriptor({ + type: 'my_pause', version: '1.0.0', name: 'My Pause', + supportsPause: true, resumeAuthority: 'service', +}); +``` + +两个值都被接受,**只有沉默改变了含义**。三条运行时通道会指着同一件事说话:注册时按类型 +去重的一次告警、resume 被拒时那条点名缺省字段并给出处方的错误消息,以及本仓自有执行器的 +`check:resume-authority-declared` CI 门。 + +⚠️ `supportsPause` 本身是一个没有任何执行路径强制的声明(#5703)—— run 会暂停是因为 +`execute()` 返回了 `suspend: true`。所以一个「会暂停但把 `supportsPause` 留成 false」 +的执行器,注册告警与 CI 门**都看不见它**,只有 resume 时的拒绝消息会带上同一份处方。 +请按同一条规则手工核一遍这类执行器。 + +### 仓内零行为变化 + +在册的六个暂停类型全部已显式声明:`screen` / `wait` / `subflow` / `map` 声明 `'any'` +(第一步补齐),`approval` / `approval_revise` 声明 `'service'`。解析器测试与端到端测试 +都把这份清单和它们的解析结果一起断言 —— 一个只靠「什么都没注册」而变绿的零点名,和真的 +零点名是两回事。 + +`@objectstack/runtime` 只是注释与路由账本(`route-ledger`)的记述同步,无行为改动。 diff --git a/content/docs/automation/flows.mdx b/content/docs/automation/flows.mdx index db5c3f4b66..f12c107b17 100644 --- a/content/docs/automation/flows.mdx +++ b/content/docs/automation/flows.mdx @@ -493,16 +493,26 @@ The resume route is generic, so **the node the run is parked on** decides whether a raw resume is a legitimate continuation. Every action descriptor carries a `resumeAuthority`: -- **`'any'`** (the default — `screen`, `wait`, your own pausing nodes): the - caller supplies the continuation and the route is the intended door. -- **`'service'`** (`approval`): continuing is a *side effect* of a decision - that some service must authorize and record first, so only that service may - drive it. `ApprovalService.decide` enforces the approver slate, writes the - `sys_approval_action` row, mirrors the status field — **then** resumes. - -A resume of a `'service'` pause through the route answers **403** and changes -nothing: the request stays pending and the run stays parked, so the real -decision can still land. (Before this gate a raw resume walked the `approve` +- **`'any'`** (`screen`, `wait`): the caller supplies the continuation and the + route is the intended door. +- **`'service'`** (`approval`, `approval_revise`): continuing is a *side effect* + of a decision that some service must authorize and record first, so only that + service may drive it. `ApprovalService.decide` enforces the approver slate, + writes the `sys_approval_action` row, mirrors the status field — **then** + resumes. + + + **There is no default — an omission means `'service'`.** A pausing node type + whose descriptor never declares `resumeAuthority` is closed to the generic + route: its pauses answer **403** with a message naming the missing field. If + the route *is* the intended door for your node, declare + `resumeAuthority: 'any'` explicitly. This is an opt-in, not an inheritance — + a pause nobody claimed is one nobody authorized. + + +A resume of a `'service'` (or undeclared) pause through the route answers **403** +and changes nothing: the request stays pending and the run stays parked, so the +real decision can still land. (Before this gate a raw resume walked the `approve` edge with no decision recorded, leaving the `sys_approval_request` row and the run permanently disagreeing.) @@ -570,9 +580,12 @@ action with no `params` is untouched: - `signal.output`, which is the node-*output* namespace of the approval-style resume envelope rather than the screen's collected-values channel. -Registering a pausing node of your own? Declare `resumeAuthority: 'service'` on -its descriptor when the decision to continue belongs to your service rather -than to whoever holds the run id. +Registering a pausing node of your own? **Declare `resumeAuthority` — the field +is not optional in practice.** Use `'any'` when whoever holds the run id is +meant to supply the continuation (a screen's inputs, a signal wait's external +producer), and `'service'` when the decision to continue belongs to your service +rather than to whoever holds the run id. Declaring neither leaves your pauses +refused on the generic route, and the engine warns about it at registration. ### Parallel approvals — one aggregating node, not two pauses diff --git a/content/docs/references/automation/node-executor.mdx b/content/docs/references/automation/node-executor.mdx index 2227fc6de4..d0e42ad243 100644 --- a/content/docs/references/automation/node-executor.mdx +++ b/content/docs/references/automation/node-executor.mdx @@ -72,7 +72,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'>` | 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) | +| **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). Carries no schema default so an omission stays observable — and an omission is fail-CLOSED at run time, equivalent to 'service': a pausing node whose pause is open to the generic route must declare 'any' explicitly (#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 | diff --git a/docs/adr/0044-approval-send-back-for-revision.md b/docs/adr/0044-approval-send-back-for-revision.md index 94ad8dcb62..31f82cf28c 100644 --- a/docs/adr/0044-approval-send-back-for-revision.md +++ b/docs/adr/0044-approval-send-back-for-revision.md @@ -392,3 +392,49 @@ window. A graph that wanted `revise → notify → window` is refused rather tha analysed for "every pause reachable on this branch is service-owned", which is unbounded. Send-back already notifies the submitter itself, so the pattern has no lost capability behind it. + +### Fail-closed descriptor default — landed (2026-08-08, #5561) + +The first of the two "directions recorded but deliberately not built here" is now +built, in two steps, and this section supersedes the paragraph above that says +`resumeAuthority` "defaults to `'any'`". + +- **Step one (#5561, PR #5725, non-breaking).** `ActionDescriptorSchema.resumeAuthority` + dropped its Zod `.default('any')` and became `.optional()`. That default was not + merely a bad value, it was an *erasure*: `defineActionDescriptor` filled the key + before any consumer saw the object, so "the author chose `'any'`" and "the author + never considered it" parsed byte-identically and the omission could not be + detected at all. With the default gone, absent means absent — which is what made + a registration warning (`AutomationEngine.registerNodeExecutor`, once per node + type) and a CI gate (`check:resume-authority-declared`, AST over shipped + `defineActionDescriptor` literals) expressible. The four pausing built-ins + (`screen`, `wait`, `subflow`, `map`) declared `'any'` explicitly in the same + step, so the warning named nothing on a stock boot the day it shipped. +- **Step two (#5561, this change, breaking).** `AutomationEngine.resolveResumeAuthority` + resolves an absent value to `'service'` instead of `'any'`. A pausing node type + that never declares who may continue its pauses is now closed to the generic + resume route: `POST /automation/:name/runs/:runId/resume` answers **403** and + names the missing field. The generic door is an **opt-in** a descriptor states + with `resumeAuthority: 'any'`, not a default every pausing node inherits. + +**Why the amendment's reasoning survives the split.** The direction was always +about which way to guess when nobody declared. Guessing `'any'` continues a run +past a decision nothing recorded, and says nothing — that is #3823 exactly, and +its demonstrated cost was an unaudited resubmit plus a destroyed remote run. +Guessing `'service'` refuses a resume and hands the author back the one-line +declaration that fixes it. Only one of the two mistakes is discoverable by the +person who made it, and for a platform whose node vocabulary is extended by +plugins and by AI-written metadata, discoverability at authoring time is the whole +argument. + +**Migration.** One line, on the descriptor of any pausing executor that relied on +the old default: `resumeAuthority: 'any'`. Registered in the ADR-0087 chain as +`action-descriptor-resume-authority-default-flip` (step 17, semantic — it is a +posture change with no metadata shape to rewrite, the same category as protocol +12's `api.requireAuth` flip). In-tree the flip moves nothing: all six shipped +pausing types already declare their authority, which the resolver tests assert +alongside the inventory they depend on. + +**The second deferred direction is untouched.** *Per-suspension owner claim* stays +unbuilt and unneeded for the same reason as before — the cases we have are +resolved by node type plus a fail-closed default, which is now what exists. diff --git a/docs/protocol-upgrade-guide.md b/docs/protocol-upgrade-guide.md index 9a87253aec..5c996e65b8 100644 --- a/docs/protocol-upgrade-guide.md +++ b/docs/protocol-upgrade-guide.md @@ -220,6 +220,8 @@ Last, it reconciles the SDUI component-props surface with the renderers that ser Finally it narrows the aggregation vocabulary: `array_agg` and `string_agg` leave `AggregationFunction` (#6188, ADR-0049). The enum declared eight functions and the SQL family compiles five — `SqlDriver.mapAggregateFunc` and the Turso `RemoteTransport.aggregate` each lower `count`/`sum`/`avg`/`min`/`max` and route the rest to one refusal — so three were declared-but-unenforced against the backends this platform targets. What makes these two worse than an ordinary inert declaration is that another package had to carry a denylist for them: `service-analytics` subtracted `array_agg` and `string_agg` by name in `UNSUPPORTED_AGGREGATES`, because without that subtraction they reached the Cube strategy's `default` and returned `COUNT(*)` — a row count in place of the requested value, with no error and no log. The maintainer SPLIT the three rather than retiring them as a block (2026-08-07), and the split is the point: `count_distinct` STAYS and takes the enforce leg — one portable lowering (`COUNT(DISTINCT x)`), a dashboard staple, already lowered by `service-analytics` — with its SQL implementation following on its own card, so that declaration leads its implementation by decision rather than by drift. These two take the remove leg: display conveniences with no measured pull, and `string_agg` never had one shape to lower to (the delimiter is a second argument in PostgreSQL, a `SEPARATOR` clause in MySQL, a differently named function in SQL Server). This is an enum VALUE, not a key, so — as with `crypto.hash` above — there is no `retiredKey()` tombstone: the enum error map carries the prescription, keyed on the received value so only the two spellings that used to be legal are told they "were removed". Of the two authoring surfaces only one is stored metadata: the conversion rewrites `dataset.measures[].aggregate`, dropping the measure outright (a measure with neither `aggregate` nor `derived` fails the dataset's own refinement, so stripping just the key would emit an item that cannot parse) plus any derived measure the drop strands, with a notice each. Nothing is lost: `compileDataset` refused both by name already, so such a measure never produced a number. `QueryAST.aggregations[].function` is a request surface with no stored source — one semantic TODO below. The mongodb and in-memory backends that implemented these two are inside the #5499 freeze and are untouched; their code is simply no longer reachable through a spec-valid request. +One entry in this step is not a removal at all but a SECURE-DEFAULT FLIP, the shape protocol 12 last used for `api.requireAuth`: an omitted `ActionDescriptor.resumeAuthority` resolves to `'service'` instead of `'any'`, so a pausing node type that never states who may continue its pauses is refused on the generic resume route rather than open to it (#5561, ADR-0044's 2026-07-28 amendment). Nothing is removed and no metadata shape changes — the field has been optional since step one of the same issue — so tsc reports nothing and only the MEANING of silence moved. That is exactly why it needs a ledger entry: a third-party plugin author has no compile error to discover it with, and the one-line prescription (declare `resumeAuthority` on the descriptor) has to arrive before a user meets a run that will not continue. + ### Mechanical (applied for you) | Conversion | Surface | Change | Load window | @@ -387,6 +389,9 @@ Finally it narrows the aggregation vocabulary: `array_agg` and `string_agg` leav - **`etl-pipeline-layer-retired`** — `automation.etlPipeline / automation.etlPipelineRun / automation.etlSource / automation.etlDestination / automation.etlTransformation (the whole L2 layer of automation/etl.zod.ts, its four enums and the `ETL` factory — 9 defs, 27 exported names)` → (removed — no protocol surface replaces it, deliberately. Layer by layer: connector-attached synchronisation is `ConnectorSchema.syncConfig` (`integration/connector.zod.ts`), which IS parsed and executed; per-field value transformation on import is `shared/mapping.zod.ts`, whose `transform` is applied row by row by the REST import path and recorded key by key in `packages/spec/liveness/mapping.json`; scheduling is `system/job.zod.ts`. What has NO replacement is multi-source, multi-stage movement with joins and aggregations — because it never had an implementation either. It returns through the ENFORCE route: the engine first, the vocabulary second) - Why not automatic: The reading #4738 used to retire L1 `DataSyncConfig`, re-measured one layer up and identical: narrative-only. No engine ever parsed, scheduled or executed an `ETLPipeline`. Measured on origin/main immediately before the removal: the only non-spec references in this repo are two fumadocs-generated documentation sources (`apps/docs/.source/*.ts`), not executors; objectui has no reference at all; there is no `liveness/etl.json` or `pipeline.json`, so no ADR-0049 gate ever had a reading on it — while the same file family's EXECUTED half does have one (`liveness/mapping.json`), which is the contrast that makes the absence meaningful rather than an oversight. The `etl` string in this registry was the one untested link the finding named, and it is not a loader path: it was the id of the #4962 retry-vocabulary entry, absorbed here. The layer was ADR-0078's asymmetry in its purest form — an author could write a complete ten-stage pipeline, get no error, and get no execution. It was also advertised: `packages/spec/docs/SYNC_ARCHITECTURE.md` named `ETLPipeline` as the recommended destination for authors displaced by the L1 retirement (#4738) and listed ten transformation types with copyable examples down to `script | Custom JavaScript/Python`. That document is rewritten in the same change; a retirement whose own doc still recommends the retired layer is self-contradictory, and forwarding L1's authors to a second layer with no executor was the defect compounding rather than closing. ⚠️ `etl-retry-converged-onto-retry-policy` (#4962) is SUBSUMED here, the #4657/#4834/#5055 way: both land in the unreleased protocol 17, so composed, a rename of `retry.maxAttempts` on a shape that does not survive the major has no observable effect — and keeping both would tell an upgrader to rewrite a key on a schema the same upgrade deletes. The `maxAttempts` `retiredKey()` tombstone goes with the shape that carried it, which is strictly stronger than the tombstone: there is no longer a `retry` block to author the key into. Route 3 — no carrier key, no parse site, so no D2 conversion and no tombstone; RETIRED_DEFS_BY_MAJOR plus this entry are the declaration. ADR-0049, ADR-0078, #6414. - Done when: No source imports `ETLPipeline`, `ETLPipelineParsed`, `ETLPipelineSchema`, `ETLPipelineRun(Schema)`, `ETLSource(Schema)`, `ETLDestination(Schema)`, `ETLTransformation(Schema)`, `ETLEndpointType(Schema)`, `ETLTransformationType(Schema)`, `ETLSyncMode(Schema)`, `ETLRunStatus(Schema)` or the `ETL` factory from `@objectstack/spec/automation`; `tsc` reports TS2724/TS2305 on any that survives. Every author who was pointed at L2 has been re-pointed by name: SYNC_ARCHITECTURE.md no longer lists an L2 row, no longer recommends `ETLPipeline` as L1's destination and no longer advertises a transformation-type table. The surviving layers still parse unchanged — a connector declaring `syncConfig` and an import declaring `mapping.transform` both behave exactly as they did in 16.x. +- **`action-descriptor-resume-authority-default-flip`** — `automation.ActionDescriptor.resumeAuthority — an OMITTED value on a pausing node descriptor (supportsPause: true, or any executor whose execute() returns suspend: true)` → an explicit resumeAuthority: 'any' on the descriptor, for a pausing node whose pauses really are meant to be continued through the generic resume route (POST /automation/:name/runs/:runId/resume) — a screen-style collected-input pause, or a signal wait an external producer resumes. Declare 'service' instead if continuing is the tail of a decision your own service must authorize and record first. Either value is a one-line addition; only the silence changed meaning + - Why not automatic: A SECURE-DEFAULT FLIP with no metadata shape to rewrite — the same category as protocol 12's `rest-requireauth-default-flip`, and it is registered here for the same reason: whether a given pause is genuinely open to the generic route is a trust judgment no transform can make. The #3801 resume gate keys on the SUSPENDED NODE, and `ActionDescriptor.resumeAuthority` used to default to `'any'`, so a pausing node type shipped raw-resumable unless its author remembered the field. It now resolves to `'service'` when absent: an unclaimed pause is refused on the generic route with `PERMISSION_DENIED` / 403 until its descriptor states who may continue it. #3823 is the incident that decided the direction — 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 demonstrated cost was an unaudited resubmit plus a destroyed remote run. The two possible mistakes are asymmetric, which is the whole argument: guessing `'any'` walks past a decision nothing recorded and is silent, while guessing `'service'` returns a refusal naming the missing field. ⚠️ The surface is a DESCRIPTOR FIELD set in plugin CODE, never stack metadata, so there is no source for a D2 conversion to rewrite and deliberately no schema tombstone — the disposition `data-driver-find-stream-retired` (#4484), `storage-service-list-retired` (#5540) and `actor-user-roles-to-positions` (#6011) already carry. It differs from those in one way a reader should not have to infer: nothing is REMOVED, so tsc reports nothing at all — the field was already optional after step one and an omission still compiles. The enforced channels are all run-time: a registration warning naming the node type (once per type per engine), the refusal message on the resume itself, and `check:resume-authority-declared` for executors living in this repo. For a third-party plugin the generated upgrade guide is the only channel that arrives BEFORE a user hits a run that will not continue. In-tree the flip moves nothing: all six shipped pausing types (screen, wait, subflow, map, approval, approval_revise) declare their authority explicitly. ADR-0044 amendment (2026-07-28) and its 2026-08-08 landing section, ADR-0019 #3801 addendum, #5561. + - Done when: Every action descriptor your plugin registers for a node type that can suspend declares `resumeAuthority`. Booting the stack logs no `declares supportsPause but never declares resumeAuthority` warning naming one of your types, and a run parked on each of your pausing nodes can still be continued the way you intend: a resume through the generic route succeeds for the ones you declared `'any'`, and answers 403 (`PERMISSION_DENIED`) for the ones you declared `'service'`, which continue through your own service API instead. ⚠️ `supportsPause` is a declaration nothing enforces (#5703), so an executor whose `execute()` returns `suspend: true` while leaving `supportsPause` false is warned about by NEITHER channel — check those by hand against the same rule. --- diff --git a/packages/runtime/src/domains/automation.ts b/packages/runtime/src/domains/automation.ts index 933ee76110..237046eb63 100644 --- a/packages/runtime/src/domains/automation.ts +++ b/packages/runtime/src/domains/automation.ts @@ -353,7 +353,9 @@ export async function handleAutomationRequest(deps: DomainHandlerDeps, path: str // `ApprovalService`, which records the decision and enforces the slate — // on a SYMBOL-keyed marker. Assembling the signal field-wise (never // spreading the body) keeps that unforgeable even if a caller invents - // extra keys. + // extra keys. Since #5561 a node type that declares NO `resumeAuthority` + // is gated the same way, so this door is one a descriptor opts into with + // `'any'` rather than one every pausing node inherits. // // REFUSAL codes come back from the engine and are answered as such // rather than a 200 carrying `success: false` (which reads as "your diff --git a/packages/runtime/src/route-ledger.ts b/packages/runtime/src/route-ledger.ts index f80bc678be..06ff02f327 100644 --- a/packages/runtime/src/route-ledger.ts +++ b/packages/runtime/src/route-ledger.ts @@ -273,7 +273,7 @@ export const ROUTE_LEDGER: readonly RouteLedgerEntry[] = [ { route: 'POST /automation/:name/trigger', domain: '/automation', disposition: 'sdk', client: 'automation.execute' }, { route: 'POST /automation/:name/toggle', domain: '/automation', disposition: 'sdk', client: 'automation.toggle' }, { route: 'POST /automation/:name/runs/:runId/resume', domain: '/automation', disposition: 'sdk', client: 'automation.resume', - note: "generic, so the SUSPENDED NODE gates it (#3801): a pause whose descriptor declares resumeAuthority:'service' — today `approval` — answers 403 here and continues only through its owning service (ApprovalService.decide), which authorizes and records the decision first. Screen/wait pauses are unaffected; this route is the screen-flow runner's door" }, + note: "generic, so the SUSPENDED NODE gates it (#3801): a pause whose descriptor declares resumeAuthority:'service' — today `approval` / `approval_revise` — answers 403 here and continues only through its owning service (ApprovalService.decide), which authorizes and records the decision first. A node type that declares NO resumeAuthority answers 403 too, fail-closed since #5561: this door is an opt-in a descriptor states with 'any'. Screen/wait pauses are unaffected because they declare it; this route is the screen-flow runner's door" }, { route: 'GET /automation/:name/runs/:runId/screen', domain: '/automation', disposition: 'sdk', client: 'automation.getScreen' }, { route: 'GET /automation/:name/runs/:runId', domain: '/automation', disposition: 'sdk', client: 'automation.getRun' }, { route: 'GET /automation/:name/runs', domain: '/automation', disposition: 'sdk', client: 'automation.listRuns' }, diff --git a/packages/services/service-automation/src/builtin/map-node.test.ts b/packages/services/service-automation/src/builtin/map-node.test.ts index f0a81eb38c..d544d6dd62 100644 --- a/packages/services/service-automation/src/builtin/map-node.test.ts +++ b/packages/services/service-automation/src/builtin/map-node.test.ts @@ -5,6 +5,20 @@ import { AutomationEngine } from '../engine.js'; import type { NodeExecutor } from '../engine.js'; import { InMemorySuspendedRunStore } from '../suspended-run-store.js'; import { registerMapNode } from './map-node.js'; +import { defineActionDescriptor } from '@objectstack/spec/automation'; + +/** + * The `resumeAuthority: 'any'` a pausing fixture needs since #5561: the resume + * gate follows a linked-run chain to the CHILD's node, so the type the child + * parks on is what a resume of the parent is judged against — and a type that + * declares nothing is now refused there. These tests are about linked-run + * mechanics, not the gate (`resume-authority-gate.test.ts` owns that), so the + * fixtures state the posture they rely on. + */ +const openPauser = (type: string) => defineActionDescriptor({ + type, version: '1.0.0', name: type, + supportsPause: true, resumeAuthority: 'any', +}); function silentLogger() { return { info() {}, warn() {}, error() {}, debug() {}, child() { return silentLogger(); } } as any; @@ -34,6 +48,7 @@ function setup(childNodes: Array<{ id: string; type: string }>, captured: unknow // Pauses the child (stands in for an approval / screen / wait). engine.registerNodeExecutor({ type: 'pauser', + descriptor: openPauser('pauser'), async execute() { return { success: true, suspend: true }; }, } as NodeExecutor); // Fails the child terminally. diff --git a/packages/services/service-automation/src/builtin/subflow-node.test.ts b/packages/services/service-automation/src/builtin/subflow-node.test.ts index a3b4f41686..42725c8b32 100644 --- a/packages/services/service-automation/src/builtin/subflow-node.test.ts +++ b/packages/services/service-automation/src/builtin/subflow-node.test.ts @@ -5,6 +5,20 @@ import { AutomationEngine } from '../engine.js'; import type { NodeExecutor } from '../engine.js'; import { InMemorySuspendedRunStore } from '../suspended-run-store.js'; import { registerSubflowNode } from './subflow-node.js'; +import { defineActionDescriptor } from '@objectstack/spec/automation'; + +/** + * The `resumeAuthority: 'any'` a pausing fixture needs since #5561: the resume + * gate follows a linked-run chain to the CHILD's node, so the type the child + * parks on is what a resume of the parent is judged against — and a type that + * declares nothing is now refused there. These tests are about linked-run + * mechanics, not the gate (`resume-authority-gate.test.ts` owns that), so the + * fixtures state the posture they rely on. + */ +const openPauser = (type: string) => defineActionDescriptor({ + type, version: '1.0.0', name: type, + supportsPause: true, resumeAuthority: 'any', +}); function silentLogger() { return { info() {}, warn() {}, error() {}, debug() {}, child() { return silentLogger(); } } as any; @@ -47,11 +61,13 @@ describe('subflow node executor', () => { // A node that suspends (to exercise nested durable pause). engine.registerNodeExecutor({ type: 'pauser', + descriptor: openPauser('pauser'), async execute() { return { success: true, suspend: true }; }, } as NodeExecutor); // A screen-style pauser: suspends surfacing the screen from node config. engine.registerNodeExecutor({ type: 'screenpauser', + descriptor: openPauser('screenpauser'), async execute(node) { return { success: true, suspend: true, screen: (node.config as any)?.screen }; }, @@ -314,7 +330,7 @@ describe('subflow node executor', () => { type: 'parentcheck', async execute(_node, variables) { capturedB.push(variables.get('subResult')); return { success: true }; }, } as NodeExecutor); - engineB.registerNodeExecutor({ type: 'pauser', async execute() { return { success: true, suspend: true }; } } as NodeExecutor); + engineB.registerNodeExecutor({ type: 'pauser', descriptor: openPauser('pauser'), async execute() { return { success: true, suspend: true }; } } as NodeExecutor); engineB.registerFlow('paused_child', pausedChild([{ id: 'p', type: 'pauser' }]) as never); engineB.registerFlow('parent_flow', parentFlow({ flowName: 'paused_child', outputVariable: 'subResult' }) as never); diff --git a/packages/services/service-automation/src/engine-residual-log-cause.test.ts b/packages/services/service-automation/src/engine-residual-log-cause.test.ts index 9e1180af93..5ab160aeb4 100644 --- a/packages/services/service-automation/src/engine-residual-log-cause.test.ts +++ b/packages/services/service-automation/src/engine-residual-log-cause.test.ts @@ -56,6 +56,8 @@ import { ObjectLogger } from '@objectstack/core'; import { AutomationEngine } from './engine.js'; import type { NodeExecutor, SuspendedRun, SuspendedRunStore, FlowTrigger } from './engine.js'; import type { AutomationContext } from '@objectstack/spec/contracts'; +import { defineActionDescriptor } from '@objectstack/spec/automation'; +import { registerScreenNodes } from './builtin/screen-nodes.js'; // ── fixtures ─────────────────────────────────────────────────────────────── @@ -83,10 +85,22 @@ function workingStore(overrides: Partial): SuspendedRunStore }; } -/** A pausing executor; `onRelease` optionally makes its teardown throw. */ +/** + * A pausing executor; `onRelease` optionally makes its teardown throw. + * + * Declares `resumeAuthority: 'any'` because these tests continue the pause + * through the public {@link AutomationEngine.resume} door. Since #5561 that is + * an opt-in: a node type that declares nothing is refused there, so a fixture + * exercising anything OTHER than the resume gate has to state the posture it + * relies on — the same declaration the four pausing built-ins carry. + */ function pauser(onRelease?: () => void): NodeExecutor { return { type: 'pauser', + descriptor: defineActionDescriptor({ + type: 'pauser', version: '1.0.0', name: 'Pauser', + supportsPause: true, resumeAuthority: 'any', + }), async execute(node) { return { success: true, suspend: true, correlation: `test-armature:${node.id}` }; }, @@ -477,6 +491,14 @@ describe('#6499 sites 9–13 — engine-internal seams carrying foreign text, al const engine = new AutomationEngine(jsonLogger(), workingStore({ async load(runId) { return runId === 'run_scr' ? parked : null; }, })); + // The real `screen` executor, as any booted engine has it. Needed since + // #5561 step two: the resume below goes through the public gate, and the + // authority of a pause is read off the SUSPENDED NODE's descriptor — with + // no `screen` registered there is no descriptor, which now resolves + // fail-closed and refuses the resume before this seam is ever reached. + // `screen` declares `resumeAuthority: 'any'`, so registering it restores + // exactly the production shape this site is about. + registerScreenNodes(engine, { logger: jsonLogger(), getService() { return undefined; } } as never); engine.registerFlow('onboard', { name: 'onboard', label: 'Onboard', diff --git a/packages/services/service-automation/src/engine.test.ts b/packages/services/service-automation/src/engine.test.ts index 8ec5a90e71..a5fa545009 100644 --- a/packages/services/service-automation/src/engine.test.ts +++ b/packages/services/service-automation/src/engine.test.ts @@ -8,6 +8,22 @@ import { registerScreenNodes } from './builtin/screen-nodes.js'; import { InMemorySuspendedRunStore } from './suspended-run-store.js'; import type { NodeExecutor } from './engine.js'; import type { IAutomationService } from '@objectstack/spec/contracts'; +import { defineActionDescriptor } from '@objectstack/spec/automation'; + +/** + * A pausing fixture's `resumeAuthority: 'any'` declaration (#5561). + * + * These tests continue their pause through the public `AutomationEngine.resume` + * door, and since #5561 step two a node type opts into that door rather than + * inheriting it — one that declares nothing is refused. Nothing in this file is + * about the resume gate itself (`resume-authority-gate.test.ts` owns it), so each + * pausing fixture states the posture it relies on, exactly as the four pausing + * built-ins do. + */ +const openPauser = (type: string) => defineActionDescriptor({ + type, version: '1.0.0', name: type, + supportsPause: true, resumeAuthority: 'any', +}); // ─── Helper: Create a minimal logger for unit tests ───────────────── @@ -463,6 +479,7 @@ describe('AutomationEngine', () => { function registerPausingNode(captured: { runId?: unknown }) { engine.registerNodeExecutor({ type: 'pause_node', + descriptor: openPauser('pause_node'), async execute(_node, variables) { captured.runId = variables.get('$runId'); return { success: true, suspend: true, correlation: 'req_1' }; @@ -690,6 +707,7 @@ describe('AutomationEngine', () => { const e = new AutomationEngine(createTestLogger(), store); e.registerNodeExecutor({ type: 'pause_node', + descriptor: openPauser('pause_node'), async execute() { // Snapshot a nested object + array so we can assert the // variable map round-trips through the store. @@ -791,6 +809,7 @@ describe('AutomationEngine', () => { const e = new AutomationEngine(createTestLogger()); // no store e.registerNodeExecutor({ type: 'pause_node', + descriptor: openPauser('pause_node'), async execute() { return { success: true, suspend: true, correlation: 'req_1' }; }, }); e.registerFlow('p', { @@ -1756,6 +1775,7 @@ describe('AutomationEngine - Back-edge re-entry (ADR-0044)', () => { it('cancelRun consumes a suspended run and records a terminal cancelled log', async () => { engine.registerNodeExecutor({ type: 'pause', + descriptor: openPauser('pause'), async execute() { return { success: true, suspend: true, correlation: 'test-pause' }; }, }); engine.registerFlow('pausing_flow', { diff --git a/packages/services/service-automation/src/engine.ts b/packages/services/service-automation/src/engine.ts index d4adcef0a4..461dc7472c 100644 --- a/packages/services/service-automation/src/engine.ts +++ b/packages/services/service-automation/src/engine.ts @@ -931,7 +931,8 @@ export interface SuspendedRun { * Registry type of the node that produced the pause (`approval`, `screen`, * `wait`, …), captured at suspend time. Keys the resume gate (#3801): the * descriptor's `resumeAuthority` decides whether a raw - * {@link AutomationEngine.resume} is a legitimate continuation. + * {@link AutomationEngine.resume} is a legitimate continuation, and a type + * that declares none is refused rather than assumed open (#5561). * * Recorded on the suspension rather than re-derived from the live flow so * the gate reflects what actually paused the run — a flow republished @@ -1438,10 +1439,19 @@ export class AutomationEngine implements IAutomationService { * 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). + * pausing type that leaves it absent is judged by nobody's 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). + * + * **Since #5561 step two this warning precedes a refusal, not a silence.** + * An omission used to resolve `'any'`, so the line was pure advice about a + * run-time behaviour that was already happening; it now resolves + * `'service'` ({@link RESUME_AUTHORITY_WHEN_UNDECLARED}), so every pause the + * named type creates will be refused on the generic resume route. The line + * says so, and says the one-line fix, because registration is the earliest + * moment the author can hear it — the alternative is hearing it from a user + * whose run will not continue. * * **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, @@ -1449,17 +1459,16 @@ export class AutomationEngine implements IAutomationService { * 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. + * which reports a missing CALL for the same reason). * * **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. + * leaving `supportsPause` false is therefore silent here, and since step two + * its pauses are refused with no prior warning. The refusal message carries + * the same prescription for exactly that reader (see + * {@link refuseGatedResume}), `check:resume-authority-declared` catches this + * repo's own executors at authoring time, and #5703 tracks the runtime half. */ private warnIfResumeAuthorityUndeclared(descriptor: ActionDescriptor): void { if (descriptor.supportsPause !== true) return; @@ -1468,13 +1477,13 @@ export class AutomationEngine implements IAutomationService { 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. ` + + `resumeAuthority, so the #3801 resume gate REFUSES every pause it creates on the generic route ` + + `(POST /automation/:name/runs/:runId/resume) — an unclaimed pause is fail-closed since #5561, ` + + `because the opposite guess 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.`, + `some service must authorize and record first. Declaring 'any' is what RESTORES the generic ` + + `route for this type. Reported once per node type per engine.`, ); } @@ -2948,7 +2957,8 @@ export class AutomationEngine implements IAutomationService { * **Authorization (#3801).** This is the public door — the generic REST * resume route and the SDK land here — so it is gated on WHAT THE RUN IS * PARKED ON before any state is touched: a suspension whose node declares - * `resumeAuthority: 'service'` is refused unless the signal carries + * `resumeAuthority: 'service'` — or declares no `resumeAuthority` at all, + * fail-closed since #5561 — is refused unless the signal carries * {@link RESUME_AUTHORITY_SERVICE}. The engine's own continuations * (subflow delegation / up-bubble, `map` re-entry, wait-timer wake) go * through {@link resumeInternal} and are not re-gated — they continue work @@ -2973,10 +2983,22 @@ export class AutomationEngine implements IAutomationService { * Resolves the EFFECTIVE suspension first: a run parked on a `subflow` or * `map` node is really waiting on a CHILD run, so the gate follows that * chain and judges the node the signal lands on (subflow) or would advance - * past (map) — see {@link LINKED_RUN_PREFIXES}. Anything it cannot resolve - * (unknown run, missing flow, unregistered node type) is left to + * past (map) — see {@link LINKED_RUN_PREFIXES}. A run it cannot resolve at + * all (unknown run id, nothing suspended, no resolvable node type) is left to * `resumeInternal`, which reports the machine-state error — the gate only * ever speaks to authorization. + * + * **An UNDECLARED node type is refused, not deferred** (#5561 step two). It + * used to be let through on the schema default's inherited `'any'`; a pause + * whose type never stated who may continue it is now closed until its author + * states it. The refusal says WHICH of the two reasons applies, because they + * ask opposite things of the reader: a declared `'service'` node is working + * exactly as designed and the caller must go through the owning service, + * while an undeclared one is a missing one-line declaration on a descriptor + * and the fix belongs to whoever registered it. Emitting the `'service'` + * wording for both would tell an author their node declares something it + * never declared — the failure mode this whole issue is about, restated as a + * log line. */ private async refuseGatedResume(runId: string, signal?: ResumeSignal): Promise { const run = await this.resolveEffectiveSuspension(runId); @@ -2989,27 +3011,45 @@ export class AutomationEngine implements IAutomationService { // decision's tail, not a way around it. if (signal?.[RESUME_AUTHORITY_SERVICE]) return null; + // Refusing. Which of the two reasons? A second registry walk, on the + // refusal path only, so the message can tell a node that deliberately + // declared `'service'` from one that declared nothing at all. + const declared = this.resolveDeclaredResumeAuthority(nodeType); const direct = run.runId === runId; const at = direct ? `'${run.nodeId}'` : `'${run.nodeId}' (linked run '${run.runId}')`; - this.logger.warn( - `[automation] refused resume of run '${runId}': parked on ${nodeType} node ${at}, which is resumable ` + - `only through its owning service (resumeAuthority: 'service')`, - ); + const why = declared === 'service' + ? `which is resumable only through its owning service (resumeAuthority: 'service')` + : `whose type never declares resumeAuthority, so it is closed to the generic route until it does ` + + `(#5561) — declare resumeAuthority: 'any' on its descriptor if this route IS the intended door`; + this.logger.warn(`[automation] refused resume of run '${runId}': parked on ${nodeType} node ${at}, ${why}`); + + // The fix, identical in both the direct and the linked-run phrasing — + // what has to change is a descriptor, not the call that just failed. + const undeclaredFix = + `and that node type never declares resumeAuthority, so the generic resume route is closed to the ` + + `pauses it creates (#5561). If that route IS the intended door — a screen's collected inputs, a ` + + `signal wait's external producer — declare resumeAuthority: 'any' on its action descriptor; declare ` + + `'service' if resuming is the tail of a decision some service must authorize and record first`; return { success: false, code: 'PERMISSION_DENIED', - error: direct - ? `Run '${runId}' is paused at a '${nodeType}' node, which only its owning service may resume — ` + - `drive it through that service's API (e.g. an approval decision), not a raw resume` - : `Run '${runId}' is waiting on run '${run.runId}', which is paused at a '${nodeType}' node that ` + - `only its owning service may resume — resuming here would continue past a decision that has not ` + - `been made; drive it through that service's API instead`, + error: declared === 'service' + ? direct + ? `Run '${runId}' is paused at a '${nodeType}' node, which only its owning service may resume — ` + + `drive it through that service's API (e.g. an approval decision), not a raw resume` + : `Run '${runId}' is waiting on run '${run.runId}', which is paused at a '${nodeType}' node that ` + + `only its owning service may resume — resuming here would continue past a decision that has not ` + + `been made; drive it through that service's API instead` + : direct + ? `Run '${runId}' is paused at a '${nodeType}' node, ${undeclaredFix}` + : `Run '${runId}' is waiting on run '${run.runId}', which is paused at a '${nodeType}' node, ` + + `${undeclaredFix}`, }; } /** - * The `resumeAuthority` in force for a node type, following a deprecated - * ADR-0018 alias to its canonical type. + * The authority a node type **declared**, following a deprecated ADR-0018 + * alias to its canonical type — `undefined` when nothing declared one. * * An alias's descriptor is synthesized by {@link registerNodeAlias} and does * NOT copy the canonical's capabilities, so reading it directly would hand @@ -3019,24 +3059,60 @@ export class AutomationEngine implements IAutomationService { * 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. + * Split out from {@link resolveResumeAuthority} because since #5561 step two + * the two facts differ: a type that declared `'service'` and a type that + * declared nothing are both refused, but for opposite reasons, and the + * refusal has to say which (one is working as designed, the other is a + * missing one-line declaration). One walk, so the alias hop can never be + * implemented twice and drift. */ - private resolveResumeAuthority(nodeType: string): NonNullable { + private resolveDeclaredResumeAuthority(nodeType: string): 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); if (!canonical || canonical === descriptor) break; descriptor = canonical; } - return descriptor?.resumeAuthority ?? 'any'; + return descriptor?.resumeAuthority; + } + + /** + * The `resumeAuthority` in force for a node type: what it declared, or + * {@link RESUME_AUTHORITY_WHEN_UNDECLARED} when it declared nothing. + * + * **The fallback is the whole of #5561 step two.** With no schema default on + * `resumeAuthority` (step one), an undeclared descriptor arrives with the key + * absent and this is the ONE place that resolves it — so this single + * expression is where "a pause nobody claimed" is either open to the world or + * closed to everyone. It used to resolve `'any'`, inherited from the schema + * default step one removed; it now resolves `'service'`, and a pause whose + * node type never stated who may continue it is refused on the generic route + * until its author says otherwise. + * + * That is the direction #3823 was decided in: ADR-0044 pointed a 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 + * cost of guessing wrong is asymmetric — guessing `'any'` walks past a + * decision nothing recorded, guessing `'service'` returns a refusal that names + * the one-line fix — so the guess is made in the direction that is loud + * instead of the direction that is silent. + */ + private resolveResumeAuthority(nodeType: string): NonNullable { + return this.resolveDeclaredResumeAuthority(nodeType) ?? AutomationEngine.RESUME_AUTHORITY_WHEN_UNDECLARED; } + /** + * What an UNDECLARED `resumeAuthority` resolves to (#5561 step two). + * + * The single source of truth for the fail-closed default — the registration + * warning, the refusal message and {@link resolveResumeAuthority} all speak + * about the same constant rather than three copies of a string literal. + * `ActionDescriptorSchema.resumeAuthority` deliberately carries no Zod + * `.default()` (that is what makes an omission observable at all), so this is + * the default in every sense that matters at run time. + */ + private static readonly RESUME_AUTHORITY_WHEN_UNDECLARED = 'service' as const; + /** Depth bound for the subflow chain walk — a corrupt correlation cycle * must not spin the gate. Far above any real nesting. */ private static readonly MAX_SUSPENSION_CHAIN_DEPTH = 32; diff --git a/packages/services/service-automation/src/resume-authority-declaration.test.ts b/packages/services/service-automation/src/resume-authority-declaration.test.ts index 2b683960d3..43fc8a6112 100644 --- a/packages/services/service-automation/src/resume-authority-declaration.test.ts +++ b/packages/services/service-automation/src/resume-authority-declaration.test.ts @@ -13,9 +13,10 @@ * decision nothing had recorded). * * The default is gone, so absent means absent, and `registerNodeExecutor` says - * so once per node type. Runtime authority resolution is unchanged — absent - * still resolves to `'any'` — which is why this file asserts about the LOG and - * `resume-authority-gate.test.ts` still asserts about behaviour. + * so once per node type. Since step two absent also RESOLVES fail-closed + * (`'service'`), so the warning now precedes a refusal rather than describing a + * silence — this file still asserts about the LOG, and + * `resume-authority-gate.test.ts` asserts the refusal it warns about. */ import { describe, it, expect } from 'vitest'; @@ -68,19 +69,23 @@ describe('resumeAuthority omission warning (#5561)', () => { expect(found[0]).toContain("'plugin_pause'"); }); - it('tells the author how to silence it, and that silencing changes no behaviour', () => { + it('names the consequence (refusal) and the declaration that lifts it', () => { const warnings: string[] = []; engineWith(warnings).registerNodeExecutor(executor('plugin_pause', { supportsPause: true })); const [line] = omissions(warnings); - // Single self-sufficient line: the two legal values, the incident, and the - // fact that declaring 'any' is a no-op at run time (so an author whose node - // really is open does not feel pushed into 'service' to quieten a log). + // Single self-sufficient line: the two legal values, the incident, and what + // now actually happens to this type's pauses. Step one's wording — that + // declaring 'any' "changes no behaviour" — is exactly what step two made + // false, and a warning that still said it would send an author away from the + // one field that restores their resume route. expect(line).toContain("'any'"); expect(line).toContain("'service'"); expect(line).toContain('#3801'); expect(line).toContain('#3823'); - expect(line).toContain("Declaring 'any' explicitly silences this and changes no behaviour"); + expect(line).toContain('REFUSES'); + expect(line).toContain("Declaring 'any' is what RESTORES the generic route"); + expect(line).not.toContain('changes no behaviour'); expect(line.split('\n')).toHaveLength(1); }); @@ -180,26 +185,63 @@ describe('resumeAuthority omission warning (#5561)', () => { }); }); -describe('resumeAuthority resolution is unchanged by the missing default (#5561)', () => { +describe('resumeAuthority resolution is fail-closed by omission (#5561 step two)', () => { /** - * The half that must NOT move in step one: an undeclared pausing type is still - * resolved fail-open, because `resolveResumeAuthority`'s `?? 'any'` is what the - * removed schema default used to do. Flipping that expression to `'service'` is - * the breaking half still tracked on #5561; this pins today's answer so that - * flip cannot happen silently. + * The breaking half. `resolveResumeAuthority`'s fallback used to be `'any'` — + * inherited from the schema default step one removed — and is now `'service'`, + * so an undeclared pausing type is closed to the generic resume route instead + * of open to it. This is the ONE expression the whole flip lives in, which is + * why it is pinned directly here as well as end-to-end in + * `resume-authority-gate.test.ts`. */ - it("resolves an undeclared pausing type to 'any', exactly as the removed default did", () => { - const warnings: string[] = []; - const engine = engineWith(warnings); - engine.registerNodeExecutor(executor('plugin_pause', { supportsPause: true })); - - const resolve = (engine as unknown as { + const resolverOf = (engine: AutomationEngine) => + (engine as unknown as { resolveResumeAuthority(t: string): 'any' | 'service'; }).resolveResumeAuthority.bind(engine); - expect(resolve('plugin_pause')).toBe('any'); - // Unregistered types keep answering 'any' too — the gate speaks only to - // authorization and leaves machine-state errors to `resumeInternal`. - expect(resolve('never_registered')).toBe('any'); + it("resolves an undeclared pausing type to 'service' — the opposite of the removed default", () => { + const engine = engineWith([]); + engine.registerNodeExecutor(executor('plugin_pause', { supportsPause: true })); + + const resolve = resolverOf(engine); + + expect(resolve('plugin_pause')).toBe('service'); + // A type nothing ever registered declared nothing either, so it answers the + // same way. The gate still speaks only to authorization: a run whose node + // type cannot be resolved at all never reaches this, and machine-state + // errors stay `resumeInternal`'s to report. + expect(resolve('never_registered')).toBe('service'); + }); + + it('leaves an explicit declaration untouched in both directions', () => { + const engine = engineWith([]); + engine.registerNodeExecutor(executor('open_pause', { supportsPause: true, resumeAuthority: 'any' })); + engine.registerNodeExecutor(executor('gated_pause', { supportsPause: true, resumeAuthority: 'service' })); + + const resolve = resolverOf(engine); + + // The flip moves the fallback only — a declared value is a decision, and + // `'any'` must keep meaning `'any'` or the migration prescription is a lie. + expect(resolve('open_pause')).toBe('any'); + expect(resolve('gated_pause')).toBe('service'); + }); + + it('reads the four pausing built-ins as open, because each declares so', () => { + // The repo-is-unchanged proof, at the resolver rather than through the + // engine: every shipped pausing type declared `'any'` in step one, so the + // flip moves nothing in this tree. A green suite alone could not tell that + // apart from "no pausing type was reachable" (the empty-green trap), so the + // inventory is asserted with it. + const engine = engineWith([]); + const ctx: any = { logger: loggerInto([]), getService() { return undefined; } }; + installBuiltinNodes(engine, ctx); + + const resolve = resolverOf(engine); + const pausing = engine.getActionDescriptors().filter((d) => d.supportsPause === true); + + expect(pausing.map((d) => d.type).sort()).toEqual(['map', 'screen', 'subflow', 'wait']); + for (const d of pausing) { + expect(resolve(d.type), `${d.type} must resolve open, from its own declaration`).toBe('any'); + } }); }); diff --git a/packages/services/service-automation/src/resume-authority-gate.test.ts b/packages/services/service-automation/src/resume-authority-gate.test.ts index 03435a882d..e044d3b752 100644 --- a/packages/services/service-automation/src/resume-authority-gate.test.ts +++ b/packages/services/service-automation/src/resume-authority-gate.test.ts @@ -15,6 +15,14 @@ * whose descriptor declares `resumeAuthority: 'service'` is resumable only * with the in-process {@link RESUME_AUTHORITY_SERVICE} marker — which no JSON * body can carry. A `screen` pause (the reason the route exists) is untouched. + * + * **#5561 step two moved where the line sits.** An omission used to resolve + * `'any'` (the schema default step one removed), so a pausing type that never + * stated its authority was raw-resumable. It now resolves `'service'`: the + * generic route is an OPT-IN a descriptor declares, not a default it inherits. + * `open_pause` below therefore declares `'any'` explicitly — it is the stand-in + * for `screen`/`wait`, which declare it too — and the undeclared shape gets its + * own describe block asserting the refusal plus the one-line fix that lifts it. */ import { describe, it, expect, beforeEach } from 'vitest'; @@ -61,11 +69,14 @@ function registerPausers(engine: AutomationEngine): void { async execute() { return { success: true, suspend: true, correlation: 'req_1' }; }, }); // Stands in for `screen` / `wait`: the caller supplies the continuation, so - // the generic route is the intended door (descriptor defaults to 'any'). + // the generic route is the intended door. Declared explicitly, exactly as the + // four pausing built-ins declare it — since #5561 there is no default to + // inherit, and an omission means the opposite of this (see `undeclared_pause`). engine.registerNodeExecutor({ type: 'open_pause', descriptor: defineActionDescriptor({ - type: 'open_pause', version: '1.0.0', name: 'Open Pause', supportsPause: true, + type: 'open_pause', version: '1.0.0', name: 'Open Pause', + supportsPause: true, resumeAuthority: 'any', }), async execute() { return { success: true, suspend: true }; }, }); @@ -143,9 +154,12 @@ describe('resume authorization gate (#3801)', () => { expect(downstream).toEqual(['after']); }); - it('a node type with no published descriptor stays ungated', async () => { - // `registerNodeExecutor` without a descriptor — the gate has nothing - // declaring itself service-owned, so behaviour is unchanged. + it('refuses a node type that published no descriptor at all (#5561)', async () => { + // `registerNodeExecutor` without a descriptor declares NOTHING — not even + // `supportsPause` — so it is the loudest instance of the shape step two + // closes, and the one neither the registration warning nor + // `check:resume-authority-declared` can see (both key on a descriptor + // literal). Before the flip this resumed and ran `after`. engine.registerNodeExecutor({ type: 'bare_pause', async execute() { return { success: true, suspend: true }; }, @@ -153,8 +167,82 @@ describe('resume authorization gate (#3801)', () => { engine.registerFlow('bare_flow', pauseFlow('bare_flow', 'bare_pause') as never); const paused = await engine.execute('bare_flow'); - expect((await engine.resume(paused.runId!)).success).toBe(true); - expect(downstream).toEqual(['after']); + const refused = await engine.resume(paused.runId!); + expect(refused.success).toBe(false); + expect(refused.code).toBe('PERMISSION_DENIED'); + expect(refused.error).toMatch(/never declares resumeAuthority/); + expect(downstream).toEqual([]); + // Refused, not consumed — the pause survives for a legitimate continuation. + expect(engine.listSuspendedRuns()).toHaveLength(1); + }); + + // ── an undeclared pausing type: fail-closed, and the fix that lifts it ── + // + // The end-to-end shape #5561 step two exists for. Before the flip the + // descriptor below (`supportsPause: true`, no `resumeAuthority`) inherited the + // schema default and was raw-resumable; a third-party pausing node shipped + // fail-open unless its author remembered a field nothing forced them to write. + + describe('a pausing type that never declares resumeAuthority (#5561)', () => { + /** The descriptor shape the whole issue is about, parameterised by authority. */ + function registerUndeclared(e: AutomationEngine, authority?: 'any' | 'service'): void { + e.registerNodeExecutor({ + type: 'undeclared_pause', + descriptor: defineActionDescriptor({ + type: 'undeclared_pause', version: '1.0.0', name: 'Undeclared Pause', + supportsPause: true, ...(authority ? { resumeAuthority: authority } : {}), + }), + async execute() { return { success: true, suspend: true }; }, + }); + e.registerFlow('undeclared_flow', pauseFlow('undeclared_flow', 'undeclared_pause') as never); + } + + it('refuses the generic resume, naming the omission and the one-line fix', async () => { + registerUndeclared(engine); + const paused = await engine.execute('undeclared_flow'); + expect(paused.status).toBe('paused'); + + const refused = await engine.resume(paused.runId!, { variables: { new_assignee: 'ada' } }); + + expect(refused.success).toBe(false); + expect(refused.code).toBe('PERMISSION_DENIED'); + // Distinct from the declared-'service' wording: this reader has to add a + // field to a descriptor, not route through somebody's service API. + expect(refused.error).toMatch(/never declares resumeAuthority/); + expect(refused.error).toContain("resumeAuthority: 'any'"); + expect(refused.error).not.toMatch(/only its owning service may resume/); + // Refused, not consumed. + expect(downstream).toEqual([]); + expect(engine.listSuspendedRuns()).toHaveLength(1); + }); + + it("declaring 'any' is the whole migration — the same flow then resumes", async () => { + // A fresh engine registering the SAME node type with the one field added: + // the prescription the refusal prints, applied verbatim. + const declared = makeEngine(downstream); + registerUndeclared(declared, 'any'); + const paused = await declared.execute('undeclared_flow'); + + const resumed = await declared.resume(paused.runId!, { variables: { new_assignee: 'ada' } }); + + expect(resumed.success).toBe(true); + expect(resumed.code).toBeUndefined(); + expect(downstream).toEqual(['after']); + expect(declared.listSuspendedRuns()).toHaveLength(0); + }); + + it('is still resumable by an owning service while undeclared — closed, not bricked', async () => { + // Fail-closed must not mean "unrecoverable": the in-process marker still + // continues the run, so a host that knows what it is doing is not stuck + // waiting on a plugin release. + registerUndeclared(engine); + const paused = await engine.execute('undeclared_flow'); + + const ok = await engine.resume(paused.runId!, { [RESUME_AUTHORITY_SERVICE]: true }); + + expect(ok.success).toBe(true); + expect(downstream).toEqual(['after']); + }); }); it('gates a flow authored against a deprecated ALIAS of the gated type', async () => { diff --git a/packages/services/service-automation/src/run-history.test.ts b/packages/services/service-automation/src/run-history.test.ts index 21a9b4779f..c7792c4882 100644 --- a/packages/services/service-automation/src/run-history.test.ts +++ b/packages/services/service-automation/src/run-history.test.ts @@ -12,6 +12,19 @@ import type { StepLogEntry, NodeExecutor } from './engine.js'; import { registerLoopNode } from './builtin/loop-node.js'; import { InMemorySuspendedRunStore } from './suspended-run-store.js'; import type { AutomationContext } from '@objectstack/spec/contracts'; +import { defineActionDescriptor } from '@objectstack/spec/automation'; + +/** + * `resumeAuthority: 'any'` is required of a pausing fixture since #5561: these + * tests continue their pause through the public `resume` door, which a node type + * now opts into rather than inherits. Nothing here is about the resume gate + * (`resume-authority-gate.test.ts` owns that), so the fixture states the posture + * it relies on — the same declaration the pausing built-ins carry. + */ +const HOLD_DESCRIPTOR = defineActionDescriptor({ + type: 'hold', version: '1.0.0', name: 'Hold', + supportsPause: true, resumeAuthority: 'any', +}); const silent = { info() {}, warn() {}, error() {}, debug() {} } as never; @@ -145,6 +158,7 @@ describe('automation run history (durable observability)', () => { const holdExecutor = { type: 'hold', + descriptor: HOLD_DESCRIPTOR, async execute() { return { success: true, suspend: true, correlation: 'held' }; }, } as never; diff --git a/packages/services/service-automation/src/run-summary.test.ts b/packages/services/service-automation/src/run-summary.test.ts index 1b4ed7cb3a..e6251dbaf8 100644 --- a/packages/services/service-automation/src/run-summary.test.ts +++ b/packages/services/service-automation/src/run-summary.test.ts @@ -22,6 +22,19 @@ import { registerMapNode } from './builtin/map-node.js'; import { registerHttpNodes } from './builtin/http-nodes.js'; import { registerConnectorNodes } from './builtin/connector-nodes.js'; import type { AutomationContext } from '@objectstack/spec/contracts'; +import { defineActionDescriptor } from '@objectstack/spec/automation'; + +/** + * `resumeAuthority: 'any'` is required of a pausing fixture since #5561: these + * tests continue their pause through the public `resume` door, which a node type + * now opts into rather than inherits. Nothing here is about the resume gate + * (`resume-authority-gate.test.ts` owns that), so the fixture states the posture + * it relies on — the same declaration the pausing built-ins carry. + */ +const HOLD_DESCRIPTOR = defineActionDescriptor({ + type: 'hold', version: '1.0.0', name: 'Hold', + supportsPause: true, resumeAuthority: 'any', +}); const AT = '2026-07-31T00:00:00.000Z'; @@ -590,6 +603,7 @@ describe('a child run that PAUSED still counts toward its parent', () => { registerSubflowNode(engine, ctx); engine.registerNodeExecutor({ type: 'hold', + descriptor: HOLD_DESCRIPTOR, async execute() { return { success: true, suspend: true, correlation: 'held' }; }, } as NodeExecutor); // A child that pauses, then writes a row once resumed. diff --git a/packages/services/service-automation/src/suspended-run-store.test.ts b/packages/services/service-automation/src/suspended-run-store.test.ts index 6c78dce212..2800278296 100644 --- a/packages/services/service-automation/src/suspended-run-store.test.ts +++ b/packages/services/service-automation/src/suspended-run-store.test.ts @@ -4,6 +4,19 @@ import { describe, it, expect } from 'vitest'; import { AutomationEngine } from './engine.js'; import { ObjectStoreSuspendedRunStore, type SuspendedRunStoreEngine } from './suspended-run-store.js'; import type { RunRecord, SuspendedRun } from './engine.js'; +import { defineActionDescriptor } from '@objectstack/spec/automation'; + +/** + * The `resumeAuthority: 'any'` declaration every pausing fixture below needs + * since #5561: these tests continue their pause through the public `resume` + * door, and a node type that declares nothing is refused there. None of them is + * about the resume gate (that is `resume-authority-gate.test.ts`), so each + * states the posture it relies on — as the four pausing built-ins do. + */ +const PAUSE_NODE_DESCRIPTOR = defineActionDescriptor({ + type: 'pause_node', version: '1.0.0', name: 'Pause Node', + supportsPause: true, resumeAuthority: 'any', +}); function createTestLogger() { return { info: () => {}, warn: () => {}, error: () => {}, debug: () => {}, child: () => createTestLogger() } as any; @@ -119,6 +132,7 @@ describe('ObjectStoreSuspendedRunStore', () => { const e = new AutomationEngine(createTestLogger(), new ObjectStoreSuspendedRunStore(engine, createTestLogger())); e.registerNodeExecutor({ type: 'pause_node', + descriptor: PAUSE_NODE_DESCRIPTOR, async execute() { return { success: true, suspend: true, correlation: 'areq_1' }; }, }); e.registerNodeExecutor({ @@ -169,6 +183,7 @@ function pausableEngine(store?: any, logger = createTestLogger()) { const e = new AutomationEngine(logger, store); e.registerNodeExecutor({ type: 'pause_node', + descriptor: PAUSE_NODE_DESCRIPTOR, async execute() { return { success: true, suspend: true, correlation: 'areq_1' }; }, }); e.registerFlow('approval_flow', { diff --git a/packages/services/service-automation/src/suspension-release.test.ts b/packages/services/service-automation/src/suspension-release.test.ts index 8f63ba5493..b9a77fe44a 100644 --- a/packages/services/service-automation/src/suspension-release.test.ts +++ b/packages/services/service-automation/src/suspension-release.test.ts @@ -5,6 +5,7 @@ import { AutomationEngine } from './engine.js'; import type { NodeExecutor, SuspensionRelease } from './engine.js'; import { InMemorySuspendedRunStore } from './suspended-run-store.js'; import { registerSubflowNode } from './builtin/subflow-node.js'; +import { defineActionDescriptor } from '@objectstack/spec/automation'; /** * `NodeExecutor.onSuspensionReleased` — the engine side of #5512. @@ -34,10 +35,26 @@ function ctx() { return { logger: silentLogger(), getService() { throw new Error('none'); } } as any; } +/** + * The declaration every pausing fixture in this file needs since #5561: these + * tests continue their pause through the public `resume` door, which a node type + * now has to opt into with `resumeAuthority: 'any'` — a type that declares + * nothing is refused there. Nothing here is about the resume gate itself (see + * `resume-authority-gate.test.ts`), so each fixture states the posture it relies + * on, exactly as the four pausing built-ins do. + */ +function openPauser(type: string) { + return defineActionDescriptor({ + type, version: '1.0.0', name: type, + supportsPause: true, resumeAuthority: 'any', + }); +} + /** A pausing executor that records every release it is told about. */ function recordingPauser(type: string, seen: SuspensionRelease[]): NodeExecutor { return { type, + descriptor: openPauser(type), async execute(node) { // A correlation stands in for "the handle on whatever I armed on entry". return { success: true, suspend: true, correlation: `test-armature:${node.id}` }; @@ -130,6 +147,7 @@ describe('NodeExecutor.onSuspensionReleased (#5512)', () => { it('is silent for a pausing executor that implements no teardown', async () => { engine.registerNodeExecutor({ type: 'bare_pauser', + descriptor: openPauser('bare_pauser'), async execute() { return { success: true, suspend: true }; }, } as NodeExecutor); engine.registerFlow('pause_flow', pauseFlow('bare_pauser')); @@ -175,6 +193,7 @@ describe('NodeExecutor.onSuspensionReleased (#5512)', () => { loud.registerNodeExecutor(markerExecutor(ranLoud)); loud.registerNodeExecutor({ type: 'pauser', + descriptor: openPauser('pauser'), async execute(node) { return { success: true, suspend: true, correlation: `test-armature:${node.id}` }; }, async onSuspensionReleased() { throw new Error('job service exploded'); }, } as NodeExecutor); @@ -245,6 +264,7 @@ describe('onSuspensionReleased — the `failed` route (subflow ancestor)', () => engine.registerNodeExecutor({ type: 'pauser', + descriptor: openPauser('pauser'), async execute() { return { success: true, suspend: true }; }, } as NodeExecutor); engine.registerNodeExecutor({ diff --git a/packages/spec/spec-changes.json b/packages/spec/spec-changes.json index e856a8b87c..b3a2ca4d7a 100644 --- a/packages/spec/spec-changes.json +++ b/packages/spec/spec-changes.json @@ -692,6 +692,13 @@ "migrationId": "etl-pipeline-layer-retired", "toMajor": 17, "rationale": "The reading #4738 used to retire L1 `DataSyncConfig`, re-measured one layer up and identical: narrative-only. No engine ever parsed, scheduled or executed an `ETLPipeline`. Measured on origin/main immediately before the removal: the only non-spec references in this repo are two fumadocs-generated documentation sources (`apps/docs/.source/*.ts`), not executors; objectui has no reference at all; there is no `liveness/etl.json` or `pipeline.json`, so no ADR-0049 gate ever had a reading on it — while the same file family's EXECUTED half does have one (`liveness/mapping.json`), which is the contrast that makes the absence meaningful rather than an oversight. The `etl` string in this registry was the one untested link the finding named, and it is not a loader path: it was the id of the #4962 retry-vocabulary entry, absorbed here. The layer was ADR-0078's asymmetry in its purest form — an author could write a complete ten-stage pipeline, get no error, and get no execution. It was also advertised: `packages/spec/docs/SYNC_ARCHITECTURE.md` named `ETLPipeline` as the recommended destination for authors displaced by the L1 retirement (#4738) and listed ten transformation types with copyable examples down to `script | Custom JavaScript/Python`. That document is rewritten in the same change; a retirement whose own doc still recommends the retired layer is self-contradictory, and forwarding L1's authors to a second layer with no executor was the defect compounding rather than closing. ⚠️ `etl-retry-converged-onto-retry-policy` (#4962) is SUBSUMED here, the #4657/#4834/#5055 way: both land in the unreleased protocol 17, so composed, a rename of `retry.maxAttempts` on a shape that does not survive the major has no observable effect — and keeping both would tell an upgrader to rewrite a key on a schema the same upgrade deletes. The `maxAttempts` `retiredKey()` tombstone goes with the shape that carried it, which is strictly stronger than the tombstone: there is no longer a `retry` block to author the key into. Route 3 — no carrier key, no parse site, so no D2 conversion and no tombstone; RETIRED_DEFS_BY_MAJOR plus this entry are the declaration. ADR-0049, ADR-0078, #6414." + }, + { + "surface": "automation.ActionDescriptor.resumeAuthority — an OMITTED value on a pausing node descriptor (supportsPause: true, or any executor whose execute() returns suspend: true)", + "replacement": "an explicit resumeAuthority: 'any' on the descriptor, for a pausing node whose pauses really are meant to be continued through the generic resume route (POST /automation/:name/runs/:runId/resume) — a screen-style collected-input pause, or a signal wait an external producer resumes. Declare 'service' instead if continuing is the tail of a decision your own service must authorize and record first. Either value is a one-line addition; only the silence changed meaning", + "migrationId": "action-descriptor-resume-authority-default-flip", + "toMajor": 17, + "rationale": "A SECURE-DEFAULT FLIP with no metadata shape to rewrite — the same category as protocol 12's `rest-requireauth-default-flip`, and it is registered here for the same reason: whether a given pause is genuinely open to the generic route is a trust judgment no transform can make. The #3801 resume gate keys on the SUSPENDED NODE, and `ActionDescriptor.resumeAuthority` used to default to `'any'`, so a pausing node type shipped raw-resumable unless its author remembered the field. It now resolves to `'service'` when absent: an unclaimed pause is refused on the generic route with `PERMISSION_DENIED` / 403 until its descriptor states who may continue it. #3823 is the incident that decided the direction — 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 demonstrated cost was an unaudited resubmit plus a destroyed remote run. The two possible mistakes are asymmetric, which is the whole argument: guessing `'any'` walks past a decision nothing recorded and is silent, while guessing `'service'` returns a refusal naming the missing field. ⚠️ The surface is a DESCRIPTOR FIELD set in plugin CODE, never stack metadata, so there is no source for a D2 conversion to rewrite and deliberately no schema tombstone — the disposition `data-driver-find-stream-retired` (#4484), `storage-service-list-retired` (#5540) and `actor-user-roles-to-positions` (#6011) already carry. It differs from those in one way a reader should not have to infer: nothing is REMOVED, so tsc reports nothing at all — the field was already optional after step one and an omission still compiles. The enforced channels are all run-time: a registration warning naming the node type (once per type per engine), the refusal message on the resume itself, and `check:resume-authority-declared` for executors living in this repo. For a third-party plugin the generated upgrade guide is the only channel that arrives BEFORE a user hits a run that will not continue. In-tree the flip moves nothing: all six shipped pausing types (screen, wait, subflow, map, approval, approval_revise) declare their authority explicitly. ADR-0044 amendment (2026-07-28) and its 2026-08-08 landing section, ADR-0019 #3801 addendum, #5561." } ], "removed": [] @@ -1443,6 +1450,13 @@ "migrationId": "etl-pipeline-layer-retired", "toMajor": 17, "rationale": "The reading #4738 used to retire L1 `DataSyncConfig`, re-measured one layer up and identical: narrative-only. No engine ever parsed, scheduled or executed an `ETLPipeline`. Measured on origin/main immediately before the removal: the only non-spec references in this repo are two fumadocs-generated documentation sources (`apps/docs/.source/*.ts`), not executors; objectui has no reference at all; there is no `liveness/etl.json` or `pipeline.json`, so no ADR-0049 gate ever had a reading on it — while the same file family's EXECUTED half does have one (`liveness/mapping.json`), which is the contrast that makes the absence meaningful rather than an oversight. The `etl` string in this registry was the one untested link the finding named, and it is not a loader path: it was the id of the #4962 retry-vocabulary entry, absorbed here. The layer was ADR-0078's asymmetry in its purest form — an author could write a complete ten-stage pipeline, get no error, and get no execution. It was also advertised: `packages/spec/docs/SYNC_ARCHITECTURE.md` named `ETLPipeline` as the recommended destination for authors displaced by the L1 retirement (#4738) and listed ten transformation types with copyable examples down to `script | Custom JavaScript/Python`. That document is rewritten in the same change; a retirement whose own doc still recommends the retired layer is self-contradictory, and forwarding L1's authors to a second layer with no executor was the defect compounding rather than closing. ⚠️ `etl-retry-converged-onto-retry-policy` (#4962) is SUBSUMED here, the #4657/#4834/#5055 way: both land in the unreleased protocol 17, so composed, a rename of `retry.maxAttempts` on a shape that does not survive the major has no observable effect — and keeping both would tell an upgrader to rewrite a key on a schema the same upgrade deletes. The `maxAttempts` `retiredKey()` tombstone goes with the shape that carried it, which is strictly stronger than the tombstone: there is no longer a `retry` block to author the key into. Route 3 — no carrier key, no parse site, so no D2 conversion and no tombstone; RETIRED_DEFS_BY_MAJOR plus this entry are the declaration. ADR-0049, ADR-0078, #6414." + }, + { + "surface": "automation.ActionDescriptor.resumeAuthority — an OMITTED value on a pausing node descriptor (supportsPause: true, or any executor whose execute() returns suspend: true)", + "replacement": "an explicit resumeAuthority: 'any' on the descriptor, for a pausing node whose pauses really are meant to be continued through the generic resume route (POST /automation/:name/runs/:runId/resume) — a screen-style collected-input pause, or a signal wait an external producer resumes. Declare 'service' instead if continuing is the tail of a decision your own service must authorize and record first. Either value is a one-line addition; only the silence changed meaning", + "migrationId": "action-descriptor-resume-authority-default-flip", + "toMajor": 17, + "rationale": "A SECURE-DEFAULT FLIP with no metadata shape to rewrite — the same category as protocol 12's `rest-requireauth-default-flip`, and it is registered here for the same reason: whether a given pause is genuinely open to the generic route is a trust judgment no transform can make. The #3801 resume gate keys on the SUSPENDED NODE, and `ActionDescriptor.resumeAuthority` used to default to `'any'`, so a pausing node type shipped raw-resumable unless its author remembered the field. It now resolves to `'service'` when absent: an unclaimed pause is refused on the generic route with `PERMISSION_DENIED` / 403 until its descriptor states who may continue it. #3823 is the incident that decided the direction — 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 demonstrated cost was an unaudited resubmit plus a destroyed remote run. The two possible mistakes are asymmetric, which is the whole argument: guessing `'any'` walks past a decision nothing recorded and is silent, while guessing `'service'` returns a refusal naming the missing field. ⚠️ The surface is a DESCRIPTOR FIELD set in plugin CODE, never stack metadata, so there is no source for a D2 conversion to rewrite and deliberately no schema tombstone — the disposition `data-driver-find-stream-retired` (#4484), `storage-service-list-retired` (#5540) and `actor-user-roles-to-positions` (#6011) already carry. It differs from those in one way a reader should not have to infer: nothing is REMOVED, so tsc reports nothing at all — the field was already optional after step one and an omission still compiles. The enforced channels are all run-time: a registration warning naming the node type (once per type per engine), the refusal message on the resume itself, and `check:resume-authority-declared` for executors living in this repo. For a third-party plugin the generated upgrade guide is the only channel that arrives BEFORE a user hits a run that will not continue. In-tree the flip moves nothing: all six shipped pausing types (screen, wait, subflow, map, approval, approval_revise) declare their authority explicitly. ADR-0044 amendment (2026-07-28) and its 2026-08-08 landing section, ADR-0019 #3801 addendum, #5561." } ], "removed": [] diff --git a/packages/spec/src/automation/node-executor.zod.ts b/packages/spec/src/automation/node-executor.zod.ts index 0d1ce84625..9bf0c8742e 100644 --- a/packages/spec/src/automation/node-executor.zod.ts +++ b/packages/spec/src/automation/node-executor.zod.ts @@ -343,7 +343,8 @@ export const ActionDescriptorSchema = lazySchema(() => z.object({ * * - `'any'` — the caller supplies the continuation and the route is the * intended door: a `screen` node's collected inputs, a `wait` node's - * external signal. + * external signal. **A pausing node must opt into this explicitly**; see + * the omission semantics below. * - `'service'` — resuming is a SIDE EFFECT of a decision some service must * authorize and record first, so only that service may drive it. An * `approval` node declares this: `ApprovalService.decide` enforces the @@ -356,28 +357,39 @@ export const ActionDescriptorSchema = lazySchema(() => z.object({ * unless the signal carries the in-process `RESUME_AUTHORITY_SERVICE` * marker — a symbol, so a JSON body can never carry it. * - * **Carrying no `.default()` is the point** (#5561). A default would make - * "the author decided `'any'`" and "the author never considered it" the same - * value by the time any consumer sees the descriptor: Zod fills the key - * inside {@link defineActionDescriptor}, so the omission becomes - * unrecoverable one function call after it happens — measured, not assumed - * (the two parses are byte-identical). That erasure is how #3823 shipped: - * ADR-0044 pointed a revise edge at a generic `wait`, `wait` is legitimately - * `'any'`, and a pause standing in a service-owned position inherited a - * fail-open value nobody had chosen. Absent therefore means absent, and two - * seams read it: `AutomationEngine.registerNodeExecutor` warns once per node - * type when a `supportsPause` descriptor omits it, and - * `check:resume-authority-declared` fails CI on an omission in this repo's - * own executors. + * **Omitting it means `'service'` — fail-closed, not `'any'`** (ADR-0044's + * 2026-07-28 amendment, landed in two steps on #5561). A pausing node type + * that never states who may continue its pauses is closed to the generic + * resume route until its author states it: `AutomationEngine` resolves an + * absent value to `'service'` (`resolveResumeAuthority`), so a raw resume of + * such a pause answers `PERMISSION_DENIED` with a message naming the + * one-line fix. **Registering a pausing node whose pause really is open to + * the route? Declare `resumeAuthority: 'any'` — it is an opt-in now, not an + * inheritance.** * - * Runtime semantics are unchanged by that: the engine still resolves an - * absent value to `'any'` (`resolveResumeAuthority`), so omitting it is - * fail-open exactly as before — loudly now instead of silently. Flipping - * that fallback to `'service'` (fail-closed by omission) is the breaking - * half, still tracked on #5561 for a version window that allows it. + * The field carries no Zod `.default()`, and that is what makes the rule + * expressible at all (step one, #5561). A default would make "the author + * decided `'any'`" and "the author never considered it" the same value by the + * time any consumer sees the descriptor: Zod fills the key inside + * {@link defineActionDescriptor}, so the omission became unrecoverable one + * function call after it happened — measured, not assumed (the two parses + * were byte-identical). That erasure is how #3823 shipped: ADR-0044 pointed a + * revise edge at a generic `wait`, `wait` is legitimately `'any'`, and a + * pause standing in a service-owned position inherited a fail-open value + * nobody had chosen. Absent now means absent, and three seams read it — + * `AutomationEngine.registerNodeExecutor` warns once per node type when a + * `supportsPause` descriptor omits it, the resume gate refuses the pauses it + * produces, and `check:resume-authority-declared` fails CI on an omission in + * this repo's own executors. + * + * The guess is made in the loud direction on purpose. Guessing `'any'` for an + * unclaimed pause continues a run past a decision nothing recorded and says + * nothing; guessing `'service'` refuses a resume and hands back the + * declaration that fixes it. Only one of those two mistakes is discoverable + * by the person who made it. */ resumeAuthority: z.enum(['any', 'service']).optional() - .describe("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)"), + .describe("Who may resume a run this node suspended: 'any' (the generic resume route) or 'service' (only the owning service, e.g. approvals). Carries no schema default so an omission stays observable — and an omission is fail-CLOSED at run time, equivalent to 'service': a pausing node whose pause is open to the generic route must declare 'any' explicitly (#5561)"), /** * Runtime maturity of the capability behind this descriptor (ADR-0041 §4). diff --git a/packages/spec/src/contracts/automation-service.ts b/packages/spec/src/contracts/automation-service.ts index edbe48d3d3..e7f21aa00d 100644 --- a/packages/spec/src/contracts/automation-service.ts +++ b/packages/spec/src/contracts/automation-service.ts @@ -185,7 +185,9 @@ export interface AutomationResult { * * - `'forbidden'` — {@link IAutomationService.resume} refused because the * run is parked on a node whose descriptor declares - * `resumeAuthority: 'service'` (#3801). A transport maps it to **403**. + * `resumeAuthority: 'service'` — or declares no `resumeAuthority` at all, + * which resolves the same way (#3801, #5561). A transport maps it to + * **403**. * - `'invalid_signal'` — the resume signal tried to write variables the * flow engine reserves for itself (a `$…` name, or one carrying a `.$` * segment: `$runId`, `.$mapItemDone`, …). A transport maps it to @@ -261,7 +263,9 @@ export interface AutomationResult { * the tail of a decision it already authorized and recorded (#3801). * * A run parked on a node whose descriptor declares `resumeAuthority: 'service'` - * (today: `approval`) is resumable ONLY with this marker present. It is a + * (today: `approval`, `approval_revise`) is resumable ONLY with this marker + * present — as is a pause whose node type declares no `resumeAuthority` at all, + * fail-closed since #5561. It is a * symbol on purpose: the generic resume route builds its signal out of a JSON * body, and no JSON body can produce a symbol-keyed property — so the marker * is unforgeable from outside the process, while an in-process owner @@ -297,9 +301,10 @@ export interface ResumeSignal { variables?: Record; /** * Set by the service that OWNS the suspension to clear the resume gate on - * a `resumeAuthority: 'service'` node (#3801). See + * a `resumeAuthority: 'service'` node — or on one that declares no + * authority, which is gated the same way since #5561 (#3801). See * {@link RESUME_AUTHORITY_SERVICE} — unforgeable from an HTTP body, and - * ignored entirely on `'any'` nodes. + * ignored entirely on nodes that declare `'any'`. */ [RESUME_AUTHORITY_SERVICE]?: true; } @@ -478,6 +483,12 @@ export interface IAutomationService { * to the child the signal would actually land on, so a parent parked on a * `subflow` node is no way around it. * + * **A node type that declares NO `resumeAuthority` is gated identically** + * (#5561): the generic route is an opt-in a descriptor declares with + * `'any'`, not a default it inherits. The refusal names the omission and + * the one-line declaration that lifts it, so this is recoverable by the + * plugin author rather than only by the platform. + * * @param runId - The paused run's id * @param signal - Optional output to merge and/or branch label to follow * @returns The result of continuing the run (may itself be `'paused'` again) diff --git a/packages/spec/src/migrations/registry.ts b/packages/spec/src/migrations/registry.ts index 882687ca4a..106e3ff569 100644 --- a/packages/spec/src/migrations/registry.ts +++ b/packages/spec/src/migrations/registry.ts @@ -1128,7 +1128,17 @@ const step17: MigrationStep = { + 'such a measure never produced a number. `QueryAST.aggregations[].function` is a request ' + 'surface with no stored source — one semantic TODO below. The mongodb and in-memory ' + 'backends that implemented these two are inside the #5499 freeze and are untouched; their ' - + 'code is simply no longer reachable through a spec-valid request.', + + 'code is simply no longer reachable through a spec-valid request.\n\n' + + 'One entry in this step is not a removal at all but a SECURE-DEFAULT FLIP, the shape ' + + "protocol 12 last used for `api.requireAuth`: an omitted `ActionDescriptor.resumeAuthority` " + + "resolves to `'service'` instead of `'any'`, so a pausing node type that never states who " + + 'may continue its pauses is refused on the generic resume route rather than open to it ' + + '(#5561, ADR-0044\'s 2026-07-28 amendment). Nothing is removed and no metadata shape ' + + 'changes — the field has been optional since step one of the same issue — so tsc reports ' + + 'nothing and only the MEANING of silence moved. That is exactly why it needs a ledger ' + + 'entry: a third-party plugin author has no compile error to discover it with, and the ' + + 'one-line prescription (declare `resumeAuthority` on the descriptor) has to arrive before ' + + 'a user meets a run that will not continue.', conversionIds: [ 'action-execute-to-target', 'field-conditionalRequired-to-requiredWhen', @@ -2679,6 +2689,64 @@ const step17: MigrationStep = { + '`syncConfig` and an import declaring `mapping.transform` both behave exactly as they ' + 'did in 16.x.', }, + { + id: 'action-descriptor-resume-authority-default-flip', + // No backticks in `surface` — build-upgrade-guide.ts renders it inside a code + // span AND a table cell (see the note on `spec-type-alias-input-suffix-retired`). + surface: + 'automation.ActionDescriptor.resumeAuthority — an OMITTED value on a pausing ' + + 'node descriptor (supportsPause: true, or any executor whose execute() returns ' + + 'suspend: true)', + replacement: + "an explicit resumeAuthority: 'any' on the descriptor, for a pausing node whose " + + 'pauses really are meant to be continued through the generic resume route ' + + '(POST /automation/:name/runs/:runId/resume) — a screen-style collected-input ' + + "pause, or a signal wait an external producer resumes. Declare 'service' instead " + + 'if continuing is the tail of a decision your own service must authorize and ' + + 'record first. Either value is a one-line addition; only the silence changed ' + + 'meaning', + reason: + 'A SECURE-DEFAULT FLIP with no metadata shape to rewrite — the same category as ' + + "protocol 12's `rest-requireauth-default-flip`, and it is registered here for the " + + 'same reason: whether a given pause is genuinely open to the generic route is a ' + + 'trust judgment no transform can make. The #3801 resume gate keys on the SUSPENDED ' + + "NODE, and `ActionDescriptor.resumeAuthority` used to default to `'any'`, so a " + + 'pausing node type shipped raw-resumable unless its author remembered the field. ' + + "It now resolves to `'service'` when absent: an unclaimed pause is refused on the " + + 'generic route with `PERMISSION_DENIED` / 403 until its descriptor states who may ' + + 'continue it. #3823 is the incident that decided the direction — 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 demonstrated cost was an unaudited resubmit plus a destroyed remote ' + + 'run. The two possible mistakes are asymmetric, which is the whole argument: ' + + "guessing `'any'` walks past a decision nothing recorded and is silent, while " + + "guessing `'service'` returns a refusal naming the missing field. ⚠️ The surface " + + 'is a DESCRIPTOR FIELD set in plugin CODE, never stack metadata, so there is no ' + + 'source for a D2 conversion to rewrite and deliberately no schema tombstone — the ' + + 'disposition `data-driver-find-stream-retired` (#4484), `storage-service-list-retired` ' + + '(#5540) and `actor-user-roles-to-positions` (#6011) already carry. It differs from ' + + 'those in one way a reader should not have to infer: nothing is REMOVED, so tsc ' + + 'reports nothing at all — the field was already optional after step one and an ' + + 'omission still compiles. The enforced channels are all run-time: a registration ' + + 'warning naming the node type (once per type per engine), the refusal message on ' + + 'the resume itself, and `check:resume-authority-declared` for executors living in ' + + 'this repo. For a third-party plugin the generated upgrade guide is the only ' + + 'channel that arrives BEFORE a user hits a run that will not continue. In-tree the ' + + 'flip moves nothing: all six shipped pausing types (screen, wait, subflow, map, ' + + 'approval, approval_revise) declare their authority explicitly. ADR-0044 amendment ' + + '(2026-07-28) and its 2026-08-08 landing section, ADR-0019 #3801 addendum, #5561.', + acceptanceCriteria: + 'Every action descriptor your plugin registers for a node type that can suspend ' + + 'declares `resumeAuthority`. Booting the stack logs no `declares supportsPause but ' + + 'never declares resumeAuthority` warning naming one of your types, and a run parked ' + + 'on each of your pausing nodes can still be continued the way you intend: a resume ' + + "through the generic route succeeds for the ones you declared `'any'`, and answers " + + "403 (`PERMISSION_DENIED`) for the ones you declared `'service'`, which continue " + + 'through your own service API instead. ⚠️ `supportsPause` is a declaration nothing ' + + 'enforces (#5703), so an executor whose `execute()` returns `suspend: true` while ' + + 'leaving `supportsPause` false is warned about by NEITHER channel — check those by ' + + 'hand against the same rule.', + }, ], }; diff --git a/scripts/check-resume-authority-declared.mjs b/scripts/check-resume-authority-declared.mjs index 51da55b0c4..95d79dfdad 100644 --- a/scripts/check-resume-authority-declared.mjs +++ b/scripts/check-resume-authority-declared.mjs @@ -26,8 +26,15 @@ // Until #5561 the omission was not merely unnoticed, it was UNOBSERVABLE: // `ActionDescriptorSchema.resumeAuthority` carried `.default('any')`, so // `defineActionDescriptor` filled the key and produced a descriptor -// byte-identical to an explicit `'any'`. #5561 removed that default, which is -// what lets both this gate and the engine's registration warning exist. +// byte-identical to an explicit `'any'`. #5561 step one removed that default, +// which is what lets both this gate and the engine's registration warning exist. +// +// #5561 step two then flipped what an omission MEANS at run time: an undeclared +// pausing type resolves to `'service'`, so the generic route refuses its pauses +// instead of walking them. That raises this gate's stakes rather than changing +// its rule -- an omission that used to ship a silent hole now ships a node whose +// runs cannot be continued by the route its author probably intended. The rule +// is still "state your intent"; the finding text names the new consequence. // // ## Why a repo gate ALONGSIDE the registration warning // @@ -57,17 +64,22 @@ // 1. `supportsPause` is itself a declaration no execution path enforces // (objectstack#5703): a run pauses because the executor's `execute()` // returned `suspend: true`. An executor that suspends while leaving -// `supportsPause` false is fail-open and invisible BOTH here and to the -// registration warning. Keying on the author's own literal is what makes -// this gate decidable without a call graph; #5703 tracks the runtime half. +// `supportsPause` false is invisible BOTH here and to the registration +// warning -- and since #5561 step two its pauses are refused with neither +// gate nor warning having said a word first. The refusal message carries the +// same prescription for exactly that reader. Keying on the author's own +// literal is what makes this gate decidable without a call graph; #5703 +// tracks the runtime half. // 1b. **Test fixtures are deliberately out of scope.** The subject here is the // SHIPPED node vocabulary — descriptors a real engine registers in a real // deployment. A fixture registers into a throwaway engine and ships to // nobody, and more decisively: an undeclared pausing descriptor is exactly // what the registration warning's own tests must construct -// (`resume-authority-declaration.test.ts`), so gating fixtures would make -// the mechanism this gate co-exists with untestable. Three such fixtures -// exist today and all three are deliberate. (Note this is the mirror of +// (`resume-authority-declaration.test.ts`), and since step two so must the +// resume gate's own tests (`resume-authority-gate.test.ts`, which pins that +// an undeclared pause is refused). Gating fixtures would make both +// mechanisms untestable; every such fixture in the tree is deliberate and +// constructs the omission on purpose. (Note this is the mirror of // `check-engine-double-contract.mjs`, which scans ONLY tests: each gate's // scope is its subject, not a repo-wide sweep.) // 2. Descriptors assembled dynamically -- spread from a variable, built by a @@ -233,13 +245,13 @@ function audit(scanRoots = DEFAULT_SCAN_ROOTS) { const named = d.type ? `'${d.type}'` : `the descriptor at line ${d.line}`; errors.push( `DECLARED: ${file}:${d.line} — node type ${named} declares supportsPause: true but never ` - + 'declares resumeAuthority, so the #3801 resume gate treats every pause it creates as ' - + 'raw-resumable through the generic resume route. Add the field to the descriptor: ' - + "resumeAuthority: '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 (an approval). Both values are accepted here ' - + '— only the silence is the finding, because an inherited value is one nobody chose, which ' - + 'is how #3823 walked past an unrecorded approval decision.', + + 'declares resumeAuthority, so the #3801 resume gate REFUSES every pause it creates on the ' + + 'generic resume route (an unclaimed pause is fail-closed since #5561 step two). Add the ' + + "field to the descriptor: resumeAuthority: '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 (an approval). Both ' + + 'values are accepted here — only the silence is the finding, because an inherited value is ' + + 'one nobody chose, which is how #3823 walked past an unrecorded approval decision.', ); } }