refactor(host_api): centralize system-sentinel id minting and pin authority contract

crates/ironclaw_host_api/src/ids.rs

@@ -99,6 +99,20 @@ fn validate_name_segment(kind: &'static str, value: &str) -> Result<(), HostApiE
 
 macro_rules! string_id {
     ($name:ident, $kind:literal, $validator:ident) => {
+        string_id!(@build $name, $kind, $validator);
+        string_id!(@deserialize_strict $name);
+    };
+    ($name:ident, $kind:literal, $validator:ident, accepts_system_sentinel) => {
+        string_id!(@build $name, $kind, $validator);
+        string_id!(@sentinel_constructor $name);
+        // Wire deserialize stays STRICT even for sentinel-aware id kinds: a
+        // bare `TenantId`/`UserId` decoded from untrusted JSON must reject the
+        // sentinel. Sentinel admission is scoped to `ResourceScope`'s own
+        // field-level `deserialize_with` (a TRUSTED-PERSISTENCE shape), never
+        // to the id type itself. See `ResourceScope` docs in resource.rs.
+        string_id!(@deserialize_strict $name);
+    };
+    (@build $name:ident, $kind:literal, $validator:ident) => {
         #[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)]
         pub struct $name(String);
 
@@ -109,10 +123,11 @@ macro_rules! string_id {
                 Ok(Self(value))
             }
 
-            /// Construct without validation. Reserved for sentinel values
-            /// that intentionally contain bytes the validator rejects (e.g.
-            /// [`crate::SYSTEM_RESERVED_ID`]), so no caller-supplied
-            /// identifier can collide with them.
+            /// Construct without validation. For non-sentinel trusted string
+            /// rehydration (e.g. DB row values handed over from a typed
+            /// upstream). To mint the system sentinel
+            /// ([`crate::SYSTEM_RESERVED_ID`]), use `system_sentinel()` instead
+            /// — it is the single `git grep`-able authority-elevating path.
             pub fn from_trusted(value: String) -> Self {
                 Self(value)
             }
@@ -140,7 +155,8 @@ macro_rules! string_id {
                 serializer.serialize_str(&self.0)
             }
         }
-
+    };
+    (@deserialize_strict $name:ident) => {
         impl<'de> Deserialize<'de> for $name {
             fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
             where
@@ -151,6 +167,26 @@ macro_rules! string_id {
             }
         }
     };
+    (@sentinel_constructor $name:ident) => {
+        impl $name {
+            /// Construct the [`crate::SYSTEM_RESERVED_ID`] sentinel value of
+            /// this id type.
+            ///
+            /// SECURITY-CRITICAL: this is the blessed path for minting a
+            /// sentinel-valued id. It exists so every authority-elevating
+            /// constructor is `git grep`-able as `system_sentinel`, rather
+            /// than hidden behind the more general `from_trusted` escape
+            /// hatch. New code that needs the sentinel must call this
+            /// method — never `from_trusted(SYSTEM_RESERVED_ID.to_string())`.
+            ///
+            /// `ResourceScope::is_system` returns `true` only when BOTH the
+            /// tenant and user ids of a scope come from this constructor;
+            /// no authority decision may key on a single field.
+            pub fn system_sentinel() -> Self {
+                Self::from_trusted(crate::SYSTEM_RESERVED_ID.to_string())
+            }
+        }
+    };
 }
 
 macro_rules! uuid_id {
@@ -191,8 +227,20 @@ macro_rules! uuid_id {
     };
 }
 
-string_id!(TenantId, "tenant", validate_scope_id);
-string_id!(UserId, "user", validate_scope_id);
+// SECURITY-CRITICAL: `accepts_system_sentinel` opts these id types into
+// admitting [`crate::SYSTEM_RESERVED_ID`] during JSON deserialization and
+// exposes the `system_sentinel()` constructor — the only blessed path that
+// can mint a sentinel-valued id. Every other id kind stays strict and
+// rejects the sentinel on the wire. Adding this flag to any new id type
+// requires security review: any HTTP/RPC endpoint that deserializes such an
+// id from an untrusted request body becomes an authority-elevation vector.
+string_id!(
+    TenantId,
+    "tenant",
+    validate_scope_id,
+    accepts_system_sentinel
+);
+string_id!(UserId, "user", validate_scope_id, accepts_system_sentinel);
 string_id!(AgentId, "agent", validate_scope_id);
 string_id!(ProjectId, "project", validate_scope_id);
 string_id!(MissionId, "mission", validate_scope_id);

crates/ironclaw_host_api/src/resource.rs

@@ -111,8 +111,8 @@ impl ResourceScope {
     /// validation rejects, so no user-supplied identifier can collide.
     pub fn system() -> Self {
         Self {
-            tenant_id: TenantId::from_trusted(SYSTEM_RESERVED_ID.to_string()),
-            user_id: UserId::from_trusted(SYSTEM_RESERVED_ID.to_string()),
+            tenant_id: TenantId::system_sentinel(),
+            user_id: UserId::system_sentinel(),
             agent_id: None,
             project_id: None,
             mission_id: None,

crates/ironclaw_host_api/tests/host_api_contract.rs

@@ -206,6 +206,145 @@ fn scope_ids_reject_path_segments_and_controls() {
     }
 }
 
+#[test]
+fn system_resource_scope_round_trips_through_json() {
+    let scope = ResourceScope::system();
+    assert!(scope.is_system());
+
+    let encoded = serde_json::to_string(&scope).expect("serialize system scope");
+    let decoded: ResourceScope = serde_json::from_str(&encoded).expect("deserialize system scope");
+
+    assert!(decoded.is_system());
+    assert_eq!(decoded.tenant_id.as_str(), SYSTEM_RESERVED_ID);
+    assert_eq!(decoded.user_id.as_str(), SYSTEM_RESERVED_ID);
+
+    // Sentinel stays unreachable through ordinary construction; only
+    // `system_sentinel()` may mint it, so user input can never collide.
+    assert!(TenantId::new(SYSTEM_RESERVED_ID).is_err());
+    assert!(UserId::new(SYSTEM_RESERVED_ID).is_err());
+}
+
+#[test]
+fn system_sentinel_constructor_produces_the_reserved_value() {
+    // `system_sentinel()` is the blessed cross-crate path for minting a
+    // sentinel-valued id. Pin its identity and pin that `ResourceScope::system`
+    // (which routes through this constructor) classifies as system — this is
+    // the contract the four legitimate sentinel call sites (host_api,
+    // loop_support, turns, threads) depend on.
+    assert_eq!(TenantId::system_sentinel().as_str(), SYSTEM_RESERVED_ID);
+    assert_eq!(UserId::system_sentinel().as_str(), SYSTEM_RESERVED_ID);
+    assert!(ResourceScope::system().is_system());
+}
+
+#[test]
+fn bare_id_kinds_reject_system_sentinel_on_the_wire() {
+    // SECURITY GUARD (PR review finding #1): NO bare id type — not even the
+    // sentinel-aware TenantId/UserId — admits the sentinel during its own
+    // `Deserialize`. Sentinel admission lives exclusively on `ResourceScope`'s
+    // field-level `deserialize_with` (a TRUSTED-PERSISTENCE shape). A bare
+    // TenantId/UserId decoded from an untrusted body must reject the sentinel,
+    // otherwise any endpoint deserializing one becomes an authority-elevation
+    // vector. Each kind asserts independently so a macro regression is caught.
+    let raw = json!(SYSTEM_RESERVED_ID);
+
+    assert!(serde_json::from_value::<TenantId>(raw.clone()).is_err());
+    assert!(serde_json::from_value::<UserId>(raw.clone()).is_err());
+    assert!(serde_json::from_value::<AgentId>(raw.clone()).is_err());
+    assert!(serde_json::from_value::<ProjectId>(raw.clone()).is_err());
+    assert!(serde_json::from_value::<MissionId>(raw.clone()).is_err());
+    assert!(serde_json::from_value::<ThreadId>(raw.clone()).is_err());
+    assert!(serde_json::from_value::<ExtensionId>(raw.clone()).is_err());
+    assert!(serde_json::from_value::<PackageId>(raw.clone()).is_err());
+    assert!(serde_json::from_value::<SecretHandle>(raw.clone()).is_err());
+    assert!(serde_json::from_value::<CapabilityId>(raw.clone()).is_err());
+    assert!(serde_json::from_value::<RuntimeCredentialAccountProviderId>(raw.clone()).is_err());
+    assert!(serde_json::from_value::<SystemServiceId>(raw).is_err());
+}
+
+#[test]
+fn tenant_and_user_ids_reject_sentinel_near_misses_on_the_wire() {
+    // The sentinel is exactly `\x1fSYSTEM\x1f`. Any near-miss containing
+    // ANY control byte (including the right one in the wrong place, or
+    // alongside extra content) must be rejected by the validator, otherwise
+    // an attacker who controlled the wire could try to smuggle an
+    // almost-sentinel through the sentinel-aware deserialize path.
+    let control_byte_near_misses = [
+        "\x1eSYSTEM\x1f",   // wrong leading control byte
+        "\x1fSYSTEM\x1e",   // wrong trailing control byte
+        "\x1fSYSTEM",       // missing trailing byte
+        "SYSTEM\x1f",       // missing leading byte
+        "\x1fSYSTEM\x1fx",  // trailing extra
+        "x\x1fSYSTEM\x1f",  // leading extra
+        "\x1fsystem\x1f",   // wrong case
+        "\x1f SYSTEM \x1f", // padded
+    ];
+    for value in control_byte_near_misses {
+        assert!(
+            serde_json::from_value::<TenantId>(json!(value)).is_err(),
+            "TenantId must reject control-byte near-miss {value:?}"
+        );
+        assert!(
+            serde_json::from_value::<UserId>(json!(value)).is_err(),
+            "UserId must reject control-byte near-miss {value:?}"
+        );
+    }
+}
+
+#[test]
+fn sentinel_look_alikes_without_control_bytes_do_not_classify_as_system() {
+    // Strings like "SYSTEM" or "system" deserialize successfully (they are
+    // valid scope-ids), but the sentinel-equality check in
+    // `ResourceScope::is_system` must NOT mistake them for the real sentinel.
+    // This is the wire-level pin: no string a caller can legally supply
+    // through TenantId/UserId may be classified as system.
+    for look_alike in ["SYSTEM", "system", "SYSTEMx"] {
+        let tenant: TenantId = serde_json::from_value(json!(look_alike))
+            .unwrap_or_else(|e| panic!("TenantId should accept {look_alike:?}: {e}"));
+        let user: UserId = serde_json::from_value(json!(look_alike))
+            .unwrap_or_else(|e| panic!("UserId should accept {look_alike:?}: {e}"));
+        assert_ne!(tenant.as_str(), SYSTEM_RESERVED_ID);
+        assert_ne!(user.as_str(), SYSTEM_RESERVED_ID);
+
+        let scope = ResourceScope {
+            tenant_id: tenant,
+            user_id: user,
+            agent_id: None,
+            project_id: None,
+            mission_id: None,
+            thread_id: None,
+            invocation_id: InvocationId::new(),
+        };
+        assert!(
+            !scope.is_system(),
+            "{look_alike:?} must never classify as the system sentinel"
+        );
+    }
+}
+
+#[test]
+fn partial_sentinel_resource_scope_is_not_treated_as_system() {
+    // `is_system` requires BOTH tenant_id and user_id to be the sentinel.
+    // If only one field carries the sentinel and the other is a normal id,
+    // the scope must round-trip through JSON without being misclassified.
+    // This pins the contract: no authority decision may key on a single
+    // field of the scope.
+    let mut tenant_only = ResourceScope::system();
+    tenant_only.user_id = UserId::new("alice").unwrap();
+    let encoded = serde_json::to_string(&tenant_only).expect("serialize tenant-only");
+    let decoded: ResourceScope = serde_json::from_str(&encoded).expect("deserialize tenant-only");
+    assert!(!decoded.is_system());
+    assert_eq!(decoded.tenant_id.as_str(), SYSTEM_RESERVED_ID);
+    assert_eq!(decoded.user_id.as_str(), "alice");
+
+    let mut user_only = ResourceScope::system();
+    user_only.tenant_id = TenantId::new("acme").unwrap();
+    let encoded = serde_json::to_string(&user_only).expect("serialize user-only");
+    let decoded: ResourceScope = serde_json::from_str(&encoded).expect("deserialize user-only");
+    assert!(!decoded.is_system());
+    assert_eq!(decoded.tenant_id.as_str(), "acme");
+    assert_eq!(decoded.user_id.as_str(), SYSTEM_RESERVED_ID);
+}
+
 #[test]
 fn local_default_resource_scope_uses_default_agent_and_bootstrap_project() {
     let invocation_id = InvocationId::new();

crates/ironclaw_loop_support/src/budget_accountant.rs

@@ -17,8 +17,7 @@ use async_trait::async_trait;
 use chrono::Utc;
 use dashmap::{DashMap, DashSet};
 use ironclaw_host_api::{
-    InvocationId, ResourceEstimate, ResourceReservationId, ResourceScope, ResourceUsage,
-    SYSTEM_RESERVED_ID, UserId,
+    InvocationId, ResourceEstimate, ResourceReservationId, ResourceScope, ResourceUsage, UserId,
 };
 use ironclaw_resources::{
     BudgetApprovalGate, BudgetEvent, BudgetEventSink, BudgetGateId, BudgetGateStatus,
@@ -397,7 +396,7 @@ impl GovernorBackedAccountant {
             .actor
             .as_ref()
             .map(|actor| actor.user_id.clone())
-            .unwrap_or_else(|| UserId::from_trusted(SYSTEM_RESERVED_ID.to_string()));
+            .unwrap_or_else(UserId::system_sentinel);
         ResourceScope {
             tenant_id: context.scope.tenant_id.clone(),
             user_id,
@@ -680,6 +679,10 @@ mod tests {
     use rust_decimal_macros::dec;
 
     fn run_context() -> LoopRunContext {
+        run_context_without_actor().with_actor(TurnActor::new(UserId::new("acct-user").unwrap()))
+    }
+
+    fn run_context_without_actor() -> LoopRunContext {
         let scope = TurnScope::new(
             TenantId::new("tenant-acct").unwrap(),
             None,
@@ -739,7 +742,6 @@ mod tests {
             },
         };
         LoopRunContext::new(scope, TurnId::new(), TurnRunId::new(), profile)
-            .with_actor(TurnActor::new(UserId::new("acct-user").unwrap()))
     }
 
     fn sample_request() -> LoopModelRequest {
@@ -783,6 +785,38 @@ mod tests {
         let _ = snapshot;
     }
 
+    #[test]
+    fn resource_scope_falls_back_to_system_sentinel_user_without_actor() {
+        // PR review finding #5: exercise the no-actor branch of
+        // `resource_scope`. With no actor, `user_id` resolves to the system
+        // sentinel; the real tenant keeps the scope from classifying as system.
+        let accountant = GovernorBackedAccountant::new(
+            Arc::new(InMemoryResourceGovernor::new()),
+            Arc::new(ZeroCostTable),
+        );
+        let context = run_context_without_actor();
+        assert!(context.actor.is_none());
+
+        let scope = accountant.resource_scope(&context);
+        assert_eq!(
+            scope.user_id.as_str(),
+            ironclaw_host_api::SYSTEM_RESERVED_ID
+        );
+        assert_eq!(scope.tenant_id.as_str(), "tenant-acct");
+        assert!(!scope.is_system());
+    }
+
+    #[test]
+    fn resource_scope_uses_actor_user_when_present() {
+        let accountant = GovernorBackedAccountant::new(
+            Arc::new(InMemoryResourceGovernor::new()),
+            Arc::new(ZeroCostTable),
+        );
+        let scope = accountant.resource_scope(&run_context());
+        assert_eq!(scope.user_id.as_str(), "acct-user");
+        assert!(!scope.is_system());
+    }
+
     #[tokio::test]
     async fn pre_model_call_returns_budget_exceeded_when_limit_zero_but_negative() {
         // A negative-USD estimate would be invalid input, but our default

crates/ironclaw_threads/src/contract.rs

@@ -29,11 +29,10 @@ impl ThreadScope {
     pub fn to_resource_scope(&self) -> ironclaw_host_api::ResourceScope {
         ironclaw_host_api::ResourceScope {
             tenant_id: self.tenant_id.clone(),
-            user_id: self.owner_user_id.clone().unwrap_or_else(|| {
-                ironclaw_host_api::UserId::from_trusted(
-                    ironclaw_host_api::SYSTEM_RESERVED_ID.to_string(),
-                )
-            }),
+            user_id: self
+                .owner_user_id
+                .clone()
+                .unwrap_or_else(ironclaw_host_api::UserId::system_sentinel),
             agent_id: Some(self.agent_id.clone()),
             project_id: self.project_id.clone(),
             mission_id: self.mission_id.clone(),
@@ -501,6 +500,14 @@ pub struct UpdateThreadGoalRequest {
 #[cfg(test)]
 mod tests {
     use super::*;
+
+    fn scope(owner: Option<UserId>) -> ThreadScope {
+        ThreadScope {
+            tenant_id: TenantId::new("acme").unwrap(),
+            agent_id: AgentId::new("agent-1").unwrap(),
+            project_id: None,
+            owner_user_id: owner,
+            mission_id: None,
     use ironclaw_common::AttachmentKind;
 
     fn sample_ref() -> AttachmentRef {
@@ -516,6 +523,24 @@ mod tests {
     }
 
     #[test]
+    fn to_resource_scope_falls_back_to_system_sentinel_user_without_owner() {
+        // PR review finding #4: drive the real caller. An ownerless thread
+        // scope must resolve `user_id` to the system sentinel, while the real
+        // tenant keeps the scope from classifying as fully system.
+        let resource = scope(None).to_resource_scope();
+        assert_eq!(
+            resource.user_id.as_str(),
+            ironclaw_host_api::SYSTEM_RESERVED_ID
+        );
+        assert_eq!(resource.tenant_id.as_str(), "acme");
+        assert!(!resource.is_system());
+    }
+
+    #[test]
+    fn to_resource_scope_uses_explicit_owner_when_present() {
+        let resource = scope(Some(UserId::new("alice").unwrap())).to_resource_scope();
+        assert_eq!(resource.user_id.as_str(), "alice");
+        assert!(!resource.is_system());
     fn text_constructor_carries_no_attachments() {
         let content = MessageContent::text("hello");
         assert_eq!(content.as_text(), "hello");