Skip to content

fix(duckdb): unbreak the build, make --engine duckdb reachable, and green up clippy - #12

Open
LifeInTheWeeds wants to merge 3 commits into
mainfrom
fix/duckdb-cli-engine-gate
Open

LifeInTheWeeds wants to merge 3 commits into
mainfrom
fix/duckdb-cli-engine-gate

Conversation

@LifeInTheWeeds

Copy link
Copy Markdown
Collaborator

Two independent DuckDB fixes. The first commit is release-blocking — main does not currently build.

1. main fails to compile against duckdb >= 1.10505.0

Cargo.toml declares duckdb = "1.10504.0", which caret-resolves forward. Upstream marked duckdb::types::Value as #[non_exhaustive] in 1.10505.0, so a fresh cargo build now fails:

error[E0004]: non-exhaustive patterns: `&_` not covered
  --> src/engine/duckdb/mod.rs:758:11
error: could not compile `plenum` (lib) due to 1 previous error

The existing match is complete for every real variant; it only lacks the wildcard the attribute requires. I used the Debug form rather than the compiler-suggested todo!() — this function runs inside a live MCP server, where an unknown future variant should stay visible in the JSON instead of panicking the process.

2. DuckDB was unreachable from the CLI

$ plenum connect --engine duckdb --file db.duckdb
error: invalid value 'duckdb' for '--engine <ENGINE>'
  [possible values: postgres, mysql, sqlite]

Three value_parser arrays hardcoded the engine list (src/main.rs:72, :156, :283). clap rejects the value before parse_engine runs, so every downstream layer was dead code on this path:

Layer State before this PR
parse_engine() "duckdb" => Ok(DatabaseType::DuckDB) — already correct
connection builder ConnectionConfig::duckdb(file) — already correct, including its --file is required for duckdb message, which could never fire
validate_connection / introspect / execute all already dispatch DuckDbEngine
is_read_only_duckdb() already implemented, REF-41 + REF-42

Notably the interactive path never had this gate — engine_choices at main.rs:1032 already lists "duckdb". The two entry points disagreed about which engines exist.

Verification

connect --test  -> {"ok":true,"engine":"duckdb","database_version":"v1.5.5"}
--list-tables   -> 7 tables returned

Read-only enforcement re-checked at runtime — enabling the engine does not widen the write surface:

Query Result
SELECT count(*) FROM macro_series allowed
DELETE FROM macro_series CAPABILITY_VIOLATION
WITH x AS (DELETE ... RETURNING *) SELECT * FROM x CAPABILITY_VIOLATION
SELECT * INTO evil FROM macro_series CAPABILITY_VIOLATION
COPY (SELECT 1) TO '...csv' CAPABILITY_VIOLATION
INSTALL httpfs CAPABILITY_VIOLATION
ATTACH '...' AS z CAPABILITY_VIOLATION

Test suite: 414 pass. The 4 *_schema_not_stale failures are pre-existing and unrelated — see the note below.

Two follow-ups, not included here

Root cause worth addressing. The engine list is declared in six places: three value_parser arrays, the interactive engine_choices vec, the parse_engine match, and parse_engine's error string. That is why one drifted unnoticed. #[derive(clap::ValueEnum)] on DatabaseType would collapse all six and make this class of bug structurally impossible.

