Skip to content

Commit cbb91f2

Browse files
committed
fix(approvals): refuse a journalled continuation the row's status can no longer have issued
The stranded-continuation journal was returned before `raw.status` was read, and nothing clears it when a later door moves the row on — so a signal could outlive the state that issued it and be replayed onto a re-armed pause. Measured, with no injected failure beyond the strand itself: a `resubmit` strands and journals `resubmit`; the submitter then recalls, a real `cancelRun` on an already-stranded run answers `false`, so the row is marked `recalled` and the run stays parked; `restoreConsumedSuspension` re-arms the pause; and `continueRestoredRun` replayed the stale `resubmit`, opening a fresh `pending` round on a request somebody deliberately withdrew — statuses `["recalled","pending"]`. Every step an ordinary action answering ordinarily. The `revise` sibling (a stranded send-back, then a recall) did the same. A row is now replayable only for a continuation its own status can have issued (`approved`→`approve`, `rejected`→`reject`, `returned`→`revise`|`resubmit`, `recalled`→`recall`), and nothing at all for a status nobody has enumerated. ⛔ Clearing the journal after a successful replay was measured NOT to close this: the offending replay is the FIRST replay of that journal, so a clear that fires afterwards can never run before the advance it would prevent. With such a clear in place both limbs still reached `resumed: true`. PIN 9 pins both limbs end to end, plus the compatible control (a stranded recall journalling `recall` on a `recalled` row still replays, and its reject branch runs) so the check reads as a gate rather than a blanket refusal. Also in this commit: - Regenerated `content/docs/permissions/system-context.mdx` with the census gate's own `--fix` (8 anchors re-anchored) and proved the result a fixpoint: a second `--fix` rewrites 0 and leaves the blob byte-identical. `pnpm check:system-context-census` exit 0. - Corrected three prose claims: "checks that the pause is genuinely re-armed" (no guard checks that — a never-consumed pause is admitted); the retracted "its journalled signal is `recall`, not `resubmit`" (a recall inside the revise window journals nothing, so the journal there is whatever an earlier strand left); and the journal-recoverable named set, which now includes the pinned `recall` journal path. - Guard 3's refusal no longer presumes a re-arm and a refusal its own PIN 7(a) population does not contain: it names the pause it was ASKED to continue against the node the outcome was ISSUED AT. Refs #15389 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y
1 parent 6e63090 commit cbb91f2

4 files changed

Lines changed: 256 additions & 17 deletions

File tree

.changeset/approvals-continue-restored-suspension.md

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,8 @@ Measured against the real engine and the real decision door: the restored suspen
1111
- **`ApprovalService.continueRestoredRun(requestId, options?)`** re-issues the continuation the recorded outcome already produced once, against a pause an operator has re-armed. It reports which outcome it replayed, which edge it walked, and whether the signal was replayed exactly or rebuilt (`source: 'journal' | 'reconstructed'`).
1212
- **The failing door now journals the signal it was carrying** on the repairable exit — the engine's own `status: 'stranded'` discriminator, the one exit that journals a repair snapshot — under `__strandedContinuation` in the request's `node_config_json`, beside the `__decisionOutputs` side-channel that was already there. Best-effort: it is awaited but can never replace the `RESUME_FAILED` throw the decision's caller is owed.
1313
- **The continuation is tied to this request's own pause, by three guards.** A boolean "is this run suspended" is not enough: a run outlives any one request, so a terminal row's continuation could be issued against whatever pause the run happened to be sitting on. It now requires that the request is still the newest on its run, that a pause exists (strictly — an unreadable store throws rather than reading as "not suspended"), and that the pause is parked **where this request's recorded outcome was issued from**. That node is signal-aware, not simply the row's own: `approve`, `reject`, `revise` and `recall` are all issued at the request's own approval node, but a `resubmit` is only ever issued from the revise window the request's `revise` edge leads to, so its pause is re-armed there while the row still records the approval node. Comparing against the row's own node refused exactly that case, and told the operator the pause was not this request's when it was. The node check is fail-closed in every direction, including an engine that cannot report where a run is parked and a revise window this service cannot derive from the flow definition. This needs no new automation-engine surface: `listSuspendedRunsDurable` is already public, and the approvals-side resume interface simply declares it.
14-
- **Runs stranded before this shipped are served too**, and where the signal cannot be proved the verb **refuses instead of guessing**. A status is not the same thing as a continuation, and three of the four terminal statuses have more than one writer or issuer: `approved` is unambiguous; `rejected` has two writers, discriminated by the `revise` action row that only ADR-0044's revision-limit auto-rejection leaves behind; `returned` has one writer but **two** issuers, discriminated by the `resubmit` action row whose sole writer is `resubmit` — without it a stranded resubmit was rebuilt as a send-back and walked the wrong edge, proceeding only through the engine's unmatched-label fallback with the wrong output; and `recalled` has two writers across **three** behaviours, two of which issue no continuation at all, so it is **refused on the rebuild path** with a message naming what an operator can do instead. Journal-recoverable is a **measured, named set** rather than a blanket claim: `approve`, `reject` and `resubmit` continuations replay end to end through the verb, and `reject` and `resubmit` do so on the rebuild path as well. Two shapes are refused by design and stay refused — a `rejected` row that also carries a `revise` action, and a `recalled` row with no journal. NOT covered by a pin, and so not claimed: the `approve` rebuild path and the `recall` journal path.
14+
- **Runs stranded before this shipped are served too**, and where the signal cannot be proved the verb **refuses instead of guessing**. A status is not the same thing as a continuation, and three of the four terminal statuses have more than one writer or issuer: `approved` is unambiguous; `rejected` has two writers, discriminated by the `revise` action row that only ADR-0044's revision-limit auto-rejection leaves behind; `returned` has one writer but **two** issuers, discriminated by the `resubmit` action row whose sole writer is `resubmit` — without it a stranded resubmit was rebuilt as a send-back and walked the wrong edge, proceeding only through the engine's unmatched-label fallback with the wrong output; and `recalled` has two writers across **three** behaviours, two of which issue no continuation at all, so it is **refused on the rebuild path** with a message naming what an operator can do instead. Journal-recoverable is a **measured, named set** rather than a blanket claim: `approve`, `reject`, `resubmit` and `recall` continuations replay end to end through the verb, and `reject` and `resubmit` do so on the rebuild path as well. Two shapes are refused by design and stay refused — a `rejected` row that also carries a `revise` action, and a `recalled` row with no journal. NOT covered by a pin, and so not claimed: the `approve` rebuild path.
1515

