Adopt new ortho_config merge API - #147
Conversation
Reviewer's GuideThis PR upgrades the CLI to leverage the new SubcmdConfigMerge API by replacing standalone merge helper calls with the load_and_merge method on subcommands, centralizes environment variable handling behind a synchronized environment module, and refactors the test suite to use newly introduced support helpers and restructure merge tests for better coverage. Sequence diagram for CLI subcommand config merging with new APIsequenceDiagram
participant Main
participant PrCli
participant SubcmdConfigMerge
Main->>PrCli: call load_and_merge()
PrCli->>SubcmdConfigMerge: load_and_merge()
SubcmdConfigMerge-->>PrCli: merged config
PrCli-->>Main: merged config
Sequence diagram for synchronized environment variable accesssequenceDiagram
participant Caller
participant environment
participant ENV_LOCK
Caller->>environment: set_var(key, value)
environment->>ENV_LOCK: acquire lock
ENV_LOCK-->>environment: lock acquired
environment->>std::env: set_var(key, value)
environment-->>Caller: done
Class diagram for SubcmdConfigMerge API adoptionclassDiagram
class SubcmdConfigMerge {
+load_and_merge()
}
class PrCli {
+load_and_merge()
}
class IssueCli {
+load_and_merge()
}
class ResolveCli {
+load_and_merge()
}
PrCli --|> SubcmdConfigMerge : implements
IssueCli --|> SubcmdConfigMerge : implements
ResolveCli --|> SubcmdConfigMerge : implements
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Reviews pausedUse the following commands to manage reviews:
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Summary by CodeRabbit
WalkthroughReplace the free-function subcommand config merge with an inherent load_and_merge method on CLI arg types. Update src/main.rs to import SubcmdConfigMerge and call load_and_merge() for PrArgs, IssueArgs and ResolveArgs. Add and refactor tests and support utilities to validate CLI/env/config precedence, including Resolve. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant CLI as CLI Parser
participant Args as <cmd>_cli (Args)
participant Cfg as Config Loader
participant Env as Environment
participant Run as Runner
U->>CLI: Invoke subcommand (pr / issue / resolve)
CLI->>Args: Construct CLI args struct
Args->>Args: call load_and_merge()
par Read sources
Args->>Cfg: Load .vk.toml and config sections
Args->>Env: Read environment variables
end
Args-->>CLI: Return merged args
CLI->>Run: run_<cmd>(merged args)
Run-->>U: Produce output
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Comment |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/main.rs:438` </location>
<code_context>
let result = match cli.command {
Commands::Pr(pr_cli) => {
- let args = load_and_merge_subcommand_for(&pr_cli)?;
+ let args = pr_cli.load_and_merge()?;
run_pr(args, &global).await
}
</code_context>
<issue_to_address>
**issue (review_instructions):** Add behavioural and unit tests for the new load_and_merge method on pr_cli.
You replaced load_and_merge_subcommand_for with pr_cli.load_and_merge, which is a change to an existing feature. Ensure that both behavioural and unit tests cover the new method's functionality and edge cases.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural *and* unit tests are required.
</details>
</issue_to_address>
### Comment 2
<location> `src/main.rs:442` </location>
<code_context>
}
Commands::Issue(issue_cli) => {
- let args = load_and_merge_subcommand_for(&issue_cli)?;
+ let args = issue_cli.load_and_merge()?;
run_issue(args, &global).await
}
</code_context>
<issue_to_address>
**issue (review_instructions):** Add behavioural and unit tests for the new load_and_merge method on issue_cli.
You replaced load_and_merge_subcommand_for with issue_cli.load_and_merge, which is a change to an existing feature. Ensure that both behavioural and unit tests cover the new method's functionality and edge cases.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural *and* unit tests are required.
</details>
</issue_to_address>
### Comment 3
<location> `src/main.rs:446` </location>
<code_context>
}
Commands::Resolve(resolve_cli) => {
- let args = load_and_merge_subcommand_for(&resolve_cli)?;
+ let args = resolve_cli.load_and_merge()?;
run_resolve(args, &global).await
}
</code_context>
<issue_to_address>
**issue (review_instructions):** Add behavioural and unit tests for the new load_and_merge method on resolve_cli.
You replaced load_and_merge_subcommand_for with resolve_cli.load_and_merge, which is a change to an existing feature. Ensure that both behavioural and unit tests cover the new method's functionality and edge cases.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural *and* unit tests are required.
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/main.rs(2 hunks)tests/subcommand_merge.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Clarity over cleverness: favour explicit, readable Rust over terse or obscure idioms
Use functions and composition; extract reusable logic; prefer declarative iterator-based code when readable
Keep functions small, single-responsibility, and obey command/query segregation
Name things precisely; use is/has/should prefixes for booleans
Use consistent en-GB-oxendict spelling and grammar in code comments (except external API references)
Function documentation must include clear examples; omit redundant examples in test docs
Keep code files ≤ 400 lines; split long switches/dispatch tables; move large test data to external files
Disallow Clippy warnings; fix warnings in code instead of silencing
Extract helper functions when functions are too long; maintain separation of concerns and CQRS
Group numerous related parameters into meaningful structs
Consider using Arc for large error values to reduce returned data size
Begin every module with a module-level //! comment describing purpose and utility
Document public APIs with /// Rustdoc so cargo doc can generate docs
Prefer immutable data; avoid unnecessary mut
Use Result for error handling; avoid panicking where feasible
Avoid unsafe unless absolutely necessary; document any usage clearly
Place function attributes after doc comments
Do not use return in single-line functions
Use predicate functions for conditionals with more than two branches
Do not silence lints except as a last resort
Scope lint suppressions narrowly and include a clear reason
Prefer #[expect(...)] over #[allow(...)] for lint suppression
Prefer .expect() over .unwrap()
Use concat!() for long string literals instead of backslash newline escapes
Prefer semantic error enums deriving std::error::Error via thiserror for caller-inspectable conditions
Never export eyre::Report from libraries; convert to domain error enums at API boundaries; use eyre only in main()/top-level async task
Files:
tests/subcommand_merge.rssrc/main.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in tests ...
Files:
tests/subcommand_merge.rssrc/main.rs
{src,tests}/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
{src,tests}/**/*.rs: Write unit and behavioural tests for new functionality; run both before and after changes
Use an opaque error (eyre::Report) only at app boundary; not in public APIs
Files:
tests/subcommand_merge.rssrc/main.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.rs: Use rstest fixtures for shared setup
Replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks/stubs
Files:
tests/subcommand_merge.rs
🧬 Code graph analysis (1)
src/main.rs (1)
tests/subcommand_merge.rs (2)
pr_cli(67-76)issue_cli(78-84)
🪛 GitHub Actions: CI
tests/subcommand_merge.rs
[error] 52-56: Rustfmt formatting check failed. Detected formatting difference in tests/subcommand_merge.rs:52. Run 'cargo fmt' to reformat the file.
🔍 Remote MCP Deepwiki
Summary — additional repository context relevant to review
-
src/main.rs defines load_with_reference_fallback(cli_args: T) that currently calls load_and_merge_subcommand_for::(&cli_args) and falls back to returning the original cli_args when OrthoError::Gathering contains a missing-reference error. Verify the new SubcmdConfigMerge::load_and_merge preserves this fallback behavior.
Snippet (from repo search):
fn load_with_reference_fallback(cli_args: T) -> Result<T, OrthoError> where T: OrthoConfig + ... { match load_and_merge_subcommand_for::(&cli_args) { Ok(v) => Ok(v), Err(OrthoError::Gathering(e)) => { if missing_reference(&e) { Ok(cli_args) } else { Err(OrthoError::Gathering(e)) } } Err(e) => Err(e), } } -
The main command dispatch currently uses load_with_reference_fallback for PR and Issue subcommands before calling the runners; confirm the PR’s replacement (pr_cli.load_and_merge(), issue_cli.load_and_merge()) is used at the same call sites and that error propagation/await semantics are unchanged:
Commands::Pr(pr_cli) => { let args = load_with_reference_fallback::(pr_cli.clone())?; run_pr(args, &global).await }
Commands::Issue(issue_cli) => { let args = load_with_reference_fallback::(issue_cli.clone())?; run_issue(args, &global).await } -
The trait SubcmdConfigMerge was not found in this repository (it appears to come from the external ortho_config crate). As part of the review, ensure the crate dependency was bumped/updated so SubcmdConfigMerge::load_and_merge is available and that the CLI arg types (PrArgs, IssueArgs, ResolveArgs) implement/derive whatever ortho_config requires (signatures/Result types must match expected usage).
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (4)
tests/subcommand_merge.rs (2)
9-9: Import the trait to enable method resolution.Keep
SubcmdConfigMergein scope so.load_and_merge()is available on the CLI types.
52-56: Fix rustfmt failure in tests/subcommand_merge.rsRun cargo fmt --all and commit the reformatted file to unblock CI.
File: tests/subcommand_merge.rs (lines 52-56)
} } let merged = cli.load_and_merge() .unwrap_or_else(|err| panic!("merge {} args: {err}", std::any::type_name::<T>()));src/main.rs (2)
57-57: Import the new merge API traits.This enables the trait-based
.load_and_merge()usage below.
438-447: Verify missing-reference fallback semantics are preserved.The previous
load_with_reference_fallbackpath returned the original CLI args when config gathering failed due to a missing reference. Confirm.load_and_merge()now embeds that behaviour; if not, reintroduce an equivalent wrapper and update these call sites to use it.#!/bin/bash set -euo pipefail # Inspect whether the old fallback helper or equivalent logic still exists rg -n 'load_with_reference_fallback|missing_reference|OrthoError::Gathering' -C2 src
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/subcommand_merge.rs (1)
39-42: Tighten the generic bound with SubcmdConfigMerge.Add
SubcmdConfigMergeto the where‑clause so the call site’s requirements are explicit and Clippy errors are clearer. This repeats prior feedback.fn merge_with_sources<T>(config: &str, env: &[(&str, Option<&str>)], cli: &T) -> T where - T: ortho_config::OrthoConfig + serde::Serialize + Default + CommandFactory, + T: SubcmdConfigMerge + ortho_config::OrthoConfig + serde::Serialize + Default + CommandFactory, {#!/bin/bash # Verify the tightened bound and absence of the old helper. rg -nP -C2 '\bfn\s+merge_with_sources<[^>]*>\([^)]*\)\s*->\s*T\s*where\s*T:\s*(?s).*SubcmdConfigMerge' tests/subcommand_merge.rs rg -n 'load_and_merge_subcommand_for' -g '!**/target/**'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/cli_args.rs(1 hunks)tests/subcommand_merge.rs(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Clarity over cleverness: favour explicit, readable Rust over terse or obscure idioms
Use functions and composition; extract reusable logic; prefer declarative iterator-based code when readable
Keep functions small, single-responsibility, and obey command/query segregation
Name things precisely; use is/has/should prefixes for booleans
Use consistent en-GB-oxendict spelling and grammar in code comments (except external API references)
Function documentation must include clear examples; omit redundant examples in test docs
Keep code files ≤ 400 lines; split long switches/dispatch tables; move large test data to external files
Disallow Clippy warnings; fix warnings in code instead of silencing
Extract helper functions when functions are too long; maintain separation of concerns and CQRS
Group numerous related parameters into meaningful structs
Consider using Arc for large error values to reduce returned data size
Begin every module with a module-level //! comment describing purpose and utility
Document public APIs with /// Rustdoc so cargo doc can generate docs
Prefer immutable data; avoid unnecessary mut
Use Result for error handling; avoid panicking where feasible
Avoid unsafe unless absolutely necessary; document any usage clearly
Place function attributes after doc comments
Do not use return in single-line functions
Use predicate functions for conditionals with more than two branches
Do not silence lints except as a last resort
Scope lint suppressions narrowly and include a clear reason
Prefer #[expect(...)] over #[allow(...)] for lint suppression
Prefer .expect() over .unwrap()
Use concat!() for long string literals instead of backslash newline escapes
Prefer semantic error enums deriving std::error::Error via thiserror for caller-inspectable conditions
Never export eyre::Report from libraries; convert to domain error enums at API boundaries; use eyre only in main()/top-level async task
Files:
src/cli_args.rstests/subcommand_merge.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in tests ...
Files:
src/cli_args.rstests/subcommand_merge.rs
{src,tests}/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
{src,tests}/**/*.rs: Write unit and behavioural tests for new functionality; run both before and after changes
Use an opaque error (eyre::Report) only at app boundary; not in public APIs
Files:
src/cli_args.rstests/subcommand_merge.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.rs: Use rstest fixtures for shared setup
Replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks/stubs
Files:
tests/subcommand_merge.rs
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
PR: leynos/ortho-config#0
File: docs/subcommand-refinements.md:0-0
Timestamp: 2025-08-28T21:56:52.319Z
Learning: Applies to docs/**/*.rs : Use the SubcmdConfigMerge trait to enable a default load_and_merge that borrows self; import with `use ortho_config::SubcmdConfigMerge;`
🧬 Code graph analysis (1)
src/cli_args.rs (2)
src/test_utils_env.rs (2)
remove_var(22-27)set_var(12-17)tests/subcommand_merge.rs (1)
write_config(17-21)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-test
- GitHub Check: Sourcery review
🔇 Additional comments (10)
src/cli_args.rs (6)
127-136: Scope tests to the trait by importing SubcmdConfigMerge.Importing the trait enables the
.load_and_merge()method; this is correct for the new API.
160-165: Keep tempdir alive for the duration of reads.Returning the TempDir alongside the path is correct; it keeps the directory alive for the caller’s scope. LGTM.
167-200: Validate CLI precedence over env and file.Assertions correctly exercise “CLI over env over file” for PrArgs. Nice coverage.
201-234: Assert selective merging (Option-only) from env.Test correctly verifies that only the optional reference is sourced from env and that eagerly-initialised fields (Vec/bool) are not overridden.
235-255: Assert absence behaviour.Test correctly ensures no sources leave Option fields as None and booleans at their CLI defaults.
256-282: Prove non-mutating merge.Test verifies
.load_and_merge()does not mutate the CLI instance; this guards the borrow‑based API. Good.tests/subcommand_merge.rs (4)
9-9: Import the trait to enable.load_and_merge().Trait import is required for method resolution; this aligns with the upgrade.
55-57: Use the trait method.load_and_merge().Call site matches the new API and keeps error messages informative with
type_name::<T>(). LGTM.
173-194: Verify env reference does not override eager fields.Test correctly asserts env fills only the optional reference and leaves Vec/bool untouched. Good signal against regressions.
196-218: Verify CLI overrides env for all fields.Test asserts the highest‑precedence path. Looks sound.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/subcommand_merge.rs (1)
39-43: Add explicit SubcmdConfigMerge bound to the genericTighten the signature so compile errors are precise and Clippy stays quiet.
Apply this diff:
where - T: ortho_config::OrthoConfig + serde::Serialize + Default + CommandFactory, + T: SubcmdConfigMerge + ortho_config::OrthoConfig + serde::Serialize + Default + CommandFactory,Refactor the function `merge_with_sources` in tests/subcommand_merge.rs to add `SubcmdConfigMerge` to the generic bounds. Re-run `cargo clippy` to confirm the previous warning disappears.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
tests/subcommand_merge.rs(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Clarity over cleverness: favour explicit, readable Rust over terse or obscure idioms
Use functions and composition; extract reusable logic; prefer declarative iterator-based code when readable
Keep functions small, single-responsibility, and obey command/query segregation
Name things precisely; use is/has/should prefixes for booleans
Use consistent en-GB-oxendict spelling and grammar in code comments (except external API references)
Function documentation must include clear examples; omit redundant examples in test docs
Keep code files ≤ 400 lines; split long switches/dispatch tables; move large test data to external files
Disallow Clippy warnings; fix warnings in code instead of silencing
Extract helper functions when functions are too long; maintain separation of concerns and CQRS
Group numerous related parameters into meaningful structs
Consider using Arc for large error values to reduce returned data size
Begin every module with a module-level //! comment describing purpose and utility
Document public APIs with /// Rustdoc so cargo doc can generate docs
Prefer immutable data; avoid unnecessary mut
Use Result for error handling; avoid panicking where feasible
Avoid unsafe unless absolutely necessary; document any usage clearly
Place function attributes after doc comments
Do not use return in single-line functions
Use predicate functions for conditionals with more than two branches
Do not silence lints except as a last resort
Scope lint suppressions narrowly and include a clear reason
Prefer #[expect(...)] over #[allow(...)] for lint suppression
Prefer .expect() over .unwrap()
Use concat!() for long string literals instead of backslash newline escapes
Prefer semantic error enums deriving std::error::Error via thiserror for caller-inspectable conditions
Never export eyre::Report from libraries; convert to domain error enums at API boundaries; use eyre only in main()/top-level async task
Files:
tests/subcommand_merge.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in tests ...
Files:
tests/subcommand_merge.rs
{src,tests}/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
{src,tests}/**/*.rs: Write unit and behavioural tests for new functionality; run both before and after changes
Use an opaque error (eyre::Report) only at app boundary; not in public APIs
Files:
tests/subcommand_merge.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.rs: Use rstest fixtures for shared setup
Replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks/stubs
Files:
tests/subcommand_merge.rs
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (2)
tests/subcommand_merge.rs (2)
9-9: Import the trait to enable method-call syntaxKeep this
useso.load_and_merge()resolves via trait method syntax.
172-236: Approve parametrised PR merge tests — verifiedMerge changes. Good use of rstest; tests in src/cli_args.rs and tests/subcommand_merge.rs confirm VKCMDS_PR_REFERENCE is read from the environment while VKCMDS_PR_FILES and VKCMDS_PR_SHOW_OUTDATED remain CLI-only.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
tests/subcommand_merge.rs (2)
40-43: Tighten the generic bound to include SubcmdConfigMerge.Make compile errors clearer and keep Clippy quiet.
fn merge_with_sources<T>(config: &str, env: &[(&str, Option<&str>)], cli: &T) -> T where - T: ortho_config::OrthoConfig + serde::Serialize + Default + CommandFactory, + T: ortho_config::OrthoConfig + + serde::Serialize + + Default + + CommandFactory + + SubcmdConfigMerge, {
56-58: Include the config path in the panic for better diagnostics.Print which config was used on failure.
- let merged = cli - .load_and_merge() - .unwrap_or_else(|err| panic!("merge {} args: {err}", std::any::type_name::<T>())); + let merged = cli.load_and_merge().unwrap_or_else(|err| { + panic!( + "merge {} args with config {}: {err}", + std::any::type_name::<T>(), + config_path.display() + ) + });src/cli_args.rs (1)
137-158: Restore previous environment values in EnvGuard.Snapshot each key’s prior value and restore it on Drop to avoid leaking state when keys were already set.
Apply:
- struct EnvGuard { - keys: Vec<&'static str>, - } + struct EnvGuard { + saved: Vec<(&'static str, Option<std::ffi::OsString>)>, + } @@ - fn new(keys: &[&'static str]) -> Self { - for key in keys { - remove_var(key); - } - Self { - keys: keys.to_vec(), - } - } + fn new(keys: &[&'static str]) -> Self { + let saved = keys + .iter() + .map(|&k| (k, std::env::var_os(k))) + .collect::<Vec<_>>(); + for (key, _) in &saved { + remove_var(key); + } + Self { saved } + } @@ - fn drop(&mut self) { - for key in &self.keys { - remove_var(key); - } - } + fn drop(&mut self) { + for (key, val) in self.saved.drain(..) { + match val { + Some(v) => set_var(key, &v), + None => remove_var(key), + } + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/cli_args.rs(1 hunks)tests/subcommand_merge.rs(5 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Clarity over cleverness: favour explicit, readable Rust over terse or obscure idioms
Use functions and composition; extract reusable logic; prefer declarative iterator-based code when readable
Keep functions small, single-responsibility, and obey command/query segregation
Name things precisely; use is/has/should prefixes for booleans
Use consistent en-GB-oxendict spelling and grammar in code comments (except external API references)
Function documentation must include clear examples; omit redundant examples in test docs
Keep code files ≤ 400 lines; split long switches/dispatch tables; move large test data to external files
Disallow Clippy warnings; fix warnings in code instead of silencing
Extract helper functions when functions are too long; maintain separation of concerns and CQRS
Group numerous related parameters into meaningful structs
Consider using Arc for large error values to reduce returned data size
Begin every module with a module-level //! comment describing purpose and utility
Document public APIs with /// Rustdoc so cargo doc can generate docs
Prefer immutable data; avoid unnecessary mut
Use Result for error handling; avoid panicking where feasible
Avoid unsafe unless absolutely necessary; document any usage clearly
Place function attributes after doc comments
Do not use return in single-line functions
Use predicate functions for conditionals with more than two branches
Do not silence lints except as a last resort
Scope lint suppressions narrowly and include a clear reason
Prefer #[expect(...)] over #[allow(...)] for lint suppression
Prefer .expect() over .unwrap()
Use concat!() for long string literals instead of backslash newline escapes
Prefer semantic error enums deriving std::error::Error via thiserror for caller-inspectable conditions
Never export eyre::Report from libraries; convert to domain error enums at API boundaries; use eyre only in main()/top-level async task
Files:
src/cli_args.rstests/subcommand_merge.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in tests ...
Files:
src/cli_args.rstests/subcommand_merge.rs
{src,tests}/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
{src,tests}/**/*.rs: Write unit and behavioural tests for new functionality; run both before and after changes
Use an opaque error (eyre::Report) only at app boundary; not in public APIs
Files:
src/cli_args.rstests/subcommand_merge.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.rs: Use rstest fixtures for shared setup
Replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks/stubs
Files:
tests/subcommand_merge.rs
🧬 Code graph analysis (1)
src/cli_args.rs (2)
src/test_utils_env.rs (2)
remove_var(22-27)set_var(12-17)tests/subcommand_merge.rs (2)
drop(28-30)write_config(18-22)
🔍 Remote MCP Deepwiki
Here are the key additional facts relevant to reviewing PR #147 “Adopt new ortho_config merge API,” drawn from the repository documentation and source:
• Definition and Removal of the Old Free Function
– The free-function load_and_merge_subcommand_for (from module ortho_config::subcommand) was originally imported and invoked in src/main.rs around lines 830–832. It has now been removed.
• Introduction of the New Trait and Methods
– The ortho_config::SubcmdConfigMerge trait is introduced in the updated code. It provides an inherent .load_and_merge() method on each subcommand arg type (e.g. PrArgs, IssueArgs, ResolveArgs).
– These methods replace calls to the old free function, e.g.:
• Before:
let args = load_and_merge_subcommand_for(&pr_cli)?;
• After:
let args = pr_cli.load_and_merge()?;
This change occurred in src/main.rs around lines 830–840.
• Tests Updated to Reflect the API Change
– In tests/subcommand_merge.rs, all invocations of load_and_merge_subcommand_for(...) were replaced by calls to cli.load_and_merge(). The import path was changed from ortho_config::subcommand::load_and_merge_subcommand_for to ortho_config::SubcmdConfigMerge.
– In src/cli_args.rs, the SubcmdConfigMerge trait is imported and applied in unit tests for PrArgs, IssueArgs, and ResolveArgs under the #[cfg(test)] module.
• Ortho_config Version
– The project depends on ortho_config version v0.2.0, specified in Cargo.toml under dependencies. It is used throughout for hierarchical configuration merging via Figment.
These collected details confirm that PR #147 comprehensively replaces the old free-function merge API with the new inherent-trait-based API (SubcmdConfigMerge), and that both production code and tests have been updated accordingly.
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/cli_args.rs(1 hunks)tests/subcommand_merge.rs(6 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Clarity over cleverness: favour explicit, readable Rust over terse or obscure idioms
Use functions and composition; extract reusable logic; prefer declarative iterator-based code when readable
Keep functions small, single-responsibility, and obey command/query segregation
Name things precisely; use is/has/should prefixes for booleans
Use consistent en-GB-oxendict spelling and grammar in code comments (except external API references)
Function documentation must include clear examples; omit redundant examples in test docs
Keep code files ≤ 400 lines; split long switches/dispatch tables; move large test data to external files
Disallow Clippy warnings; fix warnings in code instead of silencing
Extract helper functions when functions are too long; maintain separation of concerns and CQRS
Group numerous related parameters into meaningful structs
Consider using Arc for large error values to reduce returned data size
Begin every module with a module-level //! comment describing purpose and utility
Document public APIs with /// Rustdoc so cargo doc can generate docs
Prefer immutable data; avoid unnecessary mut
Use Result for error handling; avoid panicking where feasible
Avoid unsafe unless absolutely necessary; document any usage clearly
Place function attributes after doc comments
Do not use return in single-line functions
Use predicate functions for conditionals with more than two branches
Do not silence lints except as a last resort
Scope lint suppressions narrowly and include a clear reason
Prefer #[expect(...)] over #[allow(...)] for lint suppression
Prefer .expect() over .unwrap()
Use concat!() for long string literals instead of backslash newline escapes
Prefer semantic error enums deriving std::error::Error via thiserror for caller-inspectable conditions
Never export eyre::Report from libraries; convert to domain error enums at API boundaries; use eyre only in main()/top-level async task
Files:
tests/subcommand_merge.rssrc/cli_args.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in tests ...
Files:
tests/subcommand_merge.rssrc/cli_args.rs
{src,tests}/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
{src,tests}/**/*.rs: Write unit and behavioural tests for new functionality; run both before and after changes
Use an opaque error (eyre::Report) only at app boundary; not in public APIs
Files:
tests/subcommand_merge.rssrc/cli_args.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.rs: Use rstest fixtures for shared setup
Replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks/stubs
Files:
tests/subcommand_merge.rs
🧬 Code graph analysis (2)
tests/subcommand_merge.rs (1)
src/cli_args.rs (3)
merged(232-233)merged(238-239)merged(244-245)
src/cli_args.rs (2)
src/test_utils_env.rs (2)
remove_var(22-27)set_var(12-17)tests/subcommand_merge.rs (2)
drop(28-30)write_config(18-22)
🔍 Remote MCP Ref
Summary of additional relevant facts for reviewing PR #147 (Adopt new ortho_config merge API)
-
API change: The project replaces the free function ortho_config::subcommand::load_and_merge_subcommand_for with the trait-based ortho_config::SubcmdConfigMerge and its inherent method .load_and_merge() on subcommand CLI types (PrArgs, IssueArgs, ResolveArgs). Call sites in src/main.rs now use pr_cli.load_and_merge(), issue_cli.load_and_merge(), resolve_cli.load_and_merge().
-
Tests updated/expanded:
- tests/subcommand_merge.rs updated to call cli.load_and_merge() instead of the removed free function; import updated to ortho_config::SubcmdConfigMerge. Resolve subcommand added to test coverage and precedence cases expanded (CliOverEnv, EnvOverFile, FileOverDefaults).
- src/cli_args.rs: large #[cfg(test)] module added with unit tests exercising load_and_merge() for PrArgs, IssueArgs, ResolveArgs; includes EnvGuard helper, temporary config file creation, manipulation of VK_CONFIG_PATH and VKCMDS_* env vars, and tests ensuring CLI instance immutability after merge. Tests are run serially to avoid interference.
-
Surface-level implications for review:
- API compatibility: ensure ortho_config v0.2.0 (the dependency upgrade) exposes SubcmdConfigMerge and that the trait impls for all affected CLI arg types exist and return the expected Result<T, VkError> types used by callers.
- Behavioral change risk is low at call sites (only how merged config is obtained changed), but review trait implementations to confirm merge precedence and semantics match previous free-function behavior (tests were added/updated to cover precedence).
- Confirm no remaining usages of the removed free function remain and that imports were updated consistently across code and tests.
Sources used
- PR page and diff for PR #147 (Adopt new ortho_config merge API) on GitHub (leynos/vk).
🔇 Additional comments (8)
tests/subcommand_merge.rs (5)
24-38: LGTM: RAII guard for cwd is correct and panic‑safe.
40-48: LGTM: Generic merge helper and environment hygiene read well.
116-119: Good coverage: add Resolve scenarios to precedence matrix.Also applies to: 182-220
56-58: Include the config path in the panic for diagnostics.Augment the message so failures name the config used.
- let merged = cli - .load_and_merge() - .unwrap_or_else(|err| panic!("merge {} args: {err}", std::any::type_name::<T>())); + let merged = cli.load_and_merge().unwrap_or_else(|err| { + panic!( + "merge {} args with config {}: {err}", + std::any::type_name::<T>(), + config_path.display() + ) + });
13-16: Normalise imports for ResolveArgs.File: tests/subcommand_merge.rs — lines 13–16
Import ResolveArgs alongside IssueArgs and PrArgs from the crate root; if ResolveArgs is not re-exported at the crate root, re-export it or keep all three under vk::cli_args::ResolveArgs.-use vk::cli_args::ResolveArgs; -use vk::test_utils::{remove_var, set_var}; -use vk::{IssueArgs, PrArgs}; +use vk::{IssueArgs, PrArgs, ResolveArgs}; +use vk::test_utils::{remove_var, set_var};Verify whether ResolveArgs is re-exported at the crate root by running:
rg -nP '^pub\s+use\s+(.+::)?cli_args::ResolveArgs\b' -C2 || rg -n "ResolveArgs" -Ssrc/cli_args.rs (3)
139-160: Fix EnvGuard to restore previous environment values.Snapshot prior values and restore them on Drop to avoid leaking state across serial tests.
- struct EnvGuard { - keys: Vec<&'static str>, - } + struct EnvGuard { + saved: Vec<(&'static str, Option<std::ffi::OsString>)>, + } @@ - fn new(keys: &[&'static str]) -> Self { - for key in keys { - remove_var(key); - } - Self { - keys: keys.to_vec(), - } - } + fn new(keys: &[&'static str]) -> Self { + let saved = keys + .iter() + .map(|&k| (k, std::env::var_os(k))) + .collect::<Vec<_>>(); + for (key, _) in &saved { + remove_var(key); + } + Self { saved } + } @@ - impl Drop for EnvGuard { - fn drop(&mut self) { - for key in &self.keys { - remove_var(key); - } - } - } + impl Drop for EnvGuard { + fn drop(&mut self) { + for (key, val) in self.saved.drain(..) { + match val { + Some(v) => set_var(key, v), + None => remove_var(key), + } + } + } + }
409-463: Guard working‑directory changes with RAII and actually test discovery.Wrap cwd changes in a DirGuard to avoid leaks on panic, and clear VK_CONFIG_PATH when you intend to test discovery from the current directory.
@@ - let (_config_dir, config_path) = setup_env_and_config(&scenario, &cfg); + let (_config_dir, config_path) = setup_env_and_config(&scenario, &cfg); @@ - let prev_dir = std::env::current_dir().expect("current dir"); - let config_dir = config_path.parent().expect("config dir"); - std::env::set_current_dir(config_dir).expect("set dir"); - let merged = cli.load_and_merge().expect("merge issue args"); - std::env::set_current_dir(prev_dir).expect("restore dir"); + // Use discovery, not VK_CONFIG_PATH + remove_var("VK_CONFIG_PATH"); + struct DirGuard(std::path::PathBuf); + impl DirGuard { + fn enter<P: AsRef<std::path::Path>>(dir: P) -> Self { + let prev = std::env::current_dir().expect("current dir"); + std::env::set_current_dir(dir).expect("set dir"); + Self(prev) + } + } + impl Drop for DirGuard { + fn drop(&mut self) { + let _ = std::env::set_current_dir(&self.0); + } + } + let config_dir = config_path.parent().expect("config dir"); + let _dir = DirGuard::enter(config_dir); + let merged = cli.load_and_merge().expect("merge issue args"); @@ - let prev_dir = std::env::current_dir().expect("current dir"); - let config_dir = config_path.parent().expect("config dir"); - std::env::set_current_dir(config_dir).expect("set dir"); - let merged = cli.load_and_merge().expect("merge resolve args"); - std::env::set_current_dir(prev_dir).expect("restore dir"); + remove_var("VK_CONFIG_PATH"); + let config_dir = config_path.parent().expect("config dir"); + let _dir = DirGuard::enter(config_dir); + let merged = cli.load_and_merge().expect("merge resolve args");Follow‑up: extract DirGuard to a shared test_utils module and reuse here and in tests/subcommand_merge.rs.
127-134: Resolve: no in-source uses ofload_and_merge_subcommand_for;SubcmdConfigMergeimports present.Close this review — ripgrep shows
load_and_merge_subcommand_foronly in docs;SubcmdConfigMergeis imported in src/cli_args.rs:131, src/main.rs:57 and tests/subcommand_merge.rs:9.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: src/cli_args.rs ❌ New issue: Code Duplication |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@leynos I'll examine the code duplication and suggest a fix. Let me first look at the current implementation to understand the pattern. Now I can see the duplication pattern. Here's the fix: Suggested FixReplace the individual scenario functions with a data-driven approach using a macro or a parameterized function. The most elegant solution is to use a const array of scenario data: // Replace the individual functions with this data structure and helper
const SCENARIO_DATA: &[(SubcommandType, &[&str], &str)] = &[
(SubcommandType::Pr, &[
"VK_CONFIG_PATH",
"VKCMDS_PR_REFERENCE",
"VKCMDS_PR_FILES",
"VKCMDS_PR_SHOW_OUTDATED",
], "pr"),
(SubcommandType::Issue, &[
"VK_CONFIG_PATH",
"VKCMDS_ISSUE_REFERENCE",
"VKCMDS_ISSUE_MESSAGE",
], "issue"),
(SubcommandType::Resolve, &[
"VK_CONFIG_PATH",
"VKCMDS_RESOLVE_REFERENCE",
"VKCMDS_RESOLVE_MESSAGE",
], "resolve"),
];
fn get_scenario(subcommand: SubcommandType) -> TestScenario {
let (subcmd, env_vars, config_section) = SCENARIO_DATA
.iter()
.find(|(s, _, _)| *s == subcommand)
.expect("Unknown subcommand type");
TestScenario {
subcommand: *subcmd,
env_vars,
config_section,
}
}
// Then replace calls like pr_scenario() with get_scenario(SubcommandType::Pr)AI Agent Prompt🏁 Script executed: Length of output: 1096 🏁 Script executed: Length of output: 508 🏁 Script executed: Length of output: 1137 Perfect! Now I can see the full pattern. These are rstest fixtures (indicated by Suggested FixReplace the duplicated fixture functions with a data-driven approach while preserving the rstest fixture pattern: // Add this const data structure
const SCENARIO_DATA: &[(SubcommandType, &[&str], &str)] = &[
(SubcommandType::Pr, &[
"VK_CONFIG_PATH",
"VKCMDS_PR_REFERENCE",
"VKCMDS_PR_FILES",
"VKCMDS_PR_SHOW_OUTDATED",
], "pr"),
(SubcommandType::Issue, &[
"VK_CONFIG_PATH",
"VKCMDS_ISSUE_REFERENCE",
], "issue"),
(SubcommandType::Resolve, &[
"VK_CONFIG_PATH",
"VKCMDS_RESOLVE_REFERENCE",
"VKCMDS_RESOLVE_MESSAGE",
], "resolve"),
];
// Helper function to create scenarios
fn create_scenario(subcommand: SubcommandType) -> TestScenario {
let (subcmd, env_vars, config_section) = SCENARIO_DATA
.iter()
.find(|(s, _, _)| *s == subcommand)
.expect("Unknown subcommand type");
TestScenario {
subcommand: *subcmd,
env_vars,
config_section,
}
}
// Replace the three fixture functions with these simplified versions
#[fixture]
fn pr_scenario() -> TestScenario {
create_scenario(SubcommandType::Pr)
}
#[fixture]
fn issue_scenario() -> TestScenario {
create_scenario(SubcommandType::Issue)
}
#[fixture]
fn resolve_scenario() -> TestScenario {
create_scenario(SubcommandType::Resolve)
}AI Agent Prompt |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/cli_args.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Clarity over cleverness: favour explicit, readable Rust over terse or obscure idioms
Use functions and composition; extract reusable logic; prefer declarative iterator-based code when readable
Keep functions small, single-responsibility, and obey command/query segregation
Name things precisely; use is/has/should prefixes for booleans
Use consistent en-GB-oxendict spelling and grammar in code comments (except external API references)
Function documentation must include clear examples; omit redundant examples in test docs
Keep code files ≤ 400 lines; split long switches/dispatch tables; move large test data to external files
Disallow Clippy warnings; fix warnings in code instead of silencing
Extract helper functions when functions are too long; maintain separation of concerns and CQRS
Group numerous related parameters into meaningful structs
Consider using Arc for large error values to reduce returned data size
Begin every module with a module-level //! comment describing purpose and utility
Document public APIs with /// Rustdoc so cargo doc can generate docs
Prefer immutable data; avoid unnecessary mut
Use Result for error handling; avoid panicking where feasible
Avoid unsafe unless absolutely necessary; document any usage clearly
Place function attributes after doc comments
Do not use return in single-line functions
Use predicate functions for conditionals with more than two branches
Do not silence lints except as a last resort
Scope lint suppressions narrowly and include a clear reason
Prefer #[expect(...)] over #[allow(...)] for lint suppression
Prefer .expect() over .unwrap()
Use concat!() for long string literals instead of backslash newline escapes
Prefer semantic error enums deriving std::error::Error via thiserror for caller-inspectable conditions
Never export eyre::Report from libraries; convert to domain error enums at API boundaries; use eyre only in main()/top-level async task
Files:
src/cli_args.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in tests ...
Files:
src/cli_args.rs
{src,tests}/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
{src,tests}/**/*.rs: Write unit and behavioural tests for new functionality; run both before and after changes
Use an opaque error (eyre::Report) only at app boundary; not in public APIs
Files:
src/cli_args.rs
🧬 Code graph analysis (1)
src/cli_args.rs (2)
src/test_utils_env.rs (2)
remove_var(22-27)set_var(12-17)tests/subcommand_merge.rs (2)
drop(28-30)write_config(18-22)
🔍 Remote MCP Ref
Relevant findings for reviewing PR #147 (Adopt new ortho_config merge API)
-
API change: the free function ortho_config::subcommand::load_and_merge_subcommand_for was replaced by the trait ortho_config::SubcmdConfigMerge exposing an inherent method .load_and_merge() on subcommand CLI types; call sites in main.rs are updated to use pr_cli.load_and_merge(), issue_cli.load_and_merge(), resolve_cli.load_and_merge().,
-
Tests updated:
- tests/subcommand_merge.rs now imports ortho_config::SubcmdConfigMerge and calls cli.load_and_merge(); Resolve subcommand added; precedence cases expanded (CliOverEnv, EnvOverFile, FileOverDefaults).
- src/cli_args.rs gained a comprehensive #[cfg(test)] module with EnvGuard, TempDir-based config helpers, and many tests validating merge precedence, preservation of original CLI instance, and config discovery. Tests run serially to avoid env interference.
-
Review action items / risk areas:
- Verify ortho_config dependency version used in Cargo.lock exposes SubcmdConfigMerge and implements load_and_merge for PrArgs, IssueArgs, ResolveArgs with the same error/result types expected by callers. (If not, CI/build will fail.)
- Confirm no remaining uses of the removed free function remain in codebase or tests.
- Validate tests cover the previous precedence semantics (CLI > env > file > defaults) — updated tests indicate coverage for PR and Resolve; ensure Issue coverage too.
- Consider applying the suggested test refactor (consolidate pr_scenario/issue_scenario/resolve_scenario) to remove duplication in src/cli_args.rs test fixtures (not required for correctness but recommended for maintainability). The PR discussion already includes concrete refactor options.
Cited sources/tools used:
- GitHub PR page for leynos/vk PR #147 (diff, description, tests)
- Documentation/search query for ortho_config SubcmdConfigMerge (search results)
🔇 Additional comments (4)
src/cli_args.rs (4)
183-223: Good data‑driven fixtures; SCENARIO_DATA + create_scenario is the right shape.Keep the one‑line rstest fixtures to preserve names while centralising data.
Also applies to: 224-237
139-160: Fix EnvGuard to snapshot and restore environment, not just remove keys.Prevent state leaks when tests run after a pre-set environment.
- struct EnvGuard { - keys: Vec<&'static str>, - } + struct EnvGuard { + saved: Vec<(&'static str, Option<std::ffi::OsString>)>, + } @@ - fn new(keys: &[&'static str]) -> Self { - for key in keys { - remove_var(key); - } - Self { - keys: keys.to_vec(), - } - } + fn new(keys: &[&'static str]) -> Self { + let saved = keys + .iter() + .map(|&k| (k, std::env::var_os(k))) + .collect::<Vec<_>>(); + for (key, _) in &saved { + remove_var(key); + } + Self { saved } + } @@ - fn drop(&mut self) { - for key in &self.keys { - remove_var(key); - } - } + fn drop(&mut self) { + for (key, val) in self.saved.drain(..) { + match val { + Some(v) => set_var(key, &v), + None => remove_var(key), + } + } + }
460-465: Use an RAII guard for working-directory changes.Avoid cwd leaks on panic by guarding directory switches.
Add a small guard (place after write_config):
+ struct DirGuard(std::path::PathBuf); + impl DirGuard { + fn enter<P: AsRef<std::path::Path>>(dir: P) -> Self { + let prev = std::env::current_dir().expect("current dir"); + std::env::set_current_dir(dir).expect("set dir"); + Self(prev) + } + } + impl Drop for DirGuard { + fn drop(&mut self) { + let _ = std::env::set_current_dir(&self.0); + } + }Replace manual save/restore:
- let prev_dir = std::env::current_dir().expect("current dir"); - let config_dir = config_path.parent().expect("config dir"); - std::env::set_current_dir(config_dir).expect("set dir"); - let merged = cli.load_and_merge().expect("merge issue args"); - std::env::set_current_dir(prev_dir).expect("restore dir"); + let config_dir = config_path.parent().expect("config dir"); + let _dir = DirGuard::enter(config_dir); + let merged = cli.load_and_merge().expect("merge issue args");- let prev_dir = std::env::current_dir().expect("current dir"); - let config_dir = config_path.parent().expect("config dir"); - std::env::set_current_dir(config_dir).expect("set dir"); - let merged = cli.load_and_merge().expect("merge resolve args"); - std::env::set_current_dir(prev_dir).expect("restore dir"); + let config_dir = config_path.parent().expect("config dir"); + let _dir = DirGuard::enter(config_dir); + let merged = cli.load_and_merge().expect("merge resolve args");Also applies to: 474-479, 162-168
249-270: DropAny/downcasts; assert via a tiny typed accessor trait.Remove dynamic typing and simplify assertions.
Apply within this block:
- fn assert_reference_equals(subcommand: SubcommandType, merged: &dyn Any, expected: &str) { - match subcommand { - SubcommandType::Pr => { - let args = merged - .downcast_ref::<PrArgs>() - .expect("reference downcasts to PrArgs"); - assert_eq!(args.reference.as_deref(), Some(expected)); - } - SubcommandType::Issue => { - let args = merged - .downcast_ref::<IssueArgs>() - .expect("reference downcasts to IssueArgs"); - assert_eq!(args.reference.as_deref(), Some(expected)); - } - SubcommandType::Resolve => { - let args = merged - .downcast_ref::<ResolveArgs>() - .expect("reference downcasts to ResolveArgs"); - assert_eq!(args.reference, expected); - } - } - } + fn assert_reference_equals<T: RefStr>(merged: &T, expected: &str) { + assert_eq!(merged.reference_str(), Some(expected)); + }Add the trait and impls near the helpers:
trait RefStr { fn reference_str(&self) -> Option<&str>; } impl RefStr for PrArgs { fn reference_str(&self) -> Option<&str> { self.reference.as_deref() } } impl RefStr for IssueArgs { fn reference_str(&self) -> Option<&str> { self.reference.as_deref() } } impl RefStr for ResolveArgs { fn reference_str(&self) -> Option<&str> { Some(self.reference.as_str()) } }Update call sites:
-assert_reference_equals(SubcommandType::Pr, &merged, "cli_ref"); +assert_reference_equals(&merged, "cli_ref"); -assert_reference_equals(SubcommandType::Issue, &merged, "cli_ref"); +assert_reference_equals(&merged, "cli_ref"); -assert_reference_equals(SubcommandType::Resolve, &merged, "cli_ref"); +assert_reference_equals(&merged, "cli_ref"); -assert_reference_equals(SubcommandType::Pr, &merged, "env_ref"); +assert_reference_equals(&merged, "env_ref"); -assert_reference_equals(SubcommandType::Issue, &merged, "env_ref"); +assert_reference_equals(&merged, "env_ref"); -assert_reference_equals(SubcommandType::Resolve, &merged, "cli_ref"); +assert_reference_equals(&merged, "cli_ref"); -assert_reference_equals(SubcommandType::Issue, &merged, "file_ref"); +assert_reference_equals(&merged, "file_ref"); -assert_reference_equals(SubcommandType::Resolve, &merged, "cli_ref"); +assert_reference_equals(&merged, "cli_ref"); -assert_reference_equals(SubcommandType::Pr, &merged, "cli_ref"); +assert_reference_equals(&merged, "cli_ref");
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/cli_args.rs (1)
1-413: Keep test files ≤ 400 linesSplit this file (e.g., move precedence tests into
tests/cli_args_merge.rs) or extract repeated config‑string builders to helpers to drop below the limit.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
tests/cli_args.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Clarity over cleverness: favour explicit, readable Rust over terse or obscure idioms
Use functions and composition; extract reusable logic; prefer declarative iterator-based code when readable
Keep functions small, single-responsibility, and obey command/query segregation
Name things precisely; use is/has/should prefixes for booleans
Use consistent en-GB-oxendict spelling and grammar in code comments (except external API references)
Function documentation must include clear examples; omit redundant examples in test docs
Keep code files ≤ 400 lines; split long switches/dispatch tables; move large test data to external files
Disallow Clippy warnings; fix warnings in code instead of silencing
Extract helper functions when functions are too long; maintain separation of concerns and CQRS
Group numerous related parameters into meaningful structs
Consider using Arc for large error values to reduce returned data size
Begin every module with a module-level //! comment describing purpose and utility
Document public APIs with /// Rustdoc so cargo doc can generate docs
Prefer immutable data; avoid unnecessary mut
Use Result for error handling; avoid panicking where feasible
Avoid unsafe unless absolutely necessary; document any usage clearly
Place function attributes after doc comments
Do not use return in single-line functions
Use predicate functions for conditionals with more than two branches
Do not silence lints except as a last resort
Scope lint suppressions narrowly and include a clear reason
Prefer #[expect(...)] over #[allow(...)] for lint suppression
Prefer .expect() over .unwrap()
Use concat!() for long string literals instead of backslash newline escapes
Prefer semantic error enums deriving std::error::Error via thiserror for caller-inspectable conditions
Never export eyre::Report from libraries; convert to domain error enums at API boundaries; use eyre only in main()/top-level async task
Files:
tests/cli_args.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in tests ...
Files:
tests/cli_args.rs
{src,tests}/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
{src,tests}/**/*.rs: Write unit and behavioural tests for new functionality; run both before and after changes
Use an opaque error (eyre::Report) only at app boundary; not in public APIs
Files:
tests/cli_args.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.rs: Use rstest fixtures for shared setup
Replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks/stubs
Files:
tests/cli_args.rs
🧬 Code graph analysis (1)
tests/cli_args.rs (1)
src/cli_args.rs (2)
default(91-93)default(119-124)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (6)
tests/cli_args.rs (6)
1-12: Adopt the new merge API correctlyImport the trait and call
.load_and_merge()on the CLI structs. Method resolution will work across all tests.
43-96: Deduplicate scenario data via a single source of truthUse
SCENARIO_DATAandcreate_scenarioto kill duplication and keep fixtures DRY. This matches the requested refactor.
98-111: Keep rstest fixtures as one‑linersPreserve fixture names while delegating to
create_scenario. This minimises churn at call sites.
146-223: Validate CLI > env > file precedence for all subcommandsExercise each field and assert CLI wins. Coverage looks correct.
225-296: Assert env fills only unset optionalsDocument the clap behaviour and verify only optional fields are overridden. Good negative checks on vectors/bools.
360-412: Ensure.load_and_merge()does not mutate the CLI instanceClone/snapshot assertions cover immutability nicely across structs.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/subcommand_merge.rs (1)
64-76: Eliminate flakiness by clearing VKCMDS_PR_SHOW_OUTDATED in FileOverDefaultsThe test does not clear VKCMDS_PR_SHOW_OUTDATED. If present in the host environment, it can affect results despite expectations focusing on reference. Clear it explicitly.
Apply this diff:
(SubcommandType::Pr, PrecedenceScenario::FileOverDefaults) => { @@ let merged = merge_with_sources( cfg, - &[("VKCMDS_PR_REFERENCE", None), ("VKCMDS_PR_FILES", None)], + &[ + ("VKCMDS_PR_REFERENCE", None), + ("VKCMDS_PR_FILES", None), + ("VKCMDS_PR_SHOW_OUTDATED", None), + ], &cli, );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
tests/cli_args_merge.rs(1 hunks)tests/subcommand_merge.rs(3 hunks)tests/support/env.rs(1 hunks)tests/support/subcommand.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Clarity over cleverness: favour explicit, readable Rust over terse or obscure idioms
Use functions and composition; extract reusable logic; prefer declarative iterator-based code when readable
Keep functions small, single-responsibility, and obey command/query segregation
Name things precisely; use is/has/should prefixes for booleans
Use consistent en-GB-oxendict spelling and grammar in code comments (except external API references)
Function documentation must include clear examples; omit redundant examples in test docs
Keep code files ≤ 400 lines; split long switches/dispatch tables; move large test data to external files
Disallow Clippy warnings; fix warnings in code instead of silencing
Extract helper functions when functions are too long; maintain separation of concerns and CQRS
Group numerous related parameters into meaningful structs
Consider using Arc for large error values to reduce returned data size
Begin every module with a module-level //! comment describing purpose and utility
Document public APIs with /// Rustdoc so cargo doc can generate docs
Prefer immutable data; avoid unnecessary mut
Use Result for error handling; avoid panicking where feasible
Avoid unsafe unless absolutely necessary; document any usage clearly
Place function attributes after doc comments
Do not use return in single-line functions
Use predicate functions for conditionals with more than two branches
Do not silence lints except as a last resort
Scope lint suppressions narrowly and include a clear reason
Prefer #[expect(...)] over #[allow(...)] for lint suppression
Prefer .expect() over .unwrap()
Use concat!() for long string literals instead of backslash newline escapes
Prefer semantic error enums deriving std::error::Error via thiserror for caller-inspectable conditions
Never export eyre::Report from libraries; convert to domain error enums at API boundaries; use eyre only in main()/top-level async task
Files:
tests/support/env.rstests/support/subcommand.rstests/subcommand_merge.rstests/cli_args_merge.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in tests ...
Files:
tests/support/env.rstests/support/subcommand.rstests/subcommand_merge.rstests/cli_args_merge.rs
{src,tests}/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
{src,tests}/**/*.rs: Write unit and behavioural tests for new functionality; run both before and after changes
Use an opaque error (eyre::Report) only at app boundary; not in public APIs
Files:
tests/support/env.rstests/support/subcommand.rstests/subcommand_merge.rstests/cli_args_merge.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.rs: Use rstest fixtures for shared setup
Replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks/stubs
Files:
tests/support/env.rstests/support/subcommand.rstests/subcommand_merge.rstests/cli_args_merge.rs
🧬 Code graph analysis (3)
tests/support/subcommand.rs (1)
tests/support/env.rs (3)
setup_env_and_config(76-80)new(22-31)enter(54-58)
tests/subcommand_merge.rs (1)
tests/support/subcommand.rs (4)
issue_cli(48-54)merge_with_sources(14-33)pr_cli(36-45)resolve_cli(57-62)
tests/cli_args_merge.rs (1)
tests/support/env.rs (3)
setup_env_and_config(76-80)new(22-31)enter(54-58)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (7)
tests/subcommand_merge.rs (2)
103-119: LGTM: Resolve precedence path is exercised thoroughlyThe CLI-over-env branch asserts both fields and matches the new API usage correctly.
145-208: Good targeted precedence coverage for PR with structured casesTests read well and validate CLI/env/file precedence.
Run the following locally (verification script failed to run in the sandbox):
#!/bin/bash set -euo pipefail cargo fmt --all rg -nF -e 'todo!(' -e 'dbg!(' src tests || trueFix any formatting or debug/todo occurrences and push; re-run CI.
tests/support/subcommand.rs (1)
14-17: Correct trait bounds for clearer errorsAdding SubcmdConfigMerge alongside OrthoConfig/Serialize/Default/CommandFactory tightens the API and keeps Clippy quiet.
tests/cli_args_merge.rs (4)
49-75: Nice de-duplication with SCENARIO_DATA mappingThe data-driven table removes repeated fixture logic cleanly while keeping readability.
90-103: Fixtures reduced to one-liners — duplication resolvedThe wrapper fixtures now delegate to create_scenario(...) as discussed. Keep this pattern.
Confirm rstest’s recommended pattern for data-driven fixtures and one-line wrappers remains stable for the current rstest release.
220-236: Comment accurately explains boolean/vector precedence behaviourThe note on Clap initialising non-Option fields is helpful context for future readers.
320-367: Validate no mutation of the original CLI instanceGood defensive snapshotting; keep this to guard against future regressions.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
Import the crate-level environment helper for the GraphQL client so the GitHub endpoint lookup holds the shared mutex. Also update the integration test guard to snapshot variables under the same lock before clearing them.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Verify that replacing std::env::args_os() with env::args_os() still resolves to the correct API—your new environment module doesn’t expose args_os(), so this call might be broken.
- There’s a lot of duplicated precedence logic between tests/subcommand_merge.rs and tests/cli_args_merge.rs; consider consolidating them or extracting shared fixtures to reduce maintenance overhead.
- The new cli_args_merge tests are very verbose—see if you can further parameterize or abstract the common scenario setup to make the suite more concise and DRY.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Verify that replacing std::env::args_os() with env::args_os() still resolves to the correct API—your new environment module doesn’t expose args_os(), so this call might be broken.
- There’s a lot of duplicated precedence logic between tests/subcommand_merge.rs and tests/cli_args_merge.rs; consider consolidating them or extracting shared fixtures to reduce maintenance overhead.
- The new cli_args_merge tests are very verbose—see if you can further parameterize or abstract the common scenario setup to make the suite more concise and DRY.
## Individual Comments
### Comment 1
<location> `src/environment.rs:26` </location>
<code_context>
+pub fn set_var<K: AsRef<OsStr>, V: AsRef<OsStr>>(key: K, value: V) {
+ let _guard = lock();
+ // SAFETY: the mutex serialises access to the unsynchronised std env calls.
+ unsafe { env::set_var(key, value) };
+}
+
</code_context>
<issue_to_address>
**issue:** The use of unsafe here is unnecessary and could be misleading.
env::set_var and env::remove_var are safe and do not require unsafe blocks. Please remove the unsafe block unless absolutely necessary.
</issue_to_address>
### Comment 2
<location> `tests/subcommand_merge.rs:165` </location>
<code_context>
+#[derive(Debug)]
+struct PrMergeTestCase {
</code_context>
<issue_to_address>
**suggestion (testing):** Suggestion to add edge case tests for empty and malformed environment variable values.
Add tests for cases where environment variables are empty or contain invalid values to verify correct fallback behavior.
</issue_to_address>
### Comment 3
<location> `tests/cli_args_merge.rs:213-222` </location>
<code_context>
+fn load_and_merge_uses_environment_when_cli_defaults(#[case] scenario: TestScenario) {
</code_context>
<issue_to_address>
**suggestion (testing):** Suggestion to add tests for missing environment variables.
Please add test cases where required environment variables are absent to verify that the merge logic handles missing values gracefully and defaults as intended.
Suggested implementation:
```rust
#[rstest]
#[case::pr(pr_scenario())]
#[case::issue(issue_scenario())]
#[case::resolve(resolve_scenario())]
#[case::pr_missing_env(pr_scenario())]
#[case::issue_missing_env(issue_scenario())]
#[case::resolve_missing_env(resolve_scenario())]
#[serial]
fn load_and_merge_uses_environment_when_cli_defaults(#[case] scenario: TestScenario) {
let missing_env = scenario.name.ends_with("_missing_env");
if missing_env {
// Unset all relevant environment variables for this scenario
match scenario.subcommand {
SubcommandType::Pr => {
std::env::remove_var("VKCMDS_PR_REFERENCE");
std::env::remove_var("VKCMDS_PR_FILES");
std::env::remove_var("VKCMDS_PR_SHOW_OUTDATED");
}
SubcommandType::Issue => {
std::env::remove_var("VKCMDS_ISSUE_REFERENCE");
std::env::remove_var("VKCMDS_ISSUE_LABELS");
}
SubcommandType::Resolve => {
std::env::remove_var("VKCMDS_RESOLVE_REFERENCE");
std::env::remove_var("VKCMDS_RESOLVE_STATUS");
}
}
} else {
let _guard = EnvGuard::new(scenario.env_vars);
match scenario.subcommand {
SubcommandType::Pr => {
set_var("VKCMDS_PR_REFERENCE", "env_ref");
set_var("VKCMDS_PR_FILES", "env_one.rs,env_two.rs");
set_var("VKCMDS_PR_SHOW_OUTDATED", "true");
}
SubcommandType::Issue => {
set_var("VKCMDS_ISSUE_REFERENCE", "env_issue_ref");
set_var("VKCMDS_ISSUE_LABELS", "env_bug,env_feature");
}
SubcommandType::Resolve => {
set_var("VKCMDS_RESOLVE_REFERENCE", "env_resolve_ref");
set_var("VKCMDS_RESOLVE_STATUS", "env_closed");
}
}
}
let cfg = config_for_scenario(&scenario, ConfigVariant::EnvFallback);
let (_config_dir, _config_path) = setup_env_and_config(&cfg);
match scenario.subcommand {
SubcommandType::Pr => {
if missing_env {
// Check that defaults are used when env vars are missing
let merged = merge_cli_and_env(&cfg, &scenario.cli_args);
assert_eq!(merged.reference.as_deref(), Some("cli_ref"));
assert_eq!(merged.files.as_deref(), Some("cli_one.rs,cli_two.rs"));
assert_eq!(merged.show_outdated, Some(false));
} else {
set_var("VKCMDS_PR_REFERENCE", "env_ref");
set_var("VKCMDS_PR_FILES", "env_one.rs,env_two.rs");
set_var("VKCMDS_PR_SHOW_OUTDATED", "true");
let merged = merge_cli_and_env(&cfg, &scenario.cli_args);
assert_eq!(merged.reference.as_deref(), Some("env_ref"));
assert_eq!(merged.files.as_deref(), Some("env_one.rs,env_two.rs"));
assert_eq!(merged.show_outdated, Some(true));
}
}
SubcommandType::Issue => {
if missing_env {
let merged = merge_cli_and_env(&cfg, &scenario.cli_args);
assert_eq!(merged.reference.as_deref(), Some("cli_issue_ref"));
assert_eq!(merged.labels.as_deref(), Some("cli_bug,cli_feature"));
} else {
set_var("VKCMDS_ISSUE_REFERENCE", "env_issue_ref");
set_var("VKCMDS_ISSUE_LABELS", "env_bug,env_feature");
let merged = merge_cli_and_env(&cfg, &scenario.cli_args);
assert_eq!(merged.reference.as_deref(), Some("env_issue_ref"));
assert_eq!(merged.labels.as_deref(), Some("env_bug,env_feature"));
}
}
SubcommandType::Resolve => {
if missing_env {
let merged = merge_cli_and_env(&cfg, &scenario.cli_args);
assert_eq!(merged.reference.as_deref(), Some("cli_resolve_ref"));
assert_eq!(merged.status.as_deref(), Some("cli_closed"));
} else {
set_var("VKCMDS_RESOLVE_REFERENCE", "env_resolve_ref");
set_var("VKCMDS_RESOLVE_STATUS", "env_closed");
let merged = merge_cli_and_env(&cfg, &scenario.cli_args);
assert_eq!(merged.reference.as_deref(), Some("env_resolve_ref"));
assert_eq!(merged.status.as_deref(), Some("env_closed"));
}
}
}
```
You may need to update the `TestScenario` struct to include a `name` field or another way to distinguish the new "missing env" cases. Also, ensure that the `pr_scenario()`, `issue_scenario()`, and `resolve_scenario()` functions can generate scenarios for both normal and missing environment variable cases.
</issue_to_address>
### Comment 4
<location> `tests/cli_args_merge.rs:265-274` </location>
<code_context>
+fn load_and_merge_uses_config_or_defaults(#[case] scenario: TestScenario) {
</code_context>
<issue_to_address>
**suggestion (testing):** Suggestion to add tests for invalid or corrupted config files.
Add tests for cases where the config file is missing, empty, or contains invalid TOML to verify error handling and robustness.
Suggested implementation:
```rust
#[rstest]
#[case::pr(pr_scenario())]
#[case::issue(issue_scenario())]
#[case::resolve(resolve_scenario())]
#[serial]
fn load_and_merge_uses_config_or_defaults(#[case] scenario: TestScenario) {
let _guard = EnvGuard::new(scenario.env_vars);
let cfg = config_for_scenario(&scenario, ConfigVariant::ConfigFallback);
let (config_dir, _config_path) = setup_env_and_config(&cfg);
match scenario.subcommand {
SubcommandType::Pr => {
let cli = PrArgs::default();
let merged = cli.load_and_merge().expect("merge pr args");
}
// ... other subcommands
}
}
/// Test: missing config file should fall back to defaults or error gracefully
#[test]
fn test_missing_config_file() {
let scenario = pr_scenario();
let _guard = EnvGuard::new(scenario.env_vars);
// Simulate missing config by not creating any config file
let config_dir = tempfile::tempdir().unwrap();
std::env::set_var("CONFIG_DIR", config_dir.path());
let cli = PrArgs::default();
let result = cli.load_and_merge();
assert!(result.is_ok(), "Should handle missing config gracefully");
}
/// Test: empty config file should fall back to defaults or error gracefully
#[test]
fn test_empty_config_file() {
let scenario = pr_scenario();
let _guard = EnvGuard::new(scenario.env_vars);
let config_dir = tempfile::tempdir().unwrap();
let config_path = config_dir.path().join("config.toml");
std::fs::write(&config_path, "").unwrap();
std::env::set_var("CONFIG_DIR", config_dir.path());
let cli = PrArgs::default();
let result = cli.load_and_merge();
assert!(result.is_ok(), "Should handle empty config gracefully");
}
/// Test: corrupted/invalid TOML config file should error
#[test]
fn test_invalid_toml_config_file() {
let scenario = pr_scenario();
let _guard = EnvGuard::new(scenario.env_vars);
let config_dir = tempfile::tempdir().unwrap();
let config_path = config_dir.path().join("config.toml");
std::fs::write(&config_path, "not = valid = toml").unwrap();
std::env::set_var("CONFIG_DIR", config_dir.path());
let cli = PrArgs::default();
let result = cli.load_and_merge();
assert!(result.is_err(), "Should error on invalid TOML config");
}
```
- If your config loading logic uses a different environment variable or file path, adjust the test setup accordingly.
- If `load_and_merge()` does not return a `Result`, you may need to update its implementation to do so, or adjust the assertions to match its error handling.
- You may need to import `tempfile` and other dependencies at the top of the file if not already present:
```rust
use tempfile;
use std::fs;
use std::env;
```
</issue_to_address>
### Comment 5
<location> `src/environment.rs:10` </location>
<code_context>
+use std::ffi::OsStr;
+use std::sync::{Mutex, MutexGuard, OnceLock};
+
+static ENV_LOCK: OnceLock<Mutex<()>> = OnceLock::new();
+
+fn lock() -> MutexGuard<'static, ()> {
</code_context>
<issue_to_address>
**issue (complexity):** Consider replacing OnceLock and unsafe usage with a static Mutex to simplify environment locking.
```rust
// ─ Simplify the global lock: no OnceLock or unsafe required ─
use std::env;
use std::ffi::OsStr;
use std::sync::Mutex;
// make the mutex a true `const fn` on recent Rust:
static ENV_LOCK: Mutex<()> = Mutex::new(());
// helper stays tiny (optional—tests can lock ENV_LOCK directly)
pub fn with_lock<T, F: FnOnce() -> T>(f: F) -> T {
let _guard = ENV_LOCK.lock().unwrap();
f()
}
pub fn set_var<K: AsRef<OsStr>, V: AsRef<OsStr>>(key: K, val: V) {
let _guard = ENV_LOCK.lock().unwrap();
// safe std call, no `unsafe` needed
env::set_var(key, val);
}
pub fn remove_var<K: AsRef<OsStr>>(key: K) {
let _guard = ENV_LOCK.lock().unwrap();
env::remove_var(key);
}
pub fn var<K: AsRef<OsStr>>(key: K) -> Result<String, env::VarError> {
let _guard = ENV_LOCK.lock().unwrap();
env::var(key)
}
```
Tests would then simply call `env::set_var`/`remove_var` (or `with_lock`) under the global mutex—no unsafe or OnceLock boilerplate remains.
</issue_to_address>
### Comment 6
<location> `src/environment.rs:59` </location>
<code_context>
+ op()
+}
+
+#[cfg(test)]
+mod tests {
+ use super::*;
</code_context>
<issue_to_address>
**issue (review_instructions):** Add behavioural tests for environment helpers; only unit tests are present.
The new environment helpers module includes unit tests, but does not demonstrate behavioural tests that exercise the integration of these helpers in realistic scenarios. Add behavioural tests to ensure correct synchronisation and interaction in multi-threaded or multi-test contexts.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural *and* unit tests are required.
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai resolve |
1 participant
Summary
SubcmdConfigMerge::load_and_mergeafter upgrading ortho_configTesting
https://chatgpt.com/codex/tasks/task_e_68ccb4884d2083229ad87e863f89441e
Summary by Sourcery
Adopt the new ortho_config merge API for subcommands and centralize environment handling. Replace all raw std::env calls with a synchronized environment helper, refactor test utilities, and expand merge tests for full CLI/env/config precedence across pr, issue, and resolve commands.
New Features:
Enhancements:
Tests:
Chores: