From 37695c4f4b9fc5d0b0d61708808e91e3493fedbe Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 4 Sep 2026 14:58:19 +0000 Subject: [PATCH 1/3] fix(approvals,rest,types): a stranded decision publishes finalized/decision/runId/repairable beside its 500 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Maintainer ruling 2026-09-04, decision batch #37, option B. One `POST /api/v1/approvals/requests/{id}/reject` produced three coexisting outcomes: the caller read HTTP 500, the request row WAS terminal and had left the pending inbox, and the run was stranded. 500 alone reads as "the rejection did not happen", so callers retried against a durable decision. The status code does not move — the effect landing while the run strands is still a failure — and the door does not become atomic: the #13937 shape-4 ruling binds this door's own writes too, so no decision is ever rolled back. What changed is that the door stops discarding what the engine said. - `serviceResume` carries `AutomationResult.status` through. It read only success/code/error, and the stranded exit reports a status and NO code, so the repairability signal died one line before the envelope was built — a member with a producer and, until now, zero consumers. - `resumeRecordedOutcome` throws a carrier with `finalized` (the decision stands), `decision`, `runId`, and `repairable` derived from the engine's `'stranded'` discriminator. Absence of that stamp is `false`, never a default: a repair verb that would refuse is worse than no promise. - The REST approvals door forwards those four fields on `RESUME_FAILED` only, presence-gated — an error with no carrier answers exactly the body it always did. - `@objectstack/types` hosts the constructor and its recogniser in one module (the producer is a plugin; rest cannot import one), the same Home rule as the validation-failure pair beside it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y --- .../truthful-stranded-decision-envelope.md | 17 ++ .../plugin-approvals/src/approval-service.ts | 65 +++- .../src/decision-strand-envelope.test.ts | 285 ++++++++++++++++++ .../src/rest-approvals-wire-codes.test.ts | 64 ++++ packages/rest/src/rest-server.ts | 35 ++- packages/types/src/index.ts | 7 + packages/types/src/stranded-decision.test.ts | 76 +++++ packages/types/src/stranded-decision.ts | 135 +++++++++ 8 files changed, 678 insertions(+), 6 deletions(-) create mode 100644 .changeset/truthful-stranded-decision-envelope.md create mode 100644 packages/plugins/plugin-approvals/src/decision-strand-envelope.test.ts create mode 100644 packages/types/src/stranded-decision.test.ts create mode 100644 packages/types/src/stranded-decision.ts 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/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..b66d4863bd --- /dev/null +++ b/packages/plugins/plugin-approvals/src/decision-strand-envelope.test.ts @@ -0,0 +1,285 @@ +// 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'; +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 ?? {}; + let out = rows(object).filter(r => matches(r, where)); + if (opts.limit) out = out.slice(0, opts.limit); + return out.map(r => ({ ...r })); + }, + async insert(object: string, data: any) { rows(object).push({ ...data }); return { ...data }; }, + async update(object: string, idOrData: any) { + const row = rows(object).find(r => r.id === idOrData.id); + if (row) Object.assign(row, idOrData); + return row ? { ...row } : null; + }, + async delete(object: string, opts: any = {}) { + const list = rows(object); + for (let i = list.length - 1; i >= 0; i--) if (matches(list[i], opts.where ?? {})) list.splice(i, 1); + return { affected: 1 }; + }, + }; +} + +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; +} From 13b58ed7d7c116c6086cf37e63075b9c9e09b620 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 4 Sep 2026 15:22:57 +0000 Subject: [PATCH 2/3] test(approvals): route the new pin's engine double through ObjectQL's dispatch predicates `check:engine-double-contract` and `check:objectql-double-limit` both caught the new fake in `decision-strand-envelope.test.ts`: its `update()`/`delete()` hand-rolled the dispatch and its `find()` read the caller's bound by truthiness, so `limit: 0` would have returned the whole table. Both verbs now open with `assertEngineUpdateDispatch` / `assertEngineDeleteDispatch` and the bound is honoured by presence; the two new rows are recorded in the pinned ledger (`--write`, 2 added, 0 lost). `check:system-context-census` anchors are re-anchored by its own `--fix`: pure line rot from this branch's edits, uniform +5 on `rest-server.ts` (the import block) and +52 on `approval-service.ts` (the docblocks), which is what makes it rot rather than a finding. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y --- content/docs/permissions/system-context.mdx | 10 ++-- .../src/decision-strand-envelope.test.ts | 48 ++++++++++++++----- scripts/engine-double-contract.pinned.json | 10 ++++ 3 files changed, 52 insertions(+), 16 deletions(-) diff --git a/content/docs/permissions/system-context.mdx b/content/docs/permissions/system-context.mdx index e6803d98d0..2f040be5b2 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:404`). 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:411`, `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:404` | +| "A client can request it" | **No.** Never settable from inbound HTTP or from an action body | `rest-server.ts:1553`, `:1582`; `domains/actions.ts:404` | --- diff --git a/packages/plugins/plugin-approvals/src/decision-strand-envelope.test.ts b/packages/plugins/plugin-approvals/src/decision-strand-envelope.test.ts index b66d4863bd..db0cedd5be 100644 --- a/packages/plugins/plugin-approvals/src/decision-strand-envelope.test.ts +++ b/packages/plugins/plugin-approvals/src/decision-strand-envelope.test.ts @@ -43,6 +43,11 @@ 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'; @@ -64,20 +69,41 @@ function makeFakeEngine() { tables, async find(object: string, opts: any = {}) { const where = opts.where ?? opts.filter ?? {}; - let out = rows(object).filter(r => matches(r, where)); - if (opts.limit) out = out.slice(0, opts.limit); - return out.map(r => ({ ...r })); + 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, idOrData: any) { - const row = rows(object).find(r => r.id === idOrData.id); - if (row) Object.assign(row, idOrData); - return row ? { ...row } : null; + 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, opts: any = {}) { - const list = rows(object); - for (let i = list.length - 1; i >= 0; i--) if (matches(list[i], opts.where ?? {})) list.splice(i, 1); - return { affected: 1 }; + 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 }; }, }; } diff --git a/scripts/engine-double-contract.pinned.json b/scripts/engine-double-contract.pinned.json index ba0ef11772..0a24e130c0 100644 --- a/scripts/engine-double-contract.pinned.json +++ b/scripts/engine-double-contract.pinned.json @@ -2096,6 +2096,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", From b65dc1915ec06117a678e77d15aced5b8de4adc1 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 4 Sep 2026 21:28:22 +0000 Subject: [PATCH 3/3] chore(docs): regenerate the system-context census page from the merged tree MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The os-regen merge driver hands this page back rather than text-merging it — it is generated. Regenerated with `pnpm gen:system-context-census` after merging origin/main; the three re-anchors are all line rot in `packages/runtime/src/domains/actions.ts` (+10 from upstream commits), none of them this branch's own files. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y --- content/docs/permissions/system-context.mdx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/content/docs/permissions/system-context.mdx b/content/docs/permissions/system-context.mdx index 2f040be5b2..6f8740e51b 100644 --- a/content/docs/permissions/system-context.mdx +++ b/content/docs/permissions/system-context.mdx @@ -65,7 +65,7 @@ not on any flag. `isSystem` is **server-constructed and never client-supplied**. Inbound HTTP cannot set it (`packages/rest/src/rest-server.ts:1553`, `:1582`), and neither -can an action body (`packages/runtime/src/domains/actions.ts:404`). It is +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: ```ts @@ -160,7 +160,7 @@ The largest single consumer — **17 of the 106 sites**. | 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: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:411`, `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` | +| 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` | | 54 | Package REST route capability gate bypassed | rest | Get: package read/write over REST without `manage_metadata` / `studio.access` / `setup.access` | `package-routes.ts:102` | | 55 | Package domain capability gates bypassed | runtime | Get: package management and package-inventory reads without the capability | `domains/packages.ts:241`, `:274` | @@ -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:1553`, `:1582`; `domains/actions.ts:404` | +| "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` | ---