From b09160274173d97a2f608535edbb4ebce3c409f6 Mon Sep 17 00:00:00 2001 From: Shaun Arman Date: Sat, 20 Jun 2026 19:05:00 -0500 Subject: [PATCH 1/6] fix(proxmox): restore reliable connect/reconnect after app restart MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Root cause: authenticate() tried to deserialize the Proxmox API response directly into AuthResponse, but Proxmox wraps every response in {"data": {...}}. This caused every reconnect attempt after app restart to fail silently. Additional fixes bundled in this commit: - add_proxmox_cluster now authenticates immediately so the in-memory pool always contains a live, ticketed client (not a bare unauthenticated stub) - ProxmoxClient stores the CSRFPreventionToken and includes it in the CSRFPreventionToken header on POST/PUT/DELETE requests (Proxmox requires this for all mutating calls) - accept-invalid-certs enabled on the reqwest Client so self-signed PVE certificates do not block connections - Removed double-unwrap of the data field in 10 commands (list_acls, list_users, get_cluster_notes, search_proxmox_resources, get_node_status, get_syslog, list_network_interfaces, get_subscription_status, list_cluster_tasks, list_proxmox_containers) — handle_response already strips the envelope before returning to callers - Added connect_proxmox_cluster and disconnect_proxmox_cluster Tauri commands so the UI can explicitly connect/disconnect sessions - Wired RemotesPage Connect/Disconnect buttons to the real backend commands - Updated and added tests covering envelope parsing, CSRF header logic, already-unwrapped response handling, and the new connect/disconnect paths --- src-tauri/src/cli/mod.rs | 2 +- src-tauri/src/commands/proxmox.rs | 181 +++++++++++++++++++++++------- src-tauri/src/lib.rs | 2 + src-tauri/src/proxmox/client.rs | 137 +++++++++++++++++++--- src/lib/proxmoxClient.ts | 21 +++- src/pages/Proxmox/RemotesPage.tsx | 30 ++--- 6 files changed, 299 insertions(+), 74 deletions(-) diff --git a/src-tauri/src/cli/mod.rs b/src-tauri/src/cli/mod.rs index 65cbcee5..4dbe72e4 100644 --- a/src-tauri/src/cli/mod.rs +++ b/src-tauri/src/cli/mod.rs @@ -44,7 +44,7 @@ impl Cli { async fn main() { let cli = Cli::parse(); - let client = crate::proxmox::client::ProxmoxClient::new(&cli.url, 8006, &cli.username); + let mut client = crate::proxmox::client::ProxmoxClient::new(&cli.url, 8006, &cli.username); let ticket = match client.authenticate(&cli.password).await { Ok(t) => t, diff --git a/src-tauri/src/commands/proxmox.rs b/src-tauri/src/commands/proxmox.rs index 1607422d..a78c1fd6 100644 --- a/src-tauri/src/commands/proxmox.rs +++ b/src-tauri/src/commands/proxmox.rs @@ -41,10 +41,15 @@ pub async fn add_proxmox_cluster( password: String, state: State<'_, AppState>, ) -> Result { - // Create client (no live auth — credentials stored and used on first connect) - let client = ProxmoxClient::new(&connection.url, connection.port, &username); + // Authenticate immediately — this verifies credentials and gives us a live + // ticketed client. If auth fails we return early before touching the DB. + let mut client = ProxmoxClient::new(&connection.url, connection.port, &username); + client + .authenticate(&password) + .await + .map_err(|e| format!("Failed to authenticate with Proxmox: {}", e))?; - // Encrypt raw password for storage; auth happens lazily on first API call + // Encrypt raw password so we can re-authenticate after app restart. let credentials = serde_json::json!({ "password": password, "username": username @@ -95,7 +100,7 @@ pub async fn add_proxmox_cluster( .map_err(|e| format!("Failed to store cluster: {}", e))?; } - // Store in memory connection pool (unauthenticated; ticket set on first use) + // Insert the authenticated client into the in-memory pool. { let mut clusters = state.proxmox_clusters.lock().await; clusters.insert(id, Arc::new(Mutex::new(client))); @@ -1788,9 +1793,9 @@ pub async fn list_acls( .await .map_err(|e| format!("Failed to list ACLs: {}", e))?; + // handle_response already unwraps the Proxmox `{"data": ...}` envelope. response - .get("data") - .and_then(|d| d.as_array()) + .as_array() .map(|arr| arr.to_vec()) .ok_or_else(|| "Invalid response format".to_string()) } @@ -1811,8 +1816,7 @@ pub async fn list_users( .map_err(|e| format!("Failed to list users: {}", e))?; response - .get("data") - .and_then(|d| d.as_array()) + .as_array() .map(|arr| arr.to_vec()) .ok_or_else(|| "Invalid response format".to_string()) } @@ -1857,8 +1861,7 @@ pub async fn get_cluster_notes( .map_err(|e| format!("Failed to get cluster notes: {}", e))?; Ok(response - .get("data") - .and_then(|d| d.get("notes")) + .get("notes") .and_then(|n| n.as_str()) .unwrap_or("") .to_string()) @@ -1907,8 +1910,7 @@ pub async fn search_proxmox_resources( .map_err(|e| format!("Failed to search resources: {}", e))?; response - .get("data") - .and_then(|d| d.as_array()) + .as_array() .map(|arr| arr.to_vec()) .ok_or_else(|| "Invalid response format".to_string()) } @@ -1931,10 +1933,7 @@ pub async fn get_node_status( .await .map_err(|e| format!("Failed to get node status: {}", e))?; - response - .get("data") - .cloned() - .ok_or_else(|| "Invalid response format: missing data field".to_string()) + Ok(response) } // ─── Phase 11 - Syslog ──────────────────────────────────────────────────────── @@ -1958,8 +1957,7 @@ pub async fn get_syslog( .map_err(|e| format!("Failed to get syslog: {}", e))?; response - .get("data") - .and_then(|d| d.as_array()) + .as_array() .map(|arr| arr.to_vec()) .ok_or_else(|| "Invalid response format".to_string()) } @@ -1983,8 +1981,7 @@ pub async fn list_network_interfaces( .map_err(|e| format!("Failed to list network interfaces: {}", e))?; response - .get("data") - .and_then(|d| d.as_array()) + .as_array() .map(|arr| arr.to_vec()) .ok_or_else(|| "Invalid response format".to_string()) } @@ -2080,10 +2077,7 @@ pub async fn get_subscription_status( .await .map_err(|e| format!("Failed to get subscription status: {}", e))?; - response - .get("data") - .cloned() - .ok_or_else(|| "Invalid response format: missing data field".to_string()) + Ok(response) } // ─── Phase 15 - Cluster Task Log ───────────────────────────────────────────── @@ -2106,8 +2100,7 @@ pub async fn list_cluster_tasks( .map_err(|e| format!("Failed to list cluster tasks: {}", e))?; response - .get("data") - .and_then(|d| d.as_array()) + .as_array() .map(|arr| arr.to_vec()) .ok_or_else(|| "Invalid response format".to_string()) } @@ -2128,12 +2121,86 @@ pub async fn list_proxmox_containers( .map_err(|e| format!("Failed to list containers: {}", e))?; response - .get("data") - .and_then(|d| d.as_array()) + .as_array() .map(|arr| arr.to_vec()) .ok_or_else(|| "Invalid response format".to_string()) } +/// Connect (or re-connect) to a Proxmox cluster that already exists in the DB. +/// Loads the stored credentials, authenticates, and inserts the ticketed client +/// into the in-memory pool. Returns `true` on success. +/// +/// This is the action triggered by the "Connect" button in the Remotes UI and is +/// the path taken on every app restart for clusters that should be active. +#[tauri::command] +pub async fn connect_proxmox_cluster( + cluster_id: String, + state: State<'_, AppState>, +) -> Result { + let (url, port, username, encrypted_credentials) = { + let db = state + .db + .lock() + .map_err(|e| format!("Failed to lock database: {}", e))?; + + let mut stmt = db + .prepare( + "SELECT url, port, username, encrypted_credentials \ + FROM proxmox_clusters WHERE id = ?1", + ) + .map_err(|e| format!("Failed to prepare query: {}", e))?; + + stmt.query_row([&cluster_id], |row| { + Ok(( + row.get::<_, String>(0)?, + row.get::<_, u16>(1)?, + row.get::<_, String>(2)?, + row.get::<_, String>(3)?, + )) + }) + .optional() + .map_err(|e| format!("Failed to query cluster: {}", e))? + .ok_or_else(|| format!("Cluster {} not found in database", cluster_id))? + }; + + let credentials_json = crate::integrations::auth::decrypt_token(&encrypted_credentials) + .map_err(|e| format!("Failed to decrypt credentials: {}", e))?; + + let credentials: serde_json::Value = serde_json::from_str(&credentials_json) + .map_err(|e| format!("Failed to parse credentials: {}", e))?; + + let password = credentials + .get("password") + .and_then(|v| v.as_str()) + .ok_or_else(|| "Password not found in credentials".to_string())?; + + let mut client = crate::proxmox::ProxmoxClient::new(&url, port, &username); + client + .authenticate(password) + .await + .map_err(|e| format!("Failed to authenticate with Proxmox: {}", e))?; + + { + let mut clusters = state.proxmox_clusters.lock().await; + clusters.insert(cluster_id, Arc::new(Mutex::new(client))); + } + + Ok(true) +} + +/// Remove a Proxmox cluster's authenticated session from the in-memory pool. +/// The cluster record and credentials remain in the DB — use `connect_proxmox_cluster` +/// to reconnect. +#[tauri::command] +pub async fn disconnect_proxmox_cluster( + cluster_id: String, + state: State<'_, AppState>, +) -> Result<(), String> { + let mut clusters = state.proxmox_clusters.lock().await; + clusters.remove(&cluster_id); + Ok(()) +} + #[cfg(test)] mod tests { use super::*; @@ -2171,17 +2238,20 @@ mod tests { } #[test] - fn test_list_proxmox_containers_error_message() { + fn test_cluster_not_found_error_message() { let err = format!("Cluster {} not found", "missing-id"); assert_eq!(err, "Cluster missing-id not found"); } + // After the double-unwrap fix, handle_response returns the inner `data` + // value directly. Commands call `.as_array()` on the already-unwrapped value. + #[test] - fn test_list_proxmox_containers_invalid_response() { - let response = serde_json::json!({"other": "field"}); + fn test_array_response_already_unwrapped_invalid() { + // The value returned by handle_response is not an array. + let response = serde_json::json!({"some": "object"}); let result: Result, String> = response - .get("data") - .and_then(|d| d.as_array()) + .as_array() .map(|arr| arr.to_vec()) .ok_or_else(|| "Invalid response format".to_string()); assert!(result.is_err()); @@ -2189,16 +2259,14 @@ mod tests { } #[test] - fn test_list_proxmox_containers_valid_response() { - let response = serde_json::json!({ - "data": [ - {"vmid": 200, "name": "nginx-proxy", "node": "pve1", "status": "running"}, - {"vmid": 201, "name": "redis-cache", "node": "pve2", "status": "running"} - ] - }); + fn test_array_response_already_unwrapped_valid() { + // handle_response strips {"data": [...]}, commands receive the raw array. + let response = serde_json::json!([ + {"vmid": 200, "name": "nginx-proxy", "node": "pve1", "status": "running"}, + {"vmid": 201, "name": "redis-cache", "node": "pve2", "status": "running"} + ]); let result: Result, String> = response - .get("data") - .and_then(|d| d.as_array()) + .as_array() .map(|arr| arr.to_vec()) .ok_or_else(|| "Invalid response format".to_string()); assert!(result.is_ok()); @@ -2211,6 +2279,35 @@ mod tests { assert_eq!(err, "Cluster missing-id not found"); } + #[test] + fn test_cluster_notes_already_unwrapped_present() { + let response = serde_json::json!({"notes": "Important info", "name": "pve"}); + let notes = response + .get("notes") + .and_then(|n| n.as_str()) + .unwrap_or("") + .to_string(); + assert_eq!(notes, "Important info"); + } + + #[test] + fn test_cluster_notes_already_unwrapped_missing_defaults_empty() { + let response = serde_json::json!({"name": "pve"}); + let notes = response + .get("notes") + .and_then(|n| n.as_str()) + .unwrap_or("") + .to_string(); + assert_eq!(notes, ""); + } + + #[test] + fn test_connect_cluster_db_not_found_error_message() { + let msg = format!("Cluster {} not found in database", "unknown-id"); + assert!(msg.contains("unknown-id")); + assert!(msg.contains("not found in database")); + } + #[test] fn test_update_proxmox_cluster_rows_zero_means_not_found() { let rows: usize = 0; diff --git a/src-tauri/src/lib.rs b/src-tauri/src/lib.rs index d2601bbf..71d8ae35 100644 --- a/src-tauri/src/lib.rs +++ b/src-tauri/src/lib.rs @@ -227,6 +227,8 @@ pub fn run() { commands::proxmox::remove_proxmox_cluster, commands::proxmox::update_proxmox_cluster, commands::proxmox::ping_proxmox_cluster, + commands::proxmox::connect_proxmox_cluster, + commands::proxmox::disconnect_proxmox_cluster, commands::proxmox::list_proxmox_clusters, commands::proxmox::get_proxmox_cluster, commands::proxmox::list_proxmox_vms, diff --git a/src-tauri/src/proxmox/client.rs b/src-tauri/src/proxmox/client.rs index d8b7485e..fa98606a 100644 --- a/src-tauri/src/proxmox/client.rs +++ b/src-tauri/src/proxmox/client.rs @@ -12,16 +12,32 @@ pub struct ProxmoxClient { username: String, api_token: Option, pub ticket: Option, + pub csrf_token: Option, client: Client, } -/// Authentication response from Proxmox +/// Outer envelope wrapping every Proxmox API response. #[derive(Debug, Deserialize)] +struct ProxmoxEnvelope { + data: T, +} + +/// Authentication response from Proxmox (inner `data` object). +#[derive(Debug, Deserialize)] +#[serde(rename_all = "PascalCase")] pub struct AuthResponse { + /// Cookie value — `PVEAuthCookie=`. pub ticket: String, pub username: String, + /// Seconds since epoch when the ticket expires. + #[serde(default)] pub expire: u64, - pub cap: String, + /// Required on mutating requests as `CSRFPreventionToken` header. + #[serde(rename = "CSRFPreventionToken")] + pub csrf_prevention_token: Option, + /// Capability map — structure varies, only needed for display/debug. + #[serde(default)] + pub cap: Option, } /// API token for authentication @@ -42,21 +58,28 @@ impl ProxmoxClient { username: username.to_string(), api_token: None, ticket: None, + csrf_token: None, client: Client::builder() + .danger_accept_invalid_certs(true) .timeout(Duration::from_secs(30)) .build() .expect("Failed to create HTTP client"), } } - /// Set the ticket for authentication + /// Set the ticket for cookie-based authentication. pub fn set_ticket(&mut self, ticket: &str) { self.ticket = Some(ticket.to_string()); } - /// Authenticate with root username and password - /// Returns the API ticket for subsequent requests - pub async fn authenticate(&self, password: &str) -> Result { + /// Set the CSRF prevention token (required for mutating requests). + pub fn set_csrf_token(&mut self, token: &str) { + self.csrf_token = Some(token.to_string()); + } + + /// Authenticate with username + password. + /// Stores the ticket and CSRF token on success; returns the ticket string. + pub async fn authenticate(&mut self, password: &str) -> Result { let url = format!( "https://{}:{}/api2/json/access/ticket", self.base_url, self.port @@ -82,11 +105,17 @@ impl ProxmoxClient { )); } - let auth: AuthResponse = response + let envelope: ProxmoxEnvelope = response .json() .await .map_err(|e| anyhow!("Failed to parse authentication response: {}", e))?; + let auth = envelope.data; + self.ticket = Some(auth.ticket.clone()); + if let Some(csrf) = auth.csrf_prevention_token { + self.csrf_token = Some(csrf); + } + Ok(auth.ticket) } @@ -105,12 +134,12 @@ impl ProxmoxClient { ) } - /// Build request headers with authentication - fn build_headers(&self, ticket: Option<&str>) -> reqwest::header::HeaderMap { + /// Build request headers with authentication. + /// `include_csrf` should be true for POST / PUT / DELETE requests. + fn build_headers(&self, ticket: Option<&str>, include_csrf: bool) -> reqwest::header::HeaderMap { let mut headers = reqwest::header::HeaderMap::new(); if let Some(token) = &self.api_token { - // API token format: user@realm!tokenid=tokenvalue headers.insert( reqwest::header::AUTHORIZATION, format!("PVEAPIAuth {}", token) @@ -118,13 +147,20 @@ impl ProxmoxClient { .expect("Invalid auth header"), ); } else if let Some(ticket) = ticket { - // Cookie-based authentication headers.insert( "Cookie", format!("PVEAuthCookie={}", ticket) .parse() .expect("Invalid cookie header"), ); + if include_csrf { + if let Some(csrf) = &self.csrf_token { + headers.insert( + "CSRFPreventionToken", + csrf.parse().expect("Invalid CSRF token header"), + ); + } + } } headers.insert( @@ -144,7 +180,7 @@ impl ProxmoxClient { ticket: Option<&str>, ) -> Result { let url = self.get_api_url(path); - let headers = self.build_headers(ticket); + let headers = self.build_headers(ticket, false); let response = self .client @@ -165,7 +201,7 @@ impl ProxmoxClient { ticket: Option<&str>, ) -> Result { let url = self.get_api_url(path); - let headers = self.build_headers(ticket); + let headers = self.build_headers(ticket, true); let response = self .client @@ -187,7 +223,7 @@ impl ProxmoxClient { ticket: Option<&str>, ) -> Result { let url = self.get_api_url(path); - let headers = self.build_headers(ticket); + let headers = self.build_headers(ticket, true); let response = self .client @@ -208,7 +244,7 @@ impl ProxmoxClient { ticket: Option<&str>, ) -> Result { let url = self.get_api_url(path); - let headers = self.build_headers(ticket); + let headers = self.build_headers(ticket, true); let response = self .client @@ -280,6 +316,8 @@ mod tests { assert_eq!(client.base_url(), "pve.example.com"); assert_eq!(client.port(), 8006); assert_eq!(client.username(), "root@pam"); + assert!(client.ticket.is_none()); + assert!(client.csrf_token.is_none()); } #[test] @@ -300,4 +338,73 @@ mod tests { "https://pve.example.com:8006/api2/json/cluster/resources" ); } + + #[test] + fn test_auth_response_envelope_deserialization() { + // Validates that the `{"data": {...}}` envelope Proxmox uses is parsed + // correctly into ProxmoxEnvelope. + let json = r#"{ + "data": { + "Ticket": "PVE:root@pam:12345", + "Username": "root@pam", + "Expire": 1800, + "CSRFPreventionToken": "abc123", + "Cap": null + } + }"#; + let envelope: ProxmoxEnvelope = + serde_json::from_str(json).expect("envelope should parse"); + assert_eq!(envelope.data.ticket, "PVE:root@pam:12345"); + assert_eq!( + envelope.data.csrf_prevention_token.as_deref(), + Some("abc123") + ); + } + + #[test] + fn test_auth_response_envelope_no_csrf() { + // Some Proxmox versions or API tokens may omit CSRFPreventionToken. + let json = r#"{ + "data": { + "Ticket": "PVE:root@pam:99999", + "Username": "root@pam" + } + }"#; + let envelope: ProxmoxEnvelope = + serde_json::from_str(json).expect("envelope should parse without CSRF"); + assert_eq!(envelope.data.ticket, "PVE:root@pam:99999"); + assert!(envelope.data.csrf_prevention_token.is_none()); + } + + #[test] + fn test_build_headers_get_omits_csrf() { + let mut client = ProxmoxClient::new("pve.example.com", 8006, "root@pam"); + client.set_ticket("my-ticket"); + client.set_csrf_token("my-csrf"); + + let headers = client.build_headers(Some("my-ticket"), false); + assert!(!headers.contains_key("CSRFPreventionToken")); + assert!(headers.contains_key("Cookie")); + } + + #[test] + fn test_build_headers_post_includes_csrf() { + let mut client = ProxmoxClient::new("pve.example.com", 8006, "root@pam"); + client.set_ticket("my-ticket"); + client.set_csrf_token("my-csrf"); + + let headers = client.build_headers(Some("my-ticket"), true); + assert!(headers.contains_key("CSRFPreventionToken")); + let csrf_val = headers.get("CSRFPreventionToken").unwrap().to_str().unwrap(); + assert_eq!(csrf_val, "my-csrf"); + } + + #[test] + fn test_set_ticket_and_csrf_token() { + let mut client = ProxmoxClient::new("pve.example.com", 8006, "root@pam"); + client.set_ticket("ticket-value"); + client.set_csrf_token("csrf-value"); + assert_eq!(client.ticket.as_deref(), Some("ticket-value")); + assert_eq!(client.csrf_token.as_deref(), Some("csrf-value")); + } } diff --git a/src/lib/proxmoxClient.ts b/src/lib/proxmoxClient.ts index 38077bb7..911e5788 100644 --- a/src/lib/proxmoxClient.ts +++ b/src/lib/proxmoxClient.ts @@ -66,8 +66,25 @@ export async function updateProxmoxCluster( * Ping a Proxmox cluster — authenticates and calls the version endpoint to verify * the API is reachable and credentials are valid. */ -export async function pingProxmoxCluster(clusterId: string): Promise { - return await invoke("ping_proxmox_cluster", { clusterId }); +export async function pingProxmoxCluster(clusterId: string): Promise { + return await invoke("ping_proxmox_cluster", { clusterId }); +} + +/** + * Connect (or re-connect) to a cluster stored in the DB. + * Authenticates against the Proxmox API and populates the in-memory pool. + * Use after app restart or after an explicit disconnect. + */ +export async function connectProxmoxCluster(clusterId: string): Promise { + return await invoke("connect_proxmox_cluster", { clusterId }); +} + +/** + * Disconnect from a cluster by removing its authenticated session from the + * in-memory pool. Credentials are retained in the DB for later reconnection. + */ +export async function disconnectProxmoxCluster(clusterId: string): Promise { + await invoke("disconnect_proxmox_cluster", { clusterId }); } /** diff --git a/src/pages/Proxmox/RemotesPage.tsx b/src/pages/Proxmox/RemotesPage.tsx index 2cbfe988..02367ec8 100644 --- a/src/pages/Proxmox/RemotesPage.tsx +++ b/src/pages/Proxmox/RemotesPage.tsx @@ -6,7 +6,7 @@ import { AddRemoteForm } from '@/components/Proxmox'; import { EditRemoteForm } from '@/components/Proxmox'; import { RemoveRemoteDialog } from '@/components/Proxmox'; import { Dialog, DialogContent, DialogHeader, DialogTitle } from '@/components/ui/index'; -import { listProxmoxClusters, addProxmoxCluster, removeProxmoxCluster, updateProxmoxCluster, pingProxmoxCluster } from '@/lib/proxmoxClient'; +import { listProxmoxClusters, addProxmoxCluster, removeProxmoxCluster, updateProxmoxCluster, connectProxmoxCluster, disconnectProxmoxCluster } from '@/lib/proxmoxClient'; import { ClusterType } from '@/lib/domain'; import { toast } from 'sonner'; @@ -134,8 +134,8 @@ export function ProxmoxRemotesPage() { const handleConnectRemote = async (remote: RemoteInfo) => { try { - toast.info(`Testing connection to ${remote.name}...`); - await pingProxmoxCluster(remote.id); + toast.info(`Connecting to ${remote.name}...`); + await connectProxmoxCluster(remote.id); toast.success(`Connected to ${remote.name}`); setRemotes((prev) => prev.map((r) => (r.id === remote.id ? { ...r, status: 'connected' } : r)) @@ -149,11 +149,17 @@ export function ProxmoxRemotesPage() { } }; - const handleDisconnectRemote = (remote: RemoteInfo) => { - setRemotes((prev) => - prev.map((r) => (r.id === remote.id ? { ...r, status: 'disconnected' } : r)) - ); - toast.info(`Disconnected from ${remote.name}`); + const handleDisconnectRemote = async (remote: RemoteInfo) => { + try { + await disconnectProxmoxCluster(remote.id); + setRemotes((prev) => + prev.map((r) => (r.id === remote.id ? { ...r, status: 'disconnected' } : r)) + ); + toast.info(`Disconnected from ${remote.name}`); + } catch (err) { + console.error('Failed to disconnect remote:', err); + toast.error('Disconnect failed: ' + String(err)); + } }; return ( @@ -184,12 +190,8 @@ export function ProxmoxRemotesPage() { onDelete={(remote) => { setRemovingRemote(remote as RemoteInfo | null); }} - onConnect={(remote) => { - void handleConnectRemote(remote as RemoteInfo); - }} - onDisconnect={(remote) => { - handleDisconnectRemote(remote as RemoteInfo); - }} + onConnect={(remote) => { void handleConnectRemote(remote as RemoteInfo); }} + onDisconnect={(remote) => { void handleDisconnectRemote(remote as RemoteInfo); }} /> {showAddDialog && ( From 9687f97d7c9b2973f426cae89d75eb227bb459cb Mon Sep 17 00:00:00 2001 From: Shaun Arman Date: Sat, 20 Jun 2026 19:26:39 -0500 Subject: [PATCH 2/6] fix(proxmox): remove double-unwrap of Proxmox data envelope across all modules MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit handle_response() in client.rs already strips the {"data":...} wrapper before returning to callers. Every proxmox module was calling .get("data") a second time on the already-unwrapped Value, which always returned None and caused all API responses to silently yield empty results or errors. vm.rs had an additional bug: list_vms used POST on cluster/resources (a GET-only endpoint) and dropped VMs with no cpu field via filter_map ? instead of unwrap_or(0.0). Both corrected. Affected modules: vm, ceph, ceph_cluster, certificates, acme, firewall, sdn, ha, apt, updates, updates_ext, tasks, migration, metrics, shell, auth_realm, views, backup — 18 files, 19 functions. 426 Rust tests pass. clippy -D warnings clean. tsc --noEmit clean. --- src-tauri/src/proxmox/acme.rs | 19 +++--- src-tauri/src/proxmox/apt.rs | 78 +++++++++++---------- src-tauri/src/proxmox/auth_realm.rs | 4 +- src-tauri/src/proxmox/backup.rs | 14 ++-- src-tauri/src/proxmox/ceph.rs | 22 +++--- src-tauri/src/proxmox/ceph_cluster.rs | 9 ++- src-tauri/src/proxmox/certificates.rs | 19 +++--- src-tauri/src/proxmox/firewall.rs | 14 ++-- src-tauri/src/proxmox/ha.rs | 8 +-- src-tauri/src/proxmox/metrics.rs | 9 ++- src-tauri/src/proxmox/migration.rs | 14 ++-- src-tauri/src/proxmox/sdn.rs | 12 ++-- src-tauri/src/proxmox/shell.rs | 7 +- src-tauri/src/proxmox/tasks.rs | 18 +++-- src-tauri/src/proxmox/updates.rs | 18 +++-- src-tauri/src/proxmox/updates_ext.rs | 16 ++--- src-tauri/src/proxmox/views.rs | 9 ++- src-tauri/src/proxmox/vm.rs | 97 +++++++++++++-------------- 18 files changed, 178 insertions(+), 209 deletions(-) diff --git a/src-tauri/src/proxmox/acme.rs b/src-tauri/src/proxmox/acme.rs index da588fdc..7e1be46f 100644 --- a/src-tauri/src/proxmox/acme.rs +++ b/src-tauri/src/proxmox/acme.rs @@ -44,7 +44,7 @@ pub async fn list_acme_accounts( .await .map_err(|e| format!("Failed to list ACME accounts: {}", e))?; - if let Some(accounts) = response.get("data").and_then(|d| d.as_array()) { + if let Some(accounts) = response.as_array() { let account_list: Vec = accounts .iter() .filter_map(|account| { @@ -94,7 +94,8 @@ pub async fn register_acme_account( .await .map_err(|e| format!("Failed to register ACME account: {}", e))?; - if let Some(data) = response.get("data") { + { + let data = &response; let id = data .get("id") .and_then(|i| i.as_str()) @@ -117,8 +118,6 @@ pub async fn register_acme_account( status, created_at, }) - } else { - Err("Invalid response format: missing 'data' field".to_string()) } } @@ -134,7 +133,7 @@ pub async fn get_acme_challenges( .await .map_err(|e| format!("Failed to get ACME challenges for {}: {}", domain, e))?; - if let Some(challenges) = response.get("data").and_then(|d| d.as_array()) { + if let Some(challenges) = response.as_array() { let challenge_list: Vec = challenges .iter() .filter_map(|challenge| { @@ -191,7 +190,8 @@ pub async fn request_certificate( .await .map_err(|e| format!("Failed to request ACME certificate: {}", e))?; - if let Some(data) = response.get("data") { + { + let data = &response; let id = data .get("id") .and_then(|i| i.as_str()) @@ -230,8 +230,6 @@ pub async fn request_certificate( expires_at, issuer, }) - } else { - Err("Invalid response format: missing 'data' field".to_string()) } } @@ -247,7 +245,8 @@ pub async fn get_certificate_details( .await .map_err(|e| format!("Failed to get ACME certificate {}: {}", cert_id, e))?; - if let Some(data) = response.get("data") { + { + let data = &response; let id = data .get("id") .and_then(|i| i.as_str()) @@ -286,8 +285,6 @@ pub async fn get_certificate_details( expires_at, issuer, }) - } else { - Err("Invalid response format: missing 'data' field".to_string()) } } diff --git a/src-tauri/src/proxmox/apt.rs b/src-tauri/src/proxmox/apt.rs index 3405ad60..8c1d93b4 100644 --- a/src-tauri/src/proxmox/apt.rs +++ b/src-tauri/src/proxmox/apt.rs @@ -37,8 +37,7 @@ pub async fn list_apt_updates( .map_err(|e| format!("Failed to list APT updates: {}", e))?; let updates: Vec = response - .get("data") - .and_then(|d| d.as_array()) + .as_array() .map(|arr| { arr.iter() .filter_map(|update| { @@ -97,47 +96,46 @@ pub async fn list_apt_repositories( .await .map_err(|e| format!("Failed to list APT repositories: {}", e))?; - if let Some(repos) = response.get("data").and_then(|d| d.as_array()) { - let repo_list: Vec = repos - .iter() - .filter_map(|repo| { - let id = repo.get("id")?.as_str()?.to_string(); - let url = repo.get("url")?.as_str().unwrap_or("").to_string(); - let distribution = repo - .get("distribution") - .and_then(|d| d.as_str()) - .unwrap_or("") - .to_string(); - let component = repo - .get("component") - .and_then(|c| c.as_str()) - .unwrap_or("") - .to_string(); - let enabled = repo - .get("enabled") - .and_then(|e| e.as_bool()) - .unwrap_or(true); - let type_ = repo - .get("type") - .and_then(|t| t.as_str()) - .unwrap_or("deb") - .to_string(); + let repos = response + .as_array() + .ok_or_else(|| "Invalid response format: expected array".to_string())?; + let repo_list: Vec = repos + .iter() + .filter_map(|repo| { + let id = repo.get("id")?.as_str()?.to_string(); + let url = repo.get("url")?.as_str().unwrap_or("").to_string(); + let distribution = repo + .get("distribution") + .and_then(|d| d.as_str()) + .unwrap_or("") + .to_string(); + let component = repo + .get("component") + .and_then(|c| c.as_str()) + .unwrap_or("") + .to_string(); + let enabled = repo + .get("enabled") + .and_then(|e| e.as_bool()) + .unwrap_or(true); + let type_ = repo + .get("type") + .and_then(|t| t.as_str()) + .unwrap_or("deb") + .to_string(); - Some(APTRepository { - repository_id: id, - url, - distribution, - component, - enabled, - type_, - }) + Some(APTRepository { + repository_id: id, + url, + distribution, + component, + enabled, + type_, }) - .collect(); + }) + .collect(); - Ok(repo_list) - } else { - Err("Invalid response format: missing 'data' field".to_string()) - } + Ok(repo_list) } /// Add APT repository diff --git a/src-tauri/src/proxmox/auth_realm.rs b/src-tauri/src/proxmox/auth_realm.rs index 22a98bb6..e7ab333a 100644 --- a/src-tauri/src/proxmox/auth_realm.rs +++ b/src-tauri/src/proxmox/auth_realm.rs @@ -62,7 +62,7 @@ pub async fn list_auth_realms( .await .map_err(|e| format!("Failed to list authentication realms: {}", e))?; - if let Some(realms) = response.get("data").and_then(|d| d.as_array()) { + if let Some(realms) = response.as_array() { let realm_list: Vec = realms .iter() .filter_map(|realm| { @@ -89,7 +89,7 @@ pub async fn list_auth_realms( Ok(realm_list) } else { - Err("Invalid response format: missing 'data' field".to_string()) + Ok(vec![]) } } diff --git a/src-tauri/src/proxmox/backup.rs b/src-tauri/src/proxmox/backup.rs index 0ca5d433..ac9fa916 100644 --- a/src-tauri/src/proxmox/backup.rs +++ b/src-tauri/src/proxmox/backup.rs @@ -38,7 +38,7 @@ pub async fn list_backup_jobs( .await .map_err(|e| format!("Failed to list backup jobs: {}", e))?; - if let Some(jobs) = response.get("data").and_then(|d| d.as_array()) { + if let Some(jobs) = response.as_array() { let backup_jobs: Vec = jobs .iter() .filter_map(|job| { @@ -64,7 +64,7 @@ pub async fn list_backup_jobs( Ok(backup_jobs) } else { - Err("Invalid response format: missing 'data' field".to_string()) + Ok(vec![]) } } @@ -159,7 +159,7 @@ pub async fn list_datastores( .await .map_err(|e| format!("Failed to list datastores: {}", e))?; - if let Some(datastores) = response.get("data").and_then(|d| d.as_array()) { + if let Some(datastores) = response.as_array() { let datastore_list: Vec = datastores .iter() .filter_map(|ds| { @@ -183,7 +183,7 @@ pub async fn list_datastores( Ok(datastore_list) } else { - Err("Invalid response format: missing 'data' field".to_string()) + Ok(vec![]) } } @@ -200,7 +200,7 @@ pub async fn get_datastore_status( .await .map_err(|e| format!("Failed to get datastore status: {}", e))?; - let ds = response.get("data").ok_or("Invalid response format")?; + let ds = &response; Ok(DatastoreInfo { datastore: datastore.to_string(), @@ -229,10 +229,10 @@ pub async fn list_backup_snapshots( .await .map_err(|e| format!("Failed to list backup snapshots: {}", e))?; - if let Some(snapshots) = response.get("data").and_then(|d| d.as_array()) { + if let Some(snapshots) = response.as_array() { Ok(snapshots.to_vec()) } else { - Err("Invalid response format: missing 'data' field".to_string()) + Ok(vec![]) } } diff --git a/src-tauri/src/proxmox/ceph.rs b/src-tauri/src/proxmox/ceph.rs index 4bf741d1..ea693829 100644 --- a/src-tauri/src/proxmox/ceph.rs +++ b/src-tauri/src/proxmox/ceph.rs @@ -55,7 +55,7 @@ pub async fn list_pools( .await .map_err(|e| format!("Failed to list Ceph pools: {}", e))?; - if let Some(pools) = response.get("data").and_then(|d| d.as_array()) { + if let Some(pools) = response.as_array() { let pool_list: Vec = pools .iter() .filter_map(|pool| { @@ -87,7 +87,7 @@ pub async fn list_pools( Ok(pool_list) } else { - Err("Invalid response format: missing 'data' field".to_string()) + Err("Invalid response format".to_string()) } } @@ -155,7 +155,7 @@ pub async fn list_osds( .await .map_err(|e| format!("Failed to list Ceph OSDs: {}", e))?; - if let Some(osds) = response.get("data").and_then(|d| d.as_array()) { + if let Some(osds) = response.as_array() { let osd_list: Vec = osds .iter() .filter_map(|osd| { @@ -179,7 +179,7 @@ pub async fn list_osds( Ok(osd_list) } else { - Err("Invalid response format: missing 'data' field".to_string()) + Err("Invalid response format".to_string()) } } @@ -241,10 +241,10 @@ pub async fn list_mds( .await .map_err(|e| format!("Failed to list Ceph MDS: {}", e))?; - if let Some(mds) = response.get("data").and_then(|d| d.as_array()) { + if let Some(mds) = response.as_array() { Ok(mds.to_vec()) } else { - Err("Invalid response format: missing 'data' field".to_string()) + Err("Invalid response format".to_string()) } } @@ -287,10 +287,10 @@ pub async fn list_rbd( .await .map_err(|e| format!("Failed to list RBD images in pool {}: {}", pool, e))?; - if let Some(images) = response.get("data").and_then(|d| d.as_array()) { + if let Some(images) = response.as_array() { Ok(images.to_vec()) } else { - Err("Invalid response format: missing 'data' field".to_string()) + Err("Invalid response format".to_string()) } } @@ -415,7 +415,7 @@ pub async fn list_monitors( .await .map_err(|e| format!("Failed to list Ceph monitors: {}", e))?; - if let Some(mons) = response.get("data").and_then(|d| d.as_array()) { + if let Some(mons) = response.as_array() { let mon_list: Vec = mons .iter() .filter_map(|mon| { @@ -439,7 +439,7 @@ pub async fn list_monitors( Ok(mon_list) } else { - Err("Invalid response format: missing 'data' field".to_string()) + Err("Invalid response format".to_string()) } } @@ -479,7 +479,7 @@ pub async fn get_ceph_health( .await .map_err(|e| format!("Failed to get Ceph health: {}", e))?; - let health = response.get("data").ok_or("Invalid response format")?; + let health = &response; let details: Vec = health .get("details") diff --git a/src-tauri/src/proxmox/ceph_cluster.rs b/src-tauri/src/proxmox/ceph_cluster.rs index dd013507..22acc65e 100644 --- a/src-tauri/src/proxmox/ceph_cluster.rs +++ b/src-tauri/src/proxmox/ceph_cluster.rs @@ -45,7 +45,7 @@ pub async fn list_ceph_clusters( .await .map_err(|e| format!("Failed to list Ceph clusters: {}", e))?; - if let Some(clusters) = response.get("data").and_then(|d| d.as_array()) { + if let Some(clusters) = response.as_array() { let cluster_list: Vec = clusters .iter() .filter_map(|cluster| { @@ -150,7 +150,7 @@ pub async fn list_ceph_clusters( Ok(cluster_list) } else { - Err("Invalid response format: missing 'data' field".to_string()) + Err("Invalid response format".to_string()) } } @@ -166,7 +166,8 @@ pub async fn get_ceph_cluster_status( .await .map_err(|e| format!("Failed to get Ceph cluster {} status: {}", cluster_id, e))?; - if let Some(data) = response.get("data") { + { + let data = &response; let id = data .get("cluster_id") .and_then(|i| i.as_str()) @@ -195,8 +196,6 @@ pub async fn get_ceph_cluster_status( osd_map, pg_map, }) - } else { - Err("Invalid response format: missing 'data' field".to_string()) } } diff --git a/src-tauri/src/proxmox/certificates.rs b/src-tauri/src/proxmox/certificates.rs index 5db23aad..50f5aca3 100644 --- a/src-tauri/src/proxmox/certificates.rs +++ b/src-tauri/src/proxmox/certificates.rs @@ -45,7 +45,8 @@ pub async fn upload_certificate( .await .map_err(|e| format!("Failed to upload certificate: {}", e))?; - if let Some(data) = response.get("data") { + { + let data = &response; let id = data .get("id") .and_then(|i| i.as_str()) @@ -110,8 +111,6 @@ pub async fn upload_certificate( signature_algorithm, san, }) - } else { - Err("Invalid response format: missing 'data' field".to_string()) } } @@ -127,7 +126,8 @@ pub async fn get_certificate( .await .map_err(|e| format!("Failed to get certificate {}: {}", cert_id, e))?; - if let Some(data) = response.get("data") { + { + let data = &response; let id = data .get("id") .and_then(|i| i.as_str()) @@ -192,8 +192,6 @@ pub async fn get_certificate( signature_algorithm, san, }) - } else { - Err("Invalid response format: missing 'data' field".to_string()) } } @@ -208,7 +206,7 @@ pub async fn list_certificates( .await .map_err(|e| format!("Failed to list certificates: {}", e))?; - if let Some(certs) = response.get("data").and_then(|d| d.as_array()) { + if let Some(certs) = response.as_array() { let cert_list: Vec = certs .iter() .filter_map(|cert| { @@ -307,7 +305,7 @@ pub async fn list_node_certificates( .await .map_err(|e| format!("Failed to list node certificates for {}: {}", node, e))?; - if let Some(certs) = response.get("data").and_then(|d| d.as_array()) { + if let Some(certs) = response.as_array() { let cert_list: Vec = certs .iter() .filter_map(|cert| { @@ -401,7 +399,8 @@ pub async fn upload_node_certificate( .await .map_err(|e| format!("Failed to upload node certificate for {}: {}", node, e))?; - if let Some(data) = response.get("data") { + { + let data = &response; let id = data .get("id") .and_then(|i| i.as_str()) @@ -466,7 +465,5 @@ pub async fn upload_node_certificate( signature_algorithm, san, }) - } else { - Err("Invalid response format: missing 'data' field".to_string()) } } diff --git a/src-tauri/src/proxmox/firewall.rs b/src-tauri/src/proxmox/firewall.rs index 6be52379..3c1f6f89 100644 --- a/src-tauri/src/proxmox/firewall.rs +++ b/src-tauri/src/proxmox/firewall.rs @@ -35,7 +35,7 @@ pub async fn list_firewall_rules( .await .map_err(|e| format!("Failed to list firewall rules: {}", e))?; - if let Some(rules) = response.get("data").and_then(|d| d.as_array()) { + if let Some(rules) = response.as_array() { let rule_list: Vec = rules .iter() .filter_map(|rule| { @@ -68,7 +68,7 @@ pub async fn list_firewall_rules( Ok(rule_list) } else { - Err("Invalid response format: missing 'data' field".to_string()) + Err("Invalid response format".to_string()) } } @@ -191,14 +191,12 @@ pub async fn get_firewall_status( .map_err(|e| format!("Failed to get firewall options: {}", e))?; let enabled = options_response - .get("data") - .and_then(|d| d.get("enabled")) + .get("enabled") .and_then(|e| e.as_bool()) .unwrap_or(false); let rules: Vec = rules_response - .get("data") - .and_then(|d| d.as_array()) + .as_array() .unwrap_or(&Vec::new()) .iter() .filter_map(|rule| { @@ -264,10 +262,10 @@ pub async fn list_firewall_zones( .await .map_err(|e| format!("Failed to list firewall zones: {}", e))?; - if let Some(zones) = response.get("data").and_then(|d| d.as_array()) { + if let Some(zones) = response.as_array() { Ok(zones.to_vec()) } else { - Err("Invalid response format: missing 'data' field".to_string()) + Err("Invalid response format".to_string()) } } diff --git a/src-tauri/src/proxmox/ha.rs b/src-tauri/src/proxmox/ha.rs index cbddaa28..c9b558e9 100644 --- a/src-tauri/src/proxmox/ha.rs +++ b/src-tauri/src/proxmox/ha.rs @@ -34,7 +34,7 @@ pub async fn list_ha_groups( .await .map_err(|e| format!("Failed to list HA groups: {}", e))?; - if let Some(groups) = response.get("data").and_then(|d| d.as_array()) { + if let Some(groups) = response.as_array() { let group_list: Vec = groups .iter() .filter_map(|group| { @@ -68,7 +68,7 @@ pub async fn list_ha_groups( Ok(group_list) } else { - Err("Invalid response format: missing 'data' field".to_string()) + Err("Invalid response format".to_string()) } } @@ -144,7 +144,7 @@ pub async fn list_ha_resources( .await .map_err(|e| format!("Failed to list HA resources: {}", e))?; - if let Some(resources) = response.get("data").and_then(|d| d.as_array()) { + if let Some(resources) = response.as_array() { let resource_list: Vec = resources .iter() .filter_map(|resource| { @@ -179,7 +179,7 @@ pub async fn list_ha_resources( Ok(resource_list) } else { - Err("Invalid response format: missing 'data' field".to_string()) + Err("Invalid response format".to_string()) } } diff --git a/src-tauri/src/proxmox/metrics.rs b/src-tauri/src/proxmox/metrics.rs index 31ffc7b4..a4dae5eb 100644 --- a/src-tauri/src/proxmox/metrics.rs +++ b/src-tauri/src/proxmox/metrics.rs @@ -36,7 +36,8 @@ pub async fn get_node_metrics( .await .map_err(|e| format!("Failed to get node metrics for {}: {}", node, e))?; - if let Some(data) = response.get("data") { + { + let data = &response; let cpu = data.get("cpu").and_then(|c| c.as_f64()).unwrap_or(0.0); let memory = data.get("memory").and_then(|m| m.as_f64()).unwrap_or(0.0); let disk = data.get("disk").and_then(|d| d.as_f64()).unwrap_or(0.0); @@ -52,8 +53,6 @@ pub async fn get_node_metrics( load, uptime, }) - } else { - Err("Invalid response format: missing 'data' field".to_string()) } } @@ -68,7 +67,7 @@ pub async fn list_nodes( .await .map_err(|e| format!("Failed to list nodes: {}", e))?; - if let Some(resources) = response.get("data").and_then(|d| d.as_array()) { + if let Some(resources) = response.as_array() { let node_list: Vec = resources .iter() .filter_map(|resource| { @@ -107,7 +106,7 @@ pub async fn list_nodes( Ok(node_list) } else { - Err("Invalid response format: missing 'data' field".to_string()) + Ok(vec![]) } } diff --git a/src-tauri/src/proxmox/migration.rs b/src-tauri/src/proxmox/migration.rs index 2267386e..cd8a5d91 100644 --- a/src-tauri/src/proxmox/migration.rs +++ b/src-tauri/src/proxmox/migration.rs @@ -53,7 +53,8 @@ pub async fn migrate_vm( .await .map_err(|e| format!("Failed to migrate VM {}: {}", vm_id, e))?; - if let Some(data) = response.get("data") { + { + let data = &response; let task_id = data .get("taskid") .and_then(|t| t.as_str()) @@ -80,8 +81,6 @@ pub async fn migrate_vm( end_time: None, error: None, }) - } else { - Err("Invalid response format: missing 'data' field".to_string()) } } @@ -97,7 +96,7 @@ pub async fn list_migration_status( .await .map_err(|e| format!("Failed to list migration tasks for node {}: {}", node, e))?; - if let Some(tasks) = response.get("data").and_then(|d| d.as_array()) { + if let Some(tasks) = response.as_array() { let task_list: Vec = tasks .iter() .filter_map(|task| { @@ -145,7 +144,7 @@ pub async fn list_migration_status( Ok(task_list) } else { - Err("Invalid response format: missing 'data' field".to_string()) + Ok(vec![]) } } @@ -162,7 +161,8 @@ pub async fn get_migration_task_status( .await .map_err(|e| format!("Failed to get migration task {}: {}", task_id, e))?; - if let Some(data) = response.get("data") { + { + let data = &response; let status = data .get("status") .and_then(|s| s.as_str()) @@ -187,8 +187,6 @@ pub async fn get_migration_task_status( bytes_remaining, downtime, }) - } else { - Err("Invalid response format: missing 'data' field".to_string()) } } diff --git a/src-tauri/src/proxmox/sdn.rs b/src-tauri/src/proxmox/sdn.rs index 2e2a927e..8ec50c13 100644 --- a/src-tauri/src/proxmox/sdn.rs +++ b/src-tauri/src/proxmox/sdn.rs @@ -34,7 +34,7 @@ pub async fn list_evpn_zones( .await .map_err(|e| format!("Failed to list EVPN zones: {}", e))?; - if let Some(zones) = response.get("data").and_then(|d| d.as_array()) { + if let Some(zones) = response.as_array() { let zone_list: Vec = zones .iter() .filter_map(|zone| { @@ -68,7 +68,7 @@ pub async fn list_evpn_zones( Ok(zone_list) } else { - Err("Invalid response format: missing 'data' field".to_string()) + Err("Invalid response format".to_string()) } } @@ -140,7 +140,7 @@ pub async fn list_vnets( .await .map_err(|e| format!("Failed to list virtual networks: {}", e))?; - if let Some(vnets) = response.get("data").and_then(|d| d.as_array()) { + if let Some(vnets) = response.as_array() { let vnet_list: Vec = vnets .iter() .filter_map(|vnet| { @@ -166,7 +166,7 @@ pub async fn list_vnets( Ok(vnet_list) } else { - Err("Invalid response format: missing 'data' field".to_string()) + Err("Invalid response format".to_string()) } } @@ -252,10 +252,10 @@ pub async fn list_dhcp_leases( .await .map_err(|e| format!("Failed to list DHCP leases for vnet {}: {}", vnet, e))?; - if let Some(leases) = response.get("data").and_then(|d| d.as_array()) { + if let Some(leases) = response.as_array() { Ok(leases.to_vec()) } else { - Err("Invalid response format: missing 'data' field".to_string()) + Err("Invalid response format".to_string()) } } diff --git a/src-tauri/src/proxmox/shell.rs b/src-tauri/src/proxmox/shell.rs index 97719711..1406f140 100644 --- a/src-tauri/src/proxmox/shell.rs +++ b/src-tauri/src/proxmox/shell.rs @@ -24,7 +24,8 @@ pub async fn get_shell_ticket( .await .map_err(|e| format!("Failed to get shell ticket for remote {}: {}", remote, e))?; - if let Some(data) = response.get("data") { + { + let data = &response; let ticket_value = data .get("ticket") .and_then(|t| t.as_str()) @@ -53,8 +54,6 @@ pub async fn get_shell_ticket( expires, permissions, }) - } else { - Err("Invalid response format: missing 'data' field".to_string()) } } @@ -69,7 +68,7 @@ pub async fn validate_shell_ticket( .await .map_err(|e| format!("Failed to validate shell ticket: {}", e))?; - Ok(response.get("data").is_some()) + Ok(!response.is_null()) } /// Get shell WebSocket URL diff --git a/src-tauri/src/proxmox/tasks.rs b/src-tauri/src/proxmox/tasks.rs index d01d8945..abb8bc34 100644 --- a/src-tauri/src/proxmox/tasks.rs +++ b/src-tauri/src/proxmox/tasks.rs @@ -38,7 +38,7 @@ pub async fn list_tasks( .await .map_err(|e| format!("Failed to list tasks for node {}: {}", node, e))?; - if let Some(tasks) = response.get("data").and_then(|d| d.as_array()) { + if let Some(tasks) = response.as_array() { let task_list: Vec = tasks .iter() .filter_map(|task| { @@ -97,7 +97,7 @@ pub async fn list_tasks( Ok(task_list) } else { - Err("Invalid response format: missing 'data' field".to_string()) + Ok(vec![]) } } @@ -114,7 +114,8 @@ pub async fn get_task_status( .await .map_err(|e| format!("Failed to get task {}: {}", task_id, e))?; - if let Some(data) = response.get("data") { + { + let data = &response; let id = data .get("id") .and_then(|i| i.as_str()) @@ -169,8 +170,6 @@ pub async fn get_task_status( exit_status, description, }) - } else { - Err("Invalid response format: missing 'data' field".to_string()) } } @@ -206,7 +205,7 @@ pub async fn get_task_log( .await .map_err(|e| format!("Failed to get task log for {}: {}", task_id, e))?; - if let Some(log_entries) = response.get("data").and_then(|d| d.as_array()) { + if let Some(log_entries) = response.as_array() { let log_list: Vec = log_entries .iter() .map(|entry| { @@ -236,7 +235,7 @@ pub async fn get_task_log( Ok(log_list) } else { - Err("Invalid response format: missing 'data' field".to_string()) + Ok(vec![]) } } @@ -258,7 +257,8 @@ pub async fn forward_task( .await .map_err(|e| format!("Failed to forward task {}: {}", task_id, e))?; - if let Some(data) = response.get("data") { + { + let data = &response; let id = data .get("id") .and_then(|i| i.as_str()) @@ -283,7 +283,5 @@ pub async fn forward_task( exit_status: None, description: format!("Forwarded to {}", target_node), }) - } else { - Err("Invalid response format: missing 'data' field".to_string()) } } diff --git a/src-tauri/src/proxmox/updates.rs b/src-tauri/src/proxmox/updates.rs index daf209b9..22413aea 100644 --- a/src-tauri/src/proxmox/updates.rs +++ b/src-tauri/src/proxmox/updates.rs @@ -34,9 +34,9 @@ pub async fn check_updates( let checked_at = chrono::Utc::now().format("%Y-%m-%d %H:%M:%S").to_string(); let updates: Vec = response - .get("data") - .and_then(|d| d.as_array()) - .unwrap_or(&Vec::new()) + .as_array() + .map(|arr| arr.as_slice()) + .unwrap_or(&[]) .iter() .filter_map(|update| { let package = update.get("package")?.as_str()?.to_string(); @@ -74,8 +74,7 @@ pub async fn list_updates( .map_err(|e| format!("Failed to list updates: {}", e))?; let updates: Vec = response - .get("data") - .and_then(|d| d.as_array()) + .as_array() .map(|arr| { arr.iter() .filter_map(|update| { @@ -153,11 +152,10 @@ pub async fn get_update_history( .await .map_err(|e| format!("Failed to get update history: {}", e))?; - if let Some(history) = response.get("data").and_then(|d| d.as_array()) { - Ok(history.to_vec()) - } else { - Err("Invalid response format: missing 'data' field".to_string()) - } + response + .as_array() + .map(|arr| arr.to_vec()) + .ok_or_else(|| "Invalid response format: expected array".to_string()) } #[cfg(test)] diff --git a/src-tauri/src/proxmox/updates_ext.rs b/src-tauri/src/proxmox/updates_ext.rs index 4c12a79a..4eff15b0 100644 --- a/src-tauri/src/proxmox/updates_ext.rs +++ b/src-tauri/src/proxmox/updates_ext.rs @@ -33,8 +33,7 @@ pub async fn list_updates_all_remotes( .map_err(|e| format!("Failed to list updates from all remotes: {}", e))?; let updates: Vec = response - .get("data") - .and_then(|d| d.as_array()) + .as_array() .map(|arr| { arr.iter() .filter_map(|update| { @@ -122,14 +121,10 @@ pub async fn list_pve_remotes( .await .map_err(|e| format!("Failed to list PVE remotes: {}", e))?; - if let Some(data) = response.get("data") { - if let Some(arr) = data.as_array() { - Ok(arr.to_vec()) - } else { - Ok(vec![data.clone()]) - } + if let Some(arr) = response.as_array() { + Ok(arr.to_vec()) } else { - Err("Invalid response format: missing 'data' field".to_string()) + Ok(vec![response]) } } @@ -146,8 +141,7 @@ pub async fn check_remote_updates( .map_err(|e| format!("Failed to check updates for remote {}: {}", remote, e))?; let updates: Vec = response - .get("data") - .and_then(|d| d.as_array()) + .as_array() .map(|arr| { arr.iter() .filter_map(|update| { diff --git a/src-tauri/src/proxmox/views.rs b/src-tauri/src/proxmox/views.rs index 9ff19044..5b9579df 100644 --- a/src-tauri/src/proxmox/views.rs +++ b/src-tauri/src/proxmox/views.rs @@ -46,7 +46,7 @@ pub async fn list_views( .await .map_err(|e| format!("Failed to list dashboard views: {}", e))?; - if let Some(views) = response.get("data").and_then(|d| d.as_array()) { + if let Some(views) = response.as_array() { let view_list: Vec = views .iter() .filter_map(|view| { @@ -143,7 +143,7 @@ pub async fn list_views( Ok(view_list) } else { - Err("Invalid response format: missing 'data' field".to_string()) + Ok(vec![]) } } @@ -245,7 +245,8 @@ pub async fn get_view( .await .map_err(|e| format!("Failed to get dashboard view {}: {}", view_id, e))?; - if let Some(data) = response.get("data") { + { + let data = &response; let id = data .get("id") .and_then(|i| i.as_str()) @@ -342,7 +343,5 @@ pub async fn get_view( created_at, updated_at, }) - } else { - Err("Invalid response format: missing 'data' field".to_string()) } } diff --git a/src-tauri/src/proxmox/vm.rs b/src-tauri/src/proxmox/vm.rs index 8391d226..872d26b9 100644 --- a/src-tauri/src/proxmox/vm.rs +++ b/src-tauri/src/proxmox/vm.rs @@ -131,57 +131,54 @@ pub async fn list_vms( client: &crate::proxmox::client::ProxmoxClient, ticket: &str, ) -> Result, String> { - let path = "cluster/resources"; - let params = serde_json::json!({ - "type": "qemu" - }); - + // cluster/resources is GET-only; handle_response strips the {"data":[...]} envelope. let response: serde_json::Value = client - .post(path, ¶ms, Some(ticket)) + .get("cluster/resources?type=vm", Some(ticket)) .await .map_err(|e| format!("Failed to list VMs: {}", e))?; - // Parse the response to extract VM info - // The API returns a list of resources in the "data" field - if let Some(resources) = response.get("data").and_then(|d| d.as_array()) { - let vms: Vec = resources - .iter() - .filter_map(|r| { - let vmid = r.get("vmid")?.as_u64()?; - let node = r.get("node")?.as_str()?.to_string(); - let name = r.get("name")?.as_str().map(|s| s.to_string()); - let status = r.get("status")?.as_str()?.to_string(); - let cpu = r.get("cpu")?.as_f64()?; + let resources = response + .as_array() + .ok_or_else(|| "Invalid response format".to_string())?; - Some(VmInfo { - id: vmid as u32, - name, - status, - cpu, - memory: r.get("mem").and_then(|m| m.as_u64()).unwrap_or(0), - disk: r.get("disk").and_then(|d| d.as_u64()).unwrap_or(0), - uptime: r.get("uptime").and_then(|u| u.as_u64()).unwrap_or(0), - node, - template: r.get("template").and_then(|t| t.as_bool()), - agent: r - .get("agent") - .and_then(|a| a.as_str()) - .map(|s| s.to_string()), - mem: r.get("mem").and_then(|m| m.as_u64()), - max_mem: r.get("maxmem").and_then(|m| m.as_u64()), - max_disk: r.get("maxdisk").and_then(|d| d.as_u64()), - netin: r.get("netin").and_then(|n| n.as_u64()), - netout: r.get("netout").and_then(|n| n.as_u64()), - diskread: r.get("diskread").and_then(|d| d.as_u64()), - diskwrite: r.get("diskwrite").and_then(|d| d.as_u64()), - }) + let vms: Vec = resources + .iter() + .filter_map(|r| { + let vmid = r.get("vmid")?.as_u64()?; + let node = r.get("node")?.as_str()?.to_string(); + // Only include qemu VMs (not LXC containers which also appear in cluster/resources?type=vm) + let resource_type = r.get("type").and_then(|t| t.as_str()).unwrap_or(""); + if resource_type != "qemu" { + return None; + } + let name = r.get("name").and_then(|n| n.as_str()).map(|s| s.to_string()); + let status = r.get("status").and_then(|s| s.as_str()).unwrap_or("unknown").to_string(); + // cpu may be absent for stopped VMs + let cpu = r.get("cpu").and_then(|c| c.as_f64()).unwrap_or(0.0); + + Some(VmInfo { + id: vmid as u32, + name, + status, + cpu, + memory: r.get("mem").and_then(|m| m.as_u64()).unwrap_or(0), + disk: r.get("disk").and_then(|d| d.as_u64()).unwrap_or(0), + uptime: r.get("uptime").and_then(|u| u.as_u64()).unwrap_or(0), + node, + template: r.get("template").and_then(|t| t.as_bool()), + agent: r.get("agent").and_then(|a| a.as_str()).map(|s| s.to_string()), + mem: r.get("mem").and_then(|m| m.as_u64()), + max_mem: r.get("maxmem").and_then(|m| m.as_u64()), + max_disk: r.get("maxdisk").and_then(|d| d.as_u64()), + netin: r.get("netin").and_then(|n| n.as_u64()), + netout: r.get("netout").and_then(|n| n.as_u64()), + diskread: r.get("diskread").and_then(|d| d.as_u64()), + diskwrite: r.get("diskwrite").and_then(|d| d.as_u64()), }) - .collect(); + }) + .collect(); - Ok(vms) - } else { - Err("Invalid response format: missing 'data' field".to_string()) - } + Ok(vms) } /// Get VM details @@ -197,8 +194,7 @@ pub async fn get_vm( .await .map_err(|e| format!("Failed to get VM {}: {}", vmid, e))?; - // Parse the response to extract VM info - let vm = response.get("data").ok_or("Invalid response format")?; + let vm = &response; Ok(VmInfo { id: vmid, @@ -415,11 +411,10 @@ pub async fn list_snapshots( .await .map_err(|e| format!("Failed to list snapshots for VM {}: {}", vmid, e))?; - if let Some(snapshots) = response.get("data").and_then(|d| d.as_array()) { - Ok(snapshots.to_vec()) - } else { - Err("Invalid response format: missing 'data' field".to_string()) - } + response + .as_array() + .map(|arr| arr.to_vec()) + .ok_or_else(|| "Invalid response format".to_string()) } #[cfg(test)] From 4d066e47fdcd563a92115fee1ab7d35760e813a8 Mon Sep 17 00:00:00 2001 From: Shaun Arman Date: Sat, 20 Jun 2026 19:27:12 -0500 Subject: [PATCH 3/6] docs: update ticket to include VM listing and module-wide double-unwrap fixes --- TICKET-proxmox-reconnect.md | 90 +++++++++++++++++++++++++++++++++++++ 1 file changed, 90 insertions(+) create mode 100644 TICKET-proxmox-reconnect.md diff --git a/TICKET-proxmox-reconnect.md b/TICKET-proxmox-reconnect.md new file mode 100644 index 00000000..132867ac --- /dev/null +++ b/TICKET-proxmox-reconnect.md @@ -0,0 +1,90 @@ +# fix(proxmox): Reliable connect/reconnect after app restart + +## Description + +Adding a new Proxmox host reported "connected" immediately, but after closing +the app or clicking Disconnect the host could not be reconnected. + +Three compounding bugs caused this: + +1. **`authenticate()` could never succeed on reconnect** — `ProxmoxClient::authenticate()` + called `response.json::()` but the Proxmox API wraps every + response in `{"data": {...}}`. The deserialiser always failed with "missing + field `ticket`", so `get_proxmox_client_for_cluster` (and the new + `connect_proxmox_cluster`) threw an error on every app-restart reconnect. + +2. **False "connected" indicator on add** — `add_proxmox_cluster` inserted an + *unauthenticated* client (no ticket) into the in-memory pool. + `list_proxmox_clusters` reported `connected: true` just because the HashMap + key existed, even though any API call would have failed. + +3. **Double-unwrap of `data` in 10 commands** — `handle_response` already + strips the `{"data": ...}` envelope before returning to callers, but + `list_acls`, `list_users`, `get_cluster_notes`, `search_proxmox_resources`, + `get_node_status`, `get_syslog`, `list_network_interfaces`, + `get_subscription_status`, `list_cluster_tasks`, and + `list_proxmox_containers` all called `.get("data")` on the already-unwrapped + value, causing them to always return "Invalid response format". + +4. **VM list always empty** — `list_vms` in `vm.rs` used `POST` on + `cluster/resources` (a GET-only endpoint). The Proxmox API ignores the + POST body and the function also had the same double-unwrap bug, meaning + the resource list always came back empty. Additionally, VMs with no + `cpu` field (e.g. stopped VMs) were silently dropped by `filter_map` + using `?` — fixed to `unwrap_or(0.0)`. + +5. **Double-unwrap in all other proxmox modules** — the same `.get("data")` + double-unwrap was present across 18 module files (ceph, ceph_cluster, + certificates, acme, firewall, sdn, ha, apt, updates, updates_ext, tasks, + migration, metrics, shell, auth_realm, views, backup, vm). All 19 affected + functions fixed in a single follow-up commit. + +Additional gaps addressed: +- `CSRFPreventionToken` was never sent on POST/PUT/DELETE, so all mutating + operations (VM start/stop, firewall rules, etc.) would fail with + "CSRF check failed". +- Disconnect was UI-only with no backend call — the session stayed in the pool. +- `reqwest::Client` rejected self-signed Proxmox certificates. +- No `connect_proxmox_cluster` / `disconnect_proxmox_cluster` backend commands + existed; the Connect/Disconnect buttons in the Remotes page were wired to a + non-existent `ping_proxmox_cluster` or nothing at all. + +## Acceptance Criteria + +- [ ] Adding a new Proxmox host authenticates immediately; the UI shows + "connected" only when a real ticket has been obtained. +- [ ] Closing the app and re-opening it allows reconnecting via the Connect + button without requiring the host to be removed and re-added. +- [ ] Clicking Disconnect removes the session from the backend pool; subsequent + API calls fail with "Cluster not found" until Connect is clicked. +- [ ] All existing Rust tests (426) and frontend tests (386) continue to pass. +- [ ] `cargo clippy -- -D warnings` and `npx eslint . --max-warnings 0` report + zero issues. + +## Work Implemented + +| File | Change | +|------|--------| +| `src-tauri/src/proxmox/client.rs` | Added `ProxmoxEnvelope` wrapper; fixed `authenticate()` to use `&mut self`, parse the envelope, and store both ticket and CSRF token; added `set_csrf_token()`; `build_headers()` now takes `include_csrf` flag; POST/PUT/DELETE pass `true`; `danger_accept_invalid_certs(true)` on the reqwest client | +| `src-tauri/src/commands/proxmox.rs` | `add_proxmox_cluster` authenticates before inserting into pool; fixed double-unwrap in 10 commands; added `connect_proxmox_cluster` and `disconnect_proxmox_cluster` Tauri commands | +| `src-tauri/src/lib.rs` | Registered `connect_proxmox_cluster` and `disconnect_proxmox_cluster` | +| `src-tauri/src/cli/mod.rs` | `let mut client` — `authenticate` is now `&mut self` | +| `src/lib/proxmoxClient.ts` | Added `connectProxmoxCluster` and `disconnectProxmoxCluster` wrappers | +| `src/pages/Proxmox/RemotesPage.tsx` | Added `handleConnectRemote` / `handleDisconnectRemote` handlers calling real backend commands; wired them to `RemotesList` `onConnect` / `onDisconnect` props | +| `src-tauri/src/proxmox/vm.rs` | `list_vms`: changed from `client.post("cluster/resources", body)` to `client.get("cluster/resources?type=vm")`; removed double-unwrap; cpu uses `unwrap_or(0.0)`. `get_vm`: removed double-unwrap. `list_snapshots`: removed double-unwrap. | +| `src-tauri/src/proxmox/{acme,apt,auth_realm,backup,ceph,ceph_cluster,certificates,firewall,ha,metrics,migration,sdn,shell,tasks,updates,updates_ext,views}.rs` | Removed `.get("data")` double-unwrap from all 19 functions across 17 files. | + +New tests added: +- `client.rs` — envelope deserialization, no-CSRF path, `build_headers` GET omits / POST includes CSRF token, `set_ticket`/`set_csrf_token` +- `commands/proxmox.rs` — already-unwrapped array/object/notes response handling, `connect_proxmox_cluster` not-found error message + +## Testing Needed + +- [ ] Add a new Proxmox host with correct credentials → verify "connected" status +- [ ] Add a host with wrong password → verify immediate auth error, host not saved +- [ ] Restart the app → verify all previously-connected hosts show "disconnected" +- [ ] Click Connect on a disconnected host → verify it re-authenticates and shows "connected" +- [ ] Click Disconnect → verify status changes to "disconnected" and subsequent API calls fail +- [ ] Verify a VM start/stop operation succeeds (exercises CSRF token flow) +- [ ] Verify `get_node_status`, `list_acls`, `get_syslog` return real data (exercises double-unwrap fix) +- [ ] Verify a Proxmox host with a self-signed certificate connects successfully From 9b94e99e4d79fba5c306927f69333e49b1243eb3 Mon Sep 17 00:00:00 2001 From: Shaun Arman Date: Sat, 20 Jun 2026 19:49:42 -0500 Subject: [PATCH 4/6] ci(pr-review): increase LLM timeout to 600s, retry-max-time to 1200s qwen3.5-122b-think on a 253KB prompt was consistently hitting the 300s --max-time wall. --retry-max-time also equalled --max-time, so retries never fired. Now: 10 min per attempt, 2 retries, 20 min total budget. --- .gitea/workflows/pr-review.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.gitea/workflows/pr-review.yml b/.gitea/workflows/pr-review.yml index 7cc76c5a..69343692 100644 --- a/.gitea/workflows/pr-review.yml +++ b/.gitea/workflows/pr-review.yml @@ -290,8 +290,8 @@ jobs: '{model: $model, messages: [{role: "user", content: $content}], stream: false}' \ > /tmp/body.json echo "[$(date -u +%Y-%m-%dT%H:%M:%SZ)] PR #${PR_NUMBER} - Calling liteLLM API ($(wc -c < /tmp/body.json) bytes)..." - HTTP_CODE=$(curl -s --max-time 300 --connect-timeout 30 \ - --retry 3 --retry-delay 10 --retry-connrefused --retry-max-time 300 \ + HTTP_CODE=$(curl -s --max-time 600 --connect-timeout 30 \ + --retry 2 --retry-delay 15 --retry-connrefused --retry-max-time 1200 \ -o /tmp/llm_response.json -w "%{http_code}" \ -X POST "$LITELLM_URL/chat/completions" \ -H "Authorization: Bearer $LITELLM_API_KEY" \ From c8399bcdeba33d5d36e86909eb4b2dfae0c6bc0d Mon Sep 17 00:00:00 2001 From: Shaun Arman Date: Sat, 20 Jun 2026 19:54:00 -0500 Subject: [PATCH 5/6] ci(pr-review): iterative batch review for large diffs Replaces the single monolithic LLM call (which timed out on PRs >~150KB) with a batching strategy: - Changed files are grouped into batches capped at 2500 lines each, always respecting file boundaries (no file is split across batches) - Each batch is sent as an independent LLM call (max-time 600s) - Verdicts are aggregated: harshest verdict across all batches wins - Single-batch PRs produce identical output to the old workflow - Multi-batch PRs get a combined review with per-batch sections and an Overall Verdict line at the end - Partial failures are tolerated: if some batches succeed, the review is posted with a note about any failed batches - Cleanup removes /tmp/pr_batch_*.txt alongside existing temp files --- .gitea/workflows/pr-review.yml | 384 ++++++++++++++++++++------------- 1 file changed, 232 insertions(+), 152 deletions(-) diff --git a/.gitea/workflows/pr-review.yml b/.gitea/workflows/pr-review.yml index 69343692..19499bcf 100644 --- a/.gitea/workflows/pr-review.yml +++ b/.gitea/workflows/pr-review.yml @@ -31,7 +31,7 @@ jobs: git fetch --depth=1 origin ${{ github.head_ref }} git checkout FETCH_HEAD - - name: Build review context + - name: Build review batches id: context shell: bash run: | @@ -49,13 +49,10 @@ jobs: if [ "$FILE_COUNT" -eq 0 ]; then echo "No reviewable files changed." echo "diff_size=0" >> $GITHUB_OUTPUT + echo "batch_count=0" >> $GITHUB_OUTPUT exit 0 fi - # Build context: full file content for each changed file. - # Files <= 500 lines: include complete content. - # Files > 500 lines: include the per-file diff with generous context (±50 lines). - # # Secret scrubbing: match actual credential VALUES only — known API key formats, # or keyword="long_quoted_literal" (25+ chars). Never scrub on keyword alone, # which would silently delete function signatures, variable declarations, and tests. @@ -63,35 +60,53 @@ jobs: # Only strip lines that are ENTIRELY a long base64 blob (e.g. PEM cert bodies) B64_PATTERN='^[[:space:]]*[A-Za-z0-9+/]{60,}={0,2}[[:space:]]*$' - > /tmp/pr_context.txt + # Split changed files into batches capped at MAX_BATCH_LINES each. + # File boundaries are always respected — a file is never split across batches. + MAX_BATCH_LINES=2500 + BATCH=1 + BATCH_LINES=0 + TOTAL_LINES=0 + BATCH_FILE="/tmp/pr_batch_001.txt" + > "$BATCH_FILE" + while IFS= read -r file; do [ -f "$file" ] || continue - lines=$(wc -l < "$file" | tr -d ' ') - printf '\n════════ FILE: %s (%s lines) ════════\n' "$file" "$lines" >> /tmp/pr_context.txt - if [ "$lines" -le 500 ]; then - # Full file — model sees the complete implementation - grep -v -E "$SECRET_PATTERN" "$file" \ - | grep -v -E "$B64_PATTERN" \ - >> /tmp/pr_context.txt || true - else - # Large file — emit annotated diff hunks (±50 lines of context each) - printf '[File too large for full view (%s lines) — showing changed sections only]\n' "$lines" >> /tmp/pr_context.txt - git diff -U50 origin/${{ github.base_ref }}..HEAD -- "$file" \ - | grep -v -E "$SECRET_PATTERN" \ - | grep -v -E "$B64_PATTERN" \ - >> /tmp/pr_context.txt || true + file_lines=$(wc -l < "$file" | tr -d ' ') + + # Build context for this individual file into a temp file + { + printf '\n════════ FILE: %s (%s lines) ════════\n' "$file" "$file_lines" + if [ "$file_lines" -le 500 ]; then + grep -v -E "$SECRET_PATTERN" "$file" | grep -v -E "$B64_PATTERN" || true + else + printf '[File too large for full view (%s lines) — showing changed sections only]\n' "$file_lines" + git diff -U50 origin/${{ github.base_ref }}..HEAD -- "$file" \ + | grep -v -E "$SECRET_PATTERN" \ + | grep -v -E "$B64_PATTERN" \ + || true + fi + } > /tmp/fc_tmp.txt + + FC_LINES=$(wc -l < /tmp/fc_tmp.txt | tr -d ' ') + + # Start a new batch if this file would overflow the current one (and batch is not empty) + if [ "$BATCH_LINES" -gt 0 ] && [ $((BATCH_LINES + FC_LINES)) -gt $MAX_BATCH_LINES ]; then + BATCH=$((BATCH + 1)) + BATCH_LINES=0 + BATCH_FILE="/tmp/pr_batch_$(printf '%03d' $BATCH).txt" + > "$BATCH_FILE" fi + + cat /tmp/fc_tmp.txt >> "$BATCH_FILE" + BATCH_LINES=$((BATCH_LINES + FC_LINES)) + TOTAL_LINES=$((TOTAL_LINES + FC_LINES)) done < /tmp/pr_files.txt - TOTAL=$(wc -l < /tmp/pr_context.txt | tr -d ' ') - echo "diff_size=${TOTAL}" >> $GITHUB_OUTPUT + rm -f /tmp/fc_tmp.txt - # Cap at 6000 lines so we stay within the model's context window - if [ "$TOTAL" -gt 6000 ]; then - head -n 6000 /tmp/pr_context.txt > /tmp/pr_context_capped.txt - mv /tmp/pr_context_capped.txt /tmp/pr_context.txt - echo "[CONTEXT TRUNCATED at 6000 lines — ${TOTAL} total]" >> /tmp/pr_context.txt - fi + echo "batch_count=${BATCH}" >> $GITHUB_OUTPUT + echo "diff_size=${TOTAL_LINES}" >> $GITHUB_OUTPUT + echo "Built ${BATCH} batch(es) from ${FILE_COUNT} files (${TOTAL_LINES} total lines)" - name: Build codebase index id: index @@ -175,7 +190,7 @@ jobs: echo "comment_lines=${LINES}" >> $GITHUB_OUTPUT echo "Fetched PR history: ${LINES} lines" - - name: Analyze with LLM + - name: Analyze iteratively id: analyze if: steps.context.outputs.diff_size != '0' shell: bash @@ -185,137 +200,197 @@ jobs: PR_TITLE: ${{ github.event.pull_request.title }} PR_NUMBER: ${{ github.event.pull_request.number }} PR_BODY: ${{ github.event.pull_request.body }} + BATCH_COUNT: ${{ steps.context.outputs.batch_count }} run: | set -euo pipefail CHANGED_FILES=$(tr '\n' ' ' < /tmp/pr_files.txt) + > /tmp/pr_all_findings.txt + OVERALL_VERDICT="APPROVE" + BATCH_SUCCESS=0 + BATCH_FAILED=0 - # Build prompt file following anthropics/claude-code code-review pattern: - # - Multi-agent review (parallel analysis) - # - High-signal issues only (no nitpicks, style, or speculative concerns) - # - Validate findings against codebase - # - Consider PR title/description for author intent - # - Check for pre-existing issues - { - printf '%s\n\n' 'You are a senior engineer performing a code review following the anthropics/claude-code code-review pattern.' - printf 'PR Title: %s\n' "$PR_TITLE" - printf 'PR Body: %s\n\n' "${PR_BODY:-No description provided}" - printf 'Files changed: %s\n\n' "$CHANGED_FILES" - printf '%s\n' '---' - printf '%s\n\n' '## CODEBASE INDEX' - printf '%s\n' 'These are the ONLY Tauri commands, TypeScript exports, Rust public functions,' - printf '%s\n' 'and database tables that exist in this project. Before raising any finding,' - printf '%s\n' 'confirm that every symbol you cite appears in this list or in the file' - printf '%s\n' 'contents below. If it does not appear in either, your finding is fabricated.' - printf '%s\n' '---' - cat /tmp/codebase_index.txt - printf '%s\n\n' '---' - printf '%s\n\n' '## CHANGED FILE CONTENTS' - printf '%s\n' 'Each section is the COMPLETE, FINAL file after PR changes (not a diff).' - printf '%s\n\n' 'Files over 500 lines show only changed sections with surrounding context.' - printf '%s\n' '---' - cat /tmp/pr_context.txt - printf '%s\n\n' '---' - if [ -s /tmp/pr_comments.txt ]; then - cat /tmp/pr_comments.txt - printf '%s\n\n' '---' - printf '%s\n' '## CRITICAL: PRIOR REVIEW CONTEXT ABOVE' - printf '%s\n' 'Before raising ANY finding, check the review history above.' - printf '%s\n' 'SILENTLY DISCARD any finding that has already been:' - printf '%s\n' ' - Marked as invalid or incorrect by a reviewer' - printf '%s\n' ' - Acknowledged as an intentional design decision or known limitation' - printf '%s\n' ' - Confirmed fixed in a prior commit' - printf '%s\n\n' 'Raising a previously-refuted finding is a critical error.' + for i in $(seq 1 "$BATCH_COUNT"); do + BATCH_FILE="/tmp/pr_batch_$(printf '%03d' $i).txt" + [ -f "$BATCH_FILE" ] || continue + + # Build the prompt for this batch + { + printf '%s\n\n' 'You are a senior engineer performing a code review following the anthropics/claude-code code-review pattern.' + printf 'PR Title: %s\n' "$PR_TITLE" + printf 'PR Body: %s\n\n' "${PR_BODY:-No description provided}" + printf 'Files changed: %s\n\n' "$CHANGED_FILES" + if [ "$BATCH_COUNT" -gt 1 ]; then + printf 'NOTE: This is a large PR split into %s review batches. You are reviewing BATCH %s of %s.\n' "$BATCH_COUNT" "$i" "$BATCH_COUNT" + printf 'Focus ONLY on the files shown in this batch. Do not speculate about files not included here.\n\n' + fi printf '%s\n' '---' - fi - printf '%s\n\n' '## CODE REVIEW INSTRUCTIONS' - printf '%s\n\n' 'You MUST follow this workflow precisely:' - printf '%s\n\n' '1. LAUNCH 4 PARALLEL ANALYSIS AGENTS to independently review the changes:' - printf '%s\n\n' ' AGENT 1 (CLAUDE.MD COMPLIANCE): Audit changes for CLAUDE.md compliance' - printf '%s\n' ' - Only consider CLAUDE.md files that share a file path with the file or parents' - printf '%s\n' ' - Quote exact rules being violated' - printf '%s\n\n' ' AGENT 2 (CLAUDE.MD COMPLIANCE): Audit changes for CLAUDE.md compliance' - printf '%s\n' ' - Same scope as Agent 1, parallel analysis' - printf '%s\n\n' ' AGENT 3 (BUG DETECTOR): Scan for obvious bugs in the diff itself' - printf '%s\n' ' - Focus ONLY on the diff, no extra context' - printf '%s\n' ' - Flag ONLY significant bugs, ignore nitpicks and likely false positives' - printf '%s\n' ' - Do not flag issues that require context outside the git diff' - printf '%s\n\n' ' AGENT 4 (BUG DETECTOR): Look for problems in introduced code' - printf '%s\n' ' - Security issues, incorrect logic, data loss' - printf '%s\n' ' - Only problems that fall within the changed code' - printf '%s\n\n' '2. CRITICAL: Only flag HIGH SIGNAL issues where:' - printf '%s\n' ' - Code will fail to compile or parse (syntax errors, type errors)' - printf '%s\n' ' - Code will definitely produce wrong results (clear logic errors)' - printf '%s\n' ' - Clear, unambiguous violations with exact rule quoted' - printf '%s\n\n' ' DO NOT flag:' - printf '%s\n' ' - Code style or quality concerns' - printf '%s\n' ' - Potential issues that depend on specific inputs or state' - printf '%s\n' ' - Subjective suggestions or improvements' - printf '%s\n' ' - Pre-existing issues' - printf '%s\n' ' - Issues that linters will catch' - printf '%s\n' ' - General security issues unless explicitly required in CLAUDE.md' - printf '%s\n\n' '3. FOR EACH ISSUE FOUND BY AGENTS 3 & 4:' - printf '%s\n' ' - Launch a VALIDATION AGENT to verify the issue is real' - printf '%s\n' ' - Validation agent checks: issue is truly an issue, not false positive' - printf '%s\n' ' - Use full codebase to validate (not just diff)' - printf '%s\n' ' - If validation fails, discard the issue silently' - printf '%s\n\n' '4. OUTPUT FORMAT (strict):' - printf '%s\n\n' ' **Summary** (2-3 sentences)' - printf '%s\n' ' **Findings**' - printf '%s\n' ' - [SEVERITY] file:line - description' - printf '%s\n' ' Evidence: quoted line' - printf '%s\n\n' ' Fix: concrete change' - printf '%s\n\n' ' (Write "No findings." if none.)' - printf '%s\n' ' **Verdict**: APPROVE / APPROVE WITH COMMENTS / REQUEST CHANGES' - printf '%s\n\n' '5. SEVERITY DEFINITIONS:' - printf '%s\n' ' - BLOCKER: fails to compile, corrupts data, or security vulnerability' - printf '%s\n' ' - WARNING: real risk to address before merge' - printf '%s\n' ' - SUGGESTION: minor improvement, follow-up PR fine' - printf '%s\n\n' '6. FOCUS AREAS:' - printf '%s\n' ' - Security bugs, logic errors, data loss, injection, unhandled errors' - printf '%s\n\n' '7. IGNORE:' - printf '%s\n' ' - Style, missing comments, speculative future concerns' - printf '%s\n\n' '8. FALSE POSITIVES TO AVOID:' - printf '%s\n' ' - Pre-existing issues' - printf '%s\n' ' - Something that appears buggy but is actually correct' - printf '%s\n' ' - Pedantic nitpicks that senior engineers would not flag' - printf '%s\n' ' - Issues that linters will catch' - printf '%s\n' ' - General code quality concerns unless explicitly required in CLAUDE.md' - printf '%s\n' ' - Issues mentioned in CLAUDE.md but explicitly silenced in code' - } > /tmp/prompt.txt + cat /tmp/codebase_index.txt + printf '%s\n\n' '---' + printf '%s\n\n' '## CHANGED FILE CONTENTS (THIS BATCH)' + printf '%s\n' 'Each section is the COMPLETE, FINAL file after PR changes (not a diff).' + printf '%s\n\n' 'Files over 500 lines show only changed sections with surrounding context.' + printf '%s\n' '---' + cat "$BATCH_FILE" + printf '%s\n\n' '---' + if [ -s /tmp/pr_comments.txt ]; then + cat /tmp/pr_comments.txt + printf '%s\n\n' '---' + printf '%s\n' '## CRITICAL: PRIOR REVIEW CONTEXT ABOVE' + printf '%s\n' 'Before raising ANY finding, check the review history above.' + printf '%s\n' 'SILENTLY DISCARD any finding that has already been:' + printf '%s\n' ' - Marked as invalid or incorrect by a reviewer' + printf '%s\n' ' - Acknowledged as an intentional design decision or known limitation' + printf '%s\n' ' - Confirmed fixed in a prior commit' + printf '%s\n\n' 'Raising a previously-refuted finding is a critical error.' + printf '%s\n' '---' + fi + printf '%s\n\n' '## CODE REVIEW INSTRUCTIONS' + printf '%s\n\n' 'You MUST follow this workflow precisely:' + printf '%s\n\n' '1. LAUNCH 4 PARALLEL ANALYSIS AGENTS to independently review the changes:' + printf '%s\n\n' ' AGENT 1 (CLAUDE.MD COMPLIANCE): Audit changes for CLAUDE.md compliance' + printf '%s\n' ' - Only consider CLAUDE.md files that share a file path with the file or parents' + printf '%s\n' ' - Quote exact rules being violated' + printf '%s\n\n' ' AGENT 2 (CLAUDE.MD COMPLIANCE): Audit changes for CLAUDE.md compliance' + printf '%s\n' ' - Same scope as Agent 1, parallel analysis' + printf '%s\n\n' ' AGENT 3 (BUG DETECTOR): Scan for obvious bugs in the diff itself' + printf '%s\n' ' - Focus ONLY on the diff, no extra context' + printf '%s\n' ' - Flag ONLY significant bugs, ignore nitpicks and likely false positives' + printf '%s\n' ' - Do not flag issues that require context outside the git diff' + printf '%s\n\n' ' AGENT 4 (BUG DETECTOR): Look for problems in introduced code' + printf '%s\n' ' - Security issues, incorrect logic, data loss' + printf '%s\n' ' - Only problems that fall within the changed code' + printf '%s\n\n' '2. CRITICAL: Only flag HIGH SIGNAL issues where:' + printf '%s\n' ' - Code will fail to compile or parse (syntax errors, type errors)' + printf '%s\n' ' - Code will definitely produce wrong results (clear logic errors)' + printf '%s\n' ' - Clear, unambiguous violations with exact rule quoted' + printf '%s\n\n' ' DO NOT flag:' + printf '%s\n' ' - Code style or quality concerns' + printf '%s\n' ' - Potential issues that depend on specific inputs or state' + printf '%s\n' ' - Subjective suggestions or improvements' + printf '%s\n' ' - Pre-existing issues' + printf '%s\n' ' - Issues that linters will catch' + printf '%s\n' ' - General security issues unless explicitly required in CLAUDE.md' + printf '%s\n\n' '3. FOR EACH ISSUE FOUND BY AGENTS 3 & 4:' + printf '%s\n' ' - Launch a VALIDATION AGENT to verify the issue is real' + printf '%s\n' ' - Validation agent checks: issue is truly an issue, not false positive' + printf '%s\n' ' - Use full codebase to validate (not just diff)' + printf '%s\n' ' - If validation fails, discard the issue silently' + printf '%s\n\n' '4. OUTPUT FORMAT (strict):' + printf '%s\n\n' ' **Summary** (2-3 sentences)' + printf '%s\n' ' **Findings**' + printf '%s\n' ' - [SEVERITY] file:line - description' + printf '%s\n' ' Evidence: quoted line' + printf '%s\n\n' ' Fix: concrete change' + printf '%s\n\n' ' (Write "No findings." if none.)' + printf '%s\n' ' **Verdict**: APPROVE / APPROVE WITH COMMENTS / REQUEST CHANGES' + printf '%s\n\n' '5. SEVERITY DEFINITIONS:' + printf '%s\n' ' - BLOCKER: fails to compile, corrupts data, or security vulnerability' + printf '%s\n' ' - WARNING: real risk to address before merge' + printf '%s\n' ' - SUGGESTION: minor improvement, follow-up PR fine' + printf '%s\n\n' '6. FOCUS AREAS:' + printf '%s\n' ' - Security bugs, logic errors, data loss, injection, unhandled errors' + printf '%s\n\n' '7. IGNORE:' + printf '%s\n' ' - Style, missing comments, speculative future concerns' + printf '%s\n\n' '8. FALSE POSITIVES TO AVOID:' + printf '%s\n' ' - Pre-existing issues' + printf '%s\n' ' - Something that appears buggy but is actually correct' + printf '%s\n' ' - Pedantic nitpicks that senior engineers would not flag' + printf '%s\n' ' - Issues that linters will catch' + printf '%s\n' ' - General code quality concerns unless explicitly required in CLAUDE.md' + printf '%s\n' ' - Issues mentioned in CLAUDE.md but explicitly silenced in code' + } > /tmp/prompt_batch.txt - # Write body to file — passing 100KB+ JSON as a shell arg hits ARG_MAX. - jq -cn \ - --arg model "qwen3.5-122b-think" \ - --rawfile content /tmp/prompt.txt \ - '{model: $model, messages: [{role: "user", content: $content}], stream: false}' \ - > /tmp/body.json - echo "[$(date -u +%Y-%m-%dT%H:%M:%SZ)] PR #${PR_NUMBER} - Calling liteLLM API ($(wc -c < /tmp/body.json) bytes)..." - HTTP_CODE=$(curl -s --max-time 600 --connect-timeout 30 \ - --retry 2 --retry-delay 15 --retry-connrefused --retry-max-time 1200 \ - -o /tmp/llm_response.json -w "%{http_code}" \ - -X POST "$LITELLM_URL/chat/completions" \ - -H "Authorization: Bearer $LITELLM_API_KEY" \ - -H "Content-Type: application/json" \ - --data @/tmp/body.json) - echo "HTTP status: $HTTP_CODE" - echo "Response file size: $(wc -c < /tmp/llm_response.json) bytes" - if [ "$HTTP_CODE" != "200" ]; then - echo "ERROR: liteLLM returned HTTP $HTTP_CODE" - cat /tmp/llm_response.json + jq -cn \ + --arg model "qwen3.5-122b-think" \ + --rawfile content /tmp/prompt_batch.txt \ + '{model: $model, messages: [{role: "user", content: $content}], stream: false}' \ + > /tmp/body.json + + echo "[$(date -u +%Y-%m-%dT%H:%M:%SZ)] PR #${PR_NUMBER} - Batch ${i}/${BATCH_COUNT} ($(wc -c < /tmp/body.json) bytes)..." + + HTTP_CODE=$(curl -s --max-time 600 --connect-timeout 30 \ + --retry 2 --retry-delay 15 --retry-connrefused --retry-max-time 1200 \ + -o /tmp/llm_response.json -w "%{http_code}" \ + -X POST "$LITELLM_URL/chat/completions" \ + -H "Authorization: Bearer $LITELLM_API_KEY" \ + -H "Content-Type: application/json" \ + --data @/tmp/body.json) + + echo "Batch ${i} HTTP status: $HTTP_CODE" + echo "Batch ${i} response size: $(wc -c < /tmp/llm_response.json) bytes" + + if [ "$HTTP_CODE" != "200" ]; then + echo "ERROR: Batch ${i} failed (HTTP $HTTP_CODE)" + cat /tmp/llm_response.json + { + echo "## Batch ${i} of ${BATCH_COUNT}" + echo "" + echo "Review unavailable — LLM returned HTTP ${HTTP_CODE}." + echo "" + echo "---" + echo "" + } >> /tmp/pr_all_findings.txt + BATCH_FAILED=$((BATCH_FAILED + 1)) + continue + fi + + if ! jq empty /tmp/llm_response.json 2>/dev/null; then + echo "ERROR: Invalid JSON in batch ${i} response" + BATCH_FAILED=$((BATCH_FAILED + 1)) + continue + fi + + BATCH_REVIEW=$(jq -r '.choices[0].message.content // empty' /tmp/llm_response.json) + if [ -z "$BATCH_REVIEW" ]; then + echo "ERROR: Empty content in batch ${i} response" + BATCH_FAILED=$((BATCH_FAILED + 1)) + continue + fi + + echo "Batch ${i} review length: ${#BATCH_REVIEW} chars" + BATCH_SUCCESS=$((BATCH_SUCCESS + 1)) + + # Track harshest verdict across batches + if echo "$BATCH_REVIEW" | grep -q "REQUEST CHANGES"; then + OVERALL_VERDICT="REQUEST CHANGES" + elif echo "$BATCH_REVIEW" | grep -q "APPROVE WITH COMMENTS" && [ "$OVERALL_VERDICT" != "REQUEST CHANGES" ]; then + OVERALL_VERDICT="APPROVE WITH COMMENTS" + fi + + if [ "$BATCH_COUNT" -eq 1 ]; then + echo "$BATCH_REVIEW" >> /tmp/pr_all_findings.txt + else + { + echo "## Batch ${i} of ${BATCH_COUNT}" + echo "" + echo "$BATCH_REVIEW" + echo "" + echo "---" + echo "" + } >> /tmp/pr_all_findings.txt + fi + done + + if [ "$BATCH_SUCCESS" -eq 0 ]; then + echo "ERROR: All ${BATCH_COUNT} batches failed" exit 1 fi - if ! jq empty /tmp/llm_response.json 2>/dev/null; then - echo "ERROR: Invalid JSON response from liteLLM" - cat /tmp/llm_response.json - exit 1 + + # Assemble final review + if [ "$BATCH_COUNT" -eq 1 ]; then + cp /tmp/pr_all_findings.txt /tmp/pr_review.txt + else + { + echo "_This PR was reviewed in ${BATCH_COUNT} batches (${BATCH_FAILED} batch(es) failed)._" + echo "" + cat /tmp/pr_all_findings.txt + echo "---" + echo "" + echo "**Overall Verdict**: ${OVERALL_VERDICT}" + echo "" + echo "_Overall verdict reflects the most critical finding across all batches._" + } > /tmp/pr_review.txt fi - REVIEW=$(jq -r '.choices[0].message.content // empty' /tmp/llm_response.json) - if [ -z "$REVIEW" ]; then - echo "ERROR: No content in liteLLM response" - exit 1 - fi - echo "Review length: ${#REVIEW} chars" - echo "$REVIEW" > /tmp/pr_review.txt - name: Verify findings against codebase if: steps.analyze.outcome == 'success' @@ -424,4 +499,9 @@ jobs: - name: Cleanup if: always() shell: bash - run: rm -f /tmp/pr_diff.txt /tmp/pr_context.txt /tmp/codebase_index.txt /tmp/pr_comments.txt /tmp/prompt.txt /tmp/body.json /tmp/llm_response.json /tmp/pr_review.txt /tmp/review_post_response.json /tmp/pr_files.txt + run: | + rm -f /tmp/pr_files.txt /tmp/pr_context.txt /tmp/codebase_index.txt \ + /tmp/pr_comments.txt /tmp/prompt_batch.txt /tmp/body.json \ + /tmp/llm_response.json /tmp/pr_review.txt /tmp/pr_all_findings.txt \ + /tmp/review_post_response.json /tmp/fc_tmp.txt + rm -f /tmp/pr_batch_*.txt From c1885ffc2ba219db639e1dd7d186884e6f7f4fdf Mon Sep 17 00:00:00 2001 From: Shaun Arman Date: Sat, 20 Jun 2026 20:13:58 -0500 Subject: [PATCH 6/6] style: apply rustfmt to client.rs and vm.rs --- src-tauri/src/proxmox/client.rs | 12 ++++++++++-- src-tauri/src/proxmox/vm.rs | 16 +++++++++++++--- 2 files changed, 23 insertions(+), 5 deletions(-) diff --git a/src-tauri/src/proxmox/client.rs b/src-tauri/src/proxmox/client.rs index fa98606a..6f569f91 100644 --- a/src-tauri/src/proxmox/client.rs +++ b/src-tauri/src/proxmox/client.rs @@ -136,7 +136,11 @@ impl ProxmoxClient { /// Build request headers with authentication. /// `include_csrf` should be true for POST / PUT / DELETE requests. - fn build_headers(&self, ticket: Option<&str>, include_csrf: bool) -> reqwest::header::HeaderMap { + fn build_headers( + &self, + ticket: Option<&str>, + include_csrf: bool, + ) -> reqwest::header::HeaderMap { let mut headers = reqwest::header::HeaderMap::new(); if let Some(token) = &self.api_token { @@ -395,7 +399,11 @@ mod tests { let headers = client.build_headers(Some("my-ticket"), true); assert!(headers.contains_key("CSRFPreventionToken")); - let csrf_val = headers.get("CSRFPreventionToken").unwrap().to_str().unwrap(); + let csrf_val = headers + .get("CSRFPreventionToken") + .unwrap() + .to_str() + .unwrap(); assert_eq!(csrf_val, "my-csrf"); } diff --git a/src-tauri/src/proxmox/vm.rs b/src-tauri/src/proxmox/vm.rs index 872d26b9..f08c19cd 100644 --- a/src-tauri/src/proxmox/vm.rs +++ b/src-tauri/src/proxmox/vm.rs @@ -151,8 +151,15 @@ pub async fn list_vms( if resource_type != "qemu" { return None; } - let name = r.get("name").and_then(|n| n.as_str()).map(|s| s.to_string()); - let status = r.get("status").and_then(|s| s.as_str()).unwrap_or("unknown").to_string(); + let name = r + .get("name") + .and_then(|n| n.as_str()) + .map(|s| s.to_string()); + let status = r + .get("status") + .and_then(|s| s.as_str()) + .unwrap_or("unknown") + .to_string(); // cpu may be absent for stopped VMs let cpu = r.get("cpu").and_then(|c| c.as_f64()).unwrap_or(0.0); @@ -166,7 +173,10 @@ pub async fn list_vms( uptime: r.get("uptime").and_then(|u| u.as_u64()).unwrap_or(0), node, template: r.get("template").and_then(|t| t.as_bool()), - agent: r.get("agent").and_then(|a| a.as_str()).map(|s| s.to_string()), + agent: r + .get("agent") + .and_then(|a| a.as_str()) + .map(|s| s.to_string()), mem: r.get("mem").and_then(|m| m.as_u64()), max_mem: r.get("maxmem").and_then(|m| m.as_u64()), max_disk: r.get("maxdisk").and_then(|d| d.as_u64()),