From 725f3342635721c62422eaae226a5eee783001a2 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 10 Jul 2026 03:49:29 +0000 Subject: [PATCH] feat(approvals): add 'position' approver type resolved via sys_user_position (ADR-0090 D3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01DgP1vEK6nrkkvnwQxCPGbe --- .changeset/position-approver-type.md | 26 ++++ content/docs/automation/approvals.mdx | 14 +- content/docs/automation/flows.mdx | 6 +- .../docs/references/automation/approval.mdx | 5 +- .../src/automation/flows/index.ts | 16 +- packages/cli/src/commands/lint.ts | 17 ++- packages/lint/src/index.ts | 7 + .../src/validate-approval-approvers.test.ts | 89 +++++++++++ .../lint/src/validate-approval-approvers.ts | 143 ++++++++++++++++++ .../src/approval-service.test.ts | 59 ++++++++ .../plugin-approvals/src/approval-service.ts | 50 +++++- packages/spec/src/automation/approval.test.ts | 4 +- packages/spec/src/automation/approval.zod.ts | 23 ++- skills/objectstack-automation/SKILL.md | 9 +- .../evals/approvals/test-revise-loop.md | 2 +- 15 files changed, 431 insertions(+), 39 deletions(-) create mode 100644 .changeset/position-approver-type.md create mode 100644 packages/lint/src/validate-approval-approvers.test.ts create mode 100644 packages/lint/src/validate-approval-approvers.ts diff --git a/.changeset/position-approver-type.md b/.changeset/position-approver-type.md new file mode 100644 index 0000000000..64069742cb --- /dev/null +++ b/.changeset/position-approver-type.md @@ -0,0 +1,26 @@ +--- +'@objectstack/spec': minor +'@objectstack/plugin-approvals': minor +'@objectstack/lint': minor +'@objectstack/cli': patch +--- + +Add a `position` approver type so approvals can route to org positions (ADR-0090 D3 fallout). + +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. Downstream +apps that authored `{ type: 'role', value: 'sales_manager' }` silently routed approvals to +nobody. Now: + +- **spec**: `ApproverType` gains `'position'` — `value` is the position machine name; the + approver expands to its holders via `sys_user_position`. Authoring guidance: keep + `type: 'role'` ONLY for membership tiers; for org positions use + `{ type: 'position', value: '' }` (one-line fix for the mismatch above). +- **plugin-approvals**: the engine resolves `position` approvers via `sys_user_position` ∪ + the `sys_member.role` transition source (same semantics as `PositionGraphService` in + plugin-sharing). The `department` approver type is now honored by its spec spelling + (previously only the off-spec `business_unit`/`bu` dialect matched). +- **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`. diff --git a/content/docs/automation/approvals.mdx b/content/docs/automation/approvals.mdx index 0c3435dc23..3eac087d89 100644 --- a/content/docs/automation/approvals.mdx +++ b/content/docs/automation/approvals.mdx @@ -28,7 +28,7 @@ A schedule-triggered escalation has no triggering user — so it must be `system ### 3. The approval node -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. +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. ```ts // Illustrative — see the Approval reference for the exact node schema. @@ -40,7 +40,7 @@ defineFlow({ { id: 'start', type: 'start', label: 'Start', config: { objectName: 'invoice', triggerType: 'record-after-create' } }, { id: 'approval', type: 'approval', label: 'Approval', - config: { approvers: [{ type: 'role', value: 'finance_manager' }] } }, + config: { approvers: [{ type: 'position', value: 'finance_manager' }] } }, { id: 'mark_approved', type: 'update_record', label: 'Mark Approved' }, { id: 'mark_rejected', type: 'update_record', label: 'Mark Rejected' }, ], @@ -52,7 +52,15 @@ defineFlow({ }); ``` -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). + +**`position` vs `role`.** `{ type: 'position', value: 'finance_manager' }` routes to the +holders of a position (`sys_user_position`, ADR-0090 D3). The `role` approver type is the +better-auth **org-membership tier** (`sys_member.role`: `owner`/`admin`/`member`) — a position +name authored as `type: 'role'` matches nobody and the request stalls; `os lint` flags this +(`approval-role-not-membership-tier`). + + +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). ## Approval nodes in practice diff --git a/content/docs/automation/flows.mdx b/content/docs/automation/flows.mdx index 6c55f5705c..42aa7280e5 100644 --- a/content/docs/automation/flows.mdx +++ b/content/docs/automation/flows.mdx @@ -47,7 +47,7 @@ const approvalFlow = { id: 'request_approval', type: 'approval', label: 'Manager Approval', - config: { approvers: [{ type: 'role', value: 'manager' }] }, + config: { approvers: [{ type: 'position', value: 'manager' }] }, }, { id: 'end', type: 'end', label: 'End' }, ], @@ -366,8 +366,8 @@ resumes down `approve`. Any one rejection finalizes immediately down `reject`. label: 'Finance + Legal Sign-off', config: { approvers: [ - { type: 'role', value: 'finance' }, - { type: 'role', value: 'legal' }, + { type: 'position', value: 'finance' }, + { type: 'position', value: 'legal' }, ], behavior: 'unanimous', // 'first_response' = any one decides lockRecord: false, diff --git a/content/docs/references/automation/approval.mdx b/content/docs/references/automation/approval.mdx index 1ebd2ec5ac..2c94a136d8 100644 --- a/content/docs/references/automation/approval.mdx +++ b/content/docs/references/automation/approval.mdx @@ -54,8 +54,8 @@ const result = ApprovalDecision.parse(data); | Property | Type | Required | Description | | :--- | :--- | :--- | :--- | -| **type** | `Enum<'user' \| 'role' \| 'team' \| 'department' \| 'manager' \| 'field' \| 'queue'>` | ✅ | | -| **value** | `string` | optional | User id / role / team / department / field / queue — per `type` | +| **type** | `Enum<'user' \| 'role' \| 'position' \| 'team' \| 'department' \| 'manager' \| 'field' \| 'queue'>` | ✅ | | +| **value** | `string` | optional | User id / membership tier / position / team / department / field / queue — per `type` | --- @@ -82,6 +82,7 @@ const result = ApprovalDecision.parse(data); * `user` * `role` +* `position` * `team` * `department` * `manager` diff --git a/examples/app-showcase/src/automation/flows/index.ts b/examples/app-showcase/src/automation/flows/index.ts index 06c6430a32..e45e274433 100644 --- a/examples/app-showcase/src/automation/flows/index.ts +++ b/examples/app-showcase/src/automation/flows/index.ts @@ -180,7 +180,7 @@ export const BudgetApprovalFlow = defineFlow({ type: 'approval', label: 'Manager Review', config: { - approvers: [{ type: 'role', value: 'manager' }], + approvers: [{ type: 'position', value: 'manager' }], behavior: 'first_response', lockRecord: true, // ADR-0044: at most two send-backs; the third auto-rejects. @@ -206,7 +206,7 @@ export const BudgetApprovalFlow = defineFlow({ type: 'approval', label: 'Executive Review', config: { - approvers: [{ type: 'role', value: 'exec' }], + approvers: [{ type: 'position', value: 'exec' }], behavior: 'unanimous', lockRecord: true, }, @@ -562,7 +562,7 @@ export const ClosureSignoffSubflow = defineFlow({ type: 'approval', label: 'Manager Sign-off', config: { - approvers: [{ type: 'role', value: 'manager' }], + approvers: [{ type: 'position', value: 'manager' }], behavior: 'first_response', // The parent project just hit a terminal status — no point locking it. lockRecord: false, @@ -875,8 +875,8 @@ export const ResilientSyncFlow = defineFlow({ * * Decide via the approvals API (never a raw engine `resume`): * POST /api/v1/automation/showcase_invoice_signoff/runs/{runId}/... ← no - * POST /api/v1/approvals/requests/{id}/approve { actorId: 'role:finance' } - * POST /api/v1/approvals/requests/{id}/approve { actorId: 'role:legal' } ← now it continues + * POST /api/v1/approvals/requests/{id}/approve { actorId: 'position:finance' } + * POST /api/v1/approvals/requests/{id}/approve { actorId: 'position:legal' } ← now it continues */ export const InvoiceDualSignoffFlow = defineFlow({ name: 'showcase_invoice_signoff', @@ -905,8 +905,8 @@ export const InvoiceDualSignoffFlow = defineFlow({ config: { // Two approver groups, notified in parallel; `unanimous` waits for both. approvers: [ - { type: 'role', value: 'finance' }, - { type: 'role', value: 'legal' }, + { type: 'position', value: 'finance' }, + { type: 'position', value: 'legal' }, ], behavior: 'unanimous', // The invoice keeps flowing through other automations while it waits. @@ -1063,7 +1063,7 @@ export const OneTaskSignoffSubflow = defineFlow({ type: 'approval', label: 'Task Sign-off', config: { - approvers: [{ type: 'role', value: 'manager' }], + approvers: [{ type: 'position', value: 'manager' }], behavior: 'first_response', lockRecord: false, }, diff --git a/packages/cli/src/commands/lint.ts b/packages/cli/src/commands/lint.ts index e7bc0ff60e..5e61478f31 100644 --- a/packages/cli/src/commands/lint.ts +++ b/packages/cli/src/commands/lint.ts @@ -8,7 +8,7 @@ import { loadConfig, BUNDLE_REQUIRE_EXTERNALS } from '../utils/config.js'; import { computeI18nCoverage } from '../utils/i18n-coverage.js'; import { lintDataModel } from '../lint/data-model-rules.js'; import { validateWidgetBindings } from '@objectstack/lint'; -import { validateRecordTitle, validateSemanticRoles, validateCapabilityReferences, validateSecurityPosture } from '@objectstack/lint'; +import { validateRecordTitle, validateSemanticRoles, validateCapabilityReferences, validateSecurityPosture, validateApprovalApprovers } from '@objectstack/lint'; import { collectAndLintDocs } from '../utils/collect-docs.js'; import { scoreMetadata } from '../lint/score.js'; import { runMetadataEval } from '../lint/metadata-eval.js'; @@ -369,6 +369,21 @@ export function lintConfig(config: any): LintIssue[] { }); } + // ── Approval-node approvers (ADR-0090 D3 fallout) ── + // `{ type: 'role' }` resolves against the better-auth org-membership tier + // (owner/admin/member), NOT positions — a position name authored there + // silently routes the approval to nobody. Advisory: the fix-it points at + // `{ type: 'position' }` (sys_user_position). + for (const t of validateApprovalApprovers(config)) { + issues.push({ + severity: t.severity === 'info' ? 'suggestion' : t.severity, + rule: t.rule, + message: `${t.where}: ${t.message}`, + path: t.path, + fix: t.hint, + }); + } + return issues; } diff --git a/packages/lint/src/index.ts b/packages/lint/src/index.ts index b8df29c839..40763890c0 100644 --- a/packages/lint/src/index.ts +++ b/packages/lint/src/index.ts @@ -75,6 +75,13 @@ export { } from './validate-capability-references.js'; export type { CapabilityRefFinding, CapabilityRefSeverity } from './validate-capability-references.js'; +export { + validateApprovalApprovers, + APPROVAL_ROLE_NOT_MEMBERSHIP_TIER, + APPROVAL_APPROVER_TYPE_UNKNOWN, +} from './validate-approval-approvers.js'; +export type { ApprovalApproverFinding, ApprovalApproverSeverity } from './validate-approval-approvers.js'; + export { validateSecurityPosture, SECURITY_OWD_UNSET, diff --git a/packages/lint/src/validate-approval-approvers.test.ts b/packages/lint/src/validate-approval-approvers.test.ts new file mode 100644 index 0000000000..ba45a588d8 --- /dev/null +++ b/packages/lint/src/validate-approval-approvers.test.ts @@ -0,0 +1,89 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect } from 'vitest'; +import { + validateApprovalApprovers, + APPROVAL_ROLE_NOT_MEMBERSHIP_TIER, + APPROVAL_APPROVER_TYPE_UNKNOWN, +} from './validate-approval-approvers.js'; + +function stackWithApprovers(approvers: unknown[]): Record { + return { + flows: [{ + name: 'expense_approval', + nodes: [ + { id: 'start', type: 'start', config: {} }, + { id: 'step1', type: 'approval', config: { approvers } }, + ], + edges: [], + }], + }; +} + +describe('validateApprovalApprovers', () => { + it('is clean on an empty / flow-less stack', () => { + expect(validateApprovalApprovers({})).toEqual([]); + expect(validateApprovalApprovers({ flows: [] })).toEqual([]); + }); + + it('accepts membership tiers for type role (owner/admin/member/guest)', () => { + const findings = validateApprovalApprovers(stackWithApprovers([ + { type: 'role', value: 'admin' }, + { type: 'role', value: 'Owner' }, // case-insensitive + { type: 'role', value: 'member' }, + { type: 'role', value: 'guest' }, + ])); + expect(findings).toEqual([]); + }); + + it("flags a position name authored as type 'role' (the ADR-0090 D3 hotcrm class)", () => { + const findings = validateApprovalApprovers(stackWithApprovers([ + { type: 'role', value: 'sales_manager' }, + ])); + expect(findings).toHaveLength(1); + expect(findings[0].rule).toBe(APPROVAL_ROLE_NOT_MEMBERSHIP_TIER); + expect(findings[0].severity).toBe('warning'); + expect(findings[0].where).toContain('expense_approval'); + expect(findings[0].path).toBe('flows[0].nodes[1].config.approvers[0].value'); + expect(findings[0].hint).toContain("type: 'position'"); + }); + + it('accepts the position approver type and the other spec types silently', () => { + const findings = validateApprovalApprovers(stackWithApprovers([ + { type: 'position', value: 'sales_manager' }, + { type: 'user', value: 'u1' }, + { type: 'manager' }, + { type: 'department', value: 'bu_sales' }, + { type: 'field', value: 'owner_id' }, + { type: 'queue', value: 'q1' }, + { type: 'team', value: 't1' }, + ])); + expect(findings).toEqual([]); + }); + + it('flags off-spec approver types, with a canonical fix for the business_unit dialect', () => { + const findings = validateApprovalApprovers(stackWithApprovers([ + { type: 'business_unit', value: 'bu_sales' }, + { type: 'group', value: 'g1' }, + ])); + expect(findings).toHaveLength(2); + expect(findings[0].rule).toBe(APPROVAL_APPROVER_TYPE_UNKNOWN); + expect(findings[0].hint).toContain("type: 'department'"); + expect(findings[1].rule).toBe(APPROVAL_APPROVER_TYPE_UNKNOWN); + }); + + it('only scans approval nodes and tolerates malformed shapes', () => { + const findings = validateApprovalApprovers({ + flows: [{ + name: 'f', + nodes: [ + { id: 'a', type: 'script', config: { approvers: [{ type: 'role', value: 'sales_manager' }] } }, + { id: 'b', type: 'approval' }, // no config + { id: 'c', type: 'approval', config: { approvers: 'oops' } }, + null, + ], + }, null, 'garbage'], + } as never); + expect(findings).toEqual([]); + }); +}); diff --git a/packages/lint/src/validate-approval-approvers.ts b/packages/lint/src/validate-approval-approvers.ts new file mode 100644 index 0000000000..76caf7deb6 --- /dev/null +++ b/packages/lint/src/validate-approval-approvers.ts @@ -0,0 +1,143 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Approval-node approver authoring lint (ADR-0090 D3 fallout). + * + * The `role` approver type resolves against better-auth's org-membership tier + * (`sys_member.role`: owner / admin / member) — it is NOT a position. After + * ADR-0090 D3 renamed `sys_role` → `sys_position`, downstream apps that + * authored `{ type: 'role', value: 'sales_manager' }` silently route the + * approval to nobody: the expansion finds no member row, falls back to the + * `role:sales_manager` literal, and the request waits on an approver that can + * never act. This rule moves that failure from a stuck request at runtime to + * a located fix-it at author time. + * + * Rules: + * + * | Rule | Severity | Origin | + * |-------------------------------------|----------|----------------------------| + * | approval-role-not-membership-tier | warning | ADR-0090 D3 (hotcrm class) | + * | approval-approver-type-unknown | warning | contract-first (PD #12) | + * + * Warnings (not errors): a custom better-auth membership tier is legal, and + * the runtime keeps its literal fallback — but both shapes are near-certainly + * authoring mistakes, so say it out loud. + * + * Pure `(stack) => Finding[]`; accepts the NORMALIZED stack input. + */ + +import { ApproverType, APPROVAL_NODE_TYPE } from '@objectstack/spec/automation'; + +export const APPROVAL_ROLE_NOT_MEMBERSHIP_TIER = 'approval-role-not-membership-tier'; +export const APPROVAL_APPROVER_TYPE_UNKNOWN = 'approval-approver-type-unknown'; + +export type ApprovalApproverSeverity = 'error' | 'warning' | 'info'; + +export interface ApprovalApproverFinding { + severity: ApprovalApproverSeverity; + /** Diagnostic rule id (`approval-*`). */ + rule: string; + /** Human-readable location, e.g. `flow "expense_approval" · node "step1"`. */ + where: string; + /** Config path, e.g. `flows[0].nodes[2].config.approvers[0]`. */ + path: string; + /** What is wrong. */ + message: string; + /** How to fix it. */ + hint: string; +} + +type AnyRec = Record; + +/** + * The better-auth org-membership tiers `sys_member.role` actually stores + * (see `identity/organization.zod.ts` + `mapMembershipRole`). Anything else + * authored as `{ type: 'role' }` is almost certainly a position name. + */ +const MEMBERSHIP_TIERS = new Set(['owner', 'admin', 'member', 'guest']); + +/** Off-spec dialect spellings we can name a canonical fix for. */ +const TYPE_FIX: Record = { + business_unit: 'department', + bu: 'department', +}; + +/** Coerce a collection (array or name-keyed map) to an array of records. */ +function asArray(v: unknown): AnyRec[] { + if (Array.isArray(v)) return v as AnyRec[]; + if (v && typeof v === 'object') { + return Object.entries(v as AnyRec).map(([name, def]) => ({ name, ...(def as AnyRec) })); + } + return []; +} + +/** + * Validate the approvers of every Approval node in the stack's flows. + * Returns findings (empty = clean). + */ +export function validateApprovalApprovers(stack: AnyRec): ApprovalApproverFinding[] { + const findings: ApprovalApproverFinding[] = []; + if (!stack || typeof stack !== 'object') return findings; + + const flows = asArray(stack.flows); + const validTypes = new Set(ApproverType.options); + + for (let fi = 0; fi < flows.length; fi++) { + const flow = flows[fi]; + if (!flow || typeof flow !== 'object') continue; + const flowName = typeof flow.name === 'string' ? flow.name : `(flow ${fi})`; + const nodes = Array.isArray(flow.nodes) ? (flow.nodes as AnyRec[]) : []; + + for (let ni = 0; ni < nodes.length; ni++) { + const node = nodes[ni]; + if (!node || node.type !== APPROVAL_NODE_TYPE) continue; + const nodeId = typeof node.id === 'string' ? node.id : `(node ${ni})`; + const cfg = (node.config ?? {}) as AnyRec; + const approvers = Array.isArray(cfg.approvers) ? (cfg.approvers as AnyRec[]) : []; + const where = `flow "${flowName}" · node "${nodeId}"`; + + for (let ai = 0; ai < approvers.length; ai++) { + const a = approvers[ai]; + if (!a || typeof a !== 'object') continue; + const type = typeof a.type === 'string' ? a.type : ''; + const value = typeof a.value === 'string' ? a.value : ''; + const path = `flows[${fi}].nodes[${ni}].config.approvers[${ai}]`; + + if (type && !validTypes.has(type)) { + const fix = TYPE_FIX[type]; + findings.push({ + severity: 'warning', + rule: APPROVAL_APPROVER_TYPE_UNKNOWN, + where, + path: `${path}.type`, + message: + `approver type '${type}' is not an ApproverType (${ApproverType.options.join(' | ')}).`, + hint: fix + ? `Use the spec value: { type: '${fix}', value: '${value}' }.` + : `Pick one of the spec values; unmapped types degrade to an inert '${type}:${value}' literal at runtime.`, + }); + continue; + } + + if (type === 'role' && value && !MEMBERSHIP_TIERS.has(value.toLowerCase())) { + findings.push({ + severity: 'warning', + rule: APPROVAL_ROLE_NOT_MEMBERSHIP_TIER, + where, + path: `${path}.value`, + message: + `approver { type: 'role', value: '${value}' } resolves against the better-auth ` + + `org-membership tier (sys_member.role: owner/admin/member) — '${value}' is not a ` + + `membership tier, so this approver matches nobody and the request stalls.`, + hint: + `If '${value}' is an org position, author { type: 'position', value: '${value}' } ` + + `(resolved via sys_user_position, ADR-0090 D3). Keep type 'role' only for ` + + `membership tiers (owner/admin/member).`, + }); + } + } + } + } + + return findings; +} diff --git a/packages/plugins/plugin-approvals/src/approval-service.test.ts b/packages/plugins/plugin-approvals/src/approval-service.test.ts index 3fe3a26dea..d65a93c18c 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.test.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.test.ts @@ -187,6 +187,65 @@ describe('ApprovalService (node era)', () => { expect(engine._tables['opportunity'][0].approval_status).toBe('pending'); }); + // ── approver expansion: position (ADR-0090 D3) ────────────────── + + const positionInput = (extra: Record = {}) => ({ + ...openInput([]), + config: { + approvers: [{ type: 'position' as const, value: 'sales_manager' }], + behavior: 'first_response' as const, + lockRecord: true, + }, + ...extra, + }); + + it('position approver: expands via sys_user_position, org-scoped', async () => { + engine._tables['sys_user_position'] = [ + { id: 'up1', user_id: 'u5', position: 'sales_manager', organization_id: 't1' }, + { id: 'up2', user_id: 'u6', position: 'sales_manager', organization_id: 't1' }, + { id: 'up3', user_id: 'u7', position: 'sales_manager', organization_id: 't2' }, // other tenant + { id: 'up4', user_id: 'u8', position: 'cfo', organization_id: 't1' }, // other position + ]; + const req = await svc.openNodeRequest(positionInput(), CTX); + expect(req.pending_approvers.sort()).toEqual(['u5', 'u6']); + }); + + it('position approver: unions the sys_member.role transition source (ADR-0057 D4)', async () => { + engine._tables['sys_user_position'] = [ + { id: 'up1', user_id: 'u5', position: 'sales_manager', organization_id: 't1' }, + ]; + engine._tables['sys_member'] = [ + { id: 'm1', user_id: 'u6', role: 'sales_manager', organization_id: 't1' }, + { id: 'm2', user_id: 'u5', role: 'sales_manager', organization_id: 't1' }, // deduped + ]; + const req = await svc.openNodeRequest(positionInput(), CTX); + expect(req.pending_approvers.sort()).toEqual(['u5', 'u6']); + }); + + it('position approver: falls back to a position: literal when nobody holds it', async () => { + const req = await svc.openNodeRequest(positionInput(), CTX); + expect(req.pending_approvers).toEqual(['position:sales_manager']); + }); + + it("department approver: honors the spec enum value 'department' (not just the business_unit dialect)", async () => { + engine._tables['sys_business_unit'] = [ + { id: 'bu1', organization_id: 't1', active: true }, + { id: 'bu2', parent_business_unit_id: 'bu1', organization_id: 't1', active: true }, + ]; + engine._tables['sys_business_unit_member'] = [ + { id: 'bm1', business_unit_id: 'bu1', user_id: 'u5' }, + { id: 'bm2', business_unit_id: 'bu2', user_id: 'u6' }, + ]; + const req = await svc.openNodeRequest(positionInput({ + config: { + approvers: [{ type: 'department' as const, value: 'bu1' }], + behavior: 'first_response' as const, + lockRecord: true, + }, + }), CTX); + expect(req.pending_approvers.sort()).toEqual(['u5', 'u6']); + }); + // ── decideNode ────────────────────────────────────────────────── it('decideNode: first_response approve finalizes immediately', async () => { diff --git a/packages/plugins/plugin-approvals/src/approval-service.ts b/packages/plugins/plugin-approvals/src/approval-service.ts index 6db5238c4c..39ae136323 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.ts @@ -31,8 +31,8 @@ import type { * `approve` / `reject` edge. * * This service owns the durable approval *state* — `sys_approval_request` / - * `sys_approval_action`, approver resolution (team / department / role / - * manager graph), and the optional status-field mirror — plus the decision + * `sys_approval_action`, approver resolution (team / department / position / + * role / manager graph), and the optional status-field mirror — plus the decision * API. It does not author processes, submit, or walk multi-step machinery * anymore; that orchestration lives on the one automation engine. */ @@ -274,16 +274,20 @@ export class ApprovalService implements IApprovalService { /** * Expand the approvers on an Approval node into user IDs by querying the - * graph tables for `team:` / `department:` / `role:` / `manager:` approver - * types. Falls back to a prefixed literal (`type:value`) when graph lookups - * produce nothing — so existing fixtures and flows that rely on substring - * matching keep working. + * graph tables for `team:` / `department:` / `position:` / `role:` / + * `manager:` approver types. Falls back to a prefixed literal + * (`type:value`) when graph lookups produce nothing — so existing fixtures + * and flows that rely on substring matching keep working. * * **Graph semantics:** * - `team` → flat members of `sys_team` (better-auth; no BFS) * - `department` → recursive BFS of `sys_business_unit.parent_business_unit_id` * → members of every descendant via `sys_business_unit_member` - * - `role` → users with `sys_member.role = value` in tenant + * - `position` → holders via `sys_user_position` ∪ `sys_member.role` + * transition source (ADR-0090 D3 / ADR-0057 D4) + * - `role` → users with `sys_member.role = value` in tenant — the + * better-auth MEMBERSHIP TIER (owner/admin/member), not a + * position; author `position` for org positions * - `manager` → `sys_user.manager_id` of `record[value] ?? record.owner_id` * - `field` → literal user id stored in `record[value]` * - `user` → literal value @@ -299,9 +303,12 @@ export class ApprovalService implements IApprovalService { if (a.type === 'team') { const users = await this.expandTeamUsers(String(a.value)); if (users.length) { for (const u of users) out.push(u); continue; } - } else if (a.type === 'business_unit' || a.type === 'bu') { + } else if (a.type === 'department' || a.type === 'business_unit' || a.type === 'bu') { const users = await this.expandBusinessUnitUsers(String(a.value), organizationId); if (users.length) { for (const u of users) out.push(u); continue; } + } else if (a.type === 'position') { + const users = await this.expandPositionUsers(String(a.value), organizationId); + if (users.length) { for (const u of users) out.push(u); continue; } } else if (a.type === 'role') { const users = await this.expandRoleUsers(String(a.value), organizationId); if (users.length) { for (const u of users) out.push(u); continue; } @@ -377,6 +384,33 @@ export class ApprovalService implements IApprovalService { return Array.from(new Set((rows ?? []).map((r: any) => String(r.user_id ?? '')).filter(Boolean))); } + /** + * Position holders (ADR-0090 D3): `sys_user_position` is the platform-owned + * assignment table, keyed by the position's machine name (ADR-0057 D4), + * unioned with the better-auth membership string (`sys_member.role`) as a + * transition source — the same semantics as `PositionGraphService` in + * `plugin-sharing`, so an approval routes to exactly the users the sharing + * engine would expand for the same position. + */ + private async expandPositionUsers(positionName: string, organizationId?: string | null): Promise { + if (!positionName) return []; + const users = new Set(); + const filter: any = { position: positionName }; + if (organizationId) filter.organization_id = organizationId; + try { + const rows = await this.engine.find('sys_user_position', { + filter, fields: ['user_id'], limit: 10000, context: SYSTEM_CTX, + } as any); + for (const r of (rows ?? []) as any[]) { + const uid = String(r.user_id ?? ''); + if (uid) users.add(uid); + } + } catch { /* table may not exist on minimal stacks — union source below still applies */ } + for (const uid of await this.expandRoleUsers(positionName, organizationId)) users.add(uid); + return Array.from(users); + } + + /** better-auth org-membership tier (`sys_member.role`) — NOT positions. */ private async expandRoleUsers(roleName: string, organizationId?: string | null): Promise { if (!roleName) return []; const filter: any = { role: roleName }; diff --git a/packages/spec/src/automation/approval.test.ts b/packages/spec/src/automation/approval.test.ts index 24113dfb5b..f3733c02c8 100644 --- a/packages/spec/src/automation/approval.test.ts +++ b/packages/spec/src/automation/approval.test.ts @@ -11,7 +11,7 @@ import { describe('ApproverType', () => { it('should accept all valid approver types', () => { - ['user', 'role', 'team', 'department', 'manager', 'field', 'queue'].forEach(t => { + ['user', 'role', 'position', 'team', 'department', 'manager', 'field', 'queue'].forEach(t => { expect(() => ApproverType.parse(t)).not.toThrow(); }); }); @@ -39,6 +39,8 @@ describe('Approval node constants (ADR-0019)', () => { describe('ApprovalNodeApproverSchema', () => { it('accepts a typed approver with an optional value', () => { expect(() => ApprovalNodeApproverSchema.parse({ type: 'user', value: 'u1' })).not.toThrow(); + // ADR-0090 D3: positions route by machine name (sys_user_position) + expect(() => ApprovalNodeApproverSchema.parse({ type: 'position', value: 'sales_manager' })).not.toThrow(); // manager resolves from the submitter, so value is optional expect(() => ApprovalNodeApproverSchema.parse({ type: 'manager' })).not.toThrow(); }); diff --git a/packages/spec/src/automation/approval.zod.ts b/packages/spec/src/automation/approval.zod.ts index c2a48ce408..44e30f25cf 100644 --- a/packages/spec/src/automation/approval.zod.ts +++ b/packages/spec/src/automation/approval.zod.ts @@ -8,7 +8,11 @@ import { lazySchema } from '../shared/lazy-schema'; */ export const ApproverType = z.enum([ 'user', // Specific user(s) - 'role', // Users with specific role (sys_member.role) + // `role` is the better-auth ORG-MEMBERSHIP TIER (sys_member.role: owner / + // admin / member) — NOT an org position. Post ADR-0090 D3 a value like + // 'sales_manager' here silently matches nobody; author `position` instead. + 'role', + 'position', // Holders of a position (sys_user_position, ADR-0090 D3) 'team', // Members of a flat collaboration team (sys_team) 'department', // Members of a department + all descendant departments (sys_business_unit) 'manager', // Submitter's manager (sys_user.manager_id) @@ -74,25 +78,28 @@ export const APPROVAL_BRANCH_LABELS = { export const ApprovalNodeApproverSchema = lazySchema(() => z.object({ type: ApproverType, /** - * The approver reference, interpreted per `type`: a user id (`user`), role - * name (`role`), team/department id (`team`/`department`), field name + * The approver reference, interpreted per `type`: a user id (`user`), a + * membership tier — owner/admin/member (`role`), a position machine name + * (`position`), team/department id (`team`/`department`), field name * holding a user id (`field`), or queue id (`queue`). Omitted for `manager` * (resolved from the submitter's `manager_id`). */ // `xRef` marks this string as a *polymorphic* typed reference (ADR-0018 // §configSchema): the concrete picker follows the sibling `type` column, so - // the Studio designer shows a user/role/team/department/queue picker — or an - // object-field picker (resolved from the flow's `$trigger` object) when - // `type` is `field`. `manager` and any unmapped value carry no `value` and - // stay free text. A single `.meta()` carries both description and annotation. + // the Studio designer shows a user/role/position/team/department/queue + // picker — or an object-field picker (resolved from the flow's `$trigger` + // object) when `type` is `field`. `manager` and any unmapped value carry no + // `value` and stay free text. A single `.meta()` carries both description + // and annotation. value: z.string().optional().meta({ - description: 'User id / role / team / department / field / queue — per `type`', + description: 'User id / membership tier / position / team / department / field / queue — per `type`', xRef: { kindFrom: 'type', objectSource: '$trigger', map: { user: 'user', role: 'role', + position: 'position', team: 'team', department: 'department', field: 'object-field', diff --git a/skills/objectstack-automation/SKILL.md b/skills/objectstack-automation/SKILL.md index d6111ace4f..82a52d330c 100644 --- a/skills/objectstack-automation/SKILL.md +++ b/skills/objectstack-automation/SKILL.md @@ -260,7 +260,7 @@ is one diagram a reviewer (or AI) can read end-to-end. type: 'approval', label: 'Sales Manager Review', config: { - approvers: [{ type: 'role', value: 'sales_manager' }], + approvers: [{ type: 'position', value: 'sales_manager' }], behavior: 'first_response', // or 'unanimous' lockRecord: true, // lock the record while pending approvalStatusField: 'approval_status', // mirror pending|approved|rejected|recalled onto the row @@ -272,7 +272,7 @@ is one diagram a reviewer (or AI) can read end-to-end. type: 'approval', label: 'Sales Director Sign-off', config: { - approvers: [{ type: 'role', value: 'sales_director' }], + approvers: [{ type: 'position', value: 'sales_director' }], behavior: 'unanimous', approvalStatusField: 'approval_status', }, @@ -330,7 +330,7 @@ Three pieces author it: ```typescript { id: 'manager_review', type: 'approval', - config: { approvers: [{ type: 'role', value: 'manager' }], lockRecord: true, maxRevisions: 2 }, + config: { approvers: [{ type: 'position', value: 'manager' }], lockRecord: true, maxRevisions: 2 }, }, { id: 'wait_revision', type: 'wait', label: 'Awaiting Revision', config: { eventType: 'signal', signalName: 'budget_revision' } }, @@ -376,7 +376,8 @@ branch — you never resume the flow by hand. | `type` | Resolves to | |:-------|:------------| | `user` | A specific user id (`value` = user id) | -| `role` | All users with the named role (`sys_member.role`) | +| `position` | Holders of a position — `value` = the position machine name, resolved via `sys_user_position` (ADR-0090 D3) | +| `role` | The better-auth **org-membership tier** (`sys_member.role`: `owner`/`admin`/`member`) — **NOT** a position. `{ type: 'role', value: 'sales_manager' }` matches nobody; use `position` | | `team` | Members of a flat `sys_team` | | `department` | A department + all descendant departments | | `manager` | The submitter's manager (`sys_user.manager_id`) | diff --git a/skills/objectstack-automation/evals/approvals/test-revise-loop.md b/skills/objectstack-automation/evals/approvals/test-revise-loop.md index 402e3057af..aab57df76d 100644 --- a/skills/objectstack-automation/evals/approvals/test-revise-loop.md +++ b/skills/objectstack-automation/evals/approvals/test-revise-loop.md @@ -29,7 +29,7 @@ revision window, and a **declared back-edge** closing the loop: config: { objectName: 'project', triggerType: 'record-after-update', condition: 'budget > 100000 && budget != previous.budget' } }, { id: 'manager_review', type: 'approval', - config: { approvers: [{ type: 'role', value: 'manager' }], lockRecord: true, + config: { approvers: [{ type: 'position', value: 'manager' }], lockRecord: true, maxRevisions: 2 } }, // send-back budget { id: 'wait_revision', type: 'wait', label: 'Awaiting Revision', config: { eventType: 'signal', signalName: 'budget_revision' } },