16-
⛔ What this deliberately does not do, each pinned: it does not re-open or rewrite the request row — all four `pending` guards are untouched and no status, mirror field or audit row is written, so a decided request still cannot be decided again through the front door; it does not relax `resumeAuthority: 'service'`, since the resume still goes through the one call site that stamps the marker; and it does not change `ApprovalDecisionResult`, whose shape is the subject of an open ruling. It also grants no capability in-process code did not already have — `RESUME_AUTHORITY_SERVICE` is importable by any host — what it adds is the guarded form, which checks that the pause is genuinely re-armed and that the signal matches what was recorded. Like the engine verb it completes, it is an in-process operator repair: no REST route, and no entry in the spec `ApprovalService` contract.
16+
- **A journalled signal is checked against what the row's status can have issued, before it is replayed.** The journal records what the last FAILED resume was carrying, and nothing rewrites it when a later door moves the row on — so a signal can outlive the state that issued it. Measured, with no injected failure beyond the strand: a `resubmit` strands and journals `resubmit`; the submitter then recalls, a real `cancelRun` on an already-stranded run answers `false`, the row is marked `recalled` and the run stays parked; the restore re-arms the pause; and the stale `resubmit` was replayed, opening a fresh `pending` round on a request somebody deliberately withdrew. Every step an ordinary action answering ordinarily. A row is now replayable only for a continuation its own status can have issued — `approved`→`approve`, `rejected`→`reject`, `returned`→`revise` or `resubmit`, `recalled`→`recall`, and nothing at all for a status nobody has enumerated. ⛔ Clearing the journal after a successful replay does not close this and was measured not to: the offending replay is the FIRST replay of that journal, so a clear that fires afterwards can never run before the advance it would prevent.
17+
18+
⛔ What this deliberately does not do, each pinned: it does not re-open or rewrite the request row — all four `pending` guards are untouched and no status, mirror field or audit row is written, so a decided request still cannot be decided again through the front door; it does not relax `resumeAuthority: 'service'`, since the resume still goes through the one call site that stamps the marker; and it does not change `ApprovalDecisionResult`, whose shape is the subject of an open ruling. It also grants no capability in-process code did not already have — `RESUME_AUTHORITY_SERVICE` is importable by any host — what it adds is the guarded form, and the guards are stated as what they actually check: that this request is still the newest on its run, that a pause exists at all, that it is parked where this outcome was issued from, and that the recorded signal is one the row's present status can have issued. ⛔ None of them checks that the pause was consumed and genuinely re-armed, and an earlier wording of this entry claimed one did: a `returned` row with a resubmit action row and a pause that was never consumed is admitted, with `restoreConsumedSuspension` itself answering *"already resumable — nothing to restore"*. That shape is benign — the recorded action is the submitter's own resubmit, so the step it walks was decided — but it is not what any guard tests. Like the engine verb it completes, it is an in-process operator repair: no REST route, and no entry in the spec `ApprovalService` contract.

content/docs/permissions/system-context.mdx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -145,7 +145,7 @@ The largest single consumer — **17 of the 105 sites**.
145145
|:--|:---|:---|:---|:---|
146146
| 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` |
147147
| 41 | Delegation write guard bypassed | plugin-approvals | Get: service / seed / import may write delegation rows naming another delegator | `lifecycle-hooks.ts:570` |
148-
| 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:1044`, `:1153`, `:3409`, `:3557`, `:3725`, `:3796`, `:3985`, `:4025` |
148+
| 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:1093`, `:1202`, `:3458`, `:3606`, `:3774`, `:3845`, `:4034`, `:4074` |
149149
| 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` |
150150
| 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` |
151151
| 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` |

packages/plugins/plugin-approvals/src/approval-service.ts

