From b96f987c5a6eb716c078e774ebab84eee58e1073 Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Thu, 3 Sep 2026 10:40:43 -0700 Subject: [PATCH 1/5] =?UTF-8?q?fix(drivers):=20recover=20#1204=20review=20?= =?UTF-8?q?debt=20=E2=80=94=20driver-e2e=20false-skip,=20file:=20URI=20che?= =?UTF-8?q?cks,=20Windows=20paths,=20telemetry,=20docs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #1204 (fix/warehouse-store-path-resolution, merged 1caa234ff9) shipped the core path-resolution fix but merged with only 2 of 47 review threads addressed. This recovers the real bugs and finishes the incomplete P1 left behind. Recovered from an orphaned agent worktree, re-applied and re-verified here: - drivers-e2e.test.ts: three bare connect({ type: "duckdb" }) calls threw under #1204's new requireStorePath() guard, so probeDuckDB() always self-caught and 39 of 50 tests in the file silently skipped even with the duckdb binding installed. Fixed by passing path: ":memory:". Verified independently: 11 pass/39 skip -> 36 pass/14 skip. - file-store.ts isLocalFilePath: the scheme-exclusion regex matched any "2+ letter prefix + colon", misreading a local filename shaped like data:warehouse.duckdb as a remote scheme. Narrowed to real scheme:// URIs plus DuckDB's specific non-slash extension schemes (md:, motherduck:, ducklake:). Added a regression unit test. Completed the incomplete P1 (absoluteFileUriPath() was written but never wired into assertStoreExists, so behavior was unchanged on main): - Wired absoluteFileUriPath() into assertStoreExists so a missing *absolute* file: URI now fails loudly instead of opening silently empty — the exact bug class #1204 exists to fix. Relative file: URIs are deliberately left alone (tracked separately as #1209). Added tests for both. Remaining live review threads, fixed with tests where testable: - registry.ts resolveStorePaths: POSIX path.isAbsolute() doesn't recognize C:\..., so a shared/migrated Windows config path got mangled by path.resolve. Fixed with path.isAbsolute() || path.win32.isAbsolute(). By-inspection only — no Windows CI. - warehouse-add.ts: tool description said a relative path resolves "against the directory of the config that declares it"; Registry.add() actually resolves against projectRoot() at add-time regardless of where it's persisted (the global config). Description corrected to match. - sql-execute.ts: SQL fingerprint telemetry was emitted only on the success path, so failed executions (including the new error-surfacing branch) never got fingerprinted — biasing telemetry away from failures. Extracted a shared emitSqlFingerprint() helper and call it from all three outcomes (success, result-shaped error, thrown exception). - docs/configure/warehouses.md: still said path was optional/omittable for DuckDB and SQLite in-memory; requireStorePath() now rejects that. Corrected in both the DuckDB and SQLite sections. - file-store.ts assertStoreExists: fs.existsSync is also true for a directory at that path, which would have let a misconfigured directory path through to a confusing driver-level error instead of this guard's clear one. Added an existsAsFile check. Out of scope: the registry cross-tenant-scoping P1 (registry.ts:80/:175, the process-global loaded state) is tracked separately as #1237 and needs its own design PR — not attempted here. Gates: bun test packages/drivers/ packages/opencode/test/altimate/ green (5007+317 pass, 0 fail), bun turbo typecheck 13/13, marker check --base origin/main --strict clean (no upstream-shared files touched), prettier clean on all newly-written content. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01VqnuBDGkh1ZT65Ti7e6DHZ --- docs/docs/configure/warehouses.md | 4 +- packages/drivers/src/file-store.ts | 103 +++++++++++++++++- .../drivers/test/file-store-guard.test.ts | 73 ++++++++++++- .../altimate/native/connections/registry.ts | 10 +- .../src/altimate/tools/sql-execute.ts | 58 ++++++---- .../src/altimate/tools/warehouse-add.ts | 2 +- .../test/altimate/drivers-e2e.test.ts | 14 ++- .../test/altimate/telemetry-signals.test.ts | 25 +++-- 8 files changed, 247 insertions(+), 42 deletions(-) diff --git a/docs/docs/configure/warehouses.md b/docs/docs/configure/warehouses.md index d85136be46..9b96d18cd9 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" @@ -461,7 +461,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/packages/drivers/src/file-store.ts b/packages/drivers/src/file-store.ts index 4c4ad3445c..e100cd6c98 100644 --- a/packages/drivers/src/file-store.ts +++ b/packages/drivers/src/file-store.ts @@ -13,8 +13,18 @@ */ 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. + */ +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 +39,63 @@ 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 + if (/^[a-zA-Z][a-zA-Z0-9+.-]*:\/\//.test(dbPath)) return false + 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 + const isSlashForm = /^\/{1,3}/.test(rest) + 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,6 +126,26 @@ export function allowsCreate(config: ConnectionConfig): boolean { return config.create === true } +// altimate_change start — a directory at dbPath is not a valid store either +/** + * Whether `path` names a store file that actually exists. `fs.existsSync` + * alone is also true for a directory at that path, which the driver would + * then fail to open with a confusing engine-level error instead of this + * guard's clear one — a config that names a directory (a typo, or a path one + * level off) should be reported the same way a missing file is. + */ +function existsAsFile(path: string): boolean { + if (!fs.existsSync(path)) return false + try { + return !fs.statSync(path).isDirectory() + } catch { + // A stat failure (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. + return true + } +} +// 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. @@ -85,8 +162,24 @@ export function assertStoreExists( 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) { + if (existsAsFile(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 - if (fs.existsSync(dbPath)) return + if (existsAsFile(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. ` + diff --git a/packages/drivers/test/file-store-guard.test.ts b/packages/drivers/test/file-store-guard.test.ts index 391165951c..bac5da8f0d 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" @@ -57,6 +57,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 +103,64 @@ 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() + }) + + 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() + expect(() => assertStoreExists({ type: "duckdb" }, dir, "DuckDB")).toThrow("not found") + }) + // 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() + }) + + 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 9ffc704eb1..5b091ca95c 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 fe30ef79c1..2ec4c7f0e1 100644 --- a/packages/opencode/src/altimate/tools/sql-execute.ts +++ b/packages/opencode/src/altimate/tools/sql-execute.ts @@ -80,6 +80,13 @@ 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 start — fingerprint a failed execution too. The + // fingerprint used to be emitted only on the success path below, so a + // failed query (this error branch, and the thrown-exception catch + // further down) never got fingerprinted at all — biasing the sql + // structure telemetry away from exactly the queries most worth seeing. + emitSqlFingerprint(args.query, ctx.sessionID) + // altimate_change end // 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. @@ -93,26 +100,7 @@ export const SqlExecuteTool = Tool.define("sql_execute", { let output = formatResult(result) // altimate_change start — emit SQL structure fingerprint telemetry - try { - const fp = computeSqlFingerprint(args.query) - if (fp) { - Telemetry.track({ - type: "sql_fingerprint", - timestamp: Date.now(), - session_id: ctx.sessionID, - statement_types: JSON.stringify(fp.statement_types), - categories: JSON.stringify(fp.categories), - table_count: fp.table_count, - function_count: fp.function_count, - has_subqueries: fp.has_subqueries, - has_aggregation: fp.has_aggregation, - has_window_functions: fp.has_window_functions, - node_count: fp.node_count, - }) - } - } catch { - // Fingerprinting must never break query execution - } + emitSqlFingerprint(args.query, ctx.sessionID) // altimate_change end // altimate_change start — progressive disclosure suggestions const suggestion = PostConnectSuggestions.getProgressiveSuggestion("sql_execute") @@ -134,6 +122,10 @@ export const SqlExecuteTool = Tool.define("sql_execute", { }) } catch (e) { const msg = e instanceof Error ? e.message : String(e) + // altimate_change start — fingerprint a thrown-exception failure too (see the + // matching comment on the result-error branch above) + emitSqlFingerprint(args.query, ctx.sessionID) + // altimate_change end // 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 @@ -147,6 +139,32 @@ export const SqlExecuteTool = Tool.define("sql_execute", { }, }) +// altimate_change start — emit SQL structure fingerprint telemetry on every +// execution outcome (success, a result-shaped error, or a thrown exception), +// not only on success. Extracted so all three call sites stay in sync. +function emitSqlFingerprint(query: string, sessionID: string): void { + try { + const fp = computeSqlFingerprint(query) + if (!fp) return + Telemetry.track({ + type: "sql_fingerprint", + timestamp: Date.now(), + session_id: sessionID, + statement_types: JSON.stringify(fp.statement_types), + categories: JSON.stringify(fp.categories), + table_count: fp.table_count, + function_count: fp.function_count, + has_subqueries: fp.has_subqueries, + has_aggregation: fp.has_aggregation, + has_window_functions: fp.has_window_functions, + node_count: fp.node_count, + }) + } catch { + // Fingerprinting must never break query execution + } +} +// altimate_change end + // altimate_change start — pre-execution SQL validation via cached schema const CACHE_TTL_MS = 24 * 60 * 60 * 1000 // 24 hours // High ceiling so large warehouses aren't arbitrarily truncated; we emit diff --git a/packages/opencode/src/altimate/tools/warehouse-add.ts b/packages/opencode/src/altimate/tools/warehouse-add.ts index a0eb0668a1..8636e14936 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 36df1f3a54..24ce8336d4 100644 --- a/packages/opencode/test/altimate/drivers-e2e.test.ts +++ b/packages/opencode/test/altimate/drivers-e2e.test.ts @@ -72,7 +72,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 @@ -116,7 +120,9 @@ describe("DuckDB Driver E2E", () => { for (let attempt = 1; attempt <= maxAttempts; attempt++) { try { const mod = await import("@altimateai/drivers/duckdb") - connector = await mod.connect({ type: "duckdb" }) + // altimate_change start — requireStorePath() now rejects a missing path + connector = await mod.connect({ type: "duckdb", path: ":memory:" }) + // altimate_change end await connector.connect() duckdbReady = true break @@ -286,7 +292,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 f3e2e6345e..f351ba9ef9 100644 --- a/packages/opencode/test/altimate/telemetry-signals.test.ts +++ b/packages/opencode/test/altimate/telemetry-signals.test.ts @@ -942,24 +942,31 @@ 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 every outcome (success, result-error, and thrown exception) via a guarded helper", () => { + // altimate_change: fingerprinting used to run only on the success path, so a + // failed query never got fingerprinted at all — biasing sql_fingerprint + // telemetry away from exactly the queries most worth seeing. It is now + // emitted from all three outcomes through one shared, try/catch-guarded + // helper (`emitSqlFingerprint`) so they cannot drift out of sync. 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 fpCallIdx = src.indexOf("computeSqlFingerprint(args.query)") + const helperDefIdx = src.indexOf("function emitSqlFingerprint(") + const fpCallInsideHelperIdx = src.indexOf("computeSqlFingerprint(query)") 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(helperDefIdx).toBeGreaterThan(execIdx) + // The helper itself calls computeSqlFingerprint and is guarded by a try/catch. + expect(fpCallInsideHelperIdx).toBeGreaterThan(helperDefIdx) + expect(guardComment).toBeGreaterThan(fpCallInsideHelperIdx) + + // All three outcomes call the shared helper. + const callSites = [...src.matchAll(/emitSqlFingerprint\(args\.query, ctx\.sessionID\)/g)] + expect(callSites.length).toBe(3) }) test("crash-resistant SQL inputs all handled safely", () => { From 7f5ebb99433506103c5acf11aeb526fd6210f47a Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Thu, 3 Sep 2026 11:19:00 -0700 Subject: [PATCH 2/5] =?UTF-8?q?fix(drivers):=20address=20#1238=20review=20?= =?UTF-8?q?=E2=80=94=20directory/create=20ordering,=20file:=20URI=20slashe?= =?UTF-8?q?s,=20Windows=20paths,=20telemetry=20mislabel,=20post-probe=20fa?= =?UTF-8?q?ilure?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to #1238's own review round (8 threads, all on the new code that PR added). Verified each against current code before acting; fixed the real bugs with tests, replied with reasoning on the one genuine design call. - file-store.ts: `create: true` bypassed the new directory check entirely, since `if (allowCreate) return` ran before it — a directory path with `create: true` reached the driver instead of getting this guard's clear error. Extracted `rejectIfDirectory()` and moved it before the `allowCreate` bypass in both the plain-path and `file:` URI branches, since neither engine can create a database at a path that's already a directory. - file-store.ts `absoluteFileUriPath`: the leading-slash check was bounded to 1-3 slashes, so `file:////mnt/share/warehouse.duckdb` (UNC-style, 4+ slashes) was treated as relative and bypassed the existence guard. Verified `fileURLToPath` handles any number of leading slashes without throwing; widened the check to `/^\/+/`. - file-store.ts `isLocalFilePath`: the `scheme://` regex accepted a single-character scheme, so a doubled-slash Windows path like `C://data/warehouse.duckdb` matched it exactly like `s3://...` and was misclassified as remote. No real scheme is a single letter; require 2+ characters before `://`. - sql-execute.ts: the previous round's "fingerprint on every outcome" fix mislabeled the catch block. `sql.execute` never throws for a connection/query failure — it returns a result carrying `error`, already fingerprinted on the result-error branch. The catch block only fires when the query never reached a warehouse at all (e.g. dispatcher down), so fingerprinting it there folded "never executed" into a signal meant to measure "executed SQL", re-biasing the telemetry in the opposite direction. Removed the catch-block emission; kept it on success + result-error only. - drivers-e2e.test.ts: if DuckDB setup failed after the availability probe already passed, every test's `if (!duckdbReady) return` guard reported a pass instead of a failure — the same vacuous-green class as the false-skip bug, one layer down. Extracted `connectWithRetry()`, which now throws (with the underlying error) once retries are exhausted instead of swallowing it; `beforeAll` no longer catches that throw, so a genuine post-probe failure now fails the whole describe block. Added a standalone unit test for the helper that runs independent of real DuckDB availability. Not fixed — genuine design call, replied with reasoning, left open: a custom DuckDB extension using an unlisted bare `scheme:` form (e.g. `acme:catalog`) is no longer forwarded, because narrowing the bare-scheme exclusion to a closed list (to fix the `data:warehouse.duckdb` false-positive from the prior round) is in direct, unavoidable tension with forwarding arbitrary unknown bare schemes — nothing in the syntax alone can tell them apart. Gates: `bun test packages/drivers/` 323/323 pass, `bun test packages/opencode/test/altimate/` 5009/5009 pass (653 skip, unrelated), `bun turbo typecheck` 13/13, marker check `--base origin/main --strict` clean, prettier clean on every newly-written/substantially-edited file. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01VqnuBDGkh1ZT65Ti7e6DHZ --- packages/drivers/src/file-store.ts | 67 ++++++++---- .../drivers/test/file-store-guard.test.ts | 69 +++++++++++- .../src/altimate/tools/sql-execute.ts | 24 +++-- .../test/altimate/drivers-e2e.test.ts | 101 ++++++++++++++---- .../test/altimate/telemetry-signals.test.ts | 26 +++-- 5 files changed, 231 insertions(+), 56 deletions(-) diff --git a/packages/drivers/src/file-store.ts b/packages/drivers/src/file-store.ts index e100cd6c98..1805cc4df0 100644 --- a/packages/drivers/src/file-store.ts +++ b/packages/drivers/src/file-store.ts @@ -57,7 +57,15 @@ const NON_SLASH_REMOTE_SCHEMES = ["md:", "motherduck:", "ducklake:"] export function isLocalFilePath(dbPath: string): boolean { if (dbPath === "" || dbPath === ":memory:") return false if (/^file:/i.test(dbPath)) return false - if (/^[a-zA-Z][a-zA-Z0-9+.-]*:\/\//.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 } @@ -82,7 +90,13 @@ export function absoluteFileUriPath(dbPath: string): string | 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 - const isSlashForm = /^\/{1,3}/.test(rest) + // 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 { @@ -126,29 +140,40 @@ export function allowsCreate(config: ConnectionConfig): boolean { return config.create === true } -// altimate_change start — a directory at dbPath is not a valid store either +// altimate_change start — a directory at dbPath is never a valid store, with or +// without `create` /** - * Whether `path` names a store file that actually exists. `fs.existsSync` - * alone is also true for a directory at that path, which the driver would - * then fail to open with a confusing engine-level error instead of this - * guard's clear one — a config that names a directory (a typo, or a path one - * level off) should be reported the same way a missing file is. + * 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 existsAsFile(path: string): boolean { - if (!fs.existsSync(path)) return false +function rejectIfDirectory(path: string, displayPath: string, engine: string): void { + let isDir: boolean try { - return !fs.statSync(path).isDirectory() + isDir = fs.existsSync(path) && fs.statSync(path).isDirectory() } catch { - // A stat failure (e.g. a race with a delete, or a permissions error) + // 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. - return true + 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 @@ -161,7 +186,6 @@ 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 @@ -170,7 +194,11 @@ export function assertStoreExists( // the exact bug class this guard exists to catch. const fileUriPath = absoluteFileUriPath(dbPath) if (fileUriPath !== undefined) { - if (existsAsFile(fileUriPath)) return + // 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. ` + @@ -179,7 +207,10 @@ export function assertStoreExists( } // altimate_change end if (!isLocalFilePath(dbPath)) return - if (existsAsFile(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}". ` + `Opening a warehouse connection never creates the database — an empty store would answer every query with no rows. ` + diff --git a/packages/drivers/test/file-store-guard.test.ts b/packages/drivers/test/file-store-guard.test.ts index bac5da8f0d..282afe1ebb 100644 --- a/packages/drivers/test/file-store-guard.test.ts +++ b/packages/drivers/test/file-store-guard.test.ts @@ -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 @@ -124,6 +141,16 @@ describe("assertStoreExists", () => { 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 @@ -137,7 +164,37 @@ describe("assertStoreExists", () => { // 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() - expect(() => assertStoreExists({ type: "duckdb" }, dir, "DuckDB")).toThrow("not found") + // 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 }) @@ -151,6 +208,16 @@ describe("absoluteFileUriPath", () => { 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() diff --git a/packages/opencode/src/altimate/tools/sql-execute.ts b/packages/opencode/src/altimate/tools/sql-execute.ts index 2ec4c7f0e1..cb3d39d585 100644 --- a/packages/opencode/src/altimate/tools/sql-execute.ts +++ b/packages/opencode/src/altimate/tools/sql-execute.ts @@ -122,10 +122,14 @@ export const SqlExecuteTool = Tool.define("sql_execute", { }) } catch (e) { const msg = e instanceof Error ? e.message : String(e) - // altimate_change start — fingerprint a thrown-exception failure too (see the - // matching comment on the result-error branch above) - emitSqlFingerprint(args.query, ctx.sessionID) - // altimate_change end + // altimate_change: deliberately NOT fingerprinted. This catch only fires when + // `Dispatcher.call` itself throws (dispatcher down, no warehouse configured) — + // per the comment on the result-error branch above, `sql.execute` never throws + // for a connection/query failure, it returns a result carrying `error`, which + // that branch already fingerprints. A query that reaches here never ran against + // any warehouse, so fingerprinting it here would fold "never executed" into a + // signal meant to measure "executed SQL" (success and result-error), re-biasing + // the telemetry this change is meant to correct in the opposite direction. // 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 @@ -139,9 +143,15 @@ export const SqlExecuteTool = Tool.define("sql_execute", { }, }) -// altimate_change start — emit SQL structure fingerprint telemetry on every -// execution outcome (success, a result-shaped error, or a thrown exception), -// not only on success. Extracted so all three call sites stay in sync. +// altimate_change start — emit SQL structure fingerprint telemetry for every +// outcome where a warehouse actually ran the query: success, and a result-shaped +// error (sql.execute returns `{ ..., error }` rather than throwing for a +// connection/query failure — see the result-error branch above). Deliberately +// NOT called from the thrown-exception catch block: that path only fires when +// the query never reached a warehouse at all (e.g. dispatcher down), and +// fingerprinting a never-executed query there would bias this "executed SQL +// structure" signal toward attempts that never ran. Extracted so the two +// legitimate call sites stay in sync. function emitSqlFingerprint(query: string, sessionID: string): void { try { const fp = computeSqlFingerprint(query) diff --git a/packages/opencode/test/altimate/drivers-e2e.test.ts b/packages/opencode/test/altimate/drivers-e2e.test.ts index 24ce8336d4..69d312a17e 100644 --- a/packages/opencode/test/altimate/drivers-e2e.test.ts +++ b/packages/opencode/test/altimate/drivers-e2e.test.ts @@ -60,6 +60,65 @@ 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)) + } + } + throw new Error( + `Setup failed after ${maxAttempts} attempts: ${lastError instanceof Error ? lastError.message : String(lastError)}`, + ) +} +// 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 + }) + + 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 — 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 @@ -113,31 +172,27 @@ 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") - // altimate_change start — requireStorePath() now rejects a missing path - connector = await mod.connect({ type: "duckdb", path: ":memory:" }) - // altimate_change end - 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(async () => { + const mod = await import("@altimateai/drivers/duckdb") + const c = await mod.connect({ type: "duckdb", path: ":memory:" }) + await c.connect() + return c + }, 3) + duckdbReady = true }) // altimate_change end diff --git a/packages/opencode/test/altimate/telemetry-signals.test.ts b/packages/opencode/test/altimate/telemetry-signals.test.ts index f351ba9ef9..04d143ef61 100644 --- a/packages/opencode/test/altimate/telemetry-signals.test.ts +++ b/packages/opencode/test/altimate/telemetry-signals.test.ts @@ -942,12 +942,17 @@ describe("altimate-core failure isolation", () => { } }) - test("sql-execute fingerprints every outcome (success, result-error, and thrown exception) via a guarded helper", () => { + test("sql-execute fingerprints only executed outcomes (success and result-error), not a never-executed thrown exception", () => { // altimate_change: fingerprinting used to run only on the success path, so a - // failed query never got fingerprinted at all — biasing sql_fingerprint - // telemetry away from exactly the queries most worth seeing. It is now - // emitted from all three outcomes through one shared, try/catch-guarded - // helper (`emitSqlFingerprint`) so they cannot drift out of sync. + // failed-but-executed query (the result-error branch) never got fingerprinted — + // biasing sql_fingerprint telemetry away from exactly the queries most worth + // seeing. It is now emitted from both outcomes where a warehouse actually ran + // the query, through one shared, try/catch-guarded helper (`emitSqlFingerprint`) + // so they cannot drift out of sync. It is deliberately NOT called from the + // thrown-exception catch block: that path only fires when the query never + // reached a warehouse at all (e.g. dispatcher down), so fingerprinting it there + // would fold "never executed" into a signal meant to measure "executed SQL", + // re-biasing the telemetry in the opposite direction. const fs = require("fs") const src = fs.readFileSync( require("path").join(__dirname, "../../src/altimate/tools/sql-execute.ts"), @@ -964,9 +969,16 @@ describe("altimate-core failure isolation", () => { expect(fpCallInsideHelperIdx).toBeGreaterThan(helperDefIdx) expect(guardComment).toBeGreaterThan(fpCallInsideHelperIdx) - // All three outcomes call the shared helper. + // Exactly two outcomes call the shared helper: success and the result-error + // branch. The catch block (thrown exception / non-execution) must not. const callSites = [...src.matchAll(/emitSqlFingerprint\(args\.query, ctx\.sessionID\)/g)] - expect(callSites.length).toBe(3) + expect(callSites.length).toBe(2) + + // The catch block itself must not reference the fingerprint helper at all. + const catchBlockStart = src.indexOf("} catch (e) {") + const catchBlockEnd = src.indexOf("\n }\n },\n})", catchBlockStart) + const catchBlockBody = src.slice(catchBlockStart, catchBlockEnd) + expect(catchBlockBody.includes("emitSqlFingerprint")).toBe(false) }) test("crash-resistant SQL inputs all handled safely", () => { From 7fdbe26c56fb156b212782c7670785cc2982aa36 Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Thu, 3 Sep 2026 12:09:46 -0700 Subject: [PATCH 3/5] docs(drivers): record the bare-scheme-vs-local-filename decision (#1238 last thread) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Documents the maintainer decision on #1238's remaining open thread (file-store.ts:61 — narrowing the bare-scheme exclusion to a closed list broke forwarding for an unlisted custom DuckDB extension's bare-scheme target). Decision: accept the closed-list default. - file-store.ts: expanded the comment at NON_SLASH_REMOTE_SCHEMES to state the fundamental ambiguity (a bare word:target cannot be told apart syntactically from a local filename with a colon), why the closed list was chosen over the broad heuristic (local filenames with colons are the common case; known extensions are enumerable), and the escape hatch (use the extension's scheme:// form, or extend the list when a new extension is adopted). - docs/configure/warehouses.md: new note in the DuckDB section explaining bare word:target values are treated as local files unless the prefix is one of the recognized bare schemes (md:, motherduck:, ducklake:). - docs/drivers.md: one-sentence pointer to the same rule alongside the existing file-backed-driver path notes. Gates: packages/drivers file-store-guard tests 27/27 pass, bun turbo typecheck 13/13 (forced, 0 cached), marker check --base origin/main --strict clean, prettier clean on file-store.ts (the two docs files were already unformatted on origin/main before this change, left as-is). Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01VqnuBDGkh1ZT65Ti7e6DHZ --- docs/docs/configure/warehouses.md | 9 +++++++++ docs/docs/drivers.md | 5 ++++- packages/drivers/src/file-store.ts | 19 +++++++++++++++++++ 3 files changed, 32 insertions(+), 1 deletion(-) diff --git a/docs/docs/configure/warehouses.md b/docs/docs/configure/warehouses.md index 9b96d18cd9..4331cc6aa1 100644 --- a/docs/docs/configure/warehouses.md +++ b/docs/docs/configure/warehouses.md @@ -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, not a + remote target — including the exact `word` prefix a DuckDB storage + extension uses. Only a full `scheme://` URI (`s3://...`, `md://...`) or + one of the specific bare schemes DuckDB extensions actually use — + `md:`, `motherduck:`, `ducklake:` — is treated as remote. To target any + other custom storage extension, use its `scheme://` form. + !!! 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. diff --git a/docs/docs/drivers.md b/docs/docs/drivers.md index c741272d35..0d7a388edf 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 1805cc4df0..ce1bad8ccd 100644 --- a/packages/drivers/src/file-store.ts +++ b/packages/drivers/src/file-store.ts @@ -21,6 +21,25 @@ import type { ConnectionConfig } from "./types" * 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 From 9ffa8b7adb1ba70daef863a0729f9c5eee9468c0 Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Thu, 3 Sep 2026 12:36:28 -0700 Subject: [PATCH 4/5] =?UTF-8?q?fix(drivers):=20resolve=20#1238's=20round-3?= =?UTF-8?q?=20threads=20=E2=80=94=20connector=20leak,=20error=20identity,?= =?UTF-8?q?=20fingerprint-on-format-throw,=20de-scope=20telemetry?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes the 3 real bugs the round-3 review caught, and de-scopes the SQL fingerprint telemetry change per plan (approved by Anand). Bug fixes, with tests: - drivers-e2e.test.ts: the DuckDB beforeAll's connectWithRetry attempt built a connector (mod.connect()) and then opened it (c.connect()) as two separate steps. If the open step failed after the connector object already existed, the connector was dropped without close(), leaking the underlying native handle on every failed retry. Extracted connectOrClose(), which closes a half-open connector before rethrowing, and wired it into the beforeAll attempt. - drivers-e2e.test.ts: connectWithRetry's final throw was a plain new Error(message), discarding the last attempt's original error object — its stack, type, and any extra properties. Now thrown with { cause: lastError }, preserving the original through unmodified. - sql-execute.ts: the fingerprint emission ran after formatResult(result), so a genuinely-executed query went uncounted if formatting itself threw. Moved the fingerprint emission before formatResult(). De-scope (the key decision): reverted sql-execute.ts's fingerprint telemetry to fingerprint-on-success-only — the behavior that predates PR #1204. Three review rounds converged on the same root problem: sql.execute's result-shaped error (returned instead of throwing) is indistinguishable between "a warehouse ran the query and it failed" and "the query never reached a warehouse at all" (no warehouse configured, connector setup failed — see connections/register.ts). Fingerprinting the result-error branch therefore risked mislabeling never-executed queries as executed SQL — the opposite of what the original #1204 comment asked for. Rather than build a failed-execution-vs-never-executed taxonomy inside this cleanup PR, picked the smaller, clearly-correct change: fingerprint only what's provably executed (the success path). Removed the now-single-use emitSqlFingerprint helper, inlining the fingerprint block back into the success path. Follow-up issue AltimateAI/altimate-code#1242 captures the real fix: an explicit executed-phase signal from the sql.execute handler itself, which sql-execute.ts can branch on once it exists. Referenced from the code comment at the de-scoped result-error branch and from the updated structural test. Gates: packages/drivers + packages/opencode/test/altimate green (5013 + 323 pass, 0 fail), bun turbo typecheck --force 13/13 (0 cached), marker check --base origin/main --strict clean, prettier clean on sql-execute.ts (the two test files were already unformatted on origin/main before this PR touched them, per established practice). Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01VqnuBDGkh1ZT65Ti7e6DHZ --- .../src/altimate/tools/sql-execute.ts | 88 ++++++-------- .../test/altimate/drivers-e2e.test.ts | 112 +++++++++++++++++- .../test/altimate/telemetry-signals.test.ts | 59 +++++---- 3 files changed, 179 insertions(+), 80 deletions(-) diff --git a/packages/opencode/src/altimate/tools/sql-execute.ts b/packages/opencode/src/altimate/tools/sql-execute.ts index cb3d39d585..39672df90b 100644 --- a/packages/opencode/src/altimate/tools/sql-execute.ts +++ b/packages/opencode/src/altimate/tools/sql-execute.ts @@ -80,13 +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 start — fingerprint a failed execution too. The - // fingerprint used to be emitted only on the success path below, so a - // failed query (this error branch, and the thrown-exception catch - // further down) never got fingerprinted at all — biasing the sql - // structure telemetry away from exactly the queries most worth seeing. - emitSqlFingerprint(args.query, ctx.sessionID) - // altimate_change end + // 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. @@ -98,10 +99,34 @@ export const SqlExecuteTool = Tool.define("sql_execute", { } // altimate_change end - let output = formatResult(result) - // altimate_change start — emit SQL structure fingerprint telemetry - emitSqlFingerprint(args.query, ctx.sessionID) + // 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) { + Telemetry.track({ + type: "sql_fingerprint", + timestamp: Date.now(), + session_id: ctx.sessionID, + statement_types: JSON.stringify(fp.statement_types), + categories: JSON.stringify(fp.categories), + table_count: fp.table_count, + function_count: fp.function_count, + has_subqueries: fp.has_subqueries, + has_aggregation: fp.has_aggregation, + has_window_functions: fp.has_window_functions, + node_count: fp.node_count, + }) + } + } catch { + // 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) { @@ -122,14 +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. This catch only fires when - // `Dispatcher.call` itself throws (dispatcher down, no warehouse configured) — - // per the comment on the result-error branch above, `sql.execute` never throws - // for a connection/query failure, it returns a result carrying `error`, which - // that branch already fingerprints. A query that reaches here never ran against - // any warehouse, so fingerprinting it here would fold "never executed" into a - // signal meant to measure "executed SQL" (success and result-error), re-biasing - // the telemetry this change is meant to correct in the opposite direction. + // 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 @@ -143,38 +163,6 @@ export const SqlExecuteTool = Tool.define("sql_execute", { }, }) -// altimate_change start — emit SQL structure fingerprint telemetry for every -// outcome where a warehouse actually ran the query: success, and a result-shaped -// error (sql.execute returns `{ ..., error }` rather than throwing for a -// connection/query failure — see the result-error branch above). Deliberately -// NOT called from the thrown-exception catch block: that path only fires when -// the query never reached a warehouse at all (e.g. dispatcher down), and -// fingerprinting a never-executed query there would bias this "executed SQL -// structure" signal toward attempts that never ran. Extracted so the two -// legitimate call sites stay in sync. -function emitSqlFingerprint(query: string, sessionID: string): void { - try { - const fp = computeSqlFingerprint(query) - if (!fp) return - Telemetry.track({ - type: "sql_fingerprint", - timestamp: Date.now(), - session_id: sessionID, - statement_types: JSON.stringify(fp.statement_types), - categories: JSON.stringify(fp.categories), - table_count: fp.table_count, - function_count: fp.function_count, - has_subqueries: fp.has_subqueries, - has_aggregation: fp.has_aggregation, - has_window_functions: fp.has_window_functions, - node_count: fp.node_count, - }) - } catch { - // Fingerprinting must never break query execution - } -} -// altimate_change end - // altimate_change start — pre-execution SQL validation via cached schema const CACHE_TTL_MS = 24 * 60 * 60 * 1000 // 24 hours // High ceiling so large warehouses aren't arbitrarily truncated; we emit diff --git a/packages/opencode/test/altimate/drivers-e2e.test.ts b/packages/opencode/test/altimate/drivers-e2e.test.ts index 69d312a17e..b4f6a131a3 100644 --- a/packages/opencode/test/altimate/drivers-e2e.test.ts +++ b/packages/opencode/test/altimate/drivers-e2e.test.ts @@ -84,12 +84,42 @@ async function connectWithRetry(attempt: (attemptNumber: number) => Promise 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. @@ -105,6 +135,22 @@ describe("connectWithRetry", () => { 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 () => { @@ -119,6 +165,55 @@ describe("connectWithRetry", () => { }) // 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 @@ -186,12 +281,17 @@ describe("DuckDB Driver E2E", () => { // early return. beforeAll(async () => { if (!duckdbAvailable) return - connector = await connectWithRetry(async () => { - const mod = await import("@altimateai/drivers/duckdb") - const c = await mod.connect({ type: "duckdb", path: ":memory:" }) - await c.connect() - return c - }, 3) + 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 diff --git a/packages/opencode/test/altimate/telemetry-signals.test.ts b/packages/opencode/test/altimate/telemetry-signals.test.ts index 04d143ef61..378b09c45b 100644 --- a/packages/opencode/test/altimate/telemetry-signals.test.ts +++ b/packages/opencode/test/altimate/telemetry-signals.test.ts @@ -942,43 +942,54 @@ describe("altimate-core failure isolation", () => { } }) - test("sql-execute fingerprints only executed outcomes (success and result-error), not a never-executed thrown exception", () => { - // altimate_change: fingerprinting used to run only on the success path, so a - // failed-but-executed query (the result-error branch) never got fingerprinted — - // biasing sql_fingerprint telemetry away from exactly the queries most worth - // seeing. It is now emitted from both outcomes where a warehouse actually ran - // the query, through one shared, try/catch-guarded helper (`emitSqlFingerprint`) - // so they cannot drift out of sync. It is deliberately NOT called from the - // thrown-exception catch block: that path only fires when the query never - // reached a warehouse at all (e.g. dispatcher down), so fingerprinting it there - // would fold "never executed" into a signal meant to measure "executed SQL", - // re-biasing the telemetry in the opposite direction. + 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", ) const execIdx = src.indexOf('Dispatcher.call("sql.execute"') - const helperDefIdx = src.indexOf("function emitSqlFingerprint(") - const fpCallInsideHelperIdx = src.indexOf("computeSqlFingerprint(query)") + 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(helperDefIdx).toBeGreaterThan(execIdx) - // The helper itself calls computeSqlFingerprint and is guarded by a try/catch. - expect(fpCallInsideHelperIdx).toBeGreaterThan(helperDefIdx) - expect(guardComment).toBeGreaterThan(fpCallInsideHelperIdx) + 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) - // Exactly two outcomes call the shared helper: success and the result-error - // branch. The catch block (thrown exception / non-execution) must not. - const callSites = [...src.matchAll(/emitSqlFingerprint\(args\.query, ctx\.sessionID\)/g)] - expect(callSites.length).toBe(2) - - // The catch block itself must not reference the fingerprint helper at all. const catchBlockStart = src.indexOf("} catch (e) {") const catchBlockEnd = src.indexOf("\n }\n },\n})", catchBlockStart) const catchBlockBody = src.slice(catchBlockStart, catchBlockEnd) - expect(catchBlockBody.includes("emitSqlFingerprint")).toBe(false) + expect(catchBlockBody.includes("computeSqlFingerprint")).toBe(false) }) test("crash-resistant SQL inputs all handled safely", () => { From 7031ead868c9d8164f2912bf96613a72146e4646 Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Thu, 3 Sep 2026 12:50:25 -0700 Subject: [PATCH 5/5] docs(warehouses): fix self-contradicting sentence in bare-scheme note (#1238 round-4 thread) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The note added to explain the bare-word:target-vs-local-filename tradeoff read as self-contradicting: it said a bare word:target is "treated as an ordinary local filename... including the exact word prefix a DuckDB storage extension uses," which the very next sentence then said IS treated as remote for md:/motherduck:/ducklake:. Reworded in the intended, unambiguous order: local-by-default, named exceptions (md:/motherduck:/ducklake:) are remote, scheme:// forces remote for anything else. docs/drivers.md's one-line pointer already used this order and doesn't have the same confusion — left unchanged. Gates: marker check --base origin/main --strict clean. This file is part of the same pre-existing-formatting-drift set established earlier in this PR (not reformatted, per established practice). Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01VqnuBDGkh1ZT65Ti7e6DHZ --- docs/docs/configure/warehouses.md | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/docs/docs/configure/warehouses.md b/docs/docs/configure/warehouses.md index 4331cc6aa1..02bdff8949 100644 --- a/docs/docs/configure/warehouses.md +++ b/docs/docs/configure/warehouses.md @@ -285,12 +285,12 @@ If you're already authenticated via `gcloud`, omit `credentials_path`: !!! 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, not a - remote target — including the exact `word` prefix a DuckDB storage - extension uses. Only a full `scheme://` URI (`s3://...`, `md://...`) or - one of the specific bare schemes DuckDB extensions actually use — - `md:`, `motherduck:`, `ducklake:` — is treated as remote. To target any - other custom storage extension, use its `scheme://` form. + `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.