-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Replace compaction notifications with system notifications #5218
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.
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
Conversation
* 'main' of github.com:block/goose: Break compaction back into check_ and do_ compaction (#5212) fix: revert built app name to uppercase Goose (#5206) feat: add Code Documentation Generator recipe (#5121) (#5125) Revert "feat: enhance goose to search sessions for easy recall (#5177)" (#5209)
* 'main' of github.com:block/goose: Rewrite extension management tools (#5057) fix: re-sync package-lock.json (#5235) docs: Hacktoberfest MCP youtube short entry to community-content.json (#5150) feat: add schedule button to recipe entries (#5217) Autocompact threshold UI cleanup (#5232) fix: correct schema for openai tools (#5229)
* 'main' of github.com:block/goose: docs: provide more clarity in tagging step of releases (#5269) docs: add GOOSE_DEBUG environment variable (#5265) Lifei/UI parameter input (#5222) turn off WAL (#5203) Skip subagents for gemini (#5257) Revert "Standardize Session Name Attribute" (#5250) improve provider request logging a bit (#5236) Fix OpenAI empty choices panic (#5248) Revert "Rewrite extension management tools" (#5243) Standardize Session Name Attribute (#5085) Docs: Include step-by-step model configuration instructions for first… (#5239) Delete message visibility filter (#5216) delete flaky pricing integration tests (#5207) chore: upgrade most npm packages to latest (#5185) Release/1.11.0 (#5224)
|
Hmm actually what I've found is using the REST for manual compaction seems to create a lot of friction; needed a way on the client to control the thinking state. Having it send a stream message instead, which puts it in the agent loop so it can share a closer code path with auto-compact + out of context compact. |
* 'main' of github.com:block/goose: fix: include Windows GNU CLI artifact in releases (#5276) Fix bedrock tool call error response (#5067) fix: do not load active extensions when no extensions in the recipe (#5282) Fix pagination - fetch all open issues, not just first 100 Fix label case sensitivity - use lowercase labels Add GitHub Health Dashboard workflow (#5286) live testing script (#5263) escaping markdown fixing syntax Add GitHub Health Dashboard workflow Fix extension manager resource deadlock (#5066) add new prompt for memory extension (#4945) feat: Stable system prompt and tool ordering for more cache hits (#5192) Remove gtag analytics error (#5268)
|
UX question: does it work to type "Please compact this conversation" the same way it does if the message is auto-inserted by clicking the button? If it could, I actually don't even think of that as an issue for the UX compared to now, and opens up a nice way to tell it to compact without taking hands off keyboard. |
| session_metadata.as_ref(), | ||
| ) | ||
| .await; | ||
| let is_manual_compact = unfixed_conversation.messages().last().is_some_and(|msg| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems rather convoluted; I think it is because you want the manual compaction to use the same stream as we do for the agent reply? what was wrong with the old way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The old way triggered from REST vs a stream. It just made it a lot more complicated to keep the client in the right 'thinking' state. It also sent the client's view of the messages and used that which would cause a bit of rendering jitter when we did history replaced.
Could maybe split these more, but I think it's generally fine, main differences here are messaging + whether or not we continue the stream.
crates/goose/src/agents/agent.rs
Outdated
| yield AgentEvent::Message( | ||
| Message::assistant().with_system_notification( | ||
| SystemNotificationType::InlineMessage, | ||
| "Context limit reached. Attempting to compact and continue conversation...", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove attempt, or even do we need two messages here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the system notification is still nice to render, because you'll see that. Removing attempt; it should confidently succeed!
| final_token_counts.push(compaction_marker_tokens); | ||
|
|
||
| // Add the summary message (agent_visible=true, user_visible=false) | ||
| let summary_msg = summary_message.with_metadata(MessageMetadata::agent_only()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we still need this block below? I thought we had already deleted that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The assistant_message? The summary is a user message historically; I think we could make it an agent one and then we don't need this. Just don't want the conversation to end in a user message unless we're trying to continue.
| assert!(metadata.agent_visible); | ||
| } | ||
|
|
||
| #[test] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| @@ -0,0 +1,20 @@ | |||
| import { Message } from '../api'; | |||
| import { ChatState } from '../types/chatState'; | |||
| export function getThinkingMessage(messages: Message[], chatState: ChatState): string | undefined { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should live in messages.ts. it should probably not take the chatState at all though, thinking is a property of the messages. in fact since when you call this you already check on message count and chat state, it could just be a property on a single message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| </Button> | ||
| </div> | ||
| ) : filteredMessages?.length > 0 ? ( | ||
| <ContextManagerProvider> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hurray! I think this causes rerenders
|
|
||
| const hasThinkingMessage = newMessage.content.some( | ||
| (content) => | ||
| content.type === 'systemNotification' && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this pattern appears in a bunch of places; as suggested below, let's just create a method in messages.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Yeah it does. I considered /compact to mirror the cli but that didn't look as nice. I think a future feature that could be cool is running a fast model on user messages to determine specific intents such as compaction or recipe generation. |
* 'main' of github.com:block/goose: Fix gemini again (#5308) fix: synchronize local message state after conversation compaction (#5315) docs: replace broken links with working links (#5266) Add Web Accessibility Auditor recipe to cookbook (#5318) To do mcp tutorial (#5317) workflows: add a manual trigger option to pr-smoke-test (#5302) documenting `goose recipe list` command (#5278)
Resolved merge conflicts in generated OpenAPI files by regenerating with just generate-openapi. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
…message # By Better-Boy (3) and others # Via GitHub * 'main' of github.com:block/goose: Create recipe dir on save (#5337) docs: Update Discord link (#5335) [recipe workflow]: Fix `Invalid revision range` error (#5334) Add tech-article-explainer recipe (#5333) doc: added beta banner for old blog post (#5332) feat: add code refactor recipe (#5320) Security audit recipe (#5319) feat: add generate commit message recipe (#5326) fix: remove dependency on gsap library (#5330) feat: dynamically load ollama models (#5309) fix: skip temperature for goose-gpt-5 model (#5311) Replace compaction notifications with system notifications (#5218) # Conflicts: # crates/goose/src/agents/agent.rs
* main: (77 commits) Fix legacy import (#5343) Unify loading goose messages and usechatstream determines chat state (#5306) Docs: goose session export and goose session import (#5267) Create recipe dir on save (#5337) docs: Update Discord link (#5335) [recipe workflow]: Fix `Invalid revision range` error (#5334) Add tech-article-explainer recipe (#5333) doc: added beta banner for old blog post (#5332) feat: add code refactor recipe (#5320) Security audit recipe (#5319) feat: add generate commit message recipe (#5326) fix: remove dependency on gsap library (#5330) feat: dynamically load ollama models (#5309) fix: skip temperature for goose-gpt-5 model (#5311) Replace compaction notifications with system notifications (#5218) Diagnostics (#5323) Fix gemini again (#5308) fix: synchronize local message state after conversation compaction (#5315) docs: replace broken links with working links (#5266) Add Web Accessibility Auditor recipe to cookbook (#5318) ...
Signed-off-by: Blair Allan <[email protected]>
Signed-off-by: Blair Allan <[email protected]>
Replace compaction notifications with more general system notifications. Deletes a bunch of compaction specific state in the client + the REST route for manual compaction.
The client sends a manual compact trigger message in the stream that the agent detects + processes. This makes the auto-compact + manual compact code nearly identical on the backend, with just some differences in the messaging + whether or not we resume the conversation.
The only downside with this approach is the compaction message shows up in the chat + message stream but I think we could avoid that in a follow up with some kind of way to inject additional metadata.
This also adds two types of SystemNotifications
compaction_system_notify.mp4