Skip to content

Narrow the build script's module graph (#513) - #514

Draft
leynos wants to merge 1 commit into
mainfrom
issue-513-narrow-build-script-module-graph
Draft

Narrow the build script's module graph (#513)#514
leynos wants to merge 1 commit into
mainfrom
issue-513-narrow-build-script-module-graph

Conversation

@leynos

@leynos leynos commented Aug 4, 2026

Copy link
Copy Markdown
Owner

Closes #513

Problem

build.rs recompiles part of the library so it can call cli::Cli::command()
for man-page generation. It declared src/cli/mod.rs, which pulled the whole
cli subtree — merging, discovery, diagnostics, localized value parsing — plus
cli_l10n, host_pattern, output_mode, and theme. Removing the five
module-wide #[expect(dead_code, ...)] attributes and building produced 110
unused-item diagnostics, which is what those attributes were suppressing.

The suppressions also masked genuinely dead code. Appending an unused
pub fn to src/cli/config.rs on main produced no diagnostic from any
compilation unit
: the library exports cli::config publicly so it is not
dead-code linted there, and the build script's module-wide expectation covered
it here.

Change

build.rs now declares an inline cli facade naming exactly the three files
the Clap schema needs, rather than inheriting the subtree:

#[path = "src/cli"]
mod cli {
    #[path = "config.rs"] pub mod config;
    #[path = "validation.rs"] mod validation;
    #[path = "command.rs"] mod command;

    pub use command::Cli;
    pub use config::{AccessibilityPolicy, ColourPolicy, EmojiPolicy, ProgressPolicy};
}

The library is split along the same seam so that slice is self-contained:

New module Contents Why it moved
src/cli/command.rs The Clap definitions (Cli, InteractionArgs, BuildArgs, GraphArgs, Commands) Was the top half of src/cli/parser.rs; the schema is all the man page needs
src/cli/preferences.rs The four Cli output-policy accessors theme_preference was the only reason the build script compiled theme and, transitively, output_mode
src/cli/validation.rs MAX_JOBS, validation_error Lets src/cli/config.rs stop reaching up into src/cli/mod.rs
src/host_matching.rs HostCandidate, HostPattern::matches The only items in src/host_pattern.rs the schema does not need

src/cli/parser.rs keeps the localization-aware parsing entry point;
cli_l10n, output_mode, and theme are no longer declared by the build
script at all.

Result

  • All five module-wide expectations removed. cargo check --all-targets
    emits no unused-item diagnostics.
  • The probe that was silent on main now reports: an unused pub fn in
    src/cli/config.rs produces
    warning: function ... is never used from the build-script crate.
  • The generated man page is byte-identical (verified by diffing the artefact
    before and after).
  • No dependency added, no public API change, nothing under locales/ or
    src/localization/ touched. The locale_catalogues/localization
    declarations were already correct and are untouched.
  • Rerun directives now track the modules actually compiled.
  • No file exceeds the 400-line cap (largest touched: src/cli/config.rs at
    321, src/host_pattern.rs down from 344 to 304).

docs/developers-guide.md gains a section recording the slice as a maintained
boundary: widening it reintroduces unreachable items, and a dependency added
outside it surfaces as a build-script compile error.

Gates

Gate Status
cargo fmt -- --check pass
make lint-clippy (cargo doc + clippy) pass
make test pass — 1312 nextest, 47 doctests
make markdownlint pass
make nixie pass

Two gates fail identically on unmodified origin/main in this environment and
are not caused by this change:

  • make check-fmt runs cargo fmt --all, which fails resolving the
    test_support path dependency's workspace. cargo fmt -- --check on the
    root package passes.
  • make lint's Whitaker pass reports no_std_fs_operations against
    build.rs and build_l10n_audit.rs — 9 findings on main, 8 after this
    change, all in std::fs calls 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:

  • Split CLI schema, runtime preference accessors, and shared validation utilities into dedicated modules to form a self-contained slice usable by the build script.
  • Factor host matching logic into a new module so host pattern syntax can be compiled independently for the build script.
  • Adjust internal imports and re-exports across CLI and networking code to use the new command, preferences, validation, and host-matching modules.
  • Tighten cargo:rerun-if-changed directives in the build script to track only the modules it actually recompiles.

Documentation:

  • Document the build script’s maintained module slice boundary and its purpose in the developers’ guide.
  • Update the design document to reflect the new locations of the CLI schema and parsing/preference layers.

Tests:

  • Move host matching tests to the new host-matching module, keeping coverage of wildcard and exact pattern behaviour.

`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>
@coderabbitai

coderabbitai Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Summary

  • Narrow the build.rs module graph to the CLI modules required by cli::Cli::command().
  • Remove module-wide dead-code expectations and update rerun directives.
  • Split CLI command definitions, preferences, and validation into dedicated modules.
  • Move host-matching logic into host_matching and add focused tests.
  • Document the maintained build-script module boundary in docs/developers-guide.md.
  • Align CLI structure documentation with docs/netsuke-design.md.

Validation

  • Preserve the public API, generated man pages, and dependency set.
  • Pass formatting, Clippy, tests, markdownlint, and Nixie gates.
  • Detect previously masked unused items.

Walkthrough

The PR separates CLI command, preference, and validation responsibilities, narrows build.rs compilation, updates module documentation, and moves host matching into host_matching.

Changes

CLI command and validation

Layer / File(s) Summary
CLI command contract
src/cli/command.rs, src/cli/preferences.rs, src/cli/mod.rs, src/cli/parser.rs, docs/netsuke-design.md
Define Cli, subcommands, argument structures, defaults, and runtime preference accessors in dedicated modules.
CLI validation and import wiring
src/cli/validation.rs, src/cli/config.rs, src/cli/parsing.rs, src/cli/diag.rs, src/cli/discovery.rs, src/cli/merge.rs
Move shared validation values and helpers into validation, then update CLI consumers to use the new module paths.

Build-script module slice

Layer / File(s) Summary
Build-script module slice
build.rs, docs/developers-guide.md
Compile and track only the required CLI, localisation, and host-pattern sources. Document the module boundary and dependency rules.

Host matching

Layer / File(s) Summary
Host matching module
src/host_matching.rs, src/host_pattern.rs, src/lib.rs, src/stdlib/network/policy/mod.rs
Move exact and wildcard matching into host_matching, add matching tests, and update network policy imports.

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
Loading

Possibly related PRs

Suggested labels: Issue

Poem

Commands gather in a narrower tree,
Validation paths now flow clearly.
Hosts match through a dedicated door,
Build scripts track no branches more.
build waits when no command is near.


Caution

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

  • Ignore

❌ Failed checks (1 error, 5 warnings)

Check name Status Explanation Resolution
Testing (Overall) ❌ Error New functionality lacks direct rigorous tests. src/cli/preferences.rs (4 methods) has no unit tests; preference accessors are tested only indirectly through integration tests. `src/cli/validation... Add unit tests directly exercising Cli::progress_enabled(), Cli::accessibility_override(), validation_error(), and the Commands enum variants. Verify each returns expected outputs for different input combinations and would fail if...
Linked Issues check ⚠️ Warning The implementation satisfies issue #513's narrow-graph objective, but the issue requires all full gates to pass and two gates still fail. Make make check-fmt and make lint pass, or obtain an explicit exception for the unchanged environment failures.
Developer Documentation ⚠️ Warning The new slice section is clear, but existing guide/design text still says build.rs only audits localisation and ordinary builds write no help, contradicting build.rs's generate_man_page path. Update the release-help sections and relevant design/execplan records to describe both build.rs man-page generation and cargo-orthohelp release generation.
Testing (Unit And Behavioural) ⚠️ Warning Two preference accessor methods (accessibility_override(), progress_enabled()) in new src/cli/preferences.rs lack any test coverage. src/cli/command.rs schema changes (Cli, Commands, BuildA... Introduce unit tests for accessibility_override() and progress_enabled() methods in src/cli/preferences.rs that verify the translation of CLI policy flags to preference values. Add unit tests for MAX_JOBS constant and `validation...
Testing (Property / Proof) ⚠️ Warning The PR introduces host_matching::HostPattern::matches() with documented invariants (case-insensitivity, wildcard-only subdomain matching, non-empty prefix requirement). Tests use parametrised uni... Add property-based tests using proptest that verify: (1) case-insensitivity invariant across arbitrary patterns/candidates; (2) wildcard subdomain semantics; (3) exact-match suffix precision. Confirm matching is immune to non-ASCII, empt...
Testing (Compile-Time / Ui) ⚠️ Warning The PR introduces compile-time behaviour changes (narrowing the build.rs module slice, enabling detection of previously masked dead code) but includes no compile-time tests (trybuild, UI tests) to... Add a compile_fail UI test in tests/ui/ that demonstrates a compile error when build.rs attempts to compile code depending on modules outside the maintained slice boundary (e.g., cli::merge or cli::discovery), ensuring this hard guarante...
✅ Passed checks (14 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes narrowing the build script module graph and links issue #513.
Description check ✅ Passed The description clearly explains the module graph change, validation results, documentation updates, and known gate failures.
Out of Scope Changes check ✅ Passed The module refactors, host-matching move, documentation updates, and test relocation support the linked issue objectives.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
User-Facing Documentation ✅ Passed PR contains no user-facing functionality or behaviour changes. It reorganises internal build-script compilation; the CLI interface and public API remain unchanged (byte-identical man page). No user...
Module-Level Documentation ✅ Passed All changed and build-slice modules have //! documentation that states purpose, utility, and component relationships, including command, validation, preferences, and host_matching.
Unit Architecture ✅ Passed The refactor keeps CLI construction, preference accessors, validation, and host matching pure; parsing returns explicit Results with an injected Localizer, and build.rs isolates fallible file writes.
Domain Architecture ✅ Passed The patch strengthens boundaries: CLI schema and preference adapters are isolated, while host matching remains in network-policy code with no CLI, filesystem, transport, or persistence dependencies.
Observability ✅ Passed Pass: classify this as non-operational; the diff narrows build.rs and relocates existing CLI/host logic, with no new production boundary or runtime failure mode requiring telemetry.
Security And Privacy ✅ Passed Verify that the patch adds no secret-shaped data or new privileged sinks; the host matcher is byte-equivalent and policy enforcement remains unchanged.
Performance And Resource Use ✅ Passed The diff moves existing CLI and host-matching code without adding costly algorithms, collections, blocking work, or repeated I/O; the narrower build slice reduces build resource use.
Concurrency And State ✅ Passed Treat this check as not applicable: the PR only refactors synchronous CLI/module wiring; added-line searches found no async, tasks, locks, atomics, mutable globals, or cancellation paths.
Architectural Complexity And Maintainability ✅ Passed New modules (command, preferences, validation, host_matching) reduce net complexity by isolating seams for build script compilation. Dependency graph is acyclic; all have immediate usage; no specul...
Rust Compiler Lint Integrity ✅ Passed The PR removes all five broad build-script lint expectations, adds no equivalent suppression or clone, and narrows the compiled CLI boundary; host matching remains used by network policy.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch issue-513-narrow-build-script-module-graph

Comment @coderabbitai help to get the list of available commands.

@sourcery-ai

sourcery-ai Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Reviewer's Guide

Refactors 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

Change Details Files
Limit build.rs to a narrow, self-contained CLI and host-pattern slice instead of recompiling the full cli subtree.
  • Replace build.rs module import of src/cli/mod.rs with an inline cli facade that exposes only command, config, and validation modules and re-exports the needed types.
  • Stop declaring cli_l10n, output_mode, and theme in build.rs, and keep host_pattern and localization as separate modules.
  • Update build.rs rerun directives to track only the files actually compiled for man-page and localization generation.
build.rs
Split CLI schema, runtime preferences, and validation helpers into dedicated modules so the Clap schema is independent of parsing and runtime behavior.
  • Move Cli, InteractionArgs, BuildArgs, GraphArgs, and Commands from parser.rs into a new cli/command.rs, keeping only definitions and Clap derives there.
  • Introduce cli/preferences.rs to host Cli runtime preference accessors (theme_preference, accessibility_override, no_input, progress_enabled).
  • Introduce cli/validation.rs with MAX_JOBS and validation_error shared between parsing and config, and update config.rs and parsing.rs to depend on it.
  • Adjust cli/mod.rs to wire in the new submodules, re-export the public CLI surface from command.rs, and simplify parser re-exports.
  • Update merge.rs, diag.rs, discovery.rs, and parser.rs to import Cli and related types from command.rs and validation helpers from validation.rs.
src/cli/parser.rs
src/cli/command.rs
src/cli/preferences.rs
src/cli/validation.rs
src/cli/mod.rs
src/cli/merge.rs
src/cli/parsing.rs
src/cli/diag.rs
src/cli/discovery.rs
src/cli/config.rs
Separate host pattern syntax/normalization from hostname matching to keep build-script dependencies minimal while preserving behavior.
  • Remove HostCandidate and HostPattern::matches from host_pattern.rs, leaving only parsing, normalization, and related tests.
  • Add a new host_matching.rs module that defines HostCandidate and implements HostPattern::matches, including relocated wildcard/exact matching tests.
  • Update lib.rs to declare the new host_matching module and stdlib network policy code to use HostCandidate from host_matching instead of host_pattern.
  • Adjust host_pattern.rs tests and documentation comments to reflect its new focus on parsing and normalization only.
src/host_pattern.rs
src/host_matching.rs
src/lib.rs
src/stdlib/network/policy/mod.rs
Document the build script’s maintained module slice and the CLI schema split for future contributors.
  • Add a section to docs/developers-guide.md explaining the build.rs module slice, the rationale for keeping it narrow, and guidance on avoiding reintroduction of dead-code suppressions.
  • Update netsuke-design.md to describe the new locations of the Cli type, parsing entry point, and runtime preferences, consistent with the refactor.
docs/developers-guide.md
docs/netsuke-design.md

Assessment against linked issues

Issue Objective Addressed Explanation
#513 Narrow the build script's module graph (particularly around src/cli/ and related modules) so that module-wide #[expect(dead_code, unused_imports, ...)] attributes are no longer needed and unused items in src/cli/ once again produce diagnostics during a normal build.
#513 Preserve existing build-script behavior, especially the ability for build.rs to call cli::Cli::command() for man-page generation (and to use localization keys) without adding new build dependencies or breaking gates/tests.

Possibly linked issues


Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

codescene-access[bot]

This comment was marked as outdated.

@codescene-access codescene-access Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No quality gates enabled for this code.

@buzzybee-df12

Copy link
Copy Markdown
Collaborator

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot added the Issue label Aug 4, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 8e9c7cc and af77331.

📒 Files selected for processing (17)
  • build.rs
  • docs/developers-guide.md
  • docs/netsuke-design.md
  • src/cli/command.rs
  • src/cli/config.rs
  • src/cli/diag.rs
  • src/cli/discovery.rs
  • src/cli/merge.rs
  • src/cli/mod.rs
  • src/cli/parser.rs
  • src/cli/parsing.rs
  • src/cli/preferences.rs
  • src/cli/validation.rs
  • src/host_matching.rs
  • src/host_pattern.rs
  • src/lib.rs
  • src/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)

Comment thread build.rs
Comment on lines +31 to +35
// 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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 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 with build.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

Comment thread docs/developers-guide.md
Comment on lines +382 to +384
`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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 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

Comment thread src/cli/command.rs
Comment on lines +117 to +124
/// 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
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 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 shows with_default_command() selecting Commands::Build when command is None.
  • 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 produced OrthoError::Validation outcome 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-L44
  • src/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

Comment thread src/host_matching.rs
Comment on lines +21 to +23
impl HostPattern {
/// Return whether `candidate` is covered by this pattern.
pub(crate) fn matches(&self, candidate: HostCandidate<'_>) -> bool {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 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

Comment thread src/host_matching.rs
Comment on lines +23 to +35
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
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 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.rs

Repository: 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:


🏁 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.rs

Repository: 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.rs

Repository: leynos/netsuke

Length of output: 392


🏁 Script executed:

#!/bin/bash

# Read the full host_matching.rs file
head -100 src/host_matching.rs

Repository: 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 2

Repository: 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 rust

Repository: 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Narrow the build script's module graph instead of module-wide dead-code expectations

2 participants