-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Add MOIM (Minus One Info Message) for automatic context injection (TODO list and current time) #4833
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
Add MOIM (Minus One Info Message) for automatic context injection (TODO list and current time) #4833
Changes from all commits
d619a6e
0c2f8f8
6b98395
9f3ed06
c4b9ef8
632936b
57d130e
6879b5e
d78164f
d5e1b98
7ba5141
1297c94
56fd710
6e1ddb9
61739c3
1f7f5cf
d220120
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,94 @@ | ||
| use crate::agents::types::SessionConfig; | ||
| use crate::conversation::message::Message; | ||
| use crate::conversation::Conversation; | ||
| use crate::session::extension_data::{ExtensionState, TodoState}; | ||
| use crate::session::SessionManager; | ||
| use chrono::Local; | ||
|
|
||
| async fn build_moim_content(session: &Option<SessionConfig>) -> Option<String> { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd really like to see this as something the tool itself can define rather than a specific global place.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I talked about my thinking in a reply above: "... I think adding to build_moim_content() is the way to go here since there will probably be formatting and helpers involved when moving platform tool content from its storage into the MOIM (ie an array of sources is probably not sufficient, so using the build function is probably the best way forward)" Having each tool register itself to a moim system would work, too, but we might still want to format how they fit together, which requires a global function like this one also, this global function allows us to put data in the moim that isn't related to a tool. Like the timestamp here. This could also be environment information or other non-tool context that we find useful for the agent
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in light of the latest infra changes here's my proposal:
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Working in #5027 |
||
| let timestamp = Local::now().format("%Y-%m-%d %H:%M:%S").to_string(); | ||
| let mut content = format!("Current date and time: {}\n", timestamp); | ||
|
|
||
| if let Some(todo_content) = get_todo_context(session).await { | ||
| content.push_str("\nCurrent tasks and notes:\n"); | ||
| content.push_str(&todo_content); | ||
| content.push('\n'); | ||
| } | ||
|
|
||
| Some(content) | ||
| } | ||
|
|
||
| /// Find a safe insertion point for MOIM. | ||
| /// | ||
| /// We want to insert as close to the end as possible, but we must avoid | ||
| /// breaking tool call/response pairs. We check if inserting at a position | ||
| /// would separate a tool call from its response. | ||
| pub fn find_safe_insertion_point(messages: &[Message]) -> usize { | ||
| if messages.is_empty() { | ||
| return 0; | ||
| } | ||
|
|
||
| // Default to inserting before the last message | ||
| let last_pos = messages.len() - 1; | ||
|
|
||
| // Check if inserting at last_pos would break a tool pair | ||
| if last_pos > 0 { | ||
| let prev_msg = &messages[last_pos - 1]; | ||
| let curr_msg = &messages[last_pos]; | ||
|
|
||
| // If previous message has tool calls and current has matching responses, | ||
| // we can't insert between them | ||
| if prev_msg.is_tool_call() && curr_msg.is_tool_response() { | ||
| // Find the next best position (before the tool call) | ||
| return last_pos.saturating_sub(1); | ||
| } | ||
| } | ||
|
|
||
| last_pos | ||
| } | ||
|
|
||
| pub async fn inject_moim_if_enabled( | ||
| messages_for_provider: Conversation, | ||
| session: &Option<SessionConfig>, | ||
| ) -> Conversation { | ||
| // Check if MOIM is enabled | ||
| let moim_enabled = crate::config::Config::global() | ||
| .get_param::<bool>("goose_moim_enabled") | ||
| .unwrap_or(true); | ||
|
|
||
| if !moim_enabled { | ||
| return messages_for_provider; | ||
| } | ||
|
|
||
| if let Some(moim_content) = build_moim_content(session).await { | ||
| let mut msgs = messages_for_provider.messages().to_vec(); | ||
| let moim_msg = Message::user().with_text(moim_content); | ||
|
|
||
| if msgs.is_empty() { | ||
| // If conversation is empty, just add the MOIM | ||
| msgs.push(moim_msg); | ||
| } else { | ||
| // Find a safe position that won't break tool call/response pairs | ||
| let insert_pos = find_safe_insertion_point(&msgs); | ||
| msgs.insert(insert_pos, moim_msg); | ||
| } | ||
|
|
||
| Conversation::new_unvalidated(msgs) | ||
| } else { | ||
| messages_for_provider | ||
| } | ||
| } | ||
|
|
||
| async fn get_todo_context(session: &Option<SessionConfig>) -> Option<String> { | ||
| let session_config = session.as_ref()?; | ||
|
|
||
| match SessionManager::get_session(&session_config.id, false).await { | ||
| Ok(session_data) => TodoState::from_extension_data(&session_data.extension_data) | ||
| .map(|state| state.content) | ||
| .filter(|content| !content.trim().is_empty()), | ||
| Err(e) => { | ||
| tracing::debug!("Could not read session for MOIM: {}", e); | ||
| None | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -134,6 +134,7 @@ impl Agent { | |
| messages: &[Message], | ||
| tools: &[Tool], | ||
| toolshim_tools: &[Tool], | ||
| session: &Option<crate::agents::types::SessionConfig>, | ||
| ) -> Result<MessageStream, ProviderError> { | ||
| let config = provider.get_model_config(); | ||
|
|
||
|
|
@@ -144,6 +145,10 @@ impl Agent { | |
| Conversation::new_unvalidated(messages.to_vec()) | ||
| }; | ||
|
|
||
| // Inject MOIM (timestamp + TODO content) if enabled | ||
| let messages_for_provider = | ||
| super::moim::inject_moim_if_enabled(messages_for_provider, session).await; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you should do this in agent.reply. there you can stick it into the block that already checks for session and make session non optional
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I put it in This makes reasoning about it much easier. There's no chance to be written to a session file, be compacted, etc, etc
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As far as session optionality goes: it should 100% be required, but we can't do it yet. Once recipes/subagents/scheduler are on Agent Manager, they will have real sessions. But, until then, stream_response_from_provider will be called without a session when dealing with recipes/subagents
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems like the dependency is there from how the TODO stool stores its state over actually needing the session.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. Instead of passing the session to inject_moim_if_enabled, I could pass TodoState::from_extension_data directly? It just ended up looking cleaner this way
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, no, lets keep passing the session in because we'll probably want to add more to moim from the session metadata later at some point. And its neater |
||
|
|
||
| // Clone owned data to move into the async stream | ||
| let system_prompt = system_prompt.to_owned(); | ||
| let tools = tools.to_owned(); | ||
|
|
||
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.
don't set environment variables in tests like this; if they fail you now changed the settings for the next test. also if you are going to add this to all run_scenario, maybe handle this there?
Uh oh!
There was an error while loading. Please reload this page.
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 have the tests in serial now. I set the env var at the top of the test and unset at the bottom. It won't effect other tests that way
I think you're right that I could get the same effect by un/setting it in run_scenario(), but still making sure that everything that calls run_scenario() is
serialThe reason I didn't is just because I can't internally guarantee that run_scenario will be in serial, hence it would be possible for an environment race condition to occur if called by a non-serial test