From e8025bd850979e4791c431fa8d0cdfc78555b422 Mon Sep 17 00:00:00 2001 From: Douwe Osinga Date: Thu, 26 Mar 2026 13:22:30 -0400 Subject: [PATCH] fix: address codex review findings and remove redundant comments - Fix P2: auto-compaction path now correctly passes manual=false to PreCompact/PostCompact hooks (was incorrectly passing true) - Fix P1: capture user prompt text before SessionStart hooks fire, preventing UserPromptSubmit from receiving injected context instead of the actual user input - Remove comments that restate what the code does Signed-off-by: Douwe Osinga --- crates/goose/src/agents/agent.rs | 29 ++++++++------------- crates/goose/src/agents/execute_commands.rs | 2 -- crates/goose/src/agents/tool_execution.rs | 7 ----- crates/goose/src/hooks/mod.rs | 2 -- 4 files changed, 11 insertions(+), 29 deletions(-) diff --git a/crates/goose/src/agents/agent.rs b/crates/goose/src/agents/agent.rs index 3114e4460d..6db1a213fc 100644 --- a/crates/goose/src/agents/agent.rs +++ b/crates/goose/src/agents/agent.rs @@ -426,7 +426,6 @@ impl Agent { // Handle pre-approved and read-only tools for request in &permission_check_result.approved { if let Ok(tool_call) = request.tool_call.clone() { - // Fire PreToolUse hook — if blocked, create error response let invocation = crate::hooks::HookInvocation::pre_tool_use( session.id.clone(), tool_call.name.to_string(), @@ -1123,7 +1122,6 @@ impl Agent { .clone() .ok_or_else(|| anyhow::anyhow!("Session {} has no conversation", session_config.id))?; - // Load hooks configuration let hooks = Hooks::load(&session.working_dir, self.hook_context_fill_state.clone()); let needs_auto_compact = check_if_compaction_needed( @@ -1165,11 +1163,10 @@ impl Agent { ) ); - // Fire PreCompact hook let invocation = crate::hooks::HookInvocation::pre_compact( session_config.id.clone(), conversation_to_compact.messages().len(), - true, // manual = true for explicit compaction + false, session.working_dir.to_string_lossy().to_string(), ); let _ = hooks @@ -1200,7 +1197,7 @@ impl Agent { &session_config.id, pre_compact_len, post_compact_len, - true, + false, &session.working_dir, &session_manager, &mut compacted_conversation, @@ -1277,7 +1274,14 @@ impl Agent { let working_dir = session.working_dir.clone(); - // Fire SessionStart hook on first reply only (new session: exactly 1 user message, no assistant response yet) + let user_prompt = conversation + .messages() + .iter() + .rev() + .find(|m| m.role == rmcp::model::Role::User) + .map(|m| m.as_concat_text()) + .unwrap_or_default(); + if conversation.messages().len() == 1 && conversation.messages()[0].role == rmcp::model::Role::User { @@ -1307,14 +1311,7 @@ impl Agent { } } - // Fire UserPromptSubmit hook with the last user message - if let Some(last_user_msg) = conversation - .messages() - .iter() - .rev() - .find(|m| m.role == rmcp::model::Role::User) - { - let user_prompt = last_user_msg.as_concat_text(); + if !user_prompt.is_empty() { let invocation = crate::hooks::HookInvocation::user_prompt_submit( session_id.clone(), user_prompt, @@ -1590,7 +1587,6 @@ impl Agent { Some((request_id, item)) => { match item { ToolStreamItem::Result(output) => { - // Fire PostToolUse or PostToolUseFailure hooks if let Some(original_request) = request_id_to_request.get(&request_id) { if let Ok(ref tool_call) = original_request.tool_call { let tool_input = serde_json::to_value(&tool_call.arguments) @@ -1796,7 +1792,6 @@ impl Agent { ) ); - // Fire PreCompact hook (recovery compaction) let invocation = crate::hooks::HookInvocation::pre_compact( session_config.id.clone(), conversation.messages().len(), @@ -2013,7 +2008,6 @@ impl Agent { tokio::task::yield_now().await; } - // Fire Stop hook before finishing let invocation = crate::hooks::HookInvocation::stop( session_id.clone(), None, // reason @@ -2028,7 +2022,6 @@ impl Agent { ) .await; - // Fire SessionEnd hook after stop let invocation = crate::hooks::HookInvocation::session_end( session_id.clone(), None, diff --git a/crates/goose/src/agents/execute_commands.rs b/crates/goose/src/agents/execute_commands.rs index a520dd7424..dd10164101 100644 --- a/crates/goose/src/agents/execute_commands.rs +++ b/crates/goose/src/agents/execute_commands.rs @@ -89,7 +89,6 @@ impl Agent { .conversation .ok_or_else(|| anyhow!("Session has no conversation"))?; - // Load hooks and fire PreCompact let hooks = Hooks::load(&session.working_dir, self.hook_context_fill_state.clone()); let invocation = crate::hooks::HookInvocation::pre_compact( session_id.to_string(), @@ -123,7 +122,6 @@ impl Agent { self.update_session_metrics(session_id, session.schedule_id, &usage, true) .await?; - // Fire PostCompact hook and inject context if any. // The helper also pushes to the in-memory conversation, but we don't need // that here — session persistence happens via session_manager.add_message() // inside the helper, and this conversation is about to be dropped. diff --git a/crates/goose/src/agents/tool_execution.rs b/crates/goose/src/agents/tool_execution.rs index 5a967f874f..4ad48da6c8 100644 --- a/crates/goose/src/agents/tool_execution.rs +++ b/crates/goose/src/agents/tool_execution.rs @@ -91,7 +91,6 @@ impl Agent { try_stream! { for request in tool_requests.iter() { if let Ok(tool_call) = request.tool_call.clone() { - // Find the corresponding inspection result for this tool request let security_message = inspection_results.iter() .find(|result| result.tool_request_id == request.id) .and_then(|result| { @@ -117,7 +116,6 @@ impl Agent { let confirmation = confirmation_rx.await .map_err(|_| anyhow::anyhow!("Confirmation channel closed for request {}", request.id))?; - // Log user decision if this was a security alert if let Some(finding_id) = get_security_finding_id_from_results(&request.id, inspection_results) { tracing::info!( monotonic_counter.goose.prompt_injection_user_decisions = 1, @@ -129,7 +127,6 @@ impl Agent { } if confirmation.permission == Permission::AllowOnce || confirmation.permission == Permission::AlwaysAllow { - // Fire PreToolUse hook — if blocked, treat as declined let invocation = crate::hooks::HookInvocation::pre_tool_use( session.id.clone(), tool_call.name.to_string(), @@ -147,7 +144,6 @@ impl Agent { .await .unwrap_or_default(); if outcome.blocked { - // Hook blocked — fill declined response and continue to next tool if let Some(response_msg) = request_to_response_map.get(&request.id) { let mut response = response_msg.lock().await; *response = response.clone().with_tool_response_with_metadata( @@ -177,14 +173,12 @@ impl Agent { ), })); - // Update the shared permission manager when user selects "Always Allow" if confirmation.permission == Permission::AlwaysAllow { self.tool_inspection_manager .update_permission_manager(&tool_call.name, PermissionLevel::AlwaysAllow) .await; } } else { - // User declined - update the specific response message for this request if let Some(response_msg) = request_to_response_map.get(&request.id) { let mut response = response_msg.lock().await; *response = response.clone().with_tool_response_with_metadata( @@ -213,7 +207,6 @@ impl Agent { try_stream! { if let Ok(tool_call) = tool_request.tool_call.clone() { if self.is_frontend_tool(&tool_call.name).await { - // Send frontend tool request and wait for response yield Message::assistant().with_frontend_tool_request( tool_request.id.clone(), Ok(tool_call.clone()) diff --git a/crates/goose/src/hooks/mod.rs b/crates/goose/src/hooks/mod.rs index ce9e11df4c..c8b2960151 100644 --- a/crates/goose/src/hooks/mod.rs +++ b/crates/goose/src/hooks/mod.rs @@ -55,7 +55,6 @@ impl Hooks { let fill_pct = ((current_tokens as f64 / context_limit as f64) * 100.0) as u32; - // Get configured ContextFill thresholds from settings let event_configs = self .settings .get_hooks_for_event(HookEventKind::ContextFill); @@ -63,7 +62,6 @@ impl Hooks { return None; } - // Find thresholds that are newly crossed let mut new_thresholds = Vec::new(); { let mut fired = self