From edb0759fa8db45efcc4b907dde163f1f3dfa287c Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Sun, 30 Aug 2026 01:09:34 -0700 Subject: [PATCH 1/2] fix: [#1203] --dir silently read a populated warehouse store as empty MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `--dir` calls `process.chdir()` (`cli/cmd/run.ts:412`). `ConnectionRegistry` never absolutized the `path` field, so a relative store path in a connection config resolved against whatever working directory the process happened to have when the driver opened it. Both file-backed drivers then answered the miss by CREATING an empty database — `new duckdb.Database(path)` in `drivers/src/duckdb.ts`, `create: !isReadonly` in `drivers/src/sqlite.ts`. Every query afterwards succeeded and returned zero rows, with no error and no fault signal for a caller to key on. Two fixes, either of which breaks the chain; both are needed because the second is the root hazard and is independent of `--dir`: 1. Resolve store paths deterministically. `resolveStorePaths()` absolutizes a relative `path` once, at config-load time, against the directory that declared it — global config against `~/.altimate-code`, project config and `ALTIMATE_CODE_CONN_*` against the project root. A later `chdir` cannot move it. `warehouse_add` persists the absolute form for the same reason. The project root comes from `Instance.directory`, not `process.cwd()`. A server or `run --attach` session never chdirs — it carries the project in the instance context and leaves cwd at the server's launch directory — so cwd there is somebody else's project. `localConfigPath()` had that bug already; absolutizing at load time would have baked it in permanently. 2. Never conjure a store that was meant to exist. New `drivers/src/file-store.ts` holds `assertStoreExists()`, which both file-backed drivers call before opening: a missing local file throws an error naming the path it looked for. Creation is opt-in via `create: true`, which `schema_sync` and `test_local` — the two tools that deliberately materialize a scratch store — now pass. Exempt from the check: the exact string `:memory:`, an empty path, and scheme-qualified targets (`md:`, `s3://`, `ducklake:`, anything a DuckDB extension provides), which the driver reports as an unknown scheme rather than creating. Only the EXACT `:memory:` — DuckDB writes a real file for `:memory:named` and for `:foo`, so those stay inside the guard. Windows drive letters stay paths. `bun:sqlite` rejects an options object with no open flag, so the SQLite driver now passes `readwrite` explicitly where `create` used to imply it. Relative SQLite `file:` URIs are deliberately NOT handled here, and are filed as #1209 with the evidence. They follow cwd the same way, but `bun:sqlite` parses `file:` as a URI on macOS and appears to treat it as a literal filename on Linux, and Windows is unverified. A rewrite changes which database opens, so shipping one across platforms it has not been proven on would create the same class of defect this fixes. Verified through a compiled binary built with the production build options, invoked with `--dir` from an unrelated working directory. Before: `tables: []` plus a stray `warehouse.db` (+ `-wal`, `-shm`) written into the `--dir` target; the DuckDB arm left a 12,288-byte stray `.duckdb`. After: the real table, and nothing created. `test/altimate/store-path-resolution.test.ts` compiles that binary and runs five arms — including a server-style request driven through `Instance.provide` with a decoy config and store in the launch directory — of which three fail on unmodified main. Existing tests that relied on create-on-open to build their own scratch store now pass `create: true`. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01VqnuBDGkh1ZT65Ti7e6DHZ --- docs/docs/configure/warehouses.md | 26 ++ docs/docs/drivers.md | 5 + packages/drivers/src/duckdb.ts | 4 + packages/drivers/src/file-store.ts | 71 +++++ packages/drivers/src/index.ts | 3 + packages/drivers/src/sqlite.ts | 13 +- packages/drivers/test/driver-security.test.ts | 6 +- .../drivers/test/file-store-guard.test.ts | 172 ++++++++++ .../altimate/native/connections/registry.ts | 105 ++++++- .../src/altimate/native/local/schema-sync.ts | 5 +- .../src/altimate/native/local/test-local.ts | 5 +- .../src/altimate/tools/warehouse-add.ts | 5 +- .../test/altimate/drivers-e2e.test.ts | 9 +- .../altimate/fixtures/store-path-probe.ts | 76 +++++ .../test/altimate/schema-cache.test.ts | 8 +- .../altimate/store-path-resolution.test.ts | 294 ++++++++++++++++++ 16 files changed, 786 insertions(+), 21 deletions(-) create mode 100644 packages/drivers/src/file-store.ts create mode 100644 packages/drivers/test/file-store-guard.test.ts create mode 100644 packages/opencode/test/altimate/fixtures/store-path-probe.ts create mode 100644 packages/opencode/test/altimate/store-path-resolution.test.ts diff --git a/docs/docs/configure/warehouses.md b/docs/docs/configure/warehouses.md index fc473058ee..d85136be46 100644 --- a/docs/docs/configure/warehouses.md +++ b/docs/docs/configure/warehouses.md @@ -262,6 +262,26 @@ If you're already authenticated via `gcloud`, omit `credentials_path`: | Field | Required | Description | |-------|----------|-------------| | `path` | No | Database file path. Omit or use `":memory:"` for in-memory | +| `create` | No | Create the database file if it is missing (default: `false`) | + +!!! warning "The store must already exist" + Connecting never creates the database. If the file is missing, the connection + fails with an error naming the path it looked for — an empty database would + otherwise answer every query with no rows and no error, which reads as a + successful query against an empty warehouse. Set `"create": true` only when + you intend this store to be created. + +!!! note "How a relative `path` is resolved" + A relative `path` is resolved once, when the config is loaded, against the + directory that declares it: + + - `~/.altimate-code/connections.json` → resolved against `~/.altimate-code` + - `/.altimate-code/connections.json` → resolved against `` + - `ALTIMATE_CODE_CONN_*` environment variables → resolved against the project root + + It is never resolved against the current working directory, so `--dir` cannot + re-point an existing connection at a different file. Absolute paths are always + safest. !!! note "Concurrent access" DuckDB does not support concurrent write access to the same file. If another process holds a write lock, Altimate Code automatically retries the connection in **read-only** mode so you can still query the data. A clear error message is shown if read-only access also fails. @@ -443,10 +463,16 @@ If you're already authenticated via `gcloud`, omit `credentials_path`: |-------|----------|-------------| | `path` | No | Database file path. Omit or use `":memory:"` for in-memory | | `readonly` | No | Open in read-only mode (default: `false`) | +| `create` | No | Create the database file if it is missing (default: `false`) | !!! note SQLite uses Bun's built-in `bun:sqlite` driver. WAL journal mode is enabled automatically for writable databases. +!!! warning "The store must already exist" + As with DuckDB, connecting never creates the database, and a relative `path` + resolves against the directory of the config that declares it — not the current + working directory. See the DuckDB section above for the full rules. + ## SQL Server ```json diff --git a/docs/docs/drivers.md b/docs/docs/drivers.md index fc9c13530a..c741272d35 100644 --- a/docs/docs/drivers.md +++ b/docs/docs/drivers.md @@ -191,6 +191,11 @@ MongoDB supports server versions 3.6 through 8.0. Queries use MQL (MongoDB Query |--------|--------------| | File | `path: "./my-database.sqlite"` | +For both file-backed drivers, the database must already exist — connecting never +creates it. Pass `create: true` to create it deliberately. A relative `path` +resolves against the directory of the config that declares it, not the current +working directory. See [Warehouses](configure/warehouses.md#duckdb) for the full rules. + ## SSH Tunneling Connect through a bastion host by adding SSH config to any connection: diff --git a/packages/drivers/src/duckdb.ts b/packages/drivers/src/duckdb.ts index 32bf58c52a..0c7e8e99a8 100644 --- a/packages/drivers/src/duckdb.ts +++ b/packages/drivers/src/duckdb.ts @@ -2,6 +2,7 @@ * DuckDB driver using the `duckdb` package. */ +import { assertStoreExists } from "./file-store" import type { ConnectionConfig, Connector, ConnectorResult, ExecuteOptions, SchemaColumn } from "./types" import { loadOptionalDriver } from "./resolve" @@ -48,6 +49,9 @@ export async function connect(config: ConnectionConfig): Promise { return { async connect() { + // altimate_change start — never conjure an empty store on open + assertStoreExists(config, dbPath, "DuckDB") + // altimate_change end // altimate_change start — retry with read-only on lock errors const tryConnect = (accessMode?: string): Promise => new Promise((resolve, reject) => { diff --git a/packages/drivers/src/file-store.ts b/packages/drivers/src/file-store.ts new file mode 100644 index 0000000000..2adda5bef5 --- /dev/null +++ b/packages/drivers/src/file-store.ts @@ -0,0 +1,71 @@ +/** + * Shared guards for file-backed drivers (DuckDB, SQLite). + * + * Both engines create an empty database when asked to open a file that does + * not exist. For a warehouse connection that is never what the caller wants: + * a mistyped or mis-resolved path then yields a working connector over an + * empty database, so every query succeeds and returns nothing. An agent handed + * that result reports "no tables" instead of an error. + * + * Opening a store is therefore read-or-fail by default. Creation is opt-in via + * `create: true`, which the tools that deliberately materialize a local store + * (local test scratch databases, schema sync targets) pass explicitly. + */ + +import * as fs from "fs" +import type { ConnectionConfig } from "./types" + +/** + * Whether `dbPath` names a file on the local filesystem, and so can be + * existence-checked before the driver opens it. + * + * Only the exact string `:memory:` is an in-memory database. Both engines + * treat `:memory:named` — and any other colon-prefixed name — as an ordinary + * (if oddly named) file: DuckDB really does write a file called + * `:memory:named`, so those must stay inside the guard. An empty path is + * DuckDB's in-memory database and SQLite's anonymous temporary one; neither + * touches disk. + * + * A scheme-qualified target is not a local file: MotherDuck (`md:`), object + * storage (`s3://`), DuckLake, and any other scheme a DuckDB extension + * provides. Those are left to the driver, which reports an unknown scheme as a + * missing-extension error rather than silently creating anything. The pattern + * requires two or more characters before the colon so a Windows drive letter + * (`C:\data\wh.duckdb`) stays a path. + */ +export function isLocalFilePath(dbPath: string): boolean { + if (dbPath === "" || dbPath === ":memory:") return false + if (/^[a-zA-Z][a-zA-Z0-9+.-]+:/.test(dbPath)) return false + return true +} + +/** Whether the caller explicitly opted in to creating the store. */ +export function allowsCreate(config: ConnectionConfig): boolean { + return config.create === true +} + +/** + * Throw unless the store is safe to open: it already exists, the caller opted + * in to creating it, or the path is not a local file at all. + * + * @param engine Human-readable engine name used in the error message. + * @param allowCreate Whether this open will actually create the store. Defaults + * to the config's `create` flag; a driver passes it explicitly when its own + * options can veto creation — SQLite never creates a read-only connection. + */ +export function assertStoreExists( + config: ConnectionConfig, + dbPath: string, + engine: string, + allowCreate: boolean = allowsCreate(config), +): void { + if (allowCreate) return + if (!isLocalFilePath(dbPath)) return + if (fs.existsSync(dbPath)) return + throw new Error( + `${engine} database file not found: "${dbPath}". ` + + `Opening a warehouse connection never creates the database — an empty store would answer every query with no rows. ` + + `Check the "path" in your connection config (relative paths resolve against the config file's directory, not the current directory), ` + + `or pass "create": true if this store is meant to be created.`, + ) +} diff --git a/packages/drivers/src/index.ts b/packages/drivers/src/index.ts index d3c755c31d..21f9c50bf8 100644 --- a/packages/drivers/src/index.ts +++ b/packages/drivers/src/index.ts @@ -4,6 +4,9 @@ export type { Connector, ConnectorResult, SchemaColumn, ConnectionConfig } from // Re-export config normalization export { normalizeConfig, sanitizeConnectionString } from "./normalize" +// Re-export file-backed store guards +export { allowsCreate, assertStoreExists, isLocalFilePath } from "./file-store" + // Re-export driver connect functions export { connect as connectPostgres } from "./postgres" export { connect as connectSnowflake } from "./snowflake" diff --git a/packages/drivers/src/sqlite.ts b/packages/drivers/src/sqlite.ts index 48ef8321cd..aaecfdf544 100644 --- a/packages/drivers/src/sqlite.ts +++ b/packages/drivers/src/sqlite.ts @@ -4,6 +4,7 @@ */ import { Database } from "bun:sqlite" +import { allowsCreate, assertStoreExists } from "./file-store" import type { ConnectionConfig, Connector, ConnectorResult, ExecuteOptions, SchemaColumn } from "./types" export async function connect(config: ConnectionConfig): Promise { @@ -13,10 +14,20 @@ export async function connect(config: ConnectionConfig): Promise { return { async connect() { const isReadonly = config.readonly === true + // altimate_change start — never conjure an empty store on open. + // A read-only connection never creates, so `create: true` cannot excuse a + // missing file there; the guard is told the effective decision. + const willCreate = !isReadonly && allowsCreate(config) + assertStoreExists(config, dbPath, "SQLite", willCreate) db = new Database(dbPath, { readonly: isReadonly, - create: !isReadonly, + // `create` alone is no longer implied by "not readonly", so the + // read-write flag has to be explicit — bun:sqlite rejects an options + // object that sets no open flag at all. + readwrite: !isReadonly, + create: willCreate, }) + // altimate_change end if (!isReadonly) { db.exec("PRAGMA journal_mode = WAL") } diff --git a/packages/drivers/test/driver-security.test.ts b/packages/drivers/test/driver-security.test.ts index 6a0f8d8c5d..3f3f221340 100644 --- a/packages/drivers/test/driver-security.test.ts +++ b/packages/drivers/test/driver-security.test.ts @@ -168,7 +168,7 @@ describe("DuckDB driver", () => { })) const { connect } = await import("../src/duckdb") - const connector = await connect({ type: "duckdb", path: "/tmp/test.duckdb" }) + const connector = await connect({ type: "duckdb", path: "/tmp/test.duckdb", create: true }) await connector.connect() expect(connectAttempts).toBe(2) // First failed, second succeeded in READ_ONLY // The retry must specifically request READ_ONLY — two attempts alone don't @@ -262,7 +262,7 @@ describe("DuckDB driver", () => { })) const { connect } = await import("../src/duckdb") - const connector = await connect({ type: "duckdb", path: "/tmp/sync-lock.duckdb" }) + const connector = await connect({ type: "duckdb", path: "/tmp/sync-lock.duckdb", create: true }) await connector.connect() expect(connectAttempts).toBe(2) expect(await connector.execute("SELECT 1")).toMatchObject({ columns: ["ok"], rows: [[1]], row_count: 1 }) @@ -287,7 +287,7 @@ describe("DuckDB driver", () => { })) const { connect } = await import("../src/duckdb") - const connector = await connect({ type: "duckdb", path: "/tmp/corrupt.duckdb" }) + const connector = await connect({ type: "duckdb", path: "/tmp/corrupt.duckdb", create: true }) await expect(connector.connect()).rejects.toThrow("catalog is corrupt") }) }) diff --git a/packages/drivers/test/file-store-guard.test.ts b/packages/drivers/test/file-store-guard.test.ts new file mode 100644 index 0000000000..391165951c --- /dev/null +++ b/packages/drivers/test/file-store-guard.test.ts @@ -0,0 +1,172 @@ +/** + * Unit tests for the file-backed store guards (src/file-store.ts). + * + * Both DuckDB and SQLite create an empty database when opened on a path that + * does not exist. For a warehouse connection that turns a wrong path into a + * silent empty result set rather than an error, so opening is read-or-fail + * unless the caller passes `create: true`. + * + * The end-to-end proof — a populated store reached through `--dir` from an + * unrelated cwd, run against a compiled binary — lives in + * packages/opencode/test/altimate/store-path-resolution.test.ts. + */ +import { describe, test, expect, mock, afterEach } from "bun:test" +import { Database } from "bun:sqlite" +import * as fs from "fs" +import * as os from "os" +import * as path from "path" +import { allowsCreate, assertStoreExists, isLocalFilePath } from "../src/file-store" + +const CANARY_TABLE = "zorbulax_ledger" + +const tmpDirs: string[] = [] +function tmp(): string { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), "file-store-guard-")) + tmpDirs.push(dir) + return dir +} +afterEach(() => { + while (tmpDirs.length) fs.rmSync(tmpDirs.pop()!, { recursive: true, force: true }) +}) + +describe("isLocalFilePath", () => { + test("treats in-memory and remote targets as non-files", () => { + expect(isLocalFilePath(":memory:")).toBe(false) + expect(isLocalFilePath("")).toBe(false) + expect(isLocalFilePath("md:my_database")).toBe(false) + expect(isLocalFilePath("motherduck:my_database")).toBe(false) + expect(isLocalFilePath("s3://bucket/warehouse.duckdb")).toBe(false) + expect(isLocalFilePath("https://example.com/warehouse.duckdb")).toBe(false) + // A scheme from a DuckDB extension we have never heard of is still not a + // local file; the driver reports an unknown scheme rather than creating one. + expect(isLocalFilePath("ducklake:my_catalog")).toBe(false) + }) + + test("treats real paths — including Windows drive letters — as files", () => { + expect(isLocalFilePath("warehouse.duckdb")).toBe(true) + expect(isLocalFilePath("./data/warehouse.duckdb")).toBe(true) + expect(isLocalFilePath("/var/data/warehouse.duckdb")).toBe(true) + expect(isLocalFilePath("C:\\data\\warehouse.duckdb")).toBe(true) + }) + + test("only the exact `:memory:` is in-memory — `:memory:name` is a real file", () => { + // DuckDB writes a file literally named ":memory:named" for this path, and + // a colon-prefixed name is an ordinary file to both engines. Classifying + // them as non-files would let the guard be bypassed by a typo. + expect(isLocalFilePath(":memory:named")).toBe(true) + expect(isLocalFilePath(":memory")).toBe(true) + expect(isLocalFilePath(":foo")).toBe(true) + }) +}) + +describe("assertStoreExists", () => { + test("throws for a missing local file, naming the path it looked for", () => { + const missing = path.join(tmp(), "absent.duckdb") + expect(() => assertStoreExists({ type: "duckdb" }, missing, "DuckDB")).toThrow(missing) + expect(() => assertStoreExists({ type: "duckdb" }, missing, "DuckDB")).toThrow("not found") + expect(fs.existsSync(missing)).toBe(false) + }) + + test("throws for a missing `:memory:`-lookalike rather than letting it be created", () => { + const dir = tmp() + const lookalike = path.join(dir, ":memory:named") + expect(() => assertStoreExists({ type: "duckdb" }, lookalike, "DuckDB")).toThrow("not found") + expect(fs.existsSync(lookalike)).toBe(false) + }) + + test("passes for an existing file, an in-memory target, or an explicit create", () => { + const dir = tmp() + const present = path.join(dir, "present.duckdb") + fs.writeFileSync(present, "") + expect(() => assertStoreExists({ type: "duckdb" }, present, "DuckDB")).not.toThrow() + expect(() => assertStoreExists({ type: "duckdb" }, ":memory:", "DuckDB")).not.toThrow() + expect(() => + assertStoreExists({ type: "duckdb", create: true }, path.join(dir, "new.duckdb"), "DuckDB"), + ).not.toThrow() + }) + + test("allowsCreate only accepts a literal true", () => { + expect(allowsCreate({ type: "duckdb", create: true })).toBe(true) + expect(allowsCreate({ type: "duckdb", create: "true" })).toBe(false) + expect(allowsCreate({ type: "duckdb" })).toBe(false) + }) +}) + +describe("DuckDB driver create-on-open", () => { + test("refuses to open a missing store and creates nothing", async () => { + // The real duckdb addon is an optional dependency; the guard runs before + // any Database is constructed, so a stub is enough to prove it fires first. + let constructed = false + mock.module("duckdb", () => ({ + default: { + Database: class { + constructor(_p: string, optsOrCb: any, cb?: (err: Error | null) => void) { + constructed = true + const done = typeof optsOrCb === "function" ? optsOrCb : cb! + setTimeout(() => done(null), 0) + } + connect() { + return {} + } + close(cb: any) { + if (cb) cb(null) + } + }, + }, + })) + + const missing = path.join(tmp(), "absent.duckdb") + const { connect } = await import("../src/duckdb") + const connector = await connect({ type: "duckdb", path: missing }) + + await expect(connector.connect()).rejects.toThrow("not found") + expect(constructed).toBe(false) + expect(fs.existsSync(missing)).toBe(false) + }) +}) + +describe("SQLite driver create-on-open", () => { + test("reads an existing store", async () => { + const store = path.join(tmp(), "warehouse.db") + const seed = new Database(store, { create: true }) + seed.exec(`CREATE TABLE ${CANARY_TABLE}(id INTEGER)`) + seed.close() + + const { connect } = await import("../src/sqlite") + const connector = await connect({ type: "sqlite", path: store }) + await connector.connect() + const tables = await connector.listTables("main") + await connector.close() + + expect(tables.map((t) => t.name)).toContain(CANARY_TABLE) + }) + + test("refuses to open a missing store and creates nothing", async () => { + const missing = path.join(tmp(), "absent.db") + const { connect } = await import("../src/sqlite") + const connector = await connect({ type: "sqlite", path: missing }) + + await expect(connector.connect()).rejects.toThrow("not found") + expect(fs.existsSync(missing)).toBe(false) + }) + + test("refuses a read-only connection to a missing store even with create: true", async () => { + // A read-only open never creates, so `create` must not excuse the miss. + const missing = path.join(tmp(), "absent-readonly.db") + const { connect } = await import("../src/sqlite") + const connector = await connect({ type: "sqlite", path: missing, readonly: true, create: true }) + + await expect(connector.connect()).rejects.toThrow("not found") + expect(fs.existsSync(missing)).toBe(false) + }) + + test("creates only when the caller opts in", async () => { + const target = path.join(tmp(), "scratch.db") + const { connect } = await import("../src/sqlite") + const connector = await connect({ type: "sqlite", path: target, create: true }) + await connector.connect() + await connector.close() + + expect(fs.existsSync(target)).toBe(true) + }) +}) diff --git a/packages/opencode/src/altimate/native/connections/registry.ts b/packages/opencode/src/altimate/native/connections/registry.ts index 8be3bfc672..4f2735918e 100644 --- a/packages/opencode/src/altimate/native/connections/registry.ts +++ b/packages/opencode/src/altimate/native/connections/registry.ts @@ -7,14 +7,21 @@ * 3. ALTIMATE_CODE_CONN_* environment variables * * Connectors are created lazily via dynamic import of the appropriate driver. + * + * A relative `path` on a file-backed connection (duckdb, sqlite) is resolved + * once at load time against the directory that declared it — the global config + * resolves against ~/.altimate-code, a project config and the environment + * variables resolve against the project root. It is never re-resolved against + * the working directory at connect time, which `--dir` changes. */ import * as fs from "fs" import * as path from "path" import * as os from "os" +import { Instance } from "@/project/instance" import { Log } from "@/altimate/util/log" import type { ConnectionConfig, Connector } from "@altimateai/drivers" -import { normalizeConfig } from "@altimateai/drivers" +import { isLocalFilePath, normalizeConfig } from "@altimateai/drivers" import { resolveConfig, saveConnection } from "./credential-store" import { startTunnel, extractSshConfig, closeTunnel } from "./ssh-tunnel" import type { WarehouseInfo } from "../types" @@ -40,9 +47,87 @@ function globalConfigPath(): string { return path.join(os.homedir(), ".altimate-code", "connections.json") } +// altimate_change start — the project root, not the process working directory +/** + * The directory the current request is working in. + * + * A server or `run --attach` session never chdirs: it carries the project in + * the instance context and leaves the working directory wherever the server was + * launched (server.ts routes every request through `Instance.provide`). Reading + * `process.cwd()` there picks up the launch directory, which is somebody else's + * project. `Instance.directory` throws synchronously when there is no instance + * context — early CLI paths and unit tests — and the working directory is the + * right answer in exactly those cases, since `run --dir` has already chdir'd. + * + * The registry itself is still process-global: `loaded` latches on first + * access, so one server process serving two projects keeps the first project's + * configs for both. That predates this change — the base was simply the launch + * directory for everyone — and making the registry per-instance is a larger + * change than this fix. What this does guarantee is that whichever project + * loads resolves against itself, not against wherever the server was started. + */ +function projectRoot(): string { + try { + return Instance.directory + } catch { + return process.cwd() + } +} +// altimate_change end + function localConfigPath(): string { - return path.join(process.cwd(), ".altimate-code", "connections.json") + // altimate_change start — resolve against the project, not the launch cwd + return path.join(projectRoot(), ".altimate-code", "connections.json") + // altimate_change end +} + +// --------------------------------------------------------------------------- +// Store path resolution +// --------------------------------------------------------------------------- + +// altimate_change start — resolve file-backed store paths against a stable base +/** Driver types whose `path` field names a database file on local disk. */ +const FILE_STORE_TYPES = new Set(["duckdb", "sqlite"]) + +/** + * Absolutize the `path` of every file-backed connection in `entries` against + * `baseDir`, once, at load time. + * + * A relative store path must not follow the process working directory. `run` + * and `attach` call `process.chdir()` for `--dir` (see cli/cmd/run.ts), so a + * path resolved lazily at connect time points somewhere different depending on + * how the CLI was invoked — and both file-backed engines answer a miss by + * creating an empty database rather than failing. + * + * The base is the directory that declared the path, which is stable for the + * life of the config file: + * - global config -> ~/.altimate-code (the config file's own directory) + * - project config -> the project root (the parent of its .altimate-code) + * - environment -> the project root, the only directory an ambient + * variable can reasonably mean + */ +function resolveStorePaths( + entries: Record, + baseDir: string, +): Record { + const resolved: Record = {} + for (const [name, config] of Object.entries(entries)) { + const storePath = config?.path + const type = typeof config?.type === "string" ? config.type.toLowerCase() : "" + if ( + !FILE_STORE_TYPES.has(type) || + typeof storePath !== "string" || + !isLocalFilePath(storePath) || + path.isAbsolute(storePath) + ) { + resolved[name] = config + continue + } + resolved[name] = { ...config, path: path.resolve(baseDir, storePath) } + } + return resolved } +// altimate_change end // --------------------------------------------------------------------------- // Loading @@ -85,9 +170,13 @@ function loadFromEnv(): Record { export function load(): void { configs.clear() - const global = loadFromFile(globalConfigPath()) - const local = loadFromFile(localConfigPath()) - const env = loadFromEnv() + // altimate_change start — absolutize store paths against the directory that + // declared them, so a later process.chdir() (--dir) cannot move the store. + const base = projectRoot() + const global = resolveStorePaths(loadFromFile(globalConfigPath()), path.dirname(globalConfigPath())) + const local = resolveStorePaths(loadFromFile(localConfigPath()), base) + const env = resolveStorePaths(loadFromEnv(), base) + // altimate_change end // Merge: global < local < env for (const [name, config] of Object.entries(global)) { @@ -436,7 +525,11 @@ export async function add( // Normalize field names before saving so sensitive fields under alias // names (e.g., keyfileJson → credentials_json) are properly detected - const normalized = normalizeConfig(config) + // altimate_change start — a relative store path means "relative to where + // the user typed this"; persist it absolute so the saved global config is + // not silently re-pointed by the next invocation's working directory. + const normalized = resolveStorePaths({ [name]: normalizeConfig(config) }, projectRoot())[name] + // altimate_change end // Store credentials in keychain, get sanitized config const { sanitized, warnings } = await saveConnection(name, normalized) diff --git a/packages/opencode/src/altimate/native/local/schema-sync.ts b/packages/opencode/src/altimate/native/local/schema-sync.ts index a0b53a5838..9af941357a 100644 --- a/packages/opencode/src/altimate/native/local/schema-sync.ts +++ b/packages/opencode/src/altimate/native/local/schema-sync.ts @@ -96,7 +96,10 @@ export async function syncSchema(params: LocalSchemaSyncParams): Promise","warehouse":"WH","database":"db"}. (4) SSO: {"type":"snowflake","account":"xy12345","user":"admin","authenticator":"externalbrowser","warehouse":"WH","database":"db"}. IMPORTANT: For private key file paths, always use "private_key_path" (not "private_key").`, ), diff --git a/packages/opencode/test/altimate/drivers-e2e.test.ts b/packages/opencode/test/altimate/drivers-e2e.test.ts index 77a857eaf3..36df1f3a54 100644 --- a/packages/opencode/test/altimate/drivers-e2e.test.ts +++ b/packages/opencode/test/altimate/drivers-e2e.test.ts @@ -304,7 +304,9 @@ describe("DuckDB Driver E2E", () => { const dbFile = join(tmpDir, "test.duckdb") try { const mod = await import("@altimateai/drivers/duckdb") - const fileConn = await mod.connect({ type: "duckdb", path: dbFile }) + // First open materializes the store, so it opts in to create; the + // reopen below deliberately does NOT, proving the file really persisted. + const fileConn = await mod.connect({ type: "duckdb", path: dbFile, create: true }) await fileConn.connect() await fileConn.execute("CREATE TABLE persist (x INT)") @@ -495,7 +497,8 @@ describe("SQLite Driver E2E", () => { tmpDir = mkdtempSync(join(tmpdir(), "sqlite-test-")) const dbFile = join(tmpDir, "test.sqlite") const mod = await import("@altimateai/drivers/sqlite") - connector = await mod.connect({ type: "sqlite", path: dbFile }) + // This suite materializes its own scratch store, so it opts in to create. + connector = await mod.connect({ type: "sqlite", path: dbFile, create: true }) await connector.connect() }) @@ -648,7 +651,7 @@ describe("SQLite Driver E2E", () => { const dbFile = join(tmpDir2, "close.sqlite") try { const mod = await import("@altimateai/drivers/sqlite") - const conn = await mod.connect({ type: "sqlite", path: dbFile }) + const conn = await mod.connect({ type: "sqlite", path: dbFile, create: true }) await conn.connect() await conn.execute("SELECT 1") await conn.close() diff --git a/packages/opencode/test/altimate/fixtures/store-path-probe.ts b/packages/opencode/test/altimate/fixtures/store-path-probe.ts new file mode 100644 index 0000000000..2033fa5730 --- /dev/null +++ b/packages/opencode/test/altimate/fixtures/store-path-probe.ts @@ -0,0 +1,76 @@ +/** + * Compiled-binary probe for warehouse store path resolution. + * + * This entrypoint is compiled with `bun build --compile` using the same + * options the production binary uses (see packages/opencode/script/build.ts): + * bundled sources, `duckdb` left external, no bunfig/dotenv autoload. It then + * reproduces the exact `--dir` handling of `altimate-code run` + * (packages/opencode/src/cli/cmd/run.ts) — `process.chdir(args.dir)` — before + * touching the connection registry. + * + * Running it from an unrelated cwd is the only way to observe the real + * resolution behaviour: a `bun test` run keeps the package's own node_modules + * reachable and its cwd is not the rig's, so it cannot see this defect. + * + * Output is a single line of JSON so the harness can assert on it. + */ + +import * as Registry from "../../../src/altimate/native/connections/registry" + +function arg(name: string): string | undefined { + const idx = process.argv.indexOf(`--${name}`) + if (idx === -1 || idx === process.argv.length - 1) return undefined + return process.argv[idx + 1] +} + +async function main() { + // Load the native duckdb addon before any chdir. In the shipped binary the + // `bin/altimate` wrapper makes `duckdb` resolvable via NODE_PATH; here it is + // bundled, and Bun locates its embedded `.node` relative to the startup cwd. + // Warming the module cache first keeps that resolution out of the experiment — + // the behaviour under test (path resolution and create-on-open) all happens + // inside `new duckdb.Database(...)`, well after this point. + await import("duckdb").catch(() => {}) + + const connection = arg("connection") ?? "probe" + const schema = arg("schema") ?? "main" + + const read = async () => { + const connector = await Registry.get(connection) + const tables = await connector.listTables(schema) + await connector.close() + return tables.map((t) => t.name).sort() + } + + // `--instance-dir` models a server or `run --attach` request: the working + // directory stays where the process started and the project arrives through + // the instance context instead (server.ts wraps every request this way). + // `--dir` models the local `run --dir` path, which chdirs (run.ts). + const instanceDir = arg("instance-dir") + const dir = arg("dir") + if (!instanceDir && dir) process.chdir(dir) + + try { + const tables = instanceDir + ? await ( + await import("../../../src/project/instance") + ).Instance.provide({ + directory: instanceDir, + fn: read, + }) + : await read() + console.log(JSON.stringify({ ok: true, cwd: process.cwd(), tables })) + } catch (e) { + console.log( + JSON.stringify({ + ok: false, + cwd: process.cwd(), + error: e instanceof Error ? e.message : String(e), + }), + ) + } +} + +await main() +// The duckdb native addon keeps handles alive; exit explicitly. +process.exit(0) diff --git a/packages/opencode/test/altimate/schema-cache.test.ts b/packages/opencode/test/altimate/schema-cache.test.ts index 55c70f1d8b..2d5ac92bd2 100644 --- a/packages/opencode/test/altimate/schema-cache.test.ts +++ b/packages/opencode/test/altimate/schema-cache.test.ts @@ -644,7 +644,7 @@ describe("SQLite driver PRAGMA handling", () => { test("PRAGMA statements work without LIMIT clause error", async () => { const { connect } = await import("@altimateai/drivers/sqlite") const dbPath = join(tmpDir, "pragma-test.db") - const connector = await connect({ type: "sqlite", path: dbPath }) + const connector = await connect({ type: "sqlite", path: dbPath, create: true }) await connector.connect() // These should all work without "near LIMIT: syntax error" @@ -664,7 +664,7 @@ describe("SQLite driver PRAGMA handling", () => { test("SELECT statements still get LIMIT applied", async () => { const { connect } = await import("@altimateai/drivers/sqlite") const dbPath = join(tmpDir, "limit-test.db") - const connector = await connect({ type: "sqlite", path: dbPath }) + const connector = await connect({ type: "sqlite", path: dbPath, create: true }) await connector.connect() await connector.execute("CREATE TABLE nums (n INTEGER)") @@ -691,7 +691,7 @@ describe("SQLite driver readonly connections", () => { const dbPath = join(tmpDir, "readonly-test.db") // Create a database with data first - const writer = await connect({ type: "sqlite", path: dbPath }) + const writer = await connect({ type: "sqlite", path: dbPath, create: true }) await writer.connect() await writer.execute("CREATE TABLE items (id INTEGER, name TEXT)") await writer.execute("INSERT INTO items VALUES (1, 'test')") @@ -710,7 +710,7 @@ describe("SQLite driver readonly connections", () => { const dbPath = join(tmpDir, "readonly-write-test.db") // Create a database first - const writer = await connect({ type: "sqlite", path: dbPath }) + const writer = await connect({ type: "sqlite", path: dbPath, create: true }) await writer.connect() await writer.execute("CREATE TABLE items (id INTEGER)") await writer.close() diff --git a/packages/opencode/test/altimate/store-path-resolution.test.ts b/packages/opencode/test/altimate/store-path-resolution.test.ts new file mode 100644 index 0000000000..fec5d3870b --- /dev/null +++ b/packages/opencode/test/altimate/store-path-resolution.test.ts @@ -0,0 +1,294 @@ +/** + * Regression tests for warehouse store path resolution and create-on-open. + * + * Reported defect: `altimate-code run --dir ` made a populated DuckDB + * store read as empty, with no error. `--dir` calls `process.chdir()` + * (src/cli/cmd/run.ts), a relative store path in a connection config resolved + * against the new working directory, and the file-backed driver answered the + * miss by CREATING an empty database. Every query then succeeded and returned + * nothing. + * + * These tests run a COMPILED binary built with the production build options + * (packages/opencode/script/build.ts: bundled sources, warehouse SDKs external, + * no bunfig/dotenv autoload), invoked with `--dir` from an unrelated working + * directory. An in-process `bun test` cannot see this defect: the package's own + * node_modules stays reachable and the cwd is the test runner's, not the rig's. + * + * The store is driven through SQLite rather than DuckDB so the binary needs no + * native addon — `bun:sqlite` is built in. The resolution code under test is + * type-agnostic (see FILE_STORE_TYPES in native/connections/registry.ts) and + * both engines create-on-open, so SQLite exercises the same two defects. + * The DuckDB driver's own guard is covered in + * packages/drivers/test/file-store-guard.test.ts. + */ + +import { describe, expect, test, beforeAll, afterAll } from "bun:test" +import { Database } from "bun:sqlite" +import { spawnSync } from "child_process" +import * as fs from "fs" +import * as os from "os" +import * as path from "path" + +const OPENCODE_ROOT = path.resolve(import.meta.dir, "../..") +const PROBE = path.join(OPENCODE_ROOT, "test/altimate/fixtures/store-path-probe.ts") + +/** An unguessable name — a pass cannot come from anything but reading the store. */ +const CANARY_TABLE = "zorbulax_ledger" + +let rig: string +let binary: string + +/** Compile the probe the way the shipped binary is compiled. */ +function compileProbe(outfile: string) { + const result = spawnSync( + process.execPath, + [ + "build", + "--compile", + "--conditions=browser", + "--tsconfig-override", + "./tsconfig.json", + "--target=bun", + // Match script/build.ts's compile options exactly. + "--no-compile-autoload-bunfig", + "--no-compile-autoload-dotenv", + "--compile-autoload-tsconfig", + "--compile-autoload-package-json", + // Mirrors optionalExternals in script/build.ts — the warehouse SDKs are + // native addons the user installs on demand, never bundled. + ...[ + "pg", + "snowflake-sdk", + "@google-cloud/bigquery", + "@databricks/sql", + "mysql2", + "mssql", + "oracledb", + "duckdb", + "keytar", + "ssh2", + "dockerode", + ].flatMap((pkg) => ["--external", pkg]), + "--define", + "OPENCODE_VERSION='0.0.0-test'", + "--define", + "OPENCODE_CHANNEL='test'", + "--define", + "OPENCODE_LIBC=undefined", + "--define", + "OPENCODE_MIGRATIONS=[]", + "--define", + "OPENCODE_BUILTIN_SKILLS=[]", + "--define", + "OPENCODE_CHANGELOG=[]", + "--outfile", + outfile, + PROBE, + ], + { cwd: OPENCODE_ROOT, encoding: "utf-8" }, + ) + if (result.status !== 0 || !fs.existsSync(outfile)) { + throw new Error(`probe compile failed (${result.status}):\n${result.stdout}\n${result.stderr}`) + } +} + +/** Run the compiled binary from `cwd`, with `home` as its HOME. */ +function runProbe(args: string[], opts: { cwd: string; home: string }) { + // `ALTIMATE_CODE_CONN_*` variables override both config files, so an ambient + // one on the developer's or CI's environment would decide these assertions + // instead of the config the test wrote. Strip them. + const env: Record = {} + for (const [key, value] of Object.entries(process.env)) { + if (!key.startsWith("ALTIMATE_CODE_CONN_")) env[key] = value + } + + const result = spawnSync(binary, args, { + cwd: opts.cwd, + encoding: "utf-8", + env: { + ...env, + HOME: opts.home, + USERPROFILE: opts.home, + // Global.Path derives data/cache/config/state from xdg-basedir, which + // prefers XDG_* over HOME. Leaving those ambient would put this probe's + // project storage outside the rig on any machine that sets them, so the + // arm that boots an instance would depend on shared state and leave dirs + // behind. Pin them inside the rig alongside HOME. + XDG_DATA_HOME: path.join(opts.home, ".local", "share"), + XDG_CACHE_HOME: path.join(opts.home, ".cache"), + XDG_CONFIG_HOME: path.join(opts.home, ".config"), + XDG_STATE_HOME: path.join(opts.home, ".local", "state"), + OPENCODE_TEST_HOME: opts.home, + ALTIMATE_TELEMETRY_DISABLED: "1", + OPENCODE_DISABLE_AUTOUPDATE: "1", + }, + }) + const line = (result.stdout ?? "") + .split("\n") + .reverse() + .find((l) => l.trim().startsWith("{")) + if (!line) throw new Error(`probe produced no JSON:\n${result.stdout}\n${result.stderr}`) + return JSON.parse(line) as { ok: boolean; cwd: string; tables?: string[]; error?: string } +} + +/** Every file under `dir` that looks like a database, ignoring `skip`. */ +function databaseFiles(dir: string, skip: string[] = []): string[] { + const found: string[] = [] + const walk = (current: string) => { + for (const entry of fs.readdirSync(current, { withFileTypes: true })) { + const full = path.join(current, entry.name) + if (entry.isDirectory()) walk(full) + else if (/\.(db|sqlite|duckdb)(-wal|-shm|\.wal)?$/.test(entry.name) && !skip.includes(full)) found.push(full) + } + } + walk(dir) + return found +} + +/** Write a connections.json into `home`'s global config directory. */ +function writeGlobalConfig(home: string, config: Record) { + const dir = path.join(home, ".altimate-code") + fs.mkdirSync(dir, { recursive: true }) + fs.writeFileSync(path.join(dir, "connections.json"), JSON.stringify(config, null, 2)) +} + +// Compiling the probe takes ~2s locally; allow generous headroom on slow CI +// machines so the hook cannot time out and mask a real result. +beforeAll(() => { + rig = fs.mkdtempSync(path.join(os.tmpdir(), "store-path-")) + binary = path.join(rig, "probe-bin") + compileProbe(binary) +}, 180_000) + +afterAll(() => { + if (rig) fs.rmSync(rig, { recursive: true, force: true }) +}) + +describe("warehouse store path resolution", () => { + test("a populated store reached through --dir from an unrelated cwd returns its real tables", () => { + const home = path.join(rig, "case1-home") + const project = path.join(rig, "case1-project") + const unrelatedCwd = path.join(rig, "case1-cwd") + fs.mkdirSync(path.join(home, ".altimate-code"), { recursive: true }) + fs.mkdirSync(project, { recursive: true }) + fs.mkdirSync(unrelatedCwd, { recursive: true }) + + // The store lives beside the global config that names it, under a RELATIVE + // path — the shape that made the reported case silently read as empty. + const store = path.join(home, ".altimate-code", "warehouse.db") + const seed = new Database(store, { create: true }) + seed.exec(`CREATE TABLE ${CANARY_TABLE}(id INTEGER, memo TEXT)`) + seed.exec(`INSERT INTO ${CANARY_TABLE} VALUES (1, 'real')`) + seed.close() + + writeGlobalConfig(home, { probe: { type: "sqlite", path: "warehouse.db" } }) + + const result = runProbe(["--connection", "probe", "--dir", project], { cwd: unrelatedCwd, home }) + + expect(result.cwd).toBe(fs.realpathSync(project)) + expect(result.ok).toBe(true) + expect(result.tables).toContain(CANARY_TABLE) + + // Nothing may have been conjured under --dir or the invoking directory. + expect(databaseFiles(project)).toEqual([]) + expect(databaseFiles(unrelatedCwd)).toEqual([]) + }) + + test("a missing store fails loudly instead of reading as empty", () => { + const home = path.join(rig, "case2-home") + const project = path.join(rig, "case2-project") + const absent = path.join(rig, "case2-absent") + fs.mkdirSync(path.join(home, ".altimate-code"), { recursive: true }) + fs.mkdirSync(project, { recursive: true }) + fs.mkdirSync(absent, { recursive: true }) + + const missing = path.join(absent, "definitely-absent.db") + writeGlobalConfig(home, { probe: { type: "sqlite", path: missing } }) + + const result = runProbe(["--connection", "probe", "--dir", project], { cwd: project, home }) + + expect(result.ok).toBe(false) + expect(result.error).toContain("not found") + expect(result.error).toContain("definitely-absent.db") + + // The whole point: no database was conjured at the missing path. + expect(fs.existsSync(missing)).toBe(false) + expect(databaseFiles(absent)).toEqual([]) + expect(databaseFiles(project)).toEqual([]) + }) + + test("a project-local config resolves against the --dir project, not the invoking cwd", () => { + const home = path.join(rig, "case5-home") + const project = path.join(rig, "case5-project") + const unrelatedCwd = path.join(rig, "case5-cwd") + fs.mkdirSync(path.join(home, ".altimate-code"), { recursive: true }) + fs.mkdirSync(path.join(project, ".altimate-code"), { recursive: true }) + fs.mkdirSync(unrelatedCwd, { recursive: true }) + + const store = new Database(path.join(project, "warehouse.db"), { create: true }) + store.exec(`CREATE TABLE ${CANARY_TABLE}(id INTEGER)`) + store.close() + + fs.writeFileSync( + path.join(project, ".altimate-code", "connections.json"), + JSON.stringify({ probe: { type: "sqlite", path: "warehouse.db" } }), + ) + + const result = runProbe(["--connection", "probe", "--dir", project], { cwd: unrelatedCwd, home }) + + expect(result.ok).toBe(true) + expect(result.tables).toContain(CANARY_TABLE) + expect(databaseFiles(unrelatedCwd)).toEqual([]) + }) + + test("a server-style request resolves against its instance directory, not the launch cwd", () => { + // A server or `run --attach` request never chdirs: the project arrives in + // the instance context while the working directory stays where the process + // was started. Reading process.cwd() there picks up the launch directory, + // which is somebody else's project. + const home = path.join(rig, "case6-home") + const project = path.join(rig, "case6-project") + const launchCwd = path.join(rig, "case6-launch-cwd") + fs.mkdirSync(path.join(home, ".altimate-code"), { recursive: true }) + fs.mkdirSync(path.join(project, ".altimate-code"), { recursive: true }) + fs.mkdirSync(path.join(launchCwd, ".altimate-code"), { recursive: true }) + + const real = new Database(path.join(project, "warehouse.db"), { create: true }) + real.exec(`CREATE TABLE ${CANARY_TABLE}(id INTEGER)`) + real.close() + + // A decoy project config and store sit in the launch directory. + const decoy = new Database(path.join(launchCwd, "warehouse.db"), { create: true }) + decoy.exec("CREATE TABLE decoy_table(id INTEGER)") + decoy.close() + for (const dir of [project, launchCwd]) { + fs.writeFileSync( + path.join(dir, ".altimate-code", "connections.json"), + JSON.stringify({ probe: { type: "sqlite", path: "warehouse.db" } }), + ) + } + + const result = runProbe(["--connection", "probe", "--instance-dir", project], { cwd: launchCwd, home }) + + expect(result.cwd).toBe(fs.realpathSync(launchCwd)) + expect(result.ok).toBe(true) + expect(result.tables).toContain(CANARY_TABLE) + expect(result.tables).not.toContain("decoy_table") + }) + + test("an explicit create: true still materializes a store", () => { + const home = path.join(rig, "case3-home") + const project = path.join(rig, "case3-project") + fs.mkdirSync(path.join(home, ".altimate-code"), { recursive: true }) + fs.mkdirSync(project, { recursive: true }) + + const target = path.join(project, "scratch.db") + writeGlobalConfig(home, { probe: { type: "sqlite", path: target, create: true } }) + + const result = runProbe(["--connection", "probe"], { cwd: project, home }) + + expect(result.ok).toBe(true) + expect(result.tables).toEqual([]) + expect(fs.existsSync(target)).toBe(true) + }) +}) From aef0ad5209b0296dbaade889d6d15348cb257a84 Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Sun, 30 Aug 2026 08:21:12 -0700 Subject: [PATCH 2/2] fix: [#1203] a broken warehouse connection reached the agent as "(0 rows)" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Field verification against the real rig showed the first version of this fix did not help: `--dir` still returned "no tables", nothing was created anywhere on disk, and `assertStoreExists()` produced no visible error even when pointed at a store that does not exist. The diagnosis was wrong, not the measurement. A failure was shaped like success at every layer between the driver and the model, so three separate things had to be true for the agent to be misled, and all three were: 1. `drivers/src/duckdb.ts` and `drivers/src/sqlite.ts` read `(config.path as string) ?? ":memory:"`. Any failure to carry a path — a config the registry never loaded, a field under another name, a lookup that fell through — became a successful connection to an empty in-memory database. That is worse than the silent creation this PR already closed: a stray file can at least be found afterwards, whereas this leaves nothing on disk to explain the empty answer. `requireStorePath()` now rejects a missing or empty path. `:memory:` is still available, but only when asked for. 2. `sql.execute` never throws. It catches every connection and query error and returns `{ columns: [], rows: [], row_count: 0, error }` (native/connections/register.ts:437). So the loud error from (1) and from `assertStoreExists()` was demoted to a field on a success-shaped result. 3. `formatResult()` in `tools/sql-execute.ts` returned the literal string `"(0 rows)"` whenever `row_count === 0` and never read `error`. An agent running `SELECT ... FROM information_schema.tables` against an unresolvable warehouse was told, in effect, that the warehouse was healthy and empty — then offered a tip about query optimization. `sql_execute` now surfaces the error the way `schema_inspect` already did, via `normalizeError`. So `assertStoreExists()` was never bypassed. It fired, and its message was thrown away one layer above. A guard whose failure cannot reach the caller is not a guard, which is why the new tests drive the layers the product actually uses: the compiled-binary arms now go through `Dispatcher.call("sql.execute")` rather than reaching the driver via `Registry.get()`, and the rendering is tested through the `sql_execute` tool itself. `test/altimate/warehouse-failure-visibility.test.ts` covers all three: a pathless config, a missing store, and an error-carrying result that must not render as `(0 rows)` — plus the two cases that must NOT change, an explicit `:memory:` and a genuinely empty result set from a healthy warehouse. Three of its six fail on unmodified main, as do three of the five compiled-binary arms. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01VqnuBDGkh1ZT65Ti7e6DHZ --- packages/drivers/src/duckdb.ts | 6 +- packages/drivers/src/file-store.ts | 25 +++ packages/drivers/src/index.ts | 2 +- packages/drivers/src/sqlite.ts | 6 +- .../src/altimate/tools/sql-execute.ts | 25 ++- .../altimate/fixtures/store-path-probe.ts | 38 ++++- .../warehouse-failure-visibility.test.ts | 158 ++++++++++++++++++ 7 files changed, 247 insertions(+), 13 deletions(-) create mode 100644 packages/opencode/test/altimate/warehouse-failure-visibility.test.ts diff --git a/packages/drivers/src/duckdb.ts b/packages/drivers/src/duckdb.ts index 0c7e8e99a8..2854491d89 100644 --- a/packages/drivers/src/duckdb.ts +++ b/packages/drivers/src/duckdb.ts @@ -2,7 +2,7 @@ * DuckDB driver using the `duckdb` package. */ -import { assertStoreExists } from "./file-store" +import { assertStoreExists, requireStorePath } from "./file-store" import type { ConnectionConfig, Connector, ConnectorResult, ExecuteOptions, SchemaColumn } from "./types" import { loadOptionalDriver } from "./resolve" @@ -11,7 +11,9 @@ export async function connect(config: ConnectionConfig): Promise { duckdb = await loadOptionalDriver("duckdb", "duckdb") duckdb = duckdb.default || duckdb - const dbPath = (config.path as string) ?? ":memory:" + // altimate_change start — a missing path must fail loudly, not become :memory: + const dbPath = requireStorePath(config, "DuckDB") + // altimate_change end let db: any let connection: any diff --git a/packages/drivers/src/file-store.ts b/packages/drivers/src/file-store.ts index 2adda5bef5..4c4ad3445c 100644 --- a/packages/drivers/src/file-store.ts +++ b/packages/drivers/src/file-store.ts @@ -39,6 +39,31 @@ export function isLocalFilePath(dbPath: string): boolean { return true } +/** + * The store path a file-backed connection names, or a loud failure. + * + * Both drivers used to read `(config.path as string) ?? ":memory:"`. That turns + * ANY failure to carry a path — a config the registry never loaded, a field + * under the wrong name, a lookup that fell through — into a successful + * connection over an empty in-memory database. Every query then returns no rows + * and no error, which reads as a healthy warehouse that happens to be empty. + * + * It is a worse failure than creating a store on disk: a stray file can at + * least be found afterwards, whereas an in-memory database leaves nothing + * behind to explain the empty answer. `:memory:` remains available, but only + * when a caller asks for it by name. + */ +export function requireStorePath(config: ConnectionConfig, engine: string): string { + const value = config.path + if (typeof value === "string" && value !== "") return value + throw new Error( + `${engine} connection is missing its "path". A file-backed warehouse must name its database explicitly — ` + + `falling back to an in-memory database would answer every query with no rows and no error, ` + + `which is indistinguishable from a healthy but empty warehouse. ` + + `Set "path" to the database file, or to ":memory:" if a throwaway empty database is genuinely what you want.`, + ) +} + /** Whether the caller explicitly opted in to creating the store. */ export function allowsCreate(config: ConnectionConfig): boolean { return config.create === true diff --git a/packages/drivers/src/index.ts b/packages/drivers/src/index.ts index 21f9c50bf8..2e10ca75d0 100644 --- a/packages/drivers/src/index.ts +++ b/packages/drivers/src/index.ts @@ -5,7 +5,7 @@ export type { Connector, ConnectorResult, SchemaColumn, ConnectionConfig } from export { normalizeConfig, sanitizeConnectionString } from "./normalize" // Re-export file-backed store guards -export { allowsCreate, assertStoreExists, isLocalFilePath } from "./file-store" +export { allowsCreate, assertStoreExists, isLocalFilePath, requireStorePath } from "./file-store" // Re-export driver connect functions export { connect as connectPostgres } from "./postgres" diff --git a/packages/drivers/src/sqlite.ts b/packages/drivers/src/sqlite.ts index aaecfdf544..3c9da882fd 100644 --- a/packages/drivers/src/sqlite.ts +++ b/packages/drivers/src/sqlite.ts @@ -4,11 +4,13 @@ */ import { Database } from "bun:sqlite" -import { allowsCreate, assertStoreExists } from "./file-store" +import { allowsCreate, assertStoreExists, requireStorePath } from "./file-store" import type { ConnectionConfig, Connector, ConnectorResult, ExecuteOptions, SchemaColumn } from "./types" export async function connect(config: ConnectionConfig): Promise { - const dbPath = (config.path as string) ?? ":memory:" + // altimate_change start — a missing path must fail loudly, not become :memory: + const dbPath = requireStorePath(config, "SQLite") + // altimate_change end let db: Database | null = null return { diff --git a/packages/opencode/src/altimate/tools/sql-execute.ts b/packages/opencode/src/altimate/tools/sql-execute.ts index 4647c75648..4b18e13c79 100644 --- a/packages/opencode/src/altimate/tools/sql-execute.ts +++ b/packages/opencode/src/altimate/tools/sql-execute.ts @@ -13,6 +13,9 @@ import { PostConnectSuggestions } from "./post-connect-suggestions" import { getCache } from "../native/schema/cache" import * as Registry from "../native/connections/registry" // altimate_change end +// altimate_change start — never render a warehouse failure as an empty result +import { normalizeError } from "./response-normalization" +// altimate_change end export const SqlExecuteTool = Tool.define("sql_execute", { description: "Execute SQL against a connected data warehouse. Returns results as a formatted table.", @@ -53,6 +56,22 @@ export const SqlExecuteTool = Tool.define("sql_execute", { limit: args.limit, }) + // altimate_change start — a failure must not be rendered as "(0 rows)". + // sql.execute never throws: it catches every connection and query error + // and returns a result-shaped object carrying `error`, so an unresolvable + // warehouse used to reach the agent as a successful empty table with no + // fault string at all. Surface it the way schema_inspect already does. + const responseError = normalizeError((result as SqlExecuteResult & { error?: unknown }).error) + if (responseError !== undefined) { + const msg = responseError.trim() || "SQL execution failed." + return { + title: "SQL: ERROR", + metadata: { rowCount: 0, truncated: false, error: msg }, + output: `Failed to execute SQL: ${msg}`, + } + } + // altimate_change end + let output = formatResult(result) // altimate_change start — emit SQL structure fingerprint telemetry try { @@ -115,7 +134,11 @@ interface PreValidationResult { error?: string } -async function preValidateSql(sql: string, warehouse: string | undefined, queryType: string): Promise { +async function preValidateSql( + sql: string, + warehouse: string | undefined, + queryType: string, +): Promise { const startTime = Date.now() // Yield the event loop before heavy synchronous SQLite work so concurrent // tasks aren't blocked. Bun's sqlite API is sync and listColumns can touch diff --git a/packages/opencode/test/altimate/fixtures/store-path-probe.ts b/packages/opencode/test/altimate/fixtures/store-path-probe.ts index 2033fa5730..f7ee1cc93b 100644 --- a/packages/opencode/test/altimate/fixtures/store-path-probe.ts +++ b/packages/opencode/test/altimate/fixtures/store-path-probe.ts @@ -35,11 +35,29 @@ async function main() { const connection = arg("connection") ?? "probe" const schema = arg("schema") ?? "main" - const read = async () => { - const connector = await Registry.get(connection) - const tables = await connector.listTables(schema) - await connector.close() - return tables.map((t) => t.name).sort() + // `--via dispatcher` (the default) drives the same entry point the CLI's + // sql_execute tool uses: Dispatcher.call -> sql.execute -> Registry -> driver. + // Reaching the driver any other way can pass while the product path is broken, + // which is exactly how a guard gets shipped that never runs. + const via = arg("via") ?? "dispatcher" + + const read = async (): Promise<{ tables: string[]; error?: string }> => { + if (via === "registry") { + const connector = await Registry.get(connection) + const tables = await connector.listTables(schema) + await connector.close() + return { tables: tables.map((t) => t.name).sort() } + } + // Import the native index, not the dispatcher module: the index is what + // installs the lazy registration hook in production. Importing the + // dispatcher alone yields "No native handler for sql.execute". + const { Dispatcher } = await import("../../../src/altimate/native") + const result = (await Dispatcher.call("sql.execute", { + sql: "SELECT name FROM sqlite_master WHERE type = 'table'", + warehouse: connection, + })) as { rows?: unknown[][]; error?: unknown } + if (result.error !== undefined) return { tables: [], error: String(result.error) } + return { tables: (result.rows ?? []).map((r) => String(r[0])).sort() } } // `--instance-dir` models a server or `run --attach` request: the working @@ -51,7 +69,7 @@ async function main() { if (!instanceDir && dir) process.chdir(dir) try { - const tables = instanceDir + const outcome = instanceDir ? await ( await import("../../../src/project/instance") ).Instance.provide({ @@ -59,7 +77,13 @@ async function main() { fn: read, }) : await read() - console.log(JSON.stringify({ ok: true, cwd: process.cwd(), tables })) + console.log( + JSON.stringify( + outcome.error !== undefined + ? { ok: false, cwd: process.cwd(), error: outcome.error } + : { ok: true, cwd: process.cwd(), tables: outcome.tables }, + ), + ) } catch (e) { console.log( JSON.stringify({ diff --git a/packages/opencode/test/altimate/warehouse-failure-visibility.test.ts b/packages/opencode/test/altimate/warehouse-failure-visibility.test.ts new file mode 100644 index 0000000000..d8fc90753b --- /dev/null +++ b/packages/opencode/test/altimate/warehouse-failure-visibility.test.ts @@ -0,0 +1,158 @@ +/** + * A warehouse failure must never reach the agent as an empty result. + * + * Field report against the real rig: a compiled binary returned "no tables" + * with no error and no file created anywhere on disk. The cause was not the + * silent-creation path — it is that a failure was shaped like success all the + * way to the model: + * + * 1. `packages/drivers/src/duckdb.ts` read `(config.path as string) ?? ":memory:"`, + * so a config that arrived without a `path` became an in-memory database. + * Nothing is created, nothing is on disk, every query returns no rows. + * 2. `sql.execute` never throws. It catches every connection and query error + * and returns `{ columns: [], rows: [], row_count: 0, error }` + * (native/connections/register.ts). + * 3. `formatResult()` rendered that as the literal string `"(0 rows)"` and + * never read `error` — so the agent saw a healthy empty table. + * + * The layers are tested where each one breaks: the guard against the real + * registry, and the rendering against a stubbed `sql.execute` that returns the + * error-carrying shape the real handler produces. The stub is deliberate — + * `Dispatcher.reset()` leaks between test files in Bun, and the true + * end-to-end through the live dispatcher is covered by the compiled-binary + * test in store-path-resolution.test.ts, which runs in its own process. + */ + +import { afterEach, beforeEach, describe, expect, test } from "bun:test" +import { Database } from "bun:sqlite" +import * as fs from "fs" +import * as os from "os" +import * as path from "path" +import { initTool } from "./tool-fixture" +import * as Dispatcher from "../../src/altimate/native/dispatcher" +import * as Registry from "../../src/altimate/native/connections/registry" +import { SqlExecuteTool } from "../../src/altimate/tools/sql-execute" + +/** Unguessable, so a pass cannot come from anything but the real store. */ +const CANARY_TABLE = "zorbulax_ledger" + +const ctx = { + sessionID: "test-session", + messageID: "test-message", + callID: "test-call", + agent: "test", + abort: new AbortController().signal, + messages: [], + metadata: () => {}, +} + +const tmpDirs: string[] = [] +function tmp(): string { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), "warehouse-failure-")) + tmpDirs.push(dir) + return dir +} + +beforeEach(() => { + process.env.ALTIMATE_TELEMETRY_DISABLED = "true" + Registry.reset() +}) + +afterEach(() => { + Registry.reset() + while (tmpDirs.length) fs.rmSync(tmpDirs.pop()!, { recursive: true, force: true }) +}) + +const LIST_TABLES = `SELECT name FROM sqlite_master WHERE type = 'table'` + +describe("a file-backed connection that cannot be resolved fails loudly", () => { + test("a config with no path is rejected instead of becoming an in-memory database", async () => { + // The reported shape: nothing on disk, no error, every query empty. A + // `?? ":memory:"` default turns any failure to carry a path into a + // successful query against nothing. + Registry.setConfigs({ wh: { type: "sqlite" } }) + + await expect(Registry.get("wh")).rejects.toThrow(/path/) + }) + + test("a missing store file is rejected, and no store is conjured", async () => { + const missing = path.join(tmp(), "definitely-absent.db") + Registry.setConfigs({ wh: { type: "sqlite", path: missing } }) + + await expect(Registry.get("wh")).rejects.toThrow(/not found/) + expect(fs.existsSync(missing)).toBe(false) + }) + + test("an explicit :memory: is still honoured — it just has to be asked for", async () => { + Registry.setConfigs({ wh: { type: "sqlite", path: ":memory:" } }) + + const connector = await Registry.get("wh") + const result = await connector.execute("SELECT 1 AS n") + expect(result.rows[0][0]).toBe(1) + }) + + test("a populated store still opens and returns its real tables", async () => { + const store = path.join(tmp(), "warehouse.db") + const seed = new Database(store, { create: true }) + seed.exec(`CREATE TABLE ${CANARY_TABLE}(id INTEGER)`) + seed.close() + Registry.setConfigs({ wh: { type: "sqlite", path: store } }) + + const connector = await Registry.get("wh") + const result = await connector.execute(LIST_TABLES) + expect(result.rows.map((r) => r[0])).toContain(CANARY_TABLE) + }) +}) + +describe("sql_execute renders a warehouse failure as a failure", () => { + // Reset first, then register: `call()` runs the lazy registration hook + // before dispatching, and that would re-import the real handler and clobber + // the stub. This is the same order tool-response-normalization.test.ts uses. + beforeEach(() => { + Dispatcher.reset() + }) + + /** The exact shape sql.execute returns for any connection or query error. */ + function stubFailure(message: string) { + Dispatcher.register("sql.execute" as any, async () => ({ + columns: [], + rows: [], + row_count: 0, + truncated: false, + error: message, + })) + } + + test("an error-carrying result is reported, not printed as (0 rows)", async () => { + // This is the step that turned a broken connection into "no tables": + // formatResult() returned "(0 rows)" for row_count === 0 and never looked + // at `error`, so the model was told the warehouse was simply empty. + stubFailure('SQLite database file not found: "/nowhere/absent.db".') + + const tool = await initTool(SqlExecuteTool) + const result = await tool.execute({ query: LIST_TABLES }, ctx as any) + + expect(result.output).not.toContain("(0 rows)") + expect(result.title).toContain("ERROR") + expect(result.output).toContain("not found") + expect(result.metadata.error).toBeDefined() + }) + + test("a genuinely empty result set is still reported as empty, not as an error", async () => { + // The distinction that matters: zero rows from a healthy warehouse is not + // a failure, and must not start reporting as one. + Dispatcher.register("sql.execute" as any, async () => ({ + columns: ["name"], + rows: [], + row_count: 0, + truncated: false, + })) + + const tool = await initTool(SqlExecuteTool) + const result = await tool.execute({ query: LIST_TABLES }, ctx as any) + + expect(result.title).not.toContain("ERROR") + expect(result.output).toContain("(0 rows)") + expect(result.metadata.error).toBeUndefined() + }) +})