From 2cd9a3611e9c1e7e59124d145237ee754d63bd31 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 28 Jul 2026 07:44:21 +0000 Subject: [PATCH 1/2] feat(approvals): cross-organization approver targeting (ADR-0105 D9) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit One organization id decided three different things at once in `openNodeRequest`: where the request row lives, where its inbox index rows live, and where its approvers are looked up. The first two are the request's own organization by definition. The third is not — a group CFO holds her `cfo` position in the GROUP organization while the purchase order she signs off lives in the PLANT organization. `expandPositionUsers('cfo', )` matched nobody, the slot fell back to the dead `position:cfo` literal, and a group escalation could not be expressed at all. An approver may now declare which organization's directory resolves it: - { type: position, value: plant_manager, group: plant } - { type: position, value: cfo, organization: $root, group: finance } Four design points the ADR left open, settled from what the code and the authoring model require (ADR text amended in this commit): - PER APPROVER, not per node. The node-level form cannot express the commonest group shape — one node requiring a plant manager AND a group CFO in parallel. Splitting into serial nodes changes the semantics rather than the syntax. A node-level default remains addable later as sugar; the reverse is not true. - SYMBOLS FIRST (`$root` / `$parent`). Flow metadata is portable across environments; an organization id is data minted per deployment, so a literal id in a flow is unportable by construction and an AI author cannot know one. The symbols walk D6's `parent_organization_id` tree with zero deployment knowledge. A slug covers what they cannot — notably a SIBLING organization (a shared-services centre approving payables for every plant). - LEGALITY IS "SHARES A ROOT", not "is an ancestor" — the sibling case above is first-class. The rule reads only the organization tree, never the submitter, so one flow routes identically for everyone and a routing bug is reproducible. - NON-`group` POSTURES REFUSE at runtime. Posture is environment configuration, so the same portable metadata deploys into any posture and no static check can see which; ignoring the declaration would let a group → isolated migration reroute approvals with no signal. Two failure modes are made loud rather than silent: an approver type with no org-scoped directory (`user`/`field`/`manager`/`team`) refuses the declaration instead of ignoring it, with a new `approval-approver-cross-org-unsupported` lint catching it at author time; and a targeted approver holding no membership in the request's organization is dropped with a warning naming them — D2's union wall would otherwise hide the request from someone already routed to, leaving a task she cannot open. Dropping hands it to the node's existing `onEmptyApprovers` policy. An approver without `organization` is unchanged: same resolution, same queries, no extra reads. Tests: 17 resolver units (symbols, sibling/foreign slugs, posture, cycles, unreadable-membership), 7 service integration (including the pre-D9 gap, the mixed plant+group node, and that the request still belongs to the request org), 4 lint. plugin-approvals 305, lint 500, spec 6736, cli 682 all green. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015FebXPaaGrLhGKw1LHPbpL --- ...dr-0105-d9-cross-org-approver-targeting.md | 53 ++++ .../docs/references/automation/approval.mdx | 3 +- ...nancy-posture-and-first-class-org-scope.md | 51 +++- packages/lint/src/index.ts | 1 + .../src/validate-approval-approvers.test.ts | 37 +++ .../lint/src/validate-approval-approvers.ts | 28 ++ .../plugin-approvals/src/approval-service.ts | 86 +++++- .../plugin-approvals/src/approvals-plugin.ts | 14 + .../approver-cross-org.integration.test.ts | 179 ++++++++++++ .../src/approver-org-scope.test.ts | 201 ++++++++++++++ .../src/approver-org-scope.ts | 261 ++++++++++++++++++ packages/spec/api-surface.json | 4 + packages/spec/src/automation/approval.zod.ts | 96 +++++++ 13 files changed, 1001 insertions(+), 13 deletions(-) create mode 100644 .changeset/adr-0105-d9-cross-org-approver-targeting.md create mode 100644 packages/plugins/plugin-approvals/src/approver-cross-org.integration.test.ts create mode 100644 packages/plugins/plugin-approvals/src/approver-org-scope.test.ts create mode 100644 packages/plugins/plugin-approvals/src/approver-org-scope.ts diff --git a/.changeset/adr-0105-d9-cross-org-approver-targeting.md b/.changeset/adr-0105-d9-cross-org-approver-targeting.md new file mode 100644 index 0000000000..aaf2820581 --- /dev/null +++ b/.changeset/adr-0105-d9-cross-org-approver-targeting.md @@ -0,0 +1,53 @@ +--- +"@objectstack/plugin-approvals": minor +"@objectstack/spec": minor +"@objectstack/lint": minor +--- + +feat(approvals): cross-organization approver targeting — a plant document can +require a group-side sign-off (ADR-0105 D9) + +One organization id used to decide three different things at once in +`openNodeRequest`: where the request row lives, where its inbox index rows +live, and **where its approvers are looked up**. The first two are the +request's own organization by definition. The third is not — a group CFO holds +her `cfo` position in the GROUP organization while the purchase order she signs +off lives in the PLANT organization. `expandPositionUsers('cfo', )` +matched nobody, the slot fell back to the dead `position:cfo` literal, and a +group escalation could not be expressed at all. + +An approver may now declare which organization's directory resolves it: + +```yaml +approvers: + - { type: position, value: plant_manager, group: plant } + - { type: position, value: cfo, organization: $root, group: finance } +behavior: per_group +``` + +- **`$root` / `$parent`** walk D6's `parent_organization_id` tree, so the two + common intents need **no deployment knowledge** — flow metadata is portable + across environments while organization ids are minted per deployment. A slug + covers what the symbols cannot, notably a **sibling** organization (a + shared-services centre approving payables for every plant). +- Declared **per approver**, so one node can require a plant manager and a + group CFO in parallel. A node-level form cannot express that without + splitting into serial nodes, which changes the semantics. +- **Bounded, not free:** the target must share a `parent_organization_id` root + with the request's organization. The rule reads only the organization tree — + never the submitter — so one flow routes identically for everyone. + +Everything else fails loudly rather than quietly: + +- a non-`group` posture **refuses** the declaration (a `group` → `isolated` + migration must not silently reroute approvals); +- an approver type with no org-scoped directory (`user` / `field` / `manager` / + `team`) refuses it too, and a new `approval-approver-cross-org-unsupported` + lint catches that at author time; +- a targeted approver holding no membership in the request's organization is + dropped with a warning naming them — D2's union wall would otherwise hide the + request from someone already routed to, so the node's existing + `onEmptyApprovers` policy takes over instead of leaving an unopenable task. + +Nothing changes for an approver without `organization`: same resolution, same +queries, no extra reads. diff --git a/content/docs/references/automation/approval.mdx b/content/docs/references/automation/approval.mdx index 7521b9b912..cda235a42d 100644 --- a/content/docs/references/automation/approval.mdx +++ b/content/docs/references/automation/approval.mdx @@ -64,6 +64,7 @@ const result = ApprovalDecision.parse(data); | **value** | `string` | optional | User id / membership tier / position / team / department / field — per `type`; for `expression`, a CEL expression over `current.*` / `trigger.*` / `vars.*` | | **resolveAs** | `Enum<'user' \| 'department' \| 'position' \| 'team'>` | optional | How an `expression` result is expanded into approvers (default 'user') | | **group** | `string` | optional | Group label for per_group sign-off (e.g. "legal", "finance") | +| **organization** | `string` | optional | ADR-0105 D9 — organization whose directory resolves this approver: `$root` (group org), `$parent` (one level up), or an organization slug. Omitted = the request's own organization. | --- @@ -74,7 +75,7 @@ const result = ApprovalDecision.parse(data); | Property | Type | Required | Description | | :--- | :--- | :--- | :--- | -| **approvers** | `{ type: Enum<'manager' \| 'position' \| 'department' \| 'team' \| 'field' \| 'expression' \| 'org_membership_level' \| 'role' \| 'user' \| 'queue'>; value?: string; resolveAs?: Enum<'user' \| 'department' \| 'position' \| 'team'>; group?: string }[]` | ✅ | Allowed approvers for this node | +| **approvers** | `{ type: Enum<'manager' \| 'position' \| 'department' \| 'team' \| 'field' \| 'expression' \| 'org_membership_level' \| 'role' \| 'user' \| 'queue'>; value?: string; resolveAs?: Enum<'user' \| 'department' \| 'position' \| 'team'>; group?: string; … }[]` | ✅ | Allowed approvers for this node | | **behavior** | `Enum<'first_response' \| 'unanimous' \| 'quorum' \| 'per_group'>` | ✅ | How to combine multiple approvers | | **minApprovals** | `integer` | optional | Approvals required — total (quorum) or per group (per_group). Default 1 | | **lockRecord** | `boolean` | ✅ | Lock the record from editing while pending | diff --git a/docs/adr/0105-group-tenancy-posture-and-first-class-org-scope.md b/docs/adr/0105-group-tenancy-posture-and-first-class-org-scope.md index 34e2d617fe..31824bad83 100644 --- a/docs/adr/0105-group-tenancy-posture-and-first-class-org-scope.md +++ b/docs/adr/0105-group-tenancy-posture-and-first-class-org-scope.md @@ -249,14 +249,53 @@ the `single`-posture deployment (plant-autonomous member admission) and is the natural admission UX in `group` posture. **D9 — Cross-org approval targeting.** -An approval chain node may name a **target organization** for approver -resolution (default: the request's org, today's behavior at -`approval-node.ts:118`). A plant document's escalation step declares -`organization: ` and resolves group-side position holders. Reads of -the request by those approvers are covered by D2 (membership union) in `group` -posture; in `isolated` posture this decision does not apply (cross-org +An approval **approver** may name a **target organization** for its resolution +(default: the request's org, today's behavior). A plant document's escalation +step declares `organization: $root` and resolves group-side position holders. +Reads of the request by those approvers are covered by D2 (membership union) in +`group` posture; in `isolated` posture this decision does not apply (cross-org approval there remains mirroring via system context, cloud #2937 contract). +*Amended during implementation (#3812). Four points the original text left +open, each settled from what the code and the authoring model actually +require:* + +1. *Per **approver**, not per node.* The node-level form cannot express the + commonest group shape — one node requiring a plant manager **and** a group + CFO in parallel (`behavior: 'per_group'`). Expressing it as two serial nodes + changes the semantics (parallel co-sign becomes sequential approval), so the + node-level form distorts the model rather than merely inconveniencing the + author. A node-level default is a strict special case of the per-approver + form and remains addable later as sugar; the reverse is not true. +2. *Symbolic references first: `$root`, `$parent`.* Flow metadata is portable + across environments; an organization id is data minted per deployment. A + literal id in a flow is therefore unportable by construction, and an AI + author cannot know one at authoring time. The symbols express the two common + intents against D6's tree with **zero deployment knowledge**. A slug covers + what they cannot — notably a **sibling** organization (a shared-services + centre approving payables for every plant). +3. *Legality is "shares a `parent_organization_id` root", not "is an + ancestor".* The sibling case above is first-class, as is downward targeting. + The rule depends only on the organization tree and **never on the + submitter**, so one flow routes identically for everyone — which is what + makes a routing bug reproducible. A deployment with no grouping metadata + gets a refusal naming D6, not a silent pass. +4. *Non-`group` postures **refuse** at runtime rather than ignoring.* Posture is + environment configuration, so the same portable metadata may be deployed + into any posture and no static check can see which. Silently ignoring the + declaration would let a `group` → `isolated` migration reroute approvals + with no signal — an audit event, not a config detail. + +*Two consequences worth stating, both enforced:* an approver type that resolves +no org-scoped directory (`user` / `field` / `manager` / `team`) **refuses** the +declaration instead of ignoring it (an author who wrote it believed it did +something); and a targeted approver who holds no membership in the request's +organization is **dropped with a warning** naming them, because D2's union +would otherwise hide the request from someone the router had already committed +to — routing succeeds, the inbox row is written, and the approver opens a task +she cannot open. Dropping converts that silent dead-end into the node's +existing `onEmptyApprovers` policy. + **D10 — Layered master data (group template + org override).** A spec-level pattern for the SAP material-master / 用友-金蝶 distribution shape: an object may declare layered governance — group-level template rows diff --git a/packages/lint/src/index.ts b/packages/lint/src/index.ts index 4a16970fce..466c5d5681 100644 --- a/packages/lint/src/index.ts +++ b/packages/lint/src/index.ts @@ -135,6 +135,7 @@ export { APPROVAL_EXPRESSION_INVALID, APPROVAL_EXPRESSION_NO_EMPTY_POLICY, APPROVAL_DECISION_OUTPUTS_RESERVED, + APPROVAL_APPROVER_CROSS_ORG_UNSUPPORTED, } from './validate-approval-approvers.js'; export type { ApprovalApproverFinding, ApprovalApproverSeverity } from './validate-approval-approvers.js'; diff --git a/packages/lint/src/validate-approval-approvers.test.ts b/packages/lint/src/validate-approval-approvers.test.ts index 492dac6e9a..deb74d516c 100644 --- a/packages/lint/src/validate-approval-approvers.test.ts +++ b/packages/lint/src/validate-approval-approvers.test.ts @@ -12,6 +12,7 @@ import { APPROVAL_EXPRESSION_INVALID, APPROVAL_EXPRESSION_NO_EMPTY_POLICY, APPROVAL_DECISION_OUTPUTS_RESERVED, + APPROVAL_APPROVER_CROSS_ORG_UNSUPPORTED, } from './validate-approval-approvers.js'; function stackWithApprovers(approvers: unknown[]): Record { @@ -334,3 +335,39 @@ describe('expression approvers (#3447 P2)', () => { }))).toEqual([]); }); }); + +describe('cross-organization targeting (ADR-0105 D9)', () => { + it('is clean when `organization` sits on an org-scoped approver type', () => { + // The whole point of D9: a group CFO resolved against the group directory. + const findings = validateApprovalApprovers( + stackWithApprovers([{ type: 'position', value: 'cfo', organization: '$root' }]), + ); + expect(findings.filter(f => f.rule === APPROVAL_APPROVER_CROSS_ORG_UNSUPPORTED)).toEqual([]); + }); + + it('errors when `organization` sits on a type with no organization directory', () => { + // `user` names a person outright — the declaration cannot narrow anything, + // and the runtime refuses it. Catch it at author time instead. + const findings = validateApprovalApprovers( + stackWithApprovers([{ type: 'user', value: 'u1', organization: '$root' }]), + ); + const f = findings.find(x => x.rule === APPROVAL_APPROVER_CROSS_ORG_UNSUPPORTED); + expect(f?.severity).toBe('error'); + expect(f?.path).toBe('flows[0].nodes[1].config.approvers[0].organization'); + expect(f?.hint).toMatch(/position.*org_membership_level.*department.*expression/); + }); + + it('errors for `team` too — team membership carries no organization', () => { + const findings = validateApprovalApprovers( + stackWithApprovers([{ type: 'team', value: 't1', organization: 'acme-ssc' }]), + ); + expect(findings.some(f => f.rule === APPROVAL_APPROVER_CROSS_ORG_UNSUPPORTED)).toBe(true); + }); + + it('stays silent when no organization is declared — the default path', () => { + const findings = validateApprovalApprovers( + stackWithApprovers([{ type: 'user', value: 'u1' }]), + ); + expect(findings.filter(f => f.rule === APPROVAL_APPROVER_CROSS_ORG_UNSUPPORTED)).toEqual([]); + }); +}); diff --git a/packages/lint/src/validate-approval-approvers.ts b/packages/lint/src/validate-approval-approvers.ts index ffaf625d88..e0cd38ef16 100644 --- a/packages/lint/src/validate-approval-approvers.ts +++ b/packages/lint/src/validate-approval-approvers.ts @@ -24,6 +24,7 @@ * | approval-expression-invalid | error/info | #3447 P2 closed-root expressions | * | approval-expression-no-empty-policy | info | #3447 P2 empty-slate policy | * | approval-decision-outputs-reserved | error | #3447 P2 resume envelope | + * | approval-approver-cross-org-unsupported | error | ADR-0105 D9 targeting | * * The first two are mutually exclusive by construction — a bad *value* wins, * because its fix (`position`) differs from the deprecation's fix @@ -42,6 +43,7 @@ import { APPROVAL_NODE_TYPE, DEPRECATED_APPROVER_TYPES, APPROVER_VALUE_BINDINGS, + approverTypeIsOrgScoped, canonicalApproverType, normalizeDecisionOutputs, } from '@objectstack/spec/automation'; @@ -57,6 +59,7 @@ export const APPROVAL_APPROVERS_MAY_RESOLVE_EMPTY = 'approval-approvers-may-reso export const APPROVAL_EXPRESSION_INVALID = 'approval-expression-invalid'; export const APPROVAL_EXPRESSION_NO_EMPTY_POLICY = 'approval-expression-no-empty-policy'; export const APPROVAL_DECISION_OUTPUTS_RESERVED = 'approval-decision-outputs-reserved'; +export const APPROVAL_APPROVER_CROSS_ORG_UNSUPPORTED = 'approval-approver-cross-org-unsupported'; /** * The CLOSED root set an `expression` approver may reference (#3447 P2) — @@ -304,6 +307,31 @@ export function validateApprovalApprovers(stack: AnyRec): ApprovalApproverFindin `Queue approvers need a real ownership-queue implementation before they take effect.`, }); } + + // [ADR-0105 D9] Cross-organization targeting on a type that has no + // organization-scoped directory. `user` / `field` / `manager` name a + // person outright and `team` membership carries no organization, so the + // declaration cannot narrow anything — it is a misunderstanding of what + // the field does, and the runtime refuses it. Error, not warning: this + // is a certain authoring mistake with a certain fix, and letting it + // reach the runtime turns author time into an incident. + const declaredOrg = (a as AnyRec).organization; + if (typeof declaredOrg === 'string' && declaredOrg.trim() !== '' + && ApproverType.options.includes(canonical as never) + && !approverTypeIsOrgScoped(canonical)) { + findings.push({ + severity: 'error', + rule: APPROVAL_APPROVER_CROSS_ORG_UNSUPPORTED, + where, + path: `${path}.organization`, + message: + `approver type '${type}' does not resolve through an organization directory, so ` + + `'organization: ${declaredOrg}' has no effect (ADR-0105 D9) — the runtime refuses it.`, + hint: + `Drop 'organization' here. Cross-organization targeting applies to ` + + `'position', 'org_membership_level', 'department' and 'expression' approvers.`, + }); + } } // Empty-slate dead-end (#3424): when EVERY approver on the node routes to diff --git a/packages/plugins/plugin-approvals/src/approval-service.ts b/packages/plugins/plugin-approvals/src/approval-service.ts index dc591e072d..a36eb31ffc 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.ts @@ -3,6 +3,7 @@ import { createHash, randomBytes } from 'node:crypto'; import { APPROVAL_BRANCH_LABELS, + approverTypeIsOrgScoped, canonicalApproverType, normalizeDecisionOutputs, type ApprovalNodeConfig, @@ -33,6 +34,12 @@ import type { } from '@objectstack/spec/contracts'; import { isFileIdToken } from '@objectstack/spec/data'; import { isGrantActive } from '@objectstack/core'; +import { + filterApproversWhoCanRead, + resolveApproverDirectoryOrg, + type ApproverOrgScopeDeps, + type ApproverOrgScopeEngine, +} from './approver-org-scope.js'; /** * Node-era approval runtime (ADR-0019). @@ -416,6 +423,14 @@ export interface ApprovalServiceOptions { * the Console and IM webviews; outbound email needs the absolute form. */ publicBaseUrl?: string; + /** + * [ADR-0105 D9] The tenancy posture in force. Cross-organization approver + * targeting is a `group`-posture capability; the resolver refuses the + * declaration under any other posture rather than silently ignoring it. + * Absent (a stack booted with no tenancy service) reads as "unknown" and the + * guard stands down. + */ + tenancyPosture?: () => string | undefined; } export class ApprovalService implements IApprovalService { @@ -425,6 +440,7 @@ export class ApprovalService implements IApprovalService { private automation?: ApprovalResumeSurface; private messaging?: ApprovalMessagingSurface; private publicBaseUrl: string; + private tenancyPosture?: () => string | undefined; constructor(opts: ApprovalServiceOptions) { this.engine = opts.engine; @@ -433,6 +449,36 @@ export class ApprovalService implements IApprovalService { this.automation = opts.automation; this.messaging = opts.messaging; this.publicBaseUrl = (opts.publicBaseUrl ?? '').replace(/\/$/, ''); + this.tenancyPosture = opts.tenancyPosture; + } + + /** Attach (or replace) the ADR-0105 D9 posture provider. */ + attachTenancyPosture(provider: () => string | undefined): void { + this.tenancyPosture = provider; + } + + /** Deps bundle for the ADR-0105 D9 org-scope helpers. */ + private get orgScopeDeps(): ApproverOrgScopeDeps { + return { + engine: this.engine as unknown as ApproverOrgScopeEngine, + posture: this.tenancyPosture, + logger: this.logger, + }; + } + + /** + * [ADR-0105 D9] Which organization's directory resolves ONE approver spec. + * Absent declaration ⇒ the request's own organization (unchanged, no reads). + */ + private async directoryOrgFor(a: any, requestOrgId: string | null | undefined): Promise { + const rawType = String(a?.type ?? ''); + return resolveApproverDirectoryOrg( + this.orgScopeDeps, + a?.organization, + requestOrgId, + rawType, + approverTypeIsOrgScoped(rawType), + ); } /** Attach (or replace) the automation surface used to resume flow runs. */ @@ -740,18 +786,35 @@ export class ApprovalService implements IApprovalService { } return out; } + // [ADR-0105 D9] WHERE this approver is looked up — the request's own + // organization unless the spec targets another one in the same group. + // Resolution failures propagate: they are routing bugs, and this call is + // OUTSIDE the swallowing try below on purpose (see the catch's comment). + const directoryOrg = await this.directoryOrgFor(a, organizationId); + const crossOrg = directoryOrg !== organizationId; + // A cross-org slate is filtered to the people who can actually READ the + // request (D2 union); same-org routing is untouched and does no extra read. + const bounded = async (users: string[]): Promise => ( + crossOrg + ? filterApproversWhoCanRead(this.orgScopeDeps, users, organizationId, { + approverType: type, value: a.value != null ? String(a.value) : undefined, + directoryOrgId: directoryOrg, + }) + : users + ); + 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); + const users = await bounded(await this.expandBusinessUnitUsers(String(a.value), directoryOrg)); if (users.length) return users; } else if (type === 'position') { - const users = await this.expandPositionUsers(String(a.value), organizationId); + const users = await bounded(await this.expandPositionUsers(String(a.value), directoryOrg)); if (users.length) return users; } else if (type === 'org_membership_level') { - const users = await this.expandMembershipTierUsers(String(a.value), organizationId); + const users = await bounded(await this.expandMembershipTierUsers(String(a.value), directoryOrg)); if (users.length) return users; } else if (type === 'manager' && record) { const subject = (record as any)[a.value] ?? (record as any).owner_id; @@ -760,7 +823,7 @@ export class ApprovalService implements IApprovalService { if (mgr) return this.applyOooDelegation(mgr, now, organizationId, substitutions); } } - } catch { /* fall through */ } + } catch { /* a directory lookup failed → fall through to the literal slot */ } // #3508: `queue` is declared-but-unenforced — there is no queue branch // above, so a queue approver always lands here and the `queue:` slot // routes to nobody. The spec marks it non-authorable @@ -893,12 +956,18 @@ export class ApprovalService implements IApprovalService { } return { slots, raw }; } + // [ADR-0105 D9] An expression that re-expands into a graph kind consults the + // same org-scoped directories the static types do, so it honours the same + // targeting. Resolved once for the whole slate, before the per-value loop — + // the declaration is a property of the spec, not of what the CEL returned. + const directoryOrg = await this.directoryOrgFor(a, organizationId); + const crossOrg = directoryOrg !== organizationId; const slots: Array<{ id: string; subGroup: string }> = []; for (const key of raw) { let users: string[] = []; try { - if (resolveAs === 'department') users = await this.expandBusinessUnitUsers(key, organizationId); - else if (resolveAs === 'position') users = await this.expandPositionUsers(key, organizationId); + if (resolveAs === 'department') users = await this.expandBusinessUnitUsers(key, directoryOrg); + else if (resolveAs === 'position') users = await this.expandPositionUsers(key, directoryOrg); else if (resolveAs === 'team') users = await this.expandTeamUsers(key); else { throw new Error( @@ -910,6 +979,11 @@ export class ApprovalService implements IApprovalService { if (String(err?.message ?? '').startsWith('VALIDATION_FAILED')) throw err; users = []; } + if (crossOrg && users.length) { + users = await filterApproversWhoCanRead(this.orgScopeDeps, users, organizationId, { + approverType: resolveAs, value: key, directoryOrgId: directoryOrg, + }); + } if (!users.length) { slots.push({ id: `${resolveAs}:${key}`, subGroup: key }); continue; diff --git a/packages/plugins/plugin-approvals/src/approvals-plugin.ts b/packages/plugins/plugin-approvals/src/approvals-plugin.ts index f83b08bcdd..0b4ded6574 100644 --- a/packages/plugins/plugin-approvals/src/approvals-plugin.ts +++ b/packages/plugins/plugin-approvals/src/approvals-plugin.ts @@ -121,6 +121,20 @@ export class ApprovalsServicePlugin implements Plugin { engine: engine as ApprovalEngine, logger: ctx.logger, publicBaseUrl: this.options.publicBaseUrl, + // [ADR-0105 D9] Cross-organization approver targeting is a `group`-posture + // capability. Read LAZILY (not captured at start) because the tenancy + // service resolves its posture during its own start, which may not have + // run yet; an unresolvable posture reads as "unknown" and the guard + // stands down rather than refusing a legitimate flow on a minimal stack. + tenancyPosture: () => { + try { + const tenancy = ctx.getService<{ posture?: string }>('tenancy'); + const posture = tenancy?.posture; + return typeof posture === 'string' && posture ? posture : undefined; + } catch { + return undefined; + } + }, }); // Record lock: block edits to a record while it has a pending request. diff --git a/packages/plugins/plugin-approvals/src/approver-cross-org.integration.test.ts b/packages/plugins/plugin-approvals/src/approver-cross-org.integration.test.ts new file mode 100644 index 0000000000..6f449d3405 --- /dev/null +++ b/packages/plugins/plugin-approvals/src/approver-cross-org.integration.test.ts @@ -0,0 +1,179 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [ADR-0105 D9] Cross-organization approver targeting, through the SERVICE. + * + * `approver-org-scope.test.ts` owns the resolver's own rules. What only this + * level can show is that the resolved organization actually reaches the + * directory lookups — the wiring D9 is: one organization id used to decide + * three different things at once (where the request lives, where its inbox rows + * live, where its approvers are looked up), and only the third moves. + * + * The scenario is the one D9 exists for: a purchase order raised in PLANT A + * needs the group CFO, who holds `cfo` in the GROUP organization and would have + * matched nobody before. + */ + +import { describe, it, expect, beforeEach } from 'vitest'; +import { ApprovalService } from './approval-service.js'; + +interface Row { [k: string]: any } + +function makeEngine(seed: Record = {}) { + const tables: Record = { ...seed }; + const ensure = (n: string) => (tables[n] ??= []); + const matches = (row: Row, filter: any): boolean => { + if (!filter || typeof filter !== 'object') return true; + for (const [k, v] of Object.entries(filter)) { + if (k === '$or') { + if (!(v as any[]).some((sub) => matches(row, sub))) return false; + continue; + } + const rv = row[k]; + if (v != null && typeof v === 'object' && '$in' in (v as any)) { + if (!(v as any).$in.includes(rv)) return false; + continue; + } + if (v != null && typeof v === 'object' && '$ne' in (v as any)) { + if (rv === (v as any).$ne) return false; + continue; + } + if (rv !== v) return false; + } + return true; + }; + return { + _tables: tables, + async find(object: string, options?: any) { + return ensure(object).filter((r) => matches(r, options?.filter ?? options?.where)); + }, + async insert(object: string, data: Row) { ensure(object).push({ ...data }); return { ...data }; }, + async update(_o: string, _w: any, _d: any) { return {}; }, + async delete() { return {}; }, + async count(object: string) { return ensure(object).length; }, + registerHook() { /* no-op */ }, + unregisterHooksByPackage() { /* no-op */ }, + }; +} + +/** Plant A sits under the group; the CFO's position lives in the group org. */ +const SEED = () => ({ + sys_organization: [ + { id: 'o_group', slug: 'acme-group', parent_organization_id: null }, + { id: 'o_plant', slug: 'acme-plant-a', parent_organization_id: 'o_group' }, + ], + sys_user_position: [ + { id: 'up1', user_id: 'u_cfo', position: 'cfo', organization_id: 'o_group' }, + { id: 'up2', user_id: 'u_plant_mgr', position: 'plant_manager', organization_id: 'o_plant' }, + ], + sys_member: [ + // The intended group shape: group staff hold a membership in every plant + // (so D2's union lets them READ the request) while their POSITION lives in + // the group organization. + { id: 'm1', user_id: 'u_cfo', organization_id: 'o_plant', role: 'member' }, + { id: 'm2', user_id: 'u_plant_mgr', organization_id: 'o_plant', role: 'member' }, + ], +}); + +const CTX = { userId: 'u_submitter', organizationId: 'o_plant', positions: [], permissions: [] } as any; + +function openInput(approvers: any[], extra: Record = {}) { + return { + object: 'purchase_order', + recordId: 'po1', + runId: 'run_1', + nodeId: 'group_signoff', + flowName: 'po_approval', + config: { approvers, behavior: 'unanimous' as const, lockRecord: false }, + record: { id: 'po1', amount: 500000 }, + ...extra, + }; +} + +describe('ADR-0105 D9 — cross-org approver targeting through ApprovalService', () => { + let engine: ReturnType; + let svc: ApprovalService; + let n = 0; + + beforeEach(() => { + engine = makeEngine(SEED()); + n = 0; + svc = new ApprovalService({ + engine: engine as any, + clock: { now: () => new Date(1767000000000 + (n++) * 1000) }, + tenancyPosture: () => 'group', + }); + }); + + it('without targeting, a group position matches NOBODY — the gap D9 closes', async () => { + // The `cfo` position exists, but in the group org; the request is Plant A's. + // Pre-D9 this was the only possible outcome, and it was silent. + const req = await svc.openNodeRequest(openInput([{ type: 'position', value: 'cfo' }]), CTX); + expect(req.pending_approvers).toEqual(['position:cfo']); // the dead literal slot + }); + + it('`organization: $root` resolves the CFO against the GROUP directory', async () => { + const req = await svc.openNodeRequest( + openInput([{ type: 'position', value: 'cfo', organization: '$root' }]), + CTX, + ); + expect(req.pending_approvers).toEqual(['u_cfo']); + }); + + it('the request and its inbox rows still belong to the REQUEST org — only the lookup moved', async () => { + await svc.openNodeRequest( + openInput([{ type: 'position', value: 'cfo', organization: '$root' }]), + CTX, + ); + // This is the whole point of the split: targeting must not relocate the + // request into the approver's organization, or the plant would lose its own + // audit trail to the group. + expect(engine._tables['sys_approval_request'][0].organization_id).toBe('o_plant'); + expect(engine._tables['sys_approval_approver'][0].organization_id).toBe('o_plant'); + expect(engine._tables['sys_approval_action'][0].organization_id).toBe('o_plant'); + }); + + it('mixes a plant approver and a group approver in ONE node — why targeting is per-approver', async () => { + // A node-level declaration could not express this: the plant manager and the + // group CFO sign off in parallel, each resolved in a different directory. + const req = await svc.openNodeRequest( + openInput([ + { type: 'position', value: 'plant_manager', group: 'plant' }, + { type: 'position', value: 'cfo', organization: '$root', group: 'finance' }, + ]), + CTX, + ); + expect(new Set(req.pending_approvers)).toEqual(new Set(['u_plant_mgr', 'u_cfo'])); + }); + + it('drops a targeted approver who could not READ the request, leaving the empty-slate path', async () => { + // Same flow, but the CFO holds no membership in Plant A — D2's union wall + // would hide the request from her. Better an empty slate (which the node has + // an `onEmptyApprovers` policy for) than a task she cannot open. + engine._tables['sys_member'] = engine._tables['sys_member'].filter((m) => m.user_id !== 'u_cfo'); + const req = await svc.openNodeRequest( + openInput([{ type: 'position', value: 'cfo', organization: '$root' }]), + CTX, + ); + expect(req.pending_approvers).toEqual(['position:cfo']); + }); + + it('refuses a target outside the group — loudly, and no request is created', async () => { + engine._tables['sys_organization'].push({ id: 'o_rival', slug: 'rival-co', parent_organization_id: null }); + await expect( + svc.openNodeRequest(openInput([{ type: 'position', value: 'cfo', organization: 'rival-co' }]), CTX), + ).rejects.toThrow(/VALIDATION_FAILED.*not in the same group/); + expect(engine._tables['sys_approval_request'] ?? []).toHaveLength(0); + }); + + it('refuses under a non-group posture — a posture migration cannot silently reroute', async () => { + const isolated = new ApprovalService({ + engine: engine as any, + clock: { now: () => new Date(1767000000000) }, + tenancyPosture: () => 'isolated', + }); + await expect( + isolated.openNodeRequest(openInput([{ type: 'position', value: 'cfo', organization: '$root' }]), CTX), + ).rejects.toThrow(/VALIDATION_FAILED.*'group' tenancy posture/); + }); +}); diff --git a/packages/plugins/plugin-approvals/src/approver-org-scope.test.ts b/packages/plugins/plugin-approvals/src/approver-org-scope.test.ts new file mode 100644 index 0000000000..8de069a60b --- /dev/null +++ b/packages/plugins/plugin-approvals/src/approver-org-scope.test.ts @@ -0,0 +1,201 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [ADR-0105 D9] Cross-organization approver targeting. + * + * The property that matters is that this is a route WITHIN one group, not a + * channel to an arbitrary tenant — so every test here is about a boundary + * holding or a failure being LOUD. The thing D9 exists to prevent is silence: + * an approval that reads as "escalate to group" but quietly resolves inside the + * plant, or one that routes to someone the D2 wall then hides the request from. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { + filterApproversWhoCanRead, + resolveApproverDirectoryOrg, + type ApproverOrgScopeDeps, +} from './approver-org-scope.js'; + +/** + * A three-tier group: group → division → plant, plus a shared-services SIBLING + * of the plant, plus an organization in a DIFFERENT group entirely. + */ +const ORGS: Record = { + o_group: { id: 'o_group', slug: 'acme-group', parent_organization_id: null }, + o_div: { id: 'o_div', slug: 'acme-north', parent_organization_id: 'o_group' }, + o_plant: { id: 'o_plant', slug: 'acme-plant-a', parent_organization_id: 'o_div' }, + o_ssc: { id: 'o_ssc', slug: 'acme-ssc', parent_organization_id: 'o_group' }, + o_other: { id: 'o_other', slug: 'rival-co', parent_organization_id: null }, + o_lone: { id: 'o_lone', slug: 'lone-co', parent_organization_id: null }, +}; + +function makeDeps(over: Partial & { members?: Array<{ user_id: string; organization_id: string }> } = {}): ApproverOrgScopeDeps & { warn: any } { + const warn = vi.fn(); + const members = over.members ?? []; + const engine = { + find: vi.fn(async (object: string, opts: any) => { + const f = opts?.filter ?? {}; + if (object === 'sys_organization') { + const rows = Object.values(ORGS).filter((o) => + (f.id === undefined || o.id === f.id) && (f.slug === undefined || o.slug === f.slug)); + return rows; + } + if (object === 'sys_member') { + const wanted: string[] = f.user_id?.$in ?? []; + return members.filter((m) => m.organization_id === f.organization_id && wanted.includes(m.user_id)); + } + return []; + }), + }; + return { engine, logger: { warn }, posture: () => 'group', ...over, warn } as any; +} + +const resolve = ( + deps: ApproverOrgScopeDeps, + declaration: string | undefined, + requestOrg: string | null, + type = 'position', + orgScoped = true, +) => resolveApproverDirectoryOrg(deps, declaration, requestOrg, type, orgScoped); + +describe('resolveApproverDirectoryOrg — the default path is untouched', () => { + it('returns the request org and reads NOTHING when no organization is declared', async () => { + const deps = makeDeps(); + await expect(resolve(deps, undefined, 'o_plant')).resolves.toBe('o_plant'); + // The overwhelmingly common case must not cost a query. + expect((deps.engine.find as any)).not.toHaveBeenCalled(); + }); + + it('passes a null request org straight through — a single-org deployment is unaffected', async () => { + const deps = makeDeps(); + await expect(resolve(deps, undefined, null)).resolves.toBeNull(); + }); +}); + +describe('resolveApproverDirectoryOrg — symbols resolve against the D6 tree', () => { + it('$root climbs to the group organization, not just one level', async () => { + // o_plant → o_div → o_group. A one-level implementation would answer o_div. + await expect(resolve(makeDeps(), '$root', 'o_plant')).resolves.toBe('o_group'); + }); + + it('$parent stops at exactly one level — division sign-off in a three-tier group', async () => { + await expect(resolve(makeDeps(), '$parent', 'o_plant')).resolves.toBe('o_div'); + }); + + it('$root on an organization with no lineage FAILS instead of silently self-targeting', async () => { + // Returning o_lone would make "escalate to group" mean "approve in place" — + // the exact silent misrouting D9 exists to prevent. + await expect(resolve(makeDeps(), '$root', 'o_lone')).rejects.toThrow( + /VALIDATION_FAILED.*no 'parent_organization_id' lineage/, + ); + }); + + it('$parent at the root of a group fails rather than resolving to nothing', async () => { + await expect(resolve(makeDeps(), '$parent', 'o_group')).rejects.toThrow(/VALIDATION_FAILED.*no parent/); + }); +}); + +describe('resolveApproverDirectoryOrg — a slug names one organization, bounded by the group', () => { + it('accepts a SIBLING in the same group — the shared-services-centre shape', async () => { + // o_ssc is not an ancestor of o_plant; it shares the root. This is why the + // rule is "shares a root", not "is an ancestor". + await expect(resolve(makeDeps(), 'acme-ssc', 'o_plant')).resolves.toBe('o_ssc'); + }); + + it('refuses an organization in a DIFFERENT group — this is not a channel to any tenant', async () => { + await expect(resolve(makeDeps(), 'rival-co', 'o_plant')).rejects.toThrow( + /VALIDATION_FAILED.*not in the same group/, + ); + }); + + it('refuses an unknown slug and says an id is not accepted', async () => { + // The most likely authoring mistake is pasting an organization id, which is + // per-deployment and would make the flow unportable. + await expect(resolve(makeDeps(), 'o_plant', 'o_plant')).rejects.toThrow( + /VALIDATION_FAILED.*never an id/, + ); + }); +}); + +describe('resolveApproverDirectoryOrg — the guards', () => { + it('refuses under a non-group posture instead of silently ignoring the declaration', async () => { + // A deployment migrating group → isolated must not quietly reroute its + // approvals; that is an audit event, not a config detail. + const deps = makeDeps({ posture: () => 'isolated' }); + await expect(resolve(deps, '$root', 'o_plant')).rejects.toThrow( + /VALIDATION_FAILED.*requires the 'group' tenancy posture.*isolated/, + ); + }); + + it('stands down when the posture is unknown — a minimal stack is not broken by a guard it cannot answer', async () => { + const deps = makeDeps({ posture: () => undefined }); + await expect(resolve(deps, '$root', 'o_plant')).resolves.toBe('o_group'); + }); + + it('refuses the declaration on an approver type that has no organization directory', async () => { + // `user` names a person outright; an `organization` on it would have no + // effect. An author who wrote it believed it did something. + await expect(resolve(makeDeps(), '$root', 'o_plant', 'user', false)).rejects.toThrow( + /VALIDATION_FAILED.*would have no effect/, + ); + }); + + it('refuses when the request carries no organization at all', async () => { + await expect(resolve(makeDeps(), '$root', null)).rejects.toThrow( + /VALIDATION_FAILED.*carries no organization/, + ); + }); + + it('survives a cycle in the grouping metadata instead of looping forever', async () => { + const deps = makeDeps(); + (deps.engine.find as any) = vi.fn(async (object: string, opts: any) => { + if (object !== 'sys_organization') return []; + const id = opts?.filter?.id; + // a → b → a + if (id === 'a') return [{ id: 'a', slug: 'a', parent_organization_id: 'b' }]; + if (id === 'b') return [{ id: 'b', slug: 'b', parent_organization_id: 'a' }]; + return []; + }); + // Terminates; the cycle is broken and whatever it settles on is bounded. + await expect(resolve(deps, '$parent', 'a')).resolves.toBe('b'); + }); +}); + +describe('filterApproversWhoCanRead — routing to someone the wall hides is a silent failure', () => { + it('keeps approvers who hold a membership in the REQUEST org', async () => { + const deps = makeDeps({ members: [{ user_id: 'u_cfo', organization_id: 'o_plant' }] }); + const kept = await filterApproversWhoCanRead(deps, ['u_cfo'], 'o_plant', { + approverType: 'position', value: 'cfo', directoryOrgId: 'o_group', + }); + expect(kept).toEqual(['u_cfo']); + expect(deps.warn).not.toHaveBeenCalled(); + }); + + it('drops an approver with no membership there, and says exactly why', async () => { + // She holds `cfo` in the group org but no membership in the plant, so D2's + // union would hide the request from her: routing succeeds, then she opens a + // task she cannot open. Convert that into the empty-slate path the node + // already has a policy for. + const deps = makeDeps({ members: [{ user_id: 'u_ok', organization_id: 'o_plant' }] }); + const kept = await filterApproversWhoCanRead(deps, ['u_ok', 'u_no_membership'], 'o_plant', { + approverType: 'position', value: 'cfo', directoryOrgId: 'o_group', + }); + expect(kept).toEqual(['u_ok']); + expect(deps.warn).toHaveBeenCalledTimes(1); + const msg = String(deps.warn.mock.calls[0][0]); + expect(msg).toMatch(/u_no_membership|cross-organization approver/); + expect(msg).toMatch(/Grant those users a membership|retarget/); + }); + + it('does NOT empty a live slate when membership is unreadable', async () => { + // An infrastructure hiccup must not silently unstaff a pending approval: + // routing to someone who may not see it is recoverable, emptying is not. + const deps = makeDeps(); + (deps.engine.find as any) = vi.fn(async () => { throw new Error('driver hiccup'); }); + const kept = await filterApproversWhoCanRead(deps, ['u1', 'u2'], 'o_plant', { + approverType: 'position', directoryOrgId: 'o_group', + }); + expect(kept).toEqual(['u1', 'u2']); + }); +}); diff --git a/packages/plugins/plugin-approvals/src/approver-org-scope.ts b/packages/plugins/plugin-approvals/src/approver-org-scope.ts new file mode 100644 index 0000000000..a55750460a --- /dev/null +++ b/packages/plugins/plugin-approvals/src/approver-org-scope.ts @@ -0,0 +1,261 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [ADR-0105 D9] Cross-organization approver targeting — resolving WHICH + * organization's directory an approver is looked up in. + * + * One organization id used to do three jobs at once in `openNodeRequest`: + * where the request row lives, where its inbox index rows live, and where its + * approvers are looked up. The first two are the request's own organization by + * definition. The third is not: a group CFO holds her `cfo` position in the + * GROUP organization while the purchase order she signs off lives in the PLANT + * organization. Binding all three together meant + * `expandPositionUsers('cfo', )` matched nobody and the slot fell into + * `onEmptyApprovers` — a group escalation could not be expressed at all. + * + * This module resolves only the third job. The request keeps living in its own + * organization, and D2's membership union is what lets a group-side approver + * READ it — see `assertApproversCanRead` below for why that is a precondition + * worth checking rather than assuming. + */ + +/** Cycle guard for the `parent_organization_id` walk (mirrors the BU subtree walk). */ +const MAX_ORG_DEPTH = 32; + +const SYSTEM_CTX = { isSystem: true } as const; + +export interface ApproverOrgScopeEngine { + find(object: string, options: any): Promise; +} + +export interface ApproverOrgScopeDeps { + engine: ApproverOrgScopeEngine; + /** + * The tenancy posture in force, when the host could resolve one. `undefined` + * means "unknown" — a stack booted without the tenancy service — and is + * treated as permissive so a minimal test/embedded stack is not broken by a + * guard it cannot answer. + */ + posture?: () => string | undefined; + logger?: { warn?: (msg: any, ...rest: any[]) => void; debug?: (msg: any, ...rest: any[]) => void }; +} + +/** + * Every failure here THROWS `VALIDATION_FAILED`, matching how `expression` + * approvers already fail (#3447 P2): an approver declaration that cannot be + * resolved is a ROUTING BUG, never "condition not met". Silently falling back + * to the request's own organization would be the worst option available — a + * flow that reads as "escalate to group" would quietly approve inside the + * plant, and the deployment that changed posture would reroute its approvals + * with no signal at all. + * + * Messages name the offending value because their primary reader is the AI + * author fixing the flow on the next validate pass. + */ +function fail(message: string): never { + throw new Error(`VALIDATION_FAILED: ${message}`); +} + +async function findOrg( + engine: ApproverOrgScopeEngine, + where: Record, +): Promise { + try { + const rows = await engine.find('sys_organization', { + filter: where, + fields: ['id', 'slug', 'parent_organization_id'], + limit: 1, + context: SYSTEM_CTX, + } as any); + return Array.isArray(rows) && rows[0] ? rows[0] : null; + } catch { + return null; + } +} + +/** The id chain from `orgId` up to its root, inclusive. Fail-closed on cycles. */ +async function ancestorChain( + engine: ApproverOrgScopeEngine, + orgId: string, +): Promise { + const chain: string[] = []; + const seen = new Set(); + let cursor: string | null = orgId; + for (let depth = 0; cursor && depth < MAX_ORG_DEPTH; depth++) { + if (seen.has(cursor)) break; // cycle in the grouping metadata — stop, do not loop + seen.add(cursor); + chain.push(cursor); + const row = await findOrg(engine, { id: cursor }); + const parent = row?.parent_organization_id; + cursor = parent ? String(parent) : null; + } + return chain; +} + +/** + * Resolve an approver's `organization` declaration to a concrete organization + * id for directory lookups. Returns the request's own organization when the + * declaration is absent — the unchanged default path, which does no reads. + */ +export async function resolveApproverDirectoryOrg( + deps: ApproverOrgScopeDeps, + declaration: string | null | undefined, + requestOrgId: string | null | undefined, + approverType: string, + isOrgScopedType: boolean, +): Promise { + const declared = typeof declaration === 'string' ? declaration.trim() : ''; + if (!declared) return requestOrgId; + + // An `organization` on `user` / `field` / `manager` / `team` is not a + // narrower routing — those types never consult an org-scoped directory, so + // the declaration would have no effect. Refuse rather than ignore: a flow + // author who wrote it believed it did something. + if (!isOrgScopedType) { + fail( + `approver type '${approverType}' resolves people without an organization directory, ` + + `so 'organization: ${declared}' would have no effect — remove it ` + + `(ADR-0105 D9 applies to position / org_membership_level / department / expression)`, + ); + } + + // Posture guard. ADR-0105 D9 applies to `group` only: in `isolated`, cross-org + // approval remains system-context mirroring (cloud #2937 contract), and in + // `single` there is no second organization to target. Refusing here — rather + // than in a lint — is deliberate: posture is ENVIRONMENT configuration, so the + // same portable flow metadata may be deployed into any posture and no static + // check can see which. A deployment migrating group → isolated must fail + // loudly instead of silently rerouting its approvals. + const posture = deps.posture?.(); + if (posture && posture !== 'group') { + fail( + `cross-organization approver targeting ('organization: ${declared}') requires the ` + + `'group' tenancy posture; this deployment resolves '${posture}' (ADR-0105 D9)`, + ); + } + + const requestOrg = requestOrgId ? String(requestOrgId) : ''; + if (!requestOrg) { + fail( + `'organization: ${declared}' cannot be resolved for a request that carries no ` + + `organization — cross-organization targeting needs an organization to resolve from`, + ); + } + + const chain = await ancestorChain(deps.engine, requestOrg); + + if (declared === '$root') { + const root = chain[chain.length - 1]; + if (root === requestOrg && chain.length === 1) { + fail( + `'organization: $root' resolved to the request's own organization — this organization ` + + `has no 'parent_organization_id' lineage. Declare the group hierarchy (ADR-0105 D6) ` + + `or drop the targeting`, + ); + } + return root; + } + + if (declared === '$parent') { + const parent = chain[1]; + if (!parent) { + fail( + `'organization: $parent' has no parent to resolve to — the request's organization is ` + + `already the root of its group (ADR-0105 D6 'parent_organization_id')`, + ); + } + return parent; + } + + // A slug names one specific organization — the shape the symbols cannot + // express (a SIBLING, e.g. a shared-services centre approving payables for + // every plant). Slugs rather than ids because flow metadata is portable + // across environments and organization ids are minted per deployment. + const target = await findOrg(deps.engine, { slug: declared }); + if (!target?.id) { + fail( + `no organization with slug '${declared}' — 'organization' takes an organization SLUG ` + + `or a symbol ($root / $parent), never an id (ids are per-deployment, flows are portable)`, + ); + } + const targetId = String(target.id); + if (targetId === requestOrg) return targetId; + + // Legality: the target must share a grouping root with the request's + // organization. "Shares a root" rather than "is an ancestor" because the + // sibling case is a first-class use. The rule depends only on the + // organization tree — never on who submitted — so one flow routes identically + // for every submitter, which is what makes a routing bug reproducible. + const targetChain = await ancestorChain(deps.engine, targetId); + const targetRoot = targetChain[targetChain.length - 1]; + const requestRoot = chain[chain.length - 1]; + if (!targetRoot || !requestRoot || targetRoot !== requestRoot) { + fail( + `organization '${declared}' is not in the same group as the request's organization ` + + `— cross-organization approval routes WITHIN one group (they must share a ` + + `'parent_organization_id' root, ADR-0105 D6/D9)`, + ); + } + return targetId; +} + +/** + * Drop approvers who could not READ the request they were routed to, and say + * so. + * + * ADR-0105 D9 notes that reads by cross-org approvers "are covered by D2 + * (membership union)". True — but D2's union is `organization_id IN + * accessible_org_ids`, and the request row is stamped with the REQUEST's + * organization. So a group-side approver reaches it only if she also holds a + * membership there. That is the intended group shape (group staff are members + * of every plant while holding their positions in the group org — the shape the + * ADR-0105 acceptance dogfood already models), but nothing enforces it. + * + * Left unchecked the failure is SILENT and expensive: routing succeeds, the + * request and inbox rows are written, and the approver then opens a task she + * cannot open — the wall hides the record. Filtering here converts that into + * the empty-slate path the node already has a policy for + * (`onEmptyApprovers`), and logs exactly who was dropped and why, so the fix + * ("grant the membership" or "retarget") is legible without a debugger. + */ +export async function filterApproversWhoCanRead( + deps: ApproverOrgScopeDeps, + userIds: string[], + requestOrgId: string | null | undefined, + context: { approverType: string; value?: string; directoryOrgId?: string | null }, +): Promise { + const requestOrg = requestOrgId ? String(requestOrgId) : ''; + if (!requestOrg || userIds.length === 0) return userIds; + + let members: any[] = []; + try { + members = await deps.engine.find('sys_member', { + filter: { organization_id: requestOrg, user_id: { $in: userIds } }, + fields: ['user_id'], + limit: 10000, + context: SYSTEM_CTX, + } as any); + } catch { + // Membership unreadable — do NOT drop everyone on an infrastructure + // hiccup. Routing to someone who may not see the request is recoverable + // (an admin can grant the membership); silently emptying a live approval + // slate is not. + return userIds; + } + + const canRead = new Set( + (members ?? []).map((m: any) => String(m?.user_id ?? '')).filter(Boolean), + ); + const dropped = userIds.filter((u) => !canRead.has(u)); + if (dropped.length === 0) return userIds; + + deps.logger?.warn?.( + `[approvals] ADR-0105 D9: ${dropped.length} cross-organization approver(s) dropped — ` + + `they hold '${context.value ?? context.approverType}' in organization ` + + `'${context.directoryOrgId}' but no membership in the request's organization ` + + `'${requestOrg}', so the D2 union wall would hide the request from them. ` + + `Grant those users a membership in the request's organization, or retarget the approver.`, + { dropped, requestOrganizationId: requestOrg, directoryOrganizationId: context.directoryOrgId }, + ); + return userIds.filter((u) => canRead.has(u)); +} diff --git a/packages/spec/api-surface.json b/packages/spec/api-surface.json index 9b52d9d77d..c1be06cc53 100644 --- a/packages/spec/api-surface.json +++ b/packages/spec/api-surface.json @@ -1985,6 +1985,8 @@ "APPROVAL_BRANCH_LABELS (const)", "APPROVAL_NODE_TYPE (const)", "APPROVER_EXPRESSION_ROOTS (const)", + "APPROVER_ORG_SCOPED (const)", + "APPROVER_ORG_SYMBOLS (const)", "APPROVER_VALUE_BINDINGS (const)", "APPROVER_VALUE_SOURCES (const)", "ActionCategory (type)", @@ -2003,6 +2005,7 @@ "ApprovalNodeApproverSchema (const)", "ApprovalNodeConfig (type)", "ApprovalNodeConfigSchema (const)", + "ApproverOrgSymbol (type)", "ApproverType (const)", "ApproverValueBinding (type)", "AuthField (type)", @@ -2176,6 +2179,7 @@ "WebhookSchema (const)", "WebhookTriggerType (const)", "analyzeRegion (function)", + "approverTypeIsOrgScoped (function)", "canonicalApproverType (function)", "defineActionDescriptor (function)", "defineFlow (function)", diff --git a/packages/spec/src/automation/approval.zod.ts b/packages/spec/src/automation/approval.zod.ts index 7008ef17f5..9e8730a0d0 100644 --- a/packages/spec/src/automation/approval.zod.ts +++ b/packages/spec/src/automation/approval.zod.ts @@ -204,6 +204,67 @@ export const APPROVER_VALUE_SOURCES = Object.fromEntries( | { source: 'auto' | 'trigger-field' | 'expression' | 'unsupported' } >; +// ========================================================================== +// [ADR-0105 D9] Cross-organization approver targeting +// ========================================================================== + +/** + * Symbolic organization references usable as an approver's `organization` + * (ADR-0105 D9). + * + * Flow metadata is PORTABLE — the same flow deploys to dev, staging and every + * customer environment — while an organization id is DATA, minted per + * deployment. A literal id in a flow is therefore unportable by construction, + * and an AI author cannot know one at authoring time. These symbols express + * the two common intents against the D6 grouping tree instead, so the usual + * cases need no deployment knowledge at all: + * + * - `$root` — the group organization: walk `parent_organization_id` up from + * the request's organization to the top. "Escalate to group." + * - `$parent` — exactly one level up. Division-level sign-off in a three-tier + * group (group → division → plant). + * + * A slug (any other value) names a specific organization and covers the shapes + * the symbols cannot: a SIBLING org, e.g. a shared-services centre that + * approves payables for every plant. That is also why D9's legality rule is + * "shares a root", not "is an ancestor". + */ +export const APPROVER_ORG_SYMBOLS = ['$root', '$parent'] as const; +export type ApproverOrgSymbol = (typeof APPROVER_ORG_SYMBOLS)[number]; + +/** + * Whether an approver type resolves people through an ORGANIZATION-SCOPED + * directory — i.e. whether `organization` (ADR-0105 D9) means anything for it. + * + * `user` / `field` / `manager` name a person directly or derive one from the + * record, so they are org-agnostic: an `organization` on them is not a narrower + * routing, it is a misunderstanding, and the lint says so rather than silently + * ignoring it. `team` is org-agnostic too — `sys_team_member` carries no + * organization column and the engine never scoped it (unlike position / + * membership-tier / department, which all do). + * + * `satisfies` keeps this exhaustive for the same reason + * {@link APPROVER_VALUE_BINDINGS} is: a new {@link ApproverType} must state + * whether cross-org targeting applies to it, or this file fails to compile. + */ +export const APPROVER_ORG_SCOPED = { + user: false, + org_membership_level: true, + role: true, + position: true, + team: false, + department: true, + manager: false, + field: false, + queue: false, + expression: true, +} as const satisfies Record, boolean>; + +/** Does `organization` (ADR-0105 D9) apply to this approver type? */ +export function approverTypeIsOrgScoped(type: string): boolean { + return (APPROVER_ORG_SCOPED as Record)[canonicalApproverType(type)] ?? false; +} + // ========================================================================== // Approval as a Flow Node (ADR-0019, canonical) // ========================================================================== @@ -337,6 +398,41 @@ export const ApprovalNodeApproverSchema = lazySchema(() => z.object({ * (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")'), + /** + * [ADR-0105 D9] Which organization's directory this approver is resolved + * AGAINST. Default (omitted): the request's own organization — today's + * behaviour, unchanged. + * + * This separates two things one organization id used to do at once: WHERE + * THE REQUEST LIVES and WHERE ITS APPROVERS ARE LOOKED UP. A group CFO holds + * her `cfo` position in the GROUP organization while the purchase order + * lives in the PLANT organization; without this field + * `expandPositionUsers('cfo', )` finds nobody and the slot routes + * into `onEmptyApprovers`. + * + * Declared per APPROVER, not per node, so one node can require a plant + * manager AND a group CFO in parallel (`behavior: 'per_group'`). A node-level + * default is a strict special case of this and can be added later as sugar; + * the reverse is not true. + * + * Values: `$root` / `$parent` (see {@link APPROVER_ORG_SYMBOLS}) or an + * organization SLUG. Slugs are used rather than ids because flow metadata is + * portable across environments and ids are not. + * + * Bounded, not free: the target must share a `parent_organization_id` root + * with the request's organization (ADR-0105 D6 grouping metadata), so this is + * a route WITHIN one group — never a channel to an unrelated tenant. Applies + * only to org-scoped approver types ({@link APPROVER_ORG_SCOPED}) and only + * under the `group` tenancy posture; elsewhere the runtime refuses rather + * than silently ignoring, so a posture migration cannot quietly reroute + * approvals. + */ + organization: z.string().optional().meta({ + description: + 'ADR-0105 D9 — organization whose directory resolves this approver: `$root` (group org), ' + + '`$parent` (one level up), or an organization slug. Omitted = the request\'s own organization.', + xRef: { kind: 'organization', symbols: [...APPROVER_ORG_SYMBOLS] }, + }), })); export type ApprovalNodeApprover = z.infer; From ce10d238270290388f6dba0ccba062d0a96b5ae6 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 28 Jul 2026 07:47:06 +0000 Subject: [PATCH 2/2] docs(approvals): document cross-organization approver targeting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The drift check flagged `automation/approvals.mdx` — correctly. D9 adds an authoring capability, and the hand-written guide is where an author (human or AI) actually looks; a feature reachable only from the generated schema reference is a feature most authors never find. Adds a section covering what the reference table cannot: which value to reach for ($root over a slug, because flow metadata is portable and organization ids are not), and the three things the field deliberately does NOT do — it does not move the request, it is not a route to an arbitrary tenant, and it does not apply to approver kinds that consult no organization directory. Plus the read-visibility precondition, which is the one way a correctly-authored cross-org approval still ends up empty. Also points the approver-kinds paragraph at the new section, so the fact that only some kinds accept `organization` is discoverable from where the kinds are listed rather than only from the lint that rejects it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015FebXPaaGrLhGKw1LHPbpL --- content/docs/automation/approvals.mdx | 67 ++++++++++++++++++++++++++- 1 file changed, 66 insertions(+), 1 deletion(-) diff --git a/content/docs/automation/approvals.mdx b/content/docs/automation/approvals.mdx index 4d09b3aa09..b52746d5e1 100644 --- a/content/docs/automation/approvals.mdx +++ b/content/docs/automation/approvals.mdx @@ -173,6 +173,68 @@ submit: with `behavior: 'per_group'`, each intermediate value (e.g. each returned department) forms its own sign-off group. +## Approving across organizations (ADR-0105 D9) [#cross-org-approvers] + + +Requires the `group` tenancy posture. Under `single` or `isolated` the runtime +**refuses** an approver that declares `organization`, rather than ignoring it. + + +A group-shaped deployment routinely needs a plant's document signed off by +someone at the group: the purchase order lives in the **plant** organization, +but the CFO holds her `cfo` position in the **group** organization. By default +an approver is resolved against the request's own organization, so `cfo` would +match nobody there. + +Declare which organization's directory resolves that approver: + +```typescript +config: { + approvers: [ + { type: 'position', value: 'plant_manager', group: 'plant' }, + { type: 'position', value: 'cfo', organization: '$root', group: 'finance' }, + ], + behavior: 'per_group', // one plant sign-off AND one group sign-off +} +``` + +`organization` takes a **symbol** or an organization **slug**: + +| Value | Resolves to | +| :--- | :--- | +| `$root` | the group organization — climbs `parent_organization_id` to the top | +| `$parent` | exactly one level up (division sign-off in a three-tier group) | +| a slug, e.g. `acme-ssc` | that organization — for a **sibling**, such as a shared-services centre approving payables for every plant | + +Prefer the symbols. Flow metadata is portable across environments while +organization ids are minted per deployment, so `$root` says "the group" without +naming anything deployment-specific. An organization **id** is rejected: pass +the slug. + +Three things this deliberately does **not** do: + +- **It does not move the request.** Only the approver lookup changes — the + request, its audit trail, and its inbox rows stay in the plant's + organization. +- **It is not a route to any tenant.** The target must share a + `parent_organization_id` root with the request's organization, so approval + routes *within one group*. The rule reads only the organization tree, never + the submitter, so a flow routes identically for everyone. +- **It does not apply to every approver kind.** `user`, `field`, `manager` and + `team` name people without consulting an organization directory; + `organization` on those is refused at runtime and flagged by `os lint` + (`approval-approver-cross-org-unsupported`). + + +A cross-organization approver must also be able to **read** the request. The +request stays in the plant's organization, so a group-side approver reaches it +only if she also holds a membership there — the usual group shape is that group +staff are members of every plant while holding their positions at the group. +Approvers without that membership are dropped from the slate with a warning +naming them, and the node's `onEmptyApprovers` policy takes over — better an +empty slate you can see than a task the tenancy wall hides. + + A result may legitimately be **empty** (a present-but-empty field or variable); the node-level `onEmptyApprovers` policy decides what that means — `admin_rescue` (default: the request opens, a privileged admin takes over via @@ -230,7 +292,10 @@ hierarchy. `queue` still parses so stored flows keep loading, but it is **not implemented** by the runtime and is no longer offered for authoring (#3508) — a queue entry resolves to nobody. Route to a `team`, `department`, or `position` instead. `field`, `manager`, and `expression` resolve against the record's -**live** state at node entry (#3447). An entry that resolves +**live** state at node entry (#3447). `position`, `department`, +`org_membership_level` and `expression` may additionally name which +organization's directory resolves them — see +[Approving across organizations](#cross-org-approvers). An entry that resolves to nobody is not an error: the request opens with an empty `pending_approvers` and nothing can move it, so the run parks forever.