fix(github): preserve PR watch branch identities (#3649)

2026-09-14 14:59:28 +02:00
parent bb784a1d90
commit 0f4bf79b8d
10 changed files with 616 additions and 18 deletions
@@ -83,9 +83,9 @@ type TaskBranchInfo struct {
 // TaskBranchProvider lists tasks that should have PR watches and resolves branches.
 type TaskBranchProvider interface {
 	ListTasksNeedingPRWatch(ctx context.Context) ([]TaskBranchInfo, error)
-	// ResolveBranchForSession returns the current branch for a task+session pair.
-	// Used to detect branch renames and update stale PR watches.
-	ResolveBranchForSession(ctx context.Context, taskID, sessionID string) string
+	// ResolveBranchForWatch resolves a branch within the watch's source session and repository.
+	// An empty result leaves the watch unchanged.
+	ResolveBranchForWatch(ctx context.Context, watch *PRWatch) string
 }

 // Poller runs background loops for PR monitoring and review queue checking.
@@ -460,9 +460,7 @@ func (p *Poller) refreshStaleBranches(ctx context.Context) {
 		if watch.PRNumber != 0 {
 			continue // already found a PR, branch is correct
 		}
-		currentBranch := p.taskBranchProvider.ResolveBranchForSession(
-			ctx, watch.TaskID, watch.SessionID,
-		)
+		currentBranch := p.taskBranchProvider.ResolveBranchForWatch(ctx, watch)
 		if currentBranch == "" || currentBranch == watch.Branch {
 			continue
 		}
@@ -0,0 +1,126 @@
+package github
+
+import (
+	"context"
+	"testing"
+	"time"
+
+	"github.com/kandev/kandev/internal/events"
+	"github.com/kandev/kandev/internal/events/bus"
+)
+
+type watchIdentityProvider struct{ targets []TaskBranchInfo }
+
+func (p *watchIdentityProvider) ListTasksNeedingPRWatch(context.Context) ([]TaskBranchInfo, error) {
+	return p.targets, nil
+}
+func (p *watchIdentityProvider) ResolveBranchForWatch(_ context.Context, w *PRWatch) string {
+	for _, target := range p.targets {
+		if target.SessionID == w.SessionID && target.RepositoryID == w.RepositoryID && target.Branch == w.Branch {
+			return target.Branch
+		}
+	}
+	return ""
+}
+
+// @covers AC-INTEGRATIONS-GITHUB-PR-DISCOVERY-001.1
+// @covers AC-INTEGRATIONS-GITHUB-PR-DISCOVERY-001.4
+func TestReconcileWatches_PreservesBranchTargetsAcrossCycles(t *testing.T) {
+	poller, svc, client, store := setupBatchedPollerTest(t)
+	ctx := context.Background()
+	seedTask(t, store, "owner", false)
+	provider := &watchIdentityProvider{targets: []TaskBranchInfo{
+		{WorkspaceID: testWorkspaceID, TaskID: "owner", SessionID: "member-session", RepositoryID: "repo1", Owner: "o", Repo: "r", Branch: "primary"},
+		{WorkspaceID: testWorkspaceID, TaskID: "owner", SessionID: "member-session", RepositoryID: "repo1", Owner: "o", Repo: "r", Branch: "secondary"},
+		{WorkspaceID: testWorkspaceID, TaskID: "owner", SessionID: "member-session", RepositoryID: "repo2", Owner: "o", Repo: "other", Branch: "other-branch"},
+	}}
+	poller.SetTaskBranchProvider(provider)
+	poller.reconcileWatches(ctx)
+	initial, err := store.ListActivePRWatches(ctx)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(initial) != 3 {
+		t.Fatalf("initial watches = %d, want 3", len(initial))
+	}
+	// A numbered sibling must survive while the other branches still search.
+	for _, w := range initial {
+		if w.Branch == "primary" {
+			if err := store.UpdatePRWatchPRNumber(ctx, w.ID, 99); err != nil {
+				t.Fatal(err)
+			}
+		}
+	}
+	initial, err = store.ListActivePRWatches(ctx)
+	if err != nil {
+		t.Fatal(err)
+	}
+	assertWatchReconciliationStable(t, poller, store, initial)
+	assertSecondaryWatchDiscovery(t, svc, client, store)
+}
+
+func assertWatchReconciliationStable(t *testing.T, poller *Poller, store *Store, initial []*PRWatch) {
+	t.Helper()
+	ctx := context.Background()
+	for range 3 {
+		poller.reconcileWatches(ctx)
+		current, err := store.ListActivePRWatches(ctx)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if len(current) != len(initial) {
+			t.Fatalf("watch count = %d, want %d", len(current), len(initial))
+		}
+		for _, before := range initial {
+			after, err := store.GetPRWatchBySessionRepoAndBranch(ctx, before.SessionID, before.RepositoryID, before.Branch)
+			if err != nil {
+				t.Fatal(err)
+			}
+			if after == nil || after.ID != before.ID || !after.UpdatedAt.Equal(before.UpdatedAt) {
+				t.Fatalf("watch %s recreated or rewritten: before=%+v after=%+v", before.Branch, before, after)
+			}
+			if before.Branch == "primary" && after.PRNumber != 99 {
+				t.Fatal("numbered sibling changed")
+			}
+		}
+	}
+}
+
+func assertSecondaryWatchDiscovery(t *testing.T, svc *Service, client *graphQLMockClient, store *Store) {
+	t.Helper()
+	ctx := context.Background()
+	secondary, err := store.GetPRWatchBySessionRepoAndBranch(ctx, "member-session", "repo1", "secondary")
+	if err != nil {
+		t.Fatal(err)
+	}
+	updated := make(chan *TaskPR, 1)
+	sub, err := svc.eventBus.Subscribe(events.GitHubTaskPRUpdated, func(_ context.Context, event *bus.Event) error {
+		if pr, ok := event.Data.(*TaskPR); ok {
+			updated <- pr
+		}
+		return nil
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+	t.Cleanup(func() { _ = sub.Unsubscribe() })
+	client.branchResponses = []string{branchAliasResponse(t, 1, map[int]string{0: openPRNode(7, "secondary", "alice")})}
+	if _, err := svc.SyncWorkspaceWatchesBatched(ctx, testWorkspaceID, []*PRWatch{secondary}); err != nil {
+		t.Fatal(err)
+	}
+	associated, err := store.GetTaskPR(ctx, "owner")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if associated == nil || associated.PRNumber != 7 || associated.RepositoryID != "repo1" {
+		t.Fatalf("association = %+v", associated)
+	}
+	select {
+	case event := <-updated:
+		if event.TaskID != "owner" || event.PRNumber != 7 || event.RepositoryID != "repo1" {
+			t.Fatalf("event = %+v", event)
+		}
+	case <-time.After(time.Second):
+		t.Fatal("missing task PR event")
+	}
+}
@@ -248,9 +248,9 @@ func (m *mockTaskBranchProvider) ListTasksNeedingPRWatch(_ context.Context) ([]T
 	return m.tasks, m.err
 }

-func (m *mockTaskBranchProvider) ResolveBranchForSession(_ context.Context, _, sessionID string) string {
+func (m *mockTaskBranchProvider) ResolveBranchForWatch(_ context.Context, watch *PRWatch) string {
 	if m.branches != nil {
-		return m.branches[sessionID]
+		return m.branches[watch.SessionID]
 	}
 	return ""
 }
@@ -1444,12 +1444,6 @@ func (s *Service) ListTasksNeedingPRWatch(ctx context.Context) ([]github.TaskBra
 	return s.buildTaskBranchList(ctx, store)
 }

-// ResolveBranchForSession returns the current branch for a task+session.
-// This is used by the poller to detect branch renames on existing PR watches.
-func (s *Service) ResolveBranchForSession(ctx context.Context, taskID, sessionID string) string {
-	return s.resolvePRWatchBranch(ctx, taskID, sessionID, "")
-}
-
 // buildTaskBranchList walks sessions × their repositories and emits one
 // TaskBranchInfo per (session, repository) that doesn't already have a PR
 // watch. Multi-repo: previously dedup was keyed by sessionID, which silently
@@ -1472,11 +1466,15 @@ func (s *Service) buildTaskBranchList(ctx context.Context, store repoStore) ([]g
 	watchedKeys := s.buildWatchedSessionRepoSet(ctx)

 	var result []github.TaskBranchInfo
+	seenSessions := make(map[string]struct{})
 	for _, sess := range sessions {
-		// Pass sess.Branch as fallback for the primary repo (legacy callers
-		// stored the worktree branch on SessionBranchInfo); per-repo branches
-		// come from session.Worktrees inside resolveSessionWatchTargets.
-		targets := s.resolveSessionWatchTargets(ctx, sess.TaskID, sess.SessionID, sess.Branch)
+		if _, seen := seenSessions[sess.SessionID]; seen {
+			continue
+		}
+		seenSessions[sess.SessionID] = struct{}{}
+		// Inventory rows can carry any repository's branch. Resolve each session
+		// once from its per-repository targets, as watch refresh does.
+		targets := s.resolveSessionWatchTargets(ctx, sess.TaskID, sess.SessionID, "")
 		for _, t := range targets {
 			if watchedKeys[watchedSessionRepoKey(sess.SessionID, t.RepositoryID, t.Branch)] {
 				continue
@@ -0,0 +1,40 @@
+package orchestrator
+
+import (
+	"context"
+
+	"github.com/kandev/kandev/internal/github"
+)
+
+// ResolveBranchForWatch keeps checkout identity scoped to the source session.
+// The watch's attributed task can be a group owner with different branches.
+func (s *Service) ResolveBranchForWatch(ctx context.Context, watch *github.PRWatch) string {
+	if watch == nil || watch.SessionID == "" || watch.RepositoryID == "" {
+		return ""
+	}
+	session, err := s.repo.GetTaskSession(ctx, watch.SessionID)
+	if err != nil || session == nil {
+		return ""
+	}
+	targets := s.resolveSessionWatchTargets(ctx, session.TaskID, session.ID, "")
+	return matchingWatchBranch(targets, watch.RepositoryID, watch.Branch)
+}
+
+func matchingWatchBranch(targets []sessionWatchTarget, repositoryID, current string) string {
+	branches := make(map[string]struct{})
+	for _, target := range targets {
+		if target.RepositoryID != repositoryID || target.Branch == "" {
+			continue
+		}
+		if target.Branch == current {
+			return current
+		}
+		branches[target.Branch] = struct{}{}
+	}
+	if len(branches) == 1 {
+		for branch := range branches {
+			return branch
+		}
+	}
+	return ""
+}
@@ -0,0 +1,151 @@
+package orchestrator
+
+import (
+	"context"
+	"fmt"
+	"testing"
+
+	"github.com/kandev/kandev/internal/github"
+	"github.com/kandev/kandev/internal/task/models"
+	sqliterepo "github.com/kandev/kandev/internal/task/repository/sqlite"
+)
+
+// @covers AC-INTEGRATIONS-GITHUB-PR-DISCOVERY-001.4
+func TestResolvePRWatchBranchForWatch(t *testing.T) {
+	ctx := context.Background()
+	repo := setupTestRepo(t)
+	seedSession(t, repo, "t1", "s1", "step1")
+	seedGitHubPushAssocFixture(t, repo, "primary")
+	if err := repo.CreateRepository(ctx, &models.Repository{ID: "repo2", WorkspaceID: "ws1", Name: "secondary", Provider: "github", ProviderOwner: "myorg", ProviderName: "secondary"}); err != nil {
+		t.Fatal(err)
+	}
+	if err := repo.CreateTaskRepository(ctx, &models.TaskRepository{ID: "tr2", TaskID: "t1", RepositoryID: "repo2", CheckoutBranch: "secondary", Position: 1}); err != nil {
+		t.Fatal(err)
+	}
+	svc := createTestService(repo, newMockStepGetter(), newMockTaskRepo())
+	watch := &github.PRWatch{TaskID: "t1", SessionID: "s1", RepositoryID: "repo2", Branch: "secondary"}
+	if got := svc.ResolveBranchForWatch(ctx, nil); got != "" {
+		t.Fatalf("nil watch resolved to %q", got)
+	}
+	if got := svc.ResolveBranchForWatch(ctx, &github.PRWatch{RepositoryID: "repo1"}); got != "" {
+		t.Fatalf("missing session resolved to %q", got)
+	}
+	got := svc.ResolveBranchForWatch(ctx, watch)
+	if got != watch.Branch {
+		t.Fatalf("secondary watch resolved to %q, want %q", got, watch.Branch)
+	}
+}
+
+// @covers AC-INTEGRATIONS-GITHUB-PR-DISCOVERY-001.4
+func TestResolvePRWatchBranchForWatch_SourceIdentity(t *testing.T) {
+	for _, tt := range []struct {
+		name, session, repository, branch, want string
+		branches                                []string
+		cancelled                               bool
+	}{
+		{name: "configured checkout wins", session: "s1", repository: "repo1", branch: "old", want: "primary", branches: []string{"synthetic-review"}},
+		{name: "exact sibling survives", session: "s1", repository: "repo1", branch: "secondary", want: "secondary", branches: []string{"primary", "secondary"}},
+		{name: "ambiguous rename", session: "s1", repository: "repo1", branch: "old", branches: []string{"first", "second"}},
+		{name: "empty sibling is not candidate", session: "s1", repository: "repo1", branch: "old", want: "valid", branches: []string{"", "valid"}},
+		{name: "duplicate branches are one candidate", session: "s1", repository: "repo1", branch: "old", want: "valid", branches: []string{"valid", "valid"}},
+		{name: "missing repository", session: "s1", repository: "missing", branch: "old", branches: []string{"primary"}},
+		{name: "missing session", session: "missing", repository: "repo1", branch: "old"},
+		{name: "missing repository identity", session: "s1", branch: "old"},
+		{name: "cancelled read", session: "s1", repository: "repo1", branch: "old", cancelled: true},
+	} {
+		t.Run(tt.name, func(t *testing.T) {
+			ctx := context.Background()
+			repo := setupTestRepo(t)
+			seedSession(t, repo, "t1", "s1", "step1")
+			seedGitHubPushAssocFixture(t, repo, "primary")
+			owner, err := repo.GetTask(ctx, "t1")
+			if err != nil {
+				t.Fatal(err)
+			}
+			owner.ID = "attributed-group-owner"
+			if err := repo.CreateTask(ctx, owner); err != nil {
+				t.Fatal(err)
+			}
+			if err := repo.CreateTaskRepository(ctx, &models.TaskRepository{ID: "owner-tr", TaskID: "attributed-group-owner", RepositoryID: "repo1", CheckoutBranch: "owner-primary"}); err != nil {
+				t.Fatal(err)
+			}
+			repos := make([]*models.TaskEnvironmentRepo, 0, len(tt.branches))
+			for i, branch := range tt.branches {
+				repos = append(repos, &models.TaskEnvironmentRepo{ID: fmt.Sprintf("wt%d", i), RepositoryID: "repo1", WorktreeID: fmt.Sprintf("tree%d", i), BranchSlug: fmt.Sprintf("branch%d", i), WorktreeBranch: branch, Status: "active", Position: i})
+			}
+			if len(repos) > 0 {
+				attachWatchTestEnvironment(t, repo, repos)
+			}
+			svc := createTestService(repo, newMockStepGetter(), newMockTaskRepo())
+			if tt.cancelled {
+				cancelled, cancel := context.WithCancel(ctx)
+				cancel()
+				ctx = cancelled
+			}
+			watch := &github.PRWatch{TaskID: "attributed-group-owner", SessionID: tt.session, RepositoryID: tt.repository, Branch: tt.branch}
+			got := svc.ResolveBranchForWatch(ctx, watch)
+			if got != tt.want {
+				t.Fatalf("branch = %q, want %q", got, tt.want)
+			}
+		})
+	}
+}
+
+func attachWatchTestEnvironment(t *testing.T, repo *sqliterepo.Repository, repos []*models.TaskEnvironmentRepo) {
+	t.Helper()
+	ctx := context.Background()
+	if err := repo.CreateTaskEnvironment(ctx, &models.TaskEnvironment{ID: "env1", TaskID: "t1", ExecutorType: "worktree", WorkspacePath: t.TempDir(), Status: models.TaskEnvironmentStatusReady, Repos: repos}); err != nil {
+		t.Fatal(err)
+	}
+	session, err := repo.GetTaskSession(ctx, "s1")
+	if err != nil {
+		t.Fatal(err)
+	}
+	session.TaskEnvironmentID = "env1"
+	if err := repo.UpdateTaskSession(ctx, session); err != nil {
+		t.Fatal(err)
+	}
+}
+
+// @covers AC-INTEGRATIONS-GITHUB-PR-DISCOVERY-001.4
+func TestResolvePRWatchBranchForWatch_CreationUsesSameTargets(t *testing.T) {
+	ctx := context.Background()
+	repo := setupTestRepo(t)
+	seedSession(t, repo, "t1", "s1", "step1")
+	seedGitHubPushAssocFixture(t, repo, "")
+	if err := repo.CreateRepository(ctx, &models.Repository{ID: "repo2", WorkspaceID: "ws1", Name: "secondary", Provider: "github", ProviderOwner: "myorg", ProviderName: "secondary"}); err != nil {
+		t.Fatal(err)
+	}
+	if err := repo.CreateTaskRepository(ctx, &models.TaskRepository{ID: "tr2", TaskID: "t1", RepositoryID: "repo2", Position: 1}); err != nil {
+		t.Fatal(err)
+	}
+	attachWatchTestEnvironment(t, repo, []*models.TaskEnvironmentRepo{
+		{ID: "wt1", RepositoryID: "repo1", WorktreeID: "tree1", WorktreeBranch: "primary", Status: "active"},
+		{ID: "wt2", RepositoryID: "repo2", WorktreeID: "tree2", WorktreeBranch: "secondary", Status: "active", Position: 1},
+	})
+	svc := createTestService(repo, newMockStepGetter(), newMockTaskRepo())
+	svc.SetGitHubService(&mockGitHubService{})
+	targets, err := svc.ListTasksNeedingPRWatch(ctx)
+	if err != nil {
+		t.Fatal(err)
+	}
+	seen := make(map[string]bool)
+	for _, target := range targets {
+		want := map[string]string{"repo1": "primary", "repo2": "secondary"}[target.RepositoryID]
+		if target.Branch != want {
+			t.Fatalf("creation target = %+v, want branch %q", target, want)
+		}
+		key := target.SessionID + "/" + target.RepositoryID + "/" + target.Branch
+		if seen[key] {
+			t.Fatalf("duplicate creation target %s", key)
+		}
+		seen[key] = true
+		watch := &github.PRWatch{SessionID: target.SessionID, TaskID: target.TaskID, RepositoryID: target.RepositoryID, Branch: target.Branch}
+		if got := svc.ResolveBranchForWatch(ctx, watch); got != want {
+			t.Fatalf("refresh = %q, want %q", got, want)
+		}
+	}
+	if len(targets) != 2 {
+		t.Fatalf("targets = %d, want 2", len(targets))
+	}
+}