diff --git a/pkg/seahorse/short_compaction.go b/pkg/seahorse/short_compaction.go index 97a0795bb..38676f02f 100644 --- a/pkg/seahorse/short_compaction.go +++ b/pkg/seahorse/short_compaction.go @@ -387,13 +387,30 @@ func (e *CompactionEngine) compactCondensed(ctx context.Context, convID int64) ( startOrd := -1 endOrd := -1 + hasNonCandidate := false for _, item := range items { if item.ItemType == "summary" && candidateSet[item.SummaryID] { - if startOrd == -1 || item.Ordinal < startOrd { - startOrd = item.Ordinal - } - if endOrd == -1 || item.Ordinal > endOrd { - endOrd = item.Ordinal + if startOrd == -1 { + startOrd, endOrd = item.Ordinal, item.Ordinal + } else { + // Check for non-candidate items between endOrd and current ordinal + for _, it := range items { + if it.Ordinal > endOrd && it.Ordinal <= item.Ordinal { + if it.ItemType != "summary" || !candidateSet[it.SummaryID] { + hasNonCandidate = true + break + } + } + } + if hasNonCandidate { + break + } + if item.Ordinal < startOrd { + startOrd = item.Ordinal + } + if item.Ordinal > endOrd { + endOrd = item.Ordinal + } } } } @@ -402,8 +419,22 @@ func (e *CompactionEngine) compactCondensed(ctx context.Context, convID int64) ( return nil, nil } - if err := e.store.ReplaceContextRangeWithSummary(ctx, convID, startOrd, endOrd, summary.SummaryID); err != nil { - return nil, err + // Collect candidate summary IDs + candidateIDs := make([]string, 0, len(candidates)) + for _, c := range candidates { + candidateIDs = append(candidateIDs, c.SummaryID) + } + + if hasNonCandidate { + // Use safe per-item deletion to avoid deleting non-candidate items + if err := e.store.ReplaceContextItemsWithSummary(ctx, convID, candidateIDs, summary.SummaryID); err != nil { + return nil, err + } + } else { + // Candidates are consecutive, use efficient range deletion + if err := e.store.ReplaceContextRangeWithSummary(ctx, convID, startOrd, endOrd, summary.SummaryID); err != nil { + return nil, err + } } return &summary.SummaryID, nil diff --git a/pkg/seahorse/store.go b/pkg/seahorse/store.go index 8077f4c75..e50e25d8d 100644 --- a/pkg/seahorse/store.go +++ b/pkg/seahorse/store.go @@ -4,7 +4,6 @@ import ( "context" "database/sql" "fmt" - "strconv" "strings" "time" ) @@ -820,6 +819,101 @@ func (s *Store) ReplaceContextRangeWithSummary( return tx.Commit() } +// ReplaceContextItemsWithSummary replaces specific context items (by summary_id) with a new summary. +// Use this when candidates are not contiguous in ordinal space to avoid deleting non-candidate items. +func (s *Store) ReplaceContextItemsWithSummary( + ctx context.Context, + convID int64, + summaryIDs []string, + newSummaryID string, +) error { + if len(summaryIDs) == 0 { + return nil + } + + tx, err := s.db.BeginTx(ctx, nil) + if err != nil { + return err + } + defer tx.Rollback() + + // Find the ordinals of items to delete and calculate midpoint + placeholders := make([]string, len(summaryIDs)) + args := make([]any, len(summaryIDs)+1) + args[0] = convID + for i, sid := range summaryIDs { + placeholders[i] = "?" + args[i+1] = sid + } + + query := fmt.Sprintf( + "SELECT ordinal FROM context_items WHERE conversation_id = ? AND summary_id IN (%s) ORDER BY ordinal", + strings.Join(placeholders, ","), + ) + rows, err := tx.QueryContext(ctx, query, args...) + if err != nil { + return err + } + + var ordinals []int + for rows.Next() { + var ord int + if err := rows.Scan(&ord); err != nil { + rows.Close() + return err + } + ordinals = append(ordinals, ord) + } + rows.Close() + + if len(ordinals) == 0 { + return nil + } + + midpoint := (ordinals[0] + ordinals[len(ordinals)-1]) / 2 + + // Delete the specific items by summary_id + deleteQuery := fmt.Sprintf( + "DELETE FROM context_items WHERE conversation_id = ? AND summary_id IN (%s)", + strings.Join(placeholders, ","), + ) + _, err = tx.ExecContext(ctx, deleteQuery, args...) + if err != nil { + return err + } + + // Check if midpoint conflicts with existing ordinal + var conflict bool + var existingOrd int + err = tx.QueryRowContext(ctx, + "SELECT ordinal FROM context_items WHERE conversation_id = ? AND ordinal = ?", + convID, midpoint, + ).Scan(&existingOrd) + if err == nil { + conflict = true + } + + if conflict { + // Gap exhausted, need resequence + err = s.resequenceContextItemsTx(ctx, tx, convID, newSummaryID) + if err != nil { + return fmt.Errorf("resequence: %w", err) + } + } else { + // Normal insert at midpoint + _, err = tx.ExecContext(ctx, + `INSERT INTO context_items (conversation_id, ordinal, item_type, summary_id, token_count) + SELECT ?, ?, 'summary', ?, token_count FROM summaries WHERE summary_id = ?`, + convID, midpoint, newSummaryID, newSummaryID, + ) + if err != nil { + return err + } + } + + return tx.Commit() +} + // resequenceContextItemsTx renumbers context_items with fresh OrdinalStep gaps. // Uses temp negative ordinals to avoid PRIMARY KEY constraint violations (spec lines 1240-1247). func (s *Store) resequenceContextItemsTx(ctx context.Context, tx *sql.Tx, convID int64, newSummaryID string) error { @@ -844,17 +938,17 @@ func (s *Store) resequenceContextItemsTx(ctx context.Context, tx *sql.Tx, convID var items []item for rows.Next() { var i item - var sid, mid sql.NullString + var sid sql.NullString + var mid sql.NullInt64 var scanErr error if scanErr = rows.Scan(&i.ordinal, &i.itemType, &sid, &mid, &i.tokenCount); scanErr != nil { - return err + return scanErr } if sid.Valid { i.summaryID = sid.String } if mid.Valid { - id, _ := strconv.ParseInt(mid.String, 10, 64) - i.messageID = id + i.messageID = mid.Int64 } items = append(items, i) } diff --git a/pkg/seahorse/store_test.go b/pkg/seahorse/store_test.go index 22790ebdc..3dca90a89 100644 --- a/pkg/seahorse/store_test.go +++ b/pkg/seahorse/store_test.go @@ -1141,3 +1141,107 @@ func TestStoreSearchSummariesReturnsContent(t *testing.T) { t.Errorf("SearchResult.Content = %q, want %q", results[0].Content, "This is the summary content for testing") } } + +func TestStoreReplaceContextItemsWithSummary(t *testing.T) { + s := openTestStore(t) + ctx := context.Background() + + conv, _ := s.GetOrCreateConversation(ctx, "agent:test-replace-items") + + // Create messages + msgs := make([]int64, 5) + for i := 0; i < 5; i++ { + m, _ := s.AddMessage(ctx, conv.ConversationID, "user", fmt.Sprintf("msg%d", i), 2) + msgs[i] = m.ID + } + + // Create summaries + summaries := make([]string, 3) + for i := 0; i < 3; i++ { + sum, _ := s.CreateSummary(ctx, CreateSummaryInput{ + ConversationID: conv.ConversationID, + Kind: SummaryKindLeaf, + Depth: 0, + Content: fmt.Sprintf("summary %d", i), + TokenCount: 10, + }) + summaries[i] = sum.SummaryID + } + + // Insert context items with a message in between summaries: + // Ordinals: 100 (summary0), 200 (message), 300 (summary1), 400 (summary2) + items := []ContextItem{ + {Ordinal: 100, ItemType: "summary", SummaryID: summaries[0], TokenCount: 10}, + {Ordinal: 200, ItemType: "message", MessageID: msgs[1], TokenCount: 2}, + {Ordinal: 300, ItemType: "summary", SummaryID: summaries[1], TokenCount: 10}, + {Ordinal: 400, ItemType: "summary", SummaryID: summaries[2], TokenCount: 10}, + } + s.UpsertContextItems(ctx, conv.ConversationID, items) + + // Create a new summary to replace with + newSummary, _ := s.CreateSummary(ctx, CreateSummaryInput{ + ConversationID: conv.ConversationID, + Kind: SummaryKindCondensed, + Depth: 1, + Content: "condensed summary", + TokenCount: 15, + }) + + // Replace summaries 0 and 1 (not 2) using per-item deletion + // This should NOT delete the message at ordinal 200 + err := s.ReplaceContextItemsWithSummary(ctx, conv.ConversationID, []string{summaries[0], summaries[1]}, newSummary.SummaryID) + if err != nil { + t.Fatalf("ReplaceContextItemsWithSummary: %v", err) + } + + // Verify result: should have 3 items (message at 200, summary2 at 400, new summary) + result, _ := s.GetContextItems(ctx, conv.ConversationID) + if len(result) != 3 { + t.Fatalf("expected 3 items after replace, got %d", len(result)) + } + + // Verify message at ordinal 200 is preserved + messagePreserved := false + for _, item := range result { + if item.ItemType == "message" && item.MessageID == msgs[1] { + messagePreserved = true + break + } + } + if !messagePreserved { + t.Error("message at ordinal 200 should have been preserved") + } + + // Verify summary2 at ordinal 400 is preserved + summary2Preserved := false + for _, item := range result { + if item.ItemType == "summary" && item.SummaryID == summaries[2] { + summary2Preserved = true + break + } + } + if !summary2Preserved { + t.Error("summary2 at ordinal 400 should have been preserved") + } + + // Verify new summary exists + newSummaryFound := false + for _, item := range result { + if item.ItemType == "summary" && item.SummaryID == newSummary.SummaryID { + newSummaryFound = true + break + } + } + if !newSummaryFound { + t.Error("new summary should exist") + } + + // Verify no duplicate ordinals + ordinalSet := make(map[int]bool) + for _, item := range result { + if ordinalSet[item.Ordinal] { + t.Errorf("duplicate ordinal %d detected", item.Ordinal) + } + ordinalSet[item.Ordinal] = true + } +}