diff --git a/.changeset/truthful-stranded-decision-envelope.md b/.changeset/truthful-stranded-decision-envelope.md new file mode 100644 index 0000000000..1f8a3facda --- /dev/null +++ b/.changeset/truthful-stranded-decision-envelope.md @@ -0,0 +1,17 @@ +--- +"@objectstack/types": minor +"@objectstack/plugin-approvals": minor +"@objectstack/rest": minor +--- + +An approval decision that lands while its flow run strands now says so in fields, not only in prose. + +`POST /api/v1/approvals/requests/{id}/reject` — and its sibling decision doors — could produce three coexisting outcomes from one call: the caller read HTTP 500, the request row **was** in its terminal status and had left the pending inbox, and the workflow run was stranded. A caller reading 500 has one honest inference available — "the rejection did not happen" — and it was the wrong one, so scripts and operators retried or escalated against a decision that was already durable. The only carrier of the truth was English prose in `error`, so finding the affected run meant regexing a run id out of a sentence, and nothing said whether that run could be repaired at all. + +The 500 stays. A recorded decision whose flow never advances is still a failure and is still reported as one; the door does not become atomic and no decision is ever rolled back. What changed is that it stops discarding what the engine already said: + +- **The `RESUME_FAILED` body gains four fields**, additively — `finalized` (always `true`: the decision stands), `decision`, `runId`, and `repairable`. Existing consumers see the same `code`, the same `error` and the same status. +- **`repairable` carries the engine's own discriminator** — `AutomationResult.status === 'stranded'`, the state stamped on exactly the exit that journals a repair snapshot. `false` is the answer for every other failure, including a lost run: absence of the signal is not repairability, and a repair verb that would refuse is worse than no promise. +- **`serviceResume` carries `status`** through to the door. It previously read only `success` / `code` / `error`, and the stranded exit reports a `status` and no `code` at all — so the platform's own repairability signal died one line before the envelope was built. + +`@objectstack/types` gains `strandedDecisionFailure` / `strandedDecisionDetails` and the `StrandedDecisionDetails` type — the constructor and its recogniser in one module, so the producing service and the REST door cannot drift. A `RESUME_FAILED` raised without that carrier answers exactly the body it always did; the door never synthesises the envelope. diff --git a/content/docs/permissions/system-context.mdx b/content/docs/permissions/system-context.mdx index d00b4d8e25..6f8740e51b 100644 --- a/content/docs/permissions/system-context.mdx +++ b/content/docs/permissions/system-context.mdx @@ -64,7 +64,7 @@ not on any flag. ## How the flag is set `isSystem` is **server-constructed and never client-supplied**. Inbound HTTP -cannot set it (`packages/rest/src/rest-server.ts:1548`, `:1577`), and neither +cannot set it (`packages/rest/src/rest-server.ts:1553`, `:1582`), and neither can an action body (`packages/runtime/src/domains/actions.ts:414`). It is written by internal callers only, as an option on the engine call: @@ -103,7 +103,7 @@ that silently does not happen. | 14 | MCP stdio bridge skips the object API-exposure gate | mcp | Get: the bridge reaches objects whose `apiEnabled` / `apiMethods` would refuse an external caller | `stdio-data-bridge.ts:246` | | 15 | **Read-audit rows are not written** | plugin-audit | Lose: the "a person opened this record" trail. `sudo()` keeps the caller's `userId`, so this flag is the only thing separating a human read from a platform one | `read-audit.ts:556` | | 16 | Approval snapshot payload redaction skipped | plugin-approvals | Get: the whole snapshot on `find` / `findOne` — the audit/replay channel. Lose: field-visibility redaction over approval payloads | `payload-redaction-middleware.ts:115` | -| 17 | REST anonymous-deny seam satisfied | rest | Get: `enforceAuth` passes with no `userId`. Not reachable from the wire — `isSystem` is never set on an inbound request | `rest-server.ts:1580` | +| 17 | REST anonymous-deny seam satisfied | rest | Get: `enforceAuth` passes with no `userId`. Not reachable from the wire — `isSystem` is never set on an inbound request | `rest-server.ts:1585` | ### 2. Write pipeline and data integrity @@ -145,7 +145,7 @@ The largest single consumer — **17 of the 106 sites**. |:--|:---|:---|:---|:---| | 40 | **Approval record lock released** — a locked record is writable | plugin-approvals | Get: engine self-writes (the status mirror) pass. Lose: the lock that stops edits while an approval is live. Note there is deliberately **no admin exemption** here — only `isSystem` | `lifecycle-hooks.ts:347` | | 41 | Delegation write guard bypassed | plugin-approvals | Get: service / seed / import may write delegation rows naming another delegator | `lifecycle-hooks.ts:570` | -| 42 | Approval actor / submitter / pending-approver checks bypassed (8 sites) | plugin-approvals | Get: approve, reject, recall, reassign without being a pending approver or the submitter | `plugin-approvals/src/approval-service.ts:963`, `:1072`, `:3196`, `:3342`, `:3509`, `:3580`, `:3769`, `:3809` | +| 42 | Approval actor / submitter / pending-approver checks bypassed (8 sites) | plugin-approvals | Get: approve, reject, recall, reassign without being a pending approver or the submitter | `plugin-approvals/src/approval-service.ts:963`, `:1072`, `:3248`, `:3396`, `:3564`, `:3635`, `:3824`, `:3864` | | 43 | Saved-report ownership is **assignable**, and an update may reassign it | plugin-reports | Get: `ownerId` from input is honoured. A non-system caller always owns what it creates and can never reassign | `plugin-reports/src/report-service.ts:404`, `:425` | | 44 | Saved-report access / export / mutation gates bypassed | plugin-reports | Get: read, bulk-export and overwrite any report | `plugin-reports/src/report-service.ts:343`, `:372`, `:447`, `:684` | | 45 | Attachment access hooks return early (insert + update + delete, and the read AST) | service-storage | Lose: attachment visibility scoping | `attachment-access-hooks.ts:300`, `:349`, `:448`, `:524` | @@ -158,7 +158,7 @@ The largest single consumer — **17 of the 106 sites**. |:--|:---|:---|:---|:---| | 48 | Object API-exposure gate bypassed (`apiEnabled` / `apiMethods`) | runtime | Get: internal self-writes ignore exposure declarations — these govern **external** exposure, not engine self-writes | `action-execution.ts:138` | | 49 | Action `requiredPermissions` bypassed | runtime | Get: engine self-invocation runs any action | `action-execution.ts:401` | -| 50 | `manage_metadata` bypassed on metadata writes | runtime, rest | Get: schema writes without the capability | `domains/meta.ts:471`, `:874`, `rest-server.ts:4967`, `:6381`, `:6629`, `:7060`, `:7253` | +| 50 | `manage_metadata` bypassed on metadata writes | runtime, rest | Get: schema writes without the capability | `domains/meta.ts:471`, `:874`, `rest-server.ts:4972`, `:6386`, `:6634`, `:7065`, `:7258` | | 51 | The shared metadata-write verdict itself returns `allowed` | metadata-core | Get: the one function all of row 50's doors consult answers yes before any capability is examined | `meta-write-capability.ts:134` | | 52 | Anonymous-deny seam satisfied on the domain dispatchers and the package/federation routes | runtime, rest | Get: passes with no `userId` | `domains/actions.ts:421`, `domains/ai.ts:60`, `domains/automation.ts:989`, `domains/meta.ts:232`, `domains/security.ts:78`, `domains/packages.ts:422`, `external-datasource-routes.ts:302`, `package-routes.ts:97` | | 53 | MCP principal check satisfied | runtime | Get: MCP surface reachable with no user | `domains/mcp.ts:61` | @@ -199,7 +199,7 @@ assuming `isSystem` covers it is a documented source of bugs. | "It preserves a supplied `updated_at` / `updated_by`" | **No.** That is `preserveAudit`, a separate opt-in — and an UPDATE-path exemption only | `field.zod.ts:1580` (#3493 / #6640) | | "It stamps `created_by`" | **No.** Audit stamping reads `userId` from the context. A user-less system write stamps nothing — that is today's behaviour, not an error | `runtime-identity.ts:280`–`281` | | "It bypasses every guard" | **No.** The last-admin guard applies to **every** context, `isSystem` included — the deprovision path that actually locks an org out is the system one | `last-admin-guard.ts:299` | -| "A client can request it" | **No.** Never settable from inbound HTTP or from an action body | `rest-server.ts:1548`, `:1577`; `domains/actions.ts:414` | +| "A client can request it" | **No.** Never settable from inbound HTTP or from an action body | `rest-server.ts:1553`, `:1582`; `domains/actions.ts:414` | --- diff --git a/packages/plugins/plugin-approvals/src/approval-service.ts b/packages/plugins/plugin-approvals/src/approval-service.ts index 8b10e70d22..aed7ecacea 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.ts @@ -18,7 +18,7 @@ import { ExpressionEngine, collectCelRootIdentifiers } from '@objectstack/formul // writer-local re-derivation here was rejected by name (Option B): it would be // a third answer to a question the codebase already answered two ways. import { createRecordOrganizationResolver, type RecordOrganizationResolver } from '@objectstack/metadata-core'; -import { keysetWalk } from '@objectstack/types'; +import { keysetWalk, strandedDecisionFailure } from '@objectstack/types'; import { ADMIN_FULL_ACCESS, ORGANIZATION_ADMIN_GRANTS, @@ -2665,15 +2665,17 @@ export class ApprovalService implements IApprovalService { signal: { output?: Record; branchLabel?: string }, ): Promise { const result = await this.automation!.resume!(runId, { ...signal, [RESUME_AUTHORITY_SERVICE]: true }); - const reported = result as { success?: boolean; code?: string; error?: string } | undefined; + const reported = result as + { success?: boolean; code?: string; error?: string; status?: string } | undefined; // Only an explicit `success: false` is a failure. An engine (or a test // double) that returns nothing is reporting nothing, and has always meant // "it ran". if (reported && typeof reported === 'object' && reported.success === false) { const err = new Error( `resume of run '${runId}' failed${reported.code ? ` [${reported.code}]` : ''}: ${reported.error ?? 'unknown error'}`, - ) as Error & { resumeCode?: string }; + ) as Error & { resumeCode?: string; resumeStatus?: string }; err.resumeCode = reported.code; + err.resumeStatus = reported.status; throw err; } } @@ -2683,6 +2685,23 @@ export class ApprovalService implements IApprovalService { return (err as { resumeCode?: string } | undefined)?.resumeCode; } + /** + * The engine's own run-state discriminator behind a {@link serviceResume} + * rejection — `AutomationResult.status` — if the engine reported one + * (#13807). + * + * Read as a SIBLING of {@link resumeCodeOf}, never as a substitute: the two + * answer different questions and the stranded exit proves they are not + * interchangeable. It reports `status: 'stranded'` and **no `code` at all** + * (`service-automation` `engine.ts`, the resume catch arm), so a door that + * reads only the code sees an unnamed failure and cannot tell a repairable + * strand from a dead run — which is how the platform's own repairability + * signal had a producer and zero consumers until this call site. + */ + private static resumeStatusOf(err: unknown): string | undefined { + return (err as { resumeStatus?: string } | undefined)?.resumeStatus; + } + /** * Refuse an operation whose whole point is to advance a flow run when that * run no longer exists — BEFORE anything is written down (#4420). @@ -2788,14 +2807,38 @@ export class ApprovalService implements IApprovalService { * which cannot throw without breaking every standalone deployment — it * reports through `resumeError` instead. * + * ## The throw is truthful, not merely loud (#13807) + * + * Maintainer ruling 2026-09-04 (decision batch #37, option B): this door + * KEEPS its status code — the effect landing while the run strands is still + * a failure and must still be reported as one — and stops discarding what + * the engine said. ⛔ Not "return 200", which the card forbids; ⛔ not + * atomic, because rolling a real human decision back is excluded by the + * #13937 shape-4 ruling, which binds this door's own writes too (a machine + * that re-armed strandings by itself would re-run the node that threw, + * forever, with nobody deciding it should). + * + * So the error carries {@link StrandedDecisionDetails} beside its prose: + * `finalized` (the decision stands), `decision`, `runId`, and `repairable` + * derived from the engine's `'stranded'` discriminator. Before this a caller + * had a 500 and a sentence — and 500 alone reads as "the rejection did not + * happen", which is the misreading that makes a caller retry or escalate + * against a decision that IS durable. + * * @param what - how the recorded outcome reads in the error, e.g. * `"the approve decision"`. + * @param decision - the outcome label for the machine-readable envelope + * (`'approve'` / `'reject'` / `'revise'` / `'resubmit'`). Passed + * explicitly rather than parsed back out of `what` or the signal: the + * prose is for humans and `output` is the flow's, and neither is a place + * to keep a wire value. */ private async resumeRecordedOutcome( runId: string, requestId: string, what: string, signal: { output?: Record; branchLabel?: string }, + decision: string, ): Promise<{ resumed: boolean; resumeError?: string }> { const missing = this.missingRunCapability(runId, requestId, what, 'resume'); if (missing) return { resumed: false, resumeError: missing }; @@ -2810,12 +2853,20 @@ export class ApprovalService implements IApprovalService { }); return { resumed: false, resumeError: reason }; } + // #13807: the engine's own discriminator decides `repairable`, never + // this door and never the message text. `'stranded'` is the ONE exit + // that journalled a repair snapshot, so it is the one exit an operator + // can act on; every other failure (a lost run, an engine too old to + // report a status) is honestly `false`. + const status = ApprovalService.resumeStatusOf(err); + const repairable = status === 'stranded'; this.logger?.error?.('[approvals] resume failed — the run is stranded', { - request: requestId, run: runId, outcome: what, error: reason, + request: requestId, run: runId, outcome: what, error: reason, status, repairable, }); - throw new Error( + throw strandedDecisionFailure( `RESUME_FAILED: ${what} was recorded on request ${requestId}, but its flow run '${runId}' ` + `could not be resumed and is now stranded: ${reason}`, + { finalized: true, decision, runId, repairable }, ); } } @@ -2862,6 +2913,7 @@ export class ApprovalService implements IApprovalService { // whitelist already rejects them; this is defense in depth). output: { ...(result.outputs ?? {}), decision: result.decision, requestId }, }, + result.decision, ); resumed = outcome.resumed; resumeError = outcome.resumeError; @@ -3254,6 +3306,7 @@ export class ApprovalService implements IApprovalService { branchLabel: APPROVAL_BRANCH_LABELS.reject, output: { decision: 'reject', autoRejected: true, requestId }, }, + 'reject', ); resumed = outcome.resumed; resumeError = outcome.resumeError; @@ -3295,6 +3348,7 @@ export class ApprovalService implements IApprovalService { branchLabel: APPROVAL_BRANCH_LABELS.revise, output: { decision: 'revise', requestId }, }, + 'revise', ); resumed = outcome.resumed; resumeError = outcome.resumeError; @@ -3384,6 +3438,7 @@ export class ApprovalService implements IApprovalService { branchLabel: APPROVAL_BRANCH_LABELS.resubmit, output: { resubmitted: true, requestId }, }, + 'resubmit', ); resumed = outcome.resumed; resumeError = outcome.resumeError; diff --git a/packages/plugins/plugin-approvals/src/decision-strand-envelope.test.ts b/packages/plugins/plugin-approvals/src/decision-strand-envelope.test.ts new file mode 100644 index 0000000000..db0cedd5be --- /dev/null +++ b/packages/plugins/plugin-approvals/src/decision-strand-envelope.test.ts @@ -0,0 +1,311 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * A recorded decision whose run strands says so in FIELDS, not only in prose + * (#13807 — maintainer ruling 2026-09-04, decision batch #37, option B). + * + * ## The reported defect + * + * One `POST /api/v1/approvals/requests/{id}/reject` produced three coexisting + * outcomes: the caller read HTTP 500, the request row WAS `rejected` and had + * left the pending inbox, and the workflow run was stranded. A caller — human, + * script, or agent — reads 500 as "the rejection did not happen" and retries + * or escalates. It did happen. + * + * ## What the ruling did and did NOT change + * + * ⛔ The status code does not move. Returning 200 is named forbidden on the + * card and the ruling upholds it: the effect landing while the run strands is + * still a failure. ⛔ The door does not become atomic either — the #13937 + * shape-4 ruling binds this door's own writes too, so there is no + * approvals-side revert of the mirrored status, no automatic restore, and no + * discarding a decision a person actually made. The contract + * (`ApprovalDecisionResult`) declares the throw-rather-than-half-state + * deliberate, and this makes that throw TRUTHFUL rather than replacing it. + * + * What changed is that the door stops discarding what the engine said. The + * engine stamps `AutomationResult.status: 'stranded'` on exactly the exit that + * journals a repair snapshot — and until this change that member had a + * producer and ZERO consumers, because `serviceResume` read only + * `success` / `code` / `error` and dropped it one line before the envelope was + * built. The literal object the door receives on this exit carries + * `status: 'stranded'` and NO `code` at all, so a door reading only the code + * sees an unnamed failure. + * + * ## The three pins the ruling asked for + * + * 1. the three-outcome reproduction, asserting the new fields; + * 2. a healthy decision, unchanged; + * 3. `'stranded'` observed AT THE DOOR, not dropped — with its reverse + * control, a resume failure the engine does NOT call stranded, which must + * report `repairable: false` rather than inheriting a default. + */ + +import { describe, it, expect, beforeEach } from 'vitest'; +import { AutomationEngine, InMemorySuspendedRunStore } from '@objectstack/service-automation'; +// [#4550] The engine doubles below route their write verbs through ObjectQL's +// OWN dispatch predicates rather than a hand-mirrored copy — a double looser +// than the engine it stands in for is how #4434 shipped a dead REST route with +// its suite green. +import { assertEngineDeleteDispatch, assertEngineUpdateDispatch } from '@objectstack/objectql'; +import { strandedDecisionDetails } from '@objectstack/types'; +import { ApprovalService } from './approval-service.js'; +import { registerApprovalNode } from './approval-node.js'; + +const SYSTEM_CTX = { isSystem: true, positions: [], permissions: [] } as any; +const noopLogger = { info() {}, warn() {}, error() {}, debug() {} }; + +/** In-memory ObjectQL stand-in for the approvals tables. */ +function makeFakeEngine() { + const tables = new Map(); + const rows = (o: string) => (tables.get(o) ?? (tables.set(o, []), tables.get(o)!)); + const matches = (row: any, where: any) => Object.entries(where ?? {}).every(([k, v]) => { + if (k.startsWith('$')) throw new Error(`fake engine: unsupported filter operator ${k}`); + if (v && typeof v === 'object' && '$in' in (v as any)) return (v as any).$in.includes(row[k]); + if (v && typeof v === 'object' && '$ne' in (v as any)) return row[k] !== (v as any).$ne; + return row[k] === v; + }); + return { + tables, + async find(object: string, opts: any = {}) { + const where = opts.where ?? opts.filter ?? {}; + const out = rows(object).filter(r => matches(r, where)); + // The caller's bound is honoured by PRESENCE, never truthiness: `limit: 0` + // is a real bound that must return no rows, and a falsy test hands back + // the whole table instead. + const start = opts.offset ?? 0; + const page = typeof opts.limit === 'number' ? out.slice(start, start + opts.limit) : out.slice(start); + return page.map(r => ({ ...r })); + }, + async insert(object: string, data: any) { rows(object).push({ ...data }); return { ...data }; }, + async update(object: string, data: any, options?: any) { + const dispatch = assertEngineUpdateDispatch(data, options); + const table = rows(object); + if (dispatch.kind === 'multi') { + let n = 0; + for (let i = 0; i < table.length; i++) { + if (matches(table[i], options?.where)) { table[i] = { ...table[i], ...data }; n++; } + } + return { updated: n }; + } + const i = table.findIndex(r => r.id === dispatch.id); + if (i >= 0) table[i] = { ...table[i], ...data }; + return i >= 0 ? { ...table[i] } : null; + }, + async delete(object: string, options?: any) { + const dispatch = assertEngineDeleteDispatch(options); + const table = rows(object); + if (dispatch.kind === 'multi') { + const survivors = table.filter(r => !matches(r, options?.where)); + const deleted = table.length - survivors.length; + table.splice(0, table.length, ...survivors); + return { deleted }; + } + const i = table.findIndex(r => r.id === dispatch.id); + if (i >= 0) table.splice(i, 1); + return { id: dispatch.id }; + }, + }; +} + +const DEAL_APPROVAL = { + name: 'deal_approval', + label: 'Deal Approval', + type: 'autolaunched', + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { + id: 'approve_step', type: 'approval', label: 'Manager Approval', + config: { approvers: [{ type: 'user', value: 'u1' }] }, + }, + { id: 'on_approved', type: 'mark', label: 'Approved' }, + // The card's own failing node: the reject branch writes back to the + // record, and on the reported deployment that record was gone. + { id: 'mark_rejected', type: 'mark', label: 'Rejected' }, + { id: 'end', type: 'end', label: 'End' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'approve_step' }, + { id: 'e2', source: 'approve_step', target: 'on_approved', label: 'approve' }, + { id: 'e3', source: 'approve_step', target: 'mark_rejected', label: 'reject' }, + { id: 'e4', source: 'on_approved', target: 'end' }, + { id: 'e5', source: 'mark_rejected', target: 'end' }, + ], +}; + +describe('#13807 — a stranded decision publishes its facts, and keeps its status code', () => { + let data: ReturnType; + let service: ApprovalService; + let marks: string[]; + /** When set, `mark_rejected` throws it — the card's downstream failure. */ + let rejectBranchThrows: string | undefined; + + /** One live process: real engine, real approval node, real approvals service. */ + function boot() { + const automation = new AutomationEngine(noopLogger as any, new InMemorySuspendedRunStore()); + registerApprovalNode(automation, service, noopLogger as any); + automation.registerNodeExecutor({ + type: 'mark', + async execute(node: any) { + if (node.id === 'mark_rejected' && rejectBranchThrows) throw new Error(rejectBranchThrows); + marks.push(node.id); + return { success: true }; + }, + } as never); + automation.registerFlow('deal_approval', DEAL_APPROVAL as never); + service.attachAutomation(automation); + return automation; + } + + const pendingRequest = async () => + (await data.find('sys_approval_request', { where: { status: 'pending' } }))[0]; + + const actionsOn = async (requestId: string) => + (await data.find('sys_approval_action', { where: { request_id: requestId } })).map(a => a.action); + + beforeEach(() => { + marks = []; + rejectBranchThrows = undefined; + data = makeFakeEngine(); + service = new ApprovalService({ engine: data as any, logger: noopLogger }); + }); + + /** Park a run at the approval node and hand back its pending request. */ + async function park(automation: AutomationEngine) { + await automation.execute('deal_approval', { + object: 'crm_deal', record: { id: 'd1', amount: 100 }, userId: 'submitter', + } as never); + return pendingRequest(); + } + + it('PIN 1 — the three coexisting outcomes, with the new fields naming all three', async () => { + // The card's own node failure text, verbatim in shape: the reject branch + // updates a record that no longer exists. + rejectBranchThrows = 'update_record(crm_leave_request) failed: Record 9SEmlyRfw8D9-J7Z not found'; + const automation = boot(); + const req = await park(automation); + const runId = req.flow_run_id; + expect(runId, 'the request must name the run it gates').toBeTruthy(); + + const err = await service + .decide(req.id, { decision: 'reject', actorId: 'u1', comment: 'no' }, SYSTEM_CTX) + .then(() => null, (e: Error) => e); + + // ── OUTCOME 1: the caller is told this FAILED. Unchanged, deliberately. + expect(err, 'the door still throws — ⛔ not "return 200"').toBeTruthy(); + expect(err?.message).toMatch(/^RESUME_FAILED/); + // The prose is untouched: it is what a human reads in a log, and the + // ruling added a machine-readable half rather than rewriting the sentence. + expect(err?.message).toMatch(/could not be resumed and is now stranded/); + expect(err?.message).toContain(rejectBranchThrows); + + // ── OUTCOME 2: the decision IS durable, and the envelope says so. + const row = (await data.find('sys_approval_request', { where: { id: req.id } }))[0]; + expect(row.status, 'the effect landed — this is the fact the 500 used to hide').toBe('rejected'); + expect(row.completed_at).toBeTruthy(); + expect(await actionsOn(req.id)).toContain('reject'); + + // ── OUTCOME 3: the run is stranded, and it is NAMED. + expect(await automation.hasSuspendedRun(runId)).toBe(false); + expect((await automation.resume(runId)).code).toBe('RUN_NOT_FOUND'); + + // ⭐ The ruling's deliverable: all three readable as fields, by a caller + // that never parses the sentence. Before this an operator had to regex the + // run id out of prose and had no way at all to learn `finalized`. + const details = strandedDecisionDetails(err); + expect(details, 'the error must carry the machine-readable half').toBeDefined(); + expect(details).toEqual({ + finalized: true, + decision: 'reject', + runId, + repairable: true, + }); + }); + + it("PIN 2 — a healthy decision is unchanged: no throw, no envelope", async () => { + // The reverse control for PIN 1. Only the downstream node changes; if this + // answered the same way, PIN 1 would be measuring the harness. + const automation = boot(); + const req = await park(automation); + + const out = await service.decide( + req.id, { decision: 'reject', actorId: 'u1' }, SYSTEM_CTX, + ); + + expect(out.finalized).toBe(true); + expect(out.decision).toBe('reject'); + expect(out.resumed, 'the run advanced — nothing to report').toBe(true); + expect(out.resumeError).toBeUndefined(); + expect(marks).toEqual(['mark_rejected']); + // A success carries no stranded envelope anywhere: the fields exist to + // describe a failure, and a healthy call must not grow a failure shape. + expect(strandedDecisionDetails(out as unknown)).toBeUndefined(); + }); + + it("PIN 3 — 'stranded' is read AT THE DOOR, and its absence is not read as repairable", async () => { + // The signal the platform already produced and nobody consumed. This pins + // the DOOR's reading of it, which is the half that was missing: the + // engine's own suite proves the stamp exists. + rejectBranchThrows = 'the node blew up'; + const automation = boot(); + const req = await park(automation); + + // What the engine actually hands the door on this exit — captured here so + // the pin fails if the producer's shape moves, not only if the door does. + // ⚠️ It carries a `status` and NO `code`: a door reading only `code` (as + // this one did) sees an unnamed failure and cannot tell a repairable + // strand from a dead run. + const seen: any[] = []; + const realResume = automation.resume.bind(automation); + (automation as any).resume = async (...args: any[]) => { + const r = await (realResume as any)(...args); + seen.push(r); + return r; + }; + + const err = await service + .decide(req.id, { decision: 'reject', actorId: 'u1' }, SYSTEM_CTX) + .then(() => null, (e: Error) => e); + + expect(seen.length).toBe(1); + expect(seen[0].success).toBe(false); + expect(seen[0].status, "the producer's discriminator, #13937 shape 4").toBe('stranded'); + expect(seen[0].code, 'and it names no code — which is why status had to be carried').toBeUndefined(); + expect(strandedDecisionDetails(err)?.repairable).toBe(true); + + // ── REVERSE CONTROL: a resume failure the engine does NOT call stranded. + // `repairable` must be false, not defaulted-true and not inherited: the + // run has no journalled snapshot, so the repair verb would refuse. ⛔ The + // absence of the signal is not the presence of repairability. + const fresh = makeFakeEngine(); + const lost = new ApprovalService({ engine: fresh as any, logger: noopLogger }); + const lostAutomation = new AutomationEngine(noopLogger as any, new InMemorySuspendedRunStore()); + registerApprovalNode(lostAutomation, lost, noopLogger as any); + lostAutomation.registerNodeExecutor({ + type: 'mark', async execute() { return { success: true }; }, + } as never); + lostAutomation.registerFlow('deal_approval', DEAL_APPROVAL as never); + lost.attachAutomation({ + hasSuspendedRun: async () => true, + // The #4420 shape: the engine REPORTS the failure rather than throwing, + // names a code, and reports no run status at all. + resume: async () => ({ success: false, code: 'RUN_NOT_FOUND', error: "No suspended run 'run_x'" }), + } as never); + await lostAutomation.execute('deal_approval', { + object: 'crm_deal', record: { id: 'd2', amount: 7 }, userId: 'submitter', + } as never); + const lostReq = (await fresh.find('sys_approval_request', { where: { status: 'pending' } }))[0]; + + const lostErr = await lost + .decide(lostReq.id, { decision: 'approve', actorId: 'u1' }, SYSTEM_CTX) + .then(() => null, (e: Error) => e); + + const lostDetails = strandedDecisionDetails(lostErr); + expect(lostDetails, 'still a truthful envelope — the decision still stands').toBeDefined(); + expect(lostDetails?.finalized).toBe(true); + expect(lostDetails?.decision).toBe('approve'); + expect(lostDetails?.repairable, 'no stranded stamp ⇒ no repair promise').toBe(false); + expect(lostDetails?.runId, 'and it still names the run an operator must look at') + .toBe(lostReq.flow_run_id); + }); +}); diff --git a/packages/rest/src/rest-approvals-wire-codes.test.ts b/packages/rest/src/rest-approvals-wire-codes.test.ts index 5fa7f97fa9..eb6f0e9fd7 100644 --- a/packages/rest/src/rest-approvals-wire-codes.test.ts +++ b/packages/rest/src/rest-approvals-wire-codes.test.ts @@ -47,6 +47,11 @@ import { describe, it, expect, vi } from 'vitest'; import { ApiErrorSchema } from '@objectstack/spec/api'; +// [#13807] The PRODUCER half of the stranded-decision carrier, used here +// exactly as `plugin-approvals` uses it. Building the thrown error through the +// shared constructor is the point: it is what makes this a round-trip pin +// rather than this file's private idea of what the service attaches. +import { strandedDecisionFailure } from '@objectstack/types'; import { BUILTIN_OPERATION_MESSAGES } from '@objectstack/spec/system'; // `.js` on purpose — NodeNext resolution requires the extension (#7248). import { RestServer } from './rest-server.js'; @@ -362,6 +367,65 @@ describe('approvals wire codes are registered vocabulary (#8885)', () => { ).toBe(true); }); + it('a stranded rejection publishes finalized / decision / runId / repairable beside its 500 (#13807)', async () => { + // Maintainer ruling 2026-09-04, decision batch #37, option B. ⛔ The + // status code does NOT move: a decision that landed while its run + // stranded is still a failure, and answering 200 is named forbidden on + // the card. What the ruling added is the machine-readable half — the + // caller reading this 500 used to have exactly one honest inference + // available ("the rejection did not happen") and it was the WRONG one. + const rest = boot({ + decide: vi.fn().mockRejectedValue(strandedDecisionFailure( + "RESUME_FAILED: the reject decision was recorded on request req_1, but its flow run " + + "'run_8380f743' could not be resumed and is now stranded: Node 'mark_rejected' failed", + { finalized: true, decision: 'reject', runId: 'run_8380f743', repairable: true }, + )), + }); + const answer = await drive(rest, 'POST', `${REQ}/reject`); + + // Unchanged: status, code, and the [#13095] strip. + expect(answer.status).toBe(500); + expect(answer.body?.code).toBe('RESUME_FAILED'); + expect(answer.body?.error).toMatch(/^the reject decision was recorded on request req_1/); + expect( + ApiErrorSchema.safeParse({ code: answer.body?.code, message: answer.body?.error }).success, + 'RESUME_FAILED must be in StandardErrorCode ∪ ERROR_CODE_LEDGER', + ).toBe(true); + + // Added: the four facts, on the wire, as fields. `finalized` is the + // load-bearing one — it is the only thing on this response that says + // the decision STANDS, and there was no way to learn it before. + expect(answer.body?.finalized).toBe(true); + expect(answer.body?.decision).toBe('reject'); + expect(answer.body?.runId).toBe('run_8380f743'); + // The engine's own `'stranded'` discriminator, carried the whole way + // instead of dying at `serviceResume` — its FIRST consumer. + expect(answer.body?.repairable).toBe(true); + // ⚠️ The run id is a FIELD, not something to regex out of the prose. + // Asserting it only inside `error` would leave this pin green on the + // exact defect the card reported. + expect(Object.keys(answer.body ?? {}).sort()) + .toEqual(['code', 'decision', 'error', 'finalized', 'repairable', 'runId']); + }); + + it('REVERSE CONTROL — a RESUME_FAILED with no carrier answers exactly the body it always did', async () => { + // ⛔ The door never synthesises the envelope. A `RESUME_FAILED` raised + // by something that had no decision to report must not be dressed up + // as one, so absence of the carrier is absence of the fields — and + // this is what proves the case above is reading the producer rather + // than the route's own constant. + const rest = boot({ + decide: vi.fn().mockRejectedValue(new Error( + "RESUME_FAILED: the rejection was recorded on request req_1, but its flow run 'run_1' " + + 'could not be resumed — an operator has to advance it', + )), + }); + const answer = await drive(rest, 'POST', `${REQ}/reject`); + expect(answer.status).toBe(500); + expect(answer.body?.code).toBe('RESUME_FAILED'); + expect(Object.keys(answer.body ?? {}).sort()).toEqual(['code', 'error']); + }); + it('an unmapped service fault on approve answers 500 APPROVAL_APPROVE_FAILED — the template-generated arm, live', async () => { const rest = boot({ decide: vi.fn().mockRejectedValue(new Error('kaboom: not in the mapping table')), diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index 87db8036c9..31d5b24f52 100644 --- a/packages/rest/src/rest-server.ts +++ b/packages/rest/src/rest-server.ts @@ -25,6 +25,11 @@ import { looksLikeInternalErrorLeak, declaresServerFault, INTERNAL_ERROR_MESSAGE, + // [#13807] The recogniser half of the stranded-decision carrier — see + // `handleApprovalError` below. Constructor and reader share one module in + // `@objectstack/types` because the producer is a PLUGIN and rest cannot + // import one. + strandedDecisionDetails, } from '@objectstack/types'; import { allowPerfDisclosure, @@ -11881,6 +11886,26 @@ export class RestServer { // matching-organization context. [/^READ_BACK_FAILED/, 500, 'READ_BACK_FAILED'], ]; + // [#13807, maintainer ruling 2026-09-04 batch #37] The + // machine-readable half of a stranded decision, when the service + // attached one. The status code does NOT move — a recorded + // decision whose run strands is still a failure and the ruling + // upholds that — but the body stops being prose only: `finalized` + // says the decision stands, `decision` / `runId` name what and + // where, and `repairable` carries the engine's own `'stranded'` + // discriminator through instead of dying at the door. + // + // Before this, a caller reading 500 had exactly one honest move — + // assume the rejection did not happen — and it was the wrong one: + // the row IS terminal and the record's mirrored status HAS moved. + // An operator had to regex the run id out of a sentence. + // + // ⛔ Presence-gated, never synthesised. `strandedDecisionDetails` + // returns `undefined` for any error that did not carry a complete + // envelope, and this then answers exactly the body it always did — + // a `RESUME_FAILED` from a caller with no decision to report must + // not be dressed up as one. + const stranded = strandedDecisionDetails(err); for (const [re, status, code] of mapping) { if (re.test(msg)) { // [#13095] The strip is anchored to the CODE this row just @@ -11894,7 +11919,15 @@ export class RestServer { // message opening with a DIFFERENT capitalised word and a // colon), where the anchored form can only ever remove a // duplicate of the `code` already on the wire. - res.status(status).json({ code, error: msg.replace(new RegExp(`^${code}:\\s*`), '') }); + res.status(status).json({ + code, + error: msg.replace(new RegExp(`^${code}:\\s*`), ''), + // Anchored to the code the envelope describes, for the + // same reason the strip above is: these four facts are + // about a recorded-decision-with-stranded-run and + // about nothing else. + ...(code === 'RESUME_FAILED' && stranded ? stranded : {}), + }); return true; } } diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index cba1459dbf..fe9d686aa1 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -23,6 +23,13 @@ export * from './thrown-http-error.js'; // the CLIENT is told; this decides what the LOG says — 5xx always, 4xx never. export * from './server-fault-log.js'; export * from './validation-failure.js'; +// [#13807, maintainer ruling 2026-09-04 batch #37] The sibling carrier for the +// OTHER half-state a door can report: a decision that IS durably recorded whose +// flow run could not be resumed. Producer `@objectstack/plugin-approvals`, +// consumer the REST approvals door — and rest cannot import a plugin, so the +// constructor and its recogniser share a home here exactly as the +// validation-failure pair above do. +export * from './stranded-decision.js'; // [#6615] The one home for Postgres' `«sub-object» "x" of relation "y"` phrase, // whose missing-COLUMN spelling contains a legal missing-TABLE phrase as a // substring. Three packages had each repaired that superstring hole separately. diff --git a/packages/types/src/stranded-decision.test.ts b/packages/types/src/stranded-decision.test.ts new file mode 100644 index 0000000000..e327d937cc --- /dev/null +++ b/packages/types/src/stranded-decision.test.ts @@ -0,0 +1,76 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The stranded-decision carrier: constructor and recogniser, pinned together + * (#13807). + * + * The pair exists because the producer is `@objectstack/plugin-approvals` and + * the consumer is the REST approvals door, and rest cannot import a plugin. + * The risk that creates — and the only one this file is about — is the two + * sides drifting into a stringly-typed agreement about a property name that + * nothing checks. Keeping the constructor and the reader in ONE module removes + * the drift; these pins remove the OTHER failure, a reader tolerant enough to + * put a half-envelope on the wire. + */ + +import { describe, it, expect } from 'vitest'; +import { strandedDecisionDetails, strandedDecisionFailure } from './stranded-decision.js'; + +describe('strandedDecisionFailure / strandedDecisionDetails (#13807)', () => { + const details = { finalized: true, decision: 'reject', runId: 'run_1', repairable: true } as const; + + it('round-trips the four facts and leaves the message untouched', () => { + const err = strandedDecisionFailure('RESUME_FAILED: the reject decision was recorded', details); + expect(err).toBeInstanceOf(Error); + // The prose is the producer's and stays the producer's: a human reads + // it in a log, the fields are what a machine reads on the wire, and + // this carrier added the second without rewriting the first. + expect(err.message).toBe('RESUME_FAILED: the reject decision was recorded'); + expect(strandedDecisionDetails(err)).toEqual(details); + }); + + it('answers undefined for anything that is not one — the predicate and the payload are one call', () => { + expect(strandedDecisionDetails(undefined)).toBeUndefined(); + expect(strandedDecisionDetails(null)).toBeUndefined(); + expect(strandedDecisionDetails(new Error('RESUME_FAILED: prose only'))).toBeUndefined(); + expect(strandedDecisionDetails('RESUME_FAILED: a string')).toBeUndefined(); + expect(strandedDecisionDetails({ strandedDecision: 'not an object' })).toBeUndefined(); + }); + + it('⛔ refuses a PARTIAL carrier rather than publishing half an envelope', () => { + // The failure this forecloses is specific: a consumer branching on + // `finalized === undefined` would read a missing field as "the + // decision did not stand" — the exact misreading the whole card is + // about, reintroduced one layer down. All-or-nothing instead. + const partial = (over: Record) => + strandedDecisionDetails({ strandedDecision: { ...details, ...over } }); + expect(partial({ finalized: undefined })).toBeUndefined(); + expect(partial({ finalized: false })).toBeUndefined(); + expect(partial({ decision: undefined })).toBeUndefined(); + expect(partial({ decision: '' })).toBeUndefined(); + expect(partial({ runId: undefined })).toBeUndefined(); + expect(partial({ runId: '' })).toBeUndefined(); + expect(partial({ repairable: undefined })).toBeUndefined(); + // REVERSE CONTROL for the six above: with nothing overridden the same + // helper resolves, so the rejections are the override talking and not + // a reader that refuses everything. + expect(partial({})).toEqual(details); + }); + + it("carries repairable: false verbatim — ⛔ absence of the engine's signal is not repairability", () => { + // `repairable` is derived from `AutomationResult.status === 'stranded'` + // at the producer. A run the engine did NOT call stranded has no + // journalled snapshot, so the repair verb would refuse it; promising a + // verb that will refuse is worse than promising nothing. + const err = strandedDecisionFailure('RESUME_FAILED: …', { ...details, repairable: false }); + expect(strandedDecisionDetails(err)).toEqual({ ...details, repairable: false }); + }); + + it('narrows to exactly the four declared keys — a producer cannot smuggle extras onto the wire', () => { + const err = strandedDecisionFailure('RESUME_FAILED: …', { + ...details, requestId: 'req_1', internalNote: 'do not publish', + } as never); + expect(Object.keys(strandedDecisionDetails(err) ?? {}).sort()) + .toEqual(['decision', 'finalized', 'repairable', 'runId']); + }); +}); diff --git a/packages/types/src/stranded-decision.ts b/packages/types/src/stranded-decision.ts new file mode 100644 index 0000000000..81f38850dd --- /dev/null +++ b/packages/types/src/stranded-decision.ts @@ -0,0 +1,135 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The machine-readable half of a `RESUME_FAILED` — a decision that is durably + * recorded whose flow run could not be resumed (#13807). + * + * ## The condition + * + * An approval decision finalises: the `sys_approval_request` row flips to its + * terminal status, the audit action is written, the record's mirrored status + * field advances — and only THEN is the owning flow run resumed. When that + * resume fails the writes are already durable, so the outcome stands and the + * run is stranded. `@objectstack/plugin-approvals` throws rather than + * answering `resumed: false`, deliberately: a recorded decision whose flow + * never advances is #4420's zombie half-state, and the contract + * (`ApprovalDecisionResult`) declares the throw intentional. + * + * ⛔ This module does NOT change that posture. The maintainer ruled on + * 2026-09-04 (decision batch #37, option B) that the door **keeps its status + * code** — the 500-class `RESUME_FAILED` — because the effect landing while + * the run strands is still a failure. What the ruling changed is that the + * throw must be *truthful*: the facts a caller needs were being discarded. + * + * ## What was being discarded, measured + * + * Three states coexist after such a call: the caller reads 500, the request + * IS in its terminal status, and the run is stranded. A caller — human, + * script, or agent — reads 500 as "the rejection did not happen" and retries + * or escalates. It did happen. Before this module the only carrier of that + * fact was English prose in `error`, so an operator had to regex the run id + * out of a sentence, and nothing said whether the run was repairable at all. + * + * Meanwhile the engine already knew. `AutomationResult.status: 'stranded'` + * (`@objectstack/spec`, `automation-service.ts`) is stamped on exactly the + * exit that journals a repair snapshot — the shape-4 name from #13937 — and + * it is distinct from `'failed'` on purpose: `'failed'` says the run ran and + * was rejected, `'stranded'` says a recorded continuation stopped mid-flight + * and an operator has something to repair. It had a producer and, until this + * module, **zero consumers**: the approvals door read only + * `success` / `code` / `error` off the resume result and dropped it one line + * before the envelope was built. + * + * ## Why it lives in `@objectstack/types` + * + * Same Home rule as {@link ValidationFailureDetails} one file over: the + * PRODUCER is `@objectstack/plugin-approvals` and the CONSUMER is the REST + * door in `@objectstack/rest`, and rest cannot import a plugin. Both already + * depend on this package, so the shared declaration adds no dependency edge — + * and keeping the constructor and the reader in ONE module is what stops the + * two sides from drifting into a stringly-typed agreement about a property + * name. + * + * ⛔ Deliberately NOT a tolerant reader. There is no alias chain and no prose + * parsing: a body either carries the four facts the producer attached, or the + * response is exactly what it was before. A `RESUME_FAILED` raised by + * something that never had a decision to report (a test double, a future + * caller) must not be dressed up as one. + */ + +/** + * The four facts a stranded decision publishes alongside its `code` and + * `error`. Every field is present or the whole envelope is absent — a partial + * one would let a consumer branch on `finalized === undefined` and read it as + * "the decision did not stand", which is the exact misreading this exists to + * end. + */ +export interface StrandedDecisionDetails { + /** + * Always `true`. The decision reached a terminal state and is durable; the + * 5xx is about the run, never about the decision. Spelled as a literal + * rather than omitted so a consumer reads a fact instead of an absence. + */ + finalized: true; + /** + * Which outcome was recorded — `'approve'` / `'reject'` for a decision, and + * the sibling doors on the same path for the rest (`'revise'` on a + * send-back, `'resubmit'`). Free-form by design: the vocabulary belongs to + * the producing service, not to this recogniser. + */ + decision: string; + /** The stranded run. The one identifier an operator needs to act. */ + runId: string; + /** + * Whether the engine says this run can still be repaired — derived from the + * engine's own discriminator (`AutomationResult.status === 'stranded'`), + * never from the message text and never assumed. + * + * `false` is the honest answer for every other exit, including the ones + * that report no status at all (a lost run, an engine that predates the + * discriminator). ⛔ Absence of the signal is not repairability: promising a + * repair verb that will refuse is worse than promising nothing. + */ + repairable: boolean; +} + +/** Property under which {@link StrandedDecisionDetails} rides a thrown error. */ +const CARRIER = 'strandedDecision'; + +/** + * Structured details for a thrown stranded-decision failure, or `undefined` + * when `err` is not one. + * + * Callers use the `undefined` result as the predicate and the returned object + * as the payload, so the two can never disagree — the same contract + * `validationFailureDetails` keeps one module over. Every field is validated: + * a malformed carrier answers `undefined` rather than putting a half-envelope + * on the wire. + */ +export function strandedDecisionDetails(err: unknown): StrandedDecisionDetails | undefined { + const carried = (err as Record | null | undefined)?.[CARRIER]; + if (!carried || typeof carried !== 'object') return undefined; + const d = carried as Partial; + if (d.finalized !== true) return undefined; + if (typeof d.decision !== 'string' || d.decision === '') return undefined; + if (typeof d.runId !== 'string' || d.runId === '') return undefined; + if (typeof d.repairable !== 'boolean') return undefined; + return { finalized: true, decision: d.decision, runId: d.runId, repairable: d.repairable }; +} + +/** + * The CONSTRUCTOR for the shape {@link strandedDecisionDetails} recognises — + * kept in the same module so the two can never drift. + * + * The message stays the producer's own, unchanged: the prose is what a human + * reads in a log, the details are what a machine reads on the wire, and this + * ruling added the second without touching the first. + */ +export function strandedDecisionFailure( + message: string, + details: StrandedDecisionDetails, +): Error { + const err = new Error(message) as Error & { [CARRIER]?: StrandedDecisionDetails }; + err[CARRIER] = details; + return err; +} diff --git a/scripts/engine-double-contract.pinned.json b/scripts/engine-double-contract.pinned.json index 8e1be74196..996640222d 100644 --- a/scripts/engine-double-contract.pinned.json +++ b/scripts/engine-double-contract.pinned.json @@ -2126,6 +2126,16 @@ "verb": "update", "pinned": 1 }, + { + "file": "packages/plugins/plugin-approvals/src/decision-strand-envelope.test.ts", + "verb": "delete", + "pinned": 1 + }, + { + "file": "packages/plugins/plugin-approvals/src/decision-strand-envelope.test.ts", + "verb": "update", + "pinned": 1 + }, { "file": "packages/plugins/plugin-approvals/src/manager-approver-org-screen.test.ts", "verb": "delete",