Roadmap 3.11.4: complete OrthoConfig precedence-ladder regression tests (#385) - #582
Conversation
|
Warning Your free Security trial is over. An organization admin can activate billing to continue. |
|
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
Relates to ADR-004 and issue WalkthroughChangesConfiguration regression coverage
Suggested labels: Poem Poem
🚥 Pre-merge checks | ✅ 19 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (19 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Reviewer's GuideAdds exhaustive regression tests around Netsuke configuration precedence (discovery scopes and full ladder), property-based tests for target merging and scalar merge ordering, and exhaustive display-policy resolution tests, plus roadmap documentation updates to reflect the new coverage and deferred profile handling. Flow diagram for Netsuke configuration precedence and discoveryflowchart TD
A["--config selector"] --> B["NETSUKE_CONFIG selector"]
B --> C["Automatic discovery"]
C --> D["Project configuration"]
D --> E["User configuration"]
E --> F["System configuration"]
F --> G["Default values"]
A -. overrides .-> H["Resolved scalar settings"]
B -. overrides .-> H
C -. contributes .-> H
D -. merges .-> H
E -. merges .-> H
F -. merges .-> H
G -. fallback .-> H
Flow diagram for display-policy resolution precedenceflowchart TD
A["Explicit theme preference"] --> E["Resolved display policy"]
B["Emoji policy"] --> E
C["NO_COLOR"] --> E
D["Output mode"] --> E
A -. highest precedence .-> E
B -. next precedence .-> E
C -. next precedence .-> E
D -. fallback context .-> E
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.
Cover the complete implemented precedence ladder for scalar fields (manifest path, display policies, locale, jobs) from the lowest to the highest layer: - CLI flags > environment > project scope > discovered (system/user) > defaults. Add a hermetic system-scope harness (XDG_CONFIG_DIRS seam) to merge_probe, system-scope discovery cases to config_discovery_scopes, and a unified ladder module with one rstest case per rung plus a closed-selector guard proving NETSUKE_CONFIG_PATH is not a selector. The profile rung is explicitly deferred to roadmap 5.3.1 and never asserted. Progress is tracked in roadmap item 3.11.4.
Hand-written proptest strategies (no derive(Arbitrary)) verify the OrthoConfig layer invariants over generated inputs: - default_targets appends in discovery order (file -> environment -> CLI) whatever the generated layer lengths. - Scalar merge ordering (defaults -> file -> environment -> CLI) holds for generated locale, jobs, and display-policy values. The explicit-CLI build <targets...> replacement asymmetry is asserted by the existing merge.rs command-layer tests, which exercise a parsed command that a pure MergeComposer composition cannot observe. All strategies stay free of any second environment selector.
…num domain Verifies the consolidated theme/output_prefs display-policy resolution over the full domain of EmojiPolicy, ProgressPolicy, AccessibilityPolicy, ColourPolicy, OutputMode and json combinations, using a deterministic domain sweep against a hand-written truth model, output-mode/TERM sweep, and a probabilistic proptest. Coverage-only; no production logic changes. Part of issue #385 (roadmap 3.11.4).
Tick the precedence-expectation and CLI-override sub-items for the layers now covered by the regression tests in issue #385. Keep profile-selection portions deferred to 5.3.1 (--profile flag) and the OrthoConfig 5.2.3 sub-item blocked on upstream guidance (5.2.3 is an upstream roadmap identifier, not a crate version; Cargo.toml stays at 0.9.0). Part of issue #385.
Route fixture file I/O through test_support::fs instead of std::fs to satisfy the Whitaker capability policy, flatten nested conditional clusters in ladder_environment and expected_emoji_allowed, and trim modules to the 400-line ceiling. Also fix spelling (handwritten) so the prose gate passes. Part of #385.
72d7cdf to
dc61559
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.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
@coderabbitai based on the current codegraph, is this PR a correct and complete implementation of the associated issue (see below)? If further work is required, please provide an AI agent prompt for the remaining work to be done to address these failures.
|
This comment was marked as resolved.
This comment was marked as resolved.
display_policy_domain.rs: replace the five-level nested loop in exhaustive_domain_sweep_matches_truth_model with a flat Cartesian-product iterator (itertools::iproduct over the finite policy domain), preserving deterministic coverage of all 3x3x3x3x2 - 162 cases and the single assert_consolidated assertion path per case. config_discovery_scopes.rs: merge the byte-identical Unix and Windows system_scope_config_discovered_when_no_user_or_project_config bodies into one platform-neutral test. Both bodies used only platform-neutral helpers (run_system_scope_scenario + assert_system_config_applied), so platform coverage is preserved. Part of #385.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
The Whitaker 0.2.7 suite enforces module-max-lines at 400. Condense the module doc (drop the stale output_mode_inference_matches_truth_model reference; the TERM/output-mode dimension is covered by the proptest) and tighten the all_domain_cases helper doc so the module fits under 400 lines without changing coverage or behaviour. Part of #385.
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.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![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.
![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.
Sorry, something went wrong.
Resolve the CodeScene review findings on the issue-385 regression tests without suppressing rules or changing production behaviour: - roadmap: replace the 'full merge ladder' phrasing with the exclusive user-over-system discovery semantics (a user-scope win drops system-only fields to defaults) and add the required type:docstyle triage note. - config_precedence_ladder: point NETSUKE_CONFIG_PATH at a real existing legacy config carrying distinct values, so the guard also rejects implementations that would open an existing legacy path. - display_policy_domain: extend DomainCase with the json field, thread it through the fixtures, and assert cli.json per case; exhaustive sweep is now 3x3x3x3x2x2 = 324 cases. - Gate XDG-only ladder/system-scope harness and tests to Unix (Windows discovers config through APPDATA/LOCALAPPDATA, not XDG). - developers-guide: document the config-precedence test modules. Part of issue #385 (roadmap 3.11.4).
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 resolved.
This comment was marked as resolved.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Pull request check state =='
gh pr view 582 --repo leynos/netsuke \
--json state,updatedAt,statusCheckRollup,files \
--jq '{
state,
updatedAt,
checks: [.statusCheckRollup[] | {
name,
status,
conclusion,
detailsUrl,
workflowName
}],
files: [.files[].path]
}'
echo
echo '== Developer-guide change in PR =='
gh pr diff 582 --repo leynos/netsuke --patch -- docs/developers-guide.md || true
echo
echo '== Relevant developer-guide sections =='
if [ -f docs/developers-guide.md ]; then
rg -n -i -C 5 \
'(test strategy|cli test|configuration|precedence|system.scope|display.policy|property|proptest|environment|EnvProvider|environment_with_system_scope|child.process)' \
docs/developers-guide.md || true
fi
echo
echo '== Current changed test and helper outlines =='
for file in \
tests/cli_tests/config_precedence_ladder.rs \
tests/cli_tests/config_discovery_scopes.rs \
tests/cli_tests/display_policy_domain.rs \
tests/cli_tests/merge_targets_proptests.rs \
tests/cli_tests/merge_probe.rs; do
if [ -f "$file" ]; then
echo "-- $file"
ast-grep outline "$file" --items all
fi
done
echo
echo '== Documentation references to the added modules and helper =='
rg -n -C 2 \
'(config_precedence_ladder|config_discovery_scopes|display_policy_domain|merge_targets_proptests|environment_with_system_scope)' \
docs tests/cli_tests || trueLength of output: 116610
Treat the Developer Documentation warning as resolved.
It documents:
All completed checks now pass. This includes Do not create a follow-up issue. Do not perform further work for this warning. You are interacting with an AI system. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Review findings on the issue-385 regression tests: - roadmap: drop the literal [type:docstyle] marker and internal triage framing from the 3.11.4 note, keeping the exclusive-discovery explanation as normal prose. - display_policy_domain: vary term_dumb alongside no_color and json in the flat iproduct! matrix, mapping the generated value into DomainCase instead of hardcoding false, and bump the documented case count from 324 to 648. Construct DomainCase fields in struct-definition order to satisfy clippy::inconsistent-struct-constructor, and tighten doc comments so the file stays under the module-max-lines ceiling. Part of issue #385 (roadmap 3.11.4).
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
2 participants
Closes #385
Summary
Completes roadmap item 3.11.4 (issue #385) with exhaustive regression coverage of the Netsuke config precedence ladder, following the two-selector framing recorded in ADR-004 (
--config>NETSUKE_CONFIG> automatic discovery). No second environment selector is introduced, asserted, or assumed anywhere.Task 1 — Full-ladder scalar precedence regression tests
tests/cli_tests/merge_probe.rs: hermetic system-scope config helper via the closed discovery env set (isolated system-configuration directory), reusing the fabricatedHOME/XDG_CONFIG_HOMEisolation; no parent-process env mutation; no second selector.tests/cli_tests/config_discovery_scopes.rs: rstest cases proving system-scope discovery, user-over-system precedence, and system-only merge-through for manifest path, emoji policy, locale, and jobs.tests/cli_tests/config_precedence_ladder.rs(new): unified end-to-end ladder test — CLI > environment > project > user > system > default, with per-rung#[case]seeding; deferred profile-rung marker for roadmap 5.3.1; closed-selectorNETSUKE_CONFIG_PATHguard asserting the legacy variable has no effect.Task 2 — Property-based merge-ordering and list-append tests
tests/cli_tests/merge_targets_proptests.rs(new): hand-writtenpropteststrategies (noderive(Arbitrary)) verifying merge ordering (defaults > file > environment > CLI) anddefault_targetsappend-in-discovery-order semantics, plus the documented explicit-build <targets...>replace-over-append asymmetry.Task 3 — Display-policy resolution coverage across the enum domain
tests/cli_tests/display_policy_domain.rs(new): deterministic exhaustive sweep of everyEmojiPolicyxProgressPolicyxAccessibilityPolicyxColourPolicyxOutputModexjsoncombination (-xNO_COLORtoggle) against a hand-written truth model, an output-mode/TERMsweep, and a proptest — verifying theme/output-prefs precedence (explicit theme preference > emoji policy >NO_COLOR> output mode) without adding conflict rules.src/theme.rsandsrc/output_prefs.rsuntouched.Task 4 — Dependency-boundary docs + roadmap alignment
docs/roadmap.mditem 3.11.4 ticked for precedence expectations and CLI-override layers now covered; profile-selection deferred to 5.3.1; OrthoConfig 5.2.3 documented as an upstream roadmap identifier (not a crate version) and left blocked on upstream —Cargo.tomlunchanged (stays atortho_config = 0.9.0).Guard files
src/cli/config_path_precedence_tests.rsandsrc/cli/discovery_tracing_tests.rs::legacy_config_path_variable_is_not_a_selectorremain unmodified and passing.Verification
cargo fmt --checkandcargo clippy --workspace --all-targets --all-features -- -D warnings: clean.cargo nextest run --workspace: 2315 tests passed, 0 failures (3 environment-dependent skips).--agentreview: 0 findings across 3 passes (7 files).Commits:
8faa7102(Task 1),87eb5ea7(Task 2),7423836a(Task 3),70f70155(Task 4).References
Summary by Sourcery
Complete roadmap item 3.11.4 by formalizing Netsuke’s configuration precedence and display-policy behavior through comprehensive regression and property-based tests.
Enhancements:
Documentation:
Tests: