Skip to content

Conversation

@katzdave
Copy link
Collaborator

@katzdave katzdave commented Nov 4, 2025

Summary

Fix manual compaction triggering bug + add a smoke test.

Copilot AI review requested due to automatic review settings November 4, 2025 22:16
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds smoke tests for conversation compaction functionality and fixes a logic bug in the auto-compaction check. The changes introduce a test script to validate both manual and auto-compaction scenarios, integrate it into the CI workflow, and correct the compaction trigger logic.

  • Adds comprehensive test script for manual and auto-compaction validation
  • Fixes auto-compaction logic to properly skip when manual compaction is triggered
  • Integrates compaction tests into the GitHub Actions workflow

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
scripts/test_compaction.sh New smoke test script that validates manual compaction (via trigger prompt) and auto-compaction (via threshold), including JSON structure validation
crates/goose/src/agents/agent.rs Fixes logic bug by adding !is_manual_compact condition to auto-compaction check to prevent both from triggering simultaneously
.github/workflows/pr-smoke-test.yml Adds new compaction test step to the CI workflow with appropriate environment configuration
Comments suppressed due to low confidence (1)

crates/goose/src/agents/agent.rs:1

  • Consider adding a comment explaining why auto-compaction is skipped when manual compaction is triggered. This would clarify that the guard prevents both compaction types from running simultaneously and improve code maintainability.
use std::collections::HashMap;

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

# Tests both manual (trigger prompt) and auto compaction (threshold-based)

if [ -f .env ]; then
export $(grep -v '^#' .env | xargs)
Copy link

Copilot AI Nov 4, 2025

Choose a reason for hiding this comment

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

The export $(grep -v '^#' .env | xargs) command is vulnerable to command injection if the .env file contains malicious content. Consider using a safer approach like set -a; source .env; set +a or iterate through the file with proper validation of variable names and values.

Suggested change
export $(grep -v '^#' .env | xargs)
set -a; source .env; set +a

Copilot uses AI. Check for mistakes.
…est-and-fix

* 'main' of github.com:block/goose:
  improve linux tray icon support (#5425)
  feat: log rotation (#5561)
  use app.isPackaged instead of checking for node env development (#5465)
  disable RPM build-ID generation to prevent package conflicts (#5563)
  Add Diagnostics Info to Q&A and Bug Report Templates (#5565)
  fix: improve server error messages to include HTTP status code (#5532)
  improvement: add useful error message when attempting to use unauthenticated cursor-agent (#5300)
  fix: unblock acp via databricks (#5562)
  feat: add --output-format json flag to goose run command (#5525)
@katzdave katzdave changed the title Dkatz/compaction test and fix Manual compaction test and fix Nov 4, 2025
…est-and-fix

* 'main' of github.com:block/goose:
  Document missing recipe param types (#5584)
  docs: description required for "Add Extension" in cli (#5573)
  fix: Add schema-aware numeric coercion for MCP tool arguments (#5478)
  Add uv for uvx in Justfile (#5581)
  Keep llm logs in place (#5577)
  bump to 1.12.0 (#5580)
  automate more of the release process (#5409)
  add clippy warning for string_slice (#5422)
Copilot AI review requested due to automatic review settings November 5, 2025 18:36
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.


Ok(Box::pin(async_stream::try_stream! {
let final_conversation = if !needs_auto_compact {
let final_conversation = if !needs_auto_compact && !is_manual_compact {
Copy link

Copilot AI Nov 5, 2025

Choose a reason for hiding this comment

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

The condition !needs_auto_compact && !is_manual_compact is logically equivalent to !(needs_auto_compact || is_manual_compact), which can be simplified. However, given that needs_auto_compact already incorporates the !is_manual_compact check (line 753), this condition could be simplified to just !needs_auto_compact since when is_manual_compact is true, needs_auto_compact will always be false.

Suggested change
let final_conversation = if !needs_auto_compact && !is_manual_compact {
let final_conversation = if !needs_auto_compact {

Copilot uses AI. Check for mistakes.
@alexhancock alexhancock self-requested a review November 6, 2025 14:54
@katzdave katzdave merged commit eb29083 into main Nov 6, 2025
22 checks passed
katzdave added a commit that referenced this pull request Nov 6, 2025
jamadeo pushed a commit that referenced this pull request Nov 6, 2025
wpfleger96 added a commit that referenced this pull request Nov 6, 2025
* main: (60 commits)
  fix: add standard context menu items to prevent empty right-click menu (#5616)
  Bump openapi in prepare-release (#5611)
  docs: add access control section to Developer tutorial (#5615)
  Token state not showing on load, or after message is finished. (#5606)
  Change the other location too (#5608)
  feat(ui): bring back quick launcher (#5144)
  Support platform tools through CLI (#5570)
  Avoid web double write (#5601)
  fix: gemini flash -> pro for mcp smoke tests (#5574)
  Manual compaction test and fix (#5568)
  fix: tidy up claude cli handling (#5594)
  Remove jetbrains (#5602)
  feat(githubcopilot): add support for newer Copilot AI Models (#5603)
  fix: customised recipe to yaml string to avoid minininjia parsing error (#5494)
  Add pending extension indicator to extension panel (#5493)
  Add environment subsition for auth blocks (#5439)
  acp: ToolCallLocations and working cancellation (#5588)
  feat(providers): add Mistral AI provider (#5009)
  Listen for ctrl-c during provider request (#5585)
  Also accept null as description, not just missing (#5589)
  ...
wpfleger96 added a commit that referenced this pull request Nov 6, 2025
* main: (31 commits)
  Standardize CLI argument flags and update documentation (#5516)
  Release 1.13.0
  fix: move goosehints/AGENTS.md handling to goose, and out of developer extension (#5575)
  fix: add standard context menu items to prevent empty right-click menu (#5616)
  Bump openapi in prepare-release (#5611)
  docs: add access control section to Developer tutorial (#5615)
  Token state not showing on load, or after message is finished. (#5606)
  Change the other location too (#5608)
  feat(ui): bring back quick launcher (#5144)
  Support platform tools through CLI (#5570)
  Avoid web double write (#5601)
  fix: gemini flash -> pro for mcp smoke tests (#5574)
  Manual compaction test and fix (#5568)
  fix: tidy up claude cli handling (#5594)
  Remove jetbrains (#5602)
  feat(githubcopilot): add support for newer Copilot AI Models (#5603)
  fix: customised recipe to yaml string to avoid minininjia parsing error (#5494)
  Add pending extension indicator to extension panel (#5493)
  Add environment subsition for auth blocks (#5439)
  acp: ToolCallLocations and working cancellation (#5588)
  ...
michaelneale added a commit that referenced this pull request Nov 7, 2025
* main: (21 commits)
  differentiate debug/release in cache key (#5613)
  Unify subrecipe and subagent execution through shared recipe pipeline (#5082)
  Standardize CLI argument flags and update documentation (#5516)
  Release 1.13.0
  fix: move goosehints/AGENTS.md handling to goose, and out of developer extension (#5575)
  fix: add standard context menu items to prevent empty right-click menu (#5616)
  Bump openapi in prepare-release (#5611)
  docs: add access control section to Developer tutorial (#5615)
  Token state not showing on load, or after message is finished. (#5606)
  Change the other location too (#5608)
  feat(ui): bring back quick launcher (#5144)
  Support platform tools through CLI (#5570)
  Avoid web double write (#5601)
  fix: gemini flash -> pro for mcp smoke tests (#5574)
  Manual compaction test and fix (#5568)
  fix: tidy up claude cli handling (#5594)
  Remove jetbrains (#5602)
  feat(githubcopilot): add support for newer Copilot AI Models (#5603)
  fix: customised recipe to yaml string to avoid minininjia parsing error (#5494)
  Add pending extension indicator to extension panel (#5493)
  ...
fbalicchia pushed a commit to fbalicchia/goose that referenced this pull request Nov 7, 2025
Surendhar-N-D pushed a commit to Surendhar-N-D/goose that referenced this pull request Nov 17, 2025
arul-cc pushed a commit to arul-cc/goose that referenced this pull request Nov 17, 2025
BlairAllan pushed a commit to BlairAllan/goose that referenced this pull request Nov 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants