From 66548b52ea9040f977382d62d383ad15ed41427b Mon Sep 17 00:00:00 2001 From: Haider Date: Thu, 20 Aug 2026 21:48:27 +0530 Subject: [PATCH 1/5] fix(drivers): resolve warehouse SDKs from disk instead of reporting them missing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A bare `import("snowflake-sdk")` inside the compiled Bun binary resolves against bunfs, which has no `node_modules`. An SDK the user had already installed was invisible to the runtime, which then reported it as "not installed" — the single root cause behind nine open issues, five of which were filed automatically by the telemetry scanner. Add `packages/drivers/src/resolve.ts` and route all twelve drivers through it: - `loadOptionalDriver()` tries the ambient resolver first (unchanged behaviour in dev and the monorepo), then resolves against real directories: the managed install dir, `ALTIMATE_BIN_DIR`, `NODE_PATH`, the project and its parents, and the executable's own tree. - Installs land in `/altimate-code/drivers`, which no upgrade path touches. `~/.altimate/bin` is rebuilt by the curl installer's self-upgrade, which is how hand-installed drivers were being wiped. - A package that is present but fails to load is now reported as a broken install rather than a missing one, so users are not sent to reinstall what they already have. - `DriverNotInstalledError` names the exact install command and every location searched, replacing twelve copies of a bare `npm install` hint. Also add the `warehouse_install_driver` tool, and have `warehouse_add` report driver readiness at the point it can still be acted on. The check is filesystem-only and deliberately does not install: adding a connection must not block on a network `npm install`. Fix pre-existing drift in the driver catalogue. `mongodb` had a driver module and a workspace dependency but was missing from the binary's `optionalExternals` (so it was bundled instead of installed on demand) and from the published package's optional peer dependencies (so it was never surfaced to users). `driver-catalogue.test.ts` now holds all four declaration sites to `DRIVER_PACKAGES`. Verified in the environment the bug actually occurs in: compiled a binary with the production `Bun.build` options and confirmed bare `import("pg")` fails with `Cannot find package 'pg' from '/$bunfs/root/…'` while `loadOptionalDriver` loads the real module. Same for the subpath (`mysql2/promise`) and scoped (`@clickhouse/client`) specifier shapes. Tests: 162 drivers unit, 4,712 opencode, 140 Docker-backed driver e2e (Postgres, DuckDB, ClickHouse, MongoDB, data-diff), 29 real-Snowflake finops e2e. Typecheck clean. Closes #671 Closes #295 Closes #1075 Closes #61 Closes #769 Closes #764 Closes #713 Closes #670 Closes #659 Co-Authored-By: Claude Opus 5 (1M context) --- packages/drivers/src/bigquery.ts | 9 +- packages/drivers/src/clickhouse.ts | 13 +- packages/drivers/src/databricks.ts | 11 +- packages/drivers/src/duckdb.ts | 9 +- packages/drivers/src/mongodb.ts | 9 +- packages/drivers/src/mysql.ts | 9 +- packages/drivers/src/oracle.ts | 12 +- packages/drivers/src/postgres.ts | 7 +- packages/drivers/src/redshift.ts | 9 +- packages/drivers/src/resolve.ts | 392 ++++++++++++++++++ packages/drivers/src/snowflake.ts | 11 +- packages/drivers/src/sqlserver.ts | 16 +- packages/drivers/src/trino.ts | 8 +- packages/drivers/test/resolve-unit.test.ts | 311 ++++++++++++++ packages/opencode/script/build.ts | 11 +- packages/opencode/script/publish.ts | 6 + .../src/altimate/tools/warehouse-add.ts | 43 ++ .../tools/warehouse-install-driver.ts | 112 +++++ packages/opencode/src/tool/registry.ts | 2 + .../test/altimate/driver-catalogue.test.ts | 90 ++++ 20 files changed, 1002 insertions(+), 88 deletions(-) create mode 100644 packages/drivers/src/resolve.ts create mode 100644 packages/drivers/test/resolve-unit.test.ts create mode 100644 packages/opencode/src/altimate/tools/warehouse-install-driver.ts create mode 100644 packages/opencode/test/altimate/driver-catalogue.test.ts diff --git a/packages/drivers/src/bigquery.ts b/packages/drivers/src/bigquery.ts index abc7a8f05f..6a070fa867 100644 --- a/packages/drivers/src/bigquery.ts +++ b/packages/drivers/src/bigquery.ts @@ -3,16 +3,11 @@ */ import type { ConnectionConfig, Connector, ConnectorResult, ExecuteOptions, SchemaColumn } from "./types" +import { loadOptionalDriver } from "./resolve" export async function connect(config: ConnectionConfig): Promise { let BigQueryModule: any - try { - BigQueryModule = await import("@google-cloud/bigquery") - } catch { - throw new Error( - "BigQuery driver not installed. Run: npm install @google-cloud/bigquery", - ) - } + BigQueryModule = await loadOptionalDriver("bigquery", "@google-cloud/bigquery") const BigQuery = BigQueryModule.BigQuery ?? BigQueryModule.default?.BigQuery let client: any diff --git a/packages/drivers/src/clickhouse.ts b/packages/drivers/src/clickhouse.ts index 38eb738494..694e628242 100644 --- a/packages/drivers/src/clickhouse.ts +++ b/packages/drivers/src/clickhouse.ts @@ -6,17 +6,14 @@ */ import type { ConnectionConfig, Connector, ConnectorResult, ExecuteOptions, SchemaColumn } from "./types" +import { loadOptionalDriver } from "./resolve" export async function connect(config: ConnectionConfig): Promise { let createClient: any - try { - const mod = await import("@clickhouse/client") - createClient = mod.createClient ?? mod.default?.createClient - if (!createClient) { - throw new Error("createClient export not found in @clickhouse/client") - } - } catch { - throw new Error("ClickHouse driver not installed. Run: npm install @clickhouse/client") + const clickhouseModule = await loadOptionalDriver("clickhouse", "@clickhouse/client") + createClient = clickhouseModule.createClient ?? clickhouseModule.default?.createClient + if (!createClient) { + throw new Error("createClient export not found in @clickhouse/client — check the installed package version") } let client: any diff --git a/packages/drivers/src/databricks.ts b/packages/drivers/src/databricks.ts index 83e75dcd7c..3c9eee1f19 100644 --- a/packages/drivers/src/databricks.ts +++ b/packages/drivers/src/databricks.ts @@ -3,17 +3,12 @@ */ import type { ConnectionConfig, Connector, ConnectorResult, ExecuteOptions, SchemaColumn } from "./types" +import { loadOptionalDriver } from "./resolve" export async function connect(config: ConnectionConfig): Promise { let databricksModule: any - try { - databricksModule = await import("@databricks/sql") - databricksModule = databricksModule.default || databricksModule - } catch { - throw new Error( - "Databricks driver not installed. Run: npm install @databricks/sql", - ) - } + databricksModule = await loadOptionalDriver("databricks", "@databricks/sql") + databricksModule = databricksModule.default || databricksModule let client: any let session: any diff --git a/packages/drivers/src/duckdb.ts b/packages/drivers/src/duckdb.ts index 867840d0a4..32bf58c52a 100644 --- a/packages/drivers/src/duckdb.ts +++ b/packages/drivers/src/duckdb.ts @@ -3,15 +3,12 @@ */ import type { ConnectionConfig, Connector, ConnectorResult, ExecuteOptions, SchemaColumn } from "./types" +import { loadOptionalDriver } from "./resolve" export async function connect(config: ConnectionConfig): Promise { let duckdb: any - try { - duckdb = await import("duckdb") - duckdb = duckdb.default || duckdb - } catch { - throw new Error("DuckDB driver not installed. Run: npm install duckdb") - } + duckdb = await loadOptionalDriver("duckdb", "duckdb") + duckdb = duckdb.default || duckdb const dbPath = (config.path as string) ?? ":memory:" let db: any diff --git a/packages/drivers/src/mongodb.ts b/packages/drivers/src/mongodb.ts index 0e7ba87742..f353ee49f1 100644 --- a/packages/drivers/src/mongodb.ts +++ b/packages/drivers/src/mongodb.ts @@ -15,6 +15,7 @@ */ import type { ConnectionConfig, Connector, ConnectorResult, SchemaColumn } from "./types" +import { loadOptionalDriver } from "./resolve" /** Supported MQL commands. */ type MqlCommand = @@ -130,12 +131,8 @@ function extractFields(docs: Record[]): Map export async function connect(config: ConnectionConfig): Promise { let mongoModule: any - try { - mongoModule = await import("mongodb") - mongoModule = mongoModule.default || mongoModule - } catch { - throw new Error("MongoDB driver not installed. Run: npm install mongodb") - } + mongoModule = await loadOptionalDriver("mongodb", "mongodb") + mongoModule = mongoModule.default || mongoModule const MongoClient = mongoModule.MongoClient diff --git a/packages/drivers/src/mysql.ts b/packages/drivers/src/mysql.ts index 3859f5e993..c3e2608fcd 100644 --- a/packages/drivers/src/mysql.ts +++ b/packages/drivers/src/mysql.ts @@ -3,15 +3,12 @@ */ import type { ConnectionConfig, Connector, ConnectorResult, ExecuteOptions, SchemaColumn } from "./types" +import { loadOptionalDriver } from "./resolve" export async function connect(config: ConnectionConfig): Promise { let mysql: any - try { - mysql = await import("mysql2/promise") - mysql = mysql.default || mysql - } catch { - throw new Error("MySQL driver not installed. Run: npm install mysql2") - } + mysql = await loadOptionalDriver("mysql", "mysql2/promise") + mysql = mysql.default || mysql let pool: any diff --git a/packages/drivers/src/oracle.ts b/packages/drivers/src/oracle.ts index 39e4b11c37..30a666e2d5 100644 --- a/packages/drivers/src/oracle.ts +++ b/packages/drivers/src/oracle.ts @@ -3,18 +3,12 @@ */ import type { ConnectionConfig, Connector, ConnectorResult, ExecuteOptions, SchemaColumn } from "./types" +import { loadOptionalDriver } from "./resolve" export async function connect(config: ConnectionConfig): Promise { let oracledb: any - try { - // @ts-expect-error — optional dependency, loaded at runtime - oracledb = await import("oracledb") - oracledb = oracledb.default || oracledb - } catch { - throw new Error( - "Oracle driver not installed. Run: npm install oracledb", - ) - } + oracledb = await loadOptionalDriver("oracle", "oracledb") + oracledb = oracledb.default || oracledb // Use thin mode (pure JS, no Oracle client needed) oracledb.initOracleClient = undefined diff --git a/packages/drivers/src/postgres.ts b/packages/drivers/src/postgres.ts index 755b2e4ed9..8b8d39ab73 100644 --- a/packages/drivers/src/postgres.ts +++ b/packages/drivers/src/postgres.ts @@ -3,14 +3,11 @@ */ import type { ConnectionConfig, Connector, ConnectorResult, ExecuteOptions, SchemaColumn } from "./types" +import { loadOptionalDriver } from "./resolve" export async function connect(config: ConnectionConfig): Promise { let pg: any - try { - pg = await import("pg") - } catch { - throw new Error("PostgreSQL driver not installed. Run: npm install pg @types/pg") - } + pg = await loadOptionalDriver("postgres", "pg") const Pool = pg.default?.Pool ?? pg.Pool let pool: any diff --git a/packages/drivers/src/redshift.ts b/packages/drivers/src/redshift.ts index 92f8f32790..af3a70e7bc 100644 --- a/packages/drivers/src/redshift.ts +++ b/packages/drivers/src/redshift.ts @@ -4,16 +4,11 @@ */ import type { ConnectionConfig, Connector, ConnectorResult, ExecuteOptions, SchemaColumn } from "./types" +import { loadOptionalDriver } from "./resolve" export async function connect(config: ConnectionConfig): Promise { let pg: any - try { - pg = await import("pg") - } catch { - throw new Error( - "Redshift driver not installed (uses pg). Run: npm install pg @types/pg", - ) - } + pg = await loadOptionalDriver("redshift", "pg") const Pool = pg.default?.Pool ?? pg.Pool let pool: any diff --git a/packages/drivers/src/resolve.ts b/packages/drivers/src/resolve.ts new file mode 100644 index 0000000000..a60cf02011 --- /dev/null +++ b/packages/drivers/src/resolve.ts @@ -0,0 +1,392 @@ +/** + * Resolution and on-demand installation for optional warehouse SDKs. + * + * Warehouse SDKs (`snowflake-sdk`, `pg`, `@google-cloud/bigquery`, …) are + * optional dependencies: they are marked external in the binary build and + * installed per warehouse, on demand. Two things broke that arrangement. + * + * 1. A bare `import("snowflake-sdk")` inside the compiled Bun binary resolves + * against bunfs, which has no `node_modules`. An SDK the user had already + * installed — globally, or into the project — was invisible to the runtime, + * which then reported it as "not installed". + * 2. The curl install's self-upgrade re-runs the install script, which rebuilds + * `~/.altimate/bin`. Anything installed into that directory by hand is lost + * on the next upgrade. + * + * So: search real directories on disk rather than trusting the ambient module + * resolver, and install into a directory under the XDG data dir that no + * upgrade path touches. + */ + +import * as fs from "fs" +import * as os from "os" +import * as path from "path" +import { createRequire } from "node:module" +import { pathToFileURL } from "node:url" +import { spawn } from "node:child_process" + +/** Every driver in this package and the npm packages it needs at runtime. */ +export const DRIVER_PACKAGES = { + postgres: ["pg"], + redshift: ["pg"], + snowflake: ["snowflake-sdk"], + bigquery: ["@google-cloud/bigquery"], + databricks: ["@databricks/sql"], + mysql: ["mysql2"], + sqlserver: ["mssql"], + oracle: ["oracledb"], + duckdb: ["duckdb"], + mongodb: ["mongodb"], + clickhouse: ["@clickhouse/client"], + trino: ["trino-client"], +} as const satisfies Record + +export type DriverName = keyof typeof DRIVER_PACKAGES + +/** Human-facing driver labels, used in error text. */ +const DRIVER_LABELS: Record = { + postgres: "PostgreSQL", + redshift: "Redshift", + snowflake: "Snowflake", + bigquery: "BigQuery", + databricks: "Databricks", + mysql: "MySQL", + sqlserver: "SQL Server", + oracle: "Oracle", + duckdb: "DuckDB", + mongodb: "MongoDB", + clickhouse: "ClickHouse", + trino: "Trino", +} + +export function driverLabel(driver: DriverName): string { + return DRIVER_LABELS[driver] +} + +/** + * Raised when a driver's SDK cannot be found anywhere on the search path. + * + * Carries the searched roots so callers can tell a user with a genuinely + * missing package apart from one whose package is installed somewhere we never + * looked — the two failure modes were indistinguishable before. + */ +export class DriverNotInstalledError extends Error { + readonly driver: DriverName + readonly packages: readonly string[] + readonly searched: readonly string[] + + constructor(driver: DriverName, packages: readonly string[], searched: readonly string[]) { + const label = DRIVER_LABELS[driver] + super( + `${label} driver not installed.\n` + + `Install it with the warehouse_install_driver tool, or run:\n` + + ` npm install --prefix ${driverInstallDir()} ${packages.join(" ")}\n` + + `Searched ${searched.length} location${searched.length === 1 ? "" : "s"}: ${searched.join(", ")}`, + ) + this.name = "DriverNotInstalledError" + this.driver = driver + this.packages = packages + this.searched = searched + } +} + +/** + * Base of the XDG data dir, mirroring the `xdg-basedir` package that + * `@opencode-ai/core`'s global paths use. Duplicated rather than imported: + * importing core from here would pull in a module that creates directories as + * an import side effect, and this package is also consumed standalone. + */ +function xdgDataHome(): string { + const explicit = process.env["XDG_DATA_HOME"] + if (explicit) return explicit + return path.join(homeDir(), ".local", "share") +} + +function homeDir(): string { + // Honoured by the test suite to redirect global state away from the real home. + return process.env["OPENCODE_TEST_HOME"] ?? os.homedir() +} + +/** + * Directory that on-demand driver installs are written to. + * + * Deliberately under the XDG data dir rather than `~/.altimate/bin`: the curl + * installer owns that directory and rebuilds it on every self-upgrade, which is + * how hand-installed drivers were being wiped. + */ +export function driverInstallDir(): string { + const override = process.env["ALTIMATE_DRIVER_DIR"] + if (override) return override + return path.join(xdgDataHome(), "altimate-code", "drivers") +} + +function isDirectory(candidate: string): boolean { + try { + return fs.statSync(candidate).isDirectory() + } catch { + return false + } +} + +/** Collect every `node_modules` directory from `start` up to the filesystem root. */ +function nodeModulesUpward(start: string): string[] { + const found: string[] = [] + let current = path.resolve(start) + for (;;) { + const candidate = path.join(current, "node_modules") + if (isDirectory(candidate)) found.push(candidate) + const parent = path.dirname(current) + if (parent === current) break + current = parent + } + return found +} + +/** + * Directories to search for an optional SDK, most specific first. + * + * The managed install dir comes first so a driver we installed wins over a + * stale copy elsewhere on the machine. + */ +export function driverSearchRoots(): string[] { + const roots: string[] = [] + + const push = (dir: string | undefined) => { + if (!dir) return + const resolved = path.resolve(dir) + if (isDirectory(resolved) && !roots.includes(resolved)) roots.push(resolved) + } + + // 1. Drivers this CLI installed on demand. + push(path.join(driverInstallDir(), "node_modules")) + + // 2. Alongside the npm wrapper. bin/altimate exports ALTIMATE_BIN_DIR, which + // is where a global `npm install -g altimate-code` puts its dependencies. + const binDir = process.env["ALTIMATE_BIN_DIR"] + if (binDir) for (const dir of nodeModulesUpward(binDir)) push(dir) + + // 3. NODE_PATH, which the npm wrapper populates and users may also set. + const nodePath = process.env["NODE_PATH"] + if (nodePath) for (const entry of nodePath.split(path.delimiter)) push(entry) + + // 4. The project the user is working in, and every parent of it — covers a + // plain `npm install snowflake-sdk` in the dbt project. + for (const dir of nodeModulesUpward(process.cwd())) push(dir) + + // 5. Around the running executable. For `npm install -g` this is the global + // root, which is what makes a globally installed SDK resolvable. + try { + for (const dir of nodeModulesUpward(path.dirname(fs.realpathSync(process.execPath)))) push(dir) + } catch { + // execPath may not be resolvable (bunfs); the roots above still apply. + } + + return roots +} + +/** Split a specifier such as `mysql2/promise` into its package name. */ +export function packageNameOf(specifier: string): string { + const segments = specifier.split("/") + if (specifier.startsWith("@")) return segments.slice(0, 2).join("/") + return segments[0]! +} + +/** + * Absolute path to `specifier` if it is installed under any search root. + * + * Returns the resolved entry file, or the package directory when the package is + * present but exports no CommonJS entry that `require.resolve` can name. + */ +export function resolveOptionalPackage(specifier: string, roots = driverSearchRoots()): string | undefined { + const pkg = packageNameOf(specifier) + const require = createRequire(pathToFileURL(path.join(process.cwd(), "noop.js")).href) + + for (const root of roots) { + if (!isDirectory(path.join(root, pkg))) continue + try { + return require.resolve(specifier, { paths: [root] }) + } catch { + // ESM-only packages have no require-resolvable entry. Hand back the + // package directory and let the dynamic import read its export map. + const dir = path.join(root, specifier) + if (isDirectory(dir) || fs.existsSync(dir)) return dir + return path.join(root, pkg) + } + } + + return undefined +} + +/** + * Import an optional warehouse SDK. + * + * Tries the ambient resolver first so development, the monorepo, and any + * already-working install behave exactly as before, then falls back to + * searching real directories. + * + * @throws {DriverNotInstalledError} when the package is genuinely absent. + */ +export async function loadOptionalDriver(driver: DriverName, specifier: string): Promise { + try { + return await import(/* @vite-ignore */ specifier) + } catch (ambientError) { + // Only a resolution failure means "look elsewhere". A package that resolves + // ambiently but throws while loading is a broken install, and re-reporting + // it as missing would send the user to install what they already have. + if (!isModuleNotFound(ambientError)) throw loadFailure(driver, specifier, ambientError) + + const roots = driverSearchRoots() + const resolved = resolveOptionalPackage(specifier, roots) + if (!resolved) throw new DriverNotInstalledError(driver, DRIVER_PACKAGES[driver], roots) + + try { + return await import(/* @vite-ignore */ pathToFileURL(resolved).href) + } catch (loadError) { + // On disk but will not load — a half-installed copy, or a native addon + // built for another platform. + throw loadFailure(driver, resolved, loadError) + } + } +} + +/** True when `error` means the module could not be resolved, not that it failed while loading. */ +function isModuleNotFound(error: unknown): boolean { + const code = (error as { code?: string } | null)?.code + if (code === "ERR_MODULE_NOT_FOUND" || code === "MODULE_NOT_FOUND") return true + const message = error instanceof Error ? error.message : String(error) + return /Cannot find (module|package)/i.test(message) +} + +function loadFailure(driver: DriverName, where: string, error: unknown): Error { + return new Error( + `${DRIVER_LABELS[driver]} driver found at ${where} but failed to load: ` + + `${error instanceof Error ? error.message : String(error)}`, + ) +} + +/** True when `driver`'s packages are all resolvable right now. */ +export function isDriverInstalled(driver: DriverName, roots = driverSearchRoots()): boolean { + return DRIVER_PACKAGES[driver].every((pkg) => resolveOptionalPackage(pkg, roots) !== undefined) +} + +export interface InstallResult { + readonly driver: DriverName + readonly packages: readonly string[] + readonly dir: string + readonly installed: boolean + readonly alreadyPresent: boolean + readonly error?: string +} + +function runNpm(args: string[], cwd: string, timeoutMs: number): Promise<{ code: number; output: string }> { + return new Promise((resolve) => { + // npm ships as a shell script on POSIX and a .cmd on Windows; `shell: true` + // lets the platform resolve whichever is present on PATH. + const child = spawn("npm", args, { cwd, shell: true, stdio: ["ignore", "pipe", "pipe"] }) + let output = "" + let settled = false + const finish = (code: number) => { + if (settled) return + settled = true + clearTimeout(timer) + resolve({ code, output: output.trim() }) + } + const timer = setTimeout(() => { + child.kill() + output += `\nTimed out after ${Math.round(timeoutMs / 1000)}s.` + finish(124) + }, timeoutMs) + child.stdout?.on("data", (chunk) => (output += String(chunk))) + child.stderr?.on("data", (chunk) => (output += String(chunk))) + child.on("error", (err) => { + output += String(err instanceof Error ? err.message : err) + finish(127) + }) + child.on("close", (code) => finish(code ?? 1)) + }) +} + +/** + * Install a driver's SDK into the managed driver directory. + * + * Installs are additive — `--no-save` against a private package.json — so + * installing a second driver never removes the first. + */ +export async function installOptionalDriver( + driver: DriverName, + options: { timeoutMs?: number } = {}, +): Promise { + const packages = DRIVER_PACKAGES[driver] + const dir = driverInstallDir() + + if (isDriverInstalled(driver)) { + return { driver, packages, dir, installed: true, alreadyPresent: true } + } + + try { + fs.mkdirSync(dir, { recursive: true }) + const manifest = path.join(dir, "package.json") + if (!fs.existsSync(manifest)) { + // A private, versionless manifest keeps npm from warning on every install + // and marks the directory as ours rather than a stray project. + fs.writeFileSync( + manifest, + JSON.stringify({ name: "altimate-code-drivers", private: true, description: "Warehouse SDKs installed on demand by Altimate Code." }, null, 2) + "\n", + ) + } + } catch (e) { + return { + driver, + packages, + dir, + installed: false, + alreadyPresent: false, + error: `Could not create the driver directory ${dir}: ${e instanceof Error ? e.message : String(e)}`, + } + } + + const { code, output } = await runNpm( + ["install", "--no-save", "--no-audit", "--no-fund", "--loglevel=error", ...packages], + dir, + options.timeoutMs ?? 180_000, + ) + + if (code === 127) { + return { + driver, + packages, + dir, + installed: false, + alreadyPresent: false, + error: + `npm is not available on PATH, so ${DRIVER_LABELS[driver]} cannot be installed automatically. ` + + `Install Node.js, then run: npm install --prefix ${dir} ${packages.join(" ")}`, + } + } + + if (code !== 0) { + return { + driver, + packages, + dir, + installed: false, + alreadyPresent: false, + error: `npm install failed (exit ${code}) for ${packages.join(", ")}: ${output || "no output"}`, + } + } + + // Confirm against the resolver rather than trusting npm's exit code — an + // install that lands somewhere we do not search is not a working driver. + if (!isDriverInstalled(driver)) { + return { + driver, + packages, + dir, + installed: false, + alreadyPresent: false, + error: `npm reported success but ${packages.join(", ")} is still not resolvable from ${dir}.`, + } + } + + return { driver, packages, dir, installed: true, alreadyPresent: false } +} diff --git a/packages/drivers/src/snowflake.ts b/packages/drivers/src/snowflake.ts index 47b8ee942a..9cafa5a0f5 100644 --- a/packages/drivers/src/snowflake.ts +++ b/packages/drivers/src/snowflake.ts @@ -4,6 +4,7 @@ import * as fs from "fs" import type { ConnectionConfig, Connector, ConnectorResult, ExecuteOptions, SchemaColumn } from "./types" +import { loadOptionalDriver } from "./resolve" /** * Run `fn` with stdout/stderr writes swallowed for the (synchronous) duration of @@ -52,14 +53,8 @@ export function suppressSnowflakeLogging(snowflake: any): void { export async function connect(config: ConnectionConfig): Promise { let snowflake: any - try { - snowflake = await import("snowflake-sdk") - snowflake = snowflake.default || snowflake - } catch { - throw new Error( - "Snowflake driver not installed. Run: npm install snowflake-sdk", - ) - } + snowflake = await loadOptionalDriver("snowflake", "snowflake-sdk") + snowflake = snowflake.default || snowflake // Suppress snowflake-sdk's Winston console logging as early as possible — it // writes JSON log lines into the interactive TUI output and corrupts the diff --git a/packages/drivers/src/sqlserver.ts b/packages/drivers/src/sqlserver.ts index 8d2b45bd81..973f6a6a15 100644 --- a/packages/drivers/src/sqlserver.ts +++ b/packages/drivers/src/sqlserver.ts @@ -3,6 +3,7 @@ */ import type { ConnectionConfig, Connector, ConnectorResult, ExecuteOptions, SchemaColumn } from "./types" +import { loadOptionalDriver } from "./resolve" // --------------------------------------------------------------------------- // Azure AD helpers — cache + resource URL resolution @@ -74,17 +75,10 @@ export function _resetTokenCacheForTests(): void { export async function connect(config: ConnectionConfig): Promise { let mssql: any let MssqlConnectionPool: any - try { - // @ts-expect-error — mssql has no type declarations; installed as optional peerDependency - const mod = await import("mssql") - mssql = mod.default || mod - // ConnectionPool is a named export, not on .default - MssqlConnectionPool = mod.ConnectionPool ?? mssql.ConnectionPool - } catch { - throw new Error( - "SQL Server driver not installed. Run: npm install mssql", - ) - } + const mssqlModule = await loadOptionalDriver("sqlserver", "mssql") + mssql = mssqlModule.default || mssqlModule + // ConnectionPool is a named export, not on .default + MssqlConnectionPool = mssqlModule.ConnectionPool ?? mssql.ConnectionPool let pool: any diff --git a/packages/drivers/src/trino.ts b/packages/drivers/src/trino.ts index a989217dde..19d251894f 100644 --- a/packages/drivers/src/trino.ts +++ b/packages/drivers/src/trino.ts @@ -6,6 +6,7 @@ */ import type { ConnectionConfig, Connector, ConnectorResult, ExecuteOptions, SchemaColumn } from "./types" +import { loadOptionalDriver } from "./resolve" type QueryResult = { columns?: Array<{ name: string; type: string }> @@ -87,12 +88,7 @@ function trinoError(result: QueryResult): Error | null { export async function connect(config: ConnectionConfig): Promise { let Trino: any let BasicAuth: any - let mod: any - try { - mod = await import("trino-client") - } catch { - throw new Error("Trino driver not installed. Run: npm install trino-client") - } + const mod: any = await loadOptionalDriver("trino", "trino-client") Trino = mod.Trino ?? mod.default?.Trino ?? mod.default BasicAuth = mod.BasicAuth ?? mod.default?.BasicAuth if (!Trino?.create) { diff --git a/packages/drivers/test/resolve-unit.test.ts b/packages/drivers/test/resolve-unit.test.ts new file mode 100644 index 0000000000..ac1ae26b3c --- /dev/null +++ b/packages/drivers/test/resolve-unit.test.ts @@ -0,0 +1,311 @@ +/** + * Unit tests for optional-driver resolution and installation. + * + * These cover the reports the resolver exists to fix: + * - #671 / #295 — an SDK the user already installed was invisible to the + * compiled binary, which reported it as "not installed". + * - #1075 — drivers installed by hand into ~/.altimate/bin were wiped by the + * self-upgrade, so installs must land somewhere the upgrade never touches. + * - #769 / #764 / #713 / #670 / #659 — the error text named a bare `npm + * install` with no indication of where to run it or where we looked. + */ +import { afterEach, beforeEach, describe, expect, test } from "bun:test" +import * as fs from "fs" +import * as os from "os" +import * as path from "path" + +import { + DRIVER_PACKAGES, + DriverNotInstalledError, + driverInstallDir, + driverLabel, + driverSearchRoots, + isDriverInstalled, + loadOptionalDriver, + packageNameOf, + resolveOptionalPackage, +} from "../src/resolve" + +let tmpRoot: string +const savedEnv: Record = {} +const ENV_KEYS = ["ALTIMATE_DRIVER_DIR", "ALTIMATE_BIN_DIR", "NODE_PATH", "XDG_DATA_HOME", "OPENCODE_TEST_HOME"] + +/** Write a minimal installed package at /node_modules/. */ +function installFakePackage(root: string, name: string, body: string): string { + const dir = path.join(root, "node_modules", ...name.split("/")) + fs.mkdirSync(dir, { recursive: true }) + fs.writeFileSync(path.join(dir, "index.js"), body) + fs.writeFileSync( + path.join(dir, "package.json"), + JSON.stringify({ name, version: "1.0.0", main: "index.js" }), + ) + return dir +} + +beforeEach(() => { + for (const key of ENV_KEYS) savedEnv[key] = process.env[key] + // realpath it: require.resolve returns realpaths, and on macOS the temp dir + // is reached through the /var -> /private/var symlink. + tmpRoot = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), "altimate-drivers-"))) +}) + +afterEach(() => { + for (const key of ENV_KEYS) { + if (savedEnv[key] === undefined) delete process.env[key] + else process.env[key] = savedEnv[key] + } + fs.rmSync(tmpRoot, { recursive: true, force: true }) +}) + +describe("packageNameOf", () => { + test("returns the package for a bare specifier", () => { + expect(packageNameOf("pg")).toBe("pg") + }) + + test("strips a subpath", () => { + // mysql.ts imports mysql2/promise, so the package probe must not look for + // a directory literally named "mysql2/promise". + expect(packageNameOf("mysql2/promise")).toBe("mysql2") + }) + + test("keeps both segments of a scoped package", () => { + expect(packageNameOf("@google-cloud/bigquery")).toBe("@google-cloud/bigquery") + expect(packageNameOf("@clickhouse/client/dist/x")).toBe("@clickhouse/client") + }) +}) + +describe("driverInstallDir", () => { + test("sits under the XDG data dir, not ~/.altimate/bin", () => { + delete process.env["ALTIMATE_DRIVER_DIR"] + process.env["XDG_DATA_HOME"] = path.join(tmpRoot, "xdg") + + const dir = driverInstallDir() + + expect(dir).toBe(path.join(tmpRoot, "xdg", "altimate-code", "drivers")) + // The curl installer rebuilds ~/.altimate/bin on every self-upgrade (#1075), + // so an install target inside it would be wiped on the next upgrade. + expect(dir.includes(path.join(".altimate", "bin"))).toBe(false) + }) + + test("honours an explicit override", () => { + process.env["ALTIMATE_DRIVER_DIR"] = path.join(tmpRoot, "custom") + expect(driverInstallDir()).toBe(path.join(tmpRoot, "custom")) + }) + + test("falls back to ~/.local/share when XDG_DATA_HOME is unset", () => { + delete process.env["ALTIMATE_DRIVER_DIR"] + delete process.env["XDG_DATA_HOME"] + process.env["OPENCODE_TEST_HOME"] = tmpRoot + + expect(driverInstallDir()).toBe(path.join(tmpRoot, ".local", "share", "altimate-code", "drivers")) + }) +}) + +describe("driverSearchRoots", () => { + test("puts the managed install dir first", () => { + const managed = path.join(tmpRoot, "managed") + fs.mkdirSync(path.join(managed, "node_modules"), { recursive: true }) + process.env["ALTIMATE_DRIVER_DIR"] = managed + + const roots = driverSearchRoots() + + expect(roots[0]).toBe(path.join(managed, "node_modules")) + }) + + test("includes node_modules next to ALTIMATE_BIN_DIR", () => { + // The npm wrapper (bin/altimate) exports ALTIMATE_BIN_DIR; for a global + // `npm install -g altimate-code` this is where dependencies live. + const binDir = path.join(tmpRoot, "global", "lib", "node_modules", "altimate-code", "bin") + fs.mkdirSync(binDir, { recursive: true }) + installFakePackage(path.join(tmpRoot, "global", "lib"), "pg", "module.exports = {}") + process.env["ALTIMATE_BIN_DIR"] = binDir + + const roots = driverSearchRoots() + + expect(roots).toContain(path.join(tmpRoot, "global", "lib", "node_modules")) + }) + + test("includes every NODE_PATH entry that exists", () => { + const a = path.join(tmpRoot, "a", "node_modules") + const b = path.join(tmpRoot, "b", "node_modules") + fs.mkdirSync(a, { recursive: true }) + fs.mkdirSync(b, { recursive: true }) + process.env["NODE_PATH"] = [a, b, path.join(tmpRoot, "missing")].join(path.delimiter) + + const roots = driverSearchRoots() + + expect(roots).toContain(a) + expect(roots).toContain(b) + // A NODE_PATH entry that does not exist must not become a search root. + expect(roots).not.toContain(path.join(tmpRoot, "missing")) + }) + + test("does not return duplicates", () => { + const shared = path.join(tmpRoot, "shared") + fs.mkdirSync(path.join(shared, "node_modules"), { recursive: true }) + process.env["ALTIMATE_DRIVER_DIR"] = shared + process.env["NODE_PATH"] = path.join(shared, "node_modules") + + const roots = driverSearchRoots() + + expect(roots.length).toBe(new Set(roots).size) + }) +}) + +describe("resolveOptionalPackage", () => { + test("finds a package installed under a search root", () => { + installFakePackage(tmpRoot, "pg", "module.exports = { Pool: function () {} }") + + const resolved = resolveOptionalPackage("pg", [path.join(tmpRoot, "node_modules")]) + + expect(resolved).toBeDefined() + expect(resolved!.startsWith(path.join(tmpRoot, "node_modules", "pg"))).toBe(true) + }) + + test("finds a scoped package", () => { + installFakePackage(tmpRoot, "@clickhouse/client", "module.exports = { createClient: function () {} }") + + const resolved = resolveOptionalPackage("@clickhouse/client", [path.join(tmpRoot, "node_modules")]) + + expect(resolved).toBeDefined() + }) + + test("returns undefined when the package is absent", () => { + fs.mkdirSync(path.join(tmpRoot, "node_modules"), { recursive: true }) + + expect(resolveOptionalPackage("snowflake-sdk", [path.join(tmpRoot, "node_modules")])).toBeUndefined() + }) + + test("prefers the earlier root when a package is installed twice", () => { + const first = path.join(tmpRoot, "first") + const second = path.join(tmpRoot, "second") + installFakePackage(first, "pg", "module.exports = { which: 'first' }") + installFakePackage(second, "pg", "module.exports = { which: 'second' }") + + const resolved = resolveOptionalPackage("pg", [ + path.join(first, "node_modules"), + path.join(second, "node_modules"), + ]) + + expect(resolved!.startsWith(first)).toBe(true) + }) +}) + +describe("loadOptionalDriver", () => { + test("loads a package that only exists on a search root", async () => { + // The regression from #671: the SDK is installed, but not anywhere the + // ambient module resolver looks from inside the compiled binary. The + // specifier is deliberately one that can never resolve ambiently, so this + // exercises the on-disk fallback rather than the workspace's own copy. + installFakePackage(tmpRoot, "altimate-fake-sdk", "module.exports = { marker: 'resolved-from-disk' }") + process.env["ALTIMATE_DRIVER_DIR"] = tmpRoot + + const mod: any = await loadOptionalDriver("postgres", "altimate-fake-sdk") + + expect(mod.marker ?? mod.default?.marker).toBe("resolved-from-disk") + }) + + test("throws DriverNotInstalledError naming the searched roots", async () => { + process.env["ALTIMATE_DRIVER_DIR"] = path.join(tmpRoot, "empty") + delete process.env["ALTIMATE_BIN_DIR"] + delete process.env["NODE_PATH"] + + let error: unknown + try { + await loadOptionalDriver("snowflake", "definitely-not-a-real-sdk-xyz") + } catch (e) { + error = e + } + + expect(error).toBeInstanceOf(DriverNotInstalledError) + const err = error as DriverNotInstalledError + expect(err.driver).toBe("snowflake") + expect(err.packages).toEqual(DRIVER_PACKAGES.snowflake) + // The old message was a bare "Run: npm install snowflake-sdk" with no + // target directory and no account of where we had looked. + expect(err.message).toContain("--prefix") + expect(err.message).toContain("Searched") + }) + + test("does not fall back when an ambiently-resolvable package fails to load", async () => { + // A package that resolves but throws on import is broken, not absent. + // Reporting it as "not installed" sends the user to install what they have. + const broken = path.join(tmpRoot, "ambient") + installFakePackage(broken, "altimate-ambient-broken", "throw new Error('boom')") + process.env["ALTIMATE_DRIVER_DIR"] = broken + + let error: unknown + try { + await loadOptionalDriver("postgres", "altimate-ambient-broken") + } catch (e) { + error = e + } + + expect(error).not.toBeInstanceOf(DriverNotInstalledError) + expect((error as Error).message).toContain("failed to load") + expect((error as Error).message).toContain("boom") + }) + + test("reports a broken install as a load failure, not as missing", async () => { + // A package that is present but throws on import used to be reported as + // "not installed", sending users to reinstall something already there. + installFakePackage(tmpRoot, "altimate-broken-sdk", "throw new Error('native binding is for another platform')") + process.env["ALTIMATE_DRIVER_DIR"] = tmpRoot + + let error: unknown + try { + await loadOptionalDriver("postgres", "altimate-broken-sdk") + } catch (e) { + error = e + } + + expect(error).toBeInstanceOf(Error) + expect(error).not.toBeInstanceOf(DriverNotInstalledError) + expect((error as Error).message).toContain("failed to load") + }) +}) + +describe("isDriverInstalled", () => { + test("is false for a driver with no packages under the given roots", () => { + const empty = path.join(tmpRoot, "empty", "node_modules") + fs.mkdirSync(empty, { recursive: true }) + + expect(isDriverInstalled("oracle", [empty])).toBe(false) + }) + + test("is true once the package is present", () => { + installFakePackage(tmpRoot, "oracledb", "module.exports = {}") + + expect(isDriverInstalled("oracle", [path.join(tmpRoot, "node_modules")])).toBe(true) + }) +}) + +describe("driver catalogue", () => { + test("every driver has a label and at least one package", () => { + for (const driver of Object.keys(DRIVER_PACKAGES) as Array) { + expect(driverLabel(driver).length).toBeGreaterThan(0) + expect(DRIVER_PACKAGES[driver].length).toBeGreaterThan(0) + } + }) + + test("covers every driver module that loads an optional SDK", () => { + // Guards against adding a driver file without registering its package — + // the resolver would then have nothing to install or search for. + const expected = [ + "postgres", + "redshift", + "snowflake", + "bigquery", + "databricks", + "mysql", + "sqlserver", + "oracle", + "duckdb", + "mongodb", + "clickhouse", + "trino", + ].sort() + + expect(Object.keys(DRIVER_PACKAGES).sort()).toEqual(expected) + }) +}) diff --git a/packages/opencode/script/build.ts b/packages/opencode/script/build.ts index 94ce36d04c..9f135ee8d5 100755 --- a/packages/opencode/script/build.ts +++ b/packages/opencode/script/build.ts @@ -237,9 +237,14 @@ await $`rm -rf dist` // without bloating it with 5 platforms' worth of native addons. const requiredExternals: string[] = [] const optionalExternals = [ - // Database drivers — native addons, users install on demand per warehouse + // Database drivers — native addons, users install on demand per warehouse. + // Must stay in step with DRIVER_PACKAGES in packages/drivers/src/resolve.ts: + // a driver package that is missing here gets bundled into the binary, so the + // on-demand install path never runs for it and the bundled copy is frozen at + // whatever version built the release. "pg", "snowflake-sdk", "@google-cloud/bigquery", "@databricks/sql", "mysql2", "mssql", "oracledb", "duckdb", + "mongodb", "@clickhouse/client", "trino-client", // Optional infra packages — native addons or heavy optional deps "keytar", "ssh2", "dockerode", ] @@ -475,6 +480,10 @@ for (const item of targets) { autoloadBunfig: false, autoloadDotenv: false, autoloadTsconfig: true, + // Load-bearing for the optional drivers above: it is what lets the + // compiled binary resolve an `external` package from node_modules on + // disk at runtime. Verified by compiling with and without it — without + // it every driver import fails inside bunfs, whatever NODE_PATH says. autoloadPackageJson: true, target: name.replace(pkg.name, "bun") as any, outfile: `dist/${name}/bin/altimate`, diff --git a/packages/opencode/script/publish.ts b/packages/opencode/script/publish.ts index f86c852295..a5e3830959 100755 --- a/packages/opencode/script/publish.ts +++ b/packages/opencode/script/publish.ts @@ -20,6 +20,11 @@ const runtimeDependencies: Record = { "@altimateai/altimate-core": altimateCoreDep, } +// Optional peer deps so `npm ls` and IDEs know which SDK versions a warehouse +// needs, without npm installing any of them. Keys must cover every package in +// DRIVER_PACKAGES (packages/drivers/src/resolve.ts); the driver-catalogue test +// asserts that, because a package missing here is one users are never told +// about. `mongodb` was absent until v0.9.6 for exactly that reason. const driverPeerDependencies: Record = { pg: ">=8", "snowflake-sdk": ">=1", @@ -29,6 +34,7 @@ const driverPeerDependencies: Record = { mssql: ">=11", oracledb: ">=6", duckdb: ">=1", + mongodb: ">=6", "@clickhouse/client": ">=1", "trino-client": ">=0.2", } diff --git a/packages/opencode/src/altimate/tools/warehouse-add.ts b/packages/opencode/src/altimate/tools/warehouse-add.ts index 9e9e9e8c42..07e3265c25 100644 --- a/packages/opencode/src/altimate/tools/warehouse-add.ts +++ b/packages/opencode/src/altimate/tools/warehouse-add.ts @@ -5,6 +5,15 @@ import { Dispatcher } from "../native" import { PostConnectSuggestions } from "./post-connect-suggestions" import { Telemetry } from "../../telemetry" // altimate_change end +// altimate_change start — report driver readiness when adding a warehouse +import { + driverForWarehouseType, + driverInstallDir, + driverLabel, + isDriverInstalled, + DRIVER_PACKAGES, +} from "./warehouse-install-driver" +// altimate_change end export const WarehouseAddTool = Tool.define("warehouse_add", { description: @@ -48,6 +57,12 @@ IMPORTANT: For private key file paths, always use "private_key_path" (not "priva // altimate_change start — append post-connect feature suggestions (async, non-blocking) let output = `Successfully added warehouse '${result.name}' (type: ${result.type}).\n\nUse warehouse_test to verify connectivity.` + // Adding a connection whose driver is missing used to leave a broken + // entry behind: every later operation failed with "driver not + // installed" and nothing said so at the point of adding. Say so here + // instead, at the point where it can still be acted on. + output += driverReadinessNote(result.type) + // Run suggestion gathering concurrently with a timeout to avoid // adding noticeable latency to the warehouse add response. try { @@ -131,3 +146,31 @@ IMPORTANT: For private key file paths, always use "private_key_path" (not "priva } }, }) + +// altimate_change start — driver readiness note for newly added warehouses +/** + * Note appended to a successful add when the warehouse's driver is missing. + * + * Deliberately a filesystem check and not an install: adding a connection must + * not block on a network `npm install`, which can take minutes. The install + * itself is the warehouse_install_driver tool, which this points at. + */ +function driverReadinessNote(type: string): string { + const driver = driverForWarehouseType(type) + // sqlite and any unrecognised type need no optional SDK. + if (!driver) return "" + + try { + if (isDriverInstalled(driver)) return "" + const packages = DRIVER_PACKAGES[driver].join(" ") + return ( + `\n\nNOTE: the ${driverLabel(driver)} driver is not installed yet, so this connection cannot be used until it is.\n` + + `Run the warehouse_install_driver tool with driver="${driver}", or install it manually:\n` + + ` npm install --prefix ${driverInstallDir()} ${packages}` + ) + } catch { + // A driver probe must never fail an add whose configuration was stored. + return "" + } +} +// altimate_change end diff --git a/packages/opencode/src/altimate/tools/warehouse-install-driver.ts b/packages/opencode/src/altimate/tools/warehouse-install-driver.ts new file mode 100644 index 0000000000..020cbd0e6b --- /dev/null +++ b/packages/opencode/src/altimate/tools/warehouse-install-driver.ts @@ -0,0 +1,112 @@ +import z from "zod" +import { Tool } from "../../tool/tool" +import { + DRIVER_PACKAGES, + driverInstallDir, + driverLabel, + installOptionalDriver, + isDriverInstalled, + type DriverName, +} from "@altimateai/drivers/resolve" + +// Listed literally rather than derived from DRIVER_PACKAGES so zod infers a +// concrete union; the catalogue test in packages/drivers keeps the two in step. +const DRIVER_NAMES = [ + "postgres", + "redshift", + "snowflake", + "bigquery", + "databricks", + "mysql", + "sqlserver", + "oracle", + "duckdb", + "mongodb", + "clickhouse", + "trino", +] as const + +/** + * Declared rather than inferred: Tool.define infers its metadata type from the + * execute return, and cannot unify branches whose object literals carry + * different keys. + */ +interface InstallDriverMetadata { + [key: string]: any + driver: DriverName + installed: boolean + alreadyPresent: boolean + dir: string + error?: string +} + +interface InstallDriverResult { + title: string + metadata: InstallDriverMetadata + output: string +} + +export const WarehouseInstallDriverTool = Tool.define("warehouse_install_driver", { + description: + "Install the database driver a warehouse type needs. Drivers are optional dependencies installed on demand; " + + "use this when a connection reports that its driver is not installed. The driver is installed into Altimate " + + "Code's own directory, so it survives CLI upgrades, and takes effect immediately — no session restart.", + parameters: z.object({ + driver: z.enum(DRIVER_NAMES).describe("Warehouse type whose driver should be installed"), + }), + async execute(args): Promise { + // The zod enum above guarantees one of the 12 driver names; the assertion + // re-narrows it, since z.enum over a readonly tuple widens to string. + const driver = args.driver as DriverName + const label = driverLabel(driver) + const dir = driverInstallDir() + + if (isDriverInstalled(driver)) { + return { + title: `${label} driver: already installed`, + metadata: { driver, installed: true, alreadyPresent: true, dir }, + output: `The ${label} driver is already installed and resolvable. No action taken.`, + } + } + + const result = await installOptionalDriver(driver) + const packages = result.packages.join(" ") + + if (!result.installed) { + return { + title: `${label} driver: install FAILED`, + metadata: { + driver, + installed: false, + alreadyPresent: false, + dir: result.dir, + error: result.error ?? "unknown error", + }, + output: + `Could not install the ${label} driver (${packages}).\n` + + `${result.error}\n\n` + + `Install it manually with:\n npm install --prefix ${result.dir} ${packages}`, + } + } + + return { + title: `${label} driver: installed`, + metadata: { driver, installed: true, alreadyPresent: false, dir: result.dir }, + output: + `Installed the ${label} driver (${packages}) into ${result.dir}.\n` + + `It is available now — connections using ${driver} will work without restarting the session.`, + } + }, +}) + +/** + * Driver name for a warehouse config `type`, or undefined when the type needs + * no optional SDK (sqlite ships with the runtime). + */ +export function driverForWarehouseType(type: string): DriverName | undefined { + const normalized = type.trim().toLowerCase() + return (DRIVER_NAMES as readonly string[]).includes(normalized) ? (normalized as DriverName) : undefined +} + +export { DRIVER_PACKAGES, driverInstallDir, isDriverInstalled, installOptionalDriver, driverLabel } +export type { DriverName } diff --git a/packages/opencode/src/tool/registry.ts b/packages/opencode/src/tool/registry.ts index 20e8bd1ba6..866cf3ee5a 100644 --- a/packages/opencode/src/tool/registry.ts +++ b/packages/opencode/src/tool/registry.ts @@ -64,6 +64,7 @@ import { LineageCheckTool } from "../altimate/tools/lineage-check" import { WarehouseListTool } from "../altimate/tools/warehouse-list" import { WarehouseTestTool } from "../altimate/tools/warehouse-test" import { WarehouseAddTool } from "../altimate/tools/warehouse-add" +import { WarehouseInstallDriverTool } from "../altimate/tools/warehouse-install-driver" import { WarehouseRemoveTool } from "../altimate/tools/warehouse-remove" import { WarehouseDiscoverTool } from "../altimate/tools/warehouse-discover" import { McpDiscoverTool } from "../altimate/tools/mcp-discover" @@ -395,6 +396,7 @@ export namespace ToolRegistry { WarehouseListTool, WarehouseTestTool, WarehouseAddTool, + WarehouseInstallDriverTool, WarehouseRemoveTool, WarehouseDiscoverTool, // altimate_change start - register MCP discovery tool diff --git a/packages/opencode/test/altimate/driver-catalogue.test.ts b/packages/opencode/test/altimate/driver-catalogue.test.ts new file mode 100644 index 0000000000..5e6196fcc0 --- /dev/null +++ b/packages/opencode/test/altimate/driver-catalogue.test.ts @@ -0,0 +1,90 @@ +/** + * The set of optional warehouse SDKs is declared in four places that must agree. + * They had already drifted: `mongodb` was in the drivers workspace and had a + * driver module, but was missing from the binary's externals (so it would be + * bundled instead of installed on demand) and from the published package's + * optional peer dependencies (so `npm ls` never mentioned it). + * + * DRIVER_PACKAGES in packages/drivers/src/resolve.ts is the source of truth; + * this test holds the other three to it. + */ +import { describe, expect, test } from "bun:test" +import * as fs from "fs" +import * as path from "path" +import { DRIVER_PACKAGES } from "@altimateai/drivers/resolve" + +const repoRoot = path.resolve(import.meta.dir, "../../../..") +const driversPkgPath = path.join(repoRoot, "packages/drivers/package.json") +const buildScriptPath = path.join(repoRoot, "packages/opencode/script/build.ts") +const publishScriptPath = path.join(repoRoot, "packages/opencode/script/publish.ts") + +/** Every npm package any driver needs, deduplicated (postgres and redshift share `pg`). */ +const expectedPackages = [...new Set(Object.values(DRIVER_PACKAGES).flat())].sort() + +/** Optional infra externals that are not warehouse drivers. */ +const NON_DRIVER_EXTERNALS = new Set(["keytar", "ssh2", "dockerode"]) + +function readBlock(file: string, startMarker: string, endMarker: string): string { + const source = fs.readFileSync(file, "utf8") + const start = source.indexOf(startMarker) + expect(start, `${startMarker} not found in ${path.basename(file)}`).toBeGreaterThan(-1) + const end = source.indexOf(endMarker, start + startMarker.length) + expect(end, `${endMarker} not found after ${startMarker}`).toBeGreaterThan(-1) + return source.slice(start + startMarker.length, end) +} + +describe("driver catalogue consistency", () => { + test("the drivers workspace declares every driver package as an optional dependency", () => { + const manifest = JSON.parse(fs.readFileSync(driversPkgPath, "utf8")) + const declared = Object.keys(manifest.optionalDependencies ?? {}).sort() + + expect(declared).toEqual(expectedPackages) + }) + + test("the binary build marks every driver package external", () => { + // A driver package missing from `external` is bundled into the binary, which + // freezes it at the release's version and bypasses on-demand install. + const block = readBlock(buildScriptPath, "const optionalExternals = [", "]") + const listed = [...block.matchAll(/"([^"]+)"/g)] + .map((m) => m[1]!) + .filter((name) => !NON_DRIVER_EXTERNALS.has(name)) + .sort() + + expect(listed).toEqual(expectedPackages) + }) + + test("the published package lists every driver package as an optional peer dependency", () => { + const block = readBlock( + publishScriptPath, + "const driverPeerDependencies: Record = {", + "\n}", + ) + const listed = [...block.matchAll(/^\s*"?([@\w\-/.]+)"?\s*:/gm)].map((m) => m[1]!).sort() + + expect(listed).toEqual(expectedPackages) + }) + + test("every driver package resolves to at least one driver module", () => { + for (const driver of Object.keys(DRIVER_PACKAGES)) { + const modulePath = path.join(repoRoot, "packages/drivers/src", `${driver}.ts`) + expect(fs.existsSync(modulePath), `packages/drivers/src/${driver}.ts is missing`).toBe(true) + } + }) + + test("every driver module that loads an optional SDK is in the catalogue", () => { + // Guards the other direction: a new driver file that imports an SDK but is + // never registered would silently have no install path. + const dir = path.join(repoRoot, "packages/drivers/src") + const registered = new Set(Object.keys(DRIVER_PACKAGES)) + const skip = new Set(["index", "types", "normalize", "resolve", "sqlite"]) + + for (const file of fs.readdirSync(dir)) { + if (!file.endsWith(".ts")) continue + const name = file.slice(0, -3) + if (skip.has(name)) continue + const source = fs.readFileSync(path.join(dir, file), "utf8") + if (!source.includes("loadOptionalDriver")) continue + expect(registered.has(name), `${file} loads an optional SDK but is not in DRIVER_PACKAGES`).toBe(true) + } + }) +}) From 9589cc20ed1ed2cfc077ff8dc51541c49985baff Mon Sep 17 00:00:00 2001 From: Haider Date: Thu, 20 Aug 2026 23:18:16 +0530 Subject: [PATCH 2/5] =?UTF-8?q?fix(drivers):=20address=20consensus=20revie?= =?UTF-8?q?w=20=E2=80=94=20additive=20installs,=20half-installed=20package?= =?UTF-8?q?s,=20azure=20auth,=20type=20aliases?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Findings from the multi-model consensus review of #1122. Each was reproduced before being fixed. **Installing a driver deleted the previous one.** `installOptionalDriver` ran `npm install --no-save`, so npm treated every already-installed driver as extraneous and pruned it. Reproduced on npm 11.12.1: installing `mysql2` into a prefix holding `pg` printed `added 12 packages, and removed 14 packages`. A user adding a second warehouse silently lost the first — re-creating the exact defect this module exists to fix. The install now saves to the directory's manifest, which makes it genuinely additive (verified across three drivers). **A half-installed package reported as installed.** `resolveOptionalPackage` fell back to returning the package directory when `require.resolve` failed, so an empty `node_modules/pg` resolved successfully and `isDriverInstalled` was true. `warehouse_install_driver` then answered "already installed, no action taken" and the driver could never be repaired. Resolution now requires a manifest and an entry file that exists, and keeps searching later roots instead of returning a path the caller cannot import. **Azure AD auth used the pattern this PR removes.** `sqlserver.ts` still called `import("@azure/identity" as string)`, which cannot resolve inside the compiled binary, so an installed `@azure/identity` was invisible and every Azure AD login silently fell through to the az CLI. Routed through a new `loadOptionalPackage` (soft variant that returns undefined rather than throwing, since this caller has a real fallback), and declared as a non-driver external. **Six warehouse types never got a readiness note.** `DRIVER_MAP` routes 18 type strings onto 13 drivers, but `driverForWarehouseType` matched only the 12 canonical names, so a connection added as `postgresql`, `mariadb`, `mssql`, `fabric` or `mongo` skipped the check added for #61 — the silent-broken- connection case that issue is about. **Test quality.** The review mutation-tested `isModuleNotFound` by deleting it and all 22 tests still passed; its fixture was never ambiently resolvable, so the branch was unreachable. Applying the same technique to the new fixes showed the first half-installed test was also vacuous. `isModuleNotFound` and `npmInstallArgs` are now exported and pinned directly, and four mutants — always- missing predicate, `--no-save` restored, manifest check removed, bare-directory return — each fail at least one test. Tests: 172 drivers unit (was 162), 4,712 opencode, 140 Docker-backed driver e2e. Typecheck clean. Co-Authored-By: Claude Opus 5 (1M context) --- packages/drivers/src/resolve.ts | 113 +++++++++++++++--- packages/drivers/src/sqlserver.ts | 9 +- packages/drivers/test/resolve-unit.test.ts | 94 +++++++++++++++ packages/opencode/script/build.ts | 6 +- .../tools/warehouse-install-driver.ts | 21 +++- .../test/altimate/driver-catalogue.test.ts | 2 +- 6 files changed, 224 insertions(+), 21 deletions(-) diff --git a/packages/drivers/src/resolve.ts b/packages/drivers/src/resolve.ts index a60cf02011..febf5cd07e 100644 --- a/packages/drivers/src/resolve.ts +++ b/packages/drivers/src/resolve.ts @@ -202,15 +202,72 @@ export function resolveOptionalPackage(specifier: string, roots = driverSearchRo const require = createRequire(pathToFileURL(path.join(process.cwd(), "noop.js")).href) for (const root of roots) { - if (!isDirectory(path.join(root, pkg))) continue + const pkgDir = path.join(root, pkg) + if (!isDirectory(pkgDir)) continue + // A directory without a manifest is not an installed package — an + // interrupted or half-deleted install leaves one behind. Treating it as + // installed made `isDriverInstalled` report true for an empty directory, + // so the install path refused to run and the driver could never be repaired. + if (!fs.existsSync(path.join(pkgDir, "package.json"))) continue + try { return require.resolve(specifier, { paths: [root] }) } catch { - // ESM-only packages have no require-resolvable entry. Hand back the - // package directory and let the dynamic import read its export map. - const dir = path.join(root, specifier) - if (isDirectory(dir) || fs.existsSync(dir)) return dir - return path.join(root, pkg) + // ESM-only packages expose no require-resolvable entry. Read the entry + // out of the manifest instead, and only accept a file that exists. + const entry = entryFromManifest(pkgDir, specifier, pkg) + if (entry) return entry + // Nothing importable here. Keep searching the remaining roots rather + // than returning a path the caller cannot import. + continue + } + } + + return undefined +} + +/** + * Entry file for `specifier` derived from its package manifest, or undefined + * when nothing resolvable exists on disk. + */ +function entryFromManifest(pkgDir: string, specifier: string, pkg: string): string | undefined { + const subpath = specifier.slice(pkg.length).replace(/^\//, "") + + const candidates: string[] = [] + if (subpath) { + // A subpath such as `mysql2/promise` usually maps to a physical file. + candidates.push( + path.join(pkgDir, subpath), + path.join(pkgDir, `${subpath}.js`), + path.join(pkgDir, `${subpath}.mjs`), + path.join(pkgDir, `${subpath}.cjs`), + path.join(pkgDir, subpath, "index.js"), + ) + } else { + try { + const manifest = JSON.parse(fs.readFileSync(path.join(pkgDir, "package.json"), "utf8")) + for (const field of ["module", "main"]) { + const value = manifest?.[field] + if (typeof value === "string") candidates.push(path.join(pkgDir, value)) + } + } catch { + // Unreadable or malformed manifest — fall through to the index probes. + } + candidates.push(path.join(pkgDir, "index.js"), path.join(pkgDir, "index.mjs"), path.join(pkgDir, "index.cjs")) + } + + for (const candidate of candidates) { + try { + const stat = fs.statSync(candidate) + if (stat.isFile()) return candidate + if (stat.isDirectory()) { + for (const index of ["index.js", "index.mjs", "index.cjs"]) { + const nested = path.join(candidate, index) + if (fs.existsSync(nested)) return nested + } + } + } catch { + // Candidate does not exist; try the next one. } } @@ -250,7 +307,7 @@ export async function loadOptionalDriver(driver: DriverName, specifier: string): } /** True when `error` means the module could not be resolved, not that it failed while loading. */ -function isModuleNotFound(error: unknown): boolean { +export function isModuleNotFound(error: unknown): boolean { const code = (error as { code?: string } | null)?.code if (code === "ERR_MODULE_NOT_FOUND" || code === "MODULE_NOT_FOUND") return true const message = error instanceof Error ? error.message : String(error) @@ -264,6 +321,25 @@ function loadFailure(driver: DriverName, where: string, error: unknown): Error { ) } +/** + * Import an optional package that is not a warehouse driver, returning + * undefined when it is unavailable. + * + * Same bunfs problem as the drivers — a bare specifier cannot resolve inside + * the compiled binary — but these callers have a legitimate fallback and must + * not be handed an exception. + */ +export async function loadOptionalPackage(specifier: string): Promise { + try { + return await import(/* @vite-ignore */ specifier) + } catch (ambientError) { + if (!isModuleNotFound(ambientError)) throw ambientError + const resolved = resolveOptionalPackage(specifier) + if (!resolved) return undefined + return await import(/* @vite-ignore */ pathToFileURL(resolved).href) + } +} + /** True when `driver`'s packages are all resolvable right now. */ export function isDriverInstalled(driver: DriverName, roots = driverSearchRoots()): boolean { return DRIVER_PACKAGES[driver].every((pkg) => resolveOptionalPackage(pkg, roots) !== undefined) @@ -306,11 +382,24 @@ function runNpm(args: string[], cwd: string, timeoutMs: number): Promise<{ code: }) } +/** + * npm arguments for installing `packages` into the managed driver directory. + * + * `--save` is required, not incidental: with `--no-save` npm treats already + * installed drivers as extraneous and prunes them on the next install. + */ +export function npmInstallArgs(packages: readonly string[]): string[] { + return ["install", "--save", "--no-audit", "--no-fund", "--loglevel=error", ...packages] +} + /** * Install a driver's SDK into the managed driver directory. * - * Installs are additive — `--no-save` against a private package.json — so - * installing a second driver never removes the first. + * Installs must be recorded in the directory's own package.json. With + * `--no-save`, npm treats every previously installed driver as extraneous and + * prunes it: installing MySQL deleted Postgres, re-creating the very bug this + * module exists to fix. Verified on npm 11.12.1 — + * `added 12 packages, and removed 14 packages`. */ export async function installOptionalDriver( driver: DriverName, @@ -345,11 +434,7 @@ export async function installOptionalDriver( } } - const { code, output } = await runNpm( - ["install", "--no-save", "--no-audit", "--no-fund", "--loglevel=error", ...packages], - dir, - options.timeoutMs ?? 180_000, - ) + const { code, output } = await runNpm(npmInstallArgs(packages), dir, options.timeoutMs ?? 180_000) if (code === 127) { return { diff --git a/packages/drivers/src/sqlserver.ts b/packages/drivers/src/sqlserver.ts index 973f6a6a15..a3aded0eca 100644 --- a/packages/drivers/src/sqlserver.ts +++ b/packages/drivers/src/sqlserver.ts @@ -3,7 +3,7 @@ */ import type { ConnectionConfig, Connector, ConnectorResult, ExecuteOptions, SchemaColumn } from "./types" -import { loadOptionalDriver } from "./resolve" +import { loadOptionalDriver, loadOptionalPackage } from "./resolve" // --------------------------------------------------------------------------- // Azure AD helpers — cache + resource URL resolution @@ -162,7 +162,12 @@ export async function connect(config: ConnectionConfig): Promise { // who don't use Azure AD don't need to install it. Typed `any` (via a non-literal // specifier) so it compiles regardless of which @azure/identity version (if any) is // installed; the runtime API is resolved from the user's installed package. - const azureIdentity: any = await import("@azure/identity" as string) + // Resolved through the shared optional-package loader: a bare + // specifier does not resolve inside the compiled binary, so an + // installed @azure/identity was invisible and every Azure AD login + // silently fell through to the az CLI path. + const azureIdentity: any = await loadOptionalPackage("@azure/identity") + if (!azureIdentity) throw new Error("@azure/identity is not installed") const credential = new azureIdentity.DefaultAzureCredential( config.azure_client_id ? { managedIdentityClientId: config.azure_client_id as string } diff --git a/packages/drivers/test/resolve-unit.test.ts b/packages/drivers/test/resolve-unit.test.ts index ac1ae26b3c..28a79d2c6d 100644 --- a/packages/drivers/test/resolve-unit.test.ts +++ b/packages/drivers/test/resolve-unit.test.ts @@ -16,6 +16,8 @@ import * as path from "path" import { DRIVER_PACKAGES, + isModuleNotFound, + npmInstallArgs, DriverNotInstalledError, driverInstallDir, driverLabel, @@ -309,3 +311,95 @@ describe("driver catalogue", () => { expect(Object.keys(DRIVER_PACKAGES).sort()).toEqual(expected) }) }) + +// --------------------------------------------------------------------------- +// Regression cover for the consensus-review criticals (PR #1122) +// --------------------------------------------------------------------------- + +describe("installOptionalDriver arguments", () => { + test("saves to the manifest so installs are additive", () => { + // Verified on npm 11.12.1: with `--no-save`, installing mysql2 into a prefix + // that already had pg printed "added 12 packages, and removed 14 packages". + // Every previously installed driver is pruned as extraneous, re-creating the + // exact "driver not installed" bug this module exists to fix. + const args = npmInstallArgs(["mysql2"]) + + expect(args).toContain("--save") + expect(args).not.toContain("--no-save") + }) + + test("passes every requested package through", () => { + expect(npmInstallArgs(["pg", "@types/pg"]).slice(-2)).toEqual(["pg", "@types/pg"]) + }) +}) + +describe("isModuleNotFound", () => { + // Pinned directly: deleting this predicate left the behavioural tests passing, + // because their fixtures are not ambiently resolvable and so never reach it. + test("recognises the Node resolution error code", () => { + const err = Object.assign(new Error("nope"), { code: "ERR_MODULE_NOT_FOUND" }) + expect(isModuleNotFound(err)).toBe(true) + }) + + test("recognises the CommonJS resolution error code", () => { + expect(isModuleNotFound(Object.assign(new Error("nope"), { code: "MODULE_NOT_FOUND" }))).toBe(true) + }) + + test("recognises the message Bun emits inside bunfs", () => { + expect(isModuleNotFound(new Error("Cannot find package 'pg' from '/$bunfs/root/index.js'"))).toBe(true) + expect(isModuleNotFound(new Error("Cannot find module 'mysql2/promise'"))).toBe(true) + }) + + test("does NOT classify a load-time failure as missing", () => { + // The distinction that matters: a package that resolves but throws while + // initialising (broken native binding) must not be reported as absent. + expect(isModuleNotFound(new Error("dlopen failed: wrong architecture"))).toBe(false) + expect(isModuleNotFound(new TypeError("x is not a function"))).toBe(false) + expect(isModuleNotFound(undefined)).toBe(false) + }) +}) + +describe("half-installed packages", () => { + test("an empty package directory does not count as installed", () => { + // An interrupted or half-deleted install leaves a bare directory behind. + // Counting it as installed made warehouse_install_driver answer "already + // installed, no action taken", so the driver could never be repaired. + const root = path.join(tmpRoot, "node_modules") + fs.mkdirSync(path.join(root, "pg"), { recursive: true }) + + expect(resolveOptionalPackage("pg", [root])).toBeUndefined() + expect(isDriverInstalled("postgres", [root])).toBe(false) + }) + + test("a directory with a manifest but no entry file does not count as installed", () => { + const root = path.join(tmpRoot, "node_modules") + const dir = path.join(root, "oracledb") + fs.mkdirSync(dir, { recursive: true }) + fs.writeFileSync(path.join(dir, "package.json"), JSON.stringify({ name: "oracledb", main: "index.js" })) + + expect(resolveOptionalPackage("oracledb", [root])).toBeUndefined() + }) + + test("a directory with no manifest is not a package, even if a subpath file exists", () => { + // Subpath probing looks for physical files (mysql2/promise.js), so without + // the manifest check a bare directory holding one would resolve as an + // installed package. + const root = path.join(tmpRoot, "node_modules") + fs.mkdirSync(path.join(root, "mysql2"), { recursive: true }) + fs.writeFileSync(path.join(root, "mysql2", "promise.js"), "module.exports = {}") + + expect(resolveOptionalPackage("mysql2/promise", [root])).toBeUndefined() + }) + + test("keeps searching later roots when an earlier one is half-installed", () => { + const broken = path.join(tmpRoot, "broken", "node_modules") + fs.mkdirSync(path.join(broken, "pg"), { recursive: true }) + const good = path.join(tmpRoot, "good") + installFakePackage(good, "pg", "module.exports = { which: 'good' }") + + const resolved = resolveOptionalPackage("pg", [broken, path.join(good, "node_modules")]) + + expect(resolved).toBeDefined() + expect(resolved!.includes(path.join("good", "node_modules"))).toBe(true) + }) +}) diff --git a/packages/opencode/script/build.ts b/packages/opencode/script/build.ts index 9f135ee8d5..a7913456c3 100755 --- a/packages/opencode/script/build.ts +++ b/packages/opencode/script/build.ts @@ -245,8 +245,10 @@ const optionalExternals = [ "pg", "snowflake-sdk", "@google-cloud/bigquery", "@databricks/sql", "mysql2", "mssql", "oracledb", "duckdb", "mongodb", "@clickhouse/client", "trino-client", - // Optional infra packages — native addons or heavy optional deps - "keytar", "ssh2", "dockerode", + // Optional infra packages — native addons or heavy optional deps. + // @azure/identity is dynamically imported by the sqlserver driver for Azure + // AD auth; it resolves through the same on-disk loader as the drivers. + "keytar", "ssh2", "dockerode", "@azure/identity", ] const binaries: Record = {} diff --git a/packages/opencode/src/altimate/tools/warehouse-install-driver.ts b/packages/opencode/src/altimate/tools/warehouse-install-driver.ts index 020cbd0e6b..f901f234d0 100644 --- a/packages/opencode/src/altimate/tools/warehouse-install-driver.ts +++ b/packages/opencode/src/altimate/tools/warehouse-install-driver.ts @@ -99,13 +99,30 @@ export const WarehouseInstallDriverTool = Tool.define("warehouse_install_driver" }, }) +/** + * Aliases the connection registry accepts for a warehouse type. + * + * `DRIVER_MAP` in native/connections/registry.ts routes 18 type strings onto + * 13 drivers. Matching only the 12 canonical names meant a connection added as + * `postgresql`, `mariadb`, `mssql`, `fabric` or `mongo` never got a readiness + * note — the exact silent-broken-connection case #61 is about. + */ +const DRIVER_TYPE_ALIASES: Record = { + postgresql: "postgres", + mariadb: "mysql", + mssql: "sqlserver", + fabric: "sqlserver", + mongo: "mongodb", +} + /** * Driver name for a warehouse config `type`, or undefined when the type needs - * no optional SDK (sqlite ships with the runtime). + * no optional SDK (sqlite ships with the runtime) or is unrecognised. */ export function driverForWarehouseType(type: string): DriverName | undefined { const normalized = type.trim().toLowerCase() - return (DRIVER_NAMES as readonly string[]).includes(normalized) ? (normalized as DriverName) : undefined + if ((DRIVER_NAMES as readonly string[]).includes(normalized)) return normalized as DriverName + return DRIVER_TYPE_ALIASES[normalized] } export { DRIVER_PACKAGES, driverInstallDir, isDriverInstalled, installOptionalDriver, driverLabel } diff --git a/packages/opencode/test/altimate/driver-catalogue.test.ts b/packages/opencode/test/altimate/driver-catalogue.test.ts index 5e6196fcc0..e6b476b5cb 100644 --- a/packages/opencode/test/altimate/driver-catalogue.test.ts +++ b/packages/opencode/test/altimate/driver-catalogue.test.ts @@ -22,7 +22,7 @@ const publishScriptPath = path.join(repoRoot, "packages/opencode/script/publish. const expectedPackages = [...new Set(Object.values(DRIVER_PACKAGES).flat())].sort() /** Optional infra externals that are not warehouse drivers. */ -const NON_DRIVER_EXTERNALS = new Set(["keytar", "ssh2", "dockerode"]) +const NON_DRIVER_EXTERNALS = new Set(["keytar", "ssh2", "dockerode", "@azure/identity"]) function readBlock(file: string, startMarker: string, endMarker: string): string { const source = fs.readFileSync(file, "utf8") From a698abc39d33cb5c35d43e8e78cce357762c2f82 Mon Sep 17 00:00:00 2001 From: Haider Date: Thu, 20 Aug 2026 23:54:40 +0530 Subject: [PATCH 3/5] =?UTF-8?q?fix(drivers):=20address=20PR=20bot=20review?= =?UTF-8?q?=20=E2=80=94=20transitive=20deps,=20load=20probe,=20telemetry,?= =?UTF-8?q?=20quoting?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit and cubic-dev-ai findings on #1122. Each verified before fixing. **A missing transitive dependency read as a missing driver.** `isModuleNotFound` matched any "Cannot find module/package" text, but a driver whose own dependency tree is incomplete raises exactly that shape — observed for real inside a compiled binary as `Cannot find package 'pg-protocol' from '.../pg/lib/ connection.js'`, where pg itself is installed. The predicate now takes the specifier and, when the runtime names the module it could not find, only counts a name matching what was asked for. Without a specifier it stays conservative. **A broken install could not be repaired.** `warehouse_install_driver` gated on `isDriverInstalled`, which only asks whether the package resolves. A copy that resolves but throws on import — a native addon for another platform, or a half-written install — answered "already installed", so the one command that could fix it declined to run. It now probes an actual load. **Failed installs were recorded as successes.** `Tool` reads `metadata.success === false` as its soft-failure signal (tool/tool.ts), and every sibling warehouse tool sets it. This tool omitted it, so a failed install skipped failure telemetry entirely. **Install hints broke on paths containing spaces.** The printed `npm install --prefix ` is meant to be pasted; an unquoted path split and npm received the wrong prefix. Added `shellQuote` and applied it at both sites. **Two test-quality fixes.** CodeRabbit and cubic independently flagged that "does not fall back when an ambiently-resolvable package fails to load" never reaches the branch it names — its fixture is not ambiently resolvable, so the disk fallback handles it first. Renamed to what it actually proves, with the ambient branch now pinned directly through `isModuleNotFound`. Separately, a comment claimed the catalogue test kept the tool's `DRIVER_NAMES` and alias map in step with `DRIVER_PACKAGES`; no such test existed. It does now, and it also asserts every `DRIVER_MAP` type resolves to an installable driver — removing the alias map fails it, which is the #61 gap this PR set out to close. Tests: 177 drivers unit (was 172), 4,714 opencode. Typecheck clean, 0 lint errors. Co-Authored-By: Claude Opus 5 (1M context) --- packages/drivers/src/resolve.ts | 33 ++++++++++--- packages/drivers/test/resolve-unit.test.ts | 46 +++++++++++++++++-- .../src/altimate/tools/warehouse-add.ts | 3 +- .../tools/warehouse-install-driver.ts | 37 +++++++++++++-- .../test/altimate/driver-catalogue.test.ts | 44 ++++++++++++++++++ 5 files changed, 148 insertions(+), 15 deletions(-) diff --git a/packages/drivers/src/resolve.ts b/packages/drivers/src/resolve.ts index febf5cd07e..2f96c41f5b 100644 --- a/packages/drivers/src/resolve.ts +++ b/packages/drivers/src/resolve.ts @@ -25,6 +25,11 @@ import { createRequire } from "node:module" import { pathToFileURL } from "node:url" import { spawn } from "node:child_process" +/** Quote a path for inclusion in a copy-pasteable shell command. */ +export function shellQuote(value: string): string { + return /^[A-Za-z0-9_./@:-]+$/.test(value) ? value : `'${value.replace(/'/g, `'\\''`)}'` +} + /** Every driver in this package and the npm packages it needs at runtime. */ export const DRIVER_PACKAGES = { postgres: ["pg"], @@ -80,7 +85,7 @@ export class DriverNotInstalledError extends Error { super( `${label} driver not installed.\n` + `Install it with the warehouse_install_driver tool, or run:\n` + - ` npm install --prefix ${driverInstallDir()} ${packages.join(" ")}\n` + + ` npm install --prefix ${shellQuote(driverInstallDir())} ${packages.join(" ")}\n` + `Searched ${searched.length} location${searched.length === 1 ? "" : "s"}: ${searched.join(", ")}`, ) this.name = "DriverNotInstalledError" @@ -290,7 +295,7 @@ export async function loadOptionalDriver(driver: DriverName, specifier: string): // Only a resolution failure means "look elsewhere". A package that resolves // ambiently but throws while loading is a broken install, and re-reporting // it as missing would send the user to install what they already have. - if (!isModuleNotFound(ambientError)) throw loadFailure(driver, specifier, ambientError) + if (!isModuleNotFound(ambientError, specifier)) throw loadFailure(driver, specifier, ambientError) const roots = driverSearchRoots() const resolved = resolveOptionalPackage(specifier, roots) @@ -306,11 +311,27 @@ export async function loadOptionalDriver(driver: DriverName, specifier: string): } } -/** True when `error` means the module could not be resolved, not that it failed while loading. */ -export function isModuleNotFound(error: unknown): boolean { +/** + * True when `error` means **`specifier` itself** could not be resolved. + * + * A package that loads but whose own dependency tree is incomplete raises the + * same error shape — `Cannot find package 'pg-protocol' from '…/pg/lib/ + * connection.js'` — for a driver that is very much installed. Treating that as + * "not installed" sends the user to reinstall something already present. So + * when the runtime names the module it could not find, only a name matching + * what we asked for counts as missing. + */ +export function isModuleNotFound(error: unknown, specifier?: string): boolean { + const message = error instanceof Error ? error.message : String(error) + const named = /Cannot find (?:module|package)\s+['"]([^'"]+)['"]/i.exec(message) + + if (named && specifier) { + const missing = named[1]! + return missing === specifier || missing === packageNameOf(specifier) + } + const code = (error as { code?: string } | null)?.code if (code === "ERR_MODULE_NOT_FOUND" || code === "MODULE_NOT_FOUND") return true - const message = error instanceof Error ? error.message : String(error) return /Cannot find (module|package)/i.test(message) } @@ -333,7 +354,7 @@ export async function loadOptionalPackage(specifier: string): Promise { expect(err.message).toContain("Searched") }) - test("does not fall back when an ambiently-resolvable package fails to load", async () => { - // A package that resolves but throws on import is broken, not absent. - // Reporting it as "not installed" sends the user to install what they have. + test("reports a disk-resolved package that throws on import as a load failure", async () => { + // Named for what it actually exercises: the fixture is not ambiently + // resolvable, so this covers the on-disk load path, not the ambient rethrow. + // The ambient branch is pinned directly in the isModuleNotFound tests below. const broken = path.join(tmpRoot, "ambient") installFakePackage(broken, "altimate-ambient-broken", "throw new Error('boom')") process.env["ALTIMATE_DRIVER_DIR"] = broken @@ -350,6 +352,26 @@ describe("isModuleNotFound", () => { expect(isModuleNotFound(new Error("Cannot find module 'mysql2/promise'"))).toBe(true) }) + test("a missing transitive dependency is NOT the driver going missing", () => { + // Observed for real when importing pg's entry inside a compiled binary: + // `Cannot find package 'pg-protocol' from '.../pg/lib/connection.js'`. + // pg is installed; its dependency tree is incomplete. Classifying that as + // "not installed" sends the user to reinstall what they already have. + const transitive = new Error("Cannot find package 'pg-protocol' from '/x/node_modules/pg/lib/connection.js'") + + expect(isModuleNotFound(transitive, "pg")).toBe(false) + // Same error with no specifier context stays conservative. + expect(isModuleNotFound(transitive)).toBe(true) + }) + + test("the driver's own absence still counts as missing", () => { + const own = new Error("Cannot find package 'pg' from '/$bunfs/root/index.js'") + + expect(isModuleNotFound(own, "pg")).toBe(true) + // Subpath specifiers resolve against their package name. + expect(isModuleNotFound(new Error("Cannot find module 'mysql2'"), "mysql2/promise")).toBe(true) + }) + test("does NOT classify a load-time failure as missing", () => { // The distinction that matters: a package that resolves but throws while // initialising (broken native binding) must not be reported as absent. @@ -403,3 +425,21 @@ describe("half-installed packages", () => { expect(resolved!.includes(path.join("good", "node_modules"))).toBe(true) }) }) + +describe("shellQuote", () => { + test("leaves ordinary paths alone", () => { + expect(shellQuote("/Users/x/.local/share/altimate-code/drivers")).toBe( + "/Users/x/.local/share/altimate-code/drivers", + ) + }) + + test("quotes a path with spaces so the printed command is copy-pasteable", () => { + // The install hint is meant to be pasted; an unquoted path with spaces + // splits and npm receives the wrong --prefix. + expect(shellQuote("/Users/x/My Drive/drivers")).toBe("'/Users/x/My Drive/drivers'") + }) + + test("escapes embedded single quotes", () => { + expect(shellQuote("/tmp/it's here")).toBe(`'/tmp/it'\\''s here'`) + }) +}) diff --git a/packages/opencode/src/altimate/tools/warehouse-add.ts b/packages/opencode/src/altimate/tools/warehouse-add.ts index 07e3265c25..d31cb2f0a1 100644 --- a/packages/opencode/src/altimate/tools/warehouse-add.ts +++ b/packages/opencode/src/altimate/tools/warehouse-add.ts @@ -6,6 +6,7 @@ import { PostConnectSuggestions } from "./post-connect-suggestions" import { Telemetry } from "../../telemetry" // altimate_change end // altimate_change start — report driver readiness when adding a warehouse +import { shellQuote } from "@altimateai/drivers/resolve" import { driverForWarehouseType, driverInstallDir, @@ -166,7 +167,7 @@ function driverReadinessNote(type: string): string { return ( `\n\nNOTE: the ${driverLabel(driver)} driver is not installed yet, so this connection cannot be used until it is.\n` + `Run the warehouse_install_driver tool with driver="${driver}", or install it manually:\n` + - ` npm install --prefix ${driverInstallDir()} ${packages}` + ` npm install --prefix ${shellQuote(driverInstallDir())} ${packages}` ) } catch { // A driver probe must never fail an add whose configuration was stored. diff --git a/packages/opencode/src/altimate/tools/warehouse-install-driver.ts b/packages/opencode/src/altimate/tools/warehouse-install-driver.ts index f901f234d0..50a6d1a56d 100644 --- a/packages/opencode/src/altimate/tools/warehouse-install-driver.ts +++ b/packages/opencode/src/altimate/tools/warehouse-install-driver.ts @@ -2,6 +2,7 @@ import z from "zod" import { Tool } from "../../tool/tool" import { DRIVER_PACKAGES, + loadOptionalDriver, driverInstallDir, driverLabel, installOptionalDriver, @@ -10,7 +11,8 @@ import { } from "@altimateai/drivers/resolve" // Listed literally rather than derived from DRIVER_PACKAGES so zod infers a -// concrete union; the catalogue test in packages/drivers keeps the two in step. +// concrete literal union. driver-catalogue.test.ts pins this list, and the alias +// map below, against DRIVER_PACKAGES and the registry's DRIVER_MAP. const DRIVER_NAMES = [ "postgres", "redshift", @@ -33,6 +35,8 @@ const DRIVER_NAMES = [ */ interface InstallDriverMetadata { [key: string]: any + /** Read by Tool as the soft-failure signal (tool/tool.ts). */ + success: boolean driver: DriverName installed: boolean alreadyPresent: boolean @@ -61,11 +65,15 @@ export const WarehouseInstallDriverTool = Tool.define("warehouse_install_driver" const label = driverLabel(driver) const dir = driverInstallDir() - if (isDriverInstalled(driver)) { + // Resolution is not usability. A package that resolves but throws on import — + // a native addon built for another platform, or a half-written copy — used to + // report "already installed", so the one command that could repair it refused + // to run. Probe an actual load and only decline when it succeeds. + if (isDriverInstalled(driver) && (await driverLoads(driver))) { return { title: `${label} driver: already installed`, - metadata: { driver, installed: true, alreadyPresent: true, dir }, - output: `The ${label} driver is already installed and resolvable. No action taken.`, + metadata: { success: true, driver, installed: true, alreadyPresent: true, dir }, + output: `The ${label} driver is already installed and loads correctly. No action taken.`, } } @@ -76,6 +84,7 @@ export const WarehouseInstallDriverTool = Tool.define("warehouse_install_driver" return { title: `${label} driver: install FAILED`, metadata: { + success: false, driver, installed: false, alreadyPresent: false, @@ -91,7 +100,7 @@ export const WarehouseInstallDriverTool = Tool.define("warehouse_install_driver" return { title: `${label} driver: installed`, - metadata: { driver, installed: true, alreadyPresent: false, dir: result.dir }, + metadata: { success: true, driver, installed: true, alreadyPresent: false, dir: result.dir }, output: `Installed the ${label} driver (${packages}) into ${result.dir}.\n` + `It is available now — connections using ${driver} will work without restarting the session.`, @@ -127,3 +136,21 @@ export function driverForWarehouseType(type: string): DriverName | undefined { export { DRIVER_PACKAGES, driverInstallDir, isDriverInstalled, installOptionalDriver, driverLabel } export type { DriverName } + +/** + * True when every package the driver needs actually imports. + * + * Separates "resolvable" from "usable". Either failure mode — genuinely absent, + * or present but unloadable — means the install should proceed, so both answer + * false; the distinction is already reported in the error text the user sees. + */ +async function driverLoads(driver: DriverName): Promise { + for (const pkg of DRIVER_PACKAGES[driver]) { + try { + await loadOptionalDriver(driver, pkg) + } catch { + return false + } + } + return true +} diff --git a/packages/opencode/test/altimate/driver-catalogue.test.ts b/packages/opencode/test/altimate/driver-catalogue.test.ts index e6b476b5cb..eabbb1f583 100644 --- a/packages/opencode/test/altimate/driver-catalogue.test.ts +++ b/packages/opencode/test/altimate/driver-catalogue.test.ts @@ -12,6 +12,7 @@ import { describe, expect, test } from "bun:test" import * as fs from "fs" import * as path from "path" import { DRIVER_PACKAGES } from "@altimateai/drivers/resolve" +import { driverForWarehouseType } from "../../src/altimate/tools/warehouse-install-driver" const repoRoot = path.resolve(import.meta.dir, "../../../..") const driversPkgPath = path.join(repoRoot, "packages/drivers/package.json") @@ -24,6 +25,14 @@ const expectedPackages = [...new Set(Object.values(DRIVER_PACKAGES).flat())].sor /** Optional infra externals that are not warehouse drivers. */ const NON_DRIVER_EXTERNALS = new Set(["keytar", "ssh2", "dockerode", "@azure/identity"]) +/** Names inside a `const X = [ "a", "b" ] as const` literal. */ +function readLiteralList(source: string, marker: string): string[] { + const start = source.indexOf(marker) + expect(start, `${marker} not found`).toBeGreaterThan(-1) + const end = source.indexOf("]", start) + return [...source.slice(start + marker.length, end).matchAll(/"([^"]+)"/g)].map((m) => m[1]!) +} + function readBlock(file: string, startMarker: string, endMarker: string): string { const source = fs.readFileSync(file, "utf8") const start = source.indexOf(startMarker) @@ -87,4 +96,39 @@ describe("driver catalogue consistency", () => { expect(registered.has(name), `${file} loads an optional SDK but is not in DRIVER_PACKAGES`).toBe(true) } }) + + test("the install tool's DRIVER_NAMES matches DRIVER_PACKAGES", () => { + // The tool declares its zod enum literally so the parameter type is a + // concrete union. Nothing pinned it to the catalogue until now, so a new + // driver could be installable by the resolver but unreachable by the tool. + const toolSource = fs.readFileSync( + path.join(repoRoot, "packages/opencode/src/altimate/tools/warehouse-install-driver.ts"), + "utf8", + ) + const names = [...readLiteralList(toolSource, "const DRIVER_NAMES = [")].sort() + + expect(names).toEqual(Object.keys(DRIVER_PACKAGES).sort()) + }) + + test("every registry warehouse type maps to a driver the tool can install", () => { + // DRIVER_MAP accepts aliases (postgresql, mariadb, mssql, fabric, mongo). + // Each must resolve through driverForWarehouseType or a connection added + // under that alias silently skips the readiness check added for #61. + const registry = fs.readFileSync( + path.join(repoRoot, "packages/opencode/src/altimate/native/connections/registry.ts"), + "utf8", + ) + const mapBlock = registry.slice( + registry.indexOf("const DRIVER_MAP: Record = {"), + registry.indexOf("}", registry.indexOf("const DRIVER_MAP: Record = {")), + ) + const types = [...mapBlock.matchAll(/^\s*([a-z0-9]+)\s*:/gm)].map((m) => m[1]!) + + expect(types.length).toBeGreaterThan(12) + for (const type of types) { + // sqlite is bundled with the runtime and needs no optional SDK. + if (type === "sqlite") continue + expect(driverForWarehouseType(type), `registry type "${type}" has no installable driver`).toBeDefined() + } + }) }) From e2c284594bceadddf8bc8c5ff8873dcd4b1fa37b Mon Sep 17 00:00:00 2001 From: Haider Date: Fri, 21 Aug 2026 00:42:24 +0530 Subject: [PATCH 4/5] fix(drivers): make the repair path actually repair, and stop a broken copy shadowing a good one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second bot round on #1122. The headline finding is that my previous commit's repair path did not work. **The reinstall never ran.** `warehouse_install_driver` gained a load probe so a resolvable-but-unloadable driver would be rebuilt — but `installOptionalDriver` short-circuits on `isDriverInstalled`, a resolution-only check, and returned `installed: true, alreadyPresent: true` without invoking npm. The probe changed nothing and the tool reported a success it had not performed. cubic-dev-ai flagged this three times over. Installs now take a `force` option for callers that know something the resolution check cannot, and the tool passes it exactly when the package resolves but fails to import. **A broken ambient copy hid a healthy managed one.** After an ambient import failed with anything other than a resolution error, the loader rethrew immediately, so installing a good copy into the managed directory could never take effect. Resolution now continues to the search roots, and the ambient error is only surfaced when nothing else loads. **Concurrent installs could corrupt the managed directory.** Two installs running npm against one manifest are serialized per target directory. **Windows install hints were unusable.** `shellQuote` emitted POSIX single quotes, which cmd.exe and PowerShell do not understand, so any path containing a space produced a command that could not be run. It is now platform-aware. **Test honesty.** The catalogue test asserted only that a registry type resolved to *something*; a stale alias naming an uninstallable driver would have passed. It now checks membership in DRIVER_PACKAGES. More importantly, the first attempt at the ambient-shadowing test was vacuous in the same way three earlier tests were — its fixture was not ambiently resolvable, so the branch under test was never reached, and the mutant survived. It now writes a genuinely ambient-resolvable fixture into this package's node_modules and removes it afterwards. Mutants for all three fixes were confirmed to fail. Tests: 182 drivers unit (was 177), 4,714 opencode. Typecheck clean. Co-Authored-By: Claude Opus 5 (1M context) --- packages/drivers/src/resolve.ts | 61 ++++++++++--- packages/drivers/test/resolve-unit.test.ts | 90 +++++++++++++++++++ .../tools/warehouse-install-driver.ts | 6 +- .../test/altimate/driver-catalogue.test.ts | 6 +- 4 files changed, 148 insertions(+), 15 deletions(-) diff --git a/packages/drivers/src/resolve.ts b/packages/drivers/src/resolve.ts index 2f96c41f5b..8952aeec39 100644 --- a/packages/drivers/src/resolve.ts +++ b/packages/drivers/src/resolve.ts @@ -25,8 +25,16 @@ import { createRequire } from "node:module" import { pathToFileURL } from "node:url" import { spawn } from "node:child_process" -/** Quote a path for inclusion in a copy-pasteable shell command. */ -export function shellQuote(value: string): string { +/** + * Quote a path for a copy-pasteable shell command on the current platform. + * + * cmd.exe and PowerShell do not understand POSIX single-quoting, so a path with + * spaces printed the POSIX way is not runnable on Windows. + */ +export function shellQuote(value: string, platform: NodeJS.Platform = process.platform): string { + if (platform === "win32") { + return /^[A-Za-z0-9_.:\\/@-]+$/.test(value) ? value : `"${value.replace(/"/g, '""')}"` + } return /^[A-Za-z0-9_./@:-]+$/.test(value) ? value : `'${value.replace(/'/g, `'\\''`)}'` } @@ -292,21 +300,23 @@ export async function loadOptionalDriver(driver: DriverName, specifier: string): try { return await import(/* @vite-ignore */ specifier) } catch (ambientError) { - // Only a resolution failure means "look elsewhere". A package that resolves - // ambiently but throws while loading is a broken install, and re-reporting - // it as missing would send the user to install what they already have. - if (!isModuleNotFound(ambientError, specifier)) throw loadFailure(driver, specifier, ambientError) - + const ambientBroken = !isModuleNotFound(ambientError, specifier) const roots = driverSearchRoots() const resolved = resolveOptionalPackage(specifier, roots) - if (!resolved) throw new DriverNotInstalledError(driver, DRIVER_PACKAGES[driver], roots) + + if (!resolved) { + // A broken ambient copy is a load failure, not an absence. + if (ambientBroken) throw loadFailure(driver, specifier, ambientError) + throw new DriverNotInstalledError(driver, DRIVER_PACKAGES[driver], roots) + } try { return await import(/* @vite-ignore */ pathToFileURL(resolved).href) } catch (loadError) { // On disk but will not load — a half-installed copy, or a native addon - // built for another platform. - throw loadFailure(driver, resolved, loadError) + // built for another platform. When an ambient copy was also broken, + // report that one: it is the copy the runtime would normally pick. + throw loadFailure(driver, ambientBroken ? specifier : resolved, ambientBroken ? ambientError : loadError) } } } @@ -424,15 +434,42 @@ export function npmInstallArgs(packages: readonly string[]): string[] { */ export async function installOptionalDriver( driver: DriverName, - options: { timeoutMs?: number } = {}, + options: { timeoutMs?: number; force?: boolean } = {}, ): Promise { const packages = DRIVER_PACKAGES[driver] const dir = driverInstallDir() - if (isDriverInstalled(driver)) { + // `force` exists because the caller may know something this check cannot: + // that the package resolves but does not import. Without it the early return + // below reported success for a copy it never rebuilt, so the repair path was + // unreachable no matter what the caller had detected. + if (!options.force && isDriverInstalled(driver)) { return { driver, packages, dir, installed: true, alreadyPresent: true } } + // Serialize per directory: concurrent npm runs against one manifest can leave + // the managed directory inconsistent. + const pending = installsInFlight.get(dir) + if (pending) await pending.catch(() => {}) + const run = performInstall(driver, packages, dir, options) + installsInFlight.set(dir, run) + try { + return await run + } finally { + if (installsInFlight.get(dir) === run) installsInFlight.delete(dir) + } +} + +/** In-flight installs keyed by target directory (see the note above). */ +const installsInFlight = new Map>() + +async function performInstall( + driver: DriverName, + packages: readonly string[], + dir: string, + options: { timeoutMs?: number; force?: boolean }, +): Promise { + try { fs.mkdirSync(dir, { recursive: true }) const manifest = path.join(dir, "package.json") diff --git a/packages/drivers/test/resolve-unit.test.ts b/packages/drivers/test/resolve-unit.test.ts index 0e488e3180..7db6a92b05 100644 --- a/packages/drivers/test/resolve-unit.test.ts +++ b/packages/drivers/test/resolve-unit.test.ts @@ -19,6 +19,7 @@ import { isModuleNotFound, npmInstallArgs, shellQuote, + installOptionalDriver, DriverNotInstalledError, driverInstallDir, driverLabel, @@ -443,3 +444,92 @@ describe("shellQuote", () => { expect(shellQuote("/tmp/it's here")).toBe(`'/tmp/it'\\''s here'`) }) }) + +describe("repairing a broken install", () => { + test("force skips the resolution-only early return", async () => { + // The bug this pins: installOptionalDriver short-circuited on + // isDriverInstalled, a resolution-only check. A caller that had detected a + // present-but-unloadable copy asked for a reinstall and got back + // `installed: true, alreadyPresent: true` with npm never run — so the + // repair path was unreachable. + installFakePackage(tmpRoot, "oracledb", "module.exports = {}") + process.env["ALTIMATE_DRIVER_DIR"] = tmpRoot + + // Without force: recognised as installed, returns immediately. + const asIs = await installOptionalDriver("oracle") + expect(asIs.alreadyPresent).toBe(true) + expect(asIs.installed).toBe(true) + + // With force: must NOT take that early return. npm is unavailable for a + // package that does not exist, so this reaches a real attempt and reports + // failure rather than a fictitious success. + const forced = await installOptionalDriver("oracle", { force: true, timeoutMs: 15_000 }) + expect(forced.alreadyPresent).toBe(false) + }, 60_000) +}) + +describe("a broken ambient copy does not hide a good one on disk", () => { + // This one needs a fixture the ambient resolver can genuinely find, so it is + // written into this package's own node_modules and removed afterwards. Every + // cheaper version of this test was vacuous: a fixture that only exists under + // ALTIMATE_DRIVER_DIR never reaches the ambient branch at all. + const AMBIENT = "altimate-ambient-throws" + const ambientDir = path.join(import.meta.dir, "..", "node_modules", AMBIENT) + + function installAmbientBroken() { + fs.mkdirSync(ambientDir, { recursive: true }) + fs.writeFileSync(path.join(ambientDir, "package.json"), JSON.stringify({ name: AMBIENT, version: "1.0.0", main: "index.js" })) + // A load-time failure, deliberately NOT a resolution error. + fs.writeFileSync(path.join(ambientDir, "index.js"), "throw new TypeError('native binding is for another platform')") + } + + function removeAmbientBroken() { + fs.rmSync(ambientDir, { recursive: true, force: true }) + } + + test("recovers from the managed root when the ambient copy throws on import", async () => { + installAmbientBroken() + try { + installFakePackage(tmpRoot, AMBIENT, "module.exports = { marker: 'managed' }") + process.env["ALTIMATE_DRIVER_DIR"] = tmpRoot + + const mod: any = await loadOptionalDriver("postgres", AMBIENT) + + expect(mod.marker ?? mod.default?.marker).toBe("managed") + } finally { + removeAmbientBroken() + } + }) + + test("reports the ambient load failure when no healthy copy exists", async () => { + installAmbientBroken() + try { + process.env["ALTIMATE_DRIVER_DIR"] = path.join(tmpRoot, "empty") + + let error: unknown + try { + await loadOptionalDriver("postgres", AMBIENT) + } catch (e) { + error = e + } + + // Broken, not absent — the user must not be told to install what they have. + expect(error).not.toBeInstanceOf(DriverNotInstalledError) + expect((error as Error).message).toContain("native binding is for another platform") + } finally { + removeAmbientBroken() + } + }) +}) + +describe("shellQuote on Windows", () => { + test("uses double quotes cmd.exe understands", () => { + // POSIX single-quoting is not runnable in cmd.exe or PowerShell, so the + // printed install command was broken on Windows for any path with a space. + expect(shellQuote("C:\\Users\\x\\My Data\\drivers", "win32")).toBe('"C:\\Users\\x\\My Data\\drivers"') + }) + + test("leaves an ordinary Windows path unquoted", () => { + expect(shellQuote("C:\\Users\\x\\drivers", "win32")).toBe("C:\\Users\\x\\drivers") + }) +}) diff --git a/packages/opencode/src/altimate/tools/warehouse-install-driver.ts b/packages/opencode/src/altimate/tools/warehouse-install-driver.ts index 50a6d1a56d..a512cd706c 100644 --- a/packages/opencode/src/altimate/tools/warehouse-install-driver.ts +++ b/packages/opencode/src/altimate/tools/warehouse-install-driver.ts @@ -69,7 +69,9 @@ export const WarehouseInstallDriverTool = Tool.define("warehouse_install_driver" // a native addon built for another platform, or a half-written copy — used to // report "already installed", so the one command that could repair it refused // to run. Probe an actual load and only decline when it succeeds. - if (isDriverInstalled(driver) && (await driverLoads(driver))) { + const resolves = isDriverInstalled(driver) + const loads = resolves && (await driverLoads(driver)) + if (resolves && loads) { return { title: `${label} driver: already installed`, metadata: { success: true, driver, installed: true, alreadyPresent: true, dir }, @@ -77,7 +79,7 @@ export const WarehouseInstallDriverTool = Tool.define("warehouse_install_driver" } } - const result = await installOptionalDriver(driver) + const result = await installOptionalDriver(driver, { force: resolves && !loads }) const packages = result.packages.join(" ") if (!result.installed) { diff --git a/packages/opencode/test/altimate/driver-catalogue.test.ts b/packages/opencode/test/altimate/driver-catalogue.test.ts index eabbb1f583..bb193f3a3f 100644 --- a/packages/opencode/test/altimate/driver-catalogue.test.ts +++ b/packages/opencode/test/altimate/driver-catalogue.test.ts @@ -128,7 +128,11 @@ describe("driver catalogue consistency", () => { for (const type of types) { // sqlite is bundled with the runtime and needs no optional SDK. if (type === "sqlite") continue - expect(driverForWarehouseType(type), `registry type "${type}" has no installable driver`).toBeDefined() + const resolved = driverForWarehouseType(type) + expect(resolved, `registry type "${type}" resolves to no driver`).toBeDefined() + // toBeDefined() alone would let a stale alias pass while naming a driver + // that DRIVER_PACKAGES cannot actually install. + expect(Object.keys(DRIVER_PACKAGES)).toContain(resolved!) } }) }) From 815e89ea97a1c4dce3d5d94aab8aadb446e03ec9 Mon Sep 17 00:00:00 2001 From: Haider Date: Wed, 26 Aug 2026 00:12:56 +0530 Subject: [PATCH 5/5] fix(drivers): make the repair actually rebuild, and fix the install queue race MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third bot round on #1122. Two findings were raised independently by both CodeRabbit and cubic-dev-ai, which is what made them worth checking closely. **The repair still did not repair.** `force` skipped the resolution-only early return, but `performInstall` then ran an ordinary `npm install`. npm compares the manifest against what is recorded, not the health of what is on disk, so with the package already present it answers "up to date" and rewrites nothing. Verified on npm 11.12.1 against a deliberately corrupted `pg`: the corrupt file survived. cubic proposed appending `--force`. That does not work either — tested, and the corrupt copy still survived, because `--force` forces *fetching* rather than overwriting an already-satisfied dependency. What does work is deleting the package directory first, so a repair now does that before invoking npm. **The install queue serialized only two callers.** Awaiting the in-flight promise released everyone waiting on it at once, and each continuation then started its own `performInstall` without re-reading the map. With three or more installs the later ones overlapped on the same manifest — the exact condition the block exists to prevent. Installs now chain onto the current tail instead. **Two tests were not hermetic.** The forced-install test spawned a real `npm install oracledb` against the live registry, so a unit test depended on npm being on PATH and on network access, with a 15s timeout to block on. The ambient tests wrote a throwing package into this package's real `node_modules`, which a killed run would have left behind to break later resolutions. Both now use injection: `installOptionalDriver` takes a `runNpm`, and `loadOptionalDriver` takes an importer. That keeps the ambient-failure branch genuinely exercised — the reason the fixture was written to disk in the first place — without touching the dependency tree or the network. The file now runs in ~100ms with no external dependencies. Mutants confirmed failing: repair that skips the delete, the old await-then-start queue, and `force` ignored entirely. Tests: 185 drivers unit (was 182), 4,565 opencode. Typecheck clean, 0 lint errors. Co-Authored-By: Claude Opus 5 (1M context) --- packages/drivers/src/resolve.ts | 58 ++++++-- packages/drivers/test/resolve-unit.test.ts | 151 +++++++++++++-------- 2 files changed, 147 insertions(+), 62 deletions(-) diff --git a/packages/drivers/src/resolve.ts b/packages/drivers/src/resolve.ts index 8952aeec39..6ff134e625 100644 --- a/packages/drivers/src/resolve.ts +++ b/packages/drivers/src/resolve.ts @@ -296,9 +296,13 @@ function entryFromManifest(pkgDir: string, specifier: string, pkg: string): stri * * @throws {DriverNotInstalledError} when the package is genuinely absent. */ -export async function loadOptionalDriver(driver: DriverName, specifier: string): Promise { +export async function loadOptionalDriver( + driver: DriverName, + specifier: string, + importer: (spec: string) => Promise = (spec) => import(/* @vite-ignore */ spec), +): Promise { try { - return await import(/* @vite-ignore */ specifier) + return await importer(specifier) } catch (ambientError) { const ambientBroken = !isModuleNotFound(ambientError, specifier) const roots = driverSearchRoots() @@ -311,7 +315,7 @@ export async function loadOptionalDriver(driver: DriverName, specifier: string): } try { - return await import(/* @vite-ignore */ pathToFileURL(resolved).href) + return await importer(pathToFileURL(resolved).href) } catch (loadError) { // On disk but will not load — a half-installed copy, or a native addon // built for another platform. When an ambient copy was also broken, @@ -376,6 +380,16 @@ export function isDriverInstalled(driver: DriverName, roots = driverSearchRoots( return DRIVER_PACKAGES[driver].every((pkg) => resolveOptionalPackage(pkg, roots) !== undefined) } +/** Runs npm. Injectable so install behaviour can be tested without a registry. */ +export type NpmRunner = (args: string[], cwd: string, timeoutMs: number) => Promise<{ code: number; output: string }> + +export interface InstallOptions { + timeoutMs?: number + /** Rebuild even when the package resolves — the caller knows it does not load. */ + force?: boolean + runNpm?: NpmRunner +} + export interface InstallResult { readonly driver: DriverName readonly packages: readonly string[] @@ -413,6 +427,21 @@ function runNpm(args: string[], cwd: string, timeoutMs: number): Promise<{ code: }) } +/** + * Delete `packages` from the managed directory so a reinstall genuinely rebuilds + * them. Best-effort: a path we cannot remove simply leaves npm to no-op, which + * is the behaviour we already had. + */ +function removeInstalledPackages(dir: string, packages: readonly string[]): void { + for (const pkg of packages) { + try { + fs.rmSync(path.join(dir, "node_modules", ...pkg.split("/")), { recursive: true, force: true }) + } catch { + // Nothing to gain from failing the install over a stale directory. + } + } +} + /** * npm arguments for installing `packages` into the managed driver directory. * @@ -434,7 +463,7 @@ export function npmInstallArgs(packages: readonly string[]): string[] { */ export async function installOptionalDriver( driver: DriverName, - options: { timeoutMs?: number; force?: boolean } = {}, + options: InstallOptions = {}, ): Promise { const packages = DRIVER_PACKAGES[driver] const dir = driverInstallDir() @@ -448,10 +477,13 @@ export async function installOptionalDriver( } // Serialize per directory: concurrent npm runs against one manifest can leave - // the managed directory inconsistent. + // the managed directory inconsistent. Chain onto the current tail rather than + // awaiting it first — awaiting released every queued caller at once, so with + // three or more installs the second and third still overlapped. const pending = installsInFlight.get(dir) - if (pending) await pending.catch(() => {}) - const run = performInstall(driver, packages, dir, options) + const run = Promise.resolve(pending) + .catch(() => undefined) + .then(() => performInstall(driver, packages, dir, options)) installsInFlight.set(dir, run) try { return await run @@ -467,7 +499,7 @@ async function performInstall( driver: DriverName, packages: readonly string[], dir: string, - options: { timeoutMs?: number; force?: boolean }, + options: InstallOptions, ): Promise { try { @@ -492,7 +524,15 @@ async function performInstall( } } - const { code, output } = await runNpm(npmInstallArgs(packages), dir, options.timeoutMs ?? 180_000) + // A repair has to delete the broken copy first. npm compares the manifest to + // what is on disk, not the health of it, so with the package already recorded + // it answers "up to date" and rewrites nothing — verified on npm 11.12.1 + // against a deliberately corrupted `pg`. `--force` does not change that; it + // forces *fetching*, not overwriting an already-satisfied dependency. + if (options.force) removeInstalledPackages(dir, packages) + + const npm = options.runNpm ?? runNpm + const { code, output } = await npm(npmInstallArgs(packages), dir, options.timeoutMs ?? 180_000) if (code === 127) { return { diff --git a/packages/drivers/test/resolve-unit.test.ts b/packages/drivers/test/resolve-unit.test.ts index 7db6a92b05..11940428fc 100644 --- a/packages/drivers/test/resolve-unit.test.ts +++ b/packages/drivers/test/resolve-unit.test.ts @@ -446,79 +446,124 @@ describe("shellQuote", () => { }) describe("repairing a broken install", () => { - test("force skips the resolution-only early return", async () => { + test("force skips the resolution-only early return and rebuilds", async () => { // The bug this pins: installOptionalDriver short-circuited on - // isDriverInstalled, a resolution-only check. A caller that had detected a - // present-but-unloadable copy asked for a reinstall and got back - // `installed: true, alreadyPresent: true` with npm never run — so the - // repair path was unreachable. + // isDriverInstalled, a resolution-only check, so a caller that had detected + // a present-but-unloadable copy got back `installed: true` with npm never + // run — the repair path was unreachable. installFakePackage(tmpRoot, "oracledb", "module.exports = {}") process.env["ALTIMATE_DRIVER_DIR"] = tmpRoot - // Without force: recognised as installed, returns immediately. - const asIs = await installOptionalDriver("oracle") + const calls: string[][] = [] + const runNpm = async (args: string[]) => { + calls.push(args) + // Re-create what a real install would leave behind. + installFakePackage(tmpRoot, "oracledb", "module.exports = { repaired: true }") + return { code: 0, output: "" } + } + + const asIs = await installOptionalDriver("oracle", { runNpm }) expect(asIs.alreadyPresent).toBe(true) - expect(asIs.installed).toBe(true) + expect(calls).toEqual([]) - // With force: must NOT take that early return. npm is unavailable for a - // package that does not exist, so this reaches a real attempt and reports - // failure rather than a fictitious success. - const forced = await installOptionalDriver("oracle", { force: true, timeoutMs: 15_000 }) + const forced = await installOptionalDriver("oracle", { force: true, runNpm }) expect(forced.alreadyPresent).toBe(false) - }, 60_000) + expect(forced.installed).toBe(true) + expect(calls.length).toBe(1) + }) + + test("a repair deletes the broken copy first, because npm will not overwrite it", async () => { + // Verified against npm 11.12.1: with the package already recorded in the + // manifest, `npm install` answers "up to date" and rewrites nothing, even + // with --force. Unless the broken directory is removed, the repair is a + // no-op that reports success. + installFakePackage(tmpRoot, "oracledb", "throw new Error('corrupt')") + process.env["ALTIMATE_DRIVER_DIR"] = tmpRoot + const pkgDir = path.join(tmpRoot, "node_modules", "oracledb") + + let presentWhenNpmRan = true + const runNpm = async () => { + presentWhenNpmRan = fs.existsSync(pkgDir) + installFakePackage(tmpRoot, "oracledb", "module.exports = {}") + return { code: 0, output: "" } + } + + await installOptionalDriver("oracle", { force: true, runNpm }) + + expect(presentWhenNpmRan).toBe(false) + }) + + test("a failed repair is reported as a failure, not a success", async () => { + installFakePackage(tmpRoot, "oracledb", "throw new Error('corrupt')") + process.env["ALTIMATE_DRIVER_DIR"] = tmpRoot + + const result = await installOptionalDriver("oracle", { + force: true, + runNpm: async () => ({ code: 1, output: "network unreachable" }), + }) + + expect(result.installed).toBe(false) + expect(result.error).toContain("network unreachable") + }) + + test("concurrent installs against one directory do not overlap", async () => { + // Awaiting the in-flight promise released every queued caller at once, so + // with three or more installs the later ones still ran npm concurrently. + process.env["ALTIMATE_DRIVER_DIR"] = tmpRoot + let active = 0 + let maxActive = 0 + const runNpm = async () => { + active += 1 + maxActive = Math.max(maxActive, active) + await new Promise((r) => setTimeout(r, 15)) + active -= 1 + return { code: 0, output: "" } + } + + await Promise.all([ + installOptionalDriver("oracle", { force: true, runNpm }), + installOptionalDriver("oracle", { force: true, runNpm }), + installOptionalDriver("oracle", { force: true, runNpm }), + installOptionalDriver("oracle", { force: true, runNpm }), + ]) + + expect(maxActive).toBe(1) + }) }) describe("a broken ambient copy does not hide a good one on disk", () => { - // This one needs a fixture the ambient resolver can genuinely find, so it is - // written into this package's own node_modules and removed afterwards. Every - // cheaper version of this test was vacuous: a fixture that only exists under - // ALTIMATE_DRIVER_DIR never reaches the ambient branch at all. - const AMBIENT = "altimate-ambient-throws" - const ambientDir = path.join(import.meta.dir, "..", "node_modules", AMBIENT) - - function installAmbientBroken() { - fs.mkdirSync(ambientDir, { recursive: true }) - fs.writeFileSync(path.join(ambientDir, "package.json"), JSON.stringify({ name: AMBIENT, version: "1.0.0", main: "index.js" })) - // A load-time failure, deliberately NOT a resolution error. - fs.writeFileSync(path.join(ambientDir, "index.js"), "throw new TypeError('native binding is for another platform')") - } - - function removeAmbientBroken() { - fs.rmSync(ambientDir, { recursive: true, force: true }) + // The ambient branch needs an import that resolves and then throws. Injecting + // the importer reaches it without writing a throwing package into this + // package's real node_modules, which a killed test run would leave behind. + const brokenAmbient = async (spec: string) => { + if (!spec.startsWith("file:")) throw new TypeError("native binding is for another platform") + return import(/* @vite-ignore */ spec) } test("recovers from the managed root when the ambient copy throws on import", async () => { - installAmbientBroken() - try { - installFakePackage(tmpRoot, AMBIENT, "module.exports = { marker: 'managed' }") - process.env["ALTIMATE_DRIVER_DIR"] = tmpRoot + installFakePackage(tmpRoot, "altimate-recovered-sdk", "module.exports = { marker: 'managed' }") + process.env["ALTIMATE_DRIVER_DIR"] = tmpRoot - const mod: any = await loadOptionalDriver("postgres", AMBIENT) + const mod: any = await loadOptionalDriver("postgres", "altimate-recovered-sdk", brokenAmbient) - expect(mod.marker ?? mod.default?.marker).toBe("managed") - } finally { - removeAmbientBroken() - } + expect(mod.marker ?? mod.default?.marker).toBe("managed") }) test("reports the ambient load failure when no healthy copy exists", async () => { - installAmbientBroken() + process.env["ALTIMATE_DRIVER_DIR"] = path.join(tmpRoot, "empty") + delete process.env["ALTIMATE_BIN_DIR"] + delete process.env["NODE_PATH"] + + let error: unknown try { - process.env["ALTIMATE_DRIVER_DIR"] = path.join(tmpRoot, "empty") - - let error: unknown - try { - await loadOptionalDriver("postgres", AMBIENT) - } catch (e) { - error = e - } - - // Broken, not absent — the user must not be told to install what they have. - expect(error).not.toBeInstanceOf(DriverNotInstalledError) - expect((error as Error).message).toContain("native binding is for another platform") - } finally { - removeAmbientBroken() + await loadOptionalDriver("postgres", "altimate-absent-sdk", brokenAmbient) + } catch (e) { + error = e } + + // Broken, not absent — the user must not be told to install what they have. + expect(error).not.toBeInstanceOf(DriverNotInstalledError) + expect((error as Error).message).toContain("native binding is for another platform") }) })