From 563f7e875daa096e8c0e509cbf825565fb0311ac Mon Sep 17 00:00:00 2001 From: quartz Date: Sun, 4 Jan 2026 11:28:39 -0800 Subject: [PATCH] fix(merge): preserve close_reason field during merge/sync (GH#891) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The close_reason and closed_by_session fields were being silently dropped during 3-way merge operations because the simplified Issue struct in internal/merge/merge.go was missing these fields. Changes: - Add CloseReason and ClosedBySession fields to merge.Issue struct - Implement merge logic that preserves these fields when status is closed - Use timestamp-based conflict resolution (later closed_at wins) - Clear close metadata when status becomes non-closed 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 --- internal/merge/merge.go | 25 +++- internal/merge/merge_test.go | 262 +++++++++++++++++++++++++++++++++++ 2 files changed, 283 insertions(+), 4 deletions(-) diff --git a/internal/merge/merge.go b/internal/merge/merge.go index e5ffac34..b4e2bb26 100644 --- a/internal/merge/merge.go +++ b/internal/merge/merge.go @@ -48,10 +48,12 @@ type Issue struct { Status string `json:"status,omitempty"` Priority int `json:"priority"` // No omitempty: 0 is valid (P0/critical) IssueType string `json:"issue_type,omitempty"` - CreatedAt string `json:"created_at,omitempty"` - UpdatedAt string `json:"updated_at,omitempty"` - ClosedAt string `json:"closed_at,omitempty"` - CreatedBy string `json:"created_by,omitempty"` + CreatedAt string `json:"created_at,omitempty"` + UpdatedAt string `json:"updated_at,omitempty"` + ClosedAt string `json:"closed_at,omitempty"` + CloseReason string `json:"close_reason,omitempty"` // Reason provided when closing (GH#891) + ClosedBySession string `json:"closed_by_session,omitempty"` // Session that closed this issue (GH#891) + CreatedBy string `json:"created_by,omitempty"` Dependencies []Dependency `json:"dependencies,omitempty"` RawLine string `json:"-"` // Store original line for conflict output // Tombstone fields: inline soft-delete support for merge @@ -596,8 +598,23 @@ func mergeIssue(base, left, right Issue) (Issue, string) { // This prevents invalid state (status=open with closed_at set) if result.Status == StatusClosed { result.ClosedAt = maxTime(left.ClosedAt, right.ClosedAt) + // Merge close_reason and closed_by_session - use value from side with later closed_at (GH#891) + // This ensures we keep the most recent close action's metadata + if isTimeAfter(left.ClosedAt, right.ClosedAt) { + result.CloseReason = left.CloseReason + result.ClosedBySession = left.ClosedBySession + } else if right.ClosedAt != "" { + result.CloseReason = right.CloseReason + result.ClosedBySession = right.ClosedBySession + } else { + // Both empty or only left has value - prefer left + result.CloseReason = left.CloseReason + result.ClosedBySession = left.ClosedBySession + } } else { result.ClosedAt = "" + result.CloseReason = "" + result.ClosedBySession = "" } // Merge dependencies - proper 3-way merge where removals win diff --git a/internal/merge/merge_test.go b/internal/merge/merge_test.go index fe0a336c..4955abdf 100644 --- a/internal/merge/merge_test.go +++ b/internal/merge/merge_test.go @@ -2460,3 +2460,265 @@ func TestMerge3Way_DeterministicOutputOrder(t *testing.T) { } }) } +// TestMerge3Way_CloseReasonPreservation tests that close_reason and closed_by_session +// are preserved during merge/sync operations (GH#891) +func TestMerge3Way_CloseReasonPreservation(t *testing.T) { + t.Run("close_reason preserved when both sides closed - later closed_at wins", func(t *testing.T) { + base := []Issue{ + { + ID: "bd-close1", + Title: "Test Issue", + Status: "open", + CreatedAt: "2024-01-01T00:00:00Z", + CreatedBy: "user1", + }, + } + left := []Issue{ + { + ID: "bd-close1", + Title: "Test Issue", + Status: "closed", + ClosedAt: "2024-01-02T00:00:00Z", // Earlier + CloseReason: "Fixed in commit abc", + ClosedBySession: "session-left", + CreatedAt: "2024-01-01T00:00:00Z", + CreatedBy: "user1", + }, + } + right := []Issue{ + { + ID: "bd-close1", + Title: "Test Issue", + Status: "closed", + ClosedAt: "2024-01-03T00:00:00Z", // Later - should win + CloseReason: "Fixed in commit xyz", + ClosedBySession: "session-right", + CreatedAt: "2024-01-01T00:00:00Z", + CreatedBy: "user1", + }, + } + + result, conflicts := merge3Way(base, left, right, false) + if len(conflicts) != 0 { + t.Errorf("expected no conflicts, got %d", len(conflicts)) + } + if len(result) != 1 { + t.Fatalf("expected 1 merged issue, got %d", len(result)) + } + // Right has later closed_at, so right's close_reason should win + if result[0].CloseReason != "Fixed in commit xyz" { + t.Errorf("expected close_reason 'Fixed in commit xyz', got %q", result[0].CloseReason) + } + if result[0].ClosedBySession != "session-right" { + t.Errorf("expected closed_by_session 'session-right', got %q", result[0].ClosedBySession) + } + }) + + t.Run("close_reason preserved when left has later closed_at", func(t *testing.T) { + base := []Issue{ + { + ID: "bd-close2", + Title: "Test Issue", + Status: "open", + CreatedAt: "2024-01-01T00:00:00Z", + CreatedBy: "user1", + }, + } + left := []Issue{ + { + ID: "bd-close2", + Title: "Test Issue", + Status: "closed", + ClosedAt: "2024-01-03T00:00:00Z", // Later - should win + CloseReason: "Resolved by PR #123", + ClosedBySession: "session-left", + CreatedAt: "2024-01-01T00:00:00Z", + CreatedBy: "user1", + }, + } + right := []Issue{ + { + ID: "bd-close2", + Title: "Test Issue", + Status: "closed", + ClosedAt: "2024-01-02T00:00:00Z", // Earlier + CloseReason: "Duplicate", + ClosedBySession: "session-right", + CreatedAt: "2024-01-01T00:00:00Z", + CreatedBy: "user1", + }, + } + + result, conflicts := merge3Way(base, left, right, false) + if len(conflicts) != 0 { + t.Errorf("expected no conflicts, got %d", len(conflicts)) + } + if len(result) != 1 { + t.Fatalf("expected 1 merged issue, got %d", len(result)) + } + // Left has later closed_at, so left's close_reason should win + if result[0].CloseReason != "Resolved by PR #123" { + t.Errorf("expected close_reason 'Resolved by PR #123', got %q", result[0].CloseReason) + } + if result[0].ClosedBySession != "session-left" { + t.Errorf("expected closed_by_session 'session-left', got %q", result[0].ClosedBySession) + } + }) + + t.Run("close_reason cleared when status becomes open", func(t *testing.T) { + base := []Issue{ + { + ID: "bd-close3", + Title: "Test Issue", + Status: "closed", + ClosedAt: "2024-01-02T00:00:00Z", + CloseReason: "Fixed", + ClosedBySession: "session-old", + CreatedAt: "2024-01-01T00:00:00Z", + CreatedBy: "user1", + }, + } + left := []Issue{ + { + ID: "bd-close3", + Title: "Test Issue", + Status: "open", // Reopened + ClosedAt: "", + CloseReason: "", // Should be cleared + ClosedBySession: "", + CreatedAt: "2024-01-01T00:00:00Z", + CreatedBy: "user1", + }, + } + right := []Issue{ + { + ID: "bd-close3", + Title: "Test Issue", + Status: "open", // Both reopened + ClosedAt: "", + CloseReason: "", + ClosedBySession: "", + CreatedAt: "2024-01-01T00:00:00Z", + CreatedBy: "user1", + }, + } + + result, conflicts := merge3Way(base, left, right, false) + if len(conflicts) != 0 { + t.Errorf("expected no conflicts, got %d", len(conflicts)) + } + if len(result) != 1 { + t.Fatalf("expected 1 merged issue, got %d", len(result)) + } + if result[0].Status != "open" { + t.Errorf("expected status 'open', got %q", result[0].Status) + } + if result[0].CloseReason != "" { + t.Errorf("expected empty close_reason when reopened, got %q", result[0].CloseReason) + } + if result[0].ClosedBySession != "" { + t.Errorf("expected empty closed_by_session when reopened, got %q", result[0].ClosedBySession) + } + }) + + t.Run("close_reason from single side preserved", func(t *testing.T) { + base := []Issue{ + { + ID: "bd-close4", + Title: "Test Issue", + Status: "open", + CreatedAt: "2024-01-01T00:00:00Z", + CreatedBy: "user1", + }, + } + left := []Issue{ + { + ID: "bd-close4", + Title: "Test Issue", + Status: "closed", + ClosedAt: "2024-01-02T00:00:00Z", + CloseReason: "Won't fix - by design", + ClosedBySession: "session-abc", + CreatedAt: "2024-01-01T00:00:00Z", + CreatedBy: "user1", + }, + } + right := []Issue{ + { + ID: "bd-close4", + Title: "Test Issue", + Status: "open", // Still open on right + CreatedAt: "2024-01-01T00:00:00Z", + CreatedBy: "user1", + }, + } + + result, conflicts := merge3Way(base, left, right, false) + if len(conflicts) != 0 { + t.Errorf("expected no conflicts, got %d", len(conflicts)) + } + if len(result) != 1 { + t.Fatalf("expected 1 merged issue, got %d", len(result)) + } + // Closed wins over open + if result[0].Status != "closed" { + t.Errorf("expected status 'closed', got %q", result[0].Status) + } + // Close reason from the closed side should be preserved + if result[0].CloseReason != "Won't fix - by design" { + t.Errorf("expected close_reason 'Won't fix - by design', got %q", result[0].CloseReason) + } + if result[0].ClosedBySession != "session-abc" { + t.Errorf("expected closed_by_session 'session-abc', got %q", result[0].ClosedBySession) + } + }) + + t.Run("close_reason survives round-trip through JSONL", func(t *testing.T) { + // This tests the full merge pipeline including JSON marshaling/unmarshaling + tmpDir := t.TempDir() + + baseContent := `{"id":"bd-jsonl1","title":"Test Issue","status":"open","created_at":"2024-01-01T00:00:00Z","created_by":"user1"}` + leftContent := `{"id":"bd-jsonl1","title":"Test Issue","status":"closed","closed_at":"2024-01-02T00:00:00Z","close_reason":"Fixed in commit def456","closed_by_session":"session-jsonl","created_at":"2024-01-01T00:00:00Z","created_by":"user1"}` + rightContent := `{"id":"bd-jsonl1","title":"Test Issue","status":"open","created_at":"2024-01-01T00:00:00Z","created_by":"user1"}` + + basePath := filepath.Join(tmpDir, "base.jsonl") + leftPath := filepath.Join(tmpDir, "left.jsonl") + rightPath := filepath.Join(tmpDir, "right.jsonl") + outputPath := filepath.Join(tmpDir, "output.jsonl") + + if err := os.WriteFile(basePath, []byte(baseContent+"\n"), 0644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(leftPath, []byte(leftContent+"\n"), 0644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(rightPath, []byte(rightContent+"\n"), 0644); err != nil { + t.Fatal(err) + } + + if err := Merge3Way(outputPath, basePath, leftPath, rightPath, false); err != nil { + t.Fatalf("Merge3Way failed: %v", err) + } + + // Read output and verify close_reason is preserved + outputData, err := os.ReadFile(outputPath) + if err != nil { + t.Fatal(err) + } + + var outputIssue Issue + if err := json.Unmarshal(outputData[:len(outputData)-1], &outputIssue); err != nil { + t.Fatalf("failed to parse output: %v", err) + } + + if outputIssue.Status != "closed" { + t.Errorf("expected status 'closed', got %q", outputIssue.Status) + } + if outputIssue.CloseReason != "Fixed in commit def456" { + t.Errorf("expected close_reason 'Fixed in commit def456', got %q", outputIssue.CloseReason) + } + if outputIssue.ClosedBySession != "session-jsonl" { + t.Errorf("expected closed_by_session 'session-jsonl', got %q", outputIssue.ClosedBySession) + } + }) +}