Repository navigation
Expire approvals at a deadline, never approving, and seal requests into the audit chain - #144
Merged
Merged
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 54 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
sunilp
force-pushed
the
approval-integrity
branch
from
October 11, 2026 01:59
98c942e to
7cc5b9f
Compare
sunilp
force-pushed
the
approval-deadlines
branch
from
October 11, 2026 02:04
a43c224 to
bc59ab3
Compare
start_run wrote the execution row, WorkflowStarted, the start node's NodeScheduled and its work item as four separate writes. A scheduler tick landing after the row and before NodeScheduled saw a live run whose start node, a root of the graph, had not been scheduled, and scheduled it again, so the start node ran twice under load. The four rows are now one write, StateBackend::start_execution_atomic, on all three backends. Both SQLite backends share one transaction body with continuation segments (create_run_in). The in-memory backend publishes the events before the execution row the scheduler reads runs from, and the work item after it. A run whose start node does not exist is refused before anything is written. The test runs a real scheduler tick after every write start_run makes, on memory, SQLite and the tenant-scoped view. Scheduler::tick is public so a test can do that.
An approval request waited forever: nothing ever closed it. A tenant, workflow or node policy may now set approval_timeout_secs, and escalate_after_secs below it. The most specific level with a timeout wins, together with its own escalation, so the pair stays consistent. Invalid values are refused at registration and at PUT /tenants, and a stored invalid tenant policy refuses nodes like any unparseable one. A request raised under a deadline records expires_at and escalate_at (UTC) in its context, on both the node guard and the agent dispatch guard, and the pending listing shows them. The scheduler tick sweeps executions holding a node for approval: past expires_at it records ApprovalExpired, a rejection by system:deadline, so the node and run fail as on a rejection; past escalate_at it records ApprovalEscalated once, the hook for notifiers. An unreadable deadline counts as passed. A decision after the deadline is 409 and records nothing, whether or not the sweep has run. A decision and an expiry of one request cannot both land: both append through the new StateBackend::append_event_if, which checks that the request is still pending in the same step as the append, on all three backends. This also replaces the read-then-append the approve route used for decisions. TimeoutConfig::approval_timeout was declared and never read, so a run that set it never expired. It is removed, and a non-null timeouts.approval_timeout is refused at registration. The new policy fields are not serialized when unset, so existing node definition hashes, which approvals bind, do not change.
Only the decision was sealed. The ToolApprovalRequired a worker or the claim route raised went to the event log alone, so the record of what was asked, its action digest and its deadline, could be rewritten without the chain noticing. Requests are now recorded through one helper, node_guard::record_request, which both guards (node and agent dispatch) and both transports use: it appends the request and seals it, with its assigned sequence, through the new ApprovalSealer trait. The deadline sweep seals the expiries and escalations it appends the same way. AuditEnricher implements the trait; the claim route passes the server's enricher, and the embedded worker pool and the scheduler take it with with_sealer. An expiry is sealed with actor system:deadline. The trait lives in jamjet-state so the worker and scheduler need no dependency on the audit crate. Sealing follows the durable append and is best effort, as on the decision path. The scheduler does not know a run's tenant, so it seals under the default tenant.
The approve route checked the request's deadline once, before the conditional append, and the append's condition was only "still pending". A decision checked before expires_at whose append was delayed past it (a SQLite lock wait, a pool acquire, a pause) landed late, and the sweep then recorded nothing. The reviewer's probe showed it on both memory and SQLite: 200, one ApprovalReceived, response after the deadline. The condition is now approvals::still_open: still pending on that request AND its deadline not passed, evaluated against the clock inside the append, under the write lock. A refusal there is 409 Expired. The probe is ported as a test with a 700 ms stall before the append.
The most specific level with a timeout won, so a workflow or node policy could set approval_timeout_secs to a year and override a tenant admin's one-hour deadline, dropping the tenant's escalation with it. Every other tenant control in the chain is one a workflow author cannot loosen. The effective timeout is now the shortest any level sets, and the effective escalation the shortest any level sets. If that escalation would not come before the timeout it is dropped and logged; the timeout is never extended to fit it. A workflow or node can only shorten. ApprovalDeadline::resolve returns a DeadlineResolution that reports a dropped escalation.
The scheduler sealed approval_expired and approval_escalated under the default tenant. GET /audit filters by the caller's tenant, so a tenant never saw its own expiries, and a default-tenant caller could list another tenant's execution ids, node ids and deadlines. The run's tenant now comes from one source, its execution row, through the new StateBackend::execution_tenant (SQLite and tenant-scoped read the column, scoped; the single-tenant memory backend answers default). The scheduler seals under that tenant, and if it cannot read it, it seals nothing and logs the gap rather than file the record under another tenant.
The refusal ran before the immutability check, so a deployment that re-registers an existing version on boot got a 400 for a version stored before this release with timeouts.approval_timeout set, while that version kept running. The identical re-register is now answered first (200), and only a new registration carrying the key is refused.
The YAML compiler refused approval_timeout_secs and escalate_after_secs as unknown policy keys, and the Agent compiler built policies from the three older keys only, so a deadline could be set only in hand-written IR or with PUT /tenants. Both compilers now carry the two keys, checked as the runtime checks them (whole seconds, 1 s to a year, escalation shorter and next to a timeout), and emit them only when set so an unset deadline adds no keys to a node's definition. hitl.md shows the YAML and Agent forms (both snippets were run against the package). The Java builder has no policy support, so nothing changes there.
With no node_id and the run's only request already expired, the approve
route answered NoPending ("no pending approval"), which hides why. It
now answers 409 Expired when the latest request was resolved by its
deadline. The deadline check deliberately precedes the digest rules, so
a late approval with no digest is 409 Expired rather than 400
DigestRequired; that order is now stated in the code and in hitl.md.
The scheduler fold failed a held node on any ApprovalExpired for it, while both approvals folds and the projector act only on the request the expiry names. The sweep appends an expiry only while its request is pending, so this was unreachable through the engine, but it was the one fold that would have failed a node on an expiry of a superseded request. The fold now records each node's latest request sequence and requires the expiry to name it.
The memory backend took an event's sequence before locking its log, so a plain append could reserve N, a conditional append land N+1 ahead of it, and N be pushed after: the log out of sequence order, and N unseen by the condition that admitted N+1. append_event, finish_execution and commit_turn now share push_event, which takes the sequence while it holds the lock. A concurrent test (8 threads, 16,000 appends) found about 2,100 out-of-order pairs on three of three runs before the fix and none after.
The tick read a run's whole log for the deadline sweep every 500 ms for every run holding a node, with or without a deadline anywhere, so a request held for days on a long log cost a full read twice a second. The scheduler's per-run fold now records which nodes' latest request carries expires_at or escalate_at, and the sweep runs only when a held node is one of them. Any recorded key counts, even an unreadable one, so the fail-closed reading of an unreadable deadline still applies.
Sealing is best effort, which is right at every site: the request, expiry or decision is already durable and withholding it would hide a hold. But a failed seal left no audit entry and only a warning in the process log, and verify_chain cannot notice an entry that was never written, so that record could later be edited undetected. A failed seal now appends AuditSealFailed to the run's own log, naming the unsealed event (id, sequence, type) and the error, and the enricher counts it (seal_failures). This covers requests, swept expiries and escalations (including a run whose tenant cannot be read) and decisions on both the REST and MCP routes. hitl.md says what verify_chain can and cannot see.
EventKind had no catch-all, so a single event of a kind this build does not know failed the whole log: after an expiry was recorded, an older binary could not read that run at all. EventKind gains a #[serde(other)] Unknown variant that every fold ignores (they all fall through on unmatched kinds). The CHANGELOG states the downgrade limits: builds before this release cannot read approval_expired, approval_escalated or audit_seal_failed, and ignoring an unknown kind is only safe for kinds that restrict nothing, so a newer store should not be read by an older build.
The Unknown event kind kept a newer log loadable, but every fold then ignored it, which is fail-open for an enforcement engine: an unknown kind could be a revocation or a denial that an older build would act past. Unknown stays, so the log still deserializes and lists (GET events shows it as unknown). Every fold that decides coverage or permission now fails closed on it: - the approval fold reports NodeApprovalStatus::Indeterminate for every node of the run, so no approval covers anything; - the node and dispatch guards return Unavailable with "event log contains event kinds this build cannot read" and raise no request; - the approve route refuses any decision (409); - the scheduler does not advance the run (no dispatch, failure, skip, expiry or completion) and records RunStalled once. A test on memory and SQLite shows an approved, re-dispatched node released over such a log before the change; it is now withheld, no request or decision lands, the run stalls once, and listing still works. The CHANGELOG now says an older build refuses to act on a newer log rather than ignoring what it cannot read.
sunilp
force-pushed
the
approval-deadlines
branch
from
October 11, 2026 02:05
bc59ab3 to
93fb3dc
Compare
sunilp
marked this pull request as ready for review
October 11, 2026 02:05
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Approvals can now expire. A deadline never approves anything.
approval_timeout_secsandescalate_after_secs. They can be set at the tenant, workflow and node level. Every level is a ceiling, so a node can tighten the tenant's deadline but never loosen it. Both values are validated at registration, atPUT /tenantsand when a stored policy is read.ApprovalExpiredis recorded with principalsystem:deadlineand the node fails exactly as it would on a rejection. A decision that arrives after expiry gets 409 and records nothing. The decision and the sweep are both compare-and-set appends (append_event_if), so only one of them can win.ApprovalEscalatedis recorded once, atescalate_at, as the hook for Slack or email notifiers.WorkflowStartedand the start node's scheduling, which ran the start node twice.Unknown, and runs fail closed on them.Upgrade note: do not roll back below this release once it has written the new event kinds. Older builds stall on them by design rather than ignoring them.
Every guard was test-first (failing run recorded) and mutation-checked against an unmutated control. Reviewed independently over two rounds.
🤖 Generated with Claude Code