Fix: Make SQL connection string parsing more sensible

[alii]

This PR makes connection string parsing more sensible in Bun.SQL, without breaking the default fallback of postgres

Added some tests checking for connection string precedence

[robobun]

^{Updated 9:28 PM PT - Sep 8th, 2025}

❌ @alii, your commit 2c85900 has 1 failures in Build #25485:

• test/js/sql/sql-mysql.test.ts - pid 36527 segmentation fault on 🐧 25.04 x64-baseline

---

🧪   To try this PR locally:

bunx bun-pr 22260

That installs a local version of the PR into your bun-22260 executable, so you can run:

bun-22260 --bun

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/js/internal/sql/shared.ts (1)

11-15: Reintroduce affectedRows or update its usages

• SQLResultArray<T> no longer declares public affectedRows, yet src/js/internal/sql/mysql.ts still assigns to it (lines 34 & 66) and test/js/sql/sql-mysql.test.ts destructures it at line 36.
• Either add back public affectedRows!: number | null; in shared.ts or remove/update these assignments and tests.

🧹 Nitpick comments (1)

test/js/sql/adapter-env-var-precedence.test.ts (1)

121-127: Improve error assertion specificity.

The current regex /mysql.*postgres/i is quite broad and might match unintended error messages. Consider using a more specific error pattern.

-    }).toThrow(/mysql.*postgres/i);
+    }).toThrow(/Protocol 'mysql' is not compatible with adapter 'postgres'/);

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 35e9f3d and a98c72e.

📒 Files selected for processing (2)

• src/js/internal/sql/shared.ts (1 hunks)
• test/js/sql/adapter-env-var-precedence.test.ts (1 hunks)

🧰 Additional context used

📓 Path-based instructions (8)

**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (CLAUDE.md)

Format JavaScript/TypeScript with Prettier (bun run prettier)

Files:

• src/js/internal/sql/shared.ts
• test/js/sql/adapter-env-var-precedence.test.ts

src/js/**/*.{ts,js}

📄 CodeRabbit inference engine (CLAUDE.md)

src/js/**/*.{ts,js}: In built-in modules, require() must use string literals resolved at compile time
In built-in modules, use export default (converted to a return statement by the preprocessor)

Files:

• src/js/internal/sql/shared.ts

test/**/*.test.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

test/**/*.test.{ts,tsx}: Place tests in test/ and ensure filenames end with .test.ts or .test.tsx
Always use port: 0 in tests; never hardcode port numbers or use custom random-port functions
Prefer snapshot tests using normalizeBunSnapshot(...).toMatchInlineSnapshot(...) over exact string comparisons
Never write tests that assert absence of crashes (e.g., no 'panic' or 'uncaught exception' in output)
Avoid shell commands like find/grep in tests; use Bun.Glob and built-in tools instead

Files:

• test/js/sql/adapter-env-var-precedence.test.ts

test/**/*.test.ts

📄 CodeRabbit inference engine (test/CLAUDE.md)

test/**/*.test.ts: Use bun:test in files that end in *.test.ts
Prefer async/await over callbacks in tests
For single-callback flows in tests, use Promise.withResolvers()
Do not set timeouts on tests; rely on Bun’s built-in timeouts
Use tempDirWithFiles from harness to create temporary directories/files in tests
Organize tests with describe blocks
Always assert exit codes and error scenarios in tests (e.g., proc.exited, expect(...).not.toBe(0), toThrow())
Use describe.each for parameterized tests; toMatchSnapshot for snapshots; and beforeAll/afterEach/beforeEach for setup/teardown
Avoid flaky tests: never wait for arbitrary time; wait for conditions to be met

Files:

• test/js/sql/adapter-env-var-precedence.test.ts

test/{**/*.test.ts,**/*-fixture.ts}

📄 CodeRabbit inference engine (test/CLAUDE.md)

test/{**/*.test.ts,**/*-fixture.ts}: When spawning Bun processes, use bunExe() and bunEnv from harness
Use using or await using for Bun APIs (e.g., Bun.spawn, Bun.listen, Bun.connect, Bun.serve) to ensure cleanup
Never hardcode port numbers; use port: 0 to get a random port
Use Buffer.alloc(count, fill).toString() instead of "A".repeat(count) for large/repetitive strings
Import testing utilities from harness (bunExe, bunEnv, tempDirWithFiles, tmpdirSync, isMacOS, isWindows, isPosix, gcTick, withoutAggressiveGC)

Files:

• test/js/sql/adapter-env-var-precedence.test.ts

test/**

📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)

Place all tests under the test/ directory

Files:

• test/js/sql/adapter-env-var-precedence.test.ts

test/js/**/*.{js,ts}

📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)

Place JavaScript and TypeScript tests under test/js/

Files:

• test/js/sql/adapter-env-var-precedence.test.ts

test/**/*.{js,ts}

📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)

test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable

Files:

• test/js/sql/adapter-env-var-precedence.test.ts

🧬 Code graph analysis (1)

test/js/sql/adapter-env-var-precedence.test.ts (1)

src/js/internal-for-testing.ts (1)

• SQL (17-17)

🔇 Additional comments (11)

src/js/internal/sql/shared.ts (1)

23-27: Verify consistency with constructor initialization.

The removal of affectedRows from the property definitions is consistent - both the public property declaration and its initialization in Object.defineProperties have been removed. This maintains internal consistency.

test/js/sql/adapter-env-var-precedence.test.ts (10)

1-3: LGTM: Proper test setup with required imports.

The imports correctly use bun:test as specified in the coding guidelines for test files ending with .test.ts, and import the SQL class from the main bun module.

---

4-36: LGTM: Well-structured test environment management.

The test setup properly handles environment variable isolation with snapshot/restore functionality. The cleanEnv() function comprehensively removes all SQL-related environment variables, and restoreEnv() ensures proper cleanup.

---

37-52: LGTM: Correctly validates explicit options precedence.

