From 8250ae01c85a656881671f48d42e12ee0adf9615 Mon Sep 17 00:00:00 2001 From: "Brian R. Jackson" Date: Sun, 26 Jul 2026 15:35:23 -0400 Subject: [PATCH 01/23] feat(errors): implement the published conflict error kind unifi schema has advertised {kind: conflict, exit_code: 6} since the clispec v0.2 upgrade, but nothing emitted it and no exit-code constant existed. Adds ApiError::Conflict and wires it into both error mappers. --- src/api/types.rs | 10 +++++++++- src/output.rs | 17 +++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/src/api/types.rs b/src/api/types.rs index 5a66812..00ce76f 100644 --- a/src/api/types.rs +++ b/src/api/types.rs @@ -485,9 +485,16 @@ pub type RtspsStreams = std::collections::HashMap>; #[derive(Debug)] pub enum ApiError { Http(reqwest::Error), - Api { status: u16, message: String }, + Api { + status: u16, + message: String, + }, NotFound(String), Auth(String), + /// A request that cannot succeed against the resource's current state, + /// rejected locally before any HTTP call. Published by `unifi schema` + /// as kind `conflict`, exit code 6. + Conflict(String), Other(String), } @@ -570,6 +577,7 @@ impl fmt::Display for ApiError { "\n Hint: Check your API key. Generate one in UniFi Settings > API" ) } + ApiError::Conflict(msg) => write!(f, "{msg}"), ApiError::Other(msg) => write!(f, "{msg}"), } } diff --git a/src/output.rs b/src/output.rs index ebe69fb..13a33bc 100644 --- a/src/output.rs +++ b/src/output.rs @@ -104,6 +104,7 @@ pub mod exit_codes { pub const AUTH_ERROR: i32 = 3; pub const NOT_FOUND: i32 = 4; pub const API_ERROR: i32 = 5; + pub const CONFLICT: i32 = 6; } /// Map an error to a specific exit code by downcasting to ApiError. @@ -113,6 +114,7 @@ pub fn exit_code_for_error(err: &(dyn std::error::Error + 'static)) -> i32 { crate::api::ApiError::Auth(_) => exit_codes::AUTH_ERROR, crate::api::ApiError::NotFound(_) => exit_codes::NOT_FOUND, crate::api::ApiError::Api { .. } => exit_codes::API_ERROR, + crate::api::ApiError::Conflict(_) => exit_codes::CONFLICT, crate::api::ApiError::Http(_) | crate::api::ApiError::Other(_) => { exit_codes::GENERAL_ERROR } @@ -129,6 +131,7 @@ pub fn error_kind_and_code(err: &(dyn std::error::Error + 'static)) -> (&'static crate::api::ApiError::Auth(_) => ("auth_error", exit_codes::AUTH_ERROR), crate::api::ApiError::NotFound(_) => ("not_found", exit_codes::NOT_FOUND), crate::api::ApiError::Api { .. } => ("api_error", exit_codes::API_ERROR), + crate::api::ApiError::Conflict(_) => ("conflict", exit_codes::CONFLICT), crate::api::ApiError::Http(_) | crate::api::ApiError::Other(_) => { ("general_error", exit_codes::GENERAL_ERROR) } @@ -215,4 +218,18 @@ mod tests { assert!(envelope["error"]["kind"].as_str().is_some()); assert!(envelope["error"]["message"].as_str().is_some()); } + + #[test] + fn exit_code_for_conflict() { + let err = ApiError::Conflict("port has no PoE".into()); + assert_eq!(exit_code_for_error(&err), exit_codes::CONFLICT); + } + + #[test] + fn error_kind_and_code_conflict() { + let err = ApiError::Conflict("port has no PoE".into()); + let (kind, code) = error_kind_and_code(&err); + assert_eq!(kind, "conflict"); + assert_eq!(code, 6); + } } From ee857fc58269edf0f7cb58d55b8c794d66d62f8d Mon Sep 17 00:00:00 2001 From: "Brian R. Jackson" Date: Sun, 26 Jul 2026 15:41:17 -0400 Subject: [PATCH 02/23] feat(api): model PoE telemetry and last_connection on PortEntry The controller already returns poe_mode, poe_class, poe_voltage, poe_current, poe_good and last_connection; all were discarded. Voltage and current reuse the string-or-number deserializer, since the same firmware that stringifies poe_power stringifies these too. --- src/api/mod.rs | 6 ++--- src/api/tests.rs | 62 ++++++++++++++++++++++++++++++++++++++++++++++++ src/api/types.rs | 28 ++++++++++++++++++++++ 3 files changed, 93 insertions(+), 3 deletions(-) diff --git a/src/api/mod.rs b/src/api/mod.rs index 580cd16..657cfc8 100644 --- a/src/api/mod.rs +++ b/src/api/mod.rs @@ -6,7 +6,7 @@ pub use client::ProtectSession; pub use client::UnifiClient; pub use client::error_for_status; pub use types::{ - ApiError, Client, Device, DeviceWithPorts, Event, HealthSubsystem, HostSystem, LegacyClient, - LegacyDevice, LegacyResponse, Network, PortEntry, SysInfo, format_bytes, format_mac, - format_uptime, normalize_mac, + ApiError, Client, Device, DeviceWithPorts, Event, HealthSubsystem, HostSystem, LastConnection, + LegacyClient, LegacyDevice, LegacyResponse, Network, PortEntry, SysInfo, format_bytes, + format_mac, format_uptime, normalize_mac, }; diff --git a/src/api/tests.rs b/src/api/tests.rs index 27b4ced..86b0405 100644 --- a/src/api/tests.rs +++ b/src/api/tests.rs @@ -812,3 +812,65 @@ fn legacy_client_clean_name_no_mac() { let client: LegacyClient = serde_json::from_str(json).unwrap(); assert_eq!(client.clean_name(), "device ee:ff"); } + +// --- PortEntry PoE telemetry --- + +#[test] +fn port_entry_parses_poe_telemetry_with_string_numbers() { + // The controller returns poe_voltage/poe_current/poe_power as JSON + // strings on some firmware, the same quirk PR #3 fixed for poe_power. + let json = serde_json::json!({ + "port_idx": 3, + "name": "Port 3", + "up": true, + "port_poe": true, + "poe_enable": true, + "poe_mode": "auto", + "poe_class": "Class 3", + "poe_power": "5.00", + "poe_voltage": "53.75", + "poe_current": "93.00", + "poe_good": true, + "autoneg": true, + "enable": true, + "is_uplink": false, + "stp_state": "forwarding", + "tx_errors": 0, + "rx_errors": 0, + "last_connection": { + "mac": "f4:e2:c6:65:47:6c", + "connected": true, + "last_seen": 1783622695 + } + }); + let p: PortEntry = serde_json::from_value(json).expect("PortEntry must parse"); + assert_eq!(p.poe_mode.as_deref(), Some("auto")); + assert_eq!(p.poe_class.as_deref(), Some("Class 3")); + assert_eq!(p.poe_voltage, Some(53.75)); + assert_eq!(p.poe_current, Some(93.00)); + assert_eq!(p.poe_good, Some(true)); + assert_eq!(p.stp_state.as_deref(), Some("forwarding")); + assert!(p.autoneg); + assert!(!p.is_uplink); + let lc = p.last_connection.expect("last_connection present"); + assert_eq!(lc.mac.as_deref(), Some("f4:e2:c6:65:47:6c")); + assert_eq!(lc.connected, Some(true)); +} + +#[test] +fn port_entry_tolerates_absent_last_connection() { + // A port nothing has ever linked to omits last_connection entirely. + // This is exactly how the empty test-target port was identified. + let json = serde_json::json!({ + "port_idx": 4, + "name": "Port 4", + "up": false, + "port_poe": true, + "poe_enable": false, + "poe_mode": "auto" + }); + let p: PortEntry = serde_json::from_value(json).expect("PortEntry must parse"); + assert!(p.last_connection.is_none()); + assert_eq!(p.poe_voltage, None); + assert!(!p.up); +} diff --git a/src/api/types.rs b/src/api/types.rs index 00ce76f..5ef86fa 100644 --- a/src/api/types.rs +++ b/src/api/types.rs @@ -302,10 +302,38 @@ pub struct PortEntry { pub poe_power: Option, #[serde(default)] pub port_poe: bool, + /// "auto", "off", "passthrough", "passive24v". Absent on some firmware. + pub poe_mode: Option, + pub poe_class: Option, + #[serde(default, deserialize_with = "deserialize_string_or_number_f64")] + pub poe_voltage: Option, + #[serde(default, deserialize_with = "deserialize_string_or_number_f64")] + pub poe_current: Option, + pub poe_good: Option, + #[serde(default)] + pub autoneg: bool, + #[serde(default)] + pub enable: bool, + #[serde(default)] + pub is_uplink: bool, + pub stp_state: Option, + pub tx_errors: Option, + pub rx_errors: Option, + /// Absent entirely on a port nothing has linked to within retention. + pub last_connection: Option, pub tx_bytes: Option, pub rx_bytes: Option, } +/// The device most recently seen on a port. `connected` distinguishes a live +/// attachment from a stale record of a device that has since moved. +#[derive(Debug, Deserialize)] +pub struct LastConnection { + pub mac: Option, + pub connected: Option, + pub last_seen: Option, +} + fn deserialize_string_or_number_f64<'de, D>(deserializer: D) -> Result, D::Error> where D: serde::Deserializer<'de>, From 35e300ebf03574c17b7600590930c54fcd3ca4d9 Mon Sep 17 00:00:00 2001 From: "Brian R. Jackson" Date: Sun, 26 Jul 2026 16:07:56 -0400 Subject: [PATCH 03/23] refactor: make enable/autoneg/is_uplink tri-state in PortEntry Change three fields from defaulted bool to Option to distinguish "firmware didn't send the field" from "field is false". This prevents misreporting a port as administratively disabled when the controller simply omitted the key. Matches the existing tri-state pattern used by poe_good; contrasts with up/poe_enable/port_poe, where absent genuinely means false. Fixes review finding on Task 2: serde(default) on bool conflates missing keys with false, which is incorrect for enable and affects operator visibility in ports show. Co-Authored-By: Claude Opus 5 --- src/api/tests.rs | 9 +++++++-- src/api/types.rs | 13 +++++++------ 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/src/api/tests.rs b/src/api/tests.rs index 86b0405..ec9b8bb 100644 --- a/src/api/tests.rs +++ b/src/api/tests.rs @@ -850,8 +850,9 @@ fn port_entry_parses_poe_telemetry_with_string_numbers() { assert_eq!(p.poe_current, Some(93.00)); assert_eq!(p.poe_good, Some(true)); assert_eq!(p.stp_state.as_deref(), Some("forwarding")); - assert!(p.autoneg); - assert!(!p.is_uplink); + assert_eq!(p.autoneg, Some(true)); + assert_eq!(p.enable, Some(true)); + assert_eq!(p.is_uplink, Some(false)); let lc = p.last_connection.expect("last_connection present"); assert_eq!(lc.mac.as_deref(), Some("f4:e2:c6:65:47:6c")); assert_eq!(lc.connected, Some(true)); @@ -873,4 +874,8 @@ fn port_entry_tolerates_absent_last_connection() { assert!(p.last_connection.is_none()); assert_eq!(p.poe_voltage, None); assert!(!p.up); + // Absent tri-state keys must read as unknown, never as a confident "no". + assert_eq!(p.enable, None, "absent enable must not read as disabled"); + assert_eq!(p.autoneg, None); + assert_eq!(p.is_uplink, None); } diff --git a/src/api/types.rs b/src/api/types.rs index 5ef86fa..ad6d6f8 100644 --- a/src/api/types.rs +++ b/src/api/types.rs @@ -310,12 +310,13 @@ pub struct PortEntry { #[serde(default, deserialize_with = "deserialize_string_or_number_f64")] pub poe_current: Option, pub poe_good: Option, - #[serde(default)] - pub autoneg: bool, - #[serde(default)] - pub enable: bool, - #[serde(default)] - pub is_uplink: bool, + /// Option, not a defaulted bool: a firmware that omits these keys must not + /// be reported as "auto-negotiation off" or "port administratively + /// disabled". Matches `poe_good` above; contrast `up`/`poe_enable`, where + /// an absent key genuinely does mean false. + pub autoneg: Option, + pub enable: Option, + pub is_uplink: Option, pub stp_state: Option, pub tx_errors: Option, pub rx_errors: Option, From 324cb71482a75512e5ebd43e5382d47d04916f80 Mon Sep 17 00:00:00 2001 From: "Brian R. Jackson" Date: Sun, 26 Jul 2026 16:15:13 -0400 Subject: [PATCH 04/23] feat(api): add power_cycle_port and list_all_device_ports power_cycle_port posts cmd=power-cycle to devmgr with the switch MAC and port_idx. The mock test asserts the exact body so the port_idx key and MAC normalisation are both pinned. --- src/api/client.rs | 19 ++++++++++++++++ tests/mock_server.rs | 52 +++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 70 insertions(+), 1 deletion(-) diff --git a/src/api/client.rs b/src/api/client.rs index 6ab5605..c8709b8 100644 --- a/src/api/client.rs +++ b/src/api/client.rs @@ -339,6 +339,18 @@ impl UnifiClient { Ok(()) } + /// Power-cycle a single PoE port. `mac` is the **switch's** MAC, not the + /// attached device's. + pub async fn power_cycle_port(&self, mac: &str, port_idx: u32) -> Result<(), ApiError> { + let formatted = format_mac(&normalize_mac(mac)); + self.post_legacy_cmd( + "devmgr", + serde_json::json!({"cmd": "power-cycle", "mac": formatted, "port_idx": port_idx}), + ) + .await?; + Ok(()) + } + pub async fn upgrade_device(&self, mac: &str) -> Result<(), ApiError> { let formatted = format_mac(&normalize_mac(mac)); self.post_legacy_cmd( @@ -407,6 +419,13 @@ impl UnifiClient { .ok_or_else(|| ApiError::NotFound(format!("Device with MAC {mac}"))) } + /// Every device that reports a port table, in one request. `/stat/device` + /// already returns all devices with their port tables, so the unfiltered + /// listing costs no more than the filtered one. + pub async fn list_all_device_ports(&self) -> Result, ApiError> { + self.get_legacy("/stat/device").await + } + // All clients with bandwidth data (legacy endpoint for richer stats) pub async fn list_clients_legacy(&self) -> Result, ApiError> { self.get_legacy("/stat/sta").await diff --git a/tests/mock_server.rs b/tests/mock_server.rs index 7b9ca44..f661d28 100644 --- a/tests/mock_server.rs +++ b/tests/mock_server.rs @@ -1,4 +1,4 @@ -use wiremock::matchers::{method, path, path_regex}; +use wiremock::matchers::{body_json, method, path, path_regex}; use wiremock::{Mock, MockServer, ResponseTemplate}; // Helper to create a UnifiClient pointing at the mock server @@ -342,6 +342,32 @@ mod client_api { client.restart_device("aa:bb:cc:dd:ee:ff").await.unwrap(); } + #[tokio::test] + async fn power_cycle_port_sends_correct_command() { + let server = MockServer::start().await; + + Mock::given(method("POST")) + .and(path("/proxy/network/api/s/default/cmd/devmgr")) + .and(body_json(serde_json::json!({ + "cmd": "power-cycle", + "mac": "aa:bb:cc:dd:ee:ff", + "port_idx": 5 + }))) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "meta": {"rc": "ok"}, + "data": [] + }))) + .expect(1) + .mount(&server) + .await; + + let client = mock_client(&server).await; + client + .power_cycle_port("AA-BB-CC-DD-EE-FF", 5) + .await + .unwrap(); + } + #[tokio::test] async fn upgrade_device_sends_correct_command() { let server = MockServer::start().await; @@ -496,6 +522,30 @@ mod client_api { let err = client.get_sysinfo().await.unwrap_err(); assert!(err.to_string().contains("No sysinfo returned")); } + + #[tokio::test] + async fn list_all_device_ports_returns_every_device() { + let server = MockServer::start().await; + + Mock::given(method("GET")) + .and(path("/proxy/network/api/s/default/stat/device")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "meta": {"rc": "ok"}, + "data": [ + {"mac": "aa:bb:cc:dd:ee:ff", "name": "SwitchA", + "port_table": [{"port_idx": 1, "port_poe": true}]}, + {"mac": "11:22:33:44:55:66", "name": "SwitchB", + "port_table": [{"port_idx": 1}, {"port_idx": 2}]} + ] + }))) + .mount(&server) + .await; + + let client = mock_client(&server).await; + let devices = client.list_all_device_ports().await.unwrap(); + assert_eq!(devices.len(), 2); + assert_eq!(devices[1].port_table.len(), 2); + } } // --- Error handling tests --- From 10f9e3eb89465921bb1b60ca8155aa6b96a50b1f Mon Sep 17 00:00:00 2001 From: "Brian R. Jackson" Date: Sun, 26 Jul 2026 16:27:18 -0400 Subject: [PATCH 05/23] feat(ports): add top-level ports list Lists one device's ports or every device's, in a single /stat/device request either way. Rows carry device_mac/device_name so the unfiltered listing is meaningful; the filtered table renders exactly as before. --- src/commands/mod.rs | 1 + src/commands/ports.rs | 201 ++++++++++++++++++++++++++++++++++++++++++ src/fields.rs | 16 ++++ src/main.rs | 79 +++++++++++++++++ src/schema.rs | 3 + tests/cli_contract.rs | 35 ++++++++ 6 files changed, 335 insertions(+) create mode 100644 src/commands/ports.rs diff --git a/src/commands/mod.rs b/src/commands/mod.rs index 78d0572..c01c087 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -2,5 +2,6 @@ pub mod clients; pub mod devices; pub mod events; pub mod networks; +pub mod ports; pub mod protect; pub mod system; diff --git a/src/commands/ports.rs b/src/commands/ports.rs new file mode 100644 index 0000000..f510df1 --- /dev/null +++ b/src/commands/ports.rs @@ -0,0 +1,201 @@ +use owo_colors::OwoColorize; + +use crate::api::{DeviceWithPorts, PortEntry, UnifiClient, format_bytes, format_mac}; +use crate::output::{OutputConfig, use_color}; + +/// One port, flattened with the device that owns it. Every `ports` subcommand +/// renders these, so the filtered and unfiltered listings cannot drift apart. +pub struct PortRow<'a> { + pub device_mac: String, + pub device_name: String, + pub port: &'a PortEntry, +} + +pub struct Pagination { + pub limit: usize, + pub offset: usize, + /// Field names already validated against `fields::PORTS_LIST`. + pub fields: Option>, +} + +/// Flatten devices into port rows, skipping devices with no port table. +pub fn collect_rows(devices: &[DeviceWithPorts]) -> Vec> { + let mut rows = Vec::new(); + for d in devices { + if d.port_table.is_empty() { + continue; + } + let device_mac = d + .mac + .as_deref() + .map(format_mac) + .unwrap_or_else(|| "-".into()); + let device_name = d + .name + .as_deref() + .or(d.model.as_deref()) + .unwrap_or("-") + .to_string(); + for port in &d.port_table { + rows.push(PortRow { + device_mac: device_mac.clone(), + device_name: device_name.clone(), + port, + }); + } + } + rows +} + +/// The `PORTS_LIST` field set for one row. +pub fn row_json(row: &PortRow) -> serde_json::Value { + let p = row.port; + serde_json::json!({ + "device_mac": row.device_mac, + "device_name": row.device_name, + "port_idx": p.port_idx, + "name": p.name, + "media": p.media, + "up": p.up, + "speed": p.speed, + "full_duplex": p.full_duplex, + "poe_enable": p.poe_enable, + "poe_power": p.poe_power, + "port_poe": p.port_poe, + "tx_bytes": p.tx_bytes, + "rx_bytes": p.rx_bytes, + }) +} + +/// Apply a validated `--fields` projection in place. +pub fn project(value: &mut serde_json::Value, fields: &Option>) { + if let Some(keep) = fields + && let Some(map) = value.as_object_mut() + { + map.retain(|k, _| keep.iter().any(|f| f == k)); + } +} + +/// Human-readable PoE cell: draw in watts, or on/off/- . +fn poe_cell(p: &PortEntry) -> String { + if p.poe_enable { + match p.poe_power { + Some(w) if w > 0.0 => format!("{w:.1}W"), + _ => "on".into(), + } + } else if p.port_poe { + "off".into() + } else { + "-".into() + } +} + +fn speed_cell(p: &PortEntry) -> String { + if !p.up { + return "down".into(); + } + match p.speed { + Some(s) => format!("{s}{}", if p.full_duplex { "FD" } else { "HD" }), + None => "up".into(), + } +} + +/// Render rows as a table. `show_device_col` is true only for the unfiltered +/// listing; the filtered table stays byte-identical to what `devices ports` +/// has always printed. +pub fn render_text(rows: &[&PortRow], show_device_col: bool, out: &OutputConfig) { + let color = use_color(); + let dev_w = rows + .iter() + .map(|r| r.device_name.len()) + .max() + .unwrap_or(6) + .max(6) + + 2; + + let header = if show_device_col { + format!( + "{:10} {:>10}", + "Device", "Port", "Name", "Link", "Speed", "PoE", "TX", "RX" + ) + } else { + format!( + "{:<6} {:<16} {:<6} {:<10} {:<8} {:>10} {:>10}", + "Port", "Name", "Link", "Speed", "PoE", "TX", "RX" + ) + }; + let rule_w = if show_device_col { 70 + dev_w } else { 70 }; + if color { + println!("{}", header.bold()); + println!("{}", "-".repeat(rule_w).dimmed()); + } else { + println!("{header}"); + println!("{}", "-".repeat(rule_w)); + } + + for r in rows { + let p = r.port; + let port = p + .port_idx + .map(|i| i.to_string()) + .unwrap_or_else(|| "-".into()); + let name = p.name.as_deref().unwrap_or("-"); + let link = if p.up { "up" } else { "down" }; + let speed = speed_cell(p); + let poe = poe_cell(p); + let tx = p.tx_bytes.map(format_bytes).unwrap_or_else(|| "-".into()); + let rx = p.rx_bytes.map(format_bytes).unwrap_or_else(|| "-".into()); + + if show_device_col { + println!( + " {:10} {:>10}", + r.device_name, port, name, link, speed, poe, tx, rx + ); + } else { + println!( + " {:<5} {:<16} {:<6} {:<10} {:<8} {:>10} {:>10}", + port, name, link, speed, poe, tx, rx + ); + } + } + out.print_message(&format!("\n{} ports", rows.len())); +} + +pub async fn list( + client: &UnifiClient, + mac: Option<&str>, + out: OutputConfig, + pagination: Pagination, +) -> Result<(), Box> { + let devices = match mac { + Some(m) => vec![client.get_device_ports(m).await?], + None => client.list_all_device_ports().await?, + }; + let rows = collect_rows(&devices); + let total = rows.len(); + let page: Vec<&PortRow> = rows + .iter() + .skip(pagination.offset) + .take(pagination.limit) + .collect(); + + if out.is_json() { + let items: Vec = page + .iter() + .map(|r| { + let mut v = row_json(r); + project(&mut v, &pagination.fields); + v + }) + .collect(); + out.print_data(&serde_json::to_string_pretty(&serde_json::json!({ + "items": items, + "total": total, + "limit": pagination.limit, + "offset": pagination.offset, + }))?); + } else { + render_text(&page, mac.is_none(), &out); + } + Ok(()) +} diff --git a/src/fields.rs b/src/fields.rs index a54aeb3..85deaab 100644 --- a/src/fields.rs +++ b/src/fields.rs @@ -67,6 +67,22 @@ pub const NETWORKS_LIST: &[Field] = &[ ("default", "boolean"), ]; +pub const PORTS_LIST: &[Field] = &[ + ("device_mac", "string"), + ("device_name", "string"), + ("port_idx", "integer"), + ("name", "string"), + ("media", "string"), + ("up", "boolean"), + ("speed", "integer"), + ("full_duplex", "boolean"), + ("poe_enable", "boolean"), + ("poe_power", "number"), + ("port_poe", "boolean"), + ("tx_bytes", "integer"), + ("rx_bytes", "integer"), +]; + /// A `--fields` request naming one or more unknown fields. #[derive(Debug, PartialEq, Eq)] pub struct InvalidFields { diff --git a/src/main.rs b/src/main.rs index 677484b..7dc7da2 100644 --- a/src/main.rs +++ b/src/main.rs @@ -75,6 +75,10 @@ enum Command { command: Option, }, + /// Inspect and manage switch ports + #[command(subcommand)] + Ports(PortsCommand), + /// View controller events #[command(subcommand)] Events(EventsCommand), @@ -229,6 +233,30 @@ enum DevicesCommand { }, } +#[derive(Subcommand)] +enum PortsCommand { + /// List ports for one device, or across all devices + List { + /// MAC address of a switch or router. Omit to list every device's ports. + mac: Option, + /// Maximum number of results to return + #[arg(long, default_value = "100")] + limit: usize, + /// Number of results to skip + #[arg(long, default_value = "0")] + offset: usize, + /// Comma-separated list of fields to include in output (see `unifi schema`) + #[arg(long)] + fields: Option, + /// Live-updating TUI view of port status (requires MAC) + #[arg(long, requires = "mac")] + live: bool, + /// Refresh interval in seconds (only with --live) + #[arg(short = 'i', long, default_value = "2")] + interval: u64, + }, +} + #[derive(Subcommand)] enum ConfigCommand { /// Create or update the configuration file interactively @@ -346,6 +374,7 @@ fn validate_requested_fields(command: &Command) -> Result>, I Command::Clients(ClientsCommand::List { fields, .. }) => (fields, fields::CLIENTS_LIST), Command::Devices(DevicesCommand::List { fields, .. }) => (fields, fields::DEVICES_LIST), Command::Events(EventsCommand::List { fields, .. }) => (fields, fields::EVENTS_LIST), + Command::Ports(PortsCommand::List { fields, .. }) => (fields, fields::PORTS_LIST), _ => return Ok(None), }; @@ -1268,6 +1297,28 @@ async fn main() { } }, Command::Networks { .. } => commands::networks::list(&mut client, out).await, + Command::Ports(cmd) => match cmd { + PortsCommand::List { + mac, + limit, + offset, + fields: _, + live, + interval, + } => { + if live { + let mac = mac.expect("clap requires --live to be paired with a MAC"); + unifi_cli::tui::run_ports(&client, &mac, interval).await + } else { + let pagination = commands::ports::Pagination { + limit, + offset, + fields: requested_fields, + }; + commands::ports::list(&client, mac.as_deref(), out, pagination).await + } + } + }, Command::Events(cmd) => match cmd { EventsCommand::List { limit, @@ -2489,6 +2540,34 @@ api_key = "work_key" } } + #[test] + fn cli_parses_ports_list_without_mac() { + let cli = Cli::parse_from(["unifi", "ports", "list"]); + match cli.command { + Command::Ports(PortsCommand::List { mac, limit, .. }) => { + assert!(mac.is_none()); + assert_eq!(limit, 100); + } + _ => panic!("expected Ports List"), + } + } + + #[test] + fn cli_parses_ports_list_with_mac() { + let cli = Cli::parse_from(["unifi", "ports", "list", "aa:bb:cc:dd:ee:ff"]); + match cli.command { + Command::Ports(PortsCommand::List { mac, .. }) => { + assert_eq!(mac.as_deref(), Some("aa:bb:cc:dd:ee:ff")); + } + _ => panic!("expected Ports List"), + } + } + + #[test] + fn cli_rejects_ports_live_without_mac() { + assert!(Cli::try_parse_from(["unifi", "ports", "list", "--live"]).is_err()); + } + #[test] fn cli_devices_upgrade() { let cli = parse(&[ diff --git a/src/schema.rs b/src/schema.rs index 7b982ef..ca62849 100644 --- a/src/schema.rs +++ b/src/schema.rs @@ -186,6 +186,9 @@ fn command_metadata() -> HashMap<&'static str, CommandMeta> { ), ); + // ports + m.insert("ports list", f(fields::PORTS_LIST, false, None)); + // networks / events / system m.insert("networks list", f(fields::NETWORKS_LIST, false, None)); m.insert("events list", f(fields::EVENTS_LIST, false, None)); diff --git a/tests/cli_contract.rs b/tests/cli_contract.rs index b83716a..c5617c5 100644 --- a/tests/cli_contract.rs +++ b/tests/cli_contract.rs @@ -159,6 +159,41 @@ fn clients_list_accepts_every_documented_field() { } } +// --- `ports` surface --- + +#[test] +fn ports_list_rejects_unknown_field() { + let out = unifi() + .args(["ports", "list", "--fields", "bogus"]) + .output() + .expect("failed to run binary"); + + assert_eq!( + out.status.code(), + Some(2), + "expected usage exit code 2, stderr: {}", + String::from_utf8_lossy(&out.stderr) + ); + assert_eq!( + error_envelope(&out.stderr)["error"]["kind"].as_str(), + Some("config_error") + ); +} + +#[test] +fn ports_live_requires_a_mac() { + let out = unifi() + .args(["ports", "list", "--live"]) + .output() + .expect("failed to run binary"); + + assert_eq!( + out.status.code(), + Some(2), + "--live without a MAC must be a usage error" + ); +} + // --- subcommand surface consistency --- #[test] From 45fb4cc657d06aaeaf5926c75c4283fa93f58201 Mon Sep 17 00:00:00 2001 From: "Brian R. Jackson" Date: Sun, 26 Jul 2026 16:48:38 -0400 Subject: [PATCH 06/23] fix(ports): restore link colouring, guard PORTS_LIST, add functional tests - render_text now colours the Link column (green up / dimmed down) in both the filtered and unfiltered listings, matching devices::ports exactly so the upcoming `devices ports ` alias renders unchanged. - Add PORTS_LIST to fields.rs's every_table_has_unique_field_names and every_field_declares_a_json_type self-checks, and allow "number" as a valid field type (poe_power already uses it in schema.rs). - Add unit tests for collect_rows (flattening, empty-table skipping, device_mac/device_name fallback), row_json (key parity with fields::PORTS_LIST), and project (filtering and no-op on None), plus a wiremock-backed test asserting `ports list` pagination reports the full total alongside a truncated items page. Co-Authored-By: Claude Opus 5 --- src/commands/ports.rs | 148 +++++++++++++++++++++++++++++++++++++++++- src/fields.rs | 4 +- tests/mock_server.rs | 77 ++++++++++++++++++++++ 3 files changed, 226 insertions(+), 3 deletions(-) diff --git a/src/commands/ports.rs b/src/commands/ports.rs index f510df1..66e4c94 100644 --- a/src/commands/ports.rs +++ b/src/commands/ports.rs @@ -141,6 +141,15 @@ pub fn render_text(rows: &[&PortRow], show_device_col: bool, out: &OutputConfig) .unwrap_or_else(|| "-".into()); let name = p.name.as_deref().unwrap_or("-"); let link = if p.up { "up" } else { "down" }; + let link_display = if color { + if p.up { + format!("{}", "up".green()) + } else { + format!("{}", "down".dimmed()) + } + } else { + link.to_string() + }; let speed = speed_cell(p); let poe = poe_cell(p); let tx = p.tx_bytes.map(format_bytes).unwrap_or_else(|| "-".into()); @@ -149,12 +158,12 @@ pub fn render_text(rows: &[&PortRow], show_device_col: bool, out: &OutputConfig) if show_device_col { println!( " {:10} {:>10}", - r.device_name, port, name, link, speed, poe, tx, rx + r.device_name, port, name, link_display, speed, poe, tx, rx ); } else { println!( " {:<5} {:<16} {:<6} {:<10} {:<8} {:>10} {:>10}", - port, name, link, speed, poe, tx, rx + port, name, link_display, speed, poe, tx, rx ); } } @@ -199,3 +208,138 @@ pub async fn list( } Ok(()) } + +#[cfg(test)] +mod tests { + use super::*; + + /// Build a `DeviceWithPorts` fixture from a JSON literal, exercising the + /// same `Deserialize` impl the API layer uses. + fn device(json: serde_json::Value) -> DeviceWithPorts { + serde_json::from_value(json).expect("test fixture must deserialize as DeviceWithPorts") + } + + #[test] + fn collect_rows_flattens_multiple_devices_and_skips_empty_port_tables() { + let devices = vec![ + device(serde_json::json!({ + "mac": "aa:bb:cc:dd:ee:01", "name": "SwitchA", + "port_table": [{"port_idx": 1}, {"port_idx": 2}] + })), + device(serde_json::json!({ + "mac": "aa:bb:cc:dd:ee:02", "name": "APWithNoPorts", + "port_table": [] + })), + device(serde_json::json!({ + "mac": "aa:bb:cc:dd:ee:03", "name": "SwitchC", + "port_table": [{"port_idx": 1}] + })), + ]; + + let rows = collect_rows(&devices); + + assert_eq!( + rows.len(), + 3, + "device with an empty port_table must contribute no rows" + ); + assert_eq!(rows[0].device_name, "SwitchA"); + assert_eq!(rows[0].port.port_idx, Some(1)); + assert_eq!(rows[1].device_name, "SwitchA"); + assert_eq!(rows[1].port.port_idx, Some(2)); + assert_eq!(rows[2].device_name, "SwitchC"); + assert_eq!(rows[2].port.port_idx, Some(1)); + assert!( + rows.iter().all(|r| r.device_name != "APWithNoPorts"), + "a device with no ports must never appear in the flattened rows" + ); + } + + #[test] + fn collect_rows_formats_device_mac_and_falls_back_to_model_when_name_is_absent() { + let devices = vec![ + device(serde_json::json!({ + "mac": "9c05d6bc0643", "name": "USW-24-PoE", + "port_table": [{"port_idx": 1}] + })), + device(serde_json::json!({ + "mac": "aabbccddeeff", "model": "USW-Lite-8", + "port_table": [{"port_idx": 1}] + })), + device(serde_json::json!({ + "mac": "112233445566", + "port_table": [{"port_idx": 1}] + })), + ]; + + let rows = collect_rows(&devices); + + assert_eq!( + rows[0].device_mac, "9c:05:d6:bc:06:43", + "device_mac must be formatted via format_mac, not passed through raw" + ); + assert_eq!(rows[0].device_name, "USW-24-PoE"); + + assert_eq!(rows[1].device_mac, "aa:bb:cc:dd:ee:ff"); + assert_eq!( + rows[1].device_name, "USW-Lite-8", + "device_name must fall back to model when name is absent" + ); + + assert_eq!(rows[2].device_mac, "11:22:33:44:55:66"); + assert_eq!( + rows[2].device_name, "-", + "device_name must fall back to '-' when both name and model are absent" + ); + } + + #[test] + fn row_json_emits_exactly_the_fields_declared_in_ports_list() { + let devices = vec![device(serde_json::json!({ + "mac": "aa:bb:cc:dd:ee:ff", "name": "SwitchA", + "port_table": [{ + "port_idx": 1, "name": "Port 1", "media": "GE", "up": true, + "speed": 1000, "full_duplex": true, "poe_enable": true, + "poe_power": 4.5, "port_poe": true, "tx_bytes": 100, "rx_bytes": 200 + }] + }))]; + let rows = collect_rows(&devices); + let value = row_json(&rows[0]); + let obj = value.as_object().expect("row_json must emit a JSON object"); + + let mut emitted: Vec<&str> = obj.keys().map(String::as_str).collect(); + emitted.sort_unstable(); + let mut declared: Vec<&str> = crate::fields::names(crate::fields::PORTS_LIST); + declared.sort_unstable(); + + assert_eq!( + emitted, declared, + "row_json keys must exactly match fields::PORTS_LIST, so the two cannot drift" + ); + } + + #[test] + fn project_retains_only_the_requested_fields() { + let mut value = serde_json::json!({"a": 1, "b": 2, "c": 3}); + project(&mut value, &Some(vec!["a".to_string(), "c".to_string()])); + + let obj = value.as_object().unwrap(); + assert_eq!(obj.len(), 2); + assert!(obj.contains_key("a")); + assert!(obj.contains_key("c")); + assert!(!obj.contains_key("b"), "unrequested fields must be dropped"); + } + + #[test] + fn project_is_a_noop_when_fields_is_none() { + let mut value = serde_json::json!({"a": 1, "b": 2, "c": 3}); + let before = value.clone(); + + project(&mut value, &None); + + assert_eq!( + value, before, + "a None projection must leave the value untouched" + ); + } +} diff --git a/src/fields.rs b/src/fields.rs index 85deaab..d29d3e7 100644 --- a/src/fields.rs +++ b/src/fields.rs @@ -213,6 +213,7 @@ mod tests { DEVICES_LIST, EVENTS_LIST, NETWORKS_LIST, + PORTS_LIST, ] { let mut seen = names(table); let before = seen.len(); @@ -230,10 +231,11 @@ mod tests { DEVICES_LIST, EVENTS_LIST, NETWORKS_LIST, + PORTS_LIST, ] { for (name, ty) in table { assert!( - ["string", "integer", "boolean"].contains(ty), + ["string", "integer", "boolean", "number"].contains(ty), "field {name} has unexpected type {ty}" ); } diff --git a/tests/mock_server.rs b/tests/mock_server.rs index f661d28..3cd5b93 100644 --- a/tests/mock_server.rs +++ b/tests/mock_server.rs @@ -1791,6 +1791,83 @@ mod command_output { assert!(err.to_string().contains("Not found")); } + // --- Ports list (top-level) --- + // + // Drives the real `unifi` binary against a wiremock server so the JSON + // envelope it actually prints can be inspected. A regression that computed + // `total` from the truncated page (instead of the full flattened result) + // would let an agent mistake a partial page for a complete one, so this + // must observe real stdout rather than call `commands::ports::list` + // in-process and only check that it returns `Ok`. + #[tokio::test] + async fn ports_list_pagination_reports_full_total_and_truncated_items() { + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/proxy/network/api/s/default/stat/device")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "meta": {"rc": "ok"}, + "data": [ + {"mac": "aa:bb:cc:dd:ee:01", "name": "SwitchA", + "port_table": [{"port_idx": 1}, {"port_idx": 2}]}, + {"mac": "aa:bb:cc:dd:ee:02", "name": "SwitchB", + "port_table": [{"port_idx": 1}, {"port_idx": 2}, {"port_idx": 3}]} + ] + }))) + .mount(&server) + .await; + + let output = std::process::Command::new(env!("CARGO_BIN_EXE_unifi")) + .args([ + "--host", + &server.uri(), + "--api-key", + "test-key", + "ports", + "list", + "--output", + "json", + "--limit", + "3", + "--offset", + "1", + ]) + .output() + .expect("failed to run the unifi binary"); + + assert!( + output.status.success(), + "ports list failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + + let body: serde_json::Value = serde_json::from_slice(&output.stdout).unwrap_or_else(|e| { + panic!( + "stdout was not valid JSON ({e}): {}", + String::from_utf8_lossy(&output.stdout) + ) + }); + + let items = body["items"] + .as_array() + .expect("envelope must have an items array"); + assert_eq!( + items.len(), + 3, + "the page must be truncated to the requested limit" + ); + assert_eq!( + body["total"], 5, + "total must reflect every port across every device, not just this page" + ); + assert_ne!( + body["total"].as_u64().unwrap(), + items.len() as u64, + "an agent must be able to tell a truncated page from a complete result" + ); + assert_eq!(body["limit"], 3); + assert_eq!(body["offset"], 1); + } + #[tokio::test] async fn list_events_returns_stat_event_records() { let server = MockServer::start().await; From d7498acdf01124124e17af6f4e9d84711d4d71a8 Mon Sep 17 00:00:00 2001 From: "Brian R. Jackson" Date: Sun, 26 Jul 2026 16:59:08 -0400 Subject: [PATCH 07/23] refactor(devices): route devices ports through the shared renderer devices ports is now an alias for ports list . It gains device_mac/device_name and keeps its bare-array shape; the schema note records why the two spellings differ in envelope but not in fields. --- src/commands/devices.rs | 108 ++++++---------------------------------- src/schema.rs | 15 +----- tests/cli_contract.rs | 18 +++++++ 3 files changed, 35 insertions(+), 106 deletions(-) diff --git a/src/commands/devices.rs b/src/commands/devices.rs index 16aad85..53f233a 100644 --- a/src/commands/devices.rs +++ b/src/commands/devices.rs @@ -1,6 +1,6 @@ use owo_colors::OwoColorize; -use crate::api::{Device, UnifiClient, format_bytes, format_mac, format_uptime}; +use crate::api::{Device, UnifiClient, format_mac, format_uptime}; use crate::output::{OutputConfig, use_color}; pub struct Pagination { @@ -253,6 +253,10 @@ pub async fn restart( Ok(()) } +/// Alias for `ports list `. Deliberately keeps the historical bare JSON +/// array shape: `ports list` emits the paginated `{items,total,...}` envelope, +/// but changing this one from array to object would break every consumer +/// indexing the top level. pub async fn ports( client: &UnifiClient, mac: &str, @@ -268,101 +272,19 @@ pub async fn ports( return Ok(()); } + let devices = vec![device]; + let rows = crate::commands::ports::collect_rows(&devices); + if out.is_json() { - out.print_data( - &serde_json::to_string_pretty( - &device - .port_table - .iter() - .map(|p| { - serde_json::json!({ - "port_idx": p.port_idx, - "name": p.name, - "media": p.media, - "up": p.up, - "speed": p.speed, - "full_duplex": p.full_duplex, - "poe_enable": p.poe_enable, - "poe_power": p.poe_power, - "port_poe": p.port_poe, - "tx_bytes": p.tx_bytes, - "rx_bytes": p.rx_bytes, - }) - }) - .collect::>(), - ) - .expect("failed to serialize JSON"), - ); + let items: Vec = + rows.iter().map(crate::commands::ports::row_json).collect(); + out.print_data(&serde_json::to_string_pretty(&items)?); } else { - let device_label = device - .name - .as_deref() - .unwrap_or(device.model.as_deref().unwrap_or("Device")); - out.print_message(&format!("Ports for {device_label}:\n")); - - let color = use_color(); - let header = format!( - "{:<6} {:<16} {:<6} {:<10} {:<8} {:>10} {:>10}", - "Port", "Name", "Link", "Speed", "PoE", "TX", "RX" - ); - if color { - println!("{}", header.bold()); - println!("{}", "-".repeat(70).dimmed()); - } else { - println!("{header}"); - println!("{}", "-".repeat(70)); - } - - for p in &device.port_table { - let port = p - .port_idx - .map(|i| i.to_string()) - .unwrap_or_else(|| "-".into()); - let name = p.name.as_deref().unwrap_or("-"); - let link = if p.up { "up" } else { "down" }; - let speed = if p.up { - match p.speed { - Some(s) => { - let duplex = if p.full_duplex { "FD" } else { "HD" }; - format!("{s}{duplex}") - } - None => "up".into(), - } - } else { - "down".into() - }; - let poe = if p.poe_enable { - match p.poe_power { - Some(w) if w > 0.0 => format!("{w:.1}W"), - _ => "on".into(), - } - } else if p.port_poe { - "off".into() - } else { - "-".into() - }; - let tx = p.tx_bytes.map(format_bytes).unwrap_or_else(|| "-".into()); - let rx = p.rx_bytes.map(format_bytes).unwrap_or_else(|| "-".into()); - - if color { - let link_display = if p.up { - format!("{}", "up".green()) - } else { - format!("{}", "down".dimmed()) - }; - println!( - " {:<5} {:<16} {:<6} {:<10} {:<8} {:>10} {:>10}", - port, name, link_display, speed, poe, tx, rx - ); - } else { - println!( - " {:<5} {:<16} {:<6} {:<10} {:<8} {:>10} {:>10}", - port, name, link, speed, poe, tx, rx - ); - } - } + let label = &rows[0].device_name; + out.print_message(&format!("Ports for {label}:\n")); + let refs: Vec<&crate::commands::ports::PortRow> = rows.iter().collect(); + crate::commands::ports::render_text(&refs, false, &out); } - out.print_message(&format!("\n{} ports", device.port_table.len())); Ok(()) } diff --git a/src/schema.rs b/src/schema.rs index ca62849..28dfae7 100644 --- a/src/schema.rs +++ b/src/schema.rs @@ -157,20 +157,9 @@ fn command_metadata() -> HashMap<&'static str, CommandMeta> { m.insert( "devices ports", f( - &[ - ("port_idx", "integer"), - ("name", "string"), - ("media", "string"), - ("up", "boolean"), - ("speed", "integer"), - ("full_duplex", "boolean"), - ("poe_enable", "boolean"), - ("poe_power", "number"), - ("tx_bytes", "integer"), - ("rx_bytes", "integer"), - ], + fields::PORTS_LIST, false, - None, + Some("Alias for `ports list`; returns a bare JSON array for backward compatibility."), ), ); m.insert( diff --git a/tests/cli_contract.rs b/tests/cli_contract.rs index c5617c5..6181269 100644 --- a/tests/cli_contract.rs +++ b/tests/cli_contract.rs @@ -180,6 +180,24 @@ fn ports_list_rejects_unknown_field() { ); } +#[test] +fn devices_ports_and_ports_list_are_the_same_command() { + // Both spellings must accept a MAC and reach the network layer, not fail + // at argument parsing. Pointed at an unroutable host, so no controller. + for args in [ + vec!["devices", "ports", "aa:bb:cc:dd:ee:ff"], + vec!["ports", "list", "aa:bb:cc:dd:ee:ff"], + ] { + let out = unifi().args(&args).output().expect("failed to run binary"); + assert_ne!( + out.status.code(), + Some(2), + "{args:?} must not be a usage error, stderr: {}", + String::from_utf8_lossy(&out.stderr) + ); + } +} + #[test] fn ports_live_requires_a_mac() { let out = unifi() From b8e91e99a79e0676339772c1bb3f2320af8239ae Mon Sep 17 00:00:00 2001 From: "Brian R. Jackson" Date: Sun, 26 Jul 2026 17:09:15 -0400 Subject: [PATCH 08/23] test(ports): lock in the devices ports bare-array vs ports list envelope split devices_ports_and_ports_list_are_the_same_command only checks the exit code, and the four existing devices_ports_* tests call commands::devices::ports in-process without ever capturing or parsing stdout, so nothing in the suite would catch a "consistency" refactor that wrapped devices ports in the {items,total,limit,offset} envelope. Add devices_ports_bare_array_vs_ports_list_envelope, following the CARGO_BIN_EXE_unifi + wiremock pattern already used by ports_list_pagination_reports_full_total_and_truncated_items: it spawns the real binary, parses actual stdout as JSON, and asserts devices ports is a bare array while ports list is the envelope, with both sharing the same per-row key set. --- tests/mock_server.rs | 117 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 117 insertions(+) diff --git a/tests/mock_server.rs b/tests/mock_server.rs index 3cd5b93..b270c48 100644 --- a/tests/mock_server.rs +++ b/tests/mock_server.rs @@ -1868,6 +1868,123 @@ mod command_output { assert_eq!(body["offset"], 1); } + // `devices ports ` is documented as an alias for `ports list ` + // that deliberately keeps the historical bare JSON array shape, while + // `ports list` emits the paginated `{items,total,limit,offset}` envelope. + // Wrapping `devices ports` in the envelope would break any consumer that + // indexes the top level, which the design explicitly forbids. + // + // Nothing else in this suite would catch that regression: the in-process + // `devices_ports_*` tests above only `.unwrap()`/`.unwrap_err()` and never + // capture stdout, and `devices_ports_and_ports_list_are_the_same_command` + // in `tests/cli_contract.rs` only asserts the exit code isn't a usage + // error. So this spawns the real compiled binary against a wiremock + // server (same pattern as `ports_list_pagination_reports_full_total_and_truncated_items` + // above) and parses actual stdout as JSON to assert on shape. + #[tokio::test] + async fn devices_ports_bare_array_vs_ports_list_envelope() { + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/proxy/network/api/s/default/stat/device")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "meta": {"rc": "ok"}, + "data": [{ + "mac": "9c:05:d6:bc:06:43", "name": "USW-24-PoE", + "port_table": [ + {"port_idx": 1, "name": "Port 1", "media": "GE", "up": true, "speed": 1000, "full_duplex": true, "poe_enable": true, "poe_power": 5.2, "port_poe": true, "tx_bytes": 123456789, "rx_bytes": 987654321}, + {"port_idx": 2, "name": "Port 2", "media": "GE", "up": true, "speed": 100, "full_duplex": false, "poe_enable": false, "port_poe": true, "tx_bytes": 1000, "rx_bytes": 2000} + ] + }] + }))) + .mount(&server) + .await; + + let run_json = |args: &[&str]| -> serde_json::Value { + let output = std::process::Command::new(env!("CARGO_BIN_EXE_unifi")) + .args(["--host", &server.uri(), "--api-key", "test-key"]) + .args(args) + .output() + .expect("failed to run the unifi binary"); + assert!( + output.status.success(), + "{args:?} failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + serde_json::from_slice(&output.stdout).unwrap_or_else(|e| { + panic!( + "{args:?} stdout was not valid JSON ({e}): {}", + String::from_utf8_lossy(&output.stdout) + ) + }) + }; + + let alias = run_json(&["devices", "ports", "9c:05:d6:bc:06:43", "-o", "json"]); + let canonical = run_json(&["ports", "list", "9c:05:d6:bc:06:43", "-o", "json"]); + + // 1. `devices ports` must be a bare array, and rows must carry the + // device_mac/device_name fields shared with `ports list`. + let alias_items = alias + .as_array() + .unwrap_or_else(|| panic!("devices ports must emit a bare JSON array, got: {alias}")); + assert!( + !alias_items.is_empty(), + "expected at least one port row from devices ports" + ); + let alias_row = alias_items[0] + .as_object() + .expect("devices ports row must be a JSON object"); + assert!( + alias_row.contains_key("device_mac"), + "devices ports row must carry device_mac: {alias_row:?}" + ); + assert!( + alias_row.contains_key("device_name"), + "devices ports row must carry device_name: {alias_row:?}" + ); + + // 2. `ports list` must be the {items,total,limit,offset} envelope. + assert!( + canonical.is_object(), + "ports list must emit an {{items,total,limit,offset}} envelope object, got: {canonical}" + ); + let items = canonical["items"] + .as_array() + .expect("ports list envelope must have an items array"); + assert!( + canonical.get("total").is_some(), + "ports list envelope must have a total field" + ); + assert!( + canonical.get("limit").is_some(), + "ports list envelope must have a limit field" + ); + assert!( + canonical.get("offset").is_some(), + "ports list envelope must have an offset field" + ); + assert!( + !items.is_empty(), + "expected at least one port row from ports list" + ); + + // 3. The two spellings must carry the same key set per row, locking + // in the shared-field-set property alongside the envelope split. + let mut alias_keys: Vec<&str> = alias_row.keys().map(String::as_str).collect(); + alias_keys.sort_unstable(); + let mut canonical_keys: Vec<&str> = items[0] + .as_object() + .expect("ports list row must be a JSON object") + .keys() + .map(String::as_str) + .collect(); + canonical_keys.sort_unstable(); + + assert_eq!( + alias_keys, canonical_keys, + "devices ports and ports list must share the same per-row field set" + ); + } + #[tokio::test] async fn list_events_returns_stat_event_records() { let server = MockServer::start().await; From fa0aa86d41e6ec4b634c3f682fb0cd39247bb79f Mon Sep 17 00:00:00 2001 From: "Brian R. Jackson" Date: Sun, 26 Jul 2026 17:20:45 -0400 Subject: [PATCH 09/23] feat(ports): add ports show with full PoE telemetry Surfaces poe_mode, poe_class, voltage, current and the attached device MAC, none of which the CLI previously exposed. Also adds integration coverage in tests/mock_server.rs: JSON output cross-checked against the published schema's output_fields, tri-state fields (autoneg/enable/is_uplink/poe_good/attached_mac) serializing as null rather than false when firmware omits them, and the not-found exit code that `ports cycle` will depend on. Co-Authored-By: Claude Opus 5 --- src/commands/ports.rs | 166 +++++++++++++++++++++++++++++- src/main.rs | 10 ++ src/schema.rs | 34 ++++++ tests/mock_server.rs | 233 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 442 insertions(+), 1 deletion(-) diff --git a/src/commands/ports.rs b/src/commands/ports.rs index 66e4c94..f24044f 100644 --- a/src/commands/ports.rs +++ b/src/commands/ports.rs @@ -1,6 +1,6 @@ use owo_colors::OwoColorize; -use crate::api::{DeviceWithPorts, PortEntry, UnifiClient, format_bytes, format_mac}; +use crate::api::{ApiError, DeviceWithPorts, PortEntry, UnifiClient, format_bytes, format_mac}; use crate::output::{OutputConfig, use_color}; /// One port, flattened with the device that owns it. Every `ports` subcommand @@ -209,6 +209,143 @@ pub async fn list( Ok(()) } +/// Locate a port by index within a device's port table. +pub fn find_port(device: &DeviceWithPorts, port_idx: u32) -> Result<&PortEntry, ApiError> { + device + .port_table + .iter() + .find(|p| p.port_idx == Some(port_idx)) + .ok_or_else(|| { + let mac = device + .mac + .as_deref() + .map(format_mac) + .unwrap_or_else(|| "device".into()); + ApiError::NotFound(format!("Port {port_idx} on {mac}")) + }) +} + +pub async fn show( + client: &UnifiClient, + mac: &str, + port_idx: u32, + out: OutputConfig, +) -> Result<(), Box> { + let device = client.get_device_ports(mac).await?; + let p = find_port(&device, port_idx)?; + let device_mac = device + .mac + .as_deref() + .map(format_mac) + .unwrap_or_else(|| "-".into()); + let device_name = device + .name + .as_deref() + .or(device.model.as_deref()) + .unwrap_or("-") + .to_string(); + let attached_mac = p + .last_connection + .as_ref() + .and_then(|lc| lc.mac.as_deref()) + .map(format_mac); + + if out.is_json() { + out.print_data(&serde_json::to_string_pretty(&serde_json::json!({ + "device_mac": device_mac, + "device_name": device_name, + "port_idx": p.port_idx, + "name": p.name, + "media": p.media, + "up": p.up, + "speed": p.speed, + "full_duplex": p.full_duplex, + "autoneg": p.autoneg, + "enable": p.enable, + "is_uplink": p.is_uplink, + "stp_state": p.stp_state, + "port_poe": p.port_poe, + "poe_enable": p.poe_enable, + "poe_mode": p.poe_mode, + "poe_class": p.poe_class, + "poe_power": p.poe_power, + "poe_voltage": p.poe_voltage, + "poe_current": p.poe_current, + "poe_good": p.poe_good, + "attached_mac": attached_mac, + "tx_bytes": p.tx_bytes, + "rx_bytes": p.rx_bytes, + "tx_errors": p.tx_errors, + "rx_errors": p.rx_errors, + }))?); + return Ok(()); + } + + let color = use_color(); + let label = |l: &str| -> String { + if color { + format!("{}", l.dimmed()) + } else { + l.to_string() + } + }; + let title = format!("Port {port_idx} on {device_name} ({device_mac})"); + if color { + println!("{}", title.bold()); + } else { + println!("{title}"); + } + println!( + " {} {}", + label("Name: "), + p.name.as_deref().unwrap_or("-") + ); + println!( + " {} {}", + label("Link: "), + if p.up { "up" } else { "down" } + ); + println!(" {} {}", label("Speed: "), speed_cell(p)); + println!( + " {} {}", + label("Media: "), + p.media.as_deref().unwrap_or("-") + ); + println!( + " {} {}", + label("PoE: "), + if p.port_poe { + poe_cell(p) + } else { + "not supported".into() + } + ); + if p.port_poe { + println!( + " {} {}", + label("PoE mode:"), + p.poe_mode.as_deref().unwrap_or("-") + ); + println!( + " {} {}", + label("PoE class"), + p.poe_class.as_deref().unwrap_or("-") + ); + if let Some(v) = p.poe_voltage { + println!(" {} {v:.2} V", label("Voltage: ")); + } + if let Some(c) = p.poe_current { + println!(" {} {c:.2} mA", label("Current: ")); + } + } + println!( + " {} {}", + label("Attached:"), + attached_mac.as_deref().unwrap_or("-") + ); + Ok(()) +} + #[cfg(test)] mod tests { use super::*; @@ -342,4 +479,31 @@ mod tests { "a None projection must leave the value untouched" ); } + + fn device_with(ports: serde_json::Value) -> DeviceWithPorts { + serde_json::from_value(serde_json::json!({ + "mac": "aa:bb:cc:dd:ee:ff", + "name": "SwitchA", + "port_table": ports + })) + .expect("fixture must parse") + } + + #[test] + fn find_port_returns_the_matching_entry() { + let d = device_with(serde_json::json!([ + {"port_idx": 1, "port_poe": true}, + {"port_idx": 5, "port_poe": true, "poe_mode": "auto"} + ])); + let p = find_port(&d, 5).expect("port 5 exists"); + assert_eq!(p.port_idx, Some(5)); + assert_eq!(p.poe_mode.as_deref(), Some("auto")); + } + + #[test] + fn find_port_missing_is_not_found() { + let d = device_with(serde_json::json!([{"port_idx": 1}])); + let err = find_port(&d, 99).expect_err("port 99 does not exist"); + assert!(matches!(err, crate::api::ApiError::NotFound(_))); + } } diff --git a/src/main.rs b/src/main.rs index 7dc7da2..b202708 100644 --- a/src/main.rs +++ b/src/main.rs @@ -255,6 +255,13 @@ enum PortsCommand { #[arg(short = 'i', long, default_value = "2")] interval: u64, }, + /// Show details for a single port + Show { + /// MAC address of the switch or router + mac: String, + /// Port index (see `unifi ports list `) + port: u32, + }, } #[derive(Subcommand)] @@ -1318,6 +1325,9 @@ async fn main() { commands::ports::list(&client, mac.as_deref(), out, pagination).await } } + PortsCommand::Show { mac, port } => { + commands::ports::show(&client, &mac, port, out).await + } }, Command::Events(cmd) => match cmd { EventsCommand::List { diff --git a/src/schema.rs b/src/schema.rs index 28dfae7..0112c84 100644 --- a/src/schema.rs +++ b/src/schema.rs @@ -177,6 +177,40 @@ fn command_metadata() -> HashMap<&'static str, CommandMeta> { // ports m.insert("ports list", f(fields::PORTS_LIST, false, None)); + m.insert( + "ports show", + f( + &[ + ("device_mac", "string"), + ("device_name", "string"), + ("port_idx", "integer"), + ("name", "string"), + ("media", "string"), + ("up", "boolean"), + ("speed", "integer"), + ("full_duplex", "boolean"), + ("autoneg", "boolean"), + ("enable", "boolean"), + ("is_uplink", "boolean"), + ("stp_state", "string"), + ("port_poe", "boolean"), + ("poe_enable", "boolean"), + ("poe_mode", "string"), + ("poe_class", "string"), + ("poe_power", "number"), + ("poe_voltage", "number"), + ("poe_current", "number"), + ("poe_good", "boolean"), + ("attached_mac", "string"), + ("tx_bytes", "integer"), + ("rx_bytes", "integer"), + ("tx_errors", "integer"), + ("rx_errors", "integer"), + ], + false, + None, + ), + ); // networks / events / system m.insert("networks list", f(fields::NETWORKS_LIST, false, None)); diff --git a/tests/mock_server.rs b/tests/mock_server.rs index b270c48..34b353b 100644 --- a/tests/mock_server.rs +++ b/tests/mock_server.rs @@ -1791,6 +1791,239 @@ mod command_output { assert!(err.to_string().contains("Not found")); } + // --- Ports show (single-port detail) --- + + #[tokio::test] + async fn ports_show_table() { + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/proxy/network/api/s/default/stat/device")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "meta": {"rc": "ok"}, + "data": [{ + "mac": "9c:05:d6:bc:06:43", "name": "USW-24-PoE", + "port_table": [ + {"port_idx": 1, "name": "Port 1", "media": "GE", "up": true}, + { + "port_idx": 5, "name": "Port 5", "media": "GE", "up": true, + "speed": 1000, "full_duplex": true, "autoneg": true, "enable": true, + "is_uplink": false, "stp_state": "forwarding", + "port_poe": true, "poe_enable": true, "poe_mode": "auto", + "poe_class": "4", "poe_power": 5.2, "poe_voltage": 53.5, + "poe_current": 120.3, "poe_good": true, + "last_connection": {"mac": "aabbccddeeff", "connected": true}, + "tx_bytes": 100, "rx_bytes": 200, "tx_errors": 0, "rx_errors": 2 + } + ] + }] + }))) + .mount(&server) + .await; + + let client = mock_client(&server).await; + unifi_cli::commands::ports::show(&client, "9c:05:d6:bc:06:43", 5, out_table()) + .await + .unwrap(); + } + + // Drives the real `unifi` binary so the JSON this command actually prints + // can be inspected, and cross-checks it against what `unifi schema` + // publishes for "ports show" — the two are supposed to be the same + // contract, and nothing else in this suite would catch them drifting + // apart. + #[tokio::test] + async fn ports_show_json_matches_schema_output_fields() { + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/proxy/network/api/s/default/stat/device")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "meta": {"rc": "ok"}, + "data": [{ + "mac": "9c:05:d6:bc:06:43", "name": "USW-24-PoE", + "port_table": [{ + "port_idx": 5, "name": "Port 5", "media": "GE", "up": true, + "speed": 1000, "full_duplex": true, "autoneg": true, "enable": true, + "is_uplink": false, "stp_state": "forwarding", + "port_poe": true, "poe_enable": true, "poe_mode": "auto", + "poe_class": "4", "poe_power": 5.2, "poe_voltage": 53.5, + "poe_current": 120.3, "poe_good": true, + "last_connection": {"mac": "aabbccddeeff", "connected": true}, + "tx_bytes": 100, "rx_bytes": 200, "tx_errors": 0, "rx_errors": 2 + }] + }] + }))) + .mount(&server) + .await; + + let output = std::process::Command::new(env!("CARGO_BIN_EXE_unifi")) + .args([ + "--host", + &server.uri(), + "--api-key", + "test-key", + "ports", + "show", + "9c:05:d6:bc:06:43", + "5", + "--output", + "json", + ]) + .output() + .expect("failed to run the unifi binary"); + assert!( + output.status.success(), + "ports show failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + + let body: serde_json::Value = serde_json::from_slice(&output.stdout).unwrap_or_else(|e| { + panic!( + "stdout was not valid JSON ({e}): {}", + String::from_utf8_lossy(&output.stdout) + ) + }); + let obj = body + .as_object() + .expect("ports show must emit a JSON object"); + + // Values that were previously fetched and thrown away. + assert_eq!(obj["device_mac"], "9c:05:d6:bc:06:43"); + assert_eq!(obj["port_idx"], 5); + assert_eq!(obj["poe_mode"], "auto"); + assert_eq!(obj["poe_class"], "4"); + assert_eq!(obj["poe_voltage"], 53.5); + assert_eq!(obj["poe_current"], 120.3); + assert_eq!(obj["poe_good"], true); + assert_eq!( + obj["attached_mac"], "aa:bb:cc:dd:ee:ff", + "attached_mac must be read from last_connection.mac and formatted" + ); + assert_eq!(obj["tx_errors"], 0); + assert_eq!(obj["rx_errors"], 2); + + // The schema's published output_fields must exactly match the keys + // this JSON branch actually emits: no undiscoverable field, and no + // documented field that never appears. + let schema_output = std::process::Command::new(env!("CARGO_BIN_EXE_unifi")) + .arg("schema") + .output() + .expect("failed to run unifi schema"); + let schema: serde_json::Value = serde_json::from_slice(&schema_output.stdout) + .expect("unifi schema must print valid JSON"); + let ports_show = schema["commands"] + .as_array() + .expect("schema must have a commands array") + .iter() + .find(|c| c["name"] == "ports show") + .expect("schema must publish a \"ports show\" command"); + let mut declared: Vec<&str> = ports_show["output_fields"] + .as_array() + .expect("ports show must declare output_fields") + .iter() + .map(|f| f["name"].as_str().expect("output field must have a name")) + .collect(); + declared.sort_unstable(); + let mut emitted: Vec<&str> = obj.keys().map(String::as_str).collect(); + emitted.sort_unstable(); + assert_eq!( + emitted, declared, + "ports show output_fields in the schema must exactly match the JSON branch's keys" + ); + } + + // `autoneg`/`enable`/`is_uplink`/`poe_good` are tri-state: a firmware that + // omits the key must serialize as JSON null, not fall back to `false`, + // since a missing key must not read as a confident "disabled". Likewise + // `attached_mac` must be null when no device has ever linked to the port. + #[tokio::test] + async fn ports_show_omitted_tri_state_fields_serialize_as_null() { + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/proxy/network/api/s/default/stat/device")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "meta": {"rc": "ok"}, + "data": [{ + "mac": "aa:bb:cc:dd:ee:ff", "name": "USW-Lite-8", + "port_table": [ + {"port_idx": 3, "name": "Port 3", "media": "GE", "up": false} + ] + }] + }))) + .mount(&server) + .await; + + let output = std::process::Command::new(env!("CARGO_BIN_EXE_unifi")) + .args([ + "--host", + &server.uri(), + "--api-key", + "test-key", + "ports", + "show", + "aa:bb:cc:dd:ee:ff", + "3", + "--output", + "json", + ]) + .output() + .expect("failed to run the unifi binary"); + assert!(output.status.success()); + + let body: serde_json::Value = serde_json::from_slice(&output.stdout).unwrap(); + for field in ["autoneg", "enable", "is_uplink", "poe_good", "attached_mac"] { + assert!( + body[field].is_null(), + "{field} must be null when firmware omits it, not false: {body}" + ); + } + } + + // Locates `unifi ports cycle 99` uses the same `find_port` lookup; + // a bogus port index must be reported as not-found (exit 4) rather than + // firing a command at the controller for a port that does not exist. + #[tokio::test] + async fn ports_show_not_found() { + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/proxy/network/api/s/default/stat/device")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "meta": {"rc": "ok"}, + "data": [{ + "mac": "aa:bb:cc:dd:ee:ff", "name": "USW-Lite-8", + "port_table": [{"port_idx": 1}] + }] + }))) + .mount(&server) + .await; + + let output = std::process::Command::new(env!("CARGO_BIN_EXE_unifi")) + .args([ + "--host", + &server.uri(), + "--api-key", + "test-key", + "ports", + "show", + "aa:bb:cc:dd:ee:ff", + "99", + ]) + .output() + .expect("failed to run the unifi binary"); + + assert_eq!( + output.status.code(), + Some(4), + "a nonexistent port must exit 4 (not found), got {:?}\nstderr: {}", + output.status.code(), + String::from_utf8_lossy(&output.stderr) + ); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("Not found"), + "stderr must explain the port was not found: {stderr}" + ); + } + // --- Ports list (top-level) --- // // Drives the real `unifi` binary against a wiremock server so the JSON From 3419e969c2c71283250f3d503772136f269b8c18 Mon Sep 17 00:00:00 2001 From: "Brian R. Jackson" Date: Sun, 26 Jul 2026 17:39:20 -0400 Subject: [PATCH 10/23] feat(ports): add ports find reverse lookup Answers which switch port a device is plugged into, using port_table.last_connection.mac. Accepts MAC, IP or client name; ambiguous names return conflict with the candidates rather than guessing. Stale records are returned with connected=false, sorted last. --- src/commands/ports.rs | 246 +++++++++++++++++++++++++++++++++++++++++- src/fields.rs | 22 ++++ src/main.rs | 12 +++ src/schema.rs | 1 + tests/mock_server.rs | 138 ++++++++++++++++++++++++ 5 files changed, 418 insertions(+), 1 deletion(-) diff --git a/src/commands/ports.rs b/src/commands/ports.rs index f24044f..de794ed 100644 --- a/src/commands/ports.rs +++ b/src/commands/ports.rs @@ -1,6 +1,9 @@ use owo_colors::OwoColorize; -use crate::api::{ApiError, DeviceWithPorts, PortEntry, UnifiClient, format_bytes, format_mac}; +use crate::api::{ + ApiError, DeviceWithPorts, LegacyClient, PortEntry, UnifiClient, format_bytes, format_mac, + normalize_mac, +}; use crate::output::{OutputConfig, use_color}; /// One port, flattened with the device that owns it. Every `ports` subcommand @@ -225,6 +228,143 @@ pub fn find_port(device: &DeviceWithPorts, port_idx: u32) -> Result<&PortEntry, }) } +/// Normalize `identifier` and return it only if it already has MAC shape (12 +/// hex digits once separators are stripped). Shared by `resolve_identifier` +/// and `find` so a MAC identifier is recognized identically in both places +/// without duplicating the predicate. +fn identifier_as_mac(identifier: &str) -> Option { + let normalized = normalize_mac(identifier); + (normalized.len() == 12 && normalized.chars().all(|c| c.is_ascii_hexdigit())) + .then_some(normalized) +} + +/// Resolve a MAC, IP, or client name to a normalized MAC. +/// +/// Ordered, stopping at the first tier that matches: normalized MAC equality, +/// then exact IP, then case-insensitive name, then hostname. Follows the +/// `protect cameras show ` precedent rather than the MAC-only +/// convention of `clients show`, because the whole point of `find` is not +/// having to look the MAC up first. +pub fn resolve_identifier(identifier: &str, clients: &[LegacyClient]) -> Result { + if let Some(mac) = identifier_as_mac(identifier) { + return Ok(mac); + } + + if let Some(c) = clients.iter().find(|c| c.ip.as_deref() == Some(identifier)) + && let Some(mac) = c.mac.as_deref() + { + return Ok(normalize_mac(mac)); + } + + let wanted = identifier.to_lowercase(); + let by_name: Vec<&LegacyClient> = clients + .iter() + .filter(|c| { + c.name + .as_deref() + .is_some_and(|n| n.to_lowercase().contains(&wanted)) + || c.hostname + .as_deref() + .is_some_and(|h| h.to_lowercase().contains(&wanted)) + }) + .collect(); + + match by_name.as_slice() { + [] => Err(ApiError::NotFound(format!( + "No client matching '{identifier}'" + ))), + [one] => one + .mac + .as_deref() + .map(normalize_mac) + .ok_or_else(|| ApiError::NotFound(format!("Client '{identifier}' has no MAC"))), + many => { + let list = many + .iter() + .map(|c| { + format!( + "{} ({})", + c.name.as_deref().or(c.hostname.as_deref()).unwrap_or("-"), + c.mac + .as_deref() + .map(format_mac) + .unwrap_or_else(|| "-".into()) + ) + }) + .collect::>() + .join(", "); + Err(ApiError::Conflict(format!( + "'{identifier}' matches {} clients: {list}", + many.len() + ))) + } + } +} + +/// Rows whose `last_connection.mac` matches, connected first so a stale record +/// reads as history rather than as the device's current location. +pub fn matching_rows<'a>( + rows: &'a [PortRow<'a>], + normalized_mac: &str, +) -> Vec<(&'a PortRow<'a>, bool)> { + let mut hits: Vec<(&PortRow, bool)> = rows + .iter() + .filter_map(|r| { + let lc = r.port.last_connection.as_ref()?; + let m = lc.mac.as_deref()?; + (normalize_mac(m) == normalized_mac).then(|| (r, lc.connected.unwrap_or(false))) + }) + .collect(); + hits.sort_by_key(|(_, connected)| !*connected); + hits +} + +/// Find which switch port a device is attached to, by MAC, IP, or client +/// name. +pub async fn find( + client: &UnifiClient, + identifier: &str, + out: OutputConfig, + fields: Option>, +) -> Result<(), Box> { + // A MAC identifier resolves locally, so the common scripted path stays a + // single round trip. + let target = if let Some(mac) = identifier_as_mac(identifier) { + mac + } else { + let clients = client.list_clients_legacy().await?; + resolve_identifier(identifier, &clients)? + }; + + let devices = client.list_all_device_ports().await?; + let rows = collect_rows(&devices); + let hits = matching_rows(&rows, &target); + + if hits.is_empty() { + return Err(Box::new(ApiError::NotFound(format!( + "No switch port with {} attached", + format_mac(&target) + )))); + } + + if out.is_json() { + let items: Vec = hits + .iter() + .map(|(r, connected)| { + let mut v = row_json(r); + v["connected"] = (*connected).into(); + project(&mut v, &fields); + v + }) + .collect(); + out.print_data(&serde_json::to_string_pretty(&items)?); + } else { + let refs: Vec<&PortRow> = hits.iter().map(|(r, _)| *r).collect(); + render_text(&refs, true, &out); + } + Ok(()) +} + pub async fn show( client: &UnifiClient, mac: &str, @@ -506,4 +646,108 @@ mod tests { let err = find_port(&d, 99).expect_err("port 99 does not exist"); assert!(matches!(err, crate::api::ApiError::NotFound(_))); } + + // `_id` is required by `LegacyClient` (every other fixture in this codebase + // supplies it); the plan's literal fixture omitted it, so it is added here + // to make the fixture actually deserialize. + fn clients_fixture() -> Vec { + serde_json::from_value(serde_json::json!([ + {"_id": "1", "mac": "d8:3a:dd:2b:fa:8a", "name": "allsky", "ip": "10.0.0.5"}, + {"_id": "2", "mac": "f4:e2:c6:65:47:6c", "name": "bedroom-ap", "ip": "10.0.0.6"}, + {"_id": "3", "mac": "c4:f7:c1:61:de:31", "name": "Main-Bedroom", "ip": "10.0.0.7"} + ])) + .expect("fixture must parse") + } + + #[test] + fn resolve_identifier_accepts_any_mac_format() { + let c = clients_fixture(); + // A MAC resolves without consulting the client list at all. + assert_eq!( + resolve_identifier("D8-3A-DD-2B-FA-8A", &c).unwrap(), + "d83add2bfa8a" + ); + } + + #[test] + fn resolve_identifier_matches_ip_then_name() { + let c = clients_fixture(); + assert_eq!(resolve_identifier("10.0.0.5", &c).unwrap(), "d83add2bfa8a"); + assert_eq!(resolve_identifier("ALLSKY", &c).unwrap(), "d83add2bfa8a"); + } + + #[test] + fn resolve_identifier_ambiguous_name_is_conflict() { + let c = clients_fixture(); + let err = resolve_identifier("bedroom", &c).expect_err("ambiguous"); + match err { + crate::api::ApiError::Conflict(msg) => { + assert!(msg.contains("matches 2 clients"), "got: {msg}"); + assert!(msg.contains("bedroom-ap"), "got: {msg}"); + assert!(msg.contains("Main-Bedroom"), "got: {msg}"); + } + other => panic!("expected Conflict, got {other:?}"), + } + } + + #[test] + fn resolve_identifier_unknown_is_not_found() { + let c = clients_fixture(); + let err = resolve_identifier("nothing-here", &c).expect_err("unknown"); + assert!(matches!(err, crate::api::ApiError::NotFound(_))); + } + + #[test] + fn matching_rows_sort_connected_first() { + let devices: Vec = serde_json::from_value(serde_json::json!([{ + "mac": "aa:bb:cc:dd:ee:ff", + "name": "SwitchA", + "port_table": [ + {"port_idx": 2, "last_connection": {"mac": "d8:3a:dd:2b:fa:8a", "connected": false}}, + {"port_idx": 7, "last_connection": {"mac": "d8:3a:dd:2b:fa:8a", "connected": true}}, + {"port_idx": 9, "last_connection": {"mac": "11:22:33:44:55:66", "connected": true}} + ] + }])) + .expect("fixture must parse"); + let rows = collect_rows(&devices); + let hits = matching_rows(&rows, "d83add2bfa8a"); + assert_eq!(hits.len(), 2, "device appears on two ports"); + assert_eq!( + hits[0].0.port.port_idx, + Some(7), + "connected port sorts first" + ); + assert!(hits[0].1, "first hit is connected"); + assert!(!hits[1].1, "second hit is the stale record"); + } + + #[test] + fn find_json_row_matches_exactly_the_fields_declared_in_ports_find() { + // Exercises the same construction `find` uses (`row_json` plus the + // manually-inserted `connected` key) without needing an HTTP mock, so + // a drift between the two can never sneak past this test. + let devices = vec![device(serde_json::json!({ + "mac": "aa:bb:cc:dd:ee:ff", "name": "SwitchA", + "port_table": [{ + "port_idx": 7, + "last_connection": {"mac": "d8:3a:dd:2b:fa:8a", "connected": true} + }] + }))]; + let rows = collect_rows(&devices); + let hits = matching_rows(&rows, "d83add2bfa8a"); + let (row, connected) = hits[0]; + let mut value = row_json(row); + value["connected"] = connected.into(); + + let obj = value.as_object().expect("must emit a JSON object"); + let mut emitted: Vec<&str> = obj.keys().map(String::as_str).collect(); + emitted.sort_unstable(); + let mut declared: Vec<&str> = crate::fields::names(crate::fields::PORTS_FIND); + declared.sort_unstable(); + + assert_eq!( + emitted, declared, + "find's emitted keys must exactly match fields::PORTS_FIND" + ); + } } diff --git a/src/fields.rs b/src/fields.rs index d29d3e7..1d38d7b 100644 --- a/src/fields.rs +++ b/src/fields.rs @@ -83,6 +83,26 @@ pub const PORTS_LIST: &[Field] = &[ ("rx_bytes", "integer"), ]; +/// `ports find` rows: the `PORTS_LIST` set plus `connected`, which +/// distinguishes a live attachment from a stale record. Kept separate from +/// `PORTS_LIST` so the `devices ports` alias gains exactly two new keys. +pub const PORTS_FIND: &[Field] = &[ + ("device_mac", "string"), + ("device_name", "string"), + ("port_idx", "integer"), + ("name", "string"), + ("media", "string"), + ("up", "boolean"), + ("speed", "integer"), + ("full_duplex", "boolean"), + ("poe_enable", "boolean"), + ("poe_power", "number"), + ("port_poe", "boolean"), + ("tx_bytes", "integer"), + ("rx_bytes", "integer"), + ("connected", "boolean"), +]; + /// A `--fields` request naming one or more unknown fields. #[derive(Debug, PartialEq, Eq)] pub struct InvalidFields { @@ -214,6 +234,7 @@ mod tests { EVENTS_LIST, NETWORKS_LIST, PORTS_LIST, + PORTS_FIND, ] { let mut seen = names(table); let before = seen.len(); @@ -232,6 +253,7 @@ mod tests { EVENTS_LIST, NETWORKS_LIST, PORTS_LIST, + PORTS_FIND, ] { for (name, ty) in table { assert!( diff --git a/src/main.rs b/src/main.rs index b202708..ac26be6 100644 --- a/src/main.rs +++ b/src/main.rs @@ -262,6 +262,14 @@ enum PortsCommand { /// Port index (see `unifi ports list `) port: u32, }, + /// Find which switch port a device is attached to + Find { + /// MAC address, IP address, or client name + identifier: String, + /// Comma-separated list of fields to include in output (see `unifi schema`) + #[arg(long)] + fields: Option, + }, } #[derive(Subcommand)] @@ -382,6 +390,7 @@ fn validate_requested_fields(command: &Command) -> Result>, I Command::Devices(DevicesCommand::List { fields, .. }) => (fields, fields::DEVICES_LIST), Command::Events(EventsCommand::List { fields, .. }) => (fields, fields::EVENTS_LIST), Command::Ports(PortsCommand::List { fields, .. }) => (fields, fields::PORTS_LIST), + Command::Ports(PortsCommand::Find { fields, .. }) => (fields, fields::PORTS_FIND), _ => return Ok(None), }; @@ -1328,6 +1337,9 @@ async fn main() { PortsCommand::Show { mac, port } => { commands::ports::show(&client, &mac, port, out).await } + PortsCommand::Find { identifier, .. } => { + commands::ports::find(&client, &identifier, out, requested_fields).await + } }, Command::Events(cmd) => match cmd { EventsCommand::List { diff --git a/src/schema.rs b/src/schema.rs index 0112c84..4b246fb 100644 --- a/src/schema.rs +++ b/src/schema.rs @@ -211,6 +211,7 @@ fn command_metadata() -> HashMap<&'static str, CommandMeta> { None, ), ); + m.insert("ports find", f(fields::PORTS_FIND, false, None)); // networks / events / system m.insert("networks list", f(fields::NETWORKS_LIST, false, None)); diff --git a/tests/mock_server.rs b/tests/mock_server.rs index 34b353b..dff3e83 100644 --- a/tests/mock_server.rs +++ b/tests/mock_server.rs @@ -2218,6 +2218,144 @@ mod command_output { ); } + // --- Ports find (reverse lookup) --- + + // A MAC identifier must resolve locally so the common scripted path stays + // a single round trip; mounting `/stat/sta` with `.expect(0)` turns an + // accidental client-list fetch into a test failure instead of a silent, + // unnoticed second request. This also locks in connected-first sorting + // and the exact `PORTS_FIND` field set end to end, through the real + // binary and JSON output, not just the in-process helpers. + #[tokio::test] + async fn ports_find_by_mac_sorts_connected_first_and_skips_client_lookup() { + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/proxy/network/api/s/default/stat/device")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "meta": {"rc": "ok"}, + "data": [{ + "mac": "9c:05:d6:bc:06:43", "name": "USW-24-PoE", + "port_table": [ + {"port_idx": 2, "last_connection": {"mac": "d8:3a:dd:2b:fa:8a", "connected": false}}, + {"port_idx": 7, "last_connection": {"mac": "d8:3a:dd:2b:fa:8a", "connected": true}}, + {"port_idx": 9, "last_connection": {"mac": "11:22:33:44:55:66", "connected": true}} + ] + }] + }))) + .mount(&server) + .await; + Mock::given(method("GET")) + .and(path("/proxy/network/api/s/default/stat/sta")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "meta": {"rc": "ok"}, "data": [] + }))) + .expect(0) + .mount(&server) + .await; + + let output = std::process::Command::new(env!("CARGO_BIN_EXE_unifi")) + .args([ + "--host", + &server.uri(), + "--api-key", + "test-key", + "ports", + "find", + "d8:3a:dd:2b:fa:8a", + "-o", + "json", + ]) + .output() + .expect("failed to run the unifi binary"); + assert!( + output.status.success(), + "ports find failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + + let body: serde_json::Value = serde_json::from_slice(&output.stdout).unwrap_or_else(|e| { + panic!( + "stdout was not valid JSON ({e}): {}", + String::from_utf8_lossy(&output.stdout) + ) + }); + let items = body + .as_array() + .expect("ports find must emit a bare JSON array, like `networks list`"); + assert_eq!(items.len(), 2, "the device appears on two ports"); + assert_eq!( + items[0]["port_idx"], 7, + "the connected port must sort first" + ); + assert_eq!(items[0]["connected"], true); + assert_eq!(items[1]["port_idx"], 2, "the stale record sorts last"); + assert_eq!(items[1]["connected"], false); + + let mut emitted: Vec<&str> = items[0] + .as_object() + .expect("row must be a JSON object") + .keys() + .map(String::as_str) + .collect(); + emitted.sort_unstable(); + let mut declared: Vec<&str> = unifi_cli::fields::names(unifi_cli::fields::PORTS_FIND); + declared.sort_unstable(); + assert_eq!( + emitted, declared, + "ports find rows must carry exactly the PORTS_FIND field set" + ); + } + + // Ambiguity is never guessed: a name matching more than one client must + // exit 6 (conflict) and name the candidates, rather than silently picking + // one. + #[tokio::test] + async fn ports_find_ambiguous_name_exits_with_conflict() { + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/proxy/network/api/s/default/stat/sta")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "meta": {"rc": "ok"}, + "data": [ + {"_id": "1", "mac": "f4:e2:c6:65:47:6c", "name": "bedroom-ap", "ip": "10.0.0.6"}, + {"_id": "2", "mac": "c4:f7:c1:61:de:31", "name": "Main-Bedroom", "ip": "10.0.0.7"} + ] + }))) + .mount(&server) + .await; + + let output = std::process::Command::new(env!("CARGO_BIN_EXE_unifi")) + .args([ + "--host", + &server.uri(), + "--api-key", + "test-key", + "ports", + "find", + "bedroom", + ]) + .output() + .expect("failed to run the unifi binary"); + + assert_eq!( + output.status.code(), + Some(6), + "an ambiguous name must exit 6 (conflict), got {:?}\nstderr: {}", + output.status.code(), + String::from_utf8_lossy(&output.stderr) + ); + let stderr = String::from_utf8_lossy(&output.stderr); + let last_line = stderr.trim_end().lines().last().unwrap_or(""); + let envelope: serde_json::Value = + serde_json::from_str(last_line).expect("last stderr line must be valid JSON"); + assert_eq!(envelope["error"]["kind"], "conflict"); + let message = envelope["error"]["message"] + .as_str() + .expect("error envelope must carry a message"); + assert!(message.contains("bedroom-ap"), "got: {message}"); + assert!(message.contains("Main-Bedroom"), "got: {message}"); + } + #[tokio::test] async fn list_events_returns_stat_event_records() { let server = MockServer::start().await; From f319c23899d937ed2372217498c26891938d568f Mon Sep 17 00:00:00 2001 From: "Brian R. Jackson" Date: Sun, 26 Jul 2026 17:50:10 -0400 Subject: [PATCH 11/23] feat(ports): add ports cycle with pre-flight guard rails Rejects non-PoE ports and administratively-disabled PoE before any HTTP call, so a doomed command never reaches the controller. Registered as mutating: true so agents do not auto-approve it. --- src/commands/ports.rs | 135 ++++++++++++++++++++++++++++++++++++++++++ src/main.rs | 16 +++++ src/schema.rs | 13 ++++ tests/cli_contract.rs | 15 +++++ 4 files changed, 179 insertions(+) diff --git a/src/commands/ports.rs b/src/commands/ports.rs index de794ed..b4fc16b 100644 --- a/src/commands/ports.rs +++ b/src/commands/ports.rs @@ -486,6 +486,94 @@ pub async fn show( Ok(()) } +/// Reject a power-cycle that cannot succeed, before any HTTP call. +pub fn check_cyclable(port: &PortEntry, device_mac: &str) -> Result<(), ApiError> { + let idx = port + .port_idx + .map(|i| i.to_string()) + .unwrap_or_else(|| "?".into()); + let mac = format_mac(device_mac); + + if !port.port_poe { + return Err(ApiError::Conflict(format!( + "Port {idx} on {mac} does not support PoE. \ + Run `unifi ports list {mac}` to see PoE-capable ports." + ))); + } + // Only an explicit "off" blocks. An absent or unrecognised poe_mode + // proceeds: the field is not guaranteed across firmware revisions. + if port.poe_mode.as_deref() == Some("off") { + return Err(ApiError::Conflict(format!( + "PoE is administratively disabled on port {idx} of {mac} (poe_mode=off)" + ))); + } + Ok(()) +} + +/// Whether the cycle actually happened. `Declined` is not an error at this +/// layer — the caller decides how to report a refused confirmation. +#[derive(Debug, PartialEq, Eq)] +pub enum CycleOutcome { + Cycled, + Declined, +} + +/// Power-cycle one PoE port. +/// +/// `confirm` receives a human-readable summary of what is about to lose power +/// and returns whether to proceed. Taking it as a callback keeps the device +/// fetch and the guard rails to exactly one pass: the prompt needs the same +/// port data the checks do, so resolving it twice would mean two round trips +/// to the controller and two chances for the answers to disagree. +pub async fn cycle( + client: &UnifiClient, + mac: &str, + port_idx: u32, + out: OutputConfig, + confirm: F, +) -> Result> +where + F: FnOnce(&str) -> std::io::Result, +{ + let device = client.get_device_ports(mac).await?; + let port = find_port(&device, port_idx)?; + let device_mac = device.mac.as_deref().unwrap_or(mac).to_string(); + check_cyclable(port, &device_mac)?; + + if !confirm(&cycle_summary(&device, port))? { + return Ok(CycleOutcome::Declined); + } + + client.power_cycle_port(&device_mac, port_idx).await?; + out.print_result( + &serde_json::json!({ + "status": "ok", + "action": "power-cycle", + "mac": format_mac(&device_mac), + "port_idx": port_idx, + }), + &format!( + "Power-cycling port {port_idx} on {}", + format_mac(&device_mac) + ), + ); + Ok(CycleOutcome::Cycled) +} + +/// One-line description of what is about to lose power, shown at the prompt. +pub fn cycle_summary(device: &DeviceWithPorts, port: &PortEntry) -> String { + let device_mac = device + .mac + .as_deref() + .map(format_mac) + .unwrap_or_else(|| "-".into()); + let idx = port + .port_idx + .map(|i| i.to_string()) + .unwrap_or_else(|| "?".into()); + format!("Port {idx} on {device_mac}") +} + #[cfg(test)] mod tests { use super::*; @@ -647,6 +735,53 @@ mod tests { assert!(matches!(err, crate::api::ApiError::NotFound(_))); } + #[test] + fn check_cyclable_rejects_non_poe_port() { + let d = device_with(serde_json::json!([{"port_idx": 9, "port_poe": false}])); + let p = find_port(&d, 9).unwrap(); + let err = check_cyclable(p, "aa:bb:cc:dd:ee:ff").expect_err("SFP+ has no PoE"); + match err { + crate::api::ApiError::Conflict(msg) => { + assert!(msg.contains("does not support PoE"), "got: {msg}") + } + other => panic!("expected Conflict, got {other:?}"), + } + } + + #[test] + fn check_cyclable_rejects_poe_mode_off() { + let d = device_with(serde_json::json!([ + {"port_idx": 4, "port_poe": true, "poe_mode": "off"} + ])); + let p = find_port(&d, 4).unwrap(); + let err = check_cyclable(p, "aa:bb:cc:dd:ee:ff").expect_err("PoE is off"); + match err { + crate::api::ApiError::Conflict(msg) => { + assert!(msg.contains("poe_mode=off"), "got: {msg}") + } + other => panic!("expected Conflict, got {other:?}"), + } + } + + #[test] + fn check_cyclable_allows_absent_poe_mode() { + // poe_mode is not guaranteed across firmware. A missing value must not + // block a port that already passed the port_poe check. + let d = device_with(serde_json::json!([{"port_idx": 4, "port_poe": true}])); + let p = find_port(&d, 4).unwrap(); + assert!(check_cyclable(p, "aa:bb:cc:dd:ee:ff").is_ok()); + } + + #[test] + fn check_cyclable_allows_empty_powered_port() { + // The live happy-path target: PoE-capable, auto, nothing attached. + let d = device_with(serde_json::json!([ + {"port_idx": 4, "port_poe": true, "poe_mode": "auto", "up": false} + ])); + let p = find_port(&d, 4).unwrap(); + assert!(check_cyclable(p, "74:ac:b9:ec:b4:5e").is_ok()); + } + // `_id` is required by `LegacyClient` (every other fixture in this codebase // supplies it); the plan's literal fixture omitted it, so it is added here // to make the fixture actually deserialize. diff --git a/src/main.rs b/src/main.rs index ac26be6..b49b64d 100644 --- a/src/main.rs +++ b/src/main.rs @@ -270,6 +270,13 @@ enum PortsCommand { #[arg(long)] fields: Option, }, + /// Power-cycle a single PoE port + Cycle { + /// MAC address of the switch (not the attached device) + mac: String, + /// Port index (see `unifi ports list `) + port: u32, + }, } #[derive(Subcommand)] @@ -1340,6 +1347,15 @@ async fn main() { PortsCommand::Find { identifier, .. } => { commands::ports::find(&client, &identifier, out, requested_fields).await } + PortsCommand::Cycle { mac, port } => { + require_confirmation(cli.yes, "power-cycle"); + // Task 9 replaces this always-proceed callback with the real + // TTY prompt. require_confirmation has already exited for the + // piped-without---yes case by this point. + commands::ports::cycle(&client, &mac, port, out, |_| Ok(true)) + .await + .map(|_| ()) + } }, Command::Events(cmd) => match cmd { EventsCommand::List { diff --git a/src/schema.rs b/src/schema.rs index 4b246fb..aea30c2 100644 --- a/src/schema.rs +++ b/src/schema.rs @@ -212,6 +212,19 @@ fn command_metadata() -> HashMap<&'static str, CommandMeta> { ), ); m.insert("ports find", f(fields::PORTS_FIND, false, None)); + m.insert( + "ports cycle", + f( + &[ + ("status", "string"), + ("action", "string"), + ("mac", "string"), + ("port_idx", "integer"), + ], + true, + None, + ), + ); // networks / events / system m.insert("networks list", f(fields::NETWORKS_LIST, false, None)); diff --git a/tests/cli_contract.rs b/tests/cli_contract.rs index 6181269..81de144 100644 --- a/tests/cli_contract.rs +++ b/tests/cli_contract.rs @@ -298,3 +298,18 @@ fn every_published_output_field_is_accepted_by_fields() { ); } } + +#[test] +fn ports_cycle_requires_yes_without_a_tty() { + let out = unifi() + .args(["ports", "cycle", "aa:bb:cc:dd:ee:ff", "5"]) + .stdin(std::process::Stdio::null()) + .output() + .expect("failed to run binary"); + + assert_eq!(out.status.code(), Some(2)); + assert_eq!( + error_envelope(&out.stderr)["error"]["kind"].as_str(), + Some("confirmation_required") + ); +} From 96dd78d6bff1a4ec0236b78676f02f1b8af0410e Mon Sep 17 00:00:00 2001 From: "Brian R. Jackson" Date: Sun, 26 Jul 2026 17:54:22 -0400 Subject: [PATCH 12/23] feat(ports): prompt before power-cycling on a TTY require_confirmation only hard-errors when piped; it never prompted, despite --yes documenting itself as skipping a prompt. ports cycle now shows what is about to lose power and requires an explicit yes. Declining reuses confirmation_required rather than adding an error kind. --- src/commands/ports.rs | 21 +++++++++- src/main.rs | 90 ++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 104 insertions(+), 7 deletions(-) diff --git a/src/commands/ports.rs b/src/commands/ports.rs index b4fc16b..b035307 100644 --- a/src/commands/ports.rs +++ b/src/commands/ports.rs @@ -567,11 +567,30 @@ pub fn cycle_summary(device: &DeviceWithPorts, port: &PortEntry) -> String { .as_deref() .map(format_mac) .unwrap_or_else(|| "-".into()); + let device_name = device + .name + .as_deref() + .or(device.model.as_deref()) + .unwrap_or("-"); let idx = port .port_idx .map(|i| i.to_string()) .unwrap_or_else(|| "?".into()); - format!("Port {idx} on {device_mac}") + let attached = port + .last_connection + .as_ref() + .filter(|lc| lc.connected.unwrap_or(false)) + .and_then(|lc| lc.mac.as_deref()) + .map(format_mac) + .unwrap_or_else(|| "nothing attached".into()); + let draw = match port.poe_power { + Some(w) if w > 0.0 => format!("{w:.2} W"), + _ => "0 W".into(), + }; + let class = port.poe_class.as_deref().unwrap_or("-"); + format!( + "Port {idx} on {device_name} ({device_mac})\n attached: {attached} • {draw} • {class}" + ) } #[cfg(test)] diff --git a/src/main.rs b/src/main.rs index b49b64d..0bd6e7e 100644 --- a/src/main.rs +++ b/src/main.rs @@ -385,6 +385,25 @@ fn require_confirmation(yes: bool, action: &str) { } } +/// Prompt for confirmation of a destructive action. Returns true only on an +/// explicit yes; an empty line, EOF, or anything else declines. +/// +/// Separate from `prompt_line`, which returns `InitError` and belongs to the +/// config-init flow. Reader/writer are injected so this is unit-testable +/// without a TTY. +fn confirm_destructive( + reader: &mut dyn std::io::BufRead, + writer: &mut dyn std::io::Write, + summary: &str, +) -> std::io::Result { + writeln!(writer, "{summary}")?; + write!(writer, "Power-cycle this port? (y/N): ")?; + writer.flush()?; + let mut line = String::new(); + reader.read_line(&mut line)?; + Ok(matches!(line.trim().to_lowercase().as_str(), "y" | "yes")) +} + fn print_schema() { schema::print_schema(Cli::command()); } @@ -1349,12 +1368,35 @@ async fn main() { } PortsCommand::Cycle { mac, port } => { require_confirmation(cli.yes, "power-cycle"); - // Task 9 replaces this always-proceed callback with the real - // TTY prompt. require_confirmation has already exited for the - // piped-without---yes case by this point. - commands::ports::cycle(&client, &mac, port, out, |_| Ok(true)) - .await - .map(|_| ()) + let skip_prompt = cli.yes; + let outcome = commands::ports::cycle(&client, &mac, port, out, |summary| { + if skip_prompt { + return Ok(true); + } + // Reached only on a TTY: require_confirmation already + // exited for the piped-without---yes case. + let mut stdin = std::io::stdin().lock(); + let mut stderr = std::io::stderr(); + confirm_destructive(&mut stdin, &mut stderr, summary) + }) + .await; + + // main() returns (), so `?` cannot be used here; match keeps + // this arm's value the same `Result<(), Box>` every + // other arm produces, so errors still flow through the single + // `if let Err(e) = result` handler below. + match outcome { + Ok(commands::ports::CycleOutcome::Cycled) => Ok(()), + Ok(commands::ports::CycleOutcome::Declined) => { + print_error_envelope( + "confirmation_required", + "Aborted: confirmation declined.", + None, + ); + std::process::exit(exit_codes::CONFIRMATION_REQUIRED); + } + Err(e) => Err(e), + } } }, Command::Events(cmd) => match cmd { @@ -1586,6 +1628,42 @@ api_key = "work_key" assert_eq!(host.as_deref(), Some("work.example.com")); } + // --- confirm_destructive --- + + #[test] + fn confirm_destructive_accepts_y_and_yes() { + for input in ["y\n", "Y\n", "yes\n", "YES\n"] { + let mut reader = std::io::BufReader::new(input.as_bytes()); + let mut writer: Vec = Vec::new(); + assert!( + confirm_destructive(&mut reader, &mut writer, "Port 4").unwrap(), + "{input:?} must confirm" + ); + } + } + + #[test] + fn confirm_destructive_declines_everything_else() { + for input in ["n\n", "no\n", "\n", "maybe\n", ""] { + let mut reader = std::io::BufReader::new(input.as_bytes()); + let mut writer: Vec = Vec::new(); + assert!( + !confirm_destructive(&mut reader, &mut writer, "Port 4").unwrap(), + "{input:?} must decline" + ); + } + } + + #[test] + fn confirm_destructive_shows_the_summary_and_default_no() { + let mut reader = std::io::BufReader::new(&b"n\n"[..]); + let mut writer: Vec = Vec::new(); + confirm_destructive(&mut reader, &mut writer, "Port 4 on SwitchA").unwrap(); + let shown = String::from_utf8(writer).unwrap(); + assert!(shown.contains("Port 4 on SwitchA"), "got: {shown}"); + assert!(shown.contains("(y/N)"), "default must read as No: {shown}"); + } + // --- mask_api_key --- #[test] From 0e8a53de3ba18053682e9d8b51e4aed6b3ce8add Mon Sep 17 00:00:00 2001 From: "Brian R. Jackson" Date: Sun, 26 Jul 2026 17:58:14 -0400 Subject: [PATCH 13/23] docs: document the ports command tree Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 13 +++++++++++++ README.md | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1bb3943..c379d3f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,19 @@ All notable changes to this project will be documented in this file. +## [Unreleased] + +### Added + +- **ports**: new `unifi ports` command tree — `list`, `show`, `find`, and `cycle` — for working with individual switch ports instead of whole devices. +- **ports cycle**: power-cycles a single PoE port via the `devmgr` `power-cycle` command, with pre-flight checks that reject non-PoE ports and administratively-disabled PoE before any request reaches the controller. Prompts for confirmation on a TTY; requires `--yes` when piped, otherwise exits 2 with `kind: confirmation_required`. +- **ports find**: resolves a MAC, an IP, or a client name (case-insensitive substring match) to the switch port(s) a device is attached to, via `port_table.last_connection`. A device that has moved between ports appears once per port, with `connected` marking its current one. A name matching more than one client returns `kind: conflict` (exit 6) listing the candidates rather than guessing. +- **ports show**: exposes PoE telemetry the CLI previously discarded — `poe_mode`, `poe_class`, `poe_voltage`, `poe_current`, `poe_good` — plus `attached_mac`. +- The `conflict` error kind (exit code 6), already advertised in `unifi schema`'s error table since 0.2.2 but never emitted by any code path until now. + +### Changed + +- **devices ports**: `unifi devices ports ` is now an alias for `unifi ports list `. Its JSON output gains `device_mac` and `device_name`; every previously emitted key is unchanged, and it keeps its historical bare-JSON-array shape for backward compatibility. ## [0.3.0](https://github.com/rvben/unifi-cli/compare/v0.2.3...v0.3.0) - 2026-07-09 diff --git a/README.md b/README.md index 7426afc..844e31f 100644 --- a/README.md +++ b/README.md @@ -152,6 +152,57 @@ unifi devices locate aa:bb:cc:dd:ee:ff # Blink locate LED unifi devices locate aa:bb:cc:dd:ee:ff --off # Stop blinking ``` +### Ports + +Find which switch port a device is plugged into, then power-cycle just that +port instead of rebooting the whole switch: + +```bash +# Which port is my Pi on? Matches by name (case-insensitive substring), +# MAC, or IP. +unifi ports find allsky +unifi ports find d8:3a:dd:2b:fa:8a + +# Inspect it — PoE mode, class, voltage, current, and what's attached +unifi ports show 8c:ed:e1:b0:74:e2 5 + +# Bounce PoE on that port only — the rest of the switch is untouched +unifi ports cycle 8c:ed:e1:b0:74:e2 5 +``` + +`ports find`'s output feeds directly into `show` and `cycle`: `device_mac` +and `port_idx` are the *switch's* MAC and port index, not the attached +device's. A name matching more than one client returns `kind: conflict` +(exit 6) listing the candidates rather than guessing. A device that has +moved between switch ports appears once per port it has ever used, with a +`connected` field distinguishing its current port from stale history. + +`ports show` exposes PoE telemetry the CLI previously discarded: +`poe_mode`, `poe_class`, `poe_voltage`, `poe_current`, `poe_good`, and the +MAC of the attached device (`attached_mac`). + +`ports cycle` is destructive. On a terminal it shows what is about to lose +power and asks for confirmation; when piped it requires `--yes` and +otherwise exits 2 with `kind: confirmation_required`. It refuses **before +contacting the controller** when: + +- the port is not PoE-capable (an SFP+ port, say) → `kind: conflict`, exit 6 +- the port's PoE is administratively off → `kind: conflict`, exit 6 +- the device has no such port index → `kind: not_found`, exit 4 + +List ports for one device, or across every device: + +```bash +unifi ports list 8c:ed:e1:b0:74:e2 +unifi ports list --limit 20 --fields port_idx,poe_power +``` + +`ports list` returns the paginated `{items, total, limit, offset}` envelope +used by the other list commands. `unifi devices ports ` remains an +alias for `unifi ports list `; it keeps its original bare-JSON-array +shape for backward compatibility, and both emit the same per-row fields, +including `device_mac` and `device_name`. + ### Events ```bash @@ -239,6 +290,7 @@ unifi schema # Dumps all commands, arguments, output fields as JSON | 3 | Authentication error (401/403) | | 4 | Not found (404) | | 5 | API error (server error) | +| 6 | Conflict (ambiguous match or failed precondition) | ## Development From 6dcebf27a0d0a915d8f6c87a8d5e304f4cc936c0 Mon Sep 17 00:00:00 2001 From: "Brian R. Jackson" Date: Sun, 26 Jul 2026 18:14:13 -0400 Subject: [PATCH 14/23] test(ports): cover the cycle() mutation path and cycle_summary `power_cycle_port_sends_correct_command` only pinned down the client method's endpoint/body, never the orchestration in commands::ports::cycle that decides whether to call it at all. Add mock-server tests for all four outcomes (confirmed, declined, non-PoE conflict, missing port), using `.expect(0)` on the devmgr mock to prove positively that decline/conflict/not-found never reach the controller. Also add unit tests for cycle_summary, the prompt text an operator reads before authorising a power cut: the connected-vs-stale last_connection filter and the watt formatting were previously untested. --- src/commands/ports.rs | 64 +++++++++++++++ tests/mock_server.rs | 176 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 240 insertions(+) diff --git a/src/commands/ports.rs b/src/commands/ports.rs index b035307..ca16846 100644 --- a/src/commands/ports.rs +++ b/src/commands/ports.rs @@ -801,6 +801,70 @@ mod tests { assert!(check_cyclable(p, "74:ac:b9:ec:b4:5e").is_ok()); } + // `cycle_summary` is the text a human reads before authorising a power + // cut. Untested, it carries real logic that would be easy to invert or + // drop silently: the `connected` filter on `last_connection`, and the + // watt formatting. + + #[test] + fn cycle_summary_shows_the_attached_mac_when_connected() { + let d = device_with(serde_json::json!([{ + "port_idx": 4, "port_poe": true, + "last_connection": {"mac": "d8:3a:dd:2b:fa:8a", "connected": true} + }])); + let p = find_port(&d, 4).unwrap(); + let summary = cycle_summary(&d, p); + assert!( + summary.contains("d8:3a:dd:2b:fa:8a"), + "a connected last_connection must show the formatted attached MAC: {summary}" + ); + } + + #[test] + fn cycle_summary_reads_nothing_attached_for_a_stale_record() { + // connected: false is history, not the device's current location; the + // summary must not read as if a live device would lose power. + let d = device_with(serde_json::json!([{ + "port_idx": 4, "port_poe": true, + "last_connection": {"mac": "d8:3a:dd:2b:fa:8a", "connected": false} + }])); + let p = find_port(&d, 4).unwrap(); + let summary = cycle_summary(&d, p); + assert!( + summary.contains("nothing attached"), + "a stale (disconnected) last_connection must read as unattached: {summary}" + ); + assert!( + !summary.contains("d8:3a:dd:2b:fa:8a"), + "a stale MAC must not appear as if it were live: {summary}" + ); + } + + #[test] + fn cycle_summary_reads_nothing_attached_when_no_last_connection() { + let d = device_with(serde_json::json!([{"port_idx": 4, "port_poe": true}])); + let p = find_port(&d, 4).unwrap(); + let summary = cycle_summary(&d, p); + assert!( + summary.contains("nothing attached"), + "an absent last_connection must read as unattached: {summary}" + ); + } + + #[test] + fn cycle_summary_shows_the_wattage_for_a_powered_port() { + let d = device_with(serde_json::json!([{ + "port_idx": 4, "port_poe": true, "poe_enable": true, + "poe_power": 5.25, "poe_class": "4" + }])); + let p = find_port(&d, 4).unwrap(); + let summary = cycle_summary(&d, p); + assert!( + summary.contains("5.25 W"), + "draw must be formatted to two decimal places: {summary}" + ); + } + // `_id` is required by `LegacyClient` (every other fixture in this codebase // supplies it); the plan's literal fixture omitted it, so it is added here // to make the fixture actually deserialize. diff --git a/tests/mock_server.rs b/tests/mock_server.rs index dff3e83..4ff2f36 100644 --- a/tests/mock_server.rs +++ b/tests/mock_server.rs @@ -2356,6 +2356,182 @@ mod command_output { assert!(message.contains("Main-Bedroom"), "got: {message}"); } + // --- Ports cycle (mutation orchestration) --- + // + // `power_cycle_port_sends_correct_command` (in `client_api` above) only + // covers the client method's endpoint and body. Nothing exercised the + // orchestration in `commands::ports::cycle` that decides *whether* to call + // it at all — and that orchestration is the only place in this CLI that + // cuts power to physical hardware. These four cases pin down the guard-rail + // ordering (find_port -> check_cyclable -> confirm -> POST) as a tested + // property rather than a code-reading exercise: the `.expect(0)` mounts on + // decline/conflict/not-found assert, via wiremock's mount-drop + // verification, that no HTTP write happens on any of the three + // non-cycling paths. + + #[tokio::test] + async fn ports_cycle_confirmed_cycles_the_port() { + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/proxy/network/api/s/default/stat/device")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "meta": {"rc": "ok"}, + "data": [{ + "mac": "9c:05:d6:bc:06:43", "name": "USW-24-PoE", + "port_table": [{"port_idx": 5, "port_poe": true, "poe_mode": "auto"}] + }] + }))) + .expect(1) + .mount(&server) + .await; + Mock::given(method("POST")) + .and(path("/proxy/network/api/s/default/cmd/devmgr")) + .and(body_json(serde_json::json!({ + "cmd": "power-cycle", + "mac": "9c:05:d6:bc:06:43", + "port_idx": 5 + }))) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "meta": {"rc": "ok"}, + "data": [] + }))) + .expect(1) + .mount(&server) + .await; + + let client = mock_client(&server).await; + let outcome = + unifi_cli::commands::ports::cycle(&client, "9c:05:d6:bc:06:43", 5, out_table(), |_| { + Ok(true) + }) + .await + .unwrap(); + assert_eq!(outcome, unifi_cli::commands::ports::CycleOutcome::Cycled); + } + + #[tokio::test] + async fn ports_cycle_declined_never_posts() { + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/proxy/network/api/s/default/stat/device")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "meta": {"rc": "ok"}, + "data": [{ + "mac": "9c:05:d6:bc:06:43", "name": "USW-24-PoE", + "port_table": [{"port_idx": 5, "port_poe": true, "poe_mode": "auto"}] + }] + }))) + .expect(1) + .mount(&server) + .await; + Mock::given(method("POST")) + .and(path("/proxy/network/api/s/default/cmd/devmgr")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "meta": {"rc": "ok"}, + "data": [] + }))) + .expect(0) + .mount(&server) + .await; + + let client = mock_client(&server).await; + let outcome = + unifi_cli::commands::ports::cycle(&client, "9c:05:d6:bc:06:43", 5, out_table(), |_| { + Ok(false) + }) + .await + .unwrap(); + assert_eq!(outcome, unifi_cli::commands::ports::CycleOutcome::Declined); + } + + #[tokio::test] + async fn ports_cycle_non_poe_port_is_conflict_and_never_posts() { + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/proxy/network/api/s/default/stat/device")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "meta": {"rc": "ok"}, + "data": [{ + "mac": "9c:05:d6:bc:06:43", "name": "USW-Lite-8", + "port_table": [{"port_idx": 9, "port_poe": false}] + }] + }))) + .expect(1) + .mount(&server) + .await; + Mock::given(method("POST")) + .and(path("/proxy/network/api/s/default/cmd/devmgr")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "meta": {"rc": "ok"}, + "data": [] + }))) + .expect(0) + .mount(&server) + .await; + + let client = mock_client(&server).await; + // The confirm callback returns `Ok(true)` deliberately: `check_cyclable` + // must reject before `confirm` is ever consulted, so a callback that + // would approve proves nothing about ordering unless it's wired to run + // second. + let err = unifi_cli::commands::ports::cycle(&client, "9c:05:d6:bc:06:43", 9, out_table(), |_| { + Ok(true) + }) + .await + .unwrap_err(); + let api_err = err + .downcast_ref::() + .unwrap_or_else(|| panic!("cycle must reject a non-PoE port as an ApiError, got {err}")); + assert!( + matches!(api_err, unifi_cli::api::ApiError::Conflict(_)), + "expected Conflict, got {api_err:?}" + ); + } + + #[tokio::test] + async fn ports_cycle_missing_port_is_not_found_and_never_posts() { + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/proxy/network/api/s/default/stat/device")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "meta": {"rc": "ok"}, + "data": [{ + "mac": "9c:05:d6:bc:06:43", "name": "USW-24-PoE", + "port_table": [{"port_idx": 1, "port_poe": true}] + }] + }))) + .expect(1) + .mount(&server) + .await; + Mock::given(method("POST")) + .and(path("/proxy/network/api/s/default/cmd/devmgr")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "meta": {"rc": "ok"}, + "data": [] + }))) + .expect(0) + .mount(&server) + .await; + + let client = mock_client(&server).await; + let err = unifi_cli::commands::ports::cycle( + &client, + "9c:05:d6:bc:06:43", + 99, + out_table(), + |_| Ok(true), + ) + .await + .unwrap_err(); + let api_err = err + .downcast_ref::() + .unwrap_or_else(|| panic!("cycle must report a missing port as an ApiError, got {err}")); + assert!( + matches!(api_err, unifi_cli::api::ApiError::NotFound(_)), + "expected NotFound, got {api_err:?}" + ); + } + #[tokio::test] async fn list_events_returns_stat_event_records() { let server = MockServer::start().await; From 9acd530e257d951edf36a5793a413c4d9bcd77bd Mon Sep 17 00:00:00 2001 From: "Brian R. Jackson" Date: Sun, 26 Jul 2026 18:15:24 -0400 Subject: [PATCH 15/23] fix(ports): terminate decline prompt line, widen confirmation_required doc confirm_destructive wrote the y/N prompt without a trailing newline; the user's Enter is echoed by the terminal, not this stream, so with a TTY stdin and redirected stderr the confirmation_required envelope printed on decline landed on the same physical line as the prompt, breaking the "envelope is the last line of stderr" contract. Terminate the prompt line explicitly after read_line. Also widen the confirmation_required error kind's schema description: it's now produced by both a missing --yes on a non-TTY and an interactive decline, and an agent reading `unifi schema` couldn't previously learn about the second producer. No kind/exit_code/retryable change. Document, at the port_poe guard rail in check_cyclable, that firmware omitting the key also fails closed there deliberately (contrast the Option treatment of autoneg/enable/is_uplink a few lines below). --- src/commands/ports.rs | 6 ++++++ src/main.rs | 26 ++++++++++++++++++++++++++ src/schema.rs | 2 +- 3 files changed, 33 insertions(+), 1 deletion(-) diff --git a/src/commands/ports.rs b/src/commands/ports.rs index ca16846..8801af3 100644 --- a/src/commands/ports.rs +++ b/src/commands/ports.rs @@ -494,6 +494,12 @@ pub fn check_cyclable(port: &PortEntry, device_mac: &str) -> Result<(), ApiError .unwrap_or_else(|| "?".into()); let mac = format_mac(device_mac); + // `port_poe` is `#[serde(default)] bool` (see src/api/types.rs), so + // firmware that simply omits the key also lands here, indistinguishable + // from a genuinely non-PoE port. That is deliberate: for a command that + // cuts power, failing closed is the right direction. It does mean the + // message/hint below can fire for PoE-capable hardware whose firmware + // didn't report the field, not only for true non-PoE ports. if !port.port_poe { return Err(ApiError::Conflict(format!( "Port {idx} on {mac} does not support PoE. \ diff --git a/src/main.rs b/src/main.rs index 0bd6e7e..ff06a47 100644 --- a/src/main.rs +++ b/src/main.rs @@ -401,6 +401,14 @@ fn confirm_destructive( writer.flush()?; let mut line = String::new(); reader.read_line(&mut line)?; + // The user's Enter is echoed by the terminal, not written to this stream, + // so without this the prompt line above stays unterminated on our writer. + // With stdin a TTY and stderr redirected, a decline's error envelope + // (printed with `eprintln!` right after) would then land on the same + // physical line as the prompt instead of starting fresh — breaking the + // "envelope is the last line of stderr" contract (tests/cli_contract.rs, + // `error_envelope_last_line_is_json` in tests/spec_compliance.rs). + writeln!(writer)?; Ok(matches!(line.trim().to_lowercase().as_str(), "y" | "yes")) } @@ -1664,6 +1672,24 @@ api_key = "work_key" assert!(shown.contains("(y/N)"), "default must read as No: {shown}"); } + #[test] + fn confirm_destructive_terminates_the_prompt_line_with_a_newline() { + // The user's Enter is echoed by the terminal, not by this writer, so + // the prompt's own `write!` leaves the stream mid-line unless + // `confirm_destructive` terminates it itself. A subsequent + // `eprintln!` (e.g. the confirmation_required envelope printed on + // decline) must start on a fresh line, not get appended to the + // prompt. + let mut reader = std::io::BufReader::new(&b"n\n"[..]); + let mut writer: Vec = Vec::new(); + confirm_destructive(&mut reader, &mut writer, "Port 4 on SwitchA").unwrap(); + let shown = String::from_utf8(writer).unwrap(); + assert!( + shown.ends_with('\n'), + "prompt output must end with a newline so a following line starts clean: {shown:?}" + ); + } + // --- mask_api_key --- #[test] diff --git a/src/schema.rs b/src/schema.rs index aea30c2..a75296d 100644 --- a/src/schema.rs +++ b/src/schema.rs @@ -547,7 +547,7 @@ pub fn print_schema(cmd: clap::Command) { "kind": "confirmation_required", "exit_code": 2, "retryable": false, - "description": "Destructive command requires --yes flag when stdin is not a terminal", + "description": "Confirmation for a destructive command was not obtained: either --yes was omitted while stdin is not a terminal, or the operator declined at an interactive confirmation prompt", }, { "kind": "auth_error", From 86c0df2793dce3a1a14363160e6d1b947f0a8081 Mon Sep 17 00:00:00 2001 From: "Brian R. Jackson" Date: Sun, 26 Jul 2026 18:20:39 -0400 Subject: [PATCH 16/23] refactor(ports): boy-scout cleanup backlog (B1-B8) - Restore devices ports's historical name -> model -> "Device" label fallback, lost when it was routed through the shared collect_rows (which keeps its "-" fallback for ports list / ports find). Threaded through a new collect_rows_with_fallback so the two call sites don't duplicate the whole flattening loop over one fallback string. - Extract device_identity as the single place that derives a port row's device_mac/device_name, used by both show() and collect_rows_with_fallback() (previously duplicated verbatim). - Replace devices.rs's four crate::commands::ports::* fully-qualified paths with a use crate::commands::ports::{self, PortRow}, matching the file's existing import style. - Compute render_text's Device column width from the full result set via a new device_col_width helper, not the paginated page it's handed, so two --offset pages of the same ports list query render with the same column width. - Give autoneg/enable/is_uplink each their own doc comment; the shared tri-state rationale previously sat only above autoneg. - Add a test pinning ApiError::Conflict's no-added-prefix Display contract, and add .expect(1) to list_all_device_ports_returns_every_device's mock for consistency with its sibling tests. Includes tests for all of the above, run via the real binary where the existing suite's convention already spawns it (devices ports label fallback, ports show text formatting, ports list column stability). --- src/api/tests.rs | 11 +++ src/api/types.rs | 12 ++- src/commands/devices.rs | 13 ++- src/commands/ports.rs | 110 +++++++++++++++------- tests/mock_server.rs | 203 ++++++++++++++++++++++++++++++++++++++-- 5 files changed, 301 insertions(+), 48 deletions(-) diff --git a/src/api/tests.rs b/src/api/tests.rs index ec9b8bb..4e45190 100644 --- a/src/api/tests.rs +++ b/src/api/tests.rs @@ -145,6 +145,17 @@ fn api_error_display_auth() { assert!(display.contains("Hint:")); } +#[test] +fn api_error_display_conflict_has_no_prefix() { + // Unlike Auth/NotFound (sentence fragments that get a prefix added), + // Conflict messages are written whole, to be read by an operator as-is. + let err = ApiError::Conflict("Port 5 on aa:bb:cc:dd:ee:ff does not support PoE.".into()); + assert_eq!( + err.to_string(), + "Port 5 on aa:bb:cc:dd:ee:ff does not support PoE." + ); +} + #[test] fn api_error_display_other() { let err = ApiError::Other("something went wrong".into()); diff --git a/src/api/types.rs b/src/api/types.rs index ad6d6f8..de63296 100644 --- a/src/api/types.rs +++ b/src/api/types.rs @@ -310,12 +310,16 @@ pub struct PortEntry { #[serde(default, deserialize_with = "deserialize_string_or_number_f64")] pub poe_current: Option, pub poe_good: Option, - /// Option, not a defaulted bool: a firmware that omits these keys must not - /// be reported as "auto-negotiation off" or "port administratively - /// disabled". Matches `poe_good` above; contrast `up`/`poe_enable`, where - /// an absent key genuinely does mean false. + /// Auto-negotiation state. `Option`, not a defaulted bool, like `enable` + /// and `is_uplink` below: a firmware that omits this key must not be + /// reported as "auto-negotiation off". Matches `poe_good` above; contrast + /// `up`/`poe_enable`, where an absent key genuinely does mean false. pub autoneg: Option, + /// Administrative enable state. Same tri-state rationale as `autoneg`: an + /// absent key must not be reported as "port administratively disabled". pub enable: Option, + /// Whether this port is the switch's uplink. Same tri-state rationale as + /// `autoneg`: an absent key must not be reported as "not an uplink". pub is_uplink: Option, pub stp_state: Option, pub tx_errors: Option, diff --git a/src/commands/devices.rs b/src/commands/devices.rs index 53f233a..68d4d66 100644 --- a/src/commands/devices.rs +++ b/src/commands/devices.rs @@ -1,6 +1,7 @@ use owo_colors::OwoColorize; use crate::api::{Device, UnifiClient, format_mac, format_uptime}; +use crate::commands::ports::{self, PortRow}; use crate::output::{OutputConfig, use_color}; pub struct Pagination { @@ -273,17 +274,19 @@ pub async fn ports( } let devices = vec![device]; - let rows = crate::commands::ports::collect_rows(&devices); + // Historical label for a device with neither `name` nor `model`; `ports + // list` / `ports find` keep "-" via `collect_rows`. + let rows = ports::collect_rows_with_fallback(&devices, "Device"); if out.is_json() { - let items: Vec = - rows.iter().map(crate::commands::ports::row_json).collect(); + let items: Vec = rows.iter().map(ports::row_json).collect(); out.print_data(&serde_json::to_string_pretty(&items)?); } else { let label = &rows[0].device_name; out.print_message(&format!("Ports for {label}:\n")); - let refs: Vec<&crate::commands::ports::PortRow> = rows.iter().collect(); - crate::commands::ports::render_text(&refs, false, &out); + let refs: Vec<&PortRow> = rows.iter().collect(); + let dev_w = ports::device_col_width(&refs); + ports::render_text(&refs, false, dev_w, &out); } Ok(()) } diff --git a/src/commands/ports.rs b/src/commands/ports.rs index 8801af3..738b2bc 100644 --- a/src/commands/ports.rs +++ b/src/commands/ports.rs @@ -21,24 +21,45 @@ pub struct Pagination { pub fields: Option>, } +/// Derive a port row's formatted device MAC and display name: `name` -> +/// `model` -> `name_fallback`. Shared by `show` and `collect_rows_with_fallback` +/// so this three-tier fallback can never drift between the two call sites. +fn device_identity(device: &DeviceWithPorts, name_fallback: &str) -> (String, String) { + let device_mac = device + .mac + .as_deref() + .map(format_mac) + .unwrap_or_else(|| "-".into()); + let device_name = device + .name + .as_deref() + .or(device.model.as_deref()) + .unwrap_or(name_fallback) + .to_string(); + (device_mac, device_name) +} + /// Flatten devices into port rows, skipping devices with no port table. +/// `ports list` / `ports find` fall back to `"-"` for a device with neither +/// `name` nor `model`. pub fn collect_rows(devices: &[DeviceWithPorts]) -> Vec> { + collect_rows_with_fallback(devices, "-") +} + +/// Same flattening as `collect_rows`, but with a caller-chosen fallback for a +/// device that has neither `name` nor `model`. `devices ports` keeps its +/// historical `"Device"` label here rather than duplicating the whole +/// flattening loop just to change one fallback string. +pub fn collect_rows_with_fallback<'a>( + devices: &'a [DeviceWithPorts], + name_fallback: &str, +) -> Vec> { let mut rows = Vec::new(); for d in devices { if d.port_table.is_empty() { continue; } - let device_mac = d - .mac - .as_deref() - .map(format_mac) - .unwrap_or_else(|| "-".into()); - let device_name = d - .name - .as_deref() - .or(d.model.as_deref()) - .unwrap_or("-") - .to_string(); + let (device_mac, device_name) = device_identity(d, name_fallback); for port in &d.port_table { rows.push(PortRow { device_mac: device_mac.clone(), @@ -103,18 +124,26 @@ fn speed_cell(p: &PortEntry) -> String { } } -/// Render rows as a table. `show_device_col` is true only for the unfiltered -/// listing; the filtered table stays byte-identical to what `devices ports` -/// has always printed. -pub fn render_text(rows: &[&PortRow], show_device_col: bool, out: &OutputConfig) { - let color = use_color(); - let dev_w = rows - .iter() +/// Device column width, in characters. Callers that paginate must compute +/// this from the full result set, not just the page handed to `render_text` +/// — otherwise two `--offset` pages of the same query can render the column +/// at different widths. +pub fn device_col_width(rows: &[&PortRow]) -> usize { + rows.iter() .map(|r| r.device_name.len()) .max() .unwrap_or(6) .max(6) - + 2; + + 2 +} + +/// Render rows as a table. `show_device_col` is true only for the unfiltered +/// listing; the filtered table stays byte-identical to what `devices ports` +/// has always printed. `dev_w` is the Device column width; pass +/// `device_col_width` of the *full* result set, not just `rows`, so a +/// paginated caller renders a stable width across pages. +pub fn render_text(rows: &[&PortRow], show_device_col: bool, dev_w: usize, out: &OutputConfig) { + let color = use_color(); let header = if show_device_col { format!( @@ -207,7 +236,12 @@ pub async fn list( "offset": pagination.offset, }))?); } else { - render_text(&page, mac.is_none(), &out); + // Computed from the full `rows`, not the paginated `page`, so two + // `--offset` pages of the same query render the Device column at the + // same width. + let full_refs: Vec<&PortRow> = rows.iter().collect(); + let dev_w = device_col_width(&full_refs); + render_text(&page, mac.is_none(), dev_w, &out); } Ok(()) } @@ -360,7 +394,9 @@ pub async fn find( out.print_data(&serde_json::to_string_pretty(&items)?); } else { let refs: Vec<&PortRow> = hits.iter().map(|(r, _)| *r).collect(); - render_text(&refs, true, &out); + // `find` never paginates, so `refs` is already the full result set. + let dev_w = device_col_width(&refs); + render_text(&refs, true, dev_w, &out); } Ok(()) } @@ -373,17 +409,7 @@ pub async fn show( ) -> Result<(), Box> { let device = client.get_device_ports(mac).await?; let p = find_port(&device, port_idx)?; - let device_mac = device - .mac - .as_deref() - .map(format_mac) - .unwrap_or_else(|| "-".into()); - let device_name = device - .name - .as_deref() - .or(device.model.as_deref()) - .unwrap_or("-") - .to_string(); + let (device_mac, device_name) = device_identity(&device, "-"); let attached_mac = p .last_connection .as_ref() @@ -683,6 +709,26 @@ mod tests { ); } + #[test] + fn collect_rows_with_fallback_uses_the_caller_supplied_fallback() { + // `devices ports` restores the historical "Device" label for a + // device with neither `name` nor `model`; `collect_rows` (used by + // `ports list` / `ports find`) must keep falling back to "-". + let devices = vec![device(serde_json::json!({ + "mac": "aa:bb:cc:dd:ee:ff", + "port_table": [{"port_idx": 1}] + }))]; + + let fallback_rows = collect_rows_with_fallback(&devices, "Device"); + assert_eq!(fallback_rows[0].device_name, "Device"); + + let default_rows = collect_rows(&devices); + assert_eq!( + default_rows[0].device_name, "-", + "collect_rows must still fall back to '-', unaffected by the new parameter" + ); + } + #[test] fn row_json_emits_exactly_the_fields_declared_in_ports_list() { let devices = vec![device(serde_json::json!({ diff --git a/tests/mock_server.rs b/tests/mock_server.rs index 4ff2f36..d2e1cfc 100644 --- a/tests/mock_server.rs +++ b/tests/mock_server.rs @@ -538,6 +538,7 @@ mod client_api { "port_table": [{"port_idx": 1}, {"port_idx": 2}]} ] }))) + .expect(1) .mount(&server) .await; @@ -1791,6 +1792,58 @@ mod command_output { assert!(err.to_string().contains("Not found")); } + // `devices::ports` used to derive its own `name -> model -> "Device"` + // device-label fallback; routing it through the shared `collect_rows` + // silently changed the fallback to "-" for a device with neither `name` + // nor `model`, and nothing caught it. Drives the real binary (JSON is + // easiest to assert on) so the regression is locked in at the command + // level, not just in the `collect_rows_with_fallback` unit test in + // `src/commands/ports.rs`. + #[tokio::test] + async fn devices_ports_falls_back_to_device_label_when_name_and_model_absent() { + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/proxy/network/api/s/default/stat/device")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "meta": {"rc": "ok"}, + "data": [{ + "mac": "aa:bb:cc:dd:ee:ff", + "port_table": [{"port_idx": 1}] + }] + }))) + .mount(&server) + .await; + + let output = std::process::Command::new(env!("CARGO_BIN_EXE_unifi")) + .args([ + "--host", + &server.uri(), + "--api-key", + "test-key", + "devices", + "ports", + "aa:bb:cc:dd:ee:ff", + "-o", + "json", + ]) + .output() + .expect("failed to run the unifi binary"); + assert!( + output.status.success(), + "devices ports failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + + let body: serde_json::Value = serde_json::from_slice(&output.stdout).unwrap(); + let items = body + .as_array() + .expect("devices ports must emit a bare JSON array"); + assert_eq!( + items[0]["device_name"], "Device", + "devices ports must keep its historical \"Device\" fallback, not \"-\": {items:?}" + ); + } + // --- Ports show (single-port detail) --- #[tokio::test] @@ -1826,6 +1879,72 @@ mod command_output { .unwrap(); } + // `ports_show_table` above only smoke-tests that the text branch doesn't + // panic (matching the pre-existing convention for other `show`-style + // commands in this file). But `ports show`'s text branch is new code with + // real formatting logic (speed_cell, poe_cell, voltage/current, the + // attached MAC), so it gets its own test that spawns the real binary + // (same pattern as `ports_list_pagination_reports_full_total_and_truncated_items` + // and `devices_ports_bare_array_vs_ports_list_envelope`) and asserts on + // the actual rendered text. + #[tokio::test] + async fn ports_show_text_output_renders_expected_fields() { + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/proxy/network/api/s/default/stat/device")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "meta": {"rc": "ok"}, + "data": [{ + "mac": "9c:05:d6:bc:06:43", "name": "USW-24-PoE", + "port_table": [{ + "port_idx": 5, "name": "Port 5", "media": "GE", "up": true, + "speed": 1000, "full_duplex": true, + "port_poe": true, "poe_enable": true, "poe_mode": "auto", + "poe_class": "4", "poe_power": 5.2, "poe_voltage": 53.5, + "poe_current": 120.3, + "last_connection": {"mac": "aabbccddeeff", "connected": true} + }] + }] + }))) + .mount(&server) + .await; + + let output = std::process::Command::new(env!("CARGO_BIN_EXE_unifi")) + .args([ + "--host", + &server.uri(), + "--api-key", + "test-key", + "ports", + "show", + "9c:05:d6:bc:06:43", + "5", + "--output", + "text", + ]) + .output() + .expect("failed to run the unifi binary"); + assert!( + output.status.success(), + "ports show failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + + let text = String::from_utf8_lossy(&output.stdout); + assert!( + text.contains("Port 5 on USW-24-PoE (9c:05:d6:bc:06:43)"), + "title line: {text}" + ); + assert!(text.contains("Port 5"), "port name: {text}"); + assert!(text.contains("1000FD"), "speed+duplex formatting: {text}"); + assert!(text.contains("GE"), "media: {text}"); + assert!(text.contains("5.2W"), "PoE wattage: {text}"); + assert!(text.contains("auto"), "PoE mode: {text}"); + assert!(text.contains("53.50 V"), "PoE voltage: {text}"); + assert!(text.contains("120.30 mA"), "PoE current: {text}"); + assert!(text.contains("aa:bb:cc:dd:ee:ff"), "attached MAC: {text}"); + } + // Drives the real `unifi` binary so the JSON this command actually prints // can be inspected, and cross-checks it against what `unifi schema` // publishes for "ports show" — the two are supposed to be the same @@ -2101,6 +2220,71 @@ mod command_output { assert_eq!(body["offset"], 1); } + // `render_text`'s Device column width used to be derived from whatever + // page it was handed, which for `ports list` is the already-paginated + // page. Two `--offset` pages of the same query could then render the + // column at different widths. The device names below are chosen so the + // longest one falls on the second page only; if the width regressed back + // to being page-local, the two headers would render at different widths + // and this comparison would fail. + #[tokio::test] + async fn ports_list_device_column_width_is_stable_across_pages() { + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/proxy/network/api/s/default/stat/device")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "meta": {"rc": "ok"}, + "data": [ + {"mac": "aa:bb:cc:dd:ee:01", "name": "SwitchA", + "port_table": [{"port_idx": 1}, {"port_idx": 2}]}, + {"mac": "aa:bb:cc:dd:ee:02", "name": "A-Very-Long-Switch-Name", + "port_table": [{"port_idx": 1}]} + ] + }))) + .mount(&server) + .await; + + let run_text = |limit: &str, offset: &str| -> String { + let output = std::process::Command::new(env!("CARGO_BIN_EXE_unifi")) + .args([ + "--host", + &server.uri(), + "--api-key", + "test-key", + "ports", + "list", + "--output", + "text", + "--limit", + limit, + "--offset", + offset, + ]) + .output() + .expect("failed to run the unifi binary"); + assert!(output.status.success()); + String::from_utf8_lossy(&output.stdout).into_owned() + }; + + // Page 1: only SwitchA's two ports (the long name lives on page 2). + let page1 = run_text("2", "0"); + // Page 2: only the long-named device's one port. + let page2 = run_text("1", "2"); + + fn header(s: &str) -> &str { + s.lines() + .find(|l| l.contains("Device")) + .expect("text output must have a header row containing \"Device\"") + } + assert_eq!( + header(&page1), + header(&page2), + "the Device column width must come from the full result set, not the page, \ + so two --offset pages of the same query render an identical header:\n\ + page1: {page1}\npage2: {page2}" + ); + } + // `devices ports ` is documented as an alias for `ports list ` // that deliberately keeps the historical bare JSON array shape, while // `ports list` emits the paginated `{items,total,limit,offset}` envelope. @@ -2474,14 +2658,17 @@ mod command_output { // must reject before `confirm` is ever consulted, so a callback that // would approve proves nothing about ordering unless it's wired to run // second. - let err = unifi_cli::commands::ports::cycle(&client, "9c:05:d6:bc:06:43", 9, out_table(), |_| { - Ok(true) - }) - .await - .unwrap_err(); + let err = + unifi_cli::commands::ports::cycle(&client, "9c:05:d6:bc:06:43", 9, out_table(), |_| { + Ok(true) + }) + .await + .unwrap_err(); let api_err = err .downcast_ref::() - .unwrap_or_else(|| panic!("cycle must reject a non-PoE port as an ApiError, got {err}")); + .unwrap_or_else(|| { + panic!("cycle must reject a non-PoE port as an ApiError, got {err}") + }); assert!( matches!(api_err, unifi_cli::api::ApiError::Conflict(_)), "expected Conflict, got {api_err:?}" @@ -2525,7 +2712,9 @@ mod command_output { .unwrap_err(); let api_err = err .downcast_ref::() - .unwrap_or_else(|| panic!("cycle must report a missing port as an ApiError, got {err}")); + .unwrap_or_else(|| { + panic!("cycle must report a missing port as an ApiError, got {err}") + }); assert!( matches!(api_err, unifi_cli::api::ApiError::NotFound(_)), "expected NotFound, got {api_err:?}" From fb36a0fdbd8e1b74cda211e83f136c51968c8a51 Mon Sep 17 00:00:00 2001 From: "Brian R. Jackson" Date: Sun, 26 Jul 2026 18:23:55 -0400 Subject: [PATCH 17/23] docs: correct when ports cycle refuses The docs claimed the pre-flight checks fire 'before contacting the controller'. They do not: cycle() reads the port table via /stat/device first, then applies the guard rails. Only the destructive power-cycle command is withheld. A reader could otherwise assume a conflict/not_found refusal implies no network activity, or that it works without valid credentials -- neither is true. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 2 +- README.md | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c379d3f..0a2d352 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ All notable changes to this project will be documented in this file. ### Added - **ports**: new `unifi ports` command tree — `list`, `show`, `find`, and `cycle` — for working with individual switch ports instead of whole devices. -- **ports cycle**: power-cycles a single PoE port via the `devmgr` `power-cycle` command, with pre-flight checks that reject non-PoE ports and administratively-disabled PoE before any request reaches the controller. Prompts for confirmation on a TTY; requires `--yes` when piped, otherwise exits 2 with `kind: confirmation_required`. +- **ports cycle**: power-cycles a single PoE port via the `devmgr` `power-cycle` command, with pre-flight checks that reject non-PoE ports and administratively-disabled PoE without ever sending the power-cycle command (the port table is read first). Prompts for confirmation on a TTY; requires `--yes` when piped, otherwise exits 2 with `kind: confirmation_required`. - **ports find**: resolves a MAC, an IP, or a client name (case-insensitive substring match) to the switch port(s) a device is attached to, via `port_table.last_connection`. A device that has moved between ports appears once per port, with `connected` marking its current one. A name matching more than one client returns `kind: conflict` (exit 6) listing the candidates rather than guessing. - **ports show**: exposes PoE telemetry the CLI previously discarded — `poe_mode`, `poe_class`, `poe_voltage`, `poe_current`, `poe_good` — plus `attached_mac`. - The `conflict` error kind (exit code 6), already advertised in `unifi schema`'s error table since 0.2.2 but never emitted by any code path until now. diff --git a/README.md b/README.md index 844e31f..98311af 100644 --- a/README.md +++ b/README.md @@ -183,8 +183,9 @@ MAC of the attached device (`attached_mac`). `ports cycle` is destructive. On a terminal it shows what is about to lose power and asks for confirmation; when piped it requires `--yes` and -otherwise exits 2 with `kind: confirmation_required`. It refuses **before -contacting the controller** when: +otherwise exits 2 with `kind: confirmation_required`. It reads the port +table first, then refuses **without ever sending the power-cycle command** +when: - the port is not PoE-capable (an SFP+ port, say) → `kind: conflict`, exit 6 - the port's PoE is administratively off → `kind: conflict`, exit 6 From d045f1c9cfd55078576358f8e41bec093f9dbfd8 Mon Sep 17 00:00:00 2001 From: "Brian R. Jackson" Date: Sun, 26 Jul 2026 18:37:21 -0400 Subject: [PATCH 18/23] feat(ports): show a Connected column in ports find's text output MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit find's whole purpose is telling the operator which port a device is on *now*, and this lookup feeds the destructive `ports cycle`. Previously the text table gave no direct signal for that — only the connected-first sort order distinguished the current port from stale history, which is easy to misread. Append a `Connected` (yes/-) column, colored like the existing Link column, without touching `list`/`devices ports` rendering: render_text keeps its old signature and delegates to a shared render_rows helper with connected=None, so those two callers are byte-identical to before. JSON output (already carrying `connected`) is untouched. --- src/commands/ports.rs | 67 +++++++++++++++++++++++++++++++----- tests/mock_server.rs | 79 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 137 insertions(+), 9 deletions(-) diff --git a/src/commands/ports.rs b/src/commands/ports.rs index 738b2bc..e561f24 100644 --- a/src/commands/ports.rs +++ b/src/commands/ports.rs @@ -143,9 +143,37 @@ pub fn device_col_width(rows: &[&PortRow]) -> usize { /// `device_col_width` of the *full* result set, not just `rows`, so a /// paginated caller renders a stable width across pages. pub fn render_text(rows: &[&PortRow], show_device_col: bool, dev_w: usize, out: &OutputConfig) { + render_rows(rows, show_device_col, dev_w, None, out); +} + +/// Same table as `render_text`, with an extra `Connected` column (`yes`/`-`) +/// appended after RX, aligned by index with `rows`. Only `ports find` calls +/// this: `find`'s entire purpose is telling the operator which port a device +/// is on *now*, and previously only the connected-first sort order +/// distinguished that from stale history. `list` and `devices ports` keep +/// calling `render_text` above, unaffected by this column's existence. +pub fn render_text_with_connected( + rows: &[&PortRow], + dev_w: usize, + connected: &[bool], + out: &OutputConfig, +) { + render_rows(rows, true, dev_w, Some(connected), out); +} + +/// Shared implementation behind `render_text` and `render_text_with_connected`. +/// `connected` is `None` for `render_text`'s two callers, so their output is +/// untouched; `Some` only from `render_text_with_connected`. +fn render_rows( + rows: &[&PortRow], + show_device_col: bool, + dev_w: usize, + connected: Option<&[bool]>, + out: &OutputConfig, +) { let color = use_color(); - let header = if show_device_col { + let mut header = if show_device_col { format!( "{:10} {:>10}", "Device", "Port", "Name", "Link", "Speed", "PoE", "TX", "RX" @@ -156,7 +184,11 @@ pub fn render_text(rows: &[&PortRow], show_device_col: bool, dev_w: usize, out: "Port", "Name", "Link", "Speed", "PoE", "TX", "RX" ) }; - let rule_w = if show_device_col { 70 + dev_w } else { 70 }; + let mut rule_w = if show_device_col { 70 + dev_w } else { 70 }; + if connected.is_some() { + header.push_str(&format!(" {:<9}", "Connected")); + rule_w += 10; + } if color { println!("{}", header.bold()); println!("{}", "-".repeat(rule_w).dimmed()); @@ -165,7 +197,7 @@ pub fn render_text(rows: &[&PortRow], show_device_col: bool, dev_w: usize, out: println!("{}", "-".repeat(rule_w)); } - for r in rows { + for (i, r) in rows.iter().enumerate() { let p = r.port; let port = p .port_idx @@ -187,17 +219,33 @@ pub fn render_text(rows: &[&PortRow], show_device_col: bool, dev_w: usize, out: let tx = p.tx_bytes.map(format_bytes).unwrap_or_else(|| "-".into()); let rx = p.rx_bytes.map(format_bytes).unwrap_or_else(|| "-".into()); - if show_device_col { - println!( + let mut line = if show_device_col { + format!( " {:10} {:>10}", r.device_name, port, name, link_display, speed, poe, tx, rx - ); + ) } else { - println!( + format!( " {:<5} {:<16} {:<6} {:<10} {:<8} {:>10} {:>10}", port, name, link_display, speed, poe, tx, rx - ); + ) + }; + if let Some(flags) = connected { + let is_connected = flags[i]; + let cell = if color { + if is_connected { + format!("{}", "yes".green()) + } else { + format!("{}", "-".dimmed()) + } + } else if is_connected { + "yes".to_string() + } else { + "-".to_string() + }; + line.push_str(&format!(" {cell:<9}")); } + println!("{line}"); } out.print_message(&format!("\n{} ports", rows.len())); } @@ -394,9 +442,10 @@ pub async fn find( out.print_data(&serde_json::to_string_pretty(&items)?); } else { let refs: Vec<&PortRow> = hits.iter().map(|(r, _)| *r).collect(); + let connected: Vec = hits.iter().map(|(_, c)| *c).collect(); // `find` never paginates, so `refs` is already the full result set. let dev_w = device_col_width(&refs); - render_text(&refs, true, dev_w, &out); + render_text_with_connected(&refs, dev_w, &connected, &out); } Ok(()) } diff --git a/tests/mock_server.rs b/tests/mock_server.rs index d2e1cfc..6ee4d53 100644 --- a/tests/mock_server.rs +++ b/tests/mock_server.rs @@ -2540,6 +2540,85 @@ mod command_output { assert!(message.contains("Main-Bedroom"), "got: {message}"); } + // `find`'s JSON output has always carried `connected`; only the text + // table lacked it, leaving the connected-first sort order as the sole + // (easy-to-miss) signal for which row is the device's *current* port — + // a distinction that matters because this lookup feeds the destructive + // `ports cycle`. Two distinctly-named single-port devices (rather than + // one device with two ports) so each rendered row can be identified by + // its device name, independent of the connected-first sort this test + // does not itself re-verify (that is `ports_find_by_mac_sorts_connected_first_and_skips_client_lookup`'s job). + #[tokio::test] + async fn ports_find_text_output_shows_connected_column() { + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/proxy/network/api/s/default/stat/device")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "meta": {"rc": "ok"}, + "data": [ + {"mac": "aa:bb:cc:dd:ee:01", "name": "SwitchConnected", + "port_table": [ + {"port_idx": 7, "last_connection": {"mac": "d8:3a:dd:2b:fa:8a", "connected": true}} + ]}, + {"mac": "aa:bb:cc:dd:ee:02", "name": "SwitchStale", + "port_table": [ + {"port_idx": 2, "last_connection": {"mac": "d8:3a:dd:2b:fa:8a", "connected": false}} + ]} + ] + }))) + .mount(&server) + .await; + + let output = std::process::Command::new(env!("CARGO_BIN_EXE_unifi")) + .args([ + "--host", + &server.uri(), + "--api-key", + "test-key", + "ports", + "find", + "d8:3a:dd:2b:fa:8a", + "-o", + "text", + ]) + .output() + .expect("failed to run the unifi binary"); + assert!( + output.status.success(), + "ports find failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + + let stdout = String::from_utf8_lossy(&output.stdout).into_owned(); + + let header = stdout + .lines() + .find(|l| l.contains("Device")) + .expect("text output must have a header row containing \"Device\""); + assert!( + header.contains("Connected"), + "find's header must carry a Connected column: {header}" + ); + + let connected_row = stdout + .lines() + .find(|l| l.contains("SwitchConnected")) + .expect("expected a row for the connected device"); + let stale_row = stdout + .lines() + .find(|l| l.contains("SwitchStale")) + .expect("expected a row for the stale device"); + + assert!( + connected_row.trim_end().ends_with("yes"), + "the connected row's Connected column must render \"yes\": {connected_row}" + ); + assert!( + stale_row.trim_end().ends_with('-'), + "the stale row's Connected column must render \"-\": {stale_row}" + ); + } + // --- Ports cycle (mutation orchestration) --- // // `power_cycle_port_sends_correct_command` (in `client_api` above) only From 840fabd4c7161d1cee3069dc574081bf3dc9f90b Mon Sep 17 00:00:00 2001 From: "Brian R. Jackson" Date: Sun, 26 Jul 2026 18:37:29 -0400 Subject: [PATCH 19/23] docs(ports): note that cycle's PoE off-interval is switch-defined MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A request came in for an --off-seconds flag on ports cycle. Verified across five independent UniFi client libraries and Ubiquiti's own Integration API spec that the power-cycle command accepts only {cmd, mac, port_idx} on both the legacy and modern endpoints, with no duration parameter — unlike restart on the same endpoint, which does take an optional reboot_type, so the omission is deliberate rather than an oversight. The interval is chosen by switch firmware and varies by model/version; IEEE 802.3 detection timing imposes a roughly 1-2 second floor regardless. Document the limitation instead of building a pass-through that has nothing to pass through to. --- README.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/README.md b/README.md index 98311af..397f89e 100644 --- a/README.md +++ b/README.md @@ -191,6 +191,14 @@ when: - the port's PoE is administratively off → `kind: conflict`, exit 6 - the device has no such port index → `kind: not_found`, exit 4 +The off interval — how long the port stays unpowered — is chosen by the +switch firmware, not by this CLI. The power-cycle command takes only the +target port, with no duration parameter, on either the legacy endpoint or +the Integration API, so the interval isn't configurable and varies by +device model and firmware version. IEEE 802.3 PoE detection timing imposes +a floor regardless: expect the port to sit dark for roughly 1-2 seconds at +minimum before power returns. + List ports for one device, or across every device: ```bash From d881b34c047d693691e63d58c73e0d11960cc217 Mon Sep 17 00:00:00 2001 From: "Brian R. Jackson" Date: Sun, 26 Jul 2026 20:40:56 -0400 Subject: [PATCH 20/23] fix(ports): reject cycling a PoE port not currently delivering power MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A live UCG-Max controller rejected `power-cycle` on a port that passed both existing guard rails (port_poe: true, poe_mode: "auto") with HTTP 400 api.err.InvalidTargetPort, which this CLI surfaced as `api_error` (exit 5) — a kind the schema advertises as retryable, even though retrying this request can never succeed. poe_enable: false was the only attribute distinguishing that port from ones that do cycle; add it as a third check_cyclable guard rail (after the poe_mode: "off" check, which stays more specific and must win) so this now surfaces locally as `conflict` (exit 6) before any HTTP write. Two existing test fixtures (ports.rs's former "happy path" unit test and mock_server.rs's confirmed/declined cycle tests) implicitly relied on poe_enable defaulting to false and are corrected to be realistic. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 2 +- README.md | 1 + src/commands/ports.rs | 78 ++++++++++++++++++++++++++++++++++++++++--- tests/mock_server.rs | 64 +++++++++++++++++++++++++++++++++-- 4 files changed, 137 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0a2d352..756858c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ All notable changes to this project will be documented in this file. ### Added - **ports**: new `unifi ports` command tree — `list`, `show`, `find`, and `cycle` — for working with individual switch ports instead of whole devices. -- **ports cycle**: power-cycles a single PoE port via the `devmgr` `power-cycle` command, with pre-flight checks that reject non-PoE ports and administratively-disabled PoE without ever sending the power-cycle command (the port table is read first). Prompts for confirmation on a TTY; requires `--yes` when piped, otherwise exits 2 with `kind: confirmation_required`. +- **ports cycle**: power-cycles a single PoE port via the `devmgr` `power-cycle` command, with pre-flight checks that reject non-PoE ports, administratively-disabled PoE, and a port not currently delivering PoE (`poe_enable: false`, observed live to make the controller reject the command) without ever sending the power-cycle command (the port table is read first). Prompts for confirmation on a TTY; requires `--yes` when piped, otherwise exits 2 with `kind: confirmation_required`. - **ports find**: resolves a MAC, an IP, or a client name (case-insensitive substring match) to the switch port(s) a device is attached to, via `port_table.last_connection`. A device that has moved between ports appears once per port, with `connected` marking its current one. A name matching more than one client returns `kind: conflict` (exit 6) listing the candidates rather than guessing. - **ports show**: exposes PoE telemetry the CLI previously discarded — `poe_mode`, `poe_class`, `poe_voltage`, `poe_current`, `poe_good` — plus `attached_mac`. - The `conflict` error kind (exit code 6), already advertised in `unifi schema`'s error table since 0.2.2 but never emitted by any code path until now. diff --git a/README.md b/README.md index 397f89e..498befa 100644 --- a/README.md +++ b/README.md @@ -189,6 +189,7 @@ when: - the port is not PoE-capable (an SFP+ port, say) → `kind: conflict`, exit 6 - the port's PoE is administratively off → `kind: conflict`, exit 6 +- the port isn't currently delivering PoE (`poe_enable: false`) → `kind: conflict`, exit 6 - the device has no such port index → `kind: not_found`, exit 4 The off interval — how long the port stays unpowered — is chosen by the diff --git a/src/commands/ports.rs b/src/commands/ports.rs index e561f24..8e772c0 100644 --- a/src/commands/ports.rs +++ b/src/commands/ports.rs @@ -588,6 +588,22 @@ pub fn check_cyclable(port: &PortEntry, device_mac: &str) -> Result<(), ApiError "PoE is administratively disabled on port {idx} of {mac} (poe_mode=off)" ))); } + // Observed against a live UCG-Max controller: a port with port_poe: true, + // poe_mode: "auto", and poe_enable: false rejected `power-cycle` with + // HTTP 400 api.err.InvalidTargetPort. poe_enable was the only attribute + // that differed from ports that do have power to cycle, so it is used + // here as the guard — that inference has not been confirmed by a + // successful cycle against a poe_enable: true port, since doing so would + // require firing at a port with a live device attached. Surfacing this + // locally as `conflict` avoids the alternative: the controller's 400 + // otherwise falls through to `api_error` (see error_for_status), which + // src/schema.rs advertises as retryable even though retrying cannot help. + if !port.poe_enable { + return Err(ApiError::Conflict(format!( + "Port {idx} on {mac} is not currently delivering PoE (poe_enable=false), \ + so there is no power to cycle." + ))); + } Ok(()) } @@ -886,20 +902,72 @@ mod tests { #[test] fn check_cyclable_allows_absent_poe_mode() { // poe_mode is not guaranteed across firmware. A missing value must not - // block a port that already passed the port_poe check. - let d = device_with(serde_json::json!([{"port_idx": 4, "port_poe": true}])); + // block a port that already passed the port_poe check. poe_enable is + // set explicitly here so this test stays about poe_mode alone, not + // about the separate poe_enable guard below. + let d = device_with(serde_json::json!([ + {"port_idx": 4, "port_poe": true, "poe_enable": true} + ])); let p = find_port(&d, 4).unwrap(); assert!(check_cyclable(p, "aa:bb:cc:dd:ee:ff").is_ok()); } #[test] - fn check_cyclable_allows_empty_powered_port() { - // The live happy-path target: PoE-capable, auto, nothing attached. + fn check_cyclable_allows_a_port_actually_delivering_power() { + // The genuinely cyclable case: PoE-capable, auto, and delivering + // power right now. + let d = device_with(serde_json::json!([ + {"port_idx": 4, "port_poe": true, "poe_mode": "auto", "poe_enable": true} + ])); + let p = find_port(&d, 4).unwrap(); + assert!(check_cyclable(p, "aa:bb:cc:dd:ee:ff").is_ok()); + } + + #[test] + fn check_cyclable_rejects_poe_enable_false() { + // This fixture is the live UCG-Max finding that prompted this guard: + // PoE-capable, mode "auto", passing both prior checks, but not + // currently delivering power (poe_enable defaults to false here, + // matching what the controller reported). Firing power-cycle at it + // was rejected with HTTP 400 api.err.InvalidTargetPort instead of + // succeeding, which is why this must be rejected locally too rather + // than treated as the earlier "happy path" this test used to assert. let d = device_with(serde_json::json!([ {"port_idx": 4, "port_poe": true, "poe_mode": "auto", "up": false} ])); let p = find_port(&d, 4).unwrap(); - assert!(check_cyclable(p, "74:ac:b9:ec:b4:5e").is_ok()); + let err = check_cyclable(p, "74:ac:b9:ec:b4:5e").expect_err("poe_enable is false"); + match err { + crate::api::ApiError::Conflict(msg) => { + assert!(msg.contains("not currently delivering PoE"), "got: {msg}") + } + other => panic!("expected Conflict, got {other:?}"), + } + } + + #[test] + fn check_cyclable_poe_mode_off_message_wins_over_poe_enable_false() { + // poe_mode: "off" implies poe_enable: false (confirmed explicitly + // here rather than relying on the default), so both guards would + // fire. The administratively-disabled message must win: it is the + // more specific and more useful of the two, and the poe_mode check + // runs first in `check_cyclable`. + let d = device_with(serde_json::json!([ + {"port_idx": 4, "port_poe": true, "poe_mode": "off", "poe_enable": false} + ])); + let p = find_port(&d, 4).unwrap(); + let err = check_cyclable(p, "aa:bb:cc:dd:ee:ff").expect_err("PoE is off"); + match err { + crate::api::ApiError::Conflict(msg) => { + assert!(msg.contains("poe_mode=off"), "got: {msg}"); + assert!( + !msg.contains("not currently delivering PoE"), + "the administratively-disabled message must win over the \ + poe_enable=false message: {msg}" + ); + } + other => panic!("expected Conflict, got {other:?}"), + } } // `cycle_summary` is the text a human reads before authorising a power diff --git a/tests/mock_server.rs b/tests/mock_server.rs index 6ee4d53..4df66c6 100644 --- a/tests/mock_server.rs +++ b/tests/mock_server.rs @@ -2641,7 +2641,10 @@ mod command_output { "meta": {"rc": "ok"}, "data": [{ "mac": "9c:05:d6:bc:06:43", "name": "USW-24-PoE", - "port_table": [{"port_idx": 5, "port_poe": true, "poe_mode": "auto"}] + "port_table": [{ + "port_idx": 5, "port_poe": true, "poe_mode": "auto", + "poe_enable": true + }] }] }))) .expect(1) @@ -2681,7 +2684,10 @@ mod command_output { "meta": {"rc": "ok"}, "data": [{ "mac": "9c:05:d6:bc:06:43", "name": "USW-24-PoE", - "port_table": [{"port_idx": 5, "port_poe": true, "poe_mode": "auto"}] + "port_table": [{ + "port_idx": 5, "port_poe": true, "poe_mode": "auto", + "poe_enable": true + }] }] }))) .expect(1) @@ -2754,6 +2760,60 @@ mod command_output { ); } + // Mirrors `ports_cycle_non_poe_port_is_conflict_and_never_posts` for the + // third guard rail: a port that is PoE-capable and not administratively + // off, but that the controller reports as not currently delivering power + // (poe_enable: false). This is the fixture from the live UCG-Max finding + // that motivated the guard — see `check_cyclable` in + // `src/commands/ports.rs` for what was actually observed. + #[tokio::test] + async fn ports_cycle_poe_enable_false_is_conflict_and_never_posts() { + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/proxy/network/api/s/default/stat/device")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "meta": {"rc": "ok"}, + "data": [{ + "mac": "74:ac:b9:ec:b4:5e", "name": "USW Lite 8 PoE", + "port_table": [{ + "port_idx": 4, "port_poe": true, "poe_mode": "auto", + "poe_enable": false, "poe_power": 0.0, "up": false + }] + }] + }))) + .expect(1) + .mount(&server) + .await; + Mock::given(method("POST")) + .and(path("/proxy/network/api/s/default/cmd/devmgr")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "meta": {"rc": "ok"}, + "data": [] + }))) + .expect(0) + .mount(&server) + .await; + + let client = mock_client(&server).await; + // `Ok(true)` deliberately, same reasoning as the non-PoE case above: + // proves `check_cyclable` rejects before `confirm` is ever consulted. + let err = + unifi_cli::commands::ports::cycle(&client, "74:ac:b9:ec:b4:5e", 4, out_table(), |_| { + Ok(true) + }) + .await + .unwrap_err(); + let api_err = err + .downcast_ref::() + .unwrap_or_else(|| { + panic!("cycle must reject a poe_enable=false port as an ApiError, got {err}") + }); + assert!( + matches!(api_err, unifi_cli::api::ApiError::Conflict(_)), + "expected Conflict, got {api_err:?}" + ); + } + #[tokio::test] async fn ports_cycle_missing_port_is_not_found_and_never_posts() { let server = MockServer::start().await; From ef3e4e1340609849ff83ece4dd863a2cae077d26 Mon Sep 17 00:00:00 2001 From: "Brian R. Jackson" Date: Sun, 26 Jul 2026 20:54:14 -0400 Subject: [PATCH 21/23] fix(ports): resolve find ambiguity by port occupancy, not client-name count MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found by live testing against a real controller: `unifi ports find garage-pi` returned a conflict because two client records share the name `garage-pi` — the wired and wireless interfaces of one physical Raspberry Pi (MACs one bit apart). Only the wired interface was ever attached to a switch port, so the question `find` exists to answer had exactly one answer, but the old code picked a single client via `resolve_identifier` before ever consulting port tables and rejected the ambiguity outright. Six names on the reporting controller collide this way, making it the common case for any device with more than one interface, not a corner case. `resolve_identifier` is replaced by `resolve_candidates`, which returns every candidate MAC a name/IP could refer to without judging ambiguity. `find` now fetches port tables once and computes matches for every candidate, keeping only ones actually on a port: exactly one -> return its rows, more than one -> conflict naming only the ported candidates (name, MAC, and location), none -> not_found. A MAC-shaped identifier still resolves locally with no client-list round trip. No changes to exit codes, output fields, or JSON envelope shapes. --- CHANGELOG.md | 2 +- README.md | 12 ++- src/commands/ports.rs | 183 ++++++++++++++++++++++++++++-------------- tests/mock_server.rs | 153 ++++++++++++++++++++++++++++++++++- 4 files changed, 280 insertions(+), 70 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 756858c..69cc7d3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,7 +10,7 @@ All notable changes to this project will be documented in this file. - **ports**: new `unifi ports` command tree — `list`, `show`, `find`, and `cycle` — for working with individual switch ports instead of whole devices. - **ports cycle**: power-cycles a single PoE port via the `devmgr` `power-cycle` command, with pre-flight checks that reject non-PoE ports, administratively-disabled PoE, and a port not currently delivering PoE (`poe_enable: false`, observed live to make the controller reject the command) without ever sending the power-cycle command (the port table is read first). Prompts for confirmation on a TTY; requires `--yes` when piped, otherwise exits 2 with `kind: confirmation_required`. -- **ports find**: resolves a MAC, an IP, or a client name (case-insensitive substring match) to the switch port(s) a device is attached to, via `port_table.last_connection`. A device that has moved between ports appears once per port, with `connected` marking its current one. A name matching more than one client returns `kind: conflict` (exit 6) listing the candidates rather than guessing. +- **ports find**: resolves a MAC, an IP, or a client name (case-insensitive substring match) to the switch port(s) a device is attached to, via `port_table.last_connection`. A device that has moved between ports appears once per port, with `connected` marking its current one. A name is ambiguous only when it matches more than one device that is actually on a switch port; that returns `kind: conflict` (exit 6) listing the candidates rather than guessing. Other client records sharing the name that aren't themselves on a port (a device's WiFi interface reporting under the same name as its wired one, say) don't count toward ambiguity. - **ports show**: exposes PoE telemetry the CLI previously discarded — `poe_mode`, `poe_class`, `poe_voltage`, `poe_current`, `poe_good` — plus `attached_mac`. - The `conflict` error kind (exit code 6), already advertised in `unifi schema`'s error table since 0.2.2 but never emitted by any code path until now. diff --git a/README.md b/README.md index 498befa..a2f6ac5 100644 --- a/README.md +++ b/README.md @@ -172,10 +172,14 @@ unifi ports cycle 8c:ed:e1:b0:74:e2 5 `ports find`'s output feeds directly into `show` and `cycle`: `device_mac` and `port_idx` are the *switch's* MAC and port index, not the attached -device's. A name matching more than one client returns `kind: conflict` -(exit 6) listing the candidates rather than guessing. A device that has -moved between switch ports appears once per port it has ever used, with a -`connected` field distinguishing its current port from stale history. +device's. A name is ambiguous only when it matches more than one device +that's actually on a switch port — that returns `kind: conflict` (exit 6) +listing the candidates rather than guessing. Other client records sharing +the name (a device's WiFi interface reporting under the same name as its +wired one, say) don't cause a conflict if they're not themselves on a port. +A device that has moved between switch ports appears once per port it has +ever used, with a `connected` field distinguishing its current port from +stale history. `ports show` exposes PoE telemetry the CLI previously discarded: `poe_mode`, `poe_class`, `poe_voltage`, `poe_current`, `poe_good`, and the diff --git a/src/commands/ports.rs b/src/commands/ports.rs index 8e772c0..edcd772 100644 --- a/src/commands/ports.rs +++ b/src/commands/ports.rs @@ -320,22 +320,31 @@ fn identifier_as_mac(identifier: &str) -> Option { .then_some(normalized) } -/// Resolve a MAC, IP, or client name to a normalized MAC. +/// Resolve a MAC, IP, or client name to every candidate MAC it could refer +/// to. Deliberately does *not* decide ambiguity here: two client records can +/// share a name because they are two interfaces (wired and wireless, say) of +/// one physical device, and only one of them may ever appear in a switch's +/// port table. `find` decides ambiguity from port occupancy instead, after +/// looking up every candidate this returns. /// /// Ordered, stopping at the first tier that matches: normalized MAC equality, -/// then exact IP, then case-insensitive name, then hostname. Follows the +/// then exact IP, then case-insensitive name/hostname substring (all matches +/// in that last tier are returned together). Follows the /// `protect cameras show ` precedent rather than the MAC-only /// convention of `clients show`, because the whole point of `find` is not /// having to look the MAC up first. -pub fn resolve_identifier(identifier: &str, clients: &[LegacyClient]) -> Result { +pub fn resolve_candidates( + identifier: &str, + clients: &[LegacyClient], +) -> Result, ApiError> { if let Some(mac) = identifier_as_mac(identifier) { - return Ok(mac); + return Ok(vec![mac]); } if let Some(c) = clients.iter().find(|c| c.ip.as_deref() == Some(identifier)) && let Some(mac) = c.mac.as_deref() { - return Ok(normalize_mac(mac)); + return Ok(vec![normalize_mac(mac)]); } let wanted = identifier.to_lowercase(); @@ -351,36 +360,44 @@ pub fn resolve_identifier(identifier: &str, clients: &[LegacyClient]) -> Result< }) .collect(); - match by_name.as_slice() { - [] => Err(ApiError::NotFound(format!( + if by_name.is_empty() { + return Err(ApiError::NotFound(format!( "No client matching '{identifier}'" - ))), - [one] => one - .mac - .as_deref() - .map(normalize_mac) - .ok_or_else(|| ApiError::NotFound(format!("Client '{identifier}' has no MAC"))), - many => { - let list = many - .iter() - .map(|c| { - format!( - "{} ({})", - c.name.as_deref().or(c.hostname.as_deref()).unwrap_or("-"), - c.mac - .as_deref() - .map(format_mac) - .unwrap_or_else(|| "-".into()) - ) - }) - .collect::>() - .join(", "); - Err(ApiError::Conflict(format!( - "'{identifier}' matches {} clients: {list}", - many.len() - ))) - } + ))); + } + + let macs: Vec = by_name + .iter() + .filter_map(|c| c.mac.as_deref().map(normalize_mac)) + .collect(); + if macs.is_empty() { + return Err(ApiError::NotFound(format!( + "Client '{identifier}' has no MAC" + ))); } + Ok(macs) +} + +/// Describe one `find` candidate for a conflict message: its name/hostname, +/// formatted MAC, and the switch port it was found on (the connected-first +/// row, i.e. `hits[0]`). Only called once a candidate is already known to +/// have at least one port match. +fn candidate_descriptor(mac: &str, clients: &[LegacyClient], row: &PortRow) -> String { + let label = clients + .iter() + .find(|c| c.mac.as_deref().map(normalize_mac).as_deref() == Some(mac)) + .and_then(|c| c.name.as_deref().or(c.hostname.as_deref())) + .unwrap_or("-"); + let port = row + .port + .port_idx + .map(|i| i.to_string()) + .unwrap_or_else(|| "-".into()); + format!( + "{label} ({}) on {} port {port}", + format_mac(mac), + row.device_name + ) } /// Rows whose `last_connection.mac` matches, connected first so a stale record @@ -403,6 +420,13 @@ pub fn matching_rows<'a>( /// Find which switch port a device is attached to, by MAC, IP, or client /// name. +/// +/// Resolution and port lookup interleave rather than picking a single client +/// up front: a name can match more than one client record while only one of +/// them is ever attached to a switch port (a device's wired and wireless +/// interfaces commonly share a name and report separately). Ambiguity is +/// judged by port occupancy, computed after fetching the port tables, not by +/// how many client records the name matched. pub async fn find( client: &UnifiClient, identifier: &str, @@ -410,24 +434,50 @@ pub async fn find( fields: Option>, ) -> Result<(), Box> { // A MAC identifier resolves locally, so the common scripted path stays a - // single round trip. - let target = if let Some(mac) = identifier_as_mac(identifier) { - mac + // single round trip: no client lookup is needed to know which MAC to + // look for on the port tables. + let (candidates, clients) = if let Some(mac) = identifier_as_mac(identifier) { + (vec![mac], Vec::new()) } else { let clients = client.list_clients_legacy().await?; - resolve_identifier(identifier, &clients)? + let candidates = resolve_candidates(identifier, &clients)?; + (candidates, clients) }; let devices = client.list_all_device_ports().await?; let rows = collect_rows(&devices); - let hits = matching_rows(&rows, &target); - if hits.is_empty() { - return Err(Box::new(ApiError::NotFound(format!( - "No switch port with {} attached", - format_mac(&target) - )))); - } + // Port matches for every candidate, keeping only the ones actually on a + // port. A candidate that matched the name/IP but never appears in any + // port table (e.g. a client's WiFi interface, when only its wired + // interface is on a switch) is not noise worth surfacing here. + let mut ported: Vec<(String, Vec<(&PortRow, bool)>)> = candidates + .into_iter() + .filter_map(|mac| { + let hits = matching_rows(&rows, &mac); + (!hits.is_empty()).then_some((mac, hits)) + }) + .collect(); + + let hits = match ported.len() { + 0 => { + return Err(Box::new(ApiError::NotFound(format!( + "No switch port with '{identifier}' attached" + )))); + } + 1 => ported.pop().expect("checked len == 1 above").1, + _ => { + let list = ported + .iter() + .map(|(mac, hits)| candidate_descriptor(mac, &clients, hits[0].0)) + .collect::>() + .join(", "); + return Err(Box::new(ApiError::Conflict(format!( + "'{identifier}' matches {} devices on switch ports: {list}", + ported.len() + )))); + } + }; if out.is_json() { let items: Vec = hits @@ -1047,40 +1097,49 @@ mod tests { } #[test] - fn resolve_identifier_accepts_any_mac_format() { + fn resolve_candidates_accepts_any_mac_format() { let c = clients_fixture(); // A MAC resolves without consulting the client list at all. assert_eq!( - resolve_identifier("D8-3A-DD-2B-FA-8A", &c).unwrap(), - "d83add2bfa8a" + resolve_candidates("D8-3A-DD-2B-FA-8A", &c).unwrap(), + vec!["d83add2bfa8a"] ); } #[test] - fn resolve_identifier_matches_ip_then_name() { + fn resolve_candidates_matches_ip_then_name() { let c = clients_fixture(); - assert_eq!(resolve_identifier("10.0.0.5", &c).unwrap(), "d83add2bfa8a"); - assert_eq!(resolve_identifier("ALLSKY", &c).unwrap(), "d83add2bfa8a"); + assert_eq!( + resolve_candidates("10.0.0.5", &c).unwrap(), + vec!["d83add2bfa8a"] + ); + assert_eq!( + resolve_candidates("ALLSKY", &c).unwrap(), + vec!["d83add2bfa8a"] + ); } + // `resolve_candidates` no longer decides ambiguity by itself — a name + // matching multiple client records is not an error here, since `find` + // only calls it a conflict once it also knows more than one candidate + // sits on a switch port. This replaces the old + // `resolve_identifier_ambiguous_name_is_conflict`, which asserted the + // opposite (that a name-match count alone was a `Conflict`). #[test] - fn resolve_identifier_ambiguous_name_is_conflict() { + fn resolve_candidates_returns_every_name_match_without_erroring() { let c = clients_fixture(); - let err = resolve_identifier("bedroom", &c).expect_err("ambiguous"); - match err { - crate::api::ApiError::Conflict(msg) => { - assert!(msg.contains("matches 2 clients"), "got: {msg}"); - assert!(msg.contains("bedroom-ap"), "got: {msg}"); - assert!(msg.contains("Main-Bedroom"), "got: {msg}"); - } - other => panic!("expected Conflict, got {other:?}"), - } + let macs = resolve_candidates("bedroom", &c).expect("both are valid candidates"); + assert_eq!( + macs, + vec!["f4e2c665476c".to_string(), "c4f7c161de31".to_string()], + "both bedroom-ap and Main-Bedroom must come back as candidates" + ); } #[test] - fn resolve_identifier_unknown_is_not_found() { + fn resolve_candidates_unknown_is_not_found() { let c = clients_fixture(); - let err = resolve_identifier("nothing-here", &c).expect_err("unknown"); + let err = resolve_candidates("nothing-here", &c).expect_err("unknown"); assert!(matches!(err, crate::api::ApiError::NotFound(_))); } diff --git a/tests/mock_server.rs b/tests/mock_server.rs index 4df66c6..acb83f5 100644 --- a/tests/mock_server.rs +++ b/tests/mock_server.rs @@ -2490,9 +2490,13 @@ mod command_output { ); } - // Ambiguity is never guessed: a name matching more than one client must - // exit 6 (conflict) and name the candidates, rather than silently picking - // one. + // Ambiguity is judged by port occupancy, not by how many client records a + // name matches: `bedroom` genuinely matches two devices here, and both + // are actually attached to a switch port (unlike the "one interface + // never shows up" fixtures below), so this must still exit 6 (conflict) + // and name both candidates. Modeled on a live-controller case — two + // physically distinct bedroom devices on the same switch — reported + // against the pre-fix behavior in the original bug report. #[tokio::test] async fn ports_find_ambiguous_name_exits_with_conflict() { let server = MockServer::start().await; @@ -2507,6 +2511,20 @@ mod command_output { }))) .mount(&server) .await; + Mock::given(method("GET")) + .and(path("/proxy/network/api/s/default/stat/device")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "meta": {"rc": "ok"}, + "data": [{ + "mac": "9c:05:d6:bc:06:43", "name": "USW Pro XG 8 PoE", + "port_table": [ + {"port_idx": 3, "last_connection": {"mac": "f4:e2:c6:65:47:6c", "connected": true}}, + {"port_idx": 4, "last_connection": {"mac": "c4:f7:c1:61:de:31", "connected": true}} + ] + }] + }))) + .mount(&server) + .await; let output = std::process::Command::new(env!("CARGO_BIN_EXE_unifi")) .args([ @@ -2540,6 +2558,135 @@ mod command_output { assert!(message.contains("Main-Bedroom"), "got: {message}"); } + // The live-testing case that prompted this whole restructure: `allsky` + // matches two client records (a Raspberry Pi's wired and wireless + // interfaces, MACs one bit apart in the last octet), but only the wired + // interface ever shows up in a port table. That must resolve cleanly to + // the one candidate that is actually on a port, not conflict. + #[tokio::test] + async fn ports_find_name_matches_two_clients_only_one_on_a_port_resolves_without_conflict() { + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/proxy/network/api/s/default/stat/sta")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "meta": {"rc": "ok"}, + "data": [ + {"_id": "1", "mac": "d8:3a:dd:2b:fa:8a", "name": "allsky", "ip": "10.0.0.5"}, + {"_id": "2", "mac": "d8:3a:dd:2b:fa:8b", "name": "allsky", "ip": "10.0.0.9"} + ] + }))) + .mount(&server) + .await; + Mock::given(method("GET")) + .and(path("/proxy/network/api/s/default/stat/device")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "meta": {"rc": "ok"}, + "data": [{ + "mac": "9c:05:d6:bc:06:43", "name": "USW Pro XG 8 PoE", + "port_table": [ + {"port_idx": 5, "last_connection": {"mac": "d8:3a:dd:2b:fa:8a", "connected": true}} + ] + }] + }))) + .mount(&server) + .await; + + let output = std::process::Command::new(env!("CARGO_BIN_EXE_unifi")) + .args([ + "--host", + &server.uri(), + "--api-key", + "test-key", + "ports", + "find", + "allsky", + "-o", + "json", + ]) + .output() + .expect("failed to run the unifi binary"); + assert!( + output.status.success(), + "ports find must resolve the single ported candidate, not conflict: {}", + String::from_utf8_lossy(&output.stderr) + ); + + let items: serde_json::Value = serde_json::from_slice(&output.stdout).unwrap_or_else(|e| { + panic!( + "stdout was not valid JSON ({e}): {}", + String::from_utf8_lossy(&output.stdout) + ) + }); + let items = items + .as_array() + .expect("ports find must emit a bare JSON array"); + assert_eq!(items.len(), 1, "only the wired interface is on a port"); + assert_eq!(items[0]["port_idx"], 5); + assert_eq!(items[0]["connected"], true); + } + + // The other client record sharing the name never appears in any port + // table at all — not "only the wireless interface", but no candidate on + // a port whatsoever — so this must be not_found, not a conflict. + #[tokio::test] + async fn ports_find_name_matches_clients_none_on_a_port_is_not_found() { + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/proxy/network/api/s/default/stat/sta")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "meta": {"rc": "ok"}, + "data": [ + {"_id": "1", "mac": "d8:3a:dd:2b:fa:8a", "name": "eink", "ip": "10.0.0.15"}, + {"_id": "2", "mac": "d8:3a:dd:2b:fa:8b", "name": "eink", "ip": "10.0.0.16"} + ] + }))) + .mount(&server) + .await; + Mock::given(method("GET")) + .and(path("/proxy/network/api/s/default/stat/device")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "meta": {"rc": "ok"}, + "data": [{ + "mac": "9c:05:d6:bc:06:43", "name": "USW Pro XG 8 PoE", + "port_table": [ + {"port_idx": 1, "last_connection": {"mac": "11:22:33:44:55:66", "connected": true}} + ] + }] + }))) + .mount(&server) + .await; + + let output = std::process::Command::new(env!("CARGO_BIN_EXE_unifi")) + .args([ + "--host", + &server.uri(), + "--api-key", + "test-key", + "ports", + "find", + "eink", + ]) + .output() + .expect("failed to run the unifi binary"); + + assert_eq!( + output.status.code(), + Some(4), + "neither candidate is on any port, so this must exit 4 (not_found), got {:?}\nstderr: {}", + output.status.code(), + String::from_utf8_lossy(&output.stderr) + ); + let stderr = String::from_utf8_lossy(&output.stderr); + let last_line = stderr.trim_end().lines().last().unwrap_or(""); + let envelope: serde_json::Value = + serde_json::from_str(last_line).expect("last stderr line must be valid JSON"); + assert_eq!(envelope["error"]["kind"], "not_found"); + let message = envelope["error"]["message"] + .as_str() + .expect("error envelope must carry a message"); + assert!(message.contains("eink"), "got: {message}"); + } + // `find`'s JSON output has always carried `connected`; only the text // table lacked it, leaving the connected-first sort order as the sole // (easy-to-miss) signal for which row is the device's *current* port — From dfd68cbc809b0dc232ee708ec51eda5d315f77e8 Mon Sep 17 00:00:00 2001 From: "Brian R. Jackson" Date: Sun, 26 Jul 2026 21:07:53 -0400 Subject: [PATCH 22/23] chore(ports): replace real host names and MACs in test fixtures with generic placeholders Regression fixtures added on this branch were written against real hardware during live testing and captured real host names and MAC addresses. Swap them for generic placeholders (aa:bb:cc:dd:ee:NN MACs, neutral device names) before this branch is published, preserving every fixture's exact shape: the wired/wireless one-bit-apart MAC pairs and the name-substring ambiguity relationships still hold. --- README.md | 10 ++++----- src/api/tests.rs | 4 ++-- src/commands/ports.rs | 42 +++++++++++++++++----------------- tests/mock_server.rs | 52 +++++++++++++++++++++---------------------- 4 files changed, 54 insertions(+), 54 deletions(-) diff --git a/README.md b/README.md index a2f6ac5..177a5a3 100644 --- a/README.md +++ b/README.md @@ -160,14 +160,14 @@ port instead of rebooting the whole switch: ```bash # Which port is my Pi on? Matches by name (case-insensitive substring), # MAC, or IP. -unifi ports find allsky -unifi ports find d8:3a:dd:2b:fa:8a +unifi ports find garage-pi +unifi ports find aa:bb:cc:dd:ee:10 # Inspect it — PoE mode, class, voltage, current, and what's attached -unifi ports show 8c:ed:e1:b0:74:e2 5 +unifi ports show aa:bb:cc:dd:ee:ff 5 # Bounce PoE on that port only — the rest of the switch is untouched -unifi ports cycle 8c:ed:e1:b0:74:e2 5 +unifi ports cycle aa:bb:cc:dd:ee:ff 5 ``` `ports find`'s output feeds directly into `show` and `cycle`: `device_mac` @@ -207,7 +207,7 @@ minimum before power returns. List ports for one device, or across every device: ```bash -unifi ports list 8c:ed:e1:b0:74:e2 +unifi ports list aa:bb:cc:dd:ee:ff unifi ports list --limit 20 --fields port_idx,poe_power ``` diff --git a/src/api/tests.rs b/src/api/tests.rs index 4e45190..2d8f36a 100644 --- a/src/api/tests.rs +++ b/src/api/tests.rs @@ -849,7 +849,7 @@ fn port_entry_parses_poe_telemetry_with_string_numbers() { "tx_errors": 0, "rx_errors": 0, "last_connection": { - "mac": "f4:e2:c6:65:47:6c", + "mac": "aa:bb:cc:dd:ee:20", "connected": true, "last_seen": 1783622695 } @@ -865,7 +865,7 @@ fn port_entry_parses_poe_telemetry_with_string_numbers() { assert_eq!(p.enable, Some(true)); assert_eq!(p.is_uplink, Some(false)); let lc = p.last_connection.expect("last_connection present"); - assert_eq!(lc.mac.as_deref(), Some("f4:e2:c6:65:47:6c")); + assert_eq!(lc.mac.as_deref(), Some("aa:bb:cc:dd:ee:20")); assert_eq!(lc.connected, Some(true)); } diff --git a/src/commands/ports.rs b/src/commands/ports.rs index edcd772..eaeb44b 100644 --- a/src/commands/ports.rs +++ b/src/commands/ports.rs @@ -986,7 +986,7 @@ mod tests { {"port_idx": 4, "port_poe": true, "poe_mode": "auto", "up": false} ])); let p = find_port(&d, 4).unwrap(); - let err = check_cyclable(p, "74:ac:b9:ec:b4:5e").expect_err("poe_enable is false"); + let err = check_cyclable(p, "aa:bb:cc:dd:ee:fe").expect_err("poe_enable is false"); match err { crate::api::ApiError::Conflict(msg) => { assert!(msg.contains("not currently delivering PoE"), "got: {msg}") @@ -1029,12 +1029,12 @@ mod tests { fn cycle_summary_shows_the_attached_mac_when_connected() { let d = device_with(serde_json::json!([{ "port_idx": 4, "port_poe": true, - "last_connection": {"mac": "d8:3a:dd:2b:fa:8a", "connected": true} + "last_connection": {"mac": "aa:bb:cc:dd:ee:10", "connected": true} }])); let p = find_port(&d, 4).unwrap(); let summary = cycle_summary(&d, p); assert!( - summary.contains("d8:3a:dd:2b:fa:8a"), + summary.contains("aa:bb:cc:dd:ee:10"), "a connected last_connection must show the formatted attached MAC: {summary}" ); } @@ -1045,7 +1045,7 @@ mod tests { // summary must not read as if a live device would lose power. let d = device_with(serde_json::json!([{ "port_idx": 4, "port_poe": true, - "last_connection": {"mac": "d8:3a:dd:2b:fa:8a", "connected": false} + "last_connection": {"mac": "aa:bb:cc:dd:ee:10", "connected": false} }])); let p = find_port(&d, 4).unwrap(); let summary = cycle_summary(&d, p); @@ -1054,7 +1054,7 @@ mod tests { "a stale (disconnected) last_connection must read as unattached: {summary}" ); assert!( - !summary.contains("d8:3a:dd:2b:fa:8a"), + !summary.contains("aa:bb:cc:dd:ee:10"), "a stale MAC must not appear as if it were live: {summary}" ); } @@ -1089,9 +1089,9 @@ mod tests { // to make the fixture actually deserialize. fn clients_fixture() -> Vec { serde_json::from_value(serde_json::json!([ - {"_id": "1", "mac": "d8:3a:dd:2b:fa:8a", "name": "allsky", "ip": "10.0.0.5"}, - {"_id": "2", "mac": "f4:e2:c6:65:47:6c", "name": "bedroom-ap", "ip": "10.0.0.6"}, - {"_id": "3", "mac": "c4:f7:c1:61:de:31", "name": "Main-Bedroom", "ip": "10.0.0.7"} + {"_id": "1", "mac": "aa:bb:cc:dd:ee:10", "name": "garage-pi", "ip": "10.0.0.5"}, + {"_id": "2", "mac": "aa:bb:cc:dd:ee:20", "name": "office-ap", "ip": "10.0.0.6"}, + {"_id": "3", "mac": "aa:bb:cc:dd:ee:21", "name": "Main-Office", "ip": "10.0.0.7"} ])) .expect("fixture must parse") } @@ -1101,8 +1101,8 @@ mod tests { let c = clients_fixture(); // A MAC resolves without consulting the client list at all. assert_eq!( - resolve_candidates("D8-3A-DD-2B-FA-8A", &c).unwrap(), - vec!["d83add2bfa8a"] + resolve_candidates("AA-BB-CC-DD-EE-10", &c).unwrap(), + vec!["aabbccddee10"] ); } @@ -1111,11 +1111,11 @@ mod tests { let c = clients_fixture(); assert_eq!( resolve_candidates("10.0.0.5", &c).unwrap(), - vec!["d83add2bfa8a"] + vec!["aabbccddee10"] ); assert_eq!( - resolve_candidates("ALLSKY", &c).unwrap(), - vec!["d83add2bfa8a"] + resolve_candidates("GARAGE-PI", &c).unwrap(), + vec!["aabbccddee10"] ); } @@ -1128,11 +1128,11 @@ mod tests { #[test] fn resolve_candidates_returns_every_name_match_without_erroring() { let c = clients_fixture(); - let macs = resolve_candidates("bedroom", &c).expect("both are valid candidates"); + let macs = resolve_candidates("office", &c).expect("both are valid candidates"); assert_eq!( macs, - vec!["f4e2c665476c".to_string(), "c4f7c161de31".to_string()], - "both bedroom-ap and Main-Bedroom must come back as candidates" + vec!["aabbccddee20".to_string(), "aabbccddee21".to_string()], + "both office-ap and Main-Office must come back as candidates" ); } @@ -1149,14 +1149,14 @@ mod tests { "mac": "aa:bb:cc:dd:ee:ff", "name": "SwitchA", "port_table": [ - {"port_idx": 2, "last_connection": {"mac": "d8:3a:dd:2b:fa:8a", "connected": false}}, - {"port_idx": 7, "last_connection": {"mac": "d8:3a:dd:2b:fa:8a", "connected": true}}, + {"port_idx": 2, "last_connection": {"mac": "aa:bb:cc:dd:ee:10", "connected": false}}, + {"port_idx": 7, "last_connection": {"mac": "aa:bb:cc:dd:ee:10", "connected": true}}, {"port_idx": 9, "last_connection": {"mac": "11:22:33:44:55:66", "connected": true}} ] }])) .expect("fixture must parse"); let rows = collect_rows(&devices); - let hits = matching_rows(&rows, "d83add2bfa8a"); + let hits = matching_rows(&rows, "aabbccddee10"); assert_eq!(hits.len(), 2, "device appears on two ports"); assert_eq!( hits[0].0.port.port_idx, @@ -1176,11 +1176,11 @@ mod tests { "mac": "aa:bb:cc:dd:ee:ff", "name": "SwitchA", "port_table": [{ "port_idx": 7, - "last_connection": {"mac": "d8:3a:dd:2b:fa:8a", "connected": true} + "last_connection": {"mac": "aa:bb:cc:dd:ee:10", "connected": true} }] }))]; let rows = collect_rows(&devices); - let hits = matching_rows(&rows, "d83add2bfa8a"); + let hits = matching_rows(&rows, "aabbccddee10"); let (row, connected) = hits[0]; let mut value = row_json(row); value["connected"] = connected.into(); diff --git a/tests/mock_server.rs b/tests/mock_server.rs index acb83f5..79b1d18 100644 --- a/tests/mock_server.rs +++ b/tests/mock_server.rs @@ -2420,8 +2420,8 @@ mod command_output { "data": [{ "mac": "9c:05:d6:bc:06:43", "name": "USW-24-PoE", "port_table": [ - {"port_idx": 2, "last_connection": {"mac": "d8:3a:dd:2b:fa:8a", "connected": false}}, - {"port_idx": 7, "last_connection": {"mac": "d8:3a:dd:2b:fa:8a", "connected": true}}, + {"port_idx": 2, "last_connection": {"mac": "aa:bb:cc:dd:ee:10", "connected": false}}, + {"port_idx": 7, "last_connection": {"mac": "aa:bb:cc:dd:ee:10", "connected": true}}, {"port_idx": 9, "last_connection": {"mac": "11:22:33:44:55:66", "connected": true}} ] }] @@ -2445,7 +2445,7 @@ mod command_output { "test-key", "ports", "find", - "d8:3a:dd:2b:fa:8a", + "aa:bb:cc:dd:ee:10", "-o", "json", ]) @@ -2491,11 +2491,11 @@ mod command_output { } // Ambiguity is judged by port occupancy, not by how many client records a - // name matches: `bedroom` genuinely matches two devices here, and both + // name matches: `office` genuinely matches two devices here, and both // are actually attached to a switch port (unlike the "one interface // never shows up" fixtures below), so this must still exit 6 (conflict) // and name both candidates. Modeled on a live-controller case — two - // physically distinct bedroom devices on the same switch — reported + // physically distinct office devices on the same switch — reported // against the pre-fix behavior in the original bug report. #[tokio::test] async fn ports_find_ambiguous_name_exits_with_conflict() { @@ -2505,8 +2505,8 @@ mod command_output { .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ "meta": {"rc": "ok"}, "data": [ - {"_id": "1", "mac": "f4:e2:c6:65:47:6c", "name": "bedroom-ap", "ip": "10.0.0.6"}, - {"_id": "2", "mac": "c4:f7:c1:61:de:31", "name": "Main-Bedroom", "ip": "10.0.0.7"} + {"_id": "1", "mac": "aa:bb:cc:dd:ee:20", "name": "office-ap", "ip": "10.0.0.6"}, + {"_id": "2", "mac": "aa:bb:cc:dd:ee:21", "name": "Main-Office", "ip": "10.0.0.7"} ] }))) .mount(&server) @@ -2518,8 +2518,8 @@ mod command_output { "data": [{ "mac": "9c:05:d6:bc:06:43", "name": "USW Pro XG 8 PoE", "port_table": [ - {"port_idx": 3, "last_connection": {"mac": "f4:e2:c6:65:47:6c", "connected": true}}, - {"port_idx": 4, "last_connection": {"mac": "c4:f7:c1:61:de:31", "connected": true}} + {"port_idx": 3, "last_connection": {"mac": "aa:bb:cc:dd:ee:20", "connected": true}}, + {"port_idx": 4, "last_connection": {"mac": "aa:bb:cc:dd:ee:21", "connected": true}} ] }] }))) @@ -2534,7 +2534,7 @@ mod command_output { "test-key", "ports", "find", - "bedroom", + "office", ]) .output() .expect("failed to run the unifi binary"); @@ -2554,11 +2554,11 @@ mod command_output { let message = envelope["error"]["message"] .as_str() .expect("error envelope must carry a message"); - assert!(message.contains("bedroom-ap"), "got: {message}"); - assert!(message.contains("Main-Bedroom"), "got: {message}"); + assert!(message.contains("office-ap"), "got: {message}"); + assert!(message.contains("Main-Office"), "got: {message}"); } - // The live-testing case that prompted this whole restructure: `allsky` + // A device whose wired and wireless interfaces share a name: `garage-pi` // matches two client records (a Raspberry Pi's wired and wireless // interfaces, MACs one bit apart in the last octet), but only the wired // interface ever shows up in a port table. That must resolve cleanly to @@ -2571,8 +2571,8 @@ mod command_output { .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ "meta": {"rc": "ok"}, "data": [ - {"_id": "1", "mac": "d8:3a:dd:2b:fa:8a", "name": "allsky", "ip": "10.0.0.5"}, - {"_id": "2", "mac": "d8:3a:dd:2b:fa:8b", "name": "allsky", "ip": "10.0.0.9"} + {"_id": "1", "mac": "aa:bb:cc:dd:ee:10", "name": "garage-pi", "ip": "10.0.0.5"}, + {"_id": "2", "mac": "aa:bb:cc:dd:ee:11", "name": "garage-pi", "ip": "10.0.0.9"} ] }))) .mount(&server) @@ -2584,7 +2584,7 @@ mod command_output { "data": [{ "mac": "9c:05:d6:bc:06:43", "name": "USW Pro XG 8 PoE", "port_table": [ - {"port_idx": 5, "last_connection": {"mac": "d8:3a:dd:2b:fa:8a", "connected": true}} + {"port_idx": 5, "last_connection": {"mac": "aa:bb:cc:dd:ee:10", "connected": true}} ] }] }))) @@ -2599,7 +2599,7 @@ mod command_output { "test-key", "ports", "find", - "allsky", + "garage-pi", "-o", "json", ]) @@ -2636,8 +2636,8 @@ mod command_output { .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ "meta": {"rc": "ok"}, "data": [ - {"_id": "1", "mac": "d8:3a:dd:2b:fa:8a", "name": "eink", "ip": "10.0.0.15"}, - {"_id": "2", "mac": "d8:3a:dd:2b:fa:8b", "name": "eink", "ip": "10.0.0.16"} + {"_id": "1", "mac": "aa:bb:cc:dd:ee:10", "name": "lobby-display", "ip": "10.0.0.15"}, + {"_id": "2", "mac": "aa:bb:cc:dd:ee:11", "name": "lobby-display", "ip": "10.0.0.16"} ] }))) .mount(&server) @@ -2664,7 +2664,7 @@ mod command_output { "test-key", "ports", "find", - "eink", + "lobby-display", ]) .output() .expect("failed to run the unifi binary"); @@ -2684,7 +2684,7 @@ mod command_output { let message = envelope["error"]["message"] .as_str() .expect("error envelope must carry a message"); - assert!(message.contains("eink"), "got: {message}"); + assert!(message.contains("lobby-display"), "got: {message}"); } // `find`'s JSON output has always carried `connected`; only the text @@ -2705,11 +2705,11 @@ mod command_output { "data": [ {"mac": "aa:bb:cc:dd:ee:01", "name": "SwitchConnected", "port_table": [ - {"port_idx": 7, "last_connection": {"mac": "d8:3a:dd:2b:fa:8a", "connected": true}} + {"port_idx": 7, "last_connection": {"mac": "aa:bb:cc:dd:ee:10", "connected": true}} ]}, {"mac": "aa:bb:cc:dd:ee:02", "name": "SwitchStale", "port_table": [ - {"port_idx": 2, "last_connection": {"mac": "d8:3a:dd:2b:fa:8a", "connected": false}} + {"port_idx": 2, "last_connection": {"mac": "aa:bb:cc:dd:ee:10", "connected": false}} ]} ] }))) @@ -2724,7 +2724,7 @@ mod command_output { "test-key", "ports", "find", - "d8:3a:dd:2b:fa:8a", + "aa:bb:cc:dd:ee:10", "-o", "text", ]) @@ -2921,7 +2921,7 @@ mod command_output { .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ "meta": {"rc": "ok"}, "data": [{ - "mac": "74:ac:b9:ec:b4:5e", "name": "USW Lite 8 PoE", + "mac": "aa:bb:cc:dd:ee:fe", "name": "USW Lite 8 PoE", "port_table": [{ "port_idx": 4, "port_poe": true, "poe_mode": "auto", "poe_enable": false, "poe_power": 0.0, "up": false @@ -2945,7 +2945,7 @@ mod command_output { // `Ok(true)` deliberately, same reasoning as the non-PoE case above: // proves `check_cyclable` rejects before `confirm` is ever consulted. let err = - unifi_cli::commands::ports::cycle(&client, "74:ac:b9:ec:b4:5e", 4, out_table(), |_| { + unifi_cli::commands::ports::cycle(&client, "aa:bb:cc:dd:ee:fe", 4, out_table(), |_| { Ok(true) }) .await From 14a96b1939feefeb233926b0a2ddfcaa6c8d8b0c Mon Sep 17 00:00:00 2001 From: "Brian R. Jackson" Date: Sun, 26 Jul 2026 21:33:12 -0400 Subject: [PATCH 23/23] fix(ports): pad coloured table cells by visible width, not escape byte length Coloured Link/Connected cells were built as owo_colors-wrapped strings and handed to `{: --- src/commands/ports.rs | 114 ++++++++++++++++++++++++++++++++++++++---- tests/mock_server.rs | 80 +++++++++++++++++++++++++++++ 2 files changed, 183 insertions(+), 11 deletions(-) diff --git a/src/commands/ports.rs b/src/commands/ports.rs index eaeb44b..c0b9ff8 100644 --- a/src/commands/ports.rs +++ b/src/commands/ports.rs @@ -124,6 +124,29 @@ fn speed_cell(p: &PortEntry) -> String { } } +/// Right-pad a table cell to `width` visible columns, deriving the padding +/// from `plain`'s length rather than `rendered`'s. +/// +/// This exists because `format!("{: String { + let pad = width.saturating_sub(plain.len()); + format!("{rendered}{}", " ".repeat(pad)) +} + +/// "port" for exactly one row, "ports" otherwise — the row-count trailer's +/// singular/plural noun. +fn port_noun(count: usize) -> &'static str { + if count == 1 { "port" } else { "ports" } +} + /// Device column width, in characters. Callers that paginate must compute /// this from the full result set, not just the page handed to `render_text` /// — otherwise two `--offset` pages of the same query can render the column @@ -172,6 +195,10 @@ fn render_rows( out: &OutputConfig, ) { let color = use_color(); + // Must match the `{:10} {:>10}", - r.device_name, port, name, link_display, speed, poe, tx, rx + " {:10} {:>10}", + r.device_name, port, name, speed, poe, tx, rx ) } else { format!( - " {:<5} {:<16} {:<6} {:<10} {:<8} {:>10} {:>10}", - port, name, link_display, speed, poe, tx, rx + " {:<5} {:<16} {link_cell} {:<10} {:<8} {:>10} {:>10}", + port, name, speed, poe, tx, rx ) }; if let Some(flags) = connected { let is_connected = flags[i]; - let cell = if color { + let plain = if is_connected { "yes" } else { "-" }; + let rendered = if color { if is_connected { format!("{}", "yes".green()) } else { format!("{}", "-".dimmed()) } - } else if is_connected { - "yes".to_string() } else { - "-".to_string() + plain.to_string() }; - line.push_str(&format!(" {cell:<9}")); + let cell = pad_visible(&rendered, plain, CONNECTED_W); + line.push_str(&format!(" {cell}")); } println!("{line}"); } - out.print_message(&format!("\n{} ports", rows.len())); + out.print_message(&format!("\n{} {}", rows.len(), port_noun(rows.len()))); } pub async fn list( @@ -894,6 +922,70 @@ mod tests { ); } + // `pad_visible` is what makes coloured Link/Connected cells line up with + // the header at a TTY. `use_color()` reads `stdout().is_terminal()` + // directly with no override, so a unit test cannot force colour on for + // `render_rows` itself without adding a flag/env override the task this + // was written for explicitly ruled out ("no new flags"). Testing the + // padding helper directly, with a hand-built ANSI-escaped string standing + // in for what `owo_colors` would emit, exercises the actual defect + // (padding computed from byte length instead of visible width) without + // needing a real TTY. + #[test] + fn pad_visible_pads_by_plain_width_not_escaped_byte_length() { + // The real thing `render_rows` hands `pad_visible` on the coloured + // path: `"up".green()` rendered to a `String`, several bytes of ANSI + // escapes wrapped around 2 visible characters. A `{:<6}` specifier + // sees this as already over width 6 (that was the bug) and pads with + // nothing at all. + let escaped = format!("{}", "up".green()); + assert!( + escaped.len() > "up".len(), + "fixture must actually carry escape bytes, or this test proves nothing: {escaped:?}" + ); + + let padded = pad_visible(&escaped, "up", 6); + + assert!( + padded.starts_with(&escaped), + "the coloured text itself must be emitted untouched: {padded:?}" + ); + let visible_padding = &padded[escaped.len()..]; + assert_eq!( + visible_padding, " ", + "padding must be derived from \"up\".len() (2), not the escaped \ + string's byte length: {padded:?}" + ); + } + + #[test] + fn pad_visible_matches_the_uncoloured_output_it_replaces() { + // On the uncoloured path every call site passes `rendered == plain`, + // so this must reproduce exactly what the old `{: DeviceWithPorts { serde_json::from_value(serde_json::json!({ "mac": "aa:bb:cc:dd:ee:ff", diff --git a/tests/mock_server.rs b/tests/mock_server.rs index 79b1d18..95ca7ea 100644 --- a/tests/mock_server.rs +++ b/tests/mock_server.rs @@ -2143,6 +2143,86 @@ mod command_output { ); } + // The row-count trailer must read "1 port" for a single row and "N ports" + // otherwise — it used to say "1 ports" unconditionally. Spawns the real + // binary (rather than calling `render_text` in-process) so this observes + // literal stderr text, the same surface an operator actually reads. + #[tokio::test] + async fn ports_list_trailer_is_singular_for_exactly_one_row() { + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/proxy/network/api/s/default/stat/device")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "meta": {"rc": "ok"}, + "data": [{"mac": "aa:bb:cc:dd:ee:01", "name": "SwitchA", + "port_table": [{"port_idx": 1}]}] + }))) + .mount(&server) + .await; + + let output = std::process::Command::new(env!("CARGO_BIN_EXE_unifi")) + .args([ + "--host", + &server.uri(), + "--api-key", + "test-key", + "ports", + "list", + "--output", + "text", + ]) + .output() + .expect("failed to run the unifi binary"); + assert!( + output.status.success(), + "ports list failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.trim_end().ends_with("1 port"), + "a single row must be reported as \"1 port\", not \"1 ports\": {stderr:?}" + ); + } + + #[tokio::test] + async fn ports_list_trailer_is_plural_for_multiple_rows() { + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/proxy/network/api/s/default/stat/device")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "meta": {"rc": "ok"}, + "data": [{"mac": "aa:bb:cc:dd:ee:01", "name": "SwitchA", + "port_table": [{"port_idx": 1}, {"port_idx": 2}]}] + }))) + .mount(&server) + .await; + + let output = std::process::Command::new(env!("CARGO_BIN_EXE_unifi")) + .args([ + "--host", + &server.uri(), + "--api-key", + "test-key", + "ports", + "list", + "--output", + "text", + ]) + .output() + .expect("failed to run the unifi binary"); + assert!( + output.status.success(), + "ports list failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.trim_end().ends_with("2 ports"), + "two rows must be reported as \"2 ports\": {stderr:?}" + ); + } + // --- Ports list (top-level) --- // // Drives the real `unifi` binary against a wiremock server so the JSON