fix(tests): centralize test config dir lifecycle to prevent env var pollution (#242)

## Summary

Fixes 37 test failures caused by the Ubuntu 24.04 runner image upgrade
(`20260201.15.1` → `20260209.23.1`, kernel 6.11 → 6.14) that changed
`readdir` ordering, exposing a latent env var pollution bug.

## Root Cause

Bun runs test files **sequentially in one thread** (load → run all tests
→ load next file). Several test files unconditionally `delete
process.env.SENTRY_CONFIG_DIR` in their `afterEach` hooks. When the next
file loads, its module-level code captures `undefined`, causing
`join(undefined, ...)` → `TypeError`.

The runner image upgrade changed the kernel which changed file discovery
order, putting "deleting" files before "capturing" files.

## Changes

- **`test/helpers.ts`**: Added `useTestConfigDir()` helper that creates
a unique temp directory in `beforeEach`, points `SENTRY_CONFIG_DIR` at
it, and **restores** (never deletes) the original value in `afterEach`.
Also handles `closeDatabase()` and temp dir cleanup.
- **11 test files migrated** to use `useTestConfigDir()`, eliminating:
- 3 files that unconditionally deleted `SENTRY_CONFIG_DIR` (root cause)
- 3 files with fragile module-level `process.env[CONFIG_DIR_ENV_VAR]!`
captures
- 2 files with `if (original) restore else delete` patterns (off-by-one
on falsy check)
  - 2 files missing env var restore in `afterEach`
- **`AGENTS.md`**: Added "Test Environment Isolation" section
documenting the pattern and anti-patterns

Net: **-87 lines** (157 added, 244 removed) — less boilerplate, more
safety.

## Verification

- `bun run typecheck` — passes
- `bun run lint` — passes  
- `bun run test:unit` — **1545 tests pass, 0 failures**

Supercedes #240

Author: BYK

AGENTS.md

@@ -383,6 +383,37 @@ Minimal comments, maximum clarity. Comments explain **intent and reasoning**, no
 | Unit tests | `*.test.ts` | `test/` (mirrors `src/`) |
 | E2E tests | `*.test.ts` | `test/e2e/` |
 
+### Test Environment Isolation (CRITICAL)
+
+Tests that need a database or config directory **must** use `useTestConfigDir()` from `test/helpers.ts`. This helper:
+- Creates a unique temp directory in `beforeEach`
+- Sets `SENTRY_CONFIG_DIR` to point at it
+- **Restores** (never deletes) the env var in `afterEach`
+- Closes the database and cleans up temp files
+
+**NEVER** do any of these in test files:
+- `delete process.env.SENTRY_CONFIG_DIR` — This pollutes other test files that load after yours
+- `const baseDir = process.env[CONFIG_DIR_ENV_VAR]!` at module scope — This captures a value that may be stale
+- Manual `beforeEach`/`afterEach` that sets/deletes `SENTRY_CONFIG_DIR`
+
+**Why**: Bun runs test files **sequentially in one thread** (load → run all tests → load next file). If your `afterEach` deletes the env var, the next file's module-level code reads `undefined`, causing `TypeError: The "paths[0]" property must be of type string`.
+
+```typescript
+// CORRECT: Use the helper
+import { useTestConfigDir } from "../helpers.js";
+
+const getConfigDir = useTestConfigDir("my-test-prefix-");
+
+// If you need the directory path in a test:
+test("example", () => {
+  const dir = getConfigDir();
+});
+
+// WRONG: Manual env var management
+beforeEach(() => { process.env.SENTRY_CONFIG_DIR = tmpDir; });
+afterEach(() => { delete process.env.SENTRY_CONFIG_DIR; }); // BUG!
+```
+
 ### Property-Based Testing
 
 Use property-based tests when verifying invariants that should hold for **any valid input**.

package.json

@@ -59,7 +59,7 @@
   "engines": {
     "node": ">=22"
   },
-  "packageManager": "bun@1.3.3",
+  "packageManager": "bun@1.3.9",
   "patchedDependencies": {
     "@stricli/core@1.2.5": "patches/@stricli%2Fcore@1.2.5.patch",
     "@sentry/core@10.38.0": "patches/@sentry%2Fcore@10.38.0.patch"

src/lib/oauth.ts

@@ -24,12 +24,20 @@ const SENTRY_URL = process.env.SENTRY_URL ?? "https://sentry.io";
  * Build-time: Injected via Bun.build({ define: { SENTRY_CLIENT_ID: "..." } })
  * Runtime: Can be overridden via SENTRY_CLIENT_ID env var (for self-hosted)
  *
+ * Read at call time (not module load time) so tests can set process.env.SENTRY_CLIENT_ID
+ * after module initialization.
+ *
  * @see script/build.ts
  */
 declare const SENTRY_CLIENT_ID_BUILD: string | undefined;
-const SENTRY_CLIENT_ID =
-  process.env.SENTRY_CLIENT_ID ??
-  (typeof SENTRY_CLIENT_ID_BUILD !== "undefined" ? SENTRY_CLIENT_ID_BUILD : "");
+function getClientId(): string {
+  return (
+    process.env.SENTRY_CLIENT_ID ??
+    (typeof SENTRY_CLIENT_ID_BUILD !== "undefined"
+      ? SENTRY_CLIENT_ID_BUILD
+      : "")
+  );
+}
 
 // OAuth scopes requested for the CLI
 const SCOPES = [
@@ -85,7 +93,8 @@ async function fetchWithConnectionError(
 
 /** Request a device code from Sentry's device authorization endpoint */
 function requestDeviceCode() {
-  if (!SENTRY_CLIENT_ID) {
+  const clientId = getClientId();
+  if (!clientId) {
     throw new ConfigError(
       "SENTRY_CLIENT_ID is required for authentication",
       "Set SENTRY_CLIENT_ID environment variable or use a pre-built binary"
@@ -99,7 +108,7 @@ function requestDeviceCode() {
         method: "POST",
         headers: { "Content-Type": "application/x-www-form-urlencoded" },
         body: new URLSearchParams({
-          client_id: SENTRY_CLIENT_ID,
+          client_id: clientId,
           scope: SCOPES,
         }),
       }
@@ -142,7 +151,7 @@ function pollForToken(deviceCode: string): Promise<TokenResponse> {
         method: "POST",
         headers: { "Content-Type": "application/x-www-form-urlencoded" },
         body: new URLSearchParams({
-          client_id: SENTRY_CLIENT_ID,
+          client_id: getClientId(),
           device_code: deviceCode,
           grant_type: "urn:ietf:params:oauth:grant-type:device_code",
         }),
@@ -313,7 +322,8 @@ export async function setApiToken(token: string): Promise<void> {
 export function refreshAccessToken(
   refreshToken: string
 ): Promise<TokenResponse> {
-  if (!SENTRY_CLIENT_ID) {
+  const clientId = getClientId();
+  if (!clientId) {
     throw new ConfigError(
       "SENTRY_CLIENT_ID is required for token refresh",
       "Set SENTRY_CLIENT_ID environment variable or use a pre-built binary"
@@ -327,7 +337,7 @@ export function refreshAccessToken(
         method: "POST",
         headers: { "Content-Type": "application/x-www-form-urlencoded" },
         body: new URLSearchParams({
-          client_id: SENTRY_CLIENT_ID,
+          client_id: clientId,
           grant_type: "refresh_token",
           refresh_token: refreshToken,
         }),

test/commands/cli/fix.test.ts

@@ -3,19 +3,16 @@
  */
 
 import { Database } from "bun:sqlite";
-import { afterEach, beforeEach, describe, expect, mock, test } from "bun:test";
-import { mkdirSync, rmSync } from "node:fs";
+import { describe, expect, mock, test } from "bun:test";
 import { join } from "node:path";
 import { fixCommand } from "../../../src/commands/cli/fix.js";
-import {
-  CONFIG_DIR_ENV_VAR,
-  closeDatabase,
-} from "../../../src/lib/db/index.js";
+import { closeDatabase } from "../../../src/lib/db/index.js";
 import {
   EXPECTED_TABLES,
   generatePreMigrationTableDDL,
   initSchema,
 } from "../../../src/lib/db/schema.js";
+import { useTestConfigDir } from "../../helpers.js";
 
 /**
  * Generate DDL for creating a database with pre-migration tables.
@@ -64,46 +61,12 @@ function createDatabaseWithMissingTables(
   ).run();
 }
 
-let testDir: string;
-let originalConfigDir: string | undefined;
-
-beforeEach(() => {
-  // Save original config dir
-  originalConfigDir = process.env[CONFIG_DIR_ENV_VAR];
-
-  // Close any existing database connection
-  closeDatabase();
-
-  // Create unique test directory
-  const baseDir = originalConfigDir ?? "/tmp/sentry-cli-test";
-  testDir = join(
-    baseDir,
-    `fix-test-${Date.now()}-${Math.random().toString(36).slice(2)}`
-  );
-  mkdirSync(testDir, { recursive: true });
-  process.env[CONFIG_DIR_ENV_VAR] = testDir;
-});
-
-afterEach(() => {
-  closeDatabase();
-  // Restore original config dir
-  if (originalConfigDir) {
-    process.env[CONFIG_DIR_ENV_VAR] = originalConfigDir;
-  } else {
-    delete process.env[CONFIG_DIR_ENV_VAR];
-  }
-  // Clean up test directory
-  try {
-    rmSync(testDir, { recursive: true, force: true });
-  } catch {
-    // Ignore cleanup errors
-  }
-});
+const getTestDir = useTestConfigDir("fix-test-");
 
 describe("sentry cli fix", () => {
   test("reports no issues for healthy database", async () => {
     // Create healthy database
-    const db = new Database(join(testDir, "cli.db"));
+    const db = new Database(join(getTestDir(), "cli.db"));
     initSchema(db);
     db.close();
 
@@ -124,7 +87,7 @@ describe("sentry cli fix", () => {
 
   test("detects and reports missing columns in dry-run mode", async () => {
     // Create database with pre-migration tables (missing v4 columns)
-    const db = new Database(join(testDir, "cli.db"));
+    const db = new Database(join(getTestDir(), "cli.db"));
     createPreMigrationDatabase(db);
     db.close();
 
@@ -148,7 +111,7 @@ describe("sentry cli fix", () => {
 
   test("fixes missing columns when not in dry-run mode", async () => {
     // Create database with pre-migration tables (missing v4 columns)
-    const db = new Database(join(testDir, "cli.db"));
+    const db = new Database(join(getTestDir(), "cli.db"));
     createPreMigrationDatabase(db);
     db.close();
 
@@ -169,7 +132,7 @@ describe("sentry cli fix", () => {
 
     // Verify the column was actually added
     closeDatabase();
-    const verifyDb = new Database(join(testDir, "cli.db"));
+    const verifyDb = new Database(join(getTestDir(), "cli.db"));
     const cols = verifyDb.query("PRAGMA table_info(dsn_cache)").all() as Array<{
       name: string;
     }>;
@@ -188,7 +151,7 @@ describe("sentry cli fix", () => {
   // that was previously missing tables (now fixed by auto-repair at startup).
   test("handles database that was auto-repaired at startup", async () => {
     // Create database missing dsn_cache - initSchema will create it when command runs
-    const db = new Database(join(testDir, "cli.db"));
+    const db = new Database(join(getTestDir(), "cli.db"));
     createDatabaseWithMissingTables(db, ["dsn_cache"]);
     db.close();
 
@@ -210,7 +173,7 @@ describe("sentry cli fix", () => {
 
     // Verify the table was created (by initSchema auto-repair)
     closeDatabase();
-    const verifyDb = new Database(join(testDir, "cli.db"));
+    const verifyDb = new Database(join(getTestDir(), "cli.db"));
     const tables = verifyDb
       .query(
         "SELECT name FROM sqlite_master WHERE type='table' AND name='dsn_cache'"
@@ -221,7 +184,7 @@ describe("sentry cli fix", () => {
   });
 
   test("shows database path in output", async () => {
-    const db = new Database(join(testDir, "cli.db"));
+    const db = new Database(join(getTestDir(), "cli.db"));
     initSchema(db);
     db.close();
 
@@ -237,6 +200,6 @@ describe("sentry cli fix", () => {
 
     const output = stdoutWrite.mock.calls.map((c) => c[0]).join("");
     expect(output).toContain("Database:");
-    expect(output).toContain(testDir);
+    expect(output).toContain(getTestDir());
   });
 });