fix(approvals): the #3424 override reaches a pending request only — refuse override-recall of a returned request (#12775)

.changeset/approval-override-recall-pending-only.md

@@ -0,0 +1,47 @@
+---
+"@objectstack/plugin-approvals": patch
+---
+
+fix(approvals): the #3424 override reaches a `pending` request only — override-recall of a `returned` request is now refused (#12775)
+
+**Behaviour change, declared explicitly (maintainer ruling 2026-09-02) so that
+one revert of this changeset's PR restores the previous behaviour.**
+
+`ApprovalService.recall` admits two callers: the submitter, and a platform or
+tenant admin releasing a stuck request (the #3424 privileged override). Recall
+is also valid on the LATEST `returned` request of a run — the ADR-0044 revise
+window, where the submitter abandons the revision instead of resubmitting.
+Those two rules met above the state check: the override short-circuit carried
+no status test of its own, so an override actor could recall a `returned`
+request too. Nothing else on the platform said so — `isOverrideActor`'s doc
+block names a PENDING request, `attachViewers` computes
+`viewer.can_override` as `status === 'pending' && isOverrideActor(...)`, and
+the `approval_recall` action's override arm reads that flag — so the reach was
+API-only, never offered by any UI, and pinned by nothing.
+
+What changes:
+
+- **Override-recall of a `returned` request is refused.** The override
+  short-circuit in `recall` now applies only while the request is `pending`,
+  spelled exactly as the viewer flag is computed. On `returned` an override
+  actor is judged exactly as any other non-submitter and receives the existing
+  refusal: `403 FORBIDDEN` over REST, with the operation catalog's
+  `approval_recall_not_submitter` sentence. No new error code, no new envelope.
+- **Only `pending` requests are override-recallable** — the same scope as the
+  other three override levers (approve / reject / reassign are pending-only at
+  their endpoints), and the same scope the viewer flag has always declared.
+
+What does not change:
+
+- The submitter's own recall of a `returned` request (the ADR-0044 revise
+  window) is untouched; so is the submitter's recall of a `pending` one.
+- The override actor's recall of a `pending` request — the #3424 rescue of a
+  request routed to an unstaffed position — is untouched, lock release and all.
+- The refusal's message, wire code and developer log line keep their shape; the
+  log line now also names the request status it refused on.
+
+Why: the gate now agrees with the viewer flag and the documented contract at
+one point instead of disagreeing with both. If a real operator workflow
+depended on override-recalling a `returned` request, this is the change to
+revert; the `returned` record lock is already released, so the stuck-record
+rescue motive that justifies the override does not apply on that status.

content/docs/permissions/system-context.mdx

@@ -145,7 +145,7 @@ The largest single consumer — **20 of the 109 sites**.
 |:--|:---|:---|:---|:---|
 | 40 | **Approval record lock released** — a locked record is writable | plugin-approvals | Get: engine self-writes (the status mirror) pass. Lose: the lock that stops edits while an approval is live. Note there is deliberately **no admin exemption** here — only `isSystem` | `lifecycle-hooks.ts:347` |
 | 41 | Delegation write guard bypassed | plugin-approvals | Get: service / seed / import may write delegation rows naming another delegator | `lifecycle-hooks.ts:570` |
-| 42 | Approval actor / submitter / pending-approver checks bypassed (8 sites) | plugin-approvals | Get: approve, reject, recall, reassign without being a pending approver or the submitter | `plugin-approvals/src/approval-service.ts:963`, `:1072`, `:3176`, `:3322`, `:3489`, `:3560`, `:3749`, `:3789` |
+| 42 | Approval actor / submitter / pending-approver checks bypassed (8 sites) | plugin-approvals | Get: approve, reject, recall, reassign without being a pending approver or the submitter | `plugin-approvals/src/approval-service.ts:963`, `:1072`, `:3196`, `:3342`, `:3509`, `:3580`, `:3769`, `:3809` |
 | 43 | Saved-report ownership is **assignable**, and an update may reassign it | plugin-reports | Get: `ownerId` from input is honoured. A non-system caller always owns what it creates and can never reassign | `plugin-reports/src/report-service.ts:404`, `:425` |
 | 44 | Saved-report access / export / mutation gates bypassed | plugin-reports | Get: read, bulk-export and overwrite any report | `plugin-reports/src/report-service.ts:343`, `:372`, `:447`, `:684` |
 | 45 | Attachment access hooks return early (insert + update + delete, and the read AST) | service-storage | Lose: attachment visibility scoping | `attachment-access-hooks.ts:300`, `:349`, `:448`, `:524` |

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