Windows checkouts are red. schemas/*.json are checked out CRLF under core.autocrlf=true while generate-schemas emits LF, so all 4 schema_drift tests fail on any Windows clone. There is no .gitattributes. One line fixes it:

schemas/*.json text eol=lf

I can send either as a separate PR if useful.

🤖 Generated with Claude Code

https://claude.ai/code/session_016pWGjWtzGf4UvtgiWqYQyR

LifeInTheWeeds and others added 2 commits September 22, 2026 09:38
duckdb 1.10505.0 marked `types::Value` as #[non_exhaustive]. Cargo.toml
declares `duckdb = "1.10504.0"`, which caret-resolves forward, so a fresh
`cargo build` now fails on the default feature set:

    error[E0004]: non-exhaustive patterns: `&_` not covered
      --> src/engine/duckdb/mod.rs:758:11
    error: could not compile `plenum` (lib) due to 1 previous error

The existing match is complete for every real variant; it only lacks the
wildcard the attribute now requires.

Renders the Debug form rather than the compiler-suggested `todo!()`. This
function runs inside a live MCP server, where an unknown future variant
should stay visible in the JSON rather than panicking the process or being
silently nulled.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016pWGjWtzGf4UvtgiWqYQyR
The DuckDB engine was fully implemented but unreachable from the CLI.
Three clap `value_parser` arrays hardcoded the engine list:

    src/main.rs:72   connect
    src/main.rs:156  introspect
    src/main.rs:283  query

    #[arg(long, value_parser = ["postgres", "mysql", "sqlite"])]

clap rejects the value before `parse_engine` is ever called, so every
downstream layer was dead code on this path:

  - parse_engine():            "duckdb" => Ok(DatabaseType::DuckDB)
  - connection builder:        DatabaseType::DuckDB => ConnectionConfig::duckdb(file)
                               (the "--file is required for duckdb" message
                               could never fire)
  - validate_connection / introspect / execute: all dispatch DuckDbEngine
  - is_read_only_duckdb():     already implemented, REF-41 + REF-42

The interactive path did not have this gate -- `engine_choices` at
main.rs:1032 already lists "duckdb" -- so the two entry points disagreed
about which engines exist.

Verified after the change, against a DuckDB file:

    connect --test  -> {"ok":true,"engine":"duckdb","database_version":"v1.5.5"}
    --list-tables   -> 7 tables returned

Read-only enforcement re-checked at runtime; enabling the engine does not
widen the write surface. SELECT allowed; DELETE, CTE-hidden DELETE ...
RETURNING, SELECT ... INTO, COPY ... TO <file>, INSTALL and ATTACH all
rejected with CAPABILITY_VIOLATION.

Note for follow-up: the engine list is declared in six places (three
value_parser arrays, the interactive `engine_choices` vec, the
`parse_engine` match, and `parse_engine`'s error string). Deriving
clap::ValueEnum on DatabaseType would collapse all six and make this
class of drift impossible.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016pWGjWtzGf4UvtgiWqYQyR
@LifeInTheWeeds

Copy link
Copy Markdown
Collaborator Author

On the red Clippy check — it isn't from this PR.

The 6 failures are unused_async at sqlite/mod.rs:35,77,152 and duckdb/mod.rs:43,63,100. My only change to duckdb/mod.rs here is at line 813, and I don't touch sqlite/mod.rs at all. Clippy passed on main in July against that same code — a newer stable clippy started firing the lint, and CI's -D warnings promotes it to an error.

#14 fixes it in isolation.

Everything else here is green, including Test (ubuntu-latest, stable) and Live DB integration tests.

Suggested order: #12 → #14 → #13. This one unbreaks the build, which the other two need before their CI can even reach the relevant checks. Happy to rebase the others once this lands.

LifeInTheWeeds added a commit that referenced this pull request Sep 22, 2026
CI runs `cargo clippy --all-targets --all-features -- -D warnings` against the
`stable` toolchain with nothing pinning it. A newer stable clippy began firing
`unused_async` on async trait-impl methods, producing 6 errors:

    src/engine/sqlite/mod.rs:35, 77, 152
    src/engine/duckdb/mod.rs:43, 63, 100

That is exactly the 3 `DatabaseEngine` methods (`validate_connection`,
`introspect`, `execute`) in the 2 engines whose drivers are synchronous. The
`postgres` and `mysql` impls are unaffected because they genuinely `.await`.

The `async` cannot be dropped: `DatabaseEngine` declares these methods as
returning futures, so every impl must match that signature regardless of
whether its body awaits. Satisfying the lint would mean hand-rolling
`impl Future` bodies purely to appease it, which is strictly worse code.

`#[allow(clippy::unused_async)]` on the two impl blocks, with a comment saying
why. This matches existing house style -- the tree already carries
`allow(clippy::future_not_send)`, `allow(clippy::too_many_arguments)` and
several others.

Note this is drift, not regression: clippy passed on main in July against this
same code. Pinning the clippy toolchain in CI would address the cause rather
than the symptom, but that is a maintainer call and out of scope here.

Ordering: main does not currently compile against duckdb >= 1.10505.0, so
clippy aborts on that error before it reaches these lints. This branch
therefore cannot go green until the `non_exhaustive` wildcard fix (first commit
of #12) lands. It is branched from main to stay single-purpose.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016pWGjWtzGf4UvtgiWqYQyR
@LifeInTheWeeds

Copy link
Copy Markdown
Collaborator Author

Update: folded the clippy fix in here so this PR can prove itself on CI.

I originally split the unused_async fix into #14 to keep each PR single-purpose. That was the wrong call in practice: main doesn't compile, so clippy aborts on E0004 before reaching those lints — which meant #14 could never go green on its own, and this PR stayed red on a failure it didn't cause. Three red PRs and a "trust me, the red is pre-existing" is a worse ask than one green check.

So this branch is now three commits:

  1. 99eeb20 — non_exhaustive wildcard: unbreaks the build
  2. 3aea095 — --engine duckdb reachable from the CLI
  3. d8aee71 — allow(clippy::unused_async) on the two synchronous engine impls (was fix(clippy): allow unused_async on the synchronous engine impls #14)

Each is independently revertible, and #3 is 12 lines of attribute + comment touching no logic.

#14 is now redundant and I'll close it once CI here is green. #13 (the MCP -32601 / ping spec fix) still needs to land after this one, and I'll rebase it then.

On commit 3: clippy passed on main in July against identical code. A newer stable clippy began firing unused_async on async trait-impl methods, and CI's -D warnings promotes it to an error. It hits exactly the 3 DatabaseEngine methods in the 2 engines with synchronous drivers — postgres and mysql are unaffected because they genuinely .await. The async can't be dropped, since the trait mandates the signature. Pinning the clippy toolchain in CI would fix the cause rather than the symptom, but that's your call and I left it out of scope.

@LifeInTheWeeds LifeInTheWeeds changed the title fix(duckdb): unbreak the build, and make --engine duckdb reachable from the CLI fix(duckdb): unbreak the build, make --engine duckdb reachable, and green up clippy Sep 22, 2026
@LifeInTheWeeds
LifeInTheWeeds force-pushed the fix/duckdb-cli-engine-gate branch from d8aee71 to c6ad88a Compare September 22, 2026 16:26
…impls

CI runs `cargo clippy --all-targets --all-features -- -D warnings` against
`dtolnay/rust-toolchain@stable`, with nothing pinning the version. Clippy 0.1.98
(Rust 1.98, 2026-09-01) split `unused_async_trait_impl` out of `unused_async` as
a SEPARATE lint, and it now fires 6 times:

    src/engine/sqlite/mod.rs -- validate_connection, introspect, execute
    src/engine/duckdb/mod.rs -- validate_connection, introspect, execute

That is exactly the 3 `DatabaseEngine` methods in the 2 engines whose drivers
are synchronous. `postgres` and `mysql` are unaffected because they genuinely
`.await`.

The `async` cannot be dropped: `DatabaseEngine` declares these methods as
returning futures, so every impl must match that signature regardless of whether
its body awaits. Satisfying the lint would mean hand-rolling `impl Future`
bodies purely to appease it, which is strictly worse code.

⚠ The lint name matters, and the error text is the clue: `#[allow(clippy::unused_async)]`
does NOT cover it, because since 0.1.98 the trait-impl case is its own lint. Both
names are listed so the attribute holds either side of that split.

This is drift, not regression: clippy passed on main in July against this same
code. Nothing in the source changed; the toolchain moved underneath it.

⭐ The durable fix is to pin the clippy toolchain in `.github/workflows/ci.yml`
rather than tracking `stable`. As written, any new upstream lint can turn main
red with no repo change -- which is exactly what happened here. That is a
maintainer call, so it is left out of this PR.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016pWGjWtzGf4UvtgiWqYQyR
@LifeInTheWeeds
LifeInTheWeeds force-pushed the fix/duckdb-cli-engine-gate branch from c6ad88a to 8407dbe Compare September 22, 2026 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant