diff --git a/apps/backend/internal/backendapp/gateway.go b/apps/backend/internal/backendapp/gateway.go index 1930c3797d..9355b3c69d 100644 --- a/apps/backend/internal/backendapp/gateway.go +++ b/apps/backend/internal/backendapp/gateway.go @@ -204,8 +204,8 @@ func provideGateway( } } if gitlabSvc != nil { - router.associateGitLab = func(ctx context.Context, workspaceID, taskID, repositoryID, mrURL string) error { - _, err := gitlabSvc.AssociateExistingMRByURL(ctx, workspaceID, taskID, repositoryID, mrURL) + router.associateGitLab = func(ctx context.Context, workspaceID, sessionID, taskID, repositoryID, mrURL string) error { + _, err := gitlabSvc.AssociateExistingMRByURLForSession(ctx, workspaceID, sessionID, taskID, repositoryID, mrURL) return err } } @@ -502,7 +502,7 @@ type createdChangeAssociationRouter struct { resolveRepositoryID func(context.Context, string, string) string resolveWorkspaceID func(context.Context, string) (string, error) associateGitHub func(context.Context, string, string, string, string, string, string) error - associateGitLab func(context.Context, string, string, string, string) error + associateGitLab func(context.Context, string, string, string, string, string) error } func (r createdChangeAssociationRouter) associate( @@ -528,7 +528,7 @@ func (r createdChangeAssociationRouter) associate( if err != nil { return err } - return r.associateGitLab(ctx, workspaceID, taskID, repositoryID, changeURL) + return r.associateGitLab(ctx, workspaceID, sessionID, taskID, repositoryID, changeURL) case "github", "": if r.associateGitHub == nil { return nil diff --git a/apps/backend/internal/backendapp/gateway_git_association_test.go b/apps/backend/internal/backendapp/gateway_git_association_test.go index 713e10f883..4cb0427871 100644 --- a/apps/backend/internal/backendapp/gateway_git_association_test.go +++ b/apps/backend/internal/backendapp/gateway_git_association_test.go @@ -65,8 +65,8 @@ func TestCreatedChangeAssociationRouterRoutesGitLabSingleAndMultiRepo(t *testing return "repo-" + repo }, resolveWorkspaceID: func(context.Context, string) (string, error) { return "workspace-1", nil }, - associateGitLab: func(_ context.Context, workspaceID, taskID, repositoryID, mrURL string) error { - got = append(got, workspaceID+"|"+taskID+"|"+repositoryID+"|"+mrURL) + associateGitLab: func(_ context.Context, workspaceID, sessionID, taskID, repositoryID, mrURL string) error { + got = append(got, workspaceID+"|"+sessionID+"|"+taskID+"|"+repositoryID+"|"+mrURL) return nil }, } @@ -76,8 +76,8 @@ func TestCreatedChangeAssociationRouterRoutesGitLabSingleAndMultiRepo(t *testing } } want := []string{ - "workspace-1|task-1|repo-primary|https://gitlab.example/g/r/-/merge_requests/4", - "workspace-1|task-1|repo-secondary|https://gitlab.example/g/r/-/merge_requests/4", + "workspace-1|session-1|task-1|repo-primary|https://gitlab.example/g/r/-/merge_requests/4", + "workspace-1|session-1|task-1|repo-secondary|https://gitlab.example/g/r/-/merge_requests/4", } if len(got) != len(want) || got[0] != want[0] || got[1] != want[1] { t.Fatalf("associations = %#v", got) @@ -112,7 +112,7 @@ func TestCreatedChangeAssociationRouterGitLabFailureCanRetry(t *testing.T) { router := createdChangeAssociationRouter{ resolveRepositoryID: func(context.Context, string, string) string { return "repo-1" }, resolveWorkspaceID: func(context.Context, string) (string, error) { return "workspace-1", nil }, - associateGitLab: func(context.Context, string, string, string, string) error { + associateGitLab: func(context.Context, string, string, string, string, string) error { attempts++ if attempts == 1 { return errors.New("temporary failure containing sensitive provider output") diff --git a/apps/backend/internal/backendapp/main.go b/apps/backend/internal/backendapp/main.go index 295d9215ce..619c3eafd9 100644 --- a/apps/backend/internal/backendapp/main.go +++ b/apps/backend/internal/backendapp/main.go @@ -547,6 +547,7 @@ func startAgentInfrastructure( // orchestrator so review/issue watch events get turned into tasks. if services.GitLab != nil { orchestratorSvc.SetGitLabService(services.GitLab) + orchestratorSvc.SetGitLabMRLinkService(services.GitLab) orchestratorSvc.SetGitLabCredentialResolver(services.GitLab) services.GitLab.SetTaskDeleter(&taskDeleterAdapter{svc: services.Task}) services.GitLab.SetTaskSessionChecker(&taskSessionCheckerAdapter{repo: repos.Task}) diff --git a/apps/backend/internal/gitlab/service_check_mr_watch_refresh_test.go b/apps/backend/internal/gitlab/service_check_mr_watch_refresh_test.go new file mode 100644 index 0000000000..d79776c205 --- /dev/null +++ b/apps/backend/internal/gitlab/service_check_mr_watch_refresh_test.go @@ -0,0 +1,317 @@ +package gitlab + +import ( + "context" + "errors" + "testing" + + "github.com/kandev/kandev/internal/events" + "github.com/kandev/kandev/internal/events/bus" +) + +func TestCheckMRWatch_RefreshesTaskMRAndPublishes(t *testing.T) { + store := newTestStore(t) + ctx := context.Background() + seedTaskMRLinkFixture(t, store, "ws-1", "task-1", "repo-1") + setTaskMRRepositoryIdentity(t, store, "repo-1", "https://gitlab.com", "group/proj") + + client := NewMockClient("https://gitlab.com") + client.SeedMR("group/proj", &MR{ + IID: 5, State: mrStateOpen, HeadBranch: "feat/a", Title: "Feat A", + WebURL: "https://gitlab.com/group/proj/-/merge_requests/5", + }) + + svc := NewService("https://gitlab.com", client, "none", nil, newTestLogger(t)) + svc.SetStore(store) + log := newTestLogger(t) + eventBus := bus.NewMemoryEventBus(log) + svc.SetEventBus(eventBus) + + received := make(chan *bus.Event, 1) + if _, err := eventBus.Subscribe(events.GitLabTaskMRUpdated, func(_ context.Context, evt *bus.Event) error { + received <- evt + return nil + }); err != nil { + t.Fatalf("subscribe: %v", err) + } + + watch := &MRWatch{ + SessionID: "sess-1", TaskID: "task-1", RepositoryID: "repo-1", + ProjectPath: "group/proj", MRIID: 5, Branch: "feat/a", + } + if err := store.CreateMRWatch(ctx, watch); err != nil { + t.Fatalf("create watch: %v", err) + } + + status, notable, err := svc.CheckMRWatch(ctx, watch) + if err != nil { + t.Fatalf("check watch: %v", err) + } + if status == nil || status.MR == nil || status.MR.IID != 5 { + t.Fatalf("unexpected status: %+v", status) + } + _ = notable + + mrs, err := store.ListTaskMRsByTask(ctx, "task-1") + if err != nil { + t.Fatalf("list task MRs: %v", err) + } + if len(mrs) != 1 { + t.Fatalf("expected 1 task MR row after refresh, got %d", len(mrs)) + } + if mrs[0].MRIID != 5 || mrs[0].RepositoryID != "repo-1" || mrs[0].ProjectPath != "group/proj" { + t.Fatalf("unexpected task MR row: %+v", mrs[0]) + } + + select { + case evt := <-received: + if evt == nil { + t.Fatal("expected non-nil published event") + } + default: + t.Fatal("expected gitlab.task_mr.updated to be published") + } +} + +// TestCheckMRWatch_RejectsMismatchedIdentityOnRefresh guards the refresh path +// the same way AutoLinkMRForBranch and AssociateExistingMRByURL already are: +// a status response whose MR doesn't actually match the watch's (host, +// project, iid) — a client bug, or a race where the watch's mr_iid was +// updated between fetch and refresh — must not create or overwrite the +// task-MR association. +func TestCheckMRWatch_RejectsMismatchedIdentityOnRefresh(t *testing.T) { + store := newTestStore(t) + ctx := context.Background() + seedTaskMRLinkFixture(t, store, "ws-1", "task-1", "repo-1") + setTaskMRRepositoryIdentity(t, store, "repo-1", "https://gitlab.com", "group/proj") + + client := NewMockClient("https://gitlab.com") + svc := NewService("https://gitlab.com", client, "none", nil, newTestLogger(t)) + svc.SetStore(store) + + watch := &MRWatch{ + SessionID: "sess-1", TaskID: "task-1", RepositoryID: "repo-1", + ProjectPath: "group/proj", MRIID: 5, Branch: "feat/a", + } + if err := store.CreateMRWatch(ctx, watch); err != nil { + t.Fatalf("create watch: %v", err) + } + + // GetMRStatus is keyed by (projectPath, iid) — the watch's own + // (group/proj, 5) — but the seeded MR's WebURL points at a different + // project, simulating a misbehaving/mismatched client response. + client.SeedMR("group/proj", &MR{ + IID: 5, State: mrStateOpen, HeadBranch: "feat/a", Title: "Wrong URL", + WebURL: "https://gitlab.com/group/other/-/merge_requests/5", + }) + + if _, _, err := svc.CheckMRWatch(ctx, watch); err != nil { + t.Fatalf("check watch: %v", err) + } + + mrs, err := store.ListTaskMRsByTask(ctx, "task-1") + if err != nil { + t.Fatalf("list task MRs: %v", err) + } + if len(mrs) != 0 { + t.Fatalf("expected no task MR row for a mismatched identity response, got %+v", mrs) + } +} + +// TestCheckMRWatch_MismatchedIdentityDoesNotOverwriteExistingRow covers the +// other half of the same guarantee: a mismatched refresh must not just skip +// creating a row, it must leave an already-correct row untouched rather than +// clobbering it with the mismatched response's fields. +func TestCheckMRWatch_MismatchedIdentityDoesNotOverwriteExistingRow(t *testing.T) { + store := newTestStore(t) + ctx := context.Background() + seedTaskMRLinkFixture(t, store, "ws-1", "task-1", "repo-1") + setTaskMRRepositoryIdentity(t, store, "repo-1", "https://gitlab.com", "group/proj") + + client := NewMockClient("https://gitlab.com") + svc := NewService("https://gitlab.com", client, "none", nil, newTestLogger(t)) + svc.SetStore(store) + + watch := &MRWatch{ + SessionID: "sess-1", TaskID: "task-1", RepositoryID: "repo-1", + ProjectPath: "group/proj", MRIID: 5, Branch: "feat/a", + } + if err := store.CreateMRWatch(ctx, watch); err != nil { + t.Fatalf("create watch: %v", err) + } + + existing := &TaskMR{ + TaskID: "task-1", RepositoryID: "repo-1", Host: "https://gitlab.com", + ProjectPath: "group/proj", MRIID: 5, MRTitle: "Correct Title", + HeadBranch: "feat/a", State: mrStateOpen, + } + if err := store.UpsertTaskMR(ctx, existing); err != nil { + t.Fatalf("seed existing task MR: %v", err) + } + + // Same mismatched-identity response as the sibling test above: seeded + // MR's WebURL points at a different project than the watch's own. + client.SeedMR("group/proj", &MR{ + IID: 5, State: mrStateOpen, HeadBranch: "feat/a", Title: "Wrong URL", + WebURL: "https://gitlab.com/group/other/-/merge_requests/5", + }) + + if _, _, err := svc.CheckMRWatch(ctx, watch); err != nil { + t.Fatalf("check watch: %v", err) + } + + mrs, err := store.ListTaskMRsByTask(ctx, "task-1") + if err != nil { + t.Fatalf("list task MRs: %v", err) + } + if len(mrs) != 1 || mrs[0].MRTitle != "Correct Title" { + t.Fatalf("expected existing row to remain unchanged, got %+v", mrs) + } +} + +// TestCheckMRWatch_DoesNotRepublishWhenNothingVisibleChanged covers the +// smaller follow-up cubic-dev-ai raised alongside the identity-revalidation +// finding: refreshTaskMRFromWatch runs on every poll, not just "notable" +// pipeline/approval transitions, so publishing gitlab.task_mr.updated +// unconditionally on every poll would broadcast far more WS events than +// anything actually changed. A second poll with byte-identical MR data must +// not publish again. +func TestCheckMRWatch_DoesNotRepublishWhenNothingVisibleChanged(t *testing.T) { + store := newTestStore(t) + ctx := context.Background() + seedTaskMRLinkFixture(t, store, "ws-1", "task-1", "repo-1") + setTaskMRRepositoryIdentity(t, store, "repo-1", "https://gitlab.com", "group/proj") + + client := NewMockClient("https://gitlab.com") + client.SeedMR("group/proj", &MR{ + IID: 5, State: mrStateOpen, HeadBranch: "feat/a", Title: "Feat A", + WebURL: "https://gitlab.com/group/proj/-/merge_requests/5", + }) + + svc := NewService("https://gitlab.com", client, "none", nil, newTestLogger(t)) + svc.SetStore(store) + eventBus := bus.NewMemoryEventBus(newTestLogger(t)) + svc.SetEventBus(eventBus) + + received := make(chan *bus.Event, 4) + if _, err := eventBus.Subscribe(events.GitLabTaskMRUpdated, func(_ context.Context, evt *bus.Event) error { + received <- evt + return nil + }); err != nil { + t.Fatalf("subscribe: %v", err) + } + + watch := &MRWatch{ + SessionID: "sess-1", TaskID: "task-1", RepositoryID: "repo-1", + ProjectPath: "group/proj", MRIID: 5, Branch: "feat/a", + } + if err := store.CreateMRWatch(ctx, watch); err != nil { + t.Fatalf("create watch: %v", err) + } + + if _, _, err := svc.CheckMRWatch(ctx, watch); err != nil { + t.Fatalf("check watch 1: %v", err) + } + if _, _, err := svc.CheckMRWatch(ctx, watch); err != nil { + t.Fatalf("check watch 2: %v", err) + } + + if len(received) != 1 { + t.Fatalf("expected exactly 1 published event across 2 identical polls, got %d", len(received)) + } +} + +// TestCheckMRWatch_DoesNotDeleteWatchOnNotFoundError guards the coderabbitai +// finding: ValidateTaskMRRepositoryIdentity returns ErrTaskMRNotFound (and +// wraps generic store errors) as well as the direct ErrTaskMRRepositoryMismatch +// sentinel. Only the mismatch sentinel proves the watch is durably wrong; +// ErrTaskMRNotFound can be a transient race (e.g. the task_repositories link +// row not yet visible) or a real backend problem, either of which should be +// retried rather than destroying the watch. Here the watch's repository_id +// points at a repository that was never linked to the task at all — a +// not-found, not a mismatch — so the watch must survive and the error must +// propagate instead of being swallowed as a clean "not valid" result. +func TestCheckMRWatch_DoesNotDeleteWatchOnNotFoundError(t *testing.T) { + store := newTestStore(t) + ctx := context.Background() + // No repository row at all — repo-1 is never linked via task_repositories. + seedTaskMRLinkFixture(t, store, "ws-1", "task-1", "") + + client := NewMockClient("https://gitlab.com") + svc := NewService("https://gitlab.com", client, "none", nil, newTestLogger(t)) + svc.SetStore(store) + + watch := &MRWatch{ + SessionID: "sess-1", TaskID: "task-1", RepositoryID: "repo-1", + ProjectPath: "group/proj", MRIID: 5, Branch: "feat/a", + } + if err := store.CreateMRWatch(ctx, watch); err != nil { + t.Fatalf("create watch: %v", err) + } + + _, _, err := svc.CheckMRWatch(ctx, watch) + if !errors.Is(err, ErrTaskMRNotFound) { + t.Fatalf("expected ErrTaskMRNotFound to propagate, got %v", err) + } + + remaining, err := store.GetMRWatchBySessionRepoAndBranch(ctx, "sess-1", "repo-1", "feat/a") + if err != nil { + t.Fatalf("list watches: %v", err) + } + if remaining == nil { + t.Fatal("expected the watch to survive a not-found (non-mismatch) error, but it was deleted") + } +} + +// TestCheckMRWatch_DeletesWatchWhoseRepositoryNoLongerMatchesTask guards the +// gap cubic-dev-ai flagged: refreshTaskMRFromWatch only validates the MR +// *returned by GitLab* against the watch's own (host, project, iid) — that +// alone still lets a stale or mis-scoped watch (whose repository_id was +// never actually tied to this GitLab project) poll and self-consistently +// upsert data anyway, since GetMRStatus itself only cares about project path +// and iid, not repository_id. The watch's own repository identity must be +// validated against the task before any GitLab request or upsert, and a +// mismatched watch is deleted rather than left to retry forever. +func TestCheckMRWatch_DeletesWatchWhoseRepositoryNoLongerMatchesTask(t *testing.T) { + store := newTestStore(t) + ctx := context.Background() + seedTaskMRLinkFixture(t, store, "ws-1", "task-1", "repo-1") + // repo-1 belongs to a different GitLab project than the watch below. + setTaskMRRepositoryIdentity(t, store, "repo-1", "https://gitlab.com", "group/other") + + client := NewMockClient("https://gitlab.com") + client.SeedMR("group/proj", &MR{ + IID: 5, State: mrStateOpen, HeadBranch: "feat/a", Title: "Feat A", + WebURL: "https://gitlab.com/group/proj/-/merge_requests/5", + }) + svc := NewService("https://gitlab.com", client, "none", nil, newTestLogger(t)) + svc.SetStore(store) + + watch := &MRWatch{ + SessionID: "sess-1", TaskID: "task-1", RepositoryID: "repo-1", + ProjectPath: "group/proj", MRIID: 5, Branch: "feat/a", + } + if err := store.CreateMRWatch(ctx, watch); err != nil { + t.Fatalf("create watch: %v", err) + } + + status, notable, err := svc.CheckMRWatch(ctx, watch) + if err != nil { + t.Fatalf("check watch: %v", err) + } + if status != nil || notable { + t.Fatalf("expected no status for a watch whose repository doesn't match the task, got status=%+v notable=%v", status, notable) + } + + mrs, err := store.ListTaskMRsByTask(ctx, "task-1") + if err != nil || len(mrs) != 0 { + t.Fatalf("expected no task MR row created, got %d rows err=%v", len(mrs), err) + } + remaining, err := store.GetMRWatchBySessionRepoAndBranch(ctx, "sess-1", "repo-1", "feat/a") + if err != nil { + t.Fatalf("list watches: %v", err) + } + if remaining != nil { + t.Fatalf("expected the mismatched watch to be deleted, got %+v", remaining) + } +} diff --git a/apps/backend/internal/gitlab/service_ensure_mr_watch_concurrency_test.go b/apps/backend/internal/gitlab/service_ensure_mr_watch_concurrency_test.go new file mode 100644 index 0000000000..b8f8791414 --- /dev/null +++ b/apps/backend/internal/gitlab/service_ensure_mr_watch_concurrency_test.go @@ -0,0 +1,66 @@ +package gitlab + +import ( + "context" + "sync" + "testing" +) + +// EnsureMRWatch is a SELECT-then-INSERT, so concurrent callers for one +// (session, repository, branch) triple can all miss the existing row and race +// into the INSERT, where UNIQUE(session_id, repository_id, branch) rejects +// every loser. That race is reachable in production: push detection runs in +// its own goroutine while the on-demand gitlab.check_session_mr action can be +// answering for the same session. Losers must resolve to the winner's row +// rather than surfacing a constraint error. +func TestEnsureMRWatch_ConcurrentCallsAreSafe(t *testing.T) { + svc := newServiceWithStore(t) + store := svc.requireStore() + ctx := context.Background() + + const goroutines = 8 + var wg sync.WaitGroup + errs := make([]error, goroutines) + ids := make([]string, goroutines) + start := make(chan struct{}) + + for i := 0; i < goroutines; i++ { + wg.Add(1) + go func(idx int) { + defer wg.Done() + <-start + w, err := svc.EnsureMRWatch(ctx, "sess-1", "task-1", "repo-1", "group/proj", 7, "feat/a") + errs[idx] = err + if w != nil { + ids[idx] = w.ID + } + }(i) + } + close(start) + wg.Wait() + + for i, err := range errs { + if err != nil { + t.Errorf("goroutine %d: EnsureMRWatch returned %v, want nil", i, err) + } + } + + // Every caller must observe the same single row. + for i, id := range ids { + if id == "" { + t.Errorf("goroutine %d: got no watch back", i) + continue + } + if id != ids[0] { + t.Errorf("goroutine %d returned watch %q, want the single shared row %q", i, id, ids[0]) + } + } + + var count int + if err := store.ro.Get(&count, `SELECT COUNT(*) FROM gitlab_mr_watches`); err != nil { + t.Fatalf("count watches: %v", err) + } + if count != 1 { + t.Fatalf("expected exactly 1 watch row after %d concurrent calls, got %d", goroutines, count) + } +} diff --git a/apps/backend/internal/gitlab/service_ensure_mr_watch_test.go b/apps/backend/internal/gitlab/service_ensure_mr_watch_test.go new file mode 100644 index 0000000000..f0c81db9c3 --- /dev/null +++ b/apps/backend/internal/gitlab/service_ensure_mr_watch_test.go @@ -0,0 +1,136 @@ +package gitlab + +import ( + "context" + "testing" +) + +func TestEnsureMRWatch_CreatesWhenMissing(t *testing.T) { + store := newTestStore(t) + svc := NewService("", nil, "none", nil, newTestLogger(t)) + svc.SetStore(store) + ctx := context.Background() + + w, err := svc.EnsureMRWatch(ctx, "sess-1", "task-1", "repo-1", "group/proj", 5, "feat/a") + if err != nil { + t.Fatalf("ensure watch: %v", err) + } + if w == nil || w.ID == "" || w.MRIID != 5 || w.Branch != "feat/a" { + t.Fatalf("unexpected watch: %+v", w) + } + + got, err := store.GetMRWatchBySessionRepoAndBranch(ctx, "sess-1", "repo-1", "feat/a") + if err != nil || got == nil || got.ID != w.ID { + t.Fatalf("watch not persisted: %+v err=%v", got, err) + } +} + +func TestEnsureMRWatch_IdempotentReturnsExistingRow(t *testing.T) { + store := newTestStore(t) + svc := NewService("", nil, "none", nil, newTestLogger(t)) + svc.SetStore(store) + ctx := context.Background() + + first, err := svc.EnsureMRWatch(ctx, "sess-1", "task-1", "repo-1", "group/proj", 5, "feat/a") + if err != nil { + t.Fatalf("first ensure: %v", err) + } + second, err := svc.EnsureMRWatch(ctx, "sess-1", "task-1", "repo-1", "group/proj", 5, "feat/a") + if err != nil { + t.Fatalf("second ensure: %v", err) + } + if second.ID != first.ID { + t.Fatalf("expected same watch row, got %s and %s", first.ID, second.ID) + } + + all, err := store.ListMRWatchesBySession(ctx, "sess-1") + if err != nil || len(all) != 1 { + t.Fatalf("expected exactly 1 watch row, got %d err=%v", len(all), err) + } +} + +func TestEnsureMRWatch_UpdatesIIDWhenPreviouslyUnknown(t *testing.T) { + store := newTestStore(t) + svc := NewService("", nil, "none", nil, newTestLogger(t)) + svc.SetStore(store) + ctx := context.Background() + + first, err := svc.EnsureMRWatch(ctx, "sess-1", "task-1", "repo-1", "group/proj", 0, "feat/a") + if err != nil { + t.Fatalf("first ensure (unknown iid): %v", err) + } + if first.MRIID != 0 { + t.Fatalf("expected iid 0 on first ensure, got %d", first.MRIID) + } + + second, err := svc.EnsureMRWatch(ctx, "sess-1", "task-1", "repo-1", "group/proj", 9, "feat/a") + if err != nil { + t.Fatalf("second ensure (now known iid): %v", err) + } + if second.ID != first.ID { + t.Fatalf("expected same watch row updated in place, got %s and %s", first.ID, second.ID) + } + if second.MRIID != 9 { + t.Fatalf("expected iid backfilled to 9, got %d", second.MRIID) + } + + got, err := store.GetMRWatchBySessionRepoAndBranch(ctx, "sess-1", "repo-1", "feat/a") + if err != nil || got == nil || got.MRIID != 9 { + t.Fatalf("persisted iid not updated: %+v err=%v", got, err) + } +} + +// TestEnsureMRWatch_ReplacesIIDWhenBranchRelinkedToNewMR covers a watch that +// already knows a positive iid, not just the "previously unknown" case: the +// original MR closed and a new one opened on the same branch. Without +// replacing a known-but-stale iid, the watch would keep polling the closed +// MR forever. +func TestEnsureMRWatch_ReplacesIIDWhenBranchRelinkedToNewMR(t *testing.T) { + store := newTestStore(t) + svc := NewService("", nil, "none", nil, newTestLogger(t)) + svc.SetStore(store) + ctx := context.Background() + + first, err := svc.EnsureMRWatch(ctx, "sess-1", "task-1", "repo-1", "group/proj", 5, "feat/a") + if err != nil { + t.Fatalf("first ensure: %v", err) + } + if first.MRIID != 5 { + t.Fatalf("expected iid 5, got %d", first.MRIID) + } + + second, err := svc.EnsureMRWatch(ctx, "sess-1", "task-1", "repo-1", "group/proj", 11, "feat/a") + if err != nil { + t.Fatalf("second ensure (replacement MR): %v", err) + } + if second.ID != first.ID { + t.Fatalf("expected the same watch row updated in place, got %s and %s", first.ID, second.ID) + } + if second.MRIID != 11 { + t.Fatalf("expected iid replaced with 11, got %d", second.MRIID) + } + + got, err := store.GetMRWatchBySessionRepoAndBranch(ctx, "sess-1", "repo-1", "feat/a") + if err != nil || got == nil || got.MRIID != 11 { + t.Fatalf("persisted iid not replaced: %+v err=%v", got, err) + } +} + +func TestEnsureMRWatch_MultiBranchCoexist(t *testing.T) { + store := newTestStore(t) + svc := NewService("", nil, "none", nil, newTestLogger(t)) + svc.SetStore(store) + ctx := context.Background() + + if _, err := svc.EnsureMRWatch(ctx, "sess-1", "task-1", "repo-1", "group/proj", 1, "feat/a"); err != nil { + t.Fatalf("ensure branch a: %v", err) + } + if _, err := svc.EnsureMRWatch(ctx, "sess-1", "task-1", "repo-1", "group/proj", 2, "feat/b"); err != nil { + t.Fatalf("ensure branch b: %v", err) + } + + all, err := store.ListMRWatchesBySession(ctx, "sess-1") + if err != nil || len(all) != 2 { + t.Fatalf("expected 2 coexisting branch watches, got %d err=%v", len(all), err) + } +} diff --git a/apps/backend/internal/gitlab/service_task_mr_autolink.go b/apps/backend/internal/gitlab/service_task_mr_autolink.go new file mode 100644 index 0000000000..cb7e90291c --- /dev/null +++ b/apps/backend/internal/gitlab/service_task_mr_autolink.go @@ -0,0 +1,99 @@ +package gitlab + +import ( + "context" + "fmt" + "strings" + + "go.uber.org/zap" +) + +// AutoLinkMRForBranch is the push-detection counterpart to +// AssociateExistingMRByURL: instead of a user-supplied MR URL, it searches +// the branch for an open MR and links whatever it finds. Returns (nil, nil) +// when no open MR exists on the branch yet — that is the normal "pushed but +// no MR opened" case, not an error. +// +// Order matters: ResolveTaskMRRepository and ValidateTaskMRRepositoryIdentity +// run before any network call, so a task whose repository doesn't match the +// requested project fails closed without leaking an API call to a merge +// request the caller was never entitled to link. +// +// Idempotent: UpsertTaskMR and EnsureMRWatch are both get-or-create/update +// keyed on their respective unique constraints, so re-running this for an +// already-linked (task, repository, project, iid) updates the existing rows +// in place rather than duplicating them. +func (s *Service) AutoLinkMRForBranch( + ctx context.Context, + workspaceID, sessionID, taskID, repositoryID, projectPath, branch string, +) (*TaskMR, error) { + // A blank branch must never reach FindMRByBranch: PATClient interpolates + // it into `?source_branch=&state=opened&per_page=1`, a query with no + // effective source-branch filter, so GitLab would answer with an + // arbitrary open MR of the project and the wrong MR would get linked. + // Git refs cannot contain whitespace, so a whitespace-only value is + // equally invalid. Guarded here — not just at orchestrator call sites — + // so every caller of this service method gets the same safety guarantee. + branch = strings.TrimSpace(branch) + if branch == "" { + return nil, nil + } + store := s.requireStore() + if store == nil { + return nil, errStoreUnavailable + } + repositoryID, err := store.ResolveTaskMRRepository(ctx, workspaceID, taskID, repositoryID) + if err != nil { + return nil, err + } + client, err := s.ClientForWorkspace(ctx, workspaceID) + if err != nil { + return nil, err + } + if err := store.ValidateTaskMRRepositoryIdentity( + ctx, workspaceID, taskID, repositoryID, client.Host(), projectPath, + ); err != nil { + return nil, err + } + mr, err := client.FindMRByBranch(ctx, projectPath, branch) + if err != nil { + return nil, fmt.Errorf("find merge request by branch: %w", err) + } + if mr == nil { + // No MR is open on this branch yet — the normal "pushed but nothing + // opened" result, not an error. Still leave a placeholder (iid=0) + // watch behind so the poller's own iid<=0 resolution (CheckMRWatch) + // can discover an MR opened later — e.g. from the GitLab web UI — + // well after push detection's own retry window ([0, 30s, 60s]) + // closes. Best-effort: a watch failure here must not turn "no MR + // yet" into a hard auto-link error. + if _, err := s.EnsureMRWatch(ctx, sessionID, taskID, repositoryID, projectPath, 0, branch); err != nil { + s.logger.Warn("failed to ensure placeholder MR watch while no MR is open on branch", + zap.String("task_id", taskID), zap.String("session_id", sessionID), zap.Error(err)) + } + return nil, nil + } + status, err := client.GetMRStatus(ctx, projectPath, mr.IID) + if err != nil { + return nil, fmt.Errorf("fetch merge request: %w", err) + } + if err := validateReturnedMRIdentity(status, client.Host(), projectPath, mr.IID); err != nil { + return nil, ErrTaskMRNotFound + } + association := taskMRFromStatus(taskID, repositoryID, client.Host(), projectPath, status) + if err := store.UpsertTaskMR(ctx, association); err != nil { + return nil, fmt.Errorf("upsert task MR: %w", err) + } + // Detach from ctx's cancellation, bounded by the same timeout as the + // URL-link flow's equivalent post-commit call: the association above + // already committed, so a caller's context canceling right after must + // not skip watch creation and silently leave this MR unpolled. + watchCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), watchDeleteTimeout) + defer cancel() + if _, err := s.EnsureMRWatch(watchCtx, sessionID, taskID, repositoryID, projectPath, mr.IID, branch); err != nil { + s.logger.Warn("failed to ensure MR watch after auto-link", + zap.String("task_id", taskID), zap.String("session_id", sessionID), zap.Error(err)) + } + s.publishTaskMRUpdated(ctx, workspaceID, association) + return association, nil +} diff --git a/apps/backend/internal/gitlab/service_task_mr_autolink_test.go b/apps/backend/internal/gitlab/service_task_mr_autolink_test.go new file mode 100644 index 0000000000..abd6c3c1d8 --- /dev/null +++ b/apps/backend/internal/gitlab/service_task_mr_autolink_test.go @@ -0,0 +1,251 @@ +package gitlab + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/kandev/kandev/internal/events" + "github.com/kandev/kandev/internal/events/bus" +) + +func TestAutoLinkMRForBranch_CreatesAssociationAndWatch(t *testing.T) { + const host = "https://gitlab.acme.test" + svc, store, client := newTaskMRLinkService(t, host) + seedTaskMRLinkFixture(t, store, "ws-1", "task-1", "repo-1") + setTaskMRRepositoryIdentity(t, store, "repo-1", host, "group/proj") + client.SeedMR("group/proj", &MR{ + IID: 7, Title: "Feat A", HeadBranch: "feat/a", State: mrStateOpen, + WebURL: host + "/group/proj/-/merge_requests/7", CreatedAt: time.Now().UTC(), + }) + + log := newTestLogger(t) + eventBus := bus.NewMemoryEventBus(log) + svc.SetEventBus(eventBus) + received := make(chan *bus.Event, 1) + if _, err := eventBus.Subscribe(events.GitLabTaskMRUpdated, func(_ context.Context, evt *bus.Event) error { + received <- evt + return nil + }); err != nil { + t.Fatalf("subscribe: %v", err) + } + + assoc, err := svc.AutoLinkMRForBranch(context.Background(), "ws-1", "sess-1", "task-1", "repo-1", "group/proj", "feat/a") + if err != nil { + t.Fatalf("AutoLinkMRForBranch: %v", err) + } + if assoc == nil || assoc.MRIID != 7 || assoc.RepositoryID != "repo-1" { + t.Fatalf("unexpected association: %+v", assoc) + } + + watch, err := store.GetMRWatchBySessionRepoAndBranch(context.Background(), "sess-1", "repo-1", "feat/a") + if err != nil || watch == nil || watch.MRIID != 7 { + t.Fatalf("expected watch created with iid 7, got %+v err=%v", watch, err) + } + + select { + case <-received: + default: + t.Fatal("expected gitlab.task_mr.updated to be published") + } +} + +func TestAutoLinkMRForBranch_NoOpenMR(t *testing.T) { + const host = "https://gitlab.acme.test" + svc, store, _ := newTaskMRLinkService(t, host) + seedTaskMRLinkFixture(t, store, "ws-1", "task-1", "repo-1") + setTaskMRRepositoryIdentity(t, store, "repo-1", host, "group/proj") + + assoc, err := svc.AutoLinkMRForBranch(context.Background(), "ws-1", "sess-1", "task-1", "repo-1", "group/proj", "feat/none") + if err != nil { + t.Fatalf("AutoLinkMRForBranch: %v", err) + } + if assoc != nil { + t.Fatalf("expected nil association when no MR is open, got %+v", assoc) + } + + rows, err := store.ListTaskMRsByTask(context.Background(), "task-1") + if err != nil || len(rows) != 0 { + t.Fatalf("expected no rows created, got %d err=%v", len(rows), err) + } +} + +// TestAutoLinkMRForBranch_NoOpenMRLeavesPlaceholderWatch covers late MR +// discovery: when push detection's own [0, 30s, 60s] retry window closes +// with no MR open yet, a placeholder (iid=0) watch must still exist +// afterward so an MR opened later — e.g. from the GitLab web UI, well after +// the retry window — is discovered by the poller's own iid<=0 resolution +// instead of being lost forever. +func TestAutoLinkMRForBranch_NoOpenMRLeavesPlaceholderWatch(t *testing.T) { + const host = "https://gitlab.acme.test" + svc, store, _ := newTaskMRLinkService(t, host) + seedTaskMRLinkFixture(t, store, "ws-1", "task-1", "repo-1") + setTaskMRRepositoryIdentity(t, store, "repo-1", host, "group/proj") + + assoc, err := svc.AutoLinkMRForBranch(context.Background(), "ws-1", "sess-1", "task-1", "repo-1", "group/proj", "feat/none") + if err != nil { + t.Fatalf("AutoLinkMRForBranch: %v", err) + } + if assoc != nil { + t.Fatalf("expected nil association when no MR is open, got %+v", assoc) + } + + watch, err := store.GetMRWatchBySessionRepoAndBranch(context.Background(), "sess-1", "repo-1", "feat/none") + if err != nil || watch == nil { + t.Fatalf("expected a placeholder watch to be created, got %+v err=%v", watch, err) + } + if watch.MRIID != 0 { + t.Fatalf("expected placeholder watch iid=0, got %d", watch.MRIID) + } +} + +func TestAutoLinkMRForBranch_Idempotent(t *testing.T) { + const host = "https://gitlab.acme.test" + svc, store, client := newTaskMRLinkService(t, host) + seedTaskMRLinkFixture(t, store, "ws-1", "task-1", "repo-1") + setTaskMRRepositoryIdentity(t, store, "repo-1", host, "group/proj") + client.SeedMR("group/proj", &MR{ + IID: 7, Title: "Feat A", HeadBranch: "feat/a", State: mrStateOpen, + WebURL: host + "/group/proj/-/merge_requests/7", CreatedAt: time.Now().UTC(), + }) + + for i := 0; i < 2; i++ { + if _, err := svc.AutoLinkMRForBranch(context.Background(), "ws-1", "sess-1", "task-1", "repo-1", "group/proj", "feat/a"); err != nil { + t.Fatalf("AutoLinkMRForBranch run %d: %v", i, err) + } + } + + rows, err := store.ListTaskMRsByTask(context.Background(), "task-1") + if err != nil || len(rows) != 1 { + t.Fatalf("expected exactly 1 task MR row, got %d err=%v", len(rows), err) + } + watches, err := store.ListMRWatchesByTask(context.Background(), "task-1") + if err != nil || len(watches) != 1 { + t.Fatalf("expected exactly 1 watch row, got %d err=%v", len(watches), err) + } +} + +func TestAutoLinkMRForBranch_RejectsRepositoryIdentityMismatch(t *testing.T) { + const host = "https://gitlab.acme.test" + svc, store, client := newTaskMRLinkService(t, host) + seedTaskMRLinkFixture(t, store, "ws-1", "task-1", "repo-1") + setTaskMRRepositoryIdentity(t, store, "repo-1", host, "group/other") + client.SeedMR("group/proj", &MR{ + IID: 7, Title: "Feat A", HeadBranch: "feat/a", State: mrStateOpen, + WebURL: host + "/group/proj/-/merge_requests/7", CreatedAt: time.Now().UTC(), + }) + + _, err := svc.AutoLinkMRForBranch(context.Background(), "ws-1", "sess-1", "task-1", "repo-1", "group/proj", "feat/a") + if !errors.Is(err, ErrTaskMRRepositoryMismatch) { + t.Fatalf("error = %v, want ErrTaskMRRepositoryMismatch", err) + } + + rows, _ := store.ListTaskMRsByTask(context.Background(), "task-1") + if len(rows) != 0 { + t.Fatalf("expected no association on identity mismatch, got %d rows", len(rows)) + } +} + +func TestAutoLinkMRForBranch_MultiBranchCoexist(t *testing.T) { + const host = "https://gitlab.acme.test" + svc, store, client := newTaskMRLinkService(t, host) + seedTaskMRLinkFixture(t, store, "ws-1", "task-1", "repo-1") + setTaskMRRepositoryIdentity(t, store, "repo-1", host, "group/proj") + client.SeedMR("group/proj", &MR{ + IID: 7, Title: "Feat A", HeadBranch: "feat/a", State: mrStateOpen, + WebURL: host + "/group/proj/-/merge_requests/7", CreatedAt: time.Now().UTC(), + }) + client.SeedMR("group/proj", &MR{ + IID: 8, Title: "Feat B", HeadBranch: "feat/b", State: mrStateOpen, + WebURL: host + "/group/proj/-/merge_requests/8", CreatedAt: time.Now().UTC(), + }) + + if _, err := svc.AutoLinkMRForBranch(context.Background(), "ws-1", "sess-1", "task-1", "repo-1", "group/proj", "feat/a"); err != nil { + t.Fatalf("link branch a: %v", err) + } + if _, err := svc.AutoLinkMRForBranch(context.Background(), "ws-1", "sess-1", "task-1", "repo-1", "group/proj", "feat/b"); err != nil { + t.Fatalf("link branch b: %v", err) + } + + rows, err := store.ListTaskMRsByTask(context.Background(), "task-1") + if err != nil || len(rows) != 2 { + t.Fatalf("expected 2 task MR rows for 2 branches, got %d err=%v", len(rows), err) + } + watches, err := store.ListMRWatchesByTask(context.Background(), "task-1") + if err != nil || len(watches) != 2 { + t.Fatalf("expected 2 watch rows for 2 branches, got %d err=%v", len(watches), err) + } +} + +func TestAutoLinkMRForBranch_MultiRepoScoping(t *testing.T) { + const host = "https://gitlab.acme.test" + svc, store, client := newTaskMRLinkService(t, host) + seedTaskMRLinkFixture(t, store, "ws-1", "task-1", "repo-1") + setTaskMRRepositoryIdentity(t, store, "repo-1", host, "group/front") + if _, err := store.db.Exec(`INSERT INTO repositories (id, workspace_id) VALUES ('repo-2', 'ws-1'); + INSERT INTO task_repositories (id, task_id, repository_id) VALUES ('task-repo-2', 'task-1', 'repo-2')`); err != nil { + t.Fatalf("seed second repo: %v", err) + } + setTaskMRRepositoryIdentity(t, store, "repo-2", host, "group/back") + client.SeedMR("group/front", &MR{ + IID: 1, Title: "Front", HeadBranch: "feat/x", State: mrStateOpen, + WebURL: host + "/group/front/-/merge_requests/1", CreatedAt: time.Now().UTC(), + }) + client.SeedMR("group/back", &MR{ + IID: 2, Title: "Back", HeadBranch: "feat/x", State: mrStateOpen, + WebURL: host + "/group/back/-/merge_requests/2", CreatedAt: time.Now().UTC(), + }) + + if _, err := svc.AutoLinkMRForBranch(context.Background(), "ws-1", "sess-1", "task-1", "repo-1", "group/front", "feat/x"); err != nil { + t.Fatalf("link repo-1: %v", err) + } + if _, err := svc.AutoLinkMRForBranch(context.Background(), "ws-1", "sess-1", "task-1", "repo-2", "group/back", "feat/x"); err != nil { + t.Fatalf("link repo-2: %v", err) + } + + front, err := store.ListTaskMRsByTask(context.Background(), "task-1") + if err != nil || len(front) != 2 { + t.Fatalf("expected 2 rows scoped by repository, got %d err=%v", len(front), err) + } + byRepo := map[string]int{} + for _, r := range front { + byRepo[r.RepositoryID] = r.MRIID + } + if byRepo["repo-1"] != 1 || byRepo["repo-2"] != 2 { + t.Fatalf("cross-repo overwrite: %+v", byRepo) + } +} + +// TestAutoLinkMRForBranch_RejectsBlankBranch guards this at the service +// layer directly (not just at the orchestrator's push-detection call site): +// a blank branch must never reach FindMRByBranch, which would otherwise +// interpolate it into a query with no effective source-branch filter and +// link an arbitrary open MR of the project to the task. +func TestAutoLinkMRForBranch_RejectsBlankBranch(t *testing.T) { + const host = "https://gitlab.acme.test" + for _, branch := range []string{"", " ", "\t\n"} { + t.Run("branch="+branch, func(t *testing.T) { + svc, store, client := newTaskMRLinkService(t, host) + seedTaskMRLinkFixture(t, store, "ws-1", "task-1", "repo-1") + setTaskMRRepositoryIdentity(t, store, "repo-1", host, "group/proj") + client.SeedMR("group/proj", &MR{ + IID: 7, Title: "Feat A", HeadBranch: "feat/a", State: mrStateOpen, + WebURL: host + "/group/proj/-/merge_requests/7", CreatedAt: time.Now().UTC(), + }) + + assoc, err := svc.AutoLinkMRForBranch(context.Background(), "ws-1", "sess-1", "task-1", "repo-1", "group/proj", branch) + if err != nil { + t.Fatalf("AutoLinkMRForBranch: %v", err) + } + if assoc != nil { + t.Fatalf("expected nil association for a blank branch, got %+v", assoc) + } + + rows, _ := store.ListTaskMRsByTask(context.Background(), "task-1") + if len(rows) != 0 { + t.Fatalf("expected no association created for a blank branch, got %d rows", len(rows)) + } + }) + } +} diff --git a/apps/backend/internal/gitlab/service_task_mr_link.go b/apps/backend/internal/gitlab/service_task_mr_link.go index c00210dfa9..c0d3db15c7 100644 --- a/apps/backend/internal/gitlab/service_task_mr_link.go +++ b/apps/backend/internal/gitlab/service_task_mr_link.go @@ -197,6 +197,44 @@ func (s *Service) AssociateExistingMRByURL( return association, nil } +// AssociateExistingMRByURLForSession wraps AssociateExistingMRByURL for +// callers that have a concrete session to key a refresh watch with — the +// Create-MR action and manual URL linking triggered from a session's git +// activity. This mirrors GitHub's split between AssociatePRByURLForWorkspace +// (session-aware, creates a watch) and AssociateExistingPRByURLForWorkspace +// (the workspace-level HTTP endpoint, no session, no watch). Watch creation +// is best-effort: the association already succeeded, so a watch failure is +// logged rather than surfaced, matching ensureWatchForLinkedMR's convention. +func (s *Service) AssociateExistingMRByURLForSession( + ctx context.Context, + workspaceID, sessionID, taskID, repositoryID, mrURL string, +) (*TaskMR, error) { + association, err := s.AssociateExistingMRByURL(ctx, workspaceID, taskID, repositoryID, mrURL) + if err != nil { + return nil, err + } + // A cancelable ctx (e.g. an HTTP request context that times out right + // after the association commits) must not skip this: the association + // already succeeded and is returned as a success below, so a canceled + // EnsureMRWatch would silently leave that MR with no refresh watch until + // another push recreates it. Detach from ctx's cancellation the same way + // DeleteReviewWatch does for its own post-commit side effect, but bound + // it with the same timeout so a stalled store call can't hang forever. + watchCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), watchDeleteTimeout) + defer cancel() + if _, err := s.EnsureMRWatch( + watchCtx, sessionID, taskID, association.RepositoryID, association.ProjectPath, + association.MRIID, association.HeadBranch, + ); err != nil { + s.logger.Warn("failed to ensure MR watch after URL association", + zap.String("session_id", sessionID), + zap.String("task_id", taskID), + zap.Int("mr_iid", association.MRIID), + zap.Error(err)) + } + return association, nil +} + func validateReturnedMRIdentity(status *MRStatus, host, projectPath string, iid int) error { if status == nil || status.MR == nil { return ErrTaskMRNotFound @@ -215,6 +253,31 @@ func validateReturnedMRIdentity(status *MRStatus, host, projectPath string, iid return nil } +// IsConfiguredGitLabHost reports whether remoteURL's host matches the +// workspace's own configured GitLab connection, self-managed or gitlab.com. +// Used by push-detection provider routing to recognize self-managed GitLab +// repositories: unlike github.com/gitlab.com, they never get a durable +// "gitlab" provider tag at discovery time (see +// task/service.resolveRepositoryProviderIdentity), so remote_url is their +// only durable identity signal, and it must be compared against the actual +// configured host rather than a github.com/gitlab.com hostname allowlist. +// +// Best-effort: returns false on any lookup failure (unconfigured workspace, +// unparsable remote) rather than erroring, since callers use this only to +// pick a routing branch — ValidateTaskMRRepositoryIdentity is still the real +// security boundary and still runs before any GitLab API call. +func (s *Service) IsConfiguredGitLabHost(ctx context.Context, workspaceID, remoteURL string) bool { + host, _ := parseGitLabRemoteURLIdentity(remoteURL) + if host == "" { + return false + } + cfg, err := s.GetConfigForWorkspace(ctx, workspaceID) + if err != nil || cfg == nil || cfg.Host == "" { + return false + } + return sameGitLabHost(host, cfg.Host) +} + // UnlinkTaskMR removes one association and its matching refresh watch without // mutating the task, other associations, or the upstream merge request. func (s *Service) UnlinkTaskMR(ctx context.Context, workspaceID, associationID string) error { diff --git a/apps/backend/internal/gitlab/service_task_mr_link_test.go b/apps/backend/internal/gitlab/service_task_mr_link_test.go index 0462ef6f43..b14d39f6a1 100644 --- a/apps/backend/internal/gitlab/service_task_mr_link_test.go +++ b/apps/backend/internal/gitlab/service_task_mr_link_test.go @@ -214,6 +214,54 @@ func TestAssociateExistingMRByURLCreatesIdempotentWorkspaceScopedLink(t *testing } } +// TestAssociateExistingMRByURLForSessionEnsuresWatch pins the gap the +// gateway wiring used to have: the Create-MR action and manual URL linking +// triggered from a session both have a concrete sessionID available, and +// AssociateExistingMRByURLForSession must use it to create a refresh watch +// immediately — mirroring GitHub's AssociatePRByURLForWorkspace — instead of +// leaving the MR unwatched until a later push happens to trigger +// ensureWatchForLinkedMR. +func TestAssociateExistingMRByURLForSessionEnsuresWatch(t *testing.T) { + const host = "https://gitlab.acme.test" + svc, store, client := newTaskMRLinkService(t, host) + seedTaskMRLinkFixture(t, store, "ws-1", "task-1", "repo-1") + setTaskMRRepositoryIdentity(t, store, "repo-1", host, "group/project") + client.SeedMR("group/project", &MR{ + IID: 9, Title: "MR title", WebURL: host + "/group/project/-/merge_requests/9", + State: "opened", HeadBranch: "feature", BaseBranch: "main", CreatedAt: time.Now().UTC(), + }) + + association, err := svc.AssociateExistingMRByURLForSession( + context.Background(), "ws-1", "session-1", "task-1", "repo-1", + host+"/group/project/-/merge_requests/9", + ) + if err != nil { + t.Fatalf("AssociateExistingMRByURLForSession: %v", err) + } + watch, err := store.GetMRWatchBySessionRepoAndBranch(context.Background(), "session-1", "repo-1", "feature") + if err != nil { + t.Fatalf("GetMRWatchBySessionRepoAndBranch: %v", err) + } + if watch == nil { + t.Fatal("expected a refresh watch to be created for the session") + } + if watch.MRIID != 9 || watch.ProjectPath != "group/project" { + t.Fatalf("watch = %+v, want mr_iid=9 project_path=group/project", watch) + } + if association.MRIID != 9 { + t.Fatalf("association mr_iid = %d, want 9", association.MRIID) + } + + // Calling it again for the same session/branch must not error or + // duplicate the watch (EnsureMRWatch is idempotent). + if _, err := svc.AssociateExistingMRByURLForSession( + context.Background(), "ws-1", "session-1", "task-1", "repo-1", + host+"/group/project/-/merge_requests/9", + ); err != nil { + t.Fatalf("second AssociateExistingMRByURLForSession: %v", err) + } +} + func TestAssociateExistingMRByURLAcceptsMRURLWithExplicitDefaultPort(t *testing.T) { // The workspace's configured GitLab host and the incoming MR URL should // match even when only one side spells out the scheme's implicit default diff --git a/apps/backend/internal/gitlab/service_watches.go b/apps/backend/internal/gitlab/service_watches.go index 6ba56a3189..2341cc0710 100644 --- a/apps/backend/internal/gitlab/service_watches.go +++ b/apps/backend/internal/gitlab/service_watches.go @@ -2,6 +2,7 @@ package gitlab import ( "context" + "errors" "fmt" "net/url" "strings" @@ -48,6 +49,51 @@ func (s *Service) CreateMRWatch(ctx context.Context, sessionID, taskID, reposito return w, nil } +// EnsureMRWatch is the idempotent get-or-create counterpart to CreateMRWatch, +// keyed by (sessionID, repositoryID, branch). Returns the existing row when +// one is already present, backfilling mr_iid in place if it was previously +// unknown (0) and a real iid is now available. Otherwise creates a new watch. +// Safe to call repeatedly for the same (session, repository, branch) triple — +// callers on the auto-link and on-demand-check paths call this on every push +// without checking for an existing row themselves. +func (s *Service) EnsureMRWatch(ctx context.Context, sessionID, taskID, repositoryID, projectPath string, iid int, branch string) (*MRWatch, error) { + store := s.requireStore() + if store == nil { + return nil, errStoreUnavailable + } + existing, err := store.GetMRWatchBySessionRepoAndBranch(ctx, sessionID, repositoryID, branch) + if err != nil { + return nil, err + } + if existing == nil { + w, createErr := s.CreateMRWatch(ctx, sessionID, taskID, repositoryID, projectPath, iid, branch) + if createErr == nil { + return w, nil + } + // Lost the race with a concurrent EnsureMRWatch for the same triple + // (push detection and the on-demand check can both run for one + // session): the UNIQUE(session_id, repository_id, branch) constraint + // rejected our INSERT because the row we were about to create now + // exists. Re-read and return it — the caller's intent is satisfied. + // Any other failure is still reported. + if raced, getErr := store.GetMRWatchBySessionRepoAndBranch(ctx, sessionID, repositoryID, branch); getErr == nil && raced != nil { + return raced, nil + } + return nil, createErr + } + // Replace whenever the caller supplies a different, known iid — not just + // when the stored one was still unknown (<=0). A branch can be relinked + // to a replacement MR (the prior one closed and a new one opened), and + // without this the watch keeps polling the stale iid forever. + if iid > 0 && existing.MRIID != iid { + if err := store.UpdateMRWatchMRIID(ctx, existing.ID, iid); err != nil { + return nil, fmt.Errorf("update MR watch iid: %w", err) + } + existing.MRIID = iid + } + return existing, nil +} + // GetMRWatchBySession fetches the legacy single-repo watch. func (s *Service) GetMRWatchBySession(ctx context.Context, sessionID string) (*MRWatch, error) { store := s.requireStore() @@ -171,6 +217,13 @@ func (s *Service) CheckMRWatch(ctx context.Context, watch *MRWatch) (*MRStatus, if store == nil { return nil, false, errStoreUnavailable } + valid, err := s.watchMatchesTaskRepository(ctx, store, client, watch) + if err != nil { + return nil, false, err + } + if !valid { + return nil, false, nil + } // If we don't yet know an iid, try to find it from the branch. if watch.MRIID <= 0 { mr, err := client.FindMRByBranch(ctx, watch.ProjectPath, watch.Branch) @@ -197,9 +250,161 @@ func (s *Service) CheckMRWatch(ctx context.Context, watch *MRWatch) (*MRStatus, if notable { s.publishMRFeedbackEvent(ctx, watch, status) } + s.refreshTaskMRFromWatch(ctx, store, client, watch, status) return status, notable, nil } +// watchMatchesTaskRepository confirms watch.RepositoryID/ProjectPath still +// identify the task's own GitLab repository before any GitLab request or +// gitlab_task_mrs write. refreshTaskMRFromWatch's identity check only +// validates the MR *returned by GitLab* against the watch's own (host, +// project, iid) — that alone still lets a stale or mis-scoped watch (one +// whose repository_id was never actually tied to this GitLab project) poll +// and self-consistently upsert data anyway, since GetMRStatus itself only +// cares about project path and iid, not repository_id. +// +// Returns (true, nil) when the watch is valid, or when the owning task is +// orphaned (workspaceID == "", e.g. the task was deleted) — refreshing an +// orphaned watch is already a benign no-op elsewhere in this file, so +// identity is not re-litigated here. Returns (false, nil) — not an error — +// only when ValidateTaskMRRepositoryIdentity returns the direct +// ErrTaskMRRepositoryMismatch sentinel, a durable identity mismatch; the +// watch is deleted rather than left to retry, since a mismatch is a +// permanent fact, not a transient failure, and would otherwise poll forever +// for nothing. Any other error — ErrTaskMRNotFound (the task/repository +// link row itself is missing, which can be a transient race rather than a +// proven mismatch) or a wrapped store/lookup failure — is propagated +// instead of deleting the watch, so the poller's existing per-watch error +// logging fires and retries on the next tick rather than destroying data on +// what may be a recoverable condition. +func (s *Service) watchMatchesTaskRepository(ctx context.Context, store *Store, client Client, watch *MRWatch) (bool, error) { + workspaceID, err := store.WorkspaceIDForTask(ctx, watch.TaskID) + if err != nil { + return false, err + } + if workspaceID == "" { + return true, nil + } + err = store.ValidateTaskMRRepositoryIdentity( + ctx, workspaceID, watch.TaskID, watch.RepositoryID, client.Host(), watch.ProjectPath, + ) + if err == nil { + return true, nil + } + if !errors.Is(err, ErrTaskMRRepositoryMismatch) { + return false, err + } + s.logger.Warn("dropping MR watch whose repository no longer matches the task", + zap.String("watch_id", watch.ID), zap.String("task_id", watch.TaskID), zap.Error(err)) + if delErr := store.DeleteMRWatch(ctx, watch.ID); delErr != nil { + s.logger.Warn("failed to delete mismatched MR watch", + zap.String("watch_id", watch.ID), zap.Error(delErr)) + } + return false, nil +} + +// refreshTaskMRFromWatch keeps gitlab_task_mrs in sync with every poll, not +// just the ones the caller found "notable" — otherwise the topbar MR surface +// (state, pipeline_state, approval_state, merge_status) only updates on a +// pipeline/approval transition and misses everything else (title, merge +// state, review comments' side effects, etc). Best-effort: a failure here +// must not fail the poll cycle, since UpdateMRWatchTimestamps above already +// recorded a successful check. +func (s *Service) refreshTaskMRFromWatch(ctx context.Context, store *Store, client Client, watch *MRWatch, status *MRStatus) { + // A response that doesn't actually match this watch's (host, project, + // iid) — a client bug, or a mismatched mock in tests — must not create or + // overwrite a task-MR association. Same check the manual-link and + // auto-link paths already run before trusting a fetched MR. + if err := validateReturnedMRIdentity(status, client.Host(), watch.ProjectPath, watch.MRIID); err != nil { + s.logger.Warn("dropping MR watch refresh with mismatched identity", + zap.String("watch_id", watch.ID), zap.String("task_id", watch.TaskID), zap.Error(err)) + return + } + workspaceID, err := store.WorkspaceIDForTask(ctx, watch.TaskID) + if err != nil { + // This is now the only path that refreshes gitlab_task_mrs on every + // poll, so a transient DB error here must be observable — unlike an + // empty workspaceID with no error (a benign orphaned-watch case, + // e.g. the task was deleted), which is not logged. + s.logger.Warn("failed to resolve workspace while refreshing task MR from watch", + zap.String("watch_id", watch.ID), zap.String("task_id", watch.TaskID), zap.Error(err)) + return + } + if workspaceID == "" { + return + } + association := taskMRFromStatus(watch.TaskID, watch.RepositoryID, client.Host(), watch.ProjectPath, status) + // Loaded before the upsert so it reflects the row's state prior to this + // poll; a lookup failure isn't fatal to the refresh itself, but without a + // "previous" to compare against we can't tell whether anything visible + // changed, so this poll's update is published rather than silently + // dropped by a comparison against a stale nil. + previous, prevErr := store.GetTaskMR(ctx, watch.TaskID, watch.RepositoryID, watch.ProjectPath, watch.MRIID) + if prevErr != nil { + s.logger.Warn("failed to load existing task MR before refresh", + zap.String("watch_id", watch.ID), zap.String("task_id", watch.TaskID), zap.Error(prevErr)) + } + if err := store.UpsertTaskMR(ctx, association); err != nil { + s.logger.Warn("failed to refresh task MR from watch", + zap.String("watch_id", watch.ID), zap.String("task_id", watch.TaskID), zap.Error(err)) + return + } + if prevErr != nil || taskMRVisibleFieldsChanged(previous, association) { + s.publishTaskMRUpdated(ctx, workspaceID, association) + } +} + +// taskMRVisibleFieldsChanged reports whether any user-visible field differs +// between a task-MR association's previous and current state. Excludes +// bookkeeping-only columns (id, timestamps) so a poll that re-fetches +// identical MR data doesn't broadcast a gitlab.task_mr.updated event on every +// tick — refreshTaskMRFromWatch runs on every poll, not just "notable" +// pipeline/approval transitions, so without this filter the event fires far +// more often than anything actually changed. previous == nil (first-ever +// refresh for this association) always counts as changed. +func taskMRVisibleFieldsChanged(previous, current *TaskMR) bool { + if previous == nil { + return true + } + return taskMRIdentityFieldsChanged(previous, current) || taskMRStatusFieldsChanged(previous, current) +} + +// taskMRIdentityFieldsChanged compares the descriptive fields a poll can +// revise (title, branches, author) — split out from taskMRStatusFieldsChanged +// purely to keep taskMRVisibleFieldsChanged under the cyclomatic-complexity +// budget; the two halves together are the full comparison. +func taskMRIdentityFieldsChanged(previous, current *TaskMR) bool { + return previous.MRURL != current.MRURL || + previous.MRTitle != current.MRTitle || + previous.HeadBranch != current.HeadBranch || + previous.BaseBranch != current.BaseBranch || + previous.AuthorUsername != current.AuthorUsername +} + +// taskMRStatusFieldsChanged compares the review/CI status fields a poll can +// revise (state, approvals, pipeline, merge). +func taskMRStatusFieldsChanged(previous, current *TaskMR) bool { + return previous.State != current.State || + previous.ApprovalState != current.ApprovalState || + previous.PipelineState != current.PipelineState || + previous.MergeStatus != current.MergeStatus || + previous.Draft != current.Draft || + previous.ApprovalCount != current.ApprovalCount || + previous.RequiredApprovals != current.RequiredApprovals || + previous.PipelineJobsTotal != current.PipelineJobsTotal || + previous.PipelineJobsPass != current.PipelineJobsPass || + !timePtrEqual(previous.MergedAt, current.MergedAt) || + !timePtrEqual(previous.ClosedAt, current.ClosedAt) +} + +// timePtrEqual compares two *time.Time for equal instants, nil-safe. +func timePtrEqual(a, b *time.Time) bool { + if a == nil || b == nil { + return a == b + } + return a.Equal(*b) +} + // --- Review Watch --- // CreateReviewWatch persists a new review watch. diff --git a/apps/backend/internal/gitlab/store.go b/apps/backend/internal/gitlab/store.go index a7e5fe2f84..fcc11f2892 100644 --- a/apps/backend/internal/gitlab/store.go +++ b/apps/backend/internal/gitlab/store.go @@ -111,7 +111,7 @@ const createTablesSQL = ` last_approval_state TEXT DEFAULT '', created_at DATETIME NOT NULL, updated_at DATETIME NOT NULL, - UNIQUE(session_id, repository_id) + UNIQUE(session_id, repository_id, branch) ); CREATE INDEX IF NOT EXISTS idx_gitlab_mr_watches_task_id ON gitlab_mr_watches(task_id); @@ -216,7 +216,106 @@ func (s *Store) createTables() error { if err := s.migrateConfigRevision(); err != nil { return err } - return s.migrateWatchColumns() + if err := s.migrateWatchColumns(); err != nil { + return err + } + if err := s.migrateMRWatchUniqueKey(); err != nil { + return err + } + return s.ensureMRWatchIndexes() +} + +// ensureMRWatchIndexes re-creates gitlab_mr_watches' indexes after +// migrateMRWatchUniqueKey. The rebuild DROPs the old table, and SQLite drops +// a table's indexes with it, so the index created by createTablesSQL earlier +// in this same call is gone by the time the rebuild finishes — leaving the +// table unindexed until the next process start. github.Store.initSchema +// avoids this by running its index creation after the equivalent +// migratePRTablesForMultiRepo rebuild; this is the same ordering. +func (s *Store) ensureMRWatchIndexes() error { + if _, err := s.db.Exec( + `CREATE INDEX IF NOT EXISTS idx_gitlab_mr_watches_task_id ON gitlab_mr_watches(task_id)`, + ); err != nil { + return fmt.Errorf("recreate gitlab_mr_watches indexes: %w", err) + } + return nil +} + +// migrateMRWatchUniqueKey rebuilds gitlab_mr_watches to drop the legacy +// UNIQUE(session_id, repository_id) constraint and replace it with +// UNIQUE(session_id, repository_id, branch), so one session can hold a watch +// per branch on the same repository (multi-branch tasks) instead of +// colliding on the second branch's insert. SQLite can't ALTER TABLE DROP +// CONSTRAINT, so this uses the same copy-and-rename rebuild as +// github.Store.migratePRTablesForMultiRepo. Idempotent: only runs when the +// legacy constraint string is found in sqlite_master, so fresh DBs and +// already-migrated DBs are no-ops. +func (s *Store) migrateMRWatchUniqueKey() error { + return s.rebuildIfHasLegacyConstraint( + "gitlab_mr_watches", + "UNIQUE(session_id, repository_id)\n", + `CREATE TABLE gitlab_mr_watches_new ( + id TEXT PRIMARY KEY, + session_id TEXT NOT NULL, + task_id TEXT NOT NULL, + repository_id TEXT NOT NULL DEFAULT '', + project_path TEXT NOT NULL, + mr_iid INTEGER NOT NULL, + branch TEXT NOT NULL, + last_checked_at DATETIME, + last_note_at DATETIME, + last_pipeline_state TEXT DEFAULT '', + last_approval_state TEXT DEFAULT '', + created_at DATETIME NOT NULL, + updated_at DATETIME NOT NULL, + UNIQUE(session_id, repository_id, branch) + )`, + `INSERT INTO gitlab_mr_watches_new ( + id, session_id, task_id, repository_id, project_path, mr_iid, branch, + last_checked_at, last_note_at, last_pipeline_state, last_approval_state, + created_at, updated_at + ) SELECT + id, session_id, task_id, COALESCE(repository_id, ''), project_path, mr_iid, branch, + last_checked_at, last_note_at, last_pipeline_state, last_approval_state, + created_at, updated_at + FROM gitlab_mr_watches`, + ) +} + +// rebuildIfHasLegacyConstraint checks the table's stored CREATE statement in +// sqlite_master for the literal legacyConstraint substring; if present, runs +// the table rebuild (create new, copy data, drop old, rename) inside a +// transaction. No-op when the legacy substring is absent — fresh installs +// already use the new schema and previously-migrated databases skip too. +// Mirrors github.Store.rebuildIfHasLegacyConstraint. +func (s *Store) rebuildIfHasLegacyConstraint(table, legacyConstraint, createNew, copyData string) error { + var existingSQL string + row := s.db.QueryRow(`SELECT sql FROM sqlite_master WHERE type='table' AND name=?`, table) + if err := row.Scan(&existingSQL); err != nil { + if errors.Is(err, sql.ErrNoRows) { + return nil + } + return err + } + if !strings.Contains(existingSQL, legacyConstraint) { + return nil + } + tx, err := s.db.Begin() + if err != nil { + return err + } + defer func() { _ = tx.Rollback() }() + for _, stmt := range []string{ + createNew, + copyData, + "DROP TABLE " + table, + "ALTER TABLE " + table + "_new RENAME TO " + table, + } { + if _, err := tx.Exec(stmt); err != nil { + return fmt.Errorf("rebuild %s: %w", table, err) + } + } + return tx.Commit() } func (s *Store) migrateConfigRevision() error { @@ -412,6 +511,25 @@ func (s *Store) UpsertTaskMR(ctx context.Context, tm *TaskMR) error { return rows.Scan(&tm.ID, &tm.CreatedAt, &tm.UpdatedAt) } +// GetTaskMR returns one task-MR association keyed by +// (task_id, repository_id, project_path, mr_iid), or nil when none exists. +// Used to compare against an incoming refresh before deciding whether it +// actually changed anything worth publishing. +func (s *Store) GetTaskMR(ctx context.Context, taskID, repositoryID, projectPath string, iid int) (*TaskMR, error) { + var tm TaskMR + err := s.ro.GetContext(ctx, &tm, ` + SELECT `+taskMRSelectCols+` FROM gitlab_task_mrs + WHERE task_id = ? AND repository_id = ? AND project_path = ? AND mr_iid = ? + LIMIT 1`, taskID, repositoryID, projectPath, iid) + if errors.Is(err, sql.ErrNoRows) { + return nil, nil + } + if err != nil { + return nil, err + } + return &tm, nil +} + // ListTaskMRsByTask returns every MR association for a task, oldest first. func (s *Store) ListTaskMRsByTask(ctx context.Context, taskID string) ([]*TaskMR, error) { var mrs []TaskMR diff --git a/apps/backend/internal/gitlab/store_mr_watch_migration_test.go b/apps/backend/internal/gitlab/store_mr_watch_migration_test.go new file mode 100644 index 0000000000..c0251e676a --- /dev/null +++ b/apps/backend/internal/gitlab/store_mr_watch_migration_test.go @@ -0,0 +1,177 @@ +package gitlab + +import ( + "context" + "path/filepath" + "testing" + "time" + + "github.com/jmoiron/sqlx" + + "github.com/kandev/kandev/internal/db" +) + +// newLegacyMRWatchStore opens a fresh SQLite DB, creates gitlab_mr_watches +// with the legacy UNIQUE(session_id, repository_id) constraint (pre-dating +// branch-awareness), seeds one row, then hands it to NewStore so the +// migration path under test runs exactly as it does on a real upgrade. +func newLegacyMRWatchStore(t *testing.T) *Store { + t.Helper() + tmp := t.TempDir() + dbPath := filepath.Join(tmp, "gitlab-legacy.db") + dbConn, err := db.OpenSQLite(dbPath) + if err != nil { + t.Fatalf("open db: %v", err) + } + sqlxDB := sqlx.NewDb(dbConn, "sqlite3") + t.Cleanup(func() { _ = sqlxDB.Close() }) + if _, err := sqlxDB.Exec(` + CREATE TABLE workspaces (id TEXT PRIMARY KEY, created_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP); + CREATE TABLE tasks (id TEXT PRIMARY KEY, workspace_id TEXT NOT NULL DEFAULT '', archived_at DATETIME); + CREATE TABLE gitlab_mr_watches ( + id TEXT PRIMARY KEY, + session_id TEXT NOT NULL, + task_id TEXT NOT NULL, + repository_id TEXT NOT NULL DEFAULT '', + project_path TEXT NOT NULL, + mr_iid INTEGER NOT NULL, + branch TEXT NOT NULL, + last_checked_at DATETIME, + last_note_at DATETIME, + last_pipeline_state TEXT DEFAULT '', + last_approval_state TEXT DEFAULT '', + created_at DATETIME NOT NULL, + updated_at DATETIME NOT NULL, + UNIQUE(session_id, repository_id) + )`); err != nil { + t.Fatalf("create legacy schema: %v", err) + } + now := time.Now().UTC() + if _, err := sqlxDB.Exec(` + INSERT INTO gitlab_mr_watches ( + id, session_id, task_id, repository_id, project_path, mr_iid, branch, created_at, updated_at + ) VALUES ('watch-1', 'sess-1', 'task-1', 'repo-1', 'group/proj', 5, 'feat/a', ?, ?)`, + now, now); err != nil { + t.Fatalf("seed legacy row: %v", err) + } + store, err := NewStore(sqlxDB, sqlxDB) + if err != nil { + t.Fatalf("new store: %v", err) + } + return store +} + +func TestMigrateMRWatchUniqueKey_Legacy(t *testing.T) { + store := newLegacyMRWatchStore(t) + ctx := context.Background() + + // The pre-existing row survived the rebuild. + w, err := store.GetMRWatchBySessionRepoAndBranch(ctx, "sess-1", "repo-1", "feat/a") + if err != nil { + t.Fatalf("get migrated watch: %v", err) + } + if w == nil || w.ID != "watch-1" || w.MRIID != 5 { + t.Fatalf("expected migrated watch-1 with iid=5, got %+v", w) + } + + // The new constraint now allows two branches on the same (session, repo). + second := &MRWatch{ + SessionID: "sess-1", TaskID: "task-1", RepositoryID: "repo-1", + ProjectPath: "group/proj", MRIID: 6, Branch: "feat/b", + } + if err := store.CreateMRWatch(ctx, second); err != nil { + t.Fatalf("create second-branch watch after migration: %v", err) + } + + all, err := store.ListMRWatchesBySession(ctx, "sess-1") + if err != nil { + t.Fatalf("list watches: %v", err) + } + if len(all) != 2 { + t.Fatalf("expected 2 watches after migration + new branch, got %d", len(all)) + } +} + +func TestMigrateMRWatchUniqueKey_ReplayIsNoOp(t *testing.T) { + tmp := t.TempDir() + dbPath := filepath.Join(tmp, "gitlab-replay.db") + dbConn, err := db.OpenSQLite(dbPath) + if err != nil { + t.Fatalf("open db: %v", err) + } + sqlxDB := sqlx.NewDb(dbConn, "sqlite3") + t.Cleanup(func() { _ = sqlxDB.Close() }) + if _, err := sqlxDB.Exec(` + CREATE TABLE workspaces (id TEXT PRIMARY KEY, created_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP); + CREATE TABLE tasks (id TEXT PRIMARY KEY, workspace_id TEXT NOT NULL DEFAULT '', archived_at DATETIME)`); err != nil { + t.Fatalf("create base schema: %v", err) + } + store, err := NewStore(sqlxDB, sqlxDB) + if err != nil { + t.Fatalf("new store: %v", err) + } + + ctx := context.Background() + w := &MRWatch{SessionID: "sess-1", TaskID: "task-1", RepositoryID: "repo-1", ProjectPath: "group/proj", MRIID: 1, Branch: "main"} + if err := store.CreateMRWatch(ctx, w); err != nil { + t.Fatalf("create watch: %v", err) + } + + // Re-running createTables (as boot does on every start) must be a no-op: + // the fresh-DB table already has the new constraint, so no legacy + // substring match triggers a rebuild, and the row survives untouched. + if err := store.createTables(); err != nil { + t.Fatalf("replay createTables: %v", err) + } + + got, err := store.GetMRWatchBySessionRepoAndBranch(ctx, "sess-1", "repo-1", "main") + if err != nil { + t.Fatalf("get watch after replay: %v", err) + } + if got == nil || got.ID != w.ID { + t.Fatalf("expected watch to survive replay untouched, got %+v", got) + } +} + +func TestMigrateMRWatchUniqueKey_FreshDBAllowsMultiBranch(t *testing.T) { + store := newTestStore(t) + ctx := context.Background() + + w1 := &MRWatch{SessionID: "sess-1", TaskID: "task-1", RepositoryID: "repo-1", ProjectPath: "group/proj", MRIID: 1, Branch: "feat/a"} + w2 := &MRWatch{SessionID: "sess-1", TaskID: "task-1", RepositoryID: "repo-1", ProjectPath: "group/proj", MRIID: 2, Branch: "feat/b"} + if err := store.CreateMRWatch(ctx, w1); err != nil { + t.Fatalf("create watch 1: %v", err) + } + if err := store.CreateMRWatch(ctx, w2); err != nil { + t.Fatalf("create watch 2 (different branch, same session+repo): %v", err) + } + + all, err := store.ListMRWatchesBySession(ctx, "sess-1") + if err != nil { + t.Fatalf("list: %v", err) + } + if len(all) != 2 { + t.Fatalf("expected 2 watches on a fresh DB, got %d", len(all)) + } +} + +// The rebuild DROPs gitlab_mr_watches, and SQLite drops a table's indexes +// along with it — including the one createTablesSQL created earlier in the +// same createTables() call. Without an explicit re-create, a migrating boot +// leaves the table unindexed until the next process start. +func TestMigrateMRWatchUniqueKey_PreservesTaskIndex(t *testing.T) { + store := newLegacyMRWatchStore(t) + + var names []string + if err := store.ro.Select(&names, + `SELECT name FROM sqlite_master + WHERE type='index' AND tbl_name='gitlab_mr_watches' AND name NOT LIKE 'sqlite_%'`); err != nil { + t.Fatalf("query indexes: %v", err) + } + for _, n := range names { + if n == "idx_gitlab_mr_watches_task_id" { + return + } + } + t.Fatalf("idx_gitlab_mr_watches_task_id missing after migration, got %v", names) +} diff --git a/apps/backend/internal/gitlab/store_watches.go b/apps/backend/internal/gitlab/store_watches.go index 8a5f5ce2ee..c57832a1a4 100644 --- a/apps/backend/internal/gitlab/store_watches.go +++ b/apps/backend/internal/gitlab/store_watches.go @@ -61,6 +61,25 @@ func (s *Store) GetMRWatchBySessionAndRepo(ctx context.Context, sessionID, repos return &w, nil } +// GetMRWatchBySessionRepoAndBranch returns the MR watch for the precise +// (session, repository, branch) triple. Required for multi-branch tasks +// where each branch needs its own watch — querying by (session, repo) alone +// would collapse a secondary branch's push detection onto the first watch +// found. +func (s *Store) GetMRWatchBySessionRepoAndBranch(ctx context.Context, sessionID, repositoryID, branch string) (*MRWatch, error) { + var w MRWatch + err := s.ro.GetContext(ctx, &w, + `SELECT `+mrWatchSelectCols+` FROM gitlab_mr_watches + WHERE session_id = ? AND repository_id = ? AND branch = ? LIMIT 1`, sessionID, repositoryID, branch) + if errors.Is(err, sql.ErrNoRows) { + return nil, nil + } + if err != nil { + return nil, err + } + return &w, nil +} + // ListMRWatchesBySession returns every MR watch for a session. func (s *Store) ListMRWatchesBySession(ctx context.Context, sessionID string) ([]*MRWatch, error) { var ws []MRWatch diff --git a/apps/backend/internal/integrations/AGENTS.md b/apps/backend/internal/integrations/AGENTS.md index d705ac4d56..5d4876ef67 100644 --- a/apps/backend/internal/integrations/AGENTS.md +++ b/apps/backend/internal/integrations/AGENTS.md @@ -47,6 +47,49 @@ existence leak). Jira/Linear/Slack follow this; **Sentry and GitLab's `ListAllIssueWatches` still need the same filter** — the WS gateway backstop does not read `workspace_id`, so GitLab's `workspace_id`-keyed WS list is unscoped too. +## Auto-link guarantee (GitHub pull requests / GitLab merge requests) + +A code-host PR/MR opened on a task's session branch gets linked to the task +without a manual step, on both providers, through three independent +*discovery* paths (rows 1–3 below find and create the association). Two more +rows exist alongside them: manual URL linking is an explicit user action, not +discovery, and the background poller mostly *refreshes* an association +discovery already created — except that a Discovery row can itself leave +behind a *placeholder* watch (`pr_number` / `mr_iid` = 0) when it ran before +any PR/MR existed on the branch yet, and the poller resolves that placeholder +into a real association on a later tick. The poller never creates a new watch +or discovers a PR/MR with no watch at all — resolving a placeholder is the one +exception to "refresh-only", not independent discovery. "Auto-link" +(discovery) means: a `github_task_prs` / `gitlab_task_mrs` row is created, and +a refresh watch (`github_pr_watches` / `gitlab_mr_watches`) is created so +ongoing review/CI/pipeline status keeps syncing via the poller from then on. + +| Trigger | Role | GitHub | GitLab | +| --- | --- | --- | --- | +| Kandev's own Create-PR action (`worktree.create_pr`) | Discovery | ✅ `gateway.go` → `createdChangeAssociationRouter.associate` → `associateGitHub` | ✅ same router → `associateGitLab` → `gitlab.Service.AssociateExistingMRByURL` | +| Push detected on the session branch (agent/CLI `git push`, or a human pushing directly — `gh pr create` / `glab mr create` alone create no push event, so discovery here fires from the branch push that preceded or accompanied them, not the create call itself) | Discovery | ✅ `event_handlers_git.go`'s `trackPushAndAssociatePR` → `event_handlers_github.go`'s `detectPushAndAssociatePR` (retries `[0, 30s, 60s]`) | ✅ same `trackPushAndAssociatePR` dispatches by repository provider → `event_handlers_gitlab_mr.go`'s `detectPushAndAssociateMR` (same retry shape) → `gitlab.Service.AutoLinkMRForBranch`, which now also leaves a placeholder (`mr_iid=0`) watch behind when no MR is open yet, so an MR opened later (e.g. from the GitLab web UI, well after the retry window) is still found by the poller | +| On-demand check (no UI trigger today; callable via WS action) | Discovery | ✅ `Service.CheckSessionPR`, `ws.ActionGitHubCheckSessionPR` (`github.check_session_pr`) | ✅ `Service.CheckSessionMR`, `ws.ActionGitLabCheckSessionMR` (`gitlab.check_session_mr`) | +| Manual URL linking | Explicit (not auto-link) | ✅ `POST /api/v1/github/task-prs` | ✅ `POST /api/v1/gitlab/task-mrs` (`AssociateExistingMRByURL`) | +| Background poller | Refresh, plus placeholder resolution — completes discovery only for a placeholder watch (`pr_number` / `mr_iid` = 0) a Discovery row above already created; never creates a new watch itself | `github.Poller` iterates `github_pr_watches`; `PRNumber == 0` resolves via `FindPRByBranch` before upserting `github_task_prs` | `gitlab.Poller.runMRMonitor` iterates `gitlab_mr_watches`; `MRIID <= 0` resolves via `FindMRByBranch` before `CheckMRWatch` upserts `gitlab_task_mrs` and publishes `gitlab.task_mr.updated` only when visible MR fields change | + +Provider dispatch is structural, not a runtime flag: `trackPushAndAssociatePR` +resolves the pushing repository's `provider` column once and routes to +exactly one provider's detect function, so a push on a GitHub repository +issues zero GitLab client calls and vice versa. Multi-repository and +multi-branch tasks scope every association/watch by `(repository_id, +branch)` on both providers, so linking one repo/branch never clobbers a +sibling. + +**Does not trigger auto-link on either provider:** + +- Opening the PR/MR in the code host's web UI with no corresponding push + event reaching Kandev (e.g. the agent's worktree was never pushed) — there + is nothing for push detection to observe. +- **Azure DevOps** (`internal/azuredevops`) has no auto-link of any kind — + it persists PR summaries against tasks but has no watch loop or push + detection. Same gap GitHub/GitLab had before this guarantee; file + separately if wanted. + ## Where Jira and Linear deliberately diverge - **Issue model:** Jira uses transitions + JQL; Linear uses state IDs + structured filters. Don't merge these schemas — the upstream APIs are genuinely different. diff --git a/apps/backend/internal/orchestrator/event_handlers_git.go b/apps/backend/internal/orchestrator/event_handlers_git.go index 260772677d..b6af657344 100644 --- a/apps/backend/internal/orchestrator/event_handlers_git.go +++ b/apps/backend/internal/orchestrator/event_handlers_git.go @@ -16,6 +16,7 @@ import ( "github.com/kandev/kandev/internal/events/bus" "github.com/kandev/kandev/internal/orchestrator/watcher" "github.com/kandev/kandev/internal/task/models" + "github.com/kandev/kandev/internal/task/service" ) // gitSnapshotPersistInterval is the minimum time between persisted live git @@ -28,6 +29,12 @@ const gitSnapshotPersistInterval = 30 * time.Second // full and a new session arrives, the oldest entry by lastWrite is evicted. const gitSnapshotCacheMaxEntries = 4096 +// gitlabProviderName is the repositories.provider value used for GitLab, +// mirrored locally the same way githubPRStateOpen mirrors the github +// package's vocabulary rather than importing task/service's unexported +// equivalent. +const gitlabProviderName = "gitlab" + type gitSnapshotCacheEntry struct { hash string lastWrite time.Time @@ -239,7 +246,7 @@ func (s *Service) trackPushAndAssociatePR(ctx context.Context, data watcher.GitE zap.String("repository_name", data.Status.RepositoryName), zap.String("branch", data.Status.Branch), zap.Bool("first_observation", !loaded)) - go s.detectPushAndAssociatePR( + go s.dispatchPushDetection( context.Background(), data.SessionID, data.TaskID, @@ -248,6 +255,73 @@ func (s *Service) trackPushAndAssociatePR(ctx context.Context, data watcher.GitE ) } +// dispatchPushDetection routes one push-detection run to the right +// provider's association logic, so the two providers' code paths issue zero +// calls into each other's client. GitHub's proven detectPushAndAssociatePR is +// called verbatim for every non-GitLab (including unknown/legacy-empty +// provider) repository — this wraps it, it does not replace it. Extracted +// from trackPushAndAssociatePR to keep that function inside the statement +// budget. +func (s *Service) dispatchPushDetection(ctx context.Context, sessionID, taskID, repositoryName, branch string) { + if s.resolvePushRepositoryProvider(ctx, sessionID, taskID, repositoryName) == gitlabProviderName { + s.detectPushAndAssociateMR(ctx, sessionID, taskID, repositoryName, branch) + return + } + s.detectPushAndAssociatePR(ctx, sessionID, taskID, repositoryName, branch) +} + +// resolvePushRepositoryProvider looks up the provider ("github", "gitlab", or +// "" when unknown/unresolvable) of the repository backing this push, reusing +// resolvePushRepo's owner/name matching rather than re-deriving it, so +// dispatchPushDetection can route without duplicating that logic. +func (s *Service) resolvePushRepositoryProvider(ctx context.Context, sessionID, taskID, repositoryName string) string { + _, _, repositoryID := s.resolvePushRepo(ctx, sessionID, taskID, repositoryName) + if repositoryID == "" { + return "" + } + store, ok := s.repo.(repoStore) + if !ok { + return "" + } + repoObj, err := store.GetRepository(ctx, repositoryID) + if err != nil || repoObj == nil { + return "" + } + if repoObj.Provider != "" { + return repoObj.Provider + } + // The row may not yet reflect a provider resolvePushRepo's own call just + // derived from the local git remote: matchPushRepo/resolveSessionRepo + // compute it in-memory and persist it via a detached backfill goroutine, + // so this read can race ahead of that write on the very first push from a + // repository with no durable provider yet. Recompute live from the same + // local checkout instead of trusting a possibly-stale empty column. + // ResolveGitRemoteProviderIdentity recognizes both github.com and + // gitlab.com remotes (the same helper resolveRepositoryProviderIdentity + // uses to backfill Repository rows in production), so this closes the + // race for either provider rather than only GitHub. + if repoObj.LocalPath != "" { + if provider, _, owner, _ := service.ResolveGitRemoteProviderIdentity(repoObj.LocalPath); provider != "" && owner != "" { + return provider + } + } + // Self-managed GitLab instances never get a durable "gitlab" Provider tag + // at all — resolveRepositoryProviderIdentity only recognizes github.com + // and gitlab.com at discovery time, so the well-known-host fallback above + // can never resolve them either; this is a permanent gap for self-managed + // repositories, not just a narrow backfill race. remote_url is their only + // durable identity signal (still populated by the same production + // backfill), so compare it against the workspace's own configured GitLab + // connection instead of a hostname allowlist. + if s.gitlabMRLinkService != nil && repoObj.RemoteURL != "" { + if workspaceID := s.taskWorkspaceID(ctx, taskID); workspaceID != "" && + s.gitlabMRLinkService.IsConfiguredGitLabHost(ctx, workspaceID, repoObj.RemoteURL) { + return gitlabProviderName + } + } + return "" +} + // shouldFirePushDetection decides whether to kick off PR association for one // status event. It fires in two cases (see trackPushAndAssociatePR doc): // diff --git a/apps/backend/internal/orchestrator/event_handlers_git_provider_race_test.go b/apps/backend/internal/orchestrator/event_handlers_git_provider_race_test.go new file mode 100644 index 0000000000..500261222c --- /dev/null +++ b/apps/backend/internal/orchestrator/event_handlers_git_provider_race_test.go @@ -0,0 +1,95 @@ +package orchestrator + +import ( + "context" + "os" + "path/filepath" + "testing" + "time" + + "github.com/kandev/kandev/internal/task/models" +) + +// seedLocalGitCheckoutForProviderTest writes a minimal ".git/config" (no real +// git binary needed) whose origin remote is remoteURL, mirroring the on-disk +// shape service.ResolveGitRemoteProviderIdentity reads. +func seedLocalGitCheckoutForProviderTest(t *testing.T, dir, remoteURL string) { + t.Helper() + gitDir := filepath.Join(dir, ".git") + if err := os.MkdirAll(gitDir, 0o755); err != nil { + t.Fatalf("mkdir git dir: %v", err) + } + config := "[core]\n\trepositoryformatversion = 0\n[remote \"origin\"]\n\turl = " + remoteURL + "\n\tfetch = +refs/heads/*:refs/remotes/origin/*\n" + if err := os.WriteFile(filepath.Join(gitDir, "config"), []byte(config), 0o644); err != nil { + t.Fatalf("write git config: %v", err) + } +} + +// seedUnbackfilledRepoForProviderTest seeds a session/task/repository row +// with no durable provider tag yet (Provider/ProviderOwner both blank) whose +// LocalPath is a local checkout of remoteURL — the pre-backfill state +// matchPushRepo's detached goroutine would otherwise fill in. +func seedUnbackfilledRepoForProviderTest(t *testing.T, remoteURL string) *Service { + t.Helper() + ctx := context.Background() + now := time.Now().UTC() + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + + localPath := t.TempDir() + seedLocalGitCheckoutForProviderTest(t, localPath, remoteURL) + + repoObj := &models.Repository{ + ID: "repo1", WorkspaceID: "ws1", Name: "widgets", + SourceType: "local", LocalPath: localPath, + CreatedAt: now, UpdatedAt: now, + } + if err := repo.CreateRepository(ctx, repoObj); err != nil { + t.Fatalf("create repository: %v", err) + } + if err := repo.CreateTaskRepository(ctx, &models.TaskRepository{ + ID: "tr1", TaskID: "t1", RepositoryID: "repo1", + CreatedAt: now, UpdatedAt: now, + }); err != nil { + t.Fatalf("create task repository: %v", err) + } + session, _ := repo.GetTaskSession(ctx, "s1") + session.RepositoryID = "repo1" + if err := repo.UpdateTaskSession(ctx, session); err != nil { + t.Fatalf("update session: %v", err) + } + + return createTestService(repo, newMockStepGetter(), newMockTaskRepo()) +} + +// TestResolvePushRepositoryProvider_LiveFallbackForUnbackfilledGitHubRepo +// closes the race cubic-dev-ai flagged: a repository row with no durable +// provider yet (ProviderOwner == "", matchPushRepo's own trigger condition +// for its detached backfill goroutine) must not route to the wrong provider +// just because that backfill's DB write hasn't landed by the time +// resolvePushRepositoryProvider does its own separate read. Recomputing live +// from the same local checkout removes the dependency on that write's +// timing. +func TestResolvePushRepositoryProvider_LiveFallbackForUnbackfilledGitHubRepo(t *testing.T) { + svc := seedUnbackfilledRepoForProviderTest(t, "https://github.com/acme/widgets.git") + + provider := svc.resolvePushRepositoryProvider(context.Background(), "s1", "t1", "") + if provider != "github" { + t.Fatalf("provider = %q, want %q (resolved live from the local checkout, not a possibly-stale DB column)", provider, "github") + } +} + +// TestResolvePushRepositoryProvider_LiveFallbackForUnbackfilledGitLabRepo +// pins the same guarantee for GitLab: the live fallback must use a helper +// that recognizes gitlab.com (ResolveGitRemoteProviderIdentity), not one +// that only recognizes GitHub (ResolveGitRemoteProvider) — otherwise an +// unbackfilled GitLab repository's push is silently routed to the GitHub +// detection path and never auto-links. +func TestResolvePushRepositoryProvider_LiveFallbackForUnbackfilledGitLabRepo(t *testing.T) { + svc := seedUnbackfilledRepoForProviderTest(t, "https://gitlab.com/acme/widgets.git") + + provider := svc.resolvePushRepositoryProvider(context.Background(), "s1", "t1", "") + if provider != gitlabProviderName { + t.Fatalf("provider = %q, want %q (resolved live from the local checkout, not a possibly-stale DB column)", provider, gitlabProviderName) + } +} diff --git a/apps/backend/internal/orchestrator/event_handlers_github_multi_branch_assoc_test.go b/apps/backend/internal/orchestrator/event_handlers_github_multi_branch_assoc_test.go new file mode 100644 index 0000000000..d52a818ce8 --- /dev/null +++ b/apps/backend/internal/orchestrator/event_handlers_github_multi_branch_assoc_test.go @@ -0,0 +1,137 @@ +package orchestrator + +import ( + "context" + "testing" + "time" + + "github.com/kandev/kandev/internal/github" + "github.com/kandev/kandev/internal/task/models" + sqliterepo "github.com/kandev/kandev/internal/task/repository/sqlite" +) + +// seedGitHubPushAssocFixture creates the repository, task_repository +// (checked out at checkoutBranch), and session wiring shared by +// TestGitHubMultiBranchAssociationsCoexist and +// TestGitHubPushAssociationPassesRepositoryID. +func seedGitHubPushAssocFixture(t *testing.T, repo *sqliterepo.Repository, checkoutBranch string) { + t.Helper() + ctx := context.Background() + now := time.Now().UTC() + + repoObj := &models.Repository{ + ID: "repo1", WorkspaceID: "ws1", Name: "kandev", + SourceType: "provider", Provider: "github", + ProviderOwner: "myorg", ProviderName: "kandev", + CreatedAt: now, UpdatedAt: now, + } + if err := repo.CreateRepository(ctx, repoObj); err != nil { + t.Fatalf("create repository: %v", err) + } + if err := repo.CreateTaskRepository(ctx, &models.TaskRepository{ + ID: "tr1", TaskID: "t1", RepositoryID: "repo1", CheckoutBranch: checkoutBranch, + CreatedAt: now, UpdatedAt: now, + }); err != nil { + t.Fatalf("create task repository: %v", err) + } + session, _ := repo.GetTaskSession(ctx, "s1") + session.RepositoryID = "repo1" + if err := repo.UpdateTaskSession(ctx, session); err != nil { + t.Fatalf("update session: %v", err) + } +} + +// TestGitHubMultiBranchAssociationsCoexist covers the gap flagged in +// event_handlers_github_multi_branch_test.go: TestIsMultiBranchSubdir only +// pins the pure string-matching helper, not the end-to-end behavior it +// exists to support. Two branches of ONE repository in ONE session (the +// `-` sibling-worktree naming) must each get their own +// watch and association — the second branch's push must not be dropped, and +// must not clobber the first branch's already-created rows. +func TestGitHubMultiBranchAssociationsCoexist(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + seedGitHubPushAssocFixture(t, repo, "main") + + mockClient := github.NewMockClient() + mockClient.AddPR(&github.PR{ + Number: 1, Title: "Primary", HTMLURL: "https://github.com/myorg/kandev/pull/1", + RepoOwner: "myorg", RepoName: "kandev", HeadBranch: "main", BaseBranch: "main", + }) + mockClient.AddPR(&github.PR{ + Number: 2, Title: "Secondary", HTMLURL: "https://github.com/myorg/kandev/pull/2", + RepoOwner: "myorg", RepoName: "kandev", HeadBranch: "feature-x", BaseBranch: "main", + }) + ghSvc := &mockGitHubService{client: mockClient} + svc := createTestService(repo, newMockStepGetter(), newMockTaskRepo()) + svc.SetGitHubService(ghSvc) + + // Primary branch push: repositoryName empty (legacy single-repo tag). + svc.detectPushAndAssociatePR(ctx, "s1", "t1", "", "main") + // Secondary branch push: repositoryName tagged with the multi-branch + // sibling-worktree subdir naming ("-"). + svc.detectPushAndAssociatePR(ctx, "s1", "t1", "kandev-feature-x", "feature-x") + + if ghSvc.createWatchCalls != 2 { + t.Fatalf("expected 2 CreatePRWatch calls (one per branch), got %d", ghSvc.createWatchCalls) + } + if ghSvc.associateCalls != 2 { + t.Fatalf("expected 2 AssociatePRWithTask calls (one per branch), got %d", ghSvc.associateCalls) + } + if len(ghSvc.createWatchLog) != 2 || len(ghSvc.associateLog) != 2 { + t.Fatalf("call logs incomplete: createWatch=%+v associate=%+v", ghSvc.createWatchLog, ghSvc.associateLog) + } + + branches := map[string]bool{} + for _, c := range ghSvc.createWatchLog { + branches[c.Branch] = true + // AC17: the multi-repo/multi-branch path must pass a non-empty + // repositoryID on every call — an empty value here means the push + // silently fell back to whichever watch already existed instead of + // creating its own, the exact regression the event_handlers_github.go + // comment near associatePRFromPushScoped warns about. + if c.RepositoryID == "" { + t.Fatalf("CreatePRWatch call for branch %q had empty repositoryID", c.Branch) + } + } + if !branches["main"] || !branches["feature-x"] { + t.Fatalf("expected watches for both main and feature-x, got %+v", ghSvc.createWatchLog) + } + + for _, c := range ghSvc.associateLog { + if c.RepositoryID == "" { + t.Fatalf("AssociatePRWithTask call for branch %q had empty repositoryID", c.Branch) + } + } +} + +// TestGitHubPushAssociationPassesRepositoryID pins the AC17 invariant on the +// single-repo path too: even when there is only one repository, push +// detection must resolve and pass its repositoryID rather than an empty +// string, since AssociatePRWithTaskForWorkspace and CreatePRWatchForWorkspace +// use it to scope the row. +func TestGitHubPushAssociationPassesRepositoryID(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + seedGitHubPushAssocFixture(t, repo, "main") + + mockClient := github.NewMockClient() + mockClient.AddPR(&github.PR{ + Number: 1, Title: "PR", HTMLURL: "https://github.com/myorg/kandev/pull/1", + RepoOwner: "myorg", RepoName: "kandev", HeadBranch: "main", BaseBranch: "main", + }) + ghSvc := &mockGitHubService{client: mockClient} + svc := createTestService(repo, newMockStepGetter(), newMockTaskRepo()) + svc.SetGitHubService(ghSvc) + + svc.detectPushAndAssociatePR(ctx, "s1", "t1", "", "main") + + if ghSvc.lastCreateWatchRepositoryID != "repo1" { + t.Fatalf("CreatePRWatch repositoryID = %q, want repo1", ghSvc.lastCreateWatchRepositoryID) + } + if ghSvc.lastAssociateRepositoryID != "repo1" { + t.Fatalf("AssociatePRWithTask repositoryID = %q, want repo1", ghSvc.lastAssociateRepositoryID) + } +} diff --git a/apps/backend/internal/orchestrator/event_handlers_github_test.go b/apps/backend/internal/orchestrator/event_handlers_github_test.go index b3ebf7cda1..11232357cb 100644 --- a/apps/backend/internal/orchestrator/event_handlers_github_test.go +++ b/apps/backend/internal/orchestrator/event_handlers_github_test.go @@ -12,6 +12,14 @@ import ( v1 "github.com/kandev/kandev/pkg/api/v1" ) +// repoBranchCall records one (repositoryID, branch) pair observed by a +// mockGitHubService call, so multi-branch tests can assert per-call scoping +// instead of only the most recent call's arguments. +type repoBranchCall struct { + RepositoryID string + Branch string +} + // mockGitHubService implements GitHubService for testing. type mockGitHubService struct { mu sync.Mutex @@ -25,20 +33,26 @@ type mockGitHubService struct { triggerPRSyncAllErr error triggerPRSyncAllCalls int getTaskPRCalls int - exactTaskPRCalls int - lastExactPRLookup github.PRFeedbackEvent - prWatch *github.PRWatch // returned by GetPRWatchBySession (nil = no watch) - ensureWatchCalls int - createWatchCalls int - associateCalls int - updateBranchCalls int - updatePRNumberCalls int - resetWatchCalls int - resetWatchBranch string - ensureWatchBranch string - createWatchBranch string - updatedBranch string - updatedPRNumber int + // getPRWatchBySessionRepoAndBranchCalls counts detectPushAndAssociatePR's + // entry-point watch lookup, so provider-isolation tests can positively + // assert the GitHub path actually ran rather than only asserting GitLab + // calls stayed at zero (which can't distinguish "routed to GitHub" from + // "routed nowhere"). + getPRWatchBySessionRepoAndBranchCalls int + exactTaskPRCalls int + lastExactPRLookup github.PRFeedbackEvent + prWatch *github.PRWatch // returned by GetPRWatchBySession (nil = no watch) + ensureWatchCalls int + createWatchCalls int + associateCalls int + updateBranchCalls int + updatePRNumberCalls int + resetWatchCalls int + resetWatchBranch string + ensureWatchBranch string + createWatchBranch string + updatedBranch string + updatedPRNumber int // repository_id captured by the most recent CreatePRWatch / // AssociatePRWithTask call. Used by the multi-repo push tests to assert // the per-repo scoping (an empty value indicates the legacy single-repo @@ -47,6 +61,12 @@ type mockGitHubService struct { lastAssociateRepositoryID string lastCreateWatchWorkspaceID string lastAssociateWorkspaceID string + // createWatchLog/associateLog record every call (not just the last), so + // multi-branch tests can assert each branch got its own watch/association + // scoped to the right repository, rather than only inspecting whichever + // call happened to run last. + createWatchLog []repoBranchCall + associateLog []repoBranchCall // Review PR reservation tracking. reserveCalls int @@ -298,12 +318,14 @@ func (m *mockGitHubService) GetPRWatchBySessionAndRepo(_ context.Context, _, _ s return m.prWatch, nil } func (m *mockGitHubService) GetPRWatchBySessionRepoAndBranch(_ context.Context, _, _, _ string) (*github.PRWatch, error) { + m.getPRWatchBySessionRepoAndBranchCalls++ return m.prWatch, nil } func (m *mockGitHubService) CreatePRWatch(_ context.Context, _, _, repositoryID, _, _ string, _ int, branch string) (*github.PRWatch, error) { m.createWatchCalls++ m.createWatchBranch = branch m.lastCreateWatchRepositoryID = repositoryID + m.createWatchLog = append(m.createWatchLog, repoBranchCall{RepositoryID: repositoryID, Branch: branch}) return &github.PRWatch{}, nil } func (m *mockGitHubService) CreatePRWatchForWorkspace( @@ -312,9 +334,14 @@ func (m *mockGitHubService) CreatePRWatchForWorkspace( m.lastCreateWatchWorkspaceID = workspaceID return m.CreatePRWatch(ctx, sessionID, taskID, repositoryID, owner, repo, prNumber, branch) } -func (m *mockGitHubService) AssociatePRWithTask(_ context.Context, _, repositoryID string, _ *github.PR) (*github.TaskPR, error) { +func (m *mockGitHubService) AssociatePRWithTask(_ context.Context, _, repositoryID string, pr *github.PR) (*github.TaskPR, error) { m.associateCalls++ m.lastAssociateRepositoryID = repositoryID + branch := "" + if pr != nil { + branch = pr.HeadBranch + } + m.associateLog = append(m.associateLog, repoBranchCall{RepositoryID: repositoryID, Branch: branch}) return &github.TaskPR{}, nil } func (m *mockGitHubService) AssociatePRWithTaskForWorkspace( diff --git a/apps/backend/internal/orchestrator/event_handlers_gitlab.go b/apps/backend/internal/orchestrator/event_handlers_gitlab.go index e8caa19808..e2f769257d 100644 --- a/apps/backend/internal/orchestrator/event_handlers_gitlab.go +++ b/apps/backend/internal/orchestrator/event_handlers_gitlab.go @@ -20,6 +20,14 @@ func (s *Service) SetGitLabService(svc GitLabWatchService) { s.gitlabIssueSource = NewGitLabIssueWatcherSource(svc, s.logger) } +// SetGitLabMRLinkService wires the auto-link surface into the orchestrator so +// push detection and the on-demand check can find and associate merge +// requests opened outside Kandev's own Create-PR action. Mirrors +// SetGitHubService. +func (s *Service) SetGitLabMRLinkService(svc GitLabMRLinkService) { + s.gitlabMRLinkService = svc +} + // SetGitLabCredentialResolver binds execution auth to the task workspace. func (s *Service) SetGitLabCredentialResolver(resolver executor.GitLabCredentialResolver) { s.executor.SetGitLabCredentialResolver(resolver) diff --git a/apps/backend/internal/orchestrator/event_handlers_gitlab_mr.go b/apps/backend/internal/orchestrator/event_handlers_gitlab_mr.go new file mode 100644 index 0000000000..d4fbbcd1a1 --- /dev/null +++ b/apps/backend/internal/orchestrator/event_handlers_gitlab_mr.go @@ -0,0 +1,267 @@ +package orchestrator + +import ( + "context" + "strings" + "time" + + "go.uber.org/zap" + + "github.com/kandev/kandev/internal/gitlab" +) + +// gitlabMRStateOpen mirrors the gitlab package's own normalized "open" state +// value (GitLab's API returns "opened"; the client normalizes it to "open" +// before it ever reaches a TaskMR row). Kept as a local constant rather than +// exported from the gitlab package, matching how event_handlers_github.go's +// githubPRStateOpen mirrors the github package's vocabulary. +const gitlabMRStateOpen = "open" + +// detectPushAndAssociateMR is the GitLab twin of detectPushAndAssociatePR: on +// push to a session branch, it looks up the open merge request whose source +// branch matches and links it to the task, scoped by repository_id. Mirrors +// the GitHub retry shape ([0, 30s, 60s]) so an MR opened moments after the +// push (a common `glab mr create` race) still gets picked up. +func (s *Service) detectPushAndAssociateMR( + ctx context.Context, sessionID, taskID, repositoryName, branch string, +) { + if s.gitlabMRLinkService == nil { + return + } + // An empty branch must never reach FindMRByBranch: it builds + // `?source_branch=&state=opened&per_page=1`, which carries no effective + // source-branch filter, so GitLab answers with an arbitrary open MR of the + // project and we would link the wrong merge request to the task. Git refs + // cannot contain spaces, so a whitespace-only value is equally invalid. + // CheckSessionMR already refuses an empty branch; this is the same guard on + // the push path. + branch = strings.TrimSpace(branch) + if branch == "" { + return + } + workspaceID := s.taskWorkspaceID(ctx, taskID) + if workspaceID == "" { + return + } + owner, repoName, repositoryID := s.resolvePushRepo(ctx, sessionID, taskID, repositoryName) + if owner == "" || repoName == "" || repositoryID == "" { + return + } + projectPath := owner + "/" + repoName + + // Already linked for this (task, repository, branch) — don't re-link, but + // still make sure the refresh watch exists. AssociateExistingMRByURL (the + // Create-MR action and manual URL linking) writes gitlab_task_mrs without + // a watch, so returning outright here would leave the association with + // nothing for Poller.runMRMonitor to poll and its review/pipeline status + // would never update. + if s.alreadyLinkedGitLabMR(ctx, sessionID, taskID, repositoryID, branch) { + return + } + + delays := []time.Duration{0, 30 * time.Second, 60 * time.Second} + for _, delay := range delays { + if delay > 0 { + select { + case <-ctx.Done(): + return + case <-time.After(delay): + } + if s.alreadyLinkedGitLabMR(ctx, sessionID, taskID, repositoryID, branch) { + return + } + } + if s.tryAutoLinkMRForPush(ctx, workspaceID, sessionID, taskID, repositoryID, repositoryName, projectPath, branch, delay) { + return + } + } + s.logger.Warn("exhausted all retries, no gitlab MR found after push", + zap.String("session_id", sessionID), + zap.String("task_id", taskID), + zap.String("repository_name", repositoryName), + zap.String("branch", branch)) +} + +// alreadyLinkedGitLabMR checks for an existing (repositoryID, branch) +// association and, if found, ensures its refresh watch before reporting +// true. Checked both before the retry loop starts and again after each +// delay, in case a concurrent Create-MR action or manual URL link races +// ahead of this push detection. +func (s *Service) alreadyLinkedGitLabMR(ctx context.Context, sessionID, taskID, repositoryID, branch string) bool { + existing := s.gitlabTaskMRFor(ctx, taskID, repositoryID, branch) + if existing == nil { + return false + } + s.ensureWatchForLinkedMR(ctx, sessionID, taskID, repositoryID, existing) + return true +} + +// tryAutoLinkMRForPush runs one AutoLinkMRForBranch attempt for the push +// retry loop, logging the outcome. Returns true when a link was made (the +// caller's retry loop should stop). +func (s *Service) tryAutoLinkMRForPush( + ctx context.Context, workspaceID, sessionID, taskID, repositoryID, repositoryName, projectPath, branch string, delay time.Duration, +) bool { + assoc, err := s.gitlabMRLinkService.AutoLinkMRForBranch( + ctx, workspaceID, sessionID, taskID, repositoryID, projectPath, branch, + ) + if err != nil { + // A real service/store error (e.g. an unconfigured GitLab connection) + // is a backend problem, not "no MR is open yet" — worth a louder log + // than the normal not-found retry case, even though the retry loop + // itself still just tries again (a transient error may clear before + // the next delay). + s.logger.Warn("gitlab auto-link attempt failed (will retry)", + zap.String("branch", branch), + zap.String("session_id", sessionID), + zap.String("repository_name", repositoryName), + zap.Duration("delay", delay), + zap.Error(err)) + return false + } + if assoc == nil { + s.logger.Debug("no gitlab MR found for branch (will retry)", + zap.String("branch", branch), + zap.String("session_id", sessionID), + zap.String("repository_name", repositoryName), + zap.Duration("delay", delay)) + return false + } + s.logger.Info("gitlab MR found after push, associated with task", + zap.String("session_id", sessionID), + zap.String("task_id", taskID), + zap.String("repository_name", repositoryName), + zap.Int("mr_iid", assoc.MRIID), + zap.String("branch", branch)) + // AutoLinkMRForBranch already tries to ensure the watch itself, but only + // logs a failure rather than surfacing it — this may be the push path's + // last attempt (retries are exhausted once the caller returns), so give + // the idempotent get-or-create one more chance here rather than + // declaring the link complete with the association persisted but no + // watch to keep its status polled. + s.ensureWatchForLinkedMR(ctx, sessionID, taskID, repositoryID, assoc) + return true +} + +// ensureWatchForLinkedMR creates the refresh watch for an association that +// already exists, covering the MRs linked by AssociateExistingMRByURL (the +// Create-MR action and manual URL linking), which persists gitlab_task_mrs +// but no watch. Best-effort: the association is already correct, so a watch +// failure must not turn push detection into an error path. +func (s *Service) ensureWatchForLinkedMR( + ctx context.Context, sessionID, taskID, repositoryID string, mr *gitlab.TaskMR, +) { + if _, err := s.gitlabMRLinkService.EnsureMRWatch( + ctx, sessionID, taskID, repositoryID, mr.ProjectPath, mr.MRIID, mr.HeadBranch, + ); err != nil { + s.logger.Warn("failed to ensure MR watch for already-linked merge request", + zap.String("session_id", sessionID), + zap.String("task_id", taskID), + zap.Int("mr_iid", mr.MRIID), + zap.Error(err)) + } +} + +// gitlabTaskMRFor returns taskID's existing *open* association for +// (repositoryID, branch), or nil when there is none, so retries and +// duplicate push events don't refetch or re-link an MR that's already +// linked. Scoped to state=open so a merged/closed MR's leftover row doesn't +// shadow a replacement MR opened later from the same branch (the prior MR +// closed, a new one opened): EnsureMRWatch's iid-replacement support only +// runs when the search this guards against actually re-executes instead of +// always short-circuiting on any historical row for the branch. +func (s *Service) gitlabTaskMRFor(ctx context.Context, taskID, repositoryID, branch string) *gitlab.TaskMR { + mrs, err := s.gitlabMRLinkService.ListTaskMRsByTask(ctx, taskID) + if err != nil { + return nil + } + for _, mr := range mrs { + if mr.RepositoryID == repositoryID && mr.HeadBranch == branch && mr.State == gitlabMRStateOpen { + return mr + } + } + return nil +} + +// CheckSessionMR checks whether an open GitLab merge request exists for a +// session's branch and associates it if found. On-demand counterpart to the +// push-detection path, mirroring GitHub's CheckSessionPR: a caller can +// trigger immediate MR detection without waiting for the next push or the +// background poller. +func (s *Service) CheckSessionMR(ctx context.Context, taskID, sessionID string) (bool, error) { + // Per-user scoping first, before any early return — see CheckSessionPR's + // doc comment for why both IDs (not just the session) must be authorized: + // everything below is keyed off taskID, so authorizing only the session + // would let a caller pair one of their own sessions with another user's + // task. + if err := s.authorizeTaskSessionPair(ctx, taskID, sessionID); err != nil { + return false, nil + } + + if s.gitlabMRLinkService == nil { + return false, nil + } + + // Push detection never reaches this service's GitLab client for a + // GitHub-provider repository — dispatchPushDetection routes by provider + // before either detect function runs. This on-demand entry point has no + // such router in front of it (a caller invokes it directly by WS action + // name), so it must apply the same guard itself: without it, calling + // gitlab.check_session_mr for a GitHub-backed session would install a + // bogus GitLab watch keyed off the GitHub repository's owner/name before + // AutoLinkMRForBranch's own identity check ever runs. + if s.resolvePushRepositoryProvider(ctx, sessionID, taskID, "") != gitlabProviderName { + return false, nil + } + + owner, repoName, repositoryID := s.resolvePushRepo(ctx, sessionID, taskID, "") + if owner == "" || repoName == "" || repositoryID == "" { + return false, nil + } + branch := strings.TrimSpace(s.resolvePRWatchBranch(ctx, taskID, sessionID, "")) + if branch == "" { + return false, nil + } + projectPath := owner + "/" + repoName + + // Already associated for this exact (repository, branch) — ensure its + // watch exists (Create-MR action / manual URL linking writes + // gitlab_task_mrs but no watch) and return without re-searching. Scoped + // to (repositoryID, branch) rather than "any MR linked to the task", so a + // multi-repo/multi-branch task's on-demand check for one branch isn't + // short-circuited by a sibling branch's already-linked MR. + if existing := s.gitlabTaskMRFor(ctx, taskID, repositoryID, branch); existing != nil { + s.ensureWatchForLinkedMR(ctx, sessionID, taskID, repositoryID, existing) + return true, nil + } + + workspaceID := s.taskWorkspaceID(ctx, taskID) + if workspaceID == "" { + return false, nil + } + + // Ensure a watch exists (mr_iid=0, backfilled by AutoLinkMRForBranch on a + // match) before searching, so the background poller keeps checking this + // branch even when no MR is open yet — mirrors CheckSessionPR's + // EnsurePRWatchForWorkspace-before-lookup ordering. + if _, err := s.gitlabMRLinkService.EnsureMRWatch(ctx, sessionID, taskID, repositoryID, projectPath, 0, branch); err != nil { + s.logger.Warn("failed to ensure MR watch during check", + zap.String("session_id", sessionID), zap.Error(err)) + } + + assoc, err := s.gitlabMRLinkService.AutoLinkMRForBranch( + ctx, workspaceID, sessionID, taskID, repositoryID, projectPath, branch, + ) + // A real service/store error (e.g. an unconfigured GitLab connection) is + // a backend problem, not "no MR is open on this branch" — propagate it + // to the WS caller rather than reporting a misleadingly clean + // found=false. Only assoc == nil with a nil error means "searched and + // found nothing." + if err != nil { + return false, err + } + if assoc == nil { + return false, nil + } + return true, nil +} diff --git a/apps/backend/internal/orchestrator/event_handlers_gitlab_mr_regression_test.go b/apps/backend/internal/orchestrator/event_handlers_gitlab_mr_regression_test.go new file mode 100644 index 0000000000..426f74c55f --- /dev/null +++ b/apps/backend/internal/orchestrator/event_handlers_gitlab_mr_regression_test.go @@ -0,0 +1,57 @@ +package orchestrator + +import ( + "context" + "testing" + + "github.com/kandev/kandev/internal/gitlab" +) + +// A branch that is empty (or whitespace-only, which no git ref can be) must +// never reach AutoLinkMRForBranch. PATClient.FindMRByBranch interpolates it +// into `?source_branch=&state=opened&per_page=1`, a query with no effective +// source-branch filter, so GitLab answers with an arbitrary open merge +// request of the project and the wrong MR gets linked to the task. +// CheckSessionMR has always refused an empty branch; the push path must too. +func TestDetectPushAndAssociateMR_IgnoresBlankBranch(t *testing.T) { + for _, branch := range []string{"", " ", "\t\n"} { + t.Run("branch="+branch, func(t *testing.T) { + svc, fake := seedGitLabSessionWithRepo(t) + fake.autoLinkFunc = func(_ context.Context, _, _, _, _, _, b string) (*gitlab.TaskMR, error) { + t.Fatalf("AutoLinkMRForBranch called with blank branch %q", b) + return nil, nil + } + + svc.detectPushAndAssociateMR(context.Background(), "s1", "t1", "myproj", branch) + + if fake.autoLinkCalls != 0 { + t.Fatalf("expected 0 auto-link calls for blank branch, got %d", fake.autoLinkCalls) + } + }) + } +} + +// AssociateExistingMRByURL — the path behind both Kandev's own Create-MR +// action and manual URL linking — writes gitlab_task_mrs but no watch. Push +// detection must therefore create the missing watch rather than returning as +// soon as it sees an association, otherwise Poller.runMRMonitor has nothing +// to poll and the MR's review/pipeline status never populates on the task. +func TestDetectPushAndAssociateMR_EnsuresWatchForAlreadyLinkedMR(t *testing.T) { + svc, fake := seedGitLabSessionWithRepo(t) + fake.taskMRs = map[string][]*gitlab.TaskMR{ + "t1": {{ + TaskID: "t1", RepositoryID: "repo1", + ProjectPath: "group/myproj", MRIID: 42, HeadBranch: "feat/a", + State: gitlabMRStateOpen, + }}, + } + + svc.detectPushAndAssociateMR(context.Background(), "s1", "t1", "myproj", "feat/a") + + if fake.autoLinkCalls != 0 { + t.Errorf("already-linked MR must not be re-linked, got %d auto-link calls", fake.autoLinkCalls) + } + if fake.ensureWatchCalls != 1 { + t.Fatalf("expected the missing watch to be ensured exactly once, got %d", fake.ensureWatchCalls) + } +} diff --git a/apps/backend/internal/orchestrator/event_handlers_gitlab_mr_test.go b/apps/backend/internal/orchestrator/event_handlers_gitlab_mr_test.go new file mode 100644 index 0000000000..99efdad440 --- /dev/null +++ b/apps/backend/internal/orchestrator/event_handlers_gitlab_mr_test.go @@ -0,0 +1,482 @@ +package orchestrator + +import ( + "context" + "sync" + "testing" + "time" + + "github.com/kandev/kandev/internal/github" + "github.com/kandev/kandev/internal/gitlab" + "github.com/kandev/kandev/internal/task/models" +) + +// fakeGitLabMRLinkService implements GitLabMRLinkService for testing. +type fakeGitLabMRLinkService struct { + mu sync.Mutex + + autoLinkCalls int + autoLinkFunc func(ctx context.Context, workspaceID, sessionID, taskID, repositoryID, projectPath, branch string) (*gitlab.TaskMR, error) + // lastAutoLinkRepositoryID records the repositoryID argument of the most + // recent AutoLinkMRForBranch call, so multi-repo tests can assert scoping. + lastAutoLinkRepositoryID string + + ensureWatchCalls int + + taskMRs map[string][]*gitlab.TaskMR + + isConfiguredGitLabHostCalls int + isConfiguredGitLabHostFunc func(ctx context.Context, workspaceID, remoteURL string) bool +} + +func (f *fakeGitLabMRLinkService) AutoLinkMRForBranch( + ctx context.Context, workspaceID, sessionID, taskID, repositoryID, projectPath, branch string, +) (*gitlab.TaskMR, error) { + f.mu.Lock() + f.autoLinkCalls++ + f.lastAutoLinkRepositoryID = repositoryID + fn := f.autoLinkFunc + f.mu.Unlock() + if fn != nil { + return fn(ctx, workspaceID, sessionID, taskID, repositoryID, projectPath, branch) + } + return nil, nil +} + +func (f *fakeGitLabMRLinkService) EnsureMRWatch( + _ context.Context, _, _, _, _ string, _ int, _ string, +) (*gitlab.MRWatch, error) { + f.mu.Lock() + f.ensureWatchCalls++ + f.mu.Unlock() + return &gitlab.MRWatch{}, nil +} + +func (f *fakeGitLabMRLinkService) ListTaskMRsByTask(_ context.Context, taskID string) ([]*gitlab.TaskMR, error) { + f.mu.Lock() + defer f.mu.Unlock() + return f.taskMRs[taskID], nil +} + +func (f *fakeGitLabMRLinkService) IsConfiguredGitLabHost(ctx context.Context, workspaceID, remoteURL string) bool { + f.mu.Lock() + f.isConfiguredGitLabHostCalls++ + fn := f.isConfiguredGitLabHostFunc + f.mu.Unlock() + if fn != nil { + return fn(ctx, workspaceID, remoteURL) + } + return false +} + +// seedGitLabSessionWithRepo mirrors event_handlers_github_test.go's +// seedSessionWithRepo, but tags the repository as a GitLab provider so +// resolvePushRepo/resolvePushRepositoryProvider route to the GitLab path. +func seedGitLabSessionWithRepo(t *testing.T) (*Service, *fakeGitLabMRLinkService) { + t.Helper() + ctx := context.Background() + now := time.Now().UTC() + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + + repoObj := &models.Repository{ + ID: "repo1", WorkspaceID: "ws1", Name: "myproj", + SourceType: "provider", Provider: gitlabProviderName, + ProviderOwner: "group", ProviderName: "myproj", + CreatedAt: now, UpdatedAt: now, + } + if err := repo.CreateRepository(ctx, repoObj); err != nil { + t.Fatalf("create repository: %v", err) + } + if err := repo.CreateTaskRepository(ctx, &models.TaskRepository{ + ID: "tr1", TaskID: "t1", RepositoryID: "repo1", CheckoutBranch: "feat/a", + CreatedAt: now, UpdatedAt: now, + }); err != nil { + t.Fatalf("create task repository: %v", err) + } + + session, _ := repo.GetTaskSession(ctx, "s1") + session.RepositoryID = "repo1" + if err := repo.UpdateTaskSession(ctx, session); err != nil { + t.Fatalf("update session: %v", err) + } + + svc := createTestService(repo, newMockStepGetter(), newMockTaskRepo()) + fake := &fakeGitLabMRLinkService{taskMRs: make(map[string][]*gitlab.TaskMR)} + svc.SetGitLabMRLinkService(fake) + return svc, fake +} + +func TestDetectPushAndAssociateMR_Immediate(t *testing.T) { + svc, fake := seedGitLabSessionWithRepo(t) + fake.autoLinkFunc = func(_ context.Context, _, _, _, repositoryID, projectPath, branch string) (*gitlab.TaskMR, error) { + return &gitlab.TaskMR{TaskID: "t1", RepositoryID: repositoryID, ProjectPath: projectPath, MRIID: 5, HeadBranch: branch}, nil + } + + svc.detectPushAndAssociateMR(context.Background(), "s1", "t1", "", "feat/a") + + if fake.autoLinkCalls != 1 { + t.Fatalf("expected 1 AutoLinkMRForBranch call, got %d", fake.autoLinkCalls) + } + if fake.lastAutoLinkRepositoryID != "repo1" { + t.Fatalf("repositoryID = %q, want repo1", fake.lastAutoLinkRepositoryID) + } +} + +func TestDetectPushAndAssociateMR_AlreadyLinkedIsNoOp(t *testing.T) { + svc, fake := seedGitLabSessionWithRepo(t) + fake.taskMRs["t1"] = []*gitlab.TaskMR{ + {TaskID: "t1", RepositoryID: "repo1", HeadBranch: "feat/a", MRIID: 9, State: gitlabMRStateOpen}, + } + + svc.detectPushAndAssociateMR(context.Background(), "s1", "t1", "", "feat/a") + + if fake.autoLinkCalls != 0 { + t.Fatalf("expected no AutoLinkMRForBranch calls when already linked, got %d", fake.autoLinkCalls) + } +} + +func TestDetectPushAndAssociateMR_NoServiceIsNoOp(t *testing.T) { + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + svc := createTestService(repo, newMockStepGetter(), newMockTaskRepo()) + + // Must not panic with gitlabMRLinkService unwired. + svc.detectPushAndAssociateMR(context.Background(), "s1", "t1", "", "feat/a") +} + +func TestTrackPushDispatchesByProvider_GitLabRepoCallsZeroGitHub(t *testing.T) { + svc, fake := seedGitLabSessionWithRepo(t) + fake.autoLinkFunc = func(_ context.Context, _, _, _, repositoryID, projectPath, branch string) (*gitlab.TaskMR, error) { + return &gitlab.TaskMR{TaskID: "t1", RepositoryID: repositoryID, ProjectPath: projectPath, MRIID: 1, HeadBranch: branch}, nil + } + ghSvc := &mockGitHubService{} + svc.SetGitHubService(ghSvc) + + svc.dispatchPushDetection(context.Background(), "s1", "t1", "", "feat/a") + + if fake.autoLinkCalls != 1 { + t.Fatalf("expected the GitLab path to run, got %d AutoLinkMRForBranch calls", fake.autoLinkCalls) + } + if ghSvc.associateCalls != 0 || ghSvc.createWatchCalls != 0 || ghSvc.getTaskPRCalls != 0 { + t.Fatalf("expected zero GitHub calls for a GitLab-provider repository, got associate=%d createWatch=%d getTaskPR=%d", + ghSvc.associateCalls, ghSvc.createWatchCalls, ghSvc.getTaskPRCalls) + } +} + +func TestTrackPushDispatchesByProvider_GitHubRepoCallsZeroGitLab(t *testing.T) { + ctx := context.Background() + now := time.Now().UTC() + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + repoObj := &models.Repository{ + ID: "repo1", WorkspaceID: "ws1", Name: "myrepo", + SourceType: "provider", Provider: "github", + ProviderOwner: "myorg", ProviderName: "myrepo", + CreatedAt: now, UpdatedAt: now, + } + if err := repo.CreateRepository(ctx, repoObj); err != nil { + t.Fatalf("create repository: %v", err) + } + session, _ := repo.GetTaskSession(ctx, "s1") + session.RepositoryID = "repo1" + if err := repo.UpdateTaskSession(ctx, session); err != nil { + t.Fatalf("update session: %v", err) + } + + svc := createTestService(repo, newMockStepGetter(), newMockTaskRepo()) + ghSvc := &mockGitHubService{ + prWatch: &github.PRWatch{ + ID: "w1", SessionID: "s1", TaskID: "t1", + Owner: "myorg", Repo: "myrepo", PRNumber: 10, Branch: "feature-branch", + }, + } + svc.SetGitHubService(ghSvc) + fake := &fakeGitLabMRLinkService{taskMRs: make(map[string][]*gitlab.TaskMR)} + svc.SetGitLabMRLinkService(fake) + + svc.dispatchPushDetection(ctx, "s1", "t1", "", "feature-branch") + + if fake.autoLinkCalls != 0 || fake.ensureWatchCalls != 0 { + t.Fatalf("expected zero GitLab calls for a GitHub-provider repository, got autoLink=%d ensureWatch=%d", + fake.autoLinkCalls, fake.ensureWatchCalls) + } + // Positively assert the GitHub path actually ran — zero GitLab calls + // alone can't distinguish "routed to GitHub" from "routed nowhere", + // which would silently pass this test even if dispatchPushDetection's + // routing broke entirely. + if ghSvc.getPRWatchBySessionRepoAndBranchCalls != 1 { + t.Fatalf("expected detectPushAndAssociatePR to look up the PR watch exactly once, got %d", + ghSvc.getPRWatchBySessionRepoAndBranchCalls) + } +} + +func TestCheckSessionMR_Found(t *testing.T) { + svc, fake := seedGitLabSessionWithRepo(t) + fake.autoLinkFunc = func(_ context.Context, _, _, _, repositoryID, projectPath, branch string) (*gitlab.TaskMR, error) { + return &gitlab.TaskMR{TaskID: "t1", RepositoryID: repositoryID, ProjectPath: projectPath, MRIID: 3, HeadBranch: branch}, nil + } + + found, err := svc.CheckSessionMR(context.Background(), "t1", "s1") + if err != nil { + t.Fatalf("CheckSessionMR: %v", err) + } + if !found { + t.Fatal("expected found=true when an open MR exists on the session branch") + } +} + +func TestCheckSessionMR_NotFound(t *testing.T) { + svc, _ := seedGitLabSessionWithRepo(t) + + found, err := svc.CheckSessionMR(context.Background(), "t1", "s1") + if err != nil { + t.Fatalf("CheckSessionMR: %v", err) + } + if found { + t.Fatal("expected found=false when no MR is open on the session branch") + } +} + +// TestCheckSessionMR_ScopesToCurrentBranchNotAnyLinkedMR guards against a +// multi-repo/multi-branch regression: a task that already has an MR linked +// on a DIFFERENT (repository, branch) than the caller's current session must +// not short-circuit as "found" on that unrelated association — it must still +// search and link the current branch's own MR. +func TestCheckSessionMR_ScopesToCurrentBranchNotAnyLinkedMR(t *testing.T) { + svc, fake := seedGitLabSessionWithRepo(t) + // An MR already linked on a sibling branch of the same repo. + fake.taskMRs["t1"] = []*gitlab.TaskMR{ + {TaskID: "t1", RepositoryID: "repo1", HeadBranch: "feat/other", MRIID: 99}, + } + fake.autoLinkFunc = func(_ context.Context, _, _, _, repositoryID, projectPath, branch string) (*gitlab.TaskMR, error) { + return &gitlab.TaskMR{TaskID: "t1", RepositoryID: repositoryID, ProjectPath: projectPath, MRIID: 3, HeadBranch: branch}, nil + } + + found, err := svc.CheckSessionMR(context.Background(), "t1", "s1") + if err != nil { + t.Fatalf("CheckSessionMR: %v", err) + } + if !found { + t.Fatal("expected found=true after linking the current branch's own MR") + } + if fake.autoLinkCalls != 1 { + t.Fatalf("expected AutoLinkMRForBranch to run for the current branch despite a sibling-branch association, got %d calls", fake.autoLinkCalls) + } +} + +// TestCheckSessionMR_EnsuresWatchForAlreadyLinkedMR covers the case an +// already-linked MR (via Create-MR action or manual URL linking, which write +// gitlab_task_mrs but no watch) is found for the exact current (repository, +// branch): the check must ensure its watch rather than just reporting found. +func TestCheckSessionMR_EnsuresWatchForAlreadyLinkedMR(t *testing.T) { + svc, fake := seedGitLabSessionWithRepo(t) + fake.taskMRs["t1"] = []*gitlab.TaskMR{ + {TaskID: "t1", RepositoryID: "repo1", ProjectPath: "group/myproj", HeadBranch: "feat/a", MRIID: 7, State: gitlabMRStateOpen}, + } + + found, err := svc.CheckSessionMR(context.Background(), "t1", "s1") + if err != nil { + t.Fatalf("CheckSessionMR: %v", err) + } + if !found { + t.Fatal("expected found=true for an already-linked MR on the current branch") + } + if fake.autoLinkCalls != 0 { + t.Fatalf("expected no re-link search for an already-linked MR, got %d auto-link calls", fake.autoLinkCalls) + } + if fake.ensureWatchCalls != 1 { + t.Fatalf("expected the missing watch to be ensured exactly once, got %d", fake.ensureWatchCalls) + } +} + +// TestCheckSessionMR_EnsuresWatchWhenNoMRFound mirrors CheckSessionPR's +// EnsurePRWatchForWorkspace-before-lookup ordering: even when no MR is open +// yet, a watch must exist afterward so the background poller keeps checking +// this branch instead of the caller needing to poll again themselves. +func TestCheckSessionMR_EnsuresWatchWhenNoMRFound(t *testing.T) { + svc, fake := seedGitLabSessionWithRepo(t) + + found, err := svc.CheckSessionMR(context.Background(), "t1", "s1") + if err != nil { + t.Fatalf("CheckSessionMR: %v", err) + } + if found { + t.Fatal("expected found=false when no MR is open") + } + if fake.ensureWatchCalls != 1 { + t.Fatalf("expected a placeholder watch to be ensured even when no MR was found, got %d ensure-watch calls", fake.ensureWatchCalls) + } +} + +// TestCheckSessionMR_RejectsGitHubRepository guards the gap cubic-dev-ai +// flagged: on-demand check has no router in front of it the way push +// detection has in dispatchPushDetection, so it must apply the same +// provider guard itself — otherwise calling gitlab.check_session_mr for a +// GitHub-backed session would install a bogus GitLab watch keyed off the +// GitHub repository's owner/name. +func TestCheckSessionMR_RejectsGitHubRepository(t *testing.T) { + ctx := context.Background() + now := time.Now().UTC() + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + repoObj := &models.Repository{ + ID: "repo1", WorkspaceID: "ws1", Name: "myrepo", + SourceType: "provider", Provider: "github", + ProviderOwner: "myorg", ProviderName: "myrepo", + CreatedAt: now, UpdatedAt: now, + } + if err := repo.CreateRepository(ctx, repoObj); err != nil { + t.Fatalf("create repository: %v", err) + } + session, _ := repo.GetTaskSession(ctx, "s1") + session.RepositoryID = "repo1" + if err := repo.UpdateTaskSession(ctx, session); err != nil { + t.Fatalf("update session: %v", err) + } + + svc := createTestService(repo, newMockStepGetter(), newMockTaskRepo()) + fake := &fakeGitLabMRLinkService{taskMRs: make(map[string][]*gitlab.TaskMR)} + svc.SetGitLabMRLinkService(fake) + + found, err := svc.CheckSessionMR(ctx, "t1", "s1") + if err != nil { + t.Fatalf("CheckSessionMR: %v", err) + } + if found { + t.Fatal("expected found=false for a GitHub-provider repository") + } + if fake.autoLinkCalls != 0 || fake.ensureWatchCalls != 0 { + t.Fatalf("expected zero GitLab calls for a GitHub-provider repository, got autoLink=%d ensureWatch=%d", + fake.autoLinkCalls, fake.ensureWatchCalls) + } +} + +// TestResolvePushRepositoryProvider_RecognizesSelfManagedGitLabByConfiguredHost +// pins the gap cubic-dev-ai flagged: a self-managed GitLab repository never +// gets a durable "gitlab" Provider tag at all (only github.com/gitlab.com +// are recognized at discovery time), so the well-known-host live-checkout +// fallback can never resolve it either — that's a permanent gap, not a +// narrow backfill race. remote_url must instead be compared against the +// workspace's own configured GitLab connection. +func TestResolvePushRepositoryProvider_RecognizesSelfManagedGitLabByConfiguredHost(t *testing.T) { + ctx := context.Background() + now := time.Now().UTC() + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + + const selfManagedRemote = "https://gitlab.internal.example.com/group/widgets" + repoObj := &models.Repository{ + ID: "repo1", WorkspaceID: "ws1", Name: "widgets", + SourceType: "provider", RemoteURL: selfManagedRemote, + CreatedAt: now, UpdatedAt: now, + } + if err := repo.CreateRepository(ctx, repoObj); err != nil { + t.Fatalf("create repository: %v", err) + } + if err := repo.CreateTaskRepository(ctx, &models.TaskRepository{ + ID: "tr1", TaskID: "t1", RepositoryID: "repo1", + CreatedAt: now, UpdatedAt: now, + }); err != nil { + t.Fatalf("create task repository: %v", err) + } + session, _ := repo.GetTaskSession(ctx, "s1") + session.RepositoryID = "repo1" + if err := repo.UpdateTaskSession(ctx, session); err != nil { + t.Fatalf("update session: %v", err) + } + + svc := createTestService(repo, newMockStepGetter(), newMockTaskRepo()) + fake := &fakeGitLabMRLinkService{taskMRs: make(map[string][]*gitlab.TaskMR)} + fake.isConfiguredGitLabHostFunc = func(_ context.Context, workspaceID, remoteURL string) bool { + return workspaceID == "ws1" && remoteURL == selfManagedRemote + } + svc.SetGitLabMRLinkService(fake) + + provider := svc.resolvePushRepositoryProvider(ctx, "s1", "t1", "") + if provider != gitlabProviderName { + t.Fatalf("provider = %q, want gitlab (resolved via the workspace's configured GitLab host, not a github.com/gitlab.com allowlist)", provider) + } + if fake.isConfiguredGitLabHostCalls != 1 { + t.Fatalf("expected IsConfiguredGitLabHost to be consulted exactly once, got %d", fake.isConfiguredGitLabHostCalls) + } +} + +// TestDetectPushAndAssociateMR_ClosedExistingMRDoesNotShadowReplacementSearch +// covers the replacement-MR gap cubic-dev-ai flagged: a merged/closed MR's +// leftover row must not make the "already linked" shortcut trust it +// indefinitely — otherwise a new MR opened later from the same branch is +// never found, and EnsureMRWatch's iid-replacement support (added for +// exactly this scenario) is never reached because the search that would +// discover the new iid never re-runs. +func TestDetectPushAndAssociateMR_ClosedExistingMRDoesNotShadowReplacementSearch(t *testing.T) { + svc, fake := seedGitLabSessionWithRepo(t) + fake.taskMRs["t1"] = []*gitlab.TaskMR{ + {TaskID: "t1", RepositoryID: "repo1", HeadBranch: "feat/a", MRIID: 9, State: "closed"}, + } + fake.autoLinkFunc = func(_ context.Context, _, _, _, repositoryID, projectPath, branch string) (*gitlab.TaskMR, error) { + return &gitlab.TaskMR{TaskID: "t1", RepositoryID: repositoryID, ProjectPath: projectPath, MRIID: 11, HeadBranch: branch}, nil + } + + svc.detectPushAndAssociateMR(context.Background(), "s1", "t1", "", "feat/a") + + if fake.autoLinkCalls != 1 { + t.Fatalf("expected the closed existing MR to not shadow a re-search, got %d AutoLinkMRForBranch calls", fake.autoLinkCalls) + } +} + +// TestCheckSessionMR_ClosedExistingMRDoesNotShadowReplacementSearch is the +// on-demand-check twin of the push-detection replacement-MR test above. +func TestCheckSessionMR_ClosedExistingMRDoesNotShadowReplacementSearch(t *testing.T) { + svc, fake := seedGitLabSessionWithRepo(t) + fake.taskMRs["t1"] = []*gitlab.TaskMR{ + {TaskID: "t1", RepositoryID: "repo1", HeadBranch: "feat/a", MRIID: 9, State: "merged"}, + } + fake.autoLinkFunc = func(_ context.Context, _, _, _, repositoryID, projectPath, branch string) (*gitlab.TaskMR, error) { + return &gitlab.TaskMR{TaskID: "t1", RepositoryID: repositoryID, ProjectPath: projectPath, MRIID: 11, HeadBranch: branch}, nil + } + + found, err := svc.CheckSessionMR(context.Background(), "t1", "s1") + if err != nil { + t.Fatalf("CheckSessionMR: %v", err) + } + if !found { + t.Fatal("expected found=true after linking the replacement MR") + } + if fake.autoLinkCalls != 1 { + t.Fatalf("expected the merged existing MR to not shadow a re-search, got %d AutoLinkMRForBranch calls", fake.autoLinkCalls) + } +} + +func TestCheckSessionMRDeniesForeignSession(t *testing.T) { + called := false + s := &Service{ + logger: scopeTestLogger(t), + sessionAccessCheck: denyingChecker(&called), + } + + found, err := s.CheckSessionMR(context.Background(), "task-b", "sess-b") + if err != nil { + t.Fatalf("CheckSessionMR: %v", err) + } + if !called { + t.Fatal("session access checker was not consulted before the early return") + } + if found { + t.Error("found = true; a denied session must not report an MR") + } +} + +func TestCheckSessionMRDeniesOwnSessionPairedWithForeignTask(t *testing.T) { + s := ownSessionForeignTask() + s.logger = scopeTestLogger(t) + + found, err := s.CheckSessionMR(context.Background(), "task-victim", "sess-mine") + + if err != nil { + t.Fatalf("CheckSessionMR: %v", err) + } + if found { + t.Error("found = true; an owned session must not unlock another user's task") + } +} diff --git a/apps/backend/internal/orchestrator/handlers/handlers.go b/apps/backend/internal/orchestrator/handlers/handlers.go index 5bb127bfd4..91ec8f7ff0 100644 --- a/apps/backend/internal/orchestrator/handlers/handlers.go +++ b/apps/backend/internal/orchestrator/handlers/handlers.go @@ -45,6 +45,7 @@ func (h *Handlers) RegisterHandlers(d *ws.Dispatcher) { d.RegisterFunc(ws.ActionSessionSetPlanMode, h.wsSetPlanMode) d.RegisterFunc(ws.ActionSessionRename, h.wsRenameSession) d.RegisterFunc(ws.ActionGitHubCheckSessionPR, h.wsCheckSessionPR) + d.RegisterFunc(ws.ActionGitLabCheckSessionMR, h.wsCheckSessionMR) } // WS handlers @@ -290,31 +291,50 @@ func (h *Handlers) wsRespondToPermission(ctx context.Context, msg *ws.Message) ( return ws.NewResponse(msg.ID, msg.Action, resp) } -type wsCheckSessionPRRequest struct { +type wsCheckSessionAssociationRequest struct { TaskID string `json:"task_id"` SessionID string `json:"session_id"` } -func (h *Handlers) wsCheckSessionPR(ctx context.Context, msg *ws.Message) (*ws.Message, error) { - var req wsCheckSessionPRRequest - if err := msg.ParsePayload(&req); err != nil { - return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeBadRequest, "Invalid payload: "+err.Error(), nil) - } +// wsCheckSessionAssociation is the shared body for the "check session has a +// linked PR/MR" WS handlers (github.check_session_pr, gitlab.check_session_mr). +// The two providers differ only in which Service method answers the check and +// the label used in logs/errors. +func (h *Handlers) wsCheckSessionAssociation( + ctx context.Context, msg *ws.Message, req wsCheckSessionAssociationRequest, + check func(context.Context, string, string) (bool, error), label string, +) (*ws.Message, error) { if req.TaskID == "" || req.SessionID == "" { return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeValidation, "task_id and session_id are required", nil) } - found, err := h.service.CheckSessionPR(ctx, req.TaskID, req.SessionID) + found, err := check(ctx, req.TaskID, req.SessionID) if err != nil { - h.logger.Error("failed to check session PR", + h.logger.Error("failed to check "+label, zap.String("task_id", req.TaskID), zap.String("session_id", req.SessionID), zap.Error(err)) - return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeInternalError, "Failed to check session PR", nil) + return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeInternalError, "Failed to check "+label, nil) } return ws.NewResponse(msg.ID, msg.Action, map[string]bool{"found": found}) } +func (h *Handlers) wsCheckSessionPR(ctx context.Context, msg *ws.Message) (*ws.Message, error) { + var req wsCheckSessionAssociationRequest + if err := msg.ParsePayload(&req); err != nil { + return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeBadRequest, "Invalid payload: "+err.Error(), nil) + } + return h.wsCheckSessionAssociation(ctx, msg, req, h.service.CheckSessionPR, "session PR") +} + +func (h *Handlers) wsCheckSessionMR(ctx context.Context, msg *ws.Message) (*ws.Message, error) { + var req wsCheckSessionAssociationRequest + if err := msg.ParsePayload(&req); err != nil { + return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeBadRequest, "Invalid payload: "+err.Error(), nil) + } + return h.wsCheckSessionAssociation(ctx, msg, req, h.service.CheckSessionMR, "session MR") +} + type wsGetTaskSessionStatusRequest struct { TaskID string `json:"task_id"` TaskSessionID string `json:"session_id"` diff --git a/apps/backend/internal/orchestrator/service.go b/apps/backend/internal/orchestrator/service.go index d9f91761d7..4193d73339 100644 --- a/apps/backend/internal/orchestrator/service.go +++ b/apps/backend/internal/orchestrator/service.go @@ -406,6 +406,12 @@ type Service struct { azureWorkItemSource *AzureDevOpsWorkItemWatcherSource azurePullRequestSource *AzureDevOpsPullRequestWatcherSource + // gitlabMRLinkService auto-links merge requests opened outside Kandev's + // Create-PR action, mirroring what githubService does for PRs. Separate + // from gitlabService (the review/issue watch surface) — see + // GitLabMRLinkService's doc comment. + gitlabMRLinkService GitLabMRLinkService + // Repository resolver for cloning + finding/creating repos for review tasks repositoryResolver RepositoryResolver diff --git a/apps/backend/internal/orchestrator/source_gitlab.go b/apps/backend/internal/orchestrator/source_gitlab.go index c8a325a07e..ee66b44efe 100644 --- a/apps/backend/internal/orchestrator/source_gitlab.go +++ b/apps/backend/internal/orchestrator/source_gitlab.go @@ -32,6 +32,22 @@ type GitLabWatchService interface { DisableIssueWatchWithError(context.Context, string, string) error } +// GitLabMRLinkService is the auto-link surface used by push detection and the +// on-demand check to find and associate merge requests opened outside +// Kandev's own Create-PR action. Kept separate from GitLabWatchService (the +// review/issue watch dedup surface) so a test double for one need not grow to +// satisfy the other. +type GitLabMRLinkService interface { + AutoLinkMRForBranch(ctx context.Context, workspaceID, sessionID, taskID, repositoryID, projectPath, branch string) (*gitlab.TaskMR, error) + EnsureMRWatch(ctx context.Context, sessionID, taskID, repositoryID, projectPath string, iid int, branch string) (*gitlab.MRWatch, error) + ListTaskMRsByTask(ctx context.Context, taskID string) ([]*gitlab.TaskMR, error) + // IsConfiguredGitLabHost reports whether remoteURL's host matches the + // workspace's own configured GitLab connection. Used by push-detection + // provider routing to recognize self-managed GitLab repositories, which + // carry no durable "gitlab" provider tag (see resolvePushRepositoryProvider). + IsConfiguredGitLabHost(ctx context.Context, workspaceID, remoteURL string) bool +} + type GitLabReviewWatcherSource struct { service GitLabWatchService logger *logger.Logger diff --git a/apps/backend/pkg/websocket/actions.go b/apps/backend/pkg/websocket/actions.go index de22928d66..6029026bac 100644 --- a/apps/backend/pkg/websocket/actions.go +++ b/apps/backend/pkg/websocket/actions.go @@ -456,6 +456,7 @@ const ( ActionGitHubTaskPRSync = "github.task_pr.sync" ActionGitHubStats = "github.stats" ActionGitHubCheckSessionPR = "github.check_session_pr" + ActionGitLabCheckSessionMR = "gitlab.check_session_mr" // Issue watch actions ActionGitHubIssueWatchesList = "github.issue_watches.list" diff --git a/docs/public/websocket-api.md b/docs/public/websocket-api.md index c763c8076d..0b338826c4 100644 --- a/docs/public/websocket-api.md +++ b/docs/public/websocket-api.md @@ -516,6 +516,7 @@ github.task_prs.list gitlab.action_presets.list gitlab.action_presets.reset gitlab.action_presets.update +gitlab.check_session_mr gitlab.cleanup.issue_tasks gitlab.cleanup.review_tasks gitlab.issue_watches.create diff --git a/docs/specs/gitlab-integration/plan.md b/docs/specs/gitlab-integration/plan.md index 62441cd7c4..64a6083fa8 100644 --- a/docs/specs/gitlab-integration/plan.md +++ b/docs/specs/gitlab-integration/plan.md @@ -298,6 +298,32 @@ Goal: clicking "Create PR" on a GitLab task opens a real MR. - Add the new spec/plan to `docs/specs/INDEX.md` (already done) and flip status to `shipped` once merged. +### Phase 8 — MR auto-link (post-ship follow-on) + +Goal: close the gap where an MR opened outside Kandev's own MR-creation action +(`glab mr create`, or opening the MR directly in the GitLab web UI) never got +linked — only manual URL linking and the Create-PR callback did. Mirrors +GitHub's push-detection auto-link. + +- `internal/gitlab/service_task_mr_autolink.go` — `Service.AutoLinkMRForBranch` + (find-MR-by-branch → validate repository identity → upsert association → + ensure refresh watch → publish `gitlab.task_mr.updated`). +- `internal/gitlab/service_watches.go` — `EnsureMRWatch` (idempotent + get-or-create) and `CheckMRWatch` now refresh `gitlab_task_mrs` on every + poll, not just watch timestamps. +- `gitlab_mr_watches` unique key widened to `(session_id, repository_id, + branch)` via an idempotent table-rebuild migration, so multi-branch tasks + don't collide on the second branch's watch insert. +- `internal/orchestrator/event_handlers_gitlab_mr.go` — `detectPushAndAssociateMR` + (GitLab twin of GitHub's `detectPushAndAssociatePR`, same `[0, 30s, 60s]` + retry shape) and `CheckSessionMR` (on-demand check, mirrors `CheckSessionPR`). + `event_handlers_git.go`'s push-tracking dispatches by repository provider so + the two providers' code paths never call into each other's client. + `Status: shipped.` + +Out of scope for this phase (unchanged from the root spec): webhook-driven +linking, and backfilling associations for MRs opened before this shipped. + ## 4. Risks / open points - **API shape mismatches.** GitLab's MR discussions are nested diff --git a/docs/specs/gitlab-integration/spec.md b/docs/specs/gitlab-integration/spec.md index aaaac95baf..7a0abf104a 100644 --- a/docs/specs/gitlab-integration/spec.md +++ b/docs/specs/gitlab-integration/spec.md @@ -1,7 +1,7 @@ --- status: building created: 2026-05-04 -updated: 2026-07-22 +updated: 2026-08-01 owner: tbd --- @@ -47,6 +47,16 @@ workflows are not usable end to end. - Users can link an existing task to a merge request by pasting a full MR URL, including URLs from the workspace's configured self-managed host. They can unlink one association without deleting the task or upstream MR. +- A merge request opened outside Kandev's own MR-creation action (e.g. `glab mr + create` from an agent session, or opening the MR directly in the GitLab web + UI) on a task's session branch is auto-linked: on the next push the open MR + matching that branch is found and linked, scoped to the pushing repository, + with an on-demand check available as an immediate alternative to waiting for + a push or the background poller. Linking also creates the refresh watch that + keeps review/pipeline status current, so no manual link step is required for + this common case. Multi-repository and multi-branch tasks each get their own + association and watch, scoped per `(repository_id, branch)`; re-linking an + already-linked MR is a no-op rather than a duplicate. - For a workspace with GitLab configured, task context menus expose `GitLab Merge Request` inside the shared `Link` submenu. Desktop users can reach it by right-clicking a task, and touch users can reach the same action @@ -130,7 +140,14 @@ into the secret store. records. Reservation occurs before task creation; a failed dispatch releases the reservation, while a successful dispatch attaches the created task ID. - `gitlab_mr_watches` remains the linked-MR refresh record for an active task - session. It is not a replacement for a review watch. + session. It is not a replacement for a review watch. Its unique key is + `(session_id, repository_id, branch)` — branch-scoped so a session with + multiple worktrees on the same repository (multi-branch tasks) can hold one + watch per branch instead of colliding on the second branch's insert. It is + populated by both explicit URL linking and by push-detection/on-demand + auto-link, and its periodic poll refreshes the matching `gitlab_task_mrs` + row (state, pipeline, approval, merge status) in addition to notifying on + notable transitions. - GitLab notification subscription state is owned by GitLab. Kandev reads it live and does not duplicate it in SQLite.