From 0fdcc496bee5711bf17e07d46bc4e01c4d72f833 Mon Sep 17 00:00:00 2001 From: jgrusewski Date: Tue, 3 Mar 2026 22:29:13 +0100 Subject: [PATCH] fix: type-safe proto enums, MCP protocol compliance, TUI teardown - Replace hardcoded i32 enum matches with ProtoEnum::try_from() in 8 command files (cluster, config, data, model, risk, service, train, tune) - Replace hardcoded i32 literals with enum variants (EmergencyStopType, ExportFormat, TrainingMode) - Add JSON-RPC 2.0 PARSE_ERROR (-32700) for malformed JSON input - Validate jsonrpc:"2.0" version on incoming MCP requests - Change unreachable execute_tool catch-all from Ok to Err - Fix TUI teardown to not mask original event_loop error - Rename config_cmd module to config (commands::config) - 77 tests pass, 0 clippy warnings Co-Authored-By: Claude Opus 4.6 --- bin/fxt/src/commands/cluster.rs | 35 ++++++----- .../src/commands/{config_cmd.rs => config.rs} | 30 +++++---- bin/fxt/src/commands/data.rs | 18 +++--- bin/fxt/src/commands/mod.rs | 2 +- bin/fxt/src/commands/model.rs | 56 +++++++++-------- bin/fxt/src/commands/risk.rs | 13 ++-- bin/fxt/src/commands/service.rs | 61 ++++++++++--------- bin/fxt/src/commands/train.rs | 32 +++++----- bin/fxt/src/commands/tune.rs | 24 ++++---- bin/fxt/src/main.rs | 4 +- bin/fxt/src/mcp/protocol.rs | 3 + bin/fxt/src/mcp/server.rs | 43 +++++++++++-- bin/fxt/src/tui/event_loop.rs | 4 +- 13 files changed, 193 insertions(+), 132 deletions(-) rename bin/fxt/src/commands/{config_cmd.rs => config.rs} (90%) diff --git a/bin/fxt/src/commands/cluster.rs b/bin/fxt/src/commands/cluster.rs index 6e148fbdf..1d2bf263c 100644 --- a/bin/fxt/src/commands/cluster.rs +++ b/bin/fxt/src/commands/cluster.rs @@ -155,32 +155,35 @@ impl HumanReadable for EventsResult { // ── Helpers ─────────────────────────────────────────────────────────── fn system_health_name(value: i32) -> &'static str { - match value { - 1 => "HEALTHY", - 2 => "DEGRADED", - 3 => "UNHEALTHY", - 4 => "CRITICAL", + use crate::proto::monitoring::SystemHealth; + match SystemHealth::try_from(value) { + Ok(SystemHealth::Healthy) => "HEALTHY", + Ok(SystemHealth::Degraded) => "DEGRADED", + Ok(SystemHealth::Unhealthy) => "UNHEALTHY", + Ok(SystemHealth::Critical) => "CRITICAL", _ => "UNKNOWN", } } fn service_health_name(value: i32) -> &'static str { - match value { - 1 => "HEALTHY", - 2 => "DEGRADED", - 3 => "UNHEALTHY", - 4 => "CRITICAL", + use crate::proto::monitoring::ServiceHealth; + match ServiceHealth::try_from(value) { + Ok(ServiceHealth::Healthy) => "HEALTHY", + Ok(ServiceHealth::Degraded) => "DEGRADED", + Ok(ServiceHealth::Unhealthy) => "UNHEALTHY", + Ok(ServiceHealth::Critical) => "CRITICAL", _ => "UNKNOWN", } } fn service_state_name(value: i32) -> &'static str { - match value { - 1 => "starting", - 2 => "running", - 3 => "stopping", - 4 => "stopped", - 5 => "error", + use crate::proto::monitoring::ServiceState; + match ServiceState::try_from(value) { + Ok(ServiceState::Starting) => "starting", + Ok(ServiceState::Running) => "running", + Ok(ServiceState::Stopping) => "stopping", + Ok(ServiceState::Stopped) => "stopped", + Ok(ServiceState::Error) => "error", _ => "unknown", } } diff --git a/bin/fxt/src/commands/config_cmd.rs b/bin/fxt/src/commands/config.rs similarity index 90% rename from bin/fxt/src/commands/config_cmd.rs rename to bin/fxt/src/commands/config.rs index 61d78e472..756aa5f1c 100644 --- a/bin/fxt/src/commands/config_cmd.rs +++ b/bin/fxt/src/commands/config.rs @@ -140,12 +140,13 @@ impl HumanReadable for EnvResult { // ── Helpers ─────────────────────────────────────────────────────────── fn config_data_type_name(value: i32) -> &'static str { - match value { - 1 => "string", - 2 => "number", - 3 => "boolean", - 4 => "json", - 5 => "encrypted", + use crate::proto::config::ConfigDataType; + match ConfigDataType::try_from(value) { + Ok(ConfigDataType::String) => "string", + Ok(ConfigDataType::Number) => "number", + Ok(ConfigDataType::Boolean) => "boolean", + Ok(ConfigDataType::Json) => "json", + Ok(ConfigDataType::Encrypted) => "encrypted", _ => "unspecified", } } @@ -224,17 +225,20 @@ impl ConfigCommand { .export_configuration(crate::proto::config::ExportConfigurationRequest { categories, environment: None, - format: 3, // TOML + format: crate::proto::config::ExportFormat::Toml as i32, }) .await? .into_inner(); - let format_name = match resp.format { - 1 => "json", - 2 => "yaml", - 3 => "toml", - 4 => "env", - _ => "unknown", + let format_name = { + use crate::proto::config::ExportFormat; + match ExportFormat::try_from(resp.format) { + Ok(ExportFormat::Json) => "json", + Ok(ExportFormat::Yaml) => "yaml", + Ok(ExportFormat::Toml) => "toml", + Ok(ExportFormat::Env) => "env", + _ => "unknown", + } }; let result = ExportConfigResult { diff --git a/bin/fxt/src/commands/data.rs b/bin/fxt/src/commands/data.rs index eb867172e..773c0cf67 100644 --- a/bin/fxt/src/commands/data.rs +++ b/bin/fxt/src/commands/data.rs @@ -146,15 +146,15 @@ impl HumanReadable for FeedsResult { // ── Helpers ─────────────────────────────────────────────────────────── fn download_status_name(value: i32) -> &'static str { - match value { - 0 => "UNKNOWN", - 1 => "PENDING", - 2 => "DOWNLOADING", - 3 => "VALIDATING", - 4 => "UPLOADING", - 5 => "COMPLETED", - 6 => "FAILED", - 7 => "CANCELLED", + use crate::proto::data_acquisition::DownloadStatus; + match DownloadStatus::try_from(value) { + Ok(DownloadStatus::Pending) => "PENDING", + Ok(DownloadStatus::Downloading) => "DOWNLOADING", + Ok(DownloadStatus::Validating) => "VALIDATING", + Ok(DownloadStatus::Uploading) => "UPLOADING", + Ok(DownloadStatus::Completed) => "COMPLETED", + Ok(DownloadStatus::Failed) => "FAILED", + Ok(DownloadStatus::Cancelled) => "CANCELLED", _ => "UNKNOWN", } } diff --git a/bin/fxt/src/commands/mod.rs b/bin/fxt/src/commands/mod.rs index a993c32e9..3f2dfa528 100644 --- a/bin/fxt/src/commands/mod.rs +++ b/bin/fxt/src/commands/mod.rs @@ -5,7 +5,7 @@ pub mod auth; pub mod backtest; pub mod broker; pub mod cluster; -pub mod config_cmd; +pub mod config; pub mod data; pub mod mcp; pub mod model; diff --git a/bin/fxt/src/commands/model.rs b/bin/fxt/src/commands/model.rs index 6670d93b9..f01750a7b 100644 --- a/bin/fxt/src/commands/model.rs +++ b/bin/fxt/src/commands/model.rs @@ -121,47 +121,51 @@ struct StubMessage { // ── Display helpers ────────────────────────────────────────────────── fn model_state_str(s: i32) -> &'static str { - match s { - 1 => "loading", - 2 => "ready", - 3 => "predicting", - 4 => "training", - 5 => "error", - 6 => "offline", + use crate::proto::ml::ModelState; + match ModelState::try_from(s) { + Ok(ModelState::Loading) => "loading", + Ok(ModelState::Ready) => "ready", + Ok(ModelState::Predicting) => "predicting", + Ok(ModelState::Training) => "training", + Ok(ModelState::Error) => "error", + Ok(ModelState::Offline) => "offline", _ => "unknown", } } fn model_health_str(h: i32) -> &'static str { - match h { - 1 => "healthy", - 2 => "degraded", - 3 => "unhealthy", - 4 => "critical", + use crate::proto::ml::ModelHealth; + match ModelHealth::try_from(h) { + Ok(ModelHealth::Healthy) => "healthy", + Ok(ModelHealth::Degraded) => "degraded", + Ok(ModelHealth::Unhealthy) => "unhealthy", + Ok(ModelHealth::Critical) => "critical", _ => "unknown", } } fn prediction_type_str(p: i32) -> &'static str { - match p { - 1 => "BUY", - 2 => "SELL", - 3 => "HOLD", - 4 => "PRICE_UP", - 5 => "PRICE_DOWN", - 6 => "VOL_HIGH", - 7 => "VOL_LOW", + use crate::proto::ml::PredictionType; + match PredictionType::try_from(p) { + Ok(PredictionType::Buy) => "BUY", + Ok(PredictionType::Sell) => "SELL", + Ok(PredictionType::Hold) => "HOLD", + Ok(PredictionType::PriceUp) => "PRICE_UP", + Ok(PredictionType::PriceDown) => "PRICE_DOWN", + Ok(PredictionType::VolatilityHigh) => "VOL_HIGH", + Ok(PredictionType::VolatilityLow) => "VOL_LOW", _ => "UNKNOWN", } } fn signal_strength_str(s: i32) -> &'static str { - match s { - 1 => "very_weak", - 2 => "weak", - 3 => "moderate", - 4 => "strong", - 5 => "very_strong", + use crate::proto::ml::SignalStrength; + match SignalStrength::try_from(s) { + Ok(SignalStrength::VeryWeak) => "very_weak", + Ok(SignalStrength::Weak) => "weak", + Ok(SignalStrength::Moderate) => "moderate", + Ok(SignalStrength::Strong) => "strong", + Ok(SignalStrength::VeryStrong) => "very_strong", _ => "unknown", } } diff --git a/bin/fxt/src/commands/risk.rs b/bin/fxt/src/commands/risk.rs index 5b6ccb288..e24ca287c 100644 --- a/bin/fxt/src/commands/risk.rs +++ b/bin/fxt/src/commands/risk.rs @@ -232,11 +232,12 @@ impl HumanReadable for EmergencyResult { // ── Helpers ─────────────────────────────────────────────────────────── fn breaker_type_name(value: i32) -> &'static str { - match value { - 1 => "portfolio-loss", - 2 => "symbol-volatility", - 3 => "position-size", - 4 => "drawdown", + use crate::proto::risk::CircuitBreakerType; + match CircuitBreakerType::try_from(value) { + Ok(CircuitBreakerType::PortfolioLoss) => "portfolio-loss", + Ok(CircuitBreakerType::SymbolVolatility) => "symbol-volatility", + Ok(CircuitBreakerType::PositionSize) => "position-size", + Ok(CircuitBreakerType::Drawdown) => "drawdown", _ => "unknown", } } @@ -392,7 +393,7 @@ impl RiskCommand { let resp = client .risk() .emergency_stop(crate::proto::risk::EmergencyStopRequest { - stop_type: 1, // ALL_TRADING + stop_type: crate::proto::risk::EmergencyStopType::AllTrading as i32, reason: "Manual emergency halt via fxt CLI".to_owned(), symbol: None, account_id: None, diff --git a/bin/fxt/src/commands/service.rs b/bin/fxt/src/commands/service.rs index 44434067c..1a3ceab0a 100644 --- a/bin/fxt/src/commands/service.rs +++ b/bin/fxt/src/commands/service.rs @@ -105,54 +105,59 @@ struct HealthEntry { // ── Display helpers ────────────────────────────────────────────────── fn service_health_str(h: i32) -> &'static str { - match h { - 1 => "healthy", - 2 => "degraded", - 3 => "unhealthy", - 4 => "critical", + use crate::proto::monitoring::ServiceHealth; + match ServiceHealth::try_from(h) { + Ok(ServiceHealth::Healthy) => "healthy", + Ok(ServiceHealth::Degraded) => "degraded", + Ok(ServiceHealth::Unhealthy) => "unhealthy", + Ok(ServiceHealth::Critical) => "critical", _ => "unknown", } } fn service_state_str(s: i32) -> &'static str { - match s { - 1 => "starting", - 2 => "running", - 3 => "stopping", - 4 => "stopped", - 5 => "error", + use crate::proto::monitoring::ServiceState; + match ServiceState::try_from(s) { + Ok(ServiceState::Starting) => "starting", + Ok(ServiceState::Running) => "running", + Ok(ServiceState::Stopping) => "stopping", + Ok(ServiceState::Stopped) => "stopped", + Ok(ServiceState::Error) => "error", _ => "unknown", } } fn system_health_str(h: i32) -> &'static str { - match h { - 1 => "healthy", - 2 => "degraded", - 3 => "unhealthy", - 4 => "critical", + use crate::proto::monitoring::SystemHealth; + match SystemHealth::try_from(h) { + Ok(SystemHealth::Healthy) => "healthy", + Ok(SystemHealth::Degraded) => "degraded", + Ok(SystemHealth::Unhealthy) => "unhealthy", + Ok(SystemHealth::Critical) => "critical", _ => "unknown", } } fn health_status_str(h: i32) -> &'static str { - match h { - 1 => "healthy", - 2 => "degraded", - 3 => "unhealthy", - 4 => "critical", + use crate::proto::monitoring::HealthStatus; + match HealthStatus::try_from(h) { + Ok(HealthStatus::Healthy) => "healthy", + Ok(HealthStatus::Degraded) => "degraded", + Ok(HealthStatus::Unhealthy) => "unhealthy", + Ok(HealthStatus::Critical) => "critical", _ => "unknown", } } fn dependency_type_str(t: i32) -> &'static str { - match t { - 1 => "database", - 2 => "message_queue", - 3 => "cache", - 4 => "external_api", - 5 => "file_system", - 6 => "network", + use crate::proto::monitoring::DependencyType; + match DependencyType::try_from(t) { + Ok(DependencyType::Database) => "database", + Ok(DependencyType::MessageQueue) => "message_queue", + Ok(DependencyType::Cache) => "cache", + Ok(DependencyType::ExternalApi) => "external_api", + Ok(DependencyType::FileSystem) => "file_system", + Ok(DependencyType::Network) => "network", _ => "unknown", } } diff --git a/bin/fxt/src/commands/train.rs b/bin/fxt/src/commands/train.rs index d8af0a734..23446a19f 100644 --- a/bin/fxt/src/commands/train.rs +++ b/bin/fxt/src/commands/train.rs @@ -116,13 +116,14 @@ struct TrainJobRow { // ── Display helpers ────────────────────────────────────────────────── fn training_status_str(s: i32) -> &'static str { - match s { - 1 => "pending", - 2 => "running", - 3 => "completed", - 4 => "failed", - 5 => "stopped", - 6 => "paused", + use crate::proto::ml_training::TrainingStatus; + match TrainingStatus::try_from(s) { + Ok(TrainingStatus::Pending) => "pending", + Ok(TrainingStatus::Running) => "running", + Ok(TrainingStatus::Completed) => "completed", + Ok(TrainingStatus::Failed) => "failed", + Ok(TrainingStatus::Stopped) => "stopped", + Ok(TrainingStatus::Paused) => "paused", _ => "unknown", } } @@ -140,14 +141,15 @@ fn colorize_status(s: &str) -> String { } fn status_filter_to_i32(s: &str) -> i32 { + use crate::proto::ml_training::TrainingStatus; match s.to_lowercase().as_str() { - "pending" => 1, - "running" => 2, - "completed" => 3, - "failed" => 4, - "stopped" => 5, - "paused" => 6, - _ => 0, + "pending" => TrainingStatus::Pending as i32, + "running" => TrainingStatus::Running as i32, + "completed" => TrainingStatus::Completed as i32, + "failed" => TrainingStatus::Failed as i32, + "stopped" => TrainingStatus::Stopped as i32, + "paused" => TrainingStatus::Paused as i32, + _ => TrainingStatus::Unknown as i32, } } @@ -289,7 +291,7 @@ impl TrainCommand { use_gpu: *gpu, description: String::new(), tags: Default::default(), - mode: 0, // FULL + mode: ml_training::TrainingMode::Full as i32, resume_checkpoint_path: String::new(), max_epochs: 0, }) diff --git a/bin/fxt/src/commands/tune.rs b/bin/fxt/src/commands/tune.rs index c1f9579df..3c31d626a 100644 --- a/bin/fxt/src/commands/tune.rs +++ b/bin/fxt/src/commands/tune.rs @@ -97,22 +97,24 @@ struct TuneApproveResult { // ── Display helpers ────────────────────────────────────────────────── fn tuning_status_str(s: i32) -> &'static str { - match s { - 1 => "pending", - 2 => "running", - 3 => "completed", - 4 => "failed", - 5 => "stopped", + use crate::proto::ml_training::TuningJobStatus; + match TuningJobStatus::try_from(s) { + Ok(TuningJobStatus::TuningPending) => "pending", + Ok(TuningJobStatus::TuningRunning) => "running", + Ok(TuningJobStatus::TuningCompleted) => "completed", + Ok(TuningJobStatus::TuningFailed) => "failed", + Ok(TuningJobStatus::TuningStopped) => "stopped", _ => "unknown", } } fn trial_state_str(s: i32) -> &'static str { - match s { - 1 => "running", - 2 => "complete", - 3 => "pruned", - 4 => "failed", + use crate::proto::ml_training::TrialState; + match TrialState::try_from(s) { + Ok(TrialState::TrialRunning) => "running", + Ok(TrialState::TrialComplete) => "complete", + Ok(TrialState::TrialPruned) => "pruned", + Ok(TrialState::TrialFailed) => "failed", _ => "unknown", } } diff --git a/bin/fxt/src/main.rs b/bin/fxt/src/main.rs index 1656ee07b..41d7dc6b5 100644 --- a/bin/fxt/src/main.rs +++ b/bin/fxt/src/main.rs @@ -11,7 +11,7 @@ use anyhow::Result; use clap::{Parser, Subcommand}; use fxt::commands::{ - agent, auth, backtest, broker, cluster, config_cmd, data, mcp, model, risk, service, trade, + agent, auth, backtest, broker, cluster, config, data, mcp, model, risk, service, trade, train, tune, watch, }; use fxt::grpc::FoxhuntClient; @@ -64,7 +64,7 @@ enum Commands { /// Risk management Risk(risk::RiskCommand), /// System configuration - Config(config_cmd::ConfigCommand), + Config(config::ConfigCommand), /// TUI cockpit dashboard Watch(watch::WatchCommand), /// MCP server mode diff --git a/bin/fxt/src/mcp/protocol.rs b/bin/fxt/src/mcp/protocol.rs index a1680a909..020a45ec9 100644 --- a/bin/fxt/src/mcp/protocol.rs +++ b/bin/fxt/src/mcp/protocol.rs @@ -46,6 +46,9 @@ pub struct JsonRpcError { // ── Standard JSON-RPC 2.0 error codes ─────────────────────────────── +/// The JSON sent is not valid JSON (parse error). +pub const PARSE_ERROR: i64 = -32700; + /// The JSON sent is not a valid JSON-RPC request. pub const INVALID_REQUEST: i64 = -32600; diff --git a/bin/fxt/src/mcp/server.rs b/bin/fxt/src/mcp/server.rs index 89719ab6e..9fd8e2309 100644 --- a/bin/fxt/src/mcp/server.rs +++ b/bin/fxt/src/mcp/server.rs @@ -8,7 +8,7 @@ use tokio::io::{AsyncBufReadExt, AsyncWriteExt, BufReader}; use super::protocol::{ error_response, success_response, JsonRpcRequest, JsonRpcResponse, INVALID_PARAMS, - INVALID_REQUEST, METHOD_NOT_FOUND, + INVALID_REQUEST, METHOD_NOT_FOUND, PARSE_ERROR, }; use super::tools; @@ -52,8 +52,8 @@ impl McpServer { Err(e) => { let resp = error_response( Value::Null, - INVALID_REQUEST, - &format!("invalid JSON: {e}"), + PARSE_ERROR, + &format!("Parse error: {e}"), ); write_response(&mut stdout, &resp).await?; continue; @@ -82,6 +82,14 @@ impl McpServer { async fn handle_request(&self, req: &JsonRpcRequest) -> Option { let id = req.id.clone().unwrap_or(Value::Null); + if req.jsonrpc != "2.0" { + return Some(error_response( + id, + INVALID_REQUEST, + "Invalid JSON-RPC version", + )); + } + match req.method.as_str() { "initialize" => Some(self.handle_initialize(id)), "notifications/initialized" | "notifications/cancelled" => None, @@ -349,7 +357,7 @@ impl McpServer { "stub: would get tuning progress via gRPC MLTraining/GetHyperoptStatus".into() } - _ => format!("unknown tool: {tool_name}"), + _ => return Err(anyhow::anyhow!("unknown tool: {tool_name}")), }; Ok(response) @@ -540,4 +548,31 @@ mod tests { let text = result["content"][0]["text"].as_str().unwrap(); assert!(text.contains("halt")); } + + #[tokio::test] + async fn malformed_json_returns_parse_error() { + // We cannot drive the full `run()` loop easily, so test the parse + // path directly: serde_json should fail, and server.rs uses PARSE_ERROR. + let bad_json = "{ not valid json }"; + let result: Result = serde_json::from_str(bad_json); + assert!(result.is_err()); + // Verify the constant value matches the JSON-RPC 2.0 spec. + assert_eq!(PARSE_ERROR, -32700); + } + + #[tokio::test] + async fn wrong_jsonrpc_version_returns_invalid_request() { + let server = make_server(); + let req = JsonRpcRequest { + jsonrpc: "1.0".into(), + id: Some(json!(99)), + method: "initialize".into(), + params: json!({}), + }; + let resp = server.handle_request(&req).await.unwrap(); + assert!(resp.error.is_some()); + let err = resp.error.unwrap(); + assert_eq!(err.code, INVALID_REQUEST); + assert_eq!(err.message, "Invalid JSON-RPC version"); + } } diff --git a/bin/fxt/src/tui/event_loop.rs b/bin/fxt/src/tui/event_loop.rs index d4614842b..18d9d37a0 100644 --- a/bin/fxt/src/tui/event_loop.rs +++ b/bin/fxt/src/tui/event_loop.rs @@ -41,7 +41,9 @@ pub async fn run(_api_url: &str) -> anyhow::Result<()> { let mut terminal = setup_terminal()?; let result = event_loop(&mut terminal).await; // Always restore terminal, even on error - teardown_terminal(&mut terminal)?; + if let Err(e) = teardown_terminal(&mut terminal) { + eprintln!("terminal teardown failed: {e}"); + } result }