Skip to content

Commit 2fa4ca1

Browse files
os-zhuangclaude
andauthored
feat(approvals): expression approvers, empty-slate policy, decision outputs (#3447 P2) (#3532)
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 <nodeId>.<key> variables, so a later node's expression reads vars.<nodeId>.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: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 parent 605e190 commit 2fa4ca1

19 files changed

Lines changed: 1310 additions & 36 deletions

File tree

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
---
2+
"@objectstack/spec": minor
3+
"@objectstack/formula": minor
4+
"@objectstack/lint": minor
5+
"@objectstack/service-automation": minor
6+
"@objectstack/plugin-approvals": minor
7+
"@objectstack/rest": minor
8+
---
9+
10+
Dynamic approver routing for approval nodes (#3447 P2) — three new declarative capabilities:
11+
12+
**`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.
13+
14+
**`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.
15+
16+
**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 `<nodeId>.<key>` variables — a later approval node's expression can read `vars.<nodeId>.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).
17+
18+
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.

content/docs/automation/approvals.mdx

Lines changed: 51 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -143,8 +143,50 @@ one node, no parallel branches needed:
143143
A single **rejection** always finalizes the node as `rejected` (one veto), in
144144
every mode. `minApprovals` is clamped to the resolvable approver count / group
145145
size, so it can never deadlock. A decision may also carry `attachments` (file
146-
references) recorded on its audit row — e.g. a signed contract. Weighted voting
147-
and approval-matrix governance are enterprise, not here.
146+
references) recorded on its audit row — e.g. a signed contract — and, when the
147+
node declares `decisionOutputs`, structured `outputs` the flow receives as
148+
`<nodeId>.<key>` variables (see [Dynamic approvers](#dynamic-approvers-3447)
149+
below). Weighted voting and approval-matrix governance are enterprise, not here.
150+
151+
## Dynamic approvers (#3447) [#dynamic-approvers-3447]
152+
153+
Two approver kinds bind to **live** data at the moment the node is entered —
154+
not the submit-time snapshot — so a step can route on data produced after
155+
submit:
156+
157+
- **`field`** reads the record's live field value at node entry. An earlier
158+
step (or an earlier step's approver) can write the routing field mid-flow and
159+
the next approval node sees it; a multi-select user field fans out into one
160+
approver slot per user.
161+
- **`expression`** computes the slate with a CEL expression over exactly three
162+
roots: **`current.*`** (the record's live state at node entry), **`trigger.*`**
163+
(the submit-time snapshot — what a flow condition calls `record`), and
164+
**`vars.*`** (flow variables, including node outputs and `vars.previous`).
165+
`record` and bare field names are deliberately unavailable — at an approval
166+
node "the record" is ambiguous between two times, so the expression must say
167+
which one; referencing anything else fails the node loudly at entry rather
168+
than resolving a silently-empty slate. The optional
169+
`resolveAs: 'user' (default) | 'department' | 'position' | 'team'` re-expands
170+
each resolved id through the same graph lookups the static types use — and
171+
with `behavior: 'per_group'`, each intermediate value (e.g. each returned
172+
department) forms its own sign-off group.
173+
174+
A result may legitimately be **empty** (a present-but-empty field or variable);
175+
the node-level `onEmptyApprovers` policy decides what that means —
176+
`admin_rescue` (default: the request opens, a privileged admin takes over via
177+
Reassign), `fail` (the node fails: an empty slate is a config bug), or
178+
`auto_approve` (skip the request and continue down `approve` with
179+
`output.autoApproved = true`; opt-in, since it waves the record through). A
180+
**missing** key (`vars.never_written`) is a loud error instead — guard
181+
genuinely-optional inputs with `has(vars.x) ? vars.x : []`.
182+
183+
The "previous approver picks the next step's approvers" loop needs no record
184+
field at all: declare `decisionOutputs: ['next_reviewers']` on node A, have the
185+
approver decide with `{ outputs: { next_reviewers: ['u2', 'u3'] } }`, and let
186+
node B's approver be
187+
`{ type: 'expression', value: 'vars.<nodeA-id>.next_reviewers' }`. The author
188+
declares output keys, approvers only fill values; undeclared keys reject the
189+
decision, and `decision` / `requestId` are reserved.
148190

149191
## The full lifecycle — submit to field change
150192

@@ -165,9 +207,12 @@ Only `approvers` is required on the node; everything else has a default
165207
(`behavior: 'first_response'`, `lockRecord: true`, `maxRevisions: 3`).
166208

167209
Approver entries resolve by kind — `position`, `user`, `field`, `manager`,
168-
`team`, `department`, `queue`, and `org_membership_level`, described in the
169-
[callout above](#3-the-approval-node), which is the one that silently resolves
170-
to nobody when it's mistaken for a business hierarchy. An entry that resolves
210+
`team`, `department`, `queue`, `org_membership_level`, and `expression`
211+
(described in the [callout above](#3-the-approval-node) and in
212+
[Dynamic approvers](#dynamic-approvers-3447)); `org_membership_level` is the
213+
one that silently resolves to nobody when it's mistaken for a business
214+
hierarchy. `field`, `manager`, and `expression` resolve against the record's
215+
**live** state at node entry (#3447). An entry that resolves
171216
to nobody is not an error: the request opens with an empty `pending_approvers`
172217
and nothing can move it, so the run parks forever.
173218

@@ -246,7 +291,7 @@ continuity levers on the same request. All are `POST
246291

247292
| Verb | Route | Who | Effect |
248293
|:---|:---|:---|:---|
249-
| `approve` / `reject` | `/approve` `/reject` | pending approver | Records the decision (finalizes per `behavior`). Accepts `comment`, `attachments`. |
294+
| `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 `<nodeId>.<key>` variables. |
250295
| `reassign` | `/reassign` | pending approver | Hands one slot to another user (`to`); the request stays pending. |
251296
| `revise` (send back) | `/revise` | pending approver | Ends this round as **`returned`**, unlocks the record for rework (ADR-0044). |
252297
| `request-info` | `/request-info` | pending approver | Asks the submitter for more info; the request **stays pending**. `comment` required. |

content/docs/references/automation/approval.mdx

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -54,8 +54,9 @@ const result = ApprovalDecision.parse(data);
5454

5555
| Property | Type | Required | Description |
5656
| :--- | :--- | :--- | :--- |
57-
| **type** | `Enum<'user' \| 'org_membership_level' \| 'role' \| 'position' \| 'team' \| 'department' \| 'manager' \| 'field' \| 'queue'>` || |
58-
| **value** | `string` | optional | User id / membership tier / position / team / department / field / queue — per `type` |
57+
| **type** | `Enum<'user' \| 'org_membership_level' \| 'role' \| 'position' \| 'team' \| 'department' \| 'manager' \| 'field' \| 'queue' \| 'expression'>` || |
58+
| **value** | `string` | optional | User id / membership tier / position / team / department / field / queue — per `type`; for `expression`, a CEL expression over `current.*` / `trigger.*` / `vars.*` |
59+
| **resolveAs** | `Enum<'user' \| 'department' \| 'position' \| 'team'>` | optional | How an `expression` result is expanded into approvers (default 'user') |
5960
| **group** | `string` | optional | Group label for per_group sign-off (e.g. "legal", "finance") |
6061

6162

@@ -67,11 +68,13 @@ const result = ApprovalDecision.parse(data);
6768

6869
| Property | Type | Required | Description |
6970
| :--- | :--- | :--- | :--- |
70-
| **approvers** | `{ type: Enum<'user' \| 'org_membership_level' \| 'role' \| 'position' \| 'team' \| 'department' \| 'manager' \| 'field' \| 'queue'>; value?: string; group?: string }[]` || Allowed approvers for this node |
71+
| **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 |
7172
| **behavior** | `Enum<'first_response' \| 'unanimous' \| 'quorum' \| 'per_group'>` || How to combine multiple approvers |
7273
| **minApprovals** | `integer` | optional | Approvals required — total (quorum) or per group (per_group). Default 1 |
7374
| **lockRecord** | `boolean` || Lock the record from editing while pending |
7475
| **approvalStatusField** | `string` | optional | Business-object field to mirror request status onto |
76+
| **onEmptyApprovers** | `Enum<'admin_rescue' \| 'fail' \| 'auto_approve'>` || Behavior when no concrete approver resolves at node entry |
77+
| **decisionOutputs** | `string[]` | optional | Author-declared output keys a decision may carry (approvers fill values only) |
7578
| **escalation** | `{ enabled: boolean; timeoutHours: number; action: Enum<'reassign' \| 'auto_approve' \| 'auto_reject' \| 'notify'>; escalateTo?: string; … }` | optional | Per-node SLA escalation |
7679
| **maxRevisions** | `integer` || Max send-backs for revision before auto-reject (0 = send-back disabled) |
7780

@@ -91,6 +94,7 @@ const result = ApprovalDecision.parse(data);
9194
* `manager`
9295
* `field`
9396
* `queue`
97+
* `expression`
9498

9599

96100
---

packages/formula/src/cel-engine.test.ts

Lines changed: 46 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { describe, expect, it } from 'vitest';
22

3-
import { celEngine, rewriteTemporalEquality } from './cel-engine';
3+
import { celEngine, rewriteTemporalEquality, collectCelRootIdentifiers } from './cel-engine';
44
import { CEL_STDLIB_FUNCTIONS } from './validate';
55
import type { Expression } from '@objectstack/spec';
66

@@ -640,3 +640,48 @@ describe('celEngine', () => {
640640
});
641641
});
642642
});
643+
644+
// ── collectCelRootIdentifiers (#3447 P2) ────────────────────────────────────
645+
//
646+
// Root extraction for closed-root evaluation sites (approval `expression`
647+
// approvers). Lint and the runtime pre-check both consume this helper, so its
648+
// contract IS the contract of what those sites accept.
649+
650+
describe('collectCelRootIdentifiers', () => {
651+
const roots = (src: string) => {
652+
const r = collectCelRootIdentifiers(src);
653+
if (!r.ok) throw new Error(`expected parse ok: ${r.error}`);
654+
return r.roots.sort();
655+
};
656+
657+
it('collects distinct namespace roots from member chains', () => {
658+
expect(roots('current.approvers_dynamic')).toEqual(['current']);
659+
expect(roots('current.a.b.c == trigger.a && vars.step.result')).toEqual(['current', 'trigger', 'vars']);
660+
});
661+
662+
it('reports a bare identifier as a root (the closed-site smoking gun)', () => {
663+
expect(roots('approvers_dynamic')).toEqual(['approvers_dynamic']);
664+
expect(roots('record.x')).toEqual(['record']);
665+
});
666+
667+
it('never reports member names or function names', () => {
668+
expect(roots('size(vars.picked) > 0 ? vars.picked : trigger.fallback')).toEqual(['trigger', 'vars']);
669+
expect(roots('current.owner_id.startsWith("u")')).toEqual(['current']);
670+
});
671+
672+
it('walks indexes, lists and nested calls', () => {
673+
expect(roots('vars.picked[0] == current.ids[trigger.idx]')).toEqual(['current', 'trigger', 'vars']);
674+
expect(roots('[current.a, vars.b]')).toEqual(['current', 'vars']);
675+
});
676+
677+
it('accepts the #3306 null-guard ternary the same way compile does', () => {
678+
expect(roots('current.n > 0 ? current.who : null')).toEqual(['current']);
679+
});
680+
681+
it('reports a parse failure instead of an empty root set', () => {
682+
const bad = collectCelRootIdentifiers('current..');
683+
expect(bad.ok).toBe(false);
684+
const empty = collectCelRootIdentifiers(' ');
685+
expect(empty.ok).toBe(false);
686+
});
687+
});

packages/formula/src/cel-engine.ts

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,11 @@ const SCOPE_ROOTS = [
6060
// Master-detail inline grids inject the header record as `parent` for a
6161
// child field's `readonlyWhen`/`requiredWhen` predicate (ADR-0036, #1581).
6262
'parent',
63+
// Approval-node `expression` approvers (#3447 P2): the record's LIVE state
64+
// at node entry — bound only in that evaluation site, alongside `trigger`
65+
// (the submit-time snapshot) and `vars`. Declared here so the strict lint
66+
// env doesn't misread `current.x` as a bare field reference.
67+
'current',
6368
] as const;
6469

6570
/**
@@ -127,6 +132,51 @@ export function firstUndeclaredReference(
127132
return null;
128133
}
129134

135+
/**
136+
* The distinct top-level identifiers (namespace roots) a CEL expression
137+
* references — `current.x + vars.step.y` → `['current', 'vars']`, a bare
138+
* `amount > 100` → `['amount']`. Member names and function names are not
139+
* identifiers and are never reported.
140+
*
141+
* Built for evaluation sites that expose a CLOSED set of roots (#3447 P2:
142+
* approval-node `expression` approvers allow only `current`/`trigger`/`vars`).
143+
* Such a site must reject any other root BEFORE evaluating: the runtime env is
144+
* `unlistedVariablesAreDyn: true`, so an out-of-contract root (`record.x`, a
145+
* bare field) would otherwise evaluate to `null` and silently produce an empty
146+
* result instead of an error. Both the lint rule and the runtime pre-check
147+
* consume this one helper so the two can never drift.
148+
*
149+
* Returns `{ ok: false }` with the classifier's message when the source does
150+
* not parse — callers surface that as a config error, not an empty root set.
151+
*/
152+
export function collectCelRootIdentifiers(
153+
source: string,
154+
): { ok: true; roots: string[] } | { ok: false; error: string } {
155+
if (typeof source !== 'string' || !source.trim()) {
156+
return { ok: false, error: 'expression is empty' };
157+
}
158+
try {
159+
// Same nullable-ternary rewrite as compile/evaluate so "what parses" agrees
160+
// across build, lint, and runtime (#3306).
161+
const compiled = buildEnv(() => new Date(0)).parse(rewriteNullableTernary(source));
162+
const roots = new Set<string>();
163+
const walk = (node: unknown): void => {
164+
if (Array.isArray(node)) { for (const child of node) walk(child); return; }
165+
if (!isCelNode(node)) return; // member/function-name strings, literals
166+
if (node.op === 'id' && typeof node.args === 'string') { roots.add(node.args); return; }
167+
// Member access: only the receiver can hold identifiers — `args[1]` is the
168+
// member NAME, which must not be reported as a root.
169+
if (node.op === '.' && Array.isArray(node.args)) { walk(node.args[0]); return; }
170+
walk(node.args);
171+
};
172+
walk(compiled.ast);
173+
return { ok: true, roots: [...roots] };
174+
} catch (err) {
175+
const classified = classifyError(err);
176+
return { ok: false, error: classified.ok === false ? classified.error.message : String(err) };
177+
}
178+
}
179+
130180
/**
131181
* The result type cel-js's type-checker infers for a `value`/`predicate`
132182
* expression — its raw CEL type name (`'int'`, `'double'`, `'string'`, `'bool'`,

packages/formula/src/index.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,10 @@
1111

1212
export { ExpressionEngine, getEngine, hasDialect, register } from './registry';
1313
export { celEngine, DEFAULT_LIMITS } from './cel-engine';
14+
// #3447 P2 — root-identifier extraction for closed-root evaluation sites
15+
// (approval `expression` approvers): lint and the runtime pre-check share this
16+
// one helper so what they accept can never drift.
17+
export { collectCelRootIdentifiers } from './cel-engine';
1418
export { cronEngine } from './cron-engine';
1519
export { templateEngine, TEMPLATE_FORMATTERS, formatValue } from './template-engine';
1620
export { registerStdLib, buildScope } from './stdlib';

packages/lint/src/index.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,9 @@ export {
127127
APPROVAL_APPROVER_TYPE_UNKNOWN,
128128
APPROVAL_ESCALATION_REASSIGN_NO_TARGET,
129129
APPROVAL_APPROVERS_MAY_RESOLVE_EMPTY,
130+
APPROVAL_EXPRESSION_INVALID,
131+
APPROVAL_EXPRESSION_NO_EMPTY_POLICY,
132+
APPROVAL_DECISION_OUTPUTS_RESERVED,
130133
} from './validate-approval-approvers.js';
131134
export type { ApprovalApproverFinding, ApprovalApproverSeverity } from './validate-approval-approvers.js';
132135

0 commit comments

Comments
 (0)