Tighten domain validation

src/domain.rs

@@ -94,6 +94,7 @@ pub struct PolicySnapshot {
 
 #[derive(Clone, Debug, PartialEq, Eq)]
 pub enum CommitmentValidationError {
+    MissingId,
     MissingNextAction,
     MissingSuccessCondition,
     MissingTimebox,
@@ -102,6 +103,7 @@ pub enum CommitmentValidationError {
 impl fmt::Display for CommitmentValidationError {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         match self {
+            CommitmentValidationError::MissingId => write!(f, "commitment id is required"),
             CommitmentValidationError::MissingNextAction => write!(f, "next action is required"),
             CommitmentValidationError::MissingSuccessCondition => {
                 write!(f, "success condition is required")
@@ -111,6 +113,8 @@ impl fmt::Display for CommitmentValidationError {
     }
 }
 
+impl std::error::Error for CommitmentValidationError {}
+
 #[derive(Clone, Debug, PartialEq, Eq)]
 pub enum PolicySnapshotValidationError {
     WrongSchemaVersion,
@@ -136,6 +140,8 @@ impl fmt::Display for PolicySnapshotValidationError {
     }
 }
 
+impl std::error::Error for PolicySnapshotValidationError {}
+
 impl Commitment {
     pub fn new_manual(
         next_action: impl Into<String>,
@@ -167,6 +173,9 @@ impl Commitment {
     }
 
     pub fn validate(&self) -> Result<(), CommitmentValidationError> {
+        if self.id.trim().is_empty() {
+            return Err(CommitmentValidationError::MissingId);
+        }
         if self.next_action.trim().is_empty() {
             return Err(CommitmentValidationError::MissingNextAction);
         }
@@ -187,8 +196,28 @@ impl PolicySnapshot {
         enforcement_level: EnforcementLevel,
         allowed_context: AllowedContext,
     ) -> Self {
+        Self::try_for_runtime(
+            commitment_id,
+            runtime_state,
+            enforcement_level,
+            allowed_context,
+        )
+        .expect("runtime policy snapshot requires a non-empty commitment id")
+    }
+
+    pub fn try_for_runtime(
+        commitment_id: String,
+        runtime_state: RuntimeState,
+        enforcement_level: EnforcementLevel,
+        allowed_context: AllowedContext,
+    ) -> Result<Self, PolicySnapshotValidationError> {
+        let commitment_id = commitment_id.trim().to_string();
+        if commitment_id.is_empty() {
+            return Err(PolicySnapshotValidationError::MissingCommitmentId);
+        }
+
         let created_at_unix_secs = unix_secs_now();
-        Self {
+        let policy = Self {
             id: unique_id("policy"),
             commitment_id,
             schema_version: POLICY_SCHEMA_VERSION,
@@ -200,7 +229,9 @@ impl PolicySnapshot {
             violation_actions: Vec::new(),
             expires_at_unix_secs: None,
             generated_by_agent_version: env!("CARGO_PKG_VERSION").to_string(),
-        }
+        };
+        policy.validate()?;
+        Ok(policy)
     }
 
     pub fn validate(&self) -> Result<(), PolicySnapshotValidationError> {
@@ -346,6 +377,13 @@ mod tests {
         let mut commitment =
             Commitment::new_manual("Refactor state", "tests pass", Duration::from_secs(1500))
                 .unwrap();
+        commitment.id = " ".to_string();
+        assert_eq!(
+            commitment.validate(),
+            Err(CommitmentValidationError::MissingId)
+        );
+
+        commitment.id = "commitment-1".to_string();
         commitment.next_action = " ".to_string();
         assert_eq!(
             commitment.validate(),
@@ -370,12 +408,12 @@ mod tests {
     #[test]
     fn commitment_validate_rejects_invalid_deserialized_values() {
         let json = r#"{
-            "id": "commitment-1",
+            "id": "",
             "created_at_unix_secs": 1,
             "source": "manual",
             "project_id": null,
             "template_id": null,
-            "next_action": "",
+            "next_action": "Refactor state",
             "success_condition": "tests pass",
             "timebox_secs": 1500,
             "transition_policy_id": null,
@@ -385,10 +423,31 @@ mod tests {
 
         assert_eq!(
             commitment.validate(),
-            Err(CommitmentValidationError::MissingNextAction)
+            Err(CommitmentValidationError::MissingId)
         );
     }
 
+    #[test]
+    fn validation_errors_implement_std_error() {
+        fn assert_std_error<E: std::error::Error>() {}
+
+        assert_std_error::<CommitmentValidationError>();
+        assert_std_error::<PolicySnapshotValidationError>();
+    }
+
+    #[test]
+    fn policy_snapshot_try_for_runtime_rejects_empty_commitment_id() {
+        let err = PolicySnapshot::try_for_runtime(
+            " ".to_string(),
+            RuntimeState::Active,
+            EnforcementLevel::Warn,
+            AllowedContext::default(),
+        )
+        .expect_err("empty commitment id must be rejected");
+
+        assert_eq!(err, PolicySnapshotValidationError::MissingCommitmentId);
+    }
+
     #[test]
     fn policy_snapshot_validate_rejects_invalid_public_field_values() {
         let mut policy = PolicySnapshot::for_runtime(