From 7472a06d415520ef0fa6836d55974f5d4fb363a7 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 24 Jul 2026 15:00:40 +0000 Subject: [PATCH 1/3] fix(approvals): admin override to recover an approval routed to an unstaffed position (#3424) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An `approval` node routed to a position/team with no holders resolved to only the unresolvable `position:` literal in `pending_approvers`, so no concrete user was in the slate. Every normal `decide` / `reassign` / `recall` then returned FORBIDDEN and, with `lockRecord`, the target record stayed RECORD_LOCKED forever — a data-availability dead-end with no in-product recovery. Trivial to hit in fresh/demo orgs (positions seeded, holders not) and whenever a role is vacated. Let a platform or tenant admin act on any pending request to release it: approve, reject, reassign it to a real approver, or recall it. The override finalizes the request (which releases the record lock, keyed on a pending request); a tenant admin's authority is org-scoped, a platform admin's is not, and the action is audited under the admin's own id. An admin approval is authoritative — it finalizes the node even under unanimous/quorum/per_group rather than counting as one vote among the (empty) slate. - Add server-computed `sys_approval_request.viewer.can_override`; the approve/reject/reassign declared actions OR it into their `visible` gate so the console surfaces the recovery path with no hand-wired button. - Warn loudly when a node resolves to no concrete approver, so the misconfiguration is visible instead of silently locking the record. The literal-fallback behavior (15.x slot back-compat) is otherwise unchanged. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01VmQPXXbgomoqrXtoxr3CS2 --- .../approval-empty-position-admin-override.md | 34 +++++ .../src/approval-service.test.ts | 117 ++++++++++++++++- .../plugin-approvals/src/approval-service.ts | 122 +++++++++++++++--- .../src/sys-approval-request.object.test.ts | 13 ++ .../src/sys-approval-request.object.ts | 15 ++- .../spec/src/contracts/approval-service.ts | 8 ++ 6 files changed, 286 insertions(+), 23 deletions(-) create mode 100644 .changeset/approval-empty-position-admin-override.md diff --git a/.changeset/approval-empty-position-admin-override.md b/.changeset/approval-empty-position-admin-override.md new file mode 100644 index 0000000000..f2d9e2031e --- /dev/null +++ b/.changeset/approval-empty-position-admin-override.md @@ -0,0 +1,34 @@ +--- +"@objectstack/plugin-approvals": patch +"@objectstack/spec": patch +--- + +fix(approvals): admin override for a request routed to an unstaffed approver (#3424) + +An `approval` node routed to a `position` (or `team`/`department`) with **no +holders** resolved to only the unresolvable `position:` literal in +`pending_approvers` — no concrete user was in the slate. Every normal +`decide` / `reassign` / `recall` then returned `FORBIDDEN` (not a pending +approver) and, with `lockRecord`, the target record stayed `RECORD_LOCKED` +forever: a data-availability dead-end with no in-product recovery (the only exit +was editing the DB by hand). Very easy to hit in fresh/demo orgs (positions +seeded, holders not) and whenever a role is vacated in production. + +A **platform or tenant admin** — the same posture the engine's superuser bypass +already trusts — may now act on any *pending* request to release it: **approve, +reject, reassign** it to a real approver, or **recall** it. The override finalizes +the request (which releases the record lock, keyed on a pending request); a +tenant admin's authority is org-scoped, a platform admin's is not, and the +decision is audited under the admin's own id. An admin approval is authoritative, +finalizing the node even under `unanimous` / `quorum` / `per_group` rather than +counting as one vote among the (empty) slate. + +- `sys_approval_request.viewer` gains `can_override` (server-computed): true for a + privileged admin on a pending request. The `approve` / `reject` / `reassign` + declared actions OR it into their `visible` gate, so the console surfaces the + recovery path without a hand-wired button. Existing approver/submitter gating is + unchanged. +- `openNodeRequest` now logs a loud warning when a node resolves to **no concrete + approver**, so the misconfiguration is visible instead of silently locking the + record. The literal-fallback behavior (kept for 15.x slot back-compat) is + otherwise unchanged. diff --git a/packages/plugins/plugin-approvals/src/approval-service.test.ts b/packages/plugins/plugin-approvals/src/approval-service.test.ts index c789108c15..e5d9849871 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.test.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.test.ts @@ -418,11 +418,11 @@ describe('ApprovalService (node era)', () => { it('getRequest: viewer.can_act is true for a pending approver, false for the submitter', async () => { const req = await svc.openNodeRequest(openInput(['u9']), CTX); // submitter u1, approver u9 const asApprover = await svc.getRequest(req.id, { userId: 'u9', tenantId: 't1' } as any); - expect(asApprover!.viewer).toEqual({ can_act: true, is_submitter: false }); + expect(asApprover!.viewer).toEqual({ can_act: true, is_submitter: false, can_override: false }); const asSubmitter = await svc.getRequest(req.id, { userId: 'u1', tenantId: 't1' } as any); - expect(asSubmitter!.viewer).toEqual({ can_act: false, is_submitter: true }); + expect(asSubmitter!.viewer).toEqual({ can_act: false, is_submitter: true, can_override: false }); const asOther = await svc.getRequest(req.id, { userId: 'u_stranger', tenantId: 't1' } as any); - expect(asOther!.viewer).toEqual({ can_act: false, is_submitter: false }); + expect(asOther!.viewer).toEqual({ can_act: false, is_submitter: false, can_override: false }); }); it('getRequest: viewer.can_act drops to false once the request is finalized', async () => { @@ -1034,6 +1034,117 @@ describe('ApprovalService (node era)', () => { }); }); +// ── Admin / privileged override (#3424) ────────────────────────────── +// +// An approval routed to a position/team with NO holders resolves to only the +// unresolvable `position:` literal — no concrete user is in the slate, so +// every normal decision is FORBIDDEN and (with lockRecord) the record stays +// locked forever with no in-product recovery. A platform or tenant admin may +// act on the pending request to release it: approve, reject, reassign it to a +// real approver, or recall it. Privilege is org-scoped for tenant admins. +describe('ApprovalService — admin override (#3424)', () => { + let engine: ReturnType; + let svc: ApprovalService; + let n = 0; + const baseTime = new Date('2026-01-15T10:00:00Z').getTime(); + + // Admin exec contexts, shaped like the resolved authz envelope (permissions + // carry the permission-set names the shared resolver aggregates). + 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; + + // A request routed to an UNSTAFFED position → `pending_approvers` falls back to + // the `position:sales_manager` literal, undecidable by any normal user. + const stuckInput = (extra: Record = {}) => ({ + object: 'opportunity', recordId: 'opp1', runId: 'run_1', nodeId: 'approve_step', + flowName: 'deal_approval', + config: { approvers: [{ type: 'position' as const, value: 'sales_manager' }], behavior: 'first_response' as const, lockRecord: true }, + record: { id: 'opp1', amount: 100 }, + ...extra, + }); + + beforeEach(() => { + engine = makeFakeEngine(); + n = 0; + svc = new ApprovalService({ engine: engine as any, clock: { now: () => new Date(baseTime + (n++) * 1000) } }); + }); + + it('the stuck request is undecidable by any normal user (repro)', async () => { + const req = await svc.openNodeRequest(stuckInput(), CTX); + expect(req.pending_approvers).toEqual(['position:sales_manager']); + // Even the org owner-by-id is not in the resolved (empty) slate. + await expect(svc.decideNode(req.id, { decision: 'approve', actorId: 'nobody' }, MEMBER)) + .rejects.toThrow(/FORBIDDEN/); + }); + + it('a tenant admin can approve a stuck request, finalizing it (which releases the lock)', async () => { + const req = await svc.openNodeRequest(stuckInput(), CTX); + const out = await svc.decide(req.id, { decision: 'approve', actorId: 'owner' }, TENANT_ADMIN); + expect(out.finalized).toBe(true); + expect(out.request.status).toBe('approved'); + // No pending request remains → the record-lock hook no longer blocks edits. + const fresh = await svc.getRequest(req.id, SYS); + expect(fresh?.status).toBe('approved'); + expect(fresh?.pending_approvers).toEqual([]); + // Audited under the admin's own id — never spoofed as an approver. + const acts = await svc.listActions(req.id, SYS); + expect(acts.at(-1)).toMatchObject({ action: 'approve', actor_id: 'owner' }); + }); + + it('a platform admin can reject a stuck request', async () => { + const req = await svc.openNodeRequest(stuckInput(), CTX); + const out = await svc.decide(req.id, { decision: 'reject', actorId: 'root' }, PLATFORM_ADMIN); + expect(out.finalized).toBe(true); + expect(out.request.status).toBe('rejected'); + }); + + it('an admin override finalizes even a unanimous request immediately (not one vote among the slate)', async () => { + const req = await svc.openNodeRequest(stuckInput({ + config: { approvers: [{ type: 'position' as const, value: 'sales_manager' }], behavior: 'unanimous' as const, lockRecord: true }, + }), CTX); + const out = await svc.decide(req.id, { decision: 'approve', actorId: 'owner' }, TENANT_ADMIN); + expect(out.finalized).toBe(true); + expect(out.request.status).toBe('approved'); + }); + + it('an admin can reassign a stuck request to a real approver, who then decides normally', async () => { + const req = await svc.openNodeRequest(stuckInput(), CTX); + const out = await svc.reassign(req.id, { actorId: 'owner', to: 'u7' }, TENANT_ADMIN); + expect(out.request.pending_approvers).toEqual(['u7']); + const decided = await svc.decideNode( + req.id, { decision: 'approve', actorId: 'u7' }, + { userId: 'u7', tenantId: 't1', positions: [], permissions: [] } as any, + ); + expect(decided.finalized).toBe(true); + }); + + it('an admin can recall (withdraw) a stuck request', async () => { + const req = await svc.openNodeRequest(stuckInput(), CTX); + const out = await svc.recall(req.id, { actorId: 'owner', comment: 'unstaffed role' }, TENANT_ADMIN); + expect(out.request.status).toBe('recalled'); + expect(out.request.pending_approvers).toEqual([]); + }); + + it('a tenant admin of a DIFFERENT org cannot override (privilege is org-scoped)', async () => { + const req = await svc.openNodeRequest(stuckInput(), CTX); // organization_id = t1 + await expect(svc.decideNode(req.id, { decision: 'approve', actorId: 'owner2' }, OTHER_TENANT_ADMIN)) + .rejects.toThrow(/FORBIDDEN/); + }); + + it('viewer.can_override reflects the privilege, and drops once finalized', async () => { + const req = await svc.openNodeRequest(stuckInput(), CTX); + const asAdmin = await svc.getRequest(req.id, TENANT_ADMIN); + expect(asAdmin!.viewer).toMatchObject({ can_act: false, can_override: true }); + const asMember = await svc.getRequest(req.id, MEMBER); + expect(asMember!.viewer!.can_override).toBe(false); + await svc.decide(req.id, { decision: 'approve', actorId: 'owner' }, TENANT_ADMIN); + const after = await svc.getRequest(req.id, TENANT_ADMIN); + expect(after!.viewer!.can_override).toBe(false); + }); +}); + describe('record-lock hook (node era)', () => { let engine: ReturnType; let svc: ApprovalService; diff --git a/packages/plugins/plugin-approvals/src/approval-service.ts b/packages/plugins/plugin-approvals/src/approval-service.ts index d52a449fdb..4c3aa30668 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.ts @@ -6,6 +6,13 @@ import { canonicalApproverType, type ApprovalNodeConfig, } from '@objectstack/spec/automation'; +import { + ADMIN_FULL_ACCESS, + ORGANIZATION_ADMIN, + BUILTIN_IDENTITY_PLATFORM_ADMIN, + BUILTIN_IDENTITY_ORG_OWNER, + BUILTIN_IDENTITY_ORG_ADMIN, +} from '@objectstack/spec/identity'; import type { IApprovalService, ApprovalRequestRow, @@ -307,6 +314,45 @@ export class ApprovalService implements IApprovalService { return raw; } + /** + * Privileged-override gate (#3424). A stuck approval — one routed to a + * position/team with no holders (so its `pending_approvers` is only an + * unresolvable `type:value` literal) or to approvers who have all since left — + * is otherwise undecidable: no concrete user is in the slate, so every normal + * `decide` / `reassign` / `recall` is `FORBIDDEN` and (with `lockRecord`) the + * record stays locked forever with no in-product recovery. A platform or + * tenant admin — the same posture the engine's superuser bypass already + * trusts — may always act on a PENDING request to release it: approve, reject, + * reassign it to a real approver, or recall it. + * + * A platform admin crosses the tenant wall (matching the unscoped + * `admin_full_access` evidence); a tenant admin may override only within their + * own org (or an org-less request). A system context always passes. Signals are + * read defensively off the resolved exec context (`permissions` / `positions` / + * the derived `posture`, ADR-0095) so any transport that resolves through the + * shared authz resolver lights this up without extra wiring. + */ + private isOverrideActor(context: SharingExecutionContext, requestOrg?: string | null): boolean { + if (!context) return false; + if (context.isSystem) return true; + const perms = Array.isArray(context.permissions) ? context.permissions : []; + const positions = Array.isArray(context.positions) ? context.positions : []; + const posture = (context as any).posture; + const isPlatformAdmin = posture === 'PLATFORM_ADMIN' + || perms.includes(ADMIN_FULL_ACCESS) + || positions.includes(BUILTIN_IDENTITY_PLATFORM_ADMIN); + if (isPlatformAdmin) return true; + const isTenantAdmin = posture === 'TENANT_ADMIN' + || perms.includes(ORGANIZATION_ADMIN) + || positions.includes(BUILTIN_IDENTITY_ORG_OWNER) + || positions.includes(BUILTIN_IDENTITY_ORG_ADMIN); + 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. + const actorTenant = (context as any).tenantId ?? (context as any).organizationId ?? null; + return requestOrg == null || (actorTenant != null && String(requestOrg) === String(actorTenant)); + } + /** * Expand the approvers on an Approval node into user IDs by querying the * graph tables for `team:` / `department:` / `position:` / @@ -673,6 +719,20 @@ export class ApprovalService implements IApprovalService { { approvers: input.config.approvers }, input.record, ctxOrg, { now: nowDate.getTime(), substitutions, groups }, ); + // #3424: an approval routed to a target with no holders (e.g. an unstaffed + // `position`) resolves to only unresolvable `type:value` literals — no + // concrete user can act. The request is still opened (a privileged admin can + // override it, and legacy 15.x literal slots stay queryable), but warn + // loudly so the misconfiguration surfaces instead of silently locking the + // record with no obvious cause. + if (!approvers.some(a => a && !a.includes(':'))) { + 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 now = nowDate.toISOString(); const id = uid('areq'); const processName = `flow:${input.flowName ?? input.nodeId}`; @@ -832,7 +892,12 @@ export class ApprovalService implements IApprovalService { if (raw.status !== 'pending') throw new Error(`INVALID_STATE: request is ${raw.status}`); const pendingApprovers = csvSplit(raw.pending_approvers); - if (!context.isSystem && !pendingApprovers.includes(input.actorId)) { + // A privileged admin may override a stuck request (#3424) even when they + // hold no slot — the escape hatch for an approval routed to an unstaffed + // position or to approvers who have all left. + const isOverride = this.isOverrideActor(context, raw.organization_id ?? null); + const isSlotHolder = pendingApprovers.includes(input.actorId); + if (!isSlotHolder && !isOverride) { throw new Error(`FORBIDDEN: actor '${input.actorId}' is not a pending approver`); } @@ -855,7 +920,12 @@ export class ApprovalService implements IApprovalService { // finalizes the node (one veto), so only the approve path can hold it open. // `first_response` finalizes on the first approval (falls straight through). const behavior = config.behavior ?? 'first_response'; - if (input.decision === 'approve' && behavior !== 'first_response') { + // A privileged override (an admin rescuing a stuck request, #3424) is an + // authoritative decision, not one vote among the resolved slate — it + // finalizes the node immediately, regardless of `unanimous`/`quorum`/ + // `per_group`. Only a real slot holder's approval feeds the multi-approver + // tally below. + if (input.decision === 'approve' && behavior !== 'first_response' && isSlotHolder) { const acts = await this.engine.find('sys_approval_action', { where: { request_id: requestId, step_index: 0, action: 'approve' }, limit: 1000, context: SYSTEM_CTX, }); @@ -969,7 +1039,10 @@ export class ApprovalService implements IApprovalService { if (raw.status !== 'pending' && !inReviseWindow) { throw new Error(`INVALID_STATE: request is ${raw.status}`); } - if (!context.isSystem && raw.submitter_id && String(raw.submitter_id) !== String(input.actorId)) { + // The submitter withdraws their own request; a privileged admin may recall + // any pending request to release a stuck record (#3424). + if (!this.isOverrideActor(context, raw.organization_id ?? null) + && raw.submitter_id && String(raw.submitter_id) !== String(input.actorId)) { throw new Error(`FORBIDDEN: only the submitter may recall this request`); } // A returned request is only recallable while it is still the run's live @@ -1288,7 +1361,10 @@ export class ApprovalService implements IApprovalService { /** * Hand a pending-approver slot to someone else. `from` defaults to the * actor itself; the actor must hold the slot being handed over (or be a - * system caller). Audits `reassign` and notifies the new approver. + * system caller). A privileged admin (#3424) may reassign a request whose + * slate holds no real user — an unstaffed-position literal — by handing the + * whole request to a real approver, rescuing it from the locked dead-end. + * Audits `reassign` and notifies the new approver. */ async reassign( requestId: string, @@ -1301,18 +1377,27 @@ export class ApprovalService implements IApprovalService { const raw = await this.loadPendingRow(requestId); const pending = csvSplit(raw.pending_approvers); - const from = String(input.from ?? input.actorId).trim(); - if (!pending.includes(from)) { - throw new Error(`FORBIDDEN: '${from}' is not a pending approver on this request`); - } - if (!context.isSystem && input.actorId !== from && !pending.includes(input.actorId)) { - throw new Error(`FORBIDDEN: actor '${input.actorId}' is not a pending approver`); - } if (pending.includes(to)) { throw new Error(`VALIDATION_FAILED: '${to}' is already a pending approver`); } - - const next = pending.map(a => (a === from ? to : a)); + const isOverride = this.isOverrideActor(context, raw.organization_id ?? null); + const from = String(input.from ?? input.actorId).trim(); + let next: string[]; + if (pending.includes(from)) { + // Normal hand-off: the actor holds the slot being moved (or is a + // system/admin caller acting on a real holder's slot). + if (!context.isSystem && !isOverride && input.actorId !== from && !pending.includes(input.actorId)) { + throw new Error(`FORBIDDEN: actor '${input.actorId}' is not a pending approver`); + } + next = pending.map(a => (a === from ? to : a)); + } else if (isOverride) { + // Admin rescue (#3424): the caller holds no slot — the slate is an + // unstaffed-position literal or a set of departed approvers. Reassign the + // whole request to a real approver so the normal decision flow can resume. + next = [to]; + } else { + throw new Error(`FORBIDDEN: '${from}' is not a pending approver on this request`); + } const now = this.clock.now().toISOString(); // Audit first, then mutate — mirrors decideNode(), so a failed audit // write can never leave a moved slot without a trail. @@ -2278,8 +2363,13 @@ export class ApprovalService implements IApprovalService { * caller's user id is in the resolved `pending_approvers` while the request is * still `pending` (position/team/manager approvers are already resolved to * concrete user ids at open time, so a plain membership test is faithful). - * `is_submitter` is a straight owner check. System/tokenless contexts get a - * both-false block. Cheap + synchronous — safe on list reads. + * `is_submitter` is a straight owner check. `can_override` (#3424) is true for + * a platform/tenant admin on a PENDING request — the recovery path for an + * approval routed to an unstaffed position or to approvers who have all left; + * clients OR it into the decision actions' `visible` gate so an admin can act + * even when they hold no slot. System/tokenless contexts get a both-false + * `can_act`/`is_submitter` block (system gets `can_override` too — it may act + * on anything). Cheap + synchronous — safe on list reads. */ private attachViewers(rows: ApprovalRequestRow[], context: SharingExecutionContext): void { const uid = (context as any)?.userId != null ? String((context as any).userId) : null; @@ -2288,6 +2378,8 @@ export class ApprovalService implements IApprovalService { (row as any).viewer = { can_act: row.status === 'pending' && !!uid && pending.includes(uid), is_submitter: !!uid && row.submitter_id != null && String(row.submitter_id) === uid, + can_override: row.status === 'pending' + && this.isOverrideActor(context, (row as any).organization_id ?? null), }; } } 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 5c7699e389..0b3c6f0d86 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 @@ -74,6 +74,19 @@ 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']) { + expect(vis(name)).toContain('record.viewer.can_override'); + } + for (const name of ['approval_send_back', 'approval_request_info']) { + expect(vis(name)).not.toContain('can_override'); + } + }); + it('recall stays available while a returned request is still the submitter\'s to abandon', () => { expect(vis('approval_recall')).toContain('record.status == "returned"'); expect(byName('approval_recall').confirmText).toBeTruthy(); 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 bd0c1bbb83..4f810813fe 100644 --- a/packages/plugins/plugin-approvals/src/sys-approval-request.object.ts +++ b/packages/plugins/plugin-approvals/src/sys-approval-request.object.ts @@ -240,8 +240,13 @@ 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`. `viewer` is attached by - // getRequest/listRequests; where it is absent the predicate fails closed. + // 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 + // unstaffed position — otherwise undecidable, locking the record forever — by + // approving, rejecting, or reassigning it to a real approver. `viewer` is + // attached by getRequest/listRequests; where it is absent the predicate fails + // closed. actions: [ { name: 'approval_approve', @@ -261,7 +266,7 @@ export const SysApprovalRequest = ObjectSchema.create({ // string[]`; the decision route persists them on `sys_approval_action`. { name: 'attachments', label: 'Attachments', type: 'file', multiple: true, required: false }, ], - visible: 'record.viewer.can_act', + visible: 'record.viewer.can_act || record.viewer.can_override', locations: ['record_section', 'list_item'], successMessage: 'Approved.', refreshAfter: true, @@ -280,7 +285,7 @@ export const SysApprovalRequest = ObjectSchema.create({ { name: 'comment', label: 'Comment', type: 'textarea', required: false }, { name: 'attachments', label: 'Attachments', type: 'file', multiple: true, required: false }, ], - visible: 'record.viewer.can_act', + visible: 'record.viewer.can_act || record.viewer.can_override', confirmText: 'Reject this request? A rejection is final for every approver.', locations: ['record_section', 'list_item'], successMessage: 'Rejected.', @@ -302,7 +307,7 @@ export const SysApprovalRequest = ObjectSchema.create({ { field: 'submitter_id', name: 'to', label: 'New approver', required: true, helpText: 'User to hand this step to' }, { name: 'comment', label: 'Comment', type: 'textarea', required: false }, ], - visible: 'record.viewer.can_act', + visible: 'record.viewer.can_act || record.viewer.can_override', locations: ['record_section'], successMessage: 'Reassigned.', refreshAfter: true, diff --git a/packages/spec/src/contracts/approval-service.ts b/packages/spec/src/contracts/approval-service.ts index f5e57047b2..fece0607ab 100644 --- a/packages/spec/src/contracts/approval-service.ts +++ b/packages/spec/src/contracts/approval-service.ts @@ -126,6 +126,13 @@ export interface ApprovalRequestRow { * it is strictly more accurate than a client-side identity guess (it already * reflects position/team/manager resolution baked into `pending_approvers`). * - `is_submitter` — the caller submitted the request. + * - `can_override` (#3424) — the caller is a platform/tenant admin who may act + * on a *pending* request (approve / reject / reassign / recall it) despite + * holding no approver slot. The in-product recovery path for an approval + * routed to an unstaffed position, or whose approvers have all since left, + * which would otherwise leave the request undecidable and the record locked + * forever. Clients OR it into the decision actions' `visible` gate; the + * service re-checks the same privilege before applying any override. * * Absent when the row is surfaced outside a service read with a user context * (e.g. a raw data-API grid); a `record.viewer.*` predicate then fails closed. @@ -133,6 +140,7 @@ export interface ApprovalRequestRow { viewer?: { can_act: boolean; is_submitter: boolean; + can_override: boolean; }; } From 26dee90d1176d34597963905a40803ff730329ea Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 24 Jul 2026 15:04:22 +0000 Subject: [PATCH 2/3] docs(approvals): document the admin override + viewer.can_override (#3424) Note the platform/tenant admin recovery path for a request routed to an unstaffed position (approve / reject / reassign / recall), and add `can_override` to the per-viewer block description. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01VmQPXXbgomoqrXtoxr3CS2 --- content/docs/automation/approvals.mdx | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/content/docs/automation/approvals.mdx b/content/docs/automation/approvals.mdx index 9133560ae6..3d222ac634 100644 --- a/content/docs/automation/approvals.mdx +++ b/content/docs/automation/approvals.mdx @@ -281,7 +281,7 @@ service attaches to every request it returns: ```jsonc // getRequest / listRequests responses carry, per the calling user: -"viewer": { "can_act": true, "is_submitter": false } +"viewer": { "can_act": true, "is_submitter": false, "can_override": false } ``` - `can_act` — the caller is a **current pending approver** (their id is in the @@ -289,12 +289,30 @@ service attaches to every request it returns: check the decision routes authorize with, so it already reflects position/team/manager resolution. - `is_submitter` — the caller submitted the request. +- `can_override` — the caller is a **platform or tenant admin** who may act on a + `pending` request despite holding no slot (see the admin-override callout + below). Approver actions gate on `record.viewer.can_act`, submitter levers -(remind/recall/resubmit) on `record.viewer.is_submitter`. So a submitter viewing +(remind/recall/resubmit) on `record.viewer.is_submitter`, and Approve/Reject/ +Reassign additionally OR in `record.viewer.can_override`. So a submitter viewing their own pending request never sees Approve/Reject/Reassign (buttons the server -would 403 anyway), and a position-addressed approver is never wrongly hidden. -The service stays the sole authority — the predicate only trims the UI. +would 403 anyway), a position-addressed approver is never wrongly hidden, and an +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 an +unresolvable `position:` literal in `pending_approvers`: no concrete user +can act, and (with `lockRecord`) the record stays locked. A **platform admin** +(`admin_full_access`) or **tenant admin** (`organization_admin`, org-scoped) may +act on any `pending` request — **approve, reject, reassign** it to a real +approver, or **recall** it — releasing the lock. An admin decision is +authoritative: it finalizes the node even under `unanimous`/`quorum`/`per_group`, +and is audited under the admin's own id. Prefer a guaranteed-staffed fallback +approver so the set is never empty in the first place. + ### Progress and notification deep links From e874ea28cc2bcee7d9c9ad215a4eaa83c995ec62 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 24 Jul 2026 15:18:51 +0000 Subject: [PATCH 3/3] fix(approvals): defense-in-depth for empty-approver routing (#3424) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two authoring-time / demo-time complements to the admin-override recovery: - lint: new advisory rule `approval-approvers-may-resolve-empty` (info) fires when EVERY approver on an approval node routes to a group that can be empty (position/team/department) — the exact shape that dead-ends when nobody holds the group. It prescribes a guaranteed-staffed fallback (`{ type: 'org_membership_level', value: 'owner' }`). Advisory, since staffing is runtime data a linter can't see; individual/owner-tier fallbacks suppress it. Non-gating (info → suggestion). - showcase: the seed staffed `manager`/`finance`/`legal` but not `exec`, the second tier of `showcase_budget_approval` (manager → exec) — a user driving a budget approval to step 2 hit the exact #3424 dead-end. Staff `exec` too so every position the showcase flows route to is actionable. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01VmQPXXbgomoqrXtoxr3CS2 --- .../src/security/seed-approval-demo.ts | 12 ++++- packages/lint/src/index.ts | 1 + .../src/validate-approval-approvers.test.ts | 51 +++++++++++++++++++ .../lint/src/validate-approval-approvers.ts | 46 +++++++++++++++++ 4 files changed, 108 insertions(+), 2 deletions(-) diff --git a/examples/app-showcase/src/security/seed-approval-demo.ts b/examples/app-showcase/src/security/seed-approval-demo.ts index eaa4a66e9a..795fd7e946 100644 --- a/examples/app-showcase/src/security/seed-approval-demo.ts +++ b/examples/app-showcase/src/security/seed-approval-demo.ts @@ -41,8 +41,16 @@ const SYS = { isSystem: true } as const; const ADMIN_EMAIL = 'admin@objectos.ai'; -/** Positions the admin is granted so they resolve as an approver on the demos. */ -const ADMIN_APPROVAL_POSITIONS = ['manager', 'finance', 'legal'] as const; +/** + * Positions the admin is granted so they resolve as an approver on the demos. + * Covers every `{ type: 'position' }` the showcase approval flows route to and + * expect the admin to hold — including `exec`, the SECOND tier of + * `showcase_budget_approval` (manager → exec). Without `exec` a budget approval + * that a user drives to step 2 routes to an unstaffed position and dead-ends + * (framework#3424): undecidable request, record locked. Keep this in sync with + * the position values authored in `automation/flows`. + */ +const ADMIN_APPROVAL_POSITIONS = ['manager', 'finance', 'legal', 'exec'] as const; /** A phone-based demo persona (§6 "phone sign-in surfaces"). */ const PHONE_DEMO_USER = { diff --git a/packages/lint/src/index.ts b/packages/lint/src/index.ts index af580d86ce..6a69138541 100644 --- a/packages/lint/src/index.ts +++ b/packages/lint/src/index.ts @@ -106,6 +106,7 @@ export { APPROVAL_APPROVER_TYPE_DEPRECATED, APPROVAL_APPROVER_TYPE_UNKNOWN, APPROVAL_ESCALATION_REASSIGN_NO_TARGET, + APPROVAL_APPROVERS_MAY_RESOLVE_EMPTY, } from './validate-approval-approvers.js'; export type { ApprovalApproverFinding, ApprovalApproverSeverity } from './validate-approval-approvers.js'; diff --git a/packages/lint/src/validate-approval-approvers.test.ts b/packages/lint/src/validate-approval-approvers.test.ts index 5956f7f298..2a18778751 100644 --- a/packages/lint/src/validate-approval-approvers.test.ts +++ b/packages/lint/src/validate-approval-approvers.test.ts @@ -7,6 +7,7 @@ import { APPROVAL_APPROVER_TYPE_DEPRECATED, APPROVAL_APPROVER_TYPE_UNKNOWN, APPROVAL_ESCALATION_REASSIGN_NO_TARGET, + APPROVAL_APPROVERS_MAY_RESOLVE_EMPTY, } from './validate-approval-approvers.js'; function stackWithApprovers(approvers: unknown[]): Record { @@ -120,6 +121,56 @@ describe('validateApprovalApprovers', () => { expect(validateApprovalApprovers(stack)).toEqual([]); }); + // ── empty-slate dead-end (#3424) ───────────────────────────────────────── + + it('flags a node routed only to a single group (unstaffed-position dead-end)', () => { + const findings = validateApprovalApprovers(stackWithApprovers([ + { type: 'position', value: 'exec' }, + ])); + expect(findings).toHaveLength(1); + expect(findings[0].rule).toBe(APPROVAL_APPROVERS_MAY_RESOLVE_EMPTY); + expect(findings[0].severity).toBe('info'); + expect(findings[0].path).toBe('flows[0].nodes[1].config.approvers'); + expect(findings[0].message).toContain('locked'); // lockRecord defaults true + expect(findings[0].hint).toContain("org_membership_level', value: 'owner'"); + }); + + it('flags a node routed only to groups (all position/team/department)', () => { + const findings = validateApprovalApprovers(stackWithApprovers([ + { type: 'position', value: 'finance' }, + { type: 'team', value: 't1' }, + { type: 'department', value: 'bu_sales' }, + ])); + expect(findings.filter(f => f.rule === APPROVAL_APPROVERS_MAY_RESOLVE_EMPTY)).toHaveLength(1); + }); + + it('does NOT flag when a guaranteed-staffed or individual fallback is present', () => { + // position + owner tier — the prescribed fallback. + expect(validateApprovalApprovers(stackWithApprovers([ + { type: 'position', value: 'exec' }, + { type: 'org_membership_level', value: 'owner' }, + ]))).toEqual([]); + // position + a specific user. + expect(validateApprovalApprovers(stackWithApprovers([ + { type: 'position', value: 'exec' }, + { type: 'user', value: 'u1' }, + ]))).toEqual([]); + // position + the submitter's manager. + expect(validateApprovalApprovers(stackWithApprovers([ + { type: 'position', value: 'exec' }, + { type: 'manager' }, + ]))).toEqual([]); + }); + + it('drops the record-lock clause when lockRecord is false', () => { + const stack = stackWithApprovers([{ type: 'position', value: 'exec' }]); + (stack.flows as any)[0].nodes[1].config.lockRecord = false; + const findings = validateApprovalApprovers(stack); + expect(findings).toHaveLength(1); + expect(findings[0].rule).toBe(APPROVAL_APPROVERS_MAY_RESOLVE_EMPTY); + expect(findings[0].message).not.toContain('locked'); + }); + it('only scans approval nodes and tolerates malformed shapes', () => { const findings = validateApprovalApprovers({ flows: [{ diff --git a/packages/lint/src/validate-approval-approvers.ts b/packages/lint/src/validate-approval-approvers.ts index 80717dcebe..3a39a2b8f3 100644 --- a/packages/lint/src/validate-approval-approvers.ts +++ b/packages/lint/src/validate-approval-approvers.ts @@ -20,6 +20,7 @@ * | approval-approver-type-deprecated | warning | ADR-0090 D3 (#3133) | * | approval-approver-type-unknown | warning | contract-first (PD #12) | * | approval-escalation-reassign-no-target | warning | silent notify degradation | + * | approval-approvers-may-resolve-empty | info | empty-position dead-end (#3424) | * * The first two are mutually exclusive by construction — a bad *value* wins, * because its fix (`position`) differs from the deprecation's fix @@ -44,6 +45,19 @@ export const APPROVAL_APPROVER_NOT_MEMBERSHIP_TIER = 'approval-approver-not-memb export const APPROVAL_APPROVER_TYPE_DEPRECATED = 'approval-approver-type-deprecated'; export const APPROVAL_APPROVER_TYPE_UNKNOWN = 'approval-approver-type-unknown'; export const APPROVAL_ESCALATION_REASSIGN_NO_TARGET = 'approval-escalation-reassign-no-target'; +export const APPROVAL_APPROVERS_MAY_RESOLVE_EMPTY = 'approval-approvers-may-resolve-empty'; + +/** + * Approver types that route to a GROUP whose membership is runtime data and can + * be empty (an unstaffed position, an empty team/department). When EVERY + * approver on a node is one of these, the node can resolve to an empty slate at + * runtime — the framework#3424 dead-end. Individually-routed types + * (`user`/`field`/`manager`), the guaranteed-staffed `org_membership_level` + * tiers, and the opaque `queue` are deliberately excluded: any of them present + * signals the author has a non-group route, so the node isn't purely + * group-gated. + */ +const GROUP_ROUTED_TYPES = new Set(['position', 'team', 'department']); export type ApprovalApproverSeverity = 'error' | 'warning' | 'info'; @@ -171,6 +185,38 @@ 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. + // 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( + (a) => a && typeof a === 'object' && typeof (a as AnyRec).type === 'string', + ); + if ( + routable.length > 0 && + routable.every((a) => GROUP_ROUTED_TYPES.has(canonicalApproverType(String((a as AnyRec).type)))) + ) { + const locks = (cfg as AnyRec).lockRecord !== false; // default true + findings.push({ + severity: 'info', + rule: APPROVAL_APPROVERS_MAY_RESOLVE_EMPTY, + where, + path: `flows[${fi}].nodes[${ni}].config.approvers`, + message: + `every approver on this node routes to a group (position/team/department) whose ` + + `members are runtime data — if none is staffed, the request resolves to an empty ` + + `slate and waits forever` + + (locks ? `, and (lockRecord) the record stays locked with no in-product recovery.` : `.`), + hint: + `Make sure at least one target is always staffed, or add a guaranteed-staffed ` + + `fallback approver, e.g. { type: 'org_membership_level', value: 'owner' }. A request ` + + `that still lands empty is recoverable only by a platform/tenant admin override (#3424).`, + }); + } + // escalation.action 'reassign' with no escalateTo silently degrades to a // plain SLA-breach notification at runtime — the hand-off the author // asked for never happens.