Skip to content

Commit 216fa9a

Browse files
os-zhuangclaude
andauthored
feat(approvals): add 'position' approver type resolved via sys_user_position (ADR-0090 D3) (#2738)
Post ADR-0090 D3 the 'role' approver type resolves against the better-auth org-membership tier (sys_member.role: owner/admin/member) — it was never a position, so downstream apps authoring { type: 'role', value: 'sales_manager' } silently routed approvals to nobody. - spec: ApproverType gains 'position' (value = position machine name); the xRef picker map and value description follow. 'role' is documented as the membership tier it actually is. - plugin-approvals: expandApprovers resolves 'position' via sys_user_position ∪ the sys_member.role transition source (ADR-0057 D4), mirroring PositionGraphService in plugin-sharing; the spec value 'department' is now honored alongside the pre-existing business_unit/bu dialect. - lint: new validateApprovalApprovers rule — approval-role-not-membership-tier warns when a role approver's value is not a membership tier and prescribes the position rewrite; approval-approver-type-unknown flags off-spec approver types with a business_unit→department fix-it. Wired into os lint. - docs/skill/showcase: approver examples switch to { type: 'position' }; the authoring skill's Approver Types table and the approvals guide call the trap out explicitly. Claude-Session: https://claude.ai/code/session_01DgP1vEK6nrkkvnwQxCPGbe Co-authored-by: Claude <noreply@anthropic.com>
1 parent 80f12ca commit 216fa9a

15 files changed

Lines changed: 431 additions & 39 deletions

File tree

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
---
2+
'@objectstack/spec': minor
3+
'@objectstack/plugin-approvals': minor
4+
'@objectstack/lint': minor
5+
'@objectstack/cli': patch
6+
---
7+
8+
Add a `position` approver type so approvals can route to org positions (ADR-0090 D3 fallout).
9+
10+
Post ADR-0090 D3 the `role` approver type resolves against the better-auth org-membership
11+
tier (`sys_member.role`: `owner`/`admin`/`member`) — it was never a position. Downstream
12+
apps that authored `{ type: 'role', value: 'sales_manager' }` silently routed approvals to
13+
nobody. Now:
14+
15+
- **spec**: `ApproverType` gains `'position'``value` is the position machine name; the
16+
approver expands to its holders via `sys_user_position`. Authoring guidance: keep
17+
`type: 'role'` ONLY for membership tiers; for org positions use
18+
`{ type: 'position', value: '<position_name>' }` (one-line fix for the mismatch above).
19+
- **plugin-approvals**: the engine resolves `position` approvers via `sys_user_position`
20+
the `sys_member.role` transition source (same semantics as `PositionGraphService` in
21+
plugin-sharing). The `department` approver type is now honored by its spec spelling
22+
(previously only the off-spec `business_unit`/`bu` dialect matched).
23+
- **lint**: new `validateApprovalApprovers` rule — `approval-role-not-membership-tier`
24+
warns when a `role` approver's value is not a membership tier and prescribes the
25+
`position` rewrite; `approval-approver-type-unknown` flags off-spec approver types
26+
(with a `business_unit``department` fix-it). Wired into `os lint`.

content/docs/automation/approvals.mdx

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ A schedule-triggered escalation has no triggering user — so it must be `system
2828

2929
### 3. The approval node
3030

31-
The node declares **who approves** (a named user, a role, the submitter's manager, …) and what happens on approve / reject / escalate. The authoritative shape is [`ApprovalNodeConfig` / `ApproverType`](/docs/references/automation/approval); a flow strings the trigger, the approval node, and the post-decision branches together.
31+
The node declares **who approves** (a named user, a position, the submitter's manager, …) and what happens on approve / reject / escalate. The authoritative shape is [`ApprovalNodeConfig` / `ApproverType`](/docs/references/automation/approval); a flow strings the trigger, the approval node, and the post-decision branches together.
3232

3333
```ts
3434
// Illustrative — see the Approval reference for the exact node schema.
@@ -40,7 +40,7 @@ defineFlow({
4040
{ id: 'start', type: 'start', label: 'Start',
4141
config: { objectName: 'invoice', triggerType: 'record-after-create' } },
4242
{ id: 'approval', type: 'approval', label: 'Approval',
43-
config: { approvers: [{ type: 'role', value: 'finance_manager' }] } },
43+
config: { approvers: [{ type: 'position', value: 'finance_manager' }] } },
4444
{ id: 'mark_approved', type: 'update_record', label: 'Mark Approved' },
4545
{ id: 'mark_rejected', type: 'update_record', label: 'Mark Rejected' },
4646
],
@@ -52,7 +52,15 @@ defineFlow({
5252
});
5353
```
5454

55-
Approving is itself a gated action — model "may approve" as a capability (`approve_invoice`) the approver role holds, and gate the approve action's `requiredPermissions` on it so the gate is enforced on **both** the UI and the server (ADR-0066 D4).
55+
<Callout type="warn">
56+
**`position` vs `role`.** `{ type: 'position', value: 'finance_manager' }` routes to the
57+
holders of a position (`sys_user_position`, ADR-0090 D3). The `role` approver type is the
58+
better-auth **org-membership tier** (`sys_member.role`: `owner`/`admin`/`member`) — a position
59+
name authored as `type: 'role'` matches nobody and the request stalls; `os lint` flags this
60+
(`approval-role-not-membership-tier`).
61+
</Callout>
62+
63+
Approving is itself a gated action — model "may approve" as a capability (`approve_invoice`) the approver's permission set grants, and gate the approve action's `requiredPermissions` on it so the gate is enforced on **both** the UI and the server (ADR-0066 D4).
5664

5765
## Approval nodes in practice
5866

content/docs/automation/flows.mdx

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ const approvalFlow = {
4747
id: 'request_approval',
4848
type: 'approval',
4949
label: 'Manager Approval',
50-
config: { approvers: [{ type: 'role', value: 'manager' }] },
50+
config: { approvers: [{ type: 'position', value: 'manager' }] },
5151
},
5252
{ id: 'end', type: 'end', label: 'End' },
5353
],
@@ -366,8 +366,8 @@ resumes down `approve`. Any one rejection finalizes immediately down `reject`.
366366
label: 'Finance + Legal Sign-off',
367367
config: {
368368
approvers: [
369-
{ type: 'role', value: 'finance' },
370-
{ type: 'role', value: 'legal' },
369+
{ type: 'position', value: 'finance' },
370+
{ type: 'position', value: 'legal' },
371371
],
372372
behavior: 'unanimous', // 'first_response' = any one decides
373373
lockRecord: false,

content/docs/references/automation/approval.mdx

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

5555
| Property | Type | Required | Description |
5656
| :--- | :--- | :--- | :--- |
57-
| **type** | `Enum<'user' \| 'role' \| 'team' \| 'department' \| 'manager' \| 'field' \| 'queue'>` || |
58-
| **value** | `string` | optional | User id / role / team / department / field / queue — per `type` |
57+
| **type** | `Enum<'user' \| 'role' \| 'position' \| 'team' \| 'department' \| 'manager' \| 'field' \| 'queue'>` || |
58+
| **value** | `string` | optional | User id / membership tier / position / team / department / field / queue — per `type` |
5959

6060

6161
---
@@ -82,6 +82,7 @@ const result = ApprovalDecision.parse(data);
8282

8383
* `user`
8484
* `role`
85+
* `position`
8586
* `team`
8687
* `department`
8788
* `manager`

examples/app-showcase/src/automation/flows/index.ts

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -180,7 +180,7 @@ export const BudgetApprovalFlow = defineFlow({
180180
type: 'approval',
181181
label: 'Manager Review',
182182
config: {
183-
approvers: [{ type: 'role', value: 'manager' }],
183+
approvers: [{ type: 'position', value: 'manager' }],
184184
behavior: 'first_response',
185185
lockRecord: true,
186186
// ADR-0044: at most two send-backs; the third auto-rejects.
@@ -206,7 +206,7 @@ export const BudgetApprovalFlow = defineFlow({
206206
type: 'approval',
207207
label: 'Executive Review',
208208
config: {
209-
approvers: [{ type: 'role', value: 'exec' }],
209+
approvers: [{ type: 'position', value: 'exec' }],
210210
behavior: 'unanimous',
211211
lockRecord: true,
212212
},
@@ -562,7 +562,7 @@ export const ClosureSignoffSubflow = defineFlow({
562562
type: 'approval',
563563
label: 'Manager Sign-off',
564564
config: {
565-
approvers: [{ type: 'role', value: 'manager' }],
565+
approvers: [{ type: 'position', value: 'manager' }],
566566
behavior: 'first_response',
567567
// The parent project just hit a terminal status — no point locking it.
568568
lockRecord: false,
@@ -875,8 +875,8 @@ export const ResilientSyncFlow = defineFlow({
875875
*
876876
* Decide via the approvals API (never a raw engine `resume`):
877877
* POST /api/v1/automation/showcase_invoice_signoff/runs/{runId}/... ← no
878-
* POST /api/v1/approvals/requests/{id}/approve { actorId: 'role:finance' }
879-
* POST /api/v1/approvals/requests/{id}/approve { actorId: 'role:legal' } ← now it continues
878+
* POST /api/v1/approvals/requests/{id}/approve { actorId: 'position:finance' }
879+
* POST /api/v1/approvals/requests/{id}/approve { actorId: 'position:legal' } ← now it continues
880880
*/
881881
export const InvoiceDualSignoffFlow = defineFlow({
882882
name: 'showcase_invoice_signoff',
@@ -905,8 +905,8 @@ export const InvoiceDualSignoffFlow = defineFlow({
905905
config: {
906906
// Two approver groups, notified in parallel; `unanimous` waits for both.
907907
approvers: [
908-
{ type: 'role', value: 'finance' },
909-
{ type: 'role', value: 'legal' },
908+
{ type: 'position', value: 'finance' },
909+
{ type: 'position', value: 'legal' },
910910
],
911911
behavior: 'unanimous',
912912
// The invoice keeps flowing through other automations while it waits.
@@ -1063,7 +1063,7 @@ export const OneTaskSignoffSubflow = defineFlow({
10631063
type: 'approval',
10641064
label: 'Task Sign-off',
10651065
config: {
1066-
approvers: [{ type: 'role', value: 'manager' }],
1066+
approvers: [{ type: 'position', value: 'manager' }],
10671067
behavior: 'first_response',
10681068
lockRecord: false,
10691069
},

packages/cli/src/commands/lint.ts

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ import { loadConfig, BUNDLE_REQUIRE_EXTERNALS } from '../utils/config.js';
88
import { computeI18nCoverage } from '../utils/i18n-coverage.js';
99
import { lintDataModel } from '../lint/data-model-rules.js';
1010
import { validateWidgetBindings } from '@objectstack/lint';
11-
import { validateRecordTitle, validateSemanticRoles, validateCapabilityReferences, validateSecurityPosture } from '@objectstack/lint';
11+
import { validateRecordTitle, validateSemanticRoles, validateCapabilityReferences, validateSecurityPosture, validateApprovalApprovers } from '@objectstack/lint';
1212
import { collectAndLintDocs } from '../utils/collect-docs.js';
1313
import { scoreMetadata } from '../lint/score.js';
1414
import { runMetadataEval } from '../lint/metadata-eval.js';
@@ -369,6 +369,21 @@ export function lintConfig(config: any): LintIssue[] {
369369
});
370370
}
371371

372+
// ── Approval-node approvers (ADR-0090 D3 fallout) ──
373+
// `{ type: 'role' }` resolves against the better-auth org-membership tier
374+
// (owner/admin/member), NOT positions — a position name authored there
375+
// silently routes the approval to nobody. Advisory: the fix-it points at
376+
// `{ type: 'position' }` (sys_user_position).
377+
for (const t of validateApprovalApprovers(config)) {
378+
issues.push({
379+
severity: t.severity === 'info' ? 'suggestion' : t.severity,
380+
rule: t.rule,
381+
message: `${t.where}: ${t.message}`,
382+
path: t.path,
383+
fix: t.hint,
384+
});
385+
}
386+
372387
return issues;
373388
}
374389

packages/lint/src/index.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,13 @@ export {
7575
} from './validate-capability-references.js';
7676
export type { CapabilityRefFinding, CapabilityRefSeverity } from './validate-capability-references.js';
7777

78+
export {
79+
validateApprovalApprovers,
80+
APPROVAL_ROLE_NOT_MEMBERSHIP_TIER,
81+
APPROVAL_APPROVER_TYPE_UNKNOWN,
82+
} from './validate-approval-approvers.js';
83+
export type { ApprovalApproverFinding, ApprovalApproverSeverity } from './validate-approval-approvers.js';
84+
7885
export {
7986
validateSecurityPosture,
8087
SECURITY_OWD_UNSET,
Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,89 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
import { describe, it, expect } from 'vitest';
4+
import {
5+
validateApprovalApprovers,
6+
APPROVAL_ROLE_NOT_MEMBERSHIP_TIER,
7+
APPROVAL_APPROVER_TYPE_UNKNOWN,
8+
} from './validate-approval-approvers.js';
9+
10+
function stackWithApprovers(approvers: unknown[]): Record<string, unknown> {
11+
return {
12+
flows: [{
13+
name: 'expense_approval',
14+
nodes: [
15+
{ id: 'start', type: 'start', config: {} },
16+
{ id: 'step1', type: 'approval', config: { approvers } },
17+
],
18+
edges: [],
19+
}],
20+
};
21+
}
22+
23+
describe('validateApprovalApprovers', () => {
24+
it('is clean on an empty / flow-less stack', () => {
25+
expect(validateApprovalApprovers({})).toEqual([]);
26+
expect(validateApprovalApprovers({ flows: [] })).toEqual([]);
27+
});
28+
29+
it('accepts membership tiers for type role (owner/admin/member/guest)', () => {
30+
const findings = validateApprovalApprovers(stackWithApprovers([
31+
{ type: 'role', value: 'admin' },
32+
{ type: 'role', value: 'Owner' }, // case-insensitive
33+
{ type: 'role', value: 'member' },
34+
{ type: 'role', value: 'guest' },
35+
]));
36+
expect(findings).toEqual([]);
37+
});
38+
39+
it("flags a position name authored as type 'role' (the ADR-0090 D3 hotcrm class)", () => {
40+
const findings = validateApprovalApprovers(stackWithApprovers([
41+
{ type: 'role', value: 'sales_manager' },
42+
]));
43+
expect(findings).toHaveLength(1);
44+
expect(findings[0].rule).toBe(APPROVAL_ROLE_NOT_MEMBERSHIP_TIER);
45+
expect(findings[0].severity).toBe('warning');
46+
expect(findings[0].where).toContain('expense_approval');
47+
expect(findings[0].path).toBe('flows[0].nodes[1].config.approvers[0].value');
48+
expect(findings[0].hint).toContain("type: 'position'");
49+
});
50+
51+
it('accepts the position approver type and the other spec types silently', () => {
52+
const findings = validateApprovalApprovers(stackWithApprovers([
53+
{ type: 'position', value: 'sales_manager' },
54+
{ type: 'user', value: 'u1' },
55+
{ type: 'manager' },
56+
{ type: 'department', value: 'bu_sales' },
57+
{ type: 'field', value: 'owner_id' },
58+
{ type: 'queue', value: 'q1' },
59+
{ type: 'team', value: 't1' },
60+
]));
61+
expect(findings).toEqual([]);
62+
});
63+
64+
it('flags off-spec approver types, with a canonical fix for the business_unit dialect', () => {
65+
const findings = validateApprovalApprovers(stackWithApprovers([
66+
{ type: 'business_unit', value: 'bu_sales' },
67+
{ type: 'group', value: 'g1' },
68+
]));
69+
expect(findings).toHaveLength(2);
70+
expect(findings[0].rule).toBe(APPROVAL_APPROVER_TYPE_UNKNOWN);
71+
expect(findings[0].hint).toContain("type: 'department'");
72+
expect(findings[1].rule).toBe(APPROVAL_APPROVER_TYPE_UNKNOWN);
73+
});
74+
75+
it('only scans approval nodes and tolerates malformed shapes', () => {
76+
const findings = validateApprovalApprovers({
77+
flows: [{
78+
name: 'f',
79+
nodes: [
80+
{ id: 'a', type: 'script', config: { approvers: [{ type: 'role', value: 'sales_manager' }] } },
81+
{ id: 'b', type: 'approval' }, // no config
82+
{ id: 'c', type: 'approval', config: { approvers: 'oops' } },
83+
null,
84+
],
85+
}, null, 'garbage'],
86+
} as never);
87+
expect(findings).toEqual([]);
88+
});
89+
});

0 commit comments

Comments
 (0)