-
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
Conversation
DOsinga
left a comment
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.
left some preliminary comments. I think we should do something like this, but we need a structure that allows any platform tool this sort of thing. see: #4868 and the relevant discussion
| #[serial] | ||
| async fn test_what_is_your_name() -> Result<()> { | ||
| run_scenario( | ||
| std::env::set_var("GOOSE_MOIM_ENABLED", "false"); |
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?
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 serial
The 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
|
|
||
| // Inject MOIM (timestamp + TODO content) if enabled | ||
| let messages_for_provider = | ||
| super::moim::inject_moim_if_enabled(messages_for_provider, session).await; |
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.
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
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 put it in stream_response_from_provider because I wanted it to be at the very, very edge of the shared inference execution path. This is the last point on that execution path that doesn't split between different providers
This makes reasoning about it much easier. There's no chance to be written to a session file, be compacted, etc, etc
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.
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
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.
Seems like the dependency is there from how the TODO stool stores its state over actually needing the session.
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.
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
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.
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
Agree, yes. 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) |
117dee3 to
af70b3b
Compare
# Conflicts: # crates/goose/src/prompts/subagent_system.md # crates/goose/src/prompts/system_gpt_4.1.md
af70b3b to
1f7f5cf
Compare
| use crate::session::SessionManager; | ||
| use chrono::Local; | ||
|
|
||
| async fn build_moim_content(session: &Option<SessionConfig>) -> Option<String> { |
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'd really like to see this as something the tool itself can define rather than a specific global place.
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 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
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.
in light of the latest infra changes here's my proposal:
- add a method to the platform extension type get_moim() and implement that for todo
- add a method to extension_manager get_moim that goes through its platform extensions and collects them and then adds the current time
- call this method from reply_internal (yeah, I know that is scary)
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.
Working in #5027
|
|
||
| // Inject MOIM (timestamp + TODO content) if enabled | ||
| let messages_for_provider = | ||
| super::moim::inject_moim_if_enabled(messages_for_provider, session).await; |
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.
Seems like the dependency is there from how the TODO stool stores its state over actually needing the session.
Implements ephemeral context injection at the provider boundary to automatically include timestamp and TODO content in LLM context without modifying conversation history.
Problem
Previously, accessing TODO content required explicit
todo__readtool calls, cluttering conversation history and consuming tokens. Timestamps were injected via template variables in system prompts, creating inconsistency across different prompt variants and preventing effective prompt caching.Solution
MOIM (Minus One Info Message) injects a synthetic user message at approximately position -1 during provider calls. This message contains:
The injection happens in
reply_parts.rsat the provider boundary, ensuring the message is never persisted to storage or returned in conversation history.Architecture Benefits
Message,Conversation)Testing
Comprehensive test coverage in
crates/goose/tests/moim_tests.rswith proper test isolation usingserial_testfor tests that modify environment variables.Migration
todo__readtool (breaking change for recipes using it)todo__writedescription to reflect automatic availability