crates/api-core/src/handlers/credential.rs (1)

623-628: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Anchor this TODO to a concrete tracking item.

Please include an issue/REQ reference in this TODO so cleanup of device_credential_rotation cannot be silently forgotten when the rotation-engine MR sequencing changes.

🤖 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/credential.rs` around lines 623 - 628, The TODO
in credential rotation cleanup is too vague and can be missed later; update the
comment in credential.rs near the device_credential_rotation/Vault secret
removal logic to include a concrete issue or REQ reference. Anchor it to the
relevant tracking item tied to the rotation-engine MR so the planned deletion of
the matching device_credential_rotation row is explicitly traceable and not
forgotten if sequencing changes.

crates/secrets/src/credentials.rs (1)

1120-1122: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert the exact prefix set, not just the count.

Line 1122 can still pass if CredentialPrefix::all() contains a duplicate and omits a real variant. Pinning the expected variants makes this test match its “contains every variant” intent.

Proposed test strengthening

 fn prefix_all_is_complete() {
     let all = CredentialPrefix::all();
-    assert_eq!(all.len(), 16);
+    let expected = [
+        CredentialPrefix::DpuSsh,
+        CredentialPrefix::DpuHbn,
+        CredentialPrefix::DpuRedfish,
+        CredentialPrefix::Bgp,
+        CredentialPrefix::HostRedfish,
+        CredentialPrefix::UfmAuth,
+        CredentialPrefix::DpuUefi,
+        CredentialPrefix::HostUefi,
+        CredentialPrefix::BmcCredentials,
+        CredentialPrefix::NicLockdownIkm,
+        CredentialPrefix::ExtensionService,
+        CredentialPrefix::NmxM,
+        CredentialPrefix::SwitchNvosAdmin,
+        CredentialPrefix::MqttAuth,
+        CredentialPrefix::MachineIdentityEncryptionKey,
+        CredentialPrefix::RackMaintenanceAccessToken,
+    ];
+
+    assert_eq!(all.len(), expected.len());
+    for prefix in expected {
+        assert!(
+            all.contains(&prefix),
+            "{prefix:?} missing from CredentialPrefix::all()"
+        );
+    }
 }

🤖 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/secrets/src/credentials.rs` around lines 1120 - 1122, The prefix
coverage test in prefix_all_is_complete only checks the length of
CredentialPrefix::all(), so it can miss duplicates or omissions; update the test
to assert the exact set of expected CredentialPrefix variants returned by
CredentialPrefix::all() using the existing enum symbols, so the test verifies
completeness rather than just count.

crates/api-core/src/handlers/uefi.rs (1)

262-262: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use structured fields for this error log.

Prefer tracing::error!(error=%e, "Failed to update bios_password_set_time"); so the log remains searchable as logfmt. As per coding guidelines, “When writing log messages, prefer placing common fields as attributes passed to tracing functions.”

🤖 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/uefi.rs` at line 262, The `tracing::error!` call
in the `uefi` handler is using string interpolation instead of structured
logging. Update the error log at the `bios_password_set_time` failure site to
pass the error as a field (for example, an `error` attribute) and keep the
message text separate, so the log stays searchable and follows the tracing
logging guidelines.

Source: Coding guidelines

crates/switch-controller/src/configuring.rs (2)

90-111: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Make the disabled REQ-6 branch fail closed instead of recording convergence.

Line 97 can persist nvos convergence while the required switch rotation/Vault storage is still only a TODO. Also, line 91 opens the DB transaction before the future device/Vault work described at lines 93-96, which sets up a transaction-across-external-I/O pattern.

Suggested fail-closed shape until REQ-6 lands

     if update_device_password {
-        let mut txn = ctx.services.db_pool.begin().await?;
-
         // TODO(REQ-6): rotate the NVOS password on the switch here (factory ->
         // NICo-owned) and store the resulting credential in Vault before recording
         // convergence below. Only once the password is actually changed on the
         // device has the switch converged to the current nvos target.
-        db::credential_rotation::record_device_converged(
-            &mut txn,
-            bmc_mac_address,
-            db::credential_rotation::CredentialRotationType::Nvos,
-        )
-        .await
-        .map_err(|e| {
-            StateHandlerError::GenericError(eyre::eyre!(
-                "Switch {:?}: failed to record nvos rotation convergence: {}",
-                switch_id,
-                e
-            ))
-        })?;
-
-        Ok(outcome.with_txn(txn))
+        Err(StateHandlerError::GenericError(eyre::eyre!(
+            "REQ-6 NVOS password rotation is not implemented"
+        )))
     } else {

When REQ-6 is implemented, perform switch/Vault I/O first, then open the DB transaction only for record_device_converged plus the state transition. As per coding guidelines, “Remove unused code, temporary logging, skipped assertions, placeholders, and hidden TODOs before asking for review” and “Do not hold a transaction open while doing long-running work.” As per path instructions, controller logic should be reviewed for state-machine transitions and safe recovery from partial failures.

🤖 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/switch-controller/src/configuring.rs` around lines 90 - 111, The
disabled REQ-6 branch in configuring.rs currently records nvos convergence even
though the required switch password rotation and Vault write are still TODO, so
it should fail closed instead of persisting success. Update the
update_device_password path in configuring logic to return an error or otherwise
stop before calling db::credential_rotation::record_device_converged until REQ-6
is fully implemented, and avoid opening ctx.services.db_pool.begin() until after
any future switch/Vault I/O is complete so no transaction spans external work.
Use the existing StateHandlerError and record_device_converged flow to locate
the branch and keep the state transition only in the completed, implemented
path.

Sources: Coding guidelines, Path instructions

---

138-142: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use structured tracing fields for switch/NVOS logs.

These service logs should expose switch_id and bmc_mac_address as fields instead of embedding them in the message.

Suggested logfmt-friendly rewrite

                 tracing::info!(
-                    "Switch {:?}: no NVOS credentials in vault or expected switch for BMC MAC {}, skipping",
-                    switch_id,
-                    bmc_mac_address
+                    ?switch_id,
+                    %bmc_mac_address,
+                    "no NVOS credentials in vault or expected switch; skipping"
                 );
@@
         tracing::info!(
-            "Switch {:?}: stored NVOS admin credentials from expected switch into vault for BMC MAC {}",
-            switch_id,
-            bmc_mac_address
+            ?switch_id,
+            %bmc_mac_address,
+            "stored NVOS admin credentials from expected switch into vault"
         );

As per coding guidelines, “All services should emit logs in 'logfmt' syntax” and common fields should be passed as tracing attributes instead of string interpolation.

Also applies to: 161-165

🤖 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/switch-controller/src/configuring.rs` around lines 138 - 142, The
tracing logs in the NVOS credential lookup flow are embedding switch identifiers
in the message instead of emitting structured fields. Update the
`tracing::info!` calls in `configuring.rs`—including the one in the
`switch_id`/`bmc_mac_address` skip path and the related log at the other
referenced location—to pass `switch_id` and `bmc_mac_address` as tracing
attributes, while keeping the message text generic and logfmt-friendly.

Source: Coding guidelines

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

crates/secrets/src/credentials.rs (1)

1122-1122: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert the exact prefix set instead of only its length.

This test claims CredentialPrefix::all() is complete, but a length-only assertion can still pass if one variant is dropped and another is added. Pin the exact list so Vault prefix drift is caught by name.

Suggested test tightening

         let all = CredentialPrefix::all();
-        assert_eq!(all.len(), 16);
+        assert_eq!(
+            all.as_ref(),
+            &[
+                CredentialPrefix::DpuSsh,
+                CredentialPrefix::DpuHbn,
+                CredentialPrefix::DpuRedfish,
+                CredentialPrefix::Bgp,
+                CredentialPrefix::HostRedfish,
+                CredentialPrefix::UfmAuth,
+                CredentialPrefix::DpuUefi,
+                CredentialPrefix::HostUefi,
+                CredentialPrefix::BmcCredentials,
+                CredentialPrefix::NicLockdownIkm,
+                CredentialPrefix::ExtensionService,
+                CredentialPrefix::NmxM,
+                CredentialPrefix::SwitchNvosAdmin,
+                CredentialPrefix::MqttAuth,
+                CredentialPrefix::MachineIdentityEncryptionKey,
+                CredentialPrefix::RackMaintenanceAccessToken,
+            ],
+        );

As per coding guidelines, “Verification should exercise the behavior that changed.”

🤖 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/secrets/src/credentials.rs` at line 1122, The test around
CredentialPrefix::all() is too weak because it only checks the number of
prefixes, so update the assertion to verify the exact set of returned variants
by name. Modify the test that currently uses all.len() to compare against the
full expected list from CredentialPrefix::all(), ensuring any dropped or added
prefix is caught explicitly rather than just matching the count.

Source: Coding guidelines

crates/switch-controller/src/configuring.rs (2)

138-165: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use structured tracing fields for switch identifiers.

These changed logs interpolate switch_id and bmc_mac_address into the message, which makes logfmt filtering harder. Put them in tracing fields instead.

As per coding guidelines, “When writing log messages, prefer placing common fields as attributes passed to tracing functions instead of using string interpolation.”

Proposed fix

                 tracing::info!(
-                    "Switch {:?}: no NVOS credentials in vault or expected switch for BMC MAC {}, skipping",
-                    switch_id,
-                    bmc_mac_address
+                    ?switch_id,
+                    %bmc_mac_address,
+                    "no NVOS credentials in vault or expected switch; skipping"
                 );
@@
         tracing::info!(
-            "Switch {:?}: stored NVOS admin credentials from expected switch into vault for BMC MAC {}",
-            switch_id,
-            bmc_mac_address
+            ?switch_id,
+            %bmc_mac_address,
+            "stored NVOS admin credentials from expected switch into vault"
         );

🤖 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/switch-controller/src/configuring.rs` around lines 138 - 165, The
`tracing::info!` and error mapping in `configuring.rs` are interpolating
`switch_id` and `bmc_mac_address` into message strings instead of emitting them
as structured fields. Update the logging around the credential storage flow in
this block so the `tracing` calls for the NVOS credential skip/store path and
the `set_credentials` error context attach `switch_id` and `bmc_mac_address` as
fields, while keeping the human-readable message text separate.

Source: Coding guidelines

---

90-98: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Open the DB transaction only after future device/Vault work succeeds.

Line 91 starts a transaction before the REQ-6 TODO. If that TODO is implemented in place, the handler will hold a DB transaction across switch I/O and Vault writes.

As per coding guidelines, “Do not hold a transaction open while doing long-running work.”

Proposed fix

     let update_device_password = false;
     if update_device_password {
-        let mut txn = ctx.services.db_pool.begin().await?;
-
         // TODO(REQ-6): rotate the NVOS password on the switch here (factory ->
         // NICo-owned) and store the resulting credential in Vault before recording
         // convergence below. Only once the password is actually changed on the
         // device has the switch converged to the current nvos target.
+        let mut txn = ctx.services.db_pool.begin().await?;
         db::credential_rotation::record_device_converged(

🤖 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/switch-controller/src/configuring.rs` around lines 90 - 98, The
transaction in the update_device_password path is opened too early in
configuring::record_device_converged, which would keep a DB transaction open
during future switch and Vault work. Move the ctx.services.db_pool.begin() call
to after the REQ-6 device password rotation and Vault credential write succeed,
then use that transaction only for the final
db::credential_rotation::record_device_converged call.

Sources: Coding guidelines, Path instructions

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

crates/dpa-manager/src/card_handler/svpc.rs (1)

701-759: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use table-driven coverage for these convergence cases.

These two tests exercise the same operation with different setup and expectations. Please collapse them into a table-driven test using the repository test helpers so staged, fallback, and future edge cases remain one row per contract. As per coding guidelines, “Use table-driven test style when writing tests in Rust” and prefer carbide-test-support scenarios/check helpers.

🤖 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/dpa-manager/src/card_handler/svpc.rs` around lines 701 - 759, The two
`#[crate::sqlx_test]` cases for `record_lock_convergence` duplicate the same
flow with only setup and expected outcome changing, so collapse them into one
table-driven test. Use the repository test helpers from `carbide-test-support`
and structure rows around each contract (staged promotion, fallback to sitewide
target, and future edge cases) so the assertions for `lockdown_version_of` and
`lockdown_rotating_version_of` stay data-driven and easy to extend.

Sources: Coding guidelines, Path instructions

crates/switch-controller/src/configuring.rs (2)

145-149: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use structured tracing fields here.

These tracing::info! calls interpolate the key identifiers into the message body, which makes logfmt search and aggregation harder. Emit switch_id and bmc_mac_address as fields instead.

Proposed change

-                tracing::info!(
-                    "Switch {:?}: no NVOS credentials in vault or expected switch for BMC MAC {}, skipping",
-                    switch_id,
-                    bmc_mac_address
-                );
+                tracing::info!(
+                    switch_id = ?switch_id,
+                    %bmc_mac_address,
+                    "no NVOS credentials in vault or expected switch; skipping"
+                );
@@
-        tracing::info!(
-            "Switch {:?}: stored NVOS admin credentials from expected switch into vault for BMC MAC {}",
-            switch_id,
-            bmc_mac_address
-        );
+        tracing::info!(
+            switch_id = ?switch_id,
+            %bmc_mac_address,
+            "stored NVOS admin credentials from expected switch into vault"
+        );

As per coding guidelines, "All services should emit logs in 'logfmt' syntax" and "prefer placing common fields as attributes passed to tracing function, instead of using string interpolation."

Also applies to: 168-172

🤖 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/switch-controller/src/configuring.rs` around lines 145 - 149, The
tracing logs in `configuring.rs` are interpolating `switch_id` and
`bmc_mac_address` into the message body instead of emitting them as structured
fields. Update the affected `tracing::info!` calls in the relevant branches of
`configuring.rs` to pass `switch_id` and `bmc_mac_address` as tracing attributes
alongside a stable message string, using the existing log sites around the NVOS
credential skip paths.

Source: Coding guidelines

---

119-174: 📐 Maintainability & Code Quality | 🔵 Trivial

Add focused tests for the NVOS credential backfill path.
A small table-driven test should cover the three outcomes here: missing expected-switch row, missing NVOS credentials on the expected switch, and successful Vault write. That will lock in the branch behavior and make future changes safer.

🤖 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/switch-controller/src/configuring.rs` around lines 119 - 174, The NVOS
credential backfill branch in the configuring state needs focused test coverage
for all outcomes. Add a table-driven test around the code path in configuring.rs
that exercises the expected-switch lookup and credential write flow in the same
branch that uses find_by_bmc_mac_address, Credentials::UsernamePassword, and
credential_manager.set_credentials. Cover three cases: no expected switch row
returns the Error transition, missing nvos_username or nvos_password logs and
returns the existing outcome, and a successful write stores credentials in Vault
and returns Ok(outcome). Use the existing state handler entrypoint and mock
db/services so each branch is asserted independently.

Sources: Coding guidelines, Path instructions

crates/secrets/src/credentials.rs (2)

1120-1122: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Make the prefix completeness test assert the actual variants.

len() == 16 still passes if one prefix is duplicated and another is omitted. Compare CredentialPrefix::all() to the expected slice so this test proves the contract it documents.

Proposed stronger assertion

     #[test]
     fn prefix_all_is_complete() {
         let all = CredentialPrefix::all();
-        assert_eq!(all.len(), 16);
+        assert_eq!(
+            all,
+            &[
+                CredentialPrefix::DpuSsh,
+                CredentialPrefix::DpuHbn,
+                CredentialPrefix::DpuRedfish,
+                CredentialPrefix::Bgp,
+                CredentialPrefix::HostRedfish,
+                CredentialPrefix::UfmAuth,
+                CredentialPrefix::DpuUefi,
+                CredentialPrefix::HostUefi,
+                CredentialPrefix::BmcCredentials,
+                CredentialPrefix::NicLockdownIkm,
+                CredentialPrefix::ExtensionService,
+                CredentialPrefix::NmxM,
+                CredentialPrefix::SwitchNvosAdmin,
+                CredentialPrefix::MqttAuth,
+                CredentialPrefix::MachineIdentityEncryptionKey,
+                CredentialPrefix::RackMaintenanceAccessToken,
+            ]
+        );
     }

As per coding guidelines, prefer table-driven or explicit case coverage for Rust 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/secrets/src/credentials.rs` around lines 1120 - 1122, The prefix
completeness test in CredentialPrefix::all is too weak because it only checks
the count, so update prefix_all_is_complete to assert the actual returned
variants against the expected full set of CredentialPrefix values instead of
just len(). Use the CredentialPrefix::all() helper and compare it to an explicit
expected slice or table-driven case list so the test fails if any variant is
duplicated, missing, or reordered unexpectedly.

Source: Coding guidelines

---

663-677: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Pin the unversioned UEFI aliases this test claims are unchanged.

The comment says the unversioned alias remains unchanged, but this test only asserts the new versioned paths. Add exact checks for HostUefi { SiteDefault } and DpuUefi { SiteDefault } so future path drift is caught.

Proposed test hardening

         assert_eq!(host_uefi.prefix(), CredentialPrefix::HostUefi);
+
+        let host_uefi_alias = CredentialKey::HostUefi {
+            credential_type: CredentialType::SiteDefault,
+        };
+        assert_eq!(
+            host_uefi_alias.to_key_str(),
+            "machines/all_hosts/site_default/uefi-metadata-items/auth"
+        );
 
         let dpu_uefi = CredentialKey::DpuUefiSiteVersioned { version: 7 };
         assert_eq!(
             dpu_uefi.to_key_str(),
             "machines/all_dpus/site_default/uefi-metadata-items/auth/v7"
         );
         assert_eq!(dpu_uefi.prefix(), CredentialPrefix::DpuUefi);
+
+        let dpu_uefi_alias = CredentialKey::DpuUefi {
+            credential_type: CredentialType::SiteDefault,
+        };
+        assert_eq!(
+            dpu_uefi_alias.to_key_str(),
+            "machines/all_dpus/site_default/uefi-metadata-items/auth"
+        );

As per coding guidelines, “Verification should exercise the behavior that changed.”

🤖 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/secrets/src/credentials.rs` around lines 663 - 677, The test for
CredentialKey UEFI rotation only verifies the new versioned variants, so it does
not actually pin the unchanged unversioned aliases described in the comment. In
the credentials.rs test case, add explicit assertions for
CredentialKey::HostUefi { SiteDefault } and CredentialKey::DpuUefi { SiteDefault
} to check their exact to_key_str and prefix values, alongside the existing
HostUefiSiteVersioned and DpuUefiSiteVersioned checks, so future path drift is
caught.

Source: Coding guidelines