fix(approvals): a schedule-triggered run can write its own locked record (#3712) - #3749
Merged
Merged
Conversation
…ord (#3712) #3456 let the run that opened a pending approval write its own target record, keyed on `flowRunId`. It worked for every run that resolves an identity and missed the one that doesn't: an effective `runAs:'user'` run with no trigger user — a schedule — passed no ObjectQL context at all, so nothing carried the run id and the run still died on its own `RECORD_LOCKED`. The blocker was never the lock. It was that "no identity" and "no context" were the same thing on the wire, so a run could not say who it was without also claiming what it was allowed to do. A run with no principal now passes provenance alone. `resolveRunDataContext` returns `{ flowRunId }` — no userId, no positions, no permissions, not even `isSystem: false`. Every principal gate keys on one of those fields (the elevation short-circuit on `isSystem`, the ADR-0103 engine-owned write guard and the ADR-0090 D12 delegated-admin gate on `userId`, the empty-principal fall-open on all three), so this context authorizes identically to no context at all. The run keeps its documented #1888 unscoped posture, its `[runAs]` warning, and the `flow-schedule-runas-unscoped` lint. Only attribution changed. Provenance moved out of the hook session into `ctx.provenance`. `session` answers who is calling and is absent when no identity envelope was supplied — a distinction real gates depend on (the attachment access gate skips bare-kernel writes on exactly that test). Folding a run id into `session` would have forced an identity-less run to present an empty session, silently turning "no caller" into "an anonymous caller" and narrowing the #1888 fail-open for attachments alone. Also relaxes `BaseEngineOptionsSchema.context` to a partial envelope (`ExecutionContextInput`). Parse-time defaults on positions/permissions/isSystem made them required on a caller-supplied option, asserting something untrue: that every data-engine context carries a principal. Callers have always passed slices. The #3703 miss was a hand-off gap, not a logic gap — every hop worked alone — so the new integration test stubs no hop: real `resolveRunDataContext`, real ObjectQL engine, real lock hook. Refs #3456, #3703, #1888. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QFUmz96zRXzfypG4Fa9BXJ
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
github-actions
Bot
added
documentation
Contributor
📓 Docs Drift CheckThis PR changes 5 package(s): 113 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
|
…e envelope Follow-up to the same change: `BaseEngineOptionsSchema.context` is now `ExecutionContextInput`, so the contract doc's interface listings should say so rather than implying a caller must hand over a full principal. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QFUmz96zRXzfypG4Fa9BXJ
…PI surface The public API-surface gate flagged it: 0 breaking, 1 added. Intentional — the partial execution envelope is a new named type, nothing was removed or narrowed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QFUmz96zRXzfypG4Fa9BXJ
os-zhuang
marked this pull request as ready for review
July 28, 2026 02:01
os-zhuang
added a commit
that referenced
this pull request
Jul 28, 2026
…) (#3784) An effective `runAs:'user'` run that resolves no trigger user executed its data nodes UNSCOPED: it presented no principal, and the data security middleware skips when there is no principal, so the run read and wrote every row. `runAs:'user'` is an access-NARROWING declaration, and ADR-0049's standing rule is that failing to resolve one must never resolve to a grant. It now throws `UnscopedRunDataAccessError` from `resolveRunDataContext` — the single place every data node resolves its context. This was never really about schedules. The docs, the spec, the runtime warning and the lint all described a schedule-shaped problem, and the lint only matched that shape, but the runtime predicate is "no user". The commonest way to have no user is a record-change flow fired by a write that carried none: `isSystem` does not suppress trigger dispatch — only `skipTriggers` does, and three first-party paths set it — so plugin/service system writes, the approvals status mirror, and a `runAs:'system'` flow's own data node all dispatched record-change flows with `userId: undefined`. Ordinary users reach those writes routinely, so the fail-open was reachable by unprivileged input. Deliberately NOT implemented as "inherit the write's posture and run as isSystem". That reads like a relabel but is an escalation: the middleware's isSystem short-circuit (security-plugin.ts:722) fires before the package-managed-row, system-row, audience-anchor and delegated-admin gates (795/809/817/844), all of which a principal-less context still clears. Such a run cannot write sys_user_position today; as isSystem it could. - Lint `flow-schedule-runas-unscoped` -> `flow-runas-unscoped`, now FAILS the build and covers time_relative + api (ADR-0073 D5). It documented itself as "NEVER fails the build" — a gate that behaved as a comment. It still cannot cover record_change, which is undecidable at authoring time. - Three seed writes (seed-loader pass-2 back-fill, both app-plugin fallbacks) inlined a bare `{isSystem:true}` and so seeded with automation live. - #3712's user-less provenance path is subsumed: such runs are refused before the approvals lock is consulted, and a schedule reaches its own record via `runAs:'system'`. The flowRunId exemption stays live for runs with a user. - ADR-0073 amended: its "no untrusted-input path" severity claim is falsified, and its rejection of fail-closed expired when the example flows it cited were fixed to declare `runAs:'system'`. Both new gates were verified to go red against the pre-fix behavior, including the live dogfood stack, which previously pinned the fail-open explicitly. Refs #1888, #3712, #3749, #3456, ADR-0049, ADR-0073. Co-authored-by: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Co-authored-by: Claude <noreply@anthropic.com>
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
2 participants
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.
Closes #3712. Residual of #3456, left open by #3703.
The problem
#3703 exempted the run that opened the pending request from the approvals record lock, keyed on
flowRunId. It worked for every run that resolves an identity, and missed the one that doesn't: an effectiverunAs:'user'run with no trigger user — a schedule being the canonical case — passed no ObjectQL context at all, so nothing carried the run id and the run still died on its ownRECORD_LOCKED.The blocker was never the lock. It was that "no identity" and "no context" were the same thing on the wire, so a run could not say who it was without also claiming what it was allowed to do.
The issue framed that as a choice: manufacture a principal (option 1, accepting a flip to baseline-member RLS), or resolve the #1888 fail-open first (option 2). This takes neither — it takes the third door the issue left open: carry
flowRunIdwithout presenting an identity the middleware keys on. The fail-open decision stays exactly where it was, unprejudiced.What changed
A run with no principal passes provenance alone.
resolveRunDataContextreturns{ flowRunId }— nouserId, nopositions, nopermissions, not evenisSystem: false. That absence is load-bearing. Every principal gate keys on one of those fields:context.isSystemcontext.userIdcontext.userId(missing context normalized to{}first)positions/permissions/userIdSo the envelope is indistinguishable from passing no context at all. The run keeps its documented #1888 unscoped posture, its loud
[runAs]warning, and theflow-schedule-runas-unscopedbuild-time lint. Nothing about what it may touch changed — only that it can now be attributed.Provenance moved out of the hook session, into
ctx.provenance.sessionanswers who is calling, and is absent when no identity envelope was supplied — a distinction real gates depend on (the attachment access gate skips bare-kernel writes on exactly that test). Folding a run id intosessionwould have forced an identity-less run to present an empty session, silently turning "no caller" into "an anonymous caller" and narrowing the #1888 fail-open for attachments alone. An inconsistent partial tightening is worse than either extreme, soHookContext.provenance.flowRunIdnow says what produced the write and the lock reads it there.buildSessionreturnsundefinedwhen a context yields nothing session-worthy; every real transport resolvespositions, so an anonymous HTTP request still yields a session and stays gated.BaseEngineOptionsSchema.contextrelaxed to a partial envelope (ExecutionContextInput). Parse-time defaults onpositions/permissions/isSystemmade them required on a caller-supplied option — asserting something untrue, that every data-engine context carries a principal. Callers have always passed slices ({ isSystem: true }for a system read); the type now says so.Testing
pnpm build(71/71) ·pnpm test(132/132) ·pnpm lint— all green.The #3703 miss was a hand-off gap, not a logic gap: every hop worked in isolation. So the new integration test (
record-lock-schedule-run.integration.test.ts) stubs no hop — realresolveRunDataContext, real ObjectQL engine, real lock hook, in the order a live deployment runs them. It fails onmain.Also pinned:
flowRunId— every principal field asserted absent by name;assertEngineOwnedWriteAllowed,DelegatedAdminGate) treat it exactly like no context;ctx.sessionstaysundefinedfor a provenance-only write, and stays defined for an anonymous transport request;Notes
session.flowRunIdnever shipped — fix(approvals): a dead approval run no longer leaves the record RECORD_LOCKED (#3456) #3703's changeset is still pending — so the move costs no consumer a migration. Its stale "residual, deliberately not changed here" paragraph is updated in the same commit, since both ship in one release.content/docs/references/data/hook.mdxis regenerated, not hand-edited.🤖 Generated with Claude Code
https://claude.ai/code/session_01QFUmz96zRXzfypG4Fa9BXJ
Generated by Claude Code