From bec3f39ef00892af839a974ea0233d03cd068086 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 27 Jul 2026 16:00:01 +0000 Subject: [PATCH 1/2] test(approvals): pin the ordering invariant the dead-run sweep rests on (#3456 follow-up) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `releaseDeadRunRequests` recalls a pending request whose owning run is terminal, on the premise that such a pair can only be an orphan. That premise is not self-evident — it holds only because every in-band transition moves the request out of `pending` BEFORE handing the run back. Reverse that order and a run which finishes promptly afterwards becomes indistinguishable from an orphan, so the sweep would cancel a LIVE approval: precisely the failure mode it is built never to have. Nothing enforced the ordering. It was a convention spread across four public methods and seven resume/cancelRun call sites, any of which a refactor could reorder without a single test going red. So pin the invariant itself rather than any one method's call order: at the instant the run is handed back, no request owned by that run may still be `pending`. The probe records every resume/cancelRun hand-back and asserts it, across all seven sites — decide approve/reject, recall, send-back, send-back past the revision budget (the ADR-0044 auto-reject branch), recall inside the revise window (cancelRun), and resubmit. Verified load-bearing by a REORDER-ONLY mutation of `recall` — resume moved ahead of the finalise, end state identical. Exactly one test fails, the new one; the other 235 stay green. That is also the proof no existing test covered this ordering. Also points the `resolveRunDataContext` residual comment at #3712, which now tracks the schedule-triggered run that still cannot carry `flowRunId`. plugin-approvals 236 passed, service-automation 365 passed. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JGXCuBt5mSXbN3Gc8yfbRv --- .../src/approval-service.test.ts | 121 ++++++++++++++++++ .../src/runtime-identity.ts | 15 ++- 2 files changed, 129 insertions(+), 7 deletions(-) diff --git a/packages/plugins/plugin-approvals/src/approval-service.test.ts b/packages/plugins/plugin-approvals/src/approval-service.test.ts index c8e638e22b..e9a7821e04 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.test.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.test.ts @@ -2390,3 +2390,124 @@ describe('ApprovalService — participant visibility (#3590)', () => { expect(res.request.status).toBe('approved'); }); }); + +// ── The ordering invariant the dead-run sweep rests on (#3456) ───────── +// +// `releaseDeadRunRequests` recalls a PENDING request whose owning run has +// reached a TERMINAL state, on the premise that such a pair can only be an +// orphan. That premise is not self-evident — it holds only because every +// in-band transition moves the request OUT of `pending` before it hands the run +// back. Resume first and a run that finishes promptly afterwards would be +// indistinguishable from an orphan, so the sweep would cancel a LIVE approval — +// precisely the one failure mode it is built never to have. +// +// Nothing enforces that ordering: it is a convention spread across four public +// methods and seven resume/cancelRun call sites, any of which a refactor could +// reorder without a single existing test going red. So pin the invariant +// itself rather than the call order of any one method — at the instant the run +// is handed back, no request owned by that run may still be `pending`. +describe('in-band transitions finalise before they resume (#3456 invariant)', () => { + let engine: ReturnType; + let svc: ApprovalService; + let n = 0; + const baseTime = new Date('2026-01-15T10:00:00Z').getTime(); + /** One entry per hand-back, with any still-pending requests owned by the run. */ + let handoffs: Array<{ hook: string; stillPending: string[] }>; + + /** A flow whose approval node declares the `revise` out-edge send-back needs. */ + const REVISE_FLOW = { + name: 'deal_approval', + edges: [{ id: 'e_rev', source: 'approve_step', target: 'wait_revision', label: 'revise' }], + }; + + function recordHandoff(hook: string) { + const rows = (engine._tables['sys_approval_request'] ?? []) as any[]; + handoffs.push({ + hook, + stillPending: rows + .filter(r => String(r.flow_run_id ?? '') === 'run_1' && r.status === 'pending') + .map(r => String(r.id)), + }); + } + + /** Assert every hand-back this scenario made was clean. */ + function expectCleanHandoffs() { + expect( + handoffs.length, + 'the run was never handed back — this scenario did not exercise the invariant', + ).toBeGreaterThan(0); + for (const h of handoffs) { + expect( + h.stillPending, + `${h.hook}() handed run_1 back while it still owned a pending request — ` + + 'the dead-run sweep would treat that as an orphan and cancel a live approval', + ).toEqual([]); + } + } + + beforeEach(async () => { + engine = makeFakeEngine(); + n = 0; + handoffs = []; + svc = new ApprovalService({ engine: engine as any, clock: { now: () => new Date(baseTime + (n++) * 1000) } }); + svc.attachAutomation({ + async resume() { recordHandoff('resume'); }, + async cancelRun() { recordHandoff('cancelRun'); }, + async getFlow() { return REVISE_FLOW; }, + } as any); + engine._tables['opportunity'] = [{ id: 'opp1', amount: 100 }]; + }); + + const open = (configExtra: Record = {}) => + svc.openNodeRequest(openInput(['u9'], {}, configExtra), CTX); + + it('decide(approve) finalises before resuming', async () => { + const req = await open(); + await svc.decide(req.id, { decision: 'approve', actorId: 'u9' }, SYS); + expectCleanHandoffs(); + }); + + it('decide(reject) finalises before resuming', async () => { + const req = await open(); + await svc.decide(req.id, { decision: 'reject', actorId: 'u9' }, SYS); + expectCleanHandoffs(); + }); + + it('recall finalises before resuming', async () => { + const req = await open(); + await svc.recall(req.id, { actorId: 'u1' }, CTX); + expectCleanHandoffs(); + }); + + it('sendBack finalises before resuming', async () => { + const req = await open(); + await svc.sendBack(req.id, { actorId: 'u9', comment: 'fix the totals' }, CTX); + expectCleanHandoffs(); + }); + + it('sendBack past the revision budget auto-rejects before resuming', async () => { + // `maxRevisions: 0` takes the ADR-0044 loop-guard branch on the first + // send-back — a separate resume site from the normal path above. + const req = await open({ maxRevisions: 0 }); + const out = await svc.sendBack(req.id, { actorId: 'u9' }, CTX); + expect(out.autoRejected, 'expected the auto-reject branch').toBe(true); + expectCleanHandoffs(); + }); + + it('recall inside the revise window cancels the run without a pending request', async () => { + const req = await open(); + await svc.sendBack(req.id, { actorId: 'u9' }, CTX); + handoffs = []; // isolate the recall's own hand-back + await svc.recall(req.id, { actorId: 'u1' }, CTX); + expect(handoffs.map(h => h.hook)).toContain('cancelRun'); + expectCleanHandoffs(); + }); + + it('resubmit re-enters the node without leaving the old request pending', async () => { + const req = await open(); + await svc.sendBack(req.id, { actorId: 'u9' }, CTX); + handoffs = []; // isolate the resubmit's own hand-back + await svc.resubmit(req.id, { actorId: 'u1' }, CTX); + expectCleanHandoffs(); + }); +}); diff --git a/packages/services/service-automation/src/runtime-identity.ts b/packages/services/service-automation/src/runtime-identity.ts index 33bde71881..3bf8adafb6 100644 --- a/packages/services/service-automation/src/runtime-identity.ts +++ b/packages/services/service-automation/src/runtime-identity.ts @@ -55,13 +55,14 @@ export function resolveRunDataContext(context: AutomationContext | undefined): R if (context?.runAs === 'system') { return { isSystem: true, positions: [], permissions: [], ...(flowRunId ? { flowRunId } : {}) }; } - // NOTE (#3456): the identity-less case below returns `undefined`, so a - // schedule-triggered `runAs:'user'` run with no user carries NO context at all - // — and therefore no `flowRunId` either, leaving it subject to the approvals - // record lock on its own target record. Manufacturing a context here just to - // carry the run id would flip that run from the documented unscoped fail-open - // (#1888) to baseline-member RLS — a separate, larger behavior change. The - // dead-run sweep in plugin-approvals is what recovers this shape. + // NOTE (#3456 residual, tracked as #3712): the identity-less case below returns + // `undefined`, so a schedule-triggered `runAs:'user'` run with no user carries + // NO context at all — and therefore no `flowRunId` either, leaving it subject + // to the approvals record lock on its own target record. Manufacturing a + // context here just to carry the run id would flip that run from the + // documented unscoped fail-open (#1888) to baseline-member RLS — a separate, + // larger behavior change. The dead-run sweep in plugin-approvals is what + // recovers this shape. if (!context?.userId) return undefined; // `context` is now narrowed to a defined AutomationContext with a userId. const out: RunDataContext = { From d48d965f7a97e78dc55d7d3d7ebbc63faac3162f Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 27 Jul 2026 16:03:56 +0000 Subject: [PATCH 2/2] =?UTF-8?q?chore:=20add=20an=20empty=20changeset=20?= =?UTF-8?q?=E2=80=94=20this=20PR=20releases=20nothing?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Check Changeset gate requires every PR to state its release intent explicitly, and offers an empty changeset as the way to say "nothing ships". This PR is tests plus one comment reference, so no package is bumped. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JGXCuBt5mSXbN3Gc8yfbRv --- .changeset/approval-dead-run-ordering-invariant.md | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100644 .changeset/approval-dead-run-ordering-invariant.md diff --git a/.changeset/approval-dead-run-ordering-invariant.md b/.changeset/approval-dead-run-ordering-invariant.md new file mode 100644 index 0000000000..74b600f024 --- /dev/null +++ b/.changeset/approval-dead-run-ordering-invariant.md @@ -0,0 +1,11 @@ +--- +--- + +test(approvals): pin the ordering invariant the dead-run sweep rests on (#3456 +follow-up) — releases nothing. + +Tests plus one comment reference; no runtime behaviour changes, so no package +needs a version bump. The new coverage asserts that every in-band approval +transition moves its request out of `pending` before it hands the run back — +the unenforced premise `releaseDeadRunRequests` relies on to tell an orphaned +request from a live one.