Merge pull request #78 from saagpatel/codex/refactor/app-error-small-files

refactor(src): migrate 3 small command files to AppError

Author: saagpatel

src-tauri/src/commands/app_core_commands.rs

@@ -1,37 +1,42 @@
+use crate::error::AppError;
 use crate::AppState;
 use tauri::State;
 
 #[tauri::command]
-pub fn check_fts5_enabled(state: State<'_, AppState>) -> Result<bool, String> {
-    let db_lock = state.db.lock().map_err(|e| e.to_string())?;
-    let db = db_lock.as_ref().ok_or("Database not initialized")?;
-    db.verify_fts5().map_err(|e| e.to_string())
+pub fn check_fts5_enabled(state: State<'_, AppState>) -> Result<bool, AppError> {
+    let db_lock = state.db.lock().map_err(|_| AppError::db_lock_failed())?;
+    let db = db_lock.as_ref().ok_or_else(AppError::db_not_initialized)?;
+    db.verify_fts5()
+        .map_err(|e| AppError::db_query_failed(e.to_string()))
 }
 
 #[tauri::command]
-pub fn check_db_integrity(state: State<'_, AppState>) -> Result<bool, String> {
-    let db_lock = state.db.lock().map_err(|e| e.to_string())?;
-    let db = db_lock.as_ref().ok_or("Database not initialized")?;
+pub fn check_db_integrity(state: State<'_, AppState>) -> Result<bool, AppError> {
+    let db_lock = state.db.lock().map_err(|_| AppError::db_lock_failed())?;
+    let db = db_lock.as_ref().ok_or_else(AppError::db_not_initialized)?;
     db.check_integrity()
         .map(|_| true)
-        .map_err(|e| e.to_string())
+        .map_err(|e| AppError::db_query_failed(e.to_string()))
 }
 
 #[tauri::command]
-pub fn get_vector_consent(state: State<'_, AppState>) -> Result<crate::db::VectorConsent, String> {
-    let db_lock = state.db.lock().map_err(|e| e.to_string())?;
-    let db = db_lock.as_ref().ok_or("Database not initialized")?;
-    db.get_vector_consent().map_err(|e| e.to_string())
+pub fn get_vector_consent(
+    state: State<'_, AppState>,
+) -> Result<crate::db::VectorConsent, AppError> {
+    let db_lock = state.db.lock().map_err(|_| AppError::db_lock_failed())?;
+    let db = db_lock.as_ref().ok_or_else(AppError::db_not_initialized)?;
+    db.get_vector_consent()
+        .map_err(|e| AppError::db_query_failed(e.to_string()))
 }
 
 #[tauri::command]
 pub fn set_vector_consent(
     state: State<'_, AppState>,
     enabled: bool,
     encryption_supported: bool,
-) -> Result<(), String> {
-    let db_lock = state.db.lock().map_err(|e| e.to_string())?;
-    let db = db_lock.as_ref().ok_or("Database not initialized")?;
+) -> Result<(), AppError> {
+    let db_lock = state.db.lock().map_err(|_| AppError::db_lock_failed())?;
+    let db = db_lock.as_ref().ok_or_else(AppError::db_not_initialized)?;
     db.set_vector_consent(enabled, encryption_supported)
-        .map_err(|e| e.to_string())
+        .map_err(|e| AppError::db_query_failed(e.to_string()))
 }

src-tauri/src/commands/decision_tree_runtime.rs

@@ -1,20 +1,23 @@
 use crate::commands::model_commands::DecisionTree;
+use crate::error::AppError;
 use crate::AppState;
 use tauri::State;
 
 pub(crate) fn list_decision_trees_impl(
     state: State<'_, AppState>,
-) -> Result<Vec<DecisionTree>, String> {
-    let db_lock = state.db.lock().map_err(|e| e.to_string())?;
-    let db = db_lock.as_ref().ok_or("Database not initialized")?;
-    db.list_decision_trees().map_err(|e| e.to_string())
+) -> Result<Vec<DecisionTree>, AppError> {
+    let db_lock = state.db.lock().map_err(|_| AppError::db_lock_failed())?;
+    let db = db_lock.as_ref().ok_or_else(AppError::db_not_initialized)?;
+    db.list_decision_trees()
+        .map_err(|e| AppError::db_query_failed(e.to_string()))
 }
 
 pub(crate) fn get_decision_tree_impl(
     state: State<'_, AppState>,
     tree_id: String,
-) -> Result<DecisionTree, String> {
-    let db_lock = state.db.lock().map_err(|e| e.to_string())?;
-    let db = db_lock.as_ref().ok_or("Database not initialized")?;
-    db.get_decision_tree(&tree_id).map_err(|e| e.to_string())
+) -> Result<DecisionTree, AppError> {
+    let db_lock = state.db.lock().map_err(|_| AppError::db_lock_failed())?;
+    let db = db_lock.as_ref().ok_or_else(AppError::db_not_initialized)?;
+    db.get_decision_tree(&tree_id)
+        .map_err(|e| AppError::db_query_failed(e.to_string()))
 }

src-tauri/src/commands/model_commands.rs

@@ -379,15 +379,17 @@ pub fn is_ocr_available() -> bool {
 }
 
 #[tauri::command]
-pub fn list_decision_trees(state: State<'_, AppState>) -> Result<Vec<DecisionTree>, String> {
+pub fn list_decision_trees(
+    state: State<'_, AppState>,
+) -> Result<Vec<DecisionTree>, crate::error::AppError> {
     list_decision_trees_impl(state)
 }
 
 #[tauri::command]
 pub fn get_decision_tree(
     state: State<'_, AppState>,
     tree_id: String,
-) -> Result<DecisionTree, String> {
+) -> Result<DecisionTree, crate::error::AppError> {
     get_decision_tree_impl(state, tree_id)
 }
 

src-tauri/src/commands/pilot_feedback.rs

@@ -1,3 +1,4 @@
+use crate::error::{AppError, ErrorCategory, ErrorCode};
 use crate::validation::{validate_output_file_within_home, ValidationError};
 use crate::AppState;
 use std::path::Path;
@@ -26,17 +27,34 @@ fn pilot_logging_enabled() -> bool {
     parse_bool_env(PILOT_LOGGING_POLICY_ENV, false)
 }
 
-fn require_pilot_logging_enabled() -> Result<(), String> {
+fn require_pilot_logging_enabled() -> Result<(), AppError> {
     if pilot_logging_enabled() {
         Ok(())
     } else {
-        Err(format!(
-            "Pilot logging is disabled by policy. Set {}=1 and restart AssistSupport to enable.",
-            PILOT_LOGGING_POLICY_ENV
+        Err(AppError::new(
+            ErrorCode::SECURITY_PERMISSION_DENIED,
+            format!(
+                "Pilot logging is disabled by policy. Set {}=1 and restart AssistSupport to enable.",
+                PILOT_LOGGING_POLICY_ENV
+            ),
+            ErrorCategory::Security,
         ))
     }
 }
 
+/// Map a `crate::feedback` string error to a properly-categorized AppError.
+///
+/// The feedback module returns stringly-typed errors today; until it migrates
+/// we categorize as Database (its real source) with the message as detail.
+fn feedback_err(op: &str, e: impl std::fmt::Display) -> AppError {
+    AppError::new(
+        ErrorCode::DB_QUERY_FAILED,
+        format!("Pilot {} failed", op),
+        ErrorCategory::Database,
+    )
+    .with_detail(e.to_string())
+}
+
 #[derive(serde::Serialize, Clone)]
 pub struct PilotLoggingPolicy {
     pub enabled: bool,
@@ -60,11 +78,12 @@ pub fn log_pilot_query(
     query: String,
     response: String,
     operator_id: String,
-) -> Result<String, String> {
+) -> Result<String, AppError> {
     require_pilot_logging_enabled()?;
-    let db_lock = state.db.lock().map_err(|e| e.to_string())?;
-    let db = db_lock.as_ref().ok_or("Database not initialized")?;
+    let db_lock = state.db.lock().map_err(|_| AppError::db_lock_failed())?;
+    let db = db_lock.as_ref().ok_or_else(AppError::db_not_initialized)?;
     crate::feedback::log_query(db, &query, &response, &operator_id)
+        .map_err(|e| feedback_err("query log", e))
 }
 
 /// Submit user feedback on a pilot query response
@@ -77,10 +96,10 @@ pub fn submit_pilot_feedback(
     clarity: i32,
     helpfulness: i32,
     comment: Option<String>,
-) -> Result<String, String> {
+) -> Result<String, AppError> {
     require_pilot_logging_enabled()?;
-    let db_lock = state.db.lock().map_err(|e| e.to_string())?;
-    let db = db_lock.as_ref().ok_or("Database not initialized")?;
+    let db_lock = state.db.lock().map_err(|_| AppError::db_lock_failed())?;
+    let db = db_lock.as_ref().ok_or_else(AppError::db_not_initialized)?;
     crate::feedback::submit_feedback(
         db,
         &query_log_id,
@@ -90,31 +109,34 @@ pub fn submit_pilot_feedback(
         helpfulness,
         comment.as_deref(),
     )
+    .map_err(|e| feedback_err("feedback submission", e))
 }
 
 /// Get pilot dashboard summary stats
 #[tauri::command]
-pub fn get_pilot_stats(state: State<'_, AppState>) -> Result<crate::feedback::PilotStats, String> {
+pub fn get_pilot_stats(
+    state: State<'_, AppState>,
+) -> Result<crate::feedback::PilotStats, AppError> {
     require_pilot_logging_enabled()?;
-    let db_lock = state.db.lock().map_err(|e| e.to_string())?;
-    let db = db_lock.as_ref().ok_or("Database not initialized")?;
-    crate::feedback::get_pilot_stats(db)
+    let db_lock = state.db.lock().map_err(|_| AppError::db_lock_failed())?;
+    let db = db_lock.as_ref().ok_or_else(AppError::db_not_initialized)?;
+    crate::feedback::get_pilot_stats(db).map_err(|e| feedback_err("stats read", e))
 }
 
 /// Get all pilot query logs
 #[tauri::command]
 pub fn get_pilot_query_logs(
     state: State<'_, AppState>,
-) -> Result<Vec<crate::feedback::QueryLog>, String> {
+) -> Result<Vec<crate::feedback::QueryLog>, AppError> {
     require_pilot_logging_enabled()?;
-    let db_lock = state.db.lock().map_err(|e| e.to_string())?;
-    let db = db_lock.as_ref().ok_or("Database not initialized")?;
-    crate::feedback::get_query_logs(db)
+    let db_lock = state.db.lock().map_err(|_| AppError::db_lock_failed())?;
+    let db = db_lock.as_ref().ok_or_else(AppError::db_not_initialized)?;
+    crate::feedback::get_query_logs(db).map_err(|e| feedback_err("query logs read", e))
 }
 
 /// Export pilot data to CSV
 #[tauri::command]
-pub fn export_pilot_data(state: State<'_, AppState>, path: String) -> Result<usize, String> {
+pub fn export_pilot_data(state: State<'_, AppState>, path: String) -> Result<usize, AppError> {
     require_pilot_logging_enabled()?;
 
     let candidate = Path::new(&path);
@@ -124,19 +146,26 @@ pub fn export_pilot_data(state: State<'_, AppState>, path: String) -> Result<usi
         .unwrap_or("")
         .to_ascii_lowercase();
     if ext != "csv" {
-        return Err("Export path must be a .csv file".into());
+        return Err(AppError::invalid_format("Export path must be a .csv file"));
     }
 
     let validated_path = validate_output_file_within_home(candidate).map_err(|e| match e {
-        ValidationError::PathTraversal => "Export path must be within your home directory".into(),
-        ValidationError::PathNotFound(_) => "Export parent directory does not exist".into(),
+        ValidationError::PathTraversal => AppError::new(
+            ErrorCode::VALIDATION_PATH_TRAVERSAL,
+            "Export path must be within your home directory",
+            ErrorCategory::Validation,
+        ),
+        ValidationError::PathNotFound(_) => {
+            AppError::invalid_path("Export parent directory does not exist")
+        }
         ValidationError::InvalidFormat(msg) if msg.contains("sensitive") => {
-            "This export path is blocked because it contains sensitive data".into()
+            AppError::sensitive_path()
         }
-        _ => format!("Invalid export path: {}", e),
+        other => AppError::invalid_path(format!("Invalid export path: {}", other)),
     })?;
 
-    let db_lock = state.db.lock().map_err(|e| e.to_string())?;
-    let db = db_lock.as_ref().ok_or("Database not initialized")?;
+    let db_lock = state.db.lock().map_err(|_| AppError::db_lock_failed())?;
+    let db = db_lock.as_ref().ok_or_else(AppError::db_not_initialized)?;
     crate::feedback::export::export_to_csv(db, validated_path.as_path())
+        .map_err(|e| feedback_err("data export", e))
 }