Skip to content

Remove orphaned tool calls before compaction #5059

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
katzdave merged 3 commits into from
Oct 14, 2025
Merged

Remove orphaned tool calls before compaction #5059

katzdave merged 3 commits into from
Oct 14, 2025

Conversation

@katzdave
Copy link
Collaborator

@katzdave katzdave commented Oct 7, 2025

Remove orphaned tool calls before compaction. Also add in fix_conversation to catch other potential conversation errors.

This PR is based on #4968 but adapted for the main branch.

Changes

  • Modified fix_conversation to handle agent_visible and non-visible messages separately
  • Only agent-visible messages are fixed, while non-visible messages are preserved
  • Updated fix_lead_trail to have a more specific error message for trailing assistant messages with pending tool requests
  • Added two new tests: test_last_assistant_message_with_pending_tool_request and test_last_assistant_message_with_multiple_pending_tool_requests

Testing

  • All conversation tests pass (30/30)
  • All auto-compact tests pass (9/9)
  • Code formatted with cargo fmt
  • All clippy checks pass

@katzdave
Remove orphaned tool calls before compaction
24fe252 
- Add fix_conversation to handle agent_visible vs user_visible messages separately
- Update fix_lead_trail to have more specific error message for trailing assistant messages
- Add two new tests for handling pending tool requests in last assistant message
- Conversation fixes now only apply to agent_visible messages, preserving non-visible ones
DOsinga
DOsinga approved these changes Oct 11, 2025
View reviewed changes
Copy link
Collaborator

@DOsinga DOsinga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

de LLM-ify it

crates/goose/src/conversation/mod.rs Outdated
let messages = conversation.messages().clone();
let (messages, issues) = fix_messages(messages);
(Conversation::new_unvalidated(messages), issues)
let all_messages = conversation.messages().clone();
Copy link
Collaborator

@DOsinga DOsinga Oct 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont think you need to clone here

crates/goose/src/conversation/mod.rs Outdated
/// messages should always be from the user.
///
/// This function handles agent_visible and non-visible messages separately to ensure
/// that only agent-visible messages are fixed, while preserving non-visible messages.
Copy link
Collaborator

@DOsinga DOsinga Oct 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here and below, let's cut these comments down

crates/goose/src/conversation/mod.rs
}
}

#[test]
Copy link
Collaborator

@DOsinga DOsinga Oct 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these tests don't test the new functionality, replace them with something that does

Copy link
Collaborator Author

@katzdave katzdave Oct 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

crates/goose/src/conversation/mod.rs Outdated
messages.pop();
issues.push("Removed trailing assistant message".to_string());
issues
.push("Removed trailing assistant message with pending tool requests".to_string());
Copy link
Collaborator

@DOsinga DOsinga Oct 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

huh?

Copy link
Collaborator Author

@katzdave katzdave Oct 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

crates/goose/src/conversation/mod.rs Outdated
final_messages.extend(fixed_visible);

// Sort by timestamp to maintain order
final_messages.sort_by_key(|m| m.created);
Copy link
Collaborator

@DOsinga DOsinga Oct 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems scary; if we weren't sorted by created (and we don't always set these I think), we might now have broken the conversation. use a shadow map to keep order

Copy link
Collaborator Author

@katzdave katzdave Oct 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

crates/goose/src/conversation/mod.rs
} else {
non_visible_messages.push(msg);
}
}
Copy link
Collaborator

@DOsinga DOsinga Oct 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use partition

Copy link
Collaborator Author

@katzdave katzdave Oct 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@katzdave katzdave mentioned this pull request Oct 14, 2025
Closed
@katzdave katzdave merged commit 05882bc into main Oct 14, 2025
11 checks passed
@katzdave katzdave deleted the dkatz/main-orphan-tool-calls2 branch October 14, 2025 18:12
katzdave added a commit that referenced this pull request Oct 15, 2025
@katzdave
Merge branch 'main' of github.com:block/goose into error-proxy-2
79ba3f8 
* 'main' of github.com:block/goose: (49 commits)
  fixing video embed (#5171)
  chore: clean up random unused files (#5166)
  fix: adjust download_cli.sh to tolerate no OS variable (#5169)
  mcp tutorial page for firecrawl (#5152)
  Remove orphaned tool calls before compaction (#5059)
  feat: add copy as markdown button to documentation pages (#5158)
  chore: include vendored node executable (#5160)
  remove extra whitespace from message (#5159)
  Clear deeplinks after use (#5128)
  Revert "Fix gpt-5 input context limit (#4619)" (#5135)
  fix: missing cmake and protobuf for windows build, deduplicate sh/pws… (#5028)
  Fix bedrock tool input schema (#5064)
  Add self-test recipe for goose validation (#5111)
  fix: modifies openai request logic for reasoning models (#4221) (#4294)
  Fix race condition threat when set_param and set_secret of c… (#5109)
  Clean room implementation of the chat process (#5079)
  Bump rmcp (#5096)
  set version in an env variable for testing (#5100)
  fix : enhance fuzzy file search in goose desktop (#5071)
  Make async (#5126)
  ...
lifeizhou-ap added a commit that referenced this pull request Oct 15, 2025
@lifeizhou-ap
Merge branch 'main' into lifei/execute-recipe-server-side
9b429b7 
* main:
  fix: include apple silicon build of the desktop app in build artifacts (#5174)
  fixing video embed (#5171)
  chore: clean up random unused files (#5166)
  fix: adjust download_cli.sh to tolerate no OS variable (#5169)
  mcp tutorial page for firecrawl (#5152)
  Remove orphaned tool calls before compaction (#5059)
  feat: add copy as markdown button to documentation pages (#5158)
  chore: include vendored node executable (#5160)
  remove extra whitespace from message (#5159)
  Clear deeplinks after use (#5128)
  Revert "Fix gpt-5 input context limit (#4619)" (#5135)
  fix: missing cmake and protobuf for windows build, deduplicate sh/pws… (#5028)
tlongwell-block added a commit that referenced this pull request Oct 15, 2025
@tlongwell-block
Merge remote-tracking branch 'origin/main' into no_subagents_in_non_a…
ba52e3c 
…utonomous

* origin/main:
  feat: add Daily Standup Report Generator recipe (#5123)  (#5131)
  Sort providers in alphabetical vs random (#5090)
  Declarative providers (#5084)
  adding youtube link to firecrawl mcp tutorial, merge after 9am Eastern Oct 15 (#5173)
  Ollama integration: modified default model + added models  (#5153)
  Fix codex subagent configuration in documentation (#5180)
  fix: include apple silicon build of the desktop app in build artifacts (#5174)
  fixing video embed (#5171)
  chore: clean up random unused files (#5166)
  fix: adjust download_cli.sh to tolerate no OS variable (#5169)
  mcp tutorial page for firecrawl (#5152)
  Remove orphaned tool calls before compaction (#5059)
  feat: add copy as markdown button to documentation pages (#5158)
  chore: include vendored node executable (#5160)
  remove extra whitespace from message (#5159)
wpfleger96 added a commit to wpfleger96/goose that referenced this pull request Oct 15, 2025
@wpfleger96
Merge branch 'main' into wpfleger/standardize-session-attributes
d0b0c85 
* main: (55 commits)
  [docs] Add Blog Post: "Designing AI for Users, Not Just LLMs" (block#5190)
  docs: update cognee, jetbrains, mbot extensions config (block#5172)
  Minimally disable subagents when not in autonomous model (block#5149)
  Fix provider sort (block#5188)
  blog: Getting Started with Goose on Windows (block#5156)
  feat: add CI/CD Pipeline recipe (block#5183)
  feat: add Daily Standup Report Generator recipe (block#5123)  (block#5131)
  Sort providers in alphabetical vs random (block#5090)
  Declarative providers (block#5084)
  adding youtube link to firecrawl mcp tutorial, merge after 9am Eastern Oct 15 (block#5173)
  Ollama integration: modified default model + added models  (block#5153)
  Fix codex subagent configuration in documentation (block#5180)
  fix: include apple silicon build of the desktop app in build artifacts (block#5174)
  fixing video embed (block#5171)
  chore: clean up random unused files (block#5166)
  fix: adjust download_cli.sh to tolerate no OS variable (block#5169)
  mcp tutorial page for firecrawl (block#5152)
  Remove orphaned tool calls before compaction (block#5059)
  feat: add copy as markdown button to documentation pages (block#5158)
  chore: include vendored node executable (block#5160)
  ...
michaelneale added a commit that referenced this pull request Oct 16, 2025
@michaelneale
Merge branch 'main' into micn/goose-mobile-access
65f38bc 
* main: (35 commits)
  fix: include apple silicon build of the desktop app in build artifacts (#5174)
  fixing video embed (#5171)
  chore: clean up random unused files (#5166)
  fix: adjust download_cli.sh to tolerate no OS variable (#5169)
  mcp tutorial page for firecrawl (#5152)
  Remove orphaned tool calls before compaction (#5059)
  feat: add copy as markdown button to documentation pages (#5158)
  chore: include vendored node executable (#5160)
  remove extra whitespace from message (#5159)
  Clear deeplinks after use (#5128)
  Revert "Fix gpt-5 input context limit (#4619)" (#5135)
  fix: missing cmake and protobuf for windows build, deduplicate sh/pws… (#5028)
  Fix bedrock tool input schema (#5064)
  Add self-test recipe for goose validation (#5111)
  fix: modifies openai request logic for reasoning models (#4221) (#4294)
  Fix race condition threat when set_param and set_secret of c… (#5109)
  Clean room implementation of the chat process (#5079)
  Bump rmcp (#5096)
  set version in an env variable for testing (#5100)
  fix : enhance fuzzy file search in goose desktop (#5071)
  ...
@alexhancock alexhancock mentioned this pull request Oct 17, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DOsinga DOsinga DOsinga approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@katzdave @DOsinga