@@ -16,6 +16,7 @@
 import { describe, it, expect, beforeEach } from 'vitest';
 import { AutomationEngine, registerScreenNodes } from '@objectstack/service-automation';
 import { APPROVAL_REVISE_NODE_TYPE } from '@objectstack/spec/automation';
+import { BUILTIN_OPERATION_MESSAGES } from '@objectstack/spec/system';
 import { ApprovalService } from './approval-service.js';
 import { registerApprovalNode } from './approval-node.js';
 import { bindApprovalLockHook, APPROVALS_HOOK_PACKAGE } from './lifecycle-hooks.js';
@@ -370,6 +371,148 @@ describe('Send back for revision (ADR-0044)', () => {
     await expect(service.resubmit(req.id, { actorId: 'submitter' }, asUser('submitter'))).rejects.toThrow(/INVALID_STATE/);
   });
 
+  describe('the #3424 override reaches `pending` only — override-recall of a `returned` request is refused (#12775)', () => {
+    // Maintainer ruling 2026-09-02 (via the director seat): the status-blind
+    // override short-circuit in `recall` is re-scoped to `pending`, spelled
+    // exactly as `attachViewers` computes `viewer.can_override`, so the gate,
+    // the `isOverrideActor` doc block and the viewer flag agree at one point.
+    // Before, the short-circuit sat above ADR-0044's widened state check and
+    // admitted an override actor on `returned` too — a reach no UI ever
+    // offered (the flag is ANDed with `status === 'pending'` where it is
+    // computed) and no pin ever held. The narrowing is deliberate and
+    // declared as a behaviour change in the package changeset; this block is
+    // the pin on it, with its reverse checks.
+    //
+    // Both #3424 override postures are exercised. This harness's requests
+    // carry no organization: a tenant admin qualifies as an override actor on
+    // them (`isOverrideActor`: a null-org request is global), and — as the
+    // `can_override` pin above notes — `loadRequest` narrows by the CALLER's
+    // org, so neither context carries a `tenantId` (the ADR-0095 `posture` is
+    // the tenant-admin signal here). Neither is the submitter, who is
+    // `submitter`.
+    const PLATFORM_ADMIN = { isSystem: false, userId: 'root', positions: [], permissions: ['admin_full_access'] } as any;
+    const TENANT_ADMIN = { isSystem: false, userId: 'org_owner', posture: 'TENANT_ADMIN', positions: [], permissions: [] } as any;
+    /**
+     * `@objectstack/rest`'s `handleApprovalError` derives 403 + the ADR-0112
+     * wire code `FORBIDDEN` from this prefix and strips it off the body — the
+     * prefix IS the status/code contract at the service seam.
+     */
+    const WIRE_CODE = /^FORBIDDEN:\s/;
+    const NOT_SUBMITTER = `FORBIDDEN: ${BUILTIN_OPERATION_MESSAGES.en.approval_recall_not_submitter}`;
+    const refusalOf = (p: Promise<unknown>) => p.then(() => null, (e: any) => e);
+
+    /** A pending request on `x1`, with the REAL record-lock hook bound. */
+    async function pendingRequest() {
+      registerReviseFlow();
+      const { runId, req } = await startFlow();
+      let hook: ((ctx: any) => Promise<void>) | undefined;
+      bindApprovalLockHook({
+        registerHook: (_e: string, h: any) => { hook = h; },
+        unregisterHooksByPackage: () => 0,
+        find: fake.find.bind(fake),
+      } as any, noopLogger);
+      expect(hook).toBeDefined();
+      const editAttempt = () => hook!({
+        object: 'fin_expense',
+        input: { id: 'x1', data: { amount: 1200 } },
+        session: { isSystem: false, positions: [] },
+      });
+      return { runId, req, editAttempt };
+    }
+
+    /** The same request, sent back by its approver: `returned`, unlocked (ADR-0044). */
+    async function returnedRequest() {
+      const opened = await pendingRequest();
+      await service.sendBack(opened.req.id, { actorId: 'u1' }, asUser('u1'));
+      const row = await service.getRequest(opened.req.id, SYSTEM_CTX);
+      expect(row!.status).toBe('returned');
+      return opened;
+    }
+
+    const runStatus = async () => (await automation.listRuns('expense_approval'))[0].status;
+
+    it('the narrowing: an override actor is refused on `returned` with the non-submitter refusal, and nothing moves', async () => {
+      const { req, editAttempt } = await returnedRequest();
+      await expect(editAttempt()).resolves.toBeUndefined();         // returned → unlocked (ADR-0044)
+      const runBefore = await runStatus();
+
+      for (const [label, actor, actorId] of [['platform admin', PLATFORM_ADMIN, 'root'], ['tenant admin', TENANT_ADMIN, 'org_owner']] as const) {
+        // The viewer flag the gate now agrees with, read by the same actor.
+        const seen = await service.getRequest(req.id, actor);
+        expect(seen!.status, label).toBe('returned');
+        expect(seen!.viewer!.can_override, label).toBe(false);
+
+        const err = await refusalOf(service.recall(req.id, { actorId, comment: 'rescue' }, actor));
+        expect(err, label).toBeInstanceOf(Error);
+        // The EXISTING non-submitter refusal, envelope and sentence: the
+        // `FORBIDDEN:` prefix is what REST maps to 403 + code `FORBIDDEN`;
+        // the sentence is the operation catalog's (#11993). No new code.
+        expect(err.message, label).toMatch(WIRE_CODE);
+        expect(err.message, label).toBe(NOT_SUBMITTER);
+      }
+
+      // Nothing moved: status, the action ledger, the record lock, the run.
+      const after = await service.getRequest(req.id, SYSTEM_CTX);
+      expect(after!.status).toBe('returned');
+      expect(await actionsOf(req.id)).not.toContain('recall');
+      await expect(editAttempt()).resolves.toBeUndefined();         // still unlocked — the refusal touched no lock
+      expect(await runStatus()).toBe(runBefore);
+      expect(await runStatus()).not.toBe('cancelled');
+      // The window is still open for its owner.
+      await service.resubmit(req.id, { actorId: 'submitter' }, asUser('submitter'));
+      const round2 = await pendingReq();
+      expect(round2.id).not.toBe(req.id);
+      expect((await service.getRequest(round2.id, SYSTEM_CTX))?.round).toBe(2);
+    });
+
+    it('reverse check: the same override actor still recalls the same request while `pending`, and the flag says so', async () => {
+      const { req, editAttempt } = await pendingRequest();
+      await expect(editAttempt()).rejects.toThrow(/RECORD_LOCKED/);   // pending → locked
+      const seen = await service.getRequest(req.id, PLATFORM_ADMIN);
+      expect(seen!.status).toBe('pending');
+      expect(seen!.viewer!.can_override).toBe(true);
+
+      const out = await service.recall(req.id, { actorId: 'root', comment: 'unstaffed role' }, PLATFORM_ADMIN);
+      expect(out.request.status).toBe('recalled');
+      expect(out.resumed).toBe(true);                                   // resumed down the reject branch
+      expect(await actionsOf(req.id)).toContain('recall');
+      await expect(editAttempt()).resolves.toBeUndefined();            // the #3424 release still happens
+    });
+
+    it('reverse check (tenant admin): admitted on `pending` too — the narrowing is about status, not posture', async () => {
+      const { req } = await pendingRequest();
+      const out = await service.recall(req.id, { actorId: 'org_owner' }, TENANT_ADMIN);
+      expect(out.request.status).toBe('recalled');
+      expect(await actionsOf(req.id)).toContain('recall');
+    });
+
+    it("the submitter's own revise-window recall is untouched (ADR-0044)", async () => {
+      const { runId, req } = await returnedRequest();
+      const out = await service.recall(req.id, { actorId: 'submitter', comment: 'never mind' }, asUser('submitter'));
+      expect(out.request.status).toBe('recalled');
+      expect(out.resumed).toBe(false);                                  // terminally cancelled, not resumed
+      expect(out.runId).toBe(runId);
+      expect(await runStatus()).toBe('cancelled');
+      expect(await actionsOf(req.id)).toContain('recall');
+    });
+
+    it('control: a non-submitter without the override is refused on `returned` exactly as before — same envelope, same sentence', async () => {
+      const { req } = await returnedRequest();
+      // The approver who sent it back: not the submitter, holds no override.
+      const plain = await refusalOf(service.recall(req.id, { actorId: 'u1' }, asUser('u1')));
+      expect(plain).toBeInstanceOf(Error);
+      expect(plain.message).toMatch(WIRE_CODE);
+      expect(plain.message).toBe(NOT_SUBMITTER);
+      // The override actor's refusal on `returned` is byte-identical to it:
+      // the narrowing added no refusal shape, it routed one more caller
+      // through the existing one.
+      const admin = await refusalOf(service.recall(req.id, { actorId: 'root' }, PLATFORM_ADMIN));
+      expect(admin.message).toBe(plain.message);
+      expect((await service.getRequest(req.id, SYSTEM_CTX))!.status).toBe('returned');
+      expect(await actionsOf(req.id)).not.toContain('recall');
+    });
+  });
+
   it('refuses resubmit while another pending request collides on the record (run stays resumable)', async () => {
     registerReviseFlow();
     const { runId, req } = await startFlow();

packages/plugins/plugin-approvals/src/approval-service.ts

@@ -2889,6 +2889,13 @@ export class ApprovalService implements IApprovalService {
    * is then paused at the revise-window node (no reject edge), so it is
    * terminally cancelled via {@link ApprovalResumeSurface.cancelRun} rather
    * than resumed.
+   *
+   * The #3424 privileged override reaches a PENDING request only (#12775,
+   * maintainer ruling 2026-09-02). On `returned` an override actor is refused
+   * exactly as any other non-submitter: the gate is spelled as `attachViewers`
+   * computes `viewer.can_override`, so the gate, the
+   * {@link ApprovalService.isOverrideActor} doc block and the viewer flag agree
+   * at one point.
    */
   async recall(
     requestId: string,
@@ -2907,21 +2914,34 @@ export class ApprovalService implements IApprovalService {
     if (raw.status !== 'pending' && !inReviseWindow) {
       throw new Error(`INVALID_STATE: request is ${raw.status}`);
     }
-    // The submitter withdraws their own request; a privileged admin may recall
-    // any pending request to release a stuck record (#3424).
+    // The submitter withdraws their own request — while it is `pending`, or
+    // while it is `returned` (ADR-0044: abandoning the revision instead of
+    // resubmitting). A privileged admin may recall a PENDING request to
+    // release a stuck record (#3424) — and only a pending one. This
+    // short-circuit used to sit above ADR-0044's widened state check with no
+    // status test of its own, so it also admitted the override caller on
+    // `returned`: a reach no UI ever offered (`viewer.can_override` is ANDed
+    // with `status === 'pending'` where it is computed, in `attachViewers`)
+    // and no pin ever held. #12775 (maintainer ruling 2026-09-02) re-scoped it
+    // to `pending`, spelled exactly as the viewer flag, so the gate, the
+    // `isOverrideActor` doc block and the flag agree at one point. On
+    // `returned` the override caller is judged exactly as any other
+    // non-submitter: the branch below, its catalog sentence, its `FORBIDDEN`
+    // wire code.
     //
-    // [#11993] The GATE is untouched — who may recall an approval is exactly
-    // what it was. Only the refusal's user-facing half changed: it used to be
+    // [#11993] Only the refusal's user-facing half changed there: it used to be
     // one hardcoded English sentence that Console rendered verbatim in a
     // toast. See {@link ApprovalService.userFacingRefusal}.
-    if (!this.isOverrideActor(context, raw.organization_id ?? null)
-      && raw.submitter_id && String(raw.submitter_id) !== String(actorId)) {
+    const overrideAdmits = raw.status === 'pending'
+      && this.isOverrideActor(context, raw.organization_id ?? null);
+    if (!overrideAdmits && raw.submitter_id && String(raw.submitter_id) !== String(actorId)) {
       // The developer's half: the ids the catalog sentence deliberately does
       // not name (the throw site knows the submitter only as an opaque user
       // id), kept where a developer reads them and a user never does.
       const developerMessage =
         `[approvals] recall refused: actor '${actorId}' is not the submitter of request `
-        + `'${requestId}' (submitter '${String(raw.submitter_id)}') and holds no #3424 override`;
+        + `'${requestId}' (submitter '${String(raw.submitter_id)}') and holds no #3424 override `
+        + `for a '${raw.status}' request (the override reaches pending requests only)`;
       this.logger?.warn?.(developerMessage, {
         request: requestId,
         actor: actorId,