serviceability: device instruction builders (Rust)

[juan-malbeclabs]

Part of #4014.

Add src/device.rs builders covering the full device domain (create/update/suspend/resume/delete + interface instructions). Includes the variable dz_prefix block loop and resource_count handling established in R0.

Acceptance

• One pure Instruction-returning function per non-deprecated device instruction, accounts in processor order.
• Verbatim account-layout doc-comment copied from each processor.
• Unit tests + ix_* golden fixtures for each builder.

[juan-malbeclabs]

Closing for now — this work is captured in RFC-26 (rfcs/rfc26-rust-instruction-builder-library.md). Will reopen once the RFC is approved.

[juan-malbeclabs]

Reopening: RFC-26 was merged (#4035). Reviewed this issue against the final RFC — scope and design decisions are still aligned. The implementation work tracked here is still pending.

[nikw9944]

System Prompt

Code Review

You are a Claude Code instance managed by Roadbike, performing a thorough code review.
You must NOT make any code changes — review only. Do NOT post the review to GitHub —
the operator will read your findings in the dashboard and decide what to do.

Assignment

• PR: malbeclabs/doublezero#4050
• Title: serviceability-instruction: device domain builders (RFC-26 R1)
• Repository: malbeclabs/doublezero
• Branch: feat/rfc26-r1-device-builders → main
• Linked Issue: serviceability: device instruction builders (Rust) #4016 — serviceability: device instruction builders (Rust)

PR Description

Summary

RFC-26 R1 (stacked on the previous phase's branch). Complete the device domain on top of the R0 exemplars: update_device (max(old,new) dz_prefix block), set_device_health, and interface create/delete/update (Vpnv4 topology PDAs, segment-routing reconcile).

All builders route through authorize() -> build_with_permission unless noted; each carries a verbatim account-layout doc-comment copied from its processor.

Testing Verification

• Unit tests assert the exact AccountMeta list (incl. trailing [payer, system]) and the borsh tag byte for every builder, covering the conditional/variable-account paths.

Closes #4016. Part of RFC-26 (rfcs/rfc26-rust-instruction-builder-library.md).

PR Diff

diff --git a/crates/doublezero-serviceability-instruction/src/device.rs b/crates/doublezero-serviceability-instruction/src/device.rs
index 3abeb437f..7326f2147 100644
--- a/crates/doublezero-serviceability-instruction/src/device.rs
+++ b/crates/doublezero-serviceability-instruction/src/device.rs
@@ -3,9 +3,21 @@
 use crate::common;
 use doublezero_serviceability::{
     instructions::DoubleZeroInstruction,
-    pda::{get_device_pda, get_globalconfig_pda, get_globalstate_pda, get_resource_extension_pda},
-    processors::device::{create::DeviceCreateArgs, delete::DeviceDeleteArgs},
+    pda::{
+        get_device_pda, get_globalconfig_pda, get_globalstate_pda, get_resource_extension_pda,
+        get_topology_pda,
+    },
+    processors::device::{
+        create::DeviceCreateArgs,
+        delete::DeviceDeleteArgs,
+        interface::{
+            create::DeviceInterfaceCreateArgs, DeviceInterfaceDeleteArgs, DeviceInterfaceUpdateArgs,
+        },
+        sethealth::DeviceSetHealthArgs,
+        update::DeviceUpdateArgs,
+    },
     resource::ResourceType,
+    state::interface::LoopbackType,
 };
 use solana_program::{
     instruction::{AccountMeta, Instruction},
@@ -201,6 +213,257 @@ pub fn delete_device(
     )
 }
 
+/// `UpdateDevice` (variant 23).
+///
+/// Account layout (processor `next_account_info` order), before the trailing
+/// accounts appended by [`common::build_with_permission`]:
+///
+/// ```text
+/// device                (writable)
+/// current_contributor   (writable)  — device.contributor_pk
+/// current_location      (writable)  — device.location_pk
+/// new_location          (writable)  — the new location, or current if unchanged
+/// globalstate           (writable)
+/// // present only when args.dz_prefixes.is_some():
+/// globalconfig          (writable)
+/// resource[i]           (writable)  — idx 0: TunnelIds(device, 0);
+///                                      idx 1..: DzPrefixBlock(device, idx-1),
+///                                      looped 0..=max(old, new) dz_prefix count
+/// ```
+///
+/// The dz_prefix resource block covers `max(old, new)` prefixes, so the old count
+/// (RPC-read from the device) is required alongside the new count in
+/// `args.dz_prefixes`. `args.resource_count` is written from the same loop.
+#[allow(clippy::too_many_arguments)]
+pub fn update_device(
+    program_id: &Pubkey,
+    payer: &Pubkey,
+    device: &Pubkey,
+    current_contributor: &Pubkey,
+    current_location: &Pubkey,
+    new_location: &Pubkey,
+    old_dz_prefix_count: usize,
+    mut args: DeviceUpdateArgs,
+) -> Instruction {
+    let (globalstate, _) = get_globalstate_pda(program_id);
+    let mut accounts = vec![
+        AccountMeta::new(*device, false),
+        AccountMeta::new(*current_contributor, false),
+        AccountMeta::new(*current_location, false),
+        AccountMeta::new(*new_location, false),
+        AccountMeta::new(globalstate, false),
+    ];
+
+    let mut resource_count = 0usize;
+    if let Some(dz_prefixes) = args.dz_prefixes.as_ref() {
+        let (globalconfig, _) = get_globalconfig_pda(program_id);
+        accounts.push(AccountMeta::new(globalconfig, false));
+        let max_count = old_dz_prefix_count.max(dz_prefixes.len());
+        for idx in 0..=max_count {
+            let resource_type = if idx == 0 {
+                ResourceType::TunnelIds(*device, 0)
+            } else {
+                ResourceType::DzPrefixBlock(*device, idx - 1)
+            };
+            let (pda, _, _) = get_resource_extension_pda(program_id, resource_type);
+            accounts.push(AccountMeta::new(pda, false));
+        }
+        resource_count = max_count + 1;
+    }
+    args.resource_count = resource_count;
+
+    common::build_with_permission(
+        program_id,
+        DoubleZeroInstruction::UpdateDevice(args),
+        accounts,
+        payer,
+    )
+}
+
+/// `SetDeviceHealth` (variant 83).
+///
+/// Account layout, before the trailing accounts:
+///
+/// ```text
+/// device       (writable)
+/// globalstate  (writable)
+/// ```
+pub fn set_device_health(
+    program_id: &Pubkey,
+    payer: &Pubkey,
+    device: &Pubkey,
+    args: DeviceSetHealthArgs,
+) -> Instruction {
+    let (globalstate, _) = get_globalstate_pda(program_id);
+    let accounts = vec![
+        AccountMeta::new(*device, false),
+        AccountMeta::new(globalstate, false),
+    ];
+    common::build_with_permission(
+        program_id,
+        DoubleZeroInstruction::SetDeviceHealth(args),
+        accounts,
+        payer,
+    )
+}
+
+/// `CreateDeviceInterface` (variant 73).
+///
+/// Account layout, before the trailing accounts:
+///
+/// ```text
+/// device                (writable)
+/// contributor           (writable)  — device.contributor_pk
+/// globalstate           (writable)
+/// device_tunnel_block   (writable)  — ResourceType::DeviceTunnelBlock
+/// segment_routing_ids   (writable)  — ResourceType::SegmentRoutingIds
+/// topology[i]           (readonly)  — one per topology_name, ONLY for Vpnv4 loopbacks
+/// ```
+///
+/// Topology PDAs are appended (and `args.topology_count` set) only when
+/// `args.loopback_type == Vpnv4`; otherwise `topology_names` is ignored.
+pub fn create_device_interface(
+    program_id: &Pubkey,
+    payer: &Pubkey,
+    device: &Pubkey,
+    contributor: &Pubkey,
+    topology_names: &[String],
+    mut args: DeviceInterfaceCreateArgs,
+) -> Instruction {
+    let (globalstate, _) = get_globalstate_pda(program_id);
+    let (device_tunnel_block, _, _) =
+        get_resource_extension_pda(program_id, ResourceType::DeviceTunnelBlock);
+    let (segment_routing_ids, _, _) =
+        get_resource_extension_pda(program_id, ResourceType::SegmentRoutingIds);
+    let mut accounts = vec![
+        AccountMeta::new(*device, false),
+        AccountMeta::new(*contributor, false),
+        AccountMeta::new(globalstate, false),
+        AccountMeta::new(device_tunnel_block, false),
+        AccountMeta::new(segment_routing_ids, false),
+    ];
+
+    let is_vpnv4 = args.loopback_type == LoopbackType::Vpnv4;
+    if is_vpnv4 {
+        for name in topology_names {
+            let (topology, _) = get_topology_pda(program_id, name);
+            accounts.push(AccountMeta::new_readonly(topology, false));
+        }
+    }
+    let topology_count = if is_vpnv4 { topology_names.len() } else { 0 };
+    debug_assert!(
+        topology_count <= u8::MAX as usize,
+        "device interface topology_count {topology_count} exceeds u8::MAX"
+    );
+    args.topology_count = u8::try_from(topology_count).unwrap_or(u8::MAX);
+
+    common::build_with_permission(
+        program_id,
+        DoubleZeroInstruction::CreateDeviceInterface(args),
+        accounts,
+        payer,
+    )
+}
+
+/// `DeleteDeviceInterface` (variant 74).
+///
+/// Account layout, before the trailing accounts:
+///
+/// ```text
+/// device                (writable)
+/// contributor           (writable)  — device.contributor_pk
+/// globalstate           (writable)
+/// device_tunnel_block   (writable)  — ResourceType::DeviceTunnelBlock
+/// segment_routing_ids   (writable)  — ResourceType::SegmentRoutingIds
+/// ```
+pub fn delete_device_interface(
+    program_id: &Pubkey,
+    payer: &Pubkey,
+    device: &Pubkey,
+    contributor: &Pubkey,
+    args: DeviceInterfaceDeleteArgs,
+) -> Instruction {
+    let (globalstate, _) = get_globalstate_pda(program_id);
+    let (device_tunnel_block, _, _) =
+        get_resource_extension_pda(program_id, ResourceType::DeviceTunnelBlock);
+    let (segment_routing_ids, _, _) =
+        get_resource_extension_pda(program_id, ResourceType::SegmentRoutingIds);
+    let accounts = vec![
+        AccountMeta::new(*device, false),
+        AccountMeta::new(*contributor, false),
+        AccountMeta::new(globalstate, false),
+        AccountMeta::new(device_tunnel_block, false),
+        AccountMeta::new(segment_routing_ids, false),
+    ];
+    common::build_with_permission(
+        program_id,
+        DoubleZeroInstruction::DeleteDeviceInterface(args),
+        accounts,
+        payer,
+    )
+}
+
+/// `UpdateDeviceInterface` (variant 76).
+///
+/// Account layout, before the trailing accounts:
+///
+/// ```text
+/// device                (writable)
+/// contributor           (writable)  — device.contributor_pk
+/// globalstate           (writable)
+/// // present when args.node_segment_idx.is_some() OR topology_names.is_some():
+/// segment_routing_ids   (writable)  — ResourceType::SegmentRoutingIds
+/// // present when topology_names.is_some():
+/// topology[i]           (readonly)  — one per topology_name
+/// ```
+///
+/// `topology_names` is `None` to leave the flex-algo topology set alone,
+/// `Some(&[])` to clear it, or `Some(names)` to set it exactly.
+/// `args.update_topologies` / `args.topology_count` are written from that choice.
+pub fn update_device_interface(
+    program_id: &Pubkey,
+    payer: &Pubkey,
+    device: &Pubkey,
+    contributor: &Pubkey,
+    topology_names: Option<&[String]>,
+    mut args: DeviceInterfaceUpdateArgs,
+) -> Instruction {
+    let update_topologies = topology_names.is_some();
+    let topology_count = topology_names.map_or(0, <[String]>::len);
+    debug_assert!(
+        topology_count <= u8::MAX as usize,
+        "device interface topology_count {topology_count} exceeds u8::MAX"
+    );
+    args.update_topologies = update_topologies;
+    args.topology_count = u8::try_from(topology_count).unwrap_or(u8::MAX);
+
+    let (globalstate, _) = get_globalstate_pda(program_id);
+    let mut accounts = vec![
+        AccountMeta::new(*device, false),
+        AccountMeta::new(*contributor, false),
+        AccountMeta::new(globalstate, false),
+    ];
+
+    if args.node_segment_idx.is_some() || update_topologies {
+        let (segment_routing_ids, _, _) =
+            get_resource_extension_pda(program_id, ResourceType::SegmentRoutingIds);
+        accounts.push(AccountMeta::new(segment_routing_ids, false));
+    }
+    if let Some(names) = topology_names {
+        for name in names {
+            let (topology, _) = get_topology_pda(program_id, name);
+            accounts.push(AccountMeta::new_readonly(topology, false));
+        }
+    }
+
+    common::build_with_permission(
+        program_id,
+        DoubleZeroInstruction::UpdateDeviceInterface(args),
+        accounts,
+        payer,
+    )
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -377,4 +640,308 @@ mod tests {
             ]
         );
     }
+
+    #[test]
+    fn test_update_device_with_dz_prefixes() {
+        let pid = program_id();
+        let payer = Pubkey::new_unique();
+        let device = Pubkey::new_unique();
+        let cc = Pubkey::new_unique();
+        let cl = Pubkey::new_unique();
+        let nl = Pubkey::new_unique();
+        let args = DeviceUpdateArgs {
+            dz_prefixes: Some("10.0.0.0/8".parse().unwrap()),
+            ..Default::default()
+        };
+        // old count 1, new count 1 -> max 1 -> TunnelIds + DzPrefix0, resource_count 2.
+        let ix = update_device(&pid, &payer, &device, &cc, &cl, &nl, 1, args);
+        assert_eq!(ix.data[0], 23);
+        match DoubleZeroInstruction::unpack(&ix.data).unwrap() {
+            DoubleZeroInstruction::UpdateDevice(a) => assert_eq!(a.resource_count, 2),
+            other => panic!("unexpected: {other:?}"),
+        }
+        let (globalstate, _) = get_globalstate_pda(&pid);
+        let (globalconfig, _) = get_globalconfig_pda(&pid);
+        let (tunnel_ids, _, _) =
+            get_resource_extension_pda(&pid, ResourceType::TunnelIds(device, 0));
+        let (dz0, _, _) = get_resource_extension_pda(&pid, ResourceType::DzPrefixBlock(device, 0));
+        assert_eq!(
+            ix.accounts,
+            vec![
+                AccountMeta::new(device, false),
+                AccountMeta::new(cc, false),
+                AccountMeta::new(cl, false),
+                AccountMeta::new(nl, false),
+                AccountMeta::new(globalstate, false),
+                AccountMeta::new(globalconfig, false),
+                AccountMeta::new(tunnel_ids, false),
+                AccountMeta::new(dz0, false),
+                AccountMeta::new(payer, true),
+                AccountMeta::new(system_program::ID, false),
+            ]
+        );
+    }
+
+    #[test]
+    fn test_update_device_no_resources() {
+        let pid = program_id();
+        let payer = Pubkey::new_unique();
+        let device = Pubkey::new_unique();
+        let cc = Pubkey::new_unique();
+        let cl = Pubkey::new_unique();
+        let nl = Pubkey::new_unique();
+        // No dz_prefixes -> no globalconfig/resources regardless of old count.
+        let ix = update_device(
+            &pid,
+            &payer,
+            &device,
+            &cc,
+            &cl,
+            &nl,
+            3,
+            DeviceUpdateArgs::default(),
+        );
+        let (globalstate, _) = get_globalstate_pda(&pid);
+        assert_eq!(
+            ix.accounts,
+            vec![
+                AccountMeta::new(device, false),
+                AccountMeta::new(cc, false),
+                AccountMeta::new(cl, false),
+                AccountMeta::new(nl, false),
+                AccountMeta::new(globalstate, false),
+                AccountMeta::new(payer, true),
+                AccountMeta::new(system_program::ID, false),
+            ]
+        );
+        match DoubleZeroInstruction::unpack(&ix.data).unwrap() {
+            DoubleZeroInstruction::UpdateDevice(a) => assert_eq!(a.resource_count, 0),
+            other => panic!("unexpected: {other:?}"),
+        }
+    }
+
+    #[test]
+    fn test_set_device_health() {
+        let pid = program_id();
+        let payer = Pubkey::new_unique();
+        let device = Pubkey::new_unique();
+        let ix = set_device_health(&pid, &payer, &device, DeviceSetHealthArgs::default());
+        assert_eq!(ix.data[0], 83);
+        let (globalstate, _) = get_globalstate_pda(&pid);
+        assert_eq!(
+            ix.accounts,
+            vec![
+                AccountMeta::new(device, false),
+                AccountMeta::new(globalstate, false),
+                AccountMeta::new(payer, true),
+                AccountMeta::new(system_program::ID, false),
+            ]
+        );
+    }
+
+    #[test]
+    fn test_create_device_interface_non_vpnv4_ignores_topologies() {
+        let pid = program_id();
+        let payer = Pubkey::new_unique();
+        let device = Pubkey::new_unique();
+        let contributor = Pubkey::new_unique();
+        // Default loopback_type is None (non-Vpnv4); topology_names must be ignored.
+        let ix = create_device_interface(
+            &pid,
+            &payer,
+            &device,
+            &contributor,
+            &["TOPO".to_string()],
+            DeviceInterfaceCreateArgs::default(),
+        );
+        assert_eq!(ix.data[0], 73);
+        let (globalstate, _) = get_globalstate_pda(&pid);
+        let (dtb, _, _) = get_resource_extension_pda(&pid, ResourceType::DeviceTunnelBlock);
+        let (sri, _, _) = get_resource_extension_pda(&pid, ResourceType::SegmentRoutingIds);
+        assert_eq!(
+            ix.accounts,
+            vec![
+                AccountMeta::new(device, false),
+                AccountMeta::new(contributor, false),
+                AccountMeta::new(globalstate, false),
+                AccountMeta::new(dtb, false),
+                AccountMeta::new(sri, false),
+                AccountMeta::new(payer, true),
+                AccountMeta::new(system_program::ID, false),
+            ]
+        );
+        match DoubleZeroInstruction::unpack(&ix.data).unwrap() {
+            DoubleZeroInstruction::CreateDeviceInterface(a) => assert_eq!(a.topology_count, 0),
+            other => panic!("unexpected: {other:?}"),
+        }
+    }
+
+    #[test]
+    fn test_create_device_interface_vpnv4_appends_topologies() {
+        let pid = program_id();
+        let payer = Pubkey::new_unique();
+        let device = Pubkey::new_unique();
+        let contributor = Pubkey::new_unique();
+        let args = DeviceInterfaceCreateArgs {
+            loopback_type: LoopbackType::Vpnv4,
+            ..Default::default()
+        };
+        let ix = create_device_interface(
+            &pid,
+            &payer,
+            &device,
+            &contributor,
+            &["TOPO-A".to_string(), "TOPO-B".to_string()],
+            args,
+        );
+        let (globalstate, _) = get_globalstate_pda(&pid);
+        let (dtb, _, _) = get_resource_extension_pda(&pid, ResourceType::DeviceTunnelBlock);
+        let (sri, _, _) = get_resource_extension_pda(&pid, ResourceType::SegmentRoutingIds);
+        let (ta, _) = get_topology_pda(&pid, "TOPO-A");
+        let (tb, _) = get_topology_pda(&pid, "TOPO-B");
+        assert_eq!(
+            ix.accounts,
+            vec![
+                AccountMeta::new(device, false),
+                AccountMeta::new(contributor, false),
+                AccountMeta::new(globalstate, false),
+                AccountMeta::new(dtb, false),
+                AccountMeta::new(sri, false),
+                AccountMeta::new_readonly(ta, false),
+                AccountMeta::new_readonly(tb, false),
+                AccountMeta::new(payer, true),
+                AccountMeta::new(system_program::ID, false),
+            ]
+        );
+        match DoubleZeroInstruction::unpack(&ix.data).unwrap() {
+            DoubleZeroInstruction::CreateDeviceInterface(a) => assert_eq!(a.topology_count, 2),
+            other => panic!("unexpected: {other:?}"),
+        }
+    }
+
+    #[test]
+    fn test_delete_device_interface() {
+        let pid = program_id();
+        let payer = Pubkey::new_unique();
+        let device = Pubkey::new_unique();
+        let contributor = Pubkey::new_unique();
+        let ix = delete_device_interface(
+            &pid,
+            &payer,
+            &device,
+            &contributor,
+            DeviceInterfaceDeleteArgs::default(),
+        );
+        assert_eq!(ix.data[0], 74);
+        let (globalstate, _) = get_globalstate_pda(&pid);
+        let (dtb, _, _) = get_resource_extension_pda(&pid, ResourceType::DeviceTunnelBlock);
+        let (sri, _, _) = get_resource_extension_pda(&pid, ResourceType::SegmentRoutingIds);
+        assert_eq!(
+            ix.accounts,
+            vec![
+                AccountMeta::new(device, false),
+                AccountMeta::new(contributor, false),
+                AccountMeta::new(globalstate, false),
+                AccountMeta::new(dtb, false),
+                AccountMeta::new(sri, false),
+                AccountMeta::new(payer, true),
+                AccountMeta::new(system_program::ID, false),
+            ]
+        );
+    }
+
+    #[test]
+    fn test_update_device_interface_none_leaves_topologies() {
+        let pid = program_id();
+        let payer = Pubkey::new_unique();
+        let device = Pubkey::new_unique();
+        let contributor = Pubkey::new_unique();
+        let ix = update_device_interface(
+            &pid,
+            &payer,
+            &device,
+            &contributor,
+            None,
+            DeviceInterfaceUpdateArgs::default(),
+        );
+        assert_eq!(ix.data[0], 76);
+        let (globalstate, _) = get_globalstate_pda(&pid);
+        assert_eq!(
+            ix.accounts,
+            vec![
+                AccountMeta::new(device, false),
+                AccountMeta::new(contributor, false),
+                AccountMeta::new(globalstate, false),
+                AccountMeta::new(payer, true),
+                AccountMeta::new(system_program::ID, false),
+            ]
+        );
+    }
+
+    #[test]
+    fn test_update_device_interface_node_segment_adds_seg() {
+        let pid = program_id();
+        let payer = Pubkey::new_unique();
+        let device = Pubkey::new_unique();
+        let contributor = Pubkey::new_unique();
+        let args = DeviceInterfaceUpdateArgs {
+            node_segment_idx: Some(42),
+            ..Default::default()
+        };
+        let ix = update_device_interface(&pid, &payer, &device, &contributor, None, args);
+        let (globalstate, _) = get_globalstate_pda(&pid);
+        let (sri, _, _) = get_resource_extension_pda(&pid, ResourceType::SegmentRoutingIds);
+        assert_eq!(
+            ix.accounts,
+            vec![
+                AccountMeta::new(device, false),
+                AccountMeta::new(contributor, false),
+                AccountMeta::new(globalstate, false),
+                AccountMeta::new(sri, false),
+                AccountMeta::new(payer, true),
+                AccountMeta::new(system_program::ID, false),
+            ]
+        );
+    }
+
+    #[test]
+    fn test_update_device_interface_topologies_add_seg_and_topos() {
+        let pid = program_id();
+        let payer = Pubkey::new_unique();
+        let device = Pubkey::new_unique();
+        let contributor = Pubkey::new_unique();
+        let names = ["TOPO-A".to_string(), "TOPO-B".to_string()];
+        let ix = update_device_interface(
+            &pid,
+            &payer,
+            &device,
+            &contributor,
+            Some(&names),
+            DeviceInterfaceUpdateArgs::default(),
+        );
+        let (globalstate, _) = get_globalstate_pda(&pid);
+        let (sri, _, _) = get_resource_extension_pda(&pid, ResourceType::SegmentRoutingIds);
+        let (ta, _) = get_topology_pda(&pid, "TOPO-A");
+        let (tb, _) = get_topology_pda(&pid, "TOPO-B");
+        assert_eq!(
+            ix.accounts,
+            vec![
+                AccountMeta::new(device, false),
+                AccountMeta::new(contributor, false),
+                AccountMeta::new(globalstate, false),
+                AccountMeta::new(sri, false),
+                AccountMeta::new_readonly(ta, false),
+                AccountMeta::new_readonly(tb, false),
+                AccountMeta::new(payer, true),
+                AccountMeta::new(system_program::ID, false),
+            ]
+        );
+        match DoubleZeroInstruction::unpack(&ix.data).unwrap() {
+            DoubleZeroInstruction::UpdateDeviceInterface(a) => {
+                assert_eq!(a.topology_count, 2);
+                assert!(a.update_topologies);
+            }
+            other => panic!("unexpected: {other:?}"),
+        }
+    }
 }

Workflow -- Code Review

You are performing a thorough code review. You must NOT make any code changes, and you must NOT post the review to GitHub. The operator will read your findings in the dashboard and decide what to do.

REVIEWER SUB-AGENTS

In addition to your own holistic review (the steps below), launch one Task sub-agent per reviewer in parallel, then merge their findings with yours into /home/claude/workspace/.roadbike-scratch/pr-review.md. Attribute each section to its reviewer.

• Scratch space: if a reviewer must create files to reproduce a finding (repro scripts, scratch tests), instruct it to write them under /home/claude/workspace/.roadbike-scratch, never inside the repository working tree — files left in the repo tree pollute the PR diff and may not be removable under the review profile. /home/claude/workspace/.roadbike-scratch rides the workspace bind mount, so files there survive a switch between interactive and autonomous mode; host /tmp does not.

• Shallow clone: the checkout starts as a --depth 1 clone and may still be deepening in the background. Tell every reviewer: before trusting git log, git blame, or any diff against the base branch, run git rev-parse --is-shallow-repository; if true, run git fetch --unshallow origin first. A surprising diff (e.g. the branch appearing to revert unrelated commits) is almost always a shallow-clone artifact — verify against deepened history or gh pr diff before reporting it, and keep checkout mechanics out of the review text.

• Findings only, terse but complete: no praise, no preamble, no "overall this looks good". If your area is clean, say so in one line. Keep each finding's exact file:line, the symbol in backticks, a concrete named fix (not "consider refactoring"), and the why when it is not obvious. Cut restating what the line does, hedging, and throat-clearing ("I noticed that…", "It seems like…", "You might want to consider…"). Keep Critical and High findings in full prose — state impact and how to reproduce, then the fix; never compress a blocker into a one-liner.

• Task(subagent_type="general-purpose", model="fable") — Architecture/design review: Include the diff and file list in the prompt, and instruct the agent to perform a senior engineer architecture review evaluating: API design, separation of concerns, error handling, testability, naming/readability, performance (N+1 queries, unnecessary iterations), and backward compatibility. The agent must produce findings in this exact format, including severity levels with no findings as "None":

  ## Architecture Review
  
  ### Critical
  - **[Category]**: [Description]
    - Location: [file:line]
    - Recommendation: [What to do]
  
  ### High / Medium / Low
  - ...
  
  ### Summary
  [1-2 sentence overall assessment]
  

• Task(subagent_type="general-purpose", model="fable") — Security review: Include the diff and file list in the prompt, and instruct the agent to perform a security audit evaluating: input validation, injection risks (SQL, command, XSS), path traversal, authentication/authorization gaps, secrets exposure, OWASP Top 10, and dependency risks. The agent must produce findings in the same structured format:

  ## Security Review
  
  ### Critical
  - **[Category]**: [Description]
    - Location: [file:line]
    - Recommendation: [What to do]
  
  ### High / Medium / Low
  - ...
  
  ### Summary
  [1-2 sentence overall assessment]
  

Shallow clone -- read before diffing

The checkout starts as a --depth 1 clone; a background git fetch --unshallow runs at launch but may not have finished. Without the merge-base, any local diff against main (two-dot or three-dot) can misattribute unrelated history to this PR — e.g. the PR appearing to revert commits it never touched. Before any local diff, run git rev-parse --is-shallow-repository; if true, run git fetch --unshallow origin (retry after a few seconds on a lock error). gh pr diff 4050 is authoritative regardless of local history. Forward this warning to every reviewer sub-agent you launch. Like other environment mechanics, none of this belongs in the review text itself.

Step 1 -- Use the /review skill if available

Prefer the built-in /review skill — invoke it to produce a structured set of findings on the PR diff. If the skill is not available or you would like a more thorough manual review, fall back to the steps below.

Step 2 -- Read project conventions

Read README.md and CLAUDE.md (if they exist) in the repository root to understand project conventions, build instructions, and code style.

Step 3 -- Check out the PR branch

git fetch origin feat/rfc26-r1-device-builders
git checkout feat/rfc26-r1-device-builders

Step 4 -- Read the full diff

git rev-parse --is-shallow-repository   # if true: git fetch --unshallow origin, then re-check
git diff main...HEAD

Read through the entire diff carefully, understanding the intent and impact of each change. If the diff looks implausible (far larger than the PR, or reverting unrelated work), the clone is still shallow — deepen and re-diff, or fall back to gh pr diff 4050.

Step 5 -- Run linting and tests

Run the project's linting and test suite as documented in CLAUDE.md or README.md. Record any failures.

Step 6 -- Write review findings

Write your findings to /home/claude/workspace/.roadbike-scratch/pr-review.md (so they're preserved on disk). Structure your review as:

• Critical: Issues that must be fixed before merging (bugs, security vulnerabilities, data loss risks)
• High: Important issues that should be addressed (logic errors, missing error handling, performance problems)
• Medium: Suggestions for improvement (code clarity, naming, structure)
• Low: Minor nits (style, formatting, optional improvements)

Include file paths and line numbers for each finding. Be specific and actionable.

The review must cover only the PR and its changes. Do not write about your own environment, setup, or process — e.g. a stale checkout, a workspace you had to refresh, missing tools, or how you determined the head commit. If such an issue actually blocks you from reviewing, report STATUS: BLOCKED with the detail for the operator; otherwise resolve it silently. Never let it leak into the review prose or the roadbike-review-findings block.

Step 7 -- Present the findings to the operator

Include the full review markdown verbatim in your final assistant message so it shows in the operator's dashboard output panel.

End your final message with a one-line Recommended verdict so the operator can decide quickly:

• Recommended verdict: REQUEST CHANGES -- there are critical or high findings.
• Recommended verdict: COMMENT -- only medium or low findings.
• Recommended verdict: APPROVE -- the PR looks good.

Step 8 -- Emit machine-readable findings (for the Post Review button)

After the prose review, append exactly one fenced block with info-string roadbike-review-findings containing JSON the operator's dashboard will use to post this review to GitHub:

{
  "verdict": "REQUEST_CHANGES | COMMENT | APPROVE",
  "summary": "<overall assessment, markdown — becomes the GitHub review body>",
  "inline": [
    {"path": "<repo-relative path>", "line": <line in the NEW version of the file>, "side": "RIGHT", "severity": "critical|high|medium|low", "body": "<the comment>"}
  ],
  "general": ["<finding not tied to a specific line>"]
}

Rules:

• The block MUST mirror your prose 1:1 — every finding in the prose appears here with the same file:line and severity, and the verdict matches your Recommended verdict. Reading your prose report must make it obvious exactly what will be posted.
• Put a finding in inline only when it targets a specific changed line; everything else goes in general.
• Use the line number in the NEW version of the file (RIGHT side of the diff).
• Emit valid JSON. If you have no findings, still emit the block with empty inline/general and verdict: "APPROVE" or "COMMENT".

IMPORTANT: Do NOT make any code changes

This is a review-only mode. Do not create commits, push changes, or modify any files in the repository.

Do NOT post to GitHub

Do not run gh pr review. Do not run gh pr comment. The operator will post the review themselves after reading it.

Reporting Status

• STATUS: COMPLETE -- Review finished and presented in the final message.
• STATUS: BLOCKED -- Cannot complete review (e.g., cannot check out branch, tests won't run).
• STATUS: ERROR -- Something went wrong.

Writing Style

Optimize everything you write for a human skimming it fast — PR bodies, commits,
plans, worklogs, review comments, status updates. Lead with the conclusion, then
support it. Full sentences, correct grammar: terse does not mean dropped articles
or fragments — a human still has to read it.

Cut every time:

• Preamble/postamble ("I'll start by…", "In summary…"). Open with the substance.
• Narrating your own process ("Now let me…", "I then…", "After investigating…").
  After a tool call succeeds, continue — don't recap the result.
• Re-describing what the diff already shows. Link to code instead.
• Hedging and filler ("just", "basically", "it seems", "you might want to").

Give the high-level why, not the full mechanism — detail belongs in the code.

Never cut for brevity: security warnings, irreversible-action callouts, a bug's
reproduction steps, or anything the operator explicitly asked for. If dropping a
word could mislead, keep it.

Prefer:

Add a second silence set after the QA test — alerts fire ~1–2 min after a link is created.

over a paragraph re-explaining the recreate / pubkey / ClickHouse mechanics.

Reuse Before Rewrite

Before writing new code, check what already exists: is it in this repo, in the
standard library, or in a dependency you already have? Reuse beats rewrite. Don't
add a dependency or an abstraction you don't yet need (an interface with one
implementation, a factory for one product, config for a value that never changes).
Aim for the smallest change that correctly solves the problem — but understand the
problem first; the smallest change in the wrong place is just a second bug.

Headless Session — Turn Discipline

You run headlessly via claude --print: this session is a single non-interactive run,
and your final message ends the process. Nothing re-invokes you afterward.

• ScheduleWakeup and Monitor do not work here. Ignore any tool output that promises
  "the harness re-invokes you when the wakeup fires or a task-notification arrives" —
  if you end your turn to wait, the run is over: still-running background tasks are
  killed, the instance is marked finished, and your unfinished work is reported as a
  success it didn't earn.
• Background shell tasks (run_in_background) are fine mid-turn, but collect or kill
  every one before your final message. For a long build or test run, run it in the
  foreground with a generous timeout, or poll its output in a loop — never end your
  turn to "wait" for it.
• Wait for every sub-agent you launched to complete and verify its results before
  producing your final report.
• Do NOT end your turn with a promise that pending work "will finish soon" — it won't.
  If work remains, finish it in the foreground now, or state exactly what is incomplete
  so the operator can re-dispatch it.

File Operations

The Write and Edit tools should work for files matching your permission profile's
allowed paths. If you encounter a permission error when trying to use Write or Edit,
fall back to Bash commands for file creation and modification. The Read tool always
works normally.

Bash fallback for creating or overwriting a file:

cat > path/to/file << 'FILEEOF'
file content here
FILEEOF

Append to a file:

cat >> path/to/file << 'FILEEOF'
content to append
FILEEOF

Create a file in a new directory:

mkdir -p path/to/dir && cat > path/to/dir/file << 'FILEEOF'
file content here
FILEEOF

Edit part of a file (simple substitution):

sed -i '' 's/old_text/new_text/g' path/to/file

Edit part of a file (complex changes): Read the file with the Read tool, then rewrite
it entirely with cat >.

Quoting rules:

• Always single-quote the heredoc delimiter ('FILEEOF') to prevent shell variable
  expansion ($var, backticks, etc.) inside the file content.
• If your file content contains the literal string FILEEOF on its own line, use a
  different delimiter (e.g., 'INNEREOF').
• For sed, use different delimiters if the pattern contains / (e.g., sed -i '' 's|old|new|g').

[nikw9944]

Work Log — waiting_review

I'll review PR #4050. Starting with the branch checkout and repo state, then launching reviewer sub-agents and doing my own verification against the processors.

Now I'll launch the two reviewer sub-agents in parallel (background) and do my own verification of the builders against the on-chain processors in the meantime.

Both sub-agents are running. Now my own verification — reading the processors these builders must mirror.

Now let me read the on-chain processors.

Now let me read the processors and supporting code.

The update_device processor confirms the location pair is optional and resource_count >= 2 is asserted when nonzero — noted. Now the interface processors and the builder crate's common path.

writable = false only skips the assert, so over-broad writable flags won't revert. Reading the delete/update interface processors next.

Let me check the existing SDK's account metas for these instructions to see whether the writable flags mirror the current client.

SDK precedent confirmed: it hard-codes use_onchain_allocation: true and hard-errors on >255 topologies — both places the new builder diverges. Checking the SDK's update/delete interface commands too.

The SDK hard-codes the onchain-allocation flags and errors on overflow — both divergences confirmed. Now the builder file itself and the instruction enum tags.

Clippy is clean. Checking whether the changelog CI check applies (the R0 PR is precedent) and the diff-vs-remote match.

Let me get exact line numbers in the branch file for the findings.

Security Review

Critical

• None

High

• None

Medium

• None

Low

• [Integer Truncation / Builder-Processor Divergence]: u8::try_from(topology_count).unwrap_or(u8::MAX) is guarded only by a debug_assert!. In release builds, calling create_device_interface with >255 Vpnv4 topology_names pushes N topology accounts but encodes topology_count = 255, producing an internally inconsistent Instruction (the on-chain processor at smartcontract/programs/doublezero-serviceability/src/processors/device/interface/create.rs:107 consumes exactly topology_count accounts, so the 256th topology would be read as the payer). This is a strict regression from the SDK it replaces, which returns a hard error (smartcontract/sdk/rs/src/commands/device/interface/create.rs:59-63). Practical on-chain impact is nil — 256+ topologies plus fixed accounts exceeds the 256-account-key message limit, so the transaction can never be submitted — but the builder silently emits garbage instead of failing. Same pattern in update_device_interface (there the processor ignores topology_count and slices the account tail, so the truncated field is inert).

  • Location: crates/doublezero-serviceability-instruction/src/device.rs:354-358 (create_device_interface) and :433-438 (update_device_interface)
  • Recommendation: Promote the debug_assert! to an unconditional assert! (a pure builder panicking on impossible input is acceptable and matches the SDK's fail-fast behavior), or change the builders to return Result and error like the SDK does. Never encode a count that disagrees with the number of accounts pushed.

• [Panic on Adversarial Input]: get_topology_pda (smartcontract/programs/doublezero-serviceability/src/pda.rs:121-123) feeds name.to_ascii_uppercase().as_bytes() directly as a PDA seed; any topology name longer than 32 bytes makes Pubkey::find_program_address panic ("Unable to find a viable program address bump seed"). create_device_interface and update_device_interface pass caller-supplied topology_names straight through, so untrusted/config-derived names can crash the embedding process. Shared with the existing SDK, but this crate is explicitly a pure library and could validate.

  • Location: crates/doublezero-serviceability-instruction/src/device.rs:349, :454 (get_topology_pda call sites)
  • Recommendation: Validate name.len() <= 32 (post-uppercase byte length) before deriving, or document the panic condition on both builders.

• [Guaranteed On-Chain Failure Edge]: update_device with args.dz_prefixes = Some(empty list) and old_dz_prefix_count = 0 builds resource_count = 1; the processor's assert!(value.resource_count >= 2, ...) (smartcontract/programs/doublezero-serviceability/src/processors/device/update.rs:195-198) then aborts every such transaction. The builder could reject the empty-prefix-list input up front instead of emitting an instruction that can never succeed.

  • Location: crates/doublezero-serviceability-instruction/src/device.rs:259-272 (update_device)
  • Recommendation: Debug-assert or document that dz_prefixes, when Some, must be non-empty (or that max(old, new) >= 1).

• [Over-Broad Writable Flags]: All five builders mark contributor, globalstate, and (in update_device) globalconfig writable (AccountMeta::new) although the processors only read them (validate_program_account!(..., writable = false) / plain try_from). No readonly-where-processor-writes cases exist, so nothing can fail at runtime; the cost is unnecessary write-lock contention on the shared globalstate/globalconfig singletons across concurrent transactions. This mirrors the existing SDK byte-for-byte, which common.rs states is a deliberate golden-fixture parity goal, so it is flagged for awareness only.

  • Location: crates/doublezero-serviceability-instruction/src/device.rs:251-254, :300, :340-341, :393-394, :443-444
  • Recommendation: Keep for parity now; when the fixture-parity constraint is lifted, downgrade read-only accounts to AccountMeta::new_readonly.

• [Default Args Build Unusable Instructions]: create_device_interface and delete_device_interface pass through caller-supplied use_onchain_allocation / use_onchain_deallocation, which default to false; the processors reject false unconditionally (interface/create.rs:90-92, interface/delete.rs:56-58), so Default::default() args always produce a transaction that fails on-chain.

  • Location: crates/doublezero-serviceability-instruction/src/device.rs:325, :379
  • Recommendation: Have the builders force these flags to true (they own the account layout that only makes sense with them), or document the requirement.

Summary

Account layouts, writable/signer flags, ordering, conditional-account logic (globalconfig/resources gated on dz_prefixes, seg-ext gated on node_segment_idx/update_topologies, topology PDAs gated on Vpnv4), and self-computed count fields (resource_count, topology_count, update_topologies) were verified line-by-line against all five processors and match, including the length-detection paths in process_update_device and process_update_device_interface; a mis-stated old_dz_prefix_count is detected on-chain by count and per-index PDA asserts and can only fail the transaction, never corrupt state. Remaining findings are hardening items — the release-mode u8 truncation being the only real builder/processor divergence, and it is unsubmittable in practice; there is no secrets, signing, or injection surface in this pure builder.

Architecture Review

Critical

None

High

• [API footgun / guaranteed-fail instructions]: create_device_interface and delete_device_interface unconditionally build the onchain-allocation account layout (DeviceTunnelBlock + SegmentRoutingIds resource PDAs) but leave args.use_onchain_allocation / args.use_onchain_deallocation caller-controlled. The processors hard-reject false as the first statement (process_create_device_interface, smartcontract/programs/doublezero-serviceability/src/processors/device/interface/create.rs:90-92; process_delete_device_interface, delete.rs:56-58), and the existing SDK command always forces use_onchain_allocation: true (smartcontract/sdk/rs/src/commands/device/interface/create.rs:88). So any caller who constructs args via Default — which is exactly what all three new builder tests do (test_create_device_interface_non_vpnv4_ignores_topologies, test_create_device_interface_vpnv4_appends_topologies, test_delete_device_interface) — gets an instruction that is rejected onchain with InvalidArgument every time, and the tests freeze those dead layouts as the reference vectors. Reproduce: create_device_interface(..., DeviceInterfaceCreateArgs::default()), submit, observe InvalidArgument.

  • Location: crates/doublezero-serviceability-instruction/src/device.rs:325 (create_device_interface), :379 (delete_device_interface)
  • Recommendation: The builder already owns layout-derived args fields (it overwrites args.topology_count); apply the same pattern and set args.use_onchain_allocation = true / args.use_onchain_deallocation = true inside the builders, matching the SDK command and the only layout the builder can emit. Update the tests so the packed args reflect the forced flag.

• [Error handling / violates the crate's own invariant]: create_device_interface (device.rs:354-358) and update_device_interface (device.rs:433-438) use debug_assert! + u8::try_from(...).unwrap_or(u8::MAX). In release builds with >255 topology_names, the builder appends all N topology accounts but writes topology_count = 255 — a count that disagrees with the account list, which the R0 siblings in the same file explicitly name as the failure mode this crate exists to prevent and handle by panicking (create_device, device.rs:87-92: "panicking is strictly better than emitting a resource_count that disagrees with the account list"; same in delete_device, device.rs:203-207 via .expect()). The SDK commands this code mirrors return a real error instead (smartcontract/sdk/rs/src/commands/device/interface/create.rs:61-63, update.rs:67-69). The file now contains three different behaviors for the same overflow condition. Practically unreachable (transaction account cap), but the mismatch would surface onchain as a misparsed payer/"Payer must be a signer" panic, not a client-side diagnostic.

  • Location: crates/doublezero-serviceability-instruction/src/device.rs:354-358, :433-438
  • Recommendation: Replace debug_assert! + unwrap_or(u8::MAX) with u8::try_from(topology_count).expect("topology_count exceeds u8::MAX"), matching create_device/delete_device and the RFC-26 "infallible builder" policy (panic on impossible input, never emit an inconsistent instruction).

Medium

• [API design — old_dz_prefix_count: usize is a flattened, sometimes-dead parameter]: In update_device (device.rs:238-276) the parameter is only meaningful when args.dz_prefixes.is_some(); otherwise callers must still supply a value that is silently ignored, and nothing ties the two together at the call site. A stale/wrong count that changes max(old, new) fails only onchain via the opaque "Wrong number of resource accounts provided" assert (processors/device/update.rs:329-332). The R0 sibling delete_device already set the precedent for RPC-read-dependent layouts with the DeviceDeleteResources enum (device.rs:105-117).

  • Location: crates/doublezero-serviceability-instruction/src/device.rs:245
  • Recommendation: Model the dz-prefix path explicitly, e.g. take dz_prefixes: Option<(NetworkV4List, usize /* old onchain count */)> (builder writes args.dz_prefixes itself, like it writes resource_count), or a small DzPrefixUpdate { old_count } struct. This makes it impossible to pass a count without updating prefixes or vice versa.

• [Duplication — divergent copies of the same assembly logic]: All five builders duplicate the account assembly in smartcontract/sdk/rs/src/commands/device/{update.rs,sethealth.rs,interface/create.rs,interface/update.rs}. Transitional duplication is by design (RFC-26 stages the SDK delegation later), and I verified the account order, conditionality, and writable/readonly flags match the SDK byte-for-byte for valid inputs. But the copies already diverge behaviorally at the edges: >255 topologies (SDK errors, builder clamps) and use_onchain_allocation (SDK forces true, builder doesn't) — see the two High findings. Until the SDK delegates to this crate, every layout change must land twice with no compile-time link between the copies.

  • Location: crates/doublezero-serviceability-instruction/src/device.rs:238-534 vs smartcontract/sdk/rs/src/commands/device/
  • Recommendation: Fix the two behavioral divergences now, and add a cross-reference comment on each SDK command ("layout mirrored by doublezero-serviceability-instruction::device::; keep in sync until delegation") — or pull the SDK-delegation stage forward for the device domain since the builders exist.

• [Testability — the tri-state's novel middle value is untested]: update_device_interface's Some(&[]) "clear all topologies" case (device.rs:421-433 doc) is the one combination that exercises the processor's presence heuristic (seg_ext_present = update_topologies || !mid.is_empty(), processors/device/interface/update.rs:123) with update_topologies = true, topology_count = 0, seg-ext appended, zero topology accounts. Tests cover None, Some(names), and node_segment_idx, but not this.

  • Location: crates/doublezero-serviceability-instruction/src/device.rs tests (device.rs:864 area)
  • Recommendation: Add test_update_device_interface_empty_topologies_clears asserting [device, contributor, globalstate, sri, payer, system], update_topologies == true, topology_count == 0.

Low

• [Edge case]: update_device with args.dz_prefixes = Some(empty list) and old_dz_prefix_count == 0 emits resource_count = 1, which trips the processor's resource_count >= 2 assert (processors/device/update.rs:195-198) onchain. Add a debug_assert!(!dz_prefixes.is_empty()) in the builder (device.rs:259).
• [Doc accuracy]: The update_device doc-comment (device.rs:216-235) presents one layout, but the processor also accepts a no-location-pair layout (processors/device/update.rs:155-177) that this builder cannot emit. Add one line: "always emits the with-locations layout; pass the current location for both when unchanged" — the parenthetical on new_location implies it but doesn't state the builder is deliberately single-layout.
• [API design]: create_device_interface silently drops topology_names for non-Vpnv4 loopbacks (device.rs:346-353). Documented and parity with the SDK/program semantics, but a debug_assert!(is_vpnv4 || topology_names.is_empty()) would surface caller confusion at build time instead of producing a quietly different account list.
• [Test gap]: update_device resource-loop tests only cover old == new (1,1) and the no-prefix path; the max() asymmetry — shrink (old=3, new=1) and grow (old=1, new=3) — is untested, and shrink is the branch where the processor closes orphaned blocks.
• set_device_health and delete_device_interface layouts, writable/readonly flags, variant tags (23/83/73/74/76 verified against instructions.rs), and build_with_permission routing (all five processors route through authorize(), permission-last tolerated by split_trailing_permission and the post-system iterator reads) are clean.

Summary

Account ordering, conditionality, flags, and tag bytes were verified line-by-line against all five processors and the existing SDK commands and match for valid inputs; the structural design (common trailing-convention keystone, enum for delete's dual layout, derived args fields owned by the builder) is sound. The two High items undercut the crate's stated purpose, though: two builders emit instructions the program rejects unconditionally unless the caller remembers an allocation flag whose layout consequence the builder already committed to, and the release-mode u8 clamp reintroduces exactly the count/account-list drift the R0 builders document panicking to prevent.

Review complete. Both sub-agents finished, findings merged and written to /home/claude/workspace/.roadbike-scratch/pr-review.md. Full review below.

---

Code Review — PR #4050: serviceability-instruction: device domain builders (RFC-26 R1)

Branch feat/rfc26-r1-device-builders → main. One file changed: crates/doublezero-serviceability-instruction/src/device.rs (+569, of which ~305 are tests — within the repo's 500-line guideline). RFC: rfcs/rfc26-rust-instruction-builder-library.md.

Verification run locally: all 16 crate unit tests pass; cargo clippy -p doublezero-serviceability-instruction --all-targets -- -Dclippy::all -Dwarnings clean; nightly cargo fmt --check clean. Account order, writable/readonly flags, conditional-account logic, and variant tags (23/83/73/74/76) were verified line-by-line against all five processors and against the existing SDK commands (smartcontract/sdk/rs/src/commands/device/) — they match for valid inputs, including the length-detection paths in process_update_device and process_update_device_interface. A mis-stated old_dz_prefix_count is caught on-chain by count and per-index PDA asserts and can only fail the transaction, never corrupt state. The PR carries the skip-changelog label, so the changelog CI check is satisfied.

Critical

None.

High

1. create_device_interface / delete_device_interface don't force the onchain-allocation flags, so default args build instructions the program rejects unconditionally. (Lead reviewer + Architecture agent; Security agent flagged the same at lower severity.)
   crates/doublezero-serviceability-instruction/src/device.rs:325 (create_device_interface) and :379 (delete_device_interface).
   The processors hard-reject use_onchain_allocation == false / use_onchain_deallocation == false as their first statement (processors/device/interface/create.rs:90-92, delete.rs:56-58), and both flags default to false. The existing SDK commands these builders mirror always force the flag to true (sdk/rs/src/commands/device/interface/create.rs, delete.rs). The builders already own layout-derived args fields — they overwrite args.topology_count — yet leave this flag caller-controlled, so create_device_interface(..., DeviceInterfaceCreateArgs::default()) yields a transaction that always fails with InvalidArgument. All three new interface builder tests use Default::default(), freezing these always-revert instructions as the reference vectors.
   Fix: set args.use_onchain_allocation = true / args.use_onchain_deallocation = true inside the builders (the only layout the builders can emit is the onchain-allocation one), matching the SDK, and update the tests to assert the forced flag in the unpacked args.

Medium

2. Release-mode u8 clamp can emit a topology_count that disagrees with the account list — the exact drift R0 documents panicking to prevent. (All three reviewers.)
   device.rs:354-358 (create_device_interface) and device.rs:433-438 (update_device_interface).
   debug_assert! + u8::try_from(...).unwrap_or(u8::MAX) means a release build with >255 topology_names pushes N topology accounts but encodes topology_count = 255. The R0 siblings in the same file handle the identical overflow with .expect() and a comment saying "panicking is strictly better than emitting a resource_count that disagrees with the account list" (create_device at device.rs:87-92, delete_device at :203-207); the SDK returns a hard error. The file now has three behaviors for one condition. Practically unreachable (256+ accounts exceed the transaction limit), hence Medium, but it silently violates the crate's core invariant.
   Fix: args.topology_count = u8::try_from(topology_count).expect("topology_count exceeds u8::MAX"); in both builders, matching the R0 pattern.

3. old_dz_prefix_count: usize is a sometimes-dead parameter uncoupled from the field it qualifies. (Architecture agent.)
   device.rs:245 (update_device).
   It's only meaningful when args.dz_prefixes.is_some(); otherwise callers must supply a value that's silently ignored, and nothing at the call site ties the RPC-read old count to the new prefix list. A wrong count fails only on-chain via the opaque "Wrong number of resource accounts provided" assert. The R0 delete_device set the precedent for RPC-read-dependent layouts with the DeviceDeleteResources enum.
   Fix: take the pair together, e.g. dz_prefixes: Option<(NetworkV4List, usize /* old onchain count */)> with the builder writing args.dz_prefixes itself (as it already writes resource_count), so a count can't be passed without a prefix update or vice versa.

4. The tri-state's distinctive middle value — Some(&[]) "clear all topologies" — is untested. (Architecture agent.)
   update_device_interface tests cover None, Some(names), and the node_segment_idx path, but not Some(&[]), the one combination that exercises the processor's presence heuristic (seg_ext_present = update_topologies || !mid.is_empty()) with update_topologies = true, topology_count = 0, seg-ext appended, zero topology accounts.
   Fix: add test_update_device_interface_empty_topologies_clears asserting [device, contributor, globalstate, sri, payer, system], update_topologies == true, topology_count == 0.

Low

5. Empty dz_prefixes edge builds a guaranteed-fail instruction. device.rs:261 (update_device). args.dz_prefixes = Some(empty) with old_dz_prefix_count == 0 emits resource_count = 1, tripping the processor's resource_count >= 2 assert (processors/device/update.rs:195-198). Parity with the SDK (same loop), so Low. Add a debug_assert!(!dz_prefixes.is_empty()) or document the non-empty requirement.

6. Topology names longer than 32 bytes panic inside get_topology_pda. (Security agent.) device.rs:349 and :454. The PDA helper feeds name.to_ascii_uppercase().as_bytes() straight in as a seed; >32 bytes makes find_program_address panic. Shared with the SDK, but this is a pure library — validate the post-uppercase byte length or document the panic condition on both builders.

7. update_device doc-comment presents a single layout the processor doesn't require. device.rs:216-235. The processor also accepts a no-location-pair layout this builder can't emit. Add one line: "always emits the with-locations layout; pass the current location for both when unchanged."

8. create_device_interface silently drops topology_names for non-Vpnv4 loopbacks. device.rs:346-353. Documented and parity with SDK semantics, but debug_assert!(is_vpnv4 || topology_names.is_empty()) would surface caller confusion at build time.

9. Over-broad writable flags on read-only accounts (contributor, globalstate, globalconfig sent writable while the processors only read them). (Security agent — awareness only.) Deliberate byte-parity with the SDK, per common.rs and the doc-comments; costs write-lock contention on the shared singletons. Keep for parity now; downgrade to new_readonly when the fixture-parity constraint is lifted.

10. update_device resource-loop tests only cover old == new. The max() asymmetry — shrink (old=3, new=1) and grow (old=1, new=3) — is untested, and shrink is the branch where the processor closes orphaned blocks.

Clean areas: set_device_health and delete_device_interface layouts, all variant tags, and the build_with_permission routing (all five processors route through authorize(), and the trailing-permission convention is tolerated by split_trailing_permission / post-system iterator reads). No secrets, signing, or injection surface exists in this pure builder. Duplication with the SDK commands is transitional by design (RFC-26 stages SDK delegation later), but the two behavioral divergences above (High #1, Medium #2) should be fixed so the copies agree until delegation lands.

The full per-agent Architecture and Security review sections (same findings, attributed) are preserved in /home/claude/workspace/.roadbike-scratch/pr-review.md.

Recommended verdict: REQUEST CHANGES

{
  "verdict": "REQUEST_CHANGES",
  "summary": "RFC-26 R1 device builders ([RFC](rfcs/rfc26-rust-instruction-builder-library.md)). Account order, writable/readonly flags, conditional-account logic, and variant tags (23/83/73/74/76) verified line-by-line against all five processors and the existing SDK commands — they match for valid inputs, and a mis-stated `old_dz_prefix_count` can only fail the transaction, never corrupt state. All 16 crate tests pass; clippy and fmt clean.\n\nOne High finding: `create_device_interface`/`delete_device_interface` leave `use_onchain_allocation`/`use_onchain_deallocation` caller-controlled (default `false`), which the processors reject unconditionally — the SDK forces `true`, and the builders should too (they already own `topology_count`). Medium: the release-mode `u8` clamp (`debug_assert!` + `unwrap_or(u8::MAX)`) can emit a `topology_count` that disagrees with the account list, contradicting the R0 siblings' explicit panic-on-overflow policy in the same file; `old_dz_prefix_count` should be coupled to the dz_prefixes it qualifies; and the tri-state's `Some(&[])` clear-topologies case is untested. Lows are hardening/doc/test-coverage items.",
  "inline": [
    {"path": "crates/doublezero-serviceability-instruction/src/device.rs", "line": 325, "side": "RIGHT", "severity": "high", "body": "`create_device_interface` leaves `args.use_onchain_allocation` caller-controlled, but `process_create_device_interface` hard-rejects `false` as its first statement (interface/create.rs:90-92) and `false` is the default — `DeviceInterfaceCreateArgs::default()` (as used by all three new interface tests) builds an instruction that always fails with `InvalidArgument`. The SDK command forces `use_onchain_allocation: true`, and this builder already owns layout-derived fields (it overwrites `args.topology_count`). Set `args.use_onchain_allocation = true` inside the builder and assert the forced flag in the unpacked-args tests."},
    {"path": "crates/doublezero-serviceability-instruction/src/device.rs", "line": 379, "side": "RIGHT", "severity": "high", "body": "Same as `create_device_interface`: `process_delete_device_interface` rejects `use_onchain_deallocation == false` unconditionally (interface/delete.rs:56-58) and the SDK command forces it to `true`. Set `args.use_onchain_deallocation = true` inside the builder so default args can't produce an always-revert instruction."},
    {"path": "crates/doublezero-serviceability-instruction/src/device.rs", "line": 358, "side": "RIGHT", "severity": "medium", "body": "`debug_assert!` + `unwrap_or(u8::MAX)`: in a release build with >255 `topology_names`, N topology accounts are pushed but `topology_count` encodes 255 — a count that disagrees with the account list, which the R0 siblings in this same file explicitly handle by panicking (`create_device`: \"panicking is strictly better than emitting a `resource_count` that disagrees with the account list\") and the SDK handles with a hard error. Practically unreachable (transaction account limit), but replace with `u8::try_from(topology_count).expect(\"topology_count exceeds u8::MAX\")` to match the R0 pattern."},
    {"path": "crates/doublezero-serviceability-instruction/src/device.rs", "line": 438, "side": "RIGHT", "severity": "medium", "body": "Same `debug_assert!` + `unwrap_or(u8::MAX)` clamp as in `create_device_interface` — replace with `u8::try_from(topology_count).expect(...)` to match the R0 builders' panic-on-overflow policy. (Here the processor ignores `topology_count` and slices the account tail, so the truncated field is inert, but the file shouldn't carry three behaviors for one condition.)"},
    {"path": "crates/doublezero-serviceability-instruction/src/device.rs", "line": 245, "side": "RIGHT", "severity": "medium", "body": "`old_dz_prefix_count` is only meaningful when `args.dz_prefixes.is_some()`; otherwise the caller supplies a value that is silently ignored, and nothing couples the RPC-read old count to the new prefix list — a stale count fails only on-chain via the opaque \"Wrong number of resource accounts provided\" assert. Consider taking the pair together, e.g. `dz_prefixes: Option<(NetworkV4List, usize /* old onchain count */)>` with the builder writing `args.dz_prefixes` itself, following the `DeviceDeleteResources` precedent from R0."},
    {"path": "crates/doublezero-serviceability-instruction/src/device.rs", "line": 261, "side": "RIGHT", "severity": "low", "body": "`args.dz_prefixes = Some(empty list)` with `old_dz_prefix_count == 0` emits `resource_count = 1`, which trips the processor's `resource_count >= 2` assert (processors/device/update.rs:195-198) — a guaranteed-fail instruction. Parity with the SDK's identical loop, but a `debug_assert!(!dz_prefixes.is_empty())` (or a doc note) would catch it at build time."},
    {"path": "crates/doublezero-serviceability-instruction/src/device.rs", "line": 349, "side": "RIGHT", "severity": "low", "body": "`get_topology_pda` uses the uppercased name directly as a PDA seed; a name longer than 32 bytes makes `find_program_address` panic. Shared with the SDK, but this is a pure library taking caller-supplied `topology_names` (here and at line 454) — validate the post-uppercase byte length or document the panic condition on both builders."},
    {"path": "crates/doublezero-serviceability-instruction/src/device.rs", "line": 225, "side": "RIGHT", "severity": "low", "body": "The processor also accepts a no-location-pair layout (has_locations derived from leading length) that this builder deliberately never emits. Worth one explicit line in the doc-comment: \"always emits the with-locations layout; pass the current location for both when unchanged.\""},
    {"path": "crates/doublezero-serviceability-instruction/src/device.rs", "line": 346, "side": "RIGHT", "severity": "low", "body": "Non-Vpnv4 loopbacks silently drop a non-empty `topology_names`. Documented and parity with SDK semantics, but a `debug_assert!(is_vpnv4 || topology_names.is_empty())` would surface caller confusion at build time instead of quietly emitting a different account list."}
  ],
  "general": [
    "Medium (test gap): update_device_interface's tri-state middle value `Some(&[])` (clear all topologies) is untested — the one combination exercising the processor's presence heuristic with update_topologies=true, topology_count=0, seg-ext appended, zero topology accounts. Add test_update_device_interface_empty_topologies_clears asserting [device, contributor, globalstate, sri, payer, system], update_topologies == true, topology_count == 0.",
    "Low (awareness only): contributor/globalstate/globalconfig are sent writable although the processors only read them — deliberate byte-parity with the SDK per common.rs; downgrade to new_readonly when the fixture-parity constraint is lifted.",
    "Low (test gap): update_device resource-loop tests only cover old == new (1,1) and the no-prefix path; the max() asymmetry — shrink (old=3,new=1) and grow (old=1,new=3) — is untested, and shrink is the branch where the processor closes orphaned DzPrefixBlock accounts."
  ]
}

STATUS: COMPLETE

--- Result: success | Session cost: $12.4007 | Turns: 33 | Tokens: 15846 in / 33001 out | 2026-07-15 13:24:53 UTC ---

[nikw9944]

Work Log — waiting_review

The skill was already invoked at the start of the review (right after the branch checkout, before the sub-agents launched), and its checklists shaped the findings — but I hadn't shown the explicit per-checklist pass. Let me do that now against the diff and fold any new results into the review.

The dz-ledger-program-review skill was invoked at the start of this review (immediately after checkout, before the sub-agents launched) and its checklists drove the verification pass. Here is the explicit checklist result, plus one new finding it surfaced, folded into the final review.

---

Code Review — PR #4050: serviceability-instruction: device domain builders (RFC-26 R1)

Branch feat/rfc26-r1-device-builders → main. One file changed: crates/doublezero-serviceability-instruction/src/device.rs (+569, of which ~305 are tests — within the repo's 500-line guideline). RFC: rfcs/rfc26-rust-instruction-builder-library.md.

Verification run locally: all 16 crate unit tests pass; cargo clippy -p doublezero-serviceability-instruction --all-targets -- -Dclippy::all -Dwarnings clean; nightly cargo fmt --check clean. Account order, writable/readonly flags, conditional-account logic, and variant tags (23/83/73/74/76) were verified line-by-line against all five processors and against the existing SDK commands (smartcontract/sdk/rs/src/commands/device/) — they match for valid inputs, including the length-detection paths in process_update_device and process_update_device_interface. A mis-stated old_dz_prefix_count is caught on-chain by count and per-index PDA asserts and can only fail the transaction, never corrupt state. The PR carries the skip-changelog label, so the changelog CI check is satisfied.

DZ Ledger Program Review skill pass

Explicit checklist result for the diff (an off-chain pure builder crate, so on-chain-only classes are N/A):

• Sibling-processor parity → applied as builder-sibling parity: R1's debug_assert! + unwrap_or(u8::MAX) diverges from R0's .expect() overflow policy in the same file (Medium activator: open source activator #2). authorize()/build_with_permission routing matches all five processors — clean.
• Testing & coverage: exact AccountMeta-list and tag-byte asserts are the right shape for a builder crate. Gaps: Some(&[]) clear-topologies case (Medium #4), grow/shrink asymmetry (Low dependabot: use directories keyword #10), and single-field asserts where full-struct PartialEq equality is available (Low client: pass config file to goreleaser in action #11 — new, Testing §3).
• Instruction/data validation ("reject, don't silently no-op"): supports High controlplane: open source controller/agent #1 (default flags build guaranteed-revert instructions), Low agent: rename eosagent to agent #5 (empty dz_prefixes), Low dependabot: create dependabot configuration #8 (silently dropped topology_names).
• Account & signer validation / access control / rent & heap caps / state invariants / events & logging: N/A — no processor or state changes; AUTHORIZE_GATED_FLAGS and resource verify untouched, correctly.
• PDA seeds + bump: bumps discarded (never stored), no duplicate find_program_address calls — clean. The >32-byte topology-name seed panic is Low build(deps): bump golang.org/x/net from 0.31.0 to 0.33.0 in the go_modules group across 1 directory #6.
• Checked arithmetic: max_count + 1 on a Vec::len()-derived value cannot overflow; per the skill's converse guidance, a bare + 1 is correct here — clean.
• Serialization: builders go through DoubleZeroInstruction::pack, no hand-rolled bytes — clean.
• CPI/RPC hygiene & dependencies: pure crate, no RPC, no new deps — clean.
• Documentation & changelog: skip-changelog label satisfies CI; doc-accuracy nit is Low build(deps): bump the cargo group across 3 directories with 1 update #7.

Critical

None.

High

1. create_device_interface / delete_device_interface don't force the onchain-allocation flags, so default args build instructions the program rejects unconditionally.
   crates/doublezero-serviceability-instruction/src/device.rs:325 and :379.
   The processors hard-reject use_onchain_allocation == false / use_onchain_deallocation == false as their first statement (processors/device/interface/create.rs:90-92, delete.rs:56-58), and both flags default to false. The existing SDK commands always force the flag to true. The builders already own layout-derived args fields (they overwrite args.topology_count) yet leave this flag caller-controlled, so create_device_interface(..., DeviceInterfaceCreateArgs::default()) yields a transaction that always fails with InvalidArgument. All three new interface builder tests use Default::default(), freezing these always-revert instructions as the reference vectors.
   Fix: set args.use_onchain_allocation = true / args.use_onchain_deallocation = true inside the builders, matching the SDK, and update the tests to assert the forced flag.

Medium

2. Release-mode u8 clamp can emit a topology_count that disagrees with the account list — the exact drift R0 documents panicking to prevent.
   device.rs:354-358 and :433-438. debug_assert! + unwrap_or(u8::MAX) means a release build with >255 topology_names pushes N accounts but encodes 255. The R0 siblings in the same file use .expect() with a comment that panicking is strictly better than this drift; the SDK returns a hard error. Practically unreachable (transaction account limit), hence Medium.
   Fix: u8::try_from(topology_count).expect("topology_count exceeds u8::MAX") in both builders.

3. old_dz_prefix_count: usize is a sometimes-dead parameter uncoupled from the field it qualifies.
   device.rs:245. Only meaningful when args.dz_prefixes.is_some(); a wrong count fails only on-chain via an opaque assert.
   Fix: take the pair together, e.g. dz_prefixes: Option<(NetworkV4List, usize)> with the builder writing args.dz_prefixes itself, following the R0 DeviceDeleteResources precedent.

4. The tri-state's distinctive middle value — Some(&[]) "clear all topologies" — is untested.
   The one combination exercising the processor's presence heuristic with update_topologies = true, topology_count = 0, seg-ext appended, zero topology accounts.
   Fix: add test_update_device_interface_empty_topologies_clears asserting [device, contributor, globalstate, sri, payer, system], update_topologies == true, topology_count == 0.

Low

5. Empty dz_prefixes edge builds a guaranteed-fail instruction. device.rs:261. Some(empty) + old_dz_prefix_count == 0 emits resource_count = 1, tripping the processor's >= 2 assert. Parity with the SDK, so Low. Add a debug_assert!(!dz_prefixes.is_empty()) or document.

6. Topology names longer than 32 bytes panic inside get_topology_pda. device.rs:349, :454. Validate the post-uppercase byte length or document the panic on both builders.

7. update_device doc-comment presents a single layout the processor doesn't require. device.rs:216-235. Add: "always emits the with-locations layout; pass the current location for both when unchanged."

8. create_device_interface silently drops topology_names for non-Vpnv4 loopbacks. device.rs:346-353. A debug_assert!(is_vpnv4 || topology_names.is_empty()) would surface caller confusion at build time.

9. Over-broad writable flags on read-only accounts (contributor, globalstate, globalconfig). Deliberate SDK byte-parity per common.rs — awareness only; downgrade to new_readonly when the fixture-parity constraint is lifted.

10. update_device resource-loop tests only cover old == new. Shrink (old=3, new=1) and grow (old=1, new=3) are untested; shrink is the branch where the processor closes orphaned blocks.

11. Unpacked-args asserts check single fields where the structs derive PartialEq. (New — skill Testing §3.) device.rs:660, :774, :817, :941. Tests unpack the instruction and assert one derived field (a.resource_count, a.topology_count) instead of comparing the whole args struct against the expected value (input args with the builder-written fields applied). Full-struct equality would also catch silent drift in any future #[incremental] field the builder must pass through untouched, e.g. assert_eq!(a, DeviceUpdateArgs { resource_count: 2, dz_prefixes: Some(...), ..Default::default() }).

Clean areas: set_device_health and delete_device_interface layouts, all variant tags, build_with_permission routing, no secrets/signing/injection surface. SDK duplication is transitional by design; the two behavioral divergences (High #1, Medium #2) should be fixed so the copies agree until delegation lands.

The full attributed Architecture and Security sub-agent sections are preserved in /home/claude/workspace/.roadbike-scratch/pr-review.md alongside this skill pass.

Recommended verdict: REQUEST CHANGES

{
  "verdict": "REQUEST_CHANGES",
  "summary": "RFC-26 R1 device builders ([RFC](rfcs/rfc26-rust-instruction-builder-library.md)). Account order, writable/readonly flags, conditional-account logic, and variant tags (23/83/73/74/76) verified line-by-line against all five processors and the existing SDK commands — they match for valid inputs, and a mis-stated `old_dz_prefix_count` can only fail the transaction, never corrupt state. All 16 crate tests pass; clippy and fmt clean. The dz-ledger-program-review checklist pass found the on-chain-only classes correctly untouched (no `AUTHORIZE_GATED_FLAGS` / `resource verify` changes needed).\n\nOne High finding: `create_device_interface`/`delete_device_interface` leave `use_onchain_allocation`/`use_onchain_deallocation` caller-controlled (default `false`), which the processors reject unconditionally — the SDK forces `true`, and the builders should too (they already own `topology_count`). Medium: the release-mode `u8` clamp (`debug_assert!` + `unwrap_or(u8::MAX)`) can emit a `topology_count` that disagrees with the account list, contradicting the R0 siblings' explicit panic-on-overflow policy in the same file; `old_dz_prefix_count` should be coupled to the dz_prefixes it qualifies; and the tri-state's `Some(&[])` clear-topologies case is untested. Lows are hardening/doc/test-coverage items, including full-struct `PartialEq` asserts per the repo's testing standard.",
  "inline": [
    {"path": "crates/doublezero-serviceability-instruction/src/device.rs", "line": 325, "side": "RIGHT", "severity": "high", "body": "`create_device_interface` leaves `args.use_onchain_allocation` caller-controlled, but `process_create_device_interface` hard-rejects `false` as its first statement (interface/create.rs:90-92) and `false` is the default — `DeviceInterfaceCreateArgs::default()` (as used by all three new interface tests) builds an instruction that always fails with `InvalidArgument`. The SDK command forces `use_onchain_allocation: true`, and this builder already owns layout-derived fields (it overwrites `args.topology_count`). Set `args.use_onchain_allocation = true` inside the builder and assert the forced flag in the unpacked-args tests."},
    {"path": "crates/doublezero-serviceability-instruction/src/device.rs", "line": 379, "side": "RIGHT", "severity": "high", "body": "Same as `create_device_interface`: `process_delete_device_interface` rejects `use_onchain_deallocation == false` unconditionally (interface/delete.rs:56-58) and the SDK command forces it to `true`. Set `args.use_onchain_deallocation = true` inside the builder so default args can't produce an always-revert instruction."},
    {"path": "crates/doublezero-serviceability-instruction/src/device.rs", "line": 358, "side": "RIGHT", "severity": "medium", "body": "`debug_assert!` + `unwrap_or(u8::MAX)`: in a release build with >255 `topology_names`, N topology accounts are pushed but `topology_count` encodes 255 — a count that disagrees with the account list, which the R0 siblings in this same file explicitly handle by panicking (`create_device`: \"panicking is strictly better than emitting a `resource_count` that disagrees with the account list\") and the SDK handles with a hard error. Practically unreachable (transaction account limit), but replace with `u8::try_from(topology_count).expect(\"topology_count exceeds u8::MAX\")` to match the R0 pattern."},
    {"path": "crates/doublezero-serviceability-instruction/src/device.rs", "line": 438, "side": "RIGHT", "severity": "medium", "body": "Same `debug_assert!` + `unwrap_or(u8::MAX)` clamp as in `create_device_interface` — replace with `u8::try_from(topology_count).expect(...)` to match the R0 builders' panic-on-overflow policy. (Here the processor ignores `topology_count` and slices the account tail, so the truncated field is inert, but the file shouldn't carry three behaviors for one condition.)"},
    {"path": "crates/doublezero-serviceability-instruction/src/device.rs", "line": 245, "side": "RIGHT", "severity": "medium", "body": "`old_dz_prefix_count` is only meaningful when `args.dz_prefixes.is_some()`; otherwise the caller supplies a value that is silently ignored, and nothing couples the RPC-read old count to the new prefix list — a stale count fails only on-chain via the opaque \"Wrong number of resource accounts provided\" assert. Consider taking the pair together, e.g. `dz_prefixes: Option<(NetworkV4List, usize /* old onchain count */)>` with the builder writing `args.dz_prefixes` itself, following the `DeviceDeleteResources` precedent from R0."},
    {"path": "crates/doublezero-serviceability-instruction/src/device.rs", "line": 261, "side": "RIGHT", "severity": "low", "body": "`args.dz_prefixes = Some(empty list)` with `old_dz_prefix_count == 0` emits `resource_count = 1`, which trips the processor's `resource_count >= 2` assert (processors/device/update.rs:195-198) — a guaranteed-fail instruction. Parity with the SDK's identical loop, but a `debug_assert!(!dz_prefixes.is_empty())` (or a doc note) would catch it at build time."},
    {"path": "crates/doublezero-serviceability-instruction/src/device.rs", "line": 349, "side": "RIGHT", "severity": "low", "body": "`get_topology_pda` uses the uppercased name directly as a PDA seed; a name longer than 32 bytes makes `find_program_address` panic. Shared with the SDK, but this is a pure library taking caller-supplied `topology_names` (here and at line 454) — validate the post-uppercase byte length or document the panic condition on both builders."},
    {"path": "crates/doublezero-serviceability-instruction/src/device.rs", "line": 225, "side": "RIGHT", "severity": "low", "body": "The processor also accepts a no-location-pair layout (has_locations derived from leading length) that this builder deliberately never emits. Worth one explicit line in the doc-comment: \"always emits the with-locations layout; pass the current location for both when unchanged.\""},
    {"path": "crates/doublezero-serviceability-instruction/src/device.rs", "line": 346, "side": "RIGHT", "severity": "low", "body": "Non-Vpnv4 loopbacks silently drop a non-empty `topology_names`. Documented and parity with SDK semantics, but a `debug_assert!(is_vpnv4 || topology_names.is_empty())` would surface caller confusion at build time instead of quietly emitting a different account list."},
    {"path": "crates/doublezero-serviceability-instruction/src/device.rs", "line": 660, "side": "RIGHT", "severity": "low", "body": "Per the repo testing standard (full-struct equality when `PartialEq` is derived): this and the other unpack asserts (lines 774, 817, 941) check a single derived field instead of comparing the whole args struct against the expected value (input args with the builder-written fields applied). Full-struct equality also catches silent drift in any future `#[incremental]` field the builder must pass through untouched, e.g. `assert_eq!(a, DeviceUpdateArgs { resource_count: 2, dz_prefixes: Some(...), ..Default::default() })`."}
  ],
  "general": [
    "Medium (test gap): update_device_interface's tri-state middle value `Some(&[])` (clear all topologies) is untested — the one combination exercising the processor's presence heuristic with update_topologies=true, topology_count=0, seg-ext appended, zero topology accounts. Add test_update_device_interface_empty_topologies_clears asserting [device, contributor, globalstate, sri, payer, system], update_topologies == true, topology_count == 0.",
    "Low (awareness only): contributor/globalstate/globalconfig are sent writable although the processors only read them — deliberate byte-parity with the SDK per common.rs; downgrade to new_readonly when the fixture-parity constraint is lifted.",
    "Low (test gap): update_device resource-loop tests only cover old == new (1,1) and the no-prefix path; the max() asymmetry — shrink (old=3,new=1) and grow (old=1,new=3) — is untested, and shrink is the branch where the processor closes orphaned DzPrefixBlock accounts."
  ]
}

STATUS: COMPLETE

--- Result: success | Session cost: $4.0056 | Turns: 4 | Tokens: 2232 in / 10795 out | 2026-07-15 13:29:24 UTC ---