diff --git a/apps/backend/internal/github/branch_protection.go b/apps/backend/internal/github/branch_protection.go new file mode 100644 index 0000000000..7abfa70ff6 --- /dev/null +++ b/apps/backend/internal/github/branch_protection.go @@ -0,0 +1,119 @@ +package github + +import ( + "context" + "sync" + "time" +) + +// BranchProtection captures the subset of a GitHub branch protection rule that +// the CI popover surfaces ("N approvals required to merge"). Stored values +// have a fixed TTL so changes to the protection rule on GitHub propagate +// without restarting the backend. +type BranchProtection struct { + RequiredApprovingReviewCount int + HasRule bool // false when GitHub returned 404 (no rule) + FetchedAt time.Time +} + +// branchProtectionCacheTTL is how long a cached protection lookup stays +// authoritative before a refetch is allowed. One hour balances API quota +// (4-PRs-per-cycle GraphQL polling) against rule-change latency (rare). +const branchProtectionCacheTTL = time.Hour + +// branchProtectionCache is an in-memory TTL cache keyed by +// "owner/repo@base_branch". Concurrent reads share the RLock; concurrent +// cache misses for the same key may each issue their own GitHub fetch and +// race to set() the result — values are deterministic so the loser's +// write is harmless, and the extra round-trips are bounded by the 1h TTL. +// Stale entries are evicted on read so the map cannot grow unbounded over +// a long-running process: callers always observe an active TTL window. +type branchProtectionCache struct { + mu sync.RWMutex + m map[string]BranchProtection + ttl time.Duration +} + +func newBranchProtectionCache() *branchProtectionCache { + return &branchProtectionCache{ + m: make(map[string]BranchProtection), + ttl: branchProtectionCacheTTL, + } +} + +func (c *branchProtectionCache) get(key string) (BranchProtection, bool) { + c.mu.RLock() + bp, ok := c.m[key] + c.mu.RUnlock() + if !ok { + return BranchProtection{}, false + } + if time.Since(bp.FetchedAt) <= c.ttl { + return bp, true + } + // Evict the stale entry so the map doesn't grow unbounded across + // long-running deployments with many distinct (owner, repo, branch) + // keys. Recheck under the write lock so a concurrent set() that just + // landed isn't clobbered. + c.mu.Lock() + if cur, present := c.m[key]; present && time.Since(cur.FetchedAt) > c.ttl { + delete(c.m, key) + } + c.mu.Unlock() + return BranchProtection{}, false +} + +func (c *branchProtectionCache) set(key string, bp BranchProtection) { + c.mu.Lock() + defer c.mu.Unlock() + c.m[key] = bp +} + +func branchProtectionKey(owner, repo, branch string) string { + return owner + "/" + repo + "@" + branch +} + +// BranchProtectionFetcher is implemented by the live GitHub clients (PAT, gh +// CLI). The mock client returns a sentinel "no rule" so e2e tests get +// deterministic results unless they seed via the mock controller. +type BranchProtectionFetcher interface { + FetchBranchProtection(ctx context.Context, owner, repo, branch string) (BranchProtection, error) +} + +// fetchRequiredReviews returns the branch-protection-derived +// RequiredApprovingReviewCount for (owner, repo, branch), or nil if: +// - the active client doesn't implement BranchProtectionFetcher, +// - the upstream call errors (token lacks scope, network down, etc.), +// - the repo has no protection rule on the base branch. +// +// Result is cached for branchProtectionCacheTTL. nil never gets cached so a +// transient error is retried on the next sync. +func (s *Service) fetchRequiredReviews(ctx context.Context, owner, repo, branch string) *int { + if owner == "" || repo == "" || branch == "" { + return nil + } + key := branchProtectionKey(owner, repo, branch) + if bp, ok := s.protectionCache.get(key); ok { + if !bp.HasRule { + return nil + } + n := bp.RequiredApprovingReviewCount + return &n + } + fetcher, ok := s.client.(BranchProtectionFetcher) + if !ok { + return nil + } + bp, err := fetcher.FetchBranchProtection(ctx, owner, repo, branch) + if err != nil { + // Don't cache errors — next sync retries. + return nil + } + bp.FetchedAt = time.Now().UTC() + s.protectionCache.set(key, bp) + if !bp.HasRule { + return nil + } + n := bp.RequiredApprovingReviewCount + return &n +} diff --git a/apps/backend/internal/github/branch_protection_test.go b/apps/backend/internal/github/branch_protection_test.go new file mode 100644 index 0000000000..475adf365a --- /dev/null +++ b/apps/backend/internal/github/branch_protection_test.go @@ -0,0 +1,138 @@ +package github + +import ( + "context" + "errors" + "sync/atomic" + "testing" + "testing/synctest" + "time" +) + +// fakeBPFetcher is a minimal BranchProtectionFetcher for testing the +// service-level cache wiring. It records call counts and returns canned +// values per (owner, repo, branch) key. +type fakeBPFetcher struct { + calls atomic.Int64 + bp BranchProtection + err error +} + +func (f *fakeBPFetcher) FetchBranchProtection(_ context.Context, _, _, _ string) (BranchProtection, error) { + f.calls.Add(1) + return f.bp, f.err +} + +// minimal Client surface — only the bits fetchRequiredReviews needs (it +// asserts the client implements BranchProtectionFetcher; everything else +// can be nil-valued). +type fakeBPClient struct { + *fakeBPFetcher + Client +} + +func newServiceWithBPFetcher(f *fakeBPFetcher) *Service { + return &Service{ + client: fakeBPClient{fakeBPFetcher: f}, + protectionCache: newBranchProtectionCache(), + } +} + +func TestBranchProtectionCache_GetSet_TTL(t *testing.T) { + // synctest.Test advances time in virtual ticks so a 60ms sleep is + // instantaneous wall-clock — keeps the suite fast and deterministic + // (no real sleeps that flake on heavily loaded CI runners). + synctest.Test(t, func(t *testing.T) { + c := newBranchProtectionCache() + c.ttl = 50 * time.Millisecond + key := "o/r@main" + c.set(key, BranchProtection{HasRule: true, RequiredApprovingReviewCount: 2, FetchedAt: time.Now()}) + got, ok := c.get(key) + if !ok || got.RequiredApprovingReviewCount != 2 { + t.Fatalf("fresh entry: got=%+v ok=%v", got, ok) + } + time.Sleep(60 * time.Millisecond) + if _, ok := c.get(key); ok { + t.Fatalf("stale entry should not be returned") + } + }) +} + +func TestBranchProtectionCache_StaleEntryIsEvicted(t *testing.T) { + c := newBranchProtectionCache() + c.ttl = 1 * time.Millisecond + c.set("o/r@main", BranchProtection{HasRule: true, FetchedAt: time.Now().Add(-time.Hour)}) + if _, ok := c.get("o/r@main"); ok { + t.Fatalf("expected stale miss") + } + c.mu.RLock() + _, present := c.m["o/r@main"] + c.mu.RUnlock() + if present { + t.Fatalf("stale entry should have been deleted from map") + } +} + +func TestFetchRequiredReviews_CacheHit(t *testing.T) { + f := &fakeBPFetcher{bp: BranchProtection{HasRule: true, RequiredApprovingReviewCount: 3}} + s := newServiceWithBPFetcher(f) + if got := s.fetchRequiredReviews(context.Background(), "o", "r", "main"); got == nil || *got != 3 { + t.Fatalf("first call: %v", got) + } + if got := s.fetchRequiredReviews(context.Background(), "o", "r", "main"); got == nil || *got != 3 { + t.Fatalf("cached call: %v", got) + } + if calls := f.calls.Load(); calls != 1 { + t.Fatalf("fetcher called %d times, want 1 (cache miss + hit)", calls) + } +} + +func TestFetchRequiredReviews_NoRuleIsCached(t *testing.T) { + f := &fakeBPFetcher{bp: BranchProtection{HasRule: false}} + s := newServiceWithBPFetcher(f) + if got := s.fetchRequiredReviews(context.Background(), "o", "r", "main"); got != nil { + t.Fatalf("expected nil for no-rule, got %v", got) + } + // Second call must not hit the network — the negative result is cached + // so we don't re-query upstream every poll. + _ = s.fetchRequiredReviews(context.Background(), "o", "r", "main") + if calls := f.calls.Load(); calls != 1 { + t.Fatalf("fetcher called %d times, want 1", calls) + } +} + +func TestFetchRequiredReviews_ErrorIsNotCached(t *testing.T) { + f := &fakeBPFetcher{err: errors.New("boom")} + s := newServiceWithBPFetcher(f) + if got := s.fetchRequiredReviews(context.Background(), "o", "r", "main"); got != nil { + t.Fatalf("expected nil on error, got %v", got) + } + if got := s.fetchRequiredReviews(context.Background(), "o", "r", "main"); got != nil { + t.Fatalf("expected nil on retry, got %v", got) + } + if calls := f.calls.Load(); calls != 2 { + t.Fatalf("fetcher called %d times, want 2 (error not cached)", calls) + } +} + +func TestFetchRequiredReviews_ClientWithoutFetcherReturnsNil(t *testing.T) { + // noop client doesn't satisfy BranchProtectionFetcher. + s := &Service{client: nil, protectionCache: newBranchProtectionCache()} + if got := s.fetchRequiredReviews(context.Background(), "o", "r", "main"); got != nil { + t.Fatalf("expected nil when client lacks BranchProtectionFetcher, got %v", got) + } +} + +func TestFetchRequiredReviews_TTLRefetch(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + f := &fakeBPFetcher{bp: BranchProtection{HasRule: true, RequiredApprovingReviewCount: 1}} + s := newServiceWithBPFetcher(f) + s.protectionCache.ttl = 50 * time.Millisecond + _ = s.fetchRequiredReviews(context.Background(), "o", "r", "main") + time.Sleep(60 * time.Millisecond) + _ = s.fetchRequiredReviews(context.Background(), "o", "r", "main") + if calls := f.calls.Load(); calls != 2 { + t.Fatalf("fetcher called %d times, want 2 (refetch after TTL)", calls) + } + }) +} diff --git a/apps/backend/internal/github/client_helpers.go b/apps/backend/internal/github/client_helpers.go index ff16c2292e..7df629414a 100644 --- a/apps/backend/internal/github/client_helpers.go +++ b/apps/backend/internal/github/client_helpers.go @@ -91,17 +91,103 @@ func getPRStatus(ctx context.Context, c Client, owner, repo string, number int) reviewState, pendingReviewCount := deriveReviewSyncState(pr, reviews) total, passing := countCheckResults(checks) return &PRStatus{ - PR: pr, - ReviewState: reviewState, - ChecksState: computeOverallCheckStatus(checks), - MergeableState: pr.MergeableState, - ReviewCount: len(reviews), + PR: pr, + ReviewState: reviewState, + ChecksState: computeOverallCheckStatus(checks), + MergeableState: pr.MergeableState, + // ReviewCount is the number of distinct reviewers whose latest review + // state is APPROVED — it's the value the popover renders as + // "Approved (N)" / "Approved N / M required". Counting raw review + // entries (`len(reviews)`) double-counts authors who left multiple + // reviews and also includes COMMENTED / CHANGES_REQUESTED rows. + ReviewCount: countApprovedReviewers(reviews), PendingReviewCount: pendingReviewCount, ChecksTotal: total, ChecksPassing: passing, + // REST path actually counted check runs + reviews — distinguish + // from the batched-GraphQL path that only carries rollup state. + ChecksPopulated: true, + ReviewCountsPopulated: true, }, nil } +// reviewSample is a normalized review row consumed by latestReviewStateByAuthor. +// Adapter functions below convert PRReview (REST) and reviewNode (GraphQL) +// to this shape so the per-reviewer dedup rule is only spelled out once. +type reviewSample struct { + author string + state string + at time.Time +} + +// latestReviewStateByAuthor reduces a list of reviews to "one entry per +// reviewer, latest binding state wins". COMMENTED and PENDING never +// override a prior APPROVED / CHANGES_REQUESTED for the same author so a +// reviewer who approved-then-commented still counts as approved. +// Anonymous reviewers (deleted users) get a unique synthetic key so each +// review still counts independently. +func latestReviewStateByAuthor(samples []reviewSample) map[string]reviewSample { + byAuthor := make(map[string]reviewSample, len(samples)) + for _, s := range samples { + key := s.author + if key == "" { + key = ":" + s.at.UTC().Format(time.RFC3339Nano) + } + state := strings.ToUpper(s.state) + if state != reviewStateApproved && state != reviewStateChangesRequested { + if _, ok := byAuthor[key]; ok { + continue + } + } + prev, ok := byAuthor[key] + if !ok || !s.at.Before(prev.at) { + byAuthor[key] = reviewSample{author: s.author, state: state, at: s.at} + } + } + return byAuthor +} + +// countApprovedAuthors counts distinct reviewers whose latest binding state is +// APPROVED. Shared between the REST and GraphQL count helpers. +func countApprovedAuthors(samples []reviewSample) int { + count := 0 + for _, s := range latestReviewStateByAuthor(samples) { + if s.state == reviewStateApproved { + count++ + } + } + return count +} + +// reduceReviewSummary collapses the per-author map into a single overall +// review state. CHANGES_REQUESTED wins across authors; APPROVED otherwise; +// "" when nothing is binding. Shared between REST and GraphQL paths. +func reduceReviewSummary(samples []reviewSample) string { + hasApproval := false + for _, l := range latestReviewStateByAuthor(samples) { + switch l.state { + case reviewStateChangesRequested: + return computedReviewStateChangesRequested + case reviewStateApproved: + hasApproval = true + } + } + if hasApproval { + return computedReviewStateApproved + } + return "" +} + +// countApprovedReviewers returns the number of distinct authors whose latest +// review state is APPROVED. REST-path adapter over the shared helper. +func countApprovedReviewers(reviews []PRReview) int { + samples := make([]reviewSample, len(reviews)) + for i, r := range reviews { + samples[i] = reviewSample{author: r.Author, state: r.State, at: r.CreatedAt} + } + return countApprovedAuthors(samples) +} + // countCheckResults returns (total, passing) counts using the same logic as // computeOverallCheckStatus: skipped/neutral and still-running checks are // excluded from the total so the X/Y badge communicates pass rate rather diff --git a/apps/backend/internal/github/client_helpers_test.go b/apps/backend/internal/github/client_helpers_test.go index 6500215993..5a869c4d92 100644 --- a/apps/backend/internal/github/client_helpers_test.go +++ b/apps/backend/internal/github/client_helpers_test.go @@ -471,3 +471,123 @@ func TestHasChangesRequested(t *testing.T) { }) } } + +// --- Reviewer dedup helpers (countApprovedReviewers + reduceReviewSummary) --- + +func TestCountApprovedReviewers(t *testing.T) { + t0 := time.Date(2025, 5, 1, 12, 0, 0, 0, time.UTC) + mk := func(author, state string, offset time.Duration) PRReview { + return PRReview{Author: author, State: state, CreatedAt: t0.Add(offset)} + } + cases := []struct { + name string + reviews []PRReview + want int + }{ + {name: "empty", reviews: []PRReview{}, want: 0}, + { + name: "single approval", + reviews: []PRReview{mk("alice", reviewStateApproved, 0)}, + want: 1, + }, + { + name: "dedup same author latest wins", + reviews: []PRReview{ + mk("alice", reviewStateChangesRequested, 0), + mk("alice", reviewStateApproved, time.Hour), + }, + want: 1, + }, + { + name: "COMMENTED does not replace prior APPROVED", + reviews: []PRReview{ + mk("alice", reviewStateApproved, 0), + mk("alice", "COMMENTED", time.Hour), + }, + want: 1, + }, + { + name: "PENDING never counts even alone", + reviews: []PRReview{mk("alice", "PENDING", 0)}, + want: 0, + }, + { + name: "two distinct approvers", + reviews: []PRReview{ + mk("alice", reviewStateApproved, 0), + mk("bob", reviewStateApproved, time.Hour), + }, + want: 2, + }, + { + name: "approve then change-request reverts", + reviews: []PRReview{ + mk("alice", reviewStateApproved, 0), + mk("alice", reviewStateChangesRequested, time.Hour), + }, + want: 0, + }, + { + name: "anonymous reviewers counted separately", + reviews: []PRReview{ + mk("", reviewStateApproved, 0), + mk("", reviewStateApproved, time.Hour), + }, + want: 2, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := countApprovedReviewers(tc.reviews); got != tc.want { + t.Fatalf("countApprovedReviewers = %d, want %d", got, tc.want) + } + }) + } +} + +func TestReduceReviewSummary(t *testing.T) { + t0 := time.Date(2025, 5, 1, 12, 0, 0, 0, time.UTC) + mk := func(author, state string, offset time.Duration) reviewSample { + return reviewSample{author: author, state: state, at: t0.Add(offset)} + } + cases := []struct { + name string + in []reviewSample + want string + }{ + {name: "empty", in: nil, want: ""}, + { + name: "single approval", + in: []reviewSample{mk("a", reviewStateApproved, 0)}, + want: computedReviewStateApproved, + }, + { + name: "changes requested wins across authors", + in: []reviewSample{ + mk("a", reviewStateApproved, 0), + mk("b", reviewStateChangesRequested, time.Hour), + }, + want: computedReviewStateChangesRequested, + }, + { + name: "later approval supersedes earlier changes-requested from same author", + in: []reviewSample{ + mk("a", reviewStateChangesRequested, 0), + mk("a", reviewStateApproved, time.Hour), + }, + want: computedReviewStateApproved, + }, + { + name: "only comments returns empty", + in: []reviewSample{mk("a", "COMMENTED", 0)}, + want: "", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := reduceReviewSummary(tc.in); got != tc.want { + t.Fatalf("reduceReviewSummary = %q, want %q", got, tc.want) + } + }) + } +} diff --git a/apps/backend/internal/github/gh_client.go b/apps/backend/internal/github/gh_client.go index 73c9ba7ebb..17ac5156b7 100644 --- a/apps/backend/internal/github/gh_client.go +++ b/apps/backend/internal/github/gh_client.go @@ -505,6 +505,65 @@ func (c *GHClient) GetPRStatus(ctx context.Context, owner, repo string, number i return getPRStatus(ctx, c, owner, repo, number) } +// FetchBranchProtection looks up branch protection via `gh api`. A 404 from +// `gh` (no rule) and a 403 (token lacks Administration: Read scope) are both +// treated as "no rule we can see" so callers cache the negative result and +// don't burn rate-limit quota on every poll. The string match is permissive +// across formats so a future `gh` CLI release that tweaks the wording +// doesn't silently break the cache. +func (c *GHClient) FetchBranchProtection(ctx context.Context, owner, repo, branch string) (BranchProtection, error) { + out, err := c.run(ctx, "api", + fmt.Sprintf("repos/%s/%s/branches/%s/protection", owner, repo, branch)) + if err != nil { + if isNotFoundErr(err) || isForbiddenErr(err) { + return BranchProtection{HasRule: false}, nil + } + return BranchProtection{}, err + } + var raw struct { + RequiredPullRequestReviews *struct { + RequiredApprovingReviewCount int `json:"required_approving_review_count"` + } `json:"required_pull_request_reviews"` + } + if err := json.Unmarshal([]byte(out), &raw); err != nil { + return BranchProtection{}, fmt.Errorf("decode branch protection: %w", err) + } + if raw.RequiredPullRequestReviews == nil { + return BranchProtection{HasRule: true}, nil + } + return BranchProtection{ + HasRule: true, + RequiredApprovingReviewCount: raw.RequiredPullRequestReviews.RequiredApprovingReviewCount, + }, nil +} + +// isNotFoundErr matches the formats the `gh` CLI uses to report a 404 across +// recent versions: "HTTP 404", "HTTP 404: ...", and "404 Not Found". +// Restrictive enough that an unrelated 404 substring (e.g. inside a JSON +// payload accidentally rendered into stderr) is unlikely to false-match. +func isNotFoundErr(err error) bool { + if err == nil { + return false + } + s := err.Error() + return strings.Contains(s, "HTTP 404") || + strings.Contains(s, "404 Not Found") || + strings.Contains(s, "status: 404") +} + +// isForbiddenErr matches the formats the `gh` CLI uses to report a 403, used +// by the branch-protection lookup to silently fall back to "no rule" when +// the token lacks Administration: Read scope. +func isForbiddenErr(err error) bool { + if err == nil { + return false + } + s := err.Error() + return strings.Contains(s, "HTTP 403") || + strings.Contains(s, "403 Forbidden") || + strings.Contains(s, "status: 403") +} + func (c *GHClient) ListPRFiles(ctx context.Context, owner, repo string, number int) ([]PRFile, error) { out, err := c.run(ctx, "api", fmt.Sprintf("repos/%s/%s/pulls/%d/files", owner, repo, number), diff --git a/apps/backend/internal/github/gh_client_test.go b/apps/backend/internal/github/gh_client_test.go index dac69d689b..4836ecc459 100644 --- a/apps/backend/internal/github/gh_client_test.go +++ b/apps/backend/internal/github/gh_client_test.go @@ -1,10 +1,57 @@ package github import ( + "errors" "testing" "time" ) +func TestIsNotFoundErr(t *testing.T) { + cases := []struct { + name string + err error + want bool + }{ + {"nil", nil, false}, + {"HTTP 404", errors.New("gh api: HTTP 404"), true}, + {"HTTP 404 with suffix", errors.New("gh: HTTP 404: Not Found (404)"), true}, + {"404 Not Found", errors.New("404 Not Found"), true}, + {"status: 404", errors.New("request failed (status: 404)"), true}, + {"unrelated 500", errors.New("HTTP 500: server error"), false}, + {"unrelated text", errors.New("connection refused"), false}, + {"403 not 404", errors.New("HTTP 403: Forbidden"), false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := isNotFoundErr(tc.err); got != tc.want { + t.Fatalf("isNotFoundErr(%v) = %v, want %v", tc.err, got, tc.want) + } + }) + } +} + +func TestIsForbiddenErr(t *testing.T) { + cases := []struct { + name string + err error + want bool + }{ + {"nil", nil, false}, + {"HTTP 403", errors.New("gh api: HTTP 403"), true}, + {"403 Forbidden", errors.New("403 Forbidden"), true}, + {"status: 403", errors.New("request failed (status: 403)"), true}, + {"404 not 403", errors.New("HTTP 404"), false}, + {"500 not 403", errors.New("HTTP 500"), false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := isForbiddenErr(tc.err); got != tc.want { + t.Fatalf("isForbiddenErr(%v) = %v, want %v", tc.err, got, tc.want) + } + }) + } +} + func TestParseTimePtr(t *testing.T) { tests := []struct { name string diff --git a/apps/backend/internal/github/graphql.go b/apps/backend/internal/github/graphql.go index 5cd5fe4e19..a4fed0dc1e 100644 --- a/apps/backend/internal/github/graphql.go +++ b/apps/backend/internal/github/graphql.go @@ -94,6 +94,12 @@ type batchedPRResult struct { ReviewRequests struct { TotalCount int `json:"totalCount"` } `json:"reviewRequests"` + ReviewThreads struct { + TotalCount int `json:"totalCount"` + Nodes []struct { + IsResolved bool `json:"isResolved"` + } `json:"nodes"` + } `json:"reviewThreads"` Commits struct { Nodes []struct { Commit struct { @@ -184,6 +190,7 @@ func prFieldsBlock() string { `author { login } createdAt updatedAt mergedAt closedAt ` + `reviews(last: 100) { nodes { state author { login } submittedAt } } ` + `reviewRequests(first: 0) { totalCount } ` + + `reviewThreads(first: 100) { totalCount nodes { isResolved } } ` + `commits(last: 1) { nodes { commit { statusCheckRollup { state } } } }` } @@ -236,59 +243,61 @@ func convertBatchedPRResult(raw *batchedPRResult, owner, repo string, number int if len(raw.Commits.Nodes) > 0 && raw.Commits.Nodes[0].Commit.StatusCheckRollup != nil { checksState = strings.ToLower(raw.Commits.Nodes[0].Commit.StatusCheckRollup.State) } + // Count unresolved threads from the fetched nodes. When the page is + // capped (totalCount > nodes), fall back to totalCount for the + // resolved-vs-unresolved estimate so the popover doesn't undercount on + // busy PRs. The fallback is conservative: we have no way to tell from + // the truncated page how many of the un-fetched threads are resolved, + // so attribute the unseen tail to "unresolved" — the popover's value + // is meant to be actionable, not exact, and over-reporting is safer + // than silently hiding open feedback. + unresolved := 0 + for _, t := range raw.ReviewThreads.Nodes { + if !t.IsResolved { + unresolved++ + } + } + if total := raw.ReviewThreads.TotalCount; total > len(raw.ReviewThreads.Nodes) { + unresolved += total - len(raw.ReviewThreads.Nodes) + } return &PRStatus{ - PR: pr, - ReviewState: reviewState, - ChecksState: checksState, - MergeableState: pr.MergeableState, + PR: pr, + ReviewState: reviewState, + ChecksState: checksState, + MergeableState: pr.MergeableState, + ReviewCount: countApprovedReviewerNodes(raw.Reviews.Nodes), + PendingReviewCount: raw.ReviewRequests.TotalCount, + ReviewCountsPopulated: true, + UnresolvedReviewThreads: unresolved, + UnresolvedReviewThreadsPopulated: true, } } +// reviewNodesToSamples converts the GraphQL reviewNode shape to the shared +// reviewSample slice used by the dedup helpers in client_helpers.go. +func reviewNodesToSamples(nodes []reviewNode) []reviewSample { + samples := make([]reviewSample, len(nodes)) + for i, n := range nodes { + samples[i] = reviewSample{author: n.Author.Login, state: n.State, at: n.SubmittedAt} + } + return samples +} + +// countApprovedReviewerNodes returns the number of distinct authors whose +// latest review state is APPROVED, on the GraphQL reviewNode shape. Thin +// adapter over countApprovedAuthors so REST and GraphQL agree on what +// "Approved (N)" means. +func countApprovedReviewerNodes(nodes []reviewNode) int { + return countApprovedAuthors(reviewNodesToSamples(nodes)) +} + // summarizeReviewState collapses the review history to a single -// "approved"/"changes_requested"/"" value matching the existing -// service-side summary. Per-reviewer dedup: each reviewer's most-recent -// review wins, so a CHANGES_REQUESTED followed by an APPROVED from the -// same author resolves to APPROVED. CHANGES_REQUESTED beats APPROVED across -// distinct reviewers. +// "approved"/"changes_requested"/"" value. Per-reviewer dedup: each +// reviewer's most-recent binding review wins, so a CHANGES_REQUESTED +// followed by APPROVED from the same author resolves to APPROVED. +// CHANGES_REQUESTED beats APPROVED across distinct reviewers. func summarizeReviewState(nodes []reviewNode) string { - type latest struct { - state string - at time.Time - } - byAuthor := map[string]latest{} - for _, n := range nodes { - // Anonymous reviewers (deleted users) get a unique bucket so each - // review still counts independently. - key := n.Author.Login - if key == "" { - key = ":" + n.SubmittedAt.UTC().Format(time.RFC3339Nano) - } - state := strings.ToUpper(n.State) - // COMMENTED and PENDING are non-binding states and shouldn't replace - // a prior APPROVED / CHANGES_REQUESTED for the same reviewer. - if state != reviewStateApproved && state != reviewStateChangesRequested { - if _, ok := byAuthor[key]; ok { - continue - } - } - prev, ok := byAuthor[key] - if !ok || !n.SubmittedAt.Before(prev.at) { - byAuthor[key] = latest{state: state, at: n.SubmittedAt} - } - } - hasApproval := false - for _, l := range byAuthor { - switch l.state { - case reviewStateChangesRequested: - return computedReviewStateChangesRequested - case reviewStateApproved: - hasApproval = true - } - } - if hasApproval { - return computedReviewStateApproved - } - return "" + return reduceReviewSummary(reviewNodesToSamples(nodes)) } // PATClient.ExecuteGraphQL satisfies GraphQLExecutor by POSTing to /graphql. diff --git a/apps/backend/internal/github/mock_client.go b/apps/backend/internal/github/mock_client.go index 6c1a707341..74db6dcbf6 100644 --- a/apps/backend/internal/github/mock_client.go +++ b/apps/backend/internal/github/mock_client.go @@ -51,6 +51,8 @@ type repoKey struct { type MockClient struct { mu sync.RWMutex user string + authenticated bool + authError string prs map[prKey]*PR prsByBranch map[branchKey]*PR orgs []GitHubOrg @@ -67,23 +69,26 @@ type MockClient struct { // NewMockClient creates a new MockClient with default values. func NewMockClient() *MockClient { return &MockClient{ - user: mockDefaultUser, - prs: make(map[prKey]*PR), - prsByBranch: make(map[branchKey]*PR), - repos: make(map[string][]GitHubRepo), - branches: make(map[repoKey][]RepoBranch), - reviews: make(map[prKey][]PRReview), - comments: make(map[prKey][]PRComment), - checks: make(map[checkKey][]CheckRun), - files: make(map[prKey][]PRFile), - commits: make(map[prKey][]PRCommitInfo), + user: mockDefaultUser, + authenticated: true, + prs: make(map[prKey]*PR), + prsByBranch: make(map[branchKey]*PR), + repos: make(map[string][]GitHubRepo), + branches: make(map[repoKey][]RepoBranch), + reviews: make(map[prKey][]PRReview), + comments: make(map[prKey][]PRComment), + checks: make(map[checkKey][]CheckRun), + files: make(map[prKey][]PRFile), + commits: make(map[prKey][]PRCommitInfo), } } // --- Client interface implementation --- func (m *MockClient) IsAuthenticated(context.Context) (bool, error) { - return true, nil + m.mu.RLock() + defer m.mu.RUnlock() + return m.authenticated, nil } func (m *MockClient) GetAuthenticatedUser(context.Context) (string, error) { @@ -270,6 +275,16 @@ func (m *MockClient) SetUser(username string) { m.user = username } +// SetAuthHealth toggles the authenticated state for the e2e auth-lost test. +// When authenticated=false the popover renders the "Reconnect GitHub" branch. +// authError is an opaque message preserved for diagnostics. +func (m *MockClient) SetAuthHealth(authenticated bool, authError string) { + m.mu.Lock() + defer m.mu.Unlock() + m.authenticated = authenticated + m.authError = authError +} + // AddPR adds a PR to the mock data store, indexed by owner/repo/number and branch. func (m *MockClient) AddPR(pr *PR) { m.mu.Lock() @@ -327,6 +342,34 @@ func (m *MockClient) AddCheckRuns(owner, repo, ref string, checks []CheckRun) { m.checks[k] = append(m.checks[k], checks...) } +// ReplaceCheckRuns overwrites the check runs for a ref. Used by the e2e mock +// controller so a follow-up seed call yields deterministic state. +func (m *MockClient) ReplaceCheckRuns(owner, repo, ref string, checks []CheckRun) { + m.mu.Lock() + defer m.mu.Unlock() + cp := make([]CheckRun, len(checks)) + copy(cp, checks) + m.checks[checkKey{owner, repo, ref}] = cp +} + +// ReplaceReviews overwrites the reviews for a PR. +func (m *MockClient) ReplaceReviews(owner, repo string, number int, reviews []PRReview) { + m.mu.Lock() + defer m.mu.Unlock() + cp := make([]PRReview, len(reviews)) + copy(cp, reviews) + m.reviews[prKey{owner, repo, number}] = cp +} + +// ReplaceComments overwrites the comments for a PR. +func (m *MockClient) ReplaceComments(owner, repo string, number int, comments []PRComment) { + m.mu.Lock() + defer m.mu.Unlock() + cp := make([]PRComment, len(comments)) + copy(cp, comments) + m.comments[prKey{owner, repo, number}] = cp +} + // AddPRFiles appends files for a PR. func (m *MockClient) AddPRFiles(owner, repo string, number int, files []PRFile) { m.mu.Lock() @@ -348,6 +391,8 @@ func (m *MockClient) Reset() { m.mu.Lock() defer m.mu.Unlock() m.user = mockDefaultUser + m.authenticated = true + m.authError = "" m.prs = make(map[prKey]*PR) m.prsByBranch = make(map[branchKey]*PR) m.orgs = nil diff --git a/apps/backend/internal/github/mock_controller.go b/apps/backend/internal/github/mock_controller.go index 18ebb41bfb..eee21bda92 100644 --- a/apps/backend/internal/github/mock_controller.go +++ b/apps/backend/internal/github/mock_controller.go @@ -1,6 +1,8 @@ package github import ( + "context" + "fmt" "net/http" "time" @@ -41,6 +43,8 @@ func (c *MockController) RegisterRoutes(router *gin.Engine) { api.POST("/commits", c.addPRCommits) api.POST("/branches", c.addBranches) api.POST("/task-prs", c.associateTaskPR) + api.POST("/pr-feedback", c.seedPRFeedback) + api.PUT("/auth-health", c.setAuthHealth) api.DELETE("/reset", c.reset) } @@ -188,25 +192,39 @@ func (c *MockController) addBranches(ctx *gin.Context) { ctx.JSON(http.StatusOK, gin.H{"added": len(req.Branches)}) } -// associateTaskPR directly creates a github_task_prs record for E2E testing. +// associateTaskPRRequest is the JSON body for the mock-controller's +// associateTaskPR endpoint. Pointer fields are optional — leave them nil +// to skip the corresponding TaskPR column update. +type associateTaskPRRequest struct { + TaskID string `json:"task_id"` + Owner string `json:"owner"` + Repo string `json:"repo"` + PRNumber int `json:"pr_number"` + PRURL string `json:"pr_url"` + PRTitle string `json:"pr_title"` + HeadBranch string `json:"head_branch"` + BaseBranch string `json:"base_branch"` + AuthorLogin string `json:"author_login"` + State string `json:"state"` + ReviewState string `json:"review_state"` + ChecksState string `json:"checks_state"` + MergeableState string `json:"mergeable_state"` + Additions int `json:"additions"` + Deletions int `json:"deletions"` + ReviewCount *int `json:"review_count,omitempty"` + PendingReviewCount *int `json:"pending_review_count,omitempty"` + RequiredReviews *int `json:"required_reviews,omitempty"` + ChecksTotal *int `json:"checks_total,omitempty"` + ChecksPassing *int `json:"checks_passing,omitempty"` + UnresolvedReviewThreads *int `json:"unresolved_review_threads,omitempty"` +} + +// associateTaskPR directly creates (or replaces) a github_task_prs record for +// E2E testing. Repeating the call with the same (task_id, owner, repo, +// pr_number) updates the row so tests can drive the live-refresh +// scenario by re-POSTing with new aggregate counts. func (c *MockController) associateTaskPR(ctx *gin.Context) { - var req struct { - TaskID string `json:"task_id"` - Owner string `json:"owner"` - Repo string `json:"repo"` - PRNumber int `json:"pr_number"` - PRURL string `json:"pr_url"` - PRTitle string `json:"pr_title"` - HeadBranch string `json:"head_branch"` - BaseBranch string `json:"base_branch"` - AuthorLogin string `json:"author_login"` - State string `json:"state"` - ReviewState string `json:"review_state"` - ChecksState string `json:"checks_state"` - MergeableState string `json:"mergeable_state"` - Additions int `json:"additions"` - Deletions int `json:"deletions"` - } + var req associateTaskPRRequest if err := ctx.ShouldBindJSON(&req); err != nil { ctx.JSON(http.StatusBadRequest, gin.H{"error": "invalid payload"}) return @@ -215,6 +233,26 @@ func (c *MockController) associateTaskPR(ctx *gin.Context) { req.State = defaultPRState } now := time.Now().UTC() + tp := buildTaskPRFromRequest(&req, now) + if err := c.store.ReplaceTaskPR(ctx.Request.Context(), tp); err != nil { + ctx.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) + return + } + c.ensureMockPRForRequest(ctx.Request.Context(), &req, now) + // Publish the event so the frontend Zustand store picks up the new PR + // without requiring a page reload — mirrors real AssociatePRWithTask. + if c.eventBus != nil { + event := bus.NewEvent(events.GitHubTaskPRUpdated, "github", tp) + if err := c.eventBus.Publish(ctx.Request.Context(), events.GitHubTaskPRUpdated, event); err != nil { + c.logger.Debug("mock: failed to publish task PR updated event", zap.Error(err)) + } + } + ctx.JSON(http.StatusCreated, tp) +} + +// buildTaskPRFromRequest copies the required fields from the JSON body into +// a TaskPR and applies the optional pointer fields when present. +func buildTaskPRFromRequest(req *associateTaskPRRequest, now time.Time) *TaskPR { tp := &TaskPR{ TaskID: req.TaskID, Owner: req.Owner, @@ -233,19 +271,122 @@ func (c *MockController) associateTaskPR(ctx *gin.Context) { Deletions: req.Deletions, CreatedAt: now, } - if err := c.store.CreateTaskPR(ctx.Request.Context(), tp); err != nil { - ctx.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) + if req.ReviewCount != nil { + tp.ReviewCount = *req.ReviewCount + } + if req.PendingReviewCount != nil { + tp.PendingReviewCount = *req.PendingReviewCount + } + if req.RequiredReviews != nil { + v := *req.RequiredReviews + tp.RequiredReviews = &v + } + if req.ChecksTotal != nil { + tp.ChecksTotal = *req.ChecksTotal + } + if req.ChecksPassing != nil { + tp.ChecksPassing = *req.ChecksPassing + } + if req.UnresolvedReviewThreads != nil { + tp.UnresolvedReviewThreads = *req.UnresolvedReviewThreads + } + return tp +} + +// ensureMockPRForRequest seeds a synthetic PR in the mock client so the lazy +// PRFeedback path can resolve check runs by HeadSHA. Skipped when the PR was +// already seeded explicitly via addPRs (tests that pin a head_sha for +// matching check_runs must not have it overwritten). +func (c *MockController) ensureMockPRForRequest(ctx context.Context, req *associateTaskPRRequest, now time.Time) { + if existing, _ := c.mock.GetPR(ctx, req.Owner, req.Repo, req.PRNumber); existing != nil { return } - // Publish the event so the frontend Zustand store picks up the new PR - // without requiring a page reload — mirrors real AssociatePRWithTask. - if c.eventBus != nil { - event := bus.NewEvent(events.GitHubTaskPRUpdated, "github", tp) - if err := c.eventBus.Publish(ctx.Request.Context(), events.GitHubTaskPRUpdated, event); err != nil { - c.logger.Debug("mock: failed to publish task PR updated event", zap.Error(err)) - } + c.mock.AddPR(&PR{ + Number: req.PRNumber, + Title: req.PRTitle, + URL: req.PRURL, + HTMLURL: req.PRURL, + State: req.State, + HeadBranch: req.HeadBranch, + HeadSHA: mockHeadSHA(req.Owner, req.Repo, req.PRNumber), + BaseBranch: req.BaseBranch, + AuthorLogin: req.AuthorLogin, + RepoOwner: req.Owner, + RepoName: req.Repo, + Additions: req.Additions, + Deletions: req.Deletions, + CreatedAt: now, + UpdatedAt: now, + }) +} + +// seedPRFeedback registers checks (and optionally reviews / comments) for a +// specific PR. The PR must already exist in the mock client (associateTaskPR +// synthesizes one). The check ref is the PR's HeadSHA so getPRFeedback +// resolves them via ListCheckRuns. +func (c *MockController) seedPRFeedback(ctx *gin.Context) { + var req struct { + Owner string `json:"owner"` + Repo string `json:"repo"` + PRNumber int `json:"pr_number"` + Checks []CheckRun `json:"checks"` + Reviews []PRReview `json:"reviews"` + Comments []PRComment `json:"comments"` } - ctx.JSON(http.StatusCreated, tp) + if err := ctx.ShouldBindJSON(&req); err != nil { + ctx.JSON(http.StatusBadRequest, gin.H{"error": "invalid payload"}) + return + } + if req.Owner == "" || req.Repo == "" || req.PRNumber == 0 { + ctx.JSON(http.StatusBadRequest, gin.H{"error": "owner, repo, pr_number are required"}) + return + } + headSHA := mockHeadSHA(req.Owner, req.Repo, req.PRNumber) + // If associateTaskPR ran first, an underlying PR row exists with this + // HeadSHA. Otherwise synthesize a minimal one so getPRFeedback's GetPR + // call doesn't fail. + if pr, err := c.mock.GetPR(ctx.Request.Context(), req.Owner, req.Repo, req.PRNumber); err != nil || pr == nil { + c.mock.AddPR(&PR{ + Number: req.PRNumber, + RepoOwner: req.Owner, + RepoName: req.Repo, + HeadSHA: headSHA, + CreatedAt: time.Now().UTC(), + UpdatedAt: time.Now().UTC(), + }) + } + // Replace prior seeded checks/reviews/comments so a follow-up call gives + // deterministic state; helpful for tests that drive a "then a check + // finishes" transition. + c.mock.ReplaceCheckRuns(req.Owner, req.Repo, headSHA, req.Checks) + c.mock.ReplaceReviews(req.Owner, req.Repo, req.PRNumber, req.Reviews) + c.mock.ReplaceComments(req.Owner, req.Repo, req.PRNumber, req.Comments) + ctx.JSON(http.StatusOK, gin.H{ + "checks": len(req.Checks), + "reviews": len(req.Reviews), + "comments": len(req.Comments), + }) +} + +// setAuthHealth toggles the mock client's authenticated state so e2e tests +// can drive the "Reconnect GitHub" branch in the popover. +func (c *MockController) setAuthHealth(ctx *gin.Context) { + var req struct { + Authenticated bool `json:"authenticated"` + Error string `json:"error"` + } + if err := ctx.ShouldBindJSON(&req); err != nil { + ctx.JSON(http.StatusBadRequest, gin.H{"error": "invalid payload"}) + return + } + c.mock.SetAuthHealth(req.Authenticated, req.Error) + ctx.JSON(http.StatusOK, gin.H{"authenticated": req.Authenticated}) +} + +// mockHeadSHA produces a deterministic synthetic head SHA for a (owner, repo, +// pr_number) tuple so test seed calls can compute the same key. +func mockHeadSHA(owner, repo string, prNumber int) string { + return fmt.Sprintf("mocksha-%s-%s-%d", owner, repo, prNumber) } func (c *MockController) reset(ctx *gin.Context) { diff --git a/apps/backend/internal/github/models.go b/apps/backend/internal/github/models.go index 433d65606b..5b86740698 100644 --- a/apps/backend/internal/github/models.go +++ b/apps/backend/internal/github/models.go @@ -92,8 +92,29 @@ type PRStatus struct { MergeableState string `json:"mergeable_state"` // "clean", "blocked", "behind", "dirty", "has_hooks", "unstable", "draft", "unknown", "" ReviewCount int `json:"review_count"` PendingReviewCount int `json:"pending_review_count"` + RequiredReviews *int `json:"required_reviews,omitempty"` // nil when no branch protection rule found ChecksTotal int `json:"checks_total"` ChecksPassing int `json:"checks_passing"` + // ChecksPopulated reports whether the sync path actually computed + // ChecksTotal / ChecksPassing. The batched GraphQL poller doesn't (it + // only carries the rollup state), so SyncTaskPR uses this flag to + // decide whether to overwrite the persisted counts. A value of true + // with both counts at 0 is a real "no checks" answer; a value of false + // means "I didn't look, keep what's there." + ChecksPopulated bool `json:"checks_populated,omitempty"` + UnresolvedReviewThreads int `json:"unresolved_review_threads"` + // UnresolvedReviewThreadsPopulated mirrors ChecksPopulated for the + // review-threads field. The REST path (getPRStatus) doesn't fetch + // review threads, so it leaves the field at zero; SyncTaskPR uses + // this flag to avoid clobbering a non-zero value set by the GraphQL + // path during a subsequent REST sync. + UnresolvedReviewThreadsPopulated bool `json:"unresolved_review_threads_populated,omitempty"` + // ReviewCountsPopulated covers ReviewCount + PendingReviewCount. Both + // REST and GraphQL paths compute these now, but historically only the + // REST path did, so without the guard a GraphQL poll would clobber + // the REST value back to 0 (the popover's "Approved (1)" turning + // into "Approved (0)" until a new REST call landed). + ReviewCountsPopulated bool `json:"review_counts_populated,omitempty"` } // PRSearchPage is a paginated slice of PR search results, with the total @@ -137,31 +158,37 @@ type PRWatch struct { // repository this PR belongs to (multi-repo tasks can have one PR per repo). // Empty for legacy rows persisted before multi-repo support. type TaskPR struct { - ID string `json:"id" db:"id"` - TaskID string `json:"task_id" db:"task_id"` - RepositoryID string `json:"repository_id,omitempty" db:"repository_id"` - Owner string `json:"owner" db:"owner"` - Repo string `json:"repo" db:"repo"` - PRNumber int `json:"pr_number" db:"pr_number"` - PRURL string `json:"pr_url" db:"pr_url"` - PRTitle string `json:"pr_title" db:"pr_title"` - HeadBranch string `json:"head_branch" db:"head_branch"` - BaseBranch string `json:"base_branch" db:"base_branch"` - AuthorLogin string `json:"author_login" db:"author_login"` - State string `json:"state" db:"state"` // open, closed, merged - ReviewState string `json:"review_state" db:"review_state"` // approved, changes_requested, pending, "" - ChecksState string `json:"checks_state" db:"checks_state"` // success, failure, pending, "" - MergeableState string `json:"mergeable_state" db:"mergeable_state"` // clean, blocked, behind, dirty, has_hooks, unstable, draft, unknown, "" - ReviewCount int `json:"review_count" db:"review_count"` - PendingReviewCount int `json:"pending_review_count" db:"pending_review_count"` - CommentCount int `json:"comment_count" db:"comment_count"` - Additions int `json:"additions" db:"additions"` - Deletions int `json:"deletions" db:"deletions"` - CreatedAt time.Time `json:"created_at" db:"created_at"` - MergedAt *time.Time `json:"merged_at,omitempty" db:"merged_at"` - ClosedAt *time.Time `json:"closed_at,omitempty" db:"closed_at"` - LastSyncedAt *time.Time `json:"last_synced_at,omitempty" db:"last_synced_at"` - UpdatedAt time.Time `json:"updated_at" db:"updated_at"` + ID string `json:"id" db:"id"` + TaskID string `json:"task_id" db:"task_id"` + RepositoryID string `json:"repository_id,omitempty" db:"repository_id"` + Owner string `json:"owner" db:"owner"` + Repo string `json:"repo" db:"repo"` + PRNumber int `json:"pr_number" db:"pr_number"` + PRURL string `json:"pr_url" db:"pr_url"` + PRTitle string `json:"pr_title" db:"pr_title"` + HeadBranch string `json:"head_branch" db:"head_branch"` + BaseBranch string `json:"base_branch" db:"base_branch"` + AuthorLogin string `json:"author_login" db:"author_login"` + State string `json:"state" db:"state"` // open, closed, merged + ReviewState string `json:"review_state" db:"review_state"` // approved, changes_requested, pending, "" + ChecksState string `json:"checks_state" db:"checks_state"` // success, failure, pending, "" + MergeableState string `json:"mergeable_state" db:"mergeable_state"` // clean, blocked, behind, dirty, has_hooks, unstable, draft, unknown, "" + ReviewCount int `json:"review_count" db:"review_count"` + PendingReviewCount int `json:"pending_review_count" db:"pending_review_count"` + // RequiredReviews is the branch protection's required_approving_review_count. + // Nil when no protection rule exists or the token lacks scope to read it. + RequiredReviews *int `json:"required_reviews,omitempty" db:"required_reviews"` + CommentCount int `json:"comment_count" db:"comment_count"` + UnresolvedReviewThreads int `json:"unresolved_review_threads" db:"unresolved_review_threads"` + ChecksTotal int `json:"checks_total" db:"checks_total"` + ChecksPassing int `json:"checks_passing" db:"checks_passing"` + Additions int `json:"additions" db:"additions"` + Deletions int `json:"deletions" db:"deletions"` + CreatedAt time.Time `json:"created_at" db:"created_at"` + MergedAt *time.Time `json:"merged_at,omitempty" db:"merged_at"` + ClosedAt *time.Time `json:"closed_at,omitempty" db:"closed_at"` + LastSyncedAt *time.Time `json:"last_synced_at,omitempty" db:"last_synced_at"` + UpdatedAt time.Time `json:"updated_at" db:"updated_at"` } // RepoFilter identifies a GitHub repository for review watch filtering. diff --git a/apps/backend/internal/github/pat_client.go b/apps/backend/internal/github/pat_client.go index 5383b21b38..be45ec0738 100644 --- a/apps/backend/internal/github/pat_client.go +++ b/apps/backend/internal/github/pat_client.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "encoding/json" + "errors" "fmt" "io" "net/http" @@ -551,6 +552,37 @@ func parseNextLink(header string) string { return "" } +// FetchBranchProtection looks up the branch protection rule on a repo's base +// branch via the REST API. A 404 is treated as "no rule configured" and a 403 +// is treated as "no rule we can see" (token lacks Administration: Read scope). +// Both return HasRule=false with no error so the cache stores the negative +// result and we don't burn rate-limit quota retrying every poll cycle. +// Other errors propagate so callers can decide whether to retry. +func (c *PATClient) FetchBranchProtection(ctx context.Context, owner, repo, branch string) (BranchProtection, error) { + endpoint := fmt.Sprintf("/repos/%s/%s/branches/%s/protection", owner, repo, branch) + var raw struct { + RequiredPullRequestReviews *struct { + RequiredApprovingReviewCount int `json:"required_approving_review_count"` + } `json:"required_pull_request_reviews"` + } + if err := c.get(ctx, endpoint, &raw); err != nil { + var apiErr *GitHubAPIError + if errors.As(err, &apiErr) { + if apiErr.StatusCode == http.StatusNotFound || apiErr.StatusCode == http.StatusForbidden { + return BranchProtection{HasRule: false}, nil + } + } + return BranchProtection{}, err + } + if raw.RequiredPullRequestReviews == nil { + return BranchProtection{HasRule: true}, nil + } + return BranchProtection{ + HasRule: true, + RequiredApprovingReviewCount: raw.RequiredPullRequestReviews.RequiredApprovingReviewCount, + }, nil +} + // patPR is the JSON shape from the GitHub REST API for PRs. type patPR struct { Number int `json:"number"` diff --git a/apps/backend/internal/github/pat_client_test.go b/apps/backend/internal/github/pat_client_test.go index 7902b8aa7a..a9e7605218 100644 --- a/apps/backend/internal/github/pat_client_test.go +++ b/apps/backend/internal/github/pat_client_test.go @@ -297,6 +297,80 @@ func TestPATClient_MarksExhaustedFromRateLimitBody(t *testing.T) { } } +func TestPATClient_FetchBranchProtection(t *testing.T) { + cases := []struct { + name string + status int + body string + wantHasRule bool + wantRequired int + wantErr bool + }{ + { + name: "200 with required reviews", + status: http.StatusOK, + body: `{"required_pull_request_reviews":{"required_approving_review_count":2}}`, + wantHasRule: true, + wantRequired: 2, + }, + { + name: "200 with rule but no required reviews block", + status: http.StatusOK, + body: `{"required_pull_request_reviews":null}`, + wantHasRule: true, + }, + { + name: "404 maps to no rule", + status: http.StatusNotFound, + body: `{"message":"Branch not protected"}`, + wantHasRule: false, + }, + { + name: "403 (no admin scope) maps to no rule", + status: http.StatusForbidden, + body: `{"message":"Resource not accessible by integration"}`, + wantHasRule: false, + }, + { + name: "500 propagates as error", + status: http.StatusInternalServerError, + body: `{"message":"server error"}`, + wantErr: true, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + wantPath := "/repos/o/r/branches/main/protection" + if r.URL.Path != wantPath { + t.Fatalf("path = %q, want %q", r.URL.Path, wantPath) + } + w.WriteHeader(tc.status) + _, _ = w.Write([]byte(tc.body)) + })) + t.Cleanup(srv.Close) + c := newPATClientPointingAt(t, srv.URL) + bp, err := c.FetchBranchProtection(context.Background(), "o", "r", "main") + if tc.wantErr { + if err == nil { + t.Fatal("want error, got nil") + } + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if bp.HasRule != tc.wantHasRule { + t.Fatalf("HasRule = %v, want %v", bp.HasRule, tc.wantHasRule) + } + if bp.RequiredApprovingReviewCount != tc.wantRequired { + t.Fatalf("RequiredApprovingReviewCount = %d, want %d", + bp.RequiredApprovingReviewCount, tc.wantRequired) + } + }) + } +} + // newPATClientPointingAt builds a PATClient whose underlying HTTP client // reroutes any github API URL to the given test server. func newPATClientPointingAt(t *testing.T, baseURL string) *PATClient { diff --git a/apps/backend/internal/github/service.go b/apps/backend/internal/github/service.go index 47e6a0b041..8a1cd638c9 100644 --- a/apps/backend/internal/github/service.go +++ b/apps/backend/internal/github/service.go @@ -71,21 +71,23 @@ type Service struct { taskEventSubs []bus.Subscription searchCache *ttlCache prStatusCache *ttlCache + protectionCache *branchProtectionCache rateTracker *RateTracker } // NewService creates a new GitHub service. func NewService(client Client, authMethod string, secrets SecretProvider, store *Store, eventBus bus.EventBus, log *logger.Logger) *Service { return &Service{ - client: client, - authMethod: authMethod, - secrets: secrets, - store: store, - eventBus: eventBus, - logger: log, - searchCache: newTTLCache(), - prStatusCache: newTTLCache(), - rateTracker: NewRateTracker(eventBus, log), + client: client, + authMethod: authMethod, + secrets: secrets, + store: store, + eventBus: eventBus, + logger: log, + searchCache: newTTLCache(), + prStatusCache: newTTLCache(), + protectionCache: newBranchProtectionCache(), + rateTracker: NewRateTracker(eventBus, log), } } @@ -753,6 +755,51 @@ func (s *Service) SyncTaskPR(ctx context.Context, taskID string, status *PRStatu return err } + // Some sync paths (notably the batched GraphQL poller) don't populate + // ChecksTotal / ChecksPassing — they only carry the rollup state. The + // caller sets status.ChecksPopulated=true when it actually counted + // checks; otherwise we preserve the persisted values so the popover + // doesn't flap to "0/0" between a rich REST sync and a lightweight + // GraphQL one. When the populated counter says 0/0 it really is 0/0 + // (e.g. all workflows were removed from the PR), so we honor it. + nextChecksTotal, nextChecksPassing := tp.ChecksTotal, tp.ChecksPassing + if status.ChecksPopulated { + nextChecksTotal = status.ChecksTotal + nextChecksPassing = status.ChecksPassing + } + // Same Populated/preserve dance for unresolved review threads — the + // REST path doesn't fetch them, so blindly writing status.UnresolvedReviewThreads + // would clobber the non-zero value set by the GraphQL path on every poll. + nextUnresolved := tp.UnresolvedReviewThreads + if status.UnresolvedReviewThreadsPopulated { + nextUnresolved = status.UnresolvedReviewThreads + } + // Review counts: only overwrite when the caller actually computed them. + // Both REST and GraphQL paths now populate these, but a partial sync + // path that doesn't would otherwise reset the popover's "Approved (N)" + // to zero. + nextReviewCount, nextPendingReviewCount := tp.ReviewCount, tp.PendingReviewCount + if status.ReviewCountsPopulated { + nextReviewCount = status.ReviewCount + nextPendingReviewCount = status.PendingReviewCount + } + // PRs can be retargeted to a different base branch; pick up the new + // branch from status.PR before resolving branch-protection so we don't + // indefinitely surface the wrong rule. + nextBaseBranch := tp.BaseBranch + if status.PR.BaseBranch != "" && status.PR.BaseBranch != tp.BaseBranch { + nextBaseBranch = status.PR.BaseBranch + } + // RequiredReviews comes from branch protection, fetched separately. + // Treat nil as "unknown — don't touch"; only write when the caller has it + // or our cache resolves the rule for this base branch. + nextRequiredReviews := tp.RequiredReviews + if status.RequiredReviews != nil { + nextRequiredReviews = status.RequiredReviews + } else if fetched := s.fetchRequiredReviews(ctx, tp.Owner, tp.Repo, nextBaseBranch); fetched != nil { + nextRequiredReviews = fetched + } + changed := tp.State != status.PR.State || tp.PRTitle != status.PR.Title || tp.Additions != status.PR.Additions || @@ -760,8 +807,13 @@ func (s *Service) SyncTaskPR(ctx context.Context, taskID string, status *PRStatu tp.ReviewState != status.ReviewState || tp.ChecksState != status.ChecksState || tp.MergeableState != status.MergeableState || - tp.ReviewCount != status.ReviewCount || - tp.PendingReviewCount != status.PendingReviewCount || + tp.ReviewCount != nextReviewCount || + tp.PendingReviewCount != nextPendingReviewCount || + !intPtrEqual(tp.RequiredReviews, nextRequiredReviews) || + tp.ChecksTotal != nextChecksTotal || + tp.ChecksPassing != nextChecksPassing || + tp.UnresolvedReviewThreads != nextUnresolved || + tp.BaseBranch != nextBaseBranch || !timeEqual(tp.MergedAt, status.PR.MergedAt) || !timeEqual(tp.ClosedAt, status.PR.ClosedAt) @@ -774,8 +826,13 @@ func (s *Service) SyncTaskPR(ctx context.Context, taskID string, status *PRStatu tp.ReviewState = status.ReviewState tp.ChecksState = status.ChecksState tp.MergeableState = status.MergeableState - tp.ReviewCount = status.ReviewCount - tp.PendingReviewCount = status.PendingReviewCount + tp.ReviewCount = nextReviewCount + tp.PendingReviewCount = nextPendingReviewCount + tp.RequiredReviews = nextRequiredReviews + tp.ChecksTotal = nextChecksTotal + tp.ChecksPassing = nextChecksPassing + tp.UnresolvedReviewThreads = nextUnresolved + tp.BaseBranch = nextBaseBranch // CommentCount is no longer updated from polling -- only refreshed on-demand now := time.Now().UTC() tp.LastSyncedAt = &now @@ -804,6 +861,17 @@ func timeEqual(a, b *time.Time) bool { return a.Equal(*b) } +// intPtrEqual compares two nullable int pointers for equality. +func intPtrEqual(a, b *int) bool { + if a == nil && b == nil { + return true + } + if a == nil || b == nil { + return false + } + return *a == *b +} + // --- PR info and feedback (live) --- // GetPR fetches basic PR details from GitHub. diff --git a/apps/backend/internal/github/service_test.go b/apps/backend/internal/github/service_test.go index 4cc69bf4e3..6f32e8e078 100644 --- a/apps/backend/internal/github/service_test.go +++ b/apps/backend/internal/github/service_test.go @@ -910,6 +910,253 @@ func TestSyncTaskPR_PublishesEventOnMergeableStateChange(t *testing.T) { } } +// TestSyncTaskPR_ChecksPopulated_PreservesOnLightweightSync verifies the +// "preserve when batch sync didn't populate" path: a PRStatus with +// ChecksPopulated=false must NOT clobber the persisted ChecksTotal / +// ChecksPassing, even when status carries 0/0. +func TestSyncTaskPR_ChecksPopulated_PreservesOnLightweightSync(t *testing.T) { + svc, store, _ := setupSyncTest(t) + ctx := context.Background() + if err := store.CreateTaskPR(ctx, &TaskPR{ + TaskID: "t1", + Owner: "owner", + Repo: "repo", + PRNumber: 1, + PRURL: "https://github.com/owner/repo/pull/1", + HeadBranch: "feat", + BaseBranch: "main", + State: "open", + ChecksTotal: 10, + ChecksPassing: 7, + }); err != nil { + t.Fatalf("create: %v", err) + } + status := &PRStatus{ + PR: &PR{Number: 1, State: "open", RepoOwner: "owner", RepoName: "repo"}, + ChecksTotal: 0, + ChecksPassing: 0, + ChecksPopulated: false, // batched GraphQL path: didn't count + } + if err := svc.SyncTaskPR(ctx, "t1", status); err != nil { + t.Fatalf("sync: %v", err) + } + stored, err := store.GetTaskPR(ctx, "t1") + if err != nil { + t.Fatalf("get: %v", err) + } + if stored.ChecksTotal != 10 || stored.ChecksPassing != 7 { + t.Fatalf("counts clobbered: total=%d passing=%d (want 10/7)", stored.ChecksTotal, stored.ChecksPassing) + } +} + +// TestSyncTaskPR_ChecksPopulated_AcceptsRealZero verifies the design fix: +// when the rich sync path explicitly reports 0/0 (workflows removed from +// the PR), we DO honor the new value instead of clinging to stale totals. +func TestSyncTaskPR_ChecksPopulated_AcceptsRealZero(t *testing.T) { + svc, store, _ := setupSyncTest(t) + ctx := context.Background() + if err := store.CreateTaskPR(ctx, &TaskPR{ + TaskID: "t1", Owner: "o", Repo: "r", PRNumber: 1, PRURL: "u", + HeadBranch: "feat", BaseBranch: "main", State: "open", + ChecksTotal: 5, ChecksPassing: 5, + }); err != nil { + t.Fatalf("create: %v", err) + } + status := &PRStatus{ + PR: &PR{Number: 1, State: "open", RepoOwner: "o", RepoName: "r"}, + ChecksTotal: 0, + ChecksPassing: 0, + ChecksPopulated: true, // REST path: counted to zero + } + if err := svc.SyncTaskPR(ctx, "t1", status); err != nil { + t.Fatalf("sync: %v", err) + } + stored, _ := store.GetTaskPR(ctx, "t1") + if stored.ChecksTotal != 0 || stored.ChecksPassing != 0 { + t.Fatalf("real-zero not honored: total=%d passing=%d", stored.ChecksTotal, stored.ChecksPassing) + } +} + +// TestSyncTaskPR_ReviewCounts_PreservesOnLightweightSync covers the same +// preserve-on-not-populated dance for ReviewCount/PendingReviewCount as the +// checks/threads variants. A partial sync path (status.ReviewCountsPopulated +// = false) must not reset the popover's "Approved (N)" line to zero. +func TestSyncTaskPR_ReviewCounts_PreservesOnLightweightSync(t *testing.T) { + svc, store, eb := setupSyncTest(t) + ctx := context.Background() + if err := store.CreateTaskPR(ctx, &TaskPR{ + TaskID: "t1", Owner: "o", Repo: "r", PRNumber: 1, PRURL: "u", + HeadBranch: "feat", BaseBranch: "main", State: "open", + ReviewCount: 2, PendingReviewCount: 1, + }); err != nil { + t.Fatalf("create: %v", err) + } + status := &PRStatus{ + PR: &PR{Number: 1, State: "open", RepoOwner: "o", RepoName: "r"}, + ReviewCount: 0, + PendingReviewCount: 0, + ReviewCountsPopulated: false, + } + if err := svc.SyncTaskPR(ctx, "t1", status); err != nil { + t.Fatalf("sync: %v", err) + } + stored, _ := store.GetTaskPR(ctx, "t1") + if stored.ReviewCount != 2 || stored.PendingReviewCount != 1 { + t.Fatalf("review counts clobbered: review=%d pending=%d (want 2/1)", + stored.ReviewCount, stored.PendingReviewCount) + } + if got := eb.publishedCount(); got != 0 { + t.Fatalf("expected no event when nothing changed, got %d", got) + } +} + +// TestSyncTaskPR_ReviewCounts_AcceptsRealZero is the symmetric case: when the +// caller actually counted reviewers and reports zero, we honor it (e.g. all +// approvals dismissed) instead of clinging to the previous non-zero values. +func TestSyncTaskPR_ReviewCounts_AcceptsRealZero(t *testing.T) { + svc, store, _ := setupSyncTest(t) + ctx := context.Background() + if err := store.CreateTaskPR(ctx, &TaskPR{ + TaskID: "t1", Owner: "o", Repo: "r", PRNumber: 1, PRURL: "u", + HeadBranch: "feat", BaseBranch: "main", State: "open", + ReviewCount: 2, PendingReviewCount: 1, + }); err != nil { + t.Fatalf("create: %v", err) + } + status := &PRStatus{ + PR: &PR{Number: 1, State: "open", RepoOwner: "o", RepoName: "r"}, + ReviewCount: 0, + PendingReviewCount: 0, + ReviewCountsPopulated: true, + } + if err := svc.SyncTaskPR(ctx, "t1", status); err != nil { + t.Fatalf("sync: %v", err) + } + stored, _ := store.GetTaskPR(ctx, "t1") + if stored.ReviewCount != 0 || stored.PendingReviewCount != 0 { + t.Fatalf("real-zero not honored: review=%d pending=%d", + stored.ReviewCount, stored.PendingReviewCount) + } +} + +// TestSyncTaskPR_UnresolvedThreads_PreservesOnLightweightSync verifies the +// REST single-PR poller (which doesn't fetch review threads) does NOT +// clobber the value set by the GraphQL batched poller. Without the +// UnresolvedReviewThreadsPopulated guard, every REST sync would flip the +// count to 0 and emit a spurious github.task_pr.updated event. +func TestSyncTaskPR_UnresolvedThreads_PreservesOnLightweightSync(t *testing.T) { + svc, store, eb := setupSyncTest(t) + ctx := context.Background() + if err := store.CreateTaskPR(ctx, &TaskPR{ + TaskID: "t1", Owner: "o", Repo: "r", PRNumber: 1, PRURL: "u", + HeadBranch: "feat", BaseBranch: "main", State: "open", + UnresolvedReviewThreads: 3, + }); err != nil { + t.Fatalf("create: %v", err) + } + status := &PRStatus{ + PR: &PR{Number: 1, State: "open", RepoOwner: "o", RepoName: "r"}, + UnresolvedReviewThreads: 0, + UnresolvedReviewThreadsPopulated: false, // REST path: didn't look + } + if err := svc.SyncTaskPR(ctx, "t1", status); err != nil { + t.Fatalf("sync: %v", err) + } + stored, _ := store.GetTaskPR(ctx, "t1") + if stored.UnresolvedReviewThreads != 3 { + t.Fatalf("threads clobbered: got %d, want 3", stored.UnresolvedReviewThreads) + } + if got := eb.publishedCount(); got != 0 { + t.Fatalf("expected no event when nothing changed, got %d", got) + } +} + +// TestSyncTaskPR_BaseBranchRetarget covers the case where a PR is retargeted +// to a different base branch upstream — the new branch must be persisted so +// future branch-protection lookups key off the right value. +func TestSyncTaskPR_BaseBranchRetarget(t *testing.T) { + svc, store, _ := setupSyncTest(t) + ctx := context.Background() + if err := store.CreateTaskPR(ctx, &TaskPR{ + TaskID: "t1", Owner: "o", Repo: "r", PRNumber: 1, PRURL: "u", + HeadBranch: "feat", BaseBranch: "main", State: "open", + }); err != nil { + t.Fatalf("create: %v", err) + } + status := &PRStatus{ + PR: &PR{ + Number: 1, State: "open", RepoOwner: "o", RepoName: "r", + BaseBranch: "develop", + }, + } + if err := svc.SyncTaskPR(ctx, "t1", status); err != nil { + t.Fatalf("sync: %v", err) + } + stored, _ := store.GetTaskPR(ctx, "t1") + if stored.BaseBranch != "develop" { + t.Fatalf("base branch not updated: got %q want develop", stored.BaseBranch) + } +} + +// TestSyncTaskPR_NewFieldsPublishEvent guards that a change to any of the +// CI-popover-relevant fields (UnresolvedReviewThreads, RequiredReviews, +// ChecksTotal, ChecksPassing) triggers a github.task_pr.updated event so the +// frontend popover refreshes via WS. +func TestSyncTaskPR_NewFieldsPublishEvent(t *testing.T) { + required2 := 2 + cases := []struct { + name string + init TaskPR + status PRStatus + }{ + { + name: "unresolved threads change", + init: TaskPR{TaskID: "t1", Owner: "o", Repo: "r", PRNumber: 1, PRURL: "u", + HeadBranch: "feat", BaseBranch: "main", State: "open"}, + status: PRStatus{ + PR: &PR{Number: 1, State: "open", RepoOwner: "o", RepoName: "r"}, + UnresolvedReviewThreads: 3, + UnresolvedReviewThreadsPopulated: true, + }, + }, + { + name: "required reviews change", + init: TaskPR{TaskID: "t1", Owner: "o", Repo: "r", PRNumber: 1, PRURL: "u", + HeadBranch: "feat", BaseBranch: "main", State: "open"}, + status: PRStatus{ + PR: &PR{Number: 1, State: "open", RepoOwner: "o", RepoName: "r"}, + RequiredReviews: &required2, + }, + }, + { + name: "checks total change", + init: TaskPR{TaskID: "t1", Owner: "o", Repo: "r", PRNumber: 1, PRURL: "u", + HeadBranch: "feat", BaseBranch: "main", State: "open"}, + status: PRStatus{ + PR: &PR{Number: 1, State: "open", RepoOwner: "o", RepoName: "r"}, + ChecksTotal: 5, + ChecksPassing: 3, + ChecksPopulated: true, + }, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + svc, store, eb := setupSyncTest(t) + ctx := context.Background() + if err := store.CreateTaskPR(ctx, &tc.init); err != nil { + t.Fatalf("create: %v", err) + } + if err := svc.SyncTaskPR(ctx, "t1", &tc.status); err != nil { + t.Fatalf("sync: %v", err) + } + if got := eb.publishedCount(); got != 1 { + t.Fatalf("expected 1 event, got %d", got) + } + }) + } +} + func TestTimeEqual(t *testing.T) { now := time.Now() later := now.Add(time.Second) diff --git a/apps/backend/internal/github/store.go b/apps/backend/internal/github/store.go index 151de13bfe..8991bf4afa 100644 --- a/apps/backend/internal/github/store.go +++ b/apps/backend/internal/github/store.go @@ -74,7 +74,11 @@ const createTablesSQL = ` mergeable_state TEXT NOT NULL DEFAULT '', review_count INTEGER DEFAULT 0, pending_review_count INTEGER DEFAULT 0, + required_reviews INTEGER, comment_count INTEGER DEFAULT 0, + unresolved_review_threads INTEGER DEFAULT 0, + checks_total INTEGER DEFAULT 0, + checks_passing INTEGER DEFAULT 0, additions INTEGER DEFAULT 0, deletions INTEGER DEFAULT 0, created_at DATETIME NOT NULL, @@ -163,6 +167,13 @@ func (s *Store) initSchema() error { // Phase 4 (multi-repo): per-repo PR association on github_task_prs. _, _ = s.db.Exec(`ALTER TABLE github_task_prs ADD COLUMN repository_id TEXT NOT NULL DEFAULT ''`) _, _ = s.db.Exec(`ALTER TABLE github_pr_watches ADD COLUMN repository_id TEXT NOT NULL DEFAULT ''`) + // CI popover: aggregate counts + branch protection's required_approving_review_count + // + unresolved review-threads, surfaced in the PR top-bar hover popover so the + // frontend can render the counts row without a second round-trip. + _, _ = s.db.Exec(`ALTER TABLE github_task_prs ADD COLUMN required_reviews INTEGER`) + _, _ = s.db.Exec(`ALTER TABLE github_task_prs ADD COLUMN unresolved_review_threads INTEGER DEFAULT 0`) + _, _ = s.db.Exec(`ALTER TABLE github_task_prs ADD COLUMN checks_total INTEGER DEFAULT 0`) + _, _ = s.db.Exec(`ALTER TABLE github_task_prs ADD COLUMN checks_passing INTEGER DEFAULT 0`) if err := s.migratePRTablesForMultiRepo(); err != nil { return fmt.Errorf("migrate PR tables for multi-repo: %w", err) } @@ -360,7 +371,11 @@ func (s *Store) migratePRTablesForMultiRepo() error { mergeable_state TEXT NOT NULL DEFAULT '', review_count INTEGER DEFAULT 0, pending_review_count INTEGER DEFAULT 0, + required_reviews INTEGER, comment_count INTEGER DEFAULT 0, + unresolved_review_threads INTEGER DEFAULT 0, + checks_total INTEGER DEFAULT 0, + checks_passing INTEGER DEFAULT 0, additions INTEGER DEFAULT 0, deletions INTEGER DEFAULT 0, created_at DATETIME NOT NULL, @@ -580,11 +595,13 @@ func (s *Store) CreateTaskPR(ctx context.Context, tp *TaskPR) error { tp.UpdatedAt = now _, err := s.db.ExecContext(ctx, ` INSERT INTO github_task_prs (id, task_id, repository_id, owner, repo, pr_number, pr_url, pr_title, head_branch, base_branch, author_login, - state, review_state, checks_state, mergeable_state, review_count, pending_review_count, comment_count, additions, deletions, + state, review_state, checks_state, mergeable_state, review_count, pending_review_count, required_reviews, comment_count, + unresolved_review_threads, checks_total, checks_passing, additions, deletions, created_at, merged_at, closed_at, last_synced_at, updated_at) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, tp.ID, tp.TaskID, tp.RepositoryID, tp.Owner, tp.Repo, tp.PRNumber, tp.PRURL, tp.PRTitle, tp.HeadBranch, tp.BaseBranch, tp.AuthorLogin, - tp.State, tp.ReviewState, tp.ChecksState, tp.MergeableState, tp.ReviewCount, tp.PendingReviewCount, tp.CommentCount, tp.Additions, tp.Deletions, + tp.State, tp.ReviewState, tp.ChecksState, tp.MergeableState, tp.ReviewCount, tp.PendingReviewCount, tp.RequiredReviews, tp.CommentCount, + tp.UnresolvedReviewThreads, tp.ChecksTotal, tp.ChecksPassing, tp.Additions, tp.Deletions, tp.CreatedAt, tp.MergedAt, tp.ClosedAt, tp.LastSyncedAt, tp.UpdatedAt) return err } @@ -713,11 +730,13 @@ func (s *Store) ReplaceTaskPR(ctx context.Context, tp *TaskPR) error { } if _, err := tx.ExecContext(ctx, ` INSERT INTO github_task_prs (id, task_id, repository_id, owner, repo, pr_number, pr_url, pr_title, head_branch, base_branch, author_login, - state, review_state, checks_state, mergeable_state, review_count, pending_review_count, comment_count, additions, deletions, + state, review_state, checks_state, mergeable_state, review_count, pending_review_count, required_reviews, comment_count, + unresolved_review_threads, checks_total, checks_passing, additions, deletions, created_at, merged_at, closed_at, last_synced_at, updated_at) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, tp.ID, tp.TaskID, tp.RepositoryID, tp.Owner, tp.Repo, tp.PRNumber, tp.PRURL, tp.PRTitle, tp.HeadBranch, tp.BaseBranch, tp.AuthorLogin, - tp.State, tp.ReviewState, tp.ChecksState, tp.MergeableState, tp.ReviewCount, tp.PendingReviewCount, tp.CommentCount, tp.Additions, tp.Deletions, + tp.State, tp.ReviewState, tp.ChecksState, tp.MergeableState, tp.ReviewCount, tp.PendingReviewCount, tp.RequiredReviews, tp.CommentCount, + tp.UnresolvedReviewThreads, tp.ChecksTotal, tp.ChecksPassing, tp.Additions, tp.Deletions, tp.CreatedAt, tp.MergedAt, tp.ClosedAt, tp.LastSyncedAt, tp.UpdatedAt); err != nil { return err } @@ -729,13 +748,15 @@ func (s *Store) UpdateTaskPR(ctx context.Context, tp *TaskPR) error { tp.UpdatedAt = time.Now().UTC() _, err := s.db.ExecContext(ctx, ` UPDATE github_task_prs SET state = ?, review_state = ?, checks_state = ?, mergeable_state = ?, - review_count = ?, pending_review_count = ?, comment_count = ?, - additions = ?, deletions = ?, pr_title = ?, + review_count = ?, pending_review_count = ?, required_reviews = ?, comment_count = ?, + unresolved_review_threads = ?, checks_total = ?, checks_passing = ?, + additions = ?, deletions = ?, pr_title = ?, base_branch = ?, merged_at = ?, closed_at = ?, last_synced_at = ?, updated_at = ? WHERE id = ?`, tp.State, tp.ReviewState, tp.ChecksState, tp.MergeableState, - tp.ReviewCount, tp.PendingReviewCount, tp.CommentCount, - tp.Additions, tp.Deletions, tp.PRTitle, + tp.ReviewCount, tp.PendingReviewCount, tp.RequiredReviews, tp.CommentCount, + tp.UnresolvedReviewThreads, tp.ChecksTotal, tp.ChecksPassing, + tp.Additions, tp.Deletions, tp.PRTitle, tp.BaseBranch, tp.MergedAt, tp.ClosedAt, tp.LastSyncedAt, tp.UpdatedAt, tp.ID) return err } diff --git a/apps/web/components/github/pr-ci-popover.tsx b/apps/web/components/github/pr-ci-popover.tsx new file mode 100644 index 0000000000..01c2640776 --- /dev/null +++ b/apps/web/components/github/pr-ci-popover.tsx @@ -0,0 +1,569 @@ +"use client"; + +import { useCallback, useEffect, useMemo, useState, type ReactNode } from "react"; +import { + IconCheck, + IconCircleCheck, + IconCircleDot, + IconCircleX, + IconExternalLink, + IconGitPullRequest, + IconMessageCircle, + IconPlus, +} from "@tabler/icons-react"; +import { Button } from "@kandev/ui/button"; +import { useToast } from "@/components/toast-provider"; +import { useAppStore } from "@/components/state-provider"; +import { useCommentsStore } from "@/lib/state/slices/comments"; +import type { PRFeedbackComment } from "@/lib/state/slices/comments"; +import { useGitHubStatus } from "@/hooks/domains/github/use-github-status"; +import { usePRCIPopover } from "@/hooks/domains/github/use-pr-ci-popover"; +import { + bucketCheck, + bucketCheckCounts, + groupChecksByWorkflow, + type CheckBucket, + type WorkflowGroup, +} from "@/lib/github/check-buckets"; +import type { CheckRun, TaskPR } from "@/lib/types/github"; + +type CountsView = { + passed: number; + inProgress: number; + failed: number; +}; + +const CHECK_GROUP_ORDER: CheckBucket[] = ["passed", "in_progress", "failed"]; + +function countForBucket(counts: CountsView, kind: CheckBucket): number { + if (kind === "passed") return counts.passed; + if (kind === "in_progress") return counts.inProgress; + return counts.failed; +} + +function deriveAggregateCounts(pr: TaskPR): CountsView { + // Pre-load: if checks_total is populated, derive a coarse split using + // checks_state to attribute the "non-passing" remainder. This is good + // enough for the instant render — the lazy PRFeedback fetch refines. + const total = pr.checks_total; + const passing = pr.checks_passing; + const remaining = Math.max(0, total - passing); + if (pr.checks_state === "failure") { + return { passed: passing, failed: remaining, inProgress: 0 }; + } + if (pr.checks_state === "pending") { + return { passed: passing, failed: 0, inProgress: remaining }; + } + if (pr.checks_state === "success") { + return { passed: total, failed: 0, inProgress: 0 }; + } + return { passed: passing, failed: 0, inProgress: remaining }; +} + +function PRCIPopoverHeader({ pr }: { pr: TaskPR }) { + const checksUrl = `${pr.pr_url}/checks`; + return ( +
+ CI status + e.stopPropagation()} + > + + +
+ ); +} + +function CheckGroupIcon({ kind }: { kind: CheckBucket }) { + if (kind === "passed") return ; + if (kind === "in_progress") + return ; + return ; +} + +function bucketLabel(kind: CheckBucket): string { + if (kind === "passed") return "Passed"; + if (kind === "in_progress") return "In progress"; + return "Failed"; +} + +function CheckGroupHeader({ kind, count }: { kind: CheckBucket; count: number }) { + const label = bucketLabel(kind); + return ( +
+
+ + {label} +
+ + {count} + +
+ ); +} + +function PRWorkflowRow({ + group, + onAddAsContext, +}: { + group: WorkflowGroup; + onAddAsContext: ((message: string) => void) | null; +}) { + // For an in-progress workflow, "0/1 ran" reads as "nothing finished" + // and confuses people: did it start? It's clearer to just report how + // many jobs are running. (Failed workflows go to a different bucket, + // so an in_progress group never has failed jobs.) + const badge = + group.bucket === "in_progress" + ? `${group.inProgress} running` + : `${group.passed}/${group.total} passed`; + return ( +
{ + if (group.htmlUrl) window.open(group.htmlUrl, "_blank", "noopener,noreferrer"); + }} + > + + {group.workflow} + + {badge} + {group.htmlUrl && ( + + )} + {group.bucket === "failed" && onAddAsContext && ( + + )} +
+ ); +} + +function buildWorkflowMessage(group: WorkflowGroup): string { + const failed = group.jobs.filter((j) => bucketCheck(j) === "failed"); + const lines: string[] = [ + `### Workflow **${group.workflow}** has ${failed.length} failing job${failed.length !== 1 ? "s" : ""}.`, + "", + ]; + for (const job of failed) { + lines.push(`- **${job.name}** — ${job.conclusion || job.status}`); + if (job.output) lines.push(` ${job.output}`); + if (job.html_url) lines.push(` ${job.html_url}`); + } + lines.push("", "Please investigate and fix."); + return lines.join("\n"); +} + +function PRCheckGroup({ + kind, + count, + workflows, + isLoading, + onAddAsContext, +}: { + kind: CheckBucket; + count: number; + workflows?: WorkflowGroup[]; + isLoading: boolean; + onAddAsContext: ((message: string) => void) | null; +}) { + if (count <= 0 && (!workflows || workflows.length === 0)) return null; + const showRows = kind !== "passed"; + const hasWorkflows = !!workflows && workflows.length > 0; + return ( +
+ + {showRows && ( +
+ {hasWorkflows && + workflows!.map((g) => ( + + ))} + {!hasWorkflows && isLoading + ? Array.from({ length: Math.min(2, count || 1) }).map((_, i) => ( +
+ )) + : null} +
+ )} +
+ ); +} + +/** + * Stacked progress bar mirroring TodoIndicator's bar but split into three + * segments — green (passed), yellow (in_progress), red (failed) — each + * sized proportionally to its count. The leading "pass rate" label gives + * the same at-a-glance number TodoIndicator shows ("6/10 (60%)"). + * + * Skipped/cancelled-but-not-failed jobs aren't counted toward the total + * (they never enter any bucket), so the bar always reflects the actionable + * picture: how close are we to "all green". + */ +function PRChecksProgressBar({ counts }: { counts: CountsView }) { + const total = counts.passed + counts.inProgress + counts.failed; + if (total === 0) return null; + const pct = (n: number) => (n / total) * 100; + const passedPct = pct(counts.passed); + const inProgressPct = pct(counts.inProgress); + const failedPct = pct(counts.failed); + const completePct = Math.round(passedPct); + return ( +
+
+ Pass rate + + {counts.passed}/{total} ({completePct}%) + +
+
+ {passedPct > 0 && ( +
+ )} + {inProgressPct > 0 && ( +
+ )} + {failedPct > 0 && ( +
+ )} +
+
+ ); +} + +function PRChecksSection({ + pr, + feedback, + isFetching, + onAddAsContext, +}: { + pr: TaskPR; + feedback: { checks?: CheckRun[] } | null; + isFetching: boolean; + onAddAsContext: ((message: string) => void) | null; +}) { + const aggregateCounts = useMemo(() => deriveAggregateCounts(pr), [pr]); + + const { precise, byBucket } = useMemo(() => { + // Treat empty `feedback.checks` the same as "feedback not loaded yet" so + // we keep showing the aggregate counts. Some mock paths return empty + // arrays without errors, and we don't want the popover to flash 0/0/0. + if (!feedback?.checks || feedback.checks.length === 0) { + return { precise: null as CountsView | null, byBucket: null }; + } + const counts = bucketCheckCounts(feedback.checks); + const precise: CountsView = { + passed: counts.passed, + inProgress: counts.inProgress, + failed: counts.failed, + }; + const groups = groupChecksByWorkflow(feedback.checks); + const byBucket: Record = { + passed: groups.filter((g) => g.bucket === "passed"), + in_progress: groups.filter((g) => g.bucket === "in_progress"), + failed: groups.filter((g) => g.bucket === "failed"), + }; + return { precise, byBucket }; + }, [feedback]); + + const counts = precise ?? aggregateCounts; + // "No checks at all": the lazy fetch has settled (not isFetching) and there + // are no checks anywhere — neither in PRFeedback nor in the aggregate. + const noChecksAtAll = + !isFetching && pr.checks_total === 0 && (!feedback || (feedback.checks?.length ?? 0) === 0); + if (noChecksAtAll) { + return ( +
+
+ No checks have started +
+
+ ); + } + return ( +
+ + {CHECK_GROUP_ORDER.map((kind) => { + const value = countForBucket(counts, kind); + return ( + + ); + })} +
+ ); +} + +function PRReviewRow({ pr }: { pr: TaskPR }) { + const required = pr.required_reviews ?? null; + const approved = pr.review_count; + const requested = pr.pending_review_count; + + let label: string; + let icon: ReactNode; + if (pr.review_state === "approved") { + icon = ; + label = "Approved"; + } else if (pr.review_state === "changes_requested") { + icon = ; + label = "Changes requested"; + } else { + // Default branch covers "no reviews yet", "review pending", and any + // unknown state. Always rendered (no early-return) so a fresh PR + // still surfaces its review status. + icon = ; + label = "Awaiting review"; + } + // Counts live on the right edge, mirroring the check-group rows. + // Use "K / M" when a required-minimum is known, otherwise just the + // approved count. The "(K requested)" suffix slides in when reviewers + // have been pinged but haven't submitted a review. + let countText: string; + if (required != null) countText = `${approved} / ${required}`; + else countText = `${approved}`; + if (requested > 0) countText += ` · ${requested} requested`; + return ( +
+
+ {icon} + {label} +
+ {countText} +
+ ); +} + +function PRCommentsRow({ pr }: { pr: TaskPR }) { + if (!pr.unresolved_review_threads || pr.unresolved_review_threads <= 0) return null; + return ( +
+ + + {pr.unresolved_review_threads} unresolved comment + {pr.unresolved_review_threads === 1 ? "" : "s"} + +
+ ); +} + +function elapsedLabel(elapsed: number | null): string { + if (elapsed == null) return ""; + if (elapsed === 0) return "updated just now"; + return `updated ${formatElapsedShort(elapsed)} ago`; +} + +function PRPopoverFooter({ lastUpdatedAt }: { lastUpdatedAt: number | null }) { + // Capture a "now" snapshot only inside the setInterval callback (an event + // handler — no impurity in render, no setState during commit). When no + // tick has fired yet, fall back to lastUpdatedAt so the rendered elapsed + // is 0 ("just now"). + const [now, setNow] = useState(null); + useEffect(() => { + if (lastUpdatedAt == null) return; + const id = setInterval(() => setNow(Date.now()), 10_000); + return () => clearInterval(id); + }, [lastUpdatedAt]); + const elapsed = + lastUpdatedAt == null + ? null + : Math.max(0, Math.floor(((now ?? lastUpdatedAt) - lastUpdatedAt) / 1000)); + if (lastUpdatedAt == null) return null; + return ( +
+ + {elapsedLabel(elapsed)} + +
+ ); +} + +function formatElapsedShort(seconds: number): string { + if (seconds < 60) return `${seconds}s`; + const minutes = Math.floor(seconds / 60); + if (minutes < 60) return `${minutes}m`; + const hours = Math.floor(minutes / 60); + return `${hours}h`; +} + +function ReconnectGitHubBlock() { + return ( +
+ GitHub authentication lost. + + Reconnect GitHub + +
+ ); +} + +export function PRCIPopover({ + pr, + enabled, + onOpenDetailPanel, +}: { + pr: TaskPR; + enabled: boolean; + onOpenDetailPanel?: () => void; +}) { + const ghStatus = useAppStore((s) => s.githubStatus.status); + const authLost = ghStatus !== null && !ghStatus.authenticated; + // Trigger an initial status load from the same hook the rest of the app uses. + useGitHubStatus(); + const { feedback, isFetching, lastUpdatedAt } = usePRCIPopover(pr, enabled && !authLost); + const onAddAsContext = useAddCheckToContext(pr); + + return ( +
e.stopPropagation()} + > + + {authLost ? ( + + ) : ( + <> + +
+ + +
+ + )} + {onOpenDetailPanel && ( + + )} + +
+ ); +} + +// --- Add-to-context wiring (mirrors pr-detail-panel.tsx for failed checks) --- +function useAddCheckToContext(pr: TaskPR): ((message: string) => void) | null { + const sessionId = useAppStore((s) => s.tasks.activeSessionId); + const addComment = useCommentsStore((s) => s.addComment); + const { toast } = useToast(); + const prNumber = pr.pr_number; + // Always call useCallback (rules-of-hooks) before bailing out, so the + // returned callback identity is stable across renders unless its inputs + // change. Without memoization the popover would create a new function + // every parent render, defeating PRWorkflowRow's cheap reference equality. + const handler = useCallback( + (message: string) => { + if (!sessionId) return; + const comment: PRFeedbackComment = { + id: `pr-feedback-${Date.now()}-${Math.random().toString(36).slice(2, 7)}`, + sessionId, + text: message, + createdAt: new Date().toISOString(), + status: "pending", + source: "pr-feedback", + prNumber, + feedbackType: "check", + content: message, + }; + addComment(comment); + toast({ description: "Added to chat context" }); + }, + [sessionId, prNumber, addComment, toast], + ); + return sessionId ? handler : null; +} + +// Multi-PR aggregate popover deferred — multi-PR tasks currently keep the +// existing DropdownMenu trigger (no hover popover). The aggregate component +// will land alongside that hover variant when it ships, see follow-up. diff --git a/apps/web/components/github/pr-status-chip.tsx b/apps/web/components/github/pr-status-chip.tsx new file mode 100644 index 0000000000..bf5403b716 --- /dev/null +++ b/apps/web/components/github/pr-status-chip.tsx @@ -0,0 +1,118 @@ +"use client"; + +import { useRef } from "react"; +import { + IconCircleCheckFilled, + IconCircleXFilled, + IconChecklist, + IconLoader2, + IconPointFilled, +} from "@tabler/icons-react"; +import { HoverCard, HoverCardContent, HoverCardTrigger } from "@kandev/ui/hover-card"; +import { useTaskPR } from "@/hooks/domains/github/use-task-pr"; +import { usePRFeedbackBackgroundSync } from "@/hooks/domains/github/use-pr-ci-popover"; +import { PRCIPopover } from "@/components/github/pr-ci-popover"; +import { isPRReadyToMerge } from "@/components/github/pr-task-icon"; +import type { TaskPR } from "@/lib/types/github"; + +const HOVER_OPEN_DELAY_MS = 150; +const HOVER_CLOSE_DELAY_MS = 150; + +type ChipStatus = "passed" | "failed" | "in_progress" | "merged" | "closed" | "neutral"; + +function chipStatus(pr: TaskPR): ChipStatus { + if (pr.state === "merged") return "merged"; + if (pr.state === "closed") return "closed"; + if (pr.review_state === "changes_requested" || pr.checks_state === "failure") return "failed"; + // Pending checks / pending review must beat checks_state === "success" so a + // PR with all checks green but reviewers still outstanding renders as + // in-progress, not passed. Without this order, the chip flips to green the + // moment CI finishes and ignores the human gate. + if (pr.checks_state === "pending" || pr.review_state === "pending") return "in_progress"; + if (pr.checks_state === "success") return "passed"; + return "neutral"; +} + +/** + * Compact CI indicator for the chat status bar — a "CI" prefix icon plus a + * status glyph that mirrors the popover's bucket colors: + * passed → green check + * failed → red X + * in progress → yellow spinner + * merged → purple dot + * neutral → muted dot + * + * Hovering opens the full PRCIPopover anchored to the top edge so the card + * expands upward (the chip sits just above the chat input). + * + * Returns null when the task has no PR yet. + */ +export function PRStatusChip({ taskId }: { taskId: string | null }) { + const { pr } = useTaskPR(taskId); + // Subscribe at the chip level so the cache warms even when the + // top-bar PR button isn't mounted (e.g. small viewport that hides it). + usePRFeedbackBackgroundSync(pr); + if (!pr) return null; + return ; +} + +function PRStatusChipInner({ pr }: { pr: TaskPR }) { + const status = chipStatus(pr); + const triggerRef = useRef(null); + return ( + + + + + { + // Radix HoverCard treats the trigger as outside the content's + // bounding box, so a click on the chip would auto-close the + // popover. Filter out trigger clicks so clicking the chip is + // a no-op while the popover stays open via hover. + if (triggerRef.current && triggerRef.current.contains(e.target as Node)) { + e.preventDefault(); + } + }} + > + + + + ); +} + +function ChipStatusGlyph({ status }: { status: ChipStatus }) { + switch (status) { + case "passed": + return