fix(plugin-approvals): show approval_recall for the #3424 admin override (#12716)

.changeset/approval-recall-admin-override-arm.md

@@ -0,0 +1,34 @@
+---
+"@objectstack/plugin-approvals": patch
+---
+
+fix(plugin-approvals): the `approval_recall` action shows for the #3424 admin override (#12716)
+
+`ApprovalService.recall` has admitted two callers since #3424 — the submitter,
+and a platform/tenant admin releasing a stuck request — and `isOverrideActor`'s
+own doc block names recall as one of the four override levers in so many words.
+The declared action that reaches that endpoint did not agree: `approval_recall`'s
+`visible` predicate was submitter-only, while its three siblings
+(`approval_approve` / `approval_reject` / `approval_reassign`) each OR in
+`record.viewer.can_override`.
+
+So recall was the one lever the override covers whose button never appeared. An
+admin rescuing an approval routed to an unstaffed position could approve or
+reject their way out — writing a decision nobody made — or reassign it, but
+could not simply withdraw it. This is declared-vs-enforced drift in the less
+usual direction: a capability the server grants that no UI entry exposed.
+
+`approval_recall`'s `visible` now ORs in `record.viewer.can_override`, spelled
+byte-identically to the three siblings.
+
+Not a permission change: the service's authorisation set is untouched, and
+`can_override` was already computed server-side for every viewer.
+
+**Pending-only, and enforced rather than asserted.** The new arm carries no
+status test of its own — neither do the siblings — because the flag is already
+status-scoped where it is computed: `attachViewers` sets
+`can_override: row.status === 'pending' && isOverrideActor(...)`, ANDed, so the
+flag can never be true off `pending` and the arm is pending-only in effect
+however CEL groups the expression. The submitter's own `returned` (revise
+window) arm is unchanged. Pinned in both directions, with the flag's own scoping
+pinned against the real service on a genuinely `returned` row.

packages/plugins/plugin-approvals/src/action-predicate-sparse-face.test.ts

@@ -121,14 +121,54 @@ describe('#8990 — the fail-closed intent survives as a real false, and the lev
  expect(evaluate(visibleOf('approval_remind'), approver)).toBe(false);
  });
 
