fix(drivers): recover #1204 review debt — driver-e2e false-skip, file: URI checks, Windows paths, telemetry, docs

Merged
AN
2mo ago
+556-46

Issue for this PR

Follow-up to #1204 (no dedicated tracking issue — this is review-debt cleanup, not a new bug report).

Type of change

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

What does this PR do?

#1204 (fix/warehouse-store-path-resolution, merged as 1caa234ff9) landed its core path-resolution fix but was merged with only 2 of 47 review threads addressed. This PR pays down that review debt: it recovers two verified fixes that were left uncommitted, finishes an incomplete P1, and fixes the remaining live threads that had concrete, testable fixes.

1. drivers-e2e.test.ts — a vacuous-green test suite (the priority fix).
Three bare connect({ type: "duckdb" }) calls (no path) threw under #1204's new requireStorePath() guard. probeDuckDB() catches everything and returns false on any failure, so duckdbAvailable stayed permanently false — 39 of the file's 50 tests silently skipped, even with the duckdb native binding installed and working. Fixed by passing path: ":memory:" to the three bare calls.
Verified directly, not assumed: bun test test/altimate/drivers-e2e.test.ts — 11 pass / 39 skip → 36 pass / 14 skip.

2. file-store.ts isLocalFilePath — scheme misread.
The scheme-exclusion regex matched any "2+ letter prefix + colon", so a local filename shaped like data:warehouse.duckdb was misread as a remote scheme and silently skipped both path resolution and the existence guard. Narrowed to real scheme:// URIs plus the specific non-slash DuckDB extension schemes (md:, motherduck:, ducklake:). Added a regression unit test with a data:-shaped local filename.

3. SQLite/DuckDB missing-absolute-file:-path P1 — completed.
The orphaned work had written an absoluteFileUriPath() helper but never wired it into assertStoreExists, so behavior was unchanged on main — a missing absolute file: URI store still opened silently empty, the exact bug class #1204 exists to fix. Wired it in: an absolute file: URI is now existence-checked and fails loudly when missing. Relative file: URIs are deliberately left untouched (already tracked separately as #1209 — resolving them safely across platforms is a larger, unverified change). Added tests for both the missing and present absolute case, and confirmed the relative case is unaffected.

4. registry.ts — Windows-absolute paths (registry.ts:121).
POSIX path.isAbsolute() doesn't recognize C:\..., so a shared/migrated config's Windows path would get mangled by path.resolve(baseDir, ...). Fixed with path.isAbsolute(p) || path.win32.isAbsolute(p). By-inspection only — no Windows CI in this repo, not runtime-verified.

5. warehouse-add.ts — description/behavior mismatch.
The 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 the fact it always persists into the global config file. Description corrected to describe actual behavior.

6. sql-execute.ts — telemetry bias. (see "Round 3" below for where this ended up)
SQL fingerprint telemetry was originally emitted only on the success path. A failed execution never got fingerprinted, biasing sql_fingerprint telemetry away from failures.

7. docs/configure/warehouses.md — stale docs.
Both the DuckDB and SQLite sections still said path was optional/omittable for an in-memory store. requireStorePath() now rejects a missing path outright. Corrected in both sections.

8. file-store.ts:89 directory-vs-file check.
fs.existsSync() is also true for a directory at that path, which would let a misconfigured directory path through to a confusing driver-level open error instead of this guard's clear one. Added an existsAsFile() check (exists AND not a directory).

TOCTOU threads on #1204 are addressed by reply-with-reasoning on the original PR (narrow, non-regressing limitation — not redesigned here; see below).

Explicitly out of scope: the registry cross-tenant-scoping P1 (registry.ts:80/:175 — the process-global loaded state serving two projects from one server process) is tracked separately as #1237 and needs its own design PR. Not attempted here.

On #1204 itself, I'm replying to and resolving the threads this PR addresses, the ~10 threads that are outdated (filed against a parseRelativeFileUri block removed before merge), and the one not-reproducible thread (sqlite.ts:30 memory-URI create-flag — empirically opens fine with create:false on bun@1.3.14), pointing the two registry P1 threads at #1237.

Round 2 — this PR's own first review round (8 threads, all resolved)

Pushing the fixes above triggered automated review on the new code itself. Fixed the real bugs (with tests): create: true bypassing the directory check, an absolute file: URI with 4+ leading slashes bypassing the existence guard, a doubled-slash Windows drive path (C://data/...) misclassified as remote, and the post-probe DuckDB setup failure silently reporting a pass instead of a failure. One genuine design call (a custom DuckDB storage extension using an unlisted bare scheme: form is no longer forwarded, since narrowing the bare-scheme exclusion to a closed list is in direct tension with the local-filename fix above) — documented in a code comment on NON_SLASH_REMOTE_SCHEMES in file-store.ts and in the driver docs, decision: accept the closed list.

Round 3 — de-scoping the telemetry change (7 threads, all resolved)

Fixing round 2 triggered a third review round, entirely on the telemetry fingerprinting fix from item 6 above and its test:

  • 3 real bugs, fixed with tests: (1) the DuckDB retry loop dropped a connector without close() when connect() failed after the native handle had already opened — a leak per failed retry, now closed via a connectOrClose() helper; (2) connectWithRetry's final throw discarded the original error's stack/type/cause behind a generic message — now thrown with { cause: lastError }; (3) the fingerprint emission ran after formatResult(), so a genuinely-executed query went uncounted if formatting itself threw — moved before formatting.
  • The telemetry change itself, de-scoped. All 4 remaining threads converged on one root problem: sql.execute returns the same result-shaped { ..., error } object 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). There is no way to tell those apart from the sql_execute tool alone, so fingerprinting the result-error branch risked mislabeling never-executed queries as executed SQL — the opposite of what the original comment in item 6 asked for. After three rounds trying to build that distinction from the caller's side, reverted to fingerprint-on-success-only (the behavior that predates #1204) — the smaller, clearly-correct change, rather than building a failed-execution-vs-never-executed taxonomy inside this cleanup PR. The real fix (an explicit executed-phase signal from the execution path itself) is filed as #1242, referenced from the code comment at the de-scoped branch and from the updated test.

How did you verify your code works?

  • bun test packages/drivers/ — 323 pass, 0 fail (12 files).
  • bun test packages/opencode/test/altimate/ — 5013 pass, 653 skip, 0 fail (179 files) — includes the corrected drivers-e2e.test.ts (up from 11 pass / 39 skip on unmodified main; independently re-confirmed by stashing just that file's edit and re-running).
  • bun turbo typecheck --force — 13/13 successful, 0 cached (forced re-verification on the final round).
  • bun run script/upstream/analyze.ts --markers --base origin/main --strict — clean; no upstream-shared files require markers (none flagged).
  • bunx prettier --check — clean on every file this PR newly writes or substantially edits (file-store.ts, file-store-guard.test.ts, sql-execute.ts, warehouse-add.ts). Three files (drivers-e2e.test.ts, registry.ts, telemetry-signals.test.ts) were already unformatted on origin/main before this PR touched them (verified by checking the unmodified file in place) — left as-is rather than reformatted as noise, matching #1204's own precedent for pre-existing drift.
  • Windows path handling (registry.ts:121) is by-inspection only — this repo has no Windows CI, so it is not runtime-verified, stated explicitly rather than implied as tested.
  • All other fixes have accompanying unit tests exercising the corrected behavior (see packages/drivers/test/file-store-guard.test.ts and packages/opencode/test/altimate/telemetry-signals.test.ts).

Screenshots / recordings

Not a UI change.

Checklist

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

[!NOTE]
Medium Risk
Changes how DuckDB/SQLite connection paths are classified and validated at connect time, which can surface new errors for edge-case paths but reduces silent empty-warehouse failures; Windows path handling in registry is inspection-only without Windows CI.

Overview
Follow-up to warehouse store-path work (#1204): file-backed DuckDB/SQLite paths are validated more strictly so misconfigurations fail loudly instead of opening empty databases.

Drivers (file-store.ts) narrows what counts as “remote” vs local: only scheme:// URIs and a fixed list of bare DuckDB schemes (md:, motherduck:, ducklake:) skip the existence guard—colon-containing local names like data:warehouse.duckdb and odd Windows paths like C://data/... are treated as local again. Absolute file: URIs get existence (and directory) checks via new absoluteFileUriPath; directory paths error clearly even when create: true.

Registry leaves Windows-absolute path values untouched when resolving configs on macOS/Linux (path.win32.isAbsolute). Docs and warehouse_add text now say path is required (use :memory: explicitly) and document bare-word:target vs remote rules; add-time relative paths are described as resolved against the project root.

sql_execute keeps SQL fingerprint telemetry success-only (before formatResult()), with comments pointing at #1242 for failed-vs-never-executed distinction.

Tests: expanded file-store-guard coverage; DuckDB E2E uses path: ":memory:", retries setup with connectWithRetry / connectOrClose, and throws on persistent setup failure instead of vacuous passes.

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

Summary by cubic

Completes the path-handling follow-up to #1204 across drivers, config resolution, telemetry, tests, and docs. Missing DuckDB/SQLite stores and directory paths now fail clearly instead of opening empty databases, and the DuckDB E2E suite no longer silently skips.

Bug Fixes

  • Keeps colon-containing local filenames such as data:warehouse.duckdb local while still excluding real URI and DuckDB extension schemes; doubled-slash Windows paths like C://data/warehouse.duckdb stay local too.
  • Absolute file: URIs are existence-checked, including UNC-style 4+ slash forms, while relative file: URIs stay untouched; directory paths are rejected even with create: true.
  • Preserves Windows-absolute paths when resolving configs on POSIX; verified by inspection because the repo has no Windows CI.
  • Records SQL fingerprints on success only — the result-error branch can't distinguish a failed query from one that never ran, so it stays unfingerprinted (tracked as #1242).
  • Restores DuckDB E2E coverage by passing :memory: to in-memory connections; post-probe setup failures now fail the suite, preserve the original error, and close half-open connectors so retries don't leak native handles.

Docs and Validation

  • Updates DuckDB and SQLite documentation and the warehouse_add description to match required paths and project-root resolution; docs and the file-store.ts scheme comment state the bare-scheme rule as local-by-default with md:, motherduck:, and ducklake: as the only bare exceptions.
  • Driver and Altimate tests, typechecking, marker analysis, and formatting checks pass.

Summary by CodeRabbit

  • Bug Fixes

    • Improved handling of local filenames containing colons, absolute file: URIs, and Windows-style paths.
    • Existing directories now fail with clear validation errors, including when creation is enabled.
    • Relative warehouse paths are resolved against the project root and saved as absolute paths.
    • Windows-style absolute paths remain intact across configuration environments.

  • Configuration & Documentation

    • DuckDB and SQLite configurations require an explicit path; use :memory: for in-memory databases.
    • Clarified how local and remote storage paths are interpreted.

  • Telemetry

    • SQL fingerprinting is recorded only after successful query execution.