Inject config environment access (#293)#330
Conversation
|
Warning Review limit reached
More reviews will be available in 3 hours, 50 minutes, and 32 seconds. Learn how PR review limits work. Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
ef01af0 to
e0a6dd0
Compare
e0a6dd0 to
ba1bce1
Compare
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. Code Duplicationsrc/main.rs: What lead to degradation?The module contains 2 functions with similar structure: merge_cli_or_exit,resolve_diag_mode_or_exit Why does this problem occur?Duplicated code often leads to code that's harder to change since the same logical change has to be done in multiple functions. More duplication gives lower code health. How to fix it?A certain degree of duplicated code might be acceptable. The problems start when it is the same behavior that is duplicated across the functions in the module, ie. a violation of the Don't Repeat Yourself (DRY) principle. DRY violations lead to code that is changed together in predictable patterns, which is both expensive and risky. DRY violations can be identified using CodeScene's X-Ray analysis to detect clusters of change coupled functions with high code similarity. Read More |
This comment was marked as resolved.
This comment was marked as resolved.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. Code Duplicationsrc/main.rs: What lead to degradation?The module contains 2 functions with similar structure: merge_cli_or_exit,resolve_diag_mode_or_exit Why does this problem occur?Duplicated code often leads to code that's harder to change since the same logical change has to be done in multiple functions. More duplication gives lower code health. How to fix it?A certain degree of duplicated code might be acceptable. The problems start when it is the same behavior that is duplicated across the functions in the module, ie. a violation of the Don't Repeat Yourself (DRY) principle. DRY violations lead to code that is changed together in predictable patterns, which is both expensive and risky. DRY violations can be identified using CodeScene's X-Ray analysis to detect clusters of change coupled functions with high code similarity. Read More |
This comment was marked as resolved.
This comment was marked as resolved.
Introduce an `EnvProvider` port for CLI configuration resolution. Thread it through explicit config selection, diagnostic layer collection, and early diagnostic JSON resolution so tests can use deterministic environment doubles instead of process-wide mutation. Reject malformed `NETSUKE_DIAG_JSON` values with a validation error, and keep discovery load errors visible while resolving startup diagnostic mode.
Drop provider-free compatibility wrappers that became dead code after the rebase onto the split `discovery` and `diag` modules. Keep the injected provider helpers as the single internal path for config selection and early JSON diagnostic resolution. Update the related internal docs and test helper so the branch stays clean under warnings-denied typechecking and Clippy.
Extract the shared configuration-error exit conversion used by early diagnostic-mode resolution and full config merging. Keep both callers as fallible combinator chains so the exit handling stays in one place while preserving each function's public shape.
Convert wrapped Mermaid node labels to quoted labels with explicit line breaks so `merman-cli` can parse the affected documentation diagrams.
Remove a duplicated unfenced Rust example that broke Markdown linting after the rebase and keep the conflict-resolved developer guide wrapped in the repository's documented style.
273e983 to
678929e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 678929ea38
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| env: &impl EnvProvider, | ||
| ) -> OrthoResult<bool> { | ||
| let mut diag_json = diag_json_from_file_layers(cli, env)?; | ||
| if let Some(env_diag_json) = diag_json_from_env(env)? { |
There was a problem hiding this comment.
Honour CLI diag-json before env validation
When NETSUKE_DIAG_JSON contains a malformed value but the user supplies a higher-precedence CLI override such as --diag-json, this ? returns a validation error before diag_json_from_matches can apply the command-line value. That makes startup fail even though the CLI layer would otherwise replace the bad environment value for diag_json; check explicit CLI matches before validating the lower-precedence env fallback, or skip this env validation when the CLI value is present.
Useful? React with 👍 / 👎.
Skip parsing `NETSUKE_DIAG_JSON` when a command-line diagnostic JSON selector is already present. The CLI layer has higher precedence, so a malformed lower-precedence environment value must not prevent startup from using `--diag-json` or `--output-format json`. Add regression coverage for both CLI override paths against malformed environment input.
There was a problem hiding this comment.
Gates Failed
Enforce advisory code health rules
(1 file with Code Duplication)
Our agent can fix these. Install it.
Gates Passed
5 Quality Gates Passed
Reason for failure
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| diag.rs | 1 advisory rule | 10.00 → 9.39 | Suppress |
Quality Gate Profile: Pay Down Tech Debt
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
| fn resolve_merged_diag_json_honours_cli_output_format_before_malformed_env() | ||
| -> anyhow::Result<()> { | ||
| let dir = tempdir()?; | ||
| let config_path = dir.path().join("netsuke.toml"); | ||
| std::fs::write(&config_path, "diag_json = false\n")?; | ||
| let cli = Cli::parse_from([ | ||
| "netsuke", | ||
| "--config", | ||
| config_path | ||
| .to_str() | ||
| .expect("temp config path should be UTF-8"), | ||
| "--output-format", | ||
| "json", | ||
| ]); | ||
| let matches = Cli::command().get_matches_from([ | ||
| "netsuke", | ||
| "--config", | ||
| config_path | ||
| .to_str() | ||
| .expect("temp config path should be UTF-8"), | ||
| "--output-format", | ||
| "json", | ||
| ]); | ||
| let env = TestEnv::default().with_var("NETSUKE_DIAG_JSON", "yes"); | ||
|
|
||
| ensure!( | ||
| resolve_merged_diag_json_with_env(&cli, &matches, &env)?, | ||
| "CLI --output-format json should override malformed diagnostic JSON env" | ||
| ); | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
❌ New issue: Code Duplication
The module contains 2 functions with similar structure: tests.resolve_merged_diag_json_honours_cli_diag_json_before_malformed_env,tests.resolve_merged_diag_json_honours_cli_output_format_before_malformed_env
Summary
This branch completes the issue #293 environment-access refactor for early CLI
configuration resolution. It introduces an
EnvProviderport, threads itthrough explicit config selection and early diagnostic JSON resolution, and
removes the remaining direct process-environment dependency from
collect_diag_file_layers.Closes #293.
Review walkthrough
EnvProviderport, theStdEnvProvideradapter, and the provider-aware diagnostic resolver.NETSUKE_DIAG_JSONcoverage.Validation
cargo test --workspace config_merge: passed.make check-fmt: passed.make lint: passed.make test: passed.make markdownlint: passed.make nixie: passed.coderabbit review --agent: completed with zero findings on the main implementation pass.Notes
make fmtwas run after the documentation edit. Its Rust formatting stepcompleted, but the target exits non-zero in the Markdown formatting phase
because the underlying
markdownlint --fixcommand reports repository-wideline-length findings in pre-existing untouched documents. The formatter also
rewrote unrelated Markdown files, so those unrelated edits were restored. The
actual validation gates
make check-fmt,make markdownlint, andmake nixieall pass on this branch.