From cc465742f9b963a8444c9593ca4e35b7164955e7 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 19 Jul 2026 10:48:31 +0000 Subject: [PATCH 1/2] =?UTF-8?q?feat(approvals):=20quorum=20+=20per-group?= =?UTF-8?q?=20sign-off=20(=E4=BC=9A=E7=AD=BE)=20+=20decision=20attachments?= =?UTF-8?q?=20(#3266)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Node-internal multi-approver semantics — the钉钉/Salesforce-style patterns that deliver "legal AND finance both approve" and "M-of-N" without any flow-engine change (topology-level parallel branches remain #3267). Community-core; weighted voting + approval-matrix governance stay enterprise (objectstack-ai/cloud#861). Spec (approval.zod.ts): - behavior enum += 'quorum' (M-of-N) and 'per_group' (one — or minApprovals — from EACH group), plus a `minApprovals` threshold and an optional `group` label on each approver. A single rejection is always a veto. Corrected the stale "quorum is enterprise" comment. Engine (approval-service.ts): - expandApprovers refactored to resolveApproverSpec + a group collector, so each resolved approver (OOO-substituted included) is tagged with its group; the map is snapshotted at open so the tally is stable. - decideNode: unified quorum/per_group/unanimous finalization via isApprovalSatisfied; thresholds clamp to the resolvable count / group size so a mis-set value can never deadlock. reassign transfers a slot's group so the new approver still counts. Attachments (#3266): - sys_approval_action gains a `Field.file` `attachments` column; decide/comment accept + persist file references, threaded through the contract, REST decide/comment routes, and the client approve/reject helpers. Showcase: ExpenseSignoffFlow — manager + finance per_group 会签 on a submitted expense report. Tests: 9 new (quorum boundary + clamp + veto, per_group each-group / two-in-one-group / minApprovals=2 / OOO-substituted member counts, attachments); 132 green in plugin-approvals. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_016ypkQikZ55oWUHUnMebwXA --- .../src/automation/flows/index.ts | 52 ++++ packages/client/src/index.ts | 8 +- .../src/approval-service.test.ts | 103 ++++++++ .../plugin-approvals/src/approval-service.ts | 236 +++++++++++++----- .../src/sys-approval-action.object.ts | 8 + packages/rest/src/rest-server.ts | 2 + packages/spec/src/automation/approval.zod.ts | 30 ++- .../spec/src/contracts/approval-service.ts | 6 + 8 files changed, 373 insertions(+), 72 deletions(-) diff --git a/examples/app-showcase/src/automation/flows/index.ts b/examples/app-showcase/src/automation/flows/index.ts index d44e5167fc..e28eddf2bf 100644 --- a/examples/app-showcase/src/automation/flows/index.ts +++ b/examples/app-showcase/src/automation/flows/index.ts @@ -1379,8 +1379,60 @@ export const InquiryPurgeFlow = defineFlow({ ], }); +/** + * Expense Sign-off (#3266) — a `per_group` approval demonstrating 会签: a + * MANAGER **and** a FINANCE/audit approver must EACH sign off (one from each + * group) before a submitted expense report is approved; either rejection + * vetoes. Where {@link BudgetApprovalFlow} chains position steps in series, + * this gates a SINGLE node on two groups at once — the node-internal parallel + * pattern (钉钉/Salesforce style) that needs no parallel branches. Set + * `minApprovals` > 1 for "two from each group"; use `behavior: 'quorum'` + + * `minApprovals` for M-of-N collective sign-off. + */ +export const ExpenseSignoffFlow = defineFlow({ + name: 'showcase_expense_signoff', + label: 'Expense Report Sign-off', + description: 'Manager + finance per-group sign-off (会签) on a submitted expense report (#3266).', + type: 'autolaunched', + status: 'active', + nodes: [ + { + id: 'start', + type: 'start', + label: 'On Submitted', + config: { + objectName: 'showcase_expense_report', + triggerType: 'record-after-update', + condition: 'status == "submitted" && previous.status != "submitted"', + }, + }, + { + id: 'dual_signoff', + type: 'approval', + label: 'Manager + Finance Sign-off', + config: { + approvers: [ + { type: 'position', value: 'manager', group: 'manager' }, + { type: 'position', value: 'auditor', group: 'finance' }, + ], + behavior: 'per_group', + minApprovals: 1, + lockRecord: true, + }, + }, + { id: 'approved', type: 'end', label: 'Approved' }, + { id: 'rejected', type: 'end', label: 'Rejected' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'dual_signoff' }, + { id: 'e2', source: 'dual_signoff', target: 'approved', label: 'approve' }, + { id: 'e3', source: 'dual_signoff', target: 'rejected', label: 'reject' }, + ], +}); + export const allFlows = [ TaskCompletedFlow, + ExpenseSignoffFlow, ReassignWizardFlow, InquiryPurgeFlow, BudgetApprovalFlow, diff --git a/packages/client/src/index.ts b/packages/client/src/index.ts index ad4b1eb63d..04ef4c275f 100644 --- a/packages/client/src/index.ts +++ b/packages/client/src/index.ts @@ -2493,11 +2493,11 @@ export class ObjectStackClient { * Record an approve decision on a request. Finalises the request when the * node's behaviour is satisfied and resumes the owning flow run. */ - approve: async (requestId: string, decision?: { actorId?: string; comment?: string }): Promise => { + approve: async (requestId: string, decision?: { actorId?: string; comment?: string; attachments?: string[] }): Promise => { const route = this.getRoute('approvals'); const res = await this.fetch(`${this.baseUrl}${route}/requests/${encodeURIComponent(requestId)}/approve`, { method: 'POST', - body: JSON.stringify({ actorId: decision?.actorId, comment: decision?.comment }) + body: JSON.stringify({ actorId: decision?.actorId, comment: decision?.comment, attachments: decision?.attachments }) }); return this.unwrapResponse(res); }, @@ -2506,11 +2506,11 @@ export class ObjectStackClient { * Record a reject decision on a request. Resumes the owning flow run down * the `reject` edge. */ - reject: async (requestId: string, decision?: { actorId?: string; comment?: string }): Promise => { + reject: async (requestId: string, decision?: { actorId?: string; comment?: string; attachments?: string[] }): Promise => { const route = this.getRoute('approvals'); const res = await this.fetch(`${this.baseUrl}${route}/requests/${encodeURIComponent(requestId)}/reject`, { method: 'POST', - body: JSON.stringify({ actorId: decision?.actorId, comment: decision?.comment }) + body: JSON.stringify({ actorId: decision?.actorId, comment: decision?.comment, attachments: decision?.attachments }) }); return this.unwrapResponse(res); }, diff --git a/packages/plugins/plugin-approvals/src/approval-service.test.ts b/packages/plugins/plugin-approvals/src/approval-service.test.ts index e0171883f7..a7770c6982 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.test.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.test.ts @@ -1287,3 +1287,106 @@ describe('sys_approval_delegation write guard (#1322)', () => { )).rejects.toThrow(/FORBIDDEN/); }); }); + +// ── Quorum & per-group sign-off (#3266) ─────────────────────────────── +// +// quorum = M-of-N collective sign-off; per_group = one (or minApprovals) from +// EACH group (会签). A single rejection is always a veto. Group membership is +// snapshotted at open, so OOO-substituted approvers count for their group. +describe('ApprovalService — quorum & per_group (#3266)', () => { + let engine: ReturnType; + let svc: ApprovalService; + const base = new Date('2026-08-01T10:00:00Z').getTime(); + + beforeEach(() => { + engine = makeFakeEngine(); + let n = 0; + svc = new ApprovalService({ engine: engine as any, clock: { now: () => new Date(base + (n++) * 1000) } }); + }); + + // Build an openNodeRequest input with explicit approver specs + behavior. + const cfg = (approvers: any[], behavior: string, extra: Record = {}) => ({ + ...openInput([]), + config: { approvers, behavior, lockRecord: true, ...extra }, + }); + const U = (v: string, group?: string) => (group ? { type: 'user', value: v, group } : { type: 'user', value: v }); + + it('quorum: holds until minApprovals reached, then finalizes', async () => { + const req = await svc.openNodeRequest(cfg([U('u1'), U('u2'), U('u3')], 'quorum', { minApprovals: 2 }), CTX); + const a = await svc.decideNode(req.id, { decision: 'approve', actorId: 'u1' }, SYS); + expect(a.finalized).toBe(false); + expect(a.request.status).toBe('pending'); + const b = await svc.decideNode(req.id, { decision: 'approve', actorId: 'u2' }, SYS); + expect(b.finalized).toBe(true); + expect(b.request.status).toBe('approved'); + }); + + it('quorum: minApprovals clamps to the approver count (no deadlock)', async () => { + const req = await svc.openNodeRequest(cfg([U('u1'), U('u2')], 'quorum', { minApprovals: 5 }), CTX); + await svc.decideNode(req.id, { decision: 'approve', actorId: 'u1' }, SYS); + const b = await svc.decideNode(req.id, { decision: 'approve', actorId: 'u2' }, SYS); + expect(b.finalized).toBe(true); + }); + + it('quorum: any reject is a veto', async () => { + const req = await svc.openNodeRequest(cfg([U('u1'), U('u2'), U('u3')], 'quorum', { minApprovals: 2 }), CTX); + const r = await svc.decideNode(req.id, { decision: 'reject', actorId: 'u1' }, SYS); + expect(r.finalized).toBe(true); + expect(r.request.status).toBe('rejected'); + }); + + it('per_group: advances only when EACH group approves', async () => { + const req = await svc.openNodeRequest(cfg([U('l1', 'legal'), U('f1', 'finance')], 'per_group'), CTX); + const a = await svc.decideNode(req.id, { decision: 'approve', actorId: 'l1' }, SYS); + expect(a.finalized).toBe(false); // finance still pending + const b = await svc.decideNode(req.id, { decision: 'approve', actorId: 'f1' }, SYS); + expect(b.finalized).toBe(true); + expect(b.request.status).toBe('approved'); + }); + + it('per_group: two approvals in ONE group do not satisfy another group', async () => { + const req = await svc.openNodeRequest( + cfg([U('l1', 'legal'), U('l2', 'legal'), U('f1', 'finance')], 'per_group'), CTX); + await svc.decideNode(req.id, { decision: 'approve', actorId: 'l1' }, SYS); + const b = await svc.decideNode(req.id, { decision: 'approve', actorId: 'l2' }, SYS); + expect(b.finalized).toBe(false); // finance still missing + }); + + it('per_group: minApprovals=2 needs two from each group', async () => { + const req = await svc.openNodeRequest(cfg( + [U('l1', 'legal'), U('l2', 'legal'), U('f1', 'finance'), U('f2', 'finance')], + 'per_group', { minApprovals: 2 }), CTX); + for (const u of ['l1', 'f1', 'l2']) { + const r = await svc.decideNode(req.id, { decision: 'approve', actorId: u }, SYS); + expect(r.finalized).toBe(false); + } + const done = await svc.decideNode(req.id, { decision: 'approve', actorId: 'f2' }, SYS); + expect(done.finalized).toBe(true); + }); + + it('per_group: reject is a veto', async () => { + const req = await svc.openNodeRequest(cfg([U('l1', 'legal'), U('f1', 'finance')], 'per_group'), CTX); + const r = await svc.decideNode(req.id, { decision: 'reject', actorId: 'l1' }, SYS); + expect(r.request.status).toBe('rejected'); + }); + + it('per_group: an OOO-substituted member still counts for their group', async () => { + engine._tables['sys_approval_delegation'] = [ + { id: 'd', delegator_id: 'l1', delegate_id: 'lb', organization_id: 't1', valid_from: null, valid_until: null, reason: 'leave' }, + ]; + const req = await svc.openNodeRequest(cfg([U('l1', 'legal'), U('f1', 'finance')], 'per_group'), CTX); + expect(req.pending_approvers).toContain('lb'); + expect(req.pending_approvers).not.toContain('l1'); + const a = await svc.decideNode(req.id, { decision: 'approve', actorId: 'lb' }, SYS); // delegate covers legal + expect(a.finalized).toBe(false); + const b = await svc.decideNode(req.id, { decision: 'approve', actorId: 'f1' }, SYS); + expect(b.finalized).toBe(true); + }); + + it('records decision attachments on the audit row', async () => { + const req = await svc.openNodeRequest(openInput(['u9']), CTX); + await svc.decideNode(req.id, { decision: 'approve', actorId: 'u9', attachments: ['file_1', 'file_2'] }, SYS); + const act = engine._tables['sys_approval_action'].find((a: any) => a.action === 'approve'); + expect(act.attachments).toEqual(['file_1', 'file_2']); + }); +}); diff --git a/packages/plugins/plugin-approvals/src/approval-service.ts b/packages/plugins/plugin-approvals/src/approval-service.ts index d7cc02e1f4..4cb2800eaa 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.ts @@ -328,62 +328,85 @@ export class ApprovalService implements IApprovalService { step: any, record?: any, organizationId?: string | null, - opts?: { now?: number; substitutions?: OooSubstitution[] }, + opts?: { now?: number; substitutions?: OooSubstitution[]; groups?: Record }, ): Promise { if (!step || !Array.isArray(step.approvers)) return []; const now = opts?.now ?? this.clock.now().getTime(); const out: string[] = []; - for (const a of step.approvers) { + const specs: any[] = step.approvers; + for (let idx = 0; idx < specs.length; idx++) { + const a = specs[idx]; if (!a) continue; - // ADR-0090 D3: `role` is the deprecated spelling of - // `org_membership_level`. Resolve on the canonical type, but keep the - // AUTHORED spelling in the `type:value` fallback below — stored - // `sys_approval_approver` rows and `pending_approvers` slots from 15.x - // carry the old literal, and rewriting it here would orphan them. - const type = canonicalApproverType(String(a.type)); - if (type !== a.type) { - this.logger?.warn?.( - `[approvals] approver type '${a.type}' is deprecated (ADR-0090 D3) — author '${type}' instead`, - { deprecated: a.type, canonical: type }, - ); - } - if (type === 'user') { - for (const u of await this.applyOooDelegation(String(a.value), now, organizationId, opts?.substitutions)) out.push(u); - continue; - } - if (type === 'field' && record) { - for (const u of await this.applyOooDelegation(String((record as any)[a.value] ?? ''), now, organizationId, opts?.substitutions)) out.push(u); - 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. + const groupKey = a.group != null && String(a.group) !== '' ? String(a.group) : `#${idx}`; + for (const u of ids) { + if (!u) continue; + out.push(u); + if (opts?.groups) (opts.groups[u] ??= []).push(groupKey); } - try { - if (type === 'team') { - const users = await this.expandTeamUsers(String(a.value)); - if (users.length) { for (const u of users) out.push(u); continue; } - } else if (type === 'department' || type === 'business_unit' || 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 (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 (type === 'org_membership_level') { - const users = await this.expandMembershipTierUsers(String(a.value), organizationId); - if (users.length) { for (const u of users) out.push(u); continue; } - } else if (type === 'manager' && record) { - const subject = (record as any)[a.value] ?? (record as any).owner_id; - if (subject) { - const mgr = await this.lookupManager(String(subject)); - if (mgr) { - for (const u of await this.applyOooDelegation(mgr, now, organizationId, opts?.substitutions)) out.push(u); - continue; - } - } - } - } catch { /* fall through */ } - out.push(`${a.type}:${a.value}`); } return out.filter(Boolean); } + /** + * Resolve ONE approver spec to concrete approver identities, applying OOO + * substitution (#1322) to individually-routed types. Extracted from + * {@link ApprovalService.expandApprovers} so the caller can tag each spec's + * resolved ids with a group (#3266) without duplicating the resolution logic. + * Returns the `type:value` literal as a single-element fallback when a graph + * lookup yields nothing — same behaviour as before the extraction. + */ + private async resolveApproverSpec( + a: any, + record: any, + organizationId: string | null | undefined, + now: number, + substitutions?: OooSubstitution[], + ): Promise { + // ADR-0090 D3: `role` is the deprecated spelling of `org_membership_level`. + // Resolve on the canonical type, but keep the AUTHORED spelling in the + // `type:value` fallback below — stored `sys_approval_approver` rows and + // `pending_approvers` slots from 15.x carry the old literal. + const type = canonicalApproverType(String(a.type)); + if (type !== a.type) { + this.logger?.warn?.( + `[approvals] approver type '${a.type}' is deprecated (ADR-0090 D3) — author '${type}' instead`, + { deprecated: a.type, canonical: type }, + ); + } + if (type === 'user') { + return this.applyOooDelegation(String(a.value), now, organizationId, substitutions); + } + if (type === 'field' && record) { + return this.applyOooDelegation(String((record as any)[a.value] ?? ''), now, organizationId, substitutions); + } + try { + if (type === 'team') { + const users = await this.expandTeamUsers(String(a.value)); + if (users.length) return users; + } else if (type === 'department' || type === 'business_unit' || type === 'bu') { + const users = await this.expandBusinessUnitUsers(String(a.value), organizationId); + if (users.length) return users; + } else if (type === 'position') { + const users = await this.expandPositionUsers(String(a.value), organizationId); + if (users.length) return users; + } else if (type === 'org_membership_level') { + const users = await this.expandMembershipTierUsers(String(a.value), organizationId); + if (users.length) return users; + } else if (type === 'manager' && record) { + const subject = (record as any)[a.value] ?? (record as any).owner_id; + if (subject) { + const mgr = await this.lookupManager(String(subject)); + if (mgr) return this.applyOooDelegation(mgr, now, organizationId, substitutions); + } + } + } catch { /* fall through */ } + return [`${a.type}:${a.value}`]; + } + /** Flat team — `sys_team` is better-auth's collaboration grouping (no hierarchy). */ private async expandTeamUsers(teamId: string): Promise { if (!teamId) return []; @@ -626,8 +649,12 @@ export class ApprovalService implements IApprovalService { // OOO auto-skip (#1322 M1): reroute individually-routed approvers who are // out of office. Collected hops drive the audit + notification below (M4). const substitutions: OooSubstitution[] = []; + // Group membership per resolved approver (#3266) — snapshotted so quorum / + // per_group finalization is decided against the slate resolved at OPEN time + // (OOO-substituted), not re-resolved live at each decision. + const groups: Record = {}; const approvers = await this.expandApprovers( - { approvers: input.config.approvers }, input.record, ctxOrg, { now: nowDate.getTime(), substitutions }, + { approvers: input.config.approvers }, input.record, ctxOrg, { now: nowDate.getTime(), substitutions, groups }, ); const now = nowDate.toISOString(); @@ -638,6 +665,10 @@ 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') { + configSnapshot.__approverGroups = groups; + } // 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. @@ -720,15 +751,54 @@ export class ApprovalService implements IApprovalService { } /** - * Record a decision on a node-driven request. Honours the node's `unanimous` - * behavior (holds until every approver has approved). When the request - * finalizes, returns the suspended run id + node id so the caller (or - * {@link ApprovalService.decide}) can resume the flow down the matching - * branch. + * True when the approve tally satisfies the node's `behavior` (#3266): + * - `unanimous` — every resolved approver approved. + * - `quorum` — at least `minApprovals` distinct approvals (default = all). + * - `per_group` — every group reached `minApprovals` approvals (default 1). + * Thresholds are clamped to the resolvable count / group size, so a mis-set + * value can never deadlock a request. + */ + private isApprovalSatisfied( + behavior: string, + config: ApprovalNodeConfig, + original: string[], + groupMap: Record, + approved: Set, + ): boolean { + if (behavior === 'unanimous') { + return original.length > 0 && original.every(a => approved.has(a)); + } + if (behavior === 'quorum') { + const n = original.length || 1; + const need = Math.min(Math.max(1, config.minApprovals ?? n), n); + // Count distinct approvals (robust to OOO/reassign changing who holds a slot). + return approved.size >= need; + } + if (behavior === 'per_group') { + const perGroupNeed = Math.max(1, config.minApprovals ?? 1); + const size: Record = {}; + for (const gs of Object.values(groupMap)) for (const g of gs) size[g] = (size[g] ?? 0) + 1; + const groups = Object.keys(size); + if (!groups.length) return true; // nothing to gate + const got: Record = {}; + for (const a of approved) for (const g of (groupMap[a] ?? [])) got[g] = (got[g] ?? 0) + 1; + return groups.every(g => (got[g] ?? 0) >= Math.min(perGroupNeed, size[g])); + } + return true; // first_response and unknown → first approval finalizes + } + + /** + * Record a decision on a node-driven request. Honours the node's `behavior` + * (#3266): `first_response` finalizes on the first approval; `unanimous`, + * `quorum`, and `per_group` hold the request open until their tally is met + * (see {@link ApprovalService.isApprovalSatisfied}). A rejection always + * finalizes the node (one veto). When the request finalizes, returns the + * suspended run id + node id so the caller (or {@link ApprovalService.decide}) + * can resume the flow down the matching branch. */ async decideNode( requestId: string, - input: { decision: 'approve' | 'reject'; actorId: string; comment?: string }, + input: { decision: 'approve' | 'reject'; actorId: string; comment?: string; attachments?: string[] }, context: SharingExecutionContext, ): Promise<{ request: ApprovalRequestRow; runId: string | null; nodeId: string | null; finalized: boolean; decision: 'approve' | 'reject' }> { if (!requestId) throw new Error('VALIDATION_FAILED: requestId is required'); @@ -756,24 +826,43 @@ export class ApprovalService implements IApprovalService { const runId: string | null = raw.flow_run_id ?? null; const now = this.clock.now().toISOString(); - // Audit the decision first so the unanimous tally below sees it. + // 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, step_name: nodeId, step_index: 0, action: input.decision, - actor_id: input.actorId, comment: input.comment ?? null, created_at: now, + actor_id: input.actorId, comment: input.comment ?? null, + attachments: input.attachments?.length ? input.attachments : null, + created_at: now, }, { context: SYSTEM_CTX }); - // Unanimous approve: advance only once every approver has approved. - if (input.decision === 'approve' && config.behavior === 'unanimous') { - const original = await this.expandApprovers( - { approvers: config.approvers }, parseJson(raw.payload_json, undefined), org, - ); + // Multi-approver aggregation on approve (#3266). A rejection always + // finalizes the node (one veto), so only the approve path can hold it open. + // `first_response` finalizes on the first approval (falls straight through). + const behavior = config.behavior ?? 'first_response'; + if (input.decision === 'approve' && behavior !== 'first_response') { const acts = await this.engine.find('sys_approval_action', { - where: { request_id: requestId, step_index: 0, action: 'approve' }, limit: 500, context: SYSTEM_CTX, + where: { request_id: requestId, step_index: 0, action: 'approve' }, limit: 1000, context: SYSTEM_CTX, }); const approved = new Set((acts ?? []).map((a: any) => String(a.actor_id ?? '')).filter(Boolean)); - const stillPending = original.filter(a => !approved.has(a)); - if (stillPending.length > 0) { + + // 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. + const snapshotGroups = (config as any).__approverGroups as Record | undefined; + let original: string[]; + let groupMap: Record; + if (snapshotGroups && (behavior === 'quorum' || behavior === 'per_group')) { + groupMap = snapshotGroups; + original = Object.keys(snapshotGroups); + } else { + original = await this.expandApprovers( + { approvers: config.approvers }, parseJson(raw.payload_json, undefined), org, + ); + groupMap = {}; + } + + if (!this.isApprovalSatisfied(behavior, config, original, groupMap, approved)) { + const stillPending = original.filter(a => !approved.has(a)); await this.engine.update('sys_approval_request', { id: requestId, pending_approvers: stillPending.join(','), updated_at: now, }, { context: SYSTEM_CTX }); @@ -1216,8 +1305,21 @@ export class ApprovalService implements IApprovalService { step_name: raw.flow_node_id ?? raw.current_step ?? null, step_index: 0, action: 'reassign', actor_id: input.actorId, comment: input.comment ?? `${from} → ${to}`, created_at: now, }, { context: SYSTEM_CTX }); + // per_group / quorum (#3266): carry the delegated slot's group membership to + // the new approver in the snapshot, so their approval still counts for the + // original group. + let configPatch: Record = {}; + try { + const cfg = parseJson(raw.node_config_json, null); + const groups = cfg?.__approverGroups as Record | undefined; + if (groups && groups[from] && !groups[to]) { + groups[to] = groups[from]; + delete groups[from]; + configPatch = { node_config_json: JSON.stringify(cfg) }; + } + } catch { /* snapshot left untouched on parse failure */ } await this.engine.update('sys_approval_request', { - id: requestId, pending_approvers: next.join(','), updated_at: now, + id: requestId, pending_approvers: next.join(','), updated_at: now, ...configPatch, }, { context: SYSTEM_CTX }); await this.syncApproverIndex(requestId, next, raw.organization_id ?? null, now); @@ -1463,7 +1565,7 @@ export class ApprovalService implements IApprovalService { /** Free-form reply on the thread (submitter or any pending approver). */ async comment( requestId: string, - input: { actorId: string; comment: string }, + input: { actorId: string; comment: string; attachments?: string[] }, context: SharingExecutionContext, ): Promise<{ request: ApprovalRequestRow }> { if (!input?.actorId) throw new Error('VALIDATION_FAILED: actorId is required'); @@ -1479,7 +1581,9 @@ export class ApprovalService implements IApprovalService { await this.engine.insert('sys_approval_action', { id: uid('aact'), request_id: requestId, organization_id: raw.organization_id ?? null, step_name: raw.flow_node_id ?? raw.current_step ?? null, step_index: 0, action: 'comment', - actor_id: input.actorId, comment: input.comment.trim(), created_at: now, + actor_id: input.actorId, comment: input.comment.trim(), + attachments: input.attachments?.length ? input.attachments : null, + created_at: now, }, { context: SYSTEM_CTX }); // Notify the other side of the thread. diff --git a/packages/plugins/plugin-approvals/src/sys-approval-action.object.ts b/packages/plugins/plugin-approvals/src/sys-approval-action.object.ts index 6596917563..bce6b1f63d 100644 --- a/packages/plugins/plugin-approvals/src/sys-approval-action.object.ts +++ b/packages/plugins/plugin-approvals/src/sys-approval-action.object.ts @@ -110,6 +110,14 @@ export const SysApprovalAction = ObjectSchema.create({ comment: Field.textarea({ label: 'Comment', required: false, group: 'Action' }), + attachments: Field.file({ + label: 'Attachments', + required: false, + multiple: true, + group: 'Action', + description: 'Files supporting this action — e.g. a signed contract or evidence (#3266).', + }), + created_at: Field.datetime({ label: 'Created At', required: true, diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index 6c4a68604f..da74ee8e7b 100644 --- a/packages/rest/src/rest-server.ts +++ b/packages/rest/src/rest-server.ts @@ -5899,6 +5899,7 @@ export class RestServer { decision, actorId: body.actorId ?? body.actor_id ?? context?.userId, comment: body.comment, + attachments: body.attachments, }, context ?? {}); res.json(out); } catch (err: any) { @@ -6007,6 +6008,7 @@ export class RestServer { return svc.comment(id, { actorId: body.actorId ?? body.actor_id ?? context?.userId, comment: body.comment, + attachments: body.attachments, }, context); }); diff --git a/packages/spec/src/automation/approval.zod.ts b/packages/spec/src/automation/approval.zod.ts index d9169b9fdb..4dc85ea2cb 100644 --- a/packages/spec/src/automation/approval.zod.ts +++ b/packages/spec/src/automation/approval.zod.ts @@ -142,6 +142,14 @@ export const ApprovalNodeApproverSchema = lazySchema(() => z.object({ }, }, }), + /** + * 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 + * `minApprovals` approvals — e.g. one legal AND one finance sign-off. Ignored + * by other behaviors. Approvers without a label each form their own group + * (keyed by position), so a plain per-approver list still behaves sensibly. + */ + group: z.string().optional().describe('Group label for per_group sign-off (e.g. "legal", "finance")'), })); export type ApprovalNodeApprover = z.infer; @@ -188,10 +196,28 @@ export const ApprovalNodeConfigSchema = lazySchema(() => z.object({ /** Who may act on this step. */ approvers: z.array(ApprovalNodeApproverSchema).min(1).describe('Allowed approvers for this node'), - /** How multiple approvers combine. (Enterprise adds quorum/weighted — ADR-0019 tiering.) */ - behavior: z.enum(['first_response', 'unanimous']).default('first_response') + /** + * How multiple approvers combine (#3266): + * - `first_response` — any one approval (or rejection) finalizes the node. + * - `unanimous` — every resolved approver must approve. + * - `quorum` — `minApprovals` of N approvals finalize (M-of-N collective sign-off). + * - `per_group` — each `group` (see approver `group`) must reach `minApprovals` + * approvals (default 1), i.e. one-from-each-group sign-off (会签). + * In every mode a single rejection finalizes the node as `rejected` (one veto). + * Weighted voting and approval-matrix governance are enterprise + * (objectstack-ai/cloud#861), not here. + */ + behavior: z.enum(['first_response', 'unanimous', 'quorum', 'per_group']).default('first_response') .describe('How to combine multiple approvers'), + /** + * Threshold for `quorum` (total approvals required, M of N) and `per_group` + * (approvals required from EACH group). Defaults to 1. Clamped at runtime so + * it can never exceed the resolvable approver count (no deadlock). + */ + minApprovals: z.number().int().min(1).optional() + .describe('Approvals required — total (quorum) or per group (per_group). Default 1'), + /** Lock the triggering record from edits while this node is pending. */ lockRecord: z.boolean().default(true).describe('Lock the record from editing while pending'), diff --git a/packages/spec/src/contracts/approval-service.ts b/packages/spec/src/contracts/approval-service.ts index 92b5c2290a..142d4162d0 100644 --- a/packages/spec/src/contracts/approval-service.ts +++ b/packages/spec/src/contracts/approval-service.ts @@ -140,6 +140,12 @@ export interface ApprovalDecisionInput { decision: 'approve' | 'reject'; actorId: string; comment?: string; + /** + * File references (already stored via the storage service) to attach to this + * decision — e.g. a signed contract or an evidence PDF (#3266). Recorded on + * the `sys_approval_action` audit row's `attachments` field. + */ + attachments?: string[]; } /** Input for recalling (withdrawing) a pending request. */ From 0983b1447a26fe851b060723c3af98ed6d9b6a73 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 19 Jul 2026 10:53:27 +0000 Subject: [PATCH 2/2] fix(approvals): regen approval reference doc + behavior test/docs for quorum/per_group (#3266) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI follow-ups on the #3266 spec change: - Regenerate content/docs/references/automation/approval.mdx (generated from the spec schema; the new behavior enum / minApprovals / approver group made it stale). - approval.test.ts: the "rejects an unknown behavior" case hard-coded 'quorum' as its unknown value — now a valid behavior. Switch it to 'weighted' and add positive coverage that quorum / per_group / minApprovals / grouped approvers parse. - approvals.mdx: document the behavior matrix (first_response / unanimous / quorum / per_group), the veto rule, minApprovals clamping, and decision attachments. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_016ypkQikZ55oWUHUnMebwXA --- content/docs/automation/approvals.mdx | 29 +++++++++++++++++++ .../docs/references/automation/approval.mdx | 6 ++-- packages/spec/src/automation/approval.test.ts | 17 ++++++++++- 3 files changed, 49 insertions(+), 3 deletions(-) diff --git a/content/docs/automation/approvals.mdx b/content/docs/automation/approvals.mdx index abd31296f5..de4152496d 100644 --- a/content/docs/automation/approvals.mdx +++ b/content/docs/automation/approvals.mdx @@ -117,6 +117,35 @@ export const opportunityApproval: Flow = { For multi-step approvals, chain multiple `approval` nodes. For parallel approvals, see the aggregating-node pattern in [Flow Metadata](/docs/automation/flows). +### Combining multiple approvers (#3266) + +When a node has several approvers, `behavior` decides what "approved" means — +one node, no parallel branches needed: + +| `behavior` | Advances when… | Use for | +|:---|:---|:---| +| `first_response` (default) | any one approves | single reviewer / "any manager" | +| `unanimous` | every resolved approver approves | small fixed panels | +| `quorum` | `minApprovals` of N approve (M-of-N) | "2 of 3 directors" | +| `per_group` | each `group` reaches `minApprovals` (default 1) | "legal **and** finance" sign-off (会签) | + +```ts +// One legal AND one finance approver must sign off; either rejection vetoes. +{ type: 'approval', config: { + approvers: [ + { type: 'position', value: 'legal_counsel', group: 'legal' }, + { type: 'position', value: 'controller', group: 'finance' }, + ], + behavior: 'per_group', // or 'quorum' with minApprovals: 2 +} } +``` + +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. + ## The full lifecycle — submit to field change What actually happens between "the flow hits the approval node" and "the record diff --git a/content/docs/references/automation/approval.mdx b/content/docs/references/automation/approval.mdx index 204eb44a97..fed696572c 100644 --- a/content/docs/references/automation/approval.mdx +++ b/content/docs/references/automation/approval.mdx @@ -56,6 +56,7 @@ const result = ApprovalDecision.parse(data); | :--- | :--- | :--- | :--- | | **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` | +| **group** | `string` | optional | Group label for per_group sign-off (e.g. "legal", "finance") | --- @@ -66,8 +67,9 @@ 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 }[]` | ✅ | Allowed approvers for this node | -| **behavior** | `Enum<'first_response' \| 'unanimous'>` | ✅ | How to combine multiple approvers | +| **approvers** | `{ type: Enum<'user' \| 'org_membership_level' \| 'role' \| 'position' \| 'team' \| 'department' \| 'manager' \| 'field' \| 'queue'>; value?: string; 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 | | **escalation** | `{ enabled: boolean; timeoutHours: number; action: Enum<'reassign' \| 'auto_approve' \| 'auto_reject' \| 'notify'>; escalateTo?: string; … }` | optional | Per-node SLA escalation | diff --git a/packages/spec/src/automation/approval.test.ts b/packages/spec/src/automation/approval.test.ts index 84e9927e14..02ea3dc11e 100644 --- a/packages/spec/src/automation/approval.test.ts +++ b/packages/spec/src/automation/approval.test.ts @@ -133,7 +133,22 @@ describe('ApprovalNodeConfigSchema', () => { }); it('rejects an unknown behavior', () => { - expect(() => ApprovalNodeConfigSchema.parse({ ...minimal, behavior: 'quorum' })).toThrow(); + expect(() => ApprovalNodeConfigSchema.parse({ ...minimal, behavior: 'weighted' })).toThrow(); + }); + + it('accepts quorum / per_group behaviors with minApprovals and grouped approvers (#3266)', () => { + const quorum = ApprovalNodeConfigSchema.parse({ ...minimal, behavior: 'quorum', minApprovals: 2 }); + expect(quorum.behavior).toBe('quorum'); + expect(quorum.minApprovals).toBe(2); + const perGroup = ApprovalNodeConfigSchema.parse({ + approvers: [ + { type: 'position', value: 'legal_counsel', group: 'legal' }, + { type: 'position', value: 'controller', group: 'finance' }, + ], + behavior: 'per_group', + }); + expect(perGroup.behavior).toBe('per_group'); + expect(perGroup.approvers[0].group).toBe('legal'); }); });