fix(approvals): every sys_user lookup approvals writes holds an id or null — machine actors record none, notify forwards only a person, reassign parties are slot addresses (ADR-0118 D1)

.changeset/21455-approval-actor-lookups-hold-ids.md

@@ -0,0 +1,26 @@
+---
+'@objectstack/plugin-approvals': patch
+---
+
+Every `sys_user` lookup the approvals plugin writes now holds a user id or nothing: the SLA and dead-run sweeps record no actor instead of a `system:` placeholder, notifications name only the person who acted, and `reassign_from` / `reassign_to` become slot-address text columns; stored placeholders are cleared at the next boot
+
+Clause-②: no
+
+Under ADR-0118 D1 a lookup to `sys_user` holds a user id or null, never a placeholder value. Four writers broke that, and a lookup holding a non-id drops the row from every join and report on it, silently.
+
+**This supersedes the "The SLA sweep keeps its reserved `system:sla` actor for now" sentence of the unreleased `21411-approval-actor-person` changeset.** Both ship in one release; from it, the sweep records no actor.
+
+- **Machine actors record no actor.**
+  - The SLA sweep's `escalate` row, and the `approve` / `reject` an `auto_approve` / `auto_reject` escalation then records, have `actor_id` empty. Before, both held `system:sla`. The `escalate` row's comment still names the configured action.
+  - The dead-run sweep's `recall` row has `actor_id` empty. Before, it held `system:dead-run`. Its comment still names the dead run and its status, and a submitter's own recall still records the submitter.
+- **Notifications name only a person.** The actor the plugin hands to `sys_notification.actor_id` (and so to each `sys_inbox_message.actor_id`) is the user the action's context vouches for, or nothing.
+  - Before, a reassign, reminder, request for information, comment or send-back taken under a named position or email forwarded that address as the actor.
+  - Before, every SLA notification forwarded `system:sla`. It now forwards no actor, as the out-of-office notifications already did.
+- **`reassign_from` / `reassign_to` are slot addresses.** A reassignment moves a pending-approver slot, so both columns hold the slot's address in its stored spelling: a user id, an email, or `position:<name>`. They are now text columns (max 255 characters, like `acted_as`) instead of `sys_user` lookups, which matches what they already stored.
+  - Existing values need no rewrite, and an existing database keeps its columns as they are. On SQLite and PostgreSQL 16, booting the new declaration over a table created by the old one issues no DDL, keeps every stored value, and reports no schema drift for either column. A new database creates them as `text`, as it does `acted_as`.
+  - The action log still resolves `reassign_from_name` / `reassign_to_name` where an address names an account: a user id, or an email an account carries. A position address has no name.
+- **Stored rows.** The boot-time repair that moves slot literals out of `actor_id` now also clears `system:sla` and `system:dead-run` from it, in the same pass and in the same idempotent way. A cleared sentinel gets no `acted_as`, because a sweep takes no slot. The boot log line reports the count as `sentinelsCleared`.
+- **For a report or integration that read these values:**
+  - To find the SLA sweep's actions, read the `escalate` rows, and the decision that directly follows an `escalate` row whose comment names `auto_approve` or `auto_reject`. Do not test `actor_id` for `system:sla`.
+  - To find a dead-run release, read the `recall` row whose comment names the run. Do not test `actor_id` for `system:dead-run`.
+  - Read `reassign_from` / `reassign_to` as slot addresses. Do not expand them as `sys_user` references.

content/docs/automation/approvals.mdx

