docs: define Rust visibility scoping guidelines by chet · Pull Request #4522 · NVIDIA/infra-controller

chet · 2026-08-04T00:55:01Z

Rust gives us several useful visibility boundaries, but we do not currently explain when contributors should use private, pub(super), pub(in crate::path), pub(crate), or unrestricted pub. We already protect dead-code detection in STYLE_GUIDE.md, and carbide-api-core has a useful local note about keeping its cross-crate surface small, so this makes that expectation repository-wide.

This change:

Adds a STYLE_GUIDE.md section that tells contributors to start private and widen visibility only for actual callers.
Explains the intended uses of pub(super), pub(in crate::path), pub(crate), and pub, including cross-package callers in this workspace.
Covers effective visibility through modules and re-exports, direct field visibility, unit tests, integration-test support, generated code, and exported macros.
Adds a concise AGENTS.md rule that points coding agents to the canonical style guidance.

Related issues

Define Rust Visibility Scoping Guidelines #4518

Type of Change

Add - New feature or capability
Change - Changes in existing functionality
Fix - Bug fixes
Remove - Removed features or deprecated functionality
Internal - Internal changes (refactoring, tests, docs, etc.)

Breaking Changes

This PR contains breaking changes

Testing

Unit tests added/updated
Integration tests added/updated
Manual testing performed
No testing required (docs, internal refactor, etc.)

Validation performed:

rumdl check --config docs/.rumdl.toml AGENTS.md STYLE_GUIDE.md
cargo make format-nightly
cargo make clippy
cargo make carbide-lints

Additional Notes

The repository-required Markdown formatter also normalized existing list indentation and code-fence formatting in STYLE_GUIDE.md. The same lint pass exposed a stale networking documentation link, which now points to docs/architecture/networking_integrations.md.

Closes #4518

coderabbitai · 2026-08-04T00:55:56Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 9c76aa4c-99e7-4c5a-b27e-f3e137a0e419

📥 Commits

Reviewing files that changed from the base of the PR and between 5f3ebcf and 243eac0.

📒 Files selected for processing (2)

AGENTS.md
STYLE_GUIDE.md

Summary by CodeRabbit

Documentation
- Added comprehensive Rust visibility guidance covering access levels, API boundaries, re-exports, testing, generated code, macros, and restricted access.
- Updated guidance for dead-code testing, gRPC APIs, networking, and getters.
- Improved formatting, corrected a spelling error, and linked visibility guidance from contributor instructions.

Walkthrough

The change adds Rust visibility guidance to AGENTS.md and STYLE_GUIDE.md. It also clarifies dead-code testing, getter visibility, API formatting, networking links, and related wording.

Changes

Rust visibility guidance

Layer / File(s)	Summary
Visibility rules and caller scope `AGENTS.md`, `STYLE_GUIDE.md`	Documents the narrowest required visibility, API boundaries, re-exports, test access, generated code, macros, and dead-code handling.
Related style guide updates `STYLE_GUIDE.md`	Updates introductory, gRPC, networking, and getter guidance. It also updates formatting and corrects a spelling error.

Estimated code review effort: 1 (Trivial) | ~5 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main change: defining Rust visibility scoping guidelines.
Description check	✅ Passed	The description directly explains the visibility guidance, related documentation updates, and validation performed.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

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

chet · 2026-08-04T00:55:57Z

@coderabbitai full_review, thanks!

coderabbitai · 2026-08-04T00:56:10Z

@chet: I will perform a complete review of pull request #4522.

🐇✅

✅ Action performed

Full review finished.

kensimon

This is mostly pretty great, but I generally think we should keep "how rust works" explanations out of STYLE.md, if only to save on context size. Or even more generally, don't bother documenting it in STYLE.md if any of our tooling already prevents it.

kensimon · 2026-08-04T19:48:24Z

+Code in another workspace package crosses a Rust crate boundary, so its callers require `pub`. Rust does not provide
+workspace-level visibility. The named path in `pub(in crate::path)` must be an ancestor module; use this form when that
+ancestor represents a useful internal API boundary.


This paragraph can go, IMO (it's just documenting how rust works... let's save the precious context window.)

Yeah, I think that's fair. I removed the paragraph and pulled the one repo-specific distinction into the table: another workspace package is still another crate, so callers there require pub.

kensimon · 2026-08-04T19:52:40Z

+- Do not edit generated output. Change the generator or source and regenerate the output. When generated declarations
+  intentionally use broader visibility, restrict the module or re-export that contains them, or place a documented lint
+  exemption at the generated-code boundary.


This can probably go too (just IMO...) editing generated code won't even show up in the PR, so if somebody tries to do it it's not going to work anyway.

(If we have continual issues where an LLM don't realize certain code is generated, it's probably worth calling that out as an item in AGENTS.md or something, like "don't edit files in some_path/, they're generated", or similar.)

Agreed. I removed this from the visibility guidance. We already cover generated interfaces in AGENTS.md, and if the cleanup turns up a specific generated path that agents keep trying to edit, we can call that out where it lives.

kensimon · 2026-08-04T19:55:17Z

+  intentionally use broader visibility, restrict the module or re-export that contains them, or place a documented lint
+  exemption at the generated-code boundary.
+- Exported macros can require public helpers because their expansions use those helpers from downstream crates.
+  `#[doc(hidden)]` hides an item from generated documentation; it does not make the item private. Compile downstream


I've read this a few times but I can't figure out what "Compile downstream expansion tests" means. I kinda know what "narrowing a macro helper" means but I don't really see how it makes sense in this context. (Lowering its visibility? But you can't, if the public macro needs it, right? This is why so many crates with public macros have e.g. mod __macro_support sections... because the pub macro needs it but you're not supposed to use it directly.)

Yeah, that was too hard to parse. I changed it to say directly that helper paths referenced by exported macro expansions need to remain public, even when they're hidden from generated documentation. That's the exception I was trying to capture here.

kensimon · 2026-08-04T19:55:44Z

+- Exported macros can require public helpers because their expansions use those helpers from downstream crates.
+  `#[doc(hidden)]` hides an item from generated documentation; it does not make the item private. Compile downstream
+  expansion tests before narrowing a macro helper.
+- Trait items and enum variants inherit their container's visibility and cannot declare a narrower scope independently.


This line is more rust documentation, I think we can skip it.

Agreed, I removed this. The compiler already enforces it, so it doesn't need to take up space in the style guide.

kensimon · 2026-08-04T20:16:19Z

+Select field visibility independently from type visibility. The [Fields and getters](#fields-and-getters) guidance
+favors direct field access; it does not imply unrestricted `pub`.


Hmm, I'm conflicted about this section. If a type is already private, marking its fields higher-visibility is already a compiler warning (treated by us as an error) because fields cannot have higher visibility than the owning type. So it's not really true that field visibility and type visibility are independent.

The fields and getters section encourages you to treat the fields of a struct as "public", but with the assumption that you have a good idea of how "public" the type should be... so if the type is pub, make the fields pub, if the type is pub(super), make the fields pub(super), etc.

Maybe some wording like:

See Fields and getters for guidance on direct field access; if you do decide to make fields visible to callers, select a visibility that matches the type's: a pub field for a pub struct, a pub(crate) field for a pub(crate) struct, etc.

Yeah, I agree that “independently” wasn't the right way to describe this. I changed it to say that a visible field should be no more visible than its type, while still allowing it to be narrower when only some callers need direct access. That keeps the direct-field guidance without implying unrestricted pub.

Rust gives us several useful visibility boundaries, but we do not currently explain when contributors should use private, `pub(super)`, `pub(in crate::path)`, `pub(crate)`, or unrestricted `pub`. We already protect dead-code detection in `STYLE_GUIDE.md`, and `carbide-api-core` has a useful local note about keeping its cross-crate surface small. So, this makes that expectation repository-wide and points coding agents to it from `AGENTS.md`. This supports NVIDIA#4518 Signed-off-by: Chet Nichols III <chetn@nvidia.com>

coderabbitai · 2026-08-04T20:57:46Z

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

chet · 2026-08-04T21:00:58Z

@coderabbitai full_review, thanks!

coderabbitai · 2026-08-04T21:01:08Z

@chet: I will perform a complete review of pull request #4522.

🐇🔍

✅ Action performed

Full review finished.

#4593) This adopts the new style guide rules around `pub` and module visibility introduced in #4522, applying the correct visibility throughout Admin CLI. Primary callouts are: - Replace unrestricted `pub` throughout `crates/admin-cli/src/**` with private, `pub(super)`, named-ancestor, or `pub(crate)` visibility based on real call sites. - Keep parser coverage without exposing implementation fields by reading typed `ArgMatches` through a `#[cfg(test)]` helper and colocating tests that need private conversion details. - Remove four unused items that became dead code once their accidental public visibility was removed. - Leave Clap attributes, generated manuals, RPC contracts, and command behavior unchanged. Tests updated! ## Related issues This supports #4546. This supports the updated Rust visibility scoping guidelines in `STYLE_GUIDE.md`, established in #4522. ## Type of Change - [ ] **Add** - New feature or capability - [ ] **Change** - Changes in existing functionality - [ ] **Fix** - Bug fixes - [ ] **Remove** - Removed features or deprecated functionality - [x] **Internal** - Internal changes (refactoring, tests, docs, etc.) ## Breaking Changes - [ ] **This PR contains breaking changes** ## Testing - [x] Unit tests added/updated - [ ] Integration tests added/updated - [x] Manual testing performed - [ ] No testing required (docs, internal refactor, etc.) All 412 Admin CLI tests pass. I also ran: ```bash cargo test --locked -p nico-admin-cli --all-targets --all-features cargo clippy --locked -p nico-admin-cli --all-targets --all-features --no-deps -- -F unreachable-pub cargo make clippy cargo make carbide-lints cargo make check-format-nightly cargo run --locked -q -p nico-admin-cli -- --help git diff --check ``` ## Additional Notes The 1,015-file diff covers the complete Admin CLI crate, and every changed path stays under `crates/admin-cli/src/**`. Most hunks are visibility tokens plus the resulting rustfmt reflow. `cargo make check-cli-docs` runs its five tests successfully, then reports broad drift between current `main` and the checked-in generated CLI manuals. This change does not touch Clap attributes or generated manuals, so I restored the generated output and left that baseline drift out of this PR. Local CodeRabbit and read-only Claude reviews completed across four slices of at most 260 files because CodeRabbit caps one review at 300 files. All applicable findings are incorporated. Signed-off-by: Chet Nichols III <chetn@nvidia.com>

This adopts the new style guide rules around `pub` and module visibility introduced in NVIDIA#4522, applying the correct visibility throughout API Core. Primary callouts are: - Replace unrestricted `pub` throughout `crates/api-core/src/**` with private, `pub(super)`, named-ancestor, or `pub(crate)` visibility based on real call sites. - Keep `bootstrap`, configuration, secrets, API Web, and feature-gated `test_support` contracts public where they cross crate boundaries. - Keep unit-test coverage without widening production APIs by making test modules private and using `#[cfg(test)]` adapters where sibling tests need narrow access. - Remove unused helpers and two orphaned, uncompiled test files that became visible during dead-code cleanup. - Leave RPC, database, service, and runtime behavior unchanged. Tests updated! This supports NVIDIA#4549. 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>

…#4600) This adopts the new style guide rules around `pub` and module visibility introduced in #4522, applying the correct visibility throughout API Core. Primary callouts are: - Replace unrestricted `pub` throughout `crates/api-core/src/**` with private, `pub(super)`, named-ancestor, or `pub(crate)` visibility based on real call sites. - Keep `bootstrap`, configuration, secrets, API Web, and feature-gated `test_support` contracts public where they cross crate boundaries. - Keep unit-test coverage without widening production APIs by making test modules private and using `#[cfg(test)]` adapters where sibling tests need narrow access. - Remove unused helpers and two orphaned, uncompiled test files that became visible during dead-code cleanup. - Leave RPC, database, service, and runtime behavior unchanged. Tests updated! ## Related issues This supports #4549. This supports the updated Rust visibility scoping guidelines in `STYLE_GUIDE.md`, established in #4522. ## Type of Change - [ ] **Add** - New feature or capability - [ ] **Change** - Changes in existing functionality - [ ] **Fix** - Bug fixes - [ ] **Remove** - Removed features or deprecated functionality - [x] **Internal** - Internal changes (refactoring, tests, docs, etc.) ## Breaking Changes - [ ] **This PR contains breaking changes** ## Testing - [x] Unit tests added/updated - [ ] Integration tests added/updated - [ ] Manual testing performed - [ ] No testing required (docs, internal refactor, etc.) All 21 filtered machine-identity tests and both MAC-address-pool tests pass. I also ran: ```bash cargo check --locked -p carbide-api-core --all-targets --all-features cargo check --locked -p carbide-api-core --all-targets --no-default-features cargo check --locked -p carbide-api-core --all-targets cargo check --locked -p carbide-api -p carbide-api-web -p carbide-test-harness -p carbide-api-integration-tests --all-targets --all-features cargo test --locked -p carbide-api-core --all-features --lib --no-run cargo test --locked -p carbide-api-core --all-features machine_identity --lib cargo test --locked -p carbide-api-core --all-features mac_address_pool --lib cargo clippy --locked -p carbide-api-core --all-targets --all-features --no-deps -- -D unreachable-pub cargo make clippy cargo make carbide-lints cargo +nightly-2026-06-16 fmt --all -- --check git diff --check ``` ## Additional Notes The initial visibility lint identified 624 overbroad declarations across 106 files. Making the root test module private exposed the same pattern across sibling fixtures, so the final diff covers 173 files. Every changed path stays under `crates/api-core/src/**` except the required `crates/api-web/src/lib.rs` comment update after `init_tools` became crate-visible. Most hunks are visibility tokens plus resulting rustfmt reflow. Local CodeRabbit and read-only Claude reviews completed over the full diff. All applicable findings are incorporated. Signed-off-by: Chet Nichols III <chetn@nvidia.com>

…4630) This adopts the new style guide rules around `pub` and module visibility introduced in #4522, applying the correct visibility throughout API Web. Primary callouts are: - Replace unrestricted `pub` throughout `crates/api-web/src/**` with private, `pub(super)`, or test-only `pub(crate)` visibility based on real call sites. - Keep `carbide_api_web::routes` public as the crate's HTTP integration point while scoping handlers and helpers to their actual parents and siblings. - Keep managed-host row-display coverage without widening production APIs by colocating that test with its private implementation. - Remove the unused managed-host `time_in_state` display field exposed during dead-code cleanup. - Leave HTTP routes, templates, serialized responses, RPC calls, and runtime behavior unchanged. Tests updated! ## Related issues This supports #4548. This supports the updated Rust visibility scoping guidelines in `STYLE_GUIDE.md`, established in #4522. ## Type of Change - [ ] **Add** - New feature or capability - [ ] **Change** - Changes in existing functionality - [ ] **Fix** - Bug fixes - [ ] **Remove** - Removed features or deprecated functionality - [x] **Internal** - Internal changes (refactoring, tests, docs, etc.) ## Breaking Changes - [ ] **This PR contains breaking changes** ## Testing - [x] Unit tests added/updated - [ ] Integration tests added/updated - [ ] Manual testing performed - [ ] No testing required (docs, internal refactor, etc.) All 59 API Web tests passed during implementation. The focused managed-host row-display test also passed against an isolated PostgreSQL instance after the clean rebase that brought in #4600. I also ran: ```bash make core/tests TEST_ARGS="-p carbide-api-web test_managed_host_row_display" cargo check --locked -p carbide-api-web --all-targets --all-features cargo make format-nightly cargo make clippy cargo make carbide-lints git diff --check ``` ## Additional Notes The initial visibility lint identified 280 overbroad declarations across 51 files. The final 55-file diff also includes the root and test-module boundaries exposed by that cleanup, and the only unrestricted source `pub` left is the intentional `carbide_api_web::routes` crate API. Most hunks are visibility tokens plus the resulting rustfmt reflow. Local CodeRabbit and read-only Claude reviews completed over the full diff. All applicable findings are incorporated. Signed-off-by: Chet Nichols III <chetn@nvidia.com>

This adopts the new style guide rules around module visibility introduced in NVIDIA#4522, applying the correct visibility throughout BMC Mock. Primary callouts are: - Replace unrestricted `pub` throughout `crates/bmc-mock/src/**` with private, `pub(super)`, or `pub(crate)` visibility based on actual callers. - Keep the mock server/router, `MachineInfo`, injection controls, IPMI simulator, MAC pools, callback backends, and integration-test support public where they cross crate boundaries. - Make the hardware and Redfish implementation trees internal while keeping `RackElevation` and `RackUnit` available through intentional root re-exports. - Remove unused exports, helpers, and nonserialized state, including stale blanket dead-code allowances. - Preserve HTTP routes, serialized Redfish responses, supported test fixtures, deterministic MAC allocation, and runtime behavior. Tests updated! This supports NVIDIA#4550. 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>

chet requested a review from a team as a code owner August 4, 2026 00:55

chet requested review from bcavnvidia, ianderson-nvidia, kensimon, poroh, rwthompsonii, wminckler and yoks August 4, 2026 00:57

rwthompsonii approved these changes Aug 4, 2026

View reviewed changes

chet force-pushed the gh-issue-4518 branch from 51eb985 to cf0f853 Compare August 4, 2026 05:56

bcavnvidia approved these changes Aug 4, 2026

View reviewed changes

spydaNVIDIA approved these changes Aug 4, 2026

View reviewed changes

ianderson-nvidia approved these changes Aug 4, 2026

View reviewed changes

wminckler approved these changes Aug 4, 2026

View reviewed changes

kensimon requested changes Aug 4, 2026

View reviewed changes

chet force-pushed the gh-issue-4518 branch from cf0f853 to 243eac0 Compare August 4, 2026 20:57

kensimon approved these changes Aug 4, 2026

View reviewed changes

yoks approved these changes Aug 4, 2026

View reviewed changes

poroh approved these changes Aug 4, 2026

View reviewed changes

chet enabled auto-merge (squash) August 4, 2026 21:50

chet merged commit 39a778d into NVIDIA:main Aug 4, 2026
63 checks passed

chet deleted the gh-issue-4518 branch August 4, 2026 22:07

chet mentioned this pull request Aug 4, 2026

Tighten Rust Visibility Across Infra Controller #4545

Open

coderabbitai Bot mentioned this pull request Aug 5, 2026

Upgrade to Rust 1.97, use new dead_code_pub_in_binary lint #4571

Open

10 tasks

chet mentioned this pull request Aug 5, 2026

refactor(admin-cli): adopt style guide rules for pub/module visibility #4593

Merged

10 tasks

chet mentioned this pull request Aug 5, 2026

refactor(api-core): adopt style guide rules for pub/module visibility #4600

Merged

10 tasks

chet mentioned this pull request Aug 5, 2026

refactor(api-web): adopt style guide rules for pub/module visibility #4630

Merged

10 tasks

chet mentioned this pull request Aug 5, 2026

refactor(bmc-mock): adopt style guide rules for pub/module visibility #4635

Merged

10 tasks

This was referenced Aug 6, 2026

docs: pull our concurrent database guidance into STYLE_GUIDE.md #4641

Open

refactor(dhcp,pxe): adopt style guide rules for pub/module visibility #4647

Open

		Select field visibility independently from type visibility. The [Fields and getters](#fields-and-getters) guidance
		favors direct field access; it does not imply unrestricted `pub`.

Uh oh!

Conversation

chet commented Aug 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Related issues

Type of Change

Breaking Changes

Testing

Additional Notes

Uh oh!

coderabbitai Bot commented Aug 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Walkthrough

Changes

Uh oh!

chet commented Aug 4, 2026

Uh oh!

coderabbitai Bot commented Aug 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kensimon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kensimon Aug 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot commented Aug 4, 2026

Uh oh!

chet commented Aug 4, 2026

Uh oh!

coderabbitai Bot commented Aug 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

chet commented Aug 4, 2026 •

edited

Loading

coderabbitai Bot commented Aug 4, 2026 •

edited

Loading

coderabbitai Bot commented Aug 4, 2026 •

edited

Loading

kensimon Aug 4, 2026 •

edited

Loading

coderabbitai Bot commented Aug 4, 2026 •

edited

Loading