fix: CLI commands ignore runtime DATABASE_BACKEND when both features compiled#209
Conversation
…compiled
`tool auth` and `mcp` CLI subcommands used compile-time `#[cfg]` gates
to select the database backend for secrets storage. When the binary is
compiled with both `postgres` and `libsql` features, the `#[cfg(feature
= "postgres")]` block always wins regardless of the runtime
`DATABASE_BACKEND` setting. This causes `tool auth` and `mcp auth` to
fail with a connection error for users running the libsql backend.
Switch both functions to `match config.database.backend { ... }` with
inner `#[cfg]` guards on each arm, matching the pattern already used in
`main.rs` and `app.rs`.
Also adds a top-level `auth` section to the Telegram channel
capabilities file so `ironclaw tool auth telegram` works for channels
(previously only tools had this section).
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary of ChangesHello @timetwisterfounder-tech, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical bug affecting CLI commands, specifically Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request correctly addresses a bug where CLI commands would ignore the runtime DATABASE_BACKEND setting when both postgres and libsql features were compiled. The change from compile-time #[cfg] blocks to a runtime match on the database backend is the right approach. The addition of the auth section to the Telegram capabilities file is also a good related fix. I've left a couple of suggestions to improve maintainability by reducing code duplication.
| let crypto = SecretsCrypto::new(master_key.clone())?; | ||
|
|
||
| let secrets_store: Arc<dyn SecretsStore + Send + Sync> = { | ||
| #[cfg(feature = "postgres")] | ||
| { | ||
| let store = crate::history::Store::new(&config.database).await?; | ||
| store.run_migrations().await?; | ||
| Arc::new(PostgresSecretsStore::new(store.pool(), Arc::new(crypto))) | ||
| } | ||
| #[cfg(all(feature = "libsql", not(feature = "postgres")))] | ||
| { | ||
| let crypto = Arc::new(crypto); |
There was a problem hiding this comment.
These two lines can be combined into one for conciseness. This also avoids shadowing the crypto variable, which can be a source of confusion.
| let crypto = SecretsCrypto::new(master_key.clone())?; | |
| let secrets_store: Arc<dyn SecretsStore + Send + Sync> = { | |
| #[cfg(feature = "postgres")] | |
| { | |
| let store = crate::history::Store::new(&config.database).await?; | |
| store.run_migrations().await?; | |
| Arc::new(PostgresSecretsStore::new(store.pool(), Arc::new(crypto))) | |
| } | |
| #[cfg(all(feature = "libsql", not(feature = "postgres")))] | |
| { | |
| let crypto = Arc::new(crypto); | |
| let crypto = Arc::new(SecretsCrypto::new(master_key.clone())?); |
serrrfirat
left a comment
There was a problem hiding this comment.
LGTM — Clean, well-scoped bugfix.
The core change (compile-time #[cfg] → runtime match config.database.backend) is correct and matches the established pattern in main.rs:407. The #[allow(unreachable_patterns)] fallback handles the no-features-compiled edge case properly.
The auth section addition to telegram.capabilities.json is the right approach — serde silently ignores it in the channel parser (ChannelCapabilitiesFile has no auth field, no deny_unknown_fields), and the tool auth CLI picks it up via the tool CapabilitiesFile parser.
Minor note (non-blocking): The libSQL init block (resolve path → connect → migrate) is now duplicated in main.rs, mcp.rs, and tool.rs. A shared create_secrets_store(config) -> Arc<dyn SecretsStore> helper would eliminate the duplication, but that's cleanup for another PR.
|
resolve the conflict and lets merge @timetwisterfounder-tech |
|
Thanks for the work on this, @timetwisterfounder-tech! I've picked up your changes and continued them in #740. The new PR includes your original work plus:
You're credited as co-author. Feel free to review the new PR! |
|
Closing in favor of #740 which lands your work on top of latest main. Thanks again for the well-scoped bugfix @timetwisterfounder-tech — the root cause analysis and test plan in the original description were excellent. 🙏 |
Summary
ironclaw tool authandironclaw mcpCLI subcommands used compile-time#[cfg(feature = "postgres")]/#[cfg(all(feature = "libsql", not(feature = "postgres")))]gates to initialize the secrets store. When the binary is compiled with bothpostgresandlibsqlfeatures (the default forcargo install --all-features), the postgres block always wins regardless of the runtimeDATABASE_BACKENDsettingironclaw tool auth <tool>to fail withConnection pool error: Config: configuration property "url" is invalid: invalid connection stringfor all libsql users, making it impossible to configure tool credentials via the CLImatch config.database.backend { ... }with inner#[cfg]guards on each arm, matching the pattern already used inmain.rs(line 385) andapp.rsauthsection to the Telegram channel capabilities file soironclaw tool auth telegram --dir ~/.ironclaw/channels/works (channels were missing this, only tools had it)Root cause
src/cli/tool.rsandsrc/cli/mcp.rsboth had:Fixed to:
Verified
Tested end-to-end on a libsql-backed install:
ironclaw tool auth telegram --dir ~/.ironclaw/channels/— previously crashed with connection pool error, now succeedsironclawstartup —Channel credentials injected channel=telegram credentials_injected=1url_changed=true,status=200(wasurl_changed=false,status=404)Test plan
cargo build --all-featurescompiles cleanlycargo build --no-default-features --features libsqlcompiles cleanlycargo build(default postgres only) compiles cleanlyironclaw tool auth <tool> --dir ~/.ironclaw/channels/works withDATABASE_BACKEND=libsqlironclaw mcpsubcommands work withDATABASE_BACKEND=libsql🤖 Generated with Claude Code