From af5df84dab3a5297e9b617e5b3256f9f13230484 Mon Sep 17 00:00:00 2001 From: os-warren Date: Thu, 20 Aug 2026 14:10:44 +0000 Subject: [PATCH 1/5] test(approvals): measurement harness for the #10153 manager org screen (no fix) Pins the CURRENT behaviour so the premise and the tiering question are reproducible: the manager branch resolves across the organization boundary, the sibling position expansion is screened (and is not reject-everything), team is not screened either, and a sole cross-org manager approver under onEmptyApprovers: 'fail' opens today while a screened type in the identical shape throws NO_APPROVERS. No fix is implemented. The card tripped its re-tiering wire (Clause-2) and is handed back for dispatch at the required tier. Part of #10153 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx --- ...anager-approver-org-screen.premise.test.ts | 145 ++++++++++++++++++ 1 file changed, 145 insertions(+) create mode 100644 packages/plugins/plugin-approvals/src/manager-approver-org-screen.premise.test.ts diff --git a/packages/plugins/plugin-approvals/src/manager-approver-org-screen.premise.test.ts b/packages/plugins/plugin-approvals/src/manager-approver-org-screen.premise.test.ts new file mode 100644 index 0000000000..2e18542832 --- /dev/null +++ b/packages/plugins/plugin-approvals/src/manager-approver-org-screen.premise.test.ts @@ -0,0 +1,145 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +/** + * MEASUREMENT HARNESS for #10153 — it pins the CURRENT (defective) behaviour on + * purpose, so the premise and the tiering question are reproducible rather than + * argued. It is NOT the fix and it is NOT on a pull request. + * + * ⛔ Whoever implements the org screen must INVERT `PREMISE A` and `CLAUSE-2 (a)` + * — they assert today's cross-org resolution, which is exactly what the fix + * removes. `PREMISE B` / `B2` / `CLAUSE-2 (b)` describe the sibling treatment + * and stay as they are. + * + * What it measures, all on one tree: + * A — `manager` resolves a manager whose only `sys_member` row is in + * another organization, into this organization's approver slate. + * B/B2 — the sibling `position` expansion IS screened, and the screen is not + * reject-everything (a same-org holder still resolves). + * W — `team` is NOT screened either, which is why #10153's warrant + * ("every sibling expansion is org-scoped") does not hold as stated. + * C-a/b — the tiering question: today a sole cross-org `manager` approver + * under `onEmptyApprovers: 'fail'` OPENS the request; a screened type + * in the identical shape THROWS `NO_APPROVERS`. Applying the screen + * therefore moves that input from accepted to refused. + */ +import { describe, it, expect, beforeEach } from 'vitest'; +import { ApprovalService } from './approval-service.js'; + +function makeFakeEngine() { + const tables: Record = {}; + const ensure = (n: string) => (tables[n] ??= []); + function matches(row: any, 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(s => matches(row, s))) return false; continue; } + if (k === '$and') { if (!(v as any[]).every(s => matches(row, s))) 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) { + const rows = ensure(object).filter(r => matches(r, options?.filter ?? options?.where)); + return rows.slice(0, options?.limit ?? 1000); + }, + async insert(object: string, data: any) { ensure(object).push({ ...data }); return { ...data }; }, + async update(object: string, idOrData: any, _opts?: any) { + const data = typeof idOrData === 'object' ? idOrData : _opts; + const id = typeof idOrData === 'object' ? idOrData.id : idOrData; + const t = ensure(object); const i = t.findIndex(r => r.id === id); + if (i >= 0) t[i] = { ...t[i], ...data }; + return t[i]; + }, + async delete() { return {}; }, + registerHook() {}, unregisterHooksByPackage() { return 0; }, async fire() {}, + }; +} + +const ORG_A = 'org_a'; +const CTX_A = { userId: 'u_sub', organizationId: ORG_A, positions: [], permissions: [] } as any; + +function input(approvers: any[], configExtra: Record = {}) { + return { + object: 'opportunity', recordId: 'opp1', runId: 'run_1', nodeId: 'approve_step', + flowName: 'deal_approval', + config: { approvers, behavior: 'first_response' as const, lockRecord: false, ...configExtra }, + record: { id: 'opp1', owner_id: 'u_sub', amount: 100 }, + }; +} + +describe('#10153 probe', () => { + let engine: ReturnType; + let svc: ApprovalService; + let n = 0; + + beforeEach(() => { + engine = makeFakeEngine(); + n = 0; + svc = new ApprovalService({ + engine: engine as any, + clock: { now: () => new Date(new Date('2026-01-15T10:00:00Z').getTime() + (n++) * 1000) }, + }); + // Directory: submitter in org_a; his manager is a member of org_b ONLY. + engine._tables['sys_user'] = [ + { id: 'u_sub', manager_id: 'u_mgr_b' }, + { id: 'u_mgr_b', manager_id: null }, + ]; + engine._tables['sys_member'] = [ + { id: 'm1', user_id: 'u_sub', organization_id: ORG_A, role: 'member' }, + { id: 'm2', user_id: 'u_mgr_b', organization_id: 'org_b', role: 'member' }, + ]; + // Sibling directory: the only `cfo` holder is in org_b. + engine._tables['sys_user_position'] = [ + { id: 'p1', user_id: 'u_pos_b', position: 'cfo', organization_id: 'org_b' }, + ]; + }); + + it('PREMISE A — `manager` resolves ACROSS the org boundary (unscreened)', async () => { + const req = await svc.openNodeRequest(input([{ type: 'manager' }]), CTX_A); + console.log('[PROBE A] request org =', req.organization_id, 'pending_approvers =', JSON.stringify(req.pending_approvers)); + expect(req.pending_approvers).toEqual(['u_mgr_b']); + }); + + it('PREMISE B — sibling `position` IS screened to the request org (same tree)', async () => { + const req = await svc.openNodeRequest(input([{ type: 'position', value: 'cfo' }]), CTX_A); + console.log('[PROBE B] pending_approvers =', JSON.stringify(req.pending_approvers)); + expect(req.pending_approvers).toEqual(['position:cfo']); + }); + + it('PREMISE B2 — same-org `position` holder DOES resolve (screen is not reject-everything)', async () => { + engine._tables['sys_user_position'].push({ id: 'p2', user_id: 'u_pos_a', position: 'cfo', organization_id: ORG_A }); + const req = await svc.openNodeRequest(input([{ type: 'position', value: 'cfo' }]), CTX_A); + console.log('[PROBE B2] pending_approvers =', JSON.stringify(req.pending_approvers)); + expect(req.pending_approvers).toEqual(['u_pos_a']); + }); + + it('CLAUSE-2 (a) — TODAY: sole cross-org `manager` + onEmptyApprovers:fail SUCCEEDS', async () => { + const req = await svc.openNodeRequest(input([{ type: 'manager' }], { onEmptyApprovers: 'fail' }), CTX_A); + console.log('[PROBE C-a] opened OK, status =', req.status, 'approvers =', JSON.stringify(req.pending_approvers)); + expect(req.status).toBe('pending'); + }); + + it('CLAUSE-2 (b) — a SCREENED sibling in the same shape THROWS NO_APPROVERS', async () => { + let err: any = null; + try { + await svc.openNodeRequest(input([{ type: 'position', value: 'cfo' }], { onEmptyApprovers: 'fail' }), CTX_A); + } catch (e) { err = e; } + console.log('[PROBE C-b] threw =', err ? String(err.message).slice(0, 90) : 'NOTHING'); + expect(err).toBeTruthy(); + }); + + it('WARRANT — sibling `team` is NOT org-screened either (cross-org team resolves)', async () => { + engine._tables['sys_team'] = [{ id: 'team_b', name: 'B team', organization_id: 'org_b' }]; + engine._tables['sys_team_member'] = [{ id: 'tm1', team_id: 'team_b', user_id: 'u_team_b' }]; + const req = await svc.openNodeRequest(input([{ type: 'team', value: 'team_b' }]), CTX_A); + console.log('[PROBE W] org_a request, org_b team -> pending_approvers =', JSON.stringify(req.pending_approvers)); + expect(req.pending_approvers).toEqual(['u_team_b']); + }); +}); From 13c7ac54b56efcd2b51034bc66f2868a375ef162 Mon Sep 17 00:00:00 2001 From: os-warren Date: Thu, 20 Aug 2026 14:15:06 +0000 Subject: [PATCH 2/5] test(approvals): pin the #10153 harness fake engine to ObjectQL's write dispatch check:engine-double-contract flagged the harness double's delete()/update() as looser than the engine they stand in for. Route both through assertEngineDeleteDispatch / assertEngineUpdateDispatch and record the new pinned coverage in the retained ledger, as the gate's own remedy prescribes. Part of #10153 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx --- ...anager-approver-org-screen.premise.test.ts | 32 ++++++++++++++++--- scripts/engine-double-contract.pinned.json | 10 ++++++ 2 files changed, 37 insertions(+), 5 deletions(-) diff --git a/packages/plugins/plugin-approvals/src/manager-approver-org-screen.premise.test.ts b/packages/plugins/plugin-approvals/src/manager-approver-org-screen.premise.test.ts index 2e18542832..0f3de49810 100644 --- a/packages/plugins/plugin-approvals/src/manager-approver-org-screen.premise.test.ts +++ b/packages/plugins/plugin-approvals/src/manager-approver-org-screen.premise.test.ts @@ -22,6 +22,7 @@ * therefore moves that input from accepted to refused. */ import { describe, it, expect, beforeEach } from 'vitest'; +import { assertEngineDeleteDispatch, assertEngineUpdateDispatch } from '@objectstack/objectql'; import { ApprovalService } from './approval-service.js'; function makeFakeEngine() { @@ -50,14 +51,35 @@ function makeFakeEngine() { return rows.slice(0, options?.limit ?? 1000); }, async insert(object: string, data: any) { ensure(object).push({ ...data }); return { ...data }; }, - async update(object: string, idOrData: any, _opts?: any) { - const data = typeof idOrData === 'object' ? idOrData : _opts; - const id = typeof idOrData === 'object' ? idOrData.id : idOrData; - const t = ensure(object); const i = t.findIndex(r => r.id === id); + async update(object: string, data: any, options?: any) { + // Pinned to ObjectQL.update's OWN dispatch predicate — a double looser + // than the engine it stands in for turns a green suite into no suite. + const dispatch = assertEngineUpdateDispatch(data, options); + const t = ensure(object); + if (dispatch.kind === 'multi') { + let n = 0; + for (let i = 0; i < t.length; i++) { + if (matches(t[i], options?.where)) { t[i] = { ...t[i], ...data }; n++; } + } + return { updated: n }; + } + const i = t.findIndex(r => r.id === dispatch.id); if (i >= 0) t[i] = { ...t[i], ...data }; return t[i]; }, - async delete() { return {}; }, + async delete(object: string, options?: any) { + const dispatch = assertEngineDeleteDispatch(options); + const t = ensure(object); + if (dispatch.kind === 'multi') { + const survivors = t.filter(r => !matches(r, options?.where)); + const deleted = t.length - survivors.length; + t.splice(0, t.length, ...survivors); + return { deleted }; + } + const i = t.findIndex(r => r.id === dispatch.id); + if (i >= 0) t.splice(i, 1); + return { id: dispatch.id }; + }, registerHook() {}, unregisterHooksByPackage() { return 0; }, async fire() {}, }; } diff --git a/scripts/engine-double-contract.pinned.json b/scripts/engine-double-contract.pinned.json index 30f010c403..b0fed0376a 100644 --- a/scripts/engine-double-contract.pinned.json +++ b/scripts/engine-double-contract.pinned.json @@ -1016,6 +1016,16 @@ "verb": "delete", "pinned": 1 }, + { + "file": "packages/plugins/plugin-approvals/src/manager-approver-org-screen.premise.test.ts", + "verb": "delete", + "pinned": 1 + }, + { + "file": "packages/plugins/plugin-approvals/src/manager-approver-org-screen.premise.test.ts", + "verb": "update", + "pinned": 1 + }, { "file": "packages/plugins/plugin-approvals/src/record-reader-visibility.test.ts", "verb": "delete", From d0602162d0b7099af6862b17d7d9f61e98c7ca8e Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 20 Aug 2026 15:16:45 +0000 Subject: [PATCH 3/5] fix(approvals): screen the `manager` approver to the request's organization (#10153) `lookupManager` read `sys_user.manager_id` with no organization argument while every other graph-shaped approver expansion is handed the directory org. Since `sys_user` carries no `organization_id`, a `manager_id` crossing an organization boundary routed the approval to an out-of-tenant approver. The screen is a `sys_member` membership test, applied only when the fact is present and negative: a manager with membership rows, none in the request's organization, is dropped; absent membership rows, a failed read, or a request with no organization leave routing exactly as it was. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx --- .changeset/manager-approver-org-screen.md | 65 +++++++++++ .../plugin-approvals/src/approval-service.ts | 104 ++++++++++++++++- ...ts => manager-approver-org-screen.test.ts} | 105 +++++++++++++----- 3 files changed, 246 insertions(+), 28 deletions(-) create mode 100644 .changeset/manager-approver-org-screen.md rename packages/plugins/plugin-approvals/src/{manager-approver-org-screen.premise.test.ts => manager-approver-org-screen.test.ts} (54%) diff --git a/.changeset/manager-approver-org-screen.md b/.changeset/manager-approver-org-screen.md new file mode 100644 index 0000000000..26301d1362 --- /dev/null +++ b/.changeset/manager-approver-org-screen.md @@ -0,0 +1,65 @@ +--- +"@objectstack/plugin-approvals": minor +--- + +fix(approvals): screen the `manager` approver to the request's organization (#10153) + +`expandApprovers` hands the directory organization to every graph-shaped +approver expansion — `department`, `position`, `org_membership_level`. The +`manager` branch did not: `lookupManager` read `sys_user.manager_id` under a +system context and took no organization argument at all. `sys_user` is a global +identity table with no `organization_id`, so nothing else on that path supplied +the tenancy fact either. A `manager_id` crossing an organization boundary +therefore routed the submission to an approver **in another organization** — an +out-of-tenant person granted approval authority over the record. + +The same column has been screened on the hierarchy side since cloud#1195. This +brings the approvals consumer into line for the `manager` branch. + +## What the screen is + +`lookupManager(userId, organizationId)` now resolves the manager and then asks +whether he is **provably outside** the request's organization: + +| membership rows for the manager | result | +|---|---| +| some exist, none in the request's org | **screened out** — the slot falls through to the `manager:` literal | +| one is in the request's org | resolves, unchanged | +| none exist at all | resolves, unchanged — the tenancy fact is absent, not negative | +| the `sys_member` read failed | resolves, unchanged | +| the request carries no organization | resolves, unchanged — and no read is performed | + +The fail-open half is this file's ruled posture on addressing paths, stated +twice already: `filterApproversWhoCanRead` refuses to empty a live slate on an +infrastructure hiccup, and `expandPositionUsers` carries "a step routing to +nobody is worse than one routing to a lapsed holder". A drop is logged with the +manager's id, his organizations and the request's, so the fix ("repair the link" +/ "grant the membership" / "retarget the step") is legible without a debugger. + +## ⚠️ This moves one input from accepted to refused + +A node whose **sole** approver is a cross-org `manager` and which is authored +with the **non-default** `onEmptyApprovers: 'fail'` used to open successfully; +it now throws `NO_APPROVERS`. Nothing new is thrown — a screened-out manager +leaves only a `type:value` literal, which the pre-existing empty-slate test +already classifies as empty, and `'fail'` already throws on empty. Every +screened sibling has reached that same bucket since it was written. + +**The default policy is unaffected**: `admin_rescue` still opens the request +(decidable by a privileged admin) and warns, and `auto_approve` still +auto-approves. Both directions and both policies are pinned in +`manager-approver-org-screen.test.ts`. + +## What this does NOT decide + +- **#7497** (does approver routing imply record read visibility?) stays open. + The screen reads `sys_member`, which looks like the D2 read filter beside it, + and the code says at length why it is the *sibling* treatment instead: two of + the three org-scoped expansions already screen on `sys_member.organization_id`, + and `sys_user` offers no other tenancy fact. No reads are granted and no read + screen is applied to any type that lacked one. +- **`team`** is still unscreened — it is a sibling graph expansion that is not + org-scoped either, tracked as #10230, and it touches this same file. +- `APPROVER_ORG_SCOPED` is untouched. It answers ADR-0105 D9 *retargetability* + (may an author write `organization:` on this type?), not screening, and + `manager: false` remains correct. diff --git a/packages/plugins/plugin-approvals/src/approval-service.ts b/packages/plugins/plugin-approvals/src/approval-service.ts index 4f0a054ee2..f2fc427718 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.ts @@ -985,7 +985,12 @@ export class ApprovalService implements IApprovalService { } 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)); + // #10153: the request's OWN organization, not `directoryOrg`. They are + // provably equal on this branch (`manager` is not org-scoped, so a + // `organization` declaration is refused above), and naming the request + // org says what the screen asserts: tenancy of the request, never an + // ADR-0105 D9 retarget this type does not have. + const mgr = await this.lookupManager(String(subject), organizationId); if (mgr) return this.applyOooDelegation(mgr, now, organizationId, substitutions); } } @@ -1339,16 +1344,109 @@ export class ApprovalService implements IApprovalService { return Array.from(new Set((rows ?? []).map((r: any) => String(r.user_id ?? '')).filter(Boolean))); } - private async lookupManager(userId: string): Promise { + /** + * `sys_user.manager_id`, screened to the request's organization (#10153). + * + * Takes an organization argument for the same reason its siblings do + * ({@link expandPositionUsers}, {@link expandMembershipTierUsers}): an + * approver expansion answers "who, in THIS organization". Before #10153 this + * one did not ask, and it was the only expansion that did not — a + * `manager_id` pointing at a person in another organization routed that + * person an approval over a record they are not a tenant of. + * + * ⚠️ The screen reads `sys_member`, which LOOKS like the D2 read-visibility + * filter next to it ({@link filterApproversWhoCanRead}). It is not, and this + * comment exists so the next reader does not conclude that #7497 (does + * approver routing imply record read visibility?) was settled here. It was + * not. Two facts make this the SIBLING treatment rather than a + * read-visibility ruling: + * + * 1. Two of the three org-scoped expansions already screen on exactly this + * column — `expandMembershipTierUsers` filters `sys_member.organization_id` + * outright, and it is also the second limb of `expandPositionUsers`. So + * `sys_member.organization_id` is already this file's answer to "which + * organization is this person in", independent of what they may read. + * 2. `sys_user` carries no `organization_id` at all. It is a GLOBAL identity + * table, so a membership row is the only tenancy fact that exists for a + * user — there is no other read this screen could have been written with. + * + * This change grants no reads and applies no read screen to any type that + * lacks one today, so it decides nothing #7497 asks. + */ + private async lookupManager(userId: string, organizationId?: string | null): Promise { try { const rows = await this.engine.find('sys_user', { where: { id: userId }, fields: ['id', 'manager_id'], limit: 1, context: SYSTEM_CTX, } as any); const row: any = Array.isArray(rows) ? rows[0] : null; - return row?.manager_id ? String(row.manager_id) : null; + const managerId = row?.manager_id ? String(row.manager_id) : null; + if (!managerId) return null; + if (await this.managerIsProvablyOutsideOrg(managerId, organizationId)) return null; + return managerId; } catch { return null; } } + /** + * Is `managerId` PROVABLY a member of other organizations and not of + * `organizationId`? (#10153) + * + * "Provably" is the whole shape of this screen, and it is deliberate rather + * than a weaker version of "must prove membership": + * + * - membership rows exist for this user, none in the request's org + * ⇒ the tenancy fact is present and NEGATIVE ⇒ screen him out; + * - no membership rows at all, or the read failed + * ⇒ the tenancy fact is ABSENT ⇒ leave routing exactly as it was. + * + * The fail-open half is not timidity, it is this file's ruled posture on + * addressing paths, stated twice already: {@link filterApproversWhoCanRead} + * refuses to empty a live slate on an infrastructure hiccup, and + * {@link expandPositionUsers} carries "a step routing to nobody is worse than + * one routing to a lapsed holder". It is also load-bearing in practice — a + * stack that stamps an organization on its requests but does not materialize + * `sys_member` rows would otherwise lose every manager approver at once, + * which is a bigger behaviour change than the hole being closed. Measured: + * this repo's own `type:manager` out-of-office fixture is such a stack. + * + * Screening the MANAGER only, before OOO delegation, is deliberate too: the + * delegate arrives from `sys_approval_delegation`, whose rows already carry + * (and are already filtered by) an `organization_id` in + * {@link lookupActiveDelegation}. This card is about `sys_user.manager_id`. + */ + private async managerIsProvablyOutsideOrg( + managerId: string, + organizationId?: string | null, + ): Promise { + const requestOrg = organizationId ? String(organizationId) : ''; + // No organization on the request ⇒ nothing to screen against, and no read. + // The ordinary single-organization / embedded stack costs nothing here. + if (!requestOrg) return false; + let rows: any[] = []; + try { + rows = await this.engine.find('sys_member', { + where: { user_id: managerId }, + fields: ['user_id', 'organization_id'], + limit: 1000, + context: SYSTEM_CTX, + } as any); + } catch { return false; } // membership unreadable — see the fail-open note above + const orgs = (rows ?? []) + .map((r: any) => String(r?.organization_id ?? '')) + .filter(Boolean); + if (!orgs.length) return false; // no tenancy fact recorded for this user + if (orgs.includes(requestOrg)) return false; // he is a member here — route as before + this.logger?.warn?.( + `[approvals] #10153: manager '${managerId}' was dropped from the approver slate — ` + + `'sys_user.manager_id' points across an organization boundary. He holds membership in ` + + `${orgs.length} organization(s), none of them the request's organization '${requestOrg}', ` + + `so routing this approval to him would put approval authority over the record outside its ` + + `tenant. Fix the 'manager_id' link, grant him a membership in this organization, or route ` + + `this step with an approver type that names someone in it.`, + { managerId, requestOrganizationId: requestOrg, managerOrganizationIds: orgs }, + ); + return true; + } + /** * Out-of-office auto-skip (#1322 M1). Given an individually-routed approver * id, follow any active `sys_approval_delegation` chain and return the id the diff --git a/packages/plugins/plugin-approvals/src/manager-approver-org-screen.premise.test.ts b/packages/plugins/plugin-approvals/src/manager-approver-org-screen.test.ts similarity index 54% rename from packages/plugins/plugin-approvals/src/manager-approver-org-screen.premise.test.ts rename to packages/plugins/plugin-approvals/src/manager-approver-org-screen.test.ts index 0f3de49810..eb54858572 100644 --- a/packages/plugins/plugin-approvals/src/manager-approver-org-screen.premise.test.ts +++ b/packages/plugins/plugin-approvals/src/manager-approver-org-screen.test.ts @@ -1,25 +1,37 @@ // Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. /** - * MEASUREMENT HARNESS for #10153 — it pins the CURRENT (defective) behaviour on - * purpose, so the premise and the tiering question are reproducible rather than - * argued. It is NOT the fix and it is NOT on a pull request. + * #10153 — the `manager` approver is screened to the request's organization. * - * ⛔ Whoever implements the org screen must INVERT `PREMISE A` and `CLAUSE-2 (a)` - * — they assert today's cross-org resolution, which is exactly what the fix - * removes. `PREMISE B` / `B2` / `CLAUSE-2 (b)` describe the sibling treatment - * and stay as they are. + * This file began as round 1's MEASUREMENT HARNESS, which pinned the defective + * behaviour on purpose so the premise and the tiering question were + * reproducible rather than argued. `PREMISE A` and `CLAUSE-2 (a)` asserted + * today's cross-org resolution; the fix inverts both, exactly as that file's + * header instructed. The probes that describe the SIBLING treatment (`B`, + * `B2`, `C-b`) and the `team` gap (`W`) are unchanged, and they are what makes + * the inversion readable: the same tree, the same run, one screened type next + * to the newly screened one. * - * What it measures, all on one tree: - * A — `manager` resolves a manager whose only `sys_member` row is in - * another organization, into this organization's approver slate. - * B/B2 — the sibling `position` expansion IS screened, and the screen is not - * reject-everything (a same-org holder still resolves). - * W — `team` is NOT screened either, which is why #10153's warrant - * ("every sibling expansion is org-scoped") does not hold as stated. - * C-a/b — the tiering question: today a sole cross-org `manager` approver - * under `onEmptyApprovers: 'fail'` OPENS the request; a screened type - * in the identical shape THROWS `NO_APPROVERS`. Applying the screen - * therefore moves that input from accepted to refused. + * What it pins, both directions and both policies: + * A — a manager whose only `sys_member` row is in ANOTHER organization no + * longer resolves into this organization's slate. + * A2 — a manager who IS a member here still resolves. Without this, a + * screen that rejected everything would pass A. + * A3 — a manager with NO membership row anywhere still resolves: the + * tenancy fact is absent, so routing is left exactly as it was. + * A4 — a request with no organization at all is untouched (no screen, and + * no read to perform it). + * B/B2 — the sibling `position` expansion IS screened, and its screen is not + * reject-everything. + * W — `team` is still NOT screened. #10230 owns that; this card did not + * touch it, and the pin says so out loud. + * C-a — THE ACCEPT-TO-REJECT FLIP. Under `onEmptyApprovers: 'fail'` a node + * whose sole approver is a cross-org `manager` used to OPEN; it now + * throws `NO_APPROVERS`. That throw is PRE-EXISTING code and a bare + * `Error`, not a minted ADR-0112 envelope — there is no `code` / + * `status` to assert here, and inventing one would be a fiction. + * C-a2 — the same node under the DEFAULT policy (`admin_rescue`) still + * OPENS. This is what confines the flip to one non-default policy. + * C-b — a screened sibling in the identical shape throws too (unchanged). */ import { describe, it, expect, beforeEach } from 'vitest'; import { assertEngineDeleteDispatch, assertEngineUpdateDispatch } from '@objectstack/objectql'; @@ -96,7 +108,7 @@ function input(approvers: any[], configExtra: Record = {}) { }; } -describe('#10153 probe', () => { +describe('#10153 manager approver org screen', () => { let engine: ReturnType; let svc: ApprovalService; let n = 0; @@ -123,9 +135,38 @@ describe('#10153 probe', () => { ]; }); - it('PREMISE A — `manager` resolves ACROSS the org boundary (unscreened)', async () => { - const req = await svc.openNodeRequest(input([{ type: 'manager' }]), CTX_A); + // `{ type: 'manager' }` and `{ type: 'manager', value: 'owner_id' }` resolve + // through the same line (`record[a.value] ?? record.owner_id`); the explicit + // spelling is used from here on only so the unresolved fallback slot reads as + // `manager:owner_id` rather than `manager:undefined`. + const MGR = { type: 'manager', value: 'owner_id' }; + + it('A — a manager whose membership is in ANOTHER organization is screened OUT', async () => { + const req = await svc.openNodeRequest(input([MGR]), CTX_A); console.log('[PROBE A] request org =', req.organization_id, 'pending_approvers =', JSON.stringify(req.pending_approvers)); + // Inverted from round 1, which measured ['u_mgr_b'] here. + expect(req.pending_approvers).toEqual(['manager:owner_id']); + expect(req.pending_approvers.some((x: string) => !x.includes(':'))).toBe(false); + }); + + it('A2 — a manager who IS a member of the request org still resolves', async () => { + engine._tables['sys_member'].push({ id: 'm3', user_id: 'u_mgr_b', organization_id: ORG_A, role: 'member' }); + const req = await svc.openNodeRequest(input([MGR]), CTX_A); + console.log('[PROBE A2] pending_approvers =', JSON.stringify(req.pending_approvers)); + expect(req.pending_approvers).toEqual(['u_mgr_b']); + }); + + it('A3 — a manager with NO membership row anywhere is left alone (no tenancy fact)', async () => { + engine._tables['sys_member'] = engine._tables['sys_member'].filter((m: any) => m.user_id !== 'u_mgr_b'); + const req = await svc.openNodeRequest(input([MGR]), CTX_A); + console.log('[PROBE A3] pending_approvers =', JSON.stringify(req.pending_approvers)); + expect(req.pending_approvers).toEqual(['u_mgr_b']); + }); + + it('A4 — a request carrying no organization is untouched by the screen', async () => { + const ctxNoOrg = { userId: 'u_sub', positions: [], permissions: [] } as any; + const req = await svc.openNodeRequest(input([MGR]), ctxNoOrg); + console.log('[PROBE A4] request org =', req.organization_id, 'pending_approvers =', JSON.stringify(req.pending_approvers)); expect(req.pending_approvers).toEqual(['u_mgr_b']); }); @@ -142,10 +183,24 @@ describe('#10153 probe', () => { expect(req.pending_approvers).toEqual(['u_pos_a']); }); - it('CLAUSE-2 (a) — TODAY: sole cross-org `manager` + onEmptyApprovers:fail SUCCEEDS', async () => { - const req = await svc.openNodeRequest(input([{ type: 'manager' }], { onEmptyApprovers: 'fail' }), CTX_A); - console.log('[PROBE C-a] opened OK, status =', req.status, 'approvers =', JSON.stringify(req.pending_approvers)); + it('C-a — THE FLIP: sole cross-org `manager` + onEmptyApprovers:fail now THROWS', async () => { + // Round 1 measured this same call OPENING (status 'pending'). This is the + // accept-to-reject flip that re-graded the card `Clause-2: yes`, pinned so + // a reviewer reads it here rather than discovering it. + let err: any = null; + try { + await svc.openNodeRequest(input([MGR], { onEmptyApprovers: 'fail' }), CTX_A); + } catch (e) { err = e; } + console.log('[PROBE C-a] threw =', err ? String(err.message).slice(0, 90) : 'NOTHING'); + expect(err).toBeTruthy(); + expect(String(err.message)).toMatch(/^NO_APPROVERS:/); + }); + + it('C-a2 — the SAME node under the DEFAULT policy still opens (the flip is confined)', async () => { + const req = await svc.openNodeRequest(input([MGR]), CTX_A); // onEmptyApprovers absent => admin_rescue + console.log('[PROBE C-a2] status =', req.status, 'approvers =', JSON.stringify(req.pending_approvers)); expect(req.status).toBe('pending'); + expect(req.pending_approvers).toEqual(['manager:owner_id']); }); it('CLAUSE-2 (b) — a SCREENED sibling in the same shape THROWS NO_APPROVERS', async () => { @@ -157,7 +212,7 @@ describe('#10153 probe', () => { expect(err).toBeTruthy(); }); - it('WARRANT — sibling `team` is NOT org-screened either (cross-org team resolves)', async () => { + it('W — `team` is STILL not org-screened (#10230 owns it; this card did not touch it)', async () => { engine._tables['sys_team'] = [{ id: 'team_b', name: 'B team', organization_id: 'org_b' }]; engine._tables['sys_team_member'] = [{ id: 'tm1', team_id: 'team_b', user_id: 'u_team_b' }]; const req = await svc.openNodeRequest(input([{ type: 'team', value: 'team_b' }]), CTX_A); From 0e00b87a4b599a050b3650c2a9ab5b285bd4279f Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 20 Aug 2026 15:34:38 +0000 Subject: [PATCH 4/5] fix(approvals): type the membership read and re-point the double ledger at the renamed pin file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The #4918 ratchet grandfathers `approval-service.ts` for its EXISTING query-options erasures only, so the new `sys_member` read carries no `as any`. The engine-double ledger follows the harness file's rename — same two pinned doubles, no coverage lost. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx --- packages/plugins/plugin-approvals/src/approval-service.ts | 5 ++++- scripts/engine-double-contract.pinned.json | 4 ++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/packages/plugins/plugin-approvals/src/approval-service.ts b/packages/plugins/plugin-approvals/src/approval-service.ts index f2fc427718..8e66838682 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.ts @@ -1423,12 +1423,15 @@ export class ApprovalService implements IApprovalService { if (!requestOrg) return false; let rows: any[] = []; try { + // No `as any` on this options bag — #4918's ratchet grandfathers this + // file for its EXISTING erasures only, and a NEW one must carry the + // declared type. `ApprovalEngine.find` already accepts it as written. rows = await this.engine.find('sys_member', { where: { user_id: managerId }, fields: ['user_id', 'organization_id'], limit: 1000, context: SYSTEM_CTX, - } as any); + }); } catch { return false; } // membership unreadable — see the fail-open note above const orgs = (rows ?? []) .map((r: any) => String(r?.organization_id ?? '')) diff --git a/scripts/engine-double-contract.pinned.json b/scripts/engine-double-contract.pinned.json index b0fed0376a..14bd327590 100644 --- a/scripts/engine-double-contract.pinned.json +++ b/scripts/engine-double-contract.pinned.json @@ -1017,12 +1017,12 @@ "pinned": 1 }, { - "file": "packages/plugins/plugin-approvals/src/manager-approver-org-screen.premise.test.ts", + "file": "packages/plugins/plugin-approvals/src/manager-approver-org-screen.test.ts", "verb": "delete", "pinned": 1 }, { - "file": "packages/plugins/plugin-approvals/src/manager-approver-org-screen.premise.test.ts", + "file": "packages/plugins/plugin-approvals/src/manager-approver-org-screen.test.ts", "verb": "update", "pinned": 1 }, From 5f538cdf2f5d0d50f4390e6c68c6719bcfc2d091 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 20 Aug 2026 15:45:05 +0000 Subject: [PATCH 5/5] test(approvals): narrow the openNodeRequest union so the pins type-check in the hidden layer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `plugin-approvals` excludes `**/*.test.ts` from its tsconfig, so `pnpm --filter @objectstack/plugin-approvals typecheck` never read this file and reported exit 0 over it. `check:type-check-debt --re-measure` did: the pins billed TEST_DEBT 21 raw TS2339/TS18048, all from reading `pending_approvers` straight off `ApprovalRequestRow | ApprovalNodeAutoOutcome`. Narrowed through an `opened()` helper that REFUSES the auto-approval arm rather than casting past it — every probe asserts something about an opened request, so an auto-approval reaching one is a wrong answer that must say so. Re-measured: the package is back to its frozen 348, contributing 0. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx --- .../src/manager-approver-org-screen.test.ts | 49 ++++++++++++++----- 1 file changed, 37 insertions(+), 12 deletions(-) diff --git a/packages/plugins/plugin-approvals/src/manager-approver-org-screen.test.ts b/packages/plugins/plugin-approvals/src/manager-approver-org-screen.test.ts index eb54858572..af93008915 100644 --- a/packages/plugins/plugin-approvals/src/manager-approver-org-screen.test.ts +++ b/packages/plugins/plugin-approvals/src/manager-approver-org-screen.test.ts @@ -35,7 +35,8 @@ */ import { describe, it, expect, beforeEach } from 'vitest'; import { assertEngineDeleteDispatch, assertEngineUpdateDispatch } from '@objectstack/objectql'; -import { ApprovalService } from './approval-service.js'; +import type { ApprovalRequestRow } from '@objectstack/spec/contracts'; +import { ApprovalService, type ApprovalNodeAutoOutcome } from './approval-service.js'; function makeFakeEngine() { const tables: Record = {}; @@ -96,6 +97,24 @@ function makeFakeEngine() { }; } +/** + * `openNodeRequest` returns `ApprovalRequestRow | ApprovalNodeAutoOutcome` — the + * second arm is the `onEmptyApprovers: 'auto_approve'` exit, which opens no + * request at all. Narrow rather than read through the union: every probe below + * asserts something about an OPENED request, so an auto-approval reaching one of + * them is a wrong answer that must say so, not a property read off the wrong arm. + * (Left unnarrowed this file billed the package's TEST_DEBT ledger 21 raw TS2339 + * — invisible to `pnpm --filter @objectstack/plugin-approvals typecheck`, whose + * tsconfig excludes `**\/*.test.ts`, and caught only by `check:type-check-debt + * --re-measure`.) + */ +function opened(result: ApprovalRequestRow | ApprovalNodeAutoOutcome): ApprovalRequestRow { + if ('autoApproved' in result) { + throw new Error('expected an OPENED approval request, got an auto-approval outcome'); + } + return result; +} + const ORG_A = 'org_a'; const CTX_A = { userId: 'u_sub', organizationId: ORG_A, positions: [], permissions: [] } as any; @@ -135,6 +154,12 @@ describe('#10153 manager approver org screen', () => { ]; }); + // `ApprovalRequestRow` — the PUBLISHED contract type — declares no + // `organization_id`, though `openNodeRequest` stamps one on the row it writes + // and returns. Read the stamp off the persisted row rather than through a + // property the contract does not have. (Filed separately; not this card.) + const storedOrg = () => (engine._tables['sys_approval_request'] ?? [])[0]?.organization_id; + // `{ type: 'manager' }` and `{ type: 'manager', value: 'owner_id' }` resolve // through the same line (`record[a.value] ?? record.owner_id`); the explicit // spelling is used from here on only so the unresolved fallback slot reads as @@ -142,43 +167,43 @@ describe('#10153 manager approver org screen', () => { const MGR = { type: 'manager', value: 'owner_id' }; it('A — a manager whose membership is in ANOTHER organization is screened OUT', async () => { - const req = await svc.openNodeRequest(input([MGR]), CTX_A); - console.log('[PROBE A] request org =', req.organization_id, 'pending_approvers =', JSON.stringify(req.pending_approvers)); + const req = opened(await svc.openNodeRequest(input([MGR]), CTX_A)); + console.log('[PROBE A] request org =', storedOrg(), 'pending_approvers =', JSON.stringify(req.pending_approvers)); // Inverted from round 1, which measured ['u_mgr_b'] here. expect(req.pending_approvers).toEqual(['manager:owner_id']); - expect(req.pending_approvers.some((x: string) => !x.includes(':'))).toBe(false); + expect((req.pending_approvers ?? []).some((x: string) => !x.includes(':'))).toBe(false); }); it('A2 — a manager who IS a member of the request org still resolves', async () => { engine._tables['sys_member'].push({ id: 'm3', user_id: 'u_mgr_b', organization_id: ORG_A, role: 'member' }); - const req = await svc.openNodeRequest(input([MGR]), CTX_A); + const req = opened(await svc.openNodeRequest(input([MGR]), CTX_A)); console.log('[PROBE A2] pending_approvers =', JSON.stringify(req.pending_approvers)); expect(req.pending_approvers).toEqual(['u_mgr_b']); }); it('A3 — a manager with NO membership row anywhere is left alone (no tenancy fact)', async () => { engine._tables['sys_member'] = engine._tables['sys_member'].filter((m: any) => m.user_id !== 'u_mgr_b'); - const req = await svc.openNodeRequest(input([MGR]), CTX_A); + const req = opened(await svc.openNodeRequest(input([MGR]), CTX_A)); console.log('[PROBE A3] pending_approvers =', JSON.stringify(req.pending_approvers)); expect(req.pending_approvers).toEqual(['u_mgr_b']); }); it('A4 — a request carrying no organization is untouched by the screen', async () => { const ctxNoOrg = { userId: 'u_sub', positions: [], permissions: [] } as any; - const req = await svc.openNodeRequest(input([MGR]), ctxNoOrg); - console.log('[PROBE A4] request org =', req.organization_id, 'pending_approvers =', JSON.stringify(req.pending_approvers)); + const req = opened(await svc.openNodeRequest(input([MGR]), ctxNoOrg)); + console.log('[PROBE A4] request org =', storedOrg(), 'pending_approvers =', JSON.stringify(req.pending_approvers)); expect(req.pending_approvers).toEqual(['u_mgr_b']); }); it('PREMISE B — sibling `position` IS screened to the request org (same tree)', async () => { - const req = await svc.openNodeRequest(input([{ type: 'position', value: 'cfo' }]), CTX_A); + const req = opened(await svc.openNodeRequest(input([{ type: 'position', value: 'cfo' }]), CTX_A)); console.log('[PROBE B] pending_approvers =', JSON.stringify(req.pending_approvers)); expect(req.pending_approvers).toEqual(['position:cfo']); }); it('PREMISE B2 — same-org `position` holder DOES resolve (screen is not reject-everything)', async () => { engine._tables['sys_user_position'].push({ id: 'p2', user_id: 'u_pos_a', position: 'cfo', organization_id: ORG_A }); - const req = await svc.openNodeRequest(input([{ type: 'position', value: 'cfo' }]), CTX_A); + const req = opened(await svc.openNodeRequest(input([{ type: 'position', value: 'cfo' }]), CTX_A)); console.log('[PROBE B2] pending_approvers =', JSON.stringify(req.pending_approvers)); expect(req.pending_approvers).toEqual(['u_pos_a']); }); @@ -197,7 +222,7 @@ describe('#10153 manager approver org screen', () => { }); it('C-a2 — the SAME node under the DEFAULT policy still opens (the flip is confined)', async () => { - const req = await svc.openNodeRequest(input([MGR]), CTX_A); // onEmptyApprovers absent => admin_rescue + const req = opened(await svc.openNodeRequest(input([MGR]), CTX_A)); // onEmptyApprovers absent => admin_rescue console.log('[PROBE C-a2] status =', req.status, 'approvers =', JSON.stringify(req.pending_approvers)); expect(req.status).toBe('pending'); expect(req.pending_approvers).toEqual(['manager:owner_id']); @@ -215,7 +240,7 @@ describe('#10153 manager approver org screen', () => { it('W — `team` is STILL not org-screened (#10230 owns it; this card did not touch it)', async () => { engine._tables['sys_team'] = [{ id: 'team_b', name: 'B team', organization_id: 'org_b' }]; engine._tables['sys_team_member'] = [{ id: 'tm1', team_id: 'team_b', user_id: 'u_team_b' }]; - const req = await svc.openNodeRequest(input([{ type: 'team', value: 'team_b' }]), CTX_A); + const req = opened(await svc.openNodeRequest(input([{ type: 'team', value: 'team_b' }]), CTX_A)); console.log('[PROBE W] org_a request, org_b team -> pending_approvers =', JSON.stringify(req.pending_approvers)); expect(req.pending_approvers).toEqual(['u_team_b']); });