- it('an override-only admin still gets the three core decision levers and nothing else (#3424)', () => {
+ it('an override-only admin gets the four levers the override covers, and nothing else (#3424, +recall #12716)', () => {
+ // Re-expressed for #12716. This pin previously read "the three core decision
+ // levers and nothing else" — true when written, and it is the shape of pin
+ // the recall override arm was expected to turn red. It did not go red (it
+ // never asserted anything about recall), but its TITLE became false the
+ // moment recall joined the set, so it is restated rather than left to read
+ // as a claim the code no longer honours.
  const admin = { status: 'pending', viewer: { can_act: false, can_override: true, is_submitter: false } };
  expect(evaluate(visibleOf('approval_approve'), admin)).toBe(true);
  expect(evaluate(visibleOf('approval_reject'), admin)).toBe(true);
  expect(evaluate(visibleOf('approval_reassign'), admin)).toBe(true);
+ // #12716 — the fourth lever. An override admin is a NON-SUBMITTER who now
+ // sees Recall: the service has authorised them since #3424, and this is the
+ // one lever that releases a stuck request without writing a decision on
+ // someone else's behalf.
+ expect(evaluate(visibleOf('approval_recall'), admin)).toBe(true);
  // `can_override` was never OR'd into the secondary levers, and still is not.
  expect(evaluate(visibleOf('approval_send_back'), admin)).toBe(false);
  expect(evaluate(visibleOf('approval_request_info'), admin)).toBe(false);
+ // Nor into the remaining submitter levers — recall is the only one that
+ // moved, and remind/resubmit stay shut for an actor who is not the submitter.
+ expect(evaluate(visibleOf('approval_remind'), admin)).toBe(false);
+ expect(evaluate(visibleOf('approval_resubmit'), { ...admin, status: 'returned' })).toBe(false);
+ });
+
+ it('the recall override arm is pending-only in effect, because `can_override` is itself pending-scoped (#12716)', () => {
+ // The negative direction, and the reason the arm needs no status test of its
+ // own. `attachViewers` computes
+ // can_override: row.status === 'pending' && isOverrideActor(...)
+ // — ANDed — so for the SAME override actor the flag the service attaches is
+ // true on `pending` and false on `returned`. Both rows below are viewer
+ // blocks the service really emits: forcing `can_override: true` onto a
+ // `returned` row would pin a state the server never produces, and would
+ // measure CEL's grouping rather than the product's behaviour.
+ //
+ // That is what makes "pending-only" enforced rather than asserted in prose.
+ // The flag's own scoping is pinned against the REAL service — an override
+ // admin reading a genuinely `returned` request — in `approval-revise.test.ts`;
+ // this pair is the predicate half of the same claim.
+ const onPending = { status: 'pending', viewer: { can_act: false, can_override: true, is_submitter: false } };
+ const onReturned = { status: 'returned', viewer: { can_act: false, can_override: false, is_submitter: false } };
+ expect(evaluate(visibleOf('approval_recall'), onPending)).toBe(true);
+ expect(evaluate(visibleOf('approval_recall'), onReturned)).toBe(false);
+ // And the submitter's own `returned` arm is untouched by the new OR — the
+ // revise-window withdraw stays exactly as available as it was.
+ expect(evaluate(visibleOf('approval_recall'), {
+ status: 'returned', viewer: { can_act: false, can_override: false, is_submitter: true },
+ })).toBe(true);
  });
 
  it('the submitter still gets remind / recall on pending and resubmit / recall on returned', () => {

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

@@ -315,6 +315,39 @@ describe('Send back for revision (ADR-0044)', () => {
  await expect(editAttempt()).rejects.toThrow(/RECORD_LOCKED/); // round 2 pending → re-locked
  });
 
+ it('viewer.can_override drops on `returned`, which is what keeps the recall override arm pending-only (#12716)', async () => {
+ // `sys_approval_request`'s `approval_recall` action ORs in
+ // `record.viewer.can_override`, spelled exactly like its three siblings and
+ // with no status test of its own. What makes that arm pending-only is not
+ // the predicate — it is `attachViewers`, which ANDs the status in when it
+ // COMPUTES the flag. `action-predicate-sparse-face.test.ts` pins the
+ // predicate half against fixture viewer blocks; this pins the half those
+ // fixtures stand for, against a genuinely `returned` row produced by the
+ // real send-back path. Without it, "pending-only" would be a property of
+ // hand-written fixtures rather than of the service.
+ //
+ // Relaxing the flag is also not available as a fix: it feeds the three
+ // sibling predicates too, and THEIR endpoints are pending-only
+ // (`decideNode`, and reassign via `loadPendingRow`), so widening it would
+ // put those three buttons on statuses their services refuse.
+ registerReviseFlow();
+ const { req } = await startFlow();
+ // A platform admin with no tenant scope: `loadRequest` narrows by the
+ // CALLER's org, and this harness's requests carry none.
+ const ADMIN = { isSystem: false, userId: 'root', positions: [], permissions: ['admin_full_access'] } as any;
+
+ // Positive control first — the same actor, same request, on `pending`.
+ const whilePending = await service.getRequest(req.id, ADMIN);
+ expect(whilePending!.status).toBe('pending');
+ expect(whilePending!.viewer!.can_override).toBe(true);
+
+ await service.sendBack(req.id, { actorId: 'u1' }, asUser('u1'));
+
+ const whileReturned = await service.getRequest(req.id, ADMIN);
+ expect(whileReturned!.status).toBe('returned');
+ expect(whileReturned!.viewer!.can_override).toBe(false);
+ });
+
  it('recall crossing the revise window cancels the run (returned → recalled)', async () => {
  registerReviseFlow();
  const { runId, req } = await startFlow();

packages/plugins/plugin-approvals/src/sys-approval-request.object.test.ts

@@ -13,8 +13,11 @@
  * • every `type:'api'` target resolves `{id}` and points at a route that the
  * REST server actually registers (approve/reject/reassign/recall/remind/
  * request-info/revise/resubmit) — a typo'd verb would 404 silently in the UI;
- * • submitter-only levers (remind/recall/resubmit) gate on
- * `submitter_id == ctx.user.id` so a non-submitter never sees them.
+ * • submitter levers (remind/recall/resubmit) gate on the server-computed
+ * `record.viewer.is_submitter`, so a plain non-submitter never sees them.
+ * `recall` additionally ORs in the #3424 admin override (#12716) — an
+ * override admin is therefore the one non-submitter who does see it, and
+ * is a caller `ApprovalService.recall` already authorises.
  */
 
 import { describe, it, expect } from 'vitest';
@@ -74,17 +77,40 @@ describe('sys_approval_request declared actions', () => {
  }
  });
 
- it('the core decision levers OR in the admin override (#3424) so a stuck request is recoverable', () => {
- // approve / reject / reassign additionally show for a platform/tenant admin
- // (`record.viewer.can_override`) so an approval routed to an unstaffed
- // position — otherwise undecidable, locking the record forever — can be
- // rescued in-product. The secondary approver levers stay slot-only.
- for (const name of ['approval_approve', 'approval_reject', 'approval_reassign']) {
+ it('the levers the admin override covers OR it in (#3424, +recall #12716) so a stuck request is recoverable', () => {
+ // approve / reject / reassign / recall additionally show for a
+ // platform/tenant admin (`record.viewer.can_override`) so an approval routed
+ // to an unstaffed position — otherwise undecidable, locking the record
+ // forever — can be rescued in-product. Recall joined the set in #12716: the
+ // service has admitted the override caller on that endpoint since #3424
+ // (`isOverrideActor`'s doc block names recall as one of the four levers),
+ // and it is the only one that RELEASES the record without recording a
+ // decision on someone else's behalf. The secondary approver levers and the
+ // remaining submitter levers stay slot-only.
+ for (const name of ['approval_approve', 'approval_reject', 'approval_reassign', 'approval_recall']) {
  expect(vis(name)).toContain('record.viewer.can_override');
  }
- for (const name of ['approval_send_back', 'approval_request_info']) {
+ for (const name of ['approval_send_back', 'approval_request_info', 'approval_remind', 'approval_resubmit']) {
  expect(vis(name)).not.toContain('can_override');
  }
+
+ // Exhaustive, so a FIFTH lever cannot quietly join the override set without
+ // this pin moving: the two loops above only constrain the names they name.
+ expect(
+ actions.filter((a) => vis(a.name).includes('can_override')).map((a) => a.name).sort(),
+ 'exactly these levers OR in the #3424 override',
+ ).toEqual(['approval_approve', 'approval_reassign', 'approval_recall', 'approval_reject']);
+
+ // ONE spelling of the arm, not four. This file has a spelling-drift history,
+ // and a fourth wording of the same idea is its own defect — `toContain` on
+ // the whole arm (leading ` || ` included) is what makes drift fail here
+ // rather than in review.
+ const OVERRIDE_ARM =
+ ' || has(record.viewer) && has(record.viewer.can_override) && record.viewer.can_override == true';
+ for (const name of ['approval_approve', 'approval_reject', 'approval_reassign', 'approval_recall']) {
+ expect(vis(name), `${name} must spell the override arm exactly as its siblings do`)
+ .toContain(OVERRIDE_ARM);
+ }
  });
 
  it('recall stays available while a returned request is still the submitter\'s to abandon', () => {

packages/plugins/plugin-approvals/src/sys-approval-request.object.ts

@@ -328,11 +328,14 @@ export const SysApprovalRequest = ObjectSchema.create({
  // per-viewer block (#3310): approver actions on `record.viewer.can_act`
  // (the caller is a current pending approver — same check the service
  // authorizes a decision with, so position/team approvers resolve correctly),
- // submitter actions on `record.viewer.is_submitter`. The core decision levers
- // (approve/reject/reassign) additionally OR in `record.viewer.can_override`
- // (#3424) so a platform/tenant admin can rescue a request routed to an
+ // submitter actions on `record.viewer.is_submitter`. The four levers the
+ // #3424 override covers (approve/reject/reassign, and recall since #12716)
+ // additionally OR in `record.viewer.can_override`
+ // so a platform/tenant admin can rescue a request routed to an
  // unstaffed position — otherwise undecidable, locking the record forever — by
- // approving, rejecting, or reassigning it to a real approver. `viewer` is
+ // approving, rejecting, reassigning it to a real approver, or recalling it
+ // (the lever that releases the record without recording a decision nobody
+ // made). `viewer` is
  // attached by getRequest/listRequests; where it is absent the predicate fails
  // closed.
  //
@@ -476,8 +479,13 @@ export const SysApprovalRequest = ObjectSchema.create({
  // Remind / recall (pending) and resubmit / recall (returned). These are the
  // submitter's own levers, so `visible` gates on `record.viewer.is_submitter`
  // (server-computed on the current viewer). The service re-checks ownership;
- // the predicate keeps a non-submitter from ever seeing a button they cannot
- // use.
+ // the predicate keeps a plain non-submitter from ever seeing a button they
+ // cannot use.
+ //
+ // `recall` is the one exception, and it is not a widening: it ALSO ORs in
+ // the #3424 admin override (#12716), because an override admin is a caller
+ // `ApprovalService.recall` already authorises. Remind and resubmit keep no
+ // override arm.
  {
  name: 'approval_remind',
  label: 'Send reminder',
@@ -508,9 +516,29 @@ export const SysApprovalRequest = ObjectSchema.create({
  ],
  // Recall applies while the request is live for the submitter — pending
  // (withdraw) or returned (abandon the revision instead of resubmitting).
+ //
+ // The second arm is the #3424 admin override, spelled byte-identically to
+ // the three core decision levers above (#12716). `ApprovalService.recall`
+ // has admitted the override caller since #3424 — `isOverrideActor`'s own
+ // doc block names recall as one of the four levers — so until this arm
+ // landed, recall was the one authorised capability with no button: an
+ // admin could approve or reject their way out of a stuck request (writing
+ // a decision that did not happen) or reassign it, but could not withdraw.
+ //
+ // The override arm carries no status test of its own, on purpose, because
+ // it does not need one and the siblings do not have one either: the flag
+ // is already status-scoped where it is COMPUTED. `attachViewers` in
+ // `approval-service.ts` sets
+ // `can_override: row.status === 'pending' && isOverrideActor(...)` —
+ // ANDed — so `record.viewer.can_override` can never be true off `pending`,
+ // and this arm is pending-only in effect however CEL groups the
+ // expression. Pinned in both directions in
+ // `action-predicate-sparse-face.test.ts`, with the flag's own scoping
+ // pinned against the real service in `approval-revise.test.ts`.
  visible:
  'has(record.status) && (record.status == "pending" || record.status == "returned")' +
- ' && has(record.viewer) && has(record.viewer.is_submitter) && record.viewer.is_submitter == true',
+ ' && has(record.viewer) && has(record.viewer.is_submitter) && record.viewer.is_submitter == true' +
+ ' || has(record.viewer) && has(record.viewer.can_override) && record.viewer.can_override == true',
  locations: ['record_section'],
  successMessage: 'Recalled.',
  refreshAfter: true,