@@ -512,7 +512,9 @@ not a pending approver`); a request that isn't pending returns 409
 (`INVALID_STATE`). The decision records two facts on its `sys_approval_action`
 row: `actor_id` is the user who decided, and `acted_as` is the slot the decision
 took, in that slot's stored spelling — the multi-approver tally counts approvals
-by matching `acted_as` against the slate. Always go through
+by matching `acted_as` against the slate. A `reassign` row records the slot it
+moved the same way: `reassign_from` and `reassign_to` hold slot addresses (a
+user id, an email or a position address), not users. Always go through
 these endpoints — never resume the flow run directly, and since #3801 you
 **cannot**: `POST /api/v1/automation/{flow}/runs/{runId}/resume` answers 403 for
 a run parked on an `approval` node (including via a `subflow` pause) and changes
@@ -672,8 +674,9 @@ trace that a slate had been bypassed was the designated approver's later
 reaches a terminal state without a decision — it failed, was cancelled, timed
 out, or the process hosting it crashed — nothing is left to decide the request,
 so a periodic sweep finalizes it as `recalled` and releases the record. The
-audit row records the actor `system:dead-run` and names the run and its status,
-so it reads distinctly from a submitter's own recall.
+audit row records no actor — a sweep is not a user, so `actor_id` is empty,
+never a placeholder value (ADR-0118 D1) — and names the run and its status, so
+it reads distinctly from a submitter's own recall, which records the submitter.
 
 The sweep only ever acts on a run it can positively confirm is terminal: a
 paused run (the normal state of a live approval), an unknown run, or an
@@ -724,7 +727,10 @@ that request's drawer directly instead of a generic list.
 `escalation` is real, not decorative: set `enabled: true` and `timeoutHours`,
 and pick an `action` — `notify` (default), `reassign`, `auto_approve`, or
 `auto_reject`. Auto decisions run through the normal decide path, so the flow
-resumes exactly as if a human had clicked. Every escalation writes an audit row.
+resumes exactly as if a human had clicked. Every escalation writes an audit row
+of kind `escalate` naming the configured action; it, the auto decision that
+follows it, and the escalation's notifications record no actor, because no
+person acted (ADR-0118 D1).
 `timeoutHours` counts **calendar (wall-clock) hours** — nights, weekends and
 holidays included, because the platform ships no business-hours calendar — so a
 request opened at 17:00 on a Friday with `timeoutHours: 4` escalates at 21:00

packages/plugins/plugin-approvals/src/action-slot-backfill-wiring.test.ts

@@ -15,7 +15,7 @@
 import { describe, it, expect, vi, beforeEach } from 'vitest';
 
 const backfill = vi.hoisted(() => ({
-  fn: vi.fn(async (_engine: unknown) => ({ literalsMoved: 0, votesStamped: 0 })),
+  fn: vi.fn(async (_engine: unknown) => ({ literalsMoved: 0, sentinelsCleared: 0, votesStamped: 0 })),
 }));
 
 vi.mock('./action-slot-backfill.js', async (importOriginal) => ({
@@ -71,6 +71,14 @@ describe('the action-slot backfill is wired on kernel:ready (#21411)', () => {
     expect(backfill.fn.mock.calls[0][0]).toBe(engine);
   });
 
+  it('a run that only cleared machine sentinels still reports what it wrote', async () => {
+    backfill.fn.mockResolvedValueOnce({ literalsMoved: 0, sentinelsCleared: 2, votesStamped: 0 });
+    const { ctx, fire, logged } = fakeContext(fakeEngine());
+    await new ApprovalsServicePlugin({ disableAutoHooks: true }).start(ctx);
+    await fire('kernel:ready');
+    expect(logged.info.some((m) => m.includes('action slots backfilled'))).toBe(true);
+  });
+
   it('a failed run is logged at error, naming what stays wrong and the fix', async () => {
     backfill.fn.mockRejectedValueOnce(new Error('driver went away'));
     const { ctx, fire, logged } = fakeContext(fakeEngine());

packages/plugins/plugin-approvals/src/action-slot-backfill.integration.test.ts

@@ -8,9 +8,12 @@
  * the person who decided. The writers now put the person in `actor_id` and the
  * slot in `acted_as`, and every slot reader reads `acted_as` with no fallback.
  * Rows written before that are moved by `backfillActionSlots`, wired on
- * `kernel:ready`. This pins what it moves, what it leaves alone, that it is
- * idempotent, and — end to end — that an in-flight multi-approver tally still
- * counts the votes it had already collected.
+ * `kernel:ready`. The same column also held the two machine sweeps' sentinels
+ * (`system:sla`, `system:dead-run`); a machine now records null (ADR-0118 D1),
+ * and the same pass nulls the stored ones. This pins what it moves, what it
+ * clears, what it leaves alone, that it is idempotent, and — end to end — that
+ * an in-flight multi-approver tally still counts the votes it had already
+ * collected.
  *
  * ## Why this file boots the real engine
  *
@@ -27,8 +30,8 @@ import { describe, it, expect, beforeEach, afterEach } from 'vitest';
 import { ObjectQL } from '@objectstack/objectql';
 import { SqlDriver } from '@objectstack/driver-sql';
 import type { EngineQueryOptions } from '@objectstack/spec/data';
-import { ApprovalService, SLA_ACTOR_ID, DEAD_RUN_ACTOR_ID } from './approval-service.js';
-import { backfillActionSlots, RESERVED_MACHINE_ACTORS } from './action-slot-backfill.js';
+import { ApprovalService, SLA_ACTOR_ID } from './approval-service.js';
+import { backfillActionSlots, LEGACY_MACHINE_SENTINELS } from './action-slot-backfill.js';
 import { SysApprovalRequest } from './sys-approval-request.object.js';
 import { SysApprovalAction } from './sys-approval-action.object.js';
 import { SysApprovalApprover } from './sys-approval-approver.object.js';
@@ -86,11 +89,14 @@ describe('the boot-time action-slot backfill (#21411)', () => {
     await engine.insert('crm_deal', { id: 'D1', title: 'Deal' }, { context: SYSTEM } as any);
   });
 
-  it('the reserved machine actors it leaves alone are the service\'s own sentinels', () => {
-    expect([...RESERVED_MACHINE_ACTORS].sort()).toEqual([SLA_ACTOR_ID, DEAD_RUN_ACTOR_ID].sort());
+  /** The value the dead-run sweep stored in `actor_id` before it recorded null. */
+  const DEAD_RUN_SENTINEL = 'system:dead-run';
+
+  it('the sentinels it clears are the two the machine sweeps stored — the SLA one is still the sweep\'s acting identity', () => {
+    expect([...LEGACY_MACHINE_SENTINELS].sort()).toEqual([SLA_ACTOR_ID, DEAD_RUN_SENTINEL].sort());
   });
 
-  it('moves slot literals out of actor_id, stamps the votes a pending tally counts, leaves everything else — and a second run writes nothing', async () => {
+  it('moves slot literals out of actor_id, nulls the machine sentinels, stamps the votes a pending tally counts, leaves everything else — and a second run writes nothing', async () => {
     // A request still collecting votes, opened by the real service: unanimous
     // over a position nobody holds (a literal slot) and two user-id slots.
     const open = await svc.openNodeRequest({
@@ -103,6 +109,10 @@ describe('the boot-time action-slot backfill (#21411)', () => {
     // and the slate the old tally then left.
     await legacy('aact_vote_literal', open.id, 'approve', 'position:finance');
     await legacy('aact_vote_user', open.id, 'approve', 'u9');
+    // A sentinel row on the still-pending request, in the very shape pass 2
+    // stamps (an approve at step 0): it took no slot, so it is cleared and
+    // never stamped as a vote.
+    await legacy('aact_sla_vote', open.id, 'approve', SLA_ACTOR_ID);
     await engine.update('sys_approval_request', { id: open.id, pending_approvers: 'u8' }, { context: SYSTEM } as any);
 
     // A finished request's history.
@@ -116,7 +126,7 @@ describe('the boot-time action-slot backfill (#21411)', () => {
     await legacy('aact_role', done, 'comment', 'role:finance');
     await legacy('aact_user_done', done, 'approve', 'u7');
     await legacy('aact_sla', done, 'escalate', SLA_ACTOR_ID);
-    await legacy('aact_dead', done, 'recall', DEAD_RUN_ACTOR_ID);
+    await legacy('aact_dead', done, 'recall', DEAD_RUN_SENTINEL);
     await legacy('aact_system', done, 'ooo_substitute', null);
     // A row the NEW writer wrote: already two facts.
     await engine.insert('sys_approval_action', {
@@ -125,9 +135,9 @@ describe('the boot-time action-slot backfill (#21411)', () => {
     }, { context: SYSTEM } as any);
 
     const first = await backfillActionSlots(engine as any, { pageSize: 2 });
-    // Pass 1 moved three literals (two finished, one pending); pass 2 stamped
-    // the one user-id vote still being counted.
-    expect(first).toEqual({ literalsMoved: 3, votesStamped: 1 });
+    // Pass 1 moved three literals (two finished, one pending) and nulled three
+    // sentinels; pass 2 stamped the one user-id vote still being counted.
+    expect(first).toEqual({ literalsMoved: 3, sentinelsCleared: 3, votesStamped: 1 });
 
     // Slot literals: the slot moves to acted_as, the person is unknown — null.
     expect(await recorded('aact_vote_literal')).toEqual([null, 'position:finance']);
@@ -138,14 +148,23 @@ describe('the boot-time action-slot backfill (#21411)', () => {
     // A finished request's user-id row: no guessed slot (it might be an
     // override that predates via_override). Found by its person instead.
     expect(await recorded('aact_user_done')).toEqual(['u7', null]);
-    // The machine sentinels, the system row and the new row: untouched.
-    expect(await recorded('aact_sla')).toEqual([SLA_ACTOR_ID, null]);
-    expect(await recorded('aact_dead')).toEqual([DEAD_RUN_ACTOR_ID, null]);
+    // The machine sentinels: null, and no slot — the sweep took none.
+    expect(await recorded('aact_sla')).toEqual([null, null]);
+    expect(await recorded('aact_dead')).toEqual([null, null]);
+    expect(await recorded('aact_sla_vote')).toEqual([null, null]);
+    // Their kind and comment are what say a sweep acted, and stay as stored.
+    expect((await action('aact_dead'))?.action).toBe('recall');
+    // The system row and the new row: untouched.
     expect(await recorded('aact_system')).toEqual([null, null]);
     expect(await recorded('aact_new')).toEqual(['u5', 'position:legal']);
+    // No sentinel is left anywhere in the lookup.
+    const leftover = (await engine.find('sys_approval_action', {
+      where: { actor_id: { $in: [...LEGACY_MACHINE_SENTINELS] } }, context: SYSTEM,
+    } satisfies EngineQueryOptions)) as any[];
+    expect(leftover).toEqual([]);
 
     // Idempotent: nothing left that either predicate matches.
-    expect(await backfillActionSlots(engine as any, { pageSize: 2 })).toEqual({ literalsMoved: 0, votesStamped: 0 });
+    expect(await backfillActionSlots(engine as any, { pageSize: 2 })).toEqual({ literalsMoved: 0, sentinelsCleared: 0, votesStamped: 0 });
 
     // ⭐ End to end: the last slot holder's approval finalizes the request,
     // because the tally counts the two votes the old writer recorded.