diff --git a/.changeset/21411-approval-actor-person.md b/.changeset/21411-approval-actor-person.md new file mode 100644 index 00000000000..be4e1b85e0e --- /dev/null +++ b/.changeset/21411-approval-actor-person.md @@ -0,0 +1,27 @@ +--- +'@objectstack/plugin-approvals': patch +--- + +An approval action now records the user who took it in `sys_approval_action.actor_id`, and the pending-approver slot it was taken as in a new `acted_as` column; rows stored before this move their slot out of `actor_id` at the next boot + +Clause-②: no + +`actor_id` is a lookup to `sys_user`, so under ADR-0118 D1 it holds a user id or nothing. A slot-gated action used to record the slot it took there instead: a `position:` literal for a position staffed after the request opened, or an email for a `user` approver authored as one. On those decisions no record named the person who decided. The audit ledger and activity rows the write produces carry no user, so the attribution was lost, and every join or report on the lookup silently dropped the row. + +**This supersedes the "What is recorded" sentence of the unreleased `21379-position-address-readers` changeset**, which says `actor_id` holds the slot. From this release it holds the person. + +- **What is recorded.** + - `actor_id` is the user the request's context vouches for: the signed-in caller, whatever address they named. + - `acted_as` is the slot the action took, in the slot's stored spelling (a user id, an email, or `position:`). It is empty on actions no slot admitted: the submitter's own actions, system actions, and an admin override, which `via_override` still marks. + - An emailed action link records the one account that carries the token's email. If no account carries it, the link records no person. + - The SLA sweep keeps its reserved `system:sla` actor for now. +- **What reads it.** + - The multi-approver tally and `decision_progress` count `acted_as`. + - A participant who already acted keeps sight of a request by either of two facts: `actor_id` is their user id, or `acted_as` is a slot they act under (so a decision taken as `position:` stays visible to that position's holders). + - Nothing compares a slot with `actor_id` any more. + - The action log (`GET /api/v1/approvals/requests/:id/actions`, `listActions`) returns `acted_as` beside `actor_id` and `actor_name`, filling the `ApprovalActionRow.acted_as` member `@objectstack/spec` declares. It is omitted when the action took no slot, or when no stored record kept the slot. +- **Stored rows.** A repair runs on every boot and is idempotent. + - Pass 1: a row whose `actor_id` still holds a slot address gets `acted_as` set to it and `actor_id` cleared. No stored record names who decided it, so it shows the slot and no person. + - Pass 2: the approve votes a still-pending request's tally counts get their `acted_as`, so in-flight `unanimous`, `quorum` and `per_group` requests keep the approvals they already collected. + - A failure is logged at error level and retried at the next boot. +- **For a report or integration that read `actor_id` as the slot:** read `acted_as` instead. `actor_id` now always joins to `sys_user`. diff --git a/content/docs/automation/approvals.mdx b/content/docs/automation/approvals.mdx index 614a59a6829..456f95e0529 100644 --- a/content/docs/automation/approvals.mdx +++ b/content/docs/automation/approvals.mdx @@ -509,9 +509,10 @@ opened, once someone is staffed into it). A named `actorId` must be one of those identities, and a position named under either spelling takes that position's slot. No such slot and no admin override returns 403 (`FORBIDDEN: actor '…' is not a pending approver`); a request that isn't pending returns 409 -(`INVALID_STATE`). The decision is recorded in `sys_approval_action.actor_id` -under the slot it took, in that slot's stored spelling — the multi-approver -tally counts approvals by matching that value against the slate. Always go through +(`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 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 diff --git a/content/docs/permissions/tenant-audit-census.mdx b/content/docs/permissions/tenant-audit-census.mdx index 1c6b642827b..f0da07d96e7 100644 --- a/content/docs/permissions/tenant-audit-census.mdx +++ b/content/docs/permissions/tenant-audit-census.mdx @@ -122,7 +122,7 @@ are reported as `undecidable` rather than assumed either way. The same holds twice over for the context. An options argument spelled as a literal can be read; one spelled `options`, `{ ...opts }`, or handed through a -forwarding shim cannot, and **67 of the 229 sites are spelled that way**. A +forwarding shim cannot, and **67 of the 231 sites are spelled that way**. A context resolved from an inline literal or a local `const` can be tested for `isSystem`; one arriving from a helper call cannot. @@ -187,10 +187,10 @@ reproduce them. Where it disagrees, it disagrees on the page: | carried figure | where it survives | this census | | :--- | :--- | ---: | -| 175 write call sites | quoted in the merged changeset | **229** | +| 175 write call sites | quoted in the merged changeset | **231** | | 24 carrying no tenant context | quoted in the merged changeset | **9** provable and tenancy-enabled; **34** more whose options argument is unreadable | -| 127 of 175 statically decidable, 48 runtime-parameter-name sites | restated on the `isSystem`-scoping card | **152 of 229** decidable, **77** undecidable | -| 135 (77%) silenced by the `isSystem` guard before the posture gate | the lost issue body — **no surviving corroboration** | **not reproduced**: 110 decidably elevated, 0 decidably not, 102 undecidable | +| 127 of 175 statically decidable, 48 runtime-parameter-name sites | restated on the `isSystem`-scoping card | **154 of 231** decidable, **77** undecidable | +| 135 (77%) silenced by the `isSystem` guard before the posture gate | the lost issue body — **no surviving corroboration** | **not reproduced**: 112 decidably elevated, 0 decidably not, 102 undecidable | | 141 and 132, two independent re-derivations | the card that filed this work | — | **The differences are not reconciled, and deliberately so.** The old census's @@ -207,11 +207,11 @@ would report a smaller number and would not say so. The fourth row is the one worth flagging to anyone citing it. **The 135 / 77% figure has no surviving corroboration anywhere in the tree.** This census reads -110 of 229 (48%) as decidably elevated, with 102 more whose elevation is a +112 of 231 (48%) as decidably elevated, with 102 more whose elevation is a run-time fact — so the claim is neither confirmed nor refuted, and the honest answer is that a static reading cannot settle it. -⇒ **Cite `9 / 229`, and say what it is**: the sites whose options argument was +⇒ **Cite `9 / 231`, and say what it is**: the sites whose options argument was READ and holds no tenant context, against a decidably tenancy-enabled object. That is the control's provable yield surface. ⛔ Do not cite it as "the sites without tenant context" — **34 further sites** have an options argument this @@ -223,28 +223,28 @@ cannot read, and they are neither in nor out. | what | count | | :--- | ---: | -| write call sites on the application surface | **229** | -| …whose object name is statically decidable | 152 | +| write call sites on the application surface | **231** | +| …whose object name is statically decidable | 154 | | …whose object name is chosen at run time | 77 | -| …against an object with tenancy ENABLED | 151 | +| …against an object with tenancy ENABLED | 153 | | …against an object that declares tenancy off | 1 | -| threading a tenant context | 145 | +| threading a tenant context | 147 | | PROVABLY carrying none (options read, no context key) | **17** | | …of those, against a decidably tenancy-enabled object | **9** | | options argument UNREADABLE — may or may not carry one | 67 | | …of those, against a decidably tenancy-enabled object | 34 | -| threading a decidably ELEVATED (`isSystem`) context | 110 | +| threading a decidably ELEVATED (`isSystem`) context | 112 | | threading a context that is decidably NOT elevated | 0 | | threading a context whose elevation is a run-time fact | 102 | | how the instrument reached the site | count | | :--- | ---: | -| receiver carried a readable engine type | 181 | +| receiver carried a readable engine type | 183 | | receiver erased, placed by the object NAME | 28 | | receiver erased, placed by an `object: string` PARAMETER | 15 | | receiver erased, placed by an `UNTYPED_RECEIVERS` row | 5 | -| object name spelled inline | 101 | +| object name spelled inline | 103 | | object name spelled through a `const` | 51 | | object name is an `object: string` parameter | 17 | | object name is some other run-time expression | 60 | @@ -297,13 +297,13 @@ holds still. They are required to be HERE and to say WHEN they were true; their values are not compared. The reasoning, and the measurement behind it, are in `scripts/check-tenant-audit-census.mjs`. -Measured on 2026-10-02 at `c41817b12`. +Measured on 2026-10-02 at `b668cf134`. | corpus scale (not enforced) | count | | :--- | ---: | -| tracked non-test sources scanned | 599 | -| engine-shaped types recognised | 66 | +| tracked non-test sources scanned | 602 | +| engine-shaped types recognised | 67 | | declared objects in the registry | 116 | -| same-named calls subtracted as non-engine | 150 | +| same-named calls subtracted as non-engine | 152 | {/* END GENERATED: tenant-audit-census */} diff --git a/docs/audits/2026-08-tenant-audit-write-call-sites.counts.md b/docs/audits/2026-08-tenant-audit-write-call-sites.counts.md index 69d9ce0a7f2..9aea141c009 100644 --- a/docs/audits/2026-08-tenant-audit-write-call-sites.counts.md +++ b/docs/audits/2026-08-tenant-audit-write-call-sites.counts.md @@ -33,17 +33,17 @@ silent, and `node scripts/tenant-audit-census.mjs --write` is the resolution. | Measure | Value | |---|---:| -| Write call sites | 229 | -| Object name statically decidable | 152 | +| Write call sites | 231 | +| Object name statically decidable | 154 | | Object name chosen at run time | 77 | -| Against a tenancy-enabled object | 151 | +| Against a tenancy-enabled object | 153 | | Against an object declaring tenancy off | 1 | -| Threading a tenant context | 145 | +| Threading a tenant context | 147 | | Provably carrying none | 17 | | …and decidably tenancy-enabled | 9 | | Options argument unreadable | 67 | | …and decidably tenancy-enabled | 34 | -| Threading a decidably elevated context | 110 | +| Threading a decidably elevated context | 112 | | Threading a decidably non-elevated context | 0 | | Threading a context of undecidable elevation | 102 | @@ -90,14 +90,14 @@ holds still. They are required to be HERE and to say WHEN they were true; their values are not compared. The reasoning, and the measurement behind it, are in `scripts/check-tenant-audit-census.mjs`. -Measured on 2026-10-02 at `c41817b12`. +Measured on 2026-10-02 at `b668cf134`. | corpus scale (not enforced) | count | | :--- | ---: | -| tracked non-test sources scanned | 599 | -| engine-shaped types recognised | 66 | +| tracked non-test sources scanned | 602 | +| engine-shaped types recognised | 67 | | declared objects in the registry | 116 | -| same-named calls subtracted as non-engine | 150 | +| same-named calls subtracted as non-engine | 152 | ## Every site @@ -105,6 +105,7 @@ Measured on 2026-10-02 at `c41817b12`. |---|---|---|---|---|---:| | `packages/plugins/organizations/src/claim-org-seed-ownership.ts` | `update` | `schema.name` | undecidable | elevated | 1 | | `packages/plugins/organizations/src/claim-orphan-org-rows.ts` | `update` | `schema.name` | undecidable | elevated | 1 | +| `packages/plugins/plugin-approvals/src/action-slot-backfill.ts` | `update` | `sys_approval_action` | enabled | elevated | 2 | | `packages/plugins/plugin-approvals/src/approval-service.ts` | `update` | `object` | undecidable | context, elevation undecidable | 1 | | `packages/plugins/plugin-approvals/src/approval-service.ts` | `insert` | `sys_approval_action` | enabled | elevated | 14 | | `packages/plugins/plugin-approvals/src/approval-service.ts` | `delete` | `sys_approval_approver` | enabled | elevated | 2 | diff --git a/packages/plugins/plugin-approvals/src/action-slot-backfill-wiring.test.ts b/packages/plugins/plugin-approvals/src/action-slot-backfill-wiring.test.ts new file mode 100644 index 00000000000..8383fead566 --- /dev/null +++ b/packages/plugins/plugin-approvals/src/action-slot-backfill-wiring.test.ts @@ -0,0 +1,86 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #21411 — the action-slot backfill is WIRED: `ApprovalsServicePlugin.start()` + * registers it on `kernel:ready`, hands it the plugin's own engine, and logs a + * failure at `error`. + * + * `action-slot-backfill.integration.test.ts` proves what the repair does. This + * file proves it runs at all — a repair module nothing calls is code `grep` + * finds and production never executes, and every slot reader would then miss + * the rows it exists to move, silently. The module is replaced by a spy so the + * assertion is about the call, not about the repair. + */ + +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +const backfill = vi.hoisted(() => ({ + fn: vi.fn(async (_engine: unknown) => ({ literalsMoved: 0, votesStamped: 0 })), +})); + +vi.mock('./action-slot-backfill.js', async (importOriginal) => ({ + ...(await importOriginal()), + backfillActionSlots: backfill.fn, +})); + +import { ApprovalsServicePlugin } from './approvals-plugin.js'; + +function fakeContext(engine: unknown) { + const hooks: Record Promise | void>> = {}; + const logged = { info: [] as string[], warn: [] as string[], error: [] as string[] }; + const ctx: any = { + hook: (name: string, fn: () => Promise | void) => { (hooks[name] ??= []).push(fn); }, + getService: (name: string) => { + if (name === 'objectql') return engine; + throw new Error(`no service '${name}'`); + }, + registerService: () => {}, + logger: { + info: (msg: string) => { logged.info.push(String(msg)); }, + warn: (msg: string) => { logged.warn.push(String(msg)); }, + error: (msg: string) => { logged.error.push(String(msg)); }, + debug: () => {}, + }, + }; + const fire = async (name: string) => { for (const fn of hooks[name] ?? []) await fn(); }; + return { ctx, fire, logged }; +} + +/** + * Enough engine for `start()` with `disableAutoHooks`: the service only needs + * an object to hold, and the one boot-time read that reaches it (the + * approver-index rebuild) finds nothing. The repair itself is the spy above, + * so nothing here writes — and a double with no write verbs makes no claim + * about how writes dispatch. + */ +function fakeEngine() { + return { find: async () => [] }; +} + +describe('the action-slot backfill is wired on kernel:ready (#21411)', () => { + beforeEach(() => { backfill.fn.mockClear(); }); + + it('runs once at kernel:ready, never before, against the plugin\'s own engine', async () => { + const engine = fakeEngine(); + const { ctx, fire } = fakeContext(engine); + await new ApprovalsServicePlugin({ disableAutoHooks: true }).start(ctx); + + expect(backfill.fn, 'not at start(): the registries are still filling').not.toHaveBeenCalled(); + await fire('kernel:ready'); + expect(backfill.fn).toHaveBeenCalledTimes(1); + expect(backfill.fn.mock.calls[0][0]).toBe(engine); + }); + + 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()); + await new ApprovalsServicePlugin({ disableAutoHooks: true }).start(ctx); + await fire('kernel:ready'); + + const line = logged.error.find((m) => m.includes('action-slot backfill failed')); + expect(line, `error lines: ${JSON.stringify(logged.error)}`).toBeDefined(); + expect(line).toMatch(/multi-approver tallies/); + expect(line).toMatch(/restart/); + expect(logged.warn.some((m) => m.includes('action-slot backfill'))).toBe(false); + }); +}); diff --git a/packages/plugins/plugin-approvals/src/action-slot-backfill.integration.test.ts b/packages/plugins/plugin-approvals/src/action-slot-backfill.integration.test.ts new file mode 100644 index 00000000000..abe34798eea --- /dev/null +++ b/packages/plugins/plugin-approvals/src/action-slot-backfill.integration.test.ts @@ -0,0 +1,156 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #21411 — the boot-time action-slot backfill, on the real engine. + * + * `sys_approval_action.actor_id` used to hold the SLOT a slot-gated action took + * — a `position:

` literal, an email — so on those decisions no record named + * 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. + * + * ## Why this file boots the real engine + * + * The repair is a set of driver-side predicates (`$contains` on a lookup + * column, `$nin`, `$and` / `$or`, the keyset seek) plus writes of a column the + * DDL must actually carry. A fake engine would be a matcher written by the same + * author as the assertion, deciding the one thing this file exists to measure. + * So: a real `ObjectQL` over `@objectstack/driver-sql` + better-sqlite3 + * `:memory:`, the real `sys_approval_*` schemas synced through the real DDL, + * and the real `ApprovalService` opening and deciding the request. + */ + +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 { SysApprovalRequest } from './sys-approval-request.object.js'; +import { SysApprovalAction } from './sys-approval-action.object.js'; +import { SysApprovalApprover } from './sys-approval-approver.object.js'; +import { SysApprovalDelegation } from './sys-approval-delegation.object.js'; + +const SYSTEM = { isSystem: true, positions: [], permissions: [] } as any; +const SUBMITTER = { userId: 'u_submitter', positions: [], permissions: [] } as any; +const asUser = (userId: string) => ({ userId, positions: [], permissions: [] }) as any; + +const deal = { + name: 'crm_deal', + label: 'Deal', + fields: { + id: { name: 'id', label: 'Id', type: 'text' as const, primaryKey: true }, + title: { name: 'title', label: 'Title', type: 'text' as const }, + }, +}; + +describe('the boot-time action-slot backfill (#21411)', () => { + let engine: ObjectQL; + let svc: ApprovalService; + + const action = async (id: string) => + ((await engine.find('sys_approval_action', { + where: { id }, context: SYSTEM, + } satisfies EngineQueryOptions)) as any[])[0]; + const recorded = async (id: string) => { + const row = await action(id); + return [row?.actor_id ?? null, row?.acted_as ?? null]; + }; + /** A row exactly as the pre-`acted_as` writer stored it. */ + const legacy = (id: string, requestId: string, act: string, actorId: string | null) => + engine.insert('sys_approval_action', { + id, request_id: requestId, step_name: 'review', step_index: 0, action: act, + actor_id: actorId, created_at: '2026-09-01T00:00:00.000Z', + }, { context: SYSTEM } as any); + + afterEach(async () => { + try { await engine?.destroy(); } catch { /* noop */ } + }); + + beforeEach(async () => { + engine = new ObjectQL(); + engine.registerDriver(new SqlDriver({ + client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true, + }), true); + await engine.init(); + for (const def of [deal, SysApprovalRequest, SysApprovalAction, SysApprovalApprover, SysApprovalDelegation]) { + engine.registry.registerObject(def as any, 'approvals-test', 'approvals-test'); + } + // Real DDL — including the `acted_as` column, so a write of it the schema + // does not carry fails here rather than passing against a permissive fake. + await engine.syncSchemas(); + svc = new ApprovalService({ engine: engine as any, logger: { warn: () => {} } as any }); + 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()); + }); + + 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 () => { + // 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({ + object: 'crm_deal', recordId: 'D1', runId: 'run_1', nodeId: 'review', flowName: 'deal_review', + config: { approvers: [{ type: 'position', value: 'finance' }, { type: 'user', value: 'u9' }, { type: 'user', value: 'u8' }], behavior: 'unanimous' } as any, + record: { id: 'D1', title: 'Deal' }, + }, SUBMITTER) as any; + expect(open.pending_approvers).toEqual(['position:finance', 'u9', 'u8']); + // Two votes the OLD writer recorded — each under its slot, in actor_id — + // 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'); + await engine.update('sys_approval_request', { id: open.id, pending_approvers: 'u8' }, { context: SYSTEM } as any); + + // A finished request's history. + const done = 'areq_done'; + await engine.insert('sys_approval_request', { + id: done, process_name: 'flow:deal_review', object_name: 'crm_deal', record_id: 'D1', + submitter_id: 'u_submitter', status: 'approved', current_step: 'review', current_step_index: 0, + created_at: '2026-09-01T00:00:00.000Z', updated_at: '2026-09-01T00:00:00.000Z', + }, { context: SYSTEM } as any); + await legacy('aact_email', done, 'approve', 'mail.reviewer@example.com'); + 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_system', done, 'ooo_substitute', null); + // A row the NEW writer wrote: already two facts. + await engine.insert('sys_approval_action', { + id: 'aact_new', request_id: done, step_index: 0, action: 'approve', + actor_id: 'u5', acted_as: 'position:legal', created_at: '2026-09-02T00:00:00.000Z', + }, { 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 }); + + // Slot literals: the slot moves to acted_as, the person is unknown — null. + expect(await recorded('aact_vote_literal')).toEqual([null, 'position:finance']); + expect(await recorded('aact_email')).toEqual([null, 'mail.reviewer@example.com']); + expect(await recorded('aact_role')).toEqual([null, 'role:finance']); + // The pending tally's user-id vote: the person stays, the slot is stamped. + expect(await recorded('aact_vote_user')).toEqual(['u9', 'u9']); + // 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]); + expect(await recorded('aact_system')).toEqual([null, null]); + expect(await recorded('aact_new')).toEqual(['u5', 'position:legal']); + + // Idempotent: nothing left that either predicate matches. + expect(await backfillActionSlots(engine as any, { pageSize: 2 })).toEqual({ literalsMoved: 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. + const last = await svc.decideNode(open.id, { decision: 'approve' } as any, asUser('u8')); + expect(last.finalized).toBe(true); + expect(last.request.status).toBe('approved'); + }); +}); diff --git a/packages/plugins/plugin-approvals/src/action-slot-backfill.ts b/packages/plugins/plugin-approvals/src/action-slot-backfill.ts new file mode 100644 index 00000000000..74c100ffbfa --- /dev/null +++ b/packages/plugins/plugin-approvals/src/action-slot-backfill.ts @@ -0,0 +1,180 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * action-slot-backfill — the boot-time repair that moves stored SLOT + * addresses out of `sys_approval_action.actor_id` and into `acted_as`. + * + * ## Why rows need moving + * + * `actor_id` is a `sys_user` lookup, and ADR-0118 D1 says such a column holds + * an id or nothing — never a sentinel, never any other non-id value. Until the + * slot got its own column, a slot-gated action recorded the SLOT it took in + * `actor_id`: a `position:

` literal for a position nobody held when the + * request opened, an email for a `user` approver authored as one. On those rows + * the person who decided is on no record at all — measured on a booted app, + * not one of the rows a decision writes names them — and every join or report + * on the lookup silently drops the row. + * + * The writers now record the person in `actor_id` and the slot in `acted_as`, + * and every slot reader (the multi-approver tally, `decision_progress`, the + * slot half of the already-acted probe) reads `acted_as` with NO fallback to + * `actor_id`. So rows written before that have to be moved, or those readers + * would not see them. + * + * ## The two passes + * + * 1. **Slot literals, the whole history.** A row whose `actor_id` holds an + * address — it contains `:` (a `type:value` literal) or `@` (an email) — gets + * `acted_as := actor_id` and `actor_id := null`. `null` is ADR-0118's own + * value for "no person recorded", and it is the honest one: no stored record + * names the decider of such a row, and an email maps to a person only + * through whichever account carries it TODAY, which is a guess. The + * reserved machine actors ({@link RESERVED_MACHINE_ACTORS}) also contain + * `:` and are left alone; they are a separate ADR-0118 D1 debt. + * + * 2. **Votes still being counted.** An `approve` row at `step_index` 0 of a + * request that is still `pending`, with no `acted_as`, gets + * `acted_as := actor_id`. This is exact, not a guess: an approval on a + * still-pending request can only be a non-finalizing slot vote (an admin + * override finalizes the node), and the old writer recorded such a vote + * under the slot it took. Without this pass an in-flight `unanimous` / + * `quorum` / `per_group` request would drop the approvals it has already + * collected, and their slots would reappear pending. + * + * Nothing else is stamped. A historical user-id row on a finished request keeps + * the person it always held in `actor_id` and gets no `acted_as`: which of + * those rows were slot votes cannot be told apart from an admin override that + * predates `via_override`, so writing one would be a guess. The already-acted + * probe reads such rows by their person (`actor_id` = the caller's user id), + * which is a fact, not a fallback. + * + * ## Idempotent, and run at every boot + * + * Wired on `kernel:ready` beside the approver-index rebuild, for the same + * reason that one is: the readers move off `actor_id` in the same release, so + * a repair that waited for an operator would leave a window in which tallies + * and visibility are wrong. Pass 1 writes the column its own predicate reads, + * and pass 2 fills the column it requires empty, so a second run over an + * unchanged database writes nothing — `action-slot-backfill.integration.test.ts` + * asserts exactly that. Pass 1 costs one substring scan of the action table + * per boot; pass 2 is bounded by the pending queue. + */ + +import { keysetWalk } from '@objectstack/types'; + +/** + * The engine surface the repair needs — a structural subset of + * `ApprovalEngine`, so the module can be driven by the real ObjectQL engine in + * its pin without pulling the service in. + */ +export interface ActionSlotBackfillEngine { + find(object: string, options?: unknown): Promise; + update(object: string, data: unknown, options?: unknown): Promise; +} + +/** + * Machine actors that live in `actor_id` by name and are not slots: the SLA + * sweep's and the dead-run sweep's sentinels. Spelled here rather than imported + * from the service so this module stays importable on its own; the pin holds + * them equal to the service's exports. + */ +export const RESERVED_MACHINE_ACTORS: readonly string[] = ['system:sla', 'system:dead-run']; + +/** What one run wrote. */ +export interface ActionSlotBackfillResult { + /** Pass 1: rows whose slot literal moved from `actor_id` to `acted_as`. */ + literalsMoved: number; + /** Pass 2: still-counted approve votes that got their `acted_as`. */ + votesStamped: number; +} + +const SYSTEM_CONTEXT = { isSystem: true, positions: [], permissions: [] }; +const PAGE = 500; + +function nonEmpty(value: unknown): string | null { + return typeof value === 'string' && value.length > 0 ? value : null; +} + +/** Does this stored `actor_id` hold a slot ADDRESS rather than a user id? */ +export function isSlotLiteral(actorId: string): boolean { + if (RESERVED_MACHINE_ACTORS.includes(actorId)) return false; + return actorId.includes(':') || actorId.includes('@'); +} + +/** + * Run both passes. Throws on a failed read or write — the caller logs it, and + * the next boot retries: every write is keyed by id and fills exactly the + * column its predicate tests, so a partial run completes on the next one. + */ +export async function backfillActionSlots( + engine: ActionSlotBackfillEngine, + options: { pageSize?: number } = {}, +): Promise { + const pageSize = options.pageSize ?? PAGE; + let literalsMoved = 0; + let votesStamped = 0; + + // Pass 1 — slot literals in `actor_id`, the whole history. A seek walk, not + // an offset walk: every page moves rows OUT of the predicate it pages over. + const literals = keysetWalk>( + (q) => engine.find('sys_approval_action', { + ...q, + fields: ['id', 'actor_id', 'acted_as'], + context: SYSTEM_CONTEXT, + }) as Promise[]>, + { + where: { + $and: [ + { $or: [{ actor_id: { $contains: ':' } }, { actor_id: { $contains: '@' } }] }, + { actor_id: { $nin: [...RESERVED_MACHINE_ACTORS] } }, + ], + }, + pageSize, + }, + ); + for await (const rows of literals.pages()) { + for (const row of rows) { + const id = nonEmpty(row.id); + const actorId = nonEmpty(row.actor_id); + if (!id || !actorId || !isSlotLiteral(actorId)) continue; + const patch: Record = { id, actor_id: null }; + if (!nonEmpty(row.acted_as)) patch.acted_as = actorId; + await engine.update('sys_approval_action', patch, { context: SYSTEM_CONTEXT }); + literalsMoved++; + } + } + + // Pass 2 — the votes a pending request's tally still counts. + const pending = keysetWalk>( + (q) => engine.find('sys_approval_request', { + ...q, + fields: ['id'], + context: SYSTEM_CONTEXT, + }) as Promise[]>, + { where: { status: 'pending' }, pageSize }, + ); + for await (const requests of pending.pages()) { + const ids = requests.map((r) => nonEmpty(r.id)).filter((id): id is string => id !== null); + if (!ids.length) continue; + const votes = keysetWalk>( + (q) => engine.find('sys_approval_action', { + ...q, + fields: ['id', 'actor_id', 'acted_as'], + context: SYSTEM_CONTEXT, + }) as Promise[]>, + { where: { request_id: { $in: ids }, action: 'approve', step_index: 0 }, pageSize }, + ); + for await (const rows of votes.pages()) { + for (const row of rows) { + const id = nonEmpty(row.id); + const actorId = nonEmpty(row.actor_id); + if (!id || !actorId || nonEmpty(row.acted_as)) continue; + if (RESERVED_MACHINE_ACTORS.includes(actorId)) continue; + await engine.update('sys_approval_action', { id, acted_as: actorId }, { context: SYSTEM_CONTEXT }); + votesStamped++; + } + } + } + + return { literalsMoved, votesStamped }; +} diff --git a/packages/plugins/plugin-approvals/src/approval-service.test.ts b/packages/plugins/plugin-approvals/src/approval-service.test.ts index 75d476ecaac..ee82b869749 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.test.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.test.ts @@ -11,7 +11,7 @@ import { describe, it, expect, beforeEach, vi } from 'vitest'; import { APPROVAL_REVISE_NODE_TYPE } from '@objectstack/spec/automation'; -import { ApprovalService, REMIND_COOLDOWN_MS, ESCALATION_ENABLED_FLIP_CUTOFF_MS } from './approval-service.js'; +import { ApprovalService, REMIND_COOLDOWN_MS, ESCALATION_ENABLED_FLIP_CUTOFF_MS, SLA_ACTOR_ID } from './approval-service.js'; import { bindApprovalLockHook, bindDelegationWriteGuard, unbindAllHooks } from './lifecycle-hooks.js'; interface FakeRow { [k: string]: any } @@ -1310,7 +1310,9 @@ describe('ApprovalService (node era)', () => { { id: 'u9', name: 'Grace Hopper', email: 'grace@example.com' }, ]; const req = await svc.openNodeRequest(openInput(['u9']), CTX); - await svc.decideNode(req.id, { decision: 'approve', actorId: 'u9' }, SYS); + // Decided by u9 THEMSELF: `actor_id` records the person the context + // vouches for (#21411), and a bare system context vouches for nobody. + await svc.decideNode(req.id, { decision: 'approve', actorId: 'u9' }, asUser('u9')); const actions = await svc.listActions(req.id, SYS); expect(actions.map(a => (a as any).actor_name)).toEqual(['Ada Lovelace', 'Grace Hopper']); }); @@ -3809,7 +3811,7 @@ describe('ApprovalService — every slot reader takes the acting addresses (#213 } }); - it('decision slot test: the default actor and BOTH spellings take the position slot; the decision records the slot\'s stored spelling', async () => { + it('decision slot test: the default actor and BOTH spellings take the position slot; the decision records the person in actor_id and the slot\'s stored spelling in acted_as', async () => { const engine = makeFakeEngine(); const svc = svcFor(engine); for (const actorId of [undefined, 'role:sales_manager', 'position:sales_manager']) { @@ -3818,14 +3820,18 @@ describe('ApprovalService — every slot reader takes the acting addresses (#213 expect(out.finalized, `actor ${actorId ?? '(default)'}`).toBe(true); expect(out.request.status).toBe('approved'); const [act] = actionsOf(engine, req.id, 'approve'); - expect(act.actor_id, `recorded for actor ${actorId ?? '(default)'}`).toBe(SLOT); - expect(act.via_override).toBe(false); + // #21411: two facts, two columns — whoever named what. + expect( + [act.actor_id, act.acted_as, act.via_override], + `recorded for actor ${actorId ?? '(default)'}`, + ).toEqual(['u_holder', SLOT, false]); } // A 15.x-era slot keeps its own spelling, under the default actor too. const legacy = await open(svc, [{ type: 'role', value: 'sales_manager' }]); expect(legacy.pending_approvers).toEqual(['role:sales_manager']); await svc.decideNode(legacy.id, { decision: 'reject' } as any, HOLDER); - expect(actionsOf(engine, legacy.id, 'reject')[0].actor_id).toBe('role:sales_manager'); + const [rejected] = actionsOf(engine, legacy.id, 'reject'); + expect([rejected.actor_id, rejected.acted_as]).toEqual(['u_holder', 'role:sales_manager']); // This widens nobody: holding A position is not holding THIS one. const req = await open(svc, toPosition); @@ -3850,12 +3856,14 @@ describe('ApprovalService — every slot reader takes the acting addresses (#213 await expect(svc.requestInfo(req.id, { comment: 'why?' } as any, BYSTANDER)) .rejects.toThrow("FORBIDDEN: actor 'u_bystander' is not a pending approver"); await svc.requestInfo(req.id, { comment: 'why?' } as any, HOLDER); - expect(actionsOf(engine, req.id, 'request_info')[0].actor_id).toBe(SLOT); + const [asked] = actionsOf(engine, req.id, 'request_info'); + expect([asked.actor_id, asked.acted_as]).toEqual(['u_holder', SLOT]); await expect(svc.comment(req.id, { comment: 'hi' } as any, BYSTANDER)) .rejects.toThrow("FORBIDDEN: actor 'u_bystander' is not on this request"); await svc.comment(req.id, { comment: 'hi' } as any, HOLDER); - expect(actionsOf(engine, req.id, 'comment')[0].actor_id).toBe(SLOT); + const [said] = actionsOf(engine, req.id, 'comment'); + expect([said.actor_id, said.acted_as]).toEqual(['u_holder', SLOT]); await expect(svc.reassign(req.id, { to: 'u_next' } as any, BYSTANDER)) .rejects.toThrow("FORBIDDEN: 'u_bystander' is not a pending approver on this request"); @@ -3863,7 +3871,9 @@ describe('ApprovalService — every slot reader takes the acting addresses (#213 const moved = await svc.reassign(req.id, { to: 'u_next' } as any, HOLDER); expect(moved.request.pending_approvers).toEqual(['u_next']); const [reassigned] = actionsOf(engine, req.id, 'reassign'); - expect([reassigned.actor_id, reassigned.reassign_from, reassigned.via_override]).toEqual([SLOT, SLOT, false]); + // `reassign_from` still holds the slot handed over: that column is #21455's. + expect([reassigned.actor_id, reassigned.acted_as, reassigned.reassign_from, reassigned.via_override]) + .toEqual(['u_holder', SLOT, SLOT, false]); }); it('decision slot test feeds the multi-approver tally: a default-actor approval consumes exactly the position slot', async () => { @@ -3917,7 +3927,8 @@ describe('ApprovalService — every slot reader takes the acting addresses (#213 const out = await svc.decideNode(req.id, { decision: 'approve' } as any, MAIL); expect(out.finalized).toBe(true); - expect(actionsOf(engine, req.id, 'approve')[0].actor_id).toBe('mail.reviewer@example.com'); + const [act] = actionsOf(engine, req.id, 'approve'); + expect([act.actor_id, act.acted_as]).toEqual(['u_mail', 'mail.reviewer@example.com']); expect(await svc.getRequest(req.id, MAIL), 'after deciding').not.toBeNull(); }); @@ -3928,7 +3939,200 @@ describe('ApprovalService — every slot reader takes the acting addresses (#213 expect(req.pending_approvers).toEqual(['u_holder', SLOT]); const out = await svc.decideNode(req.id, { decision: 'approve' } as any, HOLDER); expect(out.request.pending_approvers).toEqual([SLOT]); - expect(actionsOf(engine, req.id, 'approve')[0].actor_id).toBe('u_holder'); + const [act] = actionsOf(engine, req.id, 'approve'); + expect([act.actor_id, act.acted_as]).toEqual(['u_holder', 'u_holder']); + }); +}); + +// ── The person in `actor_id`, the slot in `acted_as` (#21411) ────────── +// +// `actor_id` is a `sys_user` lookup (ADR-0118 D1: an id or nothing), but a +// slot-gated action recorded the SLOT it took there — a `position:

` literal, +// an email. Measured on a booted app, not one of the rows a position-slot +// decision writes named the person who decided. Triage ruled the person into +// `actor_id` and the slot into a column of its own; these pin the three slot +// kinds through every door that decides, what the readers then read, and the +// two doors that record nobody. +describe('ApprovalService — actor_id records the person, acted_as the slot (#21411)', () => { + const svcFor = (engine: any) => { + let n = 0; + return new ApprovalService({ engine, clock: { now: () => new Date(1757500000000 + (n++) * 1000) } }); + }; + const holding = (userId: string, positions: unknown[], extra: Record = {}) => + ({ userId, tenantId: 't1', positions, permissions: [], ...extra }) as any; + const HOLDER = holding('u_holder', ['sales_manager']); + const BYSTANDER = holding('u_bystander', ['finance']); + const ADMIN = holding('root', ['sales_manager'], { permissions: ['admin_full_access'] }); + const SLOT = 'position:sales_manager'; + const toPosition = [{ type: 'position', value: 'sales_manager' }]; + + let recordSeq = 0; + const open = async (svc: ApprovalService, approvers: any[], behavior = 'first_response') => { + const recordId = `deal_${++recordSeq}`; + const out = await svc.openNodeRequest({ + object: 'opportunity', recordId, runId: `run_a${recordSeq}`, nodeId: 'approve_step', + flowName: 'deal_approval', + config: { approvers, behavior: behavior as any }, + record: { id: recordId, amount: 100 }, + }, CTX); + if (!('id' in out)) throw new Error('scene did not open: the empty slate auto-approved'); + return out; + }; + const actionsOf = (engine: any, requestId: string, action: string) => + (engine._tables['sys_approval_action'] ?? []).filter((a: any) => a.request_id === requestId && a.action === action); + const recorded = (row: any) => [row.actor_id ?? null, row.acted_as ?? null]; + + it('the three slot kinds, through the session door: position, email and user id', async () => { + const engine = makeFakeEngine(); + await engine.insert('sys_user', { id: 'u_mail', email: 'mail.reviewer@example.com' }); + const svc = svcFor(engine); + + // Position — named, exactly the card's call. + const pos = await open(svc, toPosition); + await svc.decideNode(pos.id, { decision: 'approve', actorId: SLOT }, HOLDER); + expect(recorded(actionsOf(engine, pos.id, 'approve')[0])).toEqual(['u_holder', SLOT]); + + // Email — the default actor takes the slot its own account's email keys. + const mail = await open(svc, [{ type: 'user', value: 'mail.reviewer@example.com' }]); + await svc.decideNode(mail.id, { decision: 'approve' } as any, holding('u_mail', [])); + expect(recorded(actionsOf(engine, mail.id, 'approve')[0])).toEqual(['u_mail', 'mail.reviewer@example.com']); + + // User id — the slot IS the person's id, so both columns carry it. + const user = await open(svc, [{ type: 'user', value: 'u9' }]); + await svc.decideNode(user.id, { decision: 'approve' } as any, asUser('u9')); + expect(recorded(actionsOf(engine, user.id, 'approve')[0])).toEqual(['u9', 'u9']); + }); + + it('the action link: a user-id token records that user, an email-bound token the one account carrying it — or nobody', async () => { + const engine = makeFakeEngine(); + await engine.insert('sys_user', { id: 'u_mail', email: 'mail.reviewer@example.com' }); + const svc = svcFor(engine); + + const user = await open(svc, [{ type: 'user', value: 'u9' }]); + await svc.redeemActionToken((await svc.issueActionTokens(user.id, 'u9')).approve); + expect(recorded(actionsOf(engine, user.id, 'approve')[0])).toEqual(['u9', 'u9']); + + const mail = await open(svc, [{ type: 'user', value: 'mail.reviewer@example.com' }]); + const out = await svc.redeemActionToken((await svc.issueActionTokens(mail.id, 'mail.reviewer@example.com')).approve); + expect(out).toMatchObject({ ok: true, action: 'approve' }); + expect(recorded(actionsOf(engine, mail.id, 'approve')[0])).toEqual(['u_mail', 'mail.reviewer@example.com']); + + // No account carries the email: the slot is still taken (the token admits + // it), and the person is unknown — `null`, ADR-0118's value, never the email. + const stranger = await open(svc, [{ type: 'user', value: 'outside@example.com' }]); + await svc.redeemActionToken((await svc.issueActionTokens(stranger.id, 'outside@example.com')).approve); + const [act] = actionsOf(engine, stranger.id, 'approve'); + expect(recorded(act)).toEqual([null, 'outside@example.com']); + expect(act.via_override).toBe(false); + }); + + it('an override records the admin and NO slot — even when the admin named a position address', async () => { + const engine = makeFakeEngine(); + const svc = svcFor(engine); + // The admin holds the position, but the slate keys a user id instead, so + // the named address misses it and the override admits the call. This + // recorded the NAMED literal before: measured on a booted app (#21411). + const req = await open(svc, [{ type: 'user', value: 'u9' }]); + await svc.decideNode(req.id, { decision: 'approve', actorId: SLOT }, ADMIN); + const [act] = actionsOf(engine, req.id, 'approve'); + expect([...recorded(act), act.via_override]).toEqual(['root', null, true]); + }); + + it('a system context that vouches for nobody records nobody; the SLA sweep keeps its reserved sentinel', async () => { + const engine = makeFakeEngine(); + const svc = svcFor(engine); + const named = await open(svc, [{ type: 'user', value: 'u9' }]); + await svc.decideNode(named.id, { decision: 'approve', actorId: 'u9' }, SYS); + // The slot test still runs on the named address; the PERSON is the context's. + expect(recorded(actionsOf(engine, named.id, 'approve')[0])).toEqual([null, 'u9']); + + const sla = await open(svc, [{ type: 'user', value: 'u9' }]); + await svc.decideNode(sla.id, { decision: 'approve', actorId: SLA_ACTOR_ID }, SYS); + expect(recorded(actionsOf(engine, sla.id, 'approve')[0])).toEqual([SLA_ACTOR_ID, null]); + }); + + it('the multi-approver tally and decision_progress count SLOTS from acted_as, never actor_id', async () => { + const engine = makeFakeEngine(); + const svc = svcFor(engine); + const req = await open(svc, [ + { type: 'position', value: 'sales_manager', group: 'mgmt' }, + { type: 'user', value: 'u9', group: 'ops' }, + ], 'per_group'); + expect(req.pending_approvers).toEqual([SLOT, 'u9']); + + const first = await svc.decideNode(req.id, { decision: 'approve' } as any, HOLDER); + expect(first.finalized).toBe(false); + expect(first.request.pending_approvers).toEqual(['u9']); + // The person is `u_holder`, a key of no group: only the slot can satisfy `mgmt`. + const progress = (await svc.getRequest(req.id, SYS) as any)?.decision_progress; + expect(progress).toMatchObject({ behavior: 'per_group', got: 1, need: 2 }); + expect(progress.groups).toEqual([ + { group: 'mgmt', got: 1, need: 1, satisfied: true }, + { group: 'ops', got: 0, need: 1, satisfied: false }, + ]); + + const second = await svc.decideNode(req.id, { decision: 'approve' } as any, asUser('u9')); + expect(second.finalized).toBe(true); + }); + + it('already acted: the PERSON keeps sight of what they decided, the slot\'s holders do too, the bystander never', async () => { + const engine = makeFakeEngine(); + const svc = svcFor(engine); + const req = await open(svc, toPosition); + await svc.decideNode(req.id, { decision: 'approve' } as any, HOLDER); + + expect(await svc.getRequest(req.id, HOLDER), 'the person who decided').not.toBeNull(); + expect(await svc.getRequest(req.id, holding('u_successor', ['sales_manager'])), 'the slot\'s holder').not.toBeNull(); + expect(await svc.getRequest(req.id, BYSTANDER), 'the bystander').toBeNull(); + // The person half: the holder later loses the position and still sees what + // they decided (#21411 Q3 = A — the one widening, by ruling). + expect(await svc.getRequest(req.id, holding('u_holder', [])), 'the person, position lost').not.toBeNull(); + }); + + it('the action log shows the person by name and the slot beside it — and a backfilled row the slot alone', async () => { + const engine = makeFakeEngine(); + await engine.insert('sys_user', { id: 'u_holder', name: 'Hana Holder', email: 'hana@example.com' }); + const svc = svcFor(engine); + const req = await open(svc, [...toPosition, { type: 'user', value: 'u9' }], 'unanimous'); + await svc.decideNode(req.id, { decision: 'approve' } as any, HOLDER); + // A row as the boot-time backfill leaves a pre-acted_as slot literal: the + // slot kept, the person unknown. + await engine.insert('sys_approval_action', { + id: 'aact_backfilled', request_id: req.id, step_index: 0, action: 'comment', + actor_id: null, acted_as: SLOT, comment: 'older note', created_at: '2099-01-01T00:00:00.000Z', + }); + + const log = await svc.listActions(req.id, SYS); + const approve = log.find((a) => a.action === 'approve')!; + expect([approve.actor_id, approve.actor_name, approve.acted_as]).toEqual(['u_holder', 'Hana Holder', SLOT]); + // The submitter's own action took no slot: no acted_as member at all. + const submit = log.find((a) => a.action === 'submit')!; + expect(submit.actor_id).toBe('u1'); + expect('acted_as' in submit && submit.acted_as !== undefined).toBe(false); + const backfilled = log.find((a) => a.id === 'aact_backfilled')!; + expect([backfilled.actor_id, backfilled.actor_name, backfilled.acted_as]).toEqual([undefined, undefined, SLOT]); + }); + + it('already acted: a row written before acted_as existed is found by its person, and a slot is never matched against actor_id', async () => { + const engine = makeFakeEngine(); + const svc = svcFor(engine); + const old = await open(svc, [{ type: 'user', value: 'u_old' }]); + const legacy = await open(svc, toPosition); + // Both finished: neither is anyone's current slot any more, so only the + // already-acted probe can make either visible. + for (const r of [old, legacy]) { + await engine.update('sys_approval_request', { id: r.id, status: 'approved', pending_approvers: null }); + } + engine._tables['sys_approval_approver'] = (engine._tables['sys_approval_approver'] ?? []) + .filter((row: any) => row.request_id !== old.id && row.request_id !== legacy.id); + // The pre-acted_as shapes: a user-id slot vote (the person), and a slot + // literal still sitting in actor_id (as if the backfill had not run). + await engine.insert('sys_approval_action', { id: 'aact_old', request_id: old.id, step_index: 0, action: 'approve', actor_id: 'u_old' }); + await engine.insert('sys_approval_action', { id: 'aact_lit', request_id: legacy.id, step_index: 0, action: 'approve', actor_id: SLOT }); + + expect(await svc.getRequest(old.id, asUser('u_old')), 'found by its person').not.toBeNull(); + // A literal left in actor_id is NOT read as a slot — the backfill moves it. + expect(await svc.getRequest(legacy.id, holding('u_successor', ['sales_manager']))).toBeNull(); }); }); diff --git a/packages/plugins/plugin-approvals/src/approval-service.ts b/packages/plugins/plugin-approvals/src/approval-service.ts index f43d82818e9..912588fa856 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.ts @@ -837,6 +837,32 @@ function actingUserId(context: ExecutionContext | undefined): string | null { return typeof userId === 'string' && userId ? userId : null; } +/** + * The PERSON a `sys_approval_action` row records in `actor_id` (ADR-0118 D1): + * a `sys_user` id, or `null` — never the slot the action took (that is + * `acted_as`), and never an address the caller named. + * + * It is the user the CONTEXT vouches for: the session's user, or — on the + * ADR-0043 action link, which authenticates by its single-use token — the + * account `redeemActionToken` resolved the token's slot to. A named address + * (`position:

