feat(github): move PR monitoring to backend with lightweight polling (#502)

This commit is contained in:
Carlos Florêncio
2026-03-30 23:26:01 +01:00
committed by GitHub
parent 59ee830d71
commit c56dfe8537
28 changed files with 1174 additions and 179 deletions
+4
View File
@@ -48,6 +48,10 @@ type Client interface {
// GetPRFeedback fetches aggregated feedback (reviews, comments, checks) for a PR.
GetPRFeedback(ctx context.Context, owner, repo string, number int) (*PRFeedback, error)
// GetPRStatus fetches lightweight PR status (state, review summary, checks summary).
// Unlike GetPRFeedback, it skips comments for efficiency.
GetPRStatus(ctx context.Context, owner, repo string, number int) (*PRStatus, error)
// ListPRFiles lists files changed in a pull request.
ListPRFiles(ctx context.Context, owner, repo string, number int) ([]PRFile, error)
@@ -67,6 +67,37 @@ func getPRFeedback(ctx context.Context, c Client, owner, repo string, number int
}, nil
}
// getPRStatus fetches lightweight PR status using any Client implementation.
// Unlike getPRFeedback, it skips comments to reduce API calls.
func getPRStatus(ctx context.Context, c Client, owner, repo string, number int) (*PRStatus, error) {
pr, err := c.GetPR(ctx, owner, repo, number)
if err != nil {
return nil, err
}
reviews, err := c.ListPRReviews(ctx, owner, repo, number)
if err != nil {
return nil, err
}
checks, err := c.ListCheckRuns(ctx, owner, repo, pr.HeadSHA)
if err != nil {
return nil, err
}
if reviews == nil {
reviews = []PRReview{}
}
if checks == nil {
checks = []CheckRun{}
}
reviewState, pendingReviewCount := deriveReviewSyncState(pr, reviews)
return &PRStatus{
PR: pr,
ReviewState: reviewState,
ChecksState: computeOverallCheckStatus(checks),
ReviewCount: len(reviews),
PendingReviewCount: pendingReviewCount,
}, nil
}
// convertRawCheckRuns converts raw ghCheckRun structs into the domain CheckRun type.
func convertRawCheckRuns(raw []ghCheckRun) []CheckRun {
checks := make([]CheckRun, len(raw))
@@ -66,6 +66,9 @@ func (s *stubClient) SubmitReview(context.Context, string, string, int, string,
func (s *stubClient) ListRepoBranches(context.Context, string, string) ([]RepoBranch, error) {
return nil, nil
}
func (s *stubClient) GetPRStatus(context.Context, string, string, int) (*PRStatus, error) {
return nil, nil
}
func newControllerTestLogger() *logger.Logger {
log, _ := logger.NewLogger(logger.LoggingConfig{
@@ -359,6 +359,10 @@ func (c *GHClient) GetPRFeedback(ctx context.Context, owner, repo string, number
return getPRFeedback(ctx, c, owner, repo, number)
}
func (c *GHClient) GetPRStatus(ctx context.Context, owner, repo string, number int) (*PRStatus, error) {
return getPRStatus(ctx, c, owner, repo, number)
}
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),
+30 -14
View File
@@ -45,6 +45,7 @@ func registerWSHandlers(dispatcher *ws.Dispatcher, svc *Service, log *logger.Log
dispatcher.RegisterFunc(ws.ActionGitHubPRWatchDelete, wsDeletePRWatch(svc, log))
dispatcher.RegisterFunc(ws.ActionGitHubPRFilesGet, wsGetPRFiles(svc, log))
dispatcher.RegisterFunc(ws.ActionGitHubPRCommitsGet, wsGetPRCommits(svc, log))
dispatcher.RegisterFunc(ws.ActionGitHubTaskPRSync, wsSyncTaskPR(svc, log))
dispatcher.RegisterFunc(ws.ActionGitHubStats, wsGetStats(svc, log))
}
@@ -59,6 +60,26 @@ func parseMap(msg *ws.Message) (map[string]interface{}, error) {
return m, err
}
// wsWithField returns a WS handler that parses a single named string field from
// the payload and passes it to serviceFn, returning the result as the response.
func wsWithField(field string, serviceFn func(ctx context.Context, val string) (interface{}, error)) func(ctx context.Context, msg *ws.Message) (*ws.Message, error) {
return func(ctx context.Context, msg *ws.Message) (*ws.Message, error) {
payload, parseErr := parseMap(msg)
if parseErr != nil {
return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeBadRequest, "invalid payload: "+parseErr.Error(), nil)
}
val, _ := payload[field].(string)
if val == "" {
return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeBadRequest, field+" required", nil)
}
result, err := serviceFn(ctx, val)
if err != nil {
return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeInternalError, err.Error(), nil)
}
return ws.NewResponse(msg.ID, msg.Action, result)
}
}
// wsDeleteByID returns a WS handler that parses an "id" field from the payload
// and calls deleteFn. Used by both wsDeletePRWatch and wsDeleteReviewWatch.
func wsDeleteByID(deleteFn func(ctx context.Context, id string) error) func(ctx context.Context, msg *ws.Message) (*ws.Message, error) {
@@ -129,6 +150,12 @@ func wsGetTaskPR(svc *Service, _ *logger.Logger) func(ctx context.Context, msg *
}
}
func wsSyncTaskPR(svc *Service, _ *logger.Logger) func(ctx context.Context, msg *ws.Message) (*ws.Message, error) {
return wsWithField("task_id", func(ctx context.Context, taskID string) (interface{}, error) {
return svc.TriggerPRSync(ctx, taskID)
})
}
func wsGetPRFeedback(svc *Service, _ *logger.Logger) func(ctx context.Context, msg *ws.Message) (*ws.Message, error) {
return func(ctx context.Context, msg *ws.Message) (*ws.Message, error) {
payload, parseErr := parseMap(msg)
@@ -151,21 +178,10 @@ func wsGetPRFeedback(svc *Service, _ *logger.Logger) func(ctx context.Context, m
}
func wsListReviewWatches(svc *Service, _ *logger.Logger) func(ctx context.Context, msg *ws.Message) (*ws.Message, error) {
return func(ctx context.Context, msg *ws.Message) (*ws.Message, error) {
payload, parseErr := parseMap(msg)
if parseErr != nil {
return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeBadRequest, "invalid payload: "+parseErr.Error(), nil)
}
workspaceID, _ := payload["workspace_id"].(string)
if workspaceID == "" {
return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeBadRequest, "workspace_id required", nil)
}
return wsWithField("workspace_id", func(ctx context.Context, workspaceID string) (interface{}, error) {
watches, err := svc.ListReviewWatches(ctx, workspaceID)
if err != nil {
return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeInternalError, err.Error(), nil)
}
return ws.NewResponse(msg.ID, msg.Action, watches)
}
return watches, err
})
}
func wsCreateReviewWatch(svc *Service, _ *logger.Logger) func(ctx context.Context, msg *ws.Message) (*ws.Message, error) {
@@ -190,6 +190,10 @@ func (m *MockClient) GetPRFeedback(ctx context.Context, owner, repo string, numb
return getPRFeedback(ctx, m, owner, repo, number)
}
func (m *MockClient) GetPRStatus(ctx context.Context, owner, repo string, number int) (*PRStatus, error) {
return getPRStatus(ctx, m, owner, repo, number)
}
func (m *MockClient) ListPRFiles(_ context.Context, owner, repo string, number int) ([]PRFile, error) {
m.mu.RLock()
defer m.mu.RUnlock()
+11 -2
View File
@@ -82,6 +82,16 @@ type PRFeedback struct {
HasIssues bool `json:"has_issues"`
}
// PRStatus contains lightweight PR state used by the background poller.
// Unlike PRFeedback, it skips comments to reduce API calls.
type PRStatus struct {
PR *PR `json:"pr"`
ReviewState string `json:"review_state"` // "approved", "changes_requested", "pending", ""
ChecksState string `json:"checks_state"` // "success", "failure", "pending", ""
ReviewCount int `json:"review_count"`
PendingReviewCount int `json:"pending_review_count"`
}
// PRWatch tracks active PR monitoring (session → PR).
type PRWatch struct {
ID string `json:"id" db:"id"`
@@ -94,6 +104,7 @@ type PRWatch struct {
LastCheckedAt *time.Time `json:"last_checked_at,omitempty" db:"last_checked_at"`
LastCommentAt *time.Time `json:"last_comment_at,omitempty" db:"last_comment_at"`
LastCheckStatus string `json:"last_check_status" db:"last_check_status"`
LastReviewState string `json:"last_review_state" db:"last_review_state"`
CreatedAt time.Time `json:"created_at" db:"created_at"`
UpdatedAt time.Time `json:"updated_at" db:"updated_at"`
}
@@ -243,8 +254,6 @@ type PRFeedbackEvent struct {
PRNumber int `json:"pr_number"`
Owner string `json:"owner"`
Repo string `json:"repo"`
NewComments int `json:"new_comments"`
ChecksChanged bool `json:"checks_changed"`
NewCheckStatus string `json:"new_check_status"`
NewReviewState string `json:"new_review_state"`
}
@@ -61,6 +61,10 @@ func (c *NoopClient) GetPRFeedback(context.Context, string, string, int) (*PRFee
return nil, ErrNoClient
}
func (c *NoopClient) GetPRStatus(context.Context, string, string, int) (*PRStatus, error) {
return nil, ErrNoClient
}
func (c *NoopClient) ListPRFiles(context.Context, string, string, int) ([]PRFile, error) {
return nil, ErrNoClient
}
@@ -256,6 +256,10 @@ func (c *PATClient) GetPRFeedback(ctx context.Context, owner, repo string, numbe
return getPRFeedback(ctx, c, owner, repo, number)
}
func (c *PATClient) GetPRStatus(ctx context.Context, owner, repo string, number int) (*PRStatus, error) {
return getPRStatus(ctx, c, owner, repo, number)
}
func (c *PATClient) ListPRFiles(ctx context.Context, owner, repo string, number int) ([]PRFile, error) {
var raw []ghPRFile
endpoint := fmt.Sprintf("/repos/%s/%s/pulls/%d/files?per_page=100", owner, repo, number)
+63 -21
View File
@@ -13,15 +13,30 @@ import (
)
const (
defaultPRPollInterval = 1 * time.Minute
defaultPRPollInterval = 30 * time.Second
defaultReviewPollInterval = 5 * time.Minute
)
// TaskBranchInfo describes a task+session that may need a PR watch.
type TaskBranchInfo struct {
TaskID string
SessionID string
Owner string
Repo string
Branch string
}
// TaskBranchProvider lists tasks that should have PR watches.
type TaskBranchProvider interface {
ListTasksNeedingPRWatch(ctx context.Context) ([]TaskBranchInfo, error)
}
// Poller runs background loops for PR monitoring and review queue checking.
type Poller struct {
service *Service
eventBus bus.EventBus
logger *logger.Logger
service *Service
eventBus bus.EventBus
logger *logger.Logger
taskBranchProvider TaskBranchProvider
cancel context.CancelFunc
wg sync.WaitGroup
@@ -87,6 +102,8 @@ func (p *Poller) prMonitorLoop(ctx context.Context) {
}
func (p *Poller) checkPRWatches(ctx context.Context) {
p.reconcileWatches(ctx)
watches, err := p.service.ListActivePRWatches(ctx)
if err != nil {
p.logger.Error("failed to list PR watches", zap.Error(err))
@@ -104,25 +121,33 @@ func (p *Poller) checkSinglePRWatch(ctx context.Context, watch *PRWatch) {
return
}
feedback, hasNew, err := p.service.CheckPRWatch(ctx, watch)
status, hasNew, err := p.service.CheckPRWatch(ctx, watch)
if err != nil {
p.logger.Debug("failed to check PR watch",
zap.String("id", watch.ID), zap.Error(err))
return
}
if feedback == nil {
if status == nil {
return
}
// Always sync latest PR state to the task-PR record.
if syncErr := p.service.SyncTaskPR(ctx, watch.TaskID, status); syncErr != nil {
p.logger.Error("failed to sync task PR",
zap.String("task_id", watch.TaskID), zap.Error(syncErr))
return // Keep watch so the next cycle can retry
}
// Auto-cleanup: remove watch when PR is merged or closed.
if feedback.PR != nil && (feedback.PR.State == prStateMerged || feedback.PR.State == prStateClosed) {
if status.PR != nil && (status.PR.State == prStateMerged || status.PR.State == prStateClosed) {
p.publishPRStatusEvent(ctx, watch, status)
if delErr := p.service.DeletePRWatch(ctx, watch.ID); delErr != nil {
p.logger.Error("failed to delete completed PR watch",
zap.String("id", watch.ID), zap.Error(delErr))
} else {
p.logger.Info("removed PR watch for completed PR",
zap.String("id", watch.ID),
zap.String("state", feedback.PR.State),
zap.String("state", status.PR.State),
zap.Int("pr_number", watch.PRNumber))
}
return
@@ -132,13 +157,7 @@ func (p *Poller) checkSinglePRWatch(ctx context.Context, watch *PRWatch) {
return
}
// Sync task-PR record with latest data
if syncErr := p.service.SyncTaskPR(ctx, watch.TaskID, feedback); syncErr != nil {
p.logger.Error("failed to sync task PR", zap.String("task_id", watch.TaskID), zap.Error(syncErr))
}
// Publish feedback event for UI notification
p.publishPRFeedbackEvent(ctx, watch, feedback)
p.publishPRStatusEvent(ctx, watch, status)
}
// detectPRForWatch searches GitHub for a PR on the watch's branch.
@@ -159,7 +178,7 @@ func (p *Poller) detectPRForWatch(ctx context.Context, watch *PRWatch) {
// Update last_checked_at regardless of result
now := time.Now().UTC()
_ = p.service.store.UpdatePRWatchTimestamps(ctx, watch.ID, now, nil, "")
_ = p.service.store.UpdatePRWatchTimestamps(ctx, watch.ID, now, nil, "", "")
if pr == nil {
return
@@ -188,7 +207,7 @@ func (p *Poller) detectPRForWatch(ctx context.Context, watch *PRWatch) {
zap.Int("pr_number", pr.Number))
}
func (p *Poller) publishPRFeedbackEvent(ctx context.Context, watch *PRWatch, feedback *PRFeedback) {
func (p *Poller) publishPRStatusEvent(ctx context.Context, watch *PRWatch, status *PRStatus) {
if p.eventBus == nil {
return
}
@@ -198,10 +217,8 @@ func (p *Poller) publishPRFeedbackEvent(ctx context.Context, watch *PRWatch, fee
PRNumber: watch.PRNumber,
Owner: watch.Owner,
Repo: watch.Repo,
NewComments: len(feedback.Comments),
ChecksChanged: true,
NewCheckStatus: computeOverallCheckStatus(feedback.Checks),
NewReviewState: computeOverallReviewState(feedback.Reviews),
NewCheckStatus: status.ChecksState,
NewReviewState: status.ReviewState,
}
event := bus.NewEvent(events.GitHubPRFeedback, "github_poller", evt)
if err := p.eventBus.Publish(ctx, events.GitHubPRFeedback, event); err != nil {
@@ -209,6 +226,31 @@ func (p *Poller) publishPRFeedbackEvent(ctx context.Context, watch *PRWatch, fee
}
}
// SetTaskBranchProvider sets the provider used for watch reconciliation.
func (p *Poller) SetTaskBranchProvider(provider TaskBranchProvider) {
p.taskBranchProvider = provider
}
// reconcileWatches ensures PR watches exist for all tasks that need them.
func (p *Poller) reconcileWatches(ctx context.Context) {
if p.taskBranchProvider == nil {
return
}
tasks, err := p.taskBranchProvider.ListTasksNeedingPRWatch(ctx)
if err != nil {
p.logger.Error("failed to list tasks needing PR watch", zap.Error(err))
return
}
for _, task := range tasks {
if _, ensureErr := p.service.EnsurePRWatch(
ctx, task.SessionID, task.TaskID, task.Owner, task.Repo, task.Branch,
); ensureErr != nil {
p.logger.Error("failed to ensure PR watch",
zap.String("session_id", task.SessionID), zap.Error(ensureErr))
}
}
}
// reviewQueueLoop polls review watches for new PRs.
func (p *Poller) reviewQueueLoop(ctx context.Context) {
defer p.wg.Done()
+361
View File
@@ -0,0 +1,361 @@
package github
import (
"context"
"database/sql"
"testing"
"time"
"github.com/jmoiron/sqlx"
_ "github.com/mattn/go-sqlite3"
"github.com/kandev/kandev/internal/common/logger"
"github.com/kandev/kandev/internal/events/bus"
)
// setupPollerTest creates a Poller backed by an in-memory SQLite store and MockClient.
func setupPollerTest(t *testing.T) (*Poller, *Service, *MockClient, *Store) {
t.Helper()
rawDB, err := sql.Open("sqlite3", ":memory:")
if err != nil {
t.Fatalf("open sqlite: %v", err)
}
rawDB.SetMaxOpenConns(1)
rawDB.SetMaxIdleConns(1)
sqlxDB := sqlx.NewDb(rawDB, "sqlite3")
t.Cleanup(func() { _ = sqlxDB.Close() })
store, err := NewStore(sqlxDB, sqlxDB)
if err != nil {
t.Fatalf("new store: %v", err)
}
mockClient := NewMockClient()
log, _ := logger.NewLogger(logger.LoggingConfig{Level: "error", Format: "console"})
eventBus := bus.NewMemoryEventBus(log)
svc := NewService(mockClient, "pat", nil, store, eventBus, log)
poller := NewPoller(svc, eventBus, log)
return poller, svc, mockClient, store
}
func TestCheckSinglePRWatch_MergedPR_SyncsThenDeletes(t *testing.T) {
poller, _, mockClient, store := setupPollerTest(t)
ctx := context.Background()
// Set up a merged PR in the mock client.
now := time.Now().UTC()
mergedAt := now.Add(-1 * time.Hour)
mockClient.AddPR(&PR{
Number: 42,
Title: "Feature PR",
State: prStateMerged,
HeadSHA: "abc123",
HeadBranch: "feature-branch",
RepoOwner: "owner",
RepoName: "repo",
MergedAt: &mergedAt,
})
// Create a PRWatch in the DB.
watch := &PRWatch{
SessionID: "sess-1",
TaskID: "task-1",
Owner: "owner",
Repo: "repo",
PRNumber: 42,
Branch: "feature-branch",
}
if err := store.CreatePRWatch(ctx, watch); err != nil {
t.Fatalf("create PR watch: %v", err)
}
// Create a TaskPR record so SyncTaskPR has something to update.
taskPR := &TaskPR{
TaskID: "task-1",
Owner: "owner",
Repo: "repo",
PRNumber: 42,
PRURL: "https://github.com/owner/repo/pull/42",
PRTitle: "Feature PR",
HeadBranch: "feature-branch",
BaseBranch: "main",
State: "open",
}
if err := store.CreateTaskPR(ctx, taskPR); err != nil {
t.Fatalf("create task PR: %v", err)
}
// Act
poller.checkSinglePRWatch(ctx, watch)
// Assert: TaskPR record should be updated with state="merged".
updatedTP, err := store.GetTaskPR(ctx, "task-1")
if err != nil {
t.Fatalf("get task PR: %v", err)
}
if updatedTP == nil {
t.Fatal("expected task PR to exist after sync")
}
if updatedTP.State != prStateMerged {
t.Errorf("expected task PR state=%q, got %q", prStateMerged, updatedTP.State)
}
if updatedTP.MergedAt == nil {
t.Error("expected task PR MergedAt to be set")
}
// Assert: PRWatch should be deleted.
remainingWatch, err := store.GetPRWatchBySession(ctx, "sess-1")
if err != nil {
t.Fatalf("get PR watch: %v", err)
}
if remainingWatch != nil {
t.Error("expected PR watch to be deleted after merged PR")
}
}
func TestCheckSinglePRWatch_OpenPR_SyncsOnChange(t *testing.T) {
poller, _, mockClient, store := setupPollerTest(t)
ctx := context.Background()
// Set up an open PR with a passing check.
mockClient.AddPR(&PR{
Number: 10,
Title: "Open PR",
State: "open",
HeadSHA: "def456",
HeadBranch: "open-branch",
RepoOwner: "owner",
RepoName: "repo",
Additions: 5,
Deletions: 3,
})
mockClient.AddCheckRuns("owner", "repo", "def456", []CheckRun{
{Name: "ci", Status: "completed", Conclusion: "success"},
})
// Create a PRWatch with a different last_check_status to trigger hasNew.
watch := &PRWatch{
SessionID: "sess-2",
TaskID: "task-2",
Owner: "owner",
Repo: "repo",
PRNumber: 10,
Branch: "open-branch",
LastCheckStatus: "pending", // different from "success" -> hasNew=true
}
if err := store.CreatePRWatch(ctx, watch); err != nil {
t.Fatalf("create PR watch: %v", err)
}
// Create a TaskPR record.
taskPR := &TaskPR{
TaskID: "task-2",
Owner: "owner",
Repo: "repo",
PRNumber: 10,
PRURL: "https://github.com/owner/repo/pull/10",
PRTitle: "Open PR",
HeadBranch: "open-branch",
BaseBranch: "main",
State: "open",
}
if err := store.CreateTaskPR(ctx, taskPR); err != nil {
t.Fatalf("create task PR: %v", err)
}
// Act
poller.checkSinglePRWatch(ctx, watch)
// Assert: TaskPR should be synced with latest data.
updatedTP, err := store.GetTaskPR(ctx, "task-2")
if err != nil {
t.Fatalf("get task PR: %v", err)
}
if updatedTP == nil {
t.Fatal("expected task PR to exist")
}
if updatedTP.State != "open" {
t.Errorf("expected state=open, got %q", updatedTP.State)
}
if updatedTP.ChecksState != "success" {
t.Errorf("expected checks_state=success, got %q", updatedTP.ChecksState)
}
if updatedTP.Additions != 5 {
t.Errorf("expected additions=5, got %d", updatedTP.Additions)
}
// Assert: PRWatch should NOT be deleted (PR is still open).
remainingWatch, err := store.GetPRWatchBySession(ctx, "sess-2")
if err != nil {
t.Fatalf("get PR watch: %v", err)
}
if remainingWatch == nil {
t.Error("expected PR watch to still exist for open PR")
}
}
// mockTaskBranchProvider implements TaskBranchProvider for testing.
type mockTaskBranchProvider struct {
tasks []TaskBranchInfo
err error
}
func (m *mockTaskBranchProvider) ListTasksNeedingPRWatch(_ context.Context) ([]TaskBranchInfo, error) {
return m.tasks, m.err
}
func TestReconcileWatches_CreatesWatchesForTasks(t *testing.T) {
poller, _, _, store := setupPollerTest(t)
ctx := context.Background()
prov := &mockTaskBranchProvider{
tasks: []TaskBranchInfo{
{TaskID: "t1", SessionID: "s1", Owner: "myorg", Repo: "myrepo", Branch: "feature-a"},
{TaskID: "t2", SessionID: "s2", Owner: "myorg", Repo: "myrepo", Branch: "feature-b"},
},
}
poller.SetTaskBranchProvider(prov)
poller.reconcileWatches(ctx)
// Verify watches were created for both sessions.
w1, err := store.GetPRWatchBySession(ctx, "s1")
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if w1 == nil {
t.Fatal("expected PR watch for session s1")
}
if w1.Branch != "feature-a" {
t.Errorf("expected branch %q, got %q", "feature-a", w1.Branch)
}
if w1.TaskID != "t1" {
t.Errorf("expected task_id %q, got %q", "t1", w1.TaskID)
}
w2, err := store.GetPRWatchBySession(ctx, "s2")
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if w2 == nil {
t.Fatal("expected PR watch for session s2")
}
if w2.Branch != "feature-b" {
t.Errorf("expected branch %q, got %q", "feature-b", w2.Branch)
}
}
func TestReconcileWatches_NilProvider(t *testing.T) {
poller, _, _, _ := setupPollerTest(t)
ctx := context.Background()
// Should not panic when provider is nil.
poller.reconcileWatches(ctx)
}
func TestReconcileWatches_SkipsExistingWatches(t *testing.T) {
poller, _, _, store := setupPollerTest(t)
ctx := context.Background()
// Pre-create a watch for s1.
existing := &PRWatch{
SessionID: "s1",
TaskID: "t1",
Owner: "myorg",
Repo: "myrepo",
PRNumber: 0,
Branch: "feature-a",
}
if err := store.CreatePRWatch(ctx, existing); err != nil {
t.Fatalf("create PR watch: %v", err)
}
prov := &mockTaskBranchProvider{
tasks: []TaskBranchInfo{
{TaskID: "t1", SessionID: "s1", Owner: "myorg", Repo: "myrepo", Branch: "feature-a"},
{TaskID: "t2", SessionID: "s2", Owner: "myorg", Repo: "myrepo", Branch: "feature-b"},
},
}
poller.SetTaskBranchProvider(prov)
poller.reconcileWatches(ctx)
// s1 should still have its original watch (EnsurePRWatch is idempotent).
w1, _ := store.GetPRWatchBySession(ctx, "s1")
if w1 == nil {
t.Fatal("expected PR watch for session s1")
}
if w1.ID != existing.ID {
t.Errorf("expected original watch ID %q, got %q", existing.ID, w1.ID)
}
// s2 should have a new watch.
w2, _ := store.GetPRWatchBySession(ctx, "s2")
if w2 == nil {
t.Fatal("expected PR watch for session s2")
}
}
func TestCheckSinglePRWatch_OpenPR_NoChange_NoSync(t *testing.T) {
poller, _, mockClient, store := setupPollerTest(t)
ctx := context.Background()
// Set up an open PR with a passing check.
mockClient.AddPR(&PR{
Number: 20,
Title: "Stable PR",
State: "open",
HeadSHA: "ghi789",
HeadBranch: "stable-branch",
RepoOwner: "owner",
RepoName: "repo",
})
mockClient.AddCheckRuns("owner", "repo", "ghi789", []CheckRun{
{Name: "ci", Status: "completed", Conclusion: "success"},
})
// Create a PRWatch with matching last_check_status -> no change.
watch := &PRWatch{
SessionID: "sess-3",
TaskID: "task-3",
Owner: "owner",
Repo: "repo",
PRNumber: 20,
Branch: "stable-branch",
LastCheckStatus: "success", // same -> hasNew=false
}
if err := store.CreatePRWatch(ctx, watch); err != nil {
t.Fatalf("create PR watch: %v", err)
}
// Create a TaskPR record.
taskPR := &TaskPR{
TaskID: "task-3",
Owner: "owner",
Repo: "repo",
PRNumber: 20,
PRURL: "https://github.com/owner/repo/pull/20",
PRTitle: "Stable PR",
HeadBranch: "stable-branch",
BaseBranch: "main",
State: "open",
}
if err := store.CreateTaskPR(ctx, taskPR); err != nil {
t.Fatalf("create task PR: %v", err)
}
// Act
poller.checkSinglePRWatch(ctx, watch)
// Assert: PRWatch should NOT be deleted.
remainingWatch, err := store.GetPRWatchBySession(ctx, "sess-3")
if err != nil {
t.Fatalf("get PR watch: %v", err)
}
if remainingWatch == nil {
t.Error("expected PR watch to still exist")
}
}
+82 -26
View File
@@ -213,37 +213,33 @@ func (s *Service) UpdatePRWatchPRNumber(ctx context.Context, id string, prNumber
return s.store.UpdatePRWatchPRNumber(ctx, id, prNumber)
}
// CheckPRWatch fetches latest feedback for a PR watch and determines if there are changes.
func (s *Service) CheckPRWatch(ctx context.Context, watch *PRWatch) (*PRFeedback, bool, error) {
// CheckPRWatch fetches lightweight PR status for a watch and determines if there are changes.
func (s *Service) CheckPRWatch(ctx context.Context, watch *PRWatch) (*PRStatus, bool, error) {
if s.client == nil {
return nil, false, fmt.Errorf("github client not available")
}
feedback, err := s.client.GetPRFeedback(ctx, watch.Owner, watch.Repo, watch.PRNumber)
status, err := s.client.GetPRStatus(ctx, watch.Owner, watch.Repo, watch.PRNumber)
if err != nil {
return nil, false, err
}
hasNew := false
// Check for new comments
latestCommentAt := findLatestCommentTime(feedback.Comments)
if latestCommentAt != nil && (watch.LastCommentAt == nil || latestCommentAt.After(*watch.LastCommentAt)) {
// Check for check status or review state changes
if status.ChecksState != watch.LastCheckStatus {
hasNew = true
}
// Check for check status changes
checkStatus := computeOverallCheckStatus(feedback.Checks)
if checkStatus != watch.LastCheckStatus {
if status.ReviewState != watch.LastReviewState {
hasNew = true
}
// Update watch timestamps
now := time.Now().UTC()
if err := s.store.UpdatePRWatchTimestamps(ctx, watch.ID, now, latestCommentAt, checkStatus); err != nil {
if err := s.store.UpdatePRWatchTimestamps(ctx, watch.ID, now, nil, status.ChecksState, status.ReviewState); err != nil {
s.logger.Error("failed to update PR watch timestamps", zap.String("id", watch.ID), zap.Error(err))
}
return feedback, hasNew, nil
return status, hasNew, nil
}
// EnsurePRWatch creates a PRWatch with pr_number=0 for a session if one doesn't already exist.
@@ -403,25 +399,27 @@ func (s *Service) ListTaskPRs(ctx context.Context, taskIDs []string) (map[string
return s.store.ListTaskPRsByTaskIDs(ctx, taskIDs)
}
// SyncTaskPR updates a TaskPR record with the latest PR data from feedback.
func (s *Service) SyncTaskPR(ctx context.Context, taskID string, feedback *PRFeedback) error {
// SyncTaskPR updates a TaskPR record with the latest PR status.
func (s *Service) SyncTaskPR(ctx context.Context, taskID string, status *PRStatus) error {
if status == nil || status.PR == nil {
return fmt.Errorf("sync task PR: missing PR data for task %s", taskID)
}
tp, err := s.store.GetTaskPR(ctx, taskID)
if err != nil || tp == nil {
return err
}
tp.State = feedback.PR.State
tp.PRTitle = feedback.PR.Title
tp.Additions = feedback.PR.Additions
tp.Deletions = feedback.PR.Deletions
tp.MergedAt = feedback.PR.MergedAt
tp.ClosedAt = feedback.PR.ClosedAt
tp.CommentCount = len(feedback.Comments)
tp.ReviewCount = len(feedback.Reviews)
reviewState, pendingReviewCount := deriveReviewSyncState(feedback.PR, feedback.Reviews)
tp.ReviewState = reviewState
tp.ChecksState = computeOverallCheckStatus(feedback.Checks)
tp.PendingReviewCount = pendingReviewCount
tp.State = status.PR.State
tp.PRTitle = status.PR.Title
tp.Additions = status.PR.Additions
tp.Deletions = status.PR.Deletions
tp.MergedAt = status.PR.MergedAt
tp.ClosedAt = status.PR.ClosedAt
tp.ReviewState = status.ReviewState
tp.ChecksState = status.ChecksState
tp.ReviewCount = status.ReviewCount
tp.PendingReviewCount = status.PendingReviewCount
// CommentCount is no longer updated from polling -- only refreshed on-demand
now := time.Now().UTC()
tp.LastSyncedAt = &now
@@ -456,6 +454,64 @@ func (s *Service) GetPRFeedback(ctx context.Context, owner, repo string, number
return s.client.GetPRFeedback(ctx, owner, repo, number)
}
// TriggerPRSync performs an immediate PR status sync for a task.
// If the watch has a PR number, it fetches the latest status from GitHub
// and syncs it to the TaskPR record. If still searching (pr_number=0),
// it attempts to find the PR by branch.
func (s *Service) TriggerPRSync(ctx context.Context, taskID string) (*TaskPR, error) {
watch, err := s.store.GetPRWatchByTask(ctx, taskID)
if err != nil {
return nil, fmt.Errorf("get PR watch: %w", err)
}
if watch == nil {
// No watch — just return existing TaskPR if any
return s.store.GetTaskPR(ctx, taskID)
}
if watch.PRNumber == 0 {
return s.triggerPRDetection(ctx, watch, taskID)
}
return s.triggerPRStatusSync(ctx, watch, taskID)
}
func (s *Service) triggerPRDetection(ctx context.Context, watch *PRWatch, taskID string) (*TaskPR, error) {
if s.client == nil {
return nil, nil
}
pr, err := s.client.FindPRByBranch(ctx, watch.Owner, watch.Repo, watch.Branch)
if err != nil || pr == nil {
return nil, err
}
if err := s.store.UpdatePRWatchPRNumber(ctx, watch.ID, pr.Number); err != nil {
s.logger.Error("failed to update PR watch number during sync",
zap.String("watch_id", watch.ID), zap.Int("pr_number", pr.Number), zap.Error(err))
return nil, fmt.Errorf("update PR watch: %w", err)
}
if _, assocErr := s.AssociatePRWithTask(ctx, taskID, pr); assocErr != nil {
s.logger.Error("failed to associate PR with task during sync",
zap.String("task_id", taskID), zap.Int("pr_number", pr.Number), zap.Error(assocErr))
return nil, fmt.Errorf("associate PR: %w", assocErr)
}
// Also fetch status so the first response includes review/check state
watch.PRNumber = pr.Number
return s.triggerPRStatusSync(ctx, watch, taskID)
}
func (s *Service) triggerPRStatusSync(ctx context.Context, watch *PRWatch, taskID string) (*TaskPR, error) {
status, _, err := s.CheckPRWatch(ctx, watch)
if err != nil {
return nil, err
}
if status == nil {
return s.store.GetTaskPR(ctx, taskID)
}
if syncErr := s.SyncTaskPR(ctx, taskID, status); syncErr != nil {
return nil, syncErr
}
return s.store.GetTaskPR(ctx, taskID)
}
// --- PR files and commits (live) ---
// GetPRFiles fetches files changed in a PR from GitHub.
@@ -419,3 +419,107 @@ func TestFindLatestCommentTime(t *testing.T) {
})
}
}
func TestTriggerPRSync_SyncsExistingWatch(t *testing.T) {
_, svc, mockClient, store := setupPollerTest(t)
ctx := context.Background()
// Set up a PR in mock client.
mockClient.AddPR(&PR{
Number: 10,
Title: "Feature PR",
State: "open",
HeadSHA: "abc123",
HeadBranch: "feat/x",
RepoOwner: "org",
RepoName: "repo",
})
// Create a PR watch and TaskPR in DB.
watch := &PRWatch{
SessionID: "s1",
TaskID: "t1",
Owner: "org",
Repo: "repo",
PRNumber: 10,
Branch: "feat/x",
}
if err := store.CreatePRWatch(ctx, watch); err != nil {
t.Fatal(err)
}
tp := &TaskPR{
TaskID: "t1",
Owner: "org",
Repo: "repo",
PRNumber: 10,
PRTitle: "Feature PR",
State: "open",
}
if err := store.CreateTaskPR(ctx, tp); err != nil {
t.Fatal(err)
}
// Trigger sync.
result, err := svc.TriggerPRSync(ctx, "t1")
if err != nil {
t.Fatalf("TriggerPRSync: %v", err)
}
if result == nil {
t.Fatal("expected non-nil TaskPR")
}
if result.LastSyncedAt == nil {
t.Error("expected LastSyncedAt to be set after sync")
}
}
func TestTriggerPRSync_DetectsPR(t *testing.T) {
_, svc, mockClient, store := setupPollerTest(t)
ctx := context.Background()
// Set up a PR findable by branch.
mockClient.AddPR(&PR{
Number: 20,
Title: "New PR",
State: "open",
HeadBranch: "feat/y",
RepoOwner: "org",
RepoName: "repo",
})
// Create a watch with pr_number=0 (still searching).
watch := &PRWatch{
SessionID: "s2",
TaskID: "t2",
Owner: "org",
Repo: "repo",
PRNumber: 0,
Branch: "feat/y",
}
if err := store.CreatePRWatch(ctx, watch); err != nil {
t.Fatal(err)
}
result, err := svc.TriggerPRSync(ctx, "t2")
if err != nil {
t.Fatalf("TriggerPRSync: %v", err)
}
if result == nil {
t.Fatal("expected TaskPR after detection")
}
if result.PRNumber != 20 {
t.Errorf("expected PR #20, got #%d", result.PRNumber)
}
}
func TestTriggerPRSync_NoWatch(t *testing.T) {
_, svc, _, _ := setupPollerTest(t)
ctx := context.Background()
result, err := svc.TriggerPRSync(ctx, "nonexistent")
if err != nil {
t.Fatalf("TriggerPRSync: %v", err)
}
if result != nil {
t.Errorf("expected nil TaskPR for task without watch, got %+v", result)
}
}
+21 -6
View File
@@ -40,6 +40,7 @@ const createTablesSQL = `
last_checked_at DATETIME,
last_comment_at DATETIME,
last_check_status TEXT DEFAULT '',
last_review_state TEXT DEFAULT '',
created_at DATETIME NOT NULL,
updated_at DATETIME NOT NULL
);
@@ -103,8 +104,12 @@ const createTablesSQL = `
`
func (s *Store) initSchema() error {
_, err := s.db.Exec(createTablesSQL)
return err
if _, err := s.db.Exec(createTablesSQL); err != nil {
return err
}
// Idempotent migrations for existing databases.
_, _ = s.db.Exec(`ALTER TABLE github_pr_watches ADD COLUMN last_review_state TEXT DEFAULT ''`)
return nil
}
// --- PR Watch operations ---
@@ -134,6 +139,16 @@ func (s *Store) GetPRWatchBySession(ctx context.Context, sessionID string) (*PRW
return &w, err
}
// GetPRWatchByTask returns the PR watch for a task (first match).
func (s *Store) GetPRWatchByTask(ctx context.Context, taskID string) (*PRWatch, error) {
var w PRWatch
err := s.ro.GetContext(ctx, &w, `SELECT * FROM github_pr_watches WHERE task_id = ? ORDER BY updated_at DESC LIMIT 1`, taskID)
if errors.Is(err, sql.ErrNoRows) {
return nil, nil
}
return &w, err
}
// ListActivePRWatches returns all active PR watches.
func (s *Store) ListActivePRWatches(ctx context.Context) ([]*PRWatch, error) {
var watches []*PRWatch
@@ -141,12 +156,12 @@ func (s *Store) ListActivePRWatches(ctx context.Context) ([]*PRWatch, error) {
return watches, err
}
// UpdatePRWatchTimestamps updates the last checked timestamps.
func (s *Store) UpdatePRWatchTimestamps(ctx context.Context, id string, checkedAt time.Time, commentAt *time.Time, checkStatus string) error {
// UpdatePRWatchTimestamps updates the last checked timestamps and status fields.
func (s *Store) UpdatePRWatchTimestamps(ctx context.Context, id string, checkedAt time.Time, commentAt *time.Time, checkStatus, reviewState string) error {
_, err := s.db.ExecContext(ctx, `
UPDATE github_pr_watches SET last_checked_at = ?, last_comment_at = ?, last_check_status = ?, updated_at = ?
UPDATE github_pr_watches SET last_checked_at = ?, last_comment_at = ?, last_check_status = ?, last_review_state = ?, updated_at = ?
WHERE id = ?`,
checkedAt, commentAt, checkStatus, time.Now().UTC(), id)
checkedAt, commentAt, checkStatus, reviewState, time.Now().UTC(), id)
return err
}