fix(approvals): refuse loudly when a successful mutation's read-back is org-filtered out

.changeset/approval-readback-loud-refusal.md

@@ -0,0 +1,23 @@
+---
+"@objectstack/plugin-approvals": patch
+---
+
+fix(approvals): refuse loudly when a successful mutation's read-back is org-filtered out (#12769)
+
+Ten ApprovalService result sites (decide/decideNode, recall, sendBack, resubmit,
+reassign, remind, requestInfo, comment) read the row they just mutated back
+through the caller's organization narrowing and asserted the result non-null
+(`fresh!`). For an org-less request row — produced by construction on every
+schedule / time-relative / api trigger run (#10131; pinned rather than repaired
+by #9132) — an org-scoped caller's read-back matches nothing, so a call that
+SUCCEEDED shipped a well-formed success envelope whose declared-non-null
+`request` was `null` (HTTP 200 with `"request": null` through the REST
+pass-through), and a client dereferencing `request.status` crashed.
+
+The read-back now throws `READ_BACK_FAILED: …` instead: the write is recorded
+and NOT rolled back; only the result echo is refused, loudly. Over REST the
+error surfaces through each route's existing 500 arm (`APPROVAL_RECALL_FAILED`
+and siblings — already-registered codes) with the `READ_BACK_FAILED:` message
+in the body. No declared result type changed — the declared-non-null `request`
+is now always true because a result that cannot be built is never returned. The
+caller-org narrowing in `loadRequest` (tenancy wall) is deliberately untouched.

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

@@ -2010,6 +2010,105 @@ describe('ApprovalService — admin override (#3424)', () => {
  });
 });
 
