Skip to content

Commit bc0872b

Browse files
committed
fix(approvals): tie the restored continuation to this request's own pause, and refuse the rebuilds that cannot be proved
Two defects found by contract review of the first revision, both measured at the real door, and both making the repair verb advance a flow with no decision behind it - strictly worse than the dead end it exists to open. GUARDS. `hasSuspendedRun(runId)` answers a boolean and cannot tell a re-armed pause from any other live pause on the same run. Four shapes were measured advancing something they should not: a terminal request driving a LATER approval node's pending pause with no decision recorded on it; a plain RETRY of the verb doing the same after the run re-suspended; a next-round `returned` row orphaning round two; and a `recalled` request resurrected into a new pending round from a pause it never gated. The retry is the one that settles the severity: repeating a repair verb is the ordinary shape of using it, not a hostile construction. Three guards now stand, each with its own reverse-control pin: `assertLatestForRun` (already in this file, used by recall and resubmit for the same reason) refuses a superseded row; the strict `hasSuspendedRun` refuses a run with no pause and throws rather than read a store outage as "not suspended"; and a node-identity check refuses a pause parked anywhere but this request's own node. The last is fail-closed in every direction - no reader, no listing, no entry, or an entry at another node all refuse - because a false refusal costs a retry while a false admission advances a step nobody decided. It needs NO new automation-engine surface. `listSuspendedRunsDurable` is already public on AutomationEngine and already names the parked node; the approvals-side resume interface simply declares the method it already has. REBUILD. "Each status is written by exactly one door" was false, and it was the load-bearing premise of the no-journal path that serves runs stranded before this ships. `returned` has one writer but two issuers, so a stranded resubmit was rebuilt as a send-back and walked the `revise` edge with `{decision:'revise'}` where `{resubmitted:true}` was owed - proceeding, rather than failing loudly, through the engine's unmatched-label fallback. It is now discriminated by the `resubmit` action row, whose sole writer in this file is `resubmit`. `recalled` has two writers across three behaviours, two issuing no continuation at all, and nothing on the row tells them apart - so it is REFUSED on the rebuild path, with a message naming what an operator can do instead. Both stay journal-recoverable. HARNESS. The test double ignored `orderBy`, so it returned the OLDEST row where `assertLatestForRun` asks for the newest - the guard passed on every input and a pin naming it would have measured nothing. The double now honours it, and guard 1 is additionally driven in isolation on a row whose `created_at` is controlled, so the ordering is what is under test rather than the clock's resolution. PIN 3 previously asserted `decide` alone while claiming all four `pending` guards; it now asserts all four by name. The system-context census is regenerated from the merged tree with `pnpm gen:system-context-census` (9 anchors), discharging the deferral the merge commit took; it reports the same 105 sites / 44 files / 140 anchors as before. Part of #15389 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y
1 parent bc279a1 commit bc0872b

4 files changed

Lines changed: 364 additions & 46 deletions

File tree

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ Measured against the real engine and the real decision door: the restored suspen
1010

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.
13-
- **Runs stranded before this shipped are served too.** With no journal to read, the signal is rebuilt from the recorded outcome, which is exact for each accepted status because each is written by exactly one door: `approved``approve`, `rejected``reject`, `returned``revise`, `recalled``reject`. The one shape it cannot rebuild — a `rejected` row that also carries a `revise` action, which may be ADR-0044's revision-limit auto-rejection, whose resume carried `autoRejected: true` — is **refused rather than guessed**, and the journal is what makes that shape recoverable going forward.
13+
- **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 at **this request's own node**. The node check is fail-closed in every direction, including an engine that cannot report where a run is parked. 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. Every one of these is journal-recoverable going forward — the refusals are the pre-journal population only.
1415

1516
⛔ 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.

content/docs/permissions/system-context.mdx

Lines changed: 2 additions & 2 deletions
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:1027`, `:1136`, `:3392`, `:3540`, `:3708`, `:3779`, `:3968`, `:4008` |
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` |
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` |
@@ -193,7 +193,7 @@ assuming `isSystem` covers it is a documented source of bugs.
193193

194194
| Assumption | Reality | Anchor |
195195
|:---|:---|:---|
196-
| "It suppresses triggers / record-change automation" | **No.** Only `skipTriggers` does. A bare `{ isSystem: true }` on a seed write re-fired automation on freshly seeded rows and wedged first boot | `metadata-protocol/src/seed-loader.ts:2032` (rationale at `:1942``1944`, #3760), `flow.zod.ts:702` |
196+
| "It suppresses triggers / record-change automation" | **No.** Only `skipTriggers` does. A bare `{ isSystem: true }` on a seed write re-fired automation on freshly seeded rows and wedged first boot | `metadata-protocol/src/seed-loader.ts:2032` (rationale at `:1942``1944`, #3760), `flow.zod.ts:743` |
197197
| "It skips the state machine" | **No.** That is `skipStateMachine`, carried by seed replay and by `treatAsHistorical` imports | `objectql/src/engine.ts` FSM gate; see [State Machine](/docs/protocol/objectql/state-machine) |
198198
| "It skips validation rules" | **No.** Field shape, `format`, `script` and the rest still run. The `readonly` strip runs *before* validation precisely so a discarded value is not judged | `objectql/src/engine.ts:10306``10323` |
199199
| "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:1581` (#3493 / #6640) |

0 commit comments

Comments
 (0)