Skip to content

Commit 60340ff

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. The project root comes from `Instance.directory`, not `process.cwd()`. A server or `run --attach` session never chdirs — it carries the project in the instance context and leaves cwd at the server's launch directory — so cwd there is somebody else's project. `localConfigPath()` had that bug already; absolutizing at load time would have baked it in permanently. 2. Never conjure a store that was meant to exist. New `drivers/src/file-store.ts` holds `assertStoreExists()`, which both file-backed drivers call before opening: a missing local file throws an error naming the path it looked for. Creation is opt-in via `create: true`, which `schema_sync` and `test_local` — the two tools that deliberately materialize a scratch store — now pass. Exempt from the check: the exact string `:memory:`, an empty path, and scheme-qualified targets (`md:`, `s3://`, `ducklake:`, anything a DuckDB extension provides), which the driver reports as an unknown scheme rather than creating. Only the EXACT `:memory:` — DuckDB writes a real file for `:memory:named` and for `:foo`, so those stay inside the guard. Windows drive letters stay paths. `bun:sqlite` rejects an options object with no open flag, so the SQLite driver now passes `readwrite` explicitly where `create` used to imply it. Relative SQLite `file:` URIs are deliberately NOT handled here, and are filed as #1209 with the evidence. They follow cwd the same way, but `bun:sqlite` parses `file:` as a URI on macOS and appears to treat it as a literal filename on Linux, and Windows is unverified. A rewrite changes which database opens, so shipping one across platforms it has not been proven on would create the same class of defect this fixes. Verified through a compiled binary built with the production build options, invoked with `--dir` from an unrelated working directory. Before: `tables: []` plus a stray `warehouse.db` (+ `-wal`, `-shm`) written into the `--dir` target; the DuckDB arm left a 12,288-byte stray `.duckdb`. After: the real table, and nothing created. `test/altimate/store-path-resolution.test.ts` compiles that binary and runs five arms — including a server-style request driven through `Instance.provide` with a decoy config and store in the launch directory — of which three fail on unmodified main. Existing tests that relied on create-on-open to build their own scratch store now pass `create: true`. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01VqnuBDGkh1ZT65Ti7e6DHZ
1 parent 136c695 commit 60340ff

19 files changed

Lines changed: 810 additions & 27 deletions

‎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

