fix(plugin-approvals)!: an approval node refuses to open when a team or position directory read fails or a queue expansion rejects, and the ownership-queue reads page to completion

.changeset/29604-approver-directory-refusal-queue-paging.md

@@ -0,0 +1,20 @@
+---
+'@objectstack/plugin-approvals': patch
+'@objectstack/plugin-sharing': patch
+---
+
+fix(plugin-approvals)!: an approval node refuses to open when a team or position approver's directory read fails, or a queue approver's expansion rejects, and the ownership-queue reads page to completion
+
+Clause-②: yes (narrowing) — an approval node no longer opens when a `team` or `position` approver (static, or an `expression` approver with `resolveAs` `team` / `position`) cannot be expanded because a directory read or the cross-organization read filter failed, or when a `queue` approver's expansion rejects. Under every `onEmptyApprovers` policy, the `fallbackApprovers` slate included, it now fails the node with a retryable `APPROVER_DIRECTORY_UNREADABLE:` error naming the read, where it used to open on the rest of the slate, on the fallback slate or on the administrators, or auto-approve. The registered ownership-queue service does not reject yet: a failed member read inside it still answers nobody until #29604 part 2 (#30251), so a `queue` approver refuses here only when `expandMembers` itself rejects. Every other input answers as before.
+Metadata-protocol: none
+
+<!-- adr-0087: not-required (no-migration-prescription) No key of any schema is added, removed, renamed or re-shaped: the approval node config, the approver types and the ownership-queue contract keep their shapes. What moves is the answer a node gives while a directory table cannot be read, and nothing at rest records that answer, so there is nothing for a migration to rewrite. -->
+
+**BREAKING** accept-set narrowing at the approval node (`@objectstack/plugin-approvals`): a failed directory read behind a `team` or `position` approver, or a `queue` approver whose expansion rejects, refuses the node instead of opening it on a slate that is missing people (ADR-0142 D6 as amended, ruling record 6599731698 on #29604).
+
+- **Before.** A `team` or `position` approver whose `sys_team_member`, `sys_user_position` or `sys_member` read failed kept its `type:value` literal slot, which nobody can act on, and logged the failure. A `queue` approver whose expansion threw answered "expanded to nobody" and added no slot. Either way the node went on as if those people did not exist: beside other approvers it opened on them alone, and on an otherwise empty slate the node's `onEmptyApprovers` policy acted, opening on the `fallbackApprovers` or the organization's administrators, or auto-approving.
+- **Now.** The node does not open and writes no request, approver or action row. It fails with an `ApproverDirectoryUnreadableError` whose message starts `APPROVER_DIRECTORY_UNREADABLE:`, names the approver, the organization and the read that failed (a failure of the cross-organization read filter is named as that filter's read, on a static approver and on an `expression` one alike), and is retryable: the flow node's failure is a runtime one, so the run can be resumed once the directory can be read. A `fallbackApprovers` slate whose own `team` or `position` read fails refuses the same way. `department` / `business_unit`, `org_membership_level` and `manager` approvers are not named by the ruling and answer exactly as before.
+- **Unchanged.** A decision on a request opened before the approver snapshot existed still re-reads its slate on the old path, which keeps the literal slot on a failed read, because the decision has already been recorded when that read happens.
+- **Not yet.** The ownership-queue service shipped in `@objectstack/plugin-sharing` answers a failed `sys_queue` / `sys_queue_member`, team or position read with nobody (logged once) rather than rejecting, so a `queue` approver backed by it still adds no slot on such a failure. Its refusal arrives with #29604 part 2, once the contract can carry it (#30251).
+
+`@objectstack/plugin-sharing`: the ownership-queue service reads its `sys_queue` and `sys_queue_member` rows by id, page after page, to the end, where it read one page of 10,000 rows and answered on it. A queue's 10,001st member, an organization's 10,001st queue and the queues naming a recipient past that bound are now found. A read that fails partway answers as a failed read did before (that read contributes nobody, logged once, with the read's reason and without the walk's own "refuses instead" clause), never with the pages it had read.

packages/plugins/plugin-approvals/src/approval-service.test.ts

@@ -11,11 +11,21 @@
 
 import { describe, it, expect, beforeEach, vi } from 'vitest';
 import { APPROVAL_REVISE_NODE_TYPE, templateSlotSource } from '@objectstack/spec/automation';
-import { ApprovalService, REMIND_COOLDOWN_MS, ESCALATION_ENABLED_FLIP_CUTOFF_MS, SLA_ACTOR_ID } from './approval-service.js';
+import {
+  ApprovalService, ApproverDirectoryUnreadableError, REMIND_COOLDOWN_MS, ESCALATION_ENABLED_FLIP_CUTOFF_MS, SLA_ACTOR_ID,
+} from './approval-service.js';
 import { bindApprovalLockHook, bindDelegationWriteGuard, unbindAllHooks } from './lifecycle-hooks.js';
+import { filterApproversWhoCanRead } from './approver-org-scope.js';
 import { celEngine } from '@objectstack/formula';
 import { SysApprovalRequest } from './sys-approval-request.object.js';
 
+// [#29604] A pass-through spy on the ADR-0105 D9 read filter, so one describe
+// below can make it reject. Every other case runs the real filter through it.
+vi.mock('./approver-org-scope.js', async (importOriginal) => {
+  const actual = await importOriginal<typeof import('./approver-org-scope.js')>();
+  return { ...actual, filterApproversWhoCanRead: vi.fn(actual.filterApproversWhoCanRead) };
+});
+
 interface FakeRow { [k: string]: any }
 
 /**
@@ -4207,6 +4217,205 @@ describe('ApprovalService — a queue approver routes to the queue\'s members (A
   });
 });
 
+// [ADR-0142 D6 as amended; ruling record 6599731698 on #29604, letter B] A
+// directory read behind a `team`, `position` or `queue` approver that FAILS is
+// not an empty slate: the node refuses to open under every `onEmptyApprovers`
+// policy, on its own retryable error class, naming the read — and writes
+// nothing. Before, the slot fell back to its literal and the policy acted on a
+// slate missing the people who could not be read.
+describe('ApprovalService — a team, position or queue approver whose directory read fails refuses the node (ADR-0142 D6)', () => {
+  const POLICIES = ['admin_rescue', 'auto_approve', 'fail', 'fallback'] as const;
+
+  /** A service over `engine` whose reads of `failing` throw, as a driver outage does. */
+  const outage = (failing: string[], queues?: () => any) => {
+    const engine = makeFakeEngine();
+    engine._tables['sys_team_member'] = [{ id: 'tm1', team_id: 't_ops', user_id: 'u5' }];
+    engine._tables['sys_user_position'] = [{ id: 'up1', position: 'cfo', user_id: 'u9', organization_id: 't1' }];
+    const real = engine.find.bind(engine);
+    engine.find = async (object: string, options?: any) => {
+      if (failing.includes(object)) throw new Error(`SQLITE_IOERR: disk I/O error reading ${object}`);
+      return real(object, options);
+    };
+    const warnings: any[] = [];
+    let n = 0;
+    const svc = new ApprovalService({
+      engine,
+      clock: { now: () => new Date(1757000000000 + (n++) * 1000) },
+      logger: { warn: (msg: any, meta: any) => warnings.push([msg, meta]) },
+      ...(queues ? { ownershipQueues: queues } : {}),
+    });
+    return { engine, svc, warnings };
+  };
+  const node = (approvers: any[], configExtra: Record<string, any> = {}) => ({
+    ...openInput([]),
+    config: { approvers, behavior: 'first_response' as const, lockRecord: false, ...configExtra },
+  });
+  const policy = (p: (typeof POLICIES)[number]) => (
+    p === 'fallback' ? { onEmptyApprovers: p, fallbackApprovers: [{ type: 'user', value: 'u_backstop' }] } : { onEmptyApprovers: p }
+  );
+  const refusalOf = (p: Promise<unknown>) => p.then(() => null, (e: unknown) => e);
+  const nothingWritten = (engine: ReturnType<typeof makeFakeEngine>) => {
+    expect(engine._tables['sys_approval_request'] ?? []).toEqual([]);
+    expect(engine._tables['sys_approval_approver'] ?? []).toEqual([]);
+    expect(engine._tables['sys_approval_action'] ?? []).toEqual([]);
+  };
+
+  for (const [type, value, table] of [['team', 't_ops', 'sys_team_member'], ['position', 'cfo', 'sys_user_position']] as const) {
+    for (const p of POLICIES) {
+      it(`a ${type} approver whose ${table} read fails: onEmptyApprovers '${p}' opens nothing and approves nothing`, async () => {
+        const { engine, svc } = outage([table]);
+        const refusal = await refusalOf(svc.openNodeRequest(node([{ type, value }], policy(p)), CTX));
+
+        expect(refusal).toBeInstanceOf(ApproverDirectoryUnreadableError);
+        expect(refusal).toMatchObject({ approverType: type, approverValue: value, retryable: true });
+        expect((refusal as Error).message).toMatch(new RegExp(`^APPROVER_DIRECTORY_UNREADABLE: approver '${type}:${value}'`));
+        expect((refusal as ApproverDirectoryUnreadableError).failedRead).toMatch(/SQLITE_IOERR/);
+        expect((refusal as Error).message).not.toMatch(/NO_APPROVERS|expanded to nobody/);
+        nothingWritten(engine);
+      });
+    }
+  }
+
+  it('beside a staffed person, the node still refuses: no slate shortened to the people who could be read', async () => {
+    const { engine, svc } = outage(['sys_team_member']);
+    const refusal = await refusalOf(svc.openNodeRequest(
+      node([{ type: 'user', value: 'u2' }, { type: 'team', value: 't_ops' }]), CTX,
+    ));
+
+    expect(refusal).toBeInstanceOf(ApproverDirectoryUnreadableError);
+    nothingWritten(engine);
+  });
+
+  it('a fallback slate whose directory read fails refuses too, rather than degrade to the administrators', async () => {
+    // The primary slate reads fine and is empty (an unstaffed position); the
+    // declared fallback names a team whose read fails.
+    const { engine, svc } = outage(['sys_team_member']);
+    const refusal = await refusalOf(svc.openNodeRequest(node([{ type: 'position', value: 'nobody_holds_this' }], {
+      onEmptyApprovers: 'fallback', fallbackApprovers: [{ type: 'team', value: 't_ops' }],
+    }), CTX));
+
+    expect(refusal).toBeInstanceOf(ApproverDirectoryUnreadableError);
+    expect(refusal).toMatchObject({ approverType: 'team', approverValue: 't_ops' });
+    nothingWritten(engine);
+  });
+
+  it('a queue whose expansion fails refuses before its "expanded to nobody" answer', async () => {
+    const { engine, svc, warnings } = outage([], () => ({
+      expandMembers: async () => { throw new Error('ownership-queue read failed'); },
+    }));
+    const refusal = await refusalOf(svc.openNodeRequest(node([{ type: 'queue', value: 'billing' }]), CTX));
+
+    expect(refusal).toBeInstanceOf(ApproverDirectoryUnreadableError);
+    expect(refusal).toMatchObject({ approverType: 'queue', approverValue: 'billing' });
+    expect(warnings.some(([msg]) => String(msg).includes("approver 'queue:billing' expanded to nobody"))).toBe(false);
+    nothingWritten(engine);
+  });
+
+  it('control — the readable slate opens as before', async () => {
+    const { svc } = outage([]);
+    const req = await svc.openNodeRequest(node([{ type: 'team', value: 't_ops' }, { type: 'position', value: 'cfo' }]), CTX) as any;
+
+    expect(req.status).toBe('pending');
+    expect([...req.pending_approvers].sort()).toEqual(['u5', 'u9']);
+  });
+
+  it('control — a department read that fails is not named by the ruling: the node still opens, as before', async () => {
+    const { engine, svc } = outage(['sys_business_unit', 'sys_business_unit_member']);
+    const outcome = await refusalOf(svc.openNodeRequest(
+      node([{ type: 'department', value: 'bu_x' }], { onEmptyApprovers: 'fallback', fallbackApprovers: [{ type: 'user', value: 'u_backstop' }] }),
+      CTX,
+    ));
+
+    expect(outcome).toBeNull();
+    expect(engine._tables['sys_approval_request']).toHaveLength(1);
+  });
+
+  it("decide's pre-snapshot re-resolution keeps the literal: a recorded decision is never failed by the outage", async () => {
+    // A unanimous request opened while the directory read, then stripped of
+    // its open-time snapshot as a request opened before the snapshot existed.
+    const failing: string[] = [];
+    const { engine, svc } = outage(failing);
+    const req = await svc.openNodeRequest(
+      node([{ type: 'user', value: 'u2' }, { type: 'team', value: 't_ops' }], { behavior: 'unanimous' }), CTX,
+    ) as any;
+    const row = engine._tables['sys_approval_request'].find((r: any) => r.id === req.id)!;
+    const config = JSON.parse(row.node_config_json);
+    delete config.__approverGroups;
+    row.node_config_json = JSON.stringify(config);
+
+    failing.push('sys_team_member');
+    const decided = await svc.decideNode(req.id, { decision: 'approve', actorId: 'u2' }, SYS);
+    expect(decided.decision).toBe('approve');
+  });
+});
+
+// [ADR-0142 D6 as amended; ruling record 6599731698 on #29604, letter B] The
+// ADR-0105 D9 cross-organization read filter is a read behind a `position` or
+// `team` approver as much as the directory expansion is. When it fails, the
+// node refuses, naming THAT read, on the static resolver (its `bounded`
+// filter) and on an `expression` approver's `resolveAs` values alike.
+// `filterApproversWhoCanRead` answers its own failed `sys_member` read by
+// keeping the slate, so the pass-through spy above is how it is made to fail.
+// The scenario is `approver-cross-org.integration.test.ts`'s: a Plant A
+// request routed to the group CFO.
+describe('ApprovalService — a failed cross-organization read filter refuses the node, named as that read (ADR-0142 D6)', () => {
+  const PLANT_CTX = { userId: 'u_submitter', organizationId: 'o_plant', positions: [], permissions: [] } as any;
+  const FILTER_READ = /^the cross-organization read filter \(who among the people of organization 'o_group' can read this request of organization 'o_plant'\) could not be read: membership read filter down\.$/;
+  let engine: ReturnType<typeof makeFakeEngine>;
+  let svc: ApprovalService;
+  let n = 0;
+
+  beforeEach(() => {
+    vi.mocked(filterApproversWhoCanRead).mockClear();
+    engine = makeFakeEngine();
+    engine._tables['sys_user'].push({ id: 'u_cfo' });
+    engine._tables['sys_organization'] = [
+      { id: 'o_group', slug: 'acme-group', parent_organization_id: null },
+      { id: 'o_plant', slug: 'acme-plant-a', parent_organization_id: 'o_group' },
+    ];
+    engine._tables['sys_user_position'] = [{ id: 'up1', user_id: 'u_cfo', position: 'cfo', organization_id: 'o_group' }];
+    engine._tables['sys_member'] = [{ id: 'm1', user_id: 'u_cfo', organization_id: 'o_plant', role: 'member' }];
+    n = 0;
+    svc = new ApprovalService({
+      engine: engine as any,
+      clock: { now: () => new Date(1767000000000 + (n++) * 1000) },
+      tenancyPosture: () => 'group',
+    });
+  });
+
+  const node = (approver: Record<string, unknown>) => ({
+    object: 'purchase_order',
+    recordId: 'po1',
+    runId: 'run_1',
+    nodeId: 'group_signoff',
+    flowName: 'po_approval',
+    config: { approvers: [approver as any], behavior: 'unanimous' as const, lockRecord: false, onEmptyApprovers: 'admin_rescue' as const },
+    record: { id: 'po1', signoff_position: 'cfo' },
+  });
+  const refusalOf = (p: Promise<unknown>) => p.then(() => null, (e: unknown) => e);
+
+  for (const [label, approver] of [
+    ['a static position approver', { type: 'position', value: 'cfo', organization: '$root' }],
+    ['an expression approver resolving as position', { type: 'expression', value: 'trigger.signoff_position', resolveAs: 'position', organization: '$root' }],
+  ] as const) {
+    it(`control — ${label} reaches the group CFO through the filter`, async () => {
+      const req = await svc.openNodeRequest(node(approver), PLANT_CTX) as any;
+      expect(req.pending_approvers).toEqual(['u_cfo']);
+      expect(filterApproversWhoCanRead).toHaveBeenCalledTimes(1);
+    });
+
+    it(`${label}: the filter's failure refuses the node and names the filter's read`, async () => {
+      vi.mocked(filterApproversWhoCanRead).mockRejectedValueOnce(new Error('membership read filter down'));
+      const refusal = await refusalOf(svc.openNodeRequest(node(approver), PLANT_CTX));
+
+      expect(refusal).toBeInstanceOf(ApproverDirectoryUnreadableError);
+      expect(refusal).toMatchObject({ approverType: 'position', approverValue: 'cfo', retryable: true });
+      expect((refusal as ApproverDirectoryUnreadableError).failedRead).toMatch(FILTER_READ);
+      expect(engine._tables['sys_approval_request'] ?? []).toEqual([]);
+    });
+  }
+});
+
 // #3807 follow-up: `queue` was not the only way to end up with a slot nobody
 // can act on — every GRAPH approver type falls back to the same literal when
 // its lookup finds no one, and that fallback used to happen in total silence.