-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Nest TODO State in session data #4361
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
Merged
Merged
Changes from all commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
816dfa0
feat: simplify and improve context management UX
alexhancock 3a27338
fix: ensure all provider errors trigger retry UI
katzdave 55a4343
feat: only auto-continue the conversation for non-manual compaction
alexhancock 5092a10
render summarization requests
katzdave 7878408
first pass
katzdave b1fcf13
Merge branch 'dkatz/todo-state' of github.com:block/goose into dkatz/…
katzdave e432e4e
Merge branch 'main' of github.com:block/goose into dkatz/todo-state
katzdave 924d4bb
rm some comments
katzdave 2b5f141
rm tool state
katzdave 2ef1d4c
massively simplifiy todo state
katzdave 12ac40f
Merge branch 'main' of github.com:block/goose into dkatz/todo-state
katzdave 34bd7da
further simplify
katzdave d877088
fix openapi
katzdave 0e0a041
rename to extension data
katzdave File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,173 @@ | ||
| // Extension data management for sessions | ||
| // Provides a simple way to store extension-specific data with versioned keys | ||
|
|
||
| use anyhow::Result; | ||
| use serde::{Deserialize, Serialize}; | ||
| use serde_json::Value; | ||
| use std::collections::HashMap; | ||
| use utoipa::ToSchema; | ||
|
|
||
| /// Extension data containing all extension states | ||
| /// Keys are in format "extension_name.version" (e.g., "todo.v0") | ||
| #[derive(Debug, Clone, Serialize, Deserialize, Default, ToSchema)] | ||
| pub struct ExtensionData { | ||
| #[serde(flatten)] | ||
| pub extension_states: HashMap<String, Value>, | ||
| } | ||
|
|
||
| impl ExtensionData { | ||
| /// Create a new empty ExtensionData | ||
| pub fn new() -> Self { | ||
| Self { | ||
| extension_states: HashMap::new(), | ||
| } | ||
| } | ||
|
|
||
| /// Get extension state for a specific extension and version | ||
| pub fn get_extension_state(&self, extension_name: &str, version: &str) -> Option<&Value> { | ||
| let key = format!("{}.{}", extension_name, version); | ||
| self.extension_states.get(&key) | ||
| } | ||
|
|
||
| /// Set extension state for a specific extension and version | ||
| pub fn set_extension_state(&mut self, extension_name: &str, version: &str, state: Value) { | ||
| let key = format!("{}.{}", extension_name, version); | ||
| self.extension_states.insert(key, state); | ||
| } | ||
| } | ||
|
|
||
| /// Helper trait for extension-specific state management | ||
| pub trait ExtensionState: Sized + Serialize + for<'de> Deserialize<'de> { | ||
| /// The name of the extension | ||
| const EXTENSION_NAME: &'static str; | ||
|
|
||
| /// The version of the extension state format | ||
| const VERSION: &'static str; | ||
|
|
||
| /// Convert from JSON value | ||
| fn from_value(value: &Value) -> Result<Self> { | ||
| serde_json::from_value(value.clone()).map_err(|e| { | ||
| anyhow::anyhow!( | ||
| "Failed to deserialize {} state: {}", | ||
| Self::EXTENSION_NAME, | ||
| e | ||
| ) | ||
| }) | ||
| } | ||
|
|
||
| /// Convert to JSON value | ||
| fn to_value(&self) -> Result<Value> { | ||
| serde_json::to_value(self).map_err(|e| { | ||
| anyhow::anyhow!("Failed to serialize {} state: {}", Self::EXTENSION_NAME, e) | ||
| }) | ||
| } | ||
|
|
||
| /// Get state from extension data | ||
| fn from_extension_data(extension_data: &ExtensionData) -> Option<Self> { | ||
| extension_data | ||
| .get_extension_state(Self::EXTENSION_NAME, Self::VERSION) | ||
| .and_then(|v| Self::from_value(v).ok()) | ||
| } | ||
|
|
||
| /// Save state to extension data | ||
| fn to_extension_data(&self, extension_data: &mut ExtensionData) -> Result<()> { | ||
| let value = self.to_value()?; | ||
| extension_data.set_extension_state(Self::EXTENSION_NAME, Self::VERSION, value); | ||
| Ok(()) | ||
| } | ||
| } | ||
|
|
||
| /// TODO extension state implementation | ||
| #[derive(Debug, Clone, Serialize, Deserialize)] | ||
| pub struct TodoState { | ||
| pub content: String, | ||
| } | ||
|
|
||
| impl ExtensionState for TodoState { | ||
| const EXTENSION_NAME: &'static str = "todo"; | ||
| const VERSION: &'static str = "v0"; | ||
| } | ||
|
|
||
| impl TodoState { | ||
| /// Create a new TODO state | ||
| pub fn new(content: String) -> Self { | ||
| Self { content } | ||
| } | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
| use serde_json::json; | ||
|
|
||
| #[test] | ||
| fn test_extension_data_basic_operations() { | ||
| let mut extension_data = ExtensionData::new(); | ||
|
|
||
| // Test setting and getting extension state | ||
| let todo_state = json!({"content": "- Task 1\n- Task 2"}); | ||
| extension_data.set_extension_state("todo", "v0", todo_state.clone()); | ||
|
|
||
| assert_eq!( | ||
| extension_data.get_extension_state("todo", "v0"), | ||
| Some(&todo_state) | ||
| ); | ||
| assert_eq!(extension_data.get_extension_state("todo", "v1"), None); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_multiple_extension_states() { | ||
| let mut extension_data = ExtensionData::new(); | ||
|
|
||
| // Add multiple extension states | ||
| extension_data.set_extension_state("todo", "v0", json!("TODO content")); | ||
| extension_data.set_extension_state("memory", "v1", json!({"items": ["item1", "item2"]})); | ||
| extension_data.set_extension_state("config", "v2", json!({"setting": true})); | ||
|
|
||
| // Check all states exist | ||
| assert_eq!(extension_data.extension_states.len(), 3); | ||
| assert!(extension_data.get_extension_state("todo", "v0").is_some()); | ||
| assert!(extension_data.get_extension_state("memory", "v1").is_some()); | ||
| assert!(extension_data.get_extension_state("config", "v2").is_some()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_todo_state_trait() { | ||
| let mut extension_data = ExtensionData::new(); | ||
|
|
||
| // Create and save TODO state | ||
| let todo = TodoState::new("- Task 1\n- Task 2".to_string()); | ||
| todo.to_extension_data(&mut extension_data).unwrap(); | ||
|
|
||
| // Retrieve TODO state | ||
| let retrieved = TodoState::from_extension_data(&extension_data); | ||
| assert!(retrieved.is_some()); | ||
| assert_eq!(retrieved.unwrap().content, "- Task 1\n- Task 2"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_extension_data_serialization() { | ||
| let mut extension_data = ExtensionData::new(); | ||
| extension_data.set_extension_state("todo", "v0", json!("TODO content")); | ||
| extension_data.set_extension_state("memory", "v1", json!({"key": "value"})); | ||
|
|
||
| // Serialize to JSON | ||
| let json = serde_json::to_value(&extension_data).unwrap(); | ||
|
|
||
| // Check the structure | ||
| assert!(json.is_object()); | ||
| assert_eq!(json.get("todo.v0"), Some(&json!("TODO content"))); | ||
| assert_eq!(json.get("memory.v1"), Some(&json!({"key": "value"}))); | ||
|
|
||
| // Deserialize back | ||
| let deserialized: ExtensionData = serde_json::from_value(json).unwrap(); | ||
| assert_eq!( | ||
| deserialized.get_extension_state("todo", "v0"), | ||
| Some(&json!("TODO content")) | ||
| ); | ||
| assert_eq!( | ||
| deserialized.get_extension_state("memory", "v1"), | ||
| Some(&json!({"key": "value"})) | ||
| ); | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
We should figure out how to lift this out of agent; finding the path to find the config to read the state for a specific tool is quite cumbersome; it also strikes me that the state here should be per extension, not per tool; we have a read_todo and a write_todo tool, but unfortunately they are not part of an actual extension. thoughts?
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.
Yeah I'm with you that this is the messiest part. I was somewhat thinking of 'TODO' as an abstract tool but you're right with the current implementation it's really 2 different tools. So maybe re-implementing it as a pseudo extension with multiple tool calls (looks very similar in shape to how you'd make an mcp server) would help.
I think it would be really cool to in general support session scoped storage for tools, although not really part of the mcp spec so probably only relevant/helpful for the internal tools we build.
I guess trying to figure out what we want in the shortest term pre-next release.
Seems like options are:
I'm thinking (2).