feat(agent): complete Agent Accountability remediation (Phases 1-9)

moloch-core/src/agent/emergency.rs

@@ -5,6 +5,7 @@
 use serde::{Deserialize, Serialize};
 
 use crate::crypto::PublicKey;
+use crate::error::{Error, Result};
 use crate::event::{EventId, ResourceId};
 
 use super::capability::{CapabilityId, CapabilityKind};
@@ -427,9 +428,13 @@ impl EmergencyEventBuilder {
     }
 
     /// Build the emergency event.
-    pub fn build(self) -> Result<EmergencyEvent, &'static str> {
-        let action = self.action.ok_or("action is required")?;
-        let initiator = self.initiator.ok_or("initiator is required")?;
+    pub fn build(self) -> Result<EmergencyEvent> {
+        let action = self
+            .action
+            .ok_or_else(|| Error::invalid_input("action is required"))?;
+        let initiator = self
+            .initiator
+            .ok_or_else(|| Error::invalid_input("initiator is required"))?;
         let priority = self.priority.unwrap_or(EmergencyPriority::High);
         let declared_at = self
             .declared_at
@@ -1096,4 +1101,17 @@ mod tests {
         let trigger = EmergencyTrigger::human_report(test_principal());
         assert_eq!(trigger.recommended_priority(), EmergencyPriority::High);
     }
+
+    // === Builder Error Type Consistency Tests (Finding 5.1) ===
+
+    #[test]
+    fn emergency_event_build_error_is_crate_error() {
+        let result = EmergencyEvent::builder()
+            .initiator(test_principal())
+            .build();
+
+        // Should return crate::error::Error, not &'static str
+        let err: crate::error::Error = result.unwrap_err();
+        assert!(err.to_string().contains("action"));
+    }
 }

moloch-core/src/agent/hitl.rs

@@ -780,7 +780,7 @@ impl ApprovalRequest {
     /// Apply a response to this request.
     pub fn apply_response(&mut self, response: &ApprovalResponse) -> Result<()> {
         // Verify request ID matches
-        if response.request_id != self.id {
+        if response.request_id() != self.id {
             return Err(Error::invalid_input("Response request_id does not match"));
         }
 
@@ -796,42 +796,42 @@ impl ApprovalRequest {
         }
 
         // Verify responder is an approver
-        if !self.can_approve(&response.responder) {
+        if !self.can_approve(response.responder()) {
             return Err(Error::invalid_input(
                 "Responder is not a valid approver for this request",
             ));
         }
 
-        match &response.decision {
+        match response.decision() {
             ApprovalDecision::Approve => {
                 self.collected_approvals
-                    .push((response.responder.clone(), response.responded_at));
+                    .push((response.responder().clone(), response.responded_at()));
 
                 if self.collected_approvals.len() >= self.policy.required_approvals as usize {
                     self.status = ApprovalStatus::Approved {
-                        approver: response.responder.clone(),
-                        approved_at: response.responded_at,
+                        approver: response.responder().clone(),
+                        approved_at: response.responded_at(),
                         modifications: None,
                     };
                 }
             }
             ApprovalDecision::ApproveWithModifications(mods) => {
                 self.collected_approvals
-                    .push((response.responder.clone(), response.responded_at));
+                    .push((response.responder().clone(), response.responded_at()));
 
                 if self.collected_approvals.len() >= self.policy.required_approvals as usize {
                     self.status = ApprovalStatus::Approved {
-                        approver: response.responder.clone(),
-                        approved_at: response.responded_at,
+                        approver: response.responder().clone(),
+                        approved_at: response.responded_at(),
                         modifications: Some(mods.clone()),
                     };
                 }
             }
             ApprovalDecision::Reject { reason } => {
                 if self.policy.any_can_reject {
                     self.status = ApprovalStatus::Rejected {
-                        rejector: response.responder.clone(),
-                        rejected_at: response.responded_at,
+                        rejector: response.responder().clone(),
+                        rejected_at: response.responded_at(),
                         reason: reason.clone(),
                     };
                 }
@@ -980,15 +980,15 @@ impl ApprovalDecision {
 #[derive(Debug, Clone, Serialize, Deserialize)]
 pub struct ApprovalResponse {
     /// The request being responded to.
-    pub request_id: ApprovalRequestId,
+    request_id: ApprovalRequestId,
     /// The human responding.
-    pub responder: PrincipalId,
+    responder: PrincipalId,
     /// The decision.
-    pub decision: ApprovalDecision,
+    decision: ApprovalDecision,
     /// When the response was made (Unix timestamp ms).
-    pub responded_at: i64,
+    responded_at: i64,
     /// Signature proving human involvement.
-    pub signature: Sig,
+    signature: Sig,
 }
 
 impl ApprovalResponse {
@@ -1007,6 +1007,31 @@ impl ApprovalResponse {
         }
     }
 
+    /// Get the request ID this response is for.
+    pub fn request_id(&self) -> ApprovalRequestId {
+        self.request_id
+    }
+
+    /// Get the responder principal.
+    pub fn responder(&self) -> &PrincipalId {
+        &self.responder
+    }
+
+    /// Get the approval decision.
+    pub fn decision(&self) -> &ApprovalDecision {
+        &self.decision
+    }
+
+    /// Get when the response was made (Unix timestamp ms).
+    pub fn responded_at(&self) -> i64 {
+        self.responded_at
+    }
+
+    /// Get the response signature.
+    pub fn signature(&self) -> &Sig {
+        &self.signature
+    }
+
     /// Sign the response.
     pub fn sign(mut self, secret_key: &crate::crypto::SecretKey) -> Self {
         let bytes = self.canonical_bytes();
@@ -1468,4 +1493,48 @@ mod tests {
         assert!(!ApprovalDecision::approve().is_rejection());
         assert!(ApprovalDecision::reject("no").is_rejection());
     }
+
+    // === ApprovalResponse Encapsulation Tests (Finding 3.1) ===
+
+    #[test]
+    fn approval_response_accessed_through_methods() {
+        let req_id = ApprovalRequestId::generate();
+        let principal = test_principal();
+
+        let response =
+            ApprovalResponse::new(req_id, principal.clone(), ApprovalDecision::approve());
+
+        assert_eq!(response.request_id(), req_id);
+        assert_eq!(response.responder(), &principal);
+        assert!(response.decision().is_approval());
+        assert!(response.responded_at() > 0);
+    }
+
+    #[test]
+    fn approval_response_signature_accessor() {
+        let key = SecretKey::generate();
+        let req_id = ApprovalRequestId::generate();
+        let principal = test_principal();
+
+        let response = ApprovalResponse::new(req_id, principal, ApprovalDecision::approve());
+        let signed = response.sign(&key);
+
+        // Signature should not be empty after signing
+        assert!(!signed.signature().is_empty());
+    }
+
+    #[test]
+    fn approval_response_rejection_via_accessor() {
+        let req_id = ApprovalRequestId::generate();
+        let principal = test_principal();
+
+        let response = ApprovalResponse::new(
+            req_id,
+            principal,
+            ApprovalDecision::reject("policy violation"),
+        );
+
+        assert!(response.decision().is_rejection());
+        assert!(!response.decision().is_approval());
+    }
 }

moloch-core/src/agent/id.rs

@@ -0,0 +1,160 @@
+//! Generic 16-byte identifier type for agent accountability.
+//!
+//! Provides a base `Id16` type and a `define_id!` macro for creating
+//! typed identifier wrappers. This eliminates the duplicated ID
+//! generation logic across multiple modules.
+
+use serde::{Deserialize, Serialize};
+
+use crate::error::{Error, Result};
+
+/// 16-byte random identifier base type.
+///
+/// All agent module identifiers share this common structure:
+/// a 16-byte array generated from a cryptographically secure RNG,
+/// with hex encoding for display and serialization.
+#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize)]
+pub struct Id16(pub [u8; 16]);
+
+impl Id16 {
+    /// Generate a new random identifier.
+    pub fn random() -> Self {
+        use rand::RngCore;
+        let mut bytes = [0u8; 16];
+        rand::thread_rng().fill_bytes(&mut bytes);
+        Self(bytes)
+    }
+
+    /// Create from raw bytes.
+    pub fn from_bytes(bytes: [u8; 16]) -> Self {
+        Self(bytes)
+    }
+
+    /// Get the raw bytes.
+    pub fn as_bytes(&self) -> &[u8; 16] {
+        &self.0
+    }
+
+    /// Convert to hex string.
+    pub fn to_hex(&self) -> String {
+        hex::encode(self.0)
+    }
+
+    /// Parse from hex string.
+    pub fn from_hex(s: &str) -> Result<Self> {
+        let bytes = hex::decode(s).map_err(|_| Error::invalid_input("invalid hex"))?;
+        if bytes.len() != 16 {
+            return Err(Error::invalid_input("ID must be 16 bytes"));
+        }
+        let mut arr = [0u8; 16];
+        arr.copy_from_slice(&bytes);
+        Ok(Self(arr))
+    }
+}
+
+impl std::fmt::Display for Id16 {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        write!(f, "{}", self.to_hex())
+    }
+}
+
+/// Macro for defining typed identifiers backed by [`Id16`].
+///
+/// Each generated type wraps an `Id16` and exposes the same API:
+/// `generate()`, `from_bytes()`, `as_bytes()`, `to_hex()`, `from_hex()`,
+/// and `Display`.
+///
+/// # Example
+///
+/// ```ignore
+/// define_id!(MyIdentifier, "my identifier");
+/// let id = MyIdentifier::generate();
+/// let hex = id.to_hex();
+/// let restored = MyIdentifier::from_hex(&hex).unwrap();
+/// assert_eq!(id, restored);
+/// ```
+#[macro_export]
+macro_rules! define_id {
+    ($name:ident, $desc:expr) => {
+        #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, serde::Serialize, serde::Deserialize)]
+        #[allow(dead_code)]
+        pub struct $name(pub [u8; 16]);
+
+        #[allow(dead_code, clippy::wrong_self_convention)]
+        impl $name {
+            /// Generate a new random identifier.
+            pub fn generate() -> Self {
+                let inner = $crate::agent::id::Id16::random();
+                Self(inner.0)
+            }
+
+            /// Create from raw bytes.
+            pub fn from_bytes(bytes: [u8; 16]) -> Self {
+                Self(bytes)
+            }
+
+            /// Get the raw bytes.
+            pub fn as_bytes(&self) -> &[u8; 16] {
+                &self.0
+            }
+
+            /// Convert to hex string.
+            pub fn to_hex(&self) -> String {
+                hex::encode(self.0)
+            }
+
+            #[doc = concat!("Parse a ", $desc, " from a hex string.")]
+            pub fn from_hex(s: &str) -> $crate::error::Result<Self> {
+                let inner = $crate::agent::id::Id16::from_hex(s)?;
+                Ok(Self(inner.0))
+            }
+        }
+
+        impl std::fmt::Display for $name {
+            fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+                write!(f, "{}", self.to_hex())
+            }
+        }
+    };
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn random_id_uniqueness() {
+        let id1 = Id16::random();
+        let id2 = Id16::random();
+        assert_ne!(id1, id2);
+    }
+
+    #[test]
+    fn random_id_hex_roundtrip() {
+        let id = Id16::random();
+        let hex = id.to_hex();
+        let restored = Id16::from_hex(&hex).unwrap();
+        assert_eq!(id, restored);
+    }
+
+    #[test]
+    fn random_id_display() {
+        let id = Id16::random();
+        let display = format!("{}", id);
+        assert_eq!(display.len(), 32); // 16 bytes = 32 hex chars
+    }
+
+    #[test]
+    fn define_id_macro_works() {
+        define_id!(TestId, "test identifier");
+
+        let id1 = TestId::generate();
+        let id2 = TestId::generate();
+        assert_ne!(id1, id2);
+
+        let hex = id1.to_hex();
+        let restored = TestId::from_hex(&hex).unwrap();
+        assert_eq!(id1, restored);
+        assert_eq!(format!("{}", id1).len(), 32);
+    }
+}