fix: --dir silently read a populated warehouse store as empty

Merged
AN
2mo ago
+1050-30

Issue for this PR

Closes #1203

Type of change

  • Bug fix
  • New feature
  • Refactor / code improvement
  • Documentation

What does this PR do?

Read this first — the diagnosis changed. Field verification against the real rig showed the first version of this PR did not help. --dir still returned "no tables", nothing was created anywhere on disk, and the store was byte-identical before and after. Pointed at a store that does not exist, the run returned "no tables" with no error at all, with or without --dir.

So the reported defect was not primarily silent creation. A failure was shaped like success at every layer between the driver and the model:

  1. (config.path as string) ?? ":memory:" in both file-backed drivers. Any failure to carry a path became a successful connection to an empty in-memory database — worse than silent creation, because it leaves nothing on disk to explain the empty answer.
  2. sql.execute never throws; it returns { columns: [], rows: [], row_count: 0, error } (register.ts:437), demoting the error to a field on a success-shaped result.
  3. formatResult() returned the literal string "(0 rows)" for row_count === 0 and never read error. An agent asking what tables exist was told the warehouse was healthy and empty, then offered a tip about query optimization.

assertStoreExists() was never bypassed — it fired, and its message was discarded one layer above. My test proved the guard worked only because it called Registry.get() directly instead of the path the product takes. The compiled-binary arms now go through Dispatcher.call("sql.execute"), and the rendering is tested through the sql_execute tool itself.

Reproduced on unmodified main at the tool layer: output: "(0 rows)\n\nTip: Use sql_analyze to check this query...".

Added: requireStorePath() (a missing or empty path is rejected; :memory: must be asked for), sql_execute surfacing result.error via normalizeError, and test/altimate/warehouse-failure-visibility.test.ts — six tests, three failing on main, including the two cases that must not change: an explicit :memory:, and a genuinely empty result set still rendering as (0 rows).

#1210 tracks the broader class: ten more tools call the dispatcher and never check error.

Passing --dir made a populated warehouse store read as empty with no error at all. Two independent defects composed to produce it, and each is fixed separately.

1. A relative store path followed the working directory.

--dir calls process.chdir(args.dir) (packages/opencode/src/cli/cmd/run.ts:412). ConnectionRegistry.load() stored the path field verbatim, so a relative path in ~/.altimate-code/connections.json was resolved by the driver against whatever cwd the process had at open time. --dir therefore re-pointed an existing connection at a different file.

resolveStorePaths() now absolutizes a relative path once, at config-load time, against the directory that declared it:

SourceBase
~/.altimate-code/connections.json~/.altimate-code (the config file's own directory)
<project>/.altimate-code/connections.json<project> (the project root)
ALTIMATE_CODE_CONN_*the project root

I chose the declaring directory rather than the project root for everything because the global config is shared across every project — a path relative to "wherever you happen to be" has no stable meaning there, while a path relative to the config file does. The project-local config and the env vars are already scoped to one project, so the project root is the natural base for those. Absolute paths and non-file targets are passed through untouched. warehouse_add persists the absolute form for the same reason — otherwise it writes a cwd-dependent entry into the shared global config.

Two things a Codex review turned up here, both fixed:

The project root is Instance.directory, not process.cwd(). A server session or run --attach never chdirs — it carries the project in the instance context and leaves the working directory wherever the server was launched. localConfigPath() already read process.cwd() before this PR, so it was already looking for the project-local config in the wrong place; absolutizing at load time would have baked that mistake into the saved global config permanently. projectRoot() prefers Instance.directory and falls back to cwd when there is no instance context — early CLI paths and unit tests, where cwd is the right answer because run --dir has already chdir'd.

Relative SQLite file: URIs are deliberately left alone, and filed as #1209 with the evidence. They follow cwd the same way, and worse — the create guard cannot catch them, because the failure is opening the wrong existing database rather than making a new empty one. I implemented the rewrite, and then removed it: bun:sqlite parses file: as a URI on macOS, where I reproduced the decoy read, but appears to treat it as a literal filename on Linux, where my test failed in CI, 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 PR fixes. #1209 records the reproduction, the four separate regressions the attempt produced during review, and what a correct implementation needs.

SQLite shares the flaw. Its create: !isReadonly was the same hazard as DuckDB's, and it is fixed the same way. Of the other file-backed possibilities, only these two take a local path; the remaining drivers are network-backed and cannot conjure a store. Note the existing test readonly connection does not create nonexistent file already asserted this property for read-only SQLite connections — this PR extends the same guarantee to read-write ones.

Not overlapping the driver-resolution work. #1122, #1192, #1198 and #1201 all deal with loading the driver module (bunfs bare-specifier resolution, install locking, open timeouts). None of them touches path resolution or create-on-open. #1122 landed in main while I was working; this branch is rebased on top of it and its loadOptionalDriver() change to duckdb.ts sits directly above my four added lines with no conflict. #1198 is still open and edits the same connect() function, so expect a small textual conflict there — my change is four lines at the top of it, before tryConnect is defined.

How did you verify your code works?

Compiled binary, production build options, unrelated working directory. A bun test run cannot see this defect — the package's own node_modules stays reachable and the cwd is the runner's, not the rig's. So the proof runs a binary compiled the way script/build.ts compiles the shipped one (bundled sources, warehouse SDKs external, no bunfig/dotenv autoload), started from a directory unrelated to both the store and the project, and given --dir.

The store is seeded with a table named zorbulax_ledger, so a pass cannot come from anything except actually reading the file.

Before, on unmodified main:

{"ok":true,"cwd":".../project","tables":[]}

files created under the --dir target:
  .../project/warehouse.db       4096 bytes
  .../project/warehouse.db-wal      0 bytes
  .../project/warehouse.db-shm  32768 bytes

After:

{"ok":true,"cwd":".../project","tables":["zorbulax_ledger"]}

files created under the --dir target:
  (none)

The DuckDB arm behaves identically and leaves a 12,288-byte stray .duckdb file, which matches the stray 12 KB file an earlier investigation found in the wrong directory.

packages/opencode/test/altimate/store-path-resolution.test.ts compiles that binary in beforeAll and runs five arms: the reported case; a missing store that must fail loudly; a project-local config; a server-style request driven through Instance.provide with a decoy config and store planted in the launch directory, so a wrong resolution reads plausible wrong data rather than nothing; and an explicit create: true that must still work. The arms that can leak assert that no stray database file appears anywhere. Three of the five fail on unmodified main, which I confirmed by reverting only the source changes and re-running. The child process is also stripped of any ambient ALTIMATE_CODE_CONN_* variable, since those override both config files and would otherwise decide the assertions.

packages/drivers/test/file-store-guard.test.ts covers the guard directly: path classification (:memory:, md:, s3://, Windows drive letters), the DuckDB guard firing before any Database is constructed, and the SQLite read/refuse/create-on-opt-in paths.

Gates, all from a fresh bun install in this worktree:

  • bun test --cwd packages/drivers — 235 pass, 0 fail (7 files)
  • bun test --cwd packages/opencode — 11,562 pass, 9 fail across 12,394 tests / 606 files. All nine failures are pre-existing and live outside test/altimate: five in test/mcp/headers.test.ts, one each in test/server/httpapi-experimental.test.ts, the mcp HttpApi status endpoint, test/release-validation/mcp-datamate-893-codex.test.ts, and test/cli/run/run-process.test.ts. None of those files imports anything this PR touches, and I confirmed it directly: with my source changes reverted to origin/main, the same MCP/HTTP failures reproduce. The whole test/altimate tree — which is where every warehouse, driver, and connection test lives — is green: 4,231 pass, 0 fail across 152 files
  • bun turbo typecheck — 13/13 successful
  • bun run script/upstream/analyze.ts --markers --base origin/main --require-markers --strict — no upstream-shared files modified; 35/35 marker files valid
  • bun run script/upstream/analyze.ts --branding — pass
  • bun run lint — 1 error, pre-existing and environmental: typescript(tsconfig-error): Cannot find type definition file for 'bun-types', from script/upstream/tsconfig.json:6, which this branch does not touch and which has no bun-types install to resolve against. My changed files lint with 0 errors of their own.
  • prettier — my changed files introduce no formatting drift; registry.ts was clean at HEAD and is re-formatted, the other touched files were already unformatted before this PR and are left alone rather than reformatted as noise.

Nine existing tests relied on create-on-open to build their own scratch store and now pass create: true: four in schema-cache.test.ts, two in drivers-e2e.test.ts, three in driver-security.test.ts (which mocks the duckdb module outright, so its fixture files never existed on disk). Those are behaviour changes to the tests, not workarounds — each of them is a case where the caller genuinely does intend to create the store.

What I did not verify. Two things, stated plainly.

The full CLI end to end with a live model. altimate-code run --dir needs an LLM to invoke a warehouse tool, so the compiled-binary proof drives the same registry and driver code through a fixture entrypoint that reproduces run.ts's --dir chdir exactly, rather than through the agent loop.

The DuckDB arm inside the automated test. On Bun 1.3.14 a compiled binary resolves absolute module paths against the new cwd after any process.chdir(), so a --dir invocation could not load the external duckdb addon at all — the same cwd-prefix defect #1201 repairs, not something introduced here. After rebasing onto #1122 the new loadOptionalDriver() does find the addon past a chdir, but on my machine it then fails on @mapbox/node-pre-gyp not resolving out of the Bun store through an absolute-path import, which is again #1122/#1201 territory. So the automated arms drive SQLite (built into Bun, no addon), and the DuckDB evidence above was captured with the addon bundled into the probe. packages/drivers/test/file-store-guard.test.ts covers the DuckDB guard directly, and the guard is engine-independent — it runs before either driver constructs a Database.

Screenshots / recordings

Not a UI change.

Checklist

  • I have tested my changes locally
  • I have not included unrelated changes in this PR

Known limitations, stated rather than hidden

  • The registry is still process-global, filed as #1208. loaded latches on first access, so one server process serving two projects keeps the first project's configs for both, and cached connectors are keyed only by connection name. That predates this PR — the base was simply the launch directory for everyone — and the fix is a state-ownership change across the whole registry rather than a path-resolution change. What this PR guarantees is that whichever project loads resolves against itself rather than against wherever the server was started. #1208 also records two adjacent gaps: Instance.directory is the request directory rather than the worktree root, and run --attach without --dir sends no directory at all.
  • Relative file: URIs keep their existing cwd-dependent behaviour on every platform, filed as #1209. A plain relative path — the reported defect — is fixed everywhere.
  • The existsSync/open TOCTOU gap in assertStoreExists is real but accepted: only a deletion inside that window reproduces the old behaviour, and SQLite additionally opens with create: false as a backstop. The guard targets misconfiguration, not a race.

Summary by CodeRabbit

  • New Features

    • DuckDB and SQLite connections now support optional database creation with create: true; creation is disabled by default.
    • Relative database paths resolve from their configuration directory across project, global, and server configurations.
    • Warehouse additions provide driver-readiness information and post-connection suggestions.

  • Bug Fixes

    • Missing or unspecified database files now fail clearly instead of silently opening an empty database.
    • SQL execution errors are displayed as failures rather than empty results.

[!NOTE]
Medium Risk
Changes default connection semantics for DuckDB/SQLite (no implicit create or :memory: fallback) and registry path resolution, which can break existing configs that relied on cwd-relative paths or silent empty opens; agent-facing SQL error handling is improved but touches a core warehouse path.

Overview
Fixes run --dir and related flows where a populated DuckDB/SQLite warehouse could look empty with no error, because relative paths followed the process cwd and missing files were opened as new empty databases (or missing path fell back to in-memory).

Drivers: New shared file-store guards require an explicit path, refuse to open missing local files unless create: true, and treat remote/scheme paths (e.g. md:, s3://) as non-file. DuckDB and SQLite wire this in; SQLite only creates when not read-only and create is set.

Registry: Relative path values for duckdb/sqlite are absolutized once at config load against the declaring source (global config dir, project root, or env), using Instance.directory when present instead of launch cwd. warehouse_add persists absolute paths for the same reason.

Agent UX: sql_execute now surfaces result.error from sql.execute instead of formatting it as (0 rows). Docs and warehouse_add describe create and path rules. Scratch local DuckDB tools pass create: true. Regression coverage includes unit guards, compiled-binary --dir resolution, and failure-visibility tests.

Reviewed by Cursor Bugbot for commit 44091785085f742be540d8c3839451d051191df0. Bugbot is set up for automated code reviews on this repo. Configure here.