Fix bd-pq5k: merge conflicts now prefer closed>open and deletion>modification
CHANGES: 1. Merge logic (internal/merge/merge.go): - Added mergeStatus() enforcing closed ALWAYS wins over open - Fixed closed_at handling: only set when status='closed' - Changed deletion handling: deletion ALWAYS wins over modification 2. Deletion tracking (cmd/bd/snapshot_manager.go): - Updated ComputeAcceptedDeletions to accept all merge deletions - Removed "unchanged locally" check (deletion wins regardless) 3. FK constraint helper (internal/storage/sqlite/util.go): - Added IsForeignKeyConstraintError() for bd-koab - Detects FK violations for graceful import handling TESTS UPDATED: - TestMergeStatus: comprehensive status merge tests - TestIsForeignKeyConstraintError: FK constraint detection - bd-pq5k test: validates no invalid state (status=open with closed_at) - Deletion tests: reflect new deletion-wins behavior - All tests pass ✓ This ensures issues never get stuck in invalid states and prevents the insane situation where issues never die! 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
@@ -235,16 +235,17 @@ func TestDeletionWithLocalModification(t *testing.T) {
|
||||
t.Fatalf("Failed to simulate remote deletion: %v", err)
|
||||
}
|
||||
|
||||
// Try to merge - this should detect a conflict (modified locally, deleted remotely)
|
||||
// Try to merge - deletion now wins over modification (bd-pq5k)
|
||||
// This should succeed and delete the issue
|
||||
_, err = merge3WayAndPruneDeletions(ctx, store, jsonlPath)
|
||||
if err == nil {
|
||||
t.Error("Expected merge conflict error, but got nil")
|
||||
if err != nil {
|
||||
t.Errorf("Expected merge to succeed (deletion wins), but got error: %v", err)
|
||||
}
|
||||
|
||||
// The issue should still exist in the database (conflict not auto-resolved)
|
||||
// The issue should be deleted (deletion wins over modification)
|
||||
conflictIssue, err := store.GetIssue(ctx, "bd-conflict")
|
||||
if err != nil || conflictIssue == nil {
|
||||
t.Error("Issue should still exist after conflict")
|
||||
if err == nil && conflictIssue != nil {
|
||||
t.Error("Issue should be deleted after merge (deletion wins)")
|
||||
}
|
||||
}
|
||||
|
||||
@@ -295,12 +296,13 @@ func TestComputeAcceptedDeletions(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// TestComputeAcceptedDeletions_LocallyModified tests that locally modified issues are not deleted
|
||||
// TestComputeAcceptedDeletions_LocallyModified tests that deletion wins even for locally modified issues (bd-pq5k)
|
||||
func TestComputeAcceptedDeletions_LocallyModified(t *testing.T) {
|
||||
dir := t.TempDir()
|
||||
|
||||
basePath := filepath.Join(dir, "base.jsonl")
|
||||
leftPath := filepath.Join(dir, "left.jsonl")
|
||||
jsonlPath := filepath.Join(dir, "issues.jsonl")
|
||||
sm := NewSnapshotManager(jsonlPath)
|
||||
basePath, leftPath := sm.GetSnapshotPaths()
|
||||
mergedPath := filepath.Join(dir, "merged.jsonl")
|
||||
|
||||
// Base has 2 issues
|
||||
@@ -313,7 +315,7 @@ func TestComputeAcceptedDeletions_LocallyModified(t *testing.T) {
|
||||
{"id":"bd-2","title":"Modified locally"}
|
||||
`
|
||||
|
||||
// Merged has only bd-1 (bd-2 deleted remotely, but we modified it locally)
|
||||
// Merged has only bd-1 (bd-2 deleted remotely, we modified it locally, but deletion wins per bd-pq5k)
|
||||
mergedContent := `{"id":"bd-1","title":"Original 1"}
|
||||
`
|
||||
|
||||
@@ -327,16 +329,17 @@ func TestComputeAcceptedDeletions_LocallyModified(t *testing.T) {
|
||||
t.Fatalf("Failed to write merged: %v", err)
|
||||
}
|
||||
|
||||
jsonlPath := filepath.Join(dir, "issues.jsonl")
|
||||
sm := NewSnapshotManager(jsonlPath)
|
||||
deletions, err := sm.ComputeAcceptedDeletions(mergedPath)
|
||||
if err != nil {
|
||||
t.Fatalf("Failed to compute deletions: %v", err)
|
||||
}
|
||||
|
||||
// bd-2 should NOT be in accepted deletions because it was modified locally
|
||||
if len(deletions) != 0 {
|
||||
t.Errorf("Expected 0 deletions (locally modified), got %d: %v", len(deletions), deletions)
|
||||
// bd-pq5k: bd-2 SHOULD be in accepted deletions even though modified locally (deletion wins)
|
||||
if len(deletions) != 1 {
|
||||
t.Errorf("Expected 1 deletion (deletion wins over local modification), got %d: %v", len(deletions), deletions)
|
||||
}
|
||||
if len(deletions) == 1 && deletions[0] != "bd-2" {
|
||||
t.Errorf("Expected deletion of bd-2, got %v", deletions)
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -232,21 +232,19 @@ func (sm *SnapshotManager) Initialize() error {
|
||||
// An issue is an "accepted deletion" if:
|
||||
// - It exists in base (last import)
|
||||
// - It does NOT exist in merged (after 3-way merge)
|
||||
// - It is unchanged in left (pre-pull export) compared to base
|
||||
//
|
||||
// Note (bd-pq5k): Deletion always wins over modification in the merge,
|
||||
// so if an issue is deleted in the merged result, we accept it regardless
|
||||
// of local changes.
|
||||
func (sm *SnapshotManager) ComputeAcceptedDeletions(mergedPath string) ([]string, error) {
|
||||
basePath, leftPath := sm.getSnapshotPaths()
|
||||
basePath, _ := sm.getSnapshotPaths()
|
||||
|
||||
// Build map of ID -> raw line for base and left
|
||||
// Build map of ID -> raw line for base
|
||||
baseIndex, err := sm.buildIDToLineMap(basePath)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("failed to read base snapshot: %w", err)
|
||||
}
|
||||
|
||||
leftIndex, err := sm.buildIDToLineMap(leftPath)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("failed to read left snapshot: %w", err)
|
||||
}
|
||||
|
||||
// Build set of IDs in merged result
|
||||
mergedIDs, err := sm.buildIDSet(mergedPath)
|
||||
if err != nil {
|
||||
@@ -257,13 +255,13 @@ func (sm *SnapshotManager) ComputeAcceptedDeletions(mergedPath string) ([]string
|
||||
|
||||
// Find accepted deletions
|
||||
var deletions []string
|
||||
for id, baseLine := range baseIndex {
|
||||
for id := range baseIndex {
|
||||
// Issue in base but not in merged
|
||||
if !mergedIDs[id] {
|
||||
// Check if unchanged locally - try raw equality first, then semantic JSON comparison
|
||||
if leftLine, existsInLeft := leftIndex[id]; existsInLeft && (leftLine == baseLine || sm.jsonEquals(leftLine, baseLine)) {
|
||||
deletions = append(deletions, id)
|
||||
}
|
||||
// bd-pq5k: Deletion always wins over modification in 3-way merge
|
||||
// If the merge resulted in deletion, accept it regardless of local changes
|
||||
// The 3-way merge already determined that deletion should win
|
||||
deletions = append(deletions, id)
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user