+// #12769 — the read-back after a successful mutation narrows by the CALLER's
+// org (`loadRequest`), so an org-less request row (every schedule /
+// time-relative / api trigger run produces one — #10131, pinned rather than
+// repaired by #9132) is invisible to an org-scoped caller. Ten result sites
+// used to paper over that with a `fresh!` non-null assertion, shipping a
+// well-formed success envelope whose declared-non-null `request` was `null`.
+// The contract now: the write lands, and the call refuses LOUDLY instead of
+// returning a malformed result. The org narrowing itself is a tenancy wall
+// and is deliberately untouched by these cases.
+describe('ApprovalService — org-filtered read-back refuses loudly (#12769)', () => {
+ let engine: ReturnType<typeof makeFakeEngine>;
+ let svc: ApprovalService;
+ let n = 0;
+ const baseTime = new Date('2026-02-01T10:00:00Z').getTime();
+
+ // No tenantId — the acting-context shape of a schedule / time-relative /
+ // api trigger run, which stamps `organization_id = null` on the row.
+ const ORGLESS_SUBMITTER = { userId: 'u1', positions: [], permissions: [] } as any;
+ // Org-scoped platform admin: admitted by the override gate on any request
+ // (#3424), but their read-back narrows to 't1' and misses an org-less row.
+ const PLATFORM_ADMIN = { userId: 'root', tenantId: 't1', positions: [], permissions: ['admin_full_access'] } as any;
+ // Org-scoped ordinary slot holder — the same miss with no override involved.
+ const ORG_APPROVER = { userId: 'u9', tenantId: 't1', positions: [], permissions: [] } as any;
+
+ beforeEach(() => {
+ engine = makeFakeEngine();
+ n = 0;
+ svc = new ApprovalService({ engine: engine as any, clock: { now: () => new Date(baseTime + (n++) * 1000) } });
+ });
+
+ const openOrgless = async (recordId = 'opp1', runId = 'run_1') => {
+ const req: any = await svc.openNodeRequest({
+ object: 'opportunity', recordId, runId, nodeId: 'approve_step',
+ flowName: 'deal_approval',
+ config: { approvers: [{ type: 'user' as const, value: 'u9' }], behavior: 'first_response' as const },
+ record: { id: recordId, amount: 100 },
+ }, ORGLESS_SUBMITTER);
+ // Precondition pin: the row this family is about really is org-less.
+ expect(req.organization_id ?? null).toBeNull();
+ return req;
+ };
+
+ it('recall: the row IS mutated and the call throws READ_BACK_FAILED — not a success with request:null', async () => {
+ const req = await openOrgless();
+ await expect(svc.recall(req.id, { actorId: 'root' }, PLATFORM_ADMIN))
+ .rejects.toThrow(/^READ_BACK_FAILED: /);
+ // The refusal is about the echo only — the recall itself landed.
+ const after = await svc.getRequest(req.id, SYS);
+ expect(after?.status).toBe('recalled');
+ });
+
+ it('the refusal names the request and says the write was recorded, not rolled back', async () => {
+ const req = await openOrgless();
+ const err: any = await svc.recall(req.id, { actorId: 'root' }, PLATFORM_ADMIN)
+ .then(() => null, (e: any) => e);
+ expect(err).toBeTruthy();
+ expect(String(err.message)).toMatch(/^READ_BACK_FAILED: /);
+ expect(String(err.message)).toContain(`'${req.id}'`);
+ expect(String(err.message)).toContain('was recorded');
+ expect(String(err.message)).toContain('NOT rolled back');
+ });
+
+ it('decide: an ordinary org-scoped slot holder hits the same refusal — no override involved', async () => {
+ const req = await openOrgless();
+ await expect(svc.decideNode(req.id, { decision: 'approve', actorId: 'u9' }, ORG_APPROVER))
+ .rejects.toThrow(/^READ_BACK_FAILED: /);
+ const after = await svc.getRequest(req.id, SYS);
+ expect(after?.status).toBe('approved'); // the decision landed
+ });
+
+ it('comment (thread interaction): same refusal shape', async () => {
+ const req = await openOrgless();
+ await expect(svc.comment(req.id, { actorId: 'u9', comment: 'reviewing' }, ORG_APPROVER))
+ .rejects.toThrow(/^READ_BACK_FAILED: /);
+ const acts = await svc.listActions(req.id, SYS);
+ // `[length - 1]`, not `.at(-1)`: this package's test layer type-checks under
+ // lib ES2021 in the TEST_DEBT re-measure, where `.at` is a TS2550 (#5278).
+ expect(acts[acts.length - 1]).toMatchObject({ action: 'comment', actor_id: 'u9' }); // the comment landed
+ });
+
+ it('control: an org-less caller (trigger/system context shape) reads back fine — no narrowing, no refusal', async () => {
+ const req = await openOrgless();
+ const out = await svc.recall(req.id, { actorId: 'u1' }, ORGLESS_SUBMITTER);
+ expect(out.request).not.toBeNull();
+ expect(out.request.status).toBe('recalled');
+ });
+
+ it('control: an org-MATCHED caller on an org-stamped request reads back fine', async () => {
+ const req: any = await svc.openNodeRequest({
+ object: 'opportunity', recordId: 'opp2', runId: 'run_2', nodeId: 'approve_step',
+ config: { approvers: [{ type: 'user' as const, value: 'u9' }], behavior: 'first_response' as const },
+ record: { id: 'opp2' },
+ }, CTX); // CTX carries tenantId 't1' → the row lands behind the 't1' wall
+ expect(req.organization_id).toBe('t1');
+ const out = await svc.decideNode(req.id, { decision: 'approve', actorId: 'u9' }, ORG_APPROVER);
+ expect(out.request.status).toBe('approved');
+ });
+});
+
 describe('record-lock hook (node era)', () => {
  let engine: ReturnType<typeof makeFakeEngine>;
  let svc: ApprovalService;

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

@@ -2496,7 +2496,7 @@ export class ApprovalService implements IApprovalService {
  }, { context: SYSTEM_CTX });
  await this.syncApproverIndex(requestId, stillPending, org, now);
  const fresh = await this.readBackRequest(requestId, context);
- return { request: fresh!, runId, nodeId, finalized: false, decision: input.decision };
+ return { request: fresh, runId, nodeId, finalized: false, decision: input.decision };
  }
  }
 
@@ -2522,7 +2522,7 @@ export class ApprovalService implements IApprovalService {
  );
  }
  const fresh = await this.readBackRequest(requestId, context);
- return { request: fresh!, runId, nodeId, finalized: true, decision: input.decision, outputs: mergedOutputs };
+ return { request: fresh, runId, nodeId, finalized: true, decision: input.decision, outputs: mergedOutputs };
  }
 
  /**
@@ -2887,7 +2887,7 @@ export class ApprovalService implements IApprovalService {
  }
 
  const fresh = await this.readBackRequest(requestId, context);
- return { request: fresh!, runId, resumed, ...(resumeError ? { resumeError } : {}) };
+ return { request: fresh, runId, resumed, ...(resumeError ? { resumeError } : {}) };
  }
 
  // ── Send back for revision / resubmit (ADR-0044) ─────────────
@@ -2992,7 +2992,7 @@ export class ApprovalService implements IApprovalService {
  });
  }
  const fresh = await this.readBackRequest(requestId, context);
- return { request: fresh!, runId, resumed, autoRejected: true, ...(resumeError ? { resumeError } : {}) };
+ return { request: fresh, runId, resumed, autoRejected: true, ...(resumeError ? { resumeError } : {}) };
  }
 
  await this.engine.update('sys_approval_request', {
@@ -3035,7 +3035,7 @@ export class ApprovalService implements IApprovalService {
  }
 
  const fresh = await this.readBackRequest(requestId, context);
- return { request: fresh!, runId, resumed, ...(resumeError ? { resumeError } : {}) };
+ return { request: fresh, runId, resumed, ...(resumeError ? { resumeError } : {}) };
  }
 
  /**
@@ -3110,7 +3110,7 @@ export class ApprovalService implements IApprovalService {
  }
 
  const fresh = await this.readBackRequest(requestId, context);
- return { request: fresh!, runId, resumed, ...(resumeError ? { resumeError } : {}) };
+ return { request: fresh, runId, resumed, ...(resumeError ? { resumeError } : {}) };
  }
 
  /**
@@ -3283,7 +3283,7 @@ export class ApprovalService implements IApprovalService {
  });
 
  const fresh = await this.readBackRequest(requestId, context);
- return { request: fresh! };
+ return { request: fresh };
  }
 
  /**
@@ -3366,7 +3366,7 @@ export class ApprovalService implements IApprovalService {
  }
 
  const fresh = await this.readBackRequest(requestId, context);
- return { request: fresh!, notified };
+ return { request: fresh, notified };
  }
 
  // ── Actionable links (ADR-0043) ──────────────────────────────
@@ -3512,7 +3512,7 @@ export class ApprovalService implements IApprovalService {
  }
 
  const fresh = await this.readBackRequest(requestId, context);
- return { request: fresh! };
+ return { request: fresh };
  }
 
  /** Free-form reply on the thread (submitter or any pending approver). */
@@ -3554,7 +3554,7 @@ export class ApprovalService implements IApprovalService {
  });
 
  const fresh = await this.readBackRequest(requestId, context);
- return { request: fresh! };
+ return { request: fresh };
  }
 
  // ── SLA escalation (ADR-0042) ─────────────────────────────────
@@ -4757,12 +4757,33 @@ export class ApprovalService implements IApprovalService {
  * flow-driven resume, a service-to-service call), turning a successful write
  * into a `null` result. Gating belongs on the read API, not on an
  * operation's own return value.
+ *
+ * [#12769] The read still narrows by the CALLER's organization
+ * (`loadRequest` — a deliberate tenancy wall, untouched here), and an
+ * org-less request row is invisible inside an org-scoped caller's narrowing.
+ * Org-less rows arise by construction on every schedule / time-relative /
+ * api trigger run (#10131; #9132 pinned that behaviour rather than repairing
+ * it), so this is a live state, not an edge case: the write IS recorded and
+ * the echo cannot be built. It used to escape as `null` behind the callers'
+ * `fresh!` non-null assertions — a well-formed 200 whose declared-non-null
+ * `request` was `null` on the wire. Now it refuses loudly instead, which
+ * keeps every declared result type true: a result that cannot be built is
+ * never returned. Callers must NOT catch this to fall back to `null` —
+ * that would re-open the type-lie one level up.
  */
  private async readBackRequest(
  requestId: string,
  context: ExecutionContext,
- ): Promise<ApprovalRequestRow | null> {
- return this.loadRequest(requestId, context, false);
+ ): Promise<ApprovalRequestRow> {
+ const fresh = await this.loadRequest(requestId, context, false);
+ if (!fresh) {
+ throw new Error(
+ `READ_BACK_FAILED: the write to approval request '${requestId}' was recorded, but the updated row is `
+ + `not visible inside the caller's organization scope, so the result envelope cannot be built. The write `
+ + `is NOT rolled back — read the request back with a system or matching-organization context (#12769).`,
+ );
+ }
+ return fresh;
  }
 
  async getRequest(requestId: string, context: ExecutionContext): Promise<ApprovalRequestRow | null> {