fix(tests): address review — shared disposable-DB guard, optional Redis suite, restored client-info wire contract

Author: waleedlatif1

.agents/skills/cleanup/SKILL.md

@@ -44,7 +44,7 @@ Otherwise apply the surviving changes yourself (in the main context, not delegat
 
 For each pass in turn, apply all of that pass's changes, then move to the next pass. A file touched by several passes is therefore edited once per pass, in this order — not once as a merged patch. This is what makes the ordering real: a single merged-per-file patch would collapse all passes into one edit and lose it.
 
-Comments apply last, on purpose: that pass operates on whatever the earlier structural passes settled the code into, so it never edits lines a sibling pass is about to delete or rewrite.
+Comments apply after every structural pass, on purpose: that pass operates on whatever the earlier passes settled the code into, so it never edits lines a sibling pass is about to delete or rewrite. Tests apply last because they only touch test files; in Step 2, drop any other pass's proposal on a test file the tests pass deletes.
 
 **Treat every Step 1 proposal as snapshot-relative, not authoritative.** All passes analyzed the *original* files in parallel, so a proposal's line ranges and before/after text describe the code as it was *before* any edits — once an earlier pass has run, a later pass's snippet may no longer match. So for each change, before applying:
 

.agents/skills/ship/SKILL.md

@@ -30,9 +30,9 @@ When the user runs `/ship`:
   - Types: `fix`, `feat`, `improvement`, `chore`
   - Scope: short identifier (e.g., `undo-redo`, `api`, `ui`)
   - Keep it concise
-4. **Run the cleanup pass** — only if the diff modifies UI code (any `.tsx` file, or anything under `apps/sim/components/`, `apps/sim/hooks/`, or `apps/sim/stores/`): `/cleanup`
-  - `/cleanup` fans out the React/UI passes (effects, memo, callbacks, state, React Query, emcn, url-state) plus the comment pass; skip it when no UI was touched. When it runs, it applies fixes so they land in this commit.
-  - If the diff adds or changes tests (`*.test.ts(x)`, `*.integration.ts`, `e2e/**`) and `/cleanup` did not run, run `/test-audit audit <changed test files>` on its own. Every new or changed test must pass the authoring gate; delete the ones that don't rather than shipping them.
+4. **Run the cleanup and test gates**
+  - If the diff modifies UI code (any non-test `.tsx` file, or anything under `apps/sim/components/`, `apps/sim/hooks/`, or `apps/sim/stores/`), run `/cleanup`. It fans out the React/UI passes (effects, memo, callbacks, state, React Query, emcn, url-state), the comment pass, and the test-audit pass, and applies fixes so they land in this commit.
+  - Otherwise, if the diff adds or changes tests (`*.test.ts(x)`, `*.integration.ts`, `e2e/**`), run `/test-audit audit <changed test files>` on its own. Every new or changed test must pass the authoring gate; delete the ones that don't rather than shipping them.
 5. **Run migration safety** — only if the diff touches `packages/db/migrations/**` or `packages/db/schema.ts`:
   - Run `/db-migrate` to review the migration for zero-downtime safety (expand/contract phasing, backward-compatibility with the deployed app version).
   - `bun run check:migrations origin/staging` must pass (staging is the PR base). Do not silence a flagged statement with a `-- migration-safe:` annotation unless `/db-migrate` confirmed the old code no longer depends on it; otherwise split the destructive change into a later deploy.

.agents/skills/test-audit/SKILL.md

@@ -99,7 +99,8 @@ is not a deletion reason.
 
 Keep discovery read-only and report evidence before editing. Before judging a candidate, read the
 complete test and its production owner, callers, sibling implementations, overlapping tests, CI
-routing (`.github/workflows/test-build.yml` names DB suites by path), and relevant history
+routing (CI discovers `*.integration.ts` by glob; `.github/workflows/*.yml` names a few scripts and
+files by path), and relevant history
 (`git log --format='%h %s' -5 -- <file>`).
 
 Record for every deletion candidate: the test and location; the failure it can actually detect;
@@ -118,7 +119,7 @@ that restate the same implementation.
 For a whole subsystem or the whole repo:
 
 1. Partition test files into lanes of ~150–350 files by owning directory, and list the protected
-   set (every `*.integration.ts`, `*.postgres.test.ts`, `__integration__/**`, `apps/desktop/e2e/**`,
+   set (every `*.integration.ts`, `*.live.test.ts`, `__integration__/**`, `apps/desktop/e2e/**`,
    and every path named in `.github/workflows/*.yml`).
 2. Give each lane its own git worktree and branch (`git worktree add -b <branch> <path> <base>`,
    then `bun install --frozen-lockfile` inside it). Lanes never share a checkout, never symlink

apps/sim/background/cleanup-table-row-ttl.integration.ts

@@ -6,6 +6,7 @@
 import { writeFileSync } from 'node:fs'
 import { tmpdir } from 'node:os'
 import { join } from 'node:path'
+import { readTestDatabaseUrl } from '@sim/db/testing/test-infrastructure'
 import { createDeferred } from '@sim/testing'
 import { sleep } from '@sim/utils/helpers'
 import { generateId } from '@sim/utils/id'
@@ -34,25 +35,14 @@ import type { TableSchema } from '@/lib/table/types'
 import { checkBatchUniqueConstraintsDb, coerceRowToSchema } from '@/lib/table/validation'
 import { runCleanupTableRowTtl } from '@/background/cleanup-table-row-ttl'
 
-const url = process.env.TEST_DATABASE_URL
-if (url) {
-  const parsed = new URL(url)
-  const otherDatabase = Object.entries(process.env).some(
-    ([key, value]) => /^DATABASE_(URL|REPLICA_URL)(_|$)/.test(key) && value && value !== url
-  )
-  if (
-    !['127.0.0.1', 'localhost'].includes(parsed.hostname) ||
-    !/(^|_)test(_|$)/.test(parsed.pathname.slice(1)) ||
-    process.env.DATABASE_URL !== url ||
-    otherDatabase
-  ) {
-    throw new Error('This suite requires only the disposable local test database')
-  }
+const url = readTestDatabaseUrl()
+const otherDatabase = Object.entries(process.env).some(
+  ([key, value]) => /^DATABASE_(URL|REPLICA_URL)(_|$)/.test(key) && value && value !== url
+)
+if (process.env.DATABASE_URL !== url || otherDatabase) {
+  throw new Error('This suite requires only the disposable local test database')
 }
-const control = postgres(url ?? 'postgres://localhost/disabled_expiration_test', {
-  max: 4,
-  onnotice: () => {},
-})
+const control = postgres(url, { max: 4, onnotice: () => {} })
 const workspaceId = generateId()
 const userId = generateId()
 const expired = '2020-01-01T00:00:00Z'
@@ -113,6 +103,8 @@ async function waitForSleepingDelete(): Promise<number> {
 const [{ migrated }] = await control<{ migrated: boolean }[]>`SELECT EXISTS (
   SELECT 1 FROM pg_trigger WHERE tgname = 'user_table_rows_insert_stmt_trigger'
 ) AS migrated`
+/** The suite's own `afterAll` never runs when it is skipped, so release the probe connection here. */
+if (!migrated) await control.end()
 
 describe.skipIf(!migrated)('Expiration with real PostgreSQL transactions', () => {
   beforeAll(async () => {

apps/sim/ee/access-requests/lib/application/flow.integration.ts

@@ -1,6 +1,7 @@
 import { AuditAction, recordAudit } from '@sim/audit'
 import type { SessionPrincipal } from '@sim/auth/principal'
 import * as schema from '@sim/db/schema'
+import { readTestDatabaseUrl } from '@sim/db/testing/test-infrastructure'
 import { resetEnvFlagsMock, setEnvFlags } from '@sim/testing'
 import { generateId } from '@sim/utils/id'
 import { eq } from 'drizzle-orm'
@@ -9,18 +10,7 @@ import postgres from 'postgres'
 import { afterAll, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'
 import type { DbOrTx } from '@/lib/db/types'
 
-const { databaseUrl, select, transaction } = vi.hoisted(() => {
-  const databaseUrl = process.env.TEST_DATABASE_URL
-  if (databaseUrl) {
-    const url = new URL(databaseUrl)
-    if (
-      !['localhost', '127.0.0.1', '[::1]'].includes(url.hostname) ||
-      !/(^|_)test(_|$)/.test(url.pathname.slice(1))
-    )
-      throw new Error('Use a disposable local test database')
-  }
-  return { databaseUrl, select: vi.fn(), transaction: vi.fn() }
-})
+const { select, transaction } = vi.hoisted(() => ({ select: vi.fn(), transaction: vi.fn() }))
 vi.mock('@/lib/core/config/env-flags', async () => (await import('@sim/testing')).envFlagsMock)
 vi.mock('@sim/db', async () => ({
   ...(await import('@sim/db/schema')),
@@ -54,19 +44,17 @@ import {
 } from '@/ee/access-requests/lib/notification-events'
 
 const schemaName = `access_flow_${generateId().replaceAll('-', '')}`
-const connection = databaseUrl
-  ? postgres(databaseUrl, {
-      max: 3,
-      prepare: false,
-      connection: {
-        search_path: schemaName,
-        application_name: schemaName,
-        statement_timeout: 5000,
-      },
-      onnotice: () => undefined,
-    })
-  : undefined
-const database = connection ? (drizzle(connection, { schema }) as DbOrTx) : undefined
+const connection = postgres(readTestDatabaseUrl(), {
+  max: 3,
+  prepare: false,
+  connection: {
+    search_path: schemaName,
+    application_name: schemaName,
+    statement_timeout: 5000,
+  },
+  onnotice: () => undefined,
+})
+const database = drizzle(connection, { schema }) as DbOrTx
 const session = (userId: string): SessionPrincipal => ({
   kind: 'session',
   userId,
@@ -79,7 +67,6 @@ const target = { kind: 'feature', configKey: 'hideTablesTab' } as const
 const page = { limit: 10, offset: 0 }
 
 beforeAll(async () => {
-  if (!connection) return
   await connection.unsafe(`CREATE SCHEMA "${schemaName}"`)
   await connection.unsafe(`
     CREATE TABLE "user" (
@@ -158,12 +145,11 @@ beforeAll(async () => {
       created_at timestamp DEFAULT now(), processed_at timestamp
     );
   `)
-  select.mockImplementation((fields) => database!.select(fields))
-  transaction.mockImplementation((callback) => database!.transaction(callback))
+  select.mockImplementation((fields) => database.select(fields))
+  transaction.mockImplementation((callback) => database.transaction(callback))
 })
 
 beforeEach(async () => {
-  if (!connection) return
   vi.mocked(recordAudit).mockClear()
   setEnvFlags({ isHosted: true, isBillingEnabled: true, isAccessControlEnabled: true })
   await connection.unsafe(`
@@ -205,7 +191,6 @@ beforeEach(async () => {
 
 afterAll(async () => {
   resetEnvFlagsMock()
-  if (!connection) return
   try {
     await connection.unsafe(`DROP SCHEMA "${schemaName}" CASCADE`)
   } finally {
@@ -235,7 +220,7 @@ function apply(requestId: string, expectedFingerprint: string) {
 }
 
 async function authorizeTables(principal = member, workspaceId = 'primary') {
-  const workspace = await getWorkspaceWithOwner(workspaceId, { executor: database! })
+  const workspace = await getWorkspaceWithOwner(workspaceId, { executor: database })
   if (!workspace) throw new Error('Missing fixture workspace')
   return authorizeWorkspaceOperation(
     principal,
@@ -245,26 +230,26 @@ async function authorizeTables(principal = member, workspaceId = 'primary') {
       workspaceOrganizationId: workspace.organizationId,
       allowPersonalApiKeys: workspace.allowPersonalApiKeys,
     },
-    { executor: database! }
+    { executor: database }
   )
 }
 
 async function storedState() {
-  const [group] = await database!
+  const [group] = await database
     .select({ config: schema.permissionGroup.config })
     .from(schema.permissionGroup)
     .where(eq(schema.permissionGroup.id, 'restricted'))
   return {
     config: group.config,
-    requests: await connection!`
+    requests: await connection`
       SELECT id, status, decision, decision_reason, decided_by, decided_at, updated_at
       FROM permission_access_request ORDER BY id
     `,
-    memberLimits: await connection!`
+    memberLimits: await connection`
       SELECT organization_id, user_id, usage_limit, set_by, updated_at
       FROM organization_member_usage_limit ORDER BY organization_id, user_id
     `,
-    events: await connection!`SELECT event_type FROM outbox_event ORDER BY event_type`,
+    events: await connection`SELECT event_type FROM outbox_event ORDER BY event_type`,
     audit: vi.mocked(recordAudit).mock.calls.map(([entry]) => ({
       action: entry.action,
       actorId: entry.actorId,
@@ -276,7 +261,7 @@ async function storedState() {
   }
 }
 
-describe.skipIf(!databaseUrl)('access request member-to-admin flow on PostgreSQL', () => {
+describe('access request member-to-admin flow on PostgreSQL', () => {
   it('refuses workspace API keys before any protected read or transaction', async () => {
     const readsBefore = select.mock.calls.length
     const transactionsBefore = transaction.mock.calls.length
@@ -342,7 +327,7 @@ describe.skipIf(!databaseUrl)('access request member-to-admin flow on PostgreSQL
       'member',
       'primary',
       'org',
-      database!
+      database
     )
     expect(effective).toMatchObject({
       entitled: true,
@@ -369,7 +354,7 @@ describe.skipIf(!databaseUrl)('access request member-to-admin flow on PostgreSQL
   })
 
   it('fulfills an organization member credit-cap request without changing another member', async () => {
-    await connection!`
+    await connection`
       INSERT INTO organization_member_usage_limit (id, organization_id, user_id, usage_limit, set_by)
       VALUES ('member-cap', 'org', 'member', 10, 'admin'), ('peer-cap', 'org', 'peer', 7, 'admin')
     `
@@ -498,9 +483,9 @@ describe.skipIf(!databaseUrl)('access request member-to-admin flow on PostgreSQL
       const { request } = await create()
       const prepared = await preview(request.id)
       if (change === 'policy') {
-        await connection!`UPDATE permission_group SET config = config || '{"disableTableExport":true}'::jsonb WHERE id = 'restricted'`
+        await connection`UPDATE permission_group SET config = config || '{"disableTableExport":true}'::jsonb WHERE id = 'restricted'`
       } else {
-        await connection!`DELETE FROM permissions WHERE id = 'peer-primary'`
+        await connection`DELETE FROM permissions WHERE id = 'peer-primary'`
       }
       const persisted = await storedState()
       await expect(apply(request.id, prepared.fingerprint)).rejects.toMatchObject({
@@ -565,7 +550,7 @@ describe.skipIf(!databaseUrl)('access request member-to-admin flow on PostgreSQL
         input: { scope: { kind: 'organization', organizationId: 'org' }, ...page },
       })
     ).rejects.toMatchObject({ code: 'not_found' })
-    await connection!`DELETE FROM permissions WHERE id = 'external-primary'`
+    await connection`DELETE FROM permissions WHERE id = 'external-primary'`
     const prepared = await preview(request.id)
     expect(prepared.canApply).toBe(false)
     await expect(apply(request.id, prepared.fingerprint)).rejects.toMatchObject({

apps/sim/ee/access-requests/lib/impact.integration.ts

@@ -1,21 +1,16 @@
+import { readTestDatabaseUrl } from '@sim/db/testing/test-infrastructure'
 import { generateId } from '@sim/utils/id'
 import { drizzle } from 'drizzle-orm/postgres-js'
 import postgres from 'postgres'
 import { describe, expect, it } from 'vitest'
 import type { DbOrTx } from '@/lib/db/types'
 import { loadAccessRequestGroupImpact } from '@/ee/access-requests/lib/impact'
 
-const databaseUrl = process.env.TEST_DATABASE_URL
+const databaseUrl = readTestDatabaseUrl()
 
 async function createFixture() {
-  const url = new URL(databaseUrl ?? '')
-  if (
-    !['localhost', '127.0.0.1'].includes(url.hostname) ||
-    !/(^|_)test(_|$)/.test(url.pathname.slice(1))
-  )
-    throw new Error('Use a disposable local test database')
   const schema = `access_impact_${generateId().replaceAll('-', '')}`
-  const client = postgres(url.toString(), { max: 1, onnotice: () => undefined })
+  const client = postgres(databaseUrl, { max: 1, onnotice: () => undefined })
   await client.unsafe(`CREATE SCHEMA "${schema}"`)
   await client.unsafe(`SET search_path TO "${schema}"`)
   await client.unsafe(`
@@ -45,7 +40,7 @@ async function createFixture() {
   }
 }
 
-describe.skipIf(!databaseUrl)('access request impact on PostgreSQL', () => {
+describe('access request impact on PostgreSQL', () => {
   it('counts scoped people once without requiring or scanning the global user table', async () => {
     const fixture = await createFixture()
     try {

apps/sim/ee/access-requests/lib/repository.integration.ts

@@ -1,4 +1,5 @@
 import { permissionAccessRequest } from '@sim/db/schema'
+import { readTestDatabaseUrl } from '@sim/db/testing/test-infrastructure'
 import { generateId } from '@sim/utils/id'
 import { and, eq } from 'drizzle-orm'
 import { drizzle } from 'drizzle-orm/postgres-js'
@@ -8,17 +9,11 @@ import type { CursorKey } from '@/lib/api/list-query'
 import type { DbOrTx } from '@/lib/db/types'
 import { listAccessRequestRecords } from '@/ee/access-requests/lib/repository'
 
-const databaseUrl = process.env.TEST_DATABASE_URL
+const databaseUrl = readTestDatabaseUrl()
 
 async function createFixture() {
-  const url = new URL(databaseUrl ?? '')
-  if (
-    !['localhost', '127.0.0.1'].includes(url.hostname) ||
-    !/(^|_)test(_|$)/.test(url.pathname.slice(1))
-  )
-    throw new Error('Use a disposable local test database')
   const schema = `access_search_${generateId().replaceAll('-', '')}`
-  const client = postgres(url.toString(), { max: 1, onnotice: () => undefined })
+  const client = postgres(databaseUrl, { max: 1, onnotice: () => undefined })
   await client.unsafe(`CREATE SCHEMA "${schema}"`)
   await client.unsafe(`SET search_path TO "${schema}"`)
   await client.unsafe(`
@@ -52,7 +47,7 @@ async function createFixture() {
   }
 }
 
-describe.skipIf(!databaseUrl)('organization request search on PostgreSQL', () => {
+describe('organization request search on PostgreSQL', () => {
   let fixture: Awaited<ReturnType<typeof createFixture>>
   beforeAll(async () => {
     fixture = await createFixture()

apps/sim/lib/billing/calculations/usage-reservation.integration.ts

@@ -1,7 +1,8 @@
 /**
- * Pooled reservations against real Redis Lua scripts. Requires `TEST_REDIS_URL`; the hosted
+ * Pooled reservations against real Redis Lua scripts. Skipped without `TEST_REDIS_URL`; the hosted
  * billing flags and the Redis client accessor are the only fixtures.
  */
+import { readTestRedisUrl } from '@sim/db/testing/test-infrastructure'
 import { envFlagsMock, setEnvFlags } from '@sim/testing/mocks/env-flags.mock'
 import { redisConfigMock, redisConfigMockFns } from '@sim/testing/mocks/redis-config.mock'
 import { generateId } from '@sim/utils/id'
@@ -16,16 +17,15 @@ import {
 vi.mock('@/lib/core/config/env-flags', () => envFlagsMock)
 vi.mock('@/lib/core/config/redis', () => redisConfigMock)
 
-const redisUrl = process.env.TEST_REDIS_URL
-if (!redisUrl) throw new Error('Set TEST_REDIS_URL to a disposable local Redis')
+const redisUrl = readTestRedisUrl()
 
-describe('pooled usage reservations with Redis', () => {
+describe.runIf(Boolean(redisUrl))('pooled usage reservations with Redis', () => {
   let redis: Redis
   const reservations: string[] = []
   const payer = { type: 'organization' as const, id: generateId() }
 
   beforeAll(async () => {
-    redis = new Redis(redisUrl, { lazyConnect: true, maxRetriesPerRequest: 0 })
+    redis = new Redis(redisUrl!, { lazyConnect: true, maxRetriesPerRequest: 0 })
     await redis.connect()
   })
 

apps/sim/lib/knowledge/__integration__/migration-fixture.ts

@@ -1,16 +1,11 @@
 import { readFile } from 'node:fs/promises'
+import { assertDisposableTestDatabaseUrl } from '@sim/db/testing/test-infrastructure'
 import { generateId } from '@sim/utils/id'
 import postgres from 'postgres'
 
 /** Minimal pre-migration tables in an isolated schema; the entire migration runs unchanged. */
 export async function createEnterpriseSearchMigrationFixture(databaseUrl: string) {
-  const url = new URL(databaseUrl)
-  if (
-    !['localhost', '127.0.0.1'].includes(url.hostname) ||
-    !/(^|_)test(_|$)/.test(url.pathname.slice(1))
-  ) {
-    throw new Error('Search migration tests require a disposable local test database')
-  }
+  assertDisposableTestDatabaseUrl(databaseUrl)
   const client = postgres(databaseUrl, { max: 1, fetch_types: false })
   const schemaName = `search_migration_${generateId().replaceAll('-', '')}`
   await client.unsafe(`CREATE SCHEMA "${schemaName}"`)

apps/sim/lib/knowledge/__integration__/reranker-admission.integration.ts

@@ -71,6 +71,6 @@ describe('reranker shared PostgreSQL admission', () => {
     await expect(rerank('Orion', items, options)).rejects.toMatchObject({
       name: 'ProviderAdmissionTimeoutError',
     })
-    expect(upstream).toHaveBeenCalledTimes(2)
+    expect(upstream).toHaveBeenCalledOnce()
   })
 })