From b49457e0fa6bc5c64d5a1197333d8938bf6caf70 Mon Sep 17 00:00:00 2001 From: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Date: Mon, 27 Jul 2026 10:51:57 +0800 Subject: [PATCH] feat(approvals): expression approvers, empty-slate policy, decision outputs (#3447 P2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three declarative capabilities that complete dynamic approver routing: - `expression` approvers: CEL resolved at node entry over a CLOSED root set — current.* (live record), trigger.* (submit snapshot), vars.* (flow variables). `record`/bare fields are rejected before evaluation with errors that prescribe the correct spelling (the runtime env would resolve them as dyn → null → a silently-empty slate). resolveAs re-expands ids through the existing graph lookups; per_group keys each intermediate value as its own sign-off group. Missing key = loud error; only present-but-empty counts as an empty slate. - onEmptyApprovers: admin_rescue (default, = #3424) | fail | auto_approve. Auto-approve rides NodeExecutionResult.branchLabel, which existed but was never consumed on the synchronous completion path — the engine now honours it (unlabelled traversal would walk approve AND reject). - decisionOutputs: author-declared keys a decision may carry; accepted outputs resume as . variables, so a later node's expression reads vars..picked_departments — "previous approver picks the next step's approvers" with no record-field detour. Undeclared keys reject; decision/requestId reserved. Multi-approver tallies now always pin to the open-time snapshot (unanimous previously re-resolved per decision, which cannot work for expressions and could drift for live fields). collectCelRootIdentifiers is exported from @objectstack/formula and shared by the lint rules and the runtime pre-check so they can never drift. Resolution inputs are audited as __resolvedFrom on the request snapshot. Three new lint rules; SKILL.md and the approvals guide document the time-word contract (current/trigger/vars vs condition record/previous vs hook ctx). Co-Authored-By: Claude Fable 5 --- .changeset/expression-approvers-3447-p2.md | 18 + content/docs/automation/approvals.mdx | 57 ++- .../docs/references/automation/approval.mdx | 10 +- packages/formula/src/cel-engine.test.ts | 47 ++- packages/formula/src/cel-engine.ts | 50 +++ packages/formula/src/index.ts | 4 + packages/lint/src/index.ts | 3 + .../src/validate-approval-approvers.test.ts | 110 ++++++ .../lint/src/validate-approval-approvers.ts | 132 +++++++ .../src/approval-node.test.ts | 120 ++++++ .../plugin-approvals/src/approval-node.ts | 48 +++ .../src/approval-service.test.ts | 223 +++++++++++ .../plugin-approvals/src/approval-service.ts | 356 ++++++++++++++++-- .../plugins/plugin-approvals/src/index.ts | 3 + packages/rest/src/rest-server.ts | 4 + .../services/service-automation/src/engine.ts | 13 + packages/spec/src/automation/approval.zod.ts | 62 ++- .../spec/src/contracts/approval-service.ts | 10 + skills/objectstack-automation/SKILL.md | 76 +++- 19 files changed, 1310 insertions(+), 36 deletions(-) create mode 100644 .changeset/expression-approvers-3447-p2.md diff --git a/.changeset/expression-approvers-3447-p2.md b/.changeset/expression-approvers-3447-p2.md new file mode 100644 index 0000000000..bdace8841a --- /dev/null +++ b/.changeset/expression-approvers-3447-p2.md @@ -0,0 +1,18 @@ +--- +"@objectstack/spec": minor +"@objectstack/formula": minor +"@objectstack/lint": minor +"@objectstack/service-automation": minor +"@objectstack/plugin-approvals": minor +"@objectstack/rest": minor +--- + +Dynamic approver routing for approval nodes (#3447 P2) — three new declarative capabilities: + +**`expression` approvers.** A new approver type whose CEL expression resolves WHO approves at node entry, over exactly three roots: `current.*` (the record's live state), `trigger.*` (the submit-time snapshot) and `vars.*` (flow variables, incl. upstream node outputs). `record` and bare field names are rejected before evaluation — on this platform `record` always means "the record at event time", which is ambiguous at an approval node — with error messages that prescribe the correct spelling. The optional `resolveAs: 'user' | 'department' | 'position' | 'team'` re-expands each resolved id through the same graph lookups the static types use; with `behavior: 'per_group'` each intermediate value (e.g. each returned department) forms its own sign-off group. A missing key fails the node loudly; only a present-but-empty result counts as an empty slate. + +**`onEmptyApprovers` policy.** What an empty resolved slate does, node-level, for all approver types: `admin_rescue` (default — request opens for privileged takeover, the #3424 behaviour), `fail` (node fails), or `auto_approve` (skip the request, continue down the `approve` edge with `output.autoApproved = true`). To support auto-approve, the automation engine now honours `NodeExecutionResult.branchLabel` on the synchronous completion path — the field existed but was only ever consumed via resume signals. + +**Decision outputs.** `decide(..., { outputs })` hands structured data from the approver to the flow: the author declares allowed keys on the node (`decisionOutputs`), approvers fill values only, and accepted outputs resume the run as `.` variables — a later approval node's expression can read `vars..picked_departments`, closing "the previous approver picks the next step's approvers" without a record-field detour. Undeclared keys reject the decision; `decision`/`requestId` are reserved. Multi-approver tallies now always pin to the open-time approver snapshot (previously unanimous re-resolved at each decision against the payload snapshot). + +Also: `collectCelRootIdentifiers` is exported from `@objectstack/formula` (shared by the new `os lint` rules and the runtime pre-check, so they can never drift), resolution inputs are audited on the request snapshot as `__resolvedFrom`, and three new lint rules gate expressions, empty-slate policies and reserved output keys at author time. diff --git a/content/docs/automation/approvals.mdx b/content/docs/automation/approvals.mdx index 3d222ac634..be644a178d 100644 --- a/content/docs/automation/approvals.mdx +++ b/content/docs/automation/approvals.mdx @@ -143,8 +143,50 @@ one node, no parallel branches needed: A single **rejection** always finalizes the node as `rejected` (one veto), in every mode. `minApprovals` is clamped to the resolvable approver count / group size, so it can never deadlock. A decision may also carry `attachments` (file -references) recorded on its audit row — e.g. a signed contract. Weighted voting -and approval-matrix governance are enterprise, not here. +references) recorded on its audit row — e.g. a signed contract — and, when the +node declares `decisionOutputs`, structured `outputs` the flow receives as +`.` variables (see [Dynamic approvers](#dynamic-approvers-3447) +below). Weighted voting and approval-matrix governance are enterprise, not here. + +## Dynamic approvers (#3447) [#dynamic-approvers-3447] + +Two approver kinds bind to **live** data at the moment the node is entered — +not the submit-time snapshot — so a step can route on data produced after +submit: + +- **`field`** reads the record's live field value at node entry. An earlier + step (or an earlier step's approver) can write the routing field mid-flow and + the next approval node sees it; a multi-select user field fans out into one + approver slot per user. +- **`expression`** computes the slate with a CEL expression over exactly three + roots: **`current.*`** (the record's live state at node entry), **`trigger.*`** + (the submit-time snapshot — what a flow condition calls `record`), and + **`vars.*`** (flow variables, including node outputs and `vars.previous`). + `record` and bare field names are deliberately unavailable — at an approval + node "the record" is ambiguous between two times, so the expression must say + which one; referencing anything else fails the node loudly at entry rather + than resolving a silently-empty slate. The optional + `resolveAs: 'user' (default) | 'department' | 'position' | 'team'` re-expands + each resolved id through the same graph lookups the static types use — and + with `behavior: 'per_group'`, each intermediate value (e.g. each returned + department) forms its own sign-off group. + +A result may legitimately be **empty** (a present-but-empty field or variable); +the node-level `onEmptyApprovers` policy decides what that means — +`admin_rescue` (default: the request opens, a privileged admin takes over via +Reassign), `fail` (the node fails: an empty slate is a config bug), or +`auto_approve` (skip the request and continue down `approve` with +`output.autoApproved = true`; opt-in, since it waves the record through). A +**missing** key (`vars.never_written`) is a loud error instead — guard +genuinely-optional inputs with `has(vars.x) ? vars.x : []`. + +The "previous approver picks the next step's approvers" loop needs no record +field at all: declare `decisionOutputs: ['next_reviewers']` on node A, have the +approver decide with `{ outputs: { next_reviewers: ['u2', 'u3'] } }`, and let +node B's approver be +`{ type: 'expression', value: 'vars..next_reviewers' }`. The author +declares output keys, approvers only fill values; undeclared keys reject the +decision, and `decision` / `requestId` are reserved. ## The full lifecycle — submit to field change @@ -165,9 +207,12 @@ Only `approvers` is required on the node; everything else has a default (`behavior: 'first_response'`, `lockRecord: true`, `maxRevisions: 3`). Approver entries resolve by kind — `position`, `user`, `field`, `manager`, -`team`, `department`, `queue`, and `org_membership_level`, described in the -[callout above](#3-the-approval-node), which is the one that silently resolves -to nobody when it's mistaken for a business hierarchy. An entry that resolves +`team`, `department`, `queue`, `org_membership_level`, and `expression` +(described in the [callout above](#3-the-approval-node) and in +[Dynamic approvers](#dynamic-approvers-3447)); `org_membership_level` is the +one that silently resolves to nobody when it's mistaken for a business +hierarchy. `field`, `manager`, and `expression` resolve against the record's +**live** state at node entry (#3447). An entry that resolves to nobody is not an error: the request opens with an empty `pending_approvers` and nothing can move it, so the run parks forever. @@ -246,7 +291,7 @@ continuity levers on the same request. All are `POST | Verb | Route | Who | Effect | |:---|:---|:---|:---| -| `approve` / `reject` | `/approve` `/reject` | pending approver | Records the decision (finalizes per `behavior`). Accepts `comment`, `attachments`. | +| `approve` / `reject` | `/approve` `/reject` | pending approver | Records the decision (finalizes per `behavior`). Accepts `comment`, `attachments`, and — when the node declares `decisionOutputs` — structured `outputs` handed to the flow as `.` variables. | | `reassign` | `/reassign` | pending approver | Hands one slot to another user (`to`); the request stays pending. | | `revise` (send back) | `/revise` | pending approver | Ends this round as **`returned`**, unlocks the record for rework (ADR-0044). | | `request-info` | `/request-info` | pending approver | Asks the submitter for more info; the request **stays pending**. `comment` required. | diff --git a/content/docs/references/automation/approval.mdx b/content/docs/references/automation/approval.mdx index fed696572c..3382355cb1 100644 --- a/content/docs/references/automation/approval.mdx +++ b/content/docs/references/automation/approval.mdx @@ -54,8 +54,9 @@ const result = ApprovalDecision.parse(data); | Property | Type | Required | Description | | :--- | :--- | :--- | :--- | -| **type** | `Enum<'user' \| 'org_membership_level' \| 'role' \| 'position' \| 'team' \| 'department' \| 'manager' \| 'field' \| 'queue'>` | ✅ | | -| **value** | `string` | optional | User id / membership tier / position / team / department / field / queue — per `type` | +| **type** | `Enum<'user' \| 'org_membership_level' \| 'role' \| 'position' \| 'team' \| 'department' \| 'manager' \| 'field' \| 'queue' \| 'expression'>` | ✅ | | +| **value** | `string` | optional | User id / membership tier / position / team / department / field / queue — per `type`; for `expression`, a CEL expression over `current.*` / `trigger.*` / `vars.*` | +| **resolveAs** | `Enum<'user' \| 'department' \| 'position' \| 'team'>` | optional | How an `expression` result is expanded into approvers (default 'user') | | **group** | `string` | optional | Group label for per_group sign-off (e.g. "legal", "finance") | @@ -67,11 +68,13 @@ const result = ApprovalDecision.parse(data); | Property | Type | Required | Description | | :--- | :--- | :--- | :--- | -| **approvers** | `{ type: Enum<'user' \| 'org_membership_level' \| 'role' \| 'position' \| 'team' \| 'department' \| 'manager' \| 'field' \| 'queue'>; value?: string; group?: string }[]` | ✅ | Allowed approvers for this node | +| **approvers** | `{ type: Enum<'user' \| 'org_membership_level' \| 'role' \| 'position' \| 'team' \| 'department' \| 'manager' \| 'field' \| 'queue' \| 'expression'>; value?: string; resolveAs?: Enum<'user' \| 'department' \| 'position' \| 'team'>; group?: string }[]` | ✅ | Allowed approvers for this node | | **behavior** | `Enum<'first_response' \| 'unanimous' \| 'quorum' \| 'per_group'>` | ✅ | How to combine multiple approvers | | **minApprovals** | `integer` | optional | Approvals required — total (quorum) or per group (per_group). Default 1 | | **lockRecord** | `boolean` | ✅ | Lock the record from editing while pending | | **approvalStatusField** | `string` | optional | Business-object field to mirror request status onto | +| **onEmptyApprovers** | `Enum<'admin_rescue' \| 'fail' \| 'auto_approve'>` | ✅ | Behavior when no concrete approver resolves at node entry | +| **decisionOutputs** | `string[]` | optional | Author-declared output keys a decision may carry (approvers fill values only) | | **escalation** | `{ enabled: boolean; timeoutHours: number; action: Enum<'reassign' \| 'auto_approve' \| 'auto_reject' \| 'notify'>; escalateTo?: string; … }` | optional | Per-node SLA escalation | | **maxRevisions** | `integer` | ✅ | Max send-backs for revision before auto-reject (0 = send-back disabled) | @@ -91,6 +94,7 @@ const result = ApprovalDecision.parse(data); * `manager` * `field` * `queue` +* `expression` --- diff --git a/packages/formula/src/cel-engine.test.ts b/packages/formula/src/cel-engine.test.ts index 91f723c39b..63db5586df 100644 --- a/packages/formula/src/cel-engine.test.ts +++ b/packages/formula/src/cel-engine.test.ts @@ -1,6 +1,6 @@ import { describe, expect, it } from 'vitest'; -import { celEngine, rewriteTemporalEquality } from './cel-engine'; +import { celEngine, rewriteTemporalEquality, collectCelRootIdentifiers } from './cel-engine'; import { CEL_STDLIB_FUNCTIONS } from './validate'; import type { Expression } from '@objectstack/spec'; @@ -640,3 +640,48 @@ describe('celEngine', () => { }); }); }); + +// ── collectCelRootIdentifiers (#3447 P2) ──────────────────────────────────── +// +// Root extraction for closed-root evaluation sites (approval `expression` +// approvers). Lint and the runtime pre-check both consume this helper, so its +// contract IS the contract of what those sites accept. + +describe('collectCelRootIdentifiers', () => { + const roots = (src: string) => { + const r = collectCelRootIdentifiers(src); + if (!r.ok) throw new Error(`expected parse ok: ${r.error}`); + return r.roots.sort(); + }; + + it('collects distinct namespace roots from member chains', () => { + expect(roots('current.approvers_dynamic')).toEqual(['current']); + expect(roots('current.a.b.c == trigger.a && vars.step.result')).toEqual(['current', 'trigger', 'vars']); + }); + + it('reports a bare identifier as a root (the closed-site smoking gun)', () => { + expect(roots('approvers_dynamic')).toEqual(['approvers_dynamic']); + expect(roots('record.x')).toEqual(['record']); + }); + + it('never reports member names or function names', () => { + expect(roots('size(vars.picked) > 0 ? vars.picked : trigger.fallback')).toEqual(['trigger', 'vars']); + expect(roots('current.owner_id.startsWith("u")')).toEqual(['current']); + }); + + it('walks indexes, lists and nested calls', () => { + expect(roots('vars.picked[0] == current.ids[trigger.idx]')).toEqual(['current', 'trigger', 'vars']); + expect(roots('[current.a, vars.b]')).toEqual(['current', 'vars']); + }); + + it('accepts the #3306 null-guard ternary the same way compile does', () => { + expect(roots('current.n > 0 ? current.who : null')).toEqual(['current']); + }); + + it('reports a parse failure instead of an empty root set', () => { + const bad = collectCelRootIdentifiers('current..'); + expect(bad.ok).toBe(false); + const empty = collectCelRootIdentifiers(' '); + expect(empty.ok).toBe(false); + }); +}); diff --git a/packages/formula/src/cel-engine.ts b/packages/formula/src/cel-engine.ts index f3d4161311..2e595cafeb 100644 --- a/packages/formula/src/cel-engine.ts +++ b/packages/formula/src/cel-engine.ts @@ -60,6 +60,11 @@ const SCOPE_ROOTS = [ // Master-detail inline grids inject the header record as `parent` for a // child field's `readonlyWhen`/`requiredWhen` predicate (ADR-0036, #1581). 'parent', + // Approval-node `expression` approvers (#3447 P2): the record's LIVE state + // at node entry — bound only in that evaluation site, alongside `trigger` + // (the submit-time snapshot) and `vars`. Declared here so the strict lint + // env doesn't misread `current.x` as a bare field reference. + 'current', ] as const; /** @@ -127,6 +132,51 @@ export function firstUndeclaredReference( return null; } +/** + * The distinct top-level identifiers (namespace roots) a CEL expression + * references — `current.x + vars.step.y` → `['current', 'vars']`, a bare + * `amount > 100` → `['amount']`. Member names and function names are not + * identifiers and are never reported. + * + * Built for evaluation sites that expose a CLOSED set of roots (#3447 P2: + * approval-node `expression` approvers allow only `current`/`trigger`/`vars`). + * Such a site must reject any other root BEFORE evaluating: the runtime env is + * `unlistedVariablesAreDyn: true`, so an out-of-contract root (`record.x`, a + * bare field) would otherwise evaluate to `null` and silently produce an empty + * result instead of an error. Both the lint rule and the runtime pre-check + * consume this one helper so the two can never drift. + * + * Returns `{ ok: false }` with the classifier's message when the source does + * not parse — callers surface that as a config error, not an empty root set. + */ +export function collectCelRootIdentifiers( + source: string, +): { ok: true; roots: string[] } | { ok: false; error: string } { + if (typeof source !== 'string' || !source.trim()) { + return { ok: false, error: 'expression is empty' }; + } + try { + // Same nullable-ternary rewrite as compile/evaluate so "what parses" agrees + // across build, lint, and runtime (#3306). + const compiled = buildEnv(() => new Date(0)).parse(rewriteNullableTernary(source)); + const roots = new Set(); + const walk = (node: unknown): void => { + if (Array.isArray(node)) { for (const child of node) walk(child); return; } + if (!isCelNode(node)) return; // member/function-name strings, literals + if (node.op === 'id' && typeof node.args === 'string') { roots.add(node.args); return; } + // Member access: only the receiver can hold identifiers — `args[1]` is the + // member NAME, which must not be reported as a root. + if (node.op === '.' && Array.isArray(node.args)) { walk(node.args[0]); return; } + walk(node.args); + }; + walk(compiled.ast); + return { ok: true, roots: [...roots] }; + } catch (err) { + const classified = classifyError(err); + return { ok: false, error: classified.ok === false ? classified.error.message : String(err) }; + } +} + /** * The result type cel-js's type-checker infers for a `value`/`predicate` * expression — its raw CEL type name (`'int'`, `'double'`, `'string'`, `'bool'`, diff --git a/packages/formula/src/index.ts b/packages/formula/src/index.ts index 8b0e85ed9f..babcdde9e3 100644 --- a/packages/formula/src/index.ts +++ b/packages/formula/src/index.ts @@ -11,6 +11,10 @@ export { ExpressionEngine, getEngine, hasDialect, register } from './registry'; export { celEngine, DEFAULT_LIMITS } from './cel-engine'; +// #3447 P2 — root-identifier extraction for closed-root evaluation sites +// (approval `expression` approvers): lint and the runtime pre-check share this +// one helper so what they accept can never drift. +export { collectCelRootIdentifiers } from './cel-engine'; export { cronEngine } from './cron-engine'; export { templateEngine, TEMPLATE_FORMATTERS, formatValue } from './template-engine'; export { registerStdLib, buildScope } from './stdlib'; diff --git a/packages/lint/src/index.ts b/packages/lint/src/index.ts index b0fa10ff6c..6c737b5761 100644 --- a/packages/lint/src/index.ts +++ b/packages/lint/src/index.ts @@ -127,6 +127,9 @@ export { APPROVAL_APPROVER_TYPE_UNKNOWN, APPROVAL_ESCALATION_REASSIGN_NO_TARGET, APPROVAL_APPROVERS_MAY_RESOLVE_EMPTY, + APPROVAL_EXPRESSION_INVALID, + APPROVAL_EXPRESSION_NO_EMPTY_POLICY, + APPROVAL_DECISION_OUTPUTS_RESERVED, } from './validate-approval-approvers.js'; export type { ApprovalApproverFinding, ApprovalApproverSeverity } from './validate-approval-approvers.js'; diff --git a/packages/lint/src/validate-approval-approvers.test.ts b/packages/lint/src/validate-approval-approvers.test.ts index 2a18778751..506cbefec7 100644 --- a/packages/lint/src/validate-approval-approvers.test.ts +++ b/packages/lint/src/validate-approval-approvers.test.ts @@ -8,6 +8,9 @@ import { APPROVAL_APPROVER_TYPE_UNKNOWN, APPROVAL_ESCALATION_REASSIGN_NO_TARGET, APPROVAL_APPROVERS_MAY_RESOLVE_EMPTY, + APPROVAL_EXPRESSION_INVALID, + APPROVAL_EXPRESSION_NO_EMPTY_POLICY, + APPROVAL_DECISION_OUTPUTS_RESERVED, } from './validate-approval-approvers.js'; function stackWithApprovers(approvers: unknown[]): Record { @@ -186,3 +189,110 @@ describe('validateApprovalApprovers', () => { expect(findings).toEqual([]); }); }); + +// ── #3447 P2: expression approvers / decision outputs ───────────────────── + +describe('expression approvers (#3447 P2)', () => { + const stackWithConfig = (config: Record): Record => ({ + flows: [{ + name: 'expense_approval', + nodes: [ + { id: 'start', type: 'start', config: {} }, + { id: 'step1', type: 'approval', config }, + ], + edges: [], + }], + }); + + it('accepts the three legal roots (current/trigger/vars) with an explicit empty policy', () => { + const findings = validateApprovalApprovers(stackWithConfig({ + approvers: [ + { type: 'expression', value: 'current.approvers_dynamic' }, + { type: 'expression', value: 'trigger.owner_id' }, + { type: 'expression', value: 'vars.approval_lead.next_reviewers', resolveAs: 'department' }, + ], + onEmptyApprovers: 'fail', + })); + expect(findings).toEqual([]); + }); + + it('errors on a `record` root and prescribes current/trigger', () => { + const findings = validateApprovalApprovers(stackWithConfig({ + approvers: [{ type: 'expression', value: 'record.approvers_dynamic' }], + onEmptyApprovers: 'admin_rescue', + })); + expect(findings).toHaveLength(1); + expect(findings[0].rule).toBe(APPROVAL_EXPRESSION_INVALID); + expect(findings[0].severity).toBe('error'); + expect(findings[0].message).toContain('record'); + expect(findings[0].hint).toContain('current.'); + expect(findings[0].hint).toContain('trigger.'); + }); + + it('errors on a bare field reference with the closed-root hint', () => { + const findings = validateApprovalApprovers(stackWithConfig({ + approvers: [{ type: 'expression', value: 'approvers_dynamic' }], + onEmptyApprovers: 'admin_rescue', + })); + expect(findings).toHaveLength(1); + expect(findings[0].rule).toBe(APPROVAL_EXPRESSION_INVALID); + expect(findings[0].severity).toBe('error'); + expect(findings[0].hint).toContain('current.'); + }); + + it('errors on a non-parsing expression and an empty one', () => { + const bad = validateApprovalApprovers(stackWithConfig({ + approvers: [{ type: 'expression', value: 'current..' }], + onEmptyApprovers: 'admin_rescue', + })); + expect(bad).toHaveLength(1); + expect(bad[0].rule).toBe(APPROVAL_EXPRESSION_INVALID); + expect(bad[0].message).toContain('does not parse'); + + const empty = validateApprovalApprovers(stackWithConfig({ + approvers: [{ type: 'expression', value: '' }], + onEmptyApprovers: 'admin_rescue', + })); + expect(empty).toHaveLength(1); + expect(empty[0].rule).toBe(APPROVAL_EXPRESSION_INVALID); + expect(empty[0].message).toContain('empty expression'); + }); + + it('flags resolveAs on a non-expression approver as dead config', () => { + const findings = validateApprovalApprovers(stackWithConfig({ + approvers: [{ type: 'field', value: 'reviewer', resolveAs: 'department' }], + })); + expect(findings).toHaveLength(1); + expect(findings[0].rule).toBe(APPROVAL_EXPRESSION_INVALID); + expect(findings[0].severity).toBe('info'); + expect(findings[0].path).toContain('resolveAs'); + }); + + it('nudges an expression node to declare onEmptyApprovers explicitly', () => { + const findings = validateApprovalApprovers(stackWithConfig({ + approvers: [{ type: 'expression', value: 'vars.picked' }], + })); + expect(findings).toHaveLength(1); + expect(findings[0].rule).toBe(APPROVAL_EXPRESSION_NO_EMPTY_POLICY); + expect(findings[0].severity).toBe('info'); + expect(findings[0].hint).toContain('admin_rescue'); + }); + + it('errors on reserved decisionOutputs keys', () => { + const findings = validateApprovalApprovers(stackWithConfig({ + approvers: [{ type: 'user', value: 'u1' }], + decisionOutputs: ['decision', 'next_reviewers'], + })); + expect(findings).toHaveLength(1); + expect(findings[0].rule).toBe(APPROVAL_DECISION_OUTPUTS_RESERVED); + expect(findings[0].severity).toBe('error'); + expect(findings[0].message).toContain('decision'); + }); + + it('accepts declared non-reserved decisionOutputs', () => { + expect(validateApprovalApprovers(stackWithConfig({ + approvers: [{ type: 'user', value: 'u1' }], + decisionOutputs: ['next_reviewers', 'note'], + }))).toEqual([]); + }); +}); diff --git a/packages/lint/src/validate-approval-approvers.ts b/packages/lint/src/validate-approval-approvers.ts index 3a39a2b8f3..0dc5e969d1 100644 --- a/packages/lint/src/validate-approval-approvers.ts +++ b/packages/lint/src/validate-approval-approvers.ts @@ -21,6 +21,9 @@ * | approval-approver-type-unknown | warning | contract-first (PD #12) | * | approval-escalation-reassign-no-target | warning | silent notify degradation | * | approval-approvers-may-resolve-empty | info | empty-position dead-end (#3424) | + * | approval-expression-invalid | error/info | #3447 P2 closed-root expressions | + * | approval-expression-no-empty-policy | info | #3447 P2 empty-slate policy | + * | approval-decision-outputs-reserved | error | #3447 P2 resume envelope | * * The first two are mutually exclusive by construction — a bad *value* wins, * because its fix (`position`) differs from the deprecation's fix @@ -40,12 +43,28 @@ import { DEPRECATED_APPROVER_TYPES, canonicalApproverType, } from '@objectstack/spec/automation'; +import { collectCelRootIdentifiers } from '@objectstack/formula'; export const APPROVAL_APPROVER_NOT_MEMBERSHIP_TIER = 'approval-approver-not-membership-tier'; export const APPROVAL_APPROVER_TYPE_DEPRECATED = 'approval-approver-type-deprecated'; export const APPROVAL_APPROVER_TYPE_UNKNOWN = 'approval-approver-type-unknown'; export const APPROVAL_ESCALATION_REASSIGN_NO_TARGET = 'approval-escalation-reassign-no-target'; export const APPROVAL_APPROVERS_MAY_RESOLVE_EMPTY = 'approval-approvers-may-resolve-empty'; +export const APPROVAL_EXPRESSION_INVALID = 'approval-expression-invalid'; +export const APPROVAL_EXPRESSION_NO_EMPTY_POLICY = 'approval-expression-no-empty-policy'; +export const APPROVAL_DECISION_OUTPUTS_RESERVED = 'approval-decision-outputs-reserved'; + +/** + * The CLOSED root set an `expression` approver may reference (#3447 P2) — + * `current` (live record at node entry), `trigger` (submit-time snapshot), + * `vars` (flow variables). Mirrors APPROVER_EXPRESSION_ROOTS in + * plugin-approvals; both sides extract roots via the same + * {@link collectCelRootIdentifiers}, so what lints clean is what runs. + */ +const EXPRESSION_ROOTS = new Set(['current', 'trigger', 'vars']); + +/** Resume-envelope keys a decision output may never use (#3447 P2). */ +const RESERVED_OUTPUT_KEYS = new Set(['decision', 'requestId']); /** * Approver types that route to a GROUP whose membership is runtime data and can @@ -150,6 +169,74 @@ export function validateApprovalApprovers(stack: AnyRec): ApprovalApproverFindin const canonical = canonicalApproverType(type); + // Expression approvers (#3447 P2): the runtime REJECTS an expression + // that doesn't parse or references a root outside `current`/`trigger`/ + // `vars` (the CEL env would resolve an unknown root as dyn → null → a + // silently-empty slate, so the pre-check fails the node loudly). Catch + // both at author time — `error`, because the node cannot run. + if (canonical === 'expression') { + const source = value.trim(); + if (!source) { + findings.push({ + severity: 'error', + rule: APPROVAL_EXPRESSION_INVALID, + where, + path: `${path}.value`, + message: `expression approver has an empty expression — the node fails at entry.`, + hint: + `Write a CEL expression over current.* (the record's live state at node entry), ` + + `trigger.* (the submit-time snapshot) or vars.* (flow variables), ` + + `e.g. current.approvers_dynamic or vars.approval_lead.picked_departments.`, + }); + } else { + const parsed = collectCelRootIdentifiers(source); + if (!parsed.ok) { + findings.push({ + severity: 'error', + rule: APPROVAL_EXPRESSION_INVALID, + where, + path: `${path}.value`, + message: `expression approver does not parse as CEL: ${parsed.error}.`, + hint: + `Approver expressions are bare CEL (no {…} template braces), e.g. ` + + `current.approvers_dynamic or vars.get_reviewers.record.owner_id.`, + }); + } else { + const illegal = parsed.roots.filter((r) => !EXPRESSION_ROOTS.has(r)); + if (illegal.length) { + const wantsRecord = illegal.includes('record') || illegal.includes('previous'); + findings.push({ + severity: 'error', + rule: APPROVAL_EXPRESSION_INVALID, + where, + path: `${path}.value`, + message: + `expression approver references \`${illegal.join('`, `')}\` — only current.*, ` + + `trigger.* and vars.* are available, and the node fails at entry on any other root.`, + hint: wantsRecord + ? `\`record\`/\`previous\` are not bound here (on this platform \`record\` always means ` + + `"the record at event time", which is ambiguous at an approval node). Write ` + + `current. for the live value at node entry, trigger. for the ` + + `submit-time snapshot (vars.previous carries the pre-update row).` + : `Did you mean current. (live record), trigger. (submit snapshot), ` + + `or vars. (flow variable)?`, + }); + } + } + } + } else if (a.resolveAs != null) { + // resolveAs only means something on an expression approver; on any + // other type it silently does nothing — surface the dead config. + findings.push({ + severity: 'info', + rule: APPROVAL_EXPRESSION_INVALID, + where, + path: `${path}.resolveAs`, + message: `resolveAs has no effect on a '${type}' approver — it only applies to type 'expression'.`, + hint: `Remove it, or switch this approver to { type: 'expression', value: '', resolveAs: '${String(a.resolveAs)}' }.`, + }); + } + // Exactly one of the two below fires. Order matters: a bad VALUE is // the more serious (and differently-fixed) defect, so it wins. Telling // an author to rewrite { type: 'role', value: 'sales_manager' } as @@ -217,6 +304,51 @@ export function validateApprovalApprovers(stack: AnyRec): ApprovalApproverFindin }); } + // #3447 P2: a node with an `expression` approver resolves people from + // runtime data — an empty result is far likelier than for static types + // (a mid-flow field nobody wrote yet, an upstream output that came back + // empty). Nudge the author to SAY what an empty slate should do rather + // than inherit the default silently. + const hasExpression = approvers.some( + (a) => a && typeof a === 'object' && canonicalApproverType(String((a as AnyRec).type ?? '')) === 'expression', + ); + if (hasExpression && (cfg as AnyRec).onEmptyApprovers == null) { + findings.push({ + severity: 'info', + rule: APPROVAL_EXPRESSION_NO_EMPTY_POLICY, + where, + path: `flows[${fi}].nodes[${ni}].config`, + message: + `this node resolves approvers from an expression but declares no onEmptyApprovers — ` + + `an empty result falls back to the default ('admin_rescue': request opens, only a ` + + `privileged admin can act).`, + hint: + `Declare the empty-slate policy explicitly: onEmptyApprovers: 'admin_rescue' (hold for ` + + `admin takeover), 'fail' (fail the node — config bug), or 'auto_approve' (wave through, ` + + `output.autoApproved = true).`, + }); + } + + // #3447 P2: `decision`/`requestId` ride the resume envelope; a declared + // decision output with either name is rejected at runtime on every + // decide — the node can never accept the output it declares. + const declaredOutputs = Array.isArray((cfg as AnyRec).decisionOutputs) + ? ((cfg as AnyRec).decisionOutputs as unknown[]).map(String) + : []; + const reserved = declaredOutputs.filter((k) => RESERVED_OUTPUT_KEYS.has(k)); + if (reserved.length) { + findings.push({ + severity: 'error', + rule: APPROVAL_DECISION_OUTPUTS_RESERVED, + where, + path: `flows[${fi}].nodes[${ni}].config.decisionOutputs`, + message: + `decisionOutputs declares reserved key(s) \`${reserved.join('`, `')}\` — the resume ` + + `envelope owns them, so every decide carrying them is rejected.`, + hint: `Rename the output key(s); any name other than 'decision'/'requestId' works.`, + }); + } + // escalation.action 'reassign' with no escalateTo silently degrades to a // plain SLA-breach notification at runtime — the hand-off the author // asked for never happens. diff --git a/packages/plugins/plugin-approvals/src/approval-node.test.ts b/packages/plugins/plugin-approvals/src/approval-node.test.ts index 0c4d0dd968..bf1d431e3e 100644 --- a/packages/plugins/plugin-approvals/src/approval-node.test.ts +++ b/packages/plugins/plugin-approvals/src/approval-node.test.ts @@ -193,4 +193,124 @@ describe('Approval node bridge (ADR-0019)', () => { service.decideNode(request.id, { decision: 'approve', actorId: 'intruder' }, { isSystem: false, positions: [], permissions: [] } as any), ).rejects.toThrow(/FORBIDDEN/); }); + + // ── #3447 P2: dynamic approvers end-to-end ──────────────────────── + // + // The issue's headline scenario as one flow: the first approver PICKS the + // next step's approvers in their decision, the next approval node resolves + // them from `vars.*` at entry — no record-field detour, no snapshot staleness. + + it('decide outputs feed the NEXT approval node via vars.. (#3447 P2)', async () => { + automation.registerFlow('two_stage', { + name: 'two_stage', + label: 'Two Stage', + type: 'autolaunched', + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { + id: 'lead_review', type: 'approval', label: 'Lead Review', + config: { approvers: [{ type: 'user', value: 'lead' }], decisionOutputs: ['next_reviewers'] }, + }, + { + id: 'co_sign', type: 'approval', label: 'Co-sign', + config: { + approvers: [{ type: 'expression', value: 'vars.lead_review.next_reviewers' }], + behavior: 'unanimous', + }, + }, + { id: 'on_approved', type: 'mark', label: 'Approved' }, + { id: 'on_rejected', type: 'mark', label: 'Rejected' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'lead_review' }, + { id: 'e2', source: 'lead_review', target: 'co_sign', label: 'approve' }, + { id: 'e3', source: 'lead_review', target: 'on_rejected', label: 'reject' }, + { id: 'e4', source: 'co_sign', target: 'on_approved', label: 'approve' }, + { id: 'e5', source: 'co_sign', target: 'on_rejected', label: 'reject' }, + ], + }); + + const paused = await automation.execute('two_stage', { + object: 'crm_deal', record: { id: 'd1' }, userId: 'submitter', + }); + expect(paused.status).toBe('paused'); + + // The lead approves AND hands the co-reviewers to the flow. + const first = (await fake.find('sys_approval_request', { where: { status: 'pending' } }))[0]; + await service.decide(first.id, { + decision: 'approve', actorId: 'lead', outputs: { next_reviewers: ['u2', 'u3'] }, + }, SYSTEM_CTX); + + // The co-sign node resolved its slate from the lead's decision outputs. + const second = (await fake.find('sys_approval_request', { where: { status: 'pending' } }))[0]; + expect(second).toBeDefined(); + expect(second.flow_node_id).toBe('co_sign'); + expect(String(second.pending_approvers).split(',').sort()).toEqual(['u2', 'u3']); + expect(marks).toHaveLength(0); + + // Both picked reviewers sign off → the run completes down `approve`. + await service.decide(second.id, { decision: 'approve', actorId: 'u2' }, SYSTEM_CTX); + await service.decide(second.id, { decision: 'approve', actorId: 'u3' }, SYSTEM_CTX); + expect(marks).toEqual(['on_approved']); + expect(automation.listSuspendedRuns()).toHaveLength(0); + }); + + it('expression current.* resolves against the LIVE row at node entry (#3447 P2)', async () => { + fake.tables.set('crm_deal', [{ id: 'd1', reviewers: ['u7', 'u8'] }]); + registerDecisionFlow(automation, [{ type: 'expression', value: 'current.reviewers' }], 'unanimous'); + // The trigger snapshot carries an EMPTY reviewers field — only the live + // row names them, exactly the mid-flow-written-field shape of the issue. + await automation.execute('deal_approval', { + object: 'crm_deal', record: { id: 'd1', reviewers: [] }, userId: 'submitter', + }); + const request = (await fake.find('sys_approval_request', { where: { status: 'pending' } }))[0]; + expect(String(request.pending_approvers).split(',').sort()).toEqual(['u7', 'u8']); + }); + + it("onEmptyApprovers 'auto_approve' completes down the approve edge without suspending (#3447 P2)", async () => { + automation.registerFlow('auto_ok', { + name: 'auto_ok', + label: 'Auto OK', + type: 'autolaunched', + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { + id: 'gate', type: 'approval', label: 'Gate', + config: { + // Present-but-empty (a missing key would fail loudly instead). + approvers: [{ type: 'expression', value: 'trigger.reviewers' }], + onEmptyApprovers: 'auto_approve', + }, + }, + { id: 'on_approved', type: 'mark', label: 'Approved' }, + { id: 'on_rejected', type: 'mark', label: 'Rejected' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'gate' }, + { id: 'e2', source: 'gate', target: 'on_approved', label: 'approve' }, + { id: 'e3', source: 'gate', target: 'on_rejected', label: 'reject' }, + ], + }); + + const result = await automation.execute('auto_ok', { + object: 'crm_deal', record: { id: 'd1', reviewers: [] }, userId: 'submitter', + }); + + // No pause, no request row — the empty slate waved through, down `approve` + // ONLY (the branchLabel wiring; unlabelled traversal would hit both marks). + expect(result.status).not.toBe('paused'); + expect(marks).toEqual(['on_approved']); + expect(await fake.find('sys_approval_request', {})).toHaveLength(0); + expect(automation.listSuspendedRuns()).toHaveLength(0); + }); + + it('an expression referencing `record` fails the node loudly, not as an empty slate (#3447 P2)', async () => { + registerDecisionFlow(automation, [{ type: 'expression', value: 'record.reviewers' }]); + const result = await automation.execute('deal_approval', { + object: 'crm_deal', record: { id: 'd1', reviewers: ['u1'] }, userId: 'submitter', + }); + expect(result.success).toBe(false); + expect(String(result.error ?? '')).toMatch(/current\.|current\./); + expect(await fake.find('sys_approval_request', {})).toHaveLength(0); + }); }); diff --git a/packages/plugins/plugin-approvals/src/approval-node.ts b/packages/plugins/plugin-approvals/src/approval-node.ts index 2f1bc87773..bcfab04a27 100644 --- a/packages/plugins/plugin-approvals/src/approval-node.ts +++ b/packages/plugins/plugin-approvals/src/approval-node.ts @@ -38,6 +38,12 @@ export interface ApprovalAutomationSurface { error?: string; suspend?: boolean; correlation?: string; + /** + * #3447 P2: walk this labelled out-edge on normal (non-suspend) + * completion — how an `onEmptyApprovers: 'auto_approve'` node continues + * down `approve` without a decision. Mirrors NodeExecutionResult. + */ + branchLabel?: string; }>; }): void; resume?(runId: string, signal?: { output?: Record; branchLabel?: string }): Promise; @@ -50,6 +56,29 @@ interface MinimalLogger { const SYSTEM_CTX = { isSystem: true, positions: [], permissions: [] } as const; +/** + * Rebuild the nested object the engine's CEL conditions see from the flow's + * flat variable Map — dotted keys (`get_rec.record`) become nested paths, so an + * `expression` approver's `vars.get_rec.record.owner_id` reads exactly like a + * condition's. Mirrors the engine's own evaluateCondition rebuild; kept local + * because this plugin deliberately does not depend on service-automation. + */ +function nestVariables(variables: Map): Record { + const vars: Record = {}; + for (const [key, value] of variables) { + const segs = key.split('.'); + let cursor = vars; + for (let i = 0; i < segs.length - 1; i++) { + if (typeof cursor[segs[i]] !== 'object' || cursor[segs[i]] === null) { + cursor[segs[i]] = {}; + } + cursor = cursor[segs[i]] as Record; + } + cursor[segs[segs.length - 1]] = value; + } + return vars; +} + /** * Register the `approval` node executor on the automation engine. Idempotent at * the engine level (re-registering replaces). Safe to skip when no automation @@ -116,6 +145,10 @@ export function registerApprovalNode( submitterId: context?.userId ?? null, record, organizationId: context?.organizationId ?? context?.tenantId ?? null, + // #3447 P2: flow variables (nested, as CEL conditions see them) — the + // `vars.*` root for `expression` approvers; `record` above doubles as + // their `trigger.*` snapshot root. + variables: nestVariables(variables), }, { ...SYSTEM_CTX, userId: context?.userId, @@ -123,6 +156,21 @@ export function registerApprovalNode( tenantId: context?.tenantId, } as unknown as SharingExecutionContext); + // #3447 P2: empty slate + onEmptyApprovers: 'auto_approve' — nobody to + // ask, no request row. Complete (don't suspend) straight down the + // `approve` edge; `autoApproved` on the output keeps the waved-through + // hop distinguishable from a human decision in the run trace. + if ('autoApproved' in request) { + logger?.info?.('[approvals] approval node auto-approved (empty approver slate)', { + node: node.id, run: String(runId), + }); + return { + success: true, + branchLabel: 'approve', + output: { decision: 'approve', autoApproved: true }, + }; + } + logger?.info?.('[approvals] approval node suspended run', { node: node.id, request: request.id, run: String(runId), }); diff --git a/packages/plugins/plugin-approvals/src/approval-service.test.ts b/packages/plugins/plugin-approvals/src/approval-service.test.ts index 9ae1085cd9..ecf8126920 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.test.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.test.ts @@ -246,6 +246,229 @@ describe('ApprovalService (node era)', () => { expect(req.pending_approvers).toEqual(['u7']); }); + // ── approver expansion: expression (#3447 P2) ─────────────────── + // + // A CEL expression resolved at node entry over three EXPLICIT roots: + // `current.*` (live record), `trigger.*` (submit snapshot), `vars.*` (flow + // variables). `record` / bare fields are rejected before evaluation — the + // runtime env would resolve them as dyn → null → a silently-empty slate. + + const exprInput = ( + value: string, + extra: Record = {}, + approverExtra: Record = {}, + configExtra: Record = {}, + ) => ({ + ...openInput([]), + config: { + approvers: [{ type: 'expression' as const, value, ...approverExtra }], + behavior: 'unanimous' as const, + lockRecord: true, + ...configExtra, + }, + ...extra, + }); + + it('expression: current.* reads the LIVE record at node entry (#3447 P2)', async () => { + engine._tables['opportunity'] = [{ id: 'opp1', approvers_dynamic: ['u2', 'u3'] }]; + const req = await svc.openNodeRequest( + exprInput('current.approvers_dynamic', { record: { id: 'opp1', approvers_dynamic: [] } }), CTX, + ) as any; + expect(req.pending_approvers.sort()).toEqual(['u2', 'u3']); + }); + + it('expression: trigger.* reads the submit-time snapshot, not the live row (#3447 P2)', async () => { + engine._tables['opportunity'] = [{ id: 'opp1', reviewer: 'new_reviewer' }]; + const req = await svc.openNodeRequest( + exprInput('trigger.reviewer', { record: { id: 'opp1', reviewer: 'old_reviewer' } }), CTX, + ) as any; + expect(req.pending_approvers).toEqual(['old_reviewer']); + }); + + it('expression: vars.* reads flow variables; a CSV string fans out (#3447 P2)', async () => { + const req = await svc.openNodeRequest( + exprInput('vars.approval_lead.next_reviewers', { + variables: { approval_lead: { next_reviewers: 'u5, u6' } }, + }), CTX, + ) as any; + expect(req.pending_approvers.sort()).toEqual(['u5', 'u6']); + }); + + it('expression: an array result fans out into one slot per id (#3447 P2)', async () => { + const req = await svc.openNodeRequest( + exprInput('vars.picked', { variables: { picked: ['u2', 'u3', 'u4'] } }), CTX, + ) as any; + expect(req.pending_approvers.sort()).toEqual(['u2', 'u3', 'u4']); + }); + + it('expression: OOO delegation applies per resolved user (#3447 P2 / #1322)', async () => { + engine._tables['sys_approval_delegation'] = [{ + id: 'del1', delegator_id: 'u2', delegate_id: 'u9', + valid_from: '2026-01-01T00:00:00Z', valid_until: '2026-12-31T00:00:00Z', + reason: 'leave', organization_id: 't1', + }]; + const req = await svc.openNodeRequest( + exprInput('vars.picked', { variables: { picked: ['u2', 'u3'] } }), CTX, + ) as any; + expect(req.pending_approvers.sort()).toEqual(['u3', 'u9']); + }); + + it('expression: rejects a `record` root BEFORE evaluation, prescribing current/trigger (#3447 P2)', async () => { + await expect(svc.openNodeRequest(exprInput('record.approvers_dynamic'), CTX)) + .rejects.toThrow(/VALIDATION_FAILED[\s\S]*`record`[\s\S]*current\./); + }); + + it('expression: rejects an unknown bare root with the closed-root hint (#3447 P2)', async () => { + await expect(svc.openNodeRequest(exprInput('approvers_dynamic'), CTX)) + .rejects.toThrow(/VALIDATION_FAILED.*approvers_dynamic.*current\.\*/s); + }); + + it('expression: a non-parsing source fails loudly, not as an empty slate (#3447 P2)', async () => { + await expect(svc.openNodeRequest(exprInput('current..'), CTX)) + .rejects.toThrow(/VALIDATION_FAILED.*does not parse/s); + }); + + it('expression: a non-id result type (bool) fails loudly (#3447 P2)', async () => { + const req = svc.openNodeRequest( + exprInput('current.amount > 100', { record: { id: 'opp1', amount: 500 } }), CTX, + ); + await expect(req).rejects.toThrow(/EXPRESSION_FAILED.*must yield ids/s); + }); + + it('expression + resolveAs department: expands each returned id through the graph, one per_group group per department (#3447 P2)', async () => { + engine._tables['sys_business_unit'] = [ + { id: 'd1', active: true, organization_id: 't1' }, + { id: 'd2', active: true, organization_id: 't1' }, + ]; + engine._tables['sys_business_unit_member'] = [ + { id: 'm1', business_unit_id: 'd1', user_id: 'u2' }, + { id: 'm2', business_unit_id: 'd1', user_id: 'u3' }, + { id: 'm3', business_unit_id: 'd2', user_id: 'u4' }, + ]; + const req = await svc.openNodeRequest( + exprInput('vars.picked_departments', { + variables: { picked_departments: ['d1', 'd2'] }, + }, { resolveAs: 'department' }, { behavior: 'per_group' }), CTX, + ) as any; + expect(req.pending_approvers.sort()).toEqual(['u2', 'u3', 'u4']); + // Each department forms its own sub-group: one sign-off per department. + const raw = engine._tables['sys_approval_request'][0]; + const snapshot = JSON.parse(raw.node_config_json); + expect(snapshot.__approverGroups).toEqual({ + u2: ['#0:d1'], u3: ['#0:d1'], u4: ['#0:d2'], + }); + }); + + it('expression + resolveAs: an unstaffed department keeps a literal slot (#3447 P2)', async () => { + engine._tables['sys_business_unit'] = [{ id: 'd9', active: true, organization_id: 't1' }]; + const req = await svc.openNodeRequest( + exprInput('vars.picked', { variables: { picked: ['d9'] } }, { resolveAs: 'department' }), CTX, + ) as any; + expect(req.pending_approvers).toEqual(['department:d9']); + }); + + it('expression: __resolvedFrom snapshots the resolution INPUT for audit (#3447 P2)', async () => { + engine._tables['opportunity'] = [{ id: 'opp1', approvers_dynamic: ['u2'] }]; + await svc.openNodeRequest(exprInput('current.approvers_dynamic', { record: { id: 'opp1' } }), CTX); + const raw = engine._tables['sys_approval_request'][0]; + const snapshot = JSON.parse(raw.node_config_json); + expect(snapshot.__resolvedFrom).toEqual({ 'expression#0': ['u2'] }); + }); + + it('expression: a MISSING key is a loud error, never a silent empty slate (#3447 P2)', async () => { + // CEL map access on an absent key throws ("No such key") — deliberate: + // referencing a variable nobody wrote is a wiring bug, not "no approvers". + // An EMPTY slate is expressed by a present-but-empty value (next test + // group); authors guard optional inputs with `has(...)` / `.?` explicitly. + await expect(svc.openNodeRequest( + exprInput('vars.never_written', { variables: {} }), CTX, + )).rejects.toThrow(/EXPRESSION_FAILED.*No such key/s); + }); + + // ── onEmptyApprovers policy (#3447 P2) ────────────────────────── + // + // "Empty" = the expression/field RESOLVED (key present) but yielded nobody. + // A missing key is a loud error instead (test above). + + it("onEmptyApprovers 'fail': an empty slate fails the open loudly", async () => { + await expect(svc.openNodeRequest( + exprInput('vars.picked', { variables: { picked: [] } }, {}, { onEmptyApprovers: 'fail' }), CTX, + )).rejects.toThrow(/NO_APPROVERS/); + expect(engine._tables['sys_approval_request'] ?? []).toHaveLength(0); + }); + + it("onEmptyApprovers 'auto_approve': no request opens, outcome says autoApproved", async () => { + const outcome = await svc.openNodeRequest( + exprInput('vars.picked', { variables: { picked: [] } }, {}, { onEmptyApprovers: 'auto_approve' }), CTX, + ); + expect(outcome).toEqual({ autoApproved: true, reason: 'empty_approvers' }); + expect(engine._tables['sys_approval_request'] ?? []).toHaveLength(0); + }); + + it("onEmptyApprovers default ('admin_rescue'): the request still opens for admin takeover (#3424)", async () => { + const req = await svc.openNodeRequest( + exprInput('vars.picked', { variables: { picked: [] } }), CTX, + ) as any; + expect(req.status).toBe('pending'); + expect(req.pending_approvers).toEqual([]); + expect(engine._tables['sys_approval_request']).toHaveLength(1); + }); + + // ── decision outputs (#3447 P2) ───────────────────────────────── + + it('decision outputs: rejected when the node declares none', async () => { + const req = await svc.openNodeRequest(openInput(['u9']), CTX); + await expect(svc.decideNode(req.id, { + decision: 'approve', actorId: 'u9', outputs: { next: 'u2' }, + }, SYS)).rejects.toThrow(/VALIDATION_FAILED.*declares no decisionOutputs/s); + }); + + it('decision outputs: rejected when a key is undeclared', async () => { + const req = await svc.openNodeRequest( + openInput(['u9'], {}, { decisionOutputs: ['next_reviewers'] }), CTX, + ); + await expect(svc.decideNode(req.id, { + decision: 'approve', actorId: 'u9', outputs: { other_key: 1 }, + }, SYS)).rejects.toThrow(/VALIDATION_FAILED.*other_key.*not declared/s); + }); + + it('decision outputs: reserved keys are rejected even when declared', async () => { + const req = await svc.openNodeRequest( + openInput(['u9'], {}, { decisionOutputs: ['decision'] }), CTX, + ); + await expect(svc.decideNode(req.id, { + decision: 'approve', actorId: 'u9', outputs: { decision: 'spoofed' }, + }, SYS)).rejects.toThrow(/VALIDATION_FAILED.*reserved/s); + }); + + it('decision outputs: accepted keys return from decideNode and snapshot as __decisionOutputs', async () => { + const req = await svc.openNodeRequest( + openInput(['u9'], {}, { decisionOutputs: ['next_reviewers', 'note'] }), CTX, + ); + const out = await svc.decideNode(req.id, { + decision: 'approve', actorId: 'u9', outputs: { next_reviewers: ['u2', 'u3'] }, + }, SYS); + expect(out.finalized).toBe(true); + expect(out.outputs).toEqual({ next_reviewers: ['u2', 'u3'] }); + const raw = engine._tables['sys_approval_request'][0]; + expect(JSON.parse(raw.node_config_json).__decisionOutputs).toEqual({ next_reviewers: ['u2', 'u3'] }); + }); + + it('decision outputs: co-sign votes accumulate, the finalizing decision hands the merged set over', async () => { + const req = await svc.openNodeRequest( + openInput(['u1', 'u2'], {}, { behavior: 'unanimous', decisionOutputs: ['legal_note', 'finance_note'] }), CTX, + ); + const first = await svc.decideNode(req.id, { + decision: 'approve', actorId: 'u1', outputs: { legal_note: 'ok' }, + }, SYS); + expect(first.finalized).toBe(false); + const second = await svc.decideNode(req.id, { + decision: 'approve', actorId: 'u2', outputs: { finance_note: 'ok too' }, + }, SYS); + expect(second.finalized).toBe(true); + expect(second.outputs).toEqual({ legal_note: 'ok', finance_note: 'ok too' }); + }); + // ── approver expansion: position (ADR-0090 D3) ────────────────── const positionInput = (extra: Record = {}) => ({ diff --git a/packages/plugins/plugin-approvals/src/approval-service.ts b/packages/plugins/plugin-approvals/src/approval-service.ts index 40c63af663..eec7558359 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.ts @@ -6,6 +6,7 @@ import { canonicalApproverType, type ApprovalNodeConfig, } from '@objectstack/spec/automation'; +import { ExpressionEngine, collectCelRootIdentifiers } from '@objectstack/formula'; import { ADMIN_FULL_ACCESS, ORGANIZATION_ADMIN, @@ -125,6 +126,43 @@ interface OooSubstitution { reason: string | null; } +/** + * The CLOSED set of namespace roots an `expression` approver may reference + * (#3447 P2). Three explicit times/sources, no `record`, no bare field names: + * `record` means "the record at event time" everywhere else on the platform + * (flow conditions: trigger snapshot; hooks: the write payload), so binding it + * here — to either time — would silently alias one meaning to the other. The + * runtime CEL env treats unknown roots as `dyn` (→ `null` → an empty slate), + * so out-of-contract roots MUST be rejected before evaluation; both this + * pre-check and the lint rule read the roots via + * {@link collectCelRootIdentifiers} so they can never drift. + */ +const APPROVER_EXPRESSION_ROOTS = new Set(['current', 'trigger', 'vars']); + +/** + * Evaluation context an approval node hands to `expression` approvers + * (#3447 P2). `current` (the live record) is supplied by openNodeRequest's + * re-read; these two carry the other roots. + */ +export interface ApproverExpressionContext { + /** Submit-time snapshot (the flow's `$record`) — bound as `trigger.*`. */ + trigger?: Record | null; + /** Flow variables at node entry (nested by dotted key) — bound as `vars.*`. */ + vars?: Record | null; +} + +/** + * Non-request outcome of {@link ApprovalService.openNodeRequest}: the node + * resolved an empty approver slate and its `onEmptyApprovers: 'auto_approve'` + * policy waved it through (#3447 P2). No `sys_approval_request` row exists — + * nobody was ever asked — so the node must complete down its `approve` edge + * instead of suspending. + */ +export interface ApprovalNodeAutoOutcome { + autoApproved: true; + reason: 'empty_approvers'; +} + function uid(prefix: string): string { const g: any = globalThis as any; if (g.crypto?.randomUUID) return `${prefix}_${g.crypto.randomUUID()}`; @@ -390,7 +428,19 @@ export class ApprovalService implements IApprovalService { step: any, record?: any, organizationId?: string | null, - opts?: { now?: number; substitutions?: OooSubstitution[]; groups?: Record }, + opts?: { + now?: number; + substitutions?: OooSubstitution[]; + groups?: Record; + /** #3447 P2: `trigger`/`vars` roots for `expression` approvers. */ + exprCtx?: ApproverExpressionContext; + /** + * #3447 P2 audit collector: what each dynamic spec resolved FROM (the + * live field value / the expression's intermediate values), snapshotted + * as `__resolvedFrom` so "why these people" stays answerable later. + */ + resolvedFrom?: Record; + }, ): Promise { if (!step || !Array.isArray(step.approvers)) return []; const now = opts?.now ?? this.clock.now().getTime(); @@ -399,11 +449,34 @@ export class ApprovalService implements IApprovalService { for (let idx = 0; idx < specs.length; idx++) { const a = specs[idx]; if (!a) continue; - const ids = await this.resolveApproverSpec(a, record, organizationId, now, opts?.substitutions); - // per_group (#3266): tag each resolved id with this spec's group. An - // approver without an explicit `group` forms its own group keyed by - // position, so a plain per-approver list still behaves predictably. + // Approvers without an explicit `group` each form their own group keyed + // by position (#3266), so a plain per-approver list behaves predictably. const groupKey = a.group != null && String(a.group) !== '' ? String(a.group) : `#${idx}`; + + // #3447 P2: `expression` approvers resolve OUTSIDE resolveApproverSpec — + // a graph-expanded expression (resolveAs: department/…) must key each + // intermediate value as its own per_group group, which the flat string[] + // contract of resolveApproverSpec cannot carry. + if (canonicalApproverType(String(a.type)) === 'expression') { + const resolved = await this.resolveExpressionApprovers( + a, record, organizationId, now, opts?.substitutions, opts?.exprCtx, + ); + if (opts?.resolvedFrom) opts.resolvedFrom[`expression#${idx}`] = resolved.raw; + for (const entry of resolved.slots) { + if (!entry.id) continue; + out.push(entry.id); + if (opts?.groups) { + (opts.groups[entry.id] ??= []).push(entry.subGroup ? `${groupKey}:${entry.subGroup}` : groupKey); + } + } + continue; + } + + if (opts?.resolvedFrom && canonicalApproverType(String(a.type)) === 'field' && a.value != null) { + opts.resolvedFrom[`field:${a.value}`] = (record as any)?.[a.value] ?? null; + } + const ids = await this.resolveApproverSpec(a, record, organizationId, now, opts?.substitutions); + // per_group (#3266): tag each resolved id with this spec's group. for (const u of ids) { if (!u) continue; out.push(u); @@ -477,6 +550,134 @@ export class ApprovalService implements IApprovalService { return [`${a.type}:${a.value}`]; } + /** + * Resolve an `expression` approver (#3447 P2): evaluate its CEL source at + * node entry against the three explicit roots — `current` (live record), + * `trigger` (submit snapshot), `vars` (flow variables) — then expand the + * result into people per `resolveAs`. + * + * Every failure here THROWS (config/parse errors as `VALIDATION_FAILED`, + * evaluation faults as `EXPRESSION_FAILED`) so the approval node fails + * loudly instead of opening a request routed to nobody — an approver + * expression that cannot run is a routing bug, never "condition not met". + * Error messages carry the correct spelling because their primary reader is + * the AI author fixing the flow on the next validate pass. + * + * Returns `slots` (approver id + optional per_group sub-key) and `raw` (the + * expression's own values, pre-expansion) for the `__resolvedFrom` audit. + */ + private async resolveExpressionApprovers( + a: any, + liveRecord: any, + organizationId: string | null | undefined, + now: number, + substitutions?: OooSubstitution[], + exprCtx?: ApproverExpressionContext, + ): Promise<{ slots: Array<{ id: string; subGroup?: string }>; raw: string[] }> { + const source = String(a.value ?? '').trim(); + if (!source) { + throw new Error('VALIDATION_FAILED: expression approver has an empty expression'); + } + + // Closed-root pre-check. The runtime env resolves ANY unknown root as dyn → + // null, so `record.x` / a bare field would silently yield an empty slate; + // reject it here with the correct spelling instead. + const parsed = collectCelRootIdentifiers(source); + if (!parsed.ok) { + throw new Error(`VALIDATION_FAILED: expression approver does not parse: ${parsed.error} — source: \`${source}\``); + } + const illegal = parsed.roots.filter(r => !APPROVER_EXPRESSION_ROOTS.has(r)); + if (illegal.length) { + const hint = illegal.includes('record') || illegal.includes('previous') + ? `\`record\`/\`previous\` are not bound here — write \`current.\` for the record's live state ` + + `at node entry, or \`trigger.\` for the submit-time snapshot (\`vars.previous\` carries the pre-update row)` + : `did you mean \`current.\` (live record), \`trigger.\` (submit snapshot), or \`vars.\` (flow variable)?`; + throw new Error( + `VALIDATION_FAILED: expression approver references \`${illegal.join('`, `')}\` — ` + + `only \`current.*\`, \`trigger.*\` and \`vars.*\` are available; ${hint}. Source: \`${source}\``, + ); + } + + const result = ExpressionEngine.evaluate( + { dialect: 'cel', source }, + { extra: { current: liveRecord ?? {}, trigger: exprCtx?.trigger ?? {}, vars: exprCtx?.vars ?? {} } }, + ); + if (!result.ok) { + throw new Error( + `EXPRESSION_FAILED: expression approver failed to evaluate (${result.error.kind}): ` + + `${result.error.message} — source: \`${source}\``, + ); + } + + // Normalize to a string list: a user-id/CSV string, an array of ids, or + // null/empty (an EMPTY slate — legal, handled by onEmptyApprovers). Any + // other shape is a config bug, rejected loudly. + const value = result.value as unknown; + let raw: string[]; + if (value == null || value === '') { + raw = []; + } else if (typeof value === 'string') { + raw = csvSplit(value); + } else if (Array.isArray(value)) { + const bad = value.find(v => v != null && typeof v !== 'string' && typeof v !== 'number'); + if (bad !== undefined) { + throw new Error( + `EXPRESSION_FAILED: expression approver must yield ids (string / CSV / string array), ` + + `got an array containing ${typeof bad} — source: \`${source}\``, + ); + } + raw = value.map(v => String(v ?? '').trim()).filter(Boolean); + } else { + throw new Error( + `EXPRESSION_FAILED: expression approver must yield ids (string / CSV / string array), ` + + `got ${typeof value} — source: \`${source}\``, + ); + } + + // `resolveAs` expansion. `user` (default): each value IS a person — + // individually routed, so OOO delegation applies (#1322). Graph kinds + // re-expand each value through the same lookups the static types use; a + // group still has its other members, so like the static graph types they + // are NOT OOO-substituted, and with per_group each intermediate value + // forms its own sub-group (one sign-off per returned department). A value + // whose expansion is empty keeps a `:` literal slot — same + // unstaffed-target behaviour (and #3424 admin rescue) as the static types. + const resolveAs = String(a.resolveAs ?? 'user'); + if (resolveAs === 'user') { + const slots: Array<{ id: string }> = []; + for (const id of raw) { + for (const routed of await this.applyOooDelegation(id, now, organizationId, substitutions)) { + slots.push({ id: routed }); + } + } + return { slots, raw }; + } + const slots: Array<{ id: string; subGroup: string }> = []; + for (const key of raw) { + let users: string[] = []; + try { + if (resolveAs === 'department') users = await this.expandBusinessUnitUsers(key, organizationId); + else if (resolveAs === 'position') users = await this.expandPositionUsers(key, organizationId); + else if (resolveAs === 'team') users = await this.expandTeamUsers(key); + else { + throw new Error( + `VALIDATION_FAILED: expression approver has unknown resolveAs '${resolveAs}' — ` + + `use 'user', 'department', 'position', or 'team'`, + ); + } + } catch (err: any) { + if (String(err?.message ?? '').startsWith('VALIDATION_FAILED')) throw err; + users = []; + } + if (!users.length) { + slots.push({ id: `${resolveAs}:${key}`, subGroup: key }); + continue; + } + for (const u of users) slots.push({ id: u, subGroup: key }); + } + return { slots, raw }; + } + /** Flat team — `sys_team` is better-auth's collaboration grouping (no hierarchy). */ private async expandTeamUsers(teamId: string): Promise { if (!teamId) return []; @@ -721,6 +922,10 @@ export class ApprovalService implements IApprovalService { * Open a pending approval request on behalf of a flow's Approval node. The * node config (approvers / behavior / status field) is snapshotted on the row * so a decision can be made without any process to resolve against. + * + * #3447 P2: may instead return an {@link ApprovalNodeAutoOutcome} — no + * request opened — when the slate resolves empty and the node's + * `onEmptyApprovers` policy is `auto_approve`. */ async openNodeRequest( input: { @@ -737,9 +942,15 @@ export class ApprovalService implements IApprovalService { submitterId?: string | null; record?: any; organizationId?: string | null; + /** + * #3447 P2: flow variables at node entry (nested by dotted key, as the + * engine's CEL conditions see them) — the `vars.*` root for `expression` + * approvers. `input.record` doubles as their `trigger.*` root. + */ + variables?: Record | null; }, context: SharingExecutionContext, - ): Promise { + ): Promise { if (!input.object) throw new Error('VALIDATION_FAILED: object is required'); if (!input.recordId) throw new Error('VALIDATION_FAILED: recordId is required'); if (!input.runId) throw new Error('VALIDATION_FAILED: runId is required'); @@ -766,17 +977,40 @@ export class ApprovalService implements IApprovalService { // the trigger snapshot carried in `input.record`. This is the whole fix — an // earlier step may have written the field this node routes on. const liveRecord = await this.loadLiveRecord(input.object, input.recordId, input.record); + const resolvedFrom: Record = {}; const approvers = await this.expandApprovers( - { approvers: input.config.approvers }, liveRecord, ctxOrg, { now: nowDate.getTime(), substitutions, groups }, + { approvers: input.config.approvers }, liveRecord, ctxOrg, { + now: nowDate.getTime(), substitutions, groups, + exprCtx: { trigger: input.record ?? null, vars: input.variables ?? null }, + resolvedFrom, + }, ); - // #3424: an approval routed to a target with no holders (e.g. an unstaffed - // `position`) resolves to only unresolvable `type:value` literals — no - // concrete user can act. The request is still opened (a privileged admin can - // override it, and legacy 15.x literal slots stay queryable), but warn - // loudly so the misconfiguration surfaces instead of silently locking the - // record with no obvious cause. + // Empty-slate policy (#3447 P2). "Empty" = no CONCRETE person — an + // unstaffed position / empty expression result leaves only `type:value` + // literal slots, decidable by nobody. if (!approvers.some(a => a && !a.includes(':'))) { + const emptyPolicy = (input.config as any).onEmptyApprovers ?? 'admin_rescue'; + if (emptyPolicy === 'fail') { + throw new Error( + `NO_APPROVERS: approval node '${input.nodeId}' on ${input.object}/${input.recordId} resolved to no ` + + `concrete approver and its onEmptyApprovers policy is 'fail'. Check that the approver target(s) ` + + `are staffed / the routing field or expression yields user ids at node entry.`, + ); + } + if (emptyPolicy === 'auto_approve') { + this.logger?.warn?.( + `[approvals] approval node '${input.nodeId}' on ${input.object}/${input.recordId} resolved to no ` + + `concrete approver — auto-approving per onEmptyApprovers: 'auto_approve' (no request opened).`, + { object: input.object, recordId: input.recordId, node: input.nodeId, resolved: approvers }, + ); + return { autoApproved: true, reason: 'empty_approvers' }; + } + // #3424 admin_rescue (default): the request is still opened (a privileged + // admin can override it, and legacy 15.x literal slots stay queryable) — + // the only option that neither waves the record through nor kills the + // run — but warn loudly so the misconfiguration surfaces instead of + // silently locking the record with no obvious cause. this.logger?.warn?.( `[approvals] approval node '${input.nodeId}' on ${input.object}/${input.recordId} resolved to no concrete approver` + ' — the request is decidable only by a privileged admin. Check that the approver target(s) are staffed.', @@ -792,10 +1026,23 @@ export class ApprovalService implements IApprovalService { const configSnapshot: any = { ...input.config }; if (input.flowLabel) configSnapshot.__flowLabel = input.flowLabel; if (input.nodeLabel) configSnapshot.__nodeLabel = input.nodeLabel; - // Snapshot the resolved approver→group map for quorum/per_group tallying. - if (input.config.behavior === 'quorum' || input.config.behavior === 'per_group') { + // Snapshot the resolved approver→group map for EVERY multi-approver + // behavior (was quorum/per_group only). #3447 P2 makes this load-bearing + // for unanimous too: an `expression` approver can only resolve at OPEN + // time (decide has no flow variables to evaluate against), so the tally + // must read the open-time slate — which also pins unanimous+field to the + // slate the approvers actually saw, instead of re-reading a field that may + // have changed again since. + if (input.config.behavior && input.config.behavior !== 'first_response') { configSnapshot.__approverGroups = groups; } + // #3447 P2: snapshot what the dynamic approver sources resolved FROM (the + // live routing-field value / the expression's intermediate values) so the + // audit trail answers "why these people" — the resolution INPUT, pairing + // the resolution RESULT already persisted as `pending_approvers`. + if (Object.keys(resolvedFrom).length) { + configSnapshot.__resolvedFrom = resolvedFrom; + } // ADR-0044 round numbering: rounds of a revise loop share the run — count // this (run, node)'s prior requests; the new one is round N+1. Stamped on // the snapshot (precedent: __flowLabel), so no schema migration. @@ -925,9 +1172,9 @@ export class ApprovalService implements IApprovalService { */ async decideNode( requestId: string, - input: { decision: 'approve' | 'reject'; actorId: string; comment?: string; attachments?: string[] }, + input: { decision: 'approve' | 'reject'; actorId: string; comment?: string; attachments?: string[]; outputs?: Record }, context: SharingExecutionContext, - ): Promise<{ request: ApprovalRequestRow; runId: string | null; nodeId: string | null; finalized: boolean; decision: 'approve' | 'reject' }> { + ): Promise<{ request: ApprovalRequestRow; runId: string | null; nodeId: string | null; finalized: boolean; decision: 'approve' | 'reject'; outputs?: Record }> { if (!requestId) throw new Error('VALIDATION_FAILED: requestId is required'); if (!input?.actorId) throw new Error('VALIDATION_FAILED: actorId is required'); if (input.decision !== 'approve' && input.decision !== 'reject') { @@ -958,6 +1205,41 @@ export class ApprovalService implements IApprovalService { const runId: string | null = raw.flow_run_id ?? null; const now = this.clock.now().toISOString(); + // #3447 P2: decision outputs — validated BEFORE any write (audit included) + // so an out-of-contract payload rejects atomically. The trust model is a + // `screen` node's: the AUTHOR declares the keys (`config.decisionOutputs`), + // the approver only fills values. A decision carrying undeclared keys is a + // caller bug; `decision`/`requestId` are reserved by the resume envelope. + const outputKeys = input.outputs ? Object.keys(input.outputs) : []; + let acceptedOutputs: Record | undefined; + if (outputKeys.length) { + const declared = Array.isArray((config as any).decisionOutputs) + ? ((config as any).decisionOutputs as unknown[]).map(String) + : []; + if (!declared.length) { + throw new Error( + `VALIDATION_FAILED: this approval node declares no decisionOutputs — outputs are not accepted. ` + + `Declare the keys on the node config (decisionOutputs: [${outputKeys.map(k => `'${k}'`).join(', ')}]) ` + + `to let approvers hand them to the flow.`, + ); + } + const reserved = outputKeys.filter(k => k === 'decision' || k === 'requestId'); + if (reserved.length) { + throw new Error( + `VALIDATION_FAILED: decision output key(s) \`${reserved.join('`, `')}\` are reserved by the resume ` + + `envelope — pick different names.`, + ); + } + const undeclared = outputKeys.filter(k => !declared.includes(k)); + if (undeclared.length) { + throw new Error( + `VALIDATION_FAILED: decision output key(s) \`${undeclared.join('`, `')}\` are not declared on this ` + + `node — declared keys: ${declared.map(k => `'${k}'`).join(', ') || '(none)'}.`, + ); + } + acceptedOutputs = { ...input.outputs }; + } + // Audit the decision first so the quorum/per_group tally below sees it. await this.engine.insert('sys_approval_action', { id: uid('aact'), request_id: requestId, organization_id: org, @@ -982,13 +1264,16 @@ export class ApprovalService implements IApprovalService { }); const approved = new Set((acts ?? []).map((a: any) => String(a.actor_id ?? '')).filter(Boolean)); - // quorum / per_group tally against the OPEN-time snapshot (already - // OOO-substituted). unanimous re-resolves for back-compat with requests - // opened before the snapshot existed. + // Tally against the OPEN-time snapshot (already OOO-substituted) for + // every behavior that carries one. Re-resolution survives ONLY as the + // back-compat path for requests opened before the snapshot existed — + // it cannot ever run for an `expression` approver (#3447 P2: decide has + // no flow variables to evaluate against; open time is the only + // resolution point), and those always have a snapshot. const snapshotGroups = (config as any).__approverGroups as Record | undefined; let original: string[]; let groupMap: Record; - if (snapshotGroups && (behavior === 'quorum' || behavior === 'per_group')) { + if (snapshotGroups) { groupMap = snapshotGroups; original = Object.keys(snapshotGroups); } else { @@ -1002,6 +1287,16 @@ export class ApprovalService implements IApprovalService { const stillPending = original.filter(a => !approved.has(a)); await this.engine.update('sys_approval_request', { id: requestId, pending_approvers: stillPending.join(','), updated_at: now, + // #3447 P2: a mid-tally approval may carry outputs too (unanimous / + // per_group co-sign, each approver contributing their declared keys) + // — accumulate them on the snapshot so the FINALIZING decision hands + // the merged set to the flow. + ...(acceptedOutputs ? { + node_config_json: JSON.stringify({ + ...config, + __decisionOutputs: { ...((config as any).__decisionOutputs ?? {}), ...acceptedOutputs }, + }), + } : {}), }, { context: SYSTEM_CTX }); await this.syncApproverIndex(requestId, stillPending, org, now); const fresh = await this.getRequest(requestId, context); @@ -1010,15 +1305,25 @@ export class ApprovalService implements IApprovalService { } const finalStatus = input.decision === 'approve' ? 'approved' : 'rejected'; + // #3447 P2: the full accumulated output set — earlier co-sign votes' plus + // this finalizing decision's — resumes the run and stays snapshotted for + // the audit trail ("what did the approvers hand the flow"). + const mergedOutputs: Record | undefined = + acceptedOutputs || (config as any).__decisionOutputs + ? { ...((config as any).__decisionOutputs ?? {}), ...(acceptedOutputs ?? {}) } + : undefined; await this.engine.update('sys_approval_request', { id: requestId, status: finalStatus, pending_approvers: null, completed_at: now, updated_at: now, + ...(mergedOutputs ? { + node_config_json: JSON.stringify({ ...config, __decisionOutputs: mergedOutputs }), + } : {}), }, { context: SYSTEM_CTX }); await this.syncApproverIndex(requestId, [], org, now); if (config.approvalStatusField) { await this.mirrorStatusField(raw.object_name, raw.record_id, config.approvalStatusField, finalStatus); } const fresh = await this.getRequest(requestId, context); - return { request: fresh!, runId, nodeId, finalized: true, decision: input.decision }; + return { request: fresh!, runId, nodeId, finalized: true, decision: input.decision, outputs: mergedOutputs }; } /** @@ -1041,7 +1346,12 @@ export class ApprovalService implements IApprovalService { try { await this.automation.resume(result.runId, { branchLabel, - output: { decision: result.decision, requestId }, + // #3447 P2: accepted decision outputs ride the resume envelope and + // land as `.` flow variables — a later approval node's + // `expression` approver reads them as `vars..`. + // Reserved keys are spread LAST so no output can shadow them (the + // whitelist already rejects them; this is defense in depth). + output: { ...(result.outputs ?? {}), decision: result.decision, requestId }, }); resumed = true; } catch (err: any) { diff --git a/packages/plugins/plugin-approvals/src/index.ts b/packages/plugins/plugin-approvals/src/index.ts index e84a1c90bb..b72a388b4f 100644 --- a/packages/plugins/plugin-approvals/src/index.ts +++ b/packages/plugins/plugin-approvals/src/index.ts @@ -20,6 +20,9 @@ export { type ApprovalClock, type ApprovalServiceOptions, type ApprovalResumeSurface, + // #3447 P2 — expression approvers + empty-slate auto-approve outcome. + type ApproverExpressionContext, + type ApprovalNodeAutoOutcome, } from './approval-service.js'; export { ApprovalsServicePlugin, diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index 4d56fae6a9..78f87a2acb 100644 --- a/packages/rest/src/rest-server.ts +++ b/packages/rest/src/rest-server.ts @@ -6058,6 +6058,10 @@ export class RestServer { actorId: body.actorId ?? body.actor_id ?? context?.userId, comment: body.comment, attachments: body.attachments, + // #3447 P2: author-declared decision outputs — the + // service validates keys against the node's + // `decisionOutputs` whitelist before any write. + outputs: body.outputs, }, context ?? {}); res.json(out); } catch (err: any) { diff --git a/packages/services/service-automation/src/engine.ts b/packages/services/service-automation/src/engine.ts index 5db902f4d1..b4857fd92f 100644 --- a/packages/services/service-automation/src/engine.ts +++ b/packages/services/service-automation/src/engine.ts @@ -2751,6 +2751,19 @@ export class AutomationEngine implements IAutomationService { if (result.suspend) { throw new FlowSuspendSignal(node.id, result.correlation, result.screen); } + + // #3447 P2: an executor may pick its own out-edge without suspending + // (e.g. an approval node auto-approving an empty slate walks its + // `approve` edge directly). The field predates this — it was declared + // on NodeExecutionResult "for decision nodes" but never consumed on + // the synchronous path; only resume() honoured its signal twin. On a + // labelled-edge node, falling through to the unlabelled traversal + // would walk EVERY unconditional out-edge (approve AND reject), so + // the label must be honoured here. + if (result.branchLabel) { + await this.traverseNext(node, flow, variables, context, steps, result.branchLabel); + return; + } } // Continue to the node's successors. diff --git a/packages/spec/src/automation/approval.zod.ts b/packages/spec/src/automation/approval.zod.ts index 4dc85ea2cb..a22fc556be 100644 --- a/packages/spec/src/automation/approval.zod.ts +++ b/packages/spec/src/automation/approval.zod.ts @@ -26,7 +26,18 @@ export const ApproverType = z.enum([ 'department', // Members of a department + all descendant departments (sys_business_unit) 'manager', // Submitter's manager (sys_user.manager_id) 'field', // User ID defined in a record field - 'queue' // Data ownership queue + 'queue', // Data ownership queue + /** + * #3447 P2: a CEL expression resolved AT NODE ENTRY against three explicit + * roots — `current.*` (the record's live state), `trigger.*` (the submit-time + * snapshot) and `vars.*` (flow variables, incl. upstream node outputs). The + * result (a user-id string, CSV, or string array — or intermediate ids + * re-expanded per `resolveAs`) becomes the approver slate. `record` and bare + * field names are deliberately NOT available: `record` means "the record at + * event time" everywhere else on the platform (flow conditions, hooks), and + * reusing it here would silently alias one of the two times. + */ + 'expression' ]); /** @@ -125,8 +136,12 @@ export const ApprovalNodeApproverSchema = lazySchema(() => z.object({ // The `role` → `org-membership-level` picker kind is the deprecated alias's // entry: it maps to the SAME picker as the canonical spelling, so a stored // legacy node still renders correctly for its deprecation window. + // `expression` is intentionally ABSENT from the map: an unmapped type keeps + // the value as free text, so the designer renders a plain input for the CEL + // source until a dedicated expression editor lands (objectui follow-up). value: z.string().optional().meta({ - description: 'User id / membership tier / position / team / department / field / queue — per `type`', + description: 'User id / membership tier / position / team / department / field / queue — per `type`; ' + + 'for `expression`, a CEL expression over `current.*` / `trigger.*` / `vars.*`', xRef: { kindFrom: 'type', objectSource: '$trigger', @@ -142,6 +157,18 @@ export const ApprovalNodeApproverSchema = lazySchema(() => z.object({ }, }, }), + /** + * #3447 P2, `expression` approvers only: how the expression's resolved values + * are turned into people. `user` (default) treats each value as a user id. + * `department` / `position` / `team` treat each value as that kind of id and + * expand it through the same graph lookups the static approver types use — + * e.g. an expression yielding department ids + `resolveAs: 'department'` + * fans out into every member of every returned department. With + * `behavior: 'per_group'`, each intermediate value forms its own group (one + * sign-off per returned department), keyed by that value. + */ + resolveAs: z.enum(['user', 'department', 'position', 'team']).optional() + .describe("How an `expression` result is expanded into approvers (default 'user')"), /** * Optional group label (#3266). With `behavior: 'per_group'`, approvers that * share a label form one group and the node advances only once EACH group has @@ -237,6 +264,37 @@ export const ApprovalNodeConfigSchema = lazySchema(() => z.object({ xRef: { kind: 'object-field', objectSource: '$trigger' }, }), + /** + * #3447 P2: what happens when approver resolution yields NO concrete person + * at node entry (an empty expression result, an unstaffed position, an empty + * multi-select field, …): + * - `admin_rescue` (default) — open the request anyway and warn loudly; a + * privileged admin can take it over via Reassign (#3424). The only option + * that neither waves the record through nor kills the run — approval's job + * is to gatekeep, so an empty slate must never silently pass. + * - `fail` — the node fails (fault edge / run failure). Choose when an empty + * slate can only mean a configuration bug. + * - `auto_approve` — skip the request and continue down the `approve` edge + * with `output.autoApproved = true`. The DingTalk/Feishu default; opt-in + * here because it silently waves the record through. + */ + onEmptyApprovers: z.enum(['admin_rescue', 'fail', 'auto_approve']).default('admin_rescue') + .describe('Behavior when no concrete approver resolves at node entry'), + + /** + * #3447 P2: keys a decision may carry as structured outputs + * (`decide(..., { outputs })`). The AUTHOR declares the keys; approvers only + * fill values — the same trust model as a `screen` node's author-defined + * fields. Accepted outputs resume the run as `.` flow variables + * (never bare names, so an approver can't shadow author variables), where a + * later node's `expression` approver can read them + * (`vars..picked_departments`) — "the previous approver picks the + * next step's approvers" without a record-field detour. A decision carrying + * undeclared keys is rejected; `decision` / `requestId` are reserved. + */ + decisionOutputs: z.array(z.string()).optional() + .describe('Author-declared output keys a decision may carry (approvers fill values only)'), + /** Optional per-node SLA escalation. */ escalation: ApprovalEscalationSchema.optional().describe('Per-node SLA escalation'), diff --git a/packages/spec/src/contracts/approval-service.ts b/packages/spec/src/contracts/approval-service.ts index 296bc5ace8..a0fa78707a 100644 --- a/packages/spec/src/contracts/approval-service.ts +++ b/packages/spec/src/contracts/approval-service.ts @@ -204,6 +204,16 @@ export interface ApprovalDecisionInput { * the `sys_approval_action` audit row's `attachments` field. */ attachments?: string[]; + /** + * #3447 P2: structured outputs the approver hands to the flow with their + * decision. Keys MUST be declared on the node's `decisionOutputs` config — + * the author declares keys, approvers only fill values (a `screen` node's + * trust model); a decision carrying undeclared keys is rejected, and + * `decision` / `requestId` are reserved. Accepted outputs resume the run as + * `.` flow variables, where a later approval node's + * `expression` approver can read them (`vars..picked_departments`). + */ + outputs?: Record; } /** Input for recalling (withdrawing) a pending request. */ diff --git a/skills/objectstack-automation/SKILL.md b/skills/objectstack-automation/SKILL.md index adeb9d196b..e9c3d32b42 100644 --- a/skills/objectstack-automation/SKILL.md +++ b/skills/objectstack-automation/SKILL.md @@ -422,6 +422,15 @@ A decision is recorded through `ApprovalService.decide()` (or the REST routes `sys_approval_request` and **resumes** the suspended run down the matching branch — you never resume the flow by hand. +A decision may also carry **structured outputs** (`{ outputs: { … } }` in the +decide body) when the node declares the keys in `decisionOutputs` — the author +declares keys, approvers only fill values. Accepted outputs resume the run as +`.` flow variables, so a LATER node reads them as +`vars..` — this is how "the previous approver picks the next +step's approvers" works without writing to a record field (see Dynamic +approvers below). A decision carrying an undeclared key is rejected; +`decision` / `requestId` are reserved. + ### Approver Types | `type` | Resolves to | @@ -432,8 +441,71 @@ branch — you never resume the flow by hand. | `team` | Members of a flat `sys_team` | | `department` | A department + all descendant departments | | `manager` | The submitter's manager (`sys_user.manager_id`) | -| `field` | User id read from a record field (`value` = field name) | +| `field` | User id read from a record field (`value` = field name). Resolved against the record's **live** state at node entry (#3447), so a field written mid-flow routes correctly; a multi-select user field fans out into one approver per user | | `queue` | A data-ownership queue | +| `expression` | A **CEL expression** resolved at node entry (`value` = the expression) — see **Dynamic approvers** below. Only `current.*` / `trigger.*` / `vars.*` roots are available; the optional `resolveAs: 'user'(default) \| 'department' \| 'position' \| 'team'` re-expands each resolved id through the graph | + +### Dynamic approvers (`type: 'expression'`, #3447) + +An `expression` approver computes WHO approves at the moment the node is +entered. Its CEL source sees exactly **three roots** — nothing else: + +| Root | Meaning | Analog | +|:-----|:--------|:-------| +| `current.*` | The record's **live** state at node entry — fields written by earlier steps/approvers are visible | ServiceNow `current` | +| `trigger.*` | The **submit-time snapshot** (what flow conditions call `record`) | ServiceNow Flow Designer `trigger.record`, Power Automate `triggerBody()` | +| `vars.*` | Flow variables — node outputs (`vars..`), `get_record` results, `vars.previous` (the pre-update row) | BPMN process variables | + +**`record` and bare field names are NOT available and fail the node loudly.** +Everywhere else on this platform `record` means "the record at event time" +(flow conditions: the trigger snapshot; hooks: the write payload) — at an +approval node that phrase is ambiguous between two different times, so you must +say which one: `current.x` or `trigger.x`. Do not carry the `record.x` habit +over from conditions. + +Result contract: a user-id string, a CSV string, or an array of ids. An **empty** +result (present-but-empty field/variable) triggers `onEmptyApprovers`. A +**missing** key (`vars.never_written`) is a loud error, never a silent empty +slate — guard genuinely-optional inputs explicitly, e.g. +`has(vars.picked) ? vars.picked : []`. + +```typescript +// ① Route on a field an EARLIER approver filled in mid-flow (live value): +{ type: 'expression', value: cel`current.co_review_departments`, resolveAs: 'department' } + +// ② The previous approval node's decision outputs pick this node's approvers: +{ type: 'expression', value: cel`vars.lead_review.next_reviewers` } + +// ③ Dynamic co-sign (会签): expression yields department ids; resolveAs expands +// each into its members, and with behavior: 'per_group' EACH department is +// its own sign-off group: +{ + approvers: [{ type: 'expression', value: cel`current.picked_departments`, resolveAs: 'department' }], + behavior: 'per_group', + onEmptyApprovers: 'fail', +} +``` + +The full "previous approver picks the next step's approvers" loop, end to end: + +```typescript +// Node A declares what a decision may hand to the flow: +{ id: 'lead_review', type: 'approval', + config: { approvers: [{ type: 'user', value: 'lead' }], decisionOutputs: ['next_reviewers'] } }, +// The lead approves with outputs: POST …/approve { outputs: { next_reviewers: ['u2','u3'] } } +// Node B resolves them at entry: +{ id: 'co_sign', type: 'approval', + config: { approvers: [{ type: 'expression', value: cel`vars.lead_review.next_reviewers` }], + behavior: 'unanimous', onEmptyApprovers: 'fail' } }, +``` + +Time-word cheat sheet across surfaces (do not mix them up): + +| Surface | Event-time record | Pre-event record | Live record | +|:--------|:------------------|:-----------------|:------------| +| Flow condition / `{…}` template | `record` (trigger snapshot) | `previous` | — (use a `get_record` node) | +| Object hook (`ctx`) | `ctx.record` (write payload) | `ctx.previous` | — | +| Approval `expression` approver | `trigger.*` | `vars.previous` | `current.*` | ### Node Config (`ApprovalNodeConfigSchema`) @@ -444,6 +516,8 @@ branch — you never resume the flow by hand. | `minApprovals` | Approvals required — total for `quorum`, per group for `per_group`. Default `1`; clamped at runtime to the resolvable approver count so a misconfiguration can never deadlock | | `lockRecord` | Lock the triggering record from edits while pending. Default `true` | | `approvalStatusField` | Business-object field to mirror `pending`/`approved`/`rejected`/`recalled` onto (should be readonly) | +| `onEmptyApprovers` | #3447 — what an EMPTY resolved slate does: `admin_rescue` (default — request opens, only a privileged admin can act via Reassign; never waves through, never kills the run), `fail` (node fails — treat an empty slate as a config bug), `auto_approve` (skip the request, continue down `approve` with `output.autoApproved = true` — opt-in because it silently waves the record through). Declare it explicitly on any node with an `expression` approver (linted) | +| `decisionOutputs` | #3447 — keys a decision may carry as structured outputs (author declares keys, approvers fill values). Accepted outputs resume the run as `.` variables for downstream nodes; undeclared keys reject the decision; `decision`/`requestId` reserved | | `escalation` | Optional per-node SLA — `{ enabled, timeoutHours, action: reassign\|auto_approve\|auto_reject\|notify, escalateTo?, notifySubmitter }`. `escalateTo` is a **position machine name** (expanded to its holders via `sys_user_position`, ADR-0090 D3) or a specific user id — never a membership tier. `reassign` without `escalateTo` degrades to notify (linted) | | `maxRevisions` | ADR-0044 — max **send-backs-for-revision** per run before auto-reject. Default `3`; `0` disables send-back. Only meaningful when the node has a `revise` out-edge |