refactor(agent,scout): adopt style guide rules for pub/module visibility - #4660
Merged
Conversation
This adopts the new style guide rules around module visibility introduced in NVIDIA#4522, applying the correct visibility throughout Agent and Scout. Primary callouts are: - Replace unrestricted `pub` throughout `crates/agent/src/**` and `crates/scout/src/**` with private, `pub(super)`, or `pub(crate)` visibility based on actual callers. - Keep the Agent exported `duppet` logging macro, generated clients owned by VSC-01, and existing cross-crate entry points public where they are still consumed. - Keep Scout command-line, deprovisioning, firmware-upgrade, and metric internals inside the crate while narrowing sibling and parent boundaries to their actual callers. - Remove the unused Scout `Rebuild` parser, the unused Agent sysfs address helper, and stale dead-code suppression exposed during the cleanup. - Leave CLI parsing, gRPC contracts, wire data, generated client code, and runtime behavior unchanged. Tests updated! This supports NVIDIA#4551. This supports the updated Rust visibility scoping guidelines in `STYLE_GUIDE.md`, established in NVIDIA#4522. Signed-off-by: Chet Nichols III <chetn@nvidia.com>
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
ᕱᕱ
|
Contributor
|
Warning Review limit reached
Next review available in: 12 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (45)
Summary by CodeRabbit
WalkthroughThe PR narrows Rust visibility across the agent and scout crates. It changes public items to private, ChangesInternal API visibility cleanup
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/scout/src/deprovision/scrabbing.rs (1)
1148-1148: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNarrow declarations to their actual caller boundaries.
Both declarations remain broader than the supplied caller graph requires.
crates/scout/src/deprovision/scrabbing.rs#L1148-L1148: changerun_no_apifrompub(crate)topub(super)becausecrates/scout/src/deprovision/mod.rsre-exports it.crates/scout/src/tpm.rs#L28-L28: makeTPM_RECOVERY_ATTEMPTED_PATHprivate if the repository-wide search finds no caller outsidecrates/scout/src/tpm.rs.🤖 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 `@crates/scout/src/deprovision/scrabbing.rs` at line 1148, Narrow the visibility of deprovision::scrabbing::run_no_api in crates/scout/src/deprovision/scrabbing.rs:1148-1148 from pub(crate) to pub(super), preserving access through deprovision/mod.rs. In crates/scout/src/tpm.rs:28-28, make TPM_RECOVERY_ATTEMPTED_PATH private only if repository-wide usage confirms no callers outside tpm.rs.Sources: Coding guidelines, Path instructions
🤖 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.
Nitpick comments:
In `@crates/scout/src/deprovision/scrabbing.rs`:
- Line 1148: Narrow the visibility of deprovision::scrabbing::run_no_api in
crates/scout/src/deprovision/scrabbing.rs:1148-1148 from pub(crate) to
pub(super), preserving access through deprovision/mod.rs. In
crates/scout/src/tpm.rs:28-28, make TPM_RECOVERY_ATTEMPTED_PATH private only if
repository-wide usage confirms no callers outside tpm.rs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4484eb7c-add2-4a3c-8a6a-4e5fd30699c2
📒 Files selected for processing (45)
crates/agent/src/acl_rules.rscrates/agent/src/astra_weave.rscrates/agent/src/command_line.rscrates/agent/src/dhcp.rscrates/agent/src/dhcp_server_grpc_client.rscrates/agent/src/dpu/interface.rscrates/agent/src/duppet/log.rscrates/agent/src/duppet/sync.rscrates/agent/src/ethernet_virtualization.rscrates/agent/src/fmds_client.rscrates/agent/src/hbn.rscrates/agent/src/health.rscrates/agent/src/health/bgp.rscrates/agent/src/host_machine_id.rscrates/agent/src/instance_metadata_endpoint.rscrates/agent/src/lib.rscrates/agent/src/machine_inventory_updater.rscrates/agent/src/main_loop.rscrates/agent/src/managed_files.rscrates/agent/src/metadata_service.rscrates/agent/src/mtu.rscrates/agent/src/ovs.rscrates/agent/src/periodic_config_fetcher.rscrates/agent/src/sysfs.rscrates/agent/src/tests/common/mod.rscrates/agent/src/tests/full.rscrates/agent/src/tests/test_network_monitor.rscrates/agent/src/util.rscrates/scout/src/attestation.rscrates/scout/src/cfg/command_line.rscrates/scout/src/cfg/mod.rscrates/scout/src/client.rscrates/scout/src/deprovision/cmdrun.rscrates/scout/src/deprovision/mod.rscrates/scout/src/deprovision/scrabbing.rscrates/scout/src/discovery.rscrates/scout/src/firmware_upgrade.rscrates/scout/src/machine_validation.rscrates/scout/src/main.rscrates/scout/src/metrics.rscrates/scout/src/mlx_device.rscrates/scout/src/platform.rscrates/scout/src/register.rscrates/scout/src/stream.rscrates/scout/src/tpm.rs
10 tasks
chet
enabled auto-merge (squash)
August 6, 2026 14:34
ianderson-nvidia
approved these changes
Aug 6, 2026
10 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This adopts the new style guide rules around module visibility introduced in #4522, applying the correct visibility throughout Agent and Scout.
Primary callouts are:
pubthroughoutcrates/agent/src/**andcrates/scout/src/**with private,pub(super), orpub(crate)visibility based on actual callers.duppetlogging macro, generated clients owned by VSC-01, and existing cross-crate entry points public where they are still consumed.Rebuildparser, the unused Agent sysfs address helper, and stale dead-code suppression exposed during the cleanup.Tests updated!
Related issues
This supports #4551.
This supports the updated Rust visibility scoping guidelines in
STYLE_GUIDE.md, established in #4522.Type of Change
Breaking Changes
Testing
All 199 Agent tests and 56 Scout tests passed. I also ran:
cargo test --locked -p carbide-agent -p carbide-scout --all-features cargo clippy --locked -p carbide-agent -p carbide-scout --all-targets --all-features -- --force-warn unreachable-pub cargo make format-nightly cargo make clippy cargo make carbide-lints git diff --checkAdditional Notes
The current
mainbaseline identified 177 overbroad declarations across Agent and Scout. The final 45-file diff leaves no checked-in Agent or Scout source finding; the strict command still reports 24 generated tonic declarations under the Agent build output, which are intentionally untouched here as part of the generated client boundary owned by VSC-01.Local CodeRabbit and read-only Claude reviews completed over the full diff. The applicable private-helper, stale-comment, unused-helper, and private-test-field findings are incorporated.
The review also proposed spelling crate-root child visibility as
pub(crate)instead ofpub(super). Those boundaries staypub(super)because theSTYLE_GUIDE.mdcaller table maps parent-module callers topub(super); although both spellings reach the same modules for a crate-root child,pub(super)records the actual caller relationship. A helper used by a sibling test module also stayspub(super)for the same reason, and the exportedduppetlogging macro remains public as required by the issue.Closes #4551