mirror of
https://github.com/aaif-goose/goose.git
synced 2026-07-03 14:10:03 +02:00
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 <douwe@squareup.com>
This commit is contained in:
Douwe Osinga
@@ -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,
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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())
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user