The Unsafe surface: context.unsafe, its builders, executors, ORM proxy and transaction-bound shape - #1204
Conversation
…y and transaction-bound shape `context.prisma` is replaced by `context.unsafe`, a name that states the bypass. The surface carries Prisma's `sql` and `raw` builders untouched, owns `query(plan)` (Prisma's streaming result, through the scope-preserving wrapper) and `execute(plan)` (statistics), and proxies the ORM lane so every call enters the unsafe origin and every returned collection is re-proxied. Neither the bare client nor `prepare()`/`runtime()` is reachable, in the type or in the runtime value. The transaction context carries a transaction-bound shape that keeps the client's contract-scoped raw lane and runs its plans through the transaction's executor. Implements #1144. Part of #1122. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: e289fc8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Coverage Report for Core Package Coverage (./packages/core)
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||
Coverage Report for UI Package Coverage (./packages/ui)
File CoverageNo changed files found. |
Coverage Report for CLI Package Coverage (./packages/cli)
File Coverage
|
||||||||||||||||||||||||||||||||||||||
Coverage Report for Auth Package Coverage (./packages/auth)
File CoverageNo changed files found. |
Coverage Report for Storage Package Coverage (./packages/storage)
File CoverageNo changed files found. |
Coverage Report for RAG Package Coverage (./packages/rag)
File CoverageNo changed files found. |
Coverage Report for Storage S3 Package Coverage (./packages/storage-s3)
File CoverageNo changed files found. |
Coverage Report for Storage Vercel Package Coverage (./packages/storage-vercel)
File CoverageNo changed files found. |
| // `context.transaction` to reach `tx` without closing over the client | ||
| // (ADR-0056). | ||
| if (client !== undefined && _unsafeTransaction === undefined) { | ||
| return client.transaction((tx) => fn(child(prisma, tx))) |
There was a problem hiding this comment.
txContext.db.* now runs outside the transaction this branch opens — and deadlocks on a small pool.
This branch opens a real Prisma 8 interactive transaction, which checks a connection out of the pool for the whole callback, but the child context is built with prisma (the outer ORM handle) as its db binding. So every txContext.db.* call inside fn goes to a different connection.
Concrete failure: the harness in packages/core/src/testing/context.ts builds its pool with max: 1. Once the secured terminals land (spec 3 / #1145), await context.transaction(async (tx) => { await tx.db.post.create({...}) }) blocks forever — the transaction holds the single connection and the db write waits for one that will never free. This is exactly the trap the 1068 spike recorded ("db.transaction() deadlocks on PGlite and on any max:1 pool"). Anything reached through the outer context.unsafe or through a plugin/hook's AccessContext.prisma inside the callback hits the same wall today.
On a larger pool it fails the other way: the db writes auto-commit outside the transaction, so a throw later in fn rolls back only the unsafe work — while StackContext.transaction's own TSDoc still promises "a throw anywhere rolls the whole transaction back (ADR-0012)".
The previous code ran fn directly with no transaction at all, so this PR is what introduces the held lock. Worth either gating this branch until the ORM handle can be transaction-bound, or narrowing the doc contract and noting the pool requirement.
There was a problem hiding this comment.
Fixed — the engine handle is now rebound to the transaction.
runTransactionBody resolves an OrmClient off tx.orm (ormHandleFor(config, tx.orm): orm.<namespace>.<Model> by list key, namespace from listConfig.db?.schema, reads through Reflect.get because the namespaces are get-trap Proxies) and hands that to child(...), so txContext.db.* runs on the transaction's own connection. The same resolver runs against client.orm first as a probe: when it comes back undefined — a hand-built double with no collections — no transaction is opened at all and the callback runs directly, exactly as before this PR. If the transaction's own collections then fail to resolve where the client's succeeded, TransactionOrmHandleError rolls it back rather than binding db to a half-built map.
packages/core/tests/transaction-orm-binding.test.ts covers both failure shapes you named, over a Prisma-8-shaped double that stages writes and models the pool:
- spare pool: a write in a callback that then throws leaves no committed row. Fails on the previous code — the row auto-commits on the outer handle.
- starved pool (the
max: 1case): the outer collection refuses while a transaction is open. The callback completes. Fails on the previous code withPoolExhausted, which is the deadlock made deterministic rather than a hanging test.
Also added to unsafe.test.ts against the real harness: a tx.unsafe write followed by a throw leaves no row.
The pool requirement is now on StackContext.transaction's TSDoc — work reached through the OUTER context inside the callback still asks for a second connection, and that is inherent to holding an interactive transaction on a one-connection pool.
Left alone deliberately: runInTransaction's $transaction probe (#1205).
| fn: (txContext: StackContext<AccessControlledDB>) => Promise<T>, | ||
| registry: TransactionRegistry, | ||
| ormClient: TransactionCapable, | ||
| options?: TransactionOptions, |
There was a problem hiding this comment.
options is silently dropped on the Prisma 8 path.
transaction() still accepts TransactionOptions (isolationLevel, maxWait, timeout) and runTransactionBody still takes it, but it is forwarded only on the legacy $transaction branch below. On the branch that actually runs for a generated context (client.transaction(...)) the parameter is unused.
So await context.transaction(fn, { isolationLevel: 'Serializable' }) — the documented way to protect a read-modify-write today — now runs at Read Committed with no error and no warning, and the caller has no way to notice. A lost-update the caller thought was closed is silently reopened.
Spec 3 plans to delete these options (ADR-0042), but until then this should either throw on a non-empty options here or drop the parameter from the public signature, rather than accept-and-ignore.
There was a problem hiding this comment.
Fixed, by failing loudly. Non-empty options on the Prisma 8 path now rejects with TransactionOptionsUnsupportedError naming the keys, rather than accepting and ignoring them. The Prisma 7 $transaction branch still forwards them verbatim. TransactionOptions' own TSDoc says so now, and the refusal is tested both over the double and over the real harness.
| ) | ||
|
|
||
| if (typeof ormClient.$transaction === 'function') { | ||
| return ormClient.$transaction((tx) => fn(child(tx)), options) as Promise<T> |
There was a problem hiding this comment.
This branch hands fn a context whose unsafe is not transaction-bound.
child(tx) passes undefined for unsafeTransaction, so the child's surface is built by createUnsafeSurface(client) over the outer client. Any tx.unsafe.orm/query/execute inside the callback therefore executes on a different connection, outside the transaction the $transaction call just opened — silently non-atomic, and the opposite of what the branch below does.
Unreachable today (neither the Prisma 8 client nor the harness's ORM map carries $transaction), but it becomes live the moment spec 3 makes the engine's ORM handle transaction-capable — at which point this branch takes precedence over the one below and the transaction-bound surface this PR adds stops being used. Worth deriving the UnsafeTransactionScope here too, or asserting the branch is unreachable.
There was a problem hiding this comment.
Left as-is, with the condition named. The branch now carries a Known limits block saying exactly what you did: it is unreachable while $transaction is a name no Prisma 8 object carries, and the moment the engine's handle becomes transaction-capable and takes this branch, an UnsafeTransactionScope has to be derived here too or tx.unsafe executes outside the transaction it opened. A Prisma 7 tx carries no sql/orm lanes, so there is nothing to derive one from today.
| * anything other than the target's own value for a non-writable, | ||
| * non-configurable data property is a `TypeError`. | ||
| */ | ||
| function isInvariantProperty(target: object, key: string | symbol): boolean { |
There was a problem hiding this comment.
A frozen namespace turns the whole ORM lane into refused queries.
The invariant guard is correct as written, but it is checked for every property, not only for values the trap would otherwise replace. If Prisma ever hands out its collections as own non-writable, non-configurable properties — a plain Object.freeze({ Post, User }) behind the namespace proxy is the obvious shape — isInvariantProperty returns true and unsafe.orm.public.Post is handed back unproxied. Every call through it then runs with no origin in scope and UnmarkedQueryError refuses it, i.e. the entire ORM lane stops working on an ORM patch bump, with an error message that points at the tripwire rather than at here.
rc.8 evidently isn't frozen (the tests pass), which is exactly why this is worth a line in the Known limits block on markCalls — or a narrower guard that only bails when the replacement value would actually differ from value.
There was a problem hiding this comment.
Narrowed as suggested — the guard is now consulted only when the marked value differs from the raw one — but I think the finding overstates what that buys, and I would rather say so than imply I closed something I did not.
The guard is the Proxy invariant: for a non-configurable, non-writable own data property, returning anything but the target's own value from a get trap is a TypeError. So in every case where the old code bailed and the new code also bails, the value is handed back unwrapped either way; where the marked value equals the raw one (primitives, thenables) the bail was already a no-op. The narrowing is therefore behaviour-preserving by construction, and I could not write a test that fails before and passes after it — a frozen namespace still comes back unproxied, because JS will not let it come back any other way.
What I did instead: the residual is now a Known limits line on markCalls naming the shape (Object.freeze({ Post })), the consequence (calls through it carry no origin and the tripwire refuses them), and why rc.8 does not hit it. unsafe.test.ts pins the invariant with an identity assertion on a non-writable, non-configurable own property.
| function markResult(value: unknown): unknown { | ||
| if (isLazyResult(value)) return preserveOrigin('unsafe', value) | ||
| if (isThenable(value)) return value | ||
| if (typeof value === 'object' && value !== null) return markCalls(value) |
There was a problem hiding this comment.
A returned function escapes marking.
The get trap wraps function-valued properties, but markResult does not: typeof value === 'object' is false for a function, so it falls through to return value on the last line and is handed back raw. A method on the ORM lane that returns a callable — a curried/partially-applied builder, a disposer, a queryFn-style closure — is then outside the proxy, and a query issued through it compiles with no origin in scope and is refused by the tripwire.
The asymmetry looks unintentional given the trap already handles the callable case; wrapping a returned function the same way (originStore.run + markResult) would close it.
There was a problem hiding this comment.
Fixed. markResult now wraps a returned callable the same way the get trap wraps a read one — both go through a shared markApply(apply), which takes the application rather than the function so the receiver stays the caller's business (the target for the trap, nothing for a returned callable).
Test: unsafe.test.ts builds a surface over a stand-in orm whose method returns a closure that issues a real query. The closure is invoked outside any scope; the recorder sees origin: "unsafe". It fails on the previous code with UnmarkedQueryError.
| /** | ||
| * The engine's own ORM handle, reached through {@link ormModel}. Internal | ||
| * plumbing rather than the application's escape hatch — that is | ||
| * {@link AccessContext.unsafe}. |
There was a problem hiding this comment.
{@link AccessContext.unsafe} resolves to nothing — AccessContext gains no unsafe member in this PR (that is the #1145 follow-up the description calls out). TypeDoc will emit a broken reference, and a reader following it from a hook concludes context.unsafe is available inside hooks when it is not.
Suggest pointing at StackBaseContext.unsafe (or plain prose) until the field actually lands on AccessContext.
There was a problem hiding this comment.
Fixed — the link pointed at a member AccessContext does not have. It now says plainly that the escape hatch is unsafe on the request context (StackBaseContext.unsafe) and is not a member of this type, with no {@link} to a non-existent member. AccessContext.prisma itself is unchanged, as the description says.
borisno2
left a comment
borisno2
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Reviewed at effort high, strictly from the diff. Six inline comments are posted; this is the summary and the ordering by severity. (Submitted as a Comment review rather than a formal Request-changes because the reviewing identity cannot block on this PR — treat the verdict as stated here.)
Proxy containment — the headline assessment
The containment design is sound and, for the shapes the tests exercise, it holds. markCalls enters originStore.run('unsafe', …) around every method application, re-proxies returned objects, and preserveOrigins the lazy streaming result — the sixteen statement shapes in unsafe.test.ts run against a real client with the tripwire in throw mode and every one records origin: 'unsafe'. Statement counts are asserted (two for a one-row update, three for a nested create), not just "at least one marked", so a silently-unmarked second statement would fail the suite. The Reflect.get(target, key) receiver choice is correct and does not leak: the value returned is still routed through markResult/the function wrapper, so the target-as-receiver is only about #private reads, not about handing back a bare object. isLazyResult's five-member structural test (including Symbol.asyncIterator) correctly refuses to treat a bare Promise as a lazy result. The interleaving test would catch cross-contamination — it asserts per-lane origin sets, so a leaked mark shows up as an extra member rather than passing by construction, and the raw-driver-error test really is prototype identity against the client's own error, not an instanceof Error tautology.
Two holes remain, both narrow but both of the "escapes unmarked" class rather than style:
markResultdoes not wrap functions (unsafe.ts:138). A method that returns a callable is handed back bare; a query issued through it compiles with no origin and the tripwire refuses it. Asymmetric with thegettrap, which does wrap callables. Prisma's current DSL returns builders (objects), so this is latent, not live — but it is exactly the shape an ORM minor bump could introduce.isInvariantPropertyis consulted for every property, not only where the trap would substitute (unsafe.ts:125). If a collection is ever exposed as a frozen own property,unsafe.orm.public.Postcomes back unproxied and the whole lane starts throwing at the tripwire. Narrow the check to the substitution path.
prepare/runtime unreachability — the claim is weaker than stated
The doc comment says "Neither the bare client nor prepare()/runtime() is reachable". What the tests actually establish is not among the surface's own keys: Object.keys(unsafe).sort() plus Reflect.get(unsafe, 'prepare') === undefined. That is a real and useful property (no prototype-chain escape from the surface literal, runtime() resolved per call inside a closure and never handed out, executors returns only query/execute), but it says nothing about prepare reachable through the orm proxy on a Prisma collection, which is where a prepared statement would actually be minted. Either add a test that a prepare reached through the ORM lane is refused, or soften the prose to the claim the code establishes. As written a reader takes "unreachable" for the strong form.
Transaction-bound shape — the most severe finding
context/index.ts:840: the Prisma 8 branch does client.transaction((tx) => fn(child(prisma, tx))) — it opens a real interactive transaction, holding a pooled connection, while the child context's db stays bound to the outer ORM handle. So inside context.transaction:
tx.db.*writes execute outside the transaction that is open, auto-committing, whileStackContext.transaction's own contract says a throw anywhere rolls the whole thing back. That is a silent atomicity break, not a loud one.- On a single-connection pool it deadlocks instead.
The prior code opened no transaction on this path at all, so the held lock is introduced here. The inline comment acknowledges the deferral to spec 3, but the consequence is not stated on StackContext.transaction's TSDoc where a caller would see it. At minimum this needs the contract documented; better, don't hold a connection you cannot enrol the secured writes into.
Two related, smaller ones: options (isolationLevel, maxWait, timeout) is forwarded only on the legacy $transaction branch and silently ignored on the Prisma 8 path generated contexts actually take (:812) — { isolationLevel: 'Serializable' } quietly degrades to Read Committed; and the $transaction branch calls child(tx) with no unsafeTransaction (:831), so tx.unsafe there would run on the outer client — dead today, live the moment the engine's handle becomes transaction-capable.
Type safety
Clean on the stated claim: no as casts and no any in unsafe.ts, the get trap returns unknown narrowed by predicates, lanes are projected off TClient['sql' | 'raw' | 'orm'], and new Proxy<T> carries the target's type. The residual risk is the one you'd expect from a structural stand-in: UnsafeCapableClient declares the three lanes as object, so a real client whose transaction signature or lane shape diverges still compiles at the seam. The tests build the surface over a real client, which mitigates it — worth keeping that coupling deliberate.
Tests
Honest. Real database, real statements, plan-shape and origin assertions only (no SQL text), no skipIf hiding the shapes, and both negative tests fail for the right reason (foreign query → UnmarkedQueryError by type; plan-built-in-scope-run-outside asserts the recorder saw nothing before the throw).
Migration completeness
One missed call site, outside the diff so not commentable inline: examples/rag-openai-chatbot/scripts/install-pgvector.ts:11 still reads (await rawOpensaasContext).prisma, a property this PR removes from StackContext — the script now takes its "Prisma client not found in context" throw. It was already Prisma 6-shaped on this branch, so this is a gap in the rename pass rather than a new break, but it should not ship half-renamed.
Also: access/types.ts:313 carries a TSDoc {@link AccessContext.unsafe} pointing at a member AccessContext does not have (that is the #1145 follow-up) — a broken link that actively tells hook authors context.unsafe exists inside hooks. The deliberate deferral of AccessContext.prisma itself is defensible (it is the engine's internal handle, and auth/rag consume it), but the deferral is only legible if the docs don't claim otherwise.
Conventions otherwise check out: ESM .js extensions throughout, changeset present and scoped, Known limits block used as sanctioned, and the public TSDoc on UnsafeSurface states the bypass plainly — "Everything the secured surface does, this one skips" is the right register for this surface.
To unblock
- Fix or explicitly document the transaction/atomicity behaviour at
context/index.ts:840, and stop silently droppingoptionsat:812. - Wrap functions in
markResult; narrowisInvariantPropertyto the substitution path. - Soften the
prepare/runtimeprose to the claim the tests establish, or test the ORM-lane path. - Fix the
install-pgvector.tscall site and the broken{@link}.
`context.transaction` over a Prisma 8 client opened a real interactive transaction — holding a pooled connection — while the child context's `db` stayed on the outer ORM handle, so its writes committed outside the open transaction or waited for a connection a single-connection pool never frees. The engine's handle is now rebuilt from the transaction's own collections, and no transaction is opened over a client that has none to bind. Transaction `options` a Prisma 8 client cannot honour are refused with `TransactionOptionsUnsupportedError` rather than silently downgraded to the server's default isolation level. Two ORM-proxy containment holes: `markResult` now wraps a returned callable the way the `get` trap already wrapped a read one, so a query issued through it is still marked; the Proxy-invariant guard is consulted only where the trap would substitute, with the residual (a frozen own-property collection) stated as a `Known limits` note. Also narrows the `prepare()`/`runtime()` unreachability claim to what the tests establish, points `AccessContext.prisma`'s TSDoc at a member that exists, and takes `install-pgvector.ts` off the removed `context.prisma`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
# Conflicts: # packages/core/src/context/index.ts # packages/core/src/index.ts
Review addressed —
|
1 participant
Implements #1144. Part of #1122.
context.prismabecomescontext.unsafe— the Unsafe surface (ADR-0038, ADR-0056, ADR-0059, ADR-0062).The identifier
unsafe. ADR-0038 left it to this effort and suggestedunsafeRaw"or similar";rawis wrong now that the surface's largest lane is the ORM proxy, not SQL.unsafeis the wordCONTEXT.md's glossary already uses for the concept ("the Unsafe surface"), so the code and the ubiquitous language agree, and it reads as a warning at every call site it appears in —context.unsafe.orm.public.Post.all(),context.unsafe.query(plan). It is short enough that nobody aliases it away, which is the property a name meant to be visible in review needs.What it is
sqlandraware Prisma's typed builder and raw tag, handed over untouched. No curated subset.query(plan)returns Prisma's lazyAsyncIterableResultthrough The origin module: ambient Engine stamp, tripwire and refusal error #1140'spreserveOrigin, so a bulk re-embed can consume rows with a cursor long after the scope closed;execute(plan)returns statistics throughwithOrigin.runtime()is resolved per execution, never captured and never handed out.ormis a transparentProxy: every method call runs insideoriginStore.run('unsafe', …), every returned object is re-proxied, and every returned lazy result is scope-preserved. No method list to chase.prepare/runtimeis reachable, in the type or in the value.createUnsafeTransactionSurface) keeps the client's contract-scopedrawlane — Prisma'stxhas none — and binds the executors totx.context.transaction(fn)handsfna context whoseunsafeis that shape, so a script inside a transaction need not close over the outer client.Keeping the ORM proxy type-safe without a cast
new Proxy<T>(target, handler)is typedT, so the proxy's static view is Prisma's own — the caller sees the real collection types and nothing is asserted. The trap is declaredget(target: T, key: string | symbol): unknown;unknownis assignable toProxyHandler<T>['get']'sanyreturn, so the handler satisfies the interface while every value inside the trap staysunknownand is narrowed by predicate (isLazyResult,isThenable) before use.Reflect.applyis the onlyanyin the chain and its result is bound straight to anunknown.Two details the implementation had to get right:
Reflect.get(target, key)with the target as receiver, not the proxy. Prisma's collections are class instances with#privatefields, and a private read against the proxy throws.isInvariantProperty), because returning a wrapper for one is aTypeErrorunder the proxy invariants.The surface's own type projects each lane off the client it was built from (
TClient['sql']), so a caller constructing it from a concretely-typed client keeps full precision, while the context's own surface — built through the structuralUnsafeCapableClient, which is what lets a client typed by the app's emitted contract cross the seam without a cast — is structural until the generated bundle instantiates it over the contract (ADR-0052, spec 3). That limit is stated in the surface'sKnown limits.Proving
prepare/runtimeunreachableObject.keys(unsafe).sort()is asserted to be exactly['execute','orm','query','raw','sql'], andReflect.getforprepare,runtime,transaction,connect,closeis assertedundefined.// @ts-expect-erroronunsafe.prepareandunsafe.runtime. Core'stsconfigincludessrc/**/*, so those colocated tests are type-checked bypnpm build— the assertion fails the build if either member ever appears on the type.Tests
packages/core/src/unsafe.test.ts, on #1143's Test context with the tripwire in throw mode, asserting plan shape and origin viacreatePlanRecorder()and never SQL text. 18 tests: the sixteen statement shapes the spike measured (single-statement reads and creates, two-statement update/delete,updateAll/deleteAll+include, three-statement nested create and nested connect, two terminals in one transaction, a DSL plan and a raw plan through the executors, streaming); a foreign query throwingUnmarkedQueryError; a plan built inside a scope and run outside throwing; a lazy result held across anawaitstill covered; interleaved engine/unsafe/foreign calls over the single-connection pool, keyed by plan lane so each origin is checked against its own calls; raw driver errors proved by comparing the prototype of the error from the surface with the one from the client; and the two context-level tests forcontext.unsafeand the transaction-bound shape.Scope notes for the reviewer
AccessContext.prisma— the engine's internal ORM handle, which is what@opensaas/stack-auth's better-auth wiring and@opensaas/stack-rag's vector search actually consume — is deliberately untouched. AddingunsafetoAccessContextrequires one field in an object literal inpackages/core/src/context/write-pipeline.ts, which is Export the access-filter builder and delete the write pipeline's transaction-absence branch #1145's file. Recommend that follow-up land with Export the access-filter builder and delete the write pipeline's transaction-absence branch #1145 or spec 3, along withdocs/content/how-to/write-a-plugin.mdanddocs/content/concepts/hooks.md, whosectx.prismareferences are that internal handle.packages/auth/tests/*-e2e.test.tsstill callcontext.prisma.$disconnect(). Those suites aredescribe.skipIf-gated, are written against the Prisma 6 client that no longer exists on this branch, and are spec 6's to rewrite; changing the name there would be guessing at their replacement.getContexttakes the client as a new trailing option, and the generated context and the test harness both pass it. A context built from a hand-made ORM double getsunavailableUnsafeSurface(), whose every member throwsUnsafeSurfaceUnavailableErrorby name — chosen over typingcontext.unsafeas possibly absent, which would push a null check onto every real caller.CONTEXT.md's Unsafe surface entry now names the identifier;docs/content/reference/context-api.md'sprismasection is rewritten as the surface's recipe.Verification
pnpm build,pnpm lint(0 errors),pnpm manypkg check,pnpm formatall pass.packages/core1517 passed / 1 skipped andpackages/cli332 passed, both withDATABASE_URLunset (PGlite); the core suite passes identically against local PostgreSQL 14 atpostgres://joshcalder@localhost:5432/postgres, with noopensaas_test_%databases left behind. The contract fixture regenerates byte-identical as CI does.🤖 Generated with Claude Code