Next Camp Live - Added editing messages functionality #5813
Conversation
DOsinga
left a comment
DOsinga
left a comment
There was a problem hiding this comment.
I think this can all be done much simpler:
- Introduce two new primitives for sessions/conversations, copy and truncate
- truncate takes a timestamp and always truncate also the edited message
- then edit in place just becomes, truncate conversation, call submit with the edited message
- and fork becomes, copy existing conversation, truncate it and load it up with the edited message as autosubmit
| recipe_name: Option<String>, | ||
| recipe_version: Option<String>, | ||
| #[serde(default)] | ||
| skip_add_user_message: bool, |
There was a problem hiding this comment.
rule number one, no optional
crates/goose/src/agents/agent.rs
Outdated
| user_message: Message, | ||
| session_config: SessionConfig, | ||
| cancel_token: Option<CancellationToken>, | ||
| skip_add_message: bool, |
There was a problem hiding this comment.
why split this function? if we need an extra parameter, just add the parameter
| pub struct Message { | ||
| pub id: Option<String>, | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub row_id: Option<i64>, |
There was a problem hiding this comment.
why do we need a row_id here? can't we just use the id we already have?
|
|
||
| let mut stream = match agent | ||
| .reply( | ||
| .reply_with_options( |
There was a problem hiding this comment.
this strikes me as cumbersome
what I would do/expect here is when we fork the conversation we fork/truncate without the new message. in the case of edit you can then just call the normal submit which will add the edited message to the conversation just as before.
in the case of fork, you can just do the same thing as we do in hub and immediately submit the new message
| ), | ||
| tag = "Session Management" | ||
| )] | ||
| async fn edit_message( |
There was a problem hiding this comment.
I think here you'd just want two simple primitive; copy which takes a session id and returns a new one with an id and truncate which takes a session id and a message id and then deletes all messages including and after that message id
| message_row_id: i64, | ||
| new_message_content: String, | ||
| ) -> Result<Session> { | ||
| use crate::conversation::message::MessageContent; |
|
|
||
| let mut message = Message::new(role, created_timestamp, content); | ||
| message.metadata = metadata; | ||
| messages.push(message); |
There was a problem hiding this comment.
this is all duplicated code, no?
we should either add a flag to get_conversation to return messages only up to the desired message or and that's slower but simpler, just get the whole conversation and then add a truncate method to conversation class (haha)
I'd go with the last one
| let original_session = self.get_session(session_id, false).await?; | ||
| let rows = sqlx::query_as::<_, (String, String, i64, Option<String>)>( | ||
| "SELECT role, content_json, created_timestamp, metadata_json FROM messages WHERE session_id = ? AND id < ? ORDER BY id", | ||
| ) |
There was a problem hiding this comment.
we normally sort by timestamp - sorting on id probably works. but it is more consistent to use the timestamp. and from that perspective, use the timestamp also to decide where to truncate the conversation. that is what the user sees
| .fetch_all(&self.pool) | ||
| .await?; | ||
|
|
||
| let mut messages = Vec::new(); |
There was a problem hiding this comment.
even more code duplication
| chatState, | ||
| handleSubmit, | ||
| messages, | ||
| ]); |
There was a problem hiding this comment.
these checks make me feel very uneasy. this looks brittle and very specific and reminds me of the sins of old we had in the recipe manager. I don't think you need this at all if we don't add the message
Summary
Follow up for next camp live to add editing messages on the server. Added another option to fork the conversation into a new session also.