From d8a9bb79d589799f3b7cf9948d926adf2f883f95 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Salavisa?= Date: Sat, 2 May 2026 22:36:58 +0100 Subject: [PATCH] fix(github): heal legacy PR watches so single-repo tasks don't dupe PRs (#785) After the multi-repo migration, pre-existing github_pr_watches rows kept repository_id='', so the orchestrator's reconciler (keyed on (session_id, repository_id)) inserted a second watch under the resolved id. Both watches polled independently and each created its own github_task_prs row, surfacing as a "+2 PR" badge on legacy single-repo tasks. Add a startup backfill mirroring backfillTaskPRsRepositoryID: dedup the legacy '' rows where a per-repo row exists, then stamp the remainder from task_repositories. Co-authored-by: Kandev Agent Co-authored-by: Claude Opus 4.7 --- apps/backend/internal/github/store.go | 57 +++++++++ .../internal/github/store_multi_repo_test.go | 108 ++++++++++++++++++ 2 files changed, 165 insertions(+) diff --git a/apps/backend/internal/github/store.go b/apps/backend/internal/github/store.go index 3a852f0284..d064abbefc 100644 --- a/apps/backend/internal/github/store.go +++ b/apps/backend/internal/github/store.go @@ -169,6 +169,9 @@ func (s *Store) initSchema() error { if err := s.backfillTaskPRsRepositoryID(); err != nil { return fmt.Errorf("backfill github_task_prs.repository_id: %w", err) } + if err := s.backfillPRWatchesRepositoryID(); err != nil { + return fmt.Errorf("backfill github_pr_watches.repository_id: %w", err) + } return nil } @@ -233,6 +236,60 @@ func (s *Store) backfillTaskPRsRepositoryID() error { return nil } +// backfillPRWatchesRepositoryID heals github_pr_watches rows that pre-date +// the per-repo schema (repository_id = ”). Same two-pass shape as +// backfillTaskPRsRepositoryID — without this the orchestrator's reconciler +// (which keys its existence-check by (session_id, repository_id)) would see +// the legacy `(sess, ”)` row as foreign and insert a SECOND watch row +// under the resolved repository_id. Two watches → two AssociatePRWithTask +// calls when the user opens a PR → two github_task_prs rows for the same +// PR, which is the "PR appears twice on a single-repo task" symptom we hit +// after the multi-repo rollout. +// +// 1. Dedup: drop legacy `”` rows whose session already has a non-empty +// row — the reconciler-inserted row supersedes the legacy one. +// +// 2. Backfill: stamp the remaining `”` rows with the task's primary +// repository_id from `task_repositories`. Skipped silently when the +// table is absent (unit tests that init only this package's schema). +// +// Idempotent: re-running on a healed db is a no-op. +func (s *Store) backfillPRWatchesRepositoryID() error { + if _, err := s.db.Exec(` + DELETE FROM github_pr_watches + WHERE repository_id = '' + AND EXISTS ( + SELECT 1 FROM github_pr_watches other + WHERE other.session_id = github_pr_watches.session_id + AND other.repository_id != '' + ) + `); err != nil { + return fmt.Errorf("dedup legacy PR watch rows: %w", err) + } + if !s.tableExists("task_repositories") { + return nil + } + _, err := s.db.Exec(` + UPDATE github_pr_watches + SET repository_id = ( + SELECT tr.repository_id + FROM task_repositories tr + WHERE tr.task_id = github_pr_watches.task_id + ORDER BY tr.position + LIMIT 1 + ) + WHERE repository_id = '' + AND EXISTS ( + SELECT 1 FROM task_repositories tr + WHERE tr.task_id = github_pr_watches.task_id + ) + `) + if err != nil { + return fmt.Errorf("backfill PR watch repository_id: %w", err) + } + return nil +} + // tableExists returns true when the named table is present in sqlite_master. // Used by the multi-repo backfill to skip cross-package healing in unit // tests that don't bring up the task schema. diff --git a/apps/backend/internal/github/store_multi_repo_test.go b/apps/backend/internal/github/store_multi_repo_test.go index 90fd998af6..7c970994ff 100644 --- a/apps/backend/internal/github/store_multi_repo_test.go +++ b/apps/backend/internal/github/store_multi_repo_test.go @@ -181,6 +181,114 @@ func TestBackfillTaskPRsRepositoryID_SkipsWhenTaskRepositoriesAbsent(t *testing. } } +// Regression: a single-repo task created before the multi-repo migration +// kept its github_pr_watches row with repository_id=”. The orchestrator's +// reconciler then created a SECOND watch row with the resolved +// repository_id (because (session_id, ”) != (session_id, 'X') in its dedup +// set). Both watches polled independently, so when the user opened a PR +// each one called AssociatePRWithTask with its own RepositoryID — leaving +// two github_task_prs rows for the same PR and a "+2" badge on the kanban +// card. backfillPRWatchesRepositoryID must dedup the legacy row so the +// reconciler's existence-check finds it under the resolved repository_id. +func TestBackfillPRWatchesRepositoryID_DedupsLegacyEmptyRow(t *testing.T) { + store := newTestStore(t) + ctx := context.Background() + now := time.Now().UTC() + + // Legacy watch row (repository_id='') and the per-repo row that the + // reconciler inserted post-migration. Both reference the same session. + if err := store.CreatePRWatch(ctx, &PRWatch{ + ID: "legacy-w", SessionID: "sess-1", TaskID: "task-w", RepositoryID: "", + Owner: "kdlbs", Repo: "kandev", PRNumber: 0, Branch: "feat/x", + CreatedAt: now, UpdatedAt: now, + }); err != nil { + t.Fatalf("create legacy watch: %v", err) + } + if err := store.CreatePRWatch(ctx, &PRWatch{ + ID: "scoped-w", SessionID: "sess-1", TaskID: "task-w", RepositoryID: "repo-1", + Owner: "kdlbs", Repo: "kandev", PRNumber: 0, Branch: "feat/x", + CreatedAt: now, UpdatedAt: now, + }); err != nil { + t.Fatalf("create scoped watch: %v", err) + } + + if err := store.backfillPRWatchesRepositoryID(); err != nil { + t.Fatalf("backfill: %v", err) + } + + all, err := store.ListPRWatchesBySession(ctx, "sess-1") + if err != nil { + t.Fatalf("list: %v", err) + } + if len(all) != 1 { + t.Fatalf("expected 1 watch after dedup, got %d", len(all)) + } + if all[0].RepositoryID != "repo-1" { + t.Errorf("expected scoped row to win, got repository_id=%q", all[0].RepositoryID) + } + + // Idempotent: second pass is a no-op. + if err := store.backfillPRWatchesRepositoryID(); err != nil { + t.Fatalf("second backfill: %v", err) + } + all2, _ := store.ListPRWatchesBySession(ctx, "sess-1") + if len(all2) != 1 { + t.Errorf("idempotent expected 1 watch, got %d", len(all2)) + } +} + +// When task_repositories does not exist, the watch backfill must skip the +// cross-package UPDATE rather than erroring out — same contract as the +// task-PR backfill. +func TestBackfillPRWatchesRepositoryID_SkipsWhenTaskRepositoriesAbsent(t *testing.T) { + store := newTestStore(t) + if err := store.backfillPRWatchesRepositoryID(); err != nil { + t.Fatalf("backfill on store without task_repositories: %v", err) + } +} + +// The orphan case — a single-repo task whose pre-migration watch never had a +// per-repo counterpart. The dedup pass leaves it alone; the UPDATE has to +// stamp its repository_id from task_repositories. This is what stops the +// reconciler from inserting a duplicate watch on the next poll tick. +func TestBackfillPRWatchesRepositoryID_BackfillsOrphanRow(t *testing.T) { + store := newTestStore(t) + ctx := context.Background() + + if _, err := store.db.Exec(`CREATE TABLE task_repositories ( + id TEXT PRIMARY KEY, task_id TEXT NOT NULL, + repository_id TEXT NOT NULL, position INTEGER DEFAULT 0 + )`); err != nil { + t.Fatalf("create task_repositories: %v", err) + } + if _, err := store.db.Exec( + `INSERT INTO task_repositories VALUES ('r1','task-z','repo-abc',0)`, + ); err != nil { + t.Fatalf("seed task_repositories: %v", err) + } + + now := time.Now().UTC() + if err := store.CreatePRWatch(ctx, &PRWatch{ + ID: "legacy-orphan", SessionID: "sess-2", TaskID: "task-z", RepositoryID: "", + Owner: "o", Repo: "r", PRNumber: 0, Branch: "feat/y", + CreatedAt: now, UpdatedAt: now, + }); err != nil { + t.Fatalf("create legacy: %v", err) + } + + if err := store.backfillPRWatchesRepositoryID(); err != nil { + t.Fatalf("backfill: %v", err) + } + + all, _ := store.ListPRWatchesBySession(ctx, "sess-2") + if len(all) != 1 { + t.Fatalf("expected 1 watch, got %d", len(all)) + } + if all[0].RepositoryID != "repo-abc" { + t.Errorf("expected backfilled repository_id='repo-abc', got %q", all[0].RepositoryID) + } +} + // Covers the UPDATE path: a legacy row with repository_id=” and NO per-repo // counterpart should get its repository_id stamped from task_repositories, // not deleted. This is the most common case in real upgrades — a task that