diff --git a/.changeset/required-decision-outputs.md b/.changeset/required-decision-outputs.md new file mode 100644 index 0000000000..b3af853037 --- /dev/null +++ b/.changeset/required-decision-outputs.md @@ -0,0 +1,14 @@ +--- +"@objectstack/spec": minor +"@objectstack/plugin-approvals": minor +--- + +`decisionOutputs` entries may now be declared `required` (objectui#2955). A typed entry `{ key, label?, type?, multiple?, required?: true }` tells the runtime — not just the decision UI — that an approver must supply the value: an **approve** carrying no value, or a blank one (`''`, whitespace, `[]`, an array of blanks), is rejected with `VALIDATION_FAILED` before any write, so the audit row and the request are untouched and the run can never resume past the node with the key missing. + +That gap is what the flag closes. `decisionOutputs` exists so a decision can route the next step (`approvers: [{ type: 'expression', value: 'vars.lead_review.next_reviewers' }]`), but nothing made the approver actually answer: a skipped output resumed the run with the key absent, and the next node either faulted with `EXPRESSION_FAILED` or resolved an empty slate and stalled on `onEmptyApprovers: 'admin_rescue'` — long after the one person who could have filled it in had moved on. `onEmptyApprovers` was the only backstop, and it is a recovery mechanism, not a contract. + +**Reject never requires them.** The run leaves down the `reject` edge, where nothing reads the outputs — demanding routing data to say "no" would trap the rejection. Outputs still ride a reject when the approver filled them in. + +**No elevation bypass.** A one-click email action link and an `auto_approve` SLA escalation both fail the same way rather than advancing into a node that would resolve nobody; the escalation sweep already isolates a throwing request, so that decision stays pending and visibly overdue instead of silently breaking the run downstream. Enforcement is per decision, so on a `unanimous` / `quorum` node every approver supplies the required outputs and the finalizing decision's values are what the flow resumes with. + +`required` rides `normalizeDecisionOutputs`, so it reaches clients on `decision_output_defs` — a decision UI marks the field required and blocks locally instead of round-tripping to a 400. The console side ships in objectui#2955. diff --git a/content/docs/automation/approvals.mdx b/content/docs/automation/approvals.mdx index ce28d3bbbb..177ac92a8a 100644 --- a/content/docs/automation/approvals.mdx +++ b/content/docs/automation/approvals.mdx @@ -259,6 +259,19 @@ matching system-object picker; picker values are record ids, `multiple` collects an id array). The type shapes only the input widget; the whitelist still works by `key`, so bare strings and typed declarations mix freely. +Add **`required: true`** when the next step cannot run without it — the very +case above, where node B has nobody to route to if the lead skipped the field. +Unlike `type` / `multiple`, this one is enforced by the runtime: an **approve** +carrying no value (or a blank one — `''`, `[]`) for a required key is rejected +before any write, so the run can never resume past the node with the key +missing. A **reject** never requires it: the run leaves down the `reject` edge, +where nothing reads the outputs. Enforcement has no elevation bypass — a +one-click email action link and an `auto_approve` SLA escalation both fail +rather than advance into a node that would resolve nobody, leaving the request +pending and visibly overdue. Without the flag your only backstop is +`onEmptyApprovers` — the next node opens, resolves nobody, and stalls for an +admin rescue long after the approver who could have filled it in has gone. + ## The full lifecycle — submit to field change What actually happens between "the flow hits the approval node" and "the record diff --git a/content/docs/references/automation/approval.mdx b/content/docs/references/automation/approval.mdx index cda235a42d..55318a525e 100644 --- a/content/docs/references/automation/approval.mdx +++ b/content/docs/references/automation/approval.mdx @@ -81,7 +81,7 @@ const result = ApprovalDecision.parse(data); | **lockRecord** | `boolean` | ✅ | Lock the record from editing while pending | | **approvalStatusField** | `string` | optional | Business-object field to mirror request status onto | | **onEmptyApprovers** | `Enum<'admin_rescue' \| 'fail' \| 'auto_approve'>` | ✅ | Behavior when no concrete approver resolves at node entry | -| **decisionOutputs** | `string \| { key: string; label?: string; type?: Enum<'text' \| 'user' \| 'department' \| 'position' \| 'team'>; multiple?: boolean }[]` | optional | Author-declared decision outputs — bare keys or typed `{ key, type, multiple }` declarations | +| **decisionOutputs** | `string \| { key: string; label?: string; type?: Enum<'text' \| 'user' \| 'department' \| 'position' \| 'team'>; multiple?: boolean; … }[]` | optional | Author-declared decision outputs — bare keys or typed `{ key, type, multiple }` declarations | | **escalation** | `{ enabled: boolean; timeoutHours: number; action: Enum<'reassign' \| 'auto_approve' \| 'auto_reject' \| 'notify'>; escalateTo?: string; … }` | optional | Per-node SLA escalation | | **maxRevisions** | `integer` | ✅ | Max send-backs for revision before auto-reject (0 = send-back disabled) | @@ -116,6 +116,7 @@ const result = ApprovalDecision.parse(data); | **label** | `string` | optional | Field label in the decision dialog | | **type** | `Enum<'text' \| 'user' \| 'department' \| 'position' \| 'team'>` | optional | Decision-dialog input widget (default 'text') | | **multiple** | `boolean` | optional | Collect multiple values (id array) | +| **required** | `boolean` | optional | Approver must supply this output to approve | --- diff --git a/examples/app-showcase/src/automation/flows/dynamic-approval.flow.ts b/examples/app-showcase/src/automation/flows/dynamic-approval.flow.ts index 21419ddc53..80072924a1 100644 --- a/examples/app-showcase/src/automation/flows/dynamic-approval.flow.ts +++ b/examples/app-showcase/src/automation/flows/dynamic-approval.flow.ts @@ -41,8 +41,12 @@ export const DynamicApprovalFlow = defineFlow({ lockRecord: true, // The lead hands the co-signers to the flow with their decision. The // TYPED declaration renders a multi-select sys_user picker in the - // decision dialog (bare string keys render free text). - decisionOutputs: [{ key: 'next_reviewers', label: 'Next Reviewers', type: 'user', multiple: true }], + // decision dialog (bare string keys render free text), and `required` + // is enforced on approve — stage 2 declares `onEmptyApprovers: 'fail'`, + // so a lead who skipped the field would kill the run. + decisionOutputs: [{ + key: 'next_reviewers', label: 'Next Reviewers', type: 'user', multiple: true, required: true, + }], }, }, { diff --git a/packages/plugins/plugin-approvals/src/approval-service.test.ts b/packages/plugins/plugin-approvals/src/approval-service.test.ts index 32beb943e5..8fc57809f2 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.test.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.test.ts @@ -554,6 +554,115 @@ describe('ApprovalService (node era)', () => { expect(second.outputs).toEqual({ legal_note: 'ok', finance_note: 'ok too' }); }); + // ── required decision outputs (objectui#2955) ─────────────────── + // + // `type`/`multiple` only shape the input widget; `required` is the one + // declaration the RUNTIME enforces. Without it the author's only backstop + // was `onEmptyApprovers`: the approve went through with the key missing, the + // next node's `expression` approver resolved nobody, and the run stalled for + // an admin rescue — long after the approver who could have supplied it left. + + const requiredInput = (extra: Record = {}) => openInput(['u9'], {}, { + decisionOutputs: [ + { key: 'parallel_positions', label: 'Co-signing positions', type: 'position', multiple: true, required: true }, + { key: 'note' }, + ], + ...extra, + }); + + it('required outputs: surfaced on the request row so the UI can block before the round trip', async () => { + const req = await svc.openNodeRequest(requiredInput(), CTX) as any; + expect(req.decision_output_defs).toEqual([ + { key: 'parallel_positions', label: 'Co-signing positions', type: 'position', multiple: true, required: true }, + { key: 'note' }, + ]); + }); + + it('required outputs: an approve that omits one is rejected before any write', async () => { + const req = await svc.openNodeRequest(requiredInput(), CTX); + await expect(svc.decideNode(req.id, { + decision: 'approve', actorId: 'u9', outputs: { note: 'looks fine' }, + }, SYS)).rejects.toThrow(/VALIDATION_FAILED.*parallel_positions.*required to approve/s); + // Atomic: the decision left no audit row (only the open-time submit), and + // the request is untouched. + expect((engine._tables['sys_approval_action'] ?? []).map((a: any) => a.action)).toEqual(['submit']); + expect(engine._tables['sys_approval_request'][0].status).toBe('pending'); + }); + + it('required outputs: an approve carrying no outputs at all is rejected too', async () => { + // The regression that matters — a decision surface that collects nothing + // (objectui#2955: the record header) used to sail straight through here. + const req = await svc.openNodeRequest(requiredInput(), CTX); + await expect(svc.decideNode(req.id, { + decision: 'approve', actorId: 'u9', + }, SYS)).rejects.toThrow(/VALIDATION_FAILED.*parallel_positions.*required to approve/s); + }); + + it('required outputs: present-but-blank does not satisfy the requirement', async () => { + // An untouched widget submits '' or [] — accepting those would hand the + // downstream expression approver an empty slate, i.e. the exact stall. + for (const blank of ['', ' ', [], [''], null]) { + engine._tables['sys_approval_request'] = []; + engine._tables['sys_approval_action'] = []; + const req = await svc.openNodeRequest(requiredInput(), CTX); + await expect(svc.decideNode(req.id, { + decision: 'approve', actorId: 'u9', outputs: { parallel_positions: blank }, + }, SYS)).rejects.toThrow(/VALIDATION_FAILED.*required to approve/s); + } + }); + + it('required outputs: a filled approve goes through and hands the value to the flow', async () => { + const req = await svc.openNodeRequest(requiredInput(), CTX); + const out = await svc.decideNode(req.id, { + decision: 'approve', actorId: 'u9', outputs: { parallel_positions: ['pos_1', 'pos_2'] }, + }, SYS); + expect(out.finalized).toBe(true); + expect(out.outputs).toEqual({ parallel_positions: ['pos_1', 'pos_2'] }); + }); + + it('required outputs: a REJECT never needs them', async () => { + // The run leaves down the reject edge, where nothing reads the outputs — + // demanding routing data to say "no" would trap the rejection. + const req = await svc.openNodeRequest(requiredInput(), CTX); + const out = await svc.decideNode(req.id, { decision: 'reject', actorId: 'u9' }, SYS); + expect(out.finalized).toBe(true); + expect(out.decision).toBe('reject'); + }); + + it('required outputs: an unrelated node is unaffected', async () => { + // No `required` anywhere → the pre-#2955 behaviour, byte for byte. + const req = await svc.openNodeRequest( + openInput(['u9'], {}, { decisionOutputs: ['note'] }), CTX, + ); + const out = await svc.decideNode(req.id, { decision: 'approve', actorId: 'u9' }, SYS); + expect(out.finalized).toBe(true); + }); + + it('required outputs: every approver of a multi-approver node must supply them', async () => { + // Enforced per decision, not per node: each approve is a decision that + // could be the one that resumes the run, and each approver sees the field. + const req = await svc.openNodeRequest( + openInput(['u1', 'u2'], {}, { + behavior: 'unanimous', + decisionOutputs: [{ key: 'co_signer', type: 'user', required: true }], + }), CTX, + ); + await expect(svc.decideNode(req.id, { + decision: 'approve', actorId: 'u1', + }, SYS)).rejects.toThrow(/VALIDATION_FAILED.*co_signer.*required to approve/s); + const first = await svc.decideNode(req.id, { + decision: 'approve', actorId: 'u1', outputs: { co_signer: 'u7' }, + }, SYS); + expect(first.finalized).toBe(false); + const second = await svc.decideNode(req.id, { + decision: 'approve', actorId: 'u2', outputs: { co_signer: 'u8' }, + }, SYS); + expect(second.finalized).toBe(true); + // Last decision wins the merge — the finalizing approver's pick is what + // the flow resumes with. + expect(second.outputs).toEqual({ co_signer: 'u8' }); + }); + // ── approver expansion: position (ADR-0090 D3) ────────────────── const positionInput = (extra: Record = {}) => ({ diff --git a/packages/plugins/plugin-approvals/src/approval-service.ts b/packages/plugins/plugin-approvals/src/approval-service.ts index a8ed70efb2..4f12e461cf 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.ts @@ -278,6 +278,23 @@ function csvSplit(raw: unknown): string[] { return String(raw).split(',').map(s => s.trim()).filter(Boolean); } +/** + * Is a submitted decision output blank — i.e. does it fail a `required` + * declaration (objectui#2955)? + * + * "Present but empty" has to count as missing: a decision UI sends whatever + * its widget holds, and an untouched picker/text box is `''` or `[]`. Letting + * those through would satisfy `required` with a value the downstream + * `expression` approver then resolves to nobody — the exact stall the flag + * exists to prevent. `false` and `0` are real values and pass. + */ +function isBlankDecisionOutput(value: unknown): boolean { + if (value === undefined || value === null) return true; + if (typeof value === 'string') return value.trim() === ''; + if (Array.isArray(value)) return value.filter(v => v !== null && v !== undefined && String(v).trim() !== '').length === 0; + return false; +} + /** * Humanize a machine name for display fallback: strips a `flow:` prefix and * title-cases underscore/dash segments (`flow:manager_review` → "Manager @@ -1641,10 +1658,11 @@ export class ApprovalService implements IApprovalService { // caller bug; `decision`/`requestId` are reserved by the resume envelope. const outputKeys = input.outputs ? Object.keys(input.outputs) : []; let acceptedOutputs: Record | undefined; + // Typed declarations and bare keys whitelist identically — one normalizer + // (spec) is the single reader of the union shape. + const declaredDefs = normalizeDecisionOutputs((config as any).decisionOutputs); if (outputKeys.length) { - // Typed declarations and bare keys whitelist identically — one - // normalizer (spec) is the single reader of the union shape. - const declared = normalizeDecisionOutputs((config as any).decisionOutputs).map(d => d.key); + const declared = declaredDefs.map(d => d.key); if (!declared.length) { throw new Error( `VALIDATION_FAILED: this approval node declares no decisionOutputs — outputs are not accepted. ` @@ -1669,6 +1687,35 @@ export class ApprovalService implements IApprovalService { acceptedOutputs = { ...input.outputs }; } + // objectui#2955: `required` outputs. Unlike `type`/`multiple` — which only + // shape the input widget — this one is a runtime contract: the flow must + // never resume past this node with a required key missing, because that is + // precisely what a downstream `expression` approver reads. Before this, + // an author's only backstop was `onEmptyApprovers` (the next node opens, + // resolves nobody, and stalls for an admin rescue). + // + // APPROVE only. A reject leaves down the reject edge, where the outputs + // are not read — demanding routing data to say "no" would block the + // rejection. Outputs still ride a reject when the approver filled them. + // + // Enforced for EVERY approve path, with no elevation bypass: a one-click + // email action link cannot fill a form, and an `auto_approve` SLA + // escalation has nobody to ask — both must fail rather than resume the run + // with the key missing. The escalation sweep already isolates a throwing + // request (it catches per-request), so that decision simply stays pending + // and visibly overdue instead of advancing into a broken node. + if (input.decision === 'approve') { + const missing = declaredDefs + .filter(d => d.required === true && isBlankDecisionOutput(input.outputs?.[d.key])) + .map(d => d.key); + if (missing.length) { + throw new Error( + `VALIDATION_FAILED: decision output(s) \`${missing.join('`, `')}\` are required to approve this ` + + `request — open the approval and fill them in before approving.`, + ); + } + } + // Audit the decision first so the quorum/per_group tally below sees it. await this.engine.insert('sys_approval_action', { id: uid('aact'), request_id: requestId, organization_id: org, diff --git a/packages/spec/authorable-surface.json b/packages/spec/authorable-surface.json index cb696817e5..0c72a84183 100644 --- a/packages/spec/authorable-surface.json +++ b/packages/spec/authorable-surface.json @@ -2287,6 +2287,7 @@ "automation/DecisionOutputDef:key", "automation/DecisionOutputDef:label", "automation/DecisionOutputDef:multiple", + "automation/DecisionOutputDef:required", "automation/DecisionOutputDef:type", "automation/ETLDestination:config", "automation/ETLDestination:connector", diff --git a/packages/spec/src/automation/approval.test.ts b/packages/spec/src/automation/approval.test.ts index 618b0edce3..3170ca532b 100644 --- a/packages/spec/src/automation/approval.test.ts +++ b/packages/spec/src/automation/approval.test.ts @@ -14,6 +14,7 @@ import { ApprovalEscalationSchema, ApprovalNodeConfigSchema, getApprovalNodeConfigJsonSchema, + normalizeDecisionOutputs, } from './approval.zod'; describe('ApproverType', () => { @@ -280,3 +281,39 @@ describe('ApprovalEscalationSchema', () => { expect(() => ApprovalEscalationSchema.parse({ enabled: true, timeoutHours: 0 })).toThrow(); }); }); + +/** + * `normalizeDecisionOutputs` is the ONE reader of the bare-key | typed-object + * union — the runtime whitelists from it, and the request row surfaces its + * output to the decision UI. Anything it drops is invisible to both sides, + * which is how `required` has to be pinned here and not only at the service. + */ +describe('normalizeDecisionOutputs', () => { + it('lifts bare keys into the typed form', () => { + expect(normalizeDecisionOutputs(['next_reviewers', 'note'])).toEqual([ + { key: 'next_reviewers' }, + { key: 'note' }, + ]); + }); + + it('carries the widget hints and the required flag through (objectui#2955)', () => { + expect(normalizeDecisionOutputs([ + { key: 'positions', label: 'Co-signers', type: 'position', multiple: true, required: true }, + ])).toEqual([ + { key: 'positions', label: 'Co-signers', type: 'position', multiple: true, required: true }, + ]); + }); + + it('omits the falsy flags rather than spelling them out', () => { + // The normalized shape is what the request row ships to every client, so + // an absent flag must stay absent instead of becoming `required: false`. + expect(normalizeDecisionOutputs([{ key: 'note', multiple: false, required: false }])) + .toEqual([{ key: 'note' }]); + }); + + it('drops entries with no usable key, and non-arrays entirely', () => { + expect(normalizeDecisionOutputs(['', { key: '' }, { label: 'no key' }, 42])).toEqual([]); + expect(normalizeDecisionOutputs(undefined)).toEqual([]); + expect(normalizeDecisionOutputs('next_reviewers')).toEqual([]); + }); +}); diff --git a/packages/spec/src/automation/approval.zod.ts b/packages/spec/src/automation/approval.zod.ts index 9e8730a0d0..f009320fc0 100644 --- a/packages/spec/src/automation/approval.zod.ts +++ b/packages/spec/src/automation/approval.zod.ts @@ -457,6 +457,22 @@ export const DecisionOutputDefSchema = lazySchema(() => z.object({ .describe("Decision-dialog input widget (default 'text')"), /** Collect an id array instead of a single value (multi-select picker). */ multiple: z.boolean().optional().describe('Collect multiple values (id array)'), + /** + * The approver must supply this output to APPROVE (objectui#2955). + * + * Unlike `type`/`multiple` — which only shape the input widget — this one + * IS enforced by the runtime: an approve carrying no value for a required + * key is rejected before any write, so a downstream `expression` approver + * reading `vars..` can never face a missing key. Without it an + * author's only backstop was `onEmptyApprovers` — the run reaches the next + * node, finds nobody, and stalls for an admin rescue. + * + * NOT enforced on reject: the run leaves down the reject edge, where the + * outputs are not read, and demanding routing data to say "no" would block + * the rejection. Outputs are still accepted on a reject if the approver + * filled them in. + */ + required: z.boolean().optional().describe('Approver must supply this output to approve'), })); export type DecisionOutputDef = z.infer; @@ -482,6 +498,7 @@ export function normalizeDecisionOutputs( ...(typeof e.label === 'string' && e.label ? { label: e.label } : {}), ...(typeof e.type === 'string' && e.type !== 'text' ? { type: e.type as DecisionOutputDef['type'] } : {}), ...(e.multiple === true ? { multiple: true } : {}), + ...(e.required === true ? { required: true } : {}), }); } } diff --git a/packages/spec/src/contracts/approval-service.ts b/packages/spec/src/contracts/approval-service.ts index 778e74fa7f..6de7aba5e7 100644 --- a/packages/spec/src/contracts/approval-service.ts +++ b/packages/spec/src/contracts/approval-service.ts @@ -69,6 +69,13 @@ export interface ApprovalRequestRow { label?: string; type?: 'text' | 'user' | 'department' | 'position' | 'team'; multiple?: boolean; + /** + * The approver must supply this one to APPROVE (objectui#2955) — enforced + * by `decide()`, so a decision UI should block the approve action on a + * blank value rather than letting the server reject the round trip. + * Never enforced on reject. + */ + required?: boolean; }>; completed_at?: string; created_at?: string; @@ -286,6 +293,10 @@ export interface ApprovalDecisionInput { * `decision` / `requestId` are reserved. Accepted outputs resume the run as * `.` flow variables, where a later approval node's * `expression` approver can read them (`vars..picked_departments`). + * + * An output declared `required` must carry a non-blank value on an APPROVE + * (objectui#2955); the decision is rejected before any write otherwise. A + * reject never requires them. */ outputs?: Record; } diff --git a/skills/objectstack-automation/SKILL.md b/skills/objectstack-automation/SKILL.md index 0acb693057..3660efd888 100644 --- a/skills/objectstack-automation/SKILL.md +++ b/skills/objectstack-automation/SKILL.md @@ -457,7 +457,11 @@ declares keys, approvers only fill values. Accepted outputs resume the run as `vars..` — this is how "the previous approver picks the next step's approvers" works without writing to a record field (see Dynamic approvers below). A decision carrying an undeclared key is rejected; -`decision` / `requestId` are reserved. +`decision` / `requestId` are reserved. A declaration marked +`required: true` must carry a non-blank value to **approve** (never to +reject) — enforced before any write, with no elevation bypass, so the run +cannot resume past the node with the key a later `expression` approver reads +still missing. ### Approver Types @@ -536,10 +540,12 @@ export const DynamicApprovalFlow = defineFlow({ // declaration renders a multi-select sys_user picker in the decision // dialog; the lead approves with outputs: // POST …/approve { outputs: { next_reviewers: ['u2', 'u3'] } } + // `required: true` is enforced by the runtime on APPROVE (never on + // reject) — node B below has nobody to route to without it. id: 'lead_review', type: 'approval', label: 'Lead Review', config: { approvers: [{ type: 'org_membership_level', value: 'owner' }], - decisionOutputs: [{ key: 'next_reviewers', label: 'Next Reviewers', type: 'user', multiple: true }], + decisionOutputs: [{ key: 'next_reviewers', label: 'Next Reviewers', type: 'user', multiple: true, required: true }], }, }, {