Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes conversation message ordering by sorting on the message “created” time (rather than the DB insertion timestamp) and ensures ordering is re-applied after tool call/response summarization so history remains chronological.
Changes:
- Update session conversation fetch to
ORDER BY created_timestamp. - Add
Conversation::sort_by_created()helper. - Sort the in-memory conversation after extending it with newly added/summarized messages.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| crates/goose/src/session/session_manager.rs | Fetches messages ordered by message creation timestamp instead of DB timestamp. |
| crates/goose/src/conversation/mod.rs | Adds a helper to sort messages by their created field. |
| crates/goose/src/agents/agent.rs | Re-sorts the conversation after appending new messages (including tool-pair summary messages). |
| let pool = self.pool().await?; | ||
| let rows = sqlx::query_as::<_, (String, String, i64, Option<String>, Option<String>)>( | ||
| "SELECT role, content_json, created_timestamp, metadata_json, message_id FROM messages WHERE session_id = ? ORDER BY timestamp", | ||
| "SELECT role, content_json, created_timestamp, metadata_json, message_id FROM messages WHERE session_id = ? ORDER BY created_timestamp", |
There was a problem hiding this comment.
ORDER BY created_timestamp alone can still return messages out of order when multiple rows share the same created_timestamp (timestamps are second-granularity), so consider adding a deterministic tie-breaker like ORDER BY created_timestamp, id (the AUTOINCREMENT PK) to preserve insertion order for ties.
| "SELECT role, content_json, created_timestamp, metadata_json, message_id FROM messages WHERE session_id = ? ORDER BY created_timestamp", | |
| "SELECT role, content_json, created_timestamp, metadata_json, message_id FROM messages WHERE session_id = ? ORDER BY created_timestamp, rowid", |
| let pool = self.pool().await?; | ||
| let rows = sqlx::query_as::<_, (String, String, i64, Option<String>, Option<String>)>( | ||
| "SELECT role, content_json, created_timestamp, metadata_json, message_id FROM messages WHERE session_id = ? ORDER BY timestamp", | ||
| "SELECT role, content_json, created_timestamp, metadata_json, message_id FROM messages WHERE session_id = ? ORDER BY created_timestamp", |
There was a problem hiding this comment.
This query now orders by created_timestamp but the schema only indexes timestamp, so fetching a long conversation may require a full sort; consider adding an index (ideally composite on (session_id, created_timestamp)) to keep this query efficient.
| async fn get_conversation(&self, session_id: &str) -> Result<Conversation> { | ||
| let pool = self.pool().await?; | ||
| let rows = sqlx::query_as::<_, (String, String, i64, Option<String>, Option<String>)>( | ||
| "SELECT role, content_json, created_timestamp, metadata_json, message_id FROM messages WHERE session_id = ? ORDER BY timestamp", | ||
| "SELECT role, content_json, created_timestamp, metadata_json, message_id FROM messages WHERE session_id = ? ORDER BY created_timestamp", |
There was a problem hiding this comment.
There are existing tests in this file, but this ordering change isn’t covered; adding a regression test that inserts messages with non-monotonic created_timestamp values and asserts get_conversation() returns them sorted would help prevent this from regressing.
crates/goose/src/agents/agent.rs
Outdated
| session_manager.add_message(&session_config.id, msg).await?; | ||
| } | ||
| conversation.extend(messages_to_add); | ||
| conversation.sort_by_created(); |
There was a problem hiding this comment.
Sorting the entire conversation on every loop iteration is O(n log n) and will get more expensive as history grows; consider only calling sort_by_created() when you actually inserted an out-of-order message (e.g., after tool-pair summarization) or when the newest message’s created is older than the current last message.
| conversation.sort_by_created(); | |
| if !conversation.messages().is_empty() { | |
| let messages_ref = conversation.messages(); | |
| if messages_ref.len() >= 2 { | |
| let last_idx = messages_ref.len() - 1; | |
| let last_created = messages_ref[last_idx].created; | |
| let prev_created = messages_ref[last_idx - 1].created; | |
| if last_created < prev_created { | |
| conversation.sort_by_created(); | |
| } | |
| } | |
| } |
crates/goose/src/conversation/mod.rs
Outdated
| pub fn sort_by_created(&mut self) { | ||
| self.0.sort_by_key(|m| m.created); | ||
| } | ||
|
|
There was a problem hiding this comment.
what about making Conversation always keep them sorted? this method could be private and called on every mutable method
…m-cache * 'main' of github.com:block/goose: chore(release): release version 1.25.0 (minor) (#7263) Docs: Community all-stars and page update (#7483) fix: searchbar zindex modal overlay (#7502) blog: Order Lunch Without Leaving Your AI Agent (#7505) feat: gateway to chat to goose - telegram etc (#7199) Option to add changeable defaults in goose-releases (#7373) Fix out of order messages (#7472) fix: ensure latest session always displays in sidebar (#7489) docs: rename TLDR to Quick Install in MCP tutorials (#7493) docs: update Neighborhood extension page with video embed and layout improvements (#7473) feat: let AskAI Discord bot see channels in the server (#7480) Apps token limit (#7474) fix(apps): declare color-scheme to allow transparent MCP App iframes (#7479)
Co-authored-by: Douwe Osinga <[email protected]> Signed-off-by: Oleg Levchenko <[email protected]>
Summary
we weren't sorting the messages by creation, but rather by database timestamp. also make sure the messages are explicitly sorted after tool call/response summarization