Cache config file layer discovery (#319) - #548
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Summary
WalkthroughThe CLI now uses ChangesConfiguration discovery and environment abstraction
Sequence Diagram(s)sequenceDiagram
participant main
participant diag
participant discovery
participant merge
participant config_files
main->>diag: resolve JSON mode and layers
diag->>discovery: discover configuration
discovery->>config_files: load configuration layers
config_files-->>discovery: return loaded layers
discovery-->>diag: return DiscoveredLayers
diag-->>main: return JSON mode and DiscoveredLayers
main->>merge: merge with DiscoveredLayers
merge-->>main: return merged CLI configuration
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 4 warnings, 1 inconclusive)
✅ Passed checks (14 passed)
📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Reviewer's GuideRefactors CLI configuration discovery and merge to cache file-backed config layers discovered in a single pre-pass driven by a generic Env interface, then reuse those layers for diagnostics and full merge; replaces custom EnvProvider with mockable::Env/DefaultEnv, adjusts JSON/merge flows and tests to use the cached DiscoveredLayers and the mockable test helpers. Sequence diagram for cached config layer discovery and mergesequenceDiagram
actor User
participant Main as main_rs
participant Diag as cli_diag
participant Discovery as cli_discovery
participant Merge as cli_merge
participant Env as DefaultEnv
User ->> Main: run_with_args
Main ->> Diag: resolve_diag_mode_or_exit(parsed_cli, matches, fallback_mode)
Diag ->> Diag: resolve_json_and_layers_with_env(cli, matches, Env)
Diag ->> Discovery: collect_diag_file_layers_with_env(cli, Env)
Discovery ->> Discovery: discover_file_layers(cli, Env)
Discovery ->> Env: resolve_config_selector(cli.config, Env)
Discovery -->> Diag: DiscoveredLayers
Diag ->> Diag: json_from_layers(DiscoveredLayers.layers())
Diag ->> Env: json_from_env(Env)
Diag -->> Main: (DiagMode, DiscoveredLayers)
Main ->> Discovery: DiscoveredLayers.replay_config_path_trace()
Main ->> Merge: merge_cli_or_exit(parsed_cli, matches, DiagMode, DiscoveredLayers)
Merge ->> Merge: merge_with_layers(cli, matches, Env, DiscoveredLayers)
Merge ->> Discovery: push_discovered_file_layers(composer, errors, DiscoveredLayers)
Merge ->> Env: Env.all()
Merge ->> Merge: Figment::from(EnvironmentLayer::new(env_entries))
Merge -->> Main: merged Cli
Main -->> User: exit code / program outcome
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-access[bot]](https://avatars.githubusercontent.com/in/3755875?s=60&v=4){kind=small}
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 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. src/cli/discovery_layer_tests.rs Comment on lines +201 to +219 fn discover_file_layers_records_an_explicit_load_error() -> Result<()> {
let dir = tempdir().context("create temporary config directory")?;
let cli = Cli {
config: Some(dir.path().join("missing.toml")),
..Cli::default()
};
let discovered = discover_file_layers(&cli, &empty_mock_env());
ensure!(
discovered.layers().is_empty(),
"a missing explicit config should not produce layers"
);
ensure!(
discovered.errors.len() == 1,
"a missing explicit config should record one error"
);
Ok(())
}❌ New issue: Code Duplication |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
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.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/bdd/helpers/config_environment.rs`:
- Around line 13-29: Update environment_from_world so selector_values is built
directly from env_vars_forward, preserving raw OsString values for
expect_os_string; retain the UTF-8-filtered values map exclusively for
expect_all. Add Unix-specific coverage verifying that a non-UTF-8 NETSUKE_CONFIG
path is returned by Env::os_string rather than treated as unset.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 296d6fcc-282c-4b8d-aee3-3dfdbcd4d84d
📒 Files selected for processing (13)
Cargo.tomlsrc/cli/config_path_precedence_tests.rssrc/cli/diag.rssrc/cli/discovery.rssrc/cli/discovery_layer_tests.rssrc/cli/discovery_layers.rssrc/cli/discovery_tracing_tests.rssrc/cli/merge.rssrc/cli/mod.rssrc/cli/test_support.rssrc/main.rstests/bdd/helpers/config_environment.rstests/cli_tests/merge_diag.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/rstest-bdd(auto-detected)leynos/ortho-config(auto-detected)leynos/whitaker(auto-detected)leynos/shared-actions(auto-detected)
| fn environment_from_world(world: &TestWorld) -> MockEnv { | ||
| let values = world | ||
| .env_vars_forward | ||
| .borrow() | ||
| .iter() | ||
| .filter_map(|(key, raw_value)| { | ||
| raw_value | ||
| .to_str() | ||
| .map(|text| (key.clone(), text.to_owned())) | ||
| }) | ||
| .collect::<HashMap<_, _>>(); | ||
| let selector_values = values.clone(); | ||
| let mut env = MockEnv::new(); | ||
| env.expect_os_string() | ||
| .returning(move |key| selector_values.get(key).map(OsString::from)); | ||
| env.expect_all().return_const(values); | ||
| env |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -eu
printf '%s\n' '--- target helper ---'
cat -n tests/bdd/helpers/config_environment.rs
printf '%s\n' '--- relevant symbols ---'
rg -n -C 3 'MockEnv|os_string|NETSUKE_CONFIG|config.*discover|discover.*config|selector' . \
-g '*.rs' -g '*.toml' -g '*.md' | head -n 300Repository: leynos/netsuke
Length of output: 22659
🏁 Script executed:
set -eu
printf '%s\n' '--- helper structure ---'
ast-grep outline tests/bdd/helpers/config_environment.rs
printf '%s\n' '--- MockEnv definitions and implementations ---'
rg -n -C 8 'struct MockEnv|impl MockEnv|trait Env|fn os_string|fn all' . \
-g '*.rs' | head -n 400
printf '%s\n' '--- selector and environment call sites ---'
rg -n -C 6 'NETSUKE_CONFIG|os_string\(|\.all\(\)|Env::' . \
-g '*.rs' | head -n 500Repository: leynos/netsuke
Length of output: 48919
🏁 Script executed:
set -eu
printf '%s\n' '--- configuration selector resolver ---'
cat -n src/cli/discovery.rs | sed -n '140,245p'
printf '%s\n' '--- discovery callers and environment injection ---'
rg -n -C 8 'resolve_config_selector|discover.*config|merge_with_config_and_env|environment_from_world|env_vars_forward' \
src tests -g '*.rs' | head -n 500
printf '%s\n' '--- TestWorld definition and environment steps ---'
rg -n -C 10 'struct TestWorld|env_vars_forward|NETSUKE_CONFIG' tests -g '*.rs' | head -n 500
printf '%s\n' '--- dependency declarations ---'
rg -n -C 3 'mockable' Cargo.toml Cargo.lock tests test_support -g '*.toml' -g '*.rs' | head -n 250Repository: leynos/netsuke
Length of output: 50370
🏁 Script executed:
set -eu
python3 - <<'PY'
from pathlib import Path
source = Path("tests/bdd/helpers/config_environment.rs").read_text()
assert "raw_value.to_str()" in source
assert "selector_values = values.clone()" in source
# Model the exact transformation used by the helper on Unix, where arbitrary
# bytes can represent an OsString. The result must not equal the original.
raw = b"/tmp/config-\xff.toml"
decoded = raw.decode("utf-8", errors="strict") if False else None
filtered = None
try:
filtered = raw.decode("utf-8")
except UnicodeDecodeError:
filtered = None
reconstructed = filtered.encode() if filtered is not None else None
print({
"raw_value": raw,
"to_str_result": filtered,
"os_string_value_returned_by_current_fixture": reconstructed,
"value_lost": reconstructed != raw,
})
PY
printf '%s\n' '--- exact environment APIs in checked-in sources ---'
rg -n -C 5 'pub trait Env|trait Env|fn all|fn os_string|expect_all|expect_os_string' \
. -g '*.rs' -g '*.toml' | head -n 300Repository: leynos/netsuke
Length of output: 240
🏁 Script executed:
set -eu
python3 - <<'PY'
from pathlib import Path
# Reproduce the helper's transformation for a Unix OsString containing an
# invalid UTF-8 byte. The fixture filters this value before configuring
# `expect_os_string`.
raw_value = b"/tmp/config-\xff.toml"
utf8_value = None
try:
utf8_value = raw_value.decode("utf-8")
except UnicodeDecodeError:
pass
returned_by_current_fixture = (
utf8_value.encode("utf-8") if utf8_value is not None else None
)
print(f"raw_value={raw_value!r}")
print(f"to_str={utf8_value!r}")
print(f"os_string_returned_by_current_fixture={returned_by_current_fixture!r}")
print(f"selector_value_preserved={returned_by_current_fixture == raw_value}")
assert utf8_value is None
assert returned_by_current_fixture is None
assert returned_by_current_fixture != raw_value
PY
printf '%s\n' '--- helper call sites ---'
rg -n -C 4 'merge_with_world_env|track_env_var' tests/bdd -g '*.rs' | head -n 300
printf '%s\n' '--- BDD feature references to NETSUKE_CONFIG ---'
rg -n -C 3 'NETSUKE_CONFIG|configuration file|config path' tests -g '*.feature' -g '*.rs' | head -n 300Repository: leynos/netsuke
Length of output: 28433
Preserve raw OsString values for Env::os_string.
Build selector_values from env_vars_forward before the UTF-8 filter. Keep the filtered map only for Env::all. Add Unix coverage for a non-UTF-8 NETSUKE_CONFIG path, which the current filter treats as unset.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/bdd/helpers/config_environment.rs` around lines 13 - 29, Update
environment_from_world so selector_values is built directly from
env_vars_forward, preserving raw OsString values for expect_os_string; retain
the UTF-8-filtered values map exclusively for expect_all. Add Unix-specific
coverage verifying that a non-UTF-8 NETSUKE_CONFIG path is returned by
Env::os_string rather than treated as unset.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c81eefbbde
ℹ️ 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".
| let environment_entries = env | ||
| .all() | ||
| .into_iter() | ||
| .map(|(key, value)| (key.into(), value.into())) | ||
| .collect(); |
There was a problem hiding this comment.
On Unix, if the process inherits any non-UTF-8 environment key or value, DefaultEnv::all() enumerates into Unicode Strings and can panic before this conversion runs. Previously vars_os() preserved these entries so EnvironmentLayer::parse_entry could reject Netsuke-prefixed entries with its fixed error and skip unrelated ones; converting the already-Unicode map back into OsString makes that handling unreachable. Keep the production environment behind the sanctioned injected seam while retaining an OsString snapshot.
AGENTS.md reference: AGENTS.md:L300-L306
Useful? React with 👍 / 👎.
| Err(err) => { | ||
| let fallback_filter = startup_filter(fallback_mode, parsed_cli.verbose); | ||
| set_tracing_filter(fallback_filter); | ||
| // Resolution failed before its diagnostics could be emitted. Replay | ||
| // only for human output after enabling its filter; JSON remains OFF. | ||
| if fallback_filter != LevelFilter::OFF { | ||
| drop(cli::resolve_merged_json(parsed_cli, matches)); | ||
| } | ||
| Err(config_err_to_exit(err.as_ref(), fallback_mode)) |
There was a problem hiding this comment.
When discovery or NETSUKE_JSON validation fails with --verbose, the first discovery pass emitted its debug/trace events while the subscriber was still limited to WARN, and this error arm enables TRACE but returns without replaying the cached resolution. Consequently failed runs no longer show which selector won or which environment lookup occurred, even though successful runs replay those events and the users' guide promises that diagnostic context; preserve the resolution on the error path and replay it after changing the filter.
AGENTS.md reference: AGENTS.md:L358-L362
Useful? React with 👍 / 👎.
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 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. tests/logging_stderr/config_tracing.rs Comment on lines +136 to +139 ensure!(
joined.contains("resolved config path") && joined.contains("selector=\"cli_flag\""),
"verbose stderr should replay the cached selector decision: {joined}"
);❌ New issue: Code Duplication |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- current revision ---'
git rev-parse --short HEAD
printf '%s\n' '--- module outline ---'
ast-grep outline tests/logging_stderr/config_tracing.rs --items all --view expanded
printf '%s\n' '--- target tests and local helpers ---'
rg -n -A70 -B20 \
'^\s*(async\s+)?fn\s+(explicit_selection_traces_bounded_fields|invalid_config_traces_without_parser_text)\b|^\s*fn\s+[A-Za-z0-9_]+.*\{' \
tests/logging_stderr/config_tracing.rs
printf '%s\n' '--- target names across relevant test sources ---'
rg -n -A8 -B8 \
'\b(explicit_selection_traces_bounded_fields|invalid_config_traces_without_parser_text)\b' \
tests srcLength of output: 12015
Suppress this CodeScene diagnostic. I validated the finding against current revision Do not merge these tests. A helper would only hide the short command execution and diagnostic collection sequence. The test setup, configuration content, and assertions must remain separate. Parameterization would combine independent success and failure contracts and reduce failure localization. The shared structure is intentional end-to-end test scaffolding. It is not duplicated production logic. CodeScene diagnostic suppression text:
You are interacting with an AI system. |
Replace the bespoke config-selector environment trait with `mockable::Env` so discovery and merging use the established injectable seam. Keep automatic discovery from re-reading `NETSUKE_CONFIG` after that injected lookup, and adapt deterministic unit, integration, and BDD coverage to `MockEnv`.
Discover file-backed configuration layers once during diagnostic resolution and pass that result into the full merge. This preserves existing standalone merge behaviour while removing repeated startup file loading. Keep verbose selector tracing by replaying the cached decision after the diagnostic output mode enables the tracing filter. Cover the shared flow with a mock environment that permits one config-selector lookup only.
Keep non-Unicode configuration selectors and process environment entries in their raw form so selection remains correct and environment-layer validation can apply its documented policy without a startup panic. Retain cached selector diagnostics through failed startup resolution, so verbose users receive the same bounded context for configuration errors.
79f2d41 to
7847192
Compare
There was a problem hiding this comment.
Gates Failed
(1 file with Code Duplication)
Our agent can fix these. Install it.
Gates Passed
Reason for failure
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| config_tracing.rs | 1 advisory rule | 10.00 → 9.39 | Suppress |
Active suppressions
1 suppression
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.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 78471925eb
ℹ️ 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".
| pub use diag::{ | ||
| resolve_json_and_layers_outcome_with_env, resolve_json_and_layers_with_env, | ||
| resolve_merged_json, resolve_merged_json_with_env, | ||
| }; | ||
| pub use discovery::DiscoveredLayers; |
There was a problem hiding this comment.
Replacing the exported ConfigEnvProvider/ConfigStdEnvProvider seam with DiscoveredLayers and the new resolve/merge pair leaves docs/developers-guide.md lines 2257–2309 describing removed types and signatures, so its adapter examples no longer compile and the new abstraction's ownership and reuse contract is absent. Preserve compatibility or update the relevant developer/design documentation in this change.
AGENTS.md reference: AGENTS.md:L44-L56
Useful? React with 👍 / 👎.
| pub fn replay_config_path_trace(&self) { | ||
| trace_config_path_resolution(&self.resolution); |
There was a problem hiding this comment.
When --verbose is supplied, the discovery pre-pass runs while the subscriber is still filtered at WARN, and this method later replays only the selector and environment-lookup events. The using explicit config path, using config discovery, and project-layer debug events emitted during collection therefore remain discarded, even though the users' guide promises that verbose configuration diagnostics identify whether explicit or discovered layers were used; cache and replay those bounded decisions as well.
Useful? React with 👍 / 👎.
2 participants
Summary
This branch discovers file-backed configuration layers once during diagnostic
mode resolution and passes the loaded result to the full configuration merge.
It preserves standalone merge callers, selector precedence, and verbose
selector tracing without a second environment lookup or filesystem load.
The regression test drives the diagnostic and merge phases with one
mockable::MockEnv, requiring exactly oneNETSUKE_CONFIGlookup whileconfirming that configuration values remain merged.
Closes #319.
Validation
make check-fmtmake test(1,915 tests passed; 1 skipped; doctests passed)make lintmake typecheckcoderabbit review --agent(0 findings)References
Summary by Sourcery
Cache configuration file layer discovery so diagnostic JSON resolution and full configuration merge share a single environment-driven discovery pass.
New Features:
Bug Fixes:
Enhancements:
Build:
Tests: