Skip to content

test(approvals): pin the ordering invariant the dead-run sweep rests on (#3456 follow-up) - #3719

Merged
os-zhuang merged 2 commits into
mainfrom
claude/dead-approval-record-locked-enuhpy
Jul 27, 2026
Merged

test(approvals): pin the ordering invariant the dead-run sweep rests on (#3456 follow-up)#3719
os-zhuang merged 2 commits into
mainfrom
claude/dead-approval-record-locked-enuhpy

Conversation

@os-zhuang

Copy link
Copy Markdown
Contributor

Follow-up to #3703 (which closed #3456). Test-and-comment only — no behaviour change.

The unguarded assumption

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.

Reverse that order anywhere and a run that finishes promptly after the hand-back becomes indistinguishable from an orphan — so the sweep would cancel a live approval. That is precisely the one failure mode the sweep was built never to have, and #3703 leaned on the ordering to justify putting completed in the terminal set at all.

Nothing enforced it. 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.

What this adds

Rather than pin any one method's call order — which would just re-encode the convention in a second place — it pins the invariant itself: at the instant the run is handed back, no request owned by that run may still be pending.

The probe stands in as the automation surface, records every resume/cancelRun hand-back with the run's still-pending requests at that moment, and asserts the set is empty. Seven scenarios cover all seven sites:

scenario site
decide(approve) / decide(reject) resume after decideNode finalises
recall on a pending request resume down the reject edge
recall inside the revise window cancelRun (ADR-0044 — no reject edge to resume down)
sendBack resume down the revise edge
sendBack past the revision budget the ADR-0044 auto-reject branch, a separate resume site
resubmit resume back into the approval node

Why I believe the test is load-bearing

Verified with a reorder-only mutation of recallresume/cancelRun moved ahead of the finalise, end state identical:

× recall finalises before resuming
  Tests  1 failed | 235 passed (236)

Exactly one failure, the new test. That is the result worth having: it shows the test catches pure reordering (not just a changed outcome), and — since the other 235 stayed green — that no existing test covered this ordering, which is the whole justification for adding it.

An earlier, cruder mutation that changed the terminal status instead broke a dozen unrelated tests and proved nothing about ordering; it was discarded.

Also here

resolveRunDataContext's residual comment now points at #3712, filed to track the remaining #3456 gap: a schedule-triggered runAs:'user' run with no trigger user passes no ObjectQL context, so it carries no flowRunId and is still subject to the lock. Previously that residual lived only in a code comment, the changeset, and a merged PR body — nothing anyone would find.

Verification

plugin-approvals 236 passed (229 before, +7), service-automation 365 passed.


Generated by Claude Code

…on (#3456 follow-up)

`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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JGXCuBt5mSXbN3Gc8yfbRv
@vercel

vercel Bot commented Jul 27, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
objectstack Ignored Ignored Jul 27, 2026 4:04pm

Request Review

@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 2 package(s): @objectstack/plugin-approvals, packages/services.

9 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:

  • content/docs/automation/approvals.mdx (via @objectstack/plugin-approvals)
  • content/docs/automation/webhooks.mdx (via packages/services)
  • content/docs/kernel/runtime-services/audit-service.mdx (via packages/services)
  • content/docs/kernel/runtime-services/index.mdx (via packages/services)
  • content/docs/kernel/runtime-services/settings-service.mdx (via packages/services)
  • content/docs/plugins/packages.mdx (via @objectstack/plugin-approvals, packages/services)
  • content/docs/protocol/kernel/i18n-standard.mdx (via packages/services)
  • content/docs/releases/implementation-status.mdx (via @objectstack/plugin-approvals)
  • content/docs/releases/v9.mdx (via @objectstack/plugin-approvals)

Advisory only. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs origin/main → pass the list as args.docs.

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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JGXCuBt5mSXbN3Gc8yfbRv
@github-actions github-actions Bot added documentation Improvements or additions to documentation tooling labels Jul 27, 2026
@os-zhuang
os-zhuang marked this pull request as ready for review July 27, 2026 16:19
@os-zhuang
os-zhuang merged commit 6920c1c into main Jul 27, 2026
16 checks passed
@os-zhuang
os-zhuang deleted the claude/dead-approval-record-locked-enuhpy branch July 27, 2026 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/m tests tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Approval: a dead approval run can leave the record RECORD_LOCKED (#3424 follow-up, expected-behavior 3)

2 participants