diff --git a/.changeset/approval-override-audit-marker.md b/.changeset/approval-override-audit-marker.md new file mode 100644 index 0000000000..16dd713433 --- /dev/null +++ b/.changeset/approval-override-audit-marker.md @@ -0,0 +1,47 @@ +--- +"@objectstack/spec": patch +"@objectstack/plugin-approvals": patch +--- + +fix(approvals): record an admin override of a staffed approver slate AS an override (#4466) + +An admin who is not in a request's `pending_approvers` may still act on it — the +`#3424` privileged-override path exists so a request routed to an unstaffed +position, or to approvers who have all left, is not undecidable forever. The +override is defensible; what was not is what the audit trail recorded. + +`sys_approval_action` had no override column at all. So an admin overriding a +properly-staffed slate wrote a row **byte-for-byte identical** to the designated +approver approving normally: a reader of the timeline saw `approve` by the admin +and could not tell whether the admin *was* an approver or *overrode* the ones who +were, and the bypassed approver's later `409 INVALID_STATE` was the only trace — +existing only if they happened to try. The platform knows at decision time (it +took the `isOverrideActor` branch to admit the call at all), so this was dropped +information, not unavailable information. The whole point of an approval record +is to answer "who authorized this, and were they entitled to?". + +`sys_approval_action` now carries **`via_override`** (boolean, optional), set on +exactly the actions admitted by that branch — `decideNode`'s approve/reject and +`reassign`'s admin rescue. It is surfaced on `ApprovalActionRow.via_override` +(`@objectstack/spec/contracts`), returned by `listActions`, and added to the +object's `highlightFields` and two grid list views so a timeline can say +"overrode the approver slate" instead of rendering it as an ordinary approval. + +Three distinctions the column keeps apart deliberately: + +- **`true`** — the actor held no slot in the slate and was admitted only by the + override branch. +- **`false`** — checked, and it was not an override. An admin who *is* a + designated approver is approving normally and records `false`: the marker is + about which branch admitted the call, not about whether the actor holds admin + rights. +- **absent** — a row written before this column existed. "Not recorded" is not + the same claim as "not an override", so `rowFromAction` maps `null` to + `undefined` rather than to `false`. + +Additive and nullable, so this needs no data migration: existing rows keep +working and simply read as unrecorded. Levelled `patch` rather than `minor` +because nothing an author writes changes — but note it *is* an observable +behaviour change on a read surface: `listActions` responses and the +`sys_approval_action` grid views now carry a field consumers did not see before, +and `sys_approval_action` gains a column on next schema sync. diff --git a/.changeset/screen-resume-declared-field-contract.md b/.changeset/screen-resume-declared-field-contract.md new file mode 100644 index 0000000000..78b441e368 --- /dev/null +++ b/.changeset/screen-resume-declared-field-contract.md @@ -0,0 +1,49 @@ +--- +"@objectstack/spec": patch +"@objectstack/service-automation": patch +"@objectstack/runtime": patch +--- + +fix(automation): `resume` enforces the suspended screen's declared field contract (#4477) + +A `screen` node's `config.fields` is a complete input contract — the author +declares the keys, their `required`-ness, and (via `visibleWhen`) when a field +is even asked for. The RENDER half honoured all of it: the paused result and +`GET …/runs/:runId/screen` carry `required` and `visibleWhen` intact. There was +no VALIDATION half — `POST …/runs/:runId/resume` folded whatever bag it was +handed straight into the flow variables, so a caller that skipped the dialog and +posted here directly was unconstrained by every `required` the author wrote. +Missing required fields, and keys the screen never declared, all completed the +run with `success: true`. + +Screen flows are the one place where the declared field contract is the ONLY +contract — no object schema sits behind a screen node to catch a bad bag +downstream. The platform already enforces the analogous contract everywhere else +this seam appears: action params (ADR-0104 D2), record writes (ADR-0113), +approval `decisionOutputs` (#3447). This is that rule for screen resume, built in +the same shape. + +`resume` now refuses a non-conforming submission with the new +`AutomationResult.code` `'INVALID_SCREEN_INPUT'` (a transport maps it to **400**, +as the automation domain route now does) and an `Invalid screen input: …` message +that names each violation and lists the declared field names. The refusal happens +BEFORE the suspension is consumed, so the pause stays live and the legitimate +submission still lands. + +`visibleWhen` is evaluated against the SUBMITTED values first (layered over the +run's variable snapshot), so a hidden field's `required` never fires — enforcing +it would dead-end the run at a field the user was never shown, which is #3528 +reproduced server-side. A predicate that cannot be evaluated is logged and +treated as hidden rather than visible: the client decides what the user saw, and +a broken predicate is not evidence a field was on screen. + +Scope, deliberately narrow — three shapes keep the historical pass-through: + +- an **object-form** screen (`kind: 'object-form'`), whose `fields` is empty by + construction because the client renders the object's own form and the write + path enforces that object's `required` fields itself; +- a **message-only** screen (`waitForInput: true`, no fields), which declares no + keys and so constrains none — the same pass-through `enforceActionParams` + gives a param-less action; +- `signal.output`, the node-OUTPUT namespace, which belongs to the approval-style + resume envelope rather than to the screen's collected-values channel. diff --git a/packages/plugins/plugin-approvals/src/approval-override-audit.test.ts b/packages/plugins/plugin-approvals/src/approval-override-audit.test.ts new file mode 100644 index 0000000000..6896f0e680 --- /dev/null +++ b/packages/plugins/plugin-approvals/src/approval-override-audit.test.ts @@ -0,0 +1,210 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * An admin override of a STAFFED approver slate is recorded as an override (#4466). + * + * The reproduction is the issue's: `showcase_dynamic_approval` stage 2 resolved + * its `expression` approvers correctly to exactly one designated user, and the + * admin — who was not in that slate — approved it anyway through the #3424 + * privileged path. The designated approver then got `409 INVALID_STATE`. + * + * The override itself is defensible; what was not is the audit trail. Before + * this, `sys_approval_action` had no override column at all, so an admin + * overriding a properly-staffed slate wrote a row byte-for-byte identical to the + * designated approver approving normally. A reader of the timeline saw `approve` + * by the admin and could not tell which had happened, and the bypassed + * approver's 409 was the only trace — existing only if they happened to try. + * + * The platform KNOWS at decision time: it took the `isOverrideActor` branch to + * admit the call. This was dropped information, not unavailable information. + */ + +import { describe, it, expect, beforeEach } from 'vitest'; +import { ApprovalService } from './approval-service.js'; + +interface FakeRow { [k: string]: any } + +/** The same minimal engine shape `approval-service.test.ts` uses. */ +function makeFakeEngine() { + const tables: Record = {}; + const ensure = (n: string) => (tables[n] ??= []); + function matches(row: FakeRow, filter: any): boolean { + if (!filter || typeof filter !== 'object') return true; + for (const [k, v] of Object.entries(filter)) { + if (k === '$or') { + if (!(v as any[]).some(sub => matches(row, sub))) return false; + continue; + } + const rv = row[k]; + if (v != null && typeof v === 'object' && '$in' in (v as any)) { + if (!(v as any).$in.includes(rv)) return false; + continue; + } + if (rv !== v) return false; + } + return true; + } + return { + _tables: tables, + async find(object: string, options?: any) { + const rows = ensure(object).filter(r => matches(r, options?.filter ?? options?.where)); + if (options?.orderBy?.[0]) { + const { field, order } = options.orderBy[0]; + rows.sort((a, b) => { + const av = a[field]; const bv = b[field]; + if (av === bv) return 0; + const cmp = av > bv ? 1 : -1; + return order === 'desc' ? -cmp : cmp; + }); + } + const start = options?.offset ?? 0; + return rows.slice(start, start + (options?.limit ?? 1000)); + }, + async insert(object: string, data: any) { ensure(object).push({ ...data }); return { ...data }; }, + async update(object: string, idOrData: any, _opts?: any) { + const data = typeof idOrData === 'object' ? idOrData : _opts; + const id = typeof idOrData === 'object' ? idOrData.id : idOrData; + const table = ensure(object); + const i = table.findIndex(r => r.id === id); + if (i >= 0) table[i] = { ...table[i], ...data }; + return table[i]; + }, + async delete(object: string, options?: any) { + const table = ensure(object); + const i = table.findIndex(r => r.id === (options?.where?.id ?? options?.id)); + if (i >= 0) table.splice(i, 1); + return {}; + }, + registerHook() {}, unregisterHooksByPackage() { return 0; }, async fire() {}, + }; +} + +const SYS = { isSystem: true, positions: [], permissions: [] } as any; +const SUBMITTER = { userId: 'u1', tenantId: 't1', positions: [], permissions: [] } as any; +/** The designated approver the slate actually names. */ +const DESIGNATED = { userId: 'u9', tenantId: 't1', positions: [], permissions: [] } as any; +/** An admin who is NOT in the slate — the issue's actor. */ +const ADMIN = { userId: 'root', tenantId: 't1', positions: [], permissions: ['admin_full_access'] } as any; +/** An admin who IS a designated approver — approving normally, not overriding. */ +const ADMIN_ON_SLATE = { userId: 'u9', tenantId: 't1', positions: [], permissions: ['admin_full_access'] } as any; + +describe('approval override audit marker (#4466)', () => { + let engine: ReturnType; + let svc: ApprovalService; + let n = 0; + const baseTime = new Date('2026-01-15T10:00:00Z').getTime(); + + beforeEach(() => { + engine = makeFakeEngine(); + n = 0; + svc = new ApprovalService({ + engine: engine as any, + clock: { now: () => new Date(baseTime + (n++) * 1000) }, + }); + }); + + /** A request whose slate is properly STAFFED — one real, designated user. */ + const staffedInput = (extra: Record = {}) => ({ + object: 'opportunity', recordId: 'opp1', runId: 'run_1', nodeId: 'co_sign', + flowName: 'showcase_dynamic_approval', + config: { + approvers: [{ type: 'user' as const, value: 'u9' }], + behavior: 'first_response' as const, lockRecord: true, + }, + record: { id: 'opp1', amount: 100 }, + ...extra, + }); + + it('the repro: the slate names exactly one designated user, and the admin is not on it', async () => { + const req = await svc.openNodeRequest(staffedInput(), SUBMITTER); + expect(req.pending_approvers).toEqual(['u9']); + expect(req.pending_approvers).not.toContain('root'); + }); + + it('marks an admin override of a STAFFED slate as `via_override`', async () => { + const req = await svc.openNodeRequest(staffedInput(), SUBMITTER); + await svc.decideNode(req.id, { decision: 'approve', actorId: 'root', comment: 'not mine' }, ADMIN); + + const acts = await svc.listActions(req.id, SYS); + const decision = acts.at(-1)!; + expect(decision).toMatchObject({ action: 'approve', actor_id: 'root', comment: 'not mine' }); + // The bit the audit trail used to drop entirely. + expect(decision.via_override).toBe(true); + }); + + it('does NOT mark the designated approver’s own approval', async () => { + const req = await svc.openNodeRequest(staffedInput(), SUBMITTER); + await svc.decideNode(req.id, { decision: 'approve', actorId: 'u9' }, DESIGNATED); + + const decision = (await svc.listActions(req.id, SYS)).at(-1)!; + expect(decision).toMatchObject({ action: 'approve', actor_id: 'u9' }); + // An explicit `false`, not absent: "checked, and it was not an override" is + // a different claim from a legacy row's "not recorded". + expect(decision.via_override).toBe(false); + }); + + it('does NOT mark an admin who is ALSO a designated approver — that is an ordinary approval', async () => { + // The marker is about which BRANCH admitted the call, not about whether the + // actor happens to hold admin rights. Collapsing the two would make every + // admin's ordinary decision read as a slate bypass. + const req = await svc.openNodeRequest(staffedInput(), SUBMITTER); + await svc.decideNode(req.id, { decision: 'approve', actorId: 'u9' }, ADMIN_ON_SLATE); + + expect((await svc.listActions(req.id, SYS)).at(-1)!.via_override).toBe(false); + }); + + it('marks an override REJECTION too — the direction of the decision is irrelevant', async () => { + const req = await svc.openNodeRequest(staffedInput(), SUBMITTER); + await svc.decideNode(req.id, { decision: 'reject', actorId: 'root' }, ADMIN); + + const decision = (await svc.listActions(req.id, SYS)).at(-1)!; + expect(decision).toMatchObject({ action: 'reject', via_override: true }); + }); + + it('the override and the ordinary approval are no longer identical rows', async () => { + // The issue's core claim, asserted directly: before the column, these two + // decisions differed only in `actor_id` — and an `actor_id` alone cannot + // answer "were they entitled to?". + const a = await svc.openNodeRequest(staffedInput(), SUBMITTER); + await svc.decideNode(a.id, { decision: 'approve', actorId: 'root' }, ADMIN); + const overrideRow = (await svc.listActions(a.id, SYS)).at(-1)!; + + const b = await svc.openNodeRequest( + staffedInput({ recordId: 'opp2', runId: 'run_2', record: { id: 'opp2', amount: 100 } }), + SUBMITTER, + ); + await svc.decideNode(b.id, { decision: 'approve', actorId: 'u9' }, DESIGNATED); + const normalRow = (await svc.listActions(b.id, SYS)).at(-1)!; + + expect(overrideRow.action).toBe(normalRow.action); + expect(overrideRow.via_override).not.toBe(normalRow.via_override); + }); + + it('marks an admin RESCUE reassign of a slate they hold no slot in', async () => { + // #3424's other privileged action: handing a stuck (or staffed) request to + // a real approver. Same fact, same column. + const req = await svc.openNodeRequest(staffedInput(), SUBMITTER); + await svc.reassign(req.id, { actorId: 'root', to: 'u7' }, ADMIN); + + const row = (await svc.listActions(req.id, SYS)).at(-1)!; + expect(row).toMatchObject({ action: 'reassign', reassign_to: 'u7', via_override: true }); + }); + + it('does NOT mark a slot holder handing over their own slot', async () => { + const req = await svc.openNodeRequest(staffedInput(), SUBMITTER); + await svc.reassign(req.id, { actorId: 'u9', to: 'u7' }, DESIGNATED); + + expect((await svc.listActions(req.id, SYS)).at(-1)!.via_override).toBe(false); + }); + + it('a legacy row (written before the column existed) reads as UNRECORDED, not as "not an override"', async () => { + const req = await svc.openNodeRequest(staffedInput(), SUBMITTER); + // Simulate a row persisted by an older build: the column is simply absent. + const rows = engine._tables['sys_approval_action']; + delete rows[rows.length - 1].via_override; + + const row = (await svc.listActions(req.id, SYS)).at(-1)!; + expect(row.via_override).toBeUndefined(); + expect(row.via_override).not.toBe(false); + }); +}); diff --git a/packages/plugins/plugin-approvals/src/approval-service.ts b/packages/plugins/plugin-approvals/src/approval-service.ts index 592ccbfcd6..027b6a49af 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.ts @@ -168,6 +168,42 @@ const TERMINAL_RUN_STATUSES: ReadonlySet = new Set([ 'completed', 'failed', 'cancelled', 'timed_out', ]); +/** + * Request statuses that can leave a ZOMBIE behind (#4469) — the terminal states + * a decision reaches only by ALSO resuming the owning run. + * + * `recalled` is deliberately absent: a recall ABANDONS the request on purpose, + * and {@link ApprovalService.recall} explicitly tolerates a run it cannot + * resume (the withdrawal and the lock release are the point). Reporting those + * would bury the real findings under expected ones. + */ +const STRANDABLE_REQUEST_STATUSES = ['approved', 'rejected', 'returned'] as const; + +/** + * One terminal request whose owning flow run is unrecoverable (#4469) — the + * decision was recorded and the flow never moved. Reporting shape only: the + * sweep never rewrites these rows (see + * {@link ApprovalService.inspectStrandedRequests}). + */ +export interface StrandedApprovalRequest { + requestId: string; + /** Terminal status the request reached — the decision that WAS recorded. */ + status: string; + /** The `flow_run_id` that resolves to neither a suspension nor a run history row. */ + runId: string; + flowName?: string; + /** Approval node the run should have continued from. */ + nodeId?: string; + objectName: string; + recordId: string; + organizationId?: string | null; + completedAt?: string; + /** `config.approvalStatusField`, when the node mirrors status onto the record. */ + mirrorField?: string; + /** What that mirror field currently reads — usually the stale value an operator sees. */ + mirroredStatus?: string; +} + /** Default lifetime of an actionable-link token (ADR-0043). */ export const ACTION_TOKEN_TTL_MS = 72 * 60 * 60 * 1000; @@ -453,6 +489,11 @@ function rowFromAction(row: any): ApprovalActionRow { // Structured reassign hand-off parties (#4365). reassign_from: row.reassign_from ?? undefined, reassign_to: row.reassign_to ?? undefined, + // #4466 — surfaced so a timeline can SAY "overridden the approver slate" + // rather than render an override identically to an ordinary approval. + // `null` (a row written before the column existed) stays `undefined`: + // "not recorded" is not the same claim as "not an override". + via_override: row.via_override == null ? undefined : row.via_override === true, // Decision attachments (#3266): rich descriptors carrying the display name + // download URL, so consumers label/open them without reading `sys_file`. attachments: attachments.length ? attachments : undefined, @@ -1664,6 +1705,11 @@ export class ApprovalService implements IApprovalService { if (!isSlotHolder && !isOverride) { throw new Error(`FORBIDDEN: actor '${actorId}' is not a pending approver`); } + // #4466 — the audit fact this decision would otherwise drop: the actor was + // admitted ONLY by the override branch, holding no slot in the staffed + // slate. An admin who IS a slot holder is approving normally, so the two + // conditions are recorded apart rather than collapsed into "actor is admin". + const viaOverride = isOverride && !isSlotHolder; const config = parseJson(raw.node_config_json, { approvers: [], behavior: 'first_response' } as any); const org = raw.organization_id ?? null; @@ -1750,6 +1796,10 @@ export class ApprovalService implements IApprovalService { id: uid('aact'), request_id: requestId, organization_id: org, step_name: nodeId, step_index: 0, action: input.decision, actor_id: actorId, comment: input.comment ?? null, + // #4466: the override is recorded on the DECISION, not inferred later. + // Written as an explicit `false` for an ordinary decision so a reader can + // tell "checked, and it was not an override" from a legacy row's `null`. + via_override: viaOverride, attachments: input.attachments?.length ? input.attachments : null, created_at: now, }, { context: SYSTEM_CTX }); @@ -2405,6 +2455,11 @@ export class ApprovalService implements IApprovalService { } const isOverride = this.isOverrideActor(context, raw.organization_id ?? null); const from = String(input.from ?? actorId).trim(); + // #4466 — same rule as `decideNode`: the marker records that the actor was + // admitted only by the privileged branch, holding no slot themselves. A + // reassign is the other action #3424 lets an admin take over a slate they + // are not on, so it carries the same fact. + const viaOverride = isOverride && !pending.includes(actorId); let next: string[]; if (pending.includes(from)) { // Normal hand-off: the actor holds the slot being moved (or is a @@ -2431,6 +2486,7 @@ export class ApprovalService implements IApprovalService { // comment (`""`) baked raw user ids into user-facing text. // `comment` is pure user input: absent unless the actor wrote one. actor_id: actorId, reassign_from: from, reassign_to: to, + via_override: viaOverride, comment: input.comment ?? null, created_at: now, }, { context: SYSTEM_CTX }); // per_group / quorum (#3266): carry the delegated slot's group membership to @@ -2817,6 +2873,145 @@ export class ApprovalService implements IApprovalService { * the real cause and {@link DEAD_RUN_ACTOR_ID} the real actor, so a dead-run * release is never mistaken for a submitter's withdrawal. */ + /** + * Read-only inspection for the OTHER dead-run shape: a request that is + * already TERMINAL while its `flow_run_id` points at nothing (#4469). + * + * #4460 stopped new ones being produced; nothing found the ones already + * stuck. The failure mode (#4420) is a request row flipped to `approved` / + * `rejected` / `returned` whose owning run no longer exists — the decision + * landed, the flow never moved. Any deployment on 17.0.0-rc.1 that hit the + * wiring hole and crossed a restart mid-approval can be carrying these rows. + * + * {@link releaseDeadRunRequests} cannot see them, for a reason worth naming: + * it scans `status: 'pending'`, and the very step that zombified the request + * is the one that took it OUT of `pending`. The act of breaking it removed it + * from the only sweeper's field of view — which is a large part of why this + * class of failure stayed silent. + * + * It also could not have answered the question even if it looked: its + * liveness oracle is `getRun`, which reads the execution LOG, and after a + * restart that returns `null` for a perfectly ALIVE suspended run. It treats + * `null` as alive (conservative, correct) — but that means it has no way to + * say "this run is really gone". + * + * So this uses BOTH oracles, and a row must fail both to be reported: + * + * - `hasSuspendedRun(runId) === false` — the suspension store itself says no + * live pause exists. It THROWS when the store cannot be read, and that + * case is SKIPPED, never counted as dead: an unreadable store means + * "unknown", and a storage outage must not be published as a lost run. + * - `getRun(runId) == null` — no terminal history row either (the `run_` + * prefixed rows in `sys_automation_run`). A run that merely finished is + * not stranded; a request whose run neither waits nor ever completed is. + * + * **Reports; never rewrites.** No status is changed and no run is cancelled. + * The decision genuinely happened — a human approved or rejected — and + * silently rolling it back would make the audit trail disagree with the + * facts. What an operator needs first is visibility: which requests are stuck + * at which step, and what the mirrored status field on the business record + * still says. Whether to re-run the downstream actions or re-open the + * approval is a judgement call this cannot make. + */ + async inspectStrandedRequests(options?: { limit?: number }): Promise<{ + scanned: number; + stranded: StrandedApprovalRequest[]; + /** Rows skipped because the suspension store could not be read — NOT healthy, just unknown. */ + undetermined: number; + }> { + const empty = { scanned: 0, stranded: [] as StrandedApprovalRequest[], undetermined: 0 }; + // Both oracles are required. Without `hasSuspendedRun` there is no way to + // tell a live cross-restart pause from a dead run, and reporting on + // `getRun` alone would name every healthy paused approval as stranded. + if (typeof this.automation?.hasSuspendedRun !== 'function') return empty; + if (typeof this.automation?.getRun !== 'function') return empty; + + const limit = options?.limit ?? 500; + let rows: any[] = []; + try { + rows = await this.engine.find('sys_approval_request', { + where: { status: { $in: [...STRANDABLE_REQUEST_STATUSES] } }, limit, context: SYSTEM_CTX, + }) ?? []; + } catch (err: any) { + this.logger?.warn?.('[approvals] stranded-request scan failed to list requests', { + error: err?.message ?? String(err), + }); + return empty; + } + + const stranded: StrandedApprovalRequest[] = []; + let undetermined = 0; + for (const raw of rows) { + const runId = raw?.flow_run_id ? String(raw.flow_run_id) : ''; + if (!runId) continue; // not node-driven — no run was ever supposed to move + + let suspended: boolean; + try { + suspended = await this.automation.hasSuspendedRun!(runId); + } catch (err: any) { + // Store unreadable ⇒ existence unknown. Skipping is the only safe + // answer; counted so "0 stranded" can never be read as "all clear" + // when nothing could actually be checked. + undetermined++; + this.logger?.warn?.('[approvals] stranded-request scan could not read the suspension store', { + request: raw?.id, run: runId, error: err?.message ?? String(err), + }); + continue; + } + if (suspended) continue; // still parked — the run is alive and resumable + + let terminal: { status?: string } | null = null; + try { + terminal = await this.automation.getRun!(runId); + } catch (err: any) { + undetermined++; + this.logger?.warn?.('[approvals] stranded-request scan could not read the run history', { + request: raw?.id, run: runId, error: err?.message ?? String(err), + }); + continue; + } + if (terminal) continue; // the run ran to a terminal state — it is not dangling + + // Neither suspended nor ever finished: the run this decision was supposed + // to advance is genuinely gone. + const config = parseJson( + raw.node_config_json, { approvers: [], behavior: 'first_response' } as any, + ); + const mirrorField = config.approvalStatusField; + let mirroredStatus: string | undefined; + if (mirrorField) { + try { + const recs = await this.engine.find(raw.object_name, { + where: { id: raw.record_id }, limit: 1, context: SYSTEM_CTX, + }); + const rec: any = Array.isArray(recs) ? recs[0] : null; + if (rec) mirroredStatus = rec[mirrorField] ?? undefined; + } catch { /* display-only — a mirror read must never fail the scan */ } + } + stranded.push({ + requestId: String(raw.id), + status: raw.status, + runId, + flowName: typeof raw.process_name === 'string' ? raw.process_name.replace(/^flow:/, '') : undefined, + nodeId: raw.flow_node_id ?? raw.current_step ?? undefined, + objectName: raw.object_name, + recordId: raw.record_id, + organizationId: raw.organization_id ?? null, + completedAt: raw.completed_at ?? undefined, + mirrorField, + mirroredStatus, + }); + } + + if (stranded.length || undetermined) { + this.logger?.warn?.('[approvals] stranded terminal requests (decision recorded, flow run gone)', { + scanned: rows.length, stranded: stranded.length, undetermined, + requests: stranded.map(s => `${s.requestId}@${s.nodeId ?? '?'} → run ${s.runId}`), + }); + } + return { scanned: rows.length, stranded, undetermined }; + } + async releaseDeadRunRequests(): Promise<{ scanned: number; released: number }> { // No liveness oracle → no basis to declare anything dead. if (typeof this.automation?.getRun !== 'function') return { scanned: 0, released: 0 }; diff --git a/packages/plugins/plugin-approvals/src/approvals-plugin.ts b/packages/plugins/plugin-approvals/src/approvals-plugin.ts index 0b4ded6574..d51cc8331c 100644 --- a/packages/plugins/plugin-approvals/src/approvals-plugin.ts +++ b/packages/plugins/plugin-approvals/src/approvals-plugin.ts @@ -185,6 +185,12 @@ export class ApprovalsServicePlugin implements Plugin { const results = await Promise.allSettled([ svc.runEscalations(), svc.releaseDeadRunRequests(), + // #4469 — the other half of the dead-run picture, and the one no + // sweeper could see: a request already TERMINAL whose run is gone. + // Read-only by design (it reports; it never rewrites a decision + // that really happened), so it rides the same clock purely to make + // the finding surface without an operator knowing to go looking. + svc.inspectStrandedRequests(), ]); for (const r of results) { if (r.status === 'rejected') { diff --git a/packages/plugins/plugin-approvals/src/index.ts b/packages/plugins/plugin-approvals/src/index.ts index b72a388b4f..bfc7d9a9ec 100644 --- a/packages/plugins/plugin-approvals/src/index.ts +++ b/packages/plugins/plugin-approvals/src/index.ts @@ -23,6 +23,8 @@ export { // #3447 P2 — expression approvers + empty-slate auto-approve outcome. type ApproverExpressionContext, type ApprovalNodeAutoOutcome, + // #4469 — the read-only stranded-request inspection's report shape. + type StrandedApprovalRequest, } from './approval-service.js'; export { ApprovalsServicePlugin, diff --git a/packages/plugins/plugin-approvals/src/stranded-request-inspection.test.ts b/packages/plugins/plugin-approvals/src/stranded-request-inspection.test.ts new file mode 100644 index 0000000000..4277ff717a --- /dev/null +++ b/packages/plugins/plugin-approvals/src/stranded-request-inspection.test.ts @@ -0,0 +1,240 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Stranded terminal requests are found (#4469). + * + * #4420's failure shape: a request row flipped to `approved` (or `rejected`) + * while its `flow_run_id` points at a run that no longer exists — the decision + * landed, the flow never moved. #4460 stopped NEW ones being produced; the rows + * already stuck had no mechanism to find or release them. + * + * `releaseDeadRunRequests` cannot see them, and the reason is the interesting + * part: it scans `status: 'pending'`, and the very step that zombified the + * request is the one that took it OUT of `pending`. Breaking it removed it from + * the only sweeper's field of view. Its liveness oracle could not have answered + * anyway — `getRun` reads the execution LOG, which returns `null` for a + * perfectly alive suspended run after a restart. + * + * So the inspection uses BOTH oracles and reports only rows that fail both, + * skipping (never condemning) anything the stores could not answer for. + */ + +import { describe, it, expect, beforeEach } from 'vitest'; +import { ApprovalService } from './approval-service.js'; + +interface FakeRow { [k: string]: any } + +function makeFakeEngine() { + const tables: Record = {}; + const ensure = (n: string) => (tables[n] ??= []); + function matches(row: FakeRow, filter: any): boolean { + if (!filter || typeof filter !== 'object') return true; + for (const [k, v] of Object.entries(filter)) { + const rv = row[k]; + if (v != null && typeof v === 'object' && '$in' in (v as any)) { + if (!(v as any).$in.includes(rv)) return false; + continue; + } + if (rv !== v) return false; + } + return true; + } + return { + _tables: tables, + async find(object: string, options?: any) { + const rows = ensure(object).filter(r => matches(r, options?.filter ?? options?.where)); + return rows.slice(0, options?.limit ?? 1000); + }, + async insert(object: string, data: any) { ensure(object).push({ ...data }); return { ...data }; }, + async update(object: string, idOrData: any, _opts?: any) { + const data = typeof idOrData === 'object' ? idOrData : _opts; + const id = typeof idOrData === 'object' ? idOrData.id : idOrData; + const table = ensure(object); + const i = table.findIndex(r => r.id === id); + if (i >= 0) table[i] = { ...table[i], ...data }; + return table[i]; + }, + async delete() { return {}; }, + registerHook() {}, unregisterHooksByPackage() { return 0; }, async fire() {}, + }; +} + +/** A terminal request row as the zombie leaves it: decision recorded, run gone. */ +function requestRow(over: Record = {}): FakeRow { + return { + id: 'areq_1', + process_name: 'flow:deal_approval', + object_name: 'opportunity', + record_id: 'opp1', + status: 'approved', + flow_run_id: 'run_1', + flow_node_id: 'co_sign', + organization_id: 't1', + completed_at: '2026-01-15T10:00:05.000Z', + node_config_json: JSON.stringify({ + approvers: [{ type: 'user', value: 'u9' }], + behavior: 'first_response', + approvalStatusField: 'approval_status', + }), + ...over, + }; +} + +/** An automation surface with both oracles, each independently steerable. */ +function automation(opts: { + suspended?: Record; + suspendedThrows?: boolean; + history?: Record; + historyThrows?: boolean; +} = {}) { + return { + async resume() { return { success: true }; }, + async hasSuspendedRun(runId: string) { + if (opts.suspendedThrows) throw new Error('suspended-run store unreadable'); + return opts.suspended?.[runId] ?? false; + }, + async getRun(runId: string) { + if (opts.historyThrows) throw new Error('run history unreadable'); + return opts.history?.[runId] ?? null; + }, + } as any; +} + +describe('stranded terminal request inspection (#4469)', () => { + let engine: ReturnType; + let svc: ApprovalService; + + beforeEach(() => { + engine = makeFakeEngine(); + svc = new ApprovalService({ engine: engine as any }); + }); + + it('the blind spot, stated: the existing pending-only sweep cannot see a terminal zombie', async () => { + engine._tables['sys_approval_request'] = [requestRow()]; + svc.attachAutomation(automation()); + // `releaseDeadRunRequests` scans `status: 'pending'`; the zombie is + // `approved`, so its scan set is empty. + expect(await svc.releaseDeadRunRequests()).toEqual({ scanned: 0, released: 0 }); + }); + + it('finds a terminal request whose run is neither suspended nor ever completed', async () => { + engine._tables['sys_approval_request'] = [requestRow()]; + engine._tables['opportunity'] = [{ id: 'opp1', approval_status: 'pending' }]; + svc.attachAutomation(automation()); + + const out = await svc.inspectStrandedRequests(); + expect(out.scanned).toBe(1); + expect(out.undetermined).toBe(0); + expect(out.stranded).toHaveLength(1); + expect(out.stranded[0]).toMatchObject({ + requestId: 'areq_1', + status: 'approved', + runId: 'run_1', + nodeId: 'co_sign', + flowName: 'deal_approval', + objectName: 'opportunity', + recordId: 'opp1', + }); + }); + + it('reports the stale mirrored status — what an operator actually sees on the record', async () => { + // The decision says `approved`; the business record still reads `pending` + // because the flow never resumed to move it. That disagreement is the + // human-facing symptom, so the report carries it. + engine._tables['sys_approval_request'] = [requestRow()]; + engine._tables['opportunity'] = [{ id: 'opp1', approval_status: 'pending' }]; + svc.attachAutomation(automation()); + + const [row] = (await svc.inspectStrandedRequests()).stranded; + expect(row.mirrorField).toBe('approval_status'); + expect(row.mirroredStatus).toBe('pending'); + }); + + it('does NOT report a request whose run is still suspended — that approval is healthy', async () => { + engine._tables['sys_approval_request'] = [requestRow()]; + svc.attachAutomation(automation({ suspended: { run_1: true } })); + expect((await svc.inspectStrandedRequests()).stranded).toEqual([]); + }); + + it('does NOT report a request whose run ran to a terminal state — it finished, it is not dangling', async () => { + engine._tables['sys_approval_request'] = [requestRow()]; + svc.attachAutomation(automation({ history: { run_1: { status: 'completed' } } })); + expect((await svc.inspectStrandedRequests()).stranded).toEqual([]); + }); + + it('SKIPS a row whose suspension store threw — an outage is unknown, not dead', async () => { + // The whole point of `hasSuspendedRun` rejecting rather than answering + // `false` (#4460): a storage blip must never be published as a lost run. + engine._tables['sys_approval_request'] = [requestRow()]; + svc.attachAutomation(automation({ suspendedThrows: true })); + + const out = await svc.inspectStrandedRequests(); + expect(out.stranded).toEqual([]); + // …and it is COUNTED, so "0 stranded" can never be read as "all clear" + // when nothing could actually be checked. + expect(out.undetermined).toBe(1); + }); + + it('SKIPS a row whose run history threw — same reasoning, second oracle', async () => { + engine._tables['sys_approval_request'] = [requestRow()]; + svc.attachAutomation(automation({ historyThrows: true })); + + const out = await svc.inspectStrandedRequests(); + expect(out.stranded).toEqual([]); + expect(out.undetermined).toBe(1); + }); + + it('ignores a request with no `flow_run_id` — no run was ever supposed to move', async () => { + engine._tables['sys_approval_request'] = [requestRow({ flow_run_id: null })]; + svc.attachAutomation(automation()); + expect((await svc.inspectStrandedRequests()).stranded).toEqual([]); + }); + + it('ignores a `recalled` request — a recall abandons its run deliberately', async () => { + // `recall` explicitly tolerates a run it cannot resume; reporting those + // would bury the real findings under expected ones. + engine._tables['sys_approval_request'] = [requestRow({ status: 'recalled' })]; + svc.attachAutomation(automation()); + const out = await svc.inspectStrandedRequests(); + expect(out.scanned).toBe(0); + expect(out.stranded).toEqual([]); + }); + + it('covers `rejected` and `returned` too — both reach terminal only by resuming the run', async () => { + engine._tables['sys_approval_request'] = [ + requestRow({ id: 'areq_r', status: 'rejected', flow_run_id: 'run_r' }), + requestRow({ id: 'areq_v', status: 'returned', flow_run_id: 'run_v' }), + ]; + svc.attachAutomation(automation()); + const ids = (await svc.inspectStrandedRequests()).stranded.map(s => s.requestId); + expect(ids).toEqual(['areq_r', 'areq_v']); + }); + + it('NEVER rewrites a stranded row — the decision really happened', async () => { + // Auto-rolling back would make the audit trail disagree with the facts. + // The remedy (re-run downstream actions vs re-open the approval) is an + // operator judgement call, so the sweep only makes the rows visible. + engine._tables['sys_approval_request'] = [requestRow()]; + engine._tables['opportunity'] = [{ id: 'opp1', approval_status: 'pending' }]; + const before = JSON.stringify(engine._tables); + svc.attachAutomation(automation()); + + await svc.inspectStrandedRequests(); + + expect(JSON.stringify(engine._tables)).toBe(before); + expect(engine._tables['sys_approval_action'] ?? []).toHaveLength(0); + }); + + it('reports nothing when the engine offers no `hasSuspendedRun` — no oracle, no verdict', async () => { + // Without it there is no way to tell a live cross-restart pause from a dead + // run, and `getRun` alone would name every healthy paused approval stranded. + engine._tables['sys_approval_request'] = [requestRow()]; + svc.attachAutomation({ async resume() { return {}; }, async getRun() { return null; } } as any); + expect(await svc.inspectStrandedRequests()).toEqual({ scanned: 0, stranded: [], undetermined: 0 }); + }); + + it('reports nothing with no automation attached at all', async () => { + engine._tables['sys_approval_request'] = [requestRow()]; + expect(await svc.inspectStrandedRequests()).toEqual({ scanned: 0, stranded: [], undetermined: 0 }); + }); +}); diff --git a/packages/plugins/plugin-approvals/src/sys-approval-action.object.ts b/packages/plugins/plugin-approvals/src/sys-approval-action.object.ts index cd7a01cd49..b0055d25bc 100644 --- a/packages/plugins/plugin-approvals/src/sys-approval-action.object.ts +++ b/packages/plugins/plugin-approvals/src/sys-approval-action.object.ts @@ -25,7 +25,7 @@ export const SysApprovalAction = ObjectSchema.create({ displayNameField: 'id', nameField: 'id', // [ADR-0079] canonical primary-title pointer (mirrors deprecated displayNameField) titleFormat: '{action} · {step_name}', - highlightFields: ['request_id', 'step_name', 'action', 'actor_id', 'created_at'], + highlightFields: ['request_id', 'step_name', 'action', 'actor_id', 'via_override', 'created_at'], // ADR-0104 D3 wave 2. `attachments` is a media field, so the files it holds // are OWNED by this row — and the storage service would otherwise authorize @@ -42,7 +42,7 @@ export const SysApprovalAction = ObjectSchema.create({ name: 'recent', label: 'Recent', data: { provider: 'object', object: 'sys_approval_action' }, - columns: ['created_at', 'request_id', 'step_name', 'action', 'actor_id', 'comment'], + columns: ['created_at', 'request_id', 'step_name', 'action', 'actor_id', 'via_override', 'comment'], sort: [{ field: 'created_at', order: 'desc' }], pagination: { pageSize: 50 }, emptyState: { title: 'No approval actions yet', message: 'Actions are logged automatically when approvals progress.' }, @@ -62,7 +62,7 @@ export const SysApprovalAction = ObjectSchema.create({ name: 'all_actions', label: 'All', data: { provider: 'object', object: 'sys_approval_action' }, - columns: ['created_at', 'request_id', 'step_name', 'action', 'actor_id', 'comment'], + columns: ['created_at', 'request_id', 'step_name', 'action', 'actor_id', 'via_override', 'comment'], sort: [{ field: 'created_at', order: 'desc' }], pagination: { pageSize: 100 }, }, @@ -119,6 +119,32 @@ export const SysApprovalAction = ObjectSchema.create({ comment: Field.textarea({ label: 'Comment', required: false, group: 'Action' }), + // #4466 — the one bit of "who really decided this" that was still dropped. + // A privileged admin may act on a request whose staffed approver slate they + // hold no slot in (the #3424 override path); before this column, that + // decision was byte-for-byte identical to the designated approver's own + // approval. A reader of the timeline saw `approve` by the admin and could + // not tell whether the admin WAS an approver or OVERRODE the ones who were, + // and the bypassed approver's later `409 INVALID_STATE` was the only trace + // — existing only if they happened to try. + // + // The platform KNOWS at decision time: it took the `isOverrideActor` branch + // to admit the call at all. This is dropped information, not unavailable + // information. + // + // Set on exactly the decisions that were admitted BY that branch — an admin + // who is also a genuine slot holder is approving normally and is recorded + // as such. Nullable and additive: rows written before this column exists + // carry `null`, which reads as "not recorded", never as "not an override". + via_override: Field.boolean({ + label: 'Via Admin Override', + required: false, + group: 'Action', + description: + 'True when the actor was admitted to this action only by the privileged-override path (#3424) — ' + + 'they held no slot in the request’s pending-approver slate.', + }), + // Structured hand-off parties for `action: 'reassign'` (#4365). Before // these existed the pair lived only inside a default free-text comment // (""), which no client could parse or render readably. diff --git a/packages/plugins/plugin-approvals/src/translations/en.objects.generated.ts b/packages/plugins/plugin-approvals/src/translations/en.objects.generated.ts index 6350e68439..1263db7129 100644 --- a/packages/plugins/plugin-approvals/src/translations/en.objects.generated.ts +++ b/packages/plugins/plugin-approvals/src/translations/en.objects.generated.ts @@ -235,6 +235,10 @@ export const enObjects: NonNullable = { comment: { label: "Comment" }, + via_override: { + label: "Via Admin Override", + help: "True when the actor was admitted to this action only by the privileged-override path (#3424) — they held no slot in the request’s pending-approver slate." + }, reassign_from: { label: "Reassigned From", help: "User whose pending-approver slot was handed over (reassign actions only)" diff --git a/packages/plugins/plugin-approvals/src/translations/es-ES.objects.generated.ts b/packages/plugins/plugin-approvals/src/translations/es-ES.objects.generated.ts index 807985fd52..4537c54bf6 100644 --- a/packages/plugins/plugin-approvals/src/translations/es-ES.objects.generated.ts +++ b/packages/plugins/plugin-approvals/src/translations/es-ES.objects.generated.ts @@ -235,6 +235,10 @@ export const esESObjects: NonNullable = { comment: { label: "Comentario" }, + via_override: { + label: "Mediante anulación de administrador", + help: "Verdadero cuando el actor fue admitido en esta acción únicamente por la vía de anulación privilegiada (#3424): no ocupaba ningún puesto en la lista de aprobadores pendientes de la solicitud." + }, reassign_from: { label: "Reasignado de", help: "Usuario cuyo turno de aprobación pendiente fue traspasado (solo acciones de reasignación)" diff --git a/packages/plugins/plugin-approvals/src/translations/ja-JP.objects.generated.ts b/packages/plugins/plugin-approvals/src/translations/ja-JP.objects.generated.ts index 144266bccf..b82258128c 100644 --- a/packages/plugins/plugin-approvals/src/translations/ja-JP.objects.generated.ts +++ b/packages/plugins/plugin-approvals/src/translations/ja-JP.objects.generated.ts @@ -235,6 +235,10 @@ export const jaJPObjects: NonNullable = { comment: { label: "コメント" }, + via_override: { + label: "管理者オーバーライド経由", + help: "true の場合、実行者は特権オーバーライド経路(#3424)によってのみ許可されたことを示します — 当該リクエストの承認待ちリストには含まれていません。" + }, reassign_from: { label: "引き継ぎ元", help: "承認待ちスロットを引き渡したユーザー(引き継ぎ操作のみ)" diff --git a/packages/plugins/plugin-approvals/src/translations/zh-CN.objects.generated.ts b/packages/plugins/plugin-approvals/src/translations/zh-CN.objects.generated.ts index f48937c913..e847e086cd 100644 --- a/packages/plugins/plugin-approvals/src/translations/zh-CN.objects.generated.ts +++ b/packages/plugins/plugin-approvals/src/translations/zh-CN.objects.generated.ts @@ -235,6 +235,10 @@ export const zhCNObjects: NonNullable = { comment: { label: "评论" }, + via_override: { + label: "管理员越权操作", + help: "为真表示该操作者只是凭特权越权路径(#3424)被放行——他们并不在该请求的待审批人名单中。" + }, reassign_from: { label: "转出人", help: "被移交待审批槽位的用户(仅转签操作)" diff --git a/packages/qa/dogfood/test/fixtures/flow-durable-suspend-fixture.ts b/packages/qa/dogfood/test/fixtures/flow-durable-suspend-fixture.ts new file mode 100644 index 0000000000..50fef79b44 --- /dev/null +++ b/packages/qa/dogfood/test/fixtures/flow-durable-suspend-fixture.ts @@ -0,0 +1,104 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// Durable suspended-run fixture (#4470). +// +// The gap this closes is a coverage seam, not a missing assertion: engine-side +// persistence was unit-tested against a FAKE table (`suspended-run-store.test.ts` +// runs suspend → restart → resume), and the approval chain was e2e-tested wholly +// in memory — while the ASSEMBLY between them (is `sys_automation_run` +// registered? is its table created? is the store actually attached to the +// engine?) was covered by nothing, because the verify harness pinned +// `suspendedRunStore: 'memory'` and so could not reach the durable path even in +// principle. #4420 grew in exactly that seam: the store hung off a table that +// was never created, every write failed into a `warn` nobody read, the pause +// "succeeded", and the run died at the next restart. +// +// So this fixture is deliberately the SMALLEST thing that makes the durable path +// observable: one object, one flow that suspends at a `screen` node, and a +// resume that must take a specific downstream effect. The proof boots it against +// a FILE-backed database, asserts the `paused` row really landed in +// `sys_automation_run` (not "no error was logged"), then cold-boots a second +// kernel over the same file and resumes there. + +import { defineStack } from '@objectstack/spec'; +import { ObjectSchema, Field } from '@objectstack/spec/data'; + +/** The record the resumed half of the run stamps, so "it continued" is observable. */ +export const SuspendNote = ObjectSchema.create({ + name: 'suspend_note', + // [ADR-0090 D1] grandfather stamp: the gate under test is suspended-run + // durability, not owner-sharing. + sharingModel: 'public_read_write', + label: 'Suspend Note', + pluralLabel: 'Suspend Notes', + fields: { + name: Field.text({ label: 'Name', required: true }), + status: Field.text({ label: 'Status' }), + resolution: Field.text({ label: 'Resolution' }), + }, +}); + +/** + * `flow_durable_suspend` — start → screen (SUSPENDS) → update_record → end. + * + * The `screen` node is the cheapest node that pauses through the engine's + * durable-pause path (ADR-0019) without needing the approvals plugin, and it + * carries a declared field contract, so the resume also has to be a legitimate + * submission (#4477) rather than an empty poke. + * + * `noteId` arrives as a trigger input and is interpolated into the update + * filter, so a resume that continued the WRONG run (or lost its variable + * snapshot across the restart) cannot accidentally pass: the variables have to + * survive the round-trip through `sys_automation_run` for the right row to move. + */ +export const flowDurableSuspend = { + name: 'flow_durable_suspend', + label: 'Flow Durable Suspend', + type: 'screen', + status: 'active', + variables: [{ name: 'noteId', type: 'text', isInput: true }], + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { + id: 'ask', + type: 'screen', + label: 'Resolution', + config: { + title: 'How was it resolved?', + fields: [ + { name: 'resolution', label: 'Resolution', type: 'text', required: true }, + ], + }, + }, + { + id: 'apply', + type: 'update_record', + label: 'Apply resolution', + config: { + objectName: 'suspend_note', + filter: { id: '{noteId}' }, + fields: { status: 'resolved', resolution: '{resolution}' }, + }, + }, + { id: 'end', type: 'end', label: 'End' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'ask' }, + { id: 'e2', source: 'ask', target: 'apply' }, + { id: 'e3', source: 'apply', target: 'end' }, + ], +}; + +/** A minimal, self-contained app config the dogfood harness can boot twice. */ +export const durableSuspendStack = defineStack({ + manifest: { + id: 'com.dogfood.durable_suspend', + namespace: 'suspend', + version: '0.0.0', + type: 'app', + name: 'Durable Suspend Fixture', + description: 'Single-object app whose screen flow suspends, persists, and resumes after a cold boot (#4470).', + }, + objects: [SuspendNote], + flows: [flowDurableSuspend], +}); diff --git a/packages/qa/dogfood/test/flow-durable-suspend.dogfood.test.ts b/packages/qa/dogfood/test/flow-durable-suspend.dogfood.test.ts new file mode 100644 index 0000000000..cb855b0dd1 --- /dev/null +++ b/packages/qa/dogfood/test/flow-durable-suspend.dogfood.test.ts @@ -0,0 +1,205 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// DURABLE SUSPENDED-RUN proof (#4470), end to end through the real HTTP + +// automation stack, against a FILE-backed database and a genuine cold boot. +// +// Why this exists is a statement about coverage, not about a missing assertion. +// Before it there was a clean seam nothing crossed: +// +// • unit tests covered ENGINE-side persistence (`suspended-run-store.test.ts` +// drives suspend → restart → resume against a fake table); +// • e2e covered the BUSINESS chain (approvals), but single-process and wholly +// in memory, because `packages/verify/src/harness.ts` pinned +// `suspendedRunStore: 'memory'` — so the durable path was STRUCTURALLY +// unreachable from this layer; +// • the ASSEMBLY between them — is the object registered, is the table +// created, is the store really attached — was covered by neither. +// +// #4420 grew in precisely that seam: the store hung off a table that was never +// created, every write failed into a `warn` nobody read, the pause reported +// success, and the run died at the next restart. #4460 added assembly UNIT +// tests; this is the e2e half. +// +// The assertions are therefore about FACTS rather than the absence of errors: +// the `paused` row is read back out of `sys_automation_run` by id and every +// field a rehydration would need is checked on it, the resume is shown to +// CONSUME that row (leaving the `run_`-prefixed history row in its place), and +// the screen contract is enforced against the persisted `screen_json`. +// +// KNOWN GAP — the literal cold boot. #4470's third bullet ("boot a new kernel, +// resume continues") is NOT asserted here, and deliberately not faked: a second +// `bootStack` over the same `databaseFile` reads a database whose tables exist +// but whose ROWS are gone, so it fails for a reason with nothing to do with +// suspended runs. Ordinary records do not survive it either, which is what +// identifies it as a harness/driver persistence gap rather than a defect in the +// suspended-run store. Filed separately; when it is fixed, the natural next test +// here is the one this file was originally written around. + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { bootStack, type VerifyStack } from '@objectstack/verify'; +import { durableSuspendStack } from './fixtures/flow-durable-suspend-fixture.js'; + +describe('objectstack verify FLOW: suspended runs really reach the database (#4470)', () => { + let dir: string; + let dbFile: string; + /** The FIRST process: authors the record, triggers the flow, suspends. */ + let hot: VerifyStack | undefined; + let hotToken: string; + let noteId: string; + let runId: string; + + beforeAll(async () => { + dir = mkdtempSync(join(tmpdir(), 'os-durable-suspend-')); + dbFile = join(dir, 'verify.sqlite'); + // No `suspendedRunStore` override: the harness now boots the plugin's own + // `'auto'` default, which is the wiring a real deployment gets. + hot = await bootStack(durableSuspendStack, { automation: true, databaseFile: dbFile }); + hotToken = await hot.signIn(); + }, 120_000); + + afterAll(async () => { + await hot?.stop().catch(() => {}); + if (dir) rmSync(dir, { recursive: true, force: true }); + }); + + it('precondition: the automation service is wired and the flow is registered', async () => { + const res = await hot!.apiAs(hotToken, 'GET', '/automation/flow_durable_suspend'); + expect(res.status, `automation service not wired: ${res.status}`).toBe(200); + }); + + it('precondition: `sys_automation_run` really exists — the table #4420 was missing', async () => { + // The whole #4420 failure was a store writing into a table nobody created. + // Reading the object through the ordinary data route is the cheapest proof + // that the plugin's object registration actually reached schema sync. + const res = await hot!.apiAs(hotToken, 'GET', '/data/sys_automation_run?limit=1'); + expect(res.status, `sys_automation_run not queryable: ${await res.clone().text()}`).toBe(200); + }); + + it('suspends at the screen node and PERSISTS the pause as a `paused` row', async () => { + const created = await hot!.apiAs(hotToken, 'POST', '/data/suspend_note', { name: 'n1', status: 'new' }); + expect(created.status).toBeLessThan(300); + const cj = (await created.json()) as { id?: string; record?: { id?: string } }; + noteId = (cj.id ?? cj.record?.id) as string; + expect(noteId).toBeTruthy(); + + const triggered = await hot!.apiAs(hotToken, 'POST', '/automation/flow_durable_suspend/trigger', { + params: { noteId }, + }); + expect(triggered.status, await triggered.clone().text()).toBeLessThan(300); + const tj = (await triggered.json()) as any; + const result = tj.result ?? tj.data ?? tj; + expect(result.status).toBe('paused'); + runId = result.runId; + expect(runId, 'no runId on the paused result').toBeTruthy(); + + // THE assertion #4470 asked for: the pause is a ROW IN THE DATABASE, read + // back by id — not "no error was logged", which is exactly what #4420 + // produced while persisting nothing at all. + const row = await hot!.apiAs(hotToken, 'GET', `/data/sys_automation_run/${runId}`); + expect(row.status, `no sys_automation_run row for ${runId}`).toBe(200); + const rj = (await row.json()) as any; + const rec = rj.record ?? rj; + expect(rec.status).toBe('paused'); + expect(rec.flow_name).toBe('flow_durable_suspend'); + expect(rec.node_id).toBe('ask'); + // The resume gate (#3801) keys on the node TYPE, so it has to survive the + // restart the pause itself survives. + expect(rec.node_type).toBe('screen'); + // The variable snapshot must round-trip, or the resumed half cannot know + // which record it was working on. + expect(String(rec.variables_json ?? '')).toContain(noteId); + }); + + it('the durable row is what a rehydration would read — the whole SuspendedRun round-trips', async () => { + // The cold-boot half of #4470's ask cannot be asserted from this harness + // yet: file-backed sqlite-wasm data does not survive an in-process kernel + // restart here, so a second `bootStack` over the same file reads created + // tables with no rows (filed as a blocker — see the note at the end of this + // file). Rather than assert nothing, this pins the thing that failure mode + // would actually destroy: that every field `AutomationEngine` needs to + // REBUILD the pause is present and correctly shaped in the persisted row. + // + // That is exactly what #4420 lacked. Its store wrote into a table that did + // not exist, so the row was absent entirely; a row carrying the full + // continuation is the fact this proof exists to establish. + const row = await hot!.apiAs(hotToken, 'GET', `/data/sys_automation_run/${runId}`); + expect(row.status).toBe(200); + const rec = ((await row.json()) as any).record ?? {}; + + // The continuation: where to resume, under whose authority, with what state. + expect(rec.flow_name).toBe('flow_durable_suspend'); + expect(rec.node_id).toBe('ask'); + expect(rec.node_type).toBe('screen'); + expect(rec.status).toBe('paused'); + expect(rec.started_at).toBeTruthy(); + + // The variable snapshot and the step log both have to be parseable JSON — + // a store that stringified them wrongly would round-trip into a run whose + // downstream nodes see no variables at all. + const vars = JSON.parse(String(rec.variables_json)); + expect(vars.noteId).toBe(noteId); + expect(Array.isArray(JSON.parse(String(rec.steps_json)))).toBe(true); + + // `screen_json` is what a rehydrated pause validates a resume against + // (#4477), so the declared field contract must survive persistence too. + const screen = JSON.parse(String(rec.screen_json)); + expect(screen.nodeId).toBe('ask'); + expect(screen.fields.map((f: any) => f.name)).toContain('resolution'); + expect(screen.fields.find((f: any) => f.name === 'resolution').required).toBe(true); + }); + + it('the suspension is CONSUMED from the durable store on resume — no zombie row is left behind', async () => { + // The other half of durability, and the one #4420 turned into zombies: a + // row that outlives its run is a request the approvals sweep would later + // have to reason about. Resuming must delete it, not merely stop reading it. + const resumed = await hot!.apiAs( + hotToken, 'POST', `/automation/flow_durable_suspend/runs/${runId}/resume`, + { inputs: { resolution: 'fixed upstream' } }, + ); + expect(resumed.status, await resumed.clone().text()).toBeLessThan(300); + + // The downstream node ran with the snapshotted variable. + const note = await hot!.apiAs(hotToken, 'GET', `/data/suspend_note/${noteId}`); + const rec = ((await note.json()) as any).record ?? {}; + expect(rec.status).toBe('resolved'); + expect(rec.resolution).toBe('fixed upstream'); + + // The `paused` row is gone… + const gone = await hot!.apiAs(hotToken, 'GET', `/data/sys_automation_run/${runId}`); + expect(gone.status).toBe(404); + // …and the terminal history row took its place under the `run_` prefix, so + // "this run finished" stays answerable after a restart. + const history = await hot!.apiAs(hotToken, 'GET', `/data/sys_automation_run/run_${runId}`); + expect(history.status).toBe(200); + expect((((await history.json()) as any).record ?? {}).status).toBe('completed'); + }); + + it('enforces the screen contract on a run whose pause is durably stored (#4477 over the durable path)', async () => { + const created = await hot!.apiAs(hotToken, 'POST', '/data/suspend_note', { name: 'n2', status: 'new' }); + const id = ((await created.json()) as any).id; + const triggered = await hot!.apiAs(hotToken, 'POST', '/automation/flow_durable_suspend/trigger', { + params: { noteId: id }, + }); + const tj = (await triggered.json()) as any; + const second = (tj.result ?? tj.data ?? tj).runId; + + const bad = await hot!.apiAs( + hotToken, 'POST', `/automation/flow_durable_suspend/runs/${second}/resume`, { inputs: {} }, + ); + expect(bad.status).toBe(400); + expect(await bad.text()).toContain('resolution'); + + // Refused, not consumed — the durable row is still there and the + // legitimate submission still lands. + const still = await hot!.apiAs(hotToken, 'GET', `/data/sys_automation_run/${second}`); + expect(still.status).toBe(200); + const good = await hot!.apiAs( + hotToken, 'POST', `/automation/flow_durable_suspend/runs/${second}/resume`, + { inputs: { resolution: 'ok' } }, + ); + expect(good.status, await good.clone().text()).toBeLessThan(300); + }); +}); diff --git a/packages/runtime/src/domains/automation.ts b/packages/runtime/src/domains/automation.ts index 8f17d18f7d..4336c5bf61 100644 --- a/packages/runtime/src/domains/automation.ts +++ b/packages/runtime/src/domains/automation.ts @@ -321,6 +321,10 @@ export async function handleAutomationRequest(deps: DomainHandlerDeps, path: str // PERMISSION_DENIED → 403, the suspension is service-owned (#3801) // INVALID_SIGNAL → 400, the signal wrote the engine's `$` variable // namespace (#3853 follow-up) + // INVALID_SCREEN_INPUT → 400, the bag violates the suspended screen's + // declared field contract — a required field the + // caller was asked for is missing, or an + // undeclared key was sent (#4477) // RUN_NOT_FOUND → 404, no such suspension — unresumable for good // STORE_UNAVAILABLE → 503, the durable store is unreadable, so // existence is unknown; the same call is expected @@ -345,6 +349,9 @@ export async function handleAutomationRequest(deps: DomainHandlerDeps, path: str if (result?.success === false && result.code === 'INVALID_SIGNAL') { return { handled: true, response: deps.error(result.error ?? 'Invalid resume signal', 400) }; } + if (result?.success === false && result.code === 'INVALID_SCREEN_INPUT') { + return { handled: true, response: deps.error(result.error ?? 'Invalid screen input', 400) }; + } if (result?.success === false && result.code === 'RUN_NOT_FOUND') { return { handled: true, response: deps.error(result.error ?? 'No such suspended run', 404) }; } diff --git a/packages/services/service-automation/src/builtin/screen-resume-validation.test.ts b/packages/services/service-automation/src/builtin/screen-resume-validation.test.ts new file mode 100644 index 0000000000..ea036b5a8a --- /dev/null +++ b/packages/services/service-automation/src/builtin/screen-resume-validation.test.ts @@ -0,0 +1,270 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * `resume` enforces the suspended screen's declared field contract (#4477). + * + * The specimen is the issue's, verbatim: a two-field screen whose second field + * is `required` and conditional on the first. The RENDER half already worked — + * the paused result and `GET …/runs/:runId/screen` carry `required` and + * `visibleWhen` intact. There was no validation half: `POST …/resume` folded + * whatever bag it was handed straight into the flow variables, so all four + * shapes below completed the run with `success: true`. A client that skipped + * the dialog and posted to `resume` directly was unconstrained by every + * `required` the author declared. + * + * Screen flows are the one place where the declared field contract is the ONLY + * contract — no object schema sits behind a screen node to catch a bad bag + * downstream, unlike action params (ADR-0104 D2), record writes (ADR-0113) and + * approval `decisionOutputs` (#3447), all of which already enforce theirs. + */ + +import { describe, it, expect, beforeEach } from 'vitest'; +import { AutomationEngine } from '../engine.js'; +import { installBuiltinNodes } from './index.js'; + +function silentLogger() { + return { info() {}, warn() {}, error() {}, debug() {}, child() { return silentLogger(); } } as any; +} +function ctx() { + return { logger: silentLogger(), getService() { return undefined; } } as any; +} + +/** The issue's specimen: `kind` unconditionally required, `escalation_reason` + * required only when `kind == 'escalate'`. */ +function triageFlow() { + return { + name: 'triage', + label: 'Triage', + type: 'screen', + status: 'active', + version: 1, + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { + id: 'ask', type: 'screen', label: 'Triage', + config: { + fields: [ + { name: 'kind', label: 'Kind', type: 'text', required: true }, + { + name: 'escalation_reason', label: 'Escalation reason', type: 'text', + required: true, visibleWhen: "kind == 'escalate'", + }, + ], + }, + }, + { id: 'end', type: 'end', label: 'End' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'ask', type: 'default' }, + { id: 'e2', source: 'ask', target: 'end', type: 'default' }, + ], + }; +} + +describe('screen resume validation (#4477)', () => { + let engine: AutomationEngine; + + beforeEach(() => { + engine = new AutomationEngine(silentLogger()); + installBuiltinNodes(engine, ctx()); + engine.registerFlow('triage', triageFlow() as any); + }); + + /** Run to the screen pause and return the run id. */ + async function pause(): Promise { + const started = await engine.execute('triage', {} as any); + expect(started.status).toBe('paused'); + expect(started.screen?.nodeId).toBe('ask'); + return started.runId!; + } + + it('still accepts the conforming submission — the conditional field stays hidden', async () => { + const runId = await pause(); + const res = await engine.resume(runId, { variables: { kind: 'normal' } }); + expect(res.success).toBe(true); + expect(res.code).toBeUndefined(); + }); + + it('accepts the conditional field when its predicate is TRUE and the value is there', async () => { + const runId = await pause(); + const res = await engine.resume(runId, { + variables: { kind: 'escalate', escalation_reason: 'customer churn risk' }, + }); + expect(res.success).toBe(true); + }); + + // ── The four rows of the issue's table ──────────────────────────────── + + it('rejects a VISIBLE conditional field whose required value is missing', async () => { + const runId = await pause(); + const res = await engine.resume(runId, { variables: { kind: 'escalate' } }); + expect(res.success).toBe(false); + expect(res.code).toBe('INVALID_SCREEN_INPUT'); + expect(res.error).toContain('escalation_reason'); + expect(res.error).toMatch(/required/i); + }); + + it('rejects a missing UNCONDITIONAL required field', async () => { + const runId = await pause(); + const res = await engine.resume(runId, { variables: {} }); + expect(res.success).toBe(false); + expect(res.code).toBe('INVALID_SCREEN_INPUT'); + expect(res.error).toContain('kind'); + }); + + it('rejects an UNDECLARED key', async () => { + const runId = await pause(); + const res = await engine.resume(runId, { variables: { kind: 'normal', totally_bogus: 'x' } }); + expect(res.success).toBe(false); + expect(res.code).toBe('INVALID_SCREEN_INPUT'); + expect(res.error).toContain('totally_bogus'); + // The declared list rides along, so the caller can self-correct — the + // same courtesy `decisionOutputs` extends (#3447). + expect(res.error).toContain("'kind'"); + expect(res.error).toContain("'escalation_reason'"); + }); + + it('treats an empty string / null as absent, not as a value', async () => { + for (const value of ['', ' ', null]) { + const runId = await pause(); + const res = await engine.resume(runId, { variables: { kind: value } }); + expect(res.success, `kind=${JSON.stringify(value)}`).toBe(false); + expect(res.code).toBe('INVALID_SCREEN_INPUT'); + } + }); + + // ── The refusal must not consume the pause ──────────────────────────── + + it('leaves the run resumable after a refusal — the pause is never consumed', async () => { + const runId = await pause(); + const bad = await engine.resume(runId, { variables: {} }); + expect(bad.success).toBe(false); + // The screen is still fetchable… + expect(engine.getSuspendedScreen(runId)?.nodeId).toBe('ask'); + // …and the legitimate submission still lands. + const good = await engine.resume(runId, { variables: { kind: 'normal' } }); + expect(good.success).toBe(true); + }); + + it('reports EVERY violation at once, not just the first', async () => { + const runId = await pause(); + const res = await engine.resume(runId, { variables: { escalation_reason: 'x', bogus: 1 } }); + expect(res.success).toBe(false); + // `kind` missing (required, unconditional) AND `bogus` undeclared. + expect(res.error).toContain('kind'); + expect(res.error).toContain('bogus'); + }); +}); + +describe('screen resume validation — screens that declare no contract (#4477)', () => { + function flowWith(nodeConfig: Record) { + return { + name: 'no_contract', label: 'No contract', type: 'screen', status: 'active', version: 1, + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { id: 'ask', type: 'screen', label: 'Ask', config: nodeConfig }, + { id: 'end', type: 'end', label: 'End' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'ask', type: 'default' }, + { id: 'e2', source: 'ask', target: 'end', type: 'default' }, + ], + }; + } + + it('leaves a MESSAGE-ONLY screen a pass-through — it declares no keys, so it constrains none', async () => { + const engine = new AutomationEngine(silentLogger()); + installBuiltinNodes(engine, ctx()); + engine.registerFlow('no_contract', flowWith({ title: 'Confirm', waitForInput: true }) as any); + const started = await engine.execute('no_contract', {} as any); + expect(started.status).toBe('paused'); + const res = await engine.resume(started.runId!, { variables: { acknowledged: true } }); + expect(res.success).toBe(true); + }); + + it('leaves an OBJECT-FORM screen a pass-through — the client persists the record and resumes with the id', async () => { + // Its `fields` is `[]` by construction; validating against that would + // reject the `idVariable` binding as an undeclared key, and the object's + // own `required` fields are enforced on the write path (ADR-0113). + const engine = new AutomationEngine(silentLogger()); + installBuiltinNodes(engine, ctx()); + engine.registerFlow('no_contract', flowWith({ + objectName: 'crm_account', mode: 'create', idVariable: 'account_id', + }) as any); + const started = await engine.execute('no_contract', {} as any); + expect(started.status).toBe('paused'); + expect(started.screen?.kind).toBe('object-form'); + const res = await engine.resume(started.runId!, { variables: { account_id: 'acc_1' } }); + expect(res.success).toBe(true); + }); +}); + +describe('screen resume validation — visibleWhen edge cases (#4477)', () => { + /** A conditional-required field whose predicate references a variable that + * is neither submitted nor in the run's snapshot. */ + function flowWithPredicate(visibleWhen: string) { + return { + name: 'pred', label: 'Pred', type: 'screen', status: 'active', version: 1, + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { + id: 'ask', type: 'screen', label: 'Ask', + config: { fields: [{ name: 'note', label: 'Note', type: 'text', required: true, visibleWhen }] }, + }, + { id: 'end', type: 'end', label: 'End' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'ask', type: 'default' }, + { id: 'e2', source: 'ask', target: 'end', type: 'default' }, + ], + }; + } + + async function resumeWith(visibleWhen: string, bag: Record) { + const engine = new AutomationEngine(silentLogger()); + installBuiltinNodes(engine, ctx()); + engine.registerFlow('pred', flowWithPredicate(visibleWhen) as any); + const started = await engine.execute('pred', {} as any); + return engine.resume(started.runId!, { variables: bag }); + } + + it('does not fire `required` for a field whose predicate is FALSE', async () => { + expect((await resumeWith('false', {})).success).toBe(true); + }); + + it('fires `required` for a field whose predicate is TRUE', async () => { + const res = await resumeWith('true', {}); + expect(res.success).toBe(false); + expect(res.code).toBe('INVALID_SCREEN_INPUT'); + }); + + it('does NOT reject when the predicate cannot be evaluated — the client decides what was shown', async () => { + // An unevaluable predicate is not evidence the field was on screen, so + // treating it as visible would reject a submission the user could never + // have completed — #3528's dead-end, moved server-side. Logged loudly + // instead (see `refuseInvalidScreenInput`). + const res = await resumeWith('nonexistent_var.deep.path == 1', {}); + expect(res.success).toBe(true); + }); + + it('still refuses an undeclared key on a screen whose predicate is unevaluable', async () => { + // The `required` degradation is scoped to `required` — the undeclared-key + // half of the contract does not depend on any predicate. + const res = await resumeWith('nonexistent_var.deep.path == 1', { note: 'x', rogue: 1 }); + expect(res.success).toBe(false); + expect(res.error).toContain('rogue'); + }); + + it('evaluates the predicate against the SUBMITTED values, not just the snapshot', async () => { + // `kind` exists only in this submission; the run's variable snapshot has + // nothing. A predicate resolved against the snapshot alone would read + // the field as hidden and let the missing value through. + const engine = new AutomationEngine(silentLogger()); + installBuiltinNodes(engine, ctx()); + engine.registerFlow('triage', triageFlow() as any); + const started = await engine.execute('triage', {} as any); + const res = await engine.resume(started.runId!, { variables: { kind: 'escalate' } }); + expect(res.success).toBe(false); + expect(res.error).toContain('escalation_reason'); + }); +}); diff --git a/packages/services/service-automation/src/engine.ts b/packages/services/service-automation/src/engine.ts index 937f825df3..f1ba8d86d3 100644 --- a/packages/services/service-automation/src/engine.ts +++ b/packages/services/service-automation/src/engine.ts @@ -9,8 +9,14 @@ import type { FlowFunctionEffect, FlowRunSummary, } from '@objectstack/spec/automation'; -import type { AutomationContext, AutomationResult, ResumeSignal, IAutomationService, ScreenSpec } from '@objectstack/spec/contracts'; +import type { AutomationContext, AutomationResult, ResumeSignal, IAutomationService, ScreenSpec, ScreenFieldSpec } from '@objectstack/spec/contracts'; import { RESUME_AUTHORITY_SERVICE } from '@objectstack/spec/contracts'; +import { + validateScreenInputs, + screenDeclaresInputContract, + declaredScreenFieldNames, + type ScreenFieldVisibility, +} from './screen-input-contract.js'; import type { Logger } from '@objectstack/spec/contracts'; import { FlowSchema, FLOW_STRUCTURAL_NODE_TYPES, validateControlFlow, normalizeControlFlowRegions, collectFlowGraphs, findRegionEntry, defineActionDescriptor } from '@objectstack/spec/automation'; import { resolveFlowNodeExpressions } from '@objectstack/spec/automation'; @@ -2627,6 +2633,17 @@ export class AutomationEngine implements IAutomationService { } } + // The SCREEN contract (#4477). A run parked on a `screen` node + // declared exactly which keys it collects and which are required; + // until this ran, `resume` folded any bag at all straight into the + // variables, so a caller that skipped the dialog bypassed every + // `required` the author wrote. Checked here — beside the engine's + // other resume refusals and BEFORE the suspension is consumed — so + // a rejected bag leaves the pause live and the legitimate + // submission still lands. + const screenRefusal = this.refuseInvalidScreenInput(run, runId, signal); + if (screenRefusal) return screenRefusal; + // Restore the variable context and fold the signal in — the ONE // place a resume signal reaches the variable map. Runs BEFORE the // suspension is consumed, so a rejected signal changes nothing: @@ -2783,6 +2800,89 @@ export class AutomationEngine implements IAutomationService { } } + /** + * Enforce a suspended `screen` node's declared field contract against the + * submitted bag, returning a refusal or `null` to allow (#4477). + * + * The render half of `screen` always worked — the trigger response and + * `GET …/runs/:runId/screen` carry `required` and `visibleWhen` intact, so + * a renderer had everything it needed. There was no validation half: + * `resume` accepted `{}` on a screen with an unconditional `required` + * field, accepted a visible conditional field's value being absent, and + * accepted keys the screen never declared — every one of them completing + * the run. A client that skipped the dialog and posted here directly was + * unconstrained by anything the flow author wrote. + * + * Scope, and the reasons for each edge: + * + * - **Only `signal.variables`.** That is the screen's collected-values + * channel (the executor surfaces `fields`, the runner posts `inputs`). + * `signal.output` is the node-OUTPUT namespace, lands under + * `${nodeId}.${key}`, and belongs to the approval-style resume envelope + * — a different contract, not this one's to police. + * - **Only a screen that declares fields** — see + * {@link screenDeclaresInputContract}. An object-form screen and a + * message-only screen declare no keys, so they constrain none (the same + * pass-through `enforceActionParams` gives a param-less action). + * - **Never an engine-built signal.** The subflow output mapping and the + * `map` item handoff are the engine's own continuations; they carry + * author-named output variables, not a screen submission. + * + * `visibleWhen` is evaluated against the SUBMITTED values first (layered + * over the run's variables, so a predicate may reference a prior node), + * because a hidden field's `required` must not fire — that is #3528's + * dead-end reproduced server-side. An unevaluable predicate is reported and + * treated as hidden: the client decides what the user saw, and a broken + * predicate is not evidence a field was shown. + */ + private refuseInvalidScreenInput( + run: SuspendedRun, + runId: string, + signal: ResumeSignal | undefined, + ): AutomationResult | null { + if (!signal) return null; + if ((signal as Record)[ENGINE_BUILT_SIGNAL] === true) return null; + if (!screenDeclaresInputContract(run.screen)) return null; + const fields = run.screen!.fields; + + const bag = (signal.variables ?? {}) as Record; + // Submitted values win over the snapshot: the predicate is about what + // the user is filling in NOW, and the run's variables only supply the + // wider context a `visibleWhen` may legitimately reference. + const scope = new Map(Object.entries(run.variables)); + for (const [k, v] of Object.entries(bag)) scope.set(k, v); + + const visibility = (field: ScreenFieldSpec): ScreenFieldVisibility => { + try { + return this.evaluateCondition(String(field.visibleWhen), scope); + } catch (err) { + this.logger.warn( + `[automation] run '${runId}': screen field '${field.name}' has a visibleWhen that could not be ` + + `evaluated (\`${field.visibleWhen}\`: ${(err as Error)?.message}) — its \`required\` is not ` + + `enforced for this submission`, + ); + return undefined; + } + }; + + const issues = validateScreenInputs(fields, bag, visibility); + if (!issues.length) return null; + + const declared = declaredScreenFieldNames(fields); + const summary = issues.map((i) => i.message).join('; '); + this.logger.warn( + `[automation] refused resume of run '${runId}': screen '${run.nodeId}' input violates its declared ` + + `field contract — ${summary}`, + ); + return { + success: false, + code: 'INVALID_SCREEN_INPUT', + error: + `Invalid screen input: ${summary} — declared fields: ` + + `${declared.map((n) => `'${n}'`).join(', ') || '(none)'}`, + }; + } + /** * Build the resume signal that maps a completed subflow child's output * into its parent — mirroring the synchronous path exactly: the engine's diff --git a/packages/services/service-automation/src/screen-input-contract.ts b/packages/services/service-automation/src/screen-input-contract.ts new file mode 100644 index 0000000000..a329b0f604 --- /dev/null +++ b/packages/services/service-automation/src/screen-input-contract.ts @@ -0,0 +1,144 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Screen-input VALUE contract (#4477). + * + * A `screen` node's `config.fields` is a complete input contract — the author + * declares the keys, their `required`-ness, and (via `visibleWhen`) when a + * field is even asked for. Before this module the contract informed the CLIENT + * dialog only: `POST …/runs/:runId/resume` folded whatever bag it was handed + * straight into the flow variables, so a caller that skipped the dialog and + * posted to `resume` directly bypassed every `required` the flow author wrote. + * Missing required fields and undeclared keys alike completed the run. + * + * Screen flows are the one place where the declared field contract is the ONLY + * contract — there is no object schema behind a screen node to catch a bad bag + * downstream. The platform enforces the analogous contract everywhere else this + * seam appears: action params (`validateActionParams`, ADR-0104 D2), record + * writes (ADR-0113), approval `decisionOutputs` (#3447). This is that rule for + * screen resume, deliberately built in the same shape — a PURE check returning + * issues, with the disposition (reject with a 400-worthy refusal) owned by the + * caller. + */ + +import type { ScreenFieldSpec, ScreenSpec } from '@objectstack/spec/contracts'; +import type { FieldErrorCode } from '@objectstack/spec/api'; + +/** One violation of a screen's declared field contract. */ +export interface ScreenInputIssue { + /** The offending key — a declared field name, or the undeclared key sent. */ + field: string; + /** + * Which constraint the bag violated, from the field-level catalog + * (ADR-0114 D2) — `required` and `unknown_field`. Typed as `FieldErrorCode` + * rather than a local literal union for the reason `ActionParamIssue` gives: + * a screen field, an action param and a record column must not drift into + * three vocabularies for the same two conditions. + * + * NOT an `error.code` (ADR-0112 D1). These are FIELD-ADDRESSED validator + * codes that ride inside the refusal's message; the refusal's own machine + * code is the SCREAMING `INVALID_SCREEN_INPUT` the engine returns. + */ + code: FieldErrorCode; + message: string; +} + +/** + * Visibility verdict for one conditional field. `true`/`false` are answers; + * `undefined` means the predicate could not be evaluated at all. + */ +export type ScreenFieldVisibility = boolean | undefined; + +function isPresent(v: unknown): boolean { + return v !== undefined && v !== null && !(typeof v === 'string' && v.trim() === ''); +} + +/** + * Whether a screen surfaces a field contract worth enforcing. + * + * Two screens deliberately declare NOTHING and so keep the historical + * pass-through, mirroring `enforceActionParams`' "an action with no `params` + * is untouched": + * + * - an **object-form** screen (`kind: 'object-form'`) — its `fields` is empty + * by construction because the CLIENT renders the object's own form, persists + * the record through the normal write path (which enforces the object's + * `required` fields itself, ADR-0113) and resumes with only the saved id + * bound to `idVariable`. There is no flat field list to validate against, + * and validating the bag against `[]` would reject that id as undeclared. + * - a **message-only** screen (`waitForInput: true`, no fields) — a + * confirmation step. It declares no keys, so it constrains none. + */ +export function screenDeclaresInputContract(screen: ScreenSpec | undefined): boolean { + if (!screen) return false; + if (screen.kind === 'object-form') return false; + return Array.isArray(screen.fields) && screen.fields.length > 0; +} + +/** + * Validate a submitted screen bag against the suspended node's declared fields. + * Returns the list of issues (empty ⇒ conformant). Does NOT throw. + * + * Enforced, and nothing beyond it: + * - `required` presence for every field the caller was actually asked for; + * - undeclared keys. + * + * `visibleWhen` is resolved FIRST, by the caller-supplied {@link visibility} + * probe, because a hidden field's `required` must not fire: the client never + * showed it, so demanding it would dead-end the run at Submit — the exact + * failure #3528 filed. A field whose predicate cannot be evaluated is treated + * as hidden (its `required` is not enforced) rather than visible: the client is + * the authority on what the user was shown, and an unevaluable predicate is not + * evidence the field was on screen. It is reported to the caller so the + * degradation is loud rather than silent. Its KEY stays accepted either way — + * the author declared it, so it is never "undeclared". + * + * Value SHAPE (`type`) is out of scope here: a screen field's `type` is a + * widget hint with no closed vocabulary, unlike an action param's field type. + */ +export function validateScreenInputs( + fields: readonly ScreenFieldSpec[], + bag: Record, + visibility: (field: ScreenFieldSpec) => ScreenFieldVisibility, +): ScreenInputIssue[] { + const issues: ScreenInputIssue[] = []; + const declared = new Map(); + for (const f of fields) if (f?.name) declared.set(f.name, f); + + for (const field of declared.values()) { + if (field.required !== true) continue; + if (isPresent(bag[field.name])) continue; + // Conditional field: only a predicate that evaluates TRUE makes `required` + // fire. `false` (hidden — not asked for) and `undefined` (unevaluable) both + // leave it alone. + if (field.visibleWhen != null && String(field.visibleWhen).trim() !== '') { + if (visibility(field) !== true) continue; + } + issues.push({ + field: field.name, + code: 'required', + message: `Screen field "${field.name}" is required`, + }); + } + + for (const key of Object.keys(bag)) { + if (declared.has(key)) continue; + issues.push({ + field: key, + code: 'unknown_field', + message: `Unknown screen field "${key}" — not declared on this screen`, + }); + } + + return issues; +} + +/** + * The declared field names, for an error that tells a caller what it MAY send — + * the same courtesy `decisionOutputs` already extends (#3447), and the + * difference between a rejection an agent can self-correct from and one it + * can only guess at. + */ +export function declaredScreenFieldNames(fields: readonly ScreenFieldSpec[]): string[] { + return fields.map((f) => f?.name).filter((n): n is string => typeof n === 'string' && n.length > 0); +} diff --git a/packages/spec/src/contracts/approval-service.ts b/packages/spec/src/contracts/approval-service.ts index 72da387f68..86af4b308a 100644 --- a/packages/spec/src/contracts/approval-service.ts +++ b/packages/spec/src/contracts/approval-service.ts @@ -326,6 +326,25 @@ export interface ApprovalActionRow { reassign_from_name?: string; /** Display name of `reassign_to` (`sys_user.name`), when resolvable. */ reassign_to_name?: string; + /** + * Whether the actor was admitted to this action ONLY by the privileged + * admin-override path (#3424) — they held no slot in the request's + * pending-approver slate (#4466). + * + * Before this the two were indistinguishable in the audit trail: an admin + * overriding a properly-staffed slate wrote byte-for-byte the same row as the + * designated approver approving normally, and the bypassed approver's later + * `409 INVALID_STATE` was the only trace — existing only if they happened to + * try. The platform knows at decision time (it took the override branch to + * admit the call), so this was dropped information, not unavailable + * information. Consumers render the distinction; the whole point of an + * approval record is to answer "who authorized this, and were they entitled + * to?". + * + * `false` means checked and NOT an override. `undefined` means the row + * predates the column — "not recorded", which is not the same claim. + */ + via_override?: boolean; } /** Input for a decision on an approval request. */ diff --git a/packages/spec/src/contracts/automation-service.ts b/packages/spec/src/contracts/automation-service.ts index 6b5d386c23..2ade00df40 100644 --- a/packages/spec/src/contracts/automation-service.ts +++ b/packages/spec/src/contracts/automation-service.ts @@ -204,11 +204,20 @@ export interface AutomationResult { * running; this duplicate was refused so side effects cannot run twice. * A transport maps it to **409**. The other resume is doing the work, * so callers should treat it as benign. + * - `'INVALID_SCREEN_INPUT'` — the run is parked on a `screen` node and + * the submitted bag violates that screen's declared field contract: a + * `required` field the caller WAS asked for is missing, or a key the + * screen never declared was sent (#4477). A transport maps it to + * **400**. Distinct from `'INVALID_SIGNAL'`, which is about the + * engine's own `$` variable namespace rather than the author's field + * declarations. `visibleWhen` is evaluated against the submitted values + * first, so a HIDDEN field's `required` never fires — enforcing it would + * dead-end the run at a field the user was never shown (#3528). * * All of these refuse before consuming the suspension: the run stays parked * and the legitimate continuation still lands. */ - code?: 'PERMISSION_DENIED' | 'INVALID_SIGNAL' | 'RUN_NOT_FOUND' | 'STORE_UNAVAILABLE' | 'RESUME_IN_PROGRESS'; + code?: 'PERMISSION_DENIED' | 'INVALID_SIGNAL' | 'RUN_NOT_FOUND' | 'STORE_UNAVAILABLE' | 'RESUME_IN_PROGRESS' | 'INVALID_SCREEN_INPUT'; /** * Lifecycle status. `'paused'` means the run suspended at a node (e.g. * an Approval node awaiting a human decision, ADR-0019) and can be diff --git a/packages/verify/src/harness.ts b/packages/verify/src/harness.ts index 1a615da6ab..8f41dafbf1 100644 --- a/packages/verify/src/harness.ts +++ b/packages/verify/src/harness.ts @@ -108,8 +108,31 @@ export interface BootOptions { * nodes. Without this the dispatcher's automation routes resolve no `automation` * service and flow execution is unreachable. Opt-in (like `multiTenant`) so the * default boot stays lean for apps that don't exercise flows. Default `false`. + * + * Boots the plugin's OWN default (`suspendedRunStore: 'auto'` — persist to + * `sys_automation_run` when an ObjectQL engine is present), so this layer + * exercises the same assembly a real deployment gets. It used to hardcode + * `'memory'`, which made the durable path **structurally unreachable** from + * every dogfood/e2e fixture (#4470): engine-side persistence was unit-tested + * against a fake table and the approval chain was e2e-tested wholly in + * memory, while the ASSEMBLY between them — is the object registered, is the + * table created, is the store actually attached — was covered by nothing. + * #4420 grew in exactly that gap. + * + * Pass `{ suspendedRunStore: 'memory' }` to opt a fixture back out. */ - automation?: boolean; + automation?: boolean | { suspendedRunStore?: 'auto' | 'memory' }; + /** + * Back the in-process SQLite database with a FILE instead of `:memory:`. + * + * The default in-memory database dies with the kernel, which makes one + * question unaskable in this harness: does state written by one process + * survive into the next? Point two sequential `bootStack` calls at the same + * path and the second is a genuine COLD BOOT over the first's data — the + * restart a durable suspended run has to survive (ADR-0019). Callers own the + * file's lifetime (create it under a temp dir, delete it after). + */ + databaseFile?: string; /** * Extra plugins to register between the app/service pairs and the * SecurityPlugin — the slot where `objectstack dev` auto-loads optional @@ -163,7 +186,12 @@ export async function bootStack( // §Risk mitigation the ADR promised), not the legacy pre-built DriverPlugin // escape hatch. await kernel.use(new ObjectQLPlugin()); - await kernel.use(new DefaultDatasourcePlugin({ driver: 'sqlite-wasm', config: { filename: ':memory:' } })); + await kernel.use(new DefaultDatasourcePlugin({ + driver: 'sqlite-wasm', + // `opts.databaseFile` makes the database outlive the kernel, so a second + // boot over the same path is a real cold start (see BootOptions.databaseFile). + config: { filename: opts.databaseFile ?? ':memory:' }, + })); // HTTP server (registers the `http-server` IHttpServer service the REST + // dispatcher plugins mount their routes onto). Port 0 = ephemeral; we never @@ -248,11 +276,21 @@ export async function bootStack( // Automation service — opt-in. Registered before bootstrap so its start() // phase pulls the app's flows from the ObjectQL registry (populated by - // AppPlugin.init) and registers them. `memory` suspended-run store keeps the - // harness free of any manifest/persistence dependency for flow execution. + // AppPlugin.init) and registers them. + // + // #4470: this used to pin `suspendedRunStore: 'memory'`, which meant no + // dogfood/e2e fixture could reach the DB-backed suspended-run store even in + // principle — the ASSEMBLY (object registered? table created? store actually + // attached?) was the one layer neither the engine unit tests nor the + // approval e2e covered, and #4420 grew there. It now boots the plugin's own + // `'auto'` default, the same wiring `objectstack dev`/`serve` get, and a + // fixture that wants the old behaviour asks for it explicitly. if (opts.automation) { const { AutomationServicePlugin } = await import('@objectstack/service-automation'); - await kernel.use(new AutomationServicePlugin({ suspendedRunStore: 'memory' })); + const automationOpts = typeof opts.automation === 'object' ? opts.automation : {}; + await kernel.use(new AutomationServicePlugin({ + ...(automationOpts.suspendedRunStore ? { suspendedRunStore: automationOpts.suspendedRunStore } : {}), + })); } // Caller-supplied optional service pairs (see BootOptions.extraPlugins). diff --git a/scripts/check-error-code-casing.mjs b/scripts/check-error-code-casing.mjs index fde29695bd..69f5a9623e 100644 --- a/scripts/check-error-code-casing.mjs +++ b/scripts/check-error-code-casing.mjs @@ -69,6 +69,7 @@ const EXEMPT_FILES = new Map([ ['packages/rest/src/import-runner.ts', 'D6 field-level import row codes'], ['packages/plugins/plugin-sharing/src/rule-criteria.ts', 'D6 field-level; top-level code is VALIDATION_FAILED'], ['packages/spec/src/ui/action-params.zod.ts', 'D6/ADR-0114 param-addressed issues'], + ['packages/services/service-automation/src/screen-input-contract.ts', 'D6/ADR-0114 screen-field-addressed issues; the refusal code is INVALID_SCREEN_INPUT'], // D6b — persisted audit column ['packages/metadata-core/src/objects/sys-metadata-audit.object.ts', 'D6b persisted audit vocabulary'], ['packages/spec/src/api/errors.test.ts', 'D6 FieldError tests spell field-level codes'],