Narrow the build script's module graph (#513) - #514
Conversation
`build.rs` recompiles part of the library so it can call `cli::Cli::command()` for man-page generation. It did so by declaring `src/cli/mod.rs`, which dragged in the entire `cli` subtree — merging, discovery, diagnostics, localized value parsing — plus `cli_l10n`, `host_pattern`, `output_mode`, and `theme`. Almost none of that is reachable from the build script, so five module-wide `#[expect(dead_code, ...)]` attributes were needed to silence 110 unused-item diagnostics. Those suppressions were not merely noisy. They also masked genuinely dead code: an unused `pub` item in `src/cli/config.rs` was reported by neither compilation unit, because the library exports that module publicly and the build script's expectation covered the rest. Replace the five declarations with a facade: an inline `cli` module in `build.rs` naming exactly the three files the Clap schema needs. To make that slice self-contained, split the library along the same seam: - `src/cli/command.rs` holds the Clap definitions, previously the top half of `src/cli/parser.rs`. `src/cli/parser.rs` keeps the localization-aware parsing entry point. - `src/cli/preferences.rs` holds the four `Cli` accessors that resolve output policy, which were the only reason the build script compiled `theme` and, transitively, `output_mode`. - `src/cli/validation.rs` holds `MAX_JOBS` and `validation_error`, so `src/cli/config.rs` no longer reaches up into `src/cli/mod.rs`. - `src/host_matching.rs` holds `HostCandidate` and `HostPattern::matches`, the only items in `src/host_pattern.rs` the schema does not need. All five expectations are removed and the build script compiles with no unused-item diagnostics. The generated man page is byte-identical, the public API is unchanged, and no dependency was added. Rerun directives now track the modules actually compiled. Adding a dependency outside the slice surfaces as a build-script compile error, which is the intended signal; `docs/developers-guide.md` records the boundary. Closes #513 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
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
Validation
WalkthroughThe PR separates CLI command, preference, and validation responsibilities, narrows ChangesCLI command and validation
Build-script module slice
Host matching
Sequence Diagram(s)sequenceDiagram
participant build.rs
participant Cli
participant localization
build.rs->>Cli: construct command schema
Cli->>localization: resolve localisation data
localization-->>build.rs: provide command text
build.rs-->>build.rs: generate man page
Possibly related PRs
Suggested labels: Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 5 warnings)
✅ Passed checks (14 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Reviewer's GuideRefactors the CLI and host pattern modules to carve out a minimal, self-contained slice that the build script recompiles for man-page and localization audits, removing broad dead-code suppressions while keeping behavior and public API unchanged. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@build.rs`:
- Around line 31-35: Update the boundary documentation in build.rs lines 31-35
to state that src/cli/command.rs contains command-schema and default-command
behavior, including Cli::with_default_command; do not move the method.
Synchronize the corresponding boundary statement in docs/developers-guide.md
lines 391-393 with the same wording and scope.
In `@docs/developers-guide.md`:
- Around line 382-384: Reconcile the build-script description in
docs/developers-guide.md with the generate_man_page call and the statement that
build.rs only performs localization auditing. Explicitly state whether build.rs
stages a man page or cargo-orthohelp is the sole generator, then update the
related maintenance guidance so it presents one consistent rule and preserves
docs/ as the source of truth.
In `@src/cli/command.rs`:
- Around line 117-124: Add Rustdoc usage and outcome examples for each affected
public/shared function: in src/cli/command.rs lines 117-124, document
with_default_command() selecting Commands::Build when command is None; in
src/cli/preferences.rs lines 14-44, document each policy-to-preference mapping
with examples; and in src/cli/validation.rs lines 15-20, describe the produced
OrthoError::Validation and show caller context.
In `@src/host_matching.rs`:
- Around line 21-23: Expand the documentation for HostPattern::matches with a #
Examples section demonstrating an exact host match, a wildcard subdomain match,
and rejection of the wildcard apex; show the expected boolean outcomes for each
case while preserving the existing implementation.
- Around line 23-35: Update HostPattern::matches to remove one trailing DNS dot
from the lowercased candidate hostname before applying exact or wildcard
matching. Preserve the existing wildcard subdomain-only behavior after
normalization, and add regression coverage for trailing-dot hosts against both
exact and wildcard patterns.
🪄 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: 74c58235-8cae-4681-8a68-6c1908f2098a
📒 Files selected for processing (17)
build.rsdocs/developers-guide.mddocs/netsuke-design.mdsrc/cli/command.rssrc/cli/config.rssrc/cli/diag.rssrc/cli/discovery.rssrc/cli/merge.rssrc/cli/mod.rssrc/cli/parser.rssrc/cli/parsing.rssrc/cli/preferences.rssrc/cli/validation.rssrc/host_matching.rssrc/host_pattern.rssrc/lib.rssrc/stdlib/network/policy/mod.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/shared-actions(auto-detected)
| // The library modules below are laid out to keep this slice small: | ||
| // `src/cli/command.rs` holds definitions only, with runtime behaviour in | ||
| // `src/cli/preferences.rs` and `src/cli/parser.rs`; matching logic is split out | ||
| // of `src/host_pattern.rs` into `src/host_matching.rs`. Adding a dependency on | ||
| // anything outside this slice will surface here as a compile error, which is |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the actual command.rs boundary.
Replace the definitions-only claim. Cli::with_default_command in
src/cli/command.rs, Lines 116-125, changes command state. State that
command-schema and default-command behaviour belong in this module, or move the
method into the documented runtime boundary.
build.rs#L31-L35: Describe command-default behaviour as part of the
build-script slice boundary.docs/developers-guide.md#L391-L393: Keep the developer-guide boundary
statement aligned withbuild.rs.
As per coding guidelines, keep docs/developers-guide.md synchronized with
changed internal boundaries.
📍 Affects 2 files
build.rs#L31-L35(this comment)docs/developers-guide.md#L391-L393
🤖 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 `@build.rs` around lines 31 - 35, Update the boundary documentation in build.rs
lines 31-35 to state that src/cli/command.rs contains command-schema and
default-command behavior, including Cli::with_default_command; do not move the
method. Synchronize the corresponding boundary statement in
docs/developers-guide.md lines 391-393 with the same wording and scope.
Source: Coding guidelines
| `build.rs` recompiles part of the library as its own crate: it needs | ||
| `cli::Cli::command()` for man-page generation and the key registry in | ||
| `src/localization/keys.rs` for the Fluent audit. Rather than declaring |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Reconcile the build-script man-page description.
State whether build.rs stages a man page or whether cargo-orthohelp is the
sole generator. Lines 351-353 state that build.rs performs only the
localization audit, while build.rs, Lines 167-171 call
generate_man_page. Keep one unambiguous maintenance rule.
As per coding guidelines, treat docs/ Markdown as the source of truth.
🤖 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 `@docs/developers-guide.md` around lines 382 - 384, Reconcile the build-script
description in docs/developers-guide.md with the generate_man_page call and the
statement that build.rs only performs localization auditing. Explicitly state
whether build.rs stages a man page or cargo-orthohelp is the sole generator,
then update the related maintenance guidance so it presents one consistent rule
and preserves docs/ as the source of truth.
Source: Coding guidelines
| /// Apply the default command if none was specified. | ||
| #[must_use] | ||
| pub fn with_default_command(mut self) -> Self { | ||
| if self.command.is_none() { | ||
| self.command = Some(Commands::Build(BuildArgs::default())); | ||
| } | ||
| self | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add required Rustdoc usage and outcome examples.
Document each shared or public function with a concise usage example and its outcome.
src/cli/command.rs#L117-L124: Add an example that showswith_default_command()selectingCommands::BuildwhencommandisNone.src/cli/preferences.rs#L14-L44: Add examples that show each policy-to-preference mapping.src/cli/validation.rs#L15-L20: Add Rustdoc that states the producedOrthoError::Validationoutcome and shows a caller context.
As per coding guidelines: “Function documentation should include clear usage and outcome examples.”
📍 Affects 3 files
src/cli/command.rs#L117-L124(this comment)src/cli/preferences.rs#L14-L44src/cli/validation.rs#L15-L20
🤖 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 `@src/cli/command.rs` around lines 117 - 124, Add Rustdoc usage and outcome
examples for each affected public/shared function: in src/cli/command.rs lines
117-124, document with_default_command() selecting Commands::Build when command
is None; in src/cli/preferences.rs lines 14-44, document each
policy-to-preference mapping with examples; and in src/cli/validation.rs lines
15-20, describe the produced OrthoError::Validation and show caller context.
Source: Coding guidelines
| impl HostPattern { | ||
| /// Return whether `candidate` is covered by this pattern. | ||
| pub(crate) fn matches(&self, candidate: HostCandidate<'_>) -> bool { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add usage and outcome examples to HostPattern::matches.
Add a # Examples section that shows an exact match, a wildcard subdomain match, and rejection of the wildcard apex. This documents the boundary enforced by lines 25-31.
As per coding guidelines, function documentation must include clear usage and outcome examples.
🤖 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 `@src/host_matching.rs` around lines 21 - 23, Expand the documentation for
HostPattern::matches with a # Examples section demonstrating an exact host
match, a wildcard subdomain match, and rejection of the wildcard apex; show the
expected boolean outcomes for each case while preserving the existing
implementation.
Source: Coding guidelines
| pub(crate) fn matches(&self, candidate: HostCandidate<'_>) -> bool { | ||
| let host = candidate.as_str().to_ascii_lowercase(); | ||
| if self.wildcard { | ||
| // Wildcard patterns match only subdomains, not the apex domain. | ||
| // Example: "*.example.com" matches "sub.example.com" but not | ||
| // "example.com". | ||
| host.strip_suffix(&self.pattern) | ||
| .and_then(|prefix| prefix.strip_suffix('.')) | ||
| .is_some_and(|prefix| !prefix.is_empty()) | ||
| } else { | ||
| host == self.pattern | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's check the Cargo.toml for the url crate version
echo "=== Checking url crate version ==="
grep -A 2 "url" Cargo.toml | head -20
echo ""
echo "=== Checking HostCandidate construction ==="
# Find where HostCandidate is created
rg "HostCandidate\(" --type rust -B 2 -A 2
echo ""
echo "=== Checking host_matching.rs full context ==="
# Read the full host_matching.rs file
cat -n src/host_matching.rsRepository: leynos/netsuke
Length of output: 470
🏁 Script executed (no clone):
#!/bin/bash
# Test the actual behaviour of url crate with trailing dot
python3 << 'PY'
import json
import urllib.parse
# Note: Python's urllib doesn't handle trailing dots the same way as Rust url crate
# Let's check what we can determine
test_urls = [
"https://example.com/",
"https://example.com./",
"https://sub.example.com/",
"https://sub.example.com./",
]
for url in test_urls:
parsed = urllib.parse.urlparse(url)
print(f"URL: {url}")
print(f" netloc: {parsed.netloc}")
print(f" hostname: {parsed.hostname}")
print()
PY
Length of output: 447
🌐 Web query:
url crate host_str trailing dot DNS trailing period normalization
💡 Result:
The Rust url crate, which is based on the WHATWG URL Standard [1], does not perform DNS-style trailing period normalization on host strings. When a URL containing a domain with a trailing dot is parsed by the crate, the trailing dot is typically preserved in the resulting host representation, as it is considered part of the domain string rather than a structural component to be stripped [2][3][4]. The host_str method returns the string representation of the host as it was parsed (subject to IDNA punycode encoding for non-ASCII domains) [5][3]. Because the crate follows the WHATWG URL Standard, it treats the host as a domain string and does not automatically strip trailing dots, unlike some other DNS resolution libraries that might treat a trailing dot as a representation of the root zone [3][4][1]. If you require normalization (such as removing a trailing dot), you must handle this manually by checking the string returned by host_str or by post-processing the parsed domain [5][2].
Citations:
- 1: https://docs.rs/crate/url/latest
- 2: https://docs.rs/url/latest/url/
- 3: https://github.com/servo/rust-url/blob/master/url/src/lib.rs
- 4: https://codebrowser.dev/rust/crates/url/src/host.rs.html
- 5: https://docs.rs/url/1.1.0/url/struct.Url.html
🏁 Script executed:
#!/bin/bash
echo "=== Finding HostCandidate definition ==="
rg "struct HostCandidate" --type rust -A 5
echo ""
echo "=== Finding where HostCandidate is constructed ==="
# Look for HostCandidate( or HostCandidate {
rg "HostCandidate\s*\(" --type rust -B 3 -A 1
echo ""
echo "=== Checking network policy mod.rs around line 322 ==="
sed -n '310,335p' src/stdlib/network/policy/mod.rs
echo ""
echo "=== Reading host_matching.rs completely ==="
wc -l src/host_matching.rs
cat -n src/host_matching.rsRepository: leynos/netsuke
Length of output: 193
🏁 Script executed:
#!/bin/bash
echo "=== Searching for HostCandidate definition/type ==="
rg "HostCandidate" --type rust -B 2 -A 3 | head -80
echo ""
echo "=== Checking what HostCandidate is ==="
rg "type HostCandidate|struct HostCandidate|newtype HostCandidate" --type rust
echo ""
echo "=== Full host_matching.rs content ==="
cat -n src/host_matching.rsRepository: leynos/netsuke
Length of output: 392
🏁 Script executed:
#!/bin/bash
# Read the full host_matching.rs file
head -100 src/host_matching.rsRepository: leynos/netsuke
Length of output: 2373
🏁 Script executed:
#!/bin/bash
echo "=== Reading network policy mod.rs around line 322 ==="
sed -n '310,340p' src/stdlib/network/policy/mod.rs
echo ""
echo "=== Checking broader context in network policy ==="
sed -n '1,50p' src/stdlib/network/policy/mod.rs
echo ""
echo "=== Finding all calls to HostCandidate ==="
rg "HostCandidate" src/stdlib/network/policy/mod.rs -B 5 -A 2Repository: leynos/netsuke
Length of output: 3807
🏁 Script executed:
#!/bin/bash
echo "=== Checking if there's any normalization of host before or after ==="
sed -n '300,360p' src/stdlib/network/policy/mod.rs
echo ""
echo "=== Checking if host_matching.rs has any trailing dot handling in tests ==="
grep -n "trailing\|dot\|\\\." src/host_matching.rs
echo ""
echo "=== Checking if there are any tests for trailing dots ==="
rg "example\.com\." src/ --type rustRepository: leynos/netsuke
Length of output: 2495
🏁 Script executed:
#!/bin/bash
# Create a test to verify the vulnerability
cat > /tmp/test_trailing_dot.rs << 'TESTCODE'
use url::Url;
fn main() {
// Test 1: Normal URL
let url1 = Url::parse("https://example.com/data.txt").expect("parse");
println!("Normal URL host_str: {:?}", url1.host_str());
// Test 2: URL with trailing dot
let url2 = Url::parse("https://example.com./data.txt").expect("parse");
println!("Trailing dot URL host_str: {:?}", url2.host_str());
// Test 3: Another trailing dot form
let url3 = Url::parse("https://example.com./").expect("parse");
println!("Trailing dot with slash host_str: {:?}", url3.host_str());
}
TESTCODE
# Check Cargo.toml to get url version and run test
cd /tmp
echo "[package]" > Cargo.toml
echo "name = \"test\"" >> Cargo.toml
echo "version = \"0.1.0\"" >> Cargo.toml
echo "edition = \"2021\"" >> Cargo.toml
echo "" >> Cargo.toml
echo "[dependencies]" >> Cargo.toml
echo "url = \"^2.5.0\"" >> Cargo.toml
# Since we can't actually compile, let's verify the vulnerability through analysis
echo ""
echo "=== Vulnerability Analysis ==="
echo "1. url crate version: ^2.5.0 (from Cargo.toml in netsuke)"
echo "2. host_str() returns host as-is without trailing dot normalization"
echo "3. In network policy, host is obtained: url.host_str().filter(...)"
echo "4. host is passed directly: HostCandidate(host)"
echo "5. matches() does not normalize trailing dots"
echo ""
echo "VULNERABILITY CONFIRMED:"
echo "URL: https://example.com./"
echo "host_str() returns: Some(\"example.com.\")"
echo "Pattern stored: \"example.com\""
echo "matches() comparison: \"example.com.\" != \"example.com\" → no match"
echo "Result: Blocklist/allowlist bypass via trailing dot"Repository: leynos/netsuke
Length of output: 712
🏁 Script executed:
#!/bin/bash
echo "=== Checking full test suite in host_matching.rs ==="
wc -l src/host_matching.rs
echo ""
echo "=== Reading entire host_matching.rs file ==="
cat src/host_matching.rs
echo ""
echo "=== Checking for any recent commits affecting this file ==="
git log --oneline src/host_matching.rs 2>/dev/null | head -5 || echo "Git history not available"Repository: leynos/netsuke
Length of output: 2604
Authorization Bypass (CWE-863): Incorrect Authorization
Reachability: External
Reachability path
● Entry
src/cli/discovery.rs:309
as_str
│
▼
● Hop
src/host_pattern.rs:15
a
│
▼
● Sink
src/host_matching.rs
Strip trailing DNS dots from hostnames before matching policy patterns.
The url crate (pinned to ^2.5.0) preserves trailing dots in host_str() output without normalization. The matches() method lowercases the candidate hostname but does not strip trailing dots. For non-wildcard patterns, exact comparison fails: "example.com." != "example.com" returns false. An attacker-controlled URL such as https://example.com./ bypasses both blocklist and allowlist checks.
Strip one trailing dot from the hostname before the suffix check, or reject the form with an error. Add regression test cases for both exact and wildcard patterns with trailing dots.
Vulnerable code path
// src/stdlib/network/policy/mod.rs (line ~316)
let host = url.host_str()...; // Returns "example.com." for https://example.com./
pattern.matches(HostCandidate(host)) // Passes "example.com." directly
// src/host_matching.rs (line 25-35)
pub(crate) fn matches(&self, candidate: HostCandidate<'_>) -> bool {
let host = candidate.as_str().to_ascii_lowercase(); // "example.com."
// ...
host == self.pattern // "example.com." != "example.com" → false (bypass)
}
🤖 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 `@src/host_matching.rs` around lines 23 - 35, Update HostPattern::matches to
remove one trailing DNS dot from the lowercased candidate hostname before
applying exact or wildcard matching. Preserve the existing wildcard
subdomain-only behavior after normalization, and add regression coverage for
trailing-dot hosts against both exact and wildcard patterns.
Closes #513
Problem
build.rsrecompiles part of the library so it can callcli::Cli::command()for man-page generation. It declared
src/cli/mod.rs, which pulled the wholeclisubtree — merging, discovery, diagnostics, localized value parsing — pluscli_l10n,host_pattern,output_mode, andtheme. Removing the fivemodule-wide
#[expect(dead_code, ...)]attributes and building produced 110unused-item diagnostics, which is what those attributes were suppressing.
The suppressions also masked genuinely dead code. Appending an unused
pub fntosrc/cli/config.rsonmainproduced no diagnostic from anycompilation unit: the library exports
cli::configpublicly so it is notdead-code linted there, and the build script's module-wide expectation covered
it here.
Change
build.rsnow declares an inlineclifacade naming exactly the three filesthe Clap schema needs, rather than inheriting the subtree:
The library is split along the same seam so that slice is self-contained:
src/cli/command.rsCli,InteractionArgs,BuildArgs,GraphArgs,Commands)src/cli/parser.rs; the schema is all the man page needssrc/cli/preferences.rsClioutput-policy accessorstheme_preferencewas the only reason the build script compiledthemeand, transitively,output_modesrc/cli/validation.rsMAX_JOBS,validation_errorsrc/cli/config.rsstop reaching up intosrc/cli/mod.rssrc/host_matching.rsHostCandidate,HostPattern::matchessrc/host_pattern.rsthe schema does not needsrc/cli/parser.rskeeps the localization-aware parsing entry point;cli_l10n,output_mode, andthemeare no longer declared by the buildscript at all.
Result
cargo check --all-targetsemits no unused-item diagnostics.
mainnow reports: an unusedpub fninsrc/cli/config.rsproduceswarning: function ... is never usedfrom the build-script crate.before and after).
locales/orsrc/localization/touched. Thelocale_catalogues/localizationdeclarations were already correct and are untouched.
src/cli/config.rsat321,
src/host_pattern.rsdown from 344 to 304).docs/developers-guide.mdgains a section recording the slice as a maintainedboundary: widening it reintroduces unreachable items, and a dependency added
outside it surfaces as a build-script compile error.
Gates
cargo fmt -- --checkmake lint-clippy(cargo doc+ clippy)make testmake markdownlintmake nixieTwo gates fail identically on unmodified
origin/mainin this environment andare not caused by this change:
make check-fmtrunscargo fmt --all, which fails resolving thetest_supportpath dependency's workspace.cargo fmt -- --checkon theroot package passes.
make lint's Whitaker pass reportsno_std_fs_operationsagainstbuild.rsandbuild_l10n_audit.rs— 9 findings onmain, 8 after thischange, all in
std::fscalls this PR does not touch.🤖 Generated with Claude Code
Summary by Sourcery
Narrow the build script’s recompiled module slice for man-page/localization generation while keeping the CLI surface and behaviour unchanged.
Enhancements:
cargo:rerun-if-changeddirectives in the build script to track only the modules it actually recompiles.Documentation:
Tests: