refactor: make tracing logs consistently structured and searchable#3479
Open
chet wants to merge 1 commit into
Open
refactor: make tracing logs consistently structured and searchable#3479chet wants to merge 1 commit into
chet wants to merge 1 commit into
Conversation
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Summary by CodeRabbit
WalkthroughThe pull request standardizes tracing across application, controller, service, library, mock, and test code by replacing interpolated messages with stable messages and structured fields. It also documents the conventions and exceptions in ChangesStructured tracing migration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.44.1)crates/api-web/src/lib.rsast-grep timed out on this file 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: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/ib-fabric/src/lib.rs (1)
387-438: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winModel
failure_stageas an enum.
failure_stagehas a closed set of values (build_client,health_check,load_ports, andload_partitions) but is represented as&'static str. Introduce an internal enum withDisplay, pass variants at these call sites, and log it with%failure_stageso invalid stage values cannot be introduced accidentally.As per coding guidelines, finite Rust values should be modeled with enums rather than raw strings.
Also applies to: 456-468
🤖 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/ib-fabric/src/lib.rs` around lines 387 - 438, Replace the raw `&'static str` failure-stage parameter used by `note_fabric_error` with an internal enum covering `build_client`, `health_check`, `load_ports`, and `load_partitions`; implement `Display` for that enum, pass the corresponding variants at every visible call site, and format the stage with `%failure_stage` in logging.Source: Coding guidelines
crates/nvlink-manager/src/lib.rs (1)
2381-2397: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not skip DB persistence for a created NMX-C partition. If
partition_idis missing or does not fiti32, this path records success and commits the transaction while leaving the new external partition without a database row. That leaves reconciliation with an inconsistent source of truth; either widen the stored ID type or fail the operation with a recovery path instead ofcontinueing.🤖 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/nvlink-manager/src/lib.rs` around lines 2381 - 2397, The NMX-C partition creation flow must not commit success when the partition ID is missing or cannot fit the database type. Update the handling around matching_partition.partition_id and the i32::try_from conversion to use a compatible wider stored ID or fail the operation through the existing recovery/error path, ensuring no successful transaction commits without persisting the created partition.
🧹 Nitpick comments (13)
crates/machine-controller/src/handler.rs (1)
4101-4101: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid discarding the
Resultwithlet _ =.This path intentionally treats rshim failure as non-blocking, but
let _ =obscures that intent. Use an explicitif let Err(e)branch (or.ok()) so the ignored failure is deliberate and readable.As per coding guidelines, intentionally ignored results should use
.ok()rather than discarding them withlet _.🤖 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/machine-controller/src/handler.rs` at line 4101, Update the rshim enablement error-handling chain around the tracing call to avoid discarding the Result with let _. Use .ok() to explicitly mark the non-blocking failure as intentionally ignored, while preserving the existing info-level error logging.Source: Coding guidelines
crates/agent/src/upgrade.rs (1)
72-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRecord errors directly instead of eagerly flattening them.
error = format!("{err:#}")allocates and converts the report into an untyped string, contrary to the structured-field convention. Useerror = ?errfor debug/chain context, or%errwhen display formatting is intended, including at Line 146.Also applies to: 83-86, 146-146
🤖 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/agent/src/upgrade.rs` around lines 72 - 75, Update the error fields in the upgrade error logs, including the branches around the shown log and line 146, to record the error directly with structured tracing syntax instead of eagerly calling format!. Use debug capture for chained report context (or display capture where explicitly intended), preserving the existing messages and control flow.Source: Coding guidelines
crates/api-core/src/scout_stream.rs (1)
103-106: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winApply tracing shorthand consistently across the refactor. These explicit same-name assignments are valid but redundant and conflict with the repository logging convention.
crates/api-core/src/scout_stream.rs#L103-L106: replacemachine_id = %machine_idwith%machine_id.crates/api-core/src/scout_stream.rs#L120-L125: replace same-namemachine_idandflow_uuidassignments with shorthand.crates/api-core/src/scout_stream.rs#L128-L133: replace same-namemachine_id,flow_uuid, andresponseassignments with shorthand.crates/api-core/src/scout_stream.rs#L140-L143: replacemachine_id = %machine_idwith%machine_id.crates/api-core/src/scout_stream.rs#L150-L153: replacemachine_id = %machine_idwith%machine_id.crates/api-core/src/scout_stream.rs#L156-L158: replacemachine_id = %machine_idwith%machine_id.crates/api-core/src/scout_stream.rs#L218-L220: replace same-namemachine_idandflow_uuidassignments with shorthand.crates/api-core/src/scout_stream.rs#L270-L274: replace same-namemachine_idandresponseassignments with shorthand.crates/api-core/src/scout_stream.rs#L278-L282: replace same-namemachine_idandflow_uuid_pbassignments with shorthand.crates/api-core/src/handlers/machine_discovery.rs#L421-L421: replacestable_machine_id = %stable_machine_idwith%stable_machine_id.crates/api-test-helper/src/machine_a_tron.rs#L78-L78: replacedesired_firmware = ?desired_firmwarewith?desired_firmware.crates/api-core/src/handlers/site_explorer.rs#L261-L261: replacebmc_ip = %bmc_ipwith%bmc_ip.crates/firmware/src/config.rs#L69-L69: replacekey = %keywith%key.crates/firmware/src/config.rs#L186-L186: replacefirmware_directory = ?firmware_directorywith?firmware_directory.crates/firmware/src/config.rs#L207-L207: replacemetadata_path = ?metadata_pathwith?metadata_path.As per coding guidelines, use native shorthand for
tracing::Valuefields.🤖 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/api-core/src/scout_stream.rs` around lines 103 - 106, Replace redundant same-name tracing field assignments with native shorthand while preserving each field’s existing `%` or `?` formatter: update machine_id, flow_uuid, response, and flow_uuid_pb in crates/api-core/src/scout_stream.rs at 103-106, 120-125, 128-133, 140-143, 150-153, 156-158, 218-220, 270-274, and 278-282; stable_machine_id in crates/api-core/src/handlers/machine_discovery.rs at 421-421; desired_firmware in crates/api-test-helper/src/machine_a_tron.rs at 78-78; bmc_ip in crates/api-core/src/handlers/site_explorer.rs at 261-261; and key, firmware_directory, and metadata_path in crates/firmware/src/config.rs at 69-69, 186-186, and 207-207.Source: Coding guidelines
crates/api-db/src/machine_interface.rs (1)
610-614: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreserve the relay list as a structured value.
relays = %relaystrrecords a preformatted comma-separated string instead of the existing&[IpAddr]. Userelays = ?relaysat both sites so the structured event retains the endpoint collection; keeprelaystronly where the warning logs still require it.Proposed change
tracing::info!( %mac_address, - relays = %relaystr, + relays = ?relays, "Found no existing machine with mac address using networks with relays", );As per coding guidelines and the PR objective, dynamic endpoint values should remain structured and searchable rather than being preformatted into a string.
Also applies to: 669-673
🤖 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/api-db/src/machine_interface.rs` around lines 610 - 614, Update the info log in the machine lookup flow, and the corresponding log at the other relay-reporting site, to record the original relays collection with structured debug formatting (`relays = ?relays`) instead of the preformatted relaystr string. Retain relaystr only for warning logs that explicitly require the formatted representation.Source: Coding guidelines
crates/agent/src/tests/full.rs (1)
339-339: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRetain the error as a structured tracing value.
format!("{e:#}")eagerly allocates and converts the report into an untyped string. Prefererror = ?eorerror = %e, depending on the desired rendering.Proposed fix
- tracing::error!(error = format!("{e:#}"), "Failed to start DPU agent"); + tracing::error!(error = ?e, "Failed to start DPU agent");As per coding guidelines, dynamic values should use tracing’s
%field/?fieldforms rather than pre-rendered strings.🤖 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/agent/src/tests/full.rs` at line 339, Update the tracing::error! call in the DPU agent startup failure path to pass the captured error e directly as a structured field, using tracing’s ? or % formatting syntax instead of format!("{e:#}"). Preserve the existing "Failed to start DPU agent" message.Sources: Coding guidelines, Path instructions
crates/api-core/src/machine_update_manager/host_firmware.rs (1)
76-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winApply native tracing shorthand consistently.
These fields are valid but unnecessarily spell out same-name variables. Use the shorthand forms required by the repository convention.
crates/api-core/src/machine_update_manager/host_firmware.rs#L76-L79: use?fw_config_snapshot.crates/api-core/src/machine_update_manager/host_firmware.rs#L189-L192: use?firmware_config.crates/api-core/src/tests/dpu_reprovisioning.rs#L1359-L1362: use?last_reboot_requested.crates/dpu-remediation/src/remediation.rs#L231-L234: use?error.crates/machine-a-tron/src/host_machine.rs#L407-L410: use?request.crates/machine-a-tron/src/host_machine.rs#L429-L430: usedpu_index.crates/machine-a-tron/src/host_machine.rs#L671-L674: use%machine_id.As per coding guidelines, use native shorthand for
tracing::Valuefields.🤖 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/api-core/src/machine_update_manager/host_firmware.rs` around lines 76 - 79, Update the affected tracing fields to use native same-name shorthand: in crates/api-core/src/machine_update_manager/host_firmware.rs lines 76-79 for fw_config_snapshot and lines 189-192 for firmware_config; crates/api-core/src/tests/dpu_reprovisioning.rs lines 1359-1362 for last_reboot_requested; crates/dpu-remediation/src/remediation.rs lines 231-234 for error; and crates/machine-a-tron/src/host_machine.rs lines 407-410 for request, 429-430 for dpu_index, and 671-674 for machine_id. Preserve the existing tracing levels and messages while using the appropriate debug or display shorthand.Sources: Coding guidelines, Path instructions
crates/api-core/src/handlers/svpc.rs (1)
424-428: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse Display formatting for the error field.
CarbideErroris logged with?ehere, while adjacent handlers use%e. Change this toerror = %efor consistent human-readable error output.🤖 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/api-core/src/handlers/svpc.rs` around lines 424 - 428, Update the tracing::error! call in the DPA device handler to format the CarbideError field with Display syntax, changing the error field from debug formatting to %e while preserving the existing device_info field and message.Source: Coding guidelines
crates/api-core/src/handlers/extension_service.rs (1)
143-146: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse one consistent error field name for credential cleanup failures.
The create/update paths emit
delete_error, while the delete path emitserrorfor the same failure class. Standardize on one key so log queries and dashboards do not need two schemas.This supports the PR objective of stable, searchable structured logging.
Also applies to: 390-393, 524-528
🤖 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/api-core/src/handlers/extension_service.rs` around lines 143 - 146, Standardize credential cleanup failure logging on the existing delete_error field name. Update the affected tracing::warn! calls in the create, update, and delete paths, including the referenced locations, so each uses delete_error for the deletion error value while preserving the existing messages and behavior.crates/api-core/src/handlers/astra.rs (1)
332-335: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStandardize Display formatting for String-valued tracing fields. These fields use Debug formatting for values that are plain strings, which can introduce quoting and inconsistent log output.
crates/api-core/src/handlers/astra.rs#L332-L335: changemac_address = ?obs.mac_addresstomac_address = %obs.mac_address.crates/scout/src/main.rs#L532-L535: use%device_action.pci_name.crates/scout/src/main.rs#L565-L568: use%dev_pci_name.crates/scout/src/main.rs#L588-L591: use%dev_pci_name.crates/scout/src/main.rs#L665-L668: use%dev_pci_name.As per coding guidelines, use
%fieldfor Display values and?fieldfor Debug values.🤖 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/api-core/src/handlers/astra.rs` around lines 332 - 335, Standardize tracing field formatting by using Display syntax for the String-valued fields: update mac_address in crates/api-core/src/handlers/astra.rs (lines 332-335), device_action.pci_name in crates/scout/src/main.rs (lines 532-535), and dev_pci_name in crates/scout/src/main.rs (lines 565-568, 588-591, and 665-668) from Debug formatting to Display formatting; leave other fields unchanged.Source: Coding guidelines
crates/dhcp-server/src/main.rs (1)
118-122: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse Display formatting for
listen_address.Line 119 uses
?listen_address, while Line 159 uses%listen_addressfor the sameSocketAddrfield. Use%listen_addressconsistently so this semantic field follows the repository’s formatting convention.Suggested fix
- ?listen_address, + %listen_address,As per coding guidelines, use
%fieldfor Display values and?fieldfor Debug values.🤖 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/dhcp-server/src/main.rs` around lines 118 - 122, Update the DHCP server listening log’s listen_address field in the tracing::info! call to use Display formatting with %listen_address instead of Debug formatting with ?listen_address, matching the existing formatting used for the same field elsewhere.Source: Coding guidelines
crates/agent/src/machine_inventory_updater.rs (1)
107-108: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse tracing’s
%/?formatter instead of pre-formatting the error.
error = format!("{e:#}")materializes a new string before tracing records the event and bypasses the repository’s structured-field convention. Recordedirectly witherror = %eorerror = ?e, depending on the required rendering.🤖 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/agent/src/machine_inventory_updater.rs` around lines 107 - 108, Update the error field in the update_agent_reported_inventory error log to pass the captured error directly through tracing’s formatter, using error = %e or error = ?e as appropriate, instead of pre-formatting it with format!("{e:#}").Sources: Coding guidelines, Path instructions
crates/mqttea/src/client/core.rs (1)
180-183: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse Display formatting for error fields.
These are error values, so record them as
error = %e/error = %cred_errrather than implementation-oriented Debug output.Proposed change
- error = ?e, + error = %e, - error!(error = ?e, "MQTT event loop connection error"); + error!(error = %e, "MQTT event loop connection error"); - error = ?cred_err, + error = %cred_err,As per coding guidelines, “Use native shorthand for
tracing::Value,%fieldforDisplay, and?fieldforDebug.”Also applies to: 304-305, 325-327
🤖 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/mqttea/src/client/core.rs` around lines 180 - 183, Update the error fields in the re-subscription and related error logs to use tracing’s Display formatter (`%`) instead of Debug formatter (`?`) for the error values `e` and `cred_err`. Apply this consistently to the occurrences around the fresh-session handling and the additional locations identified in the comment.Source: Coding guidelines
crates/preingestion-manager/src/lib.rs (1)
3244-3247: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHandle the ignored RSHIM result explicitly.
Using
map_errfor logging and then discarding the transformed result obscures that failure is intentionally non-fatal.Proposed change
- let _ = redfish_client - .enable_rshim_bmc() - .await - .map_err(|e| tracing::error!(error = %e, "Failed to enable RSHIM on BMC")); + if let Err(error) = redfish_client.enable_rshim_bmc().await { + tracing::error!(%error, "Failed to enable RSHIM on BMC"); + }As per coding guidelines, “Do not discard results with
let _unused = foo(); use.ok()when failure is intentionally ignored.”🤖 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/preingestion-manager/src/lib.rs` around lines 3244 - 3247, Update the enable_rshim_bmc call to handle its non-fatal failure explicitly: retain the existing error logging while converting the intentionally ignored result with .ok() instead of using map_err followed by let _. Locate this change in the RSHIM enablement flow around redfish_client.enable_rshim_bmc().await.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/rpc.rs`:
- Around line 196-199: Preserve the conversion error in both structured logs:
update the invalid UUID-type log in crates/admin-cli/src/rpc.rs lines 196-199
and the invalid machine-owner log in lines 228-231 to include the caught error
as the structured field error = %e, while retaining the existing raw numeric
fields.
In `@crates/agent/src/nvue.rs`:
- Around line 1049-1055: Preserve child-process stdout/stderr as rendered log
message payloads rather than structured fields. Update the affected logging
sites in crates/agent/src/nvue.rs at lines 1049-1055, 923, 1010, 1029, and 1068,
and crates/agent/src/hbn.rs at lines 131-135; each site requires the same change
so multiline output and escaping remain operator-visible.
In `@crates/api-core/src/handlers/bmc_endpoint_explorer.rs`:
- Around line 396-399: In the is_infinite_boot_enabled handler’s successful
tracing::info! call, rename the structured field from ip_address to
endpoint_address while continuing to use bmc_endpoint_request.ip_address as its
value; keep the existing log message and other fields unchanged.
In `@crates/api-core/src/handlers/svpc.rs`:
- Around line 361-364: Update the error logging in the SVPC handler to avoid
serializing complete API payloads or model objects: at
crates/api-core/src/handlers/svpc.rs lines 361-364 remove the full req field; at
lines 372-375 replace full rep logging with a fixed diagnostic message; at lines
402-405 log only bounded report metadata; at lines 414-417 log bounded
observation identifiers; at lines 424-425 log selected device fields instead of
devinfo; and at lines 434-437 log selected DPA identifiers instead of the full
object. Apply the same bounded report metadata approach at lines 640-643, using
filtered identifiers, counts, and error context throughout.
In `@crates/api-core/src/machine_validation/mod.rs`:
- Around line 283-289: In the machine validation metrics update flow, move the
tracing::debug! call containing "Machine validation metrics updated" to execute
after self.metric_holder.update_metrics(metrics). Keep the logged metric fields
unchanged.
In `@crates/api-core/src/tests/common/api_fixtures/mod.rs`:
- Around line 2313-2317: Update the tracing::trace! fields
instance_network_config_version and instance_config_version to log their Option
values with Debug formatting, preserving None rather than converting missing
values to empty strings; leave the existing network_config_version field
unchanged.
In `@crates/api-db/src/work_lock_manager.rs`:
- Around line 322-367: Keep the keepalive task represented in the production
lock lifecycle instead of dropping join_handle, so unexpected task termination
is observed and handled. Update the surrounding release/close flow for the lock
manager to retain and asynchronously await the handle, or register it with the
manager’s JoinSet; use compile-time #[cfg(test)] and #[cfg(not(test))] branches
rather than cfg!(test), preserving the existing test behavior.
In `@crates/dns-record/src/lib.rs`:
- Line 182: Update the logging call in the serial-generation flow to parse
formatted_data into a numeric u32 before recording it. Replace the
string-formatted serial field in the debug! event with the parsed numeric value,
preserving the existing “generated serial for DNS zone” message and handling the
parse result appropriately.
In `@crates/health/src/api_client.rs`:
- Around line 330-333: Update the tracing::debug! call in the machine-details
fetch flow to report the actual fetched chunk size using ids_chunk.len() instead
of the fixed 100 value, while preserving the existing “Fetched machine details”
message and other fields.
In `@crates/machine-a-tron/src/machine_state_machine.rs`:
- Around line 566-569: Update the tracing::warn! call in the relay fallback
branch to use neutral wording that covers relay non-response, send failure, and
channel cancellation. Replace the “did not answer” message while retaining the
primary_mac context and direct-request fallback behavior.
In `@crates/machine-controller/src/handler.rs`:
- Around line 3918-3923: In the firmware lookup error handling within the
inspect_err closure, remove the standalone tracing::error call and retain only
the structured tracing::error event that includes machine_id and error. Ensure
each failed DPU UEFI firmware lookup emits exactly one error event.
In `@crates/nvlink-manager/src/lib.rs`:
- Around line 1817-1821: Update the tracing::error! message in the
RemoveFromUnknownPartition handling to use neutral wording for any missing NMX-C
partition, rather than referring specifically to a default partition; preserve
the existing GPU, machine, and partition context fields.
In `@crates/preingestion-manager/src/lib.rs`:
- Around line 1358-1360: Update the generic error arm in the chassis-reset
handling to include the endpoint address as a structured tracing field with a
stable semantic name, alongside the existing error field. Preserve the current
failure message and behavior while making the address available for correlating
concurrent reset failures.
In `@crates/scout/src/attestation.rs`:
- Around line 162-165: Remove sensitive authentication data from structured
logs: in crates/scout/src/attestation.rs lines 162-165, update the activation
log in the credential activation flow to replace digest.value() with a
non-sensitive indicator or metadata; in
crates/machine-a-tron/src/mock_ssh_server.rs lines 288-303, remove the password
field from both authentication log entries while preserving their non-sensitive
context.
In `@crates/site-explorer/src/bmc_endpoint_explorer.rs`:
- Around line 889-893: Update the credential lookup error branches throughout
the affected logic, including the handlers near the visible NVOS admin lookup
and the listed repeated branches, so they no longer classify every Err as a
missing Vault entry. Preserve and log the underlying error with error = %e while
using a generic failed-to-load-credentials message, or match MissingCredentials
explicitly and retain the setup message only for that case.
In `@crates/site-explorer/src/lib.rs`:
- Line 2631: Update the log message near the site-explorer host activation flow
to replace the malformed contraction “isnt” with “isn't”, preserving the rest of
the message unchanged.
In `@crates/ssh-console/src/bmc/connection.rs`:
- Around line 122-125: Update the tracing::info! call in the BMC connection
override flow to stop logging the full connection_details value. Log only safe
diagnostic fields, or explicitly redact ssh_key_path while preserving
machine_or_instance_id and other necessary context.
---
Outside diff comments:
In `@crates/ib-fabric/src/lib.rs`:
- Around line 387-438: Replace the raw `&'static str` failure-stage parameter
used by `note_fabric_error` with an internal enum covering `build_client`,
`health_check`, `load_ports`, and `load_partitions`; implement `Display` for
that enum, pass the corresponding variants at every visible call site, and
format the stage with `%failure_stage` in logging.
In `@crates/nvlink-manager/src/lib.rs`:
- Around line 2381-2397: The NMX-C partition creation flow must not commit
success when the partition ID is missing or cannot fit the database type. Update
the handling around matching_partition.partition_id and the i32::try_from
conversion to use a compatible wider stored ID or fail the operation through the
existing recovery/error path, ensuring no successful transaction commits without
persisting the created partition.
---
Nitpick comments:
In `@crates/agent/src/machine_inventory_updater.rs`:
- Around line 107-108: Update the error field in the
update_agent_reported_inventory error log to pass the captured error directly
through tracing’s formatter, using error = %e or error = ?e as appropriate,
instead of pre-formatting it with format!("{e:#}").
In `@crates/agent/src/tests/full.rs`:
- Line 339: Update the tracing::error! call in the DPU agent startup failure
path to pass the captured error e directly as a structured field, using
tracing’s ? or % formatting syntax instead of format!("{e:#}"). Preserve the
existing "Failed to start DPU agent" message.
In `@crates/agent/src/upgrade.rs`:
- Around line 72-75: Update the error fields in the upgrade error logs,
including the branches around the shown log and line 146, to record the error
directly with structured tracing syntax instead of eagerly calling format!. Use
debug capture for chained report context (or display capture where explicitly
intended), preserving the existing messages and control flow.
In `@crates/api-core/src/handlers/astra.rs`:
- Around line 332-335: Standardize tracing field formatting by using Display
syntax for the String-valued fields: update mac_address in
crates/api-core/src/handlers/astra.rs (lines 332-335), device_action.pci_name in
crates/scout/src/main.rs (lines 532-535), and dev_pci_name in
crates/scout/src/main.rs (lines 565-568, 588-591, and 665-668) from Debug
formatting to Display formatting; leave other fields unchanged.
In `@crates/api-core/src/handlers/extension_service.rs`:
- Around line 143-146: Standardize credential cleanup failure logging on the
existing delete_error field name. Update the affected tracing::warn! calls in
the create, update, and delete paths, including the referenced locations, so
each uses delete_error for the deletion error value while preserving the
existing messages and behavior.
In `@crates/api-core/src/handlers/svpc.rs`:
- Around line 424-428: Update the tracing::error! call in the DPA device handler
to format the CarbideError field with Display syntax, changing the error field
from debug formatting to %e while preserving the existing device_info field and
message.
In `@crates/api-core/src/machine_update_manager/host_firmware.rs`:
- Around line 76-79: Update the affected tracing fields to use native same-name
shorthand: in crates/api-core/src/machine_update_manager/host_firmware.rs lines
76-79 for fw_config_snapshot and lines 189-192 for firmware_config;
crates/api-core/src/tests/dpu_reprovisioning.rs lines 1359-1362 for
last_reboot_requested; crates/dpu-remediation/src/remediation.rs lines 231-234
for error; and crates/machine-a-tron/src/host_machine.rs lines 407-410 for
request, 429-430 for dpu_index, and 671-674 for machine_id. Preserve the
existing tracing levels and messages while using the appropriate debug or
display shorthand.
In `@crates/api-core/src/scout_stream.rs`:
- Around line 103-106: Replace redundant same-name tracing field assignments
with native shorthand while preserving each field’s existing `%` or `?`
formatter: update machine_id, flow_uuid, response, and flow_uuid_pb in
crates/api-core/src/scout_stream.rs at 103-106, 120-125, 128-133, 140-143,
150-153, 156-158, 218-220, 270-274, and 278-282; stable_machine_id in
crates/api-core/src/handlers/machine_discovery.rs at 421-421; desired_firmware
in crates/api-test-helper/src/machine_a_tron.rs at 78-78; bmc_ip in
crates/api-core/src/handlers/site_explorer.rs at 261-261; and key,
firmware_directory, and metadata_path in crates/firmware/src/config.rs at 69-69,
186-186, and 207-207.
In `@crates/api-db/src/machine_interface.rs`:
- Around line 610-614: Update the info log in the machine lookup flow, and the
corresponding log at the other relay-reporting site, to record the original
relays collection with structured debug formatting (`relays = ?relays`) instead
of the preformatted relaystr string. Retain relaystr only for warning logs that
explicitly require the formatted representation.
In `@crates/dhcp-server/src/main.rs`:
- Around line 118-122: Update the DHCP server listening log’s listen_address
field in the tracing::info! call to use Display formatting with %listen_address
instead of Debug formatting with ?listen_address, matching the existing
formatting used for the same field elsewhere.
In `@crates/machine-controller/src/handler.rs`:
- Line 4101: Update the rshim enablement error-handling chain around the tracing
call to avoid discarding the Result with let _. Use .ok() to explicitly mark the
non-blocking failure as intentionally ignored, while preserving the existing
info-level error logging.
In `@crates/mqttea/src/client/core.rs`:
- Around line 180-183: Update the error fields in the re-subscription and
related error logs to use tracing’s Display formatter (`%`) instead of Debug
formatter (`?`) for the error values `e` and `cred_err`. Apply this consistently
to the occurrences around the fresh-session handling and the additional
locations identified in the comment.
In `@crates/preingestion-manager/src/lib.rs`:
- Around line 3244-3247: Update the enable_rshim_bmc call to handle its
non-fatal failure explicitly: retain the existing error logging while converting
the intentionally ignored result with .ok() instead of using map_err followed by
let _. Locate this change in the RSHIM enablement flow around
redfish_client.enable_rshim_bmc().await.
🪄 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: 154c8f77-5413-4f5a-a6e7-f698f4a3ea25
📒 Files selected for processing (271)
STYLE_GUIDE.mdcrates/admin-cli/src/machine/show/cmd.rscrates/admin-cli/src/rpc.rscrates/agent/src/agent_platform.rscrates/agent/src/astra_weave.rscrates/agent/src/containerd/container.rscrates/agent/src/containerd/image.rscrates/agent/src/dhcp_server_grpc_client.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/dpu_extension_service_observability.rscrates/agent/src/extension_services/k8s_pod_handler.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/instrumentation.rscrates/agent/src/lib.rscrates/agent/src/machine_inventory_updater.rscrates/agent/src/main_loop.rscrates/agent/src/managed_files.rscrates/agent/src/mtu.rscrates/agent/src/netlink.rscrates/agent/src/network_monitor.rscrates/agent/src/nvue.rscrates/agent/src/ovs.rscrates/agent/src/periodic_config_fetcher.rscrates/agent/src/tests/common/mod.rscrates/agent/src/tests/full.rscrates/agent/src/tests/test_network_monitor.rscrates/agent/src/traffic_intercept_bridging.rscrates/agent/src/upgrade.rscrates/api-core/src/attestation/measured_boot.rscrates/api-core/src/attestation/tpm_ca_cert.rscrates/api-core/src/db_init.rscrates/api-core/src/dhcp/discover.rscrates/api-core/src/dpa/handler.rscrates/api-core/src/dynamic_settings.rscrates/api-core/src/handlers/api.rscrates/api-core/src/handlers/astra.rscrates/api-core/src/handlers/attestation.rscrates/api-core/src/handlers/bmc_endpoint_explorer.rscrates/api-core/src/handlers/component_manager.rscrates/api-core/src/handlers/dns.rscrates/api-core/src/handlers/dpu_remediation.rscrates/api-core/src/handlers/expected_machine.rscrates/api-core/src/handlers/extension_service.rscrates/api-core/src/handlers/finder.rscrates/api-core/src/handlers/firmware.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_scout.rscrates/api-core/src/handlers/machine_validation.rscrates/api-core/src/handlers/rack.rscrates/api-core/src/handlers/redfish.rscrates/api-core/src/handlers/scout_stream.rscrates/api-core/src/handlers/site_explorer.rscrates/api-core/src/handlers/svpc.rscrates/api-core/src/handlers/uefi.rscrates/api-core/src/instance/mod.rscrates/api-core/src/ipxe.rscrates/api-core/src/listener.rscrates/api-core/src/logging/setup.rscrates/api-core/src/machine_update_manager/dpu_nic_firmware.rscrates/api-core/src/machine_update_manager/host_firmware.rscrates/api-core/src/machine_update_manager/mod.rscrates/api-core/src/machine_validation/mod.rscrates/api-core/src/measured_boot/metrics_collector/mod.rscrates/api-core/src/mqtt_state_change_hook/hook.rscrates/api-core/src/run.rscrates/api-core/src/scout_stream.rscrates/api-core/src/setup.rscrates/api-core/src/tests/common/api_fixtures/dpu.rscrates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-core/src/tests/common/api_fixtures/site_explorer.rscrates/api-core/src/tests/common/api_fixtures/test_machine/mod.rscrates/api-core/src/tests/dns.rscrates/api-core/src/tests/dpu_reprovisioning.rscrates/api-core/src/tests/instance.rscrates/api-core/src/tests/machine_network.rscrates/api-core/src/tests/sku.rscrates/api-core/tests/integration/machine_bmc_metadata.rscrates/api-db/src/bmc_metadata.rscrates/api-db/src/carbide_version.rscrates/api-db/src/dns/resource_record.rscrates/api-db/src/expected_power_shelf.rscrates/api-db/src/expected_switch.rscrates/api-db/src/instance.rscrates/api-db/src/lib.rscrates/api-db/src/machine.rscrates/api-db/src/machine_interface.rscrates/api-db/src/machine_topology.rscrates/api-db/src/measured_boot/bundle.rscrates/api-db/src/vpc_dpu_loopback.rscrates/api-db/src/work_lock_manager.rscrates/api-integration-tests/tests/lib.rscrates/api-model/src/dpa_interface/mod.rscrates/api-model/src/firmware.rscrates/api-model/src/instance/status/network.rscrates/api-model/src/instance/status/nvlink.rscrates/api-model/src/instance/status/spx.rscrates/api-model/src/machine/mod.rscrates/api-model/src/machine/nvlink.rscrates/api-model/src/machine/spx.rscrates/api-model/src/site_explorer/mod.rscrates/api-test-helper/src/domain.rscrates/api-test-helper/src/instance.rscrates/api-test-helper/src/machine.rscrates/api-test-helper/src/machine_a_tron.rscrates/api-test-helper/src/subnet.rscrates/api-test-helper/src/tenant.rscrates/api-test-helper/src/vpc.rscrates/api-test-helper/src/vpc_prefix.rscrates/api-web/src/auth.rscrates/api-web/src/interface.rscrates/api-web/src/lib.rscrates/api-web/src/machine.rscrates/api-web/src/managed_host.rscrates/api-web/src/search.rscrates/bmc-explorer/src/chassis.rscrates/bmc-explorer/src/computer_system.rscrates/bmc-explorer/src/manager.rscrates/bmc-mock/src/combined_server.rscrates/bmc-mock/src/combined_service.rscrates/bmc-mock/src/main.rscrates/bmc-mock/src/redfish/expander_router.rscrates/bmc-mock/src/tar_router.rscrates/bmc-mock/src/tls.rscrates/bmc-proxy/src/bmc_proxy.rscrates/bmc-proxy/src/setup.rscrates/dhcp-server/src/cache.rscrates/dhcp-server/src/grpc_server.rscrates/dhcp-server/src/main.rscrates/dhcp-server/src/modes/controller.rscrates/dhcp-server/src/packet_handler.rscrates/dhcp-server/src/util.rscrates/dns-record/src/lib.rscrates/dns/src/lib.rscrates/dns/src/main.rscrates/dpa-manager/src/card_handler/astra.rscrates/dpa-manager/src/card_handler/svpc.rscrates/dpa-manager/src/lib.rscrates/dpa/src/lib.rscrates/dpf/src/sdk.rscrates/dpu-otel-agent/src/lib.rscrates/dpu-remediation/src/remediation.rscrates/dsx-exchange-consumer/src/api_client.rscrates/firmware/src/config.rscrates/firmware/src/downloader.rscrates/fmds/src/http_request_metrics.rscrates/fmds/src/main.rscrates/fmds/src/nic_init.rscrates/fmds/src/phone_home.rscrates/health/src/api_client.rscrates/health/src/collectors/logs/periodic.rscrates/health/src/discovery/cleanup.rscrates/health/src/discovery/context.rscrates/health/src/discovery/spawn.rscrates/health/src/metrics.rscrates/host-support/src/hardware_enumeration.rscrates/host-support/src/hardware_enumeration/dpu.rscrates/host-support/src/hardware_enumeration/gpu.rscrates/host-support/src/hardware_enumeration/tpm.rscrates/host-support/src/registration.rscrates/http-connector/src/connector.rscrates/http-connector/src/resolver.rscrates/ib-fabric/src/lib.rscrates/ib-partition-controller/src/handler.rscrates/ipmi/src/tool.rscrates/kms-provider/src/providers/transit.rscrates/libmlx/src/device/discovery.rscrates/libnmxc/src/lib.rscrates/libnmxm/src/lib.rscrates/machine-a-tron/src/api_client.rscrates/machine-a-tron/src/api_throttler.rscrates/machine-a-tron/src/bmc_mock_wrapper.rscrates/machine-a-tron/src/dhcp_wrapper.rscrates/machine-a-tron/src/host_machine.rscrates/machine-a-tron/src/machine_a_tron.rscrates/machine-a-tron/src/machine_state_machine.rscrates/machine-a-tron/src/machine_utils.rscrates/machine-a-tron/src/main.rscrates/machine-a-tron/src/mock_ssh_server.rscrates/machine-a-tron/src/subnet.rscrates/machine-a-tron/src/tui.rscrates/machine-a-tron/src/vpc.rscrates/machine-controller/src/dpf.rscrates/machine-controller/src/handler.rscrates/machine-controller/src/handler/attestation.rscrates/machine-controller/src/handler/bios_config.rscrates/machine-controller/src/handler/dpf.rscrates/machine-controller/src/handler/machine_validation.rscrates/machine-controller/src/handler/power.rscrates/machine-controller/src/handler/sku.rscrates/machine-validation/src/machine_validation.rscrates/metrics-endpoint/src/lib.rscrates/metrics-utils/src/lib.rscrates/mqttea-example/src/main.rscrates/mqttea/src/client/core.rscrates/mqttea/src/client/messages.rscrates/mqttea/src/registry/core.rscrates/nvlink-manager/src/lib.rscrates/nvlink-manager/src/switch_cert_monitor.rscrates/power-shelf-controller/src/configuring.rscrates/power-shelf-controller/src/deleting.rscrates/power-shelf-controller/src/fetching_data.rscrates/power-shelf-controller/src/initializing.rscrates/preingestion-manager/src/bfb_rshim_copier.rscrates/preingestion-manager/src/lib.rscrates/preingestion-manager/tests/integration/host_bmc_firmware.rscrates/rack-controller/src/created.rscrates/rack-controller/src/discovering.rscrates/rack-controller/src/error_state.rscrates/rack-controller/src/handler.rscrates/rack-controller/src/lib.rscrates/rack-controller/src/maintenance.rscrates/rack-controller/src/ready.rscrates/rack-controller/src/validating.rscrates/rpc-utils/src/managed_host_display.rscrates/rpc/src/errors.rscrates/rpc/src/forge_tls_client.rscrates/rpc/src/model/dpu_remediation.rscrates/scout/src/attestation.rscrates/scout/src/deprovision/scrabbing.rscrates/scout/src/firmware_upgrade.rscrates/scout/src/main.rscrates/scout/src/mlx_device.rscrates/scout/src/register.rscrates/scout/src/stream.rscrates/scout/src/tpm.rscrates/secrets/src/forge_vault.rscrates/secrets/src/local_credentials/file.rscrates/site-explorer/src/bmc_endpoint_explorer.rscrates/site-explorer/src/explored_endpoint_index.rscrates/site-explorer/src/lib.rscrates/site-explorer/src/machine_creator.rscrates/site-explorer/src/redfish.rscrates/site-explorer/src/switch_creator.rscrates/site-explorer/src/test_support/mock_endpoint_explorer.rscrates/site-explorer/tests/integration/power_shelf.rscrates/site-explorer/tests/integration/site_explorer.rscrates/site-explorer/tests/integration/switch.rscrates/spdm-controller/src/handler.rscrates/ssh-console-mock-api-server/src/lib.rscrates/ssh-console/src/bmc/client.rscrates/ssh-console/src/bmc/connection.rscrates/ssh-console/src/bmc/connection_impl/ipmi.rscrates/ssh-console/src/bmc/connection_impl/ssh.rscrates/ssh-console/src/bmc/message_proxy.rscrates/ssh-console/src/config.rscrates/ssh-console/src/console_logger.rscrates/ssh-console/src/frontend.rscrates/ssh-console/src/metrics.rscrates/ssh-console/src/ssh_cert_parsing.rscrates/ssh-console/src/ssh_server.rscrates/ssh-console/tests/util/ipmi_sim.rscrates/ssh-console/tests/util/mod.rscrates/state-controller/src/controller/processor.rscrates/switch-controller/src/certificate.rscrates/switch-controller/src/configuring.rscrates/switch-controller/src/created.rscrates/switch-controller/src/deleting.rscrates/switch-controller/src/error_state.rscrates/switch-controller/src/initializing.rscrates/switch-controller/src/ready.rscrates/switch-controller/src/validating.rs
💤 Files with no reviewable changes (1)
- crates/health/src/discovery/context.rs
Contributor
Author
|
Addressed the actionable structured-logging, accuracy, and sensitive-field findings in amended commit 1a82cf6. Reverification is green for nightly formatting, all-target/all-feature Clippy, custom lints, and the expanded 16-library unit-test set. Two review-body concerns were verified against origin/main and intentionally kept out of this refactor:
The HBN child-process stderr remains a rendered payload under the STYLE_GUIDE.md exception for child-process streams, matching the cloud review request to preserve operator-visible presentation. @coderabbitai full_review, thanks! |
Contributor
|
I'll kick off a full review of all changes now to verify the amended commit. (=^・ω・^=) ✏️ Learnings added
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/rack-controller/src/created.rs (1)
95-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse native tracing field shorthand.
These fields should use
compute_count,switch_count, andpower_shelf_countdirectly rather than repeating the field names. As per coding guidelines, use native tracing field shorthand for tracing values.Proposed fix
- compute_count = compute_count, - switch_count = switch_count, - power_shelf_count = power_shelf_count, + compute_count, + switch_count, + power_shelf_count,🤖 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/rack-controller/src/created.rs` around lines 95 - 100, Update the tracing::info! invocation to use native field shorthand for compute_count, switch_count, and power_shelf_count, removing the repeated field-name assignments while preserving the existing rack_id field and message.Source: Coding guidelines
crates/bmc-mock/src/combined_service.rs (1)
114-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse native tracing field shorthand.
These fields can be written as
?forwarded_host,?host, and?authority. As per coding guidelines, use native tracing field shorthand for Debug values.Proposed fix
tracing::info!( - forwarded_host = ?forwarded_host, - host = ?host, - authority = ?authority, + ?forwarded_host, + ?host, + ?authority, "No BMC mock router is configured for request", );🤖 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/bmc-mock/src/combined_service.rs` around lines 114 - 118, Update the tracing::info! call in the combined service request-handling flow to use native shorthand for the Debug fields: forwarded_host, host, and authority. Preserve the existing field names, formatting behavior, and log message.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/agent/src/mtu.rs`:
- Around line 58-61: Preserve child-process stderr as rendered diagnostic
message text rather than moving it solely into structured fields. Update the MTU
query site at crates/agent/src/mtu.rs lines 58-61, the MTU update site at
crates/agent/src/mtu.rs lines 78-81, and the ip command diagnostic at
crates/agent/src/ethernet_virtualization.rs lines 1753-1757; retain any
structured command metadata separately.
---
Nitpick comments:
In `@crates/bmc-mock/src/combined_service.rs`:
- Around line 114-118: Update the tracing::info! call in the combined service
request-handling flow to use native shorthand for the Debug fields:
forwarded_host, host, and authority. Preserve the existing field names,
formatting behavior, and log message.
In `@crates/rack-controller/src/created.rs`:
- Around line 95-100: Update the tracing::info! invocation to use native field
shorthand for compute_count, switch_count, and power_shelf_count, removing the
repeated field-name assignments while preserving the existing rack_id field and
message.
🪄 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: fa8faf74-bf3c-42c5-8f53-55920d33c13f
📒 Files selected for processing (271)
STYLE_GUIDE.mdcrates/admin-cli/src/machine/show/cmd.rscrates/admin-cli/src/rpc.rscrates/agent/src/agent_platform.rscrates/agent/src/astra_weave.rscrates/agent/src/containerd/container.rscrates/agent/src/containerd/image.rscrates/agent/src/dhcp_server_grpc_client.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/dpu_extension_service_observability.rscrates/agent/src/extension_services/k8s_pod_handler.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/instrumentation.rscrates/agent/src/lib.rscrates/agent/src/machine_inventory_updater.rscrates/agent/src/main_loop.rscrates/agent/src/managed_files.rscrates/agent/src/mtu.rscrates/agent/src/netlink.rscrates/agent/src/network_monitor.rscrates/agent/src/nvue.rscrates/agent/src/ovs.rscrates/agent/src/periodic_config_fetcher.rscrates/agent/src/tests/common/mod.rscrates/agent/src/tests/full.rscrates/agent/src/tests/test_network_monitor.rscrates/agent/src/traffic_intercept_bridging.rscrates/agent/src/upgrade.rscrates/api-core/src/attestation/measured_boot.rscrates/api-core/src/attestation/tpm_ca_cert.rscrates/api-core/src/db_init.rscrates/api-core/src/dhcp/discover.rscrates/api-core/src/dpa/handler.rscrates/api-core/src/dynamic_settings.rscrates/api-core/src/handlers/api.rscrates/api-core/src/handlers/astra.rscrates/api-core/src/handlers/attestation.rscrates/api-core/src/handlers/bmc_endpoint_explorer.rscrates/api-core/src/handlers/component_manager.rscrates/api-core/src/handlers/dns.rscrates/api-core/src/handlers/dpu_remediation.rscrates/api-core/src/handlers/expected_machine.rscrates/api-core/src/handlers/extension_service.rscrates/api-core/src/handlers/finder.rscrates/api-core/src/handlers/firmware.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_scout.rscrates/api-core/src/handlers/machine_validation.rscrates/api-core/src/handlers/rack.rscrates/api-core/src/handlers/redfish.rscrates/api-core/src/handlers/scout_stream.rscrates/api-core/src/handlers/site_explorer.rscrates/api-core/src/handlers/svpc.rscrates/api-core/src/handlers/uefi.rscrates/api-core/src/instance/mod.rscrates/api-core/src/ipxe.rscrates/api-core/src/listener.rscrates/api-core/src/logging/setup.rscrates/api-core/src/machine_update_manager/dpu_nic_firmware.rscrates/api-core/src/machine_update_manager/host_firmware.rscrates/api-core/src/machine_update_manager/mod.rscrates/api-core/src/machine_validation/mod.rscrates/api-core/src/measured_boot/metrics_collector/mod.rscrates/api-core/src/mqtt_state_change_hook/hook.rscrates/api-core/src/run.rscrates/api-core/src/scout_stream.rscrates/api-core/src/setup.rscrates/api-core/src/tests/common/api_fixtures/dpu.rscrates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-core/src/tests/common/api_fixtures/site_explorer.rscrates/api-core/src/tests/common/api_fixtures/test_machine/mod.rscrates/api-core/src/tests/dns.rscrates/api-core/src/tests/dpu_reprovisioning.rscrates/api-core/src/tests/instance.rscrates/api-core/src/tests/machine_network.rscrates/api-core/src/tests/sku.rscrates/api-core/tests/integration/machine_bmc_metadata.rscrates/api-db/src/bmc_metadata.rscrates/api-db/src/carbide_version.rscrates/api-db/src/dns/resource_record.rscrates/api-db/src/expected_power_shelf.rscrates/api-db/src/expected_switch.rscrates/api-db/src/instance.rscrates/api-db/src/lib.rscrates/api-db/src/machine.rscrates/api-db/src/machine_interface.rscrates/api-db/src/machine_topology.rscrates/api-db/src/measured_boot/bundle.rscrates/api-db/src/vpc_dpu_loopback.rscrates/api-db/src/work_lock_manager.rscrates/api-integration-tests/tests/lib.rscrates/api-model/src/dpa_interface/mod.rscrates/api-model/src/firmware.rscrates/api-model/src/instance/status/network.rscrates/api-model/src/instance/status/nvlink.rscrates/api-model/src/instance/status/spx.rscrates/api-model/src/machine/mod.rscrates/api-model/src/machine/nvlink.rscrates/api-model/src/machine/spx.rscrates/api-model/src/site_explorer/mod.rscrates/api-test-helper/src/domain.rscrates/api-test-helper/src/instance.rscrates/api-test-helper/src/machine.rscrates/api-test-helper/src/machine_a_tron.rscrates/api-test-helper/src/subnet.rscrates/api-test-helper/src/tenant.rscrates/api-test-helper/src/vpc.rscrates/api-test-helper/src/vpc_prefix.rscrates/api-web/src/auth.rscrates/api-web/src/interface.rscrates/api-web/src/lib.rscrates/api-web/src/machine.rscrates/api-web/src/managed_host.rscrates/api-web/src/search.rscrates/bmc-explorer/src/chassis.rscrates/bmc-explorer/src/computer_system.rscrates/bmc-explorer/src/manager.rscrates/bmc-mock/src/combined_server.rscrates/bmc-mock/src/combined_service.rscrates/bmc-mock/src/main.rscrates/bmc-mock/src/redfish/expander_router.rscrates/bmc-mock/src/tar_router.rscrates/bmc-mock/src/tls.rscrates/bmc-proxy/src/bmc_proxy.rscrates/bmc-proxy/src/setup.rscrates/dhcp-server/src/cache.rscrates/dhcp-server/src/grpc_server.rscrates/dhcp-server/src/main.rscrates/dhcp-server/src/modes/controller.rscrates/dhcp-server/src/packet_handler.rscrates/dhcp-server/src/util.rscrates/dns-record/src/lib.rscrates/dns/src/lib.rscrates/dns/src/main.rscrates/dpa-manager/src/card_handler/astra.rscrates/dpa-manager/src/card_handler/svpc.rscrates/dpa-manager/src/lib.rscrates/dpa/src/lib.rscrates/dpf/src/sdk.rscrates/dpu-otel-agent/src/lib.rscrates/dpu-remediation/src/remediation.rscrates/dsx-exchange-consumer/src/api_client.rscrates/firmware/src/config.rscrates/firmware/src/downloader.rscrates/fmds/src/http_request_metrics.rscrates/fmds/src/main.rscrates/fmds/src/nic_init.rscrates/fmds/src/phone_home.rscrates/health/src/api_client.rscrates/health/src/collectors/logs/periodic.rscrates/health/src/discovery/cleanup.rscrates/health/src/discovery/context.rscrates/health/src/discovery/spawn.rscrates/health/src/metrics.rscrates/host-support/src/hardware_enumeration.rscrates/host-support/src/hardware_enumeration/dpu.rscrates/host-support/src/hardware_enumeration/gpu.rscrates/host-support/src/hardware_enumeration/tpm.rscrates/host-support/src/registration.rscrates/http-connector/src/connector.rscrates/http-connector/src/resolver.rscrates/ib-fabric/src/lib.rscrates/ib-partition-controller/src/handler.rscrates/ipmi/src/tool.rscrates/kms-provider/src/providers/transit.rscrates/libmlx/src/device/discovery.rscrates/libnmxc/src/lib.rscrates/libnmxm/src/lib.rscrates/machine-a-tron/src/api_client.rscrates/machine-a-tron/src/api_throttler.rscrates/machine-a-tron/src/bmc_mock_wrapper.rscrates/machine-a-tron/src/dhcp_wrapper.rscrates/machine-a-tron/src/host_machine.rscrates/machine-a-tron/src/machine_a_tron.rscrates/machine-a-tron/src/machine_state_machine.rscrates/machine-a-tron/src/machine_utils.rscrates/machine-a-tron/src/main.rscrates/machine-a-tron/src/mock_ssh_server.rscrates/machine-a-tron/src/subnet.rscrates/machine-a-tron/src/tui.rscrates/machine-a-tron/src/vpc.rscrates/machine-controller/src/dpf.rscrates/machine-controller/src/handler.rscrates/machine-controller/src/handler/attestation.rscrates/machine-controller/src/handler/bios_config.rscrates/machine-controller/src/handler/dpf.rscrates/machine-controller/src/handler/machine_validation.rscrates/machine-controller/src/handler/power.rscrates/machine-controller/src/handler/sku.rscrates/machine-validation/src/machine_validation.rscrates/metrics-endpoint/src/lib.rscrates/metrics-utils/src/lib.rscrates/mqttea-example/src/main.rscrates/mqttea/src/client/core.rscrates/mqttea/src/client/messages.rscrates/mqttea/src/registry/core.rscrates/nvlink-manager/src/lib.rscrates/nvlink-manager/src/switch_cert_monitor.rscrates/power-shelf-controller/src/configuring.rscrates/power-shelf-controller/src/deleting.rscrates/power-shelf-controller/src/fetching_data.rscrates/power-shelf-controller/src/initializing.rscrates/preingestion-manager/src/bfb_rshim_copier.rscrates/preingestion-manager/src/lib.rscrates/preingestion-manager/tests/integration/host_bmc_firmware.rscrates/rack-controller/src/created.rscrates/rack-controller/src/discovering.rscrates/rack-controller/src/error_state.rscrates/rack-controller/src/handler.rscrates/rack-controller/src/lib.rscrates/rack-controller/src/maintenance.rscrates/rack-controller/src/ready.rscrates/rack-controller/src/validating.rscrates/rpc-utils/src/managed_host_display.rscrates/rpc/src/errors.rscrates/rpc/src/forge_tls_client.rscrates/rpc/src/model/dpu_remediation.rscrates/scout/src/attestation.rscrates/scout/src/deprovision/scrabbing.rscrates/scout/src/firmware_upgrade.rscrates/scout/src/main.rscrates/scout/src/mlx_device.rscrates/scout/src/register.rscrates/scout/src/stream.rscrates/scout/src/tpm.rscrates/secrets/src/forge_vault.rscrates/secrets/src/local_credentials/file.rscrates/site-explorer/src/bmc_endpoint_explorer.rscrates/site-explorer/src/explored_endpoint_index.rscrates/site-explorer/src/lib.rscrates/site-explorer/src/machine_creator.rscrates/site-explorer/src/redfish.rscrates/site-explorer/src/switch_creator.rscrates/site-explorer/src/test_support/mock_endpoint_explorer.rscrates/site-explorer/tests/integration/power_shelf.rscrates/site-explorer/tests/integration/site_explorer.rscrates/site-explorer/tests/integration/switch.rscrates/spdm-controller/src/handler.rscrates/ssh-console-mock-api-server/src/lib.rscrates/ssh-console/src/bmc/client.rscrates/ssh-console/src/bmc/connection.rscrates/ssh-console/src/bmc/connection_impl/ipmi.rscrates/ssh-console/src/bmc/connection_impl/ssh.rscrates/ssh-console/src/bmc/message_proxy.rscrates/ssh-console/src/config.rscrates/ssh-console/src/console_logger.rscrates/ssh-console/src/frontend.rscrates/ssh-console/src/metrics.rscrates/ssh-console/src/ssh_cert_parsing.rscrates/ssh-console/src/ssh_server.rscrates/ssh-console/tests/util/ipmi_sim.rscrates/ssh-console/tests/util/mod.rscrates/state-controller/src/controller/processor.rscrates/switch-controller/src/certificate.rscrates/switch-controller/src/configuring.rscrates/switch-controller/src/created.rscrates/switch-controller/src/deleting.rscrates/switch-controller/src/error_state.rscrates/switch-controller/src/initializing.rscrates/switch-controller/src/ready.rscrates/switch-controller/src/validating.rs
💤 Files with no reviewable changes (1)
- crates/health/src/discovery/context.rs
🚧 Files skipped from review as they are similar to previous changes (247)
- crates/api-test-helper/src/machine_a_tron.rs
- crates/ssh-console/src/ssh_server.rs
- crates/api-test-helper/src/vpc_prefix.rs
- crates/api-model/src/instance/status/nvlink.rs
- crates/api-model/src/instance/status/spx.rs
- crates/api-db/src/dns/resource_record.rs
- crates/api-core/src/tests/machine_network.rs
- crates/metrics-utils/src/lib.rs
- crates/preingestion-manager/tests/integration/host_bmc_firmware.rs
- crates/fmds/src/phone_home.rs
- crates/agent/src/host_machine_id.rs
- crates/switch-controller/src/created.rs
- crates/dpu-otel-agent/src/lib.rs
- crates/site-explorer/src/test_support/mock_endpoint_explorer.rs
- crates/api-db/src/vpc_dpu_loopback.rs
- crates/api-core/src/dynamic_settings.rs
- crates/agent/src/netlink.rs
- crates/api-model/src/machine/nvlink.rs
- crates/dhcp-server/src/grpc_server.rs
- crates/api-model/src/machine/spx.rs
- crates/bmc-mock/src/tar_router.rs
- crates/ssh-console/src/bmc/connection_impl/ipmi.rs
- crates/api-core/src/tests/common/api_fixtures/test_machine/mod.rs
- crates/api-core/src/measured_boot/metrics_collector/mod.rs
- crates/power-shelf-controller/src/initializing.rs
- crates/agent/src/health/bgp.rs
- crates/agent/src/tests/full.rs
- crates/api-core/src/handlers/site_explorer.rs
- crates/agent/src/tests/common/mod.rs
- crates/api-core/src/handlers/machine_discovery.rs
- crates/dpa/src/lib.rs
- crates/machine-a-tron/src/api_throttler.rs
- crates/dns/src/main.rs
- crates/api-core/src/handlers/firmware.rs
- crates/api-web/src/lib.rs
- STYLE_GUIDE.md
- crates/api-test-helper/src/tenant.rs
- crates/dhcp-server/src/packet_handler.rs
- crates/api-db/src/expected_power_shelf.rs
- crates/api-test-helper/src/subnet.rs
- crates/metrics-endpoint/src/lib.rs
- crates/bmc-explorer/src/chassis.rs
- crates/api-web/src/auth.rs
- crates/switch-controller/src/error_state.rs
- crates/libmlx/src/device/discovery.rs
- crates/api-model/src/site_explorer/mod.rs
- crates/ib-partition-controller/src/handler.rs
- crates/ssh-console/src/bmc/message_proxy.rs
- crates/dhcp-server/src/modes/controller.rs
- crates/agent/src/dhcp_server_grpc_client.rs
- crates/agent/src/tests/test_network_monitor.rs
- crates/agent/src/lib.rs
- crates/api-core/src/machine_update_manager/host_firmware.rs
- crates/api-core/src/machine_update_manager/dpu_nic_firmware.rs
- crates/mqttea/src/client/messages.rs
- crates/rack-controller/src/discovering.rs
- crates/switch-controller/src/certificate.rs
- crates/agent/src/instance_metadata_endpoint.rs
- crates/machine-controller/src/handler/power.rs
- crates/scout/src/tpm.rs
- crates/api-web/src/interface.rs
- crates/dhcp-server/src/util.rs
- crates/fmds/src/main.rs
- crates/api-model/src/firmware.rs
- crates/api-core/src/logging/setup.rs
- crates/firmware/src/downloader.rs
- crates/bmc-proxy/src/setup.rs
- crates/api-core/src/tests/sku.rs
- crates/api-core/src/handlers/scout_stream.rs
- crates/machine-a-tron/src/dhcp_wrapper.rs
- crates/api-test-helper/src/domain.rs
- crates/bmc-mock/src/redfish/expander_router.rs
- crates/ssh-console/tests/util/ipmi_sim.rs
- crates/api-db/src/bmc_metadata.rs
- crates/agent/src/extension_services/dpu_extension_service_observability.rs
- crates/nvlink-manager/src/switch_cert_monitor.rs
- crates/dhcp-server/src/cache.rs
- crates/health/src/metrics.rs
- crates/libnmxc/src/lib.rs
- crates/api-core/src/handlers/uefi.rs
- crates/api-web/src/search.rs
- crates/rack-controller/src/lib.rs
- crates/ssh-console/src/config.rs
- crates/api-core/src/tests/common/api_fixtures/dpu.rs
- crates/agent/src/periodic_config_fetcher.rs
- crates/fmds/src/nic_init.rs
- crates/api-core/src/handlers/dpu_remediation.rs
- crates/api-core/src/handlers/instance_type.rs
- crates/api-core/src/machine_update_manager/mod.rs
- crates/api-core/src/tests/common/api_fixtures/site_explorer.rs
- crates/site-explorer/tests/integration/switch.rs
- crates/switch-controller/src/configuring.rs
- crates/site-explorer/src/switch_creator.rs
- crates/api-db/src/measured_boot/bundle.rs
- crates/rack-controller/src/error_state.rs
- crates/ssh-console/src/metrics.rs
- crates/agent/src/machine_inventory_updater.rs
- crates/api-core/src/handlers/component_manager.rs
- crates/api-core/src/ipxe.rs
- crates/api-core/src/handlers/expected_machine.rs
- crates/agent/src/agent_platform.rs
- crates/scout/src/firmware_upgrade.rs
- crates/scout/src/register.rs
- crates/ipmi/src/tool.rs
- crates/api-core/src/handlers/finder.rs
- crates/ssh-console/src/bmc/connection_impl/ssh.rs
- crates/api-core/src/handlers/machine_scout.rs
- crates/machine-a-tron/src/tui.rs
- crates/health/src/api_client.rs
- crates/api-core/src/dhcp/discover.rs
- crates/firmware/src/config.rs
- crates/api-db/src/machine_interface.rs
- crates/api-core/src/attestation/measured_boot.rs
- crates/machine-controller/src/dpf.rs
- crates/ssh-console-mock-api-server/src/lib.rs
- crates/api-web/src/machine.rs
- crates/rpc/src/model/dpu_remediation.rs
- crates/dns-record/src/lib.rs
- crates/api-core/src/handlers/attestation.rs
- crates/agent/src/dpu/link.rs
- crates/switch-controller/src/initializing.rs
- crates/site-explorer/tests/integration/site_explorer.rs
- crates/api-core/src/handlers/redfish.rs
- crates/ssh-console/src/bmc/connection.rs
- crates/rpc/src/errors.rs
- crates/machine-a-tron/src/api_client.rs
- crates/api-core/src/listener.rs
- crates/site-explorer/src/explored_endpoint_index.rs
- crates/api-db/src/machine_topology.rs
- crates/api-core/src/tests/common/api_fixtures/mod.rs
- crates/api-db/src/expected_switch.rs
- crates/bmc-mock/src/tls.rs
- crates/site-explorer/src/redfish.rs
- crates/mqttea/src/registry/core.rs
- crates/agent/src/network_monitor.rs
- crates/admin-cli/src/machine/show/cmd.rs
- crates/machine-controller/src/handler/bios_config.rs
- crates/rack-controller/src/handler.rs
- crates/api-test-helper/src/vpc.rs
- crates/api-core/src/attestation/tpm_ca_cert.rs
- crates/http-connector/src/resolver.rs
- crates/bmc-explorer/src/manager.rs
- crates/site-explorer/src/machine_creator.rs
- crates/machine-a-tron/src/main.rs
- crates/ssh-console/src/bmc/client.rs
- crates/api-core/src/handlers/extension_service.rs
- crates/health/src/discovery/cleanup.rs
- crates/scout/src/stream.rs
- crates/machine-a-tron/src/bmc_mock_wrapper.rs
- crates/machine-controller/src/handler/machine_validation.rs
- crates/api-core/src/tests/instance.rs
- crates/api-test-helper/src/instance.rs
- crates/ssh-console/src/console_logger.rs
- crates/state-controller/src/controller/processor.rs
- crates/api-core/src/machine_validation/mod.rs
- crates/rpc/src/forge_tls_client.rs
- crates/api-model/src/machine/mod.rs
- crates/dpa-manager/src/lib.rs
- crates/bmc-mock/src/main.rs
- crates/host-support/src/hardware_enumeration/tpm.rs
- crates/api-core/src/handlers/dns.rs
- crates/dns/src/lib.rs
- crates/health/src/collectors/logs/periodic.rs
- crates/mqttea-example/src/main.rs
- crates/api-db/src/instance.rs
- crates/api-core/src/mqtt_state_change_hook/hook.rs
- crates/dsx-exchange-consumer/src/api_client.rs
- crates/agent/src/ovs.rs
- crates/dpu-remediation/src/remediation.rs
- crates/agent/src/astra_weave.rs
- crates/api-model/src/instance/status/network.rs
- crates/machine-a-tron/src/vpc.rs
- crates/api-web/src/managed_host.rs
- crates/spdm-controller/src/handler.rs
- crates/agent/src/containerd/image.rs
- crates/machine-validation/src/machine_validation.rs
- crates/ssh-console/tests/util/mod.rs
- crates/api-core/src/run.rs
- crates/api-core/src/handlers/machine_validation.rs
- crates/ssh-console/src/ssh_cert_parsing.rs
- crates/libnmxm/src/lib.rs
- crates/bmc-mock/src/combined_server.rs
- crates/secrets/src/forge_vault.rs
- crates/agent/src/containerd/container.rs
- crates/api-core/src/tests/dns.rs
- crates/host-support/src/hardware_enumeration/dpu.rs
- crates/agent/src/upgrade.rs
- crates/host-support/src/registration.rs
- crates/api-core/src/handlers/machine.rs
- crates/secrets/src/local_credentials/file.rs
- crates/dpa-manager/src/card_handler/svpc.rs
- crates/machine-controller/src/handler/attestation.rs
- crates/machine-controller/src/handler/sku.rs
- crates/health/src/discovery/spawn.rs
- crates/kms-provider/src/providers/transit.rs
- crates/preingestion-manager/src/bfb_rshim_copier.rs
- crates/site-explorer/tests/integration/power_shelf.rs
- crates/rack-controller/src/ready.rs
- crates/machine-a-tron/src/mock_ssh_server.rs
- crates/http-connector/src/connector.rs
- crates/api-model/src/dpa_interface/mod.rs
- crates/admin-cli/src/rpc.rs
- crates/bmc-proxy/src/bmc_proxy.rs
- crates/agent/src/dpu/interface.rs
- crates/agent/src/instrumentation.rs
- crates/api-core/src/tests/dpu_reprovisioning.rs
- crates/machine-a-tron/src/machine_a_tron.rs
- crates/rpc-utils/src/managed_host_display.rs
- crates/dpa-manager/src/card_handler/astra.rs
- crates/machine-a-tron/src/machine_utils.rs
- crates/api-core/src/handlers/astra.rs
- crates/rack-controller/src/maintenance.rs
- crates/api-core/src/handlers/instance.rs
- crates/scout/src/attestation.rs
- crates/api-integration-tests/tests/lib.rs
- crates/api-core/src/dpa/handler.rs
- crates/api-core/src/scout_stream.rs
- crates/fmds/src/http_request_metrics.rs
- crates/ssh-console/src/frontend.rs
- crates/machine-a-tron/src/machine_state_machine.rs
- crates/api-db/src/carbide_version.rs
- crates/ib-fabric/src/lib.rs
- crates/api-core/src/instance/mod.rs
- crates/dhcp-server/src/main.rs
- crates/api-core/src/handlers/svpc.rs
- crates/scout/src/deprovision/scrabbing.rs
- crates/scout/src/main.rs
- crates/bmc-explorer/src/computer_system.rs
- crates/machine-a-tron/src/host_machine.rs
- crates/host-support/src/hardware_enumeration.rs
- crates/api-db/src/lib.rs
- crates/dpf/src/sdk.rs
- crates/api-core/src/handlers/bmc_endpoint_explorer.rs
- crates/agent/src/dpu/route.rs
- crates/agent/src/extension_services/k8s_pod_handler.rs
- crates/agent/src/main_loop.rs
- crates/api-db/src/machine.rs
- crates/api-core/src/setup.rs
- crates/agent/src/health.rs
- crates/scout/src/mlx_device.rs
- crates/nvlink-manager/src/lib.rs
- crates/agent/src/traffic_intercept_bridging.rs
- crates/site-explorer/src/bmc_endpoint_explorer.rs
- crates/site-explorer/src/lib.rs
- crates/rack-controller/src/validating.rs
- crates/preingestion-manager/src/lib.rs
- crates/machine-controller/src/handler.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/agent/src/health.rs (1)
370-390: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not overwrite a failed file check with a pass.
When a non-DHCP file is below
MIN_SIZE, this branch recordsfailed(...), then falls through to the unconditionalpassed(...)call. That can report an invalid file as healthy. Return after recording the failure, or makepassed(...)conditional on the size check.Proposed fix
failed( hr, probe_ids::FileIsValid.clone(), Some(filename.to_string()), "Too small".to_string(), ); + continue; } passed(🤖 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/agent/src/health.rs` around lines 370 - 390, Update the file-check logic around the non-DHCP `stat.len() < MIN_SIZE` branch so it cannot execute the unconditional `passed(...)` call after recording `failed(...)`. Return immediately after the failure, or otherwise make `passed(...)` conditional, while preserving the DHCP size handling and valid-file pass behavior.
🧹 Nitpick comments (8)
crates/bmc-mock/src/main.rs (1)
154-157: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse tracing shorthand for
exit_code.
exit_codeis already a tracing value; emit it asexit_coderather thanexit_code = exit_codeto follow the repository’s structured-field convention.Proposed adjustment
- exit_code = exit_code, + exit_code,🤖 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/bmc-mock/src/main.rs` around lines 154 - 157, Update the tracing::error! invocation for the virsh reboot failure to use tracing’s shorthand field syntax for the existing exit_code value, replacing the explicit exit_code = exit_code form while preserving the message and logging behavior.Source: Coding guidelines
crates/api-db/src/carbide_version.rs (1)
64-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse native shorthand for the structured field.
Replace
superseded_count = superseded_countwithsuperseded_count. As per coding guidelines, tracingValuefields should use native shorthand.Proposed fix
tracing::warn!( version, - superseded_count = superseded_count, + superseded_count, "observed a new forge version, superseded versions", );🤖 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/api-db/src/carbide_version.rs` at line 64, Update the structured tracing field in the relevant logging statement to use the native shorthand `superseded_count` instead of the explicit `superseded_count = superseded_count` form.Source: Coding guidelines
crates/api-core/src/listener.rs (1)
288-291: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse native shorthand for same-named tracing fields.
These fields repeat a local variable name unnecessarily. Replace them with tracing shorthand while preserving the same field names and formatters:
crates/api-core/src/listener.rs#L288-L291: replacetrust_config = ?trust_configwith?trust_config.crates/api-core/src/listener.rs#L433-L437: replaceerror = %errorwith%error; retainerror_debug = ?error.crates/rack-controller/src/handler.rs#L113-L116: replacecontroller_state = %controller_statewith%controller_state.crates/dpa/src/lib.rs#L73-L77: replacetopic = %topicwith%topic.As per coding guidelines, tracing events should use native shorthand for
tracing::Valuefields.🤖 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/api-core/src/listener.rs` around lines 288 - 291, Update the tracing fields to use native shorthand while preserving their existing names and formatters: in crates/api-core/src/listener.rs lines 288-291, change trust_config = ?trust_config to ?trust_config; in crates/api-core/src/listener.rs lines 433-437, change error = %error to %error while retaining error_debug = ?error; in crates/rack-controller/src/handler.rs lines 113-116, change controller_state = %controller_state to %controller_state; and in crates/dpa/src/lib.rs lines 73-77, change topic = %topic to %topic.Source: Coding guidelines
crates/api-db/src/vpc_dpu_loopback.rs (1)
61-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse native tracing shorthand for same-name fields.
These structured fields repeat the variable name unnecessarily. Native shorthand preserves the field name and formatting while matching the repository convention.
crates/api-db/src/vpc_dpu_loopback.rs#L61-L61: replaceadmin_segments = ?admin_segmentswith?admin_segments.crates/api-model/src/firmware.rs#L146-L146: replacefirmware_type = ?firmware_typewith?firmware_type.crates/api-model/src/instance/status/network.rs#L224-L225: replaceconfig = ?configwith?config.🤖 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/api-db/src/vpc_dpu_loopback.rs` at line 61, Use native tracing shorthand for the same-name structured fields: replace admin_segments = ?admin_segments with ?admin_segments in crates/api-db/src/vpc_dpu_loopback.rs:61, firmware_type = ?firmware_type with ?firmware_type in crates/api-model/src/firmware.rs:146, and config = ?config with ?config in crates/api-model/src/instance/status/network.rs:224-225. Preserve the existing field values and formatting.Source: Coding guidelines
crates/health/src/discovery/cleanup.rs (1)
27-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winModel the closed stop-reason set as an enum.
stop_reasonnow accepts arbitrary&'static str, while the shown call sites pass fixed values from a finite set. Introduce aCollectorStopReasonenum with aDisplayimplementation and log it with%stop_reason.As per coding guidelines, finite sets should use enums with string conversion through
Display/FromStr, rather than raw strings.Also applies to: 53-53, 92-97
🤖 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/health/src/discovery/cleanup.rs` around lines 27 - 38, Replace the raw &'static str stop_reason parameter in the cleanup function with a CollectorStopReason enum covering all existing fixed reason values. Implement Display (and FromStr if required by the surrounding API), update all affected call sites to use the enum variants, and log stop_reason with % formatting in the tracing::info! call.Source: Coding guidelines
crates/dns/src/lib.rs (1)
625-625: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog the underlying server error in the structured field.
Line 625 stores the preformatted
error_msg, duplicating the static event message and makingerrorless useful for structured searches. Keeperror_msgfor the returned report, but logerror = %ehere.Proposed fix
- error!(error = %error_msg, "Carbide-dns has encountered an error"); + error!(error = %e, "Carbide-dns has encountered an error");🤖 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/dns/src/lib.rs` at line 625, Update the error logging statement in the surrounding error-handling flow to use the underlying server error variable e for the structured error field, while retaining error_msg for the returned report. Keep the existing static log message unchanged.Source: Coding guidelines
crates/machine-a-tron/src/machine_state_machine.rs (1)
984-997: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer a structured
?requestfield over a pre-formatteddetailstring.
detailre-embeds the whole formatted sentence as one opaque field, defeating the purpose of structuring this log — filtering by the specific unsupportedSystemPowerControlvariant now requires string matching instead of field matching.♻️ Proposed refactor
PushPowerButton | Nmi | Suspend | Pause | Resume => { let msg = format!("Machine-a-tron mock: unsupported power request {request:?}",); tracing::warn!( - detail = %msg, + ?request, "unsupported machine-a-tron mock power request", ); return Err(SetSystemPowerError::BadRequest(msg)); }As per coding guidelines, "record dynamic values as structured fields rather than interpolated into messages."
🤖 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/machine-a-tron/src/machine_state_machine.rs` around lines 984 - 997, The unsupported-request branch in set_system_power should log the SystemPowerControl value as a structured request field instead of pre-formatting it into the detail field. Preserve the existing warning message and BadRequest error text, but pass the matched request directly to tracing so individual variants remain filterable.Source: Coding guidelines
crates/mqttea/src/client/core.rs (1)
717-725: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBackoff delay is a duration — consider a
carbide_instrument::Eventinstead ofwarn!.
delay_msis fired on every backoff step with no paired counter/histogram (unlike the "queue full" branch, which incrementsqueue_stats_producer). As per coding guidelines, "use plain structuredtracing::macros for ordinary logging; define and emit acarbide_instrument::Eventwhen an event needs a count, rate, or duration," and this file already emits events (MqttReconnected,HandlerDispatchDropped) for comparable occurrences.🤖 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/mqttea/src/client/core.rs` around lines 717 - 725, Update the next_delay method to emit an appropriate carbide_instrument::Event for each backoff delay instead of using warn!. Follow the existing event patterns in this file, include the delay duration in the event fields, and preserve the current backoff calculation and returned delay.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/agent/src/health.rs`:
- Line 597: Update the warning message in parse_disk_utilization from “du status
line too short” to identify the df status line, leaving the surrounding parsing
and logging behavior unchanged.
In `@crates/api-core/src/handlers/instance.rs`:
- Around line 1390-1393: Update the tracing::debug! event in the instance update
method to use the established spx_config field name instead of spxconfig,
matching the earlier SPX configuration log field while leaving the surrounding
log message unchanged.
In `@crates/api-core/src/handlers/site_explorer.rs`:
- Around line 260-263: Update the tracing::info! call in the site exploration
error-clearing flow to replace the unclear message with a stable, grammatically
clear human-readable literal describing that failed preingestion was reset after
clearing the site exploration error; keep the %bmc_ip field unchanged.
In `@crates/api-core/tests/integration/machine_bmc_metadata.rs`:
- Around line 103-105: Update the “Looking up BMC credentials” log in the
relevant test flow to stop serializing the complete request; log only a safe
discriminator or use the repository’s filtered request-logging helper, ensuring
machine IDs and BMC IP/MAC selectors are excluded.
In `@crates/api-test-helper/src/instance.rs`:
- Around line 288-291: Remove the leading tab character from both log message
literals in the instance state reporting code, including the tracing call using
the “Current instance state” message and the corresponding occurrence near the
similarly reported state. Keep the structured logging fields and message text
otherwise unchanged.
- Around line 34-38: Update the tracing::info! message in the instance creation
flow to remove the duplicated “with” and use concise, natural phrasing while
preserving the existing host_machine_id and network_segment_id fields.
In `@crates/dns/src/main.rs`:
- Around line 66-69: Move the OpenTelemetry startup log in the initialization
flow to after tracing_subscriber::registry().try_init() completes successfully.
Preserve the existing endpoint field and message, ensuring the event is emitted
through the configured tracing pipeline.
In `@crates/host-support/src/registration.rs`:
- Line 313: Update the tracing::info! call that logs the newly written machine
certificate in the registration flow to format client_cert with Display syntax
(%client_cert) instead of Debug syntax (?client_cert), preserving the existing
message and behavior.
In `@crates/machine-controller/src/handler.rs`:
- Around line 11234-11237: Add the `machine_id` field to the `tracing::warn!`
call for the AC powercycle failure, sourcing it from the in-scope `mh_snapshot`
value while preserving the existing error field and message.
- Around line 9503-9508: Add the in-scope machine_id field to the
tracing::debug! call for "Moving firmware chain step to CheckingFirmware",
matching the field naming and formatting used by sibling logs in
host_waiting_fw. Preserve the existing firmware, firmware_number, and address
fields.
In `@crates/preingestion-manager/src/lib.rs`:
- Around line 3307-3314: Add address = %endpoint_clone.address to both the
multipart-not-supported tracing::warn! call and the failed-to-open-firmware-file
tracing::error! call within initiate_update, preserving their existing error
fields and messages.
In `@crates/rpc/src/forge_tls_client.rs`:
- Around line 691-694: Update the retry log message in retry_build_nmx_c to
identify the NMX-C API instead of the Forge API, matching the existing
post-retry log wording while leaving the error and URL fields unchanged.
In `@crates/site-explorer/src/lib.rs`:
- Around line 3223-3225: The Viking ingestion warnings are misleading: update
the warning at crates/site-explorer/src/lib.rs lines 3223-3225 to state that the
manager report is missing, since this branch checks report.managers.first();
update the warning at lines 3247-3250 to say the CPLD version does not match,
unless the code is changed to compare ordering before claiming it is lower.
In `@crates/spdm-controller/src/handler.rs`:
- Around line 243-246: Update the tracing::error! call in the unexpected
task-state branch of the measurement handling flow to describe the measurement
collection task entering an unexpected state, rather than failing to trigger
measurement. Preserve the existing error and task_state fields.
In `@crates/ssh-console/tests/util/mod.rs`:
- Line 195: Update the tracing::debug call for the “Configured baseline test
mock hosts” message to avoid logging mock_hosts directly, since MockHost debug
output includes bmc_password. Log only a safe aggregate such as the number of
configured hosts or another redacted summary.
---
Outside diff comments:
In `@crates/agent/src/health.rs`:
- Around line 370-390: Update the file-check logic around the non-DHCP
`stat.len() < MIN_SIZE` branch so it cannot execute the unconditional
`passed(...)` call after recording `failed(...)`. Return immediately after the
failure, or otherwise make `passed(...)` conditional, while preserving the DHCP
size handling and valid-file pass behavior.
---
Nitpick comments:
In `@crates/api-core/src/listener.rs`:
- Around line 288-291: Update the tracing fields to use native shorthand while
preserving their existing names and formatters: in
crates/api-core/src/listener.rs lines 288-291, change trust_config =
?trust_config to ?trust_config; in crates/api-core/src/listener.rs lines
433-437, change error = %error to %error while retaining error_debug = ?error;
in crates/rack-controller/src/handler.rs lines 113-116, change controller_state
= %controller_state to %controller_state; and in crates/dpa/src/lib.rs lines
73-77, change topic = %topic to %topic.
In `@crates/api-db/src/carbide_version.rs`:
- Line 64: Update the structured tracing field in the relevant logging statement
to use the native shorthand `superseded_count` instead of the explicit
`superseded_count = superseded_count` form.
In `@crates/api-db/src/vpc_dpu_loopback.rs`:
- Line 61: Use native tracing shorthand for the same-name structured fields:
replace admin_segments = ?admin_segments with ?admin_segments in
crates/api-db/src/vpc_dpu_loopback.rs:61, firmware_type = ?firmware_type with
?firmware_type in crates/api-model/src/firmware.rs:146, and config = ?config
with ?config in crates/api-model/src/instance/status/network.rs:224-225.
Preserve the existing field values and formatting.
In `@crates/bmc-mock/src/main.rs`:
- Around line 154-157: Update the tracing::error! invocation for the virsh
reboot failure to use tracing’s shorthand field syntax for the existing
exit_code value, replacing the explicit exit_code = exit_code form while
preserving the message and logging behavior.
In `@crates/dns/src/lib.rs`:
- Line 625: Update the error logging statement in the surrounding error-handling
flow to use the underlying server error variable e for the structured error
field, while retaining error_msg for the returned report. Keep the existing
static log message unchanged.
In `@crates/health/src/discovery/cleanup.rs`:
- Around line 27-38: Replace the raw &'static str stop_reason parameter in the
cleanup function with a CollectorStopReason enum covering all existing fixed
reason values. Implement Display (and FromStr if required by the surrounding
API), update all affected call sites to use the enum variants, and log
stop_reason with % formatting in the tracing::info! call.
In `@crates/machine-a-tron/src/machine_state_machine.rs`:
- Around line 984-997: The unsupported-request branch in set_system_power should
log the SystemPowerControl value as a structured request field instead of
pre-formatting it into the detail field. Preserve the existing warning message
and BadRequest error text, but pass the matched request directly to tracing so
individual variants remain filterable.
In `@crates/mqttea/src/client/core.rs`:
- Around line 717-725: Update the next_delay method to emit an appropriate
carbide_instrument::Event for each backoff delay instead of using warn!. Follow
the existing event patterns in this file, include the delay duration in the
event fields, and preserve the current backoff calculation and returned delay.
🪄 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: 4152e4f2-4b02-4fe0-b2df-16d5c698c9dd
📒 Files selected for processing (271)
STYLE_GUIDE.mdcrates/admin-cli/src/machine/show/cmd.rscrates/admin-cli/src/rpc.rscrates/agent/src/agent_platform.rscrates/agent/src/astra_weave.rscrates/agent/src/containerd/container.rscrates/agent/src/containerd/image.rscrates/agent/src/dhcp_server_grpc_client.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/dpu_extension_service_observability.rscrates/agent/src/extension_services/k8s_pod_handler.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/instrumentation.rscrates/agent/src/lib.rscrates/agent/src/machine_inventory_updater.rscrates/agent/src/main_loop.rscrates/agent/src/managed_files.rscrates/agent/src/mtu.rscrates/agent/src/netlink.rscrates/agent/src/network_monitor.rscrates/agent/src/nvue.rscrates/agent/src/ovs.rscrates/agent/src/periodic_config_fetcher.rscrates/agent/src/tests/common/mod.rscrates/agent/src/tests/full.rscrates/agent/src/tests/test_network_monitor.rscrates/agent/src/traffic_intercept_bridging.rscrates/agent/src/upgrade.rscrates/api-core/src/attestation/measured_boot.rscrates/api-core/src/attestation/tpm_ca_cert.rscrates/api-core/src/db_init.rscrates/api-core/src/dhcp/discover.rscrates/api-core/src/dpa/handler.rscrates/api-core/src/dynamic_settings.rscrates/api-core/src/handlers/api.rscrates/api-core/src/handlers/astra.rscrates/api-core/src/handlers/attestation.rscrates/api-core/src/handlers/bmc_endpoint_explorer.rscrates/api-core/src/handlers/component_manager.rscrates/api-core/src/handlers/dns.rscrates/api-core/src/handlers/dpu_remediation.rscrates/api-core/src/handlers/expected_machine.rscrates/api-core/src/handlers/extension_service.rscrates/api-core/src/handlers/finder.rscrates/api-core/src/handlers/firmware.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_scout.rscrates/api-core/src/handlers/machine_validation.rscrates/api-core/src/handlers/rack.rscrates/api-core/src/handlers/redfish.rscrates/api-core/src/handlers/scout_stream.rscrates/api-core/src/handlers/site_explorer.rscrates/api-core/src/handlers/svpc.rscrates/api-core/src/handlers/uefi.rscrates/api-core/src/instance/mod.rscrates/api-core/src/ipxe.rscrates/api-core/src/listener.rscrates/api-core/src/logging/setup.rscrates/api-core/src/machine_update_manager/dpu_nic_firmware.rscrates/api-core/src/machine_update_manager/host_firmware.rscrates/api-core/src/machine_update_manager/mod.rscrates/api-core/src/machine_validation/mod.rscrates/api-core/src/measured_boot/metrics_collector/mod.rscrates/api-core/src/mqtt_state_change_hook/hook.rscrates/api-core/src/run.rscrates/api-core/src/scout_stream.rscrates/api-core/src/setup.rscrates/api-core/src/tests/common/api_fixtures/dpu.rscrates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-core/src/tests/common/api_fixtures/site_explorer.rscrates/api-core/src/tests/common/api_fixtures/test_machine/mod.rscrates/api-core/src/tests/dns.rscrates/api-core/src/tests/dpu_reprovisioning.rscrates/api-core/src/tests/instance.rscrates/api-core/src/tests/machine_network.rscrates/api-core/src/tests/sku.rscrates/api-core/tests/integration/machine_bmc_metadata.rscrates/api-db/src/bmc_metadata.rscrates/api-db/src/carbide_version.rscrates/api-db/src/dns/resource_record.rscrates/api-db/src/expected_power_shelf.rscrates/api-db/src/expected_switch.rscrates/api-db/src/instance.rscrates/api-db/src/lib.rscrates/api-db/src/machine.rscrates/api-db/src/machine_interface.rscrates/api-db/src/machine_topology.rscrates/api-db/src/measured_boot/bundle.rscrates/api-db/src/vpc_dpu_loopback.rscrates/api-db/src/work_lock_manager.rscrates/api-integration-tests/tests/lib.rscrates/api-model/src/dpa_interface/mod.rscrates/api-model/src/firmware.rscrates/api-model/src/instance/status/network.rscrates/api-model/src/instance/status/nvlink.rscrates/api-model/src/instance/status/spx.rscrates/api-model/src/machine/mod.rscrates/api-model/src/machine/nvlink.rscrates/api-model/src/machine/spx.rscrates/api-model/src/site_explorer/mod.rscrates/api-test-helper/src/domain.rscrates/api-test-helper/src/instance.rscrates/api-test-helper/src/machine.rscrates/api-test-helper/src/machine_a_tron.rscrates/api-test-helper/src/subnet.rscrates/api-test-helper/src/tenant.rscrates/api-test-helper/src/vpc.rscrates/api-test-helper/src/vpc_prefix.rscrates/api-web/src/auth.rscrates/api-web/src/interface.rscrates/api-web/src/lib.rscrates/api-web/src/machine.rscrates/api-web/src/managed_host.rscrates/api-web/src/search.rscrates/bmc-explorer/src/chassis.rscrates/bmc-explorer/src/computer_system.rscrates/bmc-explorer/src/manager.rscrates/bmc-mock/src/combined_server.rscrates/bmc-mock/src/combined_service.rscrates/bmc-mock/src/main.rscrates/bmc-mock/src/redfish/expander_router.rscrates/bmc-mock/src/tar_router.rscrates/bmc-mock/src/tls.rscrates/bmc-proxy/src/bmc_proxy.rscrates/bmc-proxy/src/setup.rscrates/dhcp-server/src/cache.rscrates/dhcp-server/src/grpc_server.rscrates/dhcp-server/src/main.rscrates/dhcp-server/src/modes/controller.rscrates/dhcp-server/src/packet_handler.rscrates/dhcp-server/src/util.rscrates/dns-record/src/lib.rscrates/dns/src/lib.rscrates/dns/src/main.rscrates/dpa-manager/src/card_handler/astra.rscrates/dpa-manager/src/card_handler/svpc.rscrates/dpa-manager/src/lib.rscrates/dpa/src/lib.rscrates/dpf/src/sdk.rscrates/dpu-otel-agent/src/lib.rscrates/dpu-remediation/src/remediation.rscrates/dsx-exchange-consumer/src/api_client.rscrates/firmware/src/config.rscrates/firmware/src/downloader.rscrates/fmds/src/http_request_metrics.rscrates/fmds/src/main.rscrates/fmds/src/nic_init.rscrates/fmds/src/phone_home.rscrates/health/src/api_client.rscrates/health/src/collectors/logs/periodic.rscrates/health/src/discovery/cleanup.rscrates/health/src/discovery/context.rscrates/health/src/discovery/spawn.rscrates/health/src/metrics.rscrates/host-support/src/hardware_enumeration.rscrates/host-support/src/hardware_enumeration/dpu.rscrates/host-support/src/hardware_enumeration/gpu.rscrates/host-support/src/hardware_enumeration/tpm.rscrates/host-support/src/registration.rscrates/http-connector/src/connector.rscrates/http-connector/src/resolver.rscrates/ib-fabric/src/lib.rscrates/ib-partition-controller/src/handler.rscrates/ipmi/src/tool.rscrates/kms-provider/src/providers/transit.rscrates/libmlx/src/device/discovery.rscrates/libnmxc/src/lib.rscrates/libnmxm/src/lib.rscrates/machine-a-tron/src/api_client.rscrates/machine-a-tron/src/api_throttler.rscrates/machine-a-tron/src/bmc_mock_wrapper.rscrates/machine-a-tron/src/dhcp_wrapper.rscrates/machine-a-tron/src/host_machine.rscrates/machine-a-tron/src/machine_a_tron.rscrates/machine-a-tron/src/machine_state_machine.rscrates/machine-a-tron/src/machine_utils.rscrates/machine-a-tron/src/main.rscrates/machine-a-tron/src/mock_ssh_server.rscrates/machine-a-tron/src/subnet.rscrates/machine-a-tron/src/tui.rscrates/machine-a-tron/src/vpc.rscrates/machine-controller/src/dpf.rscrates/machine-controller/src/handler.rscrates/machine-controller/src/handler/attestation.rscrates/machine-controller/src/handler/bios_config.rscrates/machine-controller/src/handler/dpf.rscrates/machine-controller/src/handler/machine_validation.rscrates/machine-controller/src/handler/power.rscrates/machine-controller/src/handler/sku.rscrates/machine-validation/src/machine_validation.rscrates/metrics-endpoint/src/lib.rscrates/metrics-utils/src/lib.rscrates/mqttea-example/src/main.rscrates/mqttea/src/client/core.rscrates/mqttea/src/client/messages.rscrates/mqttea/src/registry/core.rscrates/nvlink-manager/src/lib.rscrates/nvlink-manager/src/switch_cert_monitor.rscrates/power-shelf-controller/src/configuring.rscrates/power-shelf-controller/src/deleting.rscrates/power-shelf-controller/src/fetching_data.rscrates/power-shelf-controller/src/initializing.rscrates/preingestion-manager/src/bfb_rshim_copier.rscrates/preingestion-manager/src/lib.rscrates/preingestion-manager/tests/integration/host_bmc_firmware.rscrates/rack-controller/src/created.rscrates/rack-controller/src/discovering.rscrates/rack-controller/src/error_state.rscrates/rack-controller/src/handler.rscrates/rack-controller/src/lib.rscrates/rack-controller/src/maintenance.rscrates/rack-controller/src/ready.rscrates/rack-controller/src/validating.rscrates/rpc-utils/src/managed_host_display.rscrates/rpc/src/errors.rscrates/rpc/src/forge_tls_client.rscrates/rpc/src/model/dpu_remediation.rscrates/scout/src/attestation.rscrates/scout/src/deprovision/scrabbing.rscrates/scout/src/firmware_upgrade.rscrates/scout/src/main.rscrates/scout/src/mlx_device.rscrates/scout/src/register.rscrates/scout/src/stream.rscrates/scout/src/tpm.rscrates/secrets/src/forge_vault.rscrates/secrets/src/local_credentials/file.rscrates/site-explorer/src/bmc_endpoint_explorer.rscrates/site-explorer/src/explored_endpoint_index.rscrates/site-explorer/src/lib.rscrates/site-explorer/src/machine_creator.rscrates/site-explorer/src/redfish.rscrates/site-explorer/src/switch_creator.rscrates/site-explorer/src/test_support/mock_endpoint_explorer.rscrates/site-explorer/tests/integration/power_shelf.rscrates/site-explorer/tests/integration/site_explorer.rscrates/site-explorer/tests/integration/switch.rscrates/spdm-controller/src/handler.rscrates/ssh-console-mock-api-server/src/lib.rscrates/ssh-console/src/bmc/client.rscrates/ssh-console/src/bmc/connection.rscrates/ssh-console/src/bmc/connection_impl/ipmi.rscrates/ssh-console/src/bmc/connection_impl/ssh.rscrates/ssh-console/src/bmc/message_proxy.rscrates/ssh-console/src/config.rscrates/ssh-console/src/console_logger.rscrates/ssh-console/src/frontend.rscrates/ssh-console/src/metrics.rscrates/ssh-console/src/ssh_cert_parsing.rscrates/ssh-console/src/ssh_server.rscrates/ssh-console/tests/util/ipmi_sim.rscrates/ssh-console/tests/util/mod.rscrates/state-controller/src/controller/processor.rscrates/switch-controller/src/certificate.rscrates/switch-controller/src/configuring.rscrates/switch-controller/src/created.rscrates/switch-controller/src/deleting.rscrates/switch-controller/src/error_state.rscrates/switch-controller/src/initializing.rscrates/switch-controller/src/ready.rscrates/switch-controller/src/validating.rs
💤 Files with no reviewable changes (1)
- crates/health/src/discovery/context.rs
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
Contributor
Author
|
Final feedback pass is in b70b2ba:
@coderabbitai full_review, thanks! |
Contributor
|
Kicking off the full review now to verify the final delta. (=^･ω･^=) ✏️ Learnings added
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/api-core/src/dpa/handler.rs`:
- Around line 60-66: Update the MAC parsing error branch in the DPA handler to
bind the parse failure instead of discarding it as _e, then include that bound
error as a structured tracing field in the tracing::error! call alongside
mac_address.
- Around line 151-153: Update the partition validation around partition.len() to
distinguish zero matches from multiple matches: retain the “partition not found”
error only for an empty result, and log a separate data-integrity error
including the number of duplicate matches when more than one row is returned.
Preserve the existing return behavior for both invalid cases.
🪄 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: 5d6a0909-2cd7-41ae-af1e-b83fa055a512
📒 Files selected for processing (271)
STYLE_GUIDE.mdcrates/admin-cli/src/machine/show/cmd.rscrates/admin-cli/src/rpc.rscrates/agent/src/agent_platform.rscrates/agent/src/astra_weave.rscrates/agent/src/containerd/container.rscrates/agent/src/containerd/image.rscrates/agent/src/dhcp_server_grpc_client.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/dpu_extension_service_observability.rscrates/agent/src/extension_services/k8s_pod_handler.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/instrumentation.rscrates/agent/src/lib.rscrates/agent/src/machine_inventory_updater.rscrates/agent/src/main_loop.rscrates/agent/src/managed_files.rscrates/agent/src/mtu.rscrates/agent/src/netlink.rscrates/agent/src/network_monitor.rscrates/agent/src/nvue.rscrates/agent/src/ovs.rscrates/agent/src/periodic_config_fetcher.rscrates/agent/src/tests/common/mod.rscrates/agent/src/tests/full.rscrates/agent/src/tests/test_network_monitor.rscrates/agent/src/traffic_intercept_bridging.rscrates/agent/src/upgrade.rscrates/api-core/src/attestation/measured_boot.rscrates/api-core/src/attestation/tpm_ca_cert.rscrates/api-core/src/db_init.rscrates/api-core/src/dhcp/discover.rscrates/api-core/src/dpa/handler.rscrates/api-core/src/dynamic_settings.rscrates/api-core/src/handlers/api.rscrates/api-core/src/handlers/astra.rscrates/api-core/src/handlers/attestation.rscrates/api-core/src/handlers/bmc_endpoint_explorer.rscrates/api-core/src/handlers/component_manager.rscrates/api-core/src/handlers/dns.rscrates/api-core/src/handlers/dpu_remediation.rscrates/api-core/src/handlers/expected_machine.rscrates/api-core/src/handlers/extension_service.rscrates/api-core/src/handlers/finder.rscrates/api-core/src/handlers/firmware.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_scout.rscrates/api-core/src/handlers/machine_validation.rscrates/api-core/src/handlers/rack.rscrates/api-core/src/handlers/redfish.rscrates/api-core/src/handlers/scout_stream.rscrates/api-core/src/handlers/site_explorer.rscrates/api-core/src/handlers/svpc.rscrates/api-core/src/handlers/uefi.rscrates/api-core/src/instance/mod.rscrates/api-core/src/ipxe.rscrates/api-core/src/listener.rscrates/api-core/src/logging/setup.rscrates/api-core/src/machine_update_manager/dpu_nic_firmware.rscrates/api-core/src/machine_update_manager/host_firmware.rscrates/api-core/src/machine_update_manager/mod.rscrates/api-core/src/machine_validation/mod.rscrates/api-core/src/measured_boot/metrics_collector/mod.rscrates/api-core/src/mqtt_state_change_hook/hook.rscrates/api-core/src/run.rscrates/api-core/src/scout_stream.rscrates/api-core/src/setup.rscrates/api-core/src/tests/common/api_fixtures/dpu.rscrates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-core/src/tests/common/api_fixtures/site_explorer.rscrates/api-core/src/tests/common/api_fixtures/test_machine/mod.rscrates/api-core/src/tests/dns.rscrates/api-core/src/tests/dpu_reprovisioning.rscrates/api-core/src/tests/instance.rscrates/api-core/src/tests/machine_network.rscrates/api-core/src/tests/sku.rscrates/api-core/tests/integration/machine_bmc_metadata.rscrates/api-db/src/bmc_metadata.rscrates/api-db/src/carbide_version.rscrates/api-db/src/dns/resource_record.rscrates/api-db/src/expected_power_shelf.rscrates/api-db/src/expected_switch.rscrates/api-db/src/instance.rscrates/api-db/src/lib.rscrates/api-db/src/machine.rscrates/api-db/src/machine_interface.rscrates/api-db/src/machine_topology.rscrates/api-db/src/measured_boot/bundle.rscrates/api-db/src/vpc_dpu_loopback.rscrates/api-db/src/work_lock_manager.rscrates/api-integration-tests/tests/lib.rscrates/api-model/src/dpa_interface/mod.rscrates/api-model/src/firmware.rscrates/api-model/src/instance/status/network.rscrates/api-model/src/instance/status/nvlink.rscrates/api-model/src/instance/status/spx.rscrates/api-model/src/machine/mod.rscrates/api-model/src/machine/nvlink.rscrates/api-model/src/machine/spx.rscrates/api-model/src/site_explorer/mod.rscrates/api-test-helper/src/domain.rscrates/api-test-helper/src/instance.rscrates/api-test-helper/src/machine.rscrates/api-test-helper/src/machine_a_tron.rscrates/api-test-helper/src/subnet.rscrates/api-test-helper/src/tenant.rscrates/api-test-helper/src/vpc.rscrates/api-test-helper/src/vpc_prefix.rscrates/api-web/src/auth.rscrates/api-web/src/interface.rscrates/api-web/src/lib.rscrates/api-web/src/machine.rscrates/api-web/src/managed_host.rscrates/api-web/src/search.rscrates/bmc-explorer/src/chassis.rscrates/bmc-explorer/src/computer_system.rscrates/bmc-explorer/src/manager.rscrates/bmc-mock/src/combined_server.rscrates/bmc-mock/src/combined_service.rscrates/bmc-mock/src/main.rscrates/bmc-mock/src/redfish/expander_router.rscrates/bmc-mock/src/tar_router.rscrates/bmc-mock/src/tls.rscrates/bmc-proxy/src/bmc_proxy.rscrates/bmc-proxy/src/setup.rscrates/dhcp-server/src/cache.rscrates/dhcp-server/src/grpc_server.rscrates/dhcp-server/src/main.rscrates/dhcp-server/src/modes/controller.rscrates/dhcp-server/src/packet_handler.rscrates/dhcp-server/src/util.rscrates/dns-record/src/lib.rscrates/dns/src/lib.rscrates/dns/src/main.rscrates/dpa-manager/src/card_handler/astra.rscrates/dpa-manager/src/card_handler/svpc.rscrates/dpa-manager/src/lib.rscrates/dpa/src/lib.rscrates/dpf/src/sdk.rscrates/dpu-otel-agent/src/lib.rscrates/dpu-remediation/src/remediation.rscrates/dsx-exchange-consumer/src/api_client.rscrates/firmware/src/config.rscrates/firmware/src/downloader.rscrates/fmds/src/http_request_metrics.rscrates/fmds/src/main.rscrates/fmds/src/nic_init.rscrates/fmds/src/phone_home.rscrates/health/src/api_client.rscrates/health/src/collectors/logs/periodic.rscrates/health/src/discovery/cleanup.rscrates/health/src/discovery/context.rscrates/health/src/discovery/spawn.rscrates/health/src/metrics.rscrates/host-support/src/hardware_enumeration.rscrates/host-support/src/hardware_enumeration/dpu.rscrates/host-support/src/hardware_enumeration/gpu.rscrates/host-support/src/hardware_enumeration/tpm.rscrates/host-support/src/registration.rscrates/http-connector/src/connector.rscrates/http-connector/src/resolver.rscrates/ib-fabric/src/lib.rscrates/ib-partition-controller/src/handler.rscrates/ipmi/src/tool.rscrates/kms-provider/src/providers/transit.rscrates/libmlx/src/device/discovery.rscrates/libnmxc/src/lib.rscrates/libnmxm/src/lib.rscrates/machine-a-tron/src/api_client.rscrates/machine-a-tron/src/api_throttler.rscrates/machine-a-tron/src/bmc_mock_wrapper.rscrates/machine-a-tron/src/dhcp_wrapper.rscrates/machine-a-tron/src/host_machine.rscrates/machine-a-tron/src/machine_a_tron.rscrates/machine-a-tron/src/machine_state_machine.rscrates/machine-a-tron/src/machine_utils.rscrates/machine-a-tron/src/main.rscrates/machine-a-tron/src/mock_ssh_server.rscrates/machine-a-tron/src/subnet.rscrates/machine-a-tron/src/tui.rscrates/machine-a-tron/src/vpc.rscrates/machine-controller/src/dpf.rscrates/machine-controller/src/handler.rscrates/machine-controller/src/handler/attestation.rscrates/machine-controller/src/handler/bios_config.rscrates/machine-controller/src/handler/dpf.rscrates/machine-controller/src/handler/machine_validation.rscrates/machine-controller/src/handler/power.rscrates/machine-controller/src/handler/sku.rscrates/machine-validation/src/machine_validation.rscrates/metrics-endpoint/src/lib.rscrates/metrics-utils/src/lib.rscrates/mqttea-example/src/main.rscrates/mqttea/src/client/core.rscrates/mqttea/src/client/messages.rscrates/mqttea/src/registry/core.rscrates/nvlink-manager/src/lib.rscrates/nvlink-manager/src/switch_cert_monitor.rscrates/power-shelf-controller/src/configuring.rscrates/power-shelf-controller/src/deleting.rscrates/power-shelf-controller/src/fetching_data.rscrates/power-shelf-controller/src/initializing.rscrates/preingestion-manager/src/bfb_rshim_copier.rscrates/preingestion-manager/src/lib.rscrates/preingestion-manager/tests/integration/host_bmc_firmware.rscrates/rack-controller/src/created.rscrates/rack-controller/src/discovering.rscrates/rack-controller/src/error_state.rscrates/rack-controller/src/handler.rscrates/rack-controller/src/lib.rscrates/rack-controller/src/maintenance.rscrates/rack-controller/src/ready.rscrates/rack-controller/src/validating.rscrates/rpc-utils/src/managed_host_display.rscrates/rpc/src/errors.rscrates/rpc/src/forge_tls_client.rscrates/rpc/src/model/dpu_remediation.rscrates/scout/src/attestation.rscrates/scout/src/deprovision/scrabbing.rscrates/scout/src/firmware_upgrade.rscrates/scout/src/main.rscrates/scout/src/mlx_device.rscrates/scout/src/register.rscrates/scout/src/stream.rscrates/scout/src/tpm.rscrates/secrets/src/forge_vault.rscrates/secrets/src/local_credentials/file.rscrates/site-explorer/src/bmc_endpoint_explorer.rscrates/site-explorer/src/explored_endpoint_index.rscrates/site-explorer/src/lib.rscrates/site-explorer/src/machine_creator.rscrates/site-explorer/src/redfish.rscrates/site-explorer/src/switch_creator.rscrates/site-explorer/src/test_support/mock_endpoint_explorer.rscrates/site-explorer/tests/integration/power_shelf.rscrates/site-explorer/tests/integration/site_explorer.rscrates/site-explorer/tests/integration/switch.rscrates/spdm-controller/src/handler.rscrates/ssh-console-mock-api-server/src/lib.rscrates/ssh-console/src/bmc/client.rscrates/ssh-console/src/bmc/connection.rscrates/ssh-console/src/bmc/connection_impl/ipmi.rscrates/ssh-console/src/bmc/connection_impl/ssh.rscrates/ssh-console/src/bmc/message_proxy.rscrates/ssh-console/src/config.rscrates/ssh-console/src/console_logger.rscrates/ssh-console/src/frontend.rscrates/ssh-console/src/metrics.rscrates/ssh-console/src/ssh_cert_parsing.rscrates/ssh-console/src/ssh_server.rscrates/ssh-console/tests/util/ipmi_sim.rscrates/ssh-console/tests/util/mod.rscrates/state-controller/src/controller/processor.rscrates/switch-controller/src/certificate.rscrates/switch-controller/src/configuring.rscrates/switch-controller/src/created.rscrates/switch-controller/src/deleting.rscrates/switch-controller/src/error_state.rscrates/switch-controller/src/initializing.rscrates/switch-controller/src/ready.rscrates/switch-controller/src/validating.rs
💤 Files with no reviewable changes (1)
- crates/health/src/discovery/context.rs
🚧 Files skipped from review as they are similar to previous changes (241)
- crates/site-explorer/src/test_support/mock_endpoint_explorer.rs
- crates/agent/src/netlink.rs
- crates/power-shelf-controller/src/initializing.rs
- crates/switch-controller/src/validating.rs
- crates/dhcp-server/src/modes/controller.rs
- crates/api-test-helper/src/vpc_prefix.rs
- crates/api-test-helper/src/tenant.rs
- crates/dsx-exchange-consumer/src/api_client.rs
- crates/site-explorer/tests/integration/switch.rs
- crates/host-support/src/hardware_enumeration/gpu.rs
- crates/api-core/src/handlers/scout_stream.rs
- crates/api-web/src/interface.rs
- crates/api-core/src/tests/common/api_fixtures/dpu.rs
- crates/agent/src/host_machine_id.rs
- crates/api-model/src/instance/status/spx.rs
- crates/api-core/src/tests/machine_network.rs
- crates/api-core/src/machine_update_manager/mod.rs
- crates/api-web/src/search.rs
- crates/switch-controller/src/deleting.rs
- crates/api-model/src/firmware.rs
- crates/api-test-helper/src/vpc.rs
- crates/admin-cli/src/machine/show/cmd.rs
- crates/libnmxc/src/lib.rs
- crates/machine-a-tron/src/api_throttler.rs
- crates/rack-controller/src/discovering.rs
- crates/machine-controller/src/handler/dpf.rs
- crates/dpu-otel-agent/src/lib.rs
- crates/api-db/src/expected_power_shelf.rs
- crates/api-web/src/auth.rs
- crates/api-web/src/lib.rs
- crates/bmc-mock/src/tls.rs
- crates/agent/src/agent_platform.rs
- crates/api-db/src/dns/resource_record.rs
- crates/mqttea/src/client/messages.rs
- crates/http-connector/src/resolver.rs
- crates/fmds/src/phone_home.rs
- crates/api-core/src/handlers/firmware.rs
- crates/api-core/src/run.rs
- crates/site-explorer/src/explored_endpoint_index.rs
- crates/machine-a-tron/src/vpc.rs
- crates/dpa/src/lib.rs
- crates/preingestion-manager/tests/integration/host_bmc_firmware.rs
- crates/agent/src/mtu.rs
- crates/rack-controller/src/error_state.rs
- crates/api-core/src/handlers/dpu_remediation.rs
- crates/api-core/tests/integration/machine_bmc_metadata.rs
- crates/agent/src/ovs.rs
- crates/state-controller/src/controller/processor.rs
- crates/machine-a-tron/src/tui.rs
- crates/bmc-mock/src/redfish/expander_router.rs
- crates/rpc/src/errors.rs
- crates/api-db/src/expected_switch.rs
- crates/api-model/src/machine/spx.rs
- crates/api-model/src/machine/nvlink.rs
- crates/ib-partition-controller/src/handler.rs
- crates/machine-controller/src/handler/bios_config.rs
- crates/api-core/src/dynamic_settings.rs
- crates/api-core/src/handlers/dns.rs
- crates/api-db/src/instance.rs
- crates/ssh-console-mock-api-server/src/lib.rs
- crates/health/src/metrics.rs
- crates/api-core/src/listener.rs
- crates/api-test-helper/src/machine.rs
- crates/machine-controller/src/handler/power.rs
- crates/power-shelf-controller/src/deleting.rs
- crates/switch-controller/src/created.rs
- crates/agent/src/dhcp_server_grpc_client.rs
- crates/rack-controller/src/created.rs
- crates/agent/src/health/bgp.rs
- crates/api-core/src/handlers/uefi.rs
- crates/ssh-console/src/bmc/message_proxy.rs
- crates/agent/src/extension_services/dpu_extension_service_observability.rs
- crates/ssh-console/src/ssh_cert_parsing.rs
- crates/api-core/src/mqtt_state_change_hook/hook.rs
- crates/machine-a-tron/src/api_client.rs
- crates/scout/src/register.rs
- crates/machine-a-tron/src/subnet.rs
- crates/fmds/src/nic_init.rs
- crates/agent/src/tests/common/mod.rs
- crates/secrets/src/local_credentials/file.rs
- crates/api-db/src/carbide_version.rs
- crates/host-support/src/hardware_enumeration/tpm.rs
- crates/health/src/collectors/logs/periodic.rs
- crates/ssh-console/tests/util/ipmi_sim.rs
- crates/rack-controller/src/lib.rs
- crates/kms-provider/src/providers/transit.rs
- crates/mqttea/src/registry/core.rs
- crates/api-core/src/attestation/measured_boot.rs
- crates/dhcp-server/src/util.rs
- crates/ssh-console/src/bmc/connection_impl/ipmi.rs
- crates/libnmxm/src/lib.rs
- crates/api-core/src/logging/setup.rs
- crates/api-model/src/site_explorer/mod.rs
- crates/libmlx/src/device/discovery.rs
- crates/agent/src/dpu/link.rs
- crates/dhcp-server/src/cache.rs
- crates/agent/src/lib.rs
- crates/api-core/src/handlers/machine_validation.rs
- crates/rack-controller/src/handler.rs
- crates/bmc-mock/src/combined_server.rs
- crates/firmware/src/downloader.rs
- crates/api-core/src/tests/common/api_fixtures/test_machine/mod.rs
- crates/api-model/src/instance/status/nvlink.rs
- crates/api-core/src/dhcp/discover.rs
- crates/api-core/src/handlers/attestation.rs
- crates/api-core/src/handlers/redfish.rs
- crates/dhcp-server/src/grpc_server.rs
- crates/ssh-console/src/metrics.rs
- crates/bmc-explorer/src/chassis.rs
- crates/metrics-utils/src/lib.rs
- crates/api-db/src/machine_topology.rs
- crates/agent/src/managed_files.rs
- crates/spdm-controller/src/handler.rs
- STYLE_GUIDE.md
- crates/api-core/src/handlers/rack.rs
- crates/api-core/src/handlers/expected_machine.rs
- crates/agent/src/nvue.rs
- crates/machine-a-tron/src/machine_utils.rs
- crates/api-web/src/machine.rs
- crates/api-core/src/measured_boot/metrics_collector/mod.rs
- crates/api-core/src/handlers/finder.rs
- crates/firmware/src/config.rs
- crates/api-model/src/machine/mod.rs
- crates/switch-controller/src/configuring.rs
- crates/api-core/src/machine_update_manager/dpu_nic_firmware.rs
- crates/host-support/src/registration.rs
- crates/rack-controller/src/maintenance.rs
- crates/bmc-proxy/src/setup.rs
- crates/api-core/src/tests/common/api_fixtures/mod.rs
- crates/fmds/src/http_request_metrics.rs
- crates/api-core/src/handlers/bmc_endpoint_explorer.rs
- crates/dns/src/lib.rs
- crates/api-db/src/lib.rs
- crates/site-explorer/src/switch_creator.rs
- crates/health/src/discovery/spawn.rs
- crates/api-core/src/tests/common/api_fixtures/site_explorer.rs
- crates/api-db/src/vpc_dpu_loopback.rs
- crates/scout/src/tpm.rs
- crates/scout/src/attestation.rs
- crates/ssh-console/src/config.rs
- crates/dhcp-server/src/packet_handler.rs
- crates/api-core/src/db_init.rs
- crates/api-core/src/scout_stream.rs
- crates/ssh-console/src/bmc/connection_impl/ssh.rs
- crates/agent/src/containerd/image.rs
- crates/api-core/src/ipxe.rs
- crates/api-test-helper/src/subnet.rs
- crates/api-core/src/tests/dns.rs
- crates/site-explorer/src/machine_creator.rs
- crates/bmc-mock/src/tar_router.rs
- crates/machine-controller/src/handler/attestation.rs
- crates/machine-a-tron/src/machine_state_machine.rs
- crates/api-core/src/handlers/machine_scout.rs
- crates/api-core/src/handlers/extension_service.rs
- crates/site-explorer/tests/integration/site_explorer.rs
- crates/dns/src/main.rs
- crates/switch-controller/src/certificate.rs
- crates/switch-controller/src/initializing.rs
- crates/api-db/src/work_lock_manager.rs
- crates/bmc-mock/src/combined_service.rs
- crates/bmc-explorer/src/manager.rs
- crates/machine-controller/src/handler/sku.rs
- crates/agent/src/containerd/container.rs
- crates/ssh-console/tests/util/mod.rs
- crates/api-core/src/handlers/astra.rs
- crates/agent/src/dpu/interface.rs
- crates/host-support/src/hardware_enumeration/dpu.rs
- crates/bmc-explorer/src/computer_system.rs
- crates/dpu-remediation/src/remediation.rs
- crates/api-core/src/machine_update_manager/host_firmware.rs
- crates/bmc-mock/src/main.rs
- crates/api-core/src/handlers/instance.rs
- crates/machine-a-tron/src/mock_ssh_server.rs
- crates/api-core/src/attestation/tpm_ca_cert.rs
- crates/mqttea-example/src/main.rs
- crates/agent/src/traffic_intercept_bridging.rs
- crates/agent/src/upgrade.rs
- crates/agent/src/tests/full.rs
- crates/machine-controller/src/dpf.rs
- crates/api-model/src/instance/status/network.rs
- crates/api-core/src/handlers/instance_type.rs
- crates/api-integration-tests/tests/lib.rs
- crates/rack-controller/src/validating.rs
- crates/api-core/src/tests/dpu_reprovisioning.rs
- crates/agent/src/network_monitor.rs
- crates/rpc/src/model/dpu_remediation.rs
- crates/ssh-console/src/console_logger.rs
- crates/agent/src/periodic_config_fetcher.rs
- crates/machine-controller/src/handler/machine_validation.rs
- crates/api-core/src/handlers/component_manager.rs
- crates/dhcp-server/src/main.rs
- crates/ssh-console/src/bmc/client.rs
- crates/machine-a-tron/src/machine_a_tron.rs
- crates/api-web/src/managed_host.rs
- crates/api-db/src/bmc_metadata.rs
- crates/api-db/src/machine.rs
- crates/rpc/src/forge_tls_client.rs
- crates/machine-a-tron/src/host_machine.rs
- crates/agent/src/instrumentation.rs
- crates/scout/src/stream.rs
- crates/dns-record/src/lib.rs
- crates/preingestion-manager/src/bfb_rshim_copier.rs
- crates/api-core/src/tests/sku.rs
- crates/rpc-utils/src/managed_host_display.rs
- crates/site-explorer/src/redfish.rs
- crates/machine-a-tron/src/main.rs
- crates/api-db/src/machine_interface.rs
- crates/machine-a-tron/src/dhcp_wrapper.rs
- crates/api-core/src/instance/mod.rs
- crates/scout/src/firmware_upgrade.rs
- crates/bmc-proxy/src/bmc_proxy.rs
- crates/dpa-manager/src/card_handler/astra.rs
- crates/api-test-helper/src/instance.rs
- crates/agent/src/hbn.rs
- crates/api-core/src/handlers/svpc.rs
- crates/agent/src/dpu/route.rs
- crates/machine-validation/src/machine_validation.rs
- crates/host-support/src/hardware_enumeration.rs
- crates/http-connector/src/connector.rs
- crates/agent/src/main_loop.rs
- crates/dpf/src/sdk.rs
- crates/api-core/src/handlers/api.rs
- crates/api-core/src/handlers/machine.rs
- crates/api-model/src/dpa_interface/mod.rs
- crates/ib-fabric/src/lib.rs
- crates/nvlink-manager/src/switch_cert_monitor.rs
- crates/rack-controller/src/ready.rs
- crates/scout/src/deprovision/scrabbing.rs
- crates/mqttea/src/client/core.rs
- crates/agent/src/astra_weave.rs
- crates/agent/src/extension_services/k8s_pod_handler.rs
- crates/secrets/src/forge_vault.rs
- crates/agent/src/health.rs
- crates/scout/src/main.rs
- crates/api-core/src/setup.rs
- crates/nvlink-manager/src/lib.rs
- crates/scout/src/mlx_device.rs
- crates/machine-controller/src/handler.rs
- crates/site-explorer/src/lib.rs
- crates/site-explorer/src/bmc_endpoint_explorer.rs
- crates/preingestion-manager/src/lib.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/host-support/src/registration.rs (1)
122-148: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winLog message text doesn't match the enclosing function.
Line 134's message reads
"register_machine request", but this call is insidediscover_machine_once, notregister_machine. Given the PR's goal of stable, searchable messages, this mismatch will mislead log-based filtering/debugging for this function.🩹 Proposed fix
let request = tonic::Request::new(info); - tracing::debug!(?request, "register_machine request"); + tracing::debug!(?request, "discover_machine request");🤖 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/host-support/src/registration.rs` around lines 122 - 148, Update the tracing::debug message in discover_machine_once to use discover_machine terminology instead of register_machine, while preserving the existing request field and logging behavior.crates/nvlink-manager/src/lib.rs (1)
514-571: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winInclude
partition_nmx_c_idin the "NMX-C partition not found" logs.All four
Nonebranches here (two inget_gpus_to_keep_after_removal, two inget_gpus_to_keep_in_unknown_partition_after_removal) logmachine_idanddevice_instancebut droppartition_nmx_c_id, even though it's in scope and is the actual key that failed the lookup. The sibling failure path incheck_nv_link_partitions(theRemoveFromUnknownPartitionelse-branch) already includespartition_idfor the exact same failure message — these four sites should match for consistent, useful diagnostics.As per path instructions, "Review Rust code against STYLE_GUIDE.md: ... structured tracing fields."🔧 Proposed fix (one of the four sites shown; apply to all four)
None => { tracing::error!( machine_id = %machine_id, device_instance, + partition_id = partition_nmx_c_id.partition_id, "NMX-C partition not found for machine GPU", ); return None; }Also applies to: 579-633
🤖 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/nvlink-manager/src/lib.rs` around lines 514 - 571, Update all four “NMX-C partition not found” tracing error calls in get_gpus_to_keep_after_removal and get_gpus_to_keep_in_unknown_partition_after_removal to include partition_nmx_c_id as a structured tracing field, while preserving the existing machine_id, device_instance, and error message.Source: Path instructions
crates/preingestion-manager/src/lib.rs (1)
3174-3256: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCarry
addressthrough the remaining early-exit logs ininitiate_update.Two sibling gaps in this exact function (multipart-not-supported / failed-to-open-firmware-file) were already fixed to include
address = %endpoint_clone.address. Four more sites in the same function still omit it — resolving artifact / RSHIM failures across concurrently-processed endpoints will be hard to attribute without it:
- artifact-resolution failure (
"Failed to resolve firmware artifact")- download-deferred debug log (
"Firmware artifact is being downloaded; update deferred")- missing-local-artifact error (
"Firmware artifact is not present")- RSHIM-enable failure (
"Failed to enable RSHIM on BMC")🔧 Proposed fix
Ok(artifact) => artifact, Err(error) => { - tracing::error!(%error, "Failed to resolve firmware artifact"); + tracing::error!( + address = %endpoint_clone.address, + %error, + "Failed to resolve firmware artifact" + ); return Ok(false); } @@ if !self.downloader.available(&artifact.local_path, url, sha256) { tracing::debug!( + address = %endpoint_clone.address, path = %artifact.local_path.display(), %url, "Firmware artifact is being downloaded; update deferred" ); @@ if !artifact.local_path.exists() { tracing::error!( + address = %endpoint_clone.address, path = %artifact.local_path.display(), "Firmware artifact is not present" ); @@ redfish_client .enable_rshim_bmc() .await - .inspect_err(|e| tracing::error!(error = %e, "Failed to enable RSHIM on BMC")) + .inspect_err(|e| tracing::error!(address = %endpoint_clone.address, error = %e, "Failed to enable RSHIM on BMC")) .ok();🤖 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/preingestion-manager/src/lib.rs` around lines 3174 - 3256, Update the four logging sites in initiate_update to include address = %endpoint_clone.address: the “Failed to resolve firmware artifact” error, “Firmware artifact is being downloaded; update deferred” debug message, “Firmware artifact is not present” error, and “Failed to enable RSHIM on BMC” error. Preserve their existing messages and behavior.
♻️ Duplicate comments (2)
crates/api-core/src/dpa/handler.rs (2)
151-153: 🗄️ Data Integrity & Integration | 🟡 MinorDistinguish missing and duplicate SPX partitions.
partition.len() != 1covers both zero and multiple matches, but the message says “not found”. Split the cases or include the count so duplicate database rows are not misclassified.🤖 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/api-core/src/dpa/handler.rs` around lines 151 - 153, Update the partition validation in the handler around the partition lookup to distinguish zero matches from multiple matches: retain the “not found” error only when partition.len() is zero, and add a separate error for duplicate matches that includes the count, returning in both invalid cases.
61-66: 🎯 Functional Correctness | 🟡 MinorPreserve the MAC parse error.
The changed event still logs only the token.
Err(_e)discards the actual parsing failure, so malformed MQTT topics lose the reason. Bind the error and emit it aserror = %error.🤖 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/api-core/src/dpa/handler.rs` around lines 61 - 66, Update the DPA MAC parsing error branch around the MQTT topic handler to bind the parse error instead of discarding it. Include the bound error in the tracing::error! call using the structured field error = %error, while retaining the existing mac_address field and return behavior.
🧹 Nitpick comments (6)
crates/agent/src/dpu/interface.rs (1)
70-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
%interfacefor the interface-name field.
interfaceis a string value, so?interfaceemits Debug formatting instead of the semantic string representation. Keep?networks, but change this field to%interface.As per coding guidelines, use
%fieldfor Display values and?fieldfor Debug values.Proposed fix
- tracing::info!(?networks, ?interface, "Adding addresses to Interface"); + tracing::info!(?networks, %interface, "Adding addresses to Interface");🤖 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/agent/src/dpu/interface.rs` at line 70, Update the tracing call in the interface address-addition flow to use Display formatting for the string interface name: change the interface field in the log statement to %interface while retaining ?networks.Source: Coding guidelines
crates/agent/src/extension_services/dpu_extension_service_observability.rs (1)
163-173: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMerge stdout/stderr into a single
error!event instead of two. Both sites split one command-failure occurrence into two separatetracing::error!calls (one forstdout, one forstderr), doubling error-level log/alert volume per failure and splitting correlated data across two log lines.
crates/agent/src/extension_services/dpu_extension_service_observability.rs#L163-L173: combine the twotracing::error!calls into one, emitting bothstdoutandstderrfields together.crates/agent/src/traffic_intercept_bridging.rs#L158-L168: same fix — combine into a singletracing::error!call with bothstdoutandstderrfields.♻️ Proposed fix (apply to both sites)
- tracing::error!( - command = cmd_str.as_str(), - stdout = %String::from_utf8_lossy(&out.stdout), - "OTel validation command stdout" - ); - tracing::error!( - command = cmd_str.as_str(), - stderr = %String::from_utf8_lossy(&out.stderr), - "OTel validation command stderr" - ); + tracing::error!( + command = cmd_str.as_str(), + stdout = %String::from_utf8_lossy(&out.stdout), + stderr = %String::from_utf8_lossy(&out.stderr), + "OTel validation command failed" + );🤖 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/agent/src/extension_services/dpu_extension_service_observability.rs` around lines 163 - 173, Merge the two command-failure tracing::error! calls into one event that includes both stdout and stderr fields. Apply this in crates/agent/src/extension_services/dpu_extension_service_observability.rs at lines 163-173 and crates/agent/src/traffic_intercept_bridging.rs at lines 158-168, preserving the command field and existing failure message.crates/rack-controller/src/created.rs (1)
96-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse tracing field shorthand for same-named counts.
Replace the redundant assignments with
compute_count,switch_count, andpower_shelf_count. This keeps the structured event concise and follows the repository’s tracing conventions.As per coding guidelines: “Use native tracing field shorthand for tracing::Value types.”
🤖 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/rack-controller/src/created.rs` around lines 96 - 100, In the tracing event in created.rs, update the fields for compute_count, switch_count, and power_shelf_count to use native same-name shorthand instead of explicit self-assignments. Leave the rack_id field and event message unchanged.Source: Coding guidelines
crates/api-core/src/instance/mod.rs (1)
1427-1439: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd table-driven coverage for duplicate SPX attachments.
The new guard rejects duplicate
(device, device_instance)pairs. Add regression cases for both rejection and successful allocation of distinct attachments.As per coding guidelines, Rust functions mapping inputs to outputs or errors should use table-driven tests.
🤖 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/api-core/src/instance/mod.rs` around lines 1427 - 1439, Add table-driven tests for the duplicate-attachment validation in the instance allocation flow containing cases for duplicate (device, device_instance) pairs returning the expected InvalidArgument error and distinct attachments allocating successfully. Reuse the existing instance allocation test helpers and assert each case’s result, including the duplicate rejection and successful distinct-attachment behavior.Source: Coding guidelines
crates/ib-fabric/src/lib.rs (1)
1082-1096: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop the manual
write!loop; log the pkey lists as structured?fields.These three branches still hand-build a
msg: Stringvia awrite!loop and attach it as a singledetailsfield, whiledown_ports = ?result.down_port_guidsa few lines below achieves the same goal with a plain debug field on the underlyingVec. Passing?result.missing_guid_pkeys(etc.) directly removes the loop, thewrite!().unwrap()calls, and produces a properly structured (queryable) field instead of a flattened string — consistent with this PR's own stated goal.As per path instructions, "Review Rust code against STYLE_GUIDE.md: ... structured tracing fields ... Prefer findings about behavior, concurrency, resource lifetimes, and missing tests over style-only comments." This one also removes an `unwrap()` call, so it's not purely stylistic.♻️ Proposed simplification (missing_guid_pkeys shown; same pattern applies to the other two)
if !result.missing_guid_pkeys.is_empty() { metrics.num_machines_with_missing_pkeys += 1; - let mut msg = "Machine is missing pkeys on UFM: ".to_string(); - for (idx, (_fabric, guid, pkey)) in result.missing_guid_pkeys.iter().enumerate() { - if idx != 0 { - msg.push(','); - } - write!(&mut msg, "(guid: {guid}, pkey: {pkey})").unwrap(); - } tracing::warn!( machine_id = %machine_id, - details = %msg, + missing_guid_pkeys = ?result.missing_guid_pkeys, "Machine is missing pkeys on UFM", ); }Also applies to: 1097-1111, 1112-1127
🤖 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/ib-fabric/src/lib.rs` around lines 1082 - 1096, Replace the manual message-building loops in the three missing-pkey logging branches with structured debug fields on the underlying result vectors. Update the tracing calls around missing_guid_pkeys and the corresponding branches at 1097-1111 and 1112-1127 to pass each list directly as a ? field, removing the String construction, write! calls, and details field while preserving the existing warning context and metrics updates.Source: Path instructions
crates/machine-controller/src/handler.rs (1)
7867-7871: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPer-item INFO logging in a hot reconciliation path is likely too chatty for production.
"Found device for dpu","Found device_instance for dpu", and"checking dpu"fire once per DPU, per instance, on every invocation ofcheck_instance_network_synced_and_dpu_healthy— which itself runs on everyWaitingForNetworkConfig/WaitingForConfigSyncedreconciliation tick. At INFO level, fleets with many multi-DPU tenant instances will generate a steady stream of low-value log lines that mostly document internal bookkeeping rather than an operator-relevant event.Recommend demoting these three to
debug(ortrace), consistent with how other purely-diagnostic per-iteration lines in this file (e.g., the BMC log scan at line 2226, or the "Found device for dpu"-style checks elsewhere) are kept atdebug.♻️ Suggested level change
- tracing::info!( + tracing::debug!( %device, machine_id = %dpu_machine_id, "Found device for dpu" ); ... - tracing::info!( + tracing::debug!( device_instance, machine_id = %dpu_machine_id, "Found device_instance for dpu" ); ... - tracing::info!(machine_id = %dpu_id, "checking dpu"); + tracing::debug!(machine_id = %dpu_id, "checking dpu");Also applies to: 7877-7881, 7907-7907
🤖 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/machine-controller/src/handler.rs` around lines 7867 - 7871, Demote the per-item diagnostic logs in check_instance_network_synced_and_dpu_healthy from INFO to DEBUG, specifically “Found device for dpu,” “Found device_instance for dpu,” and “checking dpu.” Keep their messages and fields unchanged while ensuring all three no longer emit at production INFO level.
🤖 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/agent/src/astra_weave.rs`:
- Around line 700-703: Update the Deleted mismatched virtual network attachment
log in the surrounding control flow to log match_attachment safely without
calling as_ref().unwrap(); reuse the existing Option-aware tracing field style
used by the sibling error branch, while preserving the current deletion
behavior.
In `@crates/agent/src/health/bgp.rs`:
- Line 76: Update the tracing warning event around the BGP daemon configuration
check to use a stable, human-readable message instead of the identifier-style
string. Preserve the existing error = %err field and provide wording that
clearly indicates reading the BGP daemon configuration failed.
In `@crates/agent/src/machine_inventory_updater.rs`:
- Line 137: Update the tracing in the machine inventory update flow to stop
serializing the complete inventory_report. Log only bounded metadata,
specifically the machine identifier and software/component count, while
preserving the existing trace event context.
In `@crates/agent/src/mtu.rs`:
- Around line 59-61: Preserve child-process output as rendered diagnostic
message content instead of logging it only as structured fields. In
crates/agent/src/mtu.rs lines 59-61 and 79-81, include the MTU query and update
stderr in the rendered diagnostics; in crates/agent/src/ovs.rs lines 47-55,
include both OVS stdout and stderr in rendered diagnostics while retaining
command as structured metadata.
In `@crates/api-core/src/dpa/handler.rs`:
- Around line 196-201: Update the error event in the machine lookup failure
branch to include the relevant DPA-interface context, using the existing dpa_if
or machine identifier as the adjacent not-found branch does, while preserving
the current error field and return behavior.
In `@crates/api-core/src/handlers/instance_type.rs`:
- Around line 597-603: Remove the dangling “ids; versions” fragment from the
tracing message in the length-mismatch error block. Keep the stable
human-readable message and the existing structured fields `ids` and
`machine_versions` unchanged.
In `@crates/api-db/src/work_lock_manager.rs`:
- Around line 303-308: Update the tracing::error! call handling the work-lock
release try_send failure to use a neutral human-readable message that does not
imply queue saturation or database overload. Preserve the structured work_key,
worker_id, and error fields while changing only the descriptive message.
In `@crates/dpu-remediation/src/remediation.rs`:
- Around line 239-242: Update the error logging in the remediation response
handling around next_remediation to remove the full response debug field. Log
only bounded presence flags and safe identifiers, such as whether the ID and
script are present, while preserving the existing skip-and-retry behavior.
In `@crates/ssh-console/src/ssh_cert_parsing.rs`:
- Around line 83-85: Remove the raw ?key_id field from both warning logs in
crates/ssh-console/src/ssh_cert_parsing.rs at lines 83-85 and 111-113. Retain
the role_field and user_field fields respectively, or replace key_id with a
non-reversible fingerprint in both diagnostics.
In `@crates/switch-controller/src/initializing.rs`:
- Around line 70-74: Update the tracing::info! message in the no-expected-switch
branch to describe the transition to SwitchControllerState::Error rather than
saying the controller is waiting. Preserve the existing switch_id and
bmc_mac_address fields and all state-transition behavior.
---
Outside diff comments:
In `@crates/host-support/src/registration.rs`:
- Around line 122-148: Update the tracing::debug message in
discover_machine_once to use discover_machine terminology instead of
register_machine, while preserving the existing request field and logging
behavior.
In `@crates/nvlink-manager/src/lib.rs`:
- Around line 514-571: Update all four “NMX-C partition not found” tracing error
calls in get_gpus_to_keep_after_removal and
get_gpus_to_keep_in_unknown_partition_after_removal to include
partition_nmx_c_id as a structured tracing field, while preserving the existing
machine_id, device_instance, and error message.
In `@crates/preingestion-manager/src/lib.rs`:
- Around line 3174-3256: Update the four logging sites in initiate_update to
include address = %endpoint_clone.address: the “Failed to resolve firmware
artifact” error, “Firmware artifact is being downloaded; update deferred” debug
message, “Firmware artifact is not present” error, and “Failed to enable RSHIM
on BMC” error. Preserve their existing messages and behavior.
---
Duplicate comments:
In `@crates/api-core/src/dpa/handler.rs`:
- Around line 151-153: Update the partition validation in the handler around the
partition lookup to distinguish zero matches from multiple matches: retain the
“not found” error only when partition.len() is zero, and add a separate error
for duplicate matches that includes the count, returning in both invalid cases.
- Around line 61-66: Update the DPA MAC parsing error branch around the MQTT
topic handler to bind the parse error instead of discarding it. Include the
bound error in the tracing::error! call using the structured field error =
%error, while retaining the existing mac_address field and return behavior.
---
Nitpick comments:
In `@crates/agent/src/dpu/interface.rs`:
- Line 70: Update the tracing call in the interface address-addition flow to use
Display formatting for the string interface name: change the interface field in
the log statement to %interface while retaining ?networks.
In `@crates/agent/src/extension_services/dpu_extension_service_observability.rs`:
- Around line 163-173: Merge the two command-failure tracing::error! calls into
one event that includes both stdout and stderr fields. Apply this in
crates/agent/src/extension_services/dpu_extension_service_observability.rs at
lines 163-173 and crates/agent/src/traffic_intercept_bridging.rs at lines
158-168, preserving the command field and existing failure message.
In `@crates/api-core/src/instance/mod.rs`:
- Around line 1427-1439: Add table-driven tests for the duplicate-attachment
validation in the instance allocation flow containing cases for duplicate
(device, device_instance) pairs returning the expected InvalidArgument error and
distinct attachments allocating successfully. Reuse the existing instance
allocation test helpers and assert each case’s result, including the duplicate
rejection and successful distinct-attachment behavior.
In `@crates/ib-fabric/src/lib.rs`:
- Around line 1082-1096: Replace the manual message-building loops in the three
missing-pkey logging branches with structured debug fields on the underlying
result vectors. Update the tracing calls around missing_guid_pkeys and the
corresponding branches at 1097-1111 and 1112-1127 to pass each list directly as
a ? field, removing the String construction, write! calls, and details field
while preserving the existing warning context and metrics updates.
In `@crates/machine-controller/src/handler.rs`:
- Around line 7867-7871: Demote the per-item diagnostic logs in
check_instance_network_synced_and_dpu_healthy from INFO to DEBUG, specifically
“Found device for dpu,” “Found device_instance for dpu,” and “checking dpu.”
Keep their messages and fields unchanged while ensuring all three no longer emit
at production INFO level.
In `@crates/rack-controller/src/created.rs`:
- Around line 96-100: In the tracing event in created.rs, update the fields for
compute_count, switch_count, and power_shelf_count to use native same-name
shorthand instead of explicit self-assignments. Leave the rack_id field and
event message unchanged.
🪄 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: 8243e9a1-9e73-48b7-9856-6f299da9715c
📒 Files selected for processing (271)
STYLE_GUIDE.mdcrates/admin-cli/src/machine/show/cmd.rscrates/admin-cli/src/rpc.rscrates/agent/src/agent_platform.rscrates/agent/src/astra_weave.rscrates/agent/src/containerd/container.rscrates/agent/src/containerd/image.rscrates/agent/src/dhcp_server_grpc_client.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/dpu_extension_service_observability.rscrates/agent/src/extension_services/k8s_pod_handler.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/instrumentation.rscrates/agent/src/lib.rscrates/agent/src/machine_inventory_updater.rscrates/agent/src/main_loop.rscrates/agent/src/managed_files.rscrates/agent/src/mtu.rscrates/agent/src/netlink.rscrates/agent/src/network_monitor.rscrates/agent/src/nvue.rscrates/agent/src/ovs.rscrates/agent/src/periodic_config_fetcher.rscrates/agent/src/tests/common/mod.rscrates/agent/src/tests/full.rscrates/agent/src/tests/test_network_monitor.rscrates/agent/src/traffic_intercept_bridging.rscrates/agent/src/upgrade.rscrates/api-core/src/attestation/measured_boot.rscrates/api-core/src/attestation/tpm_ca_cert.rscrates/api-core/src/db_init.rscrates/api-core/src/dhcp/discover.rscrates/api-core/src/dpa/handler.rscrates/api-core/src/dynamic_settings.rscrates/api-core/src/handlers/api.rscrates/api-core/src/handlers/astra.rscrates/api-core/src/handlers/attestation.rscrates/api-core/src/handlers/bmc_endpoint_explorer.rscrates/api-core/src/handlers/component_manager.rscrates/api-core/src/handlers/dns.rscrates/api-core/src/handlers/dpu_remediation.rscrates/api-core/src/handlers/expected_machine.rscrates/api-core/src/handlers/extension_service.rscrates/api-core/src/handlers/finder.rscrates/api-core/src/handlers/firmware.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_scout.rscrates/api-core/src/handlers/machine_validation.rscrates/api-core/src/handlers/rack.rscrates/api-core/src/handlers/redfish.rscrates/api-core/src/handlers/scout_stream.rscrates/api-core/src/handlers/site_explorer.rscrates/api-core/src/handlers/svpc.rscrates/api-core/src/handlers/uefi.rscrates/api-core/src/instance/mod.rscrates/api-core/src/ipxe.rscrates/api-core/src/listener.rscrates/api-core/src/logging/setup.rscrates/api-core/src/machine_update_manager/dpu_nic_firmware.rscrates/api-core/src/machine_update_manager/host_firmware.rscrates/api-core/src/machine_update_manager/mod.rscrates/api-core/src/machine_validation/mod.rscrates/api-core/src/measured_boot/metrics_collector/mod.rscrates/api-core/src/mqtt_state_change_hook/hook.rscrates/api-core/src/run.rscrates/api-core/src/scout_stream.rscrates/api-core/src/setup.rscrates/api-core/src/tests/common/api_fixtures/dpu.rscrates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-core/src/tests/common/api_fixtures/site_explorer.rscrates/api-core/src/tests/common/api_fixtures/test_machine/mod.rscrates/api-core/src/tests/dns.rscrates/api-core/src/tests/dpu_reprovisioning.rscrates/api-core/src/tests/instance.rscrates/api-core/src/tests/machine_network.rscrates/api-core/src/tests/sku.rscrates/api-core/tests/integration/machine_bmc_metadata.rscrates/api-db/src/bmc_metadata.rscrates/api-db/src/carbide_version.rscrates/api-db/src/dns/resource_record.rscrates/api-db/src/expected_power_shelf.rscrates/api-db/src/expected_switch.rscrates/api-db/src/instance.rscrates/api-db/src/lib.rscrates/api-db/src/machine.rscrates/api-db/src/machine_interface.rscrates/api-db/src/machine_topology.rscrates/api-db/src/measured_boot/bundle.rscrates/api-db/src/vpc_dpu_loopback.rscrates/api-db/src/work_lock_manager.rscrates/api-integration-tests/tests/lib.rscrates/api-model/src/dpa_interface/mod.rscrates/api-model/src/firmware.rscrates/api-model/src/instance/status/network.rscrates/api-model/src/instance/status/nvlink.rscrates/api-model/src/instance/status/spx.rscrates/api-model/src/machine/mod.rscrates/api-model/src/machine/nvlink.rscrates/api-model/src/machine/spx.rscrates/api-model/src/site_explorer/mod.rscrates/api-test-helper/src/domain.rscrates/api-test-helper/src/instance.rscrates/api-test-helper/src/machine.rscrates/api-test-helper/src/machine_a_tron.rscrates/api-test-helper/src/subnet.rscrates/api-test-helper/src/tenant.rscrates/api-test-helper/src/vpc.rscrates/api-test-helper/src/vpc_prefix.rscrates/api-web/src/auth.rscrates/api-web/src/interface.rscrates/api-web/src/lib.rscrates/api-web/src/machine.rscrates/api-web/src/managed_host.rscrates/api-web/src/search.rscrates/bmc-explorer/src/chassis.rscrates/bmc-explorer/src/computer_system.rscrates/bmc-explorer/src/manager.rscrates/bmc-mock/src/combined_server.rscrates/bmc-mock/src/combined_service.rscrates/bmc-mock/src/main.rscrates/bmc-mock/src/redfish/expander_router.rscrates/bmc-mock/src/tar_router.rscrates/bmc-mock/src/tls.rscrates/bmc-proxy/src/bmc_proxy.rscrates/bmc-proxy/src/setup.rscrates/dhcp-server/src/cache.rscrates/dhcp-server/src/grpc_server.rscrates/dhcp-server/src/main.rscrates/dhcp-server/src/modes/controller.rscrates/dhcp-server/src/packet_handler.rscrates/dhcp-server/src/util.rscrates/dns-record/src/lib.rscrates/dns/src/lib.rscrates/dns/src/main.rscrates/dpa-manager/src/card_handler/astra.rscrates/dpa-manager/src/card_handler/svpc.rscrates/dpa-manager/src/lib.rscrates/dpa/src/lib.rscrates/dpf/src/sdk.rscrates/dpu-otel-agent/src/lib.rscrates/dpu-remediation/src/remediation.rscrates/dsx-exchange-consumer/src/api_client.rscrates/firmware/src/config.rscrates/firmware/src/downloader.rscrates/fmds/src/http_request_metrics.rscrates/fmds/src/main.rscrates/fmds/src/nic_init.rscrates/fmds/src/phone_home.rscrates/health/src/api_client.rscrates/health/src/collectors/logs/periodic.rscrates/health/src/discovery/cleanup.rscrates/health/src/discovery/context.rscrates/health/src/discovery/spawn.rscrates/health/src/metrics.rscrates/host-support/src/hardware_enumeration.rscrates/host-support/src/hardware_enumeration/dpu.rscrates/host-support/src/hardware_enumeration/gpu.rscrates/host-support/src/hardware_enumeration/tpm.rscrates/host-support/src/registration.rscrates/http-connector/src/connector.rscrates/http-connector/src/resolver.rscrates/ib-fabric/src/lib.rscrates/ib-partition-controller/src/handler.rscrates/ipmi/src/tool.rscrates/kms-provider/src/providers/transit.rscrates/libmlx/src/device/discovery.rscrates/libnmxc/src/lib.rscrates/libnmxm/src/lib.rscrates/machine-a-tron/src/api_client.rscrates/machine-a-tron/src/api_throttler.rscrates/machine-a-tron/src/bmc_mock_wrapper.rscrates/machine-a-tron/src/dhcp_wrapper.rscrates/machine-a-tron/src/host_machine.rscrates/machine-a-tron/src/machine_a_tron.rscrates/machine-a-tron/src/machine_state_machine.rscrates/machine-a-tron/src/machine_utils.rscrates/machine-a-tron/src/main.rscrates/machine-a-tron/src/mock_ssh_server.rscrates/machine-a-tron/src/subnet.rscrates/machine-a-tron/src/tui.rscrates/machine-a-tron/src/vpc.rscrates/machine-controller/src/dpf.rscrates/machine-controller/src/handler.rscrates/machine-controller/src/handler/attestation.rscrates/machine-controller/src/handler/bios_config.rscrates/machine-controller/src/handler/dpf.rscrates/machine-controller/src/handler/machine_validation.rscrates/machine-controller/src/handler/power.rscrates/machine-controller/src/handler/sku.rscrates/machine-validation/src/machine_validation.rscrates/metrics-endpoint/src/lib.rscrates/metrics-utils/src/lib.rscrates/mqttea-example/src/main.rscrates/mqttea/src/client/core.rscrates/mqttea/src/client/messages.rscrates/mqttea/src/registry/core.rscrates/nvlink-manager/src/lib.rscrates/nvlink-manager/src/switch_cert_monitor.rscrates/power-shelf-controller/src/configuring.rscrates/power-shelf-controller/src/deleting.rscrates/power-shelf-controller/src/fetching_data.rscrates/power-shelf-controller/src/initializing.rscrates/preingestion-manager/src/bfb_rshim_copier.rscrates/preingestion-manager/src/lib.rscrates/preingestion-manager/tests/integration/host_bmc_firmware.rscrates/rack-controller/src/created.rscrates/rack-controller/src/discovering.rscrates/rack-controller/src/error_state.rscrates/rack-controller/src/handler.rscrates/rack-controller/src/lib.rscrates/rack-controller/src/maintenance.rscrates/rack-controller/src/ready.rscrates/rack-controller/src/validating.rscrates/rpc-utils/src/managed_host_display.rscrates/rpc/src/errors.rscrates/rpc/src/forge_tls_client.rscrates/rpc/src/model/dpu_remediation.rscrates/scout/src/attestation.rscrates/scout/src/deprovision/scrabbing.rscrates/scout/src/firmware_upgrade.rscrates/scout/src/main.rscrates/scout/src/mlx_device.rscrates/scout/src/register.rscrates/scout/src/stream.rscrates/scout/src/tpm.rscrates/secrets/src/forge_vault.rscrates/secrets/src/local_credentials/file.rscrates/site-explorer/src/bmc_endpoint_explorer.rscrates/site-explorer/src/explored_endpoint_index.rscrates/site-explorer/src/lib.rscrates/site-explorer/src/machine_creator.rscrates/site-explorer/src/redfish.rscrates/site-explorer/src/switch_creator.rscrates/site-explorer/src/test_support/mock_endpoint_explorer.rscrates/site-explorer/tests/integration/power_shelf.rscrates/site-explorer/tests/integration/site_explorer.rscrates/site-explorer/tests/integration/switch.rscrates/spdm-controller/src/handler.rscrates/ssh-console-mock-api-server/src/lib.rscrates/ssh-console/src/bmc/client.rscrates/ssh-console/src/bmc/connection.rscrates/ssh-console/src/bmc/connection_impl/ipmi.rscrates/ssh-console/src/bmc/connection_impl/ssh.rscrates/ssh-console/src/bmc/message_proxy.rscrates/ssh-console/src/config.rscrates/ssh-console/src/console_logger.rscrates/ssh-console/src/frontend.rscrates/ssh-console/src/metrics.rscrates/ssh-console/src/ssh_cert_parsing.rscrates/ssh-console/src/ssh_server.rscrates/ssh-console/tests/util/ipmi_sim.rscrates/ssh-console/tests/util/mod.rscrates/state-controller/src/controller/processor.rscrates/switch-controller/src/certificate.rscrates/switch-controller/src/configuring.rscrates/switch-controller/src/created.rscrates/switch-controller/src/deleting.rscrates/switch-controller/src/error_state.rscrates/switch-controller/src/initializing.rscrates/switch-controller/src/ready.rscrates/switch-controller/src/validating.rs
💤 Files with no reviewable changes (1)
- crates/health/src/discovery/context.rs
chet
force-pushed
the
gh-issue-3476
branch
2 times, most recently
from
15bab2c to
86829a2
Compare
Contributor
Author
|
Final review consolidation is in 86829a2:
@coderabbitai full_review, thanks! |
Contributor
|
Kicking off the full review now to verify the final delta. (=^･ω･^=) ✏️ Learnings added
✅ Action performedFull review finished. |
This was referenced Jul 14, 2026
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-3479.docs.buildwithfern.com/infra-controller |
10 tasks
NICo's tracing events now keep operational values in named fields and use stable messages, making logfmt output easier to search and aggregate. The sweep keeps presentation-oriented CLI and passthrough output intact where the rendered text is itself the payload. Primary callouts: - Convert roughly 1,400 direct tracing events across more than 50 crates to stable messages with semantic fields. - Keep IDs, endpoints, counts, states, and errors typed and searchable while preserving trace levels and intentional Display, Debug, and custom formatting. - Strengthen STYLE_GUIDE.md so new tracing events follow the same structured-logging convention. - Leave CLI output, child-process streams, aligned or multiline displays, and logging fixtures rendered where the text itself is the payload. This supports NVIDIA#3476 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
Contributor
Author
|
Final main rebase is in 603a407:
@coderabbitai full_review, thanks! |
Contributor
|
Kicking off the full review now to verify the rebased delta. (=^･ω･^=) ✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/preingestion-manager/src/lib.rs (1)
2172-2203: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winInconsistent field name for the same "attempt count" value within one function.
At line 2181 the retry-branch logs the same
attempts_donevariable asattempt = attempts_done, but the terminal-failure branch a few lines later (line 2200) logs it asattempts = attempts_done. Two consecutive branches oftime_sync_resets, same variable, two different field keys — this splits what should be one searchable field into two in log tooling. The rest of the file (lines 1567, 1586, 1796, 1816) consistently uses the singularattempt, so line 2200 is the outlier.As per coding guidelines, dynamic values should use "consistent semantic field names rather than incidental local variable names."
🔧 Proposed fix
tracing::error!( address = %endpoint.address, - attempts = attempts_done, + attempt = attempts_done, max_attempts = MAX_TIME_SYNC_RESET_ATTEMPTS, "BMC time is still out of sync after reset attempts; failing preingestion" );🤖 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/preingestion-manager/src/lib.rs` around lines 2172 - 2203, Use the same singular tracing field name for the attempt count in the terminal-failure branch of time_sync_resets: change the attempts field in the error log to attempt while continuing to log attempts_done as its value.Source: Coding guidelines
🧹 Nitpick comments (9)
crates/rpc-utils/src/managed_host_display.rs (1)
66-73: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
Displayfor this error field. Useerror = %einstead of?ehere;tonic::StatusimplementsDisplay, and this keeps the log output consistent with the other error fields in this file.🤖 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/rpc-utils/src/managed_host_display.rs` around lines 66 - 73, Update the error field in the site exploration report handling within the site exploration report retrieval flow to use Display formatting (`%e`) instead of Debug formatting (`?e`) in the warn! call, matching the file’s existing error logging style.Source: Coding guidelines
crates/agent/src/traffic_intercept_bridging.rs (1)
101-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse native tracing formatting for the error field.
error = format!("{err:#}")eagerly allocates and flattens the error before the subscriber formats it. Prefererror = %err; use?erronly if Debug-chain output is intentionally required. As per coding guidelines, errors should use semantic structured formatting such aserror = %e.Proposed fix
tracing::error!( - error = format!("{err:#}"), + error = %err, "update_intercept_bridging command failed" );🤖 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/agent/src/traffic_intercept_bridging.rs` around lines 101 - 104, Update the error field in the tracing call within the update_intercept_bridging failure path to use native semantic formatting (`%err`) instead of eagerly allocating with format!. Preserve the existing error message and failure handling.Source: Coding guidelines
crates/agent/src/machine_inventory_updater.rs (1)
144-144: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse human-readable tracing messages.
"update_machine_inventory"and"update_agent_reported_inventory response"expose Rust/RPC identifier spelling instead of operator-facing prose. Prefer messages such as"Updating machine inventory"and"Received agent-reported inventory update response".As per coding guidelines, tracing messages must be stable human-readable literals.
Also applies to: 150-150
🤖 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/agent/src/machine_inventory_updater.rs` at line 144, Update the tracing message literals in the update_machine_inventory flow, including the messages at the referenced locations, to use stable human-readable prose. Replace the Rust/RPC identifier-style text with operator-facing messages such as “Updating machine inventory” and “Received agent-reported inventory update response,” while leaving the surrounding tracing behavior unchanged.Source: Coding guidelines
crates/agent/src/main_loop.rs (1)
158-160: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid pre-formatting errors into
Stringfields.These sites allocate with
format!("{...:#}")before tracing and discard the error's native structured formatting. Useerror = %e/error = %errfor ordinary error fields; retain alternate formatting only as an explicit, documented exception.As per coding guidelines, errors should be recorded through structured Display/Debug fields rather than pre-rendered strings.
Also applies to: 173-173, 188-191, 200-203, 583-586, 736-739
🤖 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/agent/src/main_loop.rs` around lines 158 - 160, Replace the preformatted format!("{e:#}") and equivalent error fields in the affected main-loop tracing calls with tracing’s native structured Display formatting, using error = %e or error = %err as appropriate. Update all listed sites, including the warning around the external FMDS fallback and the corresponding error logs, without changing their messages or control flow.Source: Coding guidelines
crates/api-core/src/tests/sku.rs (1)
1300-1314: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse descriptive event messages for SKU snapshots.
"SKU1"and"SKU2"are too ambiguous in aggregated logs. Use messages that identify the snapshot, such as"Original SKU before mismatch"and"Broken SKU after mismatch".As per coding guidelines, tracing messages must be stable, human-readable literals that describe the event.
🤖 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/api-core/src/tests/sku.rs` around lines 1300 - 1314, Replace the ambiguous tracing event messages in the original_sku and broken_sku snapshot logs with stable, descriptive human-readable literals identifying each snapshot, such as the original SKU before mismatch and broken SKU after mismatch. Keep the existing structured fields unchanged.Source: Coding guidelines
crates/site-explorer/src/switch_creator.rs (1)
78-84: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the new
bmc_ip_addressfield with the existingbmc_ipfield name/format.This newly added field logs the same value (
explored_managed_switch.bmc_ip) that's already logged a few lines above asbmc_ip = %explored_managed_switch.bmc_ip(L55). Using a different key (bmc_ip_address) and a different formatter (?instead of%) for the identical field in the same file undermines the searchability goal of this migration.♻️ Proposed fix
Err(error) => { tracing::error!( %error, - bmc_ip_address = ?explored_managed_switch.bmc_ip, + bmc_ip = %explored_managed_switch.bmc_ip, "Failed to create managed switch" ); }🤖 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/site-explorer/src/switch_creator.rs` around lines 78 - 84, Update the tracing::error! call in the Err(error) branch to log explored_managed_switch.bmc_ip using the existing bmc_ip key and % formatter, matching the earlier log statement; remove the bmc_ip_address key and ? formatter.crates/site-explorer/tests/integration/power_shelf.rs (1)
113-117: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated "DHCP assigned ip" log block into a helper.
The identical 3-field structured log call (
mac_address,ip_address,"DHCP assigned ip") appears verbatim in 8 test functions in this file. A small helper, e.g.fn log_dhcp_assignment(mac: &MacAddress, ip: &str), would remove the duplication and centralize the field convention for future changes.♻️ Proposed helper
fn log_dhcp_assignment(mac_address: &MacAddress, ip_address: &str) { tracing::info!(%mac_address, %ip_address, "DHCP assigned ip"); }Then replace each of the 8 call sites with
log_dhcp_assignment(&power_shelf.bmc_mac_address, &response.address);.Also applies to: 473-477, 582-586, 692-696, 796-800, 894-898, 1067-1071, 1521-1525
🤖 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/site-explorer/tests/integration/power_shelf.rs` around lines 113 - 117, Extract the repeated structured “DHCP assigned ip” logging from the affected test functions into a shared log_dhcp_assignment helper accepting MacAddress and IP string references, preserving the existing mac_address and ip_address fields and message. Replace all eight identical tracing::info! call sites, including those in the listed test flows, with the helper using power_shelf.bmc_mac_address and response.address.crates/scout/src/main.rs (1)
190-202: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
%efor these error fields to match the file’s logging convention.Both error branches here should log
error = %einstead oferror = ?e; the error type isDisplay-backed, and this file already uses%econsistently for error fields.🤖 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/main.rs` around lines 190 - 202, Update both error branches in the PublishMlxDeviceReport request flow to log the captured error with Display formatting by changing the error fields in the create and publish failure tracing calls from debug formatting to `%e`, preserving their existing messages and behavior.Source: Learnings
crates/machine-controller/src/handler.rs (1)
7249-7252: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInconsistent field name for DPU
MachineId:dpu_idvsmachine_id.Within this same diff hunk, new logs alternate between
dpu_id = %dpu.id(Lines 7249, 7569, 7578) andmachine_id = %dpu_id(Line 7608) for the identical semantic value — a DPU'sMachineId. As per coding guidelines, "Use consistent semantic field names rather than incidental local variable names." Pick one key (the file overwhelmingly usesmachine_idelsewhere, including for DPUs) so cross-log queries stay uniform.Also applies to: 7568-7580, 7601-7608
🤖 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/machine-controller/src/handler.rs` around lines 7249 - 7252, Use the consistent machine_id field name for all newly added DPU MachineId tracing fields, including the logs around the use_admin_network_changed flag and the corresponding entries near machine_id = %dpu_id. Replace dpu_id keys that log the same semantic DPU identifier while preserving the existing identifier values and messages.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/agent/src/health.rs`:
- Around line 325-330: Update the tracing messages in the check_ifreload and
check_files paths to use stable, descriptive operator-facing text: replace the
check_ifreload labels with “ifreload syntax check failed” and replace
“check_files: Too small” with “file is too small,” preserving all existing
structured fields.
In `@crates/agent/src/mtu.rs`:
- Around line 59-61: Update both MTU command failure paths in the query and
update flows to append the raw child-process stderr to the rendered log message
payload, while keeping command as a structured field. Do not rely on the
structured stderr field alone; preserve the existing operator-visible multiline
output for both failures.
In `@crates/agent/src/ovs.rs`:
- Around line 47-51: Update the OVS command failure logging around the existing
tracing::error! call to keep command structured while rendering stdout and
stderr as part of the message payload, preserving their multiline and escaping
behavior. Do not emit the process streams as structured fields; retain the
existing "OVS command failed" context and use the output values directly in the
rendered log message.
In `@crates/api-core/src/handlers/astra.rs`:
- Around line 265-267: Update the error fields in both Astra
acknowledgment-config parsing log sites to use Display formatting (`%e`) instead
of Debug formatting (`?e`). Apply this to the `ConfigVersionParseError` and
`DatabaseError` log entries while preserving their existing messages and fields.
In `@crates/api-core/src/handlers/finder.rs`:
- Around line 518-520: Standardize the tracing fields in the by_mac logging
paths, updating the existing machine-interface error log near the
interface_count entry to use mac_address for the MAC value and error for the
error value, matching the new branch. Preserve the surrounding messages and
behavior.
In `@crates/api-core/src/handlers/instance_type.rs`:
- Line 601: Update the stable tracing message in the instance update handler to
use grammatically precise, human-readable wording such as “Not all machine
associations were updated,” while preserving the existing error context and
message behavior.
In `@crates/api-model/src/instance/status/network.rs`:
- Around line 292-297: Update the missing-DPU collection and logging near the
`missing_dpus` check so sentinel `None` values are not recorded as identifiers.
In the `None` branch that populates `missing_dpus`, collect the actual
configured/device `MachineId` when available, or rename `missing_dpu_ids` to
accurately describe the stored values; ensure the log never labels `[None, ...]`
as DPU IDs.
In `@crates/api-test-helper/src/machine.rs`:
- Line 66: Update the tracing::info! call that logs the current state to use the
stable literal "Current" instead of "\tCurrent", leaving the existing state
field unchanged.
In `@crates/dpa-manager/src/card_handler/astra.rs`:
- Around line 128-129: Update the unexpected-state warning events in the Astra
DPA handler to retain the existing controller state as a structured logging
field alongside dpa_interface.id. Apply this consistently to the warning sites
around lines 128, 160, 190, and 220, without removing or converting the dynamic
state value into message text.
In `@crates/host-support/src/hardware_enumeration.rs`:
- Around line 417-425: Update the NIC enumeration log in the loop over devices
to emit the sysfs path under the consistent `syspath` field name, rather than
relying on the local `sys_path` variable name via shorthand. Preserve the
existing debug message and path value.
In `@crates/host-support/src/registration.rs`:
- Line 112: Use Display formatting for the error fields at both affected call
sites: update the TLS-client error log in
crates/host-support/src/registration.rs (lines 112-112) and the connection error
log in crates/http-connector/src/connector.rs (lines 219-219) from debug
formatting to Display formatting with %e, preserving the existing log messages
and context.
In `@crates/machine-a-tron/src/tui.rs`:
- Around line 465-468: Update the error field in the tracing::warn! invocation
handling Some(Err(e)) to use Display formatting (`%e`) instead of Debug
formatting (`?e`), matching the workspace logging convention while preserving
the existing warning message.
In `@crates/machine-controller/src/handler.rs`:
- Around line 9645-9654: Update the logging statements in
host_new_firmware_reported_wait for the “Not all firmware versions match”
warning and the “resetting again” info branch to include the available
machine_id field, matching the other logs in the function; leave the existing
address and firmware details unchanged.
- Around line 9336-9337: Update the 404-recovery log in host_waiting_fw for the
missing-task completion branch to include the in-scope machine_id field
alongside endpoint.address, matching the sibling longer-wait branch’s structured
logging.
- Around line 8644-8706: Change the cmd.wait() Err branch in the upgrade-script
task to use tracing::error! instead of tracing::info!, while preserving the
existing machine_id, address, error, message, and completion-state handling.
- Around line 3884-3888: Update the failure log in the EnableRshim branch of
handle_dpu_discovering_state to include the in-scope dpu_machine_id alongside
the existing error field, matching the identifier context used by nearby logs.
- Around line 2557-2568: Update the tracing::info call in the
Some(TaskState::Completed) branch of the task.task_state match to include the
existing machine_id and task_id fields alongside bmc_address, preserving the
"Install BFB completed" message.
In `@crates/machine-controller/src/handler/sku.rs`:
- Line 129: Update the “No expected machine for bmc” debug log in
generate_missing_sku_for_machine to include the available machine_id field
alongside bmc_mac_address, preserving the existing message and log level.
In `@crates/machine-validation/src/machine_validation.rs`:
- Around line 365-373: Update the success and failure logging in pull_container
to include the same image_name identifying field in both branches, while
preserving stdout on success and error on failure. Apply the same symmetry in
run by ensuring both branches log test_name, retaining each branch’s existing
result-specific fields.
In `@crates/preingestion-manager/src/lib.rs`:
- Around line 3253-3268: Update the “Using simple update for firmware upload”
debug log in the BFB artifact branch of initiate_update to include the
structured address field using endpoint_clone.address, matching the correlation
fields on the surrounding logs.
---
Outside diff comments:
In `@crates/preingestion-manager/src/lib.rs`:
- Around line 2172-2203: Use the same singular tracing field name for the
attempt count in the terminal-failure branch of time_sync_resets: change the
attempts field in the error log to attempt while continuing to log attempts_done
as its value.
---
Nitpick comments:
In `@crates/agent/src/machine_inventory_updater.rs`:
- Line 144: Update the tracing message literals in the update_machine_inventory
flow, including the messages at the referenced locations, to use stable
human-readable prose. Replace the Rust/RPC identifier-style text with
operator-facing messages such as “Updating machine inventory” and “Received
agent-reported inventory update response,” while leaving the surrounding tracing
behavior unchanged.
In `@crates/agent/src/main_loop.rs`:
- Around line 158-160: Replace the preformatted format!("{e:#}") and equivalent
error fields in the affected main-loop tracing calls with tracing’s native
structured Display formatting, using error = %e or error = %err as appropriate.
Update all listed sites, including the warning around the external FMDS fallback
and the corresponding error logs, without changing their messages or control
flow.
In `@crates/agent/src/traffic_intercept_bridging.rs`:
- Around line 101-104: Update the error field in the tracing call within the
update_intercept_bridging failure path to use native semantic formatting
(`%err`) instead of eagerly allocating with format!. Preserve the existing error
message and failure handling.
In `@crates/api-core/src/tests/sku.rs`:
- Around line 1300-1314: Replace the ambiguous tracing event messages in the
original_sku and broken_sku snapshot logs with stable, descriptive
human-readable literals identifying each snapshot, such as the original SKU
before mismatch and broken SKU after mismatch. Keep the existing structured
fields unchanged.
In `@crates/machine-controller/src/handler.rs`:
- Around line 7249-7252: Use the consistent machine_id field name for all newly
added DPU MachineId tracing fields, including the logs around the
use_admin_network_changed flag and the corresponding entries near machine_id =
%dpu_id. Replace dpu_id keys that log the same semantic DPU identifier while
preserving the existing identifier values and messages.
In `@crates/rpc-utils/src/managed_host_display.rs`:
- Around line 66-73: Update the error field in the site exploration report
handling within the site exploration report retrieval flow to use Display
formatting (`%e`) instead of Debug formatting (`?e`) in the warn! call, matching
the file’s existing error logging style.
In `@crates/scout/src/main.rs`:
- Around line 190-202: Update both error branches in the PublishMlxDeviceReport
request flow to log the captured error with Display formatting by changing the
error fields in the create and publish failure tracing calls from debug
formatting to `%e`, preserving their existing messages and behavior.
In `@crates/site-explorer/src/switch_creator.rs`:
- Around line 78-84: Update the tracing::error! call in the Err(error) branch to
log explored_managed_switch.bmc_ip using the existing bmc_ip key and %
formatter, matching the earlier log statement; remove the bmc_ip_address key and
? formatter.
In `@crates/site-explorer/tests/integration/power_shelf.rs`:
- Around line 113-117: Extract the repeated structured “DHCP assigned ip”
logging from the affected test functions into a shared log_dhcp_assignment
helper accepting MacAddress and IP string references, preserving the existing
mac_address and ip_address fields and message. Replace all eight identical
tracing::info! call sites, including those in the listed test flows, with the
helper using power_shelf.bmc_mac_address and response.address.
🪄 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: 1c8ac7da-8be1-4b4d-b1c5-55ef542ef20d
📒 Files selected for processing (272)
STYLE_GUIDE.mdcrates/admin-cli/src/machine/show/cmd.rscrates/admin-cli/src/rpc.rscrates/agent/src/agent_platform.rscrates/agent/src/astra_weave.rscrates/agent/src/containerd/container.rscrates/agent/src/containerd/image.rscrates/agent/src/dhcp_server_grpc_client.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/dpu_extension_service_observability.rscrates/agent/src/extension_services/k8s_pod_handler.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/instrumentation.rscrates/agent/src/lib.rscrates/agent/src/machine_inventory_updater.rscrates/agent/src/main_loop.rscrates/agent/src/managed_files.rscrates/agent/src/mtu.rscrates/agent/src/netlink.rscrates/agent/src/network_monitor.rscrates/agent/src/nvue.rscrates/agent/src/ovs.rscrates/agent/src/periodic_config_fetcher.rscrates/agent/src/tests/common/mod.rscrates/agent/src/tests/full.rscrates/agent/src/tests/test_network_monitor.rscrates/agent/src/traffic_intercept_bridging.rscrates/agent/src/upgrade.rscrates/api-core/src/attestation/measured_boot.rscrates/api-core/src/attestation/tpm_ca_cert.rscrates/api-core/src/db_init.rscrates/api-core/src/dhcp/discover.rscrates/api-core/src/dpa/handler.rscrates/api-core/src/dynamic_settings.rscrates/api-core/src/handlers/api.rscrates/api-core/src/handlers/astra.rscrates/api-core/src/handlers/attestation.rscrates/api-core/src/handlers/bmc_endpoint_explorer.rscrates/api-core/src/handlers/component_manager.rscrates/api-core/src/handlers/dns.rscrates/api-core/src/handlers/dpu_remediation.rscrates/api-core/src/handlers/expected_machine.rscrates/api-core/src/handlers/extension_service.rscrates/api-core/src/handlers/finder.rscrates/api-core/src/handlers/firmware.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_scout.rscrates/api-core/src/handlers/machine_validation.rscrates/api-core/src/handlers/rack.rscrates/api-core/src/handlers/redfish.rscrates/api-core/src/handlers/scout_stream.rscrates/api-core/src/handlers/site_explorer.rscrates/api-core/src/handlers/svpc.rscrates/api-core/src/handlers/uefi.rscrates/api-core/src/instance/mod.rscrates/api-core/src/ipxe.rscrates/api-core/src/listener.rscrates/api-core/src/logging/setup.rscrates/api-core/src/machine_update_manager/dpu_nic_firmware.rscrates/api-core/src/machine_update_manager/host_firmware.rscrates/api-core/src/machine_update_manager/mod.rscrates/api-core/src/machine_validation/mod.rscrates/api-core/src/measured_boot/metrics_collector/mod.rscrates/api-core/src/mqtt_state_change_hook/hook.rscrates/api-core/src/run.rscrates/api-core/src/scout_stream.rscrates/api-core/src/setup.rscrates/api-core/src/tests/common/api_fixtures/dpu.rscrates/api-core/src/tests/common/api_fixtures/mod.rscrates/api-core/src/tests/common/api_fixtures/site_explorer.rscrates/api-core/src/tests/common/api_fixtures/test_machine/mod.rscrates/api-core/src/tests/dns.rscrates/api-core/src/tests/dpu_reprovisioning.rscrates/api-core/src/tests/instance.rscrates/api-core/src/tests/machine_network.rscrates/api-core/src/tests/sku.rscrates/api-core/tests/integration/machine_bmc_metadata.rscrates/api-db/src/bmc_metadata.rscrates/api-db/src/carbide_version.rscrates/api-db/src/dns/resource_record.rscrates/api-db/src/expected_power_shelf.rscrates/api-db/src/expected_switch.rscrates/api-db/src/instance.rscrates/api-db/src/lib.rscrates/api-db/src/machine.rscrates/api-db/src/machine_interface.rscrates/api-db/src/machine_topology.rscrates/api-db/src/measured_boot/bundle.rscrates/api-db/src/vpc_dpu_loopback.rscrates/api-db/src/work_lock_manager.rscrates/api-integration-tests/tests/lib.rscrates/api-model/src/dpa_interface/mod.rscrates/api-model/src/firmware.rscrates/api-model/src/instance/status/network.rscrates/api-model/src/instance/status/nvlink.rscrates/api-model/src/instance/status/spx.rscrates/api-model/src/machine/mod.rscrates/api-model/src/machine/nvlink.rscrates/api-model/src/machine/spx.rscrates/api-model/src/site_explorer/mod.rscrates/api-test-helper/src/domain.rscrates/api-test-helper/src/instance.rscrates/api-test-helper/src/machine.rscrates/api-test-helper/src/machine_a_tron.rscrates/api-test-helper/src/subnet.rscrates/api-test-helper/src/tenant.rscrates/api-test-helper/src/vpc.rscrates/api-test-helper/src/vpc_prefix.rscrates/api-web/src/auth.rscrates/api-web/src/interface.rscrates/api-web/src/lib.rscrates/api-web/src/machine.rscrates/api-web/src/managed_host.rscrates/api-web/src/search.rscrates/bmc-explorer/src/chassis.rscrates/bmc-explorer/src/computer_system.rscrates/bmc-explorer/src/manager.rscrates/bmc-mock/src/combined_server.rscrates/bmc-mock/src/combined_service.rscrates/bmc-mock/src/main.rscrates/bmc-mock/src/redfish/expander_router.rscrates/bmc-mock/src/tar_router.rscrates/bmc-mock/src/tls.rscrates/bmc-proxy/src/bmc_proxy.rscrates/bmc-proxy/src/setup.rscrates/dhcp-server/src/cache.rscrates/dhcp-server/src/grpc_server.rscrates/dhcp-server/src/main.rscrates/dhcp-server/src/modes/controller.rscrates/dhcp-server/src/packet_handler.rscrates/dhcp-server/src/util.rscrates/dns-record/src/lib.rscrates/dns/src/lib.rscrates/dns/src/main.rscrates/dpa-manager/src/card_handler/astra.rscrates/dpa-manager/src/card_handler/svpc.rscrates/dpa-manager/src/lib.rscrates/dpa/src/lib.rscrates/dpf/src/sdk.rscrates/dpu-otel-agent/src/lib.rscrates/dpu-remediation/src/remediation.rscrates/dsx-exchange-consumer/src/api_client.rscrates/firmware/src/config.rscrates/firmware/src/downloader.rscrates/fmds/src/http_request_metrics.rscrates/fmds/src/main.rscrates/fmds/src/nic_init.rscrates/fmds/src/phone_home.rscrates/health/src/api_client.rscrates/health/src/collectors/logs/periodic.rscrates/health/src/discovery/cleanup.rscrates/health/src/discovery/context.rscrates/health/src/discovery/spawn.rscrates/health/src/metrics.rscrates/host-support/src/hardware_enumeration.rscrates/host-support/src/hardware_enumeration/dpu.rscrates/host-support/src/hardware_enumeration/gpu.rscrates/host-support/src/hardware_enumeration/tpm.rscrates/host-support/src/registration.rscrates/http-connector/src/connector.rscrates/http-connector/src/resolver.rscrates/ib-fabric/src/lib.rscrates/ib-partition-controller/src/handler.rscrates/ipmi/src/tool.rscrates/kms-provider/src/providers/transit.rscrates/libmlx/src/device/discovery.rscrates/libnmxc/src/lib.rscrates/libnmxm/src/lib.rscrates/machine-a-tron/src/api_client.rscrates/machine-a-tron/src/api_throttler.rscrates/machine-a-tron/src/bmc_mock_wrapper.rscrates/machine-a-tron/src/dhcp_wrapper.rscrates/machine-a-tron/src/host_machine.rscrates/machine-a-tron/src/machine_a_tron.rscrates/machine-a-tron/src/machine_state_machine.rscrates/machine-a-tron/src/machine_utils.rscrates/machine-a-tron/src/main.rscrates/machine-a-tron/src/mock_ssh_server.rscrates/machine-a-tron/src/subnet.rscrates/machine-a-tron/src/tui.rscrates/machine-a-tron/src/vpc.rscrates/machine-controller/src/dpf.rscrates/machine-controller/src/handler.rscrates/machine-controller/src/handler/attestation.rscrates/machine-controller/src/handler/bios_config.rscrates/machine-controller/src/handler/dpf.rscrates/machine-controller/src/handler/machine_validation.rscrates/machine-controller/src/handler/power.rscrates/machine-controller/src/handler/sku.rscrates/machine-validation/src/machine_validation.rscrates/metrics-endpoint/src/lib.rscrates/metrics-utils/src/lib.rscrates/mqttea-example/src/main.rscrates/mqttea/src/client/core.rscrates/mqttea/src/client/messages.rscrates/mqttea/src/registry/core.rscrates/nvlink-manager/src/lib.rscrates/nvlink-manager/src/switch_cert_monitor.rscrates/power-shelf-controller/src/configuring.rscrates/power-shelf-controller/src/deleting.rscrates/power-shelf-controller/src/fetching_data.rscrates/power-shelf-controller/src/initializing.rscrates/preingestion-manager/src/bfb_rshim_copier.rscrates/preingestion-manager/src/lib.rscrates/preingestion-manager/tests/integration/host_bmc_firmware.rscrates/rack-controller/src/created.rscrates/rack-controller/src/discovering.rscrates/rack-controller/src/error_state.rscrates/rack-controller/src/handler.rscrates/rack-controller/src/lib.rscrates/rack-controller/src/maintenance.rscrates/rack-controller/src/ready.rscrates/rack-controller/src/validating.rscrates/rpc-utils/src/managed_host_display.rscrates/rpc/src/errors.rscrates/rpc/src/forge_tls_client.rscrates/rpc/src/model/dpu_remediation.rscrates/scout/src/attestation.rscrates/scout/src/deprovision/scrabbing.rscrates/scout/src/firmware_upgrade.rscrates/scout/src/main.rscrates/scout/src/mlx_device.rscrates/scout/src/register.rscrates/scout/src/stream.rscrates/scout/src/tpm.rscrates/secrets/src/forge_vault.rscrates/secrets/src/local_credentials/file.rscrates/site-explorer/src/bmc_endpoint_explorer.rscrates/site-explorer/src/explored_endpoint_index.rscrates/site-explorer/src/lib.rscrates/site-explorer/src/machine_creator.rscrates/site-explorer/src/redfish.rscrates/site-explorer/src/switch_creator.rscrates/site-explorer/src/test_support/mock_endpoint_explorer.rscrates/site-explorer/tests/integration/power_shelf.rscrates/site-explorer/tests/integration/site_explorer.rscrates/site-explorer/tests/integration/switch.rscrates/spdm-controller/src/handler.rscrates/ssh-console-mock-api-server/src/lib.rscrates/ssh-console/src/bmc/client.rscrates/ssh-console/src/bmc/connection.rscrates/ssh-console/src/bmc/connection_impl/ipmi.rscrates/ssh-console/src/bmc/connection_impl/ssh.rscrates/ssh-console/src/bmc/message_proxy.rscrates/ssh-console/src/config.rscrates/ssh-console/src/console_logger.rscrates/ssh-console/src/frontend.rscrates/ssh-console/src/metrics.rscrates/ssh-console/src/ssh_cert_parsing.rscrates/ssh-console/src/ssh_server.rscrates/ssh-console/tests/util/ipmi_sim.rscrates/ssh-console/tests/util/mod.rscrates/state-controller/src/controller/periodic_enqueuer.rscrates/state-controller/src/controller/processor.rscrates/switch-controller/src/certificate.rscrates/switch-controller/src/configuring.rscrates/switch-controller/src/created.rscrates/switch-controller/src/deleting.rscrates/switch-controller/src/error_state.rscrates/switch-controller/src/initializing.rscrates/switch-controller/src/ready.rscrates/switch-controller/src/validating.rs
💤 Files with no reviewable changes (1)
- crates/health/src/discovery/context.rs
Comment on lines
+325
to
+330
| tracing::warn!(stdout = stdout.as_str(), "check_ifreload"); | ||
| failed(hr, probe_ids::Ifreload.clone(), None, stdout); | ||
| } | ||
| } | ||
| Err(err) => { | ||
| tracing::warn!("check_ifreload: {err}"); | ||
| tracing::warn!(error = %err, "check_ifreload"); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="crates/agent/src/health.rs"
echo "== line count =="
wc -l "$file"
echo
echo "== relevant sections =="
sed -n '300,420p' "$file" | cat -n
echo
echo "== tracing message search =="
rg -n 'tracing::(warn|info|error|debug|trace)!' "$file"Repository: NVIDIA/infra-controller
Length of output: 5903
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="crates/agent/src/health.rs"
echo "== 346-402 =="
sed -n '346,402p' "$file" | cat -n
echo
echo "== 586-650 =="
sed -n '586,650p' "$file" | cat -nRepository: NVIDIA/infra-controller
Length of output: 4795
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="crates/agent/src/health.rs"
echo "== 368-398 =="
sed -n '368,398p' "$file" | cat -n
echo
echo "== 672-686 =="
sed -n '672,686p' "$file" | cat -nRepository: NVIDIA/infra-controller
Length of output: 1850
Replace the remaining internal tracing labels with descriptive messages.
check_ifreload and check_files still log "check_ifreload" / "check_files" and "check_files: Too small". Use stable operator-facing text like “ifreload syntax check failed” and “file is too small” while keeping the structured fields.
🤖 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/agent/src/health.rs` around lines 325 - 330, Update the tracing
messages in the check_ifreload and check_files paths to use stable, descriptive
operator-facing text: replace the check_ifreload labels with “ifreload syntax
check failed” and replace “check_files: Too small” with “file is too small,”
preserving all existing structured fields.
Sources: Coding guidelines, Path instructions
Comment on lines
+59
to
+61
| command = %super::pretty_cmd(cmd.as_std()), | ||
| stderr = %String::from_utf8_lossy(&out.stderr), | ||
| "MTU query command failed" |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep child-process stderr in the rendered log payload.
Both failure paths put stderr only in a structured field, changing the operator-visible multiline rendering. Keep command structured, but append the raw stderr to the message payload.
As per path instructions, child-process output must remain an operator-visible rendered payload; this is the same concern raised in the previous review comments.
Proposed adjustment
tracing::debug!(
command = %super::pretty_cmd(cmd.as_std()),
- stderr = %String::from_utf8_lossy(&out.stderr),
- "MTU query command failed"
+ "MTU query command failed: {}",
+ String::from_utf8_lossy(&out.stderr),
);Apply the equivalent change to the MTU update path.
Also applies to: 79-81
🤖 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/agent/src/mtu.rs` around lines 59 - 61, Update both MTU command
failure paths in the query and update flows to append the raw child-process
stderr to the rendered log message payload, while keeping command as a
structured field. Do not rely on the structured stderr field alone; preserve the
existing operator-visible multiline output for both failures.
Source: Path instructions
Comment on lines
47
to
+51
| tracing::error!( | ||
| " STDOUT {cmd_str}: {}", | ||
| String::from_utf8_lossy(&out.stdout) | ||
| ); | ||
| tracing::error!( | ||
| " STDERR {cmd_str}: {}", | ||
| String::from_utf8_lossy(&out.stderr) | ||
| command = cmd_str.as_str(), | ||
| stdout = %String::from_utf8_lossy(&out.stdout), | ||
| stderr = %String::from_utf8_lossy(&out.stderr), | ||
| "OVS command failed" |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve OVS child-process output as rendered payload.
Lines 49-50 move raw stdout and stderr into structured fields, changing multiline and escaping behavior for output operators use to diagnose failed ovs-vsctl commands. Keep command structured, but retain the previous rendered-message treatment for the process streams.
Proposed direction
tracing::error!(
command = cmd_str.as_str(),
- stdout = %String::from_utf8_lossy(&out.stdout),
- stderr = %String::from_utf8_lossy(&out.stderr),
- "OVS command failed"
+ "OVS command failed: stdout={:?}, stderr={:?}",
+ String::from_utf8_lossy(&out.stdout),
+ String::from_utf8_lossy(&out.stderr),
);This follows the PR objective and prior review contract that child-process streams remain rendered when they are the diagnostic payload.
🤖 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/agent/src/ovs.rs` around lines 47 - 51, Update the OVS command failure
logging around the existing tracing::error! call to keep command structured
while rendering stdout and stderr as part of the message payload, preserving
their multiline and escaping behavior. Do not emit the process streams as
structured fields; retain the existing "OVS command failed" context and use the
output values directly in the rendered log message.
Comment on lines
+265
to
+267
| astra_status_observation = ?obs, | ||
| error = ?e, | ||
| "Failed to parse Astra DPA acknowledgment config version", |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '220,320p' crates/api-core/src/handlers/astra.rs
printf '\n---\n'
rg -n "Failed to parse Astra DPA acknowledgment config version|error = \\?e|error = %e|impl Display for|enum .*Error|struct .*Error" crates/api-core/src -g '!target'Repository: NVIDIA/infra-controller
Length of output: 12926
🏁 Script executed:
sed -n '250,305p' crates/api-core/src/handlers/astra.rs
printf '\n---\n'
rg -n "type .*ConfigVersion|struct ConfigVersion|enum ConfigVersion|impl FromStr for ConfigVersion|impl Display for ConfigVersion|from_str\\(" crates/api-core/src -g '!target'Repository: NVIDIA/infra-controller
Length of output: 25526
🏁 Script executed:
rg -n "pub async fn find_by|async fn find_by|fn find_by" crates/api-core/src crates -g '!target'
printf '\n---\n'
rg -n "ConfigVersion" crates/api-core/src crates -g '!target'Repository: NVIDIA/infra-controller
Length of output: 50379
🏁 Script executed:
sed -n '235,310p' crates/config-version/src/lib.rs
printf '\n---\n'
rg -n "enum DatabaseError|impl Display for DatabaseError|thiserror::Error.*DatabaseError|pub enum DatabaseError" crates/api-db/src crates -g '!target'Repository: NVIDIA/infra-controller
Length of output: 2776
🏁 Script executed:
sed -n '235,310p' crates/config-version/src/lib.rs
printf '\n---\n'
rg -n "enum DatabaseError|impl Display for DatabaseError|thiserror::Error.*DatabaseError|pub enum DatabaseError|type DatabaseResult" crates/api-db/src crates -g '!target'Repository: NVIDIA/infra-controller
Length of output: 2944
🏁 Script executed:
sed -n '325,436p' crates/api-db/src/lib.rs
printf '\n---\n'
sed -n '1,120p' crates/api-core/src/handlers/astra.rsRepository: NVIDIA/infra-controller
Length of output: 9491
Use Display formatting for these error fields
Both ConfigVersionParseError and DatabaseError implement Display, so error = %e is preferable to error = ?e in both log sites. It keeps the output cleaner and easier to search.
🤖 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/api-core/src/handlers/astra.rs` around lines 265 - 267, Update the
error fields in both Astra acknowledgment-config parsing log sites to use
Display formatting (`%e`) instead of Debug formatting (`?e`). Apply this to the
`ConfigVersionParseError` and `DatabaseError` log entries while preserving their
existing messages and fields.
Source: Coding guidelines
Comment on lines
+518
to
+520
| interface_count = interfaces.len(), | ||
| mac_address = %mac, | ||
| "Found machine interface entries; should be impossible", |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use one semantic field schema for all by_mac logs.
The new branch emits mac_address, while the existing machine-interface error path at Line 524 still emits mac and err. Standardize those fields (for example, mac_address = %mac and error = %err) so searches do not split across incidental local variable names.
As per coding guidelines, tracing fields should use consistent semantic names rather than incidental local variable names.
🤖 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/api-core/src/handlers/finder.rs` around lines 518 - 520, Standardize
the tracing fields in the by_mac logging paths, updating the existing
machine-interface error log near the interface_count entry to use mac_address
for the MAC value and error for the error value, matching the new branch.
Preserve the surrounding messages and behavior.
Source: Coding guidelines
Comment on lines
+9336
to
+9337
| address = %endpoint.address, | ||
| "Marking completion of Redfish task of firmware upgrade with missing task" |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Missing machine_id on the "missing task" completion branch.
This 404-recovery branch of host_waiting_fw logs only address, while its sibling branch a few lines below (Line 9354) — handling the exact same scenario but with a longer wait — carries %machine_id. The machine_id parameter is already in scope here.
🔧 Proposed fix
tracing::info!(
+ %machine_id,
address = %endpoint.address,
"Marking completion of Redfish task of firmware upgrade with missing task"
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| address = %endpoint.address, | |
| "Marking completion of Redfish task of firmware upgrade with missing task" | |
| tracing::info!( | |
| %machine_id, | |
| address = %endpoint.address, | |
| "Marking completion of Redfish task of firmware upgrade with missing task" | |
| ); |
🤖 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/machine-controller/src/handler.rs` around lines 9336 - 9337, Update
the 404-recovery log in host_waiting_fw for the missing-task completion branch
to include the in-scope machine_id field alongside endpoint.address, matching
the sibling longer-wait branch’s structured logging.
Comment on lines
+9645
to
+9654
| address = %endpoint.address, | ||
| %final_version, | ||
| current_versions = ?current_versions, | ||
| "Not all firmware versions match" | ||
| ); | ||
| } | ||
|
|
||
| if versions_match_final_version { | ||
| // Done waiting, go back to overall checking of version`2s | ||
| tracing::debug!("Done waiting for {machine_id} to reach version"); | ||
| tracing::debug!(%machine_id, "Done waiting to reach version"); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
host_new_firmware_reported_wait inconsistently drops machine_id on two branches.
Every other log in this function (Lines 9614, 9632, 9654, 9671, 9707) carries %machine_id, but the "Not all firmware versions match" warning (Line 9645) and the "resetting again" info log (Line 9690) only carry address/firmware_type. The machine_id parameter is available throughout the function.
🔧 Proposed fix
if !versions_match_final_version {
tracing::warn!(
+ %machine_id,
address = %endpoint.address,
%final_version,
current_versions = ?current_versions,
"Not all firmware versions match"
);
}
...
tracing::info!(
+ %machine_id,
address = %endpoint.address,
firmware_type = ?firmware_type,
"Upgrade has taken more than 30 minutes to report new version; resetting again."
);Also applies to: 9690-9692
🤖 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/machine-controller/src/handler.rs` around lines 9645 - 9654, Update
the logging statements in host_new_firmware_reported_wait for the “Not all
firmware versions match” warning and the “resetting again” info branch to
include the available machine_id field, matching the other logs in the function;
leave the existing address and firmware details unchanged.
| .is_none_or(|em| em.data.sku_id.as_ref().is_none_or(|id| id != sku_id)) | ||
| { | ||
| tracing::debug!("No expected machine for bmc {}", bmc_mac_address); | ||
| tracing::debug!(%bmc_mac_address, "No expected machine for bmc"); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Missing machine_id on the "No expected machine for bmc" log.
Every other log in generate_missing_sku_for_machine (Lines 107, 115, and the unchanged debug/error logs further below) carries machine_id; this one only carries bmc_mac_address, weakening per-host correlation.
🔧 Proposed fix
- tracing::debug!(%bmc_mac_address, "No expected machine for bmc");
+ tracing::debug!(machine_id = %mh_snapshot.host_snapshot.id, %bmc_mac_address, "No expected machine for bmc");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| tracing::debug!(%bmc_mac_address, "No expected machine for bmc"); | |
| tracing::debug!(machine_id = %mh_snapshot.host_snapshot.id, %bmc_mac_address, "No expected machine for bmc"); |
🤖 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/machine-controller/src/handler/sku.rs` at line 129, Update the “No
expected machine for bmc” debug log in generate_missing_sku_for_machine to
include the available machine_id field alongside bmc_mac_address, preserving the
existing message and log level.
Comment on lines
+365
to
+373
| Ok(result) => info!( | ||
| stdout = %result.stdout, | ||
| "Pulled machine validation container image", | ||
| ), | ||
| Err(e) => error!( | ||
| image_name = %image_name, | ||
| error = %e, | ||
| "Failed to pull machine validation container image", | ||
| ), |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Success/failure branches log asymmetric identifying fields.
In pull_container (L365-373), the success branch logs only stdout, while the failure branch adds image_name; in run (L655-662), the success branch logs test_name, while the failure branch drops it. In both cases, the field useful for correlating the event to a specific image/test is missing on exactly one side of the match, making it harder to trace which image/test succeeded or failed without extra context.
🔧 Proposed fix
Ok(result) => info!(
+ image_name = %image_name,
stdout = %result.stdout,
"Pulled machine validation container image",
),
...
Err(e) => error!(
+ test_name = %test.name,
error = %e,
"Failed to send machine validation result to API server",
),Also applies to: 655-662
🤖 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/machine-validation/src/machine_validation.rs` around lines 365 - 373,
Update the success and failure logging in pull_container to include the same
image_name identifying field in both branches, while preserving stdout on
success and error on failure. Apply the same symmetry in run by ensuring both
branches log test_name, retaining each branch’s existing result-specific fields.
Comment on lines
3253
to
3268
| let task = if is_bfb_artifact(&artifact.local_path) { | ||
| let _ = redfish_client.enable_rshim_bmc().await.map_err(|e| { | ||
| tracing::error!("initiate_update: Failed to call enable_rshim_bmc: {e}") | ||
| }); | ||
| redfish_client | ||
| .enable_rshim_bmc() | ||
| .await | ||
| .inspect_err(|e| { | ||
| tracing::error!( | ||
| address = %endpoint_clone.address, | ||
| error = %e, | ||
| "Failed to enable RSHIM on BMC" | ||
| ) | ||
| }) | ||
| .ok(); | ||
| tracing::debug!( | ||
| "initiate_update: Using simple_update with image URI: {}", | ||
| artifact.bfb_image_uri | ||
| image_uri = artifact.bfb_image_uri.as_str(), | ||
| "Using simple update for firmware upload" | ||
| ); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Missing address correlation field on the simple-update debug log.
Every other log site inside initiate_update carries address = %endpoint_clone.address (including the immediately preceding "Failed to enable RSHIM on BMC" error two lines above), but the "Using simple update for firmware upload" debug log drops it. This is the same category of correlation gap that was already flagged and fixed twice in this exact function for the chassis-reset and multipart/file-open logs — worth closing here too.
As per coding guidelines, "Use consistent semantic field names rather than incidental local variable names" and structured fields should carry the identifying context consistently.
🔧 Proposed fix
tracing::debug!(
+ address = %endpoint_clone.address,
image_uri = artifact.bfb_image_uri.as_str(),
"Using simple update for firmware upload"
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let task = if is_bfb_artifact(&artifact.local_path) { | |
| let _ = redfish_client.enable_rshim_bmc().await.map_err(|e| { | |
| tracing::error!("initiate_update: Failed to call enable_rshim_bmc: {e}") | |
| }); | |
| redfish_client | |
| .enable_rshim_bmc() | |
| .await | |
| .inspect_err(|e| { | |
| tracing::error!( | |
| address = %endpoint_clone.address, | |
| error = %e, | |
| "Failed to enable RSHIM on BMC" | |
| ) | |
| }) | |
| .ok(); | |
| tracing::debug!( | |
| "initiate_update: Using simple_update with image URI: {}", | |
| artifact.bfb_image_uri | |
| image_uri = artifact.bfb_image_uri.as_str(), | |
| "Using simple update for firmware upload" | |
| ); | |
| tracing::debug!( | |
| address = %endpoint_clone.address, | |
| image_uri = artifact.bfb_image_uri.as_str(), | |
| "Using simple update for firmware upload" | |
| ); |
🤖 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/preingestion-manager/src/lib.rs` around lines 3253 - 3268, Update the
“Using simple update for firmware upload” debug log in the BFB artifact branch
of initiate_update to include the structured address field using
endpoint_clone.address, matching the correlation fields on the surrounding logs.
Source: Coding guidelines
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
1 participant
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
Verification
cargo make format-nightlycargo make clippycargo make carbide-lintsThis supports #3476