This test properly validates that explicit options take precedence over environment variables, which addresses the issue mentioned in the test name (#22147). The test correctly verifies that DATABASE_URL doesn't override explicitly provided connection parameters.

---

347-354: LGTM: Comprehensive adapter-protocol validation.

This test correctly validates the adapter-protocol mismatch error handling. The error message pattern matches the expected validation from the parseOptions function in shared.ts.

---

54-71: LGTM: Proper adapter-specific environment variable handling.

These tests correctly validate that:

1. PostgreSQL environment variables (PGHOST, PGUSER, etc.) are only used when the adapter is postgres
2. MySQL-specific environment variables take precedence when the adapter is mysql
3. Adapter-specific variables have priority over generic ones

The test logic properly isolates environment variables and verifies the expected behavior.

Also applies to: 73-87, 129-140

---

223-243: LGTM: Environment variable name-based adapter inference.

These tests correctly validate that environment variable names can override protocol-based adapter inference (e.g., PGURL with mysql:// protocol should result in postgres adapter). This is an important edge case for configuration flexibility.

---

401-545: LGTM: Comprehensive explicit options precedence testing.

This test section thoroughly validates that explicit options override URL parameters, including:

• Individual parameter overrides (hostname, port, username, password, database)
• Multiple simultaneous overrides
• Alternative option names (host, user, pass, db)
• Precedence over both URL parameters and environment variables

The test coverage is comprehensive and follows the expected precedence hierarchy.

---

164-175: LGTM: Correct MySQL default port handling.

This test properly validates that MySQL adapter defaults to port 3306 when no port is explicitly specified, which matches the expected behavior in the parseOptions function.

---

281-288: LGTM: Correct PostgreSQL database naming defaults.

This test properly validates that PostgreSQL uses the username as the default database name when no database is specified, which aligns with standard PostgreSQL conventions.

---

290-300: SSLMode.require value validated The SSLMode enum in src/js/internal/sql/shared.ts defines require = 2, so the test’s expect(options.options.sslMode).toBe(2) correctly reflects SSLMode.require.

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/js/internal/sql/shared.ts (1)

475-489: Bug: unix socket path logic fails when port comes from URL (string).

Number.isSafeInteger(port) is false when port is a string (e.g., from url.port), so /.s.PGSQL.${port} is never appended. This regresses socket discovery.

Apply:

   if (adapter === "postgres") {
     // add /.s.PGSQL.${port} if the unix domain socket is listening on that path
-    if (path && Number.isSafeInteger(port) && path?.indexOf("/.s.PGSQL.") === -1) {
-      const pathWithSocket = `${path}/.s.PGSQL.${port}`;
+    if (path && path.indexOf("/.s.PGSQL.") === -1) {
+      const portNum = Number(port ?? 5432);
+      if (Number.isSafeInteger(portNum)) {
+        const pathWithSocket = `${path}/.s.PGSQL.${portNum}`;
         // Only add the path if it actually exists. It would be better to just
         // always respect whatever the user passes in, but that would technically
         // be a breakpoint change at this point.
-      if (require("node:fs").existsSync(pathWithSocket)) {
-        path = pathWithSocket;
-      }
+        if (require("node:fs").existsSync(pathWithSocket)) {
+          path = pathWithSocket;
+        }
+      }
     }
   }

🧹 Nitpick comments (6)

src/js/node/fs.ts (1)

536-542: Accept PathLike in existsSync signature
Change the wrapper’s _path: string to _path: PathLike and add import type { PathLike } from 'fs'; to match Node.js’s API.

test/integration/bun-types/fixture/sql.ts (1)

277-279: Nit: fix typo in comment.

“exits” → “exists”.

-expectType(aSqlInstance.options.safeIntegers).is<boolean | undefined>(); // property exits in sqlite options
+expectType(aSqlInstance.options.safeIntegers).is<boolean | undefined>(); // property exists in sqlite options

src/js/internal/sql/shared.ts (3)

11-28: Removal of SQLResultArray.affectedRows may break consumers.

Dropping affectedRows is a breaking change for MySQL/MariaDB users who relied on it. If intentional, document in CHANGELOG and ensure typings/tests reflect it; otherwise, consider keeping it as a non-enumerable deprecated field.

 Object.defineProperties(this, {
   count: { value: null, writable: true },
   command: { value: null, writable: true },
   lastInsertRowid: { value: null, writable: true },
+  // Keep for back-compat; write-only by adapters if applicable
+  affectedRows: { value: undefined, writable: true },
 });

---

239-286: Env precedence cleanup and missing alias.

• env.PGURL is listed twice.
• Consider supporting POSTGRESQL_URL.

-      env.POSTGRES_URL ||
-      env.DATABASE_URL ||
-      env.PGURL ||
-      env.PG_URL ||
-      env.PGURL ||
+      env.POSTGRES_URL ||
+      env.POSTGRESQL_URL ||
+      env.PG_URL ||
+      env.PGURL ||
+      env.DATABASE_URL ||
       env.MYSQL_URL ||
       env.SQLITE_URL ||

And in the postgres branch:

-  url = env.POSTGRES_URL || env.DATABASE_URL || env.PG_URL || env.PGURL || env.PG_URL || null;
+  url = env.POSTGRES_URL || env.POSTGRESQL_URL || env.DATABASE_URL || env.PG_URL || env.PGURL || null;

---

350-369: Confirm protocol mapping for http/https/ftp → postgres.

This is surprising; verify it matches product requirements and tests. If it’s for cloud proxies, add a brief comment for future readers.

-    case "http":
-    case "https":
-    case "ftp":
+    case "http": // e.g. cloud/sql proxy URLs treated as postgres
+    case "https":
+    case "ftp":

src/js/private.d.ts (1)

31-36: Nit: duplicate "max" in key union.

Tiny cleanup only; no behavior change.

-type DefinedPostgresOrMySQLOptions = Define<Bun.SQL.PostgresOrMySQLOptions, "max" | "prepare" | "max"> & {
+type DefinedPostgresOrMySQLOptions = Define<Bun.SQL.PostgresOrMySQLOptions, "max" | "prepare"> & {

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between a98c72e and c99dfab.

📒 Files selected for processing (5)

• packages/bun-types/sql.d.ts (2 hunks)
• src/js/internal/sql/shared.ts (10 hunks)
• src/js/node/fs.ts (1 hunks)
• src/js/private.d.ts (2 hunks)
• test/integration/bun-types/fixture/sql.ts (1 hunks)

🧰 Additional context used

📓 Path-based instructions (5)

**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (CLAUDE.md)

Format JavaScript/TypeScript with Prettier (bun run prettier)

Files:

• src/js/node/fs.ts
• test/integration/bun-types/fixture/sql.ts
• src/js/private.d.ts
• packages/bun-types/sql.d.ts
• src/js/internal/sql/shared.ts

src/js/**/*.{ts,js}

📄 CodeRabbit inference engine (CLAUDE.md)

src/js/**/*.{ts,js}: In built-in modules, require() must use string literals resolved at compile time
In built-in modules, use export default (converted to a return statement by the preprocessor)

Files:

• src/js/node/fs.ts
• src/js/private.d.ts
• src/js/internal/sql/shared.ts

test/integration/**

📄 CodeRabbit inference engine (test/CLAUDE.md)

Place integration tests under /test/integration/

Files:

• test/integration/bun-types/fixture/sql.ts

test/**

📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)

Place all tests under the test/ directory

Files:

• test/integration/bun-types/fixture/sql.ts

test/**/*.{js,ts}

📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)

test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable

Files:

• test/integration/bun-types/fixture/sql.ts

🧬 Code graph analysis (3)

test/integration/bun-types/fixture/sql.ts (1)

test/integration/bun-types/fixture/utilities.ts (1)

• expectType (35-37)

packages/bun-types/sql.d.ts (1)

src/js/internal-for-testing.ts (1)

• SQL (17-17)

src/js/internal/sql/shared.ts (2)

src/js/internal/sql/postgres.ts (1)

• adapter (452-461)

src/js/internal/sql/mysql.ts (1)

• adapter (435-444)

🔇 Additional comments (6)

packages/bun-types/sql.d.ts (2)

341-349: Good public alias for merged options.

SQL.MergedOptions improves readability and external consumption over the internal DistributedMerge type.

---

536-536: LGTM: options now typed as SQL.MergedOptions.

Matches new ambient type checks in tests.

src/js/internal/sql/shared.ts (3)

597-604: TLS serverName defaulting looks good.

Auto-filling serverName when sslmode isn’t disable matches common PG/MySQL client expectations.

---

155-160: All call sites updated for new parameter order. No further instances found.

---

684-686: No action needed: all adapters implement isConnected() and closed.
PostgresAdapter and MySQLAdapter both define a public closed: boolean, SQLiteAdapter has get closed(): boolean, and each adapter defines isConnected().

src/js/private.d.ts (1)

15-20: Good: Make Define distributive over unions.

This fixes several union-typing edge cases.

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/js/internal/sql/shared.ts (1)

146-153: SQLite query params (e.g. ?mode=ro) are dropped — functional bug

parseDefinitelySqliteUrl() strips query strings, and parseConnectionDetailsFromOptionsOrEnvironment() stores the stripped path in url. Then parseOptions() passes that value into parseSQLiteOptionsWithQueryParams(), so SQLite mode is never observed for string URLs like sqlite:file.db?mode=ro.

Fix: preserve the original string/URL (with query) for option parsing, and compute filename separately via parseDefinitelySqliteUrl().

@@
-    if (definitelySqliteUrl) {
-      protocol = "sqlite";
-      url = definitelySqliteUrl;
-    }
+    if (definitelySqliteUrl) {
+      protocol = "sqlite";
+      // keep original for query parsing; derive filename later
+      url = stringOrUrl!;
+    }
@@
-  if (options.adapter === "sqlite") {
-    return parseSQLiteOptionsWithQueryParams(urlFromConnectionDetails, {
-      ...options,
-      adapter: "sqlite",
-      filename: urlFromConnectionDetails ?? ":memory:",
-    });
-  }
+  if (options.adapter === "sqlite") {
+    const filename =
+      parseDefinitelySqliteUrl(urlFromConnectionDetails) ??
+      (typeof urlFromConnectionDetails === "string" ? urlFromConnectionDetails : null) ??
+      ":memory:";
+    return parseSQLiteOptionsWithQueryParams(urlFromConnectionDetails, {
+      ...options,
+      adapter: "sqlite",
+      filename,
+    });
+  }

Also applies to: 335-341, 383-389

🧹 Nitpick comments (4)

src/js/internal/sql/shared.ts (4)

11-28: Public API removal: consider a compatibility shim for affectedRows

SQLResultArray no longer exposes affectedRows. If external code relies on it, this is a breaking change. Consider a non-enumerable, deprecated getter that aliases to count to ease migration.

 class SQLResultArray<T> extends PublicArray<T> {
   public count!: number | null;
   public command!: string | null;
   public lastInsertRowid!: number | bigint | null;
+  /** @deprecated Use `count` instead. */
+  public get affectedRows(): number | null {
+    return this.count;
+  }

---

239-287: Env resolution gaps and duplicates (PGURL repeated; no MariaDB URLs)

• PGURL appears twice; drop the duplicate.
• No MariaDB URL support. Add MARIADB_URL and TLS_MARIADB_DATABASE_URL (and a case "mariadb") to mirror MySQL.
• Consider including MYSQL_PWD as a password fallback for MySQL/MariaDB later in parseOptions (nice-to-have).

-      env.PGURL ||
-      env.PG_URL ||
-      env.PGURL ||
-      env.MYSQL_URL ||
+      env.PG_URL ||
+      env.PGURL ||
+      env.MYSQL_URL ||
+      env.MARIADB_URL ||
       env.SQLITE_URL ||
@@
-    case "postgres":
-      url = env.POSTGRES_URL || env.DATABASE_URL || env.PG_URL || env.PGURL || env.PG_URL || null;
+    case "postgres":
+      url = env.POSTGRES_URL || env.DATABASE_URL || env.PG_URL || env.PGURL || null;
@@
     case "mysql":
       url = env.MYSQL_URL || env.DATABASE_URL || null;
@@
       return [url, sslMode];
+
+    case "mariadb":
+      url = env.MARIADB_URL || env.DATABASE_URL || null;
+      if (!url) {
+        url = env.TLS_MARIADB_DATABASE_URL || env.TLS_DATABASE_URL || null;
+        if (url) sslMode = SSLMode.require;
+      }
+      return [url, sslMode];

---

424-436: Prefer standards-based params extraction over toJSON()

URLSearchParams#toJSON() isn’t standard across runtimes. Use Object.fromEntries(url.searchParams).

-    const queryObject = url.searchParams.toJSON();
+    const queryObject = Object.fromEntries(url.searchParams);

---

288-295: Docstring typos and clarity

Minor cleanup for readability.

- * url string, that you should continue to use for further options. In some
- * cases the it will be a parsed URL instance, and in others a string. This is
- * to save unnecessary parses in some caes. The third value is the SSL mode The last value is the options object
- * resolved from the possible overloads of the Bun.SQL constructor, it may have modifications
+ * url string to use for further options. In some cases it will be a parsed URL instance,
+ * in others a string (to avoid unnecessary parses). The third value is the SSL mode.
+ * The last value is the options object resolved from the possible Bun.SQL constructor overloads.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between c99dfab and 562732d.

📒 Files selected for processing (1)

• src/js/internal/sql/shared.ts (10 hunks)

🧰 Additional context used

📓 Path-based instructions (2)

**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (CLAUDE.md)

Format JavaScript/TypeScript with Prettier (bun run prettier)

Files:

• src/js/internal/sql/shared.ts

src/js/**/*.{ts,js}

📄 CodeRabbit inference engine (CLAUDE.md)

src/js/**/*.{ts,js}: In built-in modules, require() must use string literals resolved at compile time
In built-in modules, use export default (converted to a return statement by the preprocessor)

Files:

• src/js/internal/sql/shared.ts

🧬 Code graph analysis (1)

src/js/internal/sql/shared.ts (2)

src/js/internal/sql/postgres.ts (1)

• adapter (452-461)

src/js/internal/sql/mysql.ts (1)

• adapter (435-444)

🔇 Additional comments (3)

src/js/internal/sql/shared.ts (3)

304-313: Confirm options precedence when two options objects are provided

Merging with { ...stringOrUrlOrOptions, ...definitelyOptionsButMaybeEmpty } gives precedence to the second parameter. If that’s the intended “options override” (constructor new SQL(urlOrOpts, opts)), great—otherwise swap the spread order.

---

443-477: Host/port defaults per adapter — LGTM

Fallbacks look correct and align with typical defaults and env names.

---

601-607: TLS serverName inference — LGTM

Auto-populating serverName from hostname when sslmode isn’t disabled is a good default and backwards-compatible.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (3)

src/js/internal/sql/postgres.ts (3)

318-324: Async password providers supported — add a lightweight runtime guard

If a provider resolves to a non-string, Zig will still get coerced via || "", but being explicit avoids surprises.

Apply:

       if (typeof password === "function") {
         password = password();
       }
 
-      if (password && $isPromise(password)) {
+      if (password && $isPromise(password)) {
         password = await password;
       }
+      if (password != null && typeof password !== "string") {
+        password = String(password);
+      }

---

851-902: close() return type clarified; tighten timeout validation to ms limit

The Promise typing and withResolvers() are correct. One small edge case: setTimeout takes a 32-bit signed millisecond delay. Validating seconds against 2^31 can still overflow after multiplying by 1000.

Apply:

-      timeout = Number(timeout);
-      if (timeout > 2 ** 31 || timeout < 0 || timeout !== timeout) {
-        throw $ERR_INVALID_ARG_VALUE("options.timeout", timeout, "must be a non-negative integer less than 2^31");
-      }
+      timeout = Number(timeout);
+      const maxSeconds = Math.floor(((2 ** 31) - 1) / 1000);
+      if (timeout > maxSeconds || timeout < 0 || timeout !== timeout) {
+        throw $ERR_INVALID_ARG_VALUE(
+          "options.timeout",
+          timeout,
+          `must be a non-negative number ≤ ${maxSeconds} seconds`
+        );
+      }

Optional follow-up: confirm call sites now treat close() as async (await or handle the returned promise) where a timeout is passed.

---

123-131: Nit: parameter name typo “databae”

The PostgresDotZig.createConnection interface spells “database” as “databae”. Harmless but distracting; consider fixing for readability.

-    databae: string,
+    database: string,

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 4372d30 and c3129d3.

📒 Files selected for processing (3)

• src/bun.js/api/bun/socket/Listener.zig (1 hunks)
• src/js/internal/sql/mysql.ts (1 hunks)
• src/js/internal/sql/postgres.ts (9 hunks)

🚧 Files skipped from review as they are similar to previous changes (2)

• src/bun.js/api/bun/socket/Listener.zig
• src/js/internal/sql/mysql.ts

🧰 Additional context used

📓 Path-based instructions (2)

src/js/**/*.ts

📄 CodeRabbit inference engine (CLAUDE.md)

src/js/**/*.ts: In built-in modules under src/js, require() must use string literals (resolved at compile time)
In built-in modules, use export default to export the module object (converted to a return statement by the preprocessor)
Use special $-prefixed intrinsics and helpers (e.g., $Array, $newArrayWithSize, $debug, $assert) only within built-in modules
Use process.platform and process.arch for platform detection in built-in modules to enable dead-code elimination

Files:

• src/js/internal/sql/postgres.ts

**/*.{ts,tsx,js}

📄 CodeRabbit inference engine (CLAUDE.md)

Run Prettier to format JS/TS files (bun run prettier)

Files:

• src/js/internal/sql/postgres.ts

🧠 Learnings (1)

📚 Learning: 2025-09-03T17:10:13.486Z

Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : Prefer `async/await` in tests; for a single callback, use `Promise.withResolvers()`

Applied to files:

• src/js/internal/sql/postgres.ts

🔇 Additional comments (5)

src/js/internal/sql/postgres.ts (5)

296-314: Type widening for createConnection options looks sane

Using Bun.SQL.__internal.DefinedPostgresOrMySQLOptions here is consistent; the destructured fields are in the shared surface. No functional concerns.

---

359-361: connectionInfo type broadened

Widening to the unified options type is fine and aligns with the adapter’s usage.

---

426-431: Constructor signature updated

Accepting the unified options type here matches the internal field and call sites in this file.

---

510-511: Public connectionInfo type updated

Public surface matches internals; no issues.

---

523-527: Adapter constructor accepts unified options

Looks good; note that code depends on connectionInfo.max being defined. If callers omit it, pool size becomes 1 due to new Array(undefined) semantics—likely acceptable, just calling it out.

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)

test/js/sql/sql-mysql.test.ts (3)

55-55: Leak: top-level SQL instance isn’t closed — likely contributor to sporadic segfaults.

This instance lives for the whole describe block with no teardown. Ensure it’s ended to avoid dangling sockets/pool state between tests.

Apply this diff right after the declaration:

 const sql = new SQL(options);
+afterAll(async () => {
+  await sql.end();
+});

---

171-178: Avoid while(true) in tests; wait for max_lifetime close event instead.

Busy-looping risks hangs/flakes if the close is delayed; rely on onClosePromise to deterministically await closure.

Apply:

-  try {
-    while (true) {
-      for (let i = 0; i < 100; i++) {
-        await sql`select SLEEP(1)`;
-      }
-    }
-  } catch (e) {
-    error = e;
-  }
+  // Wait for the pool to enforce max_lifetime and close the connection.
+  const err = await onClosePromise.promise;
+  error = err;

---

897-900: Missing assertion: test returns a tuple instead of asserting result.

This test won’t fail if behavior regresses.

-        test("Array returns rows as arrays of columns", async () => {
-          await using sql = new SQL(options);
-          return [(await sql`select CAST(1 AS SIGNED) as x`.values())[0][0], 1];
-        });
+        test("Array returns rows as arrays of columns", async () => {
+          await using sql = new SQL(options);
+          expect((await sql`select CAST(1 AS SIGNED) as x`.values())[0][0]).toBe(1);
+        });

🧹 Nitpick comments (3)

test/js/sql/sql-mysql.test.ts (3)

91-112: Make error typing consistent with newer tests (unknown + instanceof).

This block still uses any and direct property access; align with the stronger pattern used elsewhere.

-          let error: any;
+          let error: unknown;
@@
-          expect(error.code).toBe(`ERR_MYSQL_CONNECTION_TIMEOUT`);
-          expect(error.message).toContain("Connection timeout after 4s");
+          expect(error).toBeInstanceOf(SQL.SQLError);
+          expect(error).toBeInstanceOf(SQL.MySQLError);
+          expect((error as SQL.MySQLError).code).toBe(`ERR_MYSQL_CONNECTION_TIMEOUT`);
+          expect((error as SQL.MySQLError).message).toContain("Connection timeout after 4s");

---

499-505: Remove arbitrary sleep; wait for idle close event to avoid flakiness.

Use onclose + Promise.withResolvers instead of Bun.sleep(1100).

-        test("Idle timeout retry works", async () => {
-          await using sql = new SQL({ ...options, idleTimeout: 1 });
-          await sql`select 1`;
-          await Bun.sleep(1100); // 1.1 seconds so it should retry
-          await sql`select 1`;
-          expect().pass();
-        });
+        test("Idle timeout retry works", async () => {
+          const onClosePromise = Promise.withResolvers();
+          const onclose = mock(err => onClosePromise.resolve(err));
+          await using sql = new SQL({ ...options, idleTimeout: 1, onclose });
+          await sql`select 1`;
+          await onClosePromise.promise; // deterministically wait for idle close
+          await sql`select 1`; // should transparently reconnect
+          expect().pass();
+        });

---

499-505: Minor naming consistency: choose one of idleTimeout vs idle_timeout across the file.

Both are supported; using one consistently improves readability.

Also applies to: 55-55

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between c3129d3 and 32a6a90.

📒 Files selected for processing (3)

• packages/bun-types/sql.d.ts (4 hunks)
• test/js/sql/adapter-env-var-precedence.test.ts (1 hunks)
• test/js/sql/sql-mysql.test.ts (4 hunks)

🚧 Files skipped from review as they are similar to previous changes (2)

• test/js/sql/adapter-env-var-precedence.test.ts
• packages/bun-types/sql.d.ts

🧰 Additional context used

📓 Path-based instructions (7)

test/**

📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)

Place all tests under the test/ directory

Files:

• test/js/sql/sql-mysql.test.ts

test/js/**/*.{js,ts}

📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)

Place JavaScript and TypeScript tests under test/js/

Files:

• test/js/sql/sql-mysql.test.ts

test/**/*.{js,ts}

📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)

test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable

Files:

• test/js/sql/sql-mysql.test.ts

test/**/*.test.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

test/**/*.test.{ts,tsx}: Tests must be named with the suffix .test.ts or .test.tsx and live under the test/ directory
In tests, always use port: 0 and never hardcode ports or use custom random-port utilities
Prefer snapshot tests using normalizeBunSnapshot(...) over direct string equality on stdout/stderr
Do not write tests that assert absence of 'panic', 'uncaught exception', or similar strings in output
Avoid shelling out to tools like find or grep in tests; use Bun.Glob and built-in utilities

Files:

• test/js/sql/sql-mysql.test.ts

**/*.{ts,tsx,js}

📄 CodeRabbit inference engine (CLAUDE.md)

Run Prettier to format JS/TS files (bun run prettier)

Files:

• test/js/sql/sql-mysql.test.ts

test/**/*.test.ts

📄 CodeRabbit inference engine (test/CLAUDE.md)

test/**/*.test.ts: Name test files *.test.ts and use bun:test
Do not write flaky tests: never wait for arbitrary time; wait for conditions instead
Never hardcode port numbers in tests; use port: 0 to get a random port
When spawning Bun in tests, use bunExe() and bunEnv from harness
Prefer async/await in tests; for a single callback, use Promise.withResolvers()
Do not set explicit test timeouts; rely on Bun’s built-in timeouts
Use tempDir/tempDirWithFiles from harness for temporary files and directories in tests
For large/repetitive strings in tests, prefer Buffer.alloc(count, fill).toString() over "A".repeat(count)
Import common test utilities from harness (e.g., bunExe, bunEnv, tempDirWithFiles, tmpdirSync, platform checks, GC helpers)
In error tests, assert non-zero exit codes for failing processes and use toThrow for synchronous errors
Use describe blocks for grouping, describe.each for parameterized tests, snapshots with toMatchSnapshot, and lifecycle hooks (beforeAll, beforeEach, afterEach); track resources for cleanup in afterEach
Use using/await using with Bun resources (e.g., Bun.listen/connect/spawn/serve) to ensure cleanup in tests

Files:

• test/js/sql/sql-mysql.test.ts

test/js/**

📄 CodeRabbit inference engine (test/CLAUDE.md)

Organize unit tests for specific features under test/js/ by module

Files:

• test/js/sql/sql-mysql.test.ts

🧬 Code graph analysis (1)

test/js/sql/sql-mysql.test.ts (2)

src/js/internal/sql/mysql.ts (2)

• err (347-376)
• err (378-407)

src/js/internal-for-testing.ts (1)

• SQL (17-17)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: Format

🔇 Additional comments (4)

test/js/sql/sql-mysql.test.ts (4)

47-55: Typed Options init looks correct (incl. TLS Bun.file).

Explicit Bun.SQL.Options and conditional CA file use are good.

---

146-146: Compile-time shape check on result: LGTM.

Using expect(…) enforces row typing without runtime cost.

---

150-153: Runtime type checks on error: LGTM.

Both base and adapter-specific instanceof checks + code read are correct.

---

167-169: Switch to unknown + typed expect: LGTM.

Good move toward safer error typing.

[coderabbitai]

Walkthrough

Optionalized many Postgres/MySQL error metadata and restructured error option interfaces; unified Postgres/MySQL option shapes (mariadb alias for mysql); overhauled URL/env parsing and SQLite in-memory handling; added TLS/BunFile support and docs deprecations; multiple test additions/updates and typing tweaks across SQL internals.

Changes

Cohort / File(s)	Summary
Type declarations: SQL errors & options packages/bun-types/sql.d.ts	Made many PostgresError fields and MySQLError.errno/sqlState optional; updated constructors/options to reflect optional fields; marked host and ssl docs deprecated in favor of hostname/tls; tls/ssl unions now accept Bun.BunFile.
Internal error classes / options src/js/internal/sql/errors.ts	Expanded PostgresErrorOptions with optional metadata; added SQLiteErrorOptions.byteOffset; made MySQL errno/sqlState optional via declaration merging; constructors adjusted and lint suppressions added.
Private/internal SQL types src/js/private.d.ts	Made Define distributive over unions; renamed DefinedPostgresOptions → DefinedPostgresOrMySQLOptions (based on PostgresOrMySQLOptions); removed DefinedMySQLOptions; added OptionsWithDefinedAdapter.
Adapter aliasing src/js/bun/sql.ts	adapterFromOptions treats "mariadb" as an alias for "mysql".
MySQL internals & public typings src/js/internal/sql/mysql.ts	tls accepts Bun.BunFile; broadened options types to DefinedPostgresOrMySQLOptions; replaced MySQLSQLConnection/MySQLSQLQuery with MySQLConnection/MySQLQuery; updated Promise.withResolvers usage; default export now includes SQLCommand; removed ts-expect-error for path.
Postgres internals & public typings src/js/internal/sql/postgres.ts	tls accepts Bun.BunFile; internal/public types use DefinedPostgresOrMySQLOptions; PostgresAdapter constructor/property signatures updated; close now explicitly returns Promise<void>; Promise.withResolvers() usage.
Shared SQL parsing, env/url resolution, SQLite src/js/internal/sql/shared.ts	Large refactor: protocol-based adapter inference, DEFAULT_PROTOCOL, env resolvers, ensureUrlHasProtocol, parseConnectionDetailsFromOptionsOrEnvironment; sqlite memory variants normalized to ":memory:"; parseSQLiteOptions added; normalized SSL mode error messaging; added SQLResultArray[Symbol.toStringTag]; adjusted DatabaseAdapter.release signature; exposed SSLMode on default export.
Node fs typing src/js/node/fs.ts	existsSync annotated as existsSync(_path: string) (type-only change).
Zig listener whitespace (no-op) src/bun.js/api/bun/socket/Listener.zig	Single blank-line insertion; no behavioral change.
Docs: TLS example & deprecations docs/api/sql.md	Replaced ssl example with tls object in MySQL docs; documented deprecations of host→hostname and ssl→tls.
Type tests & fixtures test/integration/bun-types/fixture/sql.ts	Added type-level assertions for Bun.SQL options (host, safeIntegers).
Env-var precedence tests (new) test/js/sql/adapter-env-var-precedence.test.ts	New suite testing env-var precedence, protocol inference, TLS env/url handling; adds TS augmentation for sslMode on PostgresOrMySQLOptions.
Options.url precedence & MariaDB tests test/js/sql/sqlite-sql.test.ts	Tests asserting options.url overrides first-arg URL; added mariadb expectations; sqlite empty-path now maps to ":memory:"; removed some env-based tests.
SQLite URL parsing tests test/js/sql/sqlite-url-parsing.test.ts	Updated expectations: empty path → ":memory:" across URL variants.
MySQL tests: typing & error checks test/js/sql/sql-mysql.test.ts	Stronger typings in MySQL tests: explicit Bun.SQL.Options, use unknown for caught errors, instance checks and casts for SQL.MySQLError, stricter result typing.
Test instrumentation & gating changes test/js/third_party/next-auth/next-auth.test.ts, test/js/third_party/pg-gateway/pglite.test.ts	Added logging/instrumentation in next-auth test; ungated pglite test and added configuration assertions.
Regression tests: resource cleanup test/regression/issue/21311.test.ts	Replaced manual sql.end() usage with await using sql = ... for automatic disposal.

Possibly related PRs

• fix(Bun.SQL) test failure #22438 — Modifies SQL connection close and pooled connection paths in src/js/internal/sql/mysql.ts and src/js/internal/sql/postgres.ts, overlapping connection lifecycle changes.

Suggested reviewers

• Jarred-Sumner
• cirospaciari
• zackradisic

✨ Finishing Touches

• 📝 Generate Docstrings

🧪 Generate unit tests

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch ali/fix-sql-connection-string-parsing

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

test/harness.ts (2)

11-11: Unify child_process imports; prefer node: specifier and make ChildProcess a type-only import

Avoid mixed module specifiers and reduce runtime bindings.

-import { ChildProcess, execSync, fork } from "child_process";
+import type { ChildProcess } from "node:child_process";
+import { execSync, fork } from "node:child_process";

---

33-35: Fix glibc detection typing and harden against missing report

Use the correct string shape and optional chaining instead of asserting boolean.

-      (process.report.getReport() as { header: { glibcVersionRuntime: boolean } }).header.glibcVersionRuntime
+      ((process.report as any)?.getReport?.() as { header?: { glibcVersionRuntime?: string } } | undefined)?.header?.glibcVersionRuntime

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 32a6a90 and ff91fc1.

📒 Files selected for processing (1)

• test/harness.ts (2 hunks)

🧰 Additional context used

📓 Path-based instructions (3)

test/**

📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)

Place all tests under the test/ directory

Files:

• test/harness.ts

test/**/*.{js,ts}

📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)

test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable

Files:

• test/harness.ts

**/*.{js,ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

Format JavaScript/TypeScript files with Prettier (bun run prettier)

Files:

• test/harness.ts

[coderabbitai]

Actionable comments posted: 1

♻️ Duplicate comments (2)

src/js/internal/sql/errors.ts (2)

82-101: Fix lint: remove unsafe declaration merging for SQLiteError; declare class field instead

This triggers Biome/typescript-eslint no-unsafe-declaration-merging and blocks CI. Declare the optional field on the class and drop the merged interface.

-// oxlint-disable-next-line typescript-eslint(no-unsafe-declaration-merging)
-interface SQLiteError {
-  byteOffset?: number | undefined;
-}
-
 class SQLiteError extends SQLError implements Bun.SQL.SQLiteError {
   public readonly code: string;
   public readonly errno: number;
+  // Avoid unsafe declaration merging; preserve runtime shape (no initializer emitted)
+  declare readonly byteOffset?: number | undefined;
 
   constructor(message: string, options: SQLiteErrorOptions) {

---

109-126: Fix lint: remove unsafe declaration merging for MySQLError; declare fields on class

Same issue as SQLiteError. Declare optional fields on the class and delete the merged interface.

-// oxlint-disable-next-line typescript-eslint(no-unsafe-declaration-merging)
-interface MySQLError {
-  errno?: number | undefined;
-  sqlState?: string | undefined;
-}
+// (removed unsafe declaration merging)
 
 class MySQLError extends SQLError implements Bun.SQL.MySQLError {
   public readonly code: string;
+  // Avoid unsafe declaration merging; no emitted initializers
+  declare readonly errno?: number | undefined;
+  declare readonly sqlState?: string | undefined;
 
   constructor(message: string, options: MySQLErrorOptions) {

🧹 Nitpick comments (3)

src/js/internal/sql/errors.ts (1)

28-46: Optional: align PostgresError with the same no-merge pattern

For consistency and to avoid future suppressions, declare fields on the class and drop the merged interface.

-// oxlint-disable-next-line typescript-eslint(no-unsafe-declaration-merging)
-interface PostgresError {
-  detail?: string | undefined;
-  hint?: string | undefined;
-  severity?: string | undefined;
-  errno?: string | undefined;
-  position?: string | undefined;
-  internalPosition?: string | undefined;
-  internalQuery?: string | undefined;
-  where?: string | undefined;
-  schema?: string | undefined;
-  table?: string | undefined;
-  column?: string | undefined;
-  dataType?: string | undefined;
-  constraint?: string | undefined;
-  file?: string | undefined;
-  line?: string | undefined;
-  routine?: string | undefined;
-}
+// (removed unsafe declaration merging)
 
 class PostgresError extends SQLError implements Bun.SQL.PostgresError {
   public readonly code: string;
+  // Optional properties populated from options
+  declare readonly detail?: string | undefined;
+  declare readonly hint?: string | undefined;
+  declare readonly severity?: string | undefined;
+  declare readonly errno?: string | undefined;
+  declare readonly position?: string | undefined;
+  declare readonly internalPosition?: string | undefined;
+  declare readonly internalQuery?: string | undefined;
+  declare readonly where?: string | undefined;
+  declare readonly schema?: string | undefined;
+  declare readonly table?: string | undefined;
+  declare readonly column?: string | undefined;
+  declare readonly dataType?: string | undefined;
+  declare readonly constraint?: string | undefined;
+  declare readonly file?: string | undefined;
+  declare readonly line?: string | undefined;
+  declare readonly routine?: string | undefined;

Also applies to: 49-56

src/js/internal/sql/mysql.ts (2)

105-116: Typo: databae → database in createConnection signature

Purely cosmetic but confusing.

-    databae: string,
+    database: string,

---

409-413: Unify constructor type with the rest of the file

Everything else uses DefinedPostgresOrMySQLOptions; constructor still uses DefinedMySQLOptions.

-  constructor(connectionInfo: Bun.SQL.__internal.DefinedMySQLOptions, adapter: MySQLAdapter) {
+  constructor(connectionInfo: Bun.SQL.__internal.DefinedPostgresOrMySQLOptions, adapter: MySQLAdapter) {

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between ff91fc1 and db15759.

📒 Files selected for processing (3)

• src/js/internal/sql/errors.ts (4 hunks)
• src/js/internal/sql/mysql.ts (10 hunks)
• src/js/internal/sql/postgres.ts (10 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• src/js/internal/sql/postgres.ts

🧰 Additional context used

📓 Path-based instructions (4)

src/js/internal/**/*.{js,ts}

📄 CodeRabbit inference engine (src/js/CLAUDE.md)

Place internal-only modules under internal/

Files:

• src/js/internal/sql/mysql.ts
• src/js/internal/sql/errors.ts

src/js/{builtins,node,bun,thirdparty,internal}/**/*.{js,ts}

📄 CodeRabbit inference engine (src/js/CLAUDE.md)

src/js/{builtins,node,bun,thirdparty,internal}/**/*.{js,ts}: Use .$call and .$apply instead of .call or .apply to avoid user tampering
Use string-literal require("...") only (no dynamic or non-literal specifiers)
Author modules as CommonJS-style with require(...) and export via export default {} (no ESM import/named exports)
Prefer JSC intrinsics and $-prefixed private APIs for performance and safety (e.g., $Array, $newArrayWithSize, $putByIdDirectPrivate, $assert, $debug)
Validate function arguments with $isCallable and throw $ERR_INVALID_ARG_TYPE for invalid callbacks
Use $isObject and throw appropriate TypeErrors for constructor/initializer inputs that must be objects

Files:

• src/js/internal/sql/mysql.ts
• src/js/internal/sql/errors.ts

src/js/**/*.{js,ts}

📄 CodeRabbit inference engine (CLAUDE.md)

src/js/**/*.{js,ts}: In built-in modules (src/js), require() must use string literals resolved at compile time
In built-in modules (src/js), use export default (converted to a return statement by the preprocessor)

Files:

• src/js/internal/sql/mysql.ts
• src/js/internal/sql/errors.ts

**/*.{js,ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

Format JavaScript/TypeScript files with Prettier (bun run prettier)

Files:

• src/js/internal/sql/mysql.ts
• src/js/internal/sql/errors.ts

🧬 Code graph analysis (2)

src/js/internal/sql/mysql.ts (1)

src/js/internal/sql/postgres.ts (4)

• err (364-393)
• err (395-424)
• connection (433-439)
• onClose (441-443)

src/js/internal/sql/errors.ts (1)

packages/bun-types/sqlite.d.ts (1)

• SQLiteError (1273-1303)

🪛 Biome (2.1.2)

src/js/internal/sql/errors.ts

[error] 87-87: This class is unsafely merged with an interface.

The interface is declared here.

The TypeScript compiler doesn't check whether properties defined in the interface are initialized in the class.

(lint/suspicious/noUnsafeDeclarationMerging)

---

[error] 115-115: This class is unsafely merged with an interface.

The interface is declared here.

The TypeScript compiler doesn't check whether properties defined in the interface are initialized in the class.

(lint/suspicious/noUnsafeDeclarationMerging)

🔇 Additional comments (4)

src/js/internal/sql/errors.ts (1)

13-13: Verify Postgres errno type

PostgresErrorOptions.errno is typed as string. Confirm this matches the Zig/pg source (Postgres typically exposes code as string, but “errno” is not standard in PG).

src/js/internal/sql/mysql.ts (3)

112-112: Confirm Bun.BunFile is supported for tls in Zig binding

Ensure the Zig side handles BunFile (path/FD) and that downstream conversion matches TLSOptions semantics.

---

846-846: LGTM: typed Promise.withResolvers usage

The explicit void generic fixes inference edge-cases.

Also applies to: 870-870

---

1182-1183: LGTM: export value SQLCommand in default export

Keeps runtime access while retaining type export above.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (5)

test/js/sql/tls-sql.test.ts (5)

37-40: Prefer marking the suite as skipped instead of logging; avoid printing option blobs

Use a skipped test to surface the reason in runner output and keep CI logs clean.

-  if (options.url === undefined) {
-    console.log("SKIPPING TEST", JSON.stringify(options), "BECAUSE MISSING THE URL SECRET");
-    continue;
-  }
+  if (!options.url) {
+    test.todo(`TLS SQL suite skipped: missing URL secret for ${options.adapter}`);
+    continue;
+  }

---

202-209: Tighten assertion: check error class before reading errno

Makes the expectation clearer and guards against unexpected shapes.

-      expect(
-        await sql
-          .begin(sql => [sql`select wat`, sql`select current_setting('bun_sql.test') as x, ${1} as a`])
-          .catch(e => e.errno),
-      ).toBe("42703");
+      const err = await sql
+        .begin(sql => [sql`select wat`, sql`select current_setting('bun_sql.test') as x, ${1} as a`])
+        .catch(e => e);
+      expect(err).toBeInstanceOf(SQL.SQLError);
+      expect(err).toBeInstanceOf(SQL.PostgresError);
+      expect(err.errno).toBe("42703");

---

75-82: Remove redundant close when using await using

await using will dispose automatically; explicit close() is unnecessary.

       await using sql = new SQL(options);
       const [{ one, two }] = await sql`SELECT 1 as one, '2' as two`;
       expect(one).toBe(1);
       expect(two).toBe("2");
-      await sql.close();

---

49-56: Deduplicate expectations

These two lines are repeated above; keep a single assertion each.

-      expect(sql.distributed).toBeDefined();
-      expect(sql.unsafe).toBeDefined();

---

65-72: Deduplicate expectations (postgres variant)

Same duplicates here; remove the second pair.

-      expect(postgres.distributed).toBeDefined();
-      expect(postgres.unsafe).toBeDefined();

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between db15759 and c691df7.

📒 Files selected for processing (1)

• test/js/sql/tls-sql.test.ts (2 hunks)

🧰 Additional context used

📓 Path-based instructions (7)

test/**

📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)

Place all tests under the test/ directory

Files:

• test/js/sql/tls-sql.test.ts

test/js/**/*.{js,ts}

📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)

Place JavaScript and TypeScript tests under test/js/

Files:

• test/js/sql/tls-sql.test.ts

test/**/*.{js,ts}

📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)

test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable

Files:

• test/js/sql/tls-sql.test.ts

test/**/*.test.ts

📄 CodeRabbit inference engine (test/CLAUDE.md)

test/**/*.test.ts: Name test files *.test.ts and use bun:test
Do not write flaky tests: never wait for arbitrary time; wait for conditions instead
Never hardcode port numbers in tests; use port: 0 to get a random port
When spawning Bun in tests, use bunExe() and bunEnv from harness
Prefer async/await in tests; for a single callback, use Promise.withResolvers()
Do not set explicit test timeouts; rely on Bun’s built-in timeouts
Use tempDir/tempDirWithFiles from harness for temporary files and directories in tests
For large/repetitive strings in tests, prefer Buffer.alloc(count, fill).toString() over "A".repeat(count)
Import common test utilities from harness (e.g., bunExe, bunEnv, tempDirWithFiles, tmpdirSync, platform checks, GC helpers)
In error tests, assert non-zero exit codes for failing processes and use toThrow for synchronous errors
Use describe blocks for grouping, describe.each for parameterized tests, snapshots with toMatchSnapshot, and lifecycle hooks (beforeAll, beforeEach, afterEach); track resources for cleanup in afterEach
Use using/await using with Bun resources (e.g., Bun.listen/connect/spawn/serve) to ensure cleanup in tests

Files:

• test/js/sql/tls-sql.test.ts

test/js/**

📄 CodeRabbit inference engine (test/CLAUDE.md)

Organize unit tests for specific features under test/js/ by module

Files:

• test/js/sql/tls-sql.test.ts

test/**/*.test.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

test/**/*.test.{ts,tsx}: Test files must be placed under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or use custom random port functions
In tests, use normalizeBunSnapshot when asserting snapshots
Never write tests that merely assert absence of "panic" or "uncaught exception" in output
Avoid shell commands (e.g., find, grep) in tests; use Bun.Glob and built-ins instead
Prefer snapshot tests over exact stdout equality assertions

Files:

• test/js/sql/tls-sql.test.ts

**/*.{js,ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

Format JavaScript/TypeScript files with Prettier (bun run prettier)

Files:

• test/js/sql/tls-sql.test.ts

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: Format

[Jarred-Sumner]

Suggested change

const stat = require("node:fs").statSync("/tmp/mysql.sock");

[Jarred-Sumner]

Suggested change

const file = await Bun.file("/tmp/mysql.sock").arrayBuffer();