From c1c306d19bba18600978993349d2d04c90b455b8 Mon Sep 17 00:00:00 2001 From: 514sid Date: Sat, 6 Jun 2026 13:28:51 +0400 Subject: [PATCH 01/11] Feat: surface API error messages from response body Add ApiError variant to CommandError and parse the 'error' field from JSON response bodies in commands::post, so callers see the API's own message instead of a raw status code or debug-formatted variant name. --- src/cli.rs | 2 +- src/commands/mod.rs | 12 ++++++++++-- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/src/cli.rs b/src/cli.rs index d3ba88e5..e9488393 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -509,7 +509,7 @@ pub fn handle_command_execution_result( ) } _ => { - error!("Error occurred: {e:?}"); + error!("Error occurred: {e}"); } } std::process::exit(1); diff --git a/src/commands/mod.rs b/src/commands/mod.rs index db4f6057..dd392a66 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -80,8 +80,10 @@ pub enum CommandError { Parse(#[from] serde_json::Error), #[error("parse error: {0}")] YamlParse(#[from] serde_yaml::Error), - #[error("unknown error: {0}")] + #[error("unexpected response status: {0}")] WrongResponseStatus(u16), + #[error("{0}")] + ApiError(String), #[error("Required field is missing in the response")] MissingField, #[error("Required file is missing in the edge app directory: {0}")] @@ -178,7 +180,13 @@ pub fn post( // Ok, No_Content are acceptable because some of our RPC code returns that. if ![StatusCode::CREATED, StatusCode::OK, StatusCode::NO_CONTENT].contains(&status) { - debug!("Response: {:?}", &response.text()?); + let body = response.text().unwrap_or_default(); + debug!("Response: {:?}", &body); + if let Ok(json) = serde_json::from_str::(&body) { + if let Some(msg) = json.get("error").and_then(|v| v.as_str()) { + return Err(CommandError::ApiError(msg.to_string())); + } + } return Err(CommandError::WrongResponseStatus(status.as_u16())); } if status == StatusCode::NO_CONTENT { From 1ef6872b82f5fb5f37a9067b16f791e5d8d20277 Mon Sep 17 00:00:00 2001 From: 514sid Date: Sat, 6 Jun 2026 13:28:57 +0400 Subject: [PATCH 02/11] Feat: migrate screen commands to v4.1 API All screen operations now use v4.1 endpoints: - list/get use v4.1/screens (was v4/) - add uses commands::post with v4.1/screens (was manual v3 POST with array wrapping; v4.1 returns arrays natively) - delete uses PostgREST filter syntax v4.1/screens?id=eq. (was v3/screens//) Adds a test for the invalid pin error path. --- src/cli.rs | 2 +- src/commands/screen.rs | 64 +++++++++++++++++++++++------------------ src/mcp/tests.rs | 4 +-- src/mcp/tools/screen.rs | 4 +-- 4 files changed, 41 insertions(+), 33 deletions(-) diff --git a/src/cli.rs b/src/cli.rs index e9488393..c6b50c75 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -1273,7 +1273,7 @@ mod tests { let mock_server = MockServer::start(); mock_server.mock(|when, then| { when.method(GET) - .path("/v4/screens") + .path("/v4.1/screens") .query_param("id", "eq.017a5104-524b-33d8-8026-9087b59e7eb5") .header("user-agent", format!("screenly-cli {}", env!("CARGO_PKG_VERSION"))) .header("Authorization", "Token token"); diff --git a/src/commands/screen.rs b/src/commands/screen.rs index 15f32c2c..b9c37406 100644 --- a/src/commands/screen.rs +++ b/src/commands/screen.rs @@ -1,7 +1,5 @@ use std::collections::HashMap; -use reqwest::StatusCode; - use crate::authentication::Authentication; use crate::commands; use crate::commands::{CommandError, Screens}; @@ -18,12 +16,12 @@ impl ScreenCommand { pub fn list(&self) -> anyhow::Result { Ok(Screens::new(commands::get( &self.authentication, - "v4/screens", + "v4.1/screens", )?)) } pub fn get(&self, id: &str) -> anyhow::Result { - let endpoint = format!("v4/screens?id=eq.{id}"); + let endpoint = format!("v4.1/screens?id=eq.{id}"); Ok(Screens::new(commands::get( &self.authentication, @@ -36,32 +34,20 @@ impl ScreenCommand { pin: &str, maybe_name: Option, ) -> anyhow::Result { - let url = format!("{}/v3/screens/", &self.authentication.config.url); let mut payload = HashMap::new(); payload.insert("pin".to_string(), pin.to_string()); if let Some(name) = maybe_name { payload.insert("name".to_string(), name); } - let response = self - .authentication - .build_client()? - .post(url) - .json(&payload) - .send()?; - if response.status() != StatusCode::CREATED { - return Err(CommandError::WrongResponseStatus( - response.status().as_u16(), - )); - } - - // Our newer endpoints all return arrays so let's just convert the output from v3 to be the same - let mut array: Vec = Vec::new(); - array.insert(0, serde_json::from_str(&response.text()?)?); - Ok(Screens::new(serde_json::Value::Array(array))) + Ok(Screens::new(commands::post( + &self.authentication, + "v4.1/screens", + &payload, + )?)) } pub fn delete(&self, id: &str) -> anyhow::Result<(), CommandError> { - let endpoint = format!("v3/screens/{id}/"); + let endpoint = format!("v4.1/screens?id=eq.{id}"); commands::delete(&self.authentication, &endpoint) } } @@ -86,7 +72,7 @@ mod tests { let mock_server = MockServer::start(); mock_server.mock(|when, then| { when.method(GET) - .path("/v4/screens") + .path("/v4.1/screens") .header("Authorization", "Token token") .header( "user-agent", @@ -105,10 +91,11 @@ mod tests { #[test] fn test_add_screen_should_send_correct_request() { let new_screen = serde_json::from_str::("{\"id\":\"017a5104-524b-33d8-8026-9087b59e7eb5\",\"team_id\":\"016343c2-82b8-0000-a121-e30f1035875e\",\"created_at\":\"2021-06-28T05:07:55+00:00\",\"name\":\"Test\",\"is_enabled\":true,\"coords\":[55.22931, 48.90429],\"last_ping\":\"2021-08-25T06:17:20.728+00:00\",\"last_ip\":null,\"local_ip\":\"192.168.1.146\",\"mac\":\"b8:27:eb:d6:83:6f\",\"last_screenshot_time\":\"2021-08-25T06:09:04.399+00:00\",\"uptime\":\"230728.38\",\"load_avg\":\"0.14\",\"signal_strength\":null,\"interface\":\"eth0\",\"debug\":false,\"location\":\"Kamsko-Ust'inskiy rayon, Russia\",\"team\":\"016343c2-82b8-0000-a121-e30f1035875e\",\"timezone\":\"Europe/Moscow\",\"type\":\"hardware\",\"hostname\":\"srly-4shnfrdc5cd2p0p\",\"ws_open\":false,\"status\":\"Offline\",\"last_screenshot\":\"https://us-assets.screenlyapp.com/01CD1W50NR000A28F31W83B1TY/screenshots/01F98G8MJB6FC809MGGYTSWZNN/5267668e6db35498e61b83d4c702dbe8\",\"in_sync\":false,\"software_version\":\"Screenly 2 Player\",\"hardware_version\":\"Raspberry Pi 3B\",\"config\":{\"hdmi_mode\": 34, \"hdmi_boost\": 2, \"hdmi_drive\": 0, \"hdmi_group\": 0, \"verify_ssl\": true, \"audio_output\": \"hdmi\", \"hdmi_timings\": \"\", \"overscan_top\": 0, \"overscan_left\": 0, \"use_composite\": false, \"display_rotate\": 0, \"overscan_right\": 0, \"overscan_scale\": 0, \"overscan_bottom\": 0, \"disable_overscan\": 0, \"shuffle_playlist\": false, \"framebuffer_width\": 0, \"use_composite_pal\": false, \"framebuffer_height\": 0, \"hdmi_force_hotplug\": true, \"use_composite_ntsc\": false, \"hdmi_pixel_encoding\": 0, \"play_history_enabled\": false}}").unwrap(); + let new_screen_array = serde_json::Value::Array(vec![new_screen.clone()]); let mock_server = MockServer::start(); let post_mock = mock_server.mock(|when, then| { when.method(POST) - .path("/v3/screens/") + .path("/v4.1/screens") .header("Authorization", "Token token") .header("content-type", "application/json") .header( @@ -116,7 +103,7 @@ mod tests { format!("screenly-cli {}", env!("CARGO_PKG_VERSION")), ) .json_body(json!({"pin": "test-pin", "name": "test"})); - then.status(201).json_body(new_screen.clone()); + then.status(201).json_body(new_screen_array.clone()); }); let config = Config::new(mock_server.base_url()); @@ -125,7 +112,27 @@ mod tests { let v = screen_command.add("test-pin", Some("test".to_string())); post_mock.assert(); assert!(v.is_ok()); - assert_eq!(v.unwrap().value.as_array().unwrap()[0], new_screen); + assert_eq!(v.unwrap().value, new_screen_array); + } + + #[test] + fn test_add_screen_wrong_pin_should_fail_with_api_error_message() { + let mock_server = MockServer::start(); + mock_server.mock(|when, then| { + when.method(POST).path("/v4.1/screens"); + then.status(400) + .json_body(json!({"code": "P0001", "error": "Invalid pin"})); + }); + + let config = Config::new(mock_server.base_url()); + let authentication = Authentication::new_with_config(config, "token"); + let screen_command = ScreenCommand::new(authentication); + let result = screen_command.add("wrong-pin", None); + assert!(result.is_err()); + assert_eq!( + result.unwrap_err().to_string(), + "Invalid pin" + ); } #[test] @@ -133,7 +140,7 @@ mod tests { let mock_server = MockServer::start(); mock_server.mock(|when, then| { when.method(GET) - .path("/v4/screens") + .path("/v4.1/screens") .query_param("id", "eq.017a5104-524b-33d8-8026-9087b59e7eb5") .header("user-agent", format!("screenly-cli {}", env!("CARGO_PKG_VERSION"))) .header("Authorization", "Token token"); @@ -157,7 +164,8 @@ mod tests { let mock_server = MockServer::start(); mock_server.mock(|when, then| { when.method(DELETE) - .path("/v3/screens/test-id/") + .path("/v4.1/screens") + .query_param("id", "eq.test-id") .header( "user-agent", format!("screenly-cli {}", env!("CARGO_PKG_VERSION")), diff --git a/src/mcp/tests.rs b/src/mcp/tests.rs index 86a4a0b3..b8c5b78c 100644 --- a/src/mcp/tests.rs +++ b/src/mcp/tests.rs @@ -26,7 +26,7 @@ fn test_screen_list() { let mock_server = MockServer::start(); mock_server.mock(|when, then| { when.method(GET) - .path("/v4/screens") + .path("/v4.1/screens") .header("Authorization", "Token test_token"); then.status(200) .json_body(json!([{"id": "screen-1", "name": "Test Screen"}])); @@ -45,7 +45,7 @@ fn test_screen_get() { let mock_server = MockServer::start(); mock_server.mock(|when, then| { when.method(GET) - .path("/v4/screens") + .path("/v4.1/screens") .query_param("id", "eq.screen-uuid") .header("Authorization", "Token test_token"); then.status(200) diff --git a/src/mcp/tools/screen.rs b/src/mcp/tools/screen.rs index 534b5acc..26bfe791 100644 --- a/src/mcp/tools/screen.rs +++ b/src/mcp/tools/screen.rs @@ -9,7 +9,7 @@ pub struct ScreenTools; impl ScreenTools { /// List all screens. pub fn list(auth: &Authentication) -> Result { - let result = commands::get(auth, "v4/screens") + let result = commands::get(auth, "v4.1/screens") .map_err(|e| format!("Failed to list screens: {}", e))?; serde_json::to_string_pretty(&result) @@ -18,7 +18,7 @@ impl ScreenTools { /// Get a screen by UUID. pub fn get(auth: &Authentication, uuid: &str) -> Result { - let endpoint = format!("v4/screens?id=eq.{}", uuid); + let endpoint = format!("v4.1/screens?id=eq.{}", uuid); let result = commands::get(auth, &endpoint).map_err(|e| format!("Failed to get screen: {}", e))?; From f5a22da9ca96fe26819d932d58d74ac27ec87906 Mon Sep 17 00:00:00 2001 From: 514sid Date: Sat, 6 Jun 2026 16:27:46 +0400 Subject: [PATCH 03/11] Fix: apply rustfmt formatting --- src/commands/screen.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/commands/screen.rs b/src/commands/screen.rs index b9c37406..0d2816ed 100644 --- a/src/commands/screen.rs +++ b/src/commands/screen.rs @@ -129,10 +129,7 @@ mod tests { let screen_command = ScreenCommand::new(authentication); let result = screen_command.add("wrong-pin", None); assert!(result.is_err()); - assert_eq!( - result.unwrap_err().to_string(), - "Invalid pin" - ); + assert_eq!(result.unwrap_err().to_string(), "Invalid pin"); } #[test] From a910187eff1370733796075bf6ec8c018673d077 Mon Sep 17 00:00:00 2001 From: 514sid Date: Mon, 8 Jun 2026 20:13:29 +0400 Subject: [PATCH 04/11] Add select query params for v4.1 screen nested data --- src/cli.rs | 1 + src/commands/screen.rs | 6 ++++-- src/mcp/tests.rs | 2 ++ src/mcp/tools/screen.rs | 4 ++-- 4 files changed, 9 insertions(+), 4 deletions(-) diff --git a/src/cli.rs b/src/cli.rs index 9dcb32a3..b039116b 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -1249,6 +1249,7 @@ mod tests { when.method(GET) .path("/v4.1/screens") .query_param("id", "eq.017a5104-524b-33d8-8026-9087b59e7eb5") + .query_param("select", "*,screens_configs(*),screens_pings(*),screens_reports(*),screens_statuses(*)") .header("user-agent", format!("screenly-cli {}", env!("CARGO_PKG_VERSION"))) .header("Authorization", "Token token"); then diff --git a/src/commands/screen.rs b/src/commands/screen.rs index 0d2816ed..b09a4b26 100644 --- a/src/commands/screen.rs +++ b/src/commands/screen.rs @@ -16,12 +16,12 @@ impl ScreenCommand { pub fn list(&self) -> anyhow::Result { Ok(Screens::new(commands::get( &self.authentication, - "v4.1/screens", + "v4.1/screens?select=*,screens_configs(*),screens_pings(*),screens_reports(*),screens_statuses(*)", )?)) } pub fn get(&self, id: &str) -> anyhow::Result { - let endpoint = format!("v4.1/screens?id=eq.{id}"); + let endpoint = format!("v4.1/screens?id=eq.{id}&select=*,screens_configs(*),screens_pings(*),screens_reports(*),screens_statuses(*)"); Ok(Screens::new(commands::get( &self.authentication, @@ -73,6 +73,7 @@ mod tests { mock_server.mock(|when, then| { when.method(GET) .path("/v4.1/screens") + .query_param("select", "*,screens_configs(*),screens_pings(*),screens_reports(*),screens_statuses(*)") .header("Authorization", "Token token") .header( "user-agent", @@ -139,6 +140,7 @@ mod tests { when.method(GET) .path("/v4.1/screens") .query_param("id", "eq.017a5104-524b-33d8-8026-9087b59e7eb5") + .query_param("select", "*,screens_configs(*),screens_pings(*),screens_reports(*),screens_statuses(*)") .header("user-agent", format!("screenly-cli {}", env!("CARGO_PKG_VERSION"))) .header("Authorization", "Token token"); then diff --git a/src/mcp/tests.rs b/src/mcp/tests.rs index b8c5b78c..3b8464ef 100644 --- a/src/mcp/tests.rs +++ b/src/mcp/tests.rs @@ -27,6 +27,7 @@ fn test_screen_list() { mock_server.mock(|when, then| { when.method(GET) .path("/v4.1/screens") + .query_param("select", "*,screens_configs(*),screens_pings(*),screens_reports(*),screens_statuses(*)") .header("Authorization", "Token test_token"); then.status(200) .json_body(json!([{"id": "screen-1", "name": "Test Screen"}])); @@ -47,6 +48,7 @@ fn test_screen_get() { when.method(GET) .path("/v4.1/screens") .query_param("id", "eq.screen-uuid") + .query_param("select", "*,screens_configs(*),screens_pings(*),screens_reports(*),screens_statuses(*)") .header("Authorization", "Token test_token"); then.status(200) .json_body(json!([{"id": "screen-uuid", "name": "My Screen"}])); diff --git a/src/mcp/tools/screen.rs b/src/mcp/tools/screen.rs index 26bfe791..19f4fc29 100644 --- a/src/mcp/tools/screen.rs +++ b/src/mcp/tools/screen.rs @@ -9,7 +9,7 @@ pub struct ScreenTools; impl ScreenTools { /// List all screens. pub fn list(auth: &Authentication) -> Result { - let result = commands::get(auth, "v4.1/screens") + let result = commands::get(auth, "v4.1/screens?select=*,screens_configs(*),screens_pings(*),screens_reports(*),screens_statuses(*)") .map_err(|e| format!("Failed to list screens: {}", e))?; serde_json::to_string_pretty(&result) @@ -18,7 +18,7 @@ impl ScreenTools { /// Get a screen by UUID. pub fn get(auth: &Authentication, uuid: &str) -> Result { - let endpoint = format!("v4.1/screens?id=eq.{}", uuid); + let endpoint = format!("v4.1/screens?id=eq.{}&select=*,screens_configs(*),screens_pings(*),screens_reports(*),screens_statuses(*)", uuid); let result = commands::get(auth, &endpoint).map_err(|e| format!("Failed to get screen: {}", e))?; From f6472d1087f61a900a7cda1cf19b34168d1e6545 Mon Sep 17 00:00:00 2001 From: 514sid Date: Thu, 11 Jun 2026 17:14:42 +0400 Subject: [PATCH 05/11] Extract api_error_from_body helper and apply it to get, delete, and patch Non-2xx responses from get/delete/patch previously returned WrongResponseStatus without inspecting the body. The helper tries error, message, and detail keys before falling back to WrongResponseStatus, matching the behavior post already had. --- src/commands/mod.rs | 33 +++++++++++++++++++++------------ 1 file changed, 21 insertions(+), 12 deletions(-) diff --git a/src/commands/mod.rs b/src/commands/mod.rs index 2e96dd1e..ad3297ee 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -172,6 +172,17 @@ pub enum CommandError { AppNotFound(String), } +fn api_error_from_body(body: &str, status: StatusCode) -> CommandError { + if let Ok(json) = serde_json::from_str::(body) { + for key in &["error", "message", "detail"] { + if let Some(msg) = json.get(key).and_then(|v| v.as_str()) { + return CommandError::ApiError(msg.to_string()); + } + } + } + CommandError::WrongResponseStatus(status.as_u16()) +} + pub fn get( authentication: &Authentication, endpoint: &str, @@ -191,8 +202,9 @@ pub fn get( debug!("GET {url} -> {status}"); if status != StatusCode::OK { - println!("Response: {:?}", &response.text()); - return Err(CommandError::WrongResponseStatus(status.as_u16())); + let body = response.text().unwrap_or_default(); + debug!("Response: {:?}", &body); + return Err(api_error_from_body(&body, status)); } Ok(serde_json::from_str(&response.text()?)?) } @@ -220,12 +232,7 @@ pub fn post( if ![StatusCode::CREATED, StatusCode::OK, StatusCode::NO_CONTENT].contains(&status) { let body = response.text().unwrap_or_default(); debug!("Response: {:?}", &body); - if let Ok(json) = serde_json::from_str::(&body) { - if let Some(msg) = json.get("error").and_then(|v| v.as_str()) { - return Err(CommandError::ApiError(msg.to_string())); - } - } - return Err(CommandError::WrongResponseStatus(status.as_u16())); + return Err(api_error_from_body(&body, status)); } if status == StatusCode::NO_CONTENT { return Ok(serde_json::Value::Null); @@ -241,8 +248,9 @@ pub fn delete(authentication: &Authentication, endpoint: &str) -> anyhow::Result let status = response.status(); if ![StatusCode::OK, StatusCode::NO_CONTENT].contains(&status) { - debug!("Response: {:?}", &response.text()?); - return Err(CommandError::WrongResponseStatus(status.as_u16())); + let body = response.text().unwrap_or_default(); + debug!("Response: {:?}", &body); + return Err(api_error_from_body(&body, status)); } Ok(()) } @@ -265,8 +273,9 @@ pub fn patch( let status = response.status(); if status != StatusCode::OK { - debug!("Response: {:?}", &response.text()?); - return Err(CommandError::WrongResponseStatus(status.as_u16())); + let body = response.text().unwrap_or_default(); + debug!("Response: {:?}", &body); + return Err(api_error_from_body(&body, status)); } if status == StatusCode::NO_CONTENT { From 8f47815ea4484831b6ad00717e92958a0bd59132 Mon Sep 17 00:00:00 2001 From: 514sid Date: Thu, 11 Jun 2026 17:16:14 +0400 Subject: [PATCH 06/11] Apply rustfmt --- src/commands/screen.rs | 5 ++++- src/mcp/tests.rs | 10 ++++++++-- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/src/commands/screen.rs b/src/commands/screen.rs index b09a4b26..cdf33ec9 100644 --- a/src/commands/screen.rs +++ b/src/commands/screen.rs @@ -73,7 +73,10 @@ mod tests { mock_server.mock(|when, then| { when.method(GET) .path("/v4.1/screens") - .query_param("select", "*,screens_configs(*),screens_pings(*),screens_reports(*),screens_statuses(*)") + .query_param( + "select", + "*,screens_configs(*),screens_pings(*),screens_reports(*),screens_statuses(*)", + ) .header("Authorization", "Token token") .header( "user-agent", diff --git a/src/mcp/tests.rs b/src/mcp/tests.rs index 3b8464ef..96690f39 100644 --- a/src/mcp/tests.rs +++ b/src/mcp/tests.rs @@ -27,7 +27,10 @@ fn test_screen_list() { mock_server.mock(|when, then| { when.method(GET) .path("/v4.1/screens") - .query_param("select", "*,screens_configs(*),screens_pings(*),screens_reports(*),screens_statuses(*)") + .query_param( + "select", + "*,screens_configs(*),screens_pings(*),screens_reports(*),screens_statuses(*)", + ) .header("Authorization", "Token test_token"); then.status(200) .json_body(json!([{"id": "screen-1", "name": "Test Screen"}])); @@ -48,7 +51,10 @@ fn test_screen_get() { when.method(GET) .path("/v4.1/screens") .query_param("id", "eq.screen-uuid") - .query_param("select", "*,screens_configs(*),screens_pings(*),screens_reports(*),screens_statuses(*)") + .query_param( + "select", + "*,screens_configs(*),screens_pings(*),screens_reports(*),screens_statuses(*)", + ) .header("Authorization", "Token test_token"); then.status(200) .json_body(json!([{"id": "screen-uuid", "name": "My Screen"}])); From f316b10b0573b80ad5026199efc6e40131cdae0d Mon Sep 17 00:00:00 2001 From: 514sid Date: Thu, 11 Jun 2026 17:19:35 +0400 Subject: [PATCH 07/11] Use Display instead of Debug for all user-facing error messages Switches all error!/eprintln! call sites from {e:?} to {e} so users see the clean error message rather than the internal Rust variant name. Also adds delete_mock.assert() to the delete screen test to verify the request was actually made. --- src/cli.rs | 22 +++++++++++----------- src/commands/screen.rs | 6 ++++-- 2 files changed, 15 insertions(+), 13 deletions(-) diff --git a/src/cli.rs b/src/cli.rs index b039116b..2aa7b13c 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -565,7 +565,7 @@ pub fn handle_cli(cli: &Cli) { std::process::exit(1); } _ => { - error!("Error occurred: {e:?}"); + error!("Error occurred: {e}"); std::process::exit(1); } }, @@ -657,7 +657,7 @@ pub fn handle_cli_screen_command(command: &ScreenCommands, output: OutputFormat) std::process::exit(0); } Err(e) => { - error!("Error occurred: {e:?}"); + error!("Error occurred: {e}"); std::process::exit(1); } } @@ -685,7 +685,7 @@ pub fn handle_cli_playlist_command(command: &PlaylistCommands, output: OutputFor println!("Playlist deleted successfully."); } Err(e) => { - eprintln!("Error occurred when deleting playlist: {e:?}") + eprintln!("Error occurred when deleting playlist: {e}") } }, PlaylistCommands::Append { @@ -729,7 +729,7 @@ pub fn handle_cli_playlist_command(command: &PlaylistCommands, output: OutputFor println!("Playlist updated successfully."); } Err(e) => { - eprintln!("Error occurred when updating playlist: {e:?}") + eprintln!("Error occurred when updating playlist: {e}") } } } @@ -783,7 +783,7 @@ pub fn handle_cli_asset_command(command: &AssetCommands, output: OutputFormat) { std::process::exit(0); } Err(e) => { - error!("Error occurred: {e:?}"); + error!("Error occurred: {e}"); std::process::exit(1); } } @@ -818,7 +818,7 @@ pub fn handle_cli_asset_command(command: &AssetCommands, output: OutputFormat) { info!("Asset updated successfully."); } Err(e) => { - error!("Error occurred: {e:?}"); + error!("Error occurred: {e}"); std::process::exit(1); } } @@ -829,7 +829,7 @@ pub fn handle_cli_asset_command(command: &AssetCommands, output: OutputFormat) { info!("Asset updated successfully."); } Err(e) => { - error!("Error occurred: {e:?}"); + error!("Error occurred: {e}"); std::process::exit(1); } } @@ -844,7 +844,7 @@ pub fn handle_cli_asset_command(command: &AssetCommands, output: OutputFormat) { info!("Asset updated successfully."); } Err(e) => { - error!("Error occurred: {e:?}"); + error!("Error occurred: {e}"); std::process::exit(1); } } @@ -855,7 +855,7 @@ pub fn handle_cli_asset_command(command: &AssetCommands, output: OutputFormat) { info!("Asset updated successfully."); } Err(e) => { - error!("Error occurred: {e:?}"); + error!("Error occurred: {e}"); std::process::exit(1); } } @@ -869,7 +869,7 @@ pub fn handle_cli_asset_command(command: &AssetCommands, output: OutputFormat) { info!("Asset updated successfully."); } Err(e) => { - error!("Error occurred: {e:?}"); + error!("Error occurred: {e}"); std::process::exit(1); } } @@ -999,7 +999,7 @@ pub fn handle_cli_edge_app_command(command: &EdgeAppCommands, output: OutputForm std::process::exit(0); } Err(e) => { - error!("Error occurred: {e:?}"); + error!("Error occurred: {e}"); std::process::exit(1); } } diff --git a/src/commands/screen.rs b/src/commands/screen.rs index cdf33ec9..02e81fa8 100644 --- a/src/commands/screen.rs +++ b/src/commands/screen.rs @@ -164,7 +164,7 @@ mod tests { #[test] fn test_delete_screen_should_send_correct_request() { let mock_server = MockServer::start(); - mock_server.mock(|when, then| { + let delete_mock = mock_server.mock(|when, then| { when.method(DELETE) .path("/v4.1/screens") .query_param("id", "eq.test-id") @@ -179,7 +179,9 @@ mod tests { let config = Config::new(mock_server.base_url()); let authentication = Authentication::new_with_config(config, "token"); let screen_command = ScreenCommand::new(authentication); - assert!(screen_command.delete("test-id").is_ok()); + let result = screen_command.delete("test-id"); + delete_mock.assert(); + assert!(result.is_ok()); } #[test] From 3e59dadf7d4bdf0a35d394a0a42100b9df641c54 Mon Sep 17 00:00:00 2001 From: 514sid Date: Thu, 11 Jun 2026 18:16:01 +0400 Subject: [PATCH 08/11] Add tests for api_error_from_body key variants and HTTP error paths Tests all three error body keys (error, message, detail) and fallback cases directly, plus GET and DELETE error paths via screen command tests. --- src/commands/mod.rs | 33 +++++++++++++++++++++++++++++++++ src/commands/screen.rs | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+) diff --git a/src/commands/mod.rs b/src/commands/mod.rs index ad3297ee..d4e9f702 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -676,8 +676,41 @@ impl Formatter for PlaylistItems { #[cfg(test)] mod tests { + use reqwest::StatusCode; + use super::*; + #[test] + fn test_api_error_from_body_uses_error_key() { + let err = api_error_from_body(r#"{"error": "Invalid pin"}"#, StatusCode::BAD_REQUEST); + assert_eq!(err.to_string(), "Invalid pin"); + } + + #[test] + fn test_api_error_from_body_uses_message_key() { + let err = api_error_from_body(r#"{"message": "Not found"}"#, StatusCode::NOT_FOUND); + assert_eq!(err.to_string(), "Not found"); + } + + #[test] + fn test_api_error_from_body_uses_detail_key() { + let err = api_error_from_body(r#"{"detail": "Permission denied"}"#, StatusCode::FORBIDDEN); + assert_eq!(err.to_string(), "Permission denied"); + } + + #[test] + fn test_api_error_from_body_falls_back_to_status_on_unknown_key() { + let err = + api_error_from_body(r#"{"code": "ERR123"}"#, StatusCode::INTERNAL_SERVER_ERROR); + assert_eq!(err.to_string(), "unexpected response status: 500"); + } + + #[test] + fn test_api_error_from_body_falls_back_to_status_on_non_json() { + let err = api_error_from_body("not json", StatusCode::BAD_GATEWAY); + assert_eq!(err.to_string(), "unexpected response status: 502"); + } + #[test] fn test_assets_csv_round_trip() { let data = r#"[ diff --git a/src/commands/screen.rs b/src/commands/screen.rs index 02e81fa8..54ef1913 100644 --- a/src/commands/screen.rs +++ b/src/commands/screen.rs @@ -136,6 +136,40 @@ mod tests { assert_eq!(result.unwrap_err().to_string(), "Invalid pin"); } + #[test] + fn test_get_screen_error_should_return_api_error_message() { + let mock_server = MockServer::start(); + mock_server.mock(|when, then| { + when.method(GET).path("/v4.1/screens"); + then.status(404) + .json_body(json!({"message": "Screen not found"})); + }); + + let config = Config::new(mock_server.base_url()); + let authentication = Authentication::new_with_config(config, "token"); + let screen_command = ScreenCommand::new(authentication); + let result = screen_command.get("nonexistent-id"); + assert!(result.is_err()); + assert_eq!(result.unwrap_err().to_string(), "Screen not found"); + } + + #[test] + fn test_delete_screen_error_should_return_api_error_message() { + let mock_server = MockServer::start(); + mock_server.mock(|when, then| { + when.method(DELETE).path("/v4.1/screens"); + then.status(403) + .json_body(json!({"detail": "Permission denied"})); + }); + + let config = Config::new(mock_server.base_url()); + let authentication = Authentication::new_with_config(config, "token"); + let screen_command = ScreenCommand::new(authentication); + let result = screen_command.delete("test-id"); + assert!(result.is_err()); + assert_eq!(result.unwrap_err().to_string(), "Permission denied"); + } + #[test] fn test_get_screen_should_return_screen() { let mock_server = MockServer::start(); From e747a37c52d8c8f5038c99b0f7950d282c1deb22 Mon Sep 17 00:00:00 2001 From: 514sid Date: Thu, 23 Jul 2026 11:47:28 +0400 Subject: [PATCH 09/11] Address follow-up review: empty-body safety and shared select const - post() now treats an empty 201/200 body as null instead of failing serde parsing, so 'screen add' can't error after the screen is actually created (e.g. if the server stops honoring 'Prefer: return=representation'). Added a test with an empty 201. - Extract the duplicated screens select string into a shared SCREEN_SELECT const used by both the CLI and MCP call sites so they can't drift. --- src/commands/mod.rs | 12 +++++++++--- src/commands/screen.rs | 27 +++++++++++++++++++++++++-- src/mcp/tools/screen.rs | 8 +++++--- 3 files changed, 39 insertions(+), 8 deletions(-) diff --git a/src/commands/mod.rs b/src/commands/mod.rs index d4e9f702..cb5de6a5 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -238,7 +238,14 @@ pub fn post( return Ok(serde_json::Value::Null); } - Ok(serde_json::from_str(&response.text()?)?) + // A 201/200 with an empty body (e.g. if the server ever stops honoring + // `Prefer: return=representation`) must not fail the call after the + // resource has already been created. + let body = response.text()?; + if body.trim().is_empty() { + return Ok(serde_json::Value::Null); + } + Ok(serde_json::from_str(&body)?) } pub fn delete(authentication: &Authentication, endpoint: &str) -> anyhow::Result<(), CommandError> { @@ -700,8 +707,7 @@ mod tests { #[test] fn test_api_error_from_body_falls_back_to_status_on_unknown_key() { - let err = - api_error_from_body(r#"{"code": "ERR123"}"#, StatusCode::INTERNAL_SERVER_ERROR); + let err = api_error_from_body(r#"{"code": "ERR123"}"#, StatusCode::INTERNAL_SERVER_ERROR); assert_eq!(err.to_string(), "unexpected response status: 500"); } diff --git a/src/commands/screen.rs b/src/commands/screen.rs index 54ef1913..7c9cdec8 100644 --- a/src/commands/screen.rs +++ b/src/commands/screen.rs @@ -4,6 +4,11 @@ use crate::authentication::Authentication; use crate::commands; use crate::commands::{CommandError, Screens}; +/// Nested resources selected alongside each screen record on the v4.1 API. +/// Shared by the CLI and MCP call sites so the select string can't drift. +pub const SCREEN_SELECT: &str = + "*,screens_configs(*),screens_pings(*),screens_reports(*),screens_statuses(*)"; + pub struct ScreenCommand { authentication: Authentication, } @@ -14,14 +19,15 @@ impl ScreenCommand { } pub fn list(&self) -> anyhow::Result { + let endpoint = format!("v4.1/screens?select={SCREEN_SELECT}"); Ok(Screens::new(commands::get( &self.authentication, - "v4.1/screens?select=*,screens_configs(*),screens_pings(*),screens_reports(*),screens_statuses(*)", + &endpoint, )?)) } pub fn get(&self, id: &str) -> anyhow::Result { - let endpoint = format!("v4.1/screens?id=eq.{id}&select=*,screens_configs(*),screens_pings(*),screens_reports(*),screens_statuses(*)"); + let endpoint = format!("v4.1/screens?id=eq.{id}&select={SCREEN_SELECT}"); Ok(Screens::new(commands::get( &self.authentication, @@ -119,6 +125,23 @@ mod tests { assert_eq!(v.unwrap().value, new_screen_array); } + #[test] + fn test_add_screen_handles_empty_created_body() { + // A 201 with no body must not fail the call after the screen is created. + let mock_server = MockServer::start(); + let post_mock = mock_server.mock(|when, then| { + when.method(POST).path("/v4.1/screens"); + then.status(201); + }); + + let config = Config::new(mock_server.base_url()); + let authentication = Authentication::new_with_config(config, "token"); + let screen_command = ScreenCommand::new(authentication); + let result = screen_command.add("test-pin", None); + post_mock.assert(); + assert!(result.is_ok()); + } + #[test] fn test_add_screen_wrong_pin_should_fail_with_api_error_message() { let mock_server = MockServer::start(); diff --git a/src/mcp/tools/screen.rs b/src/mcp/tools/screen.rs index 19f4fc29..fb5748ab 100644 --- a/src/mcp/tools/screen.rs +++ b/src/mcp/tools/screen.rs @@ -2,6 +2,7 @@ use crate::authentication::Authentication; use crate::commands; +use crate::commands::screen::SCREEN_SELECT; /// Screen tools for the MCP server. pub struct ScreenTools; @@ -9,8 +10,9 @@ pub struct ScreenTools; impl ScreenTools { /// List all screens. pub fn list(auth: &Authentication) -> Result { - let result = commands::get(auth, "v4.1/screens?select=*,screens_configs(*),screens_pings(*),screens_reports(*),screens_statuses(*)") - .map_err(|e| format!("Failed to list screens: {}", e))?; + let endpoint = format!("v4.1/screens?select={SCREEN_SELECT}"); + let result = + commands::get(auth, &endpoint).map_err(|e| format!("Failed to list screens: {}", e))?; serde_json::to_string_pretty(&result) .map_err(|e| format!("Failed to serialize response: {}", e)) @@ -18,7 +20,7 @@ impl ScreenTools { /// Get a screen by UUID. pub fn get(auth: &Authentication, uuid: &str) -> Result { - let endpoint = format!("v4.1/screens?id=eq.{}&select=*,screens_configs(*),screens_pings(*),screens_reports(*),screens_statuses(*)", uuid); + let endpoint = format!("v4.1/screens?id=eq.{uuid}&select={SCREEN_SELECT}"); let result = commands::get(auth, &endpoint).map_err(|e| format!("Failed to get screen: {}", e))?; From 66853bcdeb291324b42d70901d7325e47167dfcc Mon Sep 17 00:00:00 2001 From: 514sid Date: Thu, 23 Jul 2026 12:04:03 +0400 Subject: [PATCH 10/11] Fix screen select relationships: screens_ -> screen_ prefix The v4.1 nested-resource select used a pluralized 'screens_' prefix (screens_configs, screens_pings, screens_reports, screens_statuses), which the live v4.1 API rejects with 400 Bad Request, breaking 'screen list' and 'screen get'. The actual relationship names are singular ('screen_configs', etc.). Verified against the live endpoint: the corrected select returns 200 with the nested objects embedded. Updated the shared const and all mock query-param assertions. --- src/cli.rs | 2 +- src/commands/screen.rs | 6 +++--- src/mcp/tests.rs | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/cli.rs b/src/cli.rs index 2aa7b13c..7d858c43 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -1249,7 +1249,7 @@ mod tests { when.method(GET) .path("/v4.1/screens") .query_param("id", "eq.017a5104-524b-33d8-8026-9087b59e7eb5") - .query_param("select", "*,screens_configs(*),screens_pings(*),screens_reports(*),screens_statuses(*)") + .query_param("select", "*,screen_configs(*),screen_pings(*),screen_reports(*),screen_statuses(*)") .header("user-agent", format!("screenly-cli {}", env!("CARGO_PKG_VERSION"))) .header("Authorization", "Token token"); then diff --git a/src/commands/screen.rs b/src/commands/screen.rs index 7c9cdec8..98a30f44 100644 --- a/src/commands/screen.rs +++ b/src/commands/screen.rs @@ -7,7 +7,7 @@ use crate::commands::{CommandError, Screens}; /// Nested resources selected alongside each screen record on the v4.1 API. /// Shared by the CLI and MCP call sites so the select string can't drift. pub const SCREEN_SELECT: &str = - "*,screens_configs(*),screens_pings(*),screens_reports(*),screens_statuses(*)"; + "*,screen_configs(*),screen_pings(*),screen_reports(*),screen_statuses(*)"; pub struct ScreenCommand { authentication: Authentication, @@ -81,7 +81,7 @@ mod tests { .path("/v4.1/screens") .query_param( "select", - "*,screens_configs(*),screens_pings(*),screens_reports(*),screens_statuses(*)", + "*,screen_configs(*),screen_pings(*),screen_reports(*),screen_statuses(*)", ) .header("Authorization", "Token token") .header( @@ -200,7 +200,7 @@ mod tests { when.method(GET) .path("/v4.1/screens") .query_param("id", "eq.017a5104-524b-33d8-8026-9087b59e7eb5") - .query_param("select", "*,screens_configs(*),screens_pings(*),screens_reports(*),screens_statuses(*)") + .query_param("select", "*,screen_configs(*),screen_pings(*),screen_reports(*),screen_statuses(*)") .header("user-agent", format!("screenly-cli {}", env!("CARGO_PKG_VERSION"))) .header("Authorization", "Token token"); then diff --git a/src/mcp/tests.rs b/src/mcp/tests.rs index 96690f39..b6e6c281 100644 --- a/src/mcp/tests.rs +++ b/src/mcp/tests.rs @@ -29,7 +29,7 @@ fn test_screen_list() { .path("/v4.1/screens") .query_param( "select", - "*,screens_configs(*),screens_pings(*),screens_reports(*),screens_statuses(*)", + "*,screen_configs(*),screen_pings(*),screen_reports(*),screen_statuses(*)", ) .header("Authorization", "Token test_token"); then.status(200) @@ -53,7 +53,7 @@ fn test_screen_get() { .query_param("id", "eq.screen-uuid") .query_param( "select", - "*,screens_configs(*),screens_pings(*),screens_reports(*),screens_statuses(*)", + "*,screen_configs(*),screen_pings(*),screen_reports(*),screen_statuses(*)", ) .header("Authorization", "Token test_token"); then.status(200) From fe7276de9e104a006ed9ccdf40302ade49e400d7 Mon Sep 17 00:00:00 2001 From: 514sid Date: Thu, 23 Jul 2026 12:30:10 +0400 Subject: [PATCH 11/11] Make add-screen test mock the real {id} response shape The v4.1 POST /screens endpoint returns a bare object with the new screen's id ({"id": ""}), confirmed both live and in the backend (one_off/sql/api_4_1/screens/functions.sql). The test previously mocked a full-screen array, which passed but encoded an unrealistic contract. --- src/commands/screen.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/commands/screen.rs b/src/commands/screen.rs index 98a30f44..b7c241c7 100644 --- a/src/commands/screen.rs +++ b/src/commands/screen.rs @@ -100,8 +100,9 @@ mod tests { #[test] fn test_add_screen_should_send_correct_request() { - let new_screen = serde_json::from_str::("{\"id\":\"017a5104-524b-33d8-8026-9087b59e7eb5\",\"team_id\":\"016343c2-82b8-0000-a121-e30f1035875e\",\"created_at\":\"2021-06-28T05:07:55+00:00\",\"name\":\"Test\",\"is_enabled\":true,\"coords\":[55.22931, 48.90429],\"last_ping\":\"2021-08-25T06:17:20.728+00:00\",\"last_ip\":null,\"local_ip\":\"192.168.1.146\",\"mac\":\"b8:27:eb:d6:83:6f\",\"last_screenshot_time\":\"2021-08-25T06:09:04.399+00:00\",\"uptime\":\"230728.38\",\"load_avg\":\"0.14\",\"signal_strength\":null,\"interface\":\"eth0\",\"debug\":false,\"location\":\"Kamsko-Ust'inskiy rayon, Russia\",\"team\":\"016343c2-82b8-0000-a121-e30f1035875e\",\"timezone\":\"Europe/Moscow\",\"type\":\"hardware\",\"hostname\":\"srly-4shnfrdc5cd2p0p\",\"ws_open\":false,\"status\":\"Offline\",\"last_screenshot\":\"https://us-assets.screenlyapp.com/01CD1W50NR000A28F31W83B1TY/screenshots/01F98G8MJB6FC809MGGYTSWZNN/5267668e6db35498e61b83d4c702dbe8\",\"in_sync\":false,\"software_version\":\"Screenly 2 Player\",\"hardware_version\":\"Raspberry Pi 3B\",\"config\":{\"hdmi_mode\": 34, \"hdmi_boost\": 2, \"hdmi_drive\": 0, \"hdmi_group\": 0, \"verify_ssl\": true, \"audio_output\": \"hdmi\", \"hdmi_timings\": \"\", \"overscan_top\": 0, \"overscan_left\": 0, \"use_composite\": false, \"display_rotate\": 0, \"overscan_right\": 0, \"overscan_scale\": 0, \"overscan_bottom\": 0, \"disable_overscan\": 0, \"shuffle_playlist\": false, \"framebuffer_width\": 0, \"use_composite_pal\": false, \"framebuffer_height\": 0, \"hdmi_force_hotplug\": true, \"use_composite_ntsc\": false, \"hdmi_pixel_encoding\": 0, \"play_history_enabled\": false}}").unwrap(); - let new_screen_array = serde_json::Value::Array(vec![new_screen.clone()]); + // v4.1 POST /screens returns a bare object with the new screen's id, + // e.g. {"id": ""} -- not the full screen and not an array. + let created = json!({ "id": "01KY6ZMD1E3MYZA9DVQBQ187DT" }); let mock_server = MockServer::start(); let post_mock = mock_server.mock(|when, then| { when.method(POST) @@ -113,7 +114,7 @@ mod tests { format!("screenly-cli {}", env!("CARGO_PKG_VERSION")), ) .json_body(json!({"pin": "test-pin", "name": "test"})); - then.status(201).json_body(new_screen_array.clone()); + then.status(201).json_body(created.clone()); }); let config = Config::new(mock_server.base_url()); @@ -122,7 +123,7 @@ mod tests { let v = screen_command.add("test-pin", Some("test".to_string())); post_mock.assert(); assert!(v.is_ok()); - assert_eq!(v.unwrap().value, new_screen_array); + assert_eq!(v.unwrap().value, created); } #[test]