refactor(acp-nats): extract value objects to own files, add type-safe per-VO errors (#21)

Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>

Author: yordis

rsworkspace/crates/AGENTS.md

@@ -1,6 +1,6 @@
 Prefer domain-specific value objects over primitives (e.g. `AcpPrefix` not `String`). Each type's factory must guarantee correctness at construction—invalid instances should be unrepresentable. Validate per-type, not per-aggregate: avoid validating unrelated fields together in a single constructor.
 
-When validating strings for NATS subjects or domain constraints, introduce a value object in its own file (e.g. `ext_method_name.rs`, `session_id.rs`) rather than a standalone `validate_*` function or adding it to `config.rs`. The value object's constructor performs validation; invalid instances are unrepresentable.
+Every value object lives in its own file named after the type (e.g. `acp_prefix.rs`, `ext_method_name.rs`, `session_id.rs`). Never inline a value object into a config, aggregate, or service file.
 
 You must use the `test-support` feature to share test helpers between crates.
 Prefer one trait per operation over a single trait with multiple operations.

rsworkspace/crates/acp-nats/src/acp_prefix.rs

@@ -0,0 +1,126 @@
+//! NATS-safe ACP prefix value object.
+//!
+//! The prefix is embedded in every NATS subject the bridge publishes:
+//! `{prefix}.agent.*`, `{prefix}.session.*`, etc.
+//! Validation follows [NATS subject naming](https://docs.nats.io/nats-concepts/subjects#characters-allowed-and-recommended-for-subject-names):
+//! rejects `*`, `>`, whitespace; allows dotted namespaces (e.g. `my.multi.part`) but rejects
+//! malformed dots (consecutive, leading, trailing). Max 128 bytes. Validity is guaranteed at
+//! construction.
+
+use std::sync::Arc;
+
+use crate::nats::token;
+use crate::subject_token_violation::SubjectTokenViolation;
+
+const MAX_PREFIX_LENGTH: usize = 128;
+
+/// Error returned when [`AcpPrefix`] validation fails.
+#[derive(Debug, Clone, PartialEq)]
+pub struct AcpPrefixError(pub SubjectTokenViolation);
+
+impl std::fmt::Display for AcpPrefixError {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        match &self.0 {
+            SubjectTokenViolation::Empty => write!(f, "acp_prefix must not be empty"),
+            SubjectTokenViolation::InvalidCharacter(ch) => {
+                write!(f, "acp_prefix contains invalid character: {:?}", ch)
+            }
+            SubjectTokenViolation::TooLong(len) => {
+                write!(f, "acp_prefix is too long: {} bytes (max 128)", len)
+            }
+        }
+    }
+}
+
+impl std::error::Error for AcpPrefixError {}
+
+/// NATS-safe ACP prefix. Guarantees validity at construction—invalid instances are unrepresentable.
+#[derive(Clone)]
+pub struct AcpPrefix(Arc<str>);
+
+impl AcpPrefix {
+    pub fn new(s: impl Into<String>) -> Result<Self, AcpPrefixError> {
+        let s = s.into();
+        if s.is_empty() {
+            return Err(AcpPrefixError(SubjectTokenViolation::Empty));
+        }
+        if let Some(ch) = token::has_wildcards_or_whitespace(&s) {
+            return Err(AcpPrefixError(SubjectTokenViolation::InvalidCharacter(ch)));
+        }
+        if token::has_consecutive_or_boundary_dots(&s) {
+            return Err(AcpPrefixError(SubjectTokenViolation::InvalidCharacter('.')));
+        }
+        if s.len() > MAX_PREFIX_LENGTH {
+            return Err(AcpPrefixError(SubjectTokenViolation::TooLong(s.len())));
+        }
+        Ok(Self(s.into()))
+    }
+
+    pub fn as_str(&self) -> &str {
+        &self.0
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn acp_prefix_new_valid() {
+        let p = AcpPrefix::new("acp").unwrap();
+        assert_eq!(p.as_str(), "acp");
+        assert_eq!(
+            AcpPrefix::new("my.multi.part").unwrap().as_str(),
+            "my.multi.part"
+        );
+    }
+
+    #[test]
+    fn acp_prefix_new_invalid_returns_err() {
+        assert!(AcpPrefix::new("").is_err());
+        assert!(AcpPrefix::new("acp.*").is_err());
+        assert!(AcpPrefix::new("acp.>").is_err());
+        assert!(AcpPrefix::new("acp prefix").is_err());
+        assert!(AcpPrefix::new("acp\t").is_err());
+        assert!(AcpPrefix::new("acp\n").is_err());
+        assert!(AcpPrefix::new("acp..foo").is_err());
+        assert!(AcpPrefix::new(".acp").is_err());
+        assert!(AcpPrefix::new("acp.").is_err());
+        assert!(AcpPrefix::new("a".repeat(129)).is_err());
+    }
+
+    #[test]
+    fn acp_prefix_new_validates_direct() {
+        assert!(AcpPrefix::new("acp").is_ok());
+        assert!(AcpPrefix::new("a").is_ok());
+        assert!(AcpPrefix::new("my.multi.part").is_ok());
+        assert!(AcpPrefix::new("a".repeat(128)).is_ok());
+        assert!(matches!(
+            AcpPrefix::new(""),
+            Err(AcpPrefixError(SubjectTokenViolation::Empty))
+        ));
+        assert!(matches!(
+            AcpPrefix::new("a".repeat(129)),
+            Err(AcpPrefixError(SubjectTokenViolation::TooLong(129)))
+        ));
+    }
+
+    #[test]
+    fn acp_prefix_error_display() {
+        assert_eq!(
+            format!("{}", AcpPrefixError(SubjectTokenViolation::Empty)),
+            "acp_prefix must not be empty"
+        );
+        assert_eq!(
+            format!(
+                "{}",
+                AcpPrefixError(SubjectTokenViolation::InvalidCharacter('*'))
+            ),
+            "acp_prefix contains invalid character: '*'"
+        );
+        assert_eq!(
+            format!("{}", AcpPrefixError(SubjectTokenViolation::TooLong(200))),
+            "acp_prefix is too long: 200 bytes (max 128)"
+        );
+    }
+}

rsworkspace/crates/acp-nats/src/agent/load_session.rs

@@ -1,5 +1,5 @@
 use super::Bridge;
-use crate::config::AcpPrefix;
+use crate::acp_prefix::AcpPrefix;
 use crate::error::AGENT_UNAVAILABLE;
 use crate::nats::{
     self, ExtSessionReady, FlushClient, FlushPolicy, PublishClient, PublishOptions, RequestClient,

rsworkspace/crates/acp-nats/src/config.rs

@@ -1,61 +1,9 @@
-use std::sync::Arc;
 use std::time::Duration;
 use trogon_nats::NatsConfig;
 
-use crate::nats::token;
+use crate::acp_prefix::AcpPrefix;
 
 const DEFAULT_OPERATION_TIMEOUT: Duration = Duration::from_secs(30);
-const MAX_PREFIX_LENGTH: usize = 128;
-
-/// Errors returned when [`Config`] values contain empty strings or NATS-unsafe characters.
-#[derive(Debug, Clone, PartialEq)]
-pub enum ValidationError {
-    EmptyValue(&'static str),
-    InvalidCharacter(&'static str, char),
-    TooLong(&'static str, usize),
-}
-
-impl std::fmt::Display for ValidationError {
-    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        match self {
-            ValidationError::EmptyValue(field) => write!(f, "{} must not be empty", field),
-            ValidationError::InvalidCharacter(field, ch) => {
-                write!(f, "{} contains invalid character: {:?}", field, ch)
-            }
-            ValidationError::TooLong(field, len) => {
-                write!(f, "{} is too long: {} bytes (max 128)", field, len)
-            }
-        }
-    }
-}
-
-impl std::error::Error for ValidationError {}
-
-#[derive(Clone)]
-pub struct AcpPrefix(Arc<str>);
-
-impl AcpPrefix {
-    pub fn new(s: impl Into<String>) -> Result<Self, ValidationError> {
-        let s = s.into();
-        if s.is_empty() {
-            return Err(ValidationError::EmptyValue("acp_prefix"));
-        }
-        if let Some(ch) = token::has_wildcards_or_whitespace(&s) {
-            return Err(ValidationError::InvalidCharacter("acp_prefix", ch));
-        }
-        if token::has_consecutive_or_boundary_dots(&s) {
-            return Err(ValidationError::InvalidCharacter("acp_prefix", '.'));
-        }
-        if s.len() > MAX_PREFIX_LENGTH {
-            return Err(ValidationError::TooLong("acp_prefix", s.len()));
-        }
-        Ok(Self(s.into()))
-    }
-
-    pub fn as_str(&self) -> &str {
-        &self.0
-    }
-}
 
 #[derive(Clone)]
 pub struct Config {
@@ -102,7 +50,6 @@ impl Config {
 
 #[cfg(test)]
 mod tests {
-    use std::error::Error;
     use std::time::Duration;
 
     use super::*;
@@ -120,62 +67,6 @@ mod tests {
         assert_eq!(config.acp_prefix(), "acp");
     }
 
-    #[test]
-    fn acp_prefix_new_valid() {
-        let p = AcpPrefix::new("acp").unwrap();
-        assert_eq!(p.as_str(), "acp");
-        assert_eq!(
-            AcpPrefix::new("my.multi.part").unwrap().as_str(),
-            "my.multi.part"
-        );
-    }
-
-    #[test]
-    fn acp_prefix_new_invalid_returns_err() {
-        assert!(AcpPrefix::new("").is_err());
-        assert!(AcpPrefix::new("acp.*").is_err());
-        assert!(AcpPrefix::new("acp.>").is_err());
-        assert!(AcpPrefix::new("acp prefix").is_err());
-        assert!(AcpPrefix::new("acp\t").is_err());
-        assert!(AcpPrefix::new("acp\n").is_err());
-        assert!(AcpPrefix::new("acp..foo").is_err());
-        assert!(AcpPrefix::new(".acp").is_err());
-        assert!(AcpPrefix::new("acp.").is_err());
-        assert!(AcpPrefix::new("a".repeat(129)).is_err());
-    }
-
-    #[test]
-    fn acp_prefix_new_validates_direct() {
-        assert!(AcpPrefix::new("acp").is_ok());
-        assert!(AcpPrefix::new("a").is_ok());
-        assert!(AcpPrefix::new("my.multi.part").is_ok());
-        assert!(AcpPrefix::new("a".repeat(128)).is_ok());
-        assert!(matches!(
-            AcpPrefix::new(""),
-            Err(ValidationError::EmptyValue(_))
-        ));
-        assert!(matches!(
-            AcpPrefix::new("a".repeat(129)),
-            Err(ValidationError::TooLong(_, 129))
-        ));
-    }
-
-    #[test]
-    fn validation_error_display() {
-        assert_eq!(
-            format!("{}", ValidationError::EmptyValue("acp_prefix")),
-            "acp_prefix must not be empty"
-        );
-        assert_eq!(
-            format!("{}", ValidationError::InvalidCharacter("acp_prefix", '*')),
-            "acp_prefix contains invalid character: '*'"
-        );
-        assert_eq!(
-            format!("{}", ValidationError::TooLong("acp_prefix", 200)),
-            "acp_prefix is too long: 200 bytes (max 128)"
-        );
-    }
-
     #[test]
     fn config_with_operation_timeout() {
         let config = Config::new(AcpPrefix::new("acp").unwrap(), default_nats())
@@ -189,10 +80,4 @@ mod tests {
         assert_eq!(config.nats().servers.len(), 1);
         assert_eq!(config.nats().servers[0], "localhost:4222");
     }
-
-    #[test]
-    fn validation_error_impl_error() {
-        let err = ValidationError::EmptyValue("field");
-        assert!(err.source().is_none());
-    }
 }

rsworkspace/crates/acp-nats/src/ext_method_name.rs

@@ -7,31 +7,55 @@
 
 use std::sync::Arc;
 
-use crate::config::ValidationError;
 use crate::nats::token;
+use crate::subject_token_violation::SubjectTokenViolation;
 
 const MAX_METHOD_NAME_LENGTH: usize = 128;
 
+/// Error returned when [`ExtMethodName`] validation fails.
+#[derive(Debug, Clone, PartialEq)]
+pub struct ExtMethodNameError(pub SubjectTokenViolation);
+
+impl std::fmt::Display for ExtMethodNameError {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        match &self.0 {
+            SubjectTokenViolation::Empty => write!(f, "method must not be empty"),
+            SubjectTokenViolation::InvalidCharacter(ch) => {
+                write!(f, "method contains invalid character: {:?}", ch)
+            }
+            SubjectTokenViolation::TooLong(len) => {
+                write!(f, "method is too long: {} bytes (max 128)", len)
+            }
+        }
+    }
+}
+
+impl std::error::Error for ExtMethodNameError {}
+
 /// NATS-safe extension method name. Guarantees validity at construction—invalid instances are unrepresentable.
 ///
 /// Rejects empty, too-long, wildcard, whitespace, and malformed dotted names.
 #[derive(Clone, Debug, PartialEq, Eq, Hash)]
 pub struct ExtMethodName(Arc<str>);
 
 impl ExtMethodName {
-    pub fn new(method: impl AsRef<str>) -> Result<Self, ValidationError> {
+    pub fn new(method: impl AsRef<str>) -> Result<Self, ExtMethodNameError> {
         let s = method.as_ref();
         if s.is_empty() {
-            return Err(ValidationError::EmptyValue("method"));
+            return Err(ExtMethodNameError(SubjectTokenViolation::Empty));
         }
         if s.len() > MAX_METHOD_NAME_LENGTH {
-            return Err(ValidationError::TooLong("method", s.len()));
+            return Err(ExtMethodNameError(SubjectTokenViolation::TooLong(s.len())));
         }
         if let Some(ch) = token::has_wildcards_or_whitespace(s) {
-            return Err(ValidationError::InvalidCharacter("method", ch));
+            return Err(ExtMethodNameError(SubjectTokenViolation::InvalidCharacter(
+                ch,
+            )));
         }
         if token::has_consecutive_or_boundary_dots(s) {
-            return Err(ValidationError::InvalidCharacter("method", '.'));
+            return Err(ExtMethodNameError(SubjectTokenViolation::InvalidCharacter(
+                '.',
+            )));
         }
         Ok(Self(s.into()))
     }
@@ -70,7 +94,7 @@ mod tests {
     fn ext_method_name_too_long_returns_err() {
         let long = "a".repeat(129);
         let err = ExtMethodName::new(&long).err().unwrap();
-        assert_eq!(err, ValidationError::TooLong("method", 129));
+        assert_eq!(err, ExtMethodNameError(SubjectTokenViolation::TooLong(129)));
     }
 
     #[test]
@@ -92,7 +116,7 @@ mod tests {
     #[test]
     fn ext_method_name_empty_returns_err() {
         let err = ExtMethodName::new("").err().unwrap();
-        assert_eq!(err, ValidationError::EmptyValue("method"));
+        assert_eq!(err, ExtMethodNameError(SubjectTokenViolation::Empty));
     }
 
     #[test]
@@ -114,4 +138,26 @@ mod tests {
         assert_eq!(name.len(), 9);
         assert!(name.starts_with("my"));
     }
+
+    #[test]
+    fn ext_method_name_error_display() {
+        assert_eq!(
+            format!("{}", ExtMethodNameError(SubjectTokenViolation::Empty)),
+            "method must not be empty"
+        );
+        assert_eq!(
+            format!(
+                "{}",
+                ExtMethodNameError(SubjectTokenViolation::InvalidCharacter(' '))
+            ),
+            "method contains invalid character: ' '"
+        );
+        assert_eq!(
+            format!(
+                "{}",
+                ExtMethodNameError(SubjectTokenViolation::TooLong(200))
+            ),
+            "method is too long: 200 bytes (max 128)"
+        );
+    }
 }