diff --git a/apps/backend/internal/github/poller.go b/apps/backend/internal/github/poller.go index 394877be68..46239c0034 100644 --- a/apps/backend/internal/github/poller.go +++ b/apps/backend/internal/github/poller.go @@ -83,9 +83,9 @@ type TaskBranchInfo struct { // TaskBranchProvider lists tasks that should have PR watches and resolves branches. type TaskBranchProvider interface { ListTasksNeedingPRWatch(ctx context.Context) ([]TaskBranchInfo, error) - // ResolveBranchForSession returns the current branch for a task+session pair. - // Used to detect branch renames and update stale PR watches. - ResolveBranchForSession(ctx context.Context, taskID, sessionID string) string + // ResolveBranchForWatch resolves a branch within the watch's source session and repository. + // An empty result leaves the watch unchanged. + ResolveBranchForWatch(ctx context.Context, watch *PRWatch) string } // Poller runs background loops for PR monitoring and review queue checking. @@ -460,9 +460,7 @@ func (p *Poller) refreshStaleBranches(ctx context.Context) { if watch.PRNumber != 0 { continue // already found a PR, branch is correct } - currentBranch := p.taskBranchProvider.ResolveBranchForSession( - ctx, watch.TaskID, watch.SessionID, - ) + currentBranch := p.taskBranchProvider.ResolveBranchForWatch(ctx, watch) if currentBranch == "" || currentBranch == watch.Branch { continue } diff --git a/apps/backend/internal/github/poller_branch_reconciliation_test.go b/apps/backend/internal/github/poller_branch_reconciliation_test.go new file mode 100644 index 0000000000..08d2b05510 --- /dev/null +++ b/apps/backend/internal/github/poller_branch_reconciliation_test.go @@ -0,0 +1,126 @@ +package github + +import ( + "context" + "testing" + "time" + + "github.com/kandev/kandev/internal/events" + "github.com/kandev/kandev/internal/events/bus" +) + +type watchIdentityProvider struct{ targets []TaskBranchInfo } + +func (p *watchIdentityProvider) ListTasksNeedingPRWatch(context.Context) ([]TaskBranchInfo, error) { + return p.targets, nil +} +func (p *watchIdentityProvider) ResolveBranchForWatch(_ context.Context, w *PRWatch) string { + for _, target := range p.targets { + if target.SessionID == w.SessionID && target.RepositoryID == w.RepositoryID && target.Branch == w.Branch { + return target.Branch + } + } + return "" +} + +// @covers AC-INTEGRATIONS-GITHUB-PR-DISCOVERY-001.1 +// @covers AC-INTEGRATIONS-GITHUB-PR-DISCOVERY-001.4 +func TestReconcileWatches_PreservesBranchTargetsAcrossCycles(t *testing.T) { + poller, svc, client, store := setupBatchedPollerTest(t) + ctx := context.Background() + seedTask(t, store, "owner", false) + provider := &watchIdentityProvider{targets: []TaskBranchInfo{ + {WorkspaceID: testWorkspaceID, TaskID: "owner", SessionID: "member-session", RepositoryID: "repo1", Owner: "o", Repo: "r", Branch: "primary"}, + {WorkspaceID: testWorkspaceID, TaskID: "owner", SessionID: "member-session", RepositoryID: "repo1", Owner: "o", Repo: "r", Branch: "secondary"}, + {WorkspaceID: testWorkspaceID, TaskID: "owner", SessionID: "member-session", RepositoryID: "repo2", Owner: "o", Repo: "other", Branch: "other-branch"}, + }} + poller.SetTaskBranchProvider(provider) + poller.reconcileWatches(ctx) + initial, err := store.ListActivePRWatches(ctx) + if err != nil { + t.Fatal(err) + } + if len(initial) != 3 { + t.Fatalf("initial watches = %d, want 3", len(initial)) + } + // A numbered sibling must survive while the other branches still search. + for _, w := range initial { + if w.Branch == "primary" { + if err := store.UpdatePRWatchPRNumber(ctx, w.ID, 99); err != nil { + t.Fatal(err) + } + } + } + initial, err = store.ListActivePRWatches(ctx) + if err != nil { + t.Fatal(err) + } + assertWatchReconciliationStable(t, poller, store, initial) + assertSecondaryWatchDiscovery(t, svc, client, store) +} + +func assertWatchReconciliationStable(t *testing.T, poller *Poller, store *Store, initial []*PRWatch) { + t.Helper() + ctx := context.Background() + for range 3 { + poller.reconcileWatches(ctx) + current, err := store.ListActivePRWatches(ctx) + if err != nil { + t.Fatal(err) + } + if len(current) != len(initial) { + t.Fatalf("watch count = %d, want %d", len(current), len(initial)) + } + for _, before := range initial { + after, err := store.GetPRWatchBySessionRepoAndBranch(ctx, before.SessionID, before.RepositoryID, before.Branch) + if err != nil { + t.Fatal(err) + } + if after == nil || after.ID != before.ID || !after.UpdatedAt.Equal(before.UpdatedAt) { + t.Fatalf("watch %s recreated or rewritten: before=%+v after=%+v", before.Branch, before, after) + } + if before.Branch == "primary" && after.PRNumber != 99 { + t.Fatal("numbered sibling changed") + } + } + } +} + +func assertSecondaryWatchDiscovery(t *testing.T, svc *Service, client *graphQLMockClient, store *Store) { + t.Helper() + ctx := context.Background() + secondary, err := store.GetPRWatchBySessionRepoAndBranch(ctx, "member-session", "repo1", "secondary") + if err != nil { + t.Fatal(err) + } + updated := make(chan *TaskPR, 1) + sub, err := svc.eventBus.Subscribe(events.GitHubTaskPRUpdated, func(_ context.Context, event *bus.Event) error { + if pr, ok := event.Data.(*TaskPR); ok { + updated <- pr + } + return nil + }) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = sub.Unsubscribe() }) + client.branchResponses = []string{branchAliasResponse(t, 1, map[int]string{0: openPRNode(7, "secondary", "alice")})} + if _, err := svc.SyncWorkspaceWatchesBatched(ctx, testWorkspaceID, []*PRWatch{secondary}); err != nil { + t.Fatal(err) + } + associated, err := store.GetTaskPR(ctx, "owner") + if err != nil { + t.Fatal(err) + } + if associated == nil || associated.PRNumber != 7 || associated.RepositoryID != "repo1" { + t.Fatalf("association = %+v", associated) + } + select { + case event := <-updated: + if event.TaskID != "owner" || event.PRNumber != 7 || event.RepositoryID != "repo1" { + t.Fatalf("event = %+v", event) + } + case <-time.After(time.Second): + t.Fatal("missing task PR event") + } +} diff --git a/apps/backend/internal/github/poller_test.go b/apps/backend/internal/github/poller_test.go index 49e1a30e3d..caf995fd84 100644 --- a/apps/backend/internal/github/poller_test.go +++ b/apps/backend/internal/github/poller_test.go @@ -248,9 +248,9 @@ func (m *mockTaskBranchProvider) ListTasksNeedingPRWatch(_ context.Context) ([]T return m.tasks, m.err } -func (m *mockTaskBranchProvider) ResolveBranchForSession(_ context.Context, _, sessionID string) string { +func (m *mockTaskBranchProvider) ResolveBranchForWatch(_ context.Context, watch *PRWatch) string { if m.branches != nil { - return m.branches[sessionID] + return m.branches[watch.SessionID] } return "" } diff --git a/apps/backend/internal/orchestrator/event_handlers_github.go b/apps/backend/internal/orchestrator/event_handlers_github.go index 2e0bbfdc0b..e2b0444180 100644 --- a/apps/backend/internal/orchestrator/event_handlers_github.go +++ b/apps/backend/internal/orchestrator/event_handlers_github.go @@ -1444,12 +1444,6 @@ func (s *Service) ListTasksNeedingPRWatch(ctx context.Context) ([]github.TaskBra return s.buildTaskBranchList(ctx, store) } -// ResolveBranchForSession returns the current branch for a task+session. -// This is used by the poller to detect branch renames on existing PR watches. -func (s *Service) ResolveBranchForSession(ctx context.Context, taskID, sessionID string) string { - return s.resolvePRWatchBranch(ctx, taskID, sessionID, "") -} - // buildTaskBranchList walks sessions × their repositories and emits one // TaskBranchInfo per (session, repository) that doesn't already have a PR // watch. Multi-repo: previously dedup was keyed by sessionID, which silently @@ -1472,11 +1466,15 @@ func (s *Service) buildTaskBranchList(ctx context.Context, store repoStore) ([]g watchedKeys := s.buildWatchedSessionRepoSet(ctx) var result []github.TaskBranchInfo + seenSessions := make(map[string]struct{}) for _, sess := range sessions { - // Pass sess.Branch as fallback for the primary repo (legacy callers - // stored the worktree branch on SessionBranchInfo); per-repo branches - // come from session.Worktrees inside resolveSessionWatchTargets. - targets := s.resolveSessionWatchTargets(ctx, sess.TaskID, sess.SessionID, sess.Branch) + if _, seen := seenSessions[sess.SessionID]; seen { + continue + } + seenSessions[sess.SessionID] = struct{}{} + // Inventory rows can carry any repository's branch. Resolve each session + // once from its per-repository targets, as watch refresh does. + targets := s.resolveSessionWatchTargets(ctx, sess.TaskID, sess.SessionID, "") for _, t := range targets { if watchedKeys[watchedSessionRepoKey(sess.SessionID, t.RepositoryID, t.Branch)] { continue diff --git a/apps/backend/internal/orchestrator/event_handlers_github_watch_reconciliation.go b/apps/backend/internal/orchestrator/event_handlers_github_watch_reconciliation.go new file mode 100644 index 0000000000..8ed405837c --- /dev/null +++ b/apps/backend/internal/orchestrator/event_handlers_github_watch_reconciliation.go @@ -0,0 +1,40 @@ +package orchestrator + +import ( + "context" + + "github.com/kandev/kandev/internal/github" +) + +// ResolveBranchForWatch keeps checkout identity scoped to the source session. +// The watch's attributed task can be a group owner with different branches. +func (s *Service) ResolveBranchForWatch(ctx context.Context, watch *github.PRWatch) string { + if watch == nil || watch.SessionID == "" || watch.RepositoryID == "" { + return "" + } + session, err := s.repo.GetTaskSession(ctx, watch.SessionID) + if err != nil || session == nil { + return "" + } + targets := s.resolveSessionWatchTargets(ctx, session.TaskID, session.ID, "") + return matchingWatchBranch(targets, watch.RepositoryID, watch.Branch) +} + +func matchingWatchBranch(targets []sessionWatchTarget, repositoryID, current string) string { + branches := make(map[string]struct{}) + for _, target := range targets { + if target.RepositoryID != repositoryID || target.Branch == "" { + continue + } + if target.Branch == current { + return current + } + branches[target.Branch] = struct{}{} + } + if len(branches) == 1 { + for branch := range branches { + return branch + } + } + return "" +} diff --git a/apps/backend/internal/orchestrator/event_handlers_github_watch_reconciliation_test.go b/apps/backend/internal/orchestrator/event_handlers_github_watch_reconciliation_test.go new file mode 100644 index 0000000000..28a2cffebe --- /dev/null +++ b/apps/backend/internal/orchestrator/event_handlers_github_watch_reconciliation_test.go @@ -0,0 +1,151 @@ +package orchestrator + +import ( + "context" + "fmt" + "testing" + + "github.com/kandev/kandev/internal/github" + "github.com/kandev/kandev/internal/task/models" + sqliterepo "github.com/kandev/kandev/internal/task/repository/sqlite" +) + +// @covers AC-INTEGRATIONS-GITHUB-PR-DISCOVERY-001.4 +func TestResolvePRWatchBranchForWatch(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + seedGitHubPushAssocFixture(t, repo, "primary") + if err := repo.CreateRepository(ctx, &models.Repository{ID: "repo2", WorkspaceID: "ws1", Name: "secondary", Provider: "github", ProviderOwner: "myorg", ProviderName: "secondary"}); err != nil { + t.Fatal(err) + } + if err := repo.CreateTaskRepository(ctx, &models.TaskRepository{ID: "tr2", TaskID: "t1", RepositoryID: "repo2", CheckoutBranch: "secondary", Position: 1}); err != nil { + t.Fatal(err) + } + svc := createTestService(repo, newMockStepGetter(), newMockTaskRepo()) + watch := &github.PRWatch{TaskID: "t1", SessionID: "s1", RepositoryID: "repo2", Branch: "secondary"} + if got := svc.ResolveBranchForWatch(ctx, nil); got != "" { + t.Fatalf("nil watch resolved to %q", got) + } + if got := svc.ResolveBranchForWatch(ctx, &github.PRWatch{RepositoryID: "repo1"}); got != "" { + t.Fatalf("missing session resolved to %q", got) + } + got := svc.ResolveBranchForWatch(ctx, watch) + if got != watch.Branch { + t.Fatalf("secondary watch resolved to %q, want %q", got, watch.Branch) + } +} + +// @covers AC-INTEGRATIONS-GITHUB-PR-DISCOVERY-001.4 +func TestResolvePRWatchBranchForWatch_SourceIdentity(t *testing.T) { + for _, tt := range []struct { + name, session, repository, branch, want string + branches []string + cancelled bool + }{ + {name: "configured checkout wins", session: "s1", repository: "repo1", branch: "old", want: "primary", branches: []string{"synthetic-review"}}, + {name: "exact sibling survives", session: "s1", repository: "repo1", branch: "secondary", want: "secondary", branches: []string{"primary", "secondary"}}, + {name: "ambiguous rename", session: "s1", repository: "repo1", branch: "old", branches: []string{"first", "second"}}, + {name: "empty sibling is not candidate", session: "s1", repository: "repo1", branch: "old", want: "valid", branches: []string{"", "valid"}}, + {name: "duplicate branches are one candidate", session: "s1", repository: "repo1", branch: "old", want: "valid", branches: []string{"valid", "valid"}}, + {name: "missing repository", session: "s1", repository: "missing", branch: "old", branches: []string{"primary"}}, + {name: "missing session", session: "missing", repository: "repo1", branch: "old"}, + {name: "missing repository identity", session: "s1", branch: "old"}, + {name: "cancelled read", session: "s1", repository: "repo1", branch: "old", cancelled: true}, + } { + t.Run(tt.name, func(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + seedGitHubPushAssocFixture(t, repo, "primary") + owner, err := repo.GetTask(ctx, "t1") + if err != nil { + t.Fatal(err) + } + owner.ID = "attributed-group-owner" + if err := repo.CreateTask(ctx, owner); err != nil { + t.Fatal(err) + } + if err := repo.CreateTaskRepository(ctx, &models.TaskRepository{ID: "owner-tr", TaskID: "attributed-group-owner", RepositoryID: "repo1", CheckoutBranch: "owner-primary"}); err != nil { + t.Fatal(err) + } + repos := make([]*models.TaskEnvironmentRepo, 0, len(tt.branches)) + for i, branch := range tt.branches { + repos = append(repos, &models.TaskEnvironmentRepo{ID: fmt.Sprintf("wt%d", i), RepositoryID: "repo1", WorktreeID: fmt.Sprintf("tree%d", i), BranchSlug: fmt.Sprintf("branch%d", i), WorktreeBranch: branch, Status: "active", Position: i}) + } + if len(repos) > 0 { + attachWatchTestEnvironment(t, repo, repos) + } + svc := createTestService(repo, newMockStepGetter(), newMockTaskRepo()) + if tt.cancelled { + cancelled, cancel := context.WithCancel(ctx) + cancel() + ctx = cancelled + } + watch := &github.PRWatch{TaskID: "attributed-group-owner", SessionID: tt.session, RepositoryID: tt.repository, Branch: tt.branch} + got := svc.ResolveBranchForWatch(ctx, watch) + if got != tt.want { + t.Fatalf("branch = %q, want %q", got, tt.want) + } + }) + } +} + +func attachWatchTestEnvironment(t *testing.T, repo *sqliterepo.Repository, repos []*models.TaskEnvironmentRepo) { + t.Helper() + ctx := context.Background() + if err := repo.CreateTaskEnvironment(ctx, &models.TaskEnvironment{ID: "env1", TaskID: "t1", ExecutorType: "worktree", WorkspacePath: t.TempDir(), Status: models.TaskEnvironmentStatusReady, Repos: repos}); err != nil { + t.Fatal(err) + } + session, err := repo.GetTaskSession(ctx, "s1") + if err != nil { + t.Fatal(err) + } + session.TaskEnvironmentID = "env1" + if err := repo.UpdateTaskSession(ctx, session); err != nil { + t.Fatal(err) + } +} + +// @covers AC-INTEGRATIONS-GITHUB-PR-DISCOVERY-001.4 +func TestResolvePRWatchBranchForWatch_CreationUsesSameTargets(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + seedGitHubPushAssocFixture(t, repo, "") + if err := repo.CreateRepository(ctx, &models.Repository{ID: "repo2", WorkspaceID: "ws1", Name: "secondary", Provider: "github", ProviderOwner: "myorg", ProviderName: "secondary"}); err != nil { + t.Fatal(err) + } + if err := repo.CreateTaskRepository(ctx, &models.TaskRepository{ID: "tr2", TaskID: "t1", RepositoryID: "repo2", Position: 1}); err != nil { + t.Fatal(err) + } + attachWatchTestEnvironment(t, repo, []*models.TaskEnvironmentRepo{ + {ID: "wt1", RepositoryID: "repo1", WorktreeID: "tree1", WorktreeBranch: "primary", Status: "active"}, + {ID: "wt2", RepositoryID: "repo2", WorktreeID: "tree2", WorktreeBranch: "secondary", Status: "active", Position: 1}, + }) + svc := createTestService(repo, newMockStepGetter(), newMockTaskRepo()) + svc.SetGitHubService(&mockGitHubService{}) + targets, err := svc.ListTasksNeedingPRWatch(ctx) + if err != nil { + t.Fatal(err) + } + seen := make(map[string]bool) + for _, target := range targets { + want := map[string]string{"repo1": "primary", "repo2": "secondary"}[target.RepositoryID] + if target.Branch != want { + t.Fatalf("creation target = %+v, want branch %q", target, want) + } + key := target.SessionID + "/" + target.RepositoryID + "/" + target.Branch + if seen[key] { + t.Fatalf("duplicate creation target %s", key) + } + seen[key] = true + watch := &github.PRWatch{SessionID: target.SessionID, TaskID: target.TaskID, RepositoryID: target.RepositoryID, Branch: target.Branch} + if got := svc.ResolveBranchForWatch(ctx, watch); got != want { + t.Fatalf("refresh = %q, want %q", got, want) + } + } + if len(targets) != 2 { + t.Fatalf("targets = %d, want 2", len(targets)) + } +} diff --git a/docs/plans/github-pr-watch-reconciliation/plan.md b/docs/plans/github-pr-watch-reconciliation/plan.md new file mode 100644 index 0000000000..1ef862585a --- /dev/null +++ b/docs/plans/github-pr-watch-reconciliation/plan.md @@ -0,0 +1,114 @@ +--- +created: 2026-09-13 +status: done +requirements: + - REQ-INTEGRATIONS-GITHUB-PR-DISCOVERY-001 +system_design: + - ../../specs/integrations/system-design/github-pr-discovery-health.md +legacy_specs: [] +--- + +# Implementation Plan: Stable GitHub PR watch reconciliation + +## Overview + +Repair repeated branch rewrites and watch recreation in one sequential backend +work order. Extend the existing discovery requirement with AC .4 and its design; +ACs .1-.3 continue to govern association, identity, and unlink preservation. +The completed discovery-health package remains historical evidence and is not +reopened: this package changes target reconciliation only. + +## Scope + +### In scope + +- Watch-aware branch resolution shared with creation's target source. +- Multi-repository, multi-branch, and redirected-owner regression coverage. +- Preserve genuine renames and existing association safety. + +### Out of scope + +- Main-instance mutation, restarts, deployment, database repair, or delegation. +- GitLab, provider quotas, new polling loops, UI changes, or new public APIs. +- Attribution of the user's original missing PR, which was not identified. + +## Evidence and root cause + +Main instance build `509a1bb9b` repeatedly logged branch changes followed by +recreation on the previous branch every minute. For example, on September 13 +at 17:02:54 Lisbon time, one session's watches changed to the primary +branch and recreated their former branch for two repositories. Retained September 11-13 logs contained over 110,000 occurrences +of each operation. GitHub status was healthy; rate limits were not implicated. + +`refreshStaleBranches` uses session-only resolution through the attributed +task's primary repository. Creation uses source-session repository/worktree +targets and redirects association ownership separately. Refresh therefore +collapses legitimate targets; reconciliation sees them missing and recreates +them. Collision deletion can repeat the cycle without a SQL error. + +## Technical approach + +Follow the watch target reconciliation section of the discovery system design. +Update `TaskBranchProvider`, its orchestrator implementation, and test doubles +together. Pass repository and existing branch identity to the resolver; derive +source task identity from the session. Preserve exact matches before considering +a unique same-repository replacement. Keep existing atomic update guards. +Extract focused resolver helpers/tests if needed to respect file limits. +The creation/refresh regression also reproduced inventory expansion assigning +one repository's branch to another. Deduplicate session inventory rows and +resolve all targets without that unqualified branch fallback. + +## Tests + +- AC .4: `TestResolvePRWatchBranchForWatch` in new orchestrator + `event_handlers_github_watch_reconciliation_test.go`, using real target + resolution with multiple repositories, multiple branches, group ownership, + missing data, ambiguous replacements, and genuine single-branch renames. +- AC .4: `TestReconcileWatches_PreservesBranchTargetsAcrossCycles` in new GitHub + `poller_branch_reconciliation_test.go`. With real SQLite watch storage, run + at least three reconciliation cycles and assert unchanged IDs, branches, + row counts, and no repeated create/update calls for unchanged targets. +- ACs .1-.3: retain numbered-watch, collision, detached-association, and + multi-branch discovery tests in the existing GitHub suite. + +## End-to-end evidence + +Backend integration coverage carries a secondary-branch watch through +reconciliation, canned provider discovery, persisted task-PR association, and +the task-PR event. Include a numbered sibling to prove mixed-state preservation. +Use existing GraphQL/poller fixtures; do not call GitHub or the main instance. +No browser test is needed because no rendering contract changes. + +## Work orders + +- [x] [Task 01: Preserve watch branch identity](task-01-preserve-watch-identity.md) + +## Verification results + +Design validation passed on 2026-09-13: + +- `python3 scripts/list-docs.py validate`: 267 decisions and 868 specifications validated. +- `python3 scripts/lint-spec-files.py --all`: passed. +- `git diff --check`: passed. +- Scoped `git status --short`: both specification edits and the new package present. + +Focused regressions passed after reproducing both defects. The full GitHub +suite passed in 284.480s. The full orchestrator suite passed in 202.462s with +both temporary-directory variables pointing at owned tmpfs storage, following +the documented disk-I/O timeout. The targeted race check passed in 1.293s. Exact commands and results are +recorded in the completed work order. Public documentation remains +unchanged because this package records repair intent without new user operations. + +## Risks + +- Association owner and source session task differ in shared workspaces. +- Picking the first branch would hide ambiguity and repeat the defect. +- Synthetic review worktrees must retain configured checkout precedence. +- Existing corrupted watches may need one normal convergence cycle; no bulk + repair is included. Explicit unlink and numbered-watch state must survive. + +## Related contracts + +- [Discovery requirements](../../specs/integrations/requirements/github-pr-discovery-health.md) +- [Multi-branch decision](../../decisions/0013-multi-branch-tasks.md) +- [Earlier discovery package](../github-pr-discovery-health/plan.md) diff --git a/docs/plans/github-pr-watch-reconciliation/task-01-preserve-watch-identity.md b/docs/plans/github-pr-watch-reconciliation/task-01-preserve-watch-identity.md new file mode 100644 index 0000000000..3ed51fea1f --- /dev/null +++ b/docs/plans/github-pr-watch-reconciliation/task-01-preserve-watch-identity.md @@ -0,0 +1,130 @@ +--- +id: "01-preserve-watch-identity" +title: "Preserve watch branch identity" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +requirements: + - REQ-INTEGRATIONS-GITHUB-PR-DISCOVERY-001 +acceptance_criteria: + - AC-INTEGRATIONS-GITHUB-PR-DISCOVERY-001.1 + - AC-INTEGRATIONS-GITHUB-PR-DISCOVERY-001.2 + - AC-INTEGRATIONS-GITHUB-PR-DISCOVERY-001.3 + - AC-INTEGRATIONS-GITHUB-PR-DISCOVERY-001.4 +system_design: + - ../../specs/integrations/system-design/github-pr-discovery-health.md +--- + +# Task 01: Preserve watch branch identity + +## Summary + +Make refresh resolve the same per-session repository/branch targets as creation. +Prove repeated reconciliation preserves sibling targets and subsequent discovery. + +## In scope + +- Update the provider interface, orchestrator resolver, poller call, and mocks. +- Add the resolver and repeated-cycle regressions named in the plan. +- Prove secondary-branch discovery persists and publishes the owning task PR. + +## Out of scope + +Live-instance changes, new schema, frontend changes, GitLab, rate-limit policy, +bulk cleanup, and changes to workspace-group association ownership. + +## Acceptance + +1. Three unchanged reconciliation cycles preserve watch IDs and repository/branch + targets without recreation, including redirected owners and multiple branches + on the same repository. At least one searching and one numbered watch coexist. +2. Exact matches survive; unique same-repository renames update; missing/error + and ambiguous target resolution leaves state intact. No primary-repo fallback. +3. An eligible PR on a secondary branch is linked after reconciliation with + correct repository identity and task event; numbered links and unlink + tombstones remain protected by existing writers and regression coverage. + +## Verification + +Start with a failing regression using the existing production contract before +changing it. Record the expected wrong-branch or replaced-watch failure, then +implement and run these commands from the repository root: + +```bash +(cd apps/backend && go test ./internal/github ./internal/orchestrator -run 'Test(ReconcileWatches_PreservesBranchTargetsAcrossCycles|ResolvePRWatchBranchForWatch)' -count=1) +(cd apps/backend && go test ./internal/github -count=1) +(cd apps/backend && go test ./internal/orchestrator -count=1 -timeout=30m) +(cd apps/backend && go test -race ./internal/github -run 'Test(ReconcileWatches|RefreshStaleBranches)' -count=1) +python3 scripts/list-docs.py validate +python3 scripts/lint-spec-files.py --all +git diff --check +``` + +On the development host, the combined package run passed GitHub but exhausted +Go's default ten-minute timeout while the orchestrator suite was closing a +SQLite fixture. The final retry uses an owned temporary directory on the existing +`/dev/shm` tmpfs via both `TMPDIR` and `GOTMPDIR`. Go 1.26's `t.TempDir()` +prefers `GOTMPDIR`; an intermediate retry that left it at `/tmp` was stopped +before correcting that setting. The owned directory is removed after the run. No mount, live database, or instance +setting changes are involved. Use normal temporary storage on hosts without +this disk-I/O constraint. + +## Files likely touched + +- `apps/backend/internal/github/poller.go` +- `apps/backend/internal/github/poller_test.go` +- `apps/backend/internal/github/poller_branch_reconciliation_test.go` (new) +- `apps/backend/internal/orchestrator/event_handlers_github.go` +- `apps/backend/internal/orchestrator/event_handlers_github_watch_reconciliation.go` (new) +- `apps/backend/internal/orchestrator/event_handlers_github_watch_reconciliation_test.go` (new) +- Other compile-time `TaskBranchProvider` test doubles found by symbol search. + +## Dependencies + +None. Implementation was explicitly authorized after the design-package handoff. + +## Risks + +See the plan. Reuse source-session target resolution and existing storage guards; +do not infer a rename from an arbitrary sibling or hide read errors as success. + +## Parallelism + +`sequential` + +## Inputs + +- [Requirements](../../specs/integrations/requirements/github-pr-discovery-health.md) +- [Design](../../specs/integrations/system-design/github-pr-discovery-health.md), watch target reconciliation. +- Existing `TestRefreshStaleBranches_*`, `TestListTasksNeedingPRWatch`, and + `TestEnsureSessionPRWatchRedirectsToWorkspaceGroupOwner` fixtures. +- Plan evidence and backend `AGENTS.md`. + +## Results + +Completed on 2026-09-13. + +- Red: the original resolver selected `primary` for a secondary-repository watch; + repeated reconciliation grew three watches to four. Creation/refresh coverage + also reproduced a primary repository receiving the secondary branch. +- Focused `go test ./internal/github ./internal/orchestrator -run + 'Test(ReconcileWatches_PreservesBranchTargetsAcrossCycles|ResolvePRWatchBranchForWatch)' + -count=1`: passed both packages (GitHub 0.031s; orchestrator 79.458s). +- Full GitHub suite: passed (284.480s), as the GitHub component of the original + combined package command. +- Full orchestrator suite with `-count=1 -timeout=30m`: passed (202.462s) using + owned tmpfs scratch after the infrastructure timeout described above. +- `go test -race ./internal/github -run + 'Test(ReconcileWatches|RefreshStaleBranches)' -count=1`: passed (1.293s), + using owned tmpfs scratch. +- `python3 scripts/list-docs.py validate`: passed, 267 decisions and 868 specifications. +- `python3 scripts/lint-spec-files.py --all`: passed. +- `git diff --check`: passed. + +The source session and per-repository targets now govern both refresh and +creation. Existing numbered-watch and unlink guards remain unchanged. New +backend integration coverage proves secondary-PR persistence and task events. +Public docs need no change because the repair restores existing behavior. +No application restart, live database repair, deployment, or delegation occurred. +Owned tmpfs scratch directories were removed after their test runs. diff --git a/docs/specs/integrations/requirements/github-pr-discovery-health.md b/docs/specs/integrations/requirements/github-pr-discovery-health.md index 4f5d94066c..8aa3ad78dc 100644 --- a/docs/specs/integrations/requirements/github-pr-discovery-health.md +++ b/docs/specs/integrations/requirements/github-pr-discovery-health.md @@ -30,6 +30,11 @@ and provider status. Task and UI systems consume its results. - **AC-INTEGRATIONS-GITHUB-PR-DISCOVERY-001.3:** A rejected provider query shall not be treated as a successful search with no PR. Existing associations and explicit unlink decisions shall remain intact. +- **AC-INTEGRATIONS-GITHUB-PR-DISCOVERY-001.4:** For every unchanged attached + repository and branch, repeated watch reconciliation shall preserve its + discovery target independently of sibling branches and task-group ownership. + A confirmed rename may update that target; missing or ambiguous branch + information shall not redirect it to a sibling target. ### REQ-INTEGRATIONS-GITHUB-PR-DISCOVERY-HEALTH-001: Explain discovery failures @@ -74,3 +79,4 @@ runtime status, not a permanent incident archive. - [Frontend synchronization coordination](github-task-pr-sync-coordination.md) - [System design](../system-design/github-pr-discovery-health.md) - [Implementation package](../../../plans/github-pr-discovery-health/plan.md) +- [Watch reconciliation repair](../../../plans/github-pr-watch-reconciliation/plan.md) diff --git a/docs/specs/integrations/system-design/github-pr-discovery-health.md b/docs/specs/integrations/system-design/github-pr-discovery-health.md index c824eed9f7..f0efde4824 100644 --- a/docs/specs/integrations/system-design/github-pr-discovery-health.md +++ b/docs/specs/integrations/system-design/github-pr-discovery-health.md @@ -38,6 +38,41 @@ field contract, in addition to exercising response conversion and persistence. Record the upstream schema reference in the fixture. A canned response that accepts any query cannot detect this defect. +## Watch target reconciliation + +Discovery AC .4 uses the same session repository/worktree target resolution +for watch creation and refresh. In `internal/github/poller.go`, +`TaskBranchProvider.ResolveBranchForWatch` carries session, repository, and +current branch identity. The orchestrator implementation lives in +`event_handlers_github_watch_reconciliation.go`; the poller does not choose a +task's primary branch for every watch. + +The session inventory can contain one row per distinct worktree branch without +repository identity. Expand each session once and do not pass an inventory +row's branch as the primary repository fallback; load branches from the +session's per-repository worktrees instead. + +Resolve targets from the source session's task through +`resolveSessionWatchTargets`, matching creation's fallback and worktree rules. +`watch.TaskID` may name a workspace-group owner and remains the association +destination, not a substitute source of checkout identity. Reuse target +resolution without invoking effective-owner redirection for branch selection. + +Keep an exact repository/branch match. Only select a replacement when there +is exactly one distinct branch target for that repository and the current +branch is absent. Missing session or repository identity, lookup failure, no +matching targets, or multiple unmatched candidates leaves the watch unchanged. +Never use a sibling repository as a fallback. Preserve single-repository +checkout-branch precedence and real unambiguous rename handling. + +Keep the existing `pr_number = 0` atomic writer and collision handling. +Numbered watches, associations, detached tombstones, and attribution remain +unchanged. No migration or live database cleanup is required. Reconciliation +must reach a stable set of watch IDs and branches across repeated cycles; +tests include mixed numbered/searching watches and multi-branch sessions. + +Delivery: [watch reconciliation repair](../../../plans/github-pr-watch-reconciliation/plan.md). + ## Failure state and admission Add a service-owned runtime discovery-health component. Key each target by