diff --git a/.changeset/22725-approvals-unstaffed-arm.md b/.changeset/22725-approvals-unstaffed-arm.md deleted file mode 100644 index 5a675186073..00000000000 --- a/.changeset/22725-approvals-unstaffed-arm.md +++ /dev/null @@ -1,16 +0,0 @@ ---- -"@objectstack/spec": minor -"@objectstack/plugin-approvals": minor -"@objectstack/rest": minor ---- - -feat(spec,plugin-approvals,rest): the approvals inbox read gets an `unstaffed` arm, so an `admin_rescue` request is in the queue of a caller who may override it - -Clause-②: yes (widening) - -- **What is new.** `IApprovalService.listRequests`' filter declares an optional `unstaffed?: boolean`. `countRequests` takes the same filter, so it answers it too. `GET /api/v1/approvals/requests` accepts it as the declared query parameter `?unstaffed=true|false`. -- **What it selects.** The PENDING requests whose current slate names no person: no user id and no email, only `type:value` literals such as `position:

`, or nothing at all. This is what `onEmptyApprovers: 'admin_rescue'` (the default) opens. The test is the one the open path makes before it warns "decidable only by a privileged admin", applied to the slate as it stands. A slate whose last named approver acted and left only literals matches too. A slate that still names a person never matches, even if that person has left. -- **Who it returns them to.** Only a caller who may override them, judged by the privileged-override rule that admits the override decision itself. Every row it adds reads `viewer.can_override: true`, and a tenant admin's arm stops at their organization. For anyone else the member adds nothing and refuses nothing. -- **How it composes.** It is ORed with `approverId`, never ANDed. `?status=pending&approverId=&unstaffed=true` answers "my pending approvals, rescues included" in one call, paged server-side, and `total` counts both arms. Without `approverId` it selects the rescue requests alone. `unstaffed=false` is the same as leaving it out. -- **What is refused.** Any value but `true` or `false` (`1`, `TRUE`, an empty `?unstaffed=`) answers `400 VALIDATION_FAILED` with `fields[0]` `{ field: 'unstaffed', code: 'invalid_boolean' }`, and a repeated `?unstaffed=` answers the route's multiplicity `400`. Nothing changes for a caller who does not send it: `approverId` matches exactly what it matched before. -- **Before this.** The request was in no queue. `approverId` matches a slot against the addresses a caller names, and an override actor's addresses never equal an unstaffed `position:

` literal. So no "My Pending" listed it, no bell counted it, and a `lockRecord: true` record stayed locked until an admin browsed the unfiltered list. diff --git a/.changeset/22824-admin-rescue-named-slate.md b/.changeset/22824-admin-rescue-named-slate.md new file mode 100644 index 00000000000..0572f18f881 --- /dev/null +++ b/.changeset/22824-admin-rescue-named-slate.md @@ -0,0 +1,26 @@ +--- +'@objectstack/plugin-approvals': minor +'@objectstack/spec': patch +'@objectstack/lint': patch +--- + +feat(plugin-approvals): an approval step whose approvers resolve to no one opens on the organization's administrators under the default `onEmptyApprovers: 'admin_rescue'`, as ordinary approvers in their inbox + +Clause-②: yes + +**FROM → TO: who receives an `admin_rescue` request.** A node whose approvers resolve to no concrete person (an unstaffed position, an empty team or department, an unset manager, an expression that yields no one) under the default `onEmptyApprovers: 'admin_rescue'`: + +- **FROM:** the request opened on a slate that named no one, such as the literal `position:sales_manager`. It was in no one's "My Pending", the bell counted 0, no one was told, and a `lockRecord: true` record stayed locked until a platform or tenant admin found it under **All** and decided it through the privileged override. +- **TO:** the request opens on the request organization's administrators: the people who hold the organization-admin capability (`organization_admin` or `organization_admin_no_bypass`, ADR-0095 D3) in that organization. That is the same authority that lets a tenant admin override a request, resolved to the people who hold it, through the same grant resolution every request runs. An organization's owners and admins receive the capability automatically. + +They are ordinary approvers. The request is listed and counted for each of them under their own `approverId`, each is told once by `approval.requested`, out-of-office delegation applies to them, and a decision by one of them is an ordinary approval or rejection, recorded with `via_override: false`. The node's `behavior` reads them as one group. A platform administrator is not added for being one; one who also holds the organization's capability, such as its owner, is on the slate for that. + +The request says why. Its timeline carries a note, for example "Routed to this organization's administrators because the approver slate named no one: position 'sales_manager' has no holder.", and its node-config snapshot keeps the abandoned slate and the reason under `__resolvedFrom` (`admin_rescue:abandoned`, `admin_rescue:reason`). The abandoned slate is replaced, not kept beside the administrators, so a holder staffed into the position after the request opened does not receive it; an administrator can reassign it to them. + +**Unchanged.** A slate that names a person; `fallback`, `auto_approve` and `fail`; a `fallback` whose own list resolves to no one still degrades to `admin_rescue`, which now opens on the administrators. The privileged override stays for what the administrators' slate cannot reach: a request whose named approvers have all left after it opened, and an organization with no administrator (or a request with no organization), where the request still opens on the slate that names no one and the server logs a warning. + +**To name the person who should decide instead of the administrators,** declare `onEmptyApprovers: 'fallback'` with a `fallbackApprovers` list. + +**`@objectstack/spec`:** the `onEmptyApprovers` description says that `admin_rescue` opens on the organization's administrators and that `fallback` names who decides instead. + +**`@objectstack/lint`:** `approval-approvers-may-resolve-empty` no longer says the request "waits forever": it says the request opens on the organization's administrators, its remedy names `onEmptyApprovers: 'fallback'` as the way to name who decides, and its `os explain` text says the same. diff --git a/content/docs/automation/approvals.mdx b/content/docs/automation/approvals.mdx index c63dd2e3309..07906e21db6 100644 --- a/content/docs/automation/approvals.mdx +++ b/content/docs/automation/approvals.mdx @@ -104,13 +104,14 @@ through a provisioning path your own deployment supplies: this platform declares surface that authors its manager. Setting the column is still something you have to *do*. Where `manager_id` is unset the rung -expands to nobody, the request opens on a slate no one can act on, and under the default -`lockRecord: true` the record stays locked — recoverable only by a platform or tenant admin -override. So set it for everyone who submits this request, or take one of the two escapes that -need no write to the column at all: add a second approver entry that cannot resolve empty — e.g. -`{ type: 'org_membership_level', value: 'owner' }` — or declare `onEmptyApprovers: 'fallback'` -on the node with a `fallbackApprovers` list, so an empty manager rung opens the request on those -people instead. Neither makes `manager` resolve; they make the failure survivable, which is a +expands to nobody, and under the default `onEmptyApprovers: 'admin_rescue'` the request opens on +the organization's administrators instead of on the submitter's manager +([When a slate names no one](#when-a-slate-names-no-one)). So set it for everyone who submits this +request, or take one of the two escapes that need no write to the column at all: add a second +approver entry that cannot resolve empty — e.g. `{ type: 'org_membership_level', value: 'owner' }` +— or declare `onEmptyApprovers: 'fallback'` on the node with a `fallbackApprovers` list, so an +empty manager rung opens the request on those people instead of on the administrators. Neither +makes `manager` resolve; they route the request to the person who should decide it, which is a different and independently useful thing. `os lint` reports the shape at authoring time (`approval-approvers-may-resolve-empty`, `info`) and carries the same remedy text, so the two cannot drift apart — and it keeps reporting it even with a fallback declared, because a static @@ -289,16 +290,16 @@ empty slate you can see than a task the tenancy wall hides. A result may legitimately be **empty** (a present-but-empty field or variable); the node-level `onEmptyApprovers` policy decides what that means — -`admin_rescue` (default: the request opens, a privileged admin takes over via -Reassign), `fail` (the node fails: an empty slate is a config bug), +`admin_rescue` (default: the request opens on the organization's administrators — +[When a slate names no one](#when-a-slate-names-no-one)), `fail` (the node fails: an empty slate is a config bug), `auto_approve` (skip the request and continue down `approve` with `output.autoApproved = true`; opt-in, since it waves the record through), or `fallback` (open the request on the people named in `fallbackApprovers`). A **missing** key (`vars.never_written`) is a loud error instead — guard genuinely-optional inputs with `has(vars.x) ? vars.x : []`. -`fallback` is the only one of the four that **names people**, so it is the only -one that turns an empty slate into a request someone can actually decide: +`fallback` is how a node **names who decides** an empty slate, instead of leaving it to +the organization's administrators: ```ts { @@ -320,8 +321,9 @@ before the node opens any approval request. Prefer a fallback target that cannot itself resolve empty (a literal `user`, or the `owner` membership tier). If it does resolve empty the node degrades to -`admin_rescue` — the request still opens and the run is never killed — and the -log says both that the fallback fired and that it found nobody. +`admin_rescue` — the request opens on the organization's administrators and the +run is never killed — and the log says both that the fallback fired and that it +found nobody. The "previous approver picks the next step's approvers" loop needs no record field at all: declare `decisionOutputs: ['next_reviewers']` on node A, have the @@ -349,8 +351,9 @@ where nothing reads the outputs. Enforcement has no elevation bypass — a one-click email action link and an `auto_approve` SLA escalation both fail rather than advance into a node that would resolve nobody, leaving the request pending and visibly overdue. Without the flag your only backstop is -`onEmptyApprovers` — the next node opens, resolves nobody, and stalls for an -admin rescue long after the approver who could have filled it in has gone. +`onEmptyApprovers` — the next node opens, resolves nobody, and lands on the +organization's administrators long after the approver who could have filled it in +has gone. ## The full lifecycle — submit to field change @@ -409,8 +412,47 @@ What an entry that resolves to nobody leaves on the slate depends on its kind: - **A slate left empty, or holding only literals,** goes to the node's `onEmptyApprovers` policy, whose four values are listed under [Approving across organizations](#cross-org-approvers). Under the default, - `admin_rescue`, the request opens on that slate and waits for an - administrator, or for a holder of a `position:` literal on it. + `admin_rescue`, the request opens on the organization's administrators + instead. + +#### When a slate names no one [#when-a-slate-names-no-one] + +Under the default `onEmptyApprovers: 'admin_rescue'`, a slate that names no +person opens the request on the **organization's administrators**: the people +who hold the organization-admin capability (`organization_admin` or +`organization_admin_no_bypass`, ADR-0095 D3) in the request's organization. An +organization's owners and admins receive that capability automatically. It is +the same authority that lets a tenant admin override a request, resolved to the +people who hold it. + +They are ordinary approvers: + +- the request is in each administrator's queue and counted in their bell, under + their own `approverId`, as for any approver; +- each is told once, by `approval.requested`, and out-of-office delegation + applies to them as to any named approver; +- a decision by one of them is an ordinary approval or rejection, recorded with + `via_override: false`. The node's `behavior` reads them as one group: under + `first_response`, or `per_group` with the default `minApprovals`, one of them + decides; `unanimous` and `quorum` count them as they count any slate. + +A platform administrator is not added for being one: that standing crosses the +tenant wall, and the slate is the organization's own. One who also holds the +organization's capability, such as its owner, is on the slate for that. + +The request says why. Its timeline carries a note, for example *Routed to this +organization's administrators because the approver slate named no one: position +'sales_manager' has no holder.*, and its node-config snapshot keeps the abandoned +slate and the same reason under `__resolvedFrom` (`admin_rescue:abandoned`, +`admin_rescue:reason`). The abandoned slate is replaced, not kept beside them: a +holder staffed into the position after the request opened does not receive it, +and an administrator can reassign it to them. + +To name the person who should decide instead, declare +`onEmptyApprovers: 'fallback'` with `fallbackApprovers`. Only an organization +with no administrator, or a request with no organization, still opens on the +slate that names no one: the server logs a warning, no one is told, and only the +admin override decides it. ### The approver finds it in their queue @@ -453,28 +495,7 @@ position under as their own identity. The deprecated pre-rename prefix is no alias of it (ADR-0090 D3): it matches only a slot stored under that exact spelling, and no holder acts under it. Other filters: `status`, `object`, `recordId`, `submitterId`, `q`, `limit`, -`offset`, and `unstaffed`. - -`unstaffed=true` adds the pending requests whose slate names no person: only -unstaffed literals such as `position:`, or nothing at all. This is what -`admin_rescue` opens. They -are added only for a caller who may override them: an admin with override -authority (`admin_full_access`, or `organization_admin` within their -organization). Every such row reads `viewer.can_override: true`. For anyone else the parameter -adds nothing. It is ORed with `approverId`, never ANDed, so one call answers -"my pending approvals, rescues included", and its `total` counts both: - -```bash -curl -b cookies.txt \ - "https://your-app.example.com/api/v1/approvals/requests?status=pending&approverId=usr_123&unstaffed=true" -``` - -It reads `true` or `false` only (`false` is the same as leaving it out); any -other value is refused with `400`. The test reads the slate, not the directory: -a `position:` slot whose position gained a holder after the request opened -still matches (that holder also finds it under `approverId`), and a slate that -still names a person never matches, even if that person has since left. -Reassign such a request from the **All** view. +and `offset`. **`approverId` is a filter, not authorization.** What you may see is decided @@ -499,15 +520,17 @@ question back to this service via `fileAccessDelegate`). **Opening a step tells its approvers.** When a request opens, each concrete approver on its slate gets one `approval.requested` notification ("Approval requested", linked to that request in the Approvals Inbox): a named user, every -holder of a staffed position or member of a team, and the `fallbackApprovers` -when the node's `onEmptyApprovers: 'fallback'` opened on them. It goes through +holder of a staffed position or member of a team, the `fallbackApprovers` +when the node's `onEmptyApprovers: 'fallback'` opened on them, and the +organization's administrators when the default `admin_rescue` opened on them. It goes through the installed messaging service, the same way as the reminder, reassignment, escalation, and return notifications, and the same delivery preferences apply. Without a messaging service, no approval notification is delivered at all. Two cases tell nobody by design. A slot no one holds (`position:` with -no holders) names no person, so a step that resolves to nobody notifies no -one: its `onEmptyApprovers` policy decides what happens instead. An +no holders) names no person, so it is told nothing; a step whose whole slate +names no one is decided by its `onEmptyApprovers` policy, and only in an +organization with no administrator does the default leave no one to tell. An out-of-office delegate gets one message, `approval.ooo_substituted`, not a second one for the opening. @@ -711,9 +734,12 @@ admin can rescue a stuck request. The service stays the sole authority — the predicate only trims the UI. -**Admin override — recovering a stuck request.** An approval routed to a -`position` / `team` / `department` with **no holders** resolves to only its -`:` literal in `pending_approvers`. A `position:` slot is +**Admin override — recovering a stuck request.** A slate that names no one +opens on the organization's administrators under the default `admin_rescue` +([When a slate names no one](#when-a-slate-names-no-one)), so the override is +break-glass for what that cannot reach: a request whose named approvers have all +left after it opened, and an organization with no administrator, where the +request opens on its `:` literals. A `position:` slot is decided by whoever holds that position once someone is staffed into it; until then — and for a `team` or `department` literal, always — no concrete user can act, and (with `lockRecord`) the record stays locked. A **platform admin** diff --git a/content/docs/permissions/tenant-audit-census.mdx b/content/docs/permissions/tenant-audit-census.mdx index e7dc580c560..1d30a19aa13 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 **53 of the 231 sites are spelled that way**. A +forwarding shim cannot, and **53 of the 232 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 | **231** | +| 175 write call sites | quoted in the merged changeset | **232** | | 24 carrying no tenant context | quoted in the merged changeset | **2** provable and tenancy-enabled; **32** more whose options argument is unreadable | -| 127 of 175 statically decidable, 48 runtime-parameter-name sites | restated on the `isSystem`-scoping card | **156 of 231** decidable, **75** undecidable | -| 135 (77%) silenced by the `isSystem` guard before the posture gate | the lost issue body — **no surviving corroboration** | **not reproduced**: 126 decidably elevated, 0 decidably not, 97 undecidable | +| 127 of 175 statically decidable, 48 runtime-parameter-name sites | restated on the `isSystem`-scoping card | **157 of 232** decidable, **75** undecidable | +| 135 (77%) silenced by the `isSystem` guard before the posture gate | the lost issue body — **no surviving corroboration** | **not reproduced**: 127 decidably elevated, 0 decidably not, 97 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 -126 of 231 (55%) as decidably elevated, with 97 more whose elevation is a +127 of 232 (55%) as decidably elevated, with 97 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 `2 / 231`, and say what it is**: the sites whose options argument was +⇒ **Cite `2 / 232`, 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" — **32 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 | **231** | -| …whose object name is statically decidable | 156 | +| write call sites on the application surface | **232** | +| …whose object name is statically decidable | 157 | | …whose object name is chosen at run time | 75 | -| …against an object with tenancy ENABLED | 155 | +| …against an object with tenancy ENABLED | 156 | | …against an object that declares tenancy off | 1 | -| threading a tenant context | 170 | +| threading a tenant context | 171 | | PROVABLY carrying none (options read, no context key) | **8** | | …of those, against a decidably tenancy-enabled object | **2** | | options argument UNREADABLE — may or may not carry one | 53 | | …of those, against a decidably tenancy-enabled object | 32 | -| threading a decidably ELEVATED (`isSystem`) context | 126 | +| threading a decidably ELEVATED (`isSystem`) context | 127 | | threading a context that is decidably NOT elevated | 0 | | threading a context whose elevation is a run-time fact | 97 | | how the instrument reached the site | count | | :--- | ---: | -| receiver carried a readable engine type | 187 | +| receiver carried a readable engine type | 188 | | receiver erased, placed by the object NAME | 30 | | receiver erased, placed by an `object: string` PARAMETER | 9 | | receiver erased, placed by an `UNTYPED_RECEIVERS` row | 5 | -| object name spelled inline | 104 | +| object name spelled inline | 105 | | object name spelled through a `const` | 52 | | object name is an `object: string` parameter | 13 | | object name is some other run-time expression | 62 | @@ -297,7 +297,7 @@ 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-11 at `f6bce8aaf`. +Measured on 2026-10-11 at `5d0a4a152`. | corpus scale (not enforced) | count | | :--- | ---: | diff --git a/content/docs/references/automation/approval.mdx b/content/docs/references/automation/approval.mdx index 46d7f2a76aa..1045d84de36 100644 --- a/content/docs/references/automation/approval.mdx +++ b/content/docs/references/automation/approval.mdx @@ -73,7 +73,7 @@ const result = ApprovalDecision.parse(data); | **minApprovals** | `integer` | optional | Approvals required — total (quorum) or per group (per_group). Omitted ⇒ all resolvable approvers for quorum, 1 per group for per_group | | **lockRecord** | `boolean` | optional (default: `true`) | Lock the record from editing while pending | | **approvalStatusField** | `string` | optional | Business-object field to mirror request status onto | -| **onEmptyApprovers** | `Enum<'admin_rescue' \| 'fail' \| 'auto_approve' \| 'fallback'>` | optional (default: `"admin_rescue"`) | Behavior when no concrete approver resolves at node entry — 'fallback' opens the request on fallbackApprovers instead | +| **onEmptyApprovers** | `Enum<'admin_rescue' \| 'fail' \| 'auto_approve' \| 'fallback'>` | optional (default: `"admin_rescue"`) | Behavior when no concrete approver resolves at node entry — 'admin_rescue' (default) opens the request on the organization's administrators; 'fallback' opens it on fallbackApprovers instead, naming who decides | | **fallbackApprovers** | `{ type: Enum<'manager' \| 'position' \| 'department' \| 'team' \| 'field' \| 'expression' \| …>; value?: string; resolveAs?: Enum<'user' \| 'department' \| 'position' \| 'team'>; group?: string; … }[]` | optional | Approvers the request opens on when onEmptyApprovers is 'fallback' | | **decisionOutputs** | `(string \| { key: string; label?: string; type?: Enum<'text' \| 'user' \| 'department' \| 'position' \| 'team'>; multiple?: boolean; … })[]` | optional | Author-declared decision outputs — bare keys or typed `{ key, type, multiple }` declarations | | **escalation** | `{ enabled?: boolean; timeoutHours: number; action?: Enum<'reassign' \| 'auto_approve' \| 'auto_reject' \| 'notify'>; escalateTo?: string; … }` | optional | Per-node SLA escalation | 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 65787ba01f4..e296a8a5c67 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 | 231 | -| Object name statically decidable | 156 | +| Write call sites | 232 | +| Object name statically decidable | 157 | | Object name chosen at run time | 75 | -| Against a tenancy-enabled object | 155 | +| Against a tenancy-enabled object | 156 | | Against an object declaring tenancy off | 1 | -| Threading a tenant context | 170 | +| Threading a tenant context | 171 | | Provably carrying none | 8 | | …and decidably tenancy-enabled | 2 | | Options argument unreadable | 53 | | …and decidably tenancy-enabled | 32 | -| Threading a decidably elevated context | 126 | +| Threading a decidably elevated context | 127 | | Threading a decidably non-elevated context | 0 | | Threading a context of undecidable elevation | 97 | @@ -90,7 +90,7 @@ 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-11 at `f6bce8aaf`. +Measured on 2026-10-11 at `5d0a4a152`. | corpus scale (not enforced) | count | | :--- | ---: | @@ -107,7 +107,7 @@ Measured on 2026-10-11 at `f6bce8aaf`. | `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` | `insert` | `sys_approval_action` | enabled | elevated | 15 | | `packages/plugins/plugin-approvals/src/approval-service.ts` | `delete` | `sys_approval_approver` | enabled | elevated | 2 | | `packages/plugins/plugin-approvals/src/approval-service.ts` | `insert` | `sys_approval_approver` | enabled | elevated | 2 | | `packages/plugins/plugin-approvals/src/approval-service.ts` | `insert` | `sys_approval_request` | enabled | elevated | 1 | diff --git a/packages/lint/src/rule-explanations.ts b/packages/lint/src/rule-explanations.ts index 44e698bde45..d0cebdf05c6 100644 --- a/packages/lint/src/rule-explanations.ts +++ b/packages/lint/src/rule-explanations.ts @@ -2043,10 +2043,14 @@ const APPROVAL_APPROVERS_MAY_RESOLVE_EMPTY_EXPLANATION: RuleExplanation = { paragraphs: [ 'An approval node opens its request on the people its approvers expand to at runtime. When ' + 'EVERY approver on the node routes to a group whose membership is runtime data — a ' + - '`position`, a `team` or a `department` — and none is staffed, the request opens on an empty ' + - '`pending_approvers` slate: no user can act, so it waits forever, and under the default ' + - '`lockRecord: true` the record stays locked with it, with no in-product recovery. An approver ' + - 'of any other type on the node is a non-group route, so a mixed slate is not judged.', + '`position`, a `team` or a `department` — and none is staffed, no person the node names can ' + + 'act. Under the default `onEmptyApprovers: \'admin_rescue\'` the request then opens on the ' + + "organization's administrators (the people holding its organization-admin capability) " + + 'instead, and under the default `lockRecord: true` the record stays locked until one of them ' + + 'decides. Declare `onEmptyApprovers: \'fallback\'` with a `fallbackApprovers` list to name who ' + + 'decides instead; only an organization with no administrator leaves the request on an empty ' + + '`pending_approvers` slate, for a platform administrator\'s override. An approver of any ' + + 'other type on the node is a non-group route, so a mixed slate is not judged.', 'The `manager` arm is the same dead end with a cause the product cannot repair. ' + '`{ type: \'manager\' }` resolves from `sys_user.manager_id` of the user the record names ' + '(the field `value` names, else the owner), and returns nobody where that column is unset; a ' + diff --git a/packages/lint/src/validate-approval-approvers.test.ts b/packages/lint/src/validate-approval-approvers.test.ts index 9fa294e12ed..27dc9ddbfeb 100644 --- a/packages/lint/src/validate-approval-approvers.test.ts +++ b/packages/lint/src/validate-approval-approvers.test.ts @@ -737,13 +737,13 @@ describe('[#22161] one-line verdicts — approval-approvers-may-resolve-empty, a const [group] = validateApprovalApprovers(stackWithApprovers([{ type: 'position', value: 'exec' }])); expect(group.message).toBe( 'every approver on this node routes to a group (position/team/department), so if none is ' + - 'staffed the request opens on an empty slate and waits forever, and (lockRecord) the record ' + + "staffed the request opens on the organization's administrators, and (lockRecord) the record " + 'stays locked', ); const [manager] = validateApprovalApprovers(stackWithApprovers([{ type: 'manager' }])); expect(manager.message).toBe( "every approver on this node is { type: 'manager' }, so where sys_user.manager_id is unset the " + - 'request opens on an empty slate and waits forever, and (lockRecord) the record stays locked', + "request opens on the organization's administrators, and (lockRecord) the record stays locked", ); }); @@ -761,7 +761,7 @@ describe('[#22161] one-line verdicts — approval-approvers-may-resolve-empty, a const facts: Record = { [APPROVAL_APPROVERS_MAY_RESOLVE_EMPTY]: [ '`pending_approvers`', - 'no in-product recovery', + "the organization's administrators", '`sys_user.manager_id`', 'a seed row of `sys_user`', "`onEmptyApprovers: 'fallback'` does not silence it", diff --git a/packages/lint/src/validate-approval-approvers.ts b/packages/lint/src/validate-approval-approvers.ts index 19f0419c54f..cb2ff830d5b 100644 --- a/packages/lint/src/validate-approval-approvers.ts +++ b/packages/lint/src/validate-approval-approvers.ts @@ -525,9 +525,10 @@ export function validateApprovalApprovers(stack: AnyRec): ApprovalApproverFindin // Empty-slate dead-end (#3424): when EVERY approver on the node routes to // a group whose membership can be empty (an unstaffed position, an empty - // team/department), the request can resolve to an empty `pending_approvers` - // at runtime — no concrete user can act, and with `lockRecord` the record - // stays locked with no recovery except a platform/tenant admin override. + // team/department), no person the node names can act at runtime, and + // under the default `admin_rescue` the request opens on the + // organization's administrators instead (#22824), the record locked + // under `lockRecord` until one of them decides. // Advisory (`info`): staffing is runtime data a linter can't see, so this // flags the risky SHAPE and prescribes a guaranteed-staffed fallback. const routable = approvers.filter( @@ -547,15 +548,15 @@ export function validateApprovalApprovers(stack: AnyRec): ApprovalApproverFindin // what recovers a stuck request is `os explain` text. message: `every approver on this node routes to a group (position/team/department), so if ` + - `none is staffed the request opens on an empty slate and waits forever` + + `none is staffed the request opens on the organization's administrators` + (locks ? STAYS_LOCKED : ''), hint: `Make sure at least one target is always staffed, or add a guaranteed-staffed ` + `approver entry, e.g. { type: 'org_membership_level', value: 'owner' }, or declare ` + - `onEmptyApprovers: 'fallback' with a fallbackApprovers list for the node to open on ` + - `when the groups come back empty. A request that still lands empty is recoverable ` + - `only by a platform/tenant admin override, which may act on any pending request so that ` + - `one nobody in its slate can decide never stays stuck.`, + `onEmptyApprovers: 'fallback' with a fallbackApprovers list to name who decides ` + + `when the groups come back empty. A request that still lands empty opens on the ` + + `organization's administrators; only an organization with no administrator leaves it ` + + `to a platform administrator's override.`, }); } @@ -599,13 +600,14 @@ export function validateApprovalApprovers(stack: AnyRec): ApprovalApproverFindin // runtime fact; that reasoning is `os explain` text. message: `every approver on this node is { type: 'manager' }, so where sys_user.manager_id ` + - `is unset the request opens on an empty slate and waits forever` + + `is unset the request opens on the organization's administrators` + (locks ? STAYS_LOCKED : ''), hint: `${MANAGER_ONLY_REMEDY} ${MANAGER_ONLY_ROUTES} Populate it for everyone who submits ` + `this request, or take the other escape that needs none of that: add a second approver ` + `entry which cannot resolve empty, e.g. { type: 'org_membership_level', value: 'owner' }. ` + - `A request that still lands empty is recoverable only by a platform/tenant admin override.`, + `A request that still lands empty opens on the organization's administrators; to name who ` + + `decides instead, declare onEmptyApprovers: 'fallback' with a fallbackApprovers list.`, }); } diff --git a/packages/plugins/plugin-approvals/src/admin-rescue-named-slate.integration.test.ts b/packages/plugins/plugin-approvals/src/admin-rescue-named-slate.integration.test.ts new file mode 100644 index 00000000000..efea3fb0d6d --- /dev/null +++ b/packages/plugins/plugin-approvals/src/admin-rescue-named-slate.integration.test.ts @@ -0,0 +1,326 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * An empty `admin_rescue` slate opens on the organization's administrators, + * as ordinary approvers (#22824). + * + * ## The defect + * + * A node whose approvers resolved to no concrete person opened, under the + * default `onEmptyApprovers: 'admin_rescue'`, on a slate that named no one + * (the literal `position:

`). No "My Pending" listed it, the bell counted 0, + * the `approval.requested` fan-out told no one, and only the #3424 override + * could decide it — measured on every fresh HotCRM install's default + * sign-offs. + * + * ## What this pins + * + * Driven through the real `ApprovalService` on a real `ObjectQL` over + * `SqlDriver` (better-sqlite3, in-memory, real DDL), with the real + * `resolveUserAuthzGrants` reading a bound security catalog, and the messaging + * surface recording what reaches it: + * + * - a position-only slate with no holder opens on the organization's + * administrators — each holder of `organization_admin` or + * `organization_admin_no_bypass` in the request's organization — and the + * request is listed and counted for each of them under their own + * `approverId`, and each is told once; + * - a tenant administrator's decision is an ordinary approval + * (`via_override: false`); + * - a member sees nothing and is told nothing; a platform administrator is not + * on the slate for being one; another organization's administrator is not + * on it; + * - the request says why: a timeline note and `__resolvedFrom`; + * - an administrator out of office is covered by their delegate; + * - CONTROLS: a staffed slate is unchanged; `fallback`, `auto_approve` and + * `fail` are unchanged; an organization with no administrator still gets + * the request that names no one, which the override decides. + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { ObjectQL } from '@objectstack/objectql'; +import { SqlDriver } from '@objectstack/driver-sql'; +import { bindSecurityCatalogReader, type SecurityCatalogEntry, type SecurityCatalogReader } from '@objectstack/core'; +import { ApprovalService } from './approval-service.js'; +import { ApprovalsServicePlugin } from './approvals-plugin.js'; + +const SYSTEM = { isSystem: true, positions: [], permissions: [] } as any; + +const ORG = 'org_a'; +const OTHER_ORG = 'org_b'; +/** An organization whose only member holds no administrator capability. */ +const LEADERLESS_ORG = 'org_c'; + +/** The acting envelope of a member of `org`, with the grants it resolved to. */ +const as = (userId: string, org: string, permissions: string[] = []) => + ({ userId, tenantId: org, positions: [], permissions }) as any; +const OWNER = as('u_owner', ORG, ['organization_admin']); +const ADMIN = as('u_admin', ORG, ['organization_admin_no_bypass']); +const MEMBER = as('u_member', ORG); +const ROOT = as('u_root', ORG, ['admin_full_access']); + +const USERS = [ + 'u_sub', 'u_owner', 'u_admin', 'u_member', 'u_root', 'u_other_owner', 'u_alice', 'u_fallback', 'u_cover', + 'u_lonely', +].map((id) => ({ id, name: id, email: `${id}@example.com` })); + +/** `sys_member`: who is in which organization, by better-auth role. */ +const MEMBERS: Array<[string, string, string]> = [ + ['u_sub', ORG, 'member'], + ['u_owner', ORG, 'owner'], + ['u_admin', ORG, 'admin'], + ['u_member', ORG, 'member'], + ['u_root', ORG, 'member'], + ['u_other_owner', OTHER_ORG, 'owner'], + ['u_lonely', LEADERLESS_ORG, 'member'], +]; + +/** + * `sys_user_permission_set`: the grants the authorization resolver reads. The + * org-admin grants are what `auto-org-admin-grant.ts` provisions from the + * owner/admin role; the platform administrator's grant is unscoped. + */ +const GRANTS: Array<[string, string, string | null]> = [ + ['u_owner', 'organization_admin', ORG], + ['u_admin', 'organization_admin_no_bypass', ORG], + ['u_root', 'admin_full_access', null], + ['u_other_owner', 'organization_admin', OTHER_ORG], +]; + +const text = (name: string) => ({ name, label: name, type: 'text' as const }); +const objectDef = (name: string, fields: string[]) => ({ + name, + label: name, + fields: Object.fromEntries([ + ['id', { ...text('id'), primaryKey: true }], + ...fields.map((f) => [f, text(f)]), + ]), +}); + +/** A catalog serving exactly the permission sets this suite grants. */ +function catalogServing(names: string[]): SecurityCatalogReader { + const entry = (name: string): SecurityCatalogEntry => ({ + type: 'permission', name, definition: { name }, source: 'registry', + }); + return { + async resolve(type, name) { + return type === 'permission' && names.includes(name) ? entry(name) : undefined; + }, + async list(type) { + return type === 'permission' ? names.map(entry) : []; + }, + }; +} + +/** The plugin's own object definitions, as its manifest registers them. */ +async function packageObjects(): Promise { + let manifest: any; + const ctx: any = { + getService: (name: string) => { + if (name === 'manifest') return { register: (m: any) => { manifest = m; } }; + throw new Error(`no service '${name}'`); + }, + logger: { info: () => {}, warn: () => {}, error: () => {}, debug: () => {} }, + }; + await new ApprovalsServicePlugin().init(ctx); + return manifest.objects; +} + +interface Emitted { + topic: string; + audience: string[]; + source?: { object: string; id: string }; +} + +describe('an empty admin_rescue slate opens on the organization\'s administrators (#22824)', () => { + let engine: ObjectQL; + let svc: ApprovalService; + let now = Date.parse('2026-10-11T09:00:00.000Z'); + const emits: Emitted[] = []; + const warnings: string[] = []; + + const open = async (key: string, approvers: any[], extra: Record = {}, org = ORG): Promise => { + await engine.insert('crm_deal', { id: `D_${key}`, title: key }, { context: SYSTEM } as any); + return svc.openNodeRequest({ + object: 'crm_deal', recordId: `D_${key}`, runId: `run_${key}`, nodeId: 'review', flowName: 'deal_review', + config: { approvers, behavior: 'first_response', ...extra } as any, + submitterId: 'u_sub', record: { id: `D_${key}`, title: key }, organizationId: org, + }, as('u_sub', org)); + }; + const UNSTAFFED = [{ type: 'position', value: 'sales_manager' }]; + + /** Who was told on `approval.requested` about one request, one entry per message. */ + const told = (requestId: string) => emits + .filter((e) => e.topic === 'approval.requested' && e.source?.id === requestId) + .flatMap((e) => e.audience).sort(); + /** My Pending and the bell, for one caller: their own approver identities, both faces. */ + const myPending = async (ctx: any) => { + const filter = { status: 'pending' as const, approverId: [ctx.userId, `${ctx.userId}@example.com`] }; + const rows = await svc.listRequests(filter, ctx); + return { ids: rows.map((r) => r.id).sort(), total: await svc.countRequests(filter, ctx) }; + }; + const actionsOf = async (requestId: string): Promise => engine.find('sys_approval_action', { + where: { request_id: requestId }, context: SYSTEM, + }); + const snapshotOf = async (requestId: string) => { + const [row]: any[] = await engine.find('sys_approval_request', { where: { id: requestId }, context: SYSTEM }); + return JSON.parse(row.node_config_json); + }; + + beforeAll(async () => { + engine = new ObjectQL(); + engine.registerDriver(new SqlDriver({ + client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true, + }), true); + await engine.init(); + const defs = [ + objectDef('sys_user', ['name', 'email']), + objectDef('sys_member', ['user_id', 'organization_id', 'role']), + objectDef('sys_user_position', ['user_id', 'position', 'organization_id']), + objectDef('sys_user_permission_set', ['user_id', 'permission_set', 'organization_id']), + objectDef('crm_deal', ['title']), + ...(await packageObjects()), + ]; + for (const def of defs) engine.registry.registerObject(def as any, 'approvals-test', 'approvals-test'); + await engine.syncSchemas(); + bindSecurityCatalogReader(engine, catalogServing([ + 'organization_admin', 'organization_admin_no_bypass', 'admin_full_access', + ])); + for (const u of USERS) await engine.insert('sys_user', u, { context: SYSTEM } as any); + for (const [i, [user_id, organization_id, role]] of MEMBERS.entries()) { + await engine.insert('sys_member', { id: `m_${i}`, user_id, organization_id, role }, { context: SYSTEM } as any); + } + for (const [i, [user_id, permission_set, organization_id]] of GRANTS.entries()) { + await engine.insert('sys_user_permission_set', { id: `ups_${i}`, user_id, permission_set, organization_id }, + { context: SYSTEM } as any); + } + + svc = new ApprovalService({ + engine: engine as any, + clock: { now: () => new Date(now += 1000) }, + logger: { + warn: (msg: string) => { warnings.push(String(msg)); }, info: () => {}, error: () => {}, debug: () => {}, + } as any, + }); + svc.attachMessaging({ emit: (async (input: Emitted) => { emits.push(input); }) as any }); + }); + + afterAll(async () => { + try { await engine?.destroy(); } catch { /* noop */ } + }); + + it('a position nobody holds opens on the organization\'s administrators, in each one\'s queue and bell, each told once', async () => { + const row = await open('rescue', UNSTAFFED); + expect(row.status).toBe('pending'); + expect([...row.pending_approvers].sort()).toEqual(['u_admin', 'u_owner']); + + for (const admin of [OWNER, ADMIN]) { + expect(await myPending(admin), admin.userId).toEqual({ ids: [row.id], total: 1 }); + // Ordinary approvers: the slot is theirs, not an override. + const seen = await svc.getRequest(row.id, admin); + expect((seen as any)?.viewer?.can_act, admin.userId).toBe(true); + } + expect(told(row.id)).toEqual(['u_admin', 'u_owner']); + }); + + it('a member sees nothing and is told nothing; a platform admin and another organization\'s admin are not on the slate', async () => { + const row = await open('outsiders', UNSTAFFED); + // The scene first: the slate IS the administrators', so the absences + // below are not read off a slate that names no one at all. + expect([...row.pending_approvers].sort()).toEqual(['u_admin', 'u_owner']); + expect(row.pending_approvers).not.toContain('u_member'); + expect(row.pending_approvers).not.toContain('u_root'); + expect(row.pending_approvers).not.toContain('u_other_owner'); + + expect(await myPending(MEMBER)).toEqual({ ids: [], total: 0 }); + expect(await myPending(ROOT)).toEqual({ ids: [], total: 0 }); + expect(told(row.id)).not.toContain('u_member'); + expect(told(row.id)).not.toContain('u_root'); + expect(told(row.id)).not.toContain('u_other_owner'); + }); + + it('a tenant administrator\'s decision is an ordinary approval, not an override', async () => { + const row = await open('decided', UNSTAFFED); + const out = await svc.decide(row.id, { decision: 'approve', actorId: 'u_owner' } as any, OWNER); + expect([out.finalized, out.request.status]).toEqual([true, 'approved']); + const decision = (await actionsOf(row.id)).find((a) => a.action === 'approve'); + expect(decision).toMatchObject({ actor_id: 'u_owner', acted_as: 'u_owner' }); + expect(Boolean(decision?.via_override)).toBe(false); + }); + + it('the request says why: a timeline note and the abandoned slate in __resolvedFrom', async () => { + const row = await open('why', UNSTAFFED); + const notes = (await actionsOf(row.id)).filter((a) => a.action === 'comment'); + expect(notes).toHaveLength(1); + expect(notes[0].actor_id ?? null).toBeNull(); + expect(notes[0].comment).toContain("position 'sales_manager' has no holder"); + expect((await snapshotOf(row.id)).__resolvedFrom).toEqual({ + 'admin_rescue:abandoned': ['position:sales_manager'], + 'admin_rescue:reason': "position 'sales_manager' has no holder", + }); + }); + + it('a fallback that resolves to no one degrades to the administrators, and says so', async () => { + const row = await open('fallback_empty', UNSTAFFED, { + onEmptyApprovers: 'fallback', fallbackApprovers: [{ type: 'position', value: 'treasury' }], + }); + expect([...row.pending_approvers].sort()).toEqual(['u_admin', 'u_owner']); + expect((await snapshotOf(row.id)).__resolvedFrom['admin_rescue:reason']).toBe( + "position 'sales_manager' has no holder; the declared fallbackApprovers resolved to no one either", + ); + }); + + // Last of the cases whose slate is `org_a`'s administrators: the delegation + // it writes stays, and no case after it reads that slate. + it('an administrator out of office is covered by their delegate, as any named approver is', async () => { + await engine.insert('sys_approval_delegation', { + id: 'dlg_admin', delegator_id: 'u_admin', delegate_id: 'u_cover', reason: 'leave', + valid_from: '2026-09-01T00:00:00.000Z', valid_until: '2026-12-31T00:00:00.000Z', + created_at: '2026-09-01T00:00:00.000Z', updated_at: '2026-09-01T00:00:00.000Z', + }, { context: SYSTEM } as any); + const row = await open('cover', UNSTAFFED); + expect([...row.pending_approvers].sort()).toEqual(['u_cover', 'u_owner']); + expect(told(row.id)).toEqual(['u_owner']); + expect(emits.filter((e) => e.topic === 'approval.ooo_substituted' && e.source?.id === row.id) + .flatMap((e) => e.audience)).toEqual(['u_cover']); + }); + + describe('controls', () => { + it('a staffed slate is unchanged: no administrator is added, and no note is written', async () => { + const row = await open('staffed', [{ type: 'user', value: 'u_alice' }]); + expect(row.pending_approvers).toEqual(['u_alice']); + expect(told(row.id)).toEqual(['u_alice']); + expect((await actionsOf(row.id)).map((a) => a.action)).toEqual(['submit']); + }); + + it('fallback opens on its declared approvers, not on the administrators', async () => { + const row = await open('fallback', UNSTAFFED, { + onEmptyApprovers: 'fallback', fallbackApprovers: [{ type: 'user', value: 'u_fallback' }], + }); + expect(row.pending_approvers).toEqual(['u_fallback']); + expect(told(row.id)).toEqual(['u_fallback']); + }); + + it('auto_approve opens nothing and fail refuses, as before', async () => { + expect(await open('waved', UNSTAFFED, { onEmptyApprovers: 'auto_approve' })) + .toEqual({ autoApproved: true, reason: 'empty_approvers' }); + await expect(open('refused', UNSTAFFED, { onEmptyApprovers: 'fail' })).rejects.toThrow(/^NO_APPROVERS:/); + }); + + it('an organization with no administrator gets the request that names no one, said loudly, and the override decides it', async () => { + const before = warnings.length; + const row = await open('leaderless', UNSTAFFED, {}, LEADERLESS_ORG); + expect(row.pending_approvers).toEqual(['position:sales_manager']); + expect(told(row.id)).toEqual([]); + expect(warnings.slice(before).some((w) => w.includes(`organization '${LEADERLESS_ORG}' has no administrator`))) + .toBe(true); + // Break-glass stays break-glass: a platform administrator holds no slot + // and decides it through the #3424 override, recorded as one. + const root = as('u_root', LEADERLESS_ORG, ['admin_full_access']); + await svc.decide(row.id, { decision: 'approve', actorId: 'u_root' } as any, root); + const decision = (await actionsOf(row.id)).find((a) => a.action === 'approve'); + expect(decision).toMatchObject({ actor_id: 'u_root' }); + expect(Boolean(decision?.via_override)).toBe(true); + }); + }); +}); diff --git a/packages/plugins/plugin-approvals/src/approval-requested-notification.integration.test.ts b/packages/plugins/plugin-approvals/src/approval-requested-notification.integration.test.ts index b7827430cec..e56e5a55bdd 100644 --- a/packages/plugins/plugin-approvals/src/approval-requested-notification.integration.test.ts +++ b/packages/plugins/plugin-approvals/src/approval-requested-notification.integration.test.ts @@ -25,9 +25,11 @@ * literal (an unstaffed position) names no one; * - the control: a step whose approvers resolve to nobody notifies no one, * and the node's `onEmptyApprovers` policy is what decides that case — - * `admin_rescue` opens on literals and tells nobody, `auto_approve` and - * `fail` open nothing, `fallback` opens on — and tells — its fallback - * approvers; + * `admin_rescue` with no organization administrator to route to (none here) + * opens on literals and tells nobody, `auto_approve` and `fail` open + * nothing, `fallback` opens on — and tells — its fallback approvers. The + * administrators an `admin_rescue` request opens on, and their messages, + * are `admin-rescue-named-slate.integration.test.ts`'s; * - an out-of-office delegate is told once, by `approval.ooo_substituted`, * never a second time by the opening; * - a channel that throws never fails the opening. @@ -217,7 +219,7 @@ describe('opening an approval step tells each resolved approver, once (approval. describe('control — a step whose approvers resolve to nobody notifies no one; onEmptyApprovers decides it', () => { const UNSTAFFED = [{ type: 'position', value: 'treasury' }]; - it('admin_rescue (the default) opens on the literal and tells nobody', async () => { + it('admin_rescue (the default), with no organization administrator, opens on the literal and tells nobody', async () => { const row = await open('rescue', UNSTAFFED); expect([row.status, row.pending_approvers]).toEqual(['pending', ['position:treasury']]); expect(messagesAbout(row.id)).toEqual([]); diff --git a/packages/plugins/plugin-approvals/src/approval-service.test.ts b/packages/plugins/plugin-approvals/src/approval-service.test.ts index 1a025e6f9e5..87528cf3113 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.test.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.test.ts @@ -2066,200 +2066,6 @@ describe('ApprovalService — admin override (#3424)', () => { }); }); -// ── #22725: the `unstaffed` arm of the inbox read ──────────────────────────── -// -// An `onEmptyApprovers: 'admin_rescue'` request opens on a slate that names no -// person, so it was in nobody's queue: `approverId` matches a slot against the -// addresses a caller names, and an override actor's addresses never equal an -// unstaffed `position:

` literal. Only the unfiltered list showed it. The -// `unstaffed` member is the arm that returns it to the callers who may decide -// it. It is ORed with `approverId`, and both `listRequests` and -// `countRequests` answer it. -// -// Every case reads BOTH faces (the list and its count), because the console's -// paged inbox shows the one and the bell shows the other. -describe('ApprovalService — the unstaffed arm (#22725)', () => { - let engine: ReturnType; - let svc: ApprovalService; - let n = 0; - const baseTime = new Date('2026-01-15T10:00:00Z').getTime(); - - const PLATFORM_ADMIN = { userId: 'root', tenantId: 't1', positions: [], permissions: ['admin_full_access'] } as any; - const TENANT_ADMIN = { userId: 'owner', tenantId: 't1', positions: [], permissions: ['organization_admin'] } as any; - const OTHER_TENANT_ADMIN = { userId: 'owner2', tenantId: 't2', positions: [], permissions: ['organization_admin'] } as any; - const MEMBER = { userId: 'nobody', tenantId: 't1', positions: [], permissions: [] } as any; - const APPROVER = { userId: 'u9', tenantId: 't1', positions: [], permissions: [] } as any; - - /** A node routed to a position nobody holds: the `position:sales_manager` literal. */ - const stuck = (recordId: string) => ({ - object: 'opportunity', recordId, runId: `run_${recordId}`, nodeId: 'approve_step', - flowName: 'deal_approval', - config: { approvers: [{ type: 'position' as const, value: 'sales_manager' }], behavior: 'first_response' as const, lockRecord: true }, - record: { id: recordId, amount: 100 }, - }); - /** An expression that resolves to nobody: `admin_rescue` opens on an EMPTY slate. */ - const empty = (recordId: string) => ({ - ...stuck(recordId), - config: { approvers: [{ type: 'expression' as const, value: 'vars.picked' }], behavior: 'first_response' as const, lockRecord: true }, - variables: { picked: [] }, - }); - /** A staffed node: a concrete user on the slate. */ - const staffed = (recordId: string, user = 'u9') => ({ - ...openInput([user]), recordId, runId: `run_${recordId}`, record: { id: recordId, amount: 100 }, - }); - - const ids = (rows: Array<{ id: string }>) => rows.map(r => r.id).sort(); - /** Both faces of one filter, as one reader: the list's ids and the count. */ - const read = async (filter: Record, ctx: any) => { - const rows = await svc.listRequests(filter as any, ctx); - const total = await svc.countRequests(filter as any, ctx); - return { ids: ids(rows), total, rows }; - }; - - beforeEach(() => { - engine = makeFakeEngine(); - n = 0; - svc = new ApprovalService({ engine: engine as any, clock: { now: () => new Date(baseTime + (n++) * 1000) } }); - }); - - it('an admin_rescue request appears for an override actor, on both faces', async () => { - const onLiteral = await svc.openNodeRequest(stuck('opp_literal'), CTX) as any; - const onNothing = await svc.openNodeRequest(empty('opp_empty'), CTX) as any; - expect(onLiteral.pending_approvers).toEqual(['position:sales_manager']); - expect(onNothing.pending_approvers).toEqual([]); - const control = await svc.openNodeRequest(staffed('opp_staffed'), CTX) as any; - - for (const admin of [TENANT_ADMIN, PLATFORM_ADMIN]) { - const got = await read({ status: 'pending', unstaffed: true }, admin); - expect(got.ids).toEqual([onLiteral.id, onNothing.id].sort()); - expect(got.total).toBe(2); - expect(got.ids).not.toContain(control.id); - // Every row the arm adds is one the caller may override. - for (const row of got.rows) { - expect(row.viewer).toMatchObject({ can_act: false, can_override: true }); - } - } - }); - - it('it does not appear for a member: the flag adds nothing and refuses nothing', async () => { - await svc.openNodeRequest(stuck('opp_literal'), CTX); - await svc.openNodeRequest(empty('opp_empty'), CTX); - const mine = await svc.openNodeRequest(staffed('opp_mine', 'nobody'), CTX) as any; - - const slateScans = () => engine._finds.filter(f => f.object === 'sys_approval_request' - && Array.isArray(f.options?.fields) && f.options.fields.includes('pending_approvers')).length; - const before = slateScans(); - expect(await read({ status: 'pending', unstaffed: true }, MEMBER)).toMatchObject({ ids: [], total: 0 }); - // A caller who may override nothing is answered without the arm's scan. - expect(slateScans()).toBe(before); - // Beside the member's own identities, the arm leaves their queue exactly as - // `approverId` alone answers it. - const withFlag = await read({ status: 'pending', approverId: ['nobody'], unstaffed: true }, MEMBER); - const without = await read({ status: 'pending', approverId: ['nobody'] }, MEMBER); - expect(withFlag.ids).toEqual([mine.id]); - expect(withFlag.total).toBe(1); - expect(without.ids).toEqual(withFlag.ids); - expect(without.total).toBe(withFlag.total); - }); - - it('CONTROL: a staffed request\'s queue is unchanged', async () => { - await svc.openNodeRequest(stuck('opp_literal'), CTX); - const control = await svc.openNodeRequest(staffed('opp_staffed'), CTX) as any; - - // Its approver's queue reads the same with and without the arm. - for (const filter of [ - { status: 'pending', approverId: ['u9'] }, - { status: 'pending', approverId: ['u9'], unstaffed: true }, - ]) { - expect(await read(filter, APPROVER)).toMatchObject({ ids: [control.id], total: 1 }); - } - // And an override actor's rescue arm never claims it. - const rescue = await read({ status: 'pending', unstaffed: true }, TENANT_ADMIN); - expect(rescue.ids).not.toContain(control.id); - // Without the arm, the override actor's "My Pending" is what it was. - expect(await read({ status: 'pending', approverId: ['owner'] }, TENANT_ADMIN)).toMatchObject({ ids: [], total: 0 }); - }); - - it('with approverId the arms are ORed: one read answers "my pending, rescues included"', async () => { - const stuckReq = await svc.openNodeRequest(stuck('opp_literal'), CTX) as any; - const adminOwn = await svc.openNodeRequest(staffed('opp_admin_own', 'owner'), CTX) as any; - await svc.openNodeRequest(staffed('opp_other'), CTX); - - const both = await read({ status: 'pending', approverId: ['owner'], unstaffed: true }, TENANT_ADMIN); - expect(both.ids).toEqual([stuckReq.id, adminOwn.id].sort()); - expect(both.total).toBe(2); - // The paged window covers the union too: two pages of one. - const page1 = await svc.listRequests({ status: 'pending', approverId: ['owner'], unstaffed: true, limit: 1, offset: 0 }, TENANT_ADMIN); - const page2 = await svc.listRequests({ status: 'pending', approverId: ['owner'], unstaffed: true, limit: 1, offset: 1 }, TENANT_ADMIN); - expect(ids([...page1, ...page2])).toEqual(both.ids); - }); - - it('`unstaffed: false` is the same as absent', async () => { - await svc.openNodeRequest(stuck('opp_literal'), CTX); - await svc.openNodeRequest(staffed('opp_staffed'), CTX); - const absent = await read({ status: 'pending' }, TENANT_ADMIN); - const off = await read({ status: 'pending', unstaffed: false }, TENANT_ADMIN); - expect(off.ids).toEqual(absent.ids); - expect(off.total).toBe(absent.total); - expect(absent.total).toBe(2); - }); - - it('a tenant admin\'s rescue queue stops at their organization; a platform admin\'s crosses it', async () => { - const t1Stuck = await svc.openNodeRequest(stuck('opp_literal'), CTX) as any; // organization t1 - - expect(await read({ status: 'pending', unstaffed: true }, OTHER_TENANT_ADMIN)).toMatchObject({ ids: [], total: 0 }); - const crossing = { ...PLATFORM_ADMIN, tenantId: 't2' }; - expect(await read({ status: 'pending', unstaffed: true }, crossing)).toMatchObject({ ids: [], total: 0 }); - // The platform admin's own read is walled by its context organization the - // way every list read is (`buildRequestWhere`); with no organization on - // the context, the arm reaches every organization's rescue queue. - const unscoped = { userId: 'root', positions: [], permissions: ['admin_full_access'] } as any; - expect(await read({ status: 'pending', unstaffed: true }, unscoped)).toMatchObject({ ids: [t1Stuck.id], total: 1 }); - }); - - it('per row, the arm returns only what the caller may override: an org-less tenant admin reaches org-less rescues only', async () => { - const t1Stuck = await svc.openNodeRequest(stuck('opp_literal'), CTX) as any; // organization t1 - const globalStuck = await svc.openNodeRequest(stuck('opp_global'), SYS) as any; // no organization - expect(engine._tables['sys_approval_request'].find(r => r.id === globalStuck.id)?.organization_id ?? null).toBeNull(); - - // No organization on the context: the list read is unwalled, and the - // visibility read lets this caller see every row. Only the per-row - // override test keeps the t1 request, which this caller may NOT - // override, out of the arm. - const orgLessTenantAdmin = { userId: 'owner', positions: [], permissions: ['organization_admin'] } as any; - const got = await read({ status: 'pending', unstaffed: true }, orgLessTenantAdmin); - expect(got).toMatchObject({ ids: [globalStuck.id], total: 1 }); - expect(got.ids).not.toContain(t1Stuck.id); - expect(got.rows[0].viewer).toMatchObject({ can_override: true }); - }); - - it('reads the slate as it stands: a slate left holding only literals joins the arm', async () => { - const mixed = await svc.openNodeRequest({ - ...stuck('opp_mixed'), - config: { - approvers: [{ type: 'user' as const, value: 'u9' }, { type: 'position' as const, value: 'sales_manager' }], - behavior: 'unanimous' as const, - lockRecord: true, - }, - }, CTX) as any; - expect(mixed.pending_approvers).toEqual(['u9', 'position:sales_manager']); - expect((await read({ status: 'pending', unstaffed: true }, TENANT_ADMIN)).ids).toEqual([]); - - // u9 approves; the unanimous tally keeps the request open on the literal alone. - await svc.decideNode(mixed.id, { decision: 'approve', actorId: 'u9' }, APPROVER); - const after = await svc.getRequest(mixed.id, TENANT_ADMIN); - expect(after?.status).toBe('pending'); - expect(after?.pending_approvers).toEqual(['position:sales_manager']); - expect(await read({ status: 'pending', unstaffed: true }, TENANT_ADMIN)).toMatchObject({ ids: [mixed.id], total: 1 }); - }); - - it('a finalized rescue leaves the arm', async () => { - const req = await svc.openNodeRequest(stuck('opp_literal'), CTX) as any; - await svc.decide(req.id, { decision: 'approve', actorId: 'owner' }, TENANT_ADMIN); - expect(await read({ unstaffed: true }, TENANT_ADMIN)).toMatchObject({ ids: [], total: 0 }); - }); -}); - // ── approval_comment: the declared reply against the route it posts to ────── // // `sys_approval_request` declares the thread reply as the action diff --git a/packages/plugins/plugin-approvals/src/approval-service.ts b/packages/plugins/plugin-approvals/src/approval-service.ts index 60a6e36e86d..3496f7c31c4 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.ts @@ -64,7 +64,7 @@ import type { EnableLike, ObjectCapabilitiesParsed } from '@objectstack/spec/dat // mechanism for a second producer, and commit aa5994e17 landed this service's key // (`approval_recall_not_submitter`) into it ahead of this consumer half. import { renderOperationMessage, type ValidationMessageTranslator } from '@objectstack/spec/system'; -import { isGrantActive, omitInternalFieldsFromWriteResponse } from '@objectstack/core'; +import { isGrantActive, omitInternalFieldsFromWriteResponse, resolveUserAuthzGrants } from '@objectstack/core'; import { filterApproversWhoCanRead, resolveApproverDirectoryOrg, @@ -970,10 +970,68 @@ const GRAPH_APPROVER_TYPES: ReadonlySet = new Set([ * * `user` needs no entry: its `value` IS the slot and it never falls back. * `position` keeps its literal because the literal is an acting address — a - * holder staffed into the position later decides it. + * holder staffed into the position later decides it, on a slate that also + * names a person. A slate of literals alone names no one, and under the + * default `admin_rescue` it opens on the organization's administrators + * instead ({@link ApprovalService.openNodeRequest}). */ const PERSON_APPROVER_TYPES: ReadonlySet = new Set(['manager', 'field']); +/** + * [ADR-0095 D3] Does a resolved permission list hold the `TENANT_ADMIN` rung? + * + * The ONE spelling of "is an organization administrator" in this file. The + * rung is derived from the {@link ORGANIZATION_ADMIN_GRANTS} capability grants + * and from nothing else (`derivePosture` in `@objectstack/core`), so the two + * readers here ask exactly this: + * - the tenant arm of {@link ApprovalService.isOverrideActor}, over the + * acting caller's resolved envelope; + * - the administrators' slate an empty `admin_rescue` request opens on + * ({@link ApprovalService.organizationAdministrators}), over each + * candidate's grants as `resolveUserAuthzGrants` resolves them in the + * request's organization. + * One predicate, so the people a rescue request is routed to and the people + * the override admits as tenant administrators cannot drift apart. + */ +function holdsTenantAdminRung(permissions: readonly string[]): boolean { + return ORGANIZATION_ADMIN_GRANTS.some((n) => permissions.includes(n)); +} + +/** + * The one synthetic group every administrator on a rescue slate shares, so a + * `per_group` node reads "the organization's administrators" as ONE group + * (any `minApprovals` of them, default one) rather than one group per person, + * and the display drops the key as it drops every unnamed `#N` group. + */ +const ADMIN_RESCUE_GROUP = '#0'; + +/** + * [#22824] Why a slate opened on the organization's administrators, in words + * an operator reads on the request's timeline and in its `__resolvedFrom` + * snapshot: which approver entries resolved to no one. + * + * `abandoned` is the slate the node resolved (only `type:value` literals, or + * nothing at all: an empty `manager` rung adds no slot). `fallback` says why + * an `onEmptyApprovers: 'fallback'` node degraded here, when it is one. + */ +function emptySlateReason( + abandoned: readonly string[], + fallback: 'not_declared' | 'resolved_to_no_one' | undefined, +): string { + const parts = abandoned + .filter((slot) => !!slot && slot.includes(':')) + .map((slot) => { + const at = slot.indexOf(':'); + const type = slot.slice(0, at); + const value = slot.slice(at + 1); + return type === 'position' ? `position '${value}' has no holder` : `${type} '${value}' resolved to no one`; + }); + let reason = parts.length ? parts.join('; ') : 'no approver on the node resolved to a person'; + if (fallback === 'resolved_to_no_one') reason += '; the declared fallbackApprovers resolved to no one either'; + if (fallback === 'not_declared') reason += "; onEmptyApprovers is 'fallback' but no fallbackApprovers are declared"; + return reason; +} + /** One OOO delegation hop applied while resolving an approver (#1322 M1/M4). */ interface OooSubstitution { /** The approver who was skipped (out of office). */ @@ -1672,8 +1730,10 @@ export class ApprovalService implements IApprovalService { // refused new writes WITHOUT a migration, so rows predating it still resolve // on every request — and a reader that trusts a name is not an invariant in // any case. Driven in `approval-tenant-positions-name-authority.test.ts`. + // [#22824] `holdsTenantAdminRung` is that same grant read, shared with the + // administrators' slate an empty `admin_rescue` request opens on. const isTenantAdmin = posture === 'TENANT_ADMIN' - || ORGANIZATION_ADMIN_GRANTS.some((n) => perms.includes(n)); + || holdsTenantAdminRung(perms); if (!isTenantAdmin) return false; // A tenant admin's authority stops at their own org; a null-org request is // global and any admin may release it. @@ -2706,6 +2766,71 @@ export class ApprovalService implements IApprovalService { return Array.from(users); } + /** + * [#22824] The organization's administrators: the slate an empty + * `onEmptyApprovers: 'admin_rescue'` request opens on. + * + * "Administrator" has ONE meaning here, the authority the tenant arm of + * {@link ApprovalService.isOverrideActor} already trusts: a person holding + * the `TENANT_ADMIN` rung of THIS organization (ADR-0095 D3), which is + * {@link holdsTenantAdminRung} over the grants `resolveUserAuthzGrants` + * resolves for them with the organization as the active tenant. That is the + * resolver every authorization envelope comes from, so a grant window, a + * deactivated set, an organization-scoped grant of another organization and + * a position-bound set all answer here exactly as they answer when the + * person acts. Nothing reads a better-auth membership role as authority: the + * owner/admin role is only the provisioning source of the grant. + * + * ⛔ Platform administrators are not added for being platform + * administrators: that standing crosses the tenant wall, and a rescue slate + * is the organization's own. One who also holds this organization's rung + * (an owner of it, say) is on the slate for that rung. + * + * The candidates are the organization's members, `sys_member.organization_id` + * being this file's answer to "who is in this organization" (see + * {@link ApprovalService.lookupManager}); each is then asked the rung + * question. No organization means no tenant administrators: the request + * keeps the slate that names no one, for the override. + * + * A read that fails answers `{ failure }`, never an invented "nobody": the + * caller then opens the request on the slate that names no one and says the + * read failed. That is the rescue path's own fallback — killing the run here + * is the one outcome `admin_rescue` exists to avoid. + */ + private async organizationAdministrators( + organizationId: string | null | undefined, + ): Promise<{ ids: string[] } | { failure: string }> { + if (!organizationId) return { ids: [] }; + const org = String(organizationId); + try { + const rows = await this.engine.find('sys_member', { + where: { organization_id: org }, fields: ['user_id'], + limit: ApprovalService.APPROVER_INDEX_CAP, context: SYSTEM_CTX, + }); + const list: any[] = Array.isArray(rows) ? rows : []; + if (list.length >= ApprovalService.APPROVER_INDEX_CAP) { + this.logger?.warn?.('[approvals] organization-administrator probe hit its window — results may be truncated', { + cap: ApprovalService.APPROVER_INDEX_CAP, organizationId: org, + }); + } + const members = [...new Set(list.map((r) => String(r?.user_id ?? r?.userId ?? '')).filter(Boolean))]; + const administrators: string[] = []; + // A few resolutions at a time: each is the full grant resolution an + // acting request pays, and an organization may have many members. + for (let at = 0; at < members.length; at += 8) { + const batch = members.slice(at, at + 8); + const verdicts = await Promise.all(batch.map(async (userId) => { + const grants = await resolveUserAuthzGrants(this.engine, userId, { tenantId: org }); + return holdsTenantAdminRung(grants.permissions); + })); + batch.forEach((userId, i) => { if (verdicts[i]) administrators.push(userId); }); + } + return { ids: administrators }; + } catch (err: any) { + return { failure: err?.message ?? String(err) }; + } + } + /** * better-auth org-membership tier (`sys_member.role`: owner/admin/member) — * NOT positions. Named for the projection (`org_membership_level`, ADR-0057 @@ -3016,6 +3141,12 @@ export class ApprovalService implements IApprovalService { * same expansion as `approvers`. The return type is unchanged for that * policy: from the flow node's side it is an ordinary pending request. * + * An empty slate under the default `onEmptyApprovers: 'admin_rescue'` (and a + * `fallback` that found nobody either) opens on a NAMED slate too: the + * request organization's administrators ({@link ApprovalService.organizationAdministrators}), + * through the same expansion. Only an organization with no administrator + * leaves the request on the slate that names no one, for the #3424 override. + * * A request that opens tells each concrete approver on its slate, once, on * the `approval.requested` topic (see the fan-out at the end of the body). */ @@ -3112,9 +3243,16 @@ export class ApprovalService implements IApprovalService { // Empty-slate policy (#3447 P2). "Empty" = no CONCRETE person — an // unstaffed position / empty expression result leaves only `type:value` // literal slots, decidable by nobody. `slateNamesAPerson` is the one - // spelling of that test: the inbox's `unstaffed` arm reads it too (#22725). + // spelling of that test. + // + // [#22824] Set when the slate below is replaced by the organization's + // administrators: the reason, written to the request's timeline once the + // request row exists. + let routedToAdministrators: string | undefined; if (!slateNamesAPerson(approvers)) { const emptyPolicy = (input.config as any).onEmptyApprovers ?? 'admin_rescue'; + // Why a `fallback` node degraded to admin_rescue, for the reason below. + let fallbackDegraded: 'not_declared' | 'resolved_to_no_one' | undefined; if (emptyPolicy === 'fail') { throw new Error( `NO_APPROVERS: approval node '${input.nodeId}' on ${input.object}/${input.recordId} resolved to no ` @@ -3177,6 +3315,7 @@ export class ApprovalService implements IApprovalService { for (const slot of approvers) delete groups[slot]; approvers = fallbackApprovers; } else { + fallbackDegraded = 'resolved_to_no_one'; this.logger?.warn?.( `[approvals] approval node '${input.nodeId}' on ${input.object}/${input.recordId} resolved to no ` + `concrete approver AND its declared fallbackApprovers resolved to nobody either — falling back ` @@ -3188,6 +3327,7 @@ export class ApprovalService implements IApprovalService { ); } } else { + fallbackDegraded = 'not_declared'; this.logger?.warn?.( `[approvals] approval node '${input.nodeId}' on ${input.object}/${input.recordId} declares ` + `onEmptyApprovers: 'fallback' with no fallbackApprovers — falling back to admin_rescue.`, @@ -3196,21 +3336,72 @@ export class ApprovalService implements IApprovalService { } } - // #3424 admin_rescue (default): the request is still opened (a privileged - // admin can override it, and legacy 15.x literal slots stay queryable) — - // the only option that neither waves the record through nor kills the - // run — but warn loudly so the misconfiguration surfaces instead of - // silently locking the record with no obvious cause. + // [#22824] admin_rescue (default), and a `fallback` that found nobody + // above: the request opens on a NAMED slate, the request organization's + // administrators. They are ordinary approvers in the ordinary inbox: the + // `approverId` index lists and counts the request for each of them, the + // fan-out below tells each of them, and a decision by one of them is a + // slot decision, recorded as an approval and never as an override. Before + // this the request opened on the slate that names no one, which no inbox + // listed and no notification reached, so the #3424 override was the + // only way out and every downstream feature needed a rescue arm. + // + // The administrators go through the SAME resolver as `approvers` (as + // `user` entries, one shared group), so OOO delegation and the fan-out + // apply to them unchanged. The abandoned slate and the reason are kept + // in `__resolvedFrom` beside the primary resolution inputs, namespaced + // as the `fallback` branch namespaces its own. // // Re-tested rather than assumed: a `fallback` that landed people above - // has already turned this into a normal request, and warning "decidable - // only by a privileged admin" about it would be false. + // has already turned this into a normal request. if (!slateNamesAPerson(approvers)) { - this.logger?.warn?.( - `[approvals] approval node '${input.nodeId}' on ${input.object}/${input.recordId} resolved to no concrete approver` - + ' — the request is decidable only by a privileged admin. Check that the approver target(s) are staffed.', - { object: input.object, recordId: input.recordId, node: input.nodeId, resolved: approvers }, - ); + const reason = emptySlateReason(approvers, fallbackDegraded); + const found = await this.organizationAdministrators(requestOrg); + const administrators = 'ids' in found ? found.ids : []; + if (administrators.length) { + // Every per_group tag collected so far describes a slot that is no + // longer on this request (the primary slate's literals, and a failed + // fallback's): the tally reads the administrators' one group only. + for (const slot of Object.keys(groups)) delete groups[slot]; + const administratorSlate = await this.expandApprovers( + { + approvers: administrators.map((value) => ({ type: 'user', value, group: ADMIN_RESCUE_GROUP })), + }, + liveRecord, requestOrg, { now: nowDate.getTime(), substitutions, groups, exprCtx }, + ); + resolvedFrom['admin_rescue:abandoned'] = [...approvers]; + resolvedFrom['admin_rescue:reason'] = reason; + this.logger?.warn?.( + `[approvals] approval node '${input.nodeId}' on ${input.object}/${input.recordId} resolved to no concrete ` + + `approver (${reason}) — opening on this organization's administrators per onEmptyApprovers: ` + + `'admin_rescue'. Staff the approver target(s), or declare onEmptyApprovers: 'fallback' with ` + + `fallbackApprovers to name who decides instead.`, + { + object: input.object, recordId: input.recordId, node: input.nodeId, + resolved: approvers, administrators: administratorSlate, + }, + ); + approvers = administratorSlate; + routedToAdministrators = reason; + } else { + // Break-glass stays break-glass (#3424): an organization whose + // administrators' slate resolves to nobody opens the request on the + // slate that names no one — the only option that neither waves the + // record through nor kills the run — and only the privileged + // override can decide it. Said loudly, with the fix. + const why = 'failure' in found + ? `the administrators of organization '${requestOrg}' could not be read (${found.failure})` + : requestOrg + ? `organization '${requestOrg}' has no administrator` + : 'the request has no organization, so it has no organization administrators'; + this.logger?.warn?.( + `[approvals] approval node '${input.nodeId}' on ${input.object}/${input.recordId} resolved to no concrete ` + + `approver (${reason}), and ${why}, so the request opens on a slate that names no one: it is in no ` + + `one's queue and decidable only by a privileged admin's override. Staff the approver target(s), give ` + + `the organization an administrator, or declare onEmptyApprovers: 'fallback' with fallbackApprovers.`, + { object: input.object, recordId: input.recordId, node: input.nodeId, resolved: approvers, organizationId: requestOrg }, + ); + } } } @@ -3275,6 +3466,21 @@ export class ApprovalService implements IApprovalService { actor_id: input.submitterId ?? context.userId ?? null, comment: null, created_at: now, }, { context: SYSTEM_CTX }); + // [#22824] The request's display says why it is with the administrators: + // one system note on its timeline (no person took it, so `actor_id` is + // null, ADR-0118 D1 — the shape the OOO hop below records), naming which + // approver entries resolved to no one. The same reason rides + // `__resolvedFrom` for the audit snapshot. + if (routedToAdministrators) { + await this.engine.insert('sys_approval_action', { + id: uid('aact'), request_id: id, organization_id: requestOrg, + step_name: input.nodeId, step_index: 0, action: 'comment', + actor_id: null, + comment: `Routed to this organization's administrators because the approver slate named no one: ${routedToAdministrators}.`, + created_at: now, + }, { context: SYSTEM_CTX }); + } + // OOO substitution audit + notification (#1322 M4). Each hop that rerouted // an approver away from an out-of-office user is recorded on the request's // audit trail (a system action, no human actor) and notified to both the @@ -3339,14 +3545,17 @@ export class ApprovalService implements IApprovalService { // // Who is told is the reminder's recipient rule, read off the slate this // request actually opened on (after OOO delegation and after an - // `onEmptyApprovers: 'fallback'` replacement): + // `onEmptyApprovers: 'fallback'` or `'admin_rescue'` replacement — so the + // organization's administrators a rescue opened on are told like any + // other approver, #22824): // - each CONCRETE approver (a user id or an email), once — a person the // slate lists under two groups (per_group tagging) is one person; // - never a `type:value` literal: an unstaffed position / team names no // one, and `notify()` drops every slot address anyway. So a slate of - // literals only — what `admin_rescue` opens — notifies nobody, and an - // `auto_approve` / `fail` empty slate opened no request to tell anyone - // about: `onEmptyApprovers` decides that case, not this fan-out; + // literals only — what `admin_rescue` opens only in an organization + // with no administrator — notifies nobody, and an `auto_approve` / + // `fail` empty slate opened no request to tell anyone about: + // `onEmptyApprovers` decides that case, not this fan-out; // - not an out-of-office DELEGATE, who was already told above by // `approval.ooo_substituted` — one message per approver, not two. // No one-tap decision links: ADR-0043 mints those on `remind()` only, and @@ -6848,97 +7057,6 @@ export class ApprovalService implements IApprovalService { return [...new Set(list.map(r => String(r.request_id)))]; } - /** - * The "who acts next" constraint of a list or count read: the request ids - * its two arms match together, or `null` when the filter asked neither - * (callers then skip the id constraint, exactly as before #22725). - * - * - `approverId`: requests where one of those addresses holds a pending - * slot ({@link ApprovalService.approverRequestIds}); - * - `unstaffed: true`: pending requests nobody on the slate can decide, - * which this caller may override - * ({@link ApprovalService.unstaffedOverridableRequestIds}). - * - * The arms are ORed, never ANDed. The contract states this on the - * `unstaffed` member: one request answers "my pending approvals, rescues - * included", and the paged total counts both. `listRequests` and - * `countRequests` both call this, so the list and the total it pages - * cannot disagree about either arm. - */ - private async awaitingRequestIds( - filter: { approverId?: string | string[]; unstaffed?: boolean } | undefined, - context: ExecutionContext, - tenantOrg: string | null, - ): Promise { - const approverTargets = (Array.isArray(filter?.approverId) ? filter!.approverId : filter?.approverId ? [filter.approverId] : []) - .map(t => String(t).trim()) - .filter(Boolean); - const byApprover = await this.approverRequestIds(approverTargets, tenantOrg); - if (filter?.unstaffed !== true) return byApprover; - const unstaffed = await this.unstaffedOverridableRequestIds(context, tenantOrg); - return [...new Set([...(byApprover ?? []), ...unstaffed])]; - } - - /** - * [#22725] The `unstaffed` arm. These are the PENDING requests whose slate - * names no person and which this caller may override. This is the queue an - * `onEmptyApprovers: 'admin_rescue'` request waits in. Before this arm it - * waited in none: `approverRequestIds` matches a slot against the addresses - * a caller names, and an override actor's own addresses never equal an - * unstaffed `position:

` literal. So no "My Pending" listed the request - * and no bell counted it. A `lockRecord: true` record stayed locked until - * someone browsed the unfiltered list. - * - * "Names no person" is {@link slateNamesAPerson}, the test the open path - * applies before it warns "decidable only by a privileged admin". It is read - * off the stored slate as it stands now, never off a flag written at open - * time. So a slate that loses its last named approver to a decision and - * keeps only literals is caught as well. A slate that still names a person - * is not, even if that person has left: proving they left needs a directory - * read per slot, which neither this arm nor the open path makes. - * - * "May override" is {@link ApprovalService.isOverrideActor} and nothing else. - * There is no second notion of "admin" here. It is asked twice: - * - first as the visibility read asks it (`visibleRequestIds`' first line, - * the caller's organization). A caller who may override nothing pays no - * read, and gets nothing; - * - then per row, as `attachViewers` asks it for `viewer.can_override` - * (the row's organization). So every row this arm adds reads - * `can_override: true`, and a tenant admin's rescue queue never reaches - * past their organization. - * - * Read with `SYSTEM_CTX`, like every probe in this file. The scan is the - * live pending queue inside the caller's organization wall, bounded by the - * same window as the approver index. A failed read throws; it is not read as - * an empty queue. - */ - private async unstaffedOverridableRequestIds( - context: ExecutionContext, - tenantOrg: string | null, - ): Promise { - if (!this.isOverrideActor(context, tenantOrg)) return []; - const where: any = { status: 'pending' }; - if (tenantOrg) where.organization_id = tenantOrg; - const rows = await this.engine.find('sys_approval_request', { - where, fields: ['id', 'pending_approvers', 'organization_id'], - limit: ApprovalService.APPROVER_INDEX_CAP, context: SYSTEM_CTX, - }); - const list: any[] = Array.isArray(rows) ? rows : []; - if (list.length >= ApprovalService.APPROVER_INDEX_CAP) { - this.logger?.warn?.('[approvals] unstaffed-request probe hit its window — results may be truncated', { - cap: ApprovalService.APPROVER_INDEX_CAP, - }); - } - const ids: string[] = []; - for (const row of list) { - if (row?.id == null) continue; - if (slateNamesAPerson(csvSplit(row.pending_approvers))) continue; - if (!this.isOverrideActor(context, row.organization_id ?? null)) continue; - ids.push(String(row.id)); - } - return ids; - } - /** * The request ids this caller is a PARTICIPANT of, or `null` for a caller * who may see everything in scope (#3590). @@ -7230,7 +7348,6 @@ export class ApprovalService implements IApprovalService { recordId?: string; status?: ApprovalStatus | ApprovalStatus[]; approverId?: string | string[]; - unstaffed?: boolean; submitterId?: string; q?: string; limit?: number; @@ -7239,13 +7356,14 @@ export class ApprovalService implements IApprovalService { context: ExecutionContext, ): Promise { const { where, tenantOrg } = await this.buildRequestWhere(filter, context); + const approverTargets = (Array.isArray(filter?.approverId) ? filter!.approverId : filter?.approverId ? [filter.approverId] : []) + .map(t => String(t).trim()) + .filter(Boolean); // Every filter now pushes into the engine (issue #1745): approver via // the normalized index, status arrays via $in — so the page window is - // always engine-side and correct at any table size. [#22725] The - // `unstaffed` arm joins the approver arm as an id set too, so a rescue - // request pages exactly like any other row. - const ids = await this.awaitingRequestIds(filter, context, tenantOrg); + // always engine-side and correct at any table size. + const ids = await this.approverRequestIds(approverTargets, tenantOrg); if (ids) { if (ids.length === 0) return []; where.id = ids.length === 1 ? ids[0] : { $in: ids }; @@ -7288,9 +7406,11 @@ export class ApprovalService implements IApprovalService { context: ExecutionContext, ): Promise { const { where, tenantOrg } = await this.buildRequestWhere(filter, context); + const approverTargets = (Array.isArray(filter?.approverId) ? filter!.approverId : filter?.approverId ? [filter.approverId] : []) + .map(t => String(t).trim()) + .filter(Boolean); - // [#22725] The same two arms `listRequests` reads, from the same helper. - const ids = await this.awaitingRequestIds(filter, context, tenantOrg); + const ids = await this.approverRequestIds(approverTargets, tenantOrg); if (ids) { if (ids.length === 0) return 0; where.id = ids.length === 1 ? ids[0] : { $in: ids }; diff --git a/packages/plugins/plugin-approvals/src/approver-address.ts b/packages/plugins/plugin-approvals/src/approver-address.ts index 68c1030803e..260b5e61af2 100644 --- a/packages/plugins/plugin-approvals/src/approver-address.ts +++ b/packages/plugins/plugin-approvals/src/approver-address.ts @@ -140,11 +140,10 @@ export function actorAddresses(actorId: string, caller: ActingCaller | null): st * slate is a `type:value` literal, left by an approver target that resolved to * no holder (`position:

`, `team:`, a stored `role:

`). * - * `false` is the slate the open path calls "decidable only by a privileged - * admin" (the `onEmptyApprovers` policies decide it there). The - * `unstaffed` arm of the inbox list reads the same test on the slate as it - * stands (#22725). One test, so the slate the open path warns about and the - * slate that arm returns cannot drift apart. + * `false` is the slate the open path hands to the node's `onEmptyApprovers` + * policy, and the one it re-tests after a `fallback` before the default + * `admin_rescue` opens on the organization's administrators (#22824). One + * test, so every one of those decisions reads the same slate shape. * * This is a test of the slate's SHAPE. It compares no slot with a caller, so it * is not a reader of the equivalence above. diff --git a/packages/qa/dogfood/test/activity-text-predicate.dogfood.test.ts b/packages/qa/dogfood/test/activity-text-predicate.dogfood.test.ts index 55d42d125ca..7275b1fee5d 100644 --- a/packages/qa/dogfood/test/activity-text-predicate.dogfood.test.ts +++ b/packages/qa/dogfood/test/activity-text-predicate.dogfood.test.ts @@ -274,8 +274,15 @@ describe('[#21154] a query over activity text or the approval snapshot, by a rea } if (who === 'control') { // Staff the position BEFORE the subject exists: the slate resolves at request creation. + // The assignment carries the holder's organization, as a write through a + // door does: an unstamped row is UNKNOWN tenancy, which the slate's + // position expansion does not read, and an all-literal slate would then + // open on the organization's administrators instead (#22824). + const membership = await ql.findOne('sys_member', { where: { user_id: userId }, context: { isSystem: true } }); await ql.insert('sys_position', { id: 'pos_atp', name: POSITION, label: 'Reviewer', active: true }, SYS); - await ql.insert('sys_user_position', { id: 'hold_atp', user_id: userId, position: POSITION }, SYS); + await ql.insert('sys_user_position', { + id: 'hold_atp', user_id: userId, position: POSITION, organization_id: membership?.organization_id, + }, SYS); } } diff --git a/packages/qa/dogfood/test/approval-admin-rescue-slate.dogfood.test.ts b/packages/qa/dogfood/test/approval-admin-rescue-slate.dogfood.test.ts new file mode 100644 index 00000000000..d8c5a8191a2 --- /dev/null +++ b/packages/qa/dogfood/test/approval-admin-rescue-slate.dogfood.test.ts @@ -0,0 +1,166 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// #22824 — an `admin_rescue` request was in no one's queue. +// +// A node whose approvers resolve to no holder used to open, under the default +// `onEmptyApprovers: 'admin_rescue'`, on a slate that named no person (the +// literal `position:

`). No "My Pending" listed it, the bell counted 0, no +// one was told, and a `lockRecord: true` record stayed locked until an admin +// found it under All and decided it through the privileged override +// (measured on HotCRM's fresh installs). +// +// It now opens on the organization's administrators, as ordinary approvers. +// This pins that on a real boot, through the doors the console calls, with the +// real grant provisioning: the seeded admin owns the default organization, and +// a second member is promoted to `admin` by a `sys_member` write, which is +// what provisions their organization-admin capability. +// +// ⭐ the request opens on both administrators, and each one's "My Pending" +// lists it, its `total` (the bell's number) counts it, and their inbox +// holds one `approval.requested` notification; +// ⭐ a member sees nothing and is told nothing; +// ⭐ an administrator's decision through the door is an ordinary approval, +// recorded with `via_override: false`, and the timeline says why the +// request was with the administrators; +// ⭐ CONTROL: a staffed request's slate is unchanged; +// ⭐ the retired `?unstaffed=` arm is refused, never read as an answer. + +import { describe, it, expect } from 'vitest'; +import { bootStack } from '@objectstack/verify'; +import { ApprovalsServicePlugin } from '@objectstack/plugin-approvals'; +import { RecordChangeTriggerPlugin } from '@objectstack/trigger-record-change'; +import { MessagingServicePlugin } from '@objectstack/service-messaging'; +import { + adminRescueStack, + adminRescueSecurity, + STAFFED_APPROVER_EMAIL, +} from './fixtures/admin-rescue-slate-fixture.js'; + +const SYS = { context: { isSystem: true } } as const; +/** `bootStack`'s seeded dev admin: the default organization's owner. */ +const ADMIN_EMAIL = 'admin@objectos.ai'; +const CO_ADMIN = 'admin-rescue-co-admin@example.com'; +const SUBMITTER = 'admin-rescue-submitter@example.com'; +const MEMBER = 'admin-rescue-member@example.com'; + +interface Row { + id: string; + object_name?: string; + pending_approvers?: string[]; + viewer?: { can_act: boolean; is_submitter: boolean; can_override: boolean }; +} +interface ListBody { + data: Row[]; + total?: number; +} + +describe('an empty admin_rescue slate opens on the organization\'s administrators, through the REST door (#22824)', () => { + it( + 'lists, counts and notifies each administrator, not a member, records an ordinary approval, and leaves a staffed slate unchanged', + async () => { + const stack = await bootStack(adminRescueStack as unknown as Parameters[0], { + automation: true, + security: adminRescueSecurity(), + extraPlugins: [ + new MessagingServicePlugin({ reliableDelivery: false }), + new RecordChangeTriggerPlugin(), + new ApprovalsServicePlugin(), + ], + }); + try { + const ql: any = await stack.kernel.getServiceAsync('objectql'); + const idOf = async (email: string): Promise => { + const u = await ql.findOne('sys_user', { where: { email }, context: SYS.context }); + return String(u?.id ?? ''); + }; + // Seed the dev admin FIRST, so the sign-ups below are plain members. + const adminToken = await stack.signIn(); + const coAdminToken = await stack.signUp(CO_ADMIN); + const submitterToken = await stack.signUp(SUBMITTER); + const memberToken = await stack.signUp(MEMBER); + const approverToken = await stack.signUp(STAFFED_APPROVER_EMAIL); + const adminId = await idOf(ADMIN_EMAIL); + const coAdminId = await idOf(CO_ADMIN); + const memberId = await idOf(MEMBER); + const approverId = await idOf(STAFFED_APPROVER_EMAIL); + for (const id of [adminId, coAdminId, memberId, approverId]) expect(id).not.toBe(''); + + // Promote the co-admin the way an organization does: its membership + // role. The organization-admin capability is provisioned from it, and + // that capability — not the role — is what puts them on the slate. + const [membership] = await ql.find('sys_member', { where: { user_id: coAdminId }, context: SYS.context }); + expect(membership?.organization_id, 'precondition: the co-admin is a member of an organization').toBeTruthy(); + await ql.update('sys_member', { id: membership.id, role: 'admin' }, SYS); + const coAdminGrants = await ql.find('sys_user_permission_set', { where: { user_id: coAdminId }, context: SYS.context }); + expect( + coAdminGrants.map((g: any) => g.permission_set), + 'precondition: the admin role provisioned the organization-admin capability', + ).toEqual(expect.arrayContaining([expect.stringMatching(/^organization_admin/)])); + + const stuckRes = await stack.apiAs(submitterToken, 'POST', '/data/admin_rescue_stuck', { name: 'stuck' }); + expect(stuckRes.status).toBe(201); + const staffedRes = await stack.apiAs(submitterToken, 'POST', '/data/admin_rescue_staffed', { name: 'staffed' }); + expect(staffedRes.status).toBe(201); + + const list = async (token: string, query: string): Promise => { + const res = await stack.apiAs(token, 'GET', `/approvals/requests?${query}`); + expect(res.status, `GET /approvals/requests?${query}`).toBe(200); + return (await res.json()) as ListBody; + }; + const myPending = (token: string, id: string, email: string) => + list(token, `status=pending&approverId=${encodeURIComponent(id)},${encodeURIComponent(email)}&limit=50`); + const requested = async (token: string): Promise => { + const res = await stack.apiAs(token, 'GET', '/notifications?type=approval.requested'); + expect(res.status).toBe(200); + return ((await res.json()) as { data: { notifications: unknown[] } }).data.notifications; + }; + + // The scene, before anything is read off it: both requests are open. + const all = await list(adminToken, 'status=pending'); + expect(all.data).toHaveLength(2); + const stuck = all.data.find((r) => r.object_name === 'admin_rescue_stuck'); + const staffed = all.data.find((r) => r.object_name === 'admin_rescue_staffed'); + + // ⭐ 1 — opened on both administrators: listed, counted, told. + expect([...(stuck?.pending_approvers ?? [])].sort()).toEqual([adminId, coAdminId].sort()); + for (const [token, id, email] of [[adminToken, adminId, ADMIN_EMAIL], [coAdminToken, coAdminId, CO_ADMIN]]) { + const mine = await myPending(token, id, email); + expect(mine.data.map((r) => r.id), email).toEqual([stuck!.id]); + expect(mine.total, email).toBe(1); + expect(mine.data[0].viewer?.can_act, email).toBe(true); + expect(await requested(token), email).toHaveLength(1); + } + + // ⭐ 2 — a member sees nothing and is told nothing. + const asMember = await myPending(memberToken, memberId, MEMBER); + expect([asMember.data, asMember.total]).toEqual([[], 0]); + expect(await requested(memberToken)).toEqual([]); + + // ⭐ 3 — CONTROL: the staffed slate is unchanged, and its approver + // finds it exactly as before. + expect(staffed?.pending_approvers).toEqual([STAFFED_APPROVER_EMAIL]); + const asApprover = await myPending(approverToken, approverId, STAFFED_APPROVER_EMAIL); + expect(asApprover.data.map((r) => r.id)).toEqual([staffed!.id]); + + // ⭐ 4 — an administrator decides it as an ordinary approver. + const decided = await stack.apiAs(coAdminToken, 'POST', `/approvals/requests/${stuck!.id}/approve`, {}); + expect(decided.status).toBe(200); + expect(((await decided.json()) as { request?: { status?: string } }).request?.status).toBe('approved'); + const actions = await ql.find('sys_approval_action', { where: { request_id: stuck!.id }, context: SYS.context }); + const approval = actions.find((a: any) => a.action === 'approve'); + expect(approval).toMatchObject({ actor_id: coAdminId, acted_as: coAdminId }); + expect(Boolean(approval?.via_override)).toBe(false); + const note = actions.find((a: any) => a.action === 'comment'); + expect(String(note?.comment)).toContain('has no holder'); + + // ⭐ 5 — the retired rescue arm is refused at the door, never read. + const retired = await stack.apiAs(adminToken, 'GET', '/approvals/requests?status=pending&unstaffed=true'); + expect(retired.status).toBe(400); + expect(((await retired.json()) as { error?: { code?: string } }).error?.code).toBe('VALIDATION_ERROR'); + } finally { + await stack.stop(); + } + }, + 120_000, + ); +}); diff --git a/packages/qa/dogfood/test/approval-override-composite-pin.dogfood.test.ts b/packages/qa/dogfood/test/approval-override-composite-pin.dogfood.test.ts index 2fcb3b5496d..8eef69ed9cb 100644 --- a/packages/qa/dogfood/test/approval-override-composite-pin.dogfood.test.ts +++ b/packages/qa/dogfood/test/approval-override-composite-pin.dogfood.test.ts @@ -13,8 +13,11 @@ // ## What this measures, on the wire, on a real boot // // A plain member (never the platform admin) submits a record into a flow -// whose sole node routes to a POSITION NOBODY HOLDS — the "stranded" shape: -// an unresolved approver slate, `lockRecord: true`, otherwise undecidable. +// whose sole node routes to a NAMED APPROVER WHO IS NOT THERE TO ACT — the +// "stranded" shape: a slate no one present can decide, `lockRecord: true`, +// otherwise undecidable. (Not a position nobody holds: under the default +// `admin_rescue` that slate now opens on the organization's administrators, +// #22824, so it is no longer stranded.) // The platform admin then reads the SAME request the console's approvals // inbox would, on both faces (`listRequests` / `getRequest`), and the SAME // `sys_approval_request` action metadata `GET /meta/object` serves — then @@ -48,7 +51,11 @@ import { celEngine } from '@objectstack/formula'; import type { Expression } from '@objectstack/spec'; import { ApprovalsServicePlugin } from '@objectstack/plugin-approvals'; import { RecordChangeTriggerPlugin } from '@objectstack/trigger-record-change'; -import { overrideCompositeStack, overrideCompositeSecurity } from './fixtures/override-composite-fixture.js'; +import { + overrideCompositeStack, + overrideCompositeSecurity, + DEPARTED_APPROVER, +} from './fixtures/override-composite-fixture.js'; /** The four #3424 override-admitted decision levers — served `visible` must OR in `can_override`. */ const OVERRIDE_LEVERS = ['approval_approve', 'approval_reject', 'approval_reassign', 'approval_recall'] as const; @@ -123,7 +130,7 @@ describe('approval override composite (#16679)', () => { expect(listBody.data.length).toBe(1); const listRow = listBody.data[0]; expect(listRow.status).toBe('pending'); - expect(listRow.pending_approvers).toEqual(['position:override_composite_unstaffed']); + expect(listRow.pending_approvers).toEqual([DEPARTED_APPROVER]); expect(listRow.lock_record).toBe(true); expect(listRow.viewer).toBeTruthy(); diff --git a/packages/qa/dogfood/test/approval-snapshot-credential-field.dogfood.test.ts b/packages/qa/dogfood/test/approval-snapshot-credential-field.dogfood.test.ts index a461fce1f58..dc431b46b2f 100644 --- a/packages/qa/dogfood/test/approval-snapshot-credential-field.dogfood.test.ts +++ b/packages/qa/dogfood/test/approval-snapshot-credential-field.dogfood.test.ts @@ -143,9 +143,16 @@ describe('approval snapshot of a record with credential-class and internal field expect(approverId).toBeTruthy(); // Staff the position BEFORE the records exist: the slate resolves at - // request creation. + // request creation. The assignment carries the holder's organization, + // as a write through a door does: an unstamped row is UNKNOWN tenancy, + // which the slate's position expansion does not read, and an + // all-literal slate would then open on the organization's + // administrators instead (#22824). + const membership = await ql.findOne('sys_member', { where: { user_id: approverId }, context: { isSystem: true } }); await ql.insert('sys_position', { id: 'pos_credsnap', name: POSITION, label: 'Reviewer', active: true }, SYS); - await ql.insert('sys_user_position', { id: 'hold_credsnap', user_id: approverId, position: POSITION }, SYS); + await ql.insert('sys_user_position', { + id: 'hold_credsnap', user_id: approverId, position: POSITION, organization_id: membership?.organization_id, + }, SYS); // 1) Through the HTTP data door. const createRes = await stack.apiAs(submitterToken, 'POST', `/data/${OBJECT}`, { diff --git a/packages/qa/dogfood/test/approval-snapshot-masked-field.dogfood.test.ts b/packages/qa/dogfood/test/approval-snapshot-masked-field.dogfood.test.ts index 2fc33c5c4fd..b4d7d1e01b4 100644 --- a/packages/qa/dogfood/test/approval-snapshot-masked-field.dogfood.test.ts +++ b/packages/qa/dogfood/test/approval-snapshot-masked-field.dogfood.test.ts @@ -187,10 +187,16 @@ describe('[#20964] approval snapshot field served masked on the data plane', () expect(submitterId && memberId && unmaskerId).toBeTruthy(); // Staff the position BEFORE the record exists: the slate resolves at - // request creation. + // request creation. The assignments carry the holders' organization, + // as a write through a door does: an unstamped row is UNKNOWN tenancy, + // which the slate's position expansion does not read, and an + // all-literal slate would then open on the organization's + // administrators instead (#22824). + const membership = await ql.findOne('sys_member', { where: { user_id: memberId }, context: { isSystem: true } }); + const organization_id = membership?.organization_id; await ql.insert('sys_position', { id: 'pos_snapmask', name: POSITION, label: 'Reviewer', active: true }, SYS); - await ql.insert('sys_user_position', { id: 'hold_snapmask_m', user_id: memberId, position: POSITION }, SYS); - await ql.insert('sys_user_position', { id: 'hold_snapmask_u', user_id: unmaskerId, position: POSITION }, SYS); + await ql.insert('sys_user_position', { id: 'hold_snapmask_m', user_id: memberId, position: POSITION, organization_id }, SYS); + await ql.insert('sys_user_position', { id: 'hold_snapmask_u', user_id: unmaskerId, position: POSITION, organization_id }, SYS); const unmask = await ql.findOne('sys_permission_set', { where: { name: unmaskSet.name }, context: { isSystem: true } }); expect(unmask?.id, 'fixture permission set seeded').toBeTruthy(); for (const userId of [unmaskerId, submitterId]) { diff --git a/packages/qa/dogfood/test/approval-unstaffed-arm.dogfood.test.ts b/packages/qa/dogfood/test/approval-unstaffed-arm.dogfood.test.ts deleted file mode 100644 index e3c65c90bba..00000000000 --- a/packages/qa/dogfood/test/approval-unstaffed-arm.dogfood.test.ts +++ /dev/null @@ -1,151 +0,0 @@ -// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. -// -// #22725 — an `admin_rescue` request was in no one's pending queue. -// -// A node whose approvers resolve to no holder opens, under the default -// `onEmptyApprovers: 'admin_rescue'`, on a slate that names no person (here -// the literal `position:

`). A privileged admin may decide it, but the -// inbox's "My Pending" door asked by the caller's own approver identities, and -// an admin's identities never equal an unstaffed `position:

`. So no My -// Pending listed it, the bell counted 0, and the `lockRecord: true` record -// stayed locked until someone browsed the unfiltered list (measured on HotCRM). -// -// The fix is the `unstaffed` arm on `GET /approvals/requests`, ORed with -// `approverId`. This pins the card's three cases on a real boot, through the -// door the console calls, on both faces it reads (the paged list and its -// `total`, which is the bell's number): -// -// ⭐ an admin_rescue request appears for an override actor, with -// `viewer.can_override: true`, beside the admin's own identities; -// ⭐ it does not appear for a member, who sent the same parameter; -// ⭐ CONTROL: a staffed request's queue is unchanged. Its approver reads the -// same rows with the arm and without it, and the arm never claims it. -// -// And the door's half: a value other than `true` / `false` is refused, so a -// malformed ask never reads as an empty rescue queue. - -import { describe, it, expect } from 'vitest'; -import { bootStack } from '@objectstack/verify'; -import { ApprovalsServicePlugin } from '@objectstack/plugin-approvals'; -import { RecordChangeTriggerPlugin } from '@objectstack/trigger-record-change'; -import { - unstaffedArmStack, - unstaffedArmSecurity, - UNSTAFFED_POSITION, - STAFFED_APPROVER_EMAIL, -} from './fixtures/unstaffed-arm-fixture.js'; - -const SYS = { context: { isSystem: true } } as const; -const SUBMITTER = 'unstaffed-arm-submitter@example.com'; -/** `bootStack`'s seeded dev admin: a platform admin, so an override actor. */ -const ADMIN_EMAIL = 'admin@objectos.ai'; - -interface Row { - id: string; - object_name?: string; - pending_approvers?: string[]; - viewer?: { can_act: boolean; is_submitter: boolean; can_override: boolean }; -} -interface ListBody { - data: Row[]; - total?: number; -} - -describe('the approvals inbox\'s unstaffed arm, through the REST door (#22725)', () => { - it( - 'returns an admin_rescue request to an override actor, not to a member, and leaves a staffed queue unchanged', - async () => { - const stack = await bootStack(unstaffedArmStack as unknown as Parameters[0], { - automation: true, - security: unstaffedArmSecurity(), - extraPlugins: [new RecordChangeTriggerPlugin(), new ApprovalsServicePlugin()], - }); - try { - const ql: any = await stack.kernel.getServiceAsync('objectql'); - const idOf = async (email: string): Promise => { - const u = await ql.findOne('sys_user', { where: { email }, context: SYS.context }); - return String(u?.id ?? ''); - }; - // Seed the dev admin FIRST, so the sign-ups below are plain members. - const adminToken = await stack.signIn(); - const submitterToken = await stack.signUp(SUBMITTER); - const approverToken = await stack.signUp(STAFFED_APPROVER_EMAIL); - const submitterId = await idOf(SUBMITTER); - const approverId = await idOf(STAFFED_APPROVER_EMAIL); - const adminId = await idOf(ADMIN_EMAIL); - expect(submitterId).not.toBe(''); - expect(approverId).not.toBe(''); - expect(adminId).not.toBe(''); - - // A plain member submits both records: one opens on the unstaffed - // literal (admin_rescue), one on the named approver (the control). - const stuckRes = await stack.apiAs(submitterToken, 'POST', '/data/unstaffed_arm_stuck', { name: 'stuck' }); - expect(stuckRes.status).toBe(201); - const staffedRes = await stack.apiAs(submitterToken, 'POST', '/data/unstaffed_arm_staffed', { name: 'staffed' }); - expect(staffedRes.status).toBe(201); - - const list = async (token: string, query: string): Promise => { - const res = await stack.apiAs(token, 'GET', `/approvals/requests?${query}`); - expect(res.status, `GET /approvals/requests?${query}`).toBe(200); - return (await res.json()) as ListBody; - }; - - // The scene, before anything is read off it: both requests are open, - // and each slate is what its half needs. A zero-row answer below must - // never be readable as "nothing was opened". - const all = await list(adminToken, 'status=pending'); - expect(all.data).toHaveLength(2); - const stuck = all.data.find((r) => r.object_name === 'unstaffed_arm_stuck'); - const staffed = all.data.find((r) => r.object_name === 'unstaffed_arm_staffed'); - expect(stuck?.pending_approvers).toEqual([`position:${UNSTAFFED_POSITION}`]); - expect(staffed?.pending_approvers).toEqual([STAFFED_APPROVER_EMAIL]); - - // ⭐ 1 — an override actor finds it. Beside their own identities (the - // console's "My Pending" shape) and alone, on both faces. - const adminIdentities = `&approverId=${encodeURIComponent(adminId)},${encodeURIComponent(ADMIN_EMAIL)}`; - const before = await list(adminToken, `status=pending${adminIdentities}&limit=50`); - expect(before.data.map((r) => r.id)).not.toContain(stuck!.id); - for (const query of [ - `status=pending${adminIdentities}&unstaffed=true&limit=50`, - 'status=pending&unstaffed=true&limit=50', - ]) { - const got = await list(adminToken, query); - expect(got.data.map((r) => r.id), query).toEqual([stuck!.id]); - expect(got.total, query).toBe(1); - expect(got.data[0].viewer).toEqual({ can_act: false, is_submitter: false, can_override: true }); - } - - // ⭐ 2 — a member who sends the same parameter gets nothing extra. - const memberQuery = `status=pending&approverId=${encodeURIComponent(submitterId)},${encodeURIComponent(SUBMITTER)}`; - for (const query of [`${memberQuery}&unstaffed=true&limit=50`, 'status=pending&unstaffed=true&limit=50']) { - const got = await list(submitterToken, query); - expect(got.data, query).toEqual([]); - expect(got.total, query).toBe(0); - } - - // ⭐ 3 — CONTROL: the staffed request's approver reads the same queue - // with the arm and without it, and the arm never claims that request. - const approverQuery = `status=pending&approverId=${encodeURIComponent(approverId)},${encodeURIComponent(STAFFED_APPROVER_EMAIL)}&limit=50`; - const without = await list(approverToken, approverQuery); - const withArm = await list(approverToken, `${approverQuery}&unstaffed=true`); - expect(without.data.map((r) => r.id)).toEqual([staffed!.id]); - expect(without.total).toBe(1); - expect(withArm.data.map((r) => r.id)).toEqual(without.data.map((r) => r.id)); - expect(withArm.total).toBe(without.total); - const rescue = await list(adminToken, 'status=pending&unstaffed=true'); - expect(rescue.data.map((r) => r.id)).not.toContain(staffed!.id); - - // The door's half: a value the door cannot read is refused, never - // guessed into "no arm". - const refused = await stack.apiAs(adminToken, 'GET', '/approvals/requests?status=pending&unstaffed=1'); - expect(refused.status).toBe(400); - const refusal: any = await refused.json(); - expect(refusal?.code).toBe('VALIDATION_FAILED'); - expect(refusal?.fields?.[0]).toMatchObject({ field: 'unstaffed', code: 'invalid_boolean' }); - } finally { - await stack.stop(); - } - }, - 120_000, - ); -}); diff --git a/packages/qa/dogfood/test/fixtures/unstaffed-arm-fixture.ts b/packages/qa/dogfood/test/fixtures/admin-rescue-slate-fixture.ts similarity index 55% rename from packages/qa/dogfood/test/fixtures/unstaffed-arm-fixture.ts rename to packages/qa/dogfood/test/fixtures/admin-rescue-slate-fixture.ts index fe5b57f1be9..19adb631e39 100644 --- a/packages/qa/dogfood/test/fixtures/unstaffed-arm-fixture.ts +++ b/packages/qa/dogfood/test/fixtures/admin-rescue-slate-fixture.ts @@ -1,18 +1,17 @@ // Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. // -// Minimal booted-app fixture for the #22725 pin: the approvals inbox's -// `unstaffed` arm, read through the REST door on a real boot. +// Minimal booted-app fixture for the #22824 pin: an approval step whose +// approvers resolve to no one opens, under the default +// `onEmptyApprovers: 'admin_rescue'`, on the organization's administrators, +// read through the REST door on a real boot. // // Two objects, each with one autolaunched `approval` flow that fires on insert: // -// - `unstaffed_arm_stuck` routes to a POSITION nothing in this fixture ever +// - `admin_rescue_stuck` routes to a POSITION nothing in this fixture ever // staffs. Its node declares no `onEmptyApprovers`, so the default, -// `admin_rescue`, opens the request on the literal `position:

` slot: -// decidable only by a privileged admin, and before #22725 in nobody's -// queue. -// - `unstaffed_arm_staffed` routes to one named user, by email. Its slate -// names a person, so it is the CONTROL: the arm must never claim it, and -// its approver's queue must read the same with the arm and without it. +// `admin_rescue`, opens the request on the organization's administrators. +// - `admin_rescue_staffed` routes to one named user, by email. Its slate +// names a person, so it is the CONTROL: no administrator is added to it. // // Two objects rather than two flows on one: one pending request per record, // and a record that started both flows would refuse the second. @@ -27,25 +26,25 @@ import { PermissionSetSchema, type PermissionSet } from '@objectstack/spec/secur import { SecurityPlugin, securityDefaultPermissionSets } from '@objectstack/plugin-security'; /** A position nothing in this fixture ever staffs. */ -export const UNSTAFFED_POSITION = 'unstaffed_arm_nobody'; +export const UNSTAFFED_POSITION = 'admin_rescue_nobody'; /** The one person the staffed flow routes to (by email, the slot it stores). */ -export const STAFFED_APPROVER_EMAIL = 'unstaffed-arm-approver@example.com'; +export const STAFFED_APPROVER_EMAIL = 'admin-rescue-approver@example.com'; -export const UnstaffedArmStuck = ObjectSchema.create({ - name: 'unstaffed_arm_stuck', - label: 'Unstaffed Arm Stuck', - pluralLabel: 'Unstaffed Arm Stuck', +export const AdminRescueStuck = ObjectSchema.create({ + name: 'admin_rescue_stuck', + label: 'Admin Rescue Stuck', + pluralLabel: 'Admin Rescue Stuck', sharingModel: 'public_read_write', fields: { name: Field.text({ label: 'Name', required: true }), }, }); -export const UnstaffedArmStaffed = ObjectSchema.create({ - name: 'unstaffed_arm_staffed', - label: 'Unstaffed Arm Staffed', - pluralLabel: 'Unstaffed Arm Staffed', +export const AdminRescueStaffed = ObjectSchema.create({ + name: 'admin_rescue_staffed', + label: 'Admin Rescue Staffed', + pluralLabel: 'Admin Rescue Staffed', sharingModel: 'public_read_write', fields: { name: Field.text({ label: 'Name', required: true }), @@ -83,45 +82,45 @@ function approvalFlow(name: string, objectName: string, approver: { type: 'posit }); } -export const unstaffedArmStack = defineStack({ +export const adminRescueStack = defineStack({ manifest: { - id: 'com.dogfood.unstaffed-arm', - namespace: 'unstaffed_arm', + id: 'com.dogfood.admin-rescue-slate', + namespace: 'admin_rescue', version: '0.0.0', type: 'app', - name: 'Unstaffed Arm Fixture', + name: 'Admin Rescue Slate Fixture', description: 'One request routed to a position nobody holds, one routed to a named user.', }, // ADR-0097: the flows' `record-after-create` start nodes need the trigger // capability, and `defineStack` refuses `triggers` without `automation`. requires: ['automation', 'triggers'], - objects: [UnstaffedArmStuck, UnstaffedArmStaffed], + objects: [AdminRescueStuck, AdminRescueStaffed], flows: [ - approvalFlow('unstaffed_arm_stuck_flow', 'unstaffed_arm_stuck', { type: 'position', value: UNSTAFFED_POSITION }), - approvalFlow('unstaffed_arm_staffed_flow', 'unstaffed_arm_staffed', { type: 'user', value: STAFFED_APPROVER_EMAIL }), + approvalFlow('admin_rescue_stuck_flow', 'admin_rescue_stuck', { type: 'position', value: UNSTAFFED_POSITION }), + approvalFlow('admin_rescue_staffed_flow', 'admin_rescue_staffed', { type: 'user', value: STAFFED_APPROVER_EMAIL }), ], }); -const FIXTURE_SUBMITTER_SET = 'unstaffed_arm_submitter'; +const FIXTURE_SUBMITTER_SET = 'admin_rescue_submitter'; /** * The fallback set a fresh member resolves to: create + read on both objects, - * and nothing that confers override standing. So the submitter is a plain - * member, which is what the "it does not appear for a member" half needs. + * and nothing that confers administrator standing. So a fresh sign-up is a + * plain member, which is what the "a member sees nothing" half needs. */ -export const unstaffedArmSubmitterSet: PermissionSet = PermissionSetSchema.parse({ +export const adminRescueSubmitterSet: PermissionSet = PermissionSetSchema.parse({ name: FIXTURE_SUBMITTER_SET, - label: 'Unstaffed Arm Submitter — create + read on the two fixture objects only', + label: 'Admin Rescue Submitter — create + read on the two fixture objects only', objects: { - unstaffed_arm_stuck: { allowRead: true, allowCreate: true, allowEdit: false, allowDelete: false }, - unstaffed_arm_staffed: { allowRead: true, allowCreate: true, allowEdit: false, allowDelete: false }, + admin_rescue_stuck: { allowRead: true, allowCreate: true, allowEdit: false, allowDelete: false }, + admin_rescue_staffed: { allowRead: true, allowCreate: true, allowEdit: false, allowDelete: false }, }, }); /** SecurityPlugin whose fresh-member fallback is the submitter-only set above. */ -export function unstaffedArmSecurity(): SecurityPlugin { +export function adminRescueSecurity(): SecurityPlugin { return new SecurityPlugin({ - defaultPermissionSets: [...securityDefaultPermissionSets, unstaffedArmSubmitterSet], - fallbackPermissionSet: unstaffedArmSubmitterSet.name, + defaultPermissionSets: [...securityDefaultPermissionSets, adminRescueSubmitterSet], + fallbackPermissionSet: adminRescueSubmitterSet.name, }); } diff --git a/packages/qa/dogfood/test/fixtures/my-pending-position-fixture.ts b/packages/qa/dogfood/test/fixtures/my-pending-position-fixture.ts index 18550b6b071..6df5520e671 100644 --- a/packages/qa/dogfood/test/fixtures/my-pending-position-fixture.ts +++ b/packages/qa/dogfood/test/fixtures/my-pending-position-fixture.ts @@ -2,10 +2,13 @@ // // Minimal booted-app fixture for the "My Pending" position-address pin. // -// One object, one autolaunched `approval` flow whose sole node routes to a +// One object, one autolaunched `approval` flow whose node routes to a // POSITION that nobody holds when the request opens — so the request's slate -// is the literal `position:

` slot, exactly what a HotCRM approval carries -// when its approver position is staffed only after submission. The pin then +// keeps the literal `position:

` slot, the slot a holder staffed only after +// submission decides — beside one named co-approver who never acts. The +// co-approver is what keeps the literal on the slate: a slate of literals ALONE +// names no one, and under the default `onEmptyApprovers: 'admin_rescue'` it now +// opens on the organization's administrators instead (#22824). The pin then // staffs a user into that position and asks the approvals inbox's "My // Pending" door for the request: `position:

` lists it, and the retired // `role:

` spelling (ADR-0090 D3) does not. @@ -27,6 +30,9 @@ export const ROUTED_POSITION = 'my_pending_reviewer'; /** A position the negative-control user holds — never the routed one. */ export const OTHER_POSITION = 'my_pending_bystander'; +/** The named co-approver beside the position: no account is ever this address. */ +export const CO_APPROVER = 'my-pending-co-approver@example.com'; + export const MyPendingRequest = ObjectSchema.create({ name: 'my_pending_request', label: 'My Pending Request', @@ -40,7 +46,7 @@ export const MyPendingRequest = ObjectSchema.create({ export const MyPendingFlow = defineFlow({ name: 'my_pending_flow', label: 'My Pending Flow', - description: 'Fires on insert and routes to a position nobody holds yet.', + description: 'Fires on insert and routes to a position nobody holds yet, beside a named co-approver.', type: 'autolaunched', status: 'active', nodes: [ @@ -55,7 +61,7 @@ export const MyPendingFlow = defineFlow({ type: 'approval', label: 'Review', config: { - approvers: [{ type: 'position', value: ROUTED_POSITION }], + approvers: [{ type: 'position', value: ROUTED_POSITION }, { type: 'user', value: CO_APPROVER }], behavior: 'first_response', }, }, diff --git a/packages/qa/dogfood/test/fixtures/override-composite-fixture.ts b/packages/qa/dogfood/test/fixtures/override-composite-fixture.ts index fee91231b0f..cf98d41be15 100644 --- a/packages/qa/dogfood/test/fixtures/override-composite-fixture.ts +++ b/packages/qa/dogfood/test/fixtures/override-composite-fixture.ts @@ -3,9 +3,16 @@ // Minimal booted-app fixture for the #16679 composite pin. // // One object, one autolaunched `approval` flow whose sole node routes to a -// POSITION with no holder at all — the "stranded" shape the #3424 platform -// admin override exists to rescue (an unresolved slate, `lockRecord: true`, -// otherwise undecidable). +// named approver who is not there to act — the "stranded" shape the #3424 +// platform admin override exists to rescue (a slate no one present can decide, +// `lockRecord: true`, otherwise undecidable). +// +// [#22824] A named approver, not a position with no holder: under the default +// `onEmptyApprovers: 'admin_rescue'` a slate that names NO ONE now opens on the +// organization's administrators — the seeded admin among them — so it is no +// longer stranded, and the admin reading it back would hold a slot. The +// override stays break-glass for the slate that still names someone who left, +// which is this one. // // ## Why a purpose-built fixture rather than the showcase's own // `showcase_budget_approval` (the flow the #16679 measurement round drove) @@ -18,8 +25,8 @@ // UNSTAFFED for the card's scene. A dogfood pin that re-booted showcase would // either race that seed step or have to disable it, coupling this pin's // stability to an app it does not own. A minimal object + flow answerable to -// no app's `onEnable` keeps the "unstaffed position" precondition explicit and -// permanent instead of incidental. +// no app's `onEnable` keeps the "nobody on the slate can act" precondition +// explicit and permanent instead of incidental. // // `record-after-create` (not `-after-update`) is the trigger: the record is // born already routed to the gate, with no `previous` bookkeeping needed. @@ -29,8 +36,8 @@ import { ObjectSchema, Field } from '@objectstack/spec/data'; import { PermissionSetSchema, type PermissionSet } from '@objectstack/spec/security'; import { SecurityPlugin, securityDefaultPermissionSets } from '@objectstack/plugin-security'; -/** A position nothing in this fixture ever staffs — the stranded slate. */ -export const UNSTAFFED_POSITION = 'override_composite_unstaffed'; +/** A named approver no account in this fixture ever is — the stranded slate. */ +export const DEPARTED_APPROVER = 'override-composite-departed@example.com'; export const OverrideCompositeRequest = ObjectSchema.create({ name: 'override_composite_request', @@ -47,7 +54,7 @@ export const OverrideCompositeFlow = defineFlow({ name: 'override_composite_flow', label: 'Override Composite Flow', // The stranded scene, in miniature (see the module header above). - description: 'Fires on insert and routes straight to an UNSTAFFED position.', + description: 'Fires on insert and routes straight to an approver who is not there to act.', type: 'autolaunched', status: 'active', nodes: [ @@ -62,7 +69,7 @@ export const OverrideCompositeFlow = defineFlow({ type: 'approval', label: 'Gate', config: { - approvers: [{ type: 'position', value: UNSTAFFED_POSITION }], + approvers: [{ type: 'user', value: DEPARTED_APPROVER }], behavior: 'first_response', // Locked, matching the card's own scene (`showcase_budget_approval`'s // `exec_review` rung) — the record must stay immovable while stuck. @@ -87,7 +94,7 @@ export const overrideCompositeStack = defineStack({ type: 'app', name: 'Override Composite Fixture', // The gate-composite pin's fixture app (see the module header above). - description: 'One object, one flow, one permanently-unstaffed position.', + description: 'One object, one flow, one approver who never acts.', }, // ADR-0097: a `record_change` trigger (the flow's `record-after-create` // start node) only registers when the app declares it needs the capability diff --git a/packages/qa/dogfood/test/fixtures/position-address-readers-fixture.ts b/packages/qa/dogfood/test/fixtures/position-address-readers-fixture.ts index bf5227bab05..425ca06259f 100644 --- a/packages/qa/dogfood/test/fixtures/position-address-readers-fixture.ts +++ b/packages/qa/dogfood/test/fixtures/position-address-readers-fixture.ts @@ -5,9 +5,13 @@ // Two objects, each with one autolaunched `approval` flow: // // - `pa_position_request` routes to a POSITION nobody holds when the request -// opens, so its slate is the literal `position:

` slot — the shape a -// HotCRM approval carries when its approver position is staffed only after -// submission (the #21350 fixture's precondition, owned by this pin); +// opens, so its slate keeps the literal `position:

` slot, the slot a +// holder staffed only after submission decides (the #21350 fixture's +// precondition, owned by this pin) — beside a named co-approver who never +// acts. The co-approver is what keeps the literal on the slate: a slate of +// literals ALONE names no one, and under the default +// `onEmptyApprovers: 'admin_rescue'` it now opens on the organization's +// administrators instead (#22824); // - `pa_email_request` routes to a `user` approver authored as an EMAIL, so // its slate is the email itself. // @@ -29,6 +33,9 @@ export const OTHER_POSITION = 'pa_bystander'; /** The email the email-routed request's `user` approver is authored as. */ export const EMAIL_APPROVER = 'pa-email-reviewer@example.com'; +/** The named co-approver beside the position: no account is ever this address. */ +export const CO_APPROVER = 'pa-co-approver@example.com'; + const requestObject = (name: string, label: string) => ObjectSchema.create({ name, label, @@ -42,11 +49,11 @@ const requestObject = (name: string, label: string) => ObjectSchema.create({ export const PositionRequest = requestObject('pa_position_request', 'Position Request'); export const EmailRequest = requestObject('pa_email_request', 'Email Request'); -const routedFlow = (name: string, object: string, approver: { type: 'position' | 'user'; value: string }) => +const routedFlow = (name: string, object: string, approvers: Array<{ type: 'position' | 'user'; value: string }>) => defineFlow({ name, label: name, - description: `Fires on insert and routes to a ${approver.type} approver.`, + description: `Fires on insert and routes to ${approvers.map((a) => `a ${a.type}`).join(' and ')} approver.`, type: 'autolaunched', status: 'active', nodes: [ @@ -55,7 +62,7 @@ const routedFlow = (name: string, object: string, approver: { type: 'position' | id: 'review', type: 'approval', label: 'Review', - config: { approvers: [approver], behavior: 'first_response' }, + config: { approvers, behavior: 'first_response' }, }, { id: 'approved', type: 'end', label: 'Approved' }, { id: 'rejected', type: 'end', label: 'Rejected' }, @@ -81,8 +88,11 @@ export const positionAddressReadersStack = defineStack({ requires: ['automation', 'triggers'], objects: [PositionRequest, EmailRequest], flows: [ - routedFlow('pa_position_flow', 'pa_position_request', { type: 'position', value: ROUTED_POSITION }), - routedFlow('pa_email_flow', 'pa_email_request', { type: 'user', value: EMAIL_APPROVER }), + routedFlow('pa_position_flow', 'pa_position_request', [ + { type: 'position', value: ROUTED_POSITION }, + { type: 'user', value: CO_APPROVER }, + ]), + routedFlow('pa_email_flow', 'pa_email_request', [{ type: 'user', value: EMAIL_APPROVER }]), ], }); diff --git a/packages/qa/dogfood/test/flow-node-config-values-at-registration.dogfood.test.ts b/packages/qa/dogfood/test/flow-node-config-values-at-registration.dogfood.test.ts index 0df763c15e2..47fb243e059 100644 --- a/packages/qa/dogfood/test/flow-node-config-values-at-registration.dogfood.test.ts +++ b/packages/qa/dogfood/test/flow-node-config-values-at-registration.dogfood.test.ts @@ -51,7 +51,11 @@ import { ApprovalsServicePlugin } from '@objectstack/plugin-approvals'; import { RecordChangeTriggerPlugin } from '@objectstack/trigger-record-change'; const OBJECT = 'esc_value_request'; -/** A position nothing in this fixture staffs: the request opens and waits. */ +/** + * A position nothing in this fixture staffs: the request opens, and under the + * default `onEmptyApprovers: 'admin_rescue'` it opens on the organization's + * administrators (the signed-in dev admin owns the default organization). + */ const UNSTAFFED = 'esc_value_unstaffed'; const PACKAGED_SUB_HOUR = 'esc_value_packaged_sub_hour'; @@ -274,7 +278,9 @@ describe('a flow node config its contract refuses is refused at registration, an const rows = ((await pending.json()) as { data: Array> }).data; const opened = rows.filter((row) => String(row.record_id) === recordId); expect(opened.length, JSON.stringify(rows)).toBe(1); - expect(opened[0].pending_approvers).toEqual([`position:${UNSTAFFED}`]); + const ql: any = await stack.kernel.getServiceAsync('objectql'); + const admin = await ql.findOne('sys_user', { where: { email: 'admin@objectos.ai' }, context: { isSystem: true } }); + expect(opened[0].pending_approvers).toEqual([String(admin?.id)]); }); it('package load: a flow the kernel:ready bind refuses is not left registered from the boot pull', async () => { diff --git a/packages/qa/dogfood/test/my-pending-position-address.dogfood.test.ts b/packages/qa/dogfood/test/my-pending-position-address.dogfood.test.ts index 61c8385bf33..e3bddddee2e 100644 --- a/packages/qa/dogfood/test/my-pending-position-address.dogfood.test.ts +++ b/packages/qa/dogfood/test/my-pending-position-address.dogfood.test.ts @@ -39,6 +39,7 @@ import { myPendingSecurity, ROUTED_POSITION, OTHER_POSITION, + CO_APPROVER, } from './fixtures/my-pending-position-fixture.js'; const SYS = { context: { isSystem: true } } as const; @@ -72,7 +73,8 @@ describe('"My Pending" lists a position-routed request under position: (#21350; const reviewerId = await idOf(REVIEWER); const bystanderId = await idOf(BYSTANDER); - // Opens while NOBODY holds the routed position → the literal slot. + // Opens while NOBODY holds the routed position → the literal slot, + // beside the named co-approver (see the fixture's header). const createRes = await stack.apiAs(submitterToken, 'POST', '/data/my_pending_request', { name: 'r1' }); expect(createRes.status).toBe(201); @@ -95,7 +97,7 @@ describe('"My Pending" lists a position-routed request under position: (#21350; const adminToken = await stack.signIn(); const all = await myPending(adminToken, [`position:${ROUTED_POSITION}`]); expect(all.length).toBe(1); - expect(all[0].pending_approvers).toEqual([`position:${ROUTED_POSITION}`]); + expect(all[0].pending_approvers).toEqual([`position:${ROUTED_POSITION}`, CO_APPROVER]); const requestId = all[0].id; // ⭐ `position:

`, for the reviewer (the participant gate's half) 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 a153835ab3c..936072949cc 100644 --- a/packages/qa/dogfood/test/position-address-readers.dogfood.test.ts +++ b/packages/qa/dogfood/test/position-address-readers.dogfood.test.ts @@ -45,6 +45,7 @@ import { ROUTED_POSITION, OTHER_POSITION, EMAIL_APPROVER, + CO_APPROVER, } from './fixtures/position-address-readers-fixture.js'; const SYS = { context: { isSystem: true } } as const; @@ -106,13 +107,14 @@ describe('every slot reader takes the caller\'s acting addresses (#21379)', () = const created = await stack.apiAs(submitterToken, 'POST', '/data/pa_position_request', { name }); expect(created.status).toBe(201); const rows = (await list(adminToken, [SLOT])).filter((r) => !seen.has(r.id)); - expect(rows.map((r) => r.pending_approvers)).toEqual([[SLOT]]); + expect(rows.map((r) => r.pending_approvers)).toEqual([[SLOT, CO_APPROVER]]); seen.add(rows[0].id); return rows[0].id; }; // Three requests opened before anyone holds the position, so each - // slate is the literal slot. + // slate holds the literal slot (beside the named co-approver, see the + // fixture's header). const tableRequest = await openPositionRequest('table'); const consoleSpellingRequest = await openPositionRequest('console-spelling'); const adminRequest = await openPositionRequest('admin'); diff --git a/packages/rest/src/rest-server-approvals-unknown-filter.test.ts b/packages/rest/src/rest-server-approvals-unknown-filter.test.ts index 071dd7a2a6d..87b7d932eb0 100644 --- a/packages/rest/src/rest-server-approvals-unknown-filter.test.ts +++ b/packages/rest/src/rest-server-approvals-unknown-filter.test.ts @@ -253,68 +253,30 @@ describe('#7527 §2 — every parameter the route really reads still works', () it('every name in the closed set is accepted — none of them is refused', async () => { // Asserts against the exported array rather than a hand-copied list, so // the pin cannot drift away from what the route actually declares. - // `unstaffed` (#22725) reads `true` / `false` only, so it is sent one of - // its own values; `'1'` is its refusal case in §4. - const SAMPLE: Readonly> = { unstaffed: 'true' }; for (const name of APPROVAL_REQUEST_LIST_PARAMS) { const { drive } = boot(); - const answer = await drive({ [name]: SAMPLE[name] ?? '1' }); + const answer = await drive({ [name]: '1' }); expect(answer.status, `"${name}" is declared supported but was refused`).toBe(200); } }); }); // ───────────────────────────────────────────────────────────────────────────── -// 4. #22725 — `unstaffed`, the rescue arm, is a DECLARED parameter +// 4. #22824 — `unstaffed` is retired, so it is refused like any unknown name // ───────────────────────────────────────────────────────────────────────────── -describe('#22725 §4 — `unstaffed` is declared, read as a boolean, and refused otherwise', () => { - it.each([ - ['true', true], - ['false', false], - ])('?unstaffed=%s reaches listRequests AND countRequests as %s', async (raw, value) => { +describe('#22824 §4 — the retired `unstaffed` arm is no longer a parameter of this route', () => { + // An empty `admin_rescue` slate now opens on the organization's + // administrators, who find the request under their own `approverId`, so + // the rescue arm that returned it to override actors is gone. A caller + // still sending it is told so, never answered as if it had been read. + it.each(['true', 'false'])('?unstaffed=%s is refused 400, and the service never runs', async (raw) => { const { drive, approvals } = boot(); - const answer = await drive({ status: 'pending', approverId: 'u_42', unstaffed: raw, limit: '10' }); - expect(answer.status).toBe(200); - expect(answer.body?.total).toBe(2); - // The arm rides beside `approverId` on the SAME filter: the service ORs - // them, so the door must hand over both, unchanged. - const expected = expect.objectContaining({ approverId: ['u_42'], unstaffed: value, status: 'pending' }); - expect(approvals.listRequests).toHaveBeenCalledWith(expected, expect.anything()); - expect(approvals.countRequests).toHaveBeenCalledWith(expected, expect.anything()); - }); - - it('absent, the service is handed no arm at all', async () => { - const { drive, approvals } = boot(); - const answer = await drive({ approverId: 'u_42' }); - expect(answer.status).toBe(200); - expect(approvals.listRequests.mock.calls[0][0].unstaffed).toBeUndefined(); - }); - - it.each(['1', 'TRUE', 'yes', ''])('?unstaffed=%j is refused 400 invalid_boolean, and the service never runs', async (raw) => { - const { drive, approvals } = boot(); - const answer = await drive({ approverId: 'u_42', unstaffed: raw }); - expect( - answer.status, - `expected a 400 refusal for unstaffed=${JSON.stringify(raw)}, got ${answer.status} ` - + `with body ${JSON.stringify(answer.body)}`, - ).toBe(400); - expect(answer.body?.code).toBe('VALIDATION_FAILED'); - expect(answer.body?.fields?.[0]).toMatchObject({ field: 'unstaffed', code: 'invalid_boolean' }); - // A guessed `false` would have answered 200 without the arm the caller - // asked for: the defect this refusal exists to stop. + const answer = await drive({ status: 'pending', approverId: 'u_42', unstaffed: raw }); + expectRefusal(answer, 'unstaffed'); expect(approvals.listRequests).not.toHaveBeenCalled(); expect(approvals.countRequests).not.toHaveBeenCalled(); }); - - it('a repeated ?unstaffed= is refused by the multiplicity gate, before any read', async () => { - const { drive, approvals } = boot(); - const answer = await drive({ unstaffed: ['true', 'false'] }); - expect(answer.status).toBe(400); - expect(answer.body?.error?.code).toBe('VALIDATION_ERROR'); - expect(String(answer.body?.error?.message)).toContain('unstaffed'); - expect(approvals.listRequests).not.toHaveBeenCalled(); - }); }); // ───────────────────────────────────────────────────────────────────────────── diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index 7e8bcc72bc1..0d3fa6c9bfd 100644 --- a/packages/rest/src/rest-server.ts +++ b/packages/rest/src/rest-server.ts @@ -584,10 +584,9 @@ export function apiAccessDenialFromEnable( * * Measured from the handler's own reads, not from the card or the docs: the * five filters (`object`, `recordId`, `status`, `approverId`, `submitterId`), - * the `unstaffed` arm (#22725), the free-text `q`, the paging pair (`limit`, - * `offset`), and the snake_case alias spellings the handler honours for the - * three camelCase filters. A name outside this set is refused with a located - * `400` instead of being dropped. + * the free-text `q`, the paging pair (`limit`, `offset`), and the snake_case + * alias spellings the handler honours for the three camelCase filters. A name + * outside this set is refused with a located `400` instead of being dropped. * * Exported so the pin tests assert against THIS array rather than a * hand-copied second list that can drift away from what the route accepts. @@ -597,7 +596,6 @@ export const APPROVAL_REQUEST_LIST_PARAMS: readonly string[] = [ 'recordId', 'record_id', 'status', 'approverId', 'approver_id', - 'unstaffed', 'submitterId', 'submitter_id', 'q', 'limit', 'offset', @@ -810,45 +808,6 @@ function readDeclaredQueryNumber( ); } -/** - * [#22725] Read ONE boolean query parameter on a door whose request has no - * declared schema: `true` or `false`, and nothing else. - * - * The bare form `q.x === 'true'` is the defect this refuses to write. It does - * not fail; it reads every other spelling as `false`. `?unstaffed=1` or - * `?unstaffed=TRUE` would then answer `200` without the arm the caller asked - * for, and that answer is shaped exactly like "there is nothing to rescue". - * So a third spelling is refused, never guessed. This is the rule - * `@objectstack/runtime`'s `parseBooleanParam` applies on the notifications - * inbox's `?read=`; this package cannot import it (runtime depends on rest). - * The accepted set and the field code are the same. - * - * Absent is `undefined`. A present-but-empty `?x=` is refused: no answer was - * ever given for it, so there is none to preserve (contrast the per-door - * judgement in {@link readDeclaredQueryNumber}). A real boolean, which an - * in-process delegation can hand over, is itself. - * - * THROWN as `validationFailure`, never written here, so the door's - * `handleRouteError` answers `400 VALIDATION_FAILED` with `fields[0]` - * `{ field: , code: 'invalid_boolean' }`. That is the envelope its - * `limit` / `offset` refusals answer. Call it AFTER - * `refuseRepeatedQueryParams` has run for `param`. - */ -function readDeclaredQueryBoolean( - queryParams: Record | undefined, - param: string, -): boolean | undefined { - const raw = queryParams?.[param]; - if (raw === undefined || raw === null) return undefined; - if (raw === true || raw === 'true') return true; - if (raw === false || raw === 'false') return false; - const received = typeof raw === 'string' - ? JSON.stringify(raw.length > 40 ? `${raw.slice(0, 40)}…` : raw) - : 'a structured value'; - const message = `Invalid \`${param}\` query parameter: expected \`true\` or \`false\`, received ${received}`; - throw validationFailure(message, [{ field: param, code: 'invalid_boolean', message }]); -} - /** * [#16674] Which `services.*` slot each `routes.*` key is the address OF. * @@ -12458,7 +12417,7 @@ export class RestServer { * are no process-authoring or submit routes anymore. * * Routes (all under {basePath}/approvals): - * GET /requests — list (filters: status, object, recordId, approverId, unstaffed, submitterId) + * GET /requests — list (filters: status, object, recordId, approverId, submitterId) * GET /requests/:id — get request * POST /requests/:id/approve — record an approve decision (resumes the flow) * POST /requests/:id/reject — record a reject decision (resumes the flow) @@ -12611,7 +12570,7 @@ export class RestServer { // PURPOSE. Everything else here narrows the list to one // value and is declared single-valued. if (refuseRepeatedQueryParams(req, res, [ - 'object', 'recordId', 'record_id', 'status', 'unstaffed', + 'object', 'recordId', 'record_id', 'status', 'submitterId', 'submitter_id', 'q', 'limit', 'offset', ])) return; const q = req.query ?? {}; @@ -12640,18 +12599,11 @@ export class RestServer { // door cannot read is the caller's `400`. let limit: number | undefined; let offset: number | undefined; - // [#22725] The `unstaffed` arm: the pending requests whose - // slate names no person, for a caller who may override - // them, ORed with `approverId` by the service - // (the contract's member docblock). Read `true` / `false` - // only: a guessed `false` would be a 200 without the arm. - let unstaffed: boolean | undefined; try { limit = readDeclaredQueryNumber(q, 'limit', UNDECLARED_WHOLE_NUMBER_PARAM, { emptyIsAbsent: false }); offset = readDeclaredQueryNumber(q, 'offset', UNDECLARED_WHOLE_NUMBER_PARAM, { emptyIsAbsent: false }); - unstaffed = readDeclaredQueryBoolean(q, 'unstaffed'); } catch (refusal: any) { handleRouteError(res, refusal); return; @@ -12661,7 +12613,6 @@ export class RestServer { recordId: q.recordId ?? q.record_id, status: q.status, approverId: approverIds.length ? approverIds : undefined, - unstaffed, submitterId: q.submitterId ?? q.submitter_id, q: typeof q.q === 'string' ? q.q : undefined, limit, diff --git a/packages/spec/src/automation/approval.zod.ts b/packages/spec/src/automation/approval.zod.ts index ba3680f2dde..206fc815f6e 100644 --- a/packages/spec/src/automation/approval.zod.ts +++ b/packages/spec/src/automation/approval.zod.ts @@ -916,19 +916,23 @@ export const ApprovalNodeConfigSchema = lazySchema(() => strictObject( * #3447 P2: what happens when approver resolution yields NO concrete person * at node entry (an empty expression result, an unstaffed position, an empty * multi-select field, …): - * - `admin_rescue` (default) — open the request anyway and warn loudly; a - * privileged admin can take it over via Reassign (#3424). The only option - * that neither waves the record through nor kills the run — approval's job - * is to gatekeep, so an empty slate must never silently pass. + * - `admin_rescue` (default) — open the request on the request + * organization's administrators instead: the people holding its + * organization-admin capability (ADR-0095 D3), as ordinary approvers in + * their inbox, told on opening, deciding with an ordinary approval. Warns + * loudly. The only option that neither waves the record through nor kills + * the run — approval's job is to gatekeep, so an empty slate must never + * silently pass. An organization with no administrator leaves the request + * on the empty slate, decidable only by a privileged admin's override + * (#3424). * - `fail` — the node fails (fault edge / run failure). Choose when an empty * slate can only mean a configuration bug. * - `auto_approve` — skip the request and continue down the `approve` edge * with `output.autoApproved = true`. The DingTalk/Feishu default; opt-in * here because it silently waves the record through. * - `fallback` — open the request on {@link ApprovalNodeConfig.fallbackApprovers} - * instead. The only policy that NAMES people, so it is the only one that - * turns an empty slate into a request someone can actually decide rather - * than a state an operator has to recover from. Measured elsewhere as + * instead: the way to NAME who decides, rather than leave it to the + * organization's administrators. Measured elsewhere as * Entra's "Add fallback" and Odoo's "If empty, the approval is done by an * Administrator or Approver" — taken here at the NODE, not on the * `manager` rung: all five graph approver types share the same dead end @@ -944,7 +948,8 @@ export const ApprovalNodeConfigSchema = lazySchema(() => strictObject( onEmptyApprovers: z.enum(['admin_rescue', 'fail', 'auto_approve', 'fallback']).default('admin_rescue') .describe( 'Behavior when no concrete approver resolves at node entry — ' - + "'fallback' opens the request on fallbackApprovers instead", + + "'admin_rescue' (default) opens the request on the organization's administrators; " + + "'fallback' opens it on fallbackApprovers instead, naming who decides", ), /** diff --git a/packages/spec/src/contracts/approval-service.ts b/packages/spec/src/contracts/approval-service.ts index d9f4cb6dbf0..26f0fb6bc3d 100644 --- a/packages/spec/src/contracts/approval-service.ts +++ b/packages/spec/src/contracts/approval-service.ts @@ -845,9 +845,7 @@ export interface ApprovalDecisionResult { export interface IApprovalService { /** * "My approvals" inbox. Supports filtering by status, target object, - * record id, or by who is expected to act next: the caller's approver - * identities (`approverId`) and, for a caller who may override them, the - * pending requests nobody on the slate can decide (`unstaffed`). + * record id, or by the user expected to act next. */ listRequests( filter: { @@ -862,39 +860,6 @@ export interface IApprovalService { * instead of one request per identity. */ approverId?: string | string[]; - /** - * Also match the PENDING requests whose slate names no person, returned - * only to a caller who may override them. These are the requests the - * `onEmptyApprovers: 'admin_rescue'` default leaves to a privileged - * override. - * - * Unstaffed means the request's current `pending_approvers` holds no - * user id and no email: only `type:value` literals left by an approver - * target with no holder (`position:

`, `team:`), or nothing at all. - * It is the same test the open path makes before it warns that a - * request is "decidable only by a privileged admin". It reads the slate - * as it stands, not the directory: a slate whose last named approver - * has already acted and left only literals matches too, and so does a - * `position:

` slot whose position gained a holder after the request - * opened (that holder also finds it under `approverId`). A slate that - * still names a person never matches, even when that person has since - * left the organization. - * - * "May override" is the privileged-override rule that admits the - * override decision itself and sets `viewer.can_override`. A request - * this member adds is never one the caller is refused on: each returned - * row reads `viewer.can_override: true`. A caller who may override - * nothing gets nothing from it. No row is added and nothing is refused. - * - * It is an ARM of the approver match, not a narrowing of it. With - * `approverId`, a request matches when EITHER holds (OR): one of those - * identities holds a pending slot, or the request is unstaffed and the - * caller may override it. So one request answers "my pending approvals, - * rescues included", and its `countRequests` total counts both arms. - * Without `approverId` it selects the unstaffed requests alone. `false` - * is the same as absent. Every other member still narrows the result. - */ - unstaffed?: boolean; submitterId?: string; /** * Free-text search, pushed into the engine query: matches the source @@ -904,13 +869,11 @@ export interface IApprovalService { q?: string; /** * Page window, applied by the engine for every filter; a status array - * pushes down as `$in`. The two "who acts next" arms (`approverId`, - * `unstaffed`) first resolve to a bounded set of request ids: the - * approver index for `approverId`, the pending-request scan for - * `unstaffed`, each within the implementation's id window - * (`APPROVER_INDEX_CAP`, 10,000 in `@objectstack/plugin-approvals`, - * which warns when a probe reaches it). The window then applies - * engine-side to the rows in that set. + * pushes down as `$in`. `approverId` first resolves to a bounded set of + * request ids through the approver index, within the implementation's + * id window (`APPROVER_INDEX_CAP`, 10,000 in + * `@objectstack/plugin-approvals`, which warns when a probe reaches + * it). The window then applies engine-side to the rows in that set. */ limit?: number; offset?: number;