fix(cfr): replace String validate errors with typed enums (#259)

elliottneilclark · web-flow · commit 20286c4fee53 · 2026-04-15T14:45:39.000-05:00
`RoundActionConfig::validate`, `ConfigurableActionConfig::validate`, and
`PreflopChartConfig::validate` all returned `Result&lt;(), String&gt;`, which
`AgentConfig::validate` then absorbed into a catch-all
`AgentConfigError::ValidationError(String)` variant — violating the
"never bare String errors" rule in CLAUDE.md.

Introduce `ConfigurableActionConfigError` and `PreflopChartConfigError`
thiserror enums, wire them through `AgentConfigError` via `#[from]`, and
replace the `ValidationError(String)` catch-all with typed variants (plus
a dedicated `PreflopChartPresetUnavailable` variant for the preset-not-
supported path). Add unit tests for each validation failure mode.
diff --git a/src/arena/agent/config.rs b/src/arena/agent/config.rs
@@ -94,11 +94,11 @@
 //!     pub fn validate(&self) -> Result<(), AgentConfigError> {
 //!         match self {
 //!             AgentConfig::MyNewAgent { param1, param2 } => {
-//!                 // Validate parameters
-//!                 if *param1 < 0.0 {
-//!                     return Err(AgentConfigError::ValidationError(
-//!                         "param1 must be non-negative".to_string()
-//!                     ));
+//!                 // Validate parameters via a domain-specific error
+//!                 // variant; add a new variant to AgentConfigError if
+//!                 // none of the existing typed variants fits.
+//!                 if *param1 < 0.0 || *param1 > 1.0 {
+//!                     return Err(AgentConfigError::InvalidProbability(*param1));
 //!                 }
 //!             }
 //!             // ... other variants ...
@@ -153,8 +153,9 @@ use crate::arena::agent::{
 };
 use crate::arena::cfr::{
     ActionGenerator, BasicCFRActionGenerator, CFRAgentBuilder, CFRState, CfrDepthConfig,
-    ConfigurableActionConfig, ConfigurableActionGenerator, PreflopChartActionConfig,
-    PreflopChartActionGenerator, PreflopChartConfig, SimpleActionGenerator, TraversalSet,
+    ConfigurableActionConfig, ConfigurableActionConfigError, ConfigurableActionGenerator,
+    PreflopChartActionConfig, PreflopChartActionGenerator, PreflopChartConfig,
+    PreflopChartConfigError, SimpleActionGenerator, TraversalSet,
 };
 use crate::arena::{Agent, GameState};
 use rand::SeedableRng;
@@ -295,12 +296,9 @@ impl PreflopChartConfigOption {
     /// Preset names are not currently supported - use inline configuration.
     pub fn resolve(&self) -> Result<PreflopChartConfig, AgentConfigError> {
         match self {
-            PreflopChartConfigOption::Preset(name) => {
-                Err(AgentConfigError::ValidationError(format!(
-                    "Preset charts are not available. Use inline configuration instead of preset '{}'. See examples/configs/preflop_6max_rfi.json for an example.",
-                    name
-                )))
-            }
+            PreflopChartConfigOption::Preset(name) => Err(
+                AgentConfigError::PreflopChartPresetUnavailable(name.clone()),
+            ),
             PreflopChartConfigOption::Inline(config) => Ok(config.clone()),
         }
     }
@@ -333,9 +331,21 @@ pub enum AgentConfigError {
     #[error("File I/O error: {0}")]
     IoError(#[from] std::io::Error),
 
-    /// Generic validation error
-    #[error("Validation error: {0}")]
-    ValidationError(String),
+    /// Failed to validate a configurable action generator config.
+    #[error("invalid configurable action config: {0}")]
+    ConfigurableActionConfig(#[from] ConfigurableActionConfigError),
+
+    /// Failed to validate a preflop chart config.
+    #[error("invalid preflop chart config: {0}")]
+    PreflopChartConfig(#[from] PreflopChartConfigError),
+
+    /// A preflop chart preset name was supplied, but presets are not yet
+    /// supported. Use an inline configuration instead.
+    #[error(
+        "preflop chart preset '{0}' is not available; use inline configuration instead. \
+         See examples/configs/preflop_6max_rfi.json for an example"
+    )]
+    PreflopChartPresetUnavailable(String),
 }
 
 impl AgentConfig {
@@ -369,15 +379,11 @@ impl AgentConfig {
                 validate_probabilities(percent_call)?;
             }
             AgentConfig::CfrConfigurable { action_config, .. } => {
-                action_config
-                    .validate()
-                    .map_err(AgentConfigError::ValidationError)?;
+                action_config.validate()?;
             }
             AgentConfig::CfrPreflopChart { preflop_config, .. } => {
                 let resolved = preflop_config.resolve()?;
-                resolved
-                    .validate()
-                    .map_err(AgentConfigError::ValidationError)?;
+                resolved.validate()?;
             }
             _ => {}
         }
diff --git a/src/arena/cfr/action_generator/configurable.rs b/src/arena/cfr/action_generator/configurable.rs
@@ -1,10 +1,26 @@
 use std::sync::Arc;
 
+use thiserror::Error;
+
 use crate::arena::{GameState, action::AgentAction, game_state::Round};
 
 use super::super::{CFRState, TraversalState};
 use super::ActionGenerator;
 
+/// Errors produced when validating a [`ConfigurableActionConfig`] or one of
+/// its nested [`RoundActionConfig`] entries.
+#[derive(Debug, Error, PartialEq)]
+pub enum ConfigurableActionConfigError {
+    /// A `raise_mult` entry was less than 1.0 (raises must be at least the
+    /// minimum raise size).
+    #[error("raise_mult must be >= 1.0 (cannot raise less than min raise), got {0}")]
+    RaiseMultBelowOne(f32),
+
+    /// A `pot_mult` entry was negative.
+    #[error("pot_mult must be non-negative, got {0}")]
+    PotMultNegative(f32),
+}
+
 /// Configuration for per-round bet sizing options.
 #[derive(Debug, Clone)]
 #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
@@ -35,19 +51,16 @@ impl Default for RoundActionConfig {
 }
 
 impl RoundActionConfig {
-    /// Validate the round configuration
-    pub fn validate(&self) -> Result<(), String> {
+    /// Validate the round configuration.
+    pub fn validate(&self) -> Result<(), ConfigurableActionConfigError> {
         for &mult in &self.raise_mult {
             if mult < 1.0 {
-                return Err(format!(
-                    "raise_mult must be >= 1.0 (cannot raise less than min raise), got {}",
-                    mult
-                ));
+                return Err(ConfigurableActionConfigError::RaiseMultBelowOne(mult));
             }
         }
         for &mult in &self.pot_mult {
             if mult < 0.0 {
-                return Err(format!("pot_mult must be non-negative, got {}", mult));
+                return Err(ConfigurableActionConfigError::PotMultNegative(mult));
             }
         }
         Ok(())
@@ -103,8 +116,8 @@ impl ConfigurableActionConfig {
         }
     }
 
-    /// Validate the configuration
-    pub fn validate(&self) -> Result<(), String> {
+    /// Validate the configuration.
+    pub fn validate(&self) -> Result<(), ConfigurableActionConfigError> {
         self.default.validate()?;
         if let Some(ref cfg) = self.preflop {
             cfg.validate()?;
@@ -293,6 +306,45 @@ mod tests {
         }
     }
 
+    #[test]
+    fn test_validate_raise_mult_below_one() {
+        let cfg = RoundActionConfig {
+            raise_mult: vec![0.5],
+            ..RoundActionConfig::default()
+        };
+        assert_eq!(
+            cfg.validate().unwrap_err(),
+            ConfigurableActionConfigError::RaiseMultBelowOne(0.5)
+        );
+    }
+
+    #[test]
+    fn test_validate_pot_mult_negative() {
+        let cfg = RoundActionConfig {
+            pot_mult: vec![-0.1],
+            ..RoundActionConfig::default()
+        };
+        assert_eq!(
+            cfg.validate().unwrap_err(),
+            ConfigurableActionConfigError::PotMultNegative(-0.1)
+        );
+    }
+
+    #[test]
+    fn test_validate_nested_round_override() {
+        let cfg = ConfigurableActionConfig {
+            preflop: Some(RoundActionConfig {
+                raise_mult: vec![0.9],
+                ..RoundActionConfig::default()
+            }),
+            ..ConfigurableActionConfig::default()
+        };
+        assert!(matches!(
+            cfg.validate().unwrap_err(),
+            ConfigurableActionConfigError::RaiseMultBelowOne(_)
+        ));
+    }
+
     #[test]
     fn test_configurable_gen_actions_basic() {
         let stacks = vec![500.0; 2];
diff --git a/src/arena/cfr/action_generator/mod.rs b/src/arena/cfr/action_generator/mod.rs
@@ -10,9 +10,13 @@ use crate::arena::{GameState, action::AgentAction};
 use super::{CFRState, TraversalState};
 
 pub use basic::BasicCFRActionGenerator;
-pub use configurable::{ConfigurableActionConfig, ConfigurableActionGenerator, RoundActionConfig};
+pub use configurable::{
+    ConfigurableActionConfig, ConfigurableActionConfigError, ConfigurableActionGenerator,
+    RoundActionConfig,
+};
 pub use preflop_chart::{
     PreflopChartActionConfig, PreflopChartActionGenerator, PreflopChartConfig,
+    PreflopChartConfigError,
 };
 pub use simple::SimpleActionGenerator;
 
diff --git a/src/arena/cfr/action_generator/preflop_chart.rs b/src/arena/cfr/action_generator/preflop_chart.rs
@@ -8,6 +8,7 @@
 use std::sync::Arc;
 
 use serde::{Deserialize, Serialize};
+use thiserror::Error;
 use tracing::event;
 
 use crate::arena::GameState;
@@ -18,6 +19,22 @@ use crate::holdem::{PreflopActionType, PreflopChart, PreflopHand};
 
 use super::{ActionGenerator, ConfigurableActionConfig, ConfigurableActionGenerator};
 
+/// Errors produced when validating a [`PreflopChartConfig`].
+#[derive(Debug, Error, PartialEq)]
+pub enum PreflopChartConfigError {
+    /// At least one preflop chart must be supplied.
+    #[error("at least one preflop chart is required")]
+    NoCharts,
+
+    /// `raise_size_bb` must be strictly positive.
+    #[error("raise_size_bb must be positive, got {0}")]
+    NonPositiveRaiseSizeBb(f32),
+
+    /// `three_bet_multiplier` must be strictly positive.
+    #[error("three_bet_multiplier must be positive, got {0}")]
+    NonPositiveThreeBetMultiplier(f32),
+}
+
 fn default_raise_size_bb() -> f32 {
     2.5
 }
@@ -131,15 +148,19 @@ impl PreflopChartConfig {
     }
 
     /// Validate the configuration.
-    pub fn validate(&self) -> Result<(), String> {
+    pub fn validate(&self) -> Result<(), PreflopChartConfigError> {
         if self.charts.is_empty() {
-            return Err("At least one preflop chart is required".to_string());
+            return Err(PreflopChartConfigError::NoCharts);
         }
         if self.raise_size_bb <= 0.0 {
-            return Err("raise_size_bb must be positive".to_string());
+            return Err(PreflopChartConfigError::NonPositiveRaiseSizeBb(
+                self.raise_size_bb,
+            ));
         }
         if self.three_bet_multiplier <= 0.0 {
-            return Err("three_bet_multiplier must be positive".to_string());
+            return Err(PreflopChartConfigError::NonPositiveThreeBetMultiplier(
+                self.three_bet_multiplier,
+            ));
         }
         Ok(())
     }
@@ -389,6 +410,45 @@ mod tests {
     use crate::core::Value;
     use crate::holdem::{PreflopChart, PreflopStrategy};
 
+    #[test]
+    fn test_validate_no_charts() {
+        let cfg = PreflopChartConfig {
+            charts: vec![],
+            raise_size_bb: 2.5,
+            three_bet_multiplier: 3.0,
+        };
+        assert_eq!(
+            cfg.validate().unwrap_err(),
+            PreflopChartConfigError::NoCharts
+        );
+    }
+
+    #[test]
+    fn test_validate_non_positive_raise_size() {
+        let cfg = PreflopChartConfig {
+            charts: vec![PreflopChart::new()],
+            raise_size_bb: 0.0,
+            three_bet_multiplier: 3.0,
+        };
+        assert_eq!(
+            cfg.validate().unwrap_err(),
+            PreflopChartConfigError::NonPositiveRaiseSizeBb(0.0)
+        );
+    }
+
+    #[test]
+    fn test_validate_non_positive_three_bet_multiplier() {
+        let cfg = PreflopChartConfig {
+            charts: vec![PreflopChart::new()],
+            raise_size_bb: 2.5,
+            three_bet_multiplier: -1.0,
+        };
+        assert_eq!(
+            cfg.validate().unwrap_err(),
+            PreflopChartConfigError::NonPositiveThreeBetMultiplier(-1.0)
+        );
+    }
+
     fn create_test_game_state() -> GameState {
         GameStateBuilder::new()
             .num_players_with_stack(2, 100.0)
diff --git a/src/arena/cfr/mod.rs b/src/arena/cfr/mod.rs
@@ -71,8 +71,9 @@ mod traversal_state;
 
 pub use action_generator::{
     ActionGenerator, BasicCFRActionGenerator, ConfigurableActionConfig,
-    ConfigurableActionGenerator, PreflopChartActionConfig, PreflopChartActionGenerator,
-    PreflopChartConfig, RoundActionConfig, SimpleActionGenerator,
+    ConfigurableActionConfigError, ConfigurableActionGenerator, PreflopChartActionConfig,
+    PreflopChartActionGenerator, PreflopChartConfig, PreflopChartConfigError, RoundActionConfig,
+    SimpleActionGenerator,
 };
 pub use action_index_mapper::{
     ACTION_IDX_ALL_IN, ACTION_IDX_CALL, ACTION_IDX_FOLD, ACTION_IDX_RAISE_MAX,