@@ -118,6 +119,9 @@ export async function connect(config: ConnectionConfig): Promise<Connector> {
118119

119120
return {
120121
async connect() {
122+
// altimate_change start — never conjure an empty store on open
123+
assertStoreExists(config, dbPath, "DuckDB")
124+
// altimate_change end
121125
// altimate_change start — retry with read-only on lock errors
122126
const tryConnect = (accessMode?: string): Promise<any> =>
123127
new Promise<any>((resolve, reject) => {

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

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
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+
* Whether `dbPath` names a file on the local filesystem, and so can be
20+
* existence-checked before the driver opens it.
21+
*
22+
* Only the exact string `:memory:` is an in-memory database. Both engines
23+
* treat `:memory:named` — and any other colon-prefixed name — as an ordinary
24+
* (if oddly named) file: DuckDB really does write a file called
25+
* `:memory:named`, so those must stay inside the guard. An empty path is
26+
* DuckDB's in-memory database and SQLite's anonymous temporary one; neither
27+
* touches disk.
28+
*
29+
* A scheme-qualified target is not a local file: MotherDuck (`md:`), object
30+
* storage (`s3://`), DuckLake, and any other scheme a DuckDB extension
31+
* provides. Those are left to the driver, which reports an unknown scheme as a
32+
* missing-extension error rather than silently creating anything. The pattern
33+
* requires two or more characters before the colon so a Windows drive letter
34+
* (`C:\data\wh.duckdb`) stays a path.
35+
*/
36+
export function isLocalFilePath(dbPath: string): boolean {
37+
if (dbPath === "" || dbPath === ":memory:") return false
38+
if (/^[a-zA-Z][a-zA-Z0-9+.-]+:/.test(dbPath)) return false
39+
return true
40+
}
41+
42+
/** Whether the caller explicitly opted in to creating the store. */
43+
export function allowsCreate(config: ConnectionConfig): boolean {
44+
return config.create === true
45+
}
46+
47+
/**
48+
* Throw unless the store is safe to open: it already exists, the caller opted
49+
* in to creating it, or the path is not a local file at all.
50+
*
51+
* @param engine Human-readable engine name used in the error message.
52+
* @param allowCreate Whether this open will actually create the store. Defaults
53+
* to the config's `create` flag; a driver passes it explicitly when its own
54+
* options can veto creation — SQLite never creates a read-only connection.
55+
*/
56+
export function assertStoreExists(
57+
config: ConnectionConfig,
58+
dbPath: string,
59+
engine: string,
60+
allowCreate: boolean = allowsCreate(config),
61+
): void {
62+
if (allowCreate) return
63+
if (!isLocalFilePath(dbPath)) return
64+
if (fs.existsSync(dbPath)) return
65+
throw new Error(
66+
`${engine} database file not found: "${dbPath}". ` +
67+
`Opening a warehouse connection never creates the database — an empty store would answer every query with no rows. ` +
68+
`Check the "path" in your connection config (relative paths resolve against the config file's directory, not the current directory), ` +
69+
`or pass "create": true if this store is meant to be created.`,
70+
)
71+
}

‎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: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -178,7 +178,7 @@ describe("DuckDB driver", () => {
178178
}))
179179

180180
const { connect } = await import("../src/duckdb")
181-
const connector = await connect({ type: "duckdb", path: "/tmp/test.duckdb" })
181+
const connector = await connect({ type: "duckdb", path: "/tmp/test.duckdb", create: true })
182182
await connector.connect()
183183
expect(connectAttempts).toBe(2) // First failed, second succeeded in READ_ONLY
184184
// The retry must specifically request READ_ONLY — two attempts alone don't
@@ -219,7 +219,11 @@ describe("DuckDB driver", () => {
219219
}))
220220

221221
const { connect } = await import("../src/duckdb")
222-
const connector = await connect({ type: "duckdb", path: "/tmp/test.duckdb" })
222+
// altimate_change start — bypass the store-existence guard for this mock:
223+
// it never touches the real filesystem, and the test targets lock-error
224+
// classification, not the existence check.
225+
const connector = await connect({ type: "duckdb", path: "/tmp/test.duckdb", create: true })
226+
// altimate_change end
223227

224228
try {
225229
await connector.connect()
@@ -267,7 +271,10 @@ describe("DuckDB driver", () => {
267271
}))
268272

269273
const { connect } = await import("../src/duckdb")
270-
const connector = await connect({ type: "duckdb", path: "/tmp/test.duckdb" })
274+
// altimate_change start — bypass the store-existence guard for this mock;
275+
// see the identical note above.
276+
const connector = await connect({ type: "duckdb", path: "/tmp/test.duckdb", create: true })
277+
// altimate_change end
271278
await connector.connect()
272279

273280
expect(attempts).toBe(2)
@@ -306,7 +313,11 @@ describe("DuckDB driver", () => {
306313
}))
307314

308315
const { connect } = await import("../src/duckdb")
309-
const connector = await connect({ type: "duckdb", path: "/tmp/test.duckdb", readonly: true })
316+
// altimate_change start — bypass the store-existence guard for this mock;
317+
// see the identical note above. `create` and `readonly` are independent
318+
// flags: `create` only controls the existence check, not access mode.
319+
const connector = await connect({ type: "duckdb", path: "/tmp/test.duckdb", readonly: true, create: true })
320+
// altimate_change end
310321

311322
try {
312323
await connector.connect()
@@ -405,7 +416,7 @@ describe("DuckDB driver", () => {
405416
}))
406417

407418
const { connect } = await import("../src/duckdb")
408-
const connector = await connect({ type: "duckdb", path: "/tmp/sync-lock.duckdb" })
419+
const connector = await connect({ type: "duckdb", path: "/tmp/sync-lock.duckdb", create: true })
409420
await connector.connect()
410421
expect(connectAttempts).toBe(2)
411422
expect(await connector.execute("SELECT 1")).toMatchObject({ columns: ["ok"], rows: [[1]], row_count: 1 })
@@ -430,7 +441,7 @@ describe("DuckDB driver", () => {
430441
}))
431442

432443
const { connect } = await import("../src/duckdb")
433-
const connector = await connect({ type: "duckdb", path: "/tmp/corrupt.duckdb" })
444+
const connector = await connect({ type: "duckdb", path: "/tmp/corrupt.duckdb", create: true })
434445
await expect(connector.connect()).rejects.toThrow("catalog is corrupt")
435446
})
436447
})

0 commit comments

Comments
 (0)