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
WalkthroughConfiguration discovery now uses injected environment access, cached outcomes, deferred diagnostics, normalised project matching, and timed telemetry. CLI merging reuses discovered layers. Tests and documentation cover the public API, precedence rules, metrics, and migration path. ChangesConfiguration discovery and observability
Sequence Diagram(s)sequenceDiagram
participant CLI
participant resolve_json_preference
participant discover_file_layers_timed
participant DiscoveryOutcome
participant merge_with_config_and_env
CLI->>resolve_json_preference: resolve diagnostic mode
resolve_json_preference->>discover_file_layers_timed: discover configuration layers
discover_file_layers_timed-->>DiscoveryOutcome: return layers and diagnostics
resolve_json_preference-->>CLI: retain cached discovery outcome
CLI->>merge_with_config_and_env: merge cached layers
merge_with_config_and_env-->>CLI: return merged configuration
Suggested labels: Suggested reviewers: Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (19 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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 |
This comment was marked as resolved.
This comment was marked as resolved.
79f2d41 to
7847192
Compare
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 review |
✅ Action performedReview finished.
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
52bbe8a to
601eaee
Compare
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 review |
✅ Action performedReview finished.
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@coderabbitai review |
|
|
@coderabbitai review |
|
Move discovery instrumentation to the startup composition boundary, where the clock is injected, and preserve a regression test for the public merge API. Consolidate the design guide's repeated Ninja synthesis material and relocate unstable Rust composition guidance from the users' guide.
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 Have the following now been resolved? ❌ Failed check (1 error)
|
|
@coderabbitai Have the following now been resolved? docs/users-guide.md (1)
gives the reason for rejection. The clause is essential, so do not separate it with a comma. 🤖 Detailed instructionsUse a wyvern agent team to verify each finding against the current code and only fix it if needed. Fix any documentation issues using a scribe agent team. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. Use a scrutineer agent for execution of tests and summarizing the results. In Sources: Path instructions, Linters/SAST tools docs/developers-guide.md (1)
the design and developer documentation describe one contract.
synchronized with the implementation. 🤖 Detailed instructionsUse a wyvern agent team to verify each finding against the current code and only fix it if needed. Fix any documentation issues using a scribe agent team. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. Use a scrutineer agent for execution of tests and summarizing the results. In Source: Coding guidelines |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- revision ---'
git rev-parse --short HEAD
printf '%s\n' '--- users guide: reported grammar passage ---'
sed -n '1158,1178p' docs/users-guide.md
printf '%s\n' '--- developers guide: reported discovery passage ---'
sed -n '2378,2400p' docs/developers-guide.md
printf '%s\n' '--- design guide: reported discovery passage ---'
sed -n '2765,2792p' docs/netsuke-design.md
printf '%s\n' '--- current and obsolete discovery API references ---'
rg -n -C3 \
'\b(collect_diag_file_layers_with_sources|collect_diag_file_layers_with_env|push_file_layers_with_sources|discover_file_layers|DiscoveryOutcome)\b' \
src docs tests
printf '%s\n' '--- discovery module outline ---'
ast-grep outline src/cli/discovery.rs --items all --view expandedLength of output: 35196 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- nested-eval wording in users guide ---'
rg -n -i -C4 \
'nested eval|cannot safely determine which jobs to wait for|because the wrapper' \
docs/users-guide.md
printf '%s\n' '--- obsolete helper names in target guides ---'
rg -n -C2 \
'\b(collect_diag_file_layers_with_sources|push_file_layers_with_sources)\b' \
docs/users-guide.md docs/developers-guide.md docs/netsuke-design.md || true
printf '%s\n' '--- current helper names and DiscoveryOutcome hand-off in target guides ---'
rg -n -C2 \
'\b(collect_diag_file_layers_with_env|discover_file_layers|DiscoveryOutcome|into_layers|merge_with_cached_file_layers)\b' \
docs/developers-guide.md docs/netsuke-design.mdLength of output: 12400
The source matches the documentation. You are interacting with an AI system. |
|
@coderabbitai Have the following now been resolved? tests/logging_stderr/config_tracing.rs (1)
classifier maps for the missing explicit configuration case. 🤖 Detailed instructionsUse a wyvern agent team to verify each finding against the current code and only fix it if needed. Fix any documentation issues using a scribe agent team. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. Use a scrutineer agent for execution of tests and summarizing the results. In |
|
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 and working tree ---'
git rev-parse --short HEAD
git status --short
printf '%s\n' '--- merge module outline ---'
ast-grep outline src/cli/merge.rs --items all --view expanded
printf '%s\n' '--- public merge API and discovery call path ---'
rg -n -A100 -B25 \
'pub\s+fn\s+merge_with_config_and_env\b|fn\s+merge_with_config_and_env\b|discover_file_layers(?:_timed)?\b|StdMonotonicClock|record_discovery_outcome|timed_discovery' \
src/cli/merge.rs src/cli/discovery.rs src/cli/discovery_telemetry.rs src/cli/mod.rs
printf '%s\n' '--- merge API callers and regression tests ---'
rg -n -A20 -B10 \
'\bmerge_with_config_and_env\s*\(' \
src tests benches
printf '%s\n' '--- PR diff for affected implementation ---'
base="$(git merge-base HEAD origin/main)"
git diff --unified=30 "$base" HEAD -- \
src/cli/merge.rs src/cli/discovery.rs src/cli/discovery_telemetry.rs src/cli/mod.rsLength of output: 58492
Mark the Unit Architecture check as resolved at current revision
You are interacting with an AI system. |
|
@coderabbitai Have the following now been resolved? docs/users-guide.md (1)
gives the reason for rejection. The clause is essential, so do not separate it with a comma. 🤖 Detailed instructionsUse a wyvern agent team to verify each finding against the current code and only fix it if needed. Fix any documentation issues using a scribe agent team. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. Use a scrutineer agent for execution of tests and summarizing the results. In Sources: Path instructions, Linters/SAST tools docs/developers-guide.md (1)
the design and developer documentation describe one contract.
synchronized with the implementation. 🤖 Detailed instructionsUse a wyvern agent team to verify each finding against the current code and only fix it if needed. Fix any documentation issues using a scribe agent team. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. Use a scrutineer agent for execution of tests and summarizing the results. In Source: Coding guidelines |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- revision and worktree ---'
git rev-parse --short HEAD
git status --short
printf '%s\n' '--- target test module map ---'
ast-grep outline tests/logging_stderr/config_tracing.rs --items all --view expanded
printf '%s\n' '--- reported expectation and nearby failure assertions ---'
sed -n '140,195p' tests/logging_stderr/config_tracing.rs
rg -n -A30 -B12 \
'ConfigLoadFailureExpectation|diag_mode_resolution|error_category|missing.*config|missing\.toml' \
tests/logging_stderr/config_tracing.rs
printf '%s\n' '--- current discovery error classifier ---'
rg -n -A50 -B12 \
'fn error_category\b|OrthoError::File|error_category' \
src/cli/discovery_telemetry.rs src/cli src
printf '%s\n' '--- relevant tracing and test expectations ---'
rg -n -A24 -B10 \
'diag_mode_resolution|configuration load failed|ConfigLoadFailureExpectation' \
src testsLength of output: 182814
Withdraw this finding. I validated it against current revision Keep The finding conflates two different telemetry contracts:
The test also excludes the terminal event from You are interacting with an AI system. |
|
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 and worktree ---'
git rev-parse --short HEAD
git status --short
printf '%s\n' '--- users guide: nested eval wording ---'
rg -n -i -C4 \
'nested eval|cannot safely determine which jobs to wait for|because the wrapper' \
docs/users-guide.md
printf '%s\n' '--- developers guide: reported discovery passage ---'
sed -n '2378,2400p' docs/developers-guide.md
printf '%s\n' '--- design guide: reported discovery passage ---'
sed -n '2765,2795p' docs/netsuke-design.md
printf '%s\n' '--- obsolete discovery names in documentation ---'
rg -n -C2 \
'\b(collect_diag_file_layers_with_sources|push_file_layers_with_sources)\b' \
docs || true
printf '%s\n' '--- current discovery API names and hand-off in docs and source ---'
rg -n -C2 \
'\b(collect_diag_file_layers_with_env|discover_file_layers|DiscoveryOutcome|into_layers|merge_with_cached_file_layers)\b' \
docs/developers-guide.md docs/netsuke-design.md src/cli/discovery.rs src/cli/merge.rsLength of output: 19397
Keep the current documentation. Do not make further changes. The only remaining You are interacting with an AI system. |
Capture bounded discovery-span fields through the common test tracing subscriber while preserving its event capture behaviour. This removes duplicated test layers and keeps startup and telemetry checks aligned.
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.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
4 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 discovered configuration layers between diagnostic resolution and full configuration merging while preserving existing standalone callers and precedence behavior.
New Features:
Bug Fixes:
Enhancements:
Build:
Documentation:
Tests:
Chores: