fix(plugin-approvals): an SLA reassign to a target that resolves to no one keeps the slate

.changeset/25598-sla-reassign-target-resolves.md

@@ -0,0 +1,17 @@
+---
+'@objectstack/plugin-approvals': patch
+---
+
+fix(plugin-approvals): an SLA escalation that reassigns to a target resolving to no one keeps the request on its approvers and notifies them, instead of stranding it on a slate nobody can decide
+
+Clause-②: no
+Metadata-protocol: none
+
+An approval node's `escalation: { action: 'reassign', escalateTo }` expands `escalateTo` to the holders of that position, and when nobody holds it falls back to the value itself, so a config naming a specific user id keeps working. That fallback is now judged by the same rule the reassign door applies: a user id or email an account carries (and not a user of other organizations only), or `position:<name>` for a position someone in the request's organization holds.
+
+- **A target that resolves** reassigns exactly as before.
+- **A target that does not** (an unstaffed or mistyped position name, a user id with no account, a `team:` / `department:` / `org_membership_level:` / `role:` literal) no longer replaces the slate. The pending approvers are kept and receive `approval.sla_breached`, and the `escalate` audit row records `reassign → <target> — not applied: <why>`. The escalation still fires once.
+- **The submitter's escalation notice** now names the action actually taken (`notify` when the reassign was not applied, or when no `escalateTo` was set), not the configured one.
+- A directory read that fails during the sweep leaves the request untouched and unmarked, so the next sweep retries it.
+
+Nothing an author writes is refused that was accepted before. To have an overdue request move, name a staffed position or a person in the request's organization in `escalateTo`.

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

@@ -1945,7 +1945,7 @@ describe('ApprovalService (node era)', () => {
     const req = await svc.openNodeRequest(
       openInput(['u9', 'u2'], {}, { escalation: { timeoutHours: 1, action: 'reassign', escalateTo: 'boss', notifySubmitter: false } }), CTX,
     );
-    staffDirectory(engine, 'u7');
+    staffDirectory(engine, 'u7', 'boss');
     await svc.reassign(req.id, { actorId: 'u9', to: 'u7' }, SYS);
     expect(indexRows().map(r => r.approver).sort()).toEqual(['u2', 'u7']);
 
@@ -2084,6 +2084,7 @@ describe('ApprovalService (node era)', () => {
   });
 
   it('runEscalations: reassign replaces the approver set with escalateTo', async () => {
+    staffDirectory(engine, 'boss');
     const req = await svc.openNodeRequest(
       openInput(['u9', 'u2'], {}, { escalation: { timeoutHours: 1, action: 'reassign', escalateTo: 'boss', notifySubmitter: false } }), CTX,
     );
@@ -2132,6 +2133,137 @@ describe('ApprovalService (node era)', () => {
     expect(emitted[0].audience).toEqual(['u9', 'u5']);
   });
 
+  // ── SLA reassign target resolution (#25598) ─────────────────────
+  //
+  // An escalation `reassign` whose `escalateTo` expands to no position holder
+  // falls back to the literal. A literal that resolves to no one used to be
+  // written onto the slate, stranding the request the way the reassign door
+  // did (#25485). Now the door's predicate judges it: the slate stays, its
+  // approvers are told, and the audit row says why.
+  describe('SLA reassign target resolution (#25598)', () => {
+    let emitted: any[];
+
+    beforeEach(() => {
+      emitted = [];
+      svc.attachMessaging({ async emit(input) { emitted.push(input); } });
+    });
+
+    /** Open an overdue `reassign` escalation to `escalateTo` on `slate`, the opening's notices cleared. */
+    async function overdueReassign(escalateTo: string, recordId = 'opp1'): Promise<string> {
+      const { id } = (await svc.openNodeRequest(
+        openInput(['u9', 'u2'], { recordId, record: { id: recordId } }, { escalation: { timeoutHours: 1, action: 'reassign', escalateTo, notifySubmitter: true } }), CTX,
+      )) as { id: string };
+      expect(engine._tables['sys_approval_request'].find(r => r.id === id)?.organization_id).toBe('t1');
+      makeOverdue(id);
+      emitted.splice(0);
+      return id;
+    }
+
+    const requestRow = (id: string) => engine._tables['sys_approval_request'].find(r => r.id === id)!;
+    const indexedOf = (id: string) => (engine._tables['sys_approval_approver'] ?? [])
+      .filter(r => r.request_id === id).map(r => r.approver).sort();
+    const escalateRow = (id: string) => (engine._tables['sys_approval_action'] ?? [])
+      .find(r => r.request_id === id && r.action === 'escalate');
+
+    async function expectKeptAndNotified(id: string, escalateTo: string, why: string) {
+      const before = { ...requestRow(id) };
+      emitted.splice(0);
+      expect(await svc.runEscalations()).toMatchObject({ escalated: 1 });
+      const row = requestRow(id);
+      expect(row.status).toBe('pending');
+      expect(row.pending_approvers).toBe('u9,u2');
+      expect(row.pending_approver_labels).toEqual(before.pending_approver_labels);
+      expect(indexedOf(id)).toEqual(['u2', 'u9']);
+      expect(escalateRow(id)?.comment).toBe(
+        `reassign → ${escalateTo} — not applied: ${why} The pending approvers were kept and notified.`,
+      );
+      // The current slate hears the lapse; the target that resolves to no one
+      // is not in the audience, and nothing announces a move.
+      expect(emitted.map(e => e.topic)).toEqual(['approval.sla_breached', 'approval.sla_breached']);
+      expect(emitted[0].audience).toEqual(['u9', 'u2']);
+      expect(emitted.some(e => e.topic === 'approval.escalated')).toBe(false);
+      // The submitter is told the action that was TAKEN.
+      expect(emitted[1].audience).toEqual(['u1']);
+      expect(emitted[1].payload.body).toContain("escalation action 'notify' was taken");
+      // Still single-shot: the escalate row is the marker.
+      expect(await svc.runEscalations()).toMatchObject({ escalated: 0 });
+    }
+
+    it('keeps the slate when escalateTo names no account (a departed user, a mistyped id)', async () => {
+      const id = await overdueReassign('ghost_user');
+      await expectKeptAndNotified(id, 'ghost_user', 'it names no user in this request\'s organization.');
+    });
+
+    it('keeps the slate when escalateTo is a position name nobody holds here', async () => {
+      engine._tables['sys_user_position'] = [
+        { id: 'up1', user_id: 'u5', position: 'approvals_supervisor', organization_id: 't2' }, // other tenant only
+      ];
+      const id = await overdueReassign('approvals_supervisor');
+      await expectKeptAndNotified(id, 'approvals_supervisor', 'it names no user in this request\'s organization.');
+    });
+
+    it('keeps the slate when escalateTo is a user whose only membership is another organization', async () => {
+      engine._tables['sys_user'] = [{ id: 'u_far' }];
+      engine._tables['sys_member'] = [{ id: 'm1', user_id: 'u_far', organization_id: 't2' }];
+      const id = await overdueReassign('u_far');
+      await expectKeptAndNotified(id, 'u_far', 'it names no user in this request\'s organization.');
+    });
+
+    it('keeps the slate when escalateTo is a slot literal no approver acts under, or a position:<p> nobody holds', async () => {
+      const team = await overdueReassign('team:night_shift');
+      await expectKeptAndNotified(team, 'team:night_shift', 'it is a slot literal no approver acts under, so no one could ever decide the request.');
+      const ghost = await overdueReassign('position:ghost', 'opp2');
+      await expectKeptAndNotified(ghost, 'position:ghost', 'no one holds position \'ghost\' in this request\'s organization.');
+    });
+
+    it('a failed sys_user read moves nothing and records nothing, so the next sweep escalates', async () => {
+      staffDirectory(engine, 'boss');
+      const id = await overdueReassign('boss');
+      const find = engine.find.bind(engine);
+      (engine as any).find = async (object: string, q: any) => {
+        if (object === 'sys_user') throw new Error('directory down');
+        return find(object, q);
+      };
+      expect(await svc.runEscalations()).toMatchObject({ escalated: 0 });
+      expect(requestRow(id).pending_approvers).toBe('u9,u2');
+      expect(escalateRow(id)).toBeUndefined();
+      expect(emitted).toEqual([]);
+      (engine as any).find = find;
+      expect(await svc.runEscalations()).toMatchObject({ escalated: 1 });
+      expect(requestRow(id).pending_approvers).toBe('boss');
+    });
+
+    it('still reassigns to an account\'s user id, its email, and a position:<p> someone holds here', async () => {
+      engine._tables['sys_user'] = [{ id: 'boss', email: 'boss@example.com' }];
+      engine._tables['sys_member'] = [{ id: 'm1', user_id: 'boss', organization_id: 't1' }];
+      engine._tables['sys_user_position'] = [
+        { id: 'up1', user_id: 'u5', position: 'finance', organization_id: 't1' },
+      ];
+      const byId = await overdueReassign('boss');
+      const byEmail = await overdueReassign('boss@example.com', 'opp2');
+      const byPosition = await overdueReassign('position:finance', 'opp3');
+      expect(await svc.runEscalations()).toMatchObject({ escalated: 3 });
+      expect(requestRow(byId).pending_approvers).toBe('boss');
+      expect(requestRow(byEmail).pending_approvers).toBe('boss@example.com');
+      expect(requestRow(byPosition).pending_approvers).toBe('position:finance');
+      expect(indexedOf(byPosition)).toEqual(['position:finance']);
+      for (const [id, to] of [[byId, 'boss'], [byEmail, 'boss@example.com'], [byPosition, 'position:finance']]) {
+        expect(escalateRow(id)?.comment).toBe(`reassign → ${to}`);
+      }
+      // A `type:value` slot is no messaging address (`notify` drops it), so the
+      // position's hand-off announces itself to no one; that is today's notice.
+      expect(emitted.filter(e => e.topic === 'approval.escalated').map(e => e.audience))
+        .toEqual([['boss'], ['boss@example.com']]);
+      const submitterNotices = emitted.filter(e => e.topic === 'approval.sla_breached');
+      expect(submitterNotices).toHaveLength(3);
+      expect(submitterNotices.every(e => e.payload.body.includes("escalation action 'reassign' was taken"))).toBe(true);
+      // …and the holder of the position then decides with the default actor.
+      await svc.decideNode(byPosition, { decision: 'approve', actorId: 'u5' },
+        { userId: 'u5', tenantId: 't1', positions: ['finance'], permissions: [] } as any);
+      expect(requestRow(byPosition).status).toBe('approved');
+    });
+  });
+
   it('runEscalations: skips requests that are not yet due or have no SLA', async () => {
     await svc.openNodeRequest(
       openInput(['u9'], {}, { escalation: { timeoutHours: 1000, action: 'auto_approve' } }), CTX,

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

@@ -5604,38 +5604,47 @@ export class ApprovalService implements IApprovalService {
    * screens.
    */
   private async assertReassignTargetResolves(to: string, organizationId: string | null): Promise<void> {
-    const refuse = (why: string): never => {
-      throw new Error(
-        `VALIDATION_FAILED: cannot reassign to '${to}' — ${why} The request and its approvers are unchanged. `
-        + `Reassign to the user id or email of a person in this request's organization, or to `
-        + `'position:<name>' for a position someone in it holds.`,
-      );
-    };
+    const why = await this.reassignTargetRefusal(to, organizationId);
+    if (why === undefined) return;
+    throw new Error(
+      `VALIDATION_FAILED: cannot reassign to '${to}' — ${why} The request and its approvers are unchanged. `
+      + `Reassign to the user id or email of a person in this request's organization, or to `
+      + `'position:<name>' for a position someone in it holds.`,
+    );
+  }
+
+  /**
+   * Why `to` resolves to no one in `organizationId` — the sentence a refusal
+   * carries — or `undefined` when someone can take a slot under it. The ONE
+   * predicate behind {@link assertReassignTargetResolves} (the door) and
+   * {@link escalateRequest}'s `reassign` (the SLA sweep, #25598); its contract
+   * is that docblock's.
+   */
+  private async reassignTargetRefusal(to: string, organizationId: string | null): Promise<string | undefined> {
     const position = addressedPosition(to);
     if (position !== undefined) {
-      if ((await this.expandPositionUsers(position, organizationId)).length) return;
-      refuse(`no one holds position '${position}' in this request's organization.`);
+      if ((await this.expandPositionUsers(position, organizationId)).length) return undefined;
+      return `no one holds position '${position}' in this request's organization.`;
     }
     if (to.includes(':')) {
-      refuse('it is a slot literal no approver acts under, so no one could ever decide the request.');
+      return 'it is a slot literal no approver acts under, so no one could ever decide the request.';
     }
+    const noUser = 'it names no user in this request\'s organization.';
     const rows = await this.engine.find('sys_user', {
       where: to.includes('@') ? { email: to } : { id: to },
       fields: ['id'], limit: 1, context: SYSTEM_CTX,
     });
     const userId = Array.isArray(rows) && rows[0]?.id != null ? String(rows[0].id) : '';
-    if (!userId) refuse('it names no user in this request\'s organization.');
-    if (!organizationId) return;
+    if (!userId) return noUser;
+    if (!organizationId) return undefined;
     let memberships: any[] = [];
     try {
       memberships = await this.engine.find('sys_member', {
         where: { user_id: userId }, fields: ['organization_id'], limit: 1000, context: SYSTEM_CTX,
       });
-    } catch { return; } // membership unreadable ⇒ no tenancy fact, as in the sibling screens
+    } catch { return undefined; } // membership unreadable ⇒ no tenancy fact, as in the sibling screens
     const orgs = (memberships ?? []).map((r: any) => String(r?.organization_id ?? '')).filter(Boolean);
-    if (orgs.length && !orgs.includes(String(organizationId))) {
-      refuse('it names no user in this request\'s organization.');
-    }
+    return orgs.length && !orgs.includes(String(organizationId)) ? noUser : undefined;
   }
 
   /**
@@ -7040,12 +7049,26 @@ export class ApprovalService implements IApprovalService {
     // empty expansion falls back to the literal, so a config naming a
     // specific user id keeps working unchanged.
     let escalatees: string[] = [];
+    let literalTarget = false;
     if (escalateTo) {
       try {
         escalatees = await this.expandPositionUsers(escalateTo, raw.organization_id ?? null);
       } catch { escalatees = []; }
-      if (!escalatees.length) escalatees = [escalateTo];
-    }
+      if (!escalatees.length) { escalatees = [escalateTo]; literalTarget = true; }
+    }
+    // [#25598] …but a literal must still name someone who can take the slot,
+    // by the door's own predicate (#25485). One that resolves to no one (an
+    // unstaffed position, a mistyped name, a departed user, a `team:` literal)
+    // would move the request off its real approvers onto a slate nobody can
+    // decide. So the slate stays, its approvers are told the SLA lapsed (the
+    // `notify` branch), and the audit row says why. A failed `sys_user` read
+    // throws before the audit row, so the next sweep retries the request.
+    const unresolved = action === 'reassign' && literalTarget
+      ? await this.reassignTargetRefusal(escalateTo!, raw.organization_id ?? null)
+      : undefined;
+    if (unresolved !== undefined) escalatees = [];
+    // The action this sweep TAKES: a reassign with no one to hand to notifies.
+    const taken = action === 'reassign' && !escalatees.length ? 'notify' : action;
 
     // Audit first — this row IS the idempotency marker (ADR-0042 §1), and the
     // marker is its KIND (`escalate`), not its actor. A machine has no
@@ -7057,11 +7080,12 @@ export class ApprovalService implements IApprovalService {
       id: uid('aact'), request_id: raw.id, organization_id: raw.organization_id ?? null,
       step_name: raw.flow_node_id ?? raw.current_step ?? null, step_index: 0, action: 'escalate',
       actor_id: null,
-      comment: `${action}${escalateTo ? ` → ${escalateTo}` : ''}`,
+      comment: `${action}${escalateTo ? ` → ${escalateTo}` : ''}`
+        + (unresolved !== undefined ? ` — not applied: ${unresolved} The pending approvers were kept and notified.` : ''),
       created_at: now,
     }), { context: SYSTEM_CTX });
 
-    if (action === 'reassign' && escalatees.length) {
+    if (taken === 'reassign') {
       await this.engine.update('sys_approval_request', {
         id: raw.id, pending_approvers: escalatees.join(','), updated_at: now,
         pending_approver_labels: await this.slateLabels(escalatees),
@@ -7086,7 +7110,8 @@ export class ApprovalService implements IApprovalService {
         comment: 'SLA escalation',
       }, SYSTEM_CTX, 'escalation');
     } else {
-      // 'notify' (and the reassign-without-target fallback)
+      // 'notify' (and a reassign with no one to hand to: no target, or one
+      // that resolves to no one)
       await this.notify({
         topic: 'approval.sla_breached',
         organizationId: raw.organization_id,
@@ -7106,7 +7131,7 @@ export class ApprovalService implements IApprovalService {
         source: { object: 'sys_approval_request', id: raw.id },
         ...approvalNotice(approvalNotificationText({
           title: 'approvalSlaBreachedSubmitterTitle', body: 'approvalSlaBreachedSubmitterBody',
-          params: { ...subject, action },
+          params: { ...subject, action: taken },
         })),
       });
     }