fix(approvals): give a restored approval suspension an issuer, so it can be decided and not only cancelled - #15966
Conversation
…can be decided and not only cancelled `AutomationEngine.restoreConsumedSuspension` re-arms the pause of a run that stranded mid-resume and tells the operator to re-issue the continuation. For an `approval` suspension nobody could. Every approvals door that stamps the resume marker — `decide`, `recall`, `sendBack`, `resubmit` — guards on a `pending` request, and the row is terminal, written by the very call that stranded the run; the generic engine door refuses an `approval` pause outright because that node declares `resumeAuthority: 'service'`. The only verb left was `cancelRun`, which discards the branch's downstream work, so the advertised repair produced a run that looked resumable and was not decidable. Measured against the real engine and the real decision door: the restored suspension lacks nothing. A `resumeAuthority`-marked resume walks the restored pause to completion. What was missing is an ISSUER on the approvals side, which is what this adds — `ApprovalService.continueRestoredRun(requestId, options?)`. The failing door now journals the signal it was carrying, on the repairable exit only (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 already there, so no column and no authored config moves. It is awaited but can never replace the `RESUME_FAILED` throw the decision's caller is owed. Runs stranded before this shipped are served too: with no journal, 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 also carrying a `revise` action, possibly ADR-0044's revision-limit auto-rejection whose resume carried `autoRejected` — is refused rather than guessed. The verb replays a decision and rewrites nothing: all four `pending` guards are untouched, no status, mirror field or audit row is written, `resumeAuthority` stays as it is, and `ApprovalDecisionResult` is not touched. Like the engine verb it completes, it is an in-process operator repair with no REST route and no entry in the spec `ApprovalService` contract. Part of #15389 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y
…e fake engine's insert arity The raw `resumeAuthority`-marked resume against a restored suspension is the measurement that places this card's fix on the approvals side rather than in `engine.ts`, so it is pinned in its own right and not only through the new verb. Driven on a second stranded-and-restored run so the cancel measurement above it is undisturbed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y
…proval-restore-continuation
… write site and shifted lines Each repair is the one the failing gate itself prescribes, and each grows or re-anchors a ledger rather than loosening one. A base control at the merge-base (924f0fe) confirms all three are caused by this branch and were green before it. - `engine-double-contract.pinned.json` learns the two doubles (delete, update) in the new pin file. +10 lines, nothing removed, no baseline lowered: 791 pinned / 133 debt / 3 exempt, up from 789 at base. - `content/docs/permissions/system-context.mdx` re-anchored by the census's own `--fix`: inserting methods into `approval-service.ts` shifted eight cited line numbers. Pure line rot, no elevation behaviour added or removed — this branch introduces no `context.isSystem` read. - The tenant-audit census re-measured: `journalStrandedContinuation` adds one write call site, so 221 -> 222, 147 -> 148 decidable, 103 -> 104 decidably elevated. `--write` regenerates the tables; the eight prose figures the gate names as hand-written are corrected to match, each verified against the regenerated table rather than arithmetic. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y
…proval-restore-continuation
…tree Merging origin/main auto-merged content/docs/permissions/system-context.mdx textually clean and semantically stale: the census is green at origin/main (904e707, measured) and green on this branch before the merge, but red on the merge product with 26 problems, every one an anchor into plugin-security or rest — files this branch never touches and the merge brought in. Repaired with the census's own --fix, which re-anchors against the merged code. 15 anchors rewritten; the result reports 105 elevation read sites across 44 files and 140 resolving anchors, identical to what origin/main reports, so this restores the upstream reading rather than inventing one. No elevation behaviour is added or removed anywhere on this branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y
📓 Docs Drift CheckThis PR changes 1 package(s): 6 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:
⛔ 2 release-owned page(s) also name something this change touched. These are read-only:
What this run could not see
Coarse fallback — 5 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin d26b1e213fb94335257d506c94d2dbce03bde6d7 && git checkout d26b1e213fb94335257d506c94d2dbce03bde6d7
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin f50c394da55846da8d38f1e1efadbc24faa26ce2 6e63090130228860d34f82edc20f31935d2bb84c && git checkout -B drift-repro f50c394da55846da8d38f1e1efadbc24faa26ce2 && git merge --no-ff 6e63090130228860d34f82edc20f31935d2bb84c
node scripts/docs-audit/affected-docs.mjs --json f50c394da55846da8d38f1e1efadbc24faa26ce2
|
github-actions
Bot
added
documentation
Clause-② contract review —
|
PM — held. Rework dispatched, the
|
…proval-restore-continuation # Conflicts: # scripts/engine-double-contract.pinned.json
…ause, 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
Clause-② contract review — round 2 —
|
Claim — round 3 reworkTaking round 3 of the rework on this PR, continuing the existing claim on #15389.
Scope, exactly as the round-3 verdict (comment 5554014979) ranks it:
⛔ Not re-opening anything round 2 settled (the four B1 refusals at the real door, the firing reverse control, Generated by Claude Code |
…d replace a phantom pin Two things this PR claimed were not what it said they were. Both were in the safe direction — nothing escalated — but a false claim is not shippable. 1. Guard 3 compared the run's parked node against the request row's own `flow_node_id`. That is right for `approve`, `reject`, `revise` and `recall`, all of which are issued at the request's own approval node. It is WRONG for `resubmit`: a resubmit is only reachable after a send-back moved the run to the revise window, and it resumes THAT pause down the `resubmit` back-edge. So a stranded resubmit re-arms at the revise window while the row still records the approval node, and guard 3 refused it — on both the journal and the rebuild paths — with a message telling the operator the pause was not this request's when it was exactly this request's. The expected node is now signal-aware (`ApprovalService.expectedPauseNode`), derived from the flow definition the same way `assertReviseEdge` already derives the revise window, fail-closed in every direction, and needing no automation surface that method did not already use. The refusal message now names the node the signal was issued from rather than "this request's own node".⚠️ It widens what guard 3 admits for exactly one signal. Every other decision resolves byte-identically to the row's own node, so the recall-in-revise-window shape stays refused: its journalled signal is `recall`, not `resubmit`. 2. PIN 7(b) could not reach the code it named. It set the reject-branch throw for a flow with no `mark_rejected` node, never decided anything, restored a suspension that had never been consumed, and asserted a refusal that was only `request is pending` — then discarded the first call with `void`, which is what hid it. It passed on every input, and P5 therefore had no pin behind it. Replaced with the real construction: strand the first approval's `approve` continuation by failing the next round's request insert once, restore, ASSERT the first call resumes and parks the run at the second approval node, then retry and assert guard 1 refuses by exact message with the second node still pending and no branch run. The test double gains a one-shot insert failure lever, which is what makes a real strand reachable from a test; every leg that uses it asserts the lever was consumed, so a fixture that silently stopped firing reads as a failure rather than as a pass. 3. PIN 8 gained two legs through `continueRestoredRun` end to end — the resubmit replay on the journal path and on the rebuild path — because its discriminator was green over code the verb never reached. PIN 7(a)'s population comment is corrected: the row is `returned`, not `recalled` (a `recalled` row is refused before any guard runs). 4. The changeset's "every one of these is journal-recoverable" is replaced by a measured, named set, with the two by-design refusals named and the two shapes no pin covers stated as not claimed. Part of #15389 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XpTx2tbq3pZRYAdoGt6E6Y
2 participants
Part of #15389 — deliberately not
Fixes.Revision 3 answers the round-2 clause-② review's two blocking findings (verdict). Both were false claims in the safe direction — nothing escalated, nothing was unsound at the door — but a pin cited for a population it cannot reach, and an operator message naming a cause the code did not take, are not shippable. Head
6e6309013.Round 3 — finding 1: PIN 7(b) was a phantom, and P5 had no pin behind it
The old PIN 7(b) set the reject-branch throw for
two_step, a flow with nomark_rejectednode; nothing ever threw. It then restored a suspension that had never been consumed (restoreConsumedSuspensionansweredRUN_SUSPENDED … nothing to restore), called the verb on a pending request, and asserted only that a retry matched/INVALID_STATE/— whichrequest is pendingsatisfies.void firstCalldiscarded the evidence. The pin passed on every input, so P5 — the construction the PM called decisive — had nothing behind it.Replaced with the real construction, driven end to end:
two_stepparks ata1; the row is pending ata1.decide(approve)walks the approve edge intoa2, whose executor opens round 2 by inserting a request — and that insert is failed once, so the resume strands:RESUME_FAILED,repairable: true, suspension consumed, round 2 never opened.restoreConsumedSuspensionre-arms the pause ata1.resumed: true,source: 'journal',decision: 'approve', round 2 now pending ata2, run parked ata2.a2stillpending,marksempty, and the run still suspended.The test double gains a one-shot insert-failure lever, which is what makes a real strand reachable from a test. Every leg that uses it asserts the lever was consumed, so a fixture that silently stopped firing reads as a failure rather than as a pass.
Audit of the
void firstCallshape — a named count, with a firing controlScripted over the pin file, three patterns: (A) statement-level
voiddiscards; (B) bindings whose initialiser contains.then(and that are never read again; (C) deliberate.then(() => null, () => null)swallows.Firing control: the same script run against
git show HEAD~:…— the blob known to contain the defect — returns[A] 1: L449 void firstCall;. The search fires.voiddiscardsvoid firstCall).then(...)bindings never read after bindingfirstCallwas read, exactly once, by thevoidthat discarded it. [A] is the detector; [B] is the wider net and it is empty in both versions. The one added [C] swallow is PIN 8(e)'s stranded resubmit, whose next line asserts the injected failure fired, so the swallow cannot hide a no-op.Round 3 — finding 2: guard 3 asked the wrong question for one signal, and said so wrongly
Measured and reproduced: a
returnedrow whose resubmit stranded was refused by guard 3 on both the journal and the rebuild paths, and the refusal was factually wrong about why.The cause is structural. Guard 3 compared the run's parked node against the row's own
flow_node_id. That is correct for three of the four signals and wrong for the fourth:approve/rejectrevise(send-back)reviseedgerecallrejectresubmitreviseedge leads toresubmitback-edgeSo a stranded resubmit re-arms at the revise window while the row still records the approval node, and the operator was told "this pause is not the one this request's outcome was refused on" when it was exactly that pause.
Settled by making the path genuinely reachable, not by keeping the refusal.
ApprovalService.expectedPauseNodederives the expected node from the signal. Forresubmitit resolves the revise window from the flow definition the same wayassertReviseEdgealready does — areviseout-edge of this request's node into a node the flow declares asapproval_revise, the pause only this service can continue. It is fail-closed in every direction: no engine, no flow, no such edge, or more than one candidate all refuse.getFlow, whichassertReviseEdgealready requires.service-automation/src/engine.tsremains at 0 files changed in this PR (git diff --stat 1375344b6..6e6309013 -- packages/services/service-automation/is empty) — the lifted fence was not needed.expectedPauseNodereturns the row's own node byte-identically, so the recall-in-revise-window shape stays refused: its journalled signal isrecall, notresubmit. Leg M8 below proves the resubmit branch is load-bearing rather than decorative.run 'R' is parked at node 'wait_revision', but the send-back on request X was issued from its own approval node 'review' — this re-armed pause is not the one that outcome was refused on….The discriminator is re-aimed. PIN 8 gained two legs through
continueRestoredRunend to end — a stranded resubmit replayed on the journal path (population d) and on the rebuild path with the journal stripped (population e, the pre-ship population the card names). Populations (a) and (b) remain atresolveRecordedContinuationand now say so in their own comments: (a) proves only that the rebuild picksresubmitoverrevise, and it was green while guard 3 refused every real caller.The claim is corrected, identically here and in the changeset:
PIN 7(a)'s population comment is corrected too: its row is
returned, notrecalled— arecalledrow is refused byresolveRecordedContinuationbefore any guard runs, so it can never reach guard 3 on the rebuild path.What rounds 1–2 established, unchanged and not re-opened
hasSuspendedRun(runId)answers a boolean; 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. Three guards stand, each with its own reverse-control pin and its own mutation leg:assertLatestForRun(raw)— refuses a superseded row. Already in this file; recall-in-revise-window andresubmituse it for the same reason.hasSuspendedRun— a pause exists at all. Strict: an unreadable store throws rather than reading as "not suspended".B2's premise — "each status is written by exactly one door" — is false, and the rebuild does not rest on it:
approvedapproverejectedreject, or REFUSEDreviseaction row means ADR-0044's auto-reject arm is possiblereturnedresubmitorreviseresubmitaction row, whose sole writer in this file isresubmitrecalledreturned-via-resubmit was never one of those rows — it was reachable code behind a guard asking the wrong question.Every pin, and its population
decide/recall/ generic-resume all refuse,cancelRunis the only verb left — plus the rawRESUME_AUTHORITY_SERVICEresume completing, on a second run, which is the mechanismreject, journal path: continuation completes, branch runs, run completedreject: row deep-unchanged, one reject action row, and all fourpending-guarded doors (decide,recall,sendBack,resubmit) asserted by namereject, rebuild path with the journal removed; and the ADR-0044 auto-reject ambiguity refusedcreated_atreturnedrow whose send-back landed and whose run is now at the revise window — refused, message asserted verbatim; (b) guard 1 reached by a real retry ofapproveintwo_step, with the first call asserted (see finding 1)returneddiscriminator atresolveRecordedContinuationand its reverse control — resolver-level only, and the comments now say so; (c)recalledrefused; (d)(e) a stranded resubmit replayed end to end throughcontinueRestoredRun, on the journal path and on the rebuild path⛔ A passing pin is proof of what it covers and never of a claim attached to it — which is exactly how PIN 7(b) got through round 2.
Harness defect found while pinning this (round 2, unchanged)
The test double ignored
orderBy, so it returned the oldest row whereassertLatestForRunasks for the newest — the guard passed on every input. The double now honours it, and guard 1 is additionally driven in isolation on a row whosecreated_atis controlled.Verification
Every exit code captured right after a single redirected command, never off a pipe; heavy runs serialized through
os-verify-lock.shand read from itsVERDICTline.Pins.
pnpm --filter @objectstack/plugin-approvals exec vitest run --maxWorkers=2 src/restored-approval-continuation.test.ts—Test Files 1 passed (1) · Tests 8 passed (8), exit 0.Suite.
pnpm --filter @objectstack/plugin-approvals test—Test Files 40 passed (40) · Tests 683 passed (683), exit 0.typecheckexit 0;check:test-typecheck: OK — 8 file(s) / 324 error(s) / 27 pinned signature(s) held, unchanged.Mutation legs. Subject
packages/plugins/plugin-approvals/src/approval-service.ts, mutated after the implementation was committed. Each anchor asserted to occur exactly once as an exact substring in the form written; the mutation proved on disk by agit hash-objectdelta plus a marker count (removed text 0 occurrences, injected text 1); restore undertrap … EXIT INT TERMwith absolute paths, proved by blob equality againstHEAD:THE-SUBJECT-PATHand an emptygit diff HEAD. The pin file imports./approval-service.js— a relative specifier inside the same package — so the subject resolves fromsrc, notdist, and no rebuild sits between the mutation and the reading; each leg going red is itself the proof the mutation reached the code under test.assertLatestForRun) call removed fromcontinueRestoredRunexpectedPauseNode's signal-awareness removed (always the row's own node)returnedresubmit discriminator removed⭐ M4 now reds PIN 7 as well as PIN 6 — that is the round-3 fix measured: the old PIN 7(b) could not go red under any guard mutation, because it never reached a guard. Its failure text under M4 is the exact assertion:
expected 'INVALID_STATE: run … is parked at node 'a2' …' to be 'INVALID_STATE: a newer approval request supersedes this one'.⭐ M8 reproduces round 2's finding 2 verbatim. With signal-awareness removed, PIN 8(d) fails with
INVALID_STATE: run '…' is parked at node 'wait_revision', but the resubmit on request X was issued from its own approval node 'review' — this re-armed pause is not the one that outcome was refused on— the exact wrong refusal the review reported, produced on demand.M1/M2/M3/M7 from round 2 are unchanged by this revision and were not re-run; that is stated rather than implied.
Gates. Family re-derived from the actual changed files:
node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack, exit 0, answering forobjectstack-ai/objectstackat6e6309013. 81 gates, each run as a bare command with its exit captured immediately: 80 exit 0, 1 non-zero, listed below.check:nul-bytesexit 0, plus a direct control-byte scan (grep -naP '[\x00-\x08\x0b\x0c\x0e-\x1f\x7f]') over the PR's whole file set — no match.The reviewer's NOT MEASURED list, now measured:
check:type-check-debt— round 2's exit 3 was a refusal, not a pass. Re-run afterpnpm exec turbo run build --concurrency=2 --filter='./packages/*' --filter='./packages/*/*'(71/71 successful,@objectstack/plugin-approvalsa cache miss so itsdistcarries this revision). Real verdict, exit 0:check-type-check-coverage: OK — 75/79 workspace packages type-checked (plus the root), 4 in the DEBT ledger (53 frozen raw errors), 1 exemptand--re-measure: OK — 12 ledger entr(ies) re-measured in 88.7s, 140 raw tsc error(s) total, none above its recorded number.check:single-claim-paths— exit 0:✓ check-single-claim-paths self-test: 54 cases pass..mjswired withPR_NUMBER/GITHUB_REPOSITORY, which is a different invocation and is still NOT MEASURED here.check:react-declaration-parity—packages/spec/package.json, not the root — anddispatch-gates.mjsderives it aspnpm --filter @objectstack/spec run check:react-declaration-parity. Run: exit 1, refusing for a missing browser-dumped manifest ("build deliberately does not produce one (it must not pull in a browser)"). Not attributable to this PR:git diff --name-only 1375344b6..6e6309013 -- packages/specis empty, and both prior rounds recorded the same refusal at base and head.Scoped claims.
service-automationwas not run — deliberately; this PR changes no file in it. Numbers above are for@objectstack/plugin-approvalsand the derived gate family only.Lint & Repo Gatesis expected red independently of this branch: that job fails onmainitself (#15992). No fix for it is carried here —scripts/check-regen-pending.mjsis a single-writer path.