Add database configuration handling#8
Merged
Bechma merged 3 commits intocyberfabric:mainfrom Mar 5, 2026
Merged
Conversation
…d remove commands. Also, global database. Signed-off-by: Bechma <19294519+Bechma@users.noreply.github.com>
Contributor
📝 WalkthroughWalkthroughAdds typed global/module DB and tracing config, CLI subcommands to manage global and per-module DB entries, deterministic module metadata merging with precedence rules, helpers for resolving workspace + config (DEFAULT_CONFIG_FILE and resolve_workspace_and_config), and integrates upsert behavior into module add. Changes
Sequence Diagram(s)sequenceDiagram
participant User as CLI User
participant CLI as cf-cli
participant Config as AppConfig Loader
participant FS as Filesystem
participant Parser as module_parser
User->>CLI: invoke command (e.g., `modules add` / `config db add`)
CLI->>Config: load_config(config_path)
Config->>FS: read config file
FS-->>Config: config content
Config->>Parser: parse module metadata (if applicable)
Parser-->>CLI: return ConfigModuleMetadata(s)
CLI->>CLI: merge_module_metadata(config_meta, local_meta)
CLI->>Config: mutate AppConfig (upsert/patch/remove)
Config->>FS: write tmp file and atomic replace
FS-->>CLI: write success
CLI-->>User: return status/message
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
crates/cli/src/config/app_config.rs (1)
337-354: Consider trimming whitespace in key-value parsing.If a user provides
--params "key = value", the key will be stored as"key "and value as" value". Trimming would improve robustness:♻️ Suggested fix
let (key, value) = pair .split_once('=') .ok_or_else(|| format!("invalid key=value pair '{pair}'"))?; - if key.trim().is_empty() { + let key = key.trim(); + let value = value.trim(); + if key.is_empty() { return Err(format!("invalid key=value pair '{pair}'")); } - params.insert(key.to_owned(), value.to_owned()); + params.insert(key.to_owned(), value.to_owned());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/config/app_config.rs` around lines 337 - 354, The parse_params_map function currently stores raw key and value without trimming, so inputs like "key = value" keep surrounding spaces; update parse_params_map to trim whitespace on both key and value (e.g., call .trim() on key and value before validating/inserting), validate that the trimmed key is non-empty (use the trimmed key for the empty check and the map insert), and insert the owned trimmed strings into the BTreeMap so stored keys/values have no leading/trailing spaces.crates/cli/src/config/db/mod.rs (2)
153-161: Minor duplication withvalidate_module_name.This function has the same logic as
validate_module_nameinmodules/mod.rs. Consider extracting to a shared helper parameterized by the entity kind.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/config/db/mod.rs` around lines 153 - 161, The validation logic in validate_identifier duplicates validate_module_name; extract the shared logic into a single helper (e.g., validate_name or validate_identifier_common) that takes (value: &str, kind: &str) and performs the ASCII-alnum/'-'/ '_' check and error formatting, replace the body of validate_identifier and modify validate_module_name to call that helper, and update any callers to use the unified helper to avoid duplication while preserving the exact error message format.
135-151: Duplicateload_configandsave_configimplementations.These functions are identical to those in
crates/cli/src/config/modules/mod.rs(lines 125-152). Consider extracting them to a shared location (e.g., autilsmodule or the parentconfigmodule) to avoid code duplication.♻️ Suggested structure
Move to
crates/cli/src/config/mod.rsor a newcrates/cli/src/config/utils.rs:pub(crate) fn load_config(path: &Path) -> anyhow::Result<AppConfig> { ... } pub(crate) fn save_config(path: &Path, config: &AppConfig) -> anyhow::Result<()> { ... }Then import in both
db/mod.rsandmodules/mod.rs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/config/db/mod.rs` around lines 135 - 151, The load_config and save_config functions are duplicated; extract them into a shared function in the parent config module (e.g., config::load_config and config::save_config or config::utils with pub(crate) visibility) and remove the duplicates in db::mod.rs and modules::mod.rs; update both modules to call the shared functions and ensure signatures remain fn load_config(path: &Path) -> anyhow::Result<AppConfig> and fn save_config(path: &Path, config: &AppConfig) -> anyhow::Result<()> so existing callers (including fs/serde_saphyr usage and the temp-file/rename logic) continue to work.crates/cli/src/config/modules/db.rs (1)
126-131: Duplicateensure_conn_payloadimplementation.This function is identical to the one in
crates/cli/src/config/db/mod.rs(lines 128-133). Consider extracting to a shared location.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/config/modules/db.rs` around lines 126 - 131, The function ensure_conn_payload is duplicated; extract it into a single shared utility module (e.g., a new config/utils or config/common module) and have both crates/cli/src/config/modules/db.rs and crates/cli/src/config/db/mod.rs import and call that one implementation instead of defining their own copies. Move the implementation of ensure_conn_payload to the new shared module, export it (pub fn ensure_conn_payload), update both files to use the shared path (use ...::ensure_conn_payload), remove the duplicate function definitions, and run cargo build/tests to verify no symbol/visibility issues remain.crates/cli/src/config/modules/add.rs (1)
134-241: Consider adding a test for upsert behavior.The tests cover
build_required_metadatawell, but there's no test validating the upsert behavior when a module already exists in the config. Adding such a test would document the expected behavior (full replace vs. merge) and prevent regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/config/modules/add.rs` around lines 134 - 241, Add a unit test in the existing tests module that verifies upsert behavior when adding a module that already exists: create an initial ConfigModule (using ConfigModule and ConfigModuleMetadata from the diff) in a fake/in-memory config, construct AddArgs for the same module name (using AddArgs from the diff) with different metadata, run the code-path that performs the add/upsert (the CLI add handler or the function that applies build_required_metadata results into the config), and assert the final config entry matches the expected upsert semantics (full replace or merge) so future changes won't regress the intended behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/cli/src/config/modules/add.rs`:
- Around line 43-53: The upsert currently overwrites an existing
ModuleConfig.metadata (in the block that checks
config.modules.get_mut(&self.module)), which discards prior custom fields;
instead merge the incoming metadata with the existing one—retrieve
existing.metadata (if Some) and combine fields with the new metadata rather than
replacing, or invoke the same merge logic used by get_config (e.g.,
merge_module_metadata) to preserve existing features/default_features and only
overwrite CLI-provided fields; update the branch that inserts/updates
ModuleConfig (and ModuleConfig::default() fallback) to perform this merge so
existing customizations are kept.
In `@crates/cli/src/config/modules/db.rs`:
- Around line 48-56: The current code uses
config.modules.entry(self.module.clone()).or_default(), which silently creates a
module when adding DB config; change this to first check for existence with
something like if !config.modules.contains_key(&self.module) { return Err(...) }
unless a new boolean flag (e.g. self.create / --create) is provided and true; if
create is true then insert a default module entry and proceed to set or patch
module_cfg.database (keep existing apply_patch logic), and update any
command/struct parsing to accept the new --create flag so callers can opt into
creating the module.
---
Nitpick comments:
In `@crates/cli/src/config/app_config.rs`:
- Around line 337-354: The parse_params_map function currently stores raw key
and value without trimming, so inputs like "key = value" keep surrounding
spaces; update parse_params_map to trim whitespace on both key and value (e.g.,
call .trim() on key and value before validating/inserting), validate that the
trimmed key is non-empty (use the trimmed key for the empty check and the map
insert), and insert the owned trimmed strings into the BTreeMap so stored
keys/values have no leading/trailing spaces.
In `@crates/cli/src/config/db/mod.rs`:
- Around line 153-161: The validation logic in validate_identifier duplicates
validate_module_name; extract the shared logic into a single helper (e.g.,
validate_name or validate_identifier_common) that takes (value: &str, kind:
&str) and performs the ASCII-alnum/'-'/ '_' check and error formatting, replace
the body of validate_identifier and modify validate_module_name to call that
helper, and update any callers to use the unified helper to avoid duplication
while preserving the exact error message format.
- Around line 135-151: The load_config and save_config functions are duplicated;
extract them into a shared function in the parent config module (e.g.,
config::load_config and config::save_config or config::utils with pub(crate)
visibility) and remove the duplicates in db::mod.rs and modules::mod.rs; update
both modules to call the shared functions and ensure signatures remain fn
load_config(path: &Path) -> anyhow::Result<AppConfig> and fn save_config(path:
&Path, config: &AppConfig) -> anyhow::Result<()> so existing callers (including
fs/serde_saphyr usage and the temp-file/rename logic) continue to work.
In `@crates/cli/src/config/modules/add.rs`:
- Around line 134-241: Add a unit test in the existing tests module that
verifies upsert behavior when adding a module that already exists: create an
initial ConfigModule (using ConfigModule and ConfigModuleMetadata from the diff)
in a fake/in-memory config, construct AddArgs for the same module name (using
AddArgs from the diff) with different metadata, run the code-path that performs
the add/upsert (the CLI add handler or the function that applies
build_required_metadata results into the config), and assert the final config
entry matches the expected upsert semantics (full replace or merge) so future
changes won't regress the intended behavior.
In `@crates/cli/src/config/modules/db.rs`:
- Around line 126-131: The function ensure_conn_payload is duplicated; extract
it into a single shared utility module (e.g., a new config/utils or
config/common module) and have both crates/cli/src/config/modules/db.rs and
crates/cli/src/config/db/mod.rs import and call that one implementation instead
of defining their own copies. Move the implementation of ensure_conn_payload to
the new shared module, export it (pub fn ensure_conn_payload), update both files
to use the shared path (use ...::ensure_conn_payload), remove the duplicate
function definitions, and run cargo build/tests to verify no symbol/visibility
issues remain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f695777f-1cb9-4719-95ed-f830cbbb934e
📒 Files selected for processing (7)
crates/cli/src/common.rscrates/cli/src/config/app_config.rscrates/cli/src/config/db/mod.rscrates/cli/src/config/mod.rscrates/cli/src/config/modules/add.rscrates/cli/src/config/modules/db.rscrates/cli/src/config/modules/mod.rs
Signed-off-by: Bechma <19294519+Bechma@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/cli/src/config/modules/db.rs`:
- Around line 51-56: The error message references a nonexistent --create flag;
either remove that mention or implement the flag: add a boolean field named
create to AddArgs (or the relevant CLI args struct), wire it into the CLI
parser/derive so it appears in help, and update the calling logic that handles
the add command to respect this flag when a module is missing (the code path
checking config.modules and calling bail(...) should consult AddArgs::create to
create the module instead of bailing). Also update the message in the bail call
(or help text) to reflect the chosen approach and ensure context.config_path and
self.module remain used for clear feedback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cfa18e42-463f-4e49-85fe-3f7ef001add5
📒 Files selected for processing (10)
crates/cli/src/build/mod.rscrates/cli/src/common.rscrates/cli/src/config/app_config.rscrates/cli/src/config/db/mod.rscrates/cli/src/config/mod.rscrates/cli/src/config/modules/add.rscrates/cli/src/config/modules/db.rscrates/cli/src/config/modules/mod.rscrates/cli/src/main.rscrates/cli/src/run/mod.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/cli/src/config/app_config.rs
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/cli/src/config/modules/db.rs (1)
45-65: Consider extracting shared Add/Edit mutation flow.
AddArgs::runandEditArgs::runrepeat context resolution, config load, module fetch, patching, and save paths. A small shared helper would reduce future drift and keep behavior aligned.Also applies to: 78-94
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/config/modules/db.rs` around lines 45 - 65, Extract the shared mutation flow from AddArgs::run and EditArgs::run into a small helper (e.g., apply_module_db_change) that takes the module name, the conn payload, and path_config/context and encapsulates: validate_module_db_payload, resolve_modules_context, load_config, get_module_cfg_mut, the existing.apply_patch or set logic for module_cfg.database, and save_config; then call that helper from both AddArgs::run and EditArgs::run to avoid duplicated resolution/patch/save logic and keep behavior aligned.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/cli/src/config/modules/db.rs`:
- Around line 45-65: Extract the shared mutation flow from AddArgs::run and
EditArgs::run into a small helper (e.g., apply_module_db_change) that takes the
module name, the conn payload, and path_config/context and encapsulates:
validate_module_db_payload, resolve_modules_context, load_config,
get_module_cfg_mut, the existing.apply_patch or set logic for
module_cfg.database, and save_config; then call that helper from both
AddArgs::run and EditArgs::run to avoid duplicated resolution/patch/save logic
and keep behavior aligned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 177eb5af-fa04-4a32-82c9-9071b2c17a7a
📒 Files selected for processing (1)
crates/cli/src/config/modules/db.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add module-level database configuration management with add, edit, and remove commands. Also, global database.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor