Repository navigation
Commit 88fb5e8
fix(approvals): every sys_user lookup approvals writes holds an id or null — machine actors record none, notify forwards only a person, reassign parties are slot addresses (ADR-0118 D1) (#21514)
Fixes #21455
Clause-②: no
## What changes
Under ADR-0118 D1 a lookup to `sys_user` holds a user id or null, never
a placeholder. Four writers in the approvals plugin broke that, and a
lookup holding a non-id drops the row from every join and report on it,
silently. This applies triage's position-by-position ruling
(`5960380029`). It builds on #21411 (PR #21493, `6f17d1d36`): the same
`recordedActor`, the same slot-address column kind (`acted_as`), and the
same boot-time repair.
1. **SLA and dead-run sweeps: `actor_id` is null.**
- `escalateRequest` writes the `escalate` row with `actor_id: null`.
Before, it wrote `system:sla`.
- The `auto_approve` / `auto_reject` decision runs under `SYSTEM_CTX`.
That context vouches for nobody, so `recordedActor` records null. The
special case that kept `system:sla` is removed.
- `abandonForDeadRun` writes its `recall` row with `actor_id: null`.
Before, it wrote `system:dead-run`.
- The fact that a sweep acted is the row's own kind. The `escalate`
row's comment names the configured action, and the dead-run `recall`
names the run and its status. `DEAD_RUN_ACTOR_ID` is deleted.
- `SLA_ACTOR_ID` stays, as the sweep's acting identity only: `decide()`
under a system context requires an explicit actor. Its docblock says it
is never recorded.
2. **`reassign_from` / `reassign_to` are slot-address fields.** They are
now `Field.text({ maxLength: 255 })`, the declared form of `acted_as`,
instead of `Field.lookup('sys_user')`. The descriptions say they hold
the slot's address in its stored spelling: a user id, an email or a
position address. Translations are regenerated with `node
scripts/check-i18n-bundles.mjs --write`, and the zh-CN, ja-JP and es-ES
help texts are written by hand. The zh-CN labels also become neutral
(from party / to party instead of from person / to person).
3. **`notify` forwards a person or nothing.** `notify` no longer takes
an actor string. A caller hands in `actorContext` (the action's
`ExecutionContext`), and `notify` forwards `recordedActor(actorContext)`
as `actorId`, or omits it. So no string path exists for a slot literal
or a sentinel.
- Callers that pass the context: `sendBack` (both notifications),
`reassign`, `remind` (both), `requestInfo` and `comment`.
- Callers that pass nothing: the three SLA notifications, and the two
out-of-office notifications, which already passed nothing.
- `service-messaging` is unchanged. It writes
`sys_notification.actor_id` from `EmitInput.actorId` as given, so the
producer is where this is fixed.
4. **Stored sentinels.** `backfillActionSlots` pass 1 already scanned
`actor_id` for values that contain `:` or `@`. The `$nin` exclusion of
the two sentinels is gone, and each row takes one of two arms with the
same single `update` call:
- a slot literal moves to `acted_as`, as before;
- a sentinel is nulled, and gets no `acted_as`, because a sweep took no
slot.
The result gains `sentinelsCleared`, and the plugin's boot log reports
it. The pass stays idempotent: a second run writes `{0, 0, 0}`. The
sentinels are now spelled only in `LEGACY_MACHINE_SENTINELS` (renamed
from `RESERVED_MACHINE_ACTORS`). The update call-site count is
unchanged, so the tenant-audit census needs no regeneration.
5. **Docs.** In `content/docs/automation/approvals.mdx`, the dead-run
sentence that documented `system:dead-run` now says the row records no
actor and names the run. Two short sentences say the escalation rows and
notifications record no actor, and that `reassign_from` / `reassign_to`
hold slot addresses.
## Readers of the sentinels (measured by grep at `e72a7d2176`)
- **This repo:** no runtime reader keys on `system:sla` or
`system:dead-run`.
- The SLA re-run guard keys on `action: 'escalate'` (the ADR-0042 §1
marker), not the actor.
- The dead-run sweep re-scans only `status: 'pending'` requests, so it
does not key on the actor either.
- `listActions` resolves names for ids and skips `:` values.
- No REST approvals route and no analytics code reads either value.
- The remaining hits: tests (updated here), release-owned `CHANGELOG.md`
files, ADR-0042, the QA checklist JSON, and the backfill's legacy list.
- **objectui at the pinned `.objectui-sha` (`89cad75d5`):**
`apps/console/src/pages/system/ApprovalsInboxPage.tsx:2331` renders
`actor_id === 'system:sla'` as "System (SLA)". Other actors go through
`formatIdentity`, which shows an em dash for an empty actor. See the
acceptance notes.
## What the field-type change does to a stored column (measured)
The old declaration created the table and stored three reassign rows (a
position, an email, a user id). The new declaration then booted over the
same database. A positive control confirmed the DDL capture: it saw the
`create table` on the first boot.
| | SQLite (better-sqlite3) | PostgreSQL 16.14 |
| --- | --- | --- |
| column before | `varchar(255)` | `character varying(255)` |
| DDL on the upgrade boot | none | none |
| column after | `varchar(255)` | `character varying(255)` |
| stored values | all 3 unchanged | all 3 unchanged |
| foreign keys / indexes on the columns | 0 / 0, before and after | 0 /
0, before and after |
| `detectManagedDrift` for `reassign_from` / `reassign_to` | none | none
|
| a fresh column (as `acted_as` today) | `TEXT` | `text` |
The lookup arm never emitted a foreign key (`sql-driver.ts`, the #11567
note), so nothing has to be dropped. Neither column is in a list view,
`highlightFields` or an index. In `listActions`, `reassign_from_name` /
`reassign_to_name` resolve where an address names an account: a user id,
or an email an account carries (the existing `resolveUserNames`). A
position has no name. The family pin asserts all three cases.
## Pins
- **`actor-lookups-hold-ids.integration.test.ts`** (new). This is the
family pin, on a real `ObjectQL` over `SqlDriver` (better-sqlite3 in
memory, real DDL).
- It reads the `sys_user` lookups from the plugin's own manifest
`objects`, not from a re-listed set.
- It drives every write path through the real `ApprovalService`:
position-slot and email-slot actions, comment, request-info, remind,
three reassign shapes, a vote, the action link, send-back within budget
and past it, recall, an out-of-office substitution, all four SLA
actions, and the dead-run sweep.
- It then asserts:
- every stored value of every declared lookup is a `sys_user` id or
null;
- every declared lookup was written at least once, so a new lookup stays
red until the battery writes it;
- every `notify` actor is a person, and each person-driven topic
forwards the holder, never the address;
- each machine actor writes null, and the machine topics name no actor;
- a position and an email round-trip in the slot fields, with the names
resolving as described above.
- **`action-slot-backfill.integration.test.ts`** now plants three
sentinel rows, one of them an `approve` at step 0 on a pending request
(the shape pass 2 stamps). It asserts `{ literalsMoved: 3,
sentinelsCleared: 3, votesStamped: 1 }`, then 0 sentinels left in the
lookup, user-id rows untouched, and a second run writing `{0, 0, 0}`.
- **`action-slot-backfill-wiring.test.ts`:** a run that only cleared
sentinels still logs.
- The existing pins that asserted the sentinels now assert null:
- `approval-service.test.ts`: SLA notify, auto_approve, the
system-context case, and dead-run;
- `approval-actor-impersonation.test.ts`;
- `run-status-liveness.test.ts`;
- `approval-service-sla-calendar-clock.test.ts`.
The `approver-address-readers.test.ts` ledger drops the `$nin` site.
## Ablations (each through `scripts/ablation-replace.mjs`, wrapped,
direction predicted first; every restore proven: blob equals HEAD and
`git diff HEAD` empty)
| leg | mutation | predicted | observed |
| --- | --- | --- | --- |
| A | the `escalate` row writes `SLA_ACTOR_ID` again | red: id-or-null
and machine pins, the SLA tests, the SLA-clock test; round-trip green |
5 failed / 335 passed, those five exactly |
| B | `notify` forwards `SLA_ACTOR_ID` when no context vouches | red:
notify pin, machine pin, SLA notify test; stored-row pin green | 3
failed / 337 passed, those three exactly |
| C | `reassign_from` back to `Field.lookup('sys_user')` | red:
declared-kind and id-or-null pins; round-trip and coverage green | 2
failed / 4 passed; the offenders are named: `position:legal` and the
email |
| D | the backfill's sentinel arm removed (`sentinel = false`) | red:
the backfill pin | 1 failed / 4 passed (`sentinelsCleared` 0, not 3) |
| E | a new `sys_user` lookup that nothing writes | red: the coverage
pin only | 1 failed / 5 passed; it names the new field |
The first attempt at leg E was a no-op: the tool refused it because the
replacement contained the anchor (count 1 to 1), and it restored the
file. I re-ran it with a semantically identical re-spelling.
## Gates, at `e72a7d2176` (after merging `origin/main` `85e29b885`)
- `node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack
--commands` derived 95 commands, and all 95 exit 0.
- Two of them first exited 3 (PREREQUISITE NOT MET:
`check:skill-examples`, `check:dual-build-cjs-loads`). After `turbo run
build --filter=!@objectstack/docs`, both exit 0.
- `--ran` with exit codes: 95 derived, 95 run, 0 NOT-MEASURED, 0 UNRUN.
- The 4 in-path roster gates also exit 0: `check-changeset-fixed`,
`check:authz-resolver`, `check:error-code-casing`,
`check:filter-alias-parity`.
- `pnpm --filter @objectstack/plugin-approvals typecheck`: exit 0
(`check:test-typecheck` OK).
- `pnpm --filter @objectstack/plugin-approvals test`: 59 files, 875
tests passed.
- `pnpm --filter @objectstack/service-messaging test`: 46 files, 507
tests passed.
- `packages/qa/dogfood/test/position-address-readers.dogfood.test.ts`: 1
of 1 passed, against a rebuilt `plugin-approvals` dist.
- eslint is narrowed to the 16 changed `.ts` files: `--no-inline-config
--format json` reports 16 files, 0 errors and 0 warnings.
- The population comes from `eslint.config.mjs`'s `packages/**/*.{ts,…}`
globs.
- The config never enables type-aware linting (no
`parserOptions.project`), so this diff cannot move the verdict on an
untouched file.
## Changeset
`@objectstack/plugin-approvals` patch, `Clause-②: no`. It says it
supersedes the "The SLA sweep keeps its reserved `system:sla` actor for
now" sentence of the unreleased
`.changeset/21411-approval-actor-person.md`, which this PR does not
edit. Both ship in one release.
## Acceptance notes
- **Console rendering (objectui).** At the pinned sha, the inbox
timeline labels `system:sla` "System (SLA)" and otherwise shows an empty
actor as an em dash. After this PR, the SLA and dead-run rows have no
actor, so they show the em dash, as out-of-office, cancel and backfilled
rows already do. ADR-0118 D1 makes "System" a render rule for an empty
actor, and D5 answers "which automation" from association fields: for an
SLA auto-decision that is the `escalate` row right before it, whose
comment names the action. The renderer change is objectui work, the same
carrier as #21411's `acted_as` rendering. No reader needs a column the
row kind does not carry, so none is added.
- **ADR-0042 §2** ("Machine decisions → reserved actor `system:sla`",
and clients rendering it) is the decision ADR-0118 D1 supersedes for
this column. ADR-0042 has no amended status line, and `docs/adr/**` is
outside this PR's surface (Tier H). This needs a seat or maintainer
decision.
- **`ApprovalActionRow.reassign_from` / `reassign_to` TSDoc** in
`packages/spec/src/contracts/approval-service.ts` still says "the user
whose pending-approver slot was moved", and `*_name` says
`sys_user.name`. The type (`string`) is unchanged. The text needs a
spec-lane correction, and this PR makes no `packages/spec` edit.
- **`docs/qa/platform-checklist/areas/approvals.json`** (the SLA
single-shot item) says the `escalate` row's actor is `SLA_ACTOR_ID`. The
row now records no actor. The item needs a one-word change, outside this
PR's surface.
- **Behaviour change for integrations:** code that tested `actor_id` for
`system:sla` / `system:dead-run` reads the row kind instead, and code
that expanded `reassign_from` / `reassign_to` as `sys_user` reads them
as addresses. The changeset says both.
---
_Generated by [Claude
Code](https://claude.ai/code/session_01DiCSbmJrkzNhuEAier4VoJ)_
---------
Co-authored-by: Claude <noreply@anthropic.com>1 parent e9dec3d commit 88fb5e8
18 files changed
Lines changed: 628 additions & 151 deletions
File tree
- .changeset
- content/docs/automation
- packages/plugins/plugin-approvals/src
- translations
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
512 | 512 | | |
513 | 513 | | |
514 | 514 | | |
515 | | - | |
| 515 | + | |
| 516 | + | |
| 517 | + | |
516 | 518 | | |
517 | 519 | | |
518 | 520 | | |
| |||
672 | 674 | | |
673 | 675 | | |
674 | 676 | | |
675 | | - | |
676 | | - | |
| 677 | + | |
| 678 | + | |
| 679 | + | |
677 | 680 | | |
678 | 681 | | |
679 | 682 | | |
| |||
724 | 727 | | |
725 | 728 | | |
726 | 729 | | |
727 | | - | |
| 730 | + | |
| 731 | + | |
| 732 | + | |
| 733 | + | |
728 | 734 | | |
729 | 735 | | |
730 | 736 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
15 | 15 | | |
16 | 16 | | |
17 | 17 | | |
18 | | - | |
| 18 | + | |
19 | 19 | | |
20 | 20 | | |
21 | 21 | | |
| |||
71 | 71 | | |
72 | 72 | | |
73 | 73 | | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
74 | 82 | | |
75 | 83 | | |
76 | 84 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
8 | 8 | | |
9 | 9 | | |
10 | 10 | | |
11 | | - | |
12 | | - | |
13 | | - | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
14 | 17 | | |
15 | 18 | | |
16 | 19 | | |
| |||
27 | 30 | | |
28 | 31 | | |
29 | 32 | | |
30 | | - | |
31 | | - | |
| 33 | + | |
| 34 | + | |
32 | 35 | | |
33 | 36 | | |
34 | 37 | | |
| |||
86 | 89 | | |
87 | 90 | | |
88 | 91 | | |
89 | | - | |
90 | | - | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
91 | 97 | | |
92 | 98 | | |
93 | | - | |
| 99 | + | |
94 | 100 | | |
95 | 101 | | |
96 | 102 | | |
| |||
103 | 109 | | |
104 | 110 | | |
105 | 111 | | |
| 112 | + | |
| 113 | + | |
| 114 | + | |
| 115 | + | |
106 | 116 | | |
107 | 117 | | |
108 | 118 | | |
| |||
116 | 126 | | |
117 | 127 | | |
118 | 128 | | |
119 | | - | |
| 129 | + | |
120 | 130 | | |
121 | 131 | | |
122 | 132 | | |
| |||
125 | 135 | | |
126 | 136 | | |
127 | 137 | | |
128 | | - | |
129 | | - | |
130 | | - | |
| 138 | + | |
| 139 | + | |
| 140 | + | |
131 | 141 | | |
132 | 142 | | |
133 | 143 | | |
| |||
138 | 148 | | |
139 | 149 | | |
140 | 150 | | |
141 | | - | |
142 | | - | |
143 | | - | |
| 151 | + | |
| 152 | + | |
| 153 | + | |
| 154 | + | |
| 155 | + | |
| 156 | + | |
| 157 | + | |
144 | 158 | | |
145 | 159 | | |
| 160 | + | |
| 161 | + | |
| 162 | + | |
| 163 | + | |
| 164 | + | |
146 | 165 | | |
147 | 166 | | |
148 | | - | |
| 167 | + | |
149 | 168 | | |
150 | 169 | | |
151 | 170 | | |
| |||
0 commit comments