Agree the test harness escape with resolveDatabaseUrl()'s variable order by borisno2 · Pull Request #1547 · OpenSaasAU/stack

borisno2 · 2026-09-13T11:45:09Z

Summary

resolveDatabaseUrl() (packages/core/src/db/url.ts) reads DIRECT_DATABASE_URL first, then DATABASE_URL. The test harness's escape (packages/core/src/testing/escape.ts) only ever read DATABASE_URL, so an environment with only DIRECT_DATABASE_URL set silently fell back to in-process PGlite while the application would have used a real Postgres — a quiet divergence rather than a loud one.
packages/core/src/db/url.ts now exports CONNECTION_VARIABLES, the one place the ordered variable list is written down.
packages/core/src/testing/escape.ts re-exports it as ESCAPE_VARIABLES and readDatabaseEscape() walks it in the same order resolveDatabaseUrl() uses — the first variable that is set and non-empty wins, so DIRECT_DATABASE_URL alone is classified exactly as the application would resolve it.
DatabaseEscape's 'unusable' variant and UnusableDatabaseEscapeError now carry the actual variable name, so a set-but-unusable value still fails loudly naming the variable, its value, the fault and both remedies (unchanged pattern from The Dev database primitive and a URL lookup with provenance #1197) — whichever of the two variables was the culprit.
Renamed the single-variable ESCAPE_VARIABLE to ESCAPE_VARIABLES across every consumer (skip-reason messages in core's and rag's test suites); no production behavior change outside @opensaas/stack-core.
pglite-absence.test.ts now clears both connection variables before asserting the PGlite-missing path, matching the harness's real lookup.

Test plan

New packages/core/src/testing/escape.test.ts: unit coverage for readDatabaseEscape()/requireUsableDatabaseEscape() — absent, DATABASE_URL alone, DIRECT_DATABASE_URL alone (the bug fix), DIRECT_DATABASE_URL winning over DATABASE_URL, an unusable value in either variable (including an unusable DIRECT_DATABASE_URL outranking a usable DATABASE_URL, matching resolveDatabaseUrl()'s order), and the thrown error naming the variable and both remedies.
pnpm build in packages/core (typecheck)
pnpm test in packages/core — 1767 passed, 7 skipped
pnpm build + targeted vitest run in packages/rag for the two touched suites — 30 passed
pnpm lint, pnpm format, pnpm manypkg fix at the repo root — clean
Changeset added for @opensaas/stack-core (patch)

Closes #1210

🤖 Generated with Claude Code

https://claude.ai/code/session_018peyBGVF5g4MX7qUiq68UG

Generated by Claude Code

…variable order The harness's DATABASE_URL-only escape and the application's resolveDatabaseUrl() (DIRECT_DATABASE_URL first, then DATABASE_URL) disagreed about which variable counts, so a DIRECT_DATABASE_URL-only environment silently ran the suite on PGlite instead of the named server. readDatabaseEscape() now walks the same CONNECTION_VARIABLES list url.ts exports, checked in the same order, so the two lookups cannot drift apart again. A set-but-unusable value in either variable still fails loudly, naming the variable and both remedies. Closes #1210 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018peyBGVF5g4MX7qUiq68UG

vercel · 2026-09-13T11:45:13Z

Deployment failed for project stack-docs with the following error:

Resource is limited - try again in 24 hours (more than 100, code: "api-deployments-free-per-day").

Learn More: https://vercel.com/open-saas?upgradeToPro=build-rate-limit

chatgpt-codex-connector · 2026-09-13T11:45:13Z

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

changeset-bot · 2026-09-13T11:45:14Z

🦋 Changeset detected

Latest commit: 7f3fe58

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 9 packages

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

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

borisno2 · 2026-09-13T11:47:51Z

Code Review

I reviewed this PR's diff against main using the code-review skill, tracing every changed line, its callers/callees, and cross-file consumers of the renamed ESCAPE_VARIABLE(S) / CONNECTION_VARIABLES symbols and UnusableDatabaseEscapeError's new constructor signature.

Result: no blocking issues found.

CONNECTION_VARIABLES is exported once from packages/core/src/db/url.ts and correctly re-exported as ESCAPE_VARIABLES from packages/core/src/testing/escape.ts.
Every consumer of the old singular ESCAPE_VARIABLE was updated (verified via a repo-wide search, including examples/ and docs/) — no stray references remain outside the diff.
The new readDatabaseEscape() loop correctly mirrors lookupDatabaseUrl()'s precedence order (DIRECT_DATABASE_URL before DATABASE_URL) and empty-string handling, closing the exact divergence described in Harness escape variable and resolveDatabaseUrl disagree: DIRECT_DATABASE_URL silently falls back to PGlite #1210.
UnusableDatabaseEscapeError's new variable parameter is threaded through its one call site, so a bad value now fails loudly naming which variable was the culprit.
The new packages/core/src/testing/escape.test.ts suite gives focused coverage of the previously-untested cases: DIRECT_DATABASE_URL alone, DIRECT_DATABASE_URL winning over DATABASE_URL, and an unusable DIRECT_DATABASE_URL outranking a usable DATABASE_URL — exactly the precedence bug this PR fixes.

One optional, non-blocking observation (pre-existing, not introduced by this diff): packages/core/src/db/dev-database.test.ts (lines 366–394) still carries its own independent copy of the same DIRECT_DATABASE_URL/DATABASE_URL classification logic rather than importing the now-shared readDatabaseEscape()/CONNECTION_VARIABLES. It's harmless today since its literal ordering already happens to agree, but it slightly undercuts this PR's own goal of having one place the variable list is written down, and could drift later if a third connection variable is ever added. Not a blocker — just worth a follow-up if convenient.

No changes requested; this is good to merge from a correctness standpoint.

Generated by Claude Code

…d of a private copy The suite carried its own classification of DIRECT_DATABASE_URL/DATABASE_URL, duplicating the exact logic #1210 unified in testing/escape.ts. Already correctly ordered, so no behavior change — this just removes the second copy that could have drifted from the shared one. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018peyBGVF5g4MX7qUiq68UG

borisno2 · 2026-09-13T11:52:12Z

Addressed the review's nit: packages/core/src/db/dev-database.test.ts now imports readDatabaseEscape() from testing/escape.js instead of carrying its own copy of the classification logic (7f3fe58). No behavior change — it was already correctly ordered — this just removes the second copy that could have drifted from the shared one, per the issue's own preference for "one shared definition."

Generated by Claude Code

borisno2 · 2026-09-13T11:52:53Z

The Vercel status is failing with "Deployment rate limited — retry in 24 hours" (api-deployments-free-per-day — more than 100 deployments today on the stack-docs project's free tier). This is an account-level Vercel quota, unrelated to this PR's diff — the PR touches no docs content, and there's no code fix to port for a deployment rate limit. I have no way to re-trigger a Vercel deployment or raise the quota from here, so I'll leave this one and keep watching the PR; it should clear once the 24-hour window resets.

Generated by Claude Code

borisno2 · 2026-09-13T12:13:20Z

The test check failed on one test: packages/ui/tests/components/ItemFormAccessItemLayout.test.ts > the derived item-view layout checks field access against the real row (#1402) > reads the real (bounded) relation, not an empty/missing one, for a details field access rule, with Error: Test timed out in 5000ms (69/70 tests in that suite passed).

This isn't this PR's failure: the diff touches only packages/core (testing/escape.ts, testing/context.ts, testing/index.ts, db/url.ts, db/dev-database.test.ts) and two packages/rag test files (an import rename only) — nothing in packages/ui or the item-view/access-form code path the failing test exercises. CI's test job already sets DATABASE_URL (not DIRECT_DATABASE_URL) to the Postgres service container, so this PR's precedence-order fix resolves to the exact same connection string before and after — no behavior change for this job's readDatabaseEscape()/resolveDatabaseUrl() calls. The job log shows heavy concurrent Postgres traffic from the other suites sharing the one service container (many FATAL: role "root" does not exist / connection-churn lines), consistent with a resource-contention timeout rather than a real regression.

Re-running the failed job once to confirm.

Generated by Claude Code

github-actions · 2026-09-13T12:25:33Z