-
Notifications
You must be signed in to change notification settings - Fork 353
fix compaction kept-tail mapping after prior summaries #2646
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -398,7 +398,7 @@ func getAllMigrations() []Migration { | |
| ID: 21, | ||
| Name: "021_add_first_kept_entry_column", | ||
| Description: "Add first_kept_entry column to session_items for compaction-preserved messages", | ||
| UpSQL: `ALTER TABLE session_items ADD COLUMN first_kept_entry INTEGER DEFAULT 0`, | ||
| UpSQL: `ALTER TABLE session_items ADD COLUMN first_kept_entry INTEGER DEFAULT -1`, | ||
|
Contributor
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. while it looks to be ok in this context it's always dangerous to update an existing migration. A new migration 22 would be cheap:
Member
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. ooof this is no good |
||
| }, | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -354,6 +354,14 @@ func (s *Session) snapshotItems() []Item { | |
| return items | ||
| } | ||
|
|
||
| // SnapshotItems returns a copy of s.Messages safe to use without holding | ||
| // s.mu. Each Message value is deep-copied so concurrent UpdateMessage calls | ||
|
Contributor
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. Exposing |
||
| // cannot mutate the snapshot; non-Message fields (Summary, SubSession, Cost, | ||
| // FirstKeptEntry) are shallow-copied since they are not mutated in place. | ||
| func (s *Session) SnapshotItems() []Item { | ||
| return s.snapshotItems() | ||
| } | ||
|
|
||
| // cloneChatMessage returns a deep copy of a chat.Message, duplicating | ||
| // all slice and pointer fields that would otherwise alias the original. | ||
| func cloneChatMessage(m chat.Message) chat.Message { | ||
|
|
@@ -902,7 +910,7 @@ func buildSessionSummaryMessages(items []Item) ([]chat.Message, int) { | |
| startIndex := lastSummaryIndex + 1 | ||
| if lastSummaryIndex >= 0 { | ||
| kept := items[lastSummaryIndex].FirstKeptEntry | ||
| if kept > 0 && kept < lastSummaryIndex { | ||
| if kept >= 0 && kept < lastSummaryIndex { | ||
| startIndex = kept | ||
| } | ||
| } | ||
|
|
||
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 path went through
sess.GetMessages(a)which applied agent-specific transformations (e.g. injecting the system prompt, flattening sub-sessions, etc.).SnapshotItems()bypasses all of that. For compaction purposes filtering by role is sufficient — but it would be worth a quick audit confirming there are no agent-level transformations inGetMessagesthe compactor was implicitly relying on (e.g. content rewriting or implicit messages). If there are none, a brief inline comment here noting the intentional bypass would make future readers more confident this is deliberate.