fix: formatting hooks to enable correct caching with the API (#461)

Author: brandonskiser

crates/chat-cli/Cargo.toml

@@ -117,7 +117,7 @@ rustyline = { version = "15.0.0", features = [
 semantic_search_client = { path = "../semantic_search_client" }
 semver = { version = "1.0.26", features = ["serde"] }
 serde = { version = "1.0.219", features = ["derive", "rc"] }
-serde_json = "1.0.140"
+serde_json = { version = "1.0.140", features = ["preserve_order"] }
 sha2 = "0.10.9"
 shell-color = "1.0.0"
 shell-words = "1.1.0"

crates/chat-cli/src/cli/chat/cli/hooks.rs

@@ -873,8 +873,14 @@ mod tests {
         manager.add_hook(&os, "hook2".to_string(), hook2, false).await?;
 
         // Run the hooks
-        let results = manager.run_hooks(&mut vec![]).await.unwrap();
-        assert_eq!(results.len(), 2); // Should include both hooks
+        let results = manager
+            .run_hooks(HookTrigger::ConversationStart, &mut vec![])
+            .await
+            .unwrap();
+        assert_eq!(results.len(), 2);
+
+        let results = manager.run_hooks(HookTrigger::PerPrompt, &mut vec![]).await.unwrap();
+        assert_eq!(results.len(), 0);
 
         Ok(())
     }
@@ -889,14 +895,20 @@ mod tests {
         manager.add_hook(&os, "profile_hook".to_string(), hook1, false).await?;
         manager.add_hook(&os, "global_hook".to_string(), hook2, true).await?;
 
-        let results = manager.run_hooks(&mut vec![]).await.unwrap();
+        let results = manager
+            .run_hooks(HookTrigger::ConversationStart, &mut vec![])
+            .await
+            .unwrap();
         assert_eq!(results.len(), 2); // Should include both hooks
 
         // Create and switch to a new profile
         manager.create_profile(&os, "test_profile").await?;
         manager.switch_profile(&os, "test_profile").await?;
 
-        let results = manager.run_hooks(&mut vec![]).await.unwrap();
+        let results = manager
+            .run_hooks(HookTrigger::ConversationStart, &mut vec![])
+            .await
+            .unwrap();
         assert_eq!(results.len(), 1); // Should include global hook
         assert_eq!(results[0].0.name, "global_hook");
 

crates/chat-cli/src/cli/chat/context.rs

@@ -17,6 +17,7 @@ use serde::{
 };
 use tracing::debug;
 
+use super::cli::hooks::HookTrigger;
 use super::consts::CONTEXT_FILES_MAX_SIZE;
 use super::util::drop_matched_context_files;
 use crate::cli::chat::ChatError;
@@ -573,7 +574,11 @@ impl ContextManager {
     /// * `updates` - output stream to write hook run status to if Some, else do nothing if None
     /// # Returns
     /// A vector containing pairs of a [`Hook`] definition and its execution output
-    pub async fn run_hooks(&mut self, output: &mut impl Write) -> Result<Vec<(Hook, String)>, ChatError> {
+    pub async fn run_hooks(
+        &mut self,
+        trigger: HookTrigger,
+        output: &mut impl Write,
+    ) -> Result<Vec<(Hook, String)>, ChatError> {
         let mut hooks: Vec<&Hook> = Vec::new();
 
         // Set internal hook states
@@ -583,10 +588,14 @@ impl ContextManager {
         ];
 
         for (hook_list, is_global) in configs {
-            hooks.extend(hook_list.iter_mut().map(|(name, h)| {
-                h.name = name.clone();
-                h.is_global = is_global;
-                &*h
+            hooks.extend(hook_list.iter_mut().filter_map(|(name, h)| {
+                if h.trigger == trigger {
+                    h.name = name.clone();
+                    h.is_global = is_global;
+                    Some(&*h)
+                } else {
+                    None
+                }
             }));
         }
 

crates/chat-cli/src/cli/chat/conversation.rs

@@ -324,19 +324,20 @@ impl ConversationState {
     /// Returns a [FigConversationState] capable of being sent by [api_client::StreamingClient].
     ///
     /// Params:
-    /// - `run_hooks` - whether hooks should be executed and included as context
+    /// - `run_perprompt_hooks` - whether per-prompt hooks should be executed and included as
+    ///   context
     pub async fn as_sendable_conversation_state(
         &mut self,
         os: &Os,
         stderr: &mut impl Write,
-        run_hooks: bool,
+        run_perprompt_hooks: bool,
     ) -> Result<FigConversationState, ChatError> {
         debug_assert!(self.next_message.is_some());
         self.enforce_conversation_invariants();
         self.history.drain(self.valid_history_range.1..);
         self.history.drain(..self.valid_history_range.0);
 
-        let context = self.backend_conversation_state(os, run_hooks, stderr).await?;
+        let context = self.backend_conversation_state(os, run_perprompt_hooks, stderr).await?;
         if !context.dropped_context_files.is_empty() {
             execute!(
                 stderr,
@@ -390,21 +391,22 @@ impl ConversationState {
     pub async fn backend_conversation_state(
         &mut self,
         os: &Os,
-        run_hooks: bool,
+        run_perprompt_hooks: bool,
         output: &mut impl Write,
     ) -> Result<BackendConversationState<'_>, ChatError> {
         self.update_state(false).await;
         self.enforce_conversation_invariants();
 
-        // Run hooks and add to conversation start and next user message.
         let mut conversation_start_context = None;
-        if let (true, Some(cm)) = (run_hooks, self.context_manager.as_mut()) {
-            let hook_results = cm.run_hooks(output).await?;
-            conversation_start_context = Some(format_hook_context(hook_results.iter(), HookTrigger::ConversationStart));
-
-            // add per prompt content to next_user_message if available
-            if let Some(next_message) = self.next_message.as_mut() {
-                next_message.additional_context = format_hook_context(hook_results.iter(), HookTrigger::PerPrompt);
+        if let Some(cm) = self.context_manager.as_mut() {
+            let conv_start = cm.run_hooks(HookTrigger::ConversationStart, output).await?;
+            conversation_start_context = format_hook_context(&conv_start, HookTrigger::ConversationStart);
+
+            if let (true, Some(next_message)) = (run_perprompt_hooks, self.next_message.as_mut()) {
+                let per_prompt = cm.run_hooks(HookTrigger::PerPrompt, output).await?;
+                if let Some(ctx) = format_hook_context(&per_prompt, HookTrigger::PerPrompt) {
+                    next_message.additional_context = ctx;
+                }
             }
         }
 
@@ -756,7 +758,17 @@ impl From<InputSchema> for ToolInputSchema {
     }
 }
 
-fn format_hook_context<'a>(hook_results: impl IntoIterator<Item = &'a (Hook, String)>, trigger: HookTrigger) -> String {
+/// Formats hook output to be used within context blocks (e.g., in context messages or in new user
+/// prompts).
+///
+/// # Returns
+/// [Option::Some] if `hook_results` is not empty and at least one hook has content. Otherwise,
+/// [Option::None]
+fn format_hook_context(hook_results: &[(Hook, String)], trigger: HookTrigger) -> Option<String> {
+    if hook_results.iter().all(|(_, content)| content.is_empty()) {
+        return None;
+    }
+
     let mut context_content = String::new();
 
     context_content.push_str(CONTEXT_ENTRY_START_HEADER);
@@ -766,11 +778,11 @@ fn format_hook_context<'a>(hook_results: impl IntoIterator<Item = &'a (Hook, Str
     }
     context_content.push_str("\n\n");
 
-    for (hook, output) in hook_results.into_iter().filter(|(h, _)| h.trigger == trigger) {
+    for (hook, output) in hook_results.iter().filter(|(h, _)| h.trigger == trigger) {
         context_content.push_str(&format!("'{}': {output}\n\n", &hook.name));
     }
     context_content.push_str(CONTEXT_ENTRY_END_HEADER);
-    context_content
+    Some(context_content)
 }
 
 fn enforce_conversation_invariants(