From c8a0ea3966274a022e5393aaf17ef18fa644ef1a Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 13 Sep 2026 08:28:14 +0000 Subject: [PATCH 1/4] fix(approvals): recall tells its caller WHICH resume failure stranded the run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `recall` resumes directly rather than through `resumeRecordedOutcome`, so the `resumeCode` / `resumeStatus` the engine already stamps were dropped one line before the result was built: the caller got `resumed: false` plus prose and no way to learn the run is repairable. Fill the already-declared `ApprovalRecallResult.resumeFailure` slot, on the two exits whose honest code is `RESUME_FAILED` — this door's own strand, and a resume that succeeded while the subflow parent above it stranded. Claude-Session: https://claude.ai/code/session_01URLHobLUJB9K1ABV6ofdjj Co-authored-by: Claude --- .../plugin-approvals/src/approval-service.ts | 84 +++++++++++++++++-- 1 file changed, 78 insertions(+), 6 deletions(-) diff --git a/packages/plugins/plugin-approvals/src/approval-service.ts b/packages/plugins/plugin-approvals/src/approval-service.ts index 9a68d3685b..fa932e35dd 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.ts @@ -231,9 +231,10 @@ export interface ApprovalResumeSurface { * [#15556; the #16472 family ruling] The subflow PARENT strand that * `childRunId`'s own completion bubbled into, if the engine's up-bubble * hit the `'stranded'` exit — read, and CLEARED, by - * {@link ApprovalService.resumeRecordedOutcome} right after a resume it - * issued reports success, so a decision whose OWN run advanced can still - * tell the caller a run further up the chain did not. + * {@link ApprovalService.resumeRecordedOutcome} (and, since #15970, by + * {@link ApprovalService.recall}, which resumes directly) right after a + * resume it issued reports success, so a decision or a withdrawal whose OWN + * run advanced can still tell the caller a run further up the chain did not. * * ⚠️ Declares a method `AutomationEngine` ALREADY implements publicly * (`takeSubflowParentStrand`); it widens no wire surface — the engine's @@ -3491,6 +3492,20 @@ export class ApprovalService implements IApprovalService { * terminally cancelled via {@link ApprovalResumeSurface.cancelRun} rather * than resumed. * + * ## A tolerated resume failure is told in FIELDS too (#15970) + * + * The #16472 family ruling (option A) leaves this door's no-throw exactly as + * it is — the withdrawal is the point and it is durable either way — and + * fills {@link ApprovalRecallResult.resumeFailure} beside the prose + * {@link ApprovalRecallResult.resumeError}: the registered code, the `runId` + * of the run that is ACTUALLY stranded, and `repairable` straight off the + * engine's own `AutomationResult.status`. Two shapes reach it — this door's + * own resume stranding (the `catch` below), and a resume that succeeded + * while the subflow parent above it stranded (#15556's shape, reported on a + * `resumed: true` answer). Everything else answers as it always did: absent, + * which per the member's docblock means "no report was made", never "no run + * is stranded". + * * The #3424 privileged override reaches a PENDING request only (#12775, * maintainer ruling 2026-09-02). On `returned` an override actor is refused * exactly as any other non-submitter: the gate is spelled as `attachViewers` @@ -3584,8 +3599,19 @@ export class ApprovalService implements IApprovalService { // fail the call — the withdrawal and the record-lock release are the point, // and they have already happened. It is still reported rather than // swallowed: `resumed: false` plus a reason, logged at error (#4420). + // + // [#15970; the #16472 family ruling, option A] ⛔ The no-throw above stays + // — the ruling upholds it by name. What changes is that the reason stops + // being prose ALONE: the discriminator the engine already stamped reaches + // the caller on {@link ApprovalRecallResult.resumeFailure} as well, so an + // operator can read `repairable` instead of parsing a sentence. The same + // two halves of ONE telling `decide` carries since #15556 — and, exactly + // like there, only where this package's OWN ledger row lets it be told: + // `RESUME_FAILED` and no other code (`error-code-ledger.zod.ts`, + // `'@objectstack/plugin-approvals'`). let resumed = false; let resumeError: string | undefined; + let resumeFailure: ResumeFailureReport | undefined; if (inReviseWindow) { // ADR-0044: the run is paused at the revise-window node, which has no // reject out-edge to resume down — terminally cancel it instead. @@ -3606,21 +3632,58 @@ export class ApprovalService implements IApprovalService { resumeError = this.missingRunCapability(runId, requestId, 'the recall', 'resume'); if (!resumeError) { try { - await this.serviceResume(runId, { + const bubbleStrand = await this.serviceResume(runId, { branchLabel: APPROVAL_BRANCH_LABELS.reject, output: { decision: 'recall', requestId }, }); resumed = true; + if (bubbleStrand) { + // [#15556; #15970] This door's OWN resume succeeded — `runId` did + // advance — but the subflow parent it bubbled into did not, so + // `resumed` stays `true` and the strand rides beside it exactly as + // `ApprovalRecallResult.resumed`'s own docblock declares. Read + // (and cleared) by {@link serviceResume}; #17908 landed the + // producer for the `decide` door and left this door's copy of the + // value discarded. ⛔ No new log line: `bubbleToParent`'s `error` + // line in `service-automation` already said this ONCE. + resumeError = + `RESUME_FAILED: the recall was recorded on request ${requestId} and its own flow run ` + + `'${runId}' resumed, but the subflow parent above it — run '${bubbleStrand.runId}' — ` + + `consumed its suspension and is now stranded: ${bubbleStrand.error}`; + resumeFailure = { + code: 'RESUME_FAILED', + runId: bubbleStrand.runId, + status: 'stranded', + repairable: bubbleStrand.repairable, + }; + } } catch (err: any) { resumeError = err?.message ?? String(err); this.logger?.error?.('[approvals] resume after recall failed — the run may be stranded', { request: requestId, run: runId, error: resumeError, }); + // #13807's derivation, unchanged and re-used rather than re-invented: + // the engine's own `AutomationResult.status` decides `repairable`, + // never this door and never the message text. + const status = ApprovalService.resumeStatusOf(err); + const repairable = status === 'stranded'; // #15389: recall resumes directly rather than through // `resumeRecordedOutcome`, so its stranded exit needs the same stash // — otherwise a recalled run is the one outcome whose re-issue would // have to be rebuilt from the row instead of replayed. - if (ApprovalService.resumeStatusOf(err) === 'stranded') { + if (repairable) { + // [#15970] …and the same exit is the one an operator can act on, + // so it is the one that carries a machine-readable report. ⛔ The + // other exits report NOTHING rather than a rounded-off code: a + // lost run's honest code is `RESUME_TARGET_LOST` and the tolerated + // duplicate's is `RESUME_IN_PROGRESS`, and this package's ledger + // row admits neither — stamping `RESUME_FAILED` there would make + // the discriminator lie about WHICH failure this was, which is the + // defect this card is fixing, one field over. Absence is declared + // legitimate by the member's own docblock ("An absent member means + // no report was made, never that no run is stranded") and is what + // `resumeRecordedOutcome` answers on those same exits. + resumeFailure = { code: 'RESUME_FAILED', runId, status: 'stranded', repairable: true }; await this.journalStrandedContinuation(requestId, { branchLabel: APPROVAL_BRANCH_LABELS.reject, output: { decision: 'recall', requestId }, @@ -3632,7 +3695,16 @@ export class ApprovalService implements IApprovalService { } const fresh = await this.readBackRequest(requestId, context); - return { request: fresh, runId, resumed, ...(resumeError ? { resumeError } : {}) }; + return { + request: fresh, + runId, + resumed, + ...(resumeError ? { resumeError } : {}), + // [#15970; #16472 ruling] Additive, and OMITTED rather than set to + // `undefined` — the same spelling `decide` uses — so a consumer that + // reads presence with `'resumeFailure' in result` reads it correctly. + ...(resumeFailure ? { resumeFailure } : {}), + }; } // ── Record-delete lifecycle linkage (#13568) ───────────────── From 30a08d35d1bc62d5416f6cf416f6a7b3c1e212c8 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 13 Sep 2026 08:34:48 +0000 Subject: [PATCH 2/4] test(approvals): pin the recall strand discriminator, both doors, with controls The card's own table row for row (PIN 1), a healthy recall so the absence in PIN 2 is a reading and not the fixture, the non-stranded exit that deliberately reports no code (PIN 3), and the identical strand through `decide` as the control that proves the harness discriminates the two doors. The #15556 subflow fixture gains the recall case its spec docblock already declared. Claude-Session: https://claude.ai/code/session_01URLHobLUJB9K1ABV6ofdjj Co-authored-by: Claude --- .changeset/15970-recall-resume-failure.md | 26 ++ .../src/recall-strand-discriminator.test.ts | 345 ++++++++++++++++++ .../subflow-hosted-approval-strand.test.ts | 61 ++++ 3 files changed, 432 insertions(+) create mode 100644 .changeset/15970-recall-resume-failure.md create mode 100644 packages/plugins/plugin-approvals/src/recall-strand-discriminator.test.ts diff --git a/.changeset/15970-recall-resume-failure.md b/.changeset/15970-recall-resume-failure.md new file mode 100644 index 0000000000..c3e9b48838 --- /dev/null +++ b/.changeset/15970-recall-resume-failure.md @@ -0,0 +1,26 @@ +--- +"@objectstack/plugin-approvals": patch +--- + +An approval `recall()` whose resume strands the run now tells the caller WHICH failure it was, in fields — `resumeFailure: { code, runId, status, repairable }` beside the prose `resumeError` — instead of one sentence a caller has to parse (#15970; the #16472 family ruling, decision batch #76, option A). + +**The shape.** A flow parks at an `approval` node; the reject branch's downstream node throws. The submitter recalls the request, which resumes the run down the `reject` edge — and that resume strands it. The withdrawal is durable and the call correctly does not throw, but the engine's own discriminator never reached the caller: `recall` resumes DIRECTLY rather than through `resumeRecordedOutcome`, and its `catch` kept `err.message` alone, discarding the `resumeStatus` (`AutomationResult.status: 'stranded'`) the error already carried one line before the result was built. `repairable` had a producer and, on this door, no consumer. + +``` +FROM service.recall(requestId, { actorId }, ctx) + -> { request: { status: 'recalled' }, runId, resumed: false, + resumeError: "resume of run '' failed: " } + // prose only — nothing says the run is still repairable + +TO service.recall(requestId, { actorId }, ctx) + -> { request: { status: 'recalled' }, runId, resumed: false, + resumeError: "resume of run '' failed: ", + resumeFailure: { code: 'RESUME_FAILED', runId: '', + status: 'stranded', repairable: true } } +``` + +**⛔ The no-throw stays, and that is the ruling's point.** The withdrawal and the record-lock release are the product of this call and they have already happened when the resume fails; making `recall` fail would be the wrong fix, not a stricter one. The door's `error` log line is untouched too, at the same level with the same context keys — the ruling left logging alone, and the report is a sibling of that line, not a replacement for it. + +**Two exits report, and the rest deliberately do not.** A report is stamped exactly where the engine's own verdict says `'stranded'`: this door's own resume stranding, and (the sibling half of #15556, whose producer landed one door over) a resume that SUCCEEDED while the subflow parent above it stranded — which answers `resumed: true` with the PARENT's `runId` on `resumeFailure`, exactly as `ApprovalRecallResult.resumed`'s docblock already declared. Every other exit answers as it always did, with no `resumeFailure` at all: a lost run's honest code is `RESUME_TARGET_LOST` and the tolerated duplicate's is `RESUME_IN_PROGRESS`, and this package's ADR-0112 ledger row admits exactly one code, so stamping `RESUME_FAILED` there would make the discriminator lie about which failure it was — the defect this fixes, one field over. Per the member's own docblock, an absent `resumeFailure` means no report was made, never that no run is stranded. + +**Additive only — no migration, and `patch` rather than `minor`.** `ApprovalRecallResult.resumeFailure` was already declared, exported and type-pinned in `@objectstack/spec` ahead of this card (`contracts/approval-service.ts`, `resume-failure-report.pin.test.ts`); this fix is the first producer that fills it. The delivered diff adds no exported symbol to `@objectstack/plugin-approvals` — nothing new is reachable from its published entry — and adds no key to a payload that did not already declare one. Nothing existing changes shape: a consumer that ignores unknown fields sees no difference, and one that reads `resumeFailure` can now branch on `repairable` and call `restoreConsumedSuspension` on the run the report names. diff --git a/packages/plugins/plugin-approvals/src/recall-strand-discriminator.test.ts b/packages/plugins/plugin-approvals/src/recall-strand-discriminator.test.ts new file mode 100644 index 0000000000..7d46d43ad0 --- /dev/null +++ b/packages/plugins/plugin-approvals/src/recall-strand-discriminator.test.ts @@ -0,0 +1,345 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #15970 — a `recall` whose resume strands says WHICH failure it was, in + * fields, not only in prose (the #16472 family ruling, option A). + * + * ## The reported defect + * + * A flow parks at an `approval` node; the reject branch's downstream node + * throws. The submitter RECALLS the request, which resumes the run down the + * `reject` edge — and that resume strands it. The withdrawal is durable and + * the call correctly does not throw, but everything a caller could act on was + * dropped: `resumed: false` plus one sentence of prose, while the engine had + * already stamped `AutomationResult.status: 'stranded'` on the error one line + * above. `recall` resumes DIRECTLY rather than through + * `resumeRecordedOutcome`, so it never reached the derivation + * (`repairable = status === 'stranded'`) every other door shares — a producer + * with a consumer on every door but this one. + * + * ## What is fixed, and what deliberately is not + * + * ⛔ The no-throw stays. The ruling upholds it by name: the withdrawal and the + * record-lock release are the point and they have already happened. ⛔ The log + * line stays byte-for-byte as it was — the ruling left logging alone. What + * changes is `ApprovalRecallResult.resumeFailure`, a slot `packages/spec` + * already declared and left without a producer on this door. + * + * ## The population, and why the doors are measured TOGETHER + * + * The card's whole claim is *"the difference is the door, not the strand"*, so + * a harness that only exercised `recall` could pass vacuously — a fixture that + * never strands anything answers "no report" just as convincingly. Every case + * below drives the SAME lever (a throwing `mark_rejected`) through the same + * live engine, and the `decide` control asserts the #13807 envelope on it, so + * the recall readings are known to be about the door. + * + * - PIN 1 the card's own table, every row, with the fixed `resumeFailure`; + * - PIN 2 a healthy recall — absence is a READING here, not the fixture; + * - PIN 3 a resume failure the engine does NOT call stranded: still no + * report, on purpose (see the assertion's own note); + * - CONTROL the identical strand through `decide`, which throws the #13807 + * envelope — the harness discriminates the two doors. + */ + +import { describe, it, expect, beforeEach } from 'vitest'; +import { AutomationEngine, InMemorySuspendedRunStore } from '@objectstack/service-automation'; +// [#4550] The engine double below routes its write verbs through ObjectQL's OWN +// dispatch predicates rather than a hand-mirrored copy. +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; + +/** Captures the door's log lines so PIN 1 can assert the level was left alone. */ +function recordingLogger() { + const lines: { level: string; msg: string; meta?: unknown }[] = []; + const at = (level: string) => (msg: unknown, meta?: unknown) => + void lines.push({ level, msg: String(msg), meta }); + return { lines, info: at('info'), warn: at('warn'), error: at('error'), debug: at('debug') } as any; +} + +/** 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)!)); + // ⭐ The lever that makes a REAL strand reachable from a test: fail the very + // next insert into one table, once. The approval node's executor opens the + // next round by inserting a `sys_approval_request`, so failing that insert + // strands the resume the same way the card's own reject-branch throw does — + // `RESUME_FAILED` with `repairable: true`, the suspension already consumed. + // Set to a table name; the first insert into it throws and clears the lever. + let failNextInsertOn: string | undefined; + 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, + set failNextInsert(object: string | undefined) { failNextInsertOn = object; }, + get failNextInsert() { return failNextInsertOn; }, + async find(object: string, opts: any = {}) { + const where = opts.where ?? opts.filter ?? {}; + const out = rows(object).filter(r => matches(r, where)); + // ⚠️ `orderBy` is honoured, and that is load-bearing rather than polish: + // `assertLatestForRun` selects the newest request with + // `orderBy [{field:'created_at', order:'desc'}], limit 1`. A double that + // ignored it returned the OLDEST row, so the guard passed on every input + // and a pin naming it would have measured nothing — the phantom-check + // shape. SortNode's key is `order`, not `direction` (spec/data/query.zod.ts). + if (Array.isArray(opts.orderBy)) { + for (const sort of [...opts.orderBy].reverse()) { + const field = sort?.field; + if (!field) continue; + const dir = sort?.order === 'desc' ? -1 : 1; + out.sort((a, b) => (a[field] < b[field] ? -1 : a[field] > b[field] ? 1 : 0) * dir); + } + } + // The caller's bound is honoured by PRESENCE, never truthiness. + 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) { + if (failNextInsertOn === object) { + failNextInsertOn = undefined; + throw new Error(`injected one-shot insert failure on ${object}`); + } + 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' }, + { 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' }, + ], +}; + +/** The card's own failing node text: the reject branch writes to a gone record. */ +const DOWNSTREAM_FAILURE = + 'update_record(crm_leave_request) failed: Record 9SEmlyRfw8D9-J7Z not found'; + + +describe('#15970 — a recall whose resume strands carries the discriminator', () => { + let data: ReturnType; + let service: ApprovalService; + let logger: ReturnType; + let marks: string[]; + let rejectBranchThrows: string | undefined; + + /** One live process: real engine, real approval node, real approvals service. */ + function boot() { + const automation = new AutomationEngine(logger, new InMemorySuspendedRunStore()); + registerApprovalNode(automation, service, logger); + 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; + } + + beforeEach(() => { + marks = []; rejectBranchThrows = undefined; + logger = recordingLogger(); + data = makeFakeEngine(); + service = new ApprovalService({ engine: data as any, logger }); + }); + + async function park(automation: AutomationEngine) { + await automation.execute('deal_approval', { + object: 'crm_deal', record: { id: 'd1', amount: 100 }, userId: 'submitter', + } as never); + return (await data.find('sys_approval_request', { where: { status: 'pending' } }))[0]; + } + + it("PIN 1 — the card's table, row for row, with the machine-readable half filled", async () => { + rejectBranchThrows = DOWNSTREAM_FAILURE; + const automation = boot(); + const req = await park(automation); + const runId = req.flow_run_id as string; + + const outcome = await service + .recall(req.id, { actorId: 'submitter' }, SYSTEM_CTX) + .then(r => ({ ok: true as const, r }), (e: Error) => ({ ok: false as const, e })); + + // ── ROW 1: the call. ⛔ Unchanged by this card and asserted so it stays + // unchanged: the ruling upholds the no-throw, and a fix that made this + // door fail would be the wrong fix, not a stricter one. + expect(outcome.ok, 'a recall abandons the request — a lost run must not fail the call').toBe(true); + const result = outcome.ok ? outcome.r : (undefined as never); + + // ── ROWS 2-3: what the caller was ALREADY told, both unchanged. + expect(result.resumed, 'this run really was not resumed').toBe(false); + expect(result.resumeError, 'the prose half is still there').toContain(runId); + expect(result.resumeError).toContain(DOWNSTREAM_FAILURE); + + // ── ROW 4: the withdrawal is durable — the whole reason the call tolerates + // the failed resume in the first place. + expect(result.request.status).toBe('recalled'); + + // ── ROW 5: ⛔ `strandedDecisionDetails` still answers `undefined`, and that + // is CORRECT rather than unfixed. It is the ERROR-envelope reader for a + // door that THROWS (#13807) — it reads a carrier property off a thrown + // error — so against any recall RESULT it can only ever answer + // `undefined`, whatever this door does. The ruling put the success-answer + // carrier on `resumeFailure` for exactly that reason (the spec's own + // "⛔ Not `StrandedDecisionDetails`" note). + expect(strandedDecisionDetails(result as unknown)).toBeUndefined(); + + // ── ROWS 6-7: the run really is stranded, and really is repairable — the + // two facts the caller had no way to learn. + expect(await automation.hasSuspendedRun(runId), 'the pause was consumed and not re-armed').toBe(false); + expect(marks, 'the reject branch never finished — real abandoned work').toEqual([]); + + // ── THE FIX. Every member, and the `runId` by identity rather than by + // truthiness: naming the wrong run is the failure mode #15556 measured + // one door over. + expect(result.resumeFailure).toEqual({ + code: 'RESUME_FAILED', + runId, + status: 'stranded', + repairable: true, + }); + // Presence is the signal (the member's docblock): omitted, never set to + // `undefined`, so `in` reads it correctly. + expect(Object.prototype.hasOwnProperty.call(result, 'resumeFailure')).toBe(true); + + // ── `repairable: true` is a PROMISE, so it is cashed here rather than + // asserted as a literal: the verb it names actually re-arms this run. + const restored = await automation.restoreConsumedSuspension(runId, { requestedBy: 'ops' }); + expect(restored.restored, 'the discriminator promised a repair that exists').toBe(true); + expect(await automation.hasSuspendedRun(runId)).toBe(true); + + // ── ⛔ The log is untouched by this card — same message, same `error` + // level, same context keys. The ruling left logging alone, and the + // report is a SIBLING of the log line, not a replacement for it. + const failedLines = logger.lines.filter(l => l.level === 'error' + && l.msg === '[approvals] resume after recall failed — the run may be stranded'); + expect(failedLines.length, 'said once, at error').toBe(1); + expect(Object.keys(failedLines[0].meta as object).sort()).toEqual(['error', 'request', 'run']); + }); + + it('PIN 2 — a healthy recall reports nothing, so PIN 1 is a reading and not the fixture', async () => { + const automation = boot(); + const req = await park(automation); + const runId = req.flow_run_id as string; + + const result = await service.recall(req.id, { actorId: 'submitter' }, SYSTEM_CTX); + + expect(result.resumed, 'the reject edge was walked to the end').toBe(true); + expect(marks).toEqual(['mark_rejected']); + expect(result.request.status).toBe('recalled'); + expect(result.resumeError).toBeUndefined(); + // Absence is the correct reading for a run with nothing to report — and it + // is what makes PIN 1's presence a measurement. + expect(result.resumeFailure).toBeUndefined(); + expect(Object.prototype.hasOwnProperty.call(result, 'resumeFailure')).toBe(false); + expect(await automation.hasSuspendedRun(runId)).toBe(false); + expect(logger.lines.filter(l => l.level === 'error')).toEqual([]); + }); + + it('PIN 3 — a resume failure the engine does NOT call stranded reports no code at all', async () => { + // POPULATION: the run behind the request is gone by the time the recall + // reaches it, so the engine answers `RUN_NOT_FOUND` — a failure with a + // `code` and NO `status`. + // + // ⛔ This exit deliberately carries no `resumeFailure`, and the reason is a + // boundary rather than an omission: its honest code is + // `RESUME_TARGET_LOST` (the member's own docblock says so), and this + // package's ADR-0112 ledger row admits exactly one code — `RESUME_FAILED` + // (`error-code-ledger.zod.ts`, `'@objectstack/plugin-approvals'`, whose + // comment spells out that this package stamps neither of the other two). + // Stamping `RESUME_FAILED` here to fill the slot would make the + // discriminator lie about WHICH failure this was — the defect PIN 1 fixes, + // one field over. Absence is declared legitimate by the member's docblock + // ("An absent member means no report was made, never that no run is + // stranded") and is exactly what `resumeRecordedOutcome` answers on the + // same exits. + const automation = boot(); + const req = await park(automation); + const runId = req.flow_run_id as string; + expect(await automation.cancelRun(runId), 'the run is gone before the recall lands').toBe(true); + + const result = await service.recall(req.id, { actorId: 'submitter' }, SYSTEM_CTX); + + expect(result.resumed).toBe(false); + expect(result.resumeError, 'still told, in prose').toMatch(/RUN_NOT_FOUND/); + expect(result.request.status, 'and the withdrawal is still durable').toBe('recalled'); + expect(result.resumeFailure).toBeUndefined(); + }); + + it('CONTROL — the identical strand through `decide` throws the #13807 envelope', async () => { + // Without this the recall readings above could not be attributed to the + // DOOR: a fixture that never really strands anything produces the same + // "no report" answer. Same lever, same flow, same engine — different door. + rejectBranchThrows = DOWNSTREAM_FAILURE; + const automation = boot(); + const req = await park(automation); + const runId = req.flow_run_id as string; + + const err = await service + .decide(req.id, { decision: 'reject', actorId: 'u1', comment: 'no' }, SYSTEM_CTX) + .then(() => null, (e: Error) => e); + + expect(err, 'this door reports the same strand by THROWING').toBeTruthy(); + expect(err?.message).toMatch(/^RESUME_FAILED/); + expect(strandedDecisionDetails(err)).toEqual({ + finalized: true, decision: 'reject', runId, repairable: true, + }); + // The strand itself is identical on both doors — which is the card's claim: + // the difference was the door, never the strand. + expect(await automation.hasSuspendedRun(runId)).toBe(false); + expect((await automation.restoreConsumedSuspension(runId, { requestedBy: 'ops' })).restored).toBe(true); + }); +}); diff --git a/packages/plugins/plugin-approvals/src/subflow-hosted-approval-strand.test.ts b/packages/plugins/plugin-approvals/src/subflow-hosted-approval-strand.test.ts index 5a78ed5108..303b5070b5 100644 --- a/packages/plugins/plugin-approvals/src/subflow-hosted-approval-strand.test.ts +++ b/packages/plugins/plugin-approvals/src/subflow-hosted-approval-strand.test.ts @@ -350,4 +350,65 @@ describe('#15556 — an approval hosted in a subflow child, whose parent bubble finalized: true, decision: 'approve', runId: req.flow_run_id, repairable: true, }); }); + + it('[#15970] the RECALL door carries the same parent strand — the door was the only difference', async () => { + // This shape's second door. #17908 landed `takeSubflowParentStrand` and + // wired it into `resumeRecordedOutcome`, which serves `decide` / `sendBack` + // / `resubmit`; `recall` resumes DIRECTLY and discarded the value the very + // same call already returned to it. The spec declares this door's answer + // for exactly this shape (`ApprovalRecallResult.resumed`: *"a resume that + // completed and then stranded a run further up (a subflow's parent, #15556) + // still answers `true`, with the strand told on `resumeFailure`"*), so a + // recall that reported nothing here was a declared contract with no + // producer. + throwOn.after_sub = DOWNSTREAM_FAILURE; + const automation = boot(); + + const started = await automation.execute('deal_parent', { + object: 'crm_deal', record: { id: 'd4', amount: 100 }, userId: 'submitter', + } as never); + const parentRunId = (started as any).runId as string; + const req = await pendingRequest(); + const childRunId = req.flow_run_id as string; + expect(childRunId).not.toBe(parentRunId); + + const outcome = await service + .recall(req.id, { actorId: 'submitter' }, SYSTEM_CTX) + .then(r => ({ ok: true as const, r }), (e: Error) => ({ ok: false as const, e })); + + expect(outcome.ok, 'recall still does not throw — the ruling upholds that').toBe(true); + const answer = outcome.ok ? outcome.r : (undefined as never); + + // The child ran its reject branch to completion; the PARENT is the casualty. + expect(answer.resumed, "this recall's own run — the child — really did resume").toBe(true); + expect(answer.runId, 'the id handed back is still the CHILD').toBe(childRunId); + expect(marks).toEqual(['mark_rejected']); + expect(await automation.hasSuspendedRun(parentRunId)).toBe(false); + expect((await automation.getRun(childRunId))?.status).toBe('completed'); + expect(answer.request.status, 'and the withdrawal is durable either way').toBe('recalled'); + + // ── Both halves of the ONE telling, the PARENT's id on each. + expect(answer.resumeFailure).toEqual({ + code: 'RESUME_FAILED', + runId: parentRunId, + status: 'stranded', + repairable: true, + }); + expect(answer.resumeError).toContain('RESUME_FAILED'); + expect(answer.resumeError).toContain(parentRunId); + expect(answer.resumeError).toContain(DOWNSTREAM_FAILURE); + + // ⛔ No second log line from the approvals side: the engine's own `error` + // line already said this once, and the #16472 ruling left logging alone. + const durability = logger.lines.filter( + (l: any) => l.level === 'error' && String(l.msg).includes('STRANDED'), + ); + expect(durability.length, "exactly one, and it is the engine's own line").toBe(1); + expect(logger.lines.filter( + (l: any) => l.level === 'error' && String(l.msg).includes('resume after recall failed'), + ), 'the recall catch never ran — this resume SUCCEEDED').toEqual([]); + + // …and the repair the discriminator promises works on the run it names. + expect((await automation.restoreConsumedSuspension(parentRunId, { requestedBy: 'ops' })).restored).toBe(true); + }); }); From 90151d502a27c5c3948124776ff0c5fec6eaa12e Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 13 Sep 2026 08:52:29 +0000 Subject: [PATCH 3/4] chore(gates): record the new approvals engine double in the pinned ledger `check:engine-double-contract` names its own remedy: a new test file that pins its double to ObjectQL's dispatch predicates is new coverage the ledger has to learn about, or it never protects that file. Regenerated with `--write`; 0 seam rows added or grown, 0 lost. Claude-Session: https://claude.ai/code/session_01URLHobLUJB9K1ABV6ofdjj Co-authored-by: Claude --- scripts/engine-double-contract.pinned.json | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/scripts/engine-double-contract.pinned.json b/scripts/engine-double-contract.pinned.json index 42f64f365d..68b8a906d9 100644 --- a/scripts/engine-double-contract.pinned.json +++ b/scripts/engine-double-contract.pinned.json @@ -2271,6 +2271,16 @@ "verb": "update", "pinned": 1 }, + { + "file": "packages/plugins/plugin-approvals/src/recall-strand-discriminator.test.ts", + "verb": "delete", + "pinned": 1 + }, + { + "file": "packages/plugins/plugin-approvals/src/recall-strand-discriminator.test.ts", + "verb": "update", + "pinned": 1 + }, { "file": "packages/plugins/plugin-approvals/src/record-reader-visibility.test.ts", "verb": "delete", From 76ee9926e554c84f15ce1638296973ea5b487773 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 13 Sep 2026 09:02:41 +0000 Subject: [PATCH 4/4] test(approvals): type the recall pin's captured log lines MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `tsconfig.test.json` covers this file, so the two `filter` callbacks were TS7006 implicit-any. Give `recordingLogger` a declared line shape instead of annotating the parameters `any` — the debt ledger is shrink-only and a new entry is a maintainer's call, not a way to get green. Claude-Session: https://claude.ai/code/session_01URLHobLUJB9K1ABV6ofdjj Co-authored-by: Claude --- .../src/recall-strand-discriminator.test.ts | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/packages/plugins/plugin-approvals/src/recall-strand-discriminator.test.ts b/packages/plugins/plugin-approvals/src/recall-strand-discriminator.test.ts index 7d46d43ad0..046120e991 100644 --- a/packages/plugins/plugin-approvals/src/recall-strand-discriminator.test.ts +++ b/packages/plugins/plugin-approvals/src/recall-strand-discriminator.test.ts @@ -53,12 +53,15 @@ import { registerApprovalNode } from './approval-node.js'; const SYSTEM_CTX = { isSystem: true, positions: [], permissions: [] } as any; +/** One captured log line — typed so the assertions below need no `any` parameter. */ +interface LoggedLine { level: string; msg: string; meta?: unknown } + /** Captures the door's log lines so PIN 1 can assert the level was left alone. */ function recordingLogger() { - const lines: { level: string; msg: string; meta?: unknown }[] = []; + const lines: LoggedLine[] = []; const at = (level: string) => (msg: unknown, meta?: unknown) => void lines.push({ level, msg: String(msg), meta }); - return { lines, info: at('info'), warn: at('warn'), error: at('error'), debug: at('debug') } as any; + return { lines, info: at('info'), warn: at('warn'), error: at('error'), debug: at('debug') }; } /** In-memory ObjectQL stand-in for the approvals tables. */ @@ -174,8 +177,8 @@ describe('#15970 — a recall whose resume strands carries the discriminator', ( /** One live process: real engine, real approval node, real approvals service. */ function boot() { - const automation = new AutomationEngine(logger, new InMemorySuspendedRunStore()); - registerApprovalNode(automation, service, logger); + const automation = new AutomationEngine(logger as any, new InMemorySuspendedRunStore()); + registerApprovalNode(automation, service, logger as any); automation.registerNodeExecutor({ type: 'mark', async execute(node: any) { @@ -193,7 +196,7 @@ describe('#15970 — a recall whose resume strands carries the discriminator', ( marks = []; rejectBranchThrows = undefined; logger = recordingLogger(); data = makeFakeEngine(); - service = new ApprovalService({ engine: data as any, logger }); + service = new ApprovalService({ engine: data as any, logger: logger as any }); }); async function park(automation: AutomationEngine) {