Skip to content

Commit 3e5fdff

Browse files
anandgupta42claude
andcommitted
fix: [#1203] --dir silently read a populated warehouse store as empty
`--dir` calls `process.chdir()` (`cli/cmd/run.ts:412`). `ConnectionRegistry` never absolutized the `path` field, so a relative store path in a connection config resolved against whatever working directory the process happened to have when the driver opened it. Both file-backed drivers then answered the miss by CREATING an empty database — `new duckdb.Database(path)` in `drivers/src/duckdb.ts`, `create: !isReadonly` in `drivers/src/sqlite.ts`. Every query afterwards succeeded and returned zero rows, with no error and no fault signal for a caller to key on. Two fixes, either of which breaks the chain; both are needed because the second is the root hazard and is independent of `--dir`: 1. Resolve store paths deterministically. `resolveStorePaths()` absolutizes a relative `path` once, at config-load time, against the directory that declared it — global config against `~/.altimate-code`, project config and `ALTIMATE_CODE_CONN_*` against the project root. A later `chdir` cannot move it. `warehouse_add` persists the absolute form for the same reason. 2. Never conjure a store that was meant to exist. New `drivers/src/file-store.ts` holds `assertStoreExists()`, which both file-backed drivers call before opening: a missing local file throws an error naming the path it looked for. Creation is opt-in via `create: true`, which `schema_sync` and `test_local` — the two tools that deliberately materialize a scratch store — now pass. In-memory targets (`:memory:`) and scheme-qualified ones (`md:`, `s3://`) are exempt; Windows drive letters are not mistaken for schemes. `bun:sqlite` rejects an options object with no open flag, so the SQLite driver now passes `readwrite` explicitly where `create` used to imply it. Verified through a compiled binary built with the production build options, invoked with `--dir` from an unrelated working directory. Before: `tables: []` plus a stray `warehouse.db` (+ `-wal`, `-shm`) written into the `--dir` target; the DuckDB arm left a 12,288-byte stray `.duckdb`. After: the real table, and nothing created. `test/altimate/store-path-resolution.test.ts` compiles that binary and runs all three arms; it fails on unmodified main. Existing tests that relied on create-on-open to build their own scratch store now pass `create: true`. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01VqnuBDGkh1ZT65Ti7e6DHZ
1 parent babc7cb commit 3e5fdff

16 files changed

Lines changed: 632 additions & 20 deletions

File tree

‎docs/docs/configure/warehouses.md‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -262,6 +262,26 @@ If you're already authenticated via `gcloud`, omit `credentials_path`:
262262
| Field | Required | Description |
263263
|-------|----------|-------------|
264264
| `path` | No | Database file path. Omit or use `":memory:"` for in-memory |
265+
| `create` | No | Create the database file if it is missing (default: `false`) |
266+
267+
!!! warning "The store must already exist"
268+
Connecting never creates the database. If the file is missing, the connection
269+
fails with an error naming the path it looked for — an empty database would
270+
otherwise answer every query with no rows and no error, which reads as a
271+
successful query against an empty warehouse. Set `"create": true` only when
272+
you intend this store to be created.
273+
274+
!!! note "How a relative `path` is resolved"
275+
A relative `path` is resolved once, when the config is loaded, against the
276+
directory that declares it:
277+
278+
- `~/.altimate-code/connections.json` → resolved against `~/.altimate-code`
279+
- `<project>/.altimate-code/connections.json` → resolved against `<project>`
280+
- `ALTIMATE_CODE_CONN_*` environment variables → resolved against the project root
281+
282+
It is never resolved against the current working directory, so `--dir` cannot
283+
re-point an existing connection at a different file. Absolute paths are always
284+
safest.
265285

266286
!!! note "Concurrent access"
267287
DuckDB does not support concurrent write access to the same file. If another process holds a write lock, Altimate Code automatically retries the connection in **read-only** mode so you can still query the data. A clear error message is shown if read-only access also fails.
@@ -443,10 +463,16 @@ If you're already authenticated via `gcloud`, omit `credentials_path`:
443463
|-------|----------|-------------|
444464
| `path` | No | Database file path. Omit or use `":memory:"` for in-memory |
445465
| `readonly` | No | Open in read-only mode (default: `false`) |
466+
| `create` | No | Create the database file if it is missing (default: `false`) |
446467

447468
!!! note
448469
SQLite uses Bun's built-in `bun:sqlite` driver. WAL journal mode is enabled automatically for writable databases.
449470

471+
!!! warning "The store must already exist"
472+
As with DuckDB, connecting never creates the database, and a relative `path`
473+
resolves against the directory of the config that declares it — not the current
474+
working directory. See the DuckDB section above for the full rules.
475+
450476
## SQL Server
451477

452478
```json

‎docs/docs/drivers.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -191,6 +191,11 @@ MongoDB supports server versions 3.6 through 8.0. Queries use MQL (MongoDB Query
191191
|--------|--------------|
192192
| File | `path: "./my-database.sqlite"` |
193193

194+
For both file-backed drivers, the database must already exist — connecting never
195+
creates it. Pass `create: true` to create it deliberately. A relative `path`
196+
resolves against the directory of the config that declares it, not the current
197+
working directory. See [Warehouses](configure/warehouses.md#duckdb) for the full rules.
198+
194199
## SSH Tunneling
195200

196201
Connect through a bastion host by adding SSH config to any connection:

‎packages/drivers/src/duckdb.ts‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
* DuckDB driver using the `duckdb` package.
33
*/
44

5+
import { assertStoreExists } from "./file-store"
56
import type { ConnectionConfig, Connector, ConnectorResult, ExecuteOptions, SchemaColumn } from "./types"
67
import { loadOptionalDriver } from "./resolve"
78

@@ -48,6 +49,9 @@ export async function connect(config: ConnectionConfig): Promise<Connector> {
4849

4950
return {
5051
async connect() {
52+
// altimate_change start — never conjure an empty store on open
53+
assertStoreExists(config, dbPath, "DuckDB")
54+
// altimate_change end
5155
// altimate_change start — retry with read-only on lock errors
5256
const tryConnect = (accessMode?: string): Promise<any> =>
5357
new Promise<any>((resolve, reject) => {

‎packages/drivers/src/file-store.ts‎

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
/**
2+
* Shared guards for file-backed drivers (DuckDB, SQLite).
3+
*
4+
* Both engines create an empty database when asked to open a file that does
5+
* not exist. For a warehouse connection that is never what the caller wants:
6+
* a mistyped or mis-resolved path then yields a working connector over an
7+
* empty database, so every query succeeds and returns nothing. An agent handed
8+
* that result reports "no tables" instead of an error.
9+
*
10+
* Opening a store is therefore read-or-fail by default. Creation is opt-in via
11+
* `create: true`, which the tools that deliberately materialize a local store
12+
* (local test scratch databases, schema sync targets) pass explicitly.
13+
*/
14+
15+
import * as fs from "fs"
16+
import type { ConnectionConfig } from "./types"
17+
18+
/**
19+
* Paths that do not name a file on the local filesystem and so cannot be
20+
* existence-checked: in-memory databases (`:memory:`, `:memory:name`), the
21+
* SQLite anonymous temporary database (empty string), and scheme-qualified
22+
* targets such as MotherDuck (`md:mydb`) or object storage (`s3://…`).
23+
*
24+
* The scheme pattern requires at least two characters before the colon so a
25+
* Windows drive letter (`C:\data\wh.duckdb`) is treated as a local path.
26+
*/
27+
export function isLocalFilePath(dbPath: string): boolean {
28+
if (dbPath === "" || dbPath.startsWith(":")) return false
29+
if (/^[a-zA-Z][a-zA-Z0-9+.-]+:/.test(dbPath)) return false
30+
return true
31+
}
32+
33+
/** Whether the caller explicitly opted in to creating the store. */
34+
export function allowsCreate(config: ConnectionConfig): boolean {
35+
return config.create === true
36+
}
37+
38+
/**
39+
* Throw unless the store is safe to open: it already exists, the caller opted
40+
* in to creating it, or the path is not a local file at all.
41+
*
42+
* @param engine Human-readable engine name used in the error message.
43+
* @param allowCreate Whether this open will actually create the store. Defaults
44+
* to the config's `create` flag; a driver passes it explicitly when its own
45+
* options can veto creation — SQLite never creates a read-only connection.
46+
*/
47+
export function assertStoreExists(
48+
config: ConnectionConfig,
49+
dbPath: string,
50+
engine: string,
51+
allowCreate: boolean = allowsCreate(config),
52+
): void {
53+
if (allowCreate) return
54+
if (!isLocalFilePath(dbPath)) return
55+
if (fs.existsSync(dbPath)) return
56+
throw new Error(
57+
`${engine} database file not found: "${dbPath}". ` +
58+
`Opening a warehouse connection never creates the database — an empty store would answer every query with no rows. ` +
59+
`Check the "path" in your connection config (relative paths resolve against the config file's directory, not the current directory), ` +
60+
`or pass "create": true if this store is meant to be created.`,
61+
)
62+
}

‎packages/drivers/src/index.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,9 @@ export type { Connector, ConnectorResult, SchemaColumn, ConnectionConfig } from
44
// Re-export config normalization
55
export { normalizeConfig, sanitizeConnectionString } from "./normalize"
66

7+
// Re-export file-backed store guards
8+
export { allowsCreate, assertStoreExists, isLocalFilePath } from "./file-store"
9+
710
// Re-export driver connect functions
811
export { connect as connectPostgres } from "./postgres"
912
export { connect as connectSnowflake } from "./snowflake"

‎packages/drivers/src/sqlite.ts‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
*/
55

66
import { Database } from "bun:sqlite"
7+
import { allowsCreate, assertStoreExists } from "./file-store"
78
import type { ConnectionConfig, Connector, ConnectorResult, ExecuteOptions, SchemaColumn } from "./types"
89

910
export async function connect(config: ConnectionConfig): Promise<Connector> {
@@ -13,10 +14,20 @@ export async function connect(config: ConnectionConfig): Promise<Connector> {
1314
return {
1415
async connect() {
1516
const isReadonly = config.readonly === true
17+
// altimate_change start — never conjure an empty store on open.
18+
// A read-only connection never creates, so `create: true` cannot excuse a
19+
// missing file there; the guard is told the effective decision.
20+
const willCreate = !isReadonly && allowsCreate(config)
21+
assertStoreExists(config, dbPath, "SQLite", willCreate)
1622
db = new Database(dbPath, {
1723
readonly: isReadonly,
18-
create: !isReadonly,
24+
// `create` alone is no longer implied by "not readonly", so the
25+
// read-write flag has to be explicit — bun:sqlite rejects an options
26+
// object that sets no open flag at all.
27+
readwrite: !isReadonly,
28+
create: willCreate,
1929
})
30+
// altimate_change end
2031
if (!isReadonly) {
2132
db.exec("PRAGMA journal_mode = WAL")
2233
}

‎packages/drivers/test/driver-security.test.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -168,7 +168,7 @@ describe("DuckDB driver", () => {
168168
}))
169169

170170
const { connect } = await import("../src/duckdb")
171-
const connector = await connect({ type: "duckdb", path: "/tmp/test.duckdb" })
171+
const connector = await connect({ type: "duckdb", path: "/tmp/test.duckdb", create: true })
172172
await connector.connect()
173173
expect(connectAttempts).toBe(2) // First failed, second succeeded in READ_ONLY
174174
// The retry must specifically request READ_ONLY — two attempts alone don't
@@ -262,7 +262,7 @@ describe("DuckDB driver", () => {
262262
}))
263263

264264
const { connect } = await import("../src/duckdb")
265-
const connector = await connect({ type: "duckdb", path: "/tmp/sync-lock.duckdb" })
265+
const connector = await connect({ type: "duckdb", path: "/tmp/sync-lock.duckdb", create: true })
266266
await connector.connect()
267267
expect(connectAttempts).toBe(2)
268268
expect(await connector.execute("SELECT 1")).toMatchObject({ columns: ["ok"], rows: [[1]], row_count: 1 })
@@ -287,7 +287,7 @@ describe("DuckDB driver", () => {
287287
}))
288288

289289
const { connect } = await import("../src/duckdb")
290-
const connector = await connect({ type: "duckdb", path: "/tmp/corrupt.duckdb" })
290+
const connector = await connect({ type: "duckdb", path: "/tmp/corrupt.duckdb", create: true })
291291
await expect(connector.connect()).rejects.toThrow("catalog is corrupt")
292292
})
293293
})
Lines changed: 153 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,153 @@
1+
/**
2+
* Unit tests for the file-backed store guards (src/file-store.ts).
3+
*
4+
* Both DuckDB and SQLite create an empty database when opened on a path that
5+
* does not exist. For a warehouse connection that turns a wrong path into a
6+
* silent empty result set rather than an error, so opening is read-or-fail
7+
* unless the caller passes `create: true`.
8+
*
9+
* The end-to-end proof — a populated store reached through `--dir` from an
10+
* unrelated cwd, run against a compiled binary — lives in
11+
* packages/opencode/test/altimate/store-path-resolution.test.ts.
12+
*/
13+
import { describe, test, expect, mock, afterEach } from "bun:test"
14+
import { Database } from "bun:sqlite"
15+
import * as fs from "fs"
16+
import * as os from "os"
17+
import * as path from "path"
18+
import { allowsCreate, assertStoreExists, isLocalFilePath } from "../src/file-store"
19+
20+
const CANARY_TABLE = "zorbulax_ledger"
21+
22+
const tmpDirs: string[] = []
23+
function tmp(): string {
24+
const dir = fs.mkdtempSync(path.join(os.tmpdir(), "file-store-guard-"))
25+
tmpDirs.push(dir)
26+
return dir
27+
}
28+
afterEach(() => {
29+
while (tmpDirs.length) fs.rmSync(tmpDirs.pop()!, { recursive: true, force: true })
30+
})
31+
32+
describe("isLocalFilePath", () => {
33+
test("treats in-memory and scheme-qualified targets as non-files", () => {
34+
expect(isLocalFilePath(":memory:")).toBe(false)
35+
expect(isLocalFilePath(":memory:named")).toBe(false)
36+
expect(isLocalFilePath("")).toBe(false)
37+
expect(isLocalFilePath("md:my_database")).toBe(false)
38+
expect(isLocalFilePath("motherduck:my_database")).toBe(false)
39+
expect(isLocalFilePath("s3://bucket/warehouse.duckdb")).toBe(false)
40+
})
41+
42+
test("treats real paths — including Windows drive letters — as files", () => {
43+
expect(isLocalFilePath("warehouse.duckdb")).toBe(true)
44+
expect(isLocalFilePath("./data/warehouse.duckdb")).toBe(true)
45+
expect(isLocalFilePath("/var/data/warehouse.duckdb")).toBe(true)
46+
expect(isLocalFilePath("C:\\data\\warehouse.duckdb")).toBe(true)
47+
})
48+
})
49+
50+
describe("assertStoreExists", () => {
51+
test("throws for a missing local file, naming the path it looked for", () => {
52+
const missing = path.join(tmp(), "absent.duckdb")
53+
expect(() => assertStoreExists({ type: "duckdb" }, missing, "DuckDB")).toThrow(missing)
54+
expect(() => assertStoreExists({ type: "duckdb" }, missing, "DuckDB")).toThrow("not found")
55+
expect(fs.existsSync(missing)).toBe(false)
56+
})
57+
58+
test("passes for an existing file, an in-memory target, or an explicit create", () => {
59+
const dir = tmp()
60+
const present = path.join(dir, "present.duckdb")
61+
fs.writeFileSync(present, "")
62+
expect(() => assertStoreExists({ type: "duckdb" }, present, "DuckDB")).not.toThrow()
63+
expect(() => assertStoreExists({ type: "duckdb" }, ":memory:", "DuckDB")).not.toThrow()
64+
expect(() =>
65+
assertStoreExists({ type: "duckdb", create: true }, path.join(dir, "new.duckdb"), "DuckDB"),
66+
).not.toThrow()
67+
})
68+
69+
test("allowsCreate only accepts a literal true", () => {
70+
expect(allowsCreate({ type: "duckdb", create: true })).toBe(true)
71+
expect(allowsCreate({ type: "duckdb", create: "true" })).toBe(false)
72+
expect(allowsCreate({ type: "duckdb" })).toBe(false)
73+
})
74+
})
75+
76+
describe("DuckDB driver create-on-open", () => {
77+
test("refuses to open a missing store and creates nothing", async () => {
78+
// The real duckdb addon is an optional dependency; the guard runs before
79+
// any Database is constructed, so a stub is enough to prove it fires first.
80+
let constructed = false
81+
mock.module("duckdb", () => ({
82+
default: {
83+
Database: class {
84+
constructor(_p: string, optsOrCb: any, cb?: (err: Error | null) => void) {
85+
constructed = true
86+
const done = typeof optsOrCb === "function" ? optsOrCb : cb!
87+
setTimeout(() => done(null), 0)
88+
}
89+
connect() {
90+
return {}
91+
}
92+
close(cb: any) {
93+
if (cb) cb(null)
94+
}
95+
},
96+
},
97+
}))
98+
99+
const missing = path.join(tmp(), "absent.duckdb")
100+
const { connect } = await import("../src/duckdb")
101+
const connector = await connect({ type: "duckdb", path: missing })
102+
103+
await expect(connector.connect()).rejects.toThrow("not found")
104+
expect(constructed).toBe(false)
105+
expect(fs.existsSync(missing)).toBe(false)
106+
})
107+
})
108+
109+
describe("SQLite driver create-on-open", () => {
110+
test("reads an existing store", async () => {
111+
const store = path.join(tmp(), "warehouse.db")
112+
const seed = new Database(store, { create: true })
113+
seed.exec(`CREATE TABLE ${CANARY_TABLE}(id INTEGER)`)
114+
seed.close()
115+
116+
const { connect } = await import("../src/sqlite")
117+
const connector = await connect({ type: "sqlite", path: store })
118+
await connector.connect()
119+
const tables = await connector.listTables("main")
120+
await connector.close()
121+
122+
expect(tables.map((t) => t.name)).toContain(CANARY_TABLE)
123+
})
124+
125+
test("refuses to open a missing store and creates nothing", async () => {
126+
const missing = path.join(tmp(), "absent.db")
127+
const { connect } = await import("../src/sqlite")
128+
const connector = await connect({ type: "sqlite", path: missing })
129+
130+
await expect(connector.connect()).rejects.toThrow("not found")
131+
expect(fs.existsSync(missing)).toBe(false)
132+
})
133+
134+
test("refuses a read-only connection to a missing store even with create: true", async () => {
135+
// A read-only open never creates, so `create` must not excuse the miss.
136+
const missing = path.join(tmp(), "absent-readonly.db")
137+
const { connect } = await import("../src/sqlite")
138+
const connector = await connect({ type: "sqlite", path: missing, readonly: true, create: true })
139+
140+
await expect(connector.connect()).rejects.toThrow("not found")
141+
expect(fs.existsSync(missing)).toBe(false)
142+
})
143+
144+
test("creates only when the caller opts in", async () => {
145+
const target = path.join(tmp(), "scratch.db")
146+
const { connect } = await import("../src/sqlite")
147+
const connector = await connect({ type: "sqlite", path: target, create: true })
148+
await connector.connect()
149+
await connector.close()
150+
151+
expect(fs.existsSync(target)).toBe(true)
152+
})
153+
})

0 commit comments

Comments
 (0)