refactor: extend the error-message checker to format!-wrapped messages#3595
Merged
Conversation
Contributor
WalkthroughThis PR standardizes error-message casing, wording, and punctuation across Rust crates. It also extends the ChangesError message standardization
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
Actionable comments posted: 9
🧹 Nitpick comments (1)
crates/xtask/src/error_message_case.rs (1)
746-784: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover
format_args!explicitly.
FORMAT_MACROSsupports both spellings, but this integration test covers onlyformat!. Add table-driven cases forformat!andformat_args!in error and non-error positions so the advertisedformat_args!behavior cannot regress unnoticed.As per coding guidelines, “Use tables whenever two or more tests invoke the same operation with different inputs.”
🤖 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/xtask/src/error_message_case.rs` around lines 746 - 784, Extend the end_to_end_rewrite_reaches_format test into table-driven cases covering both format! and format_args! in error positions and non-error positions. Assert that error-position invocations are rewritten while bare invocations remain unchanged, preserving interpolation text and the existing log-macro coverage.Source: Coding guidelines
🤖 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 `@crates/admin-cli/src/instance/reboot/cmd.rs`:
- Line 31: Update the expected error string in
reboot_error_includes_instance_context_and_preserves_source to use the lowercase
“failed” casing now produced by the wrap_err_with message, while preserving the
rest of the assertion and source-error checks.
In `@crates/agent/src/agent_platform.rs`:
- Around line 62-64: Normalize the remaining file-operation error contexts in
the surrounding agent platform implementation, including the paths at the
referenced locations, to use the same lowercase wording as the updated
destination-file existence message. Preserve the existing errors and formatting
while changing only the inconsistent uppercase variants.
In `@crates/api-core/src/attestation/measured_boot.rs`:
- Line 225: Update the CarbideError::AttestBindKeyError message for the
fs::read(cred_out_path) failure to say “could not read creds file” instead of
“could not create creds file”, while leaving the surrounding error handling
unchanged.
In `@crates/api-core/src/handlers/dpu.rs`:
- Line 950: Update the error message in the DHCP timestamp parsing path to use
the standard uppercase “DHCP” acronym while preserving the normalized sentence
casing and existing error details.
- Around line 123-124: Update the discovery error messages in the DPU handler,
including the occurrences near the loopback-IP checks, to combine the subject
and cause into one grammatical clause. Ensure each message reads like “DPU
{dpu_machine_id} needs discovery because it does not have a loopback IP yet,”
preserving the existing error behavior.
In `@crates/api-core/src/handlers/expected_rack.rs`:
- Line 47: Update the expected-rack test assertions around the relevant cases in
expected_rack tests to match the new lowercase error message, changing the
expected substring from “Unknown rack_profile_id” to “unknown rack_profile_id”
while preserving the rest of each assertion.
In `@crates/api-core/src/handlers/ib_partition.rs`:
- Line 322: Update the error message in the partition-key mapping around the
pool return handling to use “returned” instead of “return,” preserving the
existing wording and error behavior.
In `@crates/api-core/src/handlers/instance.rs`:
- Around line 1696-1697: Update the error message in the
rpc_try_from(mh_snapshot) failure path to accurately state that the snapshot
could not be converted into an instance, replacing the contradictory “can be
converted” wording while preserving the existing machine_id context.
In `@crates/api-core/src/instance/mod.rs`:
- Around line 454-455: Update the operator-facing error messages near the shown
message and the referenced OS-image handling block: change “not support VF” to
“VF {} is not supported,” “image OS” to “OS image `{}` does not exist,” and
“failed to get OS image error” to “failed to get OS image: {e}.” Preserve the
existing placeholders and error-reporting behavior.
---
Nitpick comments:
In `@crates/xtask/src/error_message_case.rs`:
- Around line 746-784: Extend the end_to_end_rewrite_reaches_format test into
table-driven cases covering both format! and format_args! in error positions and
non-error positions. Assert that error-position invocations are rewritten while
bare invocations remain unchanged, preserving interpolation text and the
existing log-macro coverage.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2fdbec3f-6a0f-4beb-a120-809564110273
📒 Files selected for processing (75)
crates/admin-cli/src/instance/reboot/cmd.rscrates/agent/src/agent_platform.rscrates/agent/src/containerd/command/bash_command.rscrates/agent/src/dpu/interface.rscrates/agent/src/dpu/link.rscrates/agent/src/dpu/route.rscrates/agent/src/ethernet_virtualization.rscrates/agent/src/extension_services/k8s_pod_handler.rscrates/agent/src/lib.rscrates/agent/src/main_loop.rscrates/agent/src/network_monitor.rscrates/agent/src/tests/full.rscrates/agent/src/tests/test_network_monitor.rscrates/agent/src/upgrade.rscrates/api-core/src/attestation/measured_boot.rscrates/api-core/src/attestation/mod.rscrates/api-core/src/attestation/tpm_ca_cert.rscrates/api-core/src/compat.rscrates/api-core/src/credentials/mod.rscrates/api-core/src/db_init.rscrates/api-core/src/dhcp/discover.rscrates/api-core/src/ethernet_virtualization.rscrates/api-core/src/handlers/api.rscrates/api-core/src/handlers/bmc_endpoint_explorer.rscrates/api-core/src/handlers/client_resolution.rscrates/api-core/src/handlers/component_manager.rscrates/api-core/src/handlers/compute_allocation.rscrates/api-core/src/handlers/credential.rscrates/api-core/src/handlers/dns.rscrates/api-core/src/handlers/dpf.rscrates/api-core/src/handlers/dpu.rscrates/api-core/src/handlers/expected_rack.rscrates/api-core/src/handlers/extension_service.rscrates/api-core/src/handlers/host_reprovisioning.rscrates/api-core/src/handlers/ib_partition.rscrates/api-core/src/handlers/instance.rscrates/api-core/src/handlers/instance_type.rscrates/api-core/src/handlers/machine.rscrates/api-core/src/handlers/machine_discovery.rscrates/api-core/src/handlers/machine_identity.rscrates/api-core/src/handlers/machine_interface.rscrates/api-core/src/handlers/machine_validation.rscrates/api-core/src/handlers/managed_host.rscrates/api-core/src/handlers/network_security_group.rscrates/api-core/src/handlers/network_segment.rscrates/api-core/src/handlers/nmxc_browse.rscrates/api-core/src/handlers/power_options.rscrates/api-core/src/handlers/pxe.rscrates/api-core/src/handlers/rack.rscrates/api-core/src/handlers/redfish.rscrates/api-core/src/handlers/site_explorer.rscrates/api-core/src/handlers/sku.rscrates/api-core/src/handlers/uefi.rscrates/api-core/src/handlers/vpc.rscrates/api-core/src/handlers/vpc_prefix.rscrates/api-core/src/instance/mod.rscrates/api-core/src/ipxe.rscrates/api-core/src/measured_boot/rpc/journal.rscrates/api-core/src/setup.rscrates/api-core/src/tests/ib_instance.rscrates/api-core/src/tests/instance.rscrates/api-core/src/tests/measured_boot.rscrates/api-core/src/tests/site_explorer.rscrates/api-core/src/tests/tpm_ca.rscrates/dns/src/lib.rscrates/dpf/build.rscrates/dpu-otel-agent/src/lib.rscrates/dsx-exchange-consumer/src/api_client.rscrates/firmware/src/downloader.rscrates/host-support/src/registration.rscrates/network/src/virtualization.rscrates/ssh-console-mock-api-server/src/api.rscrates/ssh-console/tests/util/ssh_client.rscrates/xtask/src/error_message_case.rscrates/xtask/src/workspace_deps.rs
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
chet
force-pushed
the
gh-issue-3468-02
branch
2 times, most recently
from
July 16, 2026 08:44
be3ef07 to
3fd3115
Compare
The `lint-error-messages` checker from NVIDIA#3566 only saw a message passed as a bare string literal, so anything wrapped in a `format!` slipped through -- `CarbideError::invalid_argument(format!("Duplicate ..."))`, `.context(format!("Failed ..."))`, and friends kept their capital lead because the literal hid inside the macro. That was a documented gap, and it was the exact class that drifted between source and test while NVIDIA#3566 was landing: the checker couldn't `--fix` the source, so a hand-edited assertion could silently disagree with it. This teaches the checker to peel a wrapping `format!` when it sits in a slot we already treat as an error message, then sweeps what it now reaches. Primary callouts are: - `leading_str_lit` now peels a `format!`/`format_args!` macro down to its format-string literal, but only in the argument slots the checker already trusts -- a `CarbideError`/`Status` constructor or a `.context`/`.wrap_err` method. A bare `format!` (a log line, a `flint` command echo) is never touched, and the existing `{placeholder}` handling leaves interpolation names like `{MachineId}` intact. - Ran `--fix` over what the widening reaches: it lowercased 246 previously-invisible messages across 69 files, all inside error constructs, with acronyms (`VPC`, `EK`), CamelCase types (`RsaPublicKey`, `MachineId`), and placeholders preserved. - Updated the test assertions that pinned the old strings -- attestation, tpm-ca, site/rack explorers, machine discovery/interfaces, instance allocation, ib-partition, and the duplicate-extension-services allocate path, whose `format!` source is now swept so it rejoins its already-lowercased twin. Manual `impl Display`/`write!` bodies and struct-literal error fields (`CarbideError::Internal { message }`) stay out of reach, still marked with the checker's `TODO` -- a further follow-up. Tests added! -- `end_to_end_rewrite_reaches_format` drives the real span -> byte-range -> splice path across a `CarbideError` constructor and a `.context`, checks that `{MachineId}` survives, and asserts a bare `format!`/log macro is left alone. This continues NVIDIA#3566 and supports NVIDIA#3468 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
nick-vetro
approved these changes
Jul 16, 2026
bcavnvidia
approved these changes
Jul 16, 2026
chet
added a commit
to chet/bare-metal-manager-core
that referenced
this pull request
Jul 16, 2026
…fix hint NVIDIA#3595 taught the `lint-error-messages` checker to peel a `format!` inside an expression-bodied closure (`.wrap_err_with(|| format!(...))`), but the common *block*-bodied form (`.wrap_err_with(|| { format!(...) })`) still slipped through -- the exact siblings CodeRabbit pointed at on NVIDIA#3595. This closes that gap, and while we're in the failure path, makes the "how do I fix this?" output impossible to miss for a contributor who trips the check. Primary callouts are: - `leading_str_lit` gains an `Expr::Block` arm that forwards to the block's tail expression, so a message behind `|| { format!(...) }` is reached just like the expression-bodied form. `--fix` then lowercased 19 messages across 5 files (agent file-ops, `k8s_pod_handler`, `hbn`, machine-controller scout scripts, and the xtask workspace-deps check). - The report-mode failure now prints the rule, the exact `cargo xtask lint-error-messages --fix` command on its own line, and the `// xtask:allow-error-case` opt-out for a deliberate capital -- instead of a single sentence tacked onto the violation count. Still out of reach, and marked in the checker `TODO` as the remaining follow-up: struct-literal error fields (`CarbideError::Internal { message: "..." }`, ~150 sites), manual `impl Display`/`write!` bodies, and macro-body-nested sites. Tests added! -- `end_to_end_rewrite_reaches_block_closure` drives the real span -> byte-range -> splice path through a `.wrap_err_with(|| { format!(...) })` and checks the `{host}` interpolation survives. This continues NVIDIA#3595 and supports NVIDIA#3468 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
chet
added a commit
that referenced
this pull request
Jul 16, 2026
…fix hint (#3613) ## Summary A small follow-up to #3595. That PR reached `format!` inside *expression*-bodied closures (`.wrap_err_with(|| format!(...))`); this closes the sibling gap for *block*-bodied closures (`.wrap_err_with(|| { format!(...) })`) — the exact form CodeRabbit flagged there — and makes the checker's failure output easy for a contributor to act on. - `leading_str_lit` gains an `Expr::Block` arm (forward to the block's tail expression), so `|| { format!(...) }` is reached just like `|| format!(...)`. `--fix` lowercased **19 messages across 5 files** (agent file-ops, `k8s_pod_handler`, `hbn`, machine-controller scout scripts, and the xtask workspace-deps check). - The report-mode failure now prints the rule, the exact `cargo xtask lint-error-messages --fix` command on its own line, and the `// xtask:allow-error-case` opt-out for a deliberate capital — instead of a one sentence tacked onto the violation count. Still deferred, and documented in the checker `TODO`: struct-literal error fields (`CarbideError::Internal { message: "..." }`, ~150 sites), manual `Display`/`write!` bodies, and macro-body-nested sites. Continues #3595 · supports #3468 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
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
3 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.
Summary
A follow-up to #3566, as part of adopting the Rust API Guidelines convention (C-GOOD-ERR): that extends the
lint-error-messageschecker to reach messages wrapped in aformat!. The original checker only saw a bare string literal, soCarbideError::invalid_argument(format!("Duplicate ..."))and.context(format!("Failed ..."))kept their capital lead -- and because the checker couldn't--fixthem, those were the sites that drifted between source and test as #3566 landed.leading_str_litnow peels aformat!/format_args!down to its format-string literal, but only in the slots the checker already trusts -- aCarbideError/Statusconstructor or a.context/.wrap_errmethod. A bareformat!(a log line, a command echo) is left alone, and{placeholder}names like{MachineId}are preserved.--fixthen lowercased 246 messages across 69 files that were previously invisible, with acronyms (VPC,EK), CamelCase types (RsaPublicKey), and placeholders preserved.format!source now rejoins its already-lowercased twin.Manual
impl Display/write!bodies and struct-literal error fields (CarbideError::Internal { message }) stay out of reach, still marked with the checker'sTODO-- a further follow-up.Continues #3566 · supports #3468