diff --git a/.changeset/spec-approval-continue-restored-contract.md b/.changeset/spec-approval-continue-restored-contract.md new file mode 100644 index 00000000000..fc5e4198d1e --- /dev/null +++ b/.changeset/spec-approval-continue-restored-contract.md @@ -0,0 +1,9 @@ +--- +'@objectstack/spec': minor +--- + +Declare `continueRestoredRun` on the `IApprovalService` contract, so the approvals half of the operator repair pair is reachable through the published interface rather than only off the implementation class. + +`IAutomationService.restoreConsumedSuspension` re-arms the pause a failed resume consumed and, by its own contract, does not replay the resume signal — the continuation must be re-issued. For an approval suspension nothing could re-issue it: every front door guards on a live request — `pending` for decide and send-back, `returned` for resubmit, and `pending` or the revise window for recall — and the stranding call leaves the row where none of them can issue the continuation it owes. The issuer landed as a class member on `plugin-approvals`; this declares it, so a caller programs against the contract instead of importing the implementation. + +Additive and OPTIONAL, the way `cancelRun` / `restoreConsumedSuspension` are declared on `IAutomationService`: an existing implementation still conforms, and a service that does not declare the member has no operator door for it — a caller must probe for presence and refuse fail-closed rather than answer success for a verb it could not dispatch, because promising a repair verb that will refuse is worse than promising nothing. No REST or CLI route is declared or implied. diff --git a/packages/plugins/plugin-approvals/src/approval-service.ts b/packages/plugins/plugin-approvals/src/approval-service.ts index aecce0055f7..0f4aacb530c 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.ts @@ -4726,10 +4726,11 @@ export class ApprovalService implements IApprovalService { * `AutomationEngine.restoreConsumedSuspension` puts a stranded approval run * back on its pause and tells the operator to *re-issue the continuation* — * but for an `approval` node the only issuers are this service's doors, and - * every one of them guards on a `pending` request that the stranding call - * itself just made terminal. Re-opening the row is excluded (it would let a - * decided request be decided again), so what is kept instead is the SIGNAL: - * the exact `branchLabel` + `output` the failed resume carried. + * every one of them guards on a live status — `pending`, or `returned` for + * the revise-window doors — which the stranding call left in no state to + * issue the continuation it owes. Re-opening the row is excluded (it would + * let a decided request be decided again), so what is kept instead is the + * SIGNAL: the exact `branchLabel` + `output` the failed resume carried. * * ⚠️ Best-effort by construction, and it must stay that way: the decision is * already durable and its caller is already owed a `RESUME_FAILED` throw. A @@ -5052,9 +5053,14 @@ export class ApprovalService implements IApprovalService { * `true` — and its own reason string tells the operator to *re-issue the * continuation*. For an `approval` node there was then nobody who could: * - * - `decide` / `recall` / `sendBack` / `resubmit` all guard on a `pending` - * request, and the row is terminal — written by the very call that - * stranded the run; + * - `decide` / `recall` / `sendBack` / `resubmit` each guard on a LIVE + * request — `pending` for `decide` and `sendBack`, `returned` for + * `resubmit`, and `pending` or the revise window for `recall` — and the + * stranding call left the row where none of them can issue the + * continuation it owes. ⚠️ Not because the row is never `returned`: a + * stranded send-back leaves it exactly there, and `resubmit` is still + * no way back — submitter-only, and it owes the `resubmit` edge where + * the stranded continuation was the `revise` one; * - the generic `engine.resume` refuses, because the `approval` node * declares `resumeAuthority: 'service'` and the #3801 gate turns away any * resume that is not the tail of a decision this service authorized. @@ -5068,7 +5074,7 @@ export class ApprovalService implements IApprovalService { * ## What it deliberately does NOT do * * ⛔ It does not re-open, re-decide, or rewrite the request row: all four - * `pending` guards stay exactly as they are, and no status, mirror field or + * status guards stay exactly as they are, and no status, mirror field or * audit row is written. A person decided this once; this replays what they * decided onto the pause that was put back, and replays nothing else. * ⛔ It does not relax `resumeAuthority: 'service'` — the resume goes through @@ -5100,9 +5106,17 @@ export class ApprovalService implements IApprovalService { * * Deliberately shaped like the engine verb it completes: an in-process * operator repair, reachable from a host or a console script, with no REST - * route and no entry in the spec `ApprovalService` contract — exactly as - * `restoreConsumedSuspension` is a class method on `AutomationEngine` and - * appears in no contract. It authorizes nothing new: the decision it replays + * route. ⚠️ That last part is where it DIFFERS from + * `restoreConsumedSuspension`, which does have a platform-operator door — + * `POST /:name/runs/:runId/restore-suspension`, the #13953 services half. + * This verb has none: the #15389 ruling of 2026-09-09 refused a REST/CLI + * route for it, so a door for it would be a new card. Both verbs are + * DECLARED on their spec contracts as OPTIONAL members — + * `IAutomationService.restoreConsumedSuspension` by #16495, and this one on + * `IApprovalService` by that same ruling — which declares the + * capability without opening a door: a caller reaching this verb through the + * contract must probe for presence and refuse fail-closed when it is absent. + * It authorizes nothing new: the decision it replays * was authorized and recorded when it was made, and re-authorizing it here * against a present-day actor would be a different and wrong question (the * original approver may be long gone). `requestedBy` / `reason` ride the log diff --git a/packages/spec/src/contracts/approval-service.test.ts b/packages/spec/src/contracts/approval-service.test.ts index 785018248ce..1f882051eea 100644 --- a/packages/spec/src/contracts/approval-service.test.ts +++ b/packages/spec/src/contracts/approval-service.test.ts @@ -9,8 +9,16 @@ // ?? null` — a resolved org id, or `null` when none resolved, or absent on // rows written before stamping existed). +import { readFileSync } from 'node:fs'; +import { fileURLToPath } from 'node:url'; + import { describe, it, expect, expectTypeOf } from 'vitest'; -import type { ApprovalActionRow, ApprovalRequestRow } from './approval-service'; +import type { + ApprovalActionRow, + ApprovalRequestRow, + ApprovalStatus, + IApprovalService, +} from './approval-service'; describe('approval row organization_id declaration (#10331)', () => { it('is readable off ApprovalRequestRow without a cast, at the stamped shape', () => { @@ -54,3 +62,182 @@ describe('approval row organization_id declaration (#10331)', () => { expect(read({ ...minimal, organization_id: 'o_plant' })).toBe('o_plant'); }); }); + +// --------------------------------------------------------------------------- +// [#15389] `continueRestoredRun` — the approvals half of the operator repair +// pair, declared on the contract. +// +// The maintainer ruling of 2026-09-09 (decision batch #106, item 3) settled +// option A: the verb is declared on `IApprovalService` as an OPTIONAL member, +// with the shape and docblock discipline #16495 gave +// `IAutomationService.cancelRun` / `restoreConsumedSuspension`. These pins are +// that block's sibling, and they hold the three things the ruling actually +// decided: the member exists, it is optional, and it carries the ruled posture +// in its docblock — including the note that promising a repair verb that will +// refuse is worse than promising nothing. +// +// The type-level identities are exported aliases for the same reason the +// #16495 block's are: an unread alias inside a test body is TS6196, and a pin +// no program compiles is no pin at all. `check:test-typecheck` compiles this +// file. +// --------------------------------------------------------------------------- + +type Eq = (() => T extends A ? 1 : 2) extends (() => T extends B ? 1 : 2) ? true : false; +type Assert = T; +type ContinueRestoredRun = NonNullable; + +/** + * `continueRestoredRun(requestId, options?)` — who asked and why travel + * through the contract, exactly as they do on the engine verb this completes. + */ +export type ContinueTakesRequestIdAndOptions = Assert< + Eq, [requestId: string, options?: { requestedBy?: string; reason?: string }]> +>; + +/** + * The replay result: what moved, which run, which outcome and edge, and + * whether the signal was the literal one or was rebuilt. A dropped or widened + * member turns this alias red. + */ +export type ContinueAnswersTheReplayResult = Assert< + Eq< + Awaited>, + { + resumed: boolean; + runId: string; + decision: string; + branchLabel?: string; + source: 'journal' | 'reconstructed'; + resumeError?: string; + } + > +>; + +/** A conforming request row, at the minimum the contract requires. */ +const requestRow = (id: string, status: ApprovalStatus): ApprovalRequestRow => ({ + id, + process_name: 'flow:expense_review', + object_name: 'expense', + record_id: 'rec_1', + status, +}); + +/** + * The smallest thing that satisfies `IApprovalService` — every REQUIRED member + * and nothing else. It is the population the optionality pin is about: a + * service with no operator repair verb still conforms. + */ +const minimalService = (): IApprovalService => ({ + listRequests: async () => [], + countRequests: async () => 0, + getRequest: async () => null, + decide: async (requestId) => ({ request: requestRow(requestId, 'approved'), finalized: true, decision: 'approve' }), + recall: async (requestId) => ({ request: requestRow(requestId, 'recalled') }), + sendBack: async (requestId) => ({ request: requestRow(requestId, 'returned') }), + resubmit: async (requestId) => ({ request: requestRow(requestId, 'returned') }), + reassign: async (requestId) => ({ request: requestRow(requestId, 'pending') }), + remind: async (requestId) => ({ request: requestRow(requestId, 'pending'), notified: 0 }), + requestInfo: async (requestId) => ({ request: requestRow(requestId, 'pending') }), + comment: async (requestId) => ({ request: requestRow(requestId, 'pending') }), + listActions: async () => [], +}); + +describe('[#15389] continueRestoredRun — the approvals operator repair verb, declared', () => { + it('is optional: the minimal implementation still conforms and has no operator door', () => { + const service = minimalService(); + + // The ruled optionality. A door standing in front of THIS service has + // to probe and refuse fail-closed — it may not call and report success. + expect(service.continueRestoredRun).toBeUndefined(); + }); + + it('carries the signature through the contract — who asked, and why, reach the implementation', async () => { + const seen: Array> = []; + const service: IApprovalService = { + ...minimalService(), + continueRestoredRun: async (requestId, options) => { + seen.push({ requestId, ...options }); + return requestId === 'req_journalled' + ? { + resumed: true, + runId: 'run_stranded', + decision: 'reject', + branchLabel: 'reject', + source: 'journal', + } + : { + resumed: true, + runId: 'run_stranded', + decision: 'reject', + branchLabel: 'reject', + source: 'reconstructed', + resumeError: 'a concurrent resume is already advancing this run', + }; + }, + }; + + const exact = await service.continueRestoredRun!('req_journalled', { + requestedBy: 'ops@example.com', + reason: 'notify node fixed; re-issuing the recorded rejection', + }); + expect(exact.resumed).toBe(true); + expect(exact.runId).toBe('run_stranded'); + // The outcome is REPLAYED, never re-decided: it is the one already on + // the row, and the edge it walks is the one it always walked. + expect(exact.decision).toBe('reject'); + expect(exact.branchLabel).toBe('reject'); + // `journal` is the literal re-issue; `reconstructed` is the inferred + // one. A caller that cannot tell them apart cannot say what it trusts. + expect(exact.source).toBe('journal'); + expect(exact.resumeError).toBeUndefined(); + + const rebuilt = await service.continueRestoredRun!('req_pre_journal'); + expect(rebuilt.source).toBe('reconstructed'); + expect(rebuilt.resumeError).toContain('concurrent resume'); + + // The optional parameters ARE the reason the signature follows the + // implementation: an operator repair records who asked and why. + expect(seen).toEqual([ + { + requestId: 'req_journalled', + requestedBy: 'ops@example.com', + reason: 'notify node fixed; re-issuing the recorded rejection', + }, + { requestId: 'req_pre_journal' }, + ]); + }); + + it('refuses a replay result that omits `source` (compile-time, under check:test-typecheck)', () => { + const service: IApprovalService = { + ...minimalService(), + // @ts-expect-error — `source` is required: a caller told a run moved, but not whether the + // signal was the literal one or a rebuild, cannot tell an exact replay from an inferred one. + continueRestoredRun: async (_requestId) => ({ resumed: true, runId: 'run_stranded', decision: 'reject' }), + }; + + expect(service.continueRestoredRun).toBeDefined(); + }); + + it('the docblock carries the ruled posture: no re-decision, no door, and the promising-nothing note', () => { + const source = readFileSync(fileURLToPath(new URL('./approval-service.ts', import.meta.url)), 'utf8'); + const at = source.indexOf('continueRestoredRun?('); + expect(at).toBeGreaterThan(-1); + // The doc block immediately above the declaration — from its last `/**`. + const doc = source.slice(source.lastIndexOf('/**', at), at); + + // The sibling this was ruled to copy, named where a later author reads it. + expect(doc).toContain('#16495'); + // What the engine verb leaves undone, which is the whole reason this exists. + expect(doc).toMatch(/the[\s*]+continuation[\s*]+must[\s*]+be[\s*]+re-issued/); + // ⛔ It replays a recorded outcome; it does not re-decide. + expect(doc).toMatch(/does not re-open, re-decide or rewrite the request row/); + expect(doc).toMatch(/does not relax the node's `resumeAuthority: 'service'`/); + // Optional ⇒ absent means no door, and the door refuses fail-closed. + expect(doc).toMatch(/NO[\s*]+operator door/); + expect(doc).toMatch(/refuse[\s*]+fail-closed/); + // The #16495 note, in its own words — the reason optionality is not a shrug. + expect(doc).toMatch(/promising a repair verb that will refuse is worse[\s*]+than promising nothing/); + // C was refused: declaring the member is not declaring a route. + expect(doc).toMatch(/refused a REST\/CLI route/); + }); +}); diff --git a/packages/spec/src/contracts/approval-service.ts b/packages/spec/src/contracts/approval-service.ts index f64b7fb0d54..72902555176 100644 --- a/packages/spec/src/contracts/approval-service.ts +++ b/packages/spec/src/contracts/approval-service.ts @@ -944,4 +944,102 @@ export interface IApprovalService { /** Audit trail for a request. */ listActions(requestId: string, context: ExecutionContext): Promise; + + /** + * **Operator verb — re-issue the continuation for a run an operator has + * re-armed** (#15389; the maintainer ruling of 2026-09-09, decision batch + * #106 item 3, declared here exactly as #16495 declared + * `IAutomationService.cancelRun` / `restoreConsumedSuspension`). + * + * The missing half of `IAutomationService.restoreConsumedSuspension`, for + * approvals. That verb re-arms the pause a failed resume consumed and, by + * its own contract, deliberately does NOT replay the resume signal — the + * continuation must be re-issued. For an `approval` suspension there was + * then nobody who could: {@link decide}, {@link recall}, {@link sendBack} + * and {@link resubmit} each guard on a LIVE request — `pending` for + * {@link decide} and {@link sendBack}, `returned` for {@link resubmit}, + * and `pending` or the revise window for {@link recall} — and the stranding + * call left the row where none of them can issue the continuation it owes. + * ⚠️ Not because the row is never `returned`: a stranded send-back leaves + * it exactly there, and {@link resubmit} is still no way back — it is + * submitter-only, and it owes the `resubmit` edge where the stranded + * continuation was the `revise` one. Meanwhile a generic engine resume is + * refused at a node declaring `resumeAuthority: 'service'`. The only verb + * left was cancel, which discards the branch's downstream work. This is the + * issuer that exits that dead end. + * + * What it does, exactly, and what it does not: + * - it replays the outcome the request ALREADY recorded onto the pause that + * was put back, and replays nothing else; + * - ⛔ it does not re-open, re-decide or rewrite the request row — no + * status, no mirror field and no audit row is written, and a decided + * request still cannot be decided again through the front door; + * - ⛔ it does not relax the node's `resumeAuthority: 'service'`: the resume + * is the implementation's own, through the same single call site that + * stamps that marker; + * - it never answers a silent `false` — a resume that fails again throws + * the same `RESUME_FAILED` envelope the original decision did, + * `repairable` and all, so a second restore-and-continue is possible. + * + * **Why it takes no {@link ExecutionContext}** — deliberately shaped like + * the engine verb it completes. The decision it replays was authorized and + * recorded when it was made; re-authorizing it here against a present-day + * actor would be a different and wrong question, because the original + * approver may be long gone. `requestedBy` / `reason` ride the + * implementation's log for the reason they do on the restore: an operator + * repair records who asked, and why. + * + * **No door is declared here, and none is ruled** — the 2026-09-09 ruling + * refused a REST/CLI route for this verb. It is an in-process operator + * repair, reachable from a host or a console script; a route for it is a new + * card, never a widening at a call site. + * + * **Optional, deliberately** — for the reason `cancelRun` and + * `restoreConsumedSuspension` are. Re-issuing a consumed continuation is a + * capability of an approvals implementation that can identify the pause it + * was refused on; a service that does not declare this member has NO + * operator door for it, and a door MUST probe for presence and refuse + * fail-closed when it is absent — never answer success for a verb it could + * not dispatch, because promising a repair verb that will refuse is worse + * than promising nothing. + * + * @param requestId - The terminal request whose recorded outcome is to be + * re-issued; the run is the one that request has always named + * @param options.requestedBy - Who asked; logged + * @param options.reason - Why; logged the same way + * @returns what was replayed, and whether the run moved + */ + continueRestoredRun?( + requestId: string, + options?: { requestedBy?: string; reason?: string }, + ): Promise<{ + /** True when the restored pause was consumed and the flow moved on. */ + resumed: boolean; + /** The run this continued — the one the request has always named. */ + runId: string; + /** + * The outcome that was replayed, exactly as it was first recorded. + * Free-form by design, as it is on `StrandedDecisionDetails`: the + * vocabulary belongs to the producing service, not to this contract. + */ + decision: string; + /** The edge it walked, when the replayed signal names one. */ + branchLabel?: string; + /** + * Whether the replayed signal was the literal one the failing door sent + * (`journal`), or was rebuilt from the row's recorded outcome + * (`reconstructed`) — so a caller can tell an EXACT replay from an + * inferred one before it trusts what moved. + * + * ⚠️ Enumerated here, unlike `restoreConsumedSuspension`'s `refusal` + * (#16495 route (i)), and the difference is the point: that one is the + * implementation's own open refusal vocabulary, which this contract would + * have to keep in step with; this is a closed, binary property of the + * replay itself and the caller's branch point. A third provenance is a + * spec card, never a widening at a call site. + */ + source: 'journal' | 'reconstructed'; + /** Set only on the tolerated non-failure: a concurrent resume already had it. */ + resumeError?: string; + }>; }