From 60edf466cf935870a834a9396f5b69466569625f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Fri, 25 Sep 2026 16:47:28 +0100 Subject: [PATCH] fix: preserve GitHub review PR identity for auto-start (#3928) --- apps/backend/internal/github/mock_client.go | 40 +++- .../internal/github/mock_client_test.go | 1 + .../internal/github/service_reviews.go | 28 ++- .../internal/github/service_reviews_test.go | 212 ++++++++++++++++++ .../event_handlers_github_review_test.go | 160 ++++++++++++- docs/plans/review-watch-pr-identity/plan.md | 103 +++++++++ .../task-01-preserve-review-pr-identity.md | 119 ++++++++++ .../system-design/github-fork-review-start.md | 19 +- 8 files changed, 668 insertions(+), 14 deletions(-) create mode 100644 apps/backend/internal/github/service_reviews_test.go create mode 100644 docs/plans/review-watch-pr-identity/plan.md create mode 100644 docs/plans/review-watch-pr-identity/task-01-preserve-review-pr-identity.md diff --git a/apps/backend/internal/github/mock_client.go b/apps/backend/internal/github/mock_client.go index 0eba025f37..5e1598942c 100644 --- a/apps/backend/internal/github/mock_client.go +++ b/apps/backend/internal/github/mock_client.go @@ -105,6 +105,17 @@ type repoFileEntry struct { Content []byte } +type mockPRDetailResult struct { + pr *PR + err error +} + +type mockReviewWatchResponses struct { + details map[prKey]mockPRDetailResult + searchResults []*PR + searchResultsSet bool +} + // MockClient implements Client with in-memory configurable data for E2E testing. // All data is protected by a sync.RWMutex for thread safety. type MockClient struct { @@ -161,6 +172,7 @@ type MockClient struct { // singleflight coalescing. probeEntered chan string probeRelease chan struct{} + reviewWatch mockReviewWatchResponses } // mockGist captures a gist that was created via the mock client so tests @@ -179,6 +191,7 @@ func NewMockClient() *MockClient { user: mockDefaultUser, authenticated: true, prs: make(map[prKey]*PR), + reviewWatch: mockReviewWatchResponses{details: make(map[prKey]mockPRDetailResult)}, issues: make(map[issueKey]*Issue), prsByBranch: make(map[branchKey]*PR), repos: make(map[string][]GitHubRepo), @@ -218,7 +231,11 @@ func (m *MockClient) GetAuthenticatedUser(context.Context) (string, error) { func (m *MockClient) GetPR(_ context.Context, owner, repo string, number int) (*PR, error) { m.mu.RLock() defer m.mu.RUnlock() - pr, ok := m.prs[prKey{owner, repo, number}] + key := prKey{owner, repo, number} + if result, ok := m.reviewWatch.details[key]; ok { + return result.pr, result.err + } + pr, ok := m.prs[key] if !ok { return nil, fmt.Errorf("mock: PR %s/%s#%d not found", owner, repo, number) } @@ -306,6 +323,9 @@ func (m *MockClient) ListAuthoredPRs(_ context.Context, owner, repo string) ([]* func (m *MockClient) ListReviewRequestedPRs(context.Context, string, string, string) ([]*PR, error) { m.mu.RLock() defer m.mu.RUnlock() + if m.reviewWatch.searchResultsSet { + return append([]*PR(nil), m.reviewWatch.searchResults...), nil + } var result []*PR for _, pr := range m.prs { if len(pr.RequestedReviewers) > 0 { @@ -949,6 +969,23 @@ func (m *MockClient) AddPR(pr *PR) { } } +// SetReviewRequestedPRs overrides search-shaped review-requested results. +// Use SetPRDetail to provide the separate GetPR response for each result. +func (m *MockClient) SetReviewRequestedPRs(prs []*PR) { + m.mu.Lock() + defer m.mu.Unlock() + m.reviewWatch.searchResultsSet = true + m.reviewWatch.searchResults = append([]*PR(nil), prs...) +} + +// SetPRDetail overrides GetPR for one PR, allowing search and detail responses +// to differ in tests that exercise provider enrichment. +func (m *MockClient) SetPRDetail(owner, repo string, number int, pr *PR, err error) { + m.mu.Lock() + defer m.mu.Unlock() + m.reviewWatch.details[prKey{owner, repo, number}] = mockPRDetailResult{pr: pr, err: err} +} + func (m *MockClient) ensurePRHeadSHA(owner, repo string, number int, fallback string) (string, bool) { m.mu.Lock() defer m.mu.Unlock() @@ -1185,6 +1222,7 @@ func (m *MockClient) Reset() { m.authError = "" m.reposUnavailable = false m.prs = make(map[prKey]*PR) + m.reviewWatch = mockReviewWatchResponses{details: make(map[prKey]mockPRDetailResult)} m.issues = make(map[issueKey]*Issue) m.prsByBranch = make(map[branchKey]*PR) m.orgs = nil diff --git a/apps/backend/internal/github/mock_client_test.go b/apps/backend/internal/github/mock_client_test.go index 279eddfd96..f040949f40 100644 --- a/apps/backend/internal/github/mock_client_test.go +++ b/apps/backend/internal/github/mock_client_test.go @@ -342,6 +342,7 @@ func TestMockClient_Reset(t *testing.T) { m.SetUser("custom") m.AddPR(&PR{Number: 1, RepoOwner: "o", RepoName: "r", HeadBranch: "b"}) + m.SetPRDetail("o", "r", 1, &PR{Number: 1, Title: "detail override"}, nil) m.AddOrgs([]GitHubOrg{{Login: "org1"}}) m.Reset() diff --git a/apps/backend/internal/github/service_reviews.go b/apps/backend/internal/github/service_reviews.go index 45999d9056..6e0b616dde 100644 --- a/apps/backend/internal/github/service_reviews.go +++ b/apps/backend/internal/github/service_reviews.go @@ -340,8 +340,8 @@ func (s *Service) CheckReviewWatch(ctx context.Context, watch *ReviewWatch) ([]* } } - // Enrich new PRs with full details (branch info) from the PR API, - // since the search API does not return head/base branch. + // Enrich new PRs with provider details because search results can omit + // branch information and head repository identity. s.enrichPRDetails(ctx, resolved.Client, newPRs) s.logger.Debug("review watch check complete", @@ -476,30 +476,46 @@ func repoFilterToQualifier(repo RepoFilter) string { return fmt.Sprintf("repo:%s/%s", repo.Owner, repo.Name) } -// enrichPRDetails fetches full PR details for PRs missing branch info (from the search API). +// enrichPRDetails fetches full PR details for search results missing branch +// information or head repository identity. func (s *Service) enrichPRDetails(ctx context.Context, client Client, prs []*PR) { for _, pr := range prs { - if pr.HeadBranch != "" && pr.BaseBranch != "" { + if pr.HeadBranch != "" && pr.BaseBranch != "" && pr.HeadRepoOwner != "" && pr.HeadRepoName != "" { continue } - s.logger.Debug("enriching PR with full details (missing branch info)", + s.logger.Debug("enriching PR with full details (missing required review metadata)", zap.String("repo", pr.RepoOwner+"/"+pr.RepoName), zap.Int("pr_number", pr.Number)) full, err := client.GetPR(ctx, pr.RepoOwner, pr.RepoName, pr.Number) if err != nil { - s.logger.Warn("failed to fetch full PR details, branch info will be empty", + s.logger.Warn("failed to fetch full PR details, review metadata will remain incomplete", zap.String("repo", pr.RepoOwner+"/"+pr.RepoName), zap.Int("pr_number", pr.Number), zap.Error(err)) continue } + if full == nil { + continue + } pr.HeadBranch = full.HeadBranch pr.HeadSHA = full.HeadSHA pr.BaseBranch = full.BaseBranch + pr.BaseSHA = full.BaseSHA pr.Additions = full.Additions pr.Deletions = full.Deletions pr.Mergeable = full.Mergeable + pr.MergeableState = full.MergeableState + pr.HeadRepoID = full.HeadRepoID + pr.HeadRepoNodeID = full.HeadRepoNodeID + pr.HeadRepoOwner = full.HeadRepoOwner + pr.HeadRepoName = full.HeadRepoName + pr.HeadRepoCloneURL = full.HeadRepoCloneURL + pr.BaseRepoID = full.BaseRepoID + pr.BaseRepoOwner = full.BaseRepoOwner + pr.BaseRepoName = full.BaseRepoName + pr.BaseDefaultBranch = full.BaseDefaultBranch + pr.MaintainerCanModify = full.MaintainerCanModify } } diff --git a/apps/backend/internal/github/service_reviews_test.go b/apps/backend/internal/github/service_reviews_test.go new file mode 100644 index 0000000000..43d4238443 --- /dev/null +++ b/apps/backend/internal/github/service_reviews_test.go @@ -0,0 +1,212 @@ +package github + +import ( + "context" + "errors" + "testing" + + "github.com/kandev/kandev/internal/events" + "github.com/kandev/kandev/internal/events/bus" +) + +type reviewWatchSearchClient struct { + *MockClient + searchPR *PR + detailPR *PR + detailErr error + getPRCalls int + getPROwner string + getPRRepo string + getPRNum int +} + +func (c *reviewWatchSearchClient) ListReviewRequestedPRs(context.Context, string, string, string) ([]*PR, error) { + return []*PR{c.searchPR}, nil +} + +func (c *reviewWatchSearchClient) GetPR(_ context.Context, owner, repo string, number int) (*PR, error) { + c.getPRCalls++ + c.getPROwner = owner + c.getPRRepo = repo + c.getPRNum = number + return c.detailPR, c.detailErr +} + +type reviewPRDetails struct { + headBranch string + headSHA string + baseBranch string + baseSHA string + additions int + deletions int + mergeable bool + mergeableState string + headRepoID int64 + headRepoNodeID string + headRepoOwner string + headRepoName string + headRepoCloneURL string + baseRepoID int64 + baseRepoOwner string + baseRepoName string + baseDefaultBranch string + maintainerCanModify bool +} + +func reviewPRDetailsFrom(pr *PR) reviewPRDetails { + return reviewPRDetails{ + headBranch: pr.HeadBranch, + headSHA: pr.HeadSHA, + baseBranch: pr.BaseBranch, + baseSHA: pr.BaseSHA, + additions: pr.Additions, + deletions: pr.Deletions, + mergeable: pr.Mergeable, + mergeableState: pr.MergeableState, + headRepoID: pr.HeadRepoID, + headRepoNodeID: pr.HeadRepoNodeID, + headRepoOwner: pr.HeadRepoOwner, + headRepoName: pr.HeadRepoName, + headRepoCloneURL: pr.HeadRepoCloneURL, + baseRepoID: pr.BaseRepoID, + baseRepoOwner: pr.BaseRepoOwner, + baseRepoName: pr.BaseRepoName, + baseDefaultBranch: pr.BaseDefaultBranch, + maintainerCanModify: pr.MaintainerCanModify, + } +} + +func TestCheckReviewWatchEnrichesSearchResultsBeforePublishingEvent(t *testing.T) { + tests := []struct { + name string + detailPR *PR + detailErr error + wantDetails reviewPRDetails + }{ + { + name: "same repository identity", + detailPR: &PR{ + RepoOwner: "acme", RepoName: "widget", + HeadBranch: "feature/detail", HeadSHA: "head-sha-detail", + BaseBranch: "main-detail", BaseSHA: "base-sha-detail", + Additions: 7, Deletions: 3, Mergeable: true, MergeableState: "clean", + HeadRepoID: 101, HeadRepoNodeID: "H_repo", HeadRepoOwner: "acme", + HeadRepoName: "widget", HeadRepoCloneURL: "https://github.com/acme/widget.git", + BaseRepoID: 202, BaseRepoOwner: "acme", BaseRepoName: "widget", + BaseDefaultBranch: "main", MaintainerCanModify: true, + }, + wantDetails: reviewPRDetails{ + headBranch: "feature/detail", headSHA: "head-sha-detail", + baseBranch: "main-detail", baseSHA: "base-sha-detail", + additions: 7, deletions: 3, mergeable: true, mergeableState: "clean", + headRepoID: 101, headRepoNodeID: "H_repo", headRepoOwner: "acme", + headRepoName: "widget", headRepoCloneURL: "https://github.com/acme/widget.git", + baseRepoID: 202, baseRepoOwner: "acme", baseRepoName: "widget", + baseDefaultBranch: "main", maintainerCanModify: true, + }, + }, + { + name: "fork identity", + detailPR: &PR{ + HeadBranch: "contrib/feature", HeadSHA: "fork-head-sha", + BaseBranch: "main", BaseSHA: "base-sha", + HeadRepoID: 303, HeadRepoNodeID: "H_fork", HeadRepoOwner: "contributor", + HeadRepoName: "widget-fork", HeadRepoCloneURL: "https://github.com/contributor/widget-fork.git", + BaseRepoID: 202, BaseRepoOwner: "acme", BaseRepoName: "widget", + BaseDefaultBranch: "main", MaintainerCanModify: false, + Mergeable: false, MergeableState: "blocked", Additions: 4, Deletions: 2, + }, + wantDetails: reviewPRDetails{ + headBranch: "contrib/feature", headSHA: "fork-head-sha", + baseBranch: "main", baseSHA: "base-sha", additions: 4, deletions: 2, + mergeable: false, mergeableState: "blocked", + headRepoID: 303, headRepoNodeID: "H_fork", headRepoOwner: "contributor", + headRepoName: "widget-fork", headRepoCloneURL: "https://github.com/contributor/widget-fork.git", + baseRepoID: 202, baseRepoOwner: "acme", baseRepoName: "widget", + baseDefaultBranch: "main", maintainerCanModify: false, + }, + }, + { + name: "detail lookup failure keeps identity unavailable", + detailErr: errors.New("GitHub detail lookup failed"), + wantDetails: reviewPRDetails{ + headBranch: "feature/search", baseBranch: "main", additions: 1, deletions: 2, + }, + }, + { + name: "nil detail response keeps identity unavailable", + wantDetails: reviewPRDetails{ + headBranch: "feature/search", baseBranch: "main", additions: 1, deletions: 2, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := context.Background() + store := newTestStore(t) + searchPR := &PR{ + Number: 42, Title: "Review this change", HTMLURL: "https://github.com/acme/widget/pull/42", + RepoOwner: "acme", RepoName: "widget", + HeadBranch: "feature/search", BaseBranch: "main", Additions: 1, Deletions: 2, + } + client := &reviewWatchSearchClient{ + MockClient: NewMockClient(), searchPR: searchPR, detailPR: tt.detailPR, detailErr: tt.detailErr, + } + eventBus := bus.NewMemoryEventBus(testLogger(t)) + t.Cleanup(eventBus.Close) + var emitted *NewReviewPREvent + subscription, err := eventBus.Subscribe(events.GitHubNewReviewPR, func(_ context.Context, event *bus.Event) error { + payload, ok := event.Data.(*NewReviewPREvent) + if !ok { + return errors.New("event data is not a NewReviewPREvent") + } + emitted = payload + return nil + }) + if err != nil { + t.Fatalf("subscribe to review event: %v", err) + } + t.Cleanup(func() { _ = subscription.Unsubscribe() }) + + service := NewService(client, AuthMethodPAT, nil, store, eventBus, testLogger(t)) + t.Cleanup(service.Stop) + configureTestWorkspaceAuth(t, service, client, "workspace-1") + watch := &ReviewWatch{ + ID: "watch-1", WorkspaceID: "workspace-1", WorkflowID: "workflow-1", + WorkflowStepID: "step-1", Repos: []RepoFilter{{Owner: "acme", Name: "widget"}}, + Enabled: true, + } + if err := store.CreateReviewWatch(ctx, watch); err != nil { + t.Fatalf("create review watch: %v", err) + } + + prs, err := service.TriggerReviewWatch(ctx, watch) + if err != nil { + t.Fatalf("TriggerReviewWatch: %v", err) + } + if len(prs) != 1 { + t.Fatalf("new review PR count = %d, want 1", len(prs)) + } + if client.getPRCalls != 1 || client.getPROwner != "acme" || client.getPRRepo != "widget" || client.getPRNum != 42 { + t.Fatalf("GetPR calls = (%d, %q, %q, %d), want one lookup for acme/widget#42", + client.getPRCalls, client.getPROwner, client.getPRRepo, client.getPRNum) + } + if got := reviewPRDetailsFrom(prs[0]); got != tt.wantDetails { + t.Errorf("enriched PR details = %+v, want %+v", got, tt.wantDetails) + } + if prs[0].RepoOwner != "acme" || prs[0].RepoName != "widget" { + t.Errorf("search target repository = %s/%s, want acme/widget", prs[0].RepoOwner, prs[0].RepoName) + } + if emitted == nil || emitted.PR == nil { + t.Fatal("review watch did not publish a PR event") + } + if got := reviewPRDetailsFrom(emitted.PR); got != tt.wantDetails { + t.Errorf("event PR details = %+v, want %+v", got, tt.wantDetails) + } + if emitted.PR.RepoOwner != "acme" || emitted.PR.RepoName != "widget" { + t.Errorf("event target repository = %s/%s, want acme/widget", emitted.PR.RepoOwner, emitted.PR.RepoName) + } + }) + } +} diff --git a/apps/backend/internal/orchestrator/event_handlers_github_review_test.go b/apps/backend/internal/orchestrator/event_handlers_github_review_test.go index a1e87d893a..2e0ecdc10f 100644 --- a/apps/backend/internal/orchestrator/event_handlers_github_review_test.go +++ b/apps/backend/internal/orchestrator/event_handlers_github_review_test.go @@ -3,12 +3,17 @@ package orchestrator import ( "context" "errors" + "path/filepath" "strings" "sync/atomic" "testing" "time" "unicode/utf8" + "github.com/jmoiron/sqlx" + "github.com/kandev/kandev/internal/db" + "github.com/kandev/kandev/internal/events" + "github.com/kandev/kandev/internal/events/bus" "github.com/kandev/kandev/internal/github" "github.com/kandev/kandev/internal/orchestrator/executor" "github.com/kandev/kandev/internal/orchestrator/watcher" @@ -97,6 +102,9 @@ func TestBuildReviewTaskRequest_ForkPRRequiresManualStart(t *testing.T) { evt.PR.HeadRepoOwner = tt.headOwner evt.PR.HeadRepoName = tt.headRepo + if got := reviewPRHeadMatchesBase(evt.PR); got != tt.wantAutostart { + t.Errorf("reviewPRHeadMatchesBase() = %v, want %v", got, tt.wantAutostart) + } req := buildReviewTaskRequest(evt, nil, "acme/widget") if req.Metadata[taskmodels.MetaKeyAutoStartGuard] != true { t.Fatalf("auto-start guard = %v, want true", req.Metadata[taskmodels.MetaKeyAutoStartGuard]) @@ -105,14 +113,160 @@ func TestBuildReviewTaskRequest_ForkPRRequiresManualStart(t *testing.T) { if gotAutostart != tt.wantAutostart { t.Errorf("automatic launch token present = %v, want %v", gotAutostart, tt.wantAutostart) } - _, requiresManualStart := req.Metadata[taskmodels.MetaKeyForkPRRequiresManualStart] - if requiresManualStart != !tt.wantAutostart { - t.Errorf("manual-start marker present = %v, want %v", requiresManualStart, !tt.wantAutostart) + manualStartMarker, requiresManualStart := req.Metadata[taskmodels.MetaKeyForkPRRequiresManualStart] + if requiresManualStart != !tt.wantAutostart || (requiresManualStart && manualStartMarker != true) { + t.Errorf("manual-start marker = (%v, present=%v), want (true, present=%v)", + manualStartMarker, requiresManualStart, !tt.wantAutostart) } }) } } +func TestReviewWatchSearchResultIdentityControlsAutoStart(t *testing.T) { + tests := []struct { + name string + detailPR *github.PR + detailErr error + wantHeadOwner string + wantHeadRepo string + wantAutostart bool + }{ + { + name: "internal PR", + detailPR: &github.PR{ + HeadRepoOwner: "acme", + HeadRepoName: "widget", + BaseRepoOwner: "acme", + BaseRepoName: "widget", + }, + wantHeadOwner: "acme", + wantHeadRepo: "widget", + wantAutostart: true, + }, + { + name: "fork PR", + detailPR: &github.PR{ + HeadRepoOwner: "contributor", + HeadRepoName: "widget-fork", + BaseRepoOwner: "acme", + BaseRepoName: "widget", + }, + wantHeadOwner: "contributor", + wantHeadRepo: "widget-fork", + }, + { + name: "missing identity after detail lookup failure", + detailErr: errors.New("GitHub detail lookup failed"), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + emitted, prs := triggerReviewWatchForAutoStartTest(t, tt.detailPR, tt.detailErr) + if len(prs) != 1 || emitted == nil || emitted.PR == nil { + t.Fatalf("review watch result = (%d PRs, event=%v), want one PR event", len(prs), emitted != nil) + } + if emitted.PR != prs[0] { + t.Fatal("published event PR is not the PR returned by TriggerReviewWatch") + } + + if emitted.PR.HeadRepoOwner != tt.wantHeadOwner || emitted.PR.HeadRepoName != tt.wantHeadRepo { + t.Errorf("emitted PR head identity = %s/%s, want %s/%s", + emitted.PR.HeadRepoOwner, emitted.PR.HeadRepoName, tt.wantHeadOwner, tt.wantHeadRepo) + } + if emitted.PR.RepoOwner != "acme" || emitted.PR.RepoName != "widget" { + t.Errorf("emitted PR target identity = %s/%s, want acme/widget", emitted.PR.RepoOwner, emitted.PR.RepoName) + } + if got := reviewPRHeadMatchesBase(emitted.PR); got != tt.wantAutostart { + t.Errorf("reviewPRHeadMatchesBase(emitted PR) = %v, want %v", got, tt.wantAutostart) + } + request := buildReviewTaskRequest(emitted, nil, "acme/widget") + autoStartClaim, hasAutoStartClaim := request.Metadata[taskmodels.MetaKeyAutoStartClaimed] + if hasAutoStartClaim != tt.wantAutostart || (hasAutoStartClaim && autoStartClaim != true) { + t.Errorf("auto_start_claimed = (%v, present=%v), want true present=%v", + autoStartClaim, hasAutoStartClaim, tt.wantAutostart) + } + manualMarker, hasManualMarker := request.Metadata[taskmodels.MetaKeyForkPRRequiresManualStart] + if hasManualMarker != !tt.wantAutostart || (hasManualMarker && manualMarker != true) { + t.Errorf("fork_pr_requires_manual_start = (%v, present=%v), want present=%v", + manualMarker, hasManualMarker, !tt.wantAutostart) + } + }) + } +} + +func triggerReviewWatchForAutoStartTest( + t *testing.T, + detailPR *github.PR, + detailErr error, +) (*github.NewReviewPREvent, []*github.PR) { + t.Helper() + ctx := context.Background() + store := newReviewWatchGitHubStore(t) + mockClient := github.NewMockClient() + mockClient.SetReviewRequestedPRs([]*github.PR{{ + Number: 42, Title: "Review this change", HTMLURL: "https://github.com/acme/widget/pull/42", + RepoOwner: "acme", RepoName: "widget", HeadBranch: "feature", BaseBranch: "main", + }}) + mockClient.SetPRDetail("acme", "widget", 42, detailPR, detailErr) + if err := store.UpsertWorkspaceConnection(ctx, &github.WorkspaceConnection{ + WorkspaceID: "ws1", Source: github.ConnectionSourcePAT, GitHubHost: "github.com", Login: "reviewer", + Status: github.ConnectionStatusActive, + }); err != nil { + t.Fatalf("create workspace connection: %v", err) + } + eventBus := bus.NewMemoryEventBus(testLogger()) + t.Cleanup(eventBus.Close) + var emitted *github.NewReviewPREvent + _, err := eventBus.Subscribe(events.GitHubNewReviewPR, func(_ context.Context, event *bus.Event) error { + payload, ok := event.Data.(*github.NewReviewPREvent) + if !ok { + return errors.New("event data is not a NewReviewPREvent") + } + emitted = payload + return nil + }) + if err != nil { + t.Fatalf("subscribe to review event: %v", err) + } + githubService := github.NewService(mockClient, github.AuthMethodPAT, nil, store, eventBus, testLogger()) + t.Cleanup(githubService.Stop) + github.NewMockController(mockClient, store, eventBus, githubService, testLogger()) + watch := &github.ReviewWatch{ + ID: "watch-1", WorkspaceID: "ws1", WorkflowID: "wf1", WorkflowStepID: "step1", + AgentProfileID: testAgentProfileID, Repos: []github.RepoFilter{{Owner: "acme", Name: "widget"}}, Enabled: true, + } + if err := store.CreateReviewWatch(ctx, watch); err != nil { + t.Fatalf("create review watch: %v", err) + } + prs, err := githubService.TriggerReviewWatch(ctx, watch) + if err != nil { + t.Fatalf("TriggerReviewWatch: %v", err) + } + return emitted, prs +} + +func newReviewWatchGitHubStore(t *testing.T) *github.Store { + t.Helper() + dbConn, err := db.OpenSQLite(filepath.Join(t.TempDir(), "review-watch.db")) + if err != nil { + t.Fatalf("open review watch database: %v", err) + } + sqlxDB := sqlx.NewDb(dbConn, "sqlite3") + t.Cleanup(func() { _ = sqlxDB.Close() }) + if _, err := sqlxDB.Exec("CREATE TABLE workspaces (id TEXT PRIMARY KEY)"); err != nil { + t.Fatalf("create workspace schema: %v", err) + } + if _, err := sqlxDB.Exec("INSERT INTO workspaces (id) VALUES (?)", "ws1"); err != nil { + t.Fatalf("create workspace: %v", err) + } + store, err := github.NewStore(sqlxDB, sqlxDB) + if err != nil { + t.Fatalf("create GitHub store: %v", err) + } + return store +} + // TestBuildIssueTaskTitle_TruncatesTitle mirrors the review-title regression for // the GitHub issue watcher's "Issue #: " builder. func TestBuildIssueTaskTitle_TruncatesTitle(t *testing.T) { diff --git a/docs/plans/review-watch-pr-identity/plan.md b/docs/plans/review-watch-pr-identity/plan.md new file mode 100644 index 0000000000..aefba68645 --- /dev/null +++ b/docs/plans/review-watch-pr-identity/plan.md @@ -0,0 +1,103 @@ +--- +created: 2026-09-25 +status: complete +requirements: + - REQ-INTEGRATIONS-GITHUB-FORK-REVIEW-START-001 +system_design: + - ../../specs/integrations/system-design/github-fork-review-start.md +legacy_specs: [] +--- + +# Implementation Plan: Review-watch PR identity + +## Overview + +Restore configured auto-start for same-repository GitHub review-watch PRs by +preserving repository identity during search-result enrichment. One work order +covers the provider-to-orchestrator path and its regression tests. + +## Evidence and root cause + +`CheckReviewWatch` obtains lightweight PRs through GitHub search, then calls +`enrichPRDetails` before publishing `GitHubNewReviewPR`. The latter currently +copies branch names, SHAs, diff counts, and mergeability, but omits head +repository identity. `reviewPRHeadMatchesBase` requires that identity, so +`buildReviewTaskRequest` marks an internal PR as requiring manual start and +does not grant `auto_start_claimed`. The existing orchestrator unit test gives +the event a complete PR directly, so it does not exercise enrichment. + +This violates +[`AC-INTEGRATIONS-GITHUB-FORK-REVIEW-START-001.1`](../../specs/integrations/requirements/github-fork-review-start.md). +The existing requirement and fork security decision remain correct. The +[system design](../../specs/integrations/system-design/github-fork-review-start.md) +now states the provider-data contract that was missing. + +## Scope + +### In scope + +- Copy head and base repository metadata and PR detail fields from `GetPR` + into newly found review-watch PRs. +- Fetch details when branch names exist but head repository identity is absent. +- Prove that a same-repository search hit gains its auto-start token after + enrichment, while a fork or failed detail fetch remains manual-start only. + +### Out of scope + +- Changes to fork trust policy, automated launch gates, workflow admission, or + manual PR-link behavior. +- Automatic repair of tasks already created with the manual-start marker or + direct edits to existing SQLite task metadata. +- Rendered UI changes or new public API fields. + +## Technical approach + +In `apps/backend/internal/github/service_reviews.go`, update +`enrichPRDetails` to copy the detail response's head repository owner, name, +node ID, clone URL, base repository owner/name, base SHA, mergeability state, +and maintainer permission. Preserve all existing copied fields. Also carry +repository numeric IDs and base default branch if supplied by the same full +response, so the enriched `PR` stays coherent. Keep the detail fetch on missing +branch or head identity; on fetch error leave the identity incomplete. + +The review event already carries the returned `PR` without a second mapping. +`buildReviewTaskRequest` remains the trust decision point. Do not infer a +head repository from the search-result target. + +## Tests + +| Criterion | Evidence | +| --- | --- | +| AC-INTEGRATIONS-GITHUB-FORK-REVIEW-START-001.1 | `service_reviews_test.go` verifies detail fields are copied; `TestReviewWatchSearchResultIdentityControlsAutoStart` runs a search-shaped result through `TriggerReviewWatch`, then passes that same returned event PR to `buildReviewTaskRequest` and verifies internal identity grants `auto_start_claimed=true` without a manual-start marker. | +| AC-INTEGRATIONS-GITHUB-FORK-REVIEW-START-001.2 | The same end-to-end test verifies a fork head and failed detail lookup retain the manual-start marker and omit the auto-start claim. | + +The new provider regression must fail before the production change because +the head repository fields remain empty. + +## E2E tests + +The backend integration test connects the watch search result to the review +event and task request without a browser. Its mock client models distinct +search and detail responses. Existing +`apps/web/e2e/tests/pr/pr-watcher-dockview-layout.spec.ts` exercises a mock +watch's visible auto-start behavior; no rendered UI behavior changes here. + +## Work orders + +- [x] [Task 01: Preserve review PR identity through enrichment](task-01-preserve-review-pr-identity.md) + +## Verification results + +The full GitHub package tests and targeted orchestrator tests passed, including +the search-result enrichment event path and same-repository/fork/unknown +identity launch decisions. Focused race tests for both packages, the backend +build, and backend lint passed. The environment-isolated full backend suite did +not pass because two real-process probe assertions in +`internal/agentctl/server/process/probe` returned `live` where they expect +`settled`; an isolated rerun reproduced both failures. + +## Risks + +- A failed or incomplete `GetPR` response must continue to fail closed. +- Copying only owner/name would fix launch but leave downstream PR metadata + incomplete; the field-completeness test guards the whole detail mapping. diff --git a/docs/plans/review-watch-pr-identity/task-01-preserve-review-pr-identity.md b/docs/plans/review-watch-pr-identity/task-01-preserve-review-pr-identity.md new file mode 100644 index 0000000000..a8bb1fa85a --- /dev/null +++ b/docs/plans/review-watch-pr-identity/task-01-preserve-review-pr-identity.md @@ -0,0 +1,119 @@ +--- +id: "01-preserve-review-pr-identity" +title: "Preserve review PR identity through enrichment" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +requirements: + - REQ-INTEGRATIONS-GITHUB-FORK-REVIEW-START-001 +acceptance_criteria: + - AC-INTEGRATIONS-GITHUB-FORK-REVIEW-START-001.1 + - AC-INTEGRATIONS-GITHUB-FORK-REVIEW-START-001.2 +system_design: + - ../../specs/integrations/system-design/github-fork-review-start.md +--- + +# Task 01: Preserve review PR identity through enrichment + +## Summary + +Carry complete `GetPR` identity and related metadata onto review-watch search +results before they become events. Prove that the resulting same-repository +event grants an automatic launch token while fork and unknown identities retain +the existing manual-start boundary. + +## In scope + +- Write a failing enrichment regression using a search-shaped PR and a fake + `GetPR` result in `service_reviews_test.go`. +- Copy the requested head/base repository, SHA, mergeability, and maintainer + fields in `enrichPRDetails`, preserving the current branch and diff fields. +- Exercise `TriggerReviewWatch` to event to `buildReviewTaskRequest` in backend + tests, including same-repository, fork, and unavailable-detail cases. Pass the + same emitted PR into the request builder. +- Let `MockClient` provide separate review-search and detail responses so the + test reproduces GitHub's lightweight search result. + +## Out of scope + +- A changed fork policy, metadata backfill, database migration, and UI changes. + +## Acceptance + +1. The new enrichment regression is RED before the code change and GREEN after + it; all requested fields and existing fields match the `GetPR` result. +2. A same-repository review PR from the watch passes + `reviewPRHeadMatchesBase`, has `auto_start_claimed`, and lacks + `fork_pr_requires_manual_start`. +3. A fork or identity-incomplete PR keeps `fork_pr_requires_manual_start` and + has no auto-start token, including when `GetPR` fails. + +## Verification + +```bash +(cd apps/backend && go test ./internal/github -run 'TestEnrichPRDetails|TestCheckReviewWatch' -count=1) +(cd apps/backend && go test ./internal/orchestrator -run 'TestReviewWatchSearchResultIdentityControlsAutoStart|TestBuildReviewTaskRequest_ForkPRRequiresManualStart' -count=1) +(cd apps/backend && go test -race ./internal/github -run 'TestCheckReviewWatchEnrichesSearchResultsBeforePublishingEvent|TestMockClient_Reset' -count=1) +(cd apps/backend && go test -race ./internal/orchestrator -run 'TestReviewWatchSearchResultIdentityControlsAutoStart|TestBuildReviewTaskRequest_ForkPRRequiresManualStart' -count=1) +make -C apps/backend test +make -C apps/backend lint +``` + +## Files likely touched + +- `apps/backend/internal/github/service_reviews.go` +- `apps/backend/internal/github/mock_client.go` +- `apps/backend/internal/github/service_reviews_test.go` +- `apps/backend/internal/orchestrator/event_handlers_github_review_test.go` + +## Dependencies + +None. + +## Risks + +- Keep the lookup tied to the search result's target repository. Missing + provider identity must never be inferred from that target. +- Existing tests construct complete PRs and may miss a search-to-detail mapping + regression unless the new tests use separate search and detail objects. + +## Parallelism + +`sequential` + +## Inputs + +- [Fork review-start requirement](../../specs/integrations/requirements/github-fork-review-start.md), + [design](../../specs/integrations/system-design/github-fork-review-start.md), + and [security decision](../../decisions/2026-09-23-fork-pr-review-watch-manual-start.md). +- `CheckReviewWatch`, `enrichPRDetails`, `publishNewReviewPREvent`, + `buildReviewTaskRequest`, and nearby backend test helpers. + +## Results + +Implemented full PR detail enrichment for review-watch search results whose +head identity is absent, even when both branch names are already present. The +published event retains head/base repository identity, revisions, mergeability, +maintainer permission, branch names, and diff counts. The regression was RED +because `GetPR` was not called, then passed with the fix. The same-repository, +fork, and failed-detail paths all preserve the expected launch boundary. The +orchestrator test sends the actual `TriggerReviewWatch` event PR into +`buildReviewTaskRequest` and asserts both the event identity and launch metadata. + +Verification: + +- PASS: `go test ./internal/github -run 'TestEnrichPRDetails|TestCheckReviewWatch' -count=1` +- PASS: `go test ./internal/orchestrator -run 'TestBuildReviewTaskRequest|TestReviewWatch' -count=1` +- PASS: `go test ./internal/github -count=1` +- PASS: `go test ./internal/orchestrator -run 'TestReviewWatchSearchResultIdentityControlsAutoStart|TestBuildReviewTaskRequest_ForkPRRequiresManualStart' -count=1` +- PASS: `go test -race ./internal/github -run 'TestCheckReviewWatchEnrichesSearchResultsBeforePublishingEvent|TestMockClient_Reset' -count=1` +- PASS: `go test -race ./internal/orchestrator -run 'TestReviewWatchSearchResultIdentityControlsAutoStart|TestBuildReviewTaskRequest_ForkPRRequiresManualStart' -count=1` +- PASS: `make -C apps/backend build` +- PASS: `make -C apps/backend lint` +- BLOCKED by unrelated existing probe failures: `make -C apps/backend test` +- BLOCKED by the same two real-process probe failures after config isolation: + `env -u KANDEV_INTERNAL_CONFIG_FILE -u KANDEV_INTERNAL_CONFIG_HOME_FILE -u KANDEV_INTERNAL_AGENTCTL_STARTUP_CONFIG make -C apps/backend test` +- Confirmed: the isolated `TestProbeRealTree_AllDescendantsPreTurn_Settled` and + `TestProbeRealTree_NewDescendantAfterTurnStart_Live` tests both return `live` + where they expect `settled`. diff --git a/docs/specs/integrations/system-design/github-fork-review-start.md b/docs/specs/integrations/system-design/github-fork-review-start.md index 9e0f8249e2..b6924ca657 100644 --- a/docs/specs/integrations/system-design/github-fork-review-start.md +++ b/docs/specs/integrations/system-design/github-fork-review-start.md @@ -39,6 +39,16 @@ does not authorize execution. ## Identity and auto-start token +`CheckReviewWatch` receives lightweight PRs from GitHub search and enriches +untracked results with `GetPR` before publishing review events. Enrichment +must carry the full response's head and base repository identity, head and +base revisions, mergeability details, and maintainer modification permission +alongside the existing branch and diff fields. A PR with branch names but no +head repository identity still needs enrichment. If the detail fetch fails or +the provider has no head identity, the event retains incomplete identity and +the launch decision remains fail-closed. Search-result target identity remains +the repository used to look up the PR. + `buildReviewTaskRequest` compares the PR head owner and repository name with the target owner and name using case-insensitive GitHub identity. If both identities are present and equal, it creates the normal one-shot auto-start token. If the @@ -77,10 +87,11 @@ creation and launch behavior therefore remains unchanged for fork PR links. ## Verification -Backend tests cover same-repository auto-start tokens, fork and missing-head -identity suppression, both review-watch auto-start paths, rejection at the -central automatic launch boundary, and successful manual start. Existing -browser tests continue to prove manual PR-link launch from a fork head. +Backend tests cover search-result enrichment through review event creation, +same-repository auto-start tokens, fork and missing-head identity suppression, +both review-watch auto-start paths, rejection at the central automatic launch +boundary, and successful manual start. Existing browser tests continue to +prove manual PR-link launch from a fork head. ## Related decisions