crates/api-core/src/tests/instance.rs (1)

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

Consolidate the triplicated fixture_tenant_config() into a shared fixture. The identical helper now lives in three test modules; any future change to the fixture tenant contract must be applied in lockstep or the modules silently diverge. Extract a single definition into the existing crates/api-core/src/tests/common/api_fixtures module (alongside create_fixture_tenant) and import it in each site.

• crates/api-core/src/tests/instance.rs#L110-L115: remove the local definition and import the shared helper.
• crates/api-core/src/tests/instance_config_update.rs#L46-L51: remove the local definition and import the shared helper.
• crates/api-core/src/tests/vpc_prefix.rs#L41-L46: remove the local definition and import the shared helper.

🤖 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/instance.rs` around lines 110 - 115, Consolidate
the duplicated fixture_tenant_config helper by adding one shared definition to
the existing api_fixtures module alongside create_fixture_tenant, then remove
the local definitions and import the shared helper in
crates/api-core/src/tests/instance.rs#L110-L115,
crates/api-core/src/tests/instance_config_update.rs#L46-L51, and
crates/api-core/src/tests/vpc_prefix.rs#L41-L46.

crates/machine-controller/src/handler.rs (1)

6437-6443: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider centralising the generated-segment projection. The identical expression interfaces.iter().filter_map(InstanceInterfaceConfig::generated_network_segment_id).collect_vec() now appears verbatim at three readiness/cleanup sites. Extracting a single helper (e.g. InstanceNetworkConfig::generated_network_segment_ids() or a free function over &[InstanceInterfaceConfig]) would keep the "which segments are generated VPC-prefix segments" contract in one place and guard against future divergence between readiness gating and cleanup.

• crates/machine-controller/src/handler.rs#L6437-L6443: replace the inline filter_map(...).collect_vec() in InstanceState::WaitingForNetworkSegmentToBeReady with the shared helper.
• crates/machine-controller/src/handler.rs#L7567-L7572: replace the identical expression in NetworkConfigUpdateState::WaitingForNetworkSegmentToBeReady with the same helper.
• crates/machine-controller/src/handler.rs#L7929-L7932: replace the identical expression in release_network_segments_with_vpc_prefix with the same helper.

🤖 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 6437 - 6443,
Centralize the generated network-segment projection in a shared helper, such as
InstanceNetworkConfig::generated_network_segment_ids(), preserving the existing
filter_map and collection behavior. Use that helper in
crates/machine-controller/src/handler.rs:6437-6443 within
InstanceState::WaitingForNetworkSegmentToBeReady, :7567-7572 within
NetworkConfigUpdateState::WaitingForNetworkSegmentToBeReady, and :7929-7932
within release_network_segments_with_vpc_prefix, replacing each inline
expression.

crates/api-core/src/network_segment/allocate.rs (1)

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

Prefer value_scenarios! over a manual table loop for this table-driven test.

The five cases are exactly the kind of input→output mapping carbide-test-support's value_scenarios! macro is meant for, consistent with how sibling tests (e.g. validate_inbound_vpc_selection_modes in crates/rpc/src/model/instance/config/network.rs) are written.

♻️ Suggested refactor

-    #[test]
-    fn find_first_unoccupied_linknet_index() {
-        // These cases cover gaps, overlapping occupied ranges, and complete
-        // exhaustion without constructing an address space proportional to IPv6.
-        let cases = [
-            ("empty", vec![], 0, 7, Some(0)),
-            ("leading occupied", vec![(0, 3)], 0, 7, Some(4)),
-            ("overlapping occupied", vec![(0, 3), (2, 6)], 0, 7, Some(7)),
-            ("gap", vec![(0, 1), (4, 7)], 0, 7, Some(2)),
-            ("exhausted", vec![(0, 7)], 0, 7, None),
-        ];
-
-        for (name, occupied, range_start, range_end, expected) in cases {
-            assert_eq!(
-                first_unoccupied_index(&occupied, range_start, range_end),
-                expected,
-                "case: {name}",
-            );
-        }
-    }
+    #[test]
+    fn find_first_unoccupied_linknet_index() {
+        value_scenarios!(
+            run = |(occupied, range_start, range_end): (Vec<(u128, u128)>, u128, u128)|
+                first_unoccupied_index(&occupied, range_start, range_end);
+            "empty" {
+                (vec![], 0, 7) => Some(0),
+            }
+            "leading occupied" {
+                (vec![(0, 3)], 0, 7) => Some(4),
+            }
+            "overlapping occupied" {
+                (vec![(0, 3), (2, 6)], 0, 7) => Some(7),
+            }
+            "gap" {
+                (vec![(0, 1), (4, 7)], 0, 7) => Some(2),
+            }
+            "exhausted" {
+                (vec![(0, 7)], 0, 7) => None,
+            }
+        );
+    }

As per coding guidelines, "In Rust tests, prefer carbide-test-support's scenarios!, value_scenarios!, check_cases, and check_values appropriately."

🤖 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/network_segment/allocate.rs` around lines 470 - 492,
Replace the manual cases array and loop in find_first_unoccupied_linknet_index
with carbide-test-support’s value_scenarios! macro, expressing each
occupied-range and boundary input alongside its expected first_unoccupied_index
result. Preserve all five scenarios and their case labels, including the
exhaustion case.

Source: Coding guidelines

crates/api-model/src/instance/status/network.rs (1)

1034-1070: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Make the resolved-prefix status test table-driven.

The new test covers only DualStack, while resolved_vpc_prefixes() has distinct IPv4-only, IPv6-only, and DualStack mappings. Parameterize this test over the supported modes so a family-key regression cannot pass unnoticed. As per coding guidelines, Rust tests for input-to-output or observable-result mappings should be table-driven.

🤖 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-model/src/instance/status/network.rs` around lines 1034 - 1070,
Convert network_status_without_observations_includes_resolved_prefixes into a
table-driven test covering IPv4-only, IPv6-only, and DualStack modes. For each
mode, configure the corresponding interface family settings and assert the
expected resolved_vpc_prefixes mapping, while preserving the empty addresses
assertion and shared status construction.

Source: Coding guidelines

crates/admin-cli/src/instance/show/cmd.rs (1)

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

Verbose enum string and triplicated match; extract the VPC selection once.

selection.family_mode().as_str_name() renders the raw proto constant (e.g. INSTANCE_INTERFACE_IP_FAMILY_MODE_IPV4_ONLY), which is far more verbose than the {:?} Debug-based short names (Virtual, Synced, ...) used for every other enum field in this same block — an inconsistent, harder-to-read CLI output. Additionally, if_config.and_then(|c| c.network_details.as_ref()) is matched three separate times (VPC PREFIX ID, REQUESTED VPC ID, REQUESTED IP FAMILY) for the same underlying value.

♻️ Proposed fix: match once, use Debug formatting for the enum

+                let vpc_selection = if_config
+                    .and_then(|c| c.network_details.as_ref())
+                    .and_then(|d| match d {
+                        forgerpc::instance_interface_config::NetworkDetails::Vpc(selection) => {
+                            Some(selection)
+                        }
+                        _ => None,
+                    });
                 let data: &[(&str, Cow<str>)] = &[
                     ...
                     (
                         "REQUESTED VPC ID",
-                        match if_config.and_then(|c| c.network_details.as_ref()) {
-                            Some(forgerpc::instance_interface_config::NetworkDetails::Vpc(
-                                selection,
-                            )) => selection
-                                .vpc_id
-                                .map(|vpc_id| vpc_id.to_string().into())
-                                .unwrap_or_else(|| "NA".into()),
-                            _ => "NA".into(),
-                        },
+                        vpc_selection
+                            .and_then(|s| s.vpc_id)
+                            .map(|vpc_id| vpc_id.to_string().into())
+                            .unwrap_or_else(|| "NA".into()),
                     ),
                     (
                         "REQUESTED IP FAMILY",
-                        match if_config.and_then(|c| c.network_details.as_ref()) {
-                            Some(forgerpc::instance_interface_config::NetworkDetails::Vpc(
-                                selection,
-                            )) => selection.family_mode().as_str_name().into(),
-                            _ => "NA".into(),
-                        },
+                        vpc_selection
+                            .map(|s| format!("{:?}", s.family_mode()).into())
+                            .unwrap_or_else(|| "NA".into()),
                     ),

Please confirm whether as_str_name() was intentionally chosen (e.g. to match wire/JSON field naming for scripting consumers) before switching to Debug formatting — this depends on the prost-generated enum API contract.

🤖 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/admin-cli/src/instance/show/cmd.rs` around lines 258 - 292, Extract
if_config.and_then(|c| c.network_details.as_ref()) once and reuse the resulting
network details when producing VPC PREFIX ID, REQUESTED VPC ID, and REQUESTED IP
FAMILY. For the VPC selection’s family mode, use the same Debug-based short enum
formatting used by other enum fields instead of family_mode().as_str_name(),
unless this output intentionally serves a wire/JSON naming contract; preserve
the existing NA and other network-detail cases.

crates/api-db/src/vpc_prefix.rs (1)

155-183: 🚀 Performance & Scalability | 🔵 Trivial

Verify network_vpc_prefixes has index coverage for this hot allocation query.

find_allocation_candidates filters on vpc_id = ANY($1) AND deleted IS NULL plus computed family(prefix)/masklen(prefix) predicates, and now runs on every automatic VPC-prefix selection. If there's no partial index supporting vpc_id with deleted IS NULL (or a functional index on family(prefix)), this becomes a sequential scan as the table grows.

🤖 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_prefix.rs` around lines 155 - 183, Verify the
schema/migration indexes used by find_allocation_candidates cover vpc_id for
rows where deleted IS NULL, and add or update a partial index if needed. Assess
whether the family(prefix)/masklen(prefix) predicates require functional or
otherwise appropriate index coverage for this hot query, while preserving the
existing filtering and ORDER BY vpc_id, id behavior.

crates/api-core/src/network_segment/allocate.rs (1)

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

next_free_prefix's new cursor/interval logic has no direct unit test.

The rewritten allocation core (next_free_prefix) no longer calls self.iter()/PrefixIterator — that machinery is now #[cfg(test)]-only and unused in production. The pre-existing test_next_iter* tests still pass, but they now only validate the old, unused iterator, not the actual algorithm (occupied-index-interval construction, next-fit start_index computation, and the two-part search with wraparound) that ships in production. Only first_unoccupied_index is unit-tested in isolation with tiny synthetic ranges (Lines 474-492); the u128 conversion of real overlapping IpNetwork prefixes into indices, and the wraparound-past-start_index behavior with realistic prefixes, is exercised only indirectly through higher-level allocation integration tests.

Given this is the load-bearing allocation path for both IPv4 and IPv6 linknets, consider adding a table-driven test directly against next_free_prefix (or a thin wrapper) covering: gaps left by soft-deleted/overlapping prefixes, cursor wraparound at the top of the VPC prefix, and IPv6 /48-scale prefixes to confirm no enumeration happens.

As per coding guidelines, "Use table-driven tests for functions mapping inputs to outputs, errors, or observable results," and "Avoid landing unused Rust code; when code is introduced incrementally, provide its intended usage where practical."

🤖 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/network_segment/allocate.rs` around lines 303 - 394, Add
table-driven unit tests that invoke NetworkSegment::next_free_prefix directly,
rather than only testing PrefixIterator or first_unoccupied_index. Cover
overlapping and soft-deleted occupied prefixes, next-fit cursor wraparound at
the VPC boundary, and IPv6 /48-scale allocations to verify interval-based
behavior without enumeration. Reuse existing test setup/helpers and assert both
selected prefixes and expected exhaustion errors where applicable.

Source: Coding guidelines