hooks: address henrypark approval-with-followup items from #3911/#3912/#3913

crates/ironclaw_hooks/src/identity.rs

@@ -212,9 +212,30 @@ impl ExtensionId {
         &self.0
     }
 
-    pub fn into_string(self) -> String {
+    /// Canonical newtype-template accessor for taking the inner string by
+    /// value. Prefer this over [`Self::into_string`] in new code.
+    pub fn into_inner(self) -> String {
         self.0
     }
+
+    /// Legacy alias retained for callers that predate the canonical
+    /// `into_inner` convention from `.claude/rules/types.md`. New code
+    /// should use [`Self::into_inner`].
+    pub fn into_string(self) -> String {
+        self.into_inner()
+    }
+}
+
+impl AsRef<str> for ExtensionId {
+    fn as_ref(&self) -> &str {
+        &self.0
+    }
+}
+
+impl From<ExtensionId> for String {
+    fn from(id: ExtensionId) -> Self {
+        id.0
+    }
 }
 
 impl fmt::Display for ExtensionId {
@@ -271,9 +292,30 @@ impl HookLocalId {
         &self.0
     }
 
-    pub fn into_string(self) -> String {
+    /// Canonical newtype-template accessor for taking the inner string by
+    /// value. Prefer this over [`Self::into_string`] in new code.
+    pub fn into_inner(self) -> String {
         self.0
     }
+
+    /// Legacy alias retained for callers that predate the canonical
+    /// `into_inner` convention from `.claude/rules/types.md`. New code
+    /// should use [`Self::into_inner`].
+    pub fn into_string(self) -> String {
+        self.into_inner()
+    }
+}
+
+impl AsRef<str> for HookLocalId {
+    fn as_ref(&self) -> &str {
+        &self.0
+    }
+}
+
+impl From<HookLocalId> for String {
+    fn from(id: HookLocalId) -> Self {
+        id.0
+    }
 }
 
 impl fmt::Display for HookLocalId {
@@ -356,7 +398,16 @@ mod tests {
     #[test]
     fn builtin_id_distinct_from_extension_id() {
         // Use a `.`-separated local id, which is permitted by the segment
-        // grammar (mirroring host-api's `validate_name_segment`).
+        // grammar (mirroring host-api's `validate_name_segment`). The
+        // original fixture used `"path::module"` for the installed local id,
+        // but the post-#3912 segment grammar disallows `:` characters
+        // anywhere in a `HookLocalId`, so we substitute the equivalent
+        // legal value `"path.module"`. The assertion's intent — that the
+        // builtin and installed digests are distinct for any pair of
+        // syntactically legal ids — is unchanged; `HookId::for_builtin`
+        // accepts a free-form canonical path (not a `HookLocalId`) so the
+        // builtin side can still carry the original `"path::module"`
+        // shape.
         let installed = HookId::derive(
             &ext("builtin"),
             "x",

crates/ironclaw_hooks/src/manifest.rs

@@ -220,9 +220,12 @@ impl HookManifestEntry {
     /// (trust class assignment, scope grant matching, hook-id pinning all
     /// happen later in the installer).
     pub fn validate(&self) -> Result<(), HookManifestValidationError> {
-        if self.id.as_str().is_empty() {
-            return Err(HookManifestValidationError("hook id is empty".to_string()));
-        }
+        // Note: the previous empty-id guard here is unreachable now that
+        // `HookLocalId::new` rejects empty strings at construction time
+        // (see `crates/ironclaw_hooks/src/identity.rs`), so manifest
+        // deserialization fails before this method is ever called. Removed
+        // per henrypark133 review of PR #3912 (finding L1).
+        //
         // Phase × Trust: a manifest hook is always Installed, so it cannot
         // register at Validation or Authorization.
         if matches!(self.phase, HookPhase::Validation | HookPhase::Authorization) {