`, an email) proves only that the caller may act under it + * (`resolveActor`); it never named the person, and recording it as though it + * did left the decider on no record at all — measured on a booted app, not one + * of the rows a position-slot decision writes named them (#21411). + * + * A system context that vouches for nobody records nobody, whatever actor it + * names. One exception, by name: the SLA sweep's reserved {@link SLA_ACTOR_ID} + * keeps its sentinel. It is ADR-0118 D1 debt this column still carries, with + * the dead-run sentinel, owned by its own card (#21455) — kept here as one + * spelled constant rather than widened into "a system context's explicit actor + * is a person". + */ +function recordedActor(actorId: string, context: ExecutionContext | undefined): string | null { + const person = actingUserId(context); + if (person) return person; + return actorId === SLA_ACTOR_ID ? SLA_ACTOR_ID : null; +} + /** * Max hops when following an OOO delegation chain (#1322 M1): A out → B, B out * → C, … Bounds the walk so a mis-configured chain can't loop or resolve @@ -1119,6 +1145,12 @@ function rowFromAction(row: any): ApprovalActionRow { // `null` (a row written before the column existed) stays `undefined`: // "not recorded" is not the same claim as "not an override". via_override: row.via_override == null ? undefined : row.via_override === true, + // #21411 / #21458 — the slot the action was taken as, beside the person in + // `actor_id`, so the action log shows both: "who" and "as which slot". A + // row no slot admitted (the submitter's own actions, a system action, an + // override) — or one recorded before the column, whose slot nothing kept — + // omits it: the contract's "not recorded" case, never an empty string. + acted_as: typeof row.acted_as === 'string' && row.acted_as !== '' ? row.acted_as : undefined, // Decision attachments (#3266): rich descriptors carrying the display name + // download URL, so consumers label/open them without reading `sys_file`. attachments: attachments.length ? attachments : undefined, @@ -1511,9 +1543,12 @@ export class ApprovalService implements IApprovalService { * * A system context is exempt and keeps its explicit actor: the SLA sweep * passes the reserved {@link SLA_ACTOR_ID} sentinel, and the ADR-0043 action - * link passes the approver its single-use token is cryptographically bound to - * (having also put them on the context). Those are the only two callers that - * hold a trustworthy actor with no session behind them. + * link passes the approver slot its single-use token is cryptographically + * bound to (having put the person behind that slot on the context — + * `personForTokenSlot`). Those are the only two callers that hold a + * trustworthy actor with no session behind them. The actor is the ADDRESS + * acted under; the person an action row records is the context's + * ({@link recordedActor}). * * A caller with NO identity at all cannot act. Belt-and-suspenders: the REST * anonymous-deny now denies every anonymous request (#3963), but this service @@ -1608,13 +1643,16 @@ export class ApprovalService implements IApprovalService { * it makes the default actor and the console's `role:

` reach the same * slot. A user who holds a different position takes nothing. * - * ⭐ What a slot-gated action records: `sys_approval_action.actor_id` is the - * slot it was admitted under — the slot's own stored spelling — exactly what - * naming that slot has always recorded. It has to be: the multi-approver - * tally (`decideNode`, `attachDecisionProgress`) counts approvals by matching - * `actor_id` against the slate's slots, so a decision recorded under any - * other spelling would leave its slot pending after its holder approved. The - * already-acted probe in `visibleRequestIds` reads the same addresses back. + * ⭐ What a slot-gated action records: TWO facts, each in its own column. + * `sys_approval_action.acted_as` is the slot it was admitted under — the + * slot's own stored spelling — and `actor_id` is the person + * ({@link recordedActor}). The slot has to be recorded in its stored + * spelling: the multi-approver tally (`decideNode`, `attachDecisionProgress`) + * counts approvals by matching `acted_as` against the slate's slots, so a + * decision recorded under any other spelling would leave its slot pending + * after its holder approved. The already-acted probe in `visibleRequestIds` + * reads the same addresses back from `acted_as`. ⛔ No slot reader falls back + * to `actor_id` (#21411). */ private async takenSlot( pending: readonly string[], @@ -3207,9 +3245,11 @@ export class ApprovalService implements IApprovalService { await this.engine.insert('sys_approval_action', { id: uid('aact'), request_id: requestId, organization_id: org, step_name: nodeId, step_index: 0, action: input.decision, - // The slot this decision took (see `takenSlot`): the tally below counts - // it against the slate by this very value. - actor_id: slot ?? actorId, comment: input.comment ?? null, + // Two facts (#21411): the PERSON who decided, and the slot the decision + // took (see `takenSlot`) — the tally below counts it against the slate + // by this very value. An override holds no slot and records none. + actor_id: recordedActor(actorId, context), acted_as: slot ?? null, + comment: input.comment ?? null, // #4466: the override is recorded on the DECISION, not inferred later. // Written as an explicit `false` for an ordinary decision so a reader can // tell "checked, and it was not an override" from a legacy row's `null`. @@ -3231,7 +3271,9 @@ export class ApprovalService implements IApprovalService { const acts = await this.engine.find('sys_approval_action', { where: { request_id: requestId, step_index: 0, action: 'approve' }, limit: 1000, context: SYSTEM_CTX, }); - const approved = new Set((acts ?? []).map((a: any) => String(a.actor_id ?? '')).filter(Boolean)); + // The SLOTS approved so far — `acted_as`, never `actor_id`: a slot is + // compared only with the slot column (#21411). + const approved = new Set((acts ?? []).map((a: any) => String(a.acted_as ?? '')).filter(Boolean)); // Tally against the OPEN-time snapshot (already OOO-substituted) for // every behavior that carries one. Re-resolution survives ONLY as the @@ -3763,7 +3805,7 @@ export class ApprovalService implements IApprovalService { await this.engine.insert('sys_approval_action', { id: uid('aact'), request_id: requestId, organization_id: org, step_name: nodeId, step_index: 0, action: 'recall', - actor_id: actorId, comment: input.comment ?? null, created_at: now, + actor_id: recordedActor(actorId, context), comment: input.comment ?? null, created_at: now, }, { context: SYSTEM_CTX }); await this.engine.update('sys_approval_request', { @@ -4091,7 +4133,8 @@ export class ApprovalService implements IApprovalService { await this.engine.insert('sys_approval_action', { id: uid('aact'), request_id: requestId, organization_id: org, step_name: nodeId, step_index: 0, action: 'revise', - actor_id: slot ?? actorId, comment: input.comment ?? null, created_at: now, + actor_id: recordedActor(actorId, context), acted_as: slot ?? null, + comment: input.comment ?? null, created_at: now, }, { context: SYSTEM_CTX }); if (priorSendBacks >= maxRevisions) { @@ -4099,7 +4142,7 @@ export class ApprovalService implements IApprovalService { await this.engine.insert('sys_approval_action', { id: uid('aact'), request_id: requestId, organization_id: org, step_name: nodeId, step_index: 0, action: 'reject', - actor_id: slot ?? actorId, + actor_id: recordedActor(actorId, context), acted_as: slot ?? null, comment: `Auto-rejected: revision limit (${maxRevisions}) exceeded`, created_at: now, }, { context: SYSTEM_CTX }); await this.engine.update('sys_approval_request', { @@ -4241,7 +4284,7 @@ export class ApprovalService implements IApprovalService { await this.engine.insert('sys_approval_action', { id: uid('aact'), request_id: requestId, organization_id: org, step_name: nodeId, step_index: 0, action: 'resubmit', - actor_id: actorId, comment: input.comment ?? null, created_at: now, + actor_id: recordedActor(actorId, context), comment: input.comment ?? null, created_at: now, }, { context: SYSTEM_CTX }); let resumed = false; @@ -4400,7 +4443,8 @@ export class ApprovalService implements IApprovalService { // The hand-off parties are STRUCTURED fields (#4365) — the old default // comment (`" → "`) baked raw user ids into user-facing text. // `comment` is pure user input: absent unless the actor wrote one. - actor_id: slot ?? actorId, reassign_from: from, reassign_to: to, + actor_id: recordedActor(actorId, context), acted_as: slot ?? null, + reassign_from: from, reassign_to: to, via_override: viaOverride, comment: input.comment ?? null, created_at: now, }, { context: SYSTEM_CTX }); @@ -4469,7 +4513,7 @@ export class ApprovalService implements IApprovalService { await this.engine.insert('sys_approval_action', { id: uid('aact'), request_id: requestId, organization_id: raw.organization_id ?? null, step_name: raw.flow_node_id ?? raw.current_step ?? null, step_index: 0, action: 'remind', - actor_id: actorId, comment: input.comment ?? null, created_at: nowIso, + actor_id: recordedActor(actorId, context), comment: input.comment ?? null, created_at: nowIso, }, { context: SYSTEM_CTX }); // Per-approver fan-out: concrete identities (user ids / emails) each get @@ -4611,21 +4655,67 @@ export class ApprovalService implements IApprovalService { await this.engine.update('sys_approval_token', { id: res.token.id, consumed_at: this.clock.now().toISOString(), }, { context: SYSTEM_CTX }); + // The token IS the authentication (#3783): it is single-use, hashed at rest + // and bound to one approver SLOT, which `resolveActionToken` has just + // re-checked is still pending. The person behind that slot goes on the + // context — so the action row's `actor_id`, the status mirror and every + // flow it cascades into are attributed exactly like a decision made + // through the UI — and the slot stays the acting address, so the decision + // takes that slot (`takenSlot`) and records it as `acted_as` (#21411). + // Elevation is unchanged: `isSystem` still stands in for the missing + // session. + const person = await this.personForTokenSlot(String(res.token.approver_id)); const out = await this.decide(res.token.request_id, { decision: res.token.action, actorId: res.token.approver_id, comment: 'Via action link', - // The token IS the authentication (#3783): it is single-use, hashed at - // rest and bound to one approver, who `resolveActionToken` has just - // re-checked still holds a pending slot. So this decision has a real - // acting user even though no session carried it — name them on the - // context, so the status mirror and every flow it cascades into are - // attributed exactly like a decision made through the UI. Elevation is - // unchanged: `isSystem` still stands in for the missing session. - }, { ...SYSTEM_CTX, userId: res.token.approver_id }); + }, person ? { ...SYSTEM_CTX, userId: person } : SYSTEM_CTX); return { ok: true, action: res.token.action, request: out.request, approverId: res.token.approver_id }; } + /** + * The person an action-link token's slot belongs to — the `sys_user` id to + * vouch for on the decision's context — or `null` when no person can be + * named without guessing. + * + * Tokens are minted for concrete slots only (`remind`'s fan-out), so the + * slot is a user id or an email: + * + * - a user id IS the person; + * - an email names the ONE account carrying exactly that email — the same + * proof `resolveActor` takes for a named email, and the recipient + * resolver's own rule for delivering to it. No account, or more than one, + * names nobody; + * - a `type:value` literal names no person (no first-party path mints one). + * + * Before this the slot itself went on the context, so an email-bound link + * recorded the email as the acting user — on the action row and in the + * status mirror alike (#21411). + */ + private async personForTokenSlot(slot: string): Promise { + if (!slot) return null; + if (slot.includes('@')) { + try { + const rows = await this.engine.find('sys_user', { + where: { email: slot }, fields: ['id'], limit: 2, context: SYSTEM_CTX, + }); + const list: any[] = Array.isArray(rows) ? rows : []; + return list.length === 1 && list[0]?.id ? String(list[0].id) : null; + } catch (err) { + // Fails closed: an unreadable directory names nobody, and the decision + // still takes its slot — it is the token, not the person, that admits + // it. Said once here, because the row it leads to reads exactly like a + // decision whose decider was never known. + this.logger?.warn?.('[approvals] action link: could not resolve the account for an email-bound token — the decision records no person', { + error: err instanceof Error ? err.message : String(err), + }); + return null; + } + } + if (slot.includes(':')) return null; + return slot; + } + /** * Approver asks the submitter for more information. The request stays * pending — a thread interaction, not a flow decision. @@ -4648,7 +4738,8 @@ export class ApprovalService implements IApprovalService { await this.engine.insert('sys_approval_action', { id: uid('aact'), request_id: requestId, organization_id: raw.organization_id ?? null, step_name: raw.flow_node_id ?? raw.current_step ?? null, step_index: 0, action: 'request_info', - actor_id: slot ?? actorId, comment: input.comment.trim(), created_at: now, + actor_id: recordedActor(actorId, context), acted_as: slot ?? null, + comment: input.comment.trim(), created_at: now, }, { context: SYSTEM_CTX }); if (raw.submitter_id) { @@ -4691,7 +4782,8 @@ export class ApprovalService implements IApprovalService { await this.engine.insert('sys_approval_action', { id: uid('aact'), request_id: requestId, organization_id: raw.organization_id ?? null, step_name: raw.flow_node_id ?? raw.current_step ?? null, step_index: 0, action: 'comment', - actor_id: slot ?? actorId, comment: input.comment.trim(), + actor_id: recordedActor(actorId, context), acted_as: slot ?? null, + comment: input.comment.trim(), attachments: input.attachments?.length ? input.attachments : null, created_at: now, }, { context: SYSTEM_CTX }); @@ -6373,11 +6465,22 @@ export class ApprovalService implements IApprovalService { * `approver-address.ts`) — the set the decision methods' slot test reads, so * no request becomes visible here that the caller could not decide. * - * "Already acted" reads the same addresses, because a slot-gated action - * records the slot it took (`takenSlot`): a holder who decided a - * `position:

` request is found by `position:

`, never by their user id. - * The flip side is the position's, not the person's: a decision recorded - * under `position:

` stays visible to whoever holds `p`. + * "Already acted" asks two questions, each of its own column and each + * against its own kind of identity (#21411): + * + * - did the CALLER act on it — `actor_id` (the person) equal to the + * caller's user id. A past approver keeps sight of what they decided, and + * so does a submitter or admin who acted on it; + * - did anyone act under a SLOT the caller acts under — `acted_as` among + * the caller's acting addresses, the current-approver probe's set. The + * flip side is the position's: a decision taken as `position:

` stays + * visible to whoever holds `p`. + * + * ⛔ Neither is a fallback for the other: a slot address is never compared + * with `actor_id`, and `acted_as` is compared only with slot addresses (a + * user-id slot's address is that user id). Rows written before `acted_as` + * existed are read by their person; the boot-time `backfillActionSlots` + * moved the slot literals. * * `caller` is the caller's resolved {@link ActingCaller} when the read * already holds it (it also feeds `attachViewers`); absent, it is resolved @@ -6425,12 +6528,18 @@ export class ApprovalService implements IApprovalService { 'id', ); // Already acted on it: a past approver whose slot has moved on, or a - // commenter. They saw it legitimately; keep it that way. An action is - // recorded under the slot it took, so the probe asks for the same - // acting addresses as the current-approver probe above. + // commenter. They saw it legitimately; keep it that way. Two facts, each + // compared only with its own identity kind (see the doc block): the + // person who acted (`actor_id` = the caller's user id), and the slot it + // was taken as (`acted_as` among the current-approver probe's addresses). add( await this.engine.find('sys_approval_action', { - where: { actor_id: acting.length === 1 ? acting[0] : { $in: acting } }, + where: { + $or: [ + { actor_id: uid }, + { acted_as: acting.length === 1 ? acting[0] : { $in: acting } }, + ], + }, fields: ['request_id'], limit: cap, context: SYSTEM_CTX, }), 'request_id', @@ -6753,7 +6862,9 @@ export class ApprovalService implements IApprovalService { const acts = await this.engine.find('sys_approval_action', { where: { request_id: row.id, step_index: 0, action: 'approve' }, limit: 1000, context: SYSTEM_CTX, }); - const approved = new Set((acts ?? []).map((a: any) => String(a.actor_id ?? '')).filter(Boolean)); + // The approved SLOTS — `acted_as`, exactly what the decision tally reads + // (#21411); never `actor_id`, which holds the person. + const approved = new Set((acts ?? []).map((a: any) => String(a.acted_as ?? '')).filter(Boolean)); const snapshot = cfg?.__approverGroups as Record | undefined; const slate = snapshot ? Object.keys(snapshot) : [...approved, ...(row.pending_approvers ?? [])]; @@ -6882,9 +6993,11 @@ export class ApprovalService implements IApprovalService { context: SYSTEM_CTX, }); const actions = Array.isArray(rows) ? rows.map(rowFromAction) : []; - // Timeline display: resolve actor ids to names so the audit trail never - // shows a raw identifier. Role/team literals are already readable. The - // reassign hand-off parties (#4365) resolve through the same batch. + // Timeline display: resolve the PERSON in `actor_id` to a name so the + // audit trail never shows a raw identifier. The slot the action was taken + // as travels beside it in `acted_as`, as stored — the "acting as" half of + // the line (#21411). The reassign hand-off parties (#4365) resolve through + // the same batch; a `type:value` literal or a machine sentinel is skipped. const names = await this.resolveUserNames( actions .flatMap(a => [a.actor_id, a.reassign_from, a.reassign_to]) diff --git a/packages/plugins/plugin-approvals/src/approvals-plugin.ts b/packages/plugins/plugin-approvals/src/approvals-plugin.ts index 9f32d1732fd..7650f3c52fe 100644 --- a/packages/plugins/plugin-approvals/src/approvals-plugin.ts +++ b/packages/plugins/plugin-approvals/src/approvals-plugin.ts @@ -30,6 +30,7 @@ import { bindSnapshotRedactionMiddleware } from './payload-redaction-middleware. import { bindSnapshotPredicateGuard } from './payload-predicate-guard.js'; import type { FieldVisibilitySource } from './payload-redaction.js'; import { registerApprovalNode, type ApprovalAutomationSurface } from './approval-node.js'; +import { backfillActionSlots } from './action-slot-backfill.js'; export interface ApprovalsPluginOptions { /** Disable runtime registration (schemas still register). */ @@ -417,14 +418,44 @@ export class ApprovalsServicePlugin implements Plugin { } }; + // Action-slot backfill (#21411): move the slot literals rows written + // before `acted_as` existed still hold in `actor_id`, and give the votes a + // pending request's tally still counts their `acted_as`. Every slot reader + // reads `acted_as` with no fallback to `actor_id`, so this runs at boot + // rather than waiting for an operator. Idempotent; see the module. + // + // `error`, not `warn`: a failed run leaves the system looking normal while + // in-flight multi-approver tallies have lost the votes they already + // counted and holders have lost sight of position-slot history. + const slotEngine = engine as ApprovalEngine; + const backfillSlots = async () => { + try { + const out = await backfillActionSlots(slotEngine); + if (out.literalsMoved > 0 || out.votesStamped > 0) { + ctx.logger.info('ApprovalsServicePlugin: action slots backfilled', out); + } + } catch (err: any) { + ctx.logger.error( + '[approvals] action-slot backfill failed — approvals recorded before the slot column existed are ' + + 'missing from multi-approver tallies and from the already-acted visibility of the slot\'s holders, ' + + 'while everything else looks healthy. The repair is idempotent and runs at every boot: fix the ' + + 'cause below and restart.', + err instanceof Error ? err : undefined, + { error: err?.message ?? String(err) }, + ); + } + }; + if (typeof (ctx as any).hook === 'function') { (ctx as any).hook('kernel:ready', wireEscalationClock); (ctx as any).hook('kernel:ready', mountActionPages); (ctx as any).hook('kernel:ready', backfillApproverIndex); + (ctx as any).hook('kernel:ready', backfillSlots); } else { await wireEscalationClock(); await mountActionPages(); await backfillApproverIndex(); + await backfillSlots(); } // ADR-0019: contribute the `approval` node to the flow engine when one is diff --git a/packages/plugins/plugin-approvals/src/approver-address-readers.test.ts b/packages/plugins/plugin-approvals/src/approver-address-readers.test.ts index f16abe16311..1e3a8167279 100644 --- a/packages/plugins/plugin-approvals/src/approver-address-readers.test.ts +++ b/packages/plugins/plugin-approvals/src/approver-address-readers.test.ts @@ -22,14 +22,14 @@ * - a membership call (`includes`, `indexOf`, `has`, `some`, `find`, * `filter`, …) on a receiver whose text names a pending slate * (`/pending/i` — `pending`, `pendingApprovers`, `pending_approvers`); - * - a slot column (`approver`, `actor_id`, `pending_approvers`) set in a + * - a slot column (`approver`, `acted_as`, `pending_approvers`) set in a * query predicate — an object literal under a `where` / `filter` * property or variable, or an assignment to `where.`; * - a declarative filter row naming a slot column (`field: * 'pending_approvers'` under a `filter` in object metadata). * Every collected site must be classified in `SLOT_SITES`, by its enclosing * function and its exact source text. A NEW site — a fresh - * `pending.includes(actorId)`, a `where: { actor_id: uid }` — is + * `pending.includes(actorId)`, a `where: { acted_as: uid }` — is * unclassified and fails with its location; a classified site that changed * text or disappeared fails too, so the ledger cannot rot. Classifying a * site is a review act: a reader that compares a slot with a CALLER takes @@ -37,6 +37,18 @@ * site that compares a slot with another SLOT (a hand-off target, a token's * bound slot) is `slot-vs-slot` with its reason. * + * 3. THE PERSON COLUMN (#21411). `sys_approval_action.actor_id` is a + * `sys_user` lookup and holds the PERSON who acted; the slot an action took + * is `acted_as`. The two used to be one column, and every slot reader + * compared slots against `actor_id`. So every read of `actor_id` in this + * package — a property access, or the column in a query predicate — is + * classified in `PERSON_SITES` with its count, and the only predicate that + * compares it with anything compares it with the caller's USER ID. A slot + * reader rewritten back onto `actor_id` (the tally's `a.actor_id`, a probe's + * `actor_id: acting`) is unclassified and fails with its location. The rule, + * in one line: slots are compared only with `acted_as`, and `actor_id` only + * with a caller's user id. + * * What it does not see: a comparison spelled with none of those shapes (a * hand-written loop over a slate with `===`). The shapes are the ones every * reader in this file's history has used; a new spelling is caught in review, @@ -82,9 +94,9 @@ const SLOT_SITES: Readonly> = { role: 'reader', why: 'the same filter, several addresses', }, - 'approval-service.ts · visibleRequestIds · actor_id: acting.length === 1 ? acting[0] : { $in: acting }': { + 'approval-service.ts · visibleRequestIds · acted_as: acting.length === 1 ? acting[0] : { $in: acting }': { role: 'reader', - why: 'already-acted probe: `acting` is actingAddresses(caller), the current-approver probe\'s set', + why: 'already-acted probe, slot half: `acting` is actingAddresses(caller), the current-approver probe\'s set', }, 'approval-service.ts · reassign · pending.includes(to)': { role: 'slot-vs-slot', @@ -120,7 +132,56 @@ const SLOT_SITES: Readonly> = { const MEMBERSHIP = new Set(['includes', 'indexOf', 'lastIndexOf', 'has', 'some', 'every', 'find', 'findIndex', 'filter']); const SLATE = /pending/i; -const SLOT_COLUMNS = new Set(['approver', 'actor_id', 'pending_approvers']); +const SLOT_COLUMNS = new Set(['approver', 'acted_as', 'pending_approvers']); + +/** The person column of `sys_approval_action` (#21411). */ +const PERSON_COLUMN = 'actor_id'; + +type PersonRole = 'caller-user-id' | 'display' | 'migration'; + +/** + * Every read of `actor_id` in this package, keyed ` · · `, with how many times that exact text occurs + * there. `caller-user-id` is the one comparison allowed: the column against + * the caller's user id. Nothing compares it with a slot. + */ +const PERSON_SITES: Readonly> = { + 'approval-service.ts · visibleRequestIds · actor_id: uid': { + count: 1, + role: 'caller-user-id', + why: 'already-acted probe, person half: did the CALLER act on it (`uid` is the resolved caller\'s user id)', + }, + 'approval-service.ts · rowFromAction · row.actor_id': { + count: 1, + role: 'display', + why: 'the action-log DTO carries the person as stored', + }, + 'approval-service.ts · listActions · a.actor_id': { + count: 3, + role: 'display', + why: 'resolves the person\'s display name for the action log', + }, + 'action-slot-backfill.ts · backfillActionSlots · actor_id: { $contains: \':\' }': { + count: 1, + role: 'migration', + why: 'the boot-time repair finds rows whose actor_id still holds a type:value slot literal — a shape scan, no identity', + }, + "action-slot-backfill.ts · backfillActionSlots · actor_id: { $contains: '@' }": { + count: 1, + role: 'migration', + why: 'the same scan, for an email slot left in actor_id', + }, + 'action-slot-backfill.ts · backfillActionSlots · actor_id: { $nin: [...RESERVED_MACHINE_ACTORS] }': { + count: 1, + role: 'migration', + why: 'the same scan leaves the reserved machine sentinels alone', + }, + 'action-slot-backfill.ts · backfillActionSlots · row.actor_id': { + count: 2, + role: 'migration', + why: 'the value the repair moves to acted_as (pass 1) or copies there (pass 2)', + }, +}; const PREDICATE_NAMES = new Set(['where', 'filter']); function sourceFiles(dir: string): string[] { @@ -243,11 +304,48 @@ describe('approver-address — the readers of the equivalence, and no comparison expect(new Set(sites).size).toBe(sites.length); }); + it('actor_id is compared only with a caller\'s user id, and every read of it is classified — a slot reader moved back onto it fails with its location (#21411)', () => { + const counts = new Map(); + let predicates = 0; + for (const path of sourceFiles(HERE)) { + const sf = parse(path); + const file = relative(HERE, path); + const visit = (node: ts.Node): void => { + const isPredicate = ts.isPropertyAssignment(node) && node.name.getText(sf) === PERSON_COLUMN && inPredicate(node, sf); + const isRead = ts.isPropertyAccessExpression(node) && node.name.text === PERSON_COLUMN; + if (isPredicate || isRead) { + const key = `${file} · ${enclosingFunction(node, sf)} · ${node.getText(sf).replace(/\s+/g, ' ')}`; + counts.set(key, (counts.get(key) ?? 0) + 1); + if (isPredicate) predicates++; + } + ts.forEachChild(node, visit); + }; + visit(sf); + } + // The scan saw the column at all — not a pass over nothing. + expect(predicates).toBeGreaterThan(0); + const unclassified = [...counts.keys()].filter((k) => !(k in PERSON_SITES)); + expect( + unclassified, + 'actor_id is the PERSON: compare a slot with acted_as (and list the site in SLOT_SITES), or — for a read of ' + + 'the person — classify it in PERSON_SITES with its reason', + ).toEqual([]); + const drifted = Object.entries(PERSON_SITES) + .filter(([k, v]) => counts.get(k) !== v.count) + .map(([k, v]) => `${k}: expected ${v.count}, found ${counts.get(k) ?? 0}`); + expect(drifted, 'a classified actor_id site changed, moved or disappeared: re-classify it').toEqual([]); + // The one comparison is with the caller's user id, and with nothing else. + const comparisons = Object.entries(PERSON_SITES).filter(([, v]) => v.role === 'caller-user-id').map(([k]) => k); + expect(comparisons).toEqual(['approval-service.ts · visibleRequestIds · actor_id: uid']); + const service = parse(join(HERE, SERVICE)); + expect(method(service, 'visibleRequestIds').getText(service)).toMatch(/const uid = who\.userId;/); + }); + it('the detector catches the shapes it names (a planted user-id comparison in each shape)', () => { const planted = ts.createSourceFile('planted.ts', [ 'class S {', ' a(pending: string[], uid: string) { return pending.includes(uid); }', - ' b(engine: any, uid: string) { return engine.find("x", { where: { actor_id: uid } }); }', + ' b(engine: any, uid: string) { return engine.find("x", { where: { acted_as: uid } }); }', ' c(uid: string) { const where: any = {}; where.approver = uid; return where; }', '}', 'export const V = { filter: [{ field: "pending_approvers", operator: "contains", value: "{current_user_id}" }] };', diff --git a/packages/plugins/plugin-approvals/src/sys-approval-action.object.ts b/packages/plugins/plugin-approvals/src/sys-approval-action.object.ts index 39a26475caa..266c1d66cb8 100644 --- a/packages/plugins/plugin-approvals/src/sys-approval-action.object.ts +++ b/packages/plugins/plugin-approvals/src/sys-approval-action.object.ts @@ -32,7 +32,7 @@ export const SysApprovalAction = ObjectSchema.create({ displayNameField: 'display_title', nameField: 'display_title', // [ADR-0079] canonical primary-title pointer (mirrors deprecated displayNameField) titleFormat: '{action} · {step_name}', - highlightFields: ['request_id', 'step_name', 'action', 'actor_id', 'via_override', 'created_at'], + highlightFields: ['request_id', 'step_name', 'action', 'actor_id', 'acted_as', 'via_override', 'created_at'], // ADR-0104 D3 wave 2. `attachments` is a media field, so the files it holds // are OWNED by this row — and the storage service would otherwise authorize @@ -49,7 +49,7 @@ export const SysApprovalAction = ObjectSchema.create({ name: 'recent', label: 'Recent', data: { provider: 'object', object: 'sys_approval_action' }, - columns: ['created_at', 'request_id', 'step_name', 'action', 'actor_id', 'via_override', 'comment'], + columns: ['created_at', 'request_id', 'step_name', 'action', 'actor_id', 'acted_as', 'via_override', 'comment'], sort: [{ field: 'created_at', order: 'desc' }], pagination: { pageSize: 50 }, emptyState: { title: 'No approval actions yet', message: 'Actions are logged automatically when approvals progress.' }, @@ -69,7 +69,7 @@ export const SysApprovalAction = ObjectSchema.create({ name: 'all_actions', label: 'All', data: { provider: 'object', object: 'sys_approval_action' }, - columns: ['created_at', 'request_id', 'step_name', 'action', 'actor_id', 'via_override', 'comment'], + columns: ['created_at', 'request_id', 'step_name', 'action', 'actor_id', 'acted_as', 'via_override', 'comment'], sort: [{ field: 'created_at', order: 'desc' }], pagination: { pageSize: 100 }, }, @@ -132,10 +132,49 @@ export const SysApprovalAction = ObjectSchema.create({ }, ), + // [ADR-0118 D1] The PERSON who took the action — a `sys_user` id or + // nothing, never a slot literal or a sentinel. The slot the action was + // admitted under is a separate fact and lives in `acted_as` below: one + // holder of a position acts for it, one person can hold several slots, and + // a slot recorded HERE (as it once was) left the decider on no column at + // all, and dropped the row from every join on this lookup. Empty means no + // person is recorded: a system-initiated action, or — with `acted_as` set — + // a decision recorded before the person was captured, whose decider no + // stored record names (the boot-time `backfillActionSlots` moved its slot + // out of this column rather than guess one). actor_id: Field.lookup('sys_user', { label: 'Actor', required: false, group: 'Action', + description: + 'The user who took this action. Empty when no person is recorded: a system-initiated action, or a ' + + 'decision recorded before the deciding user was captured, which still shows the slot it was taken ' + + 'as.', + }), + + // The pending-approver slot the action was taken AS — the slot's address in + // its stored spelling, exactly as it stood in `pending_approvers` when the + // action was admitted: a user id, an email, or a `type:value` literal such + // as `position:`. `ApprovalActionRow.acted_as` is the contract's + // reading of this column. + // + // It is never a person (that is `actor_id`), and it is what every + // slot-against-slate comparison reads: the multi-approver tally, + // `decision_progress`, and the slot half of the already-acted probe. ⛔ No + // reader falls back to `actor_id` for a slot. + // + // Empty on an action no slot admitted — the submitter's own actions, a + // system action, an admin override (`via_override`) — and on a row written + // before this column whose slot could not be recovered without guessing. + acted_as: Field.text({ + label: 'Acted As', + required: false, + maxLength: 255, + group: 'Action', + description: + 'The pending-approver slot this action was taken as, in the slot’s stored spelling: a user id, an ' + + 'email, or a position address. Empty when no slot admitted the action, such as the submitter’s own ' + + 'actions, system actions and admin overrides.', }), comment: Field.textarea({ label: 'Comment', required: false, group: 'Action' }), diff --git a/packages/plugins/plugin-approvals/src/translations/en.objects.generated.ts b/packages/plugins/plugin-approvals/src/translations/en.objects.generated.ts index e2f5e276f0f..19661f08f58 100644 --- a/packages/plugins/plugin-approvals/src/translations/en.objects.generated.ts +++ b/packages/plugins/plugin-approvals/src/translations/en.objects.generated.ts @@ -253,7 +253,12 @@ export const enObjects: NonNullable = { } }, actor_id: { - label: "Actor" + label: "Actor", + help: "The user who took this action. Empty when no person is recorded: a system-initiated action, or a decision recorded before the deciding user was captured, which still shows the slot it was taken as." + }, + acted_as: { + label: "Acted As", + help: "The pending-approver slot this action was taken as, in the slot’s stored spelling: a user id, an email, or a position address. Empty when no slot admitted the action, such as the submitter’s own actions, system actions and admin overrides." }, comment: { label: "Comment" diff --git a/packages/plugins/plugin-approvals/src/translations/es-ES.objects.generated.ts b/packages/plugins/plugin-approvals/src/translations/es-ES.objects.generated.ts index e29bbb6ab06..8944c9f9f8b 100644 --- a/packages/plugins/plugin-approvals/src/translations/es-ES.objects.generated.ts +++ b/packages/plugins/plugin-approvals/src/translations/es-ES.objects.generated.ts @@ -253,7 +253,12 @@ export const esESObjects: NonNullable = { } }, actor_id: { - label: "Actor" + label: "Actor", + help: "El usuario que realizó esta acción. Vacío cuando no hay ninguna persona registrada: una acción iniciada por el sistema, o una decisión registrada antes de que se capturara al usuario que decidió, que sigue mostrando el turno en calidad del cual se tomó." + }, + acted_as: { + label: "Actuó como", + help: "El turno de aprobación pendiente en calidad del cual se realizó esta acción, con la grafía almacenada del turno: un ID de usuario, un correo electrónico o una dirección de puesto. Vacío cuando ningún turno admitió la acción, como las acciones propias del solicitante, las acciones del sistema y las anulaciones de administrador." }, comment: { label: "Comentario" diff --git a/packages/plugins/plugin-approvals/src/translations/ja-JP.objects.generated.ts b/packages/plugins/plugin-approvals/src/translations/ja-JP.objects.generated.ts index 9695955c4a7..7845ccf855c 100644 --- a/packages/plugins/plugin-approvals/src/translations/ja-JP.objects.generated.ts +++ b/packages/plugins/plugin-approvals/src/translations/ja-JP.objects.generated.ts @@ -253,7 +253,12 @@ export const jaJPObjects: NonNullable = { } }, actor_id: { - label: "操作者" + label: "操作者", + help: "この操作を行ったユーザー。人物が記録されていない場合は空です。システムが開始した操作、または判断者が記録されるようになる前の判断（代表したスロットは引き続き表示されます）が該当します。" + }, + acted_as: { + label: "代表スロット", + help: "この操作が代表した承認待ちスロット。スロットに保存された表記（ユーザー ID、メールアドレス、またはポジションアドレス）で記録されます。申請者自身の操作、システム操作、管理者オーバーライドなど、スロットを経由せずに許可された操作では空になります。" }, comment: { label: "コメント" diff --git a/packages/plugins/plugin-approvals/src/translations/zh-CN.objects.generated.ts b/packages/plugins/plugin-approvals/src/translations/zh-CN.objects.generated.ts index 889cc6b7957..a6da77e6067 100644 --- a/packages/plugins/plugin-approvals/src/translations/zh-CN.objects.generated.ts +++ b/packages/plugins/plugin-approvals/src/translations/zh-CN.objects.generated.ts @@ -253,7 +253,12 @@ export const zhCNObjects: NonNullable = { } }, actor_id: { - label: "执行人" + label: "执行人", + help: "执行此操作的用户。为空表示未记录人员：系统发起的操作，或在开始记录决定人之前写入的决定（此类决定仍会显示其所代表的槽位）。" + }, + acted_as: { + label: "代表槽位", + help: "执行此操作时所代表的待审批槽位，按该槽位存储的写法记录：用户 ID、邮箱或岗位地址。若操作并非经由槽位放行（如提交人自己的操作、系统操作和管理员越权操作），则为空。" }, comment: { label: "评论" diff --git a/packages/qa/dogfood/test/position-address-readers.dogfood.test.ts b/packages/qa/dogfood/test/position-address-readers.dogfood.test.ts index 1d54981788c..d692ca2b5ca 100644 --- a/packages/qa/dogfood/test/position-address-readers.dogfood.test.ts +++ b/packages/qa/dogfood/test/position-address-readers.dogfood.test.ts @@ -25,7 +25,9 @@ // ⭐ can_act is the default actor's decision answer, row for row — holder, // bystander, submitter, admin; // ⭐ the holder decides with the default actor AND with the console's -// spelling; the decision is recorded under the slot's stored spelling; +// spelling; the decision records the holder in `actor_id` and the slot's +// stored spelling in `acted_as` (#21411: the person and the slot are two +// facts, in two columns); // ⭐ the holder keeps sight of the request after deciding it; the bystander // never sees it (this widens nobody); // ⭐ the email-keyed slot is listed, flagged, decided and kept in sight. @@ -93,7 +95,7 @@ describe('every slot reader takes the caller\'s acting addresses (#21379)', () = const rows: any[] = await ql.find('sys_approval_action', { where: { request_id: id, action: 'approve' }, context: SYS.context, }); - return rows.map((r) => ({ actor_id: r.actor_id, via_override: r.via_override })); + return rows.map((r) => ({ actor_id: r.actor_id ?? null, acted_as: r.acted_as ?? null, via_override: r.via_override })); }; const seen = new Set(); /** Open one position-routed request while NOBODY holds the position. */ @@ -135,10 +137,13 @@ describe('every slot reader takes the caller\'s acting addresses (#21379)', () = expect([submitterTry.status, submitterTry.code]).toEqual([403, 'FORBIDDEN']); const holderDecision = await approve(holderToken, tableRequest); expect([holderDecision.status, holderDecision.requestStatus]).toEqual([200, 'approved']); - expect(await recorded(tableRequest)).toEqual([{ actor_id: SLOT, via_override: false }]); + expect(await recorded(tableRequest)).toEqual([{ actor_id: holderId, acted_as: SLOT, via_override: false }]); const adminDecision = await approve(adminToken, adminRequest); expect([adminDecision.status, adminDecision.requestStatus]).toEqual([200, 'approved']); - expect((await recorded(adminRequest)).map((r) => r.via_override)).toEqual([true]); + // An override records the admin — the person — and no slot. + const adminId = await idOf('admin@objectos.ai'); + expect(adminId).not.toBe(''); + expect(await recorded(adminRequest)).toEqual([{ actor_id: adminId, acted_as: null, via_override: true }]); // ⭐ The holder keeps sight of what they decided; the bystander never had it. expect(await detail(holderToken, tableRequest)).toBe(200); @@ -147,7 +152,7 @@ describe('every slot reader takes the caller\'s acting addresses (#21379)', () = // ⭐ The console's spelling reaches the same slot. const viaRole = await approve(holderToken, consoleSpellingRequest, { actorId: `role:${ROUTED_POSITION}` }); expect([viaRole.status, viaRole.requestStatus]).toEqual([200, 'approved']); - expect(await recorded(consoleSpellingRequest)).toEqual([{ actor_id: SLOT, via_override: false }]); + expect(await recorded(consoleSpellingRequest)).toEqual([{ actor_id: holderId, acted_as: SLOT, via_override: false }]); // ⭐ The email-keyed slot, for a reviewer who did not submit it. const createdEmail = await stack.apiAs(submitterToken, 'POST', '/data/pa_email_request', { name: 'email' }); @@ -162,7 +167,7 @@ describe('every slot reader takes the caller\'s acting addresses (#21379)', () = expect(await list(bystanderToken, [bystanderId, EMAIL_APPROVER])).toEqual([]); const emailDecision = await approve(emailToken, emailRequest); expect([emailDecision.status, emailDecision.requestStatus]).toEqual([200, 'approved']); - expect(await recorded(emailRequest)).toEqual([{ actor_id: EMAIL_APPROVER, via_override: false }]); + expect(await recorded(emailRequest)).toEqual([{ actor_id: emailId, acted_as: EMAIL_APPROVER, via_override: false }]); expect(await detail(emailToken, emailRequest)).toBe(200); } finally { await stack.stop();