diff --git a/docs/docs/configure/warehouses.md b/docs/docs/configure/warehouses.md index d85136be4..02bdff894 100644 --- a/docs/docs/configure/warehouses.md +++ b/docs/docs/configure/warehouses.md @@ -261,7 +261,7 @@ 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 | +| `path` | Yes | Database file path, or `":memory:"` for in-memory. Cannot be omitted — a missing `path` is rejected rather than silently falling back to `":memory:"` | | `create` | No | Create the database file if it is missing (default: `false`) | !!! warning "The store must already exist" @@ -283,6 +283,15 @@ If you're already authenticated via `gcloud`, omit `credentials_path`: re-point an existing connection at a different file. Absolute paths are always safest. +!!! note "Bare `word:target` values are treated as local files" + A `path` shaped like `word:target` with no `//` (for example + `data:warehouse.duckdb`) is treated as an ordinary local filename by + default. The only exceptions are `md:`, `motherduck:`, and `ducklake:` — + these specific bare prefixes are recognized remote storage schemes and + are always forwarded as remote targets. To force any other value to be + treated as remote, use its full `scheme://` form (`s3://...`, + `md://...`) instead of a bare prefix. + !!! 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. @@ -461,7 +470,7 @@ 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 | +| `path` | Yes | Database file path, or `":memory:"` for in-memory. Cannot be omitted — a missing `path` is rejected rather than silently falling back to `":memory:"` | | `readonly` | No | Open in read-only mode (default: `false`) | | `create` | No | Create the database file if it is missing (default: `false`) | diff --git a/docs/docs/drivers.md b/docs/docs/drivers.md index c741272d3..0d7a388ed 100644 --- a/docs/docs/drivers.md +++ b/docs/docs/drivers.md @@ -194,7 +194,10 @@ MongoDB supports server versions 3.6 through 8.0. Queries use MQL (MongoDB Query 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. +working directory. A bare `word:target` value (no `//`) is treated as a local +filename unless `word` is one of the recognized remote bare schemes +(`md:`, `motherduck:`, `ducklake:`) — use a `scheme://` URI to target any other +storage extension. See [Warehouses](configure/warehouses.md#duckdb) for the full rules. ## SSH Tunneling diff --git a/packages/drivers/src/file-store.ts b/packages/drivers/src/file-store.ts index 4c4ad3445..ce1bad8cc 100644 --- a/packages/drivers/src/file-store.ts +++ b/packages/drivers/src/file-store.ts @@ -13,8 +13,37 @@ */ import * as fs from "fs" +import { fileURLToPath } from "url" import type { ConnectionConfig } from "./types" +// altimate_change start — narrow the scheme exclusion to genuine remote/extension targets +/** + * DuckDB extension schemes that take a bare `scheme:rest` form with no `//` + * — MotherDuck (`md:`) and DuckLake (`ducklake:`) — and so cannot be told + * apart from a local filename by the `://` check below. + * + * Fundamental ambiguity: a bare `word:target` is syntactically identical + * whether `word` is a filename prefix (`data:warehouse.duckdb`, a real local + * file) or a remote extension scheme (`md:my_database`). Nothing in the + * string alone can distinguish them. + * + * Deliberate choice: a closed list, not a broad heuristic. Local filenames + * that happen to contain a colon are the common case here; bare-scheme + * DuckDB extensions are a small, enumerable set. Listing the known ones and + * treating everything else as local is safer than the reverse (treating + * every `word:target` as remote, which misclassified real local files — see + * `isLocalFilePath`'s own comment). + * + * Escape hatch: a custom/future extension whose target uses a bare scheme + * not in this list is NOT forwarded — it's treated as a local path and will + * fail the existence guard. Two ways out: (1) use the extension's `scheme://` + * form if it has one (always forwarded, see the `://` check below), or (2) + * add the new bare scheme to this list once it's an extension the driver + * actually needs to support. + */ +const NON_SLASH_REMOTE_SCHEMES = ["md:", "motherduck:", "ducklake:"] +// altimate_change end + /** * Whether `dbPath` names a file on the local filesystem, and so can be * existence-checked before the driver opens it. @@ -29,16 +58,77 @@ import type { ConnectionConfig } from "./types" * 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. + * missing-extension error rather than silently creating anything. + * + * altimate_change: the exclusion used to fire on ANY two-or-more-letter + * prefix followed by a colon, which misclassified an ordinary local filename + * that happens to contain one — `data:warehouse.duckdb`, `foo:warehouse.db` + * — as a remote target, silently skipping both path resolution and the + * existence guard below. Only a `scheme://` URI or one of the specific + * non-slash extension schemes DuckDB actually recognizes is excluded now; a + * `C:\...` Windows drive letter still passes through unaffected, since + * neither pattern matches it. `file:` is deliberately still excluded here — + * it is a real local path, but resolving/existence-checking it is handled + * separately (see `absoluteFileUriPath` below) because it is not safe to + * treat as an ordinary path string (see registry.ts's `resolveStorePaths`, + * which would otherwise mangle it with `path.resolve`). */ export function isLocalFilePath(dbPath: string): boolean { if (dbPath === "" || dbPath === ":memory:") return false - if (/^[a-zA-Z][a-zA-Z0-9+.-]+:/.test(dbPath)) return false + if (/^file:/i.test(dbPath)) return false + // altimate_change start — a single-letter "scheme" is a Windows drive letter, not a URI + // A doubled-slash Windows path like `C://data/warehouse.duckdb` is a valid absolute + // path (path.win32.normalize collapses it to `C:\data\warehouse.duckdb`), but the + // scheme://-form regex below used to accept a one-character scheme, so `C:` matched + // it exactly like `s3:` does. No real remote/extension scheme is a single letter — + // require at least two characters before "://" so a drive letter is never mistaken + // for one. + if (/^[a-zA-Z][a-zA-Z0-9+.-]+:\/\//.test(dbPath)) return false + // altimate_change end + if (NON_SLASH_REMOTE_SCHEMES.some((scheme) => dbPath.toLowerCase().startsWith(scheme))) return false return true } +// altimate_change start — existence-check absolute `file:` URIs too +/** + * The on-disk path an ABSOLUTE `file:` URI names, or `undefined` if `dbPath` + * is not a `file:` URI, is a relative one, or is one of SQLite/DuckDB's + * in-memory or temporary URI forms (`file:`, `file::memory:`, + * `file:name?mode=memory`) that never touch disk. + * + * Scoped deliberately narrow to the absolute case. This PR does not resolve a + * relative `file:` URI against a base directory (registry.ts's + * `resolveStorePaths` leaves `file:` paths untouched — see `isLocalFilePath` + * above), so guarding a relative one's existence here would check whatever + * the process's current directory happens to be, which is exactly the + * cwd-following bug this PR exists to remove. An absolute `file:` URI names + * one unambiguous location regardless of cwd, so it is safe to check. + */ +export function absoluteFileUriPath(dbPath: string): string | undefined { + if (!/^file:/i.test(dbPath)) return undefined + const rest = dbPath.slice("file:".length) + if (rest === "" || rest.startsWith(":")) return undefined // file:, file::memory: + if (/[?&]mode=memory\b/i.test(dbPath)) return undefined + // altimate_change start — accept any number of leading slashes, not just 1-3. + // `file:////mnt/share/warehouse.duckdb` (four or more slashes — seen with UNC-style + // shares) is a valid absolute file: URI; fileURLToPath handles the extra slashes by + // folding them into the resulting path (verified: `file:////x` -> `//x`), so there is + // no reason to reject it here before even trying to parse it. + const isSlashForm = /^\/+/.test(rest) + // altimate_change end + const isBareWindowsDrive = /^[a-zA-Z]:[\\/]/.test(rest) + if (!isSlashForm && !isBareWindowsDrive) return undefined // relative — not this guard's job + try { + // fileURLToPath requires an authority (even an empty one); `file:C:/x` + // needs a slash inserted before the drive letter to parse as one. + const href = isBareWindowsDrive ? dbPath.replace(/^file:/i, "file:/") : dbPath + return fileURLToPath(href) + } catch { + return undefined + } +} +// altimate_change end + /** * The store path a file-backed connection names, or a loud failure. * @@ -69,9 +159,40 @@ export function allowsCreate(config: ConnectionConfig): boolean { return config.create === true } +// altimate_change start — a directory at dbPath is never a valid store, with or +// without `create` +/** + * Throw if `path` names an existing directory. A directory can never be + * opened as a database by either engine, whether or not the caller passed + * `create: true` — creation only ever means "create a missing FILE", not + * "replace a directory". This must run before the `allowCreate` bypass in + * `assertStoreExists`: `create: true` is meant to skip the "does this file + * exist yet" check, not the "is this actually a directory" check, or a + * misconfigured directory path reaches the driver and fails there with a + * confusing engine-level error instead of this guard's clear one. + */ +function rejectIfDirectory(path: string, displayPath: string, engine: string): void { + let isDir: boolean + try { + isDir = fs.existsSync(path) && fs.statSync(path).isDirectory() + } catch { + // A stat failure here (e.g. a race with a delete, or a permissions error) + // is not this guard's to diagnose — let the driver's own open surface it. + isDir = false + } + if (!isDir) return + throw new Error( + `${engine} database path is a directory, not a file: "${displayPath}". ` + + `A directory can never be opened as a database — this applies even when "create" is set, ` + + `since creation only ever means creating a missing file.`, + ) +} +// altimate_change end + /** - * 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. + * Throw unless the store is safe to open: it is not a directory, and it + * either 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 @@ -84,8 +205,30 @@ export function assertStoreExists( engine: string, allowCreate: boolean = allowsCreate(config), ): void { - if (allowCreate) return + // altimate_change start — existence-check an absolute `file:` URI too; + // `isLocalFilePath` deliberately excludes `file:` (see its own comment), so + // without this branch every `file:` store — including absolute ones that + // name one unambiguous on-disk location regardless of cwd — skipped the + // guard entirely and a missing absolute file: store opened silently empty, + // the exact bug class this guard exists to catch. + const fileUriPath = absoluteFileUriPath(dbPath) + if (fileUriPath !== undefined) { + // altimate_change: the directory check MUST run before the `allowCreate` + // bypass below — see rejectIfDirectory's own comment. + rejectIfDirectory(fileUriPath, dbPath, engine) + if (allowCreate) return + if (fs.existsSync(fileUriPath)) return + throw new Error( + `${engine} database file not found: "${dbPath}" (resolved to "${fileUriPath}"). ` + + `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, or pass "create": true if this store is meant to be created.`, + ) + } + // altimate_change end if (!isLocalFilePath(dbPath)) return + // altimate_change: same ordering requirement as the file: URI branch above. + rejectIfDirectory(dbPath, dbPath, engine) + if (allowCreate) return if (fs.existsSync(dbPath)) return throw new Error( `${engine} database file not found: "${dbPath}". ` + diff --git a/packages/drivers/test/file-store-guard.test.ts b/packages/drivers/test/file-store-guard.test.ts index 391165951..282afe1eb 100644 --- a/packages/drivers/test/file-store-guard.test.ts +++ b/packages/drivers/test/file-store-guard.test.ts @@ -15,7 +15,7 @@ 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" +import { allowsCreate, assertStoreExists, absoluteFileUriPath, isLocalFilePath } from "../src/file-store" const CANARY_TABLE = "zorbulax_ledger" @@ -49,6 +49,23 @@ describe("isLocalFilePath", () => { expect(isLocalFilePath("C:\\data\\warehouse.duckdb")).toBe(true) }) + // altimate_change start — regression: a doubled-slash Windows drive path is a + // real local path, not a `scheme://` URI + test("treats a doubled-slash Windows drive path as local, not a scheme:// URI", () => { + // `C://data/warehouse.duckdb` is a valid (if unusual) absolute Windows path — + // path.win32.normalize collapses it to `C:\data\warehouse.duckdb`. The + // scheme://-form regex used to accept a single-character scheme, so "C" + // matched it exactly like "s3" does in `s3://...`, misclassifying it as + // remote and skipping both path resolution and the existence guard. No real + // remote/extension scheme is a single letter, so requiring 2+ characters + // before "://" fixes this without affecting genuine schemes. + expect(isLocalFilePath("C://data/warehouse.duckdb")).toBe(true) + expect(isLocalFilePath("D://warehouse.duckdb")).toBe(true) + // Genuine two-or-more-character schemes are still excluded. + expect(isLocalFilePath("s3://bucket/warehouse.duckdb")).toBe(false) + }) + // altimate_change end + 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 @@ -57,6 +74,19 @@ describe("isLocalFilePath", () => { expect(isLocalFilePath(":memory")).toBe(true) expect(isLocalFilePath(":foo")).toBe(true) }) + + // altimate_change start — regression: an ordinary filename that merely + // contains a colon must not be misread as a remote scheme + test("a local filename shaped like `scheme:name` (no `//`) is still a local file", () => { + // The exclusion used to match ANY "2+ letter prefix + colon", so a file + // literally named "data:warehouse.duckdb" was misclassified as a remote + // target and silently skipped both path resolution and the existence + // guard. Only a real `scheme://` URI or one of the specific non-slash + // DuckDB extension schemes (md:, motherduck:, ducklake:) should be excluded. + expect(isLocalFilePath("data:warehouse.duckdb")).toBe(true) + expect(isLocalFilePath("foo:warehouse.db")).toBe(true) + }) + // altimate_change end }) describe("assertStoreExists", () => { @@ -90,6 +120,114 @@ describe("assertStoreExists", () => { expect(allowsCreate({ type: "duckdb", create: "true" })).toBe(false) expect(allowsCreate({ type: "duckdb" })).toBe(false) }) + + // altimate_change start — an absolute `file:` URI is not a "local file" by + // isLocalFilePath (see its own comment), but it still names one unambiguous + // on-disk location and must be existence-checked, or a missing absolute + // file: store opens silently empty — the exact bug class this guard exists + // to catch. + test("throws for a missing absolute `file:` URI", () => { + const missing = path.join(tmp(), "absent.duckdb") + const uri = `file://${missing}` + expect(() => assertStoreExists({ type: "duckdb" }, uri, "DuckDB")).toThrow("not found") + expect(fs.existsSync(missing)).toBe(false) + }) + + test("passes for an existing absolute `file:` URI", () => { + const dir = tmp() + const present = path.join(dir, "present.duckdb") + fs.writeFileSync(present, "") + const uri = `file://${present}` + expect(() => assertStoreExists({ type: "duckdb" }, uri, "DuckDB")).not.toThrow() + }) + + // altimate_change: regression — a `file:////...` URI (4+ leading slashes) + // used to be misread as relative by the bounded {1,3}-slash check, so + // isLocalFilePath's own file: exclusion made assertStoreExists skip it + // entirely and a missing store there opened silently empty. + test("throws for a missing `file:////` (4-slash) absolute URI", () => { + const missing = path.join(tmp(), "absent.duckdb") + const uri = `file:///${missing}` // absoluteFileUriPath sees 4 total slashes after "file:" + expect(() => assertStoreExists({ type: "duckdb" }, uri, "DuckDB")).toThrow("not found") + }) + + test("does not existence-check a relative `file:` URI (tracked separately as #1209)", () => { + // resolveStorePaths leaves relative file: URIs untouched, so guarding + // existence here would check against whatever the process cwd happens to + // be — the exact cwd-following bug this PR removes for plain paths. That + // stays out of scope; absoluteFileUriPath returns undefined for it and + // the guard falls through isLocalFilePath's own file: exclusion. + expect(() => assertStoreExists({ type: "duckdb" }, "file:relative/warehouse.duckdb", "DuckDB")).not.toThrow() + }) + // altimate_change end + + // altimate_change start — a directory at dbPath is not a valid store + test("throws when dbPath names a directory, not a file", () => { + const dir = tmp() + // altimate_change: the message now specifically says "directory" (a more + // accurate diagnosis than "not found" — the path DOES exist), since + // rejectIfDirectory reports it before the exists/missing check ever runs. + expect(() => assertStoreExists({ type: "duckdb" }, dir, "DuckDB")).toThrow("directory") + }) + + // altimate_change: regression — `create: true` used to bypass the directory + // check entirely (the `if (allowCreate) return` ran before it), so a + // directory path reached the driver with `create: true` and failed there + // with a confusing engine-level error instead of this guard's clear one. + // Neither engine can create a database AT a path that is already a + // directory, so the directory rejection must fire regardless of `create`. + test("throws when dbPath names a directory even with create: true", () => { + const dir = tmp() + expect(() => assertStoreExists({ type: "duckdb", create: true }, dir, "DuckDB")).toThrow("directory") + // A plain "not found" would be misleading here — the path DOES exist, it's + // just not a valid store — so the message must say "directory", not "not found". + expect(() => assertStoreExists({ type: "duckdb", create: true }, dir, "DuckDB")).not.toThrow("not found") + }) + + test("still allows creation of a missing FILE with create: true (directory check doesn't over-reject)", () => { + const dir = tmp() + expect(() => + assertStoreExists({ type: "duckdb", create: true }, path.join(dir, "new.duckdb"), "DuckDB"), + ).not.toThrow() + }) + + test("throws when an absolute `file:` URI names a directory even with create: true", () => { + const dir = tmp() + const uri = `file://${dir}` + expect(() => assertStoreExists({ type: "duckdb", create: true }, uri, "DuckDB")).toThrow("directory") + }) + // altimate_change end +}) + +describe("absoluteFileUriPath", () => { + test("resolves an absolute `file://` URI to its filesystem path", () => { + expect(absoluteFileUriPath("file:///var/data/warehouse.duckdb")).toBe("/var/data/warehouse.duckdb") + }) + + test("returns undefined for a relative `file:` URI", () => { + expect(absoluteFileUriPath("file:relative/warehouse.duckdb")).toBeUndefined() + }) + + // altimate_change start — regression: 4+ leading slashes (UNC-style shares) + // used to be rejected as "not absolute" by a bounded {1,3} slash count; any + // number of leading slashes is a valid absolute file: URI form, and + // fileURLToPath folds the extra slashes into the resulting path. + test("resolves an absolute `file:` URI with four or more leading slashes", () => { + expect(absoluteFileUriPath("file:////mnt/share/warehouse.duckdb")).toBe("//mnt/share/warehouse.duckdb") + expect(absoluteFileUriPath("file://///mnt/share/warehouse.duckdb")).toBe("///mnt/share/warehouse.duckdb") + }) + // altimate_change end + + test("returns undefined for the in-memory/temporary forms", () => { + expect(absoluteFileUriPath("file:")).toBeUndefined() + expect(absoluteFileUriPath("file::memory:")).toBeUndefined() + expect(absoluteFileUriPath("file:test.db?mode=memory")).toBeUndefined() + }) + + test("returns undefined for a non-`file:` path", () => { + expect(absoluteFileUriPath("/var/data/warehouse.duckdb")).toBeUndefined() + expect(absoluteFileUriPath(":memory:")).toBeUndefined() + }) }) describe("DuckDB driver create-on-open", () => { diff --git a/packages/opencode/src/altimate/native/connections/registry.ts b/packages/opencode/src/altimate/native/connections/registry.ts index 9ffc704eb..5b091ca95 100644 --- a/packages/opencode/src/altimate/native/connections/registry.ts +++ b/packages/opencode/src/altimate/native/connections/registry.ts @@ -114,12 +114,20 @@ function resolveStorePaths( for (const [name, config] of Object.entries(entries)) { const storePath = config?.path const type = typeof config?.type === "string" ? config.type.toLowerCase() : "" + // altimate_change start — recognize a Windows-absolute path even when this + // process runs on POSIX. path.isAbsolute() is platform-bound: on macOS/Linux + // it does not recognize `C:\...`, so a config shared or migrated from a + // Windows machine had its already-absolute path re-mangled through + // path.resolve(baseDir, ...) below, producing something like + // "/project/C:\Users\me\warehouse.duckdb" instead of being left untouched. if ( !FILE_STORE_TYPES.has(type) || typeof storePath !== "string" || !isLocalFilePath(storePath) || - path.isAbsolute(storePath) + path.isAbsolute(storePath) || + path.win32.isAbsolute(storePath) ) { + // altimate_change end resolved[name] = config continue } diff --git a/packages/opencode/src/altimate/tools/sql-execute.ts b/packages/opencode/src/altimate/tools/sql-execute.ts index fe30ef79c..39672df90 100644 --- a/packages/opencode/src/altimate/tools/sql-execute.ts +++ b/packages/opencode/src/altimate/tools/sql-execute.ts @@ -80,6 +80,14 @@ export const SqlExecuteTool = Tool.define("sql_execute", { const responseError = normalizeError((result as SqlExecuteResult & { error?: unknown }).error) if (responseError !== undefined) { const msg = responseError.trim() || "SQL execution failed." + // altimate_change: deliberately NOT fingerprinted. `sql.execute` returns this + // same result shape both for a warehouse query that ran and failed AND for a + // pre-execution failure — no warehouse configured, connector setup failed + // (see connections/register.ts). This branch alone cannot tell those apart, so + // fingerprinting it would mislabel some never-executed queries as "executed + // SQL". De-scoped to fingerprint-on-success-only (below) rather than build a + // failed-execution-vs-never-executed taxonomy in this cleanup PR; tracked as + // altimate-code#1242. // altimate_change — annotate this failure too, same as the catch block below: // a fail-open notice that only rides on success under-counts fail-open in // precisely the cases most likely to fail. @@ -91,8 +99,11 @@ export const SqlExecuteTool = Tool.define("sql_execute", { } // altimate_change end - let output = formatResult(result) - // altimate_change start — emit SQL structure fingerprint telemetry + // altimate_change start — emit SQL structure fingerprint telemetry on the + // success path, BEFORE formatting the result. A query that reached this point + // genuinely executed against a warehouse; emitting the fingerprint here (rather + // than after formatResult()) means a formatting failure below still leaves this + // execution counted, instead of silently dropping it from the telemetry. try { const fp = computeSqlFingerprint(args.query) if (fp) { @@ -114,6 +125,8 @@ export const SqlExecuteTool = Tool.define("sql_execute", { // Fingerprinting must never break query execution } // altimate_change end + + let output = formatResult(result) // altimate_change start — progressive disclosure suggestions const suggestion = PostConnectSuggestions.getProgressiveSuggestion("sql_execute") if (suggestion) { @@ -134,6 +147,9 @@ export const SqlExecuteTool = Tool.define("sql_execute", { }) } catch (e) { const msg = e instanceof Error ? e.message : String(e) + // altimate_change: deliberately NOT fingerprinted, same reasoning as the + // result-error branch above — this catch only fires when `Dispatcher.call` + // itself throws, which never happened after a warehouse actually ran the query. // altimate_change — annotate the failure too. A fail-open notice that only rides // on success is worse than none: the reason vanishes exactly when the call went // wrong, and the `precedence` marker under-counts fail-open in precisely the diff --git a/packages/opencode/src/altimate/tools/warehouse-add.ts b/packages/opencode/src/altimate/tools/warehouse-add.ts index a0eb0668a..8636e1493 100644 --- a/packages/opencode/src/altimate/tools/warehouse-add.ts +++ b/packages/opencode/src/altimate/tools/warehouse-add.ts @@ -35,7 +35,7 @@ export const WarehouseAddTool = Tool.define("warehouse_add", { - sqlite: path (file path), create (optional, default false) - clickhouse: host, port, database, user, password, protocol (http/https), connection_string, request_timeout, tls_ca_cert, tls_cert, tls_key, clickhouse_settings - trino: host, port, catalog, schema, user, password, protocol (http/https), connection_string, access_token, extra_headers -File-backed stores (duckdb, sqlite): the store must already exist — connecting never creates it, because an empty database answers every query with no rows. Pass "create": true only when the store is meant to be created. A relative "path" is resolved against the directory of the config that declares it (the global config resolves against ~/.altimate-code), never against the current working directory; prefer an absolute path. +File-backed stores (duckdb, sqlite): the store must already exist — connecting never creates it, because an empty database answers every query with no rows. Pass "create": true only when the store is meant to be created. A relative "path" is resolved once, at add-time, against the project root (not the current working directory) and persisted absolute — even though this tool always writes to the global config file, "add" resolves against the project you're in right now, not against ~/.altimate-code; prefer an absolute path to avoid relying on that. Snowflake auth examples: (1) Password: {"type":"snowflake","account":"xy12345","user":"admin","password":"secret","warehouse":"WH","database":"db"}. (2) Key-pair: {"type":"snowflake","account":"xy12345","user":"admin","private_key_path":"/path/rsa_key.p8","warehouse":"WH","database":"db"}. (3) OAuth: {"type":"snowflake","account":"xy12345","authenticator":"oauth","token":"","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 36df1f3a5..b4f6a131a 100644 --- a/packages/opencode/test/altimate/drivers-e2e.test.ts +++ b/packages/opencode/test/altimate/drivers-e2e.test.ts @@ -60,6 +60,160 @@ async function waitForPort( throw new Error(`Port ${port} not reachable after ${timeoutMs}ms`) } +// altimate_change start — retry a flaky setup step, and fail loudly (not silently) +// once retries are exhausted. +/** + * Run `attempt` up to `maxAttempts` times, with a short backoff between tries, + * and return its result on the first success. If every attempt fails, THROW + * the last error rather than swallowing it. + * + * This is the difference between a genuine "not available here" (handled + * elsewhere, before this is ever called) and "was available but setup broke": + * a caller that catches this and silently leaves some "ready" flag false + * recreates the exact vacuous-green failure this file's `probeDuckDB` exists + * to prevent, one layer down — every test gated on that flag would then + * report as passing via an early `if (!ready) return` instead of failing. + */ +async function connectWithRetry(attempt: (attemptNumber: number) => Promise, maxAttempts: number): Promise { + let lastError: unknown + for (let n = 1; n <= maxAttempts; n++) { + try { + return await attempt(n) + } catch (e) { + lastError = e + if (n < maxAttempts) await new Promise((r) => setTimeout(r, 100 * n)) + } + } + // altimate_change: preserve the original error as `cause` instead of only its + // message. A plain `new Error(message)` discarded the last attempt's stack, + // type (TypeError vs the driver's own error class), and any extra properties + // it carried — exactly the details someone debugging a real setup failure + // needs. The friendly summary stays the thrown error's own message; `cause` + // carries the original through unmodified. + throw new Error( + `Setup failed after ${maxAttempts} attempts: ${lastError instanceof Error ? lastError.message : String(lastError)}`, + { cause: lastError }, + ) +} +// altimate_change end + +// altimate_change start — never leak a native handle on a failed connect +/** + * Construct a connector via `make`, then open it. If the open step (`connect()`) + * fails, close the half-open connector before rethrowing — otherwise a + * connector whose constructor already opened a native handle (as DuckDB's does) + * leaks that handle on every failed attempt a retry loop makes. + */ +async function connectOrClose; close(): Promise }>( + make: () => Promise, +): Promise { + const c = await make() + try { + await c.connect() + } catch (e) { + await c.close().catch(() => { + // best-effort cleanup of a half-open handle; the original error is what matters + }) + throw e + } + return c +} +// altimate_change end + +// altimate_change start — unit-test the retry-then-fail-loudly behavior directly, +// independent of real DuckDB availability, so this regression is caught even in +// environments where the DuckDB binding isn't installed at all. +describe("connectWithRetry", () => { + test("throws (does not silently resolve) once every attempt is exhausted", async () => { + let calls = 0 + const alwaysFails = async () => { + calls++ + throw new Error("transient setup failure") + } + await expect(connectWithRetry(alwaysFails, 3)).rejects.toThrow("Setup failed after 3 attempts") + await expect(connectWithRetry(alwaysFails, 3)).rejects.toThrow("transient setup failure") + expect(calls).toBe(6) // 3 attempts per call above, called twice + }) + + // altimate_change: regression — the thrown error used to be a plain + // `new Error(message)`, discarding the last attempt's original error object + // (its stack, type, and any extra properties) entirely. + test("preserves the last attempt's original error as `cause`", async () => { + const original = new TypeError("native binding not built") + const alwaysFailsWithOriginal = async () => { + throw original + } + try { + await connectWithRetry(alwaysFailsWithOriginal, 2) + throw new Error("expected connectWithRetry to throw") + } catch (e) { + expect((e as Error).cause).toBe(original) + } + }) + + test("resolves with the first successful attempt's result, retrying past earlier failures", async () => { + let calls = 0 + const succeedsOnThirdTry = async () => { + calls++ + if (calls < 3) throw new Error("not yet") + return "connected" + } + const result = await connectWithRetry(succeedsOnThirdTry, 5) + expect(result).toBe("connected") + expect(calls).toBe(3) + }) +}) +// altimate_change end + +// altimate_change start — regression: a connector whose connect() fails must be +// closed, not dropped, or a retry loop leaks a native handle per failed attempt. +describe("connectOrClose", () => { + function mockConnector(shouldFailConnect: boolean) { + let closed = false + return { + async connect() { + if (shouldFailConnect) throw new Error("open failed") + }, + async close() { + closed = true + }, + get closed() { + return closed + }, + } + } + + test("closes the connector when connect() fails, and rethrows the original error", async () => { + const c = mockConnector(true) + await expect(connectOrClose(async () => c)).rejects.toThrow("open failed") + expect(c.closed).toBe(true) + }) + + test("does not close a connector that opened successfully", async () => { + const c = mockConnector(false) + const result = await connectOrClose(async () => c) + expect(result).toBe(c) + expect(c.closed).toBe(false) + }) + + test("closes every connector dropped across a full connectWithRetry sequence, only the final success stays open", async () => { + const made: ReturnType[] = [] + let attempt = 0 + const result = await connectWithRetry(async () => { + attempt++ + const c = mockConnector(attempt < 3) // fails twice, succeeds on the 3rd + made.push(c) + return connectOrClose(async () => c) + }, 3) + expect(attempt).toBe(3) + expect(made[0].closed).toBe(true) + expect(made[1].closed).toBe(true) + expect(made[2].closed).toBe(false) + expect(result).toBe(made[2]) + }) +}) +// altimate_change end + // altimate_change start — authoritative DuckDB availability probe. // `require("duckdb")` (isDuckDBAvailable) can return true when the native binding // is present in the process module cache but actually fails to CONNECT in this @@ -72,7 +226,11 @@ async function probeDuckDB(): Promise { if (!isDuckDBAvailable()) return false try { const mod = await import("@altimateai/drivers/duckdb") - const probe = await mod.connect({ type: "duckdb" }) + // altimate_change start — requireStorePath() now rejects a missing path; + // an in-memory probe must ask for ":memory:" explicitly or every DuckDB + // E2E test below silently skips (duckdbAvailable stays false). + const probe = await mod.connect({ type: "duckdb", path: ":memory:" }) + // altimate_change end await probe.connect() // Guard against a leaked mock.module from another test file (e.g. // dbt-first-execution.test.ts mocks @altimateai/drivers/duckdb at module @@ -109,29 +267,32 @@ describe("DuckDB Driver E2E", () => { let duckdbReady = false // altimate_change start — retry DuckDB connection initialization to handle - // transient native binding load failures when the full suite runs in parallel + // transient native binding load failures when the full suite runs in parallel, + // but FAIL (don't silently skip) if it never recovers. + // + // `probeDuckDB()` above already proved DuckDB is genuinely available and + // working in this process. If setup here still fails after retries, that is + // a real regression, not "DuckDB isn't available" — every test below still + // runs (test.skipIf keys off `duckdbAvailable`, which stays true regardless + // of what happens here), and each one used to just `if (!duckdbReady) return` + // and report as passing: the same vacuous-green class the driver-e2e + // false-skip fix removed, one layer down. Throwing here fails the whole + // describe block instead of letting every test silently "pass" via that + // early return. beforeAll(async () => { if (!duckdbAvailable) return - const maxAttempts = 3 - for (let attempt = 1; attempt <= maxAttempts; attempt++) { - try { - const mod = await import("@altimateai/drivers/duckdb") - connector = await mod.connect({ type: "duckdb" }) - await connector.connect() - duckdbReady = true - break - } catch (e) { - if (attempt < maxAttempts) { - // Brief delay before retry to let concurrent native-binding loads settle - await new Promise((r) => setTimeout(r, 100 * attempt)) - } else { - console.warn( - "DuckDB not available (native binding may be missing); skipping DuckDB tests:", - (e as Error).message, - ) - } - } - } + connector = await connectWithRetry( + // altimate_change: wrapped in connectOrClose — mod.connect() only builds the + // connector, the native handle opens in c.connect() below. A failed c.connect() + // used to drop `c` without closing it, leaking that handle on every failed retry. + () => + connectOrClose(async () => { + const mod = await import("@altimateai/drivers/duckdb") + return mod.connect({ type: "duckdb", path: ":memory:" }) + }), + 3, + ) + duckdbReady = true }) // altimate_change end @@ -286,7 +447,9 @@ describe("DuckDB Driver E2E", () => { async () => { if (!duckdbReady) return const mod = await import("@altimateai/drivers/duckdb") - const tmp = await mod.connect({ type: "duckdb" }) + // altimate_change start — requireStorePath() now rejects a missing path + const tmp = await mod.connect({ type: "duckdb", path: ":memory:" }) + // altimate_change end await tmp.connect() const result = await tmp.execute("SELECT 42 AS answer") expect(result.rows[0][0]).toBe(42) diff --git a/packages/opencode/test/altimate/telemetry-signals.test.ts b/packages/opencode/test/altimate/telemetry-signals.test.ts index f3e2e6345..378b09c45 100644 --- a/packages/opencode/test/altimate/telemetry-signals.test.ts +++ b/packages/opencode/test/altimate/telemetry-signals.test.ts @@ -942,24 +942,54 @@ describe("altimate-core failure isolation", () => { } }) - test("sql-execute fingerprint try/catch isolates failures from query results", () => { - // Verify the code structure: fingerprinting runs AFTER query result is computed - // and is wrapped in its own try/catch + test("sql-execute fingerprints success only — de-scoped to the minimal honest form (see follow-up issue)", () => { + // altimate_change: earlier revisions of this fix tried fingerprinting on both + // the success path AND the result-error branch (sql.execute returns a + // result-shaped `{ ..., error }` instead of throwing for a connection/query + // failure). But that result shape is ALSO what a pre-execution failure returns + // — no warehouse configured, connector setup failed (connections/register.ts) + // — so the result-error branch cannot reliably tell "warehouse ran the query + // and it failed" apart from "never reached a warehouse at all". Fingerprinting + // it would mislabel some never-executed queries as executed SQL. After three + // review rounds converging on this, it was deliberately de-scoped to + // fingerprint-on-success-only (the pre-existing behavior before any of this + // started) rather than build a failed-execution-vs-never-executed taxonomy in + // this cleanup PR. A distinct execution-phase signal is tracked as + // altimate-code#1242. const fs = require("fs") const src = fs.readFileSync( require("path").join(__dirname, "../../src/altimate/tools/sql-execute.ts"), "utf8", ) - // Query execution happens first const execIdx = src.indexOf('Dispatcher.call("sql.execute"') - const formatIdx = src.indexOf("formatResult(result)") + const responseErrorIdx = src.indexOf("if (responseError !== undefined) {") const fpCallIdx = src.indexOf("computeSqlFingerprint(args.query)") + const formatIdx = src.indexOf("formatResult(result)") const guardComment = src.indexOf("Fingerprinting must never break query execution") expect(execIdx).toBeGreaterThan(0) - expect(formatIdx).toBeGreaterThan(execIdx) // format after execute - expect(fpCallIdx).toBeGreaterThan(formatIdx) // fingerprint after format - expect(guardComment).toBeGreaterThan(fpCallIdx) // catch guard exists after fingerprint + expect(responseErrorIdx).toBeGreaterThan(execIdx) + // The single fingerprint call sits strictly between the error check and + // formatResult() — i.e. only on the success path — and BEFORE formatting, so a + // formatResult() throw cannot cause a genuinely-executed query to go uncounted. + expect(fpCallIdx).toBeGreaterThan(responseErrorIdx) + expect(formatIdx).toBeGreaterThan(fpCallIdx) + expect(guardComment).toBeGreaterThan(fpCallIdx) + expect(guardComment).toBeLessThan(formatIdx) + + // Exactly one call site — no more "every outcome" fan-out. + const callSites = [...src.matchAll(/computeSqlFingerprint\(args\.query\)/g)] + expect(callSites.length).toBe(1) + + // Neither the result-error branch nor the catch block references it. + const resultErrorBlockEnd = src.indexOf("// altimate_change end", responseErrorIdx) + const resultErrorBlockBody = src.slice(responseErrorIdx, resultErrorBlockEnd) + expect(resultErrorBlockBody.includes("computeSqlFingerprint")).toBe(false) + + const catchBlockStart = src.indexOf("} catch (e) {") + const catchBlockEnd = src.indexOf("\n }\n },\n})", catchBlockStart) + const catchBlockBody = src.slice(catchBlockStart, catchBlockEnd) + expect(catchBlockBody.includes("computeSqlFingerprint")).toBe(false) }) test("crash-resistant SQL inputs all handled safely", () => {