Close the getContext no-transaction documentation gap by borisno2 · Pull Request #1552 · OpenSaasAU/stack

borisno2 · 2026-09-13T21:37:16Z

Summary

#1262 fixed the mechanism (the write pipeline's constant-false transaction probe), but left open the documented path that lets a caller rebuild the same silent no-transaction shape by hand: getContext(config, requireOrmHandle(config, db.orm), session) type-checks, runs, and drops the write's rollback guarantee without saying anything.

context.db writes now warn once per (list, operation) via console.warn when the context has neither an enclosing transaction to join nor an opener of its own — instead of silently writing outside a transaction. context.transaction() already refused this shape (TransactionUnavailableError); a plain write has no such terminal to refuse from, so it still runs, but no longer silently (packages/core/src/context/write-pipeline.ts).
getContext's client parameter docblock now states that omitting it costs the write transaction, not only that it powers context.unsafe.
requireOrmHandle's docblock now says what the returned handle does not carry (no transaction capability, no Unsafe surface — both come only from the client itself).
Checked packages/core/README.md: it already teaches the current getContext(session?) form, not the stale three-argument one, so no change was needed there.
Amended the still-unreleased eager-writes-open changeset (for fix(core): every write opens a real Prisma 8 transaction (#1205) #1262, not yet published to a changelog): corrected the "now says so" claim for a plain write — it now genuinely does, via the new warning — and recorded OrmHandleUnresolvableError's blast-radius change from a per-list lazy error to a build-time refusal for the whole context, which no changeset previously called out.

Note: the issue text says this targets a prisma-8 integration branch rather than main. That branch no longer exists and the code in question (requireOrmHandle, the Prisma 8 client threading) is already present on main, confirming the integration branch was already merged — so this PR targets main as it should.

Test plan

Added a test proving a plain context.db.User.create() on a clientless context runs (with no rollback guarantee) but warns exactly once via console.warn, and doesn't warn again for a second write to the same list/operation (packages/core/src/context/interactive-transaction.test.ts)
pnpm build (root) passes
pnpm test in packages/core — 1695 passed, 7 skipped, no regressions
pnpm lint passes
pnpm manypkg fix / pnpm format — no changes needed

Closes #1273

🤖 Generated with Claude Code

https://claude.ai/code/session_01AB3Ru72ye9FoGjBMq4bDAo

Generated by Claude Code

#1262 fixed the constant-false transaction-opener probe, but left the path that lets a caller rebuild the same silent shape by hand: getContext(config, requireOrmHandle(config, db.orm), session) type-checks, runs, and drops the write's rollback guarantee with no warning. - context.db writes now warn once per (list, operation) via console.warn when the context has neither an enclosing transaction to join nor its own opener, instead of staying silent (packages/core/src/context/write-pipeline.ts) - getContext's `client` parameter and requireOrmHandle's docblocks now say omitting the client costs the write transaction, not only context.unsafe - packages/core/README.md already documents the current getContext(session?) form, not the stale three-argument one — verified, no change needed - Amended the still-unreleased eager-writes-open changeset: corrected the "now says so" claim for a plain write (it now genuinely does, via the new warning) and recorded OrmHandleUnresolvableError's blast-radius change from a per-list lazy error to a build-time refusal for the whole context Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AB3Ru72ye9FoGjBMq4bDAo

chatgpt-codex-connector · 2026-09-13T21:37:21Z

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

changeset-bot · 2026-09-13T21:37:22Z

⚠️ No Changeset found

Latest commit: 027f4a2

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes changesets to release 9 packages

Name	Type
@opensaas/stack-core	Minor
@opensaas/stack-cli	Minor
@opensaas/stack-auth	Minor
@opensaas/stack-rag	Minor
@opensaas/stack-storage	Minor
@opensaas/stack-tiptap	Minor
@opensaas/stack-ui	Minor
@opensaas/stack-storage-s3	Minor
@opensaas/stack-storage-vercel	Minor

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

vercel · 2026-09-13T21:37:23Z

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

Project	Deployment	Actions	Updated
stack-docs	Ready	Preview	Sep 14, 2026 6:17am UTC

borisno2 · 2026-09-13T21:39:46Z

The test check is failing on check:adr-duplicates, not on anything this PR touches:

Duplicate ADR numbers found in docs/adr/:

  0070:
    - 0070-the-auth-plugins-id-strategy-is-config-not-a-hardcoded-pin.md
    - 0070-the-generated-clients-identity-is-a-process-wide-registry-entry.md

Both files are already on main (added by #1550 and #1548 respectively, merged close together with no shared PR to catch the number collision), and this PR's diff doesn't touch docs/adr/ at all, so the failure reproduces identically on main — it isn't this PR's to fix. I don't see an open PR renumbering either file yet, so there's no fix to port in.

Proposed patch (for whoever owns that renumbering): rename docs/adr/0070-the-generated-clients-identity-is-a-process-wide-registry-entry.md to docs/adr/0071-the-generated-clients-identity-is-a-process-wide-registry-entry.md (the next free number) and update its own # ADR-0070 title line and any cross-references to it (e.g. in specs/prisma-8/architecture-spec.md's decision index, if it lists one).

I'll keep watching this PR and re-check once main picks up a fix.

Generated by Claude Code

borisno2 · 2026-09-13T21:49:16Z

Code review (medium-high effort)

Reviewed the diff directly against origin/main (single commit, 4 files, ~50 net lines). Traced the transaction-ownership invariants through getContext, sudo(), withSession(), and context.transaction()'s runTransactionBody to check the specific concerns raised.

The gate condition is correct

existingOwner === undefined && opener === undefined in write-pipeline.ts (line ~225) captures exactly and only the "no transaction capability at all" case:

context._transactionOwner is only ever set by context.transaction()'s child() closure, which is only reached after runTransactionBody has successfully opened or joined a real transaction — the no-capability shape hits Promise.reject(new TransactionUnavailableError()) first and never reaches child(). So existingOwner truthy always implies a genuine enclosing transaction, never a synthetic placeholder.
sudo() and withSession() both forward the receiver's client and context._transactionOwner unchanged when rebuilding a context, so the opener/owner pairing can't drift across either call.
opener is a function-or-undefined value (TransactionOpener | undefined), so opener === undefined has no falsy-but-present edge case to worry about.

No false positives (warning on a legitimately joined write) or false negatives (missing the truly-no-capability case) found.

Docblock accuracy — one confusing sentence

packages/core/src/context/index.ts, the amended client parameter docblock, says the write:

commits directly against the handle with no rollback guarantee, silently, exactly the defect #1205 fixed (a console.warn names it once per list and operation, but nothing stops the write)

"silently" is immediately undercut by the parenthetical describing the very warning this PR adds. It's readable in context (the writes still aren't stopped, i.e. nothing changes about atomicity), but a reader skimming just the main clause gets the opposite of what the parenthetical says. Worth tightening, e.g. "...with no rollback guarantee — exactly the defect #1205 fixed, undiagnosed until now (a console.warn names it once per list and operation, but nothing stops the write)."

The requireOrmHandle docblock addition and the "field's own docblock" cross-reference are both accurate against the current code.

Reuse: a second copy of the warn-once pattern

write-pipeline.ts's new noTransactionWarnings (Set<string>) / warnNoTransactionCapability is structurally identical to the pre-existing selectWarnings / warnIfSelectIgnored in context/index.ts (same ${listName}.${operation} key shape, same has/add/console.warn-once shape, same doc-comment wording "warn once per (list, operation)"). This PR follows established precedent rather than introducing a new pattern, but there are now two independent copies of the same small mechanism in the same package — a good candidate for a shared warnOnce(set, key, message) helper so a future change (e.g. giving it a reset for tests, or scoping the key to a context/config identity) doesn't have to be applied to both by hand.

Relatedly: the Set is module-level and process-lifetime, keyed only by listName.operation with no context/config identity. In a single process hosting more than one distinct context that shares a list name (a multi-tenant host, a script iterating multiple example apps), the first misconfigured context's warning permanently suppresses the identical warning for a second, independently-misconfigured context for the rest of the process — which is exactly the kind of thing this diagnostic exists to surface. This is pre-existing precedent from selectWarnings, not a new defect, but flagging since it directly affects the mechanism this PR adds.

I also checked whether this creates test-order fragility in interactive-transaction.test.ts: the file has two clientless-context tests, but the first ('a context with no client refuses to open one') calls clientless.transaction(fn), which rejects with TransactionUnavailableError inside runTransactionBody before fn (tx.db.User.create) ever runs — so it never touches noTransactionWarnings, and the new test's expect(warn).toHaveBeenCalledTimes(1) isn't order-dependent on it. Vitest's default per-file module isolation also means this Set doesn't leak across test files. So no actual test-suite bug here.

Changeset amendment — accurate

Checked the "before" claim against git history and the "after" claim against current code:

requireOrmHandle does throw OrmHandleUnresolvableError for any unresolved list at construction time, failing the whole context rather than degrading one list — confirmed in the current implementation, and this behavior predates this PR (it's unchanged by this diff), so the changeset is correctly documenting an already-shipped-but-undocumented behavior change from the still-unreleased fix(core): every write opens a real Prisma 8 transaction (#1205) #1262 changeset, not describing something new this PR does.
The replaced final line — swapping "and now says so" for the console.warn description — corrects a claim that was false/aspirational in the original changeset (no warning existed until this PR actually implemented one).
The #1205/#1273 issue references check out against other existing in-repo references (write-transaction.test.ts already cites #1205 for the same original transaction requirement).

Nothing else stood out as a correctness bug. Overall this is a solid, narrowly-scoped documentation/diagnostics follow-up with no functional risk beyond the minor doc wording nit above.

Generated by Claude Code

- Share the warn-once-per-key mechanism between select-ignored and no-transaction-capability diagnostics instead of two copies of the same Set<string>/has/add pattern, each namespaced so their keys can't collide - Reword the client parameter docblock: it previously said the write fails "silently" in the same sentence that describes the new console.warn Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AB3Ru72ye9FoGjBMq4bDAo

borisno2 · 2026-09-13T21:53:35Z

Addressed both review points in d2ff73a:

Extracted a shared warnOnce(key, message) helper (packages/core/src/lib/warn-once.ts) and moved both warnIfSelectIgnored and the new warnNoTransactionCapability onto it, with distinct key prefixes (select-ignored: / no-transaction:) so the two diagnostics can't collide.
Reworded the client docblock so it no longer says the write fails "silently" in the same breath as describing the console.warn that names it.

pnpm build, pnpm test (packages/core, 1695 passed), pnpm lint and pnpm format all clean on the new head.

Still blocked on the unrelated check:adr-duplicates failure from my earlier comment — that's on main, not this PR.

Generated by Claude Code

github-actions · 2026-09-14T06:34:01Z