-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Replace compaction notifications with system notifications #5218
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
Changes from all commits
15224fb
a795429
814de07
af08051
69b66f2
60aca5e
d0098b3
38f00fe
86d6da7
97aa4ab
76252aa
3689708
bcb0b6d
f9dada8
c2139de
b0e1dd3
293a696
85d446e
a3c5278
8acc16c
46e8c14
e1c9b62
44f1674
2f623d4
ecb523a
22db6f8
9151686
4925dba
dfddf3e
b82dec7
2cb8636
ab83a4c
3c78a6e
80380bc
b60a69f
ea91c6d
cb1f51c
b94efe5
a8ff2ec
0841d8c
85aeda0
a3130ef
e7c106f
159d82f
d1b3f30
f32364f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -93,14 +93,6 @@ pub async fn compact_messages( | |
| final_token_counts.push(0); | ||
| } | ||
|
|
||
| // Add the compaction marker (user_visible=true, agent_visible=false) | ||
| let compaction_marker = Message::assistant() | ||
| .with_conversation_compacted("Conversation compacted and summarized") | ||
| .with_metadata(MessageMetadata::user_only()); | ||
| let compaction_marker_tokens: usize = 0; // Not counted since agent_visible=false | ||
| final_messages.push(compaction_marker); | ||
| final_token_counts.push(compaction_marker_tokens); | ||
|
|
||
| // Add the summary message (agent_visible=true, user_visible=false) | ||
| let summary_msg = summary_message.with_metadata(MessageMetadata::agent_only()); | ||
|
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. do we still need this block below? I thought we had already deleted that
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. The assistant_message? The summary is a user message historically; I think we could make it an agent one and then we don't need this. Just don't want the conversation to end in a user message unless we're trying to continue. |
||
| // For token counting purposes, we use the output tokens (the actual summary content) | ||
|
|
@@ -281,7 +273,9 @@ fn format_message_for_compacting(msg: &Message) -> String { | |
| } | ||
| MessageContent::Thinking(thinking) => format!("thinking: {}", thinking.thinking), | ||
| MessageContent::RedactedThinking(_) => "redacted_thinking".to_string(), | ||
| MessageContent::ConversationCompacted(compact) => format!("compacted: {}", compact.msg), | ||
| MessageContent::SystemNotification(notification) => { | ||
| format!("system_notification: {}", notification.msg) | ||
| } | ||
| }) | ||
| .collect(); | ||
|
|
||
|
|
||
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.
this seems rather convoluted; I think it is because you want the manual compaction to use the same stream as we do for the agent reply? what was wrong with the old 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.
The old way triggered from REST vs a stream. It just made it a lot more complicated to keep the client in the right 'thinking' state. It also sent the client's view of the messages and used that which would cause a bit of rendering jitter when we did history replaced.
Could maybe split these more, but I think it's generally fine, main differences here are messaging + whether or not we continue the stream.