From 47f2f1b4b0fbe3db70f041df554671f9e768b611 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 27 Aug 2026 19:52:26 +0000 Subject: [PATCH] fix(plugin-approvals): show `approval_recall` for the #3424 admin override (#12716) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ApprovalService.recall` has admitted two callers since #3424 — the submitter, and a platform/tenant admin releasing a stuck request — but `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`. Recall was the one lever the override covers whose button never appeared: an admin could approve or reject their way out of a stuck request (writing a decision nobody made) or reassign it, but could not withdraw. OR `record.viewer.can_override` into `approval_recall`'s `visible`, 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. The arm carries no status test of its own, as the siblings do not: `attachViewers` ANDs `row.status === 'pending'` in when it computes the flag, so the arm is pending-only in effect however CEL groups the expression. Pinned in both directions, with the flag's own scoping pinned against the real service on a genuinely `returned` row. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_0194kbQJxUvv2yvsGRtuXpP5 --- .../approval-recall-admin-override-arm.md | 34 ++++++++++++++ .../src/action-predicate-sparse-face.test.ts | 42 +++++++++++++++++- .../src/approval-revise.test.ts | 33 ++++++++++++++ .../src/sys-approval-request.object.test.ts | 44 +++++++++++++++---- .../src/sys-approval-request.object.ts | 42 +++++++++++++++--- 5 files changed, 178 insertions(+), 17 deletions(-) create mode 100644 .changeset/approval-recall-admin-override-arm.md diff --git a/.changeset/approval-recall-admin-override-arm.md b/.changeset/approval-recall-admin-override-arm.md new file mode 100644 index 0000000000..c2fd433cb0 --- /dev/null +++ b/.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. diff --git a/packages/plugins/plugin-approvals/src/action-predicate-sparse-face.test.ts b/packages/plugins/plugin-approvals/src/action-predicate-sparse-face.test.ts index cac4b033ee..4f7ad80f92 100644 --- a/packages/plugins/plugin-approvals/src/action-predicate-sparse-face.test.ts +++ b/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', () => { diff --git a/packages/plugins/plugin-approvals/src/approval-revise.test.ts b/packages/plugins/plugin-approvals/src/approval-revise.test.ts index 5e2491b80a..676c5310f4 100644 --- a/packages/plugins/plugin-approvals/src/approval-revise.test.ts +++ b/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(); diff --git a/packages/plugins/plugin-approvals/src/sys-approval-request.object.test.ts b/packages/plugins/plugin-approvals/src/sys-approval-request.object.test.ts index 73fc5c7c21..c3a8af37a3 100644 --- a/packages/plugins/plugin-approvals/src/sys-approval-request.object.test.ts +++ b/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', () => { diff --git a/packages/plugins/plugin-approvals/src/sys-approval-request.object.ts b/packages/plugins/plugin-approvals/src/sys-approval-request.object.ts index 67f89782a6..7739f9a63d 100644 --- a/packages/plugins/plugin-approvals/src/sys-approval-request.object.ts +++ b/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,