diff --git a/cmd/roborev/refine.go b/cmd/roborev/refine.go index 413f7c1..030700b 100644 --- a/cmd/roborev/refine.go +++ b/cmd/roborev/refine.go @@ -284,8 +284,8 @@ func runRefine(agentName, modelStr, reasoningStr string, maxIterations int, quie if verdict == "F" && !review.Addressed { currentFailedReview = review } else if verdict == "P" { - if err := client.MarkReviewAddressed(review.ID); err != nil { - fmt.Printf("Warning: failed to mark review %d as addressed: %v\n", review.ID, err) + if err := client.MarkReviewAddressed(review.JobID); err != nil { + fmt.Printf("Warning: failed to mark review (job %d) as addressed: %v\n", review.JobID, err) } continue // Loop back to check for more } @@ -484,8 +484,8 @@ func runRefine(agentName, modelStr, reasoningStr string, maxIterations int, quie client.AddComment(currentFailedReview.JobID, "roborev-refine", responseText) // Mark old review as addressed - if err := client.MarkReviewAddressed(currentFailedReview.ID); err != nil { - fmt.Printf("Warning: failed to mark review %d as addressed: %v\n", currentFailedReview.ID, err) + if err := client.MarkReviewAddressed(currentFailedReview.JobID); err != nil { + fmt.Printf("Warning: failed to mark review (job %d) as addressed: %v\n", currentFailedReview.JobID, err) } // Wait for new commit to be reviewed (if post-commit hook triggers it) @@ -511,8 +511,8 @@ func runRefine(agentName, modelStr, reasoningStr string, maxIterations int, quie verdict := storage.ParseVerdict(review.Output) if verdict == "P" { fmt.Println("New commit passed review!") - if err := client.MarkReviewAddressed(review.ID); err != nil { - fmt.Printf("Warning: failed to mark review %d as addressed: %v\n", review.ID, err) + if err := client.MarkReviewAddressed(review.JobID); err != nil { + fmt.Printf("Warning: failed to mark review (job %d) as addressed: %v\n", review.JobID, err) } currentFailedReview = nil // Move on to next oldest failed commit } else { @@ -573,8 +573,8 @@ func findFailedReviewForBranch(client daemon.Client, commits []string, skip map[ // Mark passing reviews as addressed so they don't need to be checked again if verdict == "P" { - if err := client.MarkReviewAddressed(review.ID); err != nil { - return nil, fmt.Errorf("marking review %d as addressed: %w", review.ID, err) + if err := client.MarkReviewAddressed(review.JobID); err != nil { + return nil, fmt.Errorf("marking review (job %d) as addressed: %w", review.JobID, err) } } } diff --git a/cmd/roborev/refine_test.go b/cmd/roborev/refine_test.go index ecdb0c1..670b84b 100644 --- a/cmd/roborev/refine_test.go +++ b/cmd/roborev/refine_test.go @@ -22,7 +22,7 @@ type mockDaemonClient struct { responses map[int64][]storage.Response // Track calls for assertions - addressedReviews []int64 + addressedJobIDs []int64 addedComments []addedComment enqueuedReviews []enqueuedReview @@ -70,11 +70,11 @@ func (m *mockDaemonClient) GetReviewByJobID(jobID int64) (*storage.Review, error return m.reviews[job.GitRef], nil } -func (m *mockDaemonClient) MarkReviewAddressed(reviewID int64) error { +func (m *mockDaemonClient) MarkReviewAddressed(jobID int64) error { if m.markAddressedErr != nil { return m.markAddressedErr } - m.addressedReviews = append(m.addressedReviews, reviewID) + m.addressedJobIDs = append(m.addressedJobIDs, jobID) return nil } @@ -497,18 +497,18 @@ func TestFindFailedReviewForBranch_MarksPassingAsAddressed(t *testing.T) { t.Errorf("expected no failed reviews, got job %d", found.JobID) } - // Both passing reviews should be marked as addressed - if len(client.addressedReviews) != 2 { - t.Errorf("expected 2 reviews to be marked addressed, got %d", len(client.addressedReviews)) + // Both passing reviews should be marked as addressed (by job ID) + if len(client.addressedJobIDs) != 2 { + t.Errorf("expected 2 reviews to be marked addressed, got %d", len(client.addressedJobIDs)) } - // Verify the specific review IDs were marked + // Verify the specific job IDs were marked addressed := make(map[int64]bool) - for _, id := range client.addressedReviews { + for _, id := range client.addressedJobIDs { addressed[id] = true } - if !addressed[1] || !addressed[2] { - t.Errorf("expected reviews 1 and 2 to be marked addressed, got %v", client.addressedReviews) + if !addressed[100] || !addressed[200] { + t.Errorf("expected jobs 100 and 200 to be marked addressed, got %v", client.addressedJobIDs) } } @@ -533,12 +533,12 @@ func TestFindFailedReviewForBranch_MarksPassingBeforeFailure(t *testing.T) { t.Errorf("expected failed review (job 200), got %v", found) } - // Passing review before the failure should be marked addressed - if len(client.addressedReviews) != 1 { - t.Errorf("expected 1 review to be marked addressed, got %d", len(client.addressedReviews)) + // Passing review before the failure should be marked addressed (by job ID) + if len(client.addressedJobIDs) != 1 { + t.Errorf("expected 1 review to be marked addressed, got %d", len(client.addressedJobIDs)) } - if len(client.addressedReviews) > 0 && client.addressedReviews[0] != 1 { - t.Errorf("expected review 1 to be marked addressed, got %v", client.addressedReviews) + if len(client.addressedJobIDs) > 0 && client.addressedJobIDs[0] != 100 { + t.Errorf("expected job 100 to be marked addressed, got %v", client.addressedJobIDs) } } @@ -563,8 +563,8 @@ func TestFindFailedReviewForBranch_DoesNotMarkAlreadyAddressed(t *testing.T) { } // Already-addressed review should NOT be marked again - if len(client.addressedReviews) != 0 { - t.Errorf("expected no reviews to be marked addressed (already addressed), got %v", client.addressedReviews) + if len(client.addressedJobIDs) != 0 { + t.Errorf("expected no reviews to be marked addressed (already addressed), got %v", client.addressedJobIDs) } } @@ -598,16 +598,16 @@ func TestFindFailedReviewForBranch_MixedScenario(t *testing.T) { } // commit1 and commit4 should be marked as addressed (unaddressed passing reviews) - if len(client.addressedReviews) != 2 { - t.Errorf("expected 2 reviews to be marked addressed, got %d: %v", len(client.addressedReviews), client.addressedReviews) + if len(client.addressedJobIDs) != 2 { + t.Errorf("expected 2 reviews to be marked addressed, got %d: %v", len(client.addressedJobIDs), client.addressedJobIDs) } addressed := make(map[int64]bool) - for _, id := range client.addressedReviews { + for _, id := range client.addressedJobIDs { addressed[id] = true } - if !addressed[1] || !addressed[4] { - t.Errorf("expected reviews 1 and 4 to be marked addressed, got %v", client.addressedReviews) + if !addressed[100] || !addressed[400] { + t.Errorf("expected jobs 100 and 400 to be marked addressed, got %v", client.addressedJobIDs) } } @@ -635,8 +635,8 @@ func TestFindFailedReviewForBranch_StopsAtFirstFailure(t *testing.T) { } // No reviews should be marked as addressed (we stopped at first failure) - if len(client.addressedReviews) != 0 { - t.Errorf("expected no reviews to be marked addressed, got %v", client.addressedReviews) + if len(client.addressedJobIDs) != 0 { + t.Errorf("expected no reviews to be marked addressed, got %v", client.addressedJobIDs) } } @@ -663,10 +663,10 @@ func TestFindFailedReviewForBranch_MarkAddressedError(t *testing.T) { t.Errorf("expected nil review when error occurs, got job %d", found.JobID) } - // Error message should indicate which review failed - expectedMsg := "marking review 1 as addressed" + // Error message should indicate which job failed + expectedMsg := "marking review (job 100) as addressed" if !strings.Contains(err.Error(), expectedMsg) { - t.Errorf("error should mention review ID, got: %v", err) + t.Errorf("error should mention job ID, got: %v", err) } } diff --git a/internal/config/config.go b/internal/config/config.go index 7545c35..0b1d1ae 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -50,6 +50,10 @@ type SyncConfig struct { // ConnectTimeout is the connection timeout (e.g., "5s"). Default: 5s ConnectTimeout string `toml:"connect_timeout"` + + // RepoNames provides custom display names for synced repos by identity. + // Example: {"git@github.com:org/repo.git": "my-project"} + RepoNames map[string]string `toml:"repo_names"` } // PostgresURLExpanded returns the PostgreSQL URL with environment variables expanded. @@ -61,6 +65,15 @@ func (c *SyncConfig) PostgresURLExpanded() string { return os.ExpandEnv(c.PostgresURL) } +// GetRepoDisplayName returns the configured display name for a repo identity, +// or empty string if no override is configured. +func (c *SyncConfig) GetRepoDisplayName(identity string) string { + if c == nil || c.RepoNames == nil { + return "" + } + return c.RepoNames[identity] +} + // Validate checks the sync configuration for common issues. // Returns a list of warnings (non-fatal issues). func (c *SyncConfig) Validate() []string { diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 3dc88a1..526f31e 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -570,6 +570,45 @@ func TestSyncConfigPostgresURLExpanded(t *testing.T) { }) } +func TestSyncConfigGetRepoDisplayName(t *testing.T) { + t.Run("nil receiver returns empty", func(t *testing.T) { + var cfg *SyncConfig + if got := cfg.GetRepoDisplayName("any"); got != "" { + t.Errorf("Expected empty string for nil receiver, got %q", got) + } + }) + + t.Run("nil map returns empty", func(t *testing.T) { + cfg := &SyncConfig{} + if got := cfg.GetRepoDisplayName("any"); got != "" { + t.Errorf("Expected empty string for nil map, got %q", got) + } + }) + + t.Run("missing key returns empty", func(t *testing.T) { + cfg := &SyncConfig{ + RepoNames: map[string]string{ + "git@github.com:org/repo.git": "my-repo", + }, + } + if got := cfg.GetRepoDisplayName("unknown"); got != "" { + t.Errorf("Expected empty string for missing key, got %q", got) + } + }) + + t.Run("returns configured name", func(t *testing.T) { + cfg := &SyncConfig{ + RepoNames: map[string]string{ + "git@github.com:org/repo.git": "my-custom-name", + }, + } + expected := "my-custom-name" + if got := cfg.GetRepoDisplayName("git@github.com:org/repo.git"); got != expected { + t.Errorf("Expected %q, got %q", expected, got) + } + }) +} + func TestSyncConfigValidate(t *testing.T) { t.Run("disabled returns no warnings", func(t *testing.T) { cfg := SyncConfig{Enabled: false} diff --git a/internal/daemon/client.go b/internal/daemon/client.go index 8a93239..53b97e8 100644 --- a/internal/daemon/client.go +++ b/internal/daemon/client.go @@ -23,8 +23,8 @@ type Client interface { // GetReviewByJobID retrieves a review by job ID GetReviewByJobID(jobID int64) (*storage.Review, error) - // MarkReviewAddressed marks a review as addressed - MarkReviewAddressed(reviewID int64) error + // MarkReviewAddressed marks a review as addressed by job ID + MarkReviewAddressed(jobID int64) error // AddComment adds a comment to a job AddComment(jobID int64, commenter, comment string) error @@ -130,9 +130,9 @@ func (c *HTTPClient) GetReviewByJobID(jobID int64) (*storage.Review, error) { return &review, nil } -func (c *HTTPClient) MarkReviewAddressed(reviewID int64) error { +func (c *HTTPClient) MarkReviewAddressed(jobID int64) error { reqBody, _ := json.Marshal(map[string]interface{}{ - "review_id": reviewID, + "job_id": jobID, "addressed": true, }) diff --git a/internal/daemon/client_test.go b/internal/daemon/client_test.go index fafbc8c..aa3e404 100644 --- a/internal/daemon/client_test.go +++ b/internal/daemon/client_test.go @@ -68,8 +68,8 @@ func TestHTTPClientMarkReviewAddressed(t *testing.T) { t.Fatalf("MarkReviewAddressed failed: %v", err) } - if received["review_id"].(float64) != 99 { - t.Errorf("expected review_id 99, got %v", received["review_id"]) + if received["job_id"].(float64) != 99 { + t.Errorf("expected job_id 99, got %v", received["job_id"]) } if received["addressed"] != true { t.Errorf("expected addressed true, got %v", received["addressed"]) diff --git a/internal/storage/db.go b/internal/storage/db.go index b4d14d7..c413f27 100644 --- a/internal/storage/db.go +++ b/internal/storage/db.go @@ -660,27 +660,34 @@ func (db *DB) migrateSyncColumns() error { } } - // Create unique index on repos.identity (allows NULL values, only enforces uniqueness on non-NULL) - // First normalize empty strings to NULL (treat empty as "unset") + // Normalize empty strings to NULL (treat empty as "unset") _, err = db.Exec(`UPDATE repos SET identity = NULL WHERE identity = ''`) if err != nil { return fmt.Errorf("normalize empty identities to NULL: %w", err) } - // Check for duplicates that would prevent index creation - var dupCount int - err = db.QueryRow(`SELECT COUNT(*) FROM ( - SELECT identity FROM repos WHERE identity IS NOT NULL - GROUP BY identity HAVING COUNT(*) > 1 - )`).Scan(&dupCount) - if err != nil { - return fmt.Errorf("check duplicate identities: %w", err) + // Create non-unique index on repos.identity for query performance. + // Note: identity is NOT unique because multiple local clones of the same repo + // (e.g., ~/project-1 and ~/project-2 both cloned from the same remote) + // should be allowed and will share the same identity. + // See: https://github.com/roborev-dev/roborev/issues/131 + + // Migration: If an old UNIQUE index exists, drop it first + var indexSQL sql.NullString + err = db.QueryRow(`SELECT sql FROM sqlite_master WHERE type='index' AND name='idx_repos_identity'`).Scan(&indexSQL) + if err != nil && err != sql.ErrNoRows { + return fmt.Errorf("check existing idx_repos_identity: %w", err) } - if dupCount > 0 { - return fmt.Errorf("cannot create unique index on repos.identity: %d duplicate non-NULL identities exist; resolve duplicates before upgrading", dupCount) + if indexSQL.Valid && strings.Contains(strings.ToUpper(indexSQL.String), "UNIQUE") { + // Drop the old unique index + _, err = db.Exec(`DROP INDEX idx_repos_identity`) + if err != nil { + return fmt.Errorf("drop old unique idx_repos_identity: %w", err) + } } - _, err = db.Exec(`CREATE UNIQUE INDEX IF NOT EXISTS idx_repos_identity ON repos(identity) WHERE identity IS NOT NULL`) + // Create non-unique index (or recreate after dropping unique) + _, err = db.Exec(`CREATE INDEX IF NOT EXISTS idx_repos_identity ON repos(identity) WHERE identity IS NOT NULL`) if err != nil { return fmt.Errorf("create idx_repos_identity: %w", err) } diff --git a/internal/storage/db_test.go b/internal/storage/db_test.go index 210f32b..15f3cdf 100644 --- a/internal/storage/db_test.go +++ b/internal/storage/db_test.go @@ -393,6 +393,67 @@ func TestMarkReviewAddressedNotFound(t *testing.T) { } } +func TestMarkReviewAddressedByJobID(t *testing.T) { + db := openTestDB(t) + defer db.Close() + + repo, _ := db.GetOrCreateRepo("/tmp/test-repo") + commit, _ := db.GetOrCreateCommit(repo.ID, "jobaddr123", "Author", "Subject", time.Now()) + job, _ := db.EnqueueJob(repo.ID, commit.ID, "jobaddr123", "codex", "", "") + db.ClaimJob("worker-1") + db.CompleteJob(job.ID, "codex", "prompt", "output") + + // Get the review to verify initial state + review, err := db.GetReviewByJobID(job.ID) + if err != nil { + t.Fatalf("GetReviewByJobID failed: %v", err) + } + + // Initially not addressed + if review.Addressed { + t.Error("Review should not be addressed initially") + } + + // Mark as addressed using job ID + err = db.MarkReviewAddressedByJobID(job.ID, true) + if err != nil { + t.Fatalf("MarkReviewAddressedByJobID failed: %v", err) + } + + // Verify it's addressed + updated, _ := db.GetReviewByJobID(job.ID) + if !updated.Addressed { + t.Error("Review should be addressed after MarkReviewAddressedByJobID(true)") + } + + // Mark as unaddressed using job ID + err = db.MarkReviewAddressedByJobID(job.ID, false) + if err != nil { + t.Fatalf("MarkReviewAddressedByJobID(false) failed: %v", err) + } + + updated2, _ := db.GetReviewByJobID(job.ID) + if updated2.Addressed { + t.Error("Review should not be addressed after MarkReviewAddressedByJobID(false)") + } +} + +func TestMarkReviewAddressedByJobIDNotFound(t *testing.T) { + db := openTestDB(t) + defer db.Close() + + // Try to mark a non-existent job + err := db.MarkReviewAddressedByJobID(999999, true) + if err == nil { + t.Fatal("Expected error for non-existent job") + } + + // Should be sql.ErrNoRows + if !errors.Is(err, sql.ErrNoRows) { + t.Errorf("Expected sql.ErrNoRows, got: %v", err) + } +} + func TestJobCounts(t *testing.T) { db := openTestDB(t) defer db.Close() @@ -1906,6 +1967,61 @@ func TestRepoIdentity(t *testing.T) { t.Errorf("Expected identity to remain 'original-identity', got %q", repo2.Identity) } }) + + t.Run("multiple clones with same identity allowed", func(t *testing.T) { + // This tests the fix for https://github.com/roborev-dev/roborev/issues/131 + // Multiple clones of the same repo (e.g., ~/project-1 and ~/project-2 both + // cloned from the same remote) should be allowed and share the same identity. + db := openTestDB(t) + defer db.Close() + + sharedIdentity := "git@github.com:org/shared-repo.git" + + // Create first clone + repo1, err := db.GetOrCreateRepo("/tmp/clone-1", sharedIdentity) + if err != nil { + t.Fatalf("GetOrCreateRepo for clone-1 failed: %v", err) + } + if repo1.Identity != sharedIdentity { + t.Errorf("Expected identity %q for clone-1, got %q", sharedIdentity, repo1.Identity) + } + + // Create second clone with same identity - should succeed (was failing before fix) + repo2, err := db.GetOrCreateRepo("/tmp/clone-2", sharedIdentity) + if err != nil { + t.Fatalf("GetOrCreateRepo for clone-2 failed: %v (multiple clones with same identity should be allowed)", err) + } + if repo2.Identity != sharedIdentity { + t.Errorf("Expected identity %q for clone-2, got %q", sharedIdentity, repo2.Identity) + } + + // Verify they are different repos + if repo1.ID == repo2.ID { + t.Errorf("Expected different repo IDs, but both are %d", repo1.ID) + } + if repo1.RootPath == repo2.RootPath { + t.Errorf("Expected different root paths") + } + + // Verify both repos exist and can be retrieved + repos, err := db.ListRepos() + if err != nil { + t.Fatalf("ListRepos failed: %v", err) + } + + foundClone1, foundClone2 := false, false + for _, r := range repos { + if r.ID == repo1.ID { + foundClone1 = true + } + if r.ID == repo2.ID { + foundClone2 = true + } + } + if !foundClone1 || !foundClone2 { + t.Errorf("Expected both clones to exist in ListRepos, found clone1=%v clone2=%v", foundClone1, foundClone2) + } + }) } func TestDuplicateSHAHandling(t *testing.T) { diff --git a/internal/storage/sync.go b/internal/storage/sync.go index 5d8d8f8..cebc3b2 100644 --- a/internal/storage/sync.go +++ b/internal/storage/sync.go @@ -621,29 +621,104 @@ func (db *DB) GetKnownJobUUIDs() ([]string, error) { return uuids, rows.Err() } -// GetOrCreateRepoByIdentity finds or creates a repo by identity. -// If the repo doesn't exist, creates it with a placeholder path. +// GetOrCreateRepoByIdentity finds or creates a repo for syncing by identity. +// The logic is: +// 1. If exactly one local repo has this identity, use it (always preferred) +// 2. If a placeholder repo exists (root_path == identity), use it +// 3. If 0 or 2+ local repos have this identity, create a placeholder +// +// This ensures synced jobs attach to the right repo: +// - Single clone: jobs attach directly to the local repo +// - Multiple clones: jobs attach to a neutral placeholder +// - No local clone: placeholder serves as a sync-only repo +// +// Note: Single local repos are always preferred, even if a placeholder exists +// from a previous sync (e.g., when there were 0 or 2+ clones before). func (db *DB) GetOrCreateRepoByIdentity(identity string) (int64, error) { - // Try to find existing repo - repo, err := db.GetRepoByIdentity(identity) + // First, check for local repos with this identity + // (excluding placeholders where root_path == identity) + rows, err := db.Query(`SELECT id FROM repos WHERE identity = ? AND root_path != ?`, identity, identity) if err != nil { - return 0, err + return 0, fmt.Errorf("find repos by identity: %w", err) + } + defer rows.Close() + + var repoIDs []int64 + for rows.Next() { + var id int64 + if err := rows.Scan(&id); err != nil { + return 0, fmt.Errorf("scan repo id: %w", err) + } + repoIDs = append(repoIDs, id) + } + if err := rows.Err(); err != nil { + return 0, fmt.Errorf("iterate repos: %w", err) + } + + // If exactly one local repo exists, always use it (even if placeholder exists) + if len(repoIDs) == 1 { + return repoIDs[0], nil } - if repo != nil { - return repo.ID, nil + + // 0 or 2+ local repos - look for existing placeholder + var placeholderID int64 + err = db.QueryRow(`SELECT id FROM repos WHERE root_path = ? AND identity = ?`, identity, identity).Scan(&placeholderID) + if err == nil { + return placeholderID, nil + } + if err != sql.ErrNoRows { + return 0, fmt.Errorf("find placeholder repo: %w", err) } - // Create a placeholder repo - path is identity since we don't know the local path + // No placeholder exists - create one + // Use extracted repo name for display, but root_path stays as identity to mark it as a placeholder + displayName := ExtractRepoNameFromIdentity(identity) result, err := db.Exec(` INSERT INTO repos (root_path, name, identity) VALUES (?, ?, ?) - `, identity, identity, identity) + `, identity, displayName, identity) if err != nil { - return 0, fmt.Errorf("create repo: %w", err) + return 0, fmt.Errorf("create placeholder repo: %w", err) } return result.LastInsertId() } +// ExtractRepoNameFromIdentity extracts a human-readable name from a git identity. +// Examples: +// - "git@github.com:org/repo.git" -> "repo" +// - "https://github.com/org/my-project.git" -> "my-project" +// - "https://github.com/org/repo" -> "repo" +// - "" -> "unknown" +func ExtractRepoNameFromIdentity(identity string) string { + // Handle empty identity + if identity == "" { + return "unknown" + } + + // Remove trailing .git if present + name := strings.TrimSuffix(identity, ".git") + + // Find the last path component + // Handle both SSH (git@host:path) and HTTPS (https://host/path) formats + if idx := strings.LastIndex(name, "/"); idx >= 0 { + name = name[idx+1:] + } else if idx := strings.LastIndex(name, ":"); idx >= 0 { + // SSH format like git@github.com:org/repo - get part after last / + afterColon := name[idx+1:] + if slashIdx := strings.LastIndex(afterColon, "/"); slashIdx >= 0 { + name = afterColon[slashIdx+1:] + } else { + name = afterColon + } + } + + // If we ended up with empty string, use the identity as-is + if name == "" { + return identity + } + return name +} + // GetOrCreateCommitByRepoAndSHA finds or creates a commit. func (db *DB) GetOrCreateCommitByRepoAndSHA(repoID int64, sha, author, subject string, timestamp time.Time) (int64, error) { // Try to find existing diff --git a/internal/storage/sync_test.go b/internal/storage/sync_test.go index 1c66f83..8a0e22f 100644 --- a/internal/storage/sync_test.go +++ b/internal/storage/sync_test.go @@ -367,10 +367,11 @@ func TestGetOrCreateRepoByIdentity(t *testing.T) { t.Fatalf("Query repo failed: %v", err) } if rootPath != localIdentity { - t.Errorf("Expected root_path %q, got %q", localIdentity, rootPath) + t.Errorf("Expected root_path %q (placeholder), got %q", localIdentity, rootPath) } - if name != localIdentity { - t.Errorf("Expected name %q, got %q", localIdentity, name) + // Name is extracted from identity + if name != "my-local-project" { + t.Errorf("Expected name 'my-local-project' (extracted), got %q", name) } if identity != localIdentity { t.Errorf("Expected identity %q, got %q", localIdentity, identity) @@ -419,16 +420,162 @@ func TestGetOrCreateRepoByIdentity(t *testing.T) { t.Fatalf("GetOrCreateRepoByIdentity failed: %v", err) } - // Verify identity is set correctly - var identity string - err = db.QueryRow(`SELECT identity FROM repos WHERE id = ?`, repoID).Scan(&identity) + // Verify identity is set correctly and name is extracted + var identity, name string + err = db.QueryRow(`SELECT identity, name FROM repos WHERE id = ?`, repoID).Scan(&identity, &name) if err != nil { t.Fatalf("Query repo failed: %v", err) } if identity != gitIdentity { t.Errorf("Expected identity %q, got %q", gitIdentity, identity) } + if name != "repo" { + t.Errorf("Expected name 'repo' (extracted from URL), got %q", name) + } + }) + + t.Run("reuses single local repo when one exists", func(t *testing.T) { + // When exactly one local repo has the identity, use it directly (no placeholder) + singleIdentity := "git@github.com:org/single-clone-repo.git" + + // Create one local clone with the identity + result, err := db.Exec(`INSERT INTO repos (root_path, name, identity) VALUES (?, ?, ?)`, + "/home/user/single-clone", "single-clone", singleIdentity) + if err != nil { + t.Fatalf("Insert single clone failed: %v", err) + } + localRepoID, _ := result.LastInsertId() + + // GetOrCreateRepoByIdentity should return the existing local repo, not create a placeholder + gotID, err := db.GetOrCreateRepoByIdentity(singleIdentity) + if err != nil { + t.Fatalf("GetOrCreateRepoByIdentity failed: %v", err) + } + if gotID != localRepoID { + t.Errorf("Expected to reuse local repo ID %d, got %d", localRepoID, gotID) + } + + // Verify no placeholder was created + var count int + err = db.QueryRow(`SELECT COUNT(*) FROM repos WHERE root_path = ?`, singleIdentity).Scan(&count) + if err != nil { + t.Fatalf("Count query failed: %v", err) + } + if count != 0 { + t.Errorf("Expected no placeholder to be created, found %d", count) + } }) + + t.Run("creates placeholder when multiple local clones exist", func(t *testing.T) { + // When multiple local clones have the same identity, create a placeholder + sharedIdentity := "git@github.com:org/shared-repo.git" + + // Create two local clones with the same identity + _, err := db.Exec(`INSERT INTO repos (root_path, name, identity) VALUES (?, ?, ?)`, + "/home/user/clone-1", "clone-1", sharedIdentity) + if err != nil { + t.Fatalf("Insert clone-1 failed: %v", err) + } + _, err = db.Exec(`INSERT INTO repos (root_path, name, identity) VALUES (?, ?, ?)`, + "/home/user/clone-2", "clone-2", sharedIdentity) + if err != nil { + t.Fatalf("Insert clone-2 failed: %v", err) + } + + // GetOrCreateRepoByIdentity should create a placeholder + placeholderID, err := db.GetOrCreateRepoByIdentity(sharedIdentity) + if err != nil { + t.Fatalf("GetOrCreateRepoByIdentity should succeed with duplicates, got: %v", err) + } + + // Verify it created a placeholder (root_path == identity) + var rootPath, name string + err = db.QueryRow(`SELECT root_path, name FROM repos WHERE id = ?`, placeholderID).Scan(&rootPath, &name) + if err != nil { + t.Fatalf("Query placeholder failed: %v", err) + } + if rootPath != sharedIdentity { + t.Errorf("Expected placeholder root_path %q, got %q", sharedIdentity, rootPath) + } + if name != "shared-repo" { + t.Errorf("Expected placeholder name 'shared-repo' (extracted), got %q", name) + } + + // Subsequent calls should return the same placeholder + placeholderID2, err := db.GetOrCreateRepoByIdentity(sharedIdentity) + if err != nil { + t.Fatalf("Second GetOrCreateRepoByIdentity failed: %v", err) + } + if placeholderID != placeholderID2 { + t.Errorf("Expected same placeholder ID, got %d and %d", placeholderID, placeholderID2) + } + }) + + t.Run("prefers single local repo over existing placeholder", func(t *testing.T) { + // This tests the fix for review #2658: when a placeholder exists from + // a previous sync (e.g., when there were 0 clones), but now there's + // exactly one local repo, we should prefer the local repo. + placeholderIdentity := "git@github.com:org/placeholder-then-clone.git" + + // First, create a placeholder (simulates sync with no local clone) + _, err := db.Exec(`INSERT INTO repos (root_path, name, identity) VALUES (?, ?, ?)`, + placeholderIdentity, "placeholder-then-clone", placeholderIdentity) + if err != nil { + t.Fatalf("Insert placeholder failed: %v", err) + } + + // Now create a single local clone (user cloned the repo after syncing) + result, err := db.Exec(`INSERT INTO repos (root_path, name, identity) VALUES (?, ?, ?)`, + "/home/user/new-clone", "new-clone", placeholderIdentity) + if err != nil { + t.Fatalf("Insert local clone failed: %v", err) + } + localRepoID, _ := result.LastInsertId() + + // GetOrCreateRepoByIdentity should return the local repo, not the placeholder + gotID, err := db.GetOrCreateRepoByIdentity(placeholderIdentity) + if err != nil { + t.Fatalf("GetOrCreateRepoByIdentity failed: %v", err) + } + if gotID != localRepoID { + t.Errorf("Expected to prefer local repo ID %d over placeholder, got %d", localRepoID, gotID) + } + }) +} + +func TestExtractRepoNameFromIdentity(t *testing.T) { + tests := []struct { + identity string + expected string + }{ + // SSH format + {"git@github.com:org/repo.git", "repo"}, + {"git@github.com:user/my-project.git", "my-project"}, + {"git@gitlab.com:group/subgroup/repo.git", "repo"}, + + // HTTPS format + {"https://github.com/org/repo.git", "repo"}, + {"https://github.com/org/repo", "repo"}, + {"https://gitlab.com/group/subgroup/project.git", "project"}, + + // Local format + {"local:my-project", "my-project"}, + {"local:another-repo", "another-repo"}, + + // Edge cases + {"repo.git", "repo"}, + {"repo", "repo"}, + {"", "unknown"}, + } + + for _, tt := range tests { + t.Run(tt.identity, func(t *testing.T) { + got := ExtractRepoNameFromIdentity(tt.identity) + if got != tt.expected { + t.Errorf("ExtractRepoNameFromIdentity(%q) = %q, want %q", tt.identity, got, tt.expected) + } + }) + } } func TestSetRepoIdentity(t *testing.T) { @@ -680,9 +827,10 @@ func openRawDB(dbPath string) (*sql.DB, error) { return sql.Open("sqlite", dbPath+"?_pragma=journal_mode(WAL)&_pragma=busy_timeout(5000)") } -func TestDuplicateRepoIdentity_MigrationError(t *testing.T) { - // This test verifies that migration fails with a clear error if duplicate - // non-NULL repos.identity values exist before creating the unique index. +func TestDuplicateRepoIdentity_MigrationSuccess(t *testing.T) { + // This test verifies that migration succeeds even when duplicate + // repos.identity values exist. Multiple clones of the same repo + // should be allowed (fix for https://github.com/roborev-dev/roborev/issues/131). dbPath := filepath.Join(t.TempDir(), "test.db") rawDB, err := openRawDB(dbPath) @@ -690,7 +838,7 @@ func TestDuplicateRepoIdentity_MigrationError(t *testing.T) { t.Fatalf("Failed to open raw database: %v", err) } - // Create schema with identity column but no unique index (simulates partial migration) + // Create schema with identity column but no index (simulates partial migration) _, err = rawDB.Exec(` CREATE TABLE repos ( id INTEGER PRIMARY KEY, @@ -751,13 +899,13 @@ func TestDuplicateRepoIdentity_MigrationError(t *testing.T) { t.Fatalf("Failed to create schema: %v", err) } - // Insert two repos with the same identity (duplicate) - _, err = rawDB.Exec(`INSERT INTO repos (root_path, name, identity) VALUES ('/repo1', 'repo1', 'dup-identity')`) + // Insert two repos with the same identity (e.g., two clones of same remote) + _, err = rawDB.Exec(`INSERT INTO repos (root_path, name, identity) VALUES ('/repo1', 'repo1', 'git@github.com:org/repo.git')`) if err != nil { rawDB.Close() t.Fatalf("Failed to insert repo1: %v", err) } - _, err = rawDB.Exec(`INSERT INTO repos (root_path, name, identity) VALUES ('/repo2', 'repo2', 'dup-identity')`) + _, err = rawDB.Exec(`INSERT INTO repos (root_path, name, identity) VALUES ('/repo2', 'repo2', 'git@github.com:org/repo.git')`) if err != nil { rawDB.Close() t.Fatalf("Failed to insert repo2: %v", err) @@ -765,33 +913,134 @@ func TestDuplicateRepoIdentity_MigrationError(t *testing.T) { rawDB.Close() - // Now open with storage.Open which runs migrations - should fail with clear error - _, err = Open(dbPath) - if err == nil { - t.Fatal("Expected migration to fail due to duplicate identities, but it succeeded") + // Now open with storage.Open which runs migrations - should succeed + db, err := Open(dbPath) + if err != nil { + t.Fatalf("Expected migration to succeed with duplicate identities, but got error: %v", err) + } + defer db.Close() + + // Verify both repos exist + repos, err := db.ListRepos() + if err != nil { + t.Fatalf("ListRepos failed: %v", err) } - if !regexp.MustCompile(`duplicate.*identit`).MatchString(err.Error()) { - t.Errorf("Expected error about duplicate identities, got: %v", err) + if len(repos) != 2 { + t.Errorf("Expected 2 repos, got %d", len(repos)) } } -func TestGetRepoByIdentity_DuplicateError(t *testing.T) { - // This test verifies GetRepoByIdentity returns an error if duplicates exist - // (which shouldn't happen with the unique index, but tests the code path) +func TestUniqueIndexMigration(t *testing.T) { + // This test verifies that an existing database with the old UNIQUE index + // on repos.identity is properly migrated to a non-unique index. + // See: https://github.com/roborev-dev/roborev/issues/131 dbPath := filepath.Join(t.TempDir(), "test.db") + + rawDB, err := openRawDB(dbPath) + if err != nil { + t.Fatalf("Failed to open raw database: %v", err) + } + + // Create schema with identity column AND the old unique index + _, err = rawDB.Exec(` + CREATE TABLE repos ( + id INTEGER PRIMARY KEY, + root_path TEXT UNIQUE NOT NULL, + name TEXT NOT NULL, + created_at TEXT NOT NULL DEFAULT (datetime('now')), + identity TEXT + ); + CREATE UNIQUE INDEX idx_repos_identity ON repos(identity) WHERE identity IS NOT NULL; + CREATE TABLE commits ( + id INTEGER PRIMARY KEY, + repo_id INTEGER NOT NULL REFERENCES repos(id), + sha TEXT NOT NULL, + author TEXT NOT NULL, + subject TEXT NOT NULL, + timestamp TEXT NOT NULL, + created_at TEXT NOT NULL DEFAULT (datetime('now')), + UNIQUE(repo_id, sha) + ); + CREATE TABLE review_jobs ( + id INTEGER PRIMARY KEY, + repo_id INTEGER NOT NULL REFERENCES repos(id), + commit_id INTEGER REFERENCES commits(id), + git_ref TEXT NOT NULL, + agent TEXT NOT NULL DEFAULT 'codex', + reasoning TEXT NOT NULL DEFAULT 'thorough', + status TEXT NOT NULL CHECK(status IN ('queued','running','done','failed','canceled')) DEFAULT 'queued', + enqueued_at TEXT NOT NULL DEFAULT (datetime('now')), + started_at TEXT, + finished_at TEXT, + worker_id TEXT, + error TEXT, + prompt TEXT, + retry_count INTEGER NOT NULL DEFAULT 0, + diff_content TEXT, + agentic INTEGER NOT NULL DEFAULT 0 + ); + CREATE TABLE reviews ( + id INTEGER PRIMARY KEY, + job_id INTEGER UNIQUE NOT NULL REFERENCES review_jobs(id), + agent TEXT NOT NULL, + prompt TEXT NOT NULL, + output TEXT NOT NULL, + created_at TEXT NOT NULL DEFAULT (datetime('now')), + addressed INTEGER NOT NULL DEFAULT 0 + ); + CREATE TABLE responses ( + id INTEGER PRIMARY KEY, + commit_id INTEGER REFERENCES commits(id), + job_id INTEGER REFERENCES review_jobs(id), + responder TEXT NOT NULL, + response TEXT NOT NULL, + created_at TEXT NOT NULL DEFAULT (datetime('now')) + ); + CREATE INDEX idx_commits_sha ON commits(sha); + `) + if err != nil { + rawDB.Close() + t.Fatalf("Failed to create schema: %v", err) + } + + // Insert one repo + _, err = rawDB.Exec(`INSERT INTO repos (root_path, name, identity) VALUES ('/repo1', 'repo1', 'git@github.com:org/repo.git')`) + if err != nil { + rawDB.Close() + t.Fatalf("Failed to insert repo1: %v", err) + } + + rawDB.Close() + + // Open with storage.Open which runs migrations db, err := Open(dbPath) if err != nil { - t.Fatalf("Failed to open database: %v", err) + t.Fatalf("Migration failed: %v", err) } - defer db.Close() - // Create two repos with different paths but we'll manually set same identity - // bypassing the unique constraint by using raw SQL after dropping the index - _, err = db.Exec(`DROP INDEX IF EXISTS idx_repos_identity`) + // Verify we can now insert a second repo with the same identity + // (this would fail if the unique index wasn't converted to non-unique) + _, err = db.Exec(`INSERT INTO repos (root_path, name, identity) VALUES ('/repo2', 'repo2', 'git@github.com:org/repo.git')`) if err != nil { - t.Fatalf("Failed to drop index: %v", err) + db.Close() + t.Fatalf("Inserting second repo with same identity should succeed after migration, but got: %v", err) } + db.Close() +} + +func TestGetRepoByIdentity_DuplicateError(t *testing.T) { + // This test verifies GetRepoByIdentity returns an error if duplicates exist. + // Multiple repos can share the same identity (e.g., multiple clones of the same remote), + // but GetRepoByIdentity should return an error when asked to find a unique repo. + dbPath := filepath.Join(t.TempDir(), "test.db") + db, err := Open(dbPath) + if err != nil { + t.Fatalf("Failed to open database: %v", err) + } + defer db.Close() + + // Create two repos with same identity (simulates two clones of the same remote) _, err = db.Exec(`INSERT INTO repos (root_path, name, identity) VALUES ('/path1', 'repo1', 'same-id')`) if err != nil { t.Fatalf("Failed to insert repo1: %v", err)