Lines changed: 92 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -162,10 +162,10 @@ export interface ApprovalResumeSurface {
162162
hasSuspendedRun?(runId: string): Promise<boolean>;
163163
/**
164164
* [#15389] Where each suspended run is currently parked. Read by
165-
* {@link ApprovalService.continueRestoredRun} to prove a re-armed pause is
166-
* the pause THIS request's recorded outcome was refused on, and not merely
167-
* some live pause on the same run — `hasSuspendedRun` answers a boolean and
168-
* cannot tell the two apart. Which node that is depends on the signal, not
165+
* {@link ApprovalService.continueRestoredRun} to prove the pause it was asked
166+
* to continue is the one THIS request's recorded outcome was issued at, and
167+
* not merely some live pause on the same run — `hasSuspendedRun` answers a
168+
* boolean and cannot tell the two apart. Which node that is depends on the signal, not
169169
* only on the row: see {@link ApprovalService.expectedPauseNode}.
170170
*
171171
* ⚠️ Declares a method `AutomationEngine` ALREADY implements publicly
@@ -278,6 +278,53 @@ const STRANDABLE_REQUEST_STATUSES = ['approved', 'rejected', 'returned'] as cons
278278
*/
279279
const STRANDED_CONTINUATION_KEY = '__strandedContinuation';
280280

281+
/**
282+
* Which continuations a request's CURRENT status can have issued (#15389) —
283+
* the compatibility the journal is checked against before it is replayed.
284+
*
285+
* ## Why the journal alone is not enough
286+
*
287+
* {@link STRANDED_CONTINUATION_KEY} records what the LAST failed resume was
288+
* carrying. It is written when that resume strands and it is never rewritten by
289+
* anything that moves the row on afterwards — so a signal can outlive the state
290+
* that issued it. Measured, with no injected failure beyond the strand itself:
291+
* a `resubmit` strands and journals `resubmit`; the submitter then recalls, and
292+
* a real `cancelRun` on an already-stranded run answers `false`, so the recall
293+
* marks the row `recalled` and leaves the run parked; `restoreConsumedSuspension`
294+
* re-arms the pause; and the verb replays the stale `resubmit`, opening a fresh
295+
* `pending` round on a request somebody deliberately withdrew — statuses
296+
* `["recalled","pending"]`. Every step is an ordinary operator or submitter
297+
* action answering ordinarily.
298+
*
299+
* ⛔ Clearing the journal after a successful replay does NOT close this: the
300+
* offending replay is the FIRST replay of that journal, so a clear that fires
301+
* after a replay succeeds can never run before the advance it would prevent.
302+
* Measured — with such a clear in place, both limbs still reach `resumed: true`.
303+
* The check has to happen BEFORE the journal is returned, which is where it is.
304+
*
305+
* ## Reading the table
306+
*
307+
* Each row is "what a row in this status can have issued", not "how many
308+
* writers the status has" — the two are different questions and only this one
309+
* gates a replay. `returned` carries two issuers by construction (`sendBack`
310+
* writes it and resumes down `revise`; a later `resubmit` on that same row
311+
* resumes down `resubmit` and writes NO status, so the row still reads
312+
* `returned`), so both signals are compatible with it and the journal says
313+
* which one actually stranded. `recalled` admits `recall` alone: a recall
314+
* stranded by a build that HAS the journal replays exactly, and that is the one
315+
* population a `recalled` row is replayable for.
316+
*
317+
* ⛔ A status absent from this table (`pending`, `cancelled`, anything new)
318+
* admits NOTHING, deliberately: a status whose replayable continuations nobody
319+
* has enumerated is not a status this verb may guess at.
320+
*/
321+
const CONTINUATIONS_A_STATUS_CAN_ISSUE: Readonly<Record<string, readonly string[]>> = {
322+
approved: ['approve'],
323+
rejected: ['reject'],
324+
returned: ['revise', 'resubmit'],
325+
recalled: ['recall'],
326+
};
327+
281328
/**
282329
* The second oracle's verdict: which unrecoverable shape this run is in, or
283330
* `undefined` for every run that must NOT be reported (#13909).
@@ -4410,13 +4457,34 @@ export class ApprovalService implements IApprovalService {
44104457
requestId: string,
44114458
): Promise<{ signal: StrandedContinuationSignal; source: 'journal' | 'reconstructed' }> {
44124459
const config = parseJson<Record<string, any>>(raw.node_config_json, {});
4460+
const status = String(raw.status ?? '');
44134461
const stashed = config?.[STRANDED_CONTINUATION_KEY];
44144462
if (stashed && typeof stashed === 'object' && typeof stashed.decision === 'string') {
4463+
// ⛔ The journal is a record of the last FAILED resume, not of the row's
4464+
// present state, and nothing clears it when a later door moves the row on.
4465+
// A stale signal is checked against what this status can have issued —
4466+
// see {@link CONTINUATIONS_A_STATUS_CAN_ISSUE} for the measured shape this
4467+
// closes and for why clearing the journal on replay does not close it.
4468+
const issuable = CONTINUATIONS_A_STATUS_CAN_ISSUE[status];
4469+
const journalled = String(stashed.decision);
4470+
if (!issuable?.includes(journalled)) {
4471+
throw new Error(
4472+
`INVALID_STATE: request ${requestId} is '${status || 'unknown'}' and its journalled continuation ` +
4473+
`is the ${journalled}, which a '${status || 'unknown'}' request cannot have issued — ` +
4474+
`${issuable
4475+
? `a '${status}' row is replayable only for ${issuable.map(d => `'${d}'`).join(' or ')}`
4476+
: `no continuation is replayable for '${status || 'unknown'}'`}. The journal records what the ` +
4477+
`last FAILED resume was carrying, so a later recall (or any other door that moved this row on) ` +
4478+
`leaves a signal behind that the row's own status no longer stands behind, and replaying it ` +
4479+
`would advance a step nobody is waiting on. Refusing to replay it: cancel the run with the ` +
4480+
`engine's cancelRun('${raw.flow_run_id}') if the newer outcome should stand, or resume it by ` +
4481+
`hand with the signal the flow expects.`,
4482+
);
4483+
}
44154484
return { signal: stashed as StrandedContinuationSignal, source: 'journal' };
44164485
}
44174486

44184487
const outputs: Record<string, unknown> = { ...(config?.__decisionOutputs ?? {}) };
4419-
const status = String(raw.status ?? '');
44204488

44214489
if (status === 'approved' || status === 'rejected') {
44224490
if (status === 'rejected') {
@@ -4552,9 +4620,22 @@ export class ApprovalService implements IApprovalService {
45524620
* use (`getFlow`), and no engine change.
45534621
*
45544622
* ⚠️ It does not widen what guard 3 admits beyond that one signal: for every
4555-
* other decision the answer is byte-identical to the row's own node, so the
4556-
* recall-in-revise-window shape (row `recalled`, run at the revise window)
4557-
* stays refused — its journalled signal is `recall`, not `resubmit`.
4623+
* other decision the answer is byte-identical to the row's own node.
4624+
*
4625+
* ⛔ It is NOT what keeps the recall-in-revise-window shape (row `recalled`,
4626+
* run at the revise window) refused, and an earlier revision of this comment
4627+
* claimed it was — on the reasoning that such a row's journalled signal is
4628+
* `recall` rather than `resubmit`. That is false: a recall taken inside the
4629+
* revise window calls `cancelRun` and journals NOTHING, so the journal on
4630+
* such a row is whatever an EARLIER strand left there — a `resubmit`, most
4631+
* often, since the resubmit is what the window exists to receive. Measured:
4632+
* with the journal returned before the row's status was looked at, that
4633+
* stale `resubmit` reached this method, was answered with the revise window,
4634+
* matched the parked node, and opened a fresh `pending` round on a withdrawn
4635+
* request. What refuses it is the journal/status compatibility check in
4636+
* {@link ApprovalService.resolveRecordedContinuation} — see
4637+
* {@link CONTINUATIONS_A_STATUS_CAN_ISSUE} — which runs BEFORE this method
4638+
* and never hands it a signal the row's status cannot have issued.
45584639
*/
45594640
private async expectedPauseNode(
45604641
raw: any,
@@ -4766,8 +4847,9 @@ export class ApprovalService implements IApprovalService {
47664847
throw new Error(
47674848
`INVALID_STATE: run '${runId}' is parked at ` +
47684849
`${parkedAt ? `node '${parkedAt}'` : 'no node this engine can see'}, but ${signal.what} on ` +
4769-
`request ${requestId} was issued from ${expected.describe} — this re-armed pause is not the one ` +
4770-
`that outcome was refused on, and continuing it would advance a step nobody decided`,
4850+
`request ${requestId} was issued from ${expected.describe} — so the pause this verb was asked to ` +
4851+
`continue is not the one that outcome was issued at, and continuing it would advance a step ` +
4852+
`nobody decided`,
47714853
);
47724854
}
47734855

0 commit comments

